Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better handling of add/add conflicts #728

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Better handling of add/add conflicts #728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
675e870
add test for add/add non-conflict
novalis Sep 6, 2019
aa7bb4b
Handle nulls in formatting for repo ast operations.
novalis Sep 6, 2019
ea3c3aa
Test that add/add conflicts happen
novalis Sep 6, 2019
34ed09b
give a more verbose message on add/add conflicts
novalis Sep 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions node/lib/util/cherry_pick_util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,9 @@ exports.computeChanges = co.wrap(function *(repo, index, targetCommit) {
* `repo`. If `conflicts` is non-empty, return a non-empty string desribing
* them. Otherwise, return the empty string.
*
* Half-open the conflicting repos and fetch all commits that are in the
* conflict; this will make it easier for users to resolve the conflict.
*
* @param {NodeGit.Repository} repo
* @param {NodeGit.Index} index
* @param {Object} conflicts from sub name to `Conflict`
Expand All @@ -523,11 +526,47 @@ exports.computeChanges = co.wrap(function *(repo, index, targetCommit) {
exports.writeConflicts = co.wrap(function *(repo, index, conflicts) {
let errorMessage = "";
const names = Object.keys(conflicts).sort();
const opener = new Open.Opener(repo, null);
const fetcher = yield opener.fetcher();
for (let name of names) {
yield ConflictUtil.addConflict(index, name, conflicts[name]);
let conflict = conflicts[name];
let configured = false;
try {
// We just want to see if it's configured
const url = yield fetcher.getSubmoduleUrl(name);
configured = true;
} catch (e) {
// nope, so we cannot fetch
}
if (configured) {
const bare = Open.SUB_OPEN_OPTION.FORCE_BARE;
const subRepo = yield opener.getSubrepo(name, bare);

for (const stage of [conflict.ancestor, conflict.our,
conflict.their]) {
if (stage !== null) {
yield fetcher.fetchSha(subRepo, name, stage.id);
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

yield ConflictUtil.addConflict(index, name, conflict);
errorMessage += `\
Conflicting entries for submodule ${colors.red(name)}
Conflicting entries for submodule ${colors.red(name)}.
`;
if (conflict.our !== null && conflict.their !== null) {
// add-add
const root = repo.workdir();
const our = conflict.our.id;
const their = conflict.their.id;
errorMessage += `
To choose your version of the ${name}, use the following magic:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

git -C ${root} update-index --cache-info 160000,${our},${name}
To choose their version of the ${name}, use the following magic:
git -C ${root} update-index --cache-info 160000,${their},${name}
To compare, try:
git -C ${root}${name} diff ${their} ${our}
`;
}
}
return errorMessage;
});
Expand Down
6 changes: 6 additions & 0 deletions node/lib/util/repo_ast_util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ function diffObjects(first,
* @return {String}
*/
function colorExp(text) {
if (text === null) {
text = "(null)";
}
return colors.green(text);
}

Expand All @@ -108,6 +111,9 @@ function colorExp(text) {
* @return {String}
*/
function colorAct(text) {
if (text === null) {
text = "(null)";
}
return colors.yellow(text);
}

Expand Down
4 changes: 4 additions & 0 deletions node/lib/util/test_util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,3 +281,7 @@ exports.makeBareCopy = co.wrap(function *(repo, path) {
yield NodeGit.Remote.delete(bare, "origin");
return bare;
});

exports.quotemeta = function(str) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you explain briefly why we suddenly need this helper now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we're now doing regex matching.

return String(str).replace(/\W/g, "\\$&");
};
24 changes: 18 additions & 6 deletions node/test/util/cherry_pick_util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,11 @@ const Open = require("../../lib/util/open");
const RepoASTTestUtil = require("../../lib/util/repo_ast_test_util");
const Submodule = require("../../lib/util/submodule");
const SubmoduleChange = require("../../lib/util/submodule_change");
const TestUtil = require("../../lib/util/test_util");
const UserError = require("../../lib/util/user_error");

const qm = TestUtil.quotemeta;

/**
* Return a commit map as expected from a manipulator for `RepoASTTestUtil`
* from a result having the `newMetaCommit` and `submoduleCommits` properties
Expand Down Expand Up @@ -558,11 +561,12 @@ describe("writeConflicts", function () {
},
expected: "x=E:I *README.md=~*~*S:1;W README.md=hello world",
result: `\
Conflicting entries for submodule ${colors.red("README.md")}
Conflicting entries for submodule ${colors.red("README.md")}.
`,
},
"two conflicts": {
state: "x=S",

conflicts: {
z: new Conflict(null,
null,
Expand All @@ -573,8 +577,8 @@ Conflicting entries for submodule ${colors.red("README.md")}
},
expected: "x=E:I *z=~*~*S:1,*a=~*~*S:1",
result: `\
Conflicting entries for submodule ${colors.red("a")}
Conflicting entries for submodule ${colors.red("z")}
Conflicting entries for submodule ${colors.red("a")}.
Conflicting entries for submodule ${colors.red("z")}.
`,
},
};
Expand Down Expand Up @@ -737,7 +741,7 @@ a
;
`,
errorMessage: `\
Submodule ${colors.red("s")} is conflicted.
Submodule ${qm(colors.red("s"))} is conflicted.*
`,
},
"conflict in a sub pick, success in another": {
Expand All @@ -755,7 +759,7 @@ a
;
`,
errorMessage: `\
Submodule ${colors.red("s")} is conflicted.
Submodule ${qm(colors.red("s"))} is conflicted.
`,
},
};
Expand All @@ -768,7 +772,15 @@ Submodule ${colors.red("s")} is conflicted.
const eightCommitSha = reverseCommitMap["8"];
const eightCommit = yield x.getCommit(eightCommitSha);
const result = yield CherryPickUtil.rewriteCommit(x, eightCommit);
assert.equal(result.errorMessage, c.errorMessage || null);
const errorMessage = c.errorMessage || null;


if (null === result.errorMessage) {
assert(null === errorMessage);
} else {
const re = new RegExp(c.errorMessage);
assert.match(result.errorMessage, re);
}
return mapCommits(maps, result);
});

Expand Down
14 changes: 11 additions & 3 deletions node/test/util/merge_full_open.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const MergeUtil = require("../../lib//util/merge_util");
const MergeCommon = require("../../lib//util/merge_common");
const RepoASTTestUtil = require("../../lib/util/repo_ast_test_util");
const Open = require("../../lib/util/open");
const TestUtil = require("../../lib/util/test_util");

/**
* Return the commit map required by 'RepoASTTestUtil.testMultiRepoManipulator'
Expand Down Expand Up @@ -315,7 +316,7 @@ x=S:C2-1 s=Sa:a;C3-1 s=Sa:b;Bmaster=2;Bfoo=3`,
fails: true,
expected: `x=E:Qmessage\n#M 2: 3: 0 3;I *s=~*S:a*S:b`,
errorMessage: `\
Conflicting entries for submodule ${colors.red("s")}
Conflicting entries for submodule ${TestUtil.quotemeta(colors.red("s"))}.*
`,
},
"conflict in submodule": {
Expand All @@ -325,7 +326,7 @@ x=U:C3-2 s=Sa:a;C4-2 s=Sa:b;Bmaster=3;Bfoo=4`,
fromCommit: "4",
fails: true,
errorMessage: `\
Submodule ${colors.red("s")} is conflicted.
Submodule ${TestUtil.quotemeta(colors.red("s"))} is conflicted.
`,
expected: `
x=E:Qmessage\n#M 3: 4: 0 4;
Expand Down Expand Up @@ -410,7 +411,14 @@ x=S:C2-1 r=Sa:1,s=Sa:1,t=Sa:1;
message,
editMessage);
const errorMessage = c.errorMessage || null;
assert.equal(result.errorMessage, errorMessage);

if (null === result.errorMessage) {
assert(null === errorMessage);
} else {
const re = new RegExp(c.errorMessage);
assert.match(result.errorMessage, re);
}

if (upToDate) {
assert.isNull(result.metaCommit);
return; // RETURN
Expand Down
35 changes: 31 additions & 4 deletions node/test/util/rebase_util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const RebaseUtil = require("../../lib/util/rebase_util");
const RepoASTTestUtil = require("../../lib/util/repo_ast_test_util");
const SequencerState = require("../../lib/util/sequencer_state");
const SequencerStateUtil = require("../../lib/util/sequencer_state_util");
const TestUtil = require("../../lib/util/test_util");

const CommitAndRef = SequencerState.CommitAndRef;
const REBASE = SequencerState.TYPE.REBASE;
Expand Down Expand Up @@ -338,13 +339,35 @@ x=S:C2-1 s=Sa:1,t=Sb:1;C3-2 s=Sa:j;C4-2 t=Sb:k;Bmaster=3;Bfoo=4;Bold=3`,
expected: `
x=E:C3M-4 s=Sa:j;Bmaster=3M`,
},
"adding subs on both": {
"adding unrelated subs on both": {
initial: `
q=B|r=B|s=B|x=S:C2-1 s=Ss:1;C3-2 q=Sq:1;C4-2 r=Sr:1;Bmaster=3;Bfoo=4;Bold=3`,
onto: "4",
expected: `
x=E:C3M-4 q=Sq:1;Bmaster=3M`,
},
"adding same sub on both": {
initial: `
q=B|r=B|s=B|x=S:C2-1 s=Ss:1;C3-2 r=Sr:1,s=Ss:2;C4-2 r=Sr:1,q=Sq:1;Bmaster=3;
Bfoo=4;Bold=3`,
onto: "4",
expected: `
x=E:C3M-4 s=Ss:2;Bmaster=3M`,
},
"add/add conflict": {
initial: `
q=B|r=B:C5-1;C6-5;Bother=6|
s=B|
x=S:C2-1 s=Ss:1;C3-2 r=Sr:6,s=Ss:2;C4-2 r=Sr:5,q=Sq:1;
Bmaster=3;Bfoo=4;Bold=3`,
onto: "4",
errorMessage:
"Conflicting entries for submodule .*",
expected: `
x=E:H=4;QR 3:refs/heads/master 4: 0 3;
I s=Ss:2,
*r=~*S:5*S:6`,
},
"open sub ffwd'd": {
initial: `
a=B:CX-1;Bmaster=X|
Expand All @@ -368,7 +391,7 @@ t
>>>>>>> message
;`,
errorMessage: `\
Submodule ${colors.red("s")} is conflicted.
Submodule ${TestUtil.quotemeta(colors.red("s"))} is conflicted.*
`,
},
"does not close open submodules when rewinding": {
Expand Down Expand Up @@ -399,10 +422,14 @@ a=B|x=S:C2-1 s=Sa:1;Bmaster=2;Os;C3-1 t=Sa:1;Bfoo=3;Bold=2`,
const onto = yield repo.getCommit(reverseCommitMap[c.onto]);
const errorMessage = c.errorMessage || null;
const result = yield RebaseUtil.rebase(repo, onto);
if (null !== result.errorMessage) {

if (null === result.errorMessage) {
assert(errorMessage === null);
} else {
assert.isString(result.errorMessage);
const re = new RegExp(errorMessage);
assert.match(result.errorMessage, re);
}
assert.equal(result.errorMessage, errorMessage);
return result;
});
const rebase = makeRebaser(rebaseOp);
Expand Down