Junio,
right now we actually haev very consistent command line options for git,
but we have two (and in your "next" branch, three) different structures
that they get parsed into, and lots of it is duplicated. We have
"diff_options", "rev_info" and now "log_info".
To make matters worse, some things aren't actually in any of them, ie
"--cc", "--abbrev" and friends actually end up being parsed into their own
private flags in diff-files. Some are in _both_ rev_info and diff_options
(the "--pretty" parsing), because both diff and rev-parse supported that
option set.
And almost all commands that take any of those options at all end up
actually taking the combination of them these days. Yeah, git-rev-parse
doesn't, but quite frankly, with your "git log --diff" changes, that's
actually the odd man out, and I think we should just make git-rev-parse
basically do it too.
And some things, like doing a builtin "git diff" would actually be quite
easy to do, except for the fact that having three different option parsers
_and_ having some options you parse by hand on top of that is just crazy
("git diff" wants even the stage diff flags that git-diff-files takes).
The easiest way to just solve all this mess would be to
- add the diff-options into "struct rev_list" and make the
"setup_revisions()" parser parse the diff flags too.
- get rid of "log_info" and "diff_options"
- possibly rename the resulting super-options structure as "struct
git_options" or something if we want to.
At that point, it would become a lot easier to do things like a built-in
"git diff", where command line parsing really is the biggest deal. It
would become something like this:
struct rev_info revs;
struct commit *src, *dst;
if (setup_revisions(&revs))
die(git_diff_usage);
/* No revision arguments: git-diff-files */
if (!revs->commits)
return diff_files(&revs);
src = revs->commits->item;
revs->commits = revs->commits->next;
/* Just one rev: git-diff-index against that */
if (!revs->commits)
return diff_index(&revs, src);
dst = revs->commits->item;
revs->commits = revs->commits->next;
/*
* More than two revs? Maybe that means a combined diff?
* Some day.. In the meantime, just make it an error.
*/
if (revs->commits->next)
die(git_diff_usage);
/*
* If it was "a..b" using git-rev-parse, the second commit
* on the list is the initial (and uninteresting) one, we
* need to make that the source..
*/
if (dst->object.flags & UNINTERESTING) {
struct commit *tmp = dst;
dst = src;
src = tmp;
}
return diff_trees(&revs, src, dst);
where obviously we'd need to do some minor moving-around to make
"diff_files()" be a function interface, but I actually did that, and it
was really trivial. The bigger part would be to just change the structures
around (which could be done first as a fairly big but trivial patch, kind
of the same way the initial "struct rev_info" was done when the
"revision.c" file was split away).
What do you think?
Linus
-
: 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