Re: [PATCH] doc: warn against --committer-date-is-author-date

From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Date: Oct 16, 2025, 14:13 UTC
Message-ID: <52fd63c0-cd43-4ae8-af3e-f3fae02eaabf@app.fastmail.com>
In-Reply-To: <601b145d-b183-4101-acb3-4a32b2ec4380@kdbg.org>

Good afternoon Hannes

On Sat, Oct 11, 2025, at 11:15, Johannes Sixt wrote:

Show 11 quoted lines

> Am 08.10.25 um 21:45 schrieb kristofferhaugsbakk@fastmail.com:
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>
>> This option has legitimate uses but could create a commit history which
>> violates the assumption that commits are strictly increasing in terms of
>> commit timestamps. Warn against that in both git-am(1) and git-rebase(1).
>
> I think that the discussion has meanwhile converged insofar that we do
> not think that the option has a legitimate use case. Rather, it was
> introduced to solve one particular problem case (that is cited below),
> but with a solution that was misguided and not well thought through.

Okay if this was the cited example:

https://lore.kernel.org/git/46d6db660901221441q60eb90bdge601a7a250c3a247@mail.gmail.com/

Then we can clarify with two questions:

1. Is the use case itself reasonable, i.e. abusing[1] git-am(1) to
   pseudo-import commits (modulo the committer)?
2. What is a better way to achieve this goal? (assuming (1) is true)

   It seemed to me that you might as well use the author date.  Unless
   setting max Unix time would be better?  Then at least you will never
   manage to apply something on top of something with a newer commit
   timestamp.

† 1: Since this is not what git-am(1) is designed for

Show 9 quoted lines

>>[snip]
>> +	NOTE: The history walking machinery assumes that commits have
>> +	strictly increasing commit timestamps, with some tolerance for
>> +	clock skew (see linkgit:git-rev-list[1]). You should only use
>> +	this option to lie about the committer date when applying
>> +	commits on top of a base which commit is older (in terms of the
>> +	commit date) than the oldest patch you are applying.
>
> IMO, "NOTE" is not strong enough, it should be at least "WARNING".

Thanks.  I’ll do that.

Show 9 quoted lines

>> ++
>> +By default the command records the date from the e-mail
>> +message as the commit author date, and uses the time of
>> +commit creation as the committer date. This allows the
>> +user to lie about the committer date by using the same
>> +value as the author date.
>
> I would not mind leaving the description first and the warning in the
> follow-up paragraph. It would make for a better flow of reading.

Okay. My thought process was that this was important enough to front-load for readers. But regarding flow: the reader can see the all-caps keyword and skip the paragraph easily if they want/on repeated reads.

But I’m perfectly fine with leaving it in the second paragraph.

Note: Not relevant here but in case there were more than one paragraph
on this option already: should the WARNING be the final paragraph? Or in
the second paragraph? (Like an imporant aside interruption after the
introduction.) I think the final one but just clarifying.

> Perhaps insert "Do not use this option." as the the first sentence,
> either before the description (my preference) or in the warning.

Regarding reading flow, this seems more back-and-forth than this patch. But let’s see.

You preferred option sounds good to me.

You say “first sentence” so I guess we’re not making a one-sentence paragraph. Like this?

    Do not use this option. By default the command records the date from
    the e-mail ...

    WARNING: ...

In that case I think parentheses makes it read better:

    (Do not use this option.) By default the command records the date from
    the e-mail ...

    WARNING: ...

Thanks for the review.