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

2014-07-28 Thread Ronnie Sahlberg
On Fri, Jul 25, 2014 at 12:37 PM, Jonathan Nieder jrnie...@gmail.com 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 jrnie...@gmail.com 1406251347 -0700'

  * without it, it should be the name-part of an ident string,
like 'Jonathan Nieder jrnie...@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.


 [...]
  /*
   * 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-07-28 Thread Eric Sunshine
On Mon, Jul 28, 2014 at 2:01 PM, Ronnie Sahlberg sahlb...@google.com wrote:
 On Fri, Jul 25, 2014 at 12:37 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:
  /*
   * 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

s/then then/then/

  * 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-07-25 Thread Jonathan Nieder
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 jrnie...@gmail.com 1406251347 -0700'

 * without it, it should be the name-part of an ident string,
   like 'Jonathan Nieder jrnie...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html