Re: [PATCH 07/31] refs.c: add a flag to allow reflog updates to truncate the log

[Ronnie Sahlberg <sahlberg@google.com>]

On Fri, May 16, 2014 at 2:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> Add a flag that allows us to truncate the reflog before we write the update.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>
> Until we read the callers it is hard to see how such a feature is
> useful, though.

Shortly later in the series comes the update to builtin/reflog.c which
uses this feature.
Even later in the series I also use this in builtin/reflog.c to both
do the ref update as well as the corresponding reflog updates all in
one single transaction.


>
> (style) The two multi-line comments are formatted differently.

Fixed. Thanks.

>
>>  refs.c | 12 ++++++++++--
>>  refs.h |  4 +++-
>>  2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index d8a1568..a8b583a 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3608,7 +3608,11 @@ int transaction_commit(struct ref_transaction *transaction,
>>               }
>>       }
>>
>> -     /* Update all reflog files */
>> +     /* Update all reflog files
>> +        We have already done all ref updates and deletes.
>> +        There is not much we can do here if there are any reflog
>> +        update errors, other than complain.
>> +     */
>>       for (i = 0; i < n; i++) {
>>               struct ref_update *update = updates[i];
>>
>> @@ -3616,7 +3620,11 @@ int transaction_commit(struct ref_transaction *transaction,
>>                       continue;
>>               if (update->reflog_fd == -1)
>>                       continue;
>> -
>> +             if (update->flags & REFLOG_TRUNCATE)
>> +                     if (lseek(update->reflog_fd, 0, SEEK_SET) < 0 ||
>> +                         ftruncate(update->reflog_fd, 0)) {
>> +                             error("Could not truncate reflog");
>> +                     }
>
> The {} looks somewhat curious here.  If you seeked to the beginning,
> and failed to truncate, do you still want to go on without "return"
> in front of the error()?  Would that overwrite existing entries?

I have changed it to do this :
+               if (update->flags & REFLOG_TRUNCATE)
+                       if (lseek(update->reflog_fd, 0, SEEK_SET) < 0 ||
+                               ftruncate(update->reflog_fd, 0)) {
+                               error("Could not truncate reflog: %s",
+                                     update->refname);
+                               rollback_lock_file(&update->reflog_lock);
+                               update->reflog_fd = -1;
+                       }


At this point we have already committed all the changes to refs and
also performed all unlinks for refs we are deleting. So if things
start to go wrong here during the reflog updates it is too late to do
much more than complain.


>
>>               if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
>>                                    update->new_sha1,
>>                                    update->committer, update->msg)) {
>> diff --git a/refs.h b/refs.h
>> index 2e22a26..b1b97e7 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -269,8 +269,10 @@ int transaction_delete_sha1(struct ref_transaction *transaction,
>>                           const unsigned char *old_sha1,
>>                           int flags, int have_old, const char *msg);
>>
>> +#define REFLOG_TRUNCATE 0x01
>>  /*
>> - * Append a reflog entry for refname.
>> + * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
>> + * this update will first truncate the reflog before writing the entry.
>>   */
>>  int transaction_update_reflog(struct ref_transaction *transaction,
>>                             const char *refname,