improve got fetch error reporting

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. 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); + 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) + 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; + } + } + 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" }

2023-02-05 22:12 Stefan Sperling:
improve got fetch error reporting
- 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