Skip to content

Conversation

@RichiH
Copy link
Owner

@RichiH RichiH commented Jun 23, 2021

Closes #305

Copy link
Collaborator

@alerque alerque left a comment

Choose a reason for hiding this comment

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

This implementation surprised me a little, but I can't come up with anything better and I can't find a reason this won't work so...

@RichiH RichiH mentioned this pull request Jun 23, 2021
@alerque alerque added this to the v2.0.0 milestone Jun 23, 2021
@RichiH
Copy link
Owner Author

RichiH commented Jun 23, 2021

I couldn't find anything else that doesn't require you to know the name of the remote and/or branch beforehand... I agree that it smells weird, but it should be pretty robust. I will add a comment before merging.

@alerque
Copy link
Collaborator

alerque commented Jun 23, 2021

The git remote porcelain command lists remotes too without knowing their names. That's what I usually use, I'm not sure if there is a better plumbing command or if there are any benefits/drawbacks over checking for the config directory.

Either way both these solutions only tell us if the repo has a remote, not if the branch we are on has a remote tracking branch.

@RichiH
Copy link
Owner Author

RichiH commented Jun 23, 2021

Heh, yes. git remote, woods for the trees, etc...

https://stackoverflow.com/a/9753364 lead me to git for-each-ref --format='%(upstream:short)' "$(git symbolic-ref -q HEAD)"

@alerque
Copy link
Collaborator

alerque commented Jun 23, 2021

I like the looks of that last one much more, and it confirms not just that we have a remote but that the current branch has a tracking branch setup on the remote. I think that's the info we really wanted to confirm in the first place.

@RichiH
Copy link
Owner Author

RichiH commented Jun 23, 2021

Thanks for the challenge on git remote!

Generally speaking, I vastly prefer to go through commands, not files, at least if they are reasonably old. That way, we benefit from future cleanups, protections, etc in upstream Git.

Unfortunately, 'empty' does still output return code 0 so we will need to go -n.

@RichiH RichiH force-pushed the richih/fix_list_has_remote branch from 12f7f65 to c9287f5 Compare June 23, 2021 08:36
Fixes #305

$VCSH_BRANCH was not set in list_has_remote() but required. This
broke both `vcsh push` and `vcsh pull`.

Signed-off-by: Richard Hartmann <[email protected]>
@RichiH RichiH force-pushed the richih/fix_list_has_remote branch from c9287f5 to b4281d0 Compare June 23, 2021 09:06
@RichiH
Copy link
Owner Author

RichiH commented Jun 23, 2021

@alerque I created #305 just to have something to actually fix and point to. As cbfd1ff is not in any tags, we don't need to add anything to the changelog but it still felt cleaner.

Once this PR is merged, we should cut an RC. Not because we would release this precise version, but to trigger users into testing.

@RichiH RichiH requested a review from alerque June 23, 2021 10:24
@alerque alerque merged commit 1d92662 into main Jun 23, 2021
@alerque alerque deleted the richih/fix_list_has_remote branch June 23, 2021 12:58
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.

push & pull are broken

3 participants