On Mon, 2015-07-27 at 14:47 -0700, Junio C Hamano wrote:
> David Turner <dturner@xxxxxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html