Re: [PATCH v2 3/3] git-merge-one-file: revise merge error reporting
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 25, 2013, 17:17 UTC
- Message-ID
- <7vfvzjz9ej.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <7vk3ovz9zb.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
Show 31 quoted lines
> Kevin Bracey <kevin@bracey.fi> writes: > >> Commit 718135e improved the merge error reporting for the resolve >> strategy's merge conflict and permission conflict cases, but led to a >> malformed "ERROR: in myfile.c" message in the case of a file added >> differently. >> >> This commit reverts that change, and uses an alternative approach without >> this flaw. >> >> Signed-off-by: Kevin Bracey <kevin@bracey.fi> > > We used to treat "Both added differently" as a separate "info" > message, just like the "Auto-merging" message, and let "content > conflict" that is an "error" to happen naturally by doing such a > merge, possibly followed by permission conflict which is another > kind of "error". We coalesced these two into a single message. > > And this patch breaks them into separate messages. I am not sure if > that aspect of the change is desirable. > > The source of "malformed" message seems suspicious. Isn't the root > cause of $msg being empty that merge-file can (sometimes) cleanly > merge two files using the phoney base in the "both added > differently" codepath? > > If you resolve that issue by forcing a "conflicted" failure when we > handle "add/add" conflict, I think the behaviour of the remainder of > the code is better in the original than the updated one. > > Perhaps something like this (I am applying these on 'maint')?
Actually, this one is even better, I think. Again on top of your two patches applied on 'maint'.
Alternatively, we can remove the whole "if $1 is empty, error the merge out" logic, which would be more in line with the spirit of f7d24bbefb06 (merge with /dev/null as base, instead of punting O==empty case, 2005-11-07), but that will be a change in behaviour (a "both side added, slightly differently" case that can cleanly merge will no longer fail), so I am not sure if it is worth it.
-- >8 -- Subject: [PATCH] merge-one-file: force content conflict for "both side added" case
Historically, we tried to be lenient to "both side added, slightly differently" case and as long as the files can be merged using a made-up common ancestor cleanly, since f7d24bbefb06 (merge with /dev/null as base, instead of punting O==empty case, 2005-11-07). This was later further refined to use a better made-up common file with fd66dbf5297a (merge-one-file: use empty- or common-base condintionally in two-stage merge., 2005-11-10), but the spirit has been the same.
But the original fix in f7d24bbefb06 to avoid punging on "both sides added" case had a code to unconditionally error out the merge. When this triggers, even though the content-level merge can be done cleanly, we end up not saying "content conflict" in the message, but still issue the error message, showing "ERROR: in <pathname>".
Signed-off-by: Junio C Hamano <gitster@pobox.com> --- git-merge-one-file.sh | 1 + 1 file changed, 1 insertion(+)
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh index 25d7714..62016f4 100755 --- a/git-merge-one-file.sh +++ b/git-merge-one-file.sh @@ -155,6 +155,7 @@ case "${1:-.}${2:-.}${3:-.}" in fi if test -z "$1" then + msg='content conflict' ret=1 fi
-- 1.8.2-297-g51e0fcd