threads / patch / 42559

patch, 4 partsbisect--helper: `bisect_clean_state` shell function in C

Subject: [PATCH 1/4] bisect--helper: `bisect_clean_state` shell function in C

## tl;dr

75 messages between Jun 7, 2016 and Jul 11, 2016. Diffs are folded; open one to read it.

replies: 74people: 5as markdown or json

Pranit Bauva· Jun 7, 2016, 20:54 UTC · lore

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>
---
 builtin/bisect--helper.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 26 +++----------------------
 2 files changed, 51 insertions(+), 24 deletions(-)
Show changes to 2 files +51 −24

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 91027b0..ad39181 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 <bad_term> <good_term>"),
+	N_("git bisect--helper --bisect-clean-state"),
 	NULL
 };
 
@@ -78,11 +87,43 @@ static int write_terms(const char *bad, const char *good)
 	return (res < 0) ? -1 : 0;
 }
 
+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;
+	struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
+	for_each_ref_in("refs/bisect/", mark_for_removal, (void *) &refs_for_removal);
+	string_list_append(&refs_for_removal, "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 */
+	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 +131,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 +151,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 @@ 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
Pranit Bauva· Jun 7, 2016, 20:54 UTC · re: Pranit Bauva · lore

[PATCH 2/4] t6030: explicitly test for bisection cleanup

This is not an improvement in the test coverage but it helps in making it explicit as to what exactly would be the error as other tests are focussed on testing other things.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

--- I faced this problem while converting `bisect_clean_state` and the tests where showing breakages but it wasn't clear as to where exactly are they breaking. This will patch will help in that. Also I tested the test coverage of the test suite before this patch and it covers this (I did this by purposely changing names of files in git-bisect.sh and running the test suite).

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Show changes to t/t6030-bisect-porcelain.sh +17 −0
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index e74662b..1fb5ad9 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git bisect reset cleans bisection state properly' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect reset &&
+	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	! test -s "$GIT_DIR/BISECT_EXPECTED_REV" &&
+	! test -s "$GIT_DIR/BISECT_ANCESTORS_OK" &&
+	! test -s "$GIT_DIR/BISECT_LOG" &&
+	! test -s "$GIT_DIR/BISECT_RUN" &&
+	! test -s "$GIT_DIR/BISECT_TERMS" &&
+	! test -s "$GIT_DIR/head-name" &&
+	! test -s "$GIT_DIR/BISECT_HEAD" &&
+	! test -s "$GIT_DIR/BISECT_START"
+'
+
 test_done
-- 
2.8.3
Eric Sunshine· Jun 7, 2016, 23:21 UTC · re: Pranit Bauva · lore

Re: [PATCH 2/4] t6030: explicitly test for bisection cleanup

On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> This is not an improvement in the test coverage but it helps in making
> it explicit as to what exactly would be the error as other tests are
> focussed on testing other things.
It's not clear why you consider this as *not* improving test coverage.
Show 11 quoted lines
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
> +test_expect_success 'git bisect reset cleans bisection state properly' '
> +       git bisect reset &&
> +       git bisect start &&
> +       git bisect good $HASH1 &&
> +       git bisect bad $HASH4 &&
> +       git bisect reset &&
> +       test -z "$(git for-each-ref "refs/bisect/*")" &&
I wonder if this would be more easily read as:
    git for-each-ref "refs/bisect/*" >actual &&
    test_must_be_empty actual &&
Show 8 quoted lines
> +       ! test -s "$GIT_DIR/BISECT_EXPECTED_REV" &&
> +       ! test -s "$GIT_DIR/BISECT_ANCESTORS_OK" &&
> +       ! test -s "$GIT_DIR/BISECT_LOG" &&
> +       ! test -s "$GIT_DIR/BISECT_RUN" &&
> +       ! test -s "$GIT_DIR/BISECT_TERMS" &&
> +       ! test -s "$GIT_DIR/head-name" &&
> +       ! test -s "$GIT_DIR/BISECT_HEAD" &&
> +       ! test -s "$GIT_DIR/BISECT_START"

Is it the intention that these should verify that the files don't exist? Maybe use test_path_is_missing() instead?

> +'
> +
>  test_done
Pranit Bauva· Jun 8, 2016, 08:07 UTC · re: Eric Sunshine · lore

Re: [PATCH 2/4] t6030: explicitly test for bisection cleanup

Hey Eric,
On Wed, Jun 8, 2016 at 4:51 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 6 quoted lines
> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> This is not an improvement in the test coverage but it helps in making
>> it explicit as to what exactly would be the error as other tests are
>> focussed on testing other things.
>
> It's not clear why you consider this as *not* improving test coverage.

My mistake as I forgot to include a comment in this patch. I did it in the previous patch[1]. I manually changed the file names and saw that there were a couple of tests failing in each case.

Show 16 quoted lines
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
>> @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
>> +test_expect_success 'git bisect reset cleans bisection state properly' '
>> +       git bisect reset &&
>> +       git bisect start &&
>> +       git bisect good $HASH1 &&
>> +       git bisect bad $HASH4 &&
>> +       git bisect reset &&
>> +       test -z "$(git for-each-ref "refs/bisect/*")" &&
>
> I wonder if this would be more easily read as:
>
>     git for-each-ref "refs/bisect/*" >actual &&
>     test_must_be_empty actual &&

I just tried to imitate what the test t6030 previously had (a lot of occurrences). Should I still change it to your specified format? Should I also change the others as a side cleanup "while I am at it"?

Show 11 quoted lines
>> +       ! test -s "$GIT_DIR/BISECT_EXPECTED_REV" &&
>> +       ! test -s "$GIT_DIR/BISECT_ANCESTORS_OK" &&
>> +       ! test -s "$GIT_DIR/BISECT_LOG" &&
>> +       ! test -s "$GIT_DIR/BISECT_RUN" &&
>> +       ! test -s "$GIT_DIR/BISECT_TERMS" &&
>> +       ! test -s "$GIT_DIR/head-name" &&
>> +       ! test -s "$GIT_DIR/BISECT_HEAD" &&
>> +       ! test -s "$GIT_DIR/BISECT_START"
>
> Is it the intention that these should verify that the files don't
> exist? Maybe use test_path_is_missing() instead?

True. In fact it would be much more appropriate to use test_path_is_missing() as we are using remove_path() and thus there shouldn't even exist a file with file size != 0. Thanks!

>> +'
>> +
>>  test_done
[1]: http://thread.gmane.org/gmane.comp.version-control.git/294520

Regards, Pranit Bauva

Eric Sunshine· Jun 8, 2016, 08:17 UTC · re: Pranit Bauva · lore

Re: [PATCH 2/4] t6030: explicitly test for bisection cleanup

On Wed, Jun 8, 2016 at 4:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 20 quoted lines
> On Wed, Jun 8, 2016 at 4:51 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
>>> @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
>>> +test_expect_success 'git bisect reset cleans bisection state properly' '
>>> +       git bisect reset &&
>>> +       git bisect start &&
>>> +       git bisect good $HASH1 &&
>>> +       git bisect bad $HASH4 &&
>>> +       git bisect reset &&
>>> +       test -z "$(git for-each-ref "refs/bisect/*")" &&
>>
>> I wonder if this would be more easily read as:
>>
>>     git for-each-ref "refs/bisect/*" >actual &&
>>     test_must_be_empty actual &&
>
> I just tried to imitate what the test t6030 previously had (a lot of
> occurrences). Should I still change it to your specified format?
> Should I also change the others as a side cleanup "while I am at it"?

No, if the 'test -z "$(...)"' is already used heavily in that script, just stick with it. As for a side cleanup, perhaps if you have time later on, but don't let it derail your project timeline. It's not that important.

Pranit Bauva· Jun 8, 2016, 10:20 UTC · re: Eric Sunshine · lore

Re: [PATCH 2/4] t6030: explicitly test for bisection cleanup

Hey Eric,
On Wed, Jun 8, 2016 at 1:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 26 quoted lines
> On Wed, Jun 8, 2016 at 4:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, Jun 8, 2016 at 4:51 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
>>>> @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
>>>> +test_expect_success 'git bisect reset cleans bisection state properly' '
>>>> +       git bisect reset &&
>>>> +       git bisect start &&
>>>> +       git bisect good $HASH1 &&
>>>> +       git bisect bad $HASH4 &&
>>>> +       git bisect reset &&
>>>> +       test -z "$(git for-each-ref "refs/bisect/*")" &&
>>>
>>> I wonder if this would be more easily read as:
>>>
>>>     git for-each-ref "refs/bisect/*" >actual &&
>>>     test_must_be_empty actual &&
>>
>> I just tried to imitate what the test t6030 previously had (a lot of
>> occurrences). Should I still change it to your specified format?
>> Should I also change the others as a side cleanup "while I am at it"?
>
> No, if the 'test -z "$(...)"' is already used heavily in that script,
> just stick with it. As for a side cleanup, perhaps if you have time
> later on, but don't let it derail your project timeline. It's not that
> important.
Sure! I can mark it as "to be cleaned up after GSoC"

Regards, Pranit Bauva

Pranit Bauva· Jun 7, 2016, 20:54 UTC · re: Pranit Bauva · lore

[PATCH 3/4] dir: introduce file_size() to check the size of file

At times we require to see if the file is empty and get the size of the file. By using stat we can get the file size without actually having to open the file to check for its contents.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 dir.c | 8 ++++++++
 dir.h | 7 +++++++
 2 files changed, 15 insertions(+)
Show changes to 2 files +15 −0

dir.c, dir.h

diff --git a/dir.c b/dir.c
index 6172b34..eaee718 100644
--- a/dir.c
+++ b/dir.c
@@ -2036,6 +2036,14 @@ int file_exists(const char *f)
 	return lstat(f, &sb) == 0;
 }
 
+ssize_t file_size(const char *filename)
+{
+	struct stat st;
+	if (stat(filename, &st) < 0)
+		return -1;
+	return xsize_t(st.st_size);
+}
+
 static int cmp_icase(char a, char b)
 {
 	if (a == b)
diff --git a/dir.h b/dir.h
index bfde698..aa9d276 100644
--- a/dir.h
+++ b/dir.h
@@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list *el);
 extern void clear_directory(struct dir_struct *dir);
 extern int file_exists(const char *);
 
+/*
+ * Return the size of the file `filename`. It returns -1 if error
+ * occurred, 0 if file is empty and a positive number denoting the size
+ * of the file.
+ */
+extern ssize_t file_size(const char *);
+
 extern int is_inside_dir(const char *dir);
 extern int dir_inside_of(const char *subdir, const char *dir);
 
-- 
2.8.3
Eric Sunshine· Jun 8, 2016, 07:37 UTC · re: Pranit Bauva · lore

Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 5 quoted lines
> dir: introduce file_size() to check the size of file
>
> At times we require to see if the file is empty and get the size of the
> file. By using stat we can get the file size without actually having to
> open the file to check for its contents.

The sole caller of this function in patch 4/4 does so only to check if the file exists; it doesn't even care about the file's size, thus neither this function nor this patch seem justified and probably ought to be dropped unless some better and stronger justification can be shown.

Show 20 quoted lines
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/dir.c b/dir.c
> @@ -2036,6 +2036,14 @@ int file_exists(const char *f)
> +ssize_t file_size(const char *filename)
> +{
> +       struct stat st;
> +       if (stat(filename, &st) < 0)
> +               return -1;
> +       return xsize_t(st.st_size);
> +}
> +
> diff --git a/dir.h b/dir.h
> @@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list *el);
> +/*
> + * Return the size of the file `filename`. It returns -1 if error
> + * occurred, 0 if file is empty and a positive number denoting the size
> + * of the file.
> + */
> +extern ssize_t file_size(const char *);
Pranit Bauva· Jun 8, 2016, 07:57 UTC · re: Eric Sunshine · lore

Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

Hey Eric,
On Wed, Jun 8, 2016 at 1:07 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 12 quoted lines
> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> dir: introduce file_size() to check the size of file
>>
>> At times we require to see if the file is empty and get the size of the
>> file. By using stat we can get the file size without actually having to
>> open the file to check for its contents.
>
> The sole caller of this function in patch 4/4 does so only to check if
> the file exists; it doesn't even care about the file's size, thus
> neither this function nor this patch seem justified and probably ought
> to be dropped unless some better and stronger justification can be
> shown.

Umm, actually there is a subtle difference between file_exists() and file_size(). file_exists() *only* checks whether the file exists or not while file_size() can also be used to check whether the file is empty or not also see the implementation of both of them which shows the difference. In fact it doesn't care at all about the file size. Now there are a lot of instances in shell scripts where there are quite some differences with -f and -s and some places *do care* about this subtle difference. For eg. in bisect_reset() we test whether the file .git/BISECT_START has some contents in it. But I guess I can add some more justification in the commit message. What do you think?

Show 20 quoted lines
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/dir.c b/dir.c
>> @@ -2036,6 +2036,14 @@ int file_exists(const char *f)
>> +ssize_t file_size(const char *filename)
>> +{
>> +       struct stat st;
>> +       if (stat(filename, &st) < 0)
>> +               return -1;
>> +       return xsize_t(st.st_size);
>> +}
>> +
>> diff --git a/dir.h b/dir.h
>> @@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list *el);
>> +/*
>> + * Return the size of the file `filename`. It returns -1 if error
>> + * occurred, 0 if file is empty and a positive number denoting the size
>> + * of the file.
>> + */
>> +extern ssize_t file_size(const char *);

Regards, Pranit Bauva

Eric Sunshine· Jun 8, 2016, 08:13 UTC · re: Pranit Bauva · lore

Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

On Wed, Jun 8, 2016 at 3:57 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 24 quoted lines
> On Wed, Jun 8, 2016 at 1:07 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> dir: introduce file_size() to check the size of file
>>>
>>> At times we require to see if the file is empty and get the size of the
>>> file. By using stat we can get the file size without actually having to
>>> open the file to check for its contents.
>>
>> The sole caller of this function in patch 4/4 does so only to check if
>> the file exists; it doesn't even care about the file's size, thus
>> neither this function nor this patch seem justified and probably ought
>> to be dropped unless some better and stronger justification can be
>> shown.
>
> Umm, actually there is a subtle difference between file_exists() and
> file_size(). file_exists() *only* checks whether the file exists or
> not while file_size() can also be used to check whether the file is
> empty or not also see the implementation of both of them which shows
> the difference. In fact it doesn't care at all about the file size.
> Now there are a lot of instances in shell scripts where there are
> quite some differences with -f and -s and some places *do care* about
> this subtle difference. For eg. in bisect_reset() we test whether the
> file .git/BISECT_START has some contents in it. But I guess I can add
> some more justification in the commit message. What do you think?

See my review of patch 4/4 which points out that C bisect_reset() does *not* presently care about the file size, which is probably a bug since that doesn't match the behavior of the shell code it's replacing (which does care that the file is not empty).

I think this would be clearer if you instead added a function to bisect--helper.c which operates at a semantically higher level than what you have here (and drop this file_size() function). Specifically, add a function which tells you precisely what you want to know: whether the file exists and has non-zero size. For instance, in bisect--helper.c:

    static int file_empty_or_missing(const char *path)
    {
        struct stat st;
        return stat(...) < 0 || st.st_size == 0;
    }

Or, if you have more cases where you want to know that it exists with non-zero size, then name it file_non_empty() and reverse the sense of the return value.

Christian Couder· Jun 8, 2016, 10:03 UTC · re: Eric Sunshine · lore

Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

On Wed, Jun 8, 2016 at 10:13 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 17 quoted lines
>
> I think this would be clearer if you instead added a function to
> bisect--helper.c which operates at a semantically higher level than
> what you have here (and drop this file_size() function). Specifically,
> add a function which tells you precisely what you want to know:
> whether the file exists and has non-zero size. For instance, in
> bisect--helper.c:
>
>     static int file_empty_or_missing(const char *path)
>     {
>         struct stat st;
>         return stat(...) < 0 || st.st_size == 0;
>     }
>
> Or, if you have more cases where you want to know that it exists with
> non-zero size, then name it file_non_empty() and reverse the sense of
> the return value.

After a quick grep it seemed to me that there are a few places were we stat a file just to get its size, so I suggested the file_size() because it could help refactor other parts of the code.

Torsten Bögershausen· Jun 8, 2016, 08:17 UTC · re: Pranit Bauva · lore

Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

On 06/08/2016 09:57 AM, Pranit Bauva wrote:
Show 46 quoted lines
> Hey Eric,
>
> On Wed, Jun 8, 2016 at 1:07 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> dir: introduce file_size() to check the size of file
>>>
>>> At times we require to see if the file is empty and get the size of the
>>> file. By using stat we can get the file size without actually having to
>>> open the file to check for its contents.
>> The sole caller of this function in patch 4/4 does so only to check if
>> the file exists; it doesn't even care about the file's size, thus
>> neither this function nor this patch seem justified and probably ought
>> to be dropped unless some better and stronger justification can be
>> shown.
> Umm, actually there is a subtle difference between file_exists() and
> file_size(). file_exists() *only* checks whether the file exists or
> not while file_size() can also be used to check whether the file is
> empty or not also see the implementation of both of them which shows
> the difference. In fact it doesn't care at all about the file size.
> Now there are a lot of instances in shell scripts where there are
> quite some differences with -f and -s and some places *do care* about
> this subtle difference. For eg. in bisect_reset() we test whether the
> file .git/BISECT_START has some contents in it. But I guess I can add
> some more justification in the commit message. What do you think?
>
>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>> ---
>>> diff --git a/dir.c b/dir.c
>>> @@ -2036,6 +2036,14 @@ int file_exists(const char *f)
>>> +ssize_t file_size(const char *filename)
>>> +{
>>> +       struct stat st;
>>> +       if (stat(filename, &st) < 0)
>>> +               return -1;
>>> +       return xsize_t(st.st_size);
>>> +}
>>> +
>>> diff --git a/dir.h b/dir.h
>>> @@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list *el);
>>> +/*
>>> + * Return the size of the file `filename`. It returns -1 if error
>>> + * occurred, 0 if file is empty and a positive number denoting the size
>>> + * of the file.
>>> + */
>>> +extern ssize_t file_size(const char *);
>
So what I understand, you want something like this:
+ssize_t file_size_not_zero(const char *filename)
+{
+       struct stat st;
+       if (stat(filename, &st) < 0)
+               return -1;
+       return !!st.st_size);
+}
Pranit Bauva· Jun 8, 2016, 13:08 UTC · re: Torsten Bögershausen · lore

Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

