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

Suggest full-path refresh() in failure message #1844

Merged
merged 2 commits into from Feb 24, 2024

Conversation

EliahKagan
Copy link
Contributor

@EliahKagan EliahKagan commented Feb 23, 2024

This makes the refresh failure message fill in the made-up path "/full/path/to/git" as an argument to git.refresh. This small expansion serves two purposes:

  • To clarify what kind of explicit refresh is being described: passing a path to git.refresh.
  • To clarify what kind of path should typically be used when this is done: a full path.

The latter clarification is more important because it cannot be reasoned out from the message and the interface of git.refresh and related functions, though it can be learned from their docstrings since #1829, from their implementations, or by trial and error.

I'm not sure how much I like the change in the error message. It seems better than before, but there are three weaknesses:

  • The path passed to git.refresh doesn't actually have to be a full path. Since this is an item listed under "must be specified in one of the following ways," from this proposed wording one can infer the false claim that git.refresh(path), where path is a relative path, never works. It is rarely what one would want, but it can work and has been deliberately supported (just not clearly documented) for quite some time.
  • To limit the length of the message, which already packs in a lot of information (the string it is part of is concatenated to other strings to produce a message that is often taller than one's terminal), I only added an argument. I didn't add a statement that one should usually pass a full path. I think that's the right choice, given how much information is already in this message and how one should usually use one of the other techniques to let GitPython find the git executable anyway. But I'm not sure.
  • Full paths don't always look like that. In particular, on Windows they usually start with a drive letter. Maybe the message should be customized for Windows to start with C:? (C: is not always the relevant drive, but I think having it vary based on the current drive or any other aspects of the current location is not justified.)

This pull request also makes a small change to the wording in the Git.refresh docstring from #1829, which could have been read to mean the opposite of what I intended.

This does not suggest or recommend *preferring* to explicitly call
refresh() over the other other techniques, but it clarifies that
the use of refresh() being presented needs a path argument. It also
shows that path in the form of a full path, so users are less
likely to be misled into thinking a command name or other relative
path should be passed to refresh(), which should rarely be done
(since refresh(path) resolves path).
@EliahKagan EliahKagan marked this pull request as ready for review February 23, 2024 23:48
@Byron
Copy link
Member

Byron commented Feb 24, 2024

Thanks a lot for making this improvement!

  • Full paths don't always look like that. In particular, on Windows they usually start with a drive letter. Maybe the message should be customized for Windows to start with C:? (C: is not always the relevant drive, but I think having it vary based on the current drive or any other aspects of the current location is not justified.)

That sounds like a very mindful way of trying to provide example paths that have a closer resemblance to valid paths on that platform. I'd go as far and use the typical installation path for git on Linux and Windows as example here. But I wouldn't want to let this PR wait based on what feels like cosmetics. And while writing this I realize that maybe the path shouldn't look too realistic, and instead be obviously a placeholder like refresh(<full-path-to-git>), so nobody could get the idea of copy-pasting.

@Byron Byron merged commit 1e044ea into gitpython-developers:main Feb 24, 2024
26 checks passed
@EliahKagan EliahKagan deleted the refresh-message branch February 24, 2024 16:08
@EliahKagan
Copy link
Contributor Author

One small advantage of "/full/path/to/git" over <full-path-to-git> is that the former hints that the git executable filename needs to be part of the path (rather than it being a path to the directory).

This may not be a sufficient reason to prefer it, I am not sure. A benefit of <full-path-to-git> is that it is not specific to any platform.

On Windows, "/full/path/to/git" could be changed to R"C:\full\path\to\git", with these further considerations:

  • Maybe R"C:\full\path\to\git.exe" is better. The version with the .exe suffix will really make clear for Windows users that the executable is expected to be part of the path. But it may be misleading, since one of the reasons to specify a full path on Windows can be to make it use a .cmd or .bat file instead. (The .exe suffix is automatically tried if absent, but other suffixes, even if in the PATHEXT environment variable, are not tried, unless shell=True, whose risks outweigh the benefit of using it for that purpose.)
  • The presence of C:\ is probably fine but may be jarring to users whose drive letter is different. (It is also possible to be using paths with no drive letter, but users who are doing this will probably know how to accommodate instructions to their case.)

A benefit of <full-path-to-git> may be that all those details fall away.

For showing common paths, most Unix-like systems that package git probably install it at /usr/bin/git. I think the most common location on Windows is C:\Program Files\Git\cmd\git.exe, though I'm not sure about this because I use scoop to install it, which puts it elsewhere. Note the presence of the drive letter C:, which is the most common system-drive drive letter, but not universal.

@Byron
Copy link
Member

Byron commented Feb 24, 2024

Maybe <full-path-to-git-binary> would clarify this form more. In any case, it's absolutely up to you to keep it as is, or change it in any way you find most suitable.

EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Feb 25, 2024
This presents it more symbolically, rather than in an example-like
style that could confuse. See discussion in gitpython-developers#1844.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Feb 26, 2024
This further improves the text previously introduced in gitpython-developers#1829 and
improved in gitpython-developers#1844.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Feb 26, 2024
This presents it more symbolically, rather than in an example-like
style that could confuse. See discussion in gitpython-developers#1844.
@EliahKagan
Copy link
Contributor Author

Maybe <full-path-to-git-binary> would clarify this form more. In any case, it's absolutely up to you to keep it as is, or change it in any way you find most suitable.

I've included a variation of this in #1850 (in cd8a312).

renovate bot added a commit to allenporter/flux-local that referenced this pull request Mar 31, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [GitPython](https://togithub.com/gitpython-developers/GitPython) |
`==3.1.42` -> `==3.1.43` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/GitPython/3.1.43?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/GitPython/3.1.43?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/GitPython/3.1.42/3.1.43?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/GitPython/3.1.42/3.1.43?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>gitpython-developers/GitPython (GitPython)</summary>

###
[`v3.1.43`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.43)

[Compare
Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.42...3.1.43)

#### Particularly Important Changes

These are likely to affect you, please do take a careful look.

- Issue and test deprecation warnings by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1886
- Fix version_info cache invalidation, typing, parsing, and
serialization by [@&#8203;EliahKagan](https://togithub.com/EliahKagan)
in
[gitpython-developers/GitPython#1838
- Document manual refresh path treatment by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1839
- Improve static typing and docstrings related to git object types by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1859

#### Other Changes

- Test in Docker with Alpine Linux on CI by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1826
- Build online docs (RTD) with -W and dependencies by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1843
- Suggest full-path refresh() in failure message by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1844
- `repo.blame` and `repo.blame_incremental` now accept `None` as the
`rev` parameter. by [@&#8203;Gaubbe](https://togithub.com/Gaubbe) in
[gitpython-developers/GitPython#1846
- Make sure diff always uses the default diff driver when
`create_patch=True` by
[@&#8203;can-taslicukur](https://togithub.com/can-taslicukur) in
[gitpython-developers/GitPython#1832
- Revise docstrings, comments, and a few messages by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1850
- Expand what is included in the API Reference by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1855
- Restore building of documentation downloads by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1856
- Revise type annotations slightly by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1860
- Updating regex pattern to handle unicode whitespaces. by
[@&#8203;jcole-crowdstrike](https://togithub.com/jcole-crowdstrike) in
[gitpython-developers/GitPython#1853
- Use upgraded pip in test fixture virtual environment by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1864
- lint: replace `flake8` with `ruff` check by
[@&#8203;Borda](https://togithub.com/Borda) in
[gitpython-developers/GitPython#1862
- lint: switch Black with `ruff-format` by
[@&#8203;Borda](https://togithub.com/Borda) in
[gitpython-developers/GitPython#1865
- Update readme and tox.ini for recent tooling changes by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1868
- Split tox lint env into three envs, all safe by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1870
- Slightly broaden Ruff, and update and clarify tool configuration by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1871
- Add a "doc" extra for documentation build dependencies by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1872
- Describe `Submodule.__init__` parent_commit parameter by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1877
- Include TagObject in git.types.Tree_ish by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1878
- Improve Sphinx role usage, including linking Git manpages by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1879
- Replace all wildcard imports with explicit imports by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1880
- Clarify how tag objects are usually tree-ish and commit-ish by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1881

#### New Contributors

- [@&#8203;Gaubbe](https://togithub.com/Gaubbe) made their first
contribution in
[gitpython-developers/GitPython#1846
- [@&#8203;can-taslicukur](https://togithub.com/can-taslicukur) made
their first contribution in
[gitpython-developers/GitPython#1832
- [@&#8203;jcole-crowdstrike](https://togithub.com/jcole-crowdstrike)
made their first contribution in
[gitpython-developers/GitPython#1853
- [@&#8203;Borda](https://togithub.com/Borda) made their first
contribution in
[gitpython-developers/GitPython#1862

**Full Changelog**:
gitpython-developers/GitPython@3.1.42...3.1.43

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/allenporter/flux-local).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
lettuce-bot bot added a commit to lettuce-financial/github-bot-signed-commit that referenced this pull request Apr 1, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [GitPython](https://togithub.com/gitpython-developers/GitPython) |
`==3.1.42` -> `==3.1.43` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/GitPython/3.1.43?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/GitPython/3.1.43?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/GitPython/3.1.42/3.1.43?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/GitPython/3.1.42/3.1.43?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>gitpython-developers/GitPython (GitPython)</summary>

###
[`v3.1.43`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.43)

[Compare
Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.42...3.1.43)

#### Particularly Important Changes

These are likely to affect you, please do take a careful look.

- Issue and test deprecation warnings by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1886
- Fix version_info cache invalidation, typing, parsing, and
serialization by [@&#8203;EliahKagan](https://togithub.com/EliahKagan)
in
[gitpython-developers/GitPython#1838
- Document manual refresh path treatment by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1839
- Improve static typing and docstrings related to git object types by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1859

#### Other Changes

- Test in Docker with Alpine Linux on CI by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1826
- Build online docs (RTD) with -W and dependencies by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1843
- Suggest full-path refresh() in failure message by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1844
- `repo.blame` and `repo.blame_incremental` now accept `None` as the
`rev` parameter. by [@&#8203;Gaubbe](https://togithub.com/Gaubbe) in
[gitpython-developers/GitPython#1846
- Make sure diff always uses the default diff driver when
`create_patch=True` by
[@&#8203;can-taslicukur](https://togithub.com/can-taslicukur) in
[gitpython-developers/GitPython#1832
- Revise docstrings, comments, and a few messages by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1850
- Expand what is included in the API Reference by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1855
- Restore building of documentation downloads by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1856
- Revise type annotations slightly by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1860
- Updating regex pattern to handle unicode whitespaces. by
[@&#8203;jcole-crowdstrike](https://togithub.com/jcole-crowdstrike) in
[gitpython-developers/GitPython#1853
- Use upgraded pip in test fixture virtual environment by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1864
- lint: replace `flake8` with `ruff` check by
[@&#8203;Borda](https://togithub.com/Borda) in
[gitpython-developers/GitPython#1862
- lint: switch Black with `ruff-format` by
[@&#8203;Borda](https://togithub.com/Borda) in
[gitpython-developers/GitPython#1865
- Update readme and tox.ini for recent tooling changes by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1868
- Split tox lint env into three envs, all safe by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1870
- Slightly broaden Ruff, and update and clarify tool configuration by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1871
- Add a "doc" extra for documentation build dependencies by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1872
- Describe `Submodule.__init__` parent_commit parameter by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1877
- Include TagObject in git.types.Tree_ish by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1878
- Improve Sphinx role usage, including linking Git manpages by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1879
- Replace all wildcard imports with explicit imports by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1880
- Clarify how tag objects are usually tree-ish and commit-ish by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1881

#### New Contributors

- [@&#8203;Gaubbe](https://togithub.com/Gaubbe) made their first
contribution in
[gitpython-developers/GitPython#1846
- [@&#8203;can-taslicukur](https://togithub.com/can-taslicukur) made
their first contribution in
[gitpython-developers/GitPython#1832
- [@&#8203;jcole-crowdstrike](https://togithub.com/jcole-crowdstrike)
made their first contribution in
[gitpython-developers/GitPython#1853
- [@&#8203;Borda](https://togithub.com/Borda) made their first
contribution in
[gitpython-developers/GitPython#1862

**Full Changelog**:
gitpython-developers/GitPython@3.1.42...3.1.43

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/lettuce-financial/github-bot-signed-commit).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants