From: Felipe Contreras <hidden> Date: 2021-05-21 22:29:51
Commits 50cff52f1a (When generating manpages, delete outdated targets
first., 2007-08-02) and f9286765b2 (Documentation/Makefile: remove
cmd-list.made before redirecting to it., 2007-08-06) created these rm
instances for a very rare corner-case: building as root by mistake.
It's odd to have workarounds here, but nowhere else in the Makefile--
which already fails in this stuation, starting from
Documentation/technical/.
We gain nothing but complexity, so let's remove them.
Comments-by: Jeff King [off-list ref]
Signed-off-by: Felipe Contreras <redacted>
---
Documentation/Makefile | 42 +++++++++++++++---------------------------
1 file changed, 15 insertions(+), 27 deletions(-)
From: Felipe Contreras <hidden> Date: 2021-05-21 22:29:56
Currently GNU make already removes files when catching an interruption
signal, however, in order to deal with other kinds of errors a
workaround is in place to store target output to a temporary file, and
only move it to its right place on success.
By enabling the built-in .DELETE_ON_ERROR we let make do this task, so
we don't have to.
This way the rules can be simplified a lot.
Suggested-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Felipe Contreras <redacted>
---
Documentation/Makefile | 47 +++++++++++++++++-------------------------
1 file changed, 19 insertions(+), 28 deletions(-)
From: Junio C Hamano <hidden> Date: 2021-05-24 17:51:57
Felipe Contreras [off-list ref] writes:
Commits 50cff52f1a (When generating manpages, delete outdated targets
first., 2007-08-02) and f9286765b2 (Documentation/Makefile: remove
cmd-list.made before redirecting to it., 2007-08-06) created these rm
instances for a very rare corner-case: building as root by mistake.
It's odd to have workarounds here, but nowhere else in the Makefile--
which already fails in this stuation, starting from
Documentation/technical/.
We gain nothing but complexity, so let's remove them.
Comments-by: Jeff King [off-list ref]
This is curious. I understand what other kinds of trailers like
Helped-by and Suggested-by people sometimes use mean, but this one
does not tell anything other than that this person had some comments
on an earlier rounds, does not tell us what kind of comments they
were and if something was done to address those comments or the
comments were totally ignored or what.
The patch text itself, and the justification given in the second
paragraph, both make perfect sense to me.
From: Junio C Hamano <hidden> Date: 2021-05-24 17:52:01
Felipe Contreras [off-list ref] writes:
This patch series is an attempt to cleanup the Makefile of the documentation.
Since v2 only certain $(RM) commands are moved from one patch to another.
The above description is correct. I can see that the end result is
identical to the previous one.
Is this satisfactory to everybody (I haven't taken a look beyond
comparing the end results)?
It does not seem to play well with what brian is already doing in
'seen'; it overlaps with its changes in interesting ways. Is this
series meant to be a superset of the topic and supersedes it? How
are you two coordinating to avoid duplicated work and stepping on
each other's toes?
Thanks.
From: Felipe Contreras <hidden> Date: 2021-05-24 18:40:36
Junio C Hamano wrote:
Felipe Contreras [off-list ref] writes:
quoted
Commits 50cff52f1a (When generating manpages, delete outdated targets
first., 2007-08-02) and f9286765b2 (Documentation/Makefile: remove
cmd-list.made before redirecting to it., 2007-08-06) created these rm
instances for a very rare corner-case: building as root by mistake.
It's odd to have workarounds here, but nowhere else in the Makefile--
which already fails in this stuation, starting from
Documentation/technical/.
We gain nothing but complexity, so let's remove them.
Comments-by: Jeff King [off-list ref]
This is curious. I understand what other kinds of trailers like
Helped-by and Suggested-by people sometimes use mean, but this one
does not tell anything other than that this person had some comments
on an earlier rounds, does not tell us what kind of comments they
were and if something was done to address those comments or the
comments were totally ignored or what.
If they were ignored they would not be part of the commit. It is
understood the comments were incorporated.
When you see 'Tested-by' do you wonder what was the result of the test?
I presume most people would understand that the test succeeded, and
that's why the trailer line was added.
I happen to have at hand a script that ranks the most common trailer
lines:
* acked-by: 25% (1945)
Doesn't apply.
* reviewed-by: 22% (1729)
This does apply, but I'm not confident Jeff would like me to add that.
* helped-by: 17% (1336)
* reported-by: 12% (960)
* mentored-by: 5% (379)
* suggested-by: 4% (281)
* cc: 3% (222)
* noticed-by: 2% (165)
* tested-by: 2% (153)
* improved-by: 1% (88)
* thanks-to: 1% (65)
* signed-off-by: 1% (50)
* based-on-patch-by: 1% (50)
* contributions-by: 1% (43)
* co-authored-by: 1% (41)
I don't think any of these apply.
Cheers.
--
Felipe Contreras
From: Felipe Contreras <hidden> Date: 2021-05-24 18:45:37
Felipe Contreras wrote:
Junio C Hamano wrote:
quoted
Felipe Contreras [off-list ref] writes:
quoted
quoted
Comments-by: Jeff King [off-list ref]
This is curious. I understand what other kinds of trailers like
Helped-by and Suggested-by people sometimes use mean, but this one
does not tell anything other than that this person had some comments
on an earlier rounds, does not tell us what kind of comments they
were and if something was done to address those comments or the
comments were totally ignored or what.
If they were ignored they would not be part of the commit. It is
understood the comments were incorporated.
Oh, and there are some instances already in the history:
7a9b0b802e (git-clean: add colors to interactive git-clean, 2013-06-25)
1b8fd46732 (git-clean: show items of del_list in columns, 2013-06-25)
52fd972d59 (git-completion: fix regression in zsh support, 2011-05-10)
Although one of those is mine.
Cheers.
--
Felipe Contreras
Is this satisfactory to everybody (I haven't taken a look beyond
comparing the end results)?
Yes, all five patches look good to me.
The "comments-by" sub-thread looked unproductive to me, so I mostly
skipped it. ;) But as the person whose name was in the trailer, I will
say that I do not care either way if it is included. It did not seem to
add anything to me. Some trailers are good for pointing later readers to
folks who may be able to help understand or debug an old commit. Some
are good for just giving credit to people who helped. "Comments-by"
didn't really seem to do either to me (and I don't feel like I
contributed much worthy of credit anyway).
-Peff
Is this satisfactory to everybody (I haven't taken a look beyond
comparing the end results)?
Yes, all five patches look good to me.
So... Acked-by?
The "comments-by" sub-thread looked unproductive to me, so I mostly
skipped it. ;) But as the person whose name was in the trailer, I will
say that I do not care either way if it is included. It did not seem to
add anything to me. Some trailers are good for pointing later readers to
folks who may be able to help understand or debug an old commit. Some
are good for just giving credit to people who helped. "Comments-by"
didn't really seem to do either to me (and I don't feel like I
contributed much worthy of credit anyway).
But you did provide comments, and those comments resulted in changes, so
the comments-by line is accurate. And for what it's worth that commit
trailer line has been used 3 times already, which places it in the 3rd
top decile.
Perhaps some people would prefer feedback-by, but that that line has
been used *zero* times. Even brown-paper-bag-fixes-by has been used more.
Cheers.
--
Felipe Contreras
Is this satisfactory to everybody (I haven't taken a look beyond
comparing the end results)?
Yes, all five patches look good to me.
So... Acked-by?
If you want. I have never found that trailer to be particularly helpful.
quoted
The "comments-by" sub-thread looked unproductive to me, so I mostly
skipped it. ;) But as the person whose name was in the trailer, I will
say that I do not care either way if it is included. It did not seem to
add anything to me. Some trailers are good for pointing later readers to
folks who may be able to help understand or debug an old commit. Some
are good for just giving credit to people who helped. "Comments-by"
didn't really seem to do either to me (and I don't feel like I
contributed much worthy of credit anyway).
But you did provide comments, and those comments resulted in changes, so
the comments-by line is accurate. And for what it's worth that commit
trailer line has been used 3 times already, which places it in the 3rd
top decile.
I didn't say it was inaccurate. I just think it is not useful.
-Peff
From: Felipe Contreras <hidden> Date: 2021-05-27 17:35:40
Jeff King wrote:
quoted
quoted
The "comments-by" sub-thread looked unproductive to me, so I mostly
skipped it. ;) But as the person whose name was in the trailer, I will
say that I do not care either way if it is included. It did not seem to
add anything to me. Some trailers are good for pointing later readers to
folks who may be able to help understand or debug an old commit. Some
are good for just giving credit to people who helped. "Comments-by"
didn't really seem to do either to me (and I don't feel like I
contributed much worthy of credit anyway).
But you did provide comments, and those comments resulted in changes, so
the comments-by line is accurate. And for what it's worth that commit
trailer line has been used 3 times already, which places it in the 3rd
top decile.
I didn't say it was inaccurate. I just think it is not useful.
I understood. I'm not saying the tag must be there because it's useful
(I agree it's usefulness is marginal at best), I'm saying including it
does not hurt.
--
Felipe Contreras