[PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C
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 .
Using `--bisect-clean-state` subcommand is a measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by
bisect_reset() and bisect_start().
Mentored-by: Lars Schneider
Mentored-by: Christian Couder
Signed-off-by: Pranit Bauva
---
builtin/bisect--helper.c | 54 +++-
git-bisect.sh| 26 +++
2 files changed, 56 insertions(+), 24 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 91027b0..3e4a458 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -3,12 +3,21 @@
#include "parse-options.h"
#include "bisect.h"
#include "refs.h"
+#include "dir.h"
static GIT_PATH_FUNC(git_path_bisect_write_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_names, "BISECT_NAMES")
+static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
+static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms "),
+ N_("git bisect--helper --bisect-clean-state"),
NULL
};
@@ -78,11 +87,48 @@ static int write_terms(const char *bad, const char *good)
return (res < 0) ? -1 : 0;
}
+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;
+}
+
+static 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);
+ 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 to support the --no-checkout option
+* introduced in the commit 4796e823a.
+*/
+ remove_path(git_path_bisect_start());
+
+ return result;
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
NEXT_ALL = 1,
- WRITE_TERMS
+ WRITE_TERMS,
+ BISECT_CLEAN_STATE
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -90,6 +136,8 @@ int cmd_bisect__helper(int argc, const char **argv, const
char *prefix)
N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", &cmdmode,
N_("write the terms to .git/BISECT_TERMS"),
WRITE_TERMS),
+ OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
+N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the
current commit")),
OPT_END()
@@ -108,6 +156,10 @@ int cmd_bisect__helper(int argc, const char **argv, const
char *prefix)
if (argc != 2)
die(_("--write-terms requires two arguments"));
return write_terms(argv[0], argv[1]);
+ case BISECT_CLEAN_STATE:
+ if (argc != 0)
+ die(_("--bisect-clean-state requires no arguments"));
+ return bisect_clean_state();
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index cd39bd0..bbc57d2 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -187,7 +187,7 @@ bisect_start() {
#
# Get rid of any old bisect state.
#
- bisect_clean_state || exit
+ git bisect--helper --bisect-clean-state || exit
#
# Change state.
@@ -196,7 +196,7 @
Re: [PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C
On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva wrote:
> 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
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> +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;
> +}
> +
> +static 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);
> + string_list_clear(&refs_for_removal, 0);
This is leaking all the strings added to 'refs_for_removal', isn't it?
Either you need to loop over the items and free the strings manually,
or (if it's not too ugly), set 'strdup_strings' before invoking
string_list_clear().
> + 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 to support the --no-checkout option
> +* introduced in the commit 4796e823a.
> +*/
> + remove_path(git_path_bisect_start());
> +
> + return result;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C
Hey Eric,
On Wed, Jun 15, 2016 at 11:34 PM, Eric Sunshine wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva wrote:
>> 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
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> +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;
>> +}
>> +
>> +static 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);
>> + string_list_clear(&refs_for_removal, 0);
>
> This is leaking all the strings added to 'refs_for_removal', isn't it?
> Either you need to loop over the items and free the strings manually,
> or (if it's not too ugly), set 'strdup_strings' before invoking
> string_list_clear().
I didn't carefully see that in the function string_list_clear() it
only free()'s the memory if strdup_strings is 1. I think changing
strdup_strings to 1 would be an easy way out but it would make the
code very ugly and non-trivial. On the other hand, I can initialize
the string as STRING_LIST_INIT_DUP which will automatically set
strdup_strings as 1 and then also free the memory of ref at that point
after the string ref was appended to the list. Personally, I will
prefer the latter one.
Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C
On Wed, Jun 15, 2016 at 2:47 PM, Pranit Bauva wrote:
> On Wed, Jun 15, 2016 at 11:34 PM, Eric Sunshine
> wrote:
>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva
>> wrote:
>>> 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
>>> ---
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> +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;
>>> +}
>>> +
>>> +static 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);
>>> + string_list_clear(&refs_for_removal, 0);
>>
>> This is leaking all the strings added to 'refs_for_removal', isn't it?
>> Either you need to loop over the items and free the strings manually,
>> or (if it's not too ugly), set 'strdup_strings' before invoking
>> string_list_clear().
>
> I didn't carefully see that in the function string_list_clear() it
> only free()'s the memory if strdup_strings is 1. I think changing
> strdup_strings to 1 would be an easy way out but it would make the
> code very ugly and non-trivial.
I disagree about it making the code "*very* ugly and non-trivial". It
is quite trivial. What I meant by "ugly" was that it may be too
intimate with the implementation of string_list. However, since the
solution is already used in the codebase, it may be acceptable. For
instance, in builtin/fetch.c:
/* All names were strdup()ed or strndup()ed */
list.strdup_strings = 1;
string_list_clear(&list, 0);
which is exactly the approach I was suggesting. You'll find the same
pattern in builtin/shortlog.c.
> On the other hand, I can initialize
> the string as STRING_LIST_INIT_DUP which will automatically set
> strdup_strings as 1 and then also free the memory of ref at that point
> after the string ref was appended to the list. Personally, I will
> prefer the latter one.
Meh.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
