[RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C

[RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

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 <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
This patch contains a bug. I have tried to identify the bug and I suppose it
exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
at this link[1]. I have seen that some patches in mailing list regarding
iterating over refs. Will those affect this? Or is this bug fixed in those
patches?

[1]: http://paste.ubuntu.com/16830752/

 builtin/bisect--helper.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 26 +++----------------------
 2 files changed, 52 insertions(+), 24 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 2b21c02..a20c9a5 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -3,10 +3,21 @@
 #include "parse-options.h"
 #include "bisect.h"
 #include "refs.h"
+#include "dir.h"
+
+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_bisect_terms, "BISECT_TERMS")
+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 <bad_term> <good_term>"),
+	N_("git bisect--helper --bisect-clean-state"),
 	NULL
 };
 
@@ -79,11 +90,42 @@ int write_terms(const char *bad, const char *good)
 	strbuf_release(&content);
 	return (res < 0) ? -1 : 0;
 }
+
+int remove_bisect_ref(const char *refname, const struct object_id *oid,
+		       int flag, void *cb_data)
+{
+	char *ref;
+	ref = xstrfmt("refs/bisect/%s", refname);
+	if (delete_ref(ref, oid->hash, flag))
+		return error(_("couldn't delete the ref %s\n"), ref);
+	return 0;
+}
+
+int bisect_clean_state(void)
+{
+	for_each_ref_in("refs/bisect/", remove_bisect_ref, NULL);
+
+	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_terms());
+	/* Cleanup head-name if it got left by an old version of git-bisect */
+	remove_path(git_path_head_name());
+	delete_ref("BISECT_HEAD", NULL, 0);
+	/* Cleanup BISECT_START last */
+	remove_path(git_path_bisect_start());
+
+	return 0;
+}
+
 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[] = {
@@ -91,6 +133,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()
@@ -109,6 +153,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 2dd7ec5..a937d1c 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 @@ bisect_start() {
 	# We have to trap this to be able to clean up using
 	# "bisect_clean_state".
 	#
-	trap 'bisect_clean_state' 0
+	trap 'git bisect--helper --bisect-clean-state' 0
 	trap 'exit 255' 1 2 3 15
 
 	#
@@ -430,27 +430,7 @@ bisect_reset() {
 		die "$(eval_gettext "Could not check out original HEAD '\$branch'.
 Try 'git bisect reset <commit>'.")"
 	fi
-	bisect_clean_state
-}
-
-bisect_clean_state() {
-	# There may be some refs packed during bisection.
-	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
 }
 
 bisect_replay () {
-- 
2.8.3

Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C

[Christian Couder]

On Mon, May 30, 2016 at 8:21 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> ---
> This patch contains a bug. I have tried to identify the bug and I suppose it
> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
> at this link[1]. I have seen that some patches in mailing list regarding
> iterating over refs. Will those affect this? Or is this bug fixed in those
> patches?
>
> [1]: http://paste.ubuntu.com/16830752/

The debug session seems to use code source from a previous version of
this patch.
Also it is not cear in which context you run git under gdb. What have
you done before?
And we don't see a crash. Could you show the crash and run the "bt"
command in gdb to get a backtrace?

> @@ -79,11 +90,42 @@ int write_terms(const char *bad, const char *good)
>         strbuf_release(&content);
>         return (res < 0) ? -1 : 0;
>  }
> +
> +int remove_bisect_ref(const char *refname, const struct object_id *oid,
> +                      int flag, void *cb_data)
> +{
> +       char *ref;
> +       ref = xstrfmt("refs/bisect/%s", refname);

You could save one line by concatenating the 2 above lines.

> +       if (delete_ref(ref, oid->hash, flag))
> +               return error(_("couldn't delete the ref %s\n"), ref);
> +       return 0;

You need to free "ref".

> +}

Thanks,
Christian.

Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C

[Michael Haggerty]

On 05/30/2016 08:21 PM, 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 .
> 
> 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 <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> This patch contains a bug. I have tried to identify the bug and I suppose it
> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
> at this link[1]. I have seen that some patches in mailing list regarding
> iterating over refs. Will those affect this? Or is this bug fixed in those
> patches?

The problem is that it is not legal to modify references while iterating
over them. See [1]. Your remove_bisect_ref() callback function deletes
references, which modifies the reference cache that is being iterated over.

Instead I suggest that your remove_bisect_ref() add the references to a
string_list, then call delete_refs() *after* the iteration is over.
Alternatively, you can change remove_bisect_ref() to call
ref_transaction_delete() to add reference deletions to a
ref_transaction, then call ref_transaction_commit() after the iteration
is over. See the rm() function in builtin/remote.c [2] for an example.

[1]
https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/refs.h#L176-L184
[2]
https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/builtin/remote.c#L738

> [...]

Michael

Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Hey Christian,

On Tue, May 31, 2016 at 3:18 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, May 30, 2016 at 8:21 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> ---
>> This patch contains a bug. I have tried to identify the bug and I suppose it
>> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
>> at this link[1]. I have seen that some patches in mailing list regarding
>> iterating over refs. Will those affect this? Or is this bug fixed in those
>> patches?
>>
>> [1]: http://paste.ubuntu.com/16830752/
>
> The debug session seems to use code source from a previous version of
> this patch.
> Also it is not cear in which context you run git under gdb. What have
> you done before?
> And we don't see a crash. Could you show the crash and run the "bt"
> command in gdb to get a backtrace?

The segmentation fault will occur if I move to the next step ie.
evaluate the value entry->flag.

>> @@ -79,11 +90,42 @@ int write_terms(const char *bad, const char *good)
>>         strbuf_release(&content);
>>         return (res < 0) ? -1 : 0;
>>  }
>> +
>> +int remove_bisect_ref(const char *refname, const struct object_id *oid,
>> +                      int flag, void *cb_data)
>> +{
>> +       char *ref;
>> +       ref = xstrfmt("refs/bisect/%s", refname);
>
> You could save one line by concatenating the 2 above lines.

Sure!

>> +       if (delete_ref(ref, oid->hash, flag))
>> +               return error(_("couldn't delete the ref %s\n"), ref);
>> +       return 0;
>
> You need to free "ref".

Sure!

Regards,
Pranit Bauva

Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Hey Michael,

On Tue, May 31, 2016 at 9:55 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/30/2016 08:21 PM, 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 .
>>
>> 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 <larsxschneider@gmail.com>
>> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> This patch contains a bug. I have tried to identify the bug and I suppose it
>> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
>> at this link[1]. I have seen that some patches in mailing list regarding
>> iterating over refs. Will those affect this? Or is this bug fixed in those
>> patches?
>
> The problem is that it is not legal to modify references while iterating
> over them. See [1]. Your remove_bisect_ref() callback function deletes
> references, which modifies the reference cache that is being iterated over.

Thanks for explaining this. I wasn't aware about this.

> Instead I suggest that your remove_bisect_ref() add the references to a
> string_list, then call delete_refs() *after* the iteration is over.
> Alternatively, you can change remove_bisect_ref() to call
> ref_transaction_delete() to add reference deletions to a
> ref_transaction, then call ref_transaction_commit() after the iteration
> is over. See the rm() function in builtin/remote.c [2] for an example.

I have gone through the links and I think I will stick with the first
one because I don't see how the latter one would be better.

> [1]
> https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/refs.h#L176-L184
> [2]
> https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/builtin/remote.c#L738
>
>> [...]
>

Regards,
Pranit Bauva