git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: git@vger.kernel.org, Jacob Keller <jacob.keller@gmail.com>,
	Lucian Poston <lucian.poston@gmail.com>
Subject: Re: [PATCH v4 1/2] diff: add --line-prefix option for passing in a prefix
Date: Thu, 11 Aug 2016 10:22:00 -0700	[thread overview]
Message-ID: <xmqqh9ar2pc7.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160810231908.26330-1-jacob.e.keller@intel.com> (Jacob Keller's message of "Wed, 10 Aug 2016 16:19:07 -0700")

Jacob Keller <jacob.e.keller@intel.com> writes:

>  const char *diff_line_prefix(struct diff_options *opt)
>  {
>  	struct strbuf *msgbuf;
> +
>  	if (!opt->output_prefix)
> -		return "";
> +		if (opt->line_prefix)
> +			return opt->line_prefix;
> +		else
> +			return "";
>  
>  	msgbuf = opt->output_prefix(opt, opt->output_prefix_data);
> +	/* line prefix must be printed before the output_prefix() */
> +	if (opt->line_prefix)
> +		strbuf_insert(msgbuf, 0, opt->line_prefix, strlen(opt->line_prefix));
>  	return msgbuf->buf;
>  }

The result of applying this change leaves the semantics of the
line_prefix field, the output_prefix() callback, and the
output_prefix_length field in the diff_options structure a bit
tricky to reason about.

The code pretends as if the remainder of the system does not even
care about the presence of line_prefix (i.e. output_prefix_length is
not updated), but its only user of output_prefix_length cares, I
think.  It is in diff.c::show_stats() where it auto-computes the
allowed display width for the stat portion:

        if (options->stat_width == -1)
                width = term_columns() - options->output_prefix_length;

The output_prefix_length is initialized to be 0, but when --graph is
in effect, it is set to the width of the graph portion of the output
in the output_prefix callback, diff_output_prefix_callback().

So the above change is clearly wrong in that it needs to add the
number of display columns needed to show opt->line_prefix to the
output_prefix_length, but I wonder if a better "fix" to this is to
get rid of output_prefix_length field from diff_options struct as a
preparatory step.  That would make the bug in this patch disappear.

Perhaps like this.  I do not know if Lucian is still interested in,
or remembers what did for, Git in 2012, but this updates his code
and replaces it with what I hope is an equivalent, so I added him
to the Cc: line.

-- >8 --
Subject: diff.c: remove output_prefix_length field

"diff/log --stat" has a logic that determines the display columns
available for the diffstat part of the output and apportions it for
pathnames and diffstat graph automatically.

5e71a84a (Add output_prefix_length to diff_options, 2012-04-16)
added the output_prefix_length field to diff_options structure to
allow this logic subtract the display columns used for display the
history graph part from the total "terminal width"; this matters
when the "git log --graph -p" option is in use.

The field be set to the number of display columns needed to show the
output from the output_prefix() callback.  Any new output_prefix()
callback must also update the field accordingly, which is error
prone.  As there is only one user of the field, and the user has the
actual value of the prefix string, let's get rid of the field and
have the user count the display width itself.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 diff.c  | 2 +-
 diff.h  | 1 -
 graph.c | 2 --
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index b43d3dd..ae069c3 100644
--- a/diff.c
+++ b/diff.c
@@ -1625,7 +1625,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	 */
 
 	if (options->stat_width == -1)
-		width = term_columns() - options->output_prefix_length;
+		width = term_columns() - strlen(line_prefix);
 	else
 		width = options->stat_width ? options->stat_width : 80;
 	number_width = decimal_width(max_change) > number_width ?
diff --git a/diff.h b/diff.h
index 125447b..49e4aaa 100644
--- a/diff.h
+++ b/diff.h
@@ -174,7 +174,6 @@ struct diff_options {
 	diff_format_fn_t format_callback;
 	void *format_callback_data;
 	diff_prefix_fn_t output_prefix;
-	int output_prefix_length;
 	void *output_prefix_data;
 
 	int diff_path_counter;
diff --git a/graph.c b/graph.c
index dd17201..a468038 100644
--- a/graph.c
+++ b/graph.c
@@ -197,7 +197,6 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void
 	assert(opt);
 	assert(graph);
 
-	opt->output_prefix_length = graph->width;
 	strbuf_reset(&msgbuf);
 	graph_padding_line(graph, &msgbuf);
 	return &msgbuf;
@@ -245,7 +244,6 @@ struct git_graph *graph_init(struct rev_info *opt)
 	 */
 	opt->diffopt.output_prefix = diff_output_prefix_callback;
 	opt->diffopt.output_prefix_data = graph;
-	opt->diffopt.output_prefix_length = 0;
 
 	return graph;
 }









  parent reply	other threads:[~2016-08-11 17:22 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-10 23:19 [PATCH v4 1/2] diff: add --line-prefix option for passing in a prefix Jacob Keller
2016-08-10 23:19 ` [PATCH v4 2/2] diff: add SUBMODULE_DIFF format to display submodule diff Jacob Keller
2016-08-11 17:53   ` Junio C Hamano
2016-08-11 18:34     ` Jacob Keller
2016-08-11 20:50   ` Junio C Hamano
2016-08-11 17:22 ` Junio C Hamano [this message]
2016-08-11 18:30   ` [PATCH v4 1/2] diff: add --line-prefix option for passing in a prefix Jacob Keller
2016-08-11 20:55 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqh9ar2pc7.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jacob.e.keller@intel.com \
    --cc=jacob.keller@gmail.com \
    --cc=lucian.poston@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).