Re: Pull request for msysGit patches

From

Junio C Hamano <gitster@pobox.com>

Date

Sep 28, 2010, 19:10 UTC

Message-ID

<7vocbhsn03.fsf@alter.siamese.dyndns.org>

In-Reply-To

<87ocbitd33.fsf@fox.patthoyts.tk>

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. 
> Please pull from
>   git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio
> also visible for inspection at
>   http://repo.or.cz/w/git/mingw/4msysgit.git/shortlog/refs/heads/work/pt/for-junio

A quick and superficial review follows.

@@ -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 ;-).

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

Please have () around "len > 0 && is_dir_sep(cwd[len-1])" for readability.

+		if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
 			die("Too long path: %.*s", 60, path);

----------------------------------------------------------------
get_home_directory()

The patch looks fine, but the commit log message is way insufficient.  Can
you restate what problem it addresses, and why it is the best solution?

----------------------------------------------------------------
Hide dotfiles

It is somewhat unfortunate that this Windows-only hack needs to touch
cache.h and environment.c.

hidedotfiles may currently be the only platform specific configuration
variable, but we might want to futureproof by abstracting this out,
perhaps by adding a call to platform_core_config() at the end of
git_default_core_config(), provide a default implementation that is a
no-op, and allow platforms to override it, or something like that?

----------------------------------------------------------------
pack-objects.c usage string
connect.c use of unchecked git_getpass()

Good eyes, but these should not be part of the series but applied to
maint.  If possible please send them separately.

----------------------------------------------------------------
compat/regex/regexec.c

Thanks for fixing up my mess ;-)

----------------------------------------------------------------
git-am.sh

This is questionable.  Does this mean that on POSIX boxes I cannot have my
patch mailbox named 0:pt.patch, 1:pt.patch, etc.?

Adding "is_absolute_path" helper that has a different implementation
depending on the platform in git-sh-setup and using it here would limit
the extent of damage?

----------------------------------------------------------------
git-merge-octopus.sh

Transliterating a-z to A-z may happen to work because A-z is A-Z with a
lot of garbage concatenated at the end that won't be used, but it feels
sloppy.

Why isn't upcasing necessary for all the other uses of environment
variables?  For example, we pass reflog action by exporting a variable,
and we use GIT_AUTHOR_NAME and friends to override configuration
variables.  Do they get upcased?

What I am getting at is that this might be just fixing a symptom, and
between applying similar band-aid to many other places and finding out why
undesired upcaing happens and fixing that, I'd rather see the latter done.

----------------------------------------------------------------
git-send-email.perl

Similar comment as is_absolute_path(), although in Perl environment I
suspect we can just use an existing package without adding our own.

----------------------------------------------------------------
t/test-lib.sh

Is "DIFF=${DIFF:-diff}" needed?  How is the existing initialization (in
'maint') of GIT_TEST_CMP working?

----------------------------------------------------------------

I didn't look at tests very deeply.  Other changes outside compat/ that I
didn't mention above looked fine.

Thanks.