[RFC PATCH] show decorations at the end of the line

[RFC PATCH] show decorations at the end of the line

[Linus Torvalds]

So I use "--show-decorations" all the time because I find it very useful 
to see where the origin branch is, where tags are etc. In fact, my global 
git config file has

    [log]
        decorate = auto

in it, so that I don't have to type it out all the time when I just do my 
usual 'git log". It's lovely.

However, it does make one particular case uglier: with commit decorations, 
the "oneline" commit format ends up being not very pretty:

    [torvalds@i7 git]$ git log --oneline -10
    3f07dac29 (HEAD -> master) pathspec: don't error out on  all-exclusionary pathspec patterns
    ca4a562f2 pathspec magic: add '^' as alias for '!'
    02555c1b2 ls-remote: add "--diff" option to show only refs that differ
    6e3a7b339 (tag: v2.12.0-rc0, origin/master, origin/HEAD) Git 2.12-rc0
    fafca0f72 Merge branch 'cw/log-updates-for-all-refs-really'
    74dee5cfa Merge branch 'pl/complete-diff-submodule-diff'
    36acf4123 Merge branch 'rs/object-id'
    ecc486b1f Merge branch 'js/re-running-failed-tests'
    4ba6bb2d1 Merge branch 'sb/submodule-update-initial-runs-custom-script'
    5348021c6 Merge branch 'sb/submodule-recursive-absorb'

and note how the decoration comes right after the shortened commit hash, 
breaking up the alignment of the messages. 

The above doesn't show it with the colorization: I also have

    [color]
        ui=auto

so on my terminal the decoration is also nicely colorized which makes it 
much more obvious, it's not as obvious in this message.

The oneline message handling is already pretty special, this makes it even 
more special by putting the decorations at the end of the line:

    3f07dac29 pathspec: don't error out on all-exclusionary pathspec patterns (HEAD -> master)
    ca4a562f2 pathspec magic: add '^' as alias for '!'
    02555c1b2 ls-remote: add "--diff" option to show only refs that differ
    6e3a7b339 Git 2.12-rc0 (tag: v2.12.0-rc0, origin/master, origin/HEAD)
    fafca0f72 Merge branch 'cw/log-updates-for-all-refs-really'
    74dee5cfa Merge branch 'pl/complete-diff-submodule-diff'
    36acf4123 Merge branch 'rs/object-id'
    ecc486b1f Merge branch 'js/re-running-failed-tests'
    4ba6bb2d1 Merge branch 'sb/submodule-update-initial-runs-custom-script'
    5348021c6 Merge branch 'sb/submodule-recursive-absorb'

which looks a lot better (again, this is all particularly noticeable with 
colorization).

NOTE! There's a very special case for "git log --oneline -g" that shows 
the reflogs as oneliners, and this does *not* fix that special case. It's 
a lot more involved and relies on the exact show_reflog_message() 
implementation, so I left the format for that alone, along with a comment 
about how it's not at the end of line.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

I've signed off on this, because I think it's an "obvious" improvement, 
but I'm putting the "RFC" in the subject line because this is clearly a 
subjective thing.

"oneline" really is special: the other commit formats will just put the 
commit SHA1 at the end of the line anyway. And with manual formats, the 
placement of decorations is also manual, so this doesn't affect that 
case.

Comments?

 log-tree.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/log-tree.c b/log-tree.c
index 8c2415747..3bf88182e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -622,10 +622,13 @@ void show_log(struct rev_info *opt)
 			       find_unique_abbrev(parent->object.oid.hash,
 						  abbrev_commit));
 		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
-		show_decorations(opt, commit);
 		if (opt->commit_format == CMIT_FMT_ONELINE) {
+			/* Not at end of line, but.. */
+			if (opt->reflog_info)
+				show_decorations(opt, commit);
 			putc(' ', opt->diffopt.file);
 		} else {
+			show_decorations(opt, commit);
 			putc('\n', opt->diffopt.file);
 			graph_show_oneline(opt->graph);
 		}
@@ -716,6 +719,8 @@ void show_log(struct rev_info *opt)
 		opt->missing_newline = 0;
 
 	graph_show_commit_msg(opt->graph, opt->diffopt.file, &msgbuf);
+	if (ctx.fmt == CMIT_FMT_ONELINE)
+		show_decorations(opt, commit);
 	if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);

Re: [RFC PATCH] show decorations at the end of the line

[Linus Torvalds]

On Sat, Feb 11, 2017 at 10:02 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I've signed off on this, because I think it's an "obvious" improvement,
> but I'm putting the "RFC" in the subject line because this is clearly a
> subjective thing.

Side note: the one downside of showing the decorations at the end of
the line is that now they are obviously at the end of the line - and
thus likely to be more hidden by things like line truncation.

The default git settings (LESS=FRX) no longer truncate log output
lines, but if you use "-S" or "--chop-long-lines", you will obviously
be missing the end of long lines.

So moving the decorations to the end does obviously have a real UI
impact, apart from just being "prettier".

I just wanted to point that out. I still prefer decorations at ends of
lines (and yes, that's despite the fact that I actually personally use
the traditional git setting of "LESS=FRSX"), but it is perhaps
something that people should be aware of if this patch causes
discussion.

               Linus

Re: [RFC PATCH] show decorations at the end of the line

[Junio C Hamano]

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

> On Sat, Feb 11, 2017 at 10:02 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I've signed off on this, because I think it's an "obvious" improvement,
>> but I'm putting the "RFC" in the subject line because this is clearly a
>> subjective thing.
>
> Side note: the one downside of showing the decorations at the end of
> the line is that now they are obviously at the end of the line - and
> thus likely to be more hidden by things like line truncation.

Side note: I refrained from commenting on this patch because
everybody knows that the what I would say anyway ;-) and I didn't
want to speak first to discourage others from raising their opinion.

An obvious downside is that people (against all recommendations) are
likely to have written a loose script expecting the --oneline format
is cast in stone.  I personally think it is OK to break them as long
as "workaround" (aka kosher way to do what they have been doing) is
obvious and easily doable, and in this case their script can switch
to use --format to keep using the order of fields and format they
have been relying on.

It would be nice if we can have that --format string they can use
somewhere in the log message, so that I can cut & paste it into the
release notes that contains this change (i.e. "those who want to
keep using the traditional --oneline --decorate can use this string
as pretty.my1line configuration variable and use --pretty=my1line
instead").

Re: [RFC PATCH] show decorations at the end of the line

[Linus Torvalds]

On Mon, Feb 13, 2017 at 12:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> An obvious downside is that people (against all recommendations) are
> likely to have written a loose script expecting the --oneline format
> is cast in stone.

Actually, I don't believe that is the case wrt decorations.

Why?

If you script the --oneline format and parse the output, you won't
have any decorations at all unless you are crazy (you can set
"log.decorations=true", but that will truly screw up any scripting).

And if you actually want decorations, and you're parsing them, you are
*not* going to script it with "--oneline --decorations", because the
end result is basically impossible to parse already (because it's
ambiguous - think about parentheses in the commit message).

So if you actually want decorations for parsing, you'd do something like

   git log --pretty="%h '%D' %s"

which is at least parseable (because now the decoration separator is
unconditional.

Yeah, I guess you could use "--decorations --color=always" and then
use the color codes to parse the decorations, but that's so
complicated as to be unrealistic.

And I considered adding a format string explanation, something along
the lines of

 - oneline used to mean "--pretty=%h%d %s", now it means "%h %s%d" instead

but that's actually not true. The "oneline" format was much more
complex than that, in that it has special rules for "-g", and it has
all those colorization ones too.

           Linus

Re: [RFC PATCH] show decorations at the end of the line

[Junio C Hamano]

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

> And if you actually want decorations, and you're parsing them, you are
> *not* going to script it with "--oneline --decorations", because the
> end result is basically impossible to parse already (because it's
> ambiguous - think about parentheses in the commit message).

OK.  So let's wait to hear from others if they like the "obviously"
improved output.  Even though I find the decorations indispensable
in my "git log" output, I personally do not have much preference
either way, as my screen is often wide enough ;-)

Thanks.  We'd need to update the tests that expects the old style
output, though.

Re: [RFC PATCH] show decorations at the end of the line

[Jeff King]

On Mon, Feb 13, 2017 at 01:01:49PM -0800, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > And if you actually want decorations, and you're parsing them, you are
> > *not* going to script it with "--oneline --decorations", because the
> > end result is basically impossible to parse already (because it's
> > ambiguous - think about parentheses in the commit message).
> 
> OK.  So let's wait to hear from others if they like the "obviously"
> improved output.  Even though I find the decorations indispensable
> in my "git log" output, I personally do not have much preference
> either way, as my screen is often wide enough ;-)

