From: Jonathan Tan <jonathantanmy@google.com>
To: gitster@pobox.com
Cc: jonathantanmy@google.com, git@vger.kernel.org, iankaz@google.com,
sandals@crustytoothpaste.net, avarab@gmail.com,
emilyshaffer@google.com
Subject: Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks
Date: Tue, 20 Jul 2021 14:11:39 -0700 [thread overview]
Message-ID: <20210720211139.3592182-1-jonathantanmy@google.com> (raw)
In-Reply-To: <xmqqzguij8nq.fsf@gitster.g>
> OK. Let's take this more as WIP.
Sounds good.
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 2a2a03bf76..c2c8596aa9 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -1393,6 +1393,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> > branch_top.buf, reflog_msg.buf, transport,
> > !is_local);
> >
> > + if (hook_should_prompt_suggestions()) {
> > + for (ref = mapped_refs; ref; ref = ref->next) {
> > + if (ref->peer_ref &&
> > + !strcmp(ref->peer_ref->name,
> > + "refs/remotes/origin/suggested-hooks")) {
> > + fprintf(stderr, _("The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\n"
> > + "Run `git hook install all` to install them.\n"));
> > + break;
> > + }
> > + }
>
> Hmph, do we really need to iterate over these mapped refs array? It
> seems to me that it would be vastly simpler to check if that single
> ref exists after "clone" finishes depositing all the refs we are
> supposed to create.
Good point. I'll do that.
> > @@ -1313,6 +1314,22 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)
> > ref_map);
> > transport_unlock_pack(transport);
> > trace2_region_leave("fetch", "consume_refs", the_repository);
> > +
> > + if (hook_should_prompt_suggestions()) {
> > + struct ref *ref;
> > +
> > + for (ref = ref_map; ref; ref = ref->next) {
> > + if (ref->peer_ref &&
> > + !strcmp(ref->peer_ref->name,
> > + "refs/remotes/origin/suggested-hooks") &&
> > + oidcmp(&ref->old_oid, &ref->peer_ref->old_oid)) {
> > + fprintf(stderr, _("The remote has updated its suggested hooks.\n"));
> > + fprintf(stderr, _("Run 'git hook install all' to update.\n"));
> > + break;
>
> Again we _could_ do "remember if we had it and at where, and then
> compare after we fetch", but this _might_ be simpler.
>
> I however do not know if it makes sense to have a separate loop just
> for this. This should be done as a part of store_updated_refs(),
> no?
I'm OK with it in either place, but it seems to me that this is separate
from storing refs - the suggestion of hooks does not influence how the
refs are stored.
> On top of this patch, if we wanted to add yet another "this ref is
> special and pay attention to it when it got updated", it makes a lot
> more sense not to add yet another loop that is a copy of this one in
> consume_refs(), but roll the handling of that new ref into a loop
> that already exists. And for the purpose of such an update, I do
> not see why that "loop that already exists" should not be the one
> that goes over ref_map in store_updated_refs(). For the same
> reason, "the remote-tracking ref 'origin/suggested-hooks' is special"
> can and should go to an existing loop in store_updated_refs(), no?
I think 2 loops makes sense - the existing one to store the refs, and
the new one I introduced in this patch that handles the special ref.
(And the handling of "yet another" special ref would be rolled into the
latter loop.) In this case, I think that these 2 loops should be
independent. For example, if the refs in Git are changed to be stored in
an append-only journal, the former loop would be deleted while the
latter loop remains; and if the refs were to be stored as a sorted
array, the former loop would be retained while the latter loop would be
changed to a binary search.
Having said that, I don't feel strongly about this, so if consensus is
that we should move it into stsore_updated_refs(), that's fine too.
> How does this interact with the "--dry-run" option, by the way?
> What if ref_map proposes to update this suggested-hooks ref, but
> "--atomic" fetch feature finds that some other branches do not
> fast-forward and we end up not updating the suggested-hooks ref,
> either?
Good point - I'll have to check these.
> So, unlike my earlier guess, it _might_ turn out that "remember the
> state before the fetch, see if the fetch actually updated and then
> report" could be simpler to design and implement. I dunno.
That's true. Right now I'm using the refmap data structure, but perhaps
it's better to consult the ref from the ref backend before and after the
fetch.
Overall, thanks for taking a look. If you have time, I would also
appreciate comments on the overall idea of this series (the concept of
remote-suggested hooks in general, the way that the user finds out about
them, and how the user can make use of them). (Same message to other
potential reviewers - general design comments are potentially more
useful, because specific code comments can become moot if there needs to
be a change in the overall design.)
next prev parent reply other threads:[~2021-07-20 21:16 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-16 23:31 [RFC PATCH 0/2] MVP implementation of remote-suggested hooks Jonathan Tan
2021-06-16 23:31 ` [RFC PATCH 1/2] hook: move list of hooks Jonathan Tan
2021-06-18 20:59 ` Emily Shaffer
2021-06-18 21:48 ` Jonathan Tan
2021-06-16 23:31 ` [RFC PATCH 2/2] clone,fetch: remote-suggested auto-updating hooks Jonathan Tan
2021-06-18 21:32 ` Emily Shaffer
2021-06-17 1:30 ` [RFC PATCH 0/2] MVP implementation of remote-suggested hooks Junio C Hamano
2021-06-18 21:46 ` Jonathan Tan
2021-06-18 20:57 ` Emily Shaffer
2021-06-18 21:58 ` Jonathan Tan
2021-06-18 22:32 ` Randall S. Becker
2021-06-19 7:58 ` Matt Rogers
2021-06-21 18:37 ` Jonathan Tan
2021-06-20 19:51 ` Ævar Arnfjörð Bjarmason
2021-06-21 18:58 ` Jonathan Tan
2021-06-21 19:35 ` Ævar Arnfjörð Bjarmason
2021-06-22 1:27 ` Jonathan Tan
2021-06-22 0:40 ` brian m. carlson
2021-06-23 22:58 ` Jonathan Tan
2021-06-24 23:11 ` brian m. carlson
2021-06-28 23:12 ` Junio C Hamano
2021-07-16 17:57 ` [RFC PATCH v2 " Jonathan Tan
2021-07-16 17:57 ` [RFC PATCH v2 1/2] hook: move list of hooks Jonathan Tan
2021-07-16 17:57 ` [RFC PATCH v2 2/2] hook: remote-suggested hooks Jonathan Tan
2021-07-19 21:28 ` Junio C Hamano
2021-07-20 21:11 ` Jonathan Tan [this message]
2021-07-20 21:28 ` Phil Hord
2021-07-20 21:56 ` Jonathan Tan
2021-07-20 20:55 ` Ævar Arnfjörð Bjarmason
2021-07-20 21:48 ` Jonathan Tan
2021-07-27 0:57 ` Emily Shaffer
2021-07-27 1:29 ` Junio C Hamano
2021-07-27 21:39 ` Jonathan Tan
2021-07-27 22:40 ` Junio C Hamano
2021-07-19 21:06 ` [RFC PATCH v2 0/2] MVP implementation of " Junio C Hamano
2021-07-20 20:49 ` Jonathan Tan
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210720211139.3592182-1-jonathantanmy@google.com \
--to=jonathantanmy@google.com \
--cc=avarab@gmail.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=iankaz@google.com \
--cc=sandals@crustytoothpaste.net \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 NNTP newsgroup(s).