Skip to content
This repository has been archived by the owner on Jul 13, 2024. It is now read-only.

fix wrong arrow angle on branching #171

Conversation

yodalee
Copy link
Contributor

@yodalee yodalee commented Jul 22, 2017

Relate Issue:
Issue #159

The Problem:
The following source code will create wrong branch arrow.

var graph1 = new GitGraph({
  elementId: 'badGraph',
  mode: 'compact',
  template: 'blackarrow'
});
var m1 = graph1.branch("master");
m1.commit();
var r1 = m1.branch('1.0.x');
m1.commit();
r1.commit();

The arrow pointed to r1 commit will pointed from second commit of m1
not follow the line from first commit of m1.

The Cause:
When create a commit, if user does not specify parent commit in options
gitgraph.js will execute following code:

options.parentCommit = options.parentCommit || _getParentCommitFromBranch(this);

to determine the parent commit of the created commit.
In the function _getParentCommitFromBranch, it will try to get the last
commit from current branch, then from parent branch. Since r1.commit()
is the first commit in its branch 1.0.x, it will set its parent commit
to last commit of its parent branch, that is master. And since master
already add some new commit, the arrow will point from wrong commit, the
last commit of master branch.

There is code that deal with branch case:

if (options.parentCommit instanceof Commit === false && this.parentBranch instanceof Branch) {
  options.parentCommit = this.parentCommit;
}

It set parentCommit to the correct commit. However, since
options.parentCommit already been set by previous code, this code will
never execute since options.parentCommit is always a Commit.

The fix:
The fix is combining two scenario. If current branch already has commits,
the parentCommit will be set to last commit in this branch. Otherwise,
it will set to parent commit of this branch, which will be the correct commit
instead of last commit of parent branch.

Relate Issue:
Issue nicoespeon#159

The Problem:
The following source code will create wrong branch arrow.
```
var graph1 = new GitGraph({
  elementId: 'badGraph',
  mode: 'compact',
  template: 'blackarrow'
});
var m1 = graph1.branch("master");
m1.commit();
var r1 = m1.branch('1.0.x');
m1.commit();
r1.commit();
```
The arrow pointed to r1 commit will pointed from second commit of m1
not follow the line from first commit of m1.

The Cause:
When create a commit, if user does not specify parent commit in options
gitgraph.js will execute following code:
```
options.parentCommit = options.parentCommit || _getParentCommitFromBranch(this);
```
to determine the parent commit of the created commit.
In the function `_getParentCommitFromBranch`, it will try to get the last
commit from current branch, then from parent branch. Since `r1.commit()`
is the first commit in its branch `1.0.x`, it will set its parent commit
to last commit of its parent branch, that is `master`. And since master
already add some new commit, the arrow will point from wrong commit, the
last commit of `master` branch.

There is code that deal with branch case:
```
if (options.parentCommit instanceof Commit === false && this.parentBranch instanceof Branch) {
  options.parentCommit = this.parentCommit;
}
```
It set `parentCommit` to the correct commit. However, since
`options.parentCommit` already been set by previous code, this code will
never execute since `options.parentCommit` is always a Commit.

The fix:
The fix is combining two scenario. If current branch already has commits,
the parentCommit will be set to last commit in this branch. Otherwise,
it will set to parent commit of this branch, which will be the correct commit
instead of last commit of parent branch.
@nicoespeon
Copy link
Owner

Hi @yodalee 👋

Coming back from holidays, that's a wonderful PR and a very nice debug. Thanks a lot 👍 🎊

I just tested your change, it works well and I agree with the debug you nicely described − thanks for that, it helps a lot.

@nicoespeon nicoespeon merged commit b9cfca2 into nicoespeon:develop Aug 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants