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

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

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

So, for the existing user of this logic, the preimage ...

> -static void print_path(const char *path, const char *prefix, enum format_type format, enum default_type def)
>  {
> -	char *cwd = NULL;
> -	/*
> -	 * We don't ever produce a relative path if prefix is NULL, so set the
> -	 * prefix to the current directory so that we can produce a relative
> -	 * path whenever possible.  If we're using RELATIVE_IF_SHARED mode, then
> -	 * we want an absolute path unless the two share a common prefix, so don't
> -	 * set it in that case, since doing so causes a relative path to always
> -	 * be produced if possible.
> -	 */
> -	if (!prefix && (format != FORMAT_DEFAULT || def != DEFAULT_RELATIVE_IF_SHARED))
> -		prefix = cwd = xgetcwd();
> -	if (format == FORMAT_DEFAULT && def == DEFAULT_UNMODIFIED) {
> -		puts(path);
> -	} else if (format == FORMAT_RELATIVE ||
> -		  (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE)) {
> -		/*
> -		 * In order for relative_path to work as expected, we need to
> -		 * make sure that both paths are absolute paths.  If we don't,
> -		 * we can end up with an unexpected absolute path that the user
> -		 * didn't want.
> -		 */
> -		struct strbuf buf = STRBUF_INIT, realbuf = STRBUF_INIT, prefixbuf = STRBUF_INIT;
> -		if (!is_absolute_path(path)) {
> -			strbuf_realpath_forgiving(&realbuf, path,  1);
> -			path = realbuf.buf;
> -		}
> -		if (!is_absolute_path(prefix)) {
> -			strbuf_realpath_forgiving(&prefixbuf, prefix, 1);
> -			prefix = prefixbuf.buf;
>  		}
> -		puts(relative_path(path, prefix, &buf));
> -		strbuf_release(&buf);
> -		strbuf_release(&realbuf);
> -		strbuf_release(&prefixbuf);
> -	} else if (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE_IF_SHARED) {
> -		struct strbuf buf = STRBUF_INIT;
> -		puts(relative_path(path, prefix, &buf));
> -		strbuf_release(&buf);
> -	} else {
> -		struct strbuf buf = STRBUF_INIT;
> -		strbuf_realpath_forgiving(&buf, path, 1);
> -		puts(buf.buf);
> -		strbuf_release(&buf);
>  	}
> -	free(cwd);
>  }

... now becomes this postimage.

> +static void print_path(const char *path, const char *prefix,
> +		       enum format_type format, enum default_type def)
>  {
> +	struct strbuf sb = STRBUF_INIT;
> +	enum path_format fmt;
> +
> +	if (format == FORMAT_RELATIVE) {
> +		fmt = PATH_FORMAT_RELATIVE;
> +	} else if (format == FORMAT_CANONICAL) {
> +		fmt = PATH_FORMAT_CANONICAL;
> +	} else /* FORMAT_DEFAULT */ {
> +		switch (def) {
> +		case DEFAULT_RELATIVE:
> +			fmt = PATH_FORMAT_RELATIVE;
> +			break;
> +		case DEFAULT_RELATIVE_IF_SHARED:
> +			fmt = PATH_FORMAT_RELATIVE_IF_SHARED;
> +			break;
> +		case DEFAULT_CANONICAL:
> +			fmt = PATH_FORMAT_CANONICAL;
> +			break;
> +		case DEFAULT_UNMODIFIED:
> +		default:
> +			fmt = PATH_FORMAT_UNMODIFIED;
> +			break;
>  		}
>  	}
> +
> +	append_formatted_path(&sb, path, prefix, fmt);
> +	puts(sb.buf);
> +
> +	strbuf_release(&sb);
>  }

Mostly, the code translates FORMAT_FOO constants into the new
PATH_FORMAT_FOO constants, and lets append_formatted_path() do the
heavy lifting.

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.

