Skip to content

Modifying tab completion for files to be relative.#626

Closed
cdonnellytx wants to merge 3 commits intodahlbyk:masterfrom
cdonnellytx:relativePathTabCompletion
Closed

Modifying tab completion for files to be relative.#626
cdonnellytx wants to merge 3 commits intodahlbyk:masterfrom
cdonnellytx:relativePathTabCompletion

Conversation

@cdonnellytx
Copy link
Copy Markdown
Contributor

Right now if you are in a subdirectory of a Git repository, and you attempt to tab complete a list of files, you get repo-relative paths.

This patch modifies the tab completion to get paths relative to your current path so that the paths can be used properly in git commands like add.

I ran the tests by hand (gci -Filter '*.ps1' .\test\ | ForEach-Object { & $_.FullName }) and did not find any test differences from master.

Please let me know if you need more information.

Thanks!
# Chris

@cdonnellytx
Copy link
Copy Markdown
Contributor Author

Not sure how my changes are causing AppVeyor to fail on getting branches... will look into it later this week.

@rkeithhill
Copy link
Copy Markdown
Collaborator

I checked in a fix for the AppVeyor tests into master, if you want to rebase and try again.

Comment thread test/TabExpansion.Tests.ps1 Outdated

&$gitbin config alias.$alias checkout
(&$gitbin config --get-all alias.$alias).Count | Should Be 1
@(&$gitbin config --get-all alias.$alias).Count | Should Be 1
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not needed e.g. (1,2).count returns 2 , (1).count returns 1 and ($null).count returns 0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll go ahead and revert this. (The thing I was seeing was that it was resolving the expression as a single string and not an array...)

If you are in a subdirectory of a repo, tab-completion will now honor your current location.
Resolve-Path always renders files with a leading ".\" (Windows) or "./" (everything else).
This differs from how paths were returned before, so now the tests are changing to reflect this.
It is also now consistent with how tab completion works for other PowerShell commands (e.g., Get-ChildItem), so it shouldn't be a problem.
@cdonnellytx cdonnellytx force-pushed the relativePathTabCompletion branch from 223d0d1 to 7bb89e4 Compare October 4, 2018 14:30
@cdonnellytx
Copy link
Copy Markdown
Contributor Author

cdonnellytx commented Oct 4, 2018

OK, fixed.

(I removed my attempt to fix CI from history entirely, which appears to have also hidden/deleted the GitHub discussion on that. Will be more mindful of that in the future and do a revert instead of chopping off HEAD.)

@rkeithhill
Copy link
Copy Markdown
Collaborator

Overall looks good but @dahlbyk will do the final review. One thing I'm not 100% sure about is if a .git dir's parent is always what you want to be relative to. I think so but then I'm not as familiar with how that works with working tree's, subtrees, etc.

@rkeithhill rkeithhill requested a review from dahlbyk October 4, 2018 15:22
Copy link
Copy Markdown
Collaborator

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

LGTM other than the unnecessary [string].

Comment thread src/GitTabExpansion.ps1 Outdated
@rkeithhill
Copy link
Copy Markdown
Collaborator

@cdonnellytx Finally getting back around to this and in my testing (without this PR) I'm seeing dir relative paths. For instance, I modify a file in the test dir (Shared.ps1) and then I cd into the test dir. When I tab complete either git add <tab> or git checkout -- <tab> I get the correct, relative path i.e. Shared.ps1 and not test/Shared.ps1. Which git commands are not tab-completing paths with the correct relative path?

@cdonnellytx
Copy link
Copy Markdown
Contributor Author

cdonnellytx commented Jun 27, 2019

@rkeithhill I'm seeing it on git add <tab> and git checkout -- <tab>.

Steps to reproduce:

  1. Load posh-git
  2. cd to a repo with a change
  3. git add <tab> - this is correct
  4. cd to a subdir of the repo (e.g. src/)
  5. git add <tab> - returns full path

(EDIT: formatting)

@rkeithhill
Copy link
Copy Markdown
Collaborator

@cdonnellytx Sorry for the delay. What version of posh-git are you using? I do not see what you're seeing on 1.0.0-beta.3 (or the tip of master). This is what I see:

posh-git-add-path

@cdonnellytx
Copy link
Copy Markdown
Contributor Author

cdonnellytx commented Jul 8, 2019

OK, so I figured it out... and it amounts to my forgetting something pretty major 🤦‍♂ : I had made a local change to the git status call in GitUtils.ps1 to use --porcelain, since that is supposed to be what you use in scripts per the man page:

--porcelain[=<version>]

Give the output in an easy-to-parse format for scripts. This is similar to the short output, but will remain stable across Git versions and regardless of user configuration. See below for details.

This is what the change looked like in GitUtils.ps1:

- $status = Invoke-Utf8ConsoleCommand { git -c core.quotepath=false -c color.status=false status $untrackedFilesOption --short --branch 2>$null }
+ $status = Invoke-Utf8ConsoleCommand { git status $untrackedFilesOption --porcelain --branch 2>$null }

I meant to submit a PR for this change, but never got to it.

But worse than that, what I did not realize was this:

The description of the short format above also describes the porcelain format, with a few exceptions:

  1. The user’s color.status configuration is not respected; color will always be off.
  2. The user’s status.relativePaths configuration is not respected; paths shown will always be relative to the repository root.

In other words, the bug is something I introduced by using --porcelain. 🤦‍♂ I apologize for that.

The good news, though, is that this will still work, since --porcelain is guaranteed to never change (per the Git maintainers), while human-readable output has no such guarantee. I don't know if that's worth the extra changes at this time, though.

One other possibility is to use porcelain format v2, which does honor status.relativePaths and thus would not be as invasive a change. The only downside there is that may require raising the minimum version of Git that posh-git supports to 2.11.0 (commit).

Thoughts?

# Chris

(EDIT: make things more obvious)

@rkeithhill
Copy link
Copy Markdown
Collaborator

Nice sleuthing. Thanks for taking the time to track that down!! RE using porcelain format v2, that's a question for @dahlbyk.

@dahlbyk
Copy link
Copy Markdown
Owner

dahlbyk commented Mar 31, 2022

One other possibility is to use porcelain format v2, which does honor status.relativePaths and thus would not be as invasive a change. The only downside there is that may require raising the minimum version of Git that posh-git supports to 2.11.0 (commit).

We do require Git 2.15+ now, and I am considering using --porcelain=2 --branch. Especially after the quoted-path fix needed in #892.

Anyway, thanks for trying to help…almost three years ago. 🤦‍♂️

@dahlbyk dahlbyk closed this Mar 31, 2022
@cdonnellytx
Copy link
Copy Markdown
Contributor Author

No worries, it happens.

@cdonnellytx cdonnellytx deleted the relativePathTabCompletion branch March 31, 2022 15:01
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.

3 participants