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.

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

BREAKING CHANGE: set default git ref to HEAD #142

Merged
merged 2 commits into from
Oct 12, 2022

Conversation

darcyclarke
Copy link
Contributor

Why/What

  • change all default references/fallbacks to HEAD (instead of the master branch)

Links & References

@lukekarrys
Copy link
Contributor

lukekarrys commented Aug 8, 2022

One thing we should add to this is the ability to differentiate between directing to files vs directories. hosted-git-info currently only has the concept of a treepath for each host, but GitHub serves files and directories differently when using HEAD as the committish portion of the path:

This change works great if we only point to directories:

curl -sSL -D - https://github.com/npm/cli/tree/latest/lib | grep -e HTTP -e 'location: http'
HTTP/2 200

curl -sSL -D - https://github.com/npm/cli/tree/HEAD/lib | grep -e HTTP -e 'location: http'
HTTP/2 200

But if point to files and use treepath, then GitHub serves redirects to a specific commit which is not ideal:

curl -sSL -D - https://github.com/npm/cli/tree/latest/lib/npm.js | grep -e HTTP -e 'location: http'
HTTP/2 301
location: https://github.com/npm/cli/blob/latest/lib/npm.js
HTTP/2 200

curl -sSL -D - https://github.com/npm/cli/tree/HEAD/lib/npm.js | grep -e HTTP -e 'location: http'
HTTP/2 301
location: https://github.com/npm/cli/blob/f281ec8a1aec43439281a8fca4c255b0d94a0c94/lib/npm.js
HTTP/2 200

But if we use HEAD and a new internal configuration blobpath:

curl -sSL -D - https://github.com/npm/cli/blob/HEAD/lib/npm.js | grep -e HTTP -e 'location: http'
HTTP/2 200

What to do?

I think we should land another breaking change with this:

  • Create a new per-host config called blobpath. GitHub would use blob as the value. Research to be done for other hosts.
  • Change file() to always use blobpath instead of treepath (if relevant, eg GitHub uses the host raw.githubusercontent.com which has no treepath portion of the url)
  • Keep browse() to use treepath
  • Create a new method browseFile() that will use blobpath for browsing directly to a file

Copy link
Contributor

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

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

Requesting changes in order to discuss my additional proposal.

@darcyclarke
Copy link
Contributor Author

darcyclarke commented Aug 18, 2022

@lukekarrys it should be noted that introducing a blob path does not eliminate redirects.

Example:

If the ref that is provided does not exist but did at some point (ex. was renamed - which is the case for most master branches) you are redirected to the default branch via 302; ex.

curl -sSL -D - https://github.com/npm/cli/blob/master/lib/npm.js | grep -e HTTP -e 'location: http'
HTTP/2 302
location: https://github.com/npm/cli/blob/latest/lib/npm.js
HTTP/2 200

ref. https://github.com/npm/cli/blob/master/lib/npm.js

Looking into GitLab, it seems as though they use a 302 to redirect as well; ex.

curl -sSL -D - https://gitlab.com/eBay/ebay/-/tree/HEAD/README.md | grep -e HTTP -e 'location: http'
HTTP/2 302
location: https://gitlab.com/eBay/ebay/-/blob/HEAD/README.md
HTTP/2 200

I personally don't think redirects are inherently bad here & introducing a new differentiator for file vs. directory links doesn't change if/when you might get redirected. That said, if you feel strongly it's a better user experience to introduce blobpath then we can gate the next major until that work is done as well.

@darcyclarke darcyclarke mentioned this pull request Aug 22, 2022
32 tasks
@darcyclarke darcyclarke added the Enhancement new feature or improvement label Sep 29, 2022
@lukekarrys lukekarrys assigned lukekarrys and unassigned lukekarrys Oct 11, 2022
darcyclarke and others added 2 commits October 11, 2022 19:12
This also adds a `browseFile` method for browsing directly to a blob as
separate from a git tree. This is useful for hosts that serve blobs and
trees differently.

BREAKING CHANGE: `GitHost` now has a static `addHost` method to use
instead of manually editing the object from `lib/git-host-info.js`.
@lukekarrys lukekarrys force-pushed the darcyclarke/change-default-git-ref branch from 69e586d to 314e782 Compare October 12, 2022 18:44
@lukekarrys lukekarrys self-requested a review October 12, 2022 18:44
@lukekarrys
Copy link
Contributor

it should be noted that introducing a blob path does not eliminate redirects.

My goal was not to completely eliminate redirects, but to try and stop redirects that redirect to a different url such as https://github.com/npm/cli/tree/HEAD/lib/npm.js -> https://github.com/npm/cli/blob/f281ec8a1aec43439281a8fca4c255b0d94a0c94/lib/npm.js.

@lukekarrys lukekarrys requested a review from a team October 12, 2022 18:50
@lukekarrys lukekarrys merged commit 9e0ce62 into main Oct 12, 2022
@lukekarrys lukekarrys deleted the darcyclarke/change-default-git-ref branch October 12, 2022 19:18
@github-actions github-actions bot mentioned this pull request Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement new feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants