On Wed, May 25, 2011 at 12:23:44PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > The if statement says "we might be passing NULL in fmt and in that case > > please fall back to user_format" to human readers, but the compiler is too > > stupid to infer such an intention, so you have to help it with your brain. > > I have to wonder if the strbuf_expand() should be passing fmt instead of > > user_format. "git blame -L1082,+7 pretty.c" points at 5b16360 (pretty: > > Initialize notes if %N is used, 2010-04-13). > > > > The only callsite that is introduced by that patch passes NULL to fmt, so > > a better fix might be to do something like this instead. > > If somebody cares about the reusability of the code for other callsites > added in the future, we could do this instead. > > I think this is what Johannes wanted to do from the beginning, and is a > better fix than my previous one to remove the fmt parameter altogether. Actually, I am to blame for this interface, see: http://article.gmane.org/gmane.comp.version-control.git/144650 and my response: http://article.gmane.org/gmane.comp.version-control.git/144715 The resulting patch definitely has a bug (albeit one that could not be triggered by current users), and should have had this: > - strbuf_expand(&dummy, user_format, userformat_want_item, w); > + strbuf_expand(&dummy, fmt, userformat_want_item, w); all along. I'm also OK with just tightening the interface to always use user_format, as no callers who wanted the extra parameter have come up in the past year. > Subject: userformat_find_requirements(): find requirement for the correct format > > This function was introduced in 5b16360 (pretty: Initialize notes if %N is > used, 2010-04-13) to check what kind of information the "log --format=..." > user format string wants. The function can be passed a NULL instead of a > format string to ask it to check user_format variable kept by an earlier > call to save_user_format(). > > But it unconditionally checked user_format and not the string it was > given. The only caller introduced by the change passes NULL, which > kept the bug unnoticed, until a new GCC noticed that there is an > assignment to fmt that is never used. Acked-by: Jeff King <peff@xxxxxxxx> > Noticed-by: Chris Wilson's compiler Heh. -Peff -- 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