threads / patch / 41709

patchpretty-print: de-tabify indented logs to make things line up properly

Subject: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

## tl;dr

57 messages between Mar 16, 2016 and Apr 5, 2016. Diffs are folded; open one to read it.

replies: 56people: 6as markdown or json

Linus Torvalds· Mar 16, 2016, 16:29 UTC · lore
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 16 Mar 2016 09:15:53 -0700
Subject: [PATCH] pretty-print: de-tabify indented logs to make things line up properly
This should all line up:
  Column 1	Column 2
  --------	--------
  A		B
  ABCD		EFGH
  SPACES        Instead of Tabs
Even with multi-byte UTF8 characters:
  Column 1	Column 2
  --------	--------
  Ä		B
  åäö		100
  A Møøse	once bit my sister..
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This seems to work for me, and while there is some cost, it's minimal. Doing a "git log > /dev/null" of the current git tree is about 1% slower because of the tab-finding. A tree with a lot of tabs in the commit messages would be more noticeable, because then you actually end up hitting the whole "how wide is this" issue.

(But if the tabs are all at the beginning of a line, you'd still be ok and avoid the utf8 width calculations).

Comments?
 pretty.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 74 insertions(+), 2 deletions(-)
Show changes to pretty.c +74 −2
diff --git a/pretty.c b/pretty.c
index 92b2870a7eab..0b40457f99f0 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1629,6 +1629,76 @@ void pp_title_line(struct pretty_print_context *pp,
 	strbuf_release(&title);
 }
 
+static int pp_utf8_width(const char *start, const char *end)
+{
+	int width = 0;
+	size_t remain = end - start;
+
+	while (remain) {
+		int n = utf8_width(&start, &remain);
+		if (n < 0 || !start)
+			return -1;
+		width += n;
+	}
+	return width;
+}
+
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * perhaps the whole line (without the final newline)
+ *
+ * Why "perhaps"? If there are tabs in the indented line
+ * it will print it out in order to de-tabify the line.
+ *
+ * But if there are no tabs, we just fall back on the
+ * normal "print the whole line".
+ */
+static int pp_handle_indent(struct strbuf *sb, int indent,
+			     const char *line, int linelen)
+{
+	const char *tab;
+
+	strbuf_addchars(sb, ' ', indent);
+
+	tab = memchr(line, '\t', linelen);
+	if (!tab)
+		return 0;
+
+	do {
+		int width = pp_utf8_width(line, tab);
+
+		/*
+		 * If it wasn't well-formed utf8, or it
+		 * had characters with badly defined
+		 * width (control characters etc), just
+		 * give up on trying to align things.
+		 */
+		if (width < 0)
+			break;
+
+		/* Output the data .. */
+		strbuf_add(sb, line, tab - line);
+
+		/* .. and the de-tabified tab */
+		strbuf_addchars(sb, ' ', 8-(width & 7));
+
+		/* Skip over the printed part .. */
+		linelen -= 1+tab-line;
+		line = tab + 1;
+
+		/* .. and look for the next tab */
+		tab = memchr(line, '\t', linelen);
+	} while (tab);
+
+	/*
+	 * Print out everything after the last tab without
+	 * worrying about width - there's nothing more to
+	 * align.
+	 */
+	strbuf_add(sb, line, linelen);
+	return 1;
+}
+
 void pp_remainder(struct pretty_print_context *pp,
 		  const char **msg_p,
 		  struct strbuf *sb,
@@ -1652,8 +1722,10 @@ void pp_remainder(struct pretty_print_context *pp,
 		first = 0;
 
 		strbuf_grow(sb, linelen + indent + 20);
-		if (indent)
-			strbuf_addchars(sb, ' ', indent);
+		if (indent) {
+			if (pp_handle_indent(sb, indent, line, linelen))
+				linelen = 0;
+		}
 		strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
 	}
-- 
2.8.0.rc2
Linus Torvalds· Mar 16, 2016, 16:52 UTC · re: Linus Torvalds · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

On Wed, Mar 16, 2016 at 9:29 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:

Show 16 quoted lines
>
> This should all line up:
>
>   Column 1      Column 2
>   --------      --------
>   A             B
>   ABCD          EFGH
>   SPACES        Instead of Tabs
>
> Even with multi-byte UTF8 characters:
>
>   Column 1      Column 2
>   --------      --------
>   Ä             B
>   åäö           100
>   A Møøse       once bit my sister..
So with current git, it looks like this for me:
      Column 1  Column 2
      --------  --------
      A         B
      ABCD              EFGH
      SPACES        Instead of Tabs
    Even with multi-byte UTF8 characters:
      Column 1  Column 2
      --------  --------
      Ä         B
      åäö               100
      A Møøse   once bit my sister..
and with that patch it looks much better:
      Column 1      Column 2
      --------      --------
      A             B
      ABCD          EFGH
      SPACES        Instead of Tabs
    Even with multi-byte UTF8 characters:
      Column 1      Column 2
      --------      --------
      Ä             B
      åäö           100
      A Møøse       once bit my sister..

(of course, to see that assumes that you have a fixed-sized font in your mail viewer, and that the spacing didn't get screwed up by my cut-and-paste).

             Linus
Junio C Hamano· Mar 16, 2016, 18:01 UTC · re: Linus Torvalds · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 33 quoted lines
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 16 Mar 2016 09:15:53 -0700
> Subject: [PATCH] pretty-print: de-tabify indented logs to make things line up properly
>
> This should all line up:
>
>   Column 1	Column 2
>   --------	--------
>   A		B
>   ABCD		EFGH
>   SPACES        Instead of Tabs
>
> Even with multi-byte UTF8 characters:
>
>   Column 1	Column 2
>   --------	--------
>   Ä		B
>   åäö		100
>   A Møøse	once bit my sister..
>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>
> This seems to work for me, and while there is some cost, it's minimal. 
> Doing a "git log > /dev/null" of the current git tree is about 1% slower 
> because of the tab-finding. A tree with a lot of tabs in the commit 
> messages would be more noticeable, because then you actually end up 
> hitting the whole "how wide is this" issue.
>
> (But if the tabs are all at the beginning of a line, you'd still be ok 
> and avoid the utf8 width calculations).
>
> Comments?
I stared at it for a while, and didn't spot anything wrong with it.
I did wonder about two things, though:
 (1) if turning your "preparation; do { ... } while()" into
     "while () { }" would make the result a bit easier to read;
 (2) if we can somehow eliminate duplication of "tab + 1" (spelled
     differently on the previous line as "1+tab"), the end result
     may get easier to follow.
but both are minor.
Show 97 quoted lines
>  pretty.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index 92b2870a7eab..0b40457f99f0 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1629,6 +1629,76 @@ void pp_title_line(struct pretty_print_context *pp,
>  	strbuf_release(&title);
>  }
>  
> +static int pp_utf8_width(const char *start, const char *end)
> +{
> +	int width = 0;
> +	size_t remain = end - start;
> +
> +	while (remain) {
> +		int n = utf8_width(&start, &remain);
> +		if (n < 0 || !start)
> +			return -1;
> +		width += n;
> +	}
> +	return width;
> +}
> +
> +/*
> + * pp_handle_indent() prints out the intendation, and
> + * perhaps the whole line (without the final newline)
> + *
> + * Why "perhaps"? If there are tabs in the indented line
> + * it will print it out in order to de-tabify the line.
> + *
> + * But if there are no tabs, we just fall back on the
> + * normal "print the whole line".
> + */
> +static int pp_handle_indent(struct strbuf *sb, int indent,
> +			     const char *line, int linelen)
> +{
> +	const char *tab;
> +
> +	strbuf_addchars(sb, ' ', indent);
> +
> +	tab = memchr(line, '\t', linelen);
> +	if (!tab)
> +		return 0;
> +
> +	do {
> +		int width = pp_utf8_width(line, tab);
> +
> +		/*
> +		 * If it wasn't well-formed utf8, or it
> +		 * had characters with badly defined
> +		 * width (control characters etc), just
> +		 * give up on trying to align things.
> +		 */
> +		if (width < 0)
> +			break;
> +
> +		/* Output the data .. */
> +		strbuf_add(sb, line, tab - line);
> +
> +		/* .. and the de-tabified tab */
> +		strbuf_addchars(sb, ' ', 8-(width & 7));
> +
> +		/* Skip over the printed part .. */
> +		linelen -= 1+tab-line;
> +		line = tab + 1;
> +
> +		/* .. and look for the next tab */
> +		tab = memchr(line, '\t', linelen);
> +	} while (tab);
> +
> +	/*
> +	 * Print out everything after the last tab without
> +	 * worrying about width - there's nothing more to
> +	 * align.
> +	 */
> +	strbuf_add(sb, line, linelen);
> +	return 1;
> +}
> +
>  void pp_remainder(struct pretty_print_context *pp,
>  		  const char **msg_p,
>  		  struct strbuf *sb,
> @@ -1652,8 +1722,10 @@ void pp_remainder(struct pretty_print_context *pp,
>  		first = 0;
>  
>  		strbuf_grow(sb, linelen + indent + 20);
> -		if (indent)
> -			strbuf_addchars(sb, ' ', indent);
> +		if (indent) {
> +			if (pp_handle_indent(sb, indent, line, linelen))
> +				linelen = 0;
> +		}
>  		strbuf_add(sb, line, linelen);
>  		strbuf_addch(sb, '\n');
>  	}
Linus Torvalds· Mar 16, 2016, 18:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

On Wed, Mar 16, 2016 at 11:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>  (1) if turning your "preparation; do { ... } while()" into
>      "while () { }" would make the result a bit easier to read;

So it's probably partly taste, but I will also disagree with your "easier to read", because of the way the code is logically structured.

In particular, the "no TAB" case is actually *fundamentally* different from the "no TAB at the end" case. The return value is different, and the caller does very different things - the code tries to make it very clear that that "no TAB" situation is very different from "we found a TAB".

So it's not "preparation + do-while".

It's "preparation + handle the no-TAB case differently", and then the "do-while" is very natural because by the time we get to the "ok, we are now going to need to do something about the line" stage, we already know we have a tab.

But the code *could* be made to just always do the whole "strbuf_add()", and not return a return value at all, and the no-tab case wouldn't be explicitly written to be different.

Let me know if you'd prefer that variant, and I'll send a new version.
>  (2) if we can somehow eliminate duplication of "tab + 1" (spelled
>      differently on the previous line as "1+tab"), the end result
>      may get easier to follow.

Yeah, I considered that. Either by just doing "tab++" before (so the +1" would come from that in both cases), or by introducing a new variable like

    ptrdiff_t bytes_used;
    ...
    bytes_used = 1 + tab - line;
and then just doing
    line += bytes_used;
    linelen -= bytes_used;

and the code I wrote just didn't do any of those temporary updates, and instead just did the "+1" by hand in both cases.

Again, I can redo the patch, just tell me which model you prefer.
                 Linus
Junio C Hamano· Mar 16, 2016, 19:32 UTC · re: Linus Torvalds · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 20 quoted lines
> On Wed, Mar 16, 2016 at 11:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>  (1) if turning your "preparation; do { ... } while()" into
>>      "while () { }" would make the result a bit easier to read;
>
> So it's probably partly taste, but I will also disagree with your
> "easier to read", because of the way the code is logically structured.
>
> In particular, the "no TAB" case is actually *fundamentally* different
> from the "no TAB at the end" case. The return value is different, and
> the caller does very different things - the code tries to make it very
> clear that that "no TAB" situation is very different from "we found a
> TAB".
>
> So it's not "preparation + do-while".
>
> It's "preparation + handle the no-TAB case differently", and then the
> "do-while" is very natural because by the time we get to the "ok, we
> are now going to need to do something about the line" stage, we
> already know we have a tab.
OK, I agree with that viewpoint; retracted.
Show 19 quoted lines
>>  (2) if we can somehow eliminate duplication of "tab + 1" (spelled
>>      differently on the previous line as "1+tab"), the end result
>>      may get easier to follow.
>
> Yeah, I considered that. Either by just doing "tab++" before (so the
> +1" would come from that in both cases), or by introducing a new
> variable like
>
>     ptrdiff_t bytes_used;
>     ...
>     bytes_used = 1 + tab - line;
>
> and then just doing
>
>     line += bytes_used;
>     linelen -= bytes_used;
>
> and the code I wrote just didn't do any of those temporary updates,
> and instead just did the "+1" by hand in both cases.

The above is most likely what I would have written if I were doing this patch. I could squash it to save a round-trip, but let me run the testsuite first to see if we need adjustments to existing tests.

Also your idea:
> But the code *could* be made to just always do the whole
> "strbuf_add()", and not return a return value at all, and the no-tab
> case wouldn't be explicitly written to be different.

may give us a better structure if we are going to give users a knob to disable this tab expansion, i.e. move the addition of 4 spaces to the caller, name the body of such a function strbuf_expand_add(), and then make the caller do something like this perhaps?

Show changes to diff +7 −3
@@ -1723,10 +1711,14 @@ void pp_remainder(struct pretty_print_context *pp,
 
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent) {
-			if (pp_handle_indent(sb, indent, line, linelen))
-				linelen = 0;
+			strbuf_addchars(sb, ' ', indent);
+			if (pp->fmt == CMIT_FMT_EXPAND_TABS)
+				strbuf_expand_add(sb, line, linelen);
+			else
+				strbuf_add(sb, line, linelen);
+		} else {
+			strbuf_add(sb, line, linelen);
 		}
-		strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
 	}
 }
Junio C Hamano· Mar 16, 2016, 19:47 UTC · re: Junio C Hamano · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

Junio C Hamano <gitster@pobox.com> writes:
> The above is most likely what I would have written if I were doing
> this patch.  I could squash it to save a round-trip, but let me run
> the testsuite first to see if we need adjustments to existing tests.

Strangely running t4201 with your patch (without any squashing) seems to show a breakage in shortlog. I won't be able to come back to this topic for at least a few hours, so this is just a single bit "breaks" report, without "how and why" analysis, sorry.

Show 30 quoted lines
>
> Also your idea:
>
>> But the code *could* be made to just always do the whole
>> "strbuf_add()", and not return a return value at all, and the no-tab
>> case wouldn't be explicitly written to be different.
>
> may give us a better structure if we are going to give users a knob
> to disable this tab expansion, i.e. move the addition of 4 spaces to
> the caller, name the body of such a function strbuf_expand_add(),
> and then make the caller do something like this perhaps?
>
> @@ -1723,10 +1711,14 @@ void pp_remainder(struct pretty_print_context *pp,
>  
>  		strbuf_grow(sb, linelen + indent + 20);
>  		if (indent) {
> -			if (pp_handle_indent(sb, indent, line, linelen))
> -				linelen = 0;
> +			strbuf_addchars(sb, ' ', indent);
> +			if (pp->fmt == CMIT_FMT_EXPAND_TABS)
> +				strbuf_expand_add(sb, line, linelen);
> +			else
> +				strbuf_add(sb, line, linelen);
> +		} else {
> +			strbuf_add(sb, line, linelen);
>  		}
> -		strbuf_add(sb, line, linelen);
>  		strbuf_addch(sb, '\n');
>  	}
>  }
Linus Torvalds· Mar 16, 2016, 19:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

On Wed, Mar 16, 2016 at 12:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
>
> Strangely running t4201 with your patch (without any squashing)
> seems to show a breakage in shortlog.  I won't be able to come back
> to this topic for at least a few hours, so this is just a single bit
> "breaks" report, without "how and why" analysis, sorry.

It's because those things have tabs in their first line, so the output now differs from the expected one exactly because of the tab-vs-space expansion.

The wrapping logic is then also different, because the .wrapping code does the tabs as "align to 8 chars" while the new code does tabs as "align to 8 chars modulo the indent offset".

I only looked at the first case, but I assume the others are just more of the same. We'd just adjust the expected output, I assume.

               Linus
Junio C Hamano· Mar 16, 2016, 21:37 UTC · re: Linus Torvalds · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 10 quoted lines
> On Wed, Mar 16, 2016 at 12:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Strangely running t4201 with your patch (without any squashing)
>> seems to show a breakage in shortlog.  I won't be able to come back
>> to this topic for at least a few hours, so this is just a single bit
>> "breaks" report, without "how and why" analysis, sorry.
>
> It's because those things have tabs in their first line, so the output
> now differs from the expected one exactly because of the tab-vs-space
> expansion.

What surprised me was that this new expand logic triggered for shortlog, actually. I somehow assumed the caller that called de-tabify helper was only called for --pretty=medium.

Linus Torvalds· Mar 16, 2016, 22:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

On Wed, Mar 16, 2016 at 2:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> What surprised me was that this new expand logic triggered for
> shortlog, actually.  I somehow assumed the caller that called
> de-tabify helper was only called for --pretty=medium.

I guess that would be ok, since shortlog by definition can't have any issues with multiple lines lining up with each other.

At the same time, it might be a bit odd to show tabs in that first line differently for the one-line vs multi-line log version. But maybe it isn't - I think shortlog is the only thing that does that wrapping anyway, so shortlog is already special.

I think the reason shortlog output gets both the de-tab and the wrapping is that shortlog_add_commit() just calls pretty_print_commit with CMIT_FMT_USERFORMAT.

            Linus
Junio C Hamano· Mar 17, 2016, 23:13 UTC · re: Linus Torvalds · lore

[PATCH v2 1/4] pretty-print: de-tabify indented logs to make things line up properly

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 16 Mar 2016 09:15:53 -0700

A commit log message sometimes tries to line things up using tabs, assuming fixed-width font with the standard 8-place tab settings. Viewing such a commit however does not work well in "git log", as we indent the lines by prefixing 4 spaces in front of them.

This should all line up:
  Column 1	Column 2
  --------	--------
  A		B
  ABCD		EFGH
  SPACES        Instead of Tabs
Even with multi-byte UTF8 characters:
  Column 1	Column 2
  --------	--------
  Ä		B
  åäö		100
  A Møøse	once bit my sister..

Tab-expand the lines in "git log --pretty=medium" output (which is the default), before prefixing 4 spaces.

This breaks a few tests in t4201, that tests "git shortlog".
 - One passes "git log" output to "git shortlog" to use the latter
   as a filter and does not expect the output of the former to be
   de-tabified.
 - The other expects that "git shortlog", when it reads the first
   line of the commit and produces the output itself, does not
   de-tabify it.
Mark them as expecting failure for now.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pretty.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 t/t4201-shortlog.sh |  4 +--
 2 files changed, 76 insertions(+), 4 deletions(-)
Show changes to 2 files +76 −4

pretty.c, t/t4201-shortlog.sh

diff --git a/pretty.c b/pretty.c
index 92b2870..0b40457 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1629,6 +1629,76 @@ void pp_title_line(struct pretty_print_context *pp,
 	strbuf_release(&title);
 }
 
+static int pp_utf8_width(const char *start, const char *end)
+{
+	int width = 0;
+	size_t remain = end - start;
+
+	while (remain) {
+		int n = utf8_width(&start, &remain);
+		if (n < 0 || !start)
+			return -1;
+		width += n;
+	}
+	return width;
+}
+
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * perhaps the whole line (without the final newline)
+ *
+ * Why "perhaps"? If there are tabs in the indented line
+ * it will print it out in order to de-tabify the line.
+ *
+ * But if there are no tabs, we just fall back on the
+ * normal "print the whole line".
+ */
+static int pp_handle_indent(struct strbuf *sb, int indent,
+			     const char *line, int linelen)
+{
+	const char *tab;
+
+	strbuf_addchars(sb, ' ', indent);
+
+	tab = memchr(line, '\t', linelen);
+	if (!tab)
+		return 0;
+
+	do {
+		int width = pp_utf8_width(line, tab);
+
+		/*
+		 * If it wasn't well-formed utf8, or it
+		 * had characters with badly defined
+		 * width (control characters etc), just
+		 * give up on trying to align things.
+		 */
+		if (width < 0)
+			break;
+
+		/* Output the data .. */
+		strbuf_add(sb, line, tab - line);
+
+		/* .. and the de-tabified tab */
+		strbuf_addchars(sb, ' ', 8-(width & 7));
+
+		/* Skip over the printed part .. */
+		linelen -= 1+tab-line;
+		line = tab + 1;
+
+		/* .. and look for the next tab */
+		tab = memchr(line, '\t', linelen);
+	} while (tab);
+
+	/*
+	 * Print out everything after the last tab without
+	 * worrying about width - there's nothing more to
+	 * align.
+	 */
+	strbuf_add(sb, line, linelen);
+	return 1;
+}
+
 void pp_remainder(struct pretty_print_context *pp,
 		  const char **msg_p,
 		  struct strbuf *sb,
@@ -1652,8 +1722,10 @@ void pp_remainder(struct pretty_print_context *pp,
 		first = 0;
 
 		strbuf_grow(sb, linelen + indent + 20);
-		if (indent)
-			strbuf_addchars(sb, ' ', indent);
+		if (indent) {
+			if (pp_handle_indent(sb, indent, line, linelen))
+				linelen = 0;
+		}
 		strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
 	}
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 7600a3e..987b708 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -93,7 +93,7 @@ test_expect_success 'output from user-defined format is re-wrapped' '
 	test_cmp expect log.predictable
 '
 
-test_expect_success !MINGW 'shortlog wrapping' '
+test_expect_failure !MINGW 'shortlog wrapping' '
 	cat >expect <<\EOF &&
 A U Thor (5):
       Test
@@ -114,7 +114,7 @@ EOF
 	test_cmp expect out
 '
 
-test_expect_success !MINGW 'shortlog from non-git directory' '
+test_expect_failure !MINGW 'shortlog from non-git directory' '
 	git log HEAD >log &&
 	GIT_DIR=non-existing git shortlog -w <log >out &&
 	test_cmp expect out
-- 
2.8.0-rc3-175-g64dcf62
Junio C Hamano· Mar 17, 2016, 23:15 UTC · re: Junio C Hamano · lore

[PATCH v2 2/4] pretty-print: simplify the interaction between pp_handle_indent() and its caller

Instead of sometimes handling the output itself and some other times forcing the caller handle the output, make pp_handle_indent() responsible to handle the output for all cases.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This and the other two patches that follow show what I meant
   during the discussion.
 pretty.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)
Show changes to pretty.c +9 −23
diff --git a/pretty.c b/pretty.c
index 0b40457..6d657fc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1645,26 +1645,17 @@ static int pp_utf8_width(const char *start, const char *end)
 
 /*
  * pp_handle_indent() prints out the intendation, and
- * perhaps the whole line (without the final newline)
- *
- * Why "perhaps"? If there are tabs in the indented line
- * it will print it out in order to de-tabify the line.
- *
- * But if there are no tabs, we just fall back on the
- * normal "print the whole line".
+ * the whole line (without the final newline), after
+ * de-tabifying.
  */
-static int pp_handle_indent(struct strbuf *sb, int indent,
+static void pp_handle_indent(struct strbuf *sb, int indent,
 			     const char *line, int linelen)
 {
 	const char *tab;
 
 	strbuf_addchars(sb, ' ', indent);
 
-	tab = memchr(line, '\t', linelen);
-	if (!tab)
-		return 0;
-
-	do {
+	while ((tab = memchr(line, '\t', linelen)) != NULL) {
 		int width = pp_utf8_width(line, tab);
 
 		/*
@@ -1685,10 +1676,7 @@ static int pp_handle_indent(struct strbuf *sb, int indent,
 		/* Skip over the printed part .. */
 		linelen -= 1+tab-line;
 		line = tab + 1;
-
-		/* .. and look for the next tab */
-		tab = memchr(line, '\t', linelen);
-	} while (tab);
+	}
 
 	/*
 	 * Print out everything after the last tab without
@@ -1696,7 +1684,6 @@ static int pp_handle_indent(struct strbuf *sb, int indent,
 	 * align.
 	 */
 	strbuf_add(sb, line, linelen);
-	return 1;
 }
 
 void pp_remainder(struct pretty_print_context *pp,
@@ -1722,11 +1709,10 @@ void pp_remainder(struct pretty_print_context *pp,
 		first = 0;
 
 		strbuf_grow(sb, linelen + indent + 20);
-		if (indent) {
-			if (pp_handle_indent(sb, indent, line, linelen))
-				linelen = 0;
-		}
-		strbuf_add(sb, line, linelen);
+		if (indent)
+			pp_handle_indent(sb, indent, line, linelen);
+		else
+			strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
 	}
 }
-- 
2.8.0-rc3-175-g64dcf62
Junio C Hamano· Mar 17, 2016, 23:15 UTC · re: Junio C Hamano · lore

[PATCH v2 3/4] pretty-print: further abstract out pp_handle_indent()

Separate the call to add 4-space indent, and a new helper to add a line after de-tabifying.

The new helper function strbuf_add_tabexpand() could later be moved to strbuf.[ch] if other callers need to.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pretty.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)
Show changes to pretty.c +15 −9
diff --git a/pretty.c b/pretty.c
index 6d657fc..717ceed 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1643,18 +1643,12 @@ static int pp_utf8_width(const char *start, const char *end)
 	return width;
 }
 
-/*
- * pp_handle_indent() prints out the intendation, and
- * the whole line (without the final newline), after
- * de-tabifying.
- */
-static void pp_handle_indent(struct strbuf *sb, int indent,
-			     const char *line, int linelen)
+
+static void strbuf_add_tabexpand(struct strbuf *sb,
+				 const char *line, int linelen)
 {
 	const char *tab;
 
-	strbuf_addchars(sb, ' ', indent);
-
 	while ((tab = memchr(line, '\t', linelen)) != NULL) {
 		int width = pp_utf8_width(line, tab);
 
@@ -1686,6 +1680,18 @@ static void pp_handle_indent(struct strbuf *sb, int indent,
 	strbuf_add(sb, line, linelen);
 }
 
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * the whole line (without the final newline), after
+ * de-tabifying.
+ */
+static void pp_handle_indent(struct strbuf *sb, int indent,
+			     const char *line, int linelen)
+{
+	strbuf_addchars(sb, ' ', indent);
+	strbuf_add_tabexpand(sb, line, linelen);
+}
+
 void pp_remainder(struct pretty_print_context *pp,
 		  const char **msg_p,
 		  struct strbuf *sb,
-- 
2.8.0-rc3-175-g64dcf62
Junio C Hamano· Mar 17, 2016, 23:16 UTC · re: Junio C Hamano · lore

[PATCH 4/4] pretty-print: add --pretty=noexpand

It is reasonable for tweak the default output mode for "git log" to untabify the commit log message, it sometimes may be necessary to see the output without tab expansion.

Invent a new --pretty option to do this. Use this to unbreak the test breakages, where "git shortlog" and output are tested.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/pretty-formats.txt | 10 ++++++++++
 Documentation/pretty-options.txt |  2 +-
 commit.h                         |  1 +
 pretty.c                         | 12 +++++++++---
 t/t4201-shortlog.sh              |  6 +++---
 5 files changed, 24 insertions(+), 7 deletions(-)
Show changes to 5 files +24 −7

Documentation/pretty-formats.txt, Documentation/pretty-options.txt, commit.h, pretty.c, t/t4201-shortlog.sh

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 671cebd..173b932 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -39,6 +39,16 @@ This is designed to be as compact as possible.
 
 	      <title line>
 
+	      <full commit message, tab-expanded>
+
+* 'noexpand'
+
+	  commit <sha1>
+	  Author: <author>
+	  Date:   <author date>
+
+	      <title line>
+
 	      <full commit message>
 
 * 'full'
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 4b659ac..7032b1a 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -3,7 +3,7 @@
 
 	Pretty-print the contents of the commit logs in a given format,
 	where '<format>' can be one of 'oneline', 'short', 'medium',
-	'full', 'fuller', 'email', 'raw', 'format:<string>'
+	'full', 'fuller', 'email', 'raw', 'noexpand', 'format:<string>'
 	and 'tformat:<string>'.  When '<format>' is none of the above,
 	and has '%placeholder' in it, it acts as if
 	'--pretty=tformat:<format>' were given.
diff --git a/commit.h b/commit.h
index 5d58be0..d511c61 100644
--- a/commit.h
+++ b/commit.h
@@ -126,6 +126,7 @@ enum cmit_fmt {
 	CMIT_FMT_RAW,
 	CMIT_FMT_MEDIUM,
 	CMIT_FMT_DEFAULT = CMIT_FMT_MEDIUM,
+	CMIT_FMT_NOEXPAND,
 	CMIT_FMT_SHORT,
 	CMIT_FMT_FULL,
 	CMIT_FMT_FULLER,
diff --git a/pretty.c b/pretty.c
index 717ceed..8b533dc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -89,6 +89,7 @@ static void setup_commit_formats(void)
 	struct cmt_fmt_map builtin_formats[] = {
 		{ "raw",	CMIT_FMT_RAW,		0 },
 		{ "medium",	CMIT_FMT_MEDIUM,	0 },
+		{ "noexpand",	CMIT_FMT_NOEXPAND,	0 },
 		{ "short",	CMIT_FMT_SHORT,		0 },
 		{ "email",	CMIT_FMT_EMAIL,		0 },
 		{ "fuller",	CMIT_FMT_FULLER,	0 },
@@ -1685,11 +1686,16 @@ static void strbuf_add_tabexpand(struct strbuf *sb,
  * the whole line (without the final newline), after
  * de-tabifying.
  */
-static void pp_handle_indent(struct strbuf *sb, int indent,
+static void pp_handle_indent(struct pretty_print_context *pp,
+			     struct strbuf *sb,
+			     int indent,
 			     const char *line, int linelen)
 {
 	strbuf_addchars(sb, ' ', indent);
-	strbuf_add_tabexpand(sb, line, linelen);
+	if (pp->fmt == CMIT_FMT_MEDIUM)
+		strbuf_add_tabexpand(sb, line, linelen);
+	else
+		strbuf_add(sb, line, linelen);
 }
 
 void pp_remainder(struct pretty_print_context *pp,
@@ -1716,7 +1722,7 @@ void pp_remainder(struct pretty_print_context *pp,
 
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent)
-			pp_handle_indent(sb, indent, line, linelen);
+			pp_handle_indent(pp, sb, indent, line, linelen);
 		else
 			strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 987b708..34a9fed 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -93,7 +93,7 @@ test_expect_success 'output from user-defined format is re-wrapped' '
 	test_cmp expect log.predictable
 '
 
-test_expect_failure !MINGW 'shortlog wrapping' '
+test_expect_success !MINGW 'shortlog wrapping' '
 	cat >expect <<\EOF &&
 A U Thor (5):
       Test
@@ -114,8 +114,8 @@ EOF
 	test_cmp expect out
 '
 
-test_expect_failure !MINGW 'shortlog from non-git directory' '
-	git log HEAD >log &&
+test_expect_success !MINGW 'shortlog from non-git directory' '
+	git log --pretty=noexpand HEAD >log &&
 	GIT_DIR=non-existing git shortlog -w <log >out &&
 	test_cmp expect out
 '
-- 
2.8.0-rc3-175-g64dcf62
Linus Torvalds· Mar 17, 2016, 23:23 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/4] pretty-print: add --pretty=noexpand

On Thu, Mar 17, 2016 at 4:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> It is reasonable for tweak the default output mode for "git log" to
> untabify the commit log message, it sometimes may be necessary to
> see the output without tab expansion.
Thanks, these all look good to me.

Sorry for not following up, it's just that I'm in the middle of the kernel merge window and haven't had the time to worry about it.

            Linus
Junio C Hamano· Mar 17, 2016, 23:40 UTC · re: Linus Torvalds · lore

Re: [PATCH 4/4] pretty-print: add --pretty=noexpand

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 9 quoted lines
> On Thu, Mar 17, 2016 at 4:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> It is reasonable for tweak the default output mode for "git log" to
>> untabify the commit log message, it sometimes may be necessary to
>> see the output without tab expansion.
>
> Thanks, these all look good to me.
>
> Sorry for not following up, it's just that I'm in the middle of the
> kernel merge window and haven't had the time to worry about it.

Sorry is mutual; I would have done this much earlier if I didn't have the four-maintenance-tracks-at-the-same-time release today ;-)

Jeff King· Mar 18, 2016, 05:08 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/4] pretty-print: add --pretty=noexpand

On Thu, Mar 17, 2016 at 04:16:21PM -0700, Junio C Hamano wrote:
Show 6 quoted lines
> It is reasonable for tweak the default output mode for "git log" to
> untabify the commit log message, it sometimes may be necessary to
> see the output without tab expansion.
> 
> Invent a new --pretty option to do this.  Use this to unbreak the
> test breakages, where "git shortlog" and output are tested.

Hmm. Isn't "expand tabs" orthogonal to the rest of the pretty format? That is, couldn't one want "--pretty=fuller, but with tabs expanded"?

I don't personally care much myself, and certainly we don't need to support "--expand-tabs" for every format until somebody actually wants them enough to implement it. I just don't want to see us painted into a corner where we have to support an awkward interface forever (e.g., the way we had to retrofit the orthogonal "local" concept onto the --date code).

E.g., start with:
  - only CMIT_FMT_MEDIUM expands tabs (and does so by default)
  - passing --no-expand-tabs suppresses this behavior
  - passing --expand-tabs is an error for now; if people care later,
    they can add support for other formats (naively this is trivial, but
    I suspect there are some corner cases around things like
    --pretty=raw, so unless somebody wants to work on it now, I don't
    think we need to).
-Peff
Linus Torvalds· Mar 18, 2016, 05:36 UTC · re: Jeff King · lore

Re: [PATCH 4/4] pretty-print: add --pretty=noexpand

On Thu, Mar 17, 2016 at 10:08 PM, Jeff King <peff@peff.net> wrote:
>
> Hmm. Isn't "expand tabs" orthogonal to the rest of the pretty format?
> That is, couldn't one want "--pretty=fuller, but with tabs expanded"?

Yeah, you are right, one easily could. And in fact I end up doing "fuller" myself occasionally, because I check peoples commit timestamps (some people have a nasty habit of rebasing when they shouldn't).

So it's not just the medium format that would want detab by default, it's "full" and "fuller" too (but probably not "raw": that indents the message too, but the only real reason to use "raw" is for scripting).

So it would probably be better to make it a separate flag, and not tie it to a particular log format (and just make the log format set the default).

               Linus
Jeff King· Mar 18, 2016, 05:55 UTC · re: Linus Torvalds · lore

Re: [PATCH 4/4] pretty-print: add --pretty=noexpand

On Thu, Mar 17, 2016 at 10:36:16PM -0700, Linus Torvalds wrote:
Show 17 quoted lines
> On Thu, Mar 17, 2016 at 10:08 PM, Jeff King <peff@peff.net> wrote:
> >
> > Hmm. Isn't "expand tabs" orthogonal to the rest of the pretty format?
> > That is, couldn't one want "--pretty=fuller, but with tabs expanded"?
> 
> Yeah, you are right, one easily could. And in fact I end up doing
> "fuller" myself occasionally, because I check peoples commit
> timestamps (some people have a nasty habit of rebasing when they
> shouldn't).
> 
> So it's not just the medium format that would want detab by default,
> it's "full" and "fuller" too (but probably not "raw": that indents the
> message too, but the only real reason to use "raw" is for scripting).
> 
> So it would probably be better to make it a separate flag, and not tie
> it to a particular log format (and just make the log format set the
> default).

Yeah, I agree with all of that. I didn't want to force anybody to have to think too hard about corner cases they don't care about (again, as long as we don't paint ourselves into a corner) but I tend to think that it makes sense to apply it consistently to all of the stock human-readable formats (short, medium, full, fuller), but not to "raw" or "email", and probably not to user-formats.

-Peff
Junio C Hamano· Mar 18, 2016, 05:44 UTC · re: Jeff King · lore

Re: [PATCH 4/4] pretty-print: add --pretty=noexpand

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> E.g., start with:
>
>   - only CMIT_FMT_MEDIUM expands tabs (and does so by default)
>
>   - passing --no-expand-tabs suppresses this behavior
>
>   - passing --expand-tabs is an error for now; if people care later,
>     they can add support for other formats (naively this is trivial, but
>     I suspect there are some corner cases around things like
>     --pretty=raw, so unless somebody wants to work on it now, I don't
>     think we need to).

Yup, I like that better, but it is now past my work hours, so I won't be looking at it right now.

Junio C Hamano· Mar 23, 2016, 23:23 UTC · re: Junio C Hamano · lore

[PATCH v3 0/5] Expanding tabs in "git log" output

So here is the third try (previous round is found at $gmane/289166 and the very first one is at $gmane/288987).

The first three patches are essentially the same as v2. The last two updates how the tab-expansion is internally controlled:

 [4/5] adds a bit to pretty-commit-context that tells if tabs should
       be expanded.  Unlike v2 that tied this to pretty print format,
       this bit is orthogonal to the format, and theoretically it is
       possible to expand tabs even with --format=email.  Also,
       unlike v2, tabs are expanded not just in `medium` format, but
       also in `full` and `fuller` formats.
 [5/5] adds a new option --no-expand-tabs that controls the bit 4/5
       introduces, so that "git log [--pretty] --no-expand-tabs"
       would show the log message indented by 4 spaces, without tab
       expansion.

By the way, I have to say that I hate how pretty formatting and revision machinery interact with each other.

pretty.c::commit_formats ought to be the authoritative source of how each named format should work, but there are quite a many codepaths that just assign CMIT_FMT_SOMETHING to revs->commit_format without bothering with other fields in the cmt_fmt_map like is_tformat, and I am not sure if they are working correctly even before this patch.

Junio C Hamano (4):
  pretty-print: simplify the interaction between pp_handle_indent() and its caller
  pretty-print: further abstract out pp_handle_indent()
  pretty-print: limit expand-tabs to selected --pretty formats
  pretty-print: teach "--no-expand-tabs" option to "git log"
Linus Torvalds (1):
  pretty-print: de-tabify indented logs to make things line up properly
 Documentation/pretty-options.txt |  6 +++
 commit.h                         |  1 +
 log-tree.c                       |  1 +
 pretty.c                         | 88 ++++++++++++++++++++++++++++++++++++----
 revision.c                       |  3 ++
 revision.h                       |  1 +
 t/t4201-shortlog.sh              |  2 +-
 7 files changed, 92 insertions(+), 10 deletions(-)
[References]
http://thread.gmane.org/gmane.comp.version-control.git/288987/focus=289166
Interdiff since v2 is shown below.
Show changes to 8 files +25 −24

Documentation/pretty-formats.txt, Documentation/pretty-options.txt, commit.h, log-tree.c, pretty.c, revision.c, revision.h, t/t4201-shortlog.sh

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 173b932..671cebd 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -39,16 +39,6 @@ This is designed to be as compact as possible.
 
 	      <title line>
 
-	      <full commit message, tab-expanded>
-
-* 'noexpand'
-
-	  commit <sha1>
-	  Author: <author>
-	  Date:   <author date>
-
-	      <title line>
-
 	      <full commit message>
 
 * 'full'
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 7032b1a..069b927 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -3,7 +3,7 @@
 
 	Pretty-print the contents of the commit logs in a given format,
 	where '<format>' can be one of 'oneline', 'short', 'medium',
-	'full', 'fuller', 'email', 'raw', 'noexpand', 'format:<string>'
+	'full', 'fuller', 'email', 'raw', 'format:<string>'
 	and 'tformat:<string>'.  When '<format>' is none of the above,
 	and has '%placeholder' in it, it acts as if
 	'--pretty=tformat:<format>' were given.
@@ -42,6 +42,12 @@ people using 80-column terminals.
 	verbatim; this means that invalid sequences in the original
 	commit may be copied to the output.
 
+--no-expand-tabs::
+	The formats that indent the log message by 4 spaces
+	(i.e. 'medium', 'full', and 'fuller') by default show tabs
+	in the log message expanded.  This option disables the
+	expansion.
+
 ifndef::git-rev-list[]
 --notes[=<ref>]::
 	Show the notes (see linkgit:git-notes[1]) that annotate the
diff --git a/commit.h b/commit.h
index d511c61..a7ef682 100644
--- a/commit.h
+++ b/commit.h
@@ -126,7 +126,6 @@ enum cmit_fmt {
 	CMIT_FMT_RAW,
 	CMIT_FMT_MEDIUM,
 	CMIT_FMT_DEFAULT = CMIT_FMT_MEDIUM,
-	CMIT_FMT_NOEXPAND,
 	CMIT_FMT_SHORT,
 	CMIT_FMT_FULL,
 	CMIT_FMT_FULLER,
@@ -148,6 +147,7 @@ struct pretty_print_context {
 	int preserve_subject;
 	struct date_mode date_mode;
 	unsigned date_mode_explicit:1;
+	unsigned expand_tabs_in_log:1;
 	int need_8bit_cte;
 	char *notes_message;
 	struct reflog_walk_info *reflog_info;
diff --git a/log-tree.c b/log-tree.c
index 60f9839..78a5381 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
 	ctx.fmt = opt->commit_format;
 	ctx.mailmap = opt->mailmap;
 	ctx.color = opt->diffopt.use_color;
+	ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
 	ctx.output_encoding = get_log_output_encoding();
 	if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
 		ctx.from_ident = &opt->from_ident;
diff --git a/pretty.c b/pretty.c
index 8b533dc..5a33b7e 100644
--- a/pretty.c
+++ b/pretty.c
@@ -16,6 +16,7 @@ static struct cmt_fmt_map {
 	const char *name;
 	enum cmit_fmt format;
 	int is_tformat;
+	int expand_tabs_in_log;
 	int is_alias;
 	const char *user_format;
 } *commit_formats;
@@ -87,14 +88,13 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
 static void setup_commit_formats(void)
 {
 	struct cmt_fmt_map builtin_formats[] = {
-		{ "raw",	CMIT_FMT_RAW,		0 },
-		{ "medium",	CMIT_FMT_MEDIUM,	0 },
-		{ "noexpand",	CMIT_FMT_NOEXPAND,	0 },
-		{ "short",	CMIT_FMT_SHORT,		0 },
-		{ "email",	CMIT_FMT_EMAIL,		0 },
-		{ "fuller",	CMIT_FMT_FULLER,	0 },
-		{ "full",	CMIT_FMT_FULL,		0 },
-		{ "oneline",	CMIT_FMT_ONELINE,	1 }
+		{ "raw",	CMIT_FMT_RAW,		0,	0 },
+		{ "medium",	CMIT_FMT_MEDIUM,	0,	1 },
+		{ "short",	CMIT_FMT_SHORT,		0,	0 },
+		{ "email",	CMIT_FMT_EMAIL,		0,	0 },
+		{ "fuller",	CMIT_FMT_FULLER,	0,	1 },
+		{ "full",	CMIT_FMT_FULL,		0,	1 },
+		{ "oneline",	CMIT_FMT_ONELINE,	1,	0 }
 	};
 	commit_formats_len = ARRAY_SIZE(builtin_formats);
 	builtin_formats_len = commit_formats_len;
@@ -173,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 
 	rev->commit_format = commit_format->format;
 	rev->use_terminator = commit_format->is_tformat;
+	rev->expand_tabs_in_log = commit_format->expand_tabs_in_log;
 	if (commit_format->format == CMIT_FMT_USERFORMAT) {
 		save_user_format(rev, commit_format->user_format,
 				 commit_format->is_tformat);
@@ -1687,12 +1688,11 @@ static void strbuf_add_tabexpand(struct strbuf *sb,
  * de-tabifying.
  */
 static void pp_handle_indent(struct pretty_print_context *pp,
-			     struct strbuf *sb,
-			     int indent,
+			     struct strbuf *sb, int indent,
 			     const char *line, int linelen)
 {
 	strbuf_addchars(sb, ' ', indent);
-	if (pp->fmt == CMIT_FMT_MEDIUM)
+	if (pp->expand_tabs_in_log)
 		strbuf_add_tabexpand(sb, line, linelen);
 	else
 		strbuf_add(sb, line, linelen);
diff --git a/revision.c b/revision.c
index df56fce..b0d2a36 100644
--- a/revision.c
+++ b/revision.c
@@ -1412,6 +1412,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->skip_count = -1;
 	revs->max_count = -1;
 	revs->max_parents = -1;
+	revs->expand_tabs_in_log = 1;
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
@@ -1915,6 +1916,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(arg+9, revs);
+	} else if (!strcmp(arg, "--no-expand-tabs")) {
+		revs->expand_tabs_in_log = 0;
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
diff --git a/revision.h b/revision.h
index 23857c0..0bbfe0e 100644
--- a/revision.h
+++ b/revision.h
@@ -137,6 +137,7 @@ struct rev_info {
 			abbrev_commit_given:1,
 			zero_commit:1,
 			use_terminator:1,
+			expand_tabs_in_log:1,
 			missing_newline:1,
 			date_mode_explicit:1,
 			preserve_subject:1;
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 34a9fed..2fec948 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -115,7 +115,7 @@ EOF
 '
 
 test_expect_success !MINGW 'shortlog from non-git directory' '
-	git log --pretty=noexpand HEAD >log &&
+	git log --no-expand-tabs HEAD >log &&
 	GIT_DIR=non-existing git shortlog -w <log >out &&
 	test_cmp expect out
 '
Junio C Hamano· Mar 23, 2016, 23:23 UTC · re: Junio C Hamano · lore

[PATCH v3 1/5] pretty-print: de-tabify indented logs to make things line up properly

From: Linus Torvalds <torvalds@linux-foundation.org>

A commit log message sometimes tries to line things up using tabs, assuming fixed-width font with the standard 8-place tab settings. Viewing such a commit however does not work well in "git log", as we indent the lines by prefixing 4 spaces in front of them.

This should all line up:
  Column 1	Column 2
  --------	--------
  A		B
  ABCD		EFGH
  SPACES        Instead of Tabs
Even with multi-byte UTF8 characters:
  Column 1	Column 2
  --------	--------
  Ä		B
  åäö		100
  A Møøse	once bit my sister..

Tab-expand the lines in "git log --pretty=medium" output (which is the default), before prefixing 4 spaces.

This breaks a few tests in t4201, that tests "git shortlog".
 - One passes "git log" output to "git shortlog" to use the latter
   as a filter and does not expect the output of the former to be
   de-tabified.
 - The other expects that "git shortlog", when it reads the first
   line of the commit and produces the output itself, does not
   de-tabify it.
Mark them as expecting failure for now.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pretty.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 t/t4201-shortlog.sh |  4 +--
 2 files changed, 76 insertions(+), 4 deletions(-)
Show changes to 2 files +76 −4

pretty.c, t/t4201-shortlog.sh

diff --git a/pretty.c b/pretty.c
index 92b2870..0b40457 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1629,6 +1629,76 @@ void pp_title_line(struct pretty_print_context *pp,
 	strbuf_release(&title);
 }
 
+static int pp_utf8_width(const char *start, const char *end)
+{
+	int width = 0;
+	size_t remain = end - start;
+
+	while (remain) {
+		int n = utf8_width(&start, &remain);
+		if (n < 0 || !start)
+			return -1;
+		width += n;
+	}
+	return width;
+}
+
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * perhaps the whole line (without the final newline)
+ *
+ * Why "perhaps"? If there are tabs in the indented line
+ * it will print it out in order to de-tabify the line.
+ *
+ * But if there are no tabs, we just fall back on the
+ * normal "print the whole line".
+ */
+static int pp_handle_indent(struct strbuf *sb, int indent,
+			     const char *line, int linelen)
+{
+	const char *tab;
+
+	strbuf_addchars(sb, ' ', indent);
+
+	tab = memchr(line, '\t', linelen);
+	if (!tab)
+		return 0;
+
+	do {
+		int width = pp_utf8_width(line, tab);
+
+		/*
+		 * If it wasn't well-formed utf8, or it
+		 * had characters with badly defined
+		 * width (control characters etc), just
+		 * give up on trying to align things.
+		 */
+		if (width < 0)
+			break;
+
+		/* Output the data .. */
+		strbuf_add(sb, line, tab - line);
+
+		/* .. and the de-tabified tab */
+		strbuf_addchars(sb, ' ', 8-(width & 7));
+
+		/* Skip over the printed part .. */
+		linelen -= 1+tab-line;
+		line = tab + 1;
+
+		/* .. and look for the next tab */
+		tab = memchr(line, '\t', linelen);
+	} while (tab);
+
+	/*
+	 * Print out everything after the last tab without
+	 * worrying about width - there's nothing more to
+	 * align.
+	 */
+	strbuf_add(sb, line, linelen);
+	return 1;
+}
+
 void pp_remainder(struct pretty_print_context *pp,
 		  const char **msg_p,
 		  struct strbuf *sb,
@@ -1652,8 +1722,10 @@ void pp_remainder(struct pretty_print_context *pp,
 		first = 0;
 
 		strbuf_grow(sb, linelen + indent + 20);
-		if (indent)
-			strbuf_addchars(sb, ' ', indent);
+		if (indent) {
+			if (pp_handle_indent(sb, indent, line, linelen))
+				linelen = 0;
+		}
 		strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
 	}
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 7600a3e..987b708 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -93,7 +93,7 @@ test_expect_success 'output from user-defined format is re-wrapped' '
 	test_cmp expect log.predictable
 '
 
-test_expect_success !MINGW 'shortlog wrapping' '
+test_expect_failure !MINGW 'shortlog wrapping' '
 	cat >expect <<\EOF &&
 A U Thor (5):
       Test
@@ -114,7 +114,7 @@ EOF
 	test_cmp expect out
 '
 
-test_expect_success !MINGW 'shortlog from non-git directory' '
+test_expect_failure !MINGW 'shortlog from non-git directory' '
 	git log HEAD >log &&
 	GIT_DIR=non-existing git shortlog -w <log >out &&
 	test_cmp expect out
-- 
2.8.0-rc4-198-g3f6b64c
Junio C Hamano· Mar 23, 2016, 23:23 UTC · re: Junio C Hamano · lore

[PATCH v3 2/5] pretty-print: simplify the interaction between pp_handle_indent() and its caller

Instead of sometimes handling the output itself and some other times forcing the caller handle the output, make the helper function pp_handle_indent() responsible for the output for all cases.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pretty.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)
Show changes to pretty.c +9 −23
diff --git a/pretty.c b/pretty.c
index 0b40457..6d657fc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1645,26 +1645,17 @@ static int pp_utf8_width(const char *start, const char *end)
 
 /*
  * pp_handle_indent() prints out the intendation, and
- * perhaps the whole line (without the final newline)
- *
- * Why "perhaps"? If there are tabs in the indented line
- * it will print it out in order to de-tabify the line.
- *
- * But if there are no tabs, we just fall back on the
- * normal "print the whole line".
+ * the whole line (without the final newline), after
+ * de-tabifying.
  */
-static int pp_handle_indent(struct strbuf *sb, int indent,
+static void pp_handle_indent(struct strbuf *sb, int indent,
 			     const char *line, int linelen)
 {
 	const char *tab;
 
 	strbuf_addchars(sb, ' ', indent);
 
-	tab = memchr(line, '\t', linelen);
-	if (!tab)
-		return 0;
-
-	do {
+	while ((tab = memchr(line, '\t', linelen)) != NULL) {
 		int width = pp_utf8_width(line, tab);
 
 		/*
@@ -1685,10 +1676,7 @@ static int pp_handle_indent(struct strbuf *sb, int indent,
 		/* Skip over the printed part .. */
 		linelen -= 1+tab-line;
 		line = tab + 1;
-
-		/* .. and look for the next tab */
-		tab = memchr(line, '\t', linelen);
-	} while (tab);
+	}
 
 	/*
 	 * Print out everything after the last tab without
@@ -1696,7 +1684,6 @@ static int pp_handle_indent(struct strbuf *sb, int indent,
 	 * align.
 	 */
 	strbuf_add(sb, line, linelen);
-	return 1;
 }
 
 void pp_remainder(struct pretty_print_context *pp,
@@ -1722,11 +1709,10 @@ void pp_remainder(struct pretty_print_context *pp,
 		first = 0;
 
 		strbuf_grow(sb, linelen + indent + 20);
-		if (indent) {
-			if (pp_handle_indent(sb, indent, line, linelen))
-				linelen = 0;
-		}
-		strbuf_add(sb, line, linelen);
+		if (indent)
+			pp_handle_indent(sb, indent, line, linelen);
+		else
+			strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
 	}
 }
-- 
2.8.0-rc4-198-g3f6b64c
Junio C Hamano· Mar 23, 2016, 23:23 UTC · re: Junio C Hamano · lore

[PATCH v3 3/5] pretty-print: further abstract out pp_handle_indent()

Separate the call to add 4-space indent, and a new helper to add a line after de-tabifying.

The new helper function strbuf_add_tabexpand() could later be moved to strbuf.[ch] if other callers need to.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pretty.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)
Show changes to pretty.c +15 −9
diff --git a/pretty.c b/pretty.c
index 6d657fc..717ceed 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1643,18 +1643,12 @@ static int pp_utf8_width(const char *start, const char *end)
 	return width;
 }
 
-/*
- * pp_handle_indent() prints out the intendation, and
- * the whole line (without the final newline), after
- * de-tabifying.
- */
-static void pp_handle_indent(struct strbuf *sb, int indent,
-			     const char *line, int linelen)
+
+static void strbuf_add_tabexpand(struct strbuf *sb,
+				 const char *line, int linelen)
 {
 	const char *tab;
 
-	strbuf_addchars(sb, ' ', indent);
-
 	while ((tab = memchr(line, '\t', linelen)) != NULL) {
 		int width = pp_utf8_width(line, tab);
 
@@ -1686,6 +1680,18 @@ static void pp_handle_indent(struct strbuf *sb, int indent,
 	strbuf_add(sb, line, linelen);
 }
 
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * the whole line (without the final newline), after
+ * de-tabifying.
+ */
+static void pp_handle_indent(struct strbuf *sb, int indent,
+			     const char *line, int linelen)
+{
+	strbuf_addchars(sb, ' ', indent);
+	strbuf_add_tabexpand(sb, line, linelen);
+}
+
 void pp_remainder(struct pretty_print_context *pp,
 		  const char **msg_p,
 		  struct strbuf *sb,
-- 
2.8.0-rc4-198-g3f6b64c
Junio C Hamano· Mar 23, 2016, 23:23 UTC · re: Junio C Hamano · lore

[PATCH v3 4/5] pretty-print: limit expand-tabs to selected --pretty formats

Make sure that "git log" (by default, it uses --pretty=medium) and "git log --pretty={full,fuller}" are the only ones that trigger the new "expand tabs in the log message" behaviour.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.h            |  1 +
 log-tree.c          |  1 +
 pretty.c            | 26 ++++++++++++++++----------
 revision.c          |  1 +
 revision.h          |  1 +
 t/t4201-shortlog.sh |  2 +-
 6 files changed, 21 insertions(+), 11 deletions(-)
Show changes to 6 files +21 −11

commit.h, log-tree.c, pretty.c, revision.c, revision.h, t/t4201-shortlog.sh

diff --git a/commit.h b/commit.h
index 5d58be0..a7ef682 100644
--- a/commit.h
+++ b/commit.h
@@ -147,6 +147,7 @@ struct pretty_print_context {
 	int preserve_subject;
 	struct date_mode date_mode;
 	unsigned date_mode_explicit:1;
+	unsigned expand_tabs_in_log:1;
 	int need_8bit_cte;
 	char *notes_message;
 	struct reflog_walk_info *reflog_info;
diff --git a/log-tree.c b/log-tree.c
index 60f9839..78a5381 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
 	ctx.fmt = opt->commit_format;
 	ctx.mailmap = opt->mailmap;
 	ctx.color = opt->diffopt.use_color;
+	ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
 	ctx.output_encoding = get_log_output_encoding();
 	if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
 		ctx.from_ident = &opt->from_ident;
diff --git a/pretty.c b/pretty.c
index 717ceed..5a33b7e 100644
--- a/pretty.c
+++ b/pretty.c
@@ -16,6 +16,7 @@ static struct cmt_fmt_map {
 	const char *name;
 	enum cmit_fmt format;
 	int is_tformat;
+	int expand_tabs_in_log;
 	int is_alias;
 	const char *user_format;
 } *commit_formats;
@@ -87,13 +88,13 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
 static void setup_commit_formats(void)
 {
 	struct cmt_fmt_map builtin_formats[] = {
-		{ "raw",	CMIT_FMT_RAW,		0 },
-		{ "medium",	CMIT_FMT_MEDIUM,	0 },
-		{ "short",	CMIT_FMT_SHORT,		0 },
-		{ "email",	CMIT_FMT_EMAIL,		0 },
-		{ "fuller",	CMIT_FMT_FULLER,	0 },
-		{ "full",	CMIT_FMT_FULL,		0 },
-		{ "oneline",	CMIT_FMT_ONELINE,	1 }
+		{ "raw",	CMIT_FMT_RAW,		0,	0 },
+		{ "medium",	CMIT_FMT_MEDIUM,	0,	1 },
+		{ "short",	CMIT_FMT_SHORT,		0,	0 },
+		{ "email",	CMIT_FMT_EMAIL,		0,	0 },
+		{ "fuller",	CMIT_FMT_FULLER,	0,	1 },
+		{ "full",	CMIT_FMT_FULL,		0,	1 },
+		{ "oneline",	CMIT_FMT_ONELINE,	1,	0 }
 	};
 	commit_formats_len = ARRAY_SIZE(builtin_formats);
 	builtin_formats_len = commit_formats_len;
@@ -172,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 
 	rev->commit_format = commit_format->format;
 	rev->use_terminator = commit_format->is_tformat;
+	rev->expand_tabs_in_log = commit_format->expand_tabs_in_log;
 	if (commit_format->format == CMIT_FMT_USERFORMAT) {
 		save_user_format(rev, commit_format->user_format,
 				 commit_format->is_tformat);
@@ -1685,11 +1687,15 @@ static void strbuf_add_tabexpand(struct strbuf *sb,
  * the whole line (without the final newline), after
  * de-tabifying.
  */
-static void pp_handle_indent(struct strbuf *sb, int indent,
+static void pp_handle_indent(struct pretty_print_context *pp,
+			     struct strbuf *sb, int indent,
 			     const char *line, int linelen)
 {
 	strbuf_addchars(sb, ' ', indent);
-	strbuf_add_tabexpand(sb, line, linelen);
+	if (pp->expand_tabs_in_log)
+		strbuf_add_tabexpand(sb, line, linelen);
+	else
+		strbuf_add(sb, line, linelen);
 }
 
 void pp_remainder(struct pretty_print_context *pp,
@@ -1716,7 +1722,7 @@ void pp_remainder(struct pretty_print_context *pp,
 
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent)
-			pp_handle_indent(sb, indent, line, linelen);
+			pp_handle_indent(pp, sb, indent, line, linelen);
 		else
 			strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
diff --git a/revision.c b/revision.c
index df56fce..8827d9f 100644
--- a/revision.c
+++ b/revision.c
@@ -1412,6 +1412,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->skip_count = -1;
 	revs->max_count = -1;
 	revs->max_parents = -1;
+	revs->expand_tabs_in_log = 1;
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
diff --git a/revision.h b/revision.h
index 23857c0..0bbfe0e 100644
--- a/revision.h
+++ b/revision.h
@@ -137,6 +137,7 @@ struct rev_info {
 			abbrev_commit_given:1,
 			zero_commit:1,
 			use_terminator:1,
+			expand_tabs_in_log:1,
 			missing_newline:1,
 			date_mode_explicit:1,
 			preserve_subject:1;
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 987b708..d1e8259 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -93,7 +93,7 @@ test_expect_success 'output from user-defined format is re-wrapped' '
 	test_cmp expect log.predictable
 '
 
-test_expect_failure !MINGW 'shortlog wrapping' '
+test_expect_success !MINGW 'shortlog wrapping' '
 	cat >expect <<\EOF &&
 A U Thor (5):
       Test
-- 
2.8.0-rc4-198-g3f6b64c
Junio C Hamano· Mar 23, 2016, 23:23 UTC · re: Junio C Hamano · lore

[PATCH v3 5/5] pretty-print: teach "--no-expand-tabs" option to "git log"

The output formats of "git log" that indent the log message by 4 spaces have been updated to expand tabs by default in previous steps, without a way to restore the original behaviour.

Introduce a new "--no-expand-tabs" option to allow this.
As the effect of options is cumulative,
    $ git log [--pretty=medium] --no-expand-tabs
would not expand, while this invocation
    $ git log --no-expand-tabs --pretty[=medium]

by virtue of having --pretty later on the command line, expands tabs again.

We _could_ introduce --expand-tabs option as well, to allow
    $ git log --pretty=email --expand-tabs

but we don't bother, as the output format that do not expand tabs by default are mostly meant to transfer the contents as literally as possible.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
---
 Documentation/pretty-options.txt | 6 ++++++
 revision.c                       | 2 ++
 t/t4201-shortlog.sh              | 4 ++--
 3 files changed, 10 insertions(+), 2 deletions(-)
Show changes to 3 files +10 −2

Documentation/pretty-options.txt, revision.c, t/t4201-shortlog.sh

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 4b659ac..069b927 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -42,6 +42,12 @@ people using 80-column terminals.
 	verbatim; this means that invalid sequences in the original
 	commit may be copied to the output.
 
+--no-expand-tabs::
+	The formats that indent the log message by 4 spaces
+	(i.e. 'medium', 'full', and 'fuller') by default show tabs
+	in the log message expanded.  This option disables the
+	expansion.
+
 ifndef::git-rev-list[]
 --notes[=<ref>]::
 	Show the notes (see linkgit:git-notes[1]) that annotate the
diff --git a/revision.c b/revision.c
index 8827d9f..b0d2a36 100644
--- a/revision.c
+++ b/revision.c
@@ -1916,6 +1916,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(arg+9, revs);
+	} else if (!strcmp(arg, "--no-expand-tabs")) {
+		revs->expand_tabs_in_log = 0;
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index d1e8259..2fec948 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -114,8 +114,8 @@ EOF
 	test_cmp expect out
 '
 
-test_expect_failure !MINGW 'shortlog from non-git directory' '
-	git log HEAD >log &&
+test_expect_success !MINGW 'shortlog from non-git directory' '
+	git log --no-expand-tabs HEAD >log &&
 	GIT_DIR=non-existing git shortlog -w <log >out &&
 	test_cmp expect out
 '
-- 
2.8.0-rc4-198-g3f6b64c
Linus Torvalds· Mar 23, 2016, 23:47 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

On Wed, Mar 23, 2016 at 4:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> So here is the third try (previous round is found at $gmane/289166
> and the very first one is at $gmane/288987).
>
> The first three patches are essentially the same as v2.  The last
> two updates how the tab-expansion is internally controlled:

I tested this (as it in in 'pu', rather than applying the patches), and it all seems to work fine. So Ack.

And I agree that it would be good if all the commit printout logic was unified rather than having some ad-hoc "let's just set the format", but I think that's a separate cleanup.

It might be more regular to have that "--expand-tabs" flag too (which would then work with the email and raw formats), but I don't see any actual real use for it so it really doesn't matter.

          Linus
Jeff King· Mar 24, 2016, 00:58 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

On Wed, Mar 23, 2016 at 04:23:41PM -0700, Junio C Hamano wrote:
> So here is the third try (previous round is found at $gmane/289166
> and the very first one is at $gmane/288987).

Is the plan to merge these as-is? The ordering is a bit funny (introduce breakage, then repair it), and I think the first patch still breaks t4201.8 (which is then repaired in the fourth one).

I think it would be a lot easier to review as:
  1. Factor out pp_handle_indent(), and any other preparation.
  2. Add --expand-tabs / --no-expand-tabs, with the logic going into
     pp_handle_indent().
  3. Flip the default for some formats to expand-tabs.

Other than that, the end result seems OK to me (I think adding --expand-tabs would be nice, but I suspect it may need to be marked as incompatible with some formats; do all formats end up in this same writing code path?).

Show 8 quoted lines
> By the way, I have to say that I hate how pretty formatting and
> revision machinery interact with each other.
> 
> pretty.c::commit_formats ought to be the authoritative source of how
> each named format should work, but there are quite a many codepaths
> that just assign CMIT_FMT_SOMETHING to revs->commit_format without
> bothering with other fields in the cmt_fmt_map like is_tformat, and
> I am not sure if they are working correctly even before this patch.

I don't disagree with any of that. I suspect some of the logic may be complicated for sticking in a table, though. Perhaps we need a:

  void set_pp_format(struct pretty_print_context *ctx, enum cmit_fmt fmt);

that sets up the whole struct based on the given format, and then the logic can live in C code. I haven't looked closely at that code in a while, though, so maybe that is overkill.

-Peff
Junio C Hamano· Mar 24, 2016, 05:17 UTC · re: Jeff King · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> On Wed, Mar 23, 2016 at 04:23:41PM -0700, Junio C Hamano wrote:
>
>> So here is the third try (previous round is found at $gmane/289166
>> and the very first one is at $gmane/288987).
>
> Is the plan to merge these as-is? The ordering is a bit funny (introduce
> breakage, then repair it), and I think the first patch still breaks
> t4201.8 (which is then repaired in the fourth one).

I do not have a firm plan yet. This was one of those "during the pre-release freeze, instead of reviewing shiny new toys by others too early, spend leftover time to tie loose ends" attempts ;-)

I'd agree that the "final" version should do our usual "progressive improvement, never stepping back one and then forward two", but I wanted to see what the endgame state would look like first, and by doing the incremental "the first one gets it 80% right, and fix it up with follow-up patches" I didn't have to worry about at what point I need to take the authorship of which patch.

Torsten Bögershausen· Mar 24, 2016, 07:05 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

>  [5/5] adds a new option --no-expand-tabs that controls the bit 4/5
>        introduces, so that "git log [--pretty] --no-expand-tabs"
>        would show the log message indented by 4 spaces, without tab
>        expansion.
Does this introduce an unnecessary regression out of the sudden ?

Would it make sense to have git log --tab-size=8 (or similar)

and add a config variable like git config ui.logtabsize which is 0 by default to get the old handling and 8 for the new one ?

Junio C Hamano· Mar 24, 2016, 15:37 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

Torsten Bögershausen <tboegi@web.de> writes:
Show 6 quoted lines
>>  [5/5] adds a new option --no-expand-tabs that controls the bit 4/5
>>        introduces, so that "git log [--pretty] --no-expand-tabs"
>>        would show the log message indented by 4 spaces, without tab
>>        expansion.
>
> Does this introduce an unnecessary regression out of the sudden ?

As I see this as a pure UI-level improvement done to a few of our Porcelain program, I would say there is no regression here.

Junio C Hamano· Mar 24, 2016, 18:22 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

Torsten Bögershausen <tboegi@web.de> writes:
Show 7 quoted lines
> Would it make sense to have
> git log --tab-size=8
> (or similar)
>
> and add a config variable like
> git config ui.logtabsize
> which is 0 by default to get the old handling and 8 for the new one ?

That may be a good approach (I agree with you that --tab-size is not the best name). Want to try it as a replacement?

Torsten Bögershausen· Mar 25, 2016, 09:34 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

On 2016-03-24 19.22, Junio C Hamano wrote:
Show 12 quoted lines
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> Would it make sense to have
>> git log --tab-size=8
>> (or similar)
>>
>> and add a config variable like
>> git config ui.logtabsize
>> which is 0 by default to get the old handling and 8 for the new one ?
> 
> That may be a good approach (I agree with you that --tab-size is not
> the best name).  Want to try it as a replacement?

May be log.tabwidth ? I'm happy to help out here, if we can cook the feature in pu for 3-4 weeks or so ?

Torsten Bögershausen· Mar 25, 2016, 14:13 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

This is copy-paste replacement for the last commit. (Most probably it is white space damaged) I'm not sure, is it's worth it ? If yes, I can send a proper patch later.

git show HEAD
commit 3ac551127d51cd59b24f49729d9ce4dd011a09a1
Author: Junio C Hamano <gitster@pobox.com>
Date:   Wed Mar 23 15:57:42 2016 -0700
    pretty-print: Add the config variable log.tabwidth
    The output formats of "git log" that indent the log message by 4
    spaces have been updated to expand tabs by default in previous
    steps, without a way to restore the original behaviour.
    Introduce a config variable log.tabwidth to allow this.
        $ git -c log.tabwidth=0 log [--pretty=medium]
    would not expand.
    The non-expansion can be made permanent:
        $ git config log.tabwidth 0
    Or the TAB width can be changed like this:
        $ git config log.tabwidth 4
Show changes to 6 files +15 −4

Documentation/config.txt, cache.h, config.c, environment.c, pretty.c, t/t4201-shortlog.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..611f5e4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1915,6 +1915,10 @@ log.showRoot::
 	Tools like linkgit:git-log[1] or linkgit:git-whatchanged[1], which
 	normally hide the root commit will now show it. True by default.

+log.tabWidth::
+	Sets the width of a TAB.  If 0, no TAB expansion is done.
+	8 by default.
+
 log.mailmap::
 	If true, makes linkgit:git-log[1], linkgit:git-show[1], and
 	linkgit:git-whatchanged[1] assume `--use-mailmap`.
diff --git a/cache.h b/cache.h
index b829410..fd115d2 100644
--- a/cache.h
+++ b/cache.h
@@ -649,6 +649,7 @@ extern int ignore_case;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
+extern unsigned log_tab_width;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
 extern int shared_repository;
diff --git a/config.c b/config.c
index 9ba40bc..e6aadfe 100644
--- a/config.c
+++ b/config.c
@@ -1030,6 +1030,11 @@ int git_default_config(const char *var, const char
*value, void *dummy)
 		pack_size_limit_cfg = git_config_ulong(var, value);
 		return 0;
 	}
+
+	if (!strcmp(var, "log.tabwidth")) {
+		log_tab_width = (unsigned)git_config_ulong(var, value);
+		return 0;
+	}
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/environment.c b/environment.c
index 6dec9d0..3c72b44 100644
--- a/environment.c
+++ b/environment.c
@@ -21,6 +21,7 @@ int ignore_case;
 int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
+unsigned log_tab_width = 8;
 int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
diff --git a/pretty.c b/pretty.c
index 5a33b7e..1d92c55 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1667,7 +1667,7 @@ static void strbuf_add_tabexpand(struct strbuf *sb,
 		strbuf_add(sb, line, tab - line);

 		/* .. and the de-tabified tab */
-		strbuf_addchars(sb, ' ', 8-(width & 7));
+		strbuf_addchars(sb, ' ', log_tab_width - (width % log_tab_width));

 		/* Skip over the printed part .. */
 		linelen -= 1+tab-line;
@@ -1692,7 +1692,7 @@ static void pp_handle_indent(struct pretty_print_context *pp,
 			     const char *line, int linelen)
 {
 	strbuf_addchars(sb, ' ', indent);
-	if (pp->expand_tabs_in_log)
+	if (pp->expand_tabs_in_log && log_tab_width)
 		strbuf_add_tabexpand(sb, line, linelen);
 	else
 		strbuf_add(sb, line, linelen);
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 96233ca..9235a2e 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -114,8 +114,8 @@ EOF
 	test_cmp expect out
 '

-test_expect_failure !MINGW 'shortlog from non-git directory' '
-	git log HEAD >log &&
+test_expect_success !MINGW 'shortlog from non-git directory' '
+	git -c log.tabwidth=0 log HEAD >log &&
 	GIT_DIR=non-existing git shortlog -w <log >out &&
 	test_cmp expect out
 '
Junio C Hamano· Mar 25, 2016, 16:41 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

Torsten Bögershausen <tboegi@web.de> writes:
> This is copy-paste replacement for the last commit.
> (Most probably it is white space damaged)
> I'm not sure, is it's worth it ?
Not if you are keeping "expand_tabs_in_log" boolean field.

I was expecting that the new "log-tab-width" thing extends the expand_tabs_in_log as the concept--it used to be a boolean "do we or do we not expand?" to "set it to 0 if we do not want to expand, set it to N if we do want to expand to every N display spaces". In other words, if you introduce this new thing, the boolean should not e necessary and it should go. Did I misread your earlier message that described your idea?

> +log.tabWidth::
> +	Sets the width of a TAB.  If 0, no TAB expansion is done.
> +	8 by default.

You need to make it clear where tabs are expanded. The readers would wonder if it expands tabs in "log -p" patch output, etc.

A related tangent. I suspect
	git format-patch --expand-tabs-in-log-message=4

might be a good feature to help people whose editors are configured to move to next-multiple-of-4 column with a tab, and applying their patch would show unaligned lines in "git log" output for others, by expanding their tabs when sending the patch out. We might even want to add a related option

	git am --unexpand-tabs

that collapses a run of SP that fills to next-multiple-of-8 into a tab on the receiving end.

Junio C Hamano· Mar 25, 2016, 16:25 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v3 0/5] Expanding tabs in "git log" output

Torsten Bögershausen <tboegi@web.de> writes:
Show 15 quoted lines
> On 2016-03-24 19.22, Junio C Hamano wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>> 
>>> Would it make sense to have
>>> git log --tab-size=8
>>> (or similar)
>>>
>>> and add a config variable like
>>> git config ui.logtabsize
>>> which is 0 by default to get the old handling and 8 for the new one ?
>> 
>> That may be a good approach (I agree with you that --tab-size is not
>> the best name).  Want to try it as a replacement?
>
> May be log.tabwidth ?

The reason why I thought "--tab-size=4" is not the best name is because it only states that the user prefers a non-standard tab stops that are every 4 display spaces, and does not say what we with do that information, i.e. it does not hint "We expand tabs to spaces according to this setting" in its name.

I think log.tabwidth shares the same characteristics.
Junio C Hamano· Mar 29, 2016, 23:15 UTC · re: Junio C Hamano · lore

[PATCH v4 0/3] Expanding tabs in "git log" output

So here is the fourth try. Previous round are at $gmane/289694, $gmane/289166, and $gmane/288987.

I didn't quite find a good order to build this progressively, so this series:

 - First adds the internal machinery and explicit --expand-tabs.
   This keeps Linus's authorship, but is different in that it is not
   enabled by default;
 - Then enable --expand-tabs by default for selected pretty formats;
 - And optionally, allow custom tab-width to be used.
Junio C Hamano (2):
  pretty: enable --expand-tabs by default for selected pretty formats
  pretty: allow tweaking tabwidth in --expand-tabs
Linus Torvalds (1):
  pretty: expand tabs in indented logs to make things line up properly
 Documentation/pretty-options.txt | 14 +++++++
 commit.h                         |  1 +
 log-tree.c                       |  1 +
 pretty.c                         | 87 +++++++++++++++++++++++++++++++++++-----
 revision.c                       | 10 +++++
 revision.h                       |  2 +-
 t/t4201-shortlog.sh              |  2 +-
 7 files changed, 106 insertions(+), 11 deletions(-)
-- 
2.8.0-215-gd29a7d9
Junio C Hamano· Mar 29, 2016, 23:15 UTC · re: Junio C Hamano · lore

[PATCH v4 1/3] pretty: expand tabs in indented logs to make things line up properly

From: Linus Torvalds <torvalds@linux-foundation.org>

A commit log message sometimes tries to line things up using tabs, assuming fixed-width font with the standard 8-place tab settings. Viewing such a commit however does not work well in "git log", as we indent the lines by prefixing 4 spaces in front of them.

This should all line up:
  Column 1	Column 2
  --------	--------
  A		B
  ABCD		EFGH
  SPACES        Instead of Tabs
Even with multi-byte UTF8 characters:
  Column 1	Column 2
  --------	--------
  Ä		B
  åäö		100
  A Møøse	once bit my sister..

Tab-expand the lines in "git log --expand-tabs" output before prefixing 4 spaces.

This is based on the patch by Linus Torvalds, but changed to require an explicit command line option to enable the behaviour.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/pretty-options.txt |  6 ++++
 commit.h                         |  1 +
 log-tree.c                       |  1 +
 pretty.c                         | 71 ++++++++++++++++++++++++++++++++++++++--
 revision.c                       |  2 ++
 revision.h                       |  1 +
 6 files changed, 80 insertions(+), 2 deletions(-)
Show changes to 6 files +80 −2

Documentation/pretty-options.txt, commit.h, log-tree.c, pretty.c, revision.c, revision.h

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 4b659ac..4fb5c76 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -42,6 +42,12 @@ people using 80-column terminals.
 	verbatim; this means that invalid sequences in the original
 	commit may be copied to the output.
 
+--expand-tabs::
+	Perform a tab expansion (replace each tab with enough number
+	of spaces to fill to the next display column that is
+	multiple of 8) in the log message before using the message
+	to show in the output.
+
 ifndef::git-rev-list[]
 --notes[=<ref>]::
 	Show the notes (see linkgit:git-notes[1]) that annotate the
diff --git a/commit.h b/commit.h
index 5d58be0..a7ef682 100644
--- a/commit.h
+++ b/commit.h
@@ -147,6 +147,7 @@ struct pretty_print_context {
 	int preserve_subject;
 	struct date_mode date_mode;
 	unsigned date_mode_explicit:1;
+	unsigned expand_tabs_in_log:1;
 	int need_8bit_cte;
 	char *notes_message;
 	struct reflog_walk_info *reflog_info;
diff --git a/log-tree.c b/log-tree.c
index 60f9839..78a5381 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
 	ctx.fmt = opt->commit_format;
 	ctx.mailmap = opt->mailmap;
 	ctx.color = opt->diffopt.use_color;
+	ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
 	ctx.output_encoding = get_log_output_encoding();
 	if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
 		ctx.from_ident = &opt->from_ident;
diff --git a/pretty.c b/pretty.c
index 92b2870..c8b075d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1629,6 +1629,72 @@ void pp_title_line(struct pretty_print_context *pp,
 	strbuf_release(&title);
 }
 
+static int pp_utf8_width(const char *start, const char *end)
+{
+	int width = 0;
+	size_t remain = end - start;
+
+	while (remain) {
+		int n = utf8_width(&start, &remain);
+		if (n < 0 || !start)
+			return -1;
+		width += n;
+	}
+	return width;
+}
+
+static void strbuf_add_tabexpand(struct strbuf *sb,
+				 const char *line, int linelen)
+{
+	const char *tab;
+
+	while ((tab = memchr(line, '\t', linelen)) != NULL) {
+		int width = pp_utf8_width(line, tab);
+
+		/*
+		 * If it wasn't well-formed utf8, or it
+		 * had characters with badly defined
+		 * width (control characters etc), just
+		 * give up on trying to align things.
+		 */
+		if (width < 0)
+			break;
+
+		/* Output the data .. */
+		strbuf_add(sb, line, tab - line);
+
+		/* .. and the de-tabified tab */
+		strbuf_addchars(sb, ' ', 8 - (width % 8));
+
+		/* Skip over the printed part .. */
+		linelen -= tab + 1 - line;
+		line = tab + 1;
+	}
+
+	/*
+	 * Print out everything after the last tab without
+	 * worrying about width - there's nothing more to
+	 * align.
+	 */
+	strbuf_add(sb, line, linelen);
+}
+
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * the whole line (without the final newline), after
+ * de-tabifying.
+ */
+static void pp_handle_indent(struct pretty_print_context *pp,
+			     struct strbuf *sb, int indent,
+			     const char *line, int linelen)
+{
+	strbuf_addchars(sb, ' ', indent);
+	if (pp->expand_tabs_in_log)
+		strbuf_add_tabexpand(sb, line, linelen);
+	else
+		strbuf_add(sb, line, linelen);
+}
+
 void pp_remainder(struct pretty_print_context *pp,
 		  const char **msg_p,
 		  struct strbuf *sb,
@@ -1653,8 +1719,9 @@ void pp_remainder(struct pretty_print_context *pp,
 
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent)
-			strbuf_addchars(sb, ' ', indent);
-		strbuf_add(sb, line, linelen);
+			pp_handle_indent(pp, sb, indent, line, linelen);
+		else
+			strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
 	}
 }
diff --git a/revision.c b/revision.c
index df56fce..e662230 100644
--- a/revision.c
+++ b/revision.c
@@ -1915,6 +1915,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(arg+9, revs);
+	} else if (!strcmp(arg, "--expand-tabs")) {
+		revs->expand_tabs_in_log = 1;
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
diff --git a/revision.h b/revision.h
index 23857c0..4079753 100644
--- a/revision.h
+++ b/revision.h
@@ -133,6 +133,7 @@ struct rev_info {
 			show_notes_given:1,
 			show_signature:1,
 			pretty_given:1,
+			expand_tabs_in_log:1,
 			abbrev_commit:1,
 			abbrev_commit_given:1,
 			zero_commit:1,
-- 
2.8.0-215-gd29a7d9
Eric Sunshine· Mar 30, 2016, 00:17 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 1/3] pretty: expand tabs in indented logs to make things line up properly

On Tue, Mar 29, 2016 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> From: Linus Torvalds <torvalds@linux-foundation.org>
>
> A commit log message sometimes tries to line things up using tabs,
> assuming fixed-width font with the standard 8-place tab settings.
> Viewing such a commit however does not work well in "git log", as
> we indent the lines by prefixing 4 spaces in front of them.
> [...]
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
> @@ -42,6 +42,12 @@ people using 80-column terminals.
> +--expand-tabs::
> +       Perform a tab expansion (replace each tab with enough number
> +       of spaces to fill to the next display column that is
Nit: "enough spaces" or "a sufficient number of spaces".
> +       multiple of 8) in the log message before using the message
> +       to show in the output.
Junio C Hamano· Mar 30, 2016, 18:20 UTC · re: Eric Sunshine · lore

Re: [PATCH v4 1/3] pretty: expand tabs in indented logs to make things line up properly

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 18 quoted lines
> On Tue, Mar 29, 2016 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> From: Linus Torvalds <torvalds@linux-foundation.org>
>>
>> A commit log message sometimes tries to line things up using tabs,
>> assuming fixed-width font with the standard 8-place tab settings.
>> Viewing such a commit however does not work well in "git log", as
>> we indent the lines by prefixing 4 spaces in front of them.
>> [...]
>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
>> @@ -42,6 +42,12 @@ people using 80-column terminals.
>> +--expand-tabs::
>> +       Perform a tab expansion (replace each tab with enough number
>> +       of spaces to fill to the next display column that is
>
> Nit: "enough spaces" or "a sufficient number of spaces".
Thanks, will be part of a reroll (when it happens ;-).
Junio C Hamano· Mar 29, 2016, 23:15 UTC · re: Junio C Hamano · lore

[PATCH v4 2/3] pretty: enable --expand-tabs by default for selected pretty formats

"git log --pretty={medium,full,fuller}" and "git log" by default prepend 4 spaces to the log message, so it makes sense to enable the new "expand-tabs" facility by default for these formats. Add --no-expand-tabs option to override the new default.

The change alone breaks a test in t4201 that runs "git shortlog" on the output from "git log", and expects that the output from "git log" does not do such a tab expansion. Adjust the test to explicitly disable expand-tabs with --no-expand-tabs.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/pretty-options.txt |  6 ++++++
 pretty.c                         | 16 +++++++++-------
 revision.c                       |  3 +++
 t/t4201-shortlog.sh              |  2 +-
 4 files changed, 19 insertions(+), 8 deletions(-)
Show changes to 4 files +19 −8

Documentation/pretty-options.txt, pretty.c, revision.c, t/t4201-shortlog.sh

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 4fb5c76..23967b6 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -43,10 +43,16 @@ people using 80-column terminals.
 	commit may be copied to the output.
 
 --expand-tabs::
+--no-expand-tabs::
 	Perform a tab expansion (replace each tab with enough number
 	of spaces to fill to the next display column that is
 	multiple of 8) in the log message before using the message
 	to show in the output.
++
+By default, tabs are expanded in pretty formats that indent the log
+message by 4 spaces (i.e.  'medium', which is the default, 'full',
+and "fuller').  `--no-expand-tabs` option can be used to disable
+this.
 
 ifndef::git-rev-list[]
 --notes[=<ref>]::
diff --git a/pretty.c b/pretty.c
index c8b075d..de22a8c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -16,6 +16,7 @@ static struct cmt_fmt_map {
 	const char *name;
 	enum cmit_fmt format;
 	int is_tformat;
+	int expand_tabs_in_log;
 	int is_alias;
 	const char *user_format;
 } *commit_formats;
@@ -87,13 +88,13 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
 static void setup_commit_formats(void)
 {
 	struct cmt_fmt_map builtin_formats[] = {
-		{ "raw",	CMIT_FMT_RAW,		0 },
-		{ "medium",	CMIT_FMT_MEDIUM,	0 },
-		{ "short",	CMIT_FMT_SHORT,		0 },
-		{ "email",	CMIT_FMT_EMAIL,		0 },
-		{ "fuller",	CMIT_FMT_FULLER,	0 },
-		{ "full",	CMIT_FMT_FULL,		0 },
-		{ "oneline",	CMIT_FMT_ONELINE,	1 }
+		{ "raw",	CMIT_FMT_RAW,		0,	0 },
+		{ "medium",	CMIT_FMT_MEDIUM,	0,	1 },
+		{ "short",	CMIT_FMT_SHORT,		0,	0 },
+		{ "email",	CMIT_FMT_EMAIL,		0,	0 },
+		{ "fuller",	CMIT_FMT_FULLER,	0,	1 },
+		{ "full",	CMIT_FMT_FULL,		0,	1 },
+		{ "oneline",	CMIT_FMT_ONELINE,	1,	0 }
 	};
 	commit_formats_len = ARRAY_SIZE(builtin_formats);
 	builtin_formats_len = commit_formats_len;
@@ -172,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 
 	rev->commit_format = commit_format->format;
 	rev->use_terminator = commit_format->is_tformat;
+	rev->expand_tabs_in_log = commit_format->expand_tabs_in_log;
 	if (commit_format->format == CMIT_FMT_USERFORMAT) {
 		save_user_format(rev, commit_format->user_format,
 				 commit_format->is_tformat);
diff --git a/revision.c b/revision.c
index e662230..b1d767a 100644
--- a/revision.c
+++ b/revision.c
@@ -1412,6 +1412,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->skip_count = -1;
 	revs->max_count = -1;
 	revs->max_parents = -1;
+	revs->expand_tabs_in_log = 1;
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
@@ -1917,6 +1918,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		get_commit_format(arg+9, revs);
 	} else if (!strcmp(arg, "--expand-tabs")) {
 		revs->expand_tabs_in_log = 1;
+	} else if (!strcmp(arg, "--no-expand-tabs")) {
+		revs->expand_tabs_in_log = 0;
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 7600a3e..2fec948 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -115,7 +115,7 @@ EOF
 '
 
 test_expect_success !MINGW 'shortlog from non-git directory' '
-	git log HEAD >log &&
+	git log --no-expand-tabs HEAD >log &&
 	GIT_DIR=non-existing git shortlog -w <log >out &&
 	test_cmp expect out
 '
-- 
2.8.0-215-gd29a7d9
Jeff King· Mar 30, 2016, 01:38 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 2/3] pretty: enable --expand-tabs by default for selected pretty formats

On Tue, Mar 29, 2016 at 04:15:08PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
> index 4fb5c76..23967b6 100644
> --- a/Documentation/pretty-options.txt
> +++ b/Documentation/pretty-options.txt
> @@ -43,10 +43,16 @@ people using 80-column terminals.
>  	commit may be copied to the output.
>  
>  --expand-tabs::
> +--no-expand-tabs::
>  	Perform a tab expansion (replace each tab with enough number
>  	of spaces to fill to the next display column that is
>  	multiple of 8) in the log message before using the message
>  	to show in the output.
> ++
> +By default, tabs are expanded in pretty formats that indent the log
> +message by 4 spaces (i.e.  'medium', which is the default, 'full',
> +and "fuller').  `--no-expand-tabs` option can be used to disable
> +this.
Mismatched quote types on "fuller".
Show 8 quoted lines
> @@ -172,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
>  
>  	rev->commit_format = commit_format->format;
>  	rev->use_terminator = commit_format->is_tformat;
> +	rev->expand_tabs_in_log = commit_format->expand_tabs_in_log;
>  	if (commit_format->format == CMIT_FMT_USERFORMAT) {
>  		save_user_format(rev, commit_format->user_format,
>  				 commit_format->is_tformat);

This feels like the wrong time to set the value in rev_info, as it means that:

  git log --no-expand-tabs --pretty=full
and
  git log --pretty=full --no-expand-tabs

behave differently. The other values set in get_commit_format, like "use_terminator", are inherently part of the format, but I don't think this is. We just want to set the default if the user did not express another preference.

Likewise, if we were to eventually add config like "[log]expandtab = 4", it should not be overridden by "--pretty=full" (but we probably _would_ want to have it kick in only for certain formats).

So I think we really want to set an alternate variable here, and then do something like:

  if (rev->expand_tabs_in_log < 0)
	rev->expand_tabs_in_log = rev->commit_format_expand_tab_default;
-Peff
Junio C Hamano· Mar 30, 2016, 19:18 UTC · re: Jeff King · lore

Re: [PATCH v4 2/3] pretty: enable --expand-tabs by default for selected pretty formats

Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> On Tue, Mar 29, 2016 at 04:15:08PM -0700, Junio C Hamano wrote:
>
>> diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
>> index 4fb5c76..23967b6 100644
>> --- a/Documentation/pretty-options.txt
>> +++ b/Documentation/pretty-options.txt
>> @@ -43,10 +43,16 @@ people using 80-column terminals.
>>  	commit may be copied to the output.
>>  
>>  --expand-tabs::
>> +--no-expand-tabs::
>>  	Perform a tab expansion (replace each tab with enough number
>>  	of spaces to fill to the next display column that is
>>  	multiple of 8) in the log message before using the message
>>  	to show in the output.
>> ++
>> +By default, tabs are expanded in pretty formats that indent the log
>> +message by 4 spaces (i.e.  'medium', which is the default, 'full',
>> +and "fuller').  `--no-expand-tabs` option can be used to disable
>> +this.
>
> Mismatched quote types on "fuller".
Thanks.
Show 19 quoted lines
>> @@ -172,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
>>  
>>  	rev->commit_format = commit_format->format;
>>  	rev->use_terminator = commit_format->is_tformat;
>> +	rev->expand_tabs_in_log = commit_format->expand_tabs_in_log;
>>  	if (commit_format->format == CMIT_FMT_USERFORMAT) {
>>  		save_user_format(rev, commit_format->user_format,
>>  				 commit_format->is_tformat);
>
> This feels like the wrong time to set the value in rev_info, as it means
> that:
>
>   git log --no-expand-tabs --pretty=full
>
> and
>
>   git log --pretty=full --no-expand-tabs
>
> behave differently.

I was sort of hoping that we can get away by defining that "an explicit --pretty asks for the full behaviour of the format it specifies, e.g. if you ask --pretty=medium, you are asking for 4-space indented tab-expanded log with the headers at the medium level of detail".

> The other values set in get_commit_format, like "use_terminator",
> are inherently part of the format, but I don't think this is.

IOW, I was hoping nobody would agree with that and rather everybody would consider tab-expansion is part of the format.

Let me try your way instead and report how it went when I send out a reroll.

> Likewise, if we were to eventually add config like "[log]expandtab = 4",
> it should not be overridden by "--pretty=full" (but we probably _would_
> want to have it kick in only for certain formats).

This is exactly why I didn't do a configuration variable, as I think we can make only 50% of people happy. Some would say "when I explicitly ask for the "email" format, I expect that expandtab configuration gets ignored" while others would say "I said I want expandtab in the configuration no matter what".

Junio C Hamano· Mar 29, 2016, 23:15 UTC · re: Junio C Hamano · lore

[PATCH v4 3/3] pretty: allow tweaking tabwidth in --expand-tabs

When the local convention of the project is to use tab width that is not 8, it may make sense to allow "git log --expand-tabs=<n>" to tweak the output to match it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/pretty-options.txt | 10 ++++++----
 commit.h                         |  2 +-
 pretty.c                         | 12 ++++++------
 revision.c                       |  9 +++++++--
 revision.h                       |  3 +--
 5 files changed, 21 insertions(+), 15 deletions(-)
Show changes to 5 files +21 −15

Documentation/pretty-options.txt, commit.h, pretty.c, revision.c, revision.h

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 23967b6..8a944b1 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -42,17 +42,19 @@ people using 80-column terminals.
 	verbatim; this means that invalid sequences in the original
 	commit may be copied to the output.
 
+--expand-tabs=<n>::
 --expand-tabs::
 --no-expand-tabs::
 	Perform a tab expansion (replace each tab with enough number
 	of spaces to fill to the next display column that is
-	multiple of 8) in the log message before using the message
-	to show in the output.
+	multiple of '<n>') in the log message before using the message
+	to show in the output.  `--expand-tabs` is a short-hand for
+	`--expand-tabs=8`, and `--no-expand-tabs` is a short-hand for
+	`--expand-tabs=0`, which disables tab expansion.
 +
 By default, tabs are expanded in pretty formats that indent the log
 message by 4 spaces (i.e.  'medium', which is the default, 'full',
-and "fuller').  `--no-expand-tabs` option can be used to disable
-this.
+and "fuller').
 
 ifndef::git-rev-list[]
 --notes[=<ref>]::
diff --git a/commit.h b/commit.h
index a7ef682..2185c8d 100644
--- a/commit.h
+++ b/commit.h
@@ -147,7 +147,7 @@ struct pretty_print_context {
 	int preserve_subject;
 	struct date_mode date_mode;
 	unsigned date_mode_explicit:1;
-	unsigned expand_tabs_in_log:1;
+	unsigned expand_tabs_in_log;
 	int need_8bit_cte;
 	char *notes_message;
 	struct reflog_walk_info *reflog_info;
diff --git a/pretty.c b/pretty.c
index de22a8c..b340ecd 100644
--- a/pretty.c
+++ b/pretty.c
@@ -89,11 +89,11 @@ static void setup_commit_formats(void)
 {
 	struct cmt_fmt_map builtin_formats[] = {
 		{ "raw",	CMIT_FMT_RAW,		0,	0 },
-		{ "medium",	CMIT_FMT_MEDIUM,	0,	1 },
+		{ "medium",	CMIT_FMT_MEDIUM,	0,	8 },
 		{ "short",	CMIT_FMT_SHORT,		0,	0 },
 		{ "email",	CMIT_FMT_EMAIL,		0,	0 },
-		{ "fuller",	CMIT_FMT_FULLER,	0,	1 },
-		{ "full",	CMIT_FMT_FULL,		0,	1 },
+		{ "fuller",	CMIT_FMT_FULLER,	0,	8 },
+		{ "full",	CMIT_FMT_FULL,		0,	8 },
 		{ "oneline",	CMIT_FMT_ONELINE,	1,	0 }
 	};
 	commit_formats_len = ARRAY_SIZE(builtin_formats);
@@ -1645,7 +1645,7 @@ static int pp_utf8_width(const char *start, const char *end)
 	return width;
 }
 
-static void strbuf_add_tabexpand(struct strbuf *sb,
+static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,
 				 const char *line, int linelen)
 {
 	const char *tab;
@@ -1666,7 +1666,7 @@ static void strbuf_add_tabexpand(struct strbuf *sb,
 		strbuf_add(sb, line, tab - line);
 
 		/* .. and the de-tabified tab */
-		strbuf_addchars(sb, ' ', 8 - (width % 8));
+		strbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));
 
 		/* Skip over the printed part .. */
 		linelen -= tab + 1 - line;
@@ -1692,7 +1692,7 @@ static void pp_handle_indent(struct pretty_print_context *pp,
 {
 	strbuf_addchars(sb, ' ', indent);
 	if (pp->expand_tabs_in_log)
-		strbuf_add_tabexpand(sb, line, linelen);
+		strbuf_add_tabexpand(sb, pp->expand_tabs_in_log, line, linelen);
 	else
 		strbuf_add(sb, line, linelen);
 }
diff --git a/revision.c b/revision.c
index b1d767a..4f9ecbe 100644
--- a/revision.c
+++ b/revision.c
@@ -1412,7 +1412,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->skip_count = -1;
 	revs->max_count = -1;
 	revs->max_parents = -1;
-	revs->expand_tabs_in_log = 1;
+	revs->expand_tabs_in_log = 8;
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
@@ -1917,9 +1917,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->pretty_given = 1;
 		get_commit_format(arg+9, revs);
 	} else if (!strcmp(arg, "--expand-tabs")) {
-		revs->expand_tabs_in_log = 1;
+		revs->expand_tabs_in_log = 8;
 	} else if (!strcmp(arg, "--no-expand-tabs")) {
 		revs->expand_tabs_in_log = 0;
+	} else if (skip_prefix(arg, "--expand-tabs=", &arg)) {
+		int val;
+		if (strtol_i(arg, 10, &val) < 0 || val < 0)
+			die("'%s': not a non-negative integer", arg);
+		revs->expand_tabs_in_log = val;
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
diff --git a/revision.h b/revision.h
index 4079753..cf6615a 100644
--- a/revision.h
+++ b/revision.h
@@ -133,7 +133,6 @@ struct rev_info {
 			show_notes_given:1,
 			show_signature:1,
 			pretty_given:1,
-			expand_tabs_in_log:1,
 			abbrev_commit:1,
 			abbrev_commit_given:1,
 			zero_commit:1,
@@ -149,7 +148,7 @@ struct rev_info {
 			linear:1;
 
 	struct date_mode date_mode;
-
+	unsigned int	expand_tabs_in_log;
 	unsigned int	abbrev;
 	enum cmit_fmt	commit_format;
 	struct log_info *loginfo;
-- 
2.8.0-215-gd29a7d9
Junio C Hamano· Apr 5, 2016, 00:58 UTC · re: Junio C Hamano · lore

[PATCH v5 0/4] Expanding tabs in "git log" output

So here is the fifth and hopefully the final try. Previous round are at $gmane/289694, $gmane/289166, $gmane/288987 and $gmane/290222.

This round is different from v4 in the following ways.
 * wording changes, grammo- and typo-fixes in the documentation
   (thanks to Eric Sunshine and Jeff King).
 * v4 made --pretty=$fmt to override an earlier --expand-tabs=<n>;
   this round only allows --pretty=$fmt to set the default behaviour
   when an explicit --expand-tabs=<n> is given on the command line
   (thanks to Jeff King).
   
 * comes with a test.
See the end of this cover letter for an interdiff.
Junio C Hamano (3):
  pretty: enable --expand-tabs by default for selected pretty formats
  pretty: allow tweaking tabwidth in --expand-tabs
  pretty: test --expand-tabs
Linus Torvalds (1):
  pretty: expand tabs in indented logs to make things line up properly
 Documentation/pretty-options.txt | 14 ++++++
 builtin/log.c                    |  1 +
 commit.h                         |  1 +
 log-tree.c                       |  1 +
 pretty.c                         | 90 ++++++++++++++++++++++++++++++++----
 revision.c                       | 14 ++++++
 revision.h                       |  2 +
 t/t4201-shortlog.sh              |  2 +-
 t/t4213-log-tabexpand.sh         | 98 ++++++++++++++++++++++++++++++++++++++++
 9 files changed, 213 insertions(+), 10 deletions(-)
 create mode 100755 t/t4213-log-tabexpand.sh
-- 
2.8.1-251-g9997610


diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 8a944b1..93ad1cd 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -45,16 +45,16 @@ people using 80-column terminals.
 --expand-tabs=<n>::
 --expand-tabs::
 --no-expand-tabs::
-	Perform a tab expansion (replace each tab with enough number
-	of spaces to fill to the next display column that is
-	multiple of '<n>') in the log message before using the message
-	to show in the output.  `--expand-tabs` is a short-hand for
-	`--expand-tabs=8`, and `--no-expand-tabs` is a short-hand for
-	`--expand-tabs=0`, which disables tab expansion.
+	Perform a tab expansion (replace each tab with enough spaces
+	to fill to the next display column that is multiple of '<n>')
+	in the log message before showing it in the output.
+	`--expand-tabs` is a short-hand for `--expand-tabs=8`, and
+	`--no-expand-tabs` is a short-hand for `--expand-tabs=0`,
+	which disables tab expansion.
 +
 By default, tabs are expanded in pretty formats that indent the log
 message by 4 spaces (i.e.  'medium', which is the default, 'full',
-and "fuller').
+and 'fuller').
 
 ifndef::git-rev-list[]
 --notes[=<ref>]::
diff --git a/builtin/log.c b/builtin/log.c
index e00cea7..e5775ae 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1281,6 +1281,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	git_config(git_format_config, NULL);
 	init_revisions(&rev, prefix);
 	rev.commit_format = CMIT_FMT_EMAIL;
+	rev.expand_tabs_in_log_default = 0;
 	rev.verbose_header = 1;
 	rev.diff = 1;
 	rev.max_parents = 1;
diff --git a/commit.h b/commit.h
index 2185c8d..b06db4d 100644
--- a/commit.h
+++ b/commit.h
@@ -147,7 +147,7 @@ struct pretty_print_context {
 	int preserve_subject;
 	struct date_mode date_mode;
 	unsigned date_mode_explicit:1;
-	unsigned expand_tabs_in_log;
+	int expand_tabs_in_log;
 	int need_8bit_cte;
 	char *notes_message;
 	struct reflog_walk_info *reflog_info;
diff --git a/pretty.c b/pretty.c
index b340ecd..87c4497 100644
--- a/pretty.c
+++ b/pretty.c
@@ -173,7 +173,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 
 	rev->commit_format = commit_format->format;
 	rev->use_terminator = commit_format->is_tformat;
-	rev->expand_tabs_in_log = commit_format->expand_tabs_in_log;
+	rev->expand_tabs_in_log_default = commit_format->expand_tabs_in_log;
 	if (commit_format->format == CMIT_FMT_USERFORMAT) {
 		save_user_format(rev, commit_format->user_format,
 				 commit_format->is_tformat);
@@ -1722,6 +1722,9 @@ void pp_remainder(struct pretty_print_context *pp,
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent)
 			pp_handle_indent(pp, sb, indent, line, linelen);
+		else if (pp->expand_tabs_in_log)
+			strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
+					     line, linelen);
 		else
 			strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
diff --git a/revision.c b/revision.c
index 4f9ecbe..47e9ee7 100644
--- a/revision.c
+++ b/revision.c
@@ -1412,9 +1412,10 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->skip_count = -1;
 	revs->max_count = -1;
 	revs->max_parents = -1;
-	revs->expand_tabs_in_log = 8;
+	revs->expand_tabs_in_log = -1;
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
+	revs->expand_tabs_in_log_default = 8;
 
 	init_grep_defaults();
 	grep_init(&revs->grep_filter, prefix);
@@ -2398,6 +2399,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->first_parent_only && revs->bisect)
 		die(_("--first-parent is incompatible with --bisect"));
 
+	if (revs->expand_tabs_in_log < 0)
+		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
+
 	return left;
 }
 
diff --git a/revision.h b/revision.h
index cf6615a..6cc36b4 100644
--- a/revision.h
+++ b/revision.h
@@ -148,7 +148,9 @@ struct rev_info {
 			linear:1;
 
 	struct date_mode date_mode;
-	unsigned int	expand_tabs_in_log;
+	int		expand_tabs_in_log; /* unset if negative */
+	int		expand_tabs_in_log_default;
+
 	unsigned int	abbrev;
 	enum cmit_fmt	commit_format;
 	struct log_info *loginfo;
diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh
new file mode 100755
index 0000000..74ca03a
--- /dev/null
+++ b/t/t4213-log-tabexpand.sh
@@ -0,0 +1,98 @@
+#!/bin/sh
+
+test_description='log/show --expand-tabs'
+
+. ./test-lib.sh
+
+HT="	"
+title='tab indent at the beginning of the title line'
+body='tab indent on a line in the body'
+
+count_expand ()
+{
+	case " $* " in
+	*' --pretty=short '*)
+		line=$title ;;
+	*)
+		line=$body ;;
+	esac
+	expect=
+	count=$(( $1 + $2 )) ;# expected spaces
+	while test $count -gt 0
+	do
+		expect="$expect "
+		count=$(( $count - 1 ))
+	done
+	shift 2
+	count=$1 ;# expected tabs
+	while test $count -gt 0
+	do
+		expect="$expect$HT"
+		count=$(( $count - 1 ))
+	done
+	shift
+	{
+		echo "git show -s $*"
+		echo "$expect$line"
+	} | sed -e 's/ /./g' >expect
+
+	{
+		echo "git show -s $*"
+		git show -s "$@" |
+		sed -n -e "/$line\$/p"
+	} | sed -e 's/ /./g' >actual
+
+	test_cmp expect actual
+}
+
+test_expand ()
+{
+	fmt=$1
+	case "$fmt" in
+	*=raw | *=short | *=email)
+		default="0 1" ;;
+	*)
+		default="8 0" ;;
+	esac
+	case "$fmt" in
+	*=email)
+		in=0 ;;
+	*)
+		in=4 ;;
+	esac
+	test_expect_success "expand/no-expand${fmt:+ for $fmt}" '
+		count_expand $in $default $fmt &&
+		count_expand $in 8 0 $fmt --expand-tabs &&
+		count_expand $in 8 0 --expand-tabs $fmt &&
+		count_expand $in 8 0 $fmt --expand-tabs=8 &&
+		count_expand $in 8 0 --expand-tabs=8 $fmt &&
+		count_expand $in 0 1 $fmt --no-expand-tabs &&
+		count_expand $in 0 1 --no-expand-tabs $fmt &&
+		count_expand $in 0 1 $fmt --expand-tabs=0 &&
+		count_expand $in 0 1 --expand-tabs=0 $fmt &&
+		count_expand $in 4 0 $fmt --expand-tabs=4 &&
+		count_expand $in 4 0 --expand-tabs=4 $fmt
+	'
+}
+
+test_expect_success 'setup' '
+	test_tick &&
+	sed -e "s/Q/$HT/g" <<-EOF >msg &&
+	Q$title
+
+	Q$body
+	EOF
+	git commit --allow-empty -F msg
+'
+
+test_expand ""
+test_expand --pretty
+test_expand --pretty=short
+test_expand --pretty=medium
+test_expand --pretty=full
+test_expand --pretty=fuller
+test_expand --pretty=fuller
+test_expand --pretty=raw
+test_expand --pretty=email
+
+test_done
Junio C Hamano· Apr 5, 2016, 00:58 UTC · re: Junio C Hamano · lore

[PATCH v5 1/4] pretty: expand tabs in indented logs to make things line up properly

From: Linus Torvalds <torvalds@linux-foundation.org>

A commit log message sometimes tries to line things up using tabs, assuming fixed-width font with the standard 8-place tab settings. Viewing such a commit however does not work well in "git log", as we indent the lines by prefixing 4 spaces in front of them.

This should all line up:
  Column 1	Column 2
  --------	--------
  A		B
  ABCD		EFGH
  SPACES        Instead of Tabs
Even with multi-byte UTF8 characters:
  Column 1	Column 2
  --------	--------
  Ä		B
  åäö		100
  A Møøse	once bit my sister..

Tab-expand the lines in "git log --expand-tabs" output before prefixing 4 spaces.

This is based on the patch by Linus Torvalds, but at this step, we require an explicit command line option to enable the behaviour.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/pretty-options.txt |  5 +++
 commit.h                         |  1 +
 log-tree.c                       |  1 +
 pretty.c                         | 71 ++++++++++++++++++++++++++++++++++++++--
 revision.c                       |  2 ++
 revision.h                       |  1 +
 6 files changed, 79 insertions(+), 2 deletions(-)
Show changes to 6 files +79 −2

Documentation/pretty-options.txt, commit.h, log-tree.c, pretty.c, revision.c, revision.h

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 4b659ac..d820653 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -42,6 +42,11 @@ people using 80-column terminals.
 	verbatim; this means that invalid sequences in the original
 	commit may be copied to the output.
 
+--expand-tabs::
+	Perform a tab expansion (replace each tab with enough spaces
+	to fill to the next display column that is multiple of 8)
+	in the log message before showing it in the output.
+
 ifndef::git-rev-list[]
 --notes[=<ref>]::
 	Show the notes (see linkgit:git-notes[1]) that annotate the
diff --git a/commit.h b/commit.h
index 5d58be0..a7ef682 100644
--- a/commit.h
+++ b/commit.h
@@ -147,6 +147,7 @@ struct pretty_print_context {
 	int preserve_subject;
 	struct date_mode date_mode;
 	unsigned date_mode_explicit:1;
+	unsigned expand_tabs_in_log:1;
 	int need_8bit_cte;
 	char *notes_message;
 	struct reflog_walk_info *reflog_info;
diff --git a/log-tree.c b/log-tree.c
index 60f9839..78a5381 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
 	ctx.fmt = opt->commit_format;
 	ctx.mailmap = opt->mailmap;
 	ctx.color = opt->diffopt.use_color;
+	ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
 	ctx.output_encoding = get_log_output_encoding();
 	if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
 		ctx.from_ident = &opt->from_ident;
diff --git a/pretty.c b/pretty.c
index 92b2870..c8b075d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1629,6 +1629,72 @@ void pp_title_line(struct pretty_print_context *pp,
 	strbuf_release(&title);
 }
 
+static int pp_utf8_width(const char *start, const char *end)
+{
+	int width = 0;
+	size_t remain = end - start;
+
+	while (remain) {
+		int n = utf8_width(&start, &remain);
+		if (n < 0 || !start)
+			return -1;
+		width += n;
+	}
+	return width;
+}
+
+static void strbuf_add_tabexpand(struct strbuf *sb,
+				 const char *line, int linelen)
+{
+	const char *tab;
+
+	while ((tab = memchr(line, '\t', linelen)) != NULL) {
+		int width = pp_utf8_width(line, tab);
+
+		/*
+		 * If it wasn't well-formed utf8, or it
+		 * had characters with badly defined
+		 * width (control characters etc), just
+		 * give up on trying to align things.
+		 */
+		if (width < 0)
+			break;
+
+		/* Output the data .. */
+		strbuf_add(sb, line, tab - line);
+
+		/* .. and the de-tabified tab */
+		strbuf_addchars(sb, ' ', 8 - (width % 8));
+
+		/* Skip over the printed part .. */
+		linelen -= tab + 1 - line;
+		line = tab + 1;
+	}
+
+	/*
+	 * Print out everything after the last tab without
+	 * worrying about width - there's nothing more to
+	 * align.
+	 */
+	strbuf_add(sb, line, linelen);
+}
+
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * the whole line (without the final newline), after
+ * de-tabifying.
+ */
+static void pp_handle_indent(struct pretty_print_context *pp,
+			     struct strbuf *sb, int indent,
+			     const char *line, int linelen)
+{
+	strbuf_addchars(sb, ' ', indent);
+	if (pp->expand_tabs_in_log)
+		strbuf_add_tabexpand(sb, line, linelen);
+	else
+		strbuf_add(sb, line, linelen);
+}
+
 void pp_remainder(struct pretty_print_context *pp,
 		  const char **msg_p,
 		  struct strbuf *sb,
@@ -1653,8 +1719,9 @@ void pp_remainder(struct pretty_print_context *pp,
 
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent)
-			strbuf_addchars(sb, ' ', indent);
-		strbuf_add(sb, line, linelen);
+			pp_handle_indent(pp, sb, indent, line, linelen);
+		else
+			strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
 	}
 }
diff --git a/revision.c b/revision.c
index df56fce..e662230 100644
--- a/revision.c
+++ b/revision.c
@@ -1915,6 +1915,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(arg+9, revs);
+	} else if (!strcmp(arg, "--expand-tabs")) {
+		revs->expand_tabs_in_log = 1;
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
diff --git a/revision.h b/revision.h
index 23857c0..4079753 100644
--- a/revision.h
+++ b/revision.h
@@ -133,6 +133,7 @@ struct rev_info {
 			show_notes_given:1,
 			show_signature:1,
 			pretty_given:1,
+			expand_tabs_in_log:1,
 			abbrev_commit:1,
 			abbrev_commit_given:1,
 			zero_commit:1,
-- 
2.8.1-251-g9997610
Junio C Hamano· Apr 5, 2016, 00:58 UTC · re: Junio C Hamano · lore

[PATCH v5 2/4] pretty: enable --expand-tabs by default for selected pretty formats

"git log --pretty={medium,full,fuller}" and "git log" by default prepend 4 spaces to the log message, so it makes sense to enable the new "expand-tabs" facility by default for these formats. Add --no-expand-tabs option to override the new default.

The change alone breaks a test in t4201 that runs "git shortlog" on the output from "git log", and expects that the output from "git log" does not do such a tab expansion. Adjust the test to explicitly disable expand-tabs with --no-expand-tabs.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/pretty-options.txt |  6 ++++++
 builtin/log.c                    |  1 +
 pretty.c                         | 18 +++++++++++-------
 revision.c                       |  7 +++++++
 revision.h                       |  3 ++-
 t/t4201-shortlog.sh              |  2 +-
 6 files changed, 28 insertions(+), 9 deletions(-)
Show changes to 6 files +28 −9

Documentation/pretty-options.txt, builtin/log.c, pretty.c, revision.c, revision.h, t/t4201-shortlog.sh

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index d820653..edbb02f 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -43,9 +43,15 @@ people using 80-column terminals.
 	commit may be copied to the output.
 
 --expand-tabs::
+--no-expand-tabs::
 	Perform a tab expansion (replace each tab with enough spaces
 	to fill to the next display column that is multiple of 8)
 	in the log message before showing it in the output.
++
+By default, tabs are expanded in pretty formats that indent the log
+message by 4 spaces (i.e.  'medium', which is the default, 'full',
+and 'fuller').  `--no-expand-tabs` option can be used to disable
+this.
 
 ifndef::git-rev-list[]
 --notes[=<ref>]::
diff --git a/builtin/log.c b/builtin/log.c
index e00cea7..e5775ae 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1281,6 +1281,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	git_config(git_format_config, NULL);
 	init_revisions(&rev, prefix);
 	rev.commit_format = CMIT_FMT_EMAIL;
+	rev.expand_tabs_in_log_default = 0;
 	rev.verbose_header = 1;
 	rev.diff = 1;
 	rev.max_parents = 1;
diff --git a/pretty.c b/pretty.c
index c8b075d..b7938e0 100644
--- a/pretty.c
+++ b/pretty.c
@@ -16,6 +16,7 @@ static struct cmt_fmt_map {
 	const char *name;
 	enum cmit_fmt format;
 	int is_tformat;
+	int expand_tabs_in_log;
 	int is_alias;
 	const char *user_format;
 } *commit_formats;
@@ -87,13 +88,13 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
 static void setup_commit_formats(void)
 {
 	struct cmt_fmt_map builtin_formats[] = {
-		{ "raw",	CMIT_FMT_RAW,		0 },
-		{ "medium",	CMIT_FMT_MEDIUM,	0 },
-		{ "short",	CMIT_FMT_SHORT,		0 },
-		{ "email",	CMIT_FMT_EMAIL,		0 },
-		{ "fuller",	CMIT_FMT_FULLER,	0 },
-		{ "full",	CMIT_FMT_FULL,		0 },
-		{ "oneline",	CMIT_FMT_ONELINE,	1 }
+		{ "raw",	CMIT_FMT_RAW,		0,	0 },
+		{ "medium",	CMIT_FMT_MEDIUM,	0,	1 },
+		{ "short",	CMIT_FMT_SHORT,		0,	0 },
+		{ "email",	CMIT_FMT_EMAIL,		0,	0 },
+		{ "fuller",	CMIT_FMT_FULLER,	0,	1 },
+		{ "full",	CMIT_FMT_FULL,		0,	1 },
+		{ "oneline",	CMIT_FMT_ONELINE,	1,	0 }
 	};
 	commit_formats_len = ARRAY_SIZE(builtin_formats);
 	builtin_formats_len = commit_formats_len;
@@ -172,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 
 	rev->commit_format = commit_format->format;
 	rev->use_terminator = commit_format->is_tformat;
+	rev->expand_tabs_in_log_default = commit_format->expand_tabs_in_log;
 	if (commit_format->format == CMIT_FMT_USERFORMAT) {
 		save_user_format(rev, commit_format->user_format,
 				 commit_format->is_tformat);
@@ -1720,6 +1722,8 @@ void pp_remainder(struct pretty_print_context *pp,
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent)
 			pp_handle_indent(pp, sb, indent, line, linelen);
+		else if (pp->expand_tabs_in_log)
+			strbuf_add_tabexpand(sb, line, linelen);
 		else
 			strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
diff --git a/revision.c b/revision.c
index e662230..da53b6c 100644
--- a/revision.c
+++ b/revision.c
@@ -1412,8 +1412,10 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->skip_count = -1;
 	revs->max_count = -1;
 	revs->max_parents = -1;
+	revs->expand_tabs_in_log = -1;
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
+	revs->expand_tabs_in_log_default = 1;
 
 	init_grep_defaults();
 	grep_init(&revs->grep_filter, prefix);
@@ -1917,6 +1919,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		get_commit_format(arg+9, revs);
 	} else if (!strcmp(arg, "--expand-tabs")) {
 		revs->expand_tabs_in_log = 1;
+	} else if (!strcmp(arg, "--no-expand-tabs")) {
+		revs->expand_tabs_in_log = 0;
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
@@ -2390,6 +2394,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->first_parent_only && revs->bisect)
 		die(_("--first-parent is incompatible with --bisect"));
 
+	if (revs->expand_tabs_in_log < 0)
+		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
+
 	return left;
 }
 
diff --git a/revision.h b/revision.h
index 4079753..6cc36b4 100644
--- a/revision.h
+++ b/revision.h
@@ -133,7 +133,6 @@ struct rev_info {
 			show_notes_given:1,
 			show_signature:1,
 			pretty_given:1,
-			expand_tabs_in_log:1,
 			abbrev_commit:1,
 			abbrev_commit_given:1,
 			zero_commit:1,
@@ -149,6 +148,8 @@ struct rev_info {
 			linear:1;
 
 	struct date_mode date_mode;
+	int		expand_tabs_in_log; /* unset if negative */
+	int		expand_tabs_in_log_default;
 
 	unsigned int	abbrev;
 	enum cmit_fmt	commit_format;
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 7600a3e..2fec948 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -115,7 +115,7 @@ EOF
 '
 
 test_expect_success !MINGW 'shortlog from non-git directory' '
-	git log HEAD >log &&
+	git log --no-expand-tabs HEAD >log &&
 	GIT_DIR=non-existing git shortlog -w <log >out &&
 	test_cmp expect out
 '
-- 
2.8.1-251-g9997610
Junio C Hamano· Apr 5, 2016, 00:58 UTC · re: Junio C Hamano · lore

[PATCH v5 3/4] pretty: allow tweaking tabwidth in --expand-tabs

When the local convention of the project is to use tab width that is not 8, it may make sense to allow "git log --expand-tabs=<n>" to tweak the output to match it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/pretty-options.txt |  9 ++++++---
 commit.h                         |  2 +-
 pretty.c                         | 15 ++++++++-------
 revision.c                       |  9 +++++++--
 4 files changed, 22 insertions(+), 13 deletions(-)
Show changes to 4 files +22 −13

Documentation/pretty-options.txt, commit.h, pretty.c, revision.c

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index edbb02f..93ad1cd 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -42,16 +42,19 @@ people using 80-column terminals.
 	verbatim; this means that invalid sequences in the original
 	commit may be copied to the output.
 
+--expand-tabs=<n>::
 --expand-tabs::
 --no-expand-tabs::
 	Perform a tab expansion (replace each tab with enough spaces
-	to fill to the next display column that is multiple of 8)
+	to fill to the next display column that is multiple of '<n>')
 	in the log message before showing it in the output.
+	`--expand-tabs` is a short-hand for `--expand-tabs=8`, and
+	`--no-expand-tabs` is a short-hand for `--expand-tabs=0`,
+	which disables tab expansion.
 +
 By default, tabs are expanded in pretty formats that indent the log
 message by 4 spaces (i.e.  'medium', which is the default, 'full',
-and 'fuller').  `--no-expand-tabs` option can be used to disable
-this.
+and 'fuller').
 
 ifndef::git-rev-list[]
 --notes[=<ref>]::
diff --git a/commit.h b/commit.h
index a7ef682..b06db4d 100644
--- a/commit.h
+++ b/commit.h
@@ -147,7 +147,7 @@ struct pretty_print_context {
 	int preserve_subject;
 	struct date_mode date_mode;
 	unsigned date_mode_explicit:1;
-	unsigned expand_tabs_in_log:1;
+	int expand_tabs_in_log;
 	int need_8bit_cte;
 	char *notes_message;
 	struct reflog_walk_info *reflog_info;
diff --git a/pretty.c b/pretty.c
index b7938e0..87c4497 100644
--- a/pretty.c
+++ b/pretty.c
@@ -89,11 +89,11 @@ static void setup_commit_formats(void)
 {
 	struct cmt_fmt_map builtin_formats[] = {
 		{ "raw",	CMIT_FMT_RAW,		0,	0 },
-		{ "medium",	CMIT_FMT_MEDIUM,	0,	1 },
+		{ "medium",	CMIT_FMT_MEDIUM,	0,	8 },
 		{ "short",	CMIT_FMT_SHORT,		0,	0 },
 		{ "email",	CMIT_FMT_EMAIL,		0,	0 },
-		{ "fuller",	CMIT_FMT_FULLER,	0,	1 },
-		{ "full",	CMIT_FMT_FULL,		0,	1 },
+		{ "fuller",	CMIT_FMT_FULLER,	0,	8 },
+		{ "full",	CMIT_FMT_FULL,		0,	8 },
 		{ "oneline",	CMIT_FMT_ONELINE,	1,	0 }
 	};
 	commit_formats_len = ARRAY_SIZE(builtin_formats);
@@ -1645,7 +1645,7 @@ static int pp_utf8_width(const char *start, const char *end)
 	return width;
 }
 
-static void strbuf_add_tabexpand(struct strbuf *sb,
+static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,
 				 const char *line, int linelen)
 {
 	const char *tab;
@@ -1666,7 +1666,7 @@ static void strbuf_add_tabexpand(struct strbuf *sb,
 		strbuf_add(sb, line, tab - line);
 
 		/* .. and the de-tabified tab */
-		strbuf_addchars(sb, ' ', 8 - (width % 8));
+		strbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));
 
 		/* Skip over the printed part .. */
 		linelen -= tab + 1 - line;
@@ -1692,7 +1692,7 @@ static void pp_handle_indent(struct pretty_print_context *pp,
 {
 	strbuf_addchars(sb, ' ', indent);
 	if (pp->expand_tabs_in_log)
-		strbuf_add_tabexpand(sb, line, linelen);
+		strbuf_add_tabexpand(sb, pp->expand_tabs_in_log, line, linelen);
 	else
 		strbuf_add(sb, line, linelen);
 }
@@ -1723,7 +1723,8 @@ void pp_remainder(struct pretty_print_context *pp,
 		if (indent)
 			pp_handle_indent(pp, sb, indent, line, linelen);
 		else if (pp->expand_tabs_in_log)
-			strbuf_add_tabexpand(sb, line, linelen);
+			strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
+					     line, linelen);
 		else
 			strbuf_add(sb, line, linelen);
 		strbuf_addch(sb, '\n');
diff --git a/revision.c b/revision.c
index da53b6c..47e9ee7 100644
--- a/revision.c
+++ b/revision.c
@@ -1415,7 +1415,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->expand_tabs_in_log = -1;
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
-	revs->expand_tabs_in_log_default = 1;
+	revs->expand_tabs_in_log_default = 8;
 
 	init_grep_defaults();
 	grep_init(&revs->grep_filter, prefix);
@@ -1918,9 +1918,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->pretty_given = 1;
 		get_commit_format(arg+9, revs);
 	} else if (!strcmp(arg, "--expand-tabs")) {
-		revs->expand_tabs_in_log = 1;
+		revs->expand_tabs_in_log = 8;
 	} else if (!strcmp(arg, "--no-expand-tabs")) {
 		revs->expand_tabs_in_log = 0;
+	} else if (skip_prefix(arg, "--expand-tabs=", &arg)) {
+		int val;
+		if (strtol_i(arg, 10, &val) < 0 || val < 0)
+			die("'%s': not a non-negative integer", arg);
+		revs->expand_tabs_in_log = val;
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
-- 
2.8.1-251-g9997610
Junio C Hamano· Apr 5, 2016, 00:58 UTC · re: Junio C Hamano · lore

[PATCH v5 4/4] pretty: test --expand-tabs

The test prepares a simple commit with HT on its log message lines, and makes sure that

 - formats that should or should not expand tabs by default do or do
   not expand tabs respectively,
 - with explicit --expand-tabs=<N> and short-hands --expand-tabs
   (equivalent to --expand-tabs=8) and --no-expand-tabs (equivalent
   to --expand-tabs=0) before or after the explicit --pretty=$fmt,
   the tabs are expanded (or not expanded) accordingly.

The tests use the second line of the log message for formats other than --pretty=short, primarily because the first line of the email format is handled specially to add the [PATCH] prefix, etc. in a separate codepath (--pretty=short uses the first line because there is no other line to test).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4213-log-tabexpand.sh | 98 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100755 t/t4213-log-tabexpand.sh
Show changes to t/t4213-log-tabexpand.sh +98 −0
diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh
new file mode 100755
index 0000000..74ca03a
--- /dev/null
+++ b/t/t4213-log-tabexpand.sh
@@ -0,0 +1,98 @@
+#!/bin/sh
+
+test_description='log/show --expand-tabs'
+
+. ./test-lib.sh
+
+HT="	"
+title='tab indent at the beginning of the title line'
+body='tab indent on a line in the body'
+
+count_expand ()
+{
+	case " $* " in
+	*' --pretty=short '*)
+		line=$title ;;
+	*)
+		line=$body ;;
+	esac
+	expect=
+	count=$(( $1 + $2 )) ;# expected spaces
+	while test $count -gt 0
+	do
+		expect="$expect "
+		count=$(( $count - 1 ))
+	done
+	shift 2
+	count=$1 ;# expected tabs
+	while test $count -gt 0
+	do
+		expect="$expect$HT"
+		count=$(( $count - 1 ))
+	done
+	shift
+	{
+		echo "git show -s $*"
+		echo "$expect$line"
+	} | sed -e 's/ /./g' >expect
+
+	{
+		echo "git show -s $*"
+		git show -s "$@" |
+		sed -n -e "/$line\$/p"
+	} | sed -e 's/ /./g' >actual
+
+	test_cmp expect actual
+}
+
+test_expand ()
+{
+	fmt=$1
+	case "$fmt" in
+	*=raw | *=short | *=email)
+		default="0 1" ;;
+	*)
+		default="8 0" ;;
+	esac
+	case "$fmt" in
+	*=email)
+		in=0 ;;
+	*)
+		in=4 ;;
+	esac
+	test_expect_success "expand/no-expand${fmt:+ for $fmt}" '
+		count_expand $in $default $fmt &&
+		count_expand $in 8 0 $fmt --expand-tabs &&
+		count_expand $in 8 0 --expand-tabs $fmt &&
+		count_expand $in 8 0 $fmt --expand-tabs=8 &&
+		count_expand $in 8 0 --expand-tabs=8 $fmt &&
+		count_expand $in 0 1 $fmt --no-expand-tabs &&
+		count_expand $in 0 1 --no-expand-tabs $fmt &&
+		count_expand $in 0 1 $fmt --expand-tabs=0 &&
+		count_expand $in 0 1 --expand-tabs=0 $fmt &&
+		count_expand $in 4 0 $fmt --expand-tabs=4 &&
+		count_expand $in 4 0 --expand-tabs=4 $fmt
+	'
+}
+
+test_expect_success 'setup' '
+	test_tick &&
+	sed -e "s/Q/$HT/g" <<-EOF >msg &&
+	Q$title
+
+	Q$body
+	EOF
+	git commit --allow-empty -F msg
+'
+
+test_expand ""
+test_expand --pretty
+test_expand --pretty=short
+test_expand --pretty=medium
+test_expand --pretty=full
+test_expand --pretty=fuller
+test_expand --pretty=fuller
+test_expand --pretty=raw
+test_expand --pretty=email
+
+test_done
-- 
2.8.1-251-g9997610
Eric Sunshine· Apr 5, 2016, 01:10 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 4/4] pretty: test --expand-tabs

On Mon, Apr 4, 2016 at 8:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 38 quoted lines
> The test prepares a simple commit with HT on its log message lines,
> and makes sure that
>
>  - formats that should or should not expand tabs by default do or do
>    not expand tabs respectively,
>
>  - with explicit --expand-tabs=<N> and short-hands --expand-tabs
>    (equivalent to --expand-tabs=8) and --no-expand-tabs (equivalent
>    to --expand-tabs=0) before or after the explicit --pretty=$fmt,
>    the tabs are expanded (or not expanded) accordingly.
>
> The tests use the second line of the log message for formats other
> than --pretty=short, primarily because the first line of the email
> format is handled specially to add the [PATCH] prefix, etc. in a
> separate codepath (--pretty=short uses the first line because there
> is no other line to test).
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh
> @@ -0,0 +1,98 @@
> +count_expand ()
> +{
> +       case " $* " in
> +       *' --pretty=short '*)
> +               line=$title ;;
> +       *)
> +               line=$body ;;
> +       esac
> +       expect=
> +       count=$(( $1 + $2 )) ;# expected spaces
> +       while test $count -gt 0
> +       do
> +               expect="$expect "
> +               count=$(( $count - 1 ))
> +       done
> +       shift 2
> +       count=$1 ;# expected tabs
Why semicolon before the hash here and above?
Jeff King· Apr 5, 2016, 01:47 UTC · re: Eric Sunshine · lore

Re: [PATCH v5 4/4] pretty: test --expand-tabs

On Mon, Apr 04, 2016 at 09:10:46PM -0400, Eric Sunshine wrote:
> > +       count=$1 ;# expected tabs
> 
> Why semicolon before the hash here and above?

I am in the habit of doing this, too. I have a vague recollection of getting bitten by a shell that treated:

  echo foo # bar

or something similar as not-a-comment. But neither bash, dash, nor ksh seem to. So I'm not sure if it was some other shell in my past, or if I simply have an irrational fear.

-Peff
Junio C Hamano· Apr 5, 2016, 06:25 UTC · re: Jeff King · lore

Re: [PATCH v5 4/4] pretty: test --expand-tabs

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> On Mon, Apr 04, 2016 at 09:10:46PM -0400, Eric Sunshine wrote:
>
>> > +       count=$1 ;# expected tabs
>> 
>> Why semicolon before the hash here and above?
>
> I am in the habit of doing this, too. I have a vague recollection of
> getting bitten by a shell that treated:
>
>   echo foo # bar
>
> or something similar as not-a-comment. But neither bash, dash, nor ksh
> seem to.

I think the reason why I started doing the same is because some shells can be configured to lose the comment-introducer-ness of "#" in interactive mode, and I wanted to make sure that many things I write can be tried out by others more easily by copy-and-paste to their interactive session.

Jeff King· Apr 5, 2016, 01:52 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 4/4] pretty: test --expand-tabs

On Mon, Apr 04, 2016 at 05:58:37PM -0700, Junio C Hamano wrote:
> +count_expand ()
> +{

This function takes a lot of unnamed arguments that we process with "shift". It might be nice to give a brief comment describing them.

Show 9 quoted lines
> +test_expand ""
> +test_expand --pretty
> +test_expand --pretty=short
> +test_expand --pretty=medium
> +test_expand --pretty=full
> +test_expand --pretty=fuller
> +test_expand --pretty=fuller
> +test_expand --pretty=raw
> +test_expand --pretty=email
Duplicated fuller?
-Peff
Junio C Hamano· Apr 5, 2016, 06:32 UTC · re: Jeff King · lore

Re: [PATCH v5 4/4] pretty: test --expand-tabs

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> On Mon, Apr 04, 2016 at 05:58:37PM -0700, Junio C Hamano wrote:
>
>> +count_expand ()
>> +{
>
> This function takes a lot of unnamed arguments that we process with
> "shift". It might be nice to give a brief comment describing them.
> ...
>> +test_expand --pretty=fuller
>> +test_expand --pretty=fuller
> ...
> Duplicated fuller?
Thanks.  Here is a replacement.

-- >8 -- The test prepares a simple commit with HT on its log message lines, and makes sure that

 - formats that should or should not expand tabs by default do or do
   not expand tabs respectively,
 - with explicit --expand-tabs=<N> and short-hands --expand-tabs
   (equivalent to --expand-tabs=8) and --no-expand-tabs (equivalent
   to --expand-tabs=0) before or after the explicit --pretty=$fmt,
   the tabs are expanded (or not expanded) accordingly.

The tests use the second line of the log message for formats other than --pretty=short, primarily because the first line of the email format is handled specially to add the [PATCH] prefix, etc. in a separate codepath (--pretty=short uses the first line because there is no other line to test).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4213-log-tabexpand.sh | 105 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100755 t/t4213-log-tabexpand.sh
Show changes to t/t4213-log-tabexpand.sh +105 −0
diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh
new file mode 100755
index 0000000..e01a8f6
--- /dev/null
+++ b/t/t4213-log-tabexpand.sh
@@ -0,0 +1,105 @@
+#!/bin/sh
+
+test_description='log/show --expand-tabs'
+
+. ./test-lib.sh
+
+HT="	"
+title='tab indent at the beginning of the title line'
+body='tab indent on a line in the body'
+
+# usage: count_expand $indent $numSP $numHT @format_args
+count_expand ()
+{
+	expect=
+	count=$(( $1 + $2 )) ;# expected spaces
+	while test $count -gt 0
+	do
+		expect="$expect "
+		count=$(( $count - 1 ))
+	done
+	shift 2
+	count=$1 ;# expected tabs
+	while test $count -gt 0
+	do
+		expect="$expect$HT"
+		count=$(( $count - 1 ))
+	done
+	shift
+
+	# The remainder of the command line is "git show -s" options
+	case " $* " in
+	*' --pretty=short '*)
+		line=$title ;;
+	*)
+		line=$body ;;
+	esac
+
+	# Prefix the output with the command line arguments, and
+	# replace SP with a dot both in the expecte and actual output
+	# so that test_cmp would show the differene together with the
+	# breakage in a way easier to consume by the debugging user.
+	{
+		echo "git show -s $*"
+		echo "$expect$line"
+	} | sed -e 's/ /./g' >expect
+
+	{
+		echo "git show -s $*"
+		git show -s "$@" |
+		sed -n -e "/$line\$/p"
+	} | sed -e 's/ /./g' >actual
+
+	test_cmp expect actual
+}
+
+test_expand ()
+{
+	fmt=$1
+	case "$fmt" in
+	*=raw | *=short | *=email)
+		default="0 1" ;;
+	*)
+		default="8 0" ;;
+	esac
+	case "$fmt" in
+	*=email)
+		in=0 ;;
+	*)
+		in=4 ;;
+	esac
+	test_expect_success "expand/no-expand${fmt:+ for $fmt}" '
+		count_expand $in $default $fmt &&
+		count_expand $in 8 0 $fmt --expand-tabs &&
+		count_expand $in 8 0 --expand-tabs $fmt &&
+		count_expand $in 8 0 $fmt --expand-tabs=8 &&
+		count_expand $in 8 0 --expand-tabs=8 $fmt &&
+		count_expand $in 0 1 $fmt --no-expand-tabs &&
+		count_expand $in 0 1 --no-expand-tabs $fmt &&
+		count_expand $in 0 1 $fmt --expand-tabs=0 &&
+		count_expand $in 0 1 --expand-tabs=0 $fmt &&
+		count_expand $in 4 0 $fmt --expand-tabs=4 &&
+		count_expand $in 4 0 --expand-tabs=4 $fmt
+	'
+}
+
+test_expect_success 'setup' '
+	test_tick &&
+	sed -e "s/Q/$HT/g" <<-EOF >msg &&
+	Q$title
+
+	Q$body
+	EOF
+	git commit --allow-empty -F msg
+'
+
+test_expand ""
+test_expand --pretty
+test_expand --pretty=short
+test_expand --pretty=medium
+test_expand --pretty=full
+test_expand --pretty=fuller
+test_expand --pretty=raw
+test_expand --pretty=email
+
+test_done
-- 
2.8.1-253-gd0f4798
Perry Hutchison· Apr 5, 2016, 07:13 UTC · re: Jeff King · lore

Re: [PATCH v5 4/4] pretty: test --expand-tabs

Jeff King <peff@peff.net> wrote:
> > +test_expand --pretty=fuller
> > +test_expand --pretty=fuller
>
> Duplicated fuller?
Just brush it off :)
[Those too young to get the joke can
 look up "fuller brush" in Wikipedia.]
Jeff King· Apr 5, 2016, 01:53 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 0/4] Expanding tabs in "git log" output

On Mon, Apr 04, 2016 at 05:58:33PM -0700, Junio C Hamano wrote:
> So here is the fifth and hopefully the final try.  Previous round
> are at $gmane/289694, $gmane/289166, $gmane/288987 and
> $gmane/290222.

With the exception of two minor nits on the final patch, this looks good to me. Thanks for cleaning up the option-parsing ordering thing; I think it turned out rather nice.

-Peff
Linus Torvalds· Mar 16, 2016, 19:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

On Wed, Mar 16, 2016 at 12:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
>
> may give us a better structure if we are going to give users a knob
> to disable this tab expansion, i.e. move the addition of 4 spaces to
> the caller, name the body of such a function strbuf_expand_add(),
> and then make the caller do something like this perhaps?

I'd suggest just putting that knob into the "pp_handle_indent()" function, and passing it the "pp" pointer.

In fact, maybe it should just be renamed as "pp_add_line()", and handle every case, and keep "pp_remainder()" as just the "loop over each line and handle the empty line and PP_SHORT special case" thing.

That makes it easty to add that CMIT_FMT_EXPAND_TABS kind of code later.

Here's an incremental patch that could be just smushed into my previous one. It doesn't change the behavior of "pp_handle_indent()", but I think it clarifies the code and makes future changes much easier (partly because now nobody has to worry about the continue case and the newline at the end of the line, so you can just print whatever you want and then return).

What do you think?
                 Linus
 pretty.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
Show changes to pretty.c +13 −6
diff --git a/pretty.c b/pretty.c
index 0b40457f99f0..b9374a1708d1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1699,6 +1699,18 @@ static int pp_handle_indent(struct strbuf *sb, int indent,
 	return 1;
 }
 
+static void pp_add_line(struct pretty_print_context *pp,
+			struct strbuf *sb, int indent,
+			const char *line, int linelen)
+{
+	strbuf_grow(sb, linelen + indent + 20);
+	if (indent) {
+		if (pp_handle_indent(sb, indent, line, linelen))
+			return;
+	}
+	strbuf_add(sb, line, linelen);
+}
+
 void pp_remainder(struct pretty_print_context *pp,
 		  const char **msg_p,
 		  struct strbuf *sb,
@@ -1721,12 +1733,7 @@ void pp_remainder(struct pretty_print_context *pp,
 		}
 		first = 0;
 
-		strbuf_grow(sb, linelen + indent + 20);
-		if (indent) {
-			if (pp_handle_indent(sb, indent, line, linelen))
-				linelen = 0;
-		}
-		strbuf_add(sb, line, linelen);
+		pp_add_line(pp, sb, indent, line, linelen);
 		strbuf_addch(sb, '\n');
 	}
 }
Junio C Hamano· Mar 16, 2016, 21:55 UTC · re: Linus Torvalds · lore

Re: [PATCH] pretty-print: de-tabify indented logs to make things line up properly

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 8 quoted lines
> Here's an incremental patch that could be just smushed into my
> previous one. It doesn't change the behavior of "pp_handle_indent()",
> but I think it clarifies the code and makes future changes much easier
> (partly because now nobody has to worry about the continue case and
> the newline at the end of the line, so you can just print whatever you
> want and then return).
>
> What do you think?

I actually was hoping that strbuf_tabexpand_add() would be an independently useful addition to strbuf_add*() family that does not have to know the indentation and pretty-print-context, so burying that new logic deep in the callchain like the patch below, while letting it still be aware of "indent" value (and adding the leading indent) does not look like a good abstraction from that point of view.

The change to pp_remainder() looks like a nice cleanup for people who read that function. I still think pp_add_line() should add the indentation to sb in "if (indent)" block itself, and the called helper should be a "I know how to tab-expand a string and add the result to a strbuf" that we can eventually move to strbuf.[ch].

Things like adding CMIT_FMT_EXPAND_TABS can still then be done with something like this:

	strbuf_grow();
	if (indent) {
		strbuf_addchars(sb, ' ', indent);
-        	strbuf_tabexpand_add();
+		if (pp->format == EXPAND_TABS)
+	        	strbuf_tabexpand_add();
+		else
+	        	strbuf_add();
	} else {
        	strbuf_add();
	}

in pp_add_line(), so there is no difference between the ease of future changes, I'd think.

Show 42 quoted lines
>                  Linus
>
>  pretty.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index 0b40457f99f0..b9374a1708d1 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1699,6 +1699,18 @@ static int pp_handle_indent(struct strbuf *sb, int indent,
>  	return 1;
>  }
>  
> +static void pp_add_line(struct pretty_print_context *pp,
> +			struct strbuf *sb, int indent,
> +			const char *line, int linelen)
> +{
> +	strbuf_grow(sb, linelen + indent + 20);
> +	if (indent) {
> +		if (pp_handle_indent(sb, indent, line, linelen))
> +			return;
> +	}
> +	strbuf_add(sb, line, linelen);
> +}
> +
>  void pp_remainder(struct pretty_print_context *pp,
>  		  const char **msg_p,
>  		  struct strbuf *sb,
> @@ -1721,12 +1733,7 @@ void pp_remainder(struct pretty_print_context *pp,
>  		}
>  		first = 0;
>  
> -		strbuf_grow(sb, linelen + indent + 20);
> -		if (indent) {
> -			if (pp_handle_indent(sb, indent, line, linelen))
> -				linelen = 0;
> -		}
> -		strbuf_add(sb, line, linelen);
> +		pp_add_line(pp, sb, indent, line, linelen);
>  		strbuf_addch(sb, '\n');
>  	}
>  }

← back to recent threads