Re: [PATCH 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog
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
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
[PATCH 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog
In many places in the code we do not have access to the individual fields
in the committer data. Instead we might only have access to prebaked data
such as what is returned by git_committer_info() containing a string
that consists of email, timestamp, zone etc.
This makes it inconvenient to use transaction_update_reflog since it means
you would have to first parse git_committer_info before you can call
update_reflog.
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.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 20
refs.h | 1 +
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 2662ef6..6c55032 100644
--- a/refs.c
+++ b/refs.c
@@ -3522,14 +3522,18 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
hashcpy(update-old_sha1, old_sha1);
update-reflog_fd = -1;
if (email) {
- struct strbuf buf = STRBUF_INIT;
- char sign = (tz 0) ? '-' : '+';
- int zone = (tz 0) ? (-tz) : tz;
-
- strbuf_addf(buf, %s %lu %c%04d, email, timestamp, sign,
- zone);
- update-committer = xstrdup(buf.buf);
- strbuf_release(buf);
+ if (flags REFLOG_EMAIL_IS_COMMITTER)
+ update-committer = xstrdup(email);
+ else {
+ struct strbuf buf = STRBUF_INIT;
+ char sign = (tz 0) ? '-' : '+';
+ int zone = (tz 0) ? (-tz) : tz;
+
+ strbuf_addf(buf, %s %lu %c%04d, email, timestamp,
+ sign, zone);
+ update-committer = xstrdup(buf.buf);
+ strbuf_release(buf);
+ }
}
if (msg)
update-msg = xstrdup(msg);
diff --git a/refs.h b/refs.h
index 0172f48..eb918a0 100644
--- a/refs.h
+++ b/refs.h
@@ -309,6 +309,7 @@ int transaction_delete_sha1(struct ref_transaction
*transaction,
* Flags = 0x100 are reserved for internal use.
*/
#define REFLOG_TRUNCATE 0x01
+#define REFLOG_EMAIL_IS_COMMITTER 0x02
/*
* Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
* this update will first truncate the reflog before writing the entry.
--
2.0.1.508.g763ab16
--
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
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
