[RFC PATCH] git-apply: Permit change of file mode when filename does not change

[RFC PATCH] git-apply: Permit change of file mode when filename does not change

[Daniel Sommermann]

The documentation for git's diff format does not expressly disallow
changing the mode of a file without splitting it into a delete and
create. Mercurial's `hg diff --git` in fact produces git diffs with such
format. When applying such patches in Git, this assert can be hit. The check
preventing this type of diff has been around since 2005 in
3cca928d4aae691572ef9a73dcc29a04f66900a1.

Simply deleting the check that prevents changing the mode when not
renaming allows such diffs to work out of the box, as the attached test
case shows.
---
 apply.c                  | 18 ------------------
 t/t4115-apply-symlink.sh | 23 +++++++++++++++++++++++
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/apply.c b/apply.c
index bdc008fae2..1b9d315771 100644
--- a/apply.c
+++ b/apply.c
@@ -3950,24 +3950,6 @@ static int check_patch(struct apply_state *state, struct patch *patch)
 		}
 	}
 
-	if (new_name && old_name) {
-		int same = !strcmp(old_name, new_name);
-		if (!patch->new_mode)
-			patch->new_mode = patch->old_mode;
-		if ((patch->old_mode ^ patch->new_mode) & S_IFMT) {
-			if (same)
-				return error(_("new mode (%o) of %s does not "
-					       "match old mode (%o)"),
-					patch->new_mode, new_name,
-					patch->old_mode);
-			else
-				return error(_("new mode (%o) of %s does not "
-					       "match old mode (%o) of %s"),
-					patch->new_mode, new_name,
-					patch->old_mode, old_name);
-		}
-	}
-
 	if (!state->unsafe_paths && check_unsafe_path(patch))
 		return -128;
 
diff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh
index 872fcda6cb..593e6142b4 100755
--- a/t/t4115-apply-symlink.sh
+++ b/t/t4115-apply-symlink.sh
@@ -44,4 +44,27 @@ test_expect_success 'apply --index symlink patch' '
 
 '
 
+cat >move_patch <<\EOF
+diff --git a/file_to_be_link b/file_to_be_link
+old mode 100644
+new mode 120000
+--- a/file_to_be_link
++++ b/file_to_be_link
+@@ -0,0 +1,1 @@
++target
+\ No newline at end of file
+EOF
+
+test_expect_success 'apply file-to-symlink patch' '
+
+	git checkout -f master &&
+	touch file_to_be_link &&
+	git add file_to_be_link &&
+	git commit -m initial &&
+
+	git apply move_patch &&
+	test target = $(readlink file_to_be_link)
+
+'
+
 test_done
-- 
2.25.1

Re: [RFC PATCH] git-apply: Permit change of file mode when filename does not change

[Jeff King]

On Tue, Mar 24, 2020 at 09:00:54AM -0700, Daniel Sommermann wrote:

> The documentation for git's diff format does not expressly disallow
> changing the mode of a file without splitting it into a delete and
> create. Mercurial's `hg diff --git` in fact produces git diffs with such
> format. When applying such patches in Git, this assert can be hit. The check
> preventing this type of diff has been around since 2005 in
> 3cca928d4aae691572ef9a73dcc29a04f66900a1.

This description confused me for a moment, because in Git we generally
refer to "mode changes" as flipping the executable bit. And anything
further is a "type change" (and this isn't just academic; options like
--diff-filter distinguish the two).

And we do indeed allow a simple mode change like:

  $ git show c9d4999155700651a37f4eb577cec1f4b5b8d6be --format=
  diff --git a/t/perf/p0004-lazy-init-name-hash.sh b/t/perf/p0004-lazy-init-name-hash.sh
  old mode 100644
  new mode 100755

But you're talking about typechanges here, and we do always represent
those as a deletion/addition pair:

  $ git show --format= -D 2efbb7f5218d5ca9d50cbcb86a365a08b2981d77 RelNotes
  diff --git a/RelNotes b/RelNotes
  deleted file mode 100644
  index 007bc065dd..0000000000
  diff --git a/RelNotes b/RelNotes
  new file mode 120000
  index 0000000000..8d0b1654d2
  --- /dev/null
  +++ b/RelNotes
  @@ -0,0 +1 @@
  +Documentation/RelNotes/2.20.0.txt
  \ No newline at end of file

I don't think we'd want to switch how we generate these diffs, but I
can't offhand think of a reason why it would be a bad idea to accept
such a patch converting a file to a symlink or vice versa.

But...

> Simply deleting the check that prevents changing the mode when not
> renaming allows such diffs to work out of the box, as the attached test
> case shows.

What about other more exotic typechanges, like a directory becoming a
file?  Or a file to a gitlink? I guess we'd never mention a directory in
--patch format anyway, but I wonder to what degree these lines in
check_patch() are protecting downstream code from doing something
stupid.

