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.
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...
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> 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.
> @@ -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.)
> + 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"?
> + res = -1;
> + goto finish;
goto fail;
:D
> + }
> +
> + if (bisect_reset(NULL)) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> +
> + fp = fopen(filename, "r");
> + if (!fp) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> +
> + 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 #
> + }
> +
> + get_terms(terms);
> + if (check_and_set_terms(terms, word.buf)) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> +
> + 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;
> + }
> + 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;
> + }
> + 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;
> + }
> + 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.
> + res = -1;
> + goto finish;
goto fail;
> + }
> + }
> + goto finish;
+fail:
+ res = -1;
I just wanted to make finally clear what I was referring to by the
"goto fail" trick. :D
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;" :)
> @@ -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"
Cheers,
Stephan