patch, 3 partsMakefile: "make tags" fixes & cleanup

A small series to fix the various "tags" targets, i.e. "make tags TAGS cscope". We'll now properly detect their dependencies, so we don't needlessly run them every time. I have this as part of my standard "make git" command, so doing nothing when we have nothing to do is preferrable, especially when my editor will eagerly reload the TAGS file every time it changes.

As noted in 3/3 this is better on top of my just-submitted .DELETE_ON_ERROR change[1], but will also work independently of that patch/series.

1. https://lore.kernel.org/git/patch-1.1-9420448e74f-20210622T141100Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  Makefile: move ".PHONY: cscope" near its target
  Makefile: fix "cscope" target to refer to cscope.out
  Makefile: don't use "FORCE" for tags targets

 .gitignore |  2 +-
 Makefile   | 31 +++++++++++++++++--------------
 2 files changed, 18 insertions(+), 15 deletions(-)

Move the ".PHONY: cscope" rule to live alongside the "cscope" target itself, not to be all the way near the bottom where we define the "FORCE" rule.

That line was last modified in 2f76919517e (MinGW: avoid collisions between "tags" and "TAGS", 2010-09-28).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

The cscope target added in a2a9150bf06 (makefile: Add a cscope target, 2007-10-06) has for some reason been referring to cscope* instead of cscope.out. Let's generate the cscope.out file directly so we don't need to speculate.

The "-fcscope.out" (note, no whitespace) argument is enabled by default on my system's cscope 15.9, but let's provide it explicitly for good measure.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore | 2 +-
 Makefile   | 9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

On Tue, Jun 22, 2021 at 04:21:26PM +0200, Ævar Arnfjörð Bjarmason wrote:

I don't use cscope myself, but it can generate other files (e.g., with "-q"). It looks like we don't even allow people to set $(CSCOPE), though, so that shouldn't ever happen.

(I wonder if anybody even really uses cscope. I wanted to love it as a better form of ctags, but I have always found it so baroque and complicated that it ends up being a waste of my time to try it).

This drops the $(RM). Does cscope always overwrite the output file, or does it append? Just trying "cscope -b foo.c; cscope -b bar.c", it looks like it overwrites. Which makes your patch correct, but that existing "xargs" is somewhat questionable (if it splits into two commands, the first half will get dropped).

-Peff

On Tue, Jun 22, 2021 at 12:26 PM Jeff King <peff@peff.net> wrote:

I use cscope all the time, from vim. If you use vim, you'll probably want cscope_maps.vim.

(Sadly, cscope is not very good at C++.)

I think you're right about the xargs issue here.

Chris

Chris Torek wrote:

Or just set cscopetag.

Jeff King wrote:

I do. I don't use it regularly, as I find `git grep` sufficient most of the time, but sometimes I do use cscope.

Ævar Arnfjörð Bjarmason wrote:

The reason for the $(RM) removal is not explained in the commit message.

Otherwise the pach looks good to me.

Remove the "FORCE" dependency from the "tags", "TAGS" and "cscope.out" targets, instead make them depend on whether or not the relevant source files have changed.

I'm also removing the "-o" option from them, that seems to have been cargo-culted when they were initially added in f81e7c626f3 (Makefile: Add TAGS and tags targets, 2006-03-18). It would make sense to use that option if we had been appending to tag files, it doesn't make any sense that it was used after we'd just removed the files file being appended to.

This will potentially cause a partial file to be left behind if the command dies, but my in-flight series to use the ".DELETE_ON_ERROR" flag in the Makefile[1] will make that problem go away. I think even without that it's not problem we need to worry about in these cases.

1. https://lore.kernel.org/git/patch-1.1-9420448e74f-20210622T141100Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

On Tue, Jun 22, 2021 at 04:21:27PM +0200, Ævar Arnfjörð Bjarmason wrote:

You mean "-a" in this second paragraph, right?

I think it would help if xargs splits the source file list across multiple invocations of the command.

-Peff

Ævar Arnfjörð Bjarmason wrote:

Very nice.

As Jeff already pointed out, this is probably "-a".

Other than that looks good to me.

