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

[Junio C Hamano <gitster@pobox.com>]

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: