patchwrite_index: optionally allow broken null sha1s

Commit 4337b58 (do not write null sha1s to on-disk index, 2012-07-28) unconditionally prevents git from writing null sha1s into the index. The intent was to catch errors in other parts of the code that might let such an entry slip into the index (or worse, a tree).

However, some repositories have history in which the trees contain entries with null sha1s. Because the restriction is unconditional, it can be difficult to work with these older histories (for example, you cannot even check out the history, because you are forbidden to write the broken index). Worst of all, you cannot use git-filter-branch's index-filter to repair such a broken history, because filter-branch will try to write an index for each tree.

We could potentially work around this by using a commit-filter, and munging the tree manually. However, filter-branch unconditionally writes the index, even if we only asked for a commit-filter, so this doesn't work. That would be possible to fix, but figuring out that a commit-filter is needed (and what it should contain) is somewhat non-intuitive.

Instead, let's introduce an environment variable for relaxing this check, and use it when checking out the index in filter-branch. We turn it on by default, so users do not have to learn any special tricks to perform such a filter-branch. But we only do so for the index check-out, so any further manipulations the user does will fail unless they remove the broken entry.

We cannot catch every case, though, since some filter-branch invocations might not rewrite the index at all. But we still print a loud warning, so it is unlikely to go unnoticed by the user.

Signed-off-by: Jeff King <peff@peff.net>
---
I was tempted to not involve filter-branch in this commit at all, and
instead require the user to manually invoke

  GIT_ALLOW_NULL_SHA1=1 git filter-branch ...

to perform such a filter. That would be slightly safer, but requires some specialized knowledge from the user (and advice on using filter-branch to remove such entries already exists on places like stackoverflow, and this patch makes it Just Work on recent versions of git).

 git-filter-branch.sh               |  5 ++--
 read-cache.c                       | 13 +++++++--
 t/t7009-filter-branch-null-sha1.sh | 54 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 4 deletions(-)
 create mode 100755 t/t7009-filter-branch-null-sha1.sh

Hi,

Jeff King wrote:

The above few paragraphs explained the most mysterious part of the patch to me. I think they would be fine to include in the commit message.

[...]

Makes sense.

[...]

To avoid pipes involving git commands, since they can losing the exit status (and hence information about whether git crashed):

	{
		git ls-tree HEAD &&
		echo "160000 commit $_z40	broken"
	} >listing &&
	echo "add broken entry" >msg &&

	tree=$(git mktree <listing) &&
	test_tick &&
	commit=$(git commit-tree "$tree" -p HEAD <msg) &&
	git update-ref HEAD "$commit"

The rest looks good.

Thanks, Jonathan

On Sat, Aug 24, 2013 at 11:15:00PM -0700, Jonathan Nieder wrote:

I rolled them into the commit message.

I didn't bother, as we do not expect a simple ls-tree to fail (and we would notice anyway as we check that we have later introduced a broken tree state). Still, the resulting code is not any harder to read (which was my main hesitation), so let's do it as you suggest.

-- >8 --
Subject: write_index: optionally allow broken null sha1s

Commit 4337b58 (do not write null sha1s to on-disk index, 2012-07-28) unconditionally prevents git from writing null sha1s into the index. The intent was to catch errors in other parts of the code that might let such an entry slip into the index (or worse, a tree).

However, some repositories have history in which the trees contain entries with null sha1s. Because the restriction is unconditional, it can be difficult to work with these older histories (for example, you cannot even check out the history, because you are forbidden to write the broken index). Worst of all, you cannot use git-filter-branch's index-filter to repair such a broken history, because filter-branch will try to write an index for each tree.

We could potentially work around this by using a commit-filter, and munging the tree manually. However, filter-branch unconditionally writes the index, even if we only asked for a commit-filter, so this doesn't work. That would be possible to fix, but figuring out that a commit-filter is needed (and what it should contain) is somewhat non-intuitive.

Instead, let's introduce an environment variable for relaxing this check. We could stop there, and require that users rewriting such broken history run:

  GIT_ALLOW_NULL_SHA=1 git filter-branch ...

themselves. This is the safest option, as it means filter-branch will never successfully run on history that contains null sha1s. However, it is also inconvenient for the user, as they need to figure out the magic "ALLOW_SHA1" variable.

Instead, let's have filter-branch turn on the variable automatically, but only when checkout out the to-be-filtered index. This means that further filter commands that touch the index will still notice and fail, unless they actually remove the broken entry.

We would still succeed on a filter-branch whose filters do not touch the index at all (since we complain of the null sha1 only on writing, not when making a tree out of the index). This is acceptable, though, as we still print a loud warning (in addition to warning of the problem via fsck), so the problem is unlikely to go unnoticed.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-filter-branch.sh               |  5 ++--
 read-cache.c                       | 13 ++++++++--
 t/t7009-filter-branch-null-sha1.sh | 52 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 4 deletions(-)
 create mode 100755 t/t7009-filter-branch-null-sha1.sh

