On Fri Sep 20, 2024 at 01:07, Junio C Hamano [off-list ref] wrote:

Bence Ferdinandy [off-list ref] writes:

quoted

If the user has remote/HEAD set already and it looks like it has changed
on the server, then print a message, otherwise set it if we can.

Signed-off-by: Bence Ferdinandy <redacted>
---

Notes:
    v3: - does not rely on remote set-head anymore so it only authenticates
        once
        - uses the new REF_CREATE_ONLY to atomically check if the ref exists
          and only write it if it doesn't
        - in all other cases the maximum it does is print a warning

Except for one thing, I can tell from the above that the design is
sensible without looking at the actual patch.  Nicely described.

The "one thing" is what to do if you normally do *not* keep track of
the remote-tracking refs for this particular remote.

If I am only fetching refs (or HEAD) in FETCH_HEAD for immediate
consumtion by doing "git pull bence HEAD" with something like

    [remote "bence"]
	 URL = http://github.com/bence/git

(with no other configuration under remote.bence.* hierarchy), I do
not think I want the code to create refs/remotes/bence/HEAD, with
no other remote-tracking refs in the same hierarchy, even if it finds
no existing refs/remotes/bence/HEAD there.  For that, I suspect that 
you'd need to see if the pointee of refs/remotes/bence/HEAD either
already exists or this fetch is about to create it, or something
like that, before deciding to create a ref with the REF_CREATE_ONLY
flag.

I'll take another look at this.

If you are doing so already in the code (I haven't started reading
it yet at this moment), you should mention it in your proposed log
message, so discount my earlier "Nicely described" a bit ;-).

:D

Let's see what actually happens in the code now.

quoted

+static const char *abbrev_ref(const char *name, const char *prefix)
+{
+	skip_prefix(name, prefix, &name);
+	return name;
+}
+#define abbrev_branch(name) abbrev_ref((name), "refs/heads/")

We do not call the act of optionally skipping prefix "abbreviate" in
this project (object names are abbreviated but that is done by
chomping the later bytes of a long name).

I suspect that a caller of either of these two functions are
inherently buggy in that they _optionally_ skip the prefix so they
do the same thing for "refs/heads/main" and "main" but not
"heads/main".  The callsites may need to be inspected to see how
they should deal with cases where skip_prefix() did *not* see
anything to skip.

I copied this verbatim from builtin/remote.c
https://github.com/git/git/blob/master/builtin/remote.c#L267 so it looked like
a safe bet T_T I'm not 100% sure, but I think there's no use case where you
pass a ref directly to remote except for set_head, but there it's not used, so
it looks safe?

That being said, I can change the name(s) to not use abbrev here, especially if
you have a good suggestion. Maybe strip_prefix_ref/branch? Again in fetch, we
(should) know what type of refs we're calling it on, so should be safe?

quoted

+static inline int set_head(const struct ref *remote_refs)

Drop "inline".  You do not want to inline this much code if there
are many callers, and if you have only one caller, the compiler
would inline it into the sole caller if it is the more efficient
thing to do.

Ack.

quoted

+{
+	int result, ref_changed = 0;
+	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, b_local_head = STRBUF_INIT, b_prefix = STRBUF_INIT;
+	const char *remote = gtransport->remote->name;
+	char * head_name = NULL;
+	struct ref *ref, *matches;
+	struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
+	struct refspec_item refspec = {
+		.force = 0,
+		.pattern = 1,
+		.src = (char *) "refs/heads/*",
+		.dst = (char *) "refs/heads/*",
+	};
+	struct string_list heads = STRING_LIST_INIT_DUP;
+
+	get_fetch_map(remote_refs, &refspec, &fetch_map_tail, 0);
+	matches = guess_remote_head(find_ref_by_name(remote_refs, "HEAD"),
+				    fetch_map, 1);
+	for (ref = matches; ref; ref = ref->next)
+		string_list_append(&heads, abbrev_branch(ref->name));
+
+
+	if (!heads.nr)
+		result = 1;
+	else if (heads.nr > 1) {
+		result = 1;
+	} else
+		head_name = xstrdup(heads.items[0].string);
+	if (head_name) {
+		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", remote);
+		strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", remote, head_name);
+		strbuf_addf(&b_prefix, "refs/remotes/%s/", remote);
+		if (!refs_read_symbolic_ref(get_main_ref_store(the_repository),b_head.buf,&b_local_head)) {

Overly long lines around here cannot be sanely reviewed on my
terminal, so I'll stop here for now.  

Do not omit SP after ",", as that is the least effective way to
shorten overly long lines.  Instead, consider introducing a
one-time-use temporary variables with meaningful name, e.g.

		struct ref_store *store = get_main_ref_store(the_repository);
                
                strbuf_addf(&b_head, "refs/remotes/%s/HEAD", remote);
		...
		if (!refs_read_symbolic_ref(store, b_head.buf, &current)) {
    			/* it's current value is in "current" */
			...

Ah, yeah terribly sorry about this, I've gotten so used to automatic
formatters, that I just didn't take a second look. I'll be more careful in the
next round.

-- 
bence.ferdinandy.com