Re: [PATCHv2 2/2] merge-one-file: fix broken merges with alternate work trees

From: Jeff King <peff@peff.net>
Date: Apr 29, 2011, 22:46 UTC
Message-ID: <20110429224646.GA4095@sigill.intra.peff.net>
In-Reply-To: <7vtydgvfx4.fsf@alter.siamese.dyndns.org>

On Fri, Apr 29, 2011 at 03:41:43PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > This patch makes merge-one-file chdir to the toplevel of the working
> > tree (and exit if we don't have one). This most closely matches the
> > assumption made by the original script (before separate work trees were
> > invented), and matches what happens when the script is called as part of
> > a merge strategy.
> >
> > While we're at it, we'll also error-check the call to cat.
> > Merging a file in a subdirectory could in fact fail, as the
> > redirection relies on the "checkout-index" call just prior
> > to create leading directories. But we never noticed, since
> > we ignored the error return from running cat.
> 
> This part is probably incorrect as we have && before cat that checks
> an error from checkout-index that fails to create such a subdirectory, no?

Sort of. In the original code, checkout-index wrote to one spot, and cat wrote to another, so they could fail independently, masking the bug.

Now that the code correctly respects GIT_WORK_TREE, that should never happen, and the only reason for cat to fail is something like ENOSPC (checkout-index wrote the original version, but we are writing the merge result, which could be larger).

> And then "exec git update-index -- $4" at the last step would have failed.

No. In the old code, it succeeded because it respected GIT_WORK_TREE, so it marked the original stage-2 version as the merge result (hence the bogus merge results).

In the new code, if we got ENOSPC, then it would mark the truncated merge result as good (except that we probably would also fail to write a new index due to the same error...).

It's unlikely, I'll grant, but I don't see a reason not to check the error.

-Peff