Re: [PATCH 2/3] do not write null sha1s to on-disk index
Hi Peff, Jeff King wrote: --- a/read-cache.c +++ b/read-cache.c @@ -1800,6 +1800,8 @@ int write_index(struct index_state *istate, int newfd) continue; if (!ce_uptodate(ce) is_racy_timestamp(istate, ce)) ce_smudge_racily_clean_entry(ce); + if (is_null_sha1(ce-sha1)) + return error(cache entry has null sha1: %s, ce-name); if (ce_write_entry(c, newfd, ce, previous_name) 0) return -1; Quick heads up: this is tripping for me in the git am --abort codepath: $ cd ~/src/linux $ git checkout v3.2.35 $ git am -3sc /tmp/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch Applying: ALSA: usb-audio: Fix missing autopm for MIDI input Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... error: Your local changes to the following files would be overwritten by merge: sound/usb/midi.c Please, commit your changes or stash them before you can merge. Aborting Failed to merge in the changes. Patch failed at 0001 ALSA: usb-audio: Fix missing autopm for MIDI input When you have resolved this problem run git am --resolved. If you would prefer to skip this patch, instead run git am --skip. To restore the original branch and stop patching run git am --abort. $ git am --abort error: cache entry has null sha1: sound/usb/midi.c fatal: unable to write new index file Unstaged changes after reset: M sound/usb/midi.c $ Reproducible using v1.8.1-rc3 and master. Bisects to 4337b585 (do not write null sha1s to on-disk index, 2012-07-28). For comparison, older gits produced $ git am --abort Unstaged changes after reset: M sound/usb/midi.c which is also not great but at least didn't involve any obviously invalid behavior. Known problem? Thanks, Jonathan -- 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/3] do not write null sha1s to on-disk index
On Sat, Dec 29, 2012 at 02:03:46AM -0800, Jonathan Nieder wrote: --- a/read-cache.c +++ b/read-cache.c @@ -1800,6 +1800,8 @@ int write_index(struct index_state *istate, int newfd) continue; if (!ce_uptodate(ce) is_racy_timestamp(istate, ce)) ce_smudge_racily_clean_entry(ce); + if (is_null_sha1(ce-sha1)) + return error(cache entry has null sha1: %s, ce-name); if (ce_write_entry(c, newfd, ce, previous_name) 0) return -1; Quick heads up: this is tripping for me in the git am --abort codepath: Thanks. The intent was that this should never happen, and we are protecting against bogus sha1s slipping into the index. So either we have found a bug in am --abort, or the assumption that it should never happen was wrong. :) $ cd ~/src/linux $ git checkout v3.2.35 $ git am -3sc /tmp/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch Applying: ALSA: usb-audio: Fix missing autopm for MIDI input Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... error: Your local changes to the following files would be overwritten by merge: sound/usb/midi.c Please, commit your changes or stash them before you can merge. Aborting Failed to merge in the changes. Patch failed at 0001 ALSA: usb-audio: Fix missing autopm for MIDI input When you have resolved this problem run git am --resolved. If you would prefer to skip this patch, instead run git am --skip. To restore the original branch and stop patching run git am --abort. $ git am --abort error: cache entry has null sha1: sound/usb/midi.c fatal: unable to write new index file Unstaged changes after reset: M sound/usb/midi.c $ I can't reproduce here. I can checkout v3.2.35, and I guess that the patch you are applying comes from f5f1654, but I don't know your local modification to sound/usb/midi.c. If I just make a fake modification, I get this: $ git checkout v3.2.35 $ echo fake sound/usb/midi.c $ git format-patch --stdout -1 f5f1654 | git am -3 ~/patch [same errors as you] $ git am --abort Unstaged changes after reset: M sound/usb/midi.c So I wonder if there is something in the way your working tree is modified. Can you give more details? Reproducible using v1.8.1-rc3 and master. Bisects to 4337b585 (do not write null sha1s to on-disk index, 2012-07-28). For comparison, older gits produced $ git am --abort Unstaged changes after reset: M sound/usb/midi.c What does your index look like afterwards? Does it have a null sha1 in it (check ls-files -s)? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] do not write null sha1s to on-disk index
Jeff King wrote: I can't reproduce here. I can checkout v3.2.35, and I guess that the patch you are applying comes from f5f1654, but I don't know your local modification to sound/usb/midi.c. No local modification. The unstaged change after git am --abort to recover from a conflicted git am is a longstanding bug (at least a couple of years old). The patch creating conflicts is queue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch from git://git.kernel.org/pub/scm/linux/kernel/git/bwh/linux-3.2.y-queue.git [...] $ git am --abort Unstaged changes after reset: M sound/usb/midi.c What does your index look like afterwards? Does it have a null sha1 in it (check ls-files -s)? $ git diff-index --abbrev HEAD :100644 100644 eeefbce3873c... ... Msound/usb/midi.c $ git ls-files --abbrev -s sound/usb/midi.c 100644 eeefbce3873c 0 sound/usb/midi.c -- 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/3] do not write null sha1s to on-disk index
Jeff King wrote: Can you give more details? $ GIT_TRACE=1 git am --abort trace: exec: 'git-am' '--abort' trace: run_command: 'git-am' '--abort' trace: built-in: git 'rev-parse' '--parseopt' '--' '--abort' trace: built-in: git 'rev-parse' '--git-dir' trace: built-in: git 'rev-parse' '--show-prefix' trace: built-in: git 'rev-parse' '--show-toplevel' trace: built-in: git 'var' 'GIT_COMMITTER_IDENT' trace: built-in: git 'rev-parse' '--verify' '-q' 'HEAD' trace: built-in: git 'config' '--bool' '--get' 'am.keepcr' trace: built-in: git 'rerere' 'clear' trace: built-in: git 'rev-parse' '--verify' '-q' 'HEAD' trace: built-in: git 'read-tree' '--reset' '-u' 'HEAD' 'ORIG_HEAD' error: cache entry has null sha1: sound/usb/midi.c fatal: unable to write new index file trace: built-in: git 'reset' 'ORIG_HEAD' Unstaged changes after reset: M sound/usb/midi.c -- 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/3] do not write null sha1s to on-disk index
On Sat, Dec 29, 2012 at 02:34:30AM -0800, Jonathan Nieder wrote: Jeff King wrote: I can't reproduce here. I can checkout v3.2.35, and I guess that the patch you are applying comes from f5f1654, but I don't know your local modification to sound/usb/midi.c. No local modification. The unstaged change after git am --abort to recover from a conflicted git am is a longstanding bug (at least a couple of years old). The patch creating conflicts is queue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch from git://git.kernel.org/pub/scm/linux/kernel/git/bwh/linux-3.2.y-queue.git Hrm. But your output does not say there is a conflict. It says you have a local modification and it does not try the merge: $ git am -3sc /tmp/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch Applying: ALSA: usb-audio: Fix missing autopm for MIDI input Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... error: Your local changes to the following files would be overwritten by merge: sound/usb/midi.c Please, commit your changes or stash them before you can merge. Aborting If I try to apply it, I get a real conflict: $ git checkout v3.2.35 $ git am -3sc linux-3.2.y-queue/queue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch Applying: ALSA: usb-audio: Fix missing autopm for MIDI input Using index info to reconstruct a base tree... M sound/usb/midi.c Falling back to patching base and 3-way merge... Auto-merging sound/usb/midi.c CONFLICT (content): Merge conflict in sound/usb/midi.c Although running git am --abort after that does seem to produce the cache entry has null sha1 error. So I can start investigating from there. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] do not write null sha1s to on-disk index
Jeff King wrote: Hrm. But your output does not say there is a conflict. It says you have a local modification and it does not try the merge: That's probably operator error on my part when gathering output to paste into the email. In other words, nothing to see there. :) Sorry for the confusion. [...] If I try to apply it, I get a real conflict: [...] Although running git am --abort after that does seem to produce the cache entry has null sha1 error. Yep, that is what my report should have said. 'night, Jonathan -- 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/3] do not write null sha1s to on-disk index
On Sat, Dec 29, 2012 at 02:34:30AM -0800, Jonathan Nieder wrote: $ git am --abort Unstaged changes after reset: M sound/usb/midi.c What does your index look like afterwards? Does it have a null sha1 in it (check ls-files -s)? $ git diff-index --abbrev HEAD :100644 100644 eeefbce3873c... ... M sound/usb/midi.c $ git ls-files --abbrev -s sound/usb/midi.c 100644 eeefbce3873c 0 sound/usb/midi.c Hmm. It looks like am --abort overwrites the index again after the read-tree which complains. If I downgrade the error in write_index to a warning, like this: diff --git a/read-cache.c b/read-cache.c index fda78bc..70a6d86 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1797,7 +1797,7 @@ int write_index(struct index_state *istate, int newfd) if (!ce_uptodate(ce) is_racy_timestamp(istate, ce)) ce_smudge_racily_clean_entry(ce); if (is_null_sha1(ce-sha1)) - return error(cache entry has null sha1: %s, ce-name); + warning(cache entry has null sha1: %s, ce-name); if (ce_write_entry(c, newfd, ce, previous_name) 0) return -1; } and then just run this: [clear state from last run] $ rm -rf .git/rebase-apply $ git reset --hard [apply the patch; we get a conflict] $ git am -3sc queue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch [now run just the read-tree from am --abort] $ git.compile read-tree --reset -u HEAD ORIG_HEAD warning: cache entry has null sha1: sound/usb/midi.c [and now check our index] $ git ls-files -s sound/usb/midi.c 100644 0 sound/usb/midi.c [yes, this index is bogus] $ git write-tree error: invalid object 100644 for 'sound/usb/midi.c' fatal: git-write-tree: error building trees So I think this check may actually be finding a real bug. I have seen these null sha1s in the wild, but I was never able to track down the actual cause. Maybe this will give us a clue. Now we just need to work backwards and figure out who is putting it in the in-memory index and why. I'll try to work on it tomorrow, but please don't let that stop you if you want to keep digging in the meantime. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] do not write null sha1s to on-disk index
We should never need to write the null sha1 into an index entry (short of the 1 in 2^160 chance that somebody actually has content that hashes to it). If we attempt to do so, it is much more likely that it is a bug, since we use the null sha1 as a sentinel value to mean not valid. The presence of null sha1s in the index (which can come from, among other things, update-index --cacheinfo, or by reading a corrupted tree) can cause problems for later readers, because they cannot distinguish the literal null sha1 from its use a sentinel value. For example, git diff-files on such an entry would make it appear as if it is stat-dirty, and until recently, the diff code assumed such an entry meant that we should be diffing a working tree file rather than a blob. Ideally, we would stop such entries from entering even our in-core index. However, we do sometimes legitimately add entries with null sha1s in order to represent these sentinel situations; simply forbidding them in add_index_entry breaks a lot of the existing code. However, we can at least make sure that our in-core sentinel representation never makes it to disk. To be thorough, we will test an attempt to add both a blob and a submodule entry. In the former case, we might run into problems anyway because we will be missing the blob object. But in the latter case, we do not enforce connectivity across gitlink entries, making this our only point of enforcement. The current implementation does not care which type of entry we are seeing, but testing both cases helps future-proof the test suite in case that changes. Signed-off-by: Jeff King p...@peff.net --- I confess to not being clear on all of the instances in which a null sha1 might enter the in-core index. I did try modifying add_index_entry, but the breakage was pretty severe. I traced through a few instances, and it seemed to be mostly related to unmerged entries. read-cache.c | 2 ++ t/t2107-update-index-basic.sh | 19 +++ 2 files changed, 21 insertions(+) diff --git a/read-cache.c b/read-cache.c index 2f8159f..d2be78e 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1800,6 +1800,8 @@ int write_index(struct index_state *istate, int newfd) continue; if (!ce_uptodate(ce) is_racy_timestamp(istate, ce)) ce_smudge_racily_clean_entry(ce); + if (is_null_sha1(ce-sha1)) + return error(cache entry has null sha1: %s, ce-name); if (ce_write_entry(c, newfd, ce, previous_name) 0) return -1; } diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh index 809fafe..0dbbb00 100755 --- a/t/t2107-update-index-basic.sh +++ b/t/t2107-update-index-basic.sh @@ -29,4 +29,23 @@ test_expect_success 'update-index -h with corrupt index' ' grep [Uu]sage: git update-index broken/usage ' +test_expect_success '--cacheinfo does not accept blob null sha1' ' + echo content file + git add file + git rev-parse :file expect + test_must_fail git update-index --cacheinfo 100644 $_z40 file + git rev-parse :file actual + test_cmp expect actual +' + +test_expect_success '--cacheinfo does not accept gitlink null sha1' ' + git init submodule + (cd submodule test_commit foo) + git add submodule + git rev-parse :submodule expect + test_must_fail git update-index --cacheinfo 16 $_z40 submodule + git rev-parse :submodule actual + test_cmp expect actual +' + test_done -- 1.7.11.3.42.g90758bf -- 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
