Re: [PATCH 4/4] repo: add the field path.toplevel

[Tian Yuchen <a3205153416@gmail.com>]

Hi Lucas

> I don't think it can be considered a low-level function, but I
> agree that its name can be misleading.

Hummm...If a function is solely responsible for string concatenation and 
resides in  like path.c, why isn't it a low level function?

I think the key issue lies in the fact that this function's 
responsibilities are not quite appropriate, rather than merely the 
name. Does a string buffer need to understand Git's path formatting 
rules? It should only know how to append bytes, right? Maybe it would be 
better suited as a domain-specific formatter like 'format_path_output()' 
in a higher level module? I am quite uncertain about it.


> In this case, no, it is defined in wrapper.h.

Yes it is defined in wrapper.h. However in wrapper.c we have:

char *xgetcwd(void)
{
	struct strbuf sb = STRBUF_INIT;
	if (strbuf_getcwd(&sb))
		die_errno(_("unable to get current working directory"));
	return strbuf_detach(&sb, NULL);
}

and the for the stfbuf_getcwd(), in strbuf.c we have:

int strbuf_getcwd(struct strbuf *sb)
{
	size_t oldalloc = sb->alloc;
	size_t guessed_len = 128;

	for (;; guessed_len *= 2) {
		strbuf_grow(sb, guessed_len);
		if (getcwd(sb->buf, sb->alloc)) {
			strbuf_setlen(sb, strlen(sb->buf));
			return 0;
...

Notice the getcwd() function, which is indeed a system call, which you 
can check with 'man 2 getcwd' in terminal. Wrapping it in wrapper.c is 
just providing a shortcut, right?

But I don't think using system calls is inherently problematic. The 
issue lies in where this xgetbuf() is placed:

In builtin/rev-parse.c, the print_path() function is inside of 
cmd_rev_parse(), which is like:

int cmd_rev_parse(....){
	for (i = 1; i < argc; i++){
	...
	if (....){
		print_path(....)
	}
	...
}

And your print_path() implement was:

> +static void print_path(const char *path, const char *prefix,
> +		       enum path_format_type format, enum path_default_type def)
>  {
> +	struct strbuf sb = STRBUF_INIT;
> +	strbuf_add_path(&sb, path, prefix, format, def);
> +	puts(sb.buf);
> +	strbuf_release(&sb);
>  }

So this system call is indeed invoked in the loop. Specifically, it gets 
called every time 'git rev-parse' is invoked, and as far as I know it 
should be a command used extensively in like shell scripts...?

Maybe cache-up approach is more robust? For example in builtin/rev-parse.c:

const char *cached_cwd = ...->original_cwd;
if (!cached_cwd)
	cached_cwd = xgetcwd();

for (...) {
	if (...) {
		print_path_with_cwd(..., cached_cwd, ...);
	}
}


> In this case, we need to add them to match the signature of
> get_value_fn. Those values will be useful for all the path.*, but
> if we start to add more than that I agree that we'll need to think
> in a better solution.

Yes indeed.

> Thanks, it's also good to see more points of view. I'm also not
> sure about it :-)

Thank you for the patch again!

Regards,

Yuchen