Fix inability to notice newer versions of git-installed packages

[savetheclocktower]

This is a fun one. Remember #56? I thought it made it possible to update packages installed via GitHub when the package's default branch is main instead of master. Instead, I introduced a dumb mistake, the result of which was that no GitHub-installed packages ever realized that their upstream had changed.

The diff makes it clear what I did wrong.

Why didn't the tests catch this?

Because the relevant specs are written wrong — probably because it was too hard to write them correctly.

To find out if there's a new version of a package, ppm compares the SHA of the version it has with the SHA on the tip of the head remote branch, then upgrades the package (or shows it in a list) if the two don't match. The way the spec works is to artificially change the SHA of the version it has, rather than simulate retrieving the SHA from a remote whose contents have changed.

That's what the existing spec did for the master branch, and that's what I did for my spec that tests the same behavior on main. The specs passed because they didn't ever test a situation where the local repo's head SHA differs from that of origin. I know what a better spec would look like, but it would mean not mocking anything about retrieving information from a git remote, and I don't know how feasible that is. That's why this PR has no spec changes.

What I should've done was test this behavior with one of my own packages before submitting the PR; that would've revealed that the code didn't do what I think. I hereby assert that I have done that this time around.

To test this yourself

These steps worked for me:

1. Install any package from git (e.g., ppm install some-git-user/some-atom-package)
2. Go into the package's directory within Pulsar
3. Open package.json and copy the entire contents of the file to your clipboard, or copy the file to package.json.bak or something
4. Grab an old commit SHA from git log, then do git reset --hard [old sha]
5. Open up the package.json again and replace its entire contents with your clipboard (or backup file or whatever). Then find the apmInstallSource field and replace the sha with the old SHA you just used; be sure to save
6. Run ppm upgrade --list and note how this package doesn't show up in the list, even though the remote head has a newer SHA
7. Reach into your live ppm directory within Pulsar — in my case, /Applications/Pulsar.app/Contents/Resources/app/ppm.
8. Open lib/upgrade.js (yes, the compiled version).
9. Find the line that says sha = repo.getReferenceTarget(repo.getHead()); and replace it with sha = repo.getReferenceTarget(repo.getUpstreamBranch(repo.getHead()));
10. Try step 6 again; this package should appear in the upgrade list

[DeeDeeG]

With https://github.com/pulsar-edit/ppm/pull/49 merged, we should be able to test stuff like this with ./bin/apm from within this repo, no need to rebuild Pulsar with newer ppm to test certain things.

(Edit: Not actually 100% sure if #49 helps with this, a lot of things in ppm actually don't need to find a Pulsar install to work, not sure if this is one of them or not. Might work even before #49.)

I intend to test this in a bit to confirm what you wrote above, thanks for looking into this/ensuring it's working as intended.

[DeeDeeG]

I know what a better spec would look like, but it would mean not mocking anything about retrieving information from a git remote, and I don't know how feasible that is.

Well, info about remotes is recorded in plain text files within the .git/ folder, we should be able to commit a fixture repo that is behind its remote, or similar to your testing steps, run some git commands on the fixture repo to make it behind its remote, something along those line.

EDIT TO ADD: This is a bit abstract, and I'm not totally sure it's reasonably easily doable, so I'd focus on getting the fix in for this PR, then possibly get around to committing an improved spec for the upgrade stuff.

[savetheclocktower]

we should be able to commit a fixture repo that is behind its remote, or similar to your testing steps, run some git commands on the fixture repo to make it behind its remote, something along those line.

Gives me the willies to have a spec that makes external network requests, but I guess sometimes it can't be helped. Would've prevented this regression, at least.

[DeeDeeG]

I just mean that such a fixture would "know" it is behind its [last recorded state of its] "remote", and as long as we don't do git remote update or some such command, nothing actually goes over the network to make this work.

(EDIT: In theory, that is. I'm not actually sure how easy this is to do until someone tries to implement the spec that way... Hmm. But I don't consider this a blocker for this PR, the manual testing steps are good enough to prove this works, and going down the specs rabbit hole seems like a follow-up task, IMO.)

[DeeDeeG]

Testing notes of my own:

ppm is checking the following bit of JSON it adds to the package's package.json at clone-time:

  "apmInstallSource": {
    "type": "git",
    "source": "https://github.com/DeeDeeG/mypackage11111",
    "sha": "35ddac48cbf6f6f966dcf0f69b806b304b4c5cc6"
  }

And doing a comparison.

---

Where the comparison SHA comes from, before/after this PR (click to expand)

Looking at the `git-utils` docs (https://github.com/atom/git-utils#docs)... **Before this PR:** ```js sha = repo.getReferenceTarget(repo.getHead()) ``` Gets the reference or SHA that the checked out state (`HEAD`) of the repo points to... something like `refs/heads/master` or `refs/heads/main`... and then finds what that points to, generally a commit SHA such as `632c7d4...`. **After this PR:** ```js sha = repo.getReferenceTarget(repo.getUpstreamBranch(repo.getHead())) ``` Gets the reference or SHA that the checked out state (`HEAD`) of the repo points to... something like `refs/heads/master` or `refs/heads/main`... **finds whatever upstream branch this reference/branch is tracking, such as `refs/remotes/origin/master`** ... and then finds what that points to, generally a commit SHA such as `0a693cb...`.

---

And then comparing this output SHA (which I explained in the above section) with the apmInstallSource.sha JSON value ppm added to the package's package.json at clone-time (see top of this comment).

        if sha isnt pack.apmInstallSource.sha

If these don't match, then ppm believes the package needs an upgrade, which I know from manual testing, but for which I admittedly haven't traced through 100% of the logic in the source code to confirm.

---

In plain English:

Before this PR, ppm compared the SHA of the commit that is currently checked out with the SHA of the commit ppm most-recently cloned/upgraded the package to.

With this PR, ppm will compare the SHA of the tip of the upstream branch that the checked out branch is tracking, with the SHA of the commit ppm most-recently cloned/upgraded the package to.

And bonus info, before PR #56, ppm used to compare the SHA of the tip of the upstream branch that local master branch was tracking, with the SHA of the commit ppm most-recently cloned/upgraded the package to.

[savetheclocktower]

(Note to follow up on later: Regardless of the current PR, this whole upgrade check doesn't work too great with ppm install [git repo URL] -b [branch name here], since that records a branch name under package.json --> apmInstallSource.sha. Before being written out to the package.json, that data should be normalized to a sha the branch points to, not the branch name. But I think this is out of scope for the current PR.)

Before #56, master was hard-coded when checking for upgrades, so I doubt that upgrades have ever worked right when installing from a remote branch. Wasn't aware you could do that.

In fact, if the goal is to keep tracking that remote branch when upgrading, then we should keep that branch name in the package.json — if present, it should be used instead of whatever repo.getHead() gives us. But, yes, another PR.

[DeeDeeG]

Continuing a mildly off-topic tangent, not relevant to reviewing this PR

@savetheclocktower the install -b flag is something @mauricioszabo added, it is new in ppm and didn't exist in apm. This is just an area of functionality that wasn't noticed in terms of how that flag interacts with the rest of ppm.

We should be able to just change what is recorded to package.json, since the currently checked out reference (HEAD) would be the specific branch we cloned from, and I suppose we'd be seeing if the remote copy of said branch we specifically cloned has a different tip than our local copy of the branch does whatever SHA ppm initially cloned, or last upgraded to. (Which, again, should be effectively the specified branch from the -b flag.) Sounds about right to me.

And you should be able to do ppm install [repo URL] (no -b) to start tracking the default branch again instead. OR -b [some other branch] to start tracking some other arbitrary branch.

[DeeDeeG]

And thanks to @savetheclocktower for giving your detailed write-up to start out, helped know exactly what to look at and I basically followed up to try and see what the code was exactly doing under the microscope and spell it out with sample data to refer to.

Your testing notes were very helpful and much appreciated. Always like knowing how to test a PR.