Re: [PATCH 1/2] fast-export: deletion action first

From: Jeff King <peff@peff.net>
Date: Apr 25, 2017, 05:58 UTC
Message-ID: <20170425055817.codq2q3fd54uebfx@sigill.intra.peff.net>
In-Reply-To: <xmqqy3upuk4o.fsf@gitster.mtv.corp.google.com>

On Mon, Apr 24, 2017 at 10:33:11PM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > Anyway. I don't think Miguel's patch needs to solve all of the lingering
> > rename cases. But I am curious whether it makes some rename cases worse,
> > because the depth-sorting was kicking in before and making them work.
> 
> I agree with you on both counts, and I care more about the second
> sentence, not just "am curious", but "am worried".  I am not sure
> that this patch is safe---it looked more like robbing peter to pay
> paul or the other way around.  Fixing for one class of breakage
> without regressing is one thing and it is perfectly fine to leave
> some already broken case broken with such a fix.  Claiming to fix
> one class and breaking other class that was happily working is quite
> different, and that is where my "Wait, we also allow renames?" comes
> from.

Yeah, I don't disagree. I am just curious first, then worried second. :)

If I had to choose, though, I'd rather see the order be reliable for the no-renames case. IOW, if we must rob one peter, I'd rather it be the renames, which already have tons of corner cases (and which I do not think can be plugged for a reader which depends on the order of the entries; the dependencies can be cycles).

Of course if we can make it work correctly in all of the non-cyclical cases, all the better.

-Peff