Re: [PATCH 1/6] odb: don't remove a ".keep" we never installed

[Qin ShiCheng <qeesung@live.com>]

On 26/09/15 01:25PM, Justin Tobler wrote:
> Something worth noting, if there are two concurrent identical pushes,
> both will generate the same ".keep", but the keep message contained will
> differ. In such cases, when the quarantined files are migrated to the
> ODB, the ".keep" file that gets migrated first "wins" and the other push
> will fail because the competing ".keep" fails the collision check and
> consequently the push fails. I mention this because the current behavior
> for how Git handles concurrent identical pushes is to reject one of
> them. So if a process encounters an already existing ".keep" file in the
> main ODB, it may be sufficient to abort early anyways.

Agreed, aborting is fine there. The two pushes that bit us were seconds
apart rather than concurrent: the first had already finished and removed
its ".keep", so the second found nothing at that path and linked its own
".keep" onto the first push's pack. What the patch is after is narrower
than handling the collision: whatever happens on the way out, we should
only remove a ".keep" we created ourselves. Today finalize unlinks the
path unconditionally, even when pre-receive rejected the push and
nothing was migrated at all, which is what the first t5547 test pins.

> Right, registering the temporary ".keep" files doesn't really need to
> happen prior to the ODB transaction commit anyways. In fact, we could go
> a step further and stop using git-index-pack(1) to prematurely create
> ".keep" files altogether in favor of letting the commit phase of the ODB
> transaction create it explicitly.
[...]
> Completely unrelated to this bug as part of another series I'm working
> on locally, I've already have some patches that start creating ".keep"
> files explicitly during the ODB commit phase in the "files" backend. I
> would be happy to pick these patches out and send them upstream with
> some small adjustments to also fix the issue here in your first patch.
> Just let me know what you would perfer. :)

That is the better shape for it, please do. Creating the file at commit
time and registering it right there gives the two guarantees this patch
gets by reading the files back: nothing foreign is removed, and a signal
after our ".keep" is in place still cleans it up. (v1 registers before
the migration for the latter: the ".keep" is migrated first, and for a
duplicate of a large pack the migration then spends a while in
check_collision().) One case worth keeping in mind is a migration that
fails partway with our ".keep" already installed, e.g. the ".idx"
colliding because pack.indexVersion changed between the two pushes; the
second t5547 test covers that.

I will drop 1/6 from v2 so the series is only the repack side; 2/6-6/6
do not depend on it. Feel free to take the two t5547 tests if they are
of use to your series.

Thanks,
Qin