Re: [GSoC Patch v7 1/3] path: extract append_formatted_path() and use in rev-parse

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
> ...
> It is a minor point, but wouldn't it make it simpler to handle
> format_default first?  I.e.,
>
> 	if (format == FORMAT_DEFAULT)
> 		switch (def) {
> 		case DEFAULT_RELATIVE:
> 			format = DEFAULT_RELATIVE;
> 			break;
> 		...
> 		case DEFAULT_UNMODIFIED:
> 		default:
> 			format = DEFAULT_UNMODIFIED; 
> 			break;
> 	}
> 	switch (format) {
>         case FORMAT_RELATIVE: fmt = PATH_FORMAT_RELATIVE; break;
> 	case FORMAT_CANONICAL: fmt = PATH_FORMAT_CANONICAL; break;
> 	...
> 	}
>
> Perhaps yes, perhaps not.  I dunno.

I do not consider the above an blocker, but it might make a
difference if we are going to acquire more modes and formats, so
once somebody tries to rewrite the logic and finds the resulting
code harder to follow (or not easier to follow), I would be happy to
see the above discarded ;-)

>> +/**
>> + * Format a path according to the specified formatting strategy and append
>> + * the result to the given strbuf.
>> + *
>> + * `dest`   : The string buffer to append the formatted path to.
>> + * `path`   : The path string that needs to be formatted.
>> + * `prefix` : The directory prefix to calculate relative offsets against.
>> + * Pass NULL to default to the current working directory where applicable.
>> + * `format` : The formatting behavior rule to execute.
>> + */
>> +void append_formatted_path(struct strbuf *dest, const char *path,
>> +			   const char *prefix, enum path_format format);
>> +
>
> It is slightly unsatisfying that this function is defined to
> "append" to any existing value in the dest strbuf, rather than
> storing the result in the dest strbuf.  The original caller
> print_path() passes an empty strbuf to this helper, so it can let
> strbuf_realpath_*() functions to strbuf_reset() it (e.g.,
> abspath.c:get_root_part() called by strbuf_realpath_1(), wihch in
> turn is called by strbuf_realpath() and strbuf_realpath_forgiving())
> it freely, which means that use of temporary strbuf like
> canonical_buf only to copy it out to dest is wasteful and unneeded.
> But other callers we will have for this helper later may want to
> append to what they already have, so perhaps it is OK (on the other
> hand, we could say that preserving and appending is what these
> callers can do themselves).

This one we may want to consider a bit more seriously, but it is
entirely up to the future callers of the helper.  If it would make
the callers much easier to write for this helper to have "append"
semantics, I'd be happy to accept the semantics of the above as-is,
but otherwise, I suspect it would be simpler to use if the helper is
defined to replase dest with the result, instead of appending the
result to dest.

> Otherwise, looking good as a no-op bug-to-bug compatible rewrite,
> with a slight optimization (to skip xgetcwd()).

This part of the review does not change in any case.  The
refactoring looks good.

Thanks.