From: Stefan Beller <sbeller@google.com>
Date: Wed, 23 Aug 2017 19:14:15 GMT
Subject: Re: [PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()
Message-ID: <CAGZ79kbvKZXGy80RCU3zkdm0S3M-tGB=LAJYiY=o+YwUB9RNHA@mail.gmail.com>
In-Reply-To: <20170823123704.16518-3-pclouds@gmail.com>

On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> The "submodule" argument in this function is a path, which can have
> either '/' or '\\' as a separator. Use is_dir_sep() to support both.
>
> Noticed-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Immediate questions that come to mind:
* Could this go in as an independent bug fix?
  -> If so do we have any test that fails or stops failing on Windows?
  -> If not, do we need one?
* Assuming this is not an independent bug fix:
  Why do we need this patch in this series here?
  (I thought this is about worktrees, not submodules.
  So does this fix a potential bug that will be exposed
  later in this series, but was harmless up to now?)

I'll read the next patches to see if I will be enlightened.

Thanks,
Stefan

> ---
>  refs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 3d549a8970..dec899a57a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1507,7 +1507,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
>         struct ref_store *refs;
>         int flags;
>
> -       while (len && submodule[len - 1] == '/')
> +       while (len && is_dir_sep(submodule[len - 1]))
>                 len--;
>
>         if (!len)
> --
> 2.11.0.157.gd943d85
>