Jeff King <p...@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. > >> +/* >> + * 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.