Hey Torsten,
On Wed, Jun 8, 2016 at 1:47 PM, Torsten Bögershausen <tboegi@web.de> wrote:
Show 65 quoted lines
> On 06/08/2016 09:57 AM, Pranit Bauva wrote:
>>
>> Hey Eric,
>>
>> On Wed, Jun 8, 2016 at 1:07 PM, Eric Sunshine <sunshine@sunshineco.com>
>> wrote:
>>>
>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com>
>>> wrote:
>>>>
>>>> dir: introduce file_size() to check the size of file
>>>>
>>>> At times we require to see if the file is empty and get the size of the
>>>> file. By using stat we can get the file size without actually having to
>>>> open the file to check for its contents.
>>>
>>> The sole caller of this function in patch 4/4 does so only to check if
>>> the file exists; it doesn't even care about the file's size, thus
>>> neither this function nor this patch seem justified and probably ought
>>> to be dropped unless some better and stronger justification can be
>>> shown.
>>
>> Umm, actually there is a subtle difference between file_exists() and
>> file_size(). file_exists() *only* checks whether the file exists or
>> not while file_size() can also be used to check whether the file is
>> empty or not also see the implementation of both of them which shows
>> the difference. In fact it doesn't care at all about the file size.
>> Now there are a lot of instances in shell scripts where there are
>> quite some differences with -f and -s and some places *do care* about
>> this subtle difference. For eg. in bisect_reset() we test whether the
>> file .git/BISECT_START has some contents in it. But I guess I can add
>> some more justification in the commit message. What do you think?
>>
>>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>>> ---
>>>> diff --git a/dir.c b/dir.c
>>>> @@ -2036,6 +2036,14 @@ int file_exists(const char *f)
>>>> +ssize_t file_size(const char *filename)
>>>> +{
>>>> +       struct stat st;
>>>> +       if (stat(filename, &st) < 0)
>>>> +               return -1;
>>>> +       return xsize_t(st.st_size);
>>>> +}
>>>> +
>>>> diff --git a/dir.h b/dir.h
>>>> @@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list
>>>> *el);
>>>> +/*
>>>> + * Return the size of the file `filename`. It returns -1 if error
>>>> + * occurred, 0 if file is empty and a positive number denoting the size
>>>> + * of the file.
>>>> + */
>>>> +extern ssize_t file_size(const char *);
>>
>>
> So what I understand, you want something like this:
>
> +ssize_t file_size_not_zero(const char *filename)
> +{
> +       struct stat st;
> +       if (stat(filename, &st) < 0)
> +               return -1;
> +       return !!st.st_size);
> +}

For the purpose of bisect_reset(), Yes. BTW a similar function exist in builtin/am.c with the name is_empty_file(). But as Christian points out file_size() could help to refactor other parts of code.

Regards, Pranit Bauva

Torsten Bögershausen· Jun 12, 2016, 10:44 UTC · re: Pranit Bauva · lore

Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

Show 14 quoted lines
>> So what I understand, you want something like this:
>>
>> +ssize_t file_size_not_zero(const char *filename)
>> +{
>> +       struct stat st;
>> +       if (stat(filename, &st) < 0)
>> +               return -1;
>> +       return !!st.st_size);
>> +}
> 
> For the purpose of bisect_reset(), Yes. BTW a similar function exist
> in builtin/am.c with the name is_empty_file(). But as Christian points
> out file_size() could help to refactor other parts of code.
> 

Please allow one or more late comments: If is_empty_file() does what you need, then it can be moved into wrapper.c and simply be re-used in your code.

If you want to introduce a new function, that can be used for other refactoring, then the whole thing would ideally go into a single commit, or into a single series. That may probably be out of the scope for your current efforts ?

What really makes me concern is the mixture of signed - and unsigned:
ssize_t file_size(const char *filename)
+{
+       struct stat st;
+       if (stat(filename, &st) < 0)
+               return -1;
+       return xsize_t(st.st_size);
+}

To my understanding a file size is either 0, or a positive integer. Returning -1 is of course impossible with a positive integer.

So either the function is changed like this:
int file_size(const char *filename, size_t *len)
+{
+       struct stat st;
+       if (stat(filename, &st) < 0)
+               return -1;
+       *len = xsize_t(st.st_size);
+       return 0;
+}
Or, if that works for you:
size_t file_size(const char *filename)
+{
+       struct stat st;
+       if (stat(filename, &st) < 0)
+               return 0;
+       return xsize_t(st.st_size);
+}
Or, more git-ish:
size_t file_size(const char *filename)
+{
+       struct stat st;
+       if (stat(filename, &st))
+               return 0;
+       return xsize_t(st.st_size);
+}
(And then builtin/am.c  can be changed to use the new function.
 
Pranit Bauva· Jun 13, 2016, 06:21 UTC · re: Torsten Bögershausen · lore

Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

Hey Torsten,
On Sun, Jun 12, 2016 at 4:14 PM, Torsten Bögershausen <tboegi@web.de> wrote:
Show 16 quoted lines
>>> So what I understand, you want something like this:
>>>
>>> +ssize_t file_size_not_zero(const char *filename)
>>> +{
>>> +       struct stat st;
>>> +       if (stat(filename, &st) < 0)
>>> +               return -1;
>>> +       return !!st.st_size);
>>> +}
>>
>> For the purpose of bisect_reset(), Yes. BTW a similar function exist
>> in builtin/am.c with the name is_empty_file(). But as Christian points
>> out file_size() could help to refactor other parts of code.
>>
>
> Please allow one or more late comments:
That's perfectly fine.
> If is_empty_file() does what you need, then it can be moved into wrapper.c
> and simply be re-used in your code.
Thanks for informing. I was unaware about the use of wrapper.c
> If you want to introduce a new function, that can be used for other refactoring,
> then the whole thing would ideally go into a single commit,
> or into a single series.
> That may probably be out of the scope for your current efforts ?

On re-thinking, I think introducing file_size() is out of the scope for the current efforts and I will stick to is_empty_file(). Will move it to wrapper.c and then use it in my code. I am not sure but I think a few other parts could also use is_empty_file(). I will check on that probably after GSoC as a cleanup.

Show 11 quoted lines
> What really makes me concern is the mixture of signed - and unsigned:
> ssize_t file_size(const char *filename)
> +{
> +       struct stat st;
> +       if (stat(filename, &st) < 0)
> +               return -1;
> +       return xsize_t(st.st_size);
> +}
>
> To my understanding a file size is either 0, or a positive integer.
> Returning -1 is of course impossible with a positive integer.
True.
Show 32 quoted lines
> So either the function is changed like this:
>
> int file_size(const char *filename, size_t *len)
> +{
> +       struct stat st;
> +       if (stat(filename, &st) < 0)
> +               return -1;
> +       *len = xsize_t(st.st_size);
> +       return 0;
> +}
>
> Or, if that works for you:
>
> size_t file_size(const char *filename)
> +{
> +       struct stat st;
> +       if (stat(filename, &st) < 0)
> +               return 0;
> +       return xsize_t(st.st_size);
> +}
>
> Or, more git-ish:
>
> size_t file_size(const char *filename)
> +{
> +       struct stat st;
> +       if (stat(filename, &st))
> +               return 0;
> +       return xsize_t(st.st_size);
> +}
>
> (And then builtin/am.c  can be changed to use the new function.
I think I will just skip file_size() for now.
Thanks for your comments!

Regards, Pranit Bauva

Pranit Bauva· Jun 7, 2016, 20:54 UTC · re: Pranit Bauva · lore

[PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

Reimplement `bisect_reset` shell function in C and add a `--bisect-reset` subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `bisect_reset` subcommand is a temporary measure to port shell functions to C so as to use the existing test suite. As more functions are ported, this subcommand would be retired and will be called by some other method.

Note: --bisect-clean-state subcommand has not been retired as there are
still a function namely `bisect_start()` which still uses this
subcommand.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 28 ++------------------------
 2 files changed, 52 insertions(+), 27 deletions(-)
Show changes to 2 files +52 −27

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index ad39181..4153e8a 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,6 +4,8 @@
 #include "bisect.h"
 #include "refs.h"
 #include "dir.h"
+#include "argv-array.h"
+#include "run-command.h"
 
 static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -13,11 +15,13 @@ 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 GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 
 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"),
+	N_("git bisect--helper --bisect-reset [<commit>]"),
 	NULL
 };
 
@@ -118,12 +122,51 @@ int bisect_clean_state(void)
 	return result;
 }
 
+int bisect_reset(const char *commit)
+{
+	struct strbuf branch = STRBUF_INIT;
+	int status = 0;
+
+	if (file_size(git_path_bisect_start()) < 1) {
+		printf("We are not bisecting.\n");
+		return 0;
+	}
+
+	if (!commit) {
+		strbuf_read_file(&branch, git_path_bisect_start(), 0);
+		strbuf_rtrim(&branch);
+	} else {
+		struct object_id oid;
+		if (get_oid(commit, &oid))
+			return error(_("'%s' is not a valid commit"), commit);
+		strbuf_addf(&branch, "%s", commit);
+	}
+
+	if (!file_exists(git_path_bisect_head())) {
+		struct argv_array argv = ARGV_ARRAY_INIT;
+		argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
+		status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
+		argv_array_clear(&argv);
+	}
+
+	if (status) {
+		error(_("Could not check out original HEAD '%s'. "
+				"Try 'git bisect reset <commit>'."), branch.buf);
+		strbuf_release(&branch);
+		return -1;
+	}
+
+	strbuf_release(&branch);
+	return bisect_clean_state();
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
-		BISECT_CLEAN_STATE
+		BISECT_CLEAN_STATE,
+		BISECT_RESET
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -133,6 +176,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 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_CMDMODE(0, "bisect-reset", &cmdmode,
+			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -155,6 +200,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 0)
 			die(_("--bisect-clean-state requires no arguments"));
 		return bisect_clean_state();
+	case BISECT_RESET:
+		if (argc > 1)
+			die(_("--bisect-reset requires either zero or one arguments"));
+		return bisect_reset(argc ? argv[0] : NULL);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index bbc57d2..18580b7 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -409,35 +409,11 @@ bisect_visualize() {
 	eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
 }
 
-bisect_reset() {
-	test -s "$GIT_DIR/BISECT_START" || {
-		gettextln "We are not bisecting."
-		return
-	}
-	case "$#" in
-	0) branch=$(cat "$GIT_DIR/BISECT_START") ;;
-	1) git rev-parse --quiet --verify "$1^{commit}" >/dev/null || {
-			invalid="$1"
-			die "$(eval_gettext "'\$invalid' is not a valid commit")"
-		}
-		branch="$1" ;;
-	*)
-		usage ;;
-	esac
-
-	if ! test -f "$GIT_DIR/BISECT_HEAD" && ! git checkout "$branch" --
-	then
-		die "$(eval_gettext "Could not check out original HEAD '\$branch'.
-Try 'git bisect reset <commit>'.")"
-	fi
-	git bisect--helper --bisect-clean-state || exit
-}
-
 bisect_replay () {
 	file="$1"
 	test "$#" -eq 1 || die "$(gettext "No logfile given")"
 	test -r "$file" || die "$(eval_gettext "cannot read \$file for replaying")"
-	bisect_reset
+	git bisect--helper --bisect-reset || exit
 	while read git bisect command rev
 	do
 		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
@@ -627,7 +603,7 @@ case "$#" in
 	visualize|view)
 		bisect_visualize "$@" ;;
 	reset)
-		bisect_reset "$@" ;;
+		git bisect--helper --bisect-reset "$@" ;;
 	replay)
 		bisect_replay "$@" ;;
 	log)
-- 
2.8.3
Eric Sunshine· Jun 8, 2016, 07:59 UTC · re: Pranit Bauva · lore

Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 17 quoted lines
> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
> subcommand to `git bisect--helper` to call it from git-bisect.sh .
>
> Using `bisect_reset` subcommand is a temporary measure to port shell
> functions to C so as to use the existing test suite. As more functions
> are ported, this subcommand would be retired and will be called by some
> other method.
>
> Note: --bisect-clean-state subcommand has not been retired as there are
> still a function namely `bisect_start()` which still uses this
> subcommand.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -118,12 +122,51 @@ int bisect_clean_state(void)
> +int bisect_reset(const char *commit)
s/^/static/
Show 5 quoted lines
> +{
> +       struct strbuf branch = STRBUF_INIT;
> +       int status = 0;
> +
> +       if (file_size(git_path_bisect_start()) < 1) {

This doesn't even care about the size of the file, only if it encountered an error while stat()'ing it. Why not just use file_exists() instead (which you already use elsewhere in this function)? Alternately, if you're trying to be faithful to the shell code, then you *do* need to check that the file has non-zero size before issuing the "not bisecting" diagnostic, so:

    if (file_size(git_path_bisect_start()) <= 0)
        printf("... not bisecting ...");

A different approach would be to invoke strbuf_read_file() unconditionally, rather than performing this separate check. If strbuf_read_file() returns -1, then the file didn't exist or couldn't be read; if it returns 0, then the file has no content:

    if (strbuf_read_file(&branch, ..., 0) <= 0)
        printf("... not bisecting ...");
Show 6 quoted lines
> +               printf("We are not bisecting.\n");
> +               return 0;
> +       }
> +
> +       if (!commit) {
> +               strbuf_read_file(&branch, git_path_bisect_start(), 0);
Shouldn't you be checking the return value of strbuf_read_file()?
Show 16 quoted lines
> +               strbuf_rtrim(&branch);
> +       } else {
> +               struct object_id oid;
> +               if (get_oid(commit, &oid))
> +                       return error(_("'%s' is not a valid commit"), commit);
> +               strbuf_addf(&branch, "%s", commit);
> +       }
> +
> +       if (!file_exists(git_path_bisect_head())) {
> +               struct argv_array argv = ARGV_ARRAY_INIT;
> +               argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
> +               status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
> +               argv_array_clear(&argv);
> +       }
> +
> +       if (status) {

What's the purpose of having this 'status' conditional outside of the above !file_exists() conditional, which is the only place that 'status' gets assigned. Likewise, 'status' itself could be declared within the scope of that conditional block.

Show 9 quoted lines
> +               error(_("Could not check out original HEAD '%s'. "
> +                               "Try 'git bisect reset <commit>'."), branch.buf);
> +               strbuf_release(&branch);
> +               return -1;
> +       }
> +
> +       strbuf_release(&branch);
> +       return bisect_clean_state();
> +}
Christian Couder· Jun 8, 2016, 09:51 UTC · re: Eric Sunshine · lore

Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

