threads / patch / 49247

patchdoc: Don't echo sed command for manpage-base-url.xsl

Subject: [PATCH] doc: Don't echo sed command for manpage-base-url.xsl

## tl;dr

9 messages between Aug 28, 2018 and Aug 29, 2018. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Tim Schumacher· Aug 28, 2018, 21:21 UTC · lore

Previously, the sed command for generating manpage-base-url.xsl was printed to the console when being run.

For the purpose of silencing it, define a $(QUIET) variable which contains an '@' if verbose mode isn't enabled and which is empty otherwise. This just silences the command invocation without doing anything else.

Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
 Documentation/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Documentation/Makefile +2 −1
diff --git a/Documentation/Makefile b/Documentation/Makefile
index a42dcfc74..45454e9b5 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -217,6 +217,7 @@ endif
 
 ifneq ($(findstring $(MAKEFLAGS),s),s)
 ifndef V
+	QUIET		= @
 	QUIET_ASCIIDOC	= @echo '   ' ASCIIDOC $@;
 	QUIET_XMLTO	= @echo '   ' XMLTO $@;
 	QUIET_DB2TEXI	= @echo '   ' DB2TEXI $@;
@@ -344,7 +345,7 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
 	mv $@+ $@
 
 manpage-base-url.xsl: manpage-base-url.xsl.in
-	sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
+	$(QUIET)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
 %.1 %.5 %.7 : %.xml manpage-base-url.xsl
 	$(QUIET_XMLTO)$(RM) $@ && \

-- 
2.19.0.rc0
Eric Sunshine· Aug 28, 2018, 21:25 UTC · re: Tim Schumacher · lore

Re: [PATCH] doc: Don't echo sed command for manpage-base-url.xsl

On Tue, Aug 28, 2018 at 5:21 PM Tim Schumacher <timschumi@gmx.de> wrote:
Show 7 quoted lines
> Previously, the sed command for generating manpage-base-url.xsl
> was printed to the console when being run.
>
> For the purpose of silencing it, define a $(QUIET) variable which
> contains an '@' if verbose mode isn't enabled and which is empty
> otherwise. This just silences the command invocation without doing
> anything else.

One thing that is missing from this commit message is the explanation of _why_ this is desirable. And, why only this command? Without understanding the underlying reason(s), it's hard to judge the value of this change.

Thanks.
Junio C Hamano· Aug 28, 2018, 21:35 UTC · re: Tim Schumacher · lore

Re: [PATCH] doc: Don't echo sed command for manpage-base-url.xsl

Tim Schumacher <timschumi@gmx.de> writes:
Show 10 quoted lines
> Previously, the sed command for generating manpage-base-url.xsl
> was printed to the console when being run.
>
> For the purpose of silencing it, define a $(QUIET) variable which
> contains an '@' if verbose mode isn't enabled and which is empty
> otherwise. This just silences the command invocation without doing
> anything else.
>
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
> ---

I am not sure if this is a good change. All these QUIET_$TOOL hide details of running the $TOOL to produce the final output of the step, but they still do report what they are creating via which $TOOL.

Shouldn't the step to create manpage-base-url.xsl be the same? The detail of creating it (i.e. token @@MAN_BASE_URL@@ is replaced with the actual value) may want to be squelched, but shouldn't we still be reporting that we are creating manpage-base-url.xsl file?

Show 24 quoted lines
>  Documentation/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index a42dcfc74..45454e9b5 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -217,6 +217,7 @@ endif
>  
>  ifneq ($(findstring $(MAKEFLAGS),s),s)
>  ifndef V
> +	QUIET		= @
>  	QUIET_ASCIIDOC	= @echo '   ' ASCIIDOC $@;
>  	QUIET_XMLTO	= @echo '   ' XMLTO $@;
>  	QUIET_DB2TEXI	= @echo '   ' DB2TEXI $@;
> @@ -344,7 +345,7 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
>  	mv $@+ $@
>  
>  manpage-base-url.xsl: manpage-base-url.xsl.in
> -	sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
> +	$(QUIET)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
>  
>  %.1 %.5 %.7 : %.xml manpage-base-url.xsl
>  	$(QUIET_XMLTO)$(RM) $@ && \
Tim Schumacher· Aug 29, 2018, 13:43 UTC · re: Tim Schumacher · lore

