[PATCH] Specify a precision for the length of a subject string

[PATCH] Specify a precision for the length of a subject string

[Nathan W. Panike]

We can specify the precision of a subject string, so that length the subjects
viewed by the user do not grow beyond a bound set by the user, in a pretty
formatted string

This makes it possible to do, e.g., 

$ git log --pretty='%h %s' d165204 -1
d165204 git-p4: fix skipSubmitEdit regression

With this patch, the user can do

$ git log --pretty='%h %30s' d165204 -1
d165204 git-p4: fix skipSubmitEdit reg

This is useful when one is working on a system where the pager is lousy.
---
Since my colleagues tend to write long subject lines, I like to truncate them
so they do not overwhelm my terminal.

 builtin/shortlog.c |    2 +-
 commit.h           |    2 +-
 pretty.c           |   35 +++++++++++++++++++++++++++--------
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 37f3193..a5a07a3 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -98,7 +98,7 @@ static void insert_one_record(struct shortlog *log,
 	}
 	while (*oneline && isspace(*oneline) && *oneline != '\n')
 		oneline++;
-	format_subject(&subject, oneline, " ");
+	format_subject(&subject, oneline, " ", 0);
 	buffer = strbuf_detach(&subject, NULL);
 
 	if (dot3) {
diff --git a/commit.h b/commit.h
index 3745f12..a95f4ff 100644
--- a/commit.h
+++ b/commit.h
@@ -100,7 +100,7 @@ extern char *reencode_commit_message(const struct commit *commit,
 				     const char **encoding_p);
 extern void get_commit_format(const char *arg, struct rev_info *);
 extern const char *format_subject(struct strbuf *sb, const char *msg,
-				  const char *line_separator);
+				  const char *line_separator, int max_len);
 extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
 extern void format_commit_message(const struct commit *commit,
 				  const char *format, struct strbuf *sb,
diff --git a/pretty.c b/pretty.c
index 230fe1c..3af7a95 100644
--- a/pretty.c
+++ b/pretty.c
@@ -738,24 +738,33 @@ static void format_sanitized_subject(struct strbuf *sb, const char *msg)
 }
 
 const char *format_subject(struct strbuf *sb, const char *msg,
-			   const char *line_separator)
+			   const char *line_separator, int max_len)
 {
 	int first = 1;
-
+	int swritten = 0;
 	for (;;) {
 		const char *line = msg;
 		int linelen = get_one_line(line);
-
 		msg += linelen;
 		if (!linelen || is_empty_line(line, &linelen))
 			break;
 
 		if (!sb)
 			continue;
+		if (0 < max_len && max_len < swritten + linelen) {
+			linelen = max_len - swritten;	
+			if(linelen <= 0) {
+				linelen = 0;
+				continue;
+			}
+		}
 		strbuf_grow(sb, linelen + 2);
-		if (!first)
+		if (!first) {
 			strbuf_addstr(sb, line_separator);
+			swritten += strlen(line_separator);
+		}
 		strbuf_add(sb, line, linelen);
+		swritten += linelen;
 		first = 0;
 	}
 	return msg;
@@ -769,7 +778,7 @@ static void parse_commit_message(struct format_commit_context *c)
 	msg = skip_empty_lines(msg);
 	c->subject_off = msg - start;
 
-	msg = format_subject(NULL, msg, NULL);
+	msg = format_subject(NULL, msg, NULL, 0);
 	msg = skip_empty_lines(msg);
 	c->body_off = msg - start;
 
@@ -830,7 +839,17 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 	const char *msg = c->message;
 	struct commit_list *p;
 	int h1, h2;
+	int subject_max_len = 0,offset=0;
 
+	while(isdigit(*placeholder)) {
+		subject_max_len *= 10;
+		subject_max_len += *placeholder - '0';
+		++placeholder;
+		++offset;
+	}
+	if(offset > 0 && *placeholder != 's')
+		die("invalid --pretty format: "
+		    "'%%(digits)' can only be followed by an s");
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
 	case 'C':
@@ -1002,8 +1021,8 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 
 	switch (placeholder[0]) {
 	case 's':	/* subject */
-		format_subject(sb, msg + c->subject_off, " ");
-		return 1;
+		format_subject(sb, msg + c->subject_off, " ",subject_max_len);
+		return offset + 1;
 	case 'f':	/* sanitized subject */
 		format_sanitized_subject(sb, msg + c->subject_off);
 		return 1;
@@ -1189,7 +1208,7 @@ void pp_title_line(const struct pretty_print_context *pp,
 
 	strbuf_init(&title, 80);
 	*msg_p = format_subject(&title, *msg_p,
-				pp->preserve_subject ? "\n" : " ");
+				pp->preserve_subject ? "\n" : " ",0);
 
 	strbuf_grow(sb, title.len + 1024);
 	if (pp->subject) {
-- 
1.7.8.352.g876a6f.dirty

Re: [PATCH] Specify a precision for the length of a subject string

[Thomas Rast]

"Nathan W. Panike" <nathan.panike@gmail.com> writes:

> This is useful when one is working on a system where the pager is lousy.

I'm curious.  Are you saying your less does not have -S (or you do not
even have less), or do you have a reason not to use it?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] Specify a precision for the length of a subject string

[Nathan Panike]

On Tue, Dec 20, 2011 at 11:15:01PM +0100, Thomas Rast wrote:
> "Nathan W. Panike" <nathan.panike@gmail.com> writes:
> 
> > This is useful when one is working on a system where the pager is lousy.
> 
> I'm curious.  Are you saying your less does not have -S (or you do not
> even have less), or do you have a reason not to use it?
> 
> -- 
> Thomas Rast
> trast@{inf,student}.ethz.ch

