This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
improves it. It fixup the source code of 'fixup [-C | -c]' command in the
sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
documentation.
(Thanks to Junio C Hamano, Eric Sunshine, Christian Couder and Phillip Wood
for the suggestions and guidance for this patch series)
Charvi Mendiratta (7):
sequencer: fixup the datatype of the 'flag' argument
sequencer: rename a few functions
rebase -i: clarify and fix 'fixup -c' rebase-todo help
t/lib-rebase: change the implementation of commands with options
t3437: fix indendation of the here-doc
t/t3437: update the tests
doc/rebase -i: fix typo in the documentation of 'fixup' command
Documentation/git-rebase.txt | 2 +-
rebase-interactive.c | 6 +-
sequencer.c | 23 +++---
t/lib-rebase.sh | 8 +-
t/t3437-rebase-fixup-options.sh | 140 +++++++++++++++++---------------
5 files changed, 93 insertions(+), 86 deletions(-)
--
2.29.0.rc1
As 'flag' is a combination of bits, so change its datatype from
'enum todo_item_flags' to 'unsigned'.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Rename functions to make them more descriptive and while at it, remove
unnecessary 'inline' of the skip_fixupish() function.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
When `-c` says "edit the commit message" it's not clear what will be
edited. The original's commit message or the replacement's message or a
combination of the two. Word it such that it states more precisely what
exactly will be edited and also remove the use of a period and
capitalized word in the to-do help text.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
rebase-interactive.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -45,8 +45,8 @@ void append_todo_help(int command_count,"e, edit <commit> = use commit, but stop for amending\n""s, squash <commit> = use commit, but meld into previous commit\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 the commit message\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"
@@ -55,7 +55,7 @@ void append_todo_help(int command_count,"m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"". create a merge commit using the original merge commit's\n"". message (or the oneline, if no original merge commit was\n"-". specified). Use -c <commit> to reword the commit message.\n"+". specified); use -c <commit> to reword the commit message\n""\n""These lines can be re-ordered; they are executed from top to bottom.\n");unsignededit_todo=!(shortrevisions&&shortonto);
@@ -894,7 +894,7 @@ 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 last last one is used. You can also use+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.
In the test scripts, the here-doc body and EOF are indented the same
amount as the command which opened the here-doc. Let's remove
one level of indendation.
Original-patch-by: Phillip Wood [off-list ref]
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 62 ++++++++++++++++-----------------
1 file changed, 31 insertions(+), 31 deletions(-)
"fixup" and "merge" mirrors the implementation of FAKE_LINES handling of
"exec", 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 very different from "exec". 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". Let's replace it with simpler tokens such as "fixup-C"
and "fixup-c".
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/lib-rebase.sh | 8 ++++----
t/t3437-rebase-fixup-options.sh | 18 +++++++++---------
2 files changed, 13 insertions(+), 13 deletions(-)
@@ -15,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.#
Let's do the changes listed below to make tests more easier to follow :
-Remove the dependency of 'expected-message' file from earlier tests to
make it easier to run tests selectively with '--run' or 'GIT_SKIP_TESTS'.
-Add author timestamp to check that the author date of fixed up commit
is unchanged.
-Simplify the test_commit_message() and add comments before the
function.
-Clarify the working of 'fixup -c' with "amend!" in the test-description.
-Remove unnecessary curly braces and use the named commits in the
tests so that they will still refer to the same commit if the setup
gets changed in the future whereas 'branch~2' will change which commit
it points to.
Original-patch-by: Phillip Wood [off-list ref]
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 82 ++++++++++++++++++---------------
1 file changed, 45 insertions(+), 37 deletions(-)
@@ -18,36 +20,34 @@ editor to allow the user to edit the message before committing.EMPTY=""+# test_commit_message <rev> -m <msg>+# test_commit_message <rev> <path>+# Verify that the commit message of <rev> matches+# <msg> or the content of <path>. test_commit_message(){-rev="$1"&&# commit or tag we want to test-file="$2"&&# test against the content of a file-gitshow--no-patch--pretty=format:%B"$rev">actual-message&&-iftest"$2"=-m-then-str="$3"&&# test against a string-printf"%s\n""$str">tmp-expected-message&&-file="tmp-expected-message"-fi-test_cmp"$file"actual-message+gitshow--no-patch--pretty=format:%B"$1">actual&&+case"$2"in+-m)echo"$3">expect&&+test_cmpexpectactual;;+*)test_cmp"$2"actual;;+esac} get_author(){rev="$1"&&-gitlog-1--pretty=format:"%an %ae""$rev"+gitlog-1--pretty=format:"%an %ae %at""$rev"} test_expect_success'setup''cat>message<<-EOF&&amend!B-${EMPTY}+$EMPTYnewsubject-${EMPTY}+$EMPTYnewbodyEOF-sed"1,2d"message>expected-message&&-test_commitAA&&test_commitBB&&get_authorHEAD>expected-author&&
From: Eric Sunshine <hidden> Date: 2021-02-07 18:46:31
On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta [off-list ref] wrote:
Let's do the changes listed below to make tests more easier to follow :
-Remove the dependency of 'expected-message' file from earlier tests to
make it easier to run tests selectively with '--run' or 'GIT_SKIP_TESTS'.
-Add author timestamp to check that the author date of fixed up commit
is unchanged.
-Simplify the test_commit_message() and add comments before the
function.
-Clarify the working of 'fixup -c' with "amend!" in the test-description.
-Remove unnecessary curly braces and use the named commits in the
tests so that they will still refer to the same commit if the setup
gets changed in the future whereas 'branch~2' will change which commit
it points to.
Typically, if you find yourself enumerating a list of distinct changes
like this in a commit message, it's a good indication that it should
be split into multiple patches, each taking care of one item from the
list. A good reason for splitting it up like this is that it's
difficult for reviewers to keep the entire list in mind while
reviewing the patch, however, it's easy to keep in mind a single
stated goal while reading the changes.
Having said that, I'm not sure it's worth a re-roll or the extra work
of actually splitting it up since you've already been dragged deeper
into this than planned, and these are relatively minor issues.
(Returning to this after reading the remainder of the patch, I did
find it reasonably confusing trying to figure out which changes
related to each other and to items from the list above. It would have
been easier to reason about the changes had they been done in separate
patches. Still, though, I'm not sure it's worth the time and effort to
split them up -- but I wouldn't complain if you did.)
More below...
@@ -8,8 +8,10 @@ test_description='git rebase interactive fixup options This test checks the "fixup [-C|-c]" command of rebase interactive. In addition to amending the contents of the commit, "fixup -C" replaces the original commit message with the message of the fixup-commit. "fixup -c" also replaces the original message, but opens the-editor to allow the user to edit the message before committing.+commit and similar to "fixup" command that works with "fixup!", "fixup -C"+works with "amend!" upon --autosquash. "fixup -c" also replaces the original+message, but opens the editor to allow the user to edit the message before+committing. '
I had trouble digesting this run-on sentence due, I think, to the
mixing of thoughts. It might be easier to understand if you first talk
only about the options to `fixup` (-c/-C), and then, as a separate
sentence, talk about how `amend!` is transformed into `fixup -C` (like
`fixup!` is transformed into `fixup`). However, as this is just minor
descriptive text in a test file, not user-facing documentation, I'm
not sure it matters enough to warrant a re-roll.
quoted hunk
@@ -18,36 +20,34 @@ editor to allow the user to edit the message before committing.+# test_commit_message <rev> -m <msg>+# test_commit_message <rev> <path>+# Verify that the commit message of <rev> matches+# <msg> or the content of <path>.
Good.
test_commit_message () {
+ git show --no-patch --pretty=format:%B "$1" >actual &&
+ case "$2" in
+ -m) echo "$3" >expect &&
+ test_cmp expect actual ;;
+ *) test_cmp "$2" actual ;;
+ esac
}
The funky indentation here is due to a mix of tabs and spaces. It
should use tabs exclusively.
From: Eric Sunshine <hidden> Date: 2021-02-07 18:50:51
On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta [off-list ref] wrote:
When `-c` says "edit the commit message" it's not clear what will be
edited. The original's commit message or the replacement's message or a
combination of the two. Word it such that it states more precisely what
exactly will be edited and also remove the use of a period and
capitalized word in the to-do help text.
If you happen to re-roll for some reason, it might be a good idea to
explain why you are removing the period and capitalization since the
reason is not otherwise clear to the casual reader. So, perhaps:
... exactly will be edited. While at it, also drop the jarring
period and capitalization, neither of which is otherwise present
in the message.
or something like that.
From: Eric Sunshine <hidden> Date: 2021-02-07 18:55:53
On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta [off-list ref] wrote:
In the test scripts, the here-doc body and EOF are indented the same
amount as the command which opened the here-doc. Let's remove
one level of indendation.
s/indendation/indentation/
I found "In the test scripts" ambiguous. It isn't clear if you are
talking about all test scripts or the script(s) this patch is fixing
up. Sp, if you happen to re-roll for some reason, perhaps clarify by
saying something like:
The most common way to format here-docs in Git test scripts is for
the body and EOF to be indented the same amount as the command
which opened the here-doc. Fix a few here-docs in this script to
conform to that standard.
Original-patch-by: Phillip Wood [off-list ref]
I don't think this new patch is based upon Phillip's, so you can
probably drop this attribution.
From: Eric Sunshine <hidden> Date: 2021-02-07 18:58:43
On Sun, Feb 7, 2021 at 1:18 PM Charvi Mendiratta [off-list ref] wrote:
This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
improves it. It fixup the source code of 'fixup [-C | -c]' command in the
sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
documentation.
Thanks for working on this. I looked over the entire series and left a
few minor comments. As mentioned in my [6/7] review, you might also
want to consider splitting that patch into several patches (though
it's not clear if the extra work of doing so is warranted). Anyhow,
aside from some botched indentation in [6/7], it all looked clean.
On Mon, 8 Feb 2021 at 00:19, Eric Sunshine [off-list ref] wrote:
On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta [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 or the replacement's message or a
combination of the two. Word it such that it states more precisely what
exactly will be edited and also remove the use of a period and
capitalized word in the to-do help text.
If you happen to re-roll for some reason, it might be a good idea to
explain why you are removing the period and capitalization since the
reason is not otherwise clear to the casual reader. So, perhaps:
... exactly will be edited. While at it, also drop the jarring
period and capitalization, neither of which is otherwise present
in the message.
or something like that.
Hi Eric,
On Mon, 8 Feb 2021 at 00:13, Eric Sunshine [off-list ref] wrote:
[...]
Typically, if you find yourself enumerating a list of distinct changes
like this in a commit message, it's a good indication that it should
be split into multiple patches, each taking care of one item from the
list. A good reason for splitting it up like this is that it's
difficult for reviewers to keep the entire list in mind while
reviewing the patch, however, it's easy to keep in mind a single
stated goal while reading the changes.
Having said that, I'm not sure it's worth a re-roll or the extra work
of actually splitting it up since you've already been dragged deeper
into this than planned, and these are relatively minor issues.
(Returning to this after reading the remainder of the patch, I did
find it reasonably confusing trying to figure out which changes
related to each other and to items from the list above. It would have
been easier to reason about the changes had they been done in separate
patches. Still, though, I'm not sure it's worth the time and effort to
split them up -- but I wouldn't complain if you did.)
@@ -8,8 +8,10 @@ test_description='git rebase interactive fixup options This test checks the "fixup [-C|-c]" command of rebase interactive. In addition to amending the contents of the commit, "fixup -C" replaces the original commit message with the message of the fixup-commit. "fixup -c" also replaces the original message, but opens the-editor to allow the user to edit the message before committing.+commit and similar to "fixup" command that works with "fixup!", "fixup -C"+works with "amend!" upon --autosquash. "fixup -c" also replaces the original+message, but opens the editor to allow the user to edit the message before+committing. '
I had trouble digesting this run-on sentence due, I think, to the
mixing of thoughts. It might be easier to understand if you first talk
only about the options to `fixup` (-c/-C), and then, as a separate
sentence, talk about how `amend!` is transformed into `fixup -C` (like
`fixup!` is transformed into `fixup`). However, as this is just minor
descriptive text in a test file, not user-facing documentation, I'm
not sure it matters enough to warrant a re-roll.
Okay, will change it.
quoted
test_commit_message () {
+ git show --no-patch --pretty=format:%B "$1" >actual &&
+ case "$2" in
+ -m) echo "$3" >expect &&
+ test_cmp expect actual ;;
+ *) test_cmp "$2" actual ;;
+ esac
}
The funky indentation here is due to a mix of tabs and spaces. It
should use tabs exclusively.
On Mon, 8 Feb 2021 at 00:24, Eric Sunshine [off-list ref] wrote:
On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta [off-list ref] wrote:
quoted
In the test scripts, the here-doc body and EOF are indented the same
amount as the command which opened the here-doc. Let's remove
one level of indendation.
s/indendation/indentation/
My spelling mistake, I will fix it.
I found "In the test scripts" ambiguous. It isn't clear if you are
talking about all test scripts or the script(s) this patch is fixing
up. Sp, if you happen to re-roll for some reason, perhaps clarify by
saying something like:
The most common way to format here-docs in Git test scripts is for
the body and EOF to be indented the same amount as the command
which opened the here-doc. Fix a few here-docs in this script to
conform to that standard.
Okay, will change in the above way.
quoted
Original-patch-by: Phillip Wood [off-list ref]
I don't think this new patch is based upon Phillip's, so you can
probably drop this attribution.
On Mon, 8 Feb 2021 at 00:28, Eric Sunshine [off-list ref] wrote:
On Sun, Feb 7, 2021 at 1:18 PM Charvi Mendiratta [off-list ref] wrote:
quoted
This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
improves it. It fixup the source code of 'fixup [-C | -c]' command in the
sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
documentation.
Thanks for working on this. I looked over the entire series and left a
few minor comments. As mentioned in my [6/7] review, you might also
want to consider splitting that patch into several patches (though
it's not clear if the extra work of doing so is warranted). Anyhow,
aside from some botched indentation in [6/7], it all looked clean.
Thanks for the corrections. I admit there are few silly mistakes, will
fixup all and
also split [6/7] in the next version.
Thanks and Regards,
Charvi
Hi Chariv and Eric
On 07/02/2021 18:54, Eric Sunshine wrote:
On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta [off-list ref] wrote:
quoted
In the test scripts, the here-doc body and EOF are indented the same
amount as the command which opened the here-doc. Let's remove
one level of indendation.
s/indendation/indentation/
I found "In the test scripts" ambiguous. It isn't clear if you are
talking about all test scripts or the script(s) this patch is fixing
up. Sp, if you happen to re-roll for some reason, perhaps clarify by
saying something like:
The most common way to format here-docs in Git test scripts is for
the body and EOF to be indented the same amount as the command
which opened the here-doc. Fix a few here-docs in this script to
conform to that standard.
quoted
Original-patch-by: Phillip Wood [off-list ref]
I don't think this new patch is based upon Phillip's, so you can
probably drop this attribution.
Good point - well spotted as ever Eric
Thanks
Phillip
This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
improves it. It fixup the source code of 'fixup [-C | -c]' command in the
sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
documentation.
Changes from v1 :
* Splits the patch 'update test-script', to make it more easy to follow.
* Modification in few commit messages as suggested by Eric.
Charvi Mendiratta (11):
sequencer: fixup the datatype of the 'flag' argument
sequencer: rename a few functions
rebase -i: clarify and fix 'fixup -c' rebase-todo help
t/lib-rebase: change the implementation of commands with options
t/t3437: fix indentation of the here-doc
t/t3437: remove the dependency of 'expected-message' file from tests
t/t3437: check author date of the fixed up commit
t/t3437: simplify and document the test helpers
t/t3437: cleanup the 'setup' test and use named commits in the tests
t/t3437: fixup the test 'multiple fixup -c opens editor once'
doc/rebase -i: fix typo in the documentation of 'fixup' command
Documentation/git-rebase.txt | 2 +-
rebase-interactive.c | 6 +-
sequencer.c | 23 +++---
t/lib-rebase.sh | 8 +-
t/t3437-rebase-fixup-options.sh | 140 +++++++++++++++++---------------
5 files changed, 94 insertions(+), 85 deletions(-)
--
2.29.0.rc1
As 'flag' is a combination of bits, so change its datatype from
'enum todo_item_flags' to 'unsigned'.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Rename functions to make them more descriptive and while at it, remove
unnecessary 'inline' of the skip_fixupish() function.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
When `-c` says "edit the commit message" it's not clear what will be
edited. The original's commit message or the replacement's message or a
combination of the two. Word it such that it states more precisely what
exactly will be edited. While at it, also drop the jarring period and
capitalization, neither of which is otherwise present in the message.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
rebase-interactive.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -45,8 +45,8 @@ void append_todo_help(int command_count,"e, edit <commit> = use commit, but stop for amending\n""s, squash <commit> = use commit, but meld into previous commit\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 the commit message\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"
@@ -55,7 +55,7 @@ void append_todo_help(int command_count,"m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"". create a merge commit using the original merge commit's\n"". message (or the oneline, if no original merge commit was\n"-". specified). Use -c <commit> to reword the commit message.\n"+". specified); use -c <commit> to reword the commit message\n""\n""These lines can be re-ordered; they are executed from top to bottom.\n");unsignededit_todo=!(shortrevisions&&shortonto);
"fixup" and "merge" mirrors the implementation of FAKE_LINES handling of
"exec", 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 very different from "exec". 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". Let's replace it with simpler tokens such as "fixup-C"
and "fixup-c".
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/lib-rebase.sh | 8 ++++----
t/t3437-rebase-fixup-options.sh | 18 +++++++++---------
2 files changed, 13 insertions(+), 13 deletions(-)
@@ -15,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.#
The most common way to format here-docs in Git test scripts is for the
body and EOF to be indented the same amount as the command which opened
the here-doc. Fix a few here-docs in this script to conform to that
standard.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 62 ++++++++++++++++-----------------
1 file changed, 31 insertions(+), 31 deletions(-)
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.
Let's set up "expected-message" to the precise content needed by the
test, so that both the problems go away and also makes easier to run
tests selectively with '--run' or 'GIT_SKIP_TESTS'
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Add '%at' format in the get_author() function and update the test to check
that the author date of the fixed up commit is unchanged.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
In the test, FAKE_COMMIT_MESSAGE replaces the commit message each
time it is invoked so there will be only one instance of "Modified-A3"
no matter how many times we invoke the editor. Let's fix this and use
FAKE_COMMIT_AMEND instead so that it adds "Modified-A3" once for each
time the editor is invoked.
This patch also removes the check for counting the number of
"Modified-A3" lines and instead compares the whole message to check
that the commenting code works correctly for 'fixup -c' as well as
'fixup -C'.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Let's simplify the test_commit_message() helper function and add
comments to the function.
This patch also document the working of 'fixup -C' with "amend!" in the
test-description.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
@@ -9,7 +9,9 @@ This test checks the "fixup [-C|-c]" command of rebase interactive. Inadditiontoamendingthecontentsofthecommit,"fixup -C" replacestheoriginalcommitmessagewiththemessageofthefixup commit."fixup -c"alsoreplacestheoriginalmessage,butopensthe-editortoallowtheusertoeditthemessagebeforecommitting.+editortoallowtheusertoeditthemessagebeforecommitting.Similar+tothe"fixup"commandthatworkswith"fixup!","fixup -C"workswith+"amend!"upon--autosquash.' ../test-lib.sh
@@ -18,17 +20,19 @@ editor to allow the user to edit the message before committing.EMPTY=""+# test_commit_message <rev> -m <msg>+# test_commit_message <rev> <path>+# Verify that the commit message of <rev> matches+# <msg> or the content of <path>. test_commit_message(){-rev="$1"&&# commit or tag we want to test-file="$2"&&# test against the content of a file-gitshow--no-patch--pretty=format:%B"$rev">actual-message&&-iftest"$2"=-m-then-str="$3"&&# test against a string-printf"%s\n""$str">tmp-expected-message&&-file="tmp-expected-message"-fi-test_cmp"$file"actual-message+gitshow--no-patch--pretty=format:%B"$1">actual&&+case"$2"in+-m)+echo"$3">expect&&+test_cmpexpectactual;;+*)+test_cmp"$2"actual;;+esac} get_author(){
Remove unnecessary curly braces and use the named commits in the
tests so that they will still refer to the same commit if the setup
gets changed in the future whereas 'branch~2' will change which commit
it points to.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 40 +++++++++++++++++----------------
1 file changed, 21 insertions(+), 19 deletions(-)
@@ -894,7 +894,7 @@ 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 last last one is used. You can also use+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.
This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
improves it. It fixup the source code of 'fixup [-C | -c]' command in the
sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
documentation.
Changes from v2 :
* Update the rebase-todo help
* Remove the changes and resets to fixup_-* command
* Update the documentation of FAKE_LINES
* Move the changes of "unnecessary curly braces in test" to the other patch
(from v2-9/11 to v2-5/11)
Thanks all for the suggestions.
Charvi Mendiratta (11):
sequencer: fixup the datatype of the 'flag' argument
sequencer: rename a few functions
rebase -i: clarify and fix 'fixup -c' rebase-todo help
t/lib-rebase: update the documentation of FAKE_LINES
t/t3437: fixup here-docs in the 'setup' test
t/t3437: remove the dependency of 'expected-message' file from tests
t/t3437: check the author date of fixed up commit
t/t3437: simplify and document the test helpers
t/t3437: use named commits in the tests
t/t3437: fixup the test 'multiple fixup -c opens editor once'
doc/rebase -i: fix typo in the documentation of 'fixup' command
Documentation/git-rebase.txt | 2 +-
rebase-interactive.c | 9 +--
sequencer.c | 23 +++---
t/lib-rebase.sh | 7 +-
t/t3437-rebase-fixup-options.sh | 122 +++++++++++++++++---------------
5 files changed, 87 insertions(+), 76 deletions(-)
--
2.29.0.rc1
As 'flag' is a combination of bits, so change its datatype from
'enum todo_item_flags' to 'unsigned'.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
When `-c` says "edit the commit message" it's not clear what will be
edited. The original's commit message or the replacement's message or a
combination of the two. Word it such that it states more precisely what
exactly will be edited. While at it, also drop the jarring period and
capitalization, neither of which is otherwise present in the message.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
rebase-interactive.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
@@ -44,9 +44,10 @@ 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 [-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 the commit message\n"+"f, fixup [-C | -c] <commit> = like \"squash\" but keep only the previous\n"+" commit's log message, unless -C is used, in which case\n"+" keep only this commit's message; -c is same as -C but\n"+" opens the editor\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"
@@ -55,7 +56,7 @@ void append_todo_help(int command_count,"m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"". create a merge commit using the original merge commit's\n"". message (or the oneline, if no original merge commit was\n"-". specified). Use -c <commit> to reword the commit message.\n"+". specified); use -c <commit> to reword the commit message\n""\n""These lines can be re-ordered; they are executed from top to bottom.\n");unsignededit_todo=!(shortrevisions&&shortonto);
The most common way to format here-docs in Git test scripts is for the
body and EOF to be indented the same amount as the command which opened
the here-doc. Fix a few here-docs in this script to conform to that
standard and also remove the unnecessary curly braces.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 62 ++++++++++++++++-----------------
1 file changed, 31 insertions(+), 31 deletions(-)
FAKE_LINES helper function use underscore to embed a space in a single
command. Let's document it and also update the list of commands.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/lib-rebase.sh | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
@@ -15,10 +15,11 @@# 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.+# "_" -- add a space, like "fixup_-C" implies "fixup -C" and+# "exec_cmd_with_args" add an "exec cmd with args" line.## "#" -- Add a comment line.#
Rename functions to make them more descriptive and while at it, remove
unnecessary 'inline' of the skip_fixupish() function.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
sequencer.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
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.
Let's set up "expected-message" to the precise content needed by the
test, so that both the problems go away and also makes easier to run
tests selectively with '--run' or 'GIT_SKIP_TESTS'
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Add '%at' format in the get_author() function and update the test to
check that the author date of the fixed up commit is unchanged.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Let's simplify the test_commit_message() helper function and add
comments to the function.
This patch also document the working of 'fixup -C' with "amend!" in the
test-description.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
@@ -9,7 +9,9 @@ This test checks the "fixup [-C|-c]" command of rebase interactive. Inadditiontoamendingthecontentsofthecommit,"fixup -C" replacestheoriginalcommitmessagewiththemessageofthefixup commit."fixup -c"alsoreplacestheoriginalmessage,butopensthe-editortoallowtheusertoeditthemessagebeforecommitting.+editortoallowtheusertoeditthemessagebeforecommitting.Similar+tothe"fixup"commandthatworkswith"fixup!","fixup -C"workswith+"amend!"upon--autosquash.' ../test-lib.sh
@@ -18,17 +20,19 @@ editor to allow the user to edit the message before committing.EMPTY=""+# test_commit_message <rev> -m <msg>+# test_commit_message <rev> <path>+# Verify that the commit message of <rev> matches+# <msg> or the content of <path>. test_commit_message(){-rev="$1"&&# commit or tag we want to test-file="$2"&&# test against the content of a file-gitshow--no-patch--pretty=format:%B"$rev">actual-message&&-iftest"$2"=-m-then-str="$3"&&# test against a string-printf"%s\n""$str">tmp-expected-message&&-file="tmp-expected-message"-fi-test_cmp"$file"actual-message+gitshow--no-patch--pretty=format:%B"$1">actual&&+case"$2"in+-m)+echo"$3">expect&&+test_cmpexpectactual;;+*)+test_cmp"$2"actual;;+esac} get_author(){
Use the named commits in the tests so that they will still refer to the
same commit if the setup gets changed in the future whereas 'branch~2'
will change which commit it points to.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
In the test, FAKE_COMMIT_MESSAGE replaces the commit message each
time it is invoked so there will be only one instance of "Modified-A3"
no matter how many times we invoke the editor. Let's fix this and use
FAKE_COMMIT_AMEND instead so that it adds "Modified-A3" once for each
time the editor is invoked.
This patch also removes the check for counting the number of
"Modified-A3" lines and instead compares the whole message to check
that the commenting code works correctly for 'fixup -c' as well as
'fixup -C'.
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
t/t3437-rebase-fixup-options.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
@@ -894,7 +894,7 @@ 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 last last one is used. You can also use+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.