| Message ID | patch-4.5-f8d151f1f6a-20210721T231900Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Makefile: "make tags" fixes & cleanup | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > In the preceding commit the "cscope" target was changed to be a phony > alias for the "cscope.out" target. > > The cscope target was added in a2a9150bf06 (makefile: Add a cscope > target, 2007-10-06), and has always referred to cscope* instead of to > cscope.out. > > As far as I can tell this ambiguity was never needed. The > "-fcscope.out" (note, no whitespace) argument is enabled by default, > but let's provide it explicitly for good measure. Isn't it because we anticipated possible use of options like '-q' in the future that we clean and ignore cscope* instead of the single cscope.out and nothing else? $ echo *.c | xargs cscope -b -q; git clean -n -x Would remove cscope.in.out Would remove cscope.out Would remove cscope.po.out I know we currently care only about cscope.out and it is perfectly fine to make the phony cscope depend on cscope.out only, but I'd feel safer to keep the exclude patterns and $(RM) clean rule to catch them. > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > .gitignore | 2 +- > Makefile | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/.gitignore b/.gitignore > index 311841f9bed..d74029c1ca7 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -217,7 +217,7 @@ > /.vscode/ > /tags > /TAGS > -/cscope* > +/cscope.out > /compile_commands.json > *.hcc > *.obj > diff --git a/Makefile b/Makefile > index 18895d94ffa..730ff23b923 100644 > --- a/Makefile > +++ b/Makefile > @@ -2752,8 +2752,8 @@ tags: $(FOUND_SOURCE_FILES) > mv tags+ tags > > cscope.out: $(FOUND_SOURCE_FILES) > - $(QUIET_GEN)$(RM) cscope* && \ > - echo $(FOUND_SOURCE_FILES) | xargs cscope -b > + $(QUIET_GEN)$(RM) cscope.out && \ > + echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b > > .PHONY: cscope > cscope: cscope.out > @@ -3230,7 +3230,7 @@ clean: profile-clean coverage-clean cocciclean > $(RM) $(HCC) > $(RM) -r bin-wrappers $(dep_dirs) $(compdb_dir) compile_commands.json > $(RM) -r po/build/ > - $(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope* > + $(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope.out > $(RM) -r .dist-tmp-dir .doc-tmp-dir > $(RM) $(GIT_TARNAME).tar.gz > $(RM) $(htmldocs).tar.gz $(manpages).tar.gz
On Thu, Jul 22, 2021 at 09:55:31AM -0700, Junio C Hamano wrote: > I know we currently care only about cscope.out and it is perfectly > fine to make the phony cscope depend on cscope.out only, but I'd > feel safer to keep the exclude patterns and $(RM) clean rule to > catch them. I agree. I wondered about whether this patch could just get dropped entirely, since after removing the changes in .gitignore and the "clean" rule, the only change left is: > > cscope.out: $(FOUND_SOURCE_FILES) > > - $(QUIET_GEN)$(RM) cscope* && \ > > - echo $(FOUND_SOURCE_FILES) | xargs cscope -b > > + $(QUIET_GEN)$(RM) cscope.out && \ > > + echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b But that alone is a good change in my mind at least. Then it's clear that that target is responsible for generating cscope.out and nothing else. So I'd be in favor of rewording the patch message and only retaining this hunk (and dropping the other two). Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: >> > cscope.out: $(FOUND_SOURCE_FILES) >> > - $(QUIET_GEN)$(RM) cscope* && \ >> > - echo $(FOUND_SOURCE_FILES) | xargs cscope -b >> > + $(QUIET_GEN)$(RM) cscope.out && \ >> > + echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b > > But that alone is a good change in my mind at least. Then it's clear > that that target is responsible for generating cscope.out and nothing > else. Probably. The preparatory $(RM) is close enough to the invocation of cscope that anybody adding other options like '-q' should be careful enough to notice that the line needs to be touched, too, so I can be talked into thinking that $(RM) change here is a good one. I do not know about "-f$@", which is "Meh" to me. Is there a good reason to suspect that they might want to change the default output filename? > So I'd be in favor of rewording the patch message and only retaining > this hunk (and dropping the other two). Yup. I do not mind dropping one half of this hunk, too, but again, I do not mind keeping -f$@, either [*1*]. [Footnote] *1* if the patch to add cscope support anew were done with -f$@, I wouldn't have insisted removing it, out of the principle "any Meh change is not worth applying once the code was written and working".
On Thu, Jul 22 2021, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > >>> > cscope.out: $(FOUND_SOURCE_FILES) >>> > - $(QUIET_GEN)$(RM) cscope* && \ >>> > - echo $(FOUND_SOURCE_FILES) | xargs cscope -b >>> > + $(QUIET_GEN)$(RM) cscope.out && \ >>> > + echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b >> >> But that alone is a good change in my mind at least. Then it's clear >> that that target is responsible for generating cscope.out and nothing >> else. > > Probably. The preparatory $(RM) is close enough to the invocation > of cscope that anybody adding other options like '-q' should be > careful enough to notice that the line needs to be touched, too, so > I can be talked into thinking that $(RM) change here is a good one. > I do not know about "-f$@", which is "Meh" to me. Is there a good > reason to suspect that they might want to change the default output > filename? > >> So I'd be in favor of rewording the patch message and only retaining >> this hunk (and dropping the other two). > > Yup. I do not mind dropping one half of this hunk, too, but again, > I do not mind keeping -f$@, either [*1*]. > > > [Footnote] > > *1* if the patch to add cscope support anew were done with -f$@, I > wouldn't have insisted removing it, out of the principle "any Meh > change is not worth applying once the code was written and > working". As long as we have no user of a -q flag ever, what's the point of forever carrying the "rm foo*" when we know it's foo.out, just because a future Makefile change might add foo.blah? Doesn't seem like something we'd trip over, and the .gitignore/rm rule is just misleading now...
diff --git a/.gitignore b/.gitignore index 311841f9bed..d74029c1ca7 100644 --- a/.gitignore +++ b/.gitignore @@ -217,7 +217,7 @@ /.vscode/ /tags /TAGS -/cscope* +/cscope.out /compile_commands.json *.hcc *.obj diff --git a/Makefile b/Makefile index 18895d94ffa..730ff23b923 100644 --- a/Makefile +++ b/Makefile @@ -2752,8 +2752,8 @@ tags: $(FOUND_SOURCE_FILES) mv tags+ tags cscope.out: $(FOUND_SOURCE_FILES) - $(QUIET_GEN)$(RM) cscope* && \ - echo $(FOUND_SOURCE_FILES) | xargs cscope -b + $(QUIET_GEN)$(RM) cscope.out && \ + echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b .PHONY: cscope cscope: cscope.out @@ -3230,7 +3230,7 @@ clean: profile-clean coverage-clean cocciclean $(RM) $(HCC) $(RM) -r bin-wrappers $(dep_dirs) $(compdb_dir) compile_commands.json $(RM) -r po/build/ - $(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope* + $(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope.out $(RM) -r .dist-tmp-dir .doc-tmp-dir $(RM) $(GIT_TARNAME).tar.gz $(RM) $(htmldocs).tar.gz $(manpages).tar.gz
In the preceding commit the "cscope" target was changed to be a phony alias for the "cscope.out" target. The cscope target was added in a2a9150bf06 (makefile: Add a cscope target, 2007-10-06), and has always referred to cscope* instead of to cscope.out. As far as I can tell this ambiguity was never needed. The "-fcscope.out" (note, no whitespace) argument is enabled by default, but let's provide it explicitly for good measure. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- .gitignore | 2 +- Makefile | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)