fix(java,rsync,scp): handle quoted space in filepaths properly

[akinomyoga]

This includes several different fixes (whose changes overlap with each other). See the respective commit messages for the details.

• Commit b653f4d fixes [2.14.0] rsync path completion fails if a folder name have a space #1232.
• Commit 38855ad fixes Escaping of remote filenames containing spaces is broken for rsync  #1255.

[Copilot]

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (2)

• completions/java: Language not supported
• completions/ssh: Language not supported

Comments suppressed due to low confidence (1)

test/t/test_rsync.py:77

• The cwd value 'scp' appears to be incorrect for an rsync test. It may be a typo and should likely be 'rsync'.

@pytest.mark.complete("rsync -na spaced\ ", cwd="scp")

[akinomyoga]

@Copilot[AI] The language of completions/java and completions/ssh are Bash.

• The cwd value 'scp' appears to be incorrect for an rsync test. It may be a typo and should likely be 'rsync'.

@pytest.mark.complete("rsync -na spaced\ ", cwd="scp")

This is a good point. Since the rsync completion internally uses the xfunc function that scp provides, I reused the fixture directory of scp. This is intentional but might be confusing to the readers. I'll add a code comment.

[akinomyoga]

I tried the Copilot code review, which was released today, but I'm not sure how to respond to Copilot and get further feedback.

[Copilot]

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (2)

• completions/java: Language not supported
• completions/ssh: Language not supported

Comments suppressed due to low confidence (3)

test/t/test_rsync.py:77

• The cwd parameter is set to 'scp' in a rsync test; confirm if this is intentional or if it should reference 'rsync' to avoid potential mis-routing of the test context.

@pytest.mark.complete("rsync -na spaced\ ", cwd="scp")

test/t/test_scp.py:191

• [nitpick] The expected completion string 'ced\ \ conf' is hard to interpret; consider adding a clarifying comment or refactoring the test to indicate the intended transformation explicitly.

assert completion == "ced\ \ conf"

test/t/test_scp.py:195

• [nitpick] Consider asserting an explicit expected value for completion in 'test_local_path_with_spaces_2' to ensure the test precisely validates the intended behavior.

assert completion

[akinomyoga]

test/t/test_rsync.py:77

• The cwd parameter is set to 'scp' in a rsync test; confirm if this is intentional or if it should reference 'rsync' to avoid potential mis-routing of the test context.

I already added the clarifying code comment. So, Copilot does not take account of the code comment.

test/t/test_scp.py:191

• [nitpick] The expected completion string 'ced\ \ conf' is hard to interpret;

This is a common pattern of the test suite in this project, where the text following the already input part (i.e., "spa" in the present case) is tested. It is impractical to add clarifying comments to all the occurrences of similar tests. Ideally, we can document the global structures and conventions of our test suite somewhere, but we currently don't have the documentation at all. To document it, too many things are not explained.

test/t/test_scp.py:195

• [nitpick] Consider asserting an explicit expected value for completion in 'test_local_path_with_spaces_2' to ensure the test precisely validates the intended behavior.

Thanks, this is simply a mistake. I fixed it.

[Copilot]

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (2)

• completions/java: Language not supported
• completions/ssh: Language not supported

Comments suppressed due to low confidence (1)

test/t/test_rsync.py:77

• The cwd parameter 'scp' is likely incorrect for an rsync test; it should probably be changed to 'rsync'.

@pytest.mark.complete("rsync -na spaced\ ", cwd="scp")

[Copilot]

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (2)

• completions/java: Language not supported
• completions/ssh: Language not supported

Comments suppressed due to low confidence (2)

test/t/test_rsync.py:77

• The cwd parameter is set to "scp" in a test within test_rsync.py; please verify if this is intentional or if it should be updated to reference the appropriate directory for rsync.

@pytest.mark.complete("rsync -na spaced\ ", cwd="scp")

test/t/test_scp.py:191

• Double-check that the expected escaped output matches the intended handling of quoted spaces, ensuring consistency with the test case logic.

assert completion == "ced\ \ conf"

[akinomyoga]

I added a fix for #1255 and related changes. See the commit messages for the details.

[Copilot]

Copilot reviewed 7 out of 13 changed files in this pull request and generated no comments.

Files not reviewed (6)

• bash_completion: Language not supported
• completions/java: Language not supported
• completions/make: Language not supported
• completions/mutt: Language not supported
• completions/pkgutil: Language not supported
• completions/ssh: Language not supported

Comments suppressed due to low confidence (1)

test/t/test_rsync.py:77

• The 'cwd' parameter is set to 'scp' in an rsync test, which might be incorrect. Consider using 'rsync' as the cwd to better align with the intended test context.

@pytest.mark.complete("rsync -na spaced\\ ", cwd="scp")

[JohnVillalovos]

Thanks @akinomyoga I have tested this and now rsync and scp works with directories that have spaces in their names.

For example if I have the two directories: /home/dir with space and /home/dir with target the completion now works. Where before it would get stuck at /home/dir with

[akinomyoga]

@JohnVillalovos Thanks for the confirmation!

[yedayak]

I'm not sure I understand everything here, so take it with a grain of salt, but this looks fine to me. Left some comments. This definitely fixes the issues with local files completions, but I still seem to have some issues with remote file:

scp localhost:/home/yedaya/dev/bash-completion/test/fixtures/scp/local_path/backslash-a<TAB>\\\

This happens also with main, so not a blocker.

[yedayak]

I didn't find other function that work like this in the codebase, maybe it should accept -d or something like that?