On Wed, Jun 8, 2016 at 9:59 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 36 quoted lines
> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
>> subcommand to `git bisect--helper` to call it from git-bisect.sh .
>>
>> Using `bisect_reset` subcommand is a temporary measure to port shell
>> functions to C so as to use the existing test suite. As more functions
>> are ported, this subcommand would be retired and will be called by some
>> other method.
>>
>> Note: --bisect-clean-state subcommand has not been retired as there are
>> still a function namely `bisect_start()` which still uses this
>> subcommand.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -118,12 +122,51 @@ int bisect_clean_state(void)
>> +int bisect_reset(const char *commit)
>
> s/^/static/
>
>> +{
>> +       struct strbuf branch = STRBUF_INIT;
>> +       int status = 0;
>> +
>> +       if (file_size(git_path_bisect_start()) < 1) {
>
> This doesn't even care about the size of the file, only if it
> encountered an error while stat()'ing it. Why not just use
> file_exists() instead (which you already use elsewhere in this
> function)? Alternately, if you're trying to be faithful to the shell
> code, then you *do* need to check that the file has non-zero size
> before issuing the "not bisecting" diagnostic, so:
>
>     if ()
>         printf("... not bisecting ...");

As file_size() returns an integer, there is no difference between "file_size(git_path_bisect_start()) <= 0" and "file_size(git_path_bisect_start()) < 1". Or am I missing something?

Christian Couder· Jun 8, 2016, 09:53 UTC · re: Christian Couder · lore

Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

On Wed, Jun 8, 2016 at 11:51 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 36 quoted lines
> On Wed, Jun 8, 2016 at 9:59 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
>>> subcommand to `git bisect--helper` to call it from git-bisect.sh .
>>>
>>> Using `bisect_reset` subcommand is a temporary measure to port shell
>>> functions to C so as to use the existing test suite. As more functions
>>> are ported, this subcommand would be retired and will be called by some
>>> other method.
>>>
>>> Note: --bisect-clean-state subcommand has not been retired as there are
>>> still a function namely `bisect_start()` which still uses this
>>> subcommand.
>>>
>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>> ---
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> @@ -118,12 +122,51 @@ int bisect_clean_state(void)
>>> +int bisect_reset(const char *commit)
>>
>> s/^/static/
>>
>>> +{
>>> +       struct strbuf branch = STRBUF_INIT;
>>> +       int status = 0;
>>> +
>>> +       if (file_size(git_path_bisect_start()) < 1) {
>>
>> This doesn't even care about the size of the file, only if it
>> encountered an error while stat()'ing it. Why not just use
>> file_exists() instead (which you already use elsewhere in this
>> function)? Alternately, if you're trying to be faithful to the shell
>> code, then you *do* need to check that the file has non-zero size
>> before issuing the "not bisecting" diagnostic, so:
>>
>>     if ()
Ooops this was:
>>     if (file_size(git_path_bisect_start()) <= 0)
but I made a copy paste mistake, sorry.
Show 6 quoted lines
>>         printf("... not bisecting ...");
>
> As file_size() returns an integer, there is no difference between
> "file_size(git_path_bisect_start()) <= 0" and
> "file_size(git_path_bisect_start()) < 1".
> Or am I missing something?
Eric Sunshine· Jun 8, 2016, 17:50 UTC · re: Christian Couder · lore

Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

On Wed, Jun 8, 2016 at 5:51 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 17 quoted lines
> On Wed, Jun 8, 2016 at 9:59 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +       if (file_size(git_path_bisect_start()) < 1) {
>>
>> This doesn't even care about the size of the file, only if it
>> encountered an error while stat()'ing it. Why not just use
>> file_exists() instead (which you already use elsewhere in this
>> function)? Alternately, if you're trying to be faithful to the shell
>> code, then you *do* need to check that the file has non-zero size
>> before issuing the "not bisecting" diagnostic, so:
>>
>>     if ()
>>         printf("... not bisecting ...");
>
> As file_size() returns an integer, there is no difference between
> "file_size(git_path_bisect_start()) <= 0" and
> "file_size(git_path_bisect_start()) < 1".
> Or am I missing something?
No, you're right. I misread the code as:
    file_size(...) < 0
rather than what it really says:
    file_size(...) < 1
Sorry for the noise.

That it was so easy to misread the code, however, may be a good argument for making a more special-purpose function, such as file_empty_or_missing() (or file_not_empty()) as suggested in the patch 3/4 thread.

Pranit Bauva· Jun 8, 2016, 13:20 UTC · re: Eric Sunshine · lore

Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

Hey Eric,
On Wed, Jun 8, 2016 at 1:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 36 quoted lines
> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
>> subcommand to `git bisect--helper` to call it from git-bisect.sh .
>>
>> Using `bisect_reset` subcommand is a temporary measure to port shell
>> functions to C so as to use the existing test suite. As more functions
>> are ported, this subcommand would be retired and will be called by some
>> other method.
>>
>> Note: --bisect-clean-state subcommand has not been retired as there are
>> still a function namely `bisect_start()` which still uses this
>> subcommand.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -118,12 +122,51 @@ int bisect_clean_state(void)
>> +int bisect_reset(const char *commit)
>
> s/^/static/
>
>> +{
>> +       struct strbuf branch = STRBUF_INIT;
>> +       int status = 0;
>> +
>> +       if (file_size(git_path_bisect_start()) < 1) {
>
> This doesn't even care about the size of the file, only if it
> encountered an error while stat()'ing it. Why not just use
> file_exists() instead (which you already use elsewhere in this
> function)? Alternately, if you're trying to be faithful to the shell
> code, then you *do* need to check that the file has non-zero size
> before issuing the "not bisecting" diagnostic, so:
>
>     if (file_size(git_path_bisect_start()) <= 0)
>         printf("... not bisecting ...");
Umm, I think that for all x belonging to integers,
            x <= 0    <=>      x < 1
> A different approach would be to invoke strbuf_read_file()
> unconditionally, rather than performing this separate check. If
> strbuf_read_file() returns -1, then the file didn't exist or couldn't
> be read; if it returns 0, then the file has no content:

strbuf_read_file() opens the file and then reads its contents. Well this much of computation isn't really required. By using stat we can get the file size without actually reading the file. Are there any benefits which I have missed of using strbuf_read_file() over stat() ?

Show 11 quoted lines
>     if (strbuf_read_file(&branch, ..., 0) <= 0)
>         printf("... not bisecting ...");
>
>> +               printf("We are not bisecting.\n");
>> +               return 0;
>> +       }
>> +
>> +       if (!commit) {
>> +               strbuf_read_file(&branch, git_path_bisect_start(), 0);
>
> Shouldn't you be checking the return value of strbuf_read_file()?

The shell script didn't report any error but I guess this doesn't have to continue. Its probably better to add error handling code while rewriting.

Show 21 quoted lines
>> +               strbuf_rtrim(&branch);
>> +       } else {
>> +               struct object_id oid;
>> +               if (get_oid(commit, &oid))
>> +                       return error(_("'%s' is not a valid commit"), commit);
>> +               strbuf_addf(&branch, "%s", commit);
>> +       }
>> +
>> +       if (!file_exists(git_path_bisect_head())) {
>> +               struct argv_array argv = ARGV_ARRAY_INIT;
>> +               argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
>> +               status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
>> +               argv_array_clear(&argv);
>> +       }
>> +
>> +       if (status) {
>
> What's the purpose of having this 'status' conditional outside of the
> above !file_exists() conditional, which is the only place that
> 'status' gets assigned. Likewise, 'status' itself could be declared
> within the scope of that conditional block.

I wanted to avoid nesting. In a few other parts of the code also, nesting is avoided as much as possible.

Show 9 quoted lines
>> +               error(_("Could not check out original HEAD '%s'. "
>> +                               "Try 'git bisect reset <commit>'."), branch.buf);
>> +               strbuf_release(&branch);
>> +               return -1;
>> +       }
>> +
>> +       strbuf_release(&branch);
>> +       return bisect_clean_state();
>> +}

Regards, Pranit Bauva

Eric Sunshine· Jun 8, 2016, 17:53 UTC · re: Pranit Bauva · lore

Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

On Wed, Jun 8, 2016 at 9:20 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 18 quoted lines
> On Wed, Jun 8, 2016 at 1:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> +       if (!file_exists(git_path_bisect_head())) {
>>> +               struct argv_array argv = ARGV_ARRAY_INIT;
>>> +               argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
>>> +               status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
>>> +               argv_array_clear(&argv);
>>> +       }
>>> +
>>> +       if (status) {
>>
>> What's the purpose of having this 'status' conditional outside of the
>> above !file_exists() conditional, which is the only place that
>> 'status' gets assigned. Likewise, 'status' itself could be declared
>> within the scope of that conditional block.
>
> I wanted to avoid nesting. In a few other parts of the code also,
> nesting is avoided as much as possible.

I figured as much, but I'm not sure that that is such a good idea in this case since it makes the code more difficult to reason about. To wit: as a reader of the code, I spent extra time trying to figure why it was structured this way and if 'status' is assigned or accessed in some other non-obvious way.

Pranit Bauva· Jun 8, 2016, 18:04 UTC · re: Eric Sunshine · lore

Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

Hey Eric,
On Wed, Jun 8, 2016 at 11:23 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 25 quoted lines
> On Wed, Jun 8, 2016 at 9:20 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, Jun 8, 2016 at 1:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> +       if (!file_exists(git_path_bisect_head())) {
>>>> +               struct argv_array argv = ARGV_ARRAY_INIT;
>>>> +               argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
>>>> +               status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
>>>> +               argv_array_clear(&argv);
>>>> +       }
>>>> +
>>>> +       if (status) {
>>>
>>> What's the purpose of having this 'status' conditional outside of the
>>> above !file_exists() conditional, which is the only place that
>>> 'status' gets assigned. Likewise, 'status' itself could be declared
>>> within the scope of that conditional block.
>>
>> I wanted to avoid nesting. In a few other parts of the code also,
>> nesting is avoided as much as possible.
>
> I figured as much, but I'm not sure that that is such a good idea in
> this case since it makes the code more difficult to reason about. To
> wit: as a reader of the code, I spent extra time trying to figure why
> it was structured this way and if 'status' is assigned or accessed in
> some other non-obvious way.
I can put status inside the if() .

Regards, Pranit Bauva

Eric Sunshine· Jun 7, 2016, 22:31 UTC · re: Pranit Bauva · lore

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

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
>  }
Eric Sunshine· Jun 8, 2016, 01:51 UTC · re: Eric Sunshine · lore

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

On Tue, Jun 7, 2016 at 6:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> +int bisect_clean_state(void)
>> +{
I forgot to mention that this and other functions should be 'static'.
>> +       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);
Pranit Bauva· Jun 8, 2016, 07:46 UTC · re: Eric Sunshine · lore

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

Hey Eric,
On Wed, Jun 8, 2016 at 4:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 30 quoted lines
> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> 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 <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...
>
>> +       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?

Yes nice catch. I would prefer using the string_list with STRING_LIST_INIT_DUP and free the ref.

Show 5 quoted lines
>> +       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.
In some initial iterations I assigned value of result before so used
|=. Then I realized it isn't necessary, so removed it. Forgot to
remove |=. Will do it!
Show 18 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).

The commit(4796e823a) by Jon Seymour (4 Aug, 2011) explains this. I am not sure how I would frame the sentence as that commit introduces a concept whose by-product is the comment. I do want to include it in the commit message otherwise it would be passed on as "you have to do it but no one knows why".

Show 10 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?

git-bisect itself never packs refs on its own. But there is a possibility that the user might have packed the refs in the bisection state. This was introduced by Christian ( 15 Nov, 2007) in commit 947a604b01.

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
>>  }
I will also include static in function declaration. Thanks!

Regards, Pranit Bauva

Eric Sunshine· Jun 8, 2016, 08:02 UTC · re: Pranit Bauva · lore

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

On Wed, Jun 8, 2016 at 3:46 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 23 quoted lines
> On Wed, Jun 8, 2016 at 4:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> +       struct string_list *refs = cb_data;
>>> +       char *ref = xstrfmt("refs/bisect/%s", refname);
>>
>> Here you're allocating a string...
>>
>>> +       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?
>
> Yes nice catch. I would prefer using the string_list with
> STRING_LIST_INIT_DUP and free the ref.
That's unnecessarily wasteful. Better would be to to use STRING_LIST_INIT_NODUP.
Pranit Bauva· Jun 8, 2016, 08:09 UTC · re: Eric Sunshine · lore

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

Hey Eric,
On Wed, Jun 8, 2016 at 1:32 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 26 quoted lines
> On Wed, Jun 8, 2016 at 3:46 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, Jun 8, 2016 at 4:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> +       struct string_list *refs = cb_data;
>>>> +       char *ref = xstrfmt("refs/bisect/%s", refname);
>>>
>>> Here you're allocating a string...
>>>
>>>> +       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?
>>
>> Yes nice catch. I would prefer using the string_list with
>> STRING_LIST_INIT_DUP and free the ref.
>
> That's unnecessarily wasteful. Better would be to to use STRING_LIST_INIT_NODUP.
Sure I will try it out!

Regards, Pranit Bauva

Christian Couder· Jun 8, 2016, 09:41 UTC · re: Eric Sunshine · lore

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

On Wed, Jun 8, 2016 at 10:02 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 26 quoted lines
> On Wed, Jun 8, 2016 at 3:46 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, Jun 8, 2016 at 4:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> +       struct string_list *refs = cb_data;
>>>> +       char *ref = xstrfmt("refs/bisect/%s", refname);
>>>
>>> Here you're allocating a string...
>>>
>>>> +       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?
>>
>> Yes nice catch. I would prefer using the string_list with
>> STRING_LIST_INIT_DUP and free the ref.
>
> That's unnecessarily wasteful. Better would be to to use STRING_LIST_INIT_NODUP.

In this case it is not possible to append "BISECT_HEAD" to 'refs_for_removal' before calling delete_refs() like this:

+       for_each_ref_in("refs/bisect/", mark_for_removal, (void *)
&refs_for_removal);
+       string_list_append(&refs_for_removal, "BISECT_HEAD");
+       result = delete_refs(&refs_for_removal);
+       string_list_clear(&refs_for_removal, 0);

And I think it's better to delete all the refs in the same call to delete_refs().

Eric Sunshine· Jun 8, 2016, 17:59 UTC · re: Christian Couder · lore

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

On Wed, Jun 8, 2016 at 5:41 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 39 quoted lines
> On Wed, Jun 8, 2016 at 10:02 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jun 8, 2016 at 3:46 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> On Wed, Jun 8, 2016 at 4:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>>> +       struct string_list *refs = cb_data;
>>>>> +       char *ref = xstrfmt("refs/bisect/%s", refname);
>>>>
>>>> Here you're allocating a string...
>>>>
>>>>> +       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?
>>>
>>> Yes nice catch. I would prefer using the string_list with
>>> STRING_LIST_INIT_DUP and free the ref.
>>
>> That's unnecessarily wasteful. Better would be to to use STRING_LIST_INIT_NODUP.
>
> In this case it is not possible to append "BISECT_HEAD" to
> 'refs_for_removal' before calling delete_refs() like this:
>
> +       for_each_ref_in("refs/bisect/", mark_for_removal, (void *)
> &refs_for_removal);
> +       string_list_append(&refs_for_removal, "BISECT_HEAD");
> +       result = delete_refs(&refs_for_removal);
> +       string_list_clear(&refs_for_removal, 0);
>
> And I think it's better to delete all the refs in the same call to
> delete_refs().
string_list_append(&..., xstrdup("BISECT_HEAD"));
perhaps?
Pranit Bauva· Jun 8, 2016, 18:04 UTC · re: Eric Sunshine · lore

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

Hey Eric,
On Wed, Jun 8, 2016 at 11:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 45 quoted lines
> On Wed, Jun 8, 2016 at 5:41 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Wed, Jun 8, 2016 at 10:02 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, Jun 8, 2016 at 3:46 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> On Wed, Jun 8, 2016 at 4:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>>>> +       struct string_list *refs = cb_data;
>>>>>> +       char *ref = xstrfmt("refs/bisect/%s", refname);
>>>>>
>>>>> Here you're allocating a string...
>>>>>
>>>>>> +       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?
>>>>
>>>> Yes nice catch. I would prefer using the string_list with
>>>> STRING_LIST_INIT_DUP and free the ref.
>>>
>>> That's unnecessarily wasteful. Better would be to to use STRING_LIST_INIT_NODUP.
>>
>> In this case it is not possible to append "BISECT_HEAD" to
>> 'refs_for_removal' before calling delete_refs() like this:
>>
>> +       for_each_ref_in("refs/bisect/", mark_for_removal, (void *)
>> &refs_for_removal);
>> +       string_list_append(&refs_for_removal, "BISECT_HEAD");
>> +       result = delete_refs(&refs_for_removal);
>> +       string_list_clear(&refs_for_removal, 0);
>>
>> And I think it's better to delete all the refs in the same call to
>> delete_refs().
>
> string_list_append(&..., xstrdup("BISECT_HEAD"));
>
> perhaps?
Seems reasonable!

Regards, Pranit Bauva

Pranit Bauva· Jun 15, 2016, 14:00 UTC · re: Pranit Bauva · lore

[PATCH v2 0/6] convert various shell functions in git-bisect to C

A previous version is available here[1].
Changes wrt previous version:
 * Use STRING_LIST_INIT_NODUP to avoid leaks in bisect_clean_state()
 * Use test_path_is_missing in the patch 2/6
 * drop file_size()
 * move is_empty_file() method from builtin/am.c to wrapper.c
 * use static for methods
 * remove the variable status in bisect_reset() altogether and put the whole
   thing inside the if block.
 * one more method converted namely bisect_write().

An important thing to be discussed: In shell script while reading/writing files, no error is reported but we can actually report it in C. In this series I have tried to output error wherever possible. What are your views?

[1]: http://thread.gmane.org/gmane.comp.version-control.git/296717
Pranit Bauva (6):
  bisect--helper: `bisect_clean_state` shell function in C
  t6030: explicitly test for bisection cleanup
  wrapper: move is_empty_file() from builtin/am.c
  bisect--helper: `bisect_reset` shell function in C
  bisect--helper: `is_expected_rev` & `check_expected_revs` shell
    function in C
  bisect--helper: `bisect_write` shell function in C
 builtin/am.c                |  16 ----
 builtin/bisect--helper.c    | 194 +++++++++++++++++++++++++++++++++++++++++++-
 cache.h                     |   3 +
 git-bisect.sh               |  97 +++-------------------
 t/t6030-bisect-porcelain.sh |  17 ++++
 wrapper.c                   |  13 +++
 6 files changed, 236 insertions(+), 104 deletions(-)
-- 
2.9.0
Pranit Bauva· Jun 15, 2016, 14:00 UTC · re: Pranit Bauva · lore

[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 <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 26 +++--------------------
 2 files changed, 56 insertions(+), 24 deletions(-)
Show changes to 2 files +56 −24

builtin/bisect--helper.c, git-bisect.sh

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 <bad_term> <good_term>"),
+	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 @@ 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.9.0
Eric Sunshine· Jun 15, 2016, 18:04 UTC · re: Pranit Bauva · lore

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 <pranit.bauva@gmail.com> wrote:
Show 26 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
> +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().

Show 16 quoted lines
> +       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;
> +}
Pranit Bauva· Jun 15, 2016, 18:47 UTC · re: Eric Sunshine · lore

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 <sunshine@sunshineco.com> wrote:
Show 32 quoted lines
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> 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 <pranit.bauva@gmail.com>
>> ---
>> 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

Eric Sunshine· Jun 15, 2016, 20:22 UTC · re: Pranit Bauva · lore

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 <pranit.bauva@gmail.com> wrote:
Show 38 quoted lines
> On Wed, Jun 15, 2016 at 11:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> 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 <pranit.bauva@gmail.com>
>>> ---
>>> 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.

Show 5 quoted lines
> 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.
Pranit Bauva· Jun 15, 2016, 14:00 UTC · re: Pranit Bauva · lore

[PATCH v2 2/6] t6030: explicitly test for bisection cleanup

This is not an improvement in the test coverage but it helps in making it explicit as to what exactly would be the error as other tests are focussed on testing other things.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

--- I faced this problem while converting `bisect_clean_state` and the tests where showing breakages but it wasn't clear as to where exactly are they breaking. This will patch will help in that. Also I tested the test coverage of the test suite before this patch and it covers this (I did this by purposely changing names of files in git-bisect.sh and running the test suite).

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Show changes to t/t6030-bisect-porcelain.sh +17 −0
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index e74662b..a17f7a6 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git bisect reset cleans bisection state properly' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect reset &&
+	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
+	test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
+	test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
+	test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
+	test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
+	test_path_is_missing "$GIT_DIR/head-name" &&
+	test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
+	test_path_is_missing "$GIT_DIR/BISECT_START"
+'
+
 test_done
-- 
2.9.0
Pranit Bauva· Jun 15, 2016, 14:00 UTC · re: Pranit Bauva · lore

[PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

is_empty_file() can help to refactor a lot of code. Also it is quite helpful while converting shell scripts which use `test -s`. Since is_empty_file() is now a "library" function, its inappropriate to die() so instead error_errno() is used to convey the message to stderr while the appropriate boolean value is returned.

Suggested-by: Torsten Bögershausen <tboegi@web.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/am.c | 16 ----------------
 cache.h      |  3 +++
 wrapper.c    | 13 +++++++++++++
 3 files changed, 16 insertions(+), 16 deletions(-)
Show changes to 3 files +16 −16

builtin/am.c, cache.h, wrapper.c

diff --git a/builtin/am.c b/builtin/am.c
index 3dfe70b..84f21d0 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -30,22 +30,6 @@
 #include "mailinfo.h"
 
 /**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-	struct stat st;
-
-	if (stat(filename, &st) < 0) {
-		if (errno == ENOENT)
-			return 1;
-		die_errno(_("could not stat %s"), filename);
-	}
-
-	return !st.st_size;
-}
-
-/**
  * Returns the length of the first line of msg.
  */
 static int linelen(const char *msg)
diff --git a/cache.h b/cache.h
index 6049f86..8eaad70 100644
--- a/cache.h
+++ b/cache.h
@@ -1870,4 +1870,7 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+extern int is_empty_file(const char *filename);
+
 #endif /* CACHE_H */
diff --git a/wrapper.c b/wrapper.c
index 5dc4e15..36a3eeb 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -696,3 +696,16 @@ void sleep_millisec(int millisec)
 {
 	poll(NULL, 0, millisec);
 }
+
+int is_empty_file(const char *filename)
+{
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		if (errno == ENOENT)
+			return 1;
+		error_errno(_("could not stat %s"), filename);
+	}
+
+	return !st.st_size;
+}
-- 
2.9.0
Junio C Hamano· Jun 15, 2016, 18:12 UTC · re: Pranit Bauva · lore

Re: [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 23 quoted lines
> diff --git a/builtin/am.c b/builtin/am.c
> index 3dfe70b..84f21d0 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -30,22 +30,6 @@
>  #include "mailinfo.h"
>  
>  /**
> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
> - */
> -static int is_empty_file(const char *filename)
> -{
> -	struct stat st;
> -
> -	if (stat(filename, &st) < 0) {
> -		if (errno == ENOENT)
> -			return 1;
> -		die_errno(_("could not stat %s"), filename);
> -	}
> -
> -	return !st.st_size;
> -}
> -

This is perfectly fine in the context of "git am", but as a public function that is called is_empty_file(), callers can come from two camps. One is like the caller of this function in "am" where an empty and a missing file are treated equivalently. The other would want to act differently.

Renaming it "is-empty-or-missing" is necessary in order to make it clear that this helper function is not targetted for the latter callers.

Pranit Bauva· Jun 15, 2016, 18:15 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

Hey Junio,
On Wed, Jun 15, 2016 at 11:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 35 quoted lines
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 3dfe70b..84f21d0 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -30,22 +30,6 @@
>>  #include "mailinfo.h"
>>
>>  /**
>> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
>> - */
>> -static int is_empty_file(const char *filename)
>> -{
>> -     struct stat st;
>> -
>> -     if (stat(filename, &st) < 0) {
>> -             if (errno == ENOENT)
>> -                     return 1;
>> -             die_errno(_("could not stat %s"), filename);
>> -     }
>> -
>> -     return !st.st_size;
>> -}
>> -
>
> This is perfectly fine in the context of "git am", but as a public
> function that is called is_empty_file(), callers can come from two
> camps.  One is like the caller of this function in "am" where an
> empty and a missing file are treated equivalently.  The other would
> want to act differently.
>
> Renaming it "is-empty-or-missing" is necessary in order to make it
> clear that this helper function is not targetted for the latter
> callers.
Yes, its better to rename the function as is_empty_or_missing() . Thanks!

Regards, Pranit Bauva

Eric Sunshine· Jun 15, 2016, 18:22 UTC · re: Pranit Bauva · lore

Re: [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> is_empty_file() can help to refactor a lot of code. Also it is quite
> helpful while converting shell scripts which use `test -s`. Since

As justification, "can help to refactor a lot of code" is very nebulous. It would be better to give a concrete reason for moving the function, such as explaining that the functionality will be needed by the "git bisect" port to C.

Show 23 quoted lines
> is_empty_file() is now a "library" function, its inappropriate to die() so
> instead error_errno() is used to convey the message to stderr while the
> appropriate boolean value is returned.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/am.c b/builtin/am.c
> @@ -30,22 +30,6 @@
>  /**
> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
> - */
> -static int is_empty_file(const char *filename)
> -{
> -       struct stat st;
> -
> -       if (stat(filename, &st) < 0) {
> -               if (errno == ENOENT)
> -                       return 1;
> -               die_errno(_("could not stat %s"), filename);
> -       }
> -
> -       return !st.st_size;
> -}

So, the original function die()'d for unexpected errors, but the rewrite does not. This is a big behavior change. To account for such a change in behavior I'd expect to see git-am updated to die() on its own for such failures, but no such changes are present in this patch. More about this below...

Show 10 quoted lines
> diff --git a/wrapper.c b/wrapper.c
> @@ -696,3 +696,16 @@ void sleep_millisec(int millisec)
> +int is_empty_file(const char *filename)
> +{
> +       struct stat st;
> +
> +       if (stat(filename, &st) < 0) {
> +               if (errno == ENOENT)
> +                       return 1;
> +               error_errno(_("could not stat %s"), filename);

Mental note: There is no 'return' in front of error_errno(), so the function does not exit here...

> +       }
> +
> +       return !st.st_size;
> +}

If stat() returns some error other than ENOENT, then the value of 'st' will be undefined, yet this return statement accesses its 'st_size' field, which is clearly a bad thing to do.

You either need to return a designated value (such as -1) upon errors other than ENOENT (and update the documentation to mention -1) so that the caller can decided what to do, or die() as the original did. While it's true that die()'ing is not necessarily friendly in library code, it may be acceptable until such time that you find a caller which needs different behavior.

Pranit Bauva· Jun 15, 2016, 18:40 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

Hey Eric,
On Wed, Jun 15, 2016 at 11:52 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 8 quoted lines
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> is_empty_file() can help to refactor a lot of code. Also it is quite
>> helpful while converting shell scripts which use `test -s`. Since
>
> As justification, "can help to refactor a lot of code" is very
> nebulous. It would be better to give a concrete reason for moving the
> function, such as explaining that the functionality will be needed by
> the "git bisect" port to C.
Sure I can include that.
Show 43 quoted lines
>> is_empty_file() is now a "library" function, its inappropriate to die() so
>> instead error_errno() is used to convey the message to stderr while the
>> appropriate boolean value is returned.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/am.c b/builtin/am.c
>> @@ -30,22 +30,6 @@
>>  /**
>> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
>> - */
>> -static int is_empty_file(const char *filename)
>> -{
>> -       struct stat st;
>> -
>> -       if (stat(filename, &st) < 0) {
>> -               if (errno == ENOENT)
>> -                       return 1;
>> -               die_errno(_("could not stat %s"), filename);
>> -       }
>> -
>> -       return !st.st_size;
>> -}
>
> So, the original function die()'d for unexpected errors, but the
> rewrite does not. This is a big behavior change. To account for such a
> change in behavior I'd expect to see git-am updated to die() on its
> own for such failures, but no such changes are present in this patch.
> More about this below...
>
>> diff --git a/wrapper.c b/wrapper.c
>> @@ -696,3 +696,16 @@ void sleep_millisec(int millisec)
>> +int is_empty_file(const char *filename)
>> +{
>> +       struct stat st;
>> +
>> +       if (stat(filename, &st) < 0) {
>> +               if (errno == ENOENT)
>> +                       return 1;
>> +               error_errno(_("could not stat %s"), filename);
>
> Mental note: There is no 'return' in front of error_errno(), so the
> function does not exit here...

This is purposely as I want to keep this function to return only boolean values ( 0 or 1).

Show 15 quoted lines
>> +       }
>> +
>> +       return !st.st_size;
>> +}
>
> If stat() returns some error other than ENOENT, then the value of 'st'
> will be undefined, yet this return statement accesses its 'st_size'
> field, which is clearly a bad thing to do.
>
> You either need to return a designated value (such as -1) upon errors
> other than ENOENT (and update the documentation to mention -1) so that
> the caller can decided what to do, or die() as the original did. While
> it's true that die()'ing is not necessarily friendly in library code,
> it may be acceptable until such time that you find a caller which
> needs different behavior.

I didn't realize the complexity the patch carried with itself before. I probably shouldn't fidget with am code right now, its a work better left to who are converting the code to library code. I think its the best fit for this situation to leave it as die()'ing.

Regards, Pranit Bauva

Pranit Bauva· Jun 15, 2016, 14:00 UTC · re: Pranit Bauva · lore

[PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

Reimplement `is_expected_rev` & `check_expected_revs` shell function in C and add a `--check-expected-revs` subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `--check-expected-revs` subcommand is a temporary measure to port shell functions to C so as to use the existing test suite. As more functions are ported, this subcommand would be retired and will be called by some other method.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 37 ++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 20 ++------------------
 2 files changed, 38 insertions(+), 19 deletions(-)
Show changes to 2 files +38 −19

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 14043a8..f11c247 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
 	return bisect_clean_state();
 }
 
+static int is_expected_rev(const char *expected_hex)
+{
+	struct strbuf actual_hex = STRBUF_INIT;
+	int res;
+
+	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
+		strbuf_release(&actual_hex);
+		return 0;
+	}
+
+	strbuf_trim(&actual_hex);
+	res = !strcmp(actual_hex.buf, expected_hex);
+	strbuf_release(&actual_hex);
+	return res;
+}
+
+static int check_expected_revs(const char **revs, int rev_nr)
+{
+	int i;
+
+	for (i = 0; i < rev_nr; i++) {
+		if (!is_expected_rev(revs[i])) {
+			remove_path(git_path_bisect_ancestors_ok());
+			remove_path(git_path_bisect_expected_rev());
+			return 0;
+		}
+	}
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
 		BISECT_CLEAN_STATE,
-		BISECT_RESET
+		BISECT_RESET,
+		CHECK_EXPECTED_REVS
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -180,6 +211,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
 			 N_("reset the bisection state"), BISECT_RESET),
+		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
+			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -206,6 +239,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc > 1)
 			die(_("--bisect-reset requires either zero or one arguments"));
 		return bisect_reset(argc ? argv[0] : NULL);
+	case CHECK_EXPECTED_REVS:
+		return check_expected_revs(argv, argc);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 18580b7..4f6545e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -238,22 +238,6 @@ bisect_write() {
 	test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
 }
 
-is_expected_rev() {
-	test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
-	test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
-}
-
-check_expected_revs() {
-	for _rev in "$@"; do
-		if ! is_expected_rev "$_rev"
-		then
-			rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
-			rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
-			return
-		fi
-	done
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -280,7 +264,7 @@ bisect_state() {
 		rev=$(git rev-parse --verify $(bisect_head)) ||
 			die "$(gettext "Bad rev input: $(bisect_head)")"
 		bisect_write "$state" "$rev"
-		check_expected_revs "$rev" ;;
+		git bisect--helper --check-expected-revs "$rev" ;;
 	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
 		shift
 		hash_list=''
@@ -294,7 +278,7 @@ bisect_state() {
 		do
 			bisect_write "$state" "$rev"
 		done
-		check_expected_revs $hash_list ;;
+		git bisect--helper --check-expected-revs $hash_list ;;
 	*,"$TERM_BAD")
 		die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
 	*)
-- 
2.9.0
Eric Sunshine· Jun 15, 2016, 21:14 UTC · re: Pranit Bauva · lore

Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 23 quoted lines
> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
> C and add a `--check-expected-revs` 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
> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
> +static int is_expected_rev(const char *expected_hex)
> +{
> +       struct strbuf actual_hex = STRBUF_INIT;
> +       int res;
> +
> +       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
> +               strbuf_release(&actual_hex);
> +               return 0;
> +       }
> +
> +       strbuf_trim(&actual_hex);
> +       res = !strcmp(actual_hex.buf, expected_hex);
> +       strbuf_release(&actual_hex);
> +       return res;
> +}

Not worth a re-roll, but this could be re-structured to avoid having to remember to release the strbuf at all exits:

    struct strbuf actual_hex = ...;
    int res = 0;
    if (strbuf_read_file(...) >= 0) {
        strbuf_trim(...);
        res = !strcmp(...);
    }
    strbuf_release(...);
    return res;
Alternately:
    if (strbuf_read_file(...) < 0)
        goto done;
    strbuf_trim(...);
    res = !strcmp(...);
done:
    strbuf_release(...);
    return res;
which is a bit less compact.
Show 13 quoted lines
> +static int check_expected_revs(const char **revs, int rev_nr)
> +{
> +       int i;
> +
> +       for (i = 0; i < rev_nr; i++) {
> +               if (!is_expected_rev(revs[i])) {
> +                       remove_path(git_path_bisect_ancestors_ok());
> +                       remove_path(git_path_bisect_expected_rev());
> +                       return 0;
> +               }
> +       }
> +       return 0;
> +}

Hmm, all execution paths return 0, so it feels a bit pointless to have this function return a value at all.

You could also use a 'break' inside the loop rather than 'return' since the return value is the same inside or outside the loop and nothing else happens after the loop.

Pranit Bauva· Jun 16, 2016, 19:05 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

Hey Eric,
On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 51 quoted lines
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>> C and add a `--check-expected-revs` 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
>> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
>> +static int is_expected_rev(const char *expected_hex)
>> +{
>> +       struct strbuf actual_hex = STRBUF_INIT;
>> +       int res;
>> +
>> +       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
>> +               strbuf_release(&actual_hex);
>> +               return 0;
>> +       }
>> +
>> +       strbuf_trim(&actual_hex);
>> +       res = !strcmp(actual_hex.buf, expected_hex);
>> +       strbuf_release(&actual_hex);
>> +       return res;
>> +}
>
> Not worth a re-roll, but this could be re-structured to avoid having
> to remember to release the strbuf at all exits:
>
>     struct strbuf actual_hex = ...;
>     int res = 0;
>
>     if (strbuf_read_file(...) >= 0) {
>         strbuf_trim(...);
>         res = !strcmp(...);
>     }
>     strbuf_release(...);
>     return res;
>
> Alternately:
>
>     if (strbuf_read_file(...) < 0)
>         goto done;
>
>     strbuf_trim(...);
>     res = !strcmp(...);
>
> done:
>     strbuf_release(...);
>     return res;
>
> which is a bit less compact.

I will avoid this for the reason that I will have to create a label for a lot of functions. If I choose to do this for one function, I think it would be more appropriate to do the same for other functions. There would be a lot of functions in future which would be in the same scenario and creating a separate label for each of them would be quite tedious. What do you think?

Show 20 quoted lines
>> +static int check_expected_revs(const char **revs, int rev_nr)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < rev_nr; i++) {
>> +               if (!is_expected_rev(revs[i])) {
>> +                       remove_path(git_path_bisect_ancestors_ok());
>> +                       remove_path(git_path_bisect_expected_rev());
>> +                       return 0;
>> +               }
>> +       }
>> +       return 0;
>> +}
>
> Hmm, all execution paths return 0, so it feels a bit pointless to have
> this function return a value at all.
>
> You could also use a 'break' inside the loop rather than 'return'
> since the return value is the same inside or outside the loop and
> nothing else happens after the loop.
Sure!
Eric Sunshine· Jun 16, 2016, 19:16 UTC · re: Pranit Bauva · lore

Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 59 quoted lines
> On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>>> C and add a `--check-expected-revs` 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
>>> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
>>> +static int is_expected_rev(const char *expected_hex)
>>> +{
>>> +       struct strbuf actual_hex = STRBUF_INIT;
>>> +       int res;
>>> +
>>> +       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
>>> +               strbuf_release(&actual_hex);
>>> +               return 0;
>>> +       }
>>> +
>>> +       strbuf_trim(&actual_hex);
>>> +       res = !strcmp(actual_hex.buf, expected_hex);
>>> +       strbuf_release(&actual_hex);
>>> +       return res;
>>> +}
>>
>> Not worth a re-roll, but this could be re-structured to avoid having
>> to remember to release the strbuf at all exits:
>>
>>     struct strbuf actual_hex = ...;
>>     int res = 0;
>>
>>     if (strbuf_read_file(...) >= 0) {
>>         strbuf_trim(...);
>>         res = !strcmp(...);
>>     }
>>     strbuf_release(...);
>>     return res;
>>
>> Alternately:
>>
>>     if (strbuf_read_file(...) < 0)
>>         goto done;
>>
>>     strbuf_trim(...);
>>     res = !strcmp(...);
>>
>> done:
>>     strbuf_release(...);
>>     return res;
>>
>> which is a bit less compact.
>
> I will avoid this for the reason that I will have to create a label
> for a lot of functions. If I choose to do this for one function, I
> think it would be more appropriate to do the same for other functions.
> There would be a lot of functions in future which would be in the same
> scenario and creating a separate label for each of them would be quite
> tedious. What do you think?