I have a slight preference for the new output (decorations at the end)
versus the original, but I could go either way.

I don't think the scripting compatibility concerns are an issue, for all
the reasons given in the thread.

There is one related option, --source, which also puts its data between
the hash and the subject in --oneline. In theory that should be treated
similarly, though:

  1. It's already really ugly, as it does not even get the parentheses
     and coloring.

  2. It's perhaps more likely to get scripted, as it really is parseable
     in the current state.

I'm not sure if a better path forward would be to just extend the idea
of "decorator" to possibly include more than just ref-tips. On the other
hand, if you really want to get fancy with formatting, we already have a
complete formatting language. Perhaps it should learn a placeholder for
the "--source" data.

Similarly, I've often wanted a "contained in this tags/branches"
annotation for each commit. It's not too expensive to compute if you
topo-sort the set of commits and just paint down as you traverse.

Anyway, I think none of that needs to block changes to --decorate
output. Just thinking out loud.

-Peff

Re: [RFC PATCH] show decorations at the end of the line

[Junio C Hamano]

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

> Thanks.  We'd need to update the tests that expects the old style
> output, though.

The updates to the expectation look like this (already squashed).
The --source decorations in 4202 are also shown at the end, which
probably is in line with the way --show-decorations adds them at the
end of the line, but was somewhat surprising from reading only the
log message.

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 48b55bfd27..dea2d449ab 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1353,9 +1353,9 @@ test_expect_success 'set up --source tests' '
 
 test_expect_success 'log --source paints branch names' '
 	cat >expect <<-\EOF &&
-	09e12a9	source-b three
-	8e393e1	source-a two
-	1ac6c77	source-b one
+	09e12a9 three	source-b
+	8e393e1 two	source-a
+	1ac6c77 one	source-b
 	EOF
 	git log --oneline --source source-a source-b >actual &&
 	test_cmp expect actual
@@ -1364,9 +1364,9 @@ test_expect_success 'log --source paints branch names' '
 test_expect_success 'log --source paints tag names' '
 	git tag -m tagged source-tag &&
 	cat >expect <<-\EOF &&
-	09e12a9	source-tag three
-	8e393e1	source-a two
-	1ac6c77	source-tag one
+	09e12a9 three	source-tag
+	8e393e1 two	source-a
+	1ac6c77 one	source-tag
 	EOF
 	git log --oneline --source source-tag source-a >actual &&
 	test_cmp expect actual
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index b972296f06..08236a83e7 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -44,15 +44,15 @@ test_expect_success setup '
 '
 
 cat >expected <<EOF
-${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD ->\
+${c_commit}COMMIT_ID${c_reset} B${c_commit} (${c_reset}${c_HEAD}HEAD ->\
  ${c_reset}${c_branch}master${c_reset}${c_commit},\
  ${c_reset}${c_tag}tag: v1.0${c_reset}${c_commit},\
- ${c_reset}${c_tag}tag: B${c_reset}${c_commit})${c_reset} B
-${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A1${c_reset}${c_commit},\
- ${c_reset}${c_remoteBranch}other/master${c_reset}${c_commit})${c_reset} A1
-${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_stash}refs/stash${c_reset}${c_commit})${c_reset}\
- On master: Changes to A.t
-${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A
+ ${c_reset}${c_tag}tag: B${c_reset}${c_commit})${c_reset}
+${c_commit}COMMIT_ID${c_reset} A1${c_commit} (${c_reset}${c_tag}tag: A1${c_reset}${c_commit},\
+ ${c_reset}${c_remoteBranch}other/master${c_reset}${c_commit})${c_reset}
+${c_commit}COMMIT_ID${c_reset} On master: Changes to A.t\
+${c_commit} (${c_reset}${c_stash}refs/stash${c_reset}${c_commit})${c_reset}
+${c_commit}COMMIT_ID${c_reset} A${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset}
 EOF
 
 # We want log to show all, but the second parent to refs/stash is irrelevant

Re: [RFC PATCH] show decorations at the end of the line

[Jeff King]

On Tue, Feb 14, 2017 at 02:11:06PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Thanks.  We'd need to update the tests that expects the old style
> > output, though.
> 
> The updates to the expectation look like this (already squashed).
> The --source decorations in 4202 are also shown at the end, which
> probably is in line with the way --show-decorations adds them at the
> end of the line, but was somewhat surprising from reading only the
> log message.

Hrm, that does surprise me. I'm not sure if that's desirable or not. I
do think some of the "nobody could possibly be parsing these" arguments
about decorations do not apply to --source (and also, they're harder for
humans to pick out from the end of the line as they lack punctuation and
color).

-Peff

Re: [RFC PATCH] show decorations at the end of the line

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> The updates to the expectation look like this (already squashed).
>> The --source decorations in 4202 are also shown at the end, which
>> probably is in line with the way --show-decorations adds them at the
>> end of the line, but was somewhat surprising from reading only the
>> log message.
>
> Hrm, that does surprise me. I'm not sure if that's desirable or not. I
> do think some of the "nobody could possibly be parsing these" arguments
> about decorations do not apply to --source (and also, they're harder for
> humans to pick out from the end of the line as they lack punctuation and
> color).

I just got bitten by a fallout.  I have

    $ git recent --help
    `git recent' is aliased to `log --oneline --branches --no-merges \
	 --source --since=3.weeks'

and often do

    $ git recent name-hash.c

primarily to see if I already queued a patch series to a topic (and
forgot about it), and/or what other recent topics in flight touch
the same thing.

I'd need that the topic name to be shown rather prominently for this
use case, i.e.

    eb2263adb1      jh/memihash-opt name-hash: remember previous dir_...
    0c04267dc8      jh/memihash-opt name-hash: specify initial size f...
    57463ce445      jh/memihash-opt name-hash: precompute hash values...
    dd3170e2cf      jh/memihash-opt name-hash: eliminate duplicate me...

but now the branch names are shown at the end, which defeats the
whole point of the alias.

If nobody gets around to fixing it, I may take a look at it when
able, but for now let me just vent^Wreport a regression first.

Re: [RFC PATCH] show decorations at the end of the line

[Linus Torvalds]

On Fri, Feb 17, 2017 at 9:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I just got bitten by a fallout.  I have
>
>     $ git recent --help
>     `git recent' is aliased to `log --oneline --branches --no-merges \
>          --source --since=3.weeks'
>
> but now the branch names are shown at the end, which defeats the
> whole point of the alias.

Yes, your situation actually wants those decorations as primary
things, so having them at the end is indeed pointless.

So I think we should just discard that patch of mine.

                 Linus

Re: [RFC PATCH] show decorations at the end of the line

[Jacob Keller]

On Sun, Feb 19, 2017 at 2:33 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Feb 17, 2017 at 9:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I just got bitten by a fallout.  I have
>>
>>     $ git recent --help
>>     `git recent' is aliased to `log --oneline --branches --no-merges \
>>          --source --since=3.weeks'
>>
>> but now the branch names are shown at the end, which defeats the
>> whole point of the alias.
>
> Yes, your situation actually wants those decorations as primary
> things, so having them at the end is indeed pointless.
>
> So I think we should just discard that patch of mine.
>
>                  Linus

I would think that in general putting them at the end makes more
sense, but we should have the ability to use them in format specifiers
so that users are free to customize it exactly how they want. That is,
I agree with the reasoning presented in the original patch, but think
Junio's case can be solved by strengthening the custom formats.

Thanks,
Jake

Re: [RFC PATCH] show decorations at the end of the line

[Jeff King]

On Sun, Feb 19, 2017 at 03:03:21PM -0800, Jacob Keller wrote:

