Stefan Sperling <stsp@stsp.name> wrote:
> 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?

sure, i've rebased the diff and tweaked the tests to only look for the
times

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

Yep, that's my reasoning too.

my initial goal actually your latest diff, i only failed to realize that
we needed to reset the committer and not use the old committer, so i've
just tweaked the time accounting.

fwiw `git log' seems to show only the author time by default, which imho
is confusing.

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

i haven't thought about this issue and diff below doesn't address it.
i'll see if this breaks things sometime soon.

> 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'?

i agree on showing the committer timestamp in all cases.  there is an
argument that can be made to show it in tog when switching the view from
committer / author with '@' but i'm not pushing for it.

have to admit that the more i look at this the more i'm realizing i'm
doing this change only because that field exists, not for a real reason.

so, i'd also be fine with dropping the patch

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

i thought of it as a mean to override the committer in case one has a
wrong $GOT_AUTHOR set.  akin to git commit --amend --reset-author if you
want.  but thinking more about it, we can get away without such feature:
it's not a common error and it's still easy to work around it (save the
commit id, histedit to drop it, `got cy $commit_id && got ci' or some
equivalent incantation)

> 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

i like it :)

will think a bit about it


here's the updated diff, but at this point i'm even fine with dropping
it.

diff /home/op/w/got
commit - d3c1b7b8a777dac7004a87b1bcc27ed4b0562f26
path + /home/op/w/got
blob - af59edb11f59ce50cc07c7c94d5fb05a9acc0f54
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -8484,9 +8484,11 @@ cmd_commit(int argc, char *argv[])
 	int allow_bad_symlinks = 0, non_interactive = 0, merge_in_progress = 0;
 	struct got_pathlist_head paths;
 	int *pack_fds = NULL;
+	time_t timestamp;
 
 	TAILQ_INIT(&paths);
 	cl_arg.logmsg_path = NULL;
