From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: Elia Pinto <gitter.spiros@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] Makefile: add missing phony target
Date: Thu, 10 Dec 2015 19:37:09 +0100 [thread overview]
Message-ID: <vpqio46b162.fsf@anie.imag.fr> (raw)
In-Reply-To: <1449761094-37915-1-git-send-email-gitter.spiros@gmail.com> (Elia Pinto's message of "Thu, 10 Dec 2015 15:24:54 +0000")
Elia Pinto <gitter.spiros@gmail•com> writes:
> Also put the .PHONY
> declaration immediately before the target declaration, where necessary,
> for a better readability and a uniform style.
[...]
> --- a/Makefile
> +++ b/Makefile
> @@ -522,11 +522,11 @@ SCRIPT_PYTHON_INS = $(filter-out $(NO_INSTALL),$(SCRIPT_PYTHON_GEN))
> # "make -C ../.. SCRIPT_PERL=contrib/foo/bar.perl build-perl-script"
> # from subdirectories like contrib/*/
> .PHONY: build-perl-script build-sh-script build-python-script
> +.PHONY: install-perl-script install-sh-script install-python-script
> build-perl-script: $(SCRIPT_PERL_GEN)
> build-sh-script: $(SCRIPT_SH_GEN)
> build-python-script: $(SCRIPT_PYTHON_GEN)
>
> -.PHONY: install-perl-script install-sh-script install-python-script
> install-sh-script: $(SCRIPT_SH_INS)
> $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> install-perl-script: $(SCRIPT_PERL_INS)
Isn't this hunk doing exactly the opposite of what the commit message
says? install-sh-script's .PHONY used to be right before the target, and
it's now in the paragraph above.
> @@ -534,7 +534,7 @@ install-perl-script: $(SCRIPT_PERL_INS)
> install-python-script: $(SCRIPT_PYTHON_INS)
> $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>
> -.PHONY: clean-perl-script clean-sh-script clean-python-script
> +.PHONY: clean-sh-script clean-perl-script clean-python-script
double-space befroe clean-python-script.
> @@ -546,7 +546,6 @@ SCRIPTS = $(SCRIPT_SH_INS) \
> $(SCRIPT_PERL_INS) \
> $(SCRIPT_PYTHON_INS) \
> git-instaweb
> -
> ETAGS_TARGET = TAGS
Is this needed/good?
> @@ -1792,7 +1794,7 @@ GIT-PERL-DEFINES: FORCE
> fi
>
>
> -.PHONY: gitweb
> +.PHONY: gitweb git-instaweb
No: git-instaweb is not a .PHONY target, it's a real file generated from
git-instaweb.sh.
This bug in your patch is hard to spot in its current form where you mix
code movement and subtle changes. The patch cannot be reviewed in good
condition as-is.
> +.PHONY: tags cscope FORCE
> tags: FORCE
> $(RM) tags
> $(FIND_SOURCE_FILES) | xargs ctags -a
If tags depends on FORCE, it doesn't need to be .PHONY. I strongly
prefer the old style, since tags is actually a file, and I prefer using
.PHONY for targets that are not meant to generate files, and depend on
FORCE where the rule actually generate a file named after its target,
but needs to be re-ran every time it's called.
If you disagree with this, then you need to justify the change in the
commit message.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
prev parent reply other threads:[~2015-12-10 18:37 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-10 15:24 [PATCH] Makefile: add missing phony target Elia Pinto
2015-12-10 18:37 ` Matthieu Moy [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vpqio46b162.fsf@anie.imag.fr \
--to=matthieu.moy@grenoble-inp$(echo .)fr \
--cc=git@vger$(echo .)kernel.org \
--cc=gitter.spiros@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox