threads / patch / 52434

patchgitk: use --pretty=reference for copysummary

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

## tl;dr

11 messages between Dec 11, 2019 and Dec 15, 2019. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Denton Liu· Dec 11, 2019, 21:39 UTC · lore

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(-)
Show changes to gitk +1 −2
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
Paul Mackerras· Dec 11, 2019, 21:58 UTC · re: Denton Liu · lore

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

On Wed, Dec 11, 2019 at 01:39:50PM -0800, Denton Liu wrote:
Show 12 quoted lines
> 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, 22:05 UTC · re: Paul Mackerras · lore

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

Paul Mackerras <paulus@ozlabs.org> writes:
Show 18 quoted lines
> 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.
Denton Liu· Dec 13, 2019, 00:44 UTC · re: Denton Liu · lore

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

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
Denton Liu· Dec 13, 2019, 00:44 UTC · re: Denton Liu · lore

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

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(-)
Show changes to gitk +1 −1
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
Junio C Hamano· Dec 13, 2019, 17:12 UTC · re: Denton Liu · lore

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

Denton Liu <liu.denton@gmail.com> writes:
Show 11 quoted lines
> 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.
Show 18 quoted lines
> 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
Paul Mackerras· Dec 15, 2019, 04:19 UTC · re: Denton Liu · lore

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

On Thu, Dec 12, 2019 at 04:44:49PM -0800, Denton Liu wrote:
Show 7 quoted lines
> 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.
Show 24 quoted lines
> [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.
Denton Liu· Dec 13, 2019, 00:44 UTC · re: Denton Liu · lore

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

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(-)
Show changes to gitk +4 −4
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
Junio C Hamano· Dec 13, 2019, 17:12 UTC · re: Denton Liu · lore

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

Denton Liu <liu.denton@gmail.com> writes:
Show 11 quoted lines
> 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.
Show 36 quoted lines
>
> 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 {} {
Paul Mackerras· Dec 15, 2019, 04:36 UTC · re: Denton Liu · lore

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

On Thu, Dec 12, 2019 at 04:44:50PM -0800, Denton Liu wrote:
Show 8 quoted lines
> 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.
Junio C Hamano· Dec 13, 2019, 17:04 UTC · re: Denton Liu · lore

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

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.
Show 9 quoted lines
>
> 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(-)

← back to recent threads