format-patch: pass --right-only to range-diff
RFC PATCH 1/1 format-patch: pass --right-only to range-diff
From: ZheNing Hu <[email protected]>
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposing 'git format-patch --cover-letter --range-diff' may mistakenly place upstream commit in the range-diff output. Teach format-patch pass --right-only to range-diff, maybe can avoid this kind of mistake.
Because git rebase --apply will internally call git format-patch -k --stdout --full-index --cherry-pick --right-only --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter --pretty=mboxrd --topo-order --no-base, In order to avoid the ambiguity of --right-only, users can call format-patch --range-diff --only-right instead of --right-only to only emit output related to the second range.
---
[GSOC][RFC] format-patch: pass --right-only to range-diff
Transfer parameters are relatively simple. But because I can' t reproduce the situation, There's no big difference between adding ---only-right and not adding ---only-right. So I haven't written the test files for the time being.
I may need to ask reviewers opinion first.
this want to fix #876 Thanks.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-898%2Fadlternative%2Fformat-patch-range-diff-right-only-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-898/adlternative/format-patch-range-diff-right-only-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/898
Documentation/git-format-patch.txt | 5 ++++- builtin/log.c | 15 ++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-)
Documentation/git-format-patch.txt +4 -1
@@ -27,7 +27,7 @@ SYNOPSIS [--[no-]encode-email-headers] [--no-notes | --notes[=<ref>]] [--interdiff=<previous>] - [--range-diff=<previous> [--creation-factor=<percent>]] + [--range-diff=<previous> [--creation-factor=<percent>] [--right-only]] [--filename-max-length=<n>] [--progress] [<common diff options>]
@@ -301,6 +301,9 @@ material (this may change in the future). creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) for details. +--only-right: + Used with `--range-diff`, only emit output related to the second range. + --notes[=<ref>]:: --no-notes:: Append the notes (see linkgit:git-notes[1]) for the commit
builtin/log.c +10 -5
@@ -1153,7 +1153,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,struct commit *origin, int nr, struct commit **list, const char *branch_name, - int quiet) + int quiet, int right_only) { const char *committer;struct shortlog log;
@@ -1228,7 +1228,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, .creation_factor = rev->creation_factor, .dual_color = 1, .diffopt = &opts, - .other_arg = &other_arg + .other_arg = &other_arg, + .right_only = right_only };diff_setup (&opts);
@@ -1732,7 +1733,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)struct strbuf rdiff2 =STRBUF_INIT ;struct strbuf rdiff_title =STRBUF_INIT ;int creation_factor =-1 ; - +int right_only =0 ;const struct option builtin_format_patch_options[] = { OPT_CALLBACK_F('n', "numbered", &numbered, NULL, N_("use [PATCH n/m] even with a single patch"),
@@ -1814,6 +1815,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) parse_opt_object_name),OPT_STRING (0 ,"range-diff" , &rdiff_prev,N_ ("refspec" ),N_ ("show changes against <refspec> in cover letter or single patch" )), +OPT_BOOL (0 ,"only-right" , &right_only, +N_ ("only emit output related to the second range" )),OPT_INTEGER (0 ,"creation-factor" , &creation_factor,N_ ("percentage by which creation is weighted" )),OPT_END ()
@@ -2087,7 +2090,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ; else if (!rdiff_prev) die(_("--creation-factor requires --range-diff")); - +if (right_only && !rdiff_prev) +die (_ ("--right_only requires --range-diff" ));if (rdiff_prev) {if (!cover_letter && total !=1 )die (_ ("--range-diff requires --cover-letter or single patch" ));
@@ -2134,7 +2138,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)if (thread)gen_message_id (&rev,"cover" );make_cover_letter (&rev, !!output_directory, - origin, nr, list, branch_name, quiet); + origin, nr, list, branch_name, quiet, + right_only);print_bases (&bases, rev.diffopt.file);print_signature (rev.diffopt.file); total++;
base-commit: be7935ed8bff19f481b033d0d242c5d5f239ed50
Signature · 1 line
gitgitgadget
Quoted 8 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposing 'git format-patch --cover-letter --range-diff' may mistakenly place upstream commit in the range-diff output. Teachformat-patchpass--right-onlyto range-diff, maybe can avoid this kind of mistake.
Quoted 7 lines
Because
git rebase --applywill internally callgit format-patch -k --stdout --full-index --cherry-pick --right-only --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter --pretty=mboxrd --topo-order --no-base, In order to avoid the ambiguity of--right-only, users can callformat-patch --range-diff --only-rightinstead of--right-onlyto only emit output related to the second range.
A couple of times this new option is called "--right-only" and other times it is "--only-right". This comment makes me think that you meant to call it "--only-right", but...
Quoted 8 lines
Signed-off-by ZheNing Hu <[email protected]>---
[GSOC][RFC] format-patch: pass --right-only to range-diffTransfer parameters are relatively simple. But because I can' t reproduce the situation, There's no big difference between adding ---only-right and not adding ---only-right. So I haven't written the test files for the time being.
Note that I think (and Johannes--cc'd--could confirm) that what you want is '--left-only' to discard rebased changes from the upstream branch:
In either case, this should help you reproduce it (and would be a good starting point for adding a test case). Here, we're using '--left-only' to discard changes from upstream (here, the 'main' branch) after rebasing:
#!/bin/sh
set -e
rm -fr repo
git init repo
cd repo
git branch -M main
echo "base" >base
git add base
git commit -m "base"
git checkout -b my-feature
echo "feature" >feature
git add feature
git commit -m "feature"
base="$(git rev-parse main)"
old="$(git rev-parse my-feature)"
git checkout main
echo "other" >>base
git add base
git commit -m "new"
git checkout my-feature
git rebase $base --onto main
tip="$(git rev-parse my-feature)"
git range-diff $base $old $tip
git range-diff --left-only $base $old $tip
Quoted 26 lines
I may need to ask reviewers opinion first.
this want to fix #876 Thanks.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-898%2Fadlternative%2Fformat-patch-range-diff-right-only-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-898/adlternative/format-patch-range-diff-right-only-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/898Documentation/git-format-patch.txt | 5 ++++- builtin/log.c | 15 ++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-)Documentation/git-format-patch.txt +1
-1@@ -27,7 +27,7 @@ SYNOPSIS [--[no-]encode-email-headers] [--no-notes | --notes[=<ref>]] [--interdiff=<previous>] - [--range-diff=<previous> [--creation-factor=<percent>]] + [--range-diff=<previous> [--creation-factor=<percent>] [--right-only]] [--filename-max-length=<n>] [--progress] [<common diff options>]
Documentation/git-format-patch.txt material (this may change in the future). · line 306
Quoted 6 lines
Documentation/git-format-patch.txt +3
-0@@ -301,6 +301,9 @@ material (this may change in the future). creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) for details. +--only-right: + Used with `--range-diff`, only emit output related to the second range. +
s/--only-right/--right-only?
Quoted 27 lines
Documentation/git-format-patch.txt +0
-0--notes[=<ref>]:: --no-notes:: Append the notes (see linkgit:git-notes[1]) for the commitbuiltin/log.c +3
-2@@ -1153,7 +1153,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,struct commit *origin, int nr, struct commit **list, const char *branch_name, - int quiet) + int quiet, int right_only) { const char *committer;struct shortlog log;@@ -1228,7 +1228,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, .creation_factor = rev->creation_factor, .dual_color = 1, .diffopt = &opts, - .other_arg = &other_arg + .other_arg = &other_arg, + .right_only = right_only };diff_setup (&opts);
builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 1736
Quoted 5 lines
builtin/log.c +1
-1@@ -1732,7 +1733,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)struct strbuf rdiff2 =STRBUF_INIT ;struct strbuf rdiff_title =STRBUF_INIT ;int creation_factor =-1 ; - +int right_only =0 ;
Nit: the newline between creation_factor's declaration and builtin_format_patch_options's is good to keep.
Quoted 13 lines
builtin/log.c +2
-0const struct option builtin_format_patch_options[] = { OPT_CALLBACK_F('n', "numbered", &numbered, NULL, N_("use [PATCH n/m] even with a single patch"),@@ -1814,6 +1815,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) parse_opt_object_name),OPT_STRING (0 ,"range-diff" , &rdiff_prev,N_ ("refspec" ),N_ ("show changes against <refspec> in cover letter or single patch" )), +OPT_BOOL (0 ,"only-right" , &right_only, +N_ ("only emit output related to the second range" )),OPT_INTEGER (0 ,"creation-factor" , &creation_factor,N_ ("percentage by which creation is weighted" )),OPT_END ()
builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 2094
Quoted 6 lines
builtin/log.c +2
-1@@ -2087,7 +2090,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ; else if (!rdiff_prev) die(_("--creation-factor requires --range-diff")); - +if (right_only && !rdiff_prev) +die (_ ("--right_only requires --range-diff" ));
These may be good to combine into one big conditional that dies whenever an option requiring --range-diff is set, but --range-diff is not:
if (!rdiff_prev) {
if (creation_factor)
die(_("--creation-factor requires --range-diff"));
if (right_only)
die(_("--right-only requires --range-diff"));
}Thanks,
Taylor
Quoted 66 lines · Taylor Blau <[email protected]> 于2021年3月9日周二 上午4:18写道：
Quoted 8 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposing 'git format-patch --cover-letter --range-diff' may mistakenly place upstream commit in the range-diff output. Teachformat-patchpass--right-onlyto range-diff, maybe can avoid this kind of mistake.Quoted 7 lines
Because
git rebase --applywill internally callgit format-patch -k --stdout --full-index --cherry-pick --right-only --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter --pretty=mboxrd --topo-order --no-base, In order to avoid the ambiguity of--right-only, users can callformat-patch --range-diff --only-rightinstead of--right-onlyto only emit output related to the second range.A couple of times this new option is called "--right-only" and other times it is "--only-right". This comment makes me think that you meant to call it "--only-right", but...
Quoted 8 lines
Signed-off-by ZheNing Hu <[email protected]>---
[GSOC][RFC] format-patch: pass --right-only to range-diffTransfer parameters are relatively simple. But because I can' t reproduce the situation, There's no big difference between adding ---only-right and not adding ---only-right. So I haven't written the test files for the time being.
Note that I think (and Johannes--cc'd--could confirm) that what you want is '--left-only' to discard rebased changes from the upstream branch:
In either case, this should help you reproduce it (and would be a good starting point for adding a test case). Here, we're using '--left-only' to discard changes from upstream (here, the 'main' branch) after rebasing:
#!/bin/sh
set -e
rm -fr repo
git init repo
cd repogit branch -M main
echo "base" >base
git add base
git commit -m "base"git checkout -b my-feature
echo "feature" >feature
git add feature
git commit -m "feature"base="$(git rev-parse main)"
old="$(git rev-parse my-feature)"git checkout main
echo "other" >>base
git add base
git commit -m "new"
Quoted 8 lines
git checkout my-feature
git rebase $base --onto maintip="$(git rev-parse my-feature)"
git range-diff $base $old $tip
git range-diff --left-only $base $old $tip
Thanks, I have seen an obvious phenomenon from your shell script, and I wrote test code based on it.
Quoted 29 lines
Quoted 29 lines
I may need to ask reviewers opinion first.
this want to fix #876 Thanks.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-898%2Fadlternative%2Fformat-patch-range-diff-right-only-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-898/adlternative/format-patch-range-diff-right-only-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/898Documentation/git-format-patch.txt | 5 ++++- builtin/log.c | 15 ++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-)Documentation/git-format-patch.txt +1
-1@@ -27,7 +27,7 @@ SYNOPSIS [--[no-]encode-email-headers] [--no-notes | --notes[=<ref>]] [--interdiff=<previous>] - [--range-diff=<previous> [--creation-factor=<percent>]] + [--range-diff=<previous> [--creation-factor=<percent>] [--right-only]] [--filename-max-length=<n>] [--progress] [<common diff options>]@@ -301,6 +301,9 @@ material (this may change in the future). creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) for details.
Quoted 5 lines
Documentation/git-format-patch.txt material (this may change in the future). · line 306
Quoted 3 lines
Documentation/git-format-patch.txt +3
-0+--only-right: + Used with `--range-diff`, only emit output related to the second range. +s/--only-right/--right-only?
Now it's all "--left-only".
Quoted 51 lines
Quoted 28 lines
Documentation/git-format-patch.txt +0
-0--notes[=<ref>]:: --no-notes:: Append the notes (see linkgit:git-notes[1]) for the commitbuiltin/log.c +3
-2@@ -1153,7 +1153,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,struct commit *origin, int nr, struct commit **list, const char *branch_name, - int quiet) + int quiet, int right_only) { const char *committer;struct shortlog log;@@ -1228,7 +1228,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, .creation_factor = rev->creation_factor, .dual_color = 1, .diffopt = &opts, - .other_arg = &other_arg + .other_arg = &other_arg, + .right_only = right_only };diff_setup (&opts);builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 1736
Quoted 5 lines
builtin/log.c +1
-1@@ -1732,7 +1733,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)struct strbuf rdiff2 =STRBUF_INIT ;struct strbuf rdiff_title =STRBUF_INIT ;int creation_factor =-1 ; - +int right_only =0 ;Nit: the newline between creation_factor's declaration and builtin_format_patch_options's is good to keep.
Quoted 14 lines
builtin/log.c +2
-0const struct option builtin_format_patch_options[] = { OPT_CALLBACK_F('n', "numbered", &numbered, NULL, N_("use [PATCH n/m] even with a single patch"),@@ -1814,6 +1815,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) parse_opt_object_name),OPT_STRING (0 ,"range-diff" , &rdiff_prev,N_ ("refspec" ),N_ ("show changes against <refspec> in cover letter or single patch" )), +OPT_BOOL (0 ,"only-right" , &right_only, +N_ ("only emit output related to the second range" )),OPT_INTEGER (0 ,"creation-factor" , &creation_factor,N_ ("percentage by which creation is weighted" )),OPT_END ()@@ -2087,7 +2090,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ;
Quoted 9 lines
builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 2094
Quoted 5 lines
builtin/log.c +2
-1else if (!rdiff_prev)die (_ ("--creation-factor requires --range-diff" )); - +if (right_only && !rdiff_prev) +die (_ ("--right_only requires --range-diff" ));These may be good to combine into one big conditional that dies whenever an option requiring --range-diff is set, but --range-diff is not:
Good tip.
Quoted 9 lines
if (!rdiff_prev) { if (creation_factor) die(_("--creation-factor requires --range-diff")); if (right_only) die(_("--right-only requires --range-diff")); }Thanks,
Taylor
Thanks,
ZheNing Hu
Quoted 2 lines · Taylor Blau
Note that I think (and Johannes--cc'd--could confirm) that what you want is '--left-only' to discard rebased changes from the upstream branch:
Does that mean range-diff got --left-only and --right-only backwards? IOW, doesn't
$ git log --left-only A...B
show what is shown in "git log B..A", and if so, shouldn't
$ git range-diff --left-only A...B
which is a synonym for
$ git range-diff B..A A..B
also give commits in the B..A range?
Puzzled....
Quoted 14 lines · Junio C Hamano
Quoted 2 lines · Taylor Blau
Note that I think (and Johannes--cc'd--could confirm) that what you want is '--left-only' to discard rebased changes from the upstream branch:
Does that mean range-diff got --left-only and --right-only backwards? IOW, doesn't
$ git log --left-only A...Bshow what is shown in "git log B..A", and if so, shouldn't
$ git range-diff --left-only A...B
Quoted 7 lines
which is a synonym for
$ git range-diff B..A A..Balso give commits in the B..A range?
Puzzled....
Ah, it was only that "range-diff --help" uses confusing double-negative phrasing. I.e.
--left-only::
Suppress commits that are missing from the first specified range
(or the "left range" when using the `<rev1>...<rev2>` format).If you "suppress those that are missing from X", you are effectively including only those that appear in X, so two conclusions I draw are
- "range-diff" does not get "--left/right-only" backwards;
- its documentation should be improved.
Perhaps
--left-only:: Include only commits that are in the first range (or the "left range" when using the `<rev1>...<rev2>` format).
wouldn't have made me puzzled.
Thanks.
RFC PATCH v2 1/1 format-patch: pass --left-only to range-diff
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposing git format-patch --cover-letter --range-diff may mistakenly place upstream commit in the range-diff output. Teach format-patch pass --left-only to range-diff,can avoid this kind of mistake.
---
[GSOC][RFC] format-patch: pass --left-only to range-diff
With the help of Taylor Blau, I understood why the upstream commit appeared in the range-diff, and completed the writing of the test.
this want to fix #876 Thanks.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-898%2Fadlternative%2Fformat-patch-range-diff-right-only-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-898/adlternative/format-patch-range-diff-right-only-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/898
Range-diff vs v1:
1: 590bd06ee5ee ! 1: 8daffd4f7546 [GSOC][RFC] format-patch: pass --right-only to range-diff
@@ Metadata
Author: ZheNing Hu <[email protected]>
## Commit message ##
- [GSOC][RFC] format-patch: pass --right-only to range-diff
+ [GSOC][RFC] format-patch: pass --left-only to range-diff
In https://lore.kernel.org/git/[email protected]/,
- Taylor Blau proposing 'git format-patch --cover-letter
- --range-diff' may mistakenly place upstream commit in the
- range-diff output. Teach `format-patch` pass `--right-only`
- to range-diff, maybe can avoid this kind of mistake.
-
- Because `git rebase --apply` will internally call `git
- format-patch -k --stdout --full-index --cherry-pick --right-only
- --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter
- --pretty=mboxrd --topo-order --no-base`, In order to avoid the
- ambiguity of `--right-only`, users can call `format-patch
- --range-diff --only-right` instead of `--right-only` to only emit
- output related to the second range.
+ Taylor Blau proposing `git format-patch --cover-letter
+ --range-diff` may mistakenly place upstream commit in the
+ range-diff output. Teach `format-patch` pass `--left-only`
+ to range-diff,can avoid this kind of mistake.
Signed-off-by: ZheNing Hu <[email protected]>
@@ Documentation/git-format-patch.txt: SYNOPSIS
[--no-notes | --notes[=<ref>]]
[--interdiff=<previous>]
- [--range-diff=<previous> [--creation-factor=<percent>]]
-+ [--range-diff=<previous> [--creation-factor=<percent>] [--right-only]]
++ [--range-diff=<previous> [--creation-factor=<percent>] [--left-only]]
[--filename-max-length=<n>]
[--progress]
[<common diff options>]
@@ Documentation/git-format-patch.txt: material (this may change in the future).
creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
for details.
-+--only-right:
-+ Used with `--range-diff`, only emit output related to the second range.
++--left-only:
++ Used with `--range-diff`, only emit output related to the first range.
+
--notes[=<ref>]::
--no-notes::
@@ builtin/log.c: static void make_cover_letter(struct rev_info *rev, int use_separ
int nr, struct commit **list,
const char *branch_name,
- int quiet)
-+ int quiet, int right_only)
++ int quiet, int left_only)
{
const char *committer;
struct shortlog log;
@@ builtin/log.c: static void make_cover_letter(struct rev_info *rev, int use_separ
.diffopt = &opts,
- .other_arg = &other_arg
+ .other_arg = &other_arg,
-+ .right_only = right_only
++ .left_only = left_only
};
diff_setup(&opts);
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
struct strbuf rdiff2 = STRBUF_INIT;
struct strbuf rdiff_title = STRBUF_INIT;
int creation_factor = -1;
--
-+ int right_only = 0;
++ int left_only = 0;
+
const struct option builtin_format_patch_options[] = {
OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
- N_("use [PATCH n/m] even with a single patch"),
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
parse_opt_object_name),
OPT_STRING(0, "range-diff", &rdiff_prev, N_("refspec"),
N_("show changes against <refspec> in cover letter or single patch")),
-+ OPT_BOOL(0, "only-right", &right_only,
-+ N_("only emit output related to the second range")),
++ OPT_BOOL(0, "left-only", &left_only,
++ N_("only emit output related to the first range")),
OPT_INTEGER(0, "creation-factor", &creation_factor,
N_("percentage by which creation is weighted")),
OPT_END()
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
+
+ if (creation_factor < 0)
creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
- else if (!rdiff_prev)
- die(_("--creation-factor requires --range-diff"));
+- else if (!rdiff_prev)
+- die(_("--creation-factor requires --range-diff"));
-
-+ if (right_only && !rdiff_prev)
-+ die(_("--right_only requires --range-diff"));
++ else if (!rdiff_prev) {
++ if (creation_factor >= 0)
++ die(_("--creation-factor requires --range-diff"));
++ if (left_only)
++ die(_("--left-only requires --range-diff"));
++ }
if (rdiff_prev) {
if (!cover_letter && total != 1)
die(_("--range-diff requires --cover-letter or single patch"));
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
make_cover_letter(&rev, !!output_directory,
- origin, nr, list, branch_name, quiet);
+ origin, nr, list, branch_name, quiet,
-+ right_only);
++ left_only);
print_bases(&bases, rev.diffopt.file);
print_signature(rev.diffopt.file);
total++;
+
+ ## t/t3206-range-diff.sh ##
+@@ t/t3206-range-diff.sh: test_expect_success '--left-only/--right-only' '
+ test_cmp expect actual
+ '
+
++test_expect_success 'format-patch --range-diff --left-only' '
++ rm -fr repo &&
++ git init repo &&
++ cd repo &&
++ git branch -M main &&
++ echo "base" >base &&
++ git add base &&
++ git commit -m "base" &&
++ git checkout -b my-feature &&
++ echo "feature" >feature &&
++ git add feature &&
++ git commit -m "feature" &&
++ base="$(git rev-parse main)" &&
++ old="$(git rev-parse my-feature)" &&
++ git checkout main &&
++ echo "other" >>base &&
++ git add base &&
++ git commit -m "new" &&
++ git checkout my-feature &&
++ git rebase $base --onto main &&
++ tip="$(git rev-parse my-feature)" &&
++ git format-patch --range-diff $base $old $tip --cover-letter &&
++ grep "> 1: .* feature$" 0000-cover-letter.patch &&
++ git format-patch --range-diff $base $old $tip --left-only --cover-letter &&
++ ! grep "> 1: .* feature$" 0000-cover-letter.patch
++'
++
++
+ test_doneDocumentation/git-format-patch.txt | 5 ++++- builtin/log.c | 20 ++++++++++++++------ t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 7 deletions(-)
Documentation/git-format-patch.txt +4 -1
@@ -27,7 +27,7 @@ SYNOPSIS [--[no-]encode-email-headers] [--no-notes | --notes[=<ref>]] [--interdiff=<previous>] - [--range-diff=<previous> [--creation-factor=<percent>]] + [--range-diff=<previous> [--creation-factor=<percent>] [--left-only]] [--filename-max-length=<n>] [--progress] [<common diff options>]
@@ -301,6 +301,9 @@ material (this may change in the future). creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) for details. +--left-only: + Used with `--range-diff`, only emit output related to the first range. + --notes[=<ref>]:: --no-notes:: Append the notes (see linkgit:git-notes[1]) for the commit
builtin/log.c +14 -6
@@ -1153,7 +1153,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,struct commit *origin, int nr, struct commit **list, const char *branch_name, - int quiet) + int quiet, int left_only) { const char *committer;struct shortlog log;
@@ -1228,7 +1228,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, .creation_factor = rev->creation_factor, .dual_color = 1, .diffopt = &opts, - .other_arg = &other_arg + .other_arg = &other_arg, + .left_only = left_only };diff_setup (&opts);
@@ -1732,6 +1733,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)struct strbuf rdiff2 =STRBUF_INIT ;struct strbuf rdiff_title =STRBUF_INIT ;int creation_factor =-1 ; +int left_only =0 ;const struct option builtin_format_patch_options[] = { OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
@@ -1814,6 +1816,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) parse_opt_object_name),OPT_STRING (0 ,"range-diff" , &rdiff_prev,N_ ("refspec" ),N_ ("show changes against <refspec> in cover letter or single patch" )), +OPT_BOOL (0 ,"left-only" , &left_only, +N_ ("only emit output related to the first range" )),OPT_INTEGER (0 ,"creation-factor" , &creation_factor,N_ ("percentage by which creation is weighted" )),OPT_END ()
@@ -2085,9 +2089,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)if (creation_factor <0 ) creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ; -else if (!rdiff_prev) -die (_ ("--creation-factor requires --range-diff" )); - +else if (!rdiff_prev) { +if (creation_factor >=0 ) +die (_ ("--creation-factor requires --range-diff" )); +if (left_only) +die (_ ("--left-only requires --range-diff" )); + }if (rdiff_prev) {if (!cover_letter && total !=1 )die (_ ("--range-diff requires --cover-letter or single patch" ));
@@ -2134,7 +2141,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)if (thread)gen_message_id (&rev,"cover" );make_cover_letter (&rev, !!output_directory, - origin, nr, list, branch_name, quiet); + origin, nr, list, branch_name, quiet, + left_only);print_bases (&bases, rev.diffopt.file);print_signature (rev.diffopt.file); total++;
t/t3206-range-diff.sh +28 -0
@@ -748,4 +748,32 @@ test_expect_success '--left-only/--right-only' ' test_cmp expect actual ' +test_expect_success 'format-patch --range-diff --left-only' ' + rm -fr repo && + git init repo && + cd repo && + git branch -M main && + echo "base" >base && + git add base && + git commit -m "base" && + git checkout -b my-feature && + echo "feature" >feature && + git add feature && + git commit -m "feature" && + base="$(git rev-parse main)" && + old="$(git rev-parse my-feature)" && + git checkout main && + echo "other" >>base && + git add base && + git commit -m "new" && + git checkout my-feature && + git rebase $base --onto main && + tip="$(git rev-parse my-feature)" && + git format-patch --range-diff $base $old $tip --cover-letter && + grep "> 1: .* feature$" 0000-cover-letter.patch && + git format-patch --range-diff $base $old $tip --left-only --cover-letter && + ! grep "> 1: .* feature$" 0000-cover-letter.patch +' + + test_done
base-commit: be7935ed8bff19f481b033d0d242c5d5f239ed50
Signature · 1 line
gitgitgadget
Re: [PATCH v2] [GSOC][RFC] format-patch: pass --left-only to range-diff
Quoted 11 lines · ZheNing Hu via GitGitGadget
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposinggit format-patch --cover-letter --range-diffmay mistakenly place upstream commit in the range-diff output. Teachformat-patchpass--left-onlyto range-diff,can avoid this kind of mistake.Signed-off-by ZheNing Hu <[email protected]>---
builtin/log.c +0
-0@@ -2085,9 +2089,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)if (creation_factor <0 )
builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 2096
Quoted 10 lines
builtin/log.c +6
-3creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ; - else if (!rdiff_prev) - die(_("--creation-factor requires --range-diff")); - + else if (!rdiff_prev) { +if (creation_factor >=0 ) +die (_ ("--creation-factor requires --range-diff" )); +if (left_only) +die (_ ("--left-only requires --range-diff" )); + }
This logic starts getting difficult to reason about. It would be easier to understand if written like this:
if (!rdiff_prev) {
if (creation_factor >= 0)
die(_("--creation-factor requires --range-diff"));
if (left_only)
die(_("--left-only requires --range-diff"));
}
if (creation_factor < 0)
creation_factor = RANGE_DIFF_..._DEFAULT;or this (which reduces the indentation a bit):
if (creation_factor >= 0 && !rdiff_prev)
die(_("--creation-factor requires --range-diff"));
if (left_only && !rdiff_prev)
die(_("--left-only requires --range-diff"));
if (creation_factor < 0)
creation_factor = RANGE_DIFF_..._DEFAULT;Subjective; not necessarily worth a re-roll.
builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 2143
Quoted 5 lines
builtin/log.c +2
-1@@ -2134,7 +2141,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)make_cover_letter (&rev, !!output_directory, - origin, nr, list, branch_name, quiet); + origin, nr, list, branch_name, quiet, + left_only);
Nit: This indentation looks odd. One would expect origin and left_only to have the same indentation.
t/t3206-range-diff.sh test_expect_success '--left-only/--right-only' ' · line 756
Quoted 11 lines
t/t3206-range-diff.sh +8
-0@@ -748,4 +748,32 @@ test_expect_success '--left-only/--right-only' ' +test_expect_success 'format-patch --range-diff --left-only' ' + rm -fr repo && + git init repo && + ... + ! grep "> 1: .* feature$" 0000-cover-letter.patch +' + + test_done
Nit: One blank line before test_done() is typical, not two.
Re: [PATCH v2] [GSOC][RFC] format-patch: pass --left-only to range-diff
Quoted 40 lines · Eric Sunshine <[email protected]> 于2021年3月9日周二 下午5:00写道：
Quoted 11 lines · ZheNing Hu via GitGitGadget
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposinggit format-patch --cover-letter --range-diffmay mistakenly place upstream commit in the range-diff output. Teachformat-patchpass--left-onlyto range-diff,can avoid this kind of mistake.Signed-off-by ZheNing Hu <[email protected]>---
builtin/log.c +0
-0@@ -2085,9 +2089,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)if (creation_factor <0 )builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 2096
Quoted 10 lines
builtin/log.c +6
-3creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ; - else if (!rdiff_prev) - die(_("--creation-factor requires --range-diff")); - + else if (!rdiff_prev) { +if (creation_factor >=0 ) +die (_ ("--creation-factor requires --range-diff" )); +if (left_only) +die (_ ("--left-only requires --range-diff" )); + }This logic starts getting difficult to reason about. It would be easier to understand if written like this:
if (!rdiff_prev) { if (creation_factor >= 0) die(_("--creation-factor requires --range-diff")); if (left_only) die(_("--left-only requires --range-diff")); } if (creation_factor < 0) creation_factor = RANGE_DIFF_..._DEFAULT;or this (which reduces the indentation a bit):
Quoted 8 lines
if (creation_factor >= 0 && !rdiff_prev)
die(_("--creation-factor requires --range-diff"));
if (left_only && !rdiff_prev)
die(_("--left-only requires --range-diff"));
if (creation_factor < 0)
creation_factor = RANGE_DIFF_..._DEFAULT;Emm,I would prefer to use the above.
Quoted 11 lines
Subjective; not necessarily worth a re-roll.
builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 2143
Quoted 5 lines
builtin/log.c +2
-1@@ -2134,7 +2141,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)make_cover_letter (&rev, !!output_directory, - origin, nr, list, branch_name, quiet); + origin, nr, list, branch_name, quiet, + left_only);Nit: This indentation looks odd. One would expect
originandleft_onlyto have the same indentation.
Quoted 13 lines
t/t3206-range-diff.sh test_expect_success '--left-only/--right-only' ' · line 756
Quoted 11 lines
t/t3206-range-diff.sh +8
-0@@ -748,4 +748,32 @@ test_expect_success '--left-only/--right-only' ' +test_expect_success 'format-patch --range-diff --left-only' ' + rm -fr repo && + git init repo && + ... + ! grep "> 1: .* feature$" 0000-cover-letter.patch +' + + test_doneNit: One blank line before
test_done()is typical, not two.
Thanks for the reminder.
I'm going to modify these format errors.
RFC PATCH v3 1/1 format-patch: pass --left-only to range-diff
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposing git format-patch --cover-letter --range-diff may mistakenly place upstream commit in the range-diff output. Teach format-patch pass --left-only to range-diff,can avoid this kind of mistake.
---
[GSOC][RFC] format-patch: pass --left-only to range-diff
With the help of Taylor Blau, I understood why the upstream commit appeared in the range-diff, and completed the writing of the test.
this want to fix #876 Thanks.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-898%2Fadlternative%2Fformat-patch-range-diff-right-only-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-898/adlternative/format-patch-range-diff-right-only-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/898
Range-diff vs v2:
1: 8daffd4f7546 ! 1: 5c58eb186d41 [GSOC][RFC] format-patch: pass --left-only to range-diff
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
N_("percentage by which creation is weighted")),
OPT_END()
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
+ _("Interdiff against v%d:"));
+ }
- if (creation_factor < 0)
- creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
-- else if (!rdiff_prev)
-- die(_("--creation-factor requires --range-diff"));
--
-+ else if (!rdiff_prev) {
++ if (!rdiff_prev) {
+ if (creation_factor >= 0)
+ die(_("--creation-factor requires --range-diff"));
+ if (left_only)
+ die(_("--left-only requires --range-diff"));
+ }
++
+ if (creation_factor < 0)
+ creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
+- else if (!rdiff_prev)
+- die(_("--creation-factor requires --range-diff"));
+
if (rdiff_prev) {
if (!cover_letter && total != 1)
- die(_("--range-diff requires --cover-letter or single patch"));
@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (thread)
gen_message_id(&rev, "cover");
make_cover_letter(&rev, !!output_directory,
- origin, nr, list, branch_name, quiet);
+ origin, nr, list, branch_name, quiet,
-+ left_only);
++ left_only);
print_bases(&bases, rev.diffopt.file);
print_signature(rev.diffopt.file);
total++;
@@ t/t3206-range-diff.sh: test_expect_success '--left-only/--right-only' '
+ git rebase $base --onto main &&
+ tip="$(git rev-parse my-feature)" &&
+ git format-patch --range-diff $base $old $tip --cover-letter &&
-+ grep "> 1: .* feature$" 0000-cover-letter.patch &&
++ grep "> 1: .* feature$" 0000-cover-letter.patch &&
+ git format-patch --range-diff $base $old $tip --left-only --cover-letter &&
-+ ! grep "> 1: .* feature$" 0000-cover-letter.patch
++ ! grep "> 1: .* feature$" 0000-cover-letter.patch
+'
-+
+
test_doneDocumentation/git-format-patch.txt | 5 ++++- builtin/log.c | 20 +++++++++++++++----- t/t3206-range-diff.sh | 27 +++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 6 deletions(-)
Documentation/git-format-patch.txt +4 -1
@@ -27,7 +27,7 @@ SYNOPSIS [--[no-]encode-email-headers] [--no-notes | --notes[=<ref>]] [--interdiff=<previous>] - [--range-diff=<previous> [--creation-factor=<percent>]] + [--range-diff=<previous> [--creation-factor=<percent>] [--left-only]] [--filename-max-length=<n>] [--progress] [<common diff options>]
@@ -301,6 +301,9 @@ material (this may change in the future). creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) for details. +--left-only: + Used with `--range-diff`, only emit output related to the first range. + --notes[=<ref>]:: --no-notes:: Append the notes (see linkgit:git-notes[1]) for the commit
builtin/log.c +15 -5
@@ -1153,7 +1153,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,struct commit *origin, int nr, struct commit **list, const char *branch_name, - int quiet) + int quiet, int left_only) { const char *committer;struct shortlog log;
@@ -1228,7 +1228,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, .creation_factor = rev->creation_factor, .dual_color = 1, .diffopt = &opts, - .other_arg = &other_arg + .other_arg = &other_arg, + .left_only = left_only };diff_setup (&opts);
@@ -1732,6 +1733,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)struct strbuf rdiff2 =STRBUF_INIT ;struct strbuf rdiff_title =STRBUF_INIT ;int creation_factor =-1 ; +int left_only =0 ;const struct option builtin_format_patch_options[] = { OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
@@ -1814,6 +1816,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) parse_opt_object_name),OPT_STRING (0 ,"range-diff" , &rdiff_prev,N_ ("refspec" ),N_ ("show changes against <refspec> in cover letter or single patch" )), +OPT_BOOL (0 ,"left-only" , &left_only, +N_ ("only emit output related to the first range" )),OPT_INTEGER (0 ,"creation-factor" , &creation_factor,N_ ("percentage by which creation is weighted" )),OPT_END ()
@@ -2083,10 +2087,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) _("Interdiff against v%d:")); } +if (!rdiff_prev) { +if (creation_factor >=0 ) +die (_ ("--creation-factor requires --range-diff" )); +if (left_only) +die (_ ("--left-only requires --range-diff" )); + } +if (creation_factor <0 ) creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ; -else if (!rdiff_prev) -die (_ ("--creation-factor requires --range-diff" ));if (rdiff_prev) {if (!cover_letter && total !=1 )
@@ -2134,7 +2143,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)if (thread)gen_message_id (&rev,"cover" );make_cover_letter (&rev, !!output_directory, - origin, nr, list, branch_name, quiet); + origin, nr, list, branch_name, quiet, + left_only);print_bases (&bases, rev.diffopt.file);print_signature (rev.diffopt.file); total++;
t/t3206-range-diff.sh +27 -0
@@ -748,4 +748,31 @@ test_expect_success '--left-only/--right-only' ' test_cmp expect actual ' +test_expect_success 'format-patch --range-diff --left-only' ' + rm -fr repo && + git init repo && + cd repo && + git branch -M main && + echo "base" >base && + git add base && + git commit -m "base" && + git checkout -b my-feature && + echo "feature" >feature && + git add feature && + git commit -m "feature" && + base="$(git rev-parse main)" && + old="$(git rev-parse my-feature)" && + git checkout main && + echo "other" >>base && + git add base && + git commit -m "new" && + git checkout my-feature && + git rebase $base --onto main && + tip="$(git rev-parse my-feature)" && + git format-patch --range-diff $base $old $tip --cover-letter && + grep "> 1: .* feature$" 0000-cover-letter.patch && + git format-patch --range-diff $base $old $tip --left-only --cover-letter && + ! grep "> 1: .* feature$" 0000-cover-letter.patch +' + test_done
base-commit: be7935ed8bff19f481b033d0d242c5d5f239ed50
Signature · 1 line
gitgitgadget
Re: [PATCH v3] [GSOC][RFC] format-patch: pass --left-only to range-diff
Quoted 7 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposinggit format-patch --cover-letter --range-diffmay mistakenly place upstream commit in the range-diff output. Teachformat-patchpass--left-onlyto range-diff,can avoid this kind of mistake.
The above is a bit too dense for average readers to grok. Even if the readers refer to the external reference, it is unclear where the "may mistakenly" can come from and why "--left-only" would be useful (and our log message should not depend on external material so heavily to begin with).
So let's think aloud to see what use case this may be helpful, and how the proposed solution makes the world a better place.
If I understand correctly, the use case this tries to help is this:
* You had sent the v1 iteration of topic. It was in the range B1..T1 where B1 is the tip of the integration branch (like 'master') from the upstream.
* To prepare for the v2 iteration, not only you updated individual commits, you rebased the series on a new upstream. Now the topic is in the range B2..T2, where B2 is the tip of the integration branch from the upstream, and it is very likely that B2 is a descendant of B1.
And you want to find out how your commits in T2 (new iteration) compares with those in T1 (old iteration). Normally,
$ git range-diff T1...T2
would be the shortest-to-type and correct version but that is invalidated because you rebased.
---o---B1--b---b---b---B2
\ \
t---t---T1 s---s---s---T2You'd have commits B1..T1 on the left hand side of the range-diff, while the right hand side has not just B2..T2 but also commits in the range B1..B2, too.
By using --left-only (i.e. show only those pair that maps from commits in the left range), you can exclude the commits in the B1..B2.
$ git range-diff --left-only T1...T2
I however wonder what --left-only (Suppress commits that are missing from the first range) would do to commits in range B2..T2 (they are all yours) that are (1) added since the v1 iteration, or (2) modified so drastically that no matching commit is found. With the right invocation, of course,
$ git range-diff B1..T1 B2..T2
you would not have such a problem. If 2 't's in B1..T1 correspond to 2 of the 3 's's in B2..T2, at least the presense of the third 's' that did not match would show up in the output, making it clear that you have one more commit relative to the earlier iteration. If use of --left-only filters it out, the output may be misleading to the readers, no?
I started writing (or "thinking aloud") hoping that I can help coming up with a better log message to describe the problem being solved, but I ended up with "does this make the system better?"
Re: [PATCH v3] [GSOC][RFC] format-patch: pass --left-only to range-diff
Quoted 11 lines · Junio C Hamano <[email protected]> 于2021年3月13日周六 上午6:51写道：
Quoted 7 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposinggit format-patch --cover-letter --range-diffmay mistakenly place upstream commit in the range-diff output. Teachformat-patchpass--left-onlyto range-diff,can avoid this kind of mistake.
Quoted 6 lines
The above is a bit too dense for average readers to grok. Even if the readers refer to the external reference, it is unclear where the "may mistakenly" can come from and why "--left-only" would be useful (and our log message should not depend on external material so heavily to begin with).
You are right, commit information with the original thread link may make it difficult for readers to read. I will pay attention.
Quoted 52 lines
So let's think aloud to see what use case this may be helpful, and how the proposed solution makes the world a better place.
If I understand correctly, the use case this tries to help is this:
* You had sent the v1 iteration of topic. It was in the range B1..T1 where B1 is the tip of the integration branch (like 'master') from the upstream.
* To prepare for the v2 iteration, not only you updated individual commits, you rebased the series on a new upstream. Now the topic is in the range B2..T2, where B2 is the tip of the integration branch from the upstream, and it is very likely that B2 is a descendant of B1.
And you want to find out how your commits in T2 (new iteration) compares with those in T1 (old iteration). Normally,
$ git range-diff T1...T2would be the shortest-to-type and correct version but that is invalidated because you rebased.
---o---B1--b---b---b---B2 \ \ t---t---T1 s---s---s---T2You'd have commits B1..T1 on the left hand side of the range-diff, while the right hand side has not just B2..T2 but also commits in the range B1..B2, too.
By using --left-only (i.e. show only those pair that maps from commits in the left range), you can exclude the commits in the B1..B2.
$ git range-diff --left-only T1...T2I however wonder what --left-only (Suppress commits that are missing from the first range) would do to commits in range B2..T2 (they are all yours) that are (1) added since the v1 iteration, or (2) modified so drastically that no matching commit is found. With the right invocation, of course,
$ git range-diff B1..T1 B2..T2you would not have such a problem. If 2 't's in B1..T1 correspond to 2 of the 3 's's in B2..T2, at least the presense of the third 's' that did not match would show up in the output, making it clear that you have one more commit relative to the earlier iteration. If use of --left-only filters it out, the output may be misleading to the readers, no?
Quoted 3 lines
I started writing (or "thinking aloud") hoping that I can help coming up with a better log message to describe the problem being solved, but I ended up with "does this make the system better?"
Junio, thank you for elaborating this issue in detail and clearly. I probably understand what you mean by "git range-diff B1..T1 B2..T2"
to correctly output the commits on my two version topic branch, without
including the upstream commits of B1..B2.So we don’t even need to specify the --left-only to avoid the output of B1...B2, right?
The only thing I can think of now is that if users tend to use T1...T2 to compare
the differences between the two topics, will the upstream commit in
B1...B2 appear
more abrupt?
Thanks.
Re: [PATCH v3] [GSOC][RFC] format-patch: pass --left-only to range-diff
Quoted 84 lines · ZheNing Hu
Quoted 11 lines · Junio C Hamano <[email protected]> 于2021年3月13日周六 上午6:51写道：
Quoted 7 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
In https://lore.kernel.org/git/[email protected]/,
Taylor Blau proposinggit format-patch --cover-letter --range-diffmay mistakenly place upstream commit in the range-diff output. Teachformat-patchpass--left-onlyto range-diff,can avoid this kind of mistake.Quoted 6 lines
The above is a bit too dense for average readers to grok. Even if the readers refer to the external reference, it is unclear where the "may mistakenly" can come from and why "--left-only" would be useful (and our log message should not depend on external material so heavily to begin with).
You are right, commit information with the original thread link may make it difficult for readers to read. I will pay attention.
Quoted 52 lines
So let's think aloud to see what use case this may be helpful, and how the proposed solution makes the world a better place.
If I understand correctly, the use case this tries to help is this:
* You had sent the v1 iteration of topic. It was in the range B1..T1 where B1 is the tip of the integration branch (like 'master') from the upstream.
* To prepare for the v2 iteration, not only you updated individual commits, you rebased the series on a new upstream. Now the topic is in the range B2..T2, where B2 is the tip of the integration branch from the upstream, and it is very likely that B2 is a descendant of B1.
And you want to find out how your commits in T2 (new iteration) compares with those in T1 (old iteration). Normally,
$ git range-diff T1...T2would be the shortest-to-type and correct version but that is invalidated because you rebased.
---o---B1--b---b---b---B2 \ \ t---t---T1 s---s---s---T2You'd have commits B1..T1 on the left hand side of the range-diff, while the right hand side has not just B2..T2 but also commits in the range B1..B2, too.
By using --left-only (i.e. show only those pair that maps from commits in the left range), you can exclude the commits in the B1..B2.
$ git range-diff --left-only T1...T2I however wonder what --left-only (Suppress commits that are missing from the first range) would do to commits in range B2..T2 (they are all yours) that are (1) added since the v1 iteration, or (2) modified so drastically that no matching commit is found. With the right invocation, of course,
$ git range-diff B1..T1 B2..T2you would not have such a problem. If 2 't's in B1..T1 correspond to 2 of the 3 's's in B2..T2, at least the presense of the third 's' that did not match would show up in the output, making it clear that you have one more commit relative to the earlier iteration. If use of --left-only filters it out, the output may be misleading to the readers, no?
Quoted 3 lines
I started writing (or "thinking aloud") hoping that I can help coming up with a better log message to describe the problem being solved, but I ended up with "does this make the system better?"
Junio, thank you for elaborating this issue in detail and clearly. I probably understand what you mean by "git range-diff B1..T1 B2..T2"
to correctly output the commits on my two version topic branch, without
including the upstream commits of B1..B2.So we don’t even need to specify the--left-onlyto avoid the output of B1...B2, right?
Quoted 5 lines
The only thing I can think of now is that if users tend to use T1...T2 to compare
the differences between the two topics, will the upstream commit in
B1...B2 appear
more abrupt?
Yes, it would be, but that is why you need to educate users what causes it, and what the right way to avoid unrelated commits from appearing, and how this --left-only fits in the solution.
If some of the time, "--left-only T1...T2" would give you the same result as the more strict "B1..T1 B2..T2", that may be why users may want to use the "--left-only" instead as an easy/lazy alternative.
But I suspect that it would give an incorrect result some of the time---for example, in the above example, wouldn't one of the commits labeled as 's' be completely hidden? And if that is the case, the end-user documentation would need to warn about it, and explain that it is a easy/lazy alternative that can produce incorrect result in the log message.
Re: [PATCH v3] [GSOC][RFC] format-patch: pass --left-only to range-diff
Quoted 9 lines · Junio C Hamano <[email protected]> 于2021年3月14日周日 上午7:23写道：
Yes, it would be, but that is why you need to educate users what causes it, and what the right way to avoid unrelated commits from appearing, and how this --left-only fits in the solution.
If some of the time, "--left-only T1...T2" would give you the same result as the more strict "B1..T1 B2..T2", that may be why users may want to use the "--left-only" instead as an easy/lazy alternative.
Quoted 6 lines
But I suspect that it would give an incorrect result some of the time---for example, in the above example, wouldn't one of the commits labeled as 's' be completely hidden? And if that is the case, the end-user documentation would need to warn about it, and explain that it is a easy/lazy alternative that can produce incorrect result in the log message.
Thanks, I will try to illustrate these issues in the document.
My another thinking is:
Since --left-only inhibits "B1..B2" and "B2..T2" ( let the user choose the left B1..T1 ), To some extent, --right-only can also add ( let the user choose the right B2..T2 ). A separate --left-only will be strange to the user ( If user call T2...T1 ). Since the git rebase --apply will internal call git format-patch -k --stdout --full-index --cherry-pick --right-only ..., I don't know what to deal with this --right-only yet, because I don't how to teach git to judge if the --right-only is pass from user or git rebase --apply, Is there any good way? If can solve this problem, the user can choose the left or right side of the free choice. (Or users don't need --right-only?)
Re: [PATCH v3] [GSOC][RFC] format-patch: pass --left-only to range-diff
Quoted 21 lines · ZheNing Hu <[email protected]> 于2021年3月14日周日 上午10:16写道：
Quoted 9 lines · Junio C Hamano <[email protected]> 于2021年3月14日周日 上午7:23写道：
Yes, it would be, but that is why you need to educate users what causes it, and what the right way to avoid unrelated commits from appearing, and how this --left-only fits in the solution.
If some of the time, "--left-only T1...T2" would give you the same result as the more strict "B1..T1 B2..T2", that may be why users may want to use the "--left-only" instead as an easy/lazy alternative.
Quoted 6 lines
But I suspect that it would give an incorrect result some of the time---for example, in the above example, wouldn't one of the commits labeled as 's' be completely hidden? And if that is the case, the end-user documentation would need to warn about it, and explain that it is a easy/lazy alternative that can produce incorrect result in the log message.
Thanks, I will try to illustrate these issues in the document.
Quoted 11 lines
My another thinking is:
Since--left-onlyinhibits "B1..B2" and "B2..T2" ( let the user choose the left B1..T1 ), To some extent,--right-onlycan also add ( let the user choose the right B2..T2 ). A separate--left-onlywill be strange to the user ( If user call T2...T1 ). Since thegit rebase --applywill internal callgit format-patch -k --stdout --full-index --cherry-pick --right-only ..., I don't know what to deal with this--right-onlyyet, because I don't how to teach git to judge if the--right-onlyis pass from user orgit rebase --apply, Is there any good way? If can solve this problem, the user can choose the left or right side of the free choice. (Or users don't need--right-only?)
Let me refute my own point just now :--right-only can not show "B2..T2", but "B1..T1", and it may be useful only when user want an inverted "T2...T1".
Re: [PATCH v3] [GSOC][RFC] format-patch: pass --left-only to range-diff
Quoted 2 lines
Let me refute my own point just now :
--right-onlycan not show "B2..T2", but "B1..T1", and it may be useful only when user want an inverted "T2...T1".
sorry,
s/B1..T1/B1..T2/
PATCH v4 1/1 format-patch: pass --left-only to range-diff
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
When two different iterative versions t1 and t2 are base upstream commit b1 and b2, b2 inherits from b1. If the user base on b1 to generate cover-letter, but git format-patch --range-diff=b1..t1 b1..t2 --cover-letter may mistakenly place upstream commit in the range-diff output in cover-letter.
Teaching format-patch pass --left-only to range-diff, it will suppress the output on the upstream commits.At the same time, it has a disadvantage: the iterative version on the right side will be ignored too. So using --left-only is just a lazy way to avoid upstream output.
---
[GSOC][RFC] format-patch: pass --left-only to range-diff
With the help of Taylor Blau and Junio, I understand why the upstream commit appeared in the range-diff, and completed the writing of the test.
But I notice that in https://github.com/gitgitgadget/git/issues/876 and
https://lore.kernel.org/git/[email protected]/ Both Junio and Johannes Schindelin have questions about the practicality of this --left-only, and I am also starting to have confusion now, and format-patch --range-diff is more suitable for two commit ranges to compare the differences. It is not suitable for T1...T2, which further proves that the practicability of this patch may not be as good as previously imagined. Should I close it?
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-898%2Fadlternative%2Fformat-patch-range-diff-right-only-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-898/adlternative/format-patch-range-diff-right-only-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/898
Range-diff vs v3:
1: 5c58eb186d41 ! 1: 3738a00129b5 [GSOC][RFC] format-patch: pass --left-only to range-diff
@@ Metadata
Author: ZheNing Hu <[email protected]>
## Commit message ##
- [GSOC][RFC] format-patch: pass --left-only to range-diff
+ [GSOC] format-patch: pass --left-only to range-diff
- In https://lore.kernel.org/git/[email protected]/,
- Taylor Blau proposing `git format-patch --cover-letter
- --range-diff` may mistakenly place upstream commit in the
- range-diff output. Teach format-patch pass --left-only
- to range-diff,can avoid this kind of mistake.
+ When two different iterative versions t1 and t2 are base upstream
+ commit b1 and b2, b2 inherits from b1. If the user base on b1 to
+ generate cover-letter, but `git format-patch --range-diff=b1..t1
+ b1..t2 --cover-letter` may mistakenly place upstream commit in the
+ range-diff output in cover-letter.
+
+ Teaching format-patch pass --left-only to range-diff, it will
+ suppress the output on the upstream commits.At the same time, it has
+ a disadvantage: the iterative version on the right side will be ignored
+ too. So using --left-only is just a lazy way to avoid upstream output.
@@ Documentation/git-format-patch.txt: material (this may change in the future).
+--left-only:
+ Used with --range-diff, only emit output related to the first range.
++ This option only can be used in this situation: The first iteration t1
++ based on the upstream commit b1, the second iteration t2 based on the
++ upstream commit b2, b2 inherited from the b1, then If the user want
++ output the range-diff between this two iterations (t1 and t2) in cover-letter,
++ and just use b1 as the same base for --range-diff, but they don't want the
++ upstream range b1..b2 included on the right side of the range-diff output.
++ By using git format-patch --range-diff=b1..t1 b1..t2 --cover-letter --left-only,
++ all content on the right side will be removed, leaving only the range
++ b1..t1 on the left side.
+++
++Note that this --left-only is just a lazy way to let user use same base ++and avoid outputting upstream commits in cover-letter, and the side effect ++is that b2..t2 on the right side will not be outputted.
++
+
--notes[=<ref>]::
--no-notes::
@@ t/t3206-range-diff.sh: test_expect_success '--left-only/--right-only' '
+ git checkout my-feature &&
+ git rebase $base --onto main &&
+ tip="$(git rev-parse my-feature)" &&
-+ git format-patch --range-diff $base $old $tip --cover-letter &&
-+ grep "> 1: .* feature$" 0000-cover-letter.patch &&
-+ git format-patch --range-diff $base $old $tip --left-only --cover-letter &&
++ git format-patch --range-diff $base..$old $base..$tip --cover-letter &&
++ grep "> 1: .* new$" 0000-cover-letter.patch &&
++ git format-patch --range-diff $base..$old $base..$tip --left-only --cover-letter &&
+ ! grep "> 1: .* feature$" 0000-cover-letter.patch
+'
+
Documentation/git-format-patch.txt | 19 ++++++++++++++++++- builtin/log.c | 20 +++++++++++++++----- t/t3206-range-diff.sh | 27 +++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 6 deletions(-)
Documentation/git-format-patch.txt +18 -1
@@ -27,7 +27,7 @@ SYNOPSIS [--[no-]encode-email-headers] [--no-notes | --notes[=<ref>]] [--interdiff=<previous>] - [--range-diff=<previous> [--creation-factor=<percent>]] + [--range-diff=<previous> [--creation-factor=<percent>] [--left-only]] [--filename-max-length=<n>] [--progress] [<common diff options>]
@@ -301,6 +301,23 @@ material (this may change in the future). creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) for details. +--left-only: + Used with `--range-diff`, only emit output related to the first range. + This option only can be used in this situation: The first iteration `t1` + based on the upstream commit `b1`, the second iteration `t2` based on the + upstream commit `b2`, `b2` inherited from the `b1`, then If the user want + output the range-diff between this two iterations (t1 and t2) in cover-letter, + and just use `b1` as the same base for `--range-diff`, but they don't want the + upstream range `b1..b2` included on the right side of the range-diff output. + By using `git format-patch --range-diff=b1..t1 b1..t2 --cover-letter --left-only`, + all content on the right side will be removed, leaving only the range + `b1..t1` on the left side. ++ +Note that this `--left-only` is just a lazy way to let user use same base +and avoid outputting upstream commits in cover-letter, and the side effect +is that `b2..t2` on the right side will not be outputted. + + --notes[=<ref>]:: --no-notes:: Append the notes (see linkgit:git-notes[1]) for the commit
builtin/log.c +15 -5
@@ -1153,7 +1153,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,struct commit *origin, int nr, struct commit **list, const char *branch_name, - int quiet) + int quiet, int left_only) { const char *committer;struct shortlog log;
@@ -1228,7 +1228,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, .creation_factor = rev->creation_factor, .dual_color = 1, .diffopt = &opts, - .other_arg = &other_arg + .other_arg = &other_arg, + .left_only = left_only };diff_setup (&opts);
@@ -1732,6 +1733,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)struct strbuf rdiff2 =STRBUF_INIT ;struct strbuf rdiff_title =STRBUF_INIT ;int creation_factor =-1 ; +int left_only =0 ;const struct option builtin_format_patch_options[] = { OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
@@ -1814,6 +1816,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) parse_opt_object_name),OPT_STRING (0 ,"range-diff" , &rdiff_prev,N_ ("refspec" ),N_ ("show changes against <refspec> in cover letter or single patch" )), +OPT_BOOL (0 ,"left-only" , &left_only, +N_ ("only emit output related to the first range" )),OPT_INTEGER (0 ,"creation-factor" , &creation_factor,N_ ("percentage by which creation is weighted" )),OPT_END ()
@@ -2083,10 +2087,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) _("Interdiff against v%d:")); } +if (!rdiff_prev) { +if (creation_factor >=0 ) +die (_ ("--creation-factor requires --range-diff" )); +if (left_only) +die (_ ("--left-only requires --range-diff" )); + } +if (creation_factor <0 ) creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ; -else if (!rdiff_prev) -die (_ ("--creation-factor requires --range-diff" ));if (rdiff_prev) {if (!cover_letter && total !=1 )
@@ -2134,7 +2143,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)if (thread)gen_message_id (&rev,"cover" );make_cover_letter (&rev, !!output_directory, - origin, nr, list, branch_name, quiet); + origin, nr, list, branch_name, quiet, + left_only);print_bases (&bases, rev.diffopt.file);print_signature (rev.diffopt.file); total++;
t/t3206-range-diff.sh +27 -0
@@ -748,4 +748,31 @@ test_expect_success '--left-only/--right-only' ' test_cmp expect actual ' +test_expect_success 'format-patch --range-diff --left-only' ' + rm -fr repo && + git init repo && + cd repo && + git branch -M main && + echo "base" >base && + git add base && + git commit -m "base" && + git checkout -b my-feature && + echo "feature" >feature && + git add feature && + git commit -m "feature" && + base="$(git rev-parse main)" && + old="$(git rev-parse my-feature)" && + git checkout main && + echo "other" >>base && + git add base && + git commit -m "new" && + git checkout my-feature && + git rebase $base --onto main && + tip="$(git rev-parse my-feature)" && + git format-patch --range-diff $base..$old $base..$tip --cover-letter && + grep "> 1: .* new$" 0000-cover-letter.patch && + git format-patch --range-diff $base..$old $base..$tip --left-only --cover-letter && + ! grep "> 1: .* feature$" 0000-cover-letter.patch +' + test_done
base-commit: be7935ed8bff19f481b033d0d242c5d5f239ed50
Signature · 1 line
gitgitgadget