From: Junio C Hamano <gitster@pobox•com>
To: David Turner <dturner@twopensource•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] tests: Remove some direct access to .git/logs
Date: Mon, 27 Jul 2015 14:47:20 -0700 [thread overview]
Message-ID: <xmqqlhe1gtnb.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1438028034-23609-1-git-send-email-dturner@twopensource.com> (David Turner's message of "Mon, 27 Jul 2015 16:13:54 -0400")
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.
> 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.
> - test_line_count = 4 .git/logs/refs/heads/master
> + git reflog refs/heads/master >output &&
> + test_line_count = 4 output
> '
These all look sensible.
> diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
> index 6f47c0d..d568b35 100755
> --- a/t/t1411-reflog-show.sh
> +++ b/t/t1411-reflog-show.sh
> @@ -138,7 +138,7 @@ test_expect_success '--date magic does not override explicit @{0} syntax' '
> : >expect
> test_expect_success 'empty reflog file' '
> git branch empty &&
> - : >.git/logs/refs/heads/empty &&
> + git reflog expire --expire=all refs/heads/empty &&
This one is what I was talking about in the earlier part of this
message. This looks very sensible.
> 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.
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 467e6c1..dc76754 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -961,13 +961,13 @@ test_expect_success 'rebase -i produces readable reflog' '
> set_fake_editor &&
> git rebase -i --onto I F branch-reflog-test &&
> cat >expect <<-\EOF &&
> - rebase -i (start): checkout I
> - rebase -i (pick): G
> - rebase -i (pick): H
> rebase -i (finish): returning to refs/heads/branch-reflog-test
> + rebase -i (pick): H
> + rebase -i (pick): G
> + rebase -i (start): checkout I
> EOF
;-)
> - 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.
> 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...
next prev parent reply other threads:[~2015-07-27 21:47 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-27 20:13 [PATCH] tests: Remove some direct access to .git/logs David Turner
2015-07-27 21:47 ` Junio C Hamano [this message]
2015-07-27 22:56 ` David Turner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqlhe1gtnb.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=dturner@twopensource$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox