Re: [PATCH] git-pickaxe: blame rewritten.

From: Junio C Hamano <junkio@cox.net>
Date: Oct 12, 2006, 19:09 UTC
Message-ID: <7viripnyfh.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0610120854440.3952@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

Show 13 quoted lines

> Btw, could we please get rid of this horrible command line syntax.
>
> Pretty much _every_ other git command takes the form
>
> 	git cmd [<rev>] [--] <path>
>
> but for some reason annotate and blame (and now pickaxe) do it the wrong 
> way around, and do
>
> 	git cmd [--] <path> [<rev>]
>
> which is just irritating to somebody who has grown very used to being able 
> to specify revisions first.

Side note: I do not intend to keep it named pickaxe -- only while it is in "pu".

I think we could support all of them to retain backward compatibility:

	git blame [-options] <path> [<rev>]		(*1*)
	git blame [-options] -- <path> [<rev>]		(*2*)
	git blame [-options] [<rev>] [--] <path>	(*3*)

(*1*) Only for path that does not start with a '-'; we should
      tighten the input to make sure <path> lstat()'s fine
      (which we currently do with pickaxe), <path> cannot be
      interpreted as a valud rev, and when <rev> is given, <rev>
      does not lstat() Ok, to avoid ambiguity.  Other cases we
      should require the newer format with explicit -- (*3*).

(*2*) Backward compatible canoncal format.  The above comment
      for validation does not apply, as (*3*) never has more
      than one path for 'annotate/blame/pickaxe'

(*3*) The canonical format for ohter git commands.  Without
      an explicit --, <rev> should not lstat() Ok, and <path>
      should, to avoid ambiguity.

Show 5 quoted lines

> (I'd actually also like to have a range-modifier, so that I could do
>
> 	git annotate --since=2.weeks.ago v2.6.18.. <path>
>
> that didn't go back beyond a certain point,...

I am not sure about revision bottom (v.2.6.18..) offhand, but the age limit (--since=2.weeks) should be trivial.

Inside pass_blame() while we iterate over the parents of the suspect we are looking at, you can skip the parent if it is older than the age limit, or an ancestor of revision bottom, like this:

--- l/builtin-pickaxe.c +++ k/builtin-pickaxe.c

@@ -450,6 +450,12 @@ static void pass_blame(struct scoreboard
 	     parent = parent->next, parent_ix++) {
 		if (parse_commit(parent->item))
 			continue;
+
+		if (parent is older than age limit)
+			continue;
+		if (parent is an ancestor of revision bottom)
+			continue;
+
 		porigin = find_origin(sb, parent->item, origin->path);
 		if (!porigin)
 			porigin = find_rename(sb, parent->item, origin);


I think we can get away by checking if the parent _is_ the
revision bottom (or one of the bottoms, if you say "--not
v2.6.18 v2.6.17.13") instead of doing "is it an ancestor" check,
in practice.  But that is not correct when a merge is involved.