git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/3] Makefile: "make tags" fixes & cleanup
@ 2021-06-22 14:21 Ævar Arnfjörð Bjarmason
  2021-06-22 14:21 ` [PATCH 1/3] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
                   ` (5 more replies)
  0 siblings, 6 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-22 14:21 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Ævar Arnfjörð Bjarmason

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

-- 
2.32.0.599.g3967b4fa4ac


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/3] Makefile: move ".PHONY: cscope" near its target
  2021-06-22 14:21 [PATCH 0/3] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
@ 2021-06-22 14:21 ` Ævar Arnfjörð Bjarmason
  2021-06-22 14:21 ` [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out Ævar Arnfjörð Bjarmason
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-22 14:21 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index c3565fc0f8f..4dd9711a653 100644
--- a/Makefile
+++ b/Makefile
@@ -2737,6 +2737,7 @@ tags: FORCE
 	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
 	mv tags+ tags
 
+.PHONY: cscope
 cscope:
 	$(RM) cscope*
 	$(FIND_SOURCE_FILES) | xargs cscope -b
@@ -3245,7 +3246,7 @@ endif
 
 .PHONY: all install profile-clean cocciclean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-.PHONY: FORCE cscope
+.PHONY: FORCE
 
 ### Check documentation
 #
-- 
2.32.0.599.g3967b4fa4ac


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out
  2021-06-22 14:21 [PATCH 0/3] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
  2021-06-22 14:21 ` [PATCH 1/3] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
@ 2021-06-22 14:21 ` Ævar Arnfjörð Bjarmason
  2021-06-22 19:25   ` Jeff King
  2021-06-23 19:25   ` Felipe Contreras
  2021-06-22 14:21 ` [PATCH 3/3] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-22 14:21 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Ævar Arnfjörð Bjarmason

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

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 4dd9711a653..25d2a3e5ddc 100644
--- a/Makefile
+++ b/Makefile
@@ -2737,10 +2737,11 @@ tags: FORCE
 	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
 	mv tags+ tags
 
+cscope.out:
+	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
+
 .PHONY: cscope
-cscope:
-	$(RM) cscope*
-	$(FIND_SOURCE_FILES) | xargs cscope -b
+cscope: cscope.out
 
 ### Detect prefix changes
 TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
@@ -3211,7 +3212,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
-- 
2.32.0.599.g3967b4fa4ac


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 3/3] Makefile: don't use "FORCE" for tags targets
  2021-06-22 14:21 [PATCH 0/3] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
  2021-06-22 14:21 ` [PATCH 1/3] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
  2021-06-22 14:21 ` [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out Ævar Arnfjörð Bjarmason
@ 2021-06-22 14:21 ` Ævar Arnfjörð Bjarmason
  2021-06-22 19:28   ` Jeff King
  2021-06-23 19:49   ` Felipe Contreras
  2021-06-22 15:16 ` [PATCH 0/3] Makefile: "make tags" fixes & cleanup Taylor Blau
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-22 14:21 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index 25d2a3e5ddc..89d261230fb 100644
--- a/Makefile
+++ b/Makefile
@@ -2727,18 +2727,19 @@ FIND_SOURCE_FILES = ( \
 		| sed -e 's|^\./||' \
 	)
 
-$(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))
 
-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 $@
 
 cscope.out:
-	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
+	$(QUIET_GEN)echo $(FOUND_SOURCE_FILES) | \
+	xargs cscope -f$@ -b
 
 .PHONY: cscope
 cscope: cscope.out
@@ -2922,7 +2923,7 @@ check: config-list.h command-list.h
 		exit 1; \
 	fi
 
-FOUND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
+FOUND_C_SOURCES = $(filter %.c,$(FOUND_SOURCE_FILES))
 COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
 
 %.cocci.patch: %.cocci $(COCCI_SOURCES)
-- 
2.32.0.599.g3967b4fa4ac


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/3] Makefile: "make tags" fixes & cleanup
  2021-06-22 14:21 [PATCH 0/3] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
                   ` (2 preceding siblings ...)
  2021-06-22 14:21 ` [PATCH 3/3] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
@ 2021-06-22 15:16 ` Taylor Blau
  2021-06-29  6:29 ` Junio C Hamano
  2021-06-29 11:12 ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 39+ messages in thread
From: Taylor Blau @ 2021-06-22 15:16 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost

On Tue, Jun 22, 2021 at 04:21:24PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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.

:-). Very nice.

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

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out
  2021-06-22 14:21 ` [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out Ævar Arnfjörð Bjarmason
@ 2021-06-22 19:25   ` Jeff King
  2021-06-22 19:49     ` Chris Torek
  2021-06-23 19:51     ` Felipe Contreras
  2021-06-23 19:25   ` Felipe Contreras
  1 sibling, 2 replies; 39+ messages in thread
From: Jeff King @ 2021-06-22 19:25 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost

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

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

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

> +cscope.out:
> +	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
> +
>  .PHONY: cscope
> -cscope:
> -	$(RM) cscope*
> -	$(FIND_SOURCE_FILES) | xargs cscope -b
> +cscope: cscope.out

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/3] Makefile: don't use "FORCE" for tags targets
  2021-06-22 14:21 ` [PATCH 3/3] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
@ 2021-06-22 19:28   ` Jeff King
  2021-06-23 19:49   ` Felipe Contreras
  1 sibling, 0 replies; 39+ messages in thread
From: Jeff King @ 2021-06-22 19:28 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost

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

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

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out
  2021-06-22 19:25   ` Jeff King
@ 2021-06-22 19:49     ` Chris Torek
  2021-06-23 19:54       ` Felipe Contreras
  2021-06-23 19:51     ` Felipe Contreras
  1 sibling, 1 reply; 39+ messages in thread
From: Chris Torek @ 2021-06-22 19:49 UTC (permalink / raw)
  To: Jeff King
  Cc: Ævar Arnfjörð Bjarmason, Git List, Junio C Hamano,
	Denton Liu, Felipe Contreras, Kristof Provost

On Tue, Jun 22, 2021 at 12:26 PM Jeff King <peff@peff.net> wrote:
> (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).

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out
  2021-06-22 14:21 ` [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out Ævar Arnfjörð Bjarmason
  2021-06-22 19:25   ` Jeff King
@ 2021-06-23 19:25   ` Felipe Contreras
  1 sibling, 0 replies; 39+ messages in thread
From: Felipe Contreras @ 2021-06-23 19:25 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Ævar Arnfjörð Bjarmason

Ævar Arnfjörð Bjarmason wrote:

> --- a/Makefile
> +++ b/Makefile
> @@ -2737,10 +2737,11 @@ tags: FORCE
>  	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
>  	mv tags+ tags
>  
> +cscope.out:
> +	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
> +
>  .PHONY: cscope
> -cscope:
> -	$(RM) cscope*
> -	$(FIND_SOURCE_FILES) | xargs cscope -b
> +cscope: cscope.out

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

Otherwise the pach looks good to me.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH 3/3] Makefile: don't use "FORCE" for tags targets
  2021-06-22 14:21 ` [PATCH 3/3] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
  2021-06-22 19:28   ` Jeff King
@ 2021-06-23 19:49   ` Felipe Contreras
  1 sibling, 0 replies; 39+ messages in thread
From: Felipe Contreras @ 2021-06-23 19:49 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Ævar Arnfjörð Bjarmason

Ævar Arnfjörð Bjarmason wrote:
> 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.

Very nice.

> I'm also removing the "-o" option from them,

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

Other than that looks good to me.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out
  2021-06-22 19:25   ` Jeff King
  2021-06-22 19:49     ` Chris Torek
@ 2021-06-23 19:51     ` Felipe Contreras
  1 sibling, 0 replies; 39+ messages in thread
From: Felipe Contreras @ 2021-06-23 19:51 UTC (permalink / raw)
  To: Jeff King, Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost

Jeff King 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 do. I don't use it regularly, as I find `git grep` sufficient most of
the time, but sometimes I do use cscope.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out
  2021-06-22 19:49     ` Chris Torek
@ 2021-06-23 19:54       ` Felipe Contreras
  0 siblings, 0 replies; 39+ messages in thread
From: Felipe Contreras @ 2021-06-23 19:54 UTC (permalink / raw)
  To: Chris Torek, Jeff King
  Cc: Ævar Arnfjörð Bjarmason, Git List, Junio C Hamano,
	Denton Liu, Felipe Contreras, Kristof Provost

Chris Torek wrote:
> On Tue, Jun 22, 2021 at 12:26 PM Jeff King <peff@peff.net> wrote:
> > (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).
> 
> I use cscope all the time, from vim.  If you use vim, you'll probably want
> cscope_maps.vim.

Or just set cscopetag.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/3] Makefile: "make tags" fixes & cleanup
  2021-06-22 14:21 [PATCH 0/3] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
                   ` (3 preceding siblings ...)
  2021-06-22 15:16 ` [PATCH 0/3] Makefile: "make tags" fixes & cleanup Taylor Blau
