Hi,
On Tue, 27 Nov 2007, Alex Riesen wrote:
> Could not stop myself. Hopefully didn't beat anyone to it :)
> Almost all code shamelessly stolen from builtin-diff-index.c.
Then I have to wonder if it would not be a better idea to refactor the
code, so that other people do not have to steal the code again, but are
able to reuse it ;-)
> Preprocessor trickery in DIFF_OPT_* macros is disgusting, it breaks Vim
> word completion and trying to use many flags in one expression looks
> just ugly.
How does it break Vim word completion? And why should something like
DIFF_OPT_SET(&rev.diffopt, QUIET | EXIT_WITH_STATUS);
look ugly? I find it highly readable.
> + if (no_edit) {
> + static const char *argv[] = { NULL, "HEAD", NULL };
> + struct rev_info rev;
> + unsigned char sha1[40];
> + int is_initial;
> +
> + fclose(fp);
> +
> + if (!active_nr && read_cache() < 0)
> + die("Cannot read index");
> +
> + if (get_sha1("HEAD", sha1) != 0)
> + return !!active_nr;
Don't want to be anal here, but are there possibly reasons (read "possible
errors") other than an empty repo where this triggers?
> +
> + init_revisions(&rev, "");
> + rev.abbrev = 0;
> + (void)setup_revisions(2, argv, &rev, NULL);
(void)?
Besides, would this not be more elegant as
setup_revisions(0, NULL, &rev, "HEAD");
Hmm?
> + (void)run_diff_index(&rev, 1 /* cached */);
(void)?
Other than that (including my remark about refactoring that piece of
code), I like it.
Thanks,
Dscho
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
