On Mon, Jul 6, 2020 at 10:27 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > Subject: Re: [PATCH 1/2] Modify pseudo refs through ref backend storage
> > From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>
> 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
