Re: [PATCH v7] read-cache: force_verify_index_checksum

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 25, 2017, 21:04 UTC
Message-ID: <alpine.DEB.2.20.1704252225450.3480@virtualbox>
In-Reply-To: <xmqqk269xmmq.fsf@gitster.mtv.corp.google.com>

Hi,

On Mon, 24 Apr 2017, Junio C Hamano wrote:

Show 15 quoted lines

> Jeff Hostetler <git@jeffhostetler.com> writes:
> 
> >>> +test_expect_success 'detect corrupt index file in fsck' '
> >>> +    cp .git/index .git/index.backup &&
> >>> +    test_when_finished "mv .git/index.backup .git/index" &&
> >>> +    echo zzzzzzzz >zzzzzzzz &&
> >>> +    git add zzzzzzzz &&
> >>> +    sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&
> >>
> >> sed on a binary file? Sooner or later we are going to run into
> >> portability issues.
> >
> > In v5 of this patch series I used "perl" and it was suggested that I
> > use "sed" instead.  It doesn't matter to me which we use.  My testing
> > showed that it was safe, but that was only Linux.

I am sorry to hear that the Git mailing list's review gives you whiplash.

The problem with sed is that BSD sed behaves a bit differently than GNU sed, and we quietly expect every contributor to be an expert in the portability aspects of sed.

TBH I am quite surprised that anybody would have suggested to use sed rather than Perl to edit binary files in the first place. In my opinion, that was bad advice.

Show 6 quoted lines

> > Does the mailing list have a preference for this ?
> 
> Instead of munging pathnames z* to y*, I'd prefer to see the actual
> checksum bytes at the end replaced in the index file.  After all
> that is what this test really cares about, and it ensures that the
> failure detected is due to checksum mismatch.

I see that v8 uses a Perl script again, and it is well written and obvious.

Just in case that certain reviewers favor length over readability, let me offer this snippet:

	size=$(perl -e "print -s \".git/index\"") &&
	dd if=/dev/zero of=.git/index bs=1 seek=$(($size-20) count=20

Since whatever hash will be used in the future is most likely larger than 20 bytes, this should still work fine (and even if somebody sane replaces the SHA-1 of the index with a CRC-32 for the same benefit we have now, the test will fail quickly and it is easy to replace the 20 by 4).

Ciao, Dscho