git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Han-Wen Nienhuys <hanwen@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com>,
	git <git@vger.kernel.org>, Han-Wen Nienhuys <hanwenn@gmail.com>
Subject: Re: [PATCH 1/2] Modify pseudo refs through ref backend storage
Date: Tue, 7 Jul 2020 15:56:38 +0200	[thread overview]
Message-ID: <CAFQ2z_OPbcX9FGsFnmraAUpTu2eKuf2PZ+DO-CTWvaBZD6WQQQ@mail.gmail.com> (raw)
In-Reply-To: <xmqqimf09ypc.fsf@gitster.c.googlers.com>

On Mon, Jul 6, 2020 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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 thought these were the same? - see below :)

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

yep.

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

oops.

> > @@ -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?

There is a  test (mentioned in the commit message), t1405, which does

$RUN create-symref FOO refs/heads/master nothing &&
# calls refs_create_symref()
..
$RUN delete-refs 0 nothing FOO refs/tags/new-tag &&
# calls refs_delete_refs, which calls delete_pseudoref
..
git rev-parse master >expected &&

Previously, the delete_pseudoref code would delete just .git/FOO file.
Without the change here, going through the ref backend ends up in the
!REF_NO_DEREF case, deleting the master branch.

I don't know what the correct semantics are (are symrefs used for
anything except HEAD ?), so


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

There is a test

  'core.logAllRefUpdates=always creates no reflog for ORIG_HEAD'

that wants there to not be reflog updates for ORIG_HEAD. I don't see
why that behavior exists, and would be happy to change it, but I'm too
much of a newbie to decide what is right here.

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

This patch doesn't introduce reftable yet, so both definitions are
equivalent for the sake of this  patch.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

  reply	other threads:[~2020-07-07 13:57 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
2020-07-07 13:56     ` Han-Wen Nienhuys [this message]
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=CAFQ2z_OPbcX9FGsFnmraAUpTu2eKuf2PZ+DO-CTWvaBZD6WQQQ@mail.gmail.com \
    --to=hanwen@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.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

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

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).