Re: [PATCH 7/7] worktree: simplify prefixing paths

From: Duy Nguyen <pclouds@gmail.com>
Date: May 11, 2016, 00:46 UTC
Message-ID: <CACsJy8CiUBryRqbBHGYcMR7VSBtys3SD10T79_Dq9H6JQ=NiFw@mail.gmail.com>
In-Reply-To: <xmqqvb2lldgg.fsf@gitster.mtv.corp.google.com>

On Wed, May 11, 2016 at 6:07 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>
> This changes semantics, doesn't it?  prefix_filename() seems to do a
> lot more than just strbuf_vadd("%s%s", prefix, filename); would do.
>
> It may be a good change (e.g. turn '\' into '/' on Windows), but
> this is way more than "simplify prefixing".  It is something else
> whose effect needs to be explained.

On Wed, May 11, 2016 at 6:07 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>
> This changes semantics, doesn't it?  prefix_filename() seems to do a
> lot more than just strbuf_vadd("%s%s", prefix, filename); would do.
>
> It may be a good change (e.g. turn '\' into '/' on Windows), but
> this is way more than "simplify prefixing".  It is something else
> whose effect needs to be explained.

I admit I forgot about Windows part in prefix_filename(). For non-Windows code, it's exactly the same behavior. Maybe we should do this to emphasize that prefix_filename() is no-op when pfx_len is zero? The same pattern is used elsewhere too (or I'm spreading it in my local tree..) Unless of course if convert_slashes() is a good thing to always do, then I need to update my commit message (+Johannes for this question).

diff --git a/abspath.c b/abspath.c
index 2825de8..bf454e0 100644
--- a/abspath.c
+++ b/abspath.c
@@ -160,8 +160,11 @@ const char *absolute_path(const char *path)
 const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 {
  static struct strbuf path = STRBUF_INIT;
+
+ if (!pfx_len)
+ return arg;
 #ifndef GIT_WINDOWS_NATIVE
- if (!pfx_len || is_absolute_path(arg))
+ if (is_absolute_path(arg))
  return arg;
  strbuf_reset(&path);
  strbuf_add(&path, pfx, pfx_len);

>
>>  builtin/worktree.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> index b53f802..f9dac37 100644
>> --- a/builtin/worktree.c
>> +++ b/builtin/worktree.c
>> @@ -337,7 +337,7 @@ static int add(int ac, const char **av, const char *prefix)
>>       if (ac < 1 || ac > 2)
>>               usage_with_options(worktree_usage, options);
>>
>> -     path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
>> +     path = prefix_filename(prefix, strlen(prefix), av[0]);
>>       branch = ac < 2 ? "HEAD" : av[1];
>>
>>       opts.force_new_branch = !!new_branch_force;
>> @@ -467,6 +467,8 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
>>
>>       if (ac < 2)
>>               usage_with_options(worktree_usage, options);
>> +     if (!prefix)
>> +             prefix = "";
>>       if (!strcmp(av[1], "add"))
>>               return add(ac - 1, av + 1, prefix);
>>       if (!strcmp(av[1], "prune"))

-- 
Duy