> >> I just got bitten by a fallout.  I have
> >>
> >>     $ git recent --help
> >>     `git recent' is aliased to `log --oneline --branches --no-merges \
> >>          --source --since=3.weeks'
> >>
> >> but now the branch names are shown at the end, which defeats the
> >> whole point of the alias.
> >
> > Yes, your situation actually wants those decorations as primary
> > things, so having them at the end is indeed pointless.
> >
> > So I think we should just discard that patch of mine.
> >
> >                  Linus
> 
> I would think that in general putting them at the end makes more
> sense, but we should have the ability to use them in format specifiers
> so that users are free to customize it exactly how they want. That is,
> I agree with the reasoning presented in the original patch, but think
> Junio's case can be solved by strengthening the custom formats.

I think there are two potential patches:

  1. Add a custom-format placeholder for the --source value.
     This is an obvious improvement that doesn't hurt anyone.

  2. Switch --decorate to the end by default, but _not_ --source.

     This use case _could_ be served already by using a custom format
     with "%d". So it's really just a matter of having better-looking
     default.

     It might hurt somebody's script, but for the reasons discussed
     earlier in the thread, people are unlikely to be parsing it (it's
     more likely somebody would just complain because they think the
     decoration-first behavior is prettier).

-Peff

Re: [RFC PATCH] show decorations at the end of the line

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think there are two potential patches:
>
>   1. Add a custom-format placeholder for the --source value.
>      This is an obvious improvement that doesn't hurt anyone.
>
>   2. Switch --decorate to the end by default, but _not_ --source.
>
>      This use case _could_ be served already by using a custom format
>      with "%d". So it's really just a matter of having better-looking
>      default.

Yes, and I agree it is a better default to have "--decorate" at the
end.

I do not mind having to use a custom format myself, but I suspect
that the default for "--source" is more useful to have it at the
beginning, because "--source" annotates each and every commit, as
opposed to "--decorate" that adds annotation few and far between.

Re: [RFC PATCH] show decorations at the end of the line

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

On Sun, Feb 19, 2017 at 4:46 PM, Jeff King <peff@peff.net> wrote:
>
> I think there are two potential patches:
>
>   1. Add a custom-format placeholder for the --source value.
>      This is an obvious improvement that doesn't hurt anyone.

Right.

>   2. Switch --decorate to the end by default, but _not_ --source.

.. and in fact the whole "--source" printing should not even have been
mixed up with the decorations.

So (2) is actually easy to fix: just don't mix "show_source()" with
"show_decorations()", because they are totally different things to
begin with.

That source showing should never have been in "show_decorations()" in
the first place. It just happened to be a convenient place for it.

So this attached patch is just my original patch updated to split up
"show_source()" from "show_decorations()", and show it where it used
to be.

Maybe this works for Junio's alias?

              Linus

