Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

From

Erik Faye-Lund <kusmabite@gmail.com>

Date

Dec 6, 2011, 11:22 UTC

Message-ID

<CABPQNSbOReM71HaPmce3v_98NDu17fT3YnySR4pWzJEDa-RKnA@mail.gmail.com>

In-Reply-To

<20111206055239.GA20671@sigill.intra.peff.net>

On Tue, Dec 6, 2011 at 6:52 AM, Jeff King <peff@peff.net> 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:

---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?