[PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C

[PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C

[Pranit Bauva]

Reimplement `is_expected_rev` shell function in C. This will further be
called from `check_expected_revs` function. This is a quite small
function thus subcommand facility is redundant.

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

 builtin/bisect--helper.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 4153e8a..06bc9b8 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -160,6 +160,20 @@ 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;
+
+	if (!file_exists(git_path_bisect_expected_rev()))
+		return 0;
+
+	if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))
+		return 0;
+
+	strbuf_trim(&actual_hex);
+	return !strcmp(actual_hex.buf, expected_hex);
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
-- 
2.8.3

[PATCH 2/2] bisect--helper: `check_expected_revs` shell function in C

[Pranit Bauva]

Reimplement the `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 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 method namely `bisect_state()`.

Note: Previously is_expected_rev() was converted but not added as a
subcommand. Now since we have converted check_expected_rev() and we are
calling is_expected_rev()'s C implementation, we can safely delete the
shell implementation.

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 | 21 ++++++++++++++++++++-
 git-bisect.sh            | 20 ++------------------
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 06bc9b8..500efd5 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -174,13 +174,28 @@ static int is_expected_rev(const char *expected_hex)
 	return !strcmp(actual_hex.buf, expected_hex);
 }
 
+static int check_expected_revs(const char **revs, int no)
+{
+	int i;
+
+	for (i = 0; i < no; 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[] = {
@@ -192,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()
@@ -218,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.8.3

Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C

[Eric Sunshine]

On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement `is_expected_rev` shell function in C. This will further be
> called from `check_expected_revs` function. This is a quite small
> function thus subcommand facility is redundant.

This patch should be squashed into patch 2/2, as it is otherwise
pointless without that patch, and merely adds dead code.

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -160,6 +160,20 @@ int bisect_reset(const char *commit)
> +static int is_expected_rev(const char *expected_hex)
> +{
> +       struct strbuf actual_hex = STRBUF_INIT;
> +
> +       if (!file_exists(git_path_bisect_expected_rev()))
> +               return 0;

Invoking file_exists() seems unnecessarily redundant when you can
discern effectively the same by checking the return value of
strbuf_read_file() below. I'd drop the file_exists() check altogether.

> +       if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))
> +               return 0;

What exactly is this trying to do? Considering that strbuf_read_file()
returns -1 upon error, otherwise the number of bytes read, if I'm
reading this correctly, is_expected_rev() returns false if
strbuf_read_file() encounters an error (which is fine) but also when
it successfully reads the file and its content length is non-zero
(which is very odd).

> +       strbuf_trim(&actual_hex);
> +       return !strcmp(actual_hex.buf, expected_hex);

Thus, it only ever gets to this point if the file exists but is empty,
which is very unlikely to match 'expected_hex'. I could understand it
if you checked the result of strbuf_read_file() with <0 or even <=0,
but the current code doesn't make sense to me.

Am I misunderstanding?

> +}

Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C

[Eric Sunshine]

On Thu, Jun 9, 2016 at 5:33 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> +       strbuf_trim(&actual_hex);
>> +       return !strcmp(actual_hex.buf, expected_hex);
>
> Thus, it only ever gets to this point if the file exists but is empty,
> which is very unlikely to match 'expected_hex'. I could understand it
> if you checked the result of strbuf_read_file() with <0 or even <=0,
> but the current code doesn't make sense to me.

By the way, this is also leaking strbuf 'actual_hex'.

Re: [PATCH 2/2] bisect--helper: `check_expected_revs` shell function in C

[Eric Sunshine]

On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement the `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
> index 06bc9b8..500efd5 100644
> @@ -174,13 +174,28 @@ static int is_expected_rev(const char *expected_hex)
> +static int check_expected_revs(const char **revs, int no)

In this codebase, it's more common to name this 'nr' rather than 'no'.
'revs_nr' would also be a good name.

> +{
> +       int i;
> +
> +       for (i = 0; i < no; 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;
> +}

Re: [PATCH 2/2] bisect--helper: `check_expected_revs` shell function in C

[Pranit Bauva]

Hey Eric,

On Fri, Jun 10, 2016 at 3:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement the `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
>> index 06bc9b8..500efd5 100644
>> @@ -174,13 +174,28 @@ static int is_expected_rev(const char *expected_hex)
>> +static int check_expected_revs(const char **revs, int no)
>
> In this codebase, it's more common to name this 'nr' rather than 'no'.
> 'revs_nr' would also be a good name.

Thanks. I will use `revs_nr`.

Regards,
Pranit Bauva

Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C

[Pranit Bauva]

Hey Eric,

On Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement `is_expected_rev` shell function in C. This will further be
>> called from `check_expected_revs` function. This is a quite small
>> function thus subcommand facility is redundant.
>
> This patch should be squashed into patch 2/2, as it is otherwise
> pointless without that patch, and merely adds dead code.

Sure I will squash and will explain it in the commit message.

>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -160,6 +160,20 @@ int bisect_reset(const char *commit)
>> +static int is_expected_rev(const char *expected_hex)
>> +{
>> +       struct strbuf actual_hex = STRBUF_INIT;
>> +
>> +       if (!file_exists(git_path_bisect_expected_rev()))
>> +               return 0;
>
> Invoking file_exists() seems unnecessarily redundant when you can
> discern effectively the same by checking the return value of
> strbuf_read_file() below. I'd drop the file_exists() check altogether.

I wanted to imitate the code. But I guess it would actually be better
if I drop this file_exists().

>> +       if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))
>> +               return 0;
>
> What exactly is this trying to do? Considering that strbuf_read_file()
> returns -1 upon error, otherwise the number of bytes read, if I'm
> reading this correctly, is_expected_rev() returns false if
> strbuf_read_file() encounters an error (which is fine) but also when
> it successfully reads the file and its content length is non-zero
> (which is very odd).
>
>> +       strbuf_trim(&actual_hex);
>> +       return !strcmp(actual_hex.buf, expected_hex);
>
> Thus, it only ever gets to this point if the file exists but is empty,
> which is very unlikely to match 'expected_hex'. I could understand it
> if you checked the result of strbuf_read_file() with <0 or even <=0,
> but the current code doesn't make sense to me.
>
> Am I misunderstanding?


Definitely not. Thanks for pointing it out. :) It went off my head
that strbuf_read_file returns the bytes it reads. Also the code
comment regarding strbuf_read_file does not mention it which probably
misguided me. I should also send a fixing patch so that someone else
does not fall into this like I did.

I will also release the 'actual_hex'.

Thanks for your comments!

Regards,
Pranit Bauva

Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C

[Eric Sunshine]

On Fri, Jun 10, 2016 at 9:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> Reimplement `is_expected_rev` shell function in C. This will further be
>>> called from `check_expected_revs` function. This is a quite small
>>> function thus subcommand facility is redundant.
>>
>> This patch should be squashed into patch 2/2, as it is otherwise
>> pointless without that patch, and merely adds dead code.
>
> Sure I will squash and will explain it in the commit message.

Explain what in the commit message? If anything, I'd expect the commit
message to shrink since you won't need to explain anymore that this
function is split out.

>>> +       if (!file_exists(git_path_bisect_expected_rev()))
>>> +               return 0;
>>
>> Invoking file_exists() seems unnecessarily redundant when you can
>> discern effectively the same by checking the return value of
>> strbuf_read_file() below. I'd drop the file_exists() check altogether.
>
> I wanted to imitate the code. But I guess it would actually be better
> if I drop this file_exists().

There is a bit of a lesson to be learned by this example. While it's
true that the C conversion should retain the behavior of the original
shell code, that does not mean blindly mirroring the implementation
line for line is a good idea. A couple things to take into
consideration:

There are idiomatic ways of doing things in each language. What is
idiomatic in shell is not necessarily so in C. The C conversion should
employ C idioms and flow in a way which is natural for C code.

Consider what the original shell code is doing at a higher level than
merely by reading it line-by-line. In the case in question, the code
is:

    test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
    test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")

While it's true that it's asking "does the file exist and is its value
the same as $1", the 'test -f' avoids a "file not found" error from
the $(cat ...) invocation. Since the return value of
strbuf_read_file() effectively encapsulates the "does the file exist"
check, a separate check isn't really needed.

>>> +       if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))
>>> +               return 0;
>>
>> What exactly is this trying to do? Considering that strbuf_read_file()
>> returns -1 upon error, otherwise the number of bytes read, if I'm
>> reading this correctly, is_expected_rev() returns false if
>> strbuf_read_file() encounters an error (which is fine) but also when
>> it successfully reads the file and its content length is non-zero
>> (which is very odd).
>>
>>> +       strbuf_trim(&actual_hex);
>>> +       return !strcmp(actual_hex.buf, expected_hex);
>>
>> Thus, it only ever gets to this point if the file exists but is empty,
>> which is very unlikely to match 'expected_hex'. I could understand it
>> if you checked the result of strbuf_read_file() with <0 or even <=0,
>> but the current code doesn't make sense to me.
>>
>> Am I misunderstanding?
>
> Definitely not. Thanks for pointing it out. :) It went off my head
> that strbuf_read_file returns the bytes it reads. Also the code
> comment regarding strbuf_read_file does not mention it which probably
> misguided me. I should also send a fixing patch so that someone else
> does not fall into this like I did.

Out of curiosity, did the test suite pass with this patch applied?
This is such an egregious bug that it's hard to imagine the tests
passing, but if they did, then that may be a good indication that
coverage is too sparse and ought to be improved.

Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C

[Pranit Bauva]

Hey Eric,

On Sat, Jun 11, 2016 at 12:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Jun 10, 2016 at 9:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> Reimplement `is_expected_rev` shell function in C. This will further be
>>>> called from `check_expected_revs` function. This is a quite small
>>>> function thus subcommand facility is redundant.
>>>
>>> This patch should be squashed into patch 2/2, as it is otherwise
>>> pointless without that patch, and merely adds dead code.
>>
>> Sure I will squash and will explain it in the commit message.
>
> Explain what in the commit message? If anything, I'd expect the commit
> message to shrink since you won't need to explain anymore that this
> function is split out.

Yes I would remove the part where it is explained that this function
is split out. I will just explain that 2 functions are converted in 1
commit.

>>>> +       if (!file_exists(git_path_bisect_expected_rev()))
>>>> +               return 0;
>>>
>>> Invoking file_exists() seems unnecessarily redundant when you can
>>> discern effectively the same by checking the return value of
>>> strbuf_read_file() below. I'd drop the file_exists() check altogether.
>>
>> I wanted to imitate the code. But I guess it would actually be better
>> if I drop this file_exists().
>
> There is a bit of a lesson to be learned by this example. While it's
> true that the C conversion should retain the behavior of the original
> shell code, that does not mean blindly mirroring the implementation
> line for line is a good idea. A couple things to take into
> consideration:
>
> There are idiomatic ways of doing things in each language. What is
> idiomatic in shell is not necessarily so in C. The C conversion should
> employ C idioms and flow in a way which is natural for C code.
>
> Consider what the original shell code is doing at a higher level than
> merely by reading it line-by-line. In the case in question, the code
> is:
>
>     test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
>     test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
>
> While it's true that it's asking "does the file exist and is its value
> the same as $1", the 'test -f' avoids a "file not found" error from
> the $(cat ...) invocation. Since the return value of
> strbuf_read_file() effectively encapsulates the "does the file exist"
> check, a separate check isn't really needed.

True. I will keep this in mind.

>>>> +       if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))
>>>> +               return 0;
>>>
>>> What exactly is this trying to do? Considering that strbuf_read_file()
>>> returns -1 upon error, otherwise the number of bytes read, if I'm
>>> reading this correctly, is_expected_rev() returns false if
>>> strbuf_read_file() encounters an error (which is fine) but also when
>>> it successfully reads the file and its content length is non-zero
>>> (which is very odd).
>>>
>>>> +       strbuf_trim(&actual_hex);
>>>> +       return !strcmp(actual_hex.buf, expected_hex);
>>>
>>> Thus, it only ever gets to this point if the file exists but is empty,
>>> which is very unlikely to match 'expected_hex'. I could understand it
>>> if you checked the result of strbuf_read_file() with <0 or even <=0,
>>> but the current code doesn't make sense to me.
>>>
>>> Am I misunderstanding?
>>
>> Definitely not. Thanks for pointing it out. :) It went off my head
>> that strbuf_read_file returns the bytes it reads. Also the code
>> comment regarding strbuf_read_file does not mention it which probably
>> misguided me. I should also send a fixing patch so that someone else
>> does not fall into this like I did.
>
> Out of curiosity, did the test suite pass with this patch applied?
> This is such an egregious bug that it's hard to imagine the tests
> passing, but if they did, then that may be a good indication that
> coverage is too sparse and ought to be improved.

Yes the test suite passed perfectly. I have inculcated the habit of
running the whole test suite before sending patches. Yes some parts of
a test suite seem to be missing. How about I do it in the end? By this
I won't have to setup yet another coverage tool for shell script. I
can use the coverage tool by GNU to test the coverage after bisect is
a C code. Till that time the patches can reside in the pu branch.

Regards,
Pranit Bauva