threads / rfc / 53082

RFC patchgit-apply: Permit change of file mode when filename does not change

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

## tl;dr

3 messages between Mar 24, 2020 and Mar 25, 2020. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Daniel Sommermann· Mar 24, 2020, 16:00 UTC · lore

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(-)
Show changes to 2 files +22 −18

apply.c, t/t4115-apply-symlink.sh

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
Jeff King· Mar 25, 2020, 06:11 UTC · re: Daniel Sommermann · lore

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

On Tue, Mar 24, 2020 at 09:00:54AM -0700, Daniel Sommermann wrote:
Show 6 quoted lines
> 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:

Show changes to t/t4115-apply-symlink.sh +16 −2
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
Junio C Hamano· Mar 25, 2020, 06:52 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> 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.

Show 9 quoted lines
> 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.

← back to recent threads