Re: [PATCH 2/3] range-diff: handle commit ranges other than A..B

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 22, 2021, 16:12 UTC
Message-ID: <nycvar.QRO.7.76.6.2101221711140.52@tvgsbejvaqbjf.bet>
In-Reply-To: <CAPig+cTMqy=57hGksH6GaCB7i=eJHTCWVzwED=PPZF1hNp_1nQ@mail.gmail.com>

Hi Eric,

On Thu, 21 Jan 2021, Eric Sunshine wrote:

Show 35 quoted lines

> On Thu, Jan 21, 2021 at 5:22 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > In the `SPECIFYING RANGES` section of gitrevisions[7], two ways are
> > described to specify commit ranges that `range-diff` does not yet
> > accept: "<commit>^!" and "<commit>^-<n>".
> >
> > Let's accept them.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> > @@ -13,7 +13,26 @@ NULL
> >  static int is_range(const char *range)
> >  {
> > +       if (strstr(range, ".."))
> > +               return 1;
> > +
> > +       i = strlen(range);
> > +       c = i ? range[--i] : 0;
> > +       if (c == '!')
> > +               i--; /* might be ...^! or ...^@ */
> > +       else if (isdigit(c)) {
> > +               /* handle ...^-<n> */
> > +               while (i > 2 && isdigit(range[--i]))
> > +                       ; /* keep trimming trailing digits */
> > +               if (i < 2 || range[i--] != '-')
> > +                       return 0;
> > +       } else
> > +               return 0;
> > +
> > +       return i > 0 && range[i] == '^';
> >  }
>
> Is this something that the --range-diff option of git-format-patch
> will want to do, as well?

Thank you for pointing that out. I should have checked via `git grep 'strstr.*"\.\."'` myself. There are two more instances, one in `rev-parse.c` and the other in `revision.c`, but both are necessary as-are because their return value is actually used to further disect a `..`-style commit range.

Thanks, Dscho

Show 5 quoted lines

> At present, builtin/log.c:infer_range_diff_ranges() detects a range only
> by checking for "..", much like this function did before this patch. If
> so, perhaps this function can be part of the public range-diff API (or,
> indeed, part of some other more general API if it's not really specific
> to range-diff).