* Re: [PATCH 3/3] Convert all fnmatch() calls to wildmatch()
From: Nguyen Thai Ngoc Duy @ 2012-12-20 12:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8v8tanyn.fsf@alter.siamese.dyndns.org>
On Thu, Dec 20, 2012 at 1:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -627,7 +628,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
>> return entry_interesting;
>>
>> if (item->use_wildcard) {
>> - if (!fnmatch(match + baselen, entry->path, 0))
>> + if (!wildmatch(match + baselen, entry->path, 0))
>> return entry_interesting;
>
> This and the change to dir.c obviously have interactions with
> 8c6abbc (pathspec: apply "*.c" optimization from exclude,
> 2012-11-24).
>
> I've already alluded to it in my response to 2/3, I guess.
Conflict of plans. I did not expect myself to work on replacing
fnmatch soon and git_fnmatch is an intermediate step to collect more
optimizations and gradually replace fnmatch. Eventually git_fnmatch()
would become a wrapper of wildmatch, all the optimizations are pushed
down there. If we replace fnmatch->wildmatch first then there's little
reason for the existence of git_fnmatch(). Maybe I should merge this
with the fnmatch elimination into a single series.
--
Duy
^ permalink raw reply
* Re: [PATCH 2/3] wildmatch: support "no FNM_PATHNAME" mode
From: Nguyen Thai Ngoc Duy @ 2012-12-20 13:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <CACsJy8C9Nhdq_xBAOxtmLcUnrUioAMvWCPk=sBZKOzFY2WR+iA@mail.gmail.com>
On Thu, Dec 20, 2012 at 8:55 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>> As long as simpler patterns fnmatch() groks (namely, '?', '*', and
>> '[class]' wildcards only) are not slowed down by replacing it with
>> wildmatch(), that is, of course.
>
> I'm concerned about performance vs fnmatch too. I'll probably write a
> small program to exercise both and measure it with glibc.
I'll send out proper patches later this weekend, but unless I make big
mistakes it seems wildmatch is faster than fnmatch from glibc 2.14.1
on x86-64 :) Maybe I did make mistakes. The test matches every line in
/tmp/filelist.txt (which is ls-files from linux-2.6.git) 2000 times.
"pathname" means FNM_PATHNAME.
$ export LANG=C
$ ./test-wildmatch-perf /tmp/filelist.txt 'Documentation/*' 2000
wildmatch 1s 528394us
fnmatch 2s 588396us
$ ./test-wildmatch-perf /tmp/filelist.txt 'drivers/*' 2000
wildmatch 1s 957243us
fnmatch 3s 134839us
$ ./test-wildmatch-perf /tmp/filelist.txt 'Documentation/*' 2000 pathname
wildmatch 1s 548575us
fnmatch 2s 629680us
$ ./test-wildmatch-perf /tmp/filelist.txt 'drivers/*' 2000 pathname
wildmatch 2s 142377us
fnmatch 3s 318295us
$ ./test-wildmatch-perf /tmp/filelist.txt '[Dd]ocu[Mn]entation/*' 2000
wildmatch 1s 748505us
fnmatch 3s 224414us
$ ./test-wildmatch-perf /tmp/filelist.txt '[Dd]o?u[Mn]en?ati?n/*' 2000
wildmatch 1s 743268us
fnmatch 3s 278034us
$ ./test-wildmatch-perf /tmp/filelist.txt '[Dd]o?u[Mn]en?ati?n/*' 2000 pathname
wildmatch 1s 745151us
fnmatch 3s 314127us
$ ./test-wildmatch-perf /tmp/filelist.txt nonsense 2000 pathname
wildmatch 1s 310727us
fnmatch 2s 279123us
--
Duy
^ permalink raw reply
* Gitweb running as FCGI does not print its output in UTF-8
From: Sergio Talens-Oliag @ 2012-12-20 13:53 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2931 bytes --]
Hello, I'm sending this message to explain a problem I've found with
gitweb.cgi when running it using a call like the following:
export FCGI_SOCKET_PATH="/run/gitweb.socket"
gitweb.cgi --fcgi --nproc 2
I've fixed the problem for my installation usign a wrapper script, as
explained below.
I'm not sending a patch because I'm no expert in Perl and I'm unsure that
the solution I've found is the right one, but in the hope that my description
of the problem and the provided solution will be useful for others.
Problem description:
When using gitweb.cgi in FCGI mode the answers from the script are returned
using the wrong encoding (UTF-8 characters are printed as LATIN-1).
It seems that the problem appears because the FCGI streams are implemented
using the older stream API, TIEHANDLE and applying PerlIO layers using
binmode() has no effect to them.
Applied solution:
A solution similar to the use of binmode for the output stream is to
redefine the FCGI::Stream::PRINT function to use UTF-8 as output encoding.
To do it with minimal chages I've created a gitweb.cgi wrapper that
redefines the function and runs the original script; I'm doing it like this
to be able to use the packaged script until upstream includes a fix for the
problem.
Wrapper code (uses /usr/share/gitweb/gitweb.cgi as the PATH for gitweb.cgi):
#!/usr/bin/perl
# gitweb.cgi wrapper that fixes the UTF-8 problem with fastcgi
# Local redefinition of FCGI::Stream::PRINT
use Encode;
use FCGI;
our $enc = Encode::find_encoding('UTF-8');
our $org = \&FCGI::Stream::PRINT;
no warnings 'redefine';
local *FCGI::Stream::PRINT = sub {
my @OUTPUT = @_;
for (my $i = 1; $i < @_; $i++) {
$OUTPUT[$i] = $enc->encode($_[$i], Encode::FB_CROAK|Encode::LEAVE_SRC);
}
@_ = @OUTPUT;
goto $org;
};
# Execute original script
do "/usr/share/gitweb/gitweb.cgi";
References:
The applied solution has been found on the following StackOverflow
question:
http://stackoverflow.com/questions/5005104
Environment:
The system I'm using is Debian Wheezy and I'm serving gitweb using nginx's
fastcgi interface.
The versions of the related debian packages are:
Package: git
Version: 1:1.7.10.4-1+wheezy1
Package: libcgi-fast-perl
Version: 5.14.2-16
Package: libfcgi-procmanager-perl
Version: 0.24-1
Package: perl
Version: 5.14.2-16
To launch gitweb as fastcgi I'm using an init.d script that runs the wrapper
script in the background as the `www-user` using a call similar to the
following:
export FCGI_SOCKET_PATH="/run/gitweb/socket"
gitweb.cgi-wrapper --fcgi --nproc 2
Greetings,
Sergio.
--
Sergio Talens-Oliag <sto@iti.es> <http://www.iti.es/>
Key fingerprint = FF77 A16B 9D09 FC7B 6656 CFAD 261D E19A 578A 36F2
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCH] Python scripts audited for minimum compatible version and checks added.
From: Eric S. Raymond @ 2012-12-20 14:13 UTC (permalink / raw)
To: git
Signed-off-by: Eric S. Raymond <esr@thyrsus.com>
---
contrib/ciabot/ciabot.py | 5 +++++
contrib/fast-import/import-zips.py | 5 +++++
contrib/hg-to-git/hg-to-git.py | 5 +++++
contrib/p4import/git-p4import.py | 5 +++++
contrib/svn-fe/svnrdump_sim.py | 4 ++++
git-p4.py | 5 +++++
git-remote-testgit.py | 5 +++++
git_remote_helpers/git/__init__.py | 4 ++++
8 files changed, 38 insertions(+)
diff --git a/contrib/ciabot/ciabot.py b/contrib/ciabot/ciabot.py
index bd24395..b55648f 100755
--- a/contrib/ciabot/ciabot.py
+++ b/contrib/ciabot/ciabot.py
@@ -50,6 +50,11 @@
import os, sys, commands, socket, urllib
from xml.sax.saxutils import escape
+if sys.hexversion < 0x02000000:
+ # The limiter is the xml.sax module
+ sys.stderr.write("import-zips.py: requires Python 2.0.0 or later.")
+ sys.exit(1)
+
# Changeset URL prefix for your repo: when the commit ID is appended
# to this, it should point at a CGI that will display the commit
# through gitweb or something similar. The defaults will probably
diff --git a/contrib/fast-import/import-zips.py b/contrib/fast-import/import-zips.py
index 82f5ed3..d9ad71d 100755
--- a/contrib/fast-import/import-zips.py
+++ b/contrib/fast-import/import-zips.py
@@ -13,6 +13,11 @@ from sys import argv, exit
from time import mktime
from zipfile import ZipFile
+if sys.hexversion < 0x01060000:
+ # The limiter is the zipfile module
+ sys.stderr.write("import-zips.py: requires Python 1.6.0 or later.")
+ sys.exit(1)
+
if len(argv) < 2:
print 'Usage:', argv[0], '<zipfile>...'
exit(1)
diff --git a/contrib/hg-to-git/hg-to-git.py b/contrib/hg-to-git/hg-to-git.py
index 046cb2b..9f39ce5 100755
--- a/contrib/hg-to-git/hg-to-git.py
+++ b/contrib/hg-to-git/hg-to-git.py
@@ -23,6 +23,11 @@ import os, os.path, sys
import tempfile, pickle, getopt
import re
+if sys.hexversion < 0x02030000:
+ # The behavior of the pickle module changed significantly in 2.3
+ sys.stderr.write("hg-to-git.py: requires Python 2.3 or later.")
+ sys.exit(1)
+
# Maps hg version -> git version
hgvers = {}
# List of children for each hg revision
diff --git a/contrib/p4import/git-p4import.py b/contrib/p4import/git-p4import.py
index b6e534b..fb48e2a 100644
--- a/contrib/p4import/git-p4import.py
+++ b/contrib/p4import/git-p4import.py
@@ -14,6 +14,11 @@ import sys
import time
import getopt
+if sys.hexversion < 0x02020000:
+ # The behavior of the marshal module changed significantly in 2.2
+ sys.stderr.write("git-p4import.py: requires Python 2.2 or later.")
+ sys.exit(1)
+
from signal import signal, \
SIGPIPE, SIGINT, SIG_DFL, \
default_int_handler
diff --git a/contrib/svn-fe/svnrdump_sim.py b/contrib/svn-fe/svnrdump_sim.py
index 1cfac4a..ed43dbb 100755
--- a/contrib/svn-fe/svnrdump_sim.py
+++ b/contrib/svn-fe/svnrdump_sim.py
@@ -7,6 +7,10 @@ to the highest revision that should be available.
"""
import sys, os
+if sys.hexversion < 0x02040000:
+ # The limiter is the ValueError() calls. This may be too conservative
+ sys.stderr.write("svnrdump-sim.py: requires Python 2.4 or later.")
+ sys.exit(1)
def getrevlimit():
var = 'SVNRMAX'
diff --git a/git-p4.py b/git-p4.py
index 551aec9..ec060b4 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -12,6 +12,11 @@ import optparse, sys, os, marshal, subprocess, shelve
import tempfile, getopt, os.path, time, platform
import re, shutil
+if sys.hexversion < 0x02040000:
+ # The limiter is the subprocess module
+ sys.stderr.write("git-p4.py: requires Python 2.4 or later.")
+ sys.exit(1)
+
verbose = False
# Only labels/tags matching this will be imported/exported
diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 5f3ebd2..22d2eb6 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -31,6 +31,11 @@ from git_remote_helpers.git.exporter import GitExporter
from git_remote_helpers.git.importer import GitImporter
from git_remote_helpers.git.non_local import NonLocalGit
+if sys.hexversion < 0x01050200:
+ # os.makedirs() is the limiter
+ sys.stderr.write("git-remote-testgit.py: requires Python 1.5.2 or later.")
+ sys.exit(1)
+
def get_repo(alias, url):
"""Returns a git repository object initialized for usage.
"""
diff --git a/git_remote_helpers/git/__init__.py b/git_remote_helpers/git/__init__.py
index e69de29..776e891 100644
--- a/git_remote_helpers/git/__init__.py
+++ b/git_remote_helpers/git/__init__.py
@@ -0,0 +1,4 @@
+if sys.hexversion < 0x02040000:
+ # The limiter is the subprocess module
+ sys.stderr.write("git_remote_helpers: requires Python 2.4 or later.")
+ sys.exit(1)
--
1.8.1.rc2
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
"The state calls its own violence `law', but that of the individual `crime'"
-- Max Stirner
^ permalink raw reply related
* Python version auditing followup
From: Eric S. Raymond @ 2012-12-20 14:34 UTC (permalink / raw)
To: git
Most of the Python scripts in the distribution are small and simple to
audit, so I am pretty sure of the results. The only place where I
have a concern is the git_helpers library; that is somewhat more
complex and I might have missed a dependency somewhere. Whoever
owns that should check my finding that it should run under 2.4
That was the first of three patches I have promised. In order to do
the next one, which will be a development guidelines recommend
compatibility back to some specific version X, I need a policy
decision. How do we set X?
I don't think X can be < 2.4, nor does it need to be - 2.4 came out
in 2004 and eight years is plenty of deployment time.
The later we set it, the more convenient for developers. But of
course by setting it late we trade away some portability to
older systems.
In previous discussion of this issue I recommended X = 2.6.
That is still my recommendation. Thoughts, comments, objections?
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
In recent years it has been suggested that the Second Amendment
protects the "collective" right of states to maintain militias, while
it does not protect the right of "the people" to keep and bear arms.
If anyone entertained this notion in the period during which the
Constitution and the Bill of Rights were debated and ratified, it
remains one of the most closely guarded secrets of the eighteenth
century, for no known writing surviving from the period between 1787
and 1791 states such a thesis.
-- Stephen P. Halbrook, "That Every Man Be Armed", 1984
^ permalink raw reply
* Re: [PATCH] Python scripts audited for minimum compatible version and checks added.
From: Jeff King @ 2012-12-20 14:48 UTC (permalink / raw)
To: Eric S. Raymond; +Cc: git
In-Reply-To: <20121220141855.05DAA44105@snark.thyrsus.com>
On Thu, Dec 20, 2012 at 09:13:37AM -0500, Eric S. Raymond wrote:
> diff --git a/contrib/ciabot/ciabot.py b/contrib/ciabot/ciabot.py
> index bd24395..b55648f 100755
> --- a/contrib/ciabot/ciabot.py
> +++ b/contrib/ciabot/ciabot.py
> @@ -50,6 +50,11 @@
> import os, sys, commands, socket, urllib
> from xml.sax.saxutils import escape
>
> +if sys.hexversion < 0x02000000:
> + # The limiter is the xml.sax module
> + sys.stderr.write("import-zips.py: requires Python 2.0.0 or later.")
> + sys.exit(1)
Should the error message say ciabot.py?
-Peff
^ permalink raw reply
* Re: $PATH pollution and t9902-completion.sh
From: Jeff King @ 2012-12-20 14:55 UTC (permalink / raw)
To: Adam Spiers; +Cc: git mailing list
In-Reply-To: <20121217010538.GC3673@gmail.com>
On Mon, Dec 17, 2012 at 01:05:38AM +0000, Adam Spiers wrote:
> t/t9902-completion.sh is currently failing for me because I happen to
> have a custom shell-script called git-check-email in ~/bin, which is
> on my $PATH. This is different to a similar-looking case reported
> recently, which was due to an unclean working tree:
>
> http://thread.gmane.org/gmane.comp.version-control.git/208085
>
> It's not unthinkable that in the future other tests could break for
> similar reasons. Therefore it would be good to sanitize $PATH in the
> test framework so that it cannot destabilize tests, although I am
> struggling to think of a good way of doing this. Naively stripping
> directories under $HOME would not protect against git "plugins" such
> as the above being installed into places like /usr/bin. Thoughts?
I've run into this, too. I think sanitizing $PATH is the wrong approach.
The real problem is that the test is overly picky. Right now it is
failing because you happen to have "check-email" in your $PATH, but it
will also need to be adjusted when a true "check-email" command is added
to git.
I can think of two other options:
1. Make the test input more specific (e.g., looking for "checkou").
This doesn't eliminate the problem, but makes it less likely
to occur.
2. Loosen the test to look for the presence of "checkout", but not
fail when other items are present. Bonus points if it makes sure
that everything returned starts with "check".
I think (2) is the ideal solution in terms of behavior, but writing it
may be more of a pain.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #05; Tue, 18)
From: Jeff King @ 2012-12-20 14:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4njjf6fk.fsf@alter.siamese.dyndns.org>
On Tue, Dec 18, 2012 at 12:31:43PM -0800, Junio C Hamano wrote:
> * jk/error-const-return (2012-12-15) 2 commits
> - silence some -Wuninitialized false positives
> - make error()'s constant return value more visible
>
> Help compilers' flow analysis by making it more explicit that
> error() always returns -1, to reduce false "variable used
> uninitialized" warnings.
>
> This is still an RFC.
What's your opinion on this? The last round I posted (and what you have
in pu) is about as good as this technique is going to get. So it is
really a taste thing. On the one hand, I really like that it is not just
papering over the problem, but rather giving the compiler more data to
do its analysis. On the other hand, it makes the source kind of ugly.
I don't think there's a rush on it, but I'm just sifting through my
half-done topics.
-Peff
^ permalink raw reply
* Re: [PATCH] Python scripts audited for minimum compatible version and checks added.
From: Eric S. Raymond @ 2012-12-20 15:02 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20121220144813.GA27211@sigill.intra.peff.net>
Jeff King <peff@peff.net>:
> On Thu, Dec 20, 2012 at 09:13:37AM -0500, Eric S. Raymond wrote:
>
> > diff --git a/contrib/ciabot/ciabot.py b/contrib/ciabot/ciabot.py
> > index bd24395..b55648f 100755
> > --- a/contrib/ciabot/ciabot.py
> > +++ b/contrib/ciabot/ciabot.py
> > @@ -50,6 +50,11 @@
> > import os, sys, commands, socket, urllib
> > from xml.sax.saxutils import escape
> >
> > +if sys.hexversion < 0x02000000:
> > + # The limiter is the xml.sax module
> > + sys.stderr.write("import-zips.py: requires Python 2.0.0 or later.")
> > + sys.exit(1)
>
> Should the error message say ciabot.py?
>
> -Peff
Gack. Yes. Thaty's what I get for cut-and-pasting too quickly.
The information about xnml.sex is correct, though.
Want me to resubmit, or will you just patch it?
Note by the way that I still think the entire ciabot subtree (which is
my code) should just be nuked. CIA is not coming back, wishful thinking
on Ilkotech's web page notwithstanding.
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
^ permalink raw reply
* Re: RFC: "git config -l" should not expose sensitive information
From: Jeff King @ 2012-12-20 15:04 UTC (permalink / raw)
To: Toralf Förster; +Cc: git
In-Reply-To: <50CF039A.7010800@gmx.de>
On Mon, Dec 17, 2012 at 12:35:54PM +0100, Toralf Förster wrote:
> often the output is requested in help forums - and a
> "git config -l | wgetpaste" exposes parameters like sendmail.smtppass -
> so hide those variables in the output (if not explicitly wanted) would
> makes sense, or ?
But if we change "git config -l", won't that break all of the
non-exposing uses that rely on seeing all of the variables (e.g., it is
perfectly fine for a porcelain to parse "git config -l" rather than
making several calls to "git config"; IIRC, git-cola does this).
The problem seems to be that people are giving bad advice to tell people
to post "git config -l" output without looking at. Maybe we could help
them with a "git config --share-config" option that dumps all config,
but sanitizes the output. It would need to have a list of sensitive keys
(which does not exist yet), and would need to not just mark up things
like smtppass, but would also need to pull credential information out of
remote.*.url strings. And maybe more (I haven't thought too long on it).
-Peff
^ permalink raw reply
* Re: $PATH pollution and t9902-completion.sh
From: Adam Spiers @ 2012-12-20 15:13 UTC (permalink / raw)
To: Jeff King; +Cc: git mailing list
In-Reply-To: <20121220145519.GB27211@sigill.intra.peff.net>
On Thu, Dec 20, 2012 at 2:55 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 17, 2012 at 01:05:38AM +0000, Adam Spiers wrote:
>> t/t9902-completion.sh is currently failing for me because I happen to
>> have a custom shell-script called git-check-email in ~/bin, which is
>> on my $PATH. This is different to a similar-looking case reported
>> recently, which was due to an unclean working tree:
>>
>> http://thread.gmane.org/gmane.comp.version-control.git/208085
>>
>> It's not unthinkable that in the future other tests could break for
>> similar reasons. Therefore it would be good to sanitize $PATH in the
>> test framework so that it cannot destabilize tests, although I am
>> struggling to think of a good way of doing this. Naively stripping
>> directories under $HOME would not protect against git "plugins" such
>> as the above being installed into places like /usr/bin. Thoughts?
>
> I've run into this, too. I think sanitizing $PATH is the wrong approach.
> The real problem is that the test is overly picky. Right now it is
> failing because you happen to have "check-email" in your $PATH, but it
> will also need to be adjusted when a true "check-email" command is added
> to git.
>
> I can think of two other options:
>
> 1. Make the test input more specific (e.g., looking for "checkou").
> This doesn't eliminate the problem, but makes it less likely
> to occur.
>
> 2. Loosen the test to look for the presence of "checkout", but not
> fail when other items are present. Bonus points if it makes sure
> that everything returned starts with "check".
>
> I think (2) is the ideal solution in terms of behavior, but writing it
> may be more of a pain.
I agree with all your points. Thanks for the suggestions.
^ permalink raw reply
* Re: [PATCH v6 0/7] make test output coloring more intuitive
From: Jeff King @ 2012-12-20 15:34 UTC (permalink / raw)
To: Adam Spiers; +Cc: Junio C Hamano, git list
In-Reply-To: <CAOkDyE9B_HfUZmqNqO35mtjTvdihBTiW=uOV2oEQgLUw1xyf=A@mail.gmail.com>
On Sun, Dec 16, 2012 at 07:01:56PM +0000, Adam Spiers wrote:
> On Sun, Dec 16, 2012 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Adam Spiers <git@adamspiers.org> writes:
> >
> >> This series of commits attempts to make test output coloring
> >> more intuitive,...
> >
> > Thanks; I understand that this is to replace the previous one
> > b465316 (tests: paint unexpectedly fixed known breakages in bold
> > red, 2012-09-19)---am I correct?
>
> Correct. AFAICS I have incorporated all feedback raised in previous
> reviews.
>
> > Will take a look; thanks.
>
> Thanks. Sorry again for the delay. I'm now (finally) resuming work
> on as/check-ignore.
I eyeballed the test output of "pu". I do think this resolves all of the
issues brought up before, and I really hate to bikeshed on the colors at
this point, but I find that bold cyan a bit hard on the eyes when
running with "-v" (where most of the output is in that color, as it
dumps the shell for each test). Is there any reason not to tone it down
a bit like:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 256f1c6..31f59af 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -227,7 +227,7 @@ then
pass)
tput setaf 2;; # green
info)
- tput bold; tput setaf 6;; # bold cyan
+ tput setaf 6;; # cyan
*)
test -n "$quiet" && return;;
esac
-Peff
^ permalink raw reply related
* Re: [PATCH v6 0/7] make test output coloring more intuitive
From: Adam Spiers @ 2012-12-20 15:44 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git list
In-Reply-To: <20121220153411.GA1497@sigill.intra.peff.net>
On Thu, Dec 20, 2012 at 3:34 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Dec 16, 2012 at 07:01:56PM +0000, Adam Spiers wrote:
>> On Sun, Dec 16, 2012 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Adam Spiers <git@adamspiers.org> writes:
>> >> This series of commits attempts to make test output coloring
>> >> more intuitive,...
>> >
>> > Thanks; I understand that this is to replace the previous one
>> > b465316 (tests: paint unexpectedly fixed known breakages in bold
>> > red, 2012-09-19)---am I correct?
>>
>> Correct. AFAICS I have incorporated all feedback raised in previous
>> reviews.
>>
>> > Will take a look; thanks.
>>
>> Thanks. Sorry again for the delay. I'm now (finally) resuming work
>> on as/check-ignore.
>
> I eyeballed the test output of "pu". I do think this resolves all of the
> issues brought up before, and I really hate to bikeshed on the colors at
> this point, but I find that bold cyan a bit hard on the eyes when
> running with "-v" (where most of the output is in that color, as it
> dumps the shell for each test). Is there any reason not to tone it down
> a bit like:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 256f1c6..31f59af 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -227,7 +227,7 @@ then
> pass)
> tput setaf 2;; # green
> info)
> - tput bold; tput setaf 6;; # bold cyan
> + tput setaf 6;; # cyan
> *)
> test -n "$quiet" && return;;
> esac
>
> -Peff
Good point, I forgot to check what it looked like with -v. Since this
series is already on v6, is there a more lightweight way of addressing
this tiny tweak than sending v7?
^ permalink raw reply
* Re: RFC: "git config -l" should not expose sensitive information
From: Aaron Schrab @ 2012-12-20 15:49 UTC (permalink / raw)
To: Jeff King; +Cc: Toralf Förster, git
In-Reply-To: <20121220150408.GD27211@sigill.intra.peff.net>
At 10:04 -0500 20 Dec 2012, Jeff King <peff@peff.net> wrote:
>The problem seems to be that people are giving bad advice to tell
>people to post "git config -l" output without looking at. Maybe we
>could help them with a "git config --share-config" option that dumps
>all config, but sanitizes the output. It would need to have a list of
>sensitive keys (which does not exist yet), and would need to not just
>mark up things like smtppass, but would also need to pull credential
>information out of remote.*.url strings. And maybe more (I haven't
>thought too long on it).
If such an option is added, it is likely to cause more people to think
that there is no need to examine the output before sharing it. But, I
don't think that the sanitizing could ever be sufficient to guarantee
that.
Tools outside of the core git tree may add support for new config keys
which are meant to contain sensitive information, and there would be no
way for `git config` to know about those.
Even for known sensitive keys, the person entering it might have made a
typo in the name (e.g. smptpass) preventing it from being recognized as
sensitive by the software, but easily recognizable as such by a human.
There's also the problem of varying opinions on what is considered as
sensitive. You mention credential information in URLs, but some people
may consider the entire URL as something which they would not want to
expose.
I think that attempting to do this would only result in a false sense of
security.
^ permalink raw reply
* Re: RFC: "git config -l" should not expose sensitive information
From: Michael Haggerty @ 2012-12-20 15:51 UTC (permalink / raw)
To: Jeff King; +Cc: Toralf Förster, git
In-Reply-To: <20121220150408.GD27211@sigill.intra.peff.net>
On 12/20/2012 04:04 PM, Jeff King wrote:
> On Mon, Dec 17, 2012 at 12:35:54PM +0100, Toralf Förster wrote:
>> often the output is requested in help forums - and a
>> "git config -l | wgetpaste" exposes parameters like sendmail.smtppass -
>> so hide those variables in the output (if not explicitly wanted) would
>> makes sense, or ?
>
> But if we change "git config -l", won't that break all of the
> non-exposing uses that rely on seeing all of the variables (e.g., it is
> perfectly fine for a porcelain to parse "git config -l" rather than
> making several calls to "git config"; IIRC, git-cola does this).
>
> The problem seems to be that people are giving bad advice to tell people
> to post "git config -l" output without looking at. Maybe we could help
> them with a "git config --share-config" option that dumps all config,
> but sanitizes the output. It would need to have a list of sensitive keys
> (which does not exist yet), and would need to not just mark up things
> like smtppass, but would also need to pull credential information out of
> remote.*.url strings. And maybe more (I haven't thought too long on it).
I think the problem is yet another step earlier: why do we build tools
that encourage people to store passwords in plaintext in a configuration
file that is by default world-readable?
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: RFC: "git config -l" should not expose sensitive information
From: Jeff King @ 2012-12-20 15:52 UTC (permalink / raw)
To: git; +Cc: Toralf Förster
In-Reply-To: <20121220154915.GA5162@pug.qqx.org>
On Thu, Dec 20, 2012 at 10:49:15AM -0500, Aaron Schrab wrote:
> At 10:04 -0500 20 Dec 2012, Jeff King <peff@peff.net> wrote:
> >The problem seems to be that people are giving bad advice to tell
> >people to post "git config -l" output without looking at. Maybe we
> >could help them with a "git config --share-config" option that
> >dumps all config, but sanitizes the output. It would need to have a
> >list of sensitive keys (which does not exist yet), and would need
> >to not just mark up things like smtppass, but would also need to
> >pull credential information out of remote.*.url strings. And maybe
> >more (I haven't thought too long on it).
>
> If such an option is added, it is likely to cause more people to
> think that there is no need to examine the output before sharing it.
> But, I don't think that the sanitizing could ever be sufficient to
> guarantee that.
>
> Tools outside of the core git tree may add support for new config
> keys which are meant to contain sensitive information, and there
> would be no way for `git config` to know about those.
Good point. I was a little on the fence already because any time you
have a "prevent known bad" list in a security setting, it is guaranteed
to go out of date and screw you. But the presence of third-party tools
means it does not even have to get out of date. It will always be
complete.
> I think that attempting to do this would only result in a false sense
> of security.
Yeah. Thanks for a dose of sanity. I was really trying not to say "the
given advice is bad, and we cannot help those people". But I think you
are right; the only sensible path is for the user to inspect the output
before posting it.
-Peff
^ permalink raw reply
* Re: RFC: "git config -l" should not expose sensitive information
From: Jeff King @ 2012-12-20 15:54 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Toralf Förster, git
In-Reply-To: <50D33409.1050309@alum.mit.edu>
On Thu, Dec 20, 2012 at 04:51:37PM +0100, Michael Haggerty wrote:
> > The problem seems to be that people are giving bad advice to tell people
> > to post "git config -l" output without looking at. Maybe we could help
> > them with a "git config --share-config" option that dumps all config,
> > but sanitizes the output. It would need to have a list of sensitive keys
> > (which does not exist yet), and would need to not just mark up things
> > like smtppass, but would also need to pull credential information out of
> > remote.*.url strings. And maybe more (I haven't thought too long on it).
>
> I think the problem is yet another step earlier: why do we build tools
> that encourage people to store passwords in plaintext in a configuration
> file that is by default world-readable?
Agreed. Most of it is hysterical raisins. We did not have any portable
secure storage for a long time. These days we have the credential helper
subsystem, which send-email can and should be using.
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/7] make test output coloring more intuitive
From: Jeff King @ 2012-12-20 16:11 UTC (permalink / raw)
To: Adam Spiers; +Cc: Junio C Hamano, git list
In-Reply-To: <CAOkDyE9y6JvNKTCBoJqu47Hn-3axfjZPUdBhf4bOEfSP-9Q84A@mail.gmail.com>
On Thu, Dec 20, 2012 at 03:44:53PM +0000, Adam Spiers wrote:
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 256f1c6..31f59af 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -227,7 +227,7 @@ then
> > pass)
> > tput setaf 2;; # green
> > info)
> > - tput bold; tput setaf 6;; # bold cyan
> > + tput setaf 6;; # cyan
> > *)
> > test -n "$quiet" && return;;
> > esac
> >
>
> Good point, I forgot to check what it looked like with -v. Since this
> series is already on v6, is there a more lightweight way of addressing
> this tiny tweak than sending v7?
It is ultimately up to Junio, but I suspect he would be OK if you just
reposted patch 4/7 with the above squashed. Or even just said "I like
this, please squash it into patch 4 (change info messages from
yellow/brown to bold cyan).
As an aside, it made me wonder how hard/useful it would be to color the
snippets even more. Doing this:
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index f9ccbf2..3d44a94 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -364,7 +364,12 @@ test_expect_success () {
export test_prereq
if ! test_skip "$@"
then
- say >&3 "expecting success: $2"
+ if test -z "$GIT_TEST_HIGHLIGHT"; then
+ say >&3 "expecting success: $2"
+ else
+ say >&3 "expecting success:"
+ echo "$2" | eval "$GIT_TEST_HIGHLIGHT"
+ fi
if test_run_ "$2"
then
test_ok_ "$1"
produces highlighted snippets with:
GIT_TEST_HIGHLIGHT='highlight -S sh -O ansi'
or
GIT_TEST_HIGHLIGHT='pygmentize -l sh'
depending on what you have installed on your system. I'm not convinced
it actually adds anything, but it's a fun toy. A real patch would
probably turn it off in non-verbose mode, as invoking the highlighter
(especially pygmentize) repeatedly is somewhat expensive.
-Peff
^ permalink raw reply related
* Re: RFC: "git config -l" should not expose sensitive information
From: Toralf Förster @ 2012-12-20 16:20 UTC (permalink / raw)
To: Jeff King, git
In-Reply-To: <20121220154915.GA5162@pug.qqx.org>
yep - understood
On 12/20/2012 04:49 PM, Aaron Schrab wrote:
> At 10:04 -0500 20 Dec 2012, Jeff King <peff@peff.net> wrote:
>> The problem seems to be that people are giving bad advice to tell
>> people to post "git config -l" output without looking at. Maybe we
>> could help them with a "git config --share-config" option that dumps
>> all config, but sanitizes the output. It would need to have a list of
>> sensitive keys (which does not exist yet), and would need to not just
>> mark up things like smtppass, but would also need to pull credential
>> information out of remote.*.url strings. And maybe more (I haven't
>> thought too long on it).
>
> If such an option is added, it is likely to cause more people to think
> that there is no need to examine the output before sharing it. But, I
> don't think that the sanitizing could ever be sufficient to guarantee that.
>
> Tools outside of the core git tree may add support for new config keys
> which are meant to contain sensitive information, and there would be no
> way for `git config` to know about those.
>
> Even for known sensitive keys, the person entering it might have made a
> typo in the name (e.g. smptpass) preventing it from being recognized as
> sensitive by the software, but easily recognizable as such by a human.
>
> There's also the problem of varying opinions on what is considered as
> sensitive. You mention credential information in URLs, but some people
> may consider the entire URL as something which they would not want to
> expose.
>
> I think that attempting to do this would only result in a false sense of
> security.
>
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
^ permalink raw reply
* Re: [RFC] test: Old shells and physical paths
From: David Michael @ 2012-12-20 16:25 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git@vger.kernel.org
In-Reply-To: <CAJDDKr78ugSo9hNerHO0Y46_bSzLJWznB3E3+6H98NjMtBwHsw@mail.gmail.com>
Hi,
On Thu, Dec 20, 2012 at 12:01 AM, David Aguilar <davvid@gmail.com> wrote:
> Do you know if the differences are relegated to "cd",
> or do other common commands such as awk, grep, sed, mktemp, expr,
> etc. have similar issues?
There are almost certainly going to be incompatibilities with other
commands. The system implemented UNIX95 plus some extensions, then
they began supporting UNIX03/SUSv3/POSIX.1-2001/whatever for certain
commands by using an environment variable to toggle between the
incompatible behaviors.
Their documentation on the UNIX03 commands indicates it is still only
partially supported. For example: "cp" supports "-L" and "-P", but
"cd" doesn't.
> I wonder if it'd be helpful to have a low-numbered test that checks
> the basics needed by the scripted Porcelains and test suite.
> It would give us an easy way to answer these questions, and could
> be a good way to document (in code) the capabilities we expect.
I'd be in favor of something like this as well.
Thanks.
David
P.S.
In the meantime, I am handling the "cd" situation by replacing "-P"
with "$PHYS" and prepending the following to t/test-lib.sh.
set +o logical >/dev/null 2>&1 || PHYS=-P
^ permalink raw reply
* Re: $PATH pollution and t9902-completion.sh
From: Torsten Bögershausen @ 2012-12-20 17:25 UTC (permalink / raw)
To: Adam Spiers; +Cc: Jeff King, git mailing list
In-Reply-To: <CAOkDyE-J7sTJ-GefhteP1wy7WorqTRnj5nn0k6hd1dp0VJz5iQ@mail.gmail.com>
On 20.12.12 16:13, Adam Spiers wrote:
> On Thu, Dec 20, 2012 at 2:55 PM, Jeff King <peff@peff.net> wrote:
>> On Mon, Dec 17, 2012 at 01:05:38AM +0000, Adam Spiers wrote:
>>> t/t9902-completion.sh is currently failing for me because I happen to
>>> have a custom shell-script called git-check-email in ~/bin, which is
>>> on my $PATH. This is different to a similar-looking case reported
>>> recently, which was due to an unclean working tree:
>>>
>>> http://thread.gmane.org/gmane.comp.version-control.git/208085
>>>
>>> It's not unthinkable that in the future other tests could break for
>>> similar reasons. Therefore it would be good to sanitize $PATH in the
>>> test framework so that it cannot destabilize tests, although I am
>>> struggling to think of a good way of doing this. Naively stripping
>>> directories under $HOME would not protect against git "plugins" such
>>> as the above being installed into places like /usr/bin. Thoughts?
>>
>> I've run into this, too. I think sanitizing $PATH is the wrong approach.
>> The real problem is that the test is overly picky. Right now it is
>> failing because you happen to have "check-email" in your $PATH, but it
>> will also need to be adjusted when a true "check-email" command is added
>> to git.
>>
>> I can think of two other options:
>>
>> 1. Make the test input more specific (e.g., looking for "checkou").
>> This doesn't eliminate the problem, but makes it less likely
>> to occur.
>>
>> 2. Loosen the test to look for the presence of "checkout", but not
>> fail when other items are present. Bonus points if it makes sure
>> that everything returned starts with "check".
>>
>> I think (2) is the ideal solution in terms of behavior, but writing it
>> may be more of a pain.
>
> I agree with all your points. Thanks for the suggestions.
I volonteer for 1) (and we got for 2) later)
^ permalink raw reply
* [PATCH] t9902: Be more specific when completing git-checkout
From: Torsten Bögershausen @ 2012-12-20 17:31 UTC (permalink / raw)
To: git; +Cc: tboegi
t9902 is trying to complete e.g.
"git --version check" into "git --version checkout"
If there are other binaries like
"git-check-email" or "git-check-ignore" in the PATH
one test case failes
By using "checkou" instead of "check" the test becomes
more future proof.
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
t/t9902-completion.sh | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..0e0e2d1 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -192,19 +192,19 @@ test_expect_success 'general options' '
'
test_expect_success 'general options plus command' '
- test_completion "git --version check" "checkout " &&
- test_completion "git --paginate check" "checkout " &&
- test_completion "git --git-dir=foo check" "checkout " &&
- test_completion "git --bare check" "checkout " &&
+ test_completion "git --version checkou" "checkout " &&
+ test_completion "git --paginate checkou" "checkout " &&
+ test_completion "git --git-dir=foo checkou" "checkout " &&
+ test_completion "git --bare checkou" "checkout " &&
test_completion "git --help des" "describe " &&
- test_completion "git --exec-path=foo check" "checkout " &&
- test_completion "git --html-path check" "checkout " &&
- test_completion "git --no-pager check" "checkout " &&
- test_completion "git --work-tree=foo check" "checkout " &&
- test_completion "git --namespace=foo check" "checkout " &&
- test_completion "git --paginate check" "checkout " &&
- test_completion "git --info-path check" "checkout " &&
- test_completion "git --no-replace-objects check" "checkout "
+ test_completion "git --exec-path=foo checkou" "checkout " &&
+ test_completion "git --html-path checkou" "checkout " &&
+ test_completion "git --no-pager checkou" "checkout " &&
+ test_completion "git --work-tree=foo checkou" "checkout " &&
+ test_completion "git --namespace=foo checkou" "checkout " &&
+ test_completion "git --paginate checkou" "checkout " &&
+ test_completion "git --info-path checkou" "checkout " &&
+ test_completion "git --no-replace-objects checkou" "checkout "
'
test_expect_success 'setup for ref completion' '
--
1.8.0
^ permalink raw reply related
* [PATCH] oneway_merge(): only lstat() when told to update worktree
From: Martin von Zweigbergk @ 2012-12-20 17:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
Although the subject line of 613f027 (read-tree -u one-way merge fix
to check out locally modified paths., 2006-05-15) mentions "read-tree
-u", it did not seem to check whether -u was in effect. Not checking
whether -u is in effect makes e.g. "read-tree --reset" lstat() the
worktree, even though the worktree stat should not matter for that
operation.
This speeds up e.g. "git reset" a little on the linux-2.6 repo (best
of five, warm cache):
Before After
real 0m0.288s 0m0.233s
user 0m0.190s 0m0.150s
sys 0m0.090s 0m0.080s
---
I am very unfamiliar with this part of git, so my attempt at a
motivation may be totally off.
I have another twenty-or-so patches to reset.c coming up that take the
timings down to around 90 ms, but this patch was quite unrelated to
that. Those other patches actually make this patch pointless for "git
reset" (it takes another path), but I hope this is still a good change
for other operations that use oneway_merge.
unpack-trees.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 6d96366..61acc5e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1834,7 +1834,7 @@ int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)
if (old && same(old, a)) {
int update = 0;
- if (o->reset && !ce_uptodate(old) && !ce_skip_worktree(old)) {
+ if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
struct stat st;
if (lstat(old->name, &st) ||
ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
--
1.8.0.1.240.ge8a1f5a
^ permalink raw reply related
* Re: What's cooking in git.git (Dec 2012, #05; Tue, 18)
From: Junio C Hamano @ 2012-12-20 18:13 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20121220145941.GC27211@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Dec 18, 2012 at 12:31:43PM -0800, Junio C Hamano wrote:
>
>> * jk/error-const-return (2012-12-15) 2 commits
>> - silence some -Wuninitialized false positives
>> - make error()'s constant return value more visible
>>
>> Help compilers' flow analysis by making it more explicit that
>> error() always returns -1, to reduce false "variable used
>> uninitialized" warnings.
>>
>> This is still an RFC.
>
> What's your opinion on this?
Ugly but not too much so, and it would be useful.
The only thing that makes it ugly is that it promises error() will
return -1 and nothing else forever, but at this point in the
evolution of the codebase, I think we are pretty much committed to
it anyway, so I do not think it is a problem.
^ permalink raw reply
* Re: [PATCH v6 0/7] make test output coloring more intuitive
From: Adam Spiers @ 2012-12-20 18:08 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git list
In-Reply-To: <20121220161110.GA10605@sigill.intra.peff.net>
On Thu, Dec 20, 2012 at 4:11 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Dec 20, 2012 at 03:44:53PM +0000, Adam Spiers wrote:
>> > diff --git a/t/test-lib.sh b/t/test-lib.sh
>> > index 256f1c6..31f59af 100644
>> > --- a/t/test-lib.sh
>> > +++ b/t/test-lib.sh
>> > @@ -227,7 +227,7 @@ then
>> > pass)
>> > tput setaf 2;; # green
>> > info)
>> > - tput bold; tput setaf 6;; # bold cyan
>> > + tput setaf 6;; # cyan
>> > *)
>> > test -n "$quiet" && return;;
>> > esac
>> >
>>
>> Good point, I forgot to check what it looked like with -v. Since this
>> series is already on v6, is there a more lightweight way of addressing
>> this tiny tweak than sending v7?
>
> It is ultimately up to Junio, but I suspect he would be OK if you just
> reposted patch 4/7 with the above squashed.
I'll do that if Junio is OK with that.
> Or even just said "I like
> this, please squash it into patch 4 (change info messages from
> yellow/brown to bold cyan).
Yes, I'm OK with this way too :) Of course "bold" would need to be dropped
from the commit message.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox