Re: [PATCH 09/15] refs.c: only write reflog update if msg is non-NULL
On Thu, Oct 23, 2014 at 11:32 AM, Junio C Hamano wrote: > Ronnie Sahlberg writes: > >> commit 020ed65a12838bdead64bc3c5de249d3c8f5cfd8 upstream. >> >> When performing a reflog transaction update, only write to the reflog iff >> msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform >> an update that only truncates but does not write. > > Does any existing caller call this codepath with update->msg == NULL? > > Will "please truncate" stay to be the only plausible special cause > to call into this codepath without having any meaningful message? > > I am trying to make sure that this patch is not painting us into a > corner where we will find out another reason for doing something > esoteric in this codepath but need to find a way other than setting > msg to NULL for the caller to trigger that new codepath. Put it in > another way, please convince me that a new boolean field in update, > e.g. update->truncate_reflog, is way overkill for this. This change only affects whether or not a reflog entry will be emitted by the update. msg==NULL means we will not create a new entry. This is orthogonal to whether we truncate the log or not. In order to truncate the reflog we do have a boolean in update->flags & REFLOG_TRUNCATE which determines whether the update will truncate the log or not. I see these two actions a) write a log entry and b) truncate the log as orthogonal and thus think we should have separate knobs for them. Currently, modulo bugs, the only caller that will use msg==NULL is when we do reflog truncations. Thus that codepath BOTH sets msg==NULL (to not write a new log entry) and also sets update->flags&REFLOG_TRUNCATE (to truncate the log). Having separate knobs for the two actions allow us the current behaviour with: msg==NULL update->flags&REFLOG_TRUNCATE but it will also allow a caller to do things like msg="truncated by foo because ..." update->flags&REFLOG_TRUNCATE If there is some future usecase where we want to truncate the log and then also generate a new initial log entry for the new log. I will work on the commit message to make the distinction between msg==NULL and update->flags&REFLOG_TRUNCATE more clear. thanks ronnie sahlberg > >> >> Change-Id: I44c89caa7e7c4960777b79cfb5d339a5aa3ddf7a >> Signed-off-by: Ronnie Sahlberg >> Signed-off-by: Jonathan Nieder >> --- >> refs.c | 5 +++-- >> refs.h | 1 + >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/refs.c b/refs.c >> index d54c3b9..f14b76e 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -3895,8 +3895,9 @@ int transaction_commit(struct transaction *transaction, >> update->reflog_fd = -1; >> continue; >> } >> - if (log_ref_write_fd(update->reflog_fd, update->old_sha1, >> - update->new_sha1, >> + if (update->msg && >> + log_ref_write_fd(update->reflog_fd, >> + update->old_sha1, update->new_sha1, >>update->committer, update->msg)) { >> error("Could write to reflog: %s. %s", >> update->refname, strerror(errno)); >> diff --git a/refs.h b/refs.h >> index 5075073..bf96b36 100644 >> --- a/refs.h >> +++ b/refs.h >> @@ -337,6 +337,7 @@ int transaction_delete_ref(struct transaction >> *transaction, >> /* >> * 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 transaction *transaction, >> const char *refname, -- 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 09/15] refs.c: only write reflog update if msg is non-NULL
Ronnie Sahlberg writes: > commit 020ed65a12838bdead64bc3c5de249d3c8f5cfd8 upstream. > > When performing a reflog transaction update, only write to the reflog iff > msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform > an update that only truncates but does not write. Does any existing caller call this codepath with update->msg == NULL? Will "please truncate" stay to be the only plausible special cause to call into this codepath without having any meaningful message? I am trying to make sure that this patch is not painting us into a corner where we will find out another reason for doing something esoteric in this codepath but need to find a way other than setting msg to NULL for the caller to trigger that new codepath. Put it in another way, please convince me that a new boolean field in update, e.g. update->truncate_reflog, is way overkill for this. > > Change-Id: I44c89caa7e7c4960777b79cfb5d339a5aa3ddf7a > Signed-off-by: Ronnie Sahlberg > Signed-off-by: Jonathan Nieder > --- > refs.c | 5 +++-- > refs.h | 1 + > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/refs.c b/refs.c > index d54c3b9..f14b76e 100644 > --- a/refs.c > +++ b/refs.c > @@ -3895,8 +3895,9 @@ int transaction_commit(struct transaction *transaction, > update->reflog_fd = -1; > continue; > } > - if (log_ref_write_fd(update->reflog_fd, update->old_sha1, > - update->new_sha1, > + if (update->msg && > + log_ref_write_fd(update->reflog_fd, > + update->old_sha1, update->new_sha1, >update->committer, update->msg)) { > error("Could write to reflog: %s. %s", > update->refname, strerror(errno)); > diff --git a/refs.h b/refs.h > index 5075073..bf96b36 100644 > --- a/refs.h > +++ b/refs.h > @@ -337,6 +337,7 @@ int transaction_delete_ref(struct transaction > *transaction, > /* > * 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 transaction *transaction, > const char *refname, -- 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 09/15] refs.c: only write reflog update if msg is non-NULL
commit 020ed65a12838bdead64bc3c5de249d3c8f5cfd8 upstream. When performing a reflog transaction update, only write to the reflog iff msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform an update that only truncates but does not write. Change-Id: I44c89caa7e7c4960777b79cfb5d339a5aa3ddf7a Signed-off-by: Ronnie Sahlberg Signed-off-by: Jonathan Nieder --- refs.c | 5 +++-- refs.h | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index d54c3b9..f14b76e 100644 --- a/refs.c +++ b/refs.c @@ -3895,8 +3895,9 @@ int transaction_commit(struct transaction *transaction, update->reflog_fd = -1; continue; } - if (log_ref_write_fd(update->reflog_fd, update->old_sha1, -update->new_sha1, + if (update->msg && + log_ref_write_fd(update->reflog_fd, +update->old_sha1, update->new_sha1, update->committer, update->msg)) { error("Could write to reflog: %s. %s", update->refname, strerror(errno)); diff --git a/refs.h b/refs.h index 5075073..bf96b36 100644 --- a/refs.h +++ b/refs.h @@ -337,6 +337,7 @@ int transaction_delete_ref(struct transaction *transaction, /* * 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 transaction *transaction, const char *refname, -- 2.1.0.rc2.206.gedb03e5 -- 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 09/15] refs.c: only write reflog update if msg is non-NULL
When performing a reflog transaction update, only write to the reflog iff msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform an update that only truncates but does not write. Signed-off-by: Ronnie Sahlberg --- refs.c | 5 +++-- refs.h | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index 181c957..c431088 100644 --- a/refs.c +++ b/refs.c @@ -3769,8 +3769,9 @@ int transaction_commit(struct ref_transaction *transaction, update->reflog_fd = -1; continue; } - if (log_ref_write_fd(update->reflog_fd, update->old_sha1, -update->new_sha1, + if (update->msg && + log_ref_write_fd(update->reflog_fd, +update->old_sha1, update->new_sha1, update->committer, update->msg)) { error("Could write to reflog: %s. %s", update->refname, strerror(errno)); diff --git a/refs.h b/refs.h index 66cf38b..a8b7047 100644 --- a/refs.h +++ b/refs.h @@ -330,6 +330,7 @@ int transaction_delete_sha1(struct ref_transaction *transaction, /* * 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, -- 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