Fixes #613

Based on #627, but:

• Supports both old (.rej files) and new (inline markers) conflict behavior
• Organizes the code to reduce likelihood of merge conflicts (while this PR is in development/review)

@yajo: This is ready for review. Most of the code is copied from #627; I mainly organized things a bit differently to support both old and new behavior.

As a long-time git user, I'm excited about this feature! Conflicts are scary enough, but they're less scary when presented in a familiar way.

The failed build looks like some sort of technical GitHub or build issue, not something about the PR.

Codecov Report

Merging #807 (7440853) into master (6f6208b) will decrease coverage by 0.02%. The diff coverage is 96.80%.

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
- Coverage   96.58%   96.56%   -0.03%     
==========================================
  Files          41       41              
  Lines        2990     3054      +64     
==========================================
+ Hits         2888     2949      +61     
- Misses        102      105       +3     

Flag	Coverage Δ
unittests	96.56% <96.80%> (-0.03%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
copier/main.py	94.34% <95.83%> (+0.03%)	:arrow_up:
copier/cli.py	96.51% <100.00%> (+0.04%)	:arrow_up:
tests/test_subdirectory.py	100.00% <100.00%> (ø)
tests/test_updatediff.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

I'm happy to remove the old behavior. Before we decide, I'll explain some things:

• In this PR, I took the code directly from #627, without studying or understanding it.
• I think this code does use 'git` to generate the conflict markers directly.

It may be possible to meet your goals of changing less code while replacing the existing behavior, but that'll be more work, possibly a *lot^ more, because it requires a deep understanding of the old and new diff algorithms, and I worry I'll get stuck, then this PR will become stale like #627 did.

My suggestions for the simplest path forward:

• Add the new diff algorithm. Make it the default behavior.
• Keep the old behavior for now. Deprecate it, then remove it in a future release, once the new one has been adopted successfully and proves stable. This eliminates the risk of adopting the new, not well understood diff method.

What do you think? If you like this idea, I can update the PR with the new default and the pre-commit doc change.

Thanks! I understand your position and I thank you for the time you took on updating that pr.

You can leave it as it is. I'll review when I have some time and get to the final solution.

I prefer to maintain just one version of the algorithm though. This subject is not really something that renders copier useless, and there's no hurry. So don't worry, be patient and it will land well supported! ❤ 😊

El sáb., 24 sept. 2022 12:26, Barry Hart @.***> escribió:

I'm happy to remove the old behavior. Before we decide, I'll explain some things:

• In this PR, I took the code directly from #627 https://github.com/copier-org/copier/pull/627, without studying or understanding it.
• I think this code does use 'git` to generate the conflict markers directly.

It may be possible to meet your goals of changing less code while replacing the existing behavior, but that'll be more work, possibly a *lot^ more, because it requires a deep understanding of the old and new diff algorithms, and I worry I'll get stuck, then this PR will become stale like #627 https://github.com/copier-org/copier/pull/627 did.

My suggestions for the simplest path forward:

• Add the new diff algorithm. Make it the default behavior.
• Keep the old behavior for now. Deprecate it, then remove it in a future release, once the new one has been adopted successfully and proves stable. This eliminates the risk of adopting the new, not well understood diff method.

What do you think? If you like this idea, I can update the PR with the new default and the pre-commit doc change.

— Reply to this email directly, view it on GitHub https://github.com/copier-org/copier/pull/807#issuecomment-1256946274, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHNXDJSXE7BK47TEZ567YLV73QPNANCNFSM6AAAAAAQPM6O6M . You are receiving this because you were mentioned.Message ID: @.***>

Ok, thanks!

@yajo: Please let me know how things are going. At work, I may have time between other tasks to work more with this. When I suggested having two methods, I wanted to let you decide about the tradeoff (quick vs slow, 2 diff methods vs 1). Like you, I wonder if it's possible to generate the new diff format with fewer changes to the current code. Intuitively, I would think you could just run a different git command. (IIRC, it's git apply versus git am.)

In the meantime, this PR is much less likely to develop conflicts than the old one was, because of how I structured it. So no pressure on either of us. 😅

This is a very useful project. Thank you for helping maintain it! I'm a maintainer on another project (SQLFluff), so I know the ups and downs of that.

