All of lore.kernel.org
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Derrick Stolee" <dstolee@microsoft.com>,
	"Jonathan Tan" <jonathantanmy@google.com>,
	"Taylor Blau" <me@ttaylorr.com>, "René Scharfe" <l.s.r@web.de>
Subject: Re: [PATCH v3 0/4] Optimization batch 12: miscellaneous unthemed stuff
Date: Fri, 4 Jun 2021 08:48:21 -0700	[thread overview]
Message-ID: <CABPp-BEsiw-E9_TGUDaGEkhQHNqLH48=ZTu7yXX7XqRaM2JdOQ@mail.gmail.com> (raw)
In-Reply-To: <a5755340-c0c8-8028-7c03-3c8e7f0268d1@gmail.com>

On Fri, Jun 4, 2021 at 6:11 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 6/4/2021 12:39 AM, Elijah Newren via GitGitGadget wrote:
> > This series depends on en/ort-perf-batch-11 textually, but is semantically
> > independent of it.
> >
> > Changes since v2:
> >
> >  * Made suggested minor tweaks from Stolee on Patch 1
> >  * Dropped patch 3, for now
>
> With these changes, I think the code is good to go.
>
> >  * Added Stolee's Acked-by
> ...
> >  1:  c4a0f6a9510c ! 1:  f63ffc2a7c22 merge-ort: replace string_list_df_name_compare with faster alternative
> >      @@ Commit message
> >               just-one-mega:   500.3  ms ±  3.8  ms   480.1  ms ±  3.9  ms
> >
> >           Signed-off-by: Elijah Newren <newren@gmail.com>
> >      +    Acked-by: Derrick Stolee <dstolee@microsoft.com>
>
> I believe the sign-off should always be the last thing in
> the message. Perhaps Junio is willing to fix this without a
> re-roll?

Interesting, this is the first I've ever heard of such a requirement,
and I've submitted patches this way numerous times and have seen
others do it.  A quick search through git.git history says there are
5133 commits that place such trailers before the author's
Signed-off-by, and 1175 that place them after.  While the former is
clearly more common, and some of the latter could have been Junio
adding trailers while applying the patches, there still seem like
plenty of counter-examples suggesting that there is no rule here.

Digging into Documentation/SubmittingPatches, I find three places that
suggest or state something about placement of trailers.

'''
[[real-name]]
Also notice that a real name is used in the `Signed-off-by` trailer. Please
don't hide your real name.

[[commit-trailers]]
If you like, you can put extra tags at the end:
'''

Which isn't definitive but seems like it could be easy to accidentally
construe as a rule that says the opposite, namely that other trailers
come after the Signed-off-by.  Another part of the document states:

'''
you add a "Signed-off-by" trailer to your commit, that looks like
this:

....
        Signed-off-by: Random J Developer <random@developer.example.org>
....
'''

which possibly suggests that the relative location compared to other
trailers does not matter.  The wording in that document that comes
closest to your interpretation is