Not sure what you're talking about. Label names are not shared across functions. Anyhow, the first suggestion I presented above is more concise than the 'goto' version.

Pranit Bauva· Jun 16, 2016, 19:25 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

Hey Eric,
On Fri, Jun 17, 2016 at 12:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 64 quoted lines
> On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>>>> C and add a `--check-expected-revs` 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
>>>> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
>>>> +static int is_expected_rev(const char *expected_hex)
>>>> +{
>>>> +       struct strbuf actual_hex = STRBUF_INIT;
>>>> +       int res;
>>>> +
>>>> +       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
>>>> +               strbuf_release(&actual_hex);
>>>> +               return 0;
>>>> +       }
>>>> +
>>>> +       strbuf_trim(&actual_hex);
>>>> +       res = !strcmp(actual_hex.buf, expected_hex);
>>>> +       strbuf_release(&actual_hex);
>>>> +       return res;
>>>> +}
>>>
>>> Not worth a re-roll, but this could be re-structured to avoid having
>>> to remember to release the strbuf at all exits:
>>>
>>>     struct strbuf actual_hex = ...;
>>>     int res = 0;
>>>
>>>     if (strbuf_read_file(...) >= 0) {
>>>         strbuf_trim(...);
>>>         res = !strcmp(...);
>>>     }
>>>     strbuf_release(...);
>>>     return res;
>>>
>>> Alternately:
>>>
>>>     if (strbuf_read_file(...) < 0)
>>>         goto done;
>>>
>>>     strbuf_trim(...);
>>>     res = !strcmp(...);
>>>
>>> done:
>>>     strbuf_release(...);
>>>     return res;
>>>
>>> which is a bit less compact.
>>
>> I will avoid this for the reason that I will have to create a label
>> for a lot of functions. If I choose to do this for one function, I
>> think it would be more appropriate to do the same for other functions.
>> There would be a lot of functions in future which would be in the same
>> scenario and creating a separate label for each of them would be quite
>> tedious. What do you think?
>
> Not sure what you're talking about. Label names are not shared across
> functions. Anyhow, the first suggestion I presented above is more
> concise than the 'goto' version.

Yes I am aware of the fact that labels aren't shared across functions. What I meant by "separate label" was that I will have to make a label "fail" in each function. But I recently noticed that its used quite a lot so I think it would be okay to use it. Will re-roll with using labels and goto.

Regards, Pranit Bauva

Christian Couder· Jun 16, 2016, 20:47 UTC · re: Pranit Bauva · lore

Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

On Thu, Jun 16, 2016 at 9:25 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 73 quoted lines
> Hey Eric,
>
> On Fri, Jun 17, 2016 at 12:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>>>>> C and add a `--check-expected-revs` 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
>>>>> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
>>>>> +static int is_expected_rev(const char *expected_hex)
>>>>> +{
>>>>> +       struct strbuf actual_hex = STRBUF_INIT;
>>>>> +       int res;
>>>>> +
>>>>> +       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
>>>>> +               strbuf_release(&actual_hex);
>>>>> +               return 0;
>>>>> +       }
>>>>> +
>>>>> +       strbuf_trim(&actual_hex);
>>>>> +       res = !strcmp(actual_hex.buf, expected_hex);
>>>>> +       strbuf_release(&actual_hex);
>>>>> +       return res;
>>>>> +}
>>>>
>>>> Not worth a re-roll, but this could be re-structured to avoid having
>>>> to remember to release the strbuf at all exits:
>>>>
>>>>     struct strbuf actual_hex = ...;
>>>>     int res = 0;
>>>>
>>>>     if (strbuf_read_file(...) >= 0) {
>>>>         strbuf_trim(...);
>>>>         res = !strcmp(...);
>>>>     }
>>>>     strbuf_release(...);
>>>>     return res;
>>>>
>>>> Alternately:
>>>>
>>>>     if (strbuf_read_file(...) < 0)
>>>>         goto done;
>>>>
>>>>     strbuf_trim(...);
>>>>     res = !strcmp(...);
>>>>
>>>> done:
>>>>     strbuf_release(...);
>>>>     return res;
>>>>
>>>> which is a bit less compact.
>>>
>>> I will avoid this for the reason that I will have to create a label
>>> for a lot of functions. If I choose to do this for one function, I
>>> think it would be more appropriate to do the same for other functions.
>>> There would be a lot of functions in future which would be in the same
>>> scenario and creating a separate label for each of them would be quite
>>> tedious. What do you think?
>>
>> Not sure what you're talking about. Label names are not shared across
>> functions. Anyhow, the first suggestion I presented above is more
>> concise than the 'goto' version.
>
> Yes I am aware of the fact that labels aren't shared across functions.
> What I meant by "separate label" was that I will have to make a label
> "fail" in each function. But I recently noticed that its used quite a
> lot so I think it would be okay to use it. Will re-roll with using
> labels and goto.

My opinion is that if there is a more concise version without labels and gotos, it's better to use it, so I would suggest Eric's first suggestion which is:

Show 9 quoted lines
>     struct strbuf actual_hex = ...;
>     int res = 0;
>
>     if (strbuf_read_file(...) >= 0) {
>         strbuf_trim(...);
>         res = !strcmp(...);
>     }
>     strbuf_release(...);
>     return res;

Thanks, Christian.

Pranit Bauva· Jun 17, 2016, 12:49 UTC · re: Christian Couder · lore

Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

Hey Christian,

On Fri, Jun 17, 2016 at 2:17 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 88 quoted lines
> On Thu, Jun 16, 2016 at 9:25 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Hey Eric,
>>
>> On Fri, Jun 17, 2016 at 12:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>>>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>>>>>> C and add a `--check-expected-revs` 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
>>>>>> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
>>>>>> +static int is_expected_rev(const char *expected_hex)
>>>>>> +{
>>>>>> +       struct strbuf actual_hex = STRBUF_INIT;
>>>>>> +       int res;
>>>>>> +
>>>>>> +       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
>>>>>> +               strbuf_release(&actual_hex);
>>>>>> +               return 0;
>>>>>> +       }
>>>>>> +
>>>>>> +       strbuf_trim(&actual_hex);
>>>>>> +       res = !strcmp(actual_hex.buf, expected_hex);
>>>>>> +       strbuf_release(&actual_hex);
>>>>>> +       return res;
>>>>>> +}
>>>>>
>>>>> Not worth a re-roll, but this could be re-structured to avoid having
>>>>> to remember to release the strbuf at all exits:
>>>>>
>>>>>     struct strbuf actual_hex = ...;
>>>>>     int res = 0;
>>>>>
>>>>>     if (strbuf_read_file(...) >= 0) {
>>>>>         strbuf_trim(...);
>>>>>         res = !strcmp(...);
>>>>>     }
>>>>>     strbuf_release(...);
>>>>>     return res;
>>>>>
>>>>> Alternately:
>>>>>
>>>>>     if (strbuf_read_file(...) < 0)
>>>>>         goto done;
>>>>>
>>>>>     strbuf_trim(...);
>>>>>     res = !strcmp(...);
>>>>>
>>>>> done:
>>>>>     strbuf_release(...);
>>>>>     return res;
>>>>>
>>>>> which is a bit less compact.
>>>>
>>>> I will avoid this for the reason that I will have to create a label
>>>> for a lot of functions. If I choose to do this for one function, I
>>>> think it would be more appropriate to do the same for other functions.
>>>> There would be a lot of functions in future which would be in the same
>>>> scenario and creating a separate label for each of them would be quite
>>>> tedious. What do you think?
>>>
>>> Not sure what you're talking about. Label names are not shared across
>>> functions. Anyhow, the first suggestion I presented above is more
>>> concise than the 'goto' version.
>>
>> Yes I am aware of the fact that labels aren't shared across functions.
>> What I meant by "separate label" was that I will have to make a label
>> "fail" in each function. But I recently noticed that its used quite a
>> lot so I think it would be okay to use it. Will re-roll with using
>> labels and goto.
>
> My opinion is that if there is a more concise version without labels
> and gotos, it's better to use it, so I would suggest Eric's first
> suggestion which is:
>
>>     struct strbuf actual_hex = ...;
>>     int res = 0;
>>
>>     if (strbuf_read_file(...) >= 0) {
>>         strbuf_trim(...);
>>         res = !strcmp(...);
>>     }
>>     strbuf_release(...);
>>     return res;
Sure I could do that!

