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