From: Jonathan Nieder <jrnieder@gmail.com>
Date: Fri, 25 Jul 2014 19:37:28 GMT
Subject: Re: [PATCH 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog
Message-ID: <20140725193728.GG12427@google.com>
In-Reply-To: <1406307521-10339-2-git-send-email-sahlberg@google.com>

Ronnie Sahlberg wrote:

> Add a new flag REFLOG_EMAIL_IS_COMMITTER to _update_reflog to tell it
> that what we pass in as email is already the fully baked committer string
> we can use as-is.

With and without the new flag, the 'email' argument has two different
meanings:

 * with the new flag, it should be an ident string, like
   'Jonathan Nieder <jrnieder@gmail.com> 1406251347 -0700'

 * without it, it should be the name-part of an ident string,
   like 'Jonathan Nieder <jrnieder@gmail.com>

In neither case is it an email address.  This seems unnecessarily
confusing.

Is the caller responsible for checking the argument for validity?
Do callers do so?  Is this performance-critical or could the
transaction_update_reflog function do a sanity-check?

[...]
>  /*
>   * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
>   * this update will first truncate the reflog before writing the entry.
>   * If msg is NULL no update will be written to the log.
>   */
>  int transaction_update_reflog(struct ref_transaction *transaction,
>                                const char *refname,
>                                const unsigned char *new_sha1,
>                                const unsigned char *old_sha1,
>                                const char *email,
>                                unsigned long timestamp, int tz,
>                                const char *msg, int flags,
>                                struct strbuf *err);

This is a lot of parameters, some optional, not all documented.  Would
it make sense to pack some into a struct?

Thanks and hope that helps,
Jonathan