@ 2021-06-29  6:29 ` Junio C Hamano
  2021-06-29 12:07   ` Ævar Arnfjörð Bjarmason
  2021-06-29 11:12 ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
  5 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2021-06-29  6:29 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Denton Liu, Felipe Contreras, Kristof Provost

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

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

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.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 0/5] Makefile: "make tags" fixes & cleanup
  2021-06-22 14:21 [PATCH 0/3] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
                   ` (4 preceding siblings ...)
  2021-06-29  6:29 ` Junio C Hamano
@ 2021-06-29 11:12 ` Ævar Arnfjörð Bjarmason
  2021-06-29 11:12   ` [PATCH v2 1/5] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
                     ` (5 more replies)
  5 siblings, 6 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 11:12 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ævar Arnfjörð Bjarmason

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 1/5] Makefile: move ".PHONY: cscope" near its target
  2021-06-29 11:12 ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
@ 2021-06-29 11:12   ` Ævar Arnfjörð Bjarmason
  2021-06-29 11:12   ` [PATCH v2 2/5] Makefile: add QUIET_GEN to "cscope" target Ævar Arnfjörð Bjarmason
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 11:12 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index c3565fc0f8..4dd9711a65 100644
--- a/Makefile
+++ b/Makefile
@@ -2737,6 +2737,7 @@ tags: FORCE
 	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
 	mv tags+ tags
 
+.PHONY: cscope
 cscope:
 	$(RM) cscope*
 	$(FIND_SOURCE_FILES) | xargs cscope -b
@@ -3245,7 +3246,7 @@ endif
 
 .PHONY: all install profile-clean cocciclean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-.PHONY: FORCE cscope
+.PHONY: FORCE
 
 ### Check documentation
 #
-- 
2.32.0.613.g20d5ce26552


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 2/5] Makefile: add QUIET_GEN to "cscope" target
  2021-06-29 11:12 ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
  2021-06-29 11:12   ` [PATCH v2 1/5] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
