Re: [PATCH v4 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase

> On Fri, Jan 29, 2021 at 1:25 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> [...] 
>> +       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? 

> 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.
> 
>> +       echo B1 >B &&
>> +       test_tick &&
>> +       git commit --fixup=HEAD -a &&
>> +       test_tick &&
> 
> Same question about whether the commit timestamps have any
> significance in these tests.

> 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.
> 
>> +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.
> 
>> +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.
> 
>> +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?
> 
>> +       git checkout --detach A1 &&
>> +       test_must_fail env FAKE_LINES="1 fixup_-C 2" git rebase -i conflicts &&
>> +       git checkout --theirs -- A &&
>> +       git add A &&
>> +       FAKE_COMMIT_AMEND=edited git rebase --continue &&
>> +       test_cmp_rev HEAD^ conflicts &&
>> +       test_cmp_rev HEAD^{tree} A1^{tree} &&
>> +       test_write_lines "" edited >>expected-message &&
> 
> 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.
> 
>> +test_expect_success 'multiple fixup -c opens editor once' '
>> +       test_when_finished "test_might_fail git rebase --abort" &&
>> +       git checkout --detach A3 &&
>> +       base=$(git rev-parse HEAD~4) &&
>> +       FAKE_COMMIT_MESSAGE="Modified-A3" \
>> +               FAKE_LINES="1 fixup_-C 2 fixup_-c 3 fixup_-c 4" \
>> +               EXPECT_HEADER_COUNT=4 \
>> +               git rebase -i $base &&
>> +       test_cmp_rev $base HEAD^ &&
>> +       test 1 = $(git show | grep Modified-A3 | wc -l)
>> +'
> 
> These days, we would phrase the last part of the test as:
> 
>      git show > raw &&
>      grep Modified-A3 raw >out &&
>      test_line_count = 1 out
>