[PATCH] Gitk Inotify support

[PATCH] Gitk Inotify support

[Florian Schüller]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

Hi
Is this correct to send possible gitk patches here? or should I send
them to Paul Mackerras somehow?

Anyway I just wanted that gitk automatically updates while I'm working
in my terminal

Are you interrested?

as described in "SubmittingPatches" this patch is based on
git://ozlabs.org/~paulus/gitk   22a713c72df8b6799c59287c50cee44c4a6db51e

The code should be robust to just don't autoupdate if
https://sourceforge.net/projects/tcl-inotify/ is not installed

Open points for now:
 - release watches for deleted directories seems to cause problems in
tcl-inotify (so I don't)
   I'm not sure how often that happens in ".git/"
 - I only call "updatecommits" and I don't know if there is a usecase
where I should be calling "reloadcommits"

Regards
Florian Schüller

[-- Attachment #2: 0001-first-support-for-inotify.patch --]
[-- Type: text/x-patch, Size: 2439 bytes --]

From 785ed6bc1b4a3b9019d3503b066afb2a025a2bc1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Florian=20Sch=C3=BCller?= <florian.schueller@gmail.com>
Date: Thu, 9 Jun 2016 22:54:43 +0200
Subject: [PATCH] first support for inotify

---
 gitk | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/gitk b/gitk
index 805a1c7..6e2ead2 100755
--- a/gitk
+++ b/gitk
@@ -8,6 +8,12 @@ exec wish "$0" -- "$@"
 # either version 2, or (at your option) any later version.
 
 package require Tk
+try {
+    package require inotify
+    set we_have_inotify true
+} on error {em} {
+    set we_have_inotify false
+}
 
 proc hasworktree {} {
     return [expr {[exec git rev-parse --is-bare-repository] == "false" &&
@@ -12363,6 +12369,59 @@ if {$i >= [llength $argv] && $revtreeargs ne {}} {
     }
 }
 
+proc inotify_handler { fd } {
+    set events [inotify_watch read]
+    set watch_info [inotify_watch info]
+    set update_view false
+
+    foreach {event} $events {
+        set current_watchid [dict get $event watchid]
+        set flags [dict get $event flags]
+        set event_filename [dict get $event filename]
+
+        foreach {path watchid watch_flags} $watch_info {
+            if {$watchid eq $current_watchid} {
+                set watch_path $path
+            }
+        }
+
+        set full_filename [file join $watch_path $event_filename]
+
+#        remove does not seem to work
+#        if {$flags eq "s"} {
+#            puts "Remove watch $full_filename"
+#            set wd [inotify_watch remove $full_filename]
+#        }
+
+        if {$flags eq "nD"} {
+            set wd [inotify_watch add $full_filename "nwds"]
+        }
+        if {![string match *.lock $event_filename]} {
+            set update_view true
+        }
+    }
+
+    #reloadcommits or updatecommits - depending on file and operation?
+    if {$update_view} {
+        updatecommits
+    }
+}
+
+proc watch_recursive { dir } {
+    inotify_watch add $dir "nwaCmMds"
+
+    foreach i [glob -nocomplain -dir $dir *] {
+        if {[file type $i] eq {directory}} {
+            watch_recursive $i
+        }
+    }
+}
+
+if { $we_have_inotify } {
+    set fd [inotify create "inotify_watch" "::inotify_handler"]
+    watch_recursive $gitdir
+}
+
 set nullid "0000000000000000000000000000000000000000"
 set nullid2 "0000000000000000000000000000000000000001"
 set nullfile "/dev/null"
-- 
2.7.4

Re: [PATCH] Gitk Inotify support

[Stefan Beller]

On Thu, Jun 9, 2016 at 2:12 PM, Florian Schüller
<florian.schueller@gmail.com> wrote:
> Hi
> Is this correct to send possible gitk patches here? or should I send
> them to Paul Mackerras somehow?

I cc'd Paul for you :)

>
> Anyway I just wanted that gitk automatically updates while I'm working
> in my terminal

Thanks for coming up with a patch. Welcome to the Git community!

>
> Are you interrested?
>

Also see the section that talks about signing off the patch and how to
send the patch
inline. :)


> From 785ed6bc1b4a3b9019d3503b066afb2a025a2bc1 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Florian=20Sch=C3=BCller?= <florian.schueller@gmail.com>
> Date: Thu, 9 Jun 2016 22:54:43 +0200
> Subject: [PATCH] first support for inotify

Here you should describe your change, i.e. what problem is solved in this patch,
what are the alternatives, why is this way the best? Also the sign off
goes here.

>
---
>  gitk | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>
diff --git a/gitk b/gitk
> index 805a1c7..6e2ead2 100755
> --- a/gitk
> +++ b/gitk
> @@ -8,6 +8,12 @@ exec wish "$0" -- "$@"
>  # either version 2, or (at your option) any later version.
>
>  package require Tk
> +try {
> +    package require inotify
> +    set we_have_inotify true
> +} on error {em} {
> +    set we_have_inotify false
> +}

There are quite a few "have_*" variables, so I would drop the leading "we_"

>
>  proc hasworktree {} {
>      return [expr {[exec git rev-parse --is-bare-repository] == "false" &&
> @@ -12363,6 +12369,59 @@ if {$i >= [llength $argv] && $revtreeargs ne {}} {
>      }
>  }
>
> +proc inotify_handler { fd } {
> +    set events [inotify_watch read]
> +    set watch_info [inotify_watch info]
> +    set update_view false
> +
> +    foreach {event} $events {
> +        set current_watchid [dict get $event watchid]
> +        set flags [dict get $event flags]
> +        set event_filename [dict get $event filename]
> +
> +        foreach {path watchid watch_flags} $watch_info {
> +            if {$watchid eq $current_watchid} {
> +                set watch_path $path
> +            }
> +        }
> +
> +        set full_filename [file join $watch_path $event_filename]
> +
> +#        remove does not seem to work
> +#        if {$flags eq "s"} {
> +#            puts "Remove watch $full_filename"
> +#            set wd [inotify_watch remove $full_filename]
> +#        }

Why do we want to carry commented code? I'd drop that.

> +
> +        if {$flags eq "nD"} {
> +            set wd [inotify_watch add $full_filename "nwds"]
> +        }
> +        if {![string match *.lock $event_filename]} {
> +            set update_view true
> +        }
> +    }
> +
> +    #reloadcommits or updatecommits - depending on file and operation?
> +    if {$update_view} {
> +        updatecommits
> +    }
> +}
> +
> +proc watch_recursive { dir } {
> +    inotify_watch add $dir "nwaCmMds"
> +
> +    foreach i [glob -nocomplain -dir $dir *] {
> +        if {[file type $i] eq {directory}} {
> +            watch_recursive $i
> +        }
> +    }
> +}
> +
> +if { $we_have_inotify } {
> +    set fd [inotify create "inotify_watch" "::inotify_handler"]
> +    watch_recursive $gitdir
> +}
> +
>  set nullid "0000000000000000000000000000000000000000"
>  set nullid2 "0000000000000000000000000000000000000001"
>  set nullfile "/dev/null"
> --
> 2.7.4
>

Thanks,
Stefan

Re: [PATCH] Gitk Inotify support

[Florian Schüller]

From 74d2f4c1ec560b358fb50b8b7fe8282e7e1457b0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Florian=20Sch=C3=BCller?= <florian.schueller@gmail.com>
Date: Thu, 9 Jun 2016 22:54:43 +0200
Subject: [PATCH] first support for inotify
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Just automatically update gitk when working in a terminal on the same repo

Open points for now:
 - release watches for deleted directories seems to
   cause problems in tcl-inotify (so I don't)
   I'm not sure how often that happens in ".git/"
 - I only call "updatecommits" and I don't know if there is a usecase
   where I should be calling "reloadcommits"

Signed-off-by: Florian Schüller <florian.schueller@gmail.com>
---
 gitk | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/gitk b/gitk
index 805a1c7..58e3dca 100755
--- a/gitk
+++ b/gitk
@@ -8,6 +8,12 @@ exec wish "$0" -- "$@"
 # either version 2, or (at your option) any later version.

 package require Tk
+try {
+    package require inotify
+    set have_inotify true
+} on error {em} {
+    set have_inotify false
+}

 proc hasworktree {} {
     return [expr {[exec git rev-parse --is-bare-repository] == "false" &&
@@ -12363,6 +12369,53 @@ if {$i >= [llength $argv] && $revtreeargs ne {}} {
     }
 }

+proc inotify_handler { fd } {
+    set events [inotify_watch read]
+    set watch_info [inotify_watch info]
+    set update_view false
+
+    foreach {event} $events {
+        set current_watchid [dict get $event watchid]
+        set flags [dict get $event flags]
+        set event_filename [dict get $event filename]
+
+        foreach {path watchid watch_flags} $watch_info {
+            if {$watchid eq $current_watchid} {
+                set watch_path $path
+            }
+        }
+
+        set full_filename [file join $watch_path $event_filename]
+
+        if {$flags eq "nD"} {
+            set wd [inotify_watch add $full_filename "nwds"]
+        }
+        if {![string match *.lock $event_filename]} {
+            set update_view true
+        }
+    }
+
+    #reloadcommits or updatecommits - depending on file and operation?
+    if {$update_view} {
+        updatecommits
+    }
+}
+
+proc watch_recursive { dir } {
+    inotify_watch add $dir "nwaCmMds"
+
+    foreach i [glob -nocomplain -dir $dir *] {
+        if {[file type $i] eq {directory}} {
+            watch_recursive $i
+        }
+    }
+}
+
+if { $have_inotify } {
+    set fd [inotify create "inotify_watch" "::inotify_handler"]
+    watch_recursive $gitdir
+}
+
 set nullid "0000000000000000000000000000000000000000"
 set nullid2 "0000000000000000000000000000000000000001"
 set nullfile "/dev/null"
-- 
2.7.4

On Thu, Jun 9, 2016 at 11:24 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Jun 9, 2016 at 2:12 PM, Florian Schüller
> <florian.schueller@gmail.com> wrote:
>> Hi
>> Is this correct to send possible gitk patches here? or should I send
>> them to Paul Mackerras somehow?
>
> I cc'd Paul for you :)
>
>>
>> Anyway I just wanted that gitk automatically updates while I'm working
>> in my terminal
>
> Thanks for coming up with a patch. Welcome to the Git community!
>
>>
>> Are you interrested?
>>
>
> Also see the section that talks about signing off the patch and how to
> send the patch
> inline. :)
>
>
>> From 785ed6bc1b4a3b9019d3503b066afb2a025a2bc1 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Florian=20Sch=C3=BCller?= <florian.schueller@gmail.com>
>> Date: Thu, 9 Jun 2016 22:54:43 +0200
>> Subject: [PATCH] first support for inotify
>
> Here you should describe your change, i.e. what problem is solved in this patch,
> what are the alternatives, why is this way the best? Also the sign off
> goes here.
>
>>
> ---
>>  gitk | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>>
> diff --git a/gitk b/gitk
>> index 805a1c7..6e2ead2 100755
>> --- a/gitk
>> +++ b/gitk
>> @@ -8,6 +8,12 @@ exec wish "$0" -- "$@"
>>  # either version 2, or (at your option) any later version.
>>
>>  package require Tk
>> +try {
>> +    package require inotify
>> +    set we_have_inotify true
>> +} on error {em} {
>> +    set we_have_inotify false
>> +}
>
> There are quite a few "have_*" variables, so I would drop the leading "we_"
>
>>
>>  proc hasworktree {} {
>>      return [expr {[exec git rev-parse --is-bare-repository] == "false" &&
>> @@ -12363,6 +12369,59 @@ if {$i >= [llength $argv] && $revtreeargs ne {}} {
>>      }
>>  }
>>
>> +proc inotify_handler { fd } {
>> +    set events [inotify_watch read]
>> +    set watch_info [inotify_watch info]
>> +    set update_view false
>> +
>> +    foreach {event} $events {
>> +        set current_watchid [dict get $event watchid]
>> +        set flags [dict get $event flags]
>> +        set event_filename [dict get $event filename]
>> +
>> +        foreach {path watchid watch_flags} $watch_info {
>> +            if {$watchid eq $current_watchid} {
>> +                set watch_path $path
>> +            }
>> +        }
>> +
>> +        set full_filename [file join $watch_path $event_filename]
>> +
>> +#        remove does not seem to work
>> +#        if {$flags eq "s"} {
>> +#            puts "Remove watch $full_filename"
>> +#            set wd [inotify_watch remove $full_filename]
>> +#        }
>
> Why do we want to carry commented code? I'd drop that.
>
>> +
>> +        if {$flags eq "nD"} {
>> +            set wd [inotify_watch add $full_filename "nwds"]
>> +        }
>> +        if {![string match *.lock $event_filename]} {
>> +            set update_view true
>> +        }
>> +    }
>> +
>> +    #reloadcommits or updatecommits - depending on file and operation?
>> +    if {$update_view} {
>> +        updatecommits
>> +    }
>> +}
>> +
>> +proc watch_recursive { dir } {
>> +    inotify_watch add $dir "nwaCmMds"
>> +
>> +    foreach i [glob -nocomplain -dir $dir *] {
>> +        if {[file type $i] eq {directory}} {
>> +            watch_recursive $i
>> +        }
>> +    }
>> +}
>> +
>> +if { $we_have_inotify } {
>> +    set fd [inotify create "inotify_watch" "::inotify_handler"]
>> +    watch_recursive $gitdir
>> +}
>> +
>>  set nullid "0000000000000000000000000000000000000000"
>>  set nullid2 "0000000000000000000000000000000000000001"
>>  set nullfile "/dev/null"
>> --
>> 2.7.4
>>
>
> Thanks,
> Stefan

Re: [PATCH] Gitk Inotify support

[Paul Mackerras]

On Sat, Jun 11, 2016 at 04:06:36PM +0200, Florian Schüller wrote:
> >From 74d2f4c1ec560b358fb50b8b7fe8282e7e1457b0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Florian=20Sch=C3=BCller?= <florian.schueller@gmail.com>
> Date: Thu, 9 Jun 2016 22:54:43 +0200
> Subject: [PATCH] first support for inotify
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Just automatically update gitk when working in a terminal on the same repo
> 
> Open points for now:
>  - release watches for deleted directories seems to
>    cause problems in tcl-inotify (so I don't)
>    I'm not sure how often that happens in ".git/"
>  - I only call "updatecommits" and I don't know if there is a usecase
>    where I should be calling "reloadcommits"

Thanks for the patch.  It's a nice idea.  I think it needs a couple of
improvements, though, to make it work even better:

* Some users might not want this behaviour, so we need an option in
  the preferences pane to enable/disable this.

* I would expect that the updates to the files in .git would come in
  bursts, so we should probably do something like wait until (say) one
  second has elapsed since the last notification, without any more
  notifications, before starting the update.

* We probably want to rate-limit the updates, since on a large tree
  (e.g. the Linux kernel) the update can take several seconds and the
  UI is less responsive during that time.

Paul.

Re: [PATCH] Gitk Inotify support

[Florian Schüller]

Just automatically update gitk when working in a terminal on the same repo

The commit is also on github if that makes things easier
https://github.com/schuellerf/gitk.git
(inotify branch)
https://github.com/schuellerf/gitk/tree/inotify

Features:
* Detects inotify support
  if inotify is not detected the options is not available
  in the preferences
* Enable/Disable auto update in the preferences
* Select "debounce" time for redraw
  i.e. the redraw will be postponed for the given time.
  if a new change is detected in this time the redraw is postponed
  even more
* Automatically scroll to the new HEAD after redrawing
* Depending on the type of change the UI is "Updated" or "Reloaded"

Open points for now:
* release watches for deleted directories seems to
  cause problems in tcl-inotify (so I don't)
  I'm not sure how often that happens in ".git/"

Signed-off-by: Florian Schüller <florian.schueller@gmail.com>
---
 gitk | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 170 insertions(+), 1 deletion(-)

diff --git a/gitk b/gitk
index a14d7a1..a2850d7 100755
--- a/gitk
+++ b/gitk
@@ -8,6 +8,12 @@ exec wish "$0" -- "$@"
 # either version 2, or (at your option) any later version.

 package require Tk
+try {
+    package require inotify
+    set have_inotify true
+} on error {em} {
+    set have_inotify false
+}

 proc hasworktree {} {
     return [expr {[exec git rev-parse --is-bare-repository] == "false" &&
@@ -11489,6 +11495,7 @@ proc prefspage_general {notebook} {
     global NS maxwidth maxgraphpct showneartags showlocalchanges
     global tabstop limitdiffs autoselect autosellen extdifftool perfile_attrs
     global hideremotes want_ttk have_ttk maxrefs
+    global autoupdate have_inotify autoupdatedebounce

     set page [create_prefs_page $notebook.general]

@@ -11505,13 +11512,21 @@ proc prefspage_general {notebook} {
     ${NS}::checkbutton $page.showlocal -text [mc "Show local changes"] \
  -variable showlocalchanges
     grid x $page.showlocal -sticky w
+
     ${NS}::checkbutton $page.autoselect -text [mc "Auto-select SHA1
(length)"] \
  -variable autoselect
     spinbox $page.autosellen -from 1 -to 40 -width 4 -textvariable autosellen
     grid x $page.autoselect $page.autosellen -sticky w
+
     ${NS}::checkbutton $page.hideremotes -text [mc "Hide remote refs"] \
  -variable hideremotes
     grid x $page.hideremotes -sticky w
+    if { $have_inotify } {
+        ${NS}::checkbutton $page.autoupdate -text [mc "Auto-update
upon change (ms)"] \
+            -variable autoupdate
+        spinbox $page.autoupdatedebounce -from 10 -to 60000 -width 7
-textvariable autoupdatedebounce
+        grid x $page.autoupdate $page.autoupdatedebounce -sticky w
+    }

     ${NS}::label $page.ddisp -text [mc "Diff display options"]
     grid $page.ddisp - -sticky w -pady 10
@@ -11765,7 +11780,8 @@ proc prefsok {} {
     global oldprefs prefstop showneartags showlocalchanges
     global fontpref mainfont textfont uifont
     global limitdiffs treediffs perfile_attrs
-    global hideremotes
+    global hideremotes autoupdate
+    global gitdir

     catch {destroy $prefstop}
     unset prefstop
@@ -11814,6 +11830,8 @@ proc prefsok {} {
     if {$hideremotes != $oldprefs(hideremotes)} {
  rereadrefs
     }
+
+    handle_inotify $gitdir true
 }

 proc formatdate {d} {
@@ -12295,6 +12313,13 @@ set autoselect 1
 set autosellen 40
 set perfile_attrs 0
 set want_ttk 1
+set autoupdate 1
+set autoupdatedebounce 100
+#timer id for inotify reloading
+set reload_id -1
+#timer id for inotify updating (less than reload)
+set update_id -1
+set inotify_instance -1

 if {[tk windowingsystem] eq "aqua"} {
     set extdifftool "opendiff"
@@ -12390,6 +12415,7 @@ set config_variables {
     filesepbgcolor filesepfgcolor linehoverbgcolor linehoverfgcolor
     linehoveroutlinecolor mainheadcirclecolor workingfilescirclecolor
     indexcirclecolor circlecolors linkfgcolor circleoutlinecolor
+    autoupdate autoupdatedebounce
 }
 foreach var $config_variables {
     config_init_trace $var
@@ -12477,6 +12503,149 @@ if {$i >= [llength $argv] && $revtreeargs ne {}} {
     }
 }

+#function to be called after inotify reload-timeout
+proc reload_helper {} {
+    #puts "RELOAD"
+    global reload_id
+    set reload_id -1
+    reloadcommits
+    set head [exec git rev-parse HEAD]
+    selbyid $head
+}
+
+#function to be called after inotify update-timeout
+proc update_helper {} {
+    #puts "UPDATE"
+    global update_id
+    set update_id -1
+    updatecommits
+    set head [exec git rev-parse HEAD]
+    selbyid $head
+}
+
+proc inotify_handler { fd } {
+    global autoupdate reload_id update_id autoupdatedebounce
+    set events [inotify_watch read]
+    set watch_info [inotify_watch info]
+    set update_view false
+    set reloadcommits false
+
+    #cancel pending timer
+    if { $reload_id ne -1 } {
+        #puts "cancel a reload"
+        after cancel $reload_id
+        set reload_id -1
+        set update_view true
+        set reloadcommits true
+    }
+
+    if { $update_id ne -1 } {
+        #puts "cancel an update"
+        after cancel $update_id
+        set update_id -1
+        set update_view true
+    }
+
+    foreach {event} $events {
+        set current_watchid [dict get $event watchid]
+        set flags [dict get $event flags]
+        set event_filename [dict get $event filename]
+
+        foreach {path watchid watch_flags} $watch_info {
+            if {$watchid eq $current_watchid} {
+                set watch_path $path
+            }
+        }
+
+        set full_filename [file join $watch_path $event_filename]
+        #check wether we should do update or reload below
+        #puts "Got: $full_filename / $event_filename ($flags)"
+
+        if {$flags eq "nD"} {
+            inotify_watch add $full_filename "nwds"
+        }
+        if {![string match *.lock $event_filename]} {
+            if { $flags eq "d" } {
+                #stuff like deleting branches should result in reloading
+                set reloadcommits true
+            }
+            set update_view true
+        }
+
+        #simple commit just needs updating right?
+        #if { $event_filename eq "COMMIT_EDITMSG" } {
+        #    set reloadcommits true
+        #}
+    }
+
+    #reloadcommits or updatecommits - depending on file and operation?
+    if { $update_view } {
+        if { $reloadcommits } {
+            #puts "schedule reload"
+            set reload_id [after $autoupdatedebounce reload_helper]
+        } else {
+            #puts "schedule update"
+            set update_id [after $autoupdatedebounce update_helper]
+        }
+    }
+}
+
+proc watch_recursive { dir } {
+    inotify_watch add $dir "nwaCmMds"
+
+    foreach i [glob -nocomplain -dir $dir *] {
+        if {[file type $i] eq {directory}} {
+            watch_recursive $i
+        }
+    }
+}
+
+proc enable_inotify { dir redraw} {
+    global inotify_instance autoupdatedebounce reload_id
+
+    if { $inotify_instance ne -1 } {
+        updatecommits
+    } else {
+        set inotify_instance [inotify create "inotify_watch"
"::inotify_handler"]
+        watch_recursive $dir
+        if { $redraw } {
+            set reload_id [after $autoupdatedebounce reload_helper]
+        }
+    }
+}
+
+proc disable_inotify {} {
+    global inotify_instance reload_id update_id
+
+    if { $inotify_instance ne -1 } {
+        rename inotify_watch {}
+        set inotify_instance -1
+    }
+
+    if { $reload_id ne -1 } {
+        after cancel $reload_id
+        set reload_id -1
+    }
+
+    if { $update_id ne -1 } {
+        after cancel $update_id
+        set update_id -1
+    }
+}
+
+proc handle_inotify { dir redraw } {
+    global have_inotify autoupdate
+    if { $have_inotify } {
+        if { $autoupdate } {
+            enable_inotify $dir $redraw
+        } else {
+            disable_inotify
+        }
+    }
+}
+
+handle_inotify $gitdir false
+
 set nullid "0000000000000000000000000000000000000000"
 set nullid2 "0000000000000000000000000000000000000001"
 set nullfile "/dev/null"
-- 
2.7.4