From: Raphaël Gallais-Pou <rgallaispou@gmail.com>
Date: Sat, 23 Mar 2024 09:45:14 GMT
Subject: Re: [PATCH] gitk: add "Hightlight commit name" menu entry
Message-ID: <146fbb87-9047-4d96-8d06-76f5710ca9e1@gmail.com>
In-Reply-To: <27b9c158-5cb1-46bf-851a-88a02448fa2d@xiplink.com>

Hi Marc,

Le 21/03/2024 à 16:51, Marc Branchaud a écrit :
> 
> 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 is based on the name of the commit. I agree that it is not ideal 
since the name can change between two versions.

As you stated below it is exactly a shortcut, and now that we are 
talking about it I think this is not the right right approach to do what 
I want.

I was not aware of the 'git patch-id' command, but this it clearly a 
better idea to base the search of the commit instances on it.

One thing that I wonder is that 'git patch-id' seems to be based on 
standard input. This means that in order to highlight every instances of 
a commit the algorithm would need to parse each and every patch and then 
proceed to hash them and compare to the one referenced.

Wouldn't it be a tad long to process ?

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

I will change my setup to use spaces instead of tabs one this file.

> 
>>           {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.)

I also am not a TCL developer. I pretty much duplicated the 
copyreference{} procedure to get what I wanted.

Best regards,
Raphaël
> 
>          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