Hey Stephan,

On Thu, Nov 17, 2016 at 3:10 PM, Stephan Beyer [off-list ref] wrote:

Hi,

I've only got some minors to mention here ;)

On 10/14/2016 04:14 PM, Pranit Bauva wrote:

quoted

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c542e8b..3f19b68 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c

@@ -19,9 +19,15 @@ 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
 };

I wouldn't write "<TERM_GOOD <TERM_BAD>" in capital letters. I'd use
something like "<good_term> <bad_term>" as you have used for
--write-terms. Note that "git bisect --help" uses "<term-old>
<term-new>" in that context.

Keeping it in small does give less strain to the eye ;)

quoted

@@ -149,6 +155,63 @@ 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 = NULL;
+     int retval = 0;
+
+     if (!strcmp(state, terms->term_bad))
+             strbuf_addf(&tag, "refs/bisect/%s", state);
+     else if (one_of(state, terms->term_good, "skip", NULL))
+             strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
+     else {
+             error(_("Bad bisect_write argument: %s"), state);
+             retval = -1;
+             goto finish;
+     }
+
+     if (get_oid(rev, &oid)) {
+             error(_("couldn't get the oid of the rev '%s'"), rev);
+             retval = -1;
+             goto finish;
+     }
+
+     if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
+                    UPDATE_REFS_MSG_ON_ERR)) {
+             retval = -1;
+             goto finish;
+     }

I'd like to mention that the "goto fail;" trick could apply in this
function, too.

Sure!

quoted

@@ -156,9 +219,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;

Why do you do this "direct return" -> "set res and return res" transition?
You don't need it in this patch, you do not need it in the subsequent
patches (you always set "res" exactly once after the initialization),
and you don't need cleanup code in this function.

Initially I was using strbuf but then I switched to const char *
according to Junio's suggestion. It seems that in this version I have
forgot to free the terms.

quoted

      struct option options[] = {
              OPT_CMDMODE(0, "next-all", &cmdmode,
                       N_("perform 'git bisect next'"), NEXT_ALL),

@@ -170,10 +234,13 @@ 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),

That info text is confusing, especially considering that there is a
"nolog" option. I think the action of bisect-write is two-fold: (1)
update the refs, (2) log.

I agree that it is confusing. I couldn't find a better way to describe
it and since this would be gone after the whole conversion, I didn't
bother putting more efforts there.

Regards,
Pranit Bauva