[akinomyoga]

Thanks. I tried to implement it in commit 8ca4fa9, but it seems non-trivial how the caller should pass the flag to this function. In commit 8ca4fa9, I'm currently doing

        _comp_cmd_scp__escape_path ${_dirs_only:+'-d'} -- \
            "$_escape_replacement"

I thought we might assign _dirs_only=-d instead of _dirs_only=set, but even in that case, we cannot do "$_dirs_only" because it will produce an empty argument "" without -d. Then we still need to write ${_dirs_only:+"$_dirs_only"}.

Another option might be to make _dirs_only an array and use "${_dirs_only[@]}". However, because of Bash-4.2 nounset bug, we anyway need to write ${_dirs_only[@]+"${_dirs_only[@]}"}.

[akinomyoga]

Probably the current version 8ca4fa9 is fine.

[yedayak]

This change seems reasonable to me, do you think this might cause breakage in external uses of the function?

[akinomyoga]

Yeah, that is a good point.

After this change, the cases where it failed will continue to fail. Some of the cases where it succeeded and produced an empty result [e.g., REPLY=() for _comp_dequote '$empty_variable'] will start to fail after this change. Since the caller should have considered failing cases even with the current main branch, I expect that increasing another case of failing wouldn't cause a problem.

Rather, an empty REPLY=() would have caused a problem when the caller uses $REPLY after _comp_dequote succeeds.

some=""
if _comp_dequote '$some'; then # -> results in REPLY=()
  use "$REPLY" # <- This failed for "set -u" before the present change
fi

Another change is in the value of REPLY when _comp_dequote failed. Since REPLY was not supposed to be used when _comp_dequote failed (REPLY was actually empty, REPLY=()), I expect the change in REPLY in the failing case wouldn't cause a problem, though I admit that there might be a case that the user expects the empty REPLY in principle. I think this change would rather improve the compatibility in the case where the caller isn't careful enough to check the exit status:

_comp_dequote "$some2"
use "$REPLY" # <- This failed for "set -u", or if "set -u" is not set, an empty value was used.

[yedayak]

Maybe a unit test for this will be nice?

[akinomyoga]

Thanks for the suggestion! I added tests in dcf662b.

[Copilot]

Copilot reviewed 12 out of 19 changed files in this pull request and generated no comments.

Files not reviewed (7)

• bash_completion: Language not supported
• completions/java: Language not supported
• completions/make: Language not supported
• completions/mutt: Language not supported
• completions/pkgutil: Language not supported
• completions/ssh: Language not supported
• test/t/unit/Makefile.am: Language not supported

Comments suppressed due to low confidence (4)

test/t/test_scp.py:77

• [nitpick] Consider adding an inline comment explaining the rationale behind the expected output 'ig' to clarify its significance for future maintainers.

assert completion == "ig"

test/t/test_scp.py:81

• [nitpick] Consider documenting why the expected completion 'n/' is produced in this test to improve clarity for future reviewers.

assert completion == "n/"

test/t/test_rsync.py:85

• [nitpick] Add a comment to document the variations in expected output based on different rsync versions, so the reasoning behind the dual expectation is clear.

assert completion == r"\ conf" or completion == r"\\\ conf"

test/t/test_scp.py:196

• [nitpick] Consider ensuring that the use of the pipe '|' character in the filename is intentional for testing special character handling and add an inline comment if needed.

files=["local_path_2-pipe|"],

[akinomyoga]

test/t/test_rsync.py:85

• [nitpick] Add a comment to document the variations in expected output based on different rsync versions, so the reasoning behind the dual expectation is clear.

assert completion == r"\ conf" or completion == r"\\\ conf"

35c1af4

test/t/test_scp.py:196

• [nitpick] Consider ensuring that the use of the pipe '|' character in the filename is intentional for testing special character handling and add an inline comment if needed.

files=["local_path_2-pipe|"],

9608d1e

[akinomyoga]

@yedayak I added many changes and squashed them into the corresponding commits. The changes after your previous review can be checked on b456d8d..1fd977d.

[akinomyoga]

(rebased)

[scop]

Instead of lumping unrelated but conflicting/overlapping PR's together, especially as many as there are in this one, I'd suggest filing several and basing them off each other. This is a good way to optimize for the reviewers -- ones with multiple unrelated ones tend to grow large, and entail many context switches for the reviewer. For example, I could have easily reviewed and approved a few of these now and we could get them out of the way, but I cannot find time and attention to do it all now, and I'm not sure when I will. (If others do, please go ahead!)

So the first PR would have branch-1 based off main, the second's branch branch-2 based off branch-1 (and set as base in the PR), etc etc. I believe for this to work the PR's need to be branches in this repo and not a fork, so this is an option directly available for people with contributor access.

[akinomyoga]

Thank you. Yes..., I have to agree with you that this PR happened to become too large.

I usually tend to combine multiple changes in a reasonable size of a PR when I introduce many conflicting changes to avoid the complication of PR dependency and conflicts. If every tiny change is processed one by one as a PR, it would take a longer time until I forget the set of changes and their relationships. However, I admit that I failed to decide on the proper size of this PR. After some adjustments, the number of commits increased too much.

I'll split the PR.

[akinomyoga]

I submitted PR #1371.

I believe for this to work the PR's need to be branches in this repo and not a fork, so this is an option directly available for people with contributor access.

Sorry, I missed this part, though I'm afraid that I'm possibly still missing the intent. Is the idea that another collaborator can split the PR (instead of the original submitter)?