* git blame not respecting --find-copies-harder ? @ 2008-07-30 9:39 Stephen R. van den Berg 2008-07-30 15:01 ` Björn Steinbrink 0 siblings, 1 reply; 17+ messages in thread From: Stephen R. van den Berg @ 2008-07-30 9:39 UTC (permalink / raw) To: Git Mailinglist git clone git://git.cuci.nl/pike Both of this "git blame" commands return the same (erroneous) results at the end of the files (the last lines are older, and are from the old README file next to it). git blame README-CVS git blame --find-copies-harder README-CVS However, when using git show with and without --find-copies-harder, we see that git indeed finds the copy with a 76% similarity index. git show 9eb55816 git show --find-copies-harder 9eb55816 Shouldn't it be expected that blame should also give the same difference in result with and without that option? On a related note, it doesn't seem possible to make --find-copies-harder a default; is that intentional? -- Sincerely, Stephen R. van den Berg. How many weeks are there in a lightyear? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-30 9:39 git blame not respecting --find-copies-harder ? Stephen R. van den Berg @ 2008-07-30 15:01 ` Björn Steinbrink 2008-07-30 15:43 ` Sverre Rabbelier 0 siblings, 1 reply; 17+ messages in thread From: Björn Steinbrink @ 2008-07-30 15:01 UTC (permalink / raw) To: Stephen R. van den Berg; +Cc: Git Mailinglist On 2008.07.30 11:39:03 +0200, Stephen R. van den Berg wrote: > git clone git://git.cuci.nl/pike > > Both of this "git blame" commands return the same (erroneous) results > at the end of the files (the last lines are older, and are from the old > README file next to it). > > git blame README-CVS > git blame --find-copies-harder README-CVS git blame doesn't know --find-copies-harder, it's -C -C for blame. Björn ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-30 15:01 ` Björn Steinbrink @ 2008-07-30 15:43 ` Sverre Rabbelier 2008-07-31 6:48 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Sverre Rabbelier @ 2008-07-30 15:43 UTC (permalink / raw) To: Björn Steinbrink; +Cc: Stephen R. van den Berg, Git Mailinglist On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink <B.Steinbrink@gmx.de> wrote: > git blame doesn't know --find-copies-harder, it's -C -C for blame. Shouldn't it have died with "don't know option --find-copies-harder" then? -- Cheers, Sverre Rabbelier ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-30 15:43 ` Sverre Rabbelier @ 2008-07-31 6:48 ` Jeff King 2008-07-31 7:05 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2008-07-31 6:48 UTC (permalink / raw) To: sverre; +Cc: Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist On Wed, Jul 30, 2008 at 05:43:52PM +0200, Sverre Rabbelier wrote: > On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink <B.Steinbrink@gmx.de> wrote: > > git blame doesn't know --find-copies-harder, it's -C -C for blame. > > Shouldn't it have died with "don't know option --find-copies-harder" then? Unfortunately, it _does_ know --find-copies-harder, because unknown options get sent to the revision option parser, which chains to the diff option parser. So it recognizes --find-copies-harder, but just sets a flag that doesn't do what we expect. I'm not sure if there is a simple fix. Does blame actually need the diff option parsing? If not, then we might be able to pass a flag to parse_revision_opt that says "don't do diff options, too". -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 6:48 ` Jeff King @ 2008-07-31 7:05 ` Junio C Hamano 2008-07-31 7:21 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2008-07-31 7:05 UTC (permalink / raw) To: Jeff King Cc: sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist Jeff King <peff@peff.net> writes: > On Wed, Jul 30, 2008 at 05:43:52PM +0200, Sverre Rabbelier wrote: > >> On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink <B.Steinbrink@gmx.de> wrote: >> > git blame doesn't know --find-copies-harder, it's -C -C for blame. >> >> Shouldn't it have died with "don't know option --find-copies-harder" then? > > Unfortunately, it _does_ know --find-copies-harder, because unknown > options get sent to the revision option parser, which chains to the diff > option parser. So it recognizes --find-copies-harder, but just sets a > flag that doesn't do what we expect. > > I'm not sure if there is a simple fix. Does blame actually need the diff > option parsing? If not, then we might be able to pass a flag to > parse_revision_opt that says "don't do diff options, too". Sigh... We can probably pick up the result revision parser parsed out of revs.diffopt, and then tweak "opt" with it, perhaps like this. builtin-blame.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/builtin-blame.c b/builtin-blame.c index 8b6b09b..4ea3431 100644 --- a/builtin-blame.c +++ b/builtin-blame.c @@ -2346,6 +2346,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix) parse_done: argc = parse_options_end(&ctx); + if (DIFF_OPT_TST(&revs.diffopt, FIND_COPIES_HARDER)) + opt |= (PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE | + PICKAXE_BLAME_COPY_HARDER); + if (!blame_move_score) blame_move_score = BLAME_DEFAULT_MOVE_SCORE; if (!blame_copy_score) ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 7:05 ` Junio C Hamano @ 2008-07-31 7:21 ` Jeff King 2008-07-31 7:36 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2008-07-31 7:21 UTC (permalink / raw) To: Junio C Hamano Cc: sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist On Thu, Jul 31, 2008 at 12:05:22AM -0700, Junio C Hamano wrote: > We can probably pick up the result revision parser parsed out of > revs.diffopt, and then tweak "opt" with it, perhaps like this. That is a sensible solution for this option, but I have to wonder: how many other such ineffective options are hiding? How many of them actually have a matching meaning in git-blame? E.g., what does "git blame --name-only" mean? Perhaps we should simply not worry about those ones, as people are unlikely to try using them, and it is easy to say "has no impact, because it doesn't make sense with blame." The truly confusing ones are ones you _expect_ to do something, but don't (like --find-copies-harder). I took a look at implementing a "don't parse the diff options" flag, but it is much larger than that. The revision parser understands a lot of options that don't really make sense for blame (or shortlog), like "--full-diff". So perhaps it is best to just fix this one (which we have actually had a bug report about) and not worry about the rest. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 7:21 ` Jeff King @ 2008-07-31 7:36 ` Junio C Hamano 2008-07-31 8:25 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2008-07-31 7:36 UTC (permalink / raw) To: Jeff King Cc: sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist Jeff King <peff@peff.net> writes: > I took a look at implementing a "don't parse the diff options" flag, but > it is much larger than that. The revision parser understands a lot of > options that don't really make sense for blame (or shortlog), like > "--full-diff". So perhaps it is best to just fix this one (which we have > actually had a bug report about) and not worry about the rest. That reminds me of an issue with shortlog. I often wish to do this: git shortlog --since=3.day --all | sort | uniq -c This is to catch a stupid mistake of (1) applying a few patches to 'master', (2) forking a new topic from 'master' and applying a few patches there, (3) realizing a few commits on 'master' that haven't been pushed out was faulty and rewrite the 'master' history. Such a new topic made in step (2) must be rebased on the updated 'master' built in step (3); otherwise merging the topic to 'next' will contaminate it with the old version of patches that have been rewritten on 'master'. Alas, shortlog does not take --all. Yes, I know git log --since=3.day --all | git shortlog | sort | uniq -c is an obvious workaround, but it is mildly irritating. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 7:36 ` Junio C Hamano @ 2008-07-31 8:25 ` Jeff King 2008-07-31 8:35 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2008-07-31 8:25 UTC (permalink / raw) To: Junio C Hamano Cc: sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote: > Alas, shortlog does not take --all. Yes, I know > > git log --since=3.day --all | git shortlog | sort | uniq -c > > is an obvious workaround, but it is mildly irritating. Hmm. Could it be as simple as: diff --git a/revision.c b/revision.c index a843c42..eaa5572 100644 --- a/revision.c +++ b/revision.c @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) { unkv[(*unkc)++] = arg; - return 0; + return 1; } if (!prefixcmp(arg, "--max-count=")) { That is, handle_revision_opt says "yes we parsed this, and it should be gone" even though it still gets stuck in the "unknown" section to be parsed by setup_revisions later. -Peff ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 8:25 ` Jeff King @ 2008-07-31 8:35 ` Junio C Hamano 2008-07-31 9:01 ` Pierre Habouzit 2008-07-31 9:03 ` Jeff King 0 siblings, 2 replies; 17+ messages in thread From: Junio C Hamano @ 2008-07-31 8:35 UTC (permalink / raw) To: Jeff King Cc: sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist Jeff King <peff@peff.net> writes: > On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote: > >> Alas, shortlog does not take --all. Yes, I know >> >> git log --since=3.day --all | git shortlog | sort | uniq -c >> >> is an obvious workaround, but it is mildly irritating. > > Hmm. Could it be as simple as: > > diff --git a/revision.c b/revision.c > index a843c42..eaa5572 100644 > --- a/revision.c > +++ b/revision.c > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg > !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) > { > unkv[(*unkc)++] = arg; > - return 0; > + return 1; > } > > if (!prefixcmp(arg, "--max-count=")) { > > That is, handle_revision_opt says "yes we parsed this, and it should be > gone" even though it still gets stuck in the "unknown" section to be > parsed by setup_revisions later. Hmm, wouldn't that suggest it needs to return 1 when an option candidate given to diff_opt_parse() turns out not to be a diff option and stuffed back to unkv[] at the end of this function? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 8:35 ` Junio C Hamano @ 2008-07-31 9:01 ` Pierre Habouzit 2008-07-31 9:05 ` Jeff King 2008-07-31 9:06 ` Pierre Habouzit 2008-07-31 9:03 ` Jeff King 1 sibling, 2 replies; 17+ messages in thread From: Pierre Habouzit @ 2008-07-31 9:01 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist [-- Attachment #1: Type: text/plain, Size: 3052 bytes --] On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote: > > > >> Alas, shortlog does not take --all. Yes, I know > >> > >> git log --since=3.day --all | git shortlog | sort | uniq -c > >> > >> is an obvious workaround, but it is mildly irritating. > > > > Hmm. Could it be as simple as: > > > > diff --git a/revision.c b/revision.c > > index a843c42..eaa5572 100644 > > --- a/revision.c > > +++ b/revision.c > > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg > > !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) > > { > > unkv[(*unkc)++] = arg; > > - return 0; > > + return 1; > > } > > > > if (!prefixcmp(arg, "--max-count=")) { > > > > That is, handle_revision_opt says "yes we parsed this, and it should be > > gone" even though it still gets stuck in the "unknown" section to be > > parsed by setup_revisions later. Ack this is a correct fix. I wonder how this even works with other commands that use --all and stuff like that. > Hmm, wouldn't that suggest it needs to return 1 when an option candidate > given to diff_opt_parse() turns out not to be a diff option and stuffed > back to unkv[] at the end of this function? No, because diff-opts are our last chance, and those are _really_ unknown options. "--all" is different because revision machinery acknowledge "yeah it's really for us, we recognize it ..." hence the 1 result "... but we have to parse it later so keep it in place". There is no such thing with unknown option for the revision _and_ diff machinery. They _must_ return 0 to say "no we really didn't know what this is". IOW, semantics is: < 0 : error 0 : option was not directed at us, unknown (this allow to chain option parsers. > 0 : yeah this was for us, and we consumed this amount of arguments in the process. Here "--all" is clearly something that we _recognized_ hence fit in the third case, even if we decide to still keep it in the "unk" array (which is ill named, I kept its name, but it's not unknown options, it's actually non option arguments that are to be deal with in the last stage, setup_revisions here, or the command as a general rule). Unknown options to both the revision _and_ diff option machineries are clearly of the 2nd kind and must return 0. They put the option in 'unk' as well (and here this is properly named) because this is how it historically worked for many commands, and is here as a legacy compatibility thing. parse-opt based parser (and git-shortlog has one) does not uses that (and you can see it in parse_revision_opt that is what such parser use: a result of `0' triggers a usage). -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 9:01 ` Pierre Habouzit @ 2008-07-31 9:05 ` Jeff King 2008-07-31 9:06 ` Pierre Habouzit 1 sibling, 0 replies; 17+ messages in thread From: Jeff King @ 2008-07-31 9:05 UTC (permalink / raw) To: Pierre Habouzit, Junio C Hamano, sverre, Björn Steinbrink, Stephen R. van den Berg On Thu, Jul 31, 2008 at 11:01:46AM +0200, Pierre Habouzit wrote: > Ack this is a correct fix. I wonder how this even works with other > commands that use --all and stuff like that. --all is parsed separately in setup_revisions _before_ we call handle_revision_opt. So the only codepath hitting that is parse_options users who call parse_revision_opt (i.e., blame and shortlog). -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 9:01 ` Pierre Habouzit 2008-07-31 9:05 ` Jeff King @ 2008-07-31 9:06 ` Pierre Habouzit 1 sibling, 0 replies; 17+ messages in thread From: Pierre Habouzit @ 2008-07-31 9:06 UTC (permalink / raw) To: Junio C Hamano, Jeff King, sverre, Björn Steinbrink, Stephen R. van den Berg [-- Attachment #1: Type: text/plain, Size: 1995 bytes --] On Thu, Jul 31, 2008 at 09:01:46AM +0000, Pierre Habouzit wrote: > On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote: > > Jeff King <peff@peff.net> writes: > > > > > On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote: > > > > > >> Alas, shortlog does not take --all. Yes, I know > > >> > > >> git log --since=3.day --all | git shortlog | sort | uniq -c > > >> > > >> is an obvious workaround, but it is mildly irritating. > > > > > > Hmm. Could it be as simple as: > > > > > > diff --git a/revision.c b/revision.c > > > index a843c42..eaa5572 100644 > > > --- a/revision.c > > > +++ b/revision.c > > > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg > > > !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) > > > { > > > unkv[(*unkc)++] = arg; > > > - return 0; > > > + return 1; > > > } > > > > > > if (!prefixcmp(arg, "--max-count=")) { > > > > > > That is, handle_revision_opt says "yes we parsed this, and it should be > > > gone" even though it still gets stuck in the "unknown" section to be > > > parsed by setup_revisions later. > > Ack this is a correct fix. I wonder how this even works with other > commands that use --all and stuff like that. Oh actually I know: the parse_revision_opt machinery (that is buggy because of this 0 result) is used in git-blame where --all is meaningless anyways, and ... git-shortlog only. The legacy way to parse revision options does not treats `0' answers differently from `1' actually, I wonder why, because this is probably wrong. This was a regression, I suppose prior to its parse-optification git-shortlog accepted --all (and I see no valid reason for it not to). Thanks a lot to Jeff for the good catch. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 8:35 ` Junio C Hamano 2008-07-31 9:01 ` Pierre Habouzit @ 2008-07-31 9:03 ` Jeff King 2008-07-31 9:15 ` Pierre Habouzit 1 sibling, 1 reply; 17+ messages in thread From: Jeff King @ 2008-07-31 9:03 UTC (permalink / raw) To: Junio C Hamano Cc: sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist On Thu, Jul 31, 2008 at 01:35:33AM -0700, Junio C Hamano wrote: > > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg > > !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) > > { > > unkv[(*unkc)++] = arg; > > - return 0; > > + return 1; > > } > > > > if (!prefixcmp(arg, "--max-count=")) { > > > > That is, handle_revision_opt says "yes we parsed this, and it should be > > gone" even though it still gets stuck in the "unknown" section to be > > parsed by setup_revisions later. > > Hmm, wouldn't that suggest it needs to return 1 when an option candidate > given to diff_opt_parse() turns out not to be a diff option and stuffed > back to unkv[] at the end of this function? Not necessarily. The logic there goes: 1. it's not a revision option, so pass to diff 2. it's not a diff opt, so it is unknown; we parsed no options 3. the caller sees we failed to parse, so it barfs but the logic here is: 1. it _is_ a revision option. Our handling of it is just a little odd, in that we need to parse it later, when we are in setup_revisions. So put it aside for now as a "revision" that just happens to look like an option. 2. caller sees we parsed, and doesn't complain 3. caller later passes the "revision" to setup_revisions, which knows what to do Now my patch doesn't just operate on "--all", but rather several other options including --no-walk. And maybe that is not right, and --all should be handled separately (though I think --remotes would follow the same logic). I'm not really sure why --no-walk is separated like that. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 9:03 ` Jeff King @ 2008-07-31 9:15 ` Pierre Habouzit 2008-07-31 9:34 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Pierre Habouzit @ 2008-07-31 9:15 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist [-- Attachment #1: Type: text/plain, Size: 1976 bytes --] On Thu, Jul 31, 2008 at 09:03:49AM +0000, Jeff King wrote: > Not necessarily. The logic there goes: > > 1. it's not a revision option, so pass to diff > 2. it's not a diff opt, so it is unknown; we parsed no options > 3. the caller sees we failed to parse, so it barfs > > but the logic here is: > > 1. it _is_ a revision option. Our handling of it is just a little odd, > in that we need to parse it later, when we are in setup_revisions. > So put it aside for now as a "revision" that just happens to look > like an option. > 2. caller sees we parsed, and doesn't complain > 3. caller later passes the "revision" to setup_revisions, which knows > what to do > > Now my patch doesn't just operate on "--all", but rather several other > options including --no-walk. And maybe that is not right, and --all > should be handled separately (though I think --remotes would follow the > same logic). I'm not really sure why --no-walk is separated like that. --no-walk alters how add_pending_object_with_mode works, which is called by handle_revision_arg. IOW, <ref1> --no-walk <ref2> is not the same as --no-walk <ref1> <ref2>. IOW reference arguments and --no-walk ordering must be preserved, IOW --no-walk is a pseudo-argument like --all --tags or --remote are. It's okay for parse_revision_opt to take out any option that doesn't alter handle_revision_arg (as for parse_option based parsers it's the sole thing setup_revision will be doing: consuming revisions in a loop with handle_revision_arg), but only those. The full list (that I made when I wrote parse_revision_opt) is actually: pseudo arguments: --all --branches --tags --remotes --reflog combining operator: --not behavior modifying: --no-walk --do-walk -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 9:15 ` Pierre Habouzit @ 2008-07-31 9:34 ` Jeff King 2008-07-31 10:22 ` Pierre Habouzit 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2008-07-31 9:34 UTC (permalink / raw) To: Pierre Habouzit Cc: Junio C Hamano, sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist [Pierre, you have the bogus MFT header that people are often complaining about. Either I have to do extra work to fix the headers, or more people end up in the To: field. I don't personally care about the latter, but it seems that others organize their mail based on To versus Cc] On Thu, Jul 31, 2008 at 11:15:15AM +0200, Pierre Habouzit wrote: > --no-walk alters how add_pending_object_with_mode works, which is called > by handle_revision_arg. IOW, <ref1> --no-walk <ref2> is not the same as > --no-walk <ref1> <ref2>. IOW reference arguments and --no-walk ordering > must be preserved, IOW --no-walk is a pseudo-argument like --all --tags > or --remote are. Ah, OK. Then that makes sense, then. And I definitely feel that the patch I posted is the right thing to do, then. So here is a patch with commit message. The commit message to patch ratio is ridiculous, but obviously this fix took some thinking, so it seems best to document it. -- >8 -- allow "non-option" revision options in parse_option-enabled commands Commands which use parse_options but also call setup_revisions must do their parsing in a two step process: 1. first, they parse all options. Anything unknown goes to parse_revision_opt (which calls handle_revision_opt), which may claim the option or say "I don't recognize this" 2. the non-option remainder goes to setup_revisions to actually get turned into revisions Some revision options are "non-options" in that they must be parsed in order with their revision counterparts in setup_revisions. For example, "--all" functions as a pseudo-option expanding to all refs, and "--no-walk" affects refs after it on the command line, but not before. The revision option parser in step 1 recognizes such options and sets them aside for later parsing by setup_revisions. However, the return value used from handle_revision_opt indicated "I didn't recognize this", which was wrong. It did, and it took appropriate action (even though that action was just deferring it for later parsing). Thus it should return "yes, I recognized this." Previously, these pseudo-options generated an error when used with parse_options parsers (currently just blame and shortlog). With this patch, they should work fine, enabling things like "git shortlog --all". Signed-off-by: Jeff King <peff@peff.net> --- revision.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/revision.c b/revision.c index a843c42..eaa5572 100644 --- a/revision.c +++ b/revision.c @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) { unkv[(*unkc)++] = arg; - return 0; + return 1; } if (!prefixcmp(arg, "--max-count=")) { -- 1.6.0.rc1.197.gc57ac6 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 9:34 ` Jeff King @ 2008-07-31 10:22 ` Pierre Habouzit 2008-07-31 10:33 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Pierre Habouzit @ 2008-07-31 10:22 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, sverre, Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist [-- Attachment #1: Type: text/plain, Size: 3074 bytes --] > [Pierre, you have the bogus MFT header that people are often complaining > about. Either I have to do extra work to fix the headers, or more people > end up in the To: field. I don't personally care about the latter, but > it seems that others organize their mail based on To versus Cc] [ err if you do a list reply, nobody ends up in To:... but oh well I can probably remove it, it'll just make my own mutt git folder awkwardly colorized. ] On Thu, Jul 31, 2008 at 09:34:10AM +0000, Jeff King wrote: > On Thu, Jul 31, 2008 at 11:15:15AM +0200, Pierre Habouzit wrote: > > --no-walk alters how add_pending_object_with_mode works, which is called > > by handle_revision_arg. IOW, <ref1> --no-walk <ref2> is not the same as > > --no-walk <ref1> <ref2>. IOW reference arguments and --no-walk ordering > > must be preserved, IOW --no-walk is a pseudo-argument like --all --tags > > or --remote are. > > Ah, OK. Then that makes sense, then. And I definitely feel that the > patch I posted is the right thing to do, Yeah I was convinced the first time I saw it already :) -- >8 -- allow "non-option" revision options in parse_option-enabled commands Commands which use parse_options but also call setup_revisions must do their parsing in a two step process: 1. first, they parse all options. Anything unknown goes to parse_revision_opt (which calls handle_revision_opt), which may claim the option or say "I don't recognize this" 2. the non-option remainder goes to setup_revisions to actually get turned into revisions Some revision options are "non-options" in that they must be parsed in order with their revision counterparts in setup_revisions. For example, "--all" functions as a pseudo-option expanding to all refs, and "--no-walk" affects refs after it on the command line, but not before. The revision option parser in step 1 recognizes such options and sets them aside for later parsing by setup_revisions. However, the return value used from handle_revision_opt indicated "I didn't recognize this", which was wrong. It did, and it took appropriate action (even though that action was just deferring it for later parsing). Thus it should return "yes, I recognized this." Previously, these pseudo-options generated an error when used with parse_options parsers (currently just blame and shortlog). With this patch, they should work fine, enabling things like "git shortlog --all". Signed-off-by: Jeff King <peff@peff.net> Acked-By: Pierre Habouzit <madcoder@debian.org> --- revision.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/revision.c b/revision.c index a843c42..eaa5572 100644 --- a/revision.c +++ b/revision.c @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) { unkv[(*unkc)++] = arg; - return 0; + return 1; } if (!prefixcmp(arg, "--max-count=")) { -- 1.6.0.rc1.197.gc57ac6 [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ? 2008-07-31 10:22 ` Pierre Habouzit @ 2008-07-31 10:33 ` Jeff King 0 siblings, 0 replies; 17+ messages in thread From: Jeff King @ 2008-07-31 10:33 UTC (permalink / raw) To: Pierre Habouzit; +Cc: Junio C Hamano, Git Mailinglist On Thu, Jul 31, 2008 at 12:22:23PM +0200, Pierre Habouzit wrote: > > [Pierre, you have the bogus MFT header that people are often complaining > > about. Either I have to do extra work to fix the headers, or more people > > end up in the To: field. I don't personally care about the latter, but > > it seems that others organize their mail based on To versus Cc] > > [ err if you do a list reply, nobody ends up in To:... but oh well I > can probably remove it, it'll just make my own mutt git folder > awkwardly colorized. ] I can't do a list reply, as mutt sees no list (and I didn't bother setting up a 'subscribe' variable, since I didn't want to generate MFTs in the first place). However, I just realized that mutt has an honor_followup_to, which will do what I want (AFAICT, there is no point in honoring MFT when using the conventions of this mailing list). But I wonder if it is as simple in other clients. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2008-07-31 10:35 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-07-30 9:39 git blame not respecting --find-copies-harder ? Stephen R. van den Berg 2008-07-30 15:01 ` Björn Steinbrink 2008-07-30 15:43 ` Sverre Rabbelier 2008-07-31 6:48 ` Jeff King 2008-07-31 7:05 ` Junio C Hamano 2008-07-31 7:21 ` Jeff King 2008-07-31 7:36 ` Junio C Hamano 2008-07-31 8:25 ` Jeff King 2008-07-31 8:35 ` Junio C Hamano 2008-07-31 9:01 ` Pierre Habouzit 2008-07-31 9:05 ` Jeff King 2008-07-31 9:06 ` Pierre Habouzit 2008-07-31 9:03 ` Jeff King 2008-07-31 9:15 ` Pierre Habouzit 2008-07-31 9:34 ` Jeff King 2008-07-31 10:22 ` Pierre Habouzit 2008-07-31 10:33 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).