On Sun, Aug 25, 2013 at 05:58:18AM -0400, Jeff King wrote:

Hm --- the most useful part was "advice on using filter-branch to remove such entries already exists on places like stackoverflow", which was dropped. From my point of view, that is exactly the motivation of the patch.

I also found the "I was tempted to ... That would be slightly safer, but requires ..." structure easier to read.

In other words, why not use something like this?

	write_index: optionally allow broken null sha1s

	Commit 4337b58 (do not write null sha1s to on-disk index, 2012-07-28)
	added a safety check preventing git from writing null sha1s into the
	index. The intent was to catch errors in other parts of the code that
	might let such an entry slip into the index (or worse, a tree).

	Some existing repositories have some invalid trees that contain null
	sha1s already, though.  Until 4337b58, a common way to clean this up
	would be to use git-filter-branch's index-filter to repair such broken
	entries.  That now fails when filter-branch tries to write out the
	index.

	Introduce a GIT_ALLOW_NULL_SHA1 environment variable to relax this check
	and make it easier to recover from such a history.

	It is tempting to not involve filter-branch in this commit at all, and
	instead require the user to manually invoke

		GIT_ALLOW_NULL_SHA1=1 git filter-branch ...

	to perform an index-filter on a history with trees with null sha1s.
	That would be slightly safer, but requires some specialized knowledge
	from the user.  So let's set the GIT_ALLOW_NULL_SHA1 variable
	automatically when checking out the to-be-filtered trees.  Advice on
	using filter-branch to remove such entries already exists on places like
	stackoverflow, and this patch makes it Just Work again on recent
	versions of git.

	Further commands that touch the index will still notice and fail,	unless
	they actually remove the broken entries.  A filter-branch whose filters
	do not touch the index at all will not error out (since we complain of
	the null sha1 only on writing, not when making a tree out of the index),
	but this is acceptable, as we still print a loud warning, so the problem
	is unlikely to go unnoticed.

With or without such a change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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.

[...]

This is kind of an old-fashioned test, since each step of the setup is treated as a separate test assertion. I don't really mind until we get better automation to make it easy to skip or rearrange tests. Just for reference, I think the usual way to do this now is

	test_expect_success 'setup' '
		# create base commits
		...

		# create a commit with bogus null sha1 in the tree
		...

		# We have to make one more commit on top removing the broken
		# entry, since otherwise our index does not match HEAD and
		# filter-branch will complain. We could make the index match
		# HEAD, but doing so would involve writing a null sha1 into
		# the index.
		...
	'

	test_expect_success 'nontrivial filter-branch bails out on null sha1' '
		old_head=$(git rev-parse HEAD) &&
		test_must_fail git filter-branch ... &&
		test_cmp_rev "$old_head" HEAD
	'

	test_expect_success 'filter-branch can filter out null sha1' '
		git filter-branch ... &&

		# resulting history is clean
		echo three >expect &&
		git log -1 --format=%s >actual &&
		test_cmp expect actual
	'

Thanks, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

I found this version more readable than Peff's (albeit slightly).

Hmm, interesting thought.

On Sun, Aug 25, 2013 at 11:03:54PM -0700, Junio C Hamano wrote:

OK. Do you want to apply with Jonathan's wording, then?

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.

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.

-Peff

Jeff King <peff@peff.net> writes:

I can do that, as it seems all of us are in agreement.

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.

Yeah, I agree with all of the above.

On Mon, Aug 26, 2013 at 09:02:46AM -0700, Junio C Hamano wrote:

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.

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:

On Sun, Aug 25, 2013 at 12:54:12PM -0700, Jonathan Nieder wrote:

I don't see that splitting it up more hurts this. If we wanted more automatic rearranging or skipping of tests, we would need tests to declare dependencies on their setup. And we would need to be able to declare dependencies on multiple tests, since having a single setup test does not work in all cases (e.g., sometimes you are testing each step, and the final step relies on earlier steps).

And I do think splitting it up has more granularity. For example, in the sequence here:

Right now it is not hard to do step 2. But I could easily imagine a world in which git-mktree learns to complain about such bogus trees. And the sequence would become:

  1. create base commits

  2. check that mktree barfs

  3. check that we can override mktree to create a broken tree

  4. clean up tip state

in which case you really want to have individual tests.

I do not care _that_ much, since mktree does not behave that way now, and the setup is otherwise not that likely to fail. But I do not think more granularity actually hurts us, and it can sometimes help (as described above, but also when something fails, the test output more clearly pinpoints what happened).

-Peff

Jeff King wrote:

Actually dependencies can already be inferred for most test scripts using the following rule:

	Each test depends on all tests with the word "setup" or the
	phrase "set up" in their title that precede it.

