[PATCH] Ant task inheritAll bug

[Jason Brittain <ja...@collab.net>]

NOTE: the below bug affects only Ant 1.4 (including the
betas) and above.

While reading through the Ant task source I noticed a
bug.  The wrong variable name had been used for a
Project object.. two were being used, one called "p1"
and another called "project".  The line of code that
was wrong used the wrong variable name -- it used
"project" instead of "p1".

The result was that whenever using the "ant" task to
call a target in another build file with the
inheritAll attribute set to false, all user and
system properties are supposed to be copied and
visible, but actually only the user properties
are copied, leaving the system properties undefined.

Attached is a zip containing the patch to fix it,
plus a test in the form of two build files that
show output so you can be sure that the bug exists
and that the patch fixes the problem.

I've found Ant to be a great tool, and the source
to be generally nicely readable and understandable.
Kudos to all contributors for that.  But, I believe
that this bug happened because of poor naming of
variables combined with the lack of testing patches
(since this is a patch to a patch that was recently
sent in via the mailing list and committed).  If
the variables were more clearly named, though, it
would have been less likely for this bug to ever
have crept in, I think.

Cheers.

-- 
Jason Brittain
<jasonb at collab d0t net>
CollabNet http://www.collab.net

Re: [PATCH] Ant task inheritAll bug

["Craeg K. Strong" <cs...@arielpartners.com>]

Jason Brittain wrote:

> While reading through the Ant task source I noticed a
> bug.  The wrong variable name had been used for a
> Project object.. two were being used, one called "p1"
> and another called "project".  The line of code that
> was wrong used the wrong variable name -- it used
> "project" instead of "p1".
>
> I believe
> that this bug happened because of poor naming of
> variables combined with the lack of testing patches
> (since this is a patch to a patch that was recently
> sent in via the mailing list and committed).  If
> the variables were more clearly named, though, it
> would have been less likely for this bug to ever
> have crept in, I think.

Right on all counts, Jason!  Mea culpa-- I am the one who introduced 
this defect :-(
Your patch looks right to me.

Renaming variables is a bit tough with an established code base, but in 
general
I agree with the principle that we (the ant-dev community) should 
contribute refactorings
(design or implementation cleanups that do not change external 
functionality) as well as
bug fixes and new features.

Having a rich set of unit tests will make this lots easier and less 
risky, and that is another area
where I think we (ant-dev'ers) can contribute more...

You have reminded me to always follow my favorite process, even for 
those "tiny innocuous one line changes"

a) write a test that demonstrates the defect
b) fix the defect
c) run the test and prove that the defect no longer exists
d) submit test and patch

[Hey its never too late for new year's resolutions, right? :-]

Cheers,

--Craeg