Hey Stephan,
On Tue, Nov 22, 2016 at 6:19 AM, Stephan Beyer <s-beyer@xxxxxxx> 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:
>> 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.)
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.
>> + 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.
>> + 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 #
Assuming that you meant "# ", yes.
>> + }
>> +
>> + 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.
Okay will change.
>> + 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
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;" :)
>
>> @@ -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