| Message ID | 20240501202229.2695774-3-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: add support for transactional symref updates | expand |
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.
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
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.
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 --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; }