[-- Attachment #2: 0001-show-decorations-at-the-end-of-the-line.patch --]
[-- Type: text/x-patch, Size: 6152 bytes --]

From 97abab56fed451476f6ec676346b1e66001ef864 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 11 Feb 2017 10:03:33 -0800
Subject: [PATCH] show decorations at the end of the line

So I use "--show-decorations" all the time because I find it very useful
to see where the origin branch is, where tags are etc. In fact, my global
git config file has

    [log]
        decorate = auto

in it, so that I don't have to type it out all the time when I just do my
usual 'git log". It's lovely.

However, it does make one particular case uglier: with commit decorations,
the "oneline" commit format ends up being not very pretty:

    [torvalds@i7 git]$ git log --oneline -10
    3f07dac29 (HEAD -> master) pathspec: don't error out on  all-exclusionary pathspec patterns
    ca4a562f2 pathspec magic: add '^' as alias for '!'
    02555c1b2 ls-remote: add "--diff" option to show only refs that differ
    6e3a7b339 (tag: v2.12.0-rc0, origin/master, origin/HEAD) Git 2.12-rc0
    fafca0f72 Merge branch 'cw/log-updates-for-all-refs-really'
    74dee5cfa Merge branch 'pl/complete-diff-submodule-diff'
    36acf4123 Merge branch 'rs/object-id'
    ecc486b1f Merge branch 'js/re-running-failed-tests'
    4ba6bb2d1 Merge branch 'sb/submodule-update-initial-runs-custom-script'
    5348021c6 Merge branch 'sb/submodule-recursive-absorb'

and note how the decoration comes right after the shortened commit hash,
breaking up the alignment of the messages.

The above doesn't show it with the colorization: I also have

    [color]
        ui=auto

so on my terminal the decoration is also nicely colorized which makes it
much more obvious, it's not as obvious in this message.

The oneline message handling is already pretty special, this makes it even
more special by putting the decorations at the end of the line:

    3f07dac29 pathspec: don't error out on all-exclusionary pathspec patterns (HEAD -> master)
    ca4a562f2 pathspec magic: add '^' as alias for '!'
    02555c1b2 ls-remote: add "--diff" option to show only refs that differ
    6e3a7b339 Git 2.12-rc0 (tag: v2.12.0-rc0, origin/master, origin/HEAD)
    fafca0f72 Merge branch 'cw/log-updates-for-all-refs-really'
    74dee5cfa Merge branch 'pl/complete-diff-submodule-diff'
    36acf4123 Merge branch 'rs/object-id'
    ecc486b1f Merge branch 'js/re-running-failed-tests'
    4ba6bb2d1 Merge branch 'sb/submodule-update-initial-runs-custom-script'
    5348021c6 Merge branch 'sb/submodule-recursive-absorb'

which looks a lot better (again, this is all particularly noticeable with
colorization).

NOTE! There's a very special case for "git log --oneline -g" that shows
the reflogs as oneliners, and this does *not* fix that special case. It's
a lot more involved and relies on the exact show_reflog_message()
implementation, so I left the format for that alone, along with a comment
about how it's not at the end of line.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 builtin/rev-list.c |  1 +
 log-tree.c         | 17 ++++++++++++++---
 log-tree.h         |  1 +
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0aa93d589..8833f029a 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -107,6 +107,7 @@ static void show_commit(struct commit *commit, void *data)
 			children = children->next;
 		}
 	}
+	show_source(revs, commit);
 	show_decorations(revs, commit);
 	if (revs->commit_format == CMIT_FMT_ONELINE)
 		putchar(' ');
diff --git a/log-tree.c b/log-tree.c
index 8c2415747..9ca3e8c1c 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -279,12 +279,16 @@ void format_decorations_extended(struct strbuf *sb,
 	strbuf_addstr(sb, color_reset);
 }
 
+void show_source(struct rev_info *opt, struct commit *commit)
+{
+	if (opt->show_source && commit->util)
+		fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
+}
+
 void show_decorations(struct rev_info *opt, struct commit *commit)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	if (opt->show_source && commit->util)
-		fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
 	if (!opt->show_decorations)
 		return;
 	format_decorations(&sb, commit, opt->diffopt.use_color);
@@ -556,6 +560,7 @@ void show_log(struct rev_info *opt)
 			show_parents(commit, abbrev_commit, opt->diffopt.file);
 		if (opt->children.name)
 			show_children(opt, commit, abbrev_commit);
+		show_source(opt, commit);
 		show_decorations(opt, commit);
 		if (opt->graph && !graph_is_commit_finished(opt->graph)) {
 			putc('\n', opt->diffopt.file);
@@ -622,10 +627,14 @@ void show_log(struct rev_info *opt)
 			       find_unique_abbrev(parent->object.oid.hash,
 						  abbrev_commit));
 		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
-		show_decorations(opt, commit);
+		show_source(opt, commit);
 		if (opt->commit_format == CMIT_FMT_ONELINE) {
+			/* Not at end of line, but.. */
+			if (opt->reflog_info)
+				show_decorations(opt, commit);
 			putc(' ', opt->diffopt.file);
 		} else {
+			show_decorations(opt, commit);
 			putc('\n', opt->diffopt.file);
 			graph_show_oneline(opt->graph);
 		}
@@ -716,6 +725,8 @@ void show_log(struct rev_info *opt)
 		opt->missing_newline = 0;
 
 	graph_show_commit_msg(opt->graph, opt->diffopt.file, &msgbuf);