@yajo: I looked over the PR and found a lot of unnecessary and/or duplicate code. I deleted what I could, and I created some helper functions that reduce the duplicate code between the old and new conflict behaviors. Now there is just one function (Worker._apply_update_inline_conflict_markers()) that differs between the old and new behavior. I think this will make reviewing it much simpler.

I'm 💯 with removing the old behavior, but I think having them both available, at least within this PR may be helpful for review and test. Once you review this and feel comfortable with the new implementation, feel free to close this and create a new PR with the old behavior removed, or ping me and I'll update this PR. (Note that a few existing tests will need updating because they currently check for the old behavior.)

@yajo: Do you think you may be able to review this soon? We'd really like to use this new behavior on our projects.

Thanks for the thorough review, @yajo. I think I've addressed all of your suggestions.

I really appreciate your work maintaining this project. 🙌💪

Please let me know if you see anything else that needs to be improved.

Looks like a back-end issue with the code coverage reporting (likely not related to this PR):

Codecov report uploader 0.3.2
[2022-11-07T10:02:24.314Z] ['info'] => Project root located at: /home/runner/work/copier/copier
[2022-11-07T10:02:24.315Z] ['info'] -> No token specified or token is empty
[2022-11-07T10:02:24.323Z] ['info'] Searching for coverage files...
[2022-11-07T10:02:24.620Z] ['info'] => Found 1 possible coverage files:
  ./coverage.xml
[2022-11-07T10:02:24.621Z] ['info'] Processing ./coverage.xml...
[2022-11-07T10:02:24.625Z] ['info'] Detected GitHub Actions as the CI provider.
[2022-11-07T10:02:24.627Z] ['info'] Pinging Codecov: https://codecov.io/upload/v4?package=github-action-3.1.1-uploader-0.3.2&token=*******&branch=flexible_conflict_resolution&build=3404684625&build_url=https%3A%2F%2Fgithub.com%2Fcopier-org%2Fcopier%2Factions%2Fruns%2F3404684625&commit=f0e067871d119564053c1e78af8d7[44](https://github.com/copier-org/copier/actions/runs/3404684625/jobs/5670911211#step:12:45)58022aac1&job=CI&pr=807&service=github-actions&slug=copier-org%2Fcopier&name=copier&tag=&flags=unittests&parent=
[2022-11-07T10:02:24.919Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}

Codecov again 🤦 I personally stopped using such services, they're just noise IMO.

Codecov problem fixed in #861. Please rebase to get the fix.

However, the logs reveal real test failures, so please could you fix them? Thanks!

@yajo: I rebased and pushed. I didn't check the build log before pushing, but all tests are passing locally on my Mac. Can you please trigger the build so I can see the failures? Thanks!

443 passed, 1 skipped, 2 xfailed, 338 warnings in 91.64s (0:01:31)

Ok, I made that change -- sorry about the issue! I'm developing on a Mac. 😿

It seems the fix was not correct. Could you check out windows test error logs please.

@yajo: I don't have a convenient Windows machine I can test on, but I added some code to normalize the line endings before comparing the strings. I'm pretty confident this will work.

Something strange happened with the tests. I'm going to revert my changes to the test, because the build was working before that. Can we please merge this, and then make any final adjustments to the tests in a separate PR?

Part of the challenge for me now is that I make a change, then have to wait hours or a day for the test run to be approved. It's a very slow workflow for small changes.

@yajo: I got the Windows test working with a workaround. Apparently, git is introducing line-ending issues when it's inserting inline conflict markers on Windows. I tried various settings for git's core.autocrlf and .gitattributes. Any other combination of settings resulted in one or both of:

• Duplicate newline characters ("\n\n")
• Duplicate carriage-return characters ("\r\r\n")

While the test does work now, perhaps we should document that caution is advised when using inline mode on Windows, or disable the feature. What do you think?

The test looks ugly now, but it's better an ugly test than a skipped test. 😉

So let's leave it as it is. The feature is flagged as experimental, so there's no need to disable it in Windows. I think that the best thing you could do with your discoveries is to open an issue for Git itself.

I'll look into reporting the issue to git. Apparently that's done using an email list, not through GitHub like most projects.

I reported the issue to Windows git (separate project from Unix git): https://github.com/git-for-windows/git/issues/4113