Regards, Pranit Bauva

Pranit Bauva· Jun 15, 2016, 14:00 UTC · re: Pranit Bauva · lore

[PATCH v2 4/6] bisect--helper: `bisect_reset` shell function in C

Reimplement `bisect_reset` shell function in C and add a `--bisect-reset` subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `bisect_reset` subcommand is a temporary measure to port shell functions to C so as to use the existing test suite. As more functions are ported, this subcommand would be retired and will be called by some other method.

Note: --bisect-clean-state subcommand has not been retired as there are
still a function namely `bisect_start()` which still uses this
subcommand.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 28 ++--------------------------
 2 files changed, 49 insertions(+), 27 deletions(-)
Show changes to 2 files +49 −27

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3e4a458..14043a8 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,6 +4,8 @@
 #include "bisect.h"
 #include "refs.h"
 #include "dir.h"
+#include "argv-array.h"
+#include "run-command.h"
 
 static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -13,11 +15,13 @@ 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 GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 
 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"),
+	N_("git bisect--helper --bisect-reset [<commit>]"),
 	NULL
 };
 
@@ -123,12 +127,48 @@ static int bisect_clean_state(void)
 	return result;
 }
 
+static int bisect_reset(const char *commit)
+{
+	struct strbuf branch = STRBUF_INIT;
+
+	if (!commit) {
+		if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) {
+			printf("We are not bisecting.\n");
+			return 0;
+		}
+		strbuf_rtrim(&branch);
+
+	} else {
+		struct object_id oid;
+		if (get_oid(commit, &oid))
+			return error(_("'%s' is not a valid commit"), commit);
+		strbuf_addf(&branch, "%s", commit);
+	}
+
+	if (!file_exists(git_path_bisect_head())) {
+		struct argv_array argv = ARGV_ARRAY_INIT;
+		argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
+		if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
+			error(_("Could not check out original HEAD '%s'. Try"
+					"'git bisect reset <commit>'."), branch.buf);
+			strbuf_release(&branch);
+			argv_array_clear(&argv);
+			return -1;
+		}
+		argv_array_clear(&argv);
+	}
+
+	strbuf_release(&branch);
+	return bisect_clean_state();
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
-		BISECT_CLEAN_STATE
+		BISECT_CLEAN_STATE,
+		BISECT_RESET
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -138,6 +178,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 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_CMDMODE(0, "bisect-reset", &cmdmode,
+			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -160,6 +202,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 0)
 			die(_("--bisect-clean-state requires no arguments"));
 		return bisect_clean_state();
+	case BISECT_RESET:
+		if (argc > 1)
+			die(_("--bisect-reset requires either zero or one arguments"));
+		return bisect_reset(argc ? argv[0] : NULL);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index bbc57d2..18580b7 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -409,35 +409,11 @@ bisect_visualize() {
 	eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
 }
 
-bisect_reset() {
-	test -s "$GIT_DIR/BISECT_START" || {
-		gettextln "We are not bisecting."
-		return
-	}
-	case "$#" in
-	0) branch=$(cat "$GIT_DIR/BISECT_START") ;;
-	1) git rev-parse --quiet --verify "$1^{commit}" >/dev/null || {
-			invalid="$1"
-			die "$(eval_gettext "'\$invalid' is not a valid commit")"
-		}
-		branch="$1" ;;
-	*)
-		usage ;;
-	esac
-
-	if ! test -f "$GIT_DIR/BISECT_HEAD" && ! git checkout "$branch" --
-	then
-		die "$(eval_gettext "Could not check out original HEAD '\$branch'.
-Try 'git bisect reset <commit>'.")"
-	fi
-	git bisect--helper --bisect-clean-state || exit
-}
-
 bisect_replay () {
 	file="$1"
 	test "$#" -eq 1 || die "$(gettext "No logfile given")"
 	test -r "$file" || die "$(eval_gettext "cannot read \$file for replaying")"
-	bisect_reset
+	git bisect--helper --bisect-reset || exit
 	while read git bisect command rev
 	do
 		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
@@ -627,7 +603,7 @@ case "$#" in
 	visualize|view)
 		bisect_visualize "$@" ;;
 	reset)
-		bisect_reset "$@" ;;
+		git bisect--helper --bisect-reset "$@" ;;
 	replay)
 		bisect_replay "$@" ;;
 	log)
-- 
2.9.0
Eric Sunshine· Jun 15, 2016, 21:05 UTC · re: Pranit Bauva · lore

Re: [PATCH v2 4/6] bisect--helper: `bisect_reset` shell function in C

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 18 quoted lines
> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
> 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
> @@ -123,12 +127,48 @@ static int bisect_clean_state(void)
> +static int bisect_reset(const char *commit)
> +{
> +       struct strbuf branch = STRBUF_INIT;
> +
> +       if (!commit) {
> +               if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) {
> +                       printf("We are not bisecting.\n");
> +                       return 0;
> +               }
> +               strbuf_rtrim(&branch);
> +
Style: unnecessary blank line
Show 5 quoted lines
> +       } else {
> +               struct object_id oid;
> +               if (get_oid(commit, &oid))
> +                       return error(_("'%s' is not a valid commit"), commit);
> +               strbuf_addf(&branch, "%s", commit);
strbuf_addstr(&branch, commit);
Show 18 quoted lines
> +       }
> +
> +       if (!file_exists(git_path_bisect_head())) {
> +               struct argv_array argv = ARGV_ARRAY_INIT;
> +               argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
> +               if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> +                       error(_("Could not check out original HEAD '%s'. Try"
> +                                       "'git bisect reset <commit>'."), branch.buf);
> +                       strbuf_release(&branch);
> +                       argv_array_clear(&argv);
> +                       return -1;
> +               }
> +               argv_array_clear(&argv);
> +       }
> +
> +       strbuf_release(&branch);
> +       return bisect_clean_state();
> +}
Pranit Bauva· Jun 16, 2016, 19:06 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 4/6] bisect--helper: `bisect_reset` shell function in C

Hey Eric,
On Thu, Jun 16, 2016 at 2:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 21 quoted lines
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
>> 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
>> @@ -123,12 +127,48 @@ static int bisect_clean_state(void)
>> +static int bisect_reset(const char *commit)
>> +{
>> +       struct strbuf branch = STRBUF_INIT;
>> +
>> +       if (!commit) {
>> +               if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) {
>> +                       printf("We are not bisecting.\n");
>> +                       return 0;
>> +               }
>> +               strbuf_rtrim(&branch);
>> +
>
> Style: unnecessary blank line
Will fix.
Show 7 quoted lines
>> +       } else {
>> +               struct object_id oid;
>> +               if (get_oid(commit, &oid))
>> +                       return error(_("'%s' is not a valid commit"), commit);
>> +               strbuf_addf(&branch, "%s", commit);
>
> strbuf_addstr(&branch, commit);
Seems better. :)
Show 18 quoted lines
>> +       }
>> +
>> +       if (!file_exists(git_path_bisect_head())) {
>> +               struct argv_array argv = ARGV_ARRAY_INIT;
>> +               argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
>> +               if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
>> +                       error(_("Could not check out original HEAD '%s'. Try"
>> +                                       "'git bisect reset <commit>'."), branch.buf);
>> +                       strbuf_release(&branch);
>> +                       argv_array_clear(&argv);
>> +                       return -1;
>> +               }
>> +               argv_array_clear(&argv);
>> +       }
>> +
>> +       strbuf_release(&branch);
>> +       return bisect_clean_state();
>> +}

Regards, Pranit Bauva

Pranit Bauva· Jun 15, 2016, 14:00 UTC · re: Pranit Bauva · lore

[PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C

Reimplement the `bisect_write` shell function in C and add a `bisect-write` subcommand to `git bisect--helper` to call it from git-bisect.sh

Using `--bisect-write` subcommand is a temporary measure to port shell function in C so as to use the existing test suite. As more functions are ported, this subcommand will be retired and will be called by some other methods.

Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
from the global shell script thus we need to pass it to the subcommand
using the arguments. After the whole conversion, we can remove the extra
arguments and make the method use the two variables from the global scope
within the C code.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 25 ++++----------------
 2 files changed, 64 insertions(+), 22 deletions(-)
Show changes to 2 files +64 −22

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index f11c247..eebfcf0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,6 +22,7 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	N_("git bisect--helper --bisect-clean-state"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
+	N_("git bisect--helper --bisect-write <state> <revision> <TERM_GOOD> <TERM_BAD> [<nolog>]"),
 	NULL
 };
 
@@ -192,6 +193,55 @@ static int check_expected_revs(const char **revs, int rev_nr)
 	return 0;
 }
 
+static int bisect_write(const char *state, const char *rev,
+			const char *term_good, const char *term_bad,
+			int nolog)
+{
+	struct strbuf tag = STRBUF_INIT;
+	struct strbuf commit_name = STRBUF_INIT;
+	struct object_id oid;
+	struct commit *commit;
+	struct pretty_print_context pp = {0};
+	FILE *fp;
+
+	if (!strcmp(state, term_bad))
+		strbuf_addf(&tag, "refs/bisect/%s", state);
+	else if(one_of(state, term_good, "skip", NULL))
+		strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
+	else
+		return error(_("Bad bisect_write argument: %s"), state);
+
+	if (get_oid(rev, &oid)) {
+		strbuf_release(&tag);
+		return error(_("couldn't get the oid of the rev '%s'"), rev);
+	}
+
+	if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
+		       UPDATE_REFS_MSG_ON_ERR)) {
+		strbuf_release(&tag);
+		return -1;
+	}
+
+	fp = fopen(git_path_bisect_log(), "a");
+	if (!fp) {
+		strbuf_release(&tag);
+		return error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
+	}
+
+	commit = lookup_commit_reference(oid.hash);
+	format_commit_message(commit, "%s", &commit_name, &pp);
+	fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
+		commit_name.buf);
+
+	if (!nolog)
+		fprintf(fp, "git bisect %s %s\n", state, rev);
+
+	strbuf_release(&commit_name);
+	strbuf_release(&tag);
+	fclose(fp);
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -199,7 +249,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		WRITE_TERMS,
 		BISECT_CLEAN_STATE,
 		BISECT_RESET,
-		CHECK_EXPECTED_REVS
+		CHECK_EXPECTED_REVS,
+		BISECT_WRITE
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -213,6 +264,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
 			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
+		OPT_CMDMODE(0, "bisect-write", &cmdmode,
+			 N_("write out the bisection state in BISECT_LOG"), BISECT_WRITE),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -225,6 +278,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	switch (cmdmode) {
+	int nolog;
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
 	case WRITE_TERMS:
@@ -241,6 +295,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		return bisect_reset(argc ? argv[0] : NULL);
 	case CHECK_EXPECTED_REVS:
 		return check_expected_revs(argv, argc);
+	case BISECT_WRITE:
+		if (argc != 4 && argc != 5)
+			die(_("--bisect-write requires either 4 or 5 arguments"));
+		nolog = (argc == 5) && !strcmp(argv[4], "nolog");
+		return bisect_write(argv[0], argv[1], argv[2], argv[3], nolog);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 4f6545e..b9896a4 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -145,7 +145,7 @@ bisect_start() {
 		0) state=$TERM_BAD ; bad_seen=1 ;;
 		*) state=$TERM_GOOD ;;
 		esac
-		eval="$eval bisect_write '$state' '$rev' 'nolog' &&"
+		eval="$eval git bisect--helper --bisect-write '$state' '$rev' '$TERM_GOOD' '$TERM_BAD' 'nolog' &&"
 	done
 	#
 	# Verify HEAD.
@@ -221,23 +221,6 @@ bisect_start() {
 	trap '-' 0
 }
 
-bisect_write() {
-	state="$1"
-	rev="$2"
-	nolog="$3"
-	case "$state" in
-		"$TERM_BAD")
-			tag="$state" ;;
-		"$TERM_GOOD"|skip)
-			tag="$state"-"$rev" ;;
-		*)
-			die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
-	esac
-	git update-ref "refs/bisect/$tag" "$rev" || exit
-	echo "# $state: $(git show-branch $rev)" >>"$GIT_DIR/BISECT_LOG"
-	test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -263,7 +246,7 @@ bisect_state() {
 	1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
 		rev=$(git rev-parse --verify $(bisect_head)) ||
 			die "$(gettext "Bad rev input: $(bisect_head)")"
-		bisect_write "$state" "$rev"
+		git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
 		git bisect--helper --check-expected-revs "$rev" ;;
 	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
 		shift
@@ -276,7 +259,7 @@ bisect_state() {
 		done
 		for rev in $hash_list
 		do
-			bisect_write "$state" "$rev"
+			git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
 		done
 		git bisect--helper --check-expected-revs $hash_list ;;
 	*,"$TERM_BAD")
@@ -413,7 +396,7 @@ bisect_replay () {
 			cmd="bisect_start $rev"
 			eval "$cmd" ;;
 		"$TERM_GOOD"|"$TERM_BAD"|skip)
-			bisect_write "$command" "$rev" ;;
+			git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
 		terms)
 			bisect_terms $rev ;;
 		*)
-- 
2.9.0
Eric Sunshine· Jun 16, 2016, 18:55 UTC · re: Pranit Bauva · lore

Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 14 quoted lines
> Reimplement the `bisect_write` shell function in C and add a
> `bisect-write` subcommand to `git bisect--helper` to call it from
> git-bisect.sh
>
> Using `--bisect-write` subcommand is a temporary measure to port shell
> function in C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other methods.
>
> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
> from the global shell script thus we need to pass it to the subcommand
> using the arguments. After the whole conversion, we can remove the extra
> arguments and make the method use the two variables from the global scope
> within the C code.

You could do this now rather than waiting for later. Instead of passing these arguments to bisect_write(), create global variables in this patch and assign them in the BISECT_WRITE case of cmd_bisect__helper() before calling bisect_write().

Not necessarily worth a re-roll, but would save you the effort of having to explain it here and then change it in some later patch.

Show 26 quoted lines
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -192,6 +193,55 @@ static int check_expected_revs(const char **revs, int rev_nr)
> +static int bisect_write(const char *state, const char *rev,
> +                       const char *term_good, const char *term_bad,
> +                       int nolog)
> +{
> +       struct strbuf tag = STRBUF_INIT;
> +       struct strbuf commit_name = STRBUF_INIT;
> +       struct object_id oid;
> +       struct commit *commit;
> +       struct pretty_print_context pp = {0};
> +       FILE *fp;
> +
> +       if (!strcmp(state, term_bad))
> +               strbuf_addf(&tag, "refs/bisect/%s", state);
> +       else if(one_of(state, term_good, "skip", NULL))
> +               strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
> +       else
> +               return error(_("Bad bisect_write argument: %s"), state);
> +
> +       if (get_oid(rev, &oid)) {
> +               strbuf_release(&tag);
> +               return error(_("couldn't get the oid of the rev '%s'"), rev);
> +       }
Minor: If you move the get_oid() conditional before the one above it,
then you won't have to worry about releasing 'strbuf tag' at this
point.
Show 5 quoted lines
> +       if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
> +                      UPDATE_REFS_MSG_ON_ERR)) {
> +               strbuf_release(&tag);
> +               return -1;
> +       }

If you release 'strbuf tag' right here, after it's final use, then you won't have to worry about releasing it anywhere below (particularly in the error cases).

Show 24 quoted lines
> +       fp = fopen(git_path_bisect_log(), "a");
> +       if (!fp) {
> +               strbuf_release(&tag);
> +               return error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
> +       }
> +
> +       commit = lookup_commit_reference(oid.hash);
> +       format_commit_message(commit, "%s", &commit_name, &pp);
> +       fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
> +               commit_name.buf);
> +
> +       if (!nolog)
> +               fprintf(fp, "git bisect %s %s\n", state, rev);
> +
> +       strbuf_release(&commit_name);
> +       strbuf_release(&tag);
> +       fclose(fp);
> +       return 0;
> +}
> @@ -241,6 +295,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> +       case BISECT_WRITE:
> +               if (argc != 4 && argc != 5)
> +                       die(_("--bisect-write requires either 4 or 5 arguments"));
> +               nolog = (argc == 5) && !strcmp(argv[4], "nolog");

This is minor and won't matter in the long run when this code goes away later in the C conversion, but this differs from the shell code which only cared that a (non-empty) fifth argument was provided but didn't care about the actual value, whereas this code expects the argument to be exactly "nolog".

> +               return bisect_write(argv[0], argv[1], argv[2], argv[3], nolog);
>         default:
>                 die("BUG: unknown subcommand '%d'", cmdmode);
>         }
Pranit Bauva· Jun 16, 2016, 19:01 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C

Hey Eric,
On Fri, Jun 17, 2016 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 23 quoted lines
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement the `bisect_write` shell function in C and add a
>> `bisect-write` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh
>>
>> Using `--bisect-write` subcommand is a temporary measure to port shell
>> function in C so as to use the existing test suite. As more functions
>> are ported, this subcommand will be retired and will be called by some
>> other methods.
>>
>> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
>> from the global shell script thus we need to pass it to the subcommand
>> using the arguments. After the whole conversion, we can remove the extra
>> arguments and make the method use the two variables from the global scope
>> within the C code.
>
> You could do this now rather than waiting for later. Instead of
> passing these arguments to bisect_write(), create global variables in
> this patch and assign them in the BISECT_WRITE case of
> cmd_bisect__helper() before calling bisect_write().
>
> Not necessarily worth a re-roll, but would save you the effort of
> having to explain it here and then change it in some later patch.

I have actually done it in my next conversion which is converting check_and_set_terms()[1] which also sets those variables to some value so its more appropriate there.

