Common option parsing..

Common option parsing..

[Linus Torvalds]

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

Re: Common option parsing..

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> 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.
>...
> What do you think?

I think it all makes sense.  And once this kind of clean-up is
done, I suspect the implementations of "git diff", "git log" and
"git show" would become quite similar -- one notable difference
being "git diff" with a single rev and "git show" with a single
rev would be quite different, but that is just how the arguments
are interpreted, not parsed.

However, I am not sure about the two-revs case.  I suspect the
incoming items are sorted in the revs->commits list, and we
wouldn't be able to tell which is src and which is dst when
setup_revisions() returns.

Re: Common option parsing..

[Linus Torvalds]

On Wed, 12 Apr 2006, Junio C Hamano wrote:
> 
> However, I am not sure about the two-revs case.  I suspect the
> incoming items are sorted in the revs->commits list

Not by the argument parsing. That happens by "rev_parse_setup()" when we 
start to do the revision walking. 

So after setup_revisions() it's all good (although I think the list may be 
in "reverse order", I didn't check).

		Linus