[PATCH] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[PATCH] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Oswald Buddenhagen]

- Make the documentation reflect better what actually happens, and add
  some missing info
- Fix recognition of reset's short command in FAKE_LINES
- Default next action after 'fakesha' to preserving the command instead
  of forcing 'pick'. This is consistent with other "instant-effect"
  keywords.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 t/lib-rebase.sh | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 7ca5b918f0..184b25b427 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"|"fixup_-C"|"fixup_-c", "edit", "reword",
+#       "drop", "merge[_-{c|C}_<SHA1>]", 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_[...]", "break" -- add the specified command.
 #
 #   "#" -- Add a comment line.
 #
@@ -49,7 +52,7 @@ set_fake_editor () {
 	action=\&
 	for line in $FAKE_LINES; do
 		case $line in
-		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
+		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|t|merge|m)
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
@@ -64,7 +67,7 @@ set_fake_editor () {
 		fakesha)
 			test \& != "$action" || action=pick
 			echo "$action XXXXXXX False commit" >> "$1"
-			action=pick;;
+			action=\&;;
 		*)
 			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
 			action=\&;;
-- 
2.40.0.152.g15d061e6df

Re: [PATCH] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Oswald Buddenhagen]

ping!

(also on three other unanswered patches from the same day.)

Re: [PATCH] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> ping!
>
> (also on three other unanswered patches from the same day.)

I would be a wrong person to ping.  

    $ git shortlog -n -s --since=18.months --no-merges master -- t/lib-rebase.sh
         2	Phillip Wood
         1	Derrick Stolee

Running "git blame" on the lines the patch touched may give you
folks who may know better than these two but those who made
contributions long time ago whose contributions still survive to
this day may no longer be around in the project, so your mileage may
vary.  

"git blame" for the line that uses "action=\&" finds 5dcdd740
(t/lib-rebase: prepare for testing `git rebase --rebase-merges`,
2019-07-31) and explains why "pick" was changed to "\&", so adding
the author of that patch to the list of people you ask reviews for
may also be a good idea.

HTH for you to find appropriate reviewers better next time.

Thanks.

Re: [PATCH] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Felipe Contreras]

Junio C Hamano wrote:
> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> 
> > ping!
> >
> > (also on three other unanswered patches from the same day.)
> 
> I would be a wrong person to ping.  
> 
>     $ git shortlog -n -s --since=18.months --no-merges master -- t/lib-rebase.sh
>          2	Phillip Wood
>          1	Derrick Stolee

Also:

https://github.com/felipec/git-related

-- 
Felipe Contreras

Re: [PATCH] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Phillip Wood]

Hi Oswald

On 23/03/2023 16:22, Oswald Buddenhagen wrote:

This project avoids commit messages that are a list of changes instead 
preferring the commit message to explain _why_ the changes are being 
made. The reason for this is that it makes it much easier for a future 
contributor to understand the reasoning behind a particular change. 
Having a list of changes is often a symptom that the commit is trying to 
do more than one thing at once and should be split up. Here there are 
three separate changes all mixed into the same commit.

> - Make the documentation reflect better what actually happens, and add
>    some missing info

Having read the proposed changes to the documentation I'm not sure what 
it is in the original you're objecting to. The script may well process 
each token separately but as far as the user is concerned it appears to 
take either "<cmd> <lineno>" or "<lineno>"

> - Fix recognition of reset's short command in FAKE_LINES

This fix is definitely needed and should be in its own commit in a 
separate series to any of the other changes here so it can be applied 
without being held up by discussions on those other changes. Presumably 
we don't have any users of the short reset command name as it has been 
broken ever since it was introduced in 5dcdd7409a (t/lib-rebase: prepare 
for testing `git rebase --rebase-merges`, 2019-07-31)

> - Default next action after 'fakesha' to preserving the command instead
>    of forcing 'pick'. This is consistent with other "instant-effect"
>    keywords.

I'm not sure what the motivation for this change is. What does it 
facilitate that we cannot do now and why do we want to do whatever that is?

Best Wishes