Show 30 quoted lines
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -192,6 +193,55 @@ static int check_expected_revs(const char **revs, int rev_nr)
>> +static int bisect_write(const char *state, const char *rev,
>> +                       const char *term_good, const char *term_bad,
>> +                       int nolog)
>> +{
>> +       struct strbuf tag = STRBUF_INIT;
>> +       struct strbuf commit_name = STRBUF_INIT;
>> +       struct object_id oid;
>> +       struct commit *commit;
>> +       struct pretty_print_context pp = {0};
>> +       FILE *fp;
>> +
>> +       if (!strcmp(state, term_bad))
>> +               strbuf_addf(&tag, "refs/bisect/%s", state);
>> +       else if(one_of(state, term_good, "skip", NULL))
>> +               strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
>> +       else
>> +               return error(_("Bad bisect_write argument: %s"), state);
>> +
>> +       if (get_oid(rev, &oid)) {
>> +               strbuf_release(&tag);
>> +               return error(_("couldn't get the oid of the rev '%s'"), rev);
>> +       }
>
> Minor: If you move the get_oid() conditional before the one above it,
> then you won't have to worry about releasing 'strbuf tag' at this
> point.
Nice. I will do that. :)
Show 9 quoted lines
>> +       if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
>> +                      UPDATE_REFS_MSG_ON_ERR)) {
>> +               strbuf_release(&tag);
>> +               return -1;
>> +       }
>
> If you release 'strbuf tag' right here, after it's final use, then you
> won't have to worry about releasing it anywhere below (particularly in
> the error cases).
True.
Show 30 quoted lines
>> +       fp = fopen(git_path_bisect_log(), "a");
>> +       if (!fp) {
>> +               strbuf_release(&tag);
>> +               return error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
>> +       }
>> +
>> +       commit = lookup_commit_reference(oid.hash);
>> +       format_commit_message(commit, "%s", &commit_name, &pp);
>> +       fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
>> +               commit_name.buf);
>> +
>> +       if (!nolog)
>> +               fprintf(fp, "git bisect %s %s\n", state, rev);
>> +
>> +       strbuf_release(&commit_name);
>> +       strbuf_release(&tag);
>> +       fclose(fp);
>> +       return 0;
>> +}
>> @@ -241,6 +295,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> +       case BISECT_WRITE:
>> +               if (argc != 4 && argc != 5)
>> +                       die(_("--bisect-write requires either 4 or 5 arguments"));
>> +               nolog = (argc == 5) && !strcmp(argv[4], "nolog");
>
> This is minor and won't matter in the long run when this code goes
> away later in the C conversion, but this differs from the shell code
> which only cared that a (non-empty) fifth argument was provided but
> didn't care about the actual value, whereas this code expects the
> argument to be exactly "nolog".

We currently have tight control over the arguments we are passing as they are only programmer defined.

>> +               return bisect_write(argv[0], argv[1], argv[2], argv[3], nolog);
>>         default:
>>                 die("BUG: unknown subcommand '%d'", cmdmode);
>>         }
[1]: https://github.com/pranitbauva1997/git/pull/16

Regards, Pranit Bauva

Christian Couder· Jun 16, 2016, 20:38 UTC · re: Pranit Bauva · lore

Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C

On Thu, Jun 16, 2016 at 9:01 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 22 quoted lines
> Hey Eric,
>
> On Fri, Jun 17, 2016 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>
>>> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
>>> from the global shell script thus we need to pass it to the subcommand
>>> using the arguments. After the whole conversion, we can remove the extra
>>> arguments and make the method use the two variables from the global scope
>>> within the C code.
>>
>> You could do this now rather than waiting for later. Instead of
>> passing these arguments to bisect_write(), create global variables in
>> this patch and assign them in the BISECT_WRITE case of
>> cmd_bisect__helper() before calling bisect_write().
>>
>> Not necessarily worth a re-roll, but would save you the effort of
>> having to explain it here and then change it in some later patch.
>
> I have actually done it in my next conversion which is converting
> check_and_set_terms()[1] which also sets those variables to some value
> so its more appropriate there.

My opinion about this is that using global variables would go against a possible future libification of the bisect functionality and might be less safe than just adding 2 parameters to a small number of functions.

If we think that 2 parameters are too much or that there could be more parameters to pass like this, we could just pass a pointer to a 'struct bisect_state' or something like that ;-)

Pranit Bauva· Jun 17, 2016, 13:10 UTC · re: Christian Couder · lore

Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C

Hey Christian,

On Fri, Jun 17, 2016 at 2:08 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 32 quoted lines
> On Thu, Jun 16, 2016 at 9:01 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Hey Eric,
>>
>> On Fri, Jun 17, 2016 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>>
>>>> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
>>>> from the global shell script thus we need to pass it to the subcommand
>>>> using the arguments. After the whole conversion, we can remove the extra
>>>> arguments and make the method use the two variables from the global scope
>>>> within the C code.
>>>
>>> You could do this now rather than waiting for later. Instead of
>>> passing these arguments to bisect_write(), create global variables in
>>> this patch and assign them in the BISECT_WRITE case of
>>> cmd_bisect__helper() before calling bisect_write().
>>>
>>> Not necessarily worth a re-roll, but would save you the effort of
>>> having to explain it here and then change it in some later patch.
>>
>> I have actually done it in my next conversion which is converting
>> check_and_set_terms()[1] which also sets those variables to some value
>> so its more appropriate there.
>
> My opinion about this is that using global variables would go against
> a possible future libification of the bisect functionality and might
> be less safe than just adding 2 parameters to a small number of
> functions.
>
> If we think that 2 parameters are too much or that there could be more
> parameters to pass like this, we could just pass a pointer to a
> 'struct bisect_state' or something like that ;-)
I had in mind something about 'struct bisect_state'.

Regards, Pranit Bauva

Eric Sunshine· Jun 15, 2016, 17:53 UTC · re: Pranit Bauva · lore

Re: [PATCH v2 0/6] convert various shell functions in git-bisect to C

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 9 quoted lines
> Changes wrt previous version:
>  * Use STRING_LIST_INIT_NODUP to avoid leaks in bisect_clean_state()
>  * Use test_path_is_missing in the patch 2/6
>  * drop file_size()
>  * move is_empty_file() method from builtin/am.c to wrapper.c
>  * use static for methods
>  * remove the variable status in bisect_reset() altogether and put the whole
>    thing inside the if block.
>  * one more method converted namely bisect_write().

As an aid to reviewers, in the future, please also include in the cover letter an interdiff between the previous and current version of the patch series. Thanks.

Pranit Bauva· Jun 15, 2016, 18:09 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 0/6] convert various shell functions in git-bisect to C

Hey Eric,
On Wed, Jun 15, 2016 at 11:23 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 14 quoted lines
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Changes wrt previous version:
>>  * Use STRING_LIST_INIT_NODUP to avoid leaks in bisect_clean_state()
>>  * Use test_path_is_missing in the patch 2/6
>>  * drop file_size()
>>  * move is_empty_file() method from builtin/am.c to wrapper.c
>>  * use static for methods
>>  * remove the variable status in bisect_reset() altogether and put the whole
>>    thing inside the if block.
>>  * one more method converted namely bisect_write().
>
> As an aid to reviewers, in the future, please also include in the
> cover letter an interdiff between the previous and current version of
> the patch series. Thanks.
Sure!

Regards, Pranit Bauva

Pranit Bauva· Jun 26, 2016, 12:23 UTC · re: Pranit Bauva · lore

[PATCH v3 0/6] convert various shell functions in git-bisect to C

The major change introduced in this version is that I have used `struct bisect_terms` to store term_good and term_bad and then I am passing around the memory address of it to functions. Also I have made various changes in accordance with the previous review.

Here is the link for v2. http://thread.gmane.org/gmane.comp.version-control.git/297372

Thanks for suggestions Eric Sunshine, Christian Couder and Junio C Hammano.
Here's the interdiff:
Show changes to 4 files +54 −32

builtin/am.c, builtin/bisect--helper.c, cache.h, wrapper.c

diff --git a/builtin/am.c b/builtin/am.c
index 84f21d0..6ee158f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1307,7 +1307,7 @@ static int parse_mail(struct am_state *state, const char *mail)
 		goto finish;
 	}
 
-	if (is_empty_file(am_path(state, "patch"))) {
+	if (is_empty_or_missing_file(am_path(state, "patch"))) {
 		printf_ln(_("Patch is empty. Was it split wrong?"));
 		die_user_resolve(state);
 	}
@@ -1895,7 +1895,7 @@ next:
 		resume = 0;
 	}
 
-	if (!is_empty_file(am_path(state, "rewritten"))) {
+	if (!is_empty_or_missing_file(am_path(state, "rewritten"))) {
 		assert(state->rebasing);
 		copy_notes_for_rebase(state);
 		run_post_rewrite_hook(state);
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index eebfcf0..e946ba9 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -26,6 +26,25 @@ static const char * const git_bisect_helper_usage[] = {
 	NULL
 };
 
+struct bisect_terms {
+	struct strbuf term_good;
+	struct strbuf term_bad;
+};
+
+static int bisect_terms_init(struct bisect_terms *term)
+{
+	strbuf_init(&term->term_good, 0);
+	strbuf_init(&term->term_bad, 0);
+	return 0;
+}
+
+static int bisect_terms_release(struct bisect_terms *term)
+{
+	strbuf_release(&term->term_good);
+	strbuf_release(&term->term_good);
+	return 0;
+}
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -110,6 +129,7 @@ static int bisect_clean_state(void)
 	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);
+	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
 	remove_path(git_path_bisect_expected_rev());
 	remove_path(git_path_bisect_ancestors_ok());
@@ -138,12 +158,11 @@ static int bisect_reset(const char *commit)
 			return 0;
 		}
 		strbuf_rtrim(&branch);
-
 	} else {
 		struct object_id oid;
 		if (get_oid(commit, &oid))
 			return error(_("'%s' is not a valid commit"), commit);
-		strbuf_addf(&branch, "%s", commit);
+		strbuf_addstr(&branch, commit);
 	}
 
 	if (!file_exists(git_path_bisect_head())) {
@@ -151,7 +170,7 @@ static int bisect_reset(const char *commit)
 		argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
 		if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
 			error(_("Could not check out original HEAD '%s'. Try"
-					"'git bisect reset <commit>'."), branch.buf);
+				"'git bisect reset <commit>'."), branch.buf);
 			strbuf_release(&branch);
 			argv_array_clear(&argv);
 			return -1;
@@ -166,15 +185,11 @@ static int bisect_reset(const char *commit)
 static int is_expected_rev(const char *expected_hex)
 {
 	struct strbuf actual_hex = STRBUF_INIT;
-	int res;
-
-	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
-		strbuf_release(&actual_hex);
-		return 0;
+	int res = 0;
+	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 0) {
+		strbuf_trim(&actual_hex);
+		res = !strcmp(actual_hex.buf, expected_hex);
 	}
-
-	strbuf_trim(&actual_hex);
-	res = !strcmp(actual_hex.buf, expected_hex);
 	strbuf_release(&actual_hex);
 	return res;
 }
@@ -194,8 +209,7 @@ static int check_expected_revs(const char **revs, int rev_nr)
 }
 
 static int bisect_write(const char *state, const char *rev,
-			const char *term_good, const char *term_bad,
-			int nolog)
+			const struct bisect_terms *term, int nolog)
 {
 	struct strbuf tag = STRBUF_INIT;
 	struct strbuf commit_name = STRBUF_INIT;
@@ -204,9 +218,9 @@ static int bisect_write(const char *state, const char *rev,
 	struct pretty_print_context pp = {0};
 	FILE *fp;
 
-	if (!strcmp(state, term_bad))
+	if (!strcmp(state, term->term_bad.buf))
 		strbuf_addf(&tag, "refs/bisect/%s", state);
-	else if(one_of(state, term_good, "skip", NULL))
+	else if(one_of(state, term->term_good.buf, "skip", NULL))
 		strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
 	else
 		return error(_("Bad bisect_write argument: %s"), state);
@@ -221,23 +235,21 @@ static int bisect_write(const char *state, const char *rev,
 		strbuf_release(&tag);
 		return -1;
 	}
+	strbuf_release(&tag);
 
 	fp = fopen(git_path_bisect_log(), "a");
-	if (!fp) {
-		strbuf_release(&tag);
+	if (!fp)
 		return error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
-	}
 
 	commit = lookup_commit_reference(oid.hash);
 	format_commit_message(commit, "%s", &commit_name, &pp);
 	fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
 		commit_name.buf);
+	strbuf_release(&commit_name);
 
 	if (!nolog)
 		fprintf(fp, "git bisect %s %s\n", state, rev);
 
-	strbuf_release(&commit_name);
-	strbuf_release(&tag);
 	fclose(fp);
 	return 0;
 }
@@ -252,7 +264,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		CHECK_EXPECTED_REVS,
 		BISECT_WRITE
 	} cmdmode = 0;
-	int no_checkout = 0;
+	int no_checkout = 0, res = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
@@ -270,6 +282,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
 	};
+	struct bisect_terms state;
+	bisect_terms_init(&state);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
@@ -284,24 +298,32 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	case WRITE_TERMS:
 		if (argc != 2)
 			die(_("--write-terms requires two arguments"));
-		return write_terms(argv[0], argv[1]);
+		res = write_terms(argv[0], argv[1]);
+		break;
 	case BISECT_CLEAN_STATE:
 		if (argc != 0)
 			die(_("--bisect-clean-state requires no arguments"));
-		return bisect_clean_state();
+		res = bisect_clean_state();
+		break;
 	case BISECT_RESET:
 		if (argc > 1)
 			die(_("--bisect-reset requires either zero or one arguments"));
-		return bisect_reset(argc ? argv[0] : NULL);
+		res = bisect_reset(argc ? argv[0] : NULL);
+		break;
 	case CHECK_EXPECTED_REVS:
-		return check_expected_revs(argv, argc);
+		res = check_expected_revs(argv, argc);
+		break;
 	case BISECT_WRITE:
 		if (argc != 4 && argc != 5)
 			die(_("--bisect-write requires either 4 or 5 arguments"));
 		nolog = (argc == 5) && !strcmp(argv[4], "nolog");
-		return bisect_write(argv[0], argv[1], argv[2], argv[3], nolog);
+		strbuf_addstr(&state.term_good, argv[2]);
+		strbuf_addstr(&state.term_bad, argv[3]);
+		res = bisect_write(argv[0], argv[1], &state, nolog);
+		break;
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
-	return 0;
+	bisect_terms_release(&state);
+	return res;
 }
diff --git a/cache.h b/cache.h
index 8eaad70..91e2f81 100644
--- a/cache.h
+++ b/cache.h
@@ -1871,6 +1871,6 @@ void sleep_millisec(int millisec);
 void safe_create_dir(const char *dir, int share);
 
 /* Return 1 if the file is empty or does not exists, 0 otherwise. */
-extern int is_empty_file(const char *filename);
+extern int is_empty_or_missing_file(const char *filename);
 
 #endif /* CACHE_H */
diff --git a/wrapper.c b/wrapper.c
index 36a3eeb..e70e4d1 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -697,14 +697,14 @@ void sleep_millisec(int millisec)
 	poll(NULL, 0, millisec);
 }
 
-int is_empty_file(const char *filename)
+int is_empty_or_missing_file(const char *filename)
 {
 	struct stat st;
 
 	if (stat(filename, &st) < 0) {
 		if (errno == ENOENT)
 			return 1;
-		error_errno(_("could not stat %s"), filename);
+		die_errno(_("could not stat %s"), filename);
 	}
 
 	return !st.st_size;



Pranit Bauva (6):
  bisect--helper: `bisect_clean_state` shell function in C
  t6030: explicitly test for bisection cleanup
  wrapper: move is_empty_file() and rename it as
    is_empty_or_missing_file()
  bisect--helper: `bisect_reset` shell function in C
  bisect--helper: `is_expected_rev` & `check_expected_revs` shell
    function in C
  bisect--helper: `bisect_write` shell function in C

 builtin/am.c                |  20 +---
 builtin/bisect--helper.c    | 222 +++++++++++++++++++++++++++++++++++++++++++-
 cache.h                     |   3 +
 git-bisect.sh               |  97 ++-----------------
 t/t6030-bisect-porcelain.sh |  17 ++++
 wrapper.c                   |  13 +++
 6 files changed, 263 insertions(+), 109 deletions(-)
-- 
2.9.0
Pranit Bauva· Jun 26, 2016, 12:23 UTC · re: Pranit Bauva · lore

[PATCH v3 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 <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 26 +++--------------------
 2 files changed, 57 insertions(+), 24 deletions(-)
Show changes to 2 files +57 −24

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 91027b0..57fd4e9 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 <bad_term> <good_term>"),
+	N_("git bisect--helper --bisect-clean-state"),
 	NULL
 };
 
@@ -78,11 +87,49 @@ 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);
+	refs_for_removal.strdup_strings = 1;
+	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 +137,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 +157,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 @@ 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.9.0
Pranit Bauva· Jun 26, 2016, 12:23 UTC · re: Pranit Bauva · lore

[PATCH v3 2/6] t6030: explicitly test for bisection cleanup

This is not an improvement in the test coverage but it helps in making it explicit as to what exactly would be the error as other tests are focussed on testing other things.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

--- I faced this problem while converting `bisect_clean_state` and the tests where showing breakages but it wasn't clear as to where exactly are they breaking. This will patch will help in that. Also I tested the test coverage of the test suite before this patch and it covers this (I did this by purposely changing names of files in git-bisect.sh and running the test suite).

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Show changes to t/t6030-bisect-porcelain.sh +17 −0
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index e74662b..a17f7a6 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git bisect reset cleans bisection state properly' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect reset &&
+	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
+	test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
+	test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
+	test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
+	test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
+	test_path_is_missing "$GIT_DIR/head-name" &&
+	test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
+	test_path_is_missing "$GIT_DIR/BISECT_START"
+'
+
 test_done
-- 
2.9.0
Pranit Bauva· Jun 26, 2016, 12:23 UTC · re: Pranit Bauva · lore

[PATCH v3 3/6] wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()

is_empty_file() can help to refactor a lot of code. This will be very helpful in porting "git bisect" to C.

Suggested-by: Torsten Bögershausen <tboegi@web.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/am.c | 20 ++------------------
 cache.h      |  3 +++
 wrapper.c    | 13 +++++++++++++
 3 files changed, 18 insertions(+), 18 deletions(-)
Show changes to 3 files +18 −18

builtin/am.c, cache.h, wrapper.c

diff --git a/builtin/am.c b/builtin/am.c
index 3dfe70b..6ee158f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -30,22 +30,6 @@
 #include "mailinfo.h"
 
 /**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-	struct stat st;
-
-	if (stat(filename, &st) < 0) {
-		if (errno == ENOENT)
-			return 1;
-		die_errno(_("could not stat %s"), filename);
-	}
-
-	return !st.st_size;
-}
-
-/**
  * Returns the length of the first line of msg.
  */
 static int linelen(const char *msg)
@@ -1323,7 +1307,7 @@ static int parse_mail(struct am_state *state, const char *mail)
 		goto finish;
 	}
 
-	if (is_empty_file(am_path(state, "patch"))) {
+	if (is_empty_or_missing_file(am_path(state, "patch"))) {
 		printf_ln(_("Patch is empty. Was it split wrong?"));
 		die_user_resolve(state);
 	}
@@ -1911,7 +1895,7 @@ next:
 		resume = 0;
 	}
 
-	if (!is_empty_file(am_path(state, "rewritten"))) {
+	if (!is_empty_or_missing_file(am_path(state, "rewritten"))) {
 		assert(state->rebasing);
 		copy_notes_for_rebase(state);
 		run_post_rewrite_hook(state);
diff --git a/cache.h b/cache.h
index 6049f86..91e2f81 100644
--- a/cache.h
+++ b/cache.h
@@ -1870,4 +1870,7 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+extern int is_empty_or_missing_file(const char *filename);
+
 #endif /* CACHE_H */
diff --git a/wrapper.c b/wrapper.c
index 5dc4e15..e70e4d1 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -696,3 +696,16 @@ void sleep_millisec(int millisec)
 {
 	poll(NULL, 0, millisec);
 }
+
+int is_empty_or_missing_file(const char *filename)
+{
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		if (errno == ENOENT)
+			return 1;
+		die_errno(_("could not stat %s"), filename);
+	}
+
+	return !st.st_size;
+}
-- 
2.9.0
Pranit Bauva· Jun 26, 2016, 12:23 UTC · re: Pranit Bauva · lore

[PATCH v3 4/6] bisect--helper: `bisect_reset` shell function in C

Reimplement `bisect_reset` shell function in C and add a `--bisect-reset` subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `bisect_reset` subcommand is a temporary measure to port shell functions to C so as to use the existing test suite. As more functions are ported, this subcommand would be retired and will be called by some other method.

Note: --bisect-clean-state subcommand has not been retired as there are
still a function namely `bisect_start()` which still uses this
subcommand.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 28 ++--------------------------
 2 files changed, 48 insertions(+), 27 deletions(-)
Show changes to 2 files +48 −27

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 57fd4e9..96f6b65 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,6 +4,8 @@
 #include "bisect.h"
 #include "refs.h"
 #include "dir.h"
+#include "argv-array.h"
+#include "run-command.h"
 
 static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -13,11 +15,13 @@ 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 GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 
 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"),
+	N_("git bisect--helper --bisect-reset [<commit>]"),
 	NULL
 };
 
