Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts

[Junio C Hamano]

Brandon Williams  writes:

> On 12/22, Johannes Sixt wrote:
>> Am 21.12.2016 um 23:33 schrieb Brandon Williams:
>> >On 12/21, Johannes Sixt wrote:
>> >>+/* copies root part from remaining to resolved, canonicalizing it on the 
>> >>way */
>> >>+static void get_root_part(struct strbuf *resolved, struct strbuf 
>> >>*remaining)
>> >>+{
>> >>+  int offset = offset_1st_component(remaining->buf);
>> >>+
>> >>+  strbuf_reset(resolved);
>> >>+  strbuf_add(resolved, remaining->buf, offset);
>> >>+#ifdef GIT_WINDOWS_NATIVE
>> >>+  convert_slashes(resolved->buf);
>> >>+#endif
>> >
>> >So then the only extra cononicalization that is happening here is
>> >converting '\\server\share' to '//server/share'? (or 'c:\' to 'c:/')
>> 
>> Correct. All other directory separators are canonicalized by the
>> primary function, strbuf_realpath.
>
> Sounds good. Logically everything looks good to me.  And I like that
> setting 'resolved' to the root of an abs path is pulled out into a
> helper function.  It took me a couple extra seconds to realize that
> offset_1st_component returns 0 with a relative path, which makes causes
> the call to get_root_part to essentially be a noop (ie nothing is
> resolved).
>
> Thanks for helping get this to work on windows!

Thanks, both.  

Let's move the topic with this patch to 'next'.  Further
micro-optimization can be done incrementally if desired.

Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts

[Johannes Sixt]

Am 22.12.2016 um 18:33 schrieb Brandon Williams:

It took me a couple extra seconds to realize that
offset_1st_component returns 0 with a relative path, which makes causes
the call to get_root_part to essentially be a noop (ie nothing is
resolved).


Yeah, I am still unsure whether it is a good idea to optimize away the 
is_absolute_path() call, because we lose the symmetry to the symlink 
case, where we cannot get rid of the call...


But I think the condition plus comment

   if (!resolved->len) {
   /* relative path; can use CWD as the initial resolved path */

makes things fairly clear.

-- Hannes

Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts

[Brandon Williams]

On 12/22, Johannes Sixt wrote:
> Am 21.12.2016 um 23:33 schrieb Brandon Williams:
> >On 12/21, Johannes Sixt wrote:
> >>+/* copies root part from remaining to resolved, canonicalizing it on the 
> >>way */
> >>+static void get_root_part(struct strbuf *resolved, struct strbuf 
> >>*remaining)
> >>+{
> >>+   int offset = offset_1st_component(remaining->buf);
> >>+
> >>+   strbuf_reset(resolved);
> >>+   strbuf_add(resolved, remaining->buf, offset);
> >>+#ifdef GIT_WINDOWS_NATIVE
> >>+   convert_slashes(resolved->buf);
> >>+#endif
> >
> >So then the only extra cononicalization that is happening here is
> >converting '\\server\share' to '//server/share'? (or 'c:\' to 'c:/')
> 
> Correct. All other directory separators are canonicalized by the
> primary function, strbuf_realpath.

Sounds good. Logically everything looks good to me.  And I like that
setting 'resolved' to the root of an abs path is pulled out into a
helper function.  It took me a couple extra seconds to realize that
offset_1st_component returns 0 with a relative path, which makes causes
the call to get_root_part to essentially be a noop (ie nothing is
resolved).

Thanks for helping get this to work on windows!

-- 
Brandon Williams

Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts

[Johannes Sixt]

Am 21.12.2016 um 23:33 schrieb Brandon Williams:

On 12/21, Johannes Sixt wrote:

+/* copies root part from remaining to resolved, canonicalizing it on the way */
+static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
+{
+   int offset = offset_1st_component(remaining->buf);
+
+   strbuf_reset(resolved);
+   strbuf_add(resolved, remaining->buf, offset);
+#ifdef GIT_WINDOWS_NATIVE
+   convert_slashes(resolved->buf);
+#endif


So then the only extra cononicalization that is happening here is
converting '\\server\share' to '//server/share'? (or 'c:\' to 'c:/')


Correct. All other directory separators are canonicalized by the primary 
function, strbuf_realpath.


-- Hannes

Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts

[Brandon Williams]

On 12/21, Johannes Sixt wrote:
> When an absolute path is resolved, resolution begins at the first path
> component after the root part. The root part is just copied verbatim,
> because it must not be inspected for symbolic links. For POSIX paths,
> this is just the initial slash, but on Windows, the root part has the
> forms c:\ or \\server\share. We do want to canonicalize the back-slashes
> in the root part because these parts are compared to the result of
> getcwd(), which does return a fully canonicalized path.
> 
> Factor out a helper that splits off the root part, and have it
> canonicalize the copied part.
> 
> This change was prompted because t1504-ceiling-dirs.sh caught a breakage
> in GIT_CEILING_DIRECTORIES handling on Windows.
> 
> Signed-off-by: Johannes Sixt 
> ---
>  This introduces the second #ifdef GIT_WINDOWS_NATIVE in this file.
>  It could be avoided if convert_slashes were defined as a do-nothing
>  on POSIX, but that would not help the other occurrence. Therefore,
>  I suggest to leave it at this.
> 
>  abspath.c | 29 +
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/abspath.c b/abspath.c
> index 79ee310867..1d56f5ed9f 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -48,6 +48,19 @@ static void get_next_component(struct strbuf *next, struct 
> strbuf *remaining)
>   strbuf_remove(remaining, 0, end - remaining->buf);
>  }
>  
> +/* copies root part from remaining to resolved, canonicalizing it on the way 
> */
> +static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
> +{
> + int offset = offset_1st_component(remaining->buf);
> +
> + strbuf_reset(resolved);
> + strbuf_add(resolved, remaining->buf, offset);
> +#ifdef GIT_WINDOWS_NATIVE
> + convert_slashes(resolved->buf);
> +#endif

So then the only extra cononicalization that is happening here is
converting '\\server\share' to '//server/share'? (or 'c:\' to 'c:/')

-- 
Brandon Williams