Attention.
Compliments of the day ; I have a business proposition for you which I sent you previously,I do not know if you received it? Please do find it proper to write me if your email is still active. Yours Faithfully, Barr. Alexander Stewart.
[PATCH v2 1/2] gitk: show part of submodule log instead of empty pane when listing trees
From: Alex Riesen <raa.l...@gmail.com>
Currently, selecting a name in the file list (bottom right) panel in
"Tree" mode does not do anything useful if the name is a submodule.
If gitk is currently showing a commit, the submodule names are not shown
at all (which is very confusing). If the gitk is showing the uncached
change, the submodules are shown, but focusing a submodule name causes a
Tcl error to appear. And finally, if gitk shows the index, the submodule
is presented as its bare name in the diff/file contents panel.
This change will show the first arbitrarily chosen number of commits.
Signed-off-by: Alex Riesen <raa.l...@gmail.com>
---
gitk | 44 ++--
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/gitk b/gitk
index a14d7a1..742f36b 100755
--- a/gitk
+++ b/gitk
@@ -7627,9 +7627,10 @@ proc gettreeline {gtf id} {
if {$i < 0} continue
set fname [string range $line [expr {$i+1}] end]
set line [string range $line 0 [expr {$i-1}]]
- if {$diffids ne $nullid2 && [lindex $line 1] ne "blob"} continue
+ set objtype [lindex $line 1]
+ if {$diffids ne $nullid2 && $objtype ne "blob" && $objtype ne
"commit" } { continue }
set sha1 [lindex $line 2]
- lappend treeidlist($id) $sha1
+ lappend treeidlist($id) "$sha1 $objtype"
}
if {[string index $fname 0] eq "\""} {
set fname [lindex $fname 0]
@@ -7659,21 +7660,44 @@ proc showfile {f} {
global ctext_file_names ctext_file_lines
global ctext commentend
+set submodlog "log --format=%h\\ %aN:\\ %s -100"
+set fcmt ""
set i [lsearch -exact $treefilelist($diffids) $f]
if {$i < 0} {
puts "oops, $f not in list for id $diffids"
return
}
if {$diffids eq $nullid} {
- if {[catch {set bf [open $f r]} err]} {
- puts "oops, can't read $f: $err"
- return
+ if {[file isdirectory $f]} {
+ # a submodule
+ set qf [shellquote $f]
+ if {[catch {set bf [open "| git -C $qf $submodlog" r]} err]} {
+ puts "oops, can't read submodule $f: $err"
+ return
+ }
+} else {
+ if {[catch {set bf [open $f r]} err]} {
+ puts "oops, can't read $f: $err"
+ return
+ }
}
} else {
- set blob [lindex $treeidlist($diffids) $i]
- if {[catch {set bf [open [concat | git cat-file blob $blob] r]} err]} {
- puts "oops, error reading blob $blob: $err"
- return
+ set bo [lindex $treeidlist($diffids) $i]
+ set blob [lindex $bo 0]
+ set objtype [lindex $bo 1]
+ if { "$objtype" eq "blob" } {
+ if {[catch {set bf [open [concat | git cat-file blob $blob] r]}
err]} {
+ puts "oops, error reading blob $blob: $err"
+ return
+ }
+ } else {
+ # also a submodule
+ set qf [shellquote $f]
+ if {[catch {set bf [open "| git -C $qf $submodlog $blob" r]} err]} {
+ puts "oops, error reading submodule commit: $err"
+ return
+ }
+ set fcmt "/"
}
}
fconfigure $bf -blocking 0 -encoding [get_path_encoding $f]
@@ -7683,7 +7707,7 @@ proc showfile {f} {
lappend ctext_file_names $f
lappend ctext_file_lines [lindex [split $commentend "."] 0]
$ctext insert end "\n"
-$ctext insert end "$f\n" filesep
+$ctext insert end "$f$fcmt\n" filesep
$ctext config -state disabled
$ctext yview $commentend
settabs 0
--
2.17.0.593.g2029711e64
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus
[PATCH v2 2/2] gitk: add an option to run gitk on an item in the file list
From: Alex Riesen <raa.l...@gmail.com>
Similar to a git gui feature which visualizes history in a submodule,
the submodules cause the gitk be started inside the submodule.
Signed-off-by: Alex Riesen <raa.l...@gmail.com>
---
gitk | 12
1 file changed, 12 insertions(+)
diff --git a/gitk b/gitk
index 742f36b..c430dfe 100755
--- a/gitk
+++ b/gitk
@@ -2682,6 +2682,7 @@ proc makewindow {} {
{mc "External diff" command {external_diff}}
{mc "Blame parent commit" command {external_blame 1}}
{mc "Copy path" command {clipboard clear; clipboard append
$flist_menu_file}}
+ {mc "Run gitk on this" command {flist_gitk}}
}
$flist_menu configure -tearoff 0
@@ -3555,6 +3556,17 @@ proc flist_hl {only} {
set gdttype [mc "touching paths:"]
}
+proc flist_gitk {} {
+global flist_menu_file findstring gdttype
+
+set x [shellquote $flist_menu_file]
+if {[file isdirectory $flist_menu_file]} {
+ exec sh -c "cd $x&" &
+} else {
+ exec gitk -- $x &
+}
+}
+
proc gitknewtmpdir {} {
global diffnum gitktmpdir gitdir env
--
2.17.0.593.g2029711e64
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus
[PATCH v2 0/2] gitk: improve handling of submodules in the file list panel
From: Alex Riesen <raa.l...@gmail.com> Currently, the submodule entries in the file list panel are mostly ignored. This series attempts to improve the situation by showing part of submodule history when focusing it in the file list panel and by adding a menu element to start gitk in the submodule (similar to git gui). This iteration does not address the behaviour of file list panel in tree mode when gitk is started from a subdirectory (gitk strictly limits the file listing to the files in that repository, without a way out). I would like to hear some more opinions regarding changing its behaviour to always list the full tree. Alex Riesen (2): gitk: show part of submodule log instead of empty pane when listing trees gitk: add an option to run gitk on an item in the file list gitk | 56 ++-- 1 file changed, 46 insertions(+), 10 deletions(-) -- 2.17.0.593.g2029711e64 --- Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus
Re: [PATCH] gitk: do not limit tree mode listing in the file list panel to current sub-directory
Bert Wesarg, Wed, May 09, 2018 14:08:51 +0200: > >> I noticed that too, while testing your patch and I'm also confused. > >> But was not able to send a request to Paul yet. ls-tree --full-tree > >> seems to be one that should be used here, I think. > > > > Well, I just tried your suggestion. 'ls-files' doesn't have --full-tree, so > > for those it is just cd-up. > > > > It is on top of the re-sent series. > > I would consider the current behavior as a bug, therefor I would put > this patch first, and than your other fixes/enhancements. I might do, just want to hear more opinions on the matter: someone might have good reasons for the current behaviour and consider a bug the fact that Patch mode behaves differently, for instance. And as I completely screwed up the resend of the series, there will definitely be a re-resend. Regards, Alex --- Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus
Re: [PATCH v2 0/2] gitk: improve handling of submodules in the file list panel
Sorry for broken threading... I'll have to work on that. --- Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus
[PATCH v2 2/2] gitk: add an option to run gitk on an item in the file list
From: Alex Riesen <raa.l...@gmail.com>
Similar to a git gui feature which visualizes history in a submodule,
the submodules cause the gitk be started inside the submodule.
---
gitk | 12
1 file changed, 12 insertions(+)
diff --git a/gitk b/gitk
index 742f36b..c430dfe 100755
--- a/gitk
+++ b/gitk
@@ -2682,6 +2682,7 @@ proc makewindow {} {
{mc "External diff" command {external_diff}}
{mc "Blame parent commit" command {external_blame 1}}
{mc "Copy path" command {clipboard clear; clipboard append
$flist_menu_file}}
+ {mc "Run gitk on this" command {flist_gitk}}
}
$flist_menu configure -tearoff 0
@@ -3555,6 +3556,17 @@ proc flist_hl {only} {
set gdttype [mc "touching paths:"]
}
+proc flist_gitk {} {
+global flist_menu_file findstring gdttype
+
+set x [shellquote $flist_menu_file]
+if {[file isdirectory $flist_menu_file]} {
+ exec sh -c "cd $x&" &
+} else {
+ exec gitk -- $x &
+}
+}
+
proc gitknewtmpdir {} {
global diffnum gitktmpdir gitdir env
--
2.17.0.593.g2029711e64
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus
[PATCH v2 1/2] gitk: show part of submodule log instead of empty pane when listing trees
From: Alex Riesen <raa.l...@gmail.com>
Currently, selecting a name in the file list (bottom right) panel in
"Tree" mode does not do anything useful if the name is a submodule.
If gitk is currently showing a commit, the submodule names are not shown
at all (which is very confusing). If the gitk is showing the uncached
change, the submodules are shown, but focusing a submodule name causes a
Tcl error to appear. And finally, if gitk shows the index, the submodule
is presented as its bare name in the diff/file contents panel.
This change will show the first arbitrarily chosen number of commits.
---
gitk | 44 ++--
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/gitk b/gitk
index a14d7a1..742f36b 100755
--- a/gitk
+++ b/gitk
@@ -7627,9 +7627,10 @@ proc gettreeline {gtf id} {
if {$i < 0} continue
set fname [string range $line [expr {$i+1}] end]
set line [string range $line 0 [expr {$i-1}]]
- if {$diffids ne $nullid2 && [lindex $line 1] ne "blob"} continue
+ set objtype [lindex $line 1]
+ if {$diffids ne $nullid2 && $objtype ne "blob" && $objtype ne
"commit" } { continue }
set sha1 [lindex $line 2]
- lappend treeidlist($id) $sha1
+ lappend treeidlist($id) "$sha1 $objtype"
}
if {[string index $fname 0] eq "\""} {
set fname [lindex $fname 0]
@@ -7659,21 +7660,44 @@ proc showfile {f} {
global ctext_file_names ctext_file_lines
global ctext commentend
+set submodlog "log --format=%h\\ %aN:\\ %s -100"
+set fcmt ""
set i [lsearch -exact $treefilelist($diffids) $f]
if {$i < 0} {
puts "oops, $f not in list for id $diffids"
return
}
if {$diffids eq $nullid} {
- if {[catch {set bf [open $f r]} err]} {
- puts "oops, can't read $f: $err"
- return
+ if {[file isdirectory $f]} {
+ # a submodule
+ set qf [shellquote $f]
+ if {[catch {set bf [open "| git -C $qf $submodlog" r]} err]} {
+ puts "oops, can't read submodule $f: $err"
+ return
+ }
+} else {
+ if {[catch {set bf [open $f r]} err]} {
+ puts "oops, can't read $f: $err"
+ return
+ }
}
} else {
- set blob [lindex $treeidlist($diffids) $i]
- if {[catch {set bf [open [concat | git cat-file blob $blob] r]} err]} {
- puts "oops, error reading blob $blob: $err"
- return
+ set bo [lindex $treeidlist($diffids) $i]
+ set blob [lindex $bo 0]
+ set objtype [lindex $bo 1]
+ if { "$objtype" eq "blob" } {
+ if {[catch {set bf [open [concat | git cat-file blob $blob] r]}
err]} {
+ puts "oops, error reading blob $blob: $err"
+ return
+ }
+ } else {
+ # also a submodule
+ set qf [shellquote $f]
+ if {[catch {set bf [open "| git -C $qf $submodlog $blob" r]} err]} {
+ puts "oops, error reading submodule commit: $err"
+ return
+ }
+ set fcmt "/"
}
}
fconfigure $bf -blocking 0 -encoding [get_path_encoding $f]
@@ -7683,7 +7707,7 @@ proc showfile {f} {
lappend ctext_file_names $f
lappend ctext_file_lines [lindex [split $commentend "."] 0]
$ctext insert end "\n"
-$ctext insert end "$f\n" filesep
+$ctext insert end "$f$fcmt\n" filesep
$ctext config -state disabled
$ctext yview $commentend
settabs 0
--
2.17.0.593.g2029711e64
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus
[PATCH v2 0/2] gitk: improve handling of submodules in the file list panel
Currently, the submodule entries in the file list panel are mostly ignored. This series attempts to improve the situation by showing part of submodule history when focusing it in the file list panel and by adding a menu element to start gitk in the submodule (similar to git gui). Alex Riesen (2): gitk: show part of submodule log instead of empty pane when listing trees gitk: add an option to run gitk on an item in the file list gitk | 56 ++-- 1 file changed, 46 insertions(+), 10 deletions(-) -- 2.17.0.593.g2029711e64 --- Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus
[PATCH] gitk: do not limit tree mode listing in the file list panel to current sub-directory
From: Alex Riesen <raa.l...@gmail.com>
The previous behavior conflicts with the "Patch" mode of the panel,
which always shows the changes from the top-level of the repository.
It is also impossible to get back to the full listing without restarting
gitk.
---
Bert Wesarg, Wed, May 09, 2018 09:19:55 +0200:
> > Frankly, this listing limited to just a sub-directory confuses me a bit. Is
> > there anyway to get to display full repository without changing to the top
> > level?
>
> I noticed that too, while testing your patch and I'm also confused.
> But was not able to send a request to Paul yet. ls-tree --full-tree
> seems to be one that should be used here, I think.
Well, I just tried your suggestion. 'ls-files' doesn't have --full-tree, so
for those it is just cd-up.
It is on top of the re-sent series.
gitk | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/gitk b/gitk
index c430dfe..03ead98 100755
--- a/gitk
+++ b/gitk
@@ -7600,18 +7600,18 @@ proc go_to_parent {i} {
proc gettree {id} {
global treefilelist treeidlist diffids diffmergeid treepending
-global nullid nullid2
+global nullid nullid2 cdup
set diffids $id
unset -nocomplain diffmergeid
if {![info exists treefilelist($id)]} {
if {![info exists treepending]} {
if {$id eq $nullid} {
- set cmd [list | git ls-files]
+ set cmd [list | git -C $cdup ls-files]
} elseif {$id eq $nullid2} {
- set cmd [list | git ls-files --stage -t]
+ set cmd [list | git -C $cdup ls-files --stage -t]
} else {
- set cmd [list | git ls-tree -r $id]
+ set cmd [list | git ls-tree --full-tree -r $id]
}
if {[catch {set gtf [open $cmd r]}]} {
return
@@ -7670,7 +7670,7 @@ proc gettreeline {gtf id} {
proc showfile {f} {
global treefilelist treeidlist diffids nullid nullid2
global ctext_file_names ctext_file_lines
-global ctext commentend
+global ctext commentend cdup
set submodlog "log --format=%h\\ %aN:\\ %s -100"
set fcmt ""
@@ -7680,15 +7680,15 @@ proc showfile {f} {
return
}
if {$diffids eq $nullid} {
- if {[file isdirectory $f]} {
+ if {[file isdirectory "$cdup$f"]} {
# a submodule
- set qf [shellquote $f]
+ set qf [shellquote "$cdup$f"]
if {[catch {set bf [open "| git -C $qf $submodlog" r]} err]} {
puts "oops, can't read submodule $f: $err"
return
}
} else {
- if {[catch {set bf [open $f r]} err]} {
+ if {[catch {set bf [open "$cdup$f" r]} err]} {
puts "oops, can't read $f: $err"
return
}
@@ -7704,7 +7704,7 @@ proc showfile {f} {
}
} else {
# also a submodule
- set qf [shellquote $f]
+ set qf [shellquote "$cdup$f"]
if {[catch {set bf [open "| git -C $qf $submodlog $blob" r]} err]} {
puts "oops, error reading submodule commit: $err"
return
--
2.17.0.593.g2029711e64
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus
Re: [PATCH 1/2] gitk: show part of submodule log instead of empty pane when listing trees
Stefan Beller, Tue, May 08, 2018 19:07:29 +0200:
> On Tue, May 8, 2018 at 5:22 AM, Alex Riesen
> <alexander.rie...@cetitec.com> wrote:
> > Currently, the submodules either are not shown at all (if listing a
> > committed tree) or a Tcl error appears (when clicking on a submodule
> > from the index list).
>
> I do not understand where this appears, yet.
> Where do I have to click to see the effects of this patch?
Er. I meant to say the file list panel (bottom right panel). Sorry,
didn't come out clear. I'll reword the commit message next time.
> > @@ -7659,21 +7660,42 @@ proc showfile {f} {
> > global ctext_file_names ctext_file_lines
> > global ctext commentend
> >
> > +set submodlog "git\\ log\\ --format='%h\\ %aN:\\ %s'\\ -100"
>
> Do we want to respect the config option diff.submodule here?
Probably not. It is already done when the file list panel is in "Patch" mode.
The "Tree" mode of the panel shows the files in full, so the submodules should
be shown similarly: in a format resembling their full (referenced) contents.
> The -100 is chosen rather arbitrarily. Ideally we'd only walk to the
> previous entry?
Yes, the limit is indeed arbitrary. I'm reluctant of listing full history,
though: it might take too long a while (and does, in my line of work). Maybe
an option in the settings? Or some kind of a more natural limit (for 1 second?
Until the end of panel?)
> > - if {[catch {set bf [open $f r]} err]} {
> > - puts "oops, can't read $f: $err"
> > - return
> > + if {[file isdirectory $f]} {
> > + # a submodule
> > + if {[catch {set bf [open "| sh -c cd\\ \"$f\"&&$submodlog" r]}
> > err]} {
>
> Can we have $submodlog use the "git -C command"
> option, then we could save the "cd &&" part, which might even
> save us from spawning a shell?
That's because I forgot about that option. Of course, I'll fix this.
Also need a shellquote for the path.
Thanks!
Alex
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus
Re: [PATCH 2/2] gitk: add an option to run gitk on an item in the file list
Bert Wesarg, Tue, May 08, 2018 15:17:03 +0200:
> On Tue, May 8, 2018 at 2:22 PM, Alex Riesen <alexander.rie...@cetitec.com>
> wrote:
> > +proc flist_gitk {} {
> > +global flist_menu_file findstring gdttype
> > +
> > +set x [shellquote $flist_menu_file]
>
> this needs to handle cdup, i.e., if gitk is run from a subdirectory,
> all paths needs to be prefixed with $cdup, like: [file join $cdup
> $flist_menu_file]
That, indeed, is easily done:
set x [shellquote [file join $cdup $flist_menu_file]]
if {[file isdirectory $flist_menu_file]} {
exec sh -c "cd $x&" &
} else {
exec gitk -- $x &
}
It just doesn't seem to work: gitk does not find any commits!
Maybe that's because the file panel lists only files for the current
subdirectory (without the path from the repo top-level)? E.g.
mkdir tst
cd tst
git init
mkdir a
touch top-file a/sub-file
git add -A ; git commit -m.
cd a
gitk
Gitk lists only sub-file.
Frankly, this listing limited to just a sub-directory confuses me a bit. Is
there anyway to get to display full repository without changing to the top
level?
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus
[PATCH 2/2] gitk: add an option to run gitk on an item in the file list
From: Alex Riesen <raa.l...@gmail.com>
Similar to a git gui feature which visualizes history in a submodule,
the submodules cause the gitk be started inside the submodule.
Signed-off-by: Alex Riesen <raa.l...@gmail.com>
---
gitk | 12
1 file changed, 12 insertions(+)
diff --git a/gitk b/gitk
index d34833f..1ec545e 100755
--- a/gitk
+++ b/gitk
@@ -2682,6 +2682,7 @@ proc makewindow {} {
{mc "External diff" command {external_diff}}
{mc "Blame parent commit" command {external_blame 1}}
{mc "Copy path" command {clipboard clear; clipboard append
$flist_menu_file}}
+ {mc "Run gitk on this" command {flist_gitk}}
}
$flist_menu configure -tearoff 0
@@ -3555,6 +3556,17 @@ proc flist_hl {only} {
set gdttype [mc "touching paths:"]
}
+proc flist_gitk {} {
+global flist_menu_file findstring gdttype
+
+set x [shellquote $flist_menu_file]
+if {[file isdirectory $flist_menu_file]} {
+ exec sh -c "cd $x&" &
+} else {
+ exec gitk -- $x &
+}
+}
+
proc gitknewtmpdir {} {
global diffnum gitktmpdir gitdir env
--
2.17.0.rc1.56.gb9824190bd
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus
[PATCH 1/2] gitk: show part of submodule log instead of empty pane when listing trees
From: Alex Riesen <raa.l...@gmail.com>
Currently, the submodules either are not shown at all (if listing a
committed tree) or a Tcl error appears (when clicking on a submodule
from the index list).
This will make it show first arbitrarily chosen number of commits,
which might be only marginally better.
Signed-off-by: Alex Riesen <raa.l...@gmail.com>
---
gitk | 42 --
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/gitk b/gitk
index a14d7a1..d34833f 100755
--- a/gitk
+++ b/gitk
@@ -7627,9 +7627,10 @@ proc gettreeline {gtf id} {
if {$i < 0} continue
set fname [string range $line [expr {$i+1}] end]
set line [string range $line 0 [expr {$i-1}]]
- if {$diffids ne $nullid2 && [lindex $line 1] ne "blob"} continue
+ set objtype [lindex $line 1]
+ if {$diffids ne $nullid2 && $objtype ne "blob" && $objtype ne
"commit" } { continue }
set sha1 [lindex $line 2]
- lappend treeidlist($id) $sha1
+ lappend treeidlist($id) "$sha1 $objtype"
}
if {[string index $fname 0] eq "\""} {
set fname [lindex $fname 0]
@@ -7659,21 +7660,42 @@ proc showfile {f} {
global ctext_file_names ctext_file_lines
global ctext commentend
+set submodlog "git\\ log\\ --format='%h\\ %aN:\\ %s'\\ -100"
+set fcmt ""
set i [lsearch -exact $treefilelist($diffids) $f]
if {$i < 0} {
puts "oops, $f not in list for id $diffids"
return
}
if {$diffids eq $nullid} {
- if {[catch {set bf [open $f r]} err]} {
- puts "oops, can't read $f: $err"
- return
+ if {[file isdirectory $f]} {
+ # a submodule
+ if {[catch {set bf [open "| sh -c cd\\ \"$f\"&&$submodlog" r]}
err]} {
+ puts "oops, can't read submodule $f: $err"
+ return
+ }
+} else {
+ if {[catch {set bf [open $f r]} err]} {
+ puts "oops, can't read $f: $err"
+ return
+ }
}
} else {
- set blob [lindex $treeidlist($diffids) $i]
- if {[catch {set bf [open [concat | git cat-file blob $blob] r]} err]} {
- puts "oops, error reading blob $blob: $err"
- return
+ set bo [lindex $treeidlist($diffids) $i]
+ set blob [lindex $bo 0]
+ set objtype [lindex $bo 1]
+ if { "$objtype" eq "blob" } {
+ if {[catch {set bf [open [concat | git cat-file blob $blob] r]}
err]} {
+ puts "oops, error reading blob $blob: $err"
+ return
+ }
+ } else {
+ # also a submodule
+ if {[catch {set bf [open "| sh -c cd\\ \"$f\"&&$submodlog\\ $blob"
r]} err]} {
+ puts "oops, error reading submodule commit: $err"
+ return
+ }
+ set fcmt "/"
}
}
fconfigure $bf -blocking 0 -encoding [get_path_encoding $f]
@@ -7683,7 +7705,7 @@ proc showfile {f} {
lappend ctext_file_names $f
lappend ctext_file_lines [lindex [split $commentend "."] 0]
$ctext insert end "\n"
-$ctext insert end "$f\n" filesep
+$ctext insert end "$f$fcmt\n" filesep
$ctext config -state disabled
$ctext yview $commentend
settabs 0
--
2.17.0.rc1.56.gb9824190bd
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus
[PATCH 0/2] gitk: improve handling of submodules in the file list panel
Currently, the submodule entries in the file list panel are mostly ignored. This series attempts to improve the situation by showing part of submodule history when focusing it in the file list panel and by adding a menu element to start gitk in the submodule (similar to git gui). [1/2]: gitk: show part of submodule log instead of empty pane when listing trees [2/2]: gitk: add an option to run gitk on an item in the file list gitk | 54 -- 1 file changed, 44 insertions(+), 10 deletions(-) --- Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus
Re: Socket activation for GitWeb FastCGI with systemd?
03.04.2018, 23:04, "Jacob Keller" <jacob.kel...@gmail.com>: > On Tue, Apr 3, 2018 at 11:53 AM, Alex Ivanov <gnido...@ya.ru> wrote: >> Hi. >> I want to use systemd as fastcgi spawner for gitweb + nginx. >> The traffic is low and number of users is limited + traversal bots. For >> that reason I've decided to use following mimimal services >> >> gitweb.socket >> [Unit] >> Description=GitWeb Socket >> >> [Socket] >> ListenStream=/run/gitweb.sock >> Accept=false >> >> [Install] >> WantedBy=sockets.target >> >> gitweb.service >> [Unit] >> Description=GitWeb Service >> >> [Service] >> Type=simple >> ExecStart=/path/to/gitweb.cgi --fcgi >> StandardInput=socket >> >> However this scheme is not resistant to simple DDOS. >> E.g. traversal bots often kill the service by opening non existing path >> (e.g http://host/?p=repo;a=blob;f=nonexisting/path;hb=HEAD showing in >> browser 404 - Cannot find file) many times consecutively, which leads to >> Apr 03 21:32:10 host systemd[1]: gitweb.service: Start request repeated too >> quickly. >> Apr 03 21:32:10 host systemd[1]: gitweb.service: Failed with result >> 'start-limit-hit'. >> Apr 03 21:32:10 host systemd[1]: Failed to start GitWeb service. >> and 502 Bad Gateway in browser. I believe the reason is that gitweb.service >> dies on failure and if it happens too often, systemd declines to restart the >> service due to start limit hit. >> So my question is how to correct systemd services for GitWeb to be >> resistant to such issue? I prefer to use single process to process all >> clients. >> Thanks. > > This sounds like a systemd specific question that might get a better > answer from the systemd mailing list. Thanks I will try that too. > > That being said, I believe if in this case gitweb is dying due to the > path not existing? You might be able to configure systemd to > understand that the particular exit code for when the path doesn't > exist is a "valid" exit, and not a failure case.. I will try to do that, but I'm afraid that there may be other ways to remotely abuse the service. > > I'm not entirely understanding your goal.. you want each request to > launch the gitweb process, and when it's done you want it to exit? But > if there are multiple connections at once you want it to stay alive > until it services them all? I think the best answer is configure > systemd to understand that the exit code for when the path is invalid > will be counted as a success. I want a single process for all connections too keep RAM usage at minimal. I also though it fits my case since number of users is low. > > Thanks, > Jake
Socket activation for GitWeb FastCGI with systemd?
Hi. I want to use systemd as fastcgi spawner for gitweb + nginx. The traffic is low and number of users is limited + traversal bots. For that reason I've decided to use following mimimal services gitweb.socket [Unit] Description=GitWeb Socket [Socket] ListenStream=/run/gitweb.sock Accept=false [Install] WantedBy=sockets.target gitweb.service [Unit] Description=GitWeb Service [Service] Type=simple ExecStart=/path/to/gitweb.cgi --fcgi StandardInput=socket However this scheme is not resistant to simple DDOS. E.g. traversal bots often kill the service by opening non existing path (e.g http://host/?p=repo;a=blob;f=nonexisting/path;hb=HEAD showing in browser 404 - Cannot find file) many times consecutively, which leads to Apr 03 21:32:10 host systemd[1]: gitweb.service: Start request repeated too quickly. Apr 03 21:32:10 host systemd[1]: gitweb.service: Failed with result 'start-limit-hit'. Apr 03 21:32:10 host systemd[1]: Failed to start GitWeb service. and 502 Bad Gateway in browser. I believe the reason is that gitweb.service dies on failure and if it happens too often, systemd declines to restart the service due to start limit hit. So my question is how to correct systemd services for GitWeb to be resistant to such issue? I prefer to use single process to process all clients. Thanks.
Git Merge contributor summit notes
It was great to meet some of you in person! Some notes from the Contributor Summit at Git Merge are below. Taken in haste, so my apologies if there are any mis-statements. - Alex "Does anyone think there's a compelling reason for git to exist?" - peff Partial clone (Jeff Hostetler / Jonathan Tan) - - Request that the server not send everything - Motivated by getting Windows into git - Also by not having to fetch large blobs that are in-tree - Allows client to request a clone that excludes some set of objects, with incomplete packfiles - Decoration on objects that include promise for later on-demand backfill - In `master`, have a way of: - omitting all blobs - omitting large blobs - sparse checkout specification stored on server - Hook in read_object to fetch objects in bulk - Future work: - A way to fetch blobsizes for virtual checkouts - Give me new blobs that this tree references relative to now - Omit some subset of trees - Modify other commits to exclude omitted blobs - Protocol v2 may have better verbs for sparse specification, etc Questions: - Reference server implementation? - In git itself - VSTS does not support - What happens if a commit becomes unreachable? Does promise still apply? - Probably yes? - If the promise is broken, probably crashes - Can differentiate between promise that was made, and one that wasn't => Demanding commitment from server to never GC seems like a strong promise - Interactions with external object db - promises include bulk fetches, as opposed to external db, which is one-at-a-time - dry-run semantics to determine which objects will be needed - very important for small objects, like commits/trees (which is not in `master`, only blobs) - perhaps for protocol V2 - server has to promise more, requires some level of online operation - annotate that only some refs are forever? - requires enabling the "fetch any SHA" flags - rebasing might require now-missing objects? - No, to build on them you must have fetched them - Well, building on someone else's work may mean you don't have all of them - server is less aggressive about GC'ing by keeping "weak references" when there are promises? - hosting requires that you be able to forcibly remove information - being able to know where a reference came from? - as being able to know why an object was needed, for more advanced logic - Does `git grep` attempt to fetch blobs that are deferred? - will always attempt to fetch - one fetch per object, even! - might not be true for sparse checkouts - Maybe limit to skipping "binary files"? - Currently sparse checkout grep "works" because grep defaults to looking at the index, not the commit - Does the above behavior for grepping revisions - Don't yet have a flag to exclude grep on non-fetched objects - Should `git grep -L` die if it can't fetch the file? - Need a config option for "should we die, or try to move on"? - What's the endgame? Only a few codepaths that are aware, or threaded through everywhere? - Fallback to fetch on demand means there's an almost-reasonable fallback - Better prediction with bulk fetching - Are most commands going to _need_ to be sensitive to it? - GVFS has a caching server in the building - A few git commands have been disabled (see recent mail from Stolee); those are likely candidates for code that needs to be aware of de-hydrated objects - Is there an API to know what objects are actually local? - No external API - GVFS has a REST API - Some way to later ask about files? - "virtualized filesystem"? - hook to say "focus on this world of files" - GVFS writes out your index currently - Will this always require turning off reachability checks? - Possibly - Shallow clones, instead of partial? - Don't download the history, just the objects - More of a protocol V2 property - Having all of the trees/commits make this reasonable - GVFS vs this? - GVFS was a first pass - Now trying to mainstream productize that - Goal is to remove features from GVFS and replace with this Protocol V2 (Brandon) - Main problem is that forward compatibility negotiation wasn't possible - Found a way to sneak in the V2 negotiation via side-channel in all transports - "environment variable" GIT_PROTOCOL which server can detect - Ability to transmit and ignore, or not transmit, means forward/backward compat - HTTP header / environment variable - ...s now what? - Keep as similar as possible, but more layout changes to remove bad characteristics - Like fixing flush semantics - Remove ref advertisement (250M of refs every fetch from Android!) - Capabilit
[ID] - [39485] [RE] - [Notice!]
PayPal Dear, YoursaccountshassBeenslimited! -To getsbacksintosyourspaypalsaccount,syousneedstosconfirmsyoursidentity. -it'sseasy ! 1. Clicksonstheslinksbelow. 2. Confirmsthatsyou'resthesownersofsthesaccount,sandsthensfollowsthesinstructions. Confirm Your account Sincerely ,
[ID] - [39485] [RE] - [Notice!]
PayPal Dear, YoursaccountshassBeenslimited! -To getsbacksintosyourspaypalsaccount,syousneedstosconfirmsyoursidentity. -it'sseasy ! 1. Clicksonstheslinksbelow. 2. Confirmsthatsyou'resthesownersofsthesaccount,sandsthensfollowsthesinstructions. Confirm Your account Sincerely ,
Re
-- Hello, I have a project i want to bring to you.. please respond for details Alex
Re: [PATCH v3 2/3] Remove now useless email-address parsing code
Matthieu Moy <g...@matthieu-moy.fr> writes:
> We now use Mail::Address unconditionaly, hence parse_mailboxes is now
> dead code. Remove it and its tests.
>
> Signed-off-by: Matthieu Moy <g...@matthieu-moy.fr>
Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
> ---
> No change since v2.
>
> perl/Git.pm | 71
>
> t/t9000-addresses.sh | 27
> t/t9000/test.pl | 67 -
> 3 files changed, 165 deletions(-)
> delete mode 100755 t/t9000-addresses.sh
> delete mode 100755 t/t9000/test.pl
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index ffa09ac..65e6b32 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -880,77 +880,6 @@ sub ident_person {
> return "$ident[0] <$ident[1]>";
> }
>
> -=item parse_mailboxes
> -
> -Return an array of mailboxes extracted from a string.
> -
> -=cut
> -
> -# Very close to Mail::Address's parser, but we still have minor
> -# differences in some cases (see t9000 for examples).
> -sub parse_mailboxes {
> - my $re_comment = qr/\((?:[^)]*)\)/;
> - my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;
> - my $re_word = qr/(?:[^]["\s()<>:;@\\,.]|\\.)+/;
> -
> - # divide the string in tokens of the above form
> - my $re_token = qr/(?:$re_quote|$re_word|$re_comment|\S)/;
> - my @tokens = map { $_ =~ /\s*($re_token)\s*/g } @_;
> - my $end_of_addr_seen = 0;
> -
> - # add a delimiter to simplify treatment for the last mailbox
> - push @tokens, ",";
> -
> - my (@addr_list, @phrase, @address, @comment, @buffer) = ();
> - foreach my $token (@tokens) {
> - if ($token =~ /^[,;]$/) {
> - # if buffer still contains undeterminated strings
> - # append it at the end of @address or @phrase
> - if ($end_of_addr_seen) {
> - push @phrase, @buffer;
> - } else {
> - push @address, @buffer;
> - }
> -
> - my $str_phrase = join ' ', @phrase;
> - my $str_address = join '', @address;
> - my $str_comment = join ' ', @comment;
> -
> - # quote are necessary if phrase contains
> - # special characters
> - if ($str_phrase =~ /[][()<>:;@\\,.\000-\037\177]/) {
> - $str_phrase =~ s/(^|[^\\])"/$1/g;
> - $str_phrase = qq["$str_phrase"];
> - }
> -
> - # add "<>" around the address if necessary
> - if ($str_address ne "" && $str_phrase ne "") {
> - $str_address = qq[<$str_address>];
> - }
> -
> - my $str_mailbox = "$str_phrase $str_address
> $str_comment";
> - $str_mailbox =~ s/^\s*|\s*$//g;
> - push @addr_list, $str_mailbox if ($str_mailbox);
> -
> - @phrase = @address = @comment = @buffer = ();
> - $end_of_addr_seen = 0;
> - } elsif ($token =~ /^\(/) {
> - push @comment, $token;
> - } elsif ($token eq "<") {
> - push @phrase, (splice @address), (splice @buffer);
> - } elsif ($token eq ">") {
> - $end_of_addr_seen = 1;
> - push @address, (splice @buffer);
> - } elsif ($token eq "@" && !$end_of_addr_seen) {
> - push @address, (splice @buffer), "@";
> - } else {
> - push @buffer, $token;
> - }
> - }
> -
> - return @addr_list;
> -}
> -
> =item hash_object ( TYPE, FILENAME )
>
> Compute the SHA1 object id of the given C considering it is
> diff --git a/t/t9000-addresses.sh b/t/t9000-addresses.sh
> deleted file mode 100755
> index a1ebef6..000
> --- a/t/t9000-addresses.sh
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -#!/bin/sh
> -
> -test_description='compare address parsing with and without Mail::Address'
> -. ./test-lib.sh
> -
> -if ! test_have_prereq PERL; then
> - skip_all='skipping perl interface tests, perl not available'
> - test_done
> -fi
> -
> -perl -MTest::More -e 0 2>/dev/null || {
> - skip_all="Perl Test::More unavailable, skipping test"
> -
Re: [PATCH v3 1/3] send-email: add and use a local copy of Mail::Address
Matthieu Moy <g...@matthieu-moy.fr> writes:
> We used to have two versions of the email parsing code. Our
> parse_mailboxes (in Git.pm), and Mail::Address which we used if
> installed. Unfortunately, both versions have different sets of bugs, and
> changing the behavior of git depending on whether Mail::Address is
> installed was a bad idea.
>
> A first attempt to solve this was cc90750 (send-email: don't use
> Mail::Address, even if available, 2017-08-23), but it turns out our
> parse_mailboxes is too buggy for some uses. For example the lack of
> nested comments support breaks get_maintainer.pl in the Linux kernel
> tree:
>
> https://public-inbox.org/git/20171116154814.23785-1-alex.ben...@linaro.org/
>
> This patch goes the other way: use Mail::Address anyway, but have a
> local copy from CPAN as a fallback, when the system one is not
> available.
>
> The duplicated script is small (276 lines of code) and stable in time.
> Maintaining the local copy should not be an issue, and will certainly be
> less burden than maintaining our own parse_mailboxes.
>
> Another option would be to consider Mail::Address as a hard dependency,
> but it's easy enough to save the trouble of extra-dependency to the end
> user or packager.
>
> Signed-off-by: Matthieu Moy <g...@matthieu-moy.fr>
Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
> ---
> No change since v2.
>
> git-send-email.perl | 3 +-
> perl/Git/FromCPAN/Mail/Address.pm | 276
> ++
> perl/Git/Mail/Address.pm | 24
> 3 files changed, 302 insertions(+), 1 deletion(-)
> create mode 100644 perl/Git/FromCPAN/Mail/Address.pm
> create mode 100755 perl/Git/Mail/Address.pm
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index edcc6d3..340b5c8 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -30,6 +30,7 @@ use Error qw(:try);
> use Cwd qw(abs_path cwd);
> use Git;
> use Git::I18N;
> +use Git::Mail::Address;
>
> Getopt::Long::Configure qw/ pass_through /;
>
> @@ -489,7 +490,7 @@ my ($repoauthor, $repocommitter);
> ($repocommitter) = Git::ident_person(@repo, 'committer');
>
> sub parse_address_line {
> - return Git::parse_mailboxes($_[0]);
> + return map { $_->format } Mail::Address->parse($_[0]);
> }
>
> sub split_addrs {
> diff --git a/perl/Git/FromCPAN/Mail/Address.pm
> b/perl/Git/FromCPAN/Mail/Address.pm
> new file mode 100644
> index 000..13b2ff7
> --- /dev/null
> +++ b/perl/Git/FromCPAN/Mail/Address.pm
> @@ -0,0 +1,276 @@
> +# Copyrights 1995-2017 by [Mark Overmeer <p...@overmeer.net>].
> +# For other contributors see ChangeLog.
> +# See the manual pages for details on the licensing terms.
> +# Pod stripped from pm file by OODoc 2.02.
> +package Mail::Address;
> +use vars '$VERSION';
> +$VERSION = '2.19';
> +
> +use strict;
> +
> +use Carp;
> +
> +# use locale; removed in version 1.78, because it causes taint problems
> +
> +sub Version { our $VERSION }
> +
> +
> +
> +# given a comment, attempt to extract a person's name
> +sub _extract_name
> +{ # This function can be called as method as well
> +my $self = @_ && ref $_[0] ? shift : undef;
> +
> +local $_ = shift
> +or return '';
> +
> +# Using encodings, too hard. See Mail::Message::Field::Full.
> +return '' if m/\=\?.*?\?\=/;
> +
> +# trim whitespace
> +s/^\s+//;
> +s/\s+$//;
> +s/\s+/ /;
> +
> +# Disregard numeric names (e.g. 123456.1...@compuserve.com)
> +return "" if /^[\d ]+$/;
> +
> +s/^\((.*)\)$/$1/; # remove outermost parenthesis
> +s/^"(.*)"$/$1/; # remove outer quotation marks
> +s/\(.*?\)//g; # remove minimal embedded comments
> +s/\\//g; # remove all escapes
> +s/^"(.*)"$/$1/; # remove internal quotation marks
> +s/^([^\s]+) ?, ?(.*)$/$2 $1/; # reverse "Last, First M." if applicable
> +s/,.*//;
> +
> +# Change casing only when the name contains only upper or only
> +# lower cased characters.
> +unless( m/[A-Z]/ && m/[a-z]/ )
> +{ # Set the case of the name to first char upper rest lower
> +s/\b(\w+)/\L\u$1/igo; # Upcase first letter on name
> +s/\bMc(\w)/Mc\u$1/igo; # Scottish names such as 'McLeod'
> +s/\bo'(\w)/O'\u$1/igo; # Irish names such as 'O'Malley, O'Reilly'
> +s/\b(x*(ix)?v*(iv)?i*)\b/\U$1/igo; # Roman numerals, eg 'Level III
> Support'
> +}
> +
> +# some cleanup
> +s/\[[^\]]*\]//g;
> +s/(^[\s'"]+|[\s'"]+$)//g;
> +s/\s{2,}/ /g;
>
Re: [PATCH] send-email: add test for Linux's get_maintainer.pl
Matthieu Moy <g...@matthieu-moy.fr> writes: > From: Alex Bennée <alex.ben...@linaro.org> > > We had a regression that broke Linux's get_maintainer.pl. Using > Mail::Address to parse email addresses fixed it, but let's protect > against future regressions. > > Patch-edited-by: Matthieu Moy <g...@matthieu-moy.fr> > Signed-off-by: Alex Bennée <alex.ben...@linaro.org> > Signed-off-by: Matthieu Moy <g...@matthieu-moy.fr> > --- > t/t9001-send-email.sh | 22 ++ > 1 file changed, 22 insertions(+) > > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh > index 4d261c2..f126177 100755 > --- a/t/t9001-send-email.sh > +++ b/t/t9001-send-email.sh > @@ -172,6 +172,28 @@ test_expect_success $PREREQ 'cc trailer with various > syntax' ' > test_cmp expected-cc commandline1 > ' > > +test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc > trailer' " > + cat >expected-cc-script.sh <<-EOF && > + #!/bin/sh > + echo 'One Person <o...@example.com> (supporter:THIS (FOO/bar))' > + echo 'Two Person <t...@example.com> (maintainer:THIS THING)' > + echo 'Third List <th...@example.com> (moderated list:THIS THING > (FOO/bar))' > + echo '<f...@example.com> (moderated list:FOR THING)' > + echo 'f...@example.com (open list:FOR THING (FOO/bar))' > + echo 's...@example.com (open list)' > + EOF > + chmod +x expected-cc-script.sh > +" > + > +test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' ' > + test_commit cc-trailer-getmaint && > + clean_fake_sendmail && > + git send-email -1 --to=recipi...@example.com \ > + --cc-cmd="./expected-cc-script.sh" \ > + --smtp-server="$(pwd)/fake.sendmail" && > + test_cmp expected-cc commandline1 > +' > + > test_expect_success $PREREQ 'setup expect' " > cat >expected-show-all-headers <<\EOF > 0001-Second.patch I think you need to apply Eric's suggestions from: From: Eric Sunshine <sunsh...@sunshineco.com> Date: Sat, 18 Nov 2017 21:54:46 -0500 Message-ID: <capig+csh0tvvkh0xf9fwcfm4gngawmsn_fxd2zhzhcy2try...@mail.gmail.com> -- Alex Bennée
Re: [RFC PATCH 2/2] Remove now useless email-address parsing code
est_done
> -}
> -
> -perl -MMail::Address -e 0 2>/dev/null || {
> - skip_all="Perl Mail::Address unavailable, skipping test"
> - test_done
> -}
> -
> -test_external_has_tap=1
> -
> -test_external_without_stderr \
> - 'Perl address parsing function' \
> - perl "$TEST_DIRECTORY"/t9000/test.pl
> -
> -test_done
> diff --git a/t/t9000/test.pl b/t/t9000/test.pl
> deleted file mode 100755
> index dfeaa9c..000
> --- a/t/t9000/test.pl
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -#!/usr/bin/perl
> -use lib (split(/:/, $ENV{GITPERLLIB}));
> -
> -use 5.008;
> -use warnings;
> -use strict;
> -
> -use Test::More qw(no_plan);
> -use Mail::Address;
> -
> -BEGIN { use_ok('Git') }
> -
> -my @success_list = (q[Jane],
> - q[j...@example.com],
> - q[<j...@example.com>],
> - q[Jane <j...@example.com>],
> - q[Jane Doe <j...@example.com>],
> - q["Jane" <j...@example.com>],
> - q["Doe, Jane" <j...@example.com>],
> - q["Jane@:;\>.,()<Doe" <j...@example.com>],
> - q[Jane!#$%&'*+-/=?^_{|}~Doe' <j...@example.com>],
> - q["<j...@example.com>"],
> - q["Jane j...@example.com"],
> - q[Jane Doe ],
> - q[Jane Doe < j...@example.com >],
> - q[Jane @ Doe @ Jane @ Doe],
> - q["Jane, 'Doe'" <j...@example.com>],
> - q['Doe, "Jane' <j...@example.com>],
> - q["Jane" "Do"e <j...@example.com>],
> - q["Jane' Doe" <j...@example.com>],
> - q["Jane Doe <j...@example.com>" <j...@example.com>],
> - q["Jane\" Doe" <j...@example.com>],
> - q[Doe, jane <j...@example.com>],
> - q["Jane Doe <j...@example.com>],
> - q['Jane 'Doe' <j...@example.com>],
> - q[Jane@:;\.,()<>Doe <j...@example.com>],
> - q[Jane <j...@example.com> Doe],
> - q[<j...@example.com> Jane Doe]);
> -
> -my @known_failure_list = (q[Jane\ Doe <j...@example.com>],
> - q["Doe, Ja"ne <j...@example.com>],
> - q["Doe, Katarina" Jane <j...@example.com>],
> - q[Jane j...@example.com],
> - q["Jane "Kat"a" ri"na" ",Doe" <j...@example.com>],
> - q[Jane Doe],
> - q[Jane "Doe <j...@example.com>"],
> - q[\"Jane Doe <j...@example.com>],
> - q[Jane\"\" Doe <j...@example.com>],
> - q['Jane "Katarina\" \' Doe' <j...@example.com>]);
> -
> -foreach my $str (@success_list) {
> - my @expected = map { $_->format } Mail::Address->parse("$str");
> - my @actual = Git::parse_mailboxes("$str");
> - is_deeply(\@expected, \@actual, qq[same output : $str]);
> -}
> -
> -TODO: {
> - local $TODO = "known breakage";
> - foreach my $str (@known_failure_list) {
> - my @expected = map { $_->format } Mail::Address->parse("$str");
> - my @actual = Git::parse_mailboxes("$str");
> - is_deeply(\@expected, \@actual, qq[same output : $str]);
> - }
> -}
> -
> -my $is_passing = eval { Test::More->is_passing };
> -exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;
--
Alex Bennée
Re: [PATCH] Makefile: replace perl/Makefile.PL with simple make rules
On Thu, 30 Nov 2017, Jeff King wrote: > On Wed, Nov 29, 2017 at 07:54:30PM +, Ævar Arnfjörð Bjarmason wrote: > > > Replace the perl/Makefile.PL and the fallback perl/Makefile used under > > NO_PERL_MAKEMAKER=NoThanks with a much simpler implementation heavily > > inspired by how the i18n infrastructure's build process works[1]. > > I'm very happy to see the recursive make invocation go away. The perl > makefile generation was one of the few places where parallel make could > racily get confused (though I haven't seen that for a while, so maybe it > was fixed alongside some of the other .stamp work you did). As a datapoint, I've seen it fairly regularly with -j8 in 2.15.1 builds from a clean tree that I've been doing recently. I'm looking forward to not having to make the choice between "maybe run it twice" or "compile slower" -- this looks great! - Alex
Re: [PATCH v6] Makefile: replace perl/Makefile.PL with simple make rules
Ævar Arnfjörð Bjarmason, Wed, Dec 20, 2017 19:24:19 +0100: > diff --git a/INSTALL b/INSTALL > index ffb071e9f0..808e07b659 100644 > --- a/INSTALL > +++ b/INSTALL > @@ -84,9 +84,24 @@ Issues of note: > > GIT_EXEC_PATH=`pwd` > PATH=`pwd`:$PATH > - GITPERLLIB=`pwd`/perl/blib/lib > + GITPERLLIB=`pwd`/perl/build/lib > export GIT_EXEC_PATH PATH GITPERLLIB Sincerely sorry for off-topic, I just wonder why this section of INSTALL uses the backticks for command substitution. Is there a shell which does not support the alternative form $(...) but has all the rest of the used syntax? --- Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus
Re: [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor
On Tue, 19 Dec 2017, Junio C Hamano wrote:
> That (and existing) uses of printf() all feel a bit overkill ;-)
> Perhaps putchar() would suffice.
>
> I am not sure if the above wants to become something like
>
> for (i = 0; i < istate->cache_nr; i++) {
> putchar(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID ? '+' :
> '-');
> quote_c_style(istate->cache[i]->name, NULL, stdout, 0);
> putchar('\n');
> }
>
> instead of "a single long incomplete line" in the first place. Your
> "fix" merely turns it into "a single long complete line", which does
> not quite feel big enough an improvement, at least to me.
The more user-digestable form like you describe already exists by way
of `git ls-files -f`. I am not sure it is worth replicating it.
The only current uses of this tool are in tests, which only examine
the first ("no fsmonitor" / "fsmonitor last update ...") line. I find
it useful as a brief summary view of the fsmonitor bits, but I suppose
I'd also be happy with just presence/absence and a count of set/unset
bits.
Barring objections from Dscho or Ben, I'll reroll with a version that
shows something like:
fsmonitor last update 1513821151547101894 (5 seconds ago)
5 files valid / 10 files invalid
- Alex
Re: [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later
On Mon, 18 Dec 2017, Alex Vandiver wrote:
> dd9005a0a ("fsmonitor: delay updating state until after split index is
> merged", 2017-10-27) began deferring the setting of the
> CE_FSMONITOR_VALID flag until later, such that do_read_index() does
> not perform those steps. This means that t/helper/test-dump-fsmonitor
> showed all bits as always unset.
This isn't the right fix, actually. With split indexes, this puts us
right back into "only shows a few bits" territory, because
do_read_index doesn't know about split indexes.
Which means we need a way to do the whole index load but _not_
add/remove the fsmonitor cache, even if the config says to do so.
The best I'm coming up with is the below -- but I'm not happy with
it, because 'keep' is meaningless as a configuration value outside of
testing, since it's normally treated as an executable path. This uses
the fact that fsmonitor.c currently has a:
switch (fsmonitor_enabled) {
case -1: /* keep: do nothing */
break;
...despite get_config_get_fsmonitor() havong no way to return -1 at
present.
Is this sort of testing generally done via environment variables,
rather than magic config values?
- Alex
-8<
diff --git a/config.c b/config.c
index 6fb06c213..75fcf1a52 100644
--- a/config.c
+++ b/config.c
@@ -2164,8 +2164,13 @@ int git_config_get_fsmonitor(void)
if (core_fsmonitor && !*core_fsmonitor)
core_fsmonitor = NULL;
- if (core_fsmonitor)
- return 1;
+
+ if (core_fsmonitor) {
+ if (!strcasecmp(core_fsmonitor, "keep"))
+ return -1;
+ else
+ return 1;
+ }
return 0;
}
diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index ad452707e..12e131530 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -5,8 +5,9 @@ int cmd_main(int ac, const char **av)
struct index_state *istate = _index;
int i;
+ git_config_push_parameter("core.fsmonitor=keep");
setup_git_directory();
- if (do_read_index(istate, get_index_file(), 0) < 0)
+ if (read_index_from(istate, get_index_file()) < 0)
die("unable to read index file");
if (!istate->fsmonitor_last_update) {
printf("no fsmonitor\n");
-8<-
Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path
On Tue, 19 Dec 2017, Junio C Hamano wrote:
> Alex Vandiver <ale...@dropbox.com> writes:
>
> > Subject: Re: [PATCH 2/6] fsmonitor: Add dir.h include, for
> > untracked_cache_invalidate_path
>
> Perhaps
>
> "Subject: fsmonitor.h: include dir.h"
Certainly more concise.
> But I am not sure if this is a right direction to go in. If a .C
> user of fsmonitor needs (does not need) things from dir.h, that file
> can (does not need to) include dir.h itself.
Hm; I was patterning based on existing .h files, which don't seem shy
about pulling in other .h files.
> I think this header does excessive "static inline" as premature
> optimization, so a better "fix" to your perceived problem may be to
> make them not "static inline".
Yeah, quite possibly. Ben, do you recall your rationale for inlining
them in 6a6da08f6 ("fsmonitor: teach git to optionally utilize a file
system monitor to speed up detecting new or changed files.",
2017-09-22) ?
- Alex
[PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh
These were mistakenly left in when the test was introduced, in
1487372d3 ("fsmonitor: store fsmonitor bitmap before splitting index",
2017-11-09)
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
t/t7519-status-fsmonitor.sh | 2 --
1 file changed, 2 deletions(-)
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index eb2d13bbc..19b2a0a0f 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -307,9 +307,7 @@ test_expect_success 'splitting the index results in the
same state' '
dirty_repo &&
git update-index --fsmonitor &&
git ls-files -f >expect &&
- test-dump-fsmonitor >&2 && echo &&
git update-index --fsmonitor --split-index &&
- test-dump-fsmonitor >&2 && echo &&
git ls-files -f >actual &&
test_cmp expect actual
'
--
2.15.1.626.gc4617b774
[PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
With fsmonitor enabled, the first call to match_stat_with_submodule
calls refresh_fsmonitor, incurring the overhead of reading the list of
updated files -- but run_diff_files does not respect the
CE_FSMONITOR_VALID flag.
Make use of the fsmonitor extension to skip lstat() calls on files
that fsmonitor judged as unmodified. Skip use of the fsmonitor
extension when called by "add", as the format_callback in such cases
expects to be called even when the file is believed to be "up to date"
with the index.
Notably, this change improves performance of the git shell prompt when
GIT_PS1_SHOWDIRTYSTATE is set.
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
builtin/add.c | 2 +-
diff-lib.c| 6 ++
diff.h| 2 ++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/builtin/add.c b/builtin/add.c
index bf01d89e2..bba20b46e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -119,7 +119,7 @@ int add_files_to_cache(const char *prefix,
rev.diffopt.format_callback_data =
rev.diffopt.flags.override_submodule_config = 1;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
- run_diff_files(, DIFF_RACY_IS_MODIFIED);
+ run_diff_files(, DIFF_RACY_IS_MODIFIED | DIFF_SKIP_FSMONITOR);
clear_pathspec(_data);
return !!data.add_errors;
}
diff --git a/diff-lib.c b/diff-lib.c
index 8104603a3..13ff00d81 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -95,6 +95,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
diff_set_mnemonic_prefix(>diffopt, "i/", "w/");
+ if (!(option & DIFF_SKIP_FSMONITOR))
+ refresh_fsmonitor(_index);
+
if (diff_unmerged_stage < 0)
diff_unmerged_stage = 2;
entries = active_nr;
@@ -197,6 +200,9 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
if (ce_uptodate(ce) || ce_skip_worktree(ce))
continue;
+ if (ce->ce_flags & CE_FSMONITOR_VALID && !(option &
DIFF_SKIP_FSMONITOR))
+ continue;
+
/* If CE_VALID is set, don't look at workdir for file removal */
if (ce->ce_flags & CE_VALID) {
changed = 0;
diff --git a/diff.h b/diff.h
index 36a09624f..1060bc495 100644
--- a/diff.h
+++ b/diff.h
@@ -395,6 +395,8 @@ extern const char *diff_aligned_abbrev(const struct
object_id *sha1, int);
#define DIFF_SILENT_ON_REMOVED 01
/* report racily-clean paths as modified */
#define DIFF_RACY_IS_MODIFIED 02
+/* skip loading the fsmonitor data */
+#define DIFF_SKIP_FSMONITOR 04
extern int run_diff_files(struct rev_info *revs, unsigned int option);
extern int run_diff_index(struct rev_info *revs, int cached);
--
2.15.1.626.gc4617b774
Re: [PATCH 2/2] fsmonitor: Store fsmonitor bitmap before splitting index
On Mon, 13 Nov 2017, Ben Peart wrote: > Why do you redirect stdout to stderr and then and perform an "echo" > afterwards? I don't understand what benefit that provides. I removed this > logic and the test still passes so am confused as to what its purpose is. Ah -- the "echo" was purely to clean up STDERR as I was running the test interactively. It serves no purpose, which is why it was hard to understand its benefit. :) Apologies for missing this (and in not replying here earlier!). I'll send a commit that drops these. - Alex > > +# test that splitting the index dosn't interfere > > +test_expect_success 'splitting the index results in the same state' ' > > + write_integration_script && > > + dirty_repo && > > + git update-index --fsmonitor && > > + git ls-files -f >expect && > > + test-dump-fsmonitor >&2 && echo && > > + git update-index --fsmonitor --split-index && > > + test-dump-fsmonitor >&2 && echo && > > + git ls-files -f >actual && > > + test_cmp expect actual > > +' > > + > > test_done
Re: [PATCH] git-send-email: fix get_maintainer.pl regression
Thomas Adam <tho...@xteddy.org> writes: > Hi, > > On Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote: >> I.e. we'd just ship a copy of Email::Valid and Mail::Address in >> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't >> need to if/else this at the code level, just always use the module, >> and it would work even on core perl. > > I disagree with the premise of this, Ævar. As soon as you go down this route, > it increases maintenance to ensure we keep up to date with what's on CPAN for > a tiny edge-case which I don't believe exists. > > You may as well just use App::FatPacker. > > We're talking about package maintenance here -- and as I said before, there's > plenty of it around. For those distributions which ship Git (and hence also > package git-send-email), the dependencies are already there, too. I just > cannot see this being a problem in relying on non-core perl modules. Every > perl program does this, and they don't go down this route of having copies of > various CPAN modules just in case. So why should we? We're not a special > snowflake. I less bothered my the potentially shipping a git specific copy than ensuring the packagers pick up the dependency when they do their builds. Do we already have a mechanism for testing for non-core perl modules during the "configure" phase of git? -- Alex Bennée
Re: [PATCH] git-send-email: fix get_maintainer.pl regression
Junio C Hamano <gits...@pobox.com> writes:
> Thomas Adam <tho...@xteddy.org> writes:
>
>> Trying to come up with a reinvention of regexps for email addresses is asking
>> for trouble, not to mention a crappy rod for your own back. Don't do that.
>> This is why people use Mail::Address.
>>
>> https://metacpan.org/pod/distribution/MailTools/lib/Mail/Address.pod
>
> Now we are coming back to cc907506 ("send-email: don't use
> Mail::Address, even if available", 2017-08-23). It argues
>
> * Having this optional Mail::Address makes it tempting to anwser "please
> install Mail::Address" to users instead of fixing our own code. We've
> reached the stage where bugs in our parser should be fixed, not worked
> around.
>
> but if it costs us maintaining our substitute that much, it seems to
> me that depending on Mail::Address is not just tempting but may be a
> sensible way forward.
>
> Was there any reason why Mail::Address was _inadequate_? I know we
> had trouble with random garbage that are *not* addresses people put
> on the in-body CC: trailer in the past, but I do not recall if they
> are something Mail::Address would give worse result and we need our
> workaround (hence our own substitute), or Mail::Address would handle
> them just fine.
Ping?
So have we come to a consensus about the best solution here?
I'm perfectly happy to send a reversion patch because to be honest
hacking on a bunch of perl to handle special mail cases is not my idea
of fun spare time hacking ;-)
I guess the full solution is to make Mail::Address a hard dependency?
--
Alex Bennée
Re: [PATCH] git-send-email: fix get_maintainer.pl regression
Matthieu Moy <matthieu@univ-lyon1.fr> writes:
> Junio C Hamano <gits...@pobox.com> writes:
>
>> Was there any reason why Mail::Address was _inadequate_?
>
> I think the main reason was that Mail::Address is not a standard perl
> module, and not relying on it avoided one external dependency. AFAIK, we
> don't really have a good way to deal with Perl dependencies, so having a
> strong requirement on Mail::Address will probably end up in a runtime
> error for users who compile Git from source (people using a packaged
> version have their package manager to deal with this).
>
>> I know we had trouble with random garbage that are *not* addresses
>> people put on the in-body CC: trailer in the past, but I do not recall
>> if they are something Mail::Address would give worse result and we
>> need our workaround (hence our own substitute), or Mail::Address would
>> handle them just fine.
>
> For a long time, we used Mail::Address if it was available and I don't
> think we had issues with it.
>
> My point in cc907506 ("send-email: don't use Mail::Address, even if
> available", 2017-08-23) was not that Mail::Address was bad, but that
> changing our behavior depending on whether it was there or not was
> really bad. For example, the issue dealt with in this thread probably
> always existed, but it was present only for *some* users.
So I just did a little digging on my systems to illustrate the point. My
work machine is Ubuntu, so has a packaged git via PPA. It depends on
libmailtools-perl which includes the perl module and:
apt-cache rdepends libmailtools-perl | wc -l
45
So for binary packaged systems it's not a big thing - libmailtools-perl
seems to be quite widely relied on.
On my system at home, running Gentoo, while requiring "perl" doesn't
explicitly pull in the Mail::Address dependency. As a result the git
send-email stanza I was running at work manages to corrupt the
addresses. If I manually install dev-perl/MailTools of course it
silently starts working again.
Good job I never usually send patches from my home machine ;-)
My hacky guess about GIT's perl use calls is:
find . -iname "*.perl" -or -iname "*.pm" -or -iname "*.pl" | xargs grep -h
"use .*::" | sort | uniq | wc -l
88
So that is about 88 perl modules used in the code base. How many of them
are not part of the core perl distribution?
Should the solution be to just make Mail::Address a hard dependency and
not have the fallback?
--
Alex Bennée
Re: [PATCH] git-send-email: fix get_maintainer.pl regression
Eric Sunshine <sunsh...@sunshineco.com> writes:
> A few more comments/observations...
>
> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée <alex.ben...@linaro.org> wrote:
>> diff --git a/perl/Git.pm b/perl/Git.pm
>> @@ -936,6 +936,9 @@ sub parse_mailboxes {
>> $end_of_addr_seen = 0;
>> } elsif ($token =~ /^\(/) {
>> push @comment, $token;
>> + } elsif ($token =~ /^\)/) {
>> + my $nested_comment = pop @comment;
>> + push @comment, "$nested_comment$token";
>
> Due to the way tokenization works, it looks like you will only ever
> see a ")" as a single character. That suggests that you should be
> using ($token eq ")"), as is done for "<" and ">", rather than ($token
> =~ /^\)/).
>
> What happens if there is text before the final closing ')'? For
> instance, "foo@bar (bibble (bobble) smoo)" or "...)smoo)". The result
> is that "smoo" ends up tacked onto the end of the email address
> ("foo@barsmoo") rather than incorporated into the comment, as
> intended.
>
> What happens if you encounter a ")" but haven't yet encountered an
> opening "(" (that is, @comment is empty)? For example, "foo@bar )". In
> that case, it unconditionally pops from the empty array, which seems
> iffy at best. It might be nice to see this case taken into
> consideration explicitly.
Yeah I was only really aiming for the current regression but I'm sure it
could be more solid. I do note that my @known_failure_list in test.pl
has a bunch of other cases that need fixing up.
> I also was wondering if it would make more sense to take advantage of
> Perl's ability to match nested expressions (??{$nested}), however,
> that feature apparently was added in 5.10, and Git.pm only requires
> 5.8, so perhaps not (unless we want to bump the requirement higher).
Hmm that might be a case of abusing regex to do something better suited
to a proper tokenizer.
>
> Aside from those observations, it looks like the tokenizer in this
> function is broken. For any input with the address enclosed in "<" and
> ">", the comment is lost entirely; it doesn't even end up in the
> @tokens array. Since you're already fixing bugs/regressions in this
> code, perhaps that's something you'd like to tackle as well in a
> separate patch? ("No" is an acceptable answer, of course.)
>
>> } elsif ($token eq "<") {
>> push @phrase, (splice @address), (splice @buffer);
>> } elsif ($token eq ">") {
I can have a go but my perl-fu has weakened somewhat since I stopped
having to maintain perl code for a living. It's almost as though my
brain was glad to dump the knowledge ;-)
I guess we could maintain a nesting count and a current token type and
use that to more intelligently direct the nested portions to the
appropriate bits. Maybe Matthieu or Remi (CC'ed) might want to chime in
on other options?
--
Alex Bennée
[PATCH v2] git-send-email: fix --cc-cmd get_maintainer.pl regression
Since the removal of Mail::Address from git-send-email certain address
patterns returned by common get_maintainer.pl scripts now fail to get
correctly parsed by the built-in Git::parse_mailboxes. Specifically
the patterns with embedded parenthesis fail. For example from the
Linux kernel MAINTAINERS:
KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)
L:linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
L:kvm...@lists.cs.columbia.edu
Which is returned by get_maintainers.pl as:
linux-arm-ker...@lists.infradead.org (moderated list:KERNEL VIRTUAL MACHINE
FOR ARM (KVM/arm))
kvm...@lists.cs.columbia.edu (open list:KERNEL VIRTUAL MACHINE FOR ARM
(KVM/arm))
However Git::parse_mailboxes code mangles the address, appending the
trailing parenthesis to the email address to the address part causing
it to fail validation:
error: unable to extract a valid address from:
linux-arm-ker...@lists.infradead.org) (moderated list:KERNEL VIRTUAL MACHINE
FOR ARM (KVM/arm)
error: unable to extract a valid address from: kvm...@lists.cs.columbia.edu)
(open list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)
As this is a common pattern which was handled by Mail::Address I've
fixed the regression by explicitly capturing a trailing bracket and
appending it to the comment token.
NB: the t9001.sh test doesn't explicitly wrap the call to the --cc-cmd
in a "$(pwd)/expected-cc-script.sh" which fails due to the space to
the full-path of the test. It is currently ambiguous as to if --cc-cmd
needs to handle this. I suspect it is not an edge case that has come
up in real-world usage as git-send-email is usually run directly from
a git directory with scripts generally in a ./script/get_maintainer.pl
path.
Fixes: cc9075067776ebd34cc08f31bf78bb05f12fd879
Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
Cc: Eric Sunshine <sunsh...@sunshineco.com>
---
perl/Git.pm | 3 +++
t/t9000/test.pl | 3 ++-
t/t9001-send-email.sh | 16
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index ffa09ace9..9b17de1cc 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -936,6 +936,9 @@ sub parse_mailboxes {
$end_of_addr_seen = 0;
} elsif ($token =~ /^\(/) {
push @comment, $token;
+ } elsif ($token =~ /^\)/) {
+ my $nested_comment = pop @comment;
+ push @comment, "$nested_comment$token";
} elsif ($token eq "<") {
push @phrase, (splice @address), (splice @buffer);
} elsif ($token eq ">") {
diff --git a/t/t9000/test.pl b/t/t9000/test.pl
index dfeaa9c65..b01642a0d 100755
--- a/t/t9000/test.pl
+++ b/t/t9000/test.pl
@@ -35,7 +35,8 @@ my @success_list = (q[Jane],
q['Jane 'Doe' <j...@example.com>],
q[Jane@:;\.,()<>Doe <j...@example.com>],
q[Jane <j...@example.com> Doe],
- q[<j...@example.com> Jane Doe]);
+ q[<j...@example.com> Jane Doe],
+ q[j...@example.com (open list:for thing (foo/bar))]);
my @known_failure_list = (q[Jane\ Doe <j...@example.com>],
q["Doe, Ja"ne <j...@example.com>],
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4d261c2a9..fa783eb87 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -172,6 +172,22 @@ test_expect_success $PREREQ 'cc trailer with various
syntax' '
test_cmp expected-cc commandline1
'
+test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
+write_script expected-cc-script.sh <<-EOF &&
+echo "One Person <o...@example.com> (supporter:THIS (FOO/bar))"
+echo "Two Person <t...@example.com> (maintainer:THIS THING)"
+echo "Third List <th...@example.com> (moderated list:THIS THING (FOO/bar))"
+echo "<f...@example.com> (moderated list:FOR THING)"
+echo "f...@example.com (open list:FOR THING (FOO/bar))"
+echo "s...@example.com (open list)"
+EOF
+ clean_fake_sendmail &&
+ git send-email -1 --to=recipi...@example.com \
+ --cc-cmd=./expected-cc-script.sh \
+ --smtp-server="$(pwd)/fake.sendmail" &&
+ test_cmp expected-cc commandline1
+'
+
test_expect_success $PREREQ 'setup expect' "
cat >expected-show-all-headers <<\EOF
0001-Second.patch
--
2.15.0
[PATCH] git-send-email: fix --cc-cmd get_maintainer.pl regression
Since the removal of Mail::Address from git-send-email certain address
patterns returned by common get_maintainer.pl scripts now fail to get
correctly parsed by the built-in Git::parse_mailboxes. Specifically
the patterns with embedded parenthesis fail. For example from the
Linux kernel MAINTAINERS:
KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)
L:linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
L:kvm...@lists.cs.columbia.edu
Which is returned by get_maintainers.pl as:
linux-arm-ker...@lists.infradead.org (moderated list:KERNEL VIRTUAL MACHINE
FOR ARM (KVM/arm))
kvm...@lists.cs.columbia.edu (open list:KERNEL VIRTUAL MACHINE FOR ARM
(KVM/arm))
However Git::parse_mailboxes code mangles the address, appending the
trailing parenthesis to the email address to the address part causing
it to fail validation:
error: unable to extract a valid address from:
linux-arm-ker...@lists.infradead.org) (moderated list:KERNEL VIRTUAL MACHINE
FOR ARM (KVM/arm)
error: unable to extract a valid address from: kvm...@lists.cs.columbia.edu)
(open list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)
As this is a common pattern which was handled by Mail::Address I've
fixed the regression by explicitly capturing a trailing bracket and
appending it to the comment token.
NB: the t9001.sh test doesn't explicitly wrap the call to the --cc-cmd
in a "$(pwd)/expected-cc-script.sh" which fails due to the space to
the full-path of the test. It is currently ambiguous as to if --cc-cmd
needs to handle this. I suspect it is not an edge case that has come
up in real-world usage as git-send-email is usually run directly from
a git directory with scripts generally in a ./script/get_maintainer.pl
path.
Fixes: cc9075067776ebd34cc08f31bf78bb05f12fd879
Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
Cc: Eric Sunshine <sunsh...@sunshineco.com>
---
perl/Git.pm | 3 +++
t/t9000/test.pl | 3 ++-
t/t9001-send-email.sh | 16
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index ffa09ace9..9b17de1cc 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -936,6 +936,9 @@ sub parse_mailboxes {
$end_of_addr_seen = 0;
} elsif ($token =~ /^\(/) {
push @comment, $token;
+ } elsif ($token =~ /^\)/) {
+ my $nested_comment = pop @comment;
+ push @comment, "$nested_comment$token";
} elsif ($token eq "<") {
push @phrase, (splice @address), (splice @buffer);
} elsif ($token eq ">") {
diff --git a/t/t9000/test.pl b/t/t9000/test.pl
index dfeaa9c65..b01642a0d 100755
--- a/t/t9000/test.pl
+++ b/t/t9000/test.pl
@@ -35,7 +35,8 @@ my @success_list = (q[Jane],
q['Jane 'Doe' <j...@example.com>],
q[Jane@:;\.,()<>Doe <j...@example.com>],
q[Jane <j...@example.com> Doe],
- q[<j...@example.com> Jane Doe]);
+ q[<j...@example.com> Jane Doe],
+ q[j...@example.com (open list:for thing (foo/bar))]);
my @known_failure_list = (q[Jane\ Doe <j...@example.com>],
q["Doe, Ja"ne <j...@example.com>],
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4d261c2a9..fa783eb87 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -172,6 +172,22 @@ test_expect_success $PREREQ 'cc trailer with various
syntax' '
test_cmp expected-cc commandline1
'
+test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
+write_script expected-cc-script.sh <<-EOF &&
+echo "One Person <o...@example.com> (supporter:THIS (FOO/bar))"
+echo "Two Person <t...@example.com> (maintainer:THIS THING)"
+echo "Third List <th...@example.com> (moderated list:THIS THING (FOO/bar))"
+echo "<f...@example.com> (moderated list:FOR THING)"
+echo "f...@example.com (open list:FOR THING (FOO/bar))"
+echo "s...@example.com (open list)"
+EOF
+ clean_fake_sendmail &&
+ git send-email -1 --to=recipi...@example.com \
+ --cc-cmd=./expected-cc-script.sh \
+ --smtp-server="$(pwd)/fake.sendmail" &&
+ test_cmp expected-cc commandline1
+'
+
test_expect_success $PREREQ 'setup expect' "
cat >expected-show-all-headers <<\EOF
0001-Second.patch
--
2.15.0
Re: [PATCH] git-send-email: fix get_maintainer.pl regression
Eric Sunshine <sunsh...@sunshineco.com> writes: > On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée <alex.ben...@linaro.org> wrote: >> Getting rid of Mail::Address regressed behaviour with common >> get_maintainer scripts such as the Linux kernel. Fix the missed corner >> case and add a test for it. > > It is not at all clear, based upon this text, what this is fixing. > When you re-roll, please provide a description of the regression in > sufficient detail for readers to easily understand the problem and the > solution. How about: Since the removal of Mail::Address from git-send-email certain addresses common in MAINTAINERS now fail to get correctly parsed by Git::parse_mailboxes. Specifically the patterns with embedded parenthesis fail, for example for MAINTAINERS: KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm) L:linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) L:kvm...@lists.cs.columbia.edu Is returned by get_maintainers.pl as: linux-arm-ker...@lists.infradead.org (moderated list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)) kvm...@lists.cs.columbia.edu (open list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)) But the parse_mailboxes code mangles the address, appending the trailing parenthesis to the email address to the address part causing it to fail validation: error: unable to extract a valid address from: linux-arm-ker...@lists.infradead.org) (moderated list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm) error: unable to extract a valid address from: kvm...@lists.cs.columbia.edu) (open list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm) As this is a common pattern which was handled by Mail::Address I've fixed the regression by explicitly capturing a trailing bracket and appending it to the comment token. > > More below... > >> Signed-off-by: Alex Bennée <alex.ben...@linaro.org> >> --- >> diff --git a/t/t9000/test.pl b/t/t9000/test.pl >> @@ -35,7 +35,9 @@ my @success_list = (q[Jane], >> q['Jane 'Doe' <j...@example.com>], >> q[Jane@:;\.,()<>Doe <j...@example.com>], >> q[Jane <j...@example.com> Doe], >> - q[<j...@example.com> Jane Doe]); >> + q[<j...@example.com> Jane Doe], >> + q[j...@example.com (open list:for thing (foo/bar))], >> +); > > Style nit: The dangling ");" introduced by this change differs from > the other list(s) in this file which have the closing ");" on the same > line as the last item in the list. > >> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh >> +test_expect_success $PREREQ 'setup get_mainter script for cc trailer' " >> +cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh >> +#!/bin/sh >> +echo 'One Person <o...@example.com> (supporter:THIS (FOO/bar))' >> +echo 'Two Person <t...@example.com> (maintainer:THIS THING)' >> +echo 'Third List <th...@example.com> (moderated list:THIS THING (FOO/bar))' >> +echo '<f...@example.com> (moderated list:FOR THING)' >> +echo 'f...@example.com (open list:FOR THING (FOO/bar))' >> +echo 's...@example.com (open list)' >> +EOF >> +" > > Use write_script() to create the script: Thanks for the pointers, I'll fix it up. I missed the existence of write_script.sh while I scanned through t/README, perhaps a stanza in in "Test harness library": - write_script <<-\EOF && echo '...' ... EOF The write_script helper takes care of ensuring the created helper script has the right shebang and is executable. ? > > --- 8< --- > write_script expected-cc-script.sh <<-\EOF && > echo '...' > ... > EOF > --- 8< --- > > No need for #!/bin/sh or chmod, both of which are handled by > write_script(). In fact, you could fold this into the following test > (since there doesn't seem a good reason for it to be separate). > >> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' ' >> + test_commit cc-trailer && >> + clean_fake_sendmail && >> + git send-email -1 --to=recipi...@example.com \ >> + --cc-cmd="$(pwd)/expected-cc-script.sh" \ >> + --smtp-server="$(pwd)/fake.sendmail" && >> + test_cmp expected-cc commandline1 >> +' >> OK I'm afraid I don't fully understand the test harness as this breaks a >> bunch of other tests. If anyone can offer some pointers on how to fix >> I'd be grateful. > > There are several problems: > > * test_commit bombs because there is already a tag named "cc-trailer" > created by an earlier test. Fix this by choosing a d
Re: [PATCH] git-send-email: fix get_maintainer.pl regression
Alex Bennée <alex.ben...@linaro.org> writes:
> Getting rid of Mail::Address regressed behaviour with common
> get_maintainer scripts such as the Linux kernel. Fix the missed corner
> case and add a test for it.
>
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 4d261c2a9..0bcd7ab96 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -172,6 +172,27 @@ test_expect_success $PREREQ 'cc trailer with various
> syntax' '
> test_cmp expected-cc commandline1
> '
>
> +test_expect_success $PREREQ 'setup get_mainter script for cc trailer' "
> +cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh
> +#!/bin/sh
> +echo 'One Person <o...@example.com> (supporter:THIS (FOO/bar))'
> +echo 'Two Person <t...@example.com> (maintainer:THIS THING)'
> +echo 'Third List <th...@example.com> (moderated list:THIS THING (FOO/bar))'
> +echo '<f...@example.com> (moderated list:FOR THING)'
> +echo 'f...@example.com (open list:FOR THING (FOO/bar))'
> +echo 's...@example.com (open list)'
> +EOF
> +"
> +
> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
> + test_commit cc-trailer &&
> + clean_fake_sendmail &&
> + git send-email -1 --to=recipi...@example.com \
> + --cc-cmd="$(pwd)/expected-cc-script.sh" \
> + --smtp-server="$(pwd)/fake.sendmail" &&
> + test_cmp expected-cc commandline1
> +'
> +
OK I'm afraid I don't fully understand the test harness as this breaks a
bunch of other tests. If anyone can offer some pointers on how to fix
I'd be grateful.
In the meantime I know the core change works because I tested with:
#+name: send-patches-dry-run
#+begin_src sh :results output
# temp workaround
export PERL5LIB=/home/alex/src/git.git/perl/
git send-email --confirm=never --dry-run --quiet ${mailto} ${series}.patches/*
#+end_src
When I sent my last set of kernel patches to the list (the workflow that
was broken before by the cc9075067776ebd34cc08f31bf78bb05f12fd879 change
landing via my git stable PPA).
--
Alex Bennée
[PATCH] git-send-email: fix get_maintainer.pl regression
Getting rid of Mail::Address regressed behaviour with common
get_maintainer scripts such as the Linux kernel. Fix the missed corner
case and add a test for it.
Fixes: cc9075067776ebd34cc08f31bf78bb05f12fd879
Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
---
perl/Git.pm | 3 +++
t/t9000/test.pl | 4 +++-
t/t9001-send-email.sh | 21 +
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index ffa09ace9..9b17de1cc 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -936,6 +936,9 @@ sub parse_mailboxes {
$end_of_addr_seen = 0;
} elsif ($token =~ /^\(/) {
push @comment, $token;
+ } elsif ($token =~ /^\)/) {
+ my $nested_comment = pop @comment;
+ push @comment, "$nested_comment$token";
} elsif ($token eq "<") {
push @phrase, (splice @address), (splice @buffer);
} elsif ($token eq ">") {
diff --git a/t/t9000/test.pl b/t/t9000/test.pl
index dfeaa9c65..f10be50cd 100755
--- a/t/t9000/test.pl
+++ b/t/t9000/test.pl
@@ -35,7 +35,9 @@ my @success_list = (q[Jane],
q['Jane 'Doe' <j...@example.com>],
q[Jane@:;\.,()<>Doe <j...@example.com>],
q[Jane <j...@example.com> Doe],
- q[<j...@example.com> Jane Doe]);
+ q[<j...@example.com> Jane Doe],
+ q[j...@example.com (open list:for thing (foo/bar))],
+);
my @known_failure_list = (q[Jane\ Doe <j...@example.com>],
q["Doe, Ja"ne <j...@example.com>],
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4d261c2a9..0bcd7ab96 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -172,6 +172,27 @@ test_expect_success $PREREQ 'cc trailer with various
syntax' '
test_cmp expected-cc commandline1
'
+test_expect_success $PREREQ 'setup get_mainter script for cc trailer' "
+cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh
+#!/bin/sh
+echo 'One Person <o...@example.com> (supporter:THIS (FOO/bar))'
+echo 'Two Person <t...@example.com> (maintainer:THIS THING)'
+echo 'Third List <th...@example.com> (moderated list:THIS THING (FOO/bar))'
+echo '<f...@example.com> (moderated list:FOR THING)'
+echo 'f...@example.com (open list:FOR THING (FOO/bar))'
+echo 's...@example.com (open list)'
+EOF
+"
+
+test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
+ test_commit cc-trailer &&
+ clean_fake_sendmail &&
+ git send-email -1 --to=recipi...@example.com \
+ --cc-cmd="$(pwd)/expected-cc-script.sh" \
+ --smtp-server="$(pwd)/fake.sendmail" &&
+ test_cmp expected-cc commandline1
+'
+
test_expect_success $PREREQ 'setup expect' "
cat >expected-show-all-headers <<\EOF
0001-Second.patch
--
2.15.0
[PATCH 0/2] fsmonitor: Stop reading from PWD, write fsmonitor+split index right
A couple further patches for the fsmonitor branch, which ideally I'd have noticed before my previous series landed. In the first patch I believe I've found the underlying reason for the PWD confusion in the previous go-around -- but I'm not sure I'm wholly convinced about my solution to it. Specifically, it seems like this problem is likely more widespread than this one place, so adjusting it in the example hook may just be leaving the same dangerous edge for others to stumble across later. - Alex
[PATCH 2/2] fsmonitor: Store fsmonitor bitmap before splitting index
ba1b9caca6 resolved the problem of the fsmonitor data being applied to
the non-base index when reading; however, a similar problem exists
when writing the index. Specifically, writing of the fsmonitor
extension happens only after the work to split the index has been
applied -- as such, the information in the index is only for the
non-"base" index, and thus the extension information contains only
partial data.
When saving, compute the ewah bitmap before the index is split, and
store it in the fsmonitor_dirty field, mirroring the behavior that
occurred during reading. fsmonitor_dirty is kept from being leaked by
being freed when the extension data is written -- which always happens
precisely once, no matter the split index configuration.
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
fsmonitor.c | 20
fsmonitor.h | 9 -
read-cache.c| 3 +++
t/t7519-status-fsmonitor.sh | 13 +
4 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index f494a866d..0af7c4edb 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -54,12 +54,19 @@ int read_fsmonitor_extension(struct index_state *istate,
const void *data,
return 0;
}
+void fill_fsmonitor_bitmap(struct index_state *istate)
+{
+ int i;
+ istate->fsmonitor_dirty = ewah_new();
+ for (i = 0; i < istate->cache_nr; i++)
+ if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
+ ewah_set(istate->fsmonitor_dirty, i);
+}
+
void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
{
uint32_t hdr_version;
uint64_t tm;
- struct ewah_bitmap *bitmap;
- int i;
uint32_t ewah_start;
uint32_t ewah_size = 0;
int fixup = 0;
@@ -73,12 +80,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct
index_state *istate)
strbuf_add(sb, _size, sizeof(uint32_t)); /* we'll fix this up
later */
ewah_start = sb->len;
- bitmap = ewah_new();
- for (i = 0; i < istate->cache_nr; i++)
- if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
- ewah_set(bitmap, i);
- ewah_serialize_strbuf(bitmap, sb);
- ewah_free(bitmap);
+ ewah_serialize_strbuf(istate->fsmonitor_dirty, sb);
+ ewah_free(istate->fsmonitor_dirty);
+ istate->fsmonitor_dirty = NULL;
/* fix up size field */
put_be32(_size, sb->len - ewah_start);
diff --git a/fsmonitor.h b/fsmonitor.h
index 0de644e01..cd3cc0ccf 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -10,7 +10,14 @@ extern struct trace_key trace_fsmonitor;
extern int read_fsmonitor_extension(struct index_state *istate, const void
*data, unsigned long sz);
/*
- * Write the CE_FSMONITOR_VALID state into the fsmonitor index extension.
+ * Fill the fsmonitor_dirty ewah bits with their state from the index,
+ * before it is split during writing.
+ */
+extern void fill_fsmonitor_bitmap(struct index_state *istate);
+
+/*
+ * Write the CE_FSMONITOR_VALID state into the fsmonitor index
+ * extension. Reads from the fsmonitor_dirty ewah in the index.
*/
extern void write_fsmonitor_extension(struct strbuf *sb, struct index_state
*istate);
diff --git a/read-cache.c b/read-cache.c
index c18e5e623..7976d39d6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2529,6 +2529,9 @@ int write_locked_index(struct index_state *istate, struct
lock_file *lock,
int new_shared_index, ret;
struct split_index *si = istate->split_index;
+ if (istate->fsmonitor_last_update)
+ fill_fsmonitor_bitmap(istate);
+
if (!si || alternate_index_output ||
(istate->cache_changed & ~EXTMASK)) {
if (si)
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index c6df85af5..eb2d13bbc 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -301,4 +301,17 @@ do
done
done
+# test that splitting the index dosn't interfere
+test_expect_success 'splitting the index results in the same state' '
+ write_integration_script &&
+ dirty_repo &&
+ git update-index --fsmonitor &&
+ git ls-files -f >expect &&
+ test-dump-fsmonitor >&2 && echo &&
+ git update-index --fsmonitor --split-index &&
+ test-dump-fsmonitor >&2 && echo &&
+ git ls-files -f >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.15.0.rc1.413.g76aedb451
[PATCH 1/2] fsmonitor: Read from getcwd(), not the PWD environment variable
Though the process has chdir'd to the root of the working tree, the
PWD environment variable is only guaranteed to be updated accordingly
if a shell is involved -- which is not guaranteed to be the case.
That is, if `/usr/bin/perl` is a binary, $ENV{PWD} is unchanged from
whatever spawned `git` -- if `/usr/bin/perl` is a trivial shell
wrapper to the real `perl`, `$ENV{PWD}` will have been updated to the
root of the working copy.
Update to read from the Cwd module using the `getcwd` syscall, not the
PWD environment variable. The Cygwin case is left unchanged, as it
necessarily _does_ go through a shell.
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
t/t7519/fsmonitor-watchman | 3 ++-
templates/hooks--fsmonitor-watchman.sample | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index a3e30bf54..5fe72cefa 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -41,7 +41,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;
} else {
- $git_work_tree = $ENV{'PWD'};
+ require Cwd;
+ $git_work_tree = Cwd::cwd();
}
my $retry = 1;
diff --git a/templates/hooks--fsmonitor-watchman.sample
b/templates/hooks--fsmonitor-watchman.sample
index 9a082f278..ba6d88c5f 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -40,7 +40,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;
} else {
- $git_work_tree = $ENV{'PWD'};
+ require Cwd;
+ $git_work_tree = Cwd::cwd();
}
my $retry = 1;
--
2.15.0.rc1.413.g76aedb451
RE: NEED A LOAN?
Hello, Do you need a loan? Regards Chen Alex Prime Funds
Re: [PATCH v3 4/4] fsmonitor: Delay updating state until after split index is merged
On Tue, 31 Oct 2017, Junio C Hamano wrote: > This makes local variable "int i;" in this function unused and gets > compiler warning. Apologies for leaving that detritus -- I saw you added a 'SQUASH??' commit to deal with it, which LGTM. On Tue, 31 Oct 2017, Johannes Schindelin wrote: > ... to which end we introduced the DEVELOPER flag to catch these: if you > call > > make DEVELOPER=1 Aha! Thanks for the tip; I'll be sure to use that from now on. - Alex
Re: [PATCH v2] read_index_from(): Skip verification of the cache entry order to speed index loading
On Mon, 30 Oct 2017, Jeff King wrote: > On Mon, Oct 30, 2017 at 08:48:48AM -0400, Ben Peart wrote: > > > Any updates or thoughts on this one? While the patch has become quite > > trivial, it does results in a savings of 5%-15% in index load time. > > I like the general direction of avoiding the check during each read. Same -- the savings here are well worth it, IMHO. > > I thought the compromise of having this test only run when DEBUG is defined > > should limit it to developer builds (hopefully everyone developing on git is > > running DEBUG builds :)). Since the test is trying to detect buggy code > > when writing the index, I thought that was the right time to test/catch any > > issues. > > I certainly don't build with DEBUG. It traditionally hasn't done > anything useful. But I'm also not convinced that this is a likely way to > find bugs in the first place, so I'm OK missing out on it. I also don't compile with DEBUG -- there's no documentation that mentions it, and I don't think I'd considered going poking for what was `#ifdef`d. I think it'd be reasonable to provide some configure-time setting that adds `CFLAGS="-ggdb3 -O0 -DDEBUG"` or similar, but that seems possibly moot for this particular change (see below). > But what we probably _do_ need is to make sure that "git fsck" would > detect such an out-of-order index. So that developers and users alike > can diagnose suspected problems. Agree -- that seems like a better home for this logic. > > I am working on other, more substantial savings for index load times > > (stay tuned) but this seemed like a small simple way to help speed > > things up. I'm interested to hear more about what direction you're looking in here. - Alex
[PATCH v3 1/4] fsmonitor: Set the PWD to the top of the working tree
The fsmonitor command inherits the PWD of its caller, which may be anywhere in the working copy. This makes is difficult for the fsmonitor command to operate on the whole repository. Specifically, for the watchman integration, this causes each subdirectory to get its own watch entry. Set the CWD to the top of the working directory, for consistency. Signed-off-by: Alex Vandiver <ale...@dropbox.com> --- fsmonitor.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fsmonitor.c b/fsmonitor.c index 7c1540c05..4ea44dcc6 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que argv[3] = NULL; cp.argv = argv; cp.use_shell = 1; + cp.dir = get_git_work_tree(); return capture_command(, query_result, 1024); } -- 2.15.0.rc1.413.g76aedb451
[PATCH v3] 0/4 fsmonitor fixes
Updates since v2: - Fix tab which crept into 1/4 - Fixed the benchmarking code in the commit message in 2/4 to just always load JSON::XS -- the previous version was the version where I'd broken that to force loading of JSON::PP. - Remove the --no-pretty from the t/ version of query-watchman in 2/4; I don't know how I messed up diff'ing the file previously, but if there are already differences, it makes sense to let them slide.
[PATCH v3 3/4] fsmonitor: Document GIT_TRACE_FSMONITOR
Signed-off-by: Alex Vandiver <ale...@dropbox.com> --- Documentation/git.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/git.txt b/Documentation/git.txt index 1fca63634..720db196e 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -594,6 +594,10 @@ into it. Unsetting the variable, or setting it to empty, "0" or "false" (case insensitive) disables trace messages. +`GIT_TRACE_FSMONITOR`:: + Enables trace messages for the filesystem monitor extension. + See `GIT_TRACE` for available trace output options. + `GIT_TRACE_PACK_ACCESS`:: Enables trace messages for all accesses to any packs. For each access, the pack file name and an offset in the pack is -- 2.15.0.rc1.413.g76aedb451
[PATCH v3 4/4] fsmonitor: Delay updating state until after split index is merged
If the fsmonitor extension is used in conjunction with the split index
extension, the set of entries in the index when it is first loaded is
only a subset of the real index. This leads to only the non-"base"
index being marked as CE_FSMONITOR_VALID.
Delay the expansion of the ewah bitmap until after tweak_split_index
has been called to merge in the base index as well.
The new fsmonitor_dirty is kept from being leaked by dint of being
cleaned up in post_read_index_from, which is guaranteed to be called
after do_read_index in read_index_from.
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
cache.h | 1 +
fsmonitor.c | 39 ---
2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/cache.h b/cache.h
index 25adcf681..0a4f43ec2 100644
--- a/cache.h
+++ b/cache.h
@@ -348,6 +348,7 @@ struct index_state {
unsigned char sha1[20];
struct untracked_cache *untracked;
uint64_t fsmonitor_last_update;
+ struct ewah_bitmap *fsmonitor_dirty;
};
extern struct index_state the_index;
diff --git a/fsmonitor.c b/fsmonitor.c
index 4ea44dcc6..417759224 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate,
const void *data,
ewah_free(fsmonitor_dirty);
return error("failed to parse ewah bitmap reading fsmonitor
index extension");
}
-
- if (git_config_get_fsmonitor()) {
- /* Mark all entries valid */
- for (i = 0; i < istate->cache_nr; i++)
- istate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;
-
- /* Mark all previously saved entries as dirty */
- ewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);
-
- /* Now mark the untracked cache for fsmonitor usage */
- if (istate->untracked)
- istate->untracked->use_fsmonitor = 1;
- }
- ewah_free(fsmonitor_dirty);
+ istate->fsmonitor_dirty = fsmonitor_dirty;
trace_printf_key(_fsmonitor, "read fsmonitor extension
successful");
return 0;
@@ -239,7 +226,29 @@ void remove_fsmonitor(struct index_state *istate)
void tweak_fsmonitor(struct index_state *istate)
{
- switch (git_config_get_fsmonitor()) {
+ int i;
+ int fsmonitor_enabled = git_config_get_fsmonitor();
+
+ if (istate->fsmonitor_dirty) {
+ if (fsmonitor_enabled) {
+ /* Mark all entries valid */
+ for (i = 0; i < istate->cache_nr; i++) {
+ istate->cache[i]->ce_flags |=
CE_FSMONITOR_VALID;
+ }
+
+ /* Mark all previously saved entries as dirty */
+ ewah_each_bit(istate->fsmonitor_dirty,
fsmonitor_ewah_callback, istate);
+
+ /* Now mark the untracked cache for fsmonitor usage */
+ if (istate->untracked)
+ istate->untracked->use_fsmonitor = 1;
+ }
+
+ ewah_free(istate->fsmonitor_dirty);
+ istate->fsmonitor_dirty = NULL;
+ }
+
+ switch (fsmonitor_enabled) {
case -1: /* keep: do nothing */
break;
case 0: /* false */
--
2.15.0.rc1.413.g76aedb451
[PATCH v3 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman
This provides modest performance savings. Benchmarking with the
following program, with and without `--no-pretty`, we find savings of
23% (0.316s -> 0.242s) in the git repository, and savings of 8% (5.24s
-> 4.86s) on a large repository with 580k files in the working copy.
#!/usr/bin/perl
use strict;
use warnings;
use IPC::Open2;
use JSON::XS;
my $pid = open2(\*CHLD_OUT, \*CHLD_IN, "watchman -j @ARGV")
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
my $query = qq|["query", "$ENV{PWD}", {}]|;
print CHLD_IN $query;
close CHLD_IN;
my $response = do {local $/; };
JSON::XS->new->utf8->decode($response);
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
templates/hooks--fsmonitor-watchman.sample | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/templates/hooks--fsmonitor-watchman.sample
b/templates/hooks--fsmonitor-watchman.sample
index 9eba8a740..9a082f278 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -49,7 +49,7 @@ launch_watchman();
sub launch_watchman {
- my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
+ my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
--
2.15.0.rc1.413.g76aedb451
Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree
On Thu, 26 Oct 2017, Ben Peart wrote: > I saw your response but actually can't replicate it locally. I've been > running with Watchman integration on all my repos for months and my "watchman > watch-list" command only shows the root of the various working directories - > no subdirectories. Weird. I double-checked and I see the same behavior with watchman 4.9.0 as with 4.7.0 that I had been using previously. I wonder if something's different between `git` in `next` from wherever your branch was based. - Alex
Re: [PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman
On Thu, 26 Oct 2017, Ben Peart wrote:
> On 10/25/2017 9:31 PM, Alex Vandiver wrote:
> > diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
> > index a3e30bf54..79f24325c 100755
> > --- a/t/t7519/fsmonitor-watchman
> > +++ b/t/t7519/fsmonitor-watchman
> > @@ -50,7 +50,7 @@ launch_watchman();
> > sub launch_watchman {
> > - my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
> > + my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
>
> Since this is a test script performance isn't critical. This version of the
> integration script logs the response to a file in .git/watchman-response.json
> and is much more human readable without the "--no-pretty." As such, I'd leave
> this one pretty.
This would be the first delta between the test file and the template
file. It seems quite important to me to attempt to ensure that we're
testing the _same_ contents that we're suggesting users set up. In
fact, it makes more sense to me to just turn this into a symlink to the
sample template.
- Alex
Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree
On Thu, 26 Oct 2017, Ben Peart wrote: > On 10/25/2017 9:31 PM, Alex Vandiver wrote: > > diff --git a/fsmonitor.c b/fsmonitor.c > > index 7c1540c05..0d26ff34f 100644 > > --- a/fsmonitor.c > > +++ b/fsmonitor.c > > @@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t > > last_update, struct strbuf *que > > argv[3] = NULL; > > cp.argv = argv; > > cp.use_shell = 1; > > +cp.dir = get_git_work_tree(); > > > > I'm not sure what would trigger this problem but I don't doubt that it is > possible. Better to be safe than sorry. This is a better/faster solution than > you're previous patch. Thank you! See my response on the v1 of this series -- I'm curious how you're _not_ seeing it, actually. Can you not replicate just by running `git status` from differing parts of the working tree? - Alex
Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree
On Wed, 25 Oct 2017, Alex Vandiver wrote: > The fsmonitor command inherits the PWD of its caller, which may be > anywhere in the working copy. This makes is difficult for the > fsmonitor command to operate on the whole repository. Specifically, > for the watchman integration, this causes each subdirectory to get its > own watch entry. > > Set the CWD to the top of the working directory, for consistency. > > Signed-off-by: Alex Vandiver <ale...@dropbox.com> > --- > fsmonitor.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fsmonitor.c b/fsmonitor.c > index 7c1540c05..0d26ff34f 100644 > --- a/fsmonitor.c > +++ b/fsmonitor.c > @@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t > last_update, struct strbuf *que > argv[3] = NULL; > cp.argv = argv; > cp.use_shell = 1; > +cp.dir = get_git_work_tree(); Looks like my editor swapped out a tab on me. I'll hold off on sending a revised version to collect any other comments. - Alex
[PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree
The fsmonitor command inherits the PWD of its caller, which may be anywhere in the working copy. This makes is difficult for the fsmonitor command to operate on the whole repository. Specifically, for the watchman integration, this causes each subdirectory to get its own watch entry. Set the CWD to the top of the working directory, for consistency. Signed-off-by: Alex Vandiver <ale...@dropbox.com> --- fsmonitor.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fsmonitor.c b/fsmonitor.c index 7c1540c05..0d26ff34f 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que argv[3] = NULL; cp.argv = argv; cp.use_shell = 1; +cp.dir = get_git_work_tree(); return capture_command(, query_result, 1024); } -- 2.15.0.rc1.413.g76aedb451
[PATCH v2 4/4] fsmonitor: Delay updating state until after split index is merged
If the fsmonitor extension is used in conjunction with the split index
extension, the set of entries in the index when it is first loaded is
only a subset of the real index. This leads to only the non-"base"
index being marked as CE_FSMONITOR_VALID.
Delay the expansion of the ewah bitmap until after tweak_split_index
has been called to merge in the base index as well.
The new fsmonitor_dirty is kept from being leaked by dint of being
cleaned up in post_read_index_from, which is guaranteed to be called
after do_read_index in read_index_from.
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
cache.h | 1 +
fsmonitor.c | 39 ---
2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/cache.h b/cache.h
index 25adcf681..0a4f43ec2 100644
--- a/cache.h
+++ b/cache.h
@@ -348,6 +348,7 @@ struct index_state {
unsigned char sha1[20];
struct untracked_cache *untracked;
uint64_t fsmonitor_last_update;
+ struct ewah_bitmap *fsmonitor_dirty;
};
extern struct index_state the_index;
diff --git a/fsmonitor.c b/fsmonitor.c
index 0d26ff34f..fad9c6b13 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate,
const void *data,
ewah_free(fsmonitor_dirty);
return error("failed to parse ewah bitmap reading fsmonitor
index extension");
}
-
- if (git_config_get_fsmonitor()) {
- /* Mark all entries valid */
- for (i = 0; i < istate->cache_nr; i++)
- istate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;
-
- /* Mark all previously saved entries as dirty */
- ewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);
-
- /* Now mark the untracked cache for fsmonitor usage */
- if (istate->untracked)
- istate->untracked->use_fsmonitor = 1;
- }
- ewah_free(fsmonitor_dirty);
+ istate->fsmonitor_dirty = fsmonitor_dirty;
trace_printf_key(_fsmonitor, "read fsmonitor extension
successful");
return 0;
@@ -239,7 +226,29 @@ void remove_fsmonitor(struct index_state *istate)
void tweak_fsmonitor(struct index_state *istate)
{
- switch (git_config_get_fsmonitor()) {
+ int i;
+ int fsmonitor_enabled = git_config_get_fsmonitor();
+
+ if (istate->fsmonitor_dirty) {
+ if (fsmonitor_enabled) {
+ /* Mark all entries valid */
+ for (i = 0; i < istate->cache_nr; i++) {
+ istate->cache[i]->ce_flags |=
CE_FSMONITOR_VALID;
+ }
+
+ /* Mark all previously saved entries as dirty */
+ ewah_each_bit(istate->fsmonitor_dirty,
fsmonitor_ewah_callback, istate);
+
+ /* Now mark the untracked cache for fsmonitor usage */
+ if (istate->untracked)
+ istate->untracked->use_fsmonitor = 1;
+ }
+
+ ewah_free(istate->fsmonitor_dirty);
+ istate->fsmonitor_dirty = NULL;
+ }
+
+ switch (fsmonitor_enabled) {
case -1: /* keep: do nothing */
break;
case 0: /* false */
--
2.15.0.rc1.413.g76aedb451
[PATCH v2 3/4] fsmonitor: Document GIT_TRACE_FSMONITOR
Signed-off-by: Alex Vandiver <ale...@dropbox.com> --- Documentation/git.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/git.txt b/Documentation/git.txt index 1fca63634..720db196e 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -594,6 +594,10 @@ into it. Unsetting the variable, or setting it to empty, "0" or "false" (case insensitive) disables trace messages. +`GIT_TRACE_FSMONITOR`:: + Enables trace messages for the filesystem monitor extension. + See `GIT_TRACE` for available trace output options. + `GIT_TRACE_PACK_ACCESS`:: Enables trace messages for all accesses to any packs. For each access, the pack file name and an offset in the pack is -- 2.15.0.rc1.413.g76aedb451
[PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman
This provides modest performance savings. Benchmarking with the
following program, with and without `--no-pretty`, we find savings of
23% (0.316s -> 0.242s) in the git repository, and savings of 8% (5.24s
-> 4.86s) on a large repository with 580k files in the working copy.
#!/usr/bin/perl
use strict;
use warnings;
use IPC::Open2;
my $pid = open2(\*CHLD_OUT, \*CHLD_IN, "watchman -j @ARGV")
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
my $query = qq|["query", "$ENV{PWD}", {}]|;
print CHLD_IN $query;
close CHLD_IN;
my $response = do {local $/; };
my $json_pkg;
eval {
require JSON::XSomething;
$json_pkg = "JSON::XSomething";
1;
} or do {
require JSON::PP;
$json_pkg = "JSON::PP";
};
my $o = $json_pkg->new->utf8->decode($response);
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
t/t7519/fsmonitor-watchman | 2 +-
templates/hooks--fsmonitor-watchman.sample | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index a3e30bf54..79f24325c 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -50,7 +50,7 @@ launch_watchman();
sub launch_watchman {
- my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
+ my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
diff --git a/templates/hooks--fsmonitor-watchman.sample
b/templates/hooks--fsmonitor-watchman.sample
index 9eba8a740..9a082f278 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -49,7 +49,7 @@ launch_watchman();
sub launch_watchman {
- my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
+ my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
--
2.15.0.rc1.413.g76aedb451
[PATCH v2 0/4] fsmonitor fixes
Updated based on comments from Dscho and Ben. Thanks for those! - Alex
Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged
On Fri, 20 Oct 2017, Johannes Schindelin wrote:
> From the diff, it is not immediately clear that fsmonitor_dirty is not
> leaked in any code path.
>
> Could you clarify this in the commit message, please?
Will do!
> > @@ -238,6 +225,29 @@ void remove_fsmonitor(struct index_state *istate)
> >
> > void tweak_fsmonitor(struct index_state *istate)
> > {
> > + int i;
> > +
> > + if (istate->fsmonitor_dirty) {
> > + /* Mark all entries valid */
> > + trace_printf_key(_fsmonitor, "fsmonitor is enabled; cache
> > is %d", istate->cache_nr);
>
> Sadly, a call to trace_printf_key() is not really a noop when tracing is
> disabled. [snip]
Apologies -- I'd meant to remove the tracing before committing. I
think we're all on the same page that it would be nice to lower the
impact of tracing to let it be more prevalent, but I'd rather not
block these changes on that.
Thanks for the comments!
- Alex
Re: [PATCH 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman
On Fri, 20 Oct 2017, Ben Peart wrote:
> > While I am very much infavor of this change (I was not aware of the
> > --no-pretty option), I would like to see some statistics on that. Could
> > you measure the impact, please, and include the results in the commit
> > message?
> >
> > Ciao,
> > Johannes
> >
>
> I was also unaware of the --no-pretty option. I've tested this on Windows
> running version 4.9.0 of Watchman and verified that it does work correctly.
> I'm also curious if it produces any measurable difference in performance.
On a repository with ~160k files, the following test harness, which
requests all files inside the repository and parses that output:
--8<---
#!/usr/bin/perl
use strict;
use warnings;
use IPC::Open2;
my $pid = open2(\*CHLD_OUT, \*CHLD_IN, "watchman -j @ARGV")
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
my $query = qq|["query", "$ENV{PWD}", {}]|;
print CHLD_IN $query;
close CHLD_IN;
my $response = do {local $/; };
my $json_pkg;
eval {
require JSON::XS;
$json_pkg = "JSON::XS";
1;
} or do {
require JSON::PP;
$json_pkg = "JSON::PP";
};
my $o = $json_pkg->new->utf8->decode($response);
--8<---
...run with dumbbench[1], produces:
$ dumbbench -- ./test.pl
cmd: Ran 22 iterations (2 outliers).
cmd: Rounded run time per iteration: 5.240e+00 +/- 1.1e-02 (0.2%)
$ dumbbench -- ./test.pl --no-pretty
cmd: Ran 21 iterations (1 outliers).
cmd: Rounded run time per iteration: 4.866e+00 +/- 1.3e-02 (0.3%)
...so a modest 8% speedup. I note that those numbers are for a perl
with JSON::XS installed; without it installed, the runtime is so long
that I gave up waiting for it.
Anyways, I'll put that in the commit message in the re-roll.
- Alex
[1] https://metacpan.org/release/Dumbbench
Re: [PATCH 1/4] fsmonitor: Watch, and ask about, the top of the repo, not the CWD
On Fri, 20 Oct 2017, Johannes Schindelin wrote:
> This is super expensive, as it means a full-blown new process instead of
> just a simple environment variable expansion.
>
> The idea behind using `PWD` instead was that Git will already have done
> all of the work of figuring out the top-level directory and switched to
> there before calling the fsmonitor hook.
I'm not seeing that PWD has been at all altered. The following does
seem like a better solution:
--8<-
diff --git a/fsmonitor.c b/fsmonitor.c
index 7c1540c05..4ea44dcc6 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t
last_update, struct strbuf *que
argv[3] = NULL;
cp.argv = argv;
cp.use_shell = 1;
+ cp.dir = get_git_work_tree();
return capture_command(, query_result, 1024);
}
--8<-
I'll re-roll with that.
> Did you see any case where the script was *not* called from the top-level
> directory?
Merely calling `git status` inside a subdirectory is enough to for the
stock watchman config to report that it's in a "new" directory:
$ watchman watch-list
{
"roots": [],
"version": "4.7.0"
}
$ git status
Adding '/Users/alexmv/src/git' to watchman's watch list.
On branch test
nothing to commit, working tree clean
$ cd builtin/
$ git status
Adding '/Users/alexmv/src/git/builtin' to watchman's watch list.
On branch test
nothing to commit, working tree clean
$ watchman watch-list
{
"roots": [
"/Users/alexmv/src/git/builtin",
"/Users/alexmv/src/git"
],
"version": "4.7.0"
}
As I understand it, that means that it then loses all performance
gains in the new directory, as it spits out "all dirty."
- Alex
[PATCH 4/4] fsmonitor: Delay updating state until after split index is merged
If the fsmonitor extension is used in conjunction with the split index
extension, the set of entries in the index when it is first loaded is
only a subset of the real index. This leads to only the non-"base"
index being marked as CE_FSMONITOR_VALID.
Delay the expansion of the ewah bitmap until after tweak_split_index
has been called to merge in the base index as well.
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
cache.h | 1 +
fsmonitor.c | 38 --
read-cache.c | 4
3 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/cache.h b/cache.h
index 25adcf681..0a4f43ec2 100644
--- a/cache.h
+++ b/cache.h
@@ -348,6 +348,7 @@ struct index_state {
unsigned char sha1[20];
struct untracked_cache *untracked;
uint64_t fsmonitor_last_update;
+ struct ewah_bitmap *fsmonitor_dirty;
};
extern struct index_state the_index;
diff --git a/fsmonitor.c b/fsmonitor.c
index 7c1540c05..4c2668f57 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate,
const void *data,
ewah_free(fsmonitor_dirty);
return error("failed to parse ewah bitmap reading fsmonitor
index extension");
}
-
- if (git_config_get_fsmonitor()) {
- /* Mark all entries valid */
- for (i = 0; i < istate->cache_nr; i++)
- istate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;
-
- /* Mark all previously saved entries as dirty */
- ewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);
-
- /* Now mark the untracked cache for fsmonitor usage */
- if (istate->untracked)
- istate->untracked->use_fsmonitor = 1;
- }
- ewah_free(fsmonitor_dirty);
+ istate->fsmonitor_dirty = fsmonitor_dirty;
trace_printf_key(_fsmonitor, "read fsmonitor extension
successful");
return 0;
@@ -238,6 +225,29 @@ void remove_fsmonitor(struct index_state *istate)
void tweak_fsmonitor(struct index_state *istate)
{
+ int i;
+
+ if (istate->fsmonitor_dirty) {
+ /* Mark all entries valid */
+ trace_printf_key(_fsmonitor, "fsmonitor is enabled; cache
is %d", istate->cache_nr);
+ for (i = 0; i < istate->cache_nr; i++) {
+ istate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;
+ }
+ trace_printf_key(_fsmonitor, "marked all as valid");
+
+ /* Mark all previously saved entries as dirty */
+ trace_printf_key(_fsmonitor, "setting each bit on %p",
istate->fsmonitor_dirty);
+ ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback,
istate);
+
+ /* Now mark the untracked cache for fsmonitor usage */
+ trace_printf_key(_fsmonitor, "setting untracked state");
+ if (istate->untracked)
+ istate->untracked->use_fsmonitor = 1;
+ ewah_free(istate->fsmonitor_dirty);
+ } else {
+ trace_printf_key(_fsmonitor, "fsmonitor not enabled");
+ }
+
switch (git_config_get_fsmonitor()) {
case -1: /* keep: do nothing */
break;
diff --git a/read-cache.c b/read-cache.c
index c18e5e623..3b5cd00a2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -330,6 +330,10 @@ int ie_match_stat(struct index_state *istate,
return 0;
if (!ignore_fsmonitor && (ce->ce_flags & CE_FSMONITOR_VALID))
return 0;
+ if (ce->ce_flags & CE_FSMONITOR_VALID)
+ trace_printf_key(_fsmonitor, "fsmon marked valid for %s",
ce->name);
+ if (ignore_fsmonitor)
+ trace_printf_key(_fsmonitor, "Ignoring fsmonitor for %s",
ce->name);
/*
* Intent-to-add entries have not been added, so the index entry
--
2.15.0.rc1.417.g05670bb6e
[PATCH 1/4] fsmonitor: Watch, and ask about, the top of the repo, not the CWD
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
t/t7519/fsmonitor-watchman | 3 ++-
templates/hooks--fsmonitor-watchman.sample | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index a3e30bf54..377edc7be 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -41,7 +41,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;
} else {
- $git_work_tree = $ENV{'PWD'};
+ $git_work_tree = `git rev-parse --show-toplevel`;
+ chomp $git_work_tree;
}
my $retry = 1;
diff --git a/templates/hooks--fsmonitor-watchman.sample
b/templates/hooks--fsmonitor-watchman.sample
index 9eba8a740..7df590d29 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -40,7 +40,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;
} else {
- $git_work_tree = $ENV{'PWD'};
+ $git_work_tree = `git rev-parse --show-toplevel`;
+ chomp $git_work_tree;
}
my $retry = 1;
--
2.15.0.rc1.417.g05670bb6e
[PATCH 0/4] fsmonitor fixes
A few fixes found from playing around with the fsmonitor branch in next. - Alex
[PATCH 3/4] fsmonitor: Document GIT_TRACE_FSMONITOR
Signed-off-by: Alex Vandiver <ale...@dropbox.com> --- Documentation/git.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/git.txt b/Documentation/git.txt index 1fca63634..720db196e 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -594,6 +594,10 @@ into it. Unsetting the variable, or setting it to empty, "0" or "false" (case insensitive) disables trace messages. +`GIT_TRACE_FSMONITOR`:: + Enables trace messages for the filesystem monitor extension. + See `GIT_TRACE` for available trace output options. + `GIT_TRACE_PACK_ACCESS`:: Enables trace messages for all accesses to any packs. For each access, the pack file name and an offset in the pack is -- 2.15.0.rc1.417.g05670bb6e
[PATCH 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman
This provides small performance savings.
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
t/t7519/fsmonitor-watchman | 2 +-
templates/hooks--fsmonitor-watchman.sample | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index 377edc7be..eba46c78b 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -51,7 +51,7 @@ launch_watchman();
sub launch_watchman {
- my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
+ my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
diff --git a/templates/hooks--fsmonitor-watchman.sample
b/templates/hooks--fsmonitor-watchman.sample
index 7df590d29..60eb7e70b 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -50,7 +50,7 @@ launch_watchman();
sub launch_watchman {
- my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
+ my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
--
2.15.0.rc1.417.g05670bb6e
Re: [RFC] Reporting index properties
On Fri, 6 Oct 2017, Junio C Hamano wrote: > Yes, and I am saying that such logic should not live in standard > install outside developer tools ;-) Fair enough. It seems a little odd to me that git provides logic for _altering_ those bits, but not to report on the state that can be so altered. - Alex
Re: [RFC] Reporting index properties
On Fri, 6 Oct 2017, Junio C Hamano wrote: > > Do folks have feelings about surfacing this information, and where such > > logic should live? > > Probably one of the t/helper/test-dump-*.c programs, if we already > do not have one. The goal would be to be able to extract this information from repositories, with a standard git install. That directory only contains developer tools, which aren't part of the install, no? - Alex
[RFC] Reporting index properties
Heya, As part of gathering some automated performance statistics of git, it would be useful to be able to extract some vital properties of the index. While `git update-index` has ways to _set_ the index version, and enable/disable various extensions, AFAICT there is no method by which one can ask for reporting about the current state of them -- short of re-implementing all of the index-parsing logic external to Git, which is not terribly appealing. I hesitate to propose adding a flag to `git update-index` which reads but does no updating, as that seems to make a misnomer of the command. But this also doesn't seem worthy of a new top-level command. Do folks have feelings about surfacing this information, and where such logic should live? - Alex
Re: [PATCH v8 00/12] Fast git status via a file system watcher
On Wed, 4 Oct 2017, Junio C Hamano wrote:
> Rats indeed. Let's go incremental as promised, perhaps like this
> (but please supply a better description if you have one).
I think you'll also want the following squashed into 5c8cdcfd8 and
def437671:
-- >8 --
>From 445d45027bb5b7823338cf111910d2884af6318b Mon Sep 17 00:00:00 2001
From: Alex Vandiver <ale...@dropbox.com>
Date: Tue, 3 Oct 2017 23:27:46 -0700
Subject: [PATCH] fsmonitor: Read entirety of watchman output
In perl, setting $/ sets the string that is used as the "record
separator," which sets the boundary that the `<>` construct reads to.
Setting `local $/ = 0666;` evaluates the octal, getting 438, and
stringifies it. Thus, the later read from `` stops as soon
as it encounters the string "438" in the watchman output, yielding
invalid JSON; repositories containing filenames with SHA1 hashes are
able to trip this easily.
Set `$/` to undefined, thus slurping all output from watchman. Also
close STDIN which is provided to watchman, to better guarantee that we
cannot deadlock with watchman while both attempting to read.
Signed-off-by: Alex Vandiver <ale...@dropbox.com>
---
t/t7519/fsmonitor-watchman | 6 ++
templates/hooks--fsmonitor-watchman.sample | 6 ++
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index 7ceb32dc1..7d6aef635 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -50,9 +50,6 @@ launch_watchman();
sub launch_watchman {
- # Set input record separator
- local $/ = 0666;
-
my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
@@ -83,7 +80,8 @@ sub launch_watchman {
close $fh;
print CHLD_IN $query;
- my $response = ;
+ close CHLD_IN;
+ my $response = do {local $/; };
open ($fh, ">", ".git/watchman-response.json");
print $fh $response;
diff --git a/templates/hooks--fsmonitor-watchman.sample
b/templates/hooks--fsmonitor-watchman.sample
index 870a59d23..1b8ed173e 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -49,9 +49,6 @@ launch_watchman();
sub launch_watchman {
- # Set input record separator
- local $/ = 0666;
-
my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
@@ -78,7 +75,8 @@ sub launch_watchman {
END
print CHLD_IN $query;
- my $response = ;
+ close CHLD_IN;
+ my $response = do {local $/; };
die "Watchman: command returned no output.\n" .
"Falling back to scanning...\n" if $response eq "";
--
2.14.2.959.g6663358d3
Re: [PATCH v3] builtin/log: honor log.decorate
2017-05-14 20:35 GMT-06:00 Junio C Hamano <gits...@pobox.com>: > "brian m. carlson" <sand...@crustytoothpaste.net> writes: > >> The recent change that introduced autodecorating of refs accidentally >> broke the ability of users to set log.decorate = false to override it. >> When the git_log_config was traversed a second time with an option other >> than log.decorate, the decoration style would be set to the automatic >> style, even if the user had already overridden it. Instead of setting >> the option in config parsing, set it in init_log_defaults instead. >> >> Add a test for this case. The actual additional config option doesn't >> matter, but it needs to be something not already set in the >> configuration file. >> >> Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net> >> --- >> Changes from v2: >> * Add a test. I tested that the config parsing both works with >> additional options and also can be overridden from the command line. > > Thanks, all. > > Will queue with Acked-by by Alex and Reviewed-by by Jonathan. That sounds great, thank you. -Alex
Re: [PATCH v2] builtin/log: honor log.decorate
2017-05-12 17:48 GMT-06:00 Jonathan Nieder <jrnie...@gmail.com>: > Hi, > > brian m. carlson wrote: > >> Does anyone else have views on whether this is good thing to test for? > > I know you don't mean to be rude, but this comes across as a bit of > a dismissive question. The question sounded neutral to me. >> On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote: >>> brian m. carlson wrote: > >>>> The recent change that introduced autodecorating of refs accidentally >>>> broke the ability of users to set log.decorate = false to override it. >>> >>> Yikes. It sounds to me like we need a test to ensure we don't regress >>> it again later. >> >> I can add one, but it's going to be a bit odd. The issue is that as far >> as I can tell, the option is honored only if it's the last one read, so >> it necessarily has to be in the per-repository configuration. >> >> I'm not sure it makes that much sense to add a test for this case. Do >> we generally want to write such tests for all config options? I don't >> suppose it's that common a mistake to make. > > In my humble opinion, the bug being subtle makes it especially useful > to have a test that describes that bug. That way, if someone is > refactoring this code later then they know what to watch out for not > reintroducing. > > I'm happy to hear other opinions, especially if they come with data or > a principle attached that I can use when writing future patches of my > own. When I saw Brian's email today, my first thought was "What was I thinking?" My mistake was pretty obvious. Then I remembered that when I wrote the original patch, I wasn't sure where to set the default value, because there were no clear examples in this file. Now that we've established a clear precedent for setting the log.decorate default (and other defaults like it) in init_log_defaults, I don't expect any more problems with log.decorate. And since it's not practical to add tests for similar bugs for every command and configuration option in Git, we'll just have to be a little more vigilant about code review. Again, I apologize for the trouble. -Alex
Re: [PATCH v2] builtin/log: honor log.decorate
2017-05-12 16:12 GMT-06:00 brian m. carlson <sand...@crustytoothpaste.net>:
> The recent change that introduced autodecorating of refs accidentally
> broke the ability of users to set log.decorate = false to override it.
> When the git_log_config was traversed a second time with an option other
> than log.decorate, the decoration style would be set to the automatic
> style, even if the user had already overridden it. Instead of setting
> the option in config parsing, set it in init_log_defaults instead.
>
> Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
> ---
> builtin/log.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1e..ec3258368 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -110,6 +110,8 @@ static void init_log_defaults(void)
> {
> init_grep_defaults();
> init_diff_ui_defaults();
> +
> + decoration_style = auto_decoration_style();
> }
>
> static void cmd_log_init_defaults(struct rev_info *rev)
> @@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char
> *value, void *cb)
> if (decoration_style < 0)
> decoration_style = 0; /* maybe warn? */
> return 0;
> - } else {
> - decoration_style = auto_decoration_style();
> }
> if (!strcmp(var, "log.showroot")) {
> default_show_root = git_config_bool(var, value);
Signed-off-by: Alex Henrie <alexhenri...@gmail.com>
Re: [PATCH] builtin/log: honor log.decorate
2017-05-12 15:34 GMT-06:00 brian m. carlson <sand...@crustytoothpaste.net>:
> The recent change that introduced autodecorating of refs accidentally
> broke the ability of users to set log.decorate = false to override it.
> When the git_log_config was traversed a second time with an option other
> than log.decorate, the decoration style would be set to the automatic
> style, even if the user had already overridden it. Only set the option
> to its default value if we haven't set it already.
>
> Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
> ---
> builtin/log.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1e..304923836 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -34,7 +34,7 @@ static int default_abbrev_commit;
> static int default_show_root = 1;
> static int default_follow;
> static int default_show_signature;
> -static int decoration_style;
> +static int decoration_style = -1;
> static int decoration_given;
> static int use_mailmap_config;
> static const char *fmt_patch_subject_prefix = "PATCH";
> @@ -410,7 +410,7 @@ static int git_log_config(const char *var, const char
> *value, void *cb)
> if (decoration_style < 0)
> decoration_style = 0; /* maybe warn? */
> return 0;
> - } else {
> + } else if (decoration_style == -1) {
> decoration_style = auto_decoration_style();
> }
> if (!strcmp(var, "log.showroot")) {
Sorry for the mistake. On second thought, I think we should set
decoration_style = auto_decoration_style() in init_log_defaults.
-Alex
Re: [PATCH] Correct compile errors when DEBUG_BISECT=1 after supporting other hash algorithms
Any news about this patch? 2017-03-21 22:24 GMT+01:00 Alex Hoffman <s...@gal.ro>: > Hi, Brian, > > We definitely prefer the wrapper function oid_to_hex() to > sha1_to_hex(). Thanks for feedback. > Below is the updated patch: > > --- > bisect.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/bisect.c b/bisect.c > index 30808cadf..7b65acbcd 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -131,7 +131,7 @@ static void show_list(const char *debug, int > counted, int nr, > unsigned flags = commit->object.flags; > enum object_type type; > unsigned long size; > - char *buf = read_sha1_file(commit->object.sha1, , ); > + char *buf = read_sha1_file(commit->object.oid.hash, > , ); > const char *subject_start; > int subject_len; > > @@ -143,10 +143,10 @@ static void show_list(const char *debug, int > counted, int nr, > fprintf(stderr, "%3d", weight(p)); > else > fprintf(stderr, "---"); > - fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.sha1)); > + fprintf(stderr, " %.*s", 8, oid_to_hex(>object.oid)); > for (pp = commit->parents; pp; pp = pp->next) > fprintf(stderr, " %.*s", 8, > - sha1_to_hex(pp->item->object.sha1)); > + oid_to_hex(>item->object.oid)); > > subject_len = find_commit_subject(buf, _start); > if (subject_len) > -- > 2.12.0.400.g54ad2d445.dirty
[PATCH v2] log: if --decorate is not given, default to --decorate=auto
Signed-off-by: Alex Henrie <alexhenri...@gmail.com>
---
builtin/log.c | 9 -
t/t4202-log.sh | 10 +-
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 281af8c1e..d755a5960 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -52,6 +52,11 @@ struct line_opt_callback_data {
struct string_list args;
};
+static int auto_decoration_style(void)
+{
+ return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
+}
+
static int parse_decoration_style(const char *var, const char *value)
{
switch (git_config_maybe_bool(var, value)) {
@@ -67,7 +72,7 @@ static int parse_decoration_style(const char *var, const char
*value)
else if (!strcmp(value, "short"))
return DECORATE_SHORT_REFS;
else if (!strcmp(value, "auto"))
- return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
+ return auto_decoration_style();
return -1;
}
@@ -405,6 +410,8 @@ static int git_log_config(const char *var, const char
*value, void *cb)
if (decoration_style < 0)
decoration_style = 0; /* maybe warn? */
return 0;
+ } else {
+ decoration_style = auto_decoration_style();
}
if (!strcmp(var, "log.showroot")) {
default_show_root = git_config_bool(var, value);
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 48b55bfd2..f57799071 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -4,6 +4,7 @@ test_description='git log'
. ./test-lib.sh
. "$TEST_DIRECTORY/lib-gpg.sh"
+. "$TEST_DIRECTORY/lib-terminal.sh"
test_expect_success setup '
@@ -520,7 +521,7 @@ test_expect_success 'log --graph with merge' '
'
test_expect_success 'log.decorate configuration' '
- git log --oneline >expect.none &&
+ git log --oneline --no-decorate >expect.none &&
git log --oneline --decorate >expect.short &&
git log --oneline --decorate=full >expect.full &&
@@ -576,6 +577,13 @@ test_expect_success 'log.decorate configuration' '
'
+test_expect_success TTY 'log output on a TTY' '
+ git log --oneline --decorate >expect.short &&
+
+ test_terminal git log --oneline >actual &&
+ test_cmp expect.short actual
+'
+
test_expect_success 'reflog is expected format' '
git log -g --abbrev-commit --pretty=oneline >expect &&
git reflog >actual &&
--
2.12.0
Re: [PATCH] log: if --decorate is not given, default to --decorate=auto
2017-03-23 12:03 GMT-06:00 Junio C Hamano <gits...@pobox.com>: > Alex Henrie <alexhenri...@gmail.com> writes: > >> Yes, that makes sense. I assume that when you talk about 'next', you >> mean 'master'? > > No, I do mean 'next'. See "A note from the maintainer" post that > are sent to the list every once in a while (i.e. after a new release > is tagged) for the project structure. > > https://public-inbox.org/git/xmqqy3vztypi@gitster.mtv.corp.google.com/ That document was very helpful, thanks. >> If we want to use `git -p log` in a test, we'll have to change the >> behavior of pager_in_use(). Alternatively, we could use >> `GIT_PAGER_IN_USE=1 git log` instead. > > Testing "git -p" is not the goal; testing that decorate defaults to > auto during an interactive session is. We could run tests under pty > like t7006 does using lib-terminal.sh if we really want to emulate > an interactive session. > > Exporting GIT_PAGER_IN_USE may be sufficient for the purpose of > convincing the command to be in an interactive session for this > test, even though it feels a bit too brittle to depend on the > internal implementation detail. Okay, I've now written a lib-terminal test for git log. I'll send the revised patch momentarily. -Alex
Re: [PATCH] log: if --decorate is not given, default to --decorate=auto
2017-03-23 9:54 GMT-06:00 Junio C Hamano <gits...@pobox.com>: > Alex Henrie <alexhenri...@gmail.com> writes: > >> 2017-03-22 10:54 GMT-06:00 Junio C Hamano <gits...@pobox.com>: >>> Alex Henrie <alexhenri...@gmail.com> writes: >>>> No problem. Do I need to submit a second version of the patch with a >>>> test for `git -p log`? >>> >>> You do want to protect this "without an option, we default to >>> 'auto'" feature from future breakage, no? >> >> Yes, but I need to know whether you want a v2 of this patch with all >> of the changes including the new test, or a second patch that depends >> on the first patch and only adds the new test. > > Sorry, I misunderstood the question. > > In general, we prefer to have tests that protects the updated > behaviour in the same patch that makes code changes that brings in > the new behaviour, i.e. a single v2 patch with new test would be > more appropriate in this case. > > When people work on a large bugfix, especially one that needs > multiple steps, we sometimes see a patch that adds new tests that > describe the desired behaviour as failing tests first, and then > subsequent patches to the code to update the behaviour flip > "test_expect_failure" to "test_expect_success" as they fix the > behaviour. But for a small change like this one, that approach is > inappropriate. > > When a patch that was reviewed, deemed good enough and has been > already merged to the 'next' branch later turns out that it needs > further work (like "we do need some tests"), we do such necessary > updates as separate follow-up patches, simply because we promise > that 'next' won't be rewound and are not allowed to replace patches. > But this one is not yet in 'next', so we can freely take a > replacement patch. > > Hope this message makes it clear enough? Yes, that makes sense. I assume that when you talk about 'next', you mean 'master'? Unfortunately, I think I found a bug. Even when using `git -p`, the function pager_in_use() always returns false if the output is not a TTY. So, `isatty(1) || pager_in_use()` and `color_stdout_is_tty || (pager_in_use() && pager_use_color)` are redundant. If we want to use `git -p log` in a test, we'll have to change the behavior of pager_in_use(). Alternatively, we could use `GIT_PAGER_IN_USE=1 git log` instead. -Alex
Re: [PATCH] log: if --decorate is not given, default to --decorate=auto
2017-03-22 10:54 GMT-06:00 Junio C Hamano <gits...@pobox.com>: > Alex Henrie <alexhenri...@gmail.com> writes: >> No problem. Do I need to submit a second version of the patch with a >> test for `git -p log`? > > You do want to protect this "without an option, we default to > 'auto'" feature from future breakage, no? Yes, but I need to know whether you want a v2 of this patch with all of the changes including the new test, or a second patch that depends on the first patch and only adds the new test. -Alex
Re: [PATCH] log: if --decorate is not given, default to --decorate=auto
2017-03-21 16:28 GMT-06:00 Junio C Hamano <gits...@pobox.com>:
> Junio C Hamano <gits...@pobox.com> writes:
>
>>> test_expect_success 'log.decorate configuration' '
>>> -git log --oneline >expect.none &&
>>> +git log --oneline --no-decorate >expect.none &&
>>> git log --oneline --decorate >expect.short &&
>>> git log --oneline --decorate=full >expect.full &&
>>
>> This ensures that an explicit --no-decorate from the command line
>> does give "none" output, which we failed to do so far, and is a good
>> change. Don't we also need a _new_ test to ensure that "auto" kicks
>> in without any explicit request? Knowing the implementation that
>> pager-in-use triggers the "auto" behaviour, perhaps testing the
>> output from "git -p log" would be sufficient?
>
> BTW,
>
>>
>> +static int auto_decoration_style()
>> +{
>> + return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
>> +}
>
> FYI, I fixed this to
>
> static int auto_decoration_style(void)
>
> while queuing to make it compile.
No problem. Do I need to submit a second version of the patch with a
test for `git -p log`?
-Alex
Re: [PATCH] Correct compile errors when DEBUG_BISECT=1 after supporting other hash algorithms
Hi, Brian, We definitely prefer the wrapper function oid_to_hex() to sha1_to_hex(). Thanks for feedback. Below is the updated patch: --- bisect.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bisect.c b/bisect.c index 30808cadf..7b65acbcd 100644 --- a/bisect.c +++ b/bisect.c @@ -131,7 +131,7 @@ static void show_list(const char *debug, int counted, int nr, unsigned flags = commit->object.flags; enum object_type type; unsigned long size; - char *buf = read_sha1_file(commit->object.sha1, , ); + char *buf = read_sha1_file(commit->object.oid.hash, , ); const char *subject_start; int subject_len; @@ -143,10 +143,10 @@ static void show_list(const char *debug, int counted, int nr, fprintf(stderr, "%3d", weight(p)); else fprintf(stderr, "---"); - fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.sha1)); + fprintf(stderr, " %.*s", 8, oid_to_hex(>object.oid)); for (pp = commit->parents; pp; pp = pp->next) fprintf(stderr, " %.*s", 8, - sha1_to_hex(pp->item->object.sha1)); + oid_to_hex(>item->object.oid)); subject_len = find_commit_subject(buf, _start); if (subject_len) -- 2.12.0.400.g54ad2d445.dirty
[PATCH] log: if --decorate is not given, default to --decorate=auto
Git's branching and merging system can be confusing, especially to new
users. When teaching people Git, I tell them to set log.decorate=auto.
This preference greatly improves the user's awareness of the local and
remote branches. So for the sake of user friendliness, I'd like to
change the default from log.decorate=no to log.decorate=auto.
Signed-off-by: Alex Henrie <alexhenri...@gmail.com>
---
builtin/log.c | 9 -
t/t4202-log.sh | 2 +-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 281af8c1e..ddb4515dc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -52,6 +52,11 @@ struct line_opt_callback_data {
struct string_list args;
};
+static int auto_decoration_style()
+{
+ return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
+}
+
static int parse_decoration_style(const char *var, const char *value)
{
switch (git_config_maybe_bool(var, value)) {
@@ -67,7 +72,7 @@ static int parse_decoration_style(const char *var, const char
*value)
else if (!strcmp(value, "short"))
return DECORATE_SHORT_REFS;
else if (!strcmp(value, "auto"))
- return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
+ return auto_decoration_style();
return -1;
}
@@ -405,6 +410,8 @@ static int git_log_config(const char *var, const char
*value, void *cb)
if (decoration_style < 0)
decoration_style = 0; /* maybe warn? */
return 0;
+ } else {
+ decoration_style = auto_decoration_style();
}
if (!strcmp(var, "log.showroot")) {
default_show_root = git_config_bool(var, value);
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 48b55bfd2..3aa8daba3 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -520,7 +520,7 @@ test_expect_success 'log --graph with merge' '
'
test_expect_success 'log.decorate configuration' '
- git log --oneline >expect.none &&
+ git log --oneline --no-decorate >expect.none &&
git log --oneline --decorate >expect.short &&
git log --oneline --decorate=full >expect.full &&
--
2.12.0
[PATCH] Correct compile errors when DEBUG_BISECT=1 after supporting other hash algorithms
--- bisect.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bisect.c b/bisect.c index 30808cadf..6feed8533 100644 --- a/bisect.c +++ b/bisect.c @@ -131,7 +131,7 @@ static void show_list(const char *debug, int counted, int nr, unsigned flags = commit->object.flags; enum object_type type; unsigned long size; - char *buf = read_sha1_file(commit->object.sha1, , ); + char *buf = read_sha1_file(commit->object.oid.hash, , ); const char *subject_start; int subject_len; @@ -143,10 +143,10 @@ static void show_list(const char *debug, int counted, int nr, fprintf(stderr, "%3d", weight(p)); else fprintf(stderr, "---"); - fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.sha1)); + fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.oid.hash)); for (pp = commit->parents; pp; pp = pp->next) fprintf(stderr, " %.*s", 8, - sha1_to_hex(pp->item->object.sha1)); + sha1_to_hex(pp->item->object.oid.hash)); subject_len = find_commit_subject(buf, _start); if (subject_len) -- 2.12.0.399.g9d77b0405.dirty
Re: Git bisect does not find commit introducing the bug
> isn't that spelt `--ancestry-path` ? > (--ancestry-path has it's own issues such as needing an --first-parent-show > option, but that's possibly a by the by) Indeed it is spelled `--ancestry-path`. And interestingly enough you may use it multiple times with the wanted effect in our case (e.g when the user has multiple good commit and a single bad commit before running the bisect itself). Also it is `--first-parent` (not `--first-parent-show`), but I do not understand why do we need this option? What kind of issues does `--ancestry-path` have? Best regards, VG
Re: Git bisect does not find commit introducing the bug
> If `git bisect` is/would be affected by `git log` history-related options > then this is what `--strict-ancestor` option gives/would give. Exactly my thoughts. All that needs to be changed in the 2nd problem is the graph where to search. But first we must agree about the usefulness of the 2nd problem. Any thoughts/comments/votes for/against?
Re: Git bisect does not find commit introducing the bug
I see two different problems each with a different assumption (see the definition of "bisectable" in the email of Junio C Hamano): 1. (Current) Assume the entire history graph is bisectable. DO: Search where in the entire graph the first 'trait'/transition occurs. 2. (New) Assume only the graph between one good commit and one bad commit is bisectable. DO: Search where the first transition occurs in the graph between these two commits. It seems that the real world needs a solution also for the second problem (example if the good commit is the FIRST good commit of a feature or if the good commit is not the first good commit, but you definitely know, that it broke first somewhere in between the good and bad commit). I find the way to go as Oleg proposed is gittish enough (with a new parameter --strategy). Beside I would underline that also the second problem is a bisect problem, just for another graph, thus it makes perfect sense to extend 'git bisect' for this. Does that look reasonably?
Re: Git bisect does not find commit introducing the bug
> Then you must adjust your definition of "good": All commits that do not have > the feature, yet, are "good": since they do not have the feature in the > first place, they cannot have the breakage that you found in the feature. > > That is exactly the situation in your original example! But you constructed > the condition of goodness in such a simplistic way (depending on the > presence of a string), that it was impossible to distinguish between "does > not have the feature at all" and "has the feature, but it is broken". Johannes, thank you for correctly identifying the error in my logic. Indeed I was using the term 'bad' also for the commit without the feature. In order to find the commit introducing the bug in my example a new state is needed, which would make 'git bisect' a bit more complicated than the user 'most of the time' probably needs. Or do you think, it would make sense to ask the user for this state (if e.g 'git bisect' would be started with a new parameter)?
Re: Git bisect does not find commit introducing the bug
Below is a correction of the first proposed algorithm: >--o1--o2--o3--G --X1 >\\ > x1--x2--x3--x4--X2--B-- > \ / > y1--y2--y3 > Step 1a. (Unchanged) keep only the commits that: a) are ancestor of the "bad" commit (including the "bad" commit itself), b) are not ancestor of a "good" commit (excluding the "good" commits). The following graph results: x1--x2--x3--x4--X2--B-- \ / y1--y2--y3 Step 1b. (New) Mark all commits of the resulting graph as unconfirmed (unconfirmed=node without good ancestors). Mark as confirmed all descendants of commits that user marked explicitly as good (e.g if user already marked its parent/grand parent/... as good right before starting bisect run). Step 1c. From all unconfirmed root commits build a set SET_GOOD of those with any good parents in the original graph (root commit = commit without parents in the resulting graph) and one SET_BAD for commits with only BAD parents. To build a set means to ask explicitly the user (or the command passed in git bisect run) whether any of its parents is good. In the example above find out whether x1 has any good parents or no parent at all and if so add it to SET_GOOD, otherwise to SET_BAD. Mark as confirmed each commit in SET_GOOD and all its descendants. For every commit in SET_BAD delete all paths from it until to the next confirmed commit. In the example above if x1 is in SET_BAD delete the path x1-x3 and x1-y3. If any new root commits result (commit x4), redo step 1c with them. Step2. Continue the existing algorithm. If this improvement works (i.e you do not find any bugs in it and it is feasible to implement, which seems to me) the following would be its advantages: 1. An assumption less, as we explicitly check the assumption. 2. It might be quicker, because we delete parts of graph that cannot contain transitions. 3. It returns more exact results. VG
Re: Git bisect does not find commit introducing the bug
> At the end of the git-bisect man page (in the SEE ALSO section) there > is a link to > https://github.com/git/git/blob/master/Documentation/git-bisect-lk2009.txt > which has a lot of details about how bisect works. > Thanks for pointing out the SEE ALSO section. I think it makes sense to include/describe the entire algorithm in the man page itself, although I am not sure whether the graphs would be always correctly visually represented in the man page format. > The goal is to find the first bad commit, which is a commit that has > only good parents. OK, bisect's mission is more exact than I thought, which is good. M > As o1 is an ancestor of G, then o1 is considered good by the bisect algorithm. > If it was bad, it would means that there is a transition from bad to > good between o1 and G. > But when a good commit is an ancestor of the bad commit, git bisect > makes the assumption that there is no transition from bad to good in > the graph. The assumption that there is no transition from bad to good in the graph did not hold in my example and it does not hold when a feature was recently introduced and gets broken relative shortly afterwards. But I consider it is easy to change the algorithm not to assume, but rather to check it. > git bisect makes some assumptions that are true most of the time, so > in practice it works well most of the time. Whatever the definition of "most of the time" everyone might have, I think there is room for improvement. Below I am trying to make a small change to the current algorithm in order to deal with the assumption that sometimes does not hold (e.g in my example), by explicitly validating the check. > --o1--o2--o3--G--X1 > \\ > x1--x2--x3--x4--X2--B-- > \ / >y1--y2--y3 Step 1a. (Unchanged) keep only the commits that: a) are ancestor of the "bad" commit (including the "bad" commit itself), b) are not ancestor of a "good" commit (excluding the "good" commits). The following graph results: x1--x2--x3--x4--X2--B-- \ / y1--y2--y3 Step 1b. (New) Mark all root commits of the resulting graph (i.e commits without parents) as unconfirmed (unconfirmed=node that has only bad parents). Remove all root commits that user already confirmed (e.g if user already marked its parent as good right before starting bisect run). For every unconfirmed root commit check if it has any good parents. In the example above check whether x1 has good parents. If the current root element has any parents and none of them is good, we can delete all paths from it until to the next commit that has a parent in the ancestors of GOOD. In the example above to delete the path x1-x3 and x1-y3. Also add new resulting root commits to the list of unconfirmed commits (commit x4). Otherwise mark it as confirmed. Step2. Continue the existing algorithm. If this improvement works (i.e you do not find any bugs in it and it is feasible to implement, which seems to me) the following would be its advantages: 1. An assumption less, as we explicitly check the assumption. 2. It might be quicker, because we delete parts of graph that cannot contain transitions. 3. It returns more exact results. VG
Re: Git bisect does not find commit introducing the bug
> But this is not how Git works. Git computes graph differences, i.e., it > subtracts from the commits reachable from v.bad those that are reachable > from v.good. This leaves more than just those on some path from v.good to > v.bad. And it should work this way. Consider this history: > > --o--o--o--G--X >\ \ > x--x--x--x--X--B-- > > When you find B bad and G good, than any one of the x or X may have > introduced the breakage, not just one of the X. > Thank you for clarifying how git bisect works. How did you find that out? Did you check the source code? If that is not documented in the man page may be it worth documenting it in order to avoid future confusion for other users. Let's consider your example with distinct names for nodes: --o1--o2--o3--G--X1 \\ x1--x2--x3--x4--X1--B-- It makes sense that git bisect is expecting a single transition, as this is a precondition for a binary search to work. My definition of "the transition" is a commit with at least one of its parents as a good version, but the commit itself a bad version. I hope we agree that git bisect's mission is to search for this transition (as I suppose that most of people need such a functionality from git, if not exactly from git bisect). How could be x1 or x3 be the transition, if chances are that o1 is not a good version? Of course it would make sense to me if bisect would check o1 whether good and only then to check also x1-x3, but this is not what git makes (at least not in my initial example). If you consider that git bisect's mission is different from finding the transition, could you please explain what exact commit git bisect is supposed to return (ideally with terms from the graph theory) and when it makes sense to return that? Because I do not see any sense in looking in the path x1-x3 without knowing that those commits may be a transition. > Oh, IMO git bisect was well thought through. If it considered just paths > from good to bad, it would not given the correct answer. See the example > history above. Bisect authors would not have deemed that sufficiently good You definitely convinced me that git MUST search more than only in the paths between good and bad commits, as the good commit G does not have to be the first good commit (thank you for that). My problem/confusion is that it returns something that does not make sense to me, because it does not make sure it returns a transition. VG PS: thank you for continuing this discussion.
Re: Git bisect does not find commit introducing the bug
>> First of all this is confusing, as this commit cannot be reached >> starting from "v.good". > Hm, IMHO it shows that your example is pretty artificial (although you > might have come across it in a real-world scenario): you introduced a > new feature in f4154e9 (and it worked) and you broke that feature by > making the merge 671cec2. However, the feature (that broke in 671cec2) > did not even exist in 04c6f4b; so a test on the feature would not fail > (leading to "bisect bad" as in the example), it would not exist (leading > to "bisect skip"). No one commented the fact, that I find this very confusing. Don't you find this confusing? I will underline, that 'git bisect good v.good' will fail if the commit 'v.good' is not a parent of the bad commit, meaning there MUST be at least a path between 'v.good' and 'v.bad', thus I would expect it looks on this path ONLY. Beside that, this is what I understand by 'binary search' (to search on this commit path). You might find this example artificial, but I doubt git is/was intentionally designed to work with 'natural' examples only (no matter how you define 'natural' and 'artificial'). >> In other words: bisect assumes that your repo is usually in a good state >> and you have a commit that changes it to a bad state. In your case you >> have a repo that is in a bad state and you have a commit that switches >> it to a good state and later you merge a bad-state branch and you have a >> bad state again. It is not made for that use-case, I think. > Correct. The assumption of bisection is that there is only one transition > between GOOD and BAD. By violating that assumption, anything can happen. I did not find that in the manpage or did I miss it? Why would someone assume that the commit graph looks in a certain way? I assume, that 'git bisect' was not thought through and that it considers the first directed path between v.good and v.bad, instead of all paths (in my example graph there are two such paths). I will also underline that git bisect was designed to work with multiple good commits and one bad commit (also multiple paths), but probably NOT with multiple paths between the same pair of good and bad commits. VG 2017-02-18 10:12 GMT+01:00 Johannes Sixt <j...@kdbg.org>: > Am 18.02.2017 um 00:21 schrieb Stephan Beyer: >> >> On 02/17/2017 11:29 PM, Alex Hoffman wrote: >> * 7a9e952 (bisect bad) >> |\ >> | * 671cec2 <--- expected >> | |\ >> | * | 04c6f4b <--- found >> * | | 3915157 >> |\ \ \ >> | | |/ >> | |/| >> | * | f4154e9 (bisect good) >> | * | 85855bf >> | |/ >> * | f1a36f5 >> |/ >> * 1b7fb88 >> >> The and markers are set by your definition of what good and >> what bad commits are. >> >> [...] >> In other words: bisect assumes that your repo is usually in a good state >> and you have a commit that changes it to a bad state. In your case you >> have a repo that is in a bad state and you have a commit that switches >> it to a good state and later you merge a bad-state branch and you have a >> bad state again. It is not made for that use-case, I think. > > > Correct. The assumption of bisection is that there is only one transition > between GOOD and BAD. By violating that assumption, anything can happen. > > -- Hannes >
Git bisect does not find commit introducing the bug
Hi there, According to the documentation "git bisect" is designed "to find the commit that introduced a bug" . I have found a situation in which it does not returns the commit I expected. In order to reproduce the problem: 1. mkdir test; cd test; git clone https://github.com/entbugger/git-bisect-issue.git cd git-bisect-issue The tag "v.bad" is one bad version and tag "v.good" is the first good version. A good version is one having the line "FEATURE2" in file1.txt, which was introduced in "v.good". 2. Copy test scripts to another folder to make sure they are not overridden by 'git bisect' cp *.sh ../ cd .. ./search-bug-git.sh 3. Run ./search-bug-git.sh to search for the commit introducing the bug. It finds that the commit 04c6f4b9ed with the message "Feature 1" is the first one introducing the bug. First of all this is confusing, as this commit cannot be reached starting from "v.good". Then I expected the commit with the message "Introduce bug" to be returned by 'git bisect', as it is the first commit between "v.good" and "v.bad" that does not contain the line "FEATURE2" in file1.txt, which is what I understand by the commit "that introduced a bug" (cited from the manpage of git bisect). Thanks for looking to this, Best regards, VG
[PATCH] clone,fetch: explain the shallow-clone option a little more clearly
"deepen by excluding" does not make sense because excluding a revision
does not deepen a repository; it makes the repository more shallow.
Signed-off-by: Alex Henrie <alexhenri...@gmail.com>
---
builtin/clone.c | 2 +-
builtin/fetch.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 6c76a6e..e3cb808 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -99,7 +99,7 @@ static struct option builtin_clone_options[] = {
OPT_STRING(0, "shallow-since", _since, N_("time"),
N_("create a shallow clone since a specific time")),
OPT_STRING_LIST(0, "shallow-exclude", _not, N_("revision"),
- N_("deepen history of shallow clone by excluding rev")),
+ N_("deepen history of shallow clone, excluding rev")),
OPT_BOOL(0, "single-branch", _single_branch,
N_("clone only one branch, HEAD or --branch")),
OPT_BOOL(0, "shallow-submodules", _shallow_submodules,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b6a5597..fc74c84 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -122,7 +122,7 @@ static struct option builtin_fetch_options[] = {
OPT_STRING(0, "shallow-since", _since, N_("time"),
N_("deepen history of shallow repository based on time")),
OPT_STRING_LIST(0, "shallow-exclude", _not, N_("revision"),
- N_("deepen history of shallow clone by excluding rev")),
+ N_("deepen history of shallow clone, excluding rev")),
OPT_INTEGER(0, "deepen", _relative,
N_("deepen history of shallow clone")),
{ OPTION_SET_INT, 0, "unshallow", , NULL,
--
2.10.2
[PATCH] receive-pack: improve English grammar of denyCurrentBranch message
The article "the" is required here. Signed-off-by: Alex Henrie <alexhenri...@gmail.com> --- builtin/receive-pack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index e6b3879..6b97cbd 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -795,8 +795,8 @@ static char *refuse_unconfigured_deny_msg = "with what you pushed, and will require 'git reset --hard' to match\n" "the work tree to HEAD.\n" "\n" - "You can set 'receive.denyCurrentBranch' configuration variable to\n" - "'ignore' or 'warn' in the remote repository to allow pushing into\n" + "You can set the 'receive.denyCurrentBranch' configuration variable\n" + "to 'ignore' or 'warn' in the remote repository to allow pushing into\n" "its current branch; however, this is not recommended unless you\n" "arranged to update its work tree to match what you pushed in some\n" "other way.\n" -- 2.10.2
[PATCH] bisect: improve English grammar of not-ancestors message
Multiple revisions cannot be a single ancestor.
Signed-off-by: Alex Henrie <alexhenri...@gmail.com>
---
bisect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bisect.c b/bisect.c
index 21bc6da..8e63c40 100644
--- a/bisect.c
+++ b/bisect.c
@@ -747,7 +747,7 @@ static void handle_bad_merge_base(void)
exit(3);
}
- fprintf(stderr, _("Some %s revs are not ancestor of the %s rev.\n"
+ fprintf(stderr, _("Some %s revs are not ancestors of the %s rev.\n"
"git bisect cannot work properly in this case.\n"
"Maybe you mistook %s and %s revs?\n"),
term_good, term_bad, term_good, term_bad);
--
2.10.2
[BUG] git-stash fails when tracked file is replaced with directory
When a file is deleted from Git and replaced with a directory+file(s),
git-stash has two unexpected behaviors. This is tested against
versions 2.8.1 and 2.10.1:
1) Old file is removed from index and newly-added directory+file(s)
are added to index.
In this scenario, 'git stash' fails with the message:
error: : is a directory - add individual files instead
fatal: Unable to process path
Cannot save the current work tree state
The expected result would be that Git properly records the state
currently stored in index.
2) Old file is removed and new-added directory+file(s) are present
only in worktree, but not added to the index.
In this scenario, 'git stash -u' works fine, but 'git stash apply'
fails with the message:
fatal: cannot create directory at '': File exists
Could not restore untracked files from stash
The expected result would be that Git removes 'file' and replaces with
the untracked contents recorded in the original stash.
It is worth noting that Git does properly handle the scenario where
the stash operation of (1) is replaced with a commit to a temporary
branch, so this quirk seems to be restricted to stashes only.
I found a similar issue reported 22 April 2016 titled "possible bug of
git stash deleting uncommitted files in corner case". The thread-view
of the message is here:
http://marc.info/?t=14613256862=1=2
Here's a quick-and-dirty bash script to re-create all three scenarios
(1, 2, and 'commit to branch'):
= BEGIN stash-test.sh
#!/bin/bash
set -x
export GIT_TRACE=1
setup() {
repo=$1
# Prepare repo
rm -rf $repo
git init $repo
cd $repo
# Add initial file (symlink to external assets)
ln -fs /external/dir dir
git add dir
git commit -m "Add symlink to /external/dir"
# Replace symlink with local copy of assets
rm dir
mkdir -p dir
touch dir/local_copy_of_dir_files
}
{
( setup stash-test-unstaged
git stash -u
git stash apply )
( setup stash-test-staged
git add .
git stash )
( setup stash-test-commit
git add .
git co -b stash-branch
git commit -m "commit to branch instead of stash" )
} 2>&1 | tee stash-test.log
===== END stash-test.sh
Thanks,
-Alex Reed
