Re: [PATCH 1/5] Git.pm Add unquote_path()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 22, 2017, 19:26 UTC
- Message-ID
- <xmqqlgojken5.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170622102622.26147-2-phillip.wood@talktalk.net>
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 ...").
Show 5 quoted lines
> 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).
Show 32 quoted lines
> 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.
Show 8 quoted lines
> + "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.
Show 8 quoted lines
> + );
> +
> + 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.