Re: ant git commit: Unbreak tests

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Stefan Bodewig
Gintas,

> -      passfile="testpassfile.tmp"%/>
> +      passfile="testpassfile.tmp"/>

the commit that introduced the issue was labeled "Trailing whitespace"
and I trusted to contain exactly that and didn't bother reviewing
it. Obviously it did not.

A single commit that spans a dozen mail messages is something you really
cannot review properly.

Glitches happen, this is not a problem. But if they hide inside a patch
that contains more than 4500 changes lines it becomes impossible to deal
with them.

Given we were planning to cut new releases I now feel very uncomfortable
and compelled to wade through all those 4500 lines as well as the 4500
lines of the other commit with the same message and the merge of the 1.9
branch (and doing the same to the equally big commits on the 1.9.x
branch). This is going to delay the releases by several days, at least.

Stefan


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Jaikiran Pai
On 05/07/18 12:57 AM, Stefan Bodewig wrote:

> Gintas,
>
>> -      passfile="testpassfile.tmp"%/>
>> +      passfile="testpassfile.tmp"/>
> the commit that introduced the issue was labeled "Trailing whitespace"
> and I trusted to contain exactly that and didn't bother reviewing
> it. Obviously it did not.
>
> A single commit that spans a dozen mail messages is something you really
> cannot review properly.
>
> Glitches happen, this is not a problem. But if they hide inside a patch
> that contains more than 4500 changes lines it becomes impossible to deal
> with them.
>
> Given we were planning to cut new releases I now feel very uncomfortable
> and compelled to wade through all those 4500 lines as well as the 4500
> lines of the other commit with the same message and the merge of the 1.9
> branch (and doing the same to the equally big commits on the 1.9.x
> branch). This is going to delay the releases by several days, at least.
>
It was made explicitly clear by committers, PMC members and Ant PMC
chairman that the commits of these nature aren't acceptable. Yet, more
and more such commits have been done over and over again, using these
upstream repos as a personal playground and forcing it on the rest of
the community. At this point, I personally believe that reviewing these
meaningless changes is a waste of time and energy. I'm in favour of
rolling back the entire commit set if that's what it takes.

-Jaikiran

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Stefan Bodewig
On 2018-07-05, Jaikiran Pai wrote:

> I personally believe that reviewing these meaningless changes is a
> waste of time and energy. I'm in favour of rolling back the entire
> commit set if that's what it takes.

+1

although reverting the commits in both branches and merging back the
1.9.x branch is likely going to end in an ugly merge that will need
review as well. Hopefullly a shorter one.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Jaikiran Pai

On 05/07/18 2:42 PM, Stefan Bodewig wrote:

> On 2018-07-05, Jaikiran Pai wrote:
>
>> I personally believe that reviewing these meaningless changes is a
>> waste of time and energy. I'm in favour of rolling back the entire
>> commit set if that's what it takes.
> +1
>
> although reverting the commits in both branches and merging back the
> 1.9.x branch is likely going to end in an ugly merge that will need
> review as well. Hopefullly a shorter one.
Yes, I agree.I plan to attempt this later tonight (around 8 hours from
now) if no one else gets to it before that. Just letting it know here,
so that we don't end up duplicating these efforts.

-Jaikiran

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Stefan Bodewig
On 2018-07-06, Jaikiran Pai wrote:

> On 05/07/18 2:42 PM, Stefan Bodewig wrote:
>> On 2018-07-05, Jaikiran Pai wrote:

>>> I personally believe that reviewing these meaningless changes is a
>>> waste of time and energy. I'm in favour of rolling back the entire
>>> commit set if that's what it takes.

>> +1

>> although reverting the commits in both branches and merging back the
>> 1.9.x branch is likely going to end in an ugly merge that will need
>> review as well. Hopefullly a shorter one.

> Yes, I agree.I plan to attempt this later tonight (around 8 hours from
> now) if no one else gets to it before that. Just letting it know here,
> so that we don't end up duplicating these efforts.

Thank you. I'm unlikely to get there before you do, would have tackled
it tomorrow.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Martijn Kruithof-2
Hello all

Just for your info, please take into consideration for a rollback or not
descision (I am neither happy with the commit, nor happy with a rollback
and therefore am 0 on rollback or not):


I have written a small checker to remove all whitespace only diffs from
the diffs on the URL below.

