[RFC PATCH] builtin-apply: prevent non-explicit permission changes

[RFC PATCH] builtin-apply: prevent non-explicit permission changes

[Alexander Potashev]

 
Prevent 'git apply' from changing permissions without
'old mode'/'new mode' lines in patch.
(WARNING: this changes the behaviour of 'git apply')

Signed-off-by: Alexander Potashev <aspotashev@gmail.com>
---

Once upon a time there was a shell script in a Git repository. But that
shell script had 100644 permission (regular file). Then I did
'chmod +x', commit... but the shell script was related to my friend's
stuff in the repository and I received a patch from him regarding the
script. But the patch was against a repository version before
'chmod +x', thus it contained an index line such as the following:

	index fc3c3a4..066a4ac 100644
(it still had '100644' permissions)

I have to note that there was no 'old/new mode' lines. But when I ran
'git am <patch>' it restored '100644' permissions. So, 'git am' changed
my permissions (100755 -> 100644) without any explicit permission
changes in the patch.

I think, 'git apply'/'git am' should apply only _changes_ _mentioned_ in
patch; if there's no 'old mode ...'/'new mode ...' lines in it, 'git
apply' shouldn't change the permissions.


Test cases are probably wanted, but I don't really know how to do them
and I'll only give a chain of commands to reproduce the issue:

	mkdir repo
	cd repo

	git init
	echo "This is a shell script" > script.sh
	git add script.sh
	git ci -m "initial commit"

	echo "a new line and a newline" >> script.sh
	git ci -a -m "only content changes"	# aka patch to apply
	git format-patch -1			# now we have a patch

	git reset --hard HEAD^
	chmod +x script.sh
	git ci -a -m "permission changes"

	git am 0001-only-content-changes.patch
	stat -c %a script.sh			# check the result

'stat' says '644' if 'git am' has changed the permissions or '755' if
it hasn't.

					Alexander


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

diff --git a/builtin-apply.c b/builtin-apply.c
index 07244b0..071f6d8 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)
 	memcpy(patch->new_sha1_prefix, line, len);
 	patch->new_sha1_prefix[len] = 0;
 	if (*ptr == ' ')
-		patch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);
+		patch->old_mode = strtoul(ptr+1, NULL, 8);
 	return 0;
 }
 
@@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 	if (st_mode != patch->old_mode)
 		fprintf(stderr, "warning: %s has type %o, expected %o\n",
 			old_name, st_mode, patch->old_mode);
+	patch->new_mode = st_mode;
 	return 0;
 
  is_new:
-- 
1.6.0.6

Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes

[Junio C Hamano]

Alexander Potashev <aspotashev@gmail.com> writes:

>  builtin-apply.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 07244b0..071f6d8 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)
>  	memcpy(patch->new_sha1_prefix, line, len);
>  	patch->new_sha1_prefix[len] = 0;
>  	if (*ptr == ' ')
> -		patch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);
> +		patch->old_mode = strtoul(ptr+1, NULL, 8);
>  	return 0;
>  }
>  
> @@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
>  	if (st_mode != patch->old_mode)
>  		fprintf(stderr, "warning: %s has type %o, expected %o\n",
>  			old_name, st_mode, patch->old_mode);
> +	patch->new_mode = st_mode;

Can you do this unconditionally, overwriting whatever we read from the
patch header metainfo lines?

Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes

[Alexander Potashev]

On 05:00 Thu 01 Jan     , Junio C Hamano wrote:
> Alexander Potashev <aspotashev@gmail.com> writes:
> 
> >  builtin-apply.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/builtin-apply.c b/builtin-apply.c
> > index 07244b0..071f6d8 100644
> > --- a/builtin-apply.c
> > +++ b/builtin-apply.c
> > @@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)
> >  	memcpy(patch->new_sha1_prefix, line, len);
> >  	patch->new_sha1_prefix[len] = 0;
> >  	if (*ptr == ' ')
> > -		patch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);
> > +		patch->old_mode = strtoul(ptr+1, NULL, 8);
> >  	return 0;
> >  }
> >  
> > @@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
> >  	if (st_mode != patch->old_mode)
> >  		fprintf(stderr, "warning: %s has type %o, expected %o\n",
> >  			old_name, st_mode, patch->old_mode);
> > +	patch->new_mode = st_mode;
> 
> Can you do this unconditionally, overwriting whatever we read from the
> patch header metainfo lines?

Do you mean overwriting of 'patch->new_mode' right after patch parsing?
If so, there would be yet another call to 'stat' to get the permissions
of the existing file (that is not very good).