+	if (ctx.fmt == CMIT_FMT_ONELINE)
+		show_decorations(opt, commit);
 	if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
diff --git a/log-tree.h b/log-tree.h
index c8116e60c..33d415820 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -20,6 +20,7 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit,
 			     const char *suffix);
 #define format_decorations(strbuf, commit, color) \
 			     format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")
+void show_source(struct rev_info *opt, struct commit *commit);
 void show_decorations(struct rev_info *opt, struct commit *commit);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **subject_p,
-- 
2.12.0.rc2.4.g97abab56f

Re: [RFC PATCH] show decorations at the end of the line

[Junio C Hamano]

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

> That source showing should never have been in "show_decorations()" in
> the first place. It just happened to be a convenient place for it.
>
> So this attached patch is just my original patch updated to split up
> "show_source()" from "show_decorations()", and show it where it used
> to be.

The updated organization smells a lot better to me ;-) 

Most of the time it is convenient to have "show source" at the
beginning of a single helper that is to show both, but oneline
format is so special that it makes it inconvenient to have them at
the same place.

I can lose the patch to 4202 (update the expectation for --source)
I added to the previous one, but the patch to 4207 (update the
expectation for --decorate) needs to be kept with this round.

Will replace; thanks.

Re: [RFC PATCH] show decorations at the end of the line

[Linus Torvalds]

On Tue, Feb 21, 2017 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> The updated organization smells a lot better to me ;-)

So I have been using the original patch for a bit over a week now, and
I have to say that I'm not sure it's the right thing to do after all.

Most of the time I much prefer the "decorations at the end" thing,
because it just looks better, and the commit log oneliners line up
nicely.

But then occasionally I end up liking the old interface better, just
because there's long commit lines, and showing the decoration at the
end effectively hides it.

So I vacillate between the two formats, and so I'm not sure this patch
is worth the change in behavior after all.

In fact, I played around with some formats, and the one I lines the
most was actually one that split the line for decorations, but that
one was admittedly pretty funky. It gives output like

  b9df16a4c (HEAD -> master)
            pathspec: don't error out on all-exclusionary pathspec patterns
  91a491f05 pathspec magic: add '^' as alias for '!'
  c8e05fd6d ls-remote: add "--diff" option to show only refs that differ
  20769079d (tag: v2.12.0-rc2, origin/master, origin/HEAD)
            Git 2.12-rc2
  076c05393 Hopefully the final batch of mini-topics before the final
  c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
  62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
  1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'
  bf5f11918 Merge branch 'jk/reset-to-break-a-commit-doc'
  e048a257b Merge branch 'js/mingw-isatty'

(which looks better with colorization than it looks in the email).

But I'm not even going to send out that patch, because it was such an
atrocious hack to line things up.

              Linus

Re: [RFC PATCH] show decorations at the end of the line

[Jeff King]

On Tue, Feb 21, 2017 at 12:40:11PM -0800, Linus Torvalds wrote:

> In fact, I played around with some formats, and the one I lines the
> most was actually one that split the line for decorations, but that
> one was admittedly pretty funky. It gives output like
> 
>   b9df16a4c (HEAD -> master)
>             pathspec: don't error out on all-exclusionary pathspec patterns
>   91a491f05 pathspec magic: add '^' as alias for '!'
>   c8e05fd6d ls-remote: add "--diff" option to show only refs that differ
>   20769079d (tag: v2.12.0-rc2, origin/master, origin/HEAD)
>             Git 2.12-rc2
>   076c05393 Hopefully the final batch of mini-topics before the final
>   c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
>   62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
>   1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'
>   bf5f11918 Merge branch 'jk/reset-to-break-a-commit-doc'
>   e048a257b Merge branch 'js/mingw-isatty'
> 
> (which looks better with colorization than it looks in the email).
> 
> But I'm not even going to send out that patch, because it was such an
> atrocious hack to line things up.

I was going to suggest a custom format string that does the same, but
what we have is not _quite_ flexible enough.

You can use "%+d" to insert a newline only when "%d" is not empty. But
it always inserts _before_ the decoration, not after. Likewise, you
cannot say "if it's not empty, then insert %d and a leading tab".

