Re: git bugs

From: Ben Lynn <benlynn@gmail.com>
Date: Jun 11, 2008, 06:18 UTC
Message-ID: <832adb090806102318k5727bb06p6c3211a6aebbfbe9@mail.gmail.com>
In-Reply-To: <832adb090806102258v3fd63605p8c45513690b78fe8@mail.gmail.com>

And I'm sure you've also realized we could use the old race fix with SIZE_MAX instead of zero, i.e:

 if (!ce_match_stat_basic(ce, &st)) {
   ce->ce_size = ~0;
 }
 return; // don't bother with ce_modified_check_fs

This is dissatisfying in that we're abusing the size variable. I'd much prefer adding a flag per entry. And now there are other file sizes, albeit ridiculously large ones, that will trick git. But it is much faster than examining file contents. Luckily the decision of which fix to use is not up to me.

-Ben

On Wed, Jun 11, 2008 at 5:58 AM, Ben Lynn <benlynn@gmail.com> wrote:

Show 56 quoted lines

> Am I going crazy? All of a sudden I think I can get away without a
> size zero hack. How about this smudging routine:
>
> if (!ce_match_stat_basic(ce, &st)) {
>  recompute_sha1_and_update_index();  // no other checks required
> }
>
> That should be sufficient. I think what happened was the following.
> Once upon a time, the race fix was "if (stats_match) cached_size = 0",
> which is nice because you don't have to examine file contents. Later,
> because of the
>
>  $ echo xyzzy >frotz ; git-update-index --add frotz ; : >frotz
>  $ sleep 3
>  $ echo filfre >nitfol ; git-update-index --add nitfol
>
> issue, the ce_modified_check_fs was added.
>
> But then if we're going to be examining file contents anyway, we may
> as well drop the whole size zero trick and simply update the hash. The
> bug I brought up also goes away.
>
> -Ben
>
> P.S: I could go through the history to see, but I bet there was a
> stage after the race condition was discovered but before it was
> realized that
>  $ git update-index 'foo'
>  : modify 'foo' in-place without changing its size
>  : wait for enough time
>  $ git update-index 'bar'
> was a problem.
>
> On Tue, Jun 10, 2008 at 7:39 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>>
>> On Tue, 10 Jun 2008, Ben Lynn wrote:
>>>
>>> Ah, I hadn't seen that. Yes, it is better to use the first write as
>>> the timestamp. Would this catch everything? If the filesystem clock is
>>> monotonically increasing and consistent then with this setup, you can
>>> touch files even as they are being indexed? (Disregarding nonsense
>>> like changing sizes by 2^32.)
>>
>> Yes, I think that at that point it would protect against arbitrary
>> modifications even concurrently to index file creation.
>>
>> That said, I don't think you even need a new index file format. We could
>> just do a stat() on starting the index file creation, and then do a
>> futimes() system call at the end to re-set the mtime to the beginning
>> before we rename it back over the old index file.
>>
>>                Linus
>>
>