From: Marc Branchaud <marcnarc@xiplink.com>
Date: Thu, 21 Mar 2024 15:51:33 GMT
Subject: Re: [PATCH] gitk: add "Hightlight commit name" menu entry
Message-ID: <27b9c158-5cb1-46bf-851a-88a02448fa2d@xiplink.com>
In-Reply-To: <20240130085308.5440-1-rgallaispou@gmail.com>


On 2024-01-30 03:53, Raphael Gallais-Pou wrote:
> When working with diverged branches, some patches can appear several times
> on different branches without having the need to merge those branches.
> On the other hand you may have to port a specific patch on another
> branch you are working on. The search with a SHA1 cannot be applied here
> since they would differ.
> 
> This patch adds an entry in the main context menu to highlight every
> instance of a commit.

Thanks for working on gitk!

Unfortunately, I don't understand the description of your new option. 
How is this different from the existing "Find containing:" feature? 
Gitk can already highlights commits that match a specified string. 
Please explain what gitk does when this new option is selected.

Also, please explain how your code identifies "every instance" of a 
commit.  When I think of a "commit instance" I think of the
"git patch-id" command, which I don't see here.

(It looks to me like this is basically a shortcut to auto-fill gitk's 
"containing:" field with the subject line of the selected commit?)

> Signed-off-by: Raphael Gallais-Pou <rgallaispou@gmail.com>
> ---
>   gitk-git/gitk | 23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 7a087f123d..4b15230a16 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -2672,6 +2672,7 @@ proc makewindow {} {
>           {mc "Make patch" command mkpatch}
>           {mc "Create tag" command mktag}
>           {mc "Copy commit reference" command copyreference}
> +	{mc "Highlight commit name" command highlightcommitname}

This line is indented with a tab, but it should use spaces.

>           {mc "Write commit to file" command writecommit}
>           {mc "Create new branch" command mkbranch}
>           {mc "Cherry-pick this commit" command cherrypick}
> @@ -9002,13 +9003,13 @@ proc rowmenu {x y id} {
>       if {$id ne $nullid && $id ne $nullid2} {
>           set menu $rowctxmenu
>           if {$mainhead ne {}} {
> -            $menu entryconfigure 8 -label [mc "Reset %s branch to here" $mainhead] -state normal
> +            $menu entryconfigure 9 -label [mc "Reset %s branch to here" $mainhead] -state normal
>           } else {
> -            $menu entryconfigure 8 -label [mc "Detached head: can't reset" $mainhead] -state disabled
> +            $menu entryconfigure 9 -label [mc "Detached head: can't reset" $mainhead] -state disabled
>           }
> -        $menu entryconfigure 10 -state $mstate
>           $menu entryconfigure 11 -state $mstate
>           $menu entryconfigure 12 -state $mstate
> +        $menu entryconfigure 13 -state $mstate
>       } else {
>           set menu $fakerowmenu
>       }
> @@ -9481,6 +9482,22 @@ proc copyreference {} {
>       clipboard append $reference
>   }
>   
> +proc highlightcommitname {} {
> +    global rowmenuid autosellen findstring gdttype
> +
> +    set format "%s"
> +    set cmd [list git show -s --pretty=format:$format --date=short]

Why bother with the $format variable here?  Couldn't you just quote the 
--pretty part?
	"--pretty=format:%s"
(FYI, I am not a TCL/TK coder.)

		M.

> +    if {$autosellen < 40} {
> +        lappend cmd --abbrev=$autosellen
> +    }
> +    set reference [eval exec $cmd $rowmenuid]
> +    set findstring $reference
> +    set gdttype [mc "containing:"]
> +
> +    clipboard clear
> +    clipboard append $reference
> +}
> +
>   proc writecommit {} {
>       global rowmenuid wrcomtop commitinfo wrcomcmd NS
>