Re: [PATCH v4 1/3] stash: add tests to ensure reflog --rewrite --updatref behavior
Hi Junio,
On 2 Mar 2022, at 18:32, Junio C Hamano wrote:
Show 81 quoted lines
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> There is missing test coverage to ensure that the resulting reflogs
>> after a git stash drop has had its old oid rewritten if applicable, and
>> if the refs/stash has been updated if applicable.
>>
>> Add two tests that verify both of these happen.
>>
>> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: John Cai <johncai86@gmail.com>
>> ---
>> t/t3903-stash.sh | 43 ++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index b149e2af441..a2f8d0c52e7 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -41,7 +41,7 @@ diff_cmp () {
>> rm -f "$1.compare" "$2.compare"
>> }
>>
>> -test_expect_success 'stash some dirty working directory' '
>> +setup_stash() {
>
> Style.
>
> setup_stash () {
>
> but more importantly, is this really setting up "stash"?
> "setup_stash_test" or something, perhaps?
>
>> +test_expect_success 'stash some dirty working directory' '
>> + setup_stash
>> '
>
> OK.
>
>> +test_expect_success 'drop stash reflog updates refs/stash' '
>> + git reset --hard &&
>> + git rev-parse refs/stash >expect &&
>> + echo 9 >file &&
>> + git stash &&
>> + git stash drop stash@{0} &&
>> + git rev-parse refs/stash >actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
>> + git init repo &&
>> + (
>> + cd repo &&
>> + setup_stash
>> + ) &&
>
> Hmph, so this is done inside the subdirectory. The implementation
> of the helper in this iteration does look cleaner than in the
> previous iteration.
>
> But these many references to "repo/" and "-C repo" we see below
> makes me wonder why we do not put the whole thing inside the
> subshell we started earlier.
>
> i.e.
>
> git init repo &&
> (
> cd repo &&
> setup_stash_test &&
>
> echo 9 >file &&
> old=$(git rev-parse stash@{0}) &&
> git stash &&
> new=$(git rev-parse stash@{0}) &&
> ...
>
> test_cmp expect actual
> )
>makes sense to me. Is this worth a re-roll and sending out another series to the list? or is it sufficient to make the change on my branch?
Show 24 quoted lines
>> + echo 9 >repo/file &&
>> +
>> + old_oid="$(git -C repo rev-parse stash@{0})" &&
>> + git -C repo stash &&
>> + new_oid="$(git -C repo rev-parse stash@{0})" &&
>> +
>> + cat >expect <<-EOF &&
>> + $(test_oid zero) $old_oid
>> + $old_oid $new_oid
>> + EOF
>> + cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
>> + test_cmp expect actual &&
>> +
>> + git -C repo stash drop stash@{1} &&
>> + cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
>> + cat >expect <<-EOF &&
>> + $(test_oid zero) $new_oid
>> + EOF
>> + test_cmp expect actual
>> +'
>> +
>> test_expect_success 'stash pop' '
>> git reset --hard &&
>> git stash pop &&John Cai <johncai86@gmail.com>