Better handling of add/add conflicts#728
Conversation
It would be nice to do something about them, but at least we can test that we're not totally doomed.
| const our = conflict.our.id; | ||
| const their = conflict.their.id; | ||
| errorMessage += ` | ||
| To choose your version of the ${name}, use the following magic: |
There was a problem hiding this comment.
is this the only/recommended way to resolve add-add conflict? Should we trust our user to resolve the conflict manually? I am afraid people may blinded run them and override upstream changes.
There was a problem hiding this comment.
The third way is more complicated: we could see if the two sets share a merge-base and if so rebase inside the submodule. That's a TODO -- it's one reason why I did the half-open. But I would guess that this same merge-base thing only happens in some small number of cases; mostly add/adds are e.g. different people importing the same ext codebase.
People might do something stupid, and if so it'll show up in code review. So hopefully someone will notice.
| return bare; | ||
| }); | ||
|
|
||
| exports.quotemeta = function(str) { |
There was a problem hiding this comment.
could you explain briefly why we suddenly need this helper now?
There was a problem hiding this comment.
Because we're now doing regex matching.
| yield fetcher.fetchSha(subRepo, name, stage.id); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a useful and orthogonal piece of code, could we make it a util function, something like "fetch sha for half opend subrepo if needed"
There was a problem hiding this comment.
The question is: what sha? Here, we loop over the stages, which is what's needed in this case. But is that really generally useful?
novalis
force-pushed
the
more-rebase
branch
2 times, most recently
from
01ecd75 to
98d4a54
Compare
|
(just removed a few console.logs) |
node/lib/util/cherry_pick_util.js
Outdated
| try { | ||
| // We just want to see if it's configured | ||
| const url = yield fetcher.getSubmoduleUrl(name); | ||
| configured = url !== null; |
There was a problem hiding this comment.
There's nothing in the contract of getSubmoduleUrl that says it might return a null; what's the check for?
There was a problem hiding this comment.
Well, it definitely happens in the tests...
There was a problem hiding this comment.
I can't see how it could return a null after a (quick) look at the implementation, but if it can the contract should be updated to explain when; as it is the contract is to throw a UserError on failure.
There was a problem hiding this comment.
if this.d_urls[name] actually contains null, that would happen. I think this may only happen in tests. I could change the code to instead detect that case and throw UserError (then catch it here instead of checking for null) -- what do you think?
There was a problem hiding this comment.
I'd want to understand how this.d_urls comes to have null values first, and what it means.
There was a problem hiding this comment.
Oh, wait a second -- I think I just checked the wrong thing. In fact it's not null. I could just set configured to true here.
novalis
force-pushed
the
more-rebase
branch
from
98d4a54 to
34ed09b
Compare
3 participants
Well, I didn't get everything I wanted done, but at least I got this.