Stefan Sperling <stsp@stsp.name> writes:

> On Thu, Feb 03, 2022 at 01:45:49PM +0100, Omar Polo wrote:
>> Hello,
>> 
>> Although it's not really used on OpenBSD (because one lists OKs and such
>> in the commit message), when working on personal projects I'd like to
>> keep the authorship of diffs I receive.
>> 
>> With git the usual workflow is to receive a git-formatted patch
>> (e.g. via git send-email) and apply it locally with git-am.  git-am is
>> able to extract the author of the patch from the email and use it in the
>> commit.  That's the "from" and "via" fields showed by tog.
>> 
>> Got only allows to set the author of the commit (the "from" field in
>> tog) but not the committer of the diff.  In the past I've set GOT_AUTHOR
>> before committing to the username and email of the author of the diff.
>> While viable, that doesn't set the committer ("via" field in tog).  OK,
>> it's a bit of an aesthetic reason, but I'd like to being able to keep
>> the authorship and still be listed as the committer of a diff.
>
> The "traditional" way of doing this is to attribute authors in
> the log message via "patch by" mentions and such. I have been
> doing it this way and simply ignored Git's dedicated meta-data
> field in commit objects. But, of course, it makes sense to allow
> people to use this feature.
>
> One problem is that this field is not free form. It requires an
> email address. I think Git might fail to parse the commit object
> otherwise. I would prefer a free-form field but we cannot use it
> as such due to compatibility concerns. I see you have addressed
> this problem in your patch, which is good.
>
> Another problem I see is that in some places people might expect to
> see a committer's name instead of the author's name. For example,
> if an OpenBSD developer opens tog on the OpenBSD repo they might
> not expect non-committer handles to be listed next to commits.
> But we can adjust 'tog log' to display the committer field instead
> of the author field.

I admit that I haven't thought in this detail the issue and agree on all
the issues.

There's also another one issue I think: nomenclature.  got(1) mention
various time the "commit author", the env variable is called GOT_AUTHOR
and the got.conf setting is `author'.  Adding a "-A author" to got ci
which does a subtly different thing can easily confuse people.

>> Diff below is a draft for this.
>> 
>> It lacks several things (manpage bits, testing, probably more) and it
>> also produces the wrong error message if -A has a wrong format, but it's
>> just to gauge the interest and see if it's worth going forward.  It adds
>> an -A flag for got commit which is then used to set the author of the
>> commit.  The committer is still chosen the same way as always (got.conf,
>> GOT_AUTHOR, ...)

(regarding the error message I noticed that it is a separate issue)

> Yes, I guess adding such an option makes sense.
> I don't have a better idea.
>
> Could you send a more complete diff?

Here's a more complete one which adds the manpage bits and a test case
too.  However, given your concerns I'm not so sure anymore!  I wanted
something like this to avoid having to switch back to `git am'
temporally when committing diffs and I thought it would be useful to
others too (AFAIK freebsd uses this approach at least for ports commits)
but the UI issues are real.

diff 4cd2c1745cda66c359c86bea412d77af0b643a7e /home/op/w/got
blob - eaafde8411ebca559ac74b4970f3a9ee755e26b9
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -1337,7 +1337,7 @@ will refuse to run if a specified
 is a directory.
 .El
 .Tg ci
-.It Cm commit Oo Fl F Ar path Oc Oo Fl m Ar message Oc Oo Fl N Oc Oo Fl S Oc Op Ar path ...
+.It Cm commit Oo Fl A Ar author Oc Oo Fl F Ar path Oc Oo Fl m Ar message Oc Oo Fl N Oc Oo Fl S Oc Op Ar path ...
 .Dl Pq alias: Cm ci
 Create a new commit in the repository from changes in a work tree
 and use this commit as the new base commit for the work tree.
@@ -1402,6 +1402,16 @@ The options for
 .Cm got commit
 are as follows:
 .Bl -tag -width Ds
+.It Fl A Ar author
+Specify the
+.Ar author
+of the commit to the specified one.
+If set,
+.Nm
+will use the provided
+.Ar author
+for the commit and set the current user as the
+.Dq committer .
 .It Fl F Ar path
 Use the prepared log message stored in the file found at
 .Ar path
blob - 16b939dba60d2210e6c75b2ffbb51a4b1f8d608a
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -597,6 +597,28 @@ import_progress(void *arg, const char *path)
 }
 
 static const struct got_error *