@@ -124,12 +128,47 @@ static int bisect_clean_state(void)
 	return result;
 }
 
+static int bisect_reset(const char *commit)
+{
+	struct strbuf branch = STRBUF_INIT;
+
+	if (!commit) {
+		if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) {
+			printf("We are not bisecting.\n");
+			return 0;
+		}
+		strbuf_rtrim(&branch);
+	} else {
+		struct object_id oid;
+		if (get_oid(commit, &oid))
+			return error(_("'%s' is not a valid commit"), commit);
+		strbuf_addstr(&branch, commit);
+	}
+
+	if (!file_exists(git_path_bisect_head())) {
+		struct argv_array argv = ARGV_ARRAY_INIT;
+		argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
+		if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
+			error(_("Could not check out original HEAD '%s'. Try"
+				"'git bisect reset <commit>'."), branch.buf);
+			strbuf_release(&branch);
+			argv_array_clear(&argv);
+			return -1;
+		}
+		argv_array_clear(&argv);
+	}
+
+	strbuf_release(&branch);
+	return bisect_clean_state();
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
-		BISECT_CLEAN_STATE
+		BISECT_CLEAN_STATE,
+		BISECT_RESET
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -139,6 +178,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 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_CMDMODE(0, "bisect-reset", &cmdmode,
+			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -161,6 +202,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 0)
 			die(_("--bisect-clean-state requires no arguments"));
 		return bisect_clean_state();
+	case BISECT_RESET:
+		if (argc > 1)
+			die(_("--bisect-reset requires either zero or one arguments"));
+		return bisect_reset(argc ? argv[0] : NULL);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index bbc57d2..18580b7 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -409,35 +409,11 @@ bisect_visualize() {
 	eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
 }
 
-bisect_reset() {
-	test -s "$GIT_DIR/BISECT_START" || {
-		gettextln "We are not bisecting."
-		return
-	}
-	case "$#" in
-	0) branch=$(cat "$GIT_DIR/BISECT_START") ;;
-	1) git rev-parse --quiet --verify "$1^{commit}" >/dev/null || {
-			invalid="$1"
-			die "$(eval_gettext "'\$invalid' is not a valid commit")"
-		}
-		branch="$1" ;;
-	*)
-		usage ;;
-	esac
-
-	if ! test -f "$GIT_DIR/BISECT_HEAD" && ! git checkout "$branch" --
-	then
-		die "$(eval_gettext "Could not check out original HEAD '\$branch'.
-Try 'git bisect reset <commit>'.")"
-	fi
-	git bisect--helper --bisect-clean-state || exit
-}
-
 bisect_replay () {
 	file="$1"
 	test "$#" -eq 1 || die "$(gettext "No logfile given")"
 	test -r "$file" || die "$(eval_gettext "cannot read \$file for replaying")"
-	bisect_reset
+	git bisect--helper --bisect-reset || exit
 	while read git bisect command rev
 	do
 		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
@@ -627,7 +603,7 @@ case "$#" in
 	visualize|view)
 		bisect_visualize "$@" ;;
 	reset)
-		bisect_reset "$@" ;;
+		git bisect--helper --bisect-reset "$@" ;;
 	replay)
 		bisect_replay "$@" ;;
 	log)
-- 
2.9.0
Pranit Bauva· Jun 26, 2016, 12:23 UTC · re: Pranit Bauva · lore

[PATCH v3 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

Reimplement `is_expected_rev` & `check_expected_revs` shell function in C and add a `--check-expected-revs` subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `--check-expected-revs` subcommand is a temporary measure to port shell functions to C so as to use the existing test suite. As more functions are ported, this subcommand would be retired and will be called by some other method.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 33 ++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 20 ++------------------
 2 files changed, 34 insertions(+), 19 deletions(-)
Show changes to 2 files +34 −19

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 96f6b65..96de65a 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -162,13 +162,40 @@ static int bisect_reset(const char *commit)
 	return bisect_clean_state();
 }
 
+static int is_expected_rev(const char *expected_hex)
+{
+	struct strbuf actual_hex = STRBUF_INIT;
+	int res = 0;
+	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 0) {
+		strbuf_trim(&actual_hex);
+		res = !strcmp(actual_hex.buf, expected_hex);
+	}
+	strbuf_release(&actual_hex);
+	return res;
+}
+
+static int check_expected_revs(const char **revs, int rev_nr)
+{
+	int i;
+
+	for (i = 0; i < rev_nr; i++) {
+		if (!is_expected_rev(revs[i])) {
+			remove_path(git_path_bisect_ancestors_ok());
+			remove_path(git_path_bisect_expected_rev());
+			return 0;
+		}
+	}
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
 		BISECT_CLEAN_STATE,
-		BISECT_RESET
+		BISECT_RESET,
+		CHECK_EXPECTED_REVS
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -180,6 +207,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
 			 N_("reset the bisection state"), BISECT_RESET),
+		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
+			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -206,6 +235,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc > 1)
 			die(_("--bisect-reset requires either zero or one arguments"));
 		return bisect_reset(argc ? argv[0] : NULL);
+	case CHECK_EXPECTED_REVS:
+		return check_expected_revs(argv, argc);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 18580b7..4f6545e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -238,22 +238,6 @@ bisect_write() {
 	test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
 }
 
-is_expected_rev() {
-	test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
-	test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
-}
-
-check_expected_revs() {
-	for _rev in "$@"; do
-		if ! is_expected_rev "$_rev"
-		then
-			rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
-			rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
-			return
-		fi
-	done
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -280,7 +264,7 @@ bisect_state() {
 		rev=$(git rev-parse --verify $(bisect_head)) ||
 			die "$(gettext "Bad rev input: $(bisect_head)")"
 		bisect_write "$state" "$rev"
-		check_expected_revs "$rev" ;;
+		git bisect--helper --check-expected-revs "$rev" ;;
 	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
 		shift
 		hash_list=''
@@ -294,7 +278,7 @@ bisect_state() {
 		do
 			bisect_write "$state" "$rev"
 		done
-		check_expected_revs $hash_list ;;
+		git bisect--helper --check-expected-revs $hash_list ;;
 	*,"$TERM_BAD")
 		die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
 	*)
-- 
2.9.0
Pranit Bauva· Jun 26, 2016, 12:23 UTC · re: Pranit Bauva · lore

[PATCH v3 6/6] bisect--helper: `bisect_write` shell function in C

Reimplement the `bisect_write` shell function in C and add a `bisect-write` subcommand to `git bisect--helper` to call it from git-bisect.sh

Using `--bisect-write` subcommand is a temporary measure to port shell function in C so as to use the existing test suite. As more functions are ported, this subcommand will be retired and will be called by some other methods.

Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
from the global shell script thus we need to pass it to the subcommand
using the arguments. We then store them in a struct bisect_state and
pass the memory address around functions.

This patch also introduces new methods namely bisect_state_init() and bisect_state_release() for easy memory management for the struct bisect_state.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 99 ++++++++++++++++++++++++++++++++++++++++++++----
 git-bisect.sh            | 25 ++----------
 2 files changed, 96 insertions(+), 28 deletions(-)
Show changes to 2 files +96 −28

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 96de65a..e946ba9 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,9 +22,29 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	N_("git bisect--helper --bisect-clean-state"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
+	N_("git bisect--helper --bisect-write <state> <revision> <TERM_GOOD> <TERM_BAD> [<nolog>]"),
 	NULL
 };
 
+struct bisect_terms {
+	struct strbuf term_good;
+	struct strbuf term_bad;
+};
+
+static int bisect_terms_init(struct bisect_terms *term)
+{
+	strbuf_init(&term->term_good, 0);
+	strbuf_init(&term->term_bad, 0);
+	return 0;
+}
+
+static int bisect_terms_release(struct bisect_terms *term)
+{
+	strbuf_release(&term->term_good);
+	strbuf_release(&term->term_good);
+	return 0;
+}
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -188,6 +208,52 @@ static int check_expected_revs(const char **revs, int rev_nr)
 	return 0;
 }
 
+static int bisect_write(const char *state, const char *rev,
+			const struct bisect_terms *term, int nolog)
+{
+	struct strbuf tag = STRBUF_INIT;
+	struct strbuf commit_name = STRBUF_INIT;
+	struct object_id oid;
+	struct commit *commit;
+	struct pretty_print_context pp = {0};
+	FILE *fp;
+
+	if (!strcmp(state, term->term_bad.buf))
+		strbuf_addf(&tag, "refs/bisect/%s", state);
+	else if(one_of(state, term->term_good.buf, "skip", NULL))
+		strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
+	else
+		return error(_("Bad bisect_write argument: %s"), state);
+
+	if (get_oid(rev, &oid)) {
+		strbuf_release(&tag);
+		return error(_("couldn't get the oid of the rev '%s'"), rev);
+	}
+
+	if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
+		       UPDATE_REFS_MSG_ON_ERR)) {
+		strbuf_release(&tag);
+		return -1;
+	}
+	strbuf_release(&tag);
+
+	fp = fopen(git_path_bisect_log(), "a");
+	if (!fp)
+		return error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
+
+	commit = lookup_commit_reference(oid.hash);
+	format_commit_message(commit, "%s", &commit_name, &pp);
+	fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
+		commit_name.buf);
+	strbuf_release(&commit_name);
+
+	if (!nolog)
+		fprintf(fp, "git bisect %s %s\n", state, rev);
+
+	fclose(fp);
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -195,9 +261,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		WRITE_TERMS,
 		BISECT_CLEAN_STATE,
 		BISECT_RESET,
-		CHECK_EXPECTED_REVS
+		CHECK_EXPECTED_REVS,
+		BISECT_WRITE
 	} cmdmode = 0;
-	int no_checkout = 0;
+	int no_checkout = 0, res = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
@@ -209,10 +276,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
 			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
+		OPT_CMDMODE(0, "bisect-write", &cmdmode,
+			 N_("write out the bisection state in BISECT_LOG"), BISECT_WRITE),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
 	};
+	struct bisect_terms state;
+	bisect_terms_init(&state);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
@@ -221,24 +292,38 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	switch (cmdmode) {
+	int nolog;
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
 	case WRITE_TERMS:
 		if (argc != 2)
 			die(_("--write-terms requires two arguments"));
-		return write_terms(argv[0], argv[1]);
+		res = write_terms(argv[0], argv[1]);
+		break;
 	case BISECT_CLEAN_STATE:
 		if (argc != 0)
 			die(_("--bisect-clean-state requires no arguments"));
-		return bisect_clean_state();
+		res = bisect_clean_state();
+		break;
 	case BISECT_RESET:
 		if (argc > 1)
 			die(_("--bisect-reset requires either zero or one arguments"));
-		return bisect_reset(argc ? argv[0] : NULL);
+		res = bisect_reset(argc ? argv[0] : NULL);
+		break;
 	case CHECK_EXPECTED_REVS:
-		return check_expected_revs(argv, argc);
+		res = check_expected_revs(argv, argc);
+		break;
+	case BISECT_WRITE:
+		if (argc != 4 && argc != 5)
+			die(_("--bisect-write requires either 4 or 5 arguments"));
+		nolog = (argc == 5) && !strcmp(argv[4], "nolog");
+		strbuf_addstr(&state.term_good, argv[2]);
+		strbuf_addstr(&state.term_bad, argv[3]);
+		res = bisect_write(argv[0], argv[1], &state, nolog);
+		break;
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
-	return 0;
+	bisect_terms_release(&state);
+	return res;
 }
diff --git a/git-bisect.sh b/git-bisect.sh
index 4f6545e..b9896a4 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -145,7 +145,7 @@ bisect_start() {
 		0) state=$TERM_BAD ; bad_seen=1 ;;
 		*) state=$TERM_GOOD ;;
 		esac
-		eval="$eval bisect_write '$state' '$rev' 'nolog' &&"
+		eval="$eval git bisect--helper --bisect-write '$state' '$rev' '$TERM_GOOD' '$TERM_BAD' 'nolog' &&"
 	done
 	#
 	# Verify HEAD.
@@ -221,23 +221,6 @@ bisect_start() {
 	trap '-' 0
 }
 
-bisect_write() {
-	state="$1"
-	rev="$2"
-	nolog="$3"
-	case "$state" in
-		"$TERM_BAD")
-			tag="$state" ;;
-		"$TERM_GOOD"|skip)
-			tag="$state"-"$rev" ;;
-		*)
-			die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
-	esac
-	git update-ref "refs/bisect/$tag" "$rev" || exit
-	echo "# $state: $(git show-branch $rev)" >>"$GIT_DIR/BISECT_LOG"
-	test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -263,7 +246,7 @@ bisect_state() {
 	1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
 		rev=$(git rev-parse --verify $(bisect_head)) ||
 			die "$(gettext "Bad rev input: $(bisect_head)")"
-		bisect_write "$state" "$rev"
+		git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
 		git bisect--helper --check-expected-revs "$rev" ;;
 	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
 		shift
@@ -276,7 +259,7 @@ bisect_state() {
 		done
 		for rev in $hash_list
 		do
-			bisect_write "$state" "$rev"
+			git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
 		done
 		git bisect--helper --check-expected-revs $hash_list ;;
 	*,"$TERM_BAD")
@@ -413,7 +396,7 @@ bisect_replay () {
 			cmd="bisect_start $rev"
 			eval "$cmd" ;;
 		"$TERM_GOOD"|"$TERM_BAD"|skip)
-			bisect_write "$command" "$rev" ;;
+			git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
 		terms)
 			bisect_terms $rev ;;
 		*)
-- 
2.9.0
Pranit Bauva· Jul 6, 2016, 20:25 UTC · re: Pranit Bauva · lore

[PATCH v4 0/6] convert various shell functions in git-bisect to C

Some minor nits. Previous version can be found here[1].
[1]: http://thread.gmane.org/gmane.comp.version-control.git/298263
The inter-diff is :
Show changes to builtin/bisect--helper.c +15 −17
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index e946ba9..c2f3cee 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -31,18 +31,16 @@ struct bisect_terms {
 	struct strbuf term_bad;
 };
 
-static int bisect_terms_init(struct bisect_terms *term)
+static void bisect_terms_init(struct bisect_terms *terms)
 {
-	strbuf_init(&term->term_good, 0);
-	strbuf_init(&term->term_bad, 0);
-	return 0;
+	strbuf_init(&terms->term_good, 0);
+	strbuf_init(&terms->term_bad, 0);
 }
 
-static int bisect_terms_release(struct bisect_terms *term)
+static void bisect_terms_release(struct bisect_terms *terms)
 {
-	strbuf_release(&term->term_good);
-	strbuf_release(&term->term_good);
-	return 0;
+	strbuf_release(&terms->term_good);
+	strbuf_release(&terms->term_bad);
 }
 
 /*
@@ -209,7 +207,7 @@ static int check_expected_revs(const char **revs, int rev_nr)
 }
 
 static int bisect_write(const char *state, const char *rev,
-			const struct bisect_terms *term, int nolog)
+			const struct bisect_terms *terms, int nolog)
 {
 	struct strbuf tag = STRBUF_INIT;
 	struct strbuf commit_name = STRBUF_INIT;
@@ -218,9 +216,9 @@ static int bisect_write(const char *state, const char *rev,
 	struct pretty_print_context pp = {0};
 	FILE *fp;
 
-	if (!strcmp(state, term->term_bad.buf))
+	if (!strcmp(state, terms->term_bad.buf))
 		strbuf_addf(&tag, "refs/bisect/%s", state);
-	else if(one_of(state, term->term_good.buf, "skip", NULL))
+	else if(one_of(state, terms->term_good.buf, "skip", NULL))
 		strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
 	else
 		return error(_("Bad bisect_write argument: %s"), state);
@@ -282,8 +280,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
 	};
-	struct bisect_terms state;
-	bisect_terms_init(&state);
+	struct bisect_terms terms;
+	bisect_terms_init(&terms);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
@@ -317,13 +315,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 4 && argc != 5)
 			die(_("--bisect-write requires either 4 or 5 arguments"));
 		nolog = (argc == 5) && !strcmp(argv[4], "nolog");
-		strbuf_addstr(&state.term_good, argv[2]);
-		strbuf_addstr(&state.term_bad, argv[3]);
-		res = bisect_write(argv[0], argv[1], &state, nolog);
+		strbuf_addstr(&terms.term_good, argv[2]);
+		strbuf_addstr(&terms.term_bad, argv[3]);
+		res = bisect_write(argv[0], argv[1], &terms, nolog);
 		break;
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
-	bisect_terms_release(&state);
+	bisect_terms_release(&terms);
 	return res;
 }


Pranit Bauva (6):
  bisect--helper: `bisect_clean_state` shell function in C
  t6030: explicitly test for bisection cleanup
  wrapper: move is_empty_file() and rename it as
    is_empty_or_missing_file()
  bisect--helper: `bisect_reset` shell function in C
  bisect--helper: `is_expected_rev` & `check_expected_revs` shell
    function in C
  bisect--helper: `bisect_write` shell function in C

 builtin/am.c                |  20 +---
 builtin/bisect--helper.c    | 220 +++++++++++++++++++++++++++++++++++++++++++-
 cache.h                     |   3 +
 git-bisect.sh               |  97 ++-----------------
 t/t6030-bisect-porcelain.sh |  17 ++++
 wrapper.c                   |  13 +++
 6 files changed, 261 insertions(+), 109 deletions(-)
-- 
2.9.0
Pranit Bauva· Jul 6, 2016, 20:25 UTC · re: Pranit Bauva · lore

[PATCH v4 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 <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 26 +++--------------------
 2 files changed, 57 insertions(+), 24 deletions(-)
Show changes to 2 files +57 −24

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 91027b0..57fd4e9 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 <bad_term> <good_term>"),
+	N_("git bisect--helper --bisect-clean-state"),
 	NULL
 };
 
@@ -78,11 +87,49 @@ 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);
+	refs_for_removal.strdup_strings = 1;
+	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 +137,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 +157,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 @@ 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.9.0
Pranit Bauva· Jul 6, 2016, 20:25 UTC · re: Pranit Bauva · lore

[PATCH v4 2/6] t6030: explicitly test for bisection cleanup

Add test to explicitly check that 'git bisect reset' is working as expected. This is already covered implicitly by the test suite.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

--- I faced this problem while converting `bisect_clean_state` and the tests where showing breakages but it wasn't clear as to where exactly are they breaking. This will patch will help in that. Also I tested the test coverage of the test suite before this patch and it covers this (I did this by purposely changing names of files in git-bisect.sh and running the test suite).

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Show changes to t/t6030-bisect-porcelain.sh +17 −0
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index e74662b..a17f7a6 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git bisect reset cleans bisection state properly' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect reset &&
+	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
+	test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
+	test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
+	test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
+	test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
+	test_path_is_missing "$GIT_DIR/head-name" &&
+	test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
+	test_path_is_missing "$GIT_DIR/BISECT_START"
+'
+
 test_done
-- 
2.9.0
Junio C Hamano· Jul 11, 2016, 19:16 UTC · re: Pranit Bauva · lore

Re: [PATCH v4 2/6] t6030: explicitly test for bisection cleanup

Pranit Bauva <pranit.bauva@gmail.com> writes:
> Add test to explicitly check that 'git bisect reset' is working as
> expected. This is already covered implicitly by the test suite.

Without fuzzy "this is not an improvement but it helps" that sounds like making excuse, the above reads much better.

Show 43 quoted lines
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>
> ---
> I faced this problem while converting `bisect_clean_state` and the tests
> where showing breakages but it wasn't clear as to where exactly are they
> breaking. This will patch  will help in that. Also I tested the test
> coverage of the test suite before this patch and it covers this (I did
> this by purposely changing names of files in git-bisect.sh and running
> the test suite).
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
>  t/t6030-bisect-porcelain.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index e74662b..a17f7a6 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'git bisect reset cleans bisection state properly' '
> +	git bisect reset &&
> +	git bisect start &&
> +	git bisect good $HASH1 &&
> +	git bisect bad $HASH4 &&
> +	git bisect reset &&
> +	test -z "$(git for-each-ref "refs/bisect/*")" &&
> +	test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
> +	test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
> +	test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
> +	test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
> +	test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
> +	test_path_is_missing "$GIT_DIR/head-name" &&
> +	test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
> +	test_path_is_missing "$GIT_DIR/BISECT_START"
> +'
> +
>  test_done
Pranit Bauva· Jul 6, 2016, 20:25 UTC · re: Pranit Bauva · lore

[PATCH v4 3/6] wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()

is_empty_file() can help to refactor a lot of code. This will be very helpful in porting "git bisect" to C.

Suggested-by: Torsten Bögershausen <tboegi@web.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/am.c | 20 ++------------------
 cache.h      |  3 +++
 wrapper.c    | 13 +++++++++++++
 3 files changed, 18 insertions(+), 18 deletions(-)
Show changes to 3 files +18 −18

builtin/am.c, cache.h, wrapper.c

diff --git a/builtin/am.c b/builtin/am.c
index 3dfe70b..6ee158f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -30,22 +30,6 @@
 #include "mailinfo.h"
 
 /**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-	struct stat st;
-
-	if (stat(filename, &st) < 0) {
-		if (errno == ENOENT)
-			return 1;
-		die_errno(_("could not stat %s"), filename);
-	}
-
-	return !st.st_size;
-}
-
-/**
  * Returns the length of the first line of msg.
  */
 static int linelen(const char *msg)
