[PATCH 0/2] Log pretty format alignment improvements

[PATCH 0/2] Log pretty format alignment improvements

[Nguyễn Thái Ngọc Duy]

The first patch was from a long time ago. The concern was it may be
breaking existing user expectation [1]. I still maintain that it's a good
thing to do and should not break anything. Hence the resubmission.

The second patch adds negative column specifier to >|() and friends.
A positive number 'n' specifies the n-th column from the left border
of the screen, '-n' specifies the n-th column from the _right_ border.

I have been happy with 2/2 improving my "git l1" (fancy --oneline)
printout for many months. It's definitely a good thing to do in my
opinion.

[1] http://thread.gmane.org/gmane.comp.version-control.git/277710/focus=278326

Josef Kufner (1):
  pretty: pass graph width to pretty formatting for use in '%>|(N)'

Nguyễn Thái Ngọc Duy (1):
  pretty.c: support <direction>|(<negative number>) forms

 commit.h                      |  1 +
 graph.c                       |  7 ++++++
 graph.h                       |  5 ++++
 log-tree.c                    |  2 ++
 pretty.c                      |  9 ++++++-
 t/t4205-log-pretty-formats.sh | 57 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 80 insertions(+), 1 deletion(-)

-- 
2.8.2.524.g6ff3d78

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

[Nguyễn Thái Ngọc Duy]

From: Josef Kufner <josef@kufner.cz>

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

For example, in the output of

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

this change will make all commit hashes align at 20th column from
the edge of the terminal, not from the edge of the graph.

Signed-off-by: Josef Kufner <josef@kufner.cz>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 commit.h                      |  1 +
 graph.c                       |  7 +++++++
 graph.h                       |  5 +++++
 log-tree.c                    |  2 ++
 pretty.c                      |  1 +
 t/t4205-log-pretty-formats.sh | 24 ++++++++++++++++++++++++
 6 files changed, 40 insertions(+)

