From: Kyle Ackerman Subject: Re: Remove got_pathlist_append() To: Stefan Sperling Cc: "gameoftrees@openbsd.org" Date: Thu, 21 Nov 2024 10:40:57 +0000 On Thursday, November 21st, 2024 at 2:08 AM, Stefan Sperling wrote: > > > On Thu, Nov 21, 2024 at 01:13:12AM +0000, Kyle Ackerman wrote: > > > @@ -780,11 +781,12 @@ got_privsep_recv_fetch_progress(int *done, struct got_ > > } > > off += s->target_len; > > remain -= s->target_len; > > - err = got_pathlist_append(symrefs, name, target); > > + err = got_pathlist_insert(&new, symrefs, name, target); > > if (err) { > > > Missing new == NULL condition here? Thanks for catching this! > > > free(name); > > free(target); > > - goto done; > > + if (err->code != GOT_ERR_REF_DUP_ENTRY) > > + goto done; > > } > > } > > break; > > @@ -938,6 +939,7 @@ main(int argc, char **argv) > > struct got_object_id *id; > > char *refname; > > > > + > > err = got_privsep_recv_imsg(&imsg, &ibuf, 0); > > if (err) { > > if (err->code == GOT_ERR_PRIVSEP_PIPE) > > > > Needless whitespace change above. > > > --- regress/cmdline/diff.sh > > +++ regress/cmdline/diff.sh > > @@ -1941,8 +1941,8 @@ date: $d > > > > changed epsilon into file > > > > - D epsilon/zeta > > A epsilon > > + D epsilon/zeta > > > The above chunk of the patch did not apply with 'got patch', probably > because leading spaces are present on these lines. Not sure why, and > not a deal-breaker of course. > > > Thoughts/comments/suggestions? > > > I'm ok with this. Would you mind me addressing the above nits > before comitting your diff? > > As I recall, the reason for having the _append function was to allow for > keeping paths sorted in the order given on the command line, such that > the output of a command would reflect the order in which paths were > specified by the user. > However, this was never used effectively, and at least in the case of > 'got diff' it was later decided to sort specified paths to provide > consistent output regardless of how the path arguments are provided. > > Today, I cannot think of a reason why paths shouldn't always be > sorted, and with an RB tree we cannot have it any other way. Here is the updated diff. diff /home/kyle/src/got-fork commit - 87e823f249effd48cf55c950a1a24cbfa39267c0 path + /home/kyle/src/got-fork blob - 7864138db9fadd684f9cc01f99ab36941f60e2fd file + cvg/cvg.c --- cvg/cvg.c +++ cvg/cvg.c @@ -1546,7 +1546,7 @@ cmd_clone(int argc, char *argv[]) fetch_all_branches = 1; break; case 'b': - error = got_pathlist_append(&wanted_branches, + error = got_pathlist_insert(NULL, &wanted_branches, optarg, NULL); if (error) return error; @@ -1562,7 +1562,7 @@ cmd_clone(int argc, char *argv[]) verbosity = -1; break; case 'R': - error = got_pathlist_append(&wanted_refs, + error = got_pathlist_insert(NULL, &wanted_refs, optarg, NULL); if (error) return error; @@ -2376,7 +2376,7 @@ cmd_checkout(int argc, char *argv[]) goto done; } - error = got_pathlist_append(&paths, "", NULL); + error = got_pathlist_insert(NULL, &paths, "", NULL); if (error) goto done; cpa.worktree_path = worktree_path; @@ -2573,7 +2573,7 @@ get_worktree_paths_from_argv(struct got_pathlist_head path = strdup(""); if (path == NULL) return got_error_from_errno("strdup"); - return got_pathlist_append(paths, path, NULL); + return got_pathlist_insert(NULL, paths, path, NULL); } for (i = 0; i < argc; i++) { blob - 8a69602e91a117f6b3b1826abfe7737979fbf616 file + got/got.c --- got/got.c +++ got/got.c @@ -1642,7 +1642,7 @@ cmd_clone(int argc, char *argv[]) fetch_all_branches = 1; break; case 'b': - error = got_pathlist_append(&wanted_branches, + error = got_pathlist_insert(NULL, &wanted_branches, optarg, NULL); if (error) return error; @@ -1658,7 +1658,7 @@ cmd_clone(int argc, char *argv[]) verbosity = -1; break; case 'R': - error = got_pathlist_append(&wanted_refs, + error = got_pathlist_insert(NULL, &wanted_refs, optarg, NULL); if (error) return error; @@ -2397,7 +2397,7 @@ cmd_fetch(int argc, char *argv[]) fetch_all_branches = 1; break; case 'b': - error = got_pathlist_append(&wanted_branches, + error = got_pathlist_insert(NULL, &wanted_branches, optarg, NULL); if (error) return error; @@ -2413,7 +2413,7 @@ cmd_fetch(int argc, char *argv[]) verbosity = -1; break; case 'R': - error = got_pathlist_append(&wanted_refs, + error = got_pathlist_insert(NULL, &wanted_refs, optarg, NULL); if (error) return error; @@ -2573,7 +2573,7 @@ cmd_fetch(int argc, char *argv[]) if (!fetch_all_branches) fetch_all_branches = remote->fetch_all_branches; for (i = 0; i < remote->nfetch_branches; i++) { - error = got_pathlist_append(&wanted_branches, + error = got_pathlist_insert(NULL, &wanted_branches, remote->fetch_branches[i], NULL); if (error) goto done; @@ -2581,7 +2581,7 @@ cmd_fetch(int argc, char *argv[]) } if (TAILQ_EMPTY(&wanted_refs)) { for (i = 0; i < remote->nfetch_refs; i++) { - error = got_pathlist_append(&wanted_refs, + error = got_pathlist_insert(NULL, &wanted_refs, remote->fetch_refs[i], NULL); if (error) goto done; @@ -3297,7 +3297,7 @@ cmd_checkout(int argc, char *argv[]) goto done; } - error = got_pathlist_append(&paths, "", NULL); + error = got_pathlist_insert(NULL, &paths, "", NULL); if (error) goto done; cpa.worktree_path = worktree_path; @@ -3539,7 +3539,7 @@ get_worktree_paths_from_argv(struct got_pathlist_head path = strdup(""); if (path == NULL) return got_error_from_errno("strdup"); - return got_pathlist_append(paths, path, NULL); + return got_pathlist_insert(NULL, paths, path, NULL); } for (i = 0; i < argc; i++) { @@ -9905,13 +9905,13 @@ cmd_send(int argc, char *argv[]) send_all_branches = 1; break; case 'b': - error = got_pathlist_append(&branches, optarg, NULL); + error = got_pathlist_insert(NULL, &branches, optarg, NULL); if (error) return error; nbranches++; break; case 'd': - error = got_pathlist_append(&delete_args, optarg, NULL); + error = got_pathlist_insert(NULL, &delete_args, optarg, NULL); if (error) return error; break; @@ -9932,7 +9932,7 @@ cmd_send(int argc, char *argv[]) send_all_tags = 1; break; case 't': - error = got_pathlist_append(&tags, optarg, NULL); + error = got_pathlist_insert(NULL, &tags, optarg, NULL); if (error) return error; break; @@ -10090,7 +10090,7 @@ cmd_send(int argc, char *argv[]) goto done; TAILQ_FOREACH(re, &all_branches, entry) { const char *branchname = got_ref_get_name(re->ref); - error = got_pathlist_append(&branches, + error = got_pathlist_insert(NULL, &branches, branchname, NULL); if (error) goto done; @@ -10098,7 +10098,7 @@ cmd_send(int argc, char *argv[]) } } else if (nbranches == 0) { for (i = 0; i < remote->nsend_branches; i++) { - error = got_pathlist_append(&branches, + error = got_pathlist_insert(NULL, &branches, remote->send_branches[i], NULL); if (error) goto done; @@ -10112,7 +10112,7 @@ cmd_send(int argc, char *argv[]) goto done; TAILQ_FOREACH(re, &all_tags, entry) { const char *tagname = got_ref_get_name(re->ref); - error = got_pathlist_append(&tags, + error = got_pathlist_insert(NULL, &tags, tagname, NULL); if (error) goto done; @@ -10164,7 +10164,7 @@ cmd_send(int argc, char *argv[]) goto done; } else ref = head_ref; - error = got_pathlist_append(&branches, got_ref_get_name(ref), + error = got_pathlist_insert(NULL, &branches, got_ref_get_name(ref), NULL); if (error) goto done; @@ -11747,7 +11747,7 @@ cmd_rebase(int argc, char *argv[]) if (error) goto done; TAILQ_INIT(&paths); - error = got_pathlist_append(&paths, "", NULL); + error = got_pathlist_insert(NULL, &paths, "", NULL); if (error) goto done; error = got_worktree_checkout_files(worktree, @@ -13210,7 +13210,7 @@ cmd_histedit(int argc, char *argv[]) int have_changes = 0; TAILQ_INIT(&paths); - error = got_pathlist_append(&paths, "", NULL); + error = got_pathlist_insert(NULL, &paths, "", NULL); if (error) goto done; error = got_worktree_status(worktree, &paths, @@ -13769,7 +13769,7 @@ cmd_merge(int argc, char *argv[]) if (error) goto done; TAILQ_INIT(&paths); - error = got_pathlist_append(&paths, "", NULL); + error = got_pathlist_insert(NULL, &paths, "", NULL); if (error) goto done; error = got_worktree_checkout_files(worktree, blob - 6678a9dd097384ee9c7b2545f4a1e9211a83054d file + gotadmin/gotadmin.c --- gotadmin/gotadmin.c +++ gotadmin/gotadmin.c @@ -758,7 +758,7 @@ cmd_pack(int argc, char *argv[]) break; case 'x': got_path_strip_trailing_slashes(optarg); - error = got_pathlist_append(&exclude_args, + error = got_pathlist_insert(NULL, &exclude_args, optarg, NULL); if (error) return error; @@ -1458,7 +1458,7 @@ cmd_dump(int argc, char *argv[]) got_path_strip_trailing_slashes(repo_path); break; case 'x': - error = got_pathlist_append(&exclude_args, + error = got_pathlist_insert(NULL, &exclude_args, optarg, NULL); if (error) return error; @@ -1733,7 +1733,7 @@ cmd_load(int argc, char *argv[]) got_path_strip_trailing_slashes(refname); if (!got_ref_name_is_valid(refname)) errx(1, "invalid reference name %s", refname); - error = got_pathlist_append(&include_args, refname, NULL); + error = got_pathlist_insert(NULL, &include_args, refname, NULL); if (error) goto done; } blob - b2bcaa2b7774cab98d41dad2df3c7777d72b0d4b file + include/got_path.h --- include/got_path.h +++ include/got_path.h @@ -85,15 +85,6 @@ TAILQ_HEAD(got_pathlist_head, got_pathlist_entry); const struct got_error *got_pathlist_insert(struct got_pathlist_entry **, struct got_pathlist_head *, const char *, void *); -/* - * Append a path to the list of paths. - * The caller should already have initialized the list head. This list stores - * the pointer to the path as-is, i.e. the path is not copied internally and - * must remain available until the list is freed with got_pathlist_free(). - */ -const struct got_error *got_pathlist_append(struct got_pathlist_head *, - const char *, void *); - /* Flags passed to got_pathlist_free() to control which pointers are freed. */ #define GOT_PATHLIST_FREE_NONE 0 /* pathlist entry only */ #define GOT_PATHLIST_FREE_PATH (1 << 0) /* entry and path pointer */ blob - 1958bf239d21b4b220fed7b8cb12e942ed4b8c24 file + lib/diff.c --- lib/diff.c +++ lib/diff.c @@ -114,10 +114,11 @@ get_diffstat(struct got_diffstat_cb_arg *ds, const cha ds->del += change->rm; ++ds->nfiles; - err = got_pathlist_append(ds->paths, path, change); - if (err) { + err = got_pathlist_insert(&pe, ds->paths, path, change); + if (err || pe == NULL) { free(change); - return err; + if (err) + return err; } pe = TAILQ_LAST(ds->paths, got_pathlist_head); @@ -890,6 +891,7 @@ got_diff_tree_collect_changed_paths(void *arg, struct { const struct got_error *err = NULL; struct got_pathlist_head *paths = arg; + struct got_pathlist_entry *new; struct got_diff_changed_path *change = NULL; char *path = NULL; @@ -915,9 +917,9 @@ got_diff_tree_collect_changed_paths(void *arg, struct change->status = GOT_STATUS_MODE_CHANGE; } - err = got_pathlist_append(paths, path, change); + err = got_pathlist_insert(&new, paths, path, change); done: - if (err) { + if (err || new == NULL) { free(path); free(change); } blob - 4d56359502a5b5c1e7fc70c62f2355e4e13e7960 file + lib/fetch.c --- lib/fetch.c +++ lib/fetch.c @@ -125,7 +125,7 @@ got_fetch_pack(struct got_object_id **pack_hash, struc char *packpath = NULL, *idxpath = NULL, *id_str = NULL; const char *repo_path = NULL; struct got_pathlist_head have_refs; - struct got_pathlist_entry *pe; + struct got_pathlist_entry *pe, *new; struct got_reflist_head my_refs; struct got_reflist_entry *re; off_t packfile_size = 0; @@ -184,9 +184,14 @@ got_fetch_pack(struct got_object_id **pack_hash, struc err = got_error_from_errno("strdup"); goto done; } - err = got_pathlist_append(&have_refs, refname, id); + err = got_pathlist_insert(&new, &have_refs, refname, id); if (err) goto done; + if (new == NULL){ + free(&refname); + free(id); + } + } if (list_refs_only) { blob - ca1719dd8649b956e27783774efab623fdba7ed8 file + lib/gitproto.c --- lib/gitproto.c +++ lib/gitproto.c @@ -259,6 +259,7 @@ add_symref(struct got_pathlist_head *symrefs, char *ca { const struct got_error *err = NULL; char *colon, *name = NULL, *target = NULL; + struct got_pathlist_entry *new; /* Need at least "A:B" */ if (strlen(capa) < 3) @@ -280,7 +281,9 @@ add_symref(struct got_pathlist_head *symrefs, char *ca } /* We can't validate the ref itself here. The main process will. */ - err = got_pathlist_append(symrefs, name, target); + err = got_pathlist_insert(&new, symrefs, name, target); + if (err == NULL && new == NULL) + err = got_error(GOT_ERR_REF_DUP_ENTRY); done: if (err) { free(name); @@ -308,7 +311,7 @@ got_gitproto_match_capabilities(char **common_capabili if (equalsign != NULL && symrefs != NULL && strncmp(capa, "symref", equalsign - capa) == 0) { err = add_symref(symrefs, equalsign + 1); - if (err) + if (err && err->code != GOT_ERR_REF_DUP_ENTRY) break; continue; } blob - be080572089318fc90fd9461180ce5d13c92521e file + lib/load.c --- lib/load.c +++ lib/load.c @@ -333,6 +333,7 @@ got_repo_load(FILE *in, struct got_pathlist_head *refs for (;;) { struct got_object_id *id; char *dup; + struct got_pathlist_entry *new; linelen = getline(&line, &linesize, in); if (linelen == -1) { @@ -376,11 +377,12 @@ got_repo_load(FILE *in, struct got_pathlist_head *refs goto done; } - err = got_pathlist_append(refs_found, dup, id); - if (err) { + err = got_pathlist_insert(&new, refs_found, dup, id); + if (err || new == NULL) { free(id); free(dup); - goto done; + if (err) + goto done; } } blob - c91ca76066d628e1d12b7c66d86c8829e1b938be file + lib/path.c --- lib/path.c +++ lib/path.c @@ -255,22 +255,6 @@ got_pathlist_insert(struct got_pathlist_entry **insert return NULL; } -const struct got_error * -got_pathlist_append(struct got_pathlist_head *pathlist, - const char *path, void *data) -{ - struct got_pathlist_entry *new; - - new = malloc(sizeof(*new)); - if (new == NULL) - return got_error_from_errno("malloc"); - new->path = path; - new->path_len = strlen(path); - new->data = data; - TAILQ_INSERT_TAIL(pathlist, new, entry); - return NULL; -} - void got_pathlist_free(struct got_pathlist_head *pathlist, int freemask) { blob - 25f3858f09dbd0bc936d97fc851892955ecf6623 file + lib/privsep.c --- lib/privsep.c +++ lib/privsep.c @@ -718,6 +718,7 @@ got_privsep_recv_fetch_progress(int *done, struct got_ size_t datalen; struct got_imsg_fetch_symrefs *isymrefs = NULL; size_t n, remain; + struct got_pathlist_entry *new; off_t off; int i; @@ -780,11 +781,12 @@ got_privsep_recv_fetch_progress(int *done, struct got_ } off += s->target_len; remain -= s->target_len; - err = got_pathlist_append(symrefs, name, target); - if (err) { + err = got_pathlist_insert(&new, symrefs, name, target); + if (err || new == NULL) { free(name); free(target); - goto done; + if (err->code != GOT_ERR_REF_DUP_ENTRY) + goto done; } } break; blob - 99f0df084a62eddfcd9809402c5834ffea4040f3 file + lib/repository.c --- lib/repository.c +++ lib/repository.c @@ -1477,7 +1477,7 @@ got_repo_list_packidx(struct got_pathlist_head *packid break; } - err = got_pathlist_append(packidx_paths, path_packidx, NULL); + err = got_pathlist_insert(NULL, packidx_paths, path_packidx, NULL); if (err) break; } blob - 407601bbacc042b33ad5cb1d87afa5beeb6f6a9d file + lib/worktree.c --- lib/worktree.c +++ lib/worktree.c @@ -4070,7 +4070,7 @@ find_missing_children(void *arg, struct got_fileindex_ } if (got_path_is_child(ie->path, a->parent_path, a->parent_len)) - err = got_pathlist_append(a->children, ie->path, NULL); + err = got_pathlist_insert(NULL, a->children, ie->path, NULL); return err; } @@ -7759,7 +7759,7 @@ get_paths_added_between_commits(struct got_pathlist_he abspath = NULL; } - err = got_pathlist_append(added_paths, wt_path, NULL); + err = got_pathlist_insert(NULL, added_paths, wt_path, NULL); if (err) goto done; wt_path = NULL; blob - da97321077d9a4c57c2b10105cd568a1bc9c944e file + lib/worktree_cvg.c --- lib/worktree_cvg.c +++ lib/worktree_cvg.c @@ -1104,7 +1104,7 @@ find_missing_children(void *arg, struct got_fileindex_ } if (got_path_is_child(ie->path, a->parent_path, a->parent_len)) - err = got_pathlist_append(a->children, ie->path, NULL); + err = got_pathlist_insert(NULL, a->children, ie->path, NULL); return err; } @@ -3067,7 +3067,7 @@ got_worktree_cvg_commit(struct got_object_id **new_com goto done; } - err = got_pathlist_append(&commit_reflist, commit_refname, + err = got_pathlist_insert(&pe, &commit_reflist, commit_refname, head_refname); if (err) goto done; blob - 6ec79e85a76b5ba40ce146e249e75e24f79d7df9 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 @@ -865,6 +865,7 @@ main(int argc, char **argv) struct got_imsg_fetch_wanted_branch wbranch; struct got_imsg_fetch_wanted_ref wref; size_t datalen, i; + struct got_pathlist_entry *new; char *remote_head = NULL, *worktree_branch = NULL; #if 0 static int attached; @@ -973,11 +974,12 @@ main(int argc, char **argv) goto done; } memcpy(id, &href.id, sizeof(*id)); - err = got_pathlist_append(&have_refs, refname, id); - if (err) { + err = got_pathlist_insert(&new, &have_refs, refname, id); + if (err || new == NULL) { free(refname); free(id); - goto done; + if (err) + goto done; } imsg_free(&imsg); @@ -1015,10 +1017,11 @@ main(int argc, char **argv) goto done; } - err = got_pathlist_append(&wanted_branches, refname, NULL); - if (err) { + err = got_pathlist_insert(&new, &wanted_branches, refname, NULL); + if (err || new == NULL) { free(refname); - goto done; + if (err) + goto done; } imsg_free(&imsg); @@ -1055,7 +1058,7 @@ main(int argc, char **argv) goto done; } - err = got_pathlist_append(&wanted_refs, refname, NULL); + err = got_pathlist_insert(NULL, &wanted_refs, refname, NULL); if (err) { free(refname); goto done; blob - 2a2c22971801db07832f237f3969bc2e90a0c745 file + libexec/got-send-pack/got-send-pack.c --- libexec/got-send-pack/got-send-pack.c +++ libexec/got-send-pack/got-send-pack.c @@ -422,10 +422,15 @@ send_pack(int fd, struct got_pathlist_head *refs, if (err) goto done; - err = got_pathlist_append(&their_refs, refname, id); - if (err) - goto done; + err = got_pathlist_insert(&pe, &their_refs, refname, id); + if (err || pe == NULL) { + free(refname); + free(id); + if (err) + goto done; + } + if (chattygot) fprintf(stderr, "%s: remote has %s %s\n", getprogname(), refname, id_str); @@ -658,6 +663,7 @@ main(int argc, char **argv) for (i = 0; i < send_req.nrefs; i++) { struct got_object_id *id; char *refname; + struct got_pathlist_entry *new; if ((err = got_privsep_recv_imsg(&imsg, &ibuf, 0)) != 0) { if (err->code == GOT_ERR_PRIVSEP_PIPE) @@ -707,13 +713,14 @@ main(int argc, char **argv) } memcpy(id, &href.id, sizeof(*id)); if (href.delete) - err = got_pathlist_append(&delete_refs, refname, id); + err = got_pathlist_insert(&new, &delete_refs, refname, id); else - err = got_pathlist_append(&refs, refname, id); - if (err) { + err = got_pathlist_insert(&new, &refs, refname, id); + if (err || new == NULL ) { free(refname); free(id); - goto done; + if (err) + goto done; } imsg_free(&imsg); blob - 1c4930ff5c3ba4b4981d6378462f6c9d4d4c92da file + regress/cmdline/clone.sh --- regress/cmdline/clone.sh +++ regress/cmdline/clone.sh @@ -727,7 +727,7 @@ test_clone_multiple_branches() { got ref -l -r $testroot/repo-clone > $testroot/stdout - echo "HEAD: refs/heads/foo" > $testroot/stdout.expected + echo "HEAD: refs/heads/bar" > $testroot/stdout.expected echo "refs/heads/bar: $commit_id" >> $testroot/stdout.expected echo "refs/heads/foo: $commit_id" >> $testroot/stdout.expected echo "refs/remotes/origin/bar: $commit_id" \ @@ -748,7 +748,7 @@ remote "origin" { server 127.0.0.1 protocol ssh repository "$testroot/repo" - branch { "foo" "bar" } + branch { "bar" "foo" } } EOF cmp -s $testroot/repo-clone/got.conf $testroot/got.conf.expected @@ -768,8 +768,8 @@ EOF [remote "origin"] url = ssh://127.0.0.1$testroot/repo - fetch = refs/heads/foo:refs/remotes/origin/foo fetch = refs/heads/bar:refs/remotes/origin/bar + fetch = refs/heads/foo:refs/remotes/origin/foo fetch = refs/tags/*:refs/tags/* EOF cmp -s $testroot/repo-clone/config $testroot/config.expected blob - 59243b5c07792da8486b3128282d07cd79ae5bbe file + regress/cmdline/diff.sh --- regress/cmdline/diff.sh +++ regress/cmdline/diff.sh @@ -1941,8 +1941,8 @@ date: $d changed epsilon into file - D epsilon/zeta A epsilon + D epsilon/zeta Thoughts/comments/suggestions?