Re: [PATCH] blame: can specify shas of commits to ignore on command line

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 04.05.2010 04:21, schrieb Dylan Reid:
> Add the ability for git-blame to ignore changes that occur in
> certain commits.  The new "-I <sha>" argument can be used to specify
> a commit that should be ignored.  This is useful if you have a
> few commits that you know didn't cause a problem, for example you
> switched from "u8" to "uint8_t" and it messed up your history.

Only the long option --ignore-commit works with your patch, -I doesn't.

>    Allow multiple commits to be specified and store an array in the
> scoreboard structure so it is accessible.  Add should_ignore_commit
> function to check if a commit should be ignored. Call
> "should_ignore_commit" from blame_overlap and if the commit should
> be ignored then pass all the blame on to the parent of the ignored
> commit.  This is done by calling "pass_whole_blame" which has been
> relocated to a above blame_overlap, but is unchanged.
> 
> Signed-off-by: Dylan Reid <dgreid@gmail.com>
> ---
>  Documentation/blame-options.txt |    6 ++
>  builtin/blame.c                 |  124 +++++++++++++++++++++++++++++----------
>  t/t8003-blame.sh                |   42 +++++++++++++
>  3 files changed, 141 insertions(+), 31 deletions(-)

Oh, nice!  And with tests and documentation! :)

> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> index d820569..eb9c825 100644
> --- a/Documentation/blame-options.txt
> +++ b/Documentation/blame-options.txt
> @@ -52,6 +52,12 @@ of lines before or after the line given by <start>.
>  --porcelain::
>  	Show in a format designed for machine consumption.
>  
> +--ignore-commit <sha>::
> +	Ignore the specified commit when assigning blame.  This is
> +	useful if there are commits in the history that make non
> +	functional changes to the lines you are interested in
> +	finding blame for.
> +
>  --incremental::
>  	Show the result incrementally in a format designed for
>  	machine consumption.
> diff --git a/builtin/blame.c b/builtin/blame.c
> index fc15863..e4bd095 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -176,6 +176,14 @@ struct blame_entry {
>  };
>  
>  /*
> + * List of any commits to ignore
> + */
> +struct ignore_commits {
> +	unsigned char (*ignore_shas)[20];
> +	unsigned num_ignore_shas;
> +};
> +
> +/*
>   * The current state of the blame assignment.
>   */
>  struct scoreboard {
> @@ -198,6 +206,9 @@ struct scoreboard {
>  	/* look-up a line in the final buffer */
>  	int num_lines;
>  	int *lineno;
> +
> +	/* List of the shas of commits that should be ignored */
> +	struct ignore_commits *ignored_commits;
>  };
>  
>  static inline int same_suspect(struct origin *a, struct origin *b)
> @@ -653,6 +664,44 @@ static void decref_split(struct blame_entry *split)
>  }
>  
>  /*
> + * The blobs of origin and porigin exactly match, so everything
> + * origin is suspected for can be blamed on the parent.
> + */
> +static void pass_whole_blame(struct scoreboard *sb,
> +			     struct origin *origin, struct origin *porigin)
> +{
> +   struct blame_entry *e;
> +
> +   if (!porigin->file.ptr && origin->file.ptr) {
> +      /* Steal its file */
> +      porigin->file = origin->file;
> +      origin->file.ptr = NULL;
> +   }
> +   for (e = sb->ent; e; e = e->next) {
> +      if (!same_suspect(e->suspect, origin))
> +	 continue;
> +      origin_incref(porigin);
> +      origin_decref(e->suspect);
> +      e->suspect = porigin;
> +   }
> +}

This function was moved from below, but it seems to be indented with
three spaces instead of tabs now.  Adding a declaration without moving
the function would avoid that and result in a smaller patch.

> +
> +/*
> + * Helper to determine if the given commit should be ignored
> + */
> +static unsigned should_ignore_commit(const struct scoreboard *sb,
> +				     struct commit *commit)
> +{
> +	unsigned i;
> +	unsigned num_shas = sb->ignored_commits->num_ignore_shas;
> +	for(i = 0; i < num_shas; i++)
> +		if(!hashcmp(commit->object.sha1,
> +			    sb->ignored_commits->ignore_shas[i]))
> +			return 1;
> +	return 0;
> +}
> +
> +/*
>   * Helper for blame_chunk().  blame_entry e is known to overlap with
>   * the patch hunk; split it and pass blame to the parent.
>   */
> @@ -660,12 +709,15 @@ static void blame_overlap(struct scoreboard *sb, struct blame_entry *e,
>  			  int tlno, int plno, int same,
>  			  struct origin *parent)
>  {
> -	struct blame_entry split[3];
> -
> -	split_overlap(split, e, tlno, plno, same, parent);
> -	if (split[1].suspect)
> -		split_blame(sb, split, e);
> -	decref_split(split);
> +	if(should_ignore_commit(sb, e->suspect->commit))
> +		pass_whole_blame(sb, e->suspect, parent);
> +	else {
> +		struct blame_entry split[3];
> +		split_overlap(split, e, tlno, plno, same, parent);
> +		if (split[1].suspect)
> +			split_blame(sb, split, e);
> +		decref_split(split);
> +	}
>  }
>  
>  /*
> @@ -1105,29 +1157,6 @@ static int find_copy_in_parent(struct scoreboard *sb,
>  }
>  
>  /*
> - * The blobs of origin and porigin exactly match, so everything
> - * origin is suspected for can be blamed on the parent.
> - */
> -static void pass_whole_blame(struct scoreboard *sb,
> -			     struct origin *origin, struct origin *porigin)
> -{
> -	struct blame_entry *e;
> -
> -	if (!porigin->file.ptr && origin->file.ptr) {
> -		/* Steal its file */
> -		porigin->file = origin->file;
> -		origin->file.ptr = NULL;
> -	}
> -	for (e = sb->ent; e; e = e->next) {
> -		if (!same_suspect(e->suspect, origin))
> -			continue;
> -		origin_incref(porigin);
> -		origin_decref(e->suspect);
> -		e->suspect = porigin;
> -	}
> -}
> -
> -/*
>   * We pass blame from the current commit to its parents.  We keep saying
>   * "parent" (and "porigin"), but what we mean is to find scapegoat to
>   * exonerate ourselves.
> @@ -1540,8 +1569,14 @@ static void assign_blame(struct scoreboard *sb, int opt)
>  
>  		/* Take responsibility for the remaining entries */
>  		for (ent = sb->ent; ent; ent = ent->next)
> -			if (same_suspect(ent->suspect, suspect))
> -				found_guilty_entry(ent);
> +			if (same_suspect(ent->suspect, suspect)) {
> +				if (should_ignore_commit(sb, commit) &&
> +				    ent->suspect->previous)
> +					pass_whole_blame(sb, ent->suspect,
> +							 ent->suspect->previous);
> +				else
> +					found_guilty_entry(ent);
> +		   }

An ignored commit can still be blamed if there is no other commit to
pass it on.  So e.g. the initial commit for the file could end up being
blamed for lines that were added by later commits which are being
ignored.  That may look confusing.

Would it make sense to pass the blame to some kind of null commit, i.e.
a special marker that says "I could tell you who is to blame for this
line but you said you don't want to know"?

René