[PATCH] log --oneline: put decoration at the end of the line

[PATCH] log --oneline: put decoration at the end of the line

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

I find it easier to read "git log --oneline" when the subject lines
align, which they don't when the log is decorated because the
decoration stands before the subject line.

I'm on colored output so moving decoration to the end of line does not
make it harder to recognize refs. What about black-and-white people?
We could right align the decoration if it makes it easier for
the black-and-whites to read (but not to the right edge when the screen
is too wide)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 log-tree.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index c894930..c51c942 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -616,11 +616,12 @@ void show_log(struct rev_info *opt)
 			printf(" (from %s)",
 			       find_unique_abbrev(parent->object.sha1,
 						  abbrev_commit));
-		show_decorations(opt, commit);
-		printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
 		if (opt->commit_format == CMIT_FMT_ONELINE) {
+			printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
 			putchar(' ');
 		} else {
+			show_decorations(opt, commit);
+			printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
 			putchar('\n');
 			graph_show_oneline(opt->graph);
 		}
@@ -682,6 +683,10 @@ void show_log(struct rev_info *opt)
 		graph_show_commit_msg(opt->graph, &msgbuf);
 	else
 		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+	if (opt->commit_format == CMIT_FMT_ONELINE) {
+		show_decorations(opt, commit);
+		printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
+	}
 	if (opt->use_terminator) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
-- 
1.7.12.403.gce5cf6f.dirty

Re: [PATCH] log --oneline: put decoration at the end of the line

[Jeff King]

On Wed, Sep 19, 2012 at 06:52:20PM +0700, Nguyen Thai Ngoc Duy wrote:

> I find it easier to read "git log --oneline" when the subject lines
> align, which they don't when the log is decorated because the
> decoration stands before the subject line.

I like it. I turned on log.decorate some time ago, and I always felt
that --oneline was a little bit messy. But for some reason I never
thought of this simple change.

> I'm on colored output so moving decoration to the end of line does not
> make it harder to recognize refs. What about black-and-white people?

Like you, I use colors. I think the decorations would be much harder to
see if not for the color.

We should also consider briefly whether anybody is relying on --oneline
for machine parsing. I think "log --oneline" is fair game, but I wonder
if people calling "rev-list --decorate --oneline" should be considered.
It seems kind of unlikely to me, considering that the decorate output is
ambiguous to parse anyway (if you see parentheses, you cannot tell if it
is decorate output or part of the commit subject).

I did not look too carefully at your patch, but I did notice an odd
behavior with it. Try "git log --graph --oneline" in git.git. With stock
git, I see this several lines down (apologies for the long lines):

  * | | | | | | | | | | | | | | | | | | | | | | | |   b1379ba Merge branch 'sb/send-email-reconfirm-fix'
  |\ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \  
  | | |_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|/ / / /  
  | |/| | | | | | | | | | | | | | | | | | | | | | |   
  | * | | | | | | | | | | | | | | | | | | | | | | | 6183749 (origin/sb/send-email-reconfirm-fix) send-email: initial_to and initial_reply_to are both optional

In other words, 6183749 looks fine: graph, decorations, then subject,
all on the same line.  But with your patch, I see:

  * | | | | | | | | | | | | | | | | | | | | | | | |   b1379ba Merge branch 'sb/send-email-reconfirm-fix'
  |\ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \  
  | | |_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|/ / / /  
  | |/| | | | | | | | | | | | | | | | | | | | | | |   
  | * | | | | | | | | | | | | | | | | | | | | | | | 6183749 send-email: initial_to and initial_reply_to are both optional
  | | |_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|/ / / /  
  | |/| | | | | | | | | | | | | | | | | | | | | |    (origin/sb/send-email-reconfirm-fix)

The decoration is broken onto a separate line (with a newline in
between). Oddly, if I start my log right at b1379ba, it looks OK. Which
makes me think we are hitting some kind of line-wrapping code related to
the width of the graph.

-Peff

Re: [PATCH] log --oneline: put decoration at the end of the line

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We should also consider briefly whether anybody is relying on --oneline
> for machine parsing. I think "log --oneline" is fair game, but I wonder
> if people calling "rev-list --decorate --oneline" should be considered.
> It seems kind of unlikely to me, considering that the decorate output is
> ambiguous to parse anyway (if you see parentheses, you cannot tell if it
> is decorate output or part of the commit subject).

Yeah, I do not think it is likely.  Among the in-tree scripts,
git-stash does use rev-list --oneline but the purpose of the call
exactly is to grab a human readable one line summary, and it will be
happy with any change to make --oneline more human readble.

t4202 has many invocations of "log --oneline --decorate", though;
these things do get tested.

Re: [PATCH] log --oneline: put decoration at the end of the line

[Jeff King]

On Wed, Sep 19, 2012 at 12:57:28PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > We should also consider briefly whether anybody is relying on --oneline
> > for machine parsing. I think "log --oneline" is fair game, but I wonder
> > if people calling "rev-list --decorate --oneline" should be considered.
> > It seems kind of unlikely to me, considering that the decorate output is
> > ambiguous to parse anyway (if you see parentheses, you cannot tell if it
> > is decorate output or part of the commit subject).
> 
> Yeah, I do not think it is likely.  Among the in-tree scripts,
> git-stash does use rev-list --oneline but the purpose of the call
> exactly is to grab a human readable one line summary, and it will be
> happy with any change to make --oneline more human readble.

Yeah, that makes sense.

