Skip to content

fix(browse): reject commits missing from remote - #1274

Closed
vjymisal0 wants to merge 4 commits into
tj:mainfrom
vjymisal0:fix/browse-unpushed-commit
Closed

vjymisal0 wants to merge 4 commits into
tj:mainfrom
vjymisal0:fix/browse-unpushed-commit

Conversation

@vjymisal0

Copy link
Copy Markdown
Contributor

Summary

git-browse can construct a repository URL for a local commit that the selected remote does not contain. Such links fail when opened. Check the remote-tracking refs before constructing the URL and report a clear error when the commit has not been pushed.

Changes

  • Verify HEAD is contained by a ref under the selected remote.
  • Add a regression test for a local-only commit.
  • Seed the existing browse tests with a remote-tracking ref.

Test plan

  • bash -n bin/git-browse
  • git diff --check
  • Manual published/unpublished commit reproduction

Fixes #1153

@spacewander spacewander left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The commit hash is only embedded in the URL when a filename is given, but the new check runs on every invocation — so git browse with no filename (whose URL never contains the commit) gets blocked unnecessarily.

Comment thread bin/git-browse Outdated
Signed-off-by: vjymisal0 <misalvijay153@gmail.com>
@vjymisal0

Copy link
Copy Markdown
Contributor Author

Thanks for catching this @spacewander! I’ve limited the remote-commit validation to invocations that include a filename, so still opens the repository even when HEAD is unpushed. I also added a regression test for that case. Verified with bash syntax checks and manual pushed/unpushed filename scenarios; the local environment doesn’t have Bats installed.

Signed-off-by: vjymisal0 <misalvijay153@gmail.com>
@vjymisal0

Copy link
Copy Markdown
Contributor Author

Thanks for the CI report. The new regression test was still invoking the no-filename form while expecting rejection. I updated it to pass ./browse_this, which preserves the intended rejection for file URLs while the separate no-filename test verifies repository browsing with an unpushed HEAD. Pushed as f7b5ec5.

Comment thread bin/git-browse Outdated
Comment thread bin/git-browse
commit_hash=$(git rev-parse HEAD 2>/dev/null)
commit_or_branch=${commit_hash:-${branch}}

if [[ -n ${filename} ]] && ! git branch --remotes --contains "$commit_or_branch" 2>/dev/null |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why add the filename guard?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When no filename is provided (e.g. git browse or git browse ), the command opens the repository root URL (e.g. https://github.com/owner/repo), which does not include any commit hash and always succeeds even with unpushed local commits.

When a filename is specified, the URL embeds $commit_or_branch (e.g. .../blob//), which returns a 404 on GitHub/GitLab if the commit has not been pushed to that remote. The filename guard allows general repo browsing to work freely while protecting file permalinks against 404s.

@vjymisal0

Copy link
Copy Markdown
Contributor Author

Thanks for catching this @spacewander! I updated the remote-commit check to run only when a filename is provided, so continues to open the repository URL even with an unpushed local commit. I also added a regression test and tightened the remote matching. Verified with bash syntax checking and manual pushed/unpushed cases; the full Bats suite could not run here because is not installed.

@vjymisal0

Copy link
Copy Markdown
Contributor Author

Thanks @spacewander — the remote commit check is now limited to URL forms that include a filename, while remote-only browsing remains unaffected. Added regression coverage for remote branch matching and unpushed file commits; the branch is clean and pushed.

@hyperupcall hyperupcall left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced this is necessary. If the commit doesn't exist on remote, the web page will open, and it will 404. This gives information to the user, who can then push and refresh the page.

With this new code, the user will have to run this command again in that case. Additionally, the user will be shown a non-standard error, and if the network is slow, then this command will take even longer to complete.

@vjymisal0

Copy link
Copy Markdown
Contributor Author

Thanks for the thoughtful feedback @hyperupcall! That makes complete sense — avoiding the additional network round-trip keeps git-browse snappy, and landing on the remote 404 page naturally prompts the user to push if needed without requiring a re-invocation.

@hyperupcall

Copy link
Copy Markdown
Collaborator

Great glad that we agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

git-browse doesn't check if a commit exists on upstream

3 participants