patch, 3 partsMakefile: mark "check" target as .PHONY

I'm re-rolling ab/config-based-hooks-base to deal with (among other things) a vcbuild failure. This doesn't fix that, but allows me to fix that in one place, instead copy/pasting things all over the place.

The reason I'd need to do that is because we define GENERATED_H, but then proceed not to use it when we should. This small series fixes that in 2/3, and fixes a couple of adjacent trivial issues in 1/3 and 3/3.

Ævar Arnfjörð Bjarmason (3):
  Makefile: mark "check" target as .PHONY
  Makefile: stop hardcoding {command,config}-list.h
  Makefile: remove an out-of-date comment

 Makefile              | 13 +++++--------
 compat/vcbuild/README |  2 +-
 config.mak.uname      |  6 +++---
 3 files changed, 9 insertions(+), 12 deletions(-)

Fix a bug in 44c9e8594e (Fix up header file dependencies and add sparse checking rules, 2005-07-03), we never marked the phony "check" target as such.

Perhaps we should just remove it, since as of a combination of 912f9980d2 (Makefile: help people who run 'make check' by mistake, 2008-11-11) 0bcd9ae85d (sparse: Fix errors due to missing target-specific variables, 2011-04-21) we've been suggesting the user run "make sparse" directly.

But under that mode it still does something, as well as directing the user to run "make test" under non-sparse. So let's punt that and narrowly fix the PHONY bug.

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

Ævar Arnfjörð Bjarmason wrote:

Obviously correct.

This comment added in dfea575017 (Makefile: lazily compute header dependencies, 2010-01-26) has been out of date since 92b88eba9f (Makefile: use `git ls-files` to list header files, if possible, 2019-03-04), when we did exactly what it tells us not to do and added $(GENERATED_H) to $(OBJECTS) dependencies.

The rest of it was also somewhere between inaccurate and outdated, since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20) it's not followed by a list of header files, that got moved earlier in the file into LIB_H in b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20).

Let's just remove it entirely, to the extent that we have anything useful to say here the comment on the "USE_COMPUTED_HEADER_DEPENDENCIES" variable a few lines above this change does the job for us.

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

Ævar Arnfjörð Bjarmason wrote:

Very true.

I don't see the point in mentioning b8ba629264 twice, perhaps you meant:

60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H into LIB_H, 2012-07-06)

This commit also removed part of the comment:

  # XXX. Please check occasionally that these include all dependencies
  # gcc detects!

I have tried to understand what was the purpose of that comment in the past, but I don't get it, it says:

  Dependencies on automatically generated headers such as command-list.h
  should _not_ be included here, since they are necessary even when
  building an object for the first time.

Why would it matter if we are building the object for the first time, or rebuilding it? If we need command-list.h to build help.o once, we need it always. What am I missing?

It seems to me this comment never made sense.

Can we remove that extra space once we are at it?

The change itself looks good to me.

Cheers.

Change various places that hardcode the names of these two files to refer to either $(GENERATED_H), or to a new generated-hdrs target. That target is consistent with the *-objs targets I recently added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23).

I have not tested the Windows-specific change in config.mak.uname being made here, but we use other variables from the Makefile in the same block, and the GENERATED_H is fully defined before we include config.mak.uname.

Hardcoding command-list.h there seems to have been a case of copy/paste programming in dce7d29551 (msvc: support building Git using MS Visual C++, 2019-06-25). The config-list.h was added later in 709df95b78 (help: move list_config_help to builtin/help, 2020-04-16).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile              | 6 ++++--
 compat/vcbuild/README | 2 +-
 config.mak.uname      | 6 +++---
 3 files changed, 8 insertions(+), 6 deletions(-)

Ævar Arnfjörð Bjarmason wrote:

Avoiding hard-coded things is generally a good idea, and I can smell there's an advantage nearby, but it's not stated.

Can you spell out what you are trying to achieve?

Actually that's not the commit, it's this one:

976aaedca0 (msvc: add a Makefile target to pre-generate the Visual Studio solution, 2019-07-29)

The changes themselves look good to me.

On Thu, Jun 17 2021, Felipe Contreras wrote:

It's hinted at in the CL, but this is series 1/3 of a re-roll of the base topic for config-based hooks, real use of this is made in step 2/3, 3/3 has a better overview: http://lore.kernel.org/git/cover-00.27-0000000000-20210617T101216Z-avarab@gmail.com

Thanks, I had both in my buffers somewhere and copied over the wrong one. Will correct pending further feedback...

Ævar Arnfjörð Bjarmason wrote:

Yes, but the commit message should stand on its own.

Yeah, I read the cover letter afterwards, but I'm just putting my reviewer cap; the rationale belongs in the commit message: we will want more generated files in GENERATED_H.

Also, I presume you meant this one:

https://lore.kernel.org/git/cover-0.3-0000000000-20210617T100239Z-avarab@gmail.com/

Cheers.

On Thu, Jun 17 2021, Felipe Contreras wrote:

Thanks, I'll reword it. I initially figured in this case it was better not to distract from a stand-alone change with what inspired it, but will do.

That's 2/3, but yes. That comes after this one. I meant to link to 3/3 to give the general overview of how this relates to that re-rolled topic at large.

