Johannes Schindelin, Wed, Nov 28, 2007 13:18:10 +0100:
> 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 ;-)
Not sure it will be worth the effort. It is really short.
OTOH, I missed a diff-index interface where I could pass a resolved
sha1 (the u8 array returned by get_sha1) and index state. Something
like "int diff_tree_index(const unsigned char *sha1, struct rev_info *)".
Tempting, but I have only one use case for it. I don't actually know
Git's code that well...
> > 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.
Oh, this would look ok. It just wont compile: DIFF_OPT_SET prepends
second argument with DIFF_OPT_:
#define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)
#define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)
#define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)
> > + 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?
Definitely. I just don't know. OTOH, I can only return "committable"
or "not committable". If I return "commitable" for a broken repo,
it should fail elsewhere. If I return "not commitable" git-commit
shall finish telling user there is nothing to commit, which is just
wrong.
> > +
> > + init_revisions(&rev, "");
> > + rev.abbrev = 0;
> > + (void)setup_revisions(2, argv, &rev, NULL);
>
> (void)?
Yep. Sorry, I just got carried away by recent tendency (here at work)
to shut up PC-Lint (but please don't ask).
> Besides, would this not be more elegant as
>
> setup_revisions(0, NULL, &rev, "HEAD");
Hmm... And I was so puzzled as to what that "def" argument could
possibly mean... Still am, in fact. But it works.
> > + (void)run_diff_index(&rev, 1 /* cached */);
>
> (void)?
I'll remove them and resubmit. Stupid custom.
> Other than that (including my remark about refactoring that piece of
> code), I like it.
Me too: I have *extensively* tested it today and a commit on the
2.6GHz/2Gb/SATA windows machine is almost as fast as on my linux
laptop now (Centrino/1.2GHz downclocked to 800MHz/384Mb/IDE).
-
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