[PATCH v2] doc: Don't echo sed command for manpage-base-url.xsl

Previously, the sed command for generating manpage-base-url.xsl was printed to the console when being run.

Make the console output for this rule similiar to all the other rules by printing a short status message instead of the whole command.

Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---

To Junio C Hamano: The rule does now print "SED manpage-base-url.xsl" to the console, which is similiar to other QUIET_$TOOL rules.

To Eric Sunshine: I reworded the commit message to focus more on _why_ the patch is relevant.

---
 Documentation/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Documentation/Makefile +2 −1
diff --git a/Documentation/Makefile b/Documentation/Makefile
index a42dcfc74..cbf33c5a7 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -225,6 +225,7 @@ ifndef V
 	QUIET_XSLTPROC	= @echo '   ' XSLTPROC $@;
 	QUIET_GEN	= @echo '   ' GEN $@;
 	QUIET_LINT	= @echo '   ' LINT $@;
+	QUIET_SED	= @echo '   ' SED $@;
 	QUIET_STDERR	= 2> /dev/null
 	QUIET_SUBDIR0	= +@subdir=
 	QUIET_SUBDIR1	= ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
@@ -344,7 +345,7 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
 	mv $@+ $@
 
 manpage-base-url.xsl: manpage-base-url.xsl.in
-	sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
+	$(QUIET_SED)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
 %.1 %.5 %.7 : %.xml manpage-base-url.xsl
 	$(QUIET_XMLTO)$(RM) $@ && \

-- 
2.19.0.rc1.1.g093671f86
Tim Schumacher· Aug 29, 2018, 15:47 UTC · re: Tim Schumacher · lore

[PATCH v3] doc: Don't echo sed command for manpage-base-url.xsl

Previously, the sed command for generating manpage-base-url.xsl was printed to the console when being run.

Make the console output for this rule similiar to all the other rules by printing a short status message instead of the whole command.

Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
 Documentation/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/Makefile +1 −1
diff --git a/Documentation/Makefile b/Documentation/Makefile
index a42dcfc74..95f6a321f 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -344,7 +344,7 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
 	mv $@+ $@
 
 manpage-base-url.xsl: manpage-base-url.xsl.in
-	sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
+	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
 %.1 %.5 %.7 : %.xml manpage-base-url.xsl
 	$(QUIET_XMLTO)$(RM) $@ && \

-- 
2.19.0.rc1.1.g093671f86
Jonathan Nieder· Aug 29, 2018, 16:55 UTC · re: Tim Schumacher · lore

Re: [PATCH v3] doc: Don't echo sed command for manpage-base-url.xsl

Tim Schumacher wrote:
> Subject: doc: Don't echo sed command for manpage-base-url.xsl
Cribbing from my review of v2: a description like
	Documentation/Makefile: make manpage-base-url.xsl generation quieter

would make it more obvious what this does when viewed in "git log --oneline".

Show 11 quoted lines
> Previously, the sed command for generating manpage-base-url.xsl
> was printed to the console when being run.
>
> Make the console output for this rule similiar to all the
> other rules by printing a short status message instead of
> the whole command.
>
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
> ---
>  Documentation/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Oh! Ignore my reply to v2; looks like you anticipated what I was going to suggest already. For next time, if you include a note about what changed between versions after the --- delimiter, that can help save some time.

With or without the suggested commit message tweak,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thank you.
Tim Schumacher· Aug 29, 2018, 17:42 UTC · re: Jonathan Nieder · lore