Significant whitespace changes that may have been lost would be unlikely
(space difference before "/>" , ">" or ")" and around "=" is also ignored)

That would reduce the original "2f64e0b5"  diff to (the extra lines
starting with \ before the diff are added by the checker i wrote)

Removing whitespace only changes from asf.txt
From: Gintas Grigelionis <[hidden email]>
Date: Sun, 1 Jul 2018 13:31:35 +0000 (+0200)
Subject: Trailing whitespace
X-Git-Url:
https://git1-us-west.apache.org/repos/asf?p=ant.git;a=commitdiff_plain;h=2f64e0b5

Trailing whitespace
---

\====
\@ -87,11 +87,11 @@ <li><xsl:value-of select="@name"/></li>
</xsl:for-each> </ul--> <h1><a name="top">JDepend Analysis</a></h1> <p
align="right">Designed for use with <a
href="http://www.clarkware.com/software/JDepend.html">JDepend</a> and <a
href="https://ant.apache.org">Ant</a>.</p> <hr size="2"/> <table
width="100%"><tr><td> <a name="NVsummary"><h2>Summary</h2></a> </td><td
align="right">
\@ -87,11 +87,11 @@ <li><xsl:value-of select="@name"/></li>
</xsl:for-each> </ul--> <h1><a name="top">JDepend Analysis</a></h1> <p
align="right">Designed for use with <a
href="http://www.clarkware.com/software/JDepend.html">JDepend</a> and <a
href="http://jakarta.apache.org">Ant</a>.</p> <hr size="2"/> <table
width="100%"><tr><td> <a name="NVsummary"><h2>Summary</h2></a> </td><td
align="right">
\====
diff --git a/src/etc/jdepend.xsl b/src/etc/jdepend.xsl
index f813297..907ade2 100644
--- a/src/etc/jdepend.xsl
+++ b/src/etc/jdepend.xsl
@@ -87,11 +87,11 @@
          <li><xsl:value-of select="@name"/></li>
      </xsl:for-each>
      </ul-->
-
+
      <h1><a name="top">JDepend Analysis</a></h1>
-    <p align="right">Designed for use with <a
href="http://www.clarkware.com/software/JDepend.html">JDepend</a> and <a
href="http://jakarta.apache.org">Ant</a>.</p>
-    <hr size="2" />
-
+    <p align="right">Designed for use with <a
href="http://www.clarkware.com/software/JDepend.html">JDepend</a> and <a
href="https://ant.apache.org">Ant</a>.</p>
+    <hr size="2"/>
+
      <table width="100%"><tr><td>
      <a name="NVsummary"><h2>Summary</h2></a>
      </td><td align="right">
\====
\@ -57,8 +57,8 @@ <target name="prepare-setup"> <mkdir
dir="${test.dir}/src/org/apache/tools/ant"/> <mkdir
dir="${test.dir}/dest"/> <echo
file="${test.dir}/src/org/apache/tools/ant/DirscannerSetup.java">
<![CDATA[ /* * Licensed to the Apache Software Foundation (ASF) under
one or more * contributor license agreements. See the NOTICE file
distributed with
\@ -57,8 +57,8 @@ <target name="prepare-setup"> <mkdir
dir="${test.dir}/src/org/apache/tools/ant"/> <mkdir
dir="${test.dir}/dest"/> <echo
file="${test.dir}/src/org/apache/tools/ant/DirscannerSetup.java"><![CDATA[
/* * Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
\====
diff --git a/src/etc/performance/dirscanner.xml
b/src/etc/performance/dirscanner.xml
index 5628a46..2c845c3 100644
--- a/src/etc/performance/dirscanner.xml
+++ b/src/etc/performance/dirscanner.xml
@@ -57,8 +57,8 @@
    <target name="prepare-setup">
      <mkdir dir="${test.dir}/src/org/apache/tools/ant"/>
      <mkdir dir="${test.dir}/dest"/>
-    <echo file="${test.dir}/src/org/apache/tools/ant/DirscannerSetup.java"
-          ><![CDATA[
+    <echo file="${test.dir}/src/org/apache/tools/ant/DirscannerSetup.java">
+      <![CDATA[
  /*
   *  Licensed to the Apache Software Foundation (ASF) under one or more
   *  contributor license agreements.  See the NOTICE file distributed with
\====
\@ -21,23 +21,21 @@ <taskdef name="cvspass"
classname="org.apache.tools.ant.taskdefs.CVSPass"/> <target
name="test1"> <cvspass/> </target> <target name="test2"> <cvspass
cvsroot=":pserver:[hidden email]:/home/cvspublic"
passfile="testpassfile.tmp"%/> </target> <!-- testPassFile --> <target
name="test3"> <cvspass
cvsroot=":pserver:[hidden email]:/home/cvspublic"
password="anoncvs" passfile="testpassfile.tmp"/> </target> <!--
testPassFileDuplicateEntry -->
\@ -21,23 +21,21 @@ <taskdef name="cvspass"
classname="org.apache.tools.ant.taskdefs.CVSPass"/> <target
name="test1"> <cvspass/> </target> <target name="test2"> <cvspass
cvsroot=":pserver:[hidden email]:/home/cvspublic"
passfile="testpassfile.tmp"/> </target> <!-- testPassFile --> <target
name="test3"> <cvspass
cvsroot=":pserver:[hidden email]:/home/cvspublic"
password="anoncvs" passfile="testpassfile.tmp"/> </target> <!--
testPassFileDuplicateEntry -->
\====
diff --git a/src/etc/testcases/taskdefs/cvspass.xml
b/src/etc/testcases/taskdefs/cvspass.xml
index bbca110..690dfcf 100644
--- a/src/etc/testcases/taskdefs/cvspass.xml
+++ b/src/etc/testcases/taskdefs/cvspass.xml
@@ -21,23 +21,21 @@
    <taskdef name="cvspass"
classname="org.apache.tools.ant.taskdefs.CVSPass"/>

    <target name="test1">
-    <cvspass />
+    <cvspass/>
    </target>
-
+
    <target name="test2">
      <cvspass
cvsroot=":pserver:[hidden email]:/home/cvspublic"
-      passfile="testpassfile.tmp"
-    />
+      passfile="testpassfile.tmp"%/>
    </target>
-
+
    <!-- testPassFile -->
    <target name="test3">
      <cvspass
cvsroot=":pserver:[hidden email]:/home/cvspublic"
        password="anoncvs"
-      passfile="testpassfile.tmp"
-    />
+      passfile="testpassfile.tmp"/>
    </target>

In the cherry-pick commit (7df9120e) the last difference does not surface.


Best regards, Martijn



On 06-07-18 11:02, Stefan Bodewig wrote:

> On 2018-07-06, Jaikiran Pai wrote:
>
>> On 05/07/18 2:42 PM, Stefan Bodewig wrote:
>>> On 2018-07-05, Jaikiran Pai wrote:
>>>> I personally believe that reviewing these meaningless changes is a
>>>> waste of time and energy. I'm in favour of rolling back the entire
>>>> commit set if that's what it takes.
>>> +1
>>> although reverting the commits in both branches and merging back the
>>> 1.9.x branch is likely going to end in an ugly merge that will need
>>> review as well. Hopefullly a shorter one.
>> Yes, I agree.I plan to attempt this later tonight (around 8 hours from
>> now) if no one else gets to it before that. Just letting it know here,
>> so that we don't end up duplicating these efforts.
> Thank you. I'm unlikely to get there before you do, would have tackled
> it tomorrow.
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Jaikiran Pai
In reply to this post by Stefan Bodewig
Here's a status of the current state of upstream repo branches "master"
and "1.9.x". But before getting to it, I would like to state that I
really had no pleasure in doing these reverts. I really do mean it. I
wish we had never ended up in this situation (and hopefully will never
again), but I do believe reverting this was the right decision.

1.9.x branch:

- Before starting the revert operation, the latest commit on this branch
was 7df9120ebc1f9bee97a6a1a47f0a5fda986e4ab0 which was the trailing
whitespace commit.
- Reverted that commit by issuing a "git revert
7df9120ebc1f9bee97a6a1a47f0a5fda986e4ab0"
- Did a clean build (just compilation and jar generation, no test cases
were run locally)
- Finally to make sure I didn't introduce any unnecessary
changes/problems of my own in this efforts, I compared the latest commit
(which was the revert commit) against the
7df9120ebc1f9bee97a6a1a47f0a5fda986e4ab0 commit's parent on 1.9.x,
b2da27513c7806357bf146bde44b3cc469757122 using the following command:

git diff b2da27513c7806357bf146bde44b3cc469757122
9b1b8dbbc6e9aa98922da28683f3773f586811e5

This returned empty (which is a good thing)

Pushed this state to upstream 1.9.x branch.

master branch:

This was a bit more complicated than the 1.9.x branch, given merge
commits plus additional master only commits that had happened on this
branch between the trailing whitespace commit, plus the fact that the
trailing whitespace commit touched extremely large number of files.

- Before starting the revert on this branch, the latest commit on it was
4ce54bf3b6c521af9c8db3229df5cd8b3199a3b2.
- The trailing whitespace commit was done before that above commit and
was 2f64e0b51c295960cb15aa77c7c1f447b2518e14. There were some other
unrelated commits between these 2 commits.
- Reverted that commit by issuing:

git revert 2f64e0b51c295960cb15aa77c7c1f447b2518e14

Needed to sort out merge conflicts with the following files:
Unmerged paths:
   (use "git reset HEAD <file>..." to unstage)
   (use "git add <file>..." to mark resolution)

     both modified:   src/etc/testcases/taskdefs/cvspass.xml
     both modified:   src/etc/testcases/taskdefs/delete.xml
     both modified:   src/etc/testcases/taskdefs/jar.xml
     both modified: src/etc/testcases/taskdefs/optional/pvcs.xml
     both modified:
src/etc/testcases/taskdefs/optional/xml/endpiece-ns-no-location.xml
     both modified: src/etc/testcases/taskdefs/optional/xml/endpiece.xml
     both modified:   src/etc/testcases/taskdefs/typedef.xml
     both modified:   src/tests/antunit/taskdefs/copy-test.xml
     both modified:   src/tests/antunit/taskdefs/get-test.xml

Manually fixed those conflicts and completed the revert.

- Did one round of local clean build to make sure things were fine.
- At this point, the revert was complete, but there remained one more
step to merge 1.9.x branch into master.
- Issued "git merge 1.9.x" (where 1.9.x was the branch which included
the fully reverted state)
- Merge conflicts had to be resolved in the following files:

Unmerged paths:
   (use "git add/rm <file>..." as appropriate to mark resolution)

     both modified:   src/etc/poms/ant-javamail/pom.xml
     both modified:   src/etc/poms/ant-swing/pom.xml
     both modified:   src/etc/poms/ant-testutil/pom.xml
     both modified:   src/etc/testcases/filters/build.xml
     both modified: src/etc/testcases/taskdefs/conditions/antversion.xml
     both modified:   src/etc/testcases/taskdefs/delete.xml
     both modified:   src/etc/testcases/taskdefs/java.xml
     both modified: src/etc/testcases/taskdefs/optional/pvcs.xml
     both modified: src/etc/testcases/taskdefs/optional/script.xml
     both modified: src/tests/antunit/core/extension-point-test.xml
     deleted by us:   src/tests/antunit/taskdefs/apt-test.xml
     both modified:   src/tests/antunit/taskdefs/get-test.xml
     both modified:   src/tests/antunit/taskdefs/javac-test.xml
     both modified:   src/tests/antunit/taskdefs/tar-test.xml
     both modified: src/tests/antunit/taskdefs/taskdef-antlib-test.xml

Manually resolved the conflicts in all of those and committed the merge
locally.

- Ran one more round of local build and it went fine (no tests were
executed locally).
- Finally to make sure that I hadn't messed up these revert + merge, I
did a compare of the latest master commit (which included the revert +
merge state) against the parent of the (reverted)
2f64e0b51c295960cb15aa77c7c1f447b2518e14 commit,
aad5b519563cfe3dab3034be9bd3e83c0fb508c0 using the command:

git diff aad5b519563cfe3dab3034be9bd3e83c0fb508c0
995b518abf60dd8cd52f9e94c4186bbf78513b96

(unlike the 1.9.x case), this command returned a diff which contained
the expected set of changes that had gone in between the trailing
whitespace, force updated merge commits on master. Reviewed this
comparison diff and it looked fine and correct to me. Pushed this state
to "master" branch of upstream repo.

At this point, these branches are in a state where they can be used for
regular development and other planned activities.

-Jaikiran

On 06/07/18 2:32 PM, Stefan Bodewig wrote:

> On 2018-07-06, Jaikiran Pai wrote:
>> On 05/07/18 2:42 PM, Stefan Bodewig wrote:
>>> On 2018-07-05, Jaikiran Pai wrote:
>>>> I personally believe that reviewing these meaningless changes is a
>>>> waste of time and energy. I'm in favour of rolling back the entire
>>>> commit set if that's what it takes.
>>> +1
>>> although reverting the commits in both branches and merging back the
>>> 1.9.x branch is likely going to end in an ugly merge that will need
>>> review as well. Hopefullly a shorter one.
>> Yes, I agree.I plan to attempt this later tonight (around 8 hours
>> from now) if no one else gets to it before that. Just letting it know
>> here, so that we don't end up duplicating these efforts.
> Thank you. I'm unlikely to get there before you do, would have tackled
> it tomorrow. Stefan
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email] For additional
> commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Jaikiran Pai
In reply to this post by Martijn Kruithof-2
Hello Martijn,

Thank you for spending time on this and for that checker. I did look
into yourmail and considered itbefore deciding on what to do with the
state of upstream branches.I decided to go with a complete revert
approach (which I explain in a separate reply in this thread), because I
personally felt a lot more confident in what it meant and involved, in
terms of git technicalities.
Again, thank you very much for these efforts. Just like you and everyone
else who's been involved in the project, I am not happy that we had to
do a revertand the sequence of eventswhich led us to this.

-Jaikiran



On 06/07/18 5:19 PM, [hidden email] wrote:

> Hello all
>
> Just for your info, please take into consideration for a rollback or
> not descision (I am neither happy with the commit, nor happy with a
> rollback and therefore am 0 on rollback or not):
>
>
> I have written a small checker to remove all whitespace only diffs
> from the diffs on the URL below.
>
> Significant whitespace changes that may have been lost would be
> unlikely (space difference before "/>" , ">" or ")" and around "=" is
> also ignored)
>
> That would reduce the original "2f64e0b5"  diff to (the extra lines
> starting with \ before the diff are added by the checker i wrote)
>
> Removing whitespace only changes from asf.txt
> From: Gintas Grigelionis <[hidden email]>
> Date: Sun, 1 Jul 2018 13:31:35 +0000 (+0200)
> Subject: Trailing whitespace
> X-Git-Url:
> https://git1-us-west.apache.org/repos/asf?p=ant.git;a=commitdiff_plain;h=2f64e0b5
>
> Trailing whitespace
> ---
>
> \====
> \@ -87,11 +87,11 @@ <li><xsl:value-of select="@name"/></li>
> </xsl:for-each> </ul--> <h1><a name="top">JDepend Analysis</a></h1> <p
> align="right">Designed for use with <a
> href="http://www.clarkware.com/software/JDepend.html">JDepend</a> and
> <a href="https://ant.apache.org">Ant</a>.</p> <hr size="2"/> <table
> width="100%"><tr><td> <a name="NVsummary"><h2>Summary</h2></a>
> </td><td align="right">
> \@ -87,11 +87,11 @@ <li><xsl:value-of select="@name"/></li>
> </xsl:for-each> </ul--> <h1><a name="top">JDepend Analysis</a></h1> <p
> align="right">Designed for use with <a
> href="http://www.clarkware.com/software/JDepend.html">JDepend</a> and
> <a href="http://jakarta.apache.org">Ant</a>.</p> <hr size="2"/> <table
> width="100%"><tr><td> <a name="NVsummary"><h2>Summary</h2></a>
> </td><td align="right">
> \====
> diff --git a/src/etc/jdepend.xsl b/src/etc/jdepend.xsl
> index f813297..907ade2 100644
> --- a/src/etc/jdepend.xsl
> +++ b/src/etc/jdepend.xsl
> @@ -87,11 +87,11 @@
>          <li><xsl:value-of select="@name"/></li>
>      </xsl:for-each>
>      </ul-->
> -
> +
>      <h1><a name="top">JDepend Analysis</a></h1>
> -    <p align="right">Designed for use with <a
> href="http://www.clarkware.com/software/JDepend.html">JDepend</a> and
> <a href="http://jakarta.apache.org">Ant</a>.</p>
> -    <hr size="2" />
> -
> +    <p align="right">Designed for use with <a
> href="http://www.clarkware.com/software/JDepend.html">JDepend</a> and
> <a href="https://ant.apache.org">Ant</a>.</p>
> +    <hr size="2"/>
> +
>      <table width="100%"><tr><td>
>      <a name="NVsummary"><h2>Summary</h2></a>
>      </td><td align="right">
> \====
> \@ -57,8 +57,8 @@ <target name="prepare-setup"> <mkdir
> dir="${test.dir}/src/org/apache/tools/ant"/> <mkdir
> dir="${test.dir}/dest"/> <echo
> file="${test.dir}/src/org/apache/tools/ant/DirscannerSetup.java">
> <![CDATA[ /* * Licensed to the Apache Software Foundation (ASF) under
> one or more * contributor license agreements. See the NOTICE file
> distributed with
> \@ -57,8 +57,8 @@ <target name="prepare-setup"> <mkdir
> dir="${test.dir}/src/org/apache/tools/ant"/> <mkdir
> dir="${test.dir}/dest"/> <echo
> file="${test.dir}/src/org/apache/tools/ant/DirscannerSetup.java"><![CDATA[
> /* * Licensed to the Apache Software Foundation (ASF) under one or
> more * contributor license agreements. See the NOTICE file distributed
> with
> \====
> diff --git a/src/etc/performance/dirscanner.xml
> b/src/etc/performance/dirscanner.xml
> index 5628a46..2c845c3 100644
> --- a/src/etc/performance/dirscanner.xml
> +++ b/src/etc/performance/dirscanner.xml
> @@ -57,8 +57,8 @@
>    <target name="prepare-setup">
>      <mkdir dir="${test.dir}/src/org/apache/tools/ant"/>
>      <mkdir dir="${test.dir}/dest"/>
> -    <echo
> file="${test.dir}/src/org/apache/tools/ant/DirscannerSetup.java"
> -          ><![CDATA[
> +    <echo
> file="${test.dir}/src/org/apache/tools/ant/DirscannerSetup.java">
> +      <![CDATA[
>  /*
>   *  Licensed to the Apache Software Foundation (ASF) under one or more
>   *  contributor license agreements.  See the NOTICE file distributed
> with
> \====
> \@ -21,23 +21,21 @@ <taskdef name="cvspass"
> classname="org.apache.tools.ant.taskdefs.CVSPass"/> <target
> name="test1"> <cvspass/> </target> <target name="test2"> <cvspass
> cvsroot=":pserver:[hidden email]:/home/cvspublic"
> passfile="testpassfile.tmp"%/> </target> <!-- testPassFile --> <target
> name="test3"> <cvspass
> cvsroot=":pserver:[hidden email]:/home/cvspublic"
> password="anoncvs" passfile="testpassfile.tmp"/> </target> <!--
> testPassFileDuplicateEntry -->
> \@ -21,23 +21,21 @@ <taskdef name="cvspass"
> classname="org.apache.tools.ant.taskdefs.CVSPass"/> <target
> name="test1"> <cvspass/> </target> <target name="test2"> <cvspass
> cvsroot=":pserver:[hidden email]:/home/cvspublic"
> passfile="testpassfile.tmp"/> </target> <!-- testPassFile --> <target
> name="test3"> <cvspass
> cvsroot=":pserver:[hidden email]:/home/cvspublic"
> password="anoncvs" passfile="testpassfile.tmp"/> </target> <!--
> testPassFileDuplicateEntry -->
> \====
> diff --git a/src/etc/testcases/taskdefs/cvspass.xml
> b/src/etc/testcases/taskdefs/cvspass.xml
> index bbca110..690dfcf 100644
> --- a/src/etc/testcases/taskdefs/cvspass.xml
> +++ b/src/etc/testcases/taskdefs/cvspass.xml
> @@ -21,23 +21,21 @@
>    <taskdef name="cvspass"
> classname="org.apache.tools.ant.taskdefs.CVSPass"/>
>
>    <target name="test1">
> -    <cvspass />
> +    <cvspass/>
>    </target>
> -
> +
>    <target name="test2">
>      <cvspass
> cvsroot=":pserver:[hidden email]:/home/cvspublic"
> -      passfile="testpassfile.tmp"
> -    />
> +      passfile="testpassfile.tmp"%/>
>    </target>
> -
> +
>    <!-- testPassFile -->
>    <target name="test3">
>      <cvspass
> cvsroot=":pserver:[hidden email]:/home/cvspublic"
>        password="anoncvs"
> -      passfile="testpassfile.tmp"
> -    />
> +      passfile="testpassfile.tmp"/>
>    </target>
>
> In the cherry-pick commit (7df9120e) the last difference does not
> surface.
>
>
> Best regards, Martijn
>
>
>
> On 06-07-18 11:02, Stefan Bodewig wrote:
>> On 2018-07-06, Jaikiran Pai wrote:
>>
>>> On 05/07/18 2:42 PM, Stefan Bodewig wrote:
>>>> On 2018-07-05, Jaikiran Pai wrote:
>>>>> I personally believe that reviewing these meaningless changes is a
>>>>> waste of time and energy. I'm in favour of rolling back the entire
>>>>> commit set if that's what it takes.
>>>> +1
>>>> although reverting the commits in both branches and merging back the
>>>> 1.9.x branch is likely going to end in an ugly merge that will need
>>>> review as well. Hopefullly a shorter one.
>>> Yes, I agree.I plan to attempt this later tonight (around 8 hours from
>>> now) if no one else gets to it before that. Just letting it know here,
>>> so that we don't end up duplicating these efforts.
>> Thank you. I'm unlikely to get there before you do, would have tackled
>> it tomorrow.
>>
>> Stefan
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Nicolas Lalevée
In reply to this post by Jaikiran Pai
I have re done the revert and merge locally, and compared the diff between my local banches and the remote one. I had only diffs due to my editor automatically removing trailing space on empty lines, and I had to figure out what the status of the apt tasks (and indeed it doesn’t need to be merged back into master).

I have not tested as much as you have, but I have tested the revert, merge and conflicts resolve, and it is ok for me.

So thank you for you detailed mail about what you did. I think we should have that rule: in case of a commits which generates a lot of diff, share the commands which generated these diffs. Thus anyone can check the rationale of the commands and make the diff between local and remote.

Nicolas

> Le 7 juil. 2018 à 14:53, Jaikiran Pai <[hidden email]> a écrit :
>
> Here's a status of the current state of upstream repo branches "master" and "1.9.x". But before getting to it, I would like to state that I really had no pleasure in doing these reverts. I really do mean it. I wish we had never ended up in this situation (and hopefully will never again), but I do believe reverting this was the right decision.
>
> 1.9.x branch:
>
> - Before starting the revert operation, the latest commit on this branch was 7df9120ebc1f9bee97a6a1a47f0a5fda986e4ab0 which was the trailing whitespace commit.
> - Reverted that commit by issuing a "git revert 7df9120ebc1f9bee97a6a1a47f0a5fda986e4ab0"
> - Did a clean build (just compilation and jar generation, no test cases were run locally)
> - Finally to make sure I didn't introduce any unnecessary changes/problems of my own in this efforts, I compared the latest commit (which was the revert commit) against the 7df9120ebc1f9bee97a6a1a47f0a5fda986e4ab0 commit's parent on 1.9.x, b2da27513c7806357bf146bde44b3cc469757122 using the following command:
>
> git diff b2da27513c7806357bf146bde44b3cc469757122 9b1b8dbbc6e9aa98922da28683f3773f586811e5
>
> This returned empty (which is a good thing)
>
> Pushed this state to upstream 1.9.x branch.
>
> master branch:
>
> This was a bit more complicated than the 1.9.x branch, given merge commits plus additional master only commits that had happened on this branch between the trailing whitespace commit, plus the fact that the trailing whitespace commit touched extremely large number of files.
>
> - Before starting the revert on this branch, the latest commit on it was 4ce54bf3b6c521af9c8db3229df5cd8b3199a3b2.
> - The trailing whitespace commit was done before that above commit and was 2f64e0b51c295960cb15aa77c7c1f447b2518e14. There were some other unrelated commits between these 2 commits.
> - Reverted that commit by issuing:
>
> git revert 2f64e0b51c295960cb15aa77c7c1f447b2518e14
>
> Needed to sort out merge conflicts with the following files:
> Unmerged paths:
>   (use "git reset HEAD <file>..." to unstage)
>   (use "git add <file>..." to mark resolution)
>
>     both modified:   src/etc/testcases/taskdefs/cvspass.xml
>     both modified:   src/etc/testcases/taskdefs/delete.xml
>     both modified:   src/etc/testcases/taskdefs/jar.xml
>     both modified: src/etc/testcases/taskdefs/optional/pvcs.xml
>     both modified: src/etc/testcases/taskdefs/optional/xml/endpiece-ns-no-location.xml
>     both modified: src/etc/testcases/taskdefs/optional/xml/endpiece.xml
>     both modified:   src/etc/testcases/taskdefs/typedef.xml
>     both modified:   src/tests/antunit/taskdefs/copy-test.xml
>     both modified:   src/tests/antunit/taskdefs/get-test.xml
>
> Manually fixed those conflicts and completed the revert.
>
> - Did one round of local clean build to make sure things were fine.
> - At this point, the revert was complete, but there remained one more step to merge 1.9.x branch into master.
> - Issued "git merge 1.9.x" (where 1.9.x was the branch which included the fully reverted state)
> - Merge conflicts had to be resolved in the following files:
>
> Unmerged paths:
>   (use "git add/rm <file>..." as appropriate to mark resolution)
>
>     both modified:   src/etc/poms/ant-javamail/pom.xml
>     both modified:   src/etc/poms/ant-swing/pom.xml
>     both modified:   src/etc/poms/ant-testutil/pom.xml
>     both modified:   src/etc/testcases/filters/build.xml
>     both modified: src/etc/testcases/taskdefs/conditions/antversion.xml
>     both modified:   src/etc/testcases/taskdefs/delete.xml
>     both modified:   src/etc/testcases/taskdefs/java.xml
>     both modified: src/etc/testcases/taskdefs/optional/pvcs.xml
>     both modified: src/etc/testcases/taskdefs/optional/script.xml
>     both modified: src/tests/antunit/core/extension-point-test.xml
>     deleted by us:   src/tests/antunit/taskdefs/apt-test.xml
>     both modified:   src/tests/antunit/taskdefs/get-test.xml
>     both modified:   src/tests/antunit/taskdefs/javac-test.xml
>     both modified:   src/tests/antunit/taskdefs/tar-test.xml
>     both modified: src/tests/antunit/taskdefs/taskdef-antlib-test.xml
>
> Manually resolved the conflicts in all of those and committed the merge locally.
>
> - Ran one more round of local build and it went fine (no tests were executed locally).
> - Finally to make sure that I hadn't messed up these revert + merge, I did a compare of the latest master commit (which included the revert + merge state) against the parent of the (reverted) 2f64e0b51c295960cb15aa77c7c1f447b2518e14 commit, aad5b519563cfe3dab3034be9bd3e83c0fb508c0 using the command:
>
> git diff aad5b519563cfe3dab3034be9bd3e83c0fb508c0 995b518abf60dd8cd52f9e94c4186bbf78513b96
>
> (unlike the 1.9.x case), this command returned a diff which contained the expected set of changes that had gone in between the trailing whitespace, force updated merge commits on master. Reviewed this comparison diff and it looked fine and correct to me. Pushed this state to "master" branch of upstream repo.
>
> At this point, these branches are in a state where they can be used for regular development and other planned activities.
>
> -Jaikiran
>
> On 06/07/18 2:32 PM, Stefan Bodewig wrote:
>> On 2018-07-06, Jaikiran Pai wrote:
>>> On 05/07/18 2:42 PM, Stefan Bodewig wrote:
>>>> On 2018-07-05, Jaikiran Pai wrote:
>>>>> I personally believe that reviewing these meaningless changes is a waste of time and energy. I'm in favour of rolling back the entire commit set if that's what it takes.
>>>> +1 although reverting the commits in both branches and merging back the 1.9.x branch is likely going to end in an ugly merge that will need review as well. Hopefullly a shorter one.
>>> Yes, I agree.I plan to attempt this later tonight (around 8 hours from now) if no one else gets to it before that. Just letting it know here, so that we don't end up duplicating these efforts.
>> Thank you. I'm unlikely to get there before you do, would have tackled it tomorrow. Stefan --------------------------------------------------------------------- To unsubscribe, e-mail: [hidden email] For additional commands, e-mail: [hidden email]
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Stefan Bodewig
In reply to this post by Martijn Kruithof-2
On 2018-07-06, <[hidden email]> wrote:

> That would reduce the original "2f64e0b5"  diff to (the extra lines
> starting with \ before the diff are added by the checker i wrote)

Thanks, Martijn

I've just committed the fix to Ant's URL inside jdepend.xsl.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: ant git commit: Unbreak tests

Stefan Bodewig
In reply to this post by Jaikiran Pai
On 2018-07-07, Jaikiran Pai wrote:

> Here's a status of the current state of upstream repo branches
> "master" and "1.9.x".

Many thanks, Jaikiran and many thanks for the detailed report. I've been
able to reproduce the checks you've performed.

I'll build RCs for the next Ant releases soon.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]