On 12/2/2022 1:32 PM, Ævar Arnfjörð Bjarmason wrote:
>> +size_t strbuf_strip_file_from_path(struct strbuf *buf)
>
> Nit: Almost every function in this API calls its argument "sb", let's do
> that for new functions.
Sure.
>> +{
>> + size_t len = buf->len;
>> + size_t offset = offset_1st_component(buf->buf);
>
> Mm, isn't the return value of offset_1st_component() a boolean? it's
> just an "is_dir_sep(buf->buf[0])".
>
> So this works to....
>
>> + char *path_sep = find_last_dir_sep(buf->buf + offset);
>
> ...find the last dir separator starting at either 0 or 1.
>
> But anyway, what sort of string is this expecting to handle where the
> last dir separator isn't >=1 offset into the string anyway? Shouldn't we
> just exclude the string "/" here? Maybe I'm missing something....
The difference is all about whether or not we start with a slash _and_ no
other slash appears in the path.
1. "/root-file": offset becomes 1 and path_sep becomes NULL, so the
length is reduced to offset (1), resulting in "/"
2. "local-file": offset becomes 0 and path_sep becomes NULL, so the
length is reduced to offset (0), resulting in ""
Of course, the first case would be the same if we did not set offset to 1
and then path_sep points to that base slash, so the length calculation
results in "/" anyway. That will simplify things.
void strbuf_strip_file_from_path(struct strbuf *sb)
{
char *path_sep = find_last_dir_sep(sb->buf);
strbuf_setlen(sb, path_sep ? path_sep - sb->buf + 1 : 0);
}
>> + strbuf_setlen(buf, path_sep ? path_sep - buf->buf + 1 : offset);
>> + return len - buf->len;
>> +}
>
> Urm, so isn't this literally one-byte away from being equivalent to a
> function that's already in the API?:
> strbuf_trim_trailing_dir_sep. I.e. this seems to me to do the same as
> this new function.
>
> Context manually adjusted so we can see the only difference is the
> "is_dir_sep" v.s. "!is_dir_sep".
>
> There's a few strbuf functions like that, and we should probably
> generalize the ctype-like test they share into some callback mechanism,
> but in the meantime keeping with the pattern & naming of existing
> functions seems better.
Using callbacks or extra options seems like overkill here.
> I removed the comment because if it's the same then the new function is
> self-documenting. It doesn't matter if the URI ends in a "/" or not, all
> we need to get across is that we're stripping non-dirsep characters from
> the URL, whether it ends in one or not.
>
> In terms of correctness: The use of is_dir_sep() seems incorrect to me
> here. On Windows won't that end up using is_xplatform_dir_sep(), so
> bundle-uri's behavior will differ there, and we'll support \\-paths as
> well as /-paths, but elsewhere only /-paths.
>
> Shouldn't this just test "/", not "is_dir_sep()"?
This method is used for file paths, too, so is_dir_sep() is important to
work properly on Windows platforms.
Thanks,
-Stolee
