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

From: Jeff King <peff@peff.net>
Date: Apr 25, 2017, 03:29 UTC
Message-ID: <20170425032927.74btvfcexbdq4rmz@sigill.intra.peff.net>
In-Reply-To: <1493079137-1838-1-git-send-email-miguel.torroja@gmail.com>

On Tue, Apr 25, 2017 at 02:12:16AM +0200, Miguel Torroja wrote:

Show 8 quoted lines

> The delete operations of the fast-export output should precede any addition
> belonging to the same commit, Addition and deletion with the same name
> entry could happen in case of file to directory and viceversa.
> 
> The fast-export sorting was added in 060df62 (fast-export: Fix output
> order of D/F changes). That change was made in order to fix the case of
> directory to file in the same commit, but it broke the reverse case
> (File to directory).

That explanation makes sense.

>  builtin/fast-export.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)

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).

Show 12 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.

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?

-Peff