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

Installation fixes (some may be Windows-specific, some not) #10

Merged
merged 2 commits into from
Nov 13, 2019

Conversation

benblo
Copy link
Contributor

@benblo benblo commented Nov 8, 2019

  • documentation fixes
  • fix for paths with spaces
  • python shebang fix, which you may not want, and might just be an issue with my own installation (I can only use "#!/usr/bin/env python", not "python3")

Side-note:
here's what ended up working for me to install on Windows:
git clone -c core.symlinks=true https://github.com/newren/git-filter-repo.git
make prefix="C:\Program Files\Git\mingw64" pythondir="C:\Python38\Lib\site-packages" mandir="C:\gnuwin32\share\man" install

  • The git path I believe is standard for a Git for Windows install
  • Python path is from a fresh chocolatey install
  • gnuwin32: not sure if it was installed by Git for Windows, or by me from earlier...

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
git-filter-repo Outdated
@@ -1,4 +1,4 @@
#!/usr/bin/env python3
#!/usr/bin/env python
Copy link
Owner

Choose a reason for hiding this comment

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

The commit message would need both an area prefix ("filter-repo: use python ...") and a Signed-off-by, but I can't take this change: see #4 (comment). We could add documentation that people need to change it, or even make it so if someone sets some variable then 'make install' will do the switch as part of the installation steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, thanks for the link, finally an explanation! I looked all over the place trying to understand why a fresh python 3.8 install failed so miserably, but without luck... I guess "python vs python3 shebang on windows" aren't great Google keywords :)

@newren
Copy link
Owner

newren commented Nov 12, 2019

Oh, also, as I've stated elsewhere about git.git contributions, "New contributors might be surprised by the rigor of the code review process, and might assume they just aren't good enough to contribute. It might be useful to countermand that subtle unspoken assumption by pointing out how much existing long-term contributors spend revising patches. Personally, despite doing my best to think of issues and make sure to send in really high quality patches, I still generally expect to spend at least as much time after submitting patches revising them as I did in coming up with them originally, and I'm not surprised if the time is doubled. And that's after contributing for years. I don't generally experience reviews anywhere near as thorough in other communities."

If I still have to spend lots of time revising patches I submit to git, you shouldn't feel bad when I come up with a list of improvements for your patches. If you'd like to update the first two as I suggested, I'd be happy to merge them.

@benblo
Copy link
Contributor Author

benblo commented Nov 12, 2019

Comments addressed (commit message, signed-off by, using "origin/docs")
Dropped the "python shebang" commit, ended up symlinking python.exe to python3.exe to support the python3 shebang... oh well.

@benblo benblo requested a review from newren November 12, 2019 21:55
@benblo
Copy link
Contributor Author

benblo commented Nov 12, 2019

Oh, also, as I've stated elsewhere about git.git contributions, "New contributors might be surprised by the rigor of the code review process, and might assume they just aren't good enough to contribute. It might be useful to countermand that subtle unspoken assumption by pointing out how much existing long-term contributors spend revising patches. Personally, despite doing my best to think of issues and make sure to send in really high quality patches, I still generally expect to spend at least as much time after submitting patches revising them as I did in coming up with them originally, and I'm not surprised if the time is doubled. And that's after contributing for years. I don't generally experience reviews anywhere near as thorough in other communities."

If I still have to spend lots of time revising patches I submit to git, you shouldn't feel bad when I come up with a list of improvements for your patches. If you'd like to update the first two as I suggested, I'd be happy to merge them.

As I said I don't have much experience contributing so I don't really have a frame of reference. Don't worry you didn't make me feel bad, I was indeed a little surprised 😄 but more like a stranger in a strange land ("oh that's how they roll over here? uh!"), I take it all in and learn.

Copy link
Owner

@newren newren left a comment

Choose a reason for hiding this comment

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

Looks good, just one more minor thing: can you word wrap commit messages at 72 characters?

Copy link
Owner

@newren newren left a comment

Choose a reason for hiding this comment

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

This commit looks good!

- correct paths to including missing "Documentation/" prefix
- use fully specified "origin/docs" branch in case the "docs" branch is
not checked out locally

Signed-off-by: Benoit Fouletier <bennews@free.fr>
- quote paths that may have spaces
- force ln in case the file already exists

Signed-off-by: Benoit Fouletier <bennews@free.fr>
@benblo
Copy link
Contributor Author

benblo commented Nov 12, 2019

Looks good, just one more minor thing: can you word wrap commit messages at 72 characters?

Done!

@benblo benblo requested a review from newren November 12, 2019 23:33
@BatmanAoD BatmanAoD mentioned this pull request Nov 13, 2019
@newren newren mentioned this pull request Nov 13, 2019
@newren newren merged commit 2cbd4a4 into newren:master Nov 13, 2019
@newren
Copy link
Owner

newren commented Nov 13, 2019

Thanks!

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.

None yet

2 participants