I'm not very familiar with Git sources.

Also, I don't understand what are the permissions in 'index ...' lines
for (e.g. "index fc3c3a4..066a4ac 100644"), my patch simply drops them:
> > -		patch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);
> > +		patch->old_mode = strtoul(ptr+1, NULL, 8);
...not completely drops, probably we should cross out this line
completely (I don't know whether it breaks something).

					Alexander

Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes

[Junio C Hamano]

Alexander Potashev <aspotashev@gmail.com> writes:

> On 05:00 Thu 01 Jan     , Junio C Hamano wrote:
>> Alexander Potashev <aspotashev@gmail.com> writes:
> ...
>> > @@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
>> >  	if (st_mode != patch->old_mode)
>> >  		fprintf(stderr, "warning: %s has type %o, expected %o\n",
>> >  			old_name, st_mode, patch->old_mode);
>> > +	patch->new_mode = st_mode;
>> 
>> Can you do this unconditionally, overwriting whatever we read from the
>> patch header metainfo lines?
>
> Do you mean overwriting of 'patch->new_mode' right after patch parsing?

My question was if we should assign st_mode to new_mode _unconditionally_
here, even when patch->new_mode has already been read from the explicit
mode change line (i.e. "new mode ", line not "index "line) of the patch
input.

The call-chain of the program looks like this:

-> apply_patch()
   -> parse_chunk()
      -> find_header()
         * initialize new_mode and old_mode to 0
         -> parse_git_header()
            * set new_mode and old_mode from the patch metainfo, i.e.
              "new mode", "old mode" and "index" lines.
      -> parse_single_patch()
   -> check_patch_list()
      -> check_patch()
         -> check_preimage()
            * make sure there is no local mods
            * warn if old_mode read from the patch (i.e. the preimage file
              the patch submitter used to prepare the patch against) does not
              match what we have
         * warn about mode inconsistency (e.g. the patch submitter thinks
           the mode should be 0644 but our tree has 0755).
         -> apply_data()
   -> write_out_results()
      -> write_out_one_result(0)
         * delete old
      -> write_out_one_result(1)
         * create new

Currently the mode 100644 on the "index" line in a patch is handled
exactly in the same way as having "old mode 100644" and "new mode 100644"
lines in the metainfo.  The patch submitter claims to have started from
100644 and he claims that he wants to have 100644 as the result.  That is
why there is a warning in check_patch().

If we stop reading the new mode from the "index" line (but we still read
"old_mode" there) without any other change you made in your patch, what
breaks (i.e. without the patch->new_mode assignment hunk)?  I haven't
followed the codepath too closely, and I suspect you found some cases
where new_mode stays 0 as initialized, and that may be the reason you have
this assignment.

But the assignment being unconditional bothered me a lot.

I tend to agree that the current "The final mode bits I want to have on
this path is this" semantics we give to the "index" line is much less
useful and less sane and it is a good idea to redefine it as "FYI, the
copy I made this patch against had this mode bits.  I do not intend to
change the mode bits of the path with this patch."

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

diff --git c/builtin-apply.c w/builtin-apply.c
index 07244b0..a8f75ed 100644
--- c/builtin-apply.c
+++ w/builtin-apply.c
@@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)
 	memcpy(patch->new_sha1_prefix, line, len);
 	patch->new_sha1_prefix[len] = 0;
 	if (*ptr == ' ')
-		patch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);
+		patch->old_mode = strtoul(ptr+1, NULL, 8);
 	return 0;
 }
 
@@ -2447,6 +2447,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 	if (st_mode != patch->old_mode)
 		fprintf(stderr, "warning: %s has type %o, expected %o\n",
 			old_name, st_mode, patch->old_mode);
+	if (!patch->new_mode)
+		patch->new_mode = st_mode;
 	return 0;
 
  is_new:

Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes

[Alexander Potashev]

