From: ZheNing Hu via GitGitGadget <hidden> Date: 2021-03-07 07:05:44
From: ZheNing Hu <redacted>
In https://lore.kernel.org/git/YBx5rmVsg1LJhSKN@nand.local/,
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.
Signed-off-by: ZheNing Hu <redacted>
---
[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(-)
@@ -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
@@ -1732,7 +1733,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)structstrbufrdiff2=STRBUF_INIT;structstrbufrdiff_title=STRBUF_INIT;intcreation_factor=-1;-+intright_only=0;conststructoptionbuiltin_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;elseif(!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"));
From: Taylor Blau <hidden> Date: 2021-03-08 20:19:07
On Sun, Mar 07, 2021 at 06:57:19AM +0000, ZheNing Hu via GitGitGadget wrote:
From: ZheNing Hu <redacted>
In https://lore.kernel.org/git/YBx5rmVsg1LJhSKN@nand.local/,
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.
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...
Signed-off-by: ZheNing Hu <redacted>
---
[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.
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
@@ -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 hunk
--notes[=<ref>]::
--no-notes::
Append the notes (see linkgit:git-notes[1]) for the commit
Nit: the newline between creation_factor's declaration and
builtin_format_patch_options's is good to keep.
quoted hunk
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"));
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
From: ZheNing Hu <hidden> Date: 2021-03-09 07:29:33
Taylor Blau [off-list ref] 于2021年3月9日周二 上午4:18写道:
On Sun, Mar 07, 2021 at 06:57:19AM +0000, ZheNing Hu via GitGitGadget wrote:
quoted
From: ZheNing Hu <redacted>
In https://lore.kernel.org/git/YBx5rmVsg1LJhSKN@nand.local/,
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.
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
Signed-off-by: ZheNing Hu <redacted>
---
[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.
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
Thanks, I have seen an obvious phenomenon from
your shell script, and I wrote test code based on it.
@@ -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?
Now it's all "--left-only".
quoted
--notes[=<ref>]::
--no-notes::
Append the notes (see linkgit:git-notes[1]) for the commit
Nit: the newline between creation_factor's declaration and
builtin_format_patch_options's is good to keep.
quoted
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"));
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.
if (!rdiff_prev) {
if (creation_factor)
die(_("--creation-factor requires --range-diff"));
if (right_only)
die(_("--right-only requires --range-diff"));
}
Thanks,
Taylor
@@ -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
@@ -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;-elseif(!rdiff_prev)-die(_("--creation-factor requires --range-diff"));-+elseif(!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"));
From: Eric Sunshine <hidden> Date: 2021-03-09 09:01:22
On Tue, Mar 9, 2021 at 3:35 AM ZheNing Hu via GitGitGadget
[off-list ref] wrote:
quoted hunk
In https://lore.kernel.org/git/YBx5rmVsg1LJhSKN@nand.local/,
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 <redacted>
---
diff --git a/builtin/log.c b/builtin/log.c
@@ -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"));+ }
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.
From: ZheNing Hu <hidden> Date: 2021-03-09 09:36:22
Eric Sunshine [off-list ref] 于2021年3月9日周二 下午5:00写道:
On Tue, Mar 9, 2021 at 3:35 AM ZheNing Hu via GitGitGadget
[off-list ref] wrote:
quoted
In https://lore.kernel.org/git/YBx5rmVsg1LJhSKN@nand.local/,
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 <redacted>
---
diff --git a/builtin/log.c b/builtin/log.c
@@ -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"));+ }
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;
@@ -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
@@ -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()
From: ZheNing Hu via GitGitGadget <hidden> Date: 2021-03-14 08:11:48
From: ZheNing Hu <redacted>
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.
Signed-off-by: ZheNing Hu <redacted>
---
[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/xmqqpn0456lr.fsf@gitster.g/ 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 [off-list ref]
## 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/YBx5rmVsg1LJhSKN@nand.local/,
- 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.
Signed-off-by: ZheNing Hu [off-list ref]
@@ 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(-)
@@ -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
@@ -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()