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

2012-09-21 Thread Junio C Hamano
Jeff King p...@peff.net writes:

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

  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?

Let's do this for now instead.  That would make it clear to people
who (rightly or wrongly) think the --follow option should do
something that we already do so, and explain the output that they
see when they do give the --follow option to the command.

I may do a --no-follow patch as a follow-up, or I may not,
depending on the mood and workload.


 Documentation/git-blame.txt | 6 ++
 1 file changed, 6 insertions(+)

diff --git c/Documentation/git-blame.txt w/Documentation/git-blame.txt
index 7ee9236..809823e 100644
--- c/Documentation/git-blame.txt
+++ w/Documentation/git-blame.txt
@@ -20,6 +20,12 @@ last modified the line. Optionally, start annotating from 
the given revision.
 
 The command can also limit the range of lines annotated.
 
+The origin of lines is automatically followed across whole-file
+renames (currently there is no option to turn the rename-following
+off). To follow lines moved from one file to another, or to follow
+lines that were copied and pasted from another file, etc., see the
+`-C` and `-M` options.
+
 The report does not tell you anything about lines which have been deleted or
 replaced; you need to use a tool such as 'git diff' or the pickaxe
 interface briefly mentioned in the following paragraph.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-21 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 Let's do this for now instead.  That would make it clear to people
 who (rightly or wrongly) think the --follow option should do
 something that we already do so, and explain the output that they
 see when they do give the --follow option to the command.

 I may do a --no-follow patch as a follow-up, or I may not,
 depending on the mood and workload.

A patch to do so looks like this.

If you know your history did not have any rename, or if you care
only about the history after a large rename that happened some time
ago, git blame --no-follow $path can be a way to tell the command
not to bother about them.

When you use -C, the lines that came from the renamed file will
still be found without the whole-file rename detection anyway, and
this is not all that interesting either way, I would think.


diff --git c/builtin/blame.c w/builtin/blame.c
index cad4111..bfa6086 100644
--- c/builtin/blame.c
+++ w/builtin/blame.c
@@ -42,6 +42,7 @@ static int blank_boundary;
 static int incremental;
 static int xdl_opts;
 static int abbrev = -1;
+static int no_whole_file_rename;
 
 static enum date_mode blame_date_mode = DATE_ISO8601;
 static size_t blame_date_width;
@@ -1226,7 +1227,7 @@ static void pass_blame(struct scoreboard *sb, struct 
origin *origin, int opt)
 * The first pass looks for unrenamed path to optimize for
 * common cases, then we look for renames in the second pass.
 */
