Re: [PATCH v3] revision: add separate field for "-m" of "diff-index -m"

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 31, 2020, 17:23 UTC
Message-ID: <xmqq4koihgpk.fsf@gitster.c.googlers.com>
In-Reply-To: <20200831125350.26472-1-sorganov@gmail.com>

Sergey Organov <sorganov@gmail.com> writes:

Show 10 quoted lines

> Add separate 'diff_index_match_missing' field for diff-index to use and set it
> when we encounter "-m" option. This field won't then be cleared when another
> meaning of "-m" is reverted (e.g., by "--no-diff-merges"), nor it will be
> affected by future option(s) that might drive 'ignore_merges' field.
>
> Use this new field from diff-lib:do_oneway_diff() instead of reusing
> 'ignore_merges' field.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---

Much easier to reason about. As I said, I think we would ideally want to detect and diagnose --[no-]diff-merges on the command line of "diff" or "diff-{files,index,trees}" as an error, but for now this is a good first step.

Show 8 quoted lines

>  	} else if (!strcmp(arg, "-m")) {
>  		revs->ignore_merges = 0;
> +		/*
> +		 * Backward compatibility wart - "diff-index -m" does
> +		 * not mean "do not ignore merges", but "match_missing",
> +		 * so set separate flag for it.
> +		 */
> +		revs->diff_index_match_missing = 1;

Half the wart has been removed thanks to this patch and the rest of the code can look at the right field for their purpose. The parsing, unless we make a bigger change that allows us to detect and diagnose "diff-index --no-diff-merges" as an error, still needs to be tricky and may deserve a comment.

The comment should apply to and treat both fields equally, perhaps like this:

	} else if (!strcmp(arg, "-m")) {
		/*
		 * To "diff-index", "-m" means "match missing", and to
		 * the "log" family of commands, it means "keep merges".
		 * Set both fields appropriately.
		 */
		revs->ignore_merges = 0;
		revs->match_missing = 1;
	}

By the way, let's drop diff_index_ prefix from the name of the new field. I do not see a strong reason to object to a possible update to "diff-files" to match the behaviour of "diff-index".

In a sparsely checked out working tree (e.g. start from "clone --no-checkout"), you can check out only paths that you want to modify, edit them, and then "git diff-files -m" would be able to show useful result without having to show deletions to all other files you are not interested in. And that is exactly the same use case as "git diff-index -m HEAD" was invented for.

Thanks.

Show 12 quoted lines

> diff --git a/revision.h b/revision.h
> index c1e5bcf139d7..5ae8254ffaed 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -188,6 +188,7 @@ struct rev_info {
>  	unsigned int	diff:1,
>  			full_diff:1,
>  			show_root_diff:1,
> +			diff_index_match_missing:1,
>  			no_commit_id:1,
>  			verbose_header:1,
>  			combine_merges:1,