Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jul 20, 2021, 21:11 UTC
Message-ID: <20210720211139.3592182-1-jonathantanmy@google.com>
In-Reply-To: <xmqqzguij8nq.fsf@gitster.g>

> OK.  Let's take this more as WIP.

Sounds good.

Show 23 quoted lines

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

Show 23 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

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