> t4202 has many invocations of "log --oneline --decorate", though;
> these things do get tested.

I think it is just trying to compare the "log.decorate" variable to the
"--decorate" command-line option. Notice that it generates the expected
output by running the latter. So I think it would be OK (and if not,
I think it would make sense to update the test, as it does not care
about the specific format).

Google Code Search doesn't show anything interesting, though I suppose
its results are getting continually out of date (they claim to have shut
it down, though you can still query it if you use the right URL).

-Peff

Re: [PATCH] log --oneline: put decoration at the end of the line

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> We should also consider briefly whether anybody is relying on --oneline
>> for machine parsing. I think "log --oneline" is fair game, but I wonder
>> if people calling "rev-list --decorate --oneline" should be considered.
>> It seems kind of unlikely to me, considering that the decorate output is
>> ambiguous to parse anyway (if you see parentheses, you cannot tell if it
>> is decorate output or part of the commit subject).
>
> Yeah, I do not think it is likely.  Among the in-tree scripts,
> git-stash does use rev-list --oneline but the purpose of the call
> exactly is to grab a human readable one line summary, and it will be
> happy with any change to make --oneline more human readble.

Having said that, one of my often used alias is

    [alias] recent = log --branches --oneline --no-merges --decorate --since=3.weeks

to help me see what topics in flight may potentially interact with
an incoming patch, when deciding on which commit to base the patch
on.  Pushing the decoration at the end to let it fall off the right
edge of the screen severely reduces the usefulness of it and defeats
the point of using --decorate, at least for this use.

I could use --source instead, though, if it is not moved by the
patch.

Re: [PATCH] log --oneline: put decoration at the end of the line

[Jeff King]

On Wed, Sep 19, 2012 at 04:34:19PM -0700, Junio C Hamano wrote:

> > Yeah, I do not think it is likely.  Among the in-tree scripts,
> > git-stash does use rev-list --oneline but the purpose of the call
> > exactly is to grab a human readable one line summary, and it will be
> > happy with any change to make --oneline more human readble.
> 
> Having said that, one of my often used alias is
> 
>     [alias] recent = log --branches --oneline --no-merges --decorate --since=3.weeks
> 
> to help me see what topics in flight may potentially interact with
> an incoming patch, when deciding on which commit to base the patch
> on.  Pushing the decoration at the end to let it fall off the right
> edge of the screen severely reduces the usefulness of it and defeats
> the point of using --decorate, at least for this use.
> 
> I could use --source instead, though, if it is not moved by the
> patch.

If you are particular about the exact format, how about using
--format="%h%d %s" instead?

Obviously Duy could do the same to achieve his format, but I think there
is still value in considering what the default for --oneline should be.

-Peff

Re: [PATCH] log --oneline: put decoration at the end of the line

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 19, 2012 at 04:34:19PM -0700, Junio C Hamano wrote:
>
>> > Yeah, I do not think it is likely.  Among the in-tree scripts,
>> > git-stash does use rev-list --oneline but the purpose of the call
>> > exactly is to grab a human readable one line summary, and it will be
>> > happy with any change to make --oneline more human readble.
>> 
>> Having said that, one of my often used alias is
>> 
>>     [alias] recent = log --branches --oneline --no-merges --decorate --since=3.weeks
>> 
>> to help me see what topics in flight may potentially interact with
>> an incoming patch, when deciding on which commit to base the patch
>> on.  Pushing the decoration at the end to let it fall off the right
>> edge of the screen severely reduces the usefulness of it and defeats
>> the point of using --decorate, at least for this use.
>> 
>> I could use --source instead, though, if it is not moved by the
>> patch.
>
> If you are particular about the exact format, how about using
> --format="%h%d %s" instead?
>
> Obviously Duy could do the same to achieve his format,...

It indeed was the first reaction when I saw the patch under
discussion that came without RFC/ in the subject.

> but I think there
> is still value in considering what the default for --oneline should be.

Yes.  And I was reasonably sure that having the decoration at the
tail is a better default, but no longer.

Re: [PATCH] log --oneline: put decoration at the end of the line

[Nguyen Thai Ngoc Duy]

On Thu, Sep 20, 2012 at 6:42 AM, Jeff King <peff@peff.net> wrote:
> If you are particular about the exact format, how about using
> --format="%h%d %s" instead?
>
> Obviously Duy could do the same to achieve his format, but I think there
> is still value in considering what the default for --oneline should be.

Yeah I was wondering if a new default would make sense.

I tried --pretty=format:"%h %s %d" but the colors were lost.
--decorate uses more than one color so %C* does not immitate the
colorful output we have with --decorate. Maybe I should add %Cd as
"colored %d" and update my alias to use it.

And perhaps a specifier to enable right alignment. It may help make
better use of wide terminal screen.
-- 
Duy

[PATCH 0/2] New pretty format color specifiers %C+ and %C-

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

On Thu, Sep 20, 2012 at 5:43 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> I tried --pretty=format:"%h %s %d" but the colors were lost.
> --decorate uses more than one color so %C* does not immitate the
> colorful output we have with --decorate. Maybe I should add %Cd as
> "colored %d" and update my alias to use it.

Here goes. I'm now a happy user with --pretty='format:%C+%h %s %d'.
Somebody might want to add coloring to sig placeholder too.

