Re: [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2020, 18:35 UTC
Message-ID: <xmqqo8mdpji9.fsf@gitster.c.googlers.com>
In-Reply-To: <ea6717e7b3a8b89fc3750505678321803cde78dc.1599723087.git.liu.denton@gmail.com>

Denton Liu <liu.denton@gmail.com> writes:

Show 28 quoted lines

> Instead of parsing the `--no-index` option with a plain strcmp, use
> parse_options() to parse options. This allows us to easily add more
> options in a future commit.
>
> As a result of this change, `--no-index` is removed from `argv` so, as a
> result, we no longer need to handle it in diff_no_index().
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  builtin/diff.c  | 33 ++++++++++++++++++++++-----------
>  diff-no-index.c | 15 +++------------
>  2 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index cb98811c21..0e086ed7c4 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -373,6 +373,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  	int nongit = 0, no_index = 0;
>  	int result = 0;
>  	struct symdiff sdiff;
> +	struct option options[] = {
> +		OPT_SET_INT_F(0, "no-index", &no_index,
> +			   N_("compare the given paths on the filesystem"),
> +			   DIFF_NO_INDEX_EXPLICIT,
> +			   PARSE_OPT_NONEG),
> +		OPT_END(),
> +	};

What's the reasoning behind teaching the "--merge-base" option only to "diff" and not allowing "diff-index" and "diff-tree" to also benefit from the new support? It should be taught to be handled by diff.c::diff_opt_parse() instead, like all other "diff" options. I simply do not see what makes "--merge-base" so special compared to other options like "-R", "--color-words", etc.

And with that, this step will become totally unnecessary, because the options[] array will still have only the "no-index" option and nothing else.

UNLESS we are planning to use this array to parse all diff options here, whether it is for the no-index mode or the normal invocation of diff, but that is unlikely to happen---we'd still share many options between the plumbing "diff-*" family and Porcelain "diff" commands.

Thanks.