Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Jul 19, 2022 at 03:31:44PM +0200, Omar Polo wrote:
> > rebased again after recent change to valid_author; now it's a bit
> > smaller.
> > 
> > (i'm still trying to come up with a description that i like for the
> > manpage)
> 
> > @@ -1541,6 +1541,13 @@ 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 with the same format of
> > +.Ev GOT_AUTHOR .
> > +The user doing the commit is then acknowledged as the
> > +.Dq committer .
> 
> How about this?
> 
> .It Fl A Ar author
> Set author information in the newly created commit to
> .Ar author .
> This is useful when committing changes which were written by someone else.
> The
> .Ar author
> argument must use the same format as the
> .Ev GOT_AUTHOR
> environment variable.
> .Pp
> In addition to storing author information, the newly created commit object
> will retain
> .Dq committer
> information which is obtained, as usual, from the
> .Ev GOT_AUTHOR
> environment variable, or
> .Xr got.conf 5 ,
> or Git configuration settings.

so much better, thanks!

> > @@ -8489,8 +8490,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 = valid_author(author)) != NULL)
> > +				return error;
> 
> Please avoid assigments inside if-statements.
> I recommend this idiom instead:
> 
> 			error = valid_author(author);
> 			if (error)
> 				return errror;

ack

> > @@ -8604,7 +8613,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) {
> 
> What happens when author and committer strings have the same content?
> Should we set committer to NULL in this case?

if committer is NULL got_object_commit_create falls back to the author

: object_create.c:40
:	if (asprintf(&committer_str, "%s%s %lld +0000\n",
:	    GOT_COMMIT_LABEL_COMMITTER, committer ? committer : author,
:	    (long long)(committer ? committer_time : author_time))

so i think there is a little to gain from setting committer to NULL if
it's the same.  However, I could easily add a case for it if you think
it's cleaner.

diff /home/op/w/got
commit - 314c3f148b7679296136b460ea9a8f0d4c74d437
path + /home/op/w/got
blob - e40b2fea7364f818d1cc038e58b39c5e8b17968b
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -1476,7 +1476,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.
@@ -1541,6 +1541,25 @@ The options for
 .Cm got commit
 are as follows:
 .Bl -tag -width Ds
+.It Fl A Ar author
+Set author information in the newly created commit to
+.Ar author .
+This is useful when committing changes which were written by someone
+else.
+The
+.Ar author
+argument must use the same format as the
+.Ev GOT_AUTHOR
+environment variable.
+.Pp
+In addition to storing author information, the newly created commit
+object will retain
+.Dq committer
+information which is obtained, as usual, from the
+.Ev GOT_AUTHOR
+environment variable, or
+.Xr got.conf 5 ,
+or Git configuration settings.
 .It Fl F Ar path
 Use the prepared log message stored in the file found at
 .Ar path
blob - fc3f426e58ce1bbe4245627188786b1b9227e1c0
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -8319,8 +8319,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);
 }
 
@@ -8480,7 +8480,8 @@ cmd_commit(int argc, char *argv[])
 	const char *logmsg = NULL;
 	char *prepared_logmsg = NULL;
 	struct collect_commit_logmsg_arg cl_arg;
-	char *gitconfig_path = NULL, *editor = NULL, *author = NULL;
+	const char *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;
@@ -8489,8 +8490,14 @@ 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;
+			error = valid_author(author);
+			if (error)
+				return error;
+			break;
 		case 'F':
 			if (logmsg != NULL)
 				option_conflict('F', 'm');
@@ -8570,10 +8577,13 @@ 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
 	 * to apply unveil after the log message has been written.
@@ -8604,7 +8614,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) {
@@ -8643,7 +8653,7 @@ done:
 	free(id_str);
 	free(gitconfig_path);
 	free(editor);
-	free(author);
+	free(committer);
 	free(prepared_logmsg);
 	return error;
 }
blob - 74b1300abdc8bad6089c22251a9efd9642490d49
file + regress/cmdline/commit.sh
--- regress/cmdline/commit.sh
+++ regress/cmdline/commit.sh
@@ -752,6 +752,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 -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	author="Foo <foo@example.com>"
+	committer="Flan Hacker <flan_hacker@openbsd.org>"
+
+	echo "modified alpha" > $testroot/wt/alpha
+	(cd $testroot/wt && got commit -A "$author" -m 'from foo') \
+		> /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	(cd $testroot/repo && got log -l1 | egrep '^(from|via):') \
+		> $testroot/stdout
+	ret=$?
+	if [ $ret -ne 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 -ne 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`
 
@@ -1575,6 +1615,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