Re: [PATCH v5] blame: print unblamable and ignored commits in porcelain mode

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Karthik

On 03/04/2025 17:03, Karthik Nayak wrote:
> The 'git-blame(1)' command allows users to ignore specific revisions via
> the '--ignore-rev <rev>' and '--ignore-revs-file <file>' flags. These
> flags are often combined with the 'blame.markIgnoredLines' and
> 'blame.markUnblamableLines' config options. These config options prefix
> ignored and unblamable lines with a '?' and '*', respectively.
> 
> However, this option was never extended to the porcelain mode of
> 'git-blame(1)'. Since the documentation does not indicate this
> exclusion, it is a bug.
> 
> Fix this by printing 'ignored' and 'unblamable' respectively for the
> options when using the porcelain modes.

This looks good to me

Best Wishes

Phillip

> Helped-by: Patrick Steinhardt <ps@pks.im>
> Helped-by: Toon Claes <toon@iotcl.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Changes in v5:
> - Fix the test to be more portable by not using '\n' in 'sed'.
> - Link to v4: https://lore.kernel.org/all/20250330204339.191382-1-karthik.188@gmail.com/
> 
> Changes in v4:
> - Remove extra newline in 'puts'. Modify the test to compare the
>    entire output, the earlier test missed the extraneous newline.
> - Link to v3:
> https://lore.kernel.org/r/20250329-514-git-blame-1-s-porcelain-output-does-not-emit-unblamable-and-ignored-markers-v3-1-10f695ae519a@gmail.com
> 
> Changes in v3:
> - Use double-qoutes in the test to ensure correct variable dereference.
> - Fix incorrect test name.
> - Rename the function from 'emit_per_line_details()' to
>    'emit_porcelain_per_line_details()' to be more descriptive.
> - Ues 'puts()' instead of 'printf()'.
> - Link to v2:
> https://lore.kernel.org/r/20250326-514-git-blame-1-s-porcelain-output-does-not-emit-unblamable-and-ignored-markers-v2-1-79037e17a74b@gmail.com
> 
> Changes in v2:
> - Instead of printing the markers before the SHA in porcelain
>    mode and breaking scripts and backward compatability, let's
>    instead add a newline printing 'unblamable' or 'ignored'.
>    This is printed per line in both the porcelain modes.
> - Link to v1:
> https://lore.kernel.org/r/20250321-514-git-blame-1-s-porcelain-output-does-not-emit-unblamable-and-ignored-markers-v1-1-44b562d9beb8@gmail.com
> ---
> Range-diff versus v4:
> 
> 1:  5250fb436e ! 1:  43bc55bffe blame: print unblamable and ignored commits in porcelain mode
>      @@ Commit message
>       
>           Helped-by: Patrick Steinhardt <ps@pks.im>
>           Helped-by: Toon Claes <toon@iotcl.com>
>      +    Helped-by: Phillip Wood <phillip.wood123@gmail.com>
>           Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>       
>        ## Documentation/blame-options.adoc ##
>      @@ t/t8013-blame-ignore-revs.sh: test_expect_success mark_unblamable_lines '
>        
>       +for opt in --porcelain --line-porcelain
>       +do
>      -+	test_expect_success "mark_unblamable_lines with $opt" '
>      ++	test_expect_success "mark_unblamable_lines with $opt" "
>       +		sha=$(git rev-parse Y) &&
>       +
>       +		git -c blame.markUnblamableLines=false blame $opt --ignore-rev Y file >raw &&
>      -+		sed -e "s/^\ty3/unblamable\n&/" raw >expect &&
>      -+		cp expect raw &&
>      -+		sed -e "s/^\ty4/unblamable\n&/" raw >expect &&
>      ++		cat > sedscript <<- 'EOF' &&
>      ++		/^	y3/i\\
>      ++		unblamable
>      ++		/^	y4/i\\
>      ++		unblamable
>      ++		EOF
>      ++		sed -f sedscript raw >expect &&
>       +
>       +		git -c blame.markUnblamableLines=true blame $opt --ignore-rev Y file >actual &&
>       +		test_cmp expect actual
>      -+	'
>      ++	"
>       +done
>       +
>        # Commit Z will touch the first two lines.  Y touched all four.
>      @@ t/t8013-blame-ignore-revs.sh: test_expect_success mark_ignored_lines '
>        
>       +for opt in --porcelain --line-porcelain
>       +do
>      -+	test_expect_success "mark_ignored_lines with $opt" '
>      ++	test_expect_success "mark_ignored_lines with $opt" "
>       +		sha=$(git rev-parse Y) &&
>       +
>       +		git -c blame.markIgnoredLines=false blame $opt --ignore-rev Z file >raw &&
>      -+		sed -e "s/^\tline-one-Z/ignored\n&/" raw >expect &&
>      -+		cp expect raw &&
>      -+		sed -e "s/^\tline-two-Z/ignored\n&/" raw >expect &&
>      ++		cat > sedscript <<- 'EOF' &&
>      ++		/^	line-one-Z/i\\
>      ++		ignored
>      ++		/^	line-two-Z/i\\
>      ++		ignored
>      ++		EOF
>      ++		sed -f sedscript raw >expect &&
>       +
>       +		git -c blame.markIgnoredLines=true blame $opt --ignore-rev Z file >actual &&
>       +		test_cmp expect actual
>      -+	'
>      ++	"
>       +done
>       +
>        # For ignored revs that added 'unblamable' lines and more recent commits changed
> 
> ---
>   Documentation/blame-options.adoc |  3 ++-
>   Documentation/git-blame.adoc     |  9 ++++----
>   builtin/blame.c                  | 15 +++++++++++++
>   t/t8013-blame-ignore-revs.sh     | 38 ++++++++++++++++++++++++++++++++
>   4 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/blame-options.adoc b/Documentation/blame-options.adoc
> index aa77406d4e..19ea187238 100644
> --- a/Documentation/blame-options.adoc
> +++ b/Documentation/blame-options.adoc
> @@ -125,7 +125,8 @@ take effect.
>   	another commit will be marked with a `?` in the blame output.  If the
>   	`blame.markUnblamableLines` config option is set, then those lines touched
>   	by an ignored commit that we could not attribute to another revision are
> -	marked with a '*'.
> +	marked with a '*'. In the porcelain modes, we print 'ignored' and
> +	'unblamable' on a newline respectively.
>   
>   --ignore-revs-file <file>::
>   	Ignore revisions listed in `file`, which must be in the same format as an
> diff --git a/Documentation/git-blame.adoc b/Documentation/git-blame.adoc
> index f75ed44790..e438d28625 100644
> --- a/Documentation/git-blame.adoc
> +++ b/Documentation/git-blame.adoc
> @@ -135,10 +135,11 @@ header elements later.
>   The porcelain format generally suppresses commit information that has
>   already been seen. For example, two lines that are blamed to the same
>   commit will both be shown, but the details for that commit will be shown
> -only once. This is more efficient, but may require more state be kept by
> -the reader. The `--line-porcelain` option can be used to output full
> -commit information for each line, allowing simpler (but less efficient)
> -usage like:
> +only once. Information which is specific to individual lines will not be
> +grouped together, like revs to be marked 'ignored' or 'unblamable'. This
> +is more efficient, but may require more state be kept by the reader. The
> +`--line-porcelain` option can be used to output full commit information
> +for each line, allowing simpler (but less efficient) usage like:
>   
>   	# count the number of lines attributed to each author
>   	git blame --line-porcelain file |
> diff --git a/builtin/blame.c b/builtin/blame.c
> index c470654c7e..9436f70aec 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -351,6 +351,19 @@ static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
>   		write_filename_info(suspect);
>   }
>   
> +/*
> + * Information which needs to be printed per-line goes here. Any
> + * information which can be clubbed on a commit/file level, should
> + * be printed via 'emit_one_suspect_detail()'.
> + */
> +static void emit_porcelain_per_line_details(struct blame_entry *ent)
> +{
> +	if (mark_unblamable_lines && ent->unblamable)
> +		puts("unblamable");
> +	if (mark_ignored_lines && ent->ignored)
> +		puts("ignored");
> +}
> +
>   static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
>   			   int opt)
>   {
> @@ -367,6 +380,7 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
>   	       ent->lno + 1,
>   	       ent->num_lines);
>   	emit_porcelain_details(suspect, repeat);
> +	emit_porcelain_per_line_details(ent);
>   
>   	cp = blame_nth_line(sb, ent->lno);
>   	for (cnt = 0; cnt < ent->num_lines; cnt++) {
> @@ -377,6 +391,7 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
>   			       ent->lno + 1 + cnt);
>   			if (repeat)
>   				emit_porcelain_details(suspect, 1);
> +			emit_porcelain_per_line_details(ent);
>   		}
>   		putchar('\t');
>   		do {
> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
> index 370b768149..cace00ae8d 100755
> --- a/t/t8013-blame-ignore-revs.sh
> +++ b/t/t8013-blame-ignore-revs.sh
> @@ -158,6 +158,25 @@ test_expect_success mark_unblamable_lines '
>   	test_cmp expect actual
>   '
>   
> +for opt in --porcelain --line-porcelain
> +do
> +	test_expect_success "mark_unblamable_lines with $opt" "
> +		sha=$(git rev-parse Y) &&
> +
> +		git -c blame.markUnblamableLines=false blame $opt --ignore-rev Y file >raw &&
> +		cat > sedscript <<- 'EOF' &&
> +		/^	y3/i\\
> +		unblamable
> +		/^	y4/i\\
> +		unblamable
> +		EOF
> +		sed -f sedscript raw >expect &&
> +
> +		git -c blame.markUnblamableLines=true blame $opt --ignore-rev Y file >actual &&
> +		test_cmp expect actual
> +	"
> +done
> +
>   # Commit Z will touch the first two lines.  Y touched all four.
>   # 	A--B--X--Y--Z
>   # The blame output when ignoring Z should be:
> @@ -191,6 +210,25 @@ test_expect_success mark_ignored_lines '
>   	! test_cmp expect actual
>   '
>   
> +for opt in --porcelain --line-porcelain
> +do
> +	test_expect_success "mark_ignored_lines with $opt" "
> +		sha=$(git rev-parse Y) &&
> +
> +		git -c blame.markIgnoredLines=false blame $opt --ignore-rev Z file >raw &&
> +		cat > sedscript <<- 'EOF' &&
> +		/^	line-one-Z/i\\
> +		ignored
> +		/^	line-two-Z/i\\
> +		ignored
> +		EOF
> +		sed -f sedscript raw >expect &&
> +
> +		git -c blame.markIgnoredLines=true blame $opt --ignore-rev Z file >actual &&
> +		test_cmp expect actual
> +	"
> +done
> +
>   # For ignored revs that added 'unblamable' lines and more recent commits changed
>   # the blamable lines, mark the unblamable lines with a
>   # '*'