On 16:56 Thu 01 Jan     , Junio C Hamano wrote:
> Alexander Potashev <aspotashev@gmail.com> writes:
> 
> > On 05:00 Thu 01 Jan     , Junio C Hamano wrote:
> >> Alexander Potashev <aspotashev@gmail.com> writes:
> > ...
> >> > @@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
> >> >  	if (st_mode != patch->old_mode)
> >> >  		fprintf(stderr, "warning: %s has type %o, expected %o\n",
> >> >  			old_name, st_mode, patch->old_mode);
> >> > +	patch->new_mode = st_mode;
> >> 
> >> Can you do this unconditionally, overwriting whatever we read from the
> >> patch header metainfo lines?
> >
> > Do you mean overwriting of 'patch->new_mode' right after patch parsing?
> 
> My question was if we should assign st_mode to new_mode _unconditionally_
> here, even when patch->new_mode has already been read from the explicit
> mode change line (i.e. "new mode ", line not "index "line) of the patch
> input.
> 
> The call-chain of the program looks like this:
> 
> -> apply_patch()
>    -> parse_chunk()
>       -> find_header()
>          * initialize new_mode and old_mode to 0
>          -> parse_git_header()
>             * set new_mode and old_mode from the patch metainfo, i.e.
>               "new mode", "old mode" and "index" lines.
>       -> parse_single_patch()
>    -> check_patch_list()
>       -> check_patch()
>          -> check_preimage()
>             * make sure there is no local mods
>             * warn if old_mode read from the patch (i.e. the preimage file
>               the patch submitter used to prepare the patch against) does not
>               match what we have
>          * warn about mode inconsistency (e.g. the patch submitter thinks
>            the mode should be 0644 but our tree has 0755).
>          -> apply_data()
>    -> write_out_results()
>       -> write_out_one_result(0)
>          * delete old
>       -> write_out_one_result(1)
>          * create new
> 
> Currently the mode 100644 on the "index" line in a patch is handled
> exactly in the same way as having "old mode 100644" and "new mode 100644"
> lines in the metainfo.  The patch submitter claims to have started from
> 100644 and he claims that he wants to have 100644 as the result.  That is
> why there is a warning in check_patch().
> 
> If we stop reading the new mode from the "index" line (but we still read
> "old_mode" there) without any other change you made in your patch, what
> breaks (i.e. without the patch->new_mode assignment hunk)?  I haven't
> followed the codepath too closely, and I suspect you found some cases
> where new_mode stays 0 as initialized, and that may be the reason you have
> this assignment.
> 
> But the assignment being unconditional bothered me a lot.
> 
> I tend to agree that the current "The final mode bits I want to have on
> this path is this" semantics we give to the "index" line is much less
> useful and less sane and it is a good idea to redefine it as "FYI, the
> copy I made this patch against had this mode bits.  I do not intend to
> change the mode bits of the path with this patch."
> 
>  builtin-apply.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git c/builtin-apply.c w/builtin-apply.c
> index 07244b0..a8f75ed 100644
> --- c/builtin-apply.c
> +++ w/builtin-apply.c
> @@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)
>  	memcpy(patch->new_sha1_prefix, line, len);
>  	patch->new_sha1_prefix[len] = 0;
>  	if (*ptr == ' ')
> -		patch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);
> +		patch->old_mode = strtoul(ptr+1, NULL, 8);
>  	return 0;
>  }
>  
> @@ -2447,6 +2447,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
>  	if (st_mode != patch->old_mode)
>  		fprintf(stderr, "warning: %s has type %o, expected %o\n",
>  			old_name, st_mode, patch->old_mode);
> +	if (!patch->new_mode)
> +		patch->new_mode = st_mode;

This is a _major_ fix, with my patch it would never change any
permissions at all.

I couldn't fully understand that problem last night, sorry for the
noise.

>  	return 0;
>  
>   is_new:

Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes

[Junio C Hamano]

A git patch that does not change the executable bit still records the mode
on its "index" line.  "git apply" used to interpret this mode exactly the
same way as it interprets the mode recorded on "new mode" line.  As the
wish by the patch submitter to set the mode to the one recorded on the
line.

The reason the mode does not agree between the submitter and the receiver
in the first place is because there is _another_ commit that only appears
on one side but not the other since their histories diverged, and that
commit changes the mode.  The patch has "index" line but not "new mode"
line because its change is about updating the contents without affecting
the mode.  The application of such a patch is an explicit wish by the
submitter to only cherry-pick the commit that updates the contents without
cherry-picking the commit that modifies the mode.  Viewed this way, the
current behaviour is problematic, even though the command does warn when
the mode of the path being patched does not match this mode, and a careful
user could detect this inconsistencies between the patch submitter and the
patch receiver.

This changes the semantics of the mode recorded on the "index" line;
instead of interpreting it as the submitter's wish to set the mode to the
recorded value, it merely informs what the mode submitter happened to
have, and the presense of the "index" line is taken as submitter's wish to
keep whatever the mode is on the receiving end.

This is based on the patch originally done by Alexander Potashev with a
minor fix; the tests are mine.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

Alexander Potashev <aspotashev@gmail.com> writes:

