Re: [PATCH] bundle: dup() output descriptor closer to point-of-use
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 17, 2018, 07:05 UTC
- Message-ID
- <xmqqk1lci3p3.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20181116094358.GA6054@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> Actually, I realized there's an even simpler way to do this. Here it is. > > -- >8 -- > Subject: [PATCH] bundle: dup() output descriptor closer to point-of-use > > When writing a bundle to a file, the bundle code actually creates > "your.bundle.lock" using our lockfile interface. We feed that output > descriptor to a child git-pack-objects via run-command, which has the > quirk that it closes the output descriptor in the parent. > > To avoid confusing the lockfile code (which still thinks the descriptor > is valid), we dup() it, and operate on the duplicate.
Yes...
Show 11 quoted lines
> We can solve this by moving the dup() much closer to start_command(), > shrinking the window in which we have the second descriptor open. It's > easy to place this in such a way that no die() is possible. We could > still die due to a signal in the exact wrong moment, but we already > tolerate races there (e.g., a signal could come before we manage to put > the file on the cleanup list in the first place). > > As a bonus, this shields create_bundle() itself from the duplicate-fd > trick, and we can simplify its error handling (note that the lock > rollback now happens unconditionally, but that's OK; it's a noop if we > didn't open the lock in the first place).
... I found this way too clever for me, but by following the codeflow, it was easy to convince myself that this does the right thing. Almost magical ;-)
Will queue, with a Tested-by: with Dscho's name on it.
TAhanks, both.