On Tue, Jun 22, 2021 at 04:21:24PM +0200, Ævar Arnfjörð Bjarmason wrote:

:-). Very nice.

I took a look at this and [1], and I agree that 3/3 is probably better applied after [1], since it doesn't leave any gap between the existing behavior of ">$@+ && mv $@+ $@" and ">$@" with .DELETE_ON_ERROR.

But I don't feel strongly about the order in which the two are applied. Your 3/3 without [1] isn't wrong, it just leaves us the opportunity to permanently leave around a broken tags file permanently, whereas having [1] applied ahead of time means that the broken tags file is only visible racily.

I reviewed these patches carefully, and they look good to me. I'm delighted that `make tags` is no longer PHONY.

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Looks mostly like good patches, with concrete suggestions for improvements given. Please do not leave another loose end that should be easy to tie untied and float away to some other topics.

Thanks.

On Mon, Jun 28 2021, Junio C Hamano wrote:

In the v2 re-roll of both I detached these two topics from each other, as it turns out it wasn't needed to begin with. Sorry about that.

A v2 of my fixes to the tags targets. This no longer depends on my .DELETE_ON_ERROR change, and goes directly on top of "master". See [2] for the just-submitted v2 of that other series.

The big win here is that none of the tags targets depend on "FORCE" anymore, so we'll only re-generate them if our sources change.

I missed the interaction of the "-a" flag and xargs splitting the arguments into am implicit -n, so in v1 of this we could end up with incomplete tag files. In this v2 we more incrementally reach similar ends, but in the end result retain our rm/gen/mv dance, since it's needed in this case.

1. http://lore.kernel.org/git/cover-0.3-00000000000-20210622T141844Z-avarab@gmail.com
2. https://lore.kernel.org/git/patch-1.1-2557117855-20210629T084356Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (5):
  Makefile: move ".PHONY: cscope" near its target
  Makefile: add QUIET_GEN to "cscope" target
  Makefile: fix "cscope" target to refer to cscope.out
  Makefile: don't use "FORCE" for tags targets
  Makefile: normalize clobbering & xargs for tags targets

 .gitignore |  2 +-
 Makefile   | 34 ++++++++++++++++++++--------------
 2 files changed, 21 insertions(+), 15 deletions(-)

Range-diff against v1:
1:  383a90c8ac = 1:  dd6cfd6022 Makefile: move ".PHONY: cscope" near its target
-:  ---------- > 2:  56daa09738 Makefile: add QUIET_GEN to "cscope" target
2:  ea39f1f5cd ! 3:  35c8b83904 Makefile: fix "cscope" target to refer to cscope.out
    @@ Commit message
     
         The cscope target added in a2a9150bf06 (makefile: Add a cscope target,
         2007-10-06) has for some reason been referring to cscope* instead of
    -    cscope.out. Let's generate the cscope.out file directly so we don't
    -    need to speculate.
    +    cscope.out.
     
    -    The "-fcscope.out" (note, no whitespace) argument is enabled by
    -    default on my system's cscope 15.9, but let's provide it explicitly
    -    for good measure.
    +    Let's generate the cscope.out file directly so we don't need to
    +    speculate. The "-fcscope.out" (note, no whitespace) argument is
    +    enabled by default on my system's cscope 15.9, but let's provide it
    +    explicitly for good measure.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ Makefile: tags: FORCE
      	mv tags+ tags
      
     +cscope.out:
    ++	$(QUIET_GEN)$(RM) cscope.out && \
     +	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
     +
      .PHONY: cscope
     -cscope:
    --	$(RM) cscope*
    +-	$(QUIET_GEN)$(RM) cscope* && \
     -	$(FIND_SOURCE_FILES) | xargs cscope -b
     +cscope: cscope.out
      
