Re: [PATCH v2 19/20] diff --no-index: use parse_options() instead of diff_opt_parse()

[Duy Nguyen <pclouds@gmail.com>]

On Wed, May 1, 2019 at 5:12 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:>
> Hi Duy,
>
> On Tue, 30 Apr 2019, Duy Nguyen wrote:
>
> > On Tue, Apr 30, 2019 at 8:02 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > Hi Duy,
> > >
> > > On Sun, 24 Mar 2019, Nguyễn Thái Ngọc Duy wrote:
> > >
> > > > While at there, move exit() back to the caller. It's easier to see the
> > > > flow that way than burying it in diff-no-index.c
> > >
> > > I just noticed that this commit message is missing more than just a
> > > trailing period. It does not explain the change of behavior: previously,
> > > `GIT_EXTERNAL_DIFF=heya git diff --no-index a b` would silently ignore the
> > > external diff, it would have required an explicit `--ext-diff` to pick it
> > > up.
> >
> > Because I was not aware of the behavior change.
>
> Well, your patch removes an early return in favor of a later return that
> allows plenty of diff options to be configured in a different way than
> before.

No (and I was terse because I did not have time to look more into it).
The code flow is the same, the number of option parsing is the same.
Even post option processing is the same.

Bisecting points to 287ab28bfa (diff: reuse diff setup for --no-index
case, 2019-02-16). From the description (i.e. "miss out some settings
like --ext-diff...") the behavior change seems delibrate. Adding Jeff
for clarification/

> So while it is obvious

I probably have problem understanding. The "commit message is missing"
seems to imply I knew about this but chose not to mention it.

> (and understandable) that you were not aware of
> this behavior change, the real question is what we should do about this,
> now that this patch is already in `master` and on its way into v2.22.0.
>
> Ciao,
> Johannes



-- 
Duy