Skip to content

fix: strip path from usage output and shell-completion scripts#2950

Merged
crazy-max merged 1 commit intodocker:masterfrom
thaJeztah:fix_usage_and_completion
Feb 2, 2025
Merged

fix: strip path from usage output and shell-completion scripts#2950
crazy-max merged 1 commit intodocker:masterfrom
thaJeztah:fix_usage_and_completion

Conversation

@thaJeztah
Copy link
Member

Before this patch, both "usage" and shell-completion scripts would preserve
the path of the invoked command, which was especially problematic for the
completion-scripts, because Cobra's completion depends on Command.Name()
for this (see 1, 2);

./bin/build/buildx --help | head -n 5
Extended build capabilities with BuildKit

Usage:
  ./bin/build/buildx
  ./bin/build/buildx [command]

./bin/build/buildx completion bash | head -n 3
# bash completion V2 for ./bin/build/buildx                   -*- shell-script -*-

__./bin/build/buildx_debug()

This would also be problematic if the path contained a space, for example;

ln -s $(pwd)/bin/build $(pwd)/bin/Program\ Files

./bin/Program\ Files/buildx completion bash | head -n 3
# bash completion V2 for ./bin/Program                        -*- shell-script -*-

__./bin/Program_debug()

With this patch, the path is stripped to prevent this issue;

make

./bin/build/buildx --help | head -n 5
Extended build capabilities with BuildKit

Usage:
  buildx
  buildx [command]

./bin/build/buildx completion bash | head -n 3
# bash completion V2 for buildx                               -*- shell-script -*-

__buildx_debug()

./bin/Program\ Files/buildx completion bash | head -n 3
# bash completion V2 for buildx                               -*- shell-script -*-

__buildx_debug()

It's worth noting that this patch only fixes these basic issues. Other cases
are not yet addressed, and may need fixes in Cobra because (especially for
the completion scripts) it should likely not conflate "Name" with "executable".

For example, command.Name() does not handle situations where the executable
itself has a space in its name:

ln -s $(pwd)/bin/build/buildx $(pwd)/bin/build/hello\ world

./bin/build/hello\ world completion bash | head -n 3
# bash completion V2 for hello                                -*- shell-script -*-

__hello_debug()

Other, less problematic, issues to address are case-insensitive filesystems,
where the binary can be invoked with any case;

./bin/build/bUiLdX --help | head -n 5
Extended build capabilities with BuildKit

Usage:
  bUiLdX
  bUiLdX [command]

./bin/build/bUiLdX completion bash | head -n 3
# bash completion V2 for bUiLdX                               -*- shell-script -*-

__bUiLdX_debug()

Before this patch, both "usage" and shell-completion scripts would preserve
the path of the invoked command, which was especially problematic for the
completion-scripts, because Cobra's completion depends on Command.Name()
for this (see [1], [2]);

    ./bin/build/buildx --help | head -n 5
    Extended build capabilities with BuildKit

    Usage:
      ./bin/build/buildx
      ./bin/build/buildx [command]

    ./bin/build/buildx completion bash | head -n 3
    # bash completion V2 for ./bin/build/buildx                   -*- shell-script -*-

    __./bin/build/buildx_debug()

This would also be problematic if the path contained a space, for example;

    ln -s $(pwd)/bin/build $(pwd)/bin/Program\ Files

    ./bin/Program\ Files/buildx completion bash | head -n 3
    # bash completion V2 for ./bin/Program                        -*- shell-script -*-

    __./bin/Program_debug()

With this patch, the path is stripped to prevent this issue;

    ./bin/build/buildx --help | head -n 5
    Extended build capabilities with BuildKit

    Usage:
      buildx
      buildx [command]

    ./bin/build/buildx completion bash | head -n 3
    # bash completion V2 for buildx                               -*- shell-script -*-

    __buildx_debug()

    ./bin/Program\ Files/buildx completion bash | head -n 3
    # bash completion V2 for buildx                               -*- shell-script -*-

    __buildx_debug()

It's worth noting that this patch only fixes these basic issues. Other cases
are not yet addressed, and may need fixes in Cobra because (especially for
the completion scripts) it should likely not conflate "Name" with "executable".

For example, command.Name() does not handle situations where the executable
itself has a space in its name:

    ln -s $(pwd)/bin/build/buildx $(pwd)/bin/build/hello\ world

    ./bin/build/hello\ world completion bash | head -n 3
    # bash completion V2 for hello                                -*- shell-script -*-

    __hello_debug()

Other, less problematic, issues to address are case-insensitive filesystems,
where the binary can be invoked with any case;

    ./bin/build/bUiLdX --help | head -n 5
    Extended build capabilities with BuildKit

    Usage:
      bUiLdX
      bUiLdX [command]

    ./bin/build/bUiLdX completion bash | head -n 3
    # bash completion V2 for bUiLdX                               -*- shell-script -*-

    __bUiLdX_debug()

[1]: https://github.com/spf13/cobra/blob/v1.8.1/bash_completionsV2.go#L24-L39
[2]: https://github.com/spf13/cobra/blob/v1.8.1/command.go#L1502-L1510

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@crazy-max LGTY?

@crazy-max crazy-max merged commit cf8bf9e into docker:master Feb 2, 2025
127 checks passed
@thaJeztah thaJeztah deleted the fix_usage_and_completion branch February 2, 2025 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants