user-guide: add a doc about resolving merge-conflicts#1684
Conversation
efiop
changed the title
| You can safely delete this file and then just run `dvc repro` after the merge is | ||
| done. | ||
|
|
||
| ``` | ||
| prepare: | ||
| cmd: python src/prepare.py data/data.xml | ||
| deps: | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
repro or commit?
Maybe no YAML sample is needed here since the rec is to delete it and regenerate it.
There was a problem hiding this comment.
Better to repro. Commit might have unexpected results.
There was a problem hiding this comment.
Oh I missed this one. OK but maybe we should mention both options?
There was a problem hiding this comment.
Yeah I think if there's more than one option they should at least be briefly mentioned.
There was a problem hiding this comment.
We could mention commit but only as something that people shouldn't really use here because it might bring unexpected results.
There was a problem hiding this comment.
But I would handle that separately later. This doc is about the proper way to do things.
There was a problem hiding this comment.
OK, I'll remove the mention. I don't understand what unexpected results you're referring to though.
There was a problem hiding this comment.
@jorgeorpinel It will commit whatever trash you might have in your output, which might not be what you want.
There was a problem hiding this comment.
Gotcha. I guess the data in the machine that os merging would correspond to the HEAD version so commit only works if you decide to keep that version. So you're right, not worth mentioning that case, probably. Except that commit will be much faster and efficient than repro in that case (unless this run config is in the local machine's run-cache)...
| ### dvc import-url | ||
|
|
||
| Simply remove the hashes |
There was a problem hiding this comment.
Same as dvc import so maybe just combine the H3 headers and merge these sections?
There was a problem hiding this comment.
The yamls are different, so better to keep them separate.
There was a problem hiding this comment.
Exact YAML samples belong to those cmd refs I think. In terms of the merging, which is what this doc is about, seems to be the same so I think we could summarize the doc substantially by merging them 🙂
There was a problem hiding this comment.
Those yamls are not the same and illustrate conflicts in different places, let's leave them be.
There was a problem hiding this comment.
Seems like I'm taking over this one so will combine them. Thanks!
There was a problem hiding this comment.
@jorgeorpinel Your call, but I wouldn't combine them.
There was a problem hiding this comment.
Just noticed your comments here Ruslan, sorry. Hmmm WDYT @shcheklein ? I think if the process is the same for import and import-url then a single section is enough and prevents us from having to maintain 2 YAML examples. On the other hand there's no huge need to optimize the last part of the doc, so no strong opinion actually.
|
Super cool doc, thanks @efiop ! Please let us know when you'd like us to review it. I just did a quick read so far. |
efiop
changed the title
jorgeorpinel
left a comment
jorgeorpinel
left a comment
There was a problem hiding this comment.
Just one pending recommendation from me: #1684 (review)
No super strong opinion though, so feel free to merge this if you disagree.
|
@jorgeorpinel what is the status of this? Can we merge this? Looks like only some minor things left? @efiop @jorgeorpinel should we put some links to @efiop great start, good and simple summary of different possibilities. |
I approved this some time ago 🙂 although
OK hold on... ⌛ |
address all pending feedback so far e.g. #1684 (review) et al.
| - path: data.xml | ||
| ``` | ||
|
|
||
| And then run `dvc repro`. |
There was a problem hiding this comment.
Don't import stages need to be unfrozen first? @efiop
There was a problem hiding this comment.
Good catch! Indeed, better to just use dvc update for imports.
| git config merge.dvc.name "DVC merge driver" | ||
| git config merge.dvc.driver "dvc git-hook merge-driver --ancestor %O --our %A --their %B" |
There was a problem hiding this comment.
Should all these be ' single quotes @efiop ? Or is it all the same.
There was a problem hiding this comment.
Doesn't matter in this case for *nix, but might indeed be problematic for windows. Good catch! Adjusted in 7a65f11 . Thank you! 🙏
3 participants
treeverse/dvc#4298
❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.
🐛 Please make sure to mention
Fix #issue(if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.Please choose to allow us to edit your branch when creating the PR.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