On Tue, Jul 19, 2022 at 04:23:57PM +0200, Omar Polo wrote:
> > 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.

Yes, the way you wrote the code it is shorter, and it works.

There is another subtle difference. When we call got_object_commit_create()
in worktree.c's commit_worktree(), we call time(NULL) twice. This could mean
that timestamp info between author and committer could differ, provided
committer is not NULL (otherwise, the result of the second time(NULL) will
be ignored). This should probably be fixed to pass a single time_t variable
into got_object_commit_create() twice instead?

Thinking about this in terms of UI:
We assume that people would only use commit -A if they were committing
someone else's work, right? If so, should we make it an error if the
-A flag tries to set the same value redundantly? Such that this would
always error out: got commit -A "$GOT_AUTHOR"
Would that be helpful, or would it be annoying?