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

From: Jeff King <peff@peff.net>
Date: Sep 19, 2012, 19:42 UTC
Message-ID: <20120919194213.GB21950@sigill.intra.peff.net>
In-Reply-To: <7vzk4lg5yf.fsf@alter.siamese.dyndns.org>

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

Show 21 quoted lines

> > 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.

Show 6 quoted lines

> 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