If I fake a diff like:

  diff --git a/file b/file
  old mode 100644
  new mode 040000

we seem to silently accept it but not write any mode change (we do write
a content change to the file). Swapping 040000 (a tree) out for 160000
(a gitlink) seems to delete file but not apply any content-level change.

Also, I'm not sure your patch works for the reverse case: a symlink
becoming a file. If I add this to your test:

diff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh
index 593e6142b4..acd94a07a7 100755
--- a/t/t4115-apply-symlink.sh
+++ b/t/t4115-apply-symlink.sh
@@ -67,4 +67,20 @@ test_expect_success 'apply file-to-symlink patch' '
 
 '
 
+test_expect_success 'apply symlink-to-file patch' '
+
+	cat >reverse-patch <<-\EOF &&
+	diff --git a/file_to_be_link b/file_to_be_link
+	new mode 120000
+	old mode 100644
+	--- a/file_to_be_link
+	+++ b/file_to_be_link
+	@@ -1,1 +1,1 @@
+	-target
+	+file
+	EOF
+
+	git apply reverse-patch
+'
+
 test_done

it fails with "error: file_to_be_link: wrong type".

> diff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh
> index 872fcda6cb..593e6142b4 100755
> --- a/t/t4115-apply-symlink.sh
> +++ b/t/t4115-apply-symlink.sh

If we do go this route, two small fixes for the tests:

> @@ -44,4 +44,27 @@ test_expect_success 'apply --index symlink patch' '
>  
>  '
>  
> +cat >move_patch <<\EOF
> +diff --git a/file_to_be_link b/file_to_be_link
> +old mode 100644
> +new mode 120000
> +--- a/file_to_be_link
> ++++ b/file_to_be_link
> +@@ -0,0 +1,1 @@
> ++target
> +\ No newline at end of file
> +EOF

We prefer this kind of setup to go inside the test_expect_success block
(you can use "<<-\EOF" to strip leading tabs and get nice indentation).

Some older tests haven't been updated yet, so you may have picked this
up from a bad example, but we try to follow it when writing new ones.

> +test_expect_success 'apply file-to-symlink patch' '
> +
> +	git checkout -f master &&
> +	touch file_to_be_link &&
> +	git add file_to_be_link &&
> +	git commit -m initial &&
> +
> +	git apply move_patch &&
> +	test target = $(readlink file_to_be_link)
> +
> +'

This probably needs a SYMLINKS prerequisite, since we'd write the actual
symlink to the filesystem. We could work around that with "apply
--index", but I think it's important to test the full patch application.

-Peff

Re: [RFC PATCH] git-apply: Permit change of file mode when filename does not change

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 24, 2020 at 09:00:54AM -0700, Daniel Sommermann wrote:
>
>> The documentation for git's diff format does not expressly disallow
>> changing the mode of a file without splitting it into a delete and
>> create. Mercurial's `hg diff --git` in fact produces git diffs with such
>> format. When applying such patches in Git, this assert can be hit. The check
>> preventing this type of diff has been around since 2005 in
>> 3cca928d4aae691572ef9a73dcc29a04f66900a1.
>
> This description confused me for a moment, because in Git we generally
> refer to "mode changes" as flipping the executable bit. And anything
> further is a "type change" (and this isn't just academic; options like
> --diff-filter distinguish the two).

I too found that file "mode" made me confused and thought we reject
a patch that flips executable bits of files with or without touching
their contents.  You are talking about a patch that changes file
"type" between regular files and symbolic links, or worse yet,
regular files or symbolic links and submodules.

It sounds more like a documentation bug, or a bug in the reader of
the documentation.  It's not like what is not explicitly disallowed
are all allowed.

> And we do indeed allow a simple mode change like:
>
>   $ git show c9d4999155700651a37f4eb577cec1f4b5b8d6be --format=
>   diff --git a/t/perf/p0004-lazy-init-name-hash.sh b/t/perf/p0004-lazy-init-name-hash.sh
>   old mode 100644
>   new mode 100755
>
> But you're talking about typechanges here, and we do always represent
> those as a deletion/addition pair:

While I, like you said below, do not offhand think of a reason why
it may hurt if we allowed to express a replacement of a symbolic
link with a regular file that holds the result of readlink(2) as
pure mode bits change without (or even with) content change at the
mechanical level, I strongly suspect that we chose not to emit such
a patch on the "diff" side because it would be easily missed by
human readers, and that is why such a change is always shown as a
deletion followed by a creation.

And this safety was there in the oldest "still not yet fully but
started working barely" version, so we can safely say that we never
allowed such a patch.

So, I am not sure if we want to lose it.  As you found out, the
removed segment is not only about regular file becoming symlink,
and I do not think we would want to allow other typechanges by
just simply removing the check like the proposed patch did.

Thanks.