Subject: Re: [PATCH v4 09/15] bugreport: generate config safelist based on docs

From: Emily Shaffer <[email-masked]>
Date: Mon, 16 Dec 2019 16:41:27 -0800

On Mon, Dec 16, 2019 at 03:01:24PM -0800, Emily Shaffer wrote:
> On Fri, Dec 13, 2019 at 02:57:17PM -0800, Junio C Hamano wrote:
> > Emily Shaffer <[email-masked]> writes:
> > 
> > > diff --git a/Makefile b/Makefile
> > > index c49f55a521..76dc51e2b1 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -651,7 +651,7 @@ install-perl-script: $(SCRIPT_PERL_GEN)
> > >  install-python-script: $(SCRIPT_PYTHON_GEN)
> > >  	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> > >  
> > > -.PHONY: clean-perl-script clean-sh-script clean-python-script
> > > +.PHONY: clean-perl-script clean-sh-script clean-python-script clean-script-dependencies
> > >  clean-sh-script:
> > >  	$(RM) $(SCRIPT_SH_GEN)
> > >  clean-perl-script:
> > > @@ -817,6 +817,7 @@ VCSSVN_LIB = vcs-svn/lib.a
> > >  
> > >  GENERATED_H += config-list.h
> > >  GENERATED_H += command-list.h
> > > +GENERATED_H += bugreport-config-safelist.h
> > 
> > OK.
> > 
> > >  LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
> > >  	$(FIND) . \
> > > @@ -2161,6 +2162,12 @@ command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Doc
> > >  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
> > >  		command-list.txt >$@+ && mv $@+ $@
> > >  
> > > +bugreport-config-safelist.h: generate-bugreport-config-safelist.sh
> > > +
> > > +bugreport-config-safelist.h: Documentation/config/*.txt
> > > +	$(QUIET_GEN)$(SHELL_PATH) ./generate-bugreport-config-safelist.sh \
> > > +		>$@+ && mv $@+ $@
> > 
> > OK.
> > 
> > >  SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
> > >  	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
> > >  	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
> > 
> > But bugreport.o needs this *.h file generated before a compiler
> > attempts to produce it out of bugreport.c; that dependency is
> > missing from this update to the Makefile.
> 
> Hm. Somewhere I misformatted my commits, then. My goal was to add the
> new header here, but not add it as a dependency to anybody; then, in the
> next commit, add it as a dependency to git-bugreport and start to use it
> (commit 1: make the thing; commit 2: use the thing). But now when I look
> at the next commit, I don't see a Makefile change. So I missed something
> while I was shuffling around fixups.  Thanks for noticing.
> 
> > 
> > > @@ -2791,7 +2798,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
> > >  .PHONY: sparse $(SP_OBJ)
> > >  sparse: $(SP_OBJ)
> > >  
> > > -GEN_HDRS := config-list.h command-list.h unicode-width.h
> > > +GEN_HDRS := config-list.h command-list.h unicode-width.h bugreport-config-safelist.h
> > >  EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/%
> > >  ifndef GCRYPT_SHA256
> > >  	EXCEPT_HDRS += sha256/gcrypt.h
> > > @@ -3117,7 +3124,8 @@ clean: profile-clean coverage-clean cocciclean
> > >  	$(RM) $(HCC)
> > >  	$(RM) -r bin-wrappers $(dep_dirs)
> > >  	$(RM) -r po/build/
> > > -	$(RM) *.pyc *.pyo */*.pyc */*.pyo config-list.h command-list.h
> > > +	$(RM) *.pyc *.pyo */*.pyc */*.pyo
> > > +	$(RM) config-list.h command-list.h bugreport-config-safelist.h
> > 
> > It is kind of sad that GEN_HDRS defines the list of build artifact
> > that we should be able to clean, and then we manually list them to
> > be removed here independently.  Can we fix it up too?
> 
> Yeah, I can do that. I thought so too when I was updating this.
> 
> I *think* what happened is that when I split config-list away (in the
> earleir commit in this chain) I didn't notice that command-list.h was
> already in GEN_HDRS, and instead just grepped Makefile for
> "command-list.h" and added config-list.h next to it. So I'll try to fix
> it much earlier, in that commit, and then simply add to the appropriate
> variables in this commit.
> 
> > 
> > We probably clean up the build/update procedure around unicode-width.h
> > before we can do so, probably.  It may be generatable using contrib/
> > script, but as far as our normal build is concerned, it is a tracked
> > source and not part of the build byproducts, so we probably would
> > want to remove it from GEN_HDRS.  When that happens, we can $(RM)
> > all of the $(GEN_HDRS) in the "clean" target.
> 
> Noted. Thanks.

Ah, after I sent this mail I saw your patch. For those playing along at
home, https://lore.kernel.org/[email-masked]

Will review.

 - Emily