Stefan Sperling <stsp@stsp.name> wrote:
> 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?

that's a good point.  yes, commit_worktree should be fixed not to call
time twice for sure.  I'll look into it soon, I was thinking of looking
at preserving the committer info during histedit.

> 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?

this is another good question.  i'm not 100% convinced, but i'm inclined
toward raising an error.

P.S.: there's a way to build a "throw-away" got_error?  it would allow
me to avoid defining a new error type for this corner case.