Re: [PATCH] diffcore-rename: don't consider unmerged path as source

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Martin von Zweigbergk <martin.von.zweigbergk@xxxxxxxxx> writes:

> The output from 'git status' and 'git diff-index --cached -M' is
> broken when there are unmerged files as well as new files similar to
> the unmerged file (in stage 1).
>
> When two copies have been created the output is:
>
> $ git status -s
> C  original -> exact-copy
> R  original -> modified-copy
>  U original

I think the above actually sort-of makes sense.  The first column of
status output is about comparing the HEAD and the index, and the second
column is about comparing the index and the working tree, but I'll begin
by explaining the latter.

When comparing an unmerged index with the working tree, the comparison
itself does not make much sense.  When producing textual diff,
"diff-index" tends to give the difference between stage #2 and the working
tree in order to show the difference from "our" version and the result of
the incomplete merge, but when we need to show the result concisely in the
"status -s" output, the fact that the index is unmerged is more important
than the incomplete merge result in the working tree is different from the
original, so we show "U".

So I think "U" is perfectly good there.

About the comparison between HEAD and index, "original" in HEAD is copied
to "exact-copy" in the index, and "modified-copy" in the index has a very
similar contents as "original" in HEAD. It may be a bug that the latter is
shown as "R" and I suspect that is because the code mistook the unmerged
entry in the index as missing.  Turning that "R" to "C" may be worth
doing. Change the code that says "ah, the index is unmerged at this path,
so treat it as if it is not there" to "if the unmerged path does not have
stage #2 entry, it is missing".

> There are several errors here: 1) "original" has invalid status " U",
> 2) "modified-copy" is listed as a rename, even though its source
> ("original") still exists, and 3) "exact-copy" is detected as a copy,
> even though 'git status' is only supposed to show renames

The prose is good but if you illustrated a bug with a command output,
please follow it up with another command output that you think is the
right output.  It becomes easier to point out potential flaws in the
design and the thought behind it, like I just did above about "U".

I don't think anybody said "only supposed to show renames", but I suspect
that the recent lt/diff-rename topic may affect this part of the output.

> Fix these problems by making diffcore-rename not consider unmerged
> paths as source for rename detection. For simplicty, don't consider
> the path independent of the type of merge conflict, even if the path
> is deleted on at least one side. Still consider unmerged paths as
> source for copy detection.

I don't think the part after "Still..." is justified enough.

For that matter, everything after "For simplicity..." is not justified
enough. What are you sacrificing in return for that simplicity?  "Ideally
we should show X but because we don't consider these we end up getting Y
but that still is better than what we get today which is Z" is missing.

> Also not sure about the "while at it" stuff...

Because "while at it" is by definition stuff that is not essential, don't
do "white at it" if you are not sure, if it adds unnecessary noise and
burden on reviewers.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]