Refactor branch/tag resolution for git dependencies, fixes #3720 #3836

Volune · 2017-07-06T03:23:10Z

Summary

For git dependencies, the branch/tag resolution does not always work as expected.
Refactor the code to make the resolution algorithm more explicit and follow discussions in #3720

Test plan

New test in __tests__/util/git-ref-resolver.js.

Also benefit from the fact that travis-ci ran the test with an older Git version. I don't know if we have a minimum git version requirement, it seems we currently don't test against any specific version.

Implementation details

I refactored parseRefs to return the full ref name, so we can differentiate branches and tags.

I use git ls-remote --symref to get the default branch name. This works only in recent versions of git (thanks travis-ci for reporting the issue), so I fallback to another algorithm if --symref is unavailable.

I tried not to use the variable name hash, which is confusing between the git-url hash and the commit hash. Any suggestion on variable names is welcomed.

sheerun · 2017-07-07T17:46:25Z

src/util/git.js

+    const resolvedResult = await resolveVersion(this.config, version, refs);
+    if (resolvedResult === false) {
+      throw new MessageError(
+        this.reporter.lang('couldntFindMatch', version, Object.keys(refs).join(','), this.gitUrl.repository),


This can result in some unreadable stuff like Couldn't find match for ">=1.5,<1.6" in "refs/heads/design_experimental,refs/heads/experimental_modal_scroll,refs/heads/master,refs/merge-requests/1/head,refs/merge-requests/10/head,refs/merge-requests/100/head,refs/merge-requests/101/head,refs/merge-requests/102/head,refs/merge-requests/103/head,refs/merge-requests/104/head,refs/merge-requests/105/head,refs/merge-requests/106/head,refs/merge-requests/107/head,refs/merge-requests/108/head,refs/merge-requests/11/head,refs/merge-requests/12/head,refs/merge-requests/13/head,refs/merge-requests/14/head,refs/merge-requests/15/head,refs/merge-requests/16/head,refs/merge-requests/17/head,refs/merge-requests/18/head,refs/merge-requests/19/head,refs/merge-requests/2/head,refs/merge-requests/20/head,refs/merge-requests/21/head,refs/merge-requests/22/head,refs/merge-requests/23/head,refs/merge-requests/24/head,refs/merge-requests/25/head,refs/merge-requests/26/head,refs/merge-requests/27/head,refs/merge-requests/28/head,refs/merge-requests/29/head,refs/merge-requests/3/head,refs/merge-requests/30/head,refs/merge-requests/31/head,refs/merge-requests/32/head,refs/merge-requests/33/head,refs/merge-requests/34/head,refs/merge-requests/35/head,refs/merge-requests/36/head,refs/merge-requests/37/head,refs/merge-requests/38/head,refs/merge-requests/39/head,refs/merge-requests/4/head,refs/merge-requests/40/head,refs/merge-requests/41/head,refs/merge-requests/42/head,refs/merge-requests/43/head,refs/merge-requests/44/head,refs/merge-requests/45/head,refs/merge-requests/46/head,refs/merge-requests/47/head,refs/merge-requests/48/head,refs/merge-requests/49/head,refs/merge-requests/5/head,refs/merge-requests/50/head,refs/merge-requests/51/head,refs/merge-requests/52/head,refs/merge-requests/53/head,refs/merge-requests/54/head,refs/merge-requests/55/head,refs/merge-requests/56/head,refs/merge-requests/57/head,refs/merge-requests/58/head,refs/merge-requests/59/head,refs/merge-requests/6/head,refs/merge-requests/60/head,refs/merge-requests/61/head,refs/merge-requests/62/head,refs/merge-requests/63/head,refs/merge-requests/64/head,refs/merge-requests/65/head,refs/merge-requests/66/head,refs/merge-requests/67/head,refs/merge-requests/68/head,refs/merge-requests/69/head,refs/merge-requests/7/head,refs/merge-requests/70/head,refs/merge-requests/71/head,refs/merge-requests/72/head,refs/merge-requests/73/head,refs/merge-requests/74/head,refs/merge-requests/75/head,refs/merge-requests/76/head,refs/merge-requests/77/head,refs/merge-requests/78/head,refs/merge-requests/79/head,refs/merge-requests/8/head,refs/merge-requests/80/head,refs/merge-requests/81/head,refs/merge-requests/82/head,refs/merge-requests/83/head,refs/merge-requests/84/head,refs/merge-requests/85/head,refs/merge-requests/86/head,refs/merge-requests/87/head,refs/merge-requests/88/head,refs/merge-requests/89/head,refs/merge-requests/9/head,refs/merge-requests/90/head,refs/merge-requests/91/head,refs/merge-requests/92/head,refs/merge-requests/93/head,refs/merge-requests/94/head,refs/merge-requests/95/head,refs/merge-requests/96/head,refs/merge-requests/97/head,refs/merge-requests/98/head,refs/merge-requests/99/head,refs/tags/1.0.0,refs/tags/1.0.1,refs/tags/1.0.10,refs/tags/1.0.11,refs/tags/1.0.3,refs/tags/1.0.4,refs/tags/1.0.5,refs/tags/1.0.6,refs/tags/1.0.7,refs/tags/1.0.8,refs/tags/1.0.9,refs/tags/1.1.0,refs/tags/1.1.1,refs/tags/1.1.2,refs/tags/1.1.3,refs/tags/1.1.4,refs/tags/1.2.0,refs/tags/1.2.1,refs/tags/1.2.2,refs/tags/1.3.0,refs/tags/1.4.0,refs/tags/1.4.1,refs/tags/1.4.2,refs/tags/1.4.3,refs/tags/1.4.4,refs/tags/1.4.5,refs/tags/1.4.6,refs/tags/1.4.7,refs/tags/1.5.0,refs/tags/1.5.1,refs/tags/1.6.0,refs/tags/1.6.1,refs/tags/1.6.2,refs/tags/1.7.0,refs/tags/1.7.1" for "https://gitlab.com/leanlabsio/kanban.git"

Maybe also replace each "refs/tags/" with space in this message

The refs/merge-requests/* had nothing to do here, it should be solved with latest commit.

sheerun · 2017-07-07T17:54:51Z

It seems to be working even if merged with #3855

One thing to notice is that yarn.lock will have "version" different than resolved tag, i.e. "version" field from package.json on resolved commit, or "0.0.0" if package has no package.json

yarn add IndexOverflow/orange-notification#2.0.x

orange-notification@IndexOverflow/orange-notification#2.0.x:
  version "0.0.0"
  resolved "https://codeload.github.com/IndexOverflow/orange-notification/tar.gz/0c39d4f2aab707bfe5cab21633010e436ca49a8f"

Another case with package.json but incorrect version:

yarn add "gitlab:leanlabsio/kanban#>=1.5 <1.6"

"gitlab-kanban-client@gitlab:leanlabsio/kanban#>=1.5 <1.6":
  version "0.0.1"
  resolved "https://gitlab.com/leanlabsio/kanban/repository/archive.tar.gz?ref=4c311001a77674ec79788d12c302afba32907b8e"

It's probably not an issue but makes one wonder why yarn.lock even has "version" field locked. It seems "resolved" is more than enough.

sheerun · 2017-07-12T17:26:20Z

Please kindly prioritize this PR, it's the last thing missing in Yarn to allow migration of Bower projects :)

BYK · 2017-07-12T17:31:26Z

@sheerun sorry for missing. Gonna look at it tomorrow!

Volune · 2017-07-13T07:10:46Z

Regarding the version in the lockfile, I think it would be interesting to put the resolved commit sha as the value of the version, and improve the outdated command to show if the dependency will resolve to a new commit. But that's out of the scope of this PR.

BYK

Great start but the code needs a bit clean up. Also there's that edge case I mentioned about tags/branches may be interpreted as SHAs.

BYK · 2017-07-13T15:43:23Z

src/util/git-ref-resolver.js

+export type ResolvedSha = {sha: string, ref: ?string};
+type Names = {tags: Array<string>, branches: Array<string>};
+
+export const isCommitSha = (target: string): boolean => Boolean(target) && /^[a-f0-9]{5,40}$/.test(target);


This should probably be case-insensitive, right?

I'll keep it case-sensitive a it was previously, but handle uppercase arguments in tryVersionAsGitCommit

The other regex to match SHA's is case-insensitive so I think it was not intentional to make this case sensitive in the old version.

I think it was intentional because there is a test for that specific case. And after thinking about it, I think it may help keeping the sha consistently lowercase over all the application (except user input)

BYK · 2017-07-13T15:43:59Z

src/util/git-ref-resolver.js

+export const isCommitSha = (target: string): boolean => Boolean(target) && /^[a-f0-9]{5,40}$/.test(target);
+
+const REF_TAG_PREFIX = 'refs/tags/';
+const REF_BRANCH_PREFIX = 'refs/heads/';


BYK · 2017-07-13T16:12:45Z

src/util/git-ref-resolver.js

+
+  async resolve(): Promise<DefaultBranch | ResolvedSha | false> {
+    const testFunctions = [
+      () => this.tryVersionAsGitCommit(),


You can do

GitRefResolver.testFunctions = [ GitRefResolver.tryVersionAsFullRef, GitRefResolver.tryVersionAsTagName, ]; //... async resolve(): Promise { for (const testFunction of testFunctions) { const result = await testFunction.call(this); if (result) { return result; } } return false; }

Would save you from creating bound functions and an array each time resolve is called which could result in significant performance and memory savings.

BYK · 2017-07-13T16:14:30Z

src/util/git-ref-resolver.js

+  async resolve(): Promise<DefaultBranch | ResolvedSha | false> {
+    const testFunctions = [
+      () => this.tryVersionAsGitCommit(),
+      () => (this.version === '' ? this.useDefaultBranch() : false),


this.version === '' && this.useDefaultBranch() would have the same effect.

BYK · 2017-07-13T16:14:56Z

src/util/git-ref-resolver.js

+      () => this.tryVersionAsBranchName(),
+      () => this.tryVersionAsTagSemver(),
+      () => this.tryVersionAsBranchSemver(),
+      () => (this.version === '*' ? this.useDefaultBranch() : false),


Same as above: this.version === '*' && this.useDefaultBranch()

BYK · 2017-07-13T16:21:16Z

src/util/git-ref-resolver.js

+
+  tryVersionAsGitCommit(): ResolvedSha | false {
+    if (isCommitSha(this.version)) {
+      for (const ref in this.refs) {


const result = this.refs.find(ref => this.refs[ref].startsWith(this.version)); return result || {sha: this.version, ref: null};

I don't think I can use that, this.refs is an object. Maybe I can improve the code by changing this.refs to an instance of Map?

Of, sorry. Map doesn't support .find or .forEach either but you can try Object.keys(this.refs).find( instead.

BYK · 2017-07-13T16:22:17Z

src/util/git-ref-resolver.js

+    return false;
+  }
+
+  tryRef(ref: string): ResolvedSha | false {


return this.refs[ref] && {sha: this.refs[ref], ref};

I need to put return this.refs[ref] ? {sha: this.refs[ref], ref} : false to match the method signature, is it worth it?

Or you can do return Boolean(this.refs[ref]) && {sha: this.refs[ref], ref}.

Also, as mentioned earlier, I don't fully agree with this signature. I'd honestly prefer null or undefined instead of returning false which would allow you to use my first suggestion and switch the return type to ?ResolveSha.

BYK · 2017-07-13T16:25:04Z

src/util/git-ref-resolver.js

+          return {sha, ref};
+        }
+      }
+      return {sha: this.version, ref: undefined};


Danger: if I pass something that looks like a commit SHA like fb00cc which actually is a branch or a tag, this would resolve it as a version. Not sure if this is actually harmful or not.

BYK · 2017-07-13T16:26:05Z

src/util/git-ref-resolver.js

+  }
+
+  getSemverNames(): Names {
+    if (!this.names) {


Same trick as above, return early:

if (this.names) { return this.names; } //actual function body

I prefer keeping a single return for lazy-initializing getters. I'll do the change to be consistent with the other one

I prefer keeping a single return for lazy-initializing getters. I'll do the change to be consistent with the other one

That preference leads to an unnecessary indentation, makes the intent less clear and makes the code harder to follow. I don't think having fewer return statements are objectively better. Would you mind explaining your thought process behind that preference so I can understand where you are coming from (instead of trying to impose my preference over yours which is not fun :))

That's a preference only for getters, most of the time I also prefer putting an if/return at the beginning and avoid indentation.
In the case of the getters:

A single return statement

Closer to the implementation of a simple getter
getValue() { // if you add/remove lazy-init later, just change this line return this.value; }

Ensure that the "value" is saved, to avoid errors like:
getValue() { if (this.value) { return this.value; } const value = computeValue(); // ... return value; }

BYK · 2017-07-13T16:31:40Z

src/util/git-ref-resolver.js

+      });
+      for (const ref in this.refs) {
+        const match = refNameRegexp.exec(ref);
+        if (match) {


const names = (this.names = { tags: [], branches: [], }); const actions = { tags: tag => names.tags.push(tag), heads: head => names.branches.push(head), }; this.refs.forEach(ref => { if (!match || semver.valid(match[2], this.config.looseSemver)) { continue; } const [, type, name] = match; actions[type](name); });

Volune · 2017-07-14T03:06:47Z

Regarding git sha and tag/branch:
A branch/tag name may indeed be interpreted as a sha. The current PR allows the repo#refs/tags/1234567 notation to workaround this issue.

I didn't put verification to ensure the sha exists in the repository, because it seemed to me that the verification was too costly for this step. If there are no tags or branches matching the commit (and I assume it will be most of the time), it would require fetching the remote repository to test that the sha is valid.

But after a few more tests, I realize that the current behavior may produce multiple cache folders for the same content, for example:

npm-test-js-git-repo-0.0.0-90f974f
npm-test-js-git-repo-0.0.0-90f974f6
npm-test-js-git-repo-0.0.0-90f974f63
npm-test-js-git-repo-0.0.0-90f974f63a1ab7f9f9030808896ff39b4d597591

So maybe I should make the verification after all. Any opinion?

BYK · 2017-07-14T11:07:07Z

Regarding git sha and tag/branch:
A branch/tag name may indeed be interpreted as a sha. The current PR allows the repo#refs/tags/1234567 notation to workaround this issue.

I think we should put this both in docs and in the code as a comment somewhere. Thanks for explaining :)

I didn't put verification to ensure the sha exists in the repository, because it seemed to me that the verification was too costly for this step. If there are no tags or branches matching the commit (and I assume it will be most of the time), it would require fetching the remote repository to test that the sha is valid.

Not necessarily. Can't we use this endpoint for a quick validation: https://developer.github.com/v3/repos/commits/#get-the-sha-1-of-a-commit-reference (agreed that it still means a network request) - This link works for me for arbitrarily shortened SHA-1 refs: https://api.github.com/repos/yarnpkg/yarn/commits/8816df3c51a4

But after a few more tests, I realize that the current behavior may produce multiple cache folders for the same content

So maybe I should make the verification after all. Any opinion?

That sounds bad. I'd try to normalize all the references, probably to SHAs since that seems to be the common denominator. What do you think?

BYK · 2017-07-14T11:08:08Z

src/util/git-ref-resolver.js

@@ -38,39 +39,36 @@ class GitRefResolver {

  async resolve(): Promise<DefaultBranch | ResolvedSha | false> {
    const testFunctions = [


Still defining a new array for each invocation and the array is actually the same for every invocation.

Btw. I'm not very convinced that we need a class for this. Just having these functions in the module may be a more efficient approach. This is up to you though, just sharing my observation.

My first motivation for the class was the multiple parameters (version, config, refs, ... possibly growing). I can refactor and pass all of these as an object argument, that should indeed be more efficient.

Volune · 2017-07-14T16:05:42Z

Not necessarily. Can't we use this endpoint for a quick validation:

That's true for Github. That would require some research for Bitbucket/Gitlab and will not work for private repositories.
There is a lot work to do about optimisations for Github/Bitbucket/... (also see my comments on #3923), but I want to have the feature working for all repositories for the moment, so I only consider the general case performance.

That sounds bad. I'd try to normalize all the references, probably to SHAs since that seems to be the common denominator. What do you think?

The tags/branches and semver ranges already resolve to a unique full-length sha, so no problem for them.
The short sha are (yarn current behavior and current PR implementation) assumed to be valid, and used that way. If we verify them, we can at the same time resolve them to the full-length sha, and use the full-length sha in the end.

So I'll have a look to make this verification, doing the git fetch (Github optimisations can come later), and cleaning the code according to other comments.

Volune · 2017-07-17T07:35:04Z

I've changed the PR code a lot to fix most of what we talked about. Let me know if you see other problems.

After this PR, I'll work on the github/bitbucket optimizations.

BYK

Okay, looks good.

I have these comments around code structure and concise code. Are you willing to address these so we can merge right after?

BYK · 2017-07-17T22:01:43Z

src/util/git/git-ref-resolver.js

+  version === '*' ? git.resolveDefaultBranch() : Promise.resolve(null);
+
+const tryRef = (refs: GitRefs, ref: string): ?ResolvedSha => {
+  if (refs[ref]) {


Ternary is better:

return refs[ref] ? { sha: refs[ref], ref } : null;

BYK · 2017-07-17T22:04:14Z

src/util/git/git-ref-resolver.js

+};
+
+const tryVersionAsFullRef = ({version, refs}: ResolveVersionOptions): ?ResolvedSha => {
+  if (version.startsWith('refs/')) {


BYK · 2017-07-17T22:06:50Z

src/util/git/git-ref-resolver.js

+  };
+  for (const ref in refs) {
+    const match = refNameRegexp.exec(ref);
+    if (match) {


I guess we're not gonna be able to agree to use early returns with you? :D

BYK · 2017-07-17T22:07:47Z

src/util/git/git-ref-resolver.js

+    if (match) {
+      const [, type, name] = match;
+      if (semver.valid(name, config.looseSemver)) {
+        switch (type) {


Honestly, I don't think using a switch here makes the code more readable. If you didn't like my object map suggestion, why not just use if/else if blocks?

I missed your comment 😕

No worries :)

BYK · 2017-07-17T22:12:19Z

src/util/git/git-ref-resolver.js

+
+  for (const testFunction of VERSION_RESOLUTION_STEPS) {
+    const result = await testFunction(options);
+    if (result !== null) {


Why are we not just doing return await testFunction(options)?

We want to try the next function if this one returns null

BYK · 2017-07-18T10:13:13Z

src/util/git.js

    }
  }

  /**
-   * Parse Git ref lines into hash of tag names to SHA hashes
+   * TODO description


Please fill this before merging :)

BYK · 2017-07-18T10:15:26Z

src/util/git/git-ref-resolver.js

+};
+type Names = {tags: Array<string>, branches: Array<string>};
+
+export const isCommitSha = (target: string): boolean => Boolean(target) && /^[a-f0-9]{5,40}$/.test(target);


I don't think you need the Boolean(target) && part. Since the type of target is string and not ?string the only case Boolean(target) && would protect you from would be when target is an empty string and that wouldn't match your regex anyways so we don't need it.

BYK · 2017-07-18T15:24:55Z

src/util/git/git-ref-resolver.js

+// This regex is designed to match output from git of the style:
+//   ebeb6eafceb61dd08441ffe086c77eb472842494  refs/tags/v0.21.0
+// and extract the hash and ref name as capture groups
+const gitRefLineRegex = /^([a-fA-F0-9]+)\s+(refs\/(?:tags|heads)\/.*)$/;


Follow the const naming conventions above: GIT_REF_LINE_REGEX

BYK · 2017-07-18T15:25:17Z

src/util/git/git-ref-resolver.js

+// and extract the hash and ref name as capture groups
+const gitRefLineRegex = /^([a-fA-F0-9]+)\s+(refs\/(?:tags|heads)\/.*)$/;
+
+const refNameRegexp = /^refs\/(tags|heads)\/(.+)$/;


Naming conventions as mentioned above: GIT_REF_NAME_REGEX.

BYK · 2017-07-18T15:27:55Z

src/util/git/git-ref-resolver.js

+  if (!isCommitSha(lowercaseVersion)) {
+    return Promise.resolve(null);
+  }
+  for (const ref in refs) {


Clearer:

const existingResolution = refs.find(ref => refs[ref].startsWith(lowercaseVersion)); return existingResolution ? Promise.resolve({ sha: refs[existingResolution], ref: existingResolution }) : git.resolveCommit(lowercaseVersion);

arcanis · 2017-07-18T16:10:06Z

@Volune I understand the issue itself, but could you explain what is actually causing it in the code before your patch? It seems to me it simply a matter of sorting branches before tags, so I'm not sure about the complexity of this PR, but I might be missing something :)

- Make tag/branch resolution before semver resolution - "refs/tags/name" format can be used if name looks like a commit sha - " range" with a leading space can be used to force semver resolution - Don't assume "master" is default branch - Resolve short-format commit sha to long format

Volune · 2017-07-18T17:59:06Z

@arcanis this small piece of feature (resolving a usable commit sha and git ref) had more issues than just the tag/branch order. I've listed them in the latest commit message, and some new code comments.
Also I refactored to make the algorithm more explicit in the code (that's the point of the VERSION_RESOLUTION_STEPS array), and more tests.

@BYK I changed GitRefs to an ES6 Map, I hope it makes the code cleaner.

BYK

Okay, I still think we can improve the code a bit, especially around code organization and may be slightly better Git calls but I think with the added tests and all the back and forth, it is not worth delaying this patch more.

@Volune - I'll wait for your response before merging but I'm okay merging this PR as it is and then keep working on certain improvements.

BYK · 2017-07-25T11:16:15Z

src/util/git.js

+      const stdout = await spawnGit(['ls-remote', '--symref', this.gitUrl.repository, 'HEAD']);
+      const lines = stdout.split('\n');
+      const [, ref] = lines[0].split(/\s+/);
+      const [sha] = lines[1].split(/\s+/);


I'd rather use a stronger regexp like /^\s*([0-9a-f]{5,40})\s+/ and just use match on it. Same goes for the ref if possible.

BYK · 2017-07-25T11:18:56Z

src/util/git.js

-      throw new MessageError(this.reporter.lang('couldntFindMatch', ref, names.join(','), this.gitUrl.repository));
+  async resolveDefaultBranch(): Promise<ResolvedSha> {
+    try {
+      const stdout = await spawnGit(['ls-remote', '--symref', this.gitUrl.repository, 'HEAD']);


May be git symbolic-ref would be more precise?

I think it doesn't work on remote repositories

BYK · 2017-07-25T11:19:27Z

src/util/git.js

+      return {sha, ref};
+    } catch (err) {
+      // older versions of git don't support "--symref"
+      const stdout = await spawnGit(['ls-remote', this.gitUrl.repository, 'HEAD']);


I think we should print a warning here about this being slower and less precise, urging the user to upgrade their Git version, what do you think?

BYK · 2017-07-25T11:19:57Z

src/util/git.js

+      const [sha] = stdout.split(/\s+/);
+      return {sha, ref: undefined};
+    } catch (err) {
+      // assuming commit not found, let's try something else


May be add some verbose logging here?

Volune · 2017-07-25T16:27:33Z

@Volune - I'll wait for your response before merging but I'm okay merging this PR as it is and then keep working on certain improvements.

@BYK Okay for me. As I said before, I also want to work on other issues about the Git Hosted resolvers, and various optimisations in these resolvers that I'd like to share with the Git Fetcher. So it will be an opportunity to improve this code too.

BYK · 2017-07-26T09:46:31Z

@Volune merged! Thanks a lot for keeping up with all the change requests and making submitting the PR in the first place!

Looking forward to more contributions from you! 🎉

sheerun reviewed Jul 7, 2017

View reviewed changes

BYK self-assigned this Jul 10, 2017

sheerun mentioned this pull request Jul 10, 2017

Discussion: NPM Migration Path bower/bower#2467

Closed

sheerun approved these changes Jul 12, 2017

View reviewed changes

BYK requested changes Jul 13, 2017

View reviewed changes

BYK requested changes Jul 14, 2017

View reviewed changes

Volune force-pushed the fix-3720 branch from 2181032 to 835eb20 Compare July 17, 2017 06:48

BYK reviewed Jul 18, 2017

View reviewed changes

Volune force-pushed the fix-3720 branch from 835eb20 to 9e3007c Compare July 18, 2017 17:50

BYK approved these changes Jul 25, 2017

View reviewed changes

BYK merged commit 67dfc7a into yarnpkg:master Jul 26, 2017

BYK mentioned this pull request Aug 10, 2017

"resolutions" do not provide enough granularity when using git #3296

Closed

		@@ -38,39 +39,36 @@ class GitRefResolver {

		async resolve(): Promise<DefaultBranch \| ResolvedSha \| false> {
		const testFunctions = [

Refactor branch/tag resolution for git dependencies, fixes #3720 #3836

Refactor branch/tag resolution for git dependencies, fixes #3720 #3836

Conversation

Volune commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheerun commented Jul 7, 2017 • edited Loading

sheerun commented Jul 12, 2017

BYK commented Jul 12, 2017

Volune commented Jul 13, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Volune commented Jul 14, 2017

BYK commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Volune commented Jul 14, 2017

Volune commented Jul 17, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented Jul 18, 2017

Volune commented Jul 18, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Volune commented Jul 25, 2017 • edited Loading

BYK commented Jul 26, 2017

sheerun commented Jul 7, 2017 •

edited

Loading

Volune commented Jul 25, 2017 •

edited

Loading