Suggest full-path refresh() in failure message by EliahKagan · Pull Request #1844 · gitpython-developers/GitPython · GitHub

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).

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.

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.

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.

This presents it more symbolically, rather than in an example-like style that could confuse. See discussion in gitpython-developers#1844.

This further improves the text previously introduced in gitpython-developers#1829 and improved in gitpython-developers#1844.

This presents it more symbolically, rather than in an example-like style that could confuse. See discussion in gitpython-developers#1844.

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).

EliahKagan added 2 commits February 23, 2024 17:37

Fix ambiguous wording in Git.refresh docstring

7d96a1a

EliahKagan marked this pull request as ready for review February 23, 2024 23:48

Byron merged commit 1e044ea into gitpython-developers:main Feb 24, 2024

EliahKagan deleted the refresh-message branch February 24, 2024 16:08

EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Feb 26, 2024

Fix typos and further clarify Git.refresh docstring

9b5531b

This further improves the text previously introduced in gitpython-developers#1829 and improved in gitpython-developers#1844.

EliahKagan mentioned this pull request Feb 29, 2024

Revise docstrings, comments, and a few messages #1850

Merged

Suggest full-path refresh() in failure message by EliahKagan · Pull Request #1844 · gitpython-developers/GitPython · GitHub

Uh oh!

Uh oh!

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

Suggest full-path refresh() in failure message#1844
Byron merged 2 commits into
gitpython-developers:mainfrom
EliahKagan:refresh-message

EliahKagan commented Feb 23, 2024 •
edited
Loading

Uh oh!

Uh oh!

Byron commented Feb 24, 2024

Uh oh!

EliahKagan commented Feb 24, 2024

Uh oh!

Byron commented Feb 24, 2024

Uh oh!

EliahKagan commented Feb 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Suggest full-path refresh() in failure message by EliahKagan · Pull Request #1844 · gitpython-developers/GitPython · GitHub

Uh oh!

Uh oh!

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

Suggest full-path refresh() in failure message#1844Byron merged 2 commits intogitpython-developers:maingitpython-developers/GitPython:mainfrom EliahKagan:refresh-messageEliahKagan/GitPython:refresh-messageCopy head branch name to clipboard

Conversation

EliahKagan commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Byron commented Feb 24, 2024

Uh oh!

EliahKagan commented Feb 24, 2024

Uh oh!

Byron commented Feb 24, 2024

Uh oh!

EliahKagan commented Feb 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Suggest full-path refresh() in failure message#1844
Byron merged 2 commits into
gitpython-developers:mainfrom
EliahKagan:refresh-message

EliahKagan commented Feb 23, 2024 •
edited
Loading