[PATCH] archive-zip: fix compressed size for stored export-subst files
Currently ZIP archive entries of files with export-subst attribute are
broken if they are stored uncompressed.
We get the size of a file from sha1_object_info(), but this number is
likely wrong for files whose contents are changed due to export-subst
placeholder expansion. We use sha1_file_to_archive() to get the
expanded file contents and size in that case. We proceed to use that
size for the uncompressed size field (good), but the compressed size
field is set based on the size from sha1_object_info() (bad).
This matters only for uncompressed files because for deflated files
we use the correct value after compression is done. And for files
without export-subst expansion the sizes from sha1_object_info() and
sha1_file_to_archive() are the same, so they are unaffected as well.
This patch fixes the issue by setting the compressed size based on the
uncompressed size only after we actually know the latter.
Also make use of the test file substfile1 to check for the breakage;
it was only stored verbatim so far. For that purpose, set the
attribute export-subst and replace its contents with the expected
expansion after committing.
Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx
---
This bug was present even before 5ea2c847 (archive-zip: write
uncompressed size into header even with streaming).
archive-zip.c | 2 +-
t/t5003-archive-zip.sh | 6 ++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/archive-zip.c b/archive-zip.c
index d3aef53..a8d1193 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -240,7 +240,6 @@ static int write_zip_entry(struct archiver_args *args,
(mode 0111) ? ((mode) 16) : 0;
if (S_ISREG(mode) args-compression_level != 0 size 0)
method = 8;
- compressed_size = (method == 0) ? size : 0;
if (S_ISREG(mode) type == OBJ_BLOB !args-convert
size big_file_threshold) {
@@ -259,6 +258,7 @@ static int write_zip_entry(struct archiver_args *args,
crc = crc32(crc, buffer, size);
out = buffer;
}
+ compressed_size = (method == 0) ? size : 0;
} else {
return error(unsupported file mode: 0%o (SHA1: %s), mode,
sha1_to_hex(sha1));
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 7cfe9ca..6a33606 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -76,6 +76,12 @@ test_expect_success \
git update-ref HEAD $(TZ=GMT GIT_COMMITTER_DATE=2005-05-27 22:00:00 \
git commit-tree $treeid /dev/null)'
+test_expect_success 'setup export-subst' '
+ echo substfile? export-subst .git/info/attributes
+ git log --max-count=1 --pretty=format:A${SUBSTFORMAT}O HEAD \
+ a/substfile1
+'
+
test_expect_success \
'create bare clone' \
'git clone --bare . bare.git
--
1.8.0
--
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] attr: allow pattern escape using backslashes
On Sun, Feb 24, 2013 at 4:15 PM, Junio C Hamano gits...@pobox.com wrote: Speaking of .gitignore, I recall that there was a hanging discussion on allowing a pattern to name the directory that the .gitignore file appears in, which I do not think we currently support. With such a feature, instead of listing /junk in the .gitignore file at the top-level to say that everything inside the junk directory is ignored by default, we could instead say this at the beginning of the .gitignore file in the junk directory. Shouldn't / alone in junk/.gitignore express that? It does not work, but I think it's a natural interpretation of the syntax. I think * attr in the .gitattributes file in a directory causes git check-attr . in that directory to report the attr, and we may want to have some way to allow git check-ignore . to be affected by the .gitignore file in the same directory in a similar way, but I do not have a very strong opinion. Do people have any comment on this? -- Duy -- 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] contrib/subtree: fix Makefile to respect non-configure `make`
Hello There was a message in this list before to try to fix the Makefile: http://marc.info/?l=gitm=134116914230460w=2 However, without any reaction following. See also here whats the problem: https://bugs.archlinux.org/task/30473 Anyone there who can have a look at the patch and integrate it upstream? Regards Jan Rüegg -- 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: prevent pprint_rename from underrunning input
The logic described in d020e27 (diff: Fix rename pretty-print when suffix and prefix overlap, 2013-02-23) is wrong: The proof in the comment is valid only if both strings are the same length. *One* of old/new can reach a-1 (b-1, resp.) if 'a' is a suffix of 'b' (or vice versa). Indeed, sorry about that. I guess I was too focused on my specific issue and didn't broaden my thoughts. Thanks. I was also having hard time convincing myself why that -1 does not under-run yesterday. Will queue. Thanks -- 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: [PATCHv4 5/6] Git.pm: add interface for git credential command
Michal Nazarewicz m...@google.com writes:
+=item credential_read( FILEHANDLE )
+
+Reads credential key-value pairs from CFILEHANDLE. Reading stops at EOF or
+when an empty line is encountered. Each line must be of the form
Ckey=value
+with a non-empty key. Function returns hash with all read values. Any white
+space (other then new-line character) is preserved.
Typo: other then - than.
+sub credential_read {
+ my ($self, $reader) = _maybe_self(@_);
+ my %credential;
+ while ($reader) {
+ chomp;
+ if ($_ eq '') {
+ last;
+ } elsif (!/^([^=]+)=(.*)$/) {
Good.
+ # Check if $credential is valid prior to writing anything
+ while (($key, $value) = each %$credential) {
+ if (!defined $key || !length $key) {
+ throw Error::Simple(credential key empty or
undefined);
+ } elsif ($key =~ /[=\n\0]/) {
+ throw Error::Simple(credential key contains invalid
characters: $key);
+ } elsif (defined $value $value =~ /[\n\0]/) {
+ throw Error::Simple(credential value for key=$key
contains invalid characters: $value);
+ }
+ }
Good.
These checks seem to address all the points raised during discussion
about when the API should reject values.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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: post-receive-email hook with custom_showrev
On Wed, Feb 27, 2013 at 1:42 AM, Michael Haggerty mhag...@alum.mit.edu wrote: If I type that into my .git/config file then type git config hooks.showrev I get bad config file line 7 in .git/config due to the \$. I think the quoting in the comment in post-receive-email is written as if it would be passed via the command line to git config; i.e., git config hooks.showrev t=%s; printf 'http://.../?id=%%s' \$t; echo;echo; git show -C \$t; echo Granted, the docs don't make that clear. See git-config(1) for the details of the config file syntax. Thanks a lot that did the trick! I imagined it'd be some quoting issue. Cheers Adam -- 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] name-hash.c: fix endless loop with core.ignorecase=true
With core.ignorecase=true, name-hash.c builds a case insensitive index of
all tracked directories. Currently, the existing cache entry structures are
added multiple times to the same hashtable (with different name lengths and
hash codes). However, there's only one dir_next pointer, which gets
completely messed up in case of hash collisions. In the worst case, this
causes an endless loop if ce == ce-dir_next:
---8---
# V/, V/XQANY/ and WURZAUP/ all have the same hash_name
mkdir V
mkdir V/XQANY
mkdir WURZAUP
touch V/XQANY/test
git init
git config core.ignorecase true
git add .
git status
---8---
Use a separate hashtable and separate structures for the directory index
so that each directory entry has its own next pointer. Use reference
counting to track which directory entry contains files.
There are only slight changes to the name-hash.c API:
- new free_name_hash() used by read_cache.c::discard_index()
- remove_name_hash() takes an additional index_state parameter
- index_name_exists() for a directory (trailing '/') may return a cache
entry that has been removed (CE_UNHASHED). This is not a problem as the
return value is only used to check if the directory exists (dir.c) or to
normalize casing of directory names (read-cache.c).
Getting rid of cache_entry.dir_next reduces memory consumption, especially
with core.ignorecase=false (which doesn't use that member at all).
With core.ignorecase=true, building the directory index is slightly faster
as we add / check the parent directory first (instead of going through all
directory levels for each file in the index). E.g. with WebKit (~200k
files, ~7k dirs), time taken in lazy_init_name_hash is reduced from 176ms
to 130ms.
Signed-off-by: Karsten Blees bl...@dcon.de
---
Also available here:
https://github.com/kblees/git/tree/kb/name-hash-fix-endless-loop
git pull git://github.com/kblees/git.git kb/name-hash-fix-endless-loop
This combines the pros of the patches suggested by Jeff and me:
- reduced memory usage due to smaller dir_entry and cache_entry structs
- faster indexing due to right-to-left directory lookup
- marginal API changes, i.e. less impact on the rest of git
Test suite runs clean on msysgit and Linux.
Have fun,
Karsten
cache.h | 17 ++-
name-hash.c | 164 +++
read-cache.c | 9 ++--
3 files changed, 126 insertions(+), 64 deletions(-)
diff --git a/cache.h b/cache.h
index e493563..898e346 100644
--- a/cache.h
+++ b/cache.h
@@ -131,7 +131,6 @@ struct cache_entry {
unsigned int ce_namelen;
unsigned char sha1[20];
struct cache_entry *next;
- struct cache_entry *dir_next;
char name[FLEX_ARRAY]; /* more */
};
@@ -267,25 +266,15 @@ struct index_state {
unsigned name_hash_initialized : 1,
initialized : 1;
struct hash_table name_hash;
+ struct hash_table dir_hash;
};
extern struct index_state the_index;
/* Name hashing */
extern void add_name_hash(struct index_state *istate, struct cache_entry *ce);
-/*
- * We don't actually *remove* it, we can just mark it invalid so that
- * we won't find it in lookups.
- *
- * Not only would we have to search the lists (simple enough), but
- * we'd also have to rehash other hash buckets in case this makes the
- * hash bucket empty (common). So it's much better to just mark
- * it.
- */
-static inline void remove_name_hash(struct cache_entry *ce)
-{
- ce-ce_flags |= CE_UNHASHED;
-}
+extern void remove_name_hash(struct index_state *istate, struct cache_entry
*ce);
+extern void free_name_hash(struct index_state *istate);
#ifndef NO_THE_INDEX_COMPATIBILITY_MACROS
diff --git a/name-hash.c b/name-hash.c
index 942c459..6b130e1 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -32,38 +32,75 @@ static unsigned int hash_name(const char *name, int namelen)
return hash;
}
-static void hash_index_entry_directories(struct index_state *istate, struct
cache_entry *ce)
+struct dir_entry {
+ struct dir_entry *next;
+ struct dir_entry *parent;
+ struct cache_entry *ce;
+ int nr;
+ unsigned int namelen;
+};
+
+static struct dir_entry *find_dir_entry(struct index_state *istate,
+ const char *name, unsigned int namelen)
+{
+ unsigned int hash = hash_name(name, namelen);
+ struct dir_entry *dir;
+
+ for (dir = lookup_hash(hash, istate-dir_hash); dir; dir = dir-next)
+ if (dir-namelen == namelen
+ !strncasecmp(dir-ce-name, name, namelen))
+ return dir;
+ return NULL;
+}
+
+static struct dir_entry *hash_dir_entry(struct index_state *istate,
+ struct cache_entry *ce, int namelen, int add)
{
/*
* Throw each directory component in the hash for quick lookup
* during a git status. Directory components are stored with their
-* closing slash. Despite submodules being a directory, they never
-
Re: clean/smudge filters on .zip/.tgz files
Johannes Sixt venit, vidit, dixit 27.02.2013 07:39: Am 2/26/2013 23:38, schrieb Tim Chase: Various programs that I use ([Open|Libre]Office, Vym, etc) use a zipped/.tgz'ed file format, usually containing multiple (usually) plain-text files within. I'm trying to figure out a way for git to treat these as virtual directories for purposes of merging/diffing. Reading up on clean/smudge filters, it looks like they expect one file coming in and one file going out, rather than one file on one side and a directory-tree of files on the other side. I tried creating my own pair of clean/smudge filters that would uncompress the files, but there's no good way put multiple files on stdout. Has anybody else played with such a scheme for uncompressing files as they go into git and recompressing them as they come back out? I attempted to do something like this for OpenDocument files (I didn't get very far) until I discovered that LibreOffice can save flat open document files. That combined with the option save files optimized switched off results in fairly readable XML in a single file that can even be merged under some circumstances. You would still need a clean filter that normalizes the style numbers, cross reference marks and other stuff that changes each time LibreOffice saves the file. -- Hannes In general, using zip -0 is a good way of getting something that delta-compresses well and that can give a meaningful diff (and has no information loss). The (my) problem is that recompressing a zip archive (i.e. multi-file) is a pita, you can't just use a pipe unzip | zip -0. You'd have to do that in a temp dir. Michael -- 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] Documentation/submodule: Add --force to update synopsis
Thanks. -- 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
[BUG] git gui alert dialogs interfere with each other
(Seen in 1.7.12.3) When launching git gui in a repo where there are 1. lots of (5000) stashed changes (to do a tree move), and 2. many non-packed files, git gui throws 2 alert dialogs: one saying that it will only show 5000 changed files and ignore the others, which very quickly hidden by the offer to repack. If I decline repacking, then both dialogs are discarded, and the first one, which is quite critical, may not have been seen at all by the user. -- Yann Dirson - Bertin Technologies -- 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] attr: allow pattern escape using backslashes
Duy Nguyen pclo...@gmail.com writes: On Sun, Feb 24, 2013 at 4:15 PM, Junio C Hamano gits...@pobox.com wrote: Speaking of .gitignore, I recall that there was a hanging discussion on allowing a pattern to name the directory that the .gitignore file appears in, which I do not think we currently support. With such a feature, instead of listing /junk in the .gitignore file at the top-level to say that everything inside the junk directory is ignored by default, we could instead say this at the beginning of the .gitignore file in the junk directory. Shouldn't / alone in junk/.gitignore express that? It does not work, but I think it's a natural interpretation of the syntax. Yup, there is nothing that you can plug to the this in the above to make it mean junk directory is ignored. A trailing / is removed after noting the fact that the entry is about a directory and leaves an empty string, and it would be OK to define that an empty string matches the directory the gitignore file appears in. -- 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 blame -M seems not to work as expected
Hi Folks!
I've posed this question already on stackoverflow and on Google Groups -
without any (satisfying) answer. So maybe you can help me to understand the
behavior of git blame -M.
First I commit the following file(file.cpp):
void func1(){return;}[CR][LF]
int func2(){return 23;}[CR][LF]
Then I modify it by moving what was in the first line and adding something new
instead:
float newFunc(){return 23.0;}[CR][LF]
int func2(){return 23;}[CR][LF]
[CR][LF]
[CR][LF]
void func1(){return;}[CR][LF]
The log now looks as follows:
git log --oneline -2
18c670f modified file.cpp
92b4186 added file.cpp
Now I run blame:
git blame -s -w -M file.cpp
18c670fa 1) float newFunc(){return 23.0;}
92b4186d 2) int func2(){return 23;}
18c670fa 3)
18c670fa 4)
18c670fa 5) void func1(){return;}
I wonder why the line containing func1() isn't recognized as moved. I've tried
to reduce the number of required characters (i.e. -M4 etc.). Furthermore spaces
should not matter because of the -w option.
On the other hand, when I move float newFunc(){return 23.0;} from line 1 to
line 6 (which was empty before) in the subsequent commit, git blame -M
correctly recognizes that it originates from commit 18c670fa even though it
firstly appeared in line 6 only in the current commit.
So what's the reason for this seemingly inconsequent behavior? As far as I
understand the documentation, both movements should be recognized. It's very
important for us to correctly understand the behavior of git blame -M since we
are about to add some code analysis logic on top of git blame.
Thanks in advance
Konstantin
--
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 Notes - Search Functionality
krishna chaitanya kurnala kkc...@gmail.com writes:
I working on Git Notes. I want to know if there is an easy way to
obtain a list of all namespaces(For eg., git notes --ref=namespace
... ) with notes objects in a specific git repository. We can easily
create, edit, merge git notes with commands if we know the namespaces
and/or the sha. But, for example, Has anyone tried to search for a
string in a git notes objects for that project etc?
The closest i can think of is using some options with git logs, for
example, git log --show-notes=* --format=%H %N etc.
Appreciate your time.
An easy way to list everything in refs/notes/ is
git for-each-ref refs/notes/
but that of course won't figure out if you have more notes e.g. in
refs/remotes/origin/notes/* or some such. I think this more general
problem can be solved only by heuristics, since the notes trees are
actually just trees -- the only distinction is that they have fairly
funny filenames in them.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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: [PATCHv4 6/6] git-send-email: use git credential to obtain password
On Wed, Feb 27 2013, Junio C Hamano gits...@pobox.com wrote:
Matthieu Moy matthieu@grenoble-inp.fr writes:
Michal Nazarewicz m...@google.com writes:
+ $auth = Git::credential({
+ 'protocol' = 'smtp',
+ 'host' = join(':', $smtp_server, $smtp_server_port),
At this point, $smtp_server_port is not always defined. I just tested
and got
Use of uninitialized value $smtp_server_port in join or string at
git-send-email line 1077.
Other than that, the whole series looks good.
Given that there is another place that conditionally append :$port
to the host string, I think we should follow suit here. Perhaps
like the attached diff?
Damn meetings, you beat me to it… I was just about to send a patch. ;)
Thanks for a review.
git-send-email.perl | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 76bbfc3..c3501d9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1045,6 +1045,14 @@ sub maildomain {
return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
}
+sub smtp_host_string {
+ if (defined $smtp_server_port) {
+ return $smtp_server:$smtp_server_port;
+ } else {
+ return $smtp_server;
+ }
+}
+
# Returns 1 if authentication succeeded or was not necessary
# (smtp_user was not specified), and 0 otherwise.
@@ -1065,7 +1073,7 @@ sub smtp_auth_maybe {
# reject credentials.
$auth = Git::credential({
'protocol' = 'smtp',
- 'host' = join(':', $smtp_server, $smtp_server_port),
+ 'host' = smtp_host_string(),
'username' = $smtp_authuser,
# if there's no password, git credential fill will
# give us one, otherwise it'll just pass this one.
@@ -1188,9 +1196,7 @@ sub send_message {
else {
require Net::SMTP;
$smtp_domain ||= maildomain();
- $smtp ||= Net::SMTP-new((defined $smtp_server_port)
- ?
$smtp_server:$smtp_server_port
- : $smtp_server,
+ $smtp ||= Net::SMTP-new(smtp_host_string(),
Hello = $smtp_domain,
Debug = $debug_net_smtp);
if ($smtp_encryption eq 'tls' $smtp) {
From reading of SMTP.pm, it seems that this could be changed to:
- $smtp ||= Net::SMTP-new((defined $smtp_server_port)
-?
$smtp_server:$smtp_server_port
-: $smtp_server,
+ $smtp ||= Net::SMTP-new($smtp_server,
+Port = $smtp_server_port,
and than the other part would become:
@@ -1060,12 +1060,17 @@ sub smtp_auth_maybe {
Authen::SASL-import(qw(Perl));
};
+ my $host = $smtp_server;
+ if (defined $smtp_server_port) {
+ $host .= ':' . $smtp_server_port;
+ }
+
# TODO: Authentication may fail not because credentials were
# invalid but due to other reasons, in which we should not
# reject credentials.
$auth = Git::credential({
'protocol' = 'smtp',
- 'host' = join(':', $smtp_server, $smtp_server_port),
+ 'host' = $host,
'username' = $smtp_authuser,
# if there's no password, git credential fill will
# give us one, otherwise it'll just pass this one.
Either way, looks good to me.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz(o o)
ooo +email/xmpp: m...@google.com--ooO--(_)--Ooo--
pgpjon85cv7ZQ.pgp
Description: PGP signature
Re: Git Notes - Search Functionality
On Wed, Feb 27, 2013 at 4:34 PM, krishna chaitanya kurnala kkc...@gmail.com wrote: Hello All I working on Git Notes. I want to know if there is an easy way to obtain a list of all namespaces(For eg., git notes --ref=namespace ... ) with notes objects in a specific git repository. The notes namespaces are stored as regular refs under refs/notes/*. Here's one way to list the available namespaces: git for-each-ref refs/notes/ Hope this helps, ...Johan We can easily create, edit, merge git notes with commands if we know the namespaces and/or the sha. But, for example, Has anyone tried to search for a string in a git notes objects for that project etc? The closest i can think of is using some options with git logs, for example, git log --show-notes=* --format=%H %N etc. Appreciate your time. thanks Krishna Chaitanya -- 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 -- Johan Herland, jo...@herland.net www.herland.net -- 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] name-hash.c: fix endless loop with core.ignorecase=true
Karsten Blees karsten.bl...@gmail.com writes:
With core.ignorecase=true, name-hash.c builds a case insensitive index of
all tracked directories. Currently, the existing cache entry structures are
added multiple times to the same hashtable (with different name lengths and
hash codes). However, there's only one dir_next pointer, which gets
completely messed up in case of hash collisions. In the worst case, this
causes an endless loop if ce == ce-dir_next:
---8---
# V/, V/XQANY/ and WURZAUP/ all have the same hash_name
mkdir V
mkdir V/XQANY
mkdir WURZAUP
touch V/XQANY/test
git init
git config core.ignorecase true
git add .
git status
---8---
Instead of using the scissors mark to confuse am -c, indenting
this block would have been easier to later readers.
Also it is somewhat a shame that we do not use the above sample
collisions in a new test case.
+static struct dir_entry *hash_dir_entry(struct index_state *istate,
+ struct cache_entry *ce, int namelen, int add)
{
/*
* Throw each directory component in the hash for quick lookup
* during a git status. Directory components are stored with their
- * closing slash. Despite submodules being a directory, they never
- * reach this point, because they are stored without a closing slash
- * in the cache.
Is the description of submodule no longer relevant?
- * Note that the cache_entry stored with the directory does not
- * represent the directory itself. It is a pointer to an existing
- * filename, and its only purpose is to represent existence of the
- * directory in the cache. It is very possible multiple directory
- * hash entries may point to the same cache_entry.
Is this paragraph no longer relevant? It seems to me that it still
holds true, given the way how dir-ce points at the given ce.
+ * closing slash.
*/
+ struct dir_entry *dir, *p;
+
+ /* get length of parent directory */
+ while (namelen 0 !is_dir_sep(ce-name[namelen - 1]))
+ namelen--;
+ if (namelen = 0)
+ return NULL;
+
+ /* lookup existing entry for that directory */
+ dir = find_dir_entry(istate, ce-name, namelen);
+ if (add !dir) {
...
}
+
+ /* add or release reference to this entry (and parents if 0) */
+ p = dir;
+ if (add) {
+ while (p !(p-nr++))
+ p = p-parent;
+ } else {
+ while (p p-nr !(--p-nr))
+ p = p-parent;
+ }
Can we free the entry when its refcnt goes down to zero? If yes, is
it worth doing so?
+
+ return dir;
}
static void hash_index_entry(struct index_state *istate, struct cache_entry
*ce)
@@ -74,7 +111,7 @@ static void hash_index_entry(struct index_state *istate,
struct cache_entry *ce)
if (ce-ce_flags CE_HASHED)
return;
ce-ce_flags |= CE_HASHED;
- ce-next = ce-dir_next = NULL;
+ ce-next = NULL;
hash = hash_name(ce-name, ce_namelen(ce));
pos = insert_hash(hash, ce, istate-name_hash);
if (pos) {
@@ -82,8 +119,8 @@ static void hash_index_entry(struct index_state *istate,
struct cache_entry *ce)
*pos = ce;
}
- if (ignore_case)
- hash_index_entry_directories(istate, ce);
+ if (ignore_case !(ce-ce_flags CE_UNHASHED))
+ hash_dir_entry(istate, ce, ce_namelen(ce), 1);
}
static void lazy_init_name_hash(struct index_state *istate)
@@ -99,11 +136,33 @@ static void lazy_init_name_hash(struct index_state
*istate)
void add_name_hash(struct index_state *istate, struct cache_entry *ce)
{
+ /* if already hashed, add reference to directory entries */
+ if (ignore_case (ce-ce_flags CE_STATE_MASK) == CE_STATE_MASK)
+ hash_dir_entry(istate, ce, ce_namelen(ce), 1);
Instead of a single function with are we adding or removing?
parameter, it would be a lot easier to read the callers if they are
wrapped in two helpers, add_dir_entry() and del_dir_entry() or
something, especially when the add=[0|1] parameter is constant for
each and every callsite (i.e. the codeflow determines it, not the
data).
ce-ce_flags = ~CE_UNHASHED;
if (istate-name_hash_initialized)
hash_index_entry(istate, ce);
}
+/*
+ * We don't actually *remove* it, we can just mark it invalid so that
+ * we won't find it in lookups.
+ *
+ * Not only would we have to search the lists (simple enough), but
+ * we'd also have to rehash other hash buckets in case this makes the
+ * hash bucket empty (common). So it's much better to just mark
+ * it.
+ */
+void remove_name_hash(struct index_state *istate, struct cache_entry *ce)
+{
+ /* if already hashed, release reference to directory entries */
+ if (ignore_case (ce-ce_flags CE_STATE_MASK) == CE_HASHED)
+ hash_dir_entry(istate, ce, ce_namelen(ce),
Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN
Greg Price pr...@mit.edu writes:
Currently when --all is passed, the effect of --match is only
to demote non-matching tags to be treated like non-tags. This
is puzzling behavior and not consistent with the documentation,
especially with the suggested usage of avoiding information leaks.
The combination of --all and --match is an oxymoron anyway, so
just forbid it.
I am not sure if this is (1) behaviour is sometimes useful in
narrow cases but is not explained well, (2) behaviour does not
make sense in any situation, or (3) the combination can make sense
if corrected, but the current behaviour is buggy. If it is (2) or
(3), I think it makes sense to forbid the combination. Also, if it
is (3), we should later come up with an improved behaviour and then
re-enable the combination.
Without --all the command considers only the annotated tags to
base the descripion on, and with --all, a ref that is not
annotated tags can be used as a base, but with a lower priority (if
an annotated tag can describe a given commit, that tag is used).
So naïvely I would expect --all and --match to base the
description on refs that match the pattern without limiting the
choice of base to annotated tags, and refs that do not match the
given pattern should not appear even as the last resort. It appears
to me that the current situation is (3).
Will queue and cook in 'next'; thanks.
Signed-off-by: Greg Price pr...@mit.edu
---
This should be applied after the preceding patch; I mistakenly omitted
the '1/2' in its subject line.
Documentation/git-describe.txt | 3 ++-
builtin/describe.c | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 711040d..fd5d8f2 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -83,7 +83,8 @@ OPTIONS
--match pattern::
Only consider tags matching the given `glob(7)` pattern,
excluding the refs/tags/ prefix. This can be used to avoid
- leaking private tags from the repository.
+ leaking private tags from the repository. This option is
+ incompatible with `--all`.
--always::
Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 04c185b..90a72af 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -435,6 +435,9 @@ int cmd_describe(int argc, const char **argv, const char
*prefix)
if (longformat abbrev == 0)
die(_(--long is incompatible with --abbrev=0));
+ if (pattern all)
+ die(_(--match is incompatible with --all));
+
if (contains) {
const char **args = xmalloc((7 + argc) * sizeof(char *));
int i = 0;
--
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: https_proxy and libcurl
On Fri, Feb 22, 2013 at 4:55 PM, Junio C Hamano gits...@pobox.com wrote: Phil Hord phil.h...@gmail.com writes: I have been unable to clone via http proxy because of a wrongly configured proxy setup in my lab. I had this env: http_proxy=http://proxy.myco.com https_proxy=https://proxy.myco.com The problem is that libcurl ignores the protocol part of the proxy url, and it defaults to port 1080. wget honors the protocol specifier, but it defaults to port 80 if none is given. IIRC, the historical norm is to set these to host:port. So many people mistakenly write them with method:// that some tools over time learned to strip and ignore that prefix, though. On the server in question, I use wget to retrieve packages in another script. When our firewall was tightened in the past, this script broke. I fixed it by setting the https_proxy as I listed above. This satisfied wget and I got on with life. I do not need git connectivity to remotes from this server very often. But when I tried, it failed with an error message that did not implicate the proxy server. But it was the proxy server that returned the error 500. error: The requested URL returned error: 500 while accessing https://github.com/git/git/info/refs?service=git-upload-pack The fix was to specify the port explicitly, like this: http_proxy=proxy.myco.com:80 https_proxy=proxy.myco.com:443 Yeah, that is the correct syntax to use. Is there anything you want Git to do to be more helpful? Maybe Git can tell me more about proxy failures. Or maybe Git doesn't know because curl hides the knowledge. Something like this: error: The proxy server at proxy.myco.com:1080 returned error: 500 while accessing https://github.com/git/git/info/refs?service=git-upload-pack Maybe Git can warn me that the protocol prefix (if provided) is going to be ignored or that I have failed to provide a port. Either of these would be above and beyond the norm. But since curl is internal to Git and its errors are digested by Git, it is less obvious where to begin looking to solve such a problem; git doesn't provide the plethora of connection-debugging output options that curl itself does. Fwiw, the proxy info originated with our IT department who mostly had to answer the question for Windows users and servers. Windows appears to default to port 8080 when no port is specified. Port 8080 would also have worked, but curl defaults to 1080. It seems that the curl default of 1080 is the only one that caused me trouble. My basest hope is that someone suffering similar troubles will find this thread in a search for git proxy error. Phil -- 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] name-hash.c: fix endless loop with core.ignorecase=true
Am 27.02.2013 17:53, schrieb Junio C Hamano:
Karsten Blees karsten.bl...@gmail.com writes:
With core.ignorecase=true, name-hash.c builds a case insensitive index of
all tracked directories. Currently, the existing cache entry structures are
added multiple times to the same hashtable (with different name lengths and
hash codes). However, there's only one dir_next pointer, which gets
completely messed up in case of hash collisions. In the worst case, this
causes an endless loop if ce == ce-dir_next:
---8---
# V/, V/XQANY/ and WURZAUP/ all have the same hash_name
mkdir V
mkdir V/XQANY
mkdir WURZAUP
touch V/XQANY/test
git init
git config core.ignorecase true
git add .
git status
---8---
Instead of using the scissors mark to confuse am -c, indenting
this block would have been easier to later readers.
Also it is somewhat a shame that we do not use the above sample
collisions in a new test case.
Duly noted.
Is there a way to run 'git status' with timeout? A test that doesn't complete
(instead of failing) isn't nice...
+static struct dir_entry *hash_dir_entry(struct index_state *istate,
+struct cache_entry *ce, int namelen, int add)
{
/*
* Throw each directory component in the hash for quick lookup
* during a git status. Directory components are stored with their
- * closing slash. Despite submodules being a directory, they never
- * reach this point, because they are stored without a closing slash
- * in the cache.
Is the description of submodule no longer relevant?
- * Note that the cache_entry stored with the directory does not
- * represent the directory itself. It is a pointer to an existing
- * filename, and its only purpose is to represent existence of the
- * directory in the cache. It is very possible multiple directory
- * hash entries may point to the same cache_entry.
Is this paragraph no longer relevant? It seems to me that it still
holds true, given the way how dir-ce points at the given ce.
I interpreted this as an explanation why it was safe to add the same
cache_entry to the same name_hash multiple times...now that we have separate
dir_entries and index_state.dir_hash, that's no longer a problem. But rereading
that paragraph again, it is still mostly true (except for the 'existance' part,
which is solved by reference counting).
+ * closing slash.
*/
+struct dir_entry *dir, *p;
+
+/* get length of parent directory */
+while (namelen 0 !is_dir_sep(ce-name[namelen - 1]))
+namelen--;
+if (namelen = 0)
+return NULL;
+
+/* lookup existing entry for that directory */
+dir = find_dir_entry(istate, ce-name, namelen);
+if (add !dir) {
...
}
+
+/* add or release reference to this entry (and parents if 0) */
+p = dir;
+if (add) {
+while (p !(p-nr++))
+p = p-parent;
+} else {
+while (p p-nr !(--p-nr))
+p = p-parent;
+}
Can we free the entry when its refcnt goes down to zero? If yes, is
it worth doing so?
There's no remove_hash in hash.[ch], and dir_entry.next may point to another
dir_entry with the same hash code, so we must not free the memory (same problem
as CE_UNHASHED).
+
+return dir;
}
static void hash_index_entry(struct index_state *istate, struct cache_entry
*ce)
@@ -74,7 +111,7 @@ static void hash_index_entry(struct index_state *istate,
struct cache_entry *ce)
if (ce-ce_flags CE_HASHED)
return;
ce-ce_flags |= CE_HASHED;
-ce-next = ce-dir_next = NULL;
+ce-next = NULL;
hash = hash_name(ce-name, ce_namelen(ce));
pos = insert_hash(hash, ce, istate-name_hash);
if (pos) {
@@ -82,8 +119,8 @@ static void hash_index_entry(struct index_state *istate,
struct cache_entry *ce)
*pos = ce;
}
-if (ignore_case)
-hash_index_entry_directories(istate, ce);
+if (ignore_case !(ce-ce_flags CE_UNHASHED))
+hash_dir_entry(istate, ce, ce_namelen(ce), 1);
}
static void lazy_init_name_hash(struct index_state *istate)
@@ -99,11 +136,33 @@ static void lazy_init_name_hash(struct index_state
*istate)
void add_name_hash(struct index_state *istate, struct cache_entry *ce)
{
+/* if already hashed, add reference to directory entries */
+if (ignore_case (ce-ce_flags CE_STATE_MASK) == CE_STATE_MASK)
+hash_dir_entry(istate, ce, ce_namelen(ce), 1);
Instead of a single function with are we adding or removing?
parameter, it would be a lot easier to read the callers if they are
wrapped in two helpers, add_dir_entry() and del_dir_entry() or
something, especially when the add=[0|1] parameter is constant for
each and every callsite (i.e. the codeflow determines it, not the
data).
OK
ce-ce_flags = ~CE_UNHASHED;
if
how to check pending commits to be pushed?
Hi, Does someone know how to identify pending commits to be pushed in a local super repository (with quite some submodules), with git-1.6.5-1 ? Thanks, Patricia-- 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 subtree workflow
On Tue, Feb 26, 2013 at 9:04 AM, Ben McCann b...@benmccann.com wrote: I'm fairly new to git and am trying to determine if git subtree would be helpful for managing our company's codebase, which has several repos some of which depend on each other. All the examples I've seen make sense to me as a one-time operation to merge separate repos into one monolithic one or to split one monolithic repo into separate repos. I'm having a harder time understanding how this fits into a larger picture and what the workflow for working with subtree would be. The git subtree command does support pushing and pulling changes to/from a subtree to a remote repo. This operation can be repeated over time as the underlying git subtree split command produces an accurate extract from the subtree. For example: $ git clone $superrepo super $ cd super $ git subtree add --prefix=child $childrepo master $ split_head=$(git subtree split --prefix=child) git subtree split creates a synthetic copy of the child subtree and returns the SHA1 for the head of that subtree. $ cd .. $ git clone $childrepo child $ cd child $ child_head=$(git rev-parse master) $split_head and $child_head match. You could pass either $*_head to gitk and be able to browse identical trees and histories. $ gitk $child_head $ cd ../super $ gitk $split_head The only difference you should see is that the tree in the super project doesn't have any branches or tags from the child repo. If I have a bunch of repos on GitHub and some depend on each other, how would I set them up to work with subtree? Would GitHub continue to host them as is, host a merged monolithic repo, or host both a monolithic repo and the splitted out repos? The exact answer probably varies, but I imagine there's basic workflow that would satisfy 80% of users. If I have GitHub host both monolithic and splitted out repos, it seems unclear as to how I keep those repos in sync and make sure all the developers in our company push their changes to both repos. This is what I'm using git-subtree for. I have a super project which holds child repos, some of which hold other child repos. You can develop on all of your projects within the super project and still periodically pull and push updates with the remote child repos. Someone who doesn't know about your child repos can take the monolithic repo and commit to it as normal. You can then pull their commits and push any changes to the child repos. Probably to a non-master branch so you they can be integrated like other pull requests. Caveats: 1) I would probably avoid the --squash option as I suspect that might mess with $split_head == $child_head. Whether or not it would have any impact of your ability to push or pull I don't know. I've not tried it. 2) When you commit you should ideally not commit a set of files that are in more than one subtree. You're commit history when you push back to the source repo for a subtree might look a bit odd if it's referring to other parts of the super project or it's subtrees. -- Paul [W] Campbell -- 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
[BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present
cmd_add() attempts to check for the validity of refspec for the repository
it is about to add as a subtree. It tries to do so before contacting the
repository. If the refspec happens to exist locally (say 'master') then
the test passes and the repo is fetched. If the refspec doesn't exist
locally then the test fails and the remote repo is never contacted.
Removing the tests still works as the git fetch command fails with the
perfectly accurate error:
fatal: Couldn't find remote ref refspec
Signed-off-by: Paul Campbell pcampb...@carnegiecollege.ac.uk
---
I must confess to not understanding the comment preceding the
rev-parse test. Given that it is against the rev-parse and not the
cmd_add_repository call I'm assuming it can be removed.
contrib/subtree/git-subtree.sh | 8
1 file changed, 8 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8a23f58..9a38b18 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -503,14 +503,6 @@ cmd_add()
cmd_add_commit $@
elif [ $# -eq 2 ]; then
- # Technically we could accept a refspec here but we're
- # just going to turn around and add FETCH_HEAD under the
- # specified directory. Allowing a refspec might be
- # misleading because we won't do anything with any other
- # branches fetched via the refspec.
- git rev-parse -q --verify $2^{commit} /dev/null ||
- die '$2' does not refer to a commit
-
cmd_add_repository $@
else
say error: parameters were '$@'
--
1.8.2.rc1
--
Paul [W] Campbell
--
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] name-hash.c: fix endless loop with core.ignorecase=true
With core.ignorecase=true, name-hash.c builds a case insensitive index of
all tracked directories. Currently, the existing cache entry structures are
added multiple times to the same hashtable (with different name lengths and
hash codes). However, there's only one dir_next pointer, which gets
completely messed up in case of hash collisions. In the worst case, this
causes an endless loop if ce == ce-dir_next (see t7062).
Use a separate hashtable and separate structures for the directory index
so that each directory entry has its own next pointer. Use reference
counting to track which directory entry contains files.
There are only slight changes to the name-hash.c API:
- new free_name_hash() used by read_cache.c::discard_index()
- remove_name_hash() takes an additional index_state parameter
- index_name_exists() for a directory (trailing '/') may return a cache
entry that has been removed (CE_UNHASHED). This is not a problem as the
return value is only used to check if the directory exists (dir.c) or to
normalize casing of directory names (read-cache.c).
Getting rid of cache_entry.dir_next reduces memory consumption, especially
with core.ignorecase=false (which doesn't use that member at all).
With core.ignorecase=true, building the directory index is slightly faster
as we add / check the parent directory first (instead of going through all
directory levels for each file in the index). E.g. with WebKit (~200k
files, ~7k dirs), time spent in lazy_init_name_hash is reduced from 176ms
to 130ms.
Signed-off-by: Karsten Blees bl...@dcon.de
---
Also available here:
https://github.com/kblees/git/tree/kb/name-hash-fix-endless-loop-v2
git pull git://github.com/kblees/git.git kb/name-hash-fix-endless-loop-v2
cache.h| 17 +---
name-hash.c| 182 +++--
read-cache.c | 9 +-
t/t7062-wtstatus-ignorecase.sh | 20 +
4 files changed, 166 insertions(+), 62 deletions(-)
create mode 100755 t/t7062-wtstatus-ignorecase.sh
diff --git a/cache.h b/cache.h
index e493563..898e346 100644
--- a/cache.h
+++ b/cache.h
@@ -131,7 +131,6 @@ struct cache_entry {
unsigned int ce_namelen;
unsigned char sha1[20];
struct cache_entry *next;
- struct cache_entry *dir_next;
char name[FLEX_ARRAY]; /* more */
};
@@ -267,25 +266,15 @@ struct index_state {
unsigned name_hash_initialized : 1,
initialized : 1;
struct hash_table name_hash;
+ struct hash_table dir_hash;
};
extern struct index_state the_index;
/* Name hashing */
extern void add_name_hash(struct index_state *istate, struct cache_entry *ce);
-/*
- * We don't actually *remove* it, we can just mark it invalid so that
- * we won't find it in lookups.
- *
- * Not only would we have to search the lists (simple enough), but
- * we'd also have to rehash other hash buckets in case this makes the
- * hash bucket empty (common). So it's much better to just mark
- * it.
- */
-static inline void remove_name_hash(struct cache_entry *ce)
-{
- ce-ce_flags |= CE_UNHASHED;
-}
+extern void remove_name_hash(struct index_state *istate, struct cache_entry
*ce);
+extern void free_name_hash(struct index_state *istate);
#ifndef NO_THE_INDEX_COMPATIBILITY_MACROS
diff --git a/name-hash.c b/name-hash.c
index 942c459..6d7e198 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -32,38 +32,96 @@ static unsigned int hash_name(const char *name, int namelen)
return hash;
}
-static void hash_index_entry_directories(struct index_state *istate, struct
cache_entry *ce)
+struct dir_entry {
+ struct dir_entry *next;
+ struct dir_entry *parent;
+ struct cache_entry *ce;
+ int nr;
+ unsigned int namelen;
+};
+
+static struct dir_entry *find_dir_entry(struct index_state *istate,
+ const char *name, unsigned int namelen)
+{
+ unsigned int hash = hash_name(name, namelen);
+ struct dir_entry *dir;
+
+ for (dir = lookup_hash(hash, istate-dir_hash); dir; dir = dir-next)
+ if (dir-namelen == namelen
+ !strncasecmp(dir-ce-name, name, namelen))
+ return dir;
+ return NULL;
+}
+
+static struct dir_entry *hash_dir_entry(struct index_state *istate,
+ struct cache_entry *ce, int namelen)
{
/*
* Throw each directory component in the hash for quick lookup
* during a git status. Directory components are stored with their
* closing slash. Despite submodules being a directory, they never
* reach this point, because they are stored without a closing slash
-* in the cache.
+* in index_state.name_hash (as ordinary cache_entries).
*
-* Note that the cache_entry stored with the directory does not
-* represent the directory itself. It is a pointer to an existing
-* filename, and its only purpose is to
Re: [BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present
Paul Campbell pcampb...@kemitix.net writes:
cmd_add() attempts to check for the validity of refspec for the repository
it is about to add as a subtree. It tries to do so before contacting the
repository. If the refspec happens to exist locally (say 'master') then
the test passes and the repo is fetched. If the refspec doesn't exist
locally then the test fails and the remote repo is never contacted.
Removing the tests still works as the git fetch command fails with the
perfectly accurate error:
fatal: Couldn't find remote ref refspec
Signed-off-by: Paul Campbell pcampb...@carnegiecollege.ac.uk
---
I must confess to not understanding the comment preceding the
rev-parse test. Given that it is against the rev-parse and not the
cmd_add_repository call I'm assuming it can be removed.
This is contrib/ material and I do not use the command, so anything
I say should be taken with a moderate amount of salt, but I think
the comment is talking about _not_ accepting a full storing refspec,
or worse yet wildcard, e.g.
refs/heads/topic:refs/remotes/origin/topic
refs/heads/*:refs/remotes/origin/*
which will not make sense given that you are only adding a single
commit via cmd_add. Also, if you have already fetched from the
remote, rev-parse $2^{commit} at this point will protect against
a typo by the end user.
As you noticed, checking if $2 is a commit using rev-parse _before_
fetching $2 from the remote repository is not a valid way to check
against such errors. So I tend to agree that removing the die
will be a good idea.
Typing tipoc when the user meant topic will error out at the
git fetch done in cmd_add_repository, but that fetch will happily
fetch and store whatever a refspec specifies, so you might want to
replace the bogus rev-parse before fetching check to reject
refspec with something else.
contrib/subtree/git-subtree.sh | 8
1 file changed, 8 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8a23f58..9a38b18 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -503,14 +503,6 @@ cmd_add()
cmd_add_commit $@
elif [ $# -eq 2 ]; then
- # Technically we could accept a refspec here but we're
- # just going to turn around and add FETCH_HEAD under the
- # specified directory. Allowing a refspec might be
- # misleading because we won't do anything with any other
- # branches fetched via the refspec.
- git rev-parse -q --verify $2^{commit} /dev/null ||
- die '$2' does not refer to a commit
-
cmd_add_repository $@
else
say error: parameters were '$@'
--
1.8.2.rc1
--
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 v2] name-hash.c: fix endless loop with core.ignorecase=true
Karsten Blees karsten.bl...@gmail.com writes: With core.ignorecase=true, name-hash.c builds a case insensitive index of all tracked directories. Currently, the existing cache entry structures are added multiple times to the same hashtable (with different name lengths and hash codes). However, there's only one dir_next pointer, which gets completely messed up in case of hash collisions. In the worst case, this causes an endless loop if ce == ce-dir_next (see t7062). Use a separate hashtable and separate structures for the directory index so that each directory entry has its own next pointer. Use reference counting to track which directory entry contains files. There are only slight changes to the name-hash.c API: - new free_name_hash() used by read_cache.c::discard_index() - remove_name_hash() takes an additional index_state parameter - index_name_exists() for a directory (trailing '/') may return a cache entry that has been removed (CE_UNHASHED). This is not a problem as the return value is only used to check if the directory exists (dir.c) or to normalize casing of directory names (read-cache.c). Getting rid of cache_entry.dir_next reduces memory consumption, especially with core.ignorecase=false (which doesn't use that member at all). With core.ignorecase=true, building the directory index is slightly faster as we add / check the parent directory first (instead of going through all directory levels for each file in the index). E.g. with WebKit (~200k files, ~7k dirs), time spent in lazy_init_name_hash is reduced from 176ms to 130ms. Signed-off-by: Karsten Blees bl...@dcon.de --- One thing that still puzzles me is what guarantee we have on the liftime of these ce's that are borrowed by these dir_hash entries. There are a few places where we call free(ce) around aliased entries (only happens with ignore_case set). I do not think it is a new issue (we used to borrow a ce to represent a directory in the name_hash by using the leading prefix of its name anyway, and this patch only changes which hash table is used to hold it), and I do not think it will be an issue for case sensitive systems, so I would stop being worried about it for now, though ;-) Thanks, will replace and queue. diff --git a/t/t7062-wtstatus-ignorecase.sh b/t/t7062-wtstatus-ignorecase.sh new file mode 100755 index 000..73709db --- /dev/null +++ b/t/t7062-wtstatus-ignorecase.sh @@ -0,0 +1,20 @@ +#!/bin/sh + +test_description='git-status with core.ignorecase=true' + +. ./test-lib.sh + +test_expect_success 'status with hash collisions' ' + # note: V/, V/XQANY/ and WURZAUP/ produce the same hash code + # in name-hash.c::hash_name + mkdir V + mkdir V/XQANY + mkdir WURZAUP + touch V/XQANY/test + git config core.ignorecase true + git add . + # test is successful if git status completes (no endless loop) + git status +' + +test_done -- 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 2/4] config: drop file pointer validity check in get_next_char()
On Wed, Feb 27, 2013 at 08:52:57AM +0100, Heiko Voigt wrote:
On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:
On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:
The only location where cf is set in this file is in do_config_from().
This function has only one callsite which is config_from_file(). In
config_from_file() its ensured that the f member is set to non-zero.
Makes sense, although...
- if (cf ((f = cf-f) != NULL)) {
+ if (cf) {
+ FILE *f = cf-f;
Couldn't we say the same thing about cf here (i.e., that it would
never be NULL)? Can we just get rid of this conditional entirely?
That might be true. I will look into it. Just wanted to get rid of an
extra callback in my series.
I had a look and it might be true that cf will never be NULL in a code
path. Nevertheless its much harder to verify by looking at the code
since its a global variable. get_next_char() is called from all over the
place and I would have to look at all the code paths. As far as I know
static global variables are always initialized to zero so its safe to
check even if has not yet been explicitly initialized.
The statement if cf is not NULL all members will be initialized is much
simpler to verify since its just one place now and two places after this
series.
Cheers Heiko
--
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 2/4] config: drop file pointer validity check in get_next_char()
On Thu, Feb 28, 2013 at 01:41:47AM +0100, Heiko Voigt wrote:
On Wed, Feb 27, 2013 at 08:52:57AM +0100, Heiko Voigt wrote:
On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:
On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:
The only location where cf is set in this file is in do_config_from().
This function has only one callsite which is config_from_file(). In
config_from_file() its ensured that the f member is set to non-zero.
Makes sense, although...
- if (cf ((f = cf-f) != NULL)) {
+ if (cf) {
+ FILE *f = cf-f;
Couldn't we say the same thing about cf here (i.e., that it would
never be NULL)? Can we just get rid of this conditional entirely?
That might be true. I will look into it. Just wanted to get rid of an
extra callback in my series.
I had a look and it might be true that cf will never be NULL in a code
path. Nevertheless its much harder to verify by looking at the code
since its a global variable. get_next_char() is called from all over the
place and I would have to look at all the code paths. As far as I know
static global variables are always initialized to zero so its safe to
check even if has not yet been explicitly initialized.
The statement if cf is not NULL all members will be initialized is much
simpler to verify since its just one place now and two places after this
series.
To add some empirical information: I just ran the testsuite without the
conditional and it still passes. To me it only make sense to start the
parsing with cf initialized. But I am not familiar enough with the code
to judge whether it is safe to assume this.
Cheers Heiko
--
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
Beats by Dre Solo
Beats by Dre Schweiz http://www.beatsbydreschweiz.org/ Girls, sehen, dass Look, sie sind die Errungenschaften der Periode von tausend Jahren gebunden sind.Der Winter dieses Jahres, die extra kalt, Ende November, um den Schnee schweben, gab es zahlreiche, der Traum in Pianfei die Strecke weißen Schmetterlingsflügel. Hongmei nur ein Baum, ein Baum zarten Duft Fry. An einen schneeweißen Frau, stehend auf dem Hof, sah nicht wie der menschliche Wohlstand verbracht Schnee Trance. Ja, sie, die fallen in der regen Vorhang, wo das Mädchen nahm den Jungen Mantel, und jetzt ist die Zeit eilte 3 Jahre ist, ist sie auch von Sentimental Qingdouchukai Jasmine die längst Tingting Frost Saussurea als je zuvor, und kälter schöner. Sie stand im Schnee, eine weiße, Hongmei Kreuz leuchtete ergreifende Jueyan Malerei, und diese schöne und für wen Blüte, diese ergreifende und für wen distressed? Heute vor drei Jahren, dieser Schnee Samuume Durchsetzung Mädchen, die Hand des Jungen, der Baum der Ome Geld kam, nahm sie eine brillantesten anderen Schleife im Haar küsste Mädchen Kondensat cream Meijiao distressed, Bestürzung, sie sanft in meine Arme zu gießen. Boy zu verlassen, um die Mädchen zu der Feldstudie zu verlassen. Sie haben eine gute lange Trennung. Das Mädchen nicht zu ihr Schrei in meinem Herzen verborgen weinen, obwohl ihre Bestürzung, aber immer noch glauben, die Jungs sind zurück, und geben Sie ihr eine märchenhafte Liebe. Beats by Dre Studio http://www.beatsbydreschweiz.org/ Der Knabe versprach das Mädchen in regelmäßigem Kontakt zu sein, und so das Buch zu lesen, werden sie versprach eine gute Zeit und Ort zu treffen. Auf diese Weise, das schwache, einsam einsam Abschied.Der Junge war verschwunden, die Jahre des Wartens, das Mädchen Fantasie einen Schmetterling mit einem geliebten Menschen mit einem kleinen Strasse im Frühling fangen: mit dem Gurgeln des Wassers im Sommer Tageslicht, im Schatten sitzen beobachten Wellen durch die Schichten der Blätter , Schuppen gesprenkelt; schlendern gemeinsam im Herbst roten Ahornblättern, starrte den fernen Ruzhui die Wildgänse: fantasy im Winter, nur zwei von ihnen auf der Couch sitzen und genießen Sie das sanfte Sonnenlicht Aufteilung flüchtig.Unerwartet, während ein Jahr später, ein Mädchen, in den Jahreszeiten, um die Dinge, sie wollte mit dem Jungen zu tun. Sie hat nie den Jungen, bevor er für sein Engagement vergessen, warten seit Jahren zu blühen, konnte sie sogar einen Brief zu erhalten. Guhuai Tim dreimal klingelt, dann das Mädchen wurde Wartezeiten von einem Schnee-Kinder generell geschnitzt. Pale Haze von notleidenden Menschen, und das scheint Organisation Organisation der Daimei soulful Mouguang eine Pfütze spiegelt die Klarheit Schnee leere See statt düster Schultern hinter, wie niedergeschlagen sinkenden einsam Sorgen um Herz, wie zerbrechlich Trauer. Beats by Dre Solo Schweiz http://www.beatsbydreschweiz.org/ Es muss rufen jede Nacht, die Seele des Liebenden, freut sich auf Abteufen der gleichen wandernden Herzen ein letzter Tanz mit ihm.Mein Herz ist gebrochen, sondern auch, sie zu heilen? Seele weh, auch wenn Jahre verschwinden, und auch auf die Spuren nicht gehabt haben Schmerzen. Woman, Chu Chen Nazhu Zeugen ihrer Liebe und Abschied von Pflaume, zog nach einer schönen Vase. Sie wusste, dass er egoistisch war, Haltung nie gesehen Samuume stolz im Schnee blühen, sie in der Wärme des Hauses eingesperrt, weil sie weiß, dass es keine Liebe Kameradschaft, Samuume, allmählich verliert seine Seele als ihre eigenen Allgemeinen. Sie wurde täglich sah Chi Mei, scheinen zu erkennen, dass Pflege zutiefst persönlich. Sticks Plum erlebt die Bitterkeit der Frau, absorbieren die Tränen der Frau, wird es auch eingefallen. Zeit, ah, du bist so herzlos sind, dann fahren rücksichtslosen Menschen nehmen es weg, das ist sicher, verließ nicht einmal der Schatten, so dass eine Frau in der Liebe finden Sie gefangen, Erleichterung zu bekommen. Leugne nicht, dass Liebe schön ist, kann es machen strahlend, können Sie ließ die verwelkte Blumen wieder blühen. Aber seine Fehler ist, dass es zu schön ist, wie ein Mohn, zu attraktiv. Ohne die richtigen Leute an der richtigen Zeit am falschen Zeit, diskrete treffen, und ist eine Lebensdauer von Schmerzen. - Beats by Dre Solo Beats by Dre Schweiz Beats by Dre Studio Schweiz Beats by Dre Online Shop Beats by Dre Solo Schweiz -- View this message in context: http://git.661346.n2.nabble.com/Beats-by-Dre-Solo-tp7578769.html Sent from the git mailing list archive at Nabble.com. -- 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
