Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

From: Jeff King <peff@peff.net>
Date: Aug 25, 2014, 20:09 UTC
Message-ID: <20140825200939.GK30953@peff.net>
In-Reply-To: <20140825200042.GJ30953@peff.net>

On Mon, Aug 25, 2014 at 04:00:42PM -0400, Jeff King wrote:

Show 15 quoted lines

> On Mon, Aug 25, 2014 at 12:46:41PM -0700, Jonathan Nieder wrote:
> 
> > Jeff King wrote:
> > 
> > > -LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
> > > +LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)
> > 
> > Why is LIB_H dropped here?  This would mean that po/git.pot stops
> > including strings from macros and static inline functions in headers
> > (e.g., in parse-options.h).
> 
> Ick, this is an accidental leftover from the earlier iteration of the
> patch, which moved that part to a separate line (inside my gross
> GIT_REAL_POT conditional). The extra line went away, but I forgot to add
> $(LIB_H) back in here. Thanks for noticing.

As an aside, this makes an interesting case study for our git.git workflow.

In some workflows, I would have made the original unacceptable patch, you would have reviewed it, and then I would have made the followup patch to adjust it, and you would have reviewed that. But it's quite hard to see my mistake in just the followup patch; the fact that $(LIB_H) was originally part of $(LOCALIZED_C) does not appear in that hunk at all.

But in our workflow, we squash out the unacceptable diff, and you review from scratch the movement from the original working state (assuming the status quo was working :) ) to the newly proposed state. And there the mistake is quite a bit more obvious.

Just an interesting observation.

-Peff