Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter
Hi Eric, On Thu, 26 Jul 2018, Eric Sunshine wrote: > On Thu, Jul 26, 2018 at 6:56 AM Johannes Schindelin > wrote: > > On Tue, 17 Jul 2018, Eric Sunshine wrote: > > > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin > > > wrote: > > > > BTW I like to have an extra space in front of all the range-diff > > > > lines, to make it easier to discern them from the rest. > > > > > > I'm not sure what you mean. Perhaps I'm misreading your comment. > > > > Sorry, I was really unclear. > > > > In the cover letters sent out by GitGitGadget (or earlier, my > > mail-patch-series.sh command), I took pains to indent the entire > > range-diff (or interdiff) with a single space. That is, the footer > > "Range-diff vs v:" is not indented at all, but all subsequent lines > > of the range-diff have a leading space. > > > > The original reason was to stop confusing `git apply` when sending an > > interdiff as part of a single patch without a cover letter (in which > > case mail-patch-series.sh inserted the interdiff below the `---` > > marker, and the interdiff would have looked like the start of the real > > diff otherwise). > > The new version[1] likewise indents the interdiff to avoid confusing > git-am / git-apply. > > [1]: > https://public-inbox.org/git/20180722095717.17912-1-sunsh...@sunshineco.com/ Great! > > In the meantime, I got used to this indentation so much that I do not > > want to miss it, it is a relatively easy and intuitive visual marker. > > > > This, however, will be harder to achieve now that you are using the > > libified range-diff. > > I toyed with indenting the range-diff in both the cover letter and > below the "---" line in a patch. With the libified range-diff, doing > so involves modifying the range-diff implementation (rather than > having the consumer of the range-diff manage the indentation locally), > so it adds a bit of complexity to show_range_diff(), though perhaps > not too much. > > However, I opted against it for a few reasons. First, "header" lines > apart, all lines of the range-diff are already indented, and the > existing indentation was sufficient (for me, at least) as a visual > marker. Second, range-diffs tend to be _wide_, especially the header > lines, and I was loath to make it wider by indenting more. Third, due > to the existing indentation of the diff proper, a range-diff won't > confuse git-am / git-apply, nor will the unindented header lines, so > extra indentation seemed superfluous. Totally understandable. For some reason, I never thought about that fact (a range-diff is *not* a diff) when changing mail-patch-series.ts to use range-diffs instead of interdiffs. > > > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char > > > > > **argv, const char *prefix) > > > > > + const char *range_diff = NULL; > > > > > > > > Maybe `range_diff_opt`? It's not exactly the range diff that is > > > > contained in this variable. > > > > > > I could, though I was trying to keep it shorter rather than longer. > > > This is still the same in the re-roll, but I can rename it if you > > > insist. > > > > I think it will confuse me in the future if I read `range_diff` and > > even the data type suggests that it could hold the output of a `git > > range-diff ` run. > > > > So I would like to insist. > > In the new version[1], this variable is named 'rdiff_prev' (the > "previous" version against which the range-diff is to be generated). Thank you. > > > > > +format_patch () { > > > > > + title=$1 && > > > > > + range=$2 && > > > > > + test_expect_success "format-patch --range-diff ($title)" ' > > > > > + git format-patch --stdout --cover-letter > > > > > --range-diff=$range \ > > > > > + master..unmodified >actual && > > > > > + grep "= 1: .* s/5/A" actual && > > > > > + grep "= 2: .* s/4/A" actual && > > > > > + grep "= 3: .* s/11/B" actual && > > > > > + grep "= 4: .* s/12/B" actual > > > > > > > > I guess this might make sense if `format_patch` was not a > > > > function, but it is specifically marked as a function... so... > > > > shouldn't these `grep`s also be using function parameters? > > > > > > A later patch adds a second test which specifies the same ranges but > > > in a different way, so the result will be the same, hence the > > > hard-coded grep'ing. The function avoids repetition across the two > > > tests. I suppose I could do this a bit differently, though, to avoid > > > pretending it's a general-purpose function. > > > > If you can think of a way that would make this easier to read for, > > say, myself if I ever find myself debugging a regression caught by > > this test, I would appreciate that. > > In the new version, the function is gone; it looks like this: > > --- >8 --- > for prev in topic master..topic > do > test_expect_success "format-patch --range-diff=$prev" ' > git format-patch --stdout --cover-letter --range-diff=$prev \ >
Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter
On Thu, Jul 26, 2018 at 6:56 AM Johannes Schindelin wrote: > On Tue, 17 Jul 2018, Eric Sunshine wrote: > > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin > > wrote: > > > BTW I like to have an extra space in front of all the range-diff lines, to > > > make it easier to discern them from the rest. > > > > I'm not sure what you mean. Perhaps I'm misreading your comment. > > Sorry, I was really unclear. > > In the cover letters sent out by GitGitGadget (or earlier, my > mail-patch-series.sh command), I took pains to indent the entire > range-diff (or interdiff) with a single space. That is, the footer > "Range-diff vs v:" is not indented at all, but all subsequent lines of > the range-diff have a leading space. > > The original reason was to stop confusing `git apply` when sending an > interdiff as part of a single patch without a cover letter (in which case > mail-patch-series.sh inserted the interdiff below the `---` marker, and > the interdiff would have looked like the start of the real diff > otherwise). The new version[1] likewise indents the interdiff to avoid confusing git-am / git-apply. [1]: https://public-inbox.org/git/20180722095717.17912-1-sunsh...@sunshineco.com/ > In the meantime, I got used to this indentation so much that I do not want > to miss it, it is a relatively easy and intuitive visual marker. > > This, however, will be harder to achieve now that you are using the > libified range-diff. I toyed with indenting the range-diff in both the cover letter and below the "---" line in a patch. With the libified range-diff, doing so involves modifying the range-diff implementation (rather than having the consumer of the range-diff manage the indentation locally), so it adds a bit of complexity to show_range_diff(), though perhaps not too much. However, I opted against it for a few reasons. First, "header" lines apart, all lines of the range-diff are already indented, and the existing indentation was sufficient (for me, at least) as a visual marker. Second, range-diffs tend to be _wide_, especially the header lines, and I was loath to make it wider by indenting more. Third, due to the existing indentation of the diff proper, a range-diff won't confuse git-am / git-apply, nor will the unindented header lines, so extra indentation seemed superfluous. > > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, > > > > const char *prefix) > > > > + const char *range_diff = NULL; > > > > > > Maybe `range_diff_opt`? It's not exactly the range diff that is contained > > > in this variable. > > > > I could, though I was trying to keep it shorter rather than longer. > > This is still the same in the re-roll, but I can rename it if you > > insist. > > I think it will confuse me in the future if I read `range_diff` and even > the data type suggests that it could hold the output of a `git range-diff > ` run. > > So I would like to insist. In the new version[1], this variable is named 'rdiff_prev' (the "previous" version against which the range-diff is to be generated). > > > > +format_patch () { > > > > + title=$1 && > > > > + range=$2 && > > > > + test_expect_success "format-patch --range-diff ($title)" ' > > > > + git format-patch --stdout --cover-letter > > > > --range-diff=$range \ > > > > + master..unmodified >actual && > > > > + grep "= 1: .* s/5/A" actual && > > > > + grep "= 2: .* s/4/A" actual && > > > > + grep "= 3: .* s/11/B" actual && > > > > + grep "= 4: .* s/12/B" actual > > > > > > I guess this might make sense if `format_patch` was not a function, but it > > > is specifically marked as a function... so... shouldn't these `grep`s also > > > be using function parameters? > > > > A later patch adds a second test which specifies the same ranges but > > in a different way, so the result will be the same, hence the > > hard-coded grep'ing. The function avoids repetition across the two > > tests. I suppose I could do this a bit differently, though, to avoid > > pretending it's a general-purpose function. > > If you can think of a way that would make this easier to read for, say, > myself if I ever find myself debugging a regression caught by this test, I > would appreciate that. In the new version, the function is gone; it looks like this: --- >8 --- for prev in topic master..topic do test_expect_success "format-patch --range-diff=$prev" ' git format-patch --stdout --cover-letter --range-diff=$prev \ master..unmodified >actual && grep "= 1: .* s/5/A" actual && grep "= 2: .* s/4/A" actual && grep "= 3: .* s/11/B" actual && grep "= 4: .* s/12/B" actual ' done --- >8 ---
Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter
Hi Eric, On Tue, 17 Jul 2018, Eric Sunshine wrote: > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin > wrote: > > On Wed, 30 May 2018, Eric Sunshine wrote: > > > > + if (range_diff) { > > > + struct argv_array ranges = ARGV_ARRAY_INIT; > > > + infer_diff_ranges(, range_diff, head); > > > + if (get_range_diff(, )) > > > + die(_("failed to generate range-diff")); > > > > BTW I like to have an extra space in front of all the range-diff lines, to > > make it easier to discern them from the rest. > > I'm not sure what you mean. Perhaps I'm misreading your comment. Sorry, I was really unclear. In the cover letters sent out by GitGitGadget (or earlier, my mail-patch-series.sh command), I took pains to indent the entire range-diff (or interdiff) with a single space. That is, the footer "Range-diff vs v:" is not indented at all, but all subsequent lines of the range-diff have a leading space. The original reason was to stop confusing `git apply` when sending an interdiff as part of a single patch without a cover letter (in which case mail-patch-series.sh inserted the interdiff below the `---` marker, and the interdiff would have looked like the start of the real diff otherwise). In the meantime, I got used to this indentation so much that I do not want to miss it, it is a relatively easy and intuitive visual marker. This, however, will be harder to achieve now that you are using the libified range-diff. > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, > > > const char *prefix) > > > + const char *range_diff = NULL; > > > > Maybe `range_diff_opt`? It's not exactly the range diff that is contained > > in this variable. > > I could, though I was trying to keep it shorter rather than longer. > This is still the same in the re-roll, but I can rename it if you > insist. I think it will confuse me in the future if I read `range_diff` and even the data type suggests that it could hold the output of a `git range-diff ` run. So I would like to insist. > > > +format_patch () { > > > + title=$1 && > > > + range=$2 && > > > + test_expect_success "format-patch --range-diff ($title)" ' > > > + git format-patch --stdout --cover-letter > > > --range-diff=$range \ > > > + master..unmodified >actual && > > > + grep "= 1: .* s/5/A" actual && > > > + grep "= 2: .* s/4/A" actual && > > > + grep "= 3: .* s/11/B" actual && > > > + grep "= 4: .* s/12/B" actual > > > > I guess this might make sense if `format_patch` was not a function, but it > > is specifically marked as a function... so... shouldn't these `grep`s also > > be using function parameters? > > A later patch adds a second test which specifies the same ranges but > in a different way, so the result will be the same, hence the > hard-coded grep'ing. The function avoids repetition across the two > tests. I suppose I could do this a bit differently, though, to avoid > pretending it's a general-purpose function. If you can think of a way that would make this easier to read for, say, myself if I ever find myself debugging a regression caught by this test, I would appreciate that. Ciao, Dscho
Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter
Thanks for the review comments. In the new version of the series (almost complete), almost the entire implementation has changed, including most of the stuff on which you commented. Anyhow, see my responses to your comments below... On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin wrote: > On Wed, 30 May 2018, Eric Sunshine wrote: > > +static int get_range_diff(struct strbuf *sb, > > If you rename `sb` to `out`, it makes it more obvious to the casual reader > that this strbuf will receive the output. This is gone in the re-roll. > > + cp.git_cmd = 1; > > + argv_array_pushl(, "branch-diff", "--no-color", NULL); > > Obviously, this needs to be "range-diff" now. The re-roll takes advantage of the libified range-diff rather than invoking it as a command. > > + argv_array_pushv(, ranges->argv); > > As there must be exactly two ranges, it would make more sense to pass them > explicitly. And then you can use one single call to `argv_array_pushl()` > instead. Gone in the re-roll. > > + struct strbuf diff = STRBUF_INIT; > > I guess you want to call it `diff` instead of `range_diff` because a > future change will reuse this for the interdiff instead? And then also to > avoid a naming conflict with the parameter. > > Dunno. I would still call it `range_diff` until the day comes (if ever) > when `--interdiff` is implemented. And I would call the `range_diff` > parameter `range_diff_opt` instead, or some such. Sharing the variable between interdiff and range-diff was the idea initially, however, I later decided that the --range-diff and --interdiff options didn't need to be mutually-exclusive, so, in the re-roll, all variables now have distinct names (no commonality between them). > > + if (range_diff) { > > + struct argv_array ranges = ARGV_ARRAY_INIT; > > + infer_diff_ranges(, range_diff, head); > > + if (get_range_diff(, )) > > + die(_("failed to generate range-diff")); > > BTW I like to have an extra space in front of all the range-diff lines, to > make it easier to discern them from the rest. I'm not sure what you mean. Perhaps I'm misreading your comment. > > if (!use_stdout && > > open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", > > rev, quiet)) > > - return; > > + goto done; > > Hmm. If you think you will add more `goto done`s in the future, I guess > this is okay. Otherwise, it would probably make sense to go ahead and > simply `strbuf_release()` before `return`ing. In the re-roll, there are several more things which need to be cleaned up, so the 'goto' makes life easier. > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, > > const char *prefix) > > + const char *range_diff = NULL; > > Maybe `range_diff_opt`? It's not exactly the range diff that is contained > in this variable. I could, though I was trying to keep it shorter rather than longer. This is still the same in the re-roll, but I can rename it if you insist. > > + if (range_diff && !cover_letter) > > + die(_("--range-diff requires --cover-letter")); > > I guess this will be changed in a future patch, allowing a single patch to > ship with a range diff, too? Yes, that's already the case in the re-roll. > > +format_patch () { > > + title=$1 && > > + range=$2 && > > + test_expect_success "format-patch --range-diff ($title)" ' > > + git format-patch --stdout --cover-letter --range-diff=$range \ > > + master..unmodified >actual && > > + grep "= 1: .* s/5/A" actual && > > + grep "= 2: .* s/4/A" actual && > > + grep "= 3: .* s/11/B" actual && > > + grep "= 4: .* s/12/B" actual > > I guess this might make sense if `format_patch` was not a function, but it > is specifically marked as a function... so... shouldn't these `grep`s also > be using function parameters? A later patch adds a second test which specifies the same ranges but in a different way, so the result will be the same, hence the hard-coded grep'ing. The function avoids repetition across the two tests. I suppose I could do this a bit differently, though, to avoid pretending it's a general-purpose function.
Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter
Hi Eric, On Wed, 30 May 2018, Eric Sunshine wrote: > diff --git a/builtin/log.c b/builtin/log.c > index e01a256c11..460c31a293 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -28,6 +28,7 @@ > #include "mailmap.h" > #include "gpg-interface.h" > #include "progress.h" > +#include "run-command.h" > > #define MAIL_DEFAULT_WRAP 72 > > @@ -992,6 +993,25 @@ static char *find_branch_name(struct rev_info *rev) > return branch; > } > > +static void infer_diff_ranges(struct argv_array *args, > + const char *prev, > + struct commit *head) > +{ > + argv_array_pushf(args, "%s...%s", prev, > + oid_to_hex(>object.oid)); > +} > + > +static int get_range_diff(struct strbuf *sb, If you rename `sb` to `out`, it makes it more obvious to the casual reader that this strbuf will receive the output. > + const struct argv_array *ranges) > +{ > + struct child_process cp = CHILD_PROCESS_INIT; > + > + cp.git_cmd = 1; > + argv_array_pushl(, "branch-diff", "--no-color", NULL); Obviously, this needs to be "range-diff" now. > + argv_array_pushv(, ranges->argv); As there must be exactly two ranges, it would make more sense to pass them explicitly. And then you can use one single call to `argv_array_pushl()` instead. > + return capture_command(, sb, 0); > +} > + > static void emit_diffstat(struct rev_info *rev, > struct commit *origin, struct commit *head) > { > @@ -1016,7 +1036,8 @@ static void make_cover_letter(struct rev_info *rev, int > use_stdout, > struct commit *origin, > int nr, struct commit **list, > const char *branch_name, > - int quiet) > + int quiet, > + const char *range_diff) > { > const char *committer; > const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n"; > @@ -1028,15 +1049,25 @@ static void make_cover_letter(struct rev_info *rev, > int use_stdout, > int need_8bit_cte = 0; > struct pretty_print_context pp = {0}; > struct commit *head = list[0]; > + struct strbuf diff = STRBUF_INIT; I guess you want to call it `diff` instead of `range_diff` because a future change will reuse this for the interdiff instead? And then also to avoid a naming conflict with the parameter. Dunno. I would still call it `range_diff` until the day comes (if ever) when `--interdiff` is implemented. And I would call the `range_diff` parameter `range_diff_opt` instead, or some such. Or maybe use `extra_footer` instead of `diff`. > if (!cmit_fmt_is_mail(rev->commit_format)) > die(_("Cover letter needs email format")); > > committer = git_committer_info(0); > > + /* 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(, range_diff, head); > + if (get_range_diff(, )) > + die(_("failed to generate range-diff")); BTW I like to have an extra space in front of all the range-diff lines, to make it easier to discern them from the rest. > + argv_array_clear(); > + } > + > if (!use_stdout && > open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", > rev, quiet)) > - return; > + goto done; Hmm. If you think you will add more `goto done`s in the future, I guess this is okay. Otherwise, it would probably make sense to go ahead and simply `strbuf_release()` before `return`ing. > log_write_email_headers(rev, head, _subject, _8bit_cte); > > @@ -1077,6 +1108,17 @@ static void make_cover_letter(struct rev_info *rev, > int use_stdout, > /* We can only do diffstat with a unique reference point */ > if (origin) > emit_diffstat(rev, origin, head); > + > + if (diff.len) { > + FILE *fp = rev->diffopt.file; > + fputs(_("Changes since previous version:"), fp); > + fputs("\n\n", fp); > + fputs(diff.buf, fp); > + fputc('\n', fp); > + } > + > +done: > + strbuf_release(); > } > > static const char *clean_message_id(const char *msg_id) > @@ -1438,6 +1480,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; > + const char *range_diff = NULL; Maybe `range_diff_opt`? It's not exactly the range diff that is contained in this variable. > const struct option builtin_format_patch_options[] = { > { OPTION_CALLBACK, 'n', "numbered", , NULL, > @@ -1511,6 +1554,8 @@ int cmd_format_patch(int argc, const char **argv, const > char *prefix) > OPT__QUIET(,
[RFC PATCH 2/5] format-patch: add --range-diff 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 or range-diff, however, doing so involves manually copy/pasting the diff into the cover letter. Add a --range-diff option to automate this process for range-diffs. The argument to --range-diff specifies the tip of the previous attempt against which to generate the range-diff. For example: git format-patch --cover-letter --range-diff=v1 -3 v2 (At this early stage, the previous attempt and the patch series being formatted must share a common base, however, a subsequent enhancement will make it possible to specify an explicit revision range for the previous attempt.) Signed-off-by: Eric Sunshine --- Documentation/git-format-patch.txt | 10 ++ builtin/log.c | 55 -- t/t7910-branch-diff.sh | 15 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 6cbe462a77..f4c70e6b64 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[=]] + [--range-diff=] [--progress] [] [ | ] @@ -228,6 +229,15 @@ 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. +--range-diff=:: + 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 + example `git format-patch --cover-letter --range-diff=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/builtin/log.c b/builtin/log.c index e01a256c11..460c31a293 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -28,6 +28,7 @@ #include "mailmap.h" #include "gpg-interface.h" #include "progress.h" +#include "run-command.h" #define MAIL_DEFAULT_WRAP 72 @@ -992,6 +993,25 @@ static char *find_branch_name(struct rev_info *rev) return branch; } +static void infer_diff_ranges(struct argv_array *args, + const char *prev, + struct commit *head) +{ + argv_array_pushf(args, "%s...%s", prev, +oid_to_hex(>object.oid)); +} + +static int get_range_diff(struct strbuf *sb, + const struct argv_array *ranges) +{ + struct child_process cp = CHILD_PROCESS_INIT; + + cp.git_cmd = 1; + argv_array_pushl(, "branch-diff", "--no-color", NULL); + argv_array_pushv(, ranges->argv); + return capture_command(, sb, 0); +} + static void emit_diffstat(struct rev_info *rev, struct commit *origin, struct commit *head) { @@ -1016,7 +1036,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, struct commit *origin, int nr, struct commit **list, const char *branch_name, - int quiet) + int quiet, + const char *range_diff) { const char *committer; const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n"; @@ -1028,15 +1049,25 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, int need_8bit_cte = 0; struct pretty_print_context pp = {0}; struct commit *head = list[0]; + struct strbuf diff = STRBUF_INIT; if (!cmit_fmt_is_mail(rev->commit_format)) die(_("Cover letter needs email format")); committer = git_committer_info(0); + /* 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(, range_diff, head); + if (get_range_diff(, )) + die(_("failed to generate range-diff")); + argv_array_clear(); + } + if (!use_stdout && open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet)) - return; + goto done; log_write_email_headers(rev, head, _subject, _8bit_cte); @@ -1077,6 +1108,17 @@ static void