+	timestamp = time(NULL);
 
 	while ((ch = getopt(argc, argv, "A:F:m:NS")) != -1) {
 		switch (ch) {
@@ -8617,9 +8619,9 @@ cmd_commit(int argc, char *argv[])
 		cl_arg.branch_name += 11;
 	}
 	cl_arg.repo_path = got_repo_get_path(repo);
-	error = got_worktree_commit(&id, worktree, &paths, author, committer,
-	    allow_bad_symlinks, collect_commit_logmsg, &cl_arg,
-	    print_status, NULL, repo);
+	error = got_worktree_commit(&id, worktree, &paths, author, timestamp,
+	    committer, timestamp, allow_bad_symlinks,
+	    collect_commit_logmsg, &cl_arg, print_status, NULL, repo);
 	if (error) {
 		if (error->code != GOT_ERR_COMMIT_MSG_EMPTY &&
 		    cl_arg.logmsg_path != NULL)
blob - b2b02753e786580598a72d4ccfa4aeb006309afe
file + include/got_worktree.h
--- include/got_worktree.h
+++ include/got_worktree.h
@@ -241,7 +241,7 @@ typedef const struct got_error *(*got_worktree_commit_
  */
 const struct got_error *got_worktree_commit(struct got_object_id **,
     struct got_worktree *, struct got_pathlist_head *, const char *,
-    const char *, int, got_worktree_commit_msg_cb, void *,
+    time_t, const char *, time_t, int, got_worktree_commit_msg_cb, void *,
     got_worktree_status_cb, void *, struct got_repository *);
 
 /* Get the path of a commitable worktree item. */
blob - 718cf72db051a1e897c1d70657ac5f12fa7b1043
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -5664,7 +5664,8 @@ commit_worktree(struct got_object_id **new_commit_id,
     struct got_object_id *head_commit_id,
     struct got_object_id *parent_id2,
     struct got_worktree *worktree,
-    const char *author, const char *committer,
+    const char *author, time_t author_time,
+    const char *committer, time_t committer_time,
     got_worktree_commit_msg_cb commit_msg_cb, void *commit_arg,
     got_worktree_status_cb status_cb, void *status_arg,
     struct got_repository *repo)
@@ -5681,7 +5682,6 @@ commit_worktree(struct got_object_id **new_commit_id,
 	struct got_object_id_queue parent_ids;
 	struct got_object_qid *pid = NULL;
 	char *logmsg = NULL;
-	time_t timestamp;
 
 	*new_commit_id = NULL;
 
@@ -5750,9 +5750,9 @@ commit_worktree(struct got_object_id **new_commit_id,
 		STAILQ_INSERT_TAIL(&parent_ids, pid, entry);
 		nparents++;
 	}
-	timestamp = time(NULL);
 	err = got_object_commit_create(new_commit_id, new_tree_id, &parent_ids,
-	    nparents, author, timestamp, committer, timestamp, logmsg, repo);
+	    nparents, author, author_time, committer, committer_time, logmsg,
+	    repo);
 	if (logmsg != NULL)
 		free(logmsg);
 	if (err)
@@ -5868,7 +5868,8 @@ check_non_staged_files(struct got_fileindex *fileindex
 const struct got_error *
 got_worktree_commit(struct got_object_id **new_commit_id,
     struct got_worktree *worktree, struct got_pathlist_head *paths,
-    const char *author, const char *committer, int allow_bad_symlinks,
+    const char *author, time_t author_time, const char *committer,
+    time_t committer_time, int allow_bad_symlinks,
     got_worktree_commit_msg_cb commit_msg_cb, void *commit_arg,
     got_worktree_status_cb status_cb, void *status_arg,
     struct got_repository *repo)
@@ -5952,8 +5953,9 @@ got_worktree_commit(struct got_object_id **new_commit_
 	}
 
 	err = commit_worktree(new_commit_id, &commitable_paths,
-	    head_commit_id, NULL, worktree, author, committer,
-	    commit_msg_cb, commit_arg, status_cb, status_arg, repo);
+	    head_commit_id, NULL, worktree, author, author_time, committer,
+	    committer_time, commit_msg_cb, commit_arg, status_cb, status_arg,
+	    repo);
 	if (err)
 		goto done;
 
@@ -6536,8 +6538,9 @@ rebase_commit(struct got_object_id **new_commit_id,
 	/* NB: commit_worktree will call free(logmsg) */
 	err = commit_worktree(new_commit_id, &commitable_paths, head_commit_id,
 	    NULL, worktree, got_object_commit_get_author(orig_commit),
-	    committer, collect_rebase_commit_msg, logmsg, rebase_status, NULL,
-	    repo);
+	    got_object_commit_get_author_time(orig_commit),
+	    committer, time(NULL), collect_rebase_commit_msg, logmsg,
+	    rebase_status, NULL, repo);
 	if (err)
 		goto done;
 
@@ -7604,10 +7607,12 @@ got_worktree_merge_commit(struct got_object_id **new_c
 	int have_staged_files = 0;
 	struct merge_commit_msg_arg mcm_arg;
 	char *fileindex_path = NULL;
+	time_t timestamp;
 
 	*new_commit_id = NULL;
 
 	TAILQ_INIT(&commitable_paths);
+	timestamp = time(NULL);
 
 	err = get_fileindex_path(&fileindex_path, worktree);
 	if (err)
@@ -7664,8 +7669,9 @@ got_worktree_merge_commit(struct got_object_id **new_c
 	mcm_arg.worktree = worktree;
 	mcm_arg.branch_name = branch_name;
 	err = commit_worktree(new_commit_id, &commitable_paths,
-	    head_commit_id, branch_tip, worktree, author, committer,
-	    merge_commit_msg_cb, &mcm_arg, status_cb, status_arg, repo);
+	    head_commit_id, branch_tip, worktree, author, timestamp,
+	    committer, timestamp, merge_commit_msg_cb, &mcm_arg,
+	    status_cb, status_arg, repo);
 	if (err)
 		goto done;
 
blob - 1264347441d2f3f6859d520ab1230f6161a9d666
file + regress/cmdline/histedit.sh
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -2157,6 +2157,58 @@ test_histedit_resets_committer() {
 	test_done "$testroot" "$ret"
 }
 
+test_histedit_keep_author_time() {
+	local testroot=`test_init keep_author_time`
+	local orig_commit=`git_show_head $testroot/repo`
+	local author='Flan Luck <flan_luck@openbsd.org>'
+
+	got checkout $testroot/repo $testroot/wt >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	echo "modified alpha on master" > $testroot/wt/alpha
+	(cd $testroot/wt && got commit -A "$author" -m 'edit alpha') >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	(cd $testroot/wt && got cat master | grep ^author) \
+		> $testroot/author.expected
+
+	local new_commit=`git_show_head $testroot/repo`
+
+	(cd $testroot/wt && got update -c $orig_commit) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	echo "pick $new_commit" > $testroot/histedit-script
+	echo "mesg edited alpha" >> $testroot/histedit-script
+	(cd $testroot/wt && got histedit -F $testroot/histedit-script) \
+		>/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	(cd $testroot/wt && got cat master | grep ^author) \
+		> $testroot/author
+	cmp -s $testroot/author $testroot/author.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/author $testroot/author.expected
+	fi
+	test_done "$testroot" $ret
+}
+
 test_parseargs "$@"
 run_test test_histedit_no_op
 run_test test_histedit_swap
@@ -2179,3 +2231,4 @@ run_test test_histedit_edit_only
 run_test test_histedit_prepend_line
 run_test test_histedit_mesg_invalid
 run_test test_histedit_resets_committer
+run_test test_histedit_keep_author_time
blob - 2bdfc42c631a9bad8763528b9082b0deacbed9d9
file + regress/cmdline/rebase.sh
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -1568,6 +1568,76 @@ test_rebase_resets_committer() {
 	test_done "$testroot" "$ret"
 }
 
+test_rebase_keep_author_time() {
+	local testroot=`test_init rebase_keep_author`
+	local commit0=`git_show_head $testroot/repo`
+	local author='Flan Luck <flan_luck@openbsd.org>'
+
+	got checkout $testroot/repo $testroot/wt >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return $ret
+	fi
+
+	(cd $testroot/wt && got branch newbranch) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return $ret
+	fi
+
+	echo 'modified alpha on branch' > $testroot/wt/alpha
+	(cd $testroot/wt && got commit -A "$author" -m 'edit alpha') >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return $ret
+	fi
+
+	(cd $testroot/wt && got cat newbranch | grep ^author) \
+		> $testroot/author.expected
+
+	(cd $testroot/wt && got update -b master) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return $ret
+	fi
+
+	echo "modified zeta on master" > $testroot/wt/epsilon/zeta
+	(cd $testroot/wt && got commit -m 'edit zeta') >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return $ret
+	fi
+
+	(cd $testroot/wt && got update) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return $ret
+	fi
+
+	(cd $testroot/wt && got rebase newbranch) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return $ret
+	fi
+
+	(cd $testroot/wt && got cat newbranch | grep ^author) \
+		> $testroot/author
+
+	cmp -s $testroot/author $testroot/author.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/author $testroot/author.expected
+	fi
+	test_done "$testroot" $ret
+}
+
 test_parseargs "$@"
 run_test test_rebase_basic
 run_test test_rebase_ancestry_check
@@ -1584,3 +1654,4 @@ run_test test_rebase_trims_empty_dir
 run_test test_rebase_delete_missing_file
 run_test test_rebase_rm_add_rm_file
 run_test test_rebase_resets_committer
+run_test test_rebase_keep_author_time