Re: [PATCH 1/5] Git.pm Add unquote_path()

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood@talktalk.net> writes:

> Subject: Re: [PATCH 1/5] Git.pm Add unquote_path()

Subject: [PATCH 1/5] Git.pm: add unquote_path()

But I think it is more customary to remove the implementation in the
other file and adjust the existing callers to call this new one in
this same commit.  And then in follow-up commits, improve the
implementation in Git.pm.

When that happens, the subject would become

Subject: [PATCH 1/?] add-i: move unquote_path() to Git.pm

and the first sentence in the body would be tweaked ("move
unquote_path() from ... to Git.pm so that ...").

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Add unquote_path() from git-add--interactive so it can be used by
> other scripts. Note this is a straight copy, it does not handle
> '\a'. That will be fixed in the next commit

s/next commit/&./

Good to notice and document the lack of '\a' which does get used by
quote.c::sq_lookup[] when showing chr(7).

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  perl/Git.pm | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index bfce1f795dfa5fea05f4f96637a1ae2333038735..8afde87fc8162271ba178e0fff3e921f070ac621 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -61,7 +61,8 @@ require Exporter;
>                  remote_refs prompt
>                  get_tz_offset get_record
>                  credential credential_read credential_write
> -                temp_acquire temp_is_locked temp_release temp_reset temp_path);
> +                temp_acquire temp_is_locked temp_release temp_reset temp_path
> +                unquote_path);
>  
>  
>  =head1 DESCRIPTION
> @@ -1451,6 +1452,56 @@ sub prefix_lines {
>  	return $string;
>  }
>  
> +=item unquote_path ( PATH )
> +
> +Unquote a quoted path containing c-escapes as returned by ls-files etc.
> +when not using -z
> +
> +=cut
> +
> +{
> +	my %unquote_map = (

The original calls this cquote_map, partly because there may be
other kinds of quoting possible.  I do not see a reason not to
follow suit here.

> +		"b" => chr(8),
> +		"t" => chr(9),
> +		"n" => chr(10),
> +		"v" => chr(11),
> +		"f" => chr(12),
> +		"r" => chr(13),
> +		"\\" => "\\",
> +		"\"" => "\""

In an early part of "sub unquote_path" we see dq spelled as "\042"
and in the original cquote_map, this is also defined with "\042".  I
do not think you want to change the above to make them inconsistent.

> +	);
> +
> +	sub unquote_path {
> +		local ($_) = @_;
> +		my ($retval, $remainder);
> +		if (!/^\042(.*)\042$/) {
> +			return $_;
> +		}

Other than that, I can see this is just a straight-copy, which is
exactly what we want in an early step of a series like this.  Squash
in a change to remove the original from add-i and adjust the caller
to call this one (you may not have to do anything as it uses Git.pm
already, or perhaps "use Git qw(unquote_path)" or something?) and it
would be perfect ;-)

Thanks.