Re: [PATCH v15 04/27] bisect--helper: `bisect_clean_state` shell function in C
- From
Stephan Beyer <s-beyer@gmx.net>
- Date
- Nov 15, 2016, 21:09 UTC
- Message-ID
- <13aa642a-2272-c5b8-4a30-382ab5e73b98@gmx.net>
- In-Reply-To
- <01020157c38b1a82-dc1c5b57-3e93-4996-87e7-4a1d83cb5817-000000@eu-west-1.amazonses.com>
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
Show 29 quoted lines
> diff --git a/bisect.c b/bisect.c
> index 6f512c2..45d598d 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -1040,3 +1046,40 @@ int estimate_bisect_steps(int all)
>
> return (e < 3 * x) ? n : n - 1;
> }
> +
> +static 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);
> + string_list_append(refs, ref);
> + return 0;
> +}
> +
> +int bisect_clean_state(void)
> +{
> + int result = 0;
> +
> + /* There may be some refs packed during bisection */
> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> + for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
> + string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
> + result = delete_refs(&refs_for_removal, REF_NODEREF);
> + refs_for_removal.strdup_strings = 1;
> + string_list_clear(&refs_for_removal, 0);Does it have advantages to populate a list (with duplicated strings), hand it to delete_refs(), and clear the list (and strings), instead of just doing a single delete_ref() (or whatever name the singular function has) in the callback? I am only seeing the disadvantage: a list with duped strings.
> + unlink_or_warn(git_path_bisect_expected_rev());
[...]
> + unlink_or_warn(git_path_bisect_start());
Comparing it with the original shell code (which uses "rm -f"), I was wondering a little after reading the function name unlink_or_warn() here... Looking it up helped: despite its name, unlink_or_warn() is really the function you have to use here ;D
Show 12 quoted lines
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 65cf519..4254d61 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -5,10 +5,15 @@ > #include "refs.h" > > static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") > +static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV") > +static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK") > +static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") > +static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
This is perhaps a non-issue, but you do not need any of these new path functions in *this* patch. I think nobody really cares, though, because you will need them later.
Cheers Stephan