Re: [PATCH 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog

From: Ronnie Sahlberg <sahlberg@google.com>
Date: Jul 28, 2014, 18:01 UTC
Message-ID: <CAL=YDWmSKBnjXiBUJjOb-+M7R8d==mMsN11DjDTDxU2+WAh+vQ@mail.gmail.com>
In-Reply-To: <20140725193728.GG12427@google.com>

On Fri, Jul 25, 2014 at 12:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 17 quoted lines

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

True.

I have changed it to to be a field named 'id' instead of 'email'. I also added changes to update other places in the code to change the name for this to 'id' too.

It is confusing. I too were caught by this a while back when just based on the name for the callback iterators I assumed what was passed there was an email address while it was not. I didn't fix that then but I fixed it now.

Thanks!

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

I have added basic sanity checks, such as required strings are non-NULL.

Show 18 quoted lines

>
> [...]
>>  /*
>>   * 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?

I changed email,timestamp,tz into a struct
/*
 * Committer data provided to reflog updates.
 * If flags contain REFLOG_COMMITTER_DATA_IS_VALID then
 * then the structure contains a prebaked committer string
 * just like git_committer_info() would return.
 *
 * If flags does not contain REFLOG_COMMITTER_DATA_IS_VALID
 * then the committer info string will be generated using the passed
 * email, timestamp and tz fields.
 * This is useful for example from reflog iterators where you are passed
 * these fields individually and not as a prebaked git_committer_info()
 * string.
 */
struct reflog_committer_info {
const char *committer_info;

const char *id; unsigned long timestamp; int tz; };

>
> Thanks and hope that helps,
> Jonathan