"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
histedit/rebase vs author/committer times
To:
gameoftrees@openbsd.org
Date:
Wed, 20 Jul 2022 17:44:30 +0200

Download raw body.

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

(the diff goes on and adds some further checks in histedit and rebase to
ensure we keep the authorship information and -to some extent- the times
too)

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.

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

	pick abcdefg commit 1
	author Flan Hacker <flan@openbsd.org>
	committer Omar Polo <op@omarpolo.com>
	mesg ...

they aren't 4 letters long like pick, edit etc but "a" and "c" are free.
(i thought of "meta" which would be 4 letters long but "m" is taken, or
"info author xyz" / "info committer xyz" that's still 4 letter and "i"
is free.)

anyway, there's no rush, we can come back to it in the future.

back on topic, ok/comments for the diff? :)

diff refs/heads/main refs/heads/authorship
commit - 97f28afb9820967508ab57f5ed6a5c75f49464b4
commit + e1d9e9decf0e09c7920560b46884c5e44350b0bf
blob - 34f6db81edda3318d140a359f85fce30ea5fc4a7
blob + a31378bbbec0b87ac7b7404efe0a5d4516f67e2f
--- got/got.c
+++ got/got.c
@@ -8486,7 +8486,10 @@ 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 now;
 
+	now = time(NULL);
+
 	TAILQ_INIT(&paths);
 	cl_arg.logmsg_path = NULL;
 
@@ -8619,8 +8622,8 @@ 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,
+	error = got_worktree_commit(&id, worktree, &paths, author, now,
+	    committer, now, allow_bad_symlinks, collect_commit_logmsg, &cl_arg,
 	    print_status, NULL, repo);
 	if (error) {
 		if (error->code != GOT_ERR_COMMIT_MSG_EMPTY &&
blob - 610e03aa8788afbfcdcdc782c96c1c1965707674
blob + 0874d5b5c83a449548ce984bc8d64dbcee49bf9e
--- 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 - 585486aa9bd3b5620681a9e4ccad89befbbaf36b
blob + 54ce7f86a3ee99f093efc3dbf88307dcb2f073b7
--- 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)
@@ -5750,7 +5751,8 @@ commit_worktree(struct got_object_id **new_commit_id,
 		nparents++;
 	}
 	err = got_object_commit_create(new_commit_id, new_tree_id, &parent_ids,
-	    nparents, author, time(NULL), committer, time(NULL), logmsg, repo);
+	    nparents, author, author_time, committer, committer_time, logmsg,
+	    repo);
 	if (logmsg != NULL)
 		free(logmsg);
 	if (err)
@@ -5866,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)
@@ -5950,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;
 
@@ -6533,7 +6537,8 @@ 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),
-	    got_object_commit_get_committer(orig_commit),
+	    got_object_commit_get_author_time(orig_commit),
+	    got_object_commit_get_committer(orig_commit), time(NULL),
 	    collect_rebase_commit_msg, logmsg, rebase_status, NULL, repo);
 	if (err)
 		goto done;
@@ -7599,6 +7604,7 @@ 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 now;
 
 	*new_commit_id = NULL;
 
@@ -7656,10 +7662,12 @@ got_worktree_merge_commit(struct got_object_id **new_c
 
 	}
 
+	now = time(NULL);
+
 	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,
+	    head_commit_id, branch_tip, worktree, author, now, committer, now,
 	    merge_commit_msg_cb, &mcm_arg, status_cb, status_arg, repo);
 	if (err)
 		goto done;
blob - 89e063ffccc47d6560386bc393c8e4351e66eb36
blob + ed23d81e72b628a7197f0a4050a7a10aee7195bf
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -2092,6 +2092,72 @@ test_histedit_mesg_invalid() {
 	test_done "$testroot" $ret
 }
 
+test_histedit_fold_keep_author() {
+	local testroot=`test_init fold_keep_author`
+	local orig_commit=`git_show_head $testroot/repo`
+	local author='Foo <foo@example.com>'
+
+	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
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	(cd $testroot/wt && got cat master | grep ^committer) | \
+		sed 's/>.*/>/' > $testroot/committer
+	echo "committer $GOT_AUTHOR" > $testroot/committer.expected
+	cmp -s $testroot/committer $testroot/committer.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/committer $testroot/committer.expected
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	test_done "$testroot" $ret
+}
+
 test_parseargs "$@"
 run_test test_histedit_no_op
 run_test test_histedit_swap
@@ -2113,3 +2179,4 @@ run_test test_histedit_fold_only_empty_logmsg
 run_test test_histedit_edit_only
 run_test test_histedit_prepend_line
 run_test test_histedit_mesg_invalid
+run_test test_histedit_fold_keep_author
blob - 350e19ca9e343605f4497444e665c380a5212b8a
blob + cd52fa5597e77d8c9d5f5190880db91925daced2
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -1480,6 +1480,75 @@ test_rebase_rm_add_rm_file() {
 	test_done "$testroot" "$ret"
 }
 
+test_rebase_keep_author() {
+	local testroot=`test_init rebase_keep_author`
+	local commit0=`git_show_head $testroot/repo`
+
+	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
+
+	local author='Foo <foo@example.com>'
+
+	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 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 log -l 1 | egrep '^(from|via):') \
+		> $testroot/meta
+	echo "from: $author" > $testroot/meta.expected
+	echo "via: $GOT_AUTHOR" >> $testroot/meta.expected
+	cmp -s $testroot/meta.expected $testroot/meta
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/meta.expected $testroot/meta
+	fi
+	test_done "$testroot" $ret
+}
+
 test_parseargs "$@"
 run_test test_rebase_basic
 run_test test_rebase_ancestry_check
@@ -1495,3 +1564,4 @@ run_test test_rebase_out_of_date
 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_keep_author