git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Han-Wen Nienhuys <hanwenn@gmail.com>,
	Han-Wen Nienhuys <hanwen@google.com>
Subject: Re: [PATCH 1/2] Modify pseudo refs through ref backend storage
Date: Mon, 06 Jul 2020 13:26:55 -0700	[thread overview]
Message-ID: <xmqqimf09ypc.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <6821f57bdf326f161f152a8af0e47b54513c77b1.1594056572.git.gitgitgadget@gmail.com> (Han-Wen Nienhuys via GitGitGadget's message of "Mon, 06 Jul 2020 17:29:31 +0000")

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH 1/2] Modify pseudo refs through ref backend storage
> From: Han-Wen Nienhuys <hanwen@google.com>

With what definition of "pseudo refs" has this change been made?
Those things like HEAD, CHERRY_PICK_HEAD, FETCH_HEAD etc. that have
traditionally been written as a plain text file in $GIT_DIR and are
used to name objects by having a full object name in it?  

Or the entities that behave like refs and stored in ref backends,
with all-uppercase-names but do not sit inside refs/ hierarchy?

I think it is OK (and possibly a good move in the longer term, but
that is just my gut feeling) to make ref backends resopnsible for
enumerating, reading and writing them (i.e. I think we want to use
the latter definition for the longer term health of the project).
And we would want to ...

> The previous behavior was introduced in commit 74ec19d4be
> ("pseudorefs: create and use pseudoref update and delete functions",
> Jul 31, 2015), with the justification "alternate ref backends still
> need to store pseudorefs in GIT_DIR".

... declare that justification invalid for that purpose.

Is that what is going on?  I just want to make sure I am following
your flow of thought.

> Refs such as REBASE_HEAD are read through the ref backend. This can
> only work consistently if they are written through the ref backend as
> well. Tooling that works directly on files under .git should be
> updated to use git commands to read refs instead.

OK.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6516c7bc8c..9951c2e403 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1228,7 +1228,6 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
>  
>  	for (i = 0; i < refnames->nr; i++) {
>  		const char *refname = refnames->items[i].string;
> -
>  		if (refs_delete_ref(&refs->base, msg, refname, NULL, flags))
>  			result |= error(_("could not remove reference %s"), refname);
>  	}

Unreleated change?

> @@ -2436,7 +2435,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>  	update->backend_data = lock;
>  
>  	if (update->type & REF_ISSYMREF) {
> -		if (update->flags & REF_NO_DEREF) {
> +		if (update->flags & REF_NO_DEREF ||
> +		    (ref_type(update->refname) == REF_TYPE_PSEUDOREF &&
> +		     strcmp(update->refname, "HEAD"))) {
>  			/*
>  			 * We won't be reading the referent as part of
>  			 * the transaction, so we have to read it here

The old "if we are not dereferencing" condition in if() exactly
matched the comment, but the condition in if() after this change is
not "if we are not dereferencing".  Even if we are dereferencing,
under some new condition, we would still drop into this block and do
not follow the "else" side that creates a new update for the
referent.  Is this part of "modify pseudo refs via backend" topic,
or should it be a separate modification?  Why is this change needed?

It seems that no matter where in the refs/ hierarchy (or even
outside) a symbolic ref resides, the way to update itself (not the
referent through the symbolic ref) should be the same, which is what
the original says, and we want to change that reasoning here, but it
is not quite clear to me why.

> @@ -2782,8 +2783,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
>  		struct ref_update *update = transaction->updates[i];
>  		struct ref_lock *lock = update->backend_data;
>  
> -		if (update->flags & REF_NEEDS_COMMIT ||
> -		    update->flags & REF_LOG_ONLY) {
> +		if ((ref_type(lock->ref_name) != REF_TYPE_PSEUDOREF ||
> +		     !strcmp(lock->ref_name, "HEAD")) &&
> +		    (update->flags & REF_NEEDS_COMMIT ||
> +		     update->flags & REF_LOG_ONLY)) {

And this one stops the files backend from touching all pseudorefs
other than HEAD with this codepath.  That somehow feels totally
opposite from what the log message explained above---weren't we
updating the code to write these pseudorefs through the individual
backends, which the files backend is one example of?  Isn't this
change stopping the backend from writing the pseudorefs other than
HEAD instead?

Puzzled.

>  			if (files_log_ref_write(refs,
>  						lock->ref_name,
>  						&lock->old_oid,


> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 27171f8261..6b8030e8fe 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -476,7 +476,7 @@ test_expect_success 'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER
>  test_expect_success 'given old value for missing pseudoref, do not create' '
>  	test_must_fail git update-ref PSEUDOREF $A $B 2>err &&
>  	test_path_is_missing .git/PSEUDOREF &&

The reason why I asked what this patch thinks the definition of
pseudoref is is because of this thing.  Shouldn't this line be fixed
not to depend on the files backend?  Likewise for $(cat .git/PSEUDOREF)
in the remaining two hunks.

> -	test_i18ngrep "could not read ref" err
> +	test_i18ngrep "unable to resolve reference" err
>  '
>  
>  test_expect_success 'create pseudoref' '
> @@ -497,7 +497,7 @@ test_expect_success 'overwrite pseudoref with correct old value' '
>  test_expect_success 'do not overwrite pseudoref with wrong old value' '
>  	test_must_fail git update-ref PSEUDOREF $D $E 2>err &&
>  	test $C = $(cat .git/PSEUDOREF) &&
> -	test_i18ngrep "unexpected object ID" err
> +	test_i18ngrep "cannot lock ref.*expected" err
>  '
>  
>  test_expect_success 'delete pseudoref' '
> @@ -509,7 +509,7 @@ test_expect_success 'do not delete pseudoref with wrong old value' '
>  	git update-ref PSEUDOREF $A &&
>  	test_must_fail git update-ref -d PSEUDOREF $B 2>err &&
>  	test $A = $(cat .git/PSEUDOREF) &&
> -	test_i18ngrep "unexpected object ID" err
> +	test_i18ngrep "cannot lock ref.*expected" err
>  '
>  
>  test_expect_success 'delete pseudoref with correct old value' '

  reply	other threads:[~2020-07-06 20:27 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-06 17:29 [PATCH 0/2] Remove special casing for PSEUDOREF updates Han-Wen Nienhuys via GitGitGadget
2020-07-06 17:29 ` [PATCH 1/2] Modify pseudo refs through ref backend storage Han-Wen Nienhuys via GitGitGadget
2020-07-06 20:26   ` Junio C Hamano [this message]
2020-07-07 13:56     ` Han-Wen Nienhuys
2020-07-07 15:20       ` Junio C Hamano
2020-07-07 17:15         ` Han-Wen Nienhuys
2020-07-07 18:14           ` Junio C Hamano
2020-07-06 17:29 ` [PATCH 2/2] Make HEAD a PSEUDOREF rather than PER_WORKTREE Han-Wen Nienhuys via GitGitGadget
2020-07-06 20:30   ` Junio C Hamano
2020-07-07  9:24     ` Han-Wen Nienhuys
2020-07-09 21:11 ` [PATCH v2 0/3] Remove special casing for PSEUDOREF updates Han-Wen Nienhuys via GitGitGadget
2020-07-09 21:11   ` [PATCH v2 1/3] t1400: use git rev-parse for testing PSEUDOREF existence Han-Wen Nienhuys via GitGitGadget
2020-07-09 21:11   ` [PATCH v2 2/3] Modify pseudo refs through ref backend storage Han-Wen Nienhuys via GitGitGadget
2020-07-09 21:11   ` [PATCH v2 3/3] Make HEAD a PSEUDOREF rather than PER_WORKTREE Han-Wen Nienhuys via GitGitGadget
2020-07-10 19:25   ` [PATCH v2 0/3] Remove special casing for PSEUDOREF updates Junio C Hamano
2020-07-16 18:45   ` [PATCH v3 " Han-Wen Nienhuys via GitGitGadget
2020-07-16 18:45     ` [PATCH v3 1/3] t1400: use git rev-parse for testing PSEUDOREF existence Han-Wen Nienhuys via GitGitGadget
2020-07-16 18:45     ` [PATCH v3 2/3] Modify pseudo refs through ref backend storage Han-Wen Nienhuys via GitGitGadget
2020-07-17  8:52       ` Johannes Schindelin
2020-07-27 15:41         ` Han-Wen Nienhuys
2020-07-16 18:45     ` [PATCH v3 3/3] Make HEAD a PSEUDOREF rather than PER_WORKTREE Han-Wen Nienhuys via GitGitGadget
2020-07-16 22:09     ` [PATCH v3 0/3] Remove special casing for PSEUDOREF updates Junio C Hamano
2020-07-27  8:50       ` Han-Wen Nienhuys
2020-07-27 16:20         ` Junio C Hamano
2020-08-03 19:07           ` Han-Wen Nienhuys
2020-08-03 20:07             ` Junio C Hamano
2020-08-05  1:45               ` Junio C Hamano
2020-08-05 10:48                 ` Han-Wen Nienhuys
2020-08-05 15:54                   ` Junio C Hamano
2020-08-10 14:27                     ` Han-Wen Nienhuys
2020-08-10 16:04                       ` Junio C Hamano
2020-08-11 10:49                         ` Han-Wen Nienhuys
2020-08-11 18:38                           ` Junio C Hamano
2020-08-19 13:19                     ` Han-Wen Nienhuys
2020-08-19 15:52                       ` Junio C Hamano
2020-07-27 16:25     ` [PATCH v4 " Han-Wen Nienhuys via GitGitGadget
2020-07-27 16:25       ` [PATCH v4 1/3] t1400: use git rev-parse for testing PSEUDOREF existence Han-Wen Nienhuys via GitGitGadget
2020-07-27 16:25       ` [PATCH v4 2/3] Modify pseudo refs through ref backend storage Han-Wen Nienhuys via GitGitGadget
2020-07-27 16:25       ` [PATCH v4 3/3] Make HEAD a PSEUDOREF rather than PER_WORKTREE Han-Wen Nienhuys via GitGitGadget

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqimf09ypc.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=hanwen@google.com \
    --cc=hanwenn@gmail.com \
    --subject='Re: [PATCH 1/2] Modify pseudo refs through ref backend storage' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git