And while there's no existing automation for testing that that's all the tests rely on (by automatically skipping or reordering tests, or running non-setup tests in separate sandboxes in parallel), in practice it is still already useful since it makes it safe to use GIT_SKIP_TESTS subject to the following constraint:

	In each test script, for every setup test skipped, all later
	tests are skipped as well.

I don't care as much about GIT_SKIP_TESTS as about being able to introduce new tests in the middle of a file.

Of course splitting up the setup into 3 steps neither helps nor hurts that. What I was complaining about is splitting the filter-branch from the verification that filter-branch had the right result.

Sorry for the confusion, Jonathan

On Mon, Aug 26, 2013 at 10:35:01AM -0700, Jonathan Nieder wrote:

I'd be very surprised if this works in practice on most of our current test scripts. There are often subtle dependencies on the state left over from previous tests. Running the script below up through t3800 (at which point I lost patience) reveals 37 test scripts that are broken. Which is only about 17%, but we're clearly not quite there yet.

But sure, I agree that is something to strive for. But I think my point stands; splitting up the setup doesn't hurt as long as you note all of the tests as dependencies. And by your rule above, the advice would be "each of these tests should say "setup" in the description". That makes sense to me, and I don't mind doing it here.

I totally missed your original point, then. I don't mind combining the last two into a single test. That makes sense to me (I only split them at all because I added the second one much later during the development).

-Peff

-- >8 -- #!/usr/bin/perl # # Run as "perl foo.pl t0000-basic.sh" (or whatever script you want to # check).

my $script = shift; my ($script_num) = $script =~ /^(t\d+)/;

# run once to get the list of tests my @tests = run_tests($script);

# mark some tests as "setup" tests that will always be run
foreach my $test (@tests) {
	if ($test->{desc} =~ /set.?up/i) {
		print STDERR "marking $test->{num} - $test->{desc} as setup\n";
		$test->{setup} = 1;
	}
}

# now try each test in isolation, but including setup tests
foreach my $test (@tests) {
	$ENV{GIT_SKIP_TESTS} = skip_all_except($script_num, $test, @tests);
	run_tests($script) or die "failed $test->{num} ($test->{desc})\n";
}

sub run_tests {
	my @r;
	open(my $fh, '-|', qw(sh), $script);
	while (<$fh>) {
		/^ok (\d+) - (.*)/ and push @r, { num => $1, desc => $2 };
	}
	$? and return ();
	return @r;
}

sub skip_all_except {
	my $prefix = shift;
	my $want = shift;

	return join(' ',
		map { "$prefix.$_->{num}" }
		grep { !$_->{setup} && $_->{num} != $want->{num} }
		@_);
}

Jeff King <peff@peff.net> writes:

Yes, I agree that in an ideal world with infinite amount of time, we should strive to make sure that each test is independent from previous steps, and if we do not have infinite amount of time, we should make sure we find some (say 5%) time to various code clean-up effort, in both writing and reviewing such, including tests.

The attached is cute.  Thanks for a food for thought.

On Mon, Aug 26, 2013 at 05:20:53PM -0400, Jeff King wrote:

So here is a re-roll with:

  1. The commit message tweaks you suggested

  2. Annotating the setup tests by using the word "setup"

  3. Combining the final two tests to filter and check the result.

I also added your Reviewed-by, since you gave it for the previous version, and all of the changes are taking your suggestions. :)

-- >8 --
Subject: write_index: optionally allow broken null sha1s

Commit 4337b58 (do not write null sha1s to on-disk index, 2012-07-28) added a safety check preventing git from writing null sha1s into the index. The intent was to catch errors in other parts of the code that might let such an entry slip into the index (or worse, a tree).

Some existing repositories may have invalid trees that contain null sha1s already, though. Until 4337b58, a common way to clean this up would be to use git-filter-branch's index-filter to repair such broken entries. That now fails when filter-branch tries to write out the index.

Introduce a GIT_ALLOW_NULL_SHA1 environment variable to relax this check and make it easier to recover from such a history.

It is tempting to not involve filter-branch in this commit at all, and instead require the user to manually invoke

	GIT_ALLOW_NULL_SHA1=1 git filter-branch ...

to perform an index-filter on a history with trees with null sha1s. That would be slightly safer, but requires some specialized knowledge from the user. So let's set the GIT_ALLOW_NULL_SHA1 variable automatically when checking out the to-be-filtered trees. Advice on using filter-branch to remove such entries already exists on places like stackoverflow, and this patch makes it Just Work again on recent versions of git.

Further commands that touch the index will still notice and fail, unless they actually remove the broken entries. A filter-branch whose filters do not touch the index at all will not error out (since we complain of the null sha1 only on writing, not when making a tree out of the index), but this is acceptable, as we still print a loud warning, so the problem is unlikely to go unnoticed.

Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-filter-branch.sh               |  5 ++--
 read-cache.c                       | 13 ++++++++--
 t/t7009-filter-branch-null-sha1.sh | 49 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 4 deletions(-)
 create mode 100755 t/t7009-filter-branch-null-sha1.sh