Re: [PATCH 14/14] format-patch: allow --range-diff to apply to a lone-patch

2018-09-07 Thread Eric Sunshine
On Wed, Jul 25, 2018 at 5:07 PM Junio C Hamano  wrote:
> Eric Sunshine  writes:
> > + if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
> > + struct diff_queue_struct dq;
> > +
> > + memcpy(, _queued_diff, sizeof(diff_queued_diff));
> > + DIFF_QUEUE_CLEAR(_queued_diff);
> > +
> > + next_commentary_block(opt, NULL);
> > + fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
> > + show_range_diff(opt->rdiff1, opt->rdiff2,
> > + opt->creation_factor, 1, >diffopt);
> > +
> > + memcpy(_queued_diff, , sizeof(diff_queued_diff));
> > + }
> >  }
>
> This essentially repeats what is already done for "interdiff".

Yes, the two blocks are very similar, although they access different
members of 'rev_info' and call different functions to perform the
actual diff. I explored ways of avoiding the repeated boilerplate
(using macros or passing a function pointer to a driver function
containing the boilerplate), but the end result was uglier and harder
to understand due to the added abstraction. Introducing
next_commentary_block()[1] reduced the repetition a bit.

> Does the global diff_queued_diff gets cleaned up when
> show_interdiff() and show_range_diff() return, like diff_flush()
> does?  Otherwise we'd be leaking the filepairs accumulated in the
> diff_queued_diff.

Both show_interdiff() and show_range_diff() call diff_flush(). So, the
"temporary" diff_queued_diff set up here does get cleaned up by
diff_flush(), as far as I understand (though this is my first foray
into the diff-generation code, so I may be missing something). And,
the diff_queued_diff which gets "interrupted" by this excursion into
interdiff/range-diff gets cleaned up normally when the interrupted
diff operation completes.

[1]: 
https://public-inbox.org/git/20180722095717.17912-6-sunsh...@sunshineco.com/


Re: [PATCH 14/14] format-patch: allow --range-diff to apply to a lone-patch

2018-07-25 Thread Junio C Hamano
Eric Sunshine  writes:

> @@ -750,6 +751,20 @@ void show_log(struct rev_info *opt)
>  
>   memcpy(_queued_diff, , sizeof(diff_queued_diff));
>   }
> +
> + if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
> + struct diff_queue_struct dq;
> +
> + memcpy(, _queued_diff, sizeof(diff_queued_diff));
> + DIFF_QUEUE_CLEAR(_queued_diff);
> +
> + next_commentary_block(opt, NULL);
> + fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
> + show_range_diff(opt->rdiff1, opt->rdiff2,
> + opt->creation_factor, 1, >diffopt);
> +
> + memcpy(_queued_diff, , sizeof(diff_queued_diff));
> + }
>  }
>  
>  int log_tree_diff_flush(struct rev_info *opt)

This essentially repeats what is already done for "interdiff".

Does the global diff_queued_diff gets cleaned up when
show_interdiff() and show_range_diff() return, like diff_flush()
does?  Otherwise we'd be leaking the filepairs accumulated in the
diff_queued_diff.



[PATCH 14/14] format-patch: allow --range-diff to apply to a lone-patch

2018-07-22 Thread Eric Sunshine
When submitting a revised version of a patch or series, it can be
helpful (to reviewers) to include a summary of changes since the
previous attempt in the form of a range-diff, typically in the cover
letter. However, it is occasionally useful, despite making for a noisy
read, to insert a range-diff into the commentary section of the lone
patch of a 1-patch series.

Therefore, extend "git format-patch --range-diff=" to insert a
range-diff into the commentary section of a lone patch rather than
requiring a cover letter.

Implementation note: Generating a range-diff for insertion into the
commentary section of a patch which itself is currently being generated
requires invoking the diffing machinery recursively. However, the
machinery does not (presently) support this since it uses global state.
Consequently, we need to take care to stash away the state of the
in-progress operation while generating the range-diff, and restore it
after.

Signed-off-by: Eric Sunshine 
---
 Documentation/git-format-patch.txt |  3 ++-
 builtin/log.c  |  9 +
 log-tree.c | 15 +++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 9b2e172159..aba4c5febe 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -241,7 +241,8 @@ feeding the result to `git send-email`.
 
 --range-diff=::
As a reviewer aid, insert a range-diff (see linkgit:git-range-diff[1])
-   into the cover letter showing the differences between the previous
+   into the cover letter, or as commentary of the lone patch of a
+   1-patch series, showing the differences between the previous
version of the patch series and the series currently being formatted.
`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
diff --git a/builtin/log.c b/builtin/log.c
index 10c3801ceb..f0e99256a0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1575,7 +1575,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
 N_("show changes against  in cover letter or 
single patch"),
 parse_opt_object_name),
OPT_STRING(0, "range-diff", _prev, N_("refspec"),
-  N_("show changes against  in cover 
letter")),
+  N_("show changes against  in cover letter 
or single patch")),
OPT_INTEGER(0, "creation-factor", _factor,
N_("percentage by which creation is weighted")),
OPT_END()
@@ -1816,8 +1816,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
die(_("--creation-factor requires --range-diff"));
 
if (rdiff_prev) {
-   if (!cover_letter)
-   die(_("--range-diff requires --cover-letter"));
+   if (!cover_letter && total != 1)
+   die(_("--range-diff requires --cover-letter or single 
patch"));
 
infer_range_diff_ranges(, , rdiff_prev,
origin, list[0]);
@@ -1866,8 +1866,9 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
print_signature(rev.diffopt.file);
total++;
start_number--;
-   /* interdiff in cover-letter; omit from patches */
+   /* interdiff/range-diff in cover-letter; omit from patches */
rev.idiff_oid1 = NULL;
+   rev.rdiff1 = NULL;
}
rev.add_signoff = do_signoff;
 
diff --git a/log-tree.c b/log-tree.c
index 56513fa83d..37afc585dc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -15,6 +15,7 @@
 #include "line-log.h"
 #include "help.h"
 #include "interdiff.h"
+#include "range-diff.h"
 
 static struct decoration name_decoration = { "object names" };
 static int decoration_loaded;
@@ -750,6 +751,20 @@ void show_log(struct rev_info *opt)
 
memcpy(_queued_diff, , sizeof(diff_queued_diff));
}
+
+   if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
+   struct diff_queue_struct dq;
+
+   memcpy(, _queued_diff, sizeof(diff_queued_diff));
+   DIFF_QUEUE_CLEAR(_queued_diff);
+
+   next_commentary_block(opt, NULL);
+   fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
+   show_range_diff(opt->rdiff1, opt->rdiff2,
+   opt->creation_factor, 1, >diffopt);
+
+   memcpy(_queued_diff, , sizeof(diff_queued_diff));
+   }
 }
 
 int log_tree_diff_flush(struct rev_info *opt)
-- 
2.18.0.345.g5c9ce644c3