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

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

[Pranit Bauva]

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

Using `--bisect-log` subcommand is a temporary 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 some
other method.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
This can be applied on the pb/bisect branch

 builtin/bisect--helper.c | 22 +++++++++++++++++++++-
 git-bisect.sh            |  7 +------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 2b21c02..87764fe 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -7,6 +7,7 @@
 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-log"),
 	NULL
 };
 
@@ -79,11 +80,26 @@ int write_terms(const char *bad, const char *good)
 	strbuf_release(&content);
 	return (res < 0) ? -1 : 0;
 }
+
+int bisect_log(void)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	if (strbuf_read_file(&buf, ".git/BISECT_LOG", 256) < 0)
+		return error(_("We are not bisecting."));
+
+	printf("%s", buf.buf);
+	strbuf_release(&buf);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
-		WRITE_TERMS
+		WRITE_TERMS,
+		BISECT_LOG
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -91,6 +107,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-log", &cmdmode,
+			 N_("output contents of .git/BISECT_LOG"), BISECT_LOG),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -109,6 +127,8 @@ 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_LOG:
+		return bisect_log();
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 2dd7ec5..612a9c5 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -542,11 +542,6 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 	done
 }
 
-bisect_log () {
-	test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")"
-	cat "$GIT_DIR/BISECT_LOG"
-}
-
 get_terms () {
 	if test -s "$GIT_DIR/BISECT_TERMS"
 	then
@@ -651,7 +646,7 @@ case "$#" in
 	replay)
 		bisect_replay "$@" ;;
 	log)
-		bisect_log ;;
+		git bisect--helper --bisect-log ;;
 	run)
 		bisect_run "$@" ;;
 	terms)
-- 
2.8.2

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

[Pranit Bauva]

Reimplement the `bisect_voc` shell function in C. This is a too small
function to be called as a subcommand though the working of this
function has been tested by calling it as a subcommand.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---
The PR for testing of this function by the subcommand approach can be
found at:
https://github.com/pranitbauva1997/git/pull/5

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 87764fe..455f1cb 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -94,6 +94,16 @@ int bisect_log(void)
 	return 0;
 }
 
