From: Ben Knoble <ben.knoble@gmail.com>
Date: Sun, 08 Mar 2026 20:56:57 GMT
Subject: Re: [PATCH] quiltimport: fix backslash expansion in patch subjects
Message-ID: <92EC21F4-52E1-4FE8-A1B8-5878D6CC654C@gmail.com>
In-Reply-To: <20260308165531.40655-1-sashal@kernel.org>


> Le 8 mars 2026 à 13:01, Sasha Levin <sashal@kernel.org> a écrit :
> 
> echo interprets backslash sequences, so a patch with "\0" in its
> subject has that expanded into a NUL byte, which git commit-tree
> rejects.

Echo _shouldn’t_ do that without flags like -e depending on your implementation, but I wouldn’t put it past echo ;) hence my preference for printf in scripts.

More interestingly, though: do the patch-names or subjects being adjusted below contain such a sequence? They look like user input, so possibly (and we can’t be sure they don’t).

It might be nice to say more about what “rejects” means (errors confusingly? Truncates user input?), but otherwise I expect this is reasonable.

> Use printf '%s\n' instead, which doesn't interpret the string.
> 
> Also quote $tmp_dir to handle paths with spaces.
> 
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> git-quiltimport.sh | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/git-quiltimport.sh b/git-quiltimport.sh
> index eb34cda409..38302d28c9 100755
> --- a/git-quiltimport.sh
> +++ b/git-quiltimport.sh
> @@ -79,7 +79,7 @@ tmp_info="$tmp_dir/info"
> # Find the initial commit
> commit=$(git rev-parse HEAD)
> 
> -mkdir $tmp_dir || exit 2
> +mkdir "$tmp_dir" || exit 2

We prefer to leave such “while at it” changes in separate patches, so perhaps a preliminary cleanup “quote variable expansions to handle whitespace” or some such step would help?

(I didn’t look past the context to see if tmp_dir may have whitespace or shell meta characters.)

> while read patch_name level garbage <&3
> do
>    case "$patch_name" in ''|'#'*) continue;; esac
> @@ -101,7 +101,7 @@ do
>        echo "$patch_name doesn't exist. Skipping."
>        continue
>    fi
> -    echo $patch_name
> +    printf '%s\n' "$patch_name"

Does this go to commit-tree? Or just protecting the output for the user’s terminal?

>    git mailinfo $MAILINFO_OPT "$tmp_msg" "$tmp_patch" \
>        <"$QUILT_PATCHES/$patch_name" >"$tmp_info" || exit 3
>    test -s "$tmp_patch" || {
> @@ -142,14 +142,14 @@ do
>    SUBJECT=$(sed -ne 's/Subject: //p' "$tmp_info")
>    export GIT_AUTHOR_DATE SUBJECT
>    if [ -z "$SUBJECT" ] ; then
> -        SUBJECT=$(echo $patch_name | sed -e 's/.patch$//')
> +        SUBJECT=$(printf '%s' "$patch_name" | sed -e 's/.patch$//')

Interesting. I think POSIX sh  supports the ${x#suffix} expansion, which could avoid sed. I think it unlikely the “.” in the RE is intended to match any character rather than a literal dot. But should be done a separate patch and could be left for another series if you wanted (assuming my memory of supported expansions is correct).

Importantly, this does get fed to commit-tree below…

>    fi
> 
>    if [ -z "$dry_run" ] ; then
>        git apply --index -C1 ${level:+"$level"} "$tmp_patch" &&
>        tree=$(git write-tree) &&
> -        commit=$( { echo "$SUBJECT"; echo; cat "$tmp_msg"; } | git commit-tree $tree -p $commit) &&
> +        commit=$( { printf '%s\n' "$SUBJECT"; echo; cat "$tmp_msg"; } | git commit-tree $tree -p $commit) &&
>        git update-ref -m "quiltimport: $patch_name" HEAD $commit || exit 4
>    fi

… so may need protected as described. Neat. 

> done 3<"$QUILT_SERIES"
> -rm -rf $tmp_dir || exit 5
> +rm -rf "$tmp_dir" || exit 5

Ditto for cleanup. 

> base-commit: 795c338de725e13bd361214c6b768019fc45a2c1
> --
> 2.51.0

Thanks