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
next prev parent 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).