Phillip

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>   t/lib-rebase.sh | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 7ca5b918f0..184b25b427 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"|"fixup_-C"|"fixup_-c", "edit", "reword",
> +#       "drop", "merge[_-{c|C}_<SHA1>]", 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_[...]", "break" -- add the specified command.
>   #
>   #   "#" -- Add a comment line.
>   #
> @@ -49,7 +52,7 @@ set_fake_editor () {
>   	action=\&
>   	for line in $FAKE_LINES; do
>   		case $line in
> -		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
> +		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|t|merge|m)
>   			action="$line";;
>   		exec_*|x_*|break|b)
>   			echo "$line" | sed 's/_/ /g' >> "$1";;
> @@ -64,7 +67,7 @@ set_fake_editor () {
>   		fakesha)
>   			test \& != "$action" || action=pick
>   			echo "$action XXXXXXX False commit" >> "$1"
> -			action=pick;;
> +			action=\&;;
>   		*)
>   			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
>   			action=\&;;

[PATCH 0/3] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Oswald Buddenhagen]

An update to the documentation, and two minor functional changes that don't
actually change anything given current use cases, and are therefore basically
documentation updates as well.

Oswald Buddenhagen (3):
  t/lib-rebase: set_fake_editor(): fix recognition of reset's short
    command
  t/lib-rebase: set_fake_editor(): handle FAKE_LINES more consistently
  t/lib-rebase: improve documentation of set_fake_editor()

 t/lib-rebase.sh | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

-- 
2.40.0.152.g15d061e6df

[PATCH 1/3] t/lib-rebase: set_fake_editor(): fix recognition of reset's short command

[Oswald Buddenhagen]

... in FAKE_LINES.

This has been broken ever since it was introduced in 5dcdd7409a
(t/lib-rebase: prepare for testing `git rebase --rebase-merges`,
2019-07-31), but it's not actually used, so it's a cosmetic defect
only.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
 t/lib-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 7ca5b918f0..e6179ab529 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -49,7 +49,7 @@ set_fake_editor () {
 	action=\&
 	for line in $FAKE_LINES; do
 		case $line in
-		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
+		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|t|merge|m)
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
-- 
2.40.0.152.g15d061e6df

[PATCH 2/3] t/lib-rebase: set_fake_editor(): handle FAKE_LINES more consistently

[Oswald Buddenhagen]

Default next action after 'fakesha' to preserving the command instead
of forcing 'pick', consistently with other "instant-effect" keywords.
There is no reason why one would want that inconsistency, so this was
clearly just an oversight in commit 5dcdd740 ("t/lib-rebase: prepare
for testing `git rebase --rebase-merges`"). Rectifying it makes the
behavior easier to reason about and document.

This would affect hypothetical "fakesha <n>" sequences where line <n>
already isn't a pick, which currently don't appear.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
 t/lib-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index e6179ab529..9ed87ca7ab 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -64,7 +64,7 @@ set_fake_editor () {
 		fakesha)
 			test \& != "$action" || action=pick
 			echo "$action XXXXXXX False commit" >> "$1"
-			action=pick;;
+			action=\&;;
 		*)
 			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
 			action=\&;;
-- 
2.40.0.152.g15d061e6df

[PATCH 3/3] t/lib-rebase: improve documentation of set_fake_editor()

[Oswald Buddenhagen]

- Make it reflect better what actually happens. This makes it easier to
  fully exploit the possibilities and to modify the code.
- Improve the structure, putting more general info further up.
- Document `fakesha` and `break`.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
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 9ed87ca7ab..184b25b427 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"|"fixup_-C"|"fixup_-c", "edit", "reword",
+#       "drop", "merge[_-{c|C}_<SHA1>]", 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_[...]", "break" -- add the specified command.
 #
 #   "#" -- Add a comment line.
 #
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 1/3] t/lib-rebase: set_fake_editor(): fix recognition of reset's short command

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> ... in FAKE_LINES.
>
> This has been broken ever since it was introduced in 5dcdd7409a
> (t/lib-rebase: prepare for testing `git rebase --rebase-merges`,
> 2019-07-31), but it's not actually used, so it's a cosmetic defect
> only.

Sharp eyes.  And this makes the list match what is actually used and
defined in sequencer.c which is good.

Thanks.

>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
>  t/lib-rebase.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 7ca5b918f0..e6179ab529 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -49,7 +49,7 @@ set_fake_editor () {
>  	action=\&
>  	for line in $FAKE_LINES; do
>  		case $line in
> -		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
> +		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|t|merge|m)
>  			action="$line";;
>  		exec_*|x_*|break|b)
>  			echo "$line" | sed 's/_/ /g' >> "$1";;

