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

From:
Omar Polo <op@omarpolo.com>
Subject:
Re: histedit/rebase vs author/committer times
To:
Stefan Sperling <stsp@stsp.name>
Cc:
gameoftrees@openbsd.org
Date:
Sat, 23 Jul 2022 11:28:50 +0200

Download raw body.

Thread
Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Jul 22, 2022 at 11:21:39AM +0200, Omar Polo wrote:
> > here's the updated diff, but at this point i'm even fine with dropping
> > it.
> 
> Thinking about this some more: How about we update the author timestamp
> in case author name+email == committer name+email, and keep the existing
> author timestamp otherwise?
> 
> This way, we would stop updating the author timestamp only once someone
> else "takes over" a commit object. This is not foolproof, of course (a
> given person could be using multiple email addresses). But a commit with
> equivalent author and committer name+email, and different timestamps
> for each, would look a bit strange.

yes, it seems a good idea

diff 89771dc50558f0fd902ecabecdc57e448bf7ab92 531381f27882dbe3fc1542c2b9c52a84f76aa3e1
commit - 89771dc50558f0fd902ecabecdc57e448bf7ab92
commit + 531381f27882dbe3fc1542c2b9c52a84f76aa3e1
blob - af59edb11f59ce50cc07c7c94d5fb05a9acc0f54
blob + 009c0d06e2e5d8714a10c5c8a008771349f971b0
--- 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
blob + ebd10359580e8245378ef80412c51e2bd57528fa
--- 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
blob + 4621b4fb7694cc20f520a898c89289890f5cce3b
--- 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;
 
@@ -6463,6 +6465,8 @@ rebase_commit(struct got_object_id **new_commit_id,
 	struct got_reference *head_ref = NULL;
 	struct got_object_id *head_commit_id = NULL;
 	char *logmsg = NULL;
+	const char *author;
+	time_t author_time, committer_time;
 
 	TAILQ_INIT(&commitable_paths);
 	*new_commit_id = NULL;
@@ -6533,11 +6537,16 @@ rebase_commit(struct got_object_id **new_commit_id,
 			goto done;
 	}
 
+	author = got_object_commit_get_author(orig_commit);
+	author_time = got_object_commit_get_author_time(orig_commit);
+	committer_time = time(NULL);
+	if (!strcmp(author, committer))
+		author_time = committer_time;
+
 	/* 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);
+	    NULL, worktree, author, author_time, committer, committer_time,
+	    collect_rebase_commit_msg, logmsg, rebase_status, NULL, repo);
 	if (err)
 		goto done;
 
@@ -7604,10 +7613,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 +7675,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 - 26f717faa35b5906f8abb7c9f459f633ebaa09dc
blob + 88c72142ecf6a2486b63f1860cc0adc15989f5d7
--- regress/cmdline/common.sh
+++ regress/cmdline/common.sh
@@ -87,6 +87,13 @@ git_show_author_time()
 	(cd $repo && git show --no-patch --pretty='format:%at' $object)
 }
 
+git_show_committer_time()
+{
+	local repo="$1"
+	local object="$2"
+	(cd $repo && git show --no-patch --pretty='format:%ct' $object)
+}
+
 git_show_tagger_time()
 {
 	local repo="$1"
blob - 1264347441d2f3f6859d520ab1230f6161a9d666
blob + f6acd386fa4215aa63dcca724a2318d5ab9b4b51
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -2157,6 +2157,108 @@ 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_histedit_reset_committer_time() {
+	local testroot=`test_init histedit_reset_committer_time`
+	local orig_commit=`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
+
+	echo 'modified alpha on master' > $testroot/wt/alpha
+	(cd $testroot/wt && got commit -m 'edit alpha') >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	local cid=`git_show_head $testroot/repo`
+	local atime0=`git_show_author_time $testroot/repo master`
+	local ctime0=`git_show_committer_time $testroot/repo master`
+
+	# wait so the timestamp for the histedited commit will be
+	# different.
+	sleep 1
+
+	(cd $testroot/wt && got update -c $orig_commit) >/dev/null
+	echo "pick $cid" > $testroot/histedit-script
+	echo "mesg edited commit mesg" >> $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
+
+	local atime1=`git_show_author_time $testroot/repo master`
+	local ctime1=`git_show_committer_time $testroot/repo master`
+
+	if [ "$atime0" -eq "$atime1" -o "$ctime0" -eq "$ctime1" ]; then
+		echo "time mismatch ($atime0, $atime1, $ctime0, $ctime1)" >&2
+		test_done "$testroot" 1
+		return
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_histedit_no_op
 run_test test_histedit_swap
@@ -2179,3 +2281,5 @@ 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
+run_test test_histedit_reset_committer_time
blob - 2bdfc42c631a9bad8763528b9082b0deacbed9d9
blob + 5facfdf7c1fab68265ac6f16dcf9e83d2583e9ce
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -1568,6 +1568,141 @@ 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_rebase_reset_committer_time() {
+	local testroot=`test_init rebase_reset_committer_time`
+
+	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
+
+	echo 'modified alpha on branch' > $testroot/wt/alpha
+	(cd $testroot/wt && got commit -m 'edit alpha') >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return $ret
+	fi
+
+	local commit0=`git_show_head $testroot/repo`
+
+	(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
+
+	# wait so the timestamp for the rebased commit will be
+	# different.
+	sleep 1
+
+	(cd $testroot/wt && got update && got rebase newbranch) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return $ret
+	fi
+
+	local commit1=`git_show_head $testroot/repo`
+
+	local atime0=`git_show_author_time $testroot/repo $commit0`
+	local atime1=`git_show_author_time $testroot/repo $commit1`
+	local ctime0=`git_show_committer_time $testroot/repo $commit0`
+	local ctime1=`git_show_committer_time $testroot/repo $commit1`
+
+	if [ "$atime0" -ne "$atime1" -o "$ctime0" -ne "$ctime1" -o \
+	     "$atime0" -ne "$ctime0" ]; then
+		echo "time mismatch ($atime0, $atime1, $ctime0, $ctime1)" >&2
+		test_done "$testroot" 1
+		return
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_rebase_basic
 run_test test_rebase_ancestry_check
@@ -1584,3 +1719,5 @@ 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
+run_test test_rebase_reset_committer_time