* [RFC 2/2] Make misuse of get_pathname() buffers detectable by valgrind
From: Michael Haggerty @ 2011-09-27 4:28 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Petr Baudis, Michael Haggerty
In-Reply-To: <1317097687-11098-1-git-send-email-mhagger@alum.mit.edu>
A temporary buffer produced by get_pathname() is recycled after a few
subsequent calls of get_pathname(). The use of such a buffer after it
has been recycled can result in the wrong file being accessed with
very strange effects. Moreover, such a bug can lie dormant until code
elsewhere is changed to use a temporary buffer, causing very
mysterious, nonlocal failures that are hard to analyze.
Add a second implementation of get_pathname() (activated if the
VALGRIND preprocessor macro is defined) that allocates and frees
buffers instead of recycling statically-allocated buffers. This does
not make the problem less serious, but it turns the errors into
access-after-free errors, making it possible to locate the guilty code
using valgrind.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
I believe that it is frowned upon to use #ifdefs in git code, but no
good alternative is obvious to me for this type of use. Suggestions
are welcome.
I would also welcome suggestions for a better name than "VALGRIND" for
the preprocessor macro. Are there standard names used elsewhere in
git for such purposes?
path.c | 40 ++++++++++++++++++++++++++++++++++++++--
1 files changed, 38 insertions(+), 2 deletions(-)
diff --git path.c path.c
index 6c4714d..3021207 100644
--- path.c
+++ path.c
@@ -9,6 +9,20 @@
* f = open(mkpath("%s/%s.git", base, name), O_RDONLY);
*
* which is what it's designed for.
+ *
+ * The temporary buffers returned by these functions will be clobbered
+ * by later calls to these functions. Therefore it is important not
+ * to expect such buffers to keep their values across calls to other
+ * git functions. Violations of this rule can cause the original
+ * buffer to be overwritten and lead to very confusing, nonlocal bugs,
+ * including data loss (you think you are writing to your file but are
+ * actually writing to a filename created by some other caller).
+ *
+ * If the VALGRIND preprocessor macro is defined, then buffers are
+ * created via xmalloc and old temporary buffers are recycled using
+ * free(). This changes the symptom of abuse of the buffers from
+ * mysterious, random errors into access-after-free errors that are
+ * detectable by valgrind.
*/
#include "cache.h"
#include "strbuf.h"
@@ -17,12 +31,34 @@
#define PATHNAME_BUFFER_COUNT (1 << 2)
static char bad_path[] = "/bad-path/";
+#ifdef VALGRIND
+static char buggy_path[] = "/git-internal-error/";
+#endif
static char *get_pathname(void)
{
- static char pathname_array[PATHNAME_BUFFER_COUNT][PATH_MAX];
static int index;
- return pathname_array[(PATHNAME_BUFFER_COUNT - 1) & ++index];
+#ifdef VALGRIND
+ static char *pathname_array[PATHNAME_BUFFER_COUNT];
+ index = (index + 1) & (PATHNAME_BUFFER_COUNT - 1);
+ if (pathname_array[index]) {
+ /*
+ * In a correct program, this will have no effect, but
+ * *if* somebody erroneously uses this buffer after it
+ * has been freed, it gives more of a chance that the
+ * error will be detected even if valgrind is not
+ * running:
+ */
+ strcpy(pathname_array[index], buggy_path);
+
+ free(pathname_array[index]);
+ }
+ pathname_array[index] = xmalloc(PATH_MAX);
+ return pathname_array[index];
+#else
+ static char pathname_array[PATHNAME_BUFFER_COUNT][PATH_MAX];
+ return pathname_array[(PATHNAME_BUFFER_COUNT - 1) & ++index];
+#endif
}
static char *cleanup_path(char *path)
--
1.7.7.rc2
^ permalink raw reply
* [PATCH] notes_merge_commit(): do not pass temporary buffer to other function
From: Michael Haggerty @ 2011-09-27 4:46 UTC (permalink / raw)
To: git; +Cc: Johan Herland, Junio C Hamano, Michael Haggerty
It is unsafe to pass a temporary buffer as an argument to
read_directory().
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
I discovered this problem when an innocent modification to unrelated
code triggered test failures.
notes-merge.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git notes-merge.c notes-merge.c
index e1aaf43..baaf31f 100644
--- notes-merge.c
+++ notes-merge.c
@@ -680,7 +680,7 @@ int notes_merge_commit(struct notes_merge_options *o,
* Finally store the new commit object SHA1 into 'result_sha1'.
*/
struct dir_struct dir;
- const char *path = git_path(NOTES_MERGE_WORKTREE "/");
+ char *path = xstrdup(git_path(NOTES_MERGE_WORKTREE "/"));
int path_len = strlen(path), i;
const char *msg = strstr(partial_commit->buffer, "\n\n");
@@ -720,6 +720,7 @@ int notes_merge_commit(struct notes_merge_options *o,
result_sha1);
OUTPUT(o, 4, "Finalized notes merge commit: %s",
sha1_to_hex(result_sha1));
+ free(path);
return 0;
}
--
1.7.7.rc2
^ permalink raw reply
* Re: [PATCH] git-remote rename should match whole string when renaming remote ref directory
From: Junio C Hamano @ 2011-09-27 6:07 UTC (permalink / raw)
To: Benny Halevy; +Cc: Martin von Zweigbergk, git
In-Reply-To: <4E8138B3.9090909@tonian.com>
Benny Halevy <bhalevy@tonian.com> writes:
> On 2011-09-26 21:04, Junio C Hamano wrote:
>> Benny Halevy <benny@tonian.com> writes:
>> ...
>>> - if (!prefixcmp(refname, buf.buf)) {
>>> + if (!strcmp(refname, buf.buf)) {
>>
>> At this point of the code, refname has "refs/remotes/test/foo" and it is
>> queued to later rename it to "refs/remotes/test-/foo" (the next invocation
>> of this function will see "refs/remotes/test/bar" in refname). And the
>> strbuf buf.buf has "refs/remotes/test"; your !strcmp(refname, buf.buf)
>> would never trigger, I suspect.
>>
>> Isn't 60e5eee (remote: "rename o foo" should not rename ref "origin/bar",
>> 2011-09-01) the correct fix for this issue?...
>
> OK, 60e5eee solves the problem too.
Hmm, what do you mean by "too" here? Martin's patch fixes the issue, but
does yours, too?
> FWIW, here's the test I used:
> ...
> cd main || fail cd main failed
> for i in test test-2; do
> $git remote add $i file://$cwd/$i || fail git remote add $i failed
> done
> $git remote update || fail git remote update fail
> $git remote rename test test-
> $git show test-2/master || fail FAILED
> echo PASSED
Before your last "echo PASSED", add this line:
$git show test-/master || fail FAILED
and see what happens with your patch.
It is unfortunately a rather common trap to fall into, so I wouldn't blame
you too much. People tend to concentrate only on an aspect of the problem
that originally motivated them, and forget about the other issues that are
equally important, if not more. In this case, you were too thrilled to see
that your updated code no longer renames "test-2" mistakenly to "test--2",
and you forgot that the primary task of the resulting code was to rename
"test" to "test-" correctly. The additional line I gave you above is to
test that.
When testing your own code, make it a habit to _always_ test both sides of
the coin. It is somewhat difficult until you get used to it [*1*], but it
is a skill that is really worth acquiring.
Thanks.
[Footnote]
*1* ...and I do not claim that I myself never forget to fully enumerate
other sides; even experienced people still overlook and embarrass
themselves in public ;-)
^ permalink raw reply
* Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links
From: Johannes Sixt @ 2011-09-27 6:44 UTC (permalink / raw)
To: Peter Stuge; +Cc: Junio C Hamano, git
In-Reply-To: <20110926222801.14985.qmail@stuge.se>
Am 9/27/2011 0:28, schrieb Peter Stuge:
> Junio C Hamano wrote:
>> You don't have to explain these to *me* specifically as a response
>> to this thread. What I meant was that your patch should have these
>> necessary descriptions in its proposed commit log message.
>
> IMO not so neccessary if one knows a little web and javascript, which
> is probably likely for a gitweb change..
>
> It's a simple fix of links broken by manual URI manipulation that
> didn't consider fragments. Is the subject description really not
> enough?
No, it is not. The target audience of a commit message are people like I
am. I do know a bit of Perl, and a bit of Javascript; I know how an URL is
structured; I would find my way through the gitweb code if the need
arises. But I am not an expert in any of these areas.
The subject alone is not sufficient because I do not know for sure what an
"URI fragment" is or what role line numbers in gitweb's links play. The
explanations and examples you gave in a later email were very
enlightening, and they would be very helpful if *I* am forced to hack
gitweb, and if I need to understand why this particular change was good.
Finding the right balance between verbosity and terseness needs practice,
but to write *no* justification is practically always wrong.
-- Hannes
^ permalink raw reply
* Re: [PATCH] git-submodule: a small fix
From: Jens Lehmann @ 2011-09-27 7:24 UTC (permalink / raw)
To: Roy Liu; +Cc: Andrew Ardill, git
In-Reply-To: <CAH5451k-6HHx2xxddJauE8=P1umjG=TnrcOKmQfeh=4GOzpCKQ@mail.gmail.com>
Am 27.09.2011 05:22, schrieb Andrew Ardill:
> On 27 September 2011 08:00, Roy Liu <carsomyr@gmail.com> wrote:
>> In git-submodule.sh, the "url" variable may contain a stale value from
>> the previous loop iteration, so clear it.
>>
>> --- git-submodule.sh.orig 2011-09-26 17:50:45.000000000 -0400
>> +++ git-submodule.sh 2011-09-26 17:51:18.000000000 -0400
>> @@ -370,6 +370,8 @@
>> esac
>> git config submodule."$name".url "$url" ||
>> die "Failed to register url for submodule path '$path'"
>> + else
>> + url=""
>> fi
>>
>> # Copy "update" setting when it is not set yet
>
> Perhaps a better commit description would be:
>
> git-submodule: clear the url variable when not set to avoid using stale values
Yes, the commit description needs to describe what was changed (and you can
also drop the "git-", to start with "submodule:" is sufficient here). Also
it would be nice if the message would describe under what circumstances this
happens (how did you notice this problem?).
But I wonder if the patch does it the right way. While it fixes the basic
issue that "url" might not be set, I doubt it does what the user expects.
The place where the - sometimes uninitialized - variable "url" is used is
a few lines down:
say "$(eval_gettext "Submodule '\$name' (\$url) registered for path '\$path'")"
I doesn't make much sense to say "Submodule 'foo' () registered for path 'foo'"
here. Shouldn't "url" be set to "$(git config "submodule.$name.url")"? And
when looking at the if you added the else to it might make sense to set it
unconditionally before the if and then test "$url" there instead of adding an
extra else.
^ permalink raw reply
* Re: [PATCH] notes_merge_commit(): do not pass temporary buffer to other function
From: Johan Herland @ 2011-09-27 7:12 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Junio C Hamano
In-Reply-To: <1317098813-30839-1-git-send-email-mhagger@alum.mit.edu>
On Tue, Sep 27, 2011 at 06:46, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> It is unsafe to pass a temporary buffer as an argument to
> read_directory().
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
ACK.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: git svn clone issues with buddypress subversion repository
From: Anand Kumria @ 2011-09-27 7:44 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <4E803413.3080709@drmicha.warpmail.net>
Hi Michael,
On 26 September 2011 09:13, Michael J Gruber <git@drmicha.warpmail.net> wrote:
> Anand Kumria venit, vidit, dixit 24.09.2011 18:50:
>> Hi,
>>
>> I'm trying:
>>
>> % git svn clone -s http://svn.buddypress.org/trunk/ buddypress
>
> Please try
>
> git svn clone -s http://svn.buddypress.org/ buddypress
Yup - works perfectly!
Cheers,
Anand
^ permalink raw reply
* Re: Git is not scalable with too many refs/*
From: Julian Phillips @ 2011-09-27 7:59 UTC (permalink / raw)
To: Martin Fick; +Cc: git
In-Reply-To: <3539dab7-0fb2-4759-baaf-8e22efab2904@email.android.com>
On Mon, 26 Sep 2011 20:34:02 -0600, Martin Fick wrote:
>> Julian Phillips <julian@quantumfyre.co.uk> wrote:
>>On Mon, 26 Sep 2011 18:12:31 -0600, Martin Fick wrote:
>>That sounds a lot better. Hopefully other commands should be faster
>>now too.
>
> Yeah, I will try this in a few other places to see.
>
>>> Thanks way much!!!
>>
>>No problem. Thank you for all the time you've put in to help chase
>>this down. Makes it so much easier when the person with original
>>problem mucks in with the investigation.
>>Just think how much time you've saved for anyone with a large number
>> of
>>
>>those Gerrit change refs ;)
>
> Perhaps this is a naive question, but why are all these refs being
> put into a list to be sorted, only to be discarded soon thereafter
> anyway? After all, git branch knows that it isn't going to print
> these, and the refs are stored precategorized, so why not only grab
> the refs which matter upfront?
I can't say that I am aware of a specific decision having been taken on
the subject, but I'll have a guess at the reason:
The extra code it would take to have an API for getting a list of only
a subset of the refs has never been considered worth the cost. It would
take effort to implement, test and maintain - and it would have to be
done separately for packed and unpacked cases to avoid still loading and
discarding unwanted refs. All that to not do something that no-one has
noticed taking any time? Until now, I doubt anyone has considered it
something that was a problem - and now that even with 100k refs it takes
less than a second, I doubt anyone will feel all that inclined to have a
crack at it now either.
--
Julian
^ permalink raw reply
* Re: Git is not scalable with too many refs/*
From: Sverre Rabbelier @ 2011-09-27 8:20 UTC (permalink / raw)
To: Julian Phillips; +Cc: Martin Fick, git, Junio C Hamano, David Michael Barr
In-Reply-To: <ece30e6a1b74bcddde5634003408f61f@quantumfyre.co.uk>
Heya,
On Tue, Sep 27, 2011 at 01:26, Julian Phillips <julian@quantumfyre.co.uk> wrote:
> Back when I made that change, I failed to notice that get_ref_dir was
> recursive for subdirectories ... sorry ...
>
> Hopefully this should speed things up. My test repo went from ~17m user
> time, to ~2.5s.
> Packing still make things much faster of course.
Can we perhaps also have some tests to prevent this from happening again?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* [RFC/PATCHv2] git-p4: handle files with shell metacharacters
From: Luke Diamand @ 2011-09-27 8:40 UTC (permalink / raw)
To: git; +Cc: pw, vitor.hda, Luke Diamand
In-Reply-To: <20110926214758.GB3433@arf.padd.com>
Updated git-p4 changes incorporating Pete's comments.
- p4CmdList's stdin argument can now be a list.
- Getting rid of the string option entirely is very hard; there are
places where currently git-p4 creates a pipeline.
- I wonder if verbose should actually be enabled for all the test
cases?
- The $ENV{PWD} is needed now because the shell used to set that; now
that the shell isn't in use git-p4 has to set it.
Pete - I wasn't sure whether you were saying I should rework
my patch against next (and you would then rework your series) or
something else. That sounds complicated though - let me know!
Thanks!
Luke
Luke Diamand (1):
git-p4: handle files with shell metacharacters
contrib/fast-import/git-p4 | 200 ++++++++++++++++++++++++---------------
t/t9803-git-shell-metachars.sh | 70 ++++++++++++++
2 files changed, 193 insertions(+), 77 deletions(-)
create mode 100755 t/t9803-git-shell-metachars.sh
--
1.7.6.347.g4db0d
^ permalink raw reply
* [RFC/PATCHv2] git-p4: handle files with shell metacharacters
From: Luke Diamand @ 2011-09-27 8:40 UTC (permalink / raw)
To: git; +Cc: pw, vitor.hda, Luke Diamand
In-Reply-To: <1317112836-14135-1-git-send-email-luke@diamand.org>
git-p4 used to simply pass strings into system() and popen(), and
relied on the shell doing the necessary expansion. This though meant
that shell metacharacters in file names would be corrupted - for
example files with $ or space in them.
Switch to using subprocess.Popen() and friends, and pass in explicit
arrays in the places where it matters. This then avoids needing shell
expansion.
Add trivial helper functions for some common perforce operations. Add
test case.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
contrib/fast-import/git-p4 | 200 ++++++++++++++++++++++++---------------
t/t9803-git-shell-metachars.sh | 70 ++++++++++++++
2 files changed, 193 insertions(+), 77 deletions(-)
create mode 100755 t/t9803-git-shell-metachars.sh
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 782b891..9770304 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -22,36 +22,39 @@ def p4_build_cmd(cmd):
location. It means that hooking into the environment, or other configuration
can be done more easily.
"""
- real_cmd = "%s " % "p4"
+ real_cmd = ["p4"]
user = gitConfig("git-p4.user")
if len(user) > 0:
- real_cmd += "-u %s " % user
+ real_cmd += ["-u",user]
password = gitConfig("git-p4.password")
if len(password) > 0:
- real_cmd += "-P %s " % password
+ real_cmd += ["-P", password]
port = gitConfig("git-p4.port")
if len(port) > 0:
- real_cmd += "-p %s " % port
+ real_cmd += ["-p", port]
host = gitConfig("git-p4.host")
if len(host) > 0:
- real_cmd += "-h %s " % host
+ real_cmd += ["-h", host]
client = gitConfig("git-p4.client")
if len(client) > 0:
- real_cmd += "-c %s " % client
+ real_cmd += ["-c", client]
- real_cmd += "%s" % (cmd)
- if verbose:
- print real_cmd
+
+ if isinstance(cmd,basestring):
+ real_cmd = ' '.join(real_cmd) + ' ' + cmd
+ else:
+ real_cmd += cmd
return real_cmd
def chdir(dir):
- if os.name == 'nt':
- os.environ['PWD']=dir
+ # P4 uses the PWD environment variable rather than getcwd(). Since we're
+ # not using the shell, we have to set it ourselves.
+ os.environ['PWD']=dir
os.chdir(dir)
def die(msg):
@@ -61,29 +64,34 @@ def die(msg):
sys.stderr.write(msg + "\n")
sys.exit(1)
-def write_pipe(c, str):
+def write_pipe(c, stdin):
if verbose:
- sys.stderr.write('Writing pipe: %s\n' % c)
+ sys.stderr.write('Writing pipe: %s\n' % str(c))
- pipe = os.popen(c, 'w')
- val = pipe.write(str)
- if pipe.close():
- die('Command failed: %s' % c)
+ expand = isinstance(c,basestring)
+ p = subprocess.Popen(c, stdin=subprocess.PIPE, shell=expand)
+ pipe = p.stdin
+ val = pipe.write(stdin)
+ pipe.close()
+ if p.wait():
+ die('Command failed: %s' % str(c))
return val
-def p4_write_pipe(c, str):
+def p4_write_pipe(c, stdin):
real_cmd = p4_build_cmd(c)
- return write_pipe(real_cmd, str)
+ return write_pipe(real_cmd, stdin)
def read_pipe(c, ignore_error=False):
if verbose:
- sys.stderr.write('Reading pipe: %s\n' % c)
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
- pipe = os.popen(c, 'rb')
+ expand = isinstance(c,basestring)
+ p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
+ pipe = p.stdout
val = pipe.read()
- if pipe.close() and not ignore_error:
- die('Command failed: %s' % c)
+ if p.wait() and not ignore_error:
+ die('Command failed: %s' % str(c))
return val
@@ -93,12 +101,14 @@ def p4_read_pipe(c, ignore_error=False):
def read_pipe_lines(c):
if verbose:
- sys.stderr.write('Reading pipe: %s\n' % c)
- ## todo: check return status
- pipe = os.popen(c, 'rb')
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
+
+ expand = isinstance(c, basestring)
+ p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
+ pipe = p.stdout
val = pipe.readlines()
- if pipe.close():
- die('Command failed: %s' % c)
+ if pipe.close() or p.wait():
+ die('Command failed: %s' % str(c))
return val
@@ -108,15 +118,37 @@ def p4_read_pipe_lines(c):
return read_pipe_lines(real_cmd)
def system(cmd):
+ expand = isinstance(cmd,basestring)
if verbose:
- sys.stderr.write("executing %s\n" % cmd)
- if os.system(cmd) != 0:
- die("command failed: %s" % cmd)
+ sys.stderr.write("executing %s\n" % str(cmd))
+ subprocess.check_call(cmd, shell=expand)
def p4_system(cmd):
"""Specifically invoke p4 as the system command. """
real_cmd = p4_build_cmd(cmd)
- return system(real_cmd)
+ expand = isinstance(real_cmd, basestring)
+ subprocess.check_call(real_cmd, shell=expand)
+
+def p4_integrate(src, dest):
+ p4_system(["integrate", "-Dt", src, dest])
+
+def p4_sync(path):
+ p4_system(["sync", path])
+
+def p4_add(f):
+ p4_system(["add", f])
+
+def p4_delete(f):
+ p4_system(["delete", f])
+
+def p4_edit(f):
+ p4_system(["edit", f])
+
+def p4_revert(f):
+ p4_system(["revert", f])
+
+def p4_reopen(type, file):
+ p4_system(["reopen", "-t", type, file])
#
# Canonicalize the p4 type and return a tuple of the
@@ -167,12 +199,12 @@ def setP4ExecBit(file, mode):
if p4Type[-1] == "+":
p4Type = p4Type[0:-1]
- p4_system("reopen -t %s %s" % (p4Type, file))
+ p4_reopen(p4Type, file)
def getP4OpenedType(file):
# Returns the perforce file type for the given file.
- result = p4_read_pipe("opened %s" % file)
+ result = p4_read_pipe(["opened", file])
match = re.match(".*\((.+)\)\r?$", result)
if match:
return match.group(1)
@@ -228,9 +260,17 @@ def isModeExecChanged(src_mode, dst_mode):
return isModeExec(src_mode) != isModeExec(dst_mode)
def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
- cmd = p4_build_cmd("-G %s" % (cmd))
+
+ if isinstance(cmd,basestring):
+ cmd = "-G " + cmd
+ expand = True
+ else:
+ cmd = ["-G"] + cmd
+ expand = False
+
+ cmd = p4_build_cmd(cmd)
if verbose:
- sys.stderr.write("Opening pipe: %s\n" % cmd)
+ sys.stderr.write("Opening pipe: %s\n" % str(cmd))
# Use a temporary file to avoid deadlocks without
# subprocess.communicate(), which would put another copy
@@ -238,11 +278,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
stdin_file = None
if stdin is not None:
stdin_file = tempfile.TemporaryFile(prefix='p4-stdin', mode=stdin_mode)
- stdin_file.write(stdin)
+ if isinstance(stdin,basestring):
+ stdin_file.write(stdin)
+ else:
+ for i in stdin:
+ stdin_file.write(i + '\n')
stdin_file.flush()
stdin_file.seek(0)
- p4 = subprocess.Popen(cmd, shell=True,
+ p4 = subprocess.Popen(cmd,
+ shell=expand,
stdin=stdin_file,
stdout=subprocess.PIPE)
@@ -275,7 +320,7 @@ def p4Where(depotPath):
if not depotPath.endswith("/"):
depotPath += "/"
depotPath = depotPath + "..."
- outputList = p4CmdList("where %s" % depotPath)
+ outputList = p4CmdList(["where", depotPath])
output = None
for entry in outputList:
if "depotFile" in entry:
@@ -477,8 +522,10 @@ def originP4BranchesExist():
def p4ChangesForPaths(depotPaths, changeRange):
assert depotPaths
- output = p4_read_pipe_lines("changes " + ' '.join (["%s...%s" % (p, changeRange)
- for p in depotPaths]))
+ cmd = ['changes']
+ for p in depotPaths:
+ cmd += ["%s...%s" % (p, changeRange)]
+ output = p4_read_pipe_lines(cmd)
changes = {}
for line in output:
@@ -561,7 +608,7 @@ class P4Debug(Command):
def run(self, args):
j = 0
- for output in p4CmdList(" ".join(args)):
+ for output in p4CmdList(args):
print 'Element: %d' % j
j += 1
print output
@@ -715,7 +762,7 @@ class P4Submit(Command, P4UserMap):
break
if not client:
die("could not get client spec")
- results = p4CmdList("changes -c %s -m 1" % client)
+ results = p4CmdList(["changes", "-c", client, "-m", "1"])
for r in results:
if r.has_key('change'):
return r['change']
@@ -778,7 +825,7 @@ class P4Submit(Command, P4UserMap):
# remove lines in the Files section that show changes to files outside the depot path we're committing into
template = ""
inFilesSection = False
- for line in p4_read_pipe_lines("change -o"):
+ for line in p4_read_pipe_lines(['change', '-o']):
if line.endswith("\r\n"):
line = line[:-2] + "\n"
if inFilesSection:
@@ -835,7 +882,7 @@ class P4Submit(Command, P4UserMap):
modifier = diff['status']
path = diff['src']
if modifier == "M":
- p4_system("edit \"%s\"" % path)
+ p4_edit(path)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
filesToChangeExecBit[path] = diff['dst_mode']
editedFiles.add(path)
@@ -850,21 +897,21 @@ class P4Submit(Command, P4UserMap):
filesToAdd.remove(path)
elif modifier == "C":
src, dest = diff['src'], diff['dst']
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+ p4_integrate(src, dest)
if diff['src_sha1'] != diff['dst_sha1']:
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
filesToChangeExecBit[dest] = diff['dst_mode']
os.unlink(dest)
editedFiles.add(dest)
elif modifier == "R":
src, dest = diff['src'], diff['dst']
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+ p4_integrate(src, dest)
if diff['src_sha1'] != diff['dst_sha1']:
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
filesToChangeExecBit[dest] = diff['dst_mode']
os.unlink(dest)
editedFiles.add(dest)
@@ -887,9 +934,9 @@ class P4Submit(Command, P4UserMap):
if response == "s":
print "Skipping! Good luck with the next patches..."
for f in editedFiles:
- p4_system("revert \"%s\"" % f);
+ p4_revert(f)
for f in filesToAdd:
- system("rm %s" %f)
+ os.remove(f)
return
elif response == "a":
os.system(applyPatchCmd)
@@ -910,10 +957,10 @@ class P4Submit(Command, P4UserMap):
system(applyPatchCmd)
for f in filesToAdd:
- p4_system("add \"%s\"" % f)
+ p4_add(f)
for f in filesToDelete:
- p4_system("revert \"%s\"" % f)
- p4_system("delete \"%s\"" % f)
+ p4_revert(f)
+ p4_delete(f)
# Set/clear executable bits
for f in filesToChangeExecBit.keys():
@@ -935,7 +982,7 @@ class P4Submit(Command, P4UserMap):
del(os.environ["P4DIFF"])
diff = ""
for editedFile in editedFiles:
- diff += p4_read_pipe("diff -du %r" % editedFile)
+ diff += p4_read_pipe(['diff', '-du', editedFile])
newdiff = ""
for newFile in filesToAdd:
@@ -987,7 +1034,7 @@ class P4Submit(Command, P4UserMap):
submitTemplate = message[:message.index(separatorLine)]
if self.isWindows:
submitTemplate = submitTemplate.replace("\r\n", "\n")
- p4_write_pipe("submit -i", submitTemplate)
+ p4_write_pipe(['submit', '-i'], submitTemplate)
if self.preserveUser:
if p4User:
@@ -998,10 +1045,10 @@ class P4Submit(Command, P4UserMap):
else:
for f in editedFiles:
- p4_system("revert \"%s\"" % f);
+ p4_revert(f)
for f in filesToAdd:
- p4_system("revert \"%s\"" % f);
- system("rm %s" %f)
+ p4_revert(f)
+ os.remove(f)
os.remove(fileName)
else:
@@ -1054,8 +1101,7 @@ class P4Submit(Command, P4UserMap):
chdir(self.clientPath)
print "Synchronizing p4 checkout..."
- p4_system("sync ...")
-
+ p4_sync("...")
self.check()
commits = []
@@ -1269,7 +1315,7 @@ class P4Sync(Command, P4UserMap):
# operations. utf16 is converted to ascii or utf8, perhaps.
# But ascii text saved as -t utf16 is completely mangled.
# Invoke print -o to get the real contents.
- text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
+ text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
contents = [ text ]
# Perhaps windows wants unicode, utf16 newlines translated too;
@@ -1365,10 +1411,11 @@ class P4Sync(Command, P4UserMap):
def streamP4FilesCbSelf(entry):
self.streamP4FilesCb(entry)
- p4CmdList("-x - print",
- '\n'.join(['%s#%s' % (f['path'], f['rev'])
- for f in filesToRead]),
- cb=streamP4FilesCbSelf)
+ fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
+
+ p4CmdList(["-x", "-", "print"],
+ stdin=fileArgs,
+ cb=streamP4FilesCbSelf)
# do the last chunk
if self.stream_file.has_key('depotFile'):
@@ -1429,8 +1476,8 @@ class P4Sync(Command, P4UserMap):
if self.verbose:
print "Change %s is labelled %s" % (change, labelDetails)
- files = p4CmdList("files " + ' '.join (["%s...@%s" % (p, change)
- for p in branchPrefixes]))
+ files = p4CmdList(["files"] + ["%s...@%s" % (p, change)
+ for p in branchPrefixes])
if len(files) == len(labelRevisions):
@@ -1478,9 +1525,9 @@ class P4Sync(Command, P4UserMap):
newestChange = 0
if self.verbose:
print "Querying files for label %s" % label
- for file in p4CmdList("files "
- + ' '.join (["%s...@%s" % (p, label)
- for p in self.depotPaths])):
+ for file in p4CmdList(["files"] +
+ ["%s...@%s" % (p, label)
+ for p in self.depotPaths]):
revisions[file["depotFile"]] = file["rev"]
change = int(file["change"])
if change > newestChange:
@@ -1735,10 +1782,9 @@ class P4Sync(Command, P4UserMap):
newestRevision = 0
fileCnt = 0
- for info in p4CmdList("files "
- + ' '.join(["%s...%s"
- % (p, revision)
- for p in self.depotPaths])):
+ fileArgs = ["%s...%s" % (p,revision) for p in self.depotPaths]
+
+ for info in p4CmdList(["files"] + fileArgs):
if 'code' in info and info['code'] == 'error':
sys.stderr.write("p4 returned an error: %s\n"
diff --git a/t/t9803-git-shell-metachars.sh b/t/t9803-git-shell-metachars.sh
new file mode 100755
index 0000000..c166603
--- /dev/null
+++ b/t/t9803-git-shell-metachars.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='git-p4 transparency to shell metachars in filenames'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ kill_p4d || : &&
+ start_p4d &&
+ cd "$TRASH_DIRECTORY"
+'
+
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "file1"
+ )
+'
+
+test_expect_success 'shell metachars in filenames' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ echo f1 >foo\$bar &&
+ git add foo\$bar &&
+ echo f2 >"file with spaces" &&
+ git add "file with spaces" &&
+ P4EDITOR=touch git commit -m "add files" &&
+ "$GITP4" submit --verbose &&
+ cd "$cli" &&
+ p4 sync ... &&
+ ls -l "file with spaces" &&
+ ls -l "foo\$bar"
+ )
+'
+
+check_missing () {
+ for i in $*; do
+ if [ -f $i ]; then
+ echo $i found but should be missing 1>&2
+ exit 1
+ fi
+ done
+}
+
+test_expect_success 'deleting with shell metachars' '
+ "$GITP4" clone --dest="$git" --verbose //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ git rm foo\$bar &&
+ git rm file\ with\ spaces &&
+ P4EDITOR=touch git commit -m "remove files" &&
+ "$GITP4" submit --verbose
+ cd "$cli" &&
+ p4 sync ... &&
+ check_missing "file with spaces" foo\$bar
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.6.347.g4db0d
^ permalink raw reply related
* Re: [PATCH v0] fast-import: Add drop command
From: Vitor Antunes @ 2011-09-27 8:57 UTC (permalink / raw)
To: Dmitry Ivankov; +Cc: Jonathan Nieder, git, Sverre Rabbelier, David Barr
In-Reply-To: <CA+gfSn8Z7Xn1hdpqNHiP3bd2KGRqcAc6O683Z4O+G=jNNYJtBA@mail.gmail.com>
On Sat, Sep 24, 2011 at 10:19 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:
> On Sun, Sep 25, 2011 at 1:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Thanks. I must have missed the earlier discussion. What are the
>> semantics of this command and its intended purpose?
> My guess is that if fast-import is used to manage a set of "remote"
> branches, it should be able to delete branches. Then, it should
> be allowed to do non-fastforward updates too (--force). Why can't
> it just ignore branches deletion (considering --force)?
I started by using --force, but I did not want to completely disable
these checks. The idea of the drop command is to add support to the
exceptions that require non-fastforward updates.
> Random thoughts:
> 1. once 'drop' is executed, fast-import can't tell if the branch was
> actually deleted. And moreover any attempt to read this branch
> head becomes illegal (either it's missing in .git or fast-import is
> instructed to use a dropped branch).
> 2. 'reset' command is a bit like proposed 'drop' but it never deletes
> a branch ref. Consider following imports:
> 1) import branch topic
> 2) reset topic
> 3) import branch topic2 starting at topic (incorrect import)
> If 1-3) is done in one fast-import process, the error is reported.
> If 3) is done separately, it succeeds but the result is strange:
> topic2 isn't started from scratch but from old "erased" topic.
> So, maybe, reset should be fixed to erase branches on --force.
I think you are not considering the possibility that checkpoints could
have been done along the way. I use them frequently to be able to
analyse branches with diff-tree. As soon as a checkpoint is done,
update-branches will issue an error (commit A is not part of branch A').
> One more scenario is:
> 1) import topic
> 2) reset topic
> 3) import topic
> If 1-3) go together - no error
> If 3) goes separate - no error, but non-fastforward update.
> Much more harmless, but still may look strange.
Not exactly true if there is a checkpoint done after step 1.
My scenario is:
1) import topic
2) checkpoint
3) diff-tree and processing
4) exit if processing returns ok
5) reset topic to another HEAD
6) goto 1)
--
Vitor Antunes
^ permalink raw reply
* Re: Git is not scalable with too many refs/*
From: Julian Phillips @ 2011-09-27 9:01 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Martin Fick, git, Junio C Hamano, David Michael Barr
In-Reply-To: <CAGdFq_hvR1MPF33YFcjDCzCM0iOO2zpiiePFFS4dBabu84cwTg@mail.gmail.com>
On Tue, 27 Sep 2011 10:20:29 +0200, Sverre Rabbelier wrote:
> Heya,
>
> On Tue, Sep 27, 2011 at 01:26, Julian Phillips
> <julian@quantumfyre.co.uk> wrote:
>> Back when I made that change, I failed to notice that get_ref_dir
>> was
>> recursive for subdirectories ... sorry ...
>>
>> Hopefully this should speed things up. My test repo went from ~17m
>> user
>> time, to ~2.5s.
>> Packing still make things much faster of course.
>
> Can we perhaps also have some tests to prevent this from happening
> again?
Um ... any suggestion what to test?
It has to be hot-cache, otherwise time taken to read the refs from disk
will mean that it is always slow. On my Mac it seems to _always_ be
slow reading the refs from disk, so even the "fast" case still takes
~17m.
Also, what counts as ok, and what as broken?
--
Julian
^ permalink raw reply
* Re: [PATCH/RFC 0/2] Teach receive-pack not to run update hook for corrupt/non existent ref
From: Pang Yan Han @ 2011-09-27 9:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Sitaram Chamarty, Shawn O. Pearce, Jeff King,
Johannes Schindelin, Pang Yan Han
In-Reply-To: <7vd3emzw8n.fsf@alter.siamese.dyndns.org>
Hi Junio,
Should I reroll this patch with this behaviour:
- Everything as usual for valid ref updates and deletes
- For deleting corrupt (dangling?) ref, post-receive and post-update hooks
also receive the same args as per valid update / delete
- For deleting non-existent refs:
- post-receive shall have empty stdin for those refs
- post-update shall have an empty arg for those refs
Thanks.
On Mon, Sep 26, 2011 at 05:04:24PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Sitaram Chamarty <sitaramc@gmail.com> writes:
> >
> >>> In that case (if "non-existent-ref" was indeed non-existent, and not just
> >>> pointing at a dangling commit), I would say the post anything hook should
> >>> not be called for that ref. These hooks of course need to run if there
> >>> are _other_ refs that were updated, though, to handle these _other_ refs,
> >>> but I do not think they should be told about the no-op.
> >>
> >> Question is what happens if none of them existed. It's a difference
> >> between not calling the hook at all, versus calling it with no
> >> arguments/empty stdin (as the case may be) -- which would you do?
> >
> > In case it was unclear, I was trying to say the hooks should not run with
> > empty input.
>
> If the purpose of "post-update" (or "post-receive") hooks were to trigger
> every time anybody attempted to push into the repository, then it would
> make perfect sense for them to trigger when "push origin :no-such-branch"
> were attempted. But if that were the purpose of these hooks, they should
> also trigger when "push origin master" is run and "master" is already at
> the right commit, as that is the same kind of no-op -- the pushed into
> repository was already up-to-date with respect to the wish of the pusher.
>
> I do not mind, and I do prefer, these hooks to run when somebody deleted
> an existing ref that points at a corrupt or non-existent object, as that
> is _not_ a no-op but is a meaningful event that has an effect that is
> observable from the outside world (e.g. ls-remote).
^ permalink raw reply
* Re: [RFC/PATCHv2] git-p4: handle files with shell metacharacters
From: Luke Diamand @ 2011-09-27 9:32 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, pw, vitor.hda
In-Reply-To: <1317112836-14135-1-git-send-email-luke@diamand.org>
On 27/09/11 09:40, Luke Diamand wrote:
> Updated git-p4 changes incorporating Pete's comments.
>
> - p4CmdList's stdin argument can now be a list.
- the only purpose of this is to avoid constructing large strings. I
should probably submit as a separate patch.
>
> - Getting rid of the string option entirely is very hard; there are
> places where currently git-p4 creates a pipeline.
>
> - I wonder if verbose should actually be enabled for all the test
> cases?
>
> - The $ENV{PWD} is needed now because the shell used to set that; now
> that the shell isn't in use git-p4 has to set it.
>
> Pete - I wasn't sure whether you were saying I should rework
> my patch against next (and you would then rework your series) or
> something else. That sounds complicated though - let me know!
>
> Thanks!
> Luke
>
> Luke Diamand (1):
> git-p4: handle files with shell metacharacters
>
> contrib/fast-import/git-p4 | 200 ++++++++++++++++++++++++---------------
> t/t9803-git-shell-metachars.sh | 70 ++++++++++++++
> 2 files changed, 193 insertions(+), 77 deletions(-)
> create mode 100755 t/t9803-git-shell-metachars.sh
>
^ permalink raw reply
* Re: Git for animation studio
From: Jonathan Nieder @ 2011-09-27 9:34 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Will Hoag, git, peff
In-Reply-To: <m3sjnkpx75.fsf@localhost.localdomain>
Jakub Narebski wrote:
> You can [probably] find more information on tools page of Git
> Wiki... when it will be up.
In the meantime there is a backup[1] from January.
[1] Okay, it's more than a backup. Thanks, Jeff!
https://github.com/peff/wikitest/blob/master/Interfaces,_frontends,_and_tools.wiki
^ permalink raw reply
* Re: [PATCH] git-remote rename should match whole string when renaming remote ref directory
From: Benny Halevy @ 2011-09-27 9:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Martin von Zweigbergk, git
In-Reply-To: <7voby6y0vs.fsf@alter.siamese.dyndns.org>
On 2011-09-27 09:07, Junio C Hamano wrote:
> Benny Halevy <bhalevy@tonian.com> writes:
>
>> On 2011-09-26 21:04, Junio C Hamano wrote:
>>> Benny Halevy <benny@tonian.com> writes:
>>> ...
>>>> - if (!prefixcmp(refname, buf.buf)) {
>>>> + if (!strcmp(refname, buf.buf)) {
>>>
>>> At this point of the code, refname has "refs/remotes/test/foo" and it is
>>> queued to later rename it to "refs/remotes/test-/foo" (the next invocation
>>> of this function will see "refs/remotes/test/bar" in refname). And the
>>> strbuf buf.buf has "refs/remotes/test"; your !strcmp(refname, buf.buf)
>>> would never trigger, I suspect.
>>>
>>> Isn't 60e5eee (remote: "rename o foo" should not rename ref "origin/bar",
>>> 2011-09-01) the correct fix for this issue?...
>>
>> OK, 60e5eee solves the problem too.
>
> Hmm, what do you mean by "too" here? Martin's patch fixes the issue, but
> does yours, too?
>
correct.
>> FWIW, here's the test I used:
>> ...
>> cd main || fail cd main failed
>> for i in test test-2; do
>> $git remote add $i file://$cwd/$i || fail git remote add $i failed
>> done
>> $git remote update || fail git remote update fail
>> $git remote rename test test-
>> $git show test-2/master || fail FAILED
>> echo PASSED
>
> Before your last "echo PASSED", add this line:
>
> $git show test-/master || fail FAILED
>
> and see what happens with your patch.
Yup, with my patch this test indeed fails, and with Martin's
it passes.
Thanks!
Benny
>
> It is unfortunately a rather common trap to fall into, so I wouldn't blame
> you too much. People tend to concentrate only on an aspect of the problem
> that originally motivated them, and forget about the other issues that are
> equally important, if not more. In this case, you were too thrilled to see
> that your updated code no longer renames "test-2" mistakenly to "test--2",
> and you forgot that the primary task of the resulting code was to rename
> "test" to "test-" correctly. The additional line I gave you above is to
> test that.
>
> When testing your own code, make it a habit to _always_ test both sides of
> the coin. It is somewhat difficult until you get used to it [*1*], but it
> is a skill that is really worth acquiring.
>
> Thanks.
>
>
> [Footnote]
>
> *1* ...and I do not claim that I myself never forget to fully enumerate
> other sides; even experienced people still overlook and embarrass
> themselves in public ;-)
^ permalink raw reply
* Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links
From: Peter Stuge @ 2011-09-27 9:49 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git
In-Reply-To: <4E8170B3.8040205@viscovery.net>
Hey!
Johannes Sixt wrote:
> > It's a simple fix of links broken by manual URI manipulation that
> > didn't consider fragments. Is the subject description really not
> > enough?
>
> No, it is not. The target audience of a commit message are people like I
> am. I do know a bit of Perl, and a bit of Javascript; I know how an URL
> is structured; I would find my way through the gitweb code if the need
> arises. But I am not an expert in any of these areas.
>
> The subject alone is not sufficient because I do not know for sure what
> an "URI fragment" is or what role line numbers in gitweb's links play.
I shall continue playing advocatus diaboli only a little longer.
> The explanations and examples you gave in a later email were very
> enlightening, and they would be very helpful if *I* am forced to hack
> gitweb, and if I need to understand why this particular change was good.
On the other hand you're just one quick google search on uri fragment
away from the same enlightenment, and relying on terminology saves on
unneccessary redundance.
Lorelei: That's repetitive
Rory: ..and redundant.
Lorelei: That's repetitive
Rory: ..and redundant.
(SCNR the pop culture reference! :)
> Finding the right balance between verbosity and terseness needs
> practice,
I disagree, but I agree with you if we qualify that a little. The
right balance is a matter of subjective review, so the only way it
can be practiced with relevance is by actually working with the same
reviewers for a while, to learn what they consider right.
It can absolutely not be practiced out of context, ie. with different
peers. No later than the day before I sent this patch I wrote a
welcome mail in another open source project, to a new contributor,
where one bit was about commit messages.
http://marc.info/?l=openocd-development&m=131698532523018
"* Write a top quality commit message, technically and logically
...
As for the logical quality, it is important to write the first line
description of the change so that it makes sense for someone who
knows nothing at all about the code, since this is used in list
views, and since the background for this code and for why this change
was done the way it was done comes only in the later lines, which may
not be available from where that list view is. ... Keep it
high level, clear and simple. Writing this one line is not
neccessarily easy."
I of course also try to practise exactly this, but it's difficult to
know what reviewers expect to be fed, or how much verbosity they
prefer. :) I tend to prefer as much useful information as possible in
the first line, while keeping it ideally <60 chars. Many times I find
it to be enough.
> but to write *no* justification is practically always wrong.
I disagree strongly that I wrote no justification. I agree that it
was not verbose. I'm sorry that this is a problem. I'm personally
allergic to redundancy such as the commit message Jakub wrote, I
think it's not only reasonable but also desirable to avoid that.
Maybe gitweb is a special case in git.git though, I don't know, but
I'm a little surprised. :)
Anyway, I'm more than happy to write a more verbose message for you!
//Peter
^ permalink raw reply
* [PATCH] gitweb: Fix links to lines in blobs when javascript-actions are enabled
From: Peter Stuge @ 2011-09-27 9:51 UTC (permalink / raw)
To: git
In-Reply-To: <20110927094947.10955.qmail@stuge.se>
Some javascript code will run in the browser onLoad and signals back to
gitweb that the browser can actually do javascript.
The code adds [?;]js=1 into the URL of all links on the page. The code
always added [?;]js=1 to the end of links, which is wrong when links
contain a URI fragment, such as links directly to a line in a blob:
..?p=repo.git;a=blob;f=file#l123
In this case, [?;]js=1 must be added before the hashmark.
Signed-off-by: Peter Stuge <peter@stuge.se>
---
gitweb/static/js/javascript-detection.js | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js
index 93dd2bd..fa2596f 100644
--- a/gitweb/static/js/javascript-detection.js
+++ b/gitweb/static/js/javascript-detection.js
@@ -16,7 +16,7 @@
* and other reasons to not add 'js=1' param at the end of link
* @constant
*/
-var jsExceptionsRe = /[;?]js=[01]$/;
+var jsExceptionsRe = /[;?]js=[01](#.*)?$/;
/**
* Add '?js=1' or ';js=1' to the end of every link in the document
@@ -33,9 +33,9 @@ function fixLinks() {
var allLinks = document.getElementsByTagName("a") || document.links;
for (var i = 0, len = allLinks.length; i < len; i++) {
var link = allLinks[i];
- if (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;
- link.href +=
- (link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';
+ if (!jsExceptionsRe.test(link)) {
+ link.href = link.href.replace(/(#|$)/,
+ (link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');
}
}
}
--
1.7.4.1.343.ga91df.dirty
^ permalink raw reply related
* Re: Git is not scalable with too many refs/*
From: Sverre Rabbelier @ 2011-09-27 10:01 UTC (permalink / raw)
To: Julian Phillips; +Cc: Martin Fick, git, Junio C Hamano, David Michael Barr
In-Reply-To: <22f055b34840e3c64f3339f7b3dc6920@quantumfyre.co.uk>
Heya,
On Tue, Sep 27, 2011 at 11:01, Julian Phillips <julian@quantumfyre.co.uk> wrote:
> It has to be hot-cache, otherwise time taken to read the refs from disk will
> mean that it is always slow. On my Mac it seems to _always_ be slow reading
> the refs from disk, so even the "fast" case still takes ~17m.
Ah, that seems unfortunate. Not sure how to test it then.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: Git is not scalable with too many refs/*
From: Nguyen Thai Ngoc Duy @ 2011-09-27 10:25 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Julian Phillips, Martin Fick, git, Junio C Hamano,
David Michael Barr
In-Reply-To: <CAGdFq_j2aa8bwxWuJvEsgA_1zkR4mMzoKjGs9TQVqw+0XYr98A@mail.gmail.com>
On Tue, Sep 27, 2011 at 8:01 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Tue, Sep 27, 2011 at 11:01, Julian Phillips <julian@quantumfyre.co.uk> wrote:
>> It has to be hot-cache, otherwise time taken to read the refs from disk will
>> mean that it is always slow. On my Mac it seems to _always_ be slow reading
>> the refs from disk, so even the "fast" case still takes ~17m.
>
> Ah, that seems unfortunate. Not sure how to test it then.
If you care about performance, a perf test suite could be made,
perhaps as a separate project. The output would be charts or
spreadsheets, that interesting parties can look at and point out
regressions. We may start with a set of common used operations.
--
Duy
^ permalink raw reply
* Re: Git is not scalable with too many refs/*
From: Michael Haggerty @ 2011-09-27 11:07 UTC (permalink / raw)
To: Julian Phillips
Cc: Sverre Rabbelier, Martin Fick, git, Junio C Hamano,
David Michael Barr
In-Reply-To: <22f055b34840e3c64f3339f7b3dc6920@quantumfyre.co.uk>
On 09/27/2011 11:01 AM, Julian Phillips wrote:
> It has to be hot-cache, otherwise time taken to read the refs from disk
> will mean that it is always slow. On my Mac it seems to _always_ be
> slow reading the refs from disk, so even the "fast" case still takes ~17m.
This case should be helped by lazy-loading of loose references, which I
am working on. So if you develop some benchmarking code, it would help
me with my work.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [PATCH] templates/hooks--*: remove sample hooks without any functionality
From: Gerrit Pape @ 2011-09-27 11:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbou742eg.fsf@alter.siamese.dyndns.org>
Remove the sample post-commit and post-receive hooks. The sample
post-commit doesn't contain any sample functionality and the comments do
not provide more information than already found in the documentation.
The sample post-receive hooks doesn't provide any sample functionality
either and refers in the comments to a contrib hook that might be
installed in different locations on different systems, which isn't that
helpful.
Signed-off-by: Gerrit Pape <pape@smarden.org>
---
On Mon, Sep 26, 2011 at 10:52:23AM -0700, Junio C Hamano wrote:
> I removed the "-" lines above. Looking at the result, I really have to
> wonder if it makes much sense to keep the file here. It is not even an
> example anymore, and the user does not gain anything by enabling it,
> following the suggestion.
>
> Let's instead remove the file altogether, Ok?
Fine with me. As the same applies to the sample post-commit hook, I
made this patch to remove both of them.
Thanks for your patience, Gerrit.
templates/hooks--post-commit.sample | 8 --------
templates/hooks--post-receive.sample | 15 ---------------
2 files changed, 0 insertions(+), 23 deletions(-)
delete mode 100755 templates/hooks--post-commit.sample
delete mode 100755 templates/hooks--post-receive.sample
diff --git a/templates/hooks--post-commit.sample b/templates/hooks--post-commit.sample
deleted file mode 100755
index 2266821..0000000
--- a/templates/hooks--post-commit.sample
+++ /dev/null
@@ -1,8 +0,0 @@
-#!/bin/sh
-#
-# An example hook script that is called after a successful
-# commit is made.
-#
-# To enable this hook, rename this file to "post-commit".
-
-: Nothing
diff --git a/templates/hooks--post-receive.sample b/templates/hooks--post-receive.sample
deleted file mode 100755
index 7a83e17..0000000
--- a/templates/hooks--post-receive.sample
+++ /dev/null
@@ -1,15 +0,0 @@
-#!/bin/sh
-#
-# An example hook script for the "post-receive" event.
-#
-# The "post-receive" script is run after receive-pack has accepted a pack
-# and the repository has been updated. It is passed arguments in through
-# stdin in the form
-# <oldrev> <newrev> <refname>
-# For example:
-# aa453216d1b3e49e7f6f98441fa56946ddcd6a20 68f7abf4e6f922807889f52bc043ecd31b79f814 refs/heads/master
-#
-# see contrib/hooks/ for a sample, or uncomment the next line and
-# rename the file to "post-receive".
-
-#. /usr/share/doc/git-core/contrib/hooks/post-receive-email
--
1.7.6.3
^ permalink raw reply related
* Re: Git is not scalable with too many refs/*
From: Julian Phillips @ 2011-09-27 12:10 UTC (permalink / raw)
To: Michael Haggerty
Cc: Sverre Rabbelier, Martin Fick, git, Junio C Hamano,
David Michael Barr
In-Reply-To: <4E81AE63.8040008@alum.mit.edu>
[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]
On Tue, 27 Sep 2011 13:07:15 +0200, Michael Haggerty wrote:
> On 09/27/2011 11:01 AM, Julian Phillips wrote:
>> It has to be hot-cache, otherwise time taken to read the refs from
>> disk
>> will mean that it is always slow. On my Mac it seems to _always_ be
>> slow reading the refs from disk, so even the "fast" case still takes
>> ~17m.
>
> This case should be helped by lazy-loading of loose references, which
> I
> am working on. So if you develop some benchmarking code, it would
> help
> me with my work.
The attached script creates the repo structure I was testing with ...
If you create a repo with 100k refs it takes quite a while to read the
refs from disk. If you are lazy-loading then it should take practically
no time, since the only interesting ref is refs/heads/master.
The following is the hot-cache timing for "./refs-stress c 40000", with
the sorting patch applied (wasn't prepared to wait for numbers with 100k
refs).
jp3@rayne: refs>(cd c; time ~/misc/git/git/git branch)
* master
real 0m0.885s
user 0m0.161s
sys 0m0.722s
After doing "rm -rf c/.git/refs/changes/*", I get:
jp3@rayne: refs>(cd c; time ~/misc/git/git/git branch)
* master
real 0m0.004s
user 0m0.001s
sys 0m0.002s
--
Julian
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: refs-stress --]
[-- Type: text/x-java; name=refs-stress, Size: 1406 bytes --]
#!/usr/bin/env python
import os
import random
import subprocess
import sys
def die(msg):
print >> sys.stderr, msg
sys.exit(1)
def new_ref(a, b, commit):
d = ".git/refs/changes/%d/%d" % (a, b)
if not os.path.exists(d):
os.makedirs(d)
e = 1
p = "%s/%d" % (d, e)
while os.path.exists(p):
e += 1
p = "%s/%d" % (d, e)
f = open(p, "w")
f.write(commit)
f.close()
def make_refs(count, commit):
while count > 0:
sys.stdout.write("left: %d%s\r" % (count, " " * 30))
a = random.randrange(10, 30)
b = random.randrange(10000, 50000)
new_ref(a, b, commit)
count -= 1
print "refs complete"
def main():
if len(sys.argv) != 3:
die("usage: %s <name> <ref count>" % sys.argv[0])
_, name, refs = sys.argv
os.mkdir(name)
os.chdir(name)
if subprocess.call(["git", "init"]) != 0:
die("failed to init repo")
f = open("foobar.txt", "w")
f.write("%s: %s refs\n" % (name, refs))
f.close()
if subprocess.call(["git", "add", "foobar.txt"]) != 0:
die("failed to add foobar.txt")
if subprocess.call(["git", "commit", "-m", "inital commit"]) != 0:
die("failed to create initial commit")
commit = subprocess.check_output(["git", "show-ref", "-s", "master"]).strip()
make_refs(int(refs), commit)
if __name__ == "__main__":
main()
^ permalink raw reply
* Re: [RFC/PATCHv2] git-p4: handle files with shell metacharacters
From: Pete Wyckoff @ 2011-09-27 13:03 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, vitor.hda
In-Reply-To: <1317112836-14135-1-git-send-email-luke@diamand.org>
luke@diamand.org wrote on Tue, 27 Sep 2011 09:40 +0100:
> Updated git-p4 changes incorporating Pete's comments.
>
> - p4CmdList's stdin argument can now be a list.
I think this fits in with the rest of the patch and can stay.
> - Getting rid of the string option entirely is very hard; there are
> places where currently git-p4 creates a pipeline.
Yeah, thanks for checking though. Best to leave it consistent
like you did.
> - I wonder if verbose should actually be enabled for all the test
> cases?
It is way too verbose, even for test, but I see the argument.
One easy place to change it would be in the definition in
t/lib-git-p4.sh. You could do this by hand when testing the
tests perhaps.
> - The $ENV{PWD} is needed now because the shell used to set that; now
> that the shell isn't in use git-p4 has to set it.
>
> Pete - I wasn't sure whether you were saying I should rework
> my patch against next (and you would then rework your series) or
> something else. That sounds complicated though - let me know!
If you don't mind, I'll just queue it up with the utf16 and
test-refactor stuff I have, and send it all to Junio post-1.7.7.
Here's how I plan to adjust your tests, given the feedback that
Junio gave earlier and from reading other tests in t/.
-- Pete
-----------8<------------------
From 6b4bd671df338210ffd0348358420f0feb6f35c0 Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <pw@padd.com>
Date: Tue, 27 Sep 2011 08:53:25 -0400
Subject: [PATCH] git-p4 t9803: align syntax with other tests
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9803-git-shell-metachars.sh | 30 ++++++++++++------------------
1 files changed, 12 insertions(+), 18 deletions(-)
diff --git a/t/t9803-git-shell-metachars.sh b/t/t9803-git-shell-metachars.sh
index c166603..6cf4298 100755
--- a/t/t9803-git-shell-metachars.sh
+++ b/t/t9803-git-shell-metachars.sh
@@ -5,9 +5,7 @@ test_description='git-p4 transparency to shell metachars in filenames'
. ./lib-git-p4.sh
test_expect_success 'start p4d' '
- kill_p4d || : &&
- start_p4d &&
- cd "$TRASH_DIRECTORY"
+ start_p4d
'
test_expect_success 'init depot' '
@@ -30,25 +28,18 @@ test_expect_success 'shell metachars in filenames' '
echo f2 >"file with spaces" &&
git add "file with spaces" &&
P4EDITOR=touch git commit -m "add files" &&
- "$GITP4" submit --verbose &&
+ "$GITP4" submit
+ ) &&
+ (
cd "$cli" &&
p4 sync ... &&
- ls -l "file with spaces" &&
- ls -l "foo\$bar"
+ test -e "file with spaces" &&
+ test -e "foo\$bar"
)
'
-check_missing () {
- for i in $*; do
- if [ -f $i ]; then
- echo $i found but should be missing 1>&2
- exit 1
- fi
- done
-}
-
test_expect_success 'deleting with shell metachars' '
- "$GITP4" clone --dest="$git" --verbose //depot &&
+ "$GITP4" clone --dest="$git" //depot &&
test_when_finished cleanup_git &&
(
cd "$git" &&
@@ -56,10 +47,13 @@ test_expect_success 'deleting with shell metachars' '
git rm foo\$bar &&
git rm file\ with\ spaces &&
P4EDITOR=touch git commit -m "remove files" &&
- "$GITP4" submit --verbose
+ "$GITP4" submit
+ ) &&
+ (
cd "$cli" &&
p4 sync ... &&
- check_missing "file with spaces" foo\$bar
+ test ! -e "file with spaces" &&
+ test ! -e foo\$bar
)
'
--
1.7.6.3
^ permalink raw reply related
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