diff mbox series

[v5,2/7] files-backend: extract out `create_symref_lock()`

Message ID 20240501202229.2695774-3-knayak@gitlab.com (mailing list archive)
State Superseded
Headers show
Series refs: add support for transactional symref updates | expand

Commit Message

karthik nayak May 1, 2024, 8:22 p.m. UTC
From: Karthik Nayak <karthik.188@gmail.com>

The function `create_symref_locked()` creates a symref by creating a
'<symref>.lock' file and then committing the symref lock, which creates
the final symref.

Extract the early half of `create_symref_locked()` into a new helper
function `create_symref_lock()`. Because the name of the new function is
too similar to the original, rename the original to
`create_and_commit_symref()` to avoid confusion.

The new function `create_symref_locked()` can be used to create the
symref lock in a separate step from that of committing it. This allows
to add transactional support for symrefs, where the lock would be
created in the preparation step and the lock would be committed in the
finish step.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

Comments

Junio C Hamano May 1, 2024, 10:06 p.m. UTC | #1
Karthik Nayak <karthik.188@gmail.com> writes:

> +	if (!fdopen_lock_file(&lock->lk, "w"))
> +		return error("unable to fdopen %s: %s",
> +			     get_lock_file_path(&lock->lk), strerror(errno));
> +
> +	if (fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target) < 0)
> +		return error("unable to fprintf %s: %s",
> +			     get_lock_file_path(&lock->lk), strerror(errno));

error() is end-user facing, so "fprintf" is probably a bit too
precise?  "fprintf" -> "write to"

Also we may want to make them (not just this new message but other
error() messages in related code paths) localizable but that is
probably beyond the scope of this topic.
Patrick Steinhardt May 2, 2024, 7:47 a.m. UTC | #2
On Wed, May 01, 2024 at 03:06:19PM -0700, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
> 
> > +	if (!fdopen_lock_file(&lock->lk, "w"))
> > +		return error("unable to fdopen %s: %s",
> > +			     get_lock_file_path(&lock->lk), strerror(errno));
> > +
> > +	if (fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target) < 0)
> > +		return error("unable to fprintf %s: %s",
> > +			     get_lock_file_path(&lock->lk), strerror(errno));
> 
> error() is end-user facing, so "fprintf" is probably a bit too
> precise?  "fprintf" -> "write to"
> 
> Also we may want to make them (not just this new message but other
> error() messages in related code paths) localizable but that is
> probably beyond the scope of this topic.

It only occurred to me now, but shouldn't we also support passing in a
`struct strbuf *err` here? The transactional code doesn't want us to
print error messages to `stderr`, but always supplies a buffer.

Patrick
karthik nayak May 2, 2024, 11:05 a.m. UTC | #3
Patrick Steinhardt <ps@pks.im> writes:

> On Wed, May 01, 2024 at 03:06:19PM -0700, Junio C Hamano wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>> > +	if (!fdopen_lock_file(&lock->lk, "w"))
>> > +		return error("unable to fdopen %s: %s",
>> > +			     get_lock_file_path(&lock->lk), strerror(errno));
>> > +
>> > +	if (fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target) < 0)
>> > +		return error("unable to fprintf %s: %s",
>> > +			     get_lock_file_path(&lock->lk), strerror(errno));
>>
>> error() is end-user facing, so "fprintf" is probably a bit too
>> precise?  "fprintf" -> "write to"
>>
>> Also we may want to make them (not just this new message but other
>> error() messages in related code paths) localizable but that is
>> probably beyond the scope of this topic.
>
> It only occurred to me now, but shouldn't we also support passing in a
> `struct strbuf *err` here? The transactional code doesn't want us to
> print error messages to `stderr`, but always supplies a buffer.
>
> Patrick

Yes I think that would fit better with the existing transaction code.
Junio C Hamano May 2, 2024, 4:49 p.m. UTC | #4
Patrick Steinhardt <ps@pks.im> writes:

> On Wed, May 01, 2024 at 03:06:19PM -0700, Junio C Hamano wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>> 
>> > +	if (!fdopen_lock_file(&lock->lk, "w"))
>> > +		return error("unable to fdopen %s: %s",
>> > +			     get_lock_file_path(&lock->lk), strerror(errno));
>> > +
>> > +	if (fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target) < 0)
>> > +		return error("unable to fprintf %s: %s",
>> > +			     get_lock_file_path(&lock->lk), strerror(errno));
>> 
>> error() is end-user facing, so "fprintf" is probably a bit too
>> precise?  "fprintf" -> "write to"
>> 
>> Also we may want to make them (not just this new message but other
>> error() messages in related code paths) localizable but that is
>> probably beyond the scope of this topic.
>
> It only occurred to me now, but shouldn't we also support passing in a
> `struct strbuf *err` here? The transactional code doesn't want us to
> print error messages to `stderr`, but always supplies a buffer.

Sounds sensible.  Thanks.
diff mbox series

Patch

diff --git a/refs/files-backend.c b/refs/files-backend.c
index e4d0aa3d41..878601ced0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1920,27 +1920,41 @@  static void update_symref_reflog(struct files_ref_store *refs,
 	}
 }
 
-static int create_symref_locked(struct files_ref_store *refs,
-				struct ref_lock *lock, const char *refname,
-				const char *target, const char *logmsg)
+static int create_symref_lock(struct files_ref_store *refs,
+			      struct ref_lock *lock, const char *refname,
+			      const char *target)
 {
+	if (!fdopen_lock_file(&lock->lk, "w"))
+		return error("unable to fdopen %s: %s",
+			     get_lock_file_path(&lock->lk), strerror(errno));
+
+	if (fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target) < 0)
+		return error("unable to fprintf %s: %s",
+			     get_lock_file_path(&lock->lk), strerror(errno));
+	return 0;
+}
+
+static int create_and_commit_symref(struct files_ref_store *refs,
+				    struct ref_lock *lock, const char *refname,
+				    const char *target, const char *logmsg)
+{
+	int ret;
+
 	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
 		update_symref_reflog(refs, lock, refname, target, logmsg);
 		return 0;
 	}
 
-	if (!fdopen_lock_file(&lock->lk, "w"))
-		return error("unable to fdopen %s: %s",
-			     get_lock_file_path(&lock->lk), strerror(errno));
+	ret = create_symref_lock(refs, lock, refname, target);
+	if (!ret) {
+		update_symref_reflog(refs, lock, refname, target, logmsg);
 
-	update_symref_reflog(refs, lock, refname, target, logmsg);
+		if (commit_ref(lock) < 0)
+			return error("unable to write symref for %s: %s", refname,
+				     strerror(errno));
+	}
 
-	/* no error check; commit_ref will check ferror */
-	fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target);
-	if (commit_ref(lock) < 0)
-		return error("unable to write symref for %s: %s", refname,
-			     strerror(errno));
-	return 0;
+	return ret;
 }
 
 static int files_create_symref(struct ref_store *ref_store,
@@ -1960,7 +1974,8 @@  static int files_create_symref(struct ref_store *ref_store,
 		return -1;
 	}
 
-	ret = create_symref_locked(refs, lock, refname, target, logmsg);
+	ret = create_and_commit_symref(refs, lock, refname, target, logmsg);
+
 	unlock_ref(lock);
 	return ret;
 }