[PATCH] apply: when -R, also reverse list of sections

[PATCH] apply: when -R, also reverse list of sections

[Jonathan Tan]

A patch changing a symlink into a file is written with 2 sections (in
the code, represented as "struct patch"): firstly, the deletion of the
symlink, and secondly, the creation of the file. When applying that
patch with -R, the sections are reversed, so we get:

 (1) creation of a symlink, then
 (2) deletion of a file.

This causes an issue when the "deletion of a file" section is checked,
because Git observes that the so-called file is not a file but a
symlink, resulting in a "wrong type" error message.

What we want is:

 (1) deletion of a file, then
 (2) creation of a symlink.

In the code, this is reflected in the behavior of previous_patch() when
invoked from check_preimage() when the deletion is checked. Creation
then deletion means that when the deletion is checked, previous_patch()
returns the creation section, triggering a mode conflict resulting in
the "wrong type" error message. But deletion then creation means that
when the deletion is checked, previous_patch() returns NULL, so the
deletion mode is checked against lstat, which is what we want.

Therefore, when building the list of sections, build them in reverse
order (by adding to the front of the list instead of the back) when -R
is passed.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
I traced the different behavior in previous_patch() to what the
invocation of to_be_deleted() returns, but I couldn't figure out why it
returns false in the current-but-wrong-order case but true in the
future-and-correct-order case. I see that prepare_fn_table() sets
PATH_TO_BE_DELETED for deletions, but couldn't figure out where and when
it is set to something else. Further compounding my confusion,
conceptually, at the point of checking the deletion, both patches are
(in theory) "to be deleted". Any help in this is appreciated.
---
 apply.c                     | 9 +++++++--
 t/t4114-apply-typechange.sh | 7 +++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index 76dba93c97..359ceb632c 100644
