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

README.md - Suggest -vcs option with 'gohack get' as having vcs is th… #47

Merged
merged 1 commit into from
May 23, 2019

Conversation

chmorgan
Copy link
Contributor

…e likely desired behavior

@davidovich
Copy link
Contributor

In fact, -vcs is an additional mode to the base use case which does not initialize a repo (if you want to make a fast one-off non tracked modification). We would need to state that in addition to the non -vcs case.

@chmorgan
Copy link
Contributor Author

I ran without -vcs and ended up without a .git, ran with -vcs and it had a .git as expected. Maybe the logic was unintentionally reversed? Imo the default should be to have a git repo to work with so it didn’t make sense to me initially.

@chmorgan
Copy link
Contributor Author

I can expand upon the two cases and why you might use one over the other. Imo it was counterintuitive that I didn’t get a vcs by default. Maybe we should reverse the logic on the option, make it -novcs? Disk space and bandwidth would be increased but these days it’s not a big deal for most people.

@davidovich
Copy link
Contributor

I think changing the default should be proposed in a new issue so that maintainers can get a shot at discussing it. There must be a reason for the current (non-vcs) default.

@chmorgan
Copy link
Contributor Author

Agreed. Let me fix this pr up tonight and open a separate one to swap the defaults.

@chmorgan chmorgan force-pushed the patch-1 branch 4 times, most recently from a1f8ced to 1bb5804 Compare March 31, 2019 00:00
@chmorgan
Copy link
Contributor Author

Hi @davidovich alright, ready for re-review. I took the liberty of using some markdown to break things into easier to follow sections as well.

@davidovich
Copy link
Contributor

@chmorgan I do not have merge privileges, but I think your changes are a great documentation addition. We just merged one of my changes that enhances setting GOHACK to a relative directory to control where the module lands. Want to document that also ? :-) (GOHACK en var was not documented before).

@chmorgan
Copy link
Contributor Author

HI @davidovich. I could take a swag at updating that once this one is merged in. I'd rather not stack anything up behind it in case people prefer the flat unformatted look and the edits are rejected :-) But I can take a quick look once this is in. Should only take a few minutes to document that but as the original author you may be the person that knows it best ;-)

@davidovich
Copy link
Contributor

You're right, I won't be lazy :-). I'll wait till the maintainers see if this change is acceptable, and make the GOHACK doc update PR on top.

@chmorgan
Copy link
Contributor Author

Any updates on this PR?

@myitcv
Copy link
Collaborator

myitcv commented Apr 21, 2019

cc @rogpeppe

@chmorgan
Copy link
Contributor Author

and on this one?

Copy link
Owner

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

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

This is a great addition, thanks!
FWIW I do plan to change the behaviour somewhat - I think the non-VCS mode works best for temporary edits and probably this mode shouldn't be checking out to the shared gohack directory.

@rogpeppe rogpeppe merged commit 4cbac3f into rogpeppe:master May 23, 2019
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.

4 participants