Re: [PATCH] apply: squash consecutive slashes with p_value > 0

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2011, 17:36 UTC
Message-ID: <7v62ikq89h.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20111116120403.GA10342@mal.justgohome.co.uk>

Robie Basak <robie.basak@canonical.com> writes:

Show 7 quoted lines

> "patch" works with -p1 and diffs in the following form:
>     --- foo.orig//bar
>     +++ foo//bar
>     ...
>
> patch(1) says that "A sequence of one or more adjacent slashes is
> counted as a single slash."

It merely says patch(1) treats duplicate slashes that way; it does not mean it is a useful thing to do in the real life.

Could you justify why such a change is a good thing in your proposed commit log message?

>  builtin/apply.c              |    8 ++++++--
>  t/t4153-apply-doubleslash.sh |   29 +++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 2 deletions(-)
>  create mode 100755 t/t4153-apply-doubleslash.sh

Can we avoid wasting a new test number for just a single trivial test by findign appropriate places in existing tests to add new ones to? I think 4133 may be one of the good places (not just checkint between a/f vs b/f, you would check a//f vs b/f and somesuch).

Show 14 quoted lines

> diff --git a/builtin/apply.c b/builtin/apply.c
> index 84a8a0b..78e25fa 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -627,9 +627,13 @@ static char *find_name_common(const char *line, char *def, int p_value,
>  			if (name_terminate(start, line-start, c, terminate))
>  				break;
>  		}
> -		line++;
> -		if (c == '/' && !--p_value)
> +		if (c == '/' && !--p_value) {
> +			while (*line == '/')
> +			    line++;
>  			start = line;

I am not sure if this is sufficient or necessarily correct.

You are de-dupling slashes only when p_value hits 0. When working on an input "a///b//c" with -p3, your loop strips one slash per decrement of p_value between "a" and "b" and then you notice slashes after "b" are duplicated and clean them up, which would mean you normalized the input to "a///b/c", not "a/b/c", no?