Hi Dscho, On Mon, May 21, 2018 at 11:41 AM, Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote: > > On Mon, 21 May 2018, Christian Couder wrote: > >> From: David Turner <dturner@xxxxxxxxxxxxxxxx> > > I vaguely remember that Dave suggested using a different email address > these days... > > *clicketyclick* > > ah, yes: > https://public-inbox.org/git/1474935093-26757-3-git-send-email-dturner@xxxxxxxxxxxx/ Yeah, I actually used "David Turner <novalis@xxxxxxxxxxx>" in the --cc option I gave to `git send-email`... > So maybe update it here, too, to > > From: David Turner <novalis@xxxxxxxxxxx> ...but I thought it was better to keep the original author and Signed-off-by as they are in the original commit: https://github.com/dturner-tw/git/commit/0a3fa7fbd7f99280b5f128be3e1ed04e045da671 Now I am ok to update them if it is preferred. >> So that they work under alternate ref storage backends. >> >> This will be really needed when such alternate ref storage backends are >> developed. But this could already help by making clear to readers that >> some tests do not depend on which ref backend is used. >> >> This patch just takes care of many low hanging fruits. It does not try >> to completely solves the issue. >> >> Signed-off-by: David Turner <dturner@xxxxxxxxxxxxxxxx> >> Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx> >> --- > > This is great. I am all for making the tests better ;-) ;-) >> diff --git a/t/lib-t6000.sh b/t/lib-t6000.sh >> index 3f2d873fec..b8567cdf94 100644 >> --- a/t/lib-t6000.sh >> +++ b/t/lib-t6000.sh >> @@ -4,11 +4,10 @@ mkdir -p .git/refs/tags >> >> >sed.script >> >> -# Answer the sha1 has associated with the tag. The tag must exist in .git/refs/tags >> +# Answer the sha1 has associated with the tag. The tag must exist under refs/tags >> tag () { >> _tag=$1 >> - test -f ".git/refs/tags/$_tag" || error "tag: \"$_tag\" does not exist" >> - cat ".git/refs/tags/$_tag" >> + git rev-parse --verify "refs/tags/$_tag" || error "tag: \"$_tag\" does not exist" > > Line longer than 80 columns... Ok, the 'error "..."' will be on another line in the next version. >> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh >> index 0680dec808..886a9e3b72 100755 >> --- a/t/t5500-fetch-pack.sh >> +++ b/t/t5500-fetch-pack.sh >> @@ -30,7 +30,7 @@ add () { >> test_tick && >> commit=$(echo "$text" | git commit-tree $tree $parents) && >> eval "$name=$commit; export $name" && >> - echo $commit > .git/refs/heads/$branch && >> + git update-ref refs/heads/$branch $commit && > > I think we have to be careful here to quote both "refs/heads/$branch" and > "$commit" here, as a bug that introduces spaces into $commit or $branch > would have been caught earlier, but not now, unless we quote. > > This goes for all introduced `update-ref` calls. Ok, they will all have quoted arguments in the next version. > Maybe even for some `git rev-parse --verify` calls. Ok, I will take a look at that. >> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh >> index 8f5c811dd7..c3b89ae783 100755 >> --- a/t/t9903-bash-prompt.sh >> +++ b/t/t9903-bash-prompt.sh >> @@ -148,7 +148,7 @@ test_expect_success 'prompt - inside .git directory' ' >> test_expect_success 'prompt - deep inside .git directory' ' >> printf " (GIT_DIR!)" >expected && >> ( >> - cd .git/refs/heads && >> + cd .git/objects && >> __git_ps1 >"$actual" >> ) && >> test_cmp expected "$actual" >> -- > > This one looks wrong. > > Upon cursory review, one might be tempted to assume that the file is now > written into .git/objects/ instead of .git/refs/heads/. And the patch > context provided in the email is not enough to see (gawd, I hate > mail-based patch review, it really takes all my Git tools away from me). > The trick is that `actual` points at an absolute path: > > #!/bin/sh > # > # Copyright (c) 2012 SZEDER Gábor > # > > test_description='test git-specific bash prompt functions' > > . ./lib-bash.sh > > . "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh" > > actual="$TRASH_DIRECTORY/actual" > [...] > > So the remaining question (which probably wants to be added to the commit > message together with a hint that `actual` points at an absolute path) is: > Why not `cd .git` instead? I think anywhere inside ".git/" should work, so I guess ".git/refs/heads" was chosen to make sure that adding anything after ".git/" does not change the result. I think I can drop this for now and change it later in its own commit with related explanations. Thanks, Christian.
