[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