@ 2021-06-29 11:12   ` Ævar Arnfjörð Bjarmason
  2021-06-29 11:12   ` [PATCH v2 3/5] Makefile: fix "cscope" target to refer to cscope.out Ævar Arnfjörð Bjarmason
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 11:12 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index 4dd9711a65..faa8900097 100644
--- a/Makefile
+++ b/Makefile
@@ -2739,7 +2739,7 @@ tags: FORCE
 
 .PHONY: cscope
 cscope:
-	$(RM) cscope*
+	$(QUIET_GEN)$(RM) cscope* && \
 	$(FIND_SOURCE_FILES) | xargs cscope -b
 
 ### Detect prefix changes
-- 
2.32.0.613.g20d5ce26552


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 3/5] Makefile: fix "cscope" target to refer to cscope.out
  2021-06-29 11:12 ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
  2021-06-29 11:12   ` [PATCH v2 1/5] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
  2021-06-29 11:12   ` [PATCH v2 2/5] Makefile: add QUIET_GEN to "cscope" target Ævar Arnfjörð Bjarmason
@ 2021-06-29 11:12   ` Ævar Arnfjörð Bjarmason
  2021-07-01 22:23     ` Jeff King
  2021-06-29 11:12   ` [PATCH v2 4/5] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
                     ` (2 subsequent siblings)
  5 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 11:12 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ævar Arnfjörð Bjarmason

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

diff --git a/.gitignore b/.gitignore
index 311841f9be..d74029c1ca 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 faa8900097..2e3b257164 100644
--- a/Makefile
+++ b/Makefile
@@ -2737,10 +2737,12 @@ tags: FORCE
 	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
 	mv tags+ tags
 
+cscope.out:
+	$(QUIET_GEN)$(RM) cscope.out && \
+	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
+
 .PHONY: cscope
-cscope:
-	$(QUIET_GEN)$(RM) cscope* && \
-	$(FIND_SOURCE_FILES) | xargs cscope -b
+cscope: cscope.out
 
 ### Detect prefix changes
 TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
@@ -3211,7 +3213,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
-- 
2.32.0.613.g20d5ce26552


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 4/5] Makefile: don't use "FORCE" for tags targets
  2021-06-29 11:12 ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
                     ` (2 preceding siblings ...)
  2021-06-29 11:12   ` [PATCH v2 3/5] Makefile: fix "cscope" target to refer to cscope.out Ævar Arnfjörð Bjarmason
