patch, 2 partsfast-export: deletion action first

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

> +/*
> + * 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;
>  	/*

>
>> +/*
>> + * 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.

> > 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?

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

>
> 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.
>
> As an equal comparison doesn't have any deterministic final order,
> it's better to keep original diff order input when there is no prefer order
> ( that's done comparing pointers)
>
> 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).
>
> The test "file becomes directory" has been added in order to exercise
> the original motivation of the deletion reorder.
>
> Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com>
> ---
>  builtin/fast-export.c  | 32 +++++++++++++++-----------------
>  t/t9350-fast-export.sh | 25 +++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index e022063..e82f654 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -260,26 +260,19 @@ static void export_blob(const struct object_id *oid)
>                 free(buf);
>  }
>
> -static int depth_first(const void *a_, const void *b_)
> +/*
> + * Compares two diff types to order based on output priorities.
> + */
> +static int diff_type_cmp(const void *a_, const void *b_)
>  {
>         const struct diff_filepair *a = *((const struct diff_filepair **)a_);
>         const struct diff_filepair *b = *((const struct diff_filepair **)b_);
> -       const char *name_a, *name_b;
> -       int len_a, len_b, len;
>         int cmp;
>
> -       name_a = a->one ? a->one->path : a->two->path;
> -       name_b = b->one ? b->one->path : b->two->path;
> -
> -       len_a = strlen(name_a);
> -       len_b = strlen(name_b);
> -       len = (len_a < len_b) ? len_a : len_b;
> -
> -       /* strcmp will sort 'd' before 'd/e', we want 'd/e' before 'd' */
> -       cmp = memcmp(name_a, name_b, len);
> -       if (cmp)
> -               return cmp;
> -       cmp = len_b - len_a;
> +       /*
> +        * 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;
>         /*
> @@ -287,7 +280,12 @@ static int depth_first(const void *a_, const void *b_)
>          * appear in the output before it is renamed (e.g., when a file
>          * was copied and renamed in the same commit).
>          */
> -       return (a->status == 'R') - (b->status == 'R');
> +       cmp = (a->status == DIFF_STATUS_RENAMED) - (b->status == DIFF_STATUS_RENAMED);
> +       if (cmp)
> +               return cmp;
> +
> +       /* For the remaining cases we keep the original ordering comparing the pointers */
> +       return (a-b);
>  }
>
>  static void print_path_1(const char *path)
> @@ -347,7 +345,7 @@ static void show_filemodify(struct diff_queue_struct *q,
>          * Handle files below a directory first, in case they are all deleted
>          * and the directory changes to a file or symlink.
>          */
> -       QSORT(q->queue, q->nr, depth_first);
> +       QSORT(q->queue, q->nr, diff_type_cmp);
>
>         for (i = 0; i < q->nr; i++) {
>                 struct diff_filespec *ospec = q->queue[i]->one;
> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> index b5149fd..d4f369a 100755
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -419,6 +419,31 @@ test_expect_success 'directory becomes symlink'        '
>         (cd result && git show master:foo)
>  '
>
> +test_expect_success 'file becomes directory'  '
> +       git init filetodir_orig &&
> +       git init --bare filetodir_replica.git &&
> +       (
> +               cd filetodir_orig &&
> +               echo foo > filethendir &&
> +               git add filethendir &&
> +               test_tick &&
> +               git commit -mfile &&
> +               git rm filethendir &&
> +               mkdir filethendir &&
> +               echo bar > filethendir/a &&
> +               git add filethendir/a &&
> +               test_tick &&
> +               git commit -mdir
> +       ) &&
> +       git --git-dir=filetodir_orig/.git fast-export master  |
> +               git --git-dir=filetodir_replica.git/ fast-import &&
> +       (
> +               ORIG=$(git --git-dir=filetodir_orig/.git rev-parse --verify master) &&
> +               REPLICA=$(git --git-dir=filetodir_replica.git rev-parse --verify master) &&
> +               test $ORIG = $REPLICA
> +       )
> +'
> +
>  test_expect_success 'fast-export quotes pathnames' '
>         git init crazy-paths &&
>         (cd crazy-paths &&
> --
> 2.1.4
>