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