@ 2021-06-29 11:12   ` Ævar Arnfjörð Bjarmason
  2021-06-30  0:05     ` Ramsay Jones
  2021-06-29 11:12   ` [PATCH v2 5/5] Makefile: normalize clobbering & xargs " Ævar Arnfjörð Bjarmason
  2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
  5 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 11:12 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index 2e3b257164..7b0d9773b0 100644
--- a/Makefile
+++ b/Makefile
@@ -2727,19 +2727,21 @@ FIND_SOURCE_FILES = ( \
 		| sed -e 's|^\./||' \
 	)
 
-$(ETAGS_TARGET): FORCE
+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
+tags: $(FOUND_SOURCE_FILES)
 	$(QUIET_GEN)$(RM) tags+ && \
 	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
 	mv tags+ tags
 
-cscope.out:
+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
 
 .PHONY: cscope
 cscope: cscope.out
@@ -2923,7 +2925,7 @@ check: config-list.h command-list.h
 		exit 1; \
 	fi
 
-FOUND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
+FOUND_C_SOURCES = $(filter %.c,$(FOUND_SOURCE_FILES))
 COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
 
 %.cocci.patch: %.cocci $(COCCI_SOURCES)
-- 
2.32.0.613.g20d5ce26552


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 5/5] Makefile: normalize clobbering & xargs for tags targets
  2021-06-29 11:12 ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
                     ` (3 preceding siblings ...)
  2021-06-29 11:12   ` [PATCH v2 4/5] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
@ 2021-06-29 11:12   ` Ævar Arnfjörð Bjarmason
  2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 11:12 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index 7b0d9773b0..926c9efe43 100644
--- a/Makefile
+++ b/Makefile
@@ -2730,18 +2730,19 @@ FIND_SOURCE_FILES = ( \
 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)"
+	$(QUIET_GEN)$(RM) $@+ && \
+	echo $(FOUND_SOURCE_FILES) | xargs etags -a -o $@+ && \
+	mv $@+ $@
 
 tags: $(FOUND_SOURCE_FILES)
-	$(QUIET_GEN)$(RM) tags+ && \
-	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
-	mv tags+ tags
+	$(QUIET_GEN)$(RM) $@+ && \
+	echo $(FOUND_SOURCE_FILES) | xargs ctags -a -o $@+ && \
+	mv $@+ $@
 
 cscope.out: $(FOUND_SOURCE_FILES)
-	$(QUIET_GEN)$(RM) cscope.out && \
-	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b
+	$(QUIET_GEN)$(RM) $@+ && \
+	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@+ -b && \
+	mv $@+ $@
 
 .PHONY: cscope
 cscope: cscope.out
-- 
2.32.0.613.g20d5ce26552


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/3] Makefile: "make tags" fixes & cleanup
  2021-06-29  6:29 ` Junio C Hamano
@ 2021-06-29 12:07   ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 12:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Denton Liu, Felipe Contreras, Kristof Provost


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

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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(-)
>
> 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.

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.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/5] Makefile: don't use "FORCE" for tags targets
  2021-06-29 11:12   ` [PATCH v2 4/5] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
@ 2021-06-30  0:05     ` Ramsay Jones
  0 siblings, 0 replies; 39+ messages in thread
From: Ramsay Jones @ 2021-06-30  0:05 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King



On 29/06/2021 12:12, Ævar Arnfjörð Bjarmason wrote:
> 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(-)
> 
> diff --git a/Makefile b/Makefile
> index 2e3b257164..7b0d9773b0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2727,19 +2727,21 @@ FIND_SOURCE_FILES = ( \
>  		| sed -e 's|^\./||' \
>  	)
>  
> -$(ETAGS_TARGET): FORCE
> +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
> +tags: $(FOUND_SOURCE_FILES)
>  	$(QUIET_GEN)$(RM) tags+ && \
>  	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
>  	mv tags+ tags

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

>  
> -cscope.out:
> +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

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

>  
>  .PHONY: cscope
>  cscope: cscope.out
> @@ -2923,7 +2925,7 @@ check: config-list.h command-list.h
>  		exit 1; \
>  	fi
>  
> -FOUND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
> +FOUND_C_SOURCES = $(filter %.c,$(FOUND_SOURCE_FILES))
>  COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
>  
>  %.cocci.patch: %.cocci $(COCCI_SOURCES)
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 3/5] Makefile: fix "cscope" target to refer to cscope.out
  2021-06-29 11:12   ` [PATCH v2 3/5] Makefile: fix "cscope" target to refer to cscope.out Ævar Arnfjörð Bjarmason
@ 2021-07-01 22:23     ` Jeff King
  2021-07-01 22:25       ` Jeff King
  0 siblings, 1 reply; 39+ messages in thread
