Re: [PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter
Eric Sunshine writes:
> On Mon, Jul 23, 2018 at 12:03 PM Duy Nguyen wrote:
>> On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine
>> wrote:
>> > @@ -0,0 +1,17 @@
>> > +void show_interdiff(struct rev_info *rev)
>> > +{
>> > + struct diff_options opts;
>> > +
>> > + memcpy(, >diffopt, sizeof(opts));
>> > + opts.output_format = DIFF_FORMAT_PATCH;
>> > + diff_setup_done();
>> > +
>> > + diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", );
>> > + diffcore_std();
>> > + diff_flush();
>> > +}
>>
>> Is it worth adding a new file just for a single function? I haven't
>> read the rest of the series, but the cover letter's diffstat suggests
>> this is it. Is interdiff intended to become a lot more complicated in
>> the future? If not maybe just add this function in diff-lib.c
>
> Good question. The functionality originally lived in builtin/log.c but
> moved to log-tree.c when I added the ability to embed an interdiff in
> a single patch. However, it didn't "feel" right in log-tree.c, so I
> moved it to its own file to mirror how the range-diff engine resides
> in its own file.
> And, the function actually did several more things as originally
> implemented. For instance, it took care of not clobbering global
> diff-queue state, and consulting 'reroll_count' and printing the
> "Interdiff:" header, but those bits eventually moved to live at more
> "correct" locations, leaving this relatively minimal function behind.
> It does get a bit more complex in a later patch, but not significantly
> so.
>
> I wasn't aware of diff-lib.c, but it does seem like show_interdiff()
> could be at home there.
Yeah, diff-lib.c is meant to be a home for the implementation of low
level "diff-{files,tree,index}" plumbing that can be called from
other routines; if you are adding "take two things, compare them"
that can be reused from places, then it is a good match.
Re: [PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter
On Mon, Jul 23, 2018 at 12:03 PM Duy Nguyen wrote:
> On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine
> wrote:
> > @@ -0,0 +1,17 @@
> > +void show_interdiff(struct rev_info *rev)
> > +{
> > + struct diff_options opts;
> > +
> > + memcpy(, >diffopt, sizeof(opts));
> > + opts.output_format = DIFF_FORMAT_PATCH;
> > + diff_setup_done();
> > +
> > + diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", );
> > + diffcore_std();
> > + diff_flush();
> > +}
>
> Is it worth adding a new file just for a single function? I haven't
> read the rest of the series, but the cover letter's diffstat suggests
> this is it. Is interdiff intended to become a lot more complicated in
> the future? If not maybe just add this function in diff-lib.c
Good question. The functionality originally lived in builtin/log.c but
moved to log-tree.c when I added the ability to embed an interdiff in
a single patch. However, it didn't "feel" right in log-tree.c, so I
moved it to its own file to mirror how the range-diff engine resides
in its own file.
And, the function actually did several more things as originally
implemented. For instance, it took care of not clobbering global
diff-queue state, and consulting 'reroll_count' and printing the
"Interdiff:" header, but those bits eventually moved to live at more
"correct" locations, leaving this relatively minimal function behind.
It does get a bit more complex in a later patch, but not significantly
so.
I wasn't aware of diff-lib.c, but it does seem like show_interdiff()
could be at home there.
Re: [PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter
On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine wrote:
> diff --git a/interdiff.c b/interdiff.c
> new file mode 100644
> index 00..d0fac10c7c
> --- /dev/null
> +++ b/interdiff.c
> @@ -0,0 +1,17 @@
> +#include "cache.h"
> +#include "commit.h"
> +#include "revision.h"
> +#include "interdiff.h"
> +
> +void show_interdiff(struct rev_info *rev)
> +{
> + struct diff_options opts;
> +
> + memcpy(, >diffopt, sizeof(opts));
> + opts.output_format = DIFF_FORMAT_PATCH;
> + diff_setup_done();
> +
> + diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", );
> + diffcore_std();
> + diff_flush();
> +}
Is it worth adding a new file just for a single function? I haven't
read the rest of the series, but the cover letter's diffstat suggests
this is it. Is interdiff intended to become a lot more complicated in
the future? If not maybe just add this function in diff-lib.c
--
Duy
Re: [PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter
On Sun, Jul 22, 2018 at 5:57 AM Eric Sunshine wrote:
> Add an --interdiff option to automate this process. The argument to
> --interdiff specifies the tip of the previous attempt against which to
> generate the interdiff.
>
> Signed-off-by: Eric Sunshine
> ---
> diff --git a/interdiff.c b/interdiff.c
> @@ -0,0 +1,17 @@
> +void show_interdiff(struct rev_info *rev)
> +{
> + struct diff_options opts;
> +
> + memcpy(, >diffopt, sizeof(opts));
> + opts.output_format = DIFF_FORMAT_PATCH;
> + diff_setup_done();
> +
> + diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", );
Passing a 'rev_info' to this function is overkill. It actually only
needs the two OID's and a 'diff_options'. I'll make the change if I
need to reroll for some other reason, otherwise I'll do it as a
follow-up patch.
[PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter
When submitting a revised version of a patch series, it can be helpful
(to reviewers) to include a summary of changes since the previous
attempt in the form of an interdiff, however, doing so involves manually
copy/pasting the diff into the cover letter.
Add an --interdiff option to automate this process. The argument to
--interdiff specifies the tip of the previous attempt against which to
generate the interdiff. For example:
git format-patch --cover-letter --interdiff=v1 -3 v2
The previous attempt and the patch series being formatted must share a
common base.
Signed-off-by: Eric Sunshine
---
Documentation/git-format-patch.txt | 9 +
Makefile | 1 +
builtin/log.c | 24 ++--
interdiff.c| 17 +
interdiff.h| 8
revision.h | 4
t/t4014-format-patch.sh| 17 +
7 files changed, 78 insertions(+), 2 deletions(-)
create mode 100644 interdiff.c
create mode 100644 interdiff.h
diff --git a/Documentation/git-format-patch.txt
b/Documentation/git-format-patch.txt
index b41e1329a7..a1b1bafee7 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -23,6 +23,7 @@ SYNOPSIS
[(--reroll-count|-v) ]
[--to=] [--cc=]
[--[no-]cover-letter] [--quiet] [--notes[=]]
+ [--interdiff=]
[--progress]
[]
[ | ]
@@ -228,6 +229,14 @@ feeding the result to `git send-email`.
containing the branch description, shortlog and the overall diffstat.
You can
fill in a description in the file before sending it out.
+--interdiff=::
+ As a reviewer aid, insert an interdiff 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 example `git format-patch
+ --cover-letter --interdiff=feature/v1 -3 feature/v2`).
+
--notes[=]::
Append the notes (see linkgit:git-notes[1]) for the commit
after the three-dash line.
diff --git a/Makefile b/Makefile
index 41b93689ad..2af389c0d9 100644
--- a/Makefile
+++ b/Makefile
@@ -872,6 +872,7 @@ LIB_OBJS += linear-assignment.o
LIB_OBJS += help.o
LIB_OBJS += hex.o
LIB_OBJS += ident.o
+LIB_OBJS += interdiff.o
LIB_OBJS += kwset.o
LIB_OBJS += levenshtein.o
LIB_OBJS += line-log.o
diff --git a/builtin/log.c b/builtin/log.c
index 873aabcf40..1020b78477 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -30,6 +30,7 @@
#include "gpg-interface.h"
#include "progress.h"
#include "commit-slab.h"
+#include "interdiff.h"
#define MAIL_DEFAULT_WRAP 72
@@ -1082,6 +1083,11 @@ static void make_cover_letter(struct rev_info *rev, int
use_stdout,
/* We can only do diffstat with a unique reference point */
if (origin)
show_diffstat(rev, origin, head);
+
+ if (rev->idiff_oid1) {
+ fprintf_ln(rev->diffopt.file, "%s", _("Interdiff:"));
+ show_interdiff(rev);
+ }
}
static const char *clean_message_id(const char *msg_id)
@@ -1448,6 +1454,7 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
struct base_tree_info bases;
int show_progress = 0;
struct progress *progress = NULL;
+ struct oid_array idiff_prev = OID_ARRAY_INIT;
const struct option builtin_format_patch_options[] = {
{ OPTION_CALLBACK, 'n', "numbered", , NULL,
@@ -1521,6 +1528,9 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
OPT__QUIET(, N_("don't print the patch filenames")),
OPT_BOOL(0, "progress", _progress,
N_("show progress while generating patches")),
+ OPT_CALLBACK(0, "interdiff", _prev, N_("rev"),
+N_("show changes against in cover letter"),
+parse_opt_object_name),
OPT_END()
};
@@ -1706,7 +1716,7 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
if (rev.pending.nr == 2) {
struct object_array_entry *o = rev.pending.objects;
if (oidcmp([0].item->oid, [1].item->oid) == 0)
- return 0;
+ goto done;
}
get_patch_ids(, );
}
@@ -1730,7 +1740,7 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
}
if (nr == 0)
/* nothing to do */
- return 0;
+ goto done;
total = nr;
if (cover_letter == -1) {
