Re: [PATCH] tests: Remove some direct access to .git/logs

[David Turner <dturner@twopensource.com>]

On Mon, 2015-07-27 at 14:47 -0700, Junio C Hamano wrote:
> 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.  

> >  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.

> >  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.

> > -	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.

> > 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.