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

From: Junio C Hamano <[email-masked]>
Date: Mon, 07 Aug 2023 12:09:05 -0700

Oswald Buddenhagen <[email-masked]> 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 <[email-masked]>
> ---
> Cc: Phillip Wood <[email-masked]>
> ---
>  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=\&;;