From: Jeff King @ 2021-07-01 22:23 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras,
	Kristof Provost, Taylor Blau

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

> diff --git a/Makefile b/Makefile
> index faa8900097..2e3b257164 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2737,10 +2737,12 @@ tags: FORCE
>  	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
>  	mv tags+ tags
>  
> +cscope.out:
> +	$(QUIET_GEN)$(RM) cscope.out && \
> +	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
> +
>  .PHONY: cscope
> -cscope:
> -	$(QUIET_GEN)$(RM) cscope* && \
> -	$(FIND_SOURCE_FILES) | xargs cscope -b
> +cscope: cscope.out

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 3/5] Makefile: fix "cscope" target to refer to cscope.out
  2021-07-01 22:23     ` Jeff King
@ 2021-07-01 22:25       ` Jeff King
  0 siblings, 0 replies; 39+ messages in thread
From: Jeff King @ 2021-07-01 22:25 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras,
	Kristof Provost, Taylor Blau

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

> On Tue, Jun 29, 2021 at 01:12:57PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > diff --git a/Makefile b/Makefile
> > index faa8900097..2e3b257164 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2737,10 +2737,12 @@ tags: FORCE
> >  	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
> >  	mv tags+ tags
> >  
> > +cscope.out:
> > +	$(QUIET_GEN)$(RM) cscope.out && \
> > +	$(FIND_SOURCE_FILES) | xargs cscope -f$@ -b
> > +
> >  .PHONY: cscope
> > -cscope:
> > -	$(QUIET_GEN)$(RM) cscope* && \
> > -	$(FIND_SOURCE_FILES) | xargs cscope -b
> > +cscope: cscope.out
> 
> 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.

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup
  2021-06-29 11:12 ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
                     ` (4 preceding siblings ...)
  2021-06-29 11:12   ` [PATCH v2 5/5] Makefile: normalize clobbering & xargs " Ævar Arnfjörð Bjarmason
@ 2021-07-21 23:23   ` Ævar Arnfjörð Bjarmason
  2021-07-21 23:23     ` [PATCH v3 1/5] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
                       ` (6 more replies)
  5 siblings, 7 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 23:23 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ramsay Jones,
	Ævar Arnfjörð Bjarmason

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 $@+ && \
-- 
2.32.0.955.ge7c5360f7e7


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 1/5] Makefile: move ".PHONY: cscope" near its target
  2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
@ 2021-07-21 23:23     ` Ævar Arnfjörð Bjarmason
  2021-07-21 23:23     ` [PATCH v3 2/5] Makefile: add QUIET_GEN to "cscope" target Ævar Arnfjörð Bjarmason
                       ` (5 subsequent siblings)
  6 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 23:23 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ramsay Jones,
	Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index c7c46c017d3..7dd93ef4c3e 100644
--- a/Makefile
+++ b/Makefile
@@ -2749,6 +2749,7 @@ tags: FORCE
 	$(FIND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
 	mv tags+ tags
 
+.PHONY: cscope
 cscope:
 	$(RM) cscope*
 	$(FIND_SOURCE_FILES) | xargs cscope -b
@@ -3260,7 +3261,7 @@ endif
 
 .PHONY: all install profile-clean cocciclean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-.PHONY: FORCE cscope
+.PHONY: FORCE
 
 ### Check documentation
 #
-- 
2.32.0.955.ge7c5360f7e7


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 2/5] Makefile: add QUIET_GEN to "cscope" target
  2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
  2021-07-21 23:23     ` [PATCH v3 1/5] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
@ 2021-07-21 23:23     ` Ævar Arnfjörð Bjarmason
  2021-07-21 23:23     ` [PATCH v3 3/5] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
                       ` (4 subsequent siblings)
  6 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 23:23 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ramsay Jones,
	Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index 7dd93ef4c3e..69410095949 100644
--- a/Makefile
+++ b/Makefile
@@ -2751,7 +2751,7 @@ tags: FORCE
 
 .PHONY: cscope
 cscope:
-	$(RM) cscope*
+	$(QUIET_GEN)$(RM) cscope* && \
 	$(FIND_SOURCE_FILES) | xargs cscope -b
 
 ### Detect prefix changes
-- 
2.32.0.955.ge7c5360f7e7


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 3/5] Makefile: don't use "FORCE" for tags targets
  2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
  2021-07-21 23:23     ` [PATCH v3 1/5] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
  2021-07-21 23:23     ` [PATCH v3 2/5] Makefile: add QUIET_GEN to "cscope" target Ævar Arnfjörð Bjarmason
