Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)

From: Martin Ågren <martin.agren@gmail.com>
Date: Dec 3, 2018, 13:27 UTC
Message-ID: <CAN0heSrfH39-37KDU3XDhxiYs1_3eUMdjbdAm37cPAmnOYUZMA@mail.gmail.com>
In-Reply-To: <CAPig+cS4peHDOz0cK7RJEHb6ch0+czvau_aGTD1j6xM23G0-pQ@mail.gmail.com>

On Fri, 30 Nov 2018 at 10:32, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 10 quoted lines

>
> On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> > So how about doing this on top of 'master' instead?  As this leaks
> > *no* information wrt how range-diff machinery should behave from the
> > format-patch side by not passing any diffopt, as long as the new
> > code I added to show_range_diff() comes up with a reasonable default
> > diffopts (for which I really would appreciate extra sets of eyes to
> > make sure), this change by definition cannot be wrong (famous last
> > words).

> Another benefit of calling show_range_diff() directly is that when
> "git format-patch --stdout --range-diff=..." is sent to the terminal,
> the range-diff gets colored output for free. I'm pleased to see that
> that still works after this change.

(If the patch below makes any sense to you and you know more about this diff/color thing, the fourth bullet in the log message below might interest you.)

Show 10 quoted lines

> > diff --git a/range-diff.h b/range-diff.h
> > @@ -5,6 +5,11 @@
> > +/*
> > + * Compare series of commmits in RANGE1 and RANGE2, and emit to the
> > + * standard output.  NULL can be passed to DIFFOPT to use the built-in
> > + * default.
> > + */
>
> It is more correct to say that the range-diff is emitted to
> diffopt->file (which may be stdout).

This seems to be an important remark. There's a pretty bad regression here since when `diffopt` is NULL, we've lost our original, intended `diffopt->file` and the range-diff ends up on stdout.

Here's my whitespace-damaged WIP. I would be able to pick this up again in about 6h, but anyone is more than welcome to pick this up and run with it in the meantime.

This is not a corner of the code that I'm particularly familiar with...

-->8--
Subject: [PATCH] range-diff: always pass at least minimal diff options
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit d8981c3f88 ("format-patch: do not let its diff-options affect --range-diff", 2018-11-30) taught `show_range_diff()` to accept a NULL-pointer as an indication that it should use its own "reasonable default". That fixed a regression from a5170794 ("Merge branch 'ab/range-diff-no-patch'", 2018-11-18), but unfortunately it introduced a regression of its own.

In particular, it means we forget the `file` member of the diff options, so rather than placing a range-diff in the cover-letter, we write it to stdout. In order to fix this, rewrite the two callers adjusted by d8981c3f88 to create a "dummy" set of diff options where they only fill in which file to use.

A couple of remarks about this commit:

  * No tests. The change in builtin/log.c has been tested manually, the
    one in log-tree.c not at all, other than by running existing tests.

  * I have not convinced myself 100% that there aren't other things that
    are just as important as `file` to pass down.

  * `show_range_diff()` can still take NULL, although that is now dead
    code. If something like this here commit is deemed the proper fix
    for this, that code path could also go, either as part of this
    commit, or separately, once we've cut 2.20.

  * The range-diff is written colored regardless of destination, i.e.,
    when written to a file, it contains garbage.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/log.c | 12 +++++++++++-
 log-tree.c    | 12 +++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..0609e41ae5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,19 @@ static void make_cover_letter(struct rev_info
*rev, int use_stdout,
     }

     if (rev->rdiff1) {
+        /**
+         * (At least for now) we only want to pass down
+         * the file handle where we want the range-diff
+         * to appear. Avoid any other diff options until
+         * we know how we want to handle them.
+         */
+        struct diff_options opts;
+        diff_setup(&opts);
+        opts.file = rev->diffopt.file;
+        diff_setup_done(&opts);
         fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
         show_range_diff(rev->rdiff1, rev->rdiff2,
-                rev->creation_factor, 1, NULL);
+                rev->creation_factor, 1, &opts);
     }
 }

diff --git a/log-tree.c b/log-tree.c
index b243779a0b..bc355a4e91 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,24 @@ void show_log(struct rev_info *opt)

     if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
         struct diff_queue_struct dq;
+        struct diff_options opts;

         memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
         DIFF_QUEUE_CLEAR(&diff_queued_diff);

         next_commentary_block(opt, NULL);
         fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
+        /**
+         * (At least for now) we only want to pass down
+         * the file handle where we want the range-diff
+         * to appear. Avoid any other diff options until
+         * we know how we want to handle them.
+        */
+        diff_setup(&opts);
+        opts.file = opt->diffopt.file;
+        diff_setup_done(&opts);
         show_range_diff(opt->rdiff1, opt->rdiff2,
-                opt->creation_factor, 1, NULL);
+                opt->creation_factor, 1, &opts);

         memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
     }

-- 
2.20.0.rc2