From b1f1ade87be595b5854c82850658c80465fdb16b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 27 Sep 2020 15:15:41 +0200 Subject: [PATCH 1/7] wt-status: replace sha1 mentions with oid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `abbrev_sha1_in_line()` uses a `struct object_id oid` and should be fully prepared to handle non-SHA1 object ids. Rename it to `abbrev_oid_in_line()`. A few comments in `wt_status_get_detached_from()` mention "sha1". The variable they refer to was renamed in e86ab2c1cd ("wt-status: convert to struct object_id", 2017-02-21). Update the comments to reference "oid" instead. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- wt-status.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/wt-status.c b/wt-status.c index bb0f9120de..528130614b 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1227,7 +1227,7 @@ static int split_commit_in_progress(struct wt_status *s) * The function assumes that the line does not contain useless spaces * before or after the command. */ -static void abbrev_sha1_in_line(struct strbuf *line) +static void abbrev_oid_in_line(struct strbuf *line) { struct strbuf **split; int i; @@ -1277,7 +1277,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines) strbuf_trim(&line); if (!line.len) continue; - abbrev_sha1_in_line(&line); + abbrev_oid_in_line(&line); string_list_append(lines, line.buf); } fclose(f); @@ -1570,9 +1570,9 @@ static void wt_status_get_detached_from(struct repository *r, } if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 && - /* sha1 is a commit? match without further lookup */ + /* oid is a commit? match without further lookup */ (oideq(&cb.noid, &oid) || - /* perhaps sha1 is a tag, try to dereference to a commit */ + /* perhaps oid is a tag, try to dereference to a commit */ ((commit = lookup_commit_reference_gently(r, &oid, 1)) != NULL && oideq(&cb.noid, &commit->object.oid)))) { const char *from = ref; From 8f7e3de0970c419688f23f505b5cb7a9690d9b09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 27 Sep 2020 15:15:42 +0200 Subject: [PATCH 2/7] wt-status: print to s->fp, not stdout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We pass around a `FILE *` in the `struct wt_status` and almost always print to it. But in a few places, we write to `stdout` instead, either explicitly through `fprintf(stdout, ...)` or implicitly with `printf(...)` (and a few `putchar(...)`). Always be explicit about writing to `s->fp`. To the best of my understanding, this never mattered in practice because these spots are involved in various forms of `git status` which always end up at standard output anyway. When we do write to another file, it's because we're creating a commit message template, and these code paths aren't involved. But let's be consistent to help future readers and avoid future bugs. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- wt-status.c | 57 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/wt-status.c b/wt-status.c index 528130614b..4a7483ee7d 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1801,29 +1801,36 @@ static void wt_longstatus_print(struct wt_status *s) ; /* nothing */ else if (s->workdir_dirty) { if (s->hints) - printf(_("no changes added to commit " - "(use \"git add\" and/or \"git commit -a\")\n")); + fprintf(s->fp, _("no changes added to commit " + "(use \"git add\" and/or " + "\"git commit -a\")\n")); else - printf(_("no changes added to commit\n")); + fprintf(s->fp, _("no changes added to " + "commit\n")); } else if (s->untracked.nr) { if (s->hints) - printf(_("nothing added to commit but untracked files " - "present (use \"git add\" to track)\n")); + fprintf(s->fp, _("nothing added to commit but " + "untracked files present (use " + "\"git add\" to track)\n")); else - printf(_("nothing added to commit but untracked files present\n")); + fprintf(s->fp, _("nothing added to commit but " + "untracked files present\n")); } else if (s->is_initial) { if (s->hints) - printf(_("nothing to commit (create/copy files " - "and use \"git add\" to track)\n")); + fprintf(s->fp, _("nothing to commit (create/" + "copy files and use \"git " + "add\" to track)\n")); else - printf(_("nothing to commit\n")); + fprintf(s->fp, _("nothing to commit\n")); } else if (!s->show_untracked_files) { if (s->hints) - printf(_("nothing to commit (use -u to show untracked files)\n")); + fprintf(s->fp, _("nothing to commit (use -u to " + "show untracked files)\n")); else - printf(_("nothing to commit\n")); + fprintf(s->fp, _("nothing to commit\n")); } else - printf(_("nothing to commit, working tree clean\n")); + fprintf(s->fp, _("nothing to commit, working tree " + "clean\n")); } if(s->show_stash) wt_longstatus_print_stash_summary(s); @@ -1846,12 +1853,12 @@ static void wt_shortstatus_unmerged(struct string_list_item *it, } color_fprintf(s->fp, color(WT_STATUS_UNMERGED, s), "%s", how); if (s->null_termination) { - fprintf(stdout, " %s%c", it->string, 0); + fprintf(s->fp, " %s%c", it->string, 0); } else { struct strbuf onebuf = STRBUF_INIT; const char *one; one = quote_path(it->string, s->prefix, &onebuf); - printf(" %s\n", one); + fprintf(s->fp, " %s\n", one); strbuf_release(&onebuf); } } @@ -1864,16 +1871,16 @@ static void wt_shortstatus_status(struct string_list_item *it, if (d->index_status) color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%c", d->index_status); else - putchar(' '); + fputc(' ', s->fp); if (d->worktree_status) color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%c", d->worktree_status); else - putchar(' '); - putchar(' '); + fputc(' ', s->fp); + fputc(' ', s->fp); if (s->null_termination) { - fprintf(stdout, "%s%c", it->string, 0); + fprintf(s->fp, "%s%c", it->string, 0); if (d->rename_source) - fprintf(stdout, "%s%c", d->rename_source, 0); + fprintf(s->fp, "%s%c", d->rename_source, 0); } else { struct strbuf onebuf = STRBUF_INIT; const char *one; @@ -1881,20 +1888,20 @@ static void wt_shortstatus_status(struct string_list_item *it, if (d->rename_source) { one = quote_path(d->rename_source, s->prefix, &onebuf); if (*one != '"' && strchr(one, ' ') != NULL) { - putchar('"'); + fputc('"', s->fp); strbuf_addch(&onebuf, '"'); one = onebuf.buf; } - printf("%s -> ", one); + fprintf(s->fp, "%s -> ", one); strbuf_release(&onebuf); } one = quote_path(it->string, s->prefix, &onebuf); if (*one != '"' && strchr(one, ' ') != NULL) { - putchar('"'); + fputc('"', s->fp); strbuf_addch(&onebuf, '"'); one = onebuf.buf; } - printf("%s\n", one); + fprintf(s->fp, "%s\n", one); strbuf_release(&onebuf); } } @@ -1903,13 +1910,13 @@ static void wt_shortstatus_other(struct string_list_item *it, struct wt_status *s, const char *sign) { if (s->null_termination) { - fprintf(stdout, "%s %s%c", sign, it->string, 0); + fprintf(s->fp, "%s %s%c", sign, it->string, 0); } else { struct strbuf onebuf = STRBUF_INIT; const char *one; one = quote_path(it->string, s->prefix, &onebuf); color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign); - printf(" %s\n", one); + fprintf(s->fp, " %s\n", one); strbuf_release(&onebuf); } } From 962dd7ebc3e76afc2c896d377c319f8140966303 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 27 Sep 2020 15:15:43 +0200 Subject: [PATCH 3/7] wt-status: introduce wt_status_state_free_buffers() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we have a `struct wt_status_state`, we manually free its `branch`, `onto` and `detached_from`, or sometimes just one or two of them. Provide a function `wt_status_state_free_buffers()` which does the freeing. The callers are still aware of these fields, e.g., they check whether `branch` was populated or not. But this way, they don't need to know about *all* of them, and if `struct wt_status_state` gets more fields, they will not need to learn to free them. Users of `struct wt_status` (which contains a `wt_status_state`) already have `wt_status_collect_free_buffers()` (corresponding to `wt_status_collect()`) which we can also teach to use this new helper. Finally, note that we're currently leaving dangling pointers behind. Some callers work on a stack-allocated struct, where this is obviously ok. But for the users of `run_status()` in builtin/commit.c, there are ample opportunities for someone to mistakenly use those dangling pointers. We seem to be ok for now, but it's a use-after-free waiting to happen. Let's leave NULL-pointers behind instead. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- ref-filter.c | 4 +--- worktree.c | 5 ++--- wt-status.c | 11 ++++++++--- wt-status.h | 7 +++++++ 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 8ba0e31915..ea00ca38c8 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1509,9 +1509,7 @@ char *get_head_description(void) strbuf_addstr(&desc, _("no branch")); strbuf_addch(&desc, ')'); - free(state.branch); - free(state.onto); - free(state.detached_from); + wt_status_state_free_buffers(&state); return strbuf_detach(&desc, NULL); } diff --git a/worktree.c b/worktree.c index 62217b4a6b..62a7eb9342 100644 --- a/worktree.c +++ b/worktree.c @@ -357,8 +357,7 @@ int is_worktree_being_rebased(const struct worktree *wt, state.branch && starts_with(target, "refs/heads/") && !strcmp(state.branch, target + strlen("refs/heads/"))); - free(state.branch); - free(state.onto); + wt_status_state_free_buffers(&state); return found_rebase; } @@ -373,7 +372,7 @@ int is_worktree_being_bisected(const struct worktree *wt, state.branch && starts_with(target, "refs/heads/") && !strcmp(state.branch, target + strlen("refs/heads/")); - free(state.branch); + wt_status_state_free_buffers(&state); return found_rebase; } diff --git a/wt-status.c b/wt-status.c index 4a7483ee7d..4345eb1749 100644 --- a/wt-status.c +++ b/wt-status.c @@ -778,9 +778,14 @@ void wt_status_collect(struct wt_status *s) void wt_status_collect_free_buffers(struct wt_status *s) { - free(s->state.branch); - free(s->state.onto); - free(s->state.detached_from); + wt_status_state_free_buffers(&s->state); +} + +void wt_status_state_free_buffers(struct wt_status_state *state) +{ + FREE_AND_NULL(state->branch); + FREE_AND_NULL(state->onto); + FREE_AND_NULL(state->detached_from); } static void wt_longstatus_print_unmerged(struct wt_status *s) diff --git a/wt-status.h b/wt-status.h index f1fa0ec1a7..35b44c388e 100644 --- a/wt-status.h +++ b/wt-status.h @@ -151,7 +151,14 @@ void wt_status_add_cut_line(FILE *fp); void wt_status_prepare(struct repository *r, struct wt_status *s); void wt_status_print(struct wt_status *s); void wt_status_collect(struct wt_status *s); +/* + * Frees the buffers allocated by wt_status_collect. + */ void wt_status_collect_free_buffers(struct wt_status *s); +/* + * Frees the buffers of the wt_status_state. + */ +void wt_status_state_free_buffers(struct wt_status_state *s); void wt_status_get_state(struct repository *repo, struct wt_status_state *state, int get_detached_from); From ef2d5547fa342197befd4be599438d7a7fa41e04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 27 Sep 2020 15:15:44 +0200 Subject: [PATCH 4/7] worktree: inline `worktree_ref()` into its only caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have `strbuf_worktree_ref()`, which works on a strbuf, and a wrapper for it, `worktree_ref()` which returns a string. We even make this wrapper available through worktree.h. But it only has a single caller, sitting right next to it in worktree.c. Just inline the wrapper into its only caller. This means the caller can quite naturally reuse a single strbuf. We currently achieve something similar by having a static strbuf in the wrapper. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- worktree.c | 17 ++++++----------- worktree.h | 7 ------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/worktree.c b/worktree.c index 62a7eb9342..a98f77d19f 100644 --- a/worktree.c +++ b/worktree.c @@ -536,18 +536,10 @@ void strbuf_worktree_ref(const struct worktree *wt, strbuf_addstr(sb, refname); } -const char *worktree_ref(const struct worktree *wt, const char *refname) -{ - static struct strbuf sb = STRBUF_INIT; - - strbuf_reset(&sb); - strbuf_worktree_ref(wt, &sb, refname); - return sb.buf; -} - int other_head_refs(each_ref_fn fn, void *cb_data) { struct worktree **worktrees, **p; + struct strbuf refname = STRBUF_INIT; int ret = 0; worktrees = get_worktrees(); @@ -559,14 +551,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data) if (wt->is_current) continue; + strbuf_reset(&refname); + strbuf_worktree_ref(wt, &refname, "HEAD"); if (!refs_read_ref_full(get_main_ref_store(the_repository), - worktree_ref(wt, "HEAD"), + refname.buf, RESOLVE_REF_READING, &oid, &flag)) - ret = fn(worktree_ref(wt, "HEAD"), &oid, flag, cb_data); + ret = fn(refname.buf, &oid, flag, cb_data); if (ret) break; } free_worktrees(worktrees); + strbuf_release(&refname); return ret; } diff --git a/worktree.h b/worktree.h index 516744c433..1449b6bf5d 100644 --- a/worktree.h +++ b/worktree.h @@ -136,11 +136,4 @@ void strbuf_worktree_ref(const struct worktree *wt, struct strbuf *sb, const char *refname); -/* - * Return a refname suitable for access from the current ref - * store. The result will be destroyed at the next call. - */ -const char *worktree_ref(const struct worktree *wt, - const char *refname); - #endif From cfaf9f05c6174b520082036c0f1439adf9c4fbf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 27 Sep 2020 15:15:45 +0200 Subject: [PATCH 5/7] worktree: update renamed variable in comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment above `add_head_info()` mentions "head_sha1", but it was renamed to "head_oid" in 0f05154c70 ("worktree: convert struct worktree to object_id", 2017-10-15). Update the comment. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- worktree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worktree.c b/worktree.c index a98f77d19f..f5eb6e7636 100644 --- a/worktree.c +++ b/worktree.c @@ -21,7 +21,7 @@ void free_worktrees(struct worktree **worktrees) } /** - * Update head_sha1, head_ref and is_detached of the given worktree + * Update head_oid, head_ref and is_detached of the given worktree */ static void add_head_info(struct worktree *wt) { From fb07bd42975bcbfbc29d4a3ef1bff1039a469336 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 27 Sep 2020 15:15:46 +0200 Subject: [PATCH 6/7] worktree: rename copy-pasted variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As the commit message of 04a3dfb8b5 ("worktree.c: check whether branch is bisected in another worktree", 2016-04-22) indicates, the function `is_worktree_being_bisected()` is based on the older function `is_worktree_being_rebased()`. This heritage can also be seen in the name of the variable where we store our return value: It was never adapted while copy-editing and remains as `found_rebase`. Rename the variable to make clear that we're looking for a bisect(ion), nothing else. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- worktree.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/worktree.c b/worktree.c index f5eb6e7636..6a236d2e72 100644 --- a/worktree.c +++ b/worktree.c @@ -365,15 +365,15 @@ int is_worktree_being_bisected(const struct worktree *wt, const char *target) { struct wt_status_state state; - int found_rebase; + int found_bisect; memset(&state, 0, sizeof(state)); - found_rebase = wt_status_check_bisect(wt, &state) && - state.branch && - starts_with(target, "refs/heads/") && - !strcmp(state.branch, target + strlen("refs/heads/")); + found_bisect = wt_status_check_bisect(wt, &state) && + state.branch && + starts_with(target, "refs/heads/") && + !strcmp(state.branch, target + strlen("refs/heads/")); wt_status_state_free_buffers(&state); - return found_rebase; + return found_bisect; } /* From a46d1f732192a8621ead7ea5c4a3ca391ad881cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 27 Sep 2020 15:15:47 +0200 Subject: [PATCH 7/7] worktree: use skip_prefix to parse target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of checking for "refs/heads/" using `starts_with()`, then skipping past "refs/heads/" using `strlen()`, just use `skip_prefix()`. In `is_worktree_being_rebased()`, we can adjust the indentation while we're here and lose a pair of parentheses which isn't needed and which might even make the reader wonder what they're missing and why that grouping is there. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- worktree.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/worktree.c b/worktree.c index 6a236d2e72..0b12525697 100644 --- a/worktree.c +++ b/worktree.c @@ -352,11 +352,11 @@ int is_worktree_being_rebased(const struct worktree *wt, memset(&state, 0, sizeof(state)); found_rebase = wt_status_check_rebase(wt, &state) && - ((state.rebase_in_progress || - state.rebase_interactive_in_progress) && - state.branch && - starts_with(target, "refs/heads/") && - !strcmp(state.branch, target + strlen("refs/heads/"))); + (state.rebase_in_progress || + state.rebase_interactive_in_progress) && + state.branch && + skip_prefix(target, "refs/heads/", &target) && + !strcmp(state.branch, target); wt_status_state_free_buffers(&state); return found_rebase; } @@ -370,8 +370,8 @@ int is_worktree_being_bisected(const struct worktree *wt, memset(&state, 0, sizeof(state)); found_bisect = wt_status_check_bisect(wt, &state) && state.branch && - starts_with(target, "refs/heads/") && - !strcmp(state.branch, target + strlen("refs/heads/")); + skip_prefix(target, "refs/heads/", &target) && + !strcmp(state.branch, target); wt_status_state_free_buffers(&state); return found_bisect; }