Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 28, 2015, 19:00 UTC
Message-ID: <xmqqpp3cds44.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1438107144-24293-3-git-send-email-dturner@twopensource.com>

David Turner <dturner@twopensource.com> writes:

Show 13 quoted lines

> All-caps files like NOTES_MERGE_REF are pseudorefs, and thus are
> per-worktree.  We don't want multiple notes merges happening at once
> (in different worktrees), so we want to make these refs true refs.
>
> So, we lowercase NOTES_MERGE_REF and friends.  That way, backends
> that distinguish between pseudorefs and real refs can correctly
> handle notes merges.
>
> This will also enable us to prevent pseudorefs from being updated by
> the ref update machinery e.g. git update-ref.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---

This seems to do a bit more than what it claims to do. As this kind of changes are very error-prone to review, I did a bulk replace of the three all-caps NOTES_*THING* and compared the result with what this patch gives to spot this:

Show 16 quoted lines

>  	# Fail to finalize merge
>  	test_must_fail git notes merge --commit >output 2>&1 &&
> -	# .git/NOTES_MERGE_* must remain
> -	test -f .git/NOTES_MERGE_PARTIAL &&
> -	test -f .git/NOTES_MERGE_REF &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
> +	# .git/notes_merge_* must remain
> +	git rev-parse --verify notes_merge_partial &&
> +	git rev-parse --verify notes_merge_ref &&
> +	test -f .git/notes_merge_worktree/$commit_sha1 &&
> +	test -f .git/notes_merge_worktree/$commit_sha2 &&
> +	test -f .git/notes_merge_worktree/$commit_sha3 &&
> +	test -f .git/notes_merge_worktree/$commit_sha4 &&

The two "rev-parse --verify" looks semi-sensible [*1*]; notes_merge_partial is all lowercase and it refers to $GIT_DIR/notes_merge_partial, because they are shared across working tree.

But then why are $GIT_DIR/notes_merge_worktree/* still checked with "test -f"? If they are not refs or ref-like things, why should they be downcased? If they are, why not "rev-parse --verify"?

[Footnote]

*1* I say "semi-" sensible, because it looks ugly.  All ref-like
    things immediately below $GIT_DIR/ are all-caps by convention.
    Perhaps it is a better idea to move it under refs/; "everything
    under refs/ is shared across working trees" is probably a much
    better rule than "all caps but HEAD is special".