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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]