Download raw body.
got: allow clone without symrefs
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);
got: allow clone without symrefs