Re: [PATCH v2 2/4] update-index: use the same structure for chmod as add

[Thomas Gummerer <t.gummerer@gmail.com>]

On 09/11, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > @@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> >  			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
> >  			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> >  			(parse_opt_cb *) cacheinfo_callback},
> > -		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
> > -			N_("override the executable bit of the listed files"),
> > -			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> > -			chmod_callback},
> > +		OPT_STRING( 0, "chmod", &chmod_arg, N_("(+/-)x"),
> > +			N_("override the executable bit of the listed files")),
> >  		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
> >  			N_("mark files as \"not changing\""),
> >  			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
> > @@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> >  	if (argc == 2 && !strcmp(argv[1], "-h"))
> >  		usage_with_options(update_index_usage, options);
> >  
> > +	if (!chmod_arg)
> > +		force_mode = 0;
> > +	else if (!strcmp(chmod_arg, "-x"))
> > +		force_mode = 0666;
> > +	else if (!strcmp(chmod_arg, "+x"))
> > +		force_mode = 0777;
> > +	else
> > +		die(_("option 'chmod' expects \"+x\" or \"-x\""));
> > +
> 
> I am afraid that this changes the behaviour drastically.
> 
> "git update-index" is an oddball command that takes options and then
> processes them immediately, exactly because it was designed to take
> 
> 	git update-index --chmod=-x A --chmod=+x B --add C
> 
> and say things like "A and B are not in the index and you are
> attempting to add them before giving me --add option".
> 
> 	git update-index --add --chmod=-x A --chmod=+x B C
> 
> is expected to add A as non-executable, and B and C as executable.
> Many exotic parse-options callback mechanisms used in this command
> were invented exactly to support its quirky way of not doing "get a
> list of options and use the last one".  And this patch breaks it for
> only one option without changing the others.
> 
> If we were willing to take such a big backward compatiblity hit in
> the upcoming release (which I personally won't be affected, but old
> scripts by others need to be audited and adjusted, which I won't
> volunteer to do myself), we should make such a change consistently,
> e.g. "git update-index A --add --remove B" should no longer error
> out when it sees A and it is not yet in the index because "--add"
> hasn't been given yet, or A is in the index but is missing from the
> working tree because "--remove" hasn't been given yet.  Then it may
> be more justifiable if "update-index --chmod=-x A --chmod=+x B"
> added A as an executable.  With the current form of this patch, it
> is not.

Thanks for the explanation, this change in backwards compatibility is
certainly not what I intended, but rather something I missed while
cooking up this patch.

> Can we do this "fix" without this change?

Yeah, let me see what I can come up with in a re-roll.

Thanks,
Thomas