Re: [PATCH 1/4] bisect--helper: `bisect_clean_state` shell function in C
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jun 7, 2016, 22:31 UTC
- Message-ID
- <CAPig+cTqk0d60Omsm12RXF2uzoxoPjbqHj7V8Y=6Uu+fH5b43Q@mail.gmail.com>
- In-Reply-To
- <20160607205454.22576-1-pranit.bauva@gmail.com>
On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 13 quoted lines
> Reimplement `bisect_clean_state` shell function in C and add a
> `bisect-clean-state` subcommand to `git bisect--helper` to call it from
> git-bisect.sh .
> [...]
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -78,11 +87,43 @@ static int write_terms(const char *bad, const char *good)
> +int mark_for_removal(const char *refname, const struct object_id *oid,
> + int flag, void *cb_data)
> +{
> + struct string_list *refs = cb_data;
> + char *ref = xstrfmt("refs/bisect/%s", refname);Here you're allocating a string...
Show 9 quoted lines
> + string_list_append(refs, ref);
> + return 0;
> +}
> +
> +int bisect_clean_state(void)
> +{
> + int result = 0;
> + struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
> + for_each_ref_in("refs/bisect/", mark_for_removal, (void *) &refs_for_removal);...and the allocated string gets inserted into a string_list which itself duplicates the string (STRING_LIST_INIT_DUP), so this is leaking the string you created with xstrfmt(), isn't it?
> + string_list_append(&refs_for_removal, "BISECT_HEAD"); > + result |= delete_refs(&refs_for_removal);
Not sure I understand the point of using |= here rather than merely = since this is the only place 'result' is assigned in this function.
Show 11 quoted lines
> + string_list_clear(&refs_for_removal, 0); > + remove_path(git_path_bisect_expected_rev()); > + remove_path(git_path_bisect_ancestors_ok()); > + remove_path(git_path_bisect_log()); > + remove_path(git_path_bisect_names()); > + remove_path(git_path_bisect_run()); > + remove_path(git_path_bisect_write_terms()); > + /* Cleanup head-name if it got left by an old version of git-bisect */ > + remove_path(git_path_head_name()); > + /* Cleanup BISECT_START last */ > + remove_path(git_path_bisect_start());
The BISECT_START comment merely repeats what the code itself already says. I realize that you merely copied this from the shell code, but it isn't helpful in its current form. Much more helpful would be to explain *why* this needs to be done last. Perhaps the commit message of the commit which introduced the comment originally would give a clue (I haven't checked).
Show 6 quoted lines
> + return result;
> +}
> diff --git a/git-bisect.sh b/git-bisect.sh
> @@ -430,27 +430,7 @@ bisect_reset() {
> -bisect_clean_state() {
> - # There may be some refs packed during bisection.This comment doesn't seem to be reproduced in the C version. Should it be? Is it no longer relevant in the C version? What does it mean exactly?
Show 18 quoted lines
> - git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* | > - while read ref hash > - do > - git update-ref -d $ref $hash || exit > - done > - rm -f "$GIT_DIR/BISECT_EXPECTED_REV" && > - rm -f "$GIT_DIR/BISECT_ANCESTORS_OK" && > - rm -f "$GIT_DIR/BISECT_LOG" && > - rm -f "$GIT_DIR/BISECT_NAMES" && > - rm -f "$GIT_DIR/BISECT_RUN" && > - rm -f "$GIT_DIR/BISECT_TERMS" && > - # Cleanup head-name if it got left by an old version of git-bisect > - rm -f "$GIT_DIR/head-name" && > - git update-ref -d --no-deref BISECT_HEAD && > - # clean up BISECT_START last > - rm -f "$GIT_DIR/BISECT_START" > + git bisect--helper --bisect-clean-state || exit > }