Jeff King [off-list ref] writes:
quoted
I found this version more readable than Peff's (albeit slightly).
OK. Do you want to apply with Jonathan's wording, then?
I can do that, as it seems all of us are in agreement.
There's one subtle thing I didn't mention in the "it is already on stack
overflow". If you have a version of git which complains about the null
sha1, then the SO advice is already broken. But if the SO works, then
you do not have a version of git which complains. So why do you care?
And the answer is: you may be pushing to a remote with a version of git
that complains, and which has receive.fsckObjects set (and in many
cases, that remote is GitHub, since we have had that check on for a
while).
I don't know if it is worth spelling that out or not.
Probably not.
You could aim to correct each and every wrong suggestions on a site
where misguided leads other misguided, but it is a hopeless task.
quoted
quoted
After this patch, do you think (in a separate change) it would make
sense for cache-tree.c::update_one() to check for null sha1 and error
out unless GIT_ALLOW_NULL_SHA1 is true? That would let us get rid of
the caveat from the last paragraph.
Hmm, interesting thought.
I think it is worth doing. The main reason I put the original check on
writing to the index is that it more clearly pinpoints the source of the
error. If we just died during git-write-tree, then you know somebody
broke your index, but you don't know which command.
But checking in both places would add extra protection, and would make
possible the "relax on read, strict on write" policy that filter-branch
wants to do.
Yeah, I agree with all of the above.
On Mon, Aug 26, 2013 at 09:02:46AM -0700, Junio C Hamano wrote:
quoted
There's one subtle thing I didn't mention in the "it is already on stack
overflow". If you have a version of git which complains about the null
sha1, then the SO advice is already broken. But if the SO works, then
you do not have a version of git which complains. So why do you care?
And the answer is: you may be pushing to a remote with a version of git
that complains, and which has receive.fsckObjects set (and in many
cases, that remote is GitHub, since we have had that check on for a
while).
I don't know if it is worth spelling that out or not.
Probably not.
You could aim to correct each and every wrong suggestions on a site
where misguided leads other misguided, but it is a hopeless task.
I do no think the advice on SO was misguided. My point was that a reader
of the revised commit message mentioning SO may wonder "why would people
on SO suggest that? It does not work since we introduced the check in
the first place".
But if neither you nor Jonathan wondered about it, then it is probably
not worth caring.
quoted
quoted
quoted
After this patch, do you think (in a separate change) it would make
sense for cache-tree.c::update_one() to check for null sha1 and error
out unless GIT_ALLOW_NULL_SHA1 is true? That would let us get rid of
the caveat from the last paragraph.
[...]
Hmm. We are already complaining about the null sha1 in most cases,
because we do not have the object. It is only for submodules that the
bogus entry can make it into a tree. So I tried this:
diff --git a/cache-tree.c b/cache-tree.c
index 0bbec43..0ec3923 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -343,7 +343,8 @@ static int update_one(struct cache_tree *it,
entlen = pathlen - baselen;
i++;
}
- if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {
+ if ((mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))
+ || (mode == S_IFGITLINK && is_null_sha1(sha1))) {
strbuf_release(&buffer);
return error("invalid object %06o %s for '%.*s'",
mode, sha1_to_hex(sha1), entlen+baselen, path);diff --git a/t/t7009-filter-branch-null-sha1.sh b/t/t7009-filter-branch-null-sha1.sh
index 50d5689..59c5edc 100755
--- a/t/t7009-filter-branch-null-sha1.sh
+++ b/t/t7009-filter-branch-null-sha1.sh
@@ -31,10 +31,10 @@ test_expect_success 'filter commands are still checked' '
git commit -a -m "back to normal"
'
-test_expect_success 'filter commands are still checked' '
+test_expect_success 'complain about trees that are broken after filtering' '
test_must_fail git filter-branch \
--force --prune-empty \
- --index-filter "git rm --cached --ignore-unmatch three.t"
+ --index-filter ""
'
test_expect_success 'removing the broken entry works' '
But it doesn't work. The reason is that we do not need to re-make the
cache-tree, because we are just handing back the crap that we got from
the original tree. And I do not think we would want to invalidate the
whole cache tree just on the off chance that it has a problem;
filter-branch is slow enough as it is.
So I think we are better off to let filter-branch run at full speed,
even if optimizations like cache-tree mean we are missing out on
validating all of the data. If you want to know whether your repo is in
good shape, you should run `git fsck`.
-Peff
PS I got more information from the user who originally experienced this
problem (and yes, it was a submodule entry). It turns out that he had
done some unusual stuff with having a git repository inside his Eclipse
project, and adding it as a sub-project of his build. So it was
almost certainly EGit/JGit which wrote the bad entry in the first
place (and he is making a bug report to EGit folks).