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  wrote:
> On Fri, May 13, 2016 at 4:02 PM, Pranit Bauva  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 
>> ---
>> 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(, ".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();
>> +
>> +   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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Fri, May 13, 2016 at 4:02 PM, Pranit Bauva  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 
> ---
> 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(, ".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();
> +
> +   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);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
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  "),
+   N_("git bisect--helper --bisect-log"),
NULL
 };
 
@@ -79,11 +80,26 @@ int write_terms(const char *bad, const char *good)
strbuf_release();
return (res < 0) ? -1 : 0;
 }
+
+int bisect_log(void)
+{
+   struct strbuf buf = STRBUF_INIT;
+
+   if (strbuf_read_file(, ".git/BISECT_LOG", 256) < 0)
+   return error(_("We are not bisecting."));
+
+   printf("%s", buf.buf);
+   strbuf_release();
+
+   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", ,
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
+   OPT_CMDMODE(0, "bisect-log", ,
+N_("output contents of .git/BISECT_LOG"), BISECT_LOG),
OPT_BOOL(0, "no-checkout", _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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html