A base topic for some larger changes. See the v1 CL for a summary: http://lore.kernel.org/git/cover-0.3-0000000000-20210617T095827Z-avarab@gmail.com

The only changes since v1 are to commit message issues pointed out by Felipe and a trivial whitespace change. I also updated the commit message of 2/3 as he suggested to point out why the change is being made.

Ævar Arnfjörð Bjarmason (3):
  Makefile: mark "check" target as .PHONY
  Makefile: stop hardcoding {command,config}-list.h
  Makefile: remove an out-of-date comment

 Makefile              | 14 +++++---------
 compat/vcbuild/README |  2 +-
 config.mak.uname      |  6 +++---
 3 files changed, 9 insertions(+), 13 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  27c94247f8 Makefile: mark "check" target as .PHONY
1:  6e164edb0b ! 2:  983d072d52 Makefile: stop hardcoding {command,config}-list.h
    @@ Commit message
         added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs
         & objects targets, 2021-02-23).
     
    +    A follow-up commit (not part of this series) will add a new generated
    +    hook-list.h. By doing this refactoring we'll only need to add the new
    +    file to the GENERATED_H variable, not EXCEPT_HDRS, the vcbuild/README
    +    etc.
    +
         I have not tested the Windows-specific change in config.mak.uname
         being made here, but we use other variables from the Makefile in the
         same block, and the GENERATED_H is fully defined before we include
         config.mak.uname.
     
         Hardcoding command-list.h there seems to have been a case of
    -    copy/paste programming in dce7d29551 (msvc: support building Git using
    -    MS Visual C++, 2019-06-25). The config-list.h was added later in
    -    709df95b78 (help: move list_config_help to builtin/help, 2020-04-16).
    +    copy/paste programming in 976aaedca0 (msvc: add a Makefile target to
    +    pre-generate the Visual Studio solution, 2019-07-29). The
    +    config-list.h was added later in 709df95b78 (help: move
    +    list_config_help to builtin/help, 2020-04-16).
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
2:  ddae86802e ! 3:  44a4781218 Makefile: remove an out-of-date comment
    @@ Commit message
         The rest of it was also somewhere between inaccurate and outdated,
         since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20)
         it's not followed by a list of header files, that got moved earlier in
    -    the file into LIB_H in b8ba629264 (Makefile: fold MISC_H into LIB_H,
    -    2012-06-20).
    +    the file into LIB_H in 60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H
    +    into LIB_H, 2012-07-06).
     
         Let's just remove it entirely, to the extent that we have anything
         useful to say here the comment on the
    @@ Makefile: ifneq ($(dep_files_present),)
     -# Dependencies on automatically generated headers such as command-list.h
     -# should _not_ be included here, since they are necessary even when
     -# building an object for the first time.
    - 
    +-
      $(OBJECTS): $(LIB_H) $(GENERATED_H)
      endif
    + 

Fix a bug in 44c9e8594e (Fix up header file dependencies and add sparse checking rules, 2005-07-03), we never marked the phony "check" target as such.

Perhaps we should just remove it, since as of a combination of 912f9980d2 (Makefile: help people who run 'make check' by mistake, 2008-11-11) 0bcd9ae85d (sparse: Fix errors due to missing target-specific variables, 2011-04-21) we've been suggesting the user run "make sparse" directly.

But under that mode it still does something, as well as directing the user to run "make test" under non-sparse. So let's punt that and narrowly fix the PHONY bug.

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

Change various places that hardcode the names of these two files to refer to either $(GENERATED_H), or to a new generated-hdrs target. That target is consistent with the *-objs targets I recently added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23).

A follow-up commit (not part of this series) will add a new generated hook-list.h. By doing this refactoring we'll only need to add the new file to the GENERATED_H variable, not EXCEPT_HDRS, the vcbuild/README etc.

I have not tested the Windows-specific change in config.mak.uname being made here, but we use other variables from the Makefile in the same block, and the GENERATED_H is fully defined before we include config.mak.uname.

Hardcoding command-list.h there seems to have been a case of copy/paste programming in 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual Studio solution, 2019-07-29). The config-list.h was added later in 709df95b78 (help: move list_config_help to builtin/help, 2020-04-16).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile              | 6 ++++--
 compat/vcbuild/README | 2 +-
 config.mak.uname      | 6 +++---
 3 files changed, 8 insertions(+), 6 deletions(-)

This comment added in dfea575017 (Makefile: lazily compute header dependencies, 2010-01-26) has been out of date since 92b88eba9f (Makefile: use `git ls-files` to list header files, if possible, 2019-03-04), when we did exactly what it tells us not to do and added $(GENERATED_H) to $(OBJECTS) dependencies.

The rest of it was also somewhere between inaccurate and outdated, since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20) it's not followed by a list of header files, that got moved earlier in the file into LIB_H in 60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H into LIB_H, 2012-07-06).

Let's just remove it entirely, to the extent that we have anything useful to say here the comment on the "USE_COMPUTED_HEADER_DEPENDENCIES" variable a few lines above this change does the job for us.

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

Ævar Arnfjörð Bjarmason wrote:

This version looks good to me:

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