Nguyễn Thái Ngọc Duy (2):
  pretty: share code between format_decoration and show_decorations
  pretty: support placeholders %C+ and %C-

 Documentation/pretty-formats.txt |  2 ++
 log-tree.c                       | 55 ++++++++++++++++++++++++----------------
 log-tree.h                       |  3 +++
 pretty.c                         | 30 +++++++++-------------
 4 files changed, 50 insertions(+), 40 deletions(-)

-- 
1.7.12.1.383.gda6001e.dirty

[PATCH 1/2] pretty: share code between format_decoration and show_decorations

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

This also adds color support to format_decoration()

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 log-tree.c | 55 +++++++++++++++++++++++++++++++++----------------------
 log-tree.h |  3 +++
 pretty.c   | 19 +------------------
 3 files changed, 37 insertions(+), 40 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index c894930..b8cea3f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -174,36 +174,47 @@ static void show_children(struct rev_info *opt, struct commit *commit, int abbre
 	}
 }
 
-void show_decorations(struct rev_info *opt, struct commit *commit)
+void format_decoration(struct strbuf *sb,
+		       const struct commit *commit,
+		       int use_color)
 {
-	const char *prefix;
-	struct name_decoration *decoration;
+	const char *prefix = " (";
+	struct name_decoration *d;
 	const char *color_commit =
-		diff_get_color_opt(&opt->diffopt, DIFF_COMMIT);
+		diff_get_color(use_color, DIFF_COMMIT);
 	const char *color_reset =
-		decorate_get_color_opt(&opt->diffopt, DECORATION_NONE);
+		decorate_get_color(use_color, DECORATION_NONE);
+
+	load_ref_decorations(DECORATE_SHORT_REFS);
+	d = lookup_decoration(&name_decoration, &commit->object);
+	if (!d)
+		return;
+	while (d) {
+		strbuf_addstr(sb, prefix);
+		strbuf_addstr(sb, decorate_get_color(use_color, d->type));
+		if (d->type == DECORATION_REF_TAG)
+			strbuf_addstr(sb, "tag: ");
+		strbuf_addstr(sb, d->name);
+		strbuf_addstr(sb, color_reset);
+		strbuf_addstr(sb, color_commit);
+		prefix = ", ";
+		d = d->next;
+	}
+	if (prefix[0] == ',')
+		strbuf_addch(sb, ')');
+}
+
+void show_decorations(struct rev_info *opt, struct commit *commit)
+{
+	struct strbuf sb = STRBUF_INIT;
 
 	if (opt->show_source && commit->util)
 		printf("\t%s", (char *) commit->util);
 	if (!opt->show_decorations)
 		return;
-	decoration = lookup_decoration(&name_decoration, &commit->object);
-	if (!decoration)
-		return;
-	prefix = " (";
-	while (decoration) {
-		printf("%s", prefix);
-		fputs(decorate_get_color_opt(&opt->diffopt, decoration->type),
-		      stdout);
-		if (decoration->type == DECORATION_REF_TAG)
-			fputs("tag: ", stdout);
-		printf("%s", decoration->name);
-		fputs(color_reset, stdout);
-		fputs(color_commit, stdout);
-		prefix = ", ";
-		decoration = decoration->next;
-	}
-	putchar(')');
+	format_decoration(&sb, commit, opt->diffopt.use_color);
+	fputs(sb.buf, stdout);
+	strbuf_release(&sb);
 }
 
 /*
diff --git a/log-tree.h b/log-tree.h
index f5ac238..10c2682 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,6 +13,9 @@ int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
 int log_tree_opt_parse(struct rev_info *, const char **, int);
 void show_log(struct rev_info *opt);
+void format_decoration(struct strbuf *sb,
+		       const struct commit *commit,
+		       int use_color);
 void show_decorations(struct rev_info *opt, struct commit *commit);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **subject_p,
diff --git a/pretty.c b/pretty.c
index 8b1ea9f..e910679 100644
--- a/pretty.c
+++ b/pretty.c
@@ -764,23 +764,6 @@ static void parse_commit_message(struct format_commit_context *c)
 	c->commit_message_parsed = 1;
 }
 
-static void format_decoration(struct strbuf *sb, const struct commit *commit)
-{
-	struct name_decoration *d;
-	const char *prefix = " (";
-
-	load_ref_decorations(DECORATE_SHORT_REFS);
-	d = lookup_decoration(&name_decoration, &commit->object);
-	while (d) {
-		strbuf_addstr(sb, prefix);
-		prefix = ", ";
-		strbuf_addstr(sb, d->name);
-		d = d->next;
-	}
-	if (prefix[0] == ',')
-		strbuf_addch(sb, ')');
-}
-
 static void strbuf_wrap(struct strbuf *sb, size_t pos,
 			size_t width, size_t indent1, size_t indent2)
 {
@@ -1005,7 +988,7 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		strbuf_addstr(sb, get_revision_mark(NULL, commit));
 		return 1;
 	case 'd':
-		format_decoration(sb, commit);
+		format_decoration(sb, commit, 0);
 		return 1;
 	case 'g':		/* reflog info */
 		switch(placeholder[1]) {
-- 
1.7.12.1.383.gda6001e.dirty

[PATCH 2/2] pretty: support placeholders %C+ and %C-

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

%C+ tells the next specifiers that color is preferred. %C- the
opposite. So far only %H, %h and %d support coloring.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/pretty-formats.txt |  2 ++
 pretty.c                         | 13 ++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index e3d8a83..6e287d6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -142,6 +142,8 @@ The placeholders are:
 - '%Cblue': switch color to blue
 - '%Creset': reset color
 - '%C(...)': color specification, as described in color.branch.* config option
+- '%C+': enable coloring on the following placeholders if supported
+- '%C-': disable coloring on the following placeholders
 - '%m': left, right or boundary mark
 - '%n': newline
 - '%%': a raw '%'
diff --git a/pretty.c b/pretty.c
index e910679..b1cec71 100644
--- a/pretty.c
+++ b/pretty.c
@@ -623,6 +623,7 @@ struct format_commit_context {
 	unsigned commit_header_parsed:1;
 	unsigned commit_message_parsed:1;
 	unsigned commit_signature_parsed:1;
+	unsigned use_color:1;
 	struct {
 		char *gpg_output;
 		char good_bad;
@@ -885,6 +886,12 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 					"--pretty format", color);
 			strbuf_addstr(sb, color);
 			return end - placeholder + 1;
+		} else if (placeholder[1] == '+') {
+			c->use_color = 1;
+			return 2;
+		} else if (placeholder[1] == '-') {
+			c->use_color = 0;
+			return 2;
 		}
 		if (!prefixcmp(placeholder + 1, "red")) {
 			strbuf_addstr(sb, GIT_COLOR_RED);
@@ -945,13 +952,17 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 
 	switch (placeholder[0]) {
 	case 'H':		/* commit hash */
+		strbuf_addstr(sb, diff_get_color(c->use_color, DIFF_COMMIT));
 		strbuf_addstr(sb, sha1_to_hex(commit->object.sha1));
+		strbuf_addstr(sb, diff_get_color(c->use_color, DIFF_RESET));
 		return 1;
 	case 'h':		/* abbreviated commit hash */
+		strbuf_addstr(sb, diff_get_color(c->use_color, DIFF_COMMIT));
 		if (add_again(sb, &c->abbrev_commit_hash))
 			return 1;
 		strbuf_addstr(sb, find_unique_abbrev(commit->object.sha1,
 						     c->pretty_ctx->abbrev));
+		strbuf_addstr(sb, diff_get_color(c->use_color, DIFF_RESET));
 		c->abbrev_commit_hash.len = sb->len - c->abbrev_commit_hash.off;
 		return 1;
 	case 'T':		/* tree hash */
@@ -988,7 +999,7 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		strbuf_addstr(sb, get_revision_mark(NULL, commit));
 		return 1;
 	case 'd':
-		format_decoration(sb, commit, 0);
+		format_decoration(sb, commit, c->use_color);
 		return 1;
 	case 'g':		/* reflog info */
 		switch(placeholder[1]) {
-- 
1.7.12.1.383.gda6001e.dirty

Re: [PATCH 3/2] pretty: support right alignment

[Nguyen Thai Ngoc Duy]

And this is a for-fun patch that adds %| to right align everything
after that. I'm ignoring problems with line wrapping, i18n and so
on. "%C+%h %s%|%d" looks quite nice. I'm not sure how much useful it
is beyond --oneline though. It looks something like this

cc543b2 pretty: support placeholders %C+ and %C-                                                (HEAD, master)
da6001e pretty: share code between format_decoration and show_decorations
b0576a6 Update draft release notes to 1.8.0                                       (origin/master, origin/HEAD)
3d7535e Merge branch 'jc/maint-log-grep-all-match'
06e211a Merge branch 'jc/make-static'
8db3865 Merge branch 'pw/p4-submit-conflicts'
3387423 Merge branch 'mv/cherry-pick-s'
d71abd9 Merge branch 'nd/fetch-status-alignment'
3c7d509 Sync with 1.7.12.1
304b7d9 Git 1.7.12.1                                                            (tag: v1.7.12.1, origin/maint)
39e2e02 Merge branch 'er/doc-fast-import-done' into maint
8ffc331 Merge branch 'jk/config-warn-on-inaccessible-paths' into maint
01f7d7f Doc: Improve shallow depth wording
8093ae8 Documentation/git-filter-branch: Move note about effect of removing commits

-- 8< --
diff --git a/pretty.c b/pretty.c
index b1cec71..6e96f83 100644
--- a/pretty.c
+++ b/pretty.c
@@ -624,6 +624,7 @@ struct format_commit_context {
 	unsigned commit_message_parsed:1;
 	unsigned commit_signature_parsed:1;
 	unsigned use_color:1;
+	unsigned right_alignment:1;
 	struct {
 		char *gpg_output;
 		char good_bad;
@@ -645,6 +646,8 @@ struct format_commit_context {
 	struct chunk abbrev_tree_hash;
 	struct chunk abbrev_parent_hashes;
 	size_t wrap_start;
+
+	struct strbuf *right_sb;
 };
 
 static int add_again(struct strbuf *sb, struct chunk *chunk)
@@ -944,6 +947,10 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 			return end - placeholder + 1;
 		} else
 			return 0;
+
+	case '|':
+		c->right_alignment = 1;
+		return 1;
 	}
 
 	/* these depend on the commit */
@@ -1099,9 +1106,44 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 	return 0;	/* unknown placeholder */
 }
 
+static void right_align(struct strbuf *sb,
+			struct format_commit_context *c,
+			int flush)
+{
+	const char *p;
+	int llen, rlen, len, total = term_columns() - 1;
+	if (!c->right_alignment)
+		return;
+	p = strchr(c->right_sb->buf, '\n');
+	if (!p && flush)
+		p = c->right_sb->buf + c->right_sb->len;
+	if (!p)
+		return;
+
+	c->right_alignment = 0;
+	len = p - c->right_sb->buf;
+	if (!len)
+		return;
+	if (total > 110)
+		total = 110;
+	rlen = utf8_strnwidth(c->right_sb->buf, len);
+	p = strrchr(sb->buf, '\n');
+	if (!p)
+		p = sb->buf;
+	else
+		p++;
+	llen = utf8_strwidth(p);
+	strbuf_addf(sb, "%*s",
+		    total - llen + (len - rlen),
+		    c->right_sb->buf);
+	strbuf_reset(c->right_sb);
+}
+
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 				 void *context)
 {
+	struct format_commit_context *c = context;
+	struct strbuf *real_sb;
 	int consumed;
 	size_t orig_len;
 	enum {
@@ -1127,10 +1169,13 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	if (magic != NO_MAGIC)
 		placeholder++;
 
+	if (c->right_alignment && c->right_sb) {
+		real_sb = sb;
+		sb = c->right_sb;
+	}
+
 	orig_len = sb->len;
 	consumed = format_commit_one(sb, placeholder, context);
-	if (magic == NO_MAGIC)
-		return consumed;
 
 	if ((orig_len == sb->len) && magic == DEL_LF_BEFORE_EMPTY) {
 		while (sb->len && sb->buf[sb->len - 1] == '\n')
@@ -1141,7 +1186,13 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 		else if (magic == ADD_SP_BEFORE_NON_EMPTY)
 			strbuf_insert(sb, orig_len, " ", 1);
 	}
-	return consumed + 1;
+
+	if (real_sb)
+		right_align(real_sb, c, 0);
+
+	if (magic != NO_MAGIC)
+		consumed++;
+	return consumed;
 }
 
 static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
@@ -1180,12 +1231,14 @@ void format_commit_message(const struct commit *commit,
 	struct format_commit_context context;
 	static const char utf8[] = "UTF-8";
 	const char *output_enc = pretty_ctx->output_encoding;
+	struct strbuf right_sb = STRBUF_INIT;
 
 	memset(&context, 0, sizeof(context));
 	context.commit = commit;
 	context.pretty_ctx = pretty_ctx;
 	context.wrap_start = sb->len;
 	context.message = commit->buffer;
+	context.right_sb = &right_sb;
 	if (output_enc) {
 		char *enc = get_header(commit, "encoding");
 		if (strcmp(enc ? enc : utf8, output_enc)) {
@@ -1197,8 +1250,11 @@ void format_commit_message(const struct commit *commit,
 	}
 
 	strbuf_expand(sb, format, format_commit_item, &context);
+	if (context.right_alignment)
+		right_align(sb, &context, 1);
 	rewrap_message_tail(sb, &context, 0, 0, 0);
 
+	strbuf_release(&right_sb);
 	if (context.message != commit->buffer)
 		free(context.message);
 	free(context.signature.gpg_output);
diff --git a/utf8.c b/utf8.c
index a544f15..68ca0b4 100644
--- a/utf8.c
+++ b/utf8.c
@@ -9,6 +9,20 @@ struct interval {
   int last;
 };
 
+static size_t display_mode_esc_sequence_len(const char *s)
+{
+	const char *p = s;
+	if (*p++ != '\033')
+		return 0;
+	if (*p++ != '[')
+		return 0;
+	while (isdigit(*p) || *p == ';')
+		p++;
+	if (*p++ != 'm')
+		return 0;
+	return p - s;
+}
+
 /* auxiliary function for binary search in interval table */
 static int bisearch(ucs_char_t ucs, const struct interval *table, int max)
 {
@@ -252,18 +266,25 @@ int utf8_width(const char **start, size_t *remainder_p)
  * string, assuming that the string is utf8.  Returns strlen() instead
  * if the string does not look like a valid utf8 string.
  */
-int utf8_strwidth(const char *string)
+int utf8_strnwidth(const char *string, int len)
 {
 	int width = 0;
 	const char *orig = string;
 
-	while (1) {
-		if (!string)
-			return strlen(orig);
-		if (!*string)
-			return width;
+	if (len == -1)
+		len = strlen(string);
+	while (string && string < orig + len) {
+		int skip;
+		while ((skip = display_mode_esc_sequence_len(string)))
+			string += skip;
 		width += utf8_width(&string, NULL);
 	}
+	return string ? width : len;
+}
+
+int utf8_strwidth(const char *string)
+{
+	return utf8_strnwidth(string, -1);
 }
 
 int is_utf8(const char *text)
@@ -303,20 +324,6 @@ static void strbuf_add_indented_text(struct strbuf *buf, const char *text,
 	}
 }
 
-static size_t display_mode_esc_sequence_len(const char *s)
-{
-	const char *p = s;
-	if (*p++ != '\033')
-		return 0;
-	if (*p++ != '[')
-		return 0;
-	while (isdigit(*p) || *p == ';')
-		p++;
-	if (*p++ != 'm')
-		return 0;
-	return p - s;
-}
-
 /*
  * Wrap the text, if necessary. The variable indent is the indent for the
  * first line, indent2 is the indent for all other lines.
diff --git a/utf8.h b/utf8.h
index 3c0ae76..1727405 100644
--- a/utf8.h
+++ b/utf8.h
@@ -4,6 +4,7 @@
 typedef unsigned int ucs_char_t;  /* assuming 32bit int */
 
 int utf8_width(const char **start, size_t *remainder_p);
+int utf8_strnwidth(const char *string, int len);
 int utf8_strwidth(const char *string);
 int is_utf8(const char *text);
 int is_encoding_utf8(const char *name);
-- 8< --

Re: [PATCH 3/2] pretty: support right alignment

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> And this is a for-fun patch that adds %| to right align everything
> after that. I'm ignoring problems with line wrapping, i18n and so
> on. "%C+%h %s%|%d" looks quite nice. I'm not sure how much useful it
> is beyond --oneline though. It looks something like this
> ...
> [oneline output ellided]
> ...

I think this is a great feature at the conceptual level, and you
know "but" is coming ;-).

 - Shouldn't it be "everything from there until the end of the
   current line" than "everything after that"?

 - How is the display width determined and is it fixed once it gets
   computed?

 - How does this interact with the wrapped output?  Should it?

 - I am wondering if somebody ever want to do this with a follow-up
   patch:

	Left %h%|Center %cd%|Right %ad

   Is %| a sensible choice for "flush right"?  I am wondering if it
   makes more sense to make %|, %< and %> as "multi-column
   introducer" (the example defines output with three columns) that
   also tells how text inside each column is flushed inside the
   column, e.g.

	%>col 1 right flushed%|col 2 centered%< col 3 left flushed

   or something like that (we may want explicit "column width"
   specifiers if we were to do this kind of thing).

Re: [PATCH 3/2] pretty: support right alignment

[Nguyen Thai Ngoc Duy]

On Thu, Sep 20, 2012 at 11:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I think this is a great feature at the conceptual level, and you
> know "but" is coming ;-).

I'm still not sure if it's useful beyond my simple example. For
example, will it be useful in multiline log format, not just
--oneline?

>  - Shouldn't it be "everything from there until the end of the
>    current line" than "everything after that"?

The patch does that. I wasn't specific in my patch description.

>  - How is the display width determined and is it fixed once it gets
>    computed?

term_columns(). But I'd rather have a (user-configurable) max limit.
It's really hard to line up two distant text parts of a 200 char line
without a physical ruler. In my patch I just hard code the max limit
around 120 char or so.

>  - How does this interact with the wrapped output?  Should it?

We have to deal with it anyway when the left aligned text takes all
the space. On one hand, I don't want to break the terminal width,
leading to ugly output, so it'll interact. On the other hand, I don't
really wish to turn pretty format machinery into a full feature text
layout engine (by ripping of links/lynx?). So we have a few options:

1. ellipses, line cutting means i18n issues ahead
2. just put the right-aligned text on another line. We do something
similar in parse-options. When the option syntax is too long, we put
help description on the next line.
3. bring in html/css box model for arranging text so that both
left/right aligned texts can share the same line.
4. tell users upfront it's not supported. don't do that

I'd vote 2, or 4.

>  - I am wondering if somebody ever want to do this with a follow-up
>    patch:
>
>         Left %h%|Center %cd%|Right %ad
>
>    Is %| a sensible choice for "flush right"?  I am wondering if it
>    makes more sense to make %|, %< and %> as "multi-column
>    introducer" (the example defines output with three columns) that
>    also tells how text inside each column is flushed inside the
>    column, e.g.
>
>         %>col 1 right flushed%|col 2 centered%< col 3 left flushed
>
>    or something like that (we may want explicit "column width"
>    specifiers if we were to do this kind of thing).

Yeah that crossed my mind. But I'll need to convince myself it's
actually useful. Once you're on that road, you may want >=4 column
tables. We can extend column.c to do that. That hard part is
converting pretty format to use column functions.
-- 
Duy

Re: [PATCH 3/2] pretty: support right alignment

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>>  - How does this interact with the wrapped output?  Should it?
>
> We have to deal with it anyway when the left aligned text takes all
> the space. On one hand, I don't want to break the terminal width,
> leading to ugly output, so it'll interact. On the other hand, I don't
> really wish to turn pretty format machinery into a full feature text
> layout engine (by ripping of links/lynx?). So we have a few options:
>
> 1. ellipses, line cutting means i18n issues ahead
> 2. just put the right-aligned text on another line. We do something
> similar in parse-options. When the option syntax is too long, we put
> help description on the next line.
> 3. bring in html/css box model for arranging text so that both
> left/right aligned texts can share the same line.
> 4. tell users upfront it's not supported. don't do that
>
> I'd vote 2, or 4.

I am fine with 4 personally.

>>  - I am wondering if somebody ever want to do this with a follow-up
>>    patch:
>>
>>         Left %h%|Center %cd%|Right %ad
>>
>>    Is %| a sensible choice for "flush right"?  I am wondering if it
>>    makes more sense to make %|, %< and %> as "multi-column
>>    introducer" (the example defines output with three columns) that
>>    also tells how text inside each column is flushed inside the
>>    column, e.g.
>>
>>         %>col 1 right flushed%|col 2 centered%< col 3 left flushed
>>
>>    or something like that (we may want explicit "column width"
>>    specifiers if we were to do this kind of thing).
>
> Yeah that crossed my mind. But I'll need to convince myself it's
> actually useful. Once you're on that road, you may want >=4 column
> tables. We can extend column.c to do that. That hard part is
> converting pretty format to use column functions.

Reading the above again, I realize that I may have sounded as if
saying "With 'flush to right', we are inviting wishes for 'left' and
'center', and a patch that only does 'right' is unacceptable.", but
that was not what I meant.  I am perfectly fine with 'flush to right'
without anything else as the first step.

The only concern I had was that somebody who later tries to add
'left', 'center', etc. to accompany the 'right' that you are adding
with your patch will find it unfortunate that the natural choice for
'center', i.e. %|, has been taken to mean a wrong thing and that
mistake cannot be corrected without breaking backward compatibility.

Re: [PATCH 3/2] pretty: support right alignment

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> ... On the other hand, I don't
> really wish to turn pretty format machinery into a full feature text
> layout engine (by ripping of links/lynx?).

That is very true.  We should restrain ourselves and avoid going
overboard piling shiny new toys on a not-so-useful foundation that
is not expressive enough.  One feature that is probably much more
needed on the foundation side would be some form of conditional
output, without which built-in output elements like --show-notes
cannot be emulated with --format option.

Re: [PATCH 3/2] pretty: support right alignment

[Jeff King]

On Sun, Sep 23, 2012 at 01:17:43AM -0700, Junio C Hamano wrote:

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 
> > ... On the other hand, I don't
> > really wish to turn pretty format machinery into a full feature text
> > layout engine (by ripping of links/lynx?).
> 
> That is very true.  We should restrain ourselves and avoid going
> overboard piling shiny new toys on a not-so-useful foundation that
> is not expressive enough.  One feature that is probably much more
> needed on the foundation side would be some form of conditional
> output, without which built-in output elements like --show-notes
> cannot be emulated with --format option.

The embedded lua patch series I just posted may interest (or horrify)
the both of you:

  http://article.gmane.org/gmane.comp.version-control.git/206335

-Peff

Re: [PATCH 3/2] pretty: support right alignment

[Nguyen Thai Ngoc Duy]

On Thu, Sep 20, 2012 at 09:40:49AM -0700, Junio C Hamano wrote:
>  - I am wondering if somebody ever want to do this with a follow-up
>    patch:
> 
> 	Left %h%|Center %cd%|Right %ad
> 
>    Is %| a sensible choice for "flush right"?  I am wondering if it
>    makes more sense to make %|, %< and %> as "multi-column
>    introducer" (the example defines output with three columns) that
>    also tells how text inside each column is flushed inside the
>    column, e.g.
> 
> 	%>col 1 right flushed%|col 2 centered%< col 3 left flushed
> 
>    or something like that (we may want explicit "column width"
>    specifiers if we were to do this kind of thing).

Instead of thinking of "columns", we could go back to "placeholders",
or in printf terms, an "%s". In addition to plain %s, we need
something similar to "%*s" and "%-*s" to pad right and left.

Conceptually it's simpler. We don't have to deal with a bunch of
problems in your quotes that I cut out. Still it allows users to do
flush right, flush left and so on within limits. They just need to
think in terms of fixed-size cells. So...

%>(N)%? is transformed roughly to printf("%-*s", N, %?). Similarly
%<(N)%? becomes printf("%*s", N, %?). We could have %|(N) to pad both
%left and right (aka centered).

Better?

We might need a modifier or something to allow cutting (and maybe
putting ellipsis in place) to keep oversized cells from breaking the
layout.

The demonstration patch follows. You can't build because I don't post
the whole series.

-- 8< --
diff --git a/pretty.c b/pretty.c
index b1cec71..543c309 100644
--- a/pretty.c
+++ b/pretty.c
@@ -617,6 +617,12 @@ struct chunk {
 	size_t len;
 };
 
+enum flush_type {
+	no_flush,
+	flush_right,
+	flush_left
+};
+
 struct format_commit_context {
 	const struct commit *commit;
 	const struct pretty_print_context *pretty_ctx;
@@ -624,13 +630,14 @@ struct format_commit_context {
 	unsigned commit_message_parsed:1;
 	unsigned commit_signature_parsed:1;
 	unsigned use_color:1;
+	enum flush_type flush_type;
 	struct {
 		char *gpg_output;
 		char good_bad;
 		char *signer;
 	} signature;
 	char *message;
-	size_t width, indent1, indent2;
+	size_t width, indent1, indent2, padding;
 
 	/* These offsets are relative to the start of the commit message. */
 	struct chunk author;
@@ -944,6 +951,24 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 			return end - placeholder + 1;
 		} else
 			return 0;
+
+	case '<':
+	case '>':
+		if (placeholder[1] == '(') {
+			const char *start = placeholder + 2;
+			const char *end = strchr(start, ')');
+			char *next;
+			int width;
+			if (!end || end == start)
+				return 0;
+			width = strtoul(start, &next, 10);
+			if (next == start || width == 0)
+				return 0;
+			c->padding = width;
+			c->flush_type = *placeholder == '<' ? flush_right : flush_left;
+			return end - placeholder + 1;
+		}
+		return 0;
 	}
 
 	/* these depend on the commit */
@@ -1102,6 +1127,8 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 				 void *context)
 {
+	struct format_commit_context *c = context;
+	struct strbuf local_sb = STRBUF_INIT;
 	int consumed;
 	size_t orig_len;
 	enum {
@@ -1127,10 +1154,23 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	if (magic != NO_MAGIC)
 		placeholder++;
 
-	orig_len = sb->len;
-	consumed = format_commit_one(sb, placeholder, context);
-	if (magic == NO_MAGIC)
-		return consumed;
+	if (c->flush_type != no_flush) {
+		int len;
+		consumed = format_commit_one(&local_sb, placeholder, context);
+		/* the number of column, esc seq skipped */
+		len = utf8_strnwidth(local_sb.buf, -1, 1);
+		strbuf_addf(sb,
+			    c->flush_type == flush_right ? "%-*s" : "%*s",
+			    (int)(c->padding + (local_sb.len - len)),
+			    local_sb.buf);
+		strbuf_release(&local_sb);
+		c->flush_type = no_flush;
+	} else {
+		orig_len = sb->len;
+		consumed = format_commit_one(sb, placeholder, context);
+		if (magic == NO_MAGIC)
+			return consumed;
+	}
 
 	if ((orig_len == sb->len) && magic == DEL_LF_BEFORE_EMPTY) {
 		while (sb->len && sb->buf[sb->len - 1] == '\n')
-- 8< --

Re: [PATCH 2/2] pretty: support placeholders %C+ and %C-

[Junio C Hamano]

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

> %C+ tells the next specifiers that color is preferred. %C- the
> opposite. So far only %H, %h and %d support coloring.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/pretty-formats.txt |  2 ++
>  pretty.c                         | 13 ++++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index e3d8a83..6e287d6 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -142,6 +142,8 @@ The placeholders are:
>  - '%Cblue': switch color to blue
>  - '%Creset': reset color
>  - '%C(...)': color specification, as described in color.branch.* config option
> +- '%C+': enable coloring on the following placeholders if supported
> +- '%C-': disable coloring on the following placeholders

OK, so typically you replace some format placeholder "%?" in your
format string with "%C+%?%C-", because you cannot get away with
replacing it with "%C+%? and other things in the format you do not
know if they support coloring%C-".

If that is the case, does it really make sense to have %C-?

It smells as if it makes more sense to make _all_ %? placeholder
reset the effect of %C+ after they are done (even the ones that they
themselves do not color their own output elements), so that you can
mechanically replace "%?" with "%C+%?".

I dunno.

Re: [PATCH 2/2] pretty: support placeholders %C+ and %C-

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> %C+ tells the next specifiers that color is preferred. %C- the
>> opposite. So far only %H, %h and %d support coloring.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  Documentation/pretty-formats.txt |  2 ++
>>  pretty.c                         | 13 ++++++++++++-
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
>> index e3d8a83..6e287d6 100644
>> --- a/Documentation/pretty-formats.txt
>> +++ b/Documentation/pretty-formats.txt
>> @@ -142,6 +142,8 @@ The placeholders are:
>>  - '%Cblue': switch color to blue
>>  - '%Creset': reset color
>>  - '%C(...)': color specification, as described in color.branch.* config option
>> +- '%C+': enable coloring on the following placeholders if supported
>> +- '%C-': disable coloring on the following placeholders
>
> OK, so typically you replace some format placeholder "%?" in your
> format string with "%C+%?%C-", because you cannot get away with
> replacing it with "%C+%? and other things in the format you do not
> know if they support coloring%C-".
>
> If that is the case, does it really make sense to have %C-?
>
> It smells as if it makes more sense to make _all_ %? placeholder
> reset the effect of %C+ after they are done (even the ones that they
> themselves do not color their own output elements), so that you can
> mechanically replace "%?" with "%C+%?".
>
> I dunno.

Thinking about this a bit more, perhaps we would want a generic
mechanism to give parameters to various %? placeholders. This is not
limited to "I can do color but there is no mechanism for the user to
tell me that I should do color" %H, %h and %d may want to say.  An
obvious and immediate example is that %h might want to be told how
many hexdigits it should use.

Re: [PATCH 2/2] pretty: support placeholders %C+ and %C-

[Nguyen Thai Ngoc Duy]

On Fri, Sep 21, 2012 at 12:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> +- '%C+': enable coloring on the following placeholders if supported
>>> +- '%C-': disable coloring on the following placeholders
>>
>> OK, so typically you replace some format placeholder "%?" in your
>> format string with "%C+%?%C-", because you cannot get away with
>> replacing it with "%C+%? and other things in the format you do not
>> know if they support coloring%C-".
>>
>> If that is the case, does it really make sense to have %C-?

"%C+%?" should work. if %? does not support coloring, the %C+ effect
is simply ignored. In my use case, I don' really use %C- because I
always want color wherever possible. Though I suspect a user might
want to turn off coloring for certain part of the format string.

Replacing every %? with %C+%?%C- is really annoying in my "always color" case..

>> It smells as if it makes more sense to make _all_ %? placeholder
>> reset the effect of %C+ after they are done (even the ones that they
>> themselves do not color their own output elements), so that you can
>> mechanically replace "%?" with "%C+%?".

.. or even "%C+%?". My format string would become "%C+%h %C+%s%C+%d",
much harder to read.

> Thinking about this a bit more, perhaps we would want a generic
> mechanism to give parameters to various %? placeholders. This is not
> limited to "I can do color but there is no mechanism for the user to
> tell me that I should do color" %H, %h and %d may want to say.  An
> obvious and immediate example is that %h might want to be told how
> many hexdigits it should use.

Yeah that'd be nice. We already use %?(..) for %C. Maybe we can generalize that.

Still I'd like a way to define attributes for a group of placeholders
instead of just individuals. Continuing with the %?(...) syntax above
for specifying attributes for a specific placeholder, %(...) may be
used to specify global attributes that affect all following
placeholders until another %(...) stops the effect, or %?(...)
overrides it.
-- 
Duy