Re: [PATCH v3 0/4] Optimization batch 12: miscellaneous unthemed stuff

[Elijah Newren <newren@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.