Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, May 30, 2018 at 2:58 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> When submitting a revised a patch series, the --range-diff option embeds
>> a range-diff in the cover letter showing changes since the previous
>> version of the patch series. The argument to --range-diff is a simple
>> revision naming the tip of the previous series, which works fine if the
>> previous and current versions of the patch series share a common base.
>>
>> However, it fails if the revision ranges of the old and new versions of
>> the series are disjoint. To address this shortcoming, extend
>> --range-diff to also accept an explicit revision range for the previous
>> series. For example:
>>
>>     git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>>  static void infer_diff_ranges(struct argv_array *args,
>>                               const char *prev,
>> +                             struct commit *origin,
>>                               struct commit *head)
>>  {
>> -       argv_array_pushf(args, "%s...%s", prev,
>> -                        oid_to_hex(&head->object.oid));
>> +       if (strstr(prev, "..")) {
>> +               if (!origin)
>> +                       die(_("failed to infer range-diff ranges"));
>
>>  static int get_range_diff(struct strbuf *sb,
>> @@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>>         /* might die from bad user input so try before creating cover letter */
>>         if (range_diff) {
>>                 struct argv_array ranges = ARGV_ARRAY_INIT;
>> -               infer_diff_ranges(&ranges, range_diff, head);
>> +               infer_diff_ranges(&ranges, range_diff, origin, head);
>
> This is way before the check for 'if (origin) emit_diffstat' as done in
> patch 1, as we want to do this early. We need to check the non-NULLness
> in infer_diff_ranges [...]

Correct.

> Would it make sense to give a better error message in:
>
>> +               if (!origin)
>> +                       die(_("failed to infer range-diff ranges"));
>
> above? (The failure sounds like it could be anything, but the
> reason for it is actually well understood: The origin was
> computed and as the boundary commit of the given range,
> or NULL if there is no boundary commit or multiple commits.

I'm not entirely happy with the generic error message either but
didn't come up with anything better which was both succinct and
actually helpful. I was hoping that a reviewer might suggest something
better.

> If we find not exactly one boundary, we cannot compute the
> range to give to the range-diff tool.

I considered allowing the argument to --range-diff to be much more
complex: to provide a way for the user to supply all information
needed for the invocation of git-range-diff (basically, to manually
supply arguments to git-range-diff) for cases when inference wasn't
possible. I even went so far as to consider allowing the
'creation-weight' to be specified as part of the --range-diff
argument. However, that sort of complexity is both difficult to
explain (document) and tends to be painful for users to specify (type
correctly).

Therefore, in the end, I opted for simplicity: namely, handle the
common cases with straight-forward minimal-learning-curve standard
options. Both --range-diff=v3 and --range-diff=v3~4..v3 are easy to
document and understand, as is the distinct --creation-weight= option.
This seems a good balance between extreme flexibility and relative
simplicity for handling common needs. (And, --range-diff is just a
convenience, after all; more complex scenarios can still be handled
manually by invoking git-range-diff followed by copy/paste.)

> Stepping back to the error message, I have no good
> suggestion on what to say there. Maybe we'd want to
> refactor the code in cmd_format_patch, that computes the
> origin:
>
>         if (prepare_revision_walk(&rev))
>                 die(_("revision walk setup failed"));
>         rev.boundary = 1;
>         while ((commit = get_revision(&rev)) != NULL) {
>                 if (commit->object.flags & BOUNDARY) {
>                         boundary_count++;
>                         origin = (boundary_count == 1) ? commit : NULL;
>                         continue;
>                 }
>
> We could prefix that with
>
>     need_origin = (range_diff && strstr(prev, "..")  || emit_diff_stat;
>
> and then if need_origin is about to be unset again we could issue
> a warning and die early after the loop warning about bad DAG form?
>
> I guess that can wait for a follow up patch.

I agree. The feature can be improved and made more fancy incrementally
as we gain better understanding of areas which need improvement. As a
first step, I wanted to keep it minimal but usable for the most common
cases.

Thanks for the review.