--- a/apply.c
+++ b/apply.c
@@ -4699,8 +4699,13 @@ static int apply_patch(struct apply_state *state,
 			reverse_patches(patch);
 		if (use_patch(state, patch)) {
 			patch_stats(state, patch);
-			*listp = patch;
-			listp = &patch->next;
+			if (!list || !state->apply_in_reverse) {
+				*listp = patch;
+				listp = &patch->next;
+			} else {
+				patch->next = list;
+				list = patch;
+			}
 
 			if ((patch->new_name &&
 			     ends_with_path_components(patch->new_name,
diff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh
index ebadbc347f..da3e64f811 100755
--- a/t/t4114-apply-typechange.sh
+++ b/t/t4114-apply-typechange.sh
@@ -88,6 +88,13 @@ test_expect_success 'symlink becomes file' '
 	'
 test_debug 'cat patch'
 
+test_expect_success 'symlink becomes file, in reverse' '
+	git checkout -f foo-symlinked-to-bar &&
+	git diff-tree -p HEAD foo-back-to-file > patch &&
+	git checkout foo-back-to-file &&
+	git apply -R --index < patch
+	'
+
 test_expect_success 'binary file becomes symlink' '
 	git checkout -f foo-becomes-binary &&
 	git diff-tree -p --binary HEAD foo-symlinked-to-bar > patch &&
-- 
2.28.0.709.gb0816b6eb0-goog

Re: [PATCH] apply: when -R, also reverse list of sections

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> A patch changing a symlink into a file is written with 2 sections (in
> the code, represented as "struct patch"): firstly, the deletion of the
> symlink, and secondly, the creation of the file. When applying that
> patch with -R, the sections are reversed, so we get:
>
>  (1) creation of a symlink, then
>  (2) deletion of a file.

Good observation.

But I have to wonder if it breaks the support for (arguably outside
the Git usecase) input that has more than one patch that touches the
same path to blindly reverse the order of all patches (instead of
the obvious implementation of the fix for the above stated problem
--- i.e. make sure the first patch is a deletion of a symlink and
what immediately follows is a creation of a regular file, and swap
them only in such a case).

Re: [PATCH] apply: when -R, also reverse list of sections

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > A patch changing a symlink into a file is written with 2 sections (in
> > the code, represented as "struct patch"): firstly, the deletion of the
> > symlink, and secondly, the creation of the file. When applying that
> > patch with -R, the sections are reversed, so we get:
> >
> >  (1) creation of a symlink, then
> >  (2) deletion of a file.
> 
> Good observation.
> 
> But I have to wonder if it breaks the support for (arguably outside
> the Git usecase) input that has more than one patch that touches the
> same path to blindly reverse the order of all patches

Sorry for getting back to this so late.

The only other case I can think of (besides symlink<->file) is
directory<->file, and even in that case, I think blindly reversing the
order still works.

If a more sophisticated rearrangement was needed, I would think that
even applying the patches in the forward direction (that is, without
"-R") wouldn't work, since Git is sensitive to the order of the patches.
So I don't think we need to support such input (since they wouldn't work
in the forward direction anyway).

> (instead of
> the obvious implementation of the fix for the above stated problem
> --- i.e. make sure the first patch is a deletion of a symlink and
> what immediately follows is a creation of a regular file, and swap
> them only in such a case).

This would make patch application more robust, but I still appreciate
the relative simplicity of the existing approach - the patches you see
in the input are all applied in order (or reverse order, in the case of
"-R"). You also wouldn't end up in the situation where you think that
your input works because it works with your version of Git, but it
actually doesn't work with an older version of Git or another
implementation. (I haven't researched how other implementation do it,
though.)

Re: [PATCH] apply: when -R, also reverse list of sections

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>> But I have to wonder if it breaks the support for (arguably outside
>> the Git usecase) input that has more than one patch that touches the
>> same path to blindly reverse the order of all patches
>
> Sorry for getting back to this so late.
>
> The only other case I can think of (besides symlink<->file) is
> directory<->file, and even in that case, I think blindly reversing the
> order still works.
>
> If a more sophisticated rearrangement was needed, I would think that
> even applying the patches in the forward direction (that is, without
> "-R") wouldn't work, since Git is sensitive to the order of the patches.
> So I don't think we need to support such input (since they wouldn't work
> in the forward direction anyway).

I wish you told that to those who added fn_table kludge to apply.c
back when they did so.  They apparently wanted to have a patch that
has more than one "diff --git a/hello.c b/hello.c" that talks about
the same file applied with a single invocation of "git apply".
Perhaps what they did is already broken with "apply -R", and blind
reversal of everything magically makes it work?  Or what they did
already works with "apply -R" and your blind reversal would break,
unless you undo what they did?

>> (instead of
>> the obvious implementation of the fix for the above stated problem
>> --- i.e. make sure the first patch is a deletion of a symlink and
>> what immediately follows is a creation of a regular file, and swap
>> them only in such a case).
>
> This would make patch application more robust, but I still appreciate
> the relative simplicity of the existing approach

I'd rather want to see that we keep the normal cases simple,
i.e. majority parts of a patch with "apply -R" that did *not* have
to futz with the application order will keep what we do, and if
there are tricky cases like typechange diff, only special case them.

Thanks.

Re: [PATCH] apply: when -R, also reverse list of sections

[Junio C Hamano]

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

> I wish you told that to those who added fn_table kludge to apply.c
> back when they did so.  They apparently wanted to have a patch that
> has more than one "diff --git a/hello.c b/hello.c" that talks about
> the same file applied with a single invocation of "git apply".
> Perhaps what they did is already broken with "apply -R", and blind
> reversal of everything magically makes it work?  Or what they did
> already works with "apply -R" and your blind reversal would break,
> unless you undo what they did?

;-)  It turns out that it was the former.

Without your "blindly reverse everything" patch, the attached patch
illustrates how the "touch the same path more than once" support
introduced in 7a07841c (git-apply: handle a patch that touches the
same path more than once better, 2008-06-27) is broken with respect
to "apply -R".

So, you should be able to sell the change to fix _two_ bugs ;-)

Thanks.

diff --git a/t/t4127-apply-same-fn.sh b/t/t4127-apply-same-fn.sh
index 972946c174..fa824ac09f 100755
--- a/t/t4127-apply-same-fn.sh
+++ b/t/t4127-apply-same-fn.sh
@@ -30,7 +30,7 @@ test_expect_success 'apply same filename with independent changes' '
 	test_cmp same_fn same_fn2
 '
 
-test_expect_success 'apply same filename with overlapping changes' '
+test_expect_failure 'apply same filename with overlapping changes' '
 	git reset --hard &&
 	modify "s/^d/z/" same_fn &&
 	git diff > patch0 &&
@@ -39,8 +39,13 @@ test_expect_success 'apply same filename with overlapping changes' '
 	git diff >> patch0 &&
 	cp same_fn same_fn2 &&
 	git reset --hard &&
+	cp same_fn same_fn1 &&
+
 	git apply patch0 &&
-	test_cmp same_fn same_fn2
+	test_cmp same_fn same_fn2 &&
+
+	git apply -R patch0 &&
+	test_cmp same_fn same_fn1
 '
 
 test_expect_success 'apply same new filename after rename' '

Re: [PATCH] apply: when -R, also reverse list of sections

[Jonathan Tan]

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I wish you told that to those who added fn_table kludge to apply.c
> > back when they did so.  They apparently wanted to have a patch that
> > has more than one "diff --git a/hello.c b/hello.c" that talks about
> > the same file applied with a single invocation of "git apply".
> > Perhaps what they did is already broken with "apply -R", and blind
> > reversal of everything magically makes it work?  Or what they did
> > already works with "apply -R" and your blind reversal would break,
> > unless you undo what they did?
> 
> ;-)  It turns out that it was the former.
> 
> Without your "blindly reverse everything" patch, the attached patch
> illustrates how the "touch the same path more than once" support
> introduced in 7a07841c (git-apply: handle a patch that touches the
> same path more than once better, 2008-06-27) is broken with respect
> to "apply -R".