@ 2021-07-21 23:23     ` Ævar Arnfjörð Bjarmason
  2021-07-21 23:23     ` [PATCH v3 4/5] Makefile: the "cscope" target always creates a "cscope.out" Ævar Arnfjörð Bjarmason
                       ` (3 subsequent siblings)
  6 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 23:23 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ramsay Jones,
	Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index 69410095949..18895d94ffa 100644
--- a/Makefile
+++ b/Makefile
@@ -2739,20 +2739,24 @@ FIND_SOURCE_FILES = ( \
 		| sed -e 's|^\./||' \
 	)
 
-$(ETAGS_TARGET): FORCE
+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)+" && \
+	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+ && \
+	echo $(FOUND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
 	mv tags+ tags
 
-.PHONY: cscope
-cscope:
+cscope.out: $(FOUND_SOURCE_FILES)
 	$(QUIET_GEN)$(RM) cscope* && \
-	$(FIND_SOURCE_FILES) | xargs cscope -b
+	echo $(FOUND_SOURCE_FILES) | xargs cscope -b
+
+.PHONY: cscope
+cscope: cscope.out
 
 ### Detect prefix changes
 TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
@@ -2936,7 +2940,7 @@ check: config-list.h command-list.h
 		exit 1; \
 	fi
 
-FOUND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
+FOUND_C_SOURCES = $(filter %.c,$(FOUND_SOURCE_FILES))
 COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
 
 %.cocci.patch: %.cocci $(COCCI_SOURCES)
-- 
2.32.0.955.ge7c5360f7e7


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 4/5] Makefile: the "cscope" target always creates a "cscope.out"
  2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
                       ` (2 preceding siblings ...)
  2021-07-21 23:23     ` [PATCH v3 3/5] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
@ 2021-07-21 23:23     ` Ævar Arnfjörð Bjarmason
  2021-07-22 16:55       ` Junio C Hamano
  2021-07-21 23:23     ` [PATCH v3 5/5] Makefile: normalize clobbering & xargs for tags targets Ævar Arnfjörð Bjarmason
                       ` (2 subsequent siblings)
  6 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 23:23 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ramsay Jones,
	Ævar Arnfjörð Bjarmason

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 5/5] Makefile: normalize clobbering & xargs for tags targets
  2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
                       ` (3 preceding siblings ...)
  2021-07-21 23:23     ` [PATCH v3 4/5] Makefile: the "cscope" target always creates a "cscope.out" Ævar Arnfjörð Bjarmason
@ 2021-07-21 23:23     ` Ævar Arnfjörð Bjarmason
  2021-07-23 10:43       ` Jeff King
  2021-07-23 10:47     ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Jeff King
  2021-07-23 19:41     ` Felipe Contreras
  6 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 23:23 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ramsay Jones,
	Ævar Arnfjörð Bjarmason

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

diff --git a/Makefile b/Makefile
index 730ff23b923..295ac359cea 100644
--- a/Makefile
+++ b/Makefile
@@ -2742,18 +2742,19 @@ FIND_SOURCE_FILES = ( \
 FOUND_SOURCE_FILES = $(shell $(FIND_SOURCE_FILES))
 
 $(ETAGS_TARGET): $(FOUND_SOURCE_FILES)
-	$(QUIET_GEN)$(RM) "$(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 $@+ && \
+	mv $@+ $@
 
 tags: $(FOUND_SOURCE_FILES)
-	$(QUIET_GEN)$(RM) tags+ && \
-	echo $(FOUND_SOURCE_FILES) | xargs ctags -a -o tags+ && \
-	mv tags+ tags
+	$(QUIET_GEN)$(RM) $@+ && \
+	echo $(FOUND_SOURCE_FILES) | xargs ctags -a -o $@+ && \
+	mv $@+ $@
 
 cscope.out: $(FOUND_SOURCE_FILES)
-	$(QUIET_GEN)$(RM) cscope.out && \
-	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@ -b
+	$(QUIET_GEN)$(RM) $@+ && \
+	echo $(FOUND_SOURCE_FILES) | xargs cscope -f$@+ -b && \
+	mv $@+ $@
 
 .PHONY: cscope
 cscope: cscope.out
-- 
2.32.0.955.ge7c5360f7e7


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 4/5] Makefile: the "cscope" target always creates a "cscope.out"
  2021-07-21 23:23     ` [PATCH v3 4/5] Makefile: the "cscope" target always creates a "cscope.out" Ævar Arnfjörð Bjarmason
@ 2021-07-22 16:55       ` Junio C Hamano
  2021-07-22 17:58         ` Taylor Blau
  0 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2021-07-22 16:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Denton Liu, Felipe Contreras, Kristof Provost, Taylor Blau,
	Jeff King, Ramsay Jones

Æ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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 4/5] Makefile: the "cscope" target always creates a "cscope.out"
  2021-07-22 16:55       ` Junio C Hamano
