From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Mon, 13 Aug 2018 18:01:50 GMT
Subject: Re: [PATCH v5 05/21] range-diff: also show the diff between patches
Message-ID: <20180813180150.GC13316@hank.intra.tgummerer.com>
In-Reply-To: <nycvar.QRO.7.76.6.1808131135290.71@tvgsbejvaqbjf.bet>

On 08/13, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Sun, 12 Aug 2018, Thomas Gummerer wrote:
> 
> > On 08/10, Johannes Schindelin via GitGitGadget wrote:
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > [...]
> > 
> > I don't think this handles "--" quite as would be expected.  Trying to
> > use "git range-diff -- js/range-diff-v4...HEAD" I get:
> > 
> >     $ ./git range-diff -- js/range-diff-v4...HEAD
> >     error: need two commit ranges
> >     usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>
> >        or: git range-diff [<options>] <old-tip>...<new-tip>
> >        or: git range-diff [<options>] <base> <old-tip> <new-tip>
> > 
> >         --creation-factor <n>
> >                               Percentage by which creation is weighted
> >         --no-dual-color       color both diff and diff-between-diffs
> > 
> > while what I would have expected is to actually get a range diff.
> > This happens because after we break out of the loop we don't add the
> > actual ranges to argv, but just skip them instead.
> 
> Ouch, good point.
> 
> > I think something like the following should be squashed in to this
> > patch.
> > 
> > --->8---
> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> > index ef3ba22e29..132574c57a 100644
> > --- a/builtin/range-diff.c
> > +++ b/builtin/range-diff.c
> > @@ -53,6 +53,11 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
> >                 else
> >                         i += c;
> >         }
> > +       if (i < argc && !strcmp("--", argv[i])) {
> > +               i++; j++;
> > +               while (i < argc)
> > +                       argv[j++] = argv[i++];
> > +       }
> >         argc = j;
> >         diff_setup_done(&diffopt);
> 
> I do not think that is correct. The original idea was for the first
> `parse_options()` call to keep the dashdash, for the second one to keep
> the dashdash, too, and for the final one to swallow it.
> 
> Also, if `i < argc` at this point, we already know that `argv[i]` refers
> to the dashdash, otherwise the previous loop would not have exited early.
> 
> I went with this simple version instead:
> 
> 	while (i < argc)
> 		argv[j++] = argv[i++];

Right, that's much better, thanks!

> Thanks!
> Dscho