'''
At the beginning of the
patch should come your commit message, ending with the
`Signed-off-by` trailers'''
'''

but if it had really meant to convey the rule you suggest here, it
should have said "ending with your Signed-off-by trailer"; the
"trailers" here to me suggest the phrasing here has a totally
different meaning than your rule.

So, if "Signed-off-by should come last" is a rule, then neither
existing practice nor our documentation seem to cover it.  I'll assume
it's not a rule for now.

> Feel free to replace Acked-by with Reviewed-by.

Thanks.

  reply	other threads:[~2021-06-04 15:49 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-27  8:37 [PATCH 0/5] " Elijah Newren via GitGitGadget
2021-05-27  8:37 ` [PATCH 1/5] merge-ort: replace string_list_df_name_compare with faster alternative Elijah Newren via GitGitGadget
2021-05-27 21:00   ` René Scharfe
2021-05-27 22:47     ` Elijah Newren
2021-05-28 16:12       ` René Scharfe
2021-05-28 18:09         ` Elijah Newren
2021-05-28  1:32   ` Taylor Blau
2021-05-28  4:10     ` Elijah Newren
2021-05-27  8:37 ` [PATCH 2/5] diffcore-rename: avoid unnecessary strdup'ing in break_idx Elijah Newren via GitGitGadget
2021-05-27  8:37 ` [PATCH 3/5] diffcore-rename: enable limiting rename detection to relevant destinations Elijah Newren via GitGitGadget
2021-05-27  8:37 ` [PATCH 4/5] Fix various issues found in comments Elijah Newren via GitGitGadget
2021-05-27  8:37 ` [PATCH 5/5] merge-ort: miscellaneous touch-ups Elijah Newren via GitGitGadget
2021-06-01 14:58 ` [PATCH v2 0/5] Optimization batch 12: miscellaneous unthemed stuff Elijah Newren via GitGitGadget
2021-06-01 14:58   ` [PATCH v2 1/5] merge-ort: replace string_list_df_name_compare with faster alternative Elijah Newren via GitGitGadget
2021-06-02 11:29     ` Derrick Stolee
2021-06-01 14:58   ` [PATCH v2 2/5] diffcore-rename: avoid unnecessary strdup'ing in break_idx Elijah Newren via GitGitGadget
2021-06-01 14:58   ` [PATCH v2 3/5] diffcore-rename: enable limiting rename detection to relevant destinations Elijah Newren via GitGitGadget
2021-06-03 12:54     ` Derrick Stolee
2021-06-03 14:13       ` Elijah Newren
2021-06-01 14:58   ` [PATCH v2 4/5] Fix various issues found in comments Elijah Newren via GitGitGadget
2021-06-01 14:58   ` [PATCH v2 5/5] merge-ort: miscellaneous touch-ups Elijah Newren via GitGitGadget
2021-06-03 12:55   ` [PATCH v2 0/5] Optimization batch 12: miscellaneous unthemed stuff Derrick Stolee
2021-06-04  4:39   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
2021-06-04  4:39     ` [PATCH v3 1/4] merge-ort: replace string_list_df_name_compare with faster alternative Elijah Newren via GitGitGadget
2021-06-04  4:39     ` [PATCH v3 2/4] diffcore-rename: avoid unnecessary strdup'ing in break_idx Elijah Newren via GitGitGadget
2021-06-04  4:39     ` [PATCH v3 3/4] Fix various issues found in comments Elijah Newren via GitGitGadget
2021-06-04  4:39     ` [PATCH v3 4/4] merge-ort: miscellaneous touch-ups Elijah Newren via GitGitGadget
2021-06-04 13:11     ` [PATCH v3 0/4] Optimization batch 12: miscellaneous unthemed stuff Derrick Stolee
2021-06-04 15:48       ` Elijah Newren [this message]
2021-06-04 16:30         ` Elijah Newren
2021-06-04 16:35         ` Jeff King
2021-06-04 18:42           ` Derrick Stolee
2021-06-04 19:43             ` Elijah Newren
2021-06-04 19:53             ` Jeff King
2021-06-08 16:11     ` [PATCH v4 " Elijah Newren via GitGitGadget
2021-06-08 16:11       ` [PATCH v4 1/4] merge-ort: replace string_list_df_name_compare with faster alternative Elijah Newren via GitGitGadget
2021-06-08 16:11       ` [PATCH v4 2/4] diffcore-rename: avoid unnecessary strdup'ing in break_idx Elijah Newren via GitGitGadget
2021-06-08 16:11       ` [PATCH v4 3/4] Fix various issues found in comments Elijah Newren via GitGitGadget
2021-06-08 16:11       ` [PATCH v4 4/4] merge-ort: miscellaneous touch-ups Elijah Newren via GitGitGadget

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='CABPp-BEsiw-E9_TGUDaGEkhQHNqLH48=ZTu7yXX7XqRaM2JdOQ@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jonathantanmy@google.com \
    --cc=l.s.r@web.de \
    --cc=me@ttaylorr.com \
    --cc=stolee@gmail.com \
    --subject='Re: [PATCH v3 0/4] Optimization batch 12: miscellaneous unthemed stuff' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.