Ah, thanks for checking.

> So, you should be able to sell the change to fix _two_ bugs ;-)
> 
> Thanks.
> 
> diff --git a/t/t4127-apply-same-fn.sh b/t/t4127-apply-same-fn.sh
> index 972946c174..fa824ac09f 100755
> --- a/t/t4127-apply-same-fn.sh
> +++ b/t/t4127-apply-same-fn.sh
> @@ -30,7 +30,7 @@ test_expect_success 'apply same filename with independent changes' '
>  	test_cmp same_fn same_fn2
>  '
>  
> -test_expect_success 'apply same filename with overlapping changes' '
> +test_expect_failure 'apply same filename with overlapping changes' '
>  	git reset --hard &&
>  	modify "s/^d/z/" same_fn &&
>  	git diff > patch0 &&
> @@ -39,8 +39,13 @@ test_expect_success 'apply same filename with overlapping changes' '
>  	git diff >> patch0 &&
>  	cp same_fn same_fn2 &&
>  	git reset --hard &&
> +	cp same_fn same_fn1 &&
> +
>  	git apply patch0 &&
> -	test_cmp same_fn same_fn2
> +	test_cmp same_fn same_fn2 &&
> +
> +	git apply -R patch0 &&
> +	test_cmp same_fn same_fn1
>  '
>  
>  test_expect_success 'apply same new filename after rename' '

Indeed, with my patch, this test passes instead of fails.

Should I resend a version 2 that includes this test or will you apply
this to your local copy?

Re: [PATCH] apply: when -R, also reverse list of sections

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Should I resend a version 2 that includes this test or will you apply
> this to your local copy?

The former, as I do not want the final version to be not shown to
the list---we'll miss a good opportunity for other people to spot
issues we didn't think about.

Thanks.

[PATCH v2] apply: when -R, also reverse list of sections

[Jonathan Tan]

