Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 17, 2018, 10:49 UTC
Message-ID: <CAPig+cTKGd8N78XvW-rmBEZC7ykcJsE+na1V_vCVXTUhGrFe4Q@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1807171219480.71@tvgsbejvaqbjf.bet>

Thanks for the review comments. In the new version of the series (almost complete), almost the entire implementation has changed, including most of the stuff on which you commented. Anyhow, see my responses to your comments below...

On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 5 quoted lines

> On Wed, 30 May 2018, Eric Sunshine wrote:
> > +static int get_range_diff(struct strbuf *sb,
>
> If you rename `sb` to `out`, it makes it more obvious to the casual reader
> that this strbuf will receive the output.

This is gone in the re-roll.

> > +     cp.git_cmd = 1;
> > +     argv_array_pushl(&cp.args, "branch-diff", "--no-color", NULL);
>
> Obviously, this needs to be "range-diff" now.

The re-roll takes advantage of the libified range-diff rather than invoking it as a command.

Show 5 quoted lines

> > +     argv_array_pushv(&cp.args, ranges->argv);
>
> As there must be exactly two ranges, it would make more sense to pass them
> explicitly. And then you can use one single call to `argv_array_pushl()`
> instead.

Gone in the re-roll.

Show 9 quoted lines

> > +     struct strbuf diff = STRBUF_INIT;
>
> I guess you want to call it `diff` instead of `range_diff` because a
> future change will reuse this for the interdiff instead? And then also to
> avoid a naming conflict with the parameter.
>
> Dunno. I would still call it `range_diff` until the day comes (if ever)
> when `--interdiff` is implemented. And I would call the `range_diff`
> parameter `range_diff_opt` instead, or some such.

Sharing the variable between interdiff and range-diff was the idea initially, however, I later decided that the --range-diff and --interdiff options didn't need to be mutually-exclusive, so, in the re-roll, all variables now have distinct names (no commonality between them).

Show 8 quoted lines

> > +     if (range_diff) {
> > +             struct argv_array ranges = ARGV_ARRAY_INIT;
> > +             infer_diff_ranges(&ranges, range_diff, head);
> > +             if (get_range_diff(&diff, &ranges))
> > +                     die(_("failed to generate range-diff"));
>
> BTW I like to have an extra space in front of all the range-diff lines, to
> make it easier to discern them from the rest.

I'm not sure what you mean. Perhaps I'm misreading your comment.

Show 8 quoted lines

> >       if (!use_stdout &&
> >           open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
> > -             return;
> > +             goto done;
>
> Hmm. If you think you will add more `goto done`s in the future, I guess
> this is okay. Otherwise, it would probably make sense to go ahead and
> simply `strbuf_release(&diff)` before `return`ing.

In the re-roll, there are several more things which need to be cleaned up, so the 'goto' makes life easier.

Show 5 quoted lines

> > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > +     const char *range_diff = NULL;
>
> Maybe `range_diff_opt`? It's not exactly the range diff that is contained
> in this variable.

I could, though I was trying to keep it shorter rather than longer. This is still the same in the re-roll, but I can rename it if you insist.

Show 5 quoted lines

> > +     if (range_diff && !cover_letter)
> > +             die(_("--range-diff requires --cover-letter"));
>
> I guess this will be changed in a future patch, allowing a single patch to
> ship with a range diff, too?

Yes, that's already the case in the re-roll.

Show 14 quoted lines

> > +format_patch () {
> > +     title=$1 &&
> > +     range=$2 &&
> > +     test_expect_success "format-patch --range-diff ($title)" '
> > +             git format-patch --stdout --cover-letter --range-diff=$range \
> > +                     master..unmodified >actual &&
> > +             grep "= 1: .* s/5/A" actual &&
> > +             grep "= 2: .* s/4/A" actual &&
> > +             grep "= 3: .* s/11/B" actual &&
> > +             grep "= 4: .* s/12/B" actual
>
> I guess this might make sense if `format_patch` was not a function, but it
> is specifically marked as a function... so... shouldn't these `grep`s also
> be using function parameters?

A later patch adds a second test which specifies the same ranges but in a different way, so the result will be the same, hence the hard-coded grep'ing. The function avoids repetition across the two tests. I suppose I could do this a bit differently, though, to avoid pretending it's a general-purpose function.