RFC patchbuiltin-apply: prevent non-explicit permission changes

 
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(-)

Alexander Potashev <aspotashev@gmail.com> writes:

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

On 05:00 Thu 01 Jan     , Junio C Hamano wrote:

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:

...not completely drops, probably we should cross out this line completely (I don't know whether it breaks something).

					Alexander

Alexander Potashev <aspotashev@gmail.com> writes:

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(-)

On 16:56 Thu 01 Jan     , Junio C Hamano wrote:

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.

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:

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(-)

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

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).

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").

The implementation looks good to me.

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