gitk: use --pretty=reference for copysummary
diff mbox series

Message ID da9321b1bd56aafd16c8dcb99d5d628b79e2244e.1576100147.git.liu.denton@gmail.com
State New
Headers show
Series
  • gitk: use --pretty=reference for copysummary
Related show

Commit Message

Denton Liu Dec. 11, 2019, 9:39 p.m. UTC
In an earlier commit[1], git learned the 'reference' pretty format.
Update copysummary to use this pretty format instead of manually
reimplementing it as a format string.

With this change, we lose the double-quotes surrounding the commit
subject but it seems the consensus is that the unquoted form is used
more often anyway[2] so this change should be acceptable.

Since gitk and git are usually packaged and distributed together, their
versions should be in sync so we should not have to worry a newer gitk
running on top of an older version of git that doesn't support the
'reference' pretty format.

[1]: 1f0fc1db85 (pretty: implement 'reference' format, 2019-11-19)
[2]: fb2ffa77a6 (SubmittingPatches: remove dq from commit reference, 2019-11-19)

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Beat Bolli sent a series out earlier that did the exact same thing[3].
Since they haven't replied yet, I'll send out the version that I've been
cooking for a while now since I think the commit message looks a bit
better too and also it's based on top of Paul's tree.

[3]: https://lore.kernel.org/git/20191209182534.309884-1-dev+git@drbeat.li/

 gitk | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Paul Mackerras Dec. 11, 2019, 9:58 p.m. UTC | #1
On Wed, Dec 11, 2019 at 01:39:50PM -0800, Denton Liu wrote:
> In an earlier commit[1], git learned the 'reference' pretty format.
> Update copysummary to use this pretty format instead of manually
> reimplementing it as a format string.
> 
> With this change, we lose the double-quotes surrounding the commit
> subject but it seems the consensus is that the unquoted form is used
> more often anyway[2] so this change should be acceptable.
> 
> Since gitk and git are usually packaged and distributed together, their
> versions should be in sync so we should not have to worry a newer gitk
> running on top of an older version of git that doesn't support the
> 'reference' pretty format.

In fact my policy is not to do this (introduce a change to gitk that
means it requires the very latest git).  I would want the code either
to test the git version (which the code already does in other places)
or handle failure gracefully and fall back to the old command.

Paul.
Junio C Hamano Dec. 11, 2019, 10:05 p.m. UTC | #2
Paul Mackerras <paulus@ozlabs.org> writes:

> On Wed, Dec 11, 2019 at 01:39:50PM -0800, Denton Liu wrote:
>> In an earlier commit[1], git learned the 'reference' pretty format.
>> Update copysummary to use this pretty format instead of manually
>> reimplementing it as a format string.
>> 
>> With this change, we lose the double-quotes surrounding the commit
>> subject but it seems the consensus is that the unquoted form is used
>> more often anyway[2] so this change should be acceptable.
>> 
>> Since gitk and git are usually packaged and distributed together, their
>> versions should be in sync so we should not have to worry a newer gitk
>> running on top of an older version of git that doesn't support the
>> 'reference' pretty format.
>
> In fact my policy is not to do this (introduce a change to gitk that
> means it requires the very latest git).  I would want the code either
> to test the git version (which the code already does in other places)
> or handle failure gracefully and fall back to the old command.

For a case like this one, the policy would mean that a single liner
patch like this will never be accepted, right?  After all, the code
that would be used as a fallback for older Git is very simple so it
is almost pointless to add a check for feature with conditional.
We can just use the fallback code always, which is essentially to
keep the current code.

It is a tangent, but arguably the current code is easier to extend.
I can even see somebody arguing for adding a gitk.summaryformat
configuration variable, whose value would default to "%h (%s, %ad)"
when missing---that can be quite straightforward to do without
Denton's patch.

So I dunno.

Patch
diff mbox series

diff --git a/gitk b/gitk
index abe4805ade..8bf198e338 100755
--- a/gitk
+++ b/gitk
@@ -9429,8 +9429,7 @@  proc mktaggo {} {
 proc copysummary {} {
     global rowmenuid autosellen
 
-    set format "%h (\"%s\", %ad)"
-    set cmd [list git show -s --pretty=format:$format --date=short]
+    set cmd [list git show -s --pretty=reference]
     if {$autosellen < 40} {
         lappend cmd --abbrev=$autosellen
     }