Re: [PATCH] Fix missing/buggy diff output prefixes w/ --graph

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 20, 2012, 17:55 UTC
Message-ID: <7vwr6f5f90.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1332230734-19734-1-git-send-email-lucian.poston@gmail.com>

Lucian Poston <lucian.poston@gmail.com> writes:

Show 11 quoted lines

> Fixed issue with `git log --graph --stat -p` in which the "---" diff output
> header appears before the diff output prefix.
>
> Fixed issue where diff output prefix is absent on empty lines separating diff
> stats and patch.
>
> Added test to verify the graph decoration prefixes of
> `git log --pretty=short --stat -p --graph` are printed correctly.
>
> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
> ---

Same comment as the one for your other patch applies to this. Especially, the Subject says "missing/buggy" but it is unclear if "missing" is the only bugginess you are addressing, or there are other "bugginess" other than "missing" from no description in the log message.

Show 17 quoted lines

> diff --git a/diff.c b/diff.c
> index 377ec1e..29003eb 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4399,6 +4399,12 @@ void diff_flush(struct diff_options *options)
>  
>  	if (output_format & DIFF_FORMAT_PATCH) {
>  		if (separator) {
> +			if (options->output_prefix) {
> +				struct strbuf *msg = NULL;
> +				msg = options->output_prefix(options,
> +					options->output_prefix_data);
> +				fwrite(msg->buf, msg->len, 1, stdout);
> +			}
>  			putc(options->line_termination, options->file);
>  			if (options->stat_sep) {
>  				/* attach patch instead of inline */

Immediately before a separator (typically LF) that comes between the log message and the patch text, we forgot to show the ancestry graph lines. This corresponds to the second paragraph of your log message.

This is a tangent, but I wonder how --graph should interact with stat_sep (i.e. patch is shown as an attachment) case. Perhaps the combination need to be forbidden, but we probably do not care (in other words, the user would get whatever the code happens to produce).

Show 18 quoted lines

> diff --git a/log-tree.c b/log-tree.c
> index cea8756..3198503 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -710,15 +710,16 @@ int log_tree_diff_flush(struct rev_info *opt)
>  		if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
>  		    opt->verbose_header &&
>  		    opt->commit_format != CMIT_FMT_ONELINE) {
> -			int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
> -			if ((pch & opt->diffopt.output_format) == pch)
> -				printf("---");
>  			if (opt->diffopt.output_prefix) {
>  				struct strbuf *msg = NULL;
>  				msg = opt->diffopt.output_prefix(&opt->diffopt,
>  					opt->diffopt.output_prefix_data);
>  				fwrite(msg->buf, msg->len, 1, stdout);
>  			}
> +			int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;

Moving this line is unnecessary, and introduces decl-after-statement to break compilation.

> +			if ((pch & opt->diffopt.output_format) == pch) {
> +				printf("---");
> +			}

We used to show the "---" before ancestry graph lines, which was nonsense. This corresponds to the first paragraph of your log message.

Looks good from a cursory review.  Thanks.