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.

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

Allow normal merge to operate on bare and half opened repos #690

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Projects
None yet
3 participants
@shijinglu
Copy link
Collaborator

shijinglu commented Feb 1, 2019

Add an option to git meta merge to allow it choose working directory mode:

  1. open submodule is not allowed
  2. always fully open submodule
  3. open submodule only when manually conflict resolution is required.

Also refactor the existing merge module to make it less monolithic

@novalis
Copy link
Collaborator

novalis left a comment

I think I haven't reviewed this whole thing but I'm getting tired so this is a start.

@@ -91,6 +91,13 @@ exports.configureParser = function (parser) {
constant: true,
help: "abort an in-progress merge",
});
parser.addArgument(["--open-submodule"], {

This comment has been minimized.

@novalis

novalis Feb 1, 2019

Collaborator

Do we want "always"?

This comment has been minimized.

@shijinglu

shijinglu Feb 1, 2019

Author Collaborator

"always" is the current behavior, if it is not needed, then this can be just a boolean flag like "--bare"

This comment has been minimized.

@shijinglu

shijinglu Feb 1, 2019

Author Collaborator

actually, there is no need to expose bare merge mode for end users. I am going to remove the option and make allow_bare the default option.

const result =
yield MergeUtil.merge(repo, commit, mode, args.message, editMessage);
let wdType;
switch (args.open_submodule.toLowerCase()) {

This comment has been minimized.

@novalis

novalis Feb 1, 2019

Collaborator

Couldn't we just use an object mapping from word to enum value?

This comment has been minimized.

@shijinglu

shijinglu Feb 6, 2019

Author Collaborator

switch statement removed

});
parser.addArgument(["theirCommit"], {
type: "string",
help: "their side commitish to merge",
nargs: 1,
nargs: "?",

This comment has been minimized.

@novalis

novalis Feb 1, 2019

Collaborator

Is this change really correct? Don't we later check if these are null and fail if so?

This comment has been minimized.

@shijinglu

shijinglu Feb 6, 2019

Author Collaborator

updated

@@ -43,6 +43,17 @@ const SubmoduleUtil = require("./submodule_util");
const SubmoduleConfigUtil = require("./submodule_config_util");
const SubmoduleFetcher = require("./submodule_fetcher");

/**
* @enum {WDTYPE}
* Flags that describe what working directory type the repo is.

This comment has been minimized.

@novalis

novalis Feb 1, 2019

Collaborator

This is really describing not just the type of the git repo, but also the action to be performed on submodules, right?

This comment has been minimized.

@shijinglu

shijinglu Feb 1, 2019

Author Collaborator

right, if we "always" open submodule is not necessary, we don't even need an enum here.

Show resolved Hide resolved node/lib/util/merge_common.js Outdated
const fromCommit = yield subRepo.getCommit(fromSha);
/**
* Write tree representation of the index to the disk, create a commit
* from the tree and update reference if needed.

This comment has been minimized.

@novalis

novalis Feb 1, 2019

Collaborator

So, this is sort of odd. Why not have the merge command update the ref (and the merge-bare command not)? That is, try to avoid having a method that does two different things.

This comment has been minimized.

@shijinglu

shijinglu Feb 1, 2019

Author Collaborator

Now it is the case that interactive merge update the ref ("HEAD") and the bare merge not. But interactive merge does more than that:

  1. it operates directly on repo's current index, so users can see all the file changes if merge is paused for user actions.
  2. it pulls necessary changes when needed
  3. it force resets subrepo's head when needed

This comment has been minimized.

@novalis

novalis Feb 1, 2019

Collaborator

The first is just a question of what index is used, right?

The second of these should be true for both kinds of merge, right? Unless by "pull", you really mean "pull" as in fetch+merge.

Even if 1 and 2 do really both need to be conflated into this method, it does not follow that 3 does. Our goal is clean code, which means separation of concerns as much as possible.

This comment has been minimized.

@shijinglu

shijinglu Feb 6, 2019

Author Collaborator

Ok, let's move the logic to MergeContext then.

Show resolved Hide resolved node/lib/util/status_util.js
Show resolved Hide resolved node/lib/util/status_util.js
Show resolved Hide resolved node/lib/util/open.js Outdated
Show resolved Hide resolved node/lib/util/open.js Outdated
@bpeabody

This comment has been minimized.

Copy link
Contributor

bpeabody commented Feb 1, 2019

If we have this capability, I don't see any reason to expose the options to end users; we should open submodules only when absolutely necessary.

@shijinglu shijinglu changed the title Allow normal merge to operates on bare and half opened repos Allow normal merge to operate on bare and half opened repos Feb 1, 2019

@bpeabody

This comment has been minimized.

Copy link
Contributor

bpeabody commented Feb 1, 2019

Also, please get @harikmenon to sign of on any UX changes.

@novalis

This comment has been minimized.

Copy link
Collaborator

novalis commented Feb 1, 2019

@shijinglu shijinglu force-pushed the shijinglu:merge_refactor branch 3 times, most recently from 4077542 to 590bbdc Feb 1, 2019

}

/**
* @property {Stirng|null}

This comment has been minimized.

@novalis

novalis Feb 5, 2019

Collaborator

Spelling of String

This comment has been minimized.

@shijinglu

shijinglu Feb 5, 2019

Author Collaborator

ack

* @param {Open.Opener} opener
* @param {NodeGit.Index} metaIndex
* @param {Object} subs map from name to SubmoduleChange
* Get the left side commit and sha for submodule `subRepo` based on workding

This comment has been minimized.

@novalis

novalis Feb 5, 2019

Collaborator

spelling of working

This comment has been minimized.

@novalis

novalis Feb 5, 2019

Collaborator

but also "based on working directory type" doesn't really make a lot of sense to me.

This comment has been minimized.

@shijinglu

shijinglu Feb 6, 2019

Author Collaborator

actually, after revisiting the compute changes logic, the left side commit for submodules have already computed correctly in CherryPickUtil.computeChangesBetweenTwoCommits, so let me remove this

* @async
* @param {NodeGit.Repository} subRepo
* @param {SubmoduleChange} subChange
* @param {SUB_OPEN_OPTION} openOption

This comment has been minimized.

@novalis

novalis Feb 5, 2019

Collaborator

Like, this isn't really about our opening policy -- it's about what we're merging. There are two options: (a) our left side is our current state (working tree / index), vs (b) our left side is an arbitrary sha.

This comment has been minimized.

@shijinglu

shijinglu Feb 6, 2019

Author Collaborator

correct. So basically the logic is now like this:

  1. merge command sets the ourCommit to null and theirCommit equal to a sha in the MergeContext. And MergeContext then set ourCommit to HEAD if current repo has a HEAD, and raises an error if we are merging in a bare repo.
  2. merge-bare sets both ourCommit and theirCommit
  3. left side commits of the merge for each submodule are also computed in the MergeContext based on the meta repo merging commits
@@ -43,6 +43,17 @@ const SubmoduleUtil = require("./submodule_util");
const SubmoduleConfigUtil = require("./submodule_config_util");
const SubmoduleFetcher = require("./submodule_fetcher");

/**
* @enum {SUB_OPEN_OPTION}
* Flags that describe wheter to open a submodule if it is part of a merge.

This comment has been minimized.

@novalis

novalis Feb 5, 2019

Collaborator

Spelling of whether

This comment has been minimized.

@shijinglu

shijinglu Feb 5, 2019

Author Collaborator

ack

* Flags that describe wheter to open a submodule if it is part of a merge.
*/
const SUB_OPEN_OPTION = {
FORCE_OPEN : 0, // non-bare repo and open submodule when needed

This comment has been minimized.

@novalis

novalis Feb 5, 2019

Collaborator

"when needed" and "force" seem like an odd fit. Also, do we actually want this ever?

This comment has been minimized.

@shijinglu

shijinglu Feb 5, 2019

Author Collaborator

yes, we still need this option for at least two reasons:

  1. FORCE_OPEN means "open the submodule when creating the subRep object". And creating the subRepo object is called in many places, not just the merge. For example the subRepo objects are needed in reset, stash, commit etc. It is hard to test if half open submodules work for all those case. So to be safe let's allow fully opening in other commands.
  2. Now the default option for normal merge is ALLOW_BARE, but in case there are bugs in its workflow, we can quickly switch back to old merge workflow with FORCE_OPEN instead of going through painful git-revert process.
mergeStepMergeSubmodules,
];

for (let i = 0; i < mergeAsyncSteps.length; i++) {

This comment has been minimized.

@novalis

novalis Feb 5, 2019

Collaborator

for (const asyncStep of asyncSteps)

This comment has been minimized.

@shijinglu

shijinglu Feb 6, 2019

Author Collaborator

ack

@shijinglu shijinglu force-pushed the shijinglu:merge_refactor branch from 590bbdc to 22ad874 Feb 5, 2019

@shijinglu
Copy link
Collaborator Author

shijinglu left a comment

mark some typos as ack-ed, and will update them in the next update.

@shijinglu shijinglu force-pushed the shijinglu:merge_refactor branch from 22ad874 to 208f625 Feb 5, 2019

@novalis

This comment has been minimized.

Copy link
Collaborator

novalis commented Feb 6, 2019

@shijinglu

This comment has been minimized.

Copy link
Collaborator Author

shijinglu commented Feb 6, 2019

I don't think any of those three cases could possibly require opening a submodule. Unless you mean "stash apply"?

yah, stash apply opens the submodule, also git meta amend opens the submodule as well. cherry-pick-abort when it resets the HEAD to some commit, it also opens the submodule

@shijinglu shijinglu force-pushed the shijinglu:merge_refactor branch from 208f625 to e9f78c8 Feb 6, 2019

@novalis

This comment has been minimized.

Copy link
Collaborator

novalis commented Feb 6, 2019

I don't think any of those three cases could possibly require opening a submodule. Unless you mean "stash apply"?

yah, stash apply opens the submodule, also git meta amend opens the submodule as well. cherry-pick-abort when it resets the HEAD to some commit, it also opens the submodule

(not sure github is going to put this comment in the right place, but why would commit --amend need to open a submodule? And why would cherry-pick --abort need to do so?

@shijinglu shijinglu force-pushed the shijinglu:merge_refactor branch from e9f78c8 to 355ff12 Feb 6, 2019

@shijinglu

This comment has been minimized.

Copy link
Collaborator Author

shijinglu commented Feb 7, 2019

(not sure github is going to put this comment in the right place, but why would commit --amend need to open a submodule? And why would cherry-pick --abort need to do so?

But the two are know situations and our users often rely on git meta commit --amend to open submodules. Is opening submodules necessary, may be may be now. But I think it is not up to this PR to answer these two questions. Let's keep these behaviors and if we feel necessary, we can always come back and remove FORCE_OPEN later.

* @param {SubmoduleFetcher} fetcher helper to fetch commits in the sub
* @param {NodeGit.Signature} sig default signature
* @param {Open.Opener} opener helper to open a sub
* @param {SUB_OPEN_OPTION} openOption opention to open a sub

This comment has been minimized.

@novalis

novalis Feb 7, 2019

Collaborator

spelling of option.

This comment has been minimized.

@shijinglu

shijinglu Feb 7, 2019

Author Collaborator

ack

@@ -111,7 +113,7 @@ x=U:C3-2 s=Sa:b;C4-2 s=Sa:a;Bmaster=3;Bfoo=4`,
theirCommit: "4",
ourCommit: "3",
},
"non-ffmerge with ffwd submodule change": {
"non-ffmerge with ffwd submodule change xxxx": {

This comment has been minimized.

@novalis

novalis Feb 7, 2019

Collaborator

What's the xxxx?

This comment has been minimized.

@shijinglu

shijinglu Feb 7, 2019

Author Collaborator

ahh, marked xxxx for debugging purpose, let me remove it

@@ -378,6 +380,7 @@ a=B:Ca-1;Cb-1;Ba=a;Bb=b|
x=S:C2-1 s=Sa:a;C3-1 s=Sa:b;Bmaster=2;Bfoo=3`,
fromCommit: "3",
expected: `x=E:Qmessage\n#M 2: 3: 0 3;I *s=~*S:a*S:b`,
fails: true,

This comment has been minimized.

@novalis

novalis Feb 7, 2019

Collaborator

Why are these two tests now expected to fail?

This comment has been minimized.

@shijinglu

shijinglu Feb 7, 2019

Author Collaborator

Originally errors are handled in two fashions: (1) merge_utils returns a object that contains errorMessage and merge command throws a UserError if errorMessage is not empty. and (2) merge_utils throws UserError directly.

Now all errors from the MergeUtil module are thrown by MergeUtil. This test case was the case where MergeUtil returns an errorMessage but now throws an error. Hence it was considered as not fail before but actually it fails.

@shijinglu shijinglu force-pushed the shijinglu:merge_refactor branch from 355ff12 to a745816 Feb 7, 2019

@novalis

novalis approved these changes Feb 7, 2019

@novalis novalis merged commit 1818721 into twosigma:master Feb 7, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment