{"thread":{"id":"48598","subject":"[RFC PATCH 0/5] format-patch: automate cover letter range-diff","startedAt":"2018-05-30T08:04:23Z","lastAt":"2018-07-27T15:18:48Z","messageCount":29,"participants":["Eric Sunshine","Ævar Arnfjörð Bjarmason","Stefan Beller","Duy Nguyen","Johannes Schindelin","Andrei Rybak"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"348786","messageId":"20180530080325.37520-1-sunshine@sunshineco.com","threadId":"48598","inReplyTo":null,"subject":"[RFC PATCH 0/5] format-patch: automate cover letter range-diff","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-30T08:03:20Z","receivedAt":"2018-05-30T08:04:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Dscho recently implemented a 'tbdiff' replacement as a Git builtin named\ngit-branch-diff[1] which computes differences between two versions of a\npatch series. Such a diff can be a useful aid for reviewers when\ninserted into a cover letter. However, doing so requires manual\ngeneration (invoking git-branch-diff) and copy/pasting the result into\nthe cover letter.\n\nThis series fully automates the process by adding a --range-diff option\nto git-format-patch. It is RFC for a couple reasons:\n\n* The final name for the 'tbdiff' replacement has not yet been nailed\n down. The name git-branch-diff is moribund[2]; Dscho favors merging\n the functionality into git-branch as a new --diff option[3]; others\n prefer a standalone command named git-range-diff or\n git-series-diff[4] or similar.\n\n* I have some ideas for future enhancements and want to be careful not\n to lock in a UI which doesn't mesh well with them (though I think the\n current UI turned out reasonably well). First, I foresee a\n complementary --interdiff option for inserting an interdiff-style diff\n for cases when that style is easier to read or simply more\n appropriate. Second, although the current patch series only supports\n --range-diff for the cover letter, some people insert interdiff- or\n tbdiff-style diffs (indented) into the commentary section of\n standalone patches. Although this often makes for a noisy mess, it is\n periodically useful.\n\nThis series is built atop js/branch-diff in 'pu'.\n\n[1]: https://public-inbox.org/git/cover.1525448066.git.johannes.schindelin@gmx.de/\n[2]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1805061401260.77@tvgsbejvaqbjf.bet/\n[3]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1805062155120.77@tvgsbejvaqbjf.bet/\n[4]: https://public-inbox.org/git/xmqqin7gzbkb.fsf@gitster-ct.c.googlers.com/\n\nEric Sunshine (5):\n format-patch: allow additional generated content in\n make_cover_letter()\n format-patch: add --range-diff option to embed diff in cover letter\n format-patch: extend --range-diff to accept revision range\n format-patch: teach --range-diff to respect -v/--reroll-count\n format-patch: add --creation-weight tweak for --range-diff\n\n Documentation/git-format-patch.txt | 18 +++++\n builtin/log.c | 119 ++++++++++++++++++++++++-----\n t/t7910-branch-diff.sh | 16 ++++\n 3 files changed, 132 insertions(+), 21 deletions(-)\n\n-- \n2.17.1.1235.ge295dfb56e\n\n"},{"id":"348787","messageId":"20180530080325.37520-3-sunshine@sunshineco.com","threadId":"48598","inReplyTo":"20180530080325.37520-1-sunshine@sunshineco.com","subject":"[RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-30T08:03:22Z","receivedAt":"2018-05-30T08:04:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When submitting a revised version of a patch series, it can be helpful\n(to reviewers) to include a summary of changes since the previous\nattempt in the form of an interdiff or range-diff, however, doing so\ninvolves manually copy/pasting the diff into the cover letter.\n\nAdd a --range-diff option to automate this process for range-diffs. The\nargument to --range-diff specifies the tip of the previous attempt\nagainst which to generate the range-diff. For example:\n\n git format-patch --cover-letter --range-diff=v1 -3 v2\n\n(At this early stage, the previous attempt and the patch series being\nformatted must share a common base, however, a subsequent enhancement\nwill make it possible to specify an explicit revision range for the\nprevious attempt.)\n\nSigned-off-by: Eric Sunshine \n---\n Documentation/git-format-patch.txt | 10 ++++++\n builtin/log.c | 55 ++++++++++++++++++++++++++++--\n t/t7910-branch-diff.sh | 15 ++++++++\n 3 files changed, 77 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 6cbe462a77..f4c70e6b64 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -23,6 +23,7 @@ SYNOPSIS\n \t\t [(--reroll-count|-v) ]\n \t\t [--to=] [--cc=]\n \t\t [--[no-]cover-letter] [--quiet] [--notes[=]]\n+\t\t [--range-diff=]\n \t\t [--progress]\n \t\t []\n \t\t [ | ]\n@@ -228,6 +229,15 @@ feeding the result to `git send-email`.\n \tcontaining the branch description, shortlog and the overall diffstat. You can\n \tfill in a description in the file before sending it out.\n \n+--range-diff=::\n+\tAs a reviewer aid, insert a range-diff (see linkgit:git-branch-diff[1])\n+\tinto the cover letter showing the differences between the previous\n+\tversion of the patch series and the series currently being formatted.\n+\t`previous` is a single revision naming the tip of the previous\n+\tseries which shares a common base with the series being formatted (for\n+\texample `git format-patch --cover-letter --range-diff=feature/v1 -3\n+\tfeature/v2`).\n+\n --notes[=]::\n \tAppend the notes (see linkgit:git-notes[1]) for the commit\n \tafter the three-dash line.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e01a256c11..460c31a293 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -28,6 +28,7 @@\n #include \"mailmap.h\"\n #include \"gpg-interface.h\"\n #include \"progress.h\"\n+#include \"run-command.h\"\n \n #define MAIL_DEFAULT_WRAP 72\n \n@@ -992,6 +993,25 @@ static char *find_branch_name(struct rev_info *rev)\n \treturn branch;\n }\n \n+static void infer_diff_ranges(struct argv_array *args,\n+\t\t\t const char *prev,\n+\t\t\t struct commit *head)\n+{\n+\targv_array_pushf(args, \"%s...%s\", prev,\n+\t\t\t oid_to_hex(&head->object.oid));\n+}\n+\n+static int get_range_diff(struct strbuf *sb,\n+\t\t\t const struct argv_array *ranges)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tcp.git_cmd = 1;\n+\targv_array_pushl(&cp.args, \"branch-diff\", \"--no-color\", NULL);\n+\targv_array_pushv(&cp.args, ranges->argv);\n+\treturn capture_command(&cp, sb, 0);\n+}\n+\n static void emit_diffstat(struct rev_info *rev,\n \t\t\t struct commit *origin, struct commit *head)\n {\n@@ -1016,7 +1036,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t\t struct commit *origin,\n \t\t\t int nr, struct commit **list,\n \t\t\t const char *branch_name,\n-\t\t\t int quiet)\n+\t\t\t int quiet,\n+\t\t\t const char *range_diff)\n {\n \tconst char *committer;\n \tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\";\n@@ -1028,15 +1049,25 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tint need_8bit_cte = 0;\n \tstruct pretty_print_context pp = {0};\n \tstruct commit *head = list[0];\n+\tstruct strbuf diff = STRBUF_INIT;\n \n \tif (!cmit_fmt_is_mail(rev->commit_format))\n \t\tdie(_(\"Cover letter needs email format\"));\n \n \tcommitter = git_committer_info(0);\n \n+\t/* might die from bad user input so try before creating cover letter */\n+\tif (range_diff) {\n+\t\tstruct argv_array ranges = ARGV_ARRAY_INIT;\n+\t\tinfer_diff_ranges(&ranges, range_diff, head);\n+\t\tif (get_range_diff(&diff, &ranges))\n+\t\t\tdie(_(\"failed to generate range-diff\"));\n+\t\targv_array_clear(&ranges);\n+\t}\n+\n \tif (!use_stdout &&\n \t open_next_file(NULL, rev->numbered_files ? NULL : \"cover-letter\", rev, quiet))\n-\t\treturn;\n+\t\tgoto done;\n \n \tlog_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte);\n \n@@ -1077,6 +1108,17 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t/* We can only do diffstat with a unique reference point */\n \tif (origin)\n \t\temit_diffstat(rev, origin, head);\n+\n+\tif (diff.len) {\n+\t\tFILE *fp = rev->diffopt.file;\n+\t\tfputs(_(\"Changes since previous version:\"), fp);\n+\t\tfputs(\"\\n\\n\", fp);\n+\t\tfputs(diff.buf, fp);\n+\t\tfputc('\\n', fp);\n+\t}\n+\n+done:\n+\tstrbuf_release(&diff);\n }\n \n static const char *clean_message_id(const char *msg_id)\n@@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstruct base_tree_info bases;\n \tint show_progress = 0;\n \tstruct progress *progress = NULL;\n+\tconst char *range_diff = NULL;\n \n \tconst struct option builtin_format_patch_options[] = {\n \t\t{ OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n@@ -1511,6 +1554,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n \t\tOPT_BOOL(0, \"progress\", &show_progress,\n \t\t\t N_(\"show progress while generating patches\")),\n+\t\tOPT_STRING(0, \"range-diff\", &range_diff, N_(\"rev-range\"),\n+\t\t\t N_(\"show changes against in cover letter\")),\n \t\tOPT_END()\n \t};\n \n@@ -1733,6 +1778,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (numbered)\n \t\trev.total = total + start_number - 1;\n \n+\tif (range_diff && !cover_letter)\n+\t\tdie(_(\"--range-diff requires --cover-letter\"));\n+\n \tif (!signature) {\n \t\t; /* --no-signature inhibits all signatures */\n \t} else if (signature && signature != git_version_string) {\n@@ -1764,7 +1812,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (thread)\n \t\t\tgen_message_id(&rev, \"cover\");\n \t\tmake_cover_letter(&rev, use_stdout,\n-\t\t\t\t origin, nr, list, branch_name, quiet);\n+\t\t\t\t origin, nr, list, branch_name, quiet,\n+\t\t\t\t range_diff);\n \t\tprint_bases(&bases, rev.diffopt.file);\n \t\tprint_signature(rev.diffopt.file);\n \t\ttotal++;\ndiff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh\nindex a7fece8804..edbd69b6f8 100755\n--- a/t/t7910-branch-diff.sh\n+++ b/t/t7910-branch-diff.sh\n@@ -141,4 +141,19 @@ test_expect_success 'changed message' '\n \ttest_cmp expected actual\n '\n \n+format_patch () {\n+\ttitle=$1 &&\n+\trange=$2 &&\n+\ttest_expect_success \"format-patch --range-diff ($title)\" '\n+\t\tgit format-patch --stdout --cover-letter --range-diff=$range \\\n+\t\t\tmaster..unmodified >actual &&\n+\t\tgrep \"= 1: .* s/5/A\" actual &&\n+\t\tgrep \"= 2: .* s/4/A\" actual &&\n+\t\tgrep \"= 3: .* s/11/B\" actual &&\n+\t\tgrep \"= 4: .* s/12/B\" actual\n+\t'\n+}\n+\n+format_patch 'B...C' topic\n+\n test_done\n-- \n2.17.1.1235.ge295dfb56e\n\n"},{"id":"348788","messageId":"20180530080325.37520-2-sunshine@sunshineco.com","threadId":"48598","inReplyTo":"20180530080325.37520-1-sunshine@sunshineco.com","subject":"[RFC PATCH 1/5] format-patch: allow additional generated content in make_cover_letter()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-30T08:03:21Z","receivedAt":"2018-05-30T08:04:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"make_cover_letter() returns early when it lacks sufficient state to emit\na diffstat, which makes it difficult to extend the function to reliably\nemit additional generated content. Work around this shortcoming by\nfactoring diffstat-printing logic out to its own function and calling it\nas needed without otherwise inhibiting normal control flow.\n\nSigned-off-by: Eric Sunshine \n---\n builtin/log.c | 43 +++++++++++++++++++++++--------------------\n 1 file changed, 23 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 71f68a3e4f..e01a256c11 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -992,6 +992,26 @@ static char *find_branch_name(struct rev_info *rev)\n \treturn branch;\n }\n \n+static void emit_diffstat(struct rev_info *rev,\n+\t\t\t struct commit *origin, struct commit *head)\n+{\n+\tstruct diff_options opts;\n+\n+\tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n+\topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n+\topts.stat_width = MAIL_DEFAULT_WRAP;\n+\n+\tdiff_setup_done(&opts);\n+\n+\tdiff_tree_oid(&origin->tree->object.oid,\n+\t\t &head->tree->object.oid,\n+\t\t \"\", &opts);\n+\tdiffcore_std(&opts);\n+\tdiff_flush(&opts);\n+\n+\tfprintf(rev->diffopt.file, \"\\n\");\n+}\n+\n static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t\t struct commit *origin,\n \t\t\t int nr, struct commit **list,\n@@ -1005,7 +1025,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tstruct strbuf sb = STRBUF_INIT;\n \tint i;\n \tconst char *encoding = \"UTF-8\";\n-\tstruct diff_options opts;\n \tint need_8bit_cte = 0;\n \tstruct pretty_print_context pp = {0};\n \tstruct commit *head = list[0];\n@@ -1055,25 +1074,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tshortlog_output(&log);\n \n-\t/*\n-\t * We can only do diffstat with a unique reference point\n-\t */\n-\tif (!origin)\n-\t\treturn;\n-\n-\tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n-\topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n-\topts.stat_width = MAIL_DEFAULT_WRAP;\n-\n-\tdiff_setup_done(&opts);\n-\n-\tdiff_tree_oid(&origin->tree->object.oid,\n-\t\t &head->tree->object.oid,\n-\t\t \"\", &opts);\n-\tdiffcore_std(&opts);\n-\tdiff_flush(&opts);\n-\n-\tfprintf(rev->diffopt.file, \"\\n\");\n+\t/* We can only do diffstat with a unique reference point */\n+\tif (origin)\n+\t\temit_diffstat(rev, origin, head);\n }\n \n static const char *clean_message_id(const char *msg_id)\n-- \n2.17.1.1235.ge295dfb56e\n\n"},{"id":"348789","messageId":"20180530080325.37520-4-sunshine@sunshineco.com","threadId":"48598","inReplyTo":"20180530080325.37520-1-sunshine@sunshineco.com","subject":"[RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-30T08:03:23Z","receivedAt":"2018-05-30T08:04:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When submitting a revised a patch series, the --range-diff option embeds\na range-diff in the cover letter showing changes since the previous\nversion of the patch series. The argument to --range-diff is a simple\nrevision naming the tip of the previous series, which works fine if the\nprevious and current versions of the patch series share a common base.\n\nHowever, it fails if the revision ranges of the old and new versions of\nthe series are disjoint. To address this shortcoming, extend\n--range-diff to also accept an explicit revision range for the previous\nseries. For example:\n\n git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2\n\nSigned-off-by: Eric Sunshine \n---\n Documentation/git-format-patch.txt | 8 +++++---\n builtin/log.c | 16 +++++++++++++---\n t/t7910-branch-diff.sh | 1 +\n 3 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex f4c70e6b64..25026ae26e 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -233,10 +233,12 @@ feeding the result to `git send-email`.\n \tAs a reviewer aid, insert a range-diff (see linkgit:git-branch-diff[1])\n \tinto the cover letter showing the differences between the previous\n \tversion of the patch series and the series currently being formatted.\n-\t`previous` is a single revision naming the tip of the previous\n-\tseries which shares a common base with the series being formatted (for\n+\t`previous` can be a single revision naming the tip of the previous\n+\tseries if it shares a common base with the series being formatted (for\n \texample `git format-patch --cover-letter --range-diff=feature/v1 -3\n-\tfeature/v2`).\n+\tfeature/v2`), or a revision range if the two versions of the series are\n+\tdisjoint (for example `git format-patch --cover-letter\n+\t--range-diff=feature/v1~3..feature/v1 -3 feature/v2`).\n \n --notes[=]::\n \tAppend the notes (see linkgit:git-notes[1]) for the commit\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 460c31a293..e38cf06050 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -995,10 +995,20 @@ static char *find_branch_name(struct rev_info *rev)\n \n static void infer_diff_ranges(struct argv_array *args,\n \t\t\t const char *prev,\n+\t\t\t struct commit *origin,\n \t\t\t struct commit *head)\n {\n-\targv_array_pushf(args, \"%s...%s\", prev,\n-\t\t\t oid_to_hex(&head->object.oid));\n+\tif (strstr(prev, \"..\")) {\n+\t\tif (!origin)\n+\t\t\tdie(_(\"failed to infer range-diff ranges\"));\n+\t\targv_array_push(args, prev);\n+\t\targv_array_pushf(args, \"%s..%s\",\n+\t\t\t\t oid_to_hex(&origin->object.oid),\n+\t\t\t\t oid_to_hex(&head->object.oid));\n+\t} else {\n+\t\targv_array_pushf(args, \"%s...%s\", prev,\n+\t\t\t\t oid_to_hex(&head->object.oid));\n+\t}\n }\n \n static int get_range_diff(struct strbuf *sb,\n@@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t/* might die from bad user input so try before creating cover letter */\n \tif (range_diff) {\n \t\tstruct argv_array ranges = ARGV_ARRAY_INIT;\n-\t\tinfer_diff_ranges(&ranges, range_diff, head);\n+\t\tinfer_diff_ranges(&ranges, range_diff, origin, head);\n \t\tif (get_range_diff(&diff, &ranges))\n \t\t\tdie(_(\"failed to generate range-diff\"));\n \t\targv_array_clear(&ranges);\ndiff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh\nindex edbd69b6f8..c0e8668ed9 100755\n--- a/t/t7910-branch-diff.sh\n+++ b/t/t7910-branch-diff.sh\n@@ -155,5 +155,6 @@ format_patch () {\n }\n \n format_patch 'B...C' topic\n+format_patch 'A..B A..C' master..topic\n \n test_done\n-- \n2.17.1.1235.ge295dfb56e\n\n"},{"id":"348790","messageId":"20180530080325.37520-5-sunshine@sunshineco.com","threadId":"48598","inReplyTo":"20180530080325.37520-1-sunshine@sunshineco.com","subject":"[RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-30T08:03:24Z","receivedAt":"2018-05-30T08:04:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The --range-diff option introduces the embedded range-diff generically\nas \"Changes since previous version:\", however, we can do better when\n--reroll-count is specified by emitting \"Changes since v{n}:\" instead.\n\nSigned-off-by: Eric Sunshine \n---\n builtin/log.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e38cf06050..3089d3a50a 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1121,7 +1121,11 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tif (diff.len) {\n \t\tFILE *fp = rev->diffopt.file;\n-\t\tfputs(_(\"Changes since previous version:\"), fp);\n+\t\tif (rev->reroll_count <= 0)\n+\t\t\tfputs(_(\"Changes since previous version:\"), fp);\n+\t\telse /* RFC may be v0, so allow -v1 to diff against v0 */\n+\t\t\tfprintf(fp, _(\"Changes since v%d:\"),\n+\t\t\t\trev->reroll_count - 1);\n \t\tfputs(\"\\n\\n\", fp);\n \t\tfputs(diff.buf, fp);\n \t\tfputc('\\n', fp);\n-- \n2.17.1.1235.ge295dfb56e\n\n"},{"id":"348791","messageId":"20180530080325.37520-6-sunshine@sunshineco.com","threadId":"48598","inReplyTo":"20180530080325.37520-1-sunshine@sunshineco.com","subject":"[RFC PATCH 5/5] format-patch: add --creation-weight tweak for --range-diff","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-30T08:03:25Z","receivedAt":"2018-05-30T08:04:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When generating a range-diff, matching up commits between two version of\na patch series involves heuristics, thus may give unexpected results.\ngit-branch-diff allows tweaking the heuristic via --creation-weight.\nFollow suit by accepting --creation-weight in combination with\n--range-diff when generating a range-diff for a cover-letter.\n\nSigned-off-by: Eric Sunshine \n---\n Documentation/git-format-patch.txt | 8 +++++++-\n builtin/log.c | 19 +++++++++++++++----\n 2 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 25026ae26e..7ed9ec9dae 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -23,7 +23,7 @@ SYNOPSIS\n \t\t [(--reroll-count|-v) ]\n \t\t [--to=] [--cc=]\n \t\t [--[no-]cover-letter] [--quiet] [--notes[=]]\n-\t\t [--range-diff=]\n+\t\t [--range-diff= [--creation-weight=]]\n \t\t [--progress]\n \t\t []\n \t\t [ | ]\n@@ -240,6 +240,12 @@ feeding the result to `git send-email`.\n \tdisjoint (for example `git format-patch --cover-letter\n \t--range-diff=feature/v1~3..feature/v1 -3 feature/v2`).\n \n+--creation-weight=::\n+\tUsed with `--range-diff`, tweak the heuristic which matches up commits\n+\tbetween the previous and current series of patches by adjusting the\n+\tcreation/deletion cost fudge factor. See linkgit:git-branch-diff[1])\n+\tfor details.\n+\n --notes[=]::\n \tAppend the notes (see linkgit:git-notes[1]) for the commit\n \tafter the three-dash line.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 3089d3a50a..2c49011b51 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1012,12 +1012,16 @@ static void infer_diff_ranges(struct argv_array *args,\n }\n \n static int get_range_diff(struct strbuf *sb,\n-\t\t\t const struct argv_array *ranges)\n+\t\t\t const struct argv_array *ranges,\n+\t\t\t const char *creation_weight)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \tcp.git_cmd = 1;\n \targv_array_pushl(&cp.args, \"branch-diff\", \"--no-color\", NULL);\n+\tif (creation_weight)\n+\t\targv_array_pushf(&cp.args,\n+\t\t\t\t \"--creation-weight=%s\", creation_weight);\n \targv_array_pushv(&cp.args, ranges->argv);\n \treturn capture_command(&cp, sb, 0);\n }\n@@ -1047,7 +1051,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t\t int nr, struct commit **list,\n \t\t\t const char *branch_name,\n \t\t\t int quiet,\n-\t\t\t const char *range_diff)\n+\t\t\t const char *range_diff,\n+\t\t\t const char *creation_weight)\n {\n \tconst char *committer;\n \tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\";\n@@ -1070,7 +1075,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tif (range_diff) {\n \t\tstruct argv_array ranges = ARGV_ARRAY_INIT;\n \t\tinfer_diff_ranges(&ranges, range_diff, origin, head);\n-\t\tif (get_range_diff(&diff, &ranges))\n+\t\tif (get_range_diff(&diff, &ranges, creation_weight))\n \t\t\tdie(_(\"failed to generate range-diff\"));\n \t\targv_array_clear(&ranges);\n \t}\n@@ -1495,6 +1500,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tint show_progress = 0;\n \tstruct progress *progress = NULL;\n \tconst char *range_diff = NULL;\n+\tconst char *creation_weight = NULL;\n \n \tconst struct option builtin_format_patch_options[] = {\n \t\t{ OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n@@ -1570,6 +1576,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"show progress while generating patches\")),\n \t\tOPT_STRING(0, \"range-diff\", &range_diff, N_(\"rev-range\"),\n \t\t\t N_(\"show changes against in cover letter\")),\n+\t\tOPT_STRING(0, \"creation-weight\", &creation_weight, N_(\"factor\"),\n+\t\t\t N_(\"fudge factor by which creation is weighted\")),\n \t\tOPT_END()\n \t};\n \n@@ -1664,6 +1672,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tdie (_(\"--subject-prefix/--rfc and -k are mutually exclusive.\"));\n \trev.preserve_subject = keep_subject;\n \n+\tif (creation_weight && !range_diff)\n+\t\tdie(_(\"--creation-weight requires --range-diff\"));\n+\n \targc = setup_revisions(argc, argv, &rev, &s_r_opt);\n \tif (argc > 1)\n \t\tdie (_(\"unrecognized argument: %s\"), argv[1]);\n@@ -1827,7 +1838,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\tgen_message_id(&rev, \"cover\");\n \t\tmake_cover_letter(&rev, use_stdout,\n \t\t\t\t origin, nr, list, branch_name, quiet,\n-\t\t\t\t range_diff);\n+\t\t\t\t range_diff, creation_weight);\n \t\tprint_bases(&bases, rev.diffopt.file);\n \t\tprint_signature(rev.diffopt.file);\n \t\ttotal++;\n-- \n2.17.1.1235.ge295dfb56e\n\n"},{"id":"348796","messageId":"878t81fqai.fsf@evledraar.gmail.com","threadId":"48598","inReplyTo":"20180530080325.37520-1-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-30T09:25:57Z","receivedAt":"2018-05-30T09:26:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 30 2018, Eric Sunshine wrote:\n\n> * The final name for the 'tbdiff' replacement has not yet been nailed\n> down. The name git-branch-diff is moribund[2]; Dscho favors merging\n> the functionality into git-branch as a new --diff option[3]; others\n> prefer a standalone command named git-range-diff or\n> git-series-diff[4] or similar.\n\nFWIW Dscho in an IRC conversation on May 25th seems to have settled on\ncalling it \"git range-diff \". He just hasn't gotten around to\nsubmitting a new version.\n"},{"id":"348830","messageId":"CAGZ79kZSjwsk-YE1G3sGX-sfdu17NjMo-Jbrw2E5AbpbNGSGyg@mail.gmail.com","threadId":"48598","inReplyTo":"20180530080325.37520-4-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-30T18:58:34Z","receivedAt":"2018-05-30T18:58:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine wrote:\n> When submitting a revised a patch series, the --range-diff option embeds\n> a range-diff in the cover letter showing changes since the previous\n> version of the patch series. The argument to --range-diff is a simple\n> revision naming the tip of the previous series, which works fine if the\n> previous and current versions of the patch series share a common base.\n>\n> However, it fails if the revision ranges of the old and new versions of\n> the series are disjoint. To address this shortcoming, extend\n> --range-diff to also accept an explicit revision range for the previous\n> series. For example:\n>\n> git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> Documentation/git-format-patch.txt | 8 +++++---\n> builtin/log.c | 16 +++++++++++++---\n> t/t7910-branch-diff.sh | 1 +\n> 3 files changed, 19 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n> index f4c70e6b64..25026ae26e 100644\n> --- a/Documentation/git-format-patch.txt\n> +++ b/Documentation/git-format-patch.txt\n> @@ -233,10 +233,12 @@ feeding the result to `git send-email`.\n> As a reviewer aid, insert a range-diff (see linkgit:git-branch-diff[1])\n> into the cover letter showing the differences between the previous\n> version of the patch series and the series currently being formatted.\n> - `previous` is a single revision naming the tip of the previous\n> - series which shares a common base with the series being formatted (for\n> + `previous` can be a single revision naming the tip of the previous\n> + series if it shares a common base with the series being formatted (for\n> example `git format-patch --cover-letter --range-diff=feature/v1 -3\n> - feature/v2`).\n> + feature/v2`), or a revision range if the two versions of the series are\n> + disjoint (for example `git format-patch --cover-letter\n> + --range-diff=feature/v1~3..feature/v1 -3 feature/v2`).\n>\n> --notes[=]::\n> Append the notes (see linkgit:git-notes[1]) for the commit\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 460c31a293..e38cf06050 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -995,10 +995,20 @@ static char *find_branch_name(struct rev_info *rev)\n>\n> static void infer_diff_ranges(struct argv_array *args,\n> const char *prev,\n> + struct commit *origin,\n> struct commit *head)\n> {\n> - argv_array_pushf(args, \"%s...%s\", prev,\n> - oid_to_hex(&head->object.oid));\n> + if (strstr(prev, \"..\")) {\n> + if (!origin)\n> + die(_(\"failed to infer range-diff ranges\"));\n\n\n> static int get_range_diff(struct strbuf *sb,\n> @@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> /* might die from bad user input so try before creating cover letter */\n> if (range_diff) {\n> struct argv_array ranges = ARGV_ARRAY_INIT;\n> - infer_diff_ranges(&ranges, range_diff, head);\n> + infer_diff_ranges(&ranges, range_diff, origin, head);\n\nThis is way before the check for 'if (origin) emit_diffstat' as done in\npatch 1, as we want to do this early. We need to check the non-NULLness\nin infer_diff_ranges as it is allowed to be NULL when the range-diff doesn't\ncontain \"..\", which means we assume the same branch point.\n\nWould it make sense to give a better error message in:\n\n> + if (!origin)\n> + die(_(\"failed to infer range-diff ranges\"));\n\nabove? (The failure sounds like it could be anything, but the\nreason for it is actually well understood: The origin was\ncomputed and as the boundary commit of the given range,\nor NULL if there is no boundary commit or multiple commits.\n\nIf we find not exactly one boundary, we cannot compute the\nrange to give to the range-diff tool.\n\nStepping back a bit:\n\n Consider the following DAG:\n\n O -\n \\\n A - B - C - D\n\na series from B..D (or '-2' when D is HEAD) would fail\nto generate both diffstat as well as the range diff as the\nmerge of O produces a second boundary, (but wouldn't\nthe creation of the patch C fail eventually anyway?)\n\nAnother DAG:\n\n A - B - C - D\n \\ /\n E\n\nwhen asking for B..D the diffstat and range diff would\ncompute ok, but the patch C would fail to generate again?\n\nStepping back to the error message, I have no good\nsuggestion on what to say there. Maybe we'd want to\nrefactor the code in cmd_format_patch, that computes the\norigin:\n\n if (prepare_revision_walk(&rev))\n die(_(\"revision walk setup failed\"));\n rev.boundary = 1;\n while ((commit = get_revision(&rev)) != NULL) {\n if (commit->object.flags & BOUNDARY) {\n boundary_count++;\n origin = (boundary_count == 1) ? commit : NULL;\n continue;\n }\n\n if (ignore_if_in_upstream && has_commit_patch_id(commit, &ids))\n continue;\n\n nr++;\n REALLOC_ARRAY(list, nr);\n list[nr - 1] = commit;\n }\n\nWe could prefix that with\n\n need_origin = (range_diff && strstr(prev, \"..\") || emit_diff_stat;\n\nand then if need_origin is about to be unset again we could issue\na warning and die early after the loop warning about bad DAG form?\n\nI guess that can wait for a follow up patch.\n\nThanks,\nStefan\n"},{"id":"348832","messageId":"CAGZ79kZ2MKpjwwx0+ZsPZ9bFu_ersPJ=kKPYWRmWYjJ8yoVhuA@mail.gmail.com","threadId":"48598","inReplyTo":"20180530080325.37520-5-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-30T19:03:16Z","receivedAt":"2018-05-30T19:03:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine wrote:\n> The --range-diff option introduces the embedded range-diff generically\n> as \"Changes since previous version:\", however, we can do better when\n> --reroll-count is specified by emitting \"Changes since v{n}:\" instead.\n\nA very similar option that I used before finding reroll count is\n--subject-prefix\nI still use that for RFC/WIP tags, but I sometimes used it with \"PATCHv2\"\nas an argument, too.\n\nWould we want to extend the niceties of this patch to that workflow?\n\nUnrelated to this patch: how does this series cope with range diffs\nthat are not in commit-ish but patches on the file system?\n"},{"id":"348837","messageId":"CAPig+cQEFQah4VM3keTE94Yj9aXp1oKhgLwsz6=m8jJMTTmecQ@mail.gmail.com","threadId":"48598","inReplyTo":"CAGZ79kZSjwsk-YE1G3sGX-sfdu17NjMo-Jbrw2E5AbpbNGSGyg@mail.gmail.com","subject":"Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-30T20:26:53Z","receivedAt":"2018-05-30T20:26:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 30, 2018 at 2:58 PM, Stefan Beller wrote:\n> On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine wrote:\n>> When submitting a revised a patch series, the --range-diff option embeds\n>> a range-diff in the cover letter showing changes since the previous\n>> version of the patch series. The argument to --range-diff is a simple\n>> revision naming the tip of the previous series, which works fine if the\n>> previous and current versions of the patch series share a common base.\n>>\n>> However, it fails if the revision ranges of the old and new versions of\n>> the series are disjoint. To address this shortcoming, extend\n>> --range-diff to also accept an explicit revision range for the previous\n>> series. For example:\n>>\n>> git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2\n>>\n>> Signed-off-by: Eric Sunshine \n>> ---\n>> static void infer_diff_ranges(struct argv_array *args,\n>> const char *prev,\n>> + struct commit *origin,\n>> struct commit *head)\n>> {\n>> - argv_array_pushf(args, \"%s...%s\", prev,\n>> - oid_to_hex(&head->object.oid));\n>> + if (strstr(prev, \"..\")) {\n>> + if (!origin)\n>> + die(_(\"failed to infer range-diff ranges\"));\n>\n>> static int get_range_diff(struct strbuf *sb,\n>> @@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n>> /* might die from bad user input so try before creating cover letter */\n>> if (range_diff) {\n>> struct argv_array ranges = ARGV_ARRAY_INIT;\n>> - infer_diff_ranges(&ranges, range_diff, head);\n>> + infer_diff_ranges(&ranges, range_diff, origin, head);\n>\n> This is way before the check for 'if (origin) emit_diffstat' as done in\n> patch 1, as we want to do this early. We need to check the non-NULLness\n> in infer_diff_ranges [...]\n\nCorrect.\n\n> Would it make sense to give a better error message in:\n>\n>> + if (!origin)\n>> + die(_(\"failed to infer range-diff ranges\"));\n>\n> above? (The failure sounds like it could be anything, but the\n> reason for it is actually well understood: The origin was\n> computed and as the boundary commit of the given range,\n> or NULL if there is no boundary commit or multiple commits.\n\nI'm not entirely happy with the generic error message either but\ndidn't come up with anything better which was both succinct and\nactually helpful. I was hoping that a reviewer might suggest something\nbetter.\n\n> If we find not exactly one boundary, we cannot compute the\n> range to give to the range-diff tool.\n\nI considered allowing the argument to --range-diff to be much more\ncomplex: to provide a way for the user to supply all information\nneeded for the invocation of git-range-diff (basically, to manually\nsupply arguments to git-range-diff) for cases when inference wasn't\npossible. I even went so far as to consider allowing the\n'creation-weight' to be specified as part of the --range-diff\nargument. However, that sort of complexity is both difficult to\nexplain (document) and tends to be painful for users to specify (type\ncorrectly).\n\nTherefore, in the end, I opted for simplicity: namely, handle the\ncommon cases with straight-forward minimal-learning-curve standard\noptions. Both --range-diff=v3 and --range-diff=v3~4..v3 are easy to\ndocument and understand, as is the distinct --creation-weight= option.\nThis seems a good balance between extreme flexibility and relative\nsimplicity for handling common needs. (And, --range-diff is just a\nconvenience, after all; more complex scenarios can still be handled\nmanually by invoking git-range-diff followed by copy/paste.)\n\n> Stepping back to the error message, I have no good\n> suggestion on what to say there. Maybe we'd want to\n> refactor the code in cmd_format_patch, that computes the\n> origin:\n>\n> if (prepare_revision_walk(&rev))\n> die(_(\"revision walk setup failed\"));\n> rev.boundary = 1;\n> while ((commit = get_revision(&rev)) != NULL) {\n> if (commit->object.flags & BOUNDARY) {\n> boundary_count++;\n> origin = (boundary_count == 1) ? commit : NULL;\n> continue;\n> }\n>\n> We could prefix that with\n>\n> need_origin = (range_diff && strstr(prev, \"..\") || emit_diff_stat;\n>\n> and then if need_origin is about to be unset again we could issue\n> a warning and die early after the loop warning about bad DAG form?\n>\n> I guess that can wait for a follow up patch.\n\nI agree. The feature can be improved and made more fancy incrementally\nas we gain better understanding of areas which need improvement. As a\nfirst step, I wanted to keep it minimal but usable for the most common\ncases.\n\nThanks for the review.\n"},{"id":"348838","messageId":"CAPig+cRh02976beGp5c5Sw5=h86VgNZgVreCHh38QKP5udJeGg@mail.gmail.com","threadId":"48598","inReplyTo":"CAGZ79kZ2MKpjwwx0+ZsPZ9bFu_ersPJ=kKPYWRmWYjJ8yoVhuA@mail.gmail.com","subject":"Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-30T20:44:54Z","receivedAt":"2018-05-30T20:44:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 30, 2018 at 3:03 PM, Stefan Beller wrote:\n> On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine wrote:\n>> The --range-diff option introduces the embedded range-diff generically\n>> as \"Changes since previous version:\", however, we can do better when\n>> --reroll-count is specified by emitting \"Changes since v{n}:\" instead.\n>\n> A very similar option that I used before finding reroll count is\n> --subject-prefix\n> I still use that for RFC/WIP tags, but I sometimes used it with \"PATCHv2\"\n> as an argument, too.\n>\n> Would we want to extend the niceties of this patch to that workflow?\n\nI would not include such functionality directly in this patch, as the\ntwo ideas are only superficially related (\"computing the previous\nversion number by *some* mechanism\") but not related in actual\nimplementation.\n\nComputing the previous version number by consulting --reroll-count, as\ndone by this patch, is deterministic and was just low-hanging fruit.\nWhat you suggest probably involves heuristics and parsing, thus ought\nto be done in its own patch (or patches). It's also the sort of\nincremental improvement that can be done later (rather than in this\ninitial implementation) if someone deems it desirable.\n\nBTW: You can use \"git format-patch --rfc\" for RFC patches (in fact, I\ndid so for this series).\n\n> Unrelated to this patch: how does this series cope with range diffs\n> that are not in commit-ish but patches on the file system?\n\nI'm not following. Can you provide a concrete example to get me up to speed?\n"},{"id":"348841","messageId":"CAGZ79ka1-emwYsEh1EQdh=pgFUV_bi2xoDu1Ajq=Z1XkkhyuDw@mail.gmail.com","threadId":"48598","inReplyTo":"CAPig+cRh02976beGp5c5Sw5=h86VgNZgVreCHh38QKP5udJeGg@mail.gmail.com","subject":"Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-30T21:03:36Z","receivedAt":"2018-05-30T21:03:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 30, 2018 at 1:44 PM, Eric Sunshine wrote:\n\n>> Unrelated to this patch: how does this series cope with range diffs\n>> that are not in commit-ish but patches on the file system?\n>\n> I'm not following. Can you provide a concrete example to get me up to speed?\n\nI was just wondering if things like\n\n git format-patch --range-diff=v3-00*.patch --reroll-count=4 -3\n\nwould be supported, but that doesn't seem to be the case, now that I read\nthe whole series. I don't think it is crucial to support right now.\n\nThis whole series is reviewed by me and I think it is good for inclusion\nonce we have a reroll of the range-diff series and aligned the command\nname to call.\n\nThanks,\nStefan\n"},{"id":"348847","messageId":"CAPig+cS+X8une=g0sSVc4dnZCsnFYe1D=v1iO+zs1ChGuQAJYg@mail.gmail.com","threadId":"48598","inReplyTo":"CAGZ79ka1-emwYsEh1EQdh=pgFUV_bi2xoDu1Ajq=Z1XkkhyuDw@mail.gmail.com","subject":"Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-30T21:14:11Z","receivedAt":"2018-05-30T21:14:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 30, 2018 at 5:03 PM, Stefan Beller wrote:\n> On Wed, May 30, 2018 at 1:44 PM, Eric Sunshine wrote:\n>>> Unrelated to this patch: how does this series cope with range diffs\n>>> that are not in commit-ish but patches on the file system?\n>>\n>> I'm not following. Can you provide a concrete example to get me up to speed?\n>\n> I was just wondering if things like\n>\n> git format-patch --range-diff=v3-00*.patch --reroll-count=4 -3\n>\n> would be supported, but that doesn't seem to be the case, now that I read\n> the whole series. I don't think it is crucial to support right now.\n\nOkay, that's what I thought you meant (upon thinking harder about it\nafter hitting Send). git-range-diff doesn't support that mode of\noperation (nor does 'tbdiff', as far as I can tell), so as a thin\nwrapper around git-range-diff, \"git format-patch --range-diff\" doesn't\nsupport it either.\n\nPerhaps that sort of functionality could implement later by someone,\nif desired. In the meantime, the following (manual procedure) would\napproximate it:\n\n git checkout --detach \n git am v3-00*.patch\n git format-patch --range-diff=...\n\n> This whole series is reviewed by me and I think it is good for inclusion\n> once we have a reroll of the range-diff series and aligned the command\n> name to call.\n\nThanks.\n"},{"id":"349543","messageId":"CACsJy8D=6fAEpO5m4cc7KZyggAW1AosSkUWaunQBFH0nr-YrdA@mail.gmail.com","threadId":"48598","inReplyTo":"20180530080325.37520-1-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-06T19:16:55Z","receivedAt":"2018-06-06T19:17:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine wrote:\n> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named\n> git-branch-diff[1] which computes differences between two versions of a\n> patch series. Such a diff can be a useful aid for reviewers when\n> inserted into a cover letter. However, doing so requires manual\n> generation (invoking git-branch-diff) and copy/pasting the result into\n> the cover letter.\n\nAnother option which I wanted to go is delegate part of cover letter\ngeneration to a hook (or just a config key that contains a shell\ncommand). This way it's easier to customize cover letters. We could\nstill have a good fallback that does shortlog, diffstat and tbdiff.\n-- \nDuy\n"},{"id":"349604","messageId":"CAPig+cS1M3U0T7yd2hKv4Uv1NMub20bTy13-D-6drpJLJjNNAA@mail.gmail.com","threadId":"48598","inReplyTo":"CACsJy8D=6fAEpO5m4cc7KZyggAW1AosSkUWaunQBFH0nr-YrdA@mail.gmail.com","subject":"Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-07T08:34:32Z","receivedAt":"2018-06-07T08:34:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 6, 2018 at 3:16 PM, Duy Nguyen wrote:\n> On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine wrote:\n>> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named\n>> git-branch-diff[1] which computes differences between two versions of a\n>> patch series. Such a diff can be a useful aid for reviewers when\n>> inserted into a cover letter. However, doing so requires manual\n>> generation (invoking git-branch-diff) and copy/pasting the result into\n>> the cover letter.\n>\n> Another option which I wanted to go is delegate part of cover letter\n> generation to a hook (or just a config key that contains a shell\n> command). This way it's easier to customize cover letters. We could\n> still have a good fallback that does shortlog, diffstat and tbdiff.\n\nIt is common on this mailing list to turn down requests for new hooks\nwhen the requested functionality could just as easily be implemented\nvia a wrapper script. So, my knee-jerk reaction is that a hook to\ncustomize the cover letter may be overkill when the same functionality\ncould likely be implemented relatively easily by a shell script which\ninvokes git-format-patch and customizes the cover letter\nafter-the-fact. Same argument regarding a config key holding a shell\ncommand. But, perhaps there are cases which don't occur to me which\ncould be helped by a config variable or such.\n\nOf course, by the same reasoning, the --range-diff functionality\nimplemented by this patch series, which is just a convenience, could\nbe handled by a wrapper script, thus is not strictly needed. On the\nother hand, given that interdiffs and range-diffs are so regularly\nused in re-rolls on this list (and perhaps other mailing list-based\nprojects) may be argument enough in favor of having such an option\nbuilt into git-format-patch.\n"},{"id":"349643","messageId":"CACsJy8D696DiWLGJgfqECBDJ9rMbpOy5bCe_knTttmR82QPCJA@mail.gmail.com","threadId":"48598","inReplyTo":"CAPig+cS1M3U0T7yd2hKv4Uv1NMub20bTy13-D-6drpJLJjNNAA@mail.gmail.com","subject":"Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-07T15:09:05Z","receivedAt":"2018-06-07T15:09:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 7, 2018 at 10:34 AM, Eric Sunshine wrote:\n> On Wed, Jun 6, 2018 at 3:16 PM, Duy Nguyen wrote:\n>> On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine wrote:\n>>> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named\n>>> git-branch-diff[1] which computes differences between two versions of a\n>>> patch series. Such a diff can be a useful aid for reviewers when\n>>> inserted into a cover letter. However, doing so requires manual\n>>> generation (invoking git-branch-diff) and copy/pasting the result into\n>>> the cover letter.\n>>\n>> Another option which I wanted to go is delegate part of cover letter\n>> generation to a hook (or just a config key that contains a shell\n>> command). This way it's easier to customize cover letters. We could\n>> still have a good fallback that does shortlog, diffstat and tbdiff.\n>\n> It is common on this mailing list to turn down requests for new hooks\n> when the requested functionality could just as easily be implemented\n> via a wrapper script. So, my knee-jerk reaction is that a hook to\n> customize the cover letter may be overkill when the same functionality\n> could likely be implemented relatively easily by a shell script which\n> invokes git-format-patch and customizes the cover letter\n> after-the-fact. Same argument regarding a config key holding a shell\n> command. But, perhaps there are cases which don't occur to me which\n> could be helped by a config variable or such.\n\nI think format-patch --cover-letter nowadays does more stuff that's\nnot so easy to simply rewrite it in a shell script. My original\nproblem with format-patch is it hard codes shortlog settings and you\ncan't list patches with patch number (e.g. \"[1/2] foo bar\"). The\nsimplest way is let format-patch does it stuff as usual and\n\"outsource\" some cover letter's body generation to a script.\n\nBut it's ok. I could try to code the patch numbering thing in\nformat-patch and maybe submit a patch or two for that later.\n\n> Of course, by the same reasoning, the --range-diff functionality\n> implemented by this patch series, which is just a convenience, could\n> be handled by a wrapper script, thus is not strictly needed. On the\n> other hand, given that interdiffs and range-diffs are so regularly\n> used in re-rolls on this list (and perhaps other mailing list-based\n> projects) may be argument enough in favor of having such an option\n> built into git-format-patch.\n-- \nDuy\n"},{"id":"352757","messageId":"nycvar.QRO.7.76.6.1807171204080.71@tvgsbejvaqbjf.bet","threadId":"48598","inReplyTo":"20180530080325.37520-1-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-17T10:04:58Z","receivedAt":"2018-07-17T10:05:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 30 May 2018, Eric Sunshine wrote:\n\n> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named\n> git-branch-diff[1] which computes differences between two versions of a\n> patch series. Such a diff can be a useful aid for reviewers when\n> inserted into a cover letter. However, doing so requires manual\n> generation (invoking git-branch-diff) and copy/pasting the result into\n> the cover letter.\n> \n> This series fully automates the process by adding a --range-diff option\n> to git-format-patch.\n\nNice!\n\n> It is RFC for a couple reasons:\n> \n> * The final name for the 'tbdiff' replacement has not yet been nailed\n> down. The name git-branch-diff is moribund[2]; Dscho favors merging\n> the functionality into git-branch as a new --diff option[3]; others\n> prefer a standalone command named git-range-diff or\n> git-series-diff[4] or similar.\n\nI think this has been settled now: range-diff it is.\n\n> * I have some ideas for future enhancements and want to be careful not\n> to lock in a UI which doesn't mesh well with them (though I think the\n> current UI turned out reasonably well). First, I foresee a\n> complementary --interdiff option for inserting an interdiff-style diff\n> for cases when that style is easier to read or simply more\n> appropriate. Second, although the current patch series only supports\n> --range-diff for the cover letter, some people insert interdiff- or\n> tbdiff-style diffs (indented) into the commentary section of\n> standalone patches. Although this often makes for a noisy mess, it is\n> periodically useful.\n\nSure.\n\n> This series is built atop js/branch-diff in 'pu'.\n> \n> [1]: https://public-inbox.org/git/cover.1525448066.git.johannes.schindelin@gmx.de/\n> [2]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1805061401260.77@tvgsbejvaqbjf.bet/\n> [3]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1805062155120.77@tvgsbejvaqbjf.bet/\n> [4]: https://public-inbox.org/git/xmqqin7gzbkb.fsf@gitster-ct.c.googlers.com/\n> \n> Eric Sunshine (5):\n> format-patch: allow additional generated content in\n> make_cover_letter()\n> format-patch: add --range-diff option to embed diff in cover letter\n> format-patch: extend --range-diff to accept revision range\n> format-patch: teach --range-diff to respect -v/--reroll-count\n> format-patch: add --creation-weight tweak for --range-diff\n> \n> Documentation/git-format-patch.txt | 18 +++++\n> builtin/log.c | 119 ++++++++++++++++++++++++-----\n> t/t7910-branch-diff.sh | 16 ++++\n> 3 files changed, 132 insertions(+), 21 deletions(-)\n> \n> -- \n> 2.17.1.1235.ge295dfb56e\n\nThanks!\nDscho\n"},{"id":"352759","messageId":"nycvar.QRO.7.76.6.1807171214590.71@tvgsbejvaqbjf.bet","threadId":"48598","inReplyTo":"20180530080325.37520-2-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 1/5] format-patch: allow additional generated content in make_cover_letter()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-17T10:15:14Z","receivedAt":"2018-07-17T10:15:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 30 May 2018, Eric Sunshine wrote:\n\n> make_cover_letter() returns early when it lacks sufficient state to emit\n> a diffstat, which makes it difficult to extend the function to reliably\n> emit additional generated content. Work around this shortcoming by\n> factoring diffstat-printing logic out to its own function and calling it\n> as needed without otherwise inhibiting normal control flow.\n> \n> Signed-off-by: Eric Sunshine \n\nMakes sense.\n\nCiao,\nDscho\n\n> ---\n> builtin/log.c | 43 +++++++++++++++++++++++--------------------\n> 1 file changed, 23 insertions(+), 20 deletions(-)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index 71f68a3e4f..e01a256c11 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -992,6 +992,26 @@ static char *find_branch_name(struct rev_info *rev)\n> \treturn branch;\n> }\n> \n> +static void emit_diffstat(struct rev_info *rev,\n> +\t\t\t struct commit *origin, struct commit *head)\n> +{\n> +\tstruct diff_options opts;\n> +\n> +\tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n> +\topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> +\topts.stat_width = MAIL_DEFAULT_WRAP;\n> +\n> +\tdiff_setup_done(&opts);\n> +\n> +\tdiff_tree_oid(&origin->tree->object.oid,\n> +\t\t &head->tree->object.oid,\n> +\t\t \"\", &opts);\n> +\tdiffcore_std(&opts);\n> +\tdiff_flush(&opts);\n> +\n> +\tfprintf(rev->diffopt.file, \"\\n\");\n> +}\n> +\n> static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \t\t\t struct commit *origin,\n> \t\t\t int nr, struct commit **list,\n> @@ -1005,7 +1025,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tint i;\n> \tconst char *encoding = \"UTF-8\";\n> -\tstruct diff_options opts;\n> \tint need_8bit_cte = 0;\n> \tstruct pretty_print_context pp = {0};\n> \tstruct commit *head = list[0];\n> @@ -1055,25 +1074,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \n> \tshortlog_output(&log);\n> \n> -\t/*\n> -\t * We can only do diffstat with a unique reference point\n> -\t */\n> -\tif (!origin)\n> -\t\treturn;\n> -\n> -\tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n> -\topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> -\topts.stat_width = MAIL_DEFAULT_WRAP;\n> -\n> -\tdiff_setup_done(&opts);\n> -\n> -\tdiff_tree_oid(&origin->tree->object.oid,\n> -\t\t &head->tree->object.oid,\n> -\t\t \"\", &opts);\n> -\tdiffcore_std(&opts);\n> -\tdiff_flush(&opts);\n> -\n> -\tfprintf(rev->diffopt.file, \"\\n\");\n> +\t/* We can only do diffstat with a unique reference point */\n> +\tif (origin)\n> +\t\temit_diffstat(rev, origin, head);\n> }\n> \n> static const char *clean_message_id(const char *msg_id)\n> -- \n> 2.17.1.1235.ge295dfb56e\n> \n> \n"},{"id":"352760","messageId":"CAPig+cSXvo31etKLaAKjt0TSuCE+5S6gS69S3viVM2cMzy3iAw@mail.gmail.com","threadId":"48598","inReplyTo":"nycvar.QRO.7.76.6.1807171214590.71@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 1/5] format-patch: allow additional generated content in make_cover_letter()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-17T10:24:42Z","receivedAt":"2018-07-17T10:24:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 17, 2018 at 6:15 AM Johannes Schindelin\n wrote:\n> On Wed, 30 May 2018, Eric Sunshine wrote:\n> > make_cover_letter() returns early when it lacks sufficient state to emit\n> > a diffstat, which makes it difficult to extend the function to reliably\n> > emit additional generated content. Work around this shortcoming by\n> > factoring diffstat-printing logic out to its own function and calling it\n> > as needed without otherwise inhibiting normal control flow.\n> >\n> > Signed-off-by: Eric Sunshine \n>\n> Makes sense.\n\nThanks, but it's probably not worth spending time reviewing this RFC\nseries. I already have a new series in the works (in fact, mostly\nfinished) in which the implementation is drastically changed from this\none. Aside from adding an --interdiff option to git-format-patch (in\naddition to a --range-diff option) and allowing interdiff and\nrange-diff to be added as commentary to a single-patch, the new series\nalso takes advantage of the newly-libified range-diff engine rather\nthan running git-range-diff as a command. So, most or all of the code\nhas changed.\n\n(Though, perhaps it wouldn't hurt to review the documentation changes\nin this RFC series to see if I botched how I described the option.)\n"},{"id":"352761","messageId":"nycvar.QRO.7.76.6.1807171219480.71@tvgsbejvaqbjf.bet","threadId":"48598","inReplyTo":"20180530080325.37520-3-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-17T10:30:34Z","receivedAt":"2018-07-17T10:30:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 30 May 2018, Eric Sunshine wrote:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index e01a256c11..460c31a293 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -28,6 +28,7 @@\n> #include \"mailmap.h\"\n> #include \"gpg-interface.h\"\n> #include \"progress.h\"\n> +#include \"run-command.h\"\n> \n> #define MAIL_DEFAULT_WRAP 72\n> \n> @@ -992,6 +993,25 @@ static char *find_branch_name(struct rev_info *rev)\n> \treturn branch;\n> }\n> \n> +static void infer_diff_ranges(struct argv_array *args,\n> +\t\t\t const char *prev,\n> +\t\t\t struct commit *head)\n> +{\n> +\targv_array_pushf(args, \"%s...%s\", prev,\n> +\t\t\t oid_to_hex(&head->object.oid));\n> +}\n> +\n> +static int get_range_diff(struct strbuf *sb,\n\nIf you rename `sb` to `out`, it makes it more obvious to the casual reader\nthat this strbuf will receive the output.\n\n> +\t\t\t const struct argv_array *ranges)\n> +{\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\n> +\tcp.git_cmd = 1;\n> +\targv_array_pushl(&cp.args, \"branch-diff\", \"--no-color\", NULL);\n\nObviously, this needs to be \"range-diff\" now.\n\n> +\targv_array_pushv(&cp.args, ranges->argv);\n\nAs there must be exactly two ranges, it would make more sense to pass them\nexplicitly. And then you can use one single call to `argv_array_pushl()`\ninstead.\n\n> +\treturn capture_command(&cp, sb, 0);\n> +}\n> +\n> static void emit_diffstat(struct rev_info *rev,\n> \t\t\t struct commit *origin, struct commit *head)\n> {\n> @@ -1016,7 +1036,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \t\t\t struct commit *origin,\n> \t\t\t int nr, struct commit **list,\n> \t\t\t const char *branch_name,\n> -\t\t\t int quiet)\n> +\t\t\t int quiet,\n> +\t\t\t const char *range_diff)\n> {\n> \tconst char *committer;\n> \tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\";\n> @@ -1028,15 +1049,25 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \tint need_8bit_cte = 0;\n> \tstruct pretty_print_context pp = {0};\n> \tstruct commit *head = list[0];\n> +\tstruct strbuf diff = STRBUF_INIT;\n\nI guess you want to call it `diff` instead of `range_diff` because a\nfuture change will reuse this for the interdiff instead? And then also to\navoid a naming conflict with the parameter.\n\nDunno. I would still call it `range_diff` until the day comes (if ever)\nwhen `--interdiff` is implemented. And I would call the `range_diff`\nparameter `range_diff_opt` instead, or some such.\n\nOr maybe use `extra_footer` instead of `diff`.\n\n> \tif (!cmit_fmt_is_mail(rev->commit_format))\n> \t\tdie(_(\"Cover letter needs email format\"));\n> \n> \tcommitter = git_committer_info(0);\n> \n> +\t/* might die from bad user input so try before creating cover letter */\n> +\tif (range_diff) {\n> +\t\tstruct argv_array ranges = ARGV_ARRAY_INIT;\n> +\t\tinfer_diff_ranges(&ranges, range_diff, head);\n> +\t\tif (get_range_diff(&diff, &ranges))\n> +\t\t\tdie(_(\"failed to generate range-diff\"));\n\nBTW I like to have an extra space in front of all the range-diff lines, to\nmake it easier to discern them from the rest.\n\n> +\t\targv_array_clear(&ranges);\n> +\t}\n> +\n> \tif (!use_stdout &&\n> \t open_next_file(NULL, rev->numbered_files ? NULL : \"cover-letter\", rev, quiet))\n> -\t\treturn;\n> +\t\tgoto done;\n\nHmm. If you think you will add more `goto done`s in the future, I guess\nthis is okay. Otherwise, it would probably make sense to go ahead and\nsimply `strbuf_release(&diff)` before `return`ing.\n\n> \tlog_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte);\n> \n> @@ -1077,6 +1108,17 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \t/* We can only do diffstat with a unique reference point */\n> \tif (origin)\n> \t\temit_diffstat(rev, origin, head);\n> +\n> +\tif (diff.len) {\n> +\t\tFILE *fp = rev->diffopt.file;\n> +\t\tfputs(_(\"Changes since previous version:\"), fp);\n> +\t\tfputs(\"\\n\\n\", fp);\n> +\t\tfputs(diff.buf, fp);\n> +\t\tfputc('\\n', fp);\n> +\t}\n> +\n> +done:\n> +\tstrbuf_release(&diff);\n> }\n> \n> static const char *clean_message_id(const char *msg_id)\n> @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \tstruct base_tree_info bases;\n> \tint show_progress = 0;\n> \tstruct progress *progress = NULL;\n> +\tconst char *range_diff = NULL;\n\nMaybe `range_diff_opt`? It's not exactly the range diff that is contained\nin this variable.\n\n> \tconst struct option builtin_format_patch_options[] = {\n> \t\t{ OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n> @@ -1511,6 +1554,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n> \t\tOPT_BOOL(0, \"progress\", &show_progress,\n> \t\t\t N_(\"show progress while generating patches\")),\n> +\t\tOPT_STRING(0, \"range-diff\", &range_diff, N_(\"rev-range\"),\n> +\t\t\t N_(\"show changes against in cover letter\")),\n> \t\tOPT_END()\n> \t};\n> \n> @@ -1733,6 +1778,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \tif (numbered)\n> \t\trev.total = total + start_number - 1;\n> \n> +\tif (range_diff && !cover_letter)\n> +\t\tdie(_(\"--range-diff requires --cover-letter\"));\n\nI guess this will be changed in a future patch, allowing a single patch to\nship with a range diff, too?\n\n> +\n> \tif (!signature) {\n> \t\t; /* --no-signature inhibits all signatures */\n> \t} else if (signature && signature != git_version_string) {\n> @@ -1764,7 +1812,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\tif (thread)\n> \t\t\tgen_message_id(&rev, \"cover\");\n> \t\tmake_cover_letter(&rev, use_stdout,\n> -\t\t\t\t origin, nr, list, branch_name, quiet);\n> +\t\t\t\t origin, nr, list, branch_name, quiet,\n> +\t\t\t\t range_diff);\n> \t\tprint_bases(&bases, rev.diffopt.file);\n> \t\tprint_signature(rev.diffopt.file);\n> \t\ttotal++;\n> diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh\n> index a7fece8804..edbd69b6f8 100755\n> --- a/t/t7910-branch-diff.sh\n> +++ b/t/t7910-branch-diff.sh\n> @@ -141,4 +141,19 @@ test_expect_success 'changed message' '\n> \ttest_cmp expected actual\n> '\n> \n> +format_patch () {\n> +\ttitle=$1 &&\n> +\trange=$2 &&\n> +\ttest_expect_success \"format-patch --range-diff ($title)\" '\n> +\t\tgit format-patch --stdout --cover-letter --range-diff=$range \\\n> +\t\t\tmaster..unmodified >actual &&\n> +\t\tgrep \"= 1: .* s/5/A\" actual &&\n> +\t\tgrep \"= 2: .* s/4/A\" actual &&\n> +\t\tgrep \"= 3: .* s/11/B\" actual &&\n> +\t\tgrep \"= 4: .* s/12/B\" actual\n\nI guess this might make sense if `format_patch` was not a function, but it\nis specifically marked as a function... so... shouldn't these `grep`s also\nbe using function parameters?\n\n> +\t'\n> +}\n> +\n> +format_patch 'B...C' topic\n> +\n> test_done\n> -- \n> 2.17.1.1235.ge295dfb56e\n\nThanks,\nDscho\n"},{"id":"352762","messageId":"nycvar.QRO.7.76.6.1807171243410.71@tvgsbejvaqbjf.bet","threadId":"48598","inReplyTo":"20180530080325.37520-4-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-17T10:44:44Z","receivedAt":"2018-07-17T10:45:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 30 May 2018, Eric Sunshine wrote:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 460c31a293..e38cf06050 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -995,10 +995,20 @@ static char *find_branch_name(struct rev_info *rev)\n> \n> static void infer_diff_ranges(struct argv_array *args,\n> \t\t\t const char *prev,\n> +\t\t\t struct commit *origin,\n> \t\t\t struct commit *head)\n> {\n> -\targv_array_pushf(args, \"%s...%s\", prev,\n> -\t\t\t oid_to_hex(&head->object.oid));\n> +\tif (strstr(prev, \"..\")) {\n> +\t\tif (!origin)\n> +\t\t\tdie(_(\"failed to infer range-diff ranges\"));\n> +\t\targv_array_push(args, prev);\n> +\t\targv_array_pushf(args, \"%s..%s\",\n> +\t\t\t\t oid_to_hex(&origin->object.oid),\n> +\t\t\t\t oid_to_hex(&head->object.oid));\n> +\t} else {\n> +\t\targv_array_pushf(args, \"%s...%s\", prev,\n> +\t\t\t\t oid_to_hex(&head->object.oid));\n> +\t}\n\nThis would be easier to read if the order was inverted:\n\n\tif (!strstr(...))\n\t\t...\n\telse if (!origin)\n\t\tdie(...)\n\telse {\n\t\t...\n\t}\n\nOtherwise, it makes sense to me.\n\nThanks,\nDscho\n\n> }\n> \n> static int get_range_diff(struct strbuf *sb,\n> @@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \t/* might die from bad user input so try before creating cover letter */\n> \tif (range_diff) {\n> \t\tstruct argv_array ranges = ARGV_ARRAY_INIT;\n> -\t\tinfer_diff_ranges(&ranges, range_diff, head);\n> +\t\tinfer_diff_ranges(&ranges, range_diff, origin, head);\n> \t\tif (get_range_diff(&diff, &ranges))\n> \t\t\tdie(_(\"failed to generate range-diff\"));\n> \t\targv_array_clear(&ranges);\n> diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh\n> index edbd69b6f8..c0e8668ed9 100755\n> --- a/t/t7910-branch-diff.sh\n> +++ b/t/t7910-branch-diff.sh\n> @@ -155,5 +155,6 @@ format_patch () {\n> }\n> \n> format_patch 'B...C' topic\n> +format_patch 'A..B A..C' master..topic\n> \n> test_done\n> -- \n> 2.17.1.1235.ge295dfb56e\n> \n> \n"},{"id":"352763","messageId":"CAPig+cTKGd8N78XvW-rmBEZC7ykcJsE+na1V_vCVXTUhGrFe4Q@mail.gmail.com","threadId":"48598","inReplyTo":"nycvar.QRO.7.76.6.1807171219480.71@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-17T10:49:13Z","receivedAt":"2018-07-17T10:49:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the review comments. In the new version of the series\n(almost complete), almost the entire implementation has changed,\nincluding most of the stuff on which you commented. Anyhow, see my\nresponses to your comments below...\n\nOn Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin\n wrote:\n> On Wed, 30 May 2018, Eric Sunshine wrote:\n> > +static int get_range_diff(struct strbuf *sb,\n>\n> If you rename `sb` to `out`, it makes it more obvious to the casual reader\n> that this strbuf will receive the output.\n\nThis is gone in the re-roll.\n\n> > + cp.git_cmd = 1;\n> > + argv_array_pushl(&cp.args, \"branch-diff\", \"--no-color\", NULL);\n>\n> Obviously, this needs to be \"range-diff\" now.\n\nThe re-roll takes advantage of the libified range-diff rather than\ninvoking it as a command.\n\n> > + argv_array_pushv(&cp.args, ranges->argv);\n>\n> As there must be exactly two ranges, it would make more sense to pass them\n> explicitly. And then you can use one single call to `argv_array_pushl()`\n> instead.\n\nGone in the re-roll.\n\n> > + struct strbuf diff = STRBUF_INIT;\n>\n> I guess you want to call it `diff` instead of `range_diff` because a\n> future change will reuse this for the interdiff instead? And then also to\n> avoid a naming conflict with the parameter.\n>\n> Dunno. I would still call it `range_diff` until the day comes (if ever)\n> when `--interdiff` is implemented. And I would call the `range_diff`\n> parameter `range_diff_opt` instead, or some such.\n\nSharing the variable between interdiff and range-diff was the idea\ninitially, however, I later decided that the --range-diff and\n--interdiff options didn't need to be mutually-exclusive, so, in the\nre-roll, all variables now have distinct names (no commonality between\nthem).\n\n> > + if (range_diff) {\n> > + struct argv_array ranges = ARGV_ARRAY_INIT;\n> > + infer_diff_ranges(&ranges, range_diff, head);\n> > + if (get_range_diff(&diff, &ranges))\n> > + die(_(\"failed to generate range-diff\"));\n>\n> BTW I like to have an extra space in front of all the range-diff lines, to\n> make it easier to discern them from the rest.\n\nI'm not sure what you mean. Perhaps I'm misreading your comment.\n\n> > if (!use_stdout &&\n> > open_next_file(NULL, rev->numbered_files ? NULL : \"cover-letter\", rev, quiet))\n> > - return;\n> > + goto done;\n>\n> Hmm. If you think you will add more `goto done`s in the future, I guess\n> this is okay. Otherwise, it would probably make sense to go ahead and\n> simply `strbuf_release(&diff)` before `return`ing.\n\nIn the re-roll, there are several more things which need to be cleaned\nup, so the 'goto' makes life easier.\n\n> > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> > + const char *range_diff = NULL;\n>\n> Maybe `range_diff_opt`? It's not exactly the range diff that is contained\n> in this variable.\n\nI could, though I was trying to keep it shorter rather than longer.\nThis is still the same in the re-roll, but I can rename it if you\ninsist.\n\n> > + if (range_diff && !cover_letter)\n> > + die(_(\"--range-diff requires --cover-letter\"));\n>\n> I guess this will be changed in a future patch, allowing a single patch to\n> ship with a range diff, too?\n\nYes, that's already the case in the re-roll.\n\n> > +format_patch () {\n> > + title=$1 &&\n> > + range=$2 &&\n> > + test_expect_success \"format-patch --range-diff ($title)\" '\n> > + git format-patch --stdout --cover-letter --range-diff=$range \\\n> > + master..unmodified >actual &&\n> > + grep \"= 1: .* s/5/A\" actual &&\n> > + grep \"= 2: .* s/4/A\" actual &&\n> > + grep \"= 3: .* s/11/B\" actual &&\n> > + grep \"= 4: .* s/12/B\" actual\n>\n> I guess this might make sense if `format_patch` was not a function, but it\n> is specifically marked as a function... so... shouldn't these `grep`s also\n> be using function parameters?\n\nA later patch adds a second test which specifies the same ranges but\nin a different way, so the result will be the same, hence the\nhard-coded grep'ing. The function avoids repetition across the two\ntests. I suppose I could do this a bit differently, though, to avoid\npretending it's a general-purpose function.\n"},{"id":"352764","messageId":"CAPig+cRXBjvgC6gmnQcYT85kGcE+Aaq1fRuQN-WqoNHQkam=aQ@mail.gmail.com","threadId":"48598","inReplyTo":"nycvar.QRO.7.76.6.1807171243410.71@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-17T10:50:18Z","receivedAt":"2018-07-17T10:50:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 17, 2018 at 6:45 AM Johannes Schindelin\n wrote:\n> On Wed, 30 May 2018, Eric Sunshine wrote:\n> > + if (strstr(prev, \"..\")) {\n> > + if (!origin)\n> > + die(_(\"failed to infer range-diff ranges\"));\n> > + argv_array_push(args, prev);\n> > + argv_array_pushf(args, \"%s..%s\",\n> > + oid_to_hex(&origin->object.oid),\n> > + oid_to_hex(&head->object.oid));\n> > + } else {\n> > + argv_array_pushf(args, \"%s...%s\", prev,\n> > + oid_to_hex(&head->object.oid));\n> > + }\n>\n> This would be easier to read if the order was inverted:\n>\n> if (!strstr(...))\n> ...\n> else if (!origin)\n> die(...)\n> else {\n> ...\n> }\n>\n> Otherwise, it makes sense to me.\n\nThanks, I re-wrote it that way in the re-roll already.\n"},{"id":"352765","messageId":"nycvar.QRO.7.76.6.1807171259290.71@tvgsbejvaqbjf.bet","threadId":"48598","inReplyTo":"20180530080325.37520-6-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 5/5] format-patch: add --creation-weight tweak for --range-diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-17T11:00:25Z","receivedAt":"2018-07-17T11:00:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 30 May 2018, Eric Sunshine wrote:\n\n> When generating a range-diff, matching up commits between two version of\n> a patch series involves heuristics, thus may give unexpected results.\n> git-branch-diff allows tweaking the heuristic via --creation-weight.\n> Follow suit by accepting --creation-weight in combination with\n> --range-diff when generating a range-diff for a cover-letter.\n\nSince I opted to change this to `--creation-factor`, taking an integer\nbetween 0 and 100 (essentially), this patch will need heavy adjustment ;-=)\n\nThanks,\nDscho\n\n> Signed-off-by: Eric Sunshine \n> ---\n> Documentation/git-format-patch.txt | 8 +++++++-\n> builtin/log.c | 19 +++++++++++++++----\n> 2 files changed, 22 insertions(+), 5 deletions(-)\n> \n> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n> index 25026ae26e..7ed9ec9dae 100644\n> --- a/Documentation/git-format-patch.txt\n> +++ b/Documentation/git-format-patch.txt\n> @@ -23,7 +23,7 @@ SYNOPSIS\n> \t\t [(--reroll-count|-v) ]\n> \t\t [--to=] [--cc=]\n> \t\t [--[no-]cover-letter] [--quiet] [--notes[=]]\n> -\t\t [--range-diff=]\n> +\t\t [--range-diff= [--creation-weight=]]\n> \t\t [--progress]\n> \t\t []\n> \t\t [ | ]\n> @@ -240,6 +240,12 @@ feeding the result to `git send-email`.\n> \tdisjoint (for example `git format-patch --cover-letter\n> \t--range-diff=feature/v1~3..feature/v1 -3 feature/v2`).\n> \n> +--creation-weight=::\n> +\tUsed with `--range-diff`, tweak the heuristic which matches up commits\n> +\tbetween the previous and current series of patches by adjusting the\n> +\tcreation/deletion cost fudge factor. See linkgit:git-branch-diff[1])\n> +\tfor details.\n> +\n> --notes[=]::\n> \tAppend the notes (see linkgit:git-notes[1]) for the commit\n> \tafter the three-dash line.\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 3089d3a50a..2c49011b51 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1012,12 +1012,16 @@ static void infer_diff_ranges(struct argv_array *args,\n> }\n> \n> static int get_range_diff(struct strbuf *sb,\n> -\t\t\t const struct argv_array *ranges)\n> +\t\t\t const struct argv_array *ranges,\n> +\t\t\t const char *creation_weight)\n> {\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \n> \tcp.git_cmd = 1;\n> \targv_array_pushl(&cp.args, \"branch-diff\", \"--no-color\", NULL);\n> +\tif (creation_weight)\n> +\t\targv_array_pushf(&cp.args,\n> +\t\t\t\t \"--creation-weight=%s\", creation_weight);\n> \targv_array_pushv(&cp.args, ranges->argv);\n> \treturn capture_command(&cp, sb, 0);\n> }\n> @@ -1047,7 +1051,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \t\t\t int nr, struct commit **list,\n> \t\t\t const char *branch_name,\n> \t\t\t int quiet,\n> -\t\t\t const char *range_diff)\n> +\t\t\t const char *range_diff,\n> +\t\t\t const char *creation_weight)\n> {\n> \tconst char *committer;\n> \tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\";\n> @@ -1070,7 +1075,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \tif (range_diff) {\n> \t\tstruct argv_array ranges = ARGV_ARRAY_INIT;\n> \t\tinfer_diff_ranges(&ranges, range_diff, origin, head);\n> -\t\tif (get_range_diff(&diff, &ranges))\n> +\t\tif (get_range_diff(&diff, &ranges, creation_weight))\n> \t\t\tdie(_(\"failed to generate range-diff\"));\n> \t\targv_array_clear(&ranges);\n> \t}\n> @@ -1495,6 +1500,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \tint show_progress = 0;\n> \tstruct progress *progress = NULL;\n> \tconst char *range_diff = NULL;\n> +\tconst char *creation_weight = NULL;\n> \n> \tconst struct option builtin_format_patch_options[] = {\n> \t\t{ OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n> @@ -1570,6 +1576,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"show progress while generating patches\")),\n> \t\tOPT_STRING(0, \"range-diff\", &range_diff, N_(\"rev-range\"),\n> \t\t\t N_(\"show changes against in cover letter\")),\n> +\t\tOPT_STRING(0, \"creation-weight\", &creation_weight, N_(\"factor\"),\n> +\t\t\t N_(\"fudge factor by which creation is weighted\")),\n> \t\tOPT_END()\n> \t};\n> \n> @@ -1664,6 +1672,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\tdie (_(\"--subject-prefix/--rfc and -k are mutually exclusive.\"));\n> \trev.preserve_subject = keep_subject;\n> \n> +\tif (creation_weight && !range_diff)\n> +\t\tdie(_(\"--creation-weight requires --range-diff\"));\n> +\n> \targc = setup_revisions(argc, argv, &rev, &s_r_opt);\n> \tif (argc > 1)\n> \t\tdie (_(\"unrecognized argument: %s\"), argv[1]);\n> @@ -1827,7 +1838,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\t\tgen_message_id(&rev, \"cover\");\n> \t\tmake_cover_letter(&rev, use_stdout,\n> \t\t\t\t origin, nr, list, branch_name, quiet,\n> -\t\t\t\t range_diff);\n> +\t\t\t\t range_diff, creation_weight);\n> \t\tprint_bases(&bases, rev.diffopt.file);\n> \t\tprint_signature(rev.diffopt.file);\n> \t\ttotal++;\n> -- \n> 2.17.1.1235.ge295dfb56e\n> \n> \n"},{"id":"353634","messageId":"nycvar.QRO.7.76.6.1807261249490.71@tvgsbejvaqbjf.bet","threadId":"48598","inReplyTo":"CAPig+cTKGd8N78XvW-rmBEZC7ykcJsE+na1V_vCVXTUhGrFe4Q@mail.gmail.com","subject":"Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-26T10:55:53Z","receivedAt":"2018-07-26T10:56:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Tue, 17 Jul 2018, Eric Sunshine wrote:\n\n> On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin\n> wrote:\n> > On Wed, 30 May 2018, Eric Sunshine wrote:\n> \n> > > + if (range_diff) {\n> > > + struct argv_array ranges = ARGV_ARRAY_INIT;\n> > > + infer_diff_ranges(&ranges, range_diff, head);\n> > > + if (get_range_diff(&diff, &ranges))\n> > > + die(_(\"failed to generate range-diff\"));\n> >\n> > BTW I like to have an extra space in front of all the range-diff lines, to\n> > make it easier to discern them from the rest.\n> \n> I'm not sure what you mean. Perhaps I'm misreading your comment.\n\nSorry, I was really unclear.\n\nIn the cover letters sent out by GitGitGadget (or earlier, my\nmail-patch-series.sh command), I took pains to indent the entire\nrange-diff (or interdiff) with a single space. That is, the footer\n\"Range-diff vs v:\" is not indented at all, but all subsequent lines of\nthe range-diff have a leading space.\n\nThe original reason was to stop confusing `git apply` when sending an\ninterdiff as part of a single patch without a cover letter (in which case\nmail-patch-series.sh inserted the interdiff below the `---` marker, and\nthe interdiff would have looked like the start of the real diff\notherwise).\n\nIn the meantime, I got used to this indentation so much that I do not want\nto miss it, it is a relatively easy and intuitive visual marker.\n\nThis, however, will be harder to achieve now that you are using the\nlibified range-diff.\n\n> > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> > > + const char *range_diff = NULL;\n> >\n> > Maybe `range_diff_opt`? It's not exactly the range diff that is contained\n> > in this variable.\n> \n> I could, though I was trying to keep it shorter rather than longer.\n> This is still the same in the re-roll, but I can rename it if you\n> insist.\n\nI think it will confuse me in the future if I read `range_diff` and even\nthe data type suggests that it could hold the output of a `git range-diff\n` run.\n\nSo I would like to insist.\n\n> > > +format_patch () {\n> > > + title=$1 &&\n> > > + range=$2 &&\n> > > + test_expect_success \"format-patch --range-diff ($title)\" '\n> > > + git format-patch --stdout --cover-letter --range-diff=$range \\\n> > > + master..unmodified >actual &&\n> > > + grep \"= 1: .* s/5/A\" actual &&\n> > > + grep \"= 2: .* s/4/A\" actual &&\n> > > + grep \"= 3: .* s/11/B\" actual &&\n> > > + grep \"= 4: .* s/12/B\" actual\n> >\n> > I guess this might make sense if `format_patch` was not a function, but it\n> > is specifically marked as a function... so... shouldn't these `grep`s also\n> > be using function parameters?\n> \n> A later patch adds a second test which specifies the same ranges but\n> in a different way, so the result will be the same, hence the\n> hard-coded grep'ing. The function avoids repetition across the two\n> tests. I suppose I could do this a bit differently, though, to avoid\n> pretending it's a general-purpose function.\n\nIf you can think of a way that would make this easier to read for, say,\nmyself if I ever find myself debugging a regression caught by this test, I\nwould appreciate that.\n\nCiao,\nDscho\n"},{"id":"353635","messageId":"b8c65df5-d150-1bc6-1858-c2c042ae4bb2@gmail.com","threadId":"48598","inReplyTo":"20180530080325.37520-1-sunshine@sunshineco.com","subject":"Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-07-26T12:03:48Z","receivedAt":"2018-07-26T12:03:54Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2018-05-30 10:03, Eric Sunshine wrote:\n> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named\n> git-branch-diff[1] which computes differences between two versions of a\n> patch series. Such a diff can be a useful aid for reviewers when\n> inserted into a cover letter. However, doing so requires manual\n> generation (invoking git-branch-diff) and copy/pasting the result into\n> the cover letter.\n> \n> This series fully automates the process by adding a --range-diff option\n> to git-format-patch. \n\n[...]\n\n> \n> Eric Sunshine (5):\n> format-patch: allow additional generated content in\n> make_cover_letter()\n> format-patch: add --range-diff option to embed diff in cover letter\n> format-patch: extend --range-diff to accept revision range\n> format-patch: teach --range-diff to respect -v/--reroll-count\n> format-patch: add --creation-weight tweak for --range-diff\n> \n> Documentation/git-format-patch.txt | 18 +++++\n> builtin/log.c | 119 ++++++++++++++++++++++++-----\n> t/t7910-branch-diff.sh | 16 ++++\n> 3 files changed, 132 insertions(+), 21 deletions(-)\n\nWould it make sense to mention new option in the cover letter section of\nDocumentation/SubmittingPatches?\n\n--\nBest regards, Andrei Rybak\n"},{"id":"353649","messageId":"nycvar.QRO.7.76.6.1807261755500.10478@tvgsbejvaqbjf.bet","threadId":"48598","inReplyTo":"b8c65df5-d150-1bc6-1858-c2c042ae4bb2@gmail.com","subject":"Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-26T15:57:01Z","receivedAt":"2018-07-26T15:57:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Andrei,\n\nOn Thu, 26 Jul 2018, Andrei Rybak wrote:\n\n> On 2018-05-30 10:03, Eric Sunshine wrote:\n> > Dscho recently implemented a 'tbdiff' replacement as a Git builtin named\n> > git-branch-diff[1] which computes differences between two versions of a\n> > patch series. Such a diff can be a useful aid for reviewers when\n> > inserted into a cover letter. However, doing so requires manual\n> > generation (invoking git-branch-diff) and copy/pasting the result into\n> > the cover letter.\n> > \n> > This series fully automates the process by adding a --range-diff option\n> > to git-format-patch. \n> \n> [...]\n> \n> > \n> > Eric Sunshine (5):\n> > format-patch: allow additional generated content in\n> > make_cover_letter()\n> > format-patch: add --range-diff option to embed diff in cover letter\n> > format-patch: extend --range-diff to accept revision range\n> > format-patch: teach --range-diff to respect -v/--reroll-count\n> > format-patch: add --creation-weight tweak for --range-diff\n> > \n> > Documentation/git-format-patch.txt | 18 +++++\n> > builtin/log.c | 119 ++++++++++++++++++++++++-----\n> > t/t7910-branch-diff.sh | 16 ++++\n> > 3 files changed, 132 insertions(+), 21 deletions(-)\n> \n> Would it make sense to mention new option in the cover letter section of\n> Documentation/SubmittingPatches?\n\nI would be hesitant to add it there already. Let's prove first that these\noptions are really as useful as we hope they are.\n\nIt might turn out that in many cases, the range-diff is just stupidly\nunreadable, for example. Personally, I already miss the color coding when\nlooking at range-diffs sent via mails.\n\nCiao,\nJohannes\n"},{"id":"353677","messageId":"CAPig+cSYXYrEdJMpkC_emi3u5PY2GFmZw0nsn5EbEBtU28ZXtw@mail.gmail.com","threadId":"48598","inReplyTo":"nycvar.QRO.7.76.6.1807261249490.71@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-26T20:57:12Z","receivedAt":"2018-07-26T20:57:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 26, 2018 at 6:56 AM Johannes Schindelin\n wrote:\n> On Tue, 17 Jul 2018, Eric Sunshine wrote:\n> > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin\n> > wrote:\n> > > BTW I like to have an extra space in front of all the range-diff lines, to\n> > > make it easier to discern them from the rest.\n> >\n> > I'm not sure what you mean. Perhaps I'm misreading your comment.\n>\n> Sorry, I was really unclear.\n>\n> In the cover letters sent out by GitGitGadget (or earlier, my\n> mail-patch-series.sh command), I took pains to indent the entire\n> range-diff (or interdiff) with a single space. That is, the footer\n> \"Range-diff vs v:\" is not indented at all, but all subsequent lines of\n> the range-diff have a leading space.\n>\n> The original reason was to stop confusing `git apply` when sending an\n> interdiff as part of a single patch without a cover letter (in which case\n> mail-patch-series.sh inserted the interdiff below the `---` marker, and\n> the interdiff would have looked like the start of the real diff\n> otherwise).\n\nThe new version[1] likewise indents the interdiff to avoid confusing\ngit-am / git-apply.\n\n[1]: https://public-inbox.org/git/20180722095717.17912-1-sunshine@sunshineco.com/\n\n> In the meantime, I got used to this indentation so much that I do not want\n> to miss it, it is a relatively easy and intuitive visual marker.\n>\n> This, however, will be harder to achieve now that you are using the\n> libified range-diff.\n\nI toyed with indenting the range-diff in both the cover letter and\nbelow the \"---\" line in a patch. With the libified range-diff, doing\nso involves modifying the range-diff implementation (rather than\nhaving the consumer of the range-diff manage the indentation locally),\nso it adds a bit of complexity to show_range_diff(), though perhaps\nnot too much.\n\nHowever, I opted against it for a few reasons. First, \"header\" lines\napart, all lines of the range-diff are already indented, and the\nexisting indentation was sufficient (for me, at least) as a visual\nmarker. Second, range-diffs tend to be _wide_, especially the header\nlines, and I was loath to make it wider by indenting more. Third, due\nto the existing indentation of the diff proper, a range-diff won't\nconfuse git-am / git-apply, nor will the unindented header lines, so\nextra indentation seemed superfluous.\n\n> > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> > > > + const char *range_diff = NULL;\n> > >\n> > > Maybe `range_diff_opt`? It's not exactly the range diff that is contained\n> > > in this variable.\n> >\n> > I could, though I was trying to keep it shorter rather than longer.\n> > This is still the same in the re-roll, but I can rename it if you\n> > insist.\n>\n> I think it will confuse me in the future if I read `range_diff` and even\n> the data type suggests that it could hold the output of a `git range-diff\n> ` run.\n>\n> So I would like to insist.\n\nIn the new version[1], this variable is named 'rdiff_prev' (the\n\"previous\" version against which the range-diff is to be generated).\n\n> > > > +format_patch () {\n> > > > + title=$1 &&\n> > > > + range=$2 &&\n> > > > + test_expect_success \"format-patch --range-diff ($title)\" '\n> > > > + git format-patch --stdout --cover-letter --range-diff=$range \\\n> > > > + master..unmodified >actual &&\n> > > > + grep \"= 1: .* s/5/A\" actual &&\n> > > > + grep \"= 2: .* s/4/A\" actual &&\n> > > > + grep \"= 3: .* s/11/B\" actual &&\n> > > > + grep \"= 4: .* s/12/B\" actual\n> > >\n> > > I guess this might make sense if `format_patch` was not a function, but it\n> > > is specifically marked as a function... so... shouldn't these `grep`s also\n> > > be using function parameters?\n> >\n> > A later patch adds a second test which specifies the same ranges but\n> > in a different way, so the result will be the same, hence the\n> > hard-coded grep'ing. The function avoids repetition across the two\n> > tests. I suppose I could do this a bit differently, though, to avoid\n> > pretending it's a general-purpose function.\n>\n> If you can think of a way that would make this easier to read for, say,\n> myself if I ever find myself debugging a regression caught by this test, I\n> would appreciate that.\n\nIn the new version, the function is gone; it looks like this:\n\n--- >8 ---\nfor prev in topic master..topic\ndo\n test_expect_success \"format-patch --range-diff=$prev\" '\n git format-patch --stdout --cover-letter --range-diff=$prev \\\n master..unmodified >actual &&\n grep \"= 1: .* s/5/A\" actual &&\n grep \"= 2: .* s/4/A\" actual &&\n grep \"= 3: .* s/11/B\" actual &&\n grep \"= 4: .* s/12/B\" actual\n '\ndone\n--- >8 ---\n"},{"id":"353719","messageId":"nycvar.QRO.7.76.6.1807271717020.10478@tvgsbejvaqbjf.bet","threadId":"48598","inReplyTo":"CAPig+cSYXYrEdJMpkC_emi3u5PY2GFmZw0nsn5EbEBtU28ZXtw@mail.gmail.com","subject":"Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-27T15:18:39Z","receivedAt":"2018-07-27T15:18:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Thu, 26 Jul 2018, Eric Sunshine wrote:\n\n> On Thu, Jul 26, 2018 at 6:56 AM Johannes Schindelin\n> wrote:\n> > On Tue, 17 Jul 2018, Eric Sunshine wrote:\n> > > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin\n> > > wrote:\n> > > > BTW I like to have an extra space in front of all the range-diff\n> > > > lines, to make it easier to discern them from the rest.\n> > >\n> > > I'm not sure what you mean. Perhaps I'm misreading your comment.\n> >\n> > Sorry, I was really unclear.\n> >\n> > In the cover letters sent out by GitGitGadget (or earlier, my\n> > mail-patch-series.sh command), I took pains to indent the entire\n> > range-diff (or interdiff) with a single space. That is, the footer\n> > \"Range-diff vs v:\" is not indented at all, but all subsequent lines\n> > of the range-diff have a leading space.\n> >\n> > The original reason was to stop confusing `git apply` when sending an\n> > interdiff as part of a single patch without a cover letter (in which\n> > case mail-patch-series.sh inserted the interdiff below the `---`\n> > marker, and the interdiff would have looked like the start of the real\n> > diff otherwise).\n> \n> The new version[1] likewise indents the interdiff to avoid confusing\n> git-am / git-apply.\n> \n> [1]: https://public-inbox.org/git/20180722095717.17912-1-sunshine@sunshineco.com/\n\nGreat!\n\n> > In the meantime, I got used to this indentation so much that I do not\n> > want to miss it, it is a relatively easy and intuitive visual marker.\n> >\n> > This, however, will be harder to achieve now that you are using the\n> > libified range-diff.\n> \n> I toyed with indenting the range-diff in both the cover letter and\n> below the \"---\" line in a patch. With the libified range-diff, doing\n> so involves modifying the range-diff implementation (rather than\n> having the consumer of the range-diff manage the indentation locally),\n> so it adds a bit of complexity to show_range_diff(), though perhaps\n> not too much.\n> \n> However, I opted against it for a few reasons. First, \"header\" lines\n> apart, all lines of the range-diff are already indented, and the\n> existing indentation was sufficient (for me, at least) as a visual\n> marker. Second, range-diffs tend to be _wide_, especially the header\n> lines, and I was loath to make it wider by indenting more. Third, due\n> to the existing indentation of the diff proper, a range-diff won't\n> confuse git-am / git-apply, nor will the unindented header lines, so\n> extra indentation seemed superfluous.\n\nTotally understandable. For some reason, I never thought about that fact\n(a range-diff is *not* a diff) when changing mail-patch-series.ts to use\nrange-diffs instead of interdiffs.\n\n> > > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> > > > > + const char *range_diff = NULL;\n> > > >\n> > > > Maybe `range_diff_opt`? It's not exactly the range diff that is\n> > > > contained in this variable.\n> > >\n> > > I could, though I was trying to keep it shorter rather than longer.\n> > > This is still the same in the re-roll, but I can rename it if you\n> > > insist.\n> >\n> > I think it will confuse me in the future if I read `range_diff` and\n> > even the data type suggests that it could hold the output of a `git\n> > range-diff ` run.\n> >\n> > So I would like to insist.\n> \n> In the new version[1], this variable is named 'rdiff_prev' (the\n> \"previous\" version against which the range-diff is to be generated).\n\nThank you.\n\n> > > > > +format_patch () {\n> > > > > + title=$1 &&\n> > > > > + range=$2 &&\n> > > > > + test_expect_success \"format-patch --range-diff ($title)\" '\n> > > > > + git format-patch --stdout --cover-letter --range-diff=$range \\\n> > > > > + master..unmodified >actual &&\n> > > > > + grep \"= 1: .* s/5/A\" actual &&\n> > > > > + grep \"= 2: .* s/4/A\" actual &&\n> > > > > + grep \"= 3: .* s/11/B\" actual &&\n> > > > > + grep \"= 4: .* s/12/B\" actual\n> > > >\n> > > > I guess this might make sense if `format_patch` was not a\n> > > > function, but it is specifically marked as a function... so...\n> > > > shouldn't these `grep`s also be using function parameters?\n> > >\n> > > A later patch adds a second test which specifies the same ranges but\n> > > in a different way, so the result will be the same, hence the\n> > > hard-coded grep'ing. The function avoids repetition across the two\n> > > tests. I suppose I could do this a bit differently, though, to avoid\n> > > pretending it's a general-purpose function.\n> >\n> > If you can think of a way that would make this easier to read for,\n> > say, myself if I ever find myself debugging a regression caught by\n> > this test, I would appreciate that.\n> \n> In the new version, the function is gone; it looks like this:\n> \n> --- >8 ---\n> for prev in topic master..topic\n> do\n> test_expect_success \"format-patch --range-diff=$prev\" '\n> git format-patch --stdout --cover-letter --range-diff=$prev \\\n> master..unmodified >actual &&\n> grep \"= 1: .* s/5/A\" actual &&\n> grep \"= 2: .* s/4/A\" actual &&\n> grep \"= 3: .* s/11/B\" actual &&\n> grep \"= 4: .* s/12/B\" actual\n> '\n> done\n> --- >8 ---\n\nLooks good.\n\nThank you so much!\nDscho\n"}]}