Re: [PATCH 2/3] t/lib-rebase: set_fake_editor(): handle FAKE_LINES more consistently

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> Default next action after 'fakesha' to preserving the command instead
> of forcing 'pick', consistently with other "instant-effect" keywords.
> There is no reason why one would want that inconsistency, so this was
> clearly just an oversight in commit 5dcdd740 ("t/lib-rebase: prepare
> for testing `git rebase --rebase-merges`"). Rectifying it makes the
> behavior easier to reason about and document.
>
> This would affect hypothetical "fakesha <n>" sequences where line <n>
> already isn't a pick, which currently don't appear.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
>  t/lib-rebase.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I do recall seeing this change and remember wondering what the
fallout from this change would be.  So relative to the previous
round, the above is a definite improvement to clearly state that no
test that is currently in the codebase is affected by this change.

As to the change itself, I do not much care among (1) what this
patch does, (2) doing nothing, or (3) barf when the action is not a
pick.  At least, having this step separate from other changes like
this round of the series does is a very good thing---when somebody
with more knowledge and stake in what the fake-editor does appears
and explains why forcing pick is a good idea, we can easily revert
only this step.

Will queue as-is together with the other two patches.

Thanks.

> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index e6179ab529..9ed87ca7ab 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -64,7 +64,7 @@ set_fake_editor () {
>  		fakesha)
>  			test \& != "$action" || action=pick
>  			echo "$action XXXXXXX False commit" >> "$1"
> -			action=pick;;
> +			action=\&;;
>  		*)
>  			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
>  			action=\&;;

Re: [PATCH 3/3] t/lib-rebase: improve documentation of set_fake_editor()

[Phillip Wood]

Hi Oswald

Thanks for splitting these patches up. We generally prefer commits 
message to be prose explaining the reason for the change rather than 
bullet points.

On 07/08/2023 18:09, Oswald Buddenhagen wrote:
> - Make it reflect better what actually happens.

> This makes it easier to
>    fully exploit the possibilities and to modify the code.

I don't really see how this follows from the first sentence.

> - Improve the structure, putting more general info further up.

Good idea

> - Document `fakesha` and `break`.

Great

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> 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 9ed87ca7ab..184b25b427 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"|"fixup_-C"|"fixup_-c", "edit", "reword",
> +#       "drop", "merge[_-{c|C}_<SHA1>]", 

There is an inconsistency here in how we document fixup and merge. The 
former explicitly lists all possibilities and the latter uses a more 
compact notation. Note that we use docopt style descriptions for options 
so "{c|C}" would be written as "(c|C)". Also "merge -c/C" takes a 
commitish (which we could write as <rev>" rather than a hex object ID.

 > or "bad" for an invalid command.

This is a useful addition

> -#   "<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_[...]", "break" -- add the specified command.

Something like
	exec[_<command ...>]

would be more accurate I think

I think with a couple of tweaks this would be worthwhile improvement to 
the documentation. Having comprehensive coverage of all the commands is 
very welcome.

Thanks

Phillip

Re: [PATCH 0/3] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Phillip Wood]

On 07/08/2023 18:09, Oswald Buddenhagen wrote:
> An update to the documentation, and two minor functional changes that don't
> actually change anything given current use cases, and are therefore basically
> documentation updates as well.

Thanks for re-rolling. I've left a couple of comments on the third 
patch, I don't have anything to add to Junio's comments on the first two.

Best Wishes

Phillip

> Oswald Buddenhagen (3):
>    t/lib-rebase: set_fake_editor(): fix recognition of reset's short
>      command
>    t/lib-rebase: set_fake_editor(): handle FAKE_LINES more consistently
>    t/lib-rebase: improve documentation of set_fake_editor()
> 
>   t/lib-rebase.sh | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
> 

[PATCH v2 0/1] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Oswald Buddenhagen]

An update to the documentation, and two minor functional changes that don't
actually change anything given current use cases, and are therefore basically
documentation updates as well.

Oswald Buddenhagen (1):
  t/lib-rebase: improve documentation of set_fake_editor()

 t/lib-rebase.sh | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

-- 
2.40.0.152.g15d061e6df

[PATCH v2 1/1] t/lib-rebase: improve documentation of set_fake_editor()

[Oswald Buddenhagen]

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(-)

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.
 #
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v2 1/1] t/lib-rebase: improve documentation of set_fake_editor()

[Junio C Hamano]

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.
>  #