-   for (pass = 0; pass  2; pass++) {
+   for (pass = 0; pass  2 - no_whole_file_rename; pass++) {
struct origin *(*find)(struct scoreboard *,
   struct commit *, struct origin *);
find = pass ? find_rename : find_origin;
@@ -2344,6 +2345,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
init_revisions(revs, NULL);
revs.date_mode = blame_date_mode;
DIFF_OPT_SET(revs.diffopt, ALLOW_TEXTCONV);
+   DIFF_OPT_SET(revs.diffopt, FOLLOW_RENAMES);
 
save_commit_buffer = 0;
dashdash_pos = 0;
@@ -2367,6 +2369,8 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
parse_revision_opt(revs, ctx, options, blame_opt_usage);
}
 parse_done:
+   no_whole_file_rename = !DIFF_OPT_TST(revs.diffopt, FOLLOW_RENAMES);
+   DIFF_OPT_CLR(revs.diffopt, FOLLOW_RENAMES);
argc = parse_options_end(ctx);
 
if (0  abbrev)
diff --git c/diff.c w/diff.c
index f1b0447..32ebcbb 100644
--- c/diff.c
+++ w/diff.c
@@ -3584,6 +3584,8 @@ int diff_opt_parse(struct diff_options *options, const 
char **av, int ac)
DIFF_OPT_SET(options, FIND_COPIES_HARDER);
else if (!strcmp(arg, --follow))
DIFF_OPT_SET(options, FOLLOW_RENAMES);
+   else if (!strcmp(arg, --no-follow))
+   DIFF_OPT_CLR(options, FOLLOW_RENAMES);
else if (!strcmp(arg, --color))
options-use_color = 1;
else if (!prefixcmp(arg, --color=)) {
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-19 Thread Jeff King
On Tue, Sep 18, 2012 at 09:38:32PM -0700, Junio C Hamano wrote:

 That is a totally wrong message to send.  You failed to teach the
 reader that there is no need to do anything special to tell the
 command to follow per-line origin across renames.
 
 So if anything, I would phrase it this way instead:
 
 --follow::
   This option is accepted but silently ignored.  git blame
 follows per-line origin across renames without any special
 options, and there is no reason to use this option.

I think that is much better than Drew's text. But I really wonder if the
right solution is to simply disallow --follow. It does not do anything,
and it is not documented. There is no special reason to think that it
would do anything, except by people who try it. So perhaps that is the
right time to say no, this is not a valid option.

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:

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-19 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 On Tue, Sep 18, 2012 at 09:38:32PM -0700, Junio C Hamano wrote:

 That is a totally wrong message to send.  You failed to teach the
 reader that there is no need to do anything special to tell the
 command to follow per-line origin across renames.
 
 So if anything, I would phrase it this way instead:
 
 --follow::
   This option is accepted but silently ignored.  git blame
follows per-line origin across renames without any special
options, and there is no reason to use this option.

 I think that is much better than Drew's text. But I really wonder if the
 right solution is to simply disallow --follow. It does not do anything,
 and it is not documented. There is no special reason to think that it
 would do anything, except by people who try it. So perhaps that is the
 right time to say no, this is not a valid option.

 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.

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?

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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

  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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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

  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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-19 Thread Kevin Ballard
On Sep 19, 2012, at 1:37 PM, Jeff King p...@peff.net wrote:

 On Wed, Sep 19, 2012 at 01:31:50PM -0700, Kevin Ballard wrote:
 
 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-19 Thread Kevin Ballard
On Sep 19, 2012, at 12:42 PM, Jeff King p...@peff.net wrote:

 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-19 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 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.

Is --follow a nonsense in the context of blame?  I am not so sure.

I think it all boils down to this question:

Does blame that does not follow rename make sense?

When you think about -M (allows us to keep track of the origin of
lines inside a single file when they are moved around) and -C
(allows us to keep track of the origin of lines that migrate from
another file), follow across whole-file rename is another optional
mode of operation in the same class to tell the command to pay more
processing cost to buy better precision.  When you know what you are
interested in happened entirely inside a file that was never
renamed, blame -M without -C and without whole-file rename
tracking is a sensible way to set that trade-off, even though we
currently do not have a way to say --no-follow.

Eventually we would review and accept a patch to fix --follow by
somebody and that patch will make --follow truly follows renames
by keeping track of a single pathspec used to limit the changes per
ancestry traversal path, instead of switching one global one, which
is the current hack does.

Once that happens, what --follow does will match exactly what the
current blame internally (and unconditionally) does. The current
blame pays no attention to --follow because it wants to do the
right thing without letting the broken --follow logic to take over
and do a half-hearted job at following renames.

So if we were to do something special for --follow inside blame, I
think the right thing to do is probably to silently ignore, and in
addition, accept --no-follow and disable the whole-file rename
tracking logic.  When a true --follow comes along, we may be able
to rip out the whole-file rename tracking logic from blame and let
the version of --follow implemented correctly for the log
family.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-18 Thread Junio C Hamano
Drew Northup n1xim.em...@gmail.com writes:

 Make note that while the --follow option is accepted by git blame it does
 nothing.

 Signed-off-by: Drew Northup n1xim.em...@gmail.com
 ---
  Documentation/git-blame.txt | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
 index 7ee9236..7465bd8 100644
 --- a/Documentation/git-blame.txt
 +++ b/Documentation/git-blame.txt
 @@ -9,7 +9,7 @@ SYNOPSIS
  
  [verse]
  'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] 
 [--incremental] [-L n,m]
 - [-S revs-file] [-M] [-C] [-C] [-C] [--since=date] [--abbrev=n]
 + [-S revs-file] [-M] [-C] [-C] [-C] [--since=date] 
 [--abbrev=n] [--follow]
   [rev | --contents file | --reverse rev] [--] file
  
  DESCRIPTION
 @@ -78,6 +78,9 @@ include::blame-options.txt[]
   abbreviated object name, use n+1 digits. Note that 1 column
   is used for a caret to mark the boundary commit.
  
 +--follow::
 +NO-OP accepted due to using the option parser also used by
 +'git log'

This is triply questionable.  If it is a useless NO-OP, it shouldn't
be advertised in the SYNOPSIS section to begin with.

Your --follow is a no-op for blame is technically correct, but I
think the only ones that can appreciate that technical correctness
are those like you who know why we can get away by having a
seemingly no-op --follow option without losing functionality.

git blame follows the whole-file renames correctly [*1*], without
any -M/-C options.  There is no _need_ to use the keep one global
filename to follow, and switch to a different filename when it
disappears hack the --follow code uses.  That is the reason why
blame pays no attention to the --follow option.  You know that,
and that is why you think it is a sane thing to describe it in
technically correct way.

But I think most of the readers of the documentation are not aware
of that true reason why it can be a No-op.  Worse yet, they may
have heard of the --follow option that the log command has from
any of the numerous misguided web pages, and are led to believe that
the --follow option is a true feature, not a checkbox hack.

If readers come from that background, thinking --follow is the way
to tell Git to follow renames, what message does your description
send them?  I would read it as git blame accepts --follow from the
command line, but it is a no-op.  There is no way to make it follow
renames.

That is a totally wrong message to send.  You failed to teach the
reader that there is no need to do anything special to tell the
command to follow per-line origin across renames.

So if anything, I would phrase it this way instead:

--follow::
  This option is accepted but silently ignored.  git blame
  follows per-line origin across renames without any special
  options, and there is no reason to use this option.

It does not matter to the reader why it is accepted by the parser at
the mechanical level (your description of the parser being shared
with the log family).  What matters to the readers is that it is
accepted as a courtesy (as opposed to being rejected as an error),
but it is unnecessary to give it in the first place.

If you followed the logic along, you would agree that it is a crime
to list it in the SYNOPSIS section.


[Footnote]

*1* Unlike --follow checkbox hack, which follows renames correctly
only in a strictly linear history, blame maintains the filename
being tracked per history traversal path and will follow a history
like this:

AB
 \\
  CD

where you originally had your file as fileA, one side of the fork
renamed it to fileB while the other side of the fork renamed it to
fileC, and a merge coalesced it to fileD.  git blame fileD will
find the line-level origin across all these renames.

Try this:

git init
printf %s\n a a a a a fileA
git add fileA
git commit -m A
git branch side
printf %s\n a b b a a fileB
git add fileB
rm fileA
git commit -a -m B
git checkout side
printf %s\n a a c c a fileC
git add fileC
rm fileA
git commit -a -m C
git merge master
git rm -f fileA fileB fileC
printf %s\n a b d c a fileD
git add fileD
git commit -a -m D

and then in the resulting history

git blame fileD

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html