Re: [WIP PATCH] fast-export: emit deletions first

[Junio C Hamano <gitster@pobox.com>]

"Raymond E. Pasco" <ray@ameretat.dev> writes:

> fast-export chooses its output order by pathname, sorting longer
> paths earlier. However, this causes faulty output when the deleted
> path is a prefix of the added one. For example, deleting a file 'a' and
> creating a file 'a/b' emits:
>
> from :prev_label
> M 100644 :blob_label a/b
> D a
>
> Fix this by sorting deletions to come before other types of change.
>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>
> This is a quick and dirty fix for the bug. However, I do want to spend a
> little more time on it - it may be that we only want to reverse the sort
> when the deletion is specifically the prefix of some addition, and I
> want to fence this off with new tests.

I recall doing something like this in "git checkout" and also "git
am" to ensure that a thing deep in the hierarchy will not be
affected by a D/F conflict at a shallower level, so I do not have
objection to this kind of change in principle.  I do not know if
depth_first() is the right place to make this decision or if the
function should keep its name if it turns out to be the right place.

In any case, it is a bit surprising that fast-export survived this
long without having encountering the problem you are solving.  I
wonder if fast-import handles such an output with some smart to
avoid the issue?

Thanks.

>  builtin/fast-export.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index b90da5e616..82d73b2f43 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -354,6 +354,12 @@ static int depth_first(const void *a_, const void *b_)
>  	int len_a, len_b, len;
>  	int cmp;
>  
> +	/* emit deletions first */
> +	int a_deletes = (a->status == DIFF_STATUS_DELETED);
> +	int b_deletes = (b->status == DIFF_STATUS_DELETED);
> +	if (a_deletes != b_deletes)
> +		return b_deletes - a_deletes;
> +
>  	name_a = a->one ? a->one->path : a->two->path;
>  	name_b = b->one ? b->one->path : b->two->path;