The reason I thought of this initially was that I have a bot reporting commits
at $dayjob in an IRC channel. Since some of my colleagues commit with long
subject lines, I thought of this as a way to control the output of the bot
(e.g., by controlling the bot's input).

Nathan Panike

Re: [PATCH] Specify a precision for the length of a subject string

[Jeff King]

On Tue, Dec 20, 2011 at 04:07:54PM -0600, Nathan W. Panike wrote:

> We can specify the precision of a subject string, so that length the subjects
> viewed by the user do not grow beyond a bound set by the user, in a pretty
> formatted string
> 
> This makes it possible to do, e.g., 
> 
> $ git log --pretty='%h %s' d165204 -1
> d165204 git-p4: fix skipSubmitEdit regression
> 
> With this patch, the user can do
> 
> $ git log --pretty='%h %30s' d165204 -1
> d165204 git-p4: fix skipSubmitEdit reg

Hmm. I think the idea of limiting is OK (though personally, I would just
pipe through a filter that truncates long lines). But I'm a bit negative
on adding a tweak like this that only affects the subject. Is there a
reason I couldn't do %30gs, or %30f, or even some other placeholder?

Also, we already have %w to handle wrapping. Could this be handled in a
similar way (perhaps it could even be considered a special form of
wrapping)?

-Peff

Re: [PATCH] Specify a precision for the length of a subject string

[Andreas Schwab]

"Nathan W. Panike" <nathan.panike@gmail.com> writes:

> $ git log --pretty='%h %30s' d165204 -1

In C's formatted output this syntax denotes a minimum field width, not a
precision, so it will probably be surprising to many people.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Specify a precision for the length of a subject string

[Nathan Panike]

On Tue, Dec 20, 2011 at 11:38:43PM -0500, Jeff King wrote:
> On Tue, Dec 20, 2011 at 04:07:54PM -0600, Nathan W. Panike wrote:
> 
> > We can specify the precision of a subject string, so that length the subjects
> > viewed by the user do not grow beyond a bound set by the user, in a pretty
> > formatted string
> > 
> > This makes it possible to do, e.g., 
> > 
> > $ git log --pretty='%h %s' d165204 -1
> > d165204 git-p4: fix skipSubmitEdit regression
> > 
> > With this patch, the user can do
> > 
> > $ git log --pretty='%h %30s' d165204 -1
> > d165204 git-p4: fix skipSubmitEdit reg
> 
> Hmm. I think the idea of limiting is OK (though personally, I would just
> pipe through a filter that truncates long lines). But I'm a bit negative
> on adding a tweak like this that only affects the subject. Is there a
> reason I couldn't do %30gs, or %30f, or even some other placeholder?

The ones that make sense to limit are all those that depend on the subject, as the
above; it does not make sense to limit other fields that don't depend on the
subject, as they are fixed width, or have small variance. And it does not make
sense to me to limit the length of the body.

> 
> Also, we already have %w to handle wrapping. Could this be handled in a
> similar way (perhaps it could even be considered a special form of
> wrapping)?

I'll look at the wrapping code and see. Thanks for the idea.
> 
> -Peff

Re: [PATCH] Specify a precision for the length of a subject string

[Nathan Panike]

On Wed, Dec 21, 2011 at 12:26:25PM +0100, Andreas Schwab wrote:
> "Nathan W. Panike" <nathan.panike@gmail.com> writes:
> 
> > $ git log --pretty='%h %30s' d165204 -1
> 
> In C's formatted output this syntax denotes a minimum field width, not a
> precision, so it will probably be surprising to many people.

C semantics are already broken because (from git-log(1))

"If you add a - (minus sign) after % of a placeholder, line-feeds that
immediately precede the expansion are deleted if and only if the placeholder
expands to an empty string."

rather than indicating justification of the field.
> 
> Andreas.
> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."

Re: [PATCH] Specify a precision for the length of a subject string

[Miles Bader]

Nathan Panike <nwp@cs.wisc.edu> writes:
>> > $ git log --pretty='%h %30s' d165204 -1
>> 
>> In C's formatted output this syntax denotes a minimum field width, not a
>> precision, so it will probably be surprising to many people.
>
> C semantics are already broken because (from git-log(1))
>
> "If you add a - (minus sign) after % of a placeholder, line-feeds that
> immediately precede the expansion are deleted if and only if the placeholder
> expands to an empty string."
>
> rather than indicating justification of the field.

There's no reason to make it _worse_ though...

For your desired feature, why not just use the C printf syntax for this
functionality, a leading dot before the max length?  E.g. "%.30s".

-miles

-- 
Youth, n. The Period of Possibility, when Archimedes finds a fulcrum,
Cassandra has a following and seven cities compete for the honor of endowing a
living Homer.

Re: [PATCH] Specify a precision for the length of a subject string

[Jeff King]

On Wed, Dec 21, 2011 at 08:51:13AM -0600, Nathan Panike wrote:

> > Hmm. I think the idea of limiting is OK (though personally, I would just
> > pipe through a filter that truncates long lines). But I'm a bit negative
> > on adding a tweak like this that only affects the subject. Is there a
> > reason I couldn't do %30gs, or %30f, or even some other placeholder?
> 
> The ones that make sense to limit are all those that depend on the subject, as the
> above; it does not make sense to limit other fields that don't depend on the
> subject, as they are fixed width, or have small variance. And it does not make
> sense to me to limit the length of the body.

I agree the subject is the most likely place. I was thinking one might
want to do it with the body, too. But whether it would be "I want N
bytes of the body" or "truncate each body line at N bytes without
wrapping", I don't know.

-Peff

Re: [PATCH] Specify a precision for the length of a subject string

[Jeff King]

On Fri, Dec 23, 2011 at 05:09:58AM -0500, Jeff King wrote:

> > The ones that make sense to limit are all those that depend on the subject, as the
> > above; it does not make sense to limit other fields that don't depend on the
> > subject, as they are fixed width, or have small variance. And it does not make
> > sense to me to limit the length of the body.
> 
> I agree the subject is the most likely place. I was thinking one might
> want to do it with the body, too. But whether it would be "I want N
> bytes of the body" or "truncate each body line at N bytes without
> wrapping", I don't know.

Another place that might want it is %N (commit notes).

Here's how I would have done it. Not involving %w at all, but applying
equally to all placeholders.

  [1/2]: pretty: refactor --format "magic" placeholders
  [2/2]: pretty: allow "max-size" magic for all placeholders

I'm not personally interested in this topic, so I won't be pushing for
this to be included in git. But if it feels like the right direction for
you, feel free to be build on it and post it as part of your series (or
just take it as inspiration and make your own commits). Off the top of
my head, it needs:

  - documentation updates
  - tests
  - userformat_want_item should also respect the same magic (it already
    duplicates some of the "-/+/ " magic. It might be nice to factor
    that part out).

-Peff

[PATCH 1/2] pretty: refactor --format "magic" placeholders

[Jeff King]

Instead of assuming each magic token is a single character,
let's handle arbitrary-sized magic.

Signed-off-by: Jeff King <peff@peff.net>
---
 pretty.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/pretty.c b/pretty.c
index 230fe1c..7b4d098 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1018,6 +1018,7 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 				 void *context)
 {
 	int consumed;
+	int magic_len = 0;
 	size_t orig_len;
 	enum {
 		NO_MAGIC,
@@ -1039,13 +1040,13 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	default:
 		break;
 	}
-	if (magic != NO_MAGIC)
+	if (magic != NO_MAGIC) {
+		magic_len++;
 		placeholder++;
+	}
 
 	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')
@@ -1056,7 +1057,7 @@ 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;
+	return consumed + magic_len;
 }
 
 static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
