Re: [PATCH 2/2] Trim trailing directory separators from new sysroots.

[John Baldwin <jhb@FreeBSD.org>]

On 1/26/19 11:10 AM, Simon Marchi wrote:
> On 2019-01-22 8:03 p.m., John Baldwin wrote:
>> The separate debugfile logic assumes that the sysroot does not include
>> a trailing separator (all of the sysroot logic in
>> find_separate_debug_file requires the next character after the sysroot
>> in a object file path to be a directory separator).  However, normal
>> filename completion will result in setting a sysroot with a trailing
>> directory separator.  If the directory portion of a new sysroot ends
>> with a directory separator and is not specifying the root directory,
>> trim the trailing separator.  This permits the sysroot logic to work
>> the same if a sysroot of "/myroot/" is specified instead of "/myroot".
>>
>> Note that the sysroot displayed by 'show sysroot' will reflect the
>> trimmed sysroot.
> 
> I am not sure about this, it seems fragile to do this one off thing.  The
> point where the path is stripped is quite far from the point where it has
> an impact, so it's really not obvious why we do it (though this could be
> fixed by saying why we do it in the comment).
> 
> I think it would be a step in a better direction to have an "is_child_path"
> function that returns whether a path is a child of another one.  It would
> be responsible for handling the case of the parent potentially having a
> trailing directory separator.  The implementation of such function might
> get hairy, but the  advantage is that it should be pretty easy to unit
> test.
> 
> Finally, I think it would make this code more obvious:
> 
>       if (canon_dir != NULL
> 	  && filename_ncmp (canon_dir, gdb_sysroot,
> 			    strlen (gdb_sysroot)) == 0
> 	  && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))
> 
> vs
> 
>       if (canon_dir != NULL && is_child_patn (gdb_sysroot, canon_dir))

I have taken this approach, but it ended up being a bit more than just a
boolean check as I needed to reliably get the base path of the child
component as the existing 'canon_dir + strlen (gdb_sysroot)' assumed no
trailing separator in gdb_sysroot.  I'll post the updated version along
with another fix I encountered in some more testing today as a v2 in a
sec.

-- 
John Baldwin