Hi Junio, On 2 Mar 2022, at 18:32, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> From: John Cai <johncai86@xxxxxxxxx> >> >> 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@xxxxxxxxx> >> Signed-off-by: John Cai <johncai86@xxxxxxxxx> >> --- >> 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? >> + 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 &&