> Prevent 'git apply' from changing permissions without
> 'old mode'/'new mode' lines in patch.
> (WARNING: this changes the behaviour of 'git apply')
> ...
> Test cases are probably wanted, but I don't really know how to do them
> and I'll only give a chain of commands to reproduce the issue:

So here is what I sent earlier but with test cases.  I suspect your
version does not pass the latter half of the test suite, because it stomps
on the explicitly recorded mode changes in the patch.

 builtin-apply.c           |    4 ++-
 t/t4129-apply-samemode.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletions(-)

diff --git c/builtin-apply.c w/builtin-apply.c
index 07244b0..a8f75ed 100644
--- c/builtin-apply.c
+++ w/builtin-apply.c
@@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)
 	memcpy(patch->new_sha1_prefix, line, len);
 	patch->new_sha1_prefix[len] = 0;
 	if (*ptr == ' ')
-		patch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);
+		patch->old_mode = strtoul(ptr+1, NULL, 8);
 	return 0;
 }
 
@@ -2447,6 +2447,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 	if (st_mode != patch->old_mode)
 		fprintf(stderr, "warning: %s has type %o, expected %o\n",
 			old_name, st_mode, patch->old_mode);
+	if (!patch->new_mode)
+		patch->new_mode = st_mode;
 	return 0;
 
  is_new:
diff --git c/t/t4129-apply-samemode.sh w/t/t4129-apply-samemode.sh
new file mode 100755
index 0000000..adfcbb5
--- /dev/null
+++ w/t/t4129-apply-samemode.sh
@@ -0,0 +1,62 @@
+#!/bin/sh
+
+test_description='applying patch with mode bits'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo original >file &&
+	git add file &&
+	test_tick &&
+	git commit -m initial &&
+	git tag initial &&
+	echo modified >file &&
+	git diff --stat -p >patch-0.txt &&
+	chmod +x file &&
+	git diff --stat -p >patch-1.txt
+'
+
+test_expect_success 'same mode (no index)' '
+	git reset --hard &&
+	chmod +x file &&
+	git apply patch-0.txt &&
+	test -x file
+'
+
+test_expect_success 'same mode (with index)' '
+	git reset --hard &&
+	chmod +x file &&
+	git add file &&
+	git apply --index patch-0.txt &&
+	test -x file &&
+	git diff --exit-code
+'
+
+test_expect_success 'same mode (index only)' '
+	git reset --hard &&
+	chmod +x file &&
+	git add file &&
+	git apply --cached patch-0.txt &&
+	git ls-files -s file | grep "^100755"
+'
+
+test_expect_success 'mode update (no index)' '
+	git reset --hard &&
+	git apply patch-1.txt &&
+	test -x file
+'
+
+test_expect_success 'mode update (with index)' '
+	git reset --hard &&
+	git apply --index patch-1.txt &&
+	test -x file &&
+	git diff --exit-code
+'
+
+test_expect_success 'mode update (index only)' '
+	git reset --hard &&
+	git apply --cached patch-1.txt &&
+	git ls-files -s file | grep "^100755"
+'
+
+test_done

Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes

[Jeff King]

On Fri, Jan 02, 2009 at 02:55:37AM -0800, Junio C Hamano wrote:

> A git patch that does not change the executable bit still records the mode
> on its "index" line.  "git apply" used to interpret this mode exactly the
> same way as it interprets the mode recorded on "new mode" line.  As the
> wish by the patch submitter to set the mode to the one recorded on the
> line.

Nit: I had to read that third sentence several times to make sense of
it, since it is not a complete sentence (I think s/line\. As/line: as/
might help).

> This changes the semantics of the mode recorded on the "index" line;
> instead of interpreting it as the submitter's wish to set the mode to the
> recorded value, it merely informs what the mode submitter happened to
> have, and the presense of the "index" line is taken as submitter's wish to
> keep whatever the mode is on the receiving end.

I have been following this thread but didn't have a chance to look
closely until now. I think this change is definitely the right thing, as
it follows the normal semantics for a patch (which are basically a
merge: "change the parts we changed, but leave everything else, even if
the other side changed it").

>  builtin-apply.c           |    4 ++-

The implementation looks good to me.

>  t/t4129-apply-samemode.sh |   62 ++++++++++++++++++++++++++++++++++++++++

And the tests make me feel warm and fuzzy. It is always nice to see
tests that aren't just "X was broken, and now it works" or "new feature
Y works" but "here is every case spelled out with its desired behavior."
I think those are the tests that really help keep us regression-proof.

-Peff