[PATCH] l10n: de.po: translate one message
Translate one message came from git.pot update in 6eebb35 (l10n: git.pot: v2.4.0 round 2 (1 update)). Signed-off-by: Ralf Thielow ralf.thie...@gmail.com --- po/de.po | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/po/de.po b/po/de.po index 49f35fe..2feaec1 100644 --- a/po/de.po +++ b/po/de.po @@ -1164,10 +1164,9 @@ msgstr die Gegenseite unterstützt keinen signierten Versand (\--signed push\) #: send-pack.c:366 -#, fuzzy msgid the receiving end does not support --atomic push msgstr -die Gegenseite unterstützt keinen signierten Versand (\--signed push\) +die Gegenseite unterstützt keinen atomaren Versand (\--atomic push\) #: sequencer.c:172 builtin/merge.c:782 builtin/merge.c:893 builtin/merge.c:995 #: builtin/merge.c:1005 -- 2.4.0.rc1.212.g5b27f3f -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Freeing struct lock_file?
On 2015-04-04 02.24, David Turner wrote: On Fri, 2015-04-03 at 15:01 -0700, Junio C Hamano wrote: David Turner dtur...@twopensource.com writes: Why is it impossible to free struct lock_files? I understand that they become part of a linked list, and that there's an atexit handler that goes over that list. But couldn't we just remove them from the linked list and then free them? I suspect that the code is worried about getting a signal, while it is manipulating the linked list, and then cause the atexit handler to walk a list that is in a broken state. This is technically possible, but practically unlikely: aligned pointer-sized writes are atomic on very nearly every processor, and that is all that is required to remove an item from a linked list safely in this case (though not, of course, in the general multi-threaded case). But I can see why git wouldn't want to depend on that behavior. C11 has a way to do this safely, but AIUI, git doesn't want to move to C99 let alone C11. So I guess this will just have to remain the way it is. If you insist on using C11, may be. But if there is an implementation that is #ifdef'ed and only enabled for known to work processors and a no-op for the others, why not ? Do you have anything in special in mind ? A git diff may be a start for a patch series.. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git 2.3.4, ssh: Could not resolve hostname
On 2015-04-04 02.19, Reid Woodbury Jr. wrote:
Thanks for keeping me in the loop!
I have two thoughts on handling input:
As a coder I want to know exactly what's going on in my code. If I've given
erroneous input I'd like to know about it in the most useful and quickest
way, never glossed over, liberally accepted, nor fixed for me even if the
input is non-ambigous. I won't learn the right way unless I'm told. I enjoy
that when I've typo'd a command in GIT it gives useful suggestions to what I
might have meant.
But, most of the coding *I* do is for the non-coder or the general end user.
These might be people that would reasonably yell at their computer screen
you know what I meant! So I try to be more liberal in the input I write
code to accept by filtering it, cleaning it, etc. I'll even filter input by
keystroke when possible. I have the philosophy: don't tell the user that they
input something bad, just prevent them from inputting it to begin with. I
know, this is appropriate when building a GUI and not for CLI.
thanks for listening
Reid Woodbury
Thanks for the report.
(And please try to avoid top-posting to this list in the future ;-)
The basic fix will look like below, but I need to update the test-suite as well.
diff --git a/connect.c b/connect.c
index ce0e121..c8744f3 100644
--- a/connect.c
+++ b/connect.c
@@ -310,6 +310,8 @@ static void get_host_and_port(char **host, const char
**port)
if (end != colon + 1 *end == '\0' 0 = portnr portnr
65536) {
*colon = 0;
*port = colon + 1;
+ } else if (!colon[1]) {
+ *colon = 0;
}
}
}
@@ -385,7 +387,7 @@ static int git_tcp_connect_sock(char *host, int flags)
freeaddrinfo(ai0);
if (sockfd 0)
- die(unable to connect to %s:\n%s, host, error_message.buf);
+ die(unable to connect to '%s' :\n%s, host, error_message.buf);
enable_keepalive(sockfd);
On Apr 3, 2015, at 2:32 PM, Kyle J. McKay mack...@gmail.com wrote:
On Apr 2, 2015, at 17:02, Torsten Bögershausen wrote:
On 2015-04-02 21.35, Jeff King wrote:
On Thu, Apr 02, 2015 at 12:31:14PM -0700, Reid Woodbury Jr. wrote:
Ah, understand. Here's my project URL for 'remote origin' with a
more meaningful representation of their internal FQDN:
url = ssh://rwoodbury@systemname.groupname.online:/opt/git/inventory.git
The online is their literal internal TLD.
Thanks. The problem is the extra : after online; your URL is
malformed. You can just drop that colon entirely.
I do not think we need to support this syntax going forward (the colon
is meaningless here, and our documentation is clear that it should go
with a port number), but on the other hand, it might be nice to be more
liberal, as we were in v2.3.3 and prior. I'll leave it to Torsten to see
whether supporting that would hurt some of the other cases, or whether
it would make the code too awkward.
-Peff
Thanks for digging.
This makes my think that it is
a) non-standard to have the extra colon
It's not. See RFC 3986 appendix A:
authority = [ userinfo @ ] host [ : port ]
port = *DIGIT
*DIGIT means (see RFC 2234 section 3.6) zero or more digits.
b) The error message could be better
c) We don't have a test case
d) This reminds my of an improvement from Linus:
608d48b2207a61528
..
So when somebody passes me a please pull request pointing to something
like the following
git://git.kernel.org:/pub/scm/linux/kernel/git/mchehab/v4l-dvb.git
(note the extraneous colon at the end of the host name), git would happily
try to connect to port 0, which would generally just cause the remote to
not even answer, and the connect() will take a long time to time out.
.
Sorry guys for the regression, the old parser handled the extra colon as
port 0,
the new one looks for the / as the end of the hostname (and the beginning
of the path)
Either we accept the extra colon as before, or the parser puts out a better
error message,
[...]
Spontaneously I would say that a trailing ':' at the end of a hostname in
the ssh:// scheme
can be safely ignored, what do you think ?
You know, there is a url_normalize routine in urlmatch.h/urlmatch.c that
checks for a lot of these things and provides a translated error message if
there's a problem as well as normalizing and separating out the various
parts of the URL. It does not currently handle default ports for anything
other than http[s] but it would be simple enough to add support for ssh,
git, ftp[s] and rsync default ports too.
-Kyle
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to
[PATCH v2 3/3] git-p4: fix filetype detection on files opened exclusively
From: Holloway, Blair blair_hollo...@playstation.sony.com If a Perforce server is configured to automatically set +l (exclusive lock) on add of certain file types, git p4 submit will fail during getP4OpenedType, as the regex doesn't expect the trailing '*exclusive*' from p4 opened: //depot/file.png#1 - add default change (binary+l) *exclusive* Signed-off-by: Blair Holloway blair_hollo...@playstation.sony.com Acked-by: Luke Diamand l...@diamand.org Signed-off-by: Luke Diamand l...@diamand.org --- git-p4.py| 2 +- t/t9816-git-p4-locked.sh | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/git-p4.py b/git-p4.py index ff132b2..d43482a 100755 --- a/git-p4.py +++ b/git-p4.py @@ -368,7 +368,7 @@ def getP4OpenedType(file): # Returns the perforce file type for the given file. result = p4_read_pipe([opened, wildcard_encode(file)]) -match = re.match(.*\((.+)\)\r?$, result) +match = re.match(.*\((.+)\)( \*exclusive\*)?\r?$, result) if match: return match.group(1) else: diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh index 464f10b..d048bd3 100755 --- a/t/t9816-git-p4-locked.sh +++ b/t/t9816-git-p4-locked.sh @@ -35,7 +35,7 @@ test_expect_success 'edit with lock not taken' ' ) ' -test_expect_failure 'add with lock not taken' ' +test_expect_success 'add with lock not taken' ' test_when_finished cleanup_git git p4 clone --dest=$git //depot ( @@ -107,7 +107,7 @@ test_expect_failure 'chmod with lock taken' ' ) ' -test_expect_failure 'copy with lock taken' ' +test_expect_success 'copy with lock taken' ' lock_in_another_client test_when_finished cleanup_git test_when_finished cd \$cli\ p4 revert file2 rm -f file2 -- 2.3.4.48.g223ab37 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] git-p4: small fix for locked-file-move-test
The test for handling of failure when trying to move a file that is locked by another client was not quite correct - it failed early on because the target file in the move already existed. The test now fails because git-p4 does not properly detect that p4 has rejected the move, and instead just crashes. At present, git-p4 has no support for detecting that a file has been locked and reporting it to the user, so this is the expected outcome. Signed-off-by: Luke Diamand l...@diamand.org --- t/t9816-git-p4-locked.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh index ce0eb22..464f10b 100755 --- a/t/t9816-git-p4-locked.sh +++ b/t/t9816-git-p4-locked.sh @@ -130,8 +130,8 @@ test_expect_failure 'move with lock taken' ' git p4 clone --dest=$git //depot ( cd $git - git mv file1 file2 - git commit -m mv file1 to file2 + git mv file1 file3 + git commit -m mv file1 to file3 git config git-p4.skipSubmitEdit true git config git-p4.detectRenames true git p4 submit --verbose -- 2.3.4.48.g223ab37 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] git-p4: fix small bug in locked test scripts
Test script t9816-git-p4-locked.sh test #4 tests for adding a file that is locked by Perforce automatically. This is currently not supported by git-p4 and so is expected to fail. However, a small typo meant it always failed, even with a fixed git-p4. Fix the typo to resolve this. Signed-off-by: Luke Diamand l...@diamand.org --- t/t9816-git-p4-locked.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh index e71e543..ce0eb22 100755 --- a/t/t9816-git-p4-locked.sh +++ b/t/t9816-git-p4-locked.sh @@ -41,7 +41,7 @@ test_expect_failure 'add with lock not taken' ' ( cd $git echo line1 add-lock-not-taken - git add file2 + git add add-lock-not-taken git commit -m add add-lock-not-taken git config git-p4.skipSubmitEdit true git p4 submit --verbose -- 2.3.4.48.g223ab37 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] git-p4: updated locked file handling patch series
Updated patch series for fixing git-p4 filetype detection when one or more files have been locked automatically by p4 (fix provided by Blair), incorporating comments from Eric: - squashes the actual fix and the test case change together - fixes typo Luke Holloway, Blair (1): git-p4: fix filetype detection on files opened exclusively Luke Diamand (2): git-p4: fix small bug in locked test scripts git-p4: small fix for locked-file-move-test git-p4.py| 2 +- t/t9816-git-p4-locked.sh | 10 +- 2 files changed, 6 insertions(+), 6 deletions(-) -- 2.3.4.48.g223ab37 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff-highlight: Fix broken multibyte string
On Fri, Apr 03, 2015 at 03:24:09PM -0700, Kyle J. McKay wrote: I thought that meant we could also optimize out the map call entirely, and just use the first split (with *) to end up with a list of $COLOR chunks and single characters, but it does not seem to work. So maybe I am misreading something about what is going on. I think our emails crossed in flight... Using just the first split (with *) produces useless empty elements which I think ends up causing problems. I suppose you could surround it with a grep /./ to remove them but that would defeat the point of the optimization. Yeah, the problem is the use of (). We want to keep the $COLOR delimiters, but not the empty ones. Perhaps you could do: split /($COLOR+)|/ but I didn't try it. I think what you posted is good and a lot less subtle. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] diff-highlight: do not split multibyte characters
On Fri, Apr 3, 2015 at 10:24 AM, Jeff King p...@peff.net wrote: EungJun, does this version meet your needs? -Peff Yes, this patch is enough to meet my needs because it works well on UTF-8, the only encoding I use. And this patch looks better than my one because it is smaller, doesn't depend on String::Multibyte and seems to have no side-effect. I hope someone who use another multibyte encoding will send a patch to support the encoding in future... :) On Sat, Apr 4, 2015 at 11:09 PM, Jeff King p...@peff.net wrote: On Fri, Apr 03, 2015 at 03:15:14PM -0700, Kyle J. McKay wrote: When the input is UTF-8 and Perl is operating on bytes instead of characters, a diff that changes one multibyte character to another that shares an initial byte sequence will result in a broken diff display as the common byte sequence prefix will be separated from the rest of the bytes in the multibyte character. For example, if a single line contains only the unicode character U+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that line is then changed to the unicode character U+C9C0 (encoded as UTF-8 0xEC, 0xA7, 0x80), when operating on bytes diff-highlight will show only the single byte change from 0x84 to 0x80 thus creating invalid UTF-8 and a broken diff display. Fix this by putting Perl into character mode when splitting the line and then back into byte mode after the split is finished. The utf8::xxx functions require Perl 5.8 so we require that as well. Also, since we are mucking with code in the split_line function, we change a '*' quantifier to a '+' quantifier when matching the $COLOR expression which has the side effect of speeding everything up while eliminating useless '' elements in the returned array. Reported-by: Yi EungJun semtlen...@gmail.com Signed-off-by: Kyle J. McKay mack...@gmail.com This version looks good to me. I looked over the diff of running git log -p --color on git.git through diff-highlight before and after this patch, and everything looks like an improvement. Acked-by: Jeff King p...@peff.net Thanks both of you for working on this. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/6] address packed-refs speed regressions
As I've mentioned before, I have some repositories with rather large numbers of refs. The worst one has ~13 million refs, for a 1.6GB packed-refs file. So I was saddened by this: $ time git.v2.0.0 rev-parse refs/heads/foo /dev/null 21 real0m6.840s user0m6.404s sys 0m0.440s $ time git.v2.4.0-rc1 rev-parse refs/heads/foo /dev/null 21 real0m19.432s user0m18.996s sys 0m0.456s The command isn't important; what I'm really measuring is loading the packed-refs file. And yes, of course this repository is absolutely ridiculous. But the slowdowns here are linear with the number of refs. So _every_ git command got a little bit slower, even in less crazy repositories. We just didn't notice it as much. Here are the numbers after this series: real0m8.539s user0m8.052s sys 0m0.496s Much better, but I'm frustrated that they are still 20% slower than the original. The main culprits seem to be d0f810f (which introduced some extra expensive code for each ref) and my 10c497a, which switched from fgets() to strbuf_getwholeline. It turns out that strbuf_getwholeline is really slow. There may be other problems lurking to account for the remaining 20%. It's hard to find performance regressions with a bisection if there are multiple of them; if you stop at a random commit and it is 500ms slow, it is hard to tell which problem is causing it. Note that while these are regressions, they are in v2.2.0 and v2.2.2 respectively. So this can wait until post-2.4. [1/6]: strbuf_getwholeline: use getc macro [2/6]: git-compat-util: add fallbacks for unlocked stdio [3/6]: strbuf_getwholeline: use get_unlocked [4/6]: strbuf: add an optimized 1-character strbuf_grow [5/6]: t1430: add another refs-escape test [6/6]: refname_is_safe: avoid expensive normalize_path_copy call -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/6] t1430: add another refs-escape test
In t1430, we check whether deleting the branch ../../foo will delete .git/foo. However, this is not that interesting a test; the precious file .git/foo does not look like a ref, so even if we did not notice the escape from the refs/ hierarchy, we would fail for that reason (i.e., if you turned refname_is_safe into a noop, the test still passes). Let's add an additional test for the same thing, but with a file that actually looks like a ref. That will make sure we are exercising the refname_is_safe code. While we're at it, let's also make the code work a little harder by adding some extra paths and some empty path components. Signed-off-by: Jeff King p...@peff.net --- t/t1430-bad-ref-name.sh | 8 1 file changed, 8 insertions(+) diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh index 468e856..16d0b8b 100755 --- a/t/t1430-bad-ref-name.sh +++ b/t/t1430-bad-ref-name.sh @@ -68,6 +68,14 @@ test_expect_success 'branch -D cannot delete non-ref in .git dir' ' test_cmp expect .git/my-private-file ' +test_expect_success 'branch -D cannot delete ref in .git dir' ' + git rev-parse HEAD .git/my-private-file + git rev-parse HEAD expect + git branch foo/legit + test_must_fail git branch -D foo./././../../../my-private-file + test_cmp expect .git/my-private-file +' + test_expect_success 'branch -D cannot delete absolute path' ' git branch -f extra test_must_fail git branch -D $(pwd)/.git/refs/heads/extra -- 2.4.0.rc0.363.gf9f328b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] strbuf_getwholeline: use getc_unlocked
strbuf_getwholeline calls getc in a tight loop. On modern
libc implementations, the stdio code locks the handle for
every operation, which means we are paying a significant
overhead. We can get around this by locking the handle for
the whole loop and using the unlocked variant.
Running git rev-parse refs/heads/does-not-exist on a repo
with an extremely large (1.6GB) packed-refs file went from:
real0m18.900s
user0m18.472s
sys 0m0.448s
to:
real0m10.953s
user0m10.384s
sys 0m0.580s
for a wall-clock speedup of 42%. All times are best-of-3,
and done on a glibc 2.19 system.
Note that we call into strbuf_grow while holding the lock.
It's possible for that function to call other stdio
functions (e.g., printing to stderr when dying due to malloc
error); however, the POSIX.1-2001 definition of flockfile
makes it clear that the locks are per-handle, so we are fine
unless somebody else tries to read from our same handle.
This doesn't ever happen in the current code, and is
unlikely to be added in the future (we would have to do
something exotic like add a die_routine that tried to read
from stdin).
Signed-off-by: Jeff King p...@peff.net
---
I don't think the complexity is worth it, but if we wanted to be more
careful about the locks, I think it would probably involve growing the
buffer, locking, doing unlocked reads until it's full, and then
unlocking for the next round of growth.
I also considered optimizing the term == '\n' case by using fgets, but
it gets rather complex (you have to pick a size, fgets into it, and then
keep going if you didn't get a newline). Also, fgets sucks, because you
have to call strlen() immediately after to find out how many bytes you
got!
strbuf.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/strbuf.c b/strbuf.c
index 14f337d..af2bad4 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -443,12 +443,14 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
term)
return EOF;
strbuf_reset(sb);
- while ((ch = getc(fp)) != EOF) {
+ flockfile(fp);
+ while ((ch = getc_unlocked(fp)) != EOF) {
strbuf_grow(sb, 1);
sb-buf[sb-len++] = ch;
if (ch == term)
break;
}
+ funlockfile(fp);
if (ch == EOF sb-len == 0)
return EOF;
--
2.4.0.rc0.363.gf9f328b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/6] strbuf: add an optimized 1-character strbuf_grow
We have to call strbuf_grow anytime we are going to add data
to a strbuf. In most cases, it's a noop (since we grow the
buffer aggressively), and the cost of the function call and
size check is dwarfed by the actual buffer operation.
For a tight loop of single-character additions, though, this
overhead is noticeable. Furthermore, the single-character
case is much easier to check; since the extra parameter is
1, we can do it without worrying about overflow.
This patch adds a simple inline function for checking
single-character growth. For the growth case, it just calls
into the regular strbuf_grow(). This is redundant, as
strbuf_grow will check again whether we need to grow. But it
keeps our inline code simple, and most calls will not need
to grow, so it's OK to treat this as a rare slow path.
We apply the new function to strbuf_getwholeline. Running
git rev-parse refs/heads/does-not-exist on a repo with an
extremely large (1.6GB) packed-refs file went from
(best-of-3):
real0m10.953s
user0m10.384s
sys 0m0.580s
to:
real0m8.910s
user0m8.452s
sys 0m0.468s
for a wall-clock speedup of 18%.
Signed-off-by: Jeff King p...@peff.net
---
strbuf.c | 2 +-
strbuf.h | 9 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/strbuf.c b/strbuf.c
index af2bad4..2facd5f 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -445,7 +445,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
term)
strbuf_reset(sb);
flockfile(fp);
while ((ch = getc_unlocked(fp)) != EOF) {
- strbuf_grow(sb, 1);
+ strbuf_grow_ch(sb);
sb-buf[sb-len++] = ch;
if (ch == term)
break;
diff --git a/strbuf.h b/strbuf.h
index 1883494..ef41151 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -137,6 +137,15 @@ static inline size_t strbuf_avail(const struct strbuf *sb)
*/
extern void strbuf_grow(struct strbuf *, size_t);
+/*
+ * An optimized version of strbuf_grow() for a single character.
+ */
+static inline void strbuf_grow_ch(struct strbuf *sb)
+{
+ if (!sb-alloc || sb-alloc - 1 = sb-len)
+ strbuf_grow(sb, 1);
+}
+
/**
* Set the length of the buffer to a given value. This function does *not*
* allocate new memory, so you should not perform a `strbuf_setlen()` to a
--
2.4.0.rc0.363.gf9f328b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] strbuf_getwholeline: use getc macro
strbuf_getwholeline calls fgetc in a tight loop. Using the
getc form, which can be implemented as a macro, should be
faster (and we do not care about it evaluating our argument
twice, as we just have a plain variable).
On my glibc system, running git rev-parse
refs/heads/does-not-exist on a file with an extremely large
(1.6GB) packed-refs file went from (best of 3 runs):
real0m19.383s
user0m18.876s
sys 0m0.528s
to:
real0m18.900s
user0m18.472s
sys 0m0.448s
for a wall-clock speedup of 2.5%.
Signed-off-by: Jeff King p...@peff.net
---
Not that exciting a speedup. But later we will switch to getc_unlocked,
and I wanted to measure how much of that was coming from the macro, and
how much from the locking.
strbuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/strbuf.c b/strbuf.c
index 88cafd4..14f337d 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -443,7 +443,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
term)
return EOF;
strbuf_reset(sb);
- while ((ch = fgetc(fp)) != EOF) {
+ while ((ch = getc(fp)) != EOF) {
strbuf_grow(sb, 1);
sb-buf[sb-len++] = ch;
if (ch == term)
--
2.4.0.rc0.363.gf9f328b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/6] git-compat-util: add fallbacks for unlocked stdio
POSIX.1-2001 specifies some functions for optimizing the locking out of tight getc() loops. Not all systems are POSIX, though, and even not all POSIX systems are required to implement these functions. We can check for the feature-test macro to see if they are available, and if not, provide a noop implementation. There's no Makefile knob here, because we should just detect this automatically. If there are very bizarre systems, we may need to add one, but it's not clear yet in which direction: 1. If a system defines _POSIX_THREAD_SAFE_FUNCTIONS but these functions are missing or broken, we would want a knob to manually turn them off. 2. If a system has these functions but does not define _POSIX_THREAD_SAFE_FUNCTIONS, we would want a knob to manually turn them on. We can add such a knob when we find a real-world system that matches this. Signed-off-by: Jeff King p...@peff.net --- git-compat-util.h | 6 ++ 1 file changed, 6 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index bc8fc8c..685a0a4 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -883,4 +883,10 @@ struct tm *git_gmtime_r(const time_t *, struct tm *); # define SHELL_PATH /bin/sh #endif +#ifndef _POSIX_THREAD_SAFE_FUNCTIONS +#define flockfile(fh) +#define funlockfile(fh) +#define getc_unlocked(fh) getc(fh) +#endif + #endif -- 2.4.0.rc0.363.gf9f328b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/6] refname_is_safe: avoid expensive normalize_path_copy call
Usually refs are not allowed to contain a .. component.
However, since d0f810f (refs.c: allow listing and deleting
badly named refs, 2014-09-03), we relax these rules in some
cases in order to help users examine and get rid of
badly-named refs. However, we do still check that these refs
cannot escape the refs hierarchy (e.g., refs/../foo).
This check is implemented by calling normalize_path_copy,
which requires us to allocate a new buffer to hold the
result. But we don't care about the result; we only care
whether the up components outnumbered the down.
We can therefore implement this check ourselves without
requiring any extra allocations. With this patch, running
git rev-parse refs/heads/does-not-exist on a repo with
large (1.6GB) packed-refs file went from:
real0m8.910s
user0m8.452s
sys 0m0.468s
to:
real0m8.529s
user0m8.044s
sys 0m0.492s
for a wall-clock speedup of 4%.
Signed-off-by: Jeff King p...@peff.net
---
This was a lot less than I was hoping for, especially considering that
going from d0f810f^ to d0f810f is more like a 15% slowdown (or in
absolute numbers, ~1.1s versus only 400ms here). What's doubly confusing
is that I think we were running check_ref_format before d0f810f, which
does way more than the check we're doing here. So we should have ended
up faster than either.
So this is certainly _an_ improvement, but I think there may be more
going on.
cache.h | 7 +++
path.c | 64
refs.c | 16 ++--
3 files changed, 73 insertions(+), 14 deletions(-)
diff --git a/cache.h b/cache.h
index 3d3244b..c2b3df4 100644
--- a/cache.h
+++ b/cache.h
@@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const char
*suffix);
int daemon_avoid_alias(const char *path);
extern int is_ntfs_dotgit(const char *name);
+/**
+ * Returns 1 if the path escapes its root (e.g., foo/../../bar) and 0
+ * otherwise. Note that this is purely textual, and does not care about
+ * symlinks or other aspects of the filesystem.
+ */
+int check_path_escape(const char *path);
+
/* object replacement */
#define LOOKUP_REPLACE_OBJECT 1
extern void *read_sha1_file_extended(const unsigned char *sha1, enum
object_type *type, unsigned long *size, unsigned flag);
diff --git a/path.c b/path.c
index e608993..1127cbd 100644
--- a/path.c
+++ b/path.c
@@ -703,6 +703,70 @@ int normalize_path_copy(char *dst, const char *src)
}
/*
+ * We want to detect a path that escapes its root. The general strategy
+ * is to parse components left to right, keeping track of our depth,
+ * which is increased by non-empty components and decreased by ..
+ * components.
+ */
+int check_path_escape(const char *path)
+{
+ int depth = 0;
+
+ while (*path) {
+ char ch = *path++;
+
+ /*
+* We always start our loop at the beginning of a path
component. So
+* we can skip past any dir separators. This handles leading
+* /, as well as any internal .
+*/
+ if (is_dir_sep(ch))
+ continue;
+
+ /*
+* If we start with a dot, we care about the four cases
+* (similar to normalize_path_copy above):
+*
+* (1) . - does not affect depth; we are done
+* (2) ./ - does not affect depth; skip
+* (3) .. - check depth and finish
+* (4) ../ - drop depth, check, and keep looking
+*/
+ if (ch == '.') {
+ ch = *path++;
+
+ if (!ch)
+ return 1; /* case (1) */
+ if (is_dir_sep(ch))
+ continue; /* case (2) */
+ if (ch == '.') {
+ ch = *path++;
+ if (!ch)
+ return depth 0; /* case (3) */
+ if (is_dir_sep(ch)) {
+ /* case (4) */
+ if (--depth 0)
+ return 0;
+ continue;
+ }
+ /* otherwise, ..foo; fall through */
+ }
+ /* otherwise .foo; fall through */
+ }
+
+ /*
+* We have a real component; inrement the depth and eat the
+* rest of the component
+*/
+ depth++;
+ while (*path !is_dir_sep(*path))
+ path++;
+ }
+
+ return 1;
+}
+
+/*
* path = Canonical absolute path
* prefixes = string_list containing normalized, absolute paths without
* trailing slashes (except for the root
Re: [PATCH 3/6] strbuf_getwholeline: use getc_unlocked
On Sat, Apr 04, 2015 at 09:11:10PM -0400, Jeff King wrote:
I also considered optimizing the term == '\n' case by using fgets, but
it gets rather complex (you have to pick a size, fgets into it, and then
keep going if you didn't get a newline). Also, fgets sucks, because you
have to call strlen() immediately after to find out how many bytes you
got!
My initial attempt at this had been to _just_ use fgets, but the
optimization becomes much simpler if you just do an initial fgets, and
then follow up with character reads. In most cases, the initial fgets
is big enough to get the whole line.
I.e., doing:
diff --git a/strbuf.c b/strbuf.c
index 2facd5f..f319d8d 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -443,6 +443,18 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
term)
return EOF;
strbuf_reset(sb);
+
+ if (term == '\n') {
+ strbuf_grow(sb, 256);
+ if (!fgets(sb-buf, sb-alloc - 1, fp)) {
+ strbuf_release(sb);
+ return EOF;
+ }
+ sb-len = strlen(sb-buf);
+ if (sb-buf[sb-len - 1] == '\n')
+ return 0;
+ }
+
flockfile(fp);
while ((ch = getc_unlocked(fp)) != EOF) {
strbuf_grow_ch(sb);
on top of the series drops me from:
real0m8.573s
user0m8.072s
sys 0m0.508s
to:
real0m6.671s
user0m6.216s
sys 0m0.460s
which is back to the v2.0.0 number. Even with the extra strlen, it seems
that what fgets does internally beats repeated getc calls. Which I guess
is not too surprising, as each getc() will have to check for underflow
in the buffer. Perhaps there is more room to micro-optimize
strbuf_getwholeline, but I kind of doubt it.
The big downside is that our input strings are no longer NUL-clean
(reading foo\0bar\n would yield just foo. I doubt that matters in
the real world, but it does fail a few of the tests (e.g., t7008 tries
to read a list of patterns which includes NUL, and we silently truncate
the pattern rather than read in the NUL and barf).
So we'd have to either:
1. Decide that doesn't matter.
2. Have callers specify a damn the NULs, I want it fast flag.
3. Find some alternative that is more robust than fgets, and faster
than getc. I don't think there is anything in stdio, but I am not
above dropping in a faster non-portable call if it is available,
and then falling back to the current code otherwise.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] strbuf_getwholeline: use getc_unlocked
On Sun, Apr 05, 2015 at 12:56:14AM -0400, Jeff King wrote:
The big downside is that our input strings are no longer NUL-clean
(reading foo\0bar\n would yield just foo. I doubt that matters in
the real world, but it does fail a few of the tests (e.g., t7008 tries
to read a list of patterns which includes NUL, and we silently truncate
the pattern rather than read in the NUL and barf).
So there is this trick:
diff --git a/strbuf.c b/strbuf.c
index f319d8d..5ceebb7 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -445,12 +445,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
term)
strbuf_reset(sb);
if (term == '\n') {
+ long pos = ftell(fp);
strbuf_grow(sb, 256);
if (!fgets(sb-buf, sb-alloc - 1, fp)) {
strbuf_release(sb);
return EOF;
}
- sb-len = strlen(sb-buf);
+ sb-len = ftell(fp) - pos;
if (sb-buf[sb-len - 1] == '\n')
return 0;
}
but much to my surprise it actually runs slower than the strlen version!
It also has a 32-bit overflow issue. There's fgetpos() as an
alternative, but fpos_t is an opaque type, and we might not be able to
do arithmetic on it (for that matter, I am not sure if arithmetic is
strictly guaranteed on ftell() results). POSIX gives us ftello(), which
returns an off_t. That would probably be fine.
The ftello() version seems slower than the strlen, but faster than
ftell(). Puzzling.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] strbuf_getwholeline: use getc_unlocked
On Sun, Apr 05, 2015 at 01:27:32AM -0400, Jeff King wrote:
On Sun, Apr 05, 2015 at 12:56:14AM -0400, Jeff King wrote:
The big downside is that our input strings are no longer NUL-clean
(reading foo\0bar\n would yield just foo. I doubt that matters in
the real world, but it does fail a few of the tests (e.g., t7008 tries
to read a list of patterns which includes NUL, and we silently truncate
the pattern rather than read in the NUL and barf).
So there is this trick:
diff --git a/strbuf.c b/strbuf.c
index f319d8d..5ceebb7 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -445,12 +445,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp,
int term)
strbuf_reset(sb);
if (term == '\n') {
+ long pos = ftell(fp);
strbuf_grow(sb, 256);
if (!fgets(sb-buf, sb-alloc - 1, fp)) {
strbuf_release(sb);
return EOF;
}
- sb-len = strlen(sb-buf);
+ sb-len = ftell(fp) - pos;
if (sb-buf[sb-len - 1] == '\n')
return 0;
}
but much to my surprise it actually runs slower than the strlen version!
It also has a 32-bit overflow issue. There's fgetpos() as an
alternative, but fpos_t is an opaque type, and we might not be able to
do arithmetic on it (for that matter, I am not sure if arithmetic is
strictly guaranteed on ftell() results). POSIX gives us ftello(), which
returns an off_t. That would probably be fine.
Actually, scratch that idea. ftell() always returns 0 on a non-seekable
file, so we can't use it in the general case. And that probably explains
the performance difference, too, if it is not keeping its own counter
and relies on lseek(fileno(fp)) or similar.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
git clean performance issues
Hi,
I'm having a performance issue with git clean -qxfd (note, not using
-ff).
The performance issue shows up when trying to clean untracked
directories that themselves contain many sub directories. The
performance is highly non linear with the number of sub
directories. Some test numbers:
DirsTime
1 0m0.754s
5 0m16.606s
10 1m31.418s
When running git clean -qxffd (note, using -ff) the performance is
fast and linear:
DirsTime
1 0m0.158s
5 0m0.918s
10 0m1.639s
After checking the source of git-clean my understanding of the problem
is as follows:
When clean.c:cmd_clean finds a directory and the -d flag is given it
will call clean.c:remove_dirs to potentially remove the directory and
all sub directories.
Unless -ff is given remove_dirs tries to be nice and not remove
directories containing other git repositories. To do this it does the
following check:
...
if ((force_flag REMOVE_DIR_KEEP_NESTED_GIT)
!resolve_gitlink_ref(path-buf, HEAD, submodule_head)) {
...
The problem is that refs.c:resolve_gitlink_ref will call
refs.c:get_ref_cache that will linearly search a linked list of cache
entries and create and insert a new ref_cache entry in the list for
each path it is given if it fails to find an existing entry:
for (refs = submodule_ref_caches; refs; refs = refs-next)
if (!strcmp(submodule, refs-name))
return refs;
refs = create_ref_cache(submodule);
refs-next = submodule_ref_caches;
submodule_ref_caches = refs;
return refs;
In my scenario get_ref_cache will be called 1+ times, each time
with a new path. The final few calls will need to search through and
compare 1+ entries before realizing that there is no existing
entry. This quickly ads up to 100 million+ calls to strcmp().
From what I can understand, the calls to get_ref_cache in this
scenario will never do any useful work. Is this correct? If so, would
it be possible to bypass it, maybe by calling
resolve_gitlink_ref_recursive directly or by using some other way of
checking for the presence of a git repo in clean.c:remove_dirs?
/Erik
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
C99 (Was: Re: Freeing struct lock_file?)
On Fri, Apr 03, 2015 at 08:24:43PM -0400, David Turner wrote: But I can see why git wouldn't want to depend on that behavior. C11 has a way to do this safely, but AIUI, git doesn't want to move to C99 let alone C11. So I guess this will just have to remain the way it is. I would really like to move to at least C99. However, MSVC bundles a C89 compiler and a C++ compiler, but Microsoft steadfastly refuses to bundle an actual C99 compiler, and Git doesn't even come close to compiling as C++. Newer versions of MSVC (2013 and newer) support enough of the features, though, that it might be possible. So it isn't as much of a we don't want to move to C99 as much as we aren't yet willing to drop support for older versions of MSVC. I don't use Windows, so I'm happy to drop support for MSVC 2012 and older, but others may have different opinions. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH] t9814: Guarantee only one source exists in git-p4 copy tests
Luke Diamand l...@diamand.org writes: On 30/03/15 04:03, Junio C Hamano wrote: Vitor Antunes vitor@gmail.com writes: * Modify source file (file2) before copying the file. * Check that only file2 is the source in the output of p4 filelog. * Remove all case statements and replace them simple tests to check that source is file2. Signed-off-by: Vitor Antunes vitor@gmail.com --- I am not a Perfoce user, so I'd like to ask Pete's and Luke's comments on these changes. It's much clearer now that the guessing of file source has been cleaned up, thanks. Ack. Thanks. Vitor, when resubmitting v2 to fix style nits, please add Luke's Acked-by just after your Sign-off, perhaps like this: t9814: guarantee only one source exists in git-p4 copy tests By using a tree with multiple identical files and allowing copy detection to choose any one of them, the check in the test is unnecessarily complex. We can simplify by: * Modify source file (file2) before copying the file. * Check that only file2 is the source in the output of p4 filelog. * Remove all case statements and replace them simple tests to check that source is file2. Signed-off-by: Vitor Antunes vitor@gmail.com Acked-by: Luke Diamand l...@diamand.org diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh index 8b9c295..d8fb22d 100755 --- a/t/t9814-git-p4-rename.sh +++ b/t/t9814-git-p4-rename.sh @@ -132,6 +132,9 @@ test_expect_success 'detect copies' ' cd $git git config git-p4.skipSubmitEdit true + echo file8 file2 Style: please lose SP between redirection and its target, i.e. echo file8 file2 The same comment applies to everywhere else. + git commit -a -m Differentiate file2 + git p4 submit cp file2 file8 git add file8 git commit -a -m Copy file2 to file8 @@ -140,6 +143,10 @@ test_expect_success 'detect copies' ' p4 filelog //depot/file8 p4 filelog //depot/file8 | test_must_fail grep -q branch from + echo file9 file2 + git commit -a -m Differentiate file2 + git p4 submit + cp file2 file9 git add file9 git commit -a -m Copy file2 to file9 @@ -149,28 +156,39 @@ test_expect_success 'detect copies' ' p4 filelog //depot/file9 p4 filelog //depot/file9 | test_must_fail grep -q branch from + echo file10 file2 + git commit -a -m Differentiate file2 + git p4 submit + echo file2 file2 cp file2 file10 git add file2 file10 git commit -a -m Modify and copy file2 to file10 git diff-tree -r -C HEAD + src=$(git diff-tree -r -C HEAD | sed 1d | sed 2d | cut -f2) + test $src = file2 git p4 submit p4 filelog //depot/file10 - p4 filelog //depot/file10 | grep -q branch from //depot/file + p4 filelog //depot/file10 | grep -q branch from //depot/file2 + + echo file11 file2 + git commit -a -m Differentiate file2 + git p4 submit cp file2 file11 git add file11 git commit -a -m Copy file2 to file11 git diff-tree -r -C --find-copies-harder HEAD src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) - case $src in - file2 | file10) : ;; # happy - *) false ;; # not - esac + test $src = file2 git config git-p4.detectCopiesHarder true git p4 submit p4 filelog //depot/file11 - p4 filelog //depot/file11 | grep -q branch from //depot/file + p4 filelog //depot/file11 | grep -q branch from //depot/file2 + + echo file12 file2 + git commit -a -m Differentiate file2 + git p4 submit cp file2 file12 echo some text file12 @@ -180,15 +198,16 @@ test_expect_success 'detect copies' ' level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d -f5 | sed s/C0*//) test -n $level test $level -gt 0 test $level -lt 98 src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) - case $src in - file10 | file11) : ;; # happy - *) false ;; # not - esac + test $src = file2 git config git-p4.detectCopies $(($level + 2)) git p4 submit p4 filelog //depot/file12 p4 filelog //depot/file12 | test_must_fail grep -q branch from + echo file13 file2 + git commit -a -m Differentiate file2 + git p4 submit +
Re: [PATCH v2 2/5] log: honor log.merges= option
Koosha Khajehmoogahi koo...@posteo.de writes:
From: Junio C Hamano gits...@pobox.com
[kk: wrote commit message]
Ehh, what exactly did you write ;-)?
I think the most important thing that needs to be explained by the
log message for this change is that the variable is honored only by
log and it needs to explain why other Porcelain commands in the same
log family, like whatchanged, should ignore the variable.
I think that we must not to allow format-patch and show to be
affected by this variable, because it is silly if log.merges=only
broke format-patch output or made git show silent. But I didn't
think about others. Whoever is doing this change needs to explain
in the log message the reason why it was decided that only git log
should pay attention to it.
Helped-by: Eris Sunshine sunsh...@sunshineco.com
Signed-off-by: Koosha Khajehmoogahi koo...@posteo.de
---
builtin/log.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..c7a7aad 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static int decoration_given;
static int use_mailmap_config;
static const char *fmt_patch_subject_prefix = PATCH;
static const char *fmt_pretty;
+static const char *log_merges;
The variable name may want to be updated to mimic other variables
used in a similar way, e.g. default_show_root is used to store the
value from log.showroot.
Perhaps default_show_merge or something?
Thanks.
static const char * const builtin_log_usage[] = {
N_(git log [options] [revision range] [[--] path...]),
@@ -386,6 +387,9 @@ static int git_log_config(const char *var, const char
*value, void *cb)
decoration_style = 0; /* maybe warn? */
return 0;
}
+ if (!strcmp(var, log.merges)) {
+ return git_config_string(log_merges, var, value);
+ }
if (!strcmp(var, log.showroot)) {
default_show_root = git_config_bool(var, value);
return 0;
@@ -628,6 +632,8 @@ int cmd_log(int argc, const char **argv, const char
*prefix)
init_revisions(rev, prefix);
rev.always_show_header = 1;
+ if (log_merges parse_merges_opt(rev, log_merges))
+ die(unknown config value for log.merges: %s, log_merges);
memset(opt, 0, sizeof(opt));
opt.def = HEAD;
opt.revarg_opt = REVARG_COMMITTISH;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: C99
On Sat, Apr 04, 2015 at 01:06:43PM -0700, Junio C Hamano wrote: brian m. carlson sand...@crustytoothpaste.net writes: So it isn't as much of a we don't want to move to C99 as much as we aren't yet willing to drop support for older versions of MSVC. I do not particularly like that 'we' in that sentence, which would give a false impression to people that we all want to switch and MSVC is the only thing that is holding us back. Okay, I should have clarified my statement. I appreciate the correction. Some people would like to move to C99, and so far the major objections I've heard have been that MSVC doesn't support it and that C99's benefits are unclear. I didn't meant to speak for others in that we should or should not, and there might be other objections I don't recall or haven't heard. I seem to recall Peff wanting to use variadic macros at some point, although I can't recall specifically where. We also already use hacks to implement some of the features and hope that compilers will DTRT. All the major compilers I'm aware of other than MSVC support C99, at least well enough to do the things we'd likely end up doing. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: git clean performance issues
On Sat, Apr 04, 2015 at 10:39:47PM +0200, erik elfström wrote: That looks like the same issue. The use is_git_directory approach sounds good to me, is that the direction you would prefer? I can try to cobble something together although I must warn you I have zero previous experience with this code base so a few iterations will probably be needed. Yeah, I think the preferred direction is building a solution in is_git_directory. Multiple iterations are fine. That's what review is for. :) See SubmittingPatches for tips, though. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git clean performance issues
On Sat, Apr 04, 2015 at 08:32:45PM +0200, erik elfström wrote: In my scenario get_ref_cache will be called 1+ times, each time with a new path. The final few calls will need to search through and compare 1+ entries before realizing that there is no existing entry. This quickly ads up to 100 million+ calls to strcmp(). From what I can understand, the calls to get_ref_cache in this scenario will never do any useful work. Is this correct? If so, would it be possible to bypass it, maybe by calling resolve_gitlink_ref_recursive directly or by using some other way of checking for the presence of a git repo in clean.c:remove_dirs? I think this is the same issue that was discussed here: http://thread.gmane.org/gmane.comp.version-control.git/265560/focus=265585 There is some discussion of a possible fix in that thread. I was hoping that Andreas was going to look further and produce a patch, but I imagine he got busy with other things. Do you want to try picking it up? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] sha1_file.c: support reading from a loose object of unknown type
On 04/05/2015 01:04 AM, Junio C Hamano wrote:
Karthik Nayak karthik@gmail.com writes:
@@ -2586,13 +2649,15 @@ int sha1_object_info_extended(const unsigned char
*sha1, struct object_info *oi,
*(oi-disk_sizep) = 0;
if (oi-delta_base_sha1)
hashclr(oi-delta_base_sha1);
+if (oi-typename)
+strbuf_addstr(oi-typename, typename(co-type));
oi-whence = OI_CACHED;
return 0;
}
Just before the pre-context of this hunk, there is this bit:
if (oi-typep)
*(oi-typep) = co-type;
which tells me that the callers of this function is allowed to pass
a NULL in oi-typep when they are not interested in the type of the
object.
if (!find_pack_entry(real, e)) {
/* Most likely it's a loose object. */
-if (!sha1_loose_object_info(real, oi)) {
+if (!sha1_loose_object_info(real, oi, flags)) {
oi-whence = OI_LOOSE;
return 0;
}
@@ -2616,6 +2681,8 @@ int sha1_object_info_extended(const unsigned char
*sha1, struct object_info *oi,
oi-u.packed.is_delta = (rtype == OBJ_REF_DELTA ||
rtype == OBJ_OFS_DELTA);
}
+if (oi-typename)
+strbuf_addstr(oi-typename, typename(*oi-typep));
So, it makes me wonder what guarantee we have that this does not
dereference a NULL here.
As per my code, oi-typename is only pointing to something when oi-typep
is ( As oi-typename is currently only used in cat-file.c).
But what you're saying also is true, there is no other guarantee, as a user may
set oi-typename to point to a struct strbuf and leave out oi-typep.
if (oi-typename oi-typep)
strbuf_addstr(oi-typename, typename(*oi-typep));
This should suffice. Do you want me to re-roll this?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git clean performance issues
That looks like the same issue. The use is_git_directory approach sounds good to me, is that the direction you would prefer? I can try to cobble something together although I must warn you I have zero previous experience with this code base so a few iterations will probably be needed. /Erik On Sat, Apr 4, 2015 at 9:55 PM, Jeff King p...@peff.net wrote: On Sat, Apr 04, 2015 at 08:32:45PM +0200, erik elfström wrote: In my scenario get_ref_cache will be called 1+ times, each time with a new path. The final few calls will need to search through and compare 1+ entries before realizing that there is no existing entry. This quickly ads up to 100 million+ calls to strcmp(). From what I can understand, the calls to get_ref_cache in this scenario will never do any useful work. Is this correct? If so, would it be possible to bypass it, maybe by calling resolve_gitlink_ref_recursive directly or by using some other way of checking for the presence of a git repo in clean.c:remove_dirs? I think this is the same issue that was discussed here: http://thread.gmane.org/gmane.comp.version-control.git/265560/focus=265585 There is some discussion of a possible fix in that thread. I was hoping that Andreas was going to look further and produce a patch, but I imagine he got busy with other things. Do you want to try picking it up? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] sha1_file.c: support reading from a loose object of unknown type
Karthik Nayak karthik@gmail.com writes:
@@ -2586,13 +2649,15 @@ int sha1_object_info_extended(const unsigned char
*sha1, struct object_info *oi,
*(oi-disk_sizep) = 0;
if (oi-delta_base_sha1)
hashclr(oi-delta_base_sha1);
+ if (oi-typename)
+ strbuf_addstr(oi-typename, typename(co-type));
oi-whence = OI_CACHED;
return 0;
}
Just before the pre-context of this hunk, there is this bit:
if (oi-typep)
*(oi-typep) = co-type;
which tells me that the callers of this function is allowed to pass
a NULL in oi-typep when they are not interested in the type of the
object.
if (!find_pack_entry(real, e)) {
/* Most likely it's a loose object. */
- if (!sha1_loose_object_info(real, oi)) {
+ if (!sha1_loose_object_info(real, oi, flags)) {
oi-whence = OI_LOOSE;
return 0;
}
@@ -2616,6 +2681,8 @@ int sha1_object_info_extended(const unsigned char
*sha1, struct object_info *oi,
oi-u.packed.is_delta = (rtype == OBJ_REF_DELTA ||
rtype == OBJ_OFS_DELTA);
}
+ if (oi-typename)
+ strbuf_addstr(oi-typename, typename(*oi-typep));
So, it makes me wonder what guarantee we have that this does not
dereference a NULL here.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: C99
brian m. carlson sand...@crustytoothpaste.net writes: So it isn't as much of a we don't want to move to C99 as much as we aren't yet willing to drop support for older versions of MSVC. I do not particularly like that 'we' in that sentence, which would give a false impression to people that we all want to switch and MSVC is the only thing that is holding us back. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
rev-list pretty format behavior
Heyup, everybody. Apologies if this turns out to be a duplicate. Gmane seems broken, so I couldn't search the archive. I'm using git version 2.4.0-rc1. The same behavior exists in 2.1.0. With git-log it is possible to specify a custom pretty format that outputs one line per commit: git log --pretty=format:%h ... HEAD~3...HEAD 826aed5 ... 915e44c ... 067178e ... Trying the same with rev-list results in: git rev-list --pretty=format:%h ... HEAD~3...HEAD commit 826aed50cbb072d8f159e4c8ba0f9bd3df21a234 826aed5 ... commit 915e44c6357f3bd9d5fa498a201872c4367302d3 915e44c ... commit 067178ed8a7822e6bc88ad606b707fc33658e6fc 067178e ... Is the separate line of commit hash a must for all formats except oneline or a possible bug? Based on the git-rev-list man page and git-log, I would expect to be able to override the format as described, since it is possible to get the commit hash line for any format by prefixing it with commit %H. The only way to get similar behaviour is to do something like: git rev-list ... | grep -v '^commit' and that's quite hacky. I looked at the code and the flow in rev-list seems odd to me. The header_prefix is set outside of show_commit(), it is empty for format=oneline, but set to commit for any other formats. It's then printed inside show_commit() and followed by the (possibly abbreviated) hash. So the display logic is split into two places, neither of which knows much about the other, both make decisions based on the pretty format specified. Even if the behavior is correct, would you agree that this could be refactored a bit, so the output is less stitched together? I'd happily try to help refactoring or fixing it, if it is indeed a bug. Thanks for your ears! Oliver -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