Re: [PATCH v2 0/1] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> An update to the documentation, and two minor functional changes that don't
> actually change anything given current use cases, and are therefore basically
> documentation updates as well.
>
> Oswald Buddenhagen (1):
>   t/lib-rebase: improve documentation of set_fake_editor()
>
>  t/lib-rebase.sh | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)

Now I lost track.  This is slightly different from one of the steps
in the three-patch series.  Were the other two steps retracted?

My time quota today to pick up newly sent patches has run out, and
we will be in pre-release freeze period any time now, so there is no
need for an immediate response, but please help readers easily see
which ones are proposed updates that are still surviving.

Thanks.

Re: [PATCH v2 0/1] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Oswald Buddenhagen]

On Wed, Aug 09, 2023 at 02:15:22PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> An update to the documentation, and two minor functional changes that don't
>> actually change anything given current use cases, and are therefore basically
>> documentation updates as well.
>>
>> Oswald Buddenhagen (1):
>>   t/lib-rebase: improve documentation of set_fake_editor()
>>
>>  t/lib-rebase.sh | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>
>Now I lost track.  This is slightly different from one of the steps
>in the three-patch series.  Were the other two steps retracted?
>
no, this cover letter was a messup on my side, caused by a lack of 
attention and still suboptimal tooling. this was meant to be an update 
to just this one commit, while keeping the other two intact.

regards

Re: [PATCH v2 1/1] t/lib-rebase: improve documentation of set_fake_editor()

[Phillip Wood]

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

Re: [PATCH v2 1/1] t/lib-rebase: improve documentation of set_fake_editor()

[Junio C Hamano]

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 ;-)

Re: [PATCH v2 0/1] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> On Wed, Aug 09, 2023 at 02:15:22PM -0700, Junio C Hamano wrote:
>>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> An update to the documentation, and two minor functional changes that don't
>>> actually change anything given current use cases, and are therefore basically
>>> documentation updates as well.
>>>
>>> Oswald Buddenhagen (1):
>>>   t/lib-rebase: improve documentation of set_fake_editor()
>>>
>>>  t/lib-rebase.sh | 21 ++++++++++++---------
>>>  1 file changed, 12 insertions(+), 9 deletions(-)
>>
>>Now I lost track.  This is slightly different from one of the steps
>>in the three-patch series.  Were the other two steps retracted?
>>
> no, this cover letter was a messup on my side, caused by a lack of
> attention and still suboptimal tooling. this was meant to be an update
> to just this one commit, while keeping the other two intact.

I see.  It is a bit too late for today's integration cycle to
resurrect the other two I have discarded, because I have other
things to do including the -rc1 release engineering, but I can
easily go back to the list archive.

For future reference, in this project, we do not generally replace
only a single patch in a three-patch series [*].  We do not want to
deal with a mixture of [PATCH v1 1/3], [PATCH v3 2/3], [PATCH v2
3/3], especially since during the evolution of a series, new patches
may become needed, a patch may become split into two, etc.  Instead
everything gets the new iteration number, i.e. v1 and v2 of patches
1/3 and 3/3 may be identical and only 2/3 may have differences
between its v1 and v2.  And that is perfectly expected around here.

Thanks.


[Footnote]

 * Of course there are execeptions.  When it is obvious to everybody
   that the series is more or less done and all things that need to
   be discussed have been discussed during the review, and the
   review conclusion is that everything in v4 patch is good except
   for this minor change necessary in one patch, it would be a good
   approach to send just a single message, saying "here is to
   replace step 2 of the 7 patches" under the three-dash line and
   marking it as [PATCH v5 2/7] (or "v4bis" or any other marking
   that makes it clear it is the "latest").

Re: [PATCH v2 0/1] t/lib-rebase: (mostly) cosmetic improvements to set_fake_editor()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> no, this cover letter was a messup on my side, caused by a lack of
>> attention and still suboptimal tooling. this was meant to be an update
>> to just this one commit, while keeping the other two intact.
>
> I see.  It is a bit too late for today's integration cycle to
> resurrect the other two I have discarded, because I have other
> things to do including the -rc1 release engineering, but I can
> easily go back to the list archive.

Now I did, so instead of queuing this as a replacement of the three,
one of the three from the earlier has been replaced with this, and
the push-out of tomorrow will have them in 'seen'.

This piece-meal replacement may break threading on the mailing list
but if we ever need v3 and later for this topic, we will see the
entire set resent (hopefully), so the problem will correct itself.
Also if everybody is happy with all three patches, we may not need
v3 ;-)

Thanks.