Skip to content

Conversation

@akinomyoga
Copy link
Collaborator

See the commit message. This is separated from PR #1357.

@akinomyoga
Copy link
Collaborator Author

There is an unresolved conversation in the original PR:

#1357 (comment) by yedayak

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

#1357 (comment) by 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.

Copy link
Collaborator

@yedayak yedayak left a comment

Choose a reason for hiding this comment

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

The change makes sense to me now, just left one comment.

@akinomyoga akinomyoga force-pushed the _comp_dequote-fallback branch 2 times, most recently from d3bfb19 to 1f81d16 Compare October 27, 2025 19:35
It is more useful to set the literal value of $1 to REPLY when $1 is
an unsafe word.  When the caller wants to use the result only when
REPLY is a safe string and suceceds without failglob, the caller can
reference the exit status $?.  When the caller wants to use any
string, it is more useful if REPLY contains a fallback value.

This patch changes the behavior so that REPLY is set to be the literal
value of $1 when an unsafe value is specified to $1.  We also change
the exit status to be 0 only when at least one word is generated by a
safe string.

When failglob happens (i.e., when no filenames match the pattern), we
continue to produce an empty array.
@akinomyoga akinomyoga force-pushed the _comp_dequote-fallback branch from 8c434ac to 683d9e5 Compare October 27, 2025 20:25
@akinomyoga akinomyoga merged commit ed61fde into scop:main Oct 27, 2025
7 checks passed
@akinomyoga akinomyoga deleted the _comp_dequote-fallback branch October 27, 2025 20:37
This was referenced Oct 27, 2025
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.

2 participants