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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2012, 16:42 UTC
Message-ID: <7v8vbd7yem.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CACsJy8CTGpDW46-37oX14+JxLxWmoz2f4mawr4QPkqCSunF7dQ@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Show 16 quoted lines

> On Thu, Oct 11, 2012 at 4:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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?
>> ...
>> 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)?
>
> I prefer the KISS principle in this case: do not interpret existing string.

Sorry, I do not quite understand what you mean.

The original function is about turning one path into a path that is relative to the other path (e.g. make the result usable for chdir() when your cwd() is the other path to name that path), and because you do not want to end up with an empty path, it adds "./" to the result when the two paths are equal.

That is simple and stupid, in the sense that you can explain in simple terms what it does even to a stupid person and make him understand.

If a patch changes one aspect of the implementation of the function, but keeps other parts intact without thinking the ramifications of doing so through, and ends up giving the end result unnecessarily complex semantics, the _patch_ might be simple and stupid, but the end result is no longer simple.

That is why I asked what it means to "append" and asked you to write it down for people who may need to decide if this function is an appropriate thing to call for their new code in the future.

You didn't answer that question, so I have to ask again. What is the existing string this function appends its result to?

 - If it is a leading path (in other words, you are transplanting a
   hierarchy to somewhere else --- see and (re)read "But if the
   caller did this instead" part from the message you are responding
   to), "because you do not want to end up with an empty path, it
   adds "./" to the result when the two paths are equal" does not
   make sense at all.

 - If it is a message that describes the resulting relative path
   (the use case above that "transplant" example in the same
   message), it needs to add "./" to the result, but the logic to
   determine it now needs to take the length of what was in the
   buffer when the function was called into account.

 - Is there a third possibility?

Whatever your answer is, please make sure the next person who wants to call this function from his code does not have to ask "Why does it append "./" when out->len is zero? Shouldn't the condition be when *rel is an empty string?"

Thanks.