+int bisect_voc(const char *term)
+{
+	if (!strcmp(term, "bad"))
+		printf("bad|new\n");
+	if (!strcmp(term, "good"))
+		printf("good|old\n");
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
-- 
2.8.2

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

[Pranit Bauva]

On Sat, May 14, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement the `bisect_voc` shell function in C. This is a too small
> function to be called as a subcommand though the working of this
> function has been tested by calling it as a subcommand.
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Pranit Bauva <pranit.bauva@gmail.com>

I missed this. It should be
     "Mentored-by: Christian Couder <chriscool@tuxfamily.org>"

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>
> ---
> The PR for testing of this function by the subcommand approach can be
> found at:
> https://github.com/pranitbauva1997/git/pull/5
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
>  builtin/bisect--helper.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 87764fe..455f1cb 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -94,6 +94,16 @@ int bisect_log(void)
>         return 0;
>  }
>
> +int bisect_voc(const char *term)
> +{
> +       if (!strcmp(term, "bad"))
> +               printf("bad|new\n");
> +       if (!strcmp(term, "good"))
> +               printf("good|old\n");
> +
> +       return 0;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>         enum {
> --
> 2.8.2
>

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

[Johannes Schindelin]

Hi Pranit,

On Sat, 14 May 2016, Pranit Bauva wrote:

> Reimplement the `bisect_voc` shell function in C. This is a too small
> function to be called as a subcommand though the working of this
> function has been tested by calling it as a subcommand.

This leaves me puzzled as to what this patch is supposed to do. Maybe
rename this function to have a more intuitive name, and then throw in a
patch that makes use of the function?

Ciao,
Dscho

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

[Eric Sunshine]

On Fri, May 13, 2016 at 4:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> bisect--helper: `bisect_log` shell function in C

Do you need to insert "rewrite" or "reimplement" in the subject?

> Reimplement the `bisect_log` shell function in C and add a
> `--bisect-log` subcommand to `git bisect--helper` to call it from
> git-bisect.sh .
>
> Using `--bisect-log` subcommand is a temporary 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 some
> other method.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -79,11 +80,26 @@ int write_terms(const char *bad, const char *good)
> +int bisect_log(void)

s/^/static/

> +{
> +       struct strbuf buf = STRBUF_INIT;
> +
> +       if (strbuf_read_file(&buf, ".git/BISECT_LOG", 256) < 0)

As mentioned in my review of the "write-terms" rewrite, hardcoding
".git/" here is wrong for a variety of reasons. See get_git_dir(),
get_git_common_dir(), etc. in cache.h instead.

> +               return error(_("We are not bisecting."));
> +
> +       printf("%s", buf.buf);
> +       strbuf_release(&buf);
> +
> +       return 0;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> @@ -109,6 +127,8 @@ 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_LOG:

Shouldn't you be die()ing here with an appropriate error message if
argc is not 0?

> +               return bisect_log();
>         default:
>                 die("BUG: unknown subcommand '%d'", cmdmode);
>         }

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

[Matthieu Moy]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> +int bisect_voc(const char *term)
> +{
> +	if (!strcmp(term, "bad"))
> +		printf("bad|new\n");
> +	if (!strcmp(term, "good"))
> +		printf("good|old\n");

If you meant to use this as a helper command, then the implementation is
right, but you're not doing that.

If you write the function because one day you'll be calling it from C,
then:

1) First, I'd wait for this "one day" to happen. In general, write code
   when you need it, don't write it ahead of time. Currently, you have
   dead and untested code (I know, *you* have tested it, but it's still
   "untested" as far as git.git is concerned). Dead code may bother
   people reading the code (one would not understand why it's there),
   and untested code means it may break later without anyone noticing.

2) Second, you'd need to return the string, not print it. You'll
   typically use it like this:

     printf(_("You need to give me at least one %s and one %s"),
            bisect_voc(BISECT_BAD), bisect_voc(BISECT_GOOD));

   which gives one more argument for 1): once you have a use-case, you
   can design the API properly, and not blindly guess that you're going
   to need printf. Actually, writting these 2 example lines, I also
   noticed that the parameters could/should be an enum type rather than
   a string, it makes the code both more efficient and clearer.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Pranit Bauva]

Hey Johannes,

On Mon, May 16, 2016 at 12:10 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Pranit,
>
> On Sat, 14 May 2016, Pranit Bauva wrote:
>
>> Reimplement the `bisect_voc` shell function in C. This is a too small
>> function to be called as a subcommand though the working of this
>> function has been tested by calling it as a subcommand.
>
> This leaves me puzzled as to what this patch is supposed to do. Maybe
> rename this function to have a more intuitive name, and then throw in a
> patch that makes use of the function?

Are you suggesting to first have an introductory patch which will
rename the function in the shell script and then this patch which will
convert the "new" shell function to C? I can do this. I have to think
of a nice name. How does 'good_or_bad" sound?

Regards,
Pranit Bauva

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

[Pranit Bauva]

Hey Eric,

On Mon, May 16, 2016 at 1:06 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, May 13, 2016 at 4:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> bisect--helper: `bisect_log` shell function in C
>
> Do you need to insert "rewrite" or "reimplement" in the subject?
>
>> Reimplement the `bisect_log` shell function in C and add a
>> `--bisect-log` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh .
>>
>> Using `--bisect-log` subcommand is a temporary 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 some
>> other method.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -79,11 +80,26 @@ int write_terms(const char *bad, const char *good)
>> +int bisect_log(void)
>
> s/^/static/

Will include this in the re-roll

>> +{
>> +       struct strbuf buf = STRBUF_INIT;
>> +
>> +       if (strbuf_read_file(&buf, ".git/BISECT_LOG", 256) < 0)
>
> As mentioned in my review of the "write-terms" rewrite, hardcoding
> ".git/" here is wrong for a variety of reasons. See get_git_dir(),
> get_git_common_dir(), etc. in cache.h instead.

Thanks. I will have a look into this.

>> +               return error(_("We are not bisecting."));
>> +
>> +       printf("%s", buf.buf);
>> +       strbuf_release(&buf);
>> +
>> +       return 0;
>> +}
>> +
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>> @@ -109,6 +127,8 @@ 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_LOG:
>
> Shouldn't you be die()ing here with an appropriate error message if
> argc is not 0?

I think it would be better to check for argc != 0 and die
appropriately. Will include this in the re-roll.

>> +               return bisect_log();
>>         default:
>>                 die("BUG: unknown subcommand '%d'", cmdmode);
>>         }

Regards,
Pranit Bauva

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

[Pranit Bauva]

Hey Matthieu,
Sorry for the late reply. Somehow your email didn't receive my
mailbox. I got to see this mail when I was going through the gmane
archives.

Matthieu Moy <Matthieu.Moy <at> grenoble-inp.fr> writes:
  Pranit Bauva <pranit.bauva <at> gmail.com> writes:

>> +int bisect_voc(const char *term)
>> +{
>> + if (!strcmp(term, "bad"))
>> + printf("bad|new\n");
>> + if (!strcmp(term, "good"))
>> + printf("good|old\n");
>
> If you meant to use this as a helper command, then the implementation is
> right, but you're not doing that.

> If you write the function because one day you'll be calling it from C,
> then:

> 1) First, I'd wait for this "one day" to happen. In general, write code
>    when you need it, don't write it ahead of time. Currently, you have
>    dead and untested code (I know, *you* have tested it, but it's still
>    "untested" as far as git.git is concerned). Dead code may bother
>    people reading the code (one would not understand why it's there),
>    and untested code means it may break later without anyone noticing.
>

I think this function can wait then. I will include this patch when
its really required. I wanted to convert this function ASAP because it
was a really tiny one and an easy one.

> 2) Second, you'd need to return the string, not print it. You'll
>    typically use it like this:

>     printf(_("You need to give me at least one %s and one %s"),
>            bisect_voc(BISECT_BAD), bisect_voc(BISECT_GOOD));

>   which gives one more argument for 1): once you have a use-case, you
>   can design the API properly, and not blindly guess that you're going
>   to need printf. Actually, writting these 2 example lines, I also
>   noticed that the parameters could/should be an enum type rather than
>   a string, it makes the code both more efficient and clearer.
>

Okay I get it. It would be much more efficient to return a enum
because its difficult to parse text output into a C program. I hadn't
looked further into the function. Thanks for pointing it out early!

I will wait before re-rolling this patch and will do it when I convert
bisect_state().

Regards,
Pranit Bauva

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

[Johannes Schindelin]

Hi Pranit,

On Fri, 20 May 2016, Pranit Bauva wrote:

> On Mon, May 16, 2016 at 12:10 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Sat, 14 May 2016, Pranit Bauva wrote:
> >
> >> Reimplement the `bisect_voc` shell function in C. This is a too small
> >> function to be called as a subcommand though the working of this
> >> function has been tested by calling it as a subcommand.
> >
> > This leaves me puzzled as to what this patch is supposed to do. Maybe
> > rename this function to have a more intuitive name, and then throw in a
> > patch that makes use of the function?
> 
> Are you suggesting to first have an introductory patch which will
> rename the function in the shell script and then this patch which will
> convert the "new" shell function to C? I can do this. I have to think
> of a nice name. How does 'good_or_bad" sound?

For such a short function, I would simply use a more sensible name in the
C version of the function. In other words, I would not bother with an
extra patch but do it all in the same patch, describing it well in the
commit message.

Ciao,
Johannes