Re: [PATCH v3] doc: Don't echo sed command for manpage-base-url.xsl

On 29.08.18 18:55, Jonathan Nieder wrote:
Show 10 quoted lines
> Tim Schumacher wrote:
> 
>> Subject: doc: Don't echo sed command for manpage-base-url.xsl
> 
> Cribbing from my review of v2: a description like
> 
> 	Documentation/Makefile: make manpage-base-url.xsl generation quieter
> 
> would make it more obvious what this does when viewed in "git log
> --oneline".

imho, the "Documentation/Makefile" is a bit too long (about 1/3 of the first line). Would it be advisable to keep the previous prefix of "doc" (which would shorten down the line from 68 characters down to 49) and to use only the second part of your proposed first line? Depending on the response I would address this in v4 of this patch.

Show 18 quoted lines
> 
>> Previously, the sed command for generating manpage-base-url.xsl
>> was printed to the console when being run.
>>
>> Make the console output for this rule similiar to all the
>> other rules by printing a short status message instead of
>> the whole command.
>>
>> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
>> ---
>>   Documentation/Makefile | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Oh!  Ignore my reply to v2; looks like you anticipated what I was
> going to suggest already.  For next time, if you include a note about
> what changed between versions after the --- delimiter, that can help
> save some time.
>

The change to QUIET_GEN was proposed by Junio, but that E-Mail wasn't CC'ed to the mailing list, probably due to him typing the response on a phone.

I originally included a note about the change as well, but I forgot to copy it over to the new patch after I generated a second version of v3.

Show 6 quoted lines
> With or without the suggested commit message tweak,
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> Thank you.
> 
Thanks for the review!
Junio C Hamano· Aug 29, 2018, 19:19 UTC · re: Jonathan Nieder · lore

Re: [PATCH v3] doc: Don't echo sed command for manpage-base-url.xsl

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 10 quoted lines
> Tim Schumacher wrote:
>
>> Subject: doc: Don't echo sed command for manpage-base-url.xsl
>
> Cribbing from my review of v2: a description like
>
> 	Documentation/Makefile: make manpage-base-url.xsl generation quieter
>
> would make it more obvious what this does when viewed in "git log
> --oneline".
Sounds good; let's take it.
>> Previously, the sed command for generating manpage-base-url.xsl
>> was printed to the console when being run.

The convention is that we talk about the state before the current series in question is applied in the present tense, so "previously" is not needed. Perhaps

    The exact sed command to generate manpage-base-url.xsl appears in
    the output, unlike the rules for other files that by default only
    show summary.

is sufficient. The output is not always going to "the console", and it is not like we change behaviour depending on where the output is going, so it is misleading to say "the console" (iow, the phrase "to the console" has negative information density in the above sentence).

>> Make the console output for this rule similiar to all the
>> other rules by printing a short status message instead of
>> the whole command.
Likewise, s/console //;
I'll all do the above tweaks while queueing.
Thanks, both.
Show 16 quoted lines
>>
>> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
>> ---
>>  Documentation/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Oh!  Ignore my reply to v2; looks like you anticipated what I was
> going to suggest already.  For next time, if you include a note about
> what changed between versions after the --- delimiter, that can help
> save some time.
>
> With or without the suggested commit message tweak,
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thank you.
Jonathan Nieder· Aug 29, 2018, 16:49 UTC · re: Tim Schumacher · lore

Re: [PATCH v2] doc: Don't echo sed command for manpage-base-url.xsl

Tim Schumacher wrote:
> Subject: doc: Don't echo sed command for manpage-base-url.xsl

At first glance, I thought this was going to change the rendered documentation in some way. Maybe this should say

	Makefile: make 'sed' commands quieter

? That led me to look in the Makefile, where it appears we already do this for some sed commands, using QUIET_GEN. What would you think of using the same for manpage-base-url.xsl?

Thanks, Jonathan

← back to recent threads