Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP

Jeff King [off-list ref] writes:

This patch would not hurt existing users very much; blame is an
unlikely thing to run in scripts, and it is easy to remove the
misguided --follow from them.

So I am in general OK with it, but if we are to go that route, we
should make sure that the documentation makes it clear that blame
follows whole-file renames without any special instruction before
doing so.  Otherwise, it again will send the same wrong message to
people who try to use the "--follow" from their experience with
"log", no?

On Wed, Sep 19, 2012 at 12:36:56PM -0700, Junio C Hamano wrote:

quoted

Like this (totally untested) patch:

diff --git a/builtin/blame.c b/builtin/blame.c
index 0e102bf..412d6dd 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -2365,6 +2365,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 			ctx.argv[0] = "--children";
 			reverse = 1;
 		}
+		else if (!strcmp(ctx.argv[0], "--follow")) {
+			error("unknown option `--follow`");
+			usage_with_options(blame_opt_usage, options);
+		}
 		parse_revision_opt(&revs, &ctx, options, blame_opt_usage);
 	}
 parse_done:

This patch would not hurt existing users very much; blame is an
unlikely thing to run in scripts, and it is easy to remove the
misguided --follow from them.

I would not worry about such users. I am of the opinion that their
scripts are buggy for calling a useless and undocumented option that
just happened to not complain.

So I am in general OK with it, but if we are to go that route, we
should make sure that the documentation makes it clear that blame
follows whole-file renames without any special instruction before
doing so.  Otherwise, it again will send the same wrong message to
people who try to use the "--follow" from their experience with
"log", no?

I guess it depends on your perspective. I can see the argument that
blame is already doing what --follow would ask for, and thus it is a
no-op. I think of it more as --follow is nonsensical for blame. But I
do not think either is wrong per se, and there is no reason not to help
people who come to git thinking the former. So yes, I think
documentation in either case is probably a good thing.

I am a little lukewarm on my patch if only because of the precedent it
sets.  There are a trillion options that revision.c parses that are not
necessarily meaningful or implemented for sub-commands that piggy-back
on its option parser. I'm not sure we want to get into manually
detecting and disallowing each one in every caller.

-Peff

On Wed, Sep 19, 2012 at 01:31:50PM -0700, Kevin Ballard wrote:

quoted

I am a little lukewarm on my patch if only because of the precedent it
sets.  There are a trillion options that revision.c parses that are not
necessarily meaningful or implemented for sub-commands that piggy-back
on its option parser. I'm not sure we want to get into manually
detecting and disallowing each one in every caller.

I tend to agree with your final sentiment there. But the point that
users may not realize that blame already follows is also valid. Perhaps
we should catch --follow, as in your patch, but instead of saying that
it's an unknown argument, just print out a helpful message saying blame
already follows renames (and then continue with the blame anyway, so
as to not set a precedent to abort on unknown-but-currently-accepted
flags).

Sure, that would probably make sense. Care to roll a patch with
suggested wording?

-Peff

On Sep 19, 2012, at 1:37 PM, Jeff King [off-list ref] wrote:

On Wed, Sep 19, 2012 at 01:31:50PM -0700, Kevin Ballard wrote:

quoted

quoted

I am a little lukewarm on my patch if only because of the precedent it
sets.  There are a trillion options that revision.c parses that are not
necessarily meaningful or implemented for sub-commands that piggy-back
on its option parser. I'm not sure we want to get into manually
detecting and disallowing each one in every caller.

I tend to agree with your final sentiment there. But the point that
users may not realize that blame already follows is also valid. Perhaps
we should catch --follow, as in your patch, but instead of saying that
it's an unknown argument, just print out a helpful message saying blame
already follows renames (and then continue with the blame anyway, so
as to not set a precedent to abort on unknown-but-currently-accepted
flags).

Sure, that would probably make sense. Care to roll a patch with
suggested wording?

Sadly, no. I'm not in a position to contribute to GPL code anymore, based
on my current job (I'd have to jump through some hoops to get the ok
to expose myself to that potential legal liability).

-Kevin

On Sep 19, 2012, at 12:42 PM, Jeff King [off-list ref] wrote:

quoted

So I am in general OK with it, but if we are to go that route, we
should make sure that the documentation makes it clear that blame
follows whole-file renames without any special instruction before
doing so.  Otherwise, it again will send the same wrong message to
people who try to use the "--follow" from their experience with
"log", no?

I guess it depends on your perspective. I can see the argument that
blame is already doing what --follow would ask for, and thus it is a
no-op. I think of it more as --follow is nonsensical for blame. But I
do not think either is wrong per se, and there is no reason not to help
people who come to git thinking the former. So yes, I think
documentation in either case is probably a good thing.

I am a little lukewarm on my patch if only because of the precedent it
sets.  There are a trillion options that revision.c parses that are not
necessarily meaningful or implemented for sub-commands that piggy-back
on its option parser. I'm not sure we want to get into manually
detecting and disallowing each one in every caller.

I tend to agree with your final sentiment there. But the point that
users may not realize that blame already follows is also valid. Perhaps
we should catch --follow, as in your patch, but instead of saying that
it's an unknown argument, just print out a helpful message saying blame
already follows renames (and then continue with the blame anyway, so
as to not set a precedent to abort on unknown-but-currently-accepted
flags).

-Kevin