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

[Eric Sunshine <sunshine@sunshineco.com>]

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:
> 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.

> > +     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.

> > +     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).

> > +     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.

> >       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.

> > @@ -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.

> > +     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.

> > +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.