| Message ID | 20240330224623.579457-2-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | update-ref: add support for update-symref option | expand |
On Sat, Mar 30, 2024 at 11:46:16PM +0100, Karthik Nayak wrote: > 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. > > Split this into two individual functions `create_and_commit_symref` and > `create_symref_locked`. This way we can create the symref lock and > commit it at different times. This will be used to provide symref > support in `git-update-ref(1)`. > > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > refs/files-backend.c | 40 +++++++++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 13 deletions(-) > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index a098d14ea0..3f0f9521cb 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -1920,26 +1920,39 @@ 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)); > + > + /* no error check; commit_ref will check ferror */ > + fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target); > + 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); I feel like the resulting code here is a bit hard to read because the successful path is now nested into the condition. This does not really conform to our typical coding style. Exiting early in case the function returns an error would be easier to read. > - 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; Also, is it correct to `return 0` here in case `create_symref_lock()` returns an error? If so it certainly requires an in-code comment to explain what is going on. If this is a bug I feel like we have identified a test gap that we might want to plug. Patrick > } > > @@ -1960,7 +1973,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; > } > -- > 2.43.GIT >
Patrick Steinhardt <ps@pks.im> writes: >> +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); > > I feel like the resulting code here is a bit hard to read because the > successful path is now nested into the condition. This does not really > conform to our typical coding style. Exiting early in case the function > returns an error would be easier to read. Agreed, will modify this to exit early. >> - 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; > > Also, is it correct to `return 0` here in case `create_symref_lock()` > returns an error? If so it certainly requires an in-code comment to > explain what is going on. If this is a bug I feel like we have > identified a test gap that we might want to plug. > It's wrong, we should definitely be catching and returning that error. Regarding fixing the test gap, it is kinda hard to do so, since this is capturing a filesystem error. It would be easier to do so in the upcoming commits where I could possibly do: 1. Start transaction 2. Add symref creation to transaction 3. Before preparing the transaction, manually create the lock file. 4. This should hit this error. I'll add something in corresponding commit.
diff --git a/refs/files-backend.c b/refs/files-backend.c index a098d14ea0..3f0f9521cb 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1920,26 +1920,39 @@ 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)); + + /* no error check; commit_ref will check ferror */ + fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target); + 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; } @@ -1960,7 +1973,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; }