From: Martijn van Duren Subject: Re: got: allow clone without symrefs To: gameoftrees@openbsd.org Cc: Stefan Sperling Date: Sat, 14 Jun 2025 22:06:22 +0200 On Sat, 2025-06-14 at 21:01 +0200, Martijn van Duren wrote: > On Sat, 2025-06-14 at 19:24 +0200, Stefan Sperling wrote: > > On Sat, Jun 14, 2025 at 06:46:04PM +0200, Martijn van Duren wrote: > > > This diff works for me, and should make the default branch logic a > > > little more robust. But I'm not sure if the added complexity is worth > > > it: > > > - If we have a symref keep the logic as is. > > > - If we have a "HEAD", try to find a "master" or "main" branch of > > > the same id_str as "HEAD". > > > - If we have a "HEAD", but no id_str matching "master" or "main" branch, > > > default to the first "refs/heads/" branch that matches "HEAD"'s > > > id_str. > > > - If there is no "HEAD", try to find "master" or "main" branch of any > > > id_str. > > > - Give up and let the user use -b. > > > > > > I guess that "master" and "main" are common enough as main branch to > > > keep those as fallback, but let's see what sticks :-) > > > > I would prefer to drop all the checks involving "master" or "main" > > because those names are not part of the protocol spec. Checking for > > those names here looks like a layer violation. It would be an acceptable > > heuristic in scripts, but not in protocol implementations. > > > > Otherwise this logic looks fine to me. > > Since the protocol files only specify symref as a valid option to select > a branch, I think that maybe it would pay to be even more conservative > than your suggestion and only go for cases where there's only one > valid option: > - If we have a symref HEAD keep the logic as is (maybe even verify that > HEAD and the symref value have the same id_str to make sure they're not > out of sync?) > - If there's a "HEAD" without symref make sure there's only one branch > with that id_str. > - If there's no "HEAD" test if there's only a single branch. > > Looking a bit closer I also noticed that the symref case is set > regardless of whether the first line is HEAD or not. So theoretically > it's possible that the firstpkt is "/refs/heads/main", with a symref > line of "HEAD:refs/heads/main", which would then be ignored since > firstpkt does a continue if default_branch is set. But I've left that > out of this patch. Disregard this specific diff for now, I ran into something. Still curious about the concept. > > diff /home/martijn/src/got > path + /home/martijn/src/got > commit - e339aba5a8f332c98d0fa01f1d9a5096e89c6045 > blob - 736c8e342f847a5c286727292fe961ed4fcac388 > file + libexec/got-fetch-pack/got-fetch-pack.c > --- libexec/got-fetch-pack/got-fetch-pack.c > +++ libexec/got-fetch-pack/got-fetch-pack.c > @@ -64,6 +64,12 @@ > #define nitems(_a) (sizeof((_a)) / sizeof((_a)[0])) > #endif > > +enum defaultref { > + DEFAULTREF_UNKNOWN, > + DEFAULTREF_SYM, > + DEFAULTREF_HEAD > +}; > + > struct got_object *indexed; > static int chattygot; > > @@ -347,10 +353,12 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1, > struct got_object_id *have, *want; > int is_firstpkt = 1, nref = 0, refsz = 16; > int i, n, nwant = 0, nhave = 0, acked = 0, eof = 0; > + enum defaultref defaultref = DEFAULTREF_UNKNOWN; > off_t packsz = 0, last_reported_packsz = 0; > char *id_str = NULL, *default_id_str = NULL, *refname = NULL; > char *server_capabilities = NULL, *my_capabilities = NULL; > const char *default_branch = NULL; > + char *free_default_branch = NULL; > struct got_pathlist_head symrefs; > struct got_pathlist_entry *pe; > int sent_my_capabilites = 0, have_sidebands = 0; > @@ -433,8 +441,19 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1, > if (strcmp(name, GOT_REF_HEAD) != 0) > continue; > default_branch = symref_target; > + defaultref = DEFAULTREF_SYM; > break; > } > + if (defaultref == DEFAULTREF_UNKNOWN && > + strcmp(refname, GOT_REF_HEAD) == 0) { > + default_id_str = strdup(id_str); > + if (default_id_str == NULL) { > + err = got_error_from_errno( > + "strdup"); > + goto done; > + } > + defaultref = DEFAULTREF_HEAD; > + } > } > if (default_branch) > continue; > @@ -446,13 +465,67 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1, > } > continue; > } > - if (default_branch && default_id_str == NULL && > - strcmp(refname, default_branch) == 0) { > - default_id_str = strdup(id_str); > - if (default_id_str == NULL) { > - err = got_error_from_errno("strdup"); > + > + switch (defaultref) { > + /* > + * If there's no HEAD, nor symref, and only one branch: > + * use that one > + */ > + case DEFAULTREF_UNKNOWN: > + if (default_branch || default_id_str) { > + err = got_error_msg(GOT_ERR_FETCH_NO_BRANCH, > + "Multiple branches with no HEAD reference"); > goto done; > } > + if (strncmp(refname, "refs/heads/", 11) == 0) { > + free_default_branch = strdup(refname); > + default_id_str = strdup(id_str); > + if (free_default_branch == NULL || > + default_id_str == NULL) { > + err = got_error_from_errno("strdup"); > + goto done; > + } > + default_branch = free_default_branch; > + > + } > + break; > + /* The symref branch always takes precedence */ > + case DEFAULTREF_SYM: > + if (default_branch && default_id_str == NULL && > + strcmp(refname, default_branch) == 0) { > + default_id_str = strdup(id_str); > + if (default_id_str == NULL) { > + err = got_error_from_errno("strdup"); > + goto done; > + } > + } > + break; > + /* > + * If we have a HEAD without symref: use the branch with the > + * matching id_str if there's only one > + */ > + case DEFAULTREF_HEAD: > + if (default_branch && > + strncmp(refname, "refs/heads/", 11) == 0 && > + strcmp(id_str, default_id_str) == 0) { > + static char msg[PATH_MAX + 33]; > + snprintf(msg, sizeof(msg), > + "multiple branches with id \"%s\" found", > + default_id_str); > + err = got_error_msg(GOT_ERR_FETCH_NO_BRANCH, msg); > + goto done; > + } > + if (default_branch == NULL && > + strncmp(refname, "refs/heads/", 11) == 0 && > + strcmp(id_str, default_id_str) == 0) { > + free_default_branch = strdup(refname); > + if (free_default_branch == NULL) { > + err = got_error_from_errno("strdup"); > + goto done; > + } > + default_branch = free_default_branch; > + } > + break; > } > > if (list_refs_only || strncmp(refname, "refs/tags/", 10) == 0) { > @@ -846,6 +919,7 @@ done: > free(have); > free(want); > free(id_str); > + free(free_default_branch); > free(default_id_str); > free(refname); > free(server_capabilities);