A patch changing a symlink into a file is written with 2 sections (in
the code, represented as "struct patch"): firstly, the deletion of the
symlink, and secondly, the creation of the file. When applying that
patch with -R, the sections are reversed, so we get:

 (1) creation of a symlink, then
 (2) deletion of a file.

This causes an issue when the "deletion of a file" section is checked,
because Git observes that the so-called file is not a file but a
symlink, resulting in a "wrong type" error message.

What we want is:

 (1) deletion of a file, then
 (2) creation of a symlink.

In the code, this is reflected in the behavior of previous_patch() when
invoked from check_preimage() when the deletion is checked. Creation
then deletion means that when the deletion is checked, previous_patch()
returns the creation section, triggering a mode conflict resulting in
the "wrong type" error message. But deletion then creation means that
when the deletion is checked, previous_patch() returns NULL, so the
deletion mode is checked against lstat, which is what we want.

There are also other ways a patch can contain 2 sections referencing the
same file, for example, in 7a07841c0b ("git-apply: handle a patch that
touches the same path more than once better", 2008-06-27). "git apply
-R" fails in the same way, and this commit makes this case succeed.

Therefore, when building the list of sections, build them in reverse
order (by adding to the front of the list instead of the back) when -R
is passed.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
OK - updated the commit message and added a test for the use case Junio
described.
---
 apply.c                     | 9 +++++++--
 t/t4114-apply-typechange.sh | 7 +++++++
 t/t4127-apply-same-fn.sh    | 9 +++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index 76dba93c97..359ceb632c 100644
--- a/apply.c
+++ b/apply.c
@@ -4699,8 +4699,13 @@ static int apply_patch(struct apply_state *state,
 			reverse_patches(patch);
 		if (use_patch(state, patch)) {
 			patch_stats(state, patch);
-			*listp = patch;
-			listp = &patch->next;
+			if (!list || !state->apply_in_reverse) {
+				*listp = patch;
+				listp = &patch->next;
+			} else {
+				patch->next = list;
+				list = patch;
+			}
 
 			if ((patch->new_name &&
 			     ends_with_path_components(patch->new_name,
diff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh
index ebadbc347f..da3e64f811 100755
--- a/t/t4114-apply-typechange.sh
+++ b/t/t4114-apply-typechange.sh
@@ -88,6 +88,13 @@ test_expect_success 'symlink becomes file' '
 	'
 test_debug 'cat patch'
 
+test_expect_success 'symlink becomes file, in reverse' '
+	git checkout -f foo-symlinked-to-bar &&
+	git diff-tree -p HEAD foo-back-to-file > patch &&
+	git checkout foo-back-to-file &&
+	git apply -R --index < patch
+	'
+
 test_expect_success 'binary file becomes symlink' '
 	git checkout -f foo-becomes-binary &&
 	git diff-tree -p --binary HEAD foo-symlinked-to-bar > patch &&
diff --git a/t/t4127-apply-same-fn.sh b/t/t4127-apply-same-fn.sh
index 972946c174..305b7e649e 100755
--- a/t/t4127-apply-same-fn.sh
+++ b/t/t4127-apply-same-fn.sh
@@ -32,6 +32,10 @@ test_expect_success 'apply same filename with independent changes' '
 
 test_expect_success 'apply same filename with overlapping changes' '
 	git reset --hard &&
+
+	# Store same_fn so that we can check apply -R in next test
+	cp same_fn same_fn1 &&
+
 	modify "s/^d/z/" same_fn &&
 	git diff > patch0 &&
 	git add same_fn &&
@@ -43,6 +47,11 @@ test_expect_success 'apply same filename with overlapping changes' '
 	test_cmp same_fn same_fn2
 '
 
+test_expect_success 'apply same filename with overlapping changes, in reverse' '
+	git apply -R patch0 &&
+	test_cmp same_fn same_fn1
+'
+
 test_expect_success 'apply same new filename after rename' '
 	git reset --hard &&
 	git mv same_fn new_fn &&
-- 
2.29.0.rc1.297.gfa9743e501-goog