[PATCH 1/3] pretty: Pass graph width to pretty formatting for use in '%>|(N)'

[PATCH 1/3] pretty: Pass graph width to pretty formatting for use in '%>|(N)'

[Josef Kufner]

Pass graph width to pretty formatting, so N in '%>|(N)' includes columns
consumed by graph rendered when --graph option is in use.

Example:
  git log --all --graph --pretty='format: [%>|(20)%h] %ar%d'

  All commit hashes should be aligned at 20th column from edge of the
  terminal, not from the edge of the graph.

Signed-off-by: Josef Kufner <josef@kufner.cz>
---
 commit.h   | 1 +
 graph.c    | 7 +++++++
 graph.h    | 5 +++++
 log-tree.c | 2 ++
 pretty.c   | 1 +
 5 files changed, 16 insertions(+)

diff --git a/commit.h b/commit.h
index 5d58be0..0a9a707 100644
--- a/commit.h
+++ b/commit.h
@@ -160,6 +160,7 @@ struct pretty_print_context {
 	 * should not be counted on by callers.
 	 */
 	struct string_list in_body_headers;
+	int graph_width;
 };
 
 struct userformat_want {
diff --git a/graph.c b/graph.c
index c25a09a..4802411 100644
--- a/graph.c
+++ b/graph.c
@@ -671,6 +671,13 @@ static void graph_output_padding_line(struct git_graph *graph,
 	graph_pad_horizontally(graph, sb, graph->num_new_columns * 2);
 }
 
+
+int graph_width(struct git_graph *graph)
+{
+	return graph->width;
+}
+
+
 static void graph_output_skip_line(struct git_graph *graph, struct strbuf *sb)
 {
 	/*
diff --git a/graph.h b/graph.h
index 0be62bd..3f48c19 100644
--- a/graph.h
+++ b/graph.h
@@ -68,6 +68,11 @@ int graph_next_line(struct git_graph *graph, struct strbuf *sb);
 
 
 /*
+ * Return current width of the graph in on-screen characters.
+ */
+int graph_width(struct git_graph *graph);
+
+/*
  * graph_show_*: helper functions for printing to stdout
  */
 
diff --git a/log-tree.c b/log-tree.c
index 7b1b57a..08fd5b6 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -686,6 +686,8 @@ void show_log(struct rev_info *opt)
 	ctx.output_encoding = get_log_output_encoding();
 	if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
 		ctx.from_ident = &opt->from_ident;
+	if (opt->graph)
+		ctx.graph_width = graph_width(opt->graph);
 	pretty_print_commit(&ctx, commit, &msgbuf);
 
 	if (opt->add_signoff)
diff --git a/pretty.c b/pretty.c
index 151c2ae..f1cf9e2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1297,6 +1297,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
 		if (!start)
 			start = sb->buf;
 		occupied = utf8_strnwidth(start, -1, 1);
+		occupied += c->pretty_ctx->graph_width;
 		padding = (-padding) - occupied;
 	}
 	while (1) {
-- 
2.5.1

[PATCH 2/3] Add tests for "pretty: Pass graph width to pretty formatting for use in '%>|(N)'"

[Josef Kufner]

Signed-off-by: Josef Kufner <josef@kufner.cz>
---
 t/t4205-log-pretty-formats.sh | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 7398605..3358837 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -319,6 +319,18 @@ EOF
 	test_cmp expected actual
 '
 
+# Note: Space between 'message' and 'two' should be in the same column as in previous test.
+test_expect_success 'right alignment formatting at the nth column with --graph. i18n.logOutputEncoding' '
+	git -c i18n.logOutputEncoding=$test_encoding log --graph --pretty="tformat:%h %>|(40)%s" >actual &&
+	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
+* $head1                    message two
+* $head2                    message one
+* $head3                        add bar
+* $head4            $(commit_msg)
+EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'right alignment formatting with no padding' '
 	git log --pretty="tformat:%>(1)%s" >actual &&
 	cat <<EOF >expected &&
@@ -330,6 +342,17 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'right alignment formatting with no padding and with --graph' '
+	git log --graph --pretty="tformat:%>(1)%s" >actual &&
+	cat <<EOF >expected &&
+* message two
+* message one
+* add bar
+* $(commit_msg)
+EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'right alignment formatting with no padding. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%>(1)%s" >actual &&
 	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-- 
2.5.1

[PATCH 3/3] t4205: remove unnecessary use of qz_to_tab_space

[Josef Kufner]

Many existing tests in this script pipes the result thru
qz_to_tab_space, but the payload does not need any Q or Z to be replaced
to tab or space. Redirect the input directly into iconv or use cat
instead.

Signed-off-by: Josef Kufner <josef@kufner.cz>
---
 t/t4205-log-pretty-formats.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 3358837..1f191cf 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -299,7 +299,7 @@ EOF
 
 test_expect_success 'right alignment formatting at the nth column' '
 	git log --pretty="tformat:%h %>|(40)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
+	cat <<EOF >expected &&
 $head1                      message two
 $head2                      message one
 $head3                          add bar
@@ -310,7 +310,7 @@ EOF
 
 test_expect_success 'right alignment formatting at the nth column. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%h %>|(40)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	iconv -f utf-8 -t $test_encoding <<EOF >expected &&
 $head1                      message two
 $head2                      message one
 $head3                          add bar
@@ -322,7 +322,7 @@ EOF
 # Note: Space between 'message' and 'two' should be in the same column as in previous test.
 test_expect_success 'right alignment formatting at the nth column with --graph. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --graph --pretty="tformat:%h %>|(40)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	iconv -f utf-8 -t $test_encoding <<EOF >expected &&
 * $head1                    message two
 * $head2                    message one
 * $head3                        add bar
@@ -423,7 +423,7 @@ EOF
 old_head1=$(git rev-parse --verify HEAD~0)
 test_expect_success 'center alignment formatting with no padding. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%><(1)%s" >actual &&
-	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	iconv -f utf-8 -t $test_encoding <<EOF >expected &&
 message two
 message one
 add bar
-- 
2.5.1

Re: [PATCH 1/3] pretty: Pass graph width to pretty formatting for use in '%>|(N)'

[Junio C Hamano]

Josef Kufner <josef@kufner.cz> writes:

> Pass graph width to pretty formatting, so N in '%>|(N)' includes columns
> consumed by graph rendered when --graph option is in use.
>
> Example:
>   git log --all --graph --pretty='format: [%>|(20)%h] %ar%d'
>
>   All commit hashes should be aligned at 20th column from edge of the
>   terminal, not from the edge of the graph.
>
> Signed-off-by: Josef Kufner <josef@kufner.cz>
> ---

[CC'ed Duy for ideas, as the "%>|(ALIGN)" thing is his invention]

If you imagine an entry for a commit in the "git log" output as a
rectangle that has its commit log message, "--graph" draws a bunch
of lines on the left hand side and place these rectangles on the
right of these lines.  Space allocated to each of these rectangles
may and do begin at a different column.  For example, here is an
output from

 $ git log -12 --graph --oneline

 *   7d211c9 Merge branch 'jk/graph-format-padding' into pu
 |\
 | * ead86db pretty: pass graph width to pretty formatting
 * |   5be4874 Merge branch 'ld/p4-detached-head' into pu
 |\ \
 | * | 4086903 git-p4: work with a detached head
 | * | 6d2be82 git-p4: add failing test for submit from detached
 head
 * | |   7cec6a3 Merge branch 'ls/p4-lfs' into pu
 |\ \ \
 | * | | 5fac7db git-p4: add Git LFS backend for large file system
 | * | | 53b938f git-p4: add support for large file systems
 | * | | 760e31c git-p4: return an empty list if a list config has
 no values
 | * | | c1e88b8 git-p4: add gitConfigInt reader
 | * | | 465af7a git-p4: add optional type specifier to gitConfig
 reader
 * | | |   5197bd3 Merge branch 'nd/clone-linked-checkout' into pu

I can understand why you would want to include the varying width of
the river on the left hand side in the "space allocated for the
commit", and under that mental model, the patch makes sense, but at
the same time, it is also a perfectly good specification to make
"%>|(20)%h" pad "%h" to 20 columns from the left edge of the space
given to the commit.

I have a suspicion that 50% of the users would appreciate this
change, and the remainder of the users see this break their
expectation.  To avoid such a regression, we may be better off if we
introduced a new alignment operator that is different from '%>|(N)'
so that this new behaviour is available to those who want to use it,
without negatively affecting existing uses.

And the use of ctx->graph_width in the hunk below would be the only
thing that needs to be changed (i.e. added to occupied only when the
new alignment operator is used).

Other than that, this round of reroll looks ready for 'next'.

Duy what do you think?

Thanks.

> diff --git a/pretty.c b/pretty.c
> index 151c2ae..f1cf9e2 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1297,6 +1297,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
>  		if (!start)
>  			start = sb->buf;
>  		occupied = utf8_strnwidth(start, -1, 1);
> +		occupied += c->pretty_ctx->graph_width;
>  		padding = (-padding) - occupied;
>  	}
>  	while (1) {

Re: [PATCH 1/3] pretty: Pass graph width to pretty formatting for use in '%>|(N)'

[Duy Nguyen]

(I'm joining the newer thread [1] back to this one, thanks for
reminding me about this)

[1] http://thread.gmane.org/gmane.comp.version-control.git/282771

On Tue, Sep 22, 2015 at 2:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Josef Kufner <josef@kufner.cz> writes:
>
>> Pass graph width to pretty formatting, so N in '%>|(N)' includes columns
>> consumed by graph rendered when --graph option is in use.
>>
>> Example:
>>   git log --all --graph --pretty='format: [%>|(20)%h] %ar%d'
>>
>>   All commit hashes should be aligned at 20th column from edge of the
>>   terminal, not from the edge of the graph.
>>
>> Signed-off-by: Josef Kufner <josef@kufner.cz>
>> ---
>
> [CC'ed Duy for ideas, as the "%>|(ALIGN)" thing is his invention]
>
> If you imagine an entry for a commit in the "git log" output as a
> rectangle that has its commit log message, "--graph" draws a bunch
> of lines on the left hand side and place these rectangles on the
> right of these lines.  Space allocated to each of these rectangles
> may and do begin at a different column.  For example, here is an
> output from
>
>  $ git log -12 --graph --oneline
>
>  *   7d211c9 Merge branch 'jk/graph-format-padding' into pu
>  |\
>  | * ead86db pretty: pass graph width to pretty formatting
>  * |   5be4874 Merge branch 'ld/p4-detached-head' into pu
>  |\ \
>  | * | 4086903 git-p4: work with a detached head
>  | * | 6d2be82 git-p4: add failing test for submit from detached
>  head
>  * | |   7cec6a3 Merge branch 'ls/p4-lfs' into pu
>  |\ \ \
>  | * | | 5fac7db git-p4: add Git LFS backend for large file system
>  | * | | 53b938f git-p4: add support for large file systems
>  | * | | 760e31c git-p4: return an empty list if a list config has
>  no values
>  | * | | c1e88b8 git-p4: add gitConfigInt reader
>  | * | | 465af7a git-p4: add optional type specifier to gitConfig
>  reader
>  * | | |   5197bd3 Merge branch 'nd/clone-linked-checkout' into pu
>
> I can understand why you would want to include the varying width of
> the river on the left hand side in the "space allocated for the
> commit", and under that mental model, the patch makes sense, but at
> the same time, it is also a perfectly good specification to make
> "%>|(20)%h" pad "%h" to 20 columns from the left edge of the space
> given to the commit.
>
> I have a suspicion that 50% of the users would appreciate this
> change, and the remainder of the users see this break their
> expectation.  To avoid such a regression, we may be better off if we
> introduced a new alignment operator that is different from '%>|(N)'
> so that this new behaviour is available to those who want to use it,
> without negatively affecting existing uses.

I tend to agree with Josef. >|(N) is about columns relative to the
left edge of the screen. You can already use >(N) to be relative to
the left edge of the space given to the commit.
-- 
Duy

Re: [PATCH 1/3] pretty: Pass graph width to pretty formatting for use in '%>|(N)'

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> (I'm joining the newer thread [1] back to this one, thanks for
> reminding me about this)
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/282771
>
> On Tue, Sep 22, 2015 at 2:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Josef Kufner <josef@kufner.cz> writes:
>>
>>> Pass graph width to pretty formatting, so N in '%>|(N)' includes columns
>>> consumed by graph rendered when --graph option is in use.
>>>
>>> Example:
>>>   git log --all --graph --pretty='format: [%>|(20)%h] %ar%d'
>>>
>>>   All commit hashes should be aligned at 20th column from edge of the
>>>   terminal, not from the edge of the graph.
>>>
>>> Signed-off-by: Josef Kufner <josef@kufner.cz>
>>> ---
>>
>> [CC'ed Duy for ideas, as the "%>|(ALIGN)" thing is his invention]
>> ...
>>
>> I have a suspicion that 50% of the users would appreciate this
>> change, and the remainder of the users see this break their
>> expectation.  To avoid such a regression, we may be better off if we
>> introduced a new alignment operator that is different from '%>|(N)'
>> so that this new behaviour is available to those who want to use it,
>> without negatively affecting existing uses.
>
> I tend to agree with Josef. >|(N) is about columns relative to the
> left edge of the screen. You can already use >(N) to be relative to
> the left edge of the space given to the commit.

OK.  I didn't check if >(N) with that old patch still behaves that
way, but if that is sensible, then it is not breaking anything, so
it may be a good idea to salvage it.  I dunno.