From: Marc Branchaud <marcnarc@xiplink.com>
Date: Mon, 25 Mar 2024 12:52:00 GMT
Subject: Re: [PATCH] gitk: add "Hightlight commit name" menu entry
Message-ID: <742ac151-74cd-46f8-8dfb-3dcdaa513314@xiplink.com>
In-Reply-To: <146fbb87-9047-4d96-8d06-76f5710ca9e1@gmail.com>


On 2024-03-23 05:45, Raphaël Gallais-Pou wrote:
> 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.

Thanks for clarifying that.

(BTW, on this list we pronounce "the name of the commit" as "the commit 
subject (line)".)

> 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 ?

Absolutely.  Maybe "git cherry" would be more appropriate?

		M.


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