Re: [PATCH 4/8] refs.c: add transaction function to append to the reflog

[Junio C Hamano]

Stefan Beller sbel...@google.com writes:

 Unlike transaction_update_ref, this writes out the proposed contents of the
 reflog to a temporary file at transaction_reflog_update time instead of
 waiting for the transaction waiting to be committed. This avoids an
 explosion of memory usage when writing lots of reflog updates within a
 single transaction.

Copying an existing reflog with thousands of entries over so that I
can append a single new entry, just so that I can rollback by not
renaming?

After ensuring that you are the only process that holds a write fd
to the reflog file (e.g. by taking a lock on the ref itself),
shouldn't you be able to ftell(), write() and then truncate() to
roll back sanely before close()?  After all you are not protecting
from power loss and other kinds of glitches that would leave *.lock
file behind, so...
--
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 4/8] refs.c: add transaction function to append to the reflog

[Stefan Beller]

Currently a transaction can include one or more reflog updates. But these
are not part of the contract of the transaction committing all or nothing.

Introduce a function transaction_update_reflog, which adds a reflog update
to a transaction. Later patches will update code that writes to reflogs to
use this function.

This will allow us to write code such as:

t = transaction_begin()
transaction_truncate_reflog(t, foo); // introduced by next patch
loop-over-something...
if (want_reflog_entry(...))
transaction_reflog_update(t, foo, 0, message);
transaction_commit(t)

transaction_ref_update still updates the reflog along with its ref update.
A later patch will make it use transaction_update_reflog internally.

Unlike transaction_update_ref, this writes out the proposed contents of the
reflog to a temporary file at transaction_reflog_update time instead of
waiting for the transaction waiting to be committed. This avoids an
explosion of memory usage when writing lots of reflog updates within a
single transaction.

This requires changing where the temporary file is located. If the temporary
file is located at $GIT_DIR/logs/$REF_NAME.lock, then a transaction that
tries to remove refs/heads/foo and introduce refs/heads/foo/bar would run
into a directory/file conflict when trying to write to
$GIT_DIR/logs/refs/heads/foo/bar.lock because the reflog for branch foo is
not safe to remove yet.

We work around this by placing the temporary files at
$GIT_DIR/logs/lock/$REF_NAME.lock Putting the temporary file under .git/logs
ensures it's likely that it will be in the same file system as the reflogs
and can be atomically renamed into place.

During one transaction we allow to make multiple reflog updates to the same
ref. This means we only need to lock the reflog once during the first update
that touches the reflog and that all further updates can just write the
reflog entry since the reflog is already locked.

transaction_update_reflog should lock the corresponding ref to avoid having
to compete with other reflog updates, but this will be deferred to a later
patch for simplicity.

Signed-off-by: Stefan Beller sbel...@google.com
---

Notes:
This has been sent to the list before, but now it is split up
into this and the next patch.
The commit message was overhauled, a huge thanks to Jonathan!

There are no major changes in the code.

 refs.c | 94 --
 1 file changed, 92 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index aad44d5..d767418 100644
--- a/refs.c
+++ b/refs.c
@@ -3559,19 +3559,30 @@ struct transaction {
struct ref_update **ref_updates;
size_t alloc;
size_t nr;
+
+   /*
+* Sorted list of reflogs to be committed.
+* the util points to the lock_file.
+*/
+   struct string_list reflog_updates;
enum transaction_state state;
 };
 
 struct transaction *transaction_begin(struct strbuf *err)
 {
+   struct transaction *ret;
+
assert(err);
 
-   return xcalloc(1, sizeof(struct transaction));
+   ret = xcalloc(1, sizeof(struct transaction));
+   ret-reflog_updates.strdup_strings = 1;
+   return ret;
 }
 
 void transaction_free(struct transaction *transaction)
 {
int i;
+   struct string_list_item *item;
 
if (!transaction)
return;
@@ -3581,6 +3592,12 @@ void transaction_free(struct transaction *transaction)
free(transaction-ref_updates[i]);
}
free(transaction-ref_updates);
+
+   for_each_string_list_item(item, transaction-reflog_updates) {
+   rollback_lock_file(item-util);
+   }
+   string_list_clear(transaction-reflog_updates, 0);
+
free(transaction);
 }
 
@@ -3631,6 +3648,71 @@ int transaction_update_ref(struct transaction 
*transaction,
return 0;
 }
 
+/*
+ * Append a reflog entry for refname.
+ */
+static int transaction_update_reflog(struct 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,
+struct strbuf *err)
+{
+   struct lock_file *lock;
+   struct strbuf buf = STRBUF_INIT;
+   struct string_list_item *item;
+
+   if (transaction-state != TRANSACTION_OPEN)
+   die(BUG: update_reflog called for transaction that is not 
open);
+
+   item = string_list_insert(transaction-reflog_updates, refname);
+   if (!item-util) {
+   int infd;
+   char *path = git_path(logs/locks/%s, refname);
+   lock = xcalloc(1,