-- 
1.7.8.1.3.gba11d

[PATCH 2/2] pretty: allow "max-size" magic for all placeholders

[Jeff King]

You can now truncate a given placeholder to no more than a
certain number of characters with something like "%30s".

Signed-off-by: Jeff King <peff@peff.net>
---
This just uses the made-up "%30s" syntax, but you could easily tweak it
to handle "%.30s" or whatever.

 pretty.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/pretty.c b/pretty.c
index 7b4d098..06d96a7 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1019,6 +1019,7 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 {
 	int consumed;
 	int magic_len = 0;
+	int max_len = 0;
 	size_t orig_len;
 	enum {
 		NO_MAGIC,
@@ -1045,9 +1046,22 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 		placeholder++;
 	}
 
+	if (isdigit(placeholder[0])) {
+		char *end;
+		max_len = strtoul(placeholder, &end, 10);
+		magic_len += (end - placeholder);
+		placeholder = end;
+	}
+
 	orig_len = sb->len;
 	consumed = format_commit_one(sb, placeholder, context);
 
+	if (max_len) {
+		size_t end = orig_len + max_len;
+		if (end < sb->len)
+			strbuf_setlen(sb, end);
+	}
+
 	if ((orig_len == sb->len) && magic == DEL_LF_BEFORE_EMPTY) {
 		while (sb->len && sb->buf[sb->len - 1] == '\n')
 			strbuf_setlen(sb, sb->len - 1);
-- 
1.7.8.1.3.gba11d

Re: [PATCH] Specify a precision for the length of a subject string

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's how I would have done it. Not involving %w at all, but applying
> equally to all placeholders.

Hmm, just curious why you rejected the %w() approach, as enhancing %w
sounded to me like a better approach at the design level, but that was a
knee-jerk reaction without inspecting the codepaths involved myself hence
not knowing the potential amount of work required.

>   - userformat_want_item should also respect the same magic (it already
>     duplicates some of the "-/+/ " magic. It might be nice to factor
>     that part out).

I recall this was a bit of a bear when I looked at the area last time.

Re: [PATCH] Specify a precision for the length of a subject string

[Jeff King]

On Fri, Dec 23, 2011 at 12:58:00PM -0800, Junio C Hamano wrote:

> > Here's how I would have done it. Not involving %w at all, but applying
> > equally to all placeholders.
> 
> Hmm, just curious why you rejected the %w() approach, as enhancing %w
> sounded to me like a better approach at the design level, but that was a
> knee-jerk reaction without inspecting the codepaths involved myself hence
> not knowing the potential amount of work required.

Not so much rejecting as I took a quick look at how I would have done
what your original patch did, and it was simple enough that I took it
all the way to working and decided to post it. I left it up to you to
decide whether using %w would be more sensible. I just wanted to present
another alternative for discussion.

-Peff

Re: [PATCH] Specify a precision for the length of a subject string

[Jeff King]

On Fri, Dec 23, 2011 at 06:02:28PM -0500, Jeff King wrote:

> On Fri, Dec 23, 2011 at 12:58:00PM -0800, Junio C Hamano wrote:
> 
> > > Here's how I would have done it. Not involving %w at all, but applying
> > > equally to all placeholders.
> > 
> > Hmm, just curious why you rejected the %w() approach, as enhancing %w
> > sounded to me like a better approach at the design level, but that was a
> > knee-jerk reaction without inspecting the codepaths involved myself hence
> > not knowing the potential amount of work required.
> 
> Not so much rejecting as I took a quick look at how I would have done
> what your original patch did, and it was simple enough that I took it
> all the way to working and decided to post it. I left it up to you to
> decide whether using %w would be more sensible. I just wanted to present
> another alternative for discussion.

Eh, I misread the "From" header. All of the "you" there is "Nathan".

-Peff