Re: [PR] fix some typos
On Wed, Sep 14, 2022 at 11:05:30PM +0200, Tim Düsterhus wrote: > On 9/14/22 22:43, Willy Tarreau wrote: > > Thanks, but quite honnestly, even this one is not a commit message. > > There really is not much to be explained for some obvious typo fixes in > comments (i.e. not even changing the binary). I don't think the commit > message was worse than Ilya's semi-regular typo patches. That's what I suspected as well, my point was that encouraging someone to go figure somewhere by following the link that in the end there was nothing more to see didn't bring any value. However, just mentioning the isolated systems touched by the patch can help quickly spot it when a backport for a fix fails due to slightly different context. > I've only put the PR link in the description, because I was unable to think > of literally anything else that might be useful for a typo fix and I didn't > want to leave the description empty (because CONTRIBUTING says I may not do > so). Yep, that rule generally helps thinking about what was not said in the subject (e.g. "this just fixes 4 trivial typos in comments in mux_quic.c, ssl_sock.c and xprt_quic.c"). > > You get the idea. Regarding the tag, it's fine (and even preferred) to > > keep the signed-off-by if you edit the patch, you just add yours below > > after a short description between square brackets. Example: > > Understood. I've dropped it, because you said in the past that you are not > editing patches containing a s-o-b. You're absolutely right, but it's in fact a rule I use to gauge what a contributor expects without wasting their time or a round-trip on a trivial change. I think that if you send me a patch, you've rechecked it 3 times to be certain it's OK before signing it, so what I could take for a typo could also be a mistunderstanding on my side, thus I will normally not change it without asking. On the contrary, an unsigned commit reads to me like "do what you want with it, I don't care", and if the effort of changing it is lower than that of requesting that it's updated, and there's no perceived benefit in teaching the submitter how to improve their future contributions, or it's really trivial, then I can update it. > I'm not sending another patch or pursuing this patch further, because that > is not a good use of either of our time. I just came across this unhandled > PR in my Inbox and wanted to save you the effort of replying to the author / > fixing the message yourself. Seeing that you took the effort of writing this > long reply, I probably should've just ignored the patch entirely, because it > ultimately wasted time for everyone involved. No, not at all, quite the opposite! I just took this opportunity to explain some tiny improvements to the process for everyone and the rationale behind them, and also to help you figure more easily how to proceed next time without having to hesitate too long. I know what it's like to submit a patch, whatever the project, there's a lot of hesitation, at least 4 or 5 "git commit --amend", sometimes even a last-minute re-edit of the patch itself. The simple fact that you explained the nature of your changes indicates that it took you some thinking. That's what I want to help make smoother so that you don't need to invest as much of your time on such patches in the future. I could very well have re-edited it myself, but that would not have been respectful of your effort. Thanks! Willy
Re: [PR] fix some typos
Willy, [dropping the original PR author from Cc, because my reply is not useful to them] On 9/14/22 22:43, Willy Tarreau wrote: Thanks, but quite honnestly, even this one is not a commit message. There really is not much to be explained for some obvious typo fixes in comments (i.e. not even changing the binary). I don't think the commit message was worse than Ilya's semi-regular typo patches. I've only put the PR link in the description, because I was unable to think of literally anything else that might be useful for a typo fix and I didn't want to leave the description empty (because CONTRIBUTING says I may not do so). You get the idea. Regarding the tag, it's fine (and even preferred) to keep the signed-off-by if you edit the patch, you just add yours below after a short description between square brackets. Example: Understood. I've dropped it, because you said in the past that you are not editing patches containing a s-o-b. I'm not sending another patch or pursuing this patch further, because that is not a good use of either of our time. I just came across this unhandled PR in my Inbox and wanted to save you the effort of replying to the author / fixing the message yourself. Seeing that you took the effort of writing this long reply, I probably should've just ignored the patch entirely, because it ultimately wasted time for everyone involved. Best regards Tim Düsterhus
Re: [PR] fix some typos
Hi Tim, On Wed, Sep 14, 2022 at 07:44:27PM +0200, Tim Düsterhus wrote: > The fixes look correct to me, but the commit message is horrible. I've > attached the patch with a proper commit message (dropping the Signed-off-by > and adding a Co-authored-by). > > Best regards > Tim Düsterhus > From e2fa51bc9f7e7d7d451df2ad9cd72071211faf6a Mon Sep 17 00:00:00 2001 > From: cui fliter > Date: Mon, 29 Aug 2022 14:42:57 +0800 > Subject: [PATCH] CLEANUP: Fix typos in C comments > > see GitHub PR #1843 > > [Tim: Rephrased the commit message] > > Co-authored-by: Tim Duesterhus Thanks, but quite honnestly, even this one is not a commit message. I mean, referencing an external issue for a complement of information is fine, but as the sole source, please no. First, it means that those who read the logs have zero info there, they need to open a browser, visit the project on github and enter the PR number just to get a very likely uninteresting context. Second it's not greppable. And third, imagine that one day a large company would decide to acquire github (yeah I know, purely fictional :-)) and to progressively change the conditions to the point that we're almost forced out. Suddenly, all such knowledge is lost. Forever. So commit messages must accurately describe the problem that they try to solve. External references for more info, more context etc is perfectly fine and welcome, but that must never be a shortcut for a lengthy message (and yes I know that here we're talking only about typos). Another thing to try to avoid is too generic subject lines. One good rule of thumb is to try to invent a subject line that is likely unique in the project. That doesn't mean it ought to be checked, just that if it's precise enough it will likely be unique. Here it could for example be something like: CLEANUP: Fix tiny typos in C comments in SSL and QUIC or: CLEANUP: quic,ssl: fix tiny typos in C comments You get the idea. Regarding the tag, it's fine (and even preferred) to keep the signed-off-by if you edit the patch, you just add yours below after a short description between square brackets. Example: Signed-off-by: joe user < > [Tim: authored a real commit message] Signed-off-by: Tim < > You must just not add an s-o-b for someone else if there wasn't (though you can always add yours). Regarding "co-authored", normally it's more for when the patch itself was adapted, but usually the first author places it from the start because all participants generally acknowledge co-authorship. Thanks! Willy
Re: [PR] fix some typos
Willy,
On 8/29/22 09:23, PR Bot wrote:
Author: cui fliter
Number of patches: 1
This is an automated relay of the Github pull request:
fix some typos
Patch title(s):
fix some typos
Link:
https://github.com/haproxy/haproxy/pull/1843
Edit locally:
wget https://github.com/haproxy/haproxy/pull/1843.patch && vi 1843.patch
Apply locally:
curl https://github.com/haproxy/haproxy/pull/1843.patch | git am -
The fixes look correct to me, but the commit message is horrible. I've
attached the patch with a proper commit message (dropping the
Signed-off-by and adding a Co-authored-by).
Best regards
Tim DüsterhusFrom e2fa51bc9f7e7d7d451df2ad9cd72071211faf6a Mon Sep 17 00:00:00 2001
From: cui fliter
Date: Mon, 29 Aug 2022 14:42:57 +0800
Subject: [PATCH] CLEANUP: Fix typos in C comments
see GitHub PR #1843
[Tim: Rephrased the commit message]
Co-authored-by: Tim Duesterhus
---
src/mux_quic.c | 2 +-
src/quic_tls.c | 4 ++--
src/ssl_sock.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/mux_quic.c b/src/mux_quic.c
index bf91750c5e..937a8cb146 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -2296,7 +2296,7 @@ static int qc_wake(struct connection *conn)
/* Check if a soft-stop is in progress.
*
- * TODO this is revelant for frontend connections only.
+ * TODO this is relevant for frontend connections only.
*
* TODO Client should be notified with a H3 GOAWAY and then a
* CONNECTION_CLOSE. However, quic-conn uses the listener socket for
diff --git a/src/quic_tls.c b/src/quic_tls.c
index 3114aa623c..a216f8678e 100644
--- a/src/quic_tls.c
+++ b/src/quic_tls.c
@@ -363,7 +363,7 @@ int quic_tls_enc_aes_ctx_init(EVP_CIPHER_CTX **aes_ctx,
}
/* Encrypt bytes from buffer into with as AES
- * cipher context. This is the responsability of the caller to check there
+ * cipher context. This is the responsibility of the caller to check there
* is at least bytes of available space in buffer.
* Return 1 if succeeded, 0 if not.
*/
@@ -403,7 +403,7 @@ int quic_tls_dec_aes_ctx_init(EVP_CIPHER_CTX **aes_ctx,
}
/* Decrypt data into with as AES cipher context.
- * This is the responsability of the caller to check there is at least
+ * This is the responsibility of the caller to check there is at least
* bytes into buffer.
* Return 1 if succeeded, 0 if not.
*/
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 84e03d33b1..cf9c57634d 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -2177,7 +2177,7 @@ static int ssl_sock_advertise_alpn_protos(SSL *s, const unsigned char **out,
#ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
#ifndef SSL_NO_GENERATE_CERTIFICATES
-/* Configure a DNS SAN extenion on a certificate. */
+/* Configure a DNS SAN extension on a certificate. */
int ssl_sock_add_san_ext(X509V3_CTX* ctx, X509* cert, const char *servername) {
int failure = 0;
X509_EXTENSION *san_ext = NULL;
--
2.37.3
