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

From: Stefan Beller <sbeller@google.com>
Date: May 30, 2018, 18:58 UTC
Message-ID: <CAGZ79kZSjwsk-YE1G3sGX-sfdu17NjMo-Jbrw2E5AbpbNGSGyg@mail.gmail.com>
In-Reply-To: <20180530080325.37520-4-sunshine@sunshineco.com>

On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 56 quoted lines

> 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>
> ---
>  Documentation/git-format-patch.txt |  8 +++++---
>  builtin/log.c                      | 16 +++++++++++++---
>  t/t7910-branch-diff.sh             |  1 +
>  3 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index f4c70e6b64..25026ae26e 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -233,10 +233,12 @@ feeding the result to `git send-email`.
>         As a reviewer aid, insert a range-diff (see linkgit:git-branch-diff[1])
>         into the cover letter showing the differences between the previous
>         version of the patch series and the series currently being formatted.
> -       `previous` is a single revision naming the tip of the previous
> -       series which shares a common base with the series being formatted (for
> +       `previous` can be a single revision naming the tip of the previous
> +       series if it shares a common base with the series being formatted (for
>         example `git format-patch --cover-letter --range-diff=feature/v1 -3
> -       feature/v2`).
> +       feature/v2`), or a revision range if the two versions of the series are
> +       disjoint (for example `git format-patch --cover-letter
> +       --range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
>
>  --notes[=<ref>]::
>         Append the notes (see linkgit:git-notes[1]) for the commit
> diff --git a/builtin/log.c b/builtin/log.c
> index 460c31a293..e38cf06050 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -995,10 +995,20 @@ static char *find_branch_name(struct rev_info *rev)
>
>  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"));

Show 7 quoted lines

>  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 as it is allowed to be NULL when the range-diff doesn't contain "..", which means we assume the same branch point.

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.

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

Stepping back a bit:

  Consider the following DAG:

        O -
           \
    A - B - C - D

a series from B..D (or '-2' when D is HEAD) would fail to generate both diffstat as well as the range diff as the merge of O produces a second boundary, (but wouldn't the creation of the patch C fail eventually anyway?)

Another DAG:

    A - B - C - D
         \    /
           E

when asking for B..D the diffstat and range diff would compute ok, but the patch C would fail to generate again?

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;
                }

                if (ignore_if_in_upstream && has_commit_patch_id(commit, &ids))
                        continue;

                nr++;
                REALLOC_ARRAY(list, nr);
                list[nr - 1] = commit;
        }

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.

Thanks, Stefan