> diff --git a/path.c b/path.c
> index d7e17bf174..6d8e892ada 100644
> --- a/path.c
> +++ b/path.c
> @@ -1579,6 +1579,75 @@ char *xdg_cache_home(const char *filename)
>  	return NULL;
>  }
>  
> +void append_formatted_path(struct strbuf *dest, const char *path,
> +			   const char *prefix, enum path_format format)
> +{
> +	switch (format) {
> +	case PATH_FORMAT_UNMODIFIED:
> +		strbuf_addstr(dest, path);
> +		break;

In the orignal "print_path()", DEFAULT/UNMODIFIED did this "show
unmodified".  OK.

> +	case PATH_FORMAT_RELATIVE: {
> +		struct strbuf relative_buf = STRBUF_INIT;
> +		struct strbuf real_path = STRBUF_INIT;
> +		struct strbuf real_prefix = STRBUF_INIT;
> +		char *cwd = NULL;
> +
> +		/*
> +		 * We don't ever produce a relative path if prefix is NULL,
> +		 * so set the prefix to the current directory so that we can
> +		 * produce a relative path whenever possible.
> +		 */
> +		if (!prefix)
> +			prefix = cwd = xgetcwd();

This is what was done in the original "print_path()" upfront, with
a similar comment to explay why this happens.  Looking good.  Also
we no longer call xgetcwd() when we do not need to, which is goodd.

> +		if (!is_absolute_path(path)) {
> +			strbuf_realpath_forgiving(&real_path, path, 1);
> +			path = real_path.buf;
> +		}
> +		if (!is_absolute_path(prefix)) {
> +			strbuf_realpath_forgiving(&real_prefix, prefix, 1);
> +			prefix = real_prefix.buf;
> +		}

There used to be a comment explaining why we make realpath calls,
which is now lost.  Perhaps what the comment said was so obvious
that we are better off without it?  I offhand do not know.

What is done to make the paths real is the same as before, which is
good.

> +		strbuf_addstr(dest, relative_path(path, prefix, &relative_buf));
> +
> +		strbuf_release(&relative_buf);
> +		strbuf_release(&real_path);
> +		strbuf_release(&real_prefix);
> +		free(cwd);
> +		break;
> +	}

OK.

> +	case PATH_FORMAT_RELATIVE_IF_SHARED: {
> +		struct strbuf relative_buf = STRBUF_INIT;
> +
> +		/*
> +		 * If we're using RELATIVE_IF_SHARED mode, then we want an
> +		 * absolute path unless the two share a common prefix, so don't
> +		 * default the prefix to the current working directory. Doing so
> +		 * would cause a relative path to always be produced if possible.
> +		 */
> +		strbuf_addstr(dest, relative_path(path, prefix, &relative_buf));
> +		strbuf_release(&relative_buf);
> +		break;
> +	}

Identical to the original, which is good.

> +
> +	case PATH_FORMAT_CANONICAL: {
> +		struct strbuf canonical_buf = STRBUF_INIT;
> +
> +		strbuf_realpath_forgiving(&canonical_buf, path, 1);
> +		strbuf_addbuf(dest, &canonical_buf);
> +
> +		strbuf_release(&canonical_buf);
> +		break;
> +	}
> +
> +	default:
> +		BUG("unknown path_format value %d", format);
> +	}
> +}

OK.

> +/**
> + * The formatting strategy to apply when writing a path into a buffer.
> + */
> +enum path_format {
> +	/* Output the path exactly as-is without any modifications. */
> +	PATH_FORMAT_UNMODIFIED,
> +
> +	/* Output a path relative to the provided directory prefix. */
> +	PATH_FORMAT_RELATIVE,
> +
> +	/* Output a relative path only if the path shares a root with the prefix. */
> +	PATH_FORMAT_RELATIVE_IF_SHARED,
> +
> +	/* Output a fully resolved, absolute canonical path. */
> +	PATH_FORMAT_CANONICAL
> +};
> +
> +/**
> + * 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).

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

Thanks.