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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2017, 04:24 UTC
Message-ID: <xmqqfugxw1us.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170425032927.74btvfcexbdq4rmz@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Perhaps we would want a test for the case you are fixing (to be sure it
> is not re-broken), as well as confirming that we have not re-broken the
> original case (it looks like 060df62 added a test, so we may be OK with
> that).

Good suggestion.

Show 17 quoted lines

>
>> +/*
>> + * Compares two diff types to order based on output priorities.
>> + */
>> +static int diff_type_cmp(const void *a_, const void *b_)
>> [...]
>> +	/*
>> +	 * Move Delete entries first so that an addition is always reported after
>> +	 */
>> +	cmp = (b->status == DIFF_STATUS_DELETED) - (a->status == DIFF_STATUS_DELETED);
>>  	if (cmp)
>>  		return cmp;
>>  	/*
>
> So we sort deletions first. And the bit that the context doesn't quite
> show here is that we then compare renames and push them to the end.
> Everything else will compare equal.

Wait--we also allow renames? Rename is like delete in the context of discussing d/f conflicts, in that it tells us that the source path will be missing in the end result. If you rename a file "d" to "e", then there is a room for you to create a directory "d" to store a file "d/f" in. Shouldn't it participate also in this "delete before add to avoid d/f conflict" logic?

> Is qsort() guaranteed to be stable? If not, then we'll get the majority
> of entries in a non-deterministic order. Should we fallback to strcmp()
> so that within a given class, the entries are sorted by name?

Again, very good point, especially with the existing comment in the comparison function that explains why renames are shown last.