| Message ID | 20230809171531.2564785-2-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor() | expand |
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > Firstly, make it reflect better what actually happens. Not omitting some > possibilities makes it easier to fully exploit them, and not > contradicting the implementation makes it easier to grok and thus modify > the code. > > Secondly, improve the overall structure, putting more general info > further up. > > Thirdly, document `merge`, `fakesha`, and `break`, which were previously > omitted entirely. > > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> > > --- > v2: > - everything phillip asked for, i think. of course, it's still bullet > points, just without the punctuation - i wouldn't know what else to > do about it which would be an actual improvement. If more bits are spent for the explanation of concrete changes (like "move general info first and then describe details") that back subjective claims (e.g. make it "better", "improve" the structure), the result probably would read better. Readers do not have to take your word on it that the result is "better", but at least would understand the reason why you think the result is "better" and can agree (or disagree) more easily. I am not Phillip but to me what you wrote in the above look clear enough. Thanks for working on it. > Cc: Phillip Wood <phillip.wood123@gmail.com> > --- > t/lib-rebase.sh | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh > index 7ca5b918f0..133c856992 100644 > --- a/t/lib-rebase.sh > +++ b/t/lib-rebase.sh > @@ -8,18 +8,21 @@ > # - 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. > -# $FAKE_LINES consists of a sequence of words separated by spaces. > -# The following word combinations are possible: > +# $FAKE_LINES consists of a sequence of words separated by spaces; > +# spaces inside the words are encoded as underscores. > +# The following words are possible: > # > -# "<lineno>" -- add a "pick" line with the SHA1 taken from the > -# specified line. > +# "<cmd>" -- override the command for the next line specification. Can be > +# "pick", "squash", "fixup[_-(c|C)]", "edit", "reword", "drop", > +# "merge[_-{c|C}_<rev>]", or "bad" for an invalid command. > # > -# "<cmd> <lineno>" -- add a line with the specified command > -# ("pick", "squash", "fixup"|"fixup_-C"|"fixup_-c", "edit", "reword" or "drop") > -# and the SHA1 taken from the specified line. > +# "<lineno>" -- add a command, using the specified line as a template. > +# If the command has not been overridden, the line will be copied > +# verbatim, usually resulting in a "pick" line. > # > -# "_" -- add a space, like "fixup_-C" implies "fixup -C" and > -# "exec_cmd_with_args" add an "exec cmd with args" line. > +# "fakesha" -- add a command ("pick" by default), using a fake SHA1. > +# > +# "exec_[command...]", "break" -- add the specified command. > # > # "#" -- Add a comment line. > #
Hi Oswald On 09/08/2023 18:15, Oswald Buddenhagen wrote: > Firstly, make it reflect better what actually happens. Not omitting some > possibilities makes it easier to fully exploit them, and not > contradicting the implementation makes it easier to grok and thus modify > the code. Thanks for expanding this first point, the new message looks fine to me. The code changes look good apart from one change in the that got missed. > -# "<lineno>" -- add a "pick" line with the SHA1 taken from the > -# specified line. > +# "<cmd>" -- override the command for the next line specification. Can be > +# "pick", "squash", "fixup[_-(c|C)]", "edit", "reword", "drop", > +# "merge[_-{c|C}_<rev>]", or "bad" for an invalid command. This is still using "{}" in the merge documentation rather than "()" like the new fixup docs. Thanks Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Oswald > > On 09/08/2023 18:15, Oswald Buddenhagen wrote: >> Firstly, make it reflect better what actually happens. Not omitting some >> possibilities makes it easier to fully exploit them, and not >> contradicting the implementation makes it easier to grok and thus modify >> the code. > > Thanks for expanding this first point, the new message looks fine to > me. The code changes look good apart from one change in the that got > missed. > >> -# "<lineno>" -- add a "pick" line with the SHA1 taken from the >> -# specified line. >> +# "<cmd>" -- override the command for the next line specification. Can be >> +# "pick", "squash", "fixup[_-(c|C)]", "edit", "reword", "drop", >> +# "merge[_-{c|C}_<rev>]", or "bad" for an invalid command. > > This is still using "{}" in the merge documentation rather than "()" > like the new fixup docs. > > Thanks Ah, thanks for sharp eyes. I can locally tweak these two bytes ;-)
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh index 7ca5b918f0..133c856992 100644 --- a/t/lib-rebase.sh +++ b/t/lib-rebase.sh @@ -8,18 +8,21 @@ # - 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. -# $FAKE_LINES consists of a sequence of words separated by spaces. -# The following word combinations are possible: +# $FAKE_LINES consists of a sequence of words separated by spaces; +# spaces inside the words are encoded as underscores. +# The following words are possible: # -# "<lineno>" -- add a "pick" line with the SHA1 taken from the -# specified line. +# "<cmd>" -- override the command for the next line specification. Can be +# "pick", "squash", "fixup[_-(c|C)]", "edit", "reword", "drop", +# "merge[_-{c|C}_<rev>]", or "bad" for an invalid command. # -# "<cmd> <lineno>" -- add a line with the specified command -# ("pick", "squash", "fixup"|"fixup_-C"|"fixup_-c", "edit", "reword" or "drop") -# and the SHA1 taken from the specified line. +# "<lineno>" -- add a command, using the specified line as a template. +# If the command has not been overridden, the line will be copied +# verbatim, usually resulting in a "pick" line. # -# "_" -- add a space, like "fixup_-C" implies "fixup -C" and -# "exec_cmd_with_args" add an "exec cmd with args" line. +# "fakesha" -- add a command ("pick" by default), using a fake SHA1. +# +# "exec_[command...]", "break" -- add the specified command. # # "#" -- Add a comment line. #
Firstly, make it reflect better what actually happens. Not omitting some possibilities makes it easier to fully exploit them, and not contradicting the implementation makes it easier to grok and thus modify the code. Secondly, improve the overall structure, putting more general info further up. Thirdly, document `merge`, `fakesha`, and `break`, which were previously omitted entirely. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- v2: - everything phillip asked for, i think. of course, it's still bullet points, just without the punctuation - i wouldn't know what else to do about it which would be an actual improvement. Cc: Phillip Wood <phillip.wood123@gmail.com> --- t/lib-rebase.sh | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)