The for-each-ref formatting code has %(if), but it's not unified with
the commit-format ones.

So the best I could come up with is:

  git config pretty.twoline '%C(auto)%h %s%C(auto)%+d'
  git log --format=twoline

which looks like:

  80ba04ed9 Merge branch 'svn-escape-backslash' of git://bogomips.org/git-svn
   (origin/master, origin/HEAD)
  20769079d Git 2.12-rc2
   (tag: v2.12.0-rc2)
  076c05393 Hopefully the final batch of mini-topics before the final
  c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
  62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
  1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'

-Peff

Re: [RFC PATCH] show decorations at the end of the line

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The for-each-ref formatting code has %(if), but it's not unified with
> the commit-format ones.
>
> So the best I could come up with is:
>
>   git config pretty.twoline '%C(auto)%h %s%C(auto)%+d'
>   git log --format=twoline
>
> which looks like:
>
>   80ba04ed9 Merge branch 'svn-escape-backslash' of git://bogomips.org/git-svn
>    (origin/master, origin/HEAD)
>   20769079d Git 2.12-rc2
>    (tag: v2.12.0-rc2)
>   076c05393 Hopefully the final batch of mini-topics before the final
>   c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
>   62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
>   1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'

Yeah, I had a similar thought to use something around "%n%-d", but

 $ git log --format='%h%n%-d%C(auto) %s %C(auto)'

is not it.

I guess we could pile on another hack to make the sign between % and
the format specifier cumulative and then "%n%-+d" may do what we
want, but we need a true %(if)...%(then)...%(else)...%(end) support
if we really want to do this thing properly.

Re: [RFC PATCH] show decorations at the end of the line

[Jeff King]

On Tue, Feb 21, 2017 at 01:47:37PM -0800, Junio C Hamano wrote:

> > So the best I could come up with is:
> >
> >   git config pretty.twoline '%C(auto)%h %s%C(auto)%+d'
> >   git log --format=twoline
> > [...]
>
> Yeah, I had a similar thought to use something around "%n%-d", but
> 
>  $ git log --format='%h%n%-d%C(auto) %s %C(auto)'
> 
> is not it.
> 
> I guess we could pile on another hack to make the sign between % and
> the format specifier cumulative and then "%n%-+d" may do what we
> want, but we need a true %(if)...%(then)...%(else)...%(end) support
> if we really want to do this thing properly.

Yeah, I'd rather not pile up more hacks. The for-each-ref placeholders
are more verbose, but I think the end result is a lot easier to read and
maintain, and the terseness doesn't matter if you're sticking it behind
an alias or config option.

(Don't get me wrong; I think the %(if) ones are pretty ugly, too, but
the next step beyond that is embedding some kind of templating or
scripting language, and that just seems like overkill).

-Peff

Re: [RFC PATCH] show decorations at the end of the line

[Stephan Beyer]

Hi,

On 02/13/2017 09:30 AM, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
>> On Sat, Feb 11, 2017 at 10:02 AM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>>
>>> I've signed off on this, because I think it's an "obvious" improvement,
>>> but I'm putting the "RFC" in the subject line because this is clearly a
>>> subjective thing.
>>
>> Side note: the one downside of showing the decorations at the end of
>> the line is that now they are obviously at the end of the line - and
>> thus likely to be more hidden by things like line truncation.
> 
> Side note: I refrained from commenting on this patch because
> everybody knows that the what I would say anyway ;-) and I didn't
> want to speak first to discourage others from raising their opinion.

A further side note: the current behavior of

	git log --oneline --decorate

is equivalent to

	git log --pretty='format:%C(auto)%h%d %s'

and Linus' preferred version is equivalent to

	git log --pretty='format:%C(auto)%h %s%d'

Most Git users I know have their own favorite version of git log
--pretty=format:... sometimes with --graph as an alias ("git lg" or "git
logk" (because its output reminds of gitk) or something).

I don't know what the main benefit of this patch would be, but if it
gets accepted, it should probably be mentioned somewhere that the old
behavior is easily accessible using the line mentioned above.

Cheers
Stephan