"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Mark Jamsek <mark@jamsek.com>
Subject:
Re: always fetch remote HEAD except when -b is used
To:
Christian Weisgerber <naddy@mips.inka.de>, gameoftrees@openbsd.org
Date:
Fri, 17 Feb 2023 02:59:44 +1100

Download raw body.

Thread
On 23-02-16 07:28PM, Mark Jamsek wrote:
> On 23-02-14 04:48PM, Stefan Sperling wrote:
> > On Tue, Feb 14, 2023 at 04:17:09PM +0100, Christian Weisgerber wrote:
> > > Mark Jamsek:
> > > 
> > > > As discussed with stsp on irc, this tweaks 'got fetch' again so that the
> > > > branch resolved via the remote's HEAD ref is not just a fallback--it is
> > > > always fetched except in the 'got fetch -b branch' case.
> > > > 
> > > > This necessitated passing the bflag to got_fetch_pack from cmd_clone(),
> > > > too, so that we don't fetch HEAD when 'got clone -b branch' is used.
> > > 
> > > I'm struggling to make sense of the semantics here.
> > > 
> > > $ got clone -b stable/13 ssh://anongit@git.freebsd.org/src.git
> > > ...
> > > $ cd src.git
> > > $ got ref -l | fgrep -v /tags/
> > > HEAD: refs/heads/stable/13
> > > refs/heads/stable/13: acaed6b1e2859cb6726e35e8c937b192c16a95c8
> > > refs/remotes/origin/stable/13: acaed6b1e2859cb6726e35e8c937b192c16a95c8
> > > $ got fetch
> > > ...
> > > $ got ref -l | fgrep -v /tags/
> > > HEAD: refs/heads/stable/13
> > > refs/heads/main: 825fbd087e6150eaf601612a5e7468ddc808e004
> > > refs/heads/stable/13: acaed6b1e2859cb6726e35e8c937b192c16a95c8
> > > refs/remotes/origin/HEAD: refs/remotes/origin/main
> > > refs/remotes/origin/main: 825fbd087e6150eaf601612a5e7468ddc808e004
> > > refs/remotes/origin/stable/13: acaed6b1e2859cb6726e35e8c937b192c16a95c8
> > > 
> > > Presumably, if I limit "clone" to a particular branch, that's all
> > > I'm interested in, and I don't expect the next "fetch" to add the
> > > remote HEAD branch.
> > 
> > Regarding got fetch behaviour, I see several ways forward:
> 
> The below diff implements stsp's suggestion:
>
> > We could fetch HEAD by default only if the symref refs/remotes/foo/HEAD
> > already exits, and if its target no longer matches the remote HEAD.
> > This would preserve the behaviour you want for -stable repositories
> > while also satisfying the above concern.

Woops! Just realised I didn't send the latest revision which fixed
a logic bug in the HEAD branch name matching in fetch_pack() and some
trivial comment changes.

Actual diff implementing stsp's fetch suggestion:

diffstat /home/mark/src/got
 M  got/got.1                                |   4+   0-
 M  got/got.c                                |  53+   9-
 M  include/got_fetch.h                      |   2+   2-
 M  lib/fetch.c                              |   3+   2-
 M  lib/got_lib_privsep.h                    |   3+   1-
 M  lib/privsep.c                            |  15+   5-
 M  libexec/got-fetch-pack/got-fetch-pack.c  |  35+  11-

7 files changed, 115 insertions(+), 30 deletions(-)

diff /home/mark/src/got
commit - ac51614e087279834159667c6f71fffe546cffa6
path + /home/mark/src/got
blob - d9209e2ae1dd1c1aa01e368bc0016cf86830a9e7
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -358,6 +358,10 @@ This default behaviour can be overridden with the
 .Xr got.conf 5
 or via a work tree, or said branches are not found on the server, a branch
 resolved via the remote repository's HEAD reference will be fetched.
+Likewise, if a HEAD reference for the
+.Ar remote-repository
+exists but its target no longer matches the remote HEAD, then
+the new target branch will be fetched.
 This default behaviour can be overridden with the
 .Fl a
 and
blob - 1ef76ea95b69bf7e45b91f9053a232d241269e7d
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -1544,7 +1544,7 @@ cmd_clone(int argc, char *argv[])
 	struct got_fetch_progress_arg fpa;
 	char *git_url = NULL;
 	int verbosity = 0, fetch_all_branches = 0, mirror_references = 0;
-	int list_refs_only = 0;
+	int bflag = 0, list_refs_only = 0;
 	int *pack_fds = NULL;
 
 	TAILQ_INIT(&refs);
@@ -1562,6 +1562,7 @@ cmd_clone(int argc, char *argv[])
 			    optarg, NULL);
 			if (error)
 				return error;
+			bflag = 1;
 			break;
 		case 'l':
 			list_refs_only = 1;
@@ -1717,7 +1718,7 @@ cmd_clone(int argc, char *argv[])
 	error = got_fetch_pack(&pack_hash, &refs, &symrefs,
 	    GOT_FETCH_DEFAULT_REMOTE_NAME, mirror_references,
 	    fetch_all_branches, &wanted_branches, &wanted_refs,
-	    list_refs_only, verbosity, fetchfd, repo, NULL, 0,
+	    list_refs_only, verbosity, fetchfd, repo, NULL, NULL, bflag,
 	    fetch_progress, &fpa);
 	if (error)
 		goto done;
@@ -2282,6 +2283,8 @@ cmd_fetch(int argc, char *argv[])
 	const struct got_gotconfig *repo_conf = NULL, *worktree_conf = NULL;
 	struct got_pathlist_head refs, symrefs, wanted_branches, wanted_refs;
 	struct got_pathlist_entry *pe;
+	struct got_reflist_head remote_refs;
+	struct got_reflist_entry *re;
 	struct got_object_id *pack_hash = NULL;
 	int i, ch, fetchfd = -1, fetchstatus;
 	pid_t fetchpid = -1;
@@ -2289,10 +2292,11 @@ cmd_fetch(int argc, char *argv[])
 	int verbosity = 0, fetch_all_branches = 0, list_refs_only = 0;
 	int delete_refs = 0, replace_tags = 0, delete_remote = 0;
 	int *pack_fds = NULL, have_bflag = 0;
-	const char *worktree_branch = NULL;
+	const char *remote_head = NULL, *worktree_branch = NULL;
 
 	TAILQ_INIT(&refs);
 	TAILQ_INIT(&symrefs);
+	TAILQ_INIT(&remote_refs);
 	TAILQ_INIT(&wanted_branches);
 	TAILQ_INIT(&wanted_refs);
 
@@ -2531,12 +2535,51 @@ cmd_fetch(int argc, char *argv[])
 	if (error)
 		goto done;
 
-	if (worktree && !have_bflag) {
-		const char *refname;
+	if (!have_bflag) {
+		/*
+		 * If set, get this remote's HEAD ref target so
+		 * if it has changed on the server we can fetch it.
+		 */
+		error = got_ref_list(&remote_refs, repo, "refs/remotes",
+		    got_ref_cmp_by_name, repo);
+		if (error)
+			goto done;
 
-		refname = got_worktree_get_head_ref_name(worktree);
-		if (strncmp(refname, "refs/heads/", 11) == 0)
-			worktree_branch = refname;
+		TAILQ_FOREACH(re, &remote_refs, entry) {
+			const char	*remote_refname, *remote_target;
+			size_t		 remote_name_len;
+
+			if (!got_ref_is_symbolic(re->ref))
+				continue;
+
+			remote_name_len = strlen(remote->name);
+			remote_refname = got_ref_get_name(re->ref);
+
+			/* we only want refs/remotes/$remote->name/HEAD */
+			if (strncmp(remote_refname + 13, remote->name,
+			    remote_name_len) != 0)
+				continue;
+
+			if (strcmp(remote_refname + remote_name_len + 14,
+			    GOT_REF_HEAD) != 0)
+				continue;
+
+			/*
+			 * Take the name itself because we already
+			 * only match with refs/heads/ in fetch_pack().
+			 */
+			remote_target = got_ref_get_symref_target(re->ref);
+			remote_head = remote_target + remote_name_len + 14;
+			break;
+		}
+
+		if (worktree) {
+			const char *refname;
+
+			refname = got_worktree_get_head_ref_name(worktree);
+			if (strncmp(refname, "refs/heads/", 11) == 0)
+				worktree_branch = refname;
+		}
 	}
 
 	fpa.last_scaled_size[0] = '\0';
@@ -2551,7 +2594,7 @@ cmd_fetch(int argc, char *argv[])
 	error = got_fetch_pack(&pack_hash, &refs, &symrefs, remote->name,
 	    remote->mirror_references, fetch_all_branches, &wanted_branches,
 	    &wanted_refs, list_refs_only, verbosity, fetchfd, repo,
-	    worktree_branch, have_bflag, fetch_progress, &fpa);
+	    worktree_branch, remote_head, have_bflag, fetch_progress, &fpa);
 	if (error)
 		goto done;
 
@@ -2730,6 +2773,7 @@ done:
 	got_pathlist_free(&symrefs, GOT_PATHLIST_FREE_ALL);
 	got_pathlist_free(&wanted_branches, GOT_PATHLIST_FREE_NONE);
 	got_pathlist_free(&wanted_refs, GOT_PATHLIST_FREE_NONE);
+	got_ref_list_free(&remote_refs);
 	free(id_str);
 	free(cwd);
 	free(repo_path);
