improve got fetch error reporting

On 23-02-06 09:54PM, Stefan Sperling wrote: > On Mon, Feb 06, 2023 at 03:09:08PM +1100, Mark Jamsek wrote: > > Yes, I like this much better! I was a little concerned by erroring out > > if fetching from a worktree with a checked-out branch that doesn't exist > > on the server. I prefer this behaviour. Thank you for unwrapping this :) > > Yet more unwrapping. This is more complicated than expected. > I've rewritten some of the logic in got-fetch-pack to make it easier > to follow. > > I still need to add test coverage for the edge-case -R fix mentioned > in the last paragraph, but I am running out of steam for now. > > fix interaction of 'got fetch -b', got.conf, and work tree > > Without branches in got.conf for a remote, and without -b/-R options, > the fallback to HEAD would only work when not invoked in a work tree. > > With this fix 'got fetch' should behave as described in the man page. > The -b option now overrides both got.conf and the fallback to the work > tree's branch. > And fallback to HEAD works as expected when invoked in a repository. > > Also, do not strictly require remote repositories to provide a branch > from the refs/heads/ namespace. In such cases users should be able to > use -R to select something to fetch. Nice! I really like the refactoring of fetch_pack(), and I very much appreciate how tricky this was! I feel like I should've studied the fetch docs and code before instigating this change so thank you :) minor nits inline but otherwise ok > diff c9003c52fc7f70dd988402560adcd22709e74800 3d7ad4c9f65020b55ebd28d8bc0e6d499ec13fc0 > commit - c9003c52fc7f70dd988402560adcd22709e74800 > commit + 3d7ad4c9f65020b55ebd28d8bc0e6d499ec13fc0 > blob - a32137a402ce476d4300983fe5e9e17d680c8ac0 > blob + 83d6a791c83d96ed0e79da912f70428804dfcb3a > --- got/got.c > +++ got/got.c > @@ -1725,7 +1725,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, > + list_refs_only, verbosity, fetchfd, repo, NULL, > fetch_progress, &fpa); > if (error) > goto done; > @@ -2296,7 +2296,7 @@ cmd_fetch(int argc, char *argv[]) > struct got_fetch_progress_arg fpa; > 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; > + int *pack_fds = NULL, have_bflag = 0; > > TAILQ_INIT(&refs); > TAILQ_INIT(&symrefs); > @@ -2313,6 +2313,7 @@ cmd_fetch(int argc, char *argv[]) > optarg, NULL); > if (error) > return error; > + have_bflag = 1; > break; > case 'd': > delete_refs = 1; > @@ -2480,12 +2481,6 @@ cmd_fetch(int argc, char *argv[]) > if (error) > goto done; > } > - if (worktree) { > - error = got_pathlist_append(&wanted_branches, > - got_worktree_get_head_ref_name(worktree), NULL); > - if (error) > - goto done; > - } > } > if (TAILQ_EMPTY(&wanted_refs)) { > for (i = 0; i < remote->nfetch_refs; i++) { > @@ -2554,6 +2549,8 @@ 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 != NULL && !have_bflag) ? > + got_worktree_get_head_ref_name(worktree) : NULL, > fetch_progress, &fpa); > if (error) > goto done; > blob - 49f9fd5da6c527fcc00a29827b61df267135112e > blob + 5703d1fbe97e2c2bfcb5c6c2095c9653f5ee5e6e > --- include/got_fetch.h > +++ include/got_fetch.h > @@ -46,4 +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 *, got_fetch_progress_cb, void *); > + int, int, int, struct got_repository *, const char *, > + got_fetch_progress_cb, void *); > blob - a006844341e6b3ee524360e3929fe3eee0536090 > blob + bfa126b8fabcca280af94961030dcae5aae3e6e6 > --- lib/fetch.c > +++ lib/fetch.c > @@ -104,7 +104,7 @@ got_fetch_pack(struct got_object_id **pack_hash, struc > int mirror_references, int fetch_all_branches, > struct got_pathlist_head *wanted_branches, > struct got_pathlist_head *wanted_refs, int list_refs_only, int verbosity, > - int fetchfd, struct got_repository *repo, > + int fetchfd, struct got_repository *repo, const char *worktree_refname, > got_fetch_progress_cb progress_cb, void *progress_arg) > { > size_t i; > @@ -261,7 +261,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, verbosity); > + list_refs_only, worktree_refname, verbosity); > if (err != NULL) > goto done; > nfetchfd = -1; > blob - 076cab5adad635b8e7ea46af68a14510dced4da4 > blob + 09e7cb7ce6e9cf826f667d69280ad2dab4b4d161 > --- lib/got_lib_privsep.h > +++ lib/got_lib_privsep.h > @@ -407,9 +407,11 @@ struct got_imsg_fetch_request { > int fetch_all_branches; > int list_refs_only; > int verbosity; > + size_t worktree_branch_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 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. */ > @@ -698,7 +700,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, int); > + struct got_pathlist_head *, int, const char *, 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 - ddb1e595d753eddc725ce0bbb056d1dc8236fc07 > blob + d99805a6e345d6d142e6a571b2f787068b19ef76 > --- lib/privsep.c > +++ lib/privsep.c > @@ -531,7 +531,8 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f > got_privsep_send_fetch_req(struct imsgbuf *ibuf, int fd, > 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, int verbosity) > + struct got_pathlist_head *wanted_refs, int list_refs_only, > + const char *worktree_branch, int verbosity) > { > const struct got_error *err = NULL; > struct ibuf *wbuf; > @@ -539,27 +540,42 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f > struct got_pathlist_entry *pe; > struct got_imsg_fetch_request fetchreq; > > + if (worktree_branch) > + len = sizeof(fetchreq) + strlen(worktree_branch); I would keep strlen(worktree_branch) for reuse; if you agree, the diff below applies on top of yours to do this. > + else > + len = sizeof(fetchreq); > + > + if (len >= MAX_IMSGSIZE - IMSG_HEADER_SIZE) { > + close(fd); > + return got_error(GOT_ERR_NO_SPACE); > + } > + > + wbuf = imsg_create(ibuf, GOT_IMSG_FETCH_REQUEST, 0, 0, len); > + if (wbuf == NULL) > + return got_error_from_errno("imsg_create FETCH_HAVE_REF"); > + > memset(&fetchreq, 0, sizeof(fetchreq)); > fetchreq.fetch_all_branches = fetch_all_branches; > fetchreq.list_refs_only = list_refs_only; > fetchreq.verbosity = verbosity; > + if (worktree_branch != NULL) > + fetchreq.worktree_branch_len = strlen(worktree_branch); > TAILQ_FOREACH(pe, have_refs, entry) > fetchreq.n_have_refs++; > TAILQ_FOREACH(pe, wanted_branches, entry) > fetchreq.n_wanted_branches++; > TAILQ_FOREACH(pe, wanted_refs, entry) > fetchreq.n_wanted_refs++; > - len = sizeof(struct got_imsg_fetch_request); > - if (len >= MAX_IMSGSIZE - IMSG_HEADER_SIZE) { > - close(fd); > - return got_error(GOT_ERR_NO_SPACE); > + if (imsg_add(wbuf, &fetchreq, sizeof(fetchreq)) == -1) > + return got_error_from_errno("imsg_add FETCH_REQUEST"); > + if (worktree_branch) { > + if (imsg_add(wbuf, worktree_branch, > + strlen(worktree_branch))== -1) style: missed whitespace between ')' and '=' (if using below diff, it's fixed in there): strlen(worktree_branch)) == -1) > + return got_error_from_errno("imsg_add FETCH_REQUEST"); > } > + wbuf->fd = fd; > + imsg_close(ibuf, wbuf); > > - if (imsg_compose(ibuf, GOT_IMSG_FETCH_REQUEST, 0, 0, fd, > - &fetchreq, sizeof(fetchreq)) == -1) > - return got_error_from_errno( > - "imsg_compose FETCH_SERVER_PROGRESS"); > - > err = flush_imsg(ibuf); > if (err) { > close(fd); > blob - 77ba914fb971f5e412b57bd1633bda6ee0e1f26d > blob + 32778499296b895c8f8a0bd7f201ca9a637c6f87 > --- libexec/got-fetch-pack/got-fetch-pack.c > +++ libexec/got-fetch-pack/got-fetch-pack.c > @@ -71,7 +71,7 @@ match_remote_ref(struct got_pathlist_head *have_refs, > > static void > match_remote_ref(struct got_pathlist_head *have_refs, > - struct got_object_id *my_id, char *refname) > + struct got_object_id *my_id, const char *refname) > { > struct got_pathlist_entry *pe; > > @@ -292,11 +292,48 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1, > } > > static const struct got_error * > +fetch_ref(struct imsgbuf *ibuf, struct got_pathlist_head *have_refs, > + struct got_object_id *have, struct got_object_id *want, > + const char *refname, const char *id_str) > +{ > + const struct got_error *err; > + char *theirs = NULL, *mine = NULL; > + if (!got_parse_sha1_digest(want->sha1, id_str)) { > + err = got_error(GOT_ERR_BAD_OBJ_ID_STR); > + goto done; > + } > + > + match_remote_ref(have_refs, have, refname); > + err = send_fetch_ref(ibuf, want, refname); > + if (err) > + goto done; > + > + if (chattygot) > + fprintf(stderr, "%s: %s will be fetched\n", > + getprogname(), refname); > + if (chattygot > 1) { > + err = got_object_id_str(&theirs, want); > + if (err) > + goto done; > + err = got_object_id_str(&mine, have); > + if (err) > + goto done; > + fprintf(stderr, "%s: remote: %s\n%s: local: %s\n", > + getprogname(), theirs, getprogname(), mine); > + } > +done: > + free(theirs); > + free(mine); > + return err; > +} > + > +static const struct got_error * > 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, > - struct imsgbuf *ibuf) > + const char *worktree_branch, struct imsgbuf *ibuf) > { > const struct got_error *err = NULL; > char buf[GOT_PKT_MAX]; > @@ -305,7 +342,7 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1, > int is_firstpkt = 1, nref = 0, refsz = 16; > int i, n, nwant = 0, nhave = 0, acked = 0; > off_t packsz = 0, last_reported_packsz = 0; > - char *id_str = NULL, *refname = NULL; > + char *id_str = NULL, *default_id_str = NULL, *refname = NULL; > char *server_capabilities = NULL, *my_capabilities = NULL; > const char *default_branch = NULL; > struct got_pathlist_head symrefs; > @@ -346,6 +383,21 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1, > &server_capabilities, buf, n); > if (err) > goto done; > + > + if (refsz == nref + 1) { > + refsz *= 2; > + have = reallocarray(have, refsz, sizeof(have[0])); > + if (have == NULL) { > + err = got_error_from_errno("reallocarray"); > + goto done; > + } > + want = reallocarray(want, refsz, sizeof(want[0])); > + if (want == NULL) { > + err = got_error_from_errno("reallocarray"); > + goto done; > + } > + } We could leak have/want on realloc failure, so could use an intermediate pointer (also included in below diff): struct got_object_id *h, *w; refsz *= 2; h = reallocarray(have, refsz, sizeof(have[0])); if (h == NULL) { err = got_error_from_errno("reallocarray"); goto done; } have = h; w = reallocarray(want, refsz, sizeof(want[0])); if (w == NULL) { err = got_error_from_errno("reallocarray"); goto done; } want = w; > + > if (is_firstpkt) { > if (chattygot && server_capabilities[0] != '\0') > fprintf(stderr, "%s: server capabilities: %s\n", > @@ -383,107 +435,80 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1, > } > continue; > } > - > - if (strncmp(refname, "refs/heads/", 11) == 0) { > - if (fetch_all_branches || list_refs_only) { > - found_branch = 1; > - } else if (!TAILQ_EMPTY(wanted_branches)) { > - TAILQ_FOREACH(pe, wanted_branches, entry) { > - if (match_branch(refname, pe->path)) > - break; > - } > - if (pe == NULL) { > - if (chattygot) { > - fprintf(stderr, > - "%s: ignoring %s\n", > - getprogname(), refname); > - } > - continue; > - } > - found_branch = 1; > - } else if (default_branch != NULL) { > - if (!match_branch(refname, default_branch)) { > - if (chattygot) { > - fprintf(stderr, > - "%s: ignoring %s\n", > - getprogname(), refname); > - } > - continue; > - } > - found_branch = 1; > - } > - } else if (strncmp(refname, "refs/tags/", 10) != 0) { > - if (!TAILQ_EMPTY(wanted_refs)) { > - TAILQ_FOREACH(pe, wanted_refs, entry) { > - if (match_wanted_ref(refname, pe->path)) > - break; > - } > - if (pe == NULL) { > - if (chattygot) { > - fprintf(stderr, > - "%s: ignoring %s\n", > - getprogname(), refname); > - } > - continue; > - } > - found_branch = 1; > - } else if (!list_refs_only) { > - if (chattygot) { > - fprintf(stderr, "%s: ignoring %s\n", > - getprogname(), refname); > - } > - continue; > - } > - } > - > - if (refsz == nref + 1) { > - refsz *= 2; > - have = reallocarray(have, refsz, sizeof(have[0])); > - if (have == NULL) { > - err = got_error_from_errno("reallocarray"); > + 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; > } > - want = reallocarray(want, refsz, sizeof(want[0])); > - if (want == NULL) { > - err = got_error_from_errno("reallocarray"); > - goto done; > - } > } > - if (!got_parse_sha1_digest(want[nref].sha1, id_str)) { > - err = got_error(GOT_ERR_BAD_OBJ_ID_STR); > - goto done; > - } > - match_remote_ref(have_refs, &have[nref], refname); > - err = send_fetch_ref(ibuf, &want[nref], refname); > - if (err) > - goto done; > > - if (chattygot) > - fprintf(stderr, "%s: %s will be fetched\n", > - getprogname(), refname); > - if (chattygot > 1) { > - char *theirs, *mine; > - err = got_object_id_str(&theirs, &want[nref]); > + if (list_refs_only || strncmp(refname, "refs/tags/", 10) == 0) { > + err = fetch_ref(ibuf, have_refs, &have[nref], > + &want[nref], refname, id_str); > if (err) > goto done; > - err = got_object_id_str(&mine, &have[nref]); > - if (err) { > - free(theirs); > - goto done; > + nref++; > + } else if (strncmp(refname, "refs/heads/", 11) == 0) { > + if (fetch_all_branches) { > + err = fetch_ref(ibuf, have_refs, &have[nref], > + &want[nref], refname, id_str); > + if (err) > + goto done; > + nref++; > + found_branch = 1; > + continue; > } > - fprintf(stderr, "%s: remote: %s\n%s: local: %s\n", > - getprogname(), theirs, getprogname(), mine); > - free(theirs); > - free(mine); > + TAILQ_FOREACH(pe, wanted_branches, entry) { > + if (match_branch(refname, pe->path)) > + break; > + } > + if (pe != NULL || (worktree_branch != NULL && > + match_branch(refname, worktree_branch))) { > + err = fetch_ref(ibuf, have_refs, &have[nref], > + &want[nref], refname, id_str); > + if (err) > + goto done; > + nref++; > + found_branch = 1; > + } else if (chattygot) { > + fprintf(stderr, "%s: ignoring %s\n", > + getprogname(), refname); > + } > + } else { > + TAILQ_FOREACH(pe, wanted_refs, entry) { > + if (match_wanted_ref(refname, pe->path)) > + break; > + } > + if (pe != NULL) { > + err = fetch_ref(ibuf, have_refs, &have[nref], > + &want[nref], refname, id_str); > + if (err) > + goto done; > + nref++; > + } else if (chattygot) { > + fprintf(stderr, "%s: ignoring %s\n", > + getprogname(), refname); > + } > } > - nref++; > } > > if (list_refs_only) > goto done; > > - /* Abort if we haven't found any branch to fetch. */ > - if (!found_branch) { > + if (!found_branch && 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; > + } > + > + /* Abort if we haven't found anything to fetch. */ > + if (nref == 0) { > err = got_error(GOT_ERR_FETCH_NO_BRANCH); > goto done; > } > @@ -763,6 +788,7 @@ done: > free(have); > free(want); > free(id_str); > + free(default_id_str); > free(refname); > free(server_capabilities); > return err; > @@ -785,6 +811,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; > #if 0 > static int attached; > while (!attached) > @@ -823,6 +850,22 @@ main(int argc, char **argv) > } > memcpy(&fetch_req, imsg.data, sizeof(fetch_req)); > fetchfd = imsg.fd; > + > + if (datalen != sizeof(fetch_req) + > + fetch_req.worktree_branch_len) { > + err = got_error(GOT_ERR_PRIVSEP_LEN); > + goto done; > + } > + > + if (fetch_req.worktree_branch_len != 0) { > + worktree_branch = strndup(imsg.data + > + sizeof(fetch_req), fetch_req.worktree_branch_len); > + if (worktree_branch == NULL) { > + err = got_error_from_errno("strndup"); > + goto done; > + } > + } > + > imsg_free(&imsg); > > if (fetch_req.verbosity > 0) > @@ -978,8 +1021,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, &ibuf); > + &wanted_refs, fetch_req.list_refs_only, > + worktree_branch, &ibuf); > done: > + free(worktree_branch); > 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) > blob - 0f381607476a2ed62e00ff9cb319557a26e8d633 > blob + 26346effc18d13d5e986f7072e63b5d9c885450f > --- regress/cmdline/fetch.sh > +++ regress/cmdline/fetch.sh > @@ -190,6 +190,44 @@ test_fetch_branch() { > > # foo is now the default HEAD branch in $testroot/repo > # but got.conf still says to fetch "master" > + got fetch -q -r $testroot/repo-clone > $testroot/stdout > + ret=$? > + if [ $ret -ne 0 ]; then > + echo "got fetch command failed unexpectedly" >&2 > + test_done "$testroot" "$ret" > + return 1 > + fi > + > + echo -n > $testroot/stdout.expected > + > + cmp -s $testroot/stdout $testroot/stdout.expected > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stdout.expected $testroot/stdout > + test_done "$testroot" "$ret" > + return 1 > + fi > + > + got ref -l -r $testroot/repo-clone > $testroot/stdout > + > + echo "HEAD: refs/heads/master" > $testroot/stdout.expected > + echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected > + echo "refs/remotes/origin/HEAD: refs/remotes/origin/master" \ > + >> $testroot/stdout.expected > + echo "refs/remotes/origin/master: $commit_id2" \ > + >> $testroot/stdout.expected > + # refs/hoo/boo/zoo is missing because it is outside of refs/heads > + echo "refs/tags/1.0: $tag_id" >> $testroot/stdout.expected > + > + cmp -s $testroot/stdout $testroot/stdout.expected > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stdout.expected $testroot/stdout > + test_done "$testroot" "$ret" > + return 1 > + fi > + > + # fetch branch foo via command-line switch > got fetch -q -r $testroot/repo-clone -b foo > $testroot/stdout > ret=$? > if [ $ret -ne 0 ]; then > @@ -216,8 +254,7 @@ test_fetch_branch() { > echo "refs/remotes/origin/HEAD: refs/remotes/origin/foo" \ > >> $testroot/stdout.expected > echo "refs/remotes/origin/foo: $commit_id3" >> $testroot/stdout.expected > - # refs/remotes/origin/master is umodified because it wasn't fetched > - echo "refs/remotes/origin/master: $commit_id" \ > + echo "refs/remotes/origin/master: $commit_id2" \ > >> $testroot/stdout.expected > # refs/hoo/boo/zoo is missing because it is outside of refs/heads > echo "refs/tags/1.0: $tag_id" >> $testroot/stdout.expected > @@ -315,7 +352,57 @@ test_fetch_branch() { > ret=$? > if [ $ret -ne 0 ]; then > diff -u $testroot/stdout.expected $testroot/stdout > + test_done "$testroot" "$ret" > + return 1 > fi > + > + # remove default branch information from got.conf > + sed -i -e "/branch {/d" $testroot/repo-clone/got.conf > + > + # make another change on 'foo' and fetch it without got.conf > + (cd $testroot/repo && git checkout -q foo) > + echo "modified beta on foo agan" > $testroot/repo/beta > + git_commit $testroot/repo -m "modified beta" > + local commit_id5=`git_show_head $testroot/repo` > + (cd $testroot/repo && git checkout -q master) > + > + # fetch new commits on branch 'foo', implicitly obtaining the > + # branch name from a work tree > + (cd $testroot/wt && got fetch -q > $testroot/stdout) > + > + echo -n > $testroot/stdout.expected > + > + cmp -s $testroot/stdout $testroot/stdout.expected > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stdout.expected $testroot/stdout > + test_done "$testroot" "$ret" > + return 1 > + fi > + > + wt_uuid=`(cd $testroot/wt && got info | grep 'UUID:' | \ > + cut -d ':' -f 2 | tr -d ' ')` > + > + got ref -l -r $testroot/repo-clone > $testroot/stdout > + > + echo "HEAD: refs/heads/master" > $testroot/stdout.expected > + echo "refs/got/worktree/base-$wt_uuid: $commit_id3" \ > + >> $testroot/stdout.expected > + echo "refs/heads/foo: $commit_id3" >> $testroot/stdout.expected > + echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected > + echo "refs/remotes/origin/HEAD: refs/remotes/origin/master" \ > + >> $testroot/stdout.expected > + echo "refs/remotes/origin/foo: $commit_id5" >> $testroot/stdout.expected > + echo "refs/remotes/origin/master: $commit_id2" \ > + >> $testroot/stdout.expected > + # refs/hoo/boo/zoo is missing because it is outside of refs/heads > + echo "refs/tags/1.0: $tag_id" >> $testroot/stdout.expected > + > + cmp -s $testroot/stdout $testroot/stdout.expected > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stdout.expected $testroot/stdout > + fi > test_done "$testroot" "$ret" > } > > Applies on top of your diff: diff /home/mark/src/got commit - 7dd1b4dd193d3baffa22badc970ee8f8fa13e236 path + /home/mark/src/got blob - d99805a6e345d6d142e6a571b2f787068b19ef76 file + lib/privsep.c --- lib/privsep.c +++ lib/privsep.c @@ -536,13 +536,14 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f { const struct got_error *err = NULL; struct ibuf *wbuf; - size_t len; + size_t len, worktree_branch_len; struct got_pathlist_entry *pe; struct got_imsg_fetch_request fetchreq; - if (worktree_branch) - len = sizeof(fetchreq) + strlen(worktree_branch); - else + if (worktree_branch) { + worktree_branch_len = strlen(worktree_branch); + len = sizeof(fetchreq) + worktree_branch_len; + } else len = sizeof(fetchreq); if (len >= MAX_IMSGSIZE - IMSG_HEADER_SIZE) { @@ -559,7 +560,7 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f fetchreq.list_refs_only = list_refs_only; fetchreq.verbosity = verbosity; if (worktree_branch != NULL) - fetchreq.worktree_branch_len = strlen(worktree_branch); + fetchreq.worktree_branch_len = worktree_branch_len; TAILQ_FOREACH(pe, have_refs, entry) fetchreq.n_have_refs++; TAILQ_FOREACH(pe, wanted_branches, entry) @@ -569,8 +570,7 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f if (imsg_add(wbuf, &fetchreq, sizeof(fetchreq)) == -1) return got_error_from_errno("imsg_add FETCH_REQUEST"); if (worktree_branch) { - if (imsg_add(wbuf, worktree_branch, - strlen(worktree_branch))== -1) + if (imsg_add(wbuf, worktree_branch, worktree_branch_len) == -1) return got_error_from_errno("imsg_add FETCH_REQUEST"); } wbuf->fd = fd; blob - 32778499296b895c8f8a0bd7f201ca9a637c6f87 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 @@ -385,17 +385,21 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1, goto done; if (refsz == nref + 1) { + struct got_object_id *h, *w; + refsz *= 2; - have = reallocarray(have, refsz, sizeof(have[0])); - if (have == NULL) { + h = reallocarray(have, refsz, sizeof(have[0])); + if (h == NULL) { err = got_error_from_errno("reallocarray"); goto done; } - want = reallocarray(want, refsz, sizeof(want[0])); - if (want == NULL) { + have = h; + w = reallocarray(want, refsz, sizeof(want[0])); + if (w == NULL) { err = got_error_from_errno("reallocarray"); goto done; } + want = w; } if (is_firstpkt) { -- Mark Jamsek <fnc.bsdbox.org> GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68

2023-02-06 04:09 Mark Jamsek:
improve got fetch error reporting
- 2023-02-06 20:54 Stefan Sperling:
  improve got fetch error reporting
- - 2023-02-07 12:16 Mark Jamsek:
    improve got fetch error reporting
  - - 2023-02-07 12:56 Stefan Sperling:
      improve got fetch error reporting
    - - 2023-02-07 15:00 Stefan Sperling:
        improve got fetch error reporting