From: Martijn van Duren Subject: Re: got: allow clone without symrefs To: gameoftrees@openbsd.org Cc: Stefan Sperling Date: Sat, 14 Jun 2025 21:01:59 +0200 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. 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);