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

From: Elijah Newren <newren@gmail.com>
Date: Jun 4, 2021, 15:48 UTC
Message-ID: <CABPp-BEsiw-E9_TGUDaGEkhQHNqLH48=ZTu7yXX7XqRaM2JdOQ@mail.gmail.com>
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:

Show 24 quoted lines

>
> 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.