* Re: [PATCH] Makefile: don't run rm without any files
From: Jonathan Nieder @ 2013-02-13 20:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matt Kraai, git, Matt Kraai
In-Reply-To: <7vehgk6l11.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> I amended the log message like so:
>
> commit bd9df384b16077337fffe9836c9255976b0e7b91
> Author: Matt Kraai <matt.kraai@amo.abbott.com>
> Date: Wed Feb 13 07:57:48 2013 -0800
>
> Makefile: don't run rm without any files
>
> When COMPUTE_HEADER_DEPENDENCIES is set to "auto" and the compiler
> does not support it, $(dep_dirs) becomes empty. "make clean" runs
> "rm -rf $(dep_dirs)", which fails in such a case.
To pedantic, that only fails on some platforms. The autoconf manual
explains:
It is not portable to invoke rm without options or operands. On the
other hand, Posix now requires rm -f to silently succeed when there are
no operands (useful for constructs like rm -rf $filelist without first
checking if ‘$filelist’ was empty). But this was not always portable; at
least NetBSD rm built before 2008 would fail with a diagnostic.
Anyway, looks like a good fix. Thanks.
^ permalink raw reply
* Re: [PATCH] Makefile: don't run rm without any files
From: Junio C Hamano @ 2013-02-13 20:01 UTC (permalink / raw)
To: Matt Kraai; +Cc: git, Jonathan Nieder, Matt Kraai
In-Reply-To: <20130213170028.GA410@ftbfs.org>
Matt Kraai <kraai@ftbfs.org> writes:
> I don't set COMPUTE_HEADER_DEPENDENCIES, so it defaults to "auto".
> The automatic detection determines that the compiler doesn't support
> it, so it's then set to "no". CHECK_HEADER_DEPENDENCIES isn't set
> either, so about 20 lines below the dep_dirs assignment you quoted,
> dep_dirs is cleared:
>
> ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
> ifndef CHECK_HEADER_DEPENDENCIES
> dep_dirs =
> ...
>
> Should I submit an updated patch with a different commit message?
I amended the log message like so:
commit bd9df384b16077337fffe9836c9255976b0e7b91
Author: Matt Kraai <matt.kraai@amo.abbott.com>
Date: Wed Feb 13 07:57:48 2013 -0800
Makefile: don't run rm without any files
When COMPUTE_HEADER_DEPENDENCIES is set to "auto" and the compiler
does not support it, $(dep_dirs) becomes empty. "make clean" runs
"rm -rf $(dep_dirs)", which fails in such a case.
Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH v4] submodule: add 'deinit' command
From: Junio C Hamano @ 2013-02-13 19:56 UTC (permalink / raw)
To: Jens Lehmann
Cc: Phil Hord, Git Mailing List, Heiko Voigt, Michael J Gruber,
Marc Branchaud, W. Trevor King
In-Reply-To: <511BEA75.6000002@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> Junio, this looks like a we have v5 as soon as we decide what to do
> with the "not initialized" messages when '.' is used, right?
OK. I myself do not deeply care if we end up special casing "." or
not; I'll leave it up to you and other submodule folks.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState
From: Junio C Hamano @ 2013-02-13 19:53 UTC (permalink / raw)
To: Martin Erik Werner; +Cc: git, trsten
In-Reply-To: <1360777065.13768.17.camel@mas>
Martin Erik Werner <martinerikwerner@gmail.com> writes:
>> Strictly speaking, you have 6 not 4 combinations (shell variable
>> set/unset * config missing/set to false/set to true). I think these
>> additional tests cover should all 6 because "config missing" case
>> should already have had tests before bash.showDirtyState was added.
>>
>
> Indeed, I only mentioned 4 since the other ones existed already, and I
> didn't change them, but maybe it should be mentioned as "combined with
> previous tests (...) cover all 6 combinations (...)" then?
It should be sufficient to change the third line of your original to
say "the config option being missing/enabled/disabled, given a dirty
file." and nothing else, I think.
>> Sign-off?
>
> Ah, just forgot the -s flag on that commit, yes it should be Signed-off
> by me.
OK, I'll locally amend the patch. Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/3] t9903: add tests for bash.showUntrackedFiles
From: Junio C Hamano @ 2013-02-13 19:51 UTC (permalink / raw)
To: Martin Erik Werner; +Cc: git, trsten
In-Reply-To: <1360776461.13768.8.camel@mas>
Martin Erik Werner <martinerikwerner@gmail.com> writes:
> So would it make sense to do:
> GIT_PS1_SHOWUNTRACKEDFILES="dummy" &&
> unset GIT_PS1_SHOWUNTRACKEDFILES &&
> (...)
> instead then?
I think we have sane_unset exactly for this reason.
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Jeff King @ 2013-02-13 19:47 UTC (permalink / raw)
To: Duy Nguyen
Cc: blees, kusmabite, Ramkumar Ramachandra, Robert Zeh,
Junio C Hamano, Git List, finnag
In-Reply-To: <20130213181851.GA5603@sigill.intra.peff.net>
On Wed, Feb 13, 2013 at 01:18:51PM -0500, Jeff King wrote:
> I think the best way forward is to actually create a separate hash table
> for the directory lookups. I note that we only care about these entries
> in directory_exists_in_index_icase, which is really about whether
> something is there, versus what exactly is there. So could we maybe get
> by with a separate hash table that stores a count of entries at each
> directory, and increment/decrement the count when we add/remove entries?
>
> The biggest problem I see with that is that we do indeed care a little
> bit what is at the directory: we check the mode to see if it is a gitdir
> or not. But I think we can maybe sneak around that: gitdirs have actual
> entries in the index, whereas the directories do not. So we would find
> them via index_name_exists; anything that is not there, but _is_ in the
> special directory hash would therefore be a directory.
>
> I realize it got pretty esoteric there in the middle. I'll see if I can
> work up a patch that expresses what I'm thinking.
So here's a patch. It's mostly meant to illustrate what I'm thinking,
and I have no clue if it introduces regressions. It does pass the test
suite, but we have virtually no ignorecase tests. It seems to behave
sanely when I set core.ignorecase on my Linux box, but I have no idea
what it will do on a real case-insensitive system (nor even, to be
honest, what kinds of scenarios should be tested for the dir-hashing
stuff).
---
diff --git a/cache.h b/cache.h
index e493563..6630a35 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,26 +266,14 @@ extern void add_name_hash(struct index_state *istate, struct cache_entry *ce);
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);
#ifndef NO_THE_INDEX_COMPATIBILITY_MACROS
#define active_cache (the_index.cache)
@@ -443,6 +430,7 @@ extern struct cache_entry *index_name_exists(struct index_state *istate, const c
extern int unmerged_index(const struct index_state *);
extern int verify_path(const char *path);
extern struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int igncase);
+extern int index_icase_dir_exists(struct index_state *istate, const char *name, int namelen);
extern int index_name_pos(const struct index_state *, const char *name, int namelen);
#define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */
#define ADD_CACHE_OK_TO_REPLACE 2 /* Ok to replace file/directory */
diff --git a/dir.c b/dir.c
index 57394e4..f73ac34 100644
--- a/dir.c
+++ b/dir.c
@@ -927,29 +927,27 @@ static enum exist_status directory_exists_in_index_icase(const char *dirname, in
*/
static enum exist_status directory_exists_in_index_icase(const char *dirname, int len)
{
- struct cache_entry *ce = index_name_exists(&the_index, dirname, len + 1, ignore_case);
- unsigned char endchar;
-
- if (!ce)
- return index_nonexistent;
- endchar = ce->name[len];
+ struct cache_entry *ce = index_name_exists(&the_index, dirname, len, ignore_case);
/*
- * The cache_entry structure returned will contain this dirname
- * and possibly additional path components.
+ * We found something in the index, which means it is either an actual
+ * file, or a gitdir.
*/
- if (endchar == '/')
- return index_directory;
+ if (ce) {
+ if (S_ISGITLINK(ce->ce_mode))
+ return index_gitdir;
+ /* We call a file "index_nonexistent" here, because the caller is
+ * asking about a directory. */
+ return index_nonexistent;
+ }
/*
- * If there are no additional path components, then this cache_entry
- * represents a submodule. Submodules, despite being directories,
- * are stored in the cache without a closing slash.
+ * Otherwise, it might be a leading path of something that is in the
+ * index. We can look it up in the special dir hash.
*/
- if (!endchar && S_ISGITLINK(ce->ce_mode))
- return index_gitdir;
+ if (index_icase_dir_exists(&the_index, dirname, len))
+ return index_directory;
- /* This should never be hit, but it exists just in case. */
return index_nonexistent;
}
diff --git a/name-hash.c b/name-hash.c
index d8d25c2..de8239f 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -32,37 +32,88 @@ static void hash_index_entry_directories(struct index_state *istate, struct cach
return hash;
}
-static void hash_index_entry_directories(struct index_state *istate, struct cache_entry *ce)
+struct dir_hash_entry {
+ struct dir_hash_entry *next;
+ int nr;
+ unsigned int namelen;
+ char name[FLEX_ARRAY];
+};
+
+static struct dir_hash_entry *find_dir_hash(struct hash_table *t,
+ const char *name,
+ unsigned int namelen)
+{
+ unsigned int hash = hash_name(name, namelen);
+ struct dir_hash_entry *ent;
+
+ for (ent = lookup_hash(hash, t); ent; ent = ent->next) {
+ if (ent->namelen == namelen &&
+ !strncasecmp(ent->name, name, namelen))
+ return ent;
+ }
+ return NULL;
+}
+
+static struct dir_hash_entry *find_or_create_dir_hash(struct hash_table *t,
+ const char *name,
+ unsigned int namelen)
+{
+ struct dir_hash_entry *ent;
+
+ ent = find_dir_hash(t, name, namelen);
+ if (!ent) {
+ void **pos;
+
+ ent = xcalloc(sizeof(*ent) + namelen + 1, 1);
+ memcpy(ent->name, name, namelen);
+ ent->namelen = namelen;
+
+ pos = insert_hash(hash_name(name, namelen), ent, t);
+ if (pos) {
+ ent->next = *pos;
+ *pos = ent;
+ }
+ }
+
+ return ent;
+}
+
+static void hash_index_entry_directories(struct index_state *istate,
+ struct cache_entry *ce,
+ int add)
{
/*
- * Throw each directory component in the hash for quick lookup
+ * Throw each directory component into a 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.
- *
- * 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.
+ * in the cache. This means we don't need to know anything about
+ * what is stored at a particular directory, just that it is a leading
+ * directory component of something else. Which means we can get away
+ * with storing a count instead of a complete
*/
- unsigned int hash;
- void **pos;
-
const char *ptr = ce->name;
while (*ptr) {
while (*ptr && *ptr != '/')
++ptr;
if (*ptr == '/') {
- ++ptr;
- hash = hash_name(ce->name, ptr - ce->name);
- pos = insert_hash(hash, ce, &istate->name_hash);
- if (pos) {
- ce->dir_next = *pos;
- *pos = ce;
+ struct dir_hash_entry *ent;
+
+ if (add) {
+ ent = find_or_create_dir_hash(&istate->dir_hash,
+ ce->name,
+ ptr - ce->name);
+ ent->nr++;
+ }
+ else {
+ ent = find_dir_hash(&istate->dir_hash,
+ ce->name,
+ ptr - ce->name);
+ if (ent)
+ ent->nr--;
}
}
+ ptr++;
}
}
@@ -74,7 +125,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) {
@@ -83,7 +134,7 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
}
if (ignore_case)
- hash_index_entry_directories(istate, ce);
+ hash_index_entry_directories(istate, ce, 1);
}
static void lazy_init_name_hash(struct index_state *istate)
@@ -104,6 +155,22 @@ void add_name_hash(struct index_state *istate, struct cache_entry *ce)
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)
+{
+ ce->ce_flags |= CE_UNHASHED;
+ if (istate->dir_hash.nr)
+ hash_index_entry_directories(istate, ce, 0);
+}
+
static int slow_same_name(const char *name1, int len1, const char *name2, int len2)
{
if (len1 != len2)
@@ -137,18 +204,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
if (!icase)
return 0;
- /*
- * If the entry we're comparing is a filename (no trailing slash), then compare
- * the lengths exactly.
- */
- if (name[namelen - 1] != '/')
- return slow_same_name(name, namelen, ce->name, len);
-
- /*
- * For a directory, we point to an arbitrary cache_entry filename. Just
- * make sure the directory portion matches.
- */
- return slow_same_name(name, namelen, ce->name, namelen < len ? namelen : len);
+ return slow_same_name(name, namelen, ce->name, len);
}
struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int icase)
@@ -164,10 +220,7 @@ struct cache_entry *index_name_exists(struct index_state *istate, const char *na
if (same_name(ce, name, namelen, icase))
return ce;
}
- if (icase && name[namelen - 1] == '/')
- ce = ce->dir_next;
- else
- ce = ce->next;
+ ce = ce->next;
}
/*
@@ -188,3 +241,11 @@ struct cache_entry *index_name_exists(struct index_state *istate, const char *na
}
return NULL;
}
+
+int index_icase_dir_exists(struct index_state *istate, const char *name, int namelen)
+{
+ struct dir_hash_entry *ent;
+
+ ent = find_dir_hash(&istate->dir_hash, name, namelen);
+ return ent && ent->nr;
+}
diff --git a/read-cache.c b/read-cache.c
index 827ae55..116c25c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -46,7 +46,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache
{
struct cache_entry *old = istate->cache[nr];
- remove_name_hash(old);
+ remove_name_hash(istate, old);
set_index_entry(istate, nr, ce);
istate->cache_changed = 1;
}
@@ -460,7 +460,7 @@ int remove_index_entry_at(struct index_state *istate, int pos)
struct cache_entry *ce = istate->cache[pos];
record_resolve_undo(istate, ce);
- remove_name_hash(ce);
+ remove_name_hash(istate, ce);
istate->cache_changed = 1;
istate->cache_nr--;
if (pos >= istate->cache_nr)
@@ -483,7 +483,7 @@ void remove_marked_cache_entries(struct index_state *istate)
for (i = j = 0; i < istate->cache_nr; i++) {
if (ce_array[i]->ce_flags & CE_REMOVE)
- remove_name_hash(ce_array[i]);
+ remove_name_hash(istate, ce_array[i]);
else
ce_array[j++] = ce_array[i];
}
^ permalink raw reply related
* Re: [PATCH v4] submodule: add 'deinit' command
From: Jens Lehmann @ 2013-02-13 19:33 UTC (permalink / raw)
To: Phil Hord
Cc: Git Mailing List, Junio C Hamano, Heiko Voigt, Michael J Gruber,
Marc Branchaud, W. Trevor King
In-Reply-To: <CABURp0oQcPotK20QcqCG1pGQPVoa4RnN2nDA=iQoKS99gnPEAQ@mail.gmail.com>
Am 12.02.2013 18:11, schrieb Phil Hord:
> On Wed, Feb 6, 2013 at 4:11 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> + die_if_unmatched "$mode"
>> + name=$(module_name "$sm_path") || exit
>> + url=$(git config submodule."$name".url)
>> + if test -z "$url"
>> + then
>> + say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")"
>
> Is it safe to shelter the user a little bit more from the git
> internals here and say instead:
>
> Submodule '\$sm_path' is not initialized.
Yeah, that makes much more sense. But I'd rather use the name too:
Submodule '\$name' is not initialized for path '\$sm_path'
> Also, I think this code will show this message for each submodule on
> 'git submodule deinit .' But I think I would prefer to suppress it in
> that case. If I have not explicitly stated which submodules to
> deinit, then I do not think git should complain that some of them are
> not initialized.
Yes, that message gets printed for each uninitialized submodule. We
could easily suppress that for '.', but it would be really hard to
get that right for other wildcards like 'foo*'. (And e.g. running a
"submodule update" also lists all submodules it skips because of an
"update=none" setting, so I'm not sure if it's that important here)
But if people really want that, I'd suppress that for the '.' case.
Further opinions?
>> + continue
>> + fi
>> +
>> + # Remove the submodule work tree (unless the user already did it)
>> + if test -d "$sm_path"
>> + then
>> + # Protect submodules containing a .git directory
>> + if test -d "$sm_path/.git"
>> + then
>> + echo >&2 "$(eval_gettext "Submodule work tree $sm_path contains a .git directory")"
>> + die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")"
>
> I expect this is the right thing to do for now. But I wonder if we
> can also move $sm_path/.git to $GIT_DIR/modules/$sm_path in this case
> (though I think I am not spelling this path correctly). Would that be
> ok? What extra work is needed to relocate the .git dir like this?
Hmm, I'm a bit torn about automagically moving the repo somewhere
else. While I think it is a sane solution for most users I suspect
some users might hate us for doing that without asking (and with no
option to turn that off). What about adding a separate "git submodule
to-gitfile" command which does that and hinting that here? However I
do have the feeling that this should be done in another commit.
>> + die "$(eval_gettext "Submodule work tree $sm_path contains local modifications, use '-f' to discard them")"
>
> Nit, grammar: use a semicolon instead of a comma.
Ok. And while looking at it I noticed that "$sm_path" should be
"'\$sm_path'" here, same a few lines above ... sigh
>> +test_expect_success 'set up a second submodule' '
>> + git submodule add ./init2 example2 &&
>> + git commit -m "submodle example2 added"
>
> Nit: submodule is misspelled
Thanks.
Junio, this looks like a we have v5 as soon as we decide what to do
with the "not initialized" messages when '.' is used, right?
My current diff to v4 looks like this:
-------------8<-------------
diff --git a/git-submodule.sh b/git-submodule.sh
index f1b552f..27f8e12 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -591,7 +591,7 @@ cmd_deinit()
url=$(git config submodule."$name".url)
if test -z "$url"
then
- say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")"
+ say "$(eval_gettext "Submodule '\$name' is not initialized for path '\$sm_path'")"
continue
fi
@@ -601,14 +601,14 @@ cmd_deinit()
# Protect submodules containing a .git directory
if test -d "$sm_path/.git"
then
- echo >&2 "$(eval_gettext "Submodule work tree $sm_path contains a .git directory")"
+ echo >&2 "$(eval_gettext "Submodule work tree '\$sm_path' contains a .git directory")"
die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")"
fi
if test -z "$force"
then
git rm -n "$sm_path" ||
- die "$(eval_gettext "Submodule work tree $sm_path contains local modifications, use '-f' to discard them")"
+ die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")"
fi
rm -rf "$sm_path" || say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
fi
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index f54a40d..e4b0a59 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -759,7 +759,7 @@ test_expect_success 'submodule add with an existing name fails unless forced' '
test_expect_success 'set up a second submodule' '
git submodule add ./init2 example2 &&
- git commit -m "submodle example2 added"
+ git commit -m "submodule example2 added"
'
test_expect_success 'submodule deinit should remove the whole submodule section from .git/config' '
@@ -837,7 +837,7 @@ test_expect_success 'submodule deinit complains but does not fail when used on a
git submodule deinit init >actual &&
test_i18ngrep "Submodule .example. (.*) unregistered for path .init" actual
git submodule deinit init >actual &&
- test_i18ngrep "No url found for submodule path .init. in .git/config" actual &&
+ test_i18ngrep "Submodule .example. is not initialized for path .init" actual &&
git submodule deinit . >actual &&
test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual
rmdir init example2
^ permalink raw reply related
* Re: inotify to minimize stat() calls
From: Jeff King @ 2013-02-13 18:18 UTC (permalink / raw)
To: Duy Nguyen
Cc: blees, kusmabite, Ramkumar Ramachandra, Robert Zeh,
Junio C Hamano, Git List, finnag
In-Reply-To: <CACsJy8C=2xKcsby048WWCFNhgKObGwrzeCOJPVVqgj88AfSHQw@mail.gmail.com>
On Wed, Feb 13, 2013 at 07:15:47PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Wed, Feb 13, 2013 at 3:48 AM, Karsten Blees <karsten.blees@gmail.com> wrote:
> > 2.) 0.135 s is spent in name-hash.c/hash_index_entry_directories, reindexing the same directories over and over again. In the end, the hashtable contains 939k directory entries, even though the WebKit test repo only has 7k directories. Checking if a directory entry already exists could reduce that, i.e.:
>
> This function is only used when core.ignorecase = true. I probably
> won't be able to test this, so I'll leave this to other people who
> care about ignorecase.
>
> This function used to have lookup_hash, but it was removed by Jeff in
> 2548183 (fix phantom untracked files when core.ignorecase is set -
> 2011-10-06). There's a looong commit message which I'm too lazy to
> read. Anybody who works on this should though.
Yeah, the problem that commit tried to solve is that linking to a single
cache entry through the hash is not enough, because we may remove cache
items. Imagine you have "dir/one" and "dir/two", and you add them to the
in-memory index in that order. The original code hashed "dir/" and
inserted a link to the "dir/one" cache entry. When it came time to put
in the "dir/two" entry, we noticed that there was already a "dir/" entry
and did nothing. Then later, if we remove "dir/one", we do so by marking
it with CE_UNHASHED. So a later query for "dir/" will see "nope, nothing
here that wasn't CE_UNHASHED", which is wrong. We never recorded that
"dir/two" existed under the hash for "dir/", so we can't know about it.
My patch just stores the cache_entry for both under the "dir/" hash.
As Karsten noticed, that can lead to a large number of hash entries,
because adding "some/deep/hierarchy/with/files" will add 4 directory
entries for just that single file. Moreover, looking at it again, I
don't think my patch produces the right behavior: we have a single
dir_next pointer, even though the same ce_entry may appear under many
directory hashes. So the cache_entries that has to "dir/foo/" and those
that hash to "dir/bar/" may get confused, because they will also both be
found under "dir/", and both try to create a linked list from the
dir_next pointer.
Looking at Karsten's patch, it seems like it will not add a cache entry
if there is one of the same name. But I'm not sure if that is right, as
the old one might be CE_UNHASHED (or it might get removed later). You
actually want to be able to find each cache_entry that has a file under
the directory at the hash of that directory, so you can make sure it is
still valid.
And of course that still leaves the existing correctness problem I
mentioned above.
I think the best way forward is to actually create a separate hash table
for the directory lookups. I note that we only care about these entries
in directory_exists_in_index_icase, which is really about whether
something is there, versus what exactly is there. So could we maybe get
by with a separate hash table that stores a count of entries at each
directory, and increment/decrement the count when we add/remove entries?
The biggest problem I see with that is that we do indeed care a little
bit what is at the directory: we check the mode to see if it is a gitdir
or not. But I think we can maybe sneak around that: gitdirs have actual
entries in the index, whereas the directories do not. So we would find
them via index_name_exists; anything that is not there, but _is_ in the
special directory hash would therefore be a directory.
I realize it got pretty esoteric there in the middle. I'll see if I can
work up a patch that expresses what I'm thinking.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState
From: Martin Erik Werner @ 2013-02-13 17:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, trsten
In-Reply-To: <7vy5es9o0g.fsf@alter.siamese.dyndns.org>
On Wed, 2013-02-13 at 08:28 -0800, Junio C Hamano wrote:
> Martin Erik Werner <martinerikwerner@gmail.com> writes:
>
> > Added 3 extra tests for the bash.showDirtyState config option, tests
> > should now cover all combinations of the shell var being set/unset and
> > the config option being enabled/disabled, given a dirty file.
>
> Strictly speaking, you have 6 not 4 combinations (shell variable
> set/unset * config missing/set to false/set to true). I think these
> additional tests cover should all 6 because "config missing" case
> should already have had tests before bash.showDirtyState was added.
>
Indeed, I only mentioned 4 since the other ones existed already, and I
didn't change them, but maybe it should be mentioned as "combined with
previous tests (...) cover all 6 combinations (...)" then?
> > * Renamed test 'disabled by config' to 'shell variable set with config
> > disabled' for consistency
> > ---
>
> Sign-off?
Ah, just forgot the -s flag on that commit, yes it should be Signed-off
by me.
>
> > t/t9903-bash-prompt.sh | 38 +++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> > index cb008e2..fa81b09 100755
> > --- a/t/t9903-bash-prompt.sh
> > +++ b/t/t9903-bash-prompt.sh
> > @@ -360,12 +360,48 @@ test_expect_success 'prompt - dirty status indicator - before root commit' '
> > test_cmp expected "$actual"
> > '
> >
> > -test_expect_success 'prompt - dirty status indicator - disabled by config' '
> > +test_expect_success 'prompt - dirty status indicator - shell variable unset with config disabled' '
> > printf " (master)" > expected &&
> > echo "dirty" > file &&
> > test_when_finished "git reset --hard" &&
> > test_config bash.showDirtyState false &&
> > (
> > + unset -v GIT_PS1_SHOWDIRTYSTATE &&
> > + __git_ps1 > "$actual"
> > + ) &&
> > + test_cmp expected "$actual"
> > +'
> > +
> > +test_expect_success 'prompt - dirty status indicator - shell variable unset with config enabled' '
> > + printf " (master)" > expected &&
> > + echo "dirty" > file &&
> > + test_when_finished "git reset --hard" &&
> > + test_config bash.showDirtyState true &&
> > + (
> > + unset -v GIT_PS1_SHOWDIRTYSTATE &&
> > + __git_ps1 > "$actual"
> > + ) &&
> > + test_cmp expected "$actual"
> > +'
> > +
> > +test_expect_success 'prompt - dirty status indicator - shell variable set with config disabled' '
> > + printf " (master)" > expected &&
> > + echo "dirty" > file &&
> > + test_when_finished "git reset --hard" &&
> > + test_config bash.showDirtyState false &&
> > + (
> > + GIT_PS1_SHOWDIRTYSTATE=y &&
> > + __git_ps1 > "$actual"
> > + ) &&
> > + test_cmp expected "$actual"
> > +'
> > +
> > +test_expect_success 'prompt - dirty status indicator - shell variable set with config enabled' '
> > + printf " (master *)" > expected &&
> > + echo "dirty" > file &&
> > + test_when_finished "git reset --hard" &&
> > + test_config bash.showDirtyState true &&
> > + (
> > GIT_PS1_SHOWDIRTYSTATE=y &&
> > __git_ps1 > "$actual"
> > ) &&
^ permalink raw reply
* Re: [PATCH v2 2/3] t9903: add tests for bash.showUntrackedFiles
From: Martin Erik Werner @ 2013-02-13 17:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, trsten
In-Reply-To: <7v621wb2sp.fsf@alter.siamese.dyndns.org>
On Wed, 2013-02-13 at 08:23 -0800, Junio C Hamano wrote:
> Martin Erik Werner <martinerikwerner@gmail.com> writes:
>
> > Add 4 test for the bash.showUntrackedFiles config option, covering all
> > combinations of the shell var being set/unset and the config option
> > being enabled/disabled.
> >
> > Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> > ---
> > t/t9903-bash-prompt.sh | 40 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> > index f17c1f8..cb008e2 100755
> > --- a/t/t9903-bash-prompt.sh
> > +++ b/t/t9903-bash-prompt.sh
> > @@ -437,6 +437,46 @@ test_expect_success 'prompt - untracked files status indicator - untracked files
> > test_cmp expected "$actual"
> > '
> >
> > +test_expect_success 'prompt - untracked files status indicator - shell variable unset with config disabled' '
> > + printf " (master)" > expected &&
> > + test_config bash.showUntrackedFiles false &&
> > + (
> > + unset -v GIT_PS1_SHOWUNTRACKEDFILES &&
>
> We do not use "unset -v" anywhere else in our system. Shells
> mimicking SysV may choke on it. A Portable POSIX script can omit
> "-v" when unsetting a variable.
>
> Also "unset" can return false when the variable is not set to begin
> with with some shells.
>
> Neither of these matters for this particular case because we know we
> are running this under bash in non-posix mode. I however wonder if
> we can do something to prevent careless coders to copy and paste
> this piece when updating other tests that are not limited to bash.
> Commenting each and every use of "unset -v" does not sound like a
> good solution and perhaps I am being unnecessarily worried too much.
>
Yeah, my (ba)sh foo is a bit limited, I was just basing on
http://wiki.bash-hackers.org/commands/builtin/unset#portability_considerations which seemed to recommend using -v.
So would it make sense to do:
GIT_PS1_SHOWUNTRACKEDFILES="dummy" &&
unset GIT_PS1_SHOWUNTRACKEDFILES &&
(...)
instead then?
--
Martin Erik Werner <martinerikwerner@gmail.com>
^ permalink raw reply
* Re: [PATCH] Makefile: don't run rm without any files
From: Matt Kraai @ 2013-02-13 17:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder, Matt Kraai
In-Reply-To: <7vtxpg9mxq.fsf@alter.siamese.dyndns.org>
On Wed, Feb 13, 2013 at 08:51:45AM -0800, Junio C Hamano wrote:
> Matt Kraai <kraai@ftbfs.org> writes:
>
> > From: Matt Kraai <matt.kraai@amo.abbott.com>
> >
> > "rm -f -r" fails on QNX when not passed any files to remove.
>
> I do not think it is limited to QNX.
>
> > the clean target, since dep_dirs is empty.
>
> And dep_dirs being empty under some circumstance shouldn't be
> limited to QNX, either.
>
> I think your change does no harm, may be a good change if dep_dirs
> goes empty, but the justification is lacking. What caused your
> dep_dirs to become empty in the first place?
>
> I am scratching my head because I see
>
> OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
> $(XDIFF_OBJS) \
> $(VCSSVN_OBJS) \
> git.o
> dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
I don't set COMPUTE_HEADER_DEPENDENCIES, so it defaults to "auto".
The automatic detection determines that the compiler doesn't support
it, so it's then set to "no". CHECK_HEADER_DEPENDENCIES isn't set
either, so about 20 lines below the dep_dirs assignment you quoted,
dep_dirs is cleared:
ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
ifndef CHECK_HEADER_DEPENDENCIES
dep_dirs =
...
Should I submit an updated patch with a different commit message?
^ permalink raw reply
* Re: [PATCH v4 3/4] count-objects: report garbage files in pack directory too
From: Junio C Hamano @ 2013-02-13 16:54 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <7vehgkb43v.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> + if (has_extension(de->d_name, ".idx") ||
>> + has_extension(de->d_name, ".pack") ||
>> + has_extension(de->d_name, ".keep"))
>> + string_list_append(&garbage, path);
>
> It might be OK to put .pack and .keep in the same "if (A || B)" as
> it may happen to be that they do not need any special treatment
> right now, but I do not think this is a good idea in general.
Actually I take this part back. I can see that the condition part
grow over time but I do not think the body should. That is the
whole point of collecting paths that cannot be judged as garbage by
themselves into a list; we shouldn't be doing anything else by
definition in the body.
Everything else I said in the review still stands, though..
Thanks.
^ permalink raw reply
* Re: [PATCH] Makefile: don't run rm without any files
From: Junio C Hamano @ 2013-02-13 16:51 UTC (permalink / raw)
To: Matt Kraai; +Cc: git, Jonathan Nieder, Matt Kraai
In-Reply-To: <1360771068-505-1-git-send-email-kraai@ftbfs.org>
Matt Kraai <kraai@ftbfs.org> writes:
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> "rm -f -r" fails on QNX when not passed any files to remove.
I do not think it is limited to QNX.
> the clean target, since dep_dirs is empty.
And dep_dirs being empty under some circumstance shouldn't be
limited to QNX, either.
I think your change does no harm, may be a good change if dep_dirs
goes empty, but the justification is lacking. What caused your
dep_dirs to become empty in the first place?
I am scratching my head because I see
OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
$(XDIFF_OBJS) \
$(VCSSVN_OBJS) \
git.o
dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
> Avoid this by merging two rm
> command lines.
>
> Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
> ---
> Makefile | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 5a2e02d..c2e3666 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2414,8 +2414,7 @@ clean: profile-clean
> builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
> $(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
> $(RM) $(TEST_PROGRAMS)
> - $(RM) -r bin-wrappers
> - $(RM) -r $(dep_dirs)
> + $(RM) -r bin-wrappers $(dep_dirs)
> $(RM) -r po/build/
> $(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*
> $(RM) -r $(GIT_TARNAME) .doc-tmp-dir
^ permalink raw reply
* Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState
From: Junio C Hamano @ 2013-02-13 16:28 UTC (permalink / raw)
To: Martin Erik Werner; +Cc: git, trsten
In-Reply-To: <1360753321-5002-3-git-send-email-martinerikwerner@gmail.com>
Martin Erik Werner <martinerikwerner@gmail.com> writes:
> Added 3 extra tests for the bash.showDirtyState config option, tests
> should now cover all combinations of the shell var being set/unset and
> the config option being enabled/disabled, given a dirty file.
Strictly speaking, you have 6 not 4 combinations (shell variable
set/unset * config missing/set to false/set to true). I think these
additional tests cover should all 6 because "config missing" case
should already have had tests before bash.showDirtyState was added.
> * Renamed test 'disabled by config' to 'shell variable set with config
> disabled' for consistency
> ---
Sign-off?
> t/t9903-bash-prompt.sh | 38 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index cb008e2..fa81b09 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -360,12 +360,48 @@ test_expect_success 'prompt - dirty status indicator - before root commit' '
> test_cmp expected "$actual"
> '
>
> -test_expect_success 'prompt - dirty status indicator - disabled by config' '
> +test_expect_success 'prompt - dirty status indicator - shell variable unset with config disabled' '
> printf " (master)" > expected &&
> echo "dirty" > file &&
> test_when_finished "git reset --hard" &&
> test_config bash.showDirtyState false &&
> (
> + unset -v GIT_PS1_SHOWDIRTYSTATE &&
> + __git_ps1 > "$actual"
> + ) &&
> + test_cmp expected "$actual"
> +'
> +
> +test_expect_success 'prompt - dirty status indicator - shell variable unset with config enabled' '
> + printf " (master)" > expected &&
> + echo "dirty" > file &&
> + test_when_finished "git reset --hard" &&
> + test_config bash.showDirtyState true &&
> + (
> + unset -v GIT_PS1_SHOWDIRTYSTATE &&
> + __git_ps1 > "$actual"
> + ) &&
> + test_cmp expected "$actual"
> +'
> +
> +test_expect_success 'prompt - dirty status indicator - shell variable set with config disabled' '
> + printf " (master)" > expected &&
> + echo "dirty" > file &&
> + test_when_finished "git reset --hard" &&
> + test_config bash.showDirtyState false &&
> + (
> + GIT_PS1_SHOWDIRTYSTATE=y &&
> + __git_ps1 > "$actual"
> + ) &&
> + test_cmp expected "$actual"
> +'
> +
> +test_expect_success 'prompt - dirty status indicator - shell variable set with config enabled' '
> + printf " (master *)" > expected &&
> + echo "dirty" > file &&
> + test_when_finished "git reset --hard" &&
> + test_config bash.showDirtyState true &&
> + (
> GIT_PS1_SHOWDIRTYSTATE=y &&
> __git_ps1 > "$actual"
> ) &&
^ permalink raw reply
* Re: [PATCH v2 2/3] t9903: add tests for bash.showUntrackedFiles
From: Junio C Hamano @ 2013-02-13 16:23 UTC (permalink / raw)
To: Martin Erik Werner; +Cc: git, trsten
In-Reply-To: <1360753321-5002-2-git-send-email-martinerikwerner@gmail.com>
Martin Erik Werner <martinerikwerner@gmail.com> writes:
> Add 4 test for the bash.showUntrackedFiles config option, covering all
> combinations of the shell var being set/unset and the config option
> being enabled/disabled.
>
> Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> ---
> t/t9903-bash-prompt.sh | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index f17c1f8..cb008e2 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -437,6 +437,46 @@ test_expect_success 'prompt - untracked files status indicator - untracked files
> test_cmp expected "$actual"
> '
>
> +test_expect_success 'prompt - untracked files status indicator - shell variable unset with config disabled' '
> + printf " (master)" > expected &&
> + test_config bash.showUntrackedFiles false &&
> + (
> + unset -v GIT_PS1_SHOWUNTRACKEDFILES &&
We do not use "unset -v" anywhere else in our system. Shells
mimicking SysV may choke on it. A Portable POSIX script can omit
"-v" when unsetting a variable.
Also "unset" can return false when the variable is not set to begin
with with some shells.
Neither of these matters for this particular case because we know we
are running this under bash in non-posix mode. I however wonder if
we can do something to prevent careless coders to copy and paste
this piece when updating other tests that are not limited to bash.
Commenting each and every use of "unset -v" does not sound like a
good solution and perhaps I am being unnecessarily worried too much.
^ permalink raw reply
* Re: [PATCH v2 1/3] shell prompt: add bash.showUntrackedFiles option
From: Junio C Hamano @ 2013-02-13 16:12 UTC (permalink / raw)
To: Martin Erik Werner; +Cc: git, trsten
In-Reply-To: <1360753321-5002-1-git-send-email-martinerikwerner@gmail.com>
Martin Erik Werner <martinerikwerner@gmail.com> writes:
> Add a config option 'bash.showUntrackedFiles' which allows enabling
> the prompt showing untracked files on a per-repository basis. This is
> useful for some repositories where the 'git ls-files ...' command may
> take a long time.
>
> Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> ---
> contrib/completion/git-prompt.sh | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 9bef053..9b2eec2 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -43,7 +43,10 @@
> #
> # If you would like to see if there're untracked files, then you can set
> # GIT_PS1_SHOWUNTRACKEDFILES to a nonempty value. If there're untracked
> -# files, then a '%' will be shown next to the branch name.
> +# files, then a '%' will be shown next to the branch name. You can
> +# configure this per-repository with the bash.showUntrackedFiles
> +# variable, which defaults to true once GIT_PS1_SHOWUNTRACKEDFILES is
> +# enabled.
> #
> # If you would like to see the difference between HEAD and its upstream,
> # set GIT_PS1_SHOWUPSTREAM="auto". A "<" indicates you are behind, ">"
> @@ -332,8 +335,10 @@ __git_ps1 ()
> fi
>
> if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
> - if [ -n "$(git ls-files --others --exclude-standard)" ]; then
> - u="%"
> + if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
> + if [ -n "$(git ls-files --others --exclude-standard)" ]; then
> + u="%"
> + fi
> fi
> fi
Somebody should simplify this deeply nested "if/then/if/then/fi/fi"
sequence to a single if/then/fi statement, i.e. something like:
if test -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" &&
test "$(git config --bool bash.showUntrackedFiles)" != false
then
u='%'
fi
And do the same for the other one this patch copies the above from.
No need to re-roll this patch, though. It is a separate clean-up to
be done on top, once this series is settles.
Thanks.
^ permalink raw reply
* Re: [RFC v2] git-multimail: a replacement for post-receive-email
From: Matthieu Moy @ 2013-02-13 16:12 UTC (permalink / raw)
To: Andy Parkins
Cc: Michael Haggerty, git discussion list, Sitaram Chamarty,
Junio C Hamano, Marc Branchaud,
Ævar Arnfjörð Bjarmason, Chris Hiestand
In-Reply-To: <201302131526.57342.andyparkins@gmail.com>
Andy Parkins <andyparkins@gmail.com> writes:
> On Wednesday 13 February 2013 14:56:25 Matthieu Moy wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> I think adding a short "dependencies" section in the README (or in an
>> INSTALL file) saying which Python version works could save new users the
>> trouble (I see the sheebang inside the scripts says python2 but since I
>> couldn't use my system's python and called
>> "path/to/python git_multimail.py", this didn't help). Making the script
>> portable with python 2 and 3 would be awesome ;-).
>
> For my 2p worth, I don't like seeing hooks called like this. Particular those
> that come as part of the standard installation.
What do you mean by "like this" ?
> I call mine by installing little scripts like this (on Debian):
>
> #!/bin/sh
> # stored as $GIT_WORK_DIR/.git/hooks/post-receive-email
> exec /bin/sh /usr/share/git-core/contrib/hooks/post-receive-email
Yes, this is what I was doing (with path/to/python instead of /bin/sh,
and git_multimail.py, or more precisely path/to/git_multimail.py,
instead of post-receive-email).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH] Makefile: don't run rm without any files
From: Matt Kraai @ 2013-02-13 15:57 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Matt Kraai
From: Matt Kraai <matt.kraai@amo.abbott.com>
"rm -f -r" fails on QNX when not passed any files to remove. This breaks
the clean target, since dep_dirs is empty. Avoid this by merging two rm
command lines.
Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
Makefile | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 5a2e02d..c2e3666 100644
--- a/Makefile
+++ b/Makefile
@@ -2414,8 +2414,7 @@ clean: profile-clean
builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
$(RM) $(TEST_PROGRAMS)
- $(RM) -r bin-wrappers
- $(RM) -r $(dep_dirs)
+ $(RM) -r bin-wrappers $(dep_dirs)
$(RM) -r po/build/
$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
--
1.8.1.3.570.g3074c9d
^ permalink raw reply related
* Re: [PATCH v4 3/4] count-objects: report garbage files in pack directory too
From: Junio C Hamano @ 2013-02-13 15:55 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1360746799-3668-4-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> prepare_packed_git_one() is modified to allow count-objects to hook a
> report function to so we don't need to duplicate the pack searching
> logic in count-objects.c. When report_pack_garbage is NULL, the
> overhead is insignificant.
>
> The garbage is reported with warning() instead of error() in packed
> garbage case because it's not an error to have garbage. Loose garbage
> is still reported as errors and will be converted to warnings later.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
Thanks.
Tests look good and the series is getting much closer.
> diff --git a/sha1_file.c b/sha1_file.c
> index 239bee7..5bedf78 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -21,6 +21,7 @@
> #include "sha1-lookup.h"
> #include "bulk-checkin.h"
> #include "streaming.h"
> +#include "dir.h"
>
> #ifndef O_NOATIME
> #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
> @@ -1000,6 +1001,57 @@ void install_packed_git(struct packed_git *pack)
> packed_git = pack;
> }
>
> +void (*report_garbage)(const char *desc, const char *path);
> +
> +static void report_helper(const struct string_list *list,
> + int seen_bits, int first, int last)
> +{
> + const char *msg;
> + switch (seen_bits) {
> + case 0: msg = "no corresponding .idx nor .pack"; break;
> + case 1: msg = "no corresponding .idx"; break;
> + case 2: msg = "no corresponding .pack"; break;
That's dense.
> + default:
> + return;
> + }
> + for (; first <= last; first++)
This looks odd. If you use the usual last+1 convention between the
caller and callee, you do not have to do this, or call this function
with "i - 1" and "list->nr -1" as the last parameter.
> +static void report_pack_garbage(struct string_list *list)
> +{
> + int i, baselen = -1, first = 0, seen_bits = 0;
> +
> + if (!report_garbage)
> + return;
> +
> + sort_string_list(list);
> +
> + for (i = 0; i < list->nr; i++) {
> + const char *path = list->items[i].string;
> + if (baselen != -1 &&
> + strncmp(path, list->items[first].string, baselen)) {
> + report_helper(list, seen_bits, first, i - 1);
> + baselen = -1;
> + seen_bits = 0;
> + }
> + if (baselen == -1) {
> + const char *dot = strrchr(path, '.');
> + if (!dot) {
> + report_garbage("garbage found", path);
> + continue;
> + }
> + baselen = dot - path + 1;
> + first = i;
> + }
> + if (!strcmp(path + baselen, "pack"))
> + seen_bits |= 1;
> + else if (!strcmp(path + baselen, "idx"))
> + seen_bits |= 2;
> + }
> + report_helper(list, seen_bits, first, list->nr - 1);
> +}
> @@ -1009,6 +1061,7 @@ static void prepare_packed_git_one(char *objdir, int local)
> int len;
> DIR *dir;
> struct dirent *de;
> + struct string_list garbage = STRING_LIST_INIT_DUP;
>
> sprintf(path, "%s/pack", objdir);
> len = strlen(path);
> ...
> @@ -1043,8 +1106,20 @@ static void prepare_packed_git_one(char *objdir, int local)
> (p = add_packed_git(path, len + namelen, local)) != NULL)
> install_packed_git(p);
> }
> +
> + if (!report_garbage)
> + continue;
> +
> + if (has_extension(de->d_name, ".idx") ||
> + has_extension(de->d_name, ".pack") ||
> + has_extension(de->d_name, ".keep"))
> + string_list_append(&garbage, path);
It might be OK to put .pack and .keep in the same "if (A || B)" as
it may happen to be that they do not need any special treatment
right now, but I do not think this is a good idea in general.
You would want to do things differently for ".idx", e.g.
diff --git a/sha1_file.c b/sha1_file.c
index 5bedf78..450521f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1076,6 +1076,7 @@ static void prepare_packed_git_one(char *objdir, int local)
while ((de = readdir(dir)) != NULL) {
int namelen = strlen(de->d_name);
struct packed_git *p;
+ int is_a_bad_idx = 0;
if (len + namelen + 1 > sizeof(path)) {
if (report_garbage) {
@@ -1105,12 +1106,14 @@ static void prepare_packed_git_one(char *objdir, int local)
*/
(p = add_packed_git(path, len + namelen, local)) != NULL)
install_packed_git(p);
+ else
+ is_a_bad_idx = 1;
}
if (!report_garbage)
continue;
- if (has_extension(de->d_name, ".idx") ||
+ if ((has_extension(de->d_name, ".idx") && !is_a_bad_idx) ||
has_extension(de->d_name, ".pack") ||
has_extension(de->d_name, ".keep"))
string_list_append(&garbage, path);
so that you can say something about .pack/.keep files that do not
have a working .idx file. In the above example, the only special
thing you would do for .idx is just to check if it is a bad one, but
in later patches you may have to do different things in the body
(i.e. something else in addition to string_list_append(&garbage))
not just in the condition. Collapsing these into a condition to a
single "if (A||B||C)" may be suffering from a lack of foresight.
> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
> index d645328..e4bb3a1 100755
> --- a/t/t5304-prune.sh
> +++ b/t/t5304-prune.sh
> @@ -195,4 +195,30 @@ test_expect_success 'gc: prune old objects after local clone' '
> )
> '
>
> +test_expect_success 'garbage report in count-objects -v' '
> + : >.git/objects/pack/foo &&
> + : >.git/objects/pack/foo.bar &&
> + : >.git/objects/pack/foo.keep &&
> + : >.git/objects/pack/foo.pack &&
> + : >.git/objects/pack/fake.bar &&
> + : >.git/objects/pack/fake.keep &&
> + : >.git/objects/pack/fake.pack &&
> + : >.git/objects/pack/fake.idx &&
> + : >.git/objects/pack/fake2.keep &&
> + : >.git/objects/pack/fake3.idx &&
> + git count-objects -v 2>stderr &&
> + grep "index file .git/objects/pack/fake.idx is too small" stderr &&
The above suggested change will make a difference to
fake.{pack,keep} because of this breakage, I think.
> + grep "^warning:" stderr | sort >actual &&
> + cat >expected <<\EOF &&
> +warning: garbage found: .git/objects/pack/fake.bar
> +warning: garbage found: .git/objects/pack/foo
> +warning: garbage found: .git/objects/pack/foo.bar
> +warning: no corresponding .idx nor .pack: .git/objects/pack/fake2.keep
> +warning: no corresponding .idx: .git/objects/pack/foo.keep
> +warning: no corresponding .idx: .git/objects/pack/foo.pack
> +warning: no corresponding .pack: .git/objects/pack/fake3.idx
> +EOF
> + test_cmp expected actual
> +'
> +
> test_done
^ permalink raw reply related
* Re: Git-aware HTTP transport docs
From: Junio C Hamano @ 2013-02-13 15:29 UTC (permalink / raw)
To: Scott Chacon; +Cc: H. Peter Anvin, Shawn O. Pearce, git list
In-Reply-To: <CAP2yMaLz=vpOVgpxG0CwVwWD_sq+T9px3w0KXE7doUFhKqNZWQ@mail.gmail.com>
Scott Chacon <schacon@gmail.com> writes:
> I don't believe it was ever merged into the Git docs. I have a copy of it here:
>
> https://www.dropbox.com/s/pwawp8kmwgyc3w2/http-protocol.txt
Thanks for a pointer. It seems that it wasn't in a shape ready to
be "merged" yet.
Does somebody want to pick it up and polish it further?
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2013, #05; Tue, 12)
From: Junio C Hamano @ 2013-02-13 15:27 UTC (permalink / raw)
To: Andrew Ardill; +Cc: git@vger.kernel.org
In-Reply-To: <CAH5451mmXg=xvb-gW0qNvp7f8M5Jk5_ZS+UHAzMaGhJ677zWmw@mail.gmail.com>
Andrew Ardill <andrew.ardill@gmail.com> writes:
> On 13 February 2013 11:34, Junio C Hamano <gitster@pobox.com> wrote:
>> The change could negatively affect people who expect that removing
>> files that are not used for their purpose (e.g. a large file that is
>> unnecessary for their build) will _not_ affect what they get from
>> "git add .";
>
> How big a problem is this?
As you said below, it could be fairly big, if you expect a lot of
people do not use "git add -u".
> If we need to support this behaviour than I would suppose a config
> option is required. A default config transition path similar to git
> push defaults would probably work well, in the case where breaking
> these expectations is unacceptable.
We've discussed that before.
http://thread.gmane.org/gmane.comp.version-control.git/171811/focus=171818
>> obviously they must have trained themselves not to do
>> "git add -u" or "git commit -a".
>
> Many people use git add -p by default, so I would not be surprised
> about people not using -u or -a.
^ permalink raw reply
* Re: [RFC v2] git-multimail: a replacement for post-receive-email
From: Andy Parkins @ 2013-02-13 15:26 UTC (permalink / raw)
To: Matthieu Moy
Cc: Michael Haggerty, git discussion list, Sitaram Chamarty,
Junio C Hamano, Marc Branchaud,
Ævar Arnfjörð Bjarmason, Chris Hiestand
In-Reply-To: <vpqtxpgb6ue.fsf@grenoble-inp.fr>
On Wednesday 13 February 2013 14:56:25 Matthieu Moy wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> I think adding a short "dependencies" section in the README (or in an
> INSTALL file) saying which Python version works could save new users the
> trouble (I see the sheebang inside the scripts says python2 but since I
> couldn't use my system's python and called
> "path/to/python git_multimail.py", this didn't help). Making the script
> portable with python 2 and 3 would be awesome ;-).
For my 2p worth, I don't like seeing hooks called like this. Particular those
that come as part of the standard installation.
I call mine by installing little scripts like this (on Debian):
#!/bin/sh
# stored as $GIT_WORK_DIR/.git/hooks/post-receive-email
exec /bin/sh /usr/share/git-core/contrib/hooks/post-receive-email
This means I don't have to make the sample script executable, it gets upgraded
automatically as git gets upgraded, and the interpreter is easily changed by
changing a file in my work directory, rather than altering a packaged file.
I'd prefer to see the /usr/share/git-core/templates/hooks/ using a similar
technique, as to my mind, installing a full copy of the sample script in every
new repository is wasteful and leaves you with potentially out-of-date scripts
when you update git.
Andy
--
Dr Andy Parkins
andyparkins@gmail.com
^ permalink raw reply
* Re: [RFC v2] git-multimail: a replacement for post-receive-email
From: Matthieu Moy @ 2013-02-13 14:56 UTC (permalink / raw)
To: Michael Haggerty
Cc: git discussion list, Andy Parkins, Sitaram Chamarty,
Junio C Hamano, Marc Branchaud,
Ævar Arnfjörð Bjarmason, Chris Hiestand
In-Reply-To: <5104E738.602@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> A while ago, I submitted an RFC for adding a new email notification
> script to "contrib" [1]. The reaction seemed favorable and it was
> suggested that the new script should replace post-receive-email rather
> than be added separately, ideally with some kind of migration support.
I think replacing the old post-receive-email is a sane goal in the long
run, but a good first step would be to have git-multimail merged in
contrib, and start considering the old script as deprecated (keeping the
old script doesn't harm IMHO, it's a one-file, 3 commits/year script,
not really a maintainance burden).
I started playing with git-multimail. In short, I do like it but had to
fight a bit with python to get it to work, and couldn't get it to do
exactly what I expect. Pull request attached :-).
Installation troubles:
I had an old python installation (Red Hat package, and I'm not root),
that did not include the email.utils package, so I couldn't use my
system's python. I found no indication about python version in README,
so I installed the latest python by hand, just to find out that
git-multimail wasn't compatible with Python 3.x. 2to3 can fix
automatically a number of 3.x compatibility issues, but not all of them
so I gave up and installed Python 2.7.
I think adding a short "dependencies" section in the README (or in an
INSTALL file) saying which Python version works could save new users the
trouble (I see the sheebang inside the scripts says python2 but since I
couldn't use my system's python and called
"path/to/python git_multimail.py", this didn't help). Making the script
portable with python 2 and 3 would be awesome ;-).
Missing feature:
git-multimail can send a summary for each push, with the "git log --oneline"
of the new revisions, and then 1 mail per patch with the complete log
and the patch.
I'd like to have the intermediate: allow the summary email to include
the complete log (not just --oneline). My colleagues already think they
receive too many emails so I don't think they'd like the "one email per
commit" way, but the 1 line summary is really short OTOH.
I wrote a quick and hopefully not-too-dirty implementation of it,
there's a pull request here:
https://github.com/mhagger/git-multimail/pull/6
essentially, it boils down to:
@@ -835,6 +837,17 @@ class ReferenceChange(Change):
for line in self.expand_lines(NO_NEW_REVISIONS_TEMPLATE):
yield line
+ if adds and self.showlog:
+ yield '\n'
+ yield 'Detailed log of added commits:\n\n'
+ for line in read_lines(
+ ['git', 'log']
+ + self.logopts
+ + ['%s..%s' % (self.old.commit, self.new.commit,)],
+ keepends=True,
+ ):
+ yield line
+
# The diffstat is shown from the old revision to the new
# revision. This is to show the truth of what happened in
# this change. There's no point showing the stat from the
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [ANN] First beta: Git export with hardlinks
From: Thomas Koch @ 2013-02-13 13:17 UTC (permalink / raw)
To: git; +Cc: Jeff King, Robert Clausecker
In-Reply-To: <20130211171357.GF16402@sigill.intra.peff.net>
Hi,
my git_export_hardlink command should now be in a usable state. I'd appreciate
any feedback: https://github.com/thkoch2001/git_export_hardlinks
I still have to choose a license: BSD/GPL/?
Jeff King:
> It looks like you create the sha1->path mapping by asking the user to
> provide <tree_sha1>,<path> pairs, and then assuming that the exported
> tree at <path> exactly matches <tree_sha1>. Which it would in the
> workflow you've proposed, but it is also easy for that not to be the
> case (e.g., somebody munges a file in <path> after it has been
> exported).
>
> So it's a bit dangerous as a general purpose tool, IMHO. It's also a
> slight pain in that you have to keep track of the tree sha1 for each
> exported path somehow.
You're right. I'd run a git reset --hard after each export to guarantee a
pristine export.
The tree sha1 of the exported tree might be part of the folder name of the
export or in some meta file related to the export, like
/deployments
/2012-03-05_14-23-02_0b96bf5f72d2c282b31726b3fbff279a89220b15
/export <- exported tree goes here
/meta <- git config file holding all relevant metadata: (who, when, tree,
commit, ref)
/index <- git index file corresponding to the exported tree (maybe?)
Regards,
Thomas Koch, http://www.koch.ro
^ permalink raw reply
* Re: git svn init throws Not a git repository (or any of the parent directories): .git
From: Konstantin Khomoutov @ 2013-02-13 13:15 UTC (permalink / raw)
To: amccloud@gmail.com; +Cc: git
In-Reply-To: <CALiJYpDPf8R2mOZn5Ey_yArSAg73WRASKS1cqLsAu1tGJcO_SA@mail.gmail.com>
On Wed, 13 Feb 2013 14:01:36 +0100
"amccloud@gmail.com" <amccloud@gmail.com> wrote:
> I have problem with git svn init:
> When I execute
> git svn init svn+ssh://username@example.com/path/repo
> I see:
> fatal: Not a git repository (or any of the parent directories): .git
> Already at toplevel, but .git not found
> at /usr/lib/git-core/git-svn line 342
Are you doing `git init` first before running `git svn init ...`?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox