Re: [PATCH 2/4] request-pull: stop depending on Perl

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2025, 16:16 UTC
Message-ID: <xmqqtt6pmmd6.fsf@gitster.g>
In-Reply-To: <20250415-b4-pks-drop-perl-v1-2-c6addf175858@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 32 quoted lines

> While git-request-pull(1) is written as a shell script, for it to
> function we depend on Perl being available. The script gets installed
> unconditionally though, regardless of whether or not Perl is even
> available on the system. When it's not available, the `@PERL_PATH@`
> variable may be substituted with a nonexistent executable path and thus
> cause the script to fail.
>
> Refactor the script so that it does not depend on Perl at all anymore.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  git-request-pull.sh     | 74 ++++++++++++++++++++++++++-----------------------
>  t/t5150-request-pull.sh |  6 ----
>  2 files changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/git-request-pull.sh b/git-request-pull.sh
> index 775ba8ea11a..59276fe265b 100755
> --- a/git-request-pull.sh
> +++ b/git-request-pull.sh
> @@ -78,41 +78,47 @@ fi
>  merge_base=$(git merge-base $baserev $headrev) ||
>  die "fatal: No commits in common between $base and $head"
>  
> -# $head is the refname from the command line.
> -# Find a ref with the same name as $head that exists at the remote
> +find_matching_ref () {
> +	while read sha1 ref
> +	do
> +		case "$ref" in
> +		*"^{}")
> +			ref="${ref%"^{}"}"
> +			deref=true

This got a bit tighter (the original allowed ls-remote output to be later extended to throw "^something-else" at the end of line), which I do not know is something we need to worry about. I think retaining the original semantics is easy in this case, e.g.,

		*"^"?*)
			ref="${ref%"^*"}"

or something, if we wanted to. As this is meant to be faithful rewrite to lose Perl, not a bugfix to correct overly loose pattern matching in the original, I think we want to in this case.

Show 38 quoted lines

> +			;;
> +		*)
> +			deref=
> +			;;
> +		esac
> +
> +		if test "$sha1" = "${remote:-HEAD}"
> +		then
> +			echo "$sha1 $sha1"
> +			break
> +		fi
> +
> +		case "$ref" in
> +		"${remote:-HEAD}"|*"/${remote:-HEAD}")
> +			if test -z "$deref"
> +			then
> +				# Remember the matching unpeeled object on the
> +				# remote side.
> +				remote_sha1="$sha1"
> +			fi
> +
> +			if test "$sha1" = "$headrev"
> +			then
> +				echo "${remote_sha1:-$headrev} $ref"
> +				break
> +			fi
> +			;;
> +		esac
> +	done
> +}
> +
> +# Find a ref with the same name as $remote that exists at the remote
>  # and points to the same commit as the local object.
> ...
> -set fnord $(git ls-remote "$url" | @PERL_PATH@ -e "$find_matching_ref" "${remote:-HEAD}" "$headrev")
> +set fnord $(git ls-remote "$url" | find_matching_ref)
>  remote_sha1=$2
>  ref=$3

This is faithful to the original, and having find_matching_ref in the process substitution means we do not have to worry about localizing variables in there. Good.