On Tue, Dec 6, 2011 at 6:52 AM, Jeff King <peff@xxxxxxxx> wrote: > On Mon, Dec 05, 2011 at 09:01:52PM -0800, Junio C Hamano wrote: > >> * jk/credentials (2011-11-28) 20 commits >> - fixup! 034c066e >> - compat/getpass: add a /dev/tty implementation >> - credential: use git_prompt instead of git_getpass >> - prompt: add PROMPT_ECHO flag >> - stub out getpass_echo function >> - refactor git_getpass into generic prompt function >> - move git_getpass to its own source file >> - t: add test harness for external credential helpers >> - credentials: add "store" helper >> - strbuf: add strbuf_add*_urlencode >> - credentials: add "cache" helper >> - docs: end-user documentation for the credential subsystem >> - credential: make relevance of http path configurable >> - credential: add credential.*.username >> - credential: apply helper config >> - http: use credential API to get passwords >> - credential: add function for parsing url components >> - introduce credentials API >> - t5550: fix typo >> - test-lib: add test_config_global variant >> >> Expecting a reroll? > > Yes, I have a re-roll ready. I was holding back to see if some of the > helper authors might comment, but got nothing. Perhaps the new version > will spur some interest. > > Also, let's drop the top git_getpass bits from the topic for now (they > will not be part of my rebase). They are a separate topic that can go on > top, but I think there was some question from Erik of whether we should > simply roll our own getpass(). > >> * jk/upload-archive-use-start-command (2011-11-21) 1 commit >> - upload-archive: use start_command instead of fork >> >> What's the status of this one? > > I think what you have in pu is good, but of course I didn't actually > test it on Windows. Erik? > I tried to check out ee27ca4 and build, and got hit by a wall of warnings: compat/mingw.h: In function 'waitpid': compat/mingw.h:117: warning: pointer targets in passing argument 1 of '_cwait' differ in signedness d:\msysgit\mingw\bin\../lib/gcc/mingw32/4.4.0/../../../../include/process.h:60: note: expected 'int *' but argument is of type 'unsigned int *' This seems to be an issue that has been around since the birth of the windows port, and can be fixed by making our waitpid signature identical to what POSIX expects (http://pubs.opengroup.org/onlinepubs/7908799/xsh/waitpid.html): ---8<--- diff --git a/compat/mingw.h b/compat/mingw.h index a255898..2036013 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -111,7 +111,7 @@ static inline int mingw_unlink(const char *pathname) } #define unlink mingw_unlink -static inline int waitpid(pid_t pid, unsigned *status, unsigned options) +static inline int waitpid(pid_t pid, int *status, int options) { if (options == 0) return _cwait(status, pid, 0); ---8<--- But then, there's a couple of much more scary warnings: decorate.c: In function 'hash_obj': decorate.c:11: warning: dereferencing type-punned pointer will break strict-aliasing rules <snip> object.c: In function 'hash_obj': object.c:48: warning: dereferencing type-punned pointer will break strict-aliasing rules <snip> builtin-merge-recursive.c: In function 'cmd_merge_recursive': builtin-merge-recursive.c:49: warning: unknown conversion type character 'z' in format builtin-merge-recursive.c:49: warning: format '%s' expects type 'char *', but ar gument 2 has type 'unsigned int' builtin-merge-recursive.c:49: warning: too many arguments for format Looking at the code, the first two looks like the real thing, and not just false positives. IIRC, the strict aliasing rule is not a part of C89, but is in C99. And GCC starts to generate code that depends on strict-aliasing rules with -O3. We don't use -O3, so this might be theoretical, at least on my machine at this time. The last one is a usage if "%zu", which our printf doesn't support. This comes from commit 73118f89 ("merge-recursive.c: Add more generic merge_recursive_generic()"), and should probably be fixed by doing: ---8<--- diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index 703045b..02242cc 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -45,8 +45,9 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) bases[bases_count++] = sha; } else - warning("Cannot handle more than %zu bases. " - "Ignoring %s.", ARRAY_SIZE(bases)-1, argv[i]); + warning("Cannot handle more than %"PRIuMAX" bases. " + "Ignoring %s.", + (uintmax_t)ARRAY_SIZE(bases)-1, argv[i]); } if (argc - i != 3) /* "--" "<head>" "<remote>" */ die("Not handling anything other than two heads merge."); ---8<--- But no matter what, something has clearly happened to our warning level, and that should probably be investigated. Back to topic: after fixing these (apart from the strict aliasing issues), t5000-tar-tree pass fine here. So I guess that's at least something. I haven't tested from git-daemon yet, but I don't have time for that right now... I'll submit the first fix later tonight, as it's clearly an improvement IMO... but perhaps it's better if Stephan just squash in the latter in the next round of the series, since it seems to be in a pu-only series so far? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html