Re: [PATCH 1/2] Modify pseudo refs through ref backend storage

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

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

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