Re: [msysGit] Re: Pull request for msysGit patches

[Eric Sunshine <sunshine@sunshineco.com>]

Hi Junio,

On 9/28/2010 3:10 PM, Junio C Hamano wrote:
> Pat Thoyts<patthoyts@users.sourceforge.net>  writes:
>> Junio,
>> The msysGit tree currently tracks some 50+ patches on top of 'next'. I
>> have gathered 42 of these that look good to move upstream.
>
> A quick and superficial review follows.
> ----------------------------------------------------------------
> abspath.c
>
> @@ -108,10 +108,15 @@ const char *make_nonrelative_path(const char *path)
>   		if (strlcpy(buf, path, PATH_MAX)>= PATH_MAX)
>   			die("Too long path: %.*s", 60, path);
>   	} else {
> +		size_t len;
> +		const char *fmt;
>   		const char *cwd = get_pwd_cwd();
>   		if (!cwd)
>   			die_errno("Cannot determine the current working directory");
> -		if (snprintf(buf, PATH_MAX, "%s/%s", cwd, path)>= PATH_MAX)
> +		len = strlen(cwd);
> +		/* For cwd c:/, return c:/foo rather than URL-like c://foo */
>
> For the patch to be regression free, the logic described by this comment
> requires get_pwd_cmd() to return a string with trailing dir-sep only at
> slash.  IOW, if you see any non-root path returned with a trailing dir-sep
> for whatever reason, you are changing the behaviour in that case as well,
> and that clearly is not "fix at the root level".
> But if you label this as "avoid duplicated dir-sep", everything flows
> smoothly ;-).

I am the author of this patch. Do I understand correctly that your 
primary concern is that you find the comment misleading? I consider the code

   fmt = (len > 0 && is_dir_sep(cwd[len-1])) ? "%s%s" : "%s/%s";

sufficiently self-documenting that the comment is superfluous, and would 
be happy to remove the comment. Would you prefer the patch submitted 
with the comment removed?

-- ES