3:  67fc87665d ! 4:  b924cc3f56 Makefile: don't use "FORCE" for tags targets
    @@ Commit message
         targets, instead make them depend on whether or not the relevant
         source files have changed.
     
    -    I'm also removing the "-o" option from them, that seems to have been
    -    cargo-culted when they were initially added in f81e7c626f3 (Makefile:
    -    Add TAGS and tags targets, 2006-03-18). It would make sense to use
    -    that option if we had been appending to tag files, it doesn't make any
    -    sense that it was used after we'd just removed the files file being
    -    appended to.
    -
    -    This will potentially cause a partial file to be left behind if the
    -    command dies, but my in-flight series to use the ".DELETE_ON_ERROR"
    -    flag in the Makefile[1] will make that problem go away. I think even
    -    without that it's not problem we need to worry about in these cases.
    -
    -    1. https://lore.kernel.org/git/patch-1.1-9420448e74f-20210622T141100Z-avarab@gmail.com/
    -
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
    @@ Makefile: FIND_SOURCE_FILES = ( \
      	)
      
     -$(ETAGS_TARGET): FORCE
    --	$(QUIET_GEN)$(RM) "$(ETAGS_TARGET)+" && \
    --	$(FIND_SOURCE_FILES) | xargs etags -a -o "$(ETAGS_TARGET)+" && \
    --	mv "$(ETAGS_TARGET)+" "$(ETAGS_TARGET)"
     +FOUND_SOURCE_FILES = $(shell $(FIND_SOURCE_FILES))
    ++
    ++$(ETAGS_TARGET): $(FOUND_SOURCE_FILES)
    + 	$(QUIET_GEN)$(RM) "$(ETAGS_TARGET)+" && \
    + 	$(FIND_SOURCE_FILES) | xargs etags -a -o "$(ETAGS_TARGET)+" && \
    + 	mv "$(ETAGS_TARGET)+" "$(ETAGS_TARGET)"
      
     -tags: FORCE
    --	$(QUIET_GEN)$(RM) tags+ && \
    --	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
    --	mv tags+ tags
    -+$(ETAGS_TARGET): $(FOUND_SOURCE_FILES)
    -+	$(QUIET_GEN)echo $(FOUND_SOURCE_FILES) | \
    -+	xargs etags -o $@
    -+
     +tags: $(FOUND_SOURCE_FILES)
    -+	$(QUIET_GEN)echo $(FOUND_SOURCE_FILES) | \
    -+	xargs ctags -o $@
    + 	$(QUIET_GEN)$(RM) tags+ && \
    + 	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
    + 	mv tags+ tags
      
    - cscope.out:
    +-cscope.out:
    ++cscope.out: $(FOUND_SOURCE_FILES)
    + 	$(QUIET_GEN)$(RM) cscope.out && \
     -	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
    -+	$(QUIET_GEN)echo $(FOUND_SOURCE_FILES) | \
    -+	xargs cscope -f$@ -b
    ++	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b
      
      .PHONY: cscope
      cscope: cscope.out
-:  ---------- > 5:  5195d99e25 Makefile: normalize clobbering & xargs for tags targets

Move the ".PHONY: cscope" rule to live alongside the "cscope" target itself, not to be all the way near the bottom where we define the "FORCE" rule.

That line was last modified in 2f76919517e (MinGW: avoid collisions between "tags" and "TAGS", 2010-09-28).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Don't show the very verbose $(FIND_SOURCE_FILES) command on every "make cscope" invocation.

See my recent 3c80fcb591 (Makefile: add QUIET_GEN to "tags" and "TAGS" targets, 2021-03-28) for the same fix for the other adjacent targets.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The cscope target added in a2a9150bf06 (makefile: Add a cscope target, 2007-10-06) has for some reason been referring to cscope* instead of cscope.out.

Let's generate the cscope.out file directly so we don't need to speculate. The "-fcscope.out" (note, no whitespace) argument is enabled by default on my system's cscope 15.9, but let's provide it explicitly for good measure.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore |  2 +-
 Makefile   | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

On Tue, Jun 29, 2021 at 01:12:57PM +0200, Ævar Arnfjörð Bjarmason wrote:

Doesn't this break subsequent runs after the first generation? With a phony "cscope" target, "make cscope" will always run the command, even if it's not necessary. But with a real "cscope.out" target but not dependencies, it will _never_ run it, even if one of the files changed.

E.g., with your patch:

  $ make cscope.out
    GEN cscope.out

  $ make cscope.out
  make: 'cscope.out' is up to date.

  $ echo 'void foo(void) { }' >>git.c
  $ make cscope.out
  GIT_VERSION = 2.32.0.96.g5daee1b7bb.dirty
  make: 'cscope.out' is up to date.

-Peff

On Thu, Jul 01, 2021 at 06:23:52PM -0400, Jeff King wrote:

Ah, I see it is un-broken in the next commit, which adds actual dependencies. I think it is OK to have a temporarily-broken state in the history for something so trivial, but it might be worth mentioning it in the commit message.

-Peff

Remove the "FORCE" dependency from the "tags", "TAGS" and "cscope.out" targets, instead make them depend on whether or not the relevant source files have changed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

On 29/06/2021 12:12, Ævar Arnfjörð Bjarmason wrote:

I was expecting to see the above targets to be changed, similarly to ...

... this hunk (ie. 'an "echo <list> | xargs" pattern') Indeed, the above phrase was taken from the commit message of the next patch (5/5), which implies that this change had already happened (presumably in this patch).

ATB, Ramsay Jones

Since the "tags", "TAGS" and "cscope.out" targets rely on ping into xargs with an "echo <list> | xargs" pattern, we need to make sure we're in an append mode.

Unlike recent changes of mine to make use of ".DELETE_ON_ERROR" we really do need the "rm $@+" at the beginning (note, not "rm $@").

This is because the xargs command may decide on multiple invocations of the program. We need to make sure we've got a union of its results at the end.

For "ctags" and "etags" we used the "-a" flag for this, for cscope that behavior is the default. Its "-u" flag disables its equivalent of an implicit "-a" flag.

Let's also consistently use the $@ and $@+ names instead of needlessly hardcoding or referring to more verbose names in the "tags" and "TAGS" rules.

These targets could perhaps be improved in the future by factoring this "echo <list> | xargs" pattern so that we make intermediate tags files for each source file, and then assemble them into one "tags" file at the end.

The etags manual page suggests that doing that (or perhaps just --update) might be counter-productive, in any case, the tag building is fast enough for me, so I'm leaving that for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

The big win here is that none of the tags targets depend on "FORCE" anymore, so we'll only re-generate them if our sources change.

For v2, see https://lore.kernel.org/git/cover-0.5-0000000000-20210629T110837Z-avarab@gmail.com/

This fixes the series per feedback from Jeff King and Ramsay Jones, i.e:

 * In v2 the 3/5 broke things in a way that 4/5 fixed, that's now
   re-arranged and fixed.

 * I now use $(FOUND_SOURCE_FILES) instead of $(FIND_SOURCE_FILES)
   consistently. I was redundantly re-running the "find" command.

Ævar Arnfjörð Bjarmason (5):
  Makefile: move ".PHONY: cscope" near its target
  Makefile: add QUIET_GEN to "cscope" target
  Makefile: don't use "FORCE" for tags targets
  Makefile: the "cscope" target always creates a "cscope.out"
  Makefile: normalize clobbering & xargs for tags targets

 .gitignore |  2 +-
 Makefile   | 34 ++++++++++++++++++++--------------
 2 files changed, 21 insertions(+), 15 deletions(-)