@ 2021-07-22 17:58         ` Taylor Blau
  2021-07-22 18:50           ` Junio C Hamano
  0 siblings, 1 reply; 39+ messages in thread
From: Taylor Blau @ 2021-07-22 17:58 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Ævar Arnfjörð Bjarmason, git, Denton Liu,
	Felipe Contreras, Kristof Provost, Jeff King, Ramsay Jones

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 4/5] Makefile: the "cscope" target always creates a "cscope.out"
  2021-07-22 17:58         ` Taylor Blau
@ 2021-07-22 18:50           ` Junio C Hamano
  2021-07-22 21:20             ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2021-07-22 18:50 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Ævar Arnfjörð Bjarmason, git, Denton Liu,
	Felipe Contreras, Kristof Provost, Jeff King, Ramsay Jones

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 4/5] Makefile: the "cscope" target always creates a "cscope.out"
  2021-07-22 18:50           ` Junio C Hamano
@ 2021-07-22 21:20             ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-22 21:20 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, git, Denton Liu, Felipe Contreras, Kristof Provost,
	Jeff King, Ramsay Jones


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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 5/5] Makefile: normalize clobbering & xargs for tags targets
  2021-07-21 23:23     ` [PATCH v3 5/5] Makefile: normalize clobbering & xargs for tags targets Ævar Arnfjörð Bjarmason
@ 2021-07-23 10:43       ` Jeff King
  2021-07-23 10:47         ` Jeff King
  0 siblings, 1 reply; 39+ messages in thread
From: Jeff King @ 2021-07-23 10:43 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras,
	Kristof Provost, Taylor Blau, Ramsay Jones

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

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

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:

diff --git a/Makefile b/Makefile
index c7c46c017d..8a1ec00938 100644
--- a/Makefile
+++ b/Makefile
@@ -2751,7 +2751,7 @@ tags: FORCE
 
 cscope:
 	$(RM) cscope*
-	$(FIND_SOURCE_FILES) | xargs cscope -b
+	$(FIND_SOURCE_FILES) | xargs -n1 cscope -b
 
 ### Detect prefix changes
 TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\

then I get a file that is much smaller (1MB versus 9MB), and fails to
find lots of things:

  $ cscope -d -L1cmd_pack_objects
  [no output]

(by the way, I confused myself several times while testing this because
without "-d", it will actually rebuild the index using files in the
current directory. So something like " cscope -L1main" gives the same
result in both cases, or even if you don't have a cscope.out file at
all!)

But it really seems like cscope is not appending as we'd want. From
skimming the manpage, I think replacing xargs with "cscope -b -i -"
should work (and seems to for me).

-Peff

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 5/5] Makefile: normalize clobbering & xargs for tags targets
  2021-07-23 10:43       ` Jeff King
@ 2021-07-23 10:47         ` Jeff King
  0 siblings, 0 replies; 39+ messages in thread
From: Jeff King @ 2021-07-23 10:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras,
	Kristof Provost, Taylor Blau, Ramsay Jones

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

> On Thu, Jul 22, 2021 at 01:23:06AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > 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.
> 
> Hrm, that's not the experience I get with cscope. E.g.:

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup
  2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
                       ` (4 preceding siblings ...)
  2021-07-21 23:23     ` [PATCH v3 5/5] Makefile: normalize clobbering & xargs for tags targets Ævar Arnfjörð Bjarmason
@ 2021-07-23 10:47     ` Jeff King
  2021-07-23 15:25       ` Junio C Hamano
  2021-07-23 19:32       ` Felipe Contreras
  2021-07-23 19:41     ` Felipe Contreras
  6 siblings, 2 replies; 39+ messages in thread
From: Jeff King @ 2021-07-23 10:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras,
	Kristof Provost, Taylor Blau, Ramsay Jones

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

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

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup
  2021-07-23 10:47     ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Jeff King
@ 2021-07-23 15:25       ` Junio C Hamano
  2021-07-23 19:32       ` Felipe Contreras
  1 sibling, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2021-07-23 15:25 UTC (permalink / raw)
  To: Jeff King
  Cc: Ævar Arnfjörð Bjarmason, git, Denton Liu,
	Felipe Contreras, Kristof Provost, Taylor Blau, Ramsay Jones

