Re: [PATCH v2 6/9] strbuf: introduce strbuf_strip_file_from_path()

From: Derrick Stolee <derrickstolee@github.com>
Date: Dec 5, 2022, 15:11 UTC
Message-ID: <0980dcd4-30eb-4ef4-9369-279abe5ca5a2@github.com>
In-Reply-To: <221202.86bkolfwzv.gmgdl@evledraar.gmail.com>

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.

Show 16 quoted lines

>> +{
>> +	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);
}

Show 16 quoted lines

>> +	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.

Show 11 quoted lines

> 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