{"thread":{"id":"31454","subject":"git blame --follow","startedAt":"2012-09-06T07:02:17Z","lastAt":"2012-09-21T21:00:18Z","messageCount":15,"participants":["norbert.nemec","Jeff King","Drew Northup","Junio C Hamano","Kevin Ballard"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"198424","messageId":"k29hpo$3av$1@ger.gmane.org","threadId":"31454","inReplyTo":null,"subject":"git blame --follow","fromName":"norbert.nemec","fromEmail":"norbert.nemec@native-instruments.de","sentAt":"2012-09-06T07:02:17Z","receivedAt":"2012-09-06T07:02:17Z","isPatch":false,"sender":{"key":"norbert.nemec@native-instruments.de","avatar":null},"body":"Hi there,\n\n'git blame --follow' seems to be undocumented. The exact behavior is not \nclear to me. Perhaps an alias for some combination of '-C' and '-M'? It \nseems not be be fully consistent with 'git log --follow'.\n\nCould someone clarify? Did I miss something?\n\nGreetings,\nNorbert\n"},{"id":"198429","messageId":"20120906095804.GA15277@sigill.intra.peff.net","threadId":"31454","inReplyTo":"k29hpo$3av$1@ger.gmane.org","subject":"Re: git blame --follow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-06T09:58:04Z","receivedAt":"2012-09-06T09:58:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 06, 2012 at 09:02:17AM +0200, norbert.nemec wrote:\n\n> 'git blame --follow' seems to be undocumented. The exact behavior is\n> not clear to me. Perhaps an alias for some combination of '-C' and\n> '-M'? It seems not be be fully consistent with 'git log --follow'.\n> \n> Could someone clarify? Did I miss something?\n\nI don't think it was ever intended to do anything; the only reason it is\nnot rejected outright is that \"blame\" piggy-backs on the regular\nrevision option parser used by \"log\" and others.\n\nWhat would you expect it to do?\n\nI can't think of a sane behavior for \"blame --follow\". The follow code\nis about tweaking path-limiting during traversal, but blame does not use\npathspecs. It tracks content, and the \"-C\" option already instructs it to\nlook across file boundaries.\n\n-Peff\n"},{"id":"198430","messageId":"k29sup$2e0$1@ger.gmane.org","threadId":"31454","inReplyTo":"20120906095804.GA15277@sigill.intra.peff.net","subject":"Re: git blame --follow","fromName":"norbert.nemec","fromEmail":"norbert.nemec@native-instruments.de","sentAt":"2012-09-06T10:12:42Z","receivedAt":"2012-09-06T10:12:42Z","isPatch":false,"sender":{"key":"norbert.nemec@native-instruments.de","avatar":null},"body":"Thanks for the explanation.\n\nI actually do not have any clear opinion what it should do. Just that \nthe current situation is confusing when experimenting and trying to \nunderstand the behavior of git blame and git log: an intuitive option \nthat is accepted but ignored.\n\nThe option should either be rejected or do *something* documented and \nuseful. Ideally, it should result in behavior that matches 'git log \n--follow' as closely as possible. So maybe, it should be a synonym for a \ncertain number of \"-C\" options?\n\nGreetings,\nNorbert\n\n\n\nAm 06.09.12 11:58, schrieb Jeff King:\n> On Thu, Sep 06, 2012 at 09:02:17AM +0200, norbert.nemec wrote:\n>\n>> 'git blame --follow' seems to be undocumented. The exact behavior is\n>> not clear to me. Perhaps an alias for some combination of '-C' and\n>> '-M'? It seems not be be fully consistent with 'git log --follow'.\n>>\n>> Could someone clarify? Did I miss something?\n>\n> I don't think it was ever intended to do anything; the only reason it is\n> not rejected outright is that \"blame\" piggy-backs on the regular\n> revision option parser used by \"log\" and others.\n>\n> What would you expect it to do?\n>\n> I can't think of a sane behavior for \"blame --follow\". The follow code\n> is about tweaking path-limiting during traversal, but blame does not use\n> pathspecs. It tracks content, and the \"-C\" option already instructs it to\n> look across file boundaries.\n>\n> -Peff\n>\n"},{"id":"198453","messageId":"20120906151317.GB7407@sigill.intra.peff.net","threadId":"31454","inReplyTo":"k29sup$2e0$1@ger.gmane.org","subject":"Re: git blame --follow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-06T15:13:17Z","receivedAt":"2012-09-06T15:13:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 06, 2012 at 12:12:42PM +0200, norbert.nemec wrote:\n\n> The option should either be rejected or do *something* documented and\n> useful. Ideally, it should result in behavior that matches 'git log\n> --follow' as closely as possible. So maybe, it should be a synonym\n> for a certain number of \"-C\" options?\n\nBut I don't see how it would match \"git log --follow\", as that is a\nfundamentally different operation that makes no sense in the context of\nblame (why would you be adjusting the pathspec? There is no pathspec). A\nsynonym for \"-C\" would just confuse things more, as log also has \"-C\"\nand it is not a synonym there.\n\nSo if anything, I'd say to simply reject it. It's not documented, and it\nnever did anything useful. Patches welcome.\n\n-Peff\n"},{"id":"199437","messageId":"1348022905-10048-1-git-send-email-n1xim.email@gmail.com","threadId":"31454","inReplyTo":"20120906151317.GB7407@sigill.intra.peff.net","subject":"[PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Drew Northup","fromEmail":"n1xim.email@gmail.com","sentAt":"2012-09-19T02:48:25Z","receivedAt":"2012-09-19T02:48:25Z","isPatch":true,"sender":{"key":"n1xim.email@gmail.com","avatar":null},"body":"Make note that while the --follow option is accepted by git blame it does\nnothing.\n\nSigned-off-by: Drew Northup \n---\n Documentation/git-blame.txt | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex 7ee9236..7465bd8 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -9,7 +9,7 @@ SYNOPSIS\n --------\n [verse]\n 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental] [-L n,m]\n-\t [-S ] [-M] [-C] [-C] [-C] [--since=] [--abbrev=]\n+\t [-S ] [-M] [-C] [-C] [-C] [--since=] [--abbrev=] [--follow]\n \t [ | --contents | --reverse ] [--] \n \n DESCRIPTION\n@@ -78,6 +78,9 @@ include::blame-options.txt[]\n \tabbreviated object name, use +1 digits. Note that 1 column\n \tis used for a caret to mark the boundary commit.\n \n+--follow::\n+ NO-OP accepted due to using the option parser also used by\n+ 'git log'\n \n THE PORCELAIN FORMAT\n --------------------\n-- \n1.7.12.rc0.54.g9e2116a\n"},{"id":"199444","messageId":"7v627aiq47.fsf@alter.siamese.dyndns.org","threadId":"31454","inReplyTo":"1348022905-10048-1-git-send-email-n1xim.email@gmail.com","subject":"Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-19T04:38:32Z","receivedAt":"2012-09-19T04:38:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Drew Northup writes:\n\n> Make note that while the --follow option is accepted by git blame it does\n> nothing.\n>\n> Signed-off-by: Drew Northup \n> ---\n> Documentation/git-blame.txt | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\n> index 7ee9236..7465bd8 100644\n> --- a/Documentation/git-blame.txt\n> +++ b/Documentation/git-blame.txt\n> @@ -9,7 +9,7 @@ SYNOPSIS\n> --------\n> [verse]\n> 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental] [-L n,m]\n> -\t [-S ] [-M] [-C] [-C] [-C] [--since=] [--abbrev=]\n> +\t [-S ] [-M] [-C] [-C] [-C] [--since=] [--abbrev=] [--follow]\n> \t [ | --contents | --reverse ] [--] \n> \n> DESCRIPTION\n> @@ -78,6 +78,9 @@ include::blame-options.txt[]\n> \tabbreviated object name, use +1 digits. Note that 1 column\n> \tis used for a caret to mark the boundary commit.\n> \n> +--follow::\n> + NO-OP accepted due to using the option parser also used by\n> + 'git log'\n\nThis is triply questionable. If it is a useless NO-OP, it shouldn't\nbe advertised in the SYNOPSIS section to begin with.\n\nYour \"--follow is a no-op for blame\" is technically correct, but I\nthink the only ones that can appreciate that technical correctness\nare those like you who know why we can get away by having a\nseemingly no-op \"--follow\" option without losing functionality.\n\n\"git blame\" follows the whole-file renames correctly [*1*], without\nany -M/-C options. There is no _need_ to use the \"keep one global\nfilename to follow, and switch to a different filename when it\ndisappears\" hack the \"--follow\" code uses. That is the reason why\n\"blame\" pays no attention to the \"--follow\" option. You know that,\nand that is why you think it is a sane thing to describe it in\ntechnically correct way.\n\nBut I think most of the readers of the documentation are not aware\nof that true reason why it can be a \"No-op\". Worse yet, they may\nhave heard of the \"--follow\" option that the \"log\" command has from\nany of the numerous misguided web pages, and are led to believe that\nthe \"--follow\" option is a true feature, not a checkbox hack.\n\nIf readers come from that background, thinking \"--follow\" is the way\nto tell Git to follow renames, what message does your description\nsend them? I would read it as \"git blame accepts --follow from the\ncommand line, but it is a no-op. There is no way to make it follow\nrenames.\"\n\nThat is a totally wrong message to send. You failed to teach the\nreader that there is no need to do anything special to tell the\ncommand to follow per-line origin across renames.\n\nSo if anything, I would phrase it this way instead:\n\n --follow::\n This option is accepted but silently ignored. \"git blame\"\n\t follows per-line origin across renames without any special\n\t options, and there is no reason to use this option.\n\nIt does not matter to the reader why it is accepted by the parser at\nthe mechanical level (your description of the parser being shared\nwith the log family). What matters to the readers is that it is\naccepted as a courtesy (as opposed to being rejected as an error),\nbut it is unnecessary to give it in the first place.\n\nIf you followed the logic along, you would agree that it is a crime\nto list it in the SYNOPSIS section.\n\n\n[Footnote]\n\n*1* Unlike \"--follow\" checkbox hack, which follows renames correctly\nonly in a strictly linear history, \"blame\" maintains the filename\nbeing tracked per history traversal path and will follow a history\nlike this:\n\n ----A----B\n \\ \\\n C----D\n\nwhere you originally had your file as fileA, one side of the fork\nrenamed it to fileB while the other side of the fork renamed it to\nfileC, and a merge coalesced it to fileD. \"git blame fileD\" will\nfind the line-level origin across all these renames.\n\nTry this:\n\n git init\n printf \"%s\\n\" a a a a a >fileA\n git add fileA\n git commit -m A\n git branch side\n printf \"%s\\n\" a b b a a >fileB\n git add fileB\n rm fileA\n git commit -a -m B\n git checkout side\n printf \"%s\\n\" a a c c a >fileC\n git add fileC\n rm fileA\n git commit -a -m C\n git merge master\n git rm -f fileA fileB fileC\n printf \"%s\\n\" a b d c a >fileD\n git add fileD\n git commit -a -m D\n\nand then in the resulting history\n\n git blame fileD\n"},{"id":"199507","messageId":"20120919182715.GF11699@sigill.intra.peff.net","threadId":"31454","inReplyTo":"7v627aiq47.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-19T18:27:15Z","receivedAt":"2012-09-19T18:27:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 18, 2012 at 09:38:32PM -0700, Junio C Hamano wrote:\n\n> That is a totally wrong message to send. You failed to teach the\n> reader that there is no need to do anything special to tell the\n> command to follow per-line origin across renames.\n> \n> So if anything, I would phrase it this way instead:\n> \n> --follow::\n> This option is accepted but silently ignored. \"git blame\"\n> \t follows per-line origin across renames without any special\n> \t options, and there is no reason to use this option.\n\nI think that is much better than Drew's text. But I really wonder if the\nright solution is to simply disallow --follow. It does not do anything,\nand it is not documented. There is no special reason to think that it\nwould do anything, except by people who try it. So perhaps that is the\nright time to say \"no, this is not a valid option\".\n\nLike this (totally untested) patch:\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 0e102bf..412d6dd 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2365,6 +2365,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t\tctx.argv[0] = \"--children\";\n \t\t\treverse = 1;\n \t\t}\n+\t\telse if (!strcmp(ctx.argv[0], \"--follow\")) {\n+\t\t\terror(\"unknown option `--follow`\");\n+\t\t\tusage_with_options(blame_opt_usage, options);\n+\t\t}\n \t\tparse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n \t}\n parse_done:\n\n-Peff\n"},{"id":"199518","messageId":"7vzk4lg5yf.fsf@alter.siamese.dyndns.org","threadId":"31454","inReplyTo":"20120919182715.GF11699@sigill.intra.peff.net","subject":"Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-19T19:36:56Z","receivedAt":"2012-09-19T19:36:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 18, 2012 at 09:38:32PM -0700, Junio C Hamano wrote:\n>\n>> That is a totally wrong message to send. You failed to teach the\n>> reader that there is no need to do anything special to tell the\n>> command to follow per-line origin across renames.\n>> \n>> So if anything, I would phrase it this way instead:\n>> \n>> --follow::\n>> This option is accepted but silently ignored. \"git blame\"\n>> \t follows per-line origin across renames without any special\n>> \t options, and there is no reason to use this option.\n>\n> I think that is much better than Drew's text. But I really wonder if the\n> right solution is to simply disallow --follow. It does not do anything,\n> and it is not documented. There is no special reason to think that it\n> would do anything, except by people who try it. So perhaps that is the\n> right time to say \"no, this is not a valid option\".\n>\n> Like this (totally untested) patch:\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 0e102bf..412d6dd 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2365,6 +2365,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> \t\t\tctx.argv[0] = \"--children\";\n> \t\t\treverse = 1;\n> \t\t}\n> +\t\telse if (!strcmp(ctx.argv[0], \"--follow\")) {\n> +\t\t\terror(\"unknown option `--follow`\");\n> +\t\t\tusage_with_options(blame_opt_usage, options);\n> +\t\t}\n> \t\tparse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n> \t}\n> parse_done:\n\nThis patch would not hurt existing users very much; blame is an\nunlikely thing to run in scripts, and it is easy to remove the\nmisguided --follow from them.\n\nSo I am in general OK with it, but if we are to go that route, we\nshould make sure that the documentation makes it clear that blame\nfollows whole-file renames without any special instruction before\ndoing so. Otherwise, it again will send the same wrong message to\npeople who try to use the \"--follow\" from their experience with\n\"log\", no?\n"},{"id":"199520","messageId":"20120919194213.GB21950@sigill.intra.peff.net","threadId":"31454","inReplyTo":"7vzk4lg5yf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-19T19:42:13Z","receivedAt":"2012-09-19T19:42:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 19, 2012 at 12:36:56PM -0700, Junio C Hamano wrote:\n\n> > Like this (totally untested) patch:\n> >\n> > diff --git a/builtin/blame.c b/builtin/blame.c\n> > index 0e102bf..412d6dd 100644\n> > --- a/builtin/blame.c\n> > +++ b/builtin/blame.c\n> > @@ -2365,6 +2365,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> > \t\t\tctx.argv[0] = \"--children\";\n> > \t\t\treverse = 1;\n> > \t\t}\n> > +\t\telse if (!strcmp(ctx.argv[0], \"--follow\")) {\n> > +\t\t\terror(\"unknown option `--follow`\");\n> > +\t\t\tusage_with_options(blame_opt_usage, options);\n> > +\t\t}\n> > \t\tparse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n> > \t}\n> > parse_done:\n> \n> This patch would not hurt existing users very much; blame is an\n> unlikely thing to run in scripts, and it is easy to remove the\n> misguided --follow from them.\n\nI would not worry about such users. I am of the opinion that their\nscripts are buggy for calling a useless and undocumented option that\njust happened to not complain.\n\n> So I am in general OK with it, but if we are to go that route, we\n> should make sure that the documentation makes it clear that blame\n> follows whole-file renames without any special instruction before\n> doing so. Otherwise, it again will send the same wrong message to\n> people who try to use the \"--follow\" from their experience with\n> \"log\", no?\n\nI guess it depends on your perspective. I can see the argument that\nblame is already doing what --follow would ask for, and thus it is a\nno-op. I think of it more as --follow is nonsensical for blame. But I\ndo not think either is wrong per se, and there is no reason not to help\npeople who come to git thinking the former. So yes, I think\ndocumentation in either case is probably a good thing.\n\nI am a little lukewarm on my patch if only because of the precedent it\nsets. There are a trillion options that revision.c parses that are not\nnecessarily meaningful or implemented for sub-commands that piggy-back\non its option parser. I'm not sure we want to get into manually\ndetecting and disallowing each one in every caller.\n\n-Peff\n"},{"id":"199540","messageId":"C07F05AC-8FBF-4F09-AF13-A291181A06D9@sb.org","threadId":"31454","inReplyTo":"20120919194213.GB21950@sigill.intra.peff.net","subject":"Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2012-09-19T20:31:50Z","receivedAt":"2012-09-19T20:31:50Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Sep 19, 2012, at 12:42 PM, Jeff King wrote:\n\n>> So I am in general OK with it, but if we are to go that route, we\n>> should make sure that the documentation makes it clear that blame\n>> follows whole-file renames without any special instruction before\n>> doing so. Otherwise, it again will send the same wrong message to\n>> people who try to use the \"--follow\" from their experience with\n>> \"log\", no?\n> \n> I guess it depends on your perspective. I can see the argument that\n> blame is already doing what --follow would ask for, and thus it is a\n> no-op. I think of it more as --follow is nonsensical for blame. But I\n> do not think either is wrong per se, and there is no reason not to help\n> people who come to git thinking the former. So yes, I think\n> documentation in either case is probably a good thing.\n> \n> I am a little lukewarm on my patch if only because of the precedent it\n> sets. There are a trillion options that revision.c parses that are not\n> necessarily meaningful or implemented for sub-commands that piggy-back\n> on its option parser. I'm not sure we want to get into manually\n> detecting and disallowing each one in every caller.\n\nI tend to agree with your final sentiment there. But the point that\nusers may not realize that blame already follows is also valid. Perhaps\nwe should catch --follow, as in your patch, but instead of saying that\nit's an unknown argument, just print out a helpful message saying blame\nalready follows renames (and then continue with the blame anyway, so\nas to not set a precedent to abort on unknown-but-currently-accepted\nflags).\n\n-Kevin\n"},{"id":"199533","messageId":"20120919203738.GA24383@sigill.intra.peff.net","threadId":"31454","inReplyTo":"C07F05AC-8FBF-4F09-AF13-A291181A06D9@sb.org","subject":"Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-19T20:37:38Z","receivedAt":"2012-09-19T20:37:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 19, 2012 at 01:31:50PM -0700, Kevin Ballard wrote:\n\n> > I am a little lukewarm on my patch if only because of the precedent it\n> > sets. There are a trillion options that revision.c parses that are not\n> > necessarily meaningful or implemented for sub-commands that piggy-back\n> > on its option parser. I'm not sure we want to get into manually\n> > detecting and disallowing each one in every caller.\n> \n> I tend to agree with your final sentiment there. But the point that\n> users may not realize that blame already follows is also valid. Perhaps\n> we should catch --follow, as in your patch, but instead of saying that\n> it's an unknown argument, just print out a helpful message saying blame\n> already follows renames (and then continue with the blame anyway, so\n> as to not set a precedent to abort on unknown-but-currently-accepted\n> flags).\n\nSure, that would probably make sense. Care to roll a patch with\nsuggested wording?\n\n-Peff\n"},{"id":"199537","messageId":"819C66A4-390B-4C29-86BD-D4CB4DB43385@sb.org","threadId":"31454","inReplyTo":"20120919203738.GA24383@sigill.intra.peff.net","subject":"Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2012-09-19T21:09:25Z","receivedAt":"2012-09-19T21:09:25Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Sep 19, 2012, at 1:37 PM, Jeff King wrote:\n\n> On Wed, Sep 19, 2012 at 01:31:50PM -0700, Kevin Ballard wrote:\n> \n>>> I am a little lukewarm on my patch if only because of the precedent it\n>>> sets. There are a trillion options that revision.c parses that are not\n>>> necessarily meaningful or implemented for sub-commands that piggy-back\n>>> on its option parser. I'm not sure we want to get into manually\n>>> detecting and disallowing each one in every caller.\n>> \n>> I tend to agree with your final sentiment there. But the point that\n>> users may not realize that blame already follows is also valid. Perhaps\n>> we should catch --follow, as in your patch, but instead of saying that\n>> it's an unknown argument, just print out a helpful message saying blame\n>> already follows renames (and then continue with the blame anyway, so\n>> as to not set a precedent to abort on unknown-but-currently-accepted\n>> flags).\n> \n> Sure, that would probably make sense. Care to roll a patch with\n> suggested wording?\n\nSadly, no. I'm not in a position to contribute to GPL code anymore, based\non my current job (I'd have to jump through some hoops to get the ok\nto expose myself to that potential legal liability).\n\n-Kevin\n"},{"id":"199553","messageId":"7vfw6deeyz.fsf@alter.siamese.dyndns.org","threadId":"31454","inReplyTo":"20120919194213.GB21950@sigill.intra.peff.net","subject":"Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T00:05:08Z","receivedAt":"2012-09-20T00:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I guess it depends on your perspective. I can see the argument that\n> blame is already doing what --follow would ask for, and thus it is a\n> no-op. I think of it more as --follow is nonsensical for blame.\n\nIs \"--follow\" a nonsense in the context of blame? I am not so sure.\n\nI think it all boils down to this question:\n\n\tDoes \"blame\" that does not follow rename make sense?\n\nWhen you think about -M (allows us to keep track of the origin of\nlines inside a single file when they are moved around) and -C\n(allows us to keep track of the origin of lines that migrate from\nanother file), \"follow across whole-file rename\" is another optional\nmode of operation in the same class to tell the command to pay more\nprocessing cost to buy better precision. When you know what you are\ninterested in happened entirely inside a file that was never\nrenamed, \"blame -M\" without \"-C\" and without whole-file rename\ntracking is a sensible way to set that trade-off, even though we\ncurrently do not have a way to say \"--no-follow\".\n\nEventually we would review and accept a patch to fix \"--follow\" by\nsomebody and that patch will make \"--follow\" truly follows renames\nby keeping track of a single pathspec used to limit the changes per\nancestry traversal path, instead of switching one global one, which\nis the current hack does.\n\nOnce that happens, what \"--follow\" does will match exactly what the\ncurrent \"blame\" internally (and unconditionally) does. The current\n\"blame\" pays no attention to \"--follow\" because it wants to do the\nright thing without letting the broken \"--follow\" logic to take over\nand do a half-hearted job at following renames.\n\nSo if we were to do something special for \"--follow\" inside blame, I\nthink the right thing to do is probably to silently ignore, and in\naddition, accept \"--no-follow\" and disable the whole-file rename\ntracking logic. When a true \"--follow\" comes along, we may be able\nto rip out the whole-file rename tracking logic from \"blame\" and let\nthe version of \"--follow\" implemented correctly for the \"log\"\nfamily.\n"},{"id":"199686","messageId":"7v7grn5h1l.fsf_-_@alter.siamese.dyndns.org","threadId":"31454","inReplyTo":"20120919203738.GA24383@sigill.intra.peff.net","subject":"Re* [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-21T19:09:42Z","receivedAt":"2012-09-21T19:09:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Sep 19, 2012 at 01:31:50PM -0700, Kevin Ballard wrote:\n>\n>> > I am a little lukewarm on my patch if only because of the precedent it\n>> > sets. There are a trillion options that revision.c parses that are not\n>> > necessarily meaningful or implemented for sub-commands that piggy-back\n>> > on its option parser. I'm not sure we want to get into manually\n>> > detecting and disallowing each one in every caller.\n>> \n>> I tend to agree with your final sentiment there. But the point that\n>> users may not realize that blame already follows is also valid. Perhaps\n>> we should catch --follow, as in your patch, but instead of saying that\n>> it's an unknown argument, just print out a helpful message saying blame\n>> already follows renames (and then continue with the blame anyway, so\n>> as to not set a precedent to abort on unknown-but-currently-accepted\n>> flags).\n>\n> Sure, that would probably make sense. Care to roll a patch with\n> suggested wording?\n\nLet's do this for now instead. That would make it clear to people\nwho (rightly or wrongly) think the \"--follow\" option should do\nsomething that we already do so, and explain the output that they\nsee when they do give the \"--follow\" option to the command.\n\nI may do a \"--no-follow\" patch as a follow-up, or I may not,\ndepending on the mood and workload.\n\n\n Documentation/git-blame.txt | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git c/Documentation/git-blame.txt w/Documentation/git-blame.txt\nindex 7ee9236..809823e 100644\n--- c/Documentation/git-blame.txt\n+++ w/Documentation/git-blame.txt\n@@ -20,6 +20,12 @@ last modified the line. Optionally, start annotating from the given revision.\n \n The command can also limit the range of lines annotated.\n \n+The origin of lines is automatically followed across whole-file\n+renames (currently there is no option to turn the rename-following\n+off). To follow lines moved from one file to another, or to follow\n+lines that were copied and pasted from another file, etc., see the\n+`-C` and `-M` options.\n+\n The report does not tell you anything about lines which have been deleted or\n replaced; you need to use a tool such as 'git diff' or the \"pickaxe\"\n interface briefly mentioned in the following paragraph.\n"},{"id":"199700","messageId":"7vpq5f3xct.fsf@alter.siamese.dyndns.org","threadId":"31454","inReplyTo":"7v7grn5h1l.fsf_-_@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH] Documentation/git-blame.txt: --follow is a NO-OP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-21T21:00:18Z","receivedAt":"2012-09-21T21:00:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Let's do this for now instead. That would make it clear to people\n> who (rightly or wrongly) think the \"--follow\" option should do\n> something that we already do so, and explain the output that they\n> see when they do give the \"--follow\" option to the command.\n>\n> I may do a \"--no-follow\" patch as a follow-up, or I may not,\n> depending on the mood and workload.\n\nA patch to do so looks like this.\n\nIf you know your history did not have any rename, or if you care\nonly about the history after a large rename that happened some time\nago, \"git blame --no-follow $path\" can be a way to tell the command\nnot to bother about them.\n\nWhen you use -C, the lines that came from the renamed file will\nstill be found without the whole-file rename detection anyway, and\nthis is not all that interesting either way, I would think.\n\n\ndiff --git c/builtin/blame.c w/builtin/blame.c\nindex cad4111..bfa6086 100644\n--- c/builtin/blame.c\n+++ w/builtin/blame.c\n@@ -42,6 +42,7 @@ static int blank_boundary;\n static int incremental;\n static int xdl_opts;\n static int abbrev = -1;\n+static int no_whole_file_rename;\n \n static enum date_mode blame_date_mode = DATE_ISO8601;\n static size_t blame_date_width;\n@@ -1226,7 +1227,7 @@ static void pass_blame(struct scoreboard *sb, struct origin *origin, int opt)\n \t * The first pass looks for unrenamed path to optimize for\n \t * common cases, then we look for renames in the second pass.\n \t */\n-\tfor (pass = 0; pass < 2; pass++) {\n+\tfor (pass = 0; pass < 2 - no_whole_file_rename; pass++) {\n \t\tstruct origin *(*find)(struct scoreboard *,\n \t\t\t\t struct commit *, struct origin *);\n \t\tfind = pass ? find_rename : find_origin;\n@@ -2344,6 +2345,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tinit_revisions(&revs, NULL);\n \trevs.date_mode = blame_date_mode;\n \tDIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);\n+\tDIFF_OPT_SET(&revs.diffopt, FOLLOW_RENAMES);\n \n \tsave_commit_buffer = 0;\n \tdashdash_pos = 0;\n@@ -2367,6 +2369,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tparse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n \t}\n parse_done:\n+\tno_whole_file_rename = !DIFF_OPT_TST(&revs.diffopt, FOLLOW_RENAMES);\n+\tDIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);\n \targc = parse_options_end(&ctx);\n \n \tif (0 < abbrev)\ndiff --git c/diff.c w/diff.c\nindex f1b0447..32ebcbb 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -3584,6 +3584,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_SET(options, FIND_COPIES_HARDER);\n \telse if (!strcmp(arg, \"--follow\"))\n \t\tDIFF_OPT_SET(options, FOLLOW_RENAMES);\n+\telse if (!strcmp(arg, \"--no-follow\"))\n+\t\tDIFF_OPT_CLR(options, FOLLOW_RENAMES);\n \telse if (!strcmp(arg, \"--color\"))\n \t\toptions->use_color = 1;\n \telse if (!prefixcmp(arg, \"--color=\")) {\n"}]}