From: Eric Sunshine <hidden> Date: 2021-02-03 05:06:39
On Tue, Feb 2, 2021 at 10:29 AM Charvi Mendiratta [off-list ref] wrote:
On Tue, 2 Feb 2021 at 06:17, Eric Sunshine [off-list ref] wrote:
quoted
When `-c` says "edit the commit message" it's not clear what will be
edited. The original's commit message? The replacement's message? A
combination of the two? If you can come up with a succinct way to word
it such that it states more precisely what exactly will be edited, it
would be nice, but not necessarily worth a re-roll.
Here the editor shows the commented out commit message of original commit and
the replacement commit message (of fixup -c commit) is not commented out.
So maybe s/edit the commit message/edit this commit message is better.
Yes, that would be clearer and is just as succinct.
quoted
This code adds to the confusion. In the function argument list, `flag`
has been declared as a single enum item, yet this code is treating
`flag` as if it is a combination of bits. So, it's not clear what the
intention is here. Is `flag` always going to be a specific enum item
in this context or is it going to be a combination of bits? If it is
only ever going to be a distinct enum item, then one would expect this
code to be written like this:
return command == TODO_FIXUP &&
(flag == TODO_REPLACE_FIXUP_MSG ||
flag == TODO_EDIT_FIXUP_MSG);
I admit it resulted in a bit of confusion. Here, its true that flag is always
going to be specific enum item( as command can be merge -c, fixup -c, or
fixup -C ) and I combined the bag of bits to denote
the specific enum item. So, maybe we can go with the first method?
Sounds fine. It would clarify the intent.
quoted
By the way, the function name check_fixup_flag() doesn't necessarily
do a good job conveying the purpose of this function. Based upon the
implementation, I gather that it is checking whether the command is a
"fixup" command, so perhaps the name could reflect that. Perhaps
is_fixup() or something?
Agree, here it's checking if the command is fixup and the flag value (
which implies either user has given command fixup -c or fixup -C )
So, I wonder if we can write is_fixup_flag() ?
Reasonable.
quoted
I was wondering if the above could be rephrased like this to avoid the
repetition:
[...]
but perhaps it's too magical and ugly.
I agree, this [tolower(bol[1]) == 'c'] is actually doing all the
magic, but I am not
sure if we should change it or not ? As in the source code just after
this code we
are checking in a similar way for the 'merge' command. So, maybe implementing
in a similar way is easier to read ?
From: Eric Sunshine <hidden> Date: 2021-02-03 05:45:38
On Tue, Feb 2, 2021 at 5:02 AM Christian Couder
[off-list ref] wrote:
On Tue, Feb 2, 2021 at 3:01 AM Eric Sunshine [off-list ref] wrote:
quoted
On Fri, Jan 29, 2021 at 1:25 PM Charvi Mendiratta [off-list ref] wrote:
quoted
+ merge_*|fixup_*)
+ action=$(echo "$line" | sed 's/_/ /g');;
What is "merge_" doing here? It doesn't seem to be used by this patch.
Yeah, it's not used, but it might be a good thing to add this for
consistency while at it.
It confuses readers (as it did to me), causing them to waste
brain-cycles trying to figure out why it's present. Thus, it would be
better to add it when it's actually needed. The waste of brain-cycles
and time is especially important on a project like Git for which
reviewers and reviewer time are limited resources.
quoted
quoted
+# Copyright (c) 2018 Phillip Wood
Did Phillip write this script? Is this patch based upon an old patch from him?
Yeah, it might be a good idea to add a "Based-on-patch-by: Phillip ..."
Agreed.
quoted
The implementation of test_commit_message() is a bit hard to follow.
It might be simpler to write it more concisely and directly like this:
git show --no-patch --pretty=format:%B "$1" >actual &&
case "$2" in
-m) echo "$3" >expect && test_cmp expect actual ;;
I think we try to avoid many commands on the same line.
For something this minor, it's not likely to matter but, of course, it
could be split over two lines:
-m) echo "$3" >expect &&
test_cmp expect actual ;;
quoted
*) test_cmp "$2" actual ;;
esac
In general I am not sure that using $1, $2, $3 directly makes things
easier to understand, but yeah, with the function documentation that
you suggest, it might be better to write the function using them
directly.
The direct $1, $2, etc. was just an example. It's certainly possible
to give them names even in the rewritten code I presented. One good
reason, however, for just using $1, $2, etc. is that $2 is not well
defined; sometimes it's a switch ("-m") and sometimes its a pathname,
so it's hard to invent a suitable variable name for it. Also, this
function becomes so simple (in the rewritten version) that explicit
variable names don't add a lot of value (the cognitive load is quite
low because the function is so short).
quoted
Style nit: In Git test scripts, the here-doc body and EOF are indented
the same amount as the command which opened the here-doc:
I don't think we are very consistent with this and I didn't find
anything about this in CodingGuidelines.
In t0008 and t0021 for example, the indentation is more like:
cat >message <<-EOF &&
amend! B
...
body
EOF
and I like this style, as it seems clearer than the other styles.
I performed a quick survey of the heredoc styles in the tests. Here
are the results[1] of my analysis on the 'seen' branch:
total-heredocs=4128
same-indent=3053 (<<EOF & body & EOF share indent)
cat >expect <<-\EOF
body
EOF
body-eof-indented=24 (body & EOF indented)
cat >expect <<-\EOF
body
EOF
body-indented=735 (body indented; EOF not)
cat >expect <<-\EOF
body
EOF
left-margin=316 (<<EOF indented; body & EOF not)
cat >expect <<\EOF
body
EOF
So, the indentation recommended in my review -- with 3053 instances
out of 4128 heredocs -- is by far the most prevalent in the project.
[1]: Note that there is a miniscule amount of inaccuracy in the
numbers because there are a few cases in which heredocs contain other
heredocs, and some scripts build heredocs piecemeal when constructing
other scripts, and I didn't bother making my analysis script handle
those few cases. The inaccuracy is tiny, thus not meaningful to the
overall picture.
quoted
I see that you mirrored the implementation of FAKE_LINES handling of
"exec" here for "fixup", but the cases are quite different. The
argument to "exec" is arbitrary and can have any number of spaces
embedded in it, which conflicts with the meaning of spaces in
FAKE_LINES, which separate the individual commands in FAKE_LINES.
Consequently, "_" was chosen as a placeholder in "exec" to mean
"space".
However, "fixup" is a very different beast. Its arguments are not
arbitrary at all, so there isn't a good reason to mirror the choice of
"_" to represent a space, which leads to rather unsightly tokens such
as "fixup_-C". It would work just as well to use simpler tokens such
as "fixup-C" and "fixup-c", in which case t/lib-rebase.sh might parse
them like this (note that I also dropped `g` from the `sed` action):
fixup-*)
action=$(echo "$line" | sed 's/-/ -/');;
I agree that "fixup" arguments are not arbitrary at all, but I think
it makes things simpler to just use one way to encode spaces instead
of many different ways.
Is that the intention here, though? Is the idea that some day `fixup`
will accept arbitrary arguments thus needs to encode spaces? If not,
then mirroring the treatment given to `exec` confuses readers into
thinking that it will/should accept arbitrary arguments. I brought
this up in my review specifically because it was confusing to a person
(me) new to this topic and reading the patches for the first time. The
more specific and exact the code can be, the less likely it will
confuse readers in the future.
Anyhow, it's a minor point, not worth expending a lot of time discussing.
quoted
It feels clunky and fragile for this test to be changing
"expected-message" which was created in the "setup" test and used
unaltered up to this point. If the content of "expected-message" is
really going to change from test to test (as I see it changes again in
a later test), then it would be easier to reason about the behavior if
each test gives "expected-message" the precise content it should have
in that local context. As it is currently implemented, it's too
difficult to follow along and remember the value of "expected-message"
from test to test. It also makes it difficult to extend tests or add
new tests in between existing tests without negatively impacting other
tests. If each test sets up "expected-message" to the precise content
needed by the test, then both those problems go away.
Yeah, perhaps the global "expected-message" could be renamed for
example "global-expected-message", and tests which need a specific one
could prepare and use a custom "expected-message" (maybe named
"custom-expected-message") without ever changing
"global-expected-message".
That would be fine, though I wondered while reviewing the patch if a
global "expect-message" file was even needed since it didn't seem like
very many tests used it (but I didn't spend a lot of time counting the
exact number of tests due to the high cognitive load tracing how that
file might mutate as it passed through each test).
Another really good reason for avoiding having later tests depend upon
mutations from earlier tests, if possible, is that it makes it easier
to run tests selectively with --run or GIT_SKIP_TESTS.
I admit it resulted in a bit of confusion. Here, its true that flag is always
going to be specific enum item( as command can be merge -c, fixup -c, or
fixup -C ) and I combined the bag of bits to denote
the specific enum item. So, maybe we can go with the first method?
Sounds fine. It would clarify the intent.
(Apology for confusion) After, looking again at the source code, as we are using
the flag element of the structure todo_item of in sequencer.h. So, I
think right
way is to let it be in binary only and change type from 'enum todo_item_flag' to
'unsigned' , as you suggested below (better than first method) :
Otherwise, if `flag` will actually be a bag of bits, then the argument
should be declared as such:
static int check_fixup_flag(enum todo_command command,
unsigned flag)
quoted
Agree, here it's checking if the command is fixup and the flag value (
which implies either user has given command fixup -c or fixup -C )
So, I wonder if we can write is_fixup_flag() ?
Reasonable.
[...]
quoted
I agree, this [tolower(bol[1]) == 'c'] is actually doing all the
magic, but I am not
sure if we should change it or not ? As in the source code just after
this code we
are checking in a similar way for the 'merge' command. So, maybe implementing
in a similar way is easier to read ?
On Wed, 3 Feb 2021 at 11:14, Eric Sunshine [off-list ref] wrote:
quoted
quoted
What is "merge_" doing here? It doesn't seem to be used by this patch.
Yeah, it's not used, but it might be a good thing to add this for
consistency while at it.
It confuses readers (as it did to me), causing them to waste
brain-cycles trying to figure out why it's present. Thus, it would be
better to add it when it's actually needed. The waste of brain-cycles
and time is especially important on a project like Git for which
reviewers and reviewer time are limited resources.
Okay, I will remove "merge_" from this patch series and maybe later will make
separate patch for it and also adding its tests and updating
t3430-rebase-merges.sh
quoted
quoted
quoted
+# Copyright (c) 2018 Phillip Wood
Did Phillip write this script? Is this patch based upon an old patch from him?
Yeah, it might be a good idea to add a "Based-on-patch-by: Phillip ..."
Agreed.
quoted
quoted
The implementation of test_commit_message() is a bit hard to follow.
It might be simpler to write it more concisely and directly like this:
git show --no-patch --pretty=format:%B "$1" >actual &&
case "$2" in
-m) echo "$3" >expect && test_cmp expect actual ;;
I think we try to avoid many commands on the same line.
For something this minor, it's not likely to matter but, of course, it
could be split over two lines:
-m) echo "$3" >expect &&
test_cmp expect actual ;;
quoted
quoted
*) test_cmp "$2" actual ;;
esac
In general I am not sure that using $1, $2, $3 directly makes things
easier to understand, but yeah, with the function documentation that
you suggest, it might be better to write the function using them
directly.
The direct $1, $2, etc. was just an example. It's certainly possible
to give them names even in the rewritten code I presented. One good
reason, however, for just using $1, $2, etc. is that $2 is not well
defined; sometimes it's a switch ("-m") and sometimes its a pathname,
so it's hard to invent a suitable variable name for it. Also, this
function becomes so simple (in the rewritten version) that explicit
variable names don't add a lot of value (the cognitive load is quite
low because the function is so short).
Agree, and will update it.
quoted
quoted
Style nit: In Git test scripts, the here-doc body and EOF are indented
the same amount as the command which opened the here-doc:
I don't think we are very consistent with this and I didn't find
anything about this in CodingGuidelines.
In t0008 and t0021 for example, the indentation is more like:
cat >message <<-EOF &&
amend! B
...
body
EOF
and I like this style, as it seems clearer than the other styles.
I performed a quick survey of the heredoc styles in the tests. Here
are the results[1] of my analysis on the 'seen' branch:
total-heredocs=4128
same-indent=3053 (<<EOF & body & EOF share indent)
cat >expect <<-\EOF
body
EOF
body-eof-indented=24 (body & EOF indented)
cat >expect <<-\EOF
body
EOF
body-indented=735 (body indented; EOF not)
cat >expect <<-\EOF
body
EOF
left-margin=316 (<<EOF indented; body & EOF not)
cat >expect <<\EOF
body
EOF
So, the indentation recommended in my review -- with 3053 instances
out of 4128 heredocs -- is by far the most prevalent in the project.
[1]: Note that there is a miniscule amount of inaccuracy in the
numbers because there are a few cases in which heredocs contain other
heredocs, and some scripts build heredocs piecemeal when constructing
other scripts, and I didn't bother making my analysis script handle
those few cases. The inaccuracy is tiny, thus not meaningful to the
overall picture.
Okay, will update the indentation.
[...]
quoted
quoted
fixup-*)
action=$(echo "$line" | sed 's/-/ -/');;
I agree that "fixup" arguments are not arbitrary at all, but I think
it makes things simpler to just use one way to encode spaces instead
of many different ways.
Is that the intention here, though? Is the idea that some day `fixup`
will accept arbitrary arguments thus needs to encode spaces? If not,
then mirroring the treatment given to `exec` confuses readers into
thinking that it will/should accept arbitrary arguments. I brought
this up in my review specifically because it was confusing to a person
(me) new to this topic and reading the patches for the first time. The
more specific and exact the code can be, the less likely it will
confuse readers in the future.
I also agree that fixup will not accept arbitrary arguments, So I think to
go with the method using fixup-*) (as suggested above).
[...]
quoted
Yeah, perhaps the global "expected-message" could be renamed for
example "global-expected-message", and tests which need a specific one
could prepare and use a custom "expected-message" (maybe named
"custom-expected-message") without ever changing
"global-expected-message".
That would be fine, though I wondered while reviewing the patch if a
global "expect-message" file was even needed since it didn't seem like
very many tests used it (but I didn't spend a lot of time counting the
exact number of tests due to the high cognitive load tracing how that
file might mutate as it passed through each test).
Another really good reason for avoiding having later tests depend upon
mutations from earlier tests, if possible, is that it makes it easier
to run tests selectively with --run or GIT_SKIP_TESTS.
Agree, also for this patch series I think to remove all tests for
amend!, change the
test setup and will take care this time to remove the test dependency
(in case of expected-message).
Thanks and Regards,
Charvi
From: Eric Sunshine <hidden> Date: 2021-02-04 00:15:47
On Wed, Feb 3, 2021 at 7:01 PM Charvi Mendiratta [off-list ref] wrote:
quoted
quoted
I admit it resulted in a bit of confusion. Here, its true that flag is always
going to be specific enum item( as command can be merge -c, fixup -c, or
fixup -C ) and I combined the bag of bits to denote
the specific enum item. So, maybe we can go with the first method?
Sounds fine. It would clarify the intent.
(Apology for confusion) After, looking again at the source code, as we are using
the flag element of the structure todo_item of in sequencer.h. So, I
think right
way is to let it be in binary only and change type from 'enum todo_item_flag' to
'unsigned' , as you suggested below (better than first method) :
Hi Eric
Thanks for taking such a close look at this series.
On 02/02/2021 02:01, Eric Sunshine wrote:
On Fri, Jan 29, 2021 at 1:25 PM Charvi Mendiratta [off-list ref] wrote:
[...]
quoted
+ ORIG_AUTHOR_NAME="$GIT_AUTHOR_NAME" &&+ ORIG_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" &&+ GIT_AUTHOR_NAME="Amend Author" &&+ GIT_AUTHOR_EMAIL="amend@example.com" &&+ test_commit "$(cat message)" A A1 A1 &&+ test_commit A2 A &&+ test_commit A3 A &&+ GIT_AUTHOR_NAME="$ORIG_AUTHOR_NAME" &&+ GIT_AUTHOR_EMAIL="$ORIG_AUTHOR_EMAIL" &&
Are the timestamps of these commits meaningful in this context?
I think we want to ensure that the timestamp of the commits created with the different author are different from the previous commits. We ought to be checking that the author date of the rebased commit matches the author date of the original commit and not the author date of the fixup commit created with the different author.
If not, another way to do this would be to assign the new author
name/email values in a subshell so that the values do not need to be
restored manually. For instance:
(
GIT_AUTHOR_NAME="Amend Author" &&
GIT_AUTHOR_EMAIL="amend@example.com" &&
test_commit "$(cat message)" A A1 A1 &&
test_commit A2 A &&
test_commit A3 A
) &&
It's a matter of taste whether or not that is preferable, though.
Same question about whether the commit timestamps have any
significance in these tests.
Same answer as above - we want different author dates so we can check the original author date is not modified.
I'm not sure that we are actually checking the dates yet though it looks like we only check the author name and email at the moment.
Best Wishes
Phillip
If not, then these test_tick() calls
mislead the reader into thinking that the timestamps are significant,
thus it would make sense to drop them.
quoted
+test_expect_success 'simple fixup -C works' '+ test_when_finished "test_might_fail git rebase --abort" &&+ git checkout --detach A2 &&+ FAKE_LINES="1 fixup_-C 2" git rebase -i B &&
I see that you mirrored the implementation of FAKE_LINES handling of
"exec" here for "fixup", but the cases are quite different. The
argument to "exec" is arbitrary and can have any number of spaces
embedded in it, which conflicts with the meaning of spaces in
FAKE_LINES, which separate the individual commands in FAKE_LINES.
Consequently, "_" was chosen as a placeholder in "exec" to mean
"space".
However, "fixup" is a very different beast. Its arguments are not
arbitrary at all, so there isn't a good reason to mirror the choice of
"_" to represent a space, which leads to rather unsightly tokens such
as "fixup_-C". It would work just as well to use simpler tokens such
as "fixup-C" and "fixup-c", in which case t/lib-rebase.sh might parse
them like this (note that I also dropped `g` from the `sed` action):
fixup-*)
action=$(echo "$line" | sed 's/-/ -/');;
In fact, the recognized set of options following "fixup" is so small,
that you could even get by with simpler tokens "fixupC" and "fixupc":
fixupC)
action="fixup -C";;
fixupc)
actions="fixup -c";;
Though it's subjective whether or not "fixupC" and "fixupc" are nicer
than "fixup-C" and "fixup-c", respectively.
quoted
+test_expect_success 'fixup -C removes amend! from message' '+ test_when_finished "test_might_fail git rebase --abort" &&+ git checkout --detach A1 &&+ FAKE_LINES="1 fixup_-C 2" git rebase -i A &&+ test_cmp_rev HEAD^ A &&+ test_cmp_rev HEAD^{tree} A1^{tree} &&+ test_commit_message HEAD expected-message &&+ get_author HEAD >actual-author &&+ test_cmp expected-author actual-author+'
This test seems out of place. I would expect to see it added in the
patch which adds "amend!" functionality.
Alternatively, if the intention really is to support "amend!" this
early in the series in [6/9], then the commit message of [6/9] should
talk about it.
quoted
+test_expect_success 'fixup -C with conflicts gives correct message' '
+ test_when_finished "test_might_fail git rebase --abort" &&
Is there a reason this isn't written as:
test_when_finished "reset_rebase" &&
which is more common? Is there something non-obvious which makes
reset_rebase() inappropriate in these tests?
It feels clunky and fragile for this test to be changing
"expected-message" which was created in the "setup" test and used
unaltered up to this point. If the content of "expected-message" is
really going to change from test to test (as I see it changes again in
a later test), then it would be easier to reason about the behavior if
each test gives "expected-message" the precise content it should have
in that local context. As it is currently implemented, it's too
difficult to follow along and remember the value of "expected-message"
from test to test. It also makes it difficult to extend tests or add
new tests in between existing tests without negatively impacting other
tests. If each test sets up "expected-message" to the precise content
needed by the test, then both those problems go away.
From: Eric Sunshine <hidden> Date: 2021-02-04 16:16:29
On Thu, Feb 4, 2021 at 5:47 AM Phillip Wood [off-list ref] wrote:
On 02/02/2021 02:01, Eric Sunshine wrote:
quoted
Are the timestamps of these commits meaningful in this context?
I think we want to ensure that the timestamp of the commits created with
the different author are different from the previous commits. We ought
to be checking that the author date of the rebased commit matches the
author date of the original commit and not the author date of the fixup
commit created with the different author.
Such date-checking would indeed make sense and would remove any
potential confusion a reader might have concerning the manual
test_tick() calls. (It's not super important, but I still lean toward
dropping the test_tick() calls until they are actually needed simply
to avoid confusing readers. I asked about it in my review since their
purpose was unclear and I was genuinely wondering if I was overlooking
the reason for their presence. Future readers of the code may
experience the same puzzlement without necessarily having ready access
to the original author of the code from whom to receive an answer.)
This patch series adds fixup [-C|-c] options to interactive rebase. In
addition to amending the contents of the commit as the `fixup` command
does now, `fixup -C` replaces the commit message of the original commit
which we are fixing up with the message of the fixup commit.
And to edit the fixup commit message before committing, `fixup -c`
command is used instead of `fixup -C`. This convention is similar to
the existing `merge` command in the interactive rebase, that also supports
`-c` and `-C` options with similar meanings.
Also, `fixup -C` is intended to support amend! commit upon --autosquash
and it's working will be added in another patch series with the implementation
of amend! commit.
Changes from v4 :
(Thanks to Eric Sunshine, Christian Couder and Phillip Wood for suggestions
and reviews)
The major change in this version is to remove the working of `fixup -C`
with amend! commit and will include in the another patch series, in order
to avoid the confusion. So there are following changes :
* removed the patch (rebase -i : teach --autosquash to work with amend!)
* updated the test script (t3437-*.sh), changed the test setup and removed
two tests.
Earlier every test includes the commit message having subject starting
with amend! So, now it includes a setup of different branch for testing
fixup with options and also updated all the tests.
Removed the test - "skip fixup -C removes amend! from message" and also
"sequence of fixup, fixup -C & squash --signoff works" as I think it would
be better to test this also in the branch with amend! commit with different
author. (Will add these tests with amend! commit implementation)
* changed the flag type from enum todo_item_flags to unsigned
* Removed amend! conditions from sequencer.c and
* Replaced fixup_-* with fixup-* in lib-rebase.sh
* fixup a small nit in Documentation
Charvi Mendiratta (5):
sequencer: pass todo_item to do_pick_commit()
sequencer: use const variable for commit message comments
rebase -i: add fixup [-C | -c] command
t3437: test script for fixup [-C|-c] options in interactive rebase
doc/git-rebase: add documentation for fixup [-C|-c] options
Phillip Wood (3):
rebase -i: only write fixup-message when it's needed
sequencer: factor out code to append squash message
rebase -i: comment out squash!/fixup! subjects from squash message
Documentation/git-rebase.txt | 13 +-
rebase-interactive.c | 4 +-
sequencer.c | 270 ++++++++++++++++++++++++++----
t/lib-rebase.sh | 8 +-
t/t3415-rebase-autosquash.sh | 30 ++--
t/t3437-rebase-fixup-options.sh | 133 +++++++++++++++
t/t3437/expected-combined-message | 19 +++
t/t3900-i18n-commit.sh | 4 -
8 files changed, 425 insertions(+), 56 deletions(-)
create mode 100755 t/t3437-rebase-fixup-options.sh
create mode 100644 t/t3437/expected-combined-message
--
2.29.0.rc1
From: Phillip Wood <redacted>
The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup
commands, there's no point in writing it for squash commands as it is
immediately deleted.
Signed-off-by: Phillip Wood <redacted>
Reviewed-by: Taylor Blau <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Phillip Wood <redacted>
When squashing commit messages the squash!/fixup! subjects are not of
interest so comment them out to stop them becoming part of the final
message.
This change breaks a bunch of --autosquash tests which rely on the
"squash! <subject>" line appearing in the final commit message. This is
addressed by adding a second line to the commit message of the "squash!
..." commits and testing for that.
Signed-off-by: Phillip Wood <redacted>
Reviewed-by: Taylor Blau <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 21 ++++++++++++++++++++-
t/t3415-rebase-autosquash.sh | 30 ++++++++++++++++--------------
t/t3900-i18n-commit.sh | 4 ----
3 files changed, 36 insertions(+), 19 deletions(-)
@@ -1718,15 +1718,34 @@ static int is_pick_or_similar(enum todo_command command)}}+staticsize_tsubject_length(constchar*body)+{+constchar*p=body;+while(*p){+constchar*next=skip_blank_lines(p);+if(next!=p)+break;+p=strchrnul(p,'\n');+if(*p)+p++;+}+returnp-body;+}+staticvoidappend_squash_message(structstrbuf*buf,constchar*body,structreplay_opts*opts){+size_tcommented_len=0;+unlink(rebase_path_fixup_msg());+if(starts_with(body,"squash!")||starts_with(body,"fixup!"))+commented_len=subject_length(body);strbuf_addf(buf,"\n%c ",comment_line_char);strbuf_addf(buf,_("This is the commit message #%d:"),++opts->current_fixup_count+1);strbuf_addstr(buf,"\n\n");-strbuf_addstr(buf,body);+strbuf_add_commented_lines(buf,body,commented_len);+strbuf_addstr(buf,body+commented_len);}staticintupdate_squash_messages(structrepository*r,
@@ -236,7 +237,7 @@ test_auto_commit_flags () {echo1>file1&&gitadd-u&&test_tick&&-gitcommit--$1first-commit&&+gitcommit--$1first-commit-m"extra para for first"&&gittagfinal-commit-$1&&test_tick&&gitrebase--autosquash-iHEAD^^^&&
@@ -264,11 +265,11 @@ test_auto_fixup_fixup () {echo1>file1&&gitadd-u&&test_tick&&-gitcommit-m"$1! first"&&+gitcommit-m"$1! first"-m"extra para for first"&&echo2>file1&&gitadd-u&&test_tick&&-gitcommit-m"$1! $2! first"&&+gitcommit-m"$1! $2! first"-m"second extra para for first"&&gittag"final-$1-$2"&&test_tick&&(
@@ -329,12 +330,12 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' 'gitadd-u&&test_tick&&oid=$(gitrev-parse--shortHEAD^)&&-gitcommit-m"squash! $oid"&&+gitcommit-m"squash! $oid"-m"extra para for first"&&echo1>file1&&gitadd-u&&test_tick&&subject=$(gitlog-n1--format=%sHEAD~2)&&-gitcommit-m"squash! $subject"&&+gitcommit-m"squash! $subject"-m"second extra para for first"&&gittagfinal-squash-instFmt&&test_tick&&gitrebase--autosquash-iHEAD~4&&
@@ -345,8 +346,9 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' 'gitcat-fileblobHEAD^:file1>actual&&test_cmpexpectactual&&gitcat-filecommitHEAD^>commit&&-grepsquashcommit>actual&&-test_line_count=2actual+!grep"squash"commit&&+grepfirstcommit>actual&&+test_line_count=3actual' test_expect_success'autosquash with empty custom instructionFormat''
@@ -4,6 +4,7 @@## - override the commit message with $FAKE_COMMIT_MESSAGE# - amend the commit message with $FAKE_COMMIT_AMEND+# - copy the original commit message to a file with $FAKE_MESSAGE_COPY# - check that non-commit messages have a certain line count with $EXPECT_COUNT# - check the commit count in the commit message header with $EXPECT_HEADER_COUNT# - rewrite a rebase -i script as directed by $FAKE_LINES.
@@ -14,8 +15,8 @@# specified line.## "<cmd> <lineno>" -- add a line with the specified command-# ("pick", "squash", "fixup", "edit", "reword" or "drop") and the-# SHA1 taken from the specified line.+# ("pick", "squash", "fixup"|"fixup-C"|"fixup-c", "edit", "reword" or "drop")+# and the SHA1 taken from the specified line.## "exec_cmd_with_args" -- add an "exec cmd with args" line.#
@@ -0,0 +1,19 @@+# This is a combination of 4 commits.+# This is the 1st commit message:++B++# This is the commit message #2:++new subject++new+body++# The commit message #3 will be skipped:++# A2++# This is the commit message #4:++A3
Add options to `fixup` command to fixup both the commit contents and
message. `fixup -C` command is used to replace the original commit
message and `fixup -c`, additionally allows to edit the commit message.
This convention is similar to the existing `merge` command in the
interactive rebase, that also supports `-c` and `-C` options with
similar meanings.
Original-patch-by: Phillip Wood [off-list ref]
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Reviewed-by: Eric Sunshine <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
rebase-interactive.c | 4 +-
sequencer.c | 211 +++++++++++++++++++++++++++++++++++++++----
2 files changed, 195 insertions(+), 20 deletions(-)
@@ -44,7 +44,9 @@ void append_todo_help(int command_count,"r, reword <commit> = use commit, but edit the commit message\n""e, edit <commit> = use commit, but stop for amending\n""s, squash <commit> = use commit, but meld into previous commit\n"-"f, fixup <commit> = like \"squash\", but discard this commit's log message\n"+"f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"+" commit's log message; use -C to replace with this\n"+" commit message or -c to edit this commit message\n""x, exec <command> = run command (the rest of the line) using shell\n""b, break = stop here (continue rebase later with 'git rebase --continue')\n""d, drop <commit> = remove commit\n"
@@ -1718,6 +1718,12 @@ static int is_pick_or_similar(enum todo_command command)}}+enumtodo_item_flags{+TODO_EDIT_MERGE_MSG=(1<<0),+TODO_REPLACE_FIXUP_MSG=(1<<1),+TODO_EDIT_FIXUP_MSG=(1<<2),+};+staticsize_tsubject_length(constchar*body){constchar*p=body;
@@ -1734,32 +1740,174 @@ static size_t subject_length(const char *body)staticconstcharfirst_commit_msg_str[]=N_("This is the 1st commit message:");staticconstcharnth_commit_msg_fmt[]=N_("This is the commit message #%d:");+staticconstcharskip_first_commit_msg_str[]=N_("The 1st commit message will be skipped:");staticconstcharskip_nth_commit_msg_fmt[]=N_("The commit message #%d will be skipped:");staticconstcharcombined_commit_msg_fmt[]=N_("This is a combination of %d commits.");-staticvoidappend_squash_message(structstrbuf*buf,constchar*body,-structreplay_opts*opts)+staticintis_fixup_flag(enumtodo_commandcommand,unsignedflag)+{+returncommand==TODO_FIXUP&&((flag&TODO_REPLACE_FIXUP_MSG)||+(flag&TODO_EDIT_FIXUP_MSG));+}++/*+*Wrapperaroundstrbuf_add_commented_lines()whichavoidsdouble+*commentingcommitsubjects.+*/+staticvoidadd_commented_lines(structstrbuf*buf,constvoid*str,size_tlen)+{+constchar*s=str;+while(len>0&&s[0]==comment_line_char){+size_tcount;+constchar*n=memchr(s,'\n',len);+if(!n)+count=len;+else+count=n-s+1;+strbuf_add(buf,s,count);+s+=count;+len-=count;+}+strbuf_add_commented_lines(buf,s,len);+}++/* Does the current fixup chain contain a squash command? */+staticintseen_squash(structreplay_opts*opts)+{+returnstarts_with(opts->current_fixups.buf,"squash")||+strstr(opts->current_fixups.buf,"\nsquash");+}++staticvoidupdate_comment_bufs(structstrbuf*buf1,structstrbuf*buf2,intn){-size_tcommented_len=0;+strbuf_setlen(buf1,2);+strbuf_addf(buf1,_(nth_commit_msg_fmt),n);+strbuf_addch(buf1,'\n');+strbuf_setlen(buf2,2);+strbuf_addf(buf2,_(skip_nth_commit_msg_fmt),n);+strbuf_addch(buf2,'\n');+}-unlink(rebase_path_fixup_msg());-if(starts_with(body,"squash!")||starts_with(body,"fixup!"))+/*+*Commentoutanyun-commentedcommitmessages,updatingthemessagecomments+*tosaytheywillbeskippedbutdonotcommentouttheemptylinesthat+*surroundcommitmessagesandtheircomments.+*/+staticvoidupdate_squash_message_for_fixup(structstrbuf*msg)+{+void(*copy_lines)(structstrbuf*,constvoid*,size_t)=strbuf_add;+structstrbufbuf1=STRBUF_INIT,buf2=STRBUF_INIT;+constchar*s,*start;+char*orig_msg;+size_torig_msg_len;+inti=1;++strbuf_addf(&buf1,"# %s\n",_(first_commit_msg_str));+strbuf_addf(&buf2,"# %s\n",_(skip_first_commit_msg_str));+s=start=orig_msg=strbuf_detach(msg,&orig_msg_len);+while(s){+constchar*next;+size_toff;+if(skip_prefix(s,buf1.buf,&next)){+/*+*Copythelastmessage,preservingtheblankline+*precedingthecurrentline+*/+off=(s>start+1&&s[-2]=='\n')?1:0;+copy_lines(msg,start,s-start-off);+if(off)+strbuf_addch(msg,'\n');+/*+*Thenextmessageneedstobecommentedoutbutthe+*messageheaderisalreadycommentedoutsojustcopy+*itandtheblanklinethatfollowsit.+*/+strbuf_addbuf(msg,&buf2);+if(*next=='\n')+strbuf_addch(msg,*next++);+start=s=next;+copy_lines=add_commented_lines;+update_comment_bufs(&buf1,&buf2,++i);+}elseif(skip_prefix(s,buf2.buf,&next)){+off=(s>start+1&&s[-2]=='\n')?1:0;+copy_lines(msg,start,s-start-off);+start=s-off;+s=next;+copy_lines=strbuf_add;+update_comment_bufs(&buf1,&buf2,++i);+}else{+s=strchr(s,'\n');+if(s)+s++;+}+}+copy_lines(msg,start,orig_msg_len-(start-orig_msg));+free(orig_msg);+strbuf_release(&buf1);+strbuf_release(&buf2);+}++staticintappend_squash_message(structstrbuf*buf,constchar*body,+enumtodo_commandcommand,structreplay_opts*opts,+unsignedflag)+{+constchar*fixup_msg;+size_tcommented_len=0,fixup_off;+/*+*fixup-Cisnon-interactiveandnotnormallyusedwithfixup!+*orsquash!commits,soonlycommentoutthosesubjectswhen+*squashingcommitmessages.+*/+if((command==TODO_SQUASH||seen_squash(opts))&&+(starts_with(body,"squash!")||starts_with(body,"fixup!")))commented_len=subject_length(body);+strbuf_addf(buf,"\n%c ",comment_line_char);strbuf_addf(buf,_(nth_commit_msg_fmt),++opts->current_fixup_count+1);strbuf_addstr(buf,"\n\n");strbuf_add_commented_lines(buf,body,commented_len);+/* buf->buf may be reallocated so store an offset into the buffer */+fixup_off=buf->len;strbuf_addstr(buf,body+commented_len);++/* fixup -C after squash behaves like squash */+if(is_fixup_flag(command,flag)&&!seen_squash(opts)){+/*+*We'rereplacingthecommitmessagesoweneedto+*appendtheSigned-off-by:traileriftheuser+*requested'--signoff'.+*/+if(opts->signoff)+append_signoff(buf,0,0);++if((command==TODO_FIXUP)&&+(flag&TODO_REPLACE_FIXUP_MSG)&&+(file_exists(rebase_path_fixup_msg())||+!file_exists(rebase_path_squash_msg()))){+fixup_msg=skip_blank_lines(buf->buf+fixup_off);+if(write_message(fixup_msg,strlen(fixup_msg),+rebase_path_fixup_msg(),0)<0)+returnerror(_("cannot write '%s'"),+rebase_path_fixup_msg());+}else{+unlink(rebase_path_fixup_msg());+}+}else{+unlink(rebase_path_fixup_msg());+}++return0;}staticintupdate_squash_messages(structrepository*r,enumtodo_commandcommand,structcommit*commit,-structreplay_opts*opts)+structreplay_opts*opts,+unsignedflag){structstrbufbuf=STRBUF_INIT;-intres;+intres=0;constchar*message,*body;constchar*encoding=get_commit_output_encoding();
@@ -1779,6 +1927,8 @@ static int update_squash_messages(struct repository *r,opts->current_fixup_count+2);strbuf_splice(&buf,0,eol-buf.buf,header.buf,header.len);strbuf_release(&header);+if(is_fixup_flag(command,flag)&&!seen_squash(opts))+update_squash_message_for_fixup(&buf);}else{structobject_idhead;structcommit*head_commit;
@@ -1792,18 +1942,22 @@ static int update_squash_messages(struct repository *r,returnerror(_("could not read HEAD's commit message"));find_commit_subject(head_message,&body);-if(command==TODO_FIXUP&&write_message(body,strlen(body),+if(command==TODO_FIXUP&&!flag&&write_message(body,strlen(body),rebase_path_fixup_msg(),0)<0){unuse_commit_buffer(head_commit,head_message);returnerror(_("cannot write '%s'"),rebase_path_fixup_msg());}-strbuf_addf(&buf,"%c ",comment_line_char);strbuf_addf(&buf,_(combined_commit_msg_fmt),2);strbuf_addf(&buf,"\n%c ",comment_line_char);-strbuf_addstr(&buf,_(first_commit_msg_str));+strbuf_addstr(&buf,is_fixup_flag(command,flag)?+_(skip_first_commit_msg_str):+_(first_commit_msg_str));strbuf_addstr(&buf,"\n\n");-strbuf_addstr(&buf,body);+if(is_fixup_flag(command,flag))+strbuf_add_commented_lines(&buf,body,strlen(body));+else+strbuf_addstr(&buf,body);unuse_commit_buffer(head_commit,head_message);}
@@ -1813,8 +1967,8 @@ static int update_squash_messages(struct repository *r,oid_to_hex(&commit->object.oid));find_commit_subject(message,&body);-if(command==TODO_SQUASH){-append_squash_message(&buf,body,opts);+if(command==TODO_SQUASH||is_fixup_flag(command,flag)){+res=append_squash_message(&buf,body,command,opts,flag);}elseif(command==TODO_FIXUP){strbuf_addf(&buf,"\n%c ",comment_line_char);strbuf_addf(&buf,_(skip_nth_commit_msg_fmt),
From: Phillip Wood <redacted>
This code is going to grow over the next two commits so move it to
its own function.
Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
@@ -1718,6 +1718,17 @@ static int is_pick_or_similar(enum todo_command command)}}+staticvoidappend_squash_message(structstrbuf*buf,constchar*body,+structreplay_opts*opts)+{+unlink(rebase_path_fixup_msg());+strbuf_addf(buf,"\n%c ",comment_line_char);+strbuf_addf(buf,_("This is the commit message #%d:"),+++opts->current_fixup_count+1);+strbuf_addstr(buf,"\n\n");+strbuf_addstr(buf,body);+}+staticintupdate_squash_messages(structrepository*r,enumtodo_commandcommand,structcommit*commit,
@@ -1779,12 +1790,7 @@ static int update_squash_messages(struct repository *r,find_commit_subject(message,&body);if(command==TODO_SQUASH){-unlink(rebase_path_fixup_msg());-strbuf_addf(&buf,"\n%c ",comment_line_char);-strbuf_addf(&buf,_("This is the commit message #%d:"),-++opts->current_fixup_count+1);-strbuf_addstr(&buf,"\n\n");-strbuf_addstr(&buf,body);+append_squash_message(&buf,body,opts);}elseif(command==TODO_FIXUP){strbuf_addf(&buf,"\n%c ",comment_line_char);strbuf_addf(&buf,_("The commit message #%d will be skipped:"),
This makes it easier to use and reuse the comments.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Reviewed-by: Taylor Blau <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
@@ -1732,6 +1732,11 @@ static size_t subject_length(const char *body)returnp-body;}+staticconstcharfirst_commit_msg_str[]=N_("This is the 1st commit message:");+staticconstcharnth_commit_msg_fmt[]=N_("This is the commit message #%d:");+staticconstcharskip_nth_commit_msg_fmt[]=N_("The commit message #%d will be skipped:");+staticconstcharcombined_commit_msg_fmt[]=N_("This is a combination of %d commits.");+staticvoidappend_squash_message(structstrbuf*buf,constchar*body,structreplay_opts*opts){
@@ -1741,7 +1746,7 @@ static void append_squash_message(struct strbuf *buf, const char *body,if(starts_with(body,"squash!")||starts_with(body,"fixup!"))commented_len=subject_length(body);strbuf_addf(buf,"\n%c ",comment_line_char);-strbuf_addf(buf,_("This is the commit message #%d:"),+strbuf_addf(buf,_(nth_commit_msg_fmt),++opts->current_fixup_count+1);strbuf_addstr(buf,"\n\n");strbuf_add_commented_lines(buf,body,commented_len);
@@ -1770,7 +1775,7 @@ static int update_squash_messages(struct repository *r,buf.buf:strchrnul(buf.buf,'\n');strbuf_addf(&header,"%c ",comment_line_char);-strbuf_addf(&header,_("This is a combination of %d commits."),+strbuf_addf(&header,_(combined_commit_msg_fmt),opts->current_fixup_count+2);strbuf_splice(&buf,0,eol-buf.buf,header.buf,header.len);strbuf_release(&header);
@@ -1794,9 +1799,9 @@ static int update_squash_messages(struct repository *r,}strbuf_addf(&buf,"%c ",comment_line_char);-strbuf_addf(&buf,_("This is a combination of %d commits."),2);+strbuf_addf(&buf,_(combined_commit_msg_fmt),2);strbuf_addf(&buf,"\n%c ",comment_line_char);-strbuf_addstr(&buf,_("This is the 1st commit message:"));+strbuf_addstr(&buf,_(first_commit_msg_str));strbuf_addstr(&buf,"\n\n");strbuf_addstr(&buf,body);
@@ -1812,7 +1817,7 @@ static int update_squash_messages(struct repository *r,append_squash_message(&buf,body,opts);}elseif(command==TODO_FIXUP){strbuf_addf(&buf,"\n%c ",comment_line_char);-strbuf_addf(&buf,_("The commit message #%d will be skipped:"),+strbuf_addf(&buf,_(skip_nth_commit_msg_fmt),++opts->current_fixup_count+1);strbuf_addstr(&buf,"\n\n");strbuf_add_commented_lines(&buf,body,strlen(body));
@@ -887,9 +887,16 @@ If you want to fold two or more commits into one, replace the command "pick" for the second and subsequent commits with "squash" or "fixup". If the commits had different authors, the folded commit will be attributed to the author of the first commit. The suggested commit-message for the folded commit is the concatenation of the commit-messages of the first commit and of those with the "squash" command,-but omits the commit messages of commits with the "fixup" command.+message for the folded commit is the concatenation of the first+commit's message with those identified by "squash" commands, omitting the+messages of commits identified by "fixup" commands, unless "fixup -c"+is used. In that case the suggested commit message is only the message+of the "fixup -c" commit, and an editor is opened allowing you to edit+the message. The contents (patch) of the "fixup -c" commit are still+incorporated into the folded commit. If there is more than one "fixup -c"+commit, the message from the final one is used. You can also use+"fixup -C" to get the same behavior as "fixup -c" except without opening+an editor. 'git rebase' will stop when "pick" has been replaced with "edit" or when a command fails due to merge errors. When you are done editing
As an additional member of the structure todo_item will be required in
future commits pass the complete structure.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
+test_expect_success 'fixup -C with conflicts gives correct message' '
+ test_when_finished "test_might_fail git rebase --abort" &&
Is there a reason this isn't written as:
test_when_finished "reset_rebase" &&
which is more common? Is there something non-obvious which makes
reset_rebase() inappropriate in these tests?
I missed this earlier, but I confirmed before sending next version that as
reset_rebase() removes the untracked files so we could not use this as otherwise
it removes the fake-editor.sh file and will be required to set a fake
editor for every
test and also removes other untracked files which may be used to debug.
So, I think it's okay with:
test_when_finished "test_might_fail git rebase --abort"
Thanks and regards,
Charvi
From: Eric Sunshine <hidden> Date: 2021-02-05 07:31:32
On Thu, Feb 4, 2021 at 2:05 PM Charvi Mendiratta [off-list ref] wrote:
Changes from v4 :
(Thanks to Eric Sunshine, Christian Couder and Phillip Wood for suggestions
and reviews)
Thanks for working on this and re-rolling. Unfortunately, it seems
that v4 already landed in Junio's `next` branch which means that he
won't be replacing v4 wholesale as would have been the case if it was
still in the `seen` branch. Once patches are in `next`, improvements
are made by building changes atop them (incrementally) rather than
replacing them. Whether or not it makes sense for you to spend time
re-doing these patches as incremental changes is not clear. In fact...
The major change in this version is to remove the working of `fixup -C`
with amend! commit and will include in the another patch series, in order
to avoid the confusion. So there are following changes :
* removed the patch (rebase -i : teach --autosquash to work with amend!)
* updated the test script (t3437-*.sh), changed the test setup and removed
two tests.
Earlier every test includes the commit message having subject starting
with amend! So, now it includes a setup of different branch for testing
fixup with options and also updated all the tests.
Removed the test - "skip fixup -C removes amend! from message" and also
"sequence of fixup, fixup -C & squash --signoff works" as I think it would
be better to test this also in the branch with amend! commit with different
author. (Will add these tests with amend! commit implementation)
Despite these being nice cleanups to the standalone series, I'm not
sure it's worth spending your time creating new patches to undo these
from `next`. Removing them only to add them back later is not
necessarily going to help "unconfuse" someone reading the commits in
the permanent project history.
* changed the flag type from enum todo_item_flags to unsigned
* Replaced fixup_-* with fixup-* in lib-rebase.sh
* fixup a small nit in Documentation
These changes are still worthwhile and can easily be done
incrementally atop what is already in next, I would think.
Anyhow, use your best judgment to decide how much work to devote to this.
Hi,
On Fri, 5 Feb 2021 at 13:00, Eric Sunshine [off-list ref] wrote:
[...]
Thanks for working on this and re-rolling. Unfortunately, it seems
that v4 already landed in Junio's `next` branch which means that he
won't be replacing v4 wholesale as would have been the case if it was
still in the `seen` branch. Once patches are in `next`, improvements
are made by building changes atop them (incrementally) rather than
replacing them. Whether or not it makes sense for you to spend time
re-doing these patches as incremental changes is not clear. In fact...
Okay, I admit I was not aware of this, before.
quoted
The major change in this version is to remove the working of `fixup -C`
with amend! commit and will include in the another patch series, in order
to avoid the confusion. So there are following changes :
* removed the patch (rebase -i : teach --autosquash to work with amend!)
* updated the test script (t3437-*.sh), changed the test setup and removed
two tests.
Earlier every test includes the commit message having subject starting
with amend! So, now it includes a setup of different branch for testing
fixup with options and also updated all the tests.
Removed the test - "skip fixup -C removes amend! from message" and also
"sequence of fixup, fixup -C & squash --signoff works" as I think it would
be better to test this also in the branch with amend! commit with different
author. (Will add these tests with amend! commit implementation)
Despite these being nice cleanups to the standalone series, I'm not
sure it's worth spending your time creating new patches to undo these
from `next`. Removing them only to add them back later is not
necessarily going to help "unconfuse" someone reading the commits in
the permanent project history.
Yes, I was also thinking that let's not remove the working of `fixup -C` with
amend! commit as it's true that it is intended to work like that way.
quoted
* changed the flag type from enum todo_item_flags to unsigned
* Replaced fixup_-* with fixup-* in lib-rebase.sh
* fixup a small nit in Documentation
These changes are still worthwhile and can easily be done
incrementally atop what is already in next, I would think.
I agree, these fixes are required. So, I am not sure but now to do these
fixup shall I send another patch cleaning this patch series(v4) and rebase the
patch on the 'next' branch ? Is it the right way ?
Thanks and Regards,
Charvi
From: Christian Couder <hidden> Date: 2021-02-05 18:29:32
On Fri, Feb 5, 2021 at 10:42 AM Charvi Mendiratta [off-list ref] wrote:
On Fri, 5 Feb 2021 at 13:00, Eric Sunshine [off-list ref] wrote:
quoted
quoted
* changed the flag type from enum todo_item_flags to unsigned
* Replaced fixup_-* with fixup-* in lib-rebase.sh
* fixup a small nit in Documentation
These changes are still worthwhile and can easily be done
incrementally atop what is already in next, I would think.
I agree, these fixes are required. So, I am not sure but now to do these
fixup shall I send another patch cleaning this patch series(v4) and rebase the
patch on the 'next' branch ? Is it the right way ?
Yeah, I think you can send each of the above 3 changes in a different
patch on top of the 'next' branch. That would create a new 3 patch
long series, which you should give a new name and not consider v5 of
the previous patch series.
Best,
Christian.
From: Eric Sunshine <hidden> Date: 2021-02-05 19:09:36
On Fri, Feb 5, 2021 at 1:25 PM Christian Couder
[off-list ref] wrote:
On Fri, Feb 5, 2021 at 10:42 AM Charvi Mendiratta [off-list ref] wrote:
quoted
On Fri, 5 Feb 2021 at 13:00, Eric Sunshine [off-list ref] wrote:
quoted
These changes are still worthwhile and can easily be done
incrementally atop what is already in next, I would think.
I agree, these fixes are required. So, I am not sure but now to do these
fixup shall I send another patch cleaning this patch series(v4) and rebase the
patch on the 'next' branch ? Is it the right way ?
Yeah, I think you can send each of the above 3 changes in a different
patch on top of the 'next' branch. That would create a new 3 patch
long series, which you should give a new name and not consider v5 of
the previous patch series.
Yes, whatever issues from my reviews seem worth fixing atop the
existing v4 can be included in this new patch series. (I think there
may have been a few things beyond the three listed in the v5 cover
letter, but I didn't bother doing a full audit of my review emails, so
I could be wrong.) As Christian said, just make it a new series,
though be sure to build it atop your v4 rather than building it atop
"next". (The problem with building atop "next" is that your series
then gets held hostage by _every_ series already in "next", which
makes it nearly impossible for your series to graduate to "master"
since it can't graduate until every other existing series in "next"
graduates to "master".) The one other important thing is to mention in
the cover letter that your new series is built atop "cm/rebase-i",
which lets Junio know where to place the new series when he picks it
up (and also lets reviewers know where to apply it if they want to
test it themselves).
On Sat, 6 Feb 2021 at 00:27, Eric Sunshine [off-list ref] wrote:
On Fri, Feb 5, 2021 at 1:25 PM Christian Couder
[off-list ref] wrote:
quoted
On Fri, Feb 5, 2021 at 10:42 AM Charvi Mendiratta [off-list ref] wrote:
quoted
On Fri, 5 Feb 2021 at 13:00, Eric Sunshine [off-list ref] wrote:
quoted
These changes are still worthwhile and can easily be done
incrementally atop what is already in next, I would think.
I agree, these fixes are required. So, I am not sure but now to do these
fixup shall I send another patch cleaning this patch series(v4) and rebase the
patch on the 'next' branch ? Is it the right way ?
Yeah, I think you can send each of the above 3 changes in a different
patch on top of the 'next' branch. That would create a new 3 patch
long series, which you should give a new name and not consider v5 of
the previous patch series.
Yes, whatever issues from my reviews seem worth fixing atop the
existing v4 can be included in this new patch series. (I think there
may have been a few things beyond the three listed in the v5 cover
letter, but I didn't bother doing a full audit of my review emails, so
I could be wrong.) As Christian said, just make it a new series,
though be sure to build it atop your v4 rather than building it atop
"next". (The problem with building atop "next" is that your series
then gets held hostage by _every_ series already in "next", which
makes it nearly impossible for your series to graduate to "master"
since it can't graduate until every other existing series in "next"
graduates to "master".) The one other important thing is to mention in
the cover letter that your new series is built atop "cm/rebase-i",
which lets Junio know where to place the new series when he picks it
up (and also lets reviewers know where to apply it if they want to
test it themselves).