diff --git a/commit.h b/commit.h
index b06db4d..0c923f0 100644
--- a/commit.h
+++ b/commit.h
@@ -161,6 +161,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 1350bdd..ad766fa 100644
--- a/graph.c
+++ b/graph.c
@@ -669,6 +669,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
@@ -67,6 +67,11 @@ int graph_is_commit_finished(struct git_graph const *graph);
 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 78a5381..8d39315 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -687,6 +687,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 87c4497..4f33b09 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1299,6 +1299,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) {
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 7398605..d75cdbb 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -319,6 +319,19 @@ 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 &&
+	iconv -f utf-8 -t $test_encoding >expected <<EOF&&
+* $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 +343,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.8.2.524.g6ff3d78

[PATCH 2/2] pretty.c: support <direction>|(<negative number>) forms

[Nguyễn Thái Ngọc Duy]

%>|(num), %><|(num) and %<|(num), where num is a positive number, sets a
fixed column from the screen's left border. There is no way for us to
specifiy a column relative to the right border, which is useful when you
want to make use of all terminal space (on big screens). Use negative
num for that. Inspired by Go's array syntax (*).

(*) I know Python has this first (or before Go, at least) but the idea
didn't occur to me until I learned Go.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 pretty.c                      |  8 +++++++-
 t/t4205-log-pretty-formats.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index 4f33b09..1c67b23 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1022,9 +1022,15 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
 		int width;
 		if (!end || end == start)
 			return 0;
-		width = strtoul(start, &next, 10);
+		width = strtol(start, &next, 10);
 		if (next == start || width == 0)
 			return 0;
+		if (width < 0) {
+			if (to_column)
+				width += term_columns();
+			if (width < 0)
+				return 0;
+		}
 		c->padding = to_column ? -width : width;
 		c->flush_type = flush_type;
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d75cdbb..d9f6242 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -176,6 +176,17 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'left alignment formatting at the nth column' '
+	COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
+	qz_to_tab_space <<EOF >expected &&
+$head1 message two                    Z
+$head2 message one                    Z
+$head3 add bar                        Z
+$head4 $(commit_msg)            Z
+EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'left 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 &&
@@ -308,6 +319,17 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'right alignment formatting at the nth column' '
+	COLUMNS=50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
+	qz_to_tab_space <<EOF >expected &&
+$head1                      message two
+$head2                      message one
+$head3                          add bar
+$head4              $(commit_msg)
+EOF
+	test_cmp expected actual
+'
+
 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 &&
@@ -397,6 +419,17 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'center alignment formatting at the nth column' '
+	COLUMNS=70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
+	qz_to_tab_space <<EOF >expected &&
+$head1           message two          Z
+$head2           message one          Z
+$head3             add bar            Z
+$head4       $(commit_msg)      Z
+EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'center 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 &&
-- 
2.8.2.524.g6ff3d78

Re: [PATCH 0/2] Log pretty format alignment improvements

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> The first patch was from a long time ago. The concern was it may be
> breaking existing user expectation [1]. I still maintain that it's a good
> thing to do and should not break anything. Hence the resubmission.

I do not think "it's a good feature to have" was a question from the
beginning.  Thread [1] stopped with me saying "as long as >(N) can
be used as Duy claims as a workaround to get the original behaviour,
it is good to allow using >|(N) for this new output format; I didn't
check if >(N) does behave that way, though".  What was necessary to
resurrect the patch was "Yes, >(N) can be used that way and here is
a test" or something like that.

> The second patch adds negative column specifier to >|() and friends.
> A positive number 'n' specifies the n-th column from the left border
> of the screen, '-n' specifies the n-th column from the _right_ border.

That is quite nice enhancement.

Re: [PATCH 0/2] Log pretty format alignment improvements

[Duy Nguyen]

On Thu, Jun 16, 2016 at 11:25:28AM -0700, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> 
> > The first patch was from a long time ago. The concern was it may be
> > breaking existing user expectation [1]. I still maintain that it's a good
> > thing to do and should not break anything. Hence the resubmission.
> 
> I do not think "it's a good feature to have" was a question from the
> beginning.  Thread [1] stopped with me saying "as long as >(N) can
> be used as Duy claims as a workaround to get the original behaviour,
> it is good to allow using >|(N) for this new output format; I didn't
> check if >(N) does behave that way, though".  What was necessary to
> resurrect the patch was "Yes, >(N) can be used that way and here is
> a test" or something like that.

Ah ok. This command with this series (using %>(n))

~/w/git/git log --format='%>(50,trunc)%s' --graph -15 origin/pu

and the system one at version 2.7.3 (using %>|(n))

/usr/bin/git log --format='%>|(50,trunc)%s' --graph -15 origin/pu

both produce the same output like this, checked with md5sum

*   Merge branch 'js/am-3-merge-recursive-direct' in..
|\  
| *          am: make a direct call to merge_recursive
| * merge_recursive_options: introduce the "gently" ..
* |            Merge branch 'tb/complete-status' into pu
|\ \  
| * |                         completion: add git status
| * |      completion: add __git_get_option_value helper
| * | completion: factor out untracked file modes into..
* | |    Merge branch 'mj/log-show-signature-conf' into pu
|\ \ \  
| * | |    log: "--no-show-signature" commmand-line option
| * | | log: add "log.showsignature" configuration varia..
* | | |              Merge branch 'nd/worktree-lock' into pu
|\ \ \ \  
| * | | |  worktree.c: find_worktree() search by path suffix
| * | | |                     worktree: add "unlock" command
| * | | |                       worktree: add "lock" command
| * | | |               worktree.c: add is_worktree_locked()

The output looks weird though, which makes me a bit hesitate to add it
in the test suite...
--
Duy

Re: [PATCH 0/2] Log pretty format alignment improvements

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Ah ok. This command with this series (using %>(n))
>
> ~/w/git/git log --format='%>(50,trunc)%s' --graph -15 origin/pu
>
> and the system one at version 2.7.3 (using %>|(n))
>
> /usr/bin/git log --format='%>|(50,trunc)%s' --graph -15 origin/pu
>
> both produce the same output like this, checked with md5sum
>
> *   Merge branch 'js/am-3-merge-recursive-direct' in..
> |\  
> | *          am: make a direct call to merge_recursive
> | * merge_recursive_options: introduce the "gently" ..
> * |            Merge branch 'tb/complete-status' into pu
> |\ \  
> | * |                         completion: add git status
> | * |      completion: add __git_get_option_value helper
> | * | completion: factor out untracked file modes into..
> * | |    Merge branch 'mj/log-show-signature-conf' into pu
> |\ \ \  
> | * | |    log: "--no-show-signature" commmand-line option
> | * | | log: add "log.showsignature" configuration varia..
> * | | |              Merge branch 'nd/worktree-lock' into pu
> |\ \ \ \  
> | * | | |  worktree.c: find_worktree() search by path suffix
> | * | | |                     worktree: add "unlock" command
> | * | | |                       worktree: add "lock" command
> | * | | |               worktree.c: add is_worktree_locked()
>
> The output looks weird though, which makes me a bit hesitate to add it
> in the test suite...

It may become easier to see if you used %<(N) vs %<|(N) instead?

Re: [PATCH 0/2] Log pretty format alignment improvements

[Josef Kufner]

[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]

Junio C Hamano wrote, on 16.6.2016 20:25:
> "as long as >(N) can be used as Duy claims as a workaround to 
> get the original behaviour, it is good to allow using >|(N)
> for this new output format; I didn't check if >(N) does behave
> that way, though"

Yes, it can be used in such way.

Example:

$ git log --format='%<(10)%t%<(20)%h.' -n 1
004419a   9ca73ba             .

$ git log --format='%<(10)%t%<|(30)%h.' -n 1
004419a   9ca73ba             .

Notice that dot is at exactly same column, because
10 + 20 == 30.

The difference is when using --graph, because width of the graph is not
known in advance. After the patch the graph width is included in %<(N).
Original behavior is to not count with the graph width.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]