On Wed, Jul 20, 2022 at 05:44:30PM +0200, Omar Polo wrote:
> regarding the commit author vs committer turns out that we are already
> doing a sensible thing (i.e. keeping the original author when rebasing
> commits.)

Sorry, I missed this patch and committed some overlapping changes, and
proposed a conflicting diff for histedit/rebase. Let's peel apart which
parts of our changes are not overlapping and continue based on that?

> So, diff below just fixes the time accounting; the idea is:
>
>  - use the current time when creating a new commit
>  - don't call time(NULL) twice
>  - when rebasing a commit use the old author_time and a new committer time
> 
> from some tests this seems to put us in par with git, and I think that
> it's a sensible behaviour.

My changes cover don't add preservation of the 'author' timestamp,
but cover the two other points you are addressing in pretty much
the same way.

I suspect what most people looking at timestamps will want to know is
when a change arrived in the repository they are looking at. The author
timestamp may well be entirely meaningless in that case.
Since we always store the commit timestamp in the committer field, I
suppose preserving the author's timestamp will not hurt.

If we do this, we need to make sure that only the committer timestamp is
used for comparison purposes, such as when sorting commits on parallel
branches for linear display.

And 'got log' and 'tog diff' only display one timestamp. This should be
the committer timestamp in all cases. Or do you see a reason to display
an author timestamp in any commands other than 'got cat'?

> git (somehow) keeps the authorship also when "squashing" commits.  (i.e.
> you fold a commit from Alice with one did by you using 'git rebase -i'
> and the resulting commit will still be from alice.)  not sure if this is
> my lazyness speaking, but i don't think `got histedit should follow the
> same behaviour in this case.

What likely happens here is that author field of the last commit
in the folded series will be used? This is what would happen with
my proposed rebase/histedit patch.

> (going out of topic) it would be nice to have a mean to change the
> commit metadata (author and committer) during histedit however.  i was
> thinking of adding `author' and `committer' that, like `mesg', would
> allow to change those info in-line; i.e.:

Yes, this could be useful in cases where the committer has made minor
edits mixed into a patch submitted by someone else. It is best practice
to commit a submitted patch as-is if possible, and then commit our own
follow-up tweaks on top. But there could be cases where committing
such a patch as-is could break things (e.g. break the build and thus
make future bisecting harder), so this feature would indeed be useful.
 
> 	pick abcdefg commit 1
> 	author Flan Hacker <flan@openbsd.org>
> 	committer Omar Polo <op@omarpolo.com>

Overriding the committer is redundant. We already set committer info based
on values found in our run-time configuration. So I don't see how making it
possible to override the committer field would be useful.

Forcing people to re-type the entire author name and email is a bit heavy.
I would only want to type it again if existing author info is incorrect or
invalid (e.g. would fail the email address check, if such a commit was
created somehow by some tool other than Got or Git).

Should we also offer an alternative syntax which picks the author from a
given (possibly abbreviated) commit ID, e.g. like this?

 	fold abcdefg commit 1
 	fold 12345ae commit 2
 	pick 890abcd commit 3
 	author abcdefg