[PATCH] 3-way merge with file move fails when diff.renames = copies

[PATCH] 3-way merge with file move fails when diff.renames = copies

[David D. Kilzer]

With diff.renames = copies, a 3-way merge (e.g. "git rebase") would
fail with the following error:

    fatal: mode change for <file>, which is not in current HEAD
    Repository lacks necessary blobs to fall back on 3-way merge.
    Cannot fall back to three-way merge.
    Patch failed at 0001.

The bug is a logic error added in ece7b749, which attempts to find
an sha1 for a patch with no index line in build_fake_ancestor().
Instead of failing unless an sha1 is found for both the old file and
the new file, a failure should only be reported if neither the old
file nor the new file is found.

Signed-off-by: David D. Kilzer <ddkilzer@kilzer.net>
---
 builtin-apply.c   |    2 +-
 t/t3400-rebase.sh |   17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 4c4d1e1..cfeb6cc 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2573,7 +2573,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 		else if (get_sha1(patch->old_sha1_prefix, sha1))
 			/* git diff has no index line for mode/type changes */
 			if (!patch->lines_added && !patch->lines_deleted) {
-				if (get_current_sha1(patch->new_name, sha1) ||
+				if (get_current_sha1(patch->new_name, sha1) &&
 				    get_current_sha1(patch->old_name, sha1))
 					die("mode change for %s, which is not "
 						"in current HEAD", name);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b7a670e..a156850 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -84,4 +84,21 @@ test_expect_success 'rebase a single mode change' '
      GIT_TRACE=1 git rebase master
 '
 
+test_expect_success 'rebase a single file move with diff.renames = copies' '
+     git config diff.renames copies &&
+     git checkout master &&
+     echo 1 > Y &&
+     git add Y &&
+     test_tick &&
+     git commit -m "prepare file move" &&
+     git checkout -b filemove HEAD^ &&
+     echo 1 > Y &&
+     git add Y &&
+     mkdir D &&
+     git mv A D/A &&
+     test_tick &&
+     git commit -m filemove &&
+     GIT_TRACE=1 git rebase master
+'
+
 test_done
-- 
1.6.0

Re: [PATCH] 3-way merge with file move fails when diff.renames = copies

[Johannes Schindelin]

Hi,

except for the commit subject I like it, especially the commit body. So:

s/(3-way.*) fails/Fix &/

Ciao,
Dscho

Re: [PATCH] 3-way merge with file move fails when diff.renames = copies

[Junio C Hamano]

"David D. Kilzer" <ddkilzer@kilzer.net> writes:

> With diff.renames = copies, a 3-way merge (e.g. "git rebase") would
> fail with the following error:
>
>     fatal: mode change for <file>, which is not in current HEAD
>     Repository lacks necessary blobs to fall back on 3-way merge.
>     Cannot fall back to three-way merge.
>     Patch failed at 0001.
>
> The bug is a logic error added in ece7b749, which attempts to find
> an sha1 for a patch with no index line in build_fake_ancestor().
> Instead of failing unless an sha1 is found for both the old file and
> the new file, a failure should only be reported if neither the old
> file nor the new file is found.
>
> Signed-off-by: David D. Kilzer <ddkilzer@kilzer.net>
> ---
>  builtin-apply.c   |    2 +-
>  t/t3400-rebase.sh |   17 +++++++++++++++++
>  2 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 4c4d1e1..cfeb6cc 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -2573,7 +2573,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
>  		else if (get_sha1(patch->old_sha1_prefix, sha1))
>  			/* git diff has no index line for mode/type changes */
>  			if (!patch->lines_added && !patch->lines_deleted) {
> -				if (get_current_sha1(patch->new_name, sha1) ||
> +				if (get_current_sha1(patch->new_name, sha1) &&
>  				    get_current_sha1(patch->old_name, sha1))
>  					die("mode change for %s, which is not "
>  						"in current HEAD", name);

Hmm.

The logic introduced by the blamed commit makes the --index-info
unreliable (I'd rather see it fail reliably if it does not have enough
information rather than pretending everything is Ok), and I think the
patch makes it slightly more so.

If new_name that is not related at all to old_name happens to exist in the
current tree you are applying the patch to, you can grab the contents of
the unrelated file as the preimage and try to merge the changes in.

When running --index-info for the purpose of "am -3" (hence rebase), the
expectation is that the tree you are applying the changes to is _similar_
to the preimage of the change, i.e. old_name.  Shouldn't missing old_name
be treated as a fatal condition?  new_name does not have to even exist
because otherwise you cannot accept a patch that creates the path.

Wouldn't this be a better patch, I wonder...

 builtin-apply.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git i/builtin-apply.c w/builtin-apply.c
index 4c4d1e1..7de70e9 100644
--- i/builtin-apply.c
+++ w/builtin-apply.c
@@ -2573,8 +2573,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 		else if (get_sha1(patch->old_sha1_prefix, sha1))
 			/* git diff has no index line for mode/type changes */
 			if (!patch->lines_added && !patch->lines_deleted) {
-				if (get_current_sha1(patch->new_name, sha1) ||
-				    get_current_sha1(patch->old_name, sha1))
+				if (get_current_sha1(patch->old_name, sha1))
 					die("mode change for %s, which is not "
 						"in current HEAD", name);
 				sha1_ptr = sha1;

[PATCH] Fix 3-way merge with file move when diff.renames = copies

[David D. Kilzer]

With diff.renames = copies, a 3-way merge (e.g. "git rebase") with a
file move would fail with the following error:

    fatal: mode change for <file>, which is not in current HEAD
    Repository lacks necessary blobs to fall back on 3-way merge.
    Cannot fall back to three-way merge.
    Patch failed at 0001.

The bug is a logic error added in ece7b749, which attempts to find
an sha1 for a patch with no index line in build_fake_ancestor().
Instead of failing unless an sha1 is found for both the old file and
the new file, a failure should only be reported if neither the old
file nor the new file is found.

Signed-off-by: David D. Kilzer <ddkilzer@kilzer.net>
---
Same as "[PATCH] 3-way merge with file move fails when diff.renames = copies"
but with an updated subject and initial sentence (added "with a file move").

 builtin-apply.c   |    2 +-
 t/t3400-rebase.sh |   17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 4c4d1e1..cfeb6cc 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2573,7 +2573,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 		else if (get_sha1(patch->old_sha1_prefix, sha1))
 			/* git diff has no index line for mode/type changes */
 			if (!patch->lines_added && !patch->lines_deleted) {
-				if (get_current_sha1(patch->new_name, sha1) ||
+				if (get_current_sha1(patch->new_name, sha1) &&
 				    get_current_sha1(patch->old_name, sha1))
 					die("mode change for %s, which is not "
 						"in current HEAD", name);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b7a670e..a156850 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -84,4 +84,21 @@ test_expect_success 'rebase a single mode change' '
      GIT_TRACE=1 git rebase master
 '
 
+test_expect_success 'rebase a single file move with diff.renames = copies' '
+     git config diff.renames copies &&
+     git checkout master &&
+     echo 1 > Y &&
+     git add Y &&
+     test_tick &&
+     git commit -m "prepare file move" &&
+     git checkout -b filemove HEAD^ &&
+     echo 1 > Y &&
+     git add Y &&
+     mkdir D &&
+     git mv A D/A &&
+     test_tick &&
+     git commit -m filemove &&
+     GIT_TRACE=1 git rebase master
+'
+
 test_done
-- 
1.6.0

Re: [PATCH] 3-way merge with file move fails when diff.renames = copies

[David D. Kilzer]

Junio C Hamano <gitster@pobox.com> writes:

> If new_name that is not related at all to old_name happens to exist in the
> current tree you are applying the patch to, you can grab the contents of
> the unrelated file as the preimage and try to merge the changes in.
> 
> When running --index-info for the purpose of "am -3" (hence rebase), the
> expectation is that the tree you are applying the changes to is _similar_
> to the preimage of the change, i.e. old_name.  Shouldn't missing old_name
> be treated as a fatal condition?  new_name does not have to even exist
> because otherwise you cannot accept a patch that creates the path.
> 
> Wouldn't this be a better patch, I wonder...
> 
> builtin-apply.c |    3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git i/builtin-apply.c w/builtin-apply.c
> index 4c4d1e1..7de70e9 100644
> --- i/builtin-apply.c
> +++ w/builtin-apply.c
> @@ -2573,8 +2573,7 @@ static void build_fake_ancestor(struct patch *list, const 
> char *filename)
>         else if (get_sha1(patch->old_sha1_prefix, sha1))
>             /* git diff has no index line for mode/type changes */
>             if (!patch->lines_added && !patch->lines_deleted) {
> -                if (get_current_sha1(patch->new_name, sha1) ||
> -                    get_current_sha1(patch->old_name, sha1))
> +                if (get_current_sha1(patch->old_name, sha1))
>                     die("mode change for %s, which is not "
>                         "in current HEAD", name);
>                 sha1_ptr = sha1;


Yes, this change makes more sense to me, but I'll defer to Johannes' opinion before submitting another patch.

Dave

Re: [PATCH] 3-way merge with file move fails when diff.renames = copies

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "David D. Kilzer" <ddkilzer@kilzer.net> writes:
>
>> With diff.renames = copies, a 3-way merge (e.g. "git rebase") would
>> fail with the following error:

By the way, I think the real issue with this one is that we currently do
not disable diff.renames configuration while rebase internally runs
"format-patch" to feed "am -3".

The end user configuration for "diff" should not affect the result
produced by the higher level command that is related to "diff" only
because internally it is implemented in terms of it.

For that matter, I have a feeling that format-patch should not even look
at diff.renames, but we seem to have been doing this for a long time so
there is no easy way to fix this thinko.

In any case, here is a much straightforward fix for "rebase".  

Running "am -3" on a copying patch would still need a patch to the
index-info codepath, and my earlier comment on it still stands, but it is
irrelevant/orthogonal to your particular test script.

 git-rebase.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git c/git-rebase.sh w/git-rebase.sh
index 023a6dc..159ccb3 100755
--- c/git-rebase.sh
+++ w/git-rebase.sh
@@ -429,7 +429,7 @@ fi
 if test -z "$do_merge"
 then
 	git format-patch -k --stdout --full-index --ignore-if-in-upstream \
-		"$upstream..$orig_head" |
+		--no-renames "$upstream..$orig_head" |
 	git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" &&
 	move_to_original_branch
 	ret=$?

[PATCH] Fix rebase with file move when diff.renames = copies

[David D. Kilzer]

With diff.renames = copies, a rebase with a file move will fail with
the following error:

    fatal: mode change for <file>, which is not in current HEAD
    Repository lacks necessary blobs to fall back on 3-way merge.
    Cannot fall back to three-way merge.
    Patch failed at 0001.

The bug is that git rebase does not disable diff.renames when it runs
format-patch internally to feed into "am -3".  The fix is simply to
include a --no-renames argument to format-patch to override any local
diff.renames setting.

Fix by Junio C Hamano.  Test case by David D. Kilzer.

Signed-off-by: David D. Kilzer <ddkilzer@kilzer.net>
---
 git-rebase.sh     |    2 +-
 t/t3400-rebase.sh |   17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 6ec155c..0718caf 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -514,7 +514,7 @@ fi
 if test -z "$do_merge"
 then
 	git format-patch -k --stdout --full-index --ignore-if-in-upstream \
-		$root_flag "$revisions" |
+		--no-renames $root_flag "$revisions" |
 	git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" &&
 	move_to_original_branch
 	ret=$?
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 4e6a44b..6d2ec91 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -155,4 +155,21 @@ test_expect_success 'Rebase a commit that sprinkles CRs in' '
 	git diff --exit-code file-with-cr:CR HEAD:CR
 '
 
+test_expect_success 'rebase a single file move with diff.renames = copies' '
+     git config diff.renames copies &&
+     git checkout master &&
+     echo 1 > Y &&
+     git add Y &&
+     test_tick &&
+     git commit -m "prepare file move" &&
+     git checkout -b filemove HEAD^ &&
+     echo 1 > Y &&
+     git add Y &&
+     mkdir D &&
+     git mv A D/A &&
+     test_tick &&
+     git commit -m filemove &&
+     GIT_TRACE=1 git rebase master
+'
+
 test_done
-- 
1.7.1

Re: [PATCH] Fix rebase with file move when diff.renames = copies

[Junio C Hamano]

"David D. Kilzer" <ddkilzer@kilzer.net> writes:

> The bug is that git rebase does not disable diff.renames when it runs
> format-patch internally to feed into "am -3".  The fix is simply to
> include a --no-renames argument to format-patch to override any local
> diff.renames setting.
>
> Fix by Junio C Hamano.  Test case by David D. Kilzer.
>
> Signed-off-by: David D. Kilzer <ddkilzer@kilzer.net>

Hmm, I actully do not recall doing this patch, even though I think what it
does probably makes sense.

Re: [PATCH] Fix rebase with file move when diff.renames = copies

[David D. Kilzer]

On Wed, July 21, 2010 at 2:54:18 PM, Junio C Hamano wrote:

> "David D. Kilzer" <ddkilzer@kilzer.net>  writes:
> 
> > The bug is that git rebase does not disable diff.renames  when it runs
> > format-patch internally to feed into "am -3".  The fix  is simply to
> > include a --no-renames argument to format-patch to override  any local
> > diff.renames setting.
> >
> > Fix by Junio C  Hamano.  Test case by David D. Kilzer.
> >
> > Signed-off-by: David  D. Kilzer <ddkilzer@kilzer.net>
> 
> Hmm, I  actully do not recall doing this patch, even though I think what it
> does  probably makes sense.


Yeah, it was a while ago (Nov 11, 2008):

<http://marc.info/?l=git&m=122636263923046&w=2>

Sorry for the delay, but the issue still affects me.  :)

Dave

Re: [PATCH] Fix rebase with file move when diff.renames = copies

[Jonathan Nieder]

David D. Kilzer wrote:

> With diff.renames = copies, a rebase with a file move will fail with
> the following error:
>
>     fatal: mode change for <file>, which is not in current HEAD
>     Repository lacks necessary blobs to fall back on 3-way merge.
>     Cannot fall back to three-way merge.
>     Patch failed at 0001.

I would think that the following works fine:

	git init test-repo &&
	cd test-repo &&
	echo hello >greeting.txt &&
	git add greeting.txt &&
	git commit -m base &&
	git checkout -b move &&
	git mv greeting.txt moved.txt &&
	git commit -m move &&
	git checkout master &&
	echo hi >greeting.txt &&
	git add greeting.txt &&
	git commit -m change &&
	git checkout move &&
	echo '[diff] renames = copies' >>.git/config &&
	git rebase master

What am I doing wrong?

On the other hand I find Junio’s explanation[1] compelling
already on its own.

 The end user configuration for "diff" should not affect the
 result produced by the higher level command that is related to
 "diff" only because internally it is implemented in terms of
 it.

In cases where a patch copies a file that was then removed on the
mainline, my intuition says ‘rebase’ without some extra flag should
accept the change without complaint.  Of course, this intuition is
totally warped --- I tend to think of rebase as diff + apply.

Patch does not apply to master or maint, due to conflict with
v1.7.1-rc0~37^2~5 (rebase: support automatic notes copying,
2010-03-12).  One sneaky way to avoid this kind of thing would be to
insert new tests at some logical point in the middle of a test script.

Test nitpicks:

> +++ b/t/t3400-rebase.sh
> @@ -155,4 +155,21 @@ test_expect_success 'Rebase a commit that sprinkles CRs in' '
>  	git diff --exit-code file-with-cr:CR HEAD:CR
>  '
>  
> +test_expect_success 'rebase a single file move with diff.renames = copies' '
> +     git config diff.renames copies &&

Use

	test_when_finished "git config --unset diff.renames" &&

to shelter future tests from the effect of this one.

> +     git checkout master &&
> +     echo 1 > Y &&
> +     git add Y &&
> +     test_tick &&
> +     git commit -m "prepare file move" &&

commit: new file Y.

> +     git checkout -b filemove HEAD^ &&
> +     echo 1 > Y &&
> +     git add Y &&
> +     mkdir D &&
> +     git mv A D/A &&
> +     test_tick &&
> +     git commit -m filemove &&

commit: new file Y, rename A to D/A.

> +     GIT_TRACE=1 git rebase master

This wants to notice that Y was already added so the top patch can be
simplified to include only a rename.

Can you explain why this test will fail without your patch?

Thanks,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/100583/focus=100606

Re: [PATCH] Fix rebase with file move when diff.renames = copies

[David D. Kilzer]

On Jonathan Nieder wrote:

> David D. Kilzer wrote:
> 
> > With diff.renames = copies, a rebase with a  file move will fail with
> > the following error:
> >
> >      fatal: mode change for <file>, which is not in current  HEAD
> >     Repository lacks necessary blobs to fall back on  3-way merge.
> >     Cannot fall back to three-way  merge.
> >     Patch failed at 0001.
> 
> I would think that the  following works fine:
> 
>     git init test-repo  &&
>     cd test-repo &&
>      echo hello >greeting.txt &&
>     git add  greeting.txt &&
>     git commit -m base  &&
>     git checkout -b move  &&
>     git mv greeting.txt moved.txt  &&
>     git commit -m move  &&
>     git checkout master  &&
>     echo hi >greeting.txt  &&
>     git add greeting.txt  &&
>     git commit -m change  &&
>     git checkout move  &&
>     echo '[diff] renames = copies'  >>.git/config &&
>     git rebase  master
> 
> What am I doing wrong?


Given the following tree:

      B' topic
     /
    A---B master

A: New file "F1" is committed.
B: New file "F2" is committed.
B': New file "F2" is committed (identical in content to "F2" on B), and "F1" is 
renamed to "F3".

When the topic branch is rebased onto master with diff.renames=copies, git fails 
when attempting to build a fake ancestor for F1.  The key to reproducing the bug 
is to have an identical new file added on both B and B'.

My original patch in <http://marc.info/?l=git&m=122635667614099&w=2> addressed 
this in builtin-apply.c, but Junio didn't like this approach as noted in 
<http://marc.info/?l=git&m=122636097120953&w=2>.

> Patch does not apply to master or maint, due to conflict  with
> v1.7.1-rc0~37^2~5 (rebase: support automatic notes  copying,
> 2010-03-12).  One sneaky way to avoid this kind of thing would  be to
> insert new tests at some logical point in the middle of a test  script.


Sorry about that--I forgot to rebase it to maint before sending it.

> Test nitpicks:


Thanks!  I'll make the requested changes in the next patch.

> This wants to notice that Y  was already added so the top patch can be
> simplified to include only a  rename.


Actually, this is the key to reproducing the bug!

> Can you explain why this test will fail without your  patch?


Here is a stand-alone script that reproduces the bug:

    git init test-repo &&
    cd test-repo &&
    echo hello > F1 &&
    git add F1 &&
    git commit -m "A" &&
    git checkout -b topic &&
    echo hi > F2 &&
    git add F2 &&
    git mv F1 F3 &&
    git commit -m "B'" &&
    git checkout master &&
    echo hi > F2 &&
    git add F2 &&
    git commit -m "B" &&
    git checkout topic &&
    git config diff.renames copies &&
    GIT_TRACE=1 git rebase master

Note that the test case in my patch depended on "F1" (which was "A") being 
committed by an earlier test.

Dave

[PATCH 0/5] Fix rebase with file move when diff.renames = copies

[Jonathan Nieder]

David D. Kilzer wrote:

> My original patch in <http://marc.info/?l=git&m=122635667614099&w=2> addressed 
> this in builtin-apply.c, but Junio didn't like this approach as noted in 
> <http://marc.info/?l=git&m=122636097120953&w=2>.

Got it.  This patch just treats the symptoms in my opinion, and if
you read Junio’s message carefully, I think he was also suggesting
that git apply should still be fixed.

Something like this series would fix both.  Please feel free to pick
it up and take it in whatever direction you like.

Hope that helps.

Jonathan Nieder (3):
  t4150 (am): style tweaks
  t4150 (am): futureproof against failing tests
  t3400 (rebase): whitespace cleanup

Junio C Hamano (2):
  Teach "apply --index-info" to handle rename patches
  rebase: protect against diff.renames configuration

 builtin/apply.c   |    3 +-
 git-rebase.sh     |    2 +-
 t/t3400-rebase.sh |  204 ++++++++++++++++++--------------
 t/t4150-am.sh     |  334 +++++++++++++++++++++++++++++++++++-----------------
 t/test-lib.sh     |    4 +
 5 files changed, 345 insertions(+), 202 deletions(-)

-- 
1.7.2.rc3

[PATCH 1/5] t4150 (am): style tweaks

[Jonathan Nieder]

Place setup commands in test_expect_success blocks.  This makes the
rare event of the setup commands breaking on some platform easier to
diagnose, and more importantly, it visually distinguishes where
each test begins and ends.

Instead of running test -z against the result of "git diff" command
substitution, use "git diff --exit-code", to improve output when
running with the "-v" option.

Use test_cmp in place of "test $(foo) = $(bar)" for similar reasons.

Remove whitespace after the > and < redirection operators for
consistency with other tests.

The order of arguments to test_cmp is "test_cmp expected actual".

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4150-am.sh |  240 +++++++++++++++++++++++++++++++--------------------------
 t/test-lib.sh |    4 +
 2 files changed, 136 insertions(+), 108 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 810b04b..b50aad3 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -4,66 +4,71 @@ test_description='git am running'
 
 . ./test-lib.sh
 
-cat >msg <<EOF
-second
-
-Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, sed diam nonumy
-eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam
-voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita
-kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem
-ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod
-tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At
-vero eos et accusam et justo duo dolores et ea rebum.
-
-	Duis autem vel eum iriure dolor in hendrerit in vulputate velit
-	esse molestie consequat, vel illum dolore eu feugiat nulla facilisis
-	at vero eros et accumsan et iusto odio dignissim qui blandit
-	praesent luptatum zzril delenit augue duis dolore te feugait nulla
-	facilisi.
-
-
-Lorem ipsum dolor sit amet,
-consectetuer adipiscing elit, sed diam nonummy nibh euismod tincidunt ut
-laoreet dolore magna aliquam erat volutpat.
-
-  git
-  ---
-  +++
-
-Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit
-lobortis nisl ut aliquip ex ea commodo consequat. Duis autem vel eum iriure
-dolor in hendrerit in vulputate velit esse molestie consequat, vel illum
-dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio
-dignissim qui blandit praesent luptatum zzril delenit augue duis dolore te
-feugait nulla facilisi.
-EOF
-
-cat >failmail <<EOF
-From foo@example.com Fri May 23 10:43:49 2008
-From:	foo@example.com
-To:	bar@example.com
-Subject: Re: [RFC/PATCH] git-foo.sh
-Date:	Fri, 23 May 2008 05:23:42 +0200
-
-Sometimes we have to find out that there's nothing left.
-
-EOF
-
-cat >pine <<EOF
-From MAILER-DAEMON Fri May 23 10:43:49 2008
-Date: 23 May 2008 05:23:42 +0200
-From: Mail System Internal Data <MAILER-DAEMON@example.com>
-Subject: DON'T DELETE THIS MESSAGE -- FOLDER INTERNAL DATA
-Message-ID: <foo-0001@example.com>
-
-This text is part of the internal format of your mail folder, and is not
-a real message.  It is created automatically by the mail system software.
-If deleted, important folder data will be lost, and it will be re-created
-with the data reset to initial values.
-
-EOF
-
-echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >expected
+test_expect_success 'setup: messages' '
+	cat >msg <<-\EOF &&
+	second
+
+	Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, sed diam nonumy
+	eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam
+	voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita
+	kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem
+	ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod
+	tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At
+	vero eos et accusam et justo duo dolores et ea rebum.
+
+	EOF
+	q_to_tab <<-\EOF >>msg &&
+	QDuis autem vel eum iriure dolor in hendrerit in vulputate velit
+	Qesse molestie consequat, vel illum dolore eu feugiat nulla facilisis
+	Qat vero eros et accumsan et iusto odio dignissim qui blandit
+	Qpraesent luptatum zzril delenit augue duis dolore te feugait nulla
+	Qfacilisi.
+	EOF
+	cat >>msg <<-\EOF &&
+
+	Lorem ipsum dolor sit amet,
+	consectetuer adipiscing elit, sed diam nonummy nibh euismod tincidunt ut
+	laoreet dolore magna aliquam erat volutpat.
+
+	  git
+	  ---
+	  +++
+
+	Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit
+	lobortis nisl ut aliquip ex ea commodo consequat. Duis autem vel eum iriure
+	dolor in hendrerit in vulputate velit esse molestie consequat, vel illum
+	dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio
+	dignissim qui blandit praesent luptatum zzril delenit augue duis dolore te
+	feugait nulla facilisi.
+	EOF
+
+	cat >failmail <<-\EOF &&
+	From foo@example.com Fri May 23 10:43:49 2008
+	From:	foo@example.com
+	To:	bar@example.com
+	Subject: Re: [RFC/PATCH] git-foo.sh
+	Date:	Fri, 23 May 2008 05:23:42 +0200
+
+	Sometimes we have to find out that there'\''s nothing left.
+
+	EOF
+
+	cat >pine <<-\EOF &&
+	From MAILER-DAEMON Fri May 23 10:43:49 2008
+	Date: 23 May 2008 05:23:42 +0200
+	From: Mail System Internal Data <MAILER-DAEMON@example.com>
+	Subject: DON'\''T DELETE THIS MESSAGE -- FOLDER INTERNAL DATA
+	Message-ID: <foo-0001@example.com>
+
+	This text is part of the internal format of your mail folder, and is not
+	a real message.  It is created automatically by the mail system software.
+	If deleted, important folder data will be lost, and it will be re-created
+	with the data reset to initial values.
+
+	EOF
+
+	signoff="Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+'
 
 test_expect_success setup '
 	echo hello >file &&
@@ -71,11 +76,13 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m first &&
 	git tag first &&
+
 	echo world >>file &&
 	git add file &&
 	test_tick &&
 	git commit -s -F msg &&
 	git tag second &&
+
 	git format-patch --stdout first >patch1 &&
 	{
 		echo "X-Fake-Field: Line One" &&
@@ -89,33 +96,37 @@ test_expect_success setup '
 		echo "X-Fake-Field: Line Three" &&
 		git format-patch --stdout first | sed -e "1d"
 	} | append_cr >patch1-crlf.eml &&
+
 	sed -n -e "3,\$p" msg >file &&
 	git add file &&
 	test_tick &&
 	git commit -m third &&
+
 	git format-patch --stdout first >patch2	&&
+
 	git checkout -b lorem &&
 	sed -n -e "11,\$p" msg >file &&
 	head -n 9 msg >>file &&
 	test_tick &&
 	git commit -a -m "moved stuff" &&
+
 	echo goodbye >another &&
 	git add another &&
 	test_tick &&
 	git commit -m "added another file" &&
-	git format-patch --stdout master >lorem-move.patch
-'
 
-# reset time
-unset test_tick
-test_tick
+	git format-patch --stdout master >lorem-move.patch &&
+	# reset time
+	unset test_tick &&
+	test_tick
+'
 
 test_expect_success 'am applies patch correctly' '
 	git checkout first &&
 	test_tick &&
 	git am <patch1 &&
 	! test -d .git/rebase-apply &&
-	test -z "$(git diff second)" &&
+	git diff --exit-code second &&
 	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
 '
@@ -124,7 +135,7 @@ test_expect_success 'am applies patch e-mail not in a mbox' '
 	git checkout first &&
 	git am patch1.eml &&
 	! test -d .git/rebase-apply &&
-	test -z "$(git diff second)" &&
+	git diff --exit-code second &&
 	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
 '
@@ -133,20 +144,23 @@ test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '
 	git checkout first &&
 	git am patch1-crlf.eml &&
 	! test -d .git/rebase-apply &&
-	test -z "$(git diff second)" &&
+	git diff --exit-code second &&
 	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
 '
 
-GIT_AUTHOR_NAME="Another Thor"
-GIT_AUTHOR_EMAIL="a.thor@example.com"
-GIT_COMMITTER_NAME="Co M Miter"
-GIT_COMMITTER_EMAIL="c.miter@example.com"
-export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL
+test_expect_success 'setup: new author and committer' '
+	GIT_AUTHOR_NAME="Another Thor" &&
+	GIT_AUTHOR_EMAIL="a.thor@example.com" &&
+	GIT_COMMITTER_NAME="Co M Miter" &&
+	GIT_COMMITTER_EMAIL="c.miter@example.com" &&
+	export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL
+'
 
 compare () {
-	test "$(git cat-file commit "$2" | grep "^$1 ")" = \
-	     "$(git cat-file commit "$3" | grep "^$1 ")"
+	a=$(git cat-file commit "$2" | grep "^$1 ") &&
+	b=$(git cat-file commit "$3" | grep "^$1 ") &&
+	test "$a" = "$b"
 }
 
 test_expect_success 'am changes committer and keeps author' '
@@ -155,8 +169,8 @@ test_expect_success 'am changes committer and keeps author' '
 	git am patch2 &&
 	! test -d .git/rebase-apply &&
 	test "$(git rev-parse master^^)" = "$(git rev-parse HEAD^^)" &&
-	test -z "$(git diff master..HEAD)" &&
-	test -z "$(git diff master^..HEAD^)" &&
+	git diff --exit-code master..HEAD &&
+	git diff --exit-code master^..HEAD^ &&
 	compare author master HEAD &&
 	compare author master^ HEAD^ &&
 	test "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" = \
@@ -166,16 +180,19 @@ test_expect_success 'am changes committer and keeps author' '
 test_expect_success 'am --signoff adds Signed-off-by: line' '
 	git checkout -b master2 first &&
 	git am --signoff <patch2 &&
+	printf "%s\n" "$signoff" >expected &&
 	echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >>expected &&
 	git cat-file commit HEAD^ | grep "Signed-off-by:" >actual &&
-	test_cmp actual expected &&
+	test_cmp expected actual &&
 	echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >expected &&
 	git cat-file commit HEAD | grep "Signed-off-by:" >actual &&
-	test_cmp actual expected
+	test_cmp expected actual
 '
 
 test_expect_success 'am stays in branch' '
-	test "refs/heads/master2" = "$(git symbolic-ref HEAD)"
+	echo refs/heads/master2 >expected &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expected actual
 '
 
 test_expect_success 'am --signoff does not add Signed-off-by: line if already there' '
@@ -183,19 +200,22 @@ test_expect_success 'am --signoff does not add Signed-off-by: line if already th
 	sed -e "/^Subject/ s,\[PATCH,Re: Re: Re: & 1/5 v2," patch3 >patch4
 	git checkout HEAD^ &&
 	git am --signoff patch4 &&
-	test "$(git cat-file commit HEAD | grep -c "^Signed-off-by:")" -eq 1
+	git cat-file commit HEAD >actual &&
+	test $(grep -c "^Signed-off-by:" actual) -eq 1
 '
 
 test_expect_success 'am without --keep removes Re: and [PATCH] stuff' '
-	test "$(git rev-parse HEAD)" = "$(git rev-parse master2)"
+	git rev-parse HEAD >expected &&
+	git rev-parse master2 >actual &&
+	test_cmp expected actual
 '
 
 test_expect_success 'am --keep really keeps the subject' '
 	git checkout HEAD^ &&
 	git am --keep patch4 &&
 	! test -d .git/rebase-apply &&
-	git cat-file commit HEAD |
-		fgrep "Re: Re: Re: [PATCH 1/5 v2] third"
+	git cat-file commit HEAD >actual &&
+	grep "Re: Re: Re: \[PATCH 1/5 v2\] third" actual
 '
 
 test_expect_success 'am -3 falls back to 3-way merge' '
@@ -207,7 +227,7 @@ test_expect_success 'am -3 falls back to 3-way merge' '
 	git commit -m "copied stuff" &&
 	git am -3 lorem-move.patch &&
 	! test -d .git/rebase-apply &&
-	test -z "$(git diff lorem)"
+	git diff --exit-code lorem
 '
 
 test_expect_success 'am -3 -q is quiet' '
@@ -217,7 +237,7 @@ test_expect_success 'am -3 -q is quiet' '
 	git add file &&
 	test_tick &&
 	git commit -m "copied stuff" &&
-	git am -3 -q lorem-move.patch > output.out 2>&1 &&
+	git am -3 -q lorem-move.patch >output.out 2>&1 &&
 	! test -s output.out
 '
 
@@ -228,13 +248,15 @@ test_expect_success 'am pauses on conflict' '
 '
 
 test_expect_success 'am --skip works' '
+	echo goodbye >expected &&
 	git am --skip &&
 	! test -d .git/rebase-apply &&
-	test -z "$(git diff lorem2^^ -- file)" &&
-	test goodbye = "$(cat another)"
+	git diff --exit-code lorem2^^ -- file &&
+	test_cmp expected another
 '
 
 test_expect_success 'am --resolved works' '
+	echo goodbye >expected &&
 	git checkout lorem2^^ &&
 	test_must_fail git am lorem-move.patch &&
 	test -d .git/rebase-apply &&
@@ -242,14 +264,14 @@ test_expect_success 'am --resolved works' '
 	git add file &&
 	git am --resolved &&
 	! test -d .git/rebase-apply &&
-	test goodbye = "$(cat another)"
+	test_cmp expected another
 '
 
 test_expect_success 'am takes patches from a Pine mailbox' '
 	git checkout first &&
 	cat pine patch1 | git am &&
 	! test -d .git/rebase-apply &&
-	test -z "$(git diff master^..HEAD)"
+	git diff --exit-code master^..HEAD
 '
 
 test_expect_success 'am fails on mail without patch' '
@@ -272,7 +294,7 @@ test_expect_success 'am works from stdin in subdirectory' '
 		cd subdir &&
 		git am <../patch1
 	) &&
-	test -z "$(git diff second)"
+	git diff --exit-code second
 '
 
 test_expect_success 'am works from file (relative path given) in subdirectory' '
@@ -283,7 +305,7 @@ test_expect_success 'am works from file (relative path given) in subdirectory' '
 		cd subdir &&
 		git am ../patch1
 	) &&
-	test -z "$(git diff second)"
+	git diff --exit-code second
 '
 
 test_expect_success 'am works from file (absolute path given) in subdirectory' '
@@ -295,7 +317,7 @@ test_expect_success 'am works from file (absolute path given) in subdirectory' '
 		cd subdir &&
 		git am "$P/patch1"
 	) &&
-	test -z "$(git diff second)"
+	git diff --exit-code second
 '
 
 test_expect_success 'am --committer-date-is-author-date' '
@@ -303,9 +325,9 @@ test_expect_success 'am --committer-date-is-author-date' '
 	test_tick &&
 	git am --committer-date-is-author-date patch1 &&
 	git cat-file commit HEAD | sed -e "/^\$/q" >head1 &&
-	at=$(sed -ne "/^author /s/.*> //p" head1) &&
-	ct=$(sed -ne "/^committer /s/.*> //p" head1) &&
-	test "$at" = "$ct"
+	sed -ne "/^author /s/.*> //p" head1 >at &&
+	sed -ne "/^committer /s/.*> //p" head1 >ct &&
+	test_cmp at ct
 '
 
 test_expect_success 'am without --committer-date-is-author-date' '
@@ -313,9 +335,9 @@ test_expect_success 'am without --committer-date-is-author-date' '
 	test_tick &&
 	git am patch1 &&
 	git cat-file commit HEAD | sed -e "/^\$/q" >head1 &&
-	at=$(sed -ne "/^author /s/.*> //p" head1) &&
-	ct=$(sed -ne "/^committer /s/.*> //p" head1) &&
-	test "$at" != "$ct"
+	sed -ne "/^author /s/.*> //p" head1 >at &&
+	sed -ne "/^committer /s/.*> //p" head1 >ct &&
+	! test_cmp at ct
 '
 
 # This checks for +0000 because TZ is set to UTC and that should
@@ -327,37 +349,39 @@ test_expect_success 'am --ignore-date' '
 	test_tick &&
 	git am --ignore-date patch1 &&
 	git cat-file commit HEAD | sed -e "/^\$/q" >head1 &&
-	at=$(sed -ne "/^author /s/.*> //p" head1) &&
-	echo "$at" | grep "+0000"
+	sed -ne "/^author /s/.*> //p" head1 >at &&
+	grep "+0000" at
 '
 
 test_expect_success 'am into an unborn branch' '
+	git rev-parse first^{tree} >expected &&
 	rm -fr subdir &&
-	mkdir -p subdir &&
+	mkdir subdir &&
 	git format-patch --numbered-files -o subdir -1 first &&
 	(
 		cd subdir &&
 		git init &&
 		git am 1
 	) &&
-	result=$(
-		cd subdir && git rev-parse HEAD^{tree}
+	(
+		cd subdir &&
+		git rev-parse HEAD^{tree} >../actual
 	) &&
-	test "z$result" = "z$(git rev-parse first^{tree})"
+	test_cmp expected actual
 '
 
 test_expect_success 'am newline in subject' '
 	git checkout first &&
 	test_tick &&
-	sed -e "s/second/second \\\n foo/" patch1 > patchnl &&
-	git am < patchnl > output.out 2>&1 &&
+	sed -e "s/second/second \\\n foo/" patch1 >patchnl &&
+	git am <patchnl >output.out 2>&1 &&
 	grep "^Applying: second \\\n foo$" output.out
 '
 
 test_expect_success 'am -q is quiet' '
 	git checkout first &&
 	test_tick &&
-	git am -q < patch1 > output.out 2>&1 &&
+	git am -q <patch1 >output.out 2>&1 &&
 	! test -s output.out
 '
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e5523dd..cb15798 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -257,6 +257,10 @@ q_to_cr () {
 	tr Q '\015'
 }
 
+q_to_tab () {
+	tr Q '\011'
+}
+
 append_cr () {
 	sed -e 's/$/Q/' | tr Q '\015'
 }
-- 
1.7.2.rc3

[PATCH 2/5] t4150 (am): futureproof against failing tests

[Jonathan Nieder]

Most tests in t4150 begin by navigating to a sane state and
applying some patch:

	git checkout first &&
	git am patch1

If a previous test left behind unmerged files or a .git/rebase-apply
directory, they are untouched and the test fails, causing later tests
to fail, too.  This is not a problem in practice because none of the
tests leave a mess behind.

But as a futureproofing measure, it is still best to avoid the problem
and clean up at the start of each test.  In particular, this
simplifies the process of adding new tests that are known to fail.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4150-am.sh |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index b50aad3..70b57de 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -122,6 +122,8 @@ test_expect_success setup '
 '
 
 test_expect_success 'am applies patch correctly' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	test_tick &&
 	git am <patch1 &&
@@ -132,6 +134,8 @@ test_expect_success 'am applies patch correctly' '
 '
 
 test_expect_success 'am applies patch e-mail not in a mbox' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	git am patch1.eml &&
 	! test -d .git/rebase-apply &&
@@ -141,6 +145,8 @@ test_expect_success 'am applies patch e-mail not in a mbox' '
 '
 
 test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	git am patch1-crlf.eml &&
 	! test -d .git/rebase-apply &&
@@ -165,6 +171,8 @@ compare () {
 
 test_expect_success 'am changes committer and keeps author' '
 	test_tick &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	git am patch2 &&
 	! test -d .git/rebase-apply &&
@@ -178,6 +186,8 @@ test_expect_success 'am changes committer and keeps author' '
 '
 
 test_expect_success 'am --signoff adds Signed-off-by: line' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout -b master2 first &&
 	git am --signoff <patch2 &&
 	printf "%s\n" "$signoff" >expected &&
@@ -198,6 +208,8 @@ test_expect_success 'am stays in branch' '
 test_expect_success 'am --signoff does not add Signed-off-by: line if already there' '
 	git format-patch --stdout HEAD^ >patch3 &&
 	sed -e "/^Subject/ s,\[PATCH,Re: Re: Re: & 1/5 v2," patch3 >patch4
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout HEAD^ &&
 	git am --signoff patch4 &&
 	git cat-file commit HEAD >actual &&
@@ -211,6 +223,8 @@ test_expect_success 'am without --keep removes Re: and [PATCH] stuff' '
 '
 
 test_expect_success 'am --keep really keeps the subject' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout HEAD^ &&
 	git am --keep patch4 &&
 	! test -d .git/rebase-apply &&
@@ -219,6 +233,8 @@ test_expect_success 'am --keep really keeps the subject' '
 '
 
 test_expect_success 'am -3 falls back to 3-way merge' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout -b lorem2 master2 &&
 	sed -n -e "3,\$p" msg >file &&
 	head -n 9 msg >>file &&
@@ -231,6 +247,7 @@ test_expect_success 'am -3 falls back to 3-way merge' '
 '
 
 test_expect_success 'am -3 -q is quiet' '
+	rm -fr .git/rebase-apply &&
 	git reset master2 --hard &&
 	sed -n -e "3,\$p" msg >file &&
 	head -n 9 msg >>file &&
@@ -242,6 +259,8 @@ test_expect_success 'am -3 -q is quiet' '
 '
 
 test_expect_success 'am pauses on conflict' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout lorem2^^ &&
 	test_must_fail git am lorem-move.patch &&
 	test -d .git/rebase-apply
@@ -257,6 +276,8 @@ test_expect_success 'am --skip works' '
 
 test_expect_success 'am --resolved works' '
 	echo goodbye >expected &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout lorem2^^ &&
 	test_must_fail git am lorem-move.patch &&
 	test -d .git/rebase-apply &&
@@ -268,6 +289,8 @@ test_expect_success 'am --resolved works' '
 '
 
 test_expect_success 'am takes patches from a Pine mailbox' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	cat pine patch1 | git am &&
 	! test -d .git/rebase-apply &&
@@ -275,11 +298,16 @@ test_expect_success 'am takes patches from a Pine mailbox' '
 '
 
 test_expect_success 'am fails on mail without patch' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	test_must_fail git am <failmail &&
-	rm -r .git/rebase-apply/
+	git am --abort &&
+	! test -d .git/rebase-apply
 '
 
 test_expect_success 'am fails on empty patch' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	echo "---" >>failmail &&
 	test_must_fail git am <failmail &&
 	git am --skip &&
@@ -288,6 +316,8 @@ test_expect_success 'am fails on empty patch' '
 
 test_expect_success 'am works from stdin in subdirectory' '
 	rm -fr subdir &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	(
 		mkdir -p subdir &&
@@ -299,6 +329,8 @@ test_expect_success 'am works from stdin in subdirectory' '
 
 test_expect_success 'am works from file (relative path given) in subdirectory' '
 	rm -fr subdir &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	(
 		mkdir -p subdir &&
@@ -310,6 +342,8 @@ test_expect_success 'am works from file (relative path given) in subdirectory' '
 
 test_expect_success 'am works from file (absolute path given) in subdirectory' '
 	rm -fr subdir &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	P=$(pwd) &&
 	(
@@ -321,6 +355,8 @@ test_expect_success 'am works from file (absolute path given) in subdirectory' '
 '
 
 test_expect_success 'am --committer-date-is-author-date' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	test_tick &&
 	git am --committer-date-is-author-date patch1 &&
@@ -331,6 +367,8 @@ test_expect_success 'am --committer-date-is-author-date' '
 '
 
 test_expect_success 'am without --committer-date-is-author-date' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	test_tick &&
 	git am patch1 &&
@@ -345,6 +383,8 @@ test_expect_success 'am without --committer-date-is-author-date' '
 # by test_tick that uses -0700 timezone; if this feature does not
 # work, we will see that instead of +0000.
 test_expect_success 'am --ignore-date' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	test_tick &&
 	git am --ignore-date patch1 &&
@@ -355,6 +395,8 @@ test_expect_success 'am --ignore-date' '
 
 test_expect_success 'am into an unborn branch' '
 	git rev-parse first^{tree} >expected &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	rm -fr subdir &&
 	mkdir subdir &&
 	git format-patch --numbered-files -o subdir -1 first &&
@@ -371,6 +413,8 @@ test_expect_success 'am into an unborn branch' '
 '
 
 test_expect_success 'am newline in subject' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	test_tick &&
 	sed -e "s/second/second \\\n foo/" patch1 >patchnl &&
@@ -379,6 +423,8 @@ test_expect_success 'am newline in subject' '
 '
 
 test_expect_success 'am -q is quiet' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
 	git checkout first &&
 	test_tick &&
 	git am -q <patch1 >output.out 2>&1 &&
-- 
1.7.2.rc3

[PATCH 3/5] Teach "apply --index-info" to handle rename patches

[Jonathan Nieder]

From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 10 Nov 2008 15:49:03 -0800

With v1.5.3.2~14 (apply --index-info: fall back to current index for
mode changes, 2007-09-17), git apply learned to stop worrying
about the lack of diff index line when a file already present in the
current index had no content change.

But it still worries too much: for rename patches, it is checking
that both the old and new filename are present in the current
index.  This makes no sense, since a file rename generally
involves creating a file there was none before.

So just check the old filename.

Noticed while trying to use “git rebase” with diff.renames = copies.

[jn: add tests]

Reported-by: David D. Kilzer <ddkilzer@kilzer.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/apply.c |    3 +--
 t/t4150-am.sh   |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 12ef9ea..f38c1f7 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2979,8 +2979,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 		else if (get_sha1(patch->old_sha1_prefix, sha1))
 			/* git diff has no index line for mode/type changes */
 			if (!patch->lines_added && !patch->lines_deleted) {
-				if (get_current_sha1(patch->new_name, sha1) ||
-				    get_current_sha1(patch->old_name, sha1))
+				if (get_current_sha1(patch->old_name, sha1))
 					die("mode change for %s, which is not "
 						"in current HEAD", name);
 				sha1_ptr = sha1;
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 70b57de..1c3d8ed 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -116,6 +116,18 @@ test_expect_success setup '
 	git commit -m "added another file" &&
 
 	git format-patch --stdout master >lorem-move.patch &&
+
+	git checkout -b rename &&
+	git mv file renamed &&
+	git commit -m "renamed a file" &&
+
+	git format-patch -M --stdout lorem >rename.patch &&
+
+	git reset --soft lorem^ &&
+	git commit -m "renamed a file and added another" &&
+
+	git format-patch -M --stdout lorem^ >rename-add.patch &&
+
 	# reset time
 	unset test_tick &&
 	test_tick
@@ -246,8 +258,42 @@ test_expect_success 'am -3 falls back to 3-way merge' '
 	git diff --exit-code lorem
 '
 
+test_expect_success 'am can rename a file' '
+	grep "^rename from" rename.patch &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	git checkout lorem^0 &&
+	git am rename.patch &&
+	! test -d .git/rebase-apply &&
+	git update-index --refresh &&
+	git diff --exit-code rename
+'
+
+test_expect_success 'am -3 can rename a file' '
+	grep "^rename from" rename.patch &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	git checkout lorem^0 &&
+	git am -3 rename.patch &&
+	! test -d .git/rebase-apply &&
+	git update-index --refresh &&
+	git diff --exit-code rename
+'
+
+test_expect_success 'am -3 can rename a file after falling back to 3-way merge' '
+	grep "^rename from" rename-add.patch &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	git checkout lorem^0 &&
+	git am -3 rename-add.patch &&
+	! test -d .git/rebase-apply &&
+	git update-index --refresh &&
+	git diff --exit-code rename
+'
+
 test_expect_success 'am -3 -q is quiet' '
 	rm -fr .git/rebase-apply &&
+	git checkout -f lorem2 &&
 	git reset master2 --hard &&
 	sed -n -e "3,\$p" msg >file &&
 	head -n 9 msg >>file &&
-- 
1.7.2.rc3

[PATCH 4/5] t3400 (rebase): whitespace cleanup

[Jonathan Nieder]

This test used 5-space indents since it was added in 2005, but
recently the temptation to use tabs to indent has been too
strong, resulting in uneven whitespace.  Switch over completely
to tabs.

While at it, use a more modern style for consistency with other
tests:

 - names of tests go on the same line as test_expect_success;
 - extra whitespace after > redirection operators is removed.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t3400-rebase.sh |  182 +++++++++++++++++++++++++++--------------------------
 1 files changed, 92 insertions(+), 90 deletions(-)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index d98c7b5..083d768 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -14,140 +14,142 @@ GIT_AUTHOR_NAME=author@name
 GIT_AUTHOR_EMAIL=bogus@email@address
 export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
 
-test_expect_success \
-    'prepare repository with topic branches' \
-    'git config core.logAllRefUpdates true &&
-     echo First > A &&
-     git update-index --add A &&
-     git commit -m "Add A." &&
-     git checkout -b my-topic-branch &&
-     echo Second > B &&
-     git update-index --add B &&
-     git commit -m "Add B." &&
-     git checkout -f master &&
-     echo Third >> A &&
-     git update-index A &&
-     git commit -m "Modify A." &&
-     git checkout -b side my-topic-branch &&
-     echo Side >> C &&
-     git add C &&
-     git commit -m "Add C" &&
-     git checkout -b nonlinear my-topic-branch &&
-     echo Edit >> B &&
-     git add B &&
-     git commit -m "Modify B" &&
-     git merge side &&
-     git checkout -b upstream-merged-nonlinear &&
-     git merge master &&
-     git checkout -f my-topic-branch &&
-     git tag topic
+test_expect_success 'prepare repository with topic branches' '
+	git config core.logAllRefUpdates true &&
+	echo First >A &&
+	git update-index --add A &&
+	git commit -m "Add A." &&
+	git checkout -b my-topic-branch &&
+	echo Second >B &&
+	git update-index --add B &&
+	git commit -m "Add B." &&
+	git checkout -f master &&
+	echo Third >>A &&
+	git update-index A &&
+	git commit -m "Modify A." &&
+	git checkout -b side my-topic-branch &&
+	echo Side >>C &&
+	git add C &&
+	git commit -m "Add C" &&
+	git checkout -b nonlinear my-topic-branch &&
+	echo Edit >>B &&
+	git add B &&
+	git commit -m "Modify B" &&
+	git merge side &&
+	git checkout -b upstream-merged-nonlinear &&
+	git merge master &&
+	git checkout -f my-topic-branch &&
+	git tag topic
 '
 
 test_expect_success 'rebase on dirty worktree' '
-     echo dirty >> A &&
-     test_must_fail git rebase master'
+	echo dirty >>A &&
+	test_must_fail git rebase master
+'
 
 test_expect_success 'rebase on dirty cache' '
-     git add A &&
-     test_must_fail git rebase master'
+	git add A &&
+	test_must_fail git rebase master
+'
 
 test_expect_success 'rebase against master' '
-     git reset --hard HEAD &&
-     git rebase master'
+	git reset --hard HEAD &&
+	git rebase master
+'
 
 test_expect_success 'rebase against master twice' '
-     git rebase master >out &&
-     grep "Current branch my-topic-branch is up to date" out
+	git rebase master >out &&
+	grep "Current branch my-topic-branch is up to date" out
 '
 
 test_expect_success 'rebase against master twice with --force' '
-     git rebase --force-rebase master >out &&
-     grep "Current branch my-topic-branch is up to date, rebase forced" out
+	git rebase --force-rebase master >out &&
+	grep "Current branch my-topic-branch is up to date, rebase forced" out
 '
 
 test_expect_success 'rebase against master twice from another branch' '
-     git checkout my-topic-branch^ &&
-     git rebase master my-topic-branch >out &&
-     grep "Current branch my-topic-branch is up to date" out
+	git checkout my-topic-branch^ &&
+	git rebase master my-topic-branch >out &&
+	grep "Current branch my-topic-branch is up to date" out
 '
 
 test_expect_success 'rebase fast-forward to master' '
-     git checkout my-topic-branch^ &&
-     git rebase my-topic-branch >out &&
-     grep "Fast-forwarded HEAD to my-topic-branch" out
+	git checkout my-topic-branch^ &&
+	git rebase my-topic-branch >out &&
+	grep "Fast-forwarded HEAD to my-topic-branch" out
 '
 
-test_expect_success \
-    'the rebase operation should not have destroyed author information' \
-    '! (git log | grep "Author:" | grep "<>")'
+test_expect_success 'the rebase operation should not have destroyed author information' '
+	! (git log | grep "Author:" | grep "<>")
+'
 
-test_expect_success \
-    'the rebase operation should not have destroyed author information (2)' \
-    "git log -1 | grep 'Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>'"
+test_expect_success 'the rebase operation should not have destroyed author information (2)' "
+	git log -1 |
+	grep 'Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>'
+"
 
 test_expect_success 'HEAD was detached during rebase' '
-     test $(git rev-parse HEAD@{1}) != $(git rev-parse my-topic-branch@{1})
+	test $(git rev-parse HEAD@{1}) != $(git rev-parse my-topic-branch@{1})
 '
 
 test_expect_success 'rebase after merge master' '
-     git reset --hard topic &&
-     git merge master &&
-     git rebase master &&
-     ! (git show | grep "^Merge:")
+	git reset --hard topic &&
+	git merge master &&
+	git rebase master &&
+	! (git show | grep "^Merge:")
 '
 
 test_expect_success 'rebase of history with merges is linearized' '
-     git checkout nonlinear &&
-     test 4 = $(git rev-list master.. | wc -l) &&
-     git rebase master &&
-     test 3 = $(git rev-list master.. | wc -l)
+	git checkout nonlinear &&
+	test 4 = $(git rev-list master.. | wc -l) &&
+	git rebase master &&
+	test 3 = $(git rev-list master.. | wc -l)
 '
 
-test_expect_success \
-    'rebase of history with merges after upstream merge is linearized' '
-     git checkout upstream-merged-nonlinear &&
-     test 5 = $(git rev-list master.. | wc -l) &&
-     git rebase master &&
-     test 3 = $(git rev-list master.. | wc -l)
+test_expect_success 'rebase of history with merges after upstream merge is linearized' '
+	git checkout upstream-merged-nonlinear &&
+	test 5 = $(git rev-list master.. | wc -l) &&
+	git rebase master &&
+	test 3 = $(git rev-list master.. | wc -l)
 '
 
 test_expect_success 'rebase a single mode change' '
-     git checkout master &&
-     echo 1 > X &&
-     git add X &&
-     test_tick &&
-     git commit -m prepare &&
-     git checkout -b modechange HEAD^ &&
-     echo 1 > X &&
-     git add X &&
-     test_chmod +x A &&
-     test_tick &&
-     git commit -m modechange &&
-     GIT_TRACE=1 git rebase master
+	git checkout master &&
+	echo 1 >X &&
+	git add X &&
+	test_tick &&
+	git commit -m prepare &&
+	git checkout -b modechange HEAD^ &&
+	echo 1 >X &&
+	git add X &&
+	test_chmod +x A &&
+	test_tick &&
+	git commit -m modechange &&
+	GIT_TRACE=1 git rebase master
 '
 
 test_expect_success 'Show verbose error when HEAD could not be detached' '
-     : > B &&
-     test_must_fail git rebase topic 2> output.err > output.out &&
-     grep "Untracked working tree file .B. would be overwritten" output.err
+	>B &&
+	test_must_fail git rebase topic 2>output.err >output.out &&
+	grep "Untracked working tree file .B. would be overwritten" output.err
 '
 rm -f B
 
 test_expect_success 'dump usage when upstream arg is missing' '
-     git checkout -b usage topic &&
-     test_must_fail git rebase 2>error1 &&
-     grep "[Uu]sage" error1 &&
-     test_must_fail git rebase --abort 2>error2 &&
-     grep "No rebase in progress" error2 &&
-     test_must_fail git rebase --onto master 2>error3 &&
-     grep "[Uu]sage" error3 &&
-     ! grep "can.t shift" error3
+	git checkout -b usage topic &&
+	test_must_fail git rebase 2>error1 &&
+	grep "[Uu]sage" error1 &&
+	test_must_fail git rebase --abort 2>error2 &&
+	grep "No rebase in progress" error2 &&
+	test_must_fail git rebase --onto master 2>error3 &&
+	grep "[Uu]sage" error3 &&
+	! grep "can.t shift" error3
 '
 
 test_expect_success 'rebase -q is quiet' '
-     git checkout -b quiet topic &&
-     git rebase -q master > output.out 2>&1 &&
-     test ! -s output.out
+	git checkout -b quiet topic &&
+	git rebase -q master >output.out 2>&1 &&
+	test ! -s output.out
 '
 
 test_expect_success 'Rebase a commit that sprinkles CRs in' '
-- 
1.7.2.rc3

[PATCH 5/5] rebase: protect against diff.renames configuration

[Jonathan Nieder]

From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 10 Nov 2008 16:15:49 -0800

We currently do not disable diff.renames configuration while rebase
internally runs "format-patch" to feed "am -3".

The end user configuration for "diff" should not affect the result
produced by the higher level command that is related to "diff" only
because internally it is implemented in terms of it.

For that matter, I have a feeling that format-patch should not even look
at diff.renames, but we seem to have been doing this for a long time so
there is no easy way to fix this thinko.

In any case, here is a much straightforward fix for "rebase".

[jn: with test case from David]

Reported-by: David D. Kilzer <ddkilzer@kilzer.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-rebase.sh     |    2 +-
 t/t3400-rebase.sh |   24 +++++++++++++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index ab4afa7..386be43 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -543,7 +543,7 @@ fi
 if test -z "$do_merge"
 then
 	git format-patch -k --stdout --full-index --ignore-if-in-upstream \
-		$root_flag "$revisions" |
+		--no-renames $root_flag "$revisions" |
 	git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" &&
 	move_to_original_branch
 	ret=$?
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 083d768..a19aeb6 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -19,7 +19,16 @@ test_expect_success 'prepare repository with topic branches' '
 	echo First >A &&
 	git update-index --add A &&
 	git commit -m "Add A." &&
-	git checkout -b my-topic-branch &&
+	git checkout -b force-3way &&
+	echo Dummy >Y &&
+	git update-index --add Y &&
+	git commit -m "Add Y." &&
+	git checkout -b filemove &&
+	git reset --soft master &&
+	mkdir D &&
+	git mv A D/A &&
+	git commit -m "Move A." &&
+	git checkout -b my-topic-branch master &&
 	echo Second >B &&
 	git update-index --add B &&
 	git commit -m "Add B." &&
@@ -128,6 +137,19 @@ test_expect_success 'rebase a single mode change' '
 	GIT_TRACE=1 git rebase master
 '
 
+test_expect_success 'rebase is not broken by diff.renames' '
+	git config diff.renames copies &&
+	test_when_finished "git config --unset diff.renames" &&
+	git checkout filemove &&
+	GIT_TRACE=1 git rebase force-3way
+'
+
+test_expect_success 'setup: recover' '
+	test_might_fail git rebase --abort &&
+	git reset --hard &&
+	git checkout modechange
+'
+
 test_expect_success 'Show verbose error when HEAD could not be detached' '
 	>B &&
 	test_must_fail git rebase topic 2>output.err >output.out &&
-- 
1.7.2.rc3

Re: [PATCH 0/5] Fix rebase with file move when diff.renames = copies

[David D. Kilzer]

On Fri, July 23, 2010 at 10:01:03 AM, Jonathan Nieder wrote:


> David D. Kilzer wrote:
> 
> > My original patch in  <http://marc.info/?l=git&m=122635667614099&w=2> 
>addressed 
>
> > this in  builtin-apply.c, but Junio didn't like this approach as noted in 
> >  <http://marc.info/?l=git&m=122636097120953&w=2>.
> 
> Got it.  This  patch just treats the symptoms in my opinion, and if
> you read Junio’s message  carefully, I think he was also suggesting
> that git apply should still be  fixed.


Thanks!  At the time I needed to fix the issue and continue working.  I didn't 
have time to investigate it further (which is why it sat for about 18 months).

> Something like this series would fix both.  Please feel free to pick
> it up and take it in whatever direction you like.


Is this comment to me or Junio?  As a part-time contributor, I'm not sure what 
my options are here.  :)

> Hope that  helps.

Yes!  Thanks again!

Dave

Re: [PATCH 5/5] rebase: protect against diff.renames configuration

[Sverre Rabbelier]

Heya,

On Fri, Jul 23, 2010 at 12:06, Jonathan Nieder <jrnieder@gmail.com> wrote:
> The end user configuration for "diff" should not affect the result
> produced by the higher level command that is related to "diff" only
> because internally it is implemented in terms of it.

Almost completely unrelated and perhaps not relevant, I seem to recall
that if you set 'ui.color' to 'always' you will get unapplyable
patches because 'git format-patch' will include the color in it's
output. Perhaps it should --no-color as well, while we're fixing it?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 5/5] rebase: protect against diff.renames configuration

[Sverre Rabbelier]

Heya,

On Fri, Jul 23, 2010 at 12:06, Jonathan Nieder <jrnieder@gmail.com> wrote:
> The end user configuration for "diff" should not affect the result
> produced by the higher level command that is related to "diff" only
> because internally it is implemented in terms of it.

Almost completely unrelated and perhaps not relevant, I seem to recall
that if you set 'ui.color' to 'always' you will get unapplyable
patches because 'git format-patch' will include the color in it's
output. Perhaps it should --no-color as well, while we're fixing it?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 5/5] rebase: protect against diff.renames configuration

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Fri, Jul 23, 2010 at 12:06, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> The end user configuration for "diff" should not affect the result
>> produced by the higher level command that is related to "diff" only
>> because internally it is implemented in terms of it.
>
> Almost completely unrelated and perhaps not relevant, I seem to recall
> that if you set 'ui.color' to 'always' you will get unapplyable
> patches because 'git format-patch' will include the color in it's
> output. Perhaps it should --no-color as well, while we're fixing it?

I think that it actually was a mistake for git_diff_basic_config() to call
git_color_default_config().  Parsing of diff.color.<slot> there is Ok and
was justified by 9a1805a (add a "basic" diff config callback, 2008-01-04),
but the change made to the function with 6b2f2d9 (Add color.ui variable
which globally enables colorization if set, 2008-02-18) was probably a
screw-up.  The call should instead have gone to diff_ui_config().

I also think format_config() should be changed not to call log_config()
(we need to move parsing of the format.subjectprefix from the latter to
the former), and instead call diff_basic_config() directly.

Re: [PATCH 0/5] Fix rebase with file move when diff.renames = copies

[Jonathan Nieder]

David D. Kilzer wrote:
> On Fri, July 23, 2010 at 10:01:03 AM, Jonathan Nieder wrote:

>> Something like this series would fix both.  Please feel free to pick
>> it up and take it in whatever direction you like.
>
> Is this comment to me or Junio?  As a part-time contributor, I'm not sure what 
> my options are here.  :)

It is to the world at large, or more precisely, anyone who is interested.

I only meant that I am not planning to keep track of what happens to
those patches in the future.  Ideally someone else (maybe you ;-))
will take care of pinging if a patch gets forgotten or improving the
patches if some obvious change suggests itself.

Thanks for the initial ping, by the way.