[PATCH] gitk: use --pretty=reference for copysummary

[PATCH] gitk: use --pretty=reference for copysummary

[Denton Liu]

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(-)

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
     }
-- 
2.24.0.627.geba02921db

Re: [PATCH] gitk: use --pretty=reference for copysummary

[Paul Mackerras]

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.

Re: [PATCH] gitk: use --pretty=reference for copysummary

[Junio C Hamano]

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 v2 0/2] gitk: match Git's 'reference' pretty format

[Denton Liu]

Recently, the 'reference' pretty format was implemented in Git, which
has the same function as the 'Copy commit summary' feature in gitk.
Since Git now has a canonical way of doing this, update gitk to match
this.

Beat Bolli (1):
  gitk: rename "commit summary" to "commit reference"

Denton Liu (1):
  gitk: drop quotes in copysummary format

 gitk | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.24.1.664.g198078bb5a

[PATCH v2 1/2] gitk: drop quotes in copysummary format

[Denton Liu]

In an earlier commit[1], git learned the 'reference' pretty format. This
format was very similar to the format that copysummary used but it omits
the quotes surrounding the commit's subject. Remove the quotes from the
format in copysummary in order to match the 'reference' pretty format.

It seems the consensus is that the unquoted form is used more often
anyway[2] so this change should be acceptable.

[These commits are in git.git.]
[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>
---
 gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH v2 1/2] gitk: drop quotes in copysummary format

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> In an earlier commit[1], git learned the 'reference' pretty format. This
> format was very similar to the format that copysummary used but it omits
> the quotes surrounding the commit's subject. Remove the quotes from the
> format in copysummary in order to match the 'reference' pretty format.
>
> It seems the consensus is that the unquoted form is used more often
> anyway[2] so this change should be acceptable.
>
> [These commits are in git.git.]
> [1]: 1f0fc1db85 (pretty: implement 'reference' format, 2019-11-19)
> [2]: fb2ffa77a6 (SubmittingPatches: remove dq from commit reference, 2019-11-19)

This change to gitk actually can easily be controversial.

The "used more often" census IIUC was done on _this_ project, and
SubmittingPatches is merely a guideline to contribute to _this_
project, but the audience of gitk that wants to use the copysummary
format is a lot wider than that.

This patch alone may not be worth sinking the time on for that
reason.

Having said that, if we teach --pretty=reference to optionally use a
custom format via a configuration variable, it would make quite a
lot of sense to do an update _like_ this patch does to gitk.  It
would probably allow the code to read from the same configuration
variable, instead of replacing a hardcoded format string with
another hardcoded format string, though.

Thanks.

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  gitk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gitk b/gitk
> index abe4805ade..d07e3302de 100755
> --- a/gitk
> +++ b/gitk
> @@ -9429,7 +9429,7 @@ proc mktaggo {} {
>  proc copysummary {} {
>      global rowmenuid autosellen
>  
> -    set format "%h (\"%s\", %ad)"
> +    set format "%h (%s, %ad)"
>      set cmd [list git show -s --pretty=format:$format --date=short]
>      if {$autosellen < 40} {
>          lappend cmd --abbrev=$autosellen

Re: [PATCH v2 1/2] gitk: drop quotes in copysummary format

[Paul Mackerras]

On Thu, Dec 12, 2019 at 04:44:49PM -0800, Denton Liu wrote:
> In an earlier commit[1], git learned the 'reference' pretty format. This
> format was very similar to the format that copysummary used but it omits
> the quotes surrounding the commit's subject. Remove the quotes from the
> format in copysummary in order to match the 'reference' pretty format.
> 
> It seems the consensus is that the unquoted form is used more often
> anyway[2] so this change should be acceptable.

I don't see any discussion about removing the quotes in the Linux
kernel community, and the Documentation/process/submitting-patches.rst
in the kernel tree says (or at least implies) that the quotes are
required.  So I am not convinced this change is a good thing to do.

In fact the gitk commit summary/reference format doesn't match what
the kernel wants exactly, in that gitk puts in the author-date, and
submitting-patches.rst doesn't have it.  There are maintainers that
have scripts that automatically check the format of Fixes: lines in
submaintainers' trees and send email if they detect one with the wrong
formatting.  Thus I would find it more convenient to change the format
to %h (\"s\") than to "%h (%s, %ad)".

Maybe the solution is to make the format customizable.

> [These commits are in git.git.]
> [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>
> ---
>  gitk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gitk b/gitk
> index abe4805ade..d07e3302de 100755
> --- a/gitk
> +++ b/gitk
> @@ -9429,7 +9429,7 @@ proc mktaggo {} {
>  proc copysummary {} {
>      global rowmenuid autosellen
>  
> -    set format "%h (\"%s\", %ad)"
> +    set format "%h (%s, %ad)"
>      set cmd [list git show -s --pretty=format:$format --date=short]
>      if {$autosellen < 40} {
>          lappend cmd --abbrev=$autosellen
> -- 
> 2.24.1.664.g198078bb5a

Paul.

[PATCH v2 2/2] gitk: rename "commit summary" to "commit reference"

[Denton Liu]

From: Beat Bolli <dev+git@drbeat.li>

Now that the commit reference format has a canonical name, let's use this
name in gitk's UI and implementation.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
[dl: based the patch on gitk's tree]
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 gitk | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gitk b/gitk
index d07e3302de..17346b4c5c 100755
--- a/gitk
+++ b/gitk
@@ -2640,7 +2640,7 @@ proc makewindow {} {
 	{mc "Diff selected -> this" command {diffvssel 1}}
 	{mc "Make patch" command mkpatch}
 	{mc "Create tag" command mktag}
-	{mc "Copy commit summary" command copysummary}
+	{mc "Copy commit reference" command copyreference}
 	{mc "Write commit to file" command writecommit}
 	{mc "Create new branch" command mkbranch}
 	{mc "Cherry-pick this commit" command cherrypick}
@@ -9426,7 +9426,7 @@ proc mktaggo {} {
     mktagcan
 }
 
-proc copysummary {} {
+proc copyreference {} {
     global rowmenuid autosellen
 
     set format "%h (%s, %ad)"
@@ -9434,10 +9434,10 @@ proc copysummary {} {
     if {$autosellen < 40} {
         lappend cmd --abbrev=$autosellen
     }
-    set summary [eval exec $cmd $rowmenuid]
+    set reference [eval exec $cmd $rowmenuid]
 
     clipboard clear
-    clipboard append $summary
+    clipboard append $reference
 }
 
 proc writecommit {} {
-- 
2.24.1.664.g198078bb5a

Re: [PATCH v2 2/2] gitk: rename "commit summary" to "commit reference"

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> From: Beat Bolli <dev+git@drbeat.li>
>
> Now that the commit reference format has a canonical name, let's use this
> name in gitk's UI and implementation.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> [dl: based the patch on gitk's tree]
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  gitk | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I think this one makes sense, even though I am on the fence about
patch 1/2.

Thanks.


>
> diff --git a/gitk b/gitk
> index d07e3302de..17346b4c5c 100755
> --- a/gitk
> +++ b/gitk
> @@ -2640,7 +2640,7 @@ proc makewindow {} {
>  	{mc "Diff selected -> this" command {diffvssel 1}}
>  	{mc "Make patch" command mkpatch}
>  	{mc "Create tag" command mktag}
> -	{mc "Copy commit summary" command copysummary}
> +	{mc "Copy commit reference" command copyreference}
>  	{mc "Write commit to file" command writecommit}
>  	{mc "Create new branch" command mkbranch}
>  	{mc "Cherry-pick this commit" command cherrypick}
> @@ -9426,7 +9426,7 @@ proc mktaggo {} {
>      mktagcan
>  }
>  
> -proc copysummary {} {
> +proc copyreference {} {
>      global rowmenuid autosellen
>  
>      set format "%h (%s, %ad)"
> @@ -9434,10 +9434,10 @@ proc copysummary {} {
>      if {$autosellen < 40} {
>          lappend cmd --abbrev=$autosellen
>      }
> -    set summary [eval exec $cmd $rowmenuid]
> +    set reference [eval exec $cmd $rowmenuid]
>  
>      clipboard clear
> -    clipboard append $summary
> +    clipboard append $reference
>  }
>  
>  proc writecommit {} {

Re: [PATCH v2 2/2] gitk: rename "commit summary" to "commit reference"

[Paul Mackerras]

On Thu, Dec 12, 2019 at 04:44:50PM -0800, Denton Liu wrote:
> From: Beat Bolli <dev+git@drbeat.li>
> 
> Now that the commit reference format has a canonical name, let's use this
> name in gitk's UI and implementation.
> 
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> [dl: based the patch on gitk's tree]
> Signed-off-by: Denton Liu <liu.denton@gmail.com>

Thanks, patch applied.

Paul.

Re: [PATCH v2 0/2] gitk: match Git's 'reference' pretty format

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> Recently, the 'reference' pretty format was implemented in Git, which
> has the same function as the 'Copy commit summary' feature in gitk.
> Since Git now has a canonical way of doing this, update gitk to match
> this.

As you no longer use "--pretty=reference", the above has become a
stale description.

I think the two patches do make sense, though.

Thanks.


>
> Beat Bolli (1):
>   gitk: rename "commit summary" to "commit reference"
>
> Denton Liu (1):
>   gitk: drop quotes in copysummary format
>
>  gitk | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)