Range-diff against v2:
1:  dd6cfd6022c = 1:  6b4ddc126d9 Makefile: move ".PHONY: cscope" near its target
2:  56daa09738f = 2:  d3d5d332e92 Makefile: add QUIET_GEN to "cscope" target
4:  b924cc3f566 ! 3:  9dd69d68178 Makefile: don't use "FORCE" for tags targets
    @@ Metadata
      ## Commit message ##
         Makefile: don't use "FORCE" for tags targets
     
    -    Remove the "FORCE" dependency from the "tags", "TAGS" and "cscope.out"
    +    Remove the "FORCE" dependency from the "tags", "TAGS" and "cscope"
         targets, instead make them depend on whether or not the relevant
         source files have changed.
     
    +    For the cscope target we need to change it to depend on the actual
    +    generated file while we generate while we're at it, as the next commit
    +    will discuss we always generate a cscope.out file.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
    @@ Makefile: FIND_SOURCE_FILES = ( \
     +
     +$(ETAGS_TARGET): $(FOUND_SOURCE_FILES)
      	$(QUIET_GEN)$(RM) "$(ETAGS_TARGET)+" && \
    - 	$(FIND_SOURCE_FILES) | xargs etags -a -o "$(ETAGS_TARGET)+" && \
    +-	$(FIND_SOURCE_FILES) | xargs etags -a -o "$(ETAGS_TARGET)+" && \
    ++	echo $(FOUND_SOURCE_FILES) | xargs etags -a -o "$(ETAGS_TARGET)+" && \
      	mv "$(ETAGS_TARGET)+" "$(ETAGS_TARGET)"
      
     -tags: FORCE
     +tags: $(FOUND_SOURCE_FILES)
      	$(QUIET_GEN)$(RM) tags+ && \
    - 	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
    +-	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
    ++	echo $(FOUND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
      	mv tags+ tags
      
    --cscope.out:
    +-.PHONY: cscope
    +-cscope:
     +cscope.out: $(FOUND_SOURCE_FILES)
    - 	$(QUIET_GEN)$(RM) cscope.out && \
    --	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
    -+	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b
    + 	$(QUIET_GEN)$(RM) cscope* && \
    +-	$(FIND_SOURCE_FILES) | xargs cscope -b
    ++	echo $(FOUND_SOURCE_FILES) | xargs cscope -b
    ++
    ++.PHONY: cscope
    ++cscope: cscope.out
      
    - .PHONY: cscope
    - cscope: cscope.out
    + ### Detect prefix changes
    + TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
     @@ Makefile: check: config-list.h command-list.h
      		exit 1; \
      	fi
3:  35c8b839048 ! 4:  f8d151f1f6a Makefile: fix "cscope" target to refer to cscope.out
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    Makefile: fix "cscope" target to refer to cscope.out
    +    Makefile: the "cscope" target always creates a "cscope.out"
     
    -    The cscope target added in a2a9150bf06 (makefile: Add a cscope target,
    -    2007-10-06) has for some reason been referring to cscope* instead of
    +    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.
     
    -    Let's generate the cscope.out file directly so we don't need to
    -    speculate. The "-fcscope.out" (note, no whitespace) argument is
    -    enabled by default on my system's cscope 15.9, but let's provide it
    -    explicitly for good measure.
    +    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
      *.obj
     
      ## Makefile ##
    -@@ Makefile: tags: FORCE
    - 	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
    +@@ Makefile: tags: $(FOUND_SOURCE_FILES)
      	mv tags+ tags
      
    -+cscope.out:
    -+	$(QUIET_GEN)$(RM) cscope.out && \
    -+	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
    -+
    - .PHONY: cscope
    --cscope:
    + cscope.out: $(FOUND_SOURCE_FILES)
     -	$(QUIET_GEN)$(RM) cscope* && \
    --	$(FIND_SOURCE_FILES) | xargs cscope -b
    -+cscope: cscope.out
    +-	echo $(FOUND_SOURCE_FILES) | xargs cscope -b
    ++	$(QUIET_GEN)$(RM) cscope.out && \
    ++	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b
      
    - ### Detect prefix changes
    - TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
    + .PHONY: cscope
    + cscope: cscope.out
     @@ Makefile: clean: profile-clean coverage-clean cocciclean
      	$(RM) $(HCC)
      	$(RM) -r bin-wrappers $(dep_dirs) $(compdb_dir) compile_commands.json
5:  5195d99e25c ! 5:  f3ff76d0e98 Makefile: normalize clobbering & xargs for tags targets
    @@ Metadata
      ## Commit message ##
         Makefile: normalize clobbering & xargs for tags targets
     
    -    Since the "tags", "TAGS" and "cscope.out" targets rely on ping into
    +    Since the "tags", "TAGS" and "cscope.out" targets rely on piping into
         xargs with an "echo <list> | xargs" pattern, we need to make sure
         we're in an append mode.
     
    -    Unlike recent changes of mine to make use of ".DELETE_ON_ERROR" we
    -    really do need the "rm $@+" at the beginning (note, not "rm $@").
    +    Unlike my recent change to make use of ".DELETE_ON_ERROR" in
    +    7b76d6bf221 (Makefile: add and use the ".DELETE_ON_ERROR" flag,
    +    2021-06-29), we really do need the "rm $@+" at the beginning (note,
    +    not "rm $@").
     
    -    This is because the xargs command may decide on multiple invocations
    -    of the program. We need to make sure we've got a union of its results
    +    This is because the xargs command may decide to invoke the program
    +    multiple times. We need to make sure we've got a union of its results
         at the end.
     
         For "ctags" and "etags" we used the "-a" flag for this, for cscope
    @@ Makefile: FIND_SOURCE_FILES = ( \
      
      $(ETAGS_TARGET): $(FOUND_SOURCE_FILES)
     -	$(QUIET_GEN)$(RM) "$(ETAGS_TARGET)+" && \
    --	$(FIND_SOURCE_FILES) | xargs etags -a -o "$(ETAGS_TARGET)+" && \
    +-	echo $(FOUND_SOURCE_FILES) | xargs etags -a -o "$(ETAGS_TARGET)+" && \
     -	mv "$(ETAGS_TARGET)+" "$(ETAGS_TARGET)"
     +	$(QUIET_GEN)$(RM) $@+ && \
     +	echo $(FOUND_SOURCE_FILES) | xargs etags -a -o $@+ && \
    @@ Makefile: FIND_SOURCE_FILES = ( \
      
      tags: $(FOUND_SOURCE_FILES)
     -	$(QUIET_GEN)$(RM) tags+ && \
    --	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
    +-	echo $(FOUND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
     -	mv tags+ tags
     +	$(QUIET_GEN)$(RM) $@+ && \
     +	echo $(FOUND_SOURCE_FILES) | xargs ctags -a -o $@+ && \

Move the ".PHONY: cscope" rule to live alongside the "cscope" target itself, not to be all the way near the bottom where we define the "FORCE" rule.

That line was last modified in 2f76919517e (MinGW: avoid collisions between "tags" and "TAGS", 2010-09-28).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Don't show the very verbose $(FIND_SOURCE_FILES) command on every "make cscope" invocation.

See my recent 3c80fcb591 (Makefile: add QUIET_GEN to "tags" and "TAGS" targets, 2021-03-28) for the same fix for the other adjacent targets.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Remove the "FORCE" dependency from the "tags", "TAGS" and "cscope" targets, instead make them depend on whether or not the relevant source files have changed.

For the cscope target we need to change it to depend on the actual generated file while we generate while we're at it, as the next commit will discuss we always generate a cscope.out file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

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.

On Thu, Jul 22, 2021 at 09:55:31AM -0700, Junio C Hamano wrote:

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:

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:

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?

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:

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

Since the "tags", "TAGS" and "cscope.out" targets rely on piping into xargs with an "echo <list> | xargs" pattern, we need to make sure we're in an append mode.

Unlike my recent change to make use of ".DELETE_ON_ERROR" in 7b76d6bf221 (Makefile: add and use the ".DELETE_ON_ERROR" flag, 2021-06-29), we really do need the "rm $@+" at the beginning (note, not "rm $@").

This is because the xargs command may decide to invoke the program multiple times. We need to make sure we've got a union of its results at the end.

For "ctags" and "etags" we used the "-a" flag for this, for cscope that behavior is the default. Its "-u" flag disables its equivalent of an implicit "-a" flag.

Let's also consistently use the $@ and $@+ names instead of needlessly hardcoding or referring to more verbose names in the "tags" and "TAGS" rules.

These targets could perhaps be improved in the future by factoring this "echo <list> | xargs" pattern so that we make intermediate tags files for each source file, and then assemble them into one "tags" file at the end.

The etags manual page suggests that doing that (or perhaps just --update) might be counter-productive, in any case, the tag building is fast enough for me, so I'm leaving that for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

On Thu, Jul 22, 2021 at 01:23:06AM +0200, Ævar Arnfjörð Bjarmason wrote:

Hrm, that's not the experience I get with cscope. E.g.:

  $ cscope -b wt-status.c
  $ grep -m1 wt-status.c cscope.out
	@wt-status.c
  $ cscope -b git.c
  $ grep -m1 wt-status.c cscope.out
  [no output]

I wondered if I was being too hacky with my grep there. But if I simulate more extreme cmdline-splitting like so:

On Fri, Jul 23, 2021 at 06:43:13AM -0400, Jeff King wrote:

Just to make it extra clear: this is not a problem introduced by your series, since we were already using xargs with cscope. But this seems like a good time to fix it.

-Peff

On Thu, Jul 22, 2021 at 01:23:01AM +0200, Ævar Arnfjörð Bjarmason wrote:

Thanks. Aside from some cscope appending arcana I found, these look good to me. I have no opinion on "cscope*" versus "cscope.out", except that it is not worth anybody's time to argue about. :)

-Peff

Jeff King <peff@peff.net> writes:

Thanks, sorry and I agree with both counts.

Jeff King wrote:

I agree with that as well.

Ævar Arnfjörð Bjarmason wrote:

Very nice.

I think most of the nitpicking in the comments is not really worth the trouble (either for or against), there is one comment from Jeff King regarding multiple runs of the xargs command that I think is valid, but too hypothetical for me to care about (ARG_MAX is 2097152 on my system), so it would be nice to fix, but not necessary.

Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>

The big win here is that none of the tags targets depend on "FORCE" anymore, so we'll only re-generate them if our sources change.

For v3, see: https://lore.kernel.org/git/cover-0.5-00000000000-20210721T231900Z-avarab@gmail.com/

This addresses the feedback about the cscope* v.s. cscope.out rule in .gitignore and "make clean", i.e. those rules are not being changed anymore. I also changed a stray cscope.out to $@ in 4/5, which was missed in v3.

Ævar Arnfjörð Bjarmason (5):
  Makefile: move ".PHONY: cscope" near its target
  Makefile: add QUIET_GEN to "cscope" target
  Makefile: don't use "FORCE" for tags targets
  Makefile: remove "cscope.out", not "cscope*" in cscope.out target
  Makefile: normalize clobbering & xargs for tags targets

 Makefile | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Range-diff against v3:
1:  6b4ddc126d9 = 1:  2ee725e2fba Makefile: move ".PHONY: cscope" near its target
2:  d3d5d332e92 = 2:  2122cb25633 Makefile: add QUIET_GEN to "cscope" target
3:  9dd69d68178 = 3:  8649716772b Makefile: don't use "FORCE" for tags targets
4:  f8d151f1f6a ! 4:  643c514e12a Makefile: the "cscope" target always creates a "cscope.out"
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    Makefile: the "cscope" target always creates a "cscope.out"
    +    Makefile: remove "cscope.out", not "cscope*" in cscope.out target
     
    -    In the preceding commit the "cscope" target was changed to be a phony
    -    alias for the "cscope.out" target.
    +    Before we generate a "cscope.out" file, remove that file explicitly,
    +    and not everything matching "cscope*". This doesn't change any
    +    behavior of the Makefile in practice, but makes this rule less
    +    confusing, and consistent with other similar rules.
     
         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.
    +    target, 2007-10-06). It has always referred to cscope* instead of to
    +    cscope.out in .gitignore and the "clean" target, even though we only
    +    ever generated a cscope.out file.
     
    -    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.
    +    This was seemingly done to aid use-cases where someone invoked cscope
    +    with the "-q" flag, which would make it create a "cscope.in.out" and
    +    "cscope.po.out" files in addition to "cscope.out".
     
    -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    But us removing those files we never generated is confusing, so let's
    +    only remove the file we need to, furthermore let's use the "-f" flag
    +    to explicitly name the cscope.out file, even though it's the default
    +    if not "-f" argument is supplied.
    +
    +    It is somewhat inconsistent to change from the glob here but not in
    +    the "clean" rule and .gitignore, an earlier version of this change
    +    updated those as well, but see [1][2] for why they were kept.
     
    - ## .gitignore ##
    -@@
    - /.vscode/
    - /tags
    - /TAGS
    --/cscope*
    -+/cscope.out
    - /compile_commands.json
    - *.hcc
    - *.obj
    +    1. https://lore.kernel.org/git/87k0lit57x.fsf@evledraar.gmail.com/
    +    2. https://lore.kernel.org/git/87im0kn983.fsf@evledraar.gmail.com/
    +
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
     @@ Makefile: tags: $(FOUND_SOURCE_FILES)
    @@ Makefile: tags: $(FOUND_SOURCE_FILES)
      cscope.out: $(FOUND_SOURCE_FILES)
     -	$(QUIET_GEN)$(RM) cscope* && \
     -	echo $(FOUND_SOURCE_FILES) | xargs cscope -b
    -+	$(QUIET_GEN)$(RM) cscope.out && \
    ++	$(QUIET_GEN)$(RM) $@ && \
     +	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b
      
      .PHONY: cscope
      cscope: cscope.out
    -@@ Makefile: 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
5:  f3ff76d0e98 ! 5:  1eaf3416329 Makefile: normalize clobbering & xargs for tags targets
    @@ Makefile: FIND_SOURCE_FILES = ( \
     +	mv $@+ $@
      
      cscope.out: $(FOUND_SOURCE_FILES)
    --	$(QUIET_GEN)$(RM) cscope.out && \
    +-	$(QUIET_GEN)$(RM) $@ && \
     -	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b
     +	$(QUIET_GEN)$(RM) $@+ && \
     +	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@+ -b && \

Move the ".PHONY: cscope" rule to live alongside the "cscope" target itself, not to be all the way near the bottom where we define the "FORCE" rule.

That line was last modified in 2f76919517e (MinGW: avoid collisions between "tags" and "TAGS", 2010-09-28).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Don't show the very verbose $(FIND_SOURCE_FILES) command on every "make cscope" invocation.

See my recent 3c80fcb591 (Makefile: add QUIET_GEN to "tags" and "TAGS" targets, 2021-03-28) for the same fix for the other adjacent targets.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Remove the "FORCE" dependency from the "tags", "TAGS" and "cscope" targets, instead make them depend on whether or not the relevant source files have changed.

For the cscope target we need to change it to depend on the actual generated file while we generate while we're at it, as the next commit will discuss we always generate a cscope.out file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Before we generate a "cscope.out" file, remove that file explicitly, and not everything matching "cscope*". This doesn't change any behavior of the Makefile in practice, but makes this rule less confusing, and consistent with other similar rules.

The cscope target was added in a2a9150bf06 (makefile: Add a cscope target, 2007-10-06). It has always referred to cscope* instead of to cscope.out in .gitignore and the "clean" target, even though we only ever generated a cscope.out file.

This was seemingly done to aid use-cases where someone invoked cscope with the "-q" flag, which would make it create a "cscope.in.out" and "cscope.po.out" files in addition to "cscope.out".

But us removing those files we never generated is confusing, so let's only remove the file we need to, furthermore let's use the "-f" flag to explicitly name the cscope.out file, even though it's the default if not "-f" argument is supplied.

It is somewhat inconsistent to change from the glob here but not in the "clean" rule and .gitignore, an earlier version of this change updated those as well, but see [1][2] for why they were kept.

1. https://lore.kernel.org/git/87k0lit57x.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/87im0kn983.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Since the "tags", "TAGS" and "cscope.out" targets rely on piping into xargs with an "echo <list> | xargs" pattern, we need to make sure we're in an append mode.

Unlike my recent change to make use of ".DELETE_ON_ERROR" in 7b76d6bf221 (Makefile: add and use the ".DELETE_ON_ERROR" flag, 2021-06-29), we really do need the "rm $@+" at the beginning (note, not "rm $@").

This is because the xargs command may decide to invoke the program multiple times. We need to make sure we've got a union of its results at the end.

For "ctags" and "etags" we used the "-a" flag for this, for cscope that behavior is the default. Its "-u" flag disables its equivalent of an implicit "-a" flag.

Let's also consistently use the $@ and $@+ names instead of needlessly hardcoding or referring to more verbose names in the "tags" and "TAGS" rules.

These targets could perhaps be improved in the future by factoring this "echo <list> | xargs" pattern so that we make intermediate tags files for each source file, and then assemble them into one "tags" file at the end.

The etags manual page suggests that doing that (or perhaps just --update) might be counter-productive, in any case, the tag building is fast enough for me, so I'm leaving that for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)