Re: [PATCH 1/3] quote: let caller reset buffer for quote_path_relative()

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2012, 21:13 UTC
Message-ID: <7vd30qav4m.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1349868894-3579-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 11 quoted lines

> quote_path_relative() resetting output buffer is sometimes unnecessary
> as the buffer has never been used, and some other times makes it
> harder for the caller to use (see builtin/grep.c, the caller has to
> insert a string after quote_path_relative)
>
> Move the buffer reset back to call sites when necessary.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  The answer for Jeff's XXX in his patch, why strbuf_insert() instead
>  of just adding in advance.

This sounds a lot stronger than "let" to me. All existing callers that assumed that buf to be emptied by the function now has to clear it. "quote: stop resetting output buffer of quote_path_relative()" may better describe what this really does.

How should this interact with the logic in the called function that used to say "if we ended up returning an empty string because the path is the same as the base, we should give ./ back", and what should the return value of this function be?

To answer these questions, you must define the meaning of the string in the output buffer that already exists when the function is called. If the caller did this:

	strbuf_addstr(&out, "The path relative to your HOME is: ");
        quote_path_relative(path, pathlen, &out, "/home/pclouds/");

then the answers are "We still need to add ./ but !out->len is no longer a good test to decide" and "It should point at the first byte of what we added, not out->buf".

But if the caller did this instead:

	srcdir = "/src/";
	strbuf_addstr(&dst, "/dst/");
        quote_path_relative(path, pathlen, &dst, srcdir);
        printf("cp '%s' '%s'\n", path, dst->buf);

then it is nonsensical to add "./" when out->len is not zero when the function returns.

So what does it mean to have an existing string in the output buffer when calling the function? Is it supposed to be a path to a directory, or just a general uninterpreted string (e.g. a message)?