Hey Stephan,

On Tue, Nov 22, 2016 at 6:19 AM, Stephan Beyer [off-list ref] wrote:

Okay Pranit,

this is the last patch for me to review in this series.

Now that I have a coarse overview of what you did, I have the general
remark that imho the "terms" variable should simply be global instead of
being passed around all the time.

In a personal conversation with my mentors, we thought it is the best
fit to keep it in a struct and pass it around so that it is easier in
libification.

I also had some other remarks but I forgot them... maybe they come to my
mind again when I see patch series v16.

I also want to remark again that I am not a Git developer and only
reviewed this because of my interest in git-bisect. So some of my
suggestions might conflict with other beliefs here. For example, I
consider it very bad style to leak memory... but Git is rather written
as a scripting tool than a genuine library, so perhaps many people here
do not care about it as long as it works...

Thanks for taking out your time to review my series extremely
carefully. I will try to post a v16 next week probably.

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

quoted

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c18ca07..b367d8d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c

@@ -601,7 +602,6 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
                      terms->term_good = arg;
              } else if (!strcmp(arg, "--term-bad") ||
                       !strcmp(arg, "--term-new")) {
-                     const char *next_arg;

This should already have been removed in patch 15/27, not here.

quoted

@@ -875,6 +875,117 @@ static int bisect_log(void)
      return status;
 }

+static int get_next_word(const char *line, int pos, struct strbuf *word)
+{
+     int i, len = strlen(line), begin = 0;
+     strbuf_reset(word);
+     for (i = pos; i < len; i++) {
+             if (line[i] == ' ' && begin)
+                     return i + 1;
+
+             if (!begin)
+                     begin = 1;
+             strbuf_addch(word, line[i]);
+     }
+
+     return i;
+}
+
+static int bisect_replay(struct bisect_terms *terms, const char *filename)
+{
+     struct strbuf line = STRBUF_INIT;
+     struct strbuf word = STRBUF_INIT;
+     FILE *fp = NULL;

(The initialization is not necessary here.)

Um. I think it is. Otherwise if it goes to the finish block before you
try to operate on fp, it will cause a seg fault.

quoted

+     int res = 0;
+
+     if (is_empty_or_missing_file(filename)) {
+             error(_("no such file with name '%s' exists"), filename);

The error message is misleading if the file exists but is empty.
Maybe something like "cannot read file '%s' for replaying"?

Okay will change.

quoted

+             res = -1;
+             goto finish;

                goto fail;
:D

quoted

+     }
+
+     if (bisect_reset(NULL)) {
+             res = -1;
+             goto finish;

                goto fail;

quoted

+     }
+
+     fp = fopen(filename, "r");
+     if (!fp) {
+             res = -1;
+             goto finish;

                goto fail;

quoted

+     }
+
+     while (strbuf_getline(&line, fp) != EOF) {
+             int pos = 0;
+             while (pos < line.len) {
+                     pos = get_next_word(line.buf, pos, &word);
+
+                     if (!strcmp(word.buf, "git")) {
+                             continue;
+                     } else if (!strcmp(word.buf, "git-bisect")) {
+                             continue;
+                     } else if (!strcmp(word.buf, "bisect")) {
+                             continue;
+                     } else if (!strcmp(word.buf, "#")) {
+                             break;

Maybe it is more robust to check whether word.buf begins with #

Assuming that you meant "# ", yes.

quoted

+                     }
+
+                     get_terms(terms);
+                     if (check_and_set_terms(terms, word.buf)) {
+                             res = -1;
+                             goto finish;

                                goto fail;

quoted

+                     }
+
+                     if (!strcmp(word.buf, "start")) {
+                             struct argv_array argv = ARGV_ARRAY_INIT;
+                             sq_dequote_to_argv_array(line.buf+pos, &argv);
+                             if (bisect_start(terms, 0, argv.argv, argv.argc)) {
+                                     argv_array_clear(&argv);
+                                     res = -1;
+                                     goto finish;

                                        goto fail;

quoted

+                             }
+                             argv_array_clear(&argv);
+                             break;
+                     }
+
+                     if (one_of(word.buf, terms->term_good,
+                         terms->term_bad, "skip", NULL)) {
+                             if (bisect_write(word.buf, line.buf+pos, terms, 0)) {
+                                     res = -1;
+                                     goto finish;

                                        goto fail;

quoted

+                             }
+                             break;
+                     }
+
+                     if (!strcmp(word.buf, "terms")) {
+                             struct argv_array argv = ARGV_ARRAY_INIT;
+                             sq_dequote_to_argv_array(line.buf+pos, &argv);
+                             if (bisect_terms(terms, argv.argv, argv.argc)) {
+                                     argv_array_clear(&argv);
+                                     res = -1;
+                                     goto finish;

                                        goto fail;

quoted

+                             }
+                             argv_array_clear(&argv);
+                             break;
+                     }
+
+                     error(_("?? what are you talking about?"));

I know this string is taken from the original source. However, even
something like error(_("Replay file contains rubbish (\"%s\")"),
word.buf) is more informative.

Okay will change.

quoted

+                     res = -1;
+                     goto finish;

                        goto fail;

quoted

+             }
+     }
+     goto finish;

+fail:
+       res = -1;

I just wanted to make finally clear what I was referring to by the
"goto fail" trick. :D

I got it. Will update accordingly.

Also I think the readability could be improved by extracting the body of
the outer while loop into an extra function replay_line(). Then most of
my suggested "goto fail;" lines simply become "return -1;" :)

quoted

@@ -998,6 +1112,13 @@ int cmd_bisect__helper(...)
                      die(_("--bisect-log requires 0 arguments"));
              res = bisect_log();
              break;
+     case BISECT_REPLAY:
+             if (argc != 1)
+                     die(_("--bisect-replay requires 1 argument"));

I'd keep the (already translated) string from the original source:
"No logfile given"

Sure

Regards.
Pranit Bauva