{"thread":{"id":"3857","subject":"Common option parsing..","startedAt":"2006-04-13T01:48:54Z","lastAt":"2006-04-13T14:46:28Z","messageCount":3,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"18597","messageId":"Pine.LNX.4.64.0604121828370.14565@g5.osdl.org","threadId":"3857","inReplyTo":null,"subject":"Common option parsing..","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-13T01:48:54Z","receivedAt":"2006-04-13T01:48:54Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nJunio,\n right now we actually haev very consistent command line options for git, \nbut we have two (and in your \"next\" branch, three) different structures \nthat they get parsed into, and lots of it is duplicated. We have \n\"diff_options\", \"rev_info\" and now \"log_info\".\n\nTo make matters worse, some things aren't actually in any of them, ie \n\"--cc\", \"--abbrev\" and friends actually end up being parsed into their own \nprivate flags in diff-files. Some are in _both_ rev_info and diff_options \n(the \"--pretty\" parsing), because both diff and rev-parse supported that \noption set.\n\nAnd almost all commands that take any of those options at all end up \nactually taking the combination of them these days. Yeah, git-rev-parse \ndoesn't, but quite frankly, with your \"git log --diff\" changes, that's \nactually the odd man out, and I think we should just make git-rev-parse \nbasically do it too. \n\nAnd some things, like doing a builtin \"git diff\" would actually be quite \neasy to do, except for the fact that having three different option parsers \n_and_ having some options you parse by hand on top of that is just crazy \n(\"git diff\" wants even the stage diff flags that git-diff-files takes).\n\nThe easiest way to just solve all this mess would be to \n - add the diff-options into \"struct rev_list\" and make the \n   \"setup_revisions()\" parser parse the diff flags too.\n - get rid of \"log_info\" and \"diff_options\"\n - possibly rename the resulting super-options structure as \"struct \n   git_options\" or something if we want to.\n\nAt that point, it would become a lot easier to do things like a built-in \n\"git diff\", where command line parsing really is the biggest deal. It \nwould become something like this:\n\n\tstruct rev_info revs;\n\tstruct commit *src, *dst;\n\n\tif (setup_revisions(&revs))\n\t\tdie(git_diff_usage);\n\n\t/* No revision arguments: git-diff-files */\n\tif (!revs->commits)\n\t\treturn diff_files(&revs);\n\n\tsrc = revs->commits->item;\n\trevs->commits = revs->commits->next;\n\n\t/* Just one rev: git-diff-index against that */\n\tif (!revs->commits)\n\t\treturn diff_index(&revs, src);\n\n\tdst = revs->commits->item;\n\trevs->commits = revs->commits->next;\n\n\t/*\n\t * More than two revs? Maybe that means a combined diff?\n\t * Some day.. In the meantime, just make it an error.\n\t */\n\tif (revs->commits->next)\n\t\tdie(git_diff_usage);\n\n\t/*\n\t * If it was \"a..b\" using git-rev-parse, the second commit\n\t * on the list is the initial (and uninteresting) one, we\n\t * need to make that the source..\n\t */\n\tif (dst->object.flags & UNINTERESTING) {\n\t\tstruct commit *tmp = dst;\n\t\tdst = src;\n\t\tsrc = tmp;\n\t}\n\n\treturn diff_trees(&revs, src, dst);\n\nwhere obviously we'd need to do some minor moving-around to make \n\"diff_files()\" be a function interface, but I actually did that, and it \nwas really trivial. The bigger part would be to just change the structures \naround (which could be done first as a fairly big but trivial patch, kind \nof the same way the initial \"struct rev_info\" was done when the \n\"revision.c\" file was split away).\n\nWhat do you think?\n\n\t\tLinus\n"},{"id":"18599","messageId":"7vmzeq12zd.fsf@assigned-by-dhcp.cox.net","threadId":"3857","inReplyTo":"Pine.LNX.4.64.0604121828370.14565@g5.osdl.org","subject":"Re: Common option parsing..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-13T05:03:34Z","receivedAt":"2006-04-13T05:03:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> The easiest way to just solve all this mess would be to \n>  - add the diff-options into \"struct rev_list\" and make the \n>    \"setup_revisions()\" parser parse the diff flags too.\n>  - get rid of \"log_info\" and \"diff_options\"\n>  - possibly rename the resulting super-options structure as \"struct \n>    git_options\" or something if we want to.\n>...\n> What do you think?\n\nI think it all makes sense.  And once this kind of clean-up is\ndone, I suspect the implementations of \"git diff\", \"git log\" and\n\"git show\" would become quite similar -- one notable difference\nbeing \"git diff\" with a single rev and \"git show\" with a single\nrev would be quite different, but that is just how the arguments\nare interpreted, not parsed.\n\nHowever, I am not sure about the two-revs case.  I suspect the\nincoming items are sorted in the revs->commits list, and we\nwouldn't be able to tell which is src and which is dst when\nsetup_revisions() returns.\n"},{"id":"18612","messageId":"Pine.LNX.4.64.0604130745160.14565@g5.osdl.org","threadId":"3857","inReplyTo":"7vmzeq12zd.fsf@assigned-by-dhcp.cox.net","subject":"Re: Common option parsing..","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-13T14:46:28Z","receivedAt":"2006-04-13T14:46:28Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 12 Apr 2006, Junio C Hamano wrote:\n> \n> However, I am not sure about the two-revs case.  I suspect the\n> incoming items are sorted in the revs->commits list\n\nNot by the argument parsing. That happens by \"rev_parse_setup()\" when we \nstart to do the revision walking. \n\nSo after setup_revisions() it's all good (although I think the list may be \nin \"reverse order\", I didn't check).\n\n\t\tLinus\n"}]}