Re: [PATCH] tests: Remove some direct access to .git/logs
- From
David Turner <dturner@twopensource.com>
- Date
- Jul 27, 2015, 22:56 UTC
- Message-ID
- <1438037765.18134.8.camel@twopensource.com>
- In-Reply-To
- <xmqqlhe1gtnb.fsf@gitster.dls.corp.google.com>
On Mon, 2015-07-27 at 14:47 -0700, Junio C Hamano wrote:
Show 11 quoted lines
> David Turner <dturner@twopensource.com> writes: > > > -: a repository with working tree always has reflog these days... > > -: >.git/logs/refs/heads/master > > +rm -f .git/logs/refs/heads/master > > This looks quite different from how other tests are updated (which > looked a lot more sensible). The original has the effect of (1) > ensuring that the log exists/is enabled for 'master' branch and (2) > that log is emptied. The updated has a quite different effect, > but only when you are using filesystem based backend.
Yes, this is one of the areas where we need to do more work for alternate backends -- for instance, we could provide a "git reflog remove" command to replace that rm. As the commit message notes, this patch is a subset of what's necessary for multiple backends.
There are a number of other tests that e.g. rm -r .git/logs, which we would also need to change for alternate backends.
Show 15 quoted lines
> > test_expect_success \ > > "create $m (logged by touch)" \ > > 'GIT_COMMITTER_DATE="2005-05-26 23:30" \ > > - git update-ref HEAD '"$A"' -m "Initial Creation" && > > + git update-ref --create-reflog HEAD '"$A"' -m "Initial Creation" && > > test '"$A"' = $(cat .git/'"$m"')' > > And this update compensates for the earlier "remove 'master' reflog" > (where it should have been "ensure creation and empty it") by > creating, but it is unclear what would happen when we start using > different ref backends. Earlier we did not remove reflog from the > backend, and we say "create" here---it would hopefully not error > out, but it does not quite feel right. Perhaps create and > immediately prune all instead? That feels like more in line with > the way the other change in this patch do things.
If we create and immediately prune all, we'll lose the initial creation entry, which we presumably want to test.
Show 9 quoted lines
> > test_expect_success 'fails silently when using -q with deleted reflogs' ' > > ref=$(git rev-parse HEAD) && > > - : >.git/logs/refs/test && > > - git update-ref -m "message for refs/test" refs/test "$ref" && > > + git update-ref --create-reflog -m "message for refs/test" refs/test "$ref" && > > I cannot tell without enough pre-context, but I assume that we know > there is no reflog for refs/test when this test piece starts---in > which case this change looks sensible.
Yes, that is correct.
Show 7 quoted lines
> > - tail -n 4 .git/logs/HEAD | > > - sed -e "s/.* //" >actual && > > + git reflog HEAD -n4 | > > This may happen to work but teaches a bad habit to readers. Always > order your command line by starting with dashed-options, then refs > and then pathspecs.
Will fix on reroll.
Show 39 quoted lines
> > diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
> > index 9ac7940..db9774e 100755
> > --- a/t/t7509-commit.sh
> > +++ b/t/t7509-commit.sh
> > @@ -90,22 +90,10 @@ sha1_file() {
> > remove_object() {
> > rm -f $(sha1_file "$*")
> > }
> > -no_reflog() {
> > - cp .git/config .git/config.saved &&
> > - echo "[core] logallrefupdates = false" >>.git/config &&
> > - test_when_finished "mv -f .git/config.saved .git/config" &&
> > -
> > - if test -e .git/logs
> > - then
> > - mv .git/logs . &&
> > - test_when_finished "mv logs .git/"
> > - fi
> > -}
> >
> > test_expect_success '--amend option with empty author' '
> > git cat-file commit Initial >tmp &&
> > sed "s/author [^<]* </author </" tmp >empty-author &&
> > - no_reflog &&
> > sha=$(git hash-object -t commit -w empty-author) &&
> > test_when_finished "remove_object $sha" &&
> > git checkout $sha &&
> > @@ -119,7 +107,6 @@ test_expect_success '--amend option with empty author' '
> > test_expect_success '--amend option with missing author' '
> > git cat-file commit Initial >tmp &&
> > sed "s/author [^<]* </author </" tmp >malformed &&
> > - no_reflog &&
> > sha=$(git hash-object -t commit -w malformed) &&
> > test_when_finished "remove_object $sha" &&
> > git checkout $sha &&
>
> I can understand that .git/logs/ cannot be relied upon when you move
> your ref backend out of filesystem, but that does not quite explain
> why this change makes sense. Puzzled...It turns out that the removed portion of the test is totally unnecessary; the tests are completely unrelated to reflogs.
On the reroll, I'll split this into a separate patch with its own explanation.