Jeff King <peff@peff.net> writes:

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

Thanks, sorry and I agree with both counts.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup
  2021-07-23 10:47     ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Jeff King
  2021-07-23 15:25       ` Junio C Hamano
@ 2021-07-23 19:32       ` Felipe Contreras
  1 sibling, 0 replies; 39+ messages in thread
From: Felipe Contreras @ 2021-07-23 19:32 UTC (permalink / raw)
  To: Jeff King, Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Denton Liu, Felipe Contreras,
	Kristof Provost, Taylor Blau, Ramsay Jones

Jeff King wrote:
> On Thu, Jul 22, 2021 at 01:23:01AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > 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.
> 
> 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. :)

I agree with that as well.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 39+ messages in thread

* RE: [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup
  2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
                       ` (5 preceding siblings ...)
  2021-07-23 10:47     ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Jeff King
@ 2021-07-23 19:41     ` Felipe Contreras
  6 siblings, 0 replies; 39+ messages in thread
From: Felipe Contreras @ 2021-07-23 19:41 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Denton Liu, Felipe Contreras, Kristof Provost,
	Taylor Blau, Jeff King, Ramsay Jones,
	Ævar Arnfjörð Bjarmason

Ævar Arnfjörð Bjarmason wrote:
> 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.

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>

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2021-07-23 19:41 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22 14:21 [PATCH 0/3] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
2021-06-22 14:21 ` [PATCH 1/3] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
2021-06-22 14:21 ` [PATCH 2/3] Makefile: fix "cscope" target to refer to cscope.out Ævar Arnfjörð Bjarmason
2021-06-22 19:25   ` Jeff King
2021-06-22 19:49     ` Chris Torek
2021-06-23 19:54       ` Felipe Contreras
2021-06-23 19:51     ` Felipe Contreras
2021-06-23 19:25   ` Felipe Contreras
2021-06-22 14:21 ` [PATCH 3/3] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
2021-06-22 19:28   ` Jeff King
2021-06-23 19:49   ` Felipe Contreras
2021-06-22 15:16 ` [PATCH 0/3] Makefile: "make tags" fixes & cleanup Taylor Blau
2021-06-29  6:29 ` Junio C Hamano
2021-06-29 12:07   ` Ævar Arnfjörð Bjarmason
2021-06-29 11:12 ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
2021-06-29 11:12   ` [PATCH v2 1/5] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
2021-06-29 11:12   ` [PATCH v2 2/5] Makefile: add QUIET_GEN to "cscope" target Ævar Arnfjörð Bjarmason
2021-06-29 11:12   ` [PATCH v2 3/5] Makefile: fix "cscope" target to refer to cscope.out Ævar Arnfjörð Bjarmason
2021-07-01 22:23     ` Jeff King
2021-07-01 22:25       ` Jeff King
2021-06-29 11:12   ` [PATCH v2 4/5] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
2021-06-30  0:05     ` Ramsay Jones
2021-06-29 11:12   ` [PATCH v2 5/5] Makefile: normalize clobbering & xargs " Ævar Arnfjörð Bjarmason
2021-07-21 23:23   ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Ævar Arnfjörð Bjarmason
2021-07-21 23:23     ` [PATCH v3 1/5] Makefile: move ".PHONY: cscope" near its target Ævar Arnfjörð Bjarmason
2021-07-21 23:23     ` [PATCH v3 2/5] Makefile: add QUIET_GEN to "cscope" target Ævar Arnfjörð Bjarmason
2021-07-21 23:23     ` [PATCH v3 3/5] Makefile: don't use "FORCE" for tags targets Ævar Arnfjörð Bjarmason
2021-07-21 23:23     ` [PATCH v3 4/5] Makefile: the "cscope" target always creates a "cscope.out" Ævar Arnfjörð Bjarmason
2021-07-22 16:55       ` Junio C Hamano
2021-07-22 17:58         ` Taylor Blau
2021-07-22 18:50           ` Junio C Hamano
2021-07-22 21:20             ` Ævar Arnfjörð Bjarmason
2021-07-21 23:23     ` [PATCH v3 5/5] Makefile: normalize clobbering & xargs for tags targets Ævar Arnfjörð Bjarmason
2021-07-23 10:43       ` Jeff King
2021-07-23 10:47         ` Jeff King
2021-07-23 10:47     ` [PATCH v3 0/5] Makefile: "make tags" fixes & cleanup Jeff King
2021-07-23 15:25       ` Junio C Hamano
2021-07-23 19:32       ` Felipe Contreras
2021-07-23 19:41     ` Felipe Contreras

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git