1. Summarise the issue (what were doing, what went wrong?)

I pulled a change which introduced a conflict. The pull succeeded silently, and Darcs did 
not tell me about the conflict or mark it in the file.

This seems to happen when, within the same pull command, I also pull a tag depending on the 
conflicting patch. (I haven't verified that the tag needs to depend on the patch to trigger 
it.)

Here is a script which reproduces the issue for me:

    # Create repositories "a" and "b"; "b" is a clone of a.
    mkdir a
    cd a
    darcs init
    echo initial>file
    darcs add file
    darcs record -am initial
    cd ..
    darcs clone a b
    
    # Record a change followed by a tag in a.
    cd a
    echo a > file
    darcs record -am a
    darcs tag 'tag a'
    
    # Record a conflicting change in b.
    cd ../b
    echo b > file
    darcs record -am b
    
    # Pull from a to b.
    #
    # This is where the bug appears: darcs should say there's a conflict and
    # mark it, but instead the pull silently succeeds.
    darcs pull -a


2. What behaviour were you expecting instead?

The output of "darcs pull" should tell me about the conflict, and the conflict should be 
marked in file.

3. What darcs version are you using? (Try: darcs --exact-version)

falsifian angel-dfly b $ darcs --exact-version
darcs compiled on Jun 14 2021, at 17:17:37

Weak Hash: not available

Context:


[TAG 2.16.3
Ben Franksen <ben.franksen@online.de>**20201022094642
 Ignore-this: 
bd76d7e31488721e4a5ce7267115e71c3b68d680c155c75c3dd275d9e54933a607d208fab502a143
] 

4. What operating system are you running?

falsifian angel-dfly b $ uname -a
DragonFly angel-dfly.falsifian.org 6.1-DEVELOPMENT DragonFly v6.1.0.83.g556042-DEVELOPMENT 
#0: Thu Jun 17 21:01:19 UTC 2021     build@angel-
dfly.falsifian.org:/usr/obj/usr/src/sys/X86_64_GENERIC  x86_64

This is not a bug, but a feature :), though unfortunately not 
well-documented :(

The general rule is that conflicts are counted as resolved iff there is 
a patch that depends on it. The usual situation is that you record a 
patch whose changes overlap with the conflicted area and therefore 
depends on it. But you can also declare a conflict as resolved with an 
explicit dependency, e.g. using `darcs record --ask-deps` or `darcs 
tag`. This is an important feature because it can happen that the effect 
of a conflictor (which is to remove both conflicting changes) is 
precisely the conflict resolution that you want. In that case depending 
on the conflict explicitly is the only sane way to mark the conflict as 
resolved.

This means that you should not record a tag if there are unresolved 
conflicts unless you want to mark these conflicts as resolved.

I am not closing this ticket to remind us that we need to close this 
hole in the documentation.

To be clear, the tag only depends on one half of the conflict. Is that still the 
desired behaviour?

This seems bad. Consider this scenario:
- Upstream project releases and tags RELEASE_1.
- In my local copy, I fix a bug in RELEASE_1, with patch A.
- Meanwhile, upstream has released and tagged RELEASE_2, which includes a different 
bugfix: patch B.
- I pull RELEASE_2 into my local repo. A and B conflict, but I am never made aware 
of that. My local repo now has neither bug fix.

I could understand if there's a patch C that depends on *both* conflicting patches 
A and B, then the conflict should be considered resolved, for the reason you 
describe. But that's not the situation I'm describing.

You are right. Sorry for dismissing your bug report. I should have 
looked more carefully at the example you provided. Indeed a conflict 
should only be considered resolved if there is a patch that depends on 
all patches involved in the conflict. I suspect that in the scenario you 
described, whether the conflict is considered resolved or not depends on 
the order of patches. That is, if you change the last command to `darcs 
pull -a --reorder-patches`, then darcs will show the conflict. This is 
pretty bad. Will try to find a fix ASAP.

I turned your example into a (failing) test case and played with it. 
Presence or absence of conflict markup does *not* depend on the order of 
patches. In fact, passing --reorder-patches to the pull fails to produce 
conflict markup in exactly the same way, even though the conflicted 
patch 'b' is now on top. The reason --reorder-patches makes no 
difference is that in tentativelyMergePatches_ we pass the un-reordered 
patches to standardResolution regardless of whether --reorder-patches is 
in effect. This is okay because the result should be independent of 
patch ordering anyway.

Notably `darcs mark-conflicts` works just fine in both orderings. The 
difference between mark-conflicts and pull (with regard to conflict 
detection) is that mark-conflicts considers all patches in the repo, 
whereas pull (via tentativelyMergePatches_) only considers the /new/ 
patches. The idea here, explained in the long comment preceding 
tentativelyMergePatches_, is that the current repo may already be 
conflicted and that we don't want to report and mark these already 
existing conflicts.

Why does this problem not appear if we replace the tag with a normal 
patch that (implicitly) depends on patch 'b'? Say

echo c > file
darcs record -am c

Well, the reason is that in this case patch 'c' /also/ becomes 
conflicted and thus does not count as a resolution. Whereas the way we 
handle resolution via explicit dependencies is such that we do not 
consider a conflicted patch at all if anything explicitly depends on it. 
While the other half of the conflict is not depended on, it appears only 
in the context and not in the trailing list of patches we scan for 
conflicts. Indeed, the way resolveConflicts is implemented in the 
instance Conflict (Named p) is buggy and I believe its result /does/ 
depend on patch ordering. This will be difficult to fix.

In order to fix this, we need to change the instance Conflict (Named p) 
so that we regard a conflict as resolved only if some other patch 
explicitly depends on /all/ patches involved in the conflict. 
Unfortunately this information is readily available only for V3 Patches, 
but not for V1 and V2.

This means we have to reconstruct it. We have to commute patches from 
the context past (forward) until the patch becomes unconflicted, then 
minimize that set by re-commuting all of those backwards where that 
keeps the patch unconflicted.

I have implemented this and changed the instance Conflict (Named p) 
accordingly. Indeed this solves the reported problem.

However, it also breaks rebase unsuspend. The reason is that the above 
algorithm relies on an invariant, namely that each conflicted patch 
becomes unconflicted when you commute enough patches from its context 
past. In other words, every patch is unconflicted in its minimal 
context. This is true for the "real" patches contained in a repository, 
but rebase unsuspend violates it: for the sole purpose of generating 
conflict markup when unsuspending, it creates "fake" conflicted patches 
that conflict with patches that do not exist in the context but were 
created using fromAnonymousPrim.

Once again this unclean rebase hack around the notion of "force 
commuting fixups" bites us.

I am not yet sure how to work around this.

Turns out that the solution is simply to not use the instance Conflict 
(Named p). This makes sense, as we don't want to consider explicit 
dependencies anyway when we create the conflict markup for rebase unsuspend.

Will send a bundle that fixes this issue.

Fixed in screened, see

patch 9411e9d8ccfb7e2c52d431c2cf2343018ec3b25b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  5 09:54:17 CEST 2021
  * resolve issue2682: conflict not marked if tag pulled at the same 
time
  
  The fix is to consider a conflict as resolved only if all 
conflicting
  patches are (transitively) explicitly depended on by a single 
patch.

See patch2207