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

From: Denton Liu <liu.denton@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()
Date: Sun, 13 Sep 2020 01:31:22 -0700	[thread overview]
Message-ID: <20200913083122.GA189023@generichostname> (raw)
In-Reply-To: <xmqqo8mdpji9.fsf@gitster.c.googlers.com>

Hi Junio,

On Thu, Sep 10, 2020 at 11:35:42AM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > 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?

This is a good idea. Initially, I was hesitant because the manpages for
diff-index and diff-tree both list the argument as <treeish> but I think
it makes sense that these commands should accept --merge-base too.

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

Unfortunately, despite the above, I don't think we'll be able to handle
this in diff_opt_parse(). --merge-base won't be common to all diff
modes. It'll only work with diff-index and diff-tree, so it'll have to
be handled in those modes specifically.

Thanks,
Denton