@@ -1323,7 +1307,7 @@ static int parse_mail(struct am_state *state, const char *mail)
 		goto finish;
 	}
 
-	if (is_empty_file(am_path(state, "patch"))) {
+	if (is_empty_or_missing_file(am_path(state, "patch"))) {
 		printf_ln(_("Patch is empty. Was it split wrong?"));
 		die_user_resolve(state);
 	}
@@ -1911,7 +1895,7 @@ next:
 		resume = 0;
 	}
 
-	if (!is_empty_file(am_path(state, "rewritten"))) {
+	if (!is_empty_or_missing_file(am_path(state, "rewritten"))) {
 		assert(state->rebasing);
 		copy_notes_for_rebase(state);
 		run_post_rewrite_hook(state);
diff --git a/cache.h b/cache.h
index 6049f86..91e2f81 100644
--- a/cache.h
+++ b/cache.h
@@ -1870,4 +1870,7 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+extern int is_empty_or_missing_file(const char *filename);
+
 #endif /* CACHE_H */
diff --git a/wrapper.c b/wrapper.c
index 5dc4e15..e70e4d1 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -696,3 +696,16 @@ void sleep_millisec(int millisec)
 {
 	poll(NULL, 0, millisec);
 }
+
+int is_empty_or_missing_file(const char *filename)
+{
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		if (errno == ENOENT)
+			return 1;
+		die_errno(_("could not stat %s"), filename);
+	}
+
+	return !st.st_size;
+}
-- 
2.9.0
Pranit Bauva· Jul 6, 2016, 20:25 UTC · re: Pranit Bauva · lore

[PATCH v4 4/6] bisect--helper: `bisect_reset` shell function in C

Reimplement `bisect_reset` shell function in C and add a `--bisect-reset` subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `bisect_reset` subcommand is a temporary measure to port shell functions to C so as to use the existing test suite. As more functions are ported, this subcommand would be retired and will be called by some other method.

Note: --bisect-clean-state subcommand has not been retired as there are
still a function namely `bisect_start()` which still uses this
subcommand.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 28 ++--------------------------
 2 files changed, 48 insertions(+), 27 deletions(-)
Show changes to 2 files +48 −27

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 57fd4e9..96f6b65 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,6 +4,8 @@
 #include "bisect.h"
 #include "refs.h"
 #include "dir.h"
+#include "argv-array.h"
+#include "run-command.h"
 
 static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -13,11 +15,13 @@ 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 GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 
 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"),
+	N_("git bisect--helper --bisect-reset [<commit>]"),
 	NULL
 };
 
@@ -124,12 +128,47 @@ static int bisect_clean_state(void)
 	return result;
 }
 
+static int bisect_reset(const char *commit)
+{
+	struct strbuf branch = STRBUF_INIT;
+
+	if (!commit) {
+		if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) {
+			printf("We are not bisecting.\n");
+			return 0;
+		}
+		strbuf_rtrim(&branch);
+	} else {
+		struct object_id oid;
+		if (get_oid(commit, &oid))
+			return error(_("'%s' is not a valid commit"), commit);
+		strbuf_addstr(&branch, commit);
+	}
+
+	if (!file_exists(git_path_bisect_head())) {
+		struct argv_array argv = ARGV_ARRAY_INIT;
+		argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
+		if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
+			error(_("Could not check out original HEAD '%s'. Try"
+				"'git bisect reset <commit>'."), branch.buf);
+			strbuf_release(&branch);
+			argv_array_clear(&argv);
+			return -1;
+		}
+		argv_array_clear(&argv);
+	}
+
+	strbuf_release(&branch);
+	return bisect_clean_state();
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
-		BISECT_CLEAN_STATE
+		BISECT_CLEAN_STATE,
+		BISECT_RESET
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -139,6 +178,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 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_CMDMODE(0, "bisect-reset", &cmdmode,
+			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -161,6 +202,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 0)
 			die(_("--bisect-clean-state requires no arguments"));
 		return bisect_clean_state();
+	case BISECT_RESET:
+		if (argc > 1)
+			die(_("--bisect-reset requires either zero or one arguments"));
+		return bisect_reset(argc ? argv[0] : NULL);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index bbc57d2..18580b7 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -409,35 +409,11 @@ bisect_visualize() {
 	eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
 }
 
-bisect_reset() {
-	test -s "$GIT_DIR/BISECT_START" || {
-		gettextln "We are not bisecting."
-		return
-	}
-	case "$#" in
-	0) branch=$(cat "$GIT_DIR/BISECT_START") ;;
-	1) git rev-parse --quiet --verify "$1^{commit}" >/dev/null || {
-			invalid="$1"
-			die "$(eval_gettext "'\$invalid' is not a valid commit")"
-		}
-		branch="$1" ;;
-	*)
-		usage ;;
-	esac
-
-	if ! test -f "$GIT_DIR/BISECT_HEAD" && ! git checkout "$branch" --
-	then
-		die "$(eval_gettext "Could not check out original HEAD '\$branch'.
-Try 'git bisect reset <commit>'.")"
-	fi
-	git bisect--helper --bisect-clean-state || exit
-}
-
 bisect_replay () {
 	file="$1"
 	test "$#" -eq 1 || die "$(gettext "No logfile given")"
 	test -r "$file" || die "$(eval_gettext "cannot read \$file for replaying")"
-	bisect_reset
+	git bisect--helper --bisect-reset || exit
 	while read git bisect command rev
 	do
 		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
@@ -627,7 +603,7 @@ case "$#" in
 	visualize|view)
 		bisect_visualize "$@" ;;
 	reset)
-		bisect_reset "$@" ;;
+		git bisect--helper --bisect-reset "$@" ;;
 	replay)
 		bisect_replay "$@" ;;
 	log)
-- 
2.9.0
Pranit Bauva· Jul 6, 2016, 20:25 UTC · re: Pranit Bauva · lore

[PATCH v4 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

Reimplement `is_expected_rev` & `check_expected_revs` shell function in C and add a `--check-expected-revs` subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `--check-expected-revs` subcommand is a temporary measure to port shell functions to C so as to use the existing test suite. As more functions are ported, this subcommand would be retired and will be called by some other method.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 33 ++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 20 ++------------------
 2 files changed, 34 insertions(+), 19 deletions(-)
Show changes to 2 files +34 −19

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 96f6b65..96de65a 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -162,13 +162,40 @@ static int bisect_reset(const char *commit)
 	return bisect_clean_state();
 }
 
+static int is_expected_rev(const char *expected_hex)
+{
+	struct strbuf actual_hex = STRBUF_INIT;
+	int res = 0;
+	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 0) {
+		strbuf_trim(&actual_hex);
+		res = !strcmp(actual_hex.buf, expected_hex);
+	}
+	strbuf_release(&actual_hex);
+	return res;
+}
+
+static int check_expected_revs(const char **revs, int rev_nr)
+{
+	int i;
+
+	for (i = 0; i < rev_nr; i++) {
+		if (!is_expected_rev(revs[i])) {
+			remove_path(git_path_bisect_ancestors_ok());
+			remove_path(git_path_bisect_expected_rev());
+			return 0;
+		}
+	}
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
 		BISECT_CLEAN_STATE,
-		BISECT_RESET
+		BISECT_RESET,
+		CHECK_EXPECTED_REVS
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -180,6 +207,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
 			 N_("reset the bisection state"), BISECT_RESET),
+		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
+			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -206,6 +235,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc > 1)
 			die(_("--bisect-reset requires either zero or one arguments"));
 		return bisect_reset(argc ? argv[0] : NULL);
+	case CHECK_EXPECTED_REVS:
+		return check_expected_revs(argv, argc);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 18580b7..4f6545e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -238,22 +238,6 @@ bisect_write() {
 	test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
 }
 
-is_expected_rev() {
-	test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
-	test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
-}
-
-check_expected_revs() {
-	for _rev in "$@"; do
-		if ! is_expected_rev "$_rev"
-		then
-			rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
-			rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
-			return
-		fi
-	done
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -280,7 +264,7 @@ bisect_state() {
 		rev=$(git rev-parse --verify $(bisect_head)) ||
 			die "$(gettext "Bad rev input: $(bisect_head)")"
 		bisect_write "$state" "$rev"
-		check_expected_revs "$rev" ;;
+		git bisect--helper --check-expected-revs "$rev" ;;
 	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
 		shift
 		hash_list=''
@@ -294,7 +278,7 @@ bisect_state() {
 		do
 			bisect_write "$state" "$rev"
 		done
-		check_expected_revs $hash_list ;;
+		git bisect--helper --check-expected-revs $hash_list ;;
 	*,"$TERM_BAD")
 		die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
 	*)
-- 
2.9.0
Pranit Bauva· Jul 6, 2016, 20:25 UTC · re: Pranit Bauva · lore

[PATCH v4 6/6] bisect--helper: `bisect_write` shell function in C

Reimplement the `bisect_write` shell function in C and add a `bisect-write` subcommand to `git bisect--helper` to call it from git-bisect.sh

Using `--bisect-write` subcommand is a temporary measure to port shell function in C so as to use the existing test suite. As more functions are ported, this subcommand will be retired and will be called by some other methods.

Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
from the global shell script thus we need to pass it to the subcommand
using the arguments. We then store them in a struct bisect_terms and
pass the memory address around functions.

This patch also introduces new methods namely bisect_state_init() and bisect_terms_release() for easy memory management for the struct bisect_terms.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 97 ++++++++++++++++++++++++++++++++++++++++++++----
 git-bisect.sh            | 25 ++-----------
 2 files changed, 94 insertions(+), 28 deletions(-)
Show changes to 2 files +94 −28

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 96de65a..c2f3cee 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,9 +22,27 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	N_("git bisect--helper --bisect-clean-state"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
+	N_("git bisect--helper --bisect-write <state> <revision> <TERM_GOOD> <TERM_BAD> [<nolog>]"),
 	NULL
 };
 
+struct bisect_terms {
+	struct strbuf term_good;
+	struct strbuf term_bad;
+};
+
+static void bisect_terms_init(struct bisect_terms *terms)
+{
+	strbuf_init(&terms->term_good, 0);
+	strbuf_init(&terms->term_bad, 0);
+}
+
+static void bisect_terms_release(struct bisect_terms *terms)
+{
+	strbuf_release(&terms->term_good);
+	strbuf_release(&terms->term_bad);
+}
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -188,6 +206,52 @@ static int check_expected_revs(const char **revs, int rev_nr)
 	return 0;
 }
 
+static int bisect_write(const char *state, const char *rev,
+			const struct bisect_terms *terms, int nolog)
+{
+	struct strbuf tag = STRBUF_INIT;
+	struct strbuf commit_name = STRBUF_INIT;
+	struct object_id oid;
+	struct commit *commit;
+	struct pretty_print_context pp = {0};
+	FILE *fp;
+
+	if (!strcmp(state, terms->term_bad.buf))
+		strbuf_addf(&tag, "refs/bisect/%s", state);
+	else if(one_of(state, terms->term_good.buf, "skip", NULL))
+		strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
+	else
+		return error(_("Bad bisect_write argument: %s"), state);
+
+	if (get_oid(rev, &oid)) {
+		strbuf_release(&tag);
+		return error(_("couldn't get the oid of the rev '%s'"), rev);
+	}
+
+	if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
+		       UPDATE_REFS_MSG_ON_ERR)) {
+		strbuf_release(&tag);
+		return -1;
+	}
+	strbuf_release(&tag);
+
+	fp = fopen(git_path_bisect_log(), "a");
+	if (!fp)
+		return error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
+
+	commit = lookup_commit_reference(oid.hash);
+	format_commit_message(commit, "%s", &commit_name, &pp);
+	fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
+		commit_name.buf);
+	strbuf_release(&commit_name);
+
+	if (!nolog)
+		fprintf(fp, "git bisect %s %s\n", state, rev);
+
+	fclose(fp);
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -195,9 +259,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		WRITE_TERMS,
 		BISECT_CLEAN_STATE,
 		BISECT_RESET,
-		CHECK_EXPECTED_REVS
+		CHECK_EXPECTED_REVS,
+		BISECT_WRITE
 	} cmdmode = 0;
-	int no_checkout = 0;
+	int no_checkout = 0, res = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
@@ -209,10 +274,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
 			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
+		OPT_CMDMODE(0, "bisect-write", &cmdmode,
+			 N_("write out the bisection state in BISECT_LOG"), BISECT_WRITE),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
 	};
+	struct bisect_terms terms;
+	bisect_terms_init(&terms);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
@@ -221,24 +290,38 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	switch (cmdmode) {
+	int nolog;
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
 	case WRITE_TERMS:
 		if (argc != 2)
 			die(_("--write-terms requires two arguments"));
-		return write_terms(argv[0], argv[1]);
+		res = write_terms(argv[0], argv[1]);
+		break;
 	case BISECT_CLEAN_STATE:
 		if (argc != 0)
 			die(_("--bisect-clean-state requires no arguments"));
-		return bisect_clean_state();
+		res = bisect_clean_state();
+		break;
 	case BISECT_RESET:
 		if (argc > 1)
 			die(_("--bisect-reset requires either zero or one arguments"));
-		return bisect_reset(argc ? argv[0] : NULL);
+		res = bisect_reset(argc ? argv[0] : NULL);
+		break;
 	case CHECK_EXPECTED_REVS:
-		return check_expected_revs(argv, argc);
+		res = check_expected_revs(argv, argc);
+		break;
+	case BISECT_WRITE:
+		if (argc != 4 && argc != 5)
+			die(_("--bisect-write requires either 4 or 5 arguments"));
+		nolog = (argc == 5) && !strcmp(argv[4], "nolog");
+		strbuf_addstr(&terms.term_good, argv[2]);
+		strbuf_addstr(&terms.term_bad, argv[3]);
+		res = bisect_write(argv[0], argv[1], &terms, nolog);
+		break;
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
-	return 0;
+	bisect_terms_release(&terms);
+	return res;
 }
diff --git a/git-bisect.sh b/git-bisect.sh
index 4f6545e..b9896a4 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -145,7 +145,7 @@ bisect_start() {
 		0) state=$TERM_BAD ; bad_seen=1 ;;
 		*) state=$TERM_GOOD ;;
 		esac
-		eval="$eval bisect_write '$state' '$rev' 'nolog' &&"
+		eval="$eval git bisect--helper --bisect-write '$state' '$rev' '$TERM_GOOD' '$TERM_BAD' 'nolog' &&"
 	done
 	#
 	# Verify HEAD.
@@ -221,23 +221,6 @@ bisect_start() {
 	trap '-' 0
 }
 
-bisect_write() {
-	state="$1"
-	rev="$2"
-	nolog="$3"
-	case "$state" in
-		"$TERM_BAD")
-			tag="$state" ;;
-		"$TERM_GOOD"|skip)
-			tag="$state"-"$rev" ;;
-		*)
-			die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
-	esac
-	git update-ref "refs/bisect/$tag" "$rev" || exit
-	echo "# $state: $(git show-branch $rev)" >>"$GIT_DIR/BISECT_LOG"
-	test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -263,7 +246,7 @@ bisect_state() {
 	1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
 		rev=$(git rev-parse --verify $(bisect_head)) ||
 			die "$(gettext "Bad rev input: $(bisect_head)")"
-		bisect_write "$state" "$rev"
+		git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
 		git bisect--helper --check-expected-revs "$rev" ;;
 	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
 		shift
@@ -276,7 +259,7 @@ bisect_state() {
 		done
 		for rev in $hash_list
 		do
-			bisect_write "$state" "$rev"
+			git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
 		done
 		git bisect--helper --check-expected-revs $hash_list ;;
 	*,"$TERM_BAD")
@@ -413,7 +396,7 @@ bisect_replay () {
 			cmd="bisect_start $rev"
 			eval "$cmd" ;;
 		"$TERM_GOOD"|"$TERM_BAD"|skip)
-			bisect_write "$command" "$rev" ;;
+			git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
 		terms)
 			bisect_terms $rev ;;
 		*)
-- 
2.9.0
Junio C Hamano· Jul 11, 2016, 19:19 UTC · re: Pranit Bauva · lore

Re: [PATCH v4 0/6] convert various shell functions in git-bisect to C

Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 25 quoted lines
> Some minor nits. Previous version can be found here[1].
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/298263
>
> The inter-diff is :
> ...
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> ...
> Pranit Bauva (6):
>   bisect--helper: `bisect_clean_state` shell function in C
>   t6030: explicitly test for bisection cleanup
>   wrapper: move is_empty_file() and rename it as
>     is_empty_or_missing_file()
>   bisect--helper: `bisect_reset` shell function in C
>   bisect--helper: `is_expected_rev` & `check_expected_revs` shell
>     function in C
>   bisect--helper: `bisect_write` shell function in C
>
>  builtin/am.c                |  20 +---
>  builtin/bisect--helper.c    | 220 +++++++++++++++++++++++++++++++++++++++++++-
>  cache.h                     |   3 +
>  git-bisect.sh               |  97 ++-----------------
>  t/t6030-bisect-porcelain.sh |  17 ++++
>  wrapper.c                   |  13 +++
>  6 files changed, 261 insertions(+), 109 deletions(-)
Will replace.  Thanks.

← back to recent threads