completion/svn: import existing, remove copy

[gaelicWizard]

Description

Delete our copy of the completion function for subversion; it's part of the upstream package. Replace it with an adaptation of completion/git to find the subversion completion file in non-standard locations.

Motivation and Context

This addresses #1818.

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code follows the code style of this project.
• If my change requires a change to the documentation, I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
• I have added tests to cover my changes, and all the new and existing tests pass.

[NoahGorny]

Very nice, I always like these type of changes 😄

[davidpfarrell]

Hi Team,

I'm not feeling this one.

If the goal is to generally hit a 'global' etc/bash_completion.d folder (i.e /etc/bash_completion.d, /usr/local/etc/bash_completion.d, etc) then I think the load is better left to the system bash completion loaders.

Is there any documentation that svn installed via package managers generally ship with a version of the completion that is not placed into the global bash-completion hierarchy? And if so, are they stored in folder structure relative to the binary?

Our completion scripts/plugins should not be looking in the standard places for completions. ie. places that system completion loaders would already be looking.

I see Bash-its role in completions more as:

1. Look in known non-standard locations (ie. package manager is known to place completion somewhere like /var/opt/..., etc) where system completion would not find the file.
2. Vendor the completion if its available online but NOT always shipped with the binary. Use system (non-standard-location) completion if present, vendored completion as fall-back
3. Contain completion in bashit completions/ IFF the completion exists no where else as to be vendored. Use system (non-standard-location) completion if present, bashit completion as fall-back

So the right course of action is based on the general availability (and placement) of the completion as part of the standard SVN installation via package managers.

At the very least, I'd be happy to have bashit vendor this completion

[gaelicWizard]

@davidpfarrell, I'm on-board with all of what you said here. How can we check if it's normally included with package managers? Just check a few distros?

[davidpfarrell]

How can we check if it's normally included with package managers? Just check a few distros?

Yeah that would likely suffice - I can confirm brew brings it and installs it in the auto-loader location

[davidpfarrell]

Debian Stretch:

• https://packages.debian.org/stretch/amd64/subversion/filelist

[davidpfarrell]

Random RPM suggests they get stored in the right spot on redhat/fedora:

• https://fedora.pkgs.org/34/fedora-x86_64/subversion-1.14.1-1.fc34.x86_64.rpm.html

[davidpfarrell]

I'm fine removing it entirely (vs vendoring it), but doing so becomes a breaking change ...

just the same, I tend to think its the right move - My feeling is that the svn completion has worked its way into the package managers and that the various systems have adopted good support for bash completions in general

[gaelicWizard]

After just going through completion/git, I'm going to try to do that here. That way, we keep the uncommon possibility of svn being installed somewhere weird, don't create a missing file to produce errors for users, and still avoid having to cary a copy of the completion functions.

I'm marking this PR as draft again until I push the new version.

[NoahGorny]

Hi @gaelicWizard, is this PR ready?

[gaelicWizard]

Ready!

[NoahGorny]

@davidpfarrell, wanna take another look?

[gaelicWizard]

Anything else needed for this PR?

[NoahGorny]

gentle ping @davidpfarrell

[davidpfarrell]

Hi All !
So it does appear that this new version has parity to the git completion ...
BUT I just don't buy that either completion script is correct.

In addition to my notes:

• re: xcrun
  The only real use I see for xcrun is as a (possible) means of detecting the false-positive for old mac os's where the svn binary was present but only to give the not-supported error - If xcrun generates an error message in such a case then I could see it being used inside the base theme setup as a better way to help detect+reject the false positive.

[gaelicWizard]

@davidpfarrell, the stubs in /usr/bin are just that: stubs. The rest of the package is inside $DEVELOPER_DIR, which is determined by xcrun.

So, /usr/bin/git may work perfectly because the stub correctly executes /Library/Developer/CommandLineTools/usr/bin/git (or /Applications/Xcode.app/Contents/Developer/usr/bin/git), but /usr/share/git-core simply doesn't exist. /Library/Developer/CommandLineTools/usr/share/git-core does exist and is populated normally, including the bundled completion scripts.

Without xcrun, we can't locate any files other than the executables stub'd from the readonly root partition.

[davidpfarrell]

Thanks for taking time to 'splain - I have a little better understanding of the xcrun usage now ...

We could guard against a version mismatch ...

I'm inclined to find value in this idea - At some point it might be better to encourage the usage to fetch a completion over activating a random one ...

[gaelicWizard]

Thanks for taking time to 'splain - I have a little better understanding of the xcrun usage now ...

We could guard against a version mismatch ...

I'm inclined to find value in this idea - At some point it might be better to encourage the usage to fetch a completion over activating a random one ...

Peeps should absolutely be just using the regular completion loaded by bash-completions; the only reason I did any attempt to load other potential completions is because you didn't want a breaking change by just deleting this file.

[davidpfarrell]

because you didn't want a breaking change by just deleting this file.

Ugh, indeed ....

OK so I've confirmed that the xcrun error output doesn't make it to the variable assignment (likely outs to stderr) ...

not sure if we should have || true to avoid exit on error settings?

Just the same, marking approved - thanks !

[gaelicWizard]

not sure if we should have || true to avoid exit on error settings?

bash-3.2$ set -e
bash-3.2$ echo "$? $(false) $?"
0  1
bash-3.2$ false
=== Window terminated (Wed Jan  5 13:25:34 2022) ===

exit-on-error (set -e) does not apply to commands inside subshells. The assignment is successful, even if the command inside the subshell is not. This is explicitly documented Bash behavior that "$?" is set after the subshell, but that it does not treat it as an error. (However, if "$?" is set in such a way at the end of a function or the end of a sourced file, then the function will have that return value and that will be an error.)