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