blob - 5768c9157c81ca12b00f65491c540dd712c37e6a
file + include/got_fetch.h
--- include/got_fetch.h
+++ include/got_fetch.h
@@ -46,5 +46,5 @@ const struct got_error *got_fetch_pack(struct got_obje
 const struct got_error *got_fetch_pack(struct got_object_id **,
 	struct got_pathlist_head *, struct got_pathlist_head *, const char *,
 	int, int, struct got_pathlist_head *, struct got_pathlist_head *,
-	int, int, int, struct got_repository *, const char *, int,
-	got_fetch_progress_cb, void *);
+	int, int, int, struct got_repository *, const char *, const char *,
+	int, got_fetch_progress_cb, void *);
blob - e37808ed2a0bf37771cbf76e39cfa92b60ad43cc
file + lib/fetch.c
--- lib/fetch.c
+++ lib/fetch.c
@@ -105,7 +105,8 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
     struct got_pathlist_head *wanted_branches,
     struct got_pathlist_head *wanted_refs, int list_refs_only, int verbosity,
     int fetchfd, struct got_repository *repo, const char *worktree_refname,
-    int no_head, got_fetch_progress_cb progress_cb, void *progress_arg)
+    const char *remote_head, int no_head, got_fetch_progress_cb progress_cb,
+    void *progress_arg)
 {
 	size_t i;
 	int imsg_fetchfds[2], imsg_idxfds[2];
@@ -261,7 +262,7 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
 	}
 	err = got_privsep_send_fetch_req(&fetchibuf, nfetchfd, &have_refs,
 	    fetch_all_branches, wanted_branches, wanted_refs,
-	    list_refs_only, worktree_refname, no_head, verbosity);
+	    list_refs_only, worktree_refname, remote_head, no_head, verbosity);
 	if (err != NULL)
 		goto done;
 	nfetchfd = -1;
blob - b8170cb9afee37b2625c705535e43662ca7dd06c
file + lib/got_lib_privsep.h
--- lib/got_lib_privsep.h
+++ lib/got_lib_privsep.h
@@ -409,10 +409,12 @@ struct got_imsg_fetch_request {
 	int list_refs_only;
 	int verbosity;
 	size_t worktree_branch_len;
+	size_t remote_head_len;
 	size_t n_have_refs;
 	size_t n_wanted_branches;
 	size_t n_wanted_refs;
 	/* Followed by worktree_branch_len bytes of reference name. */
+	/* Followed by remote_head_len bytes of reference name. */
 	/* Followed by n_have_refs GOT_IMSG_FETCH_HAVE_REF messages. */
 	/* Followed by n_wanted_branches times GOT_IMSG_FETCH_WANTED_BRANCH. */
 	/* Followed by n_wanted_refs times GOT_IMSG_FETCH_WANTED_REF. */
@@ -701,7 +703,7 @@ const struct got_error *got_privsep_send_fetch_req(str
     int *, int *, struct imsgbuf *ibuf);
 const struct got_error *got_privsep_send_fetch_req(struct imsgbuf *, int,
     struct got_pathlist_head *, int, struct got_pathlist_head *,
-    struct got_pathlist_head *, int, const char *, int, int);
+    struct got_pathlist_head *, int, const char *, const char *, int, int);
 const struct got_error *got_privsep_send_fetch_outfd(struct imsgbuf *, int);
 const struct got_error *got_privsep_recv_fetch_progress(int *,
     struct got_object_id **, char **, struct got_pathlist_head *, char **,
blob - ed227bda3d7a4ce423d882c19ae47793a302c675
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -532,19 +532,23 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f
     struct got_pathlist_head *have_refs, int fetch_all_branches,
     struct got_pathlist_head *wanted_branches,
     struct got_pathlist_head *wanted_refs, int list_refs_only,
-    const char *worktree_branch, int no_head, int verbosity)
+    const char *worktree_branch, const char *remote_head,
+    int no_head, int verbosity)
 {
 	const struct got_error *err = NULL;
 	struct ibuf *wbuf;
-	size_t len, worktree_branch_len;
 	struct got_pathlist_entry *pe;
 	struct got_imsg_fetch_request fetchreq;
+	size_t remote_head_len, worktree_branch_len, len = sizeof(fetchreq);
 
 	if (worktree_branch) {
 		worktree_branch_len = strlen(worktree_branch);
-		len = sizeof(fetchreq) + worktree_branch_len;
-	} else
-		len = sizeof(fetchreq);
+		len += worktree_branch_len;
+	}
+	if (remote_head) {
+		remote_head_len = strlen(remote_head);
+		len += remote_head_len;
+	}
 
 	if (len >= MAX_IMSGSIZE - IMSG_HEADER_SIZE) {
 		close(fd);
@@ -562,6 +566,8 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f
 	fetchreq.verbosity = verbosity;
 	if (worktree_branch != NULL)
 		fetchreq.worktree_branch_len = worktree_branch_len;
+	if (remote_head != NULL)
+		fetchreq.remote_head_len = remote_head_len;
 	TAILQ_FOREACH(pe, have_refs, entry)
 		fetchreq.n_have_refs++;
 	TAILQ_FOREACH(pe, wanted_branches, entry)
@@ -574,6 +580,10 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f
 		if (imsg_add(wbuf, worktree_branch, worktree_branch_len) == -1)
 			return got_error_from_errno("imsg_add FETCH_REQUEST");
 	}
+	if (remote_head) {
+		if (imsg_add(wbuf, remote_head, remote_head_len) == -1)
+			return got_error_from_errno("imsg_add FETCH_REQUEST");
+	}
 	wbuf->fd = fd;
 	imsg_close(ibuf, wbuf);
 
blob - 2a22df95ff37a15b31f41c7c01f263af824ddc15
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
@@ -333,7 +333,8 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
     struct got_pathlist_head *have_refs, int fetch_all_branches,
     struct got_pathlist_head *wanted_branches,
     struct got_pathlist_head *wanted_refs, int list_refs_only,
-    const char *worktree_branch, int no_head, struct imsgbuf *ibuf)
+    const char *worktree_branch, const char *remote_head,
+    int no_head, struct imsgbuf *ibuf)
 {
 	const struct got_error *err = NULL;
 	char buf[GOT_PKT_MAX];
@@ -501,14 +502,27 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 	if (list_refs_only)
 		goto done;
 
-	if (!found_branch && !no_head && default_branch && default_id_str &&
+	/*
+	 * If -b was not used and either none of the requested branches
+	 * (got.conf, worktree) were found or the client already has the
+	 * remote HEAD symref but its target changed, fetch remote's HEAD.
+	 */
+	if (!no_head && default_branch && default_id_str &&
 	    strncmp(default_branch, "refs/heads/", 11) == 0) {
-		err = fetch_ref(ibuf, have_refs, &have[nref],
-		    &want[nref], default_branch, default_id_str);
-		if (err)
-			goto done;
-		nref++;
-		found_branch = 1;
+		int remote_head_changed = 0;
+
+		if (remote_head) {
+			if (strcmp(remote_head, default_branch + 11) != 0)
+				remote_head_changed = 1;
+		}
+
+		if (!found_branch || remote_head_changed) {
+			err = fetch_ref(ibuf, have_refs, &have[nref],
+			    &want[nref], default_branch, default_id_str);
+			if (err)
+				goto done;
+			nref++;
+		}
 	}
 
 	/* Abort if we haven't found anything to fetch. */
@@ -834,7 +848,7 @@ main(int argc, char **argv)
 	struct got_imsg_fetch_wanted_branch wbranch;
 	struct got_imsg_fetch_wanted_ref wref;
 	size_t datalen, i;
-	char *worktree_branch = NULL;
+	char *remote_head = NULL, *worktree_branch = NULL;
 #if 0
 	static int attached;
 	while (!attached)
@@ -875,7 +889,7 @@ main(int argc, char **argv)
 	fetchfd = imsg.fd;
 
 	if (datalen != sizeof(fetch_req) +
-	    fetch_req.worktree_branch_len) {
+	    fetch_req.worktree_branch_len + fetch_req.remote_head_len) {
 		err = got_error(GOT_ERR_PRIVSEP_LEN);
 		goto done;
 	}
@@ -889,6 +903,15 @@ main(int argc, char **argv)
 		}
 	}
 
+	if (fetch_req.remote_head_len != 0) {
+		remote_head = strndup(imsg.data + sizeof(fetch_req) +
+		    fetch_req.worktree_branch_len, fetch_req.remote_head_len);
+		if (remote_head == NULL) {
+			err = got_error_from_errno("strndup");
+			goto done;
+		}
+	}
+
 	imsg_free(&imsg);
 
 	if (fetch_req.verbosity > 0)
@@ -1045,9 +1068,10 @@ main(int argc, char **argv)
 	err = fetch_pack(fetchfd, packfd, pack_sha1, &have_refs,
 	    fetch_req.fetch_all_branches, &wanted_branches,
 	    &wanted_refs, fetch_req.list_refs_only,
-	    worktree_branch, fetch_req.no_head, &ibuf);
+	    worktree_branch, remote_head, fetch_req.no_head, &ibuf);
 done:
 	free(worktree_branch);
+	free(remote_head);
 	got_pathlist_free(&have_refs, GOT_PATHLIST_FREE_ALL);
 	got_pathlist_free(&wanted_branches, GOT_PATHLIST_FREE_PATH);
 	if (fetchfd != -1 && close(fetchfd) == -1 && err == NULL)

-- 
Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68