Subject: Re: [PATCH 2/6] diff-merges: move specific diff-index "-m" handling to diff-index

From: Sergey Organov <[email-masked]>
Date: Tue, 11 May 2021 16:43:48 +0300

Junio C Hamano <[email-masked]> writes:

> Sergey Organov <[email-masked]> writes:
>
>>  int cmd_diff_index(int argc, const char **argv, const char *prefix)
>>  {
>>  	struct rev_info rev;
>>  	unsigned int option = 0;
>> -	int i;
>>  	int result;
>>  
>>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>> @@ -27,17 +53,16 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
>>  	rev.abbrev = 0;
>>  	prefix = precompose_argv_prefix(argc, argv, prefix);
>>  
>> +	/*
>> +	 * It's essential to parse our distinct options before calling
>> +	 * setup_revisions(), for the latter not to see "-m".
>> +	 */
>> +	argc = parse_distinct_options(argc, argv, &rev, &option);
>>  	argc = setup_revisions(argc, argv, &rev, NULL);
>
> This change is risky, as the loop below (which this patch moves to
> parse_distinct_options()) has no knowledge of other options that
> setup_revisions() helper is prepared to handle and that takes an
> argument.  When parsing "git cmd --opt --cached A", setup_revisions()
> may know that --opt takes an argument and eat both (i.e. the
> "--cached" is not an option but an arg given to "--opt"), but the
> new parse_distinct_options() helper does not; it will happily skip
> "--opt" and leave it in, mistake "--cached" as an option and remove,
> and instead make "A" the arg given to "--opt".
>
> Picking up the remnant _after_ setup_revisions() ate what it
> understands would not have such a downside, as long as none of our
> "distinct options" take any argument.
>
> Can't we make "-m means something special for diff-index" without
> butchering the command line processing in this step?  diff-index
> does not care about --diff-merges, so letting setup_revisions()
> remember only the fact that "-m" was given while parsing, and then
> postprocess what "-m" means depending on the command (i.e. everybody
> else would treat it as a short-hand for "--diff-merges=m" plus "we
> need some form of diff output, while allowing "diff-index" to treat
> it differently) should not be rocket science.

I have already considered a few ways of doing it, and what I came up
with looked least destructive to me at the moment, especially as it
broke no tests whatsoever.

I'll now re-consider my approach because of your observations, thanks!

-- Sergey Organov