On Fri, Jul 25, 2014 at 12:37 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> 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@xxxxxxxxx> 1406251347 -0700'
>
> * without it, it should be the name-part of an ident string,
> like 'Jonathan Nieder <jrnieder@xxxxxxxxx>
>
> 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.
>
> [...]
>> /*
>> * 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html