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

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2021, 04:09 UTC
Message-ID: <xmqqwns5oqrm.fsf@gitster.g>
In-Reply-To: <20210510153451.15090-3-sorganov@gmail.com>

Sergey Organov <sorganov@gmail.com> writes:

Show 18 quoted lines

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

Thanks.