+validate_author(const char *author)
+{
+	/*
+	 * Really dumb email address check; we're only doing this to
+	 * avoid git's object parser breaking on commits we create.
+	 */
+	while (*author && *author != '<')
+		author++;
+	if (*author != '<')
+		return got_error(GOT_ERR_COMMIT_NO_EMAIL);
+	while (*author && *author != '@')
+		author++;
+	if (*author != '@')
+		return got_error(GOT_ERR_COMMIT_NO_EMAIL);
+	while (*author && *author != '>')
+		author++;
+	if (*author != '>')
+		return got_error(GOT_ERR_COMMIT_NO_EMAIL);
+	return NULL;
+}
+
+static const struct got_error *
 get_author(char **author, struct got_repository *repo,
     struct got_worktree *worktree)
 {
@@ -653,27 +675,7 @@ get_author(char **author, struct got_repository *repo,
 	if (*author == NULL)
 		return got_error_from_errno("strdup");
 
-	/*
-	 * Really dumb email address check; we're only doing this to
-	 * avoid git's object parser breaking on commits we create.
-	 */
-	while (*got_author && *got_author != '<')
-		got_author++;
-	if (*got_author != '<') {
-		err = got_error(GOT_ERR_COMMIT_NO_EMAIL);
-		goto done;
-	}
-	while (*got_author && *got_author != '@')
-		got_author++;
-	if (*got_author != '@') {
-		err = got_error(GOT_ERR_COMMIT_NO_EMAIL);
-		goto done;
-	}
-	while (*got_author && *got_author != '>')
-		got_author++;
-	if (*got_author != '>')
-		err = got_error(GOT_ERR_COMMIT_NO_EMAIL);
-done:
+	err = validate_author(got_author);
 	if (err) {
 		free(*author);
 		*author = NULL;
@@ -7368,8 +7370,8 @@ done:
 __dead static void
 usage_commit(void)
 {
-	fprintf(stderr, "usage: %s commit [-F path] [-m msg] [-N] [-S] "
-	    "[path ...]\n", getprogname());
+	fprintf(stderr, "usage: %s commit [-A author] [-F path] [-m msg] [-N] "
+	    "[-S] [path ...]\n", getprogname());
 	exit(1);
 }
 
@@ -7528,8 +7530,9 @@ cmd_commit(int argc, char *argv[])
 	struct got_object_id *id = NULL;
 	const char *logmsg = NULL;
 	char *prepared_logmsg = NULL;
+	const char *author = NULL;
 	struct collect_commit_logmsg_arg cl_arg;
-	char *gitconfig_path = NULL, *editor = NULL, *author = NULL;
+	char *gitconfig_path = NULL, *editor = NULL, *committer = NULL;
 	int ch, rebase_in_progress, histedit_in_progress, preserve_logmsg = 0;
 	int allow_bad_symlinks = 0, non_interactive = 0, merge_in_progress = 0;
 	struct got_pathlist_head paths;
@@ -7537,8 +7540,13 @@ cmd_commit(int argc, char *argv[])
 	TAILQ_INIT(&paths);
 	cl_arg.logmsg_path = NULL;
 
-	while ((ch = getopt(argc, argv, "F:m:NS")) != -1) {
+	while ((ch = getopt(argc, argv, "A:F:m:NS")) != -1) {
 		switch (ch) {
+		case 'A':
+			author = optarg;
+			if ((error = validate_author(author)) != NULL)
+				return error;
+			break;
 		case 'F':
 			if (logmsg != NULL)
 				option_conflict('F', 'm');
@@ -7613,9 +7621,11 @@ cmd_commit(int argc, char *argv[])
 		goto done;
 	}
 
-	error = get_author(&author, repo, worktree);
+	error = get_author(&committer, repo, worktree);
 	if (error)
 		return error;
+	if (author == NULL)
+		author = committer;
 
 	/*
 	 * unveil(2) traverses exec(2); if an editor is used we have
@@ -7647,7 +7657,7 @@ 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, NULL,
+	error = got_worktree_commit(&id, worktree, &paths, author, committer,
 	    allow_bad_symlinks, collect_commit_logmsg, &cl_arg,
 	    print_status, NULL, repo);
 	if (error) {
@@ -7680,7 +7690,7 @@ done:
 	free(id_str);
 	free(gitconfig_path);
 	free(editor);
-	free(author);
+	free(committer);
 	free(prepared_logmsg);
 	return error;
 }
blob - 34d74d26fefb2a4461a2c489cade5dcd7ebfcd66
file + regress/cmdline/commit.sh
--- regress/cmdline/commit.sh
+++ regress/cmdline/commit.sh
@@ -700,6 +700,46 @@ test_commit_tree_entry_sorting() {
 	test_done "$testroot" "$ret"
 }
 
+test_commit_cmdline_author() {
+	local testroot=`test_init commit_cmdline_author`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ "$ret" != "0" ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	author="Foo <foo@example.com>"
+	committer="Flan Luck <flan_luck@openbsd.org>"
+
+	echo "modified alpha" > $testroot/wt/alpha
+	(cd $testroot/wt && \
+		 GOT_AUTHOR="$committer" got commit -A "$author" -m 'from foo' \
+			   >/dev/null)
+	ret=$?
+	if [ $ret != 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/repo && got log -l1 | egrep '^(from|via):' > $testroot/stdout)
+	ret=$?
+	if [ $ret != 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "from: $author" > $testroot/stdout.expected
+	echo "via: $committer" >> $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret != 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
+}
+
 test_commit_gotconfig_author() {
 	local testroot=`test_init commit_gotconfig_author`
 
@@ -1523,6 +1563,7 @@ run_test test_commit_selected_paths
 run_test test_commit_outside_refs_heads
 run_test test_commit_no_email
 run_test test_commit_tree_entry_sorting
+run_test test_commit_cmdline_author
 run_test test_commit_gotconfig_author
 run_test test_commit_gotconfig_worktree_author
 run_test test_commit_gitconfig_author