Junio C Hamano writes: > Karthik Nayak writes: > >> From: Karthik Nayak >> >> The function `create_symref_locked` creates a symref by creating a >> '.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)`. > > It is a confusing way to describe what this patch did, though. If > you truly splitted create_symref_locked() into two, you would have > functions A and B, and existing callers of create_symref_locked() > would be changed to call A() and then B(). I do not think such a > split would make sense in this case, but the above description gives > an impression that it was what you did. > > In reality, an early part of create_symref_locked() was made into a > separate helper function that can be called from callers other than > create_symref_locked(), and because the helper got a name too > similar to the original, you had to rename create_symref_locked() to > create_and_commit_symref(). The existing callers of it are not > affected, modulo the name change. > > Perhaps > > Split the early half of create_symref_locked() into a new helper > funciton 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 helper will be used to ... > > or something? > Thanks. I agree with what you're saying. I would also s/Split/Extract perhaps because it drives the point better. >> -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); > > This was a bit puzzling (see below). > >> + 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; >> } > > Offtopic: we might want to start planning to deprecate creation > of "symlink refs". Linus originally used a symlink for > .git/HEAD, but 9f0bb90d (core.prefersymlinkrefs: use symlinks > for .git/HEAD, 2006-05-02) made it default not to use of > symbolic links. As long as we preserve the ability to work on a > repository whose HEAD still uses a symbolic link, I'd hope > nothing would break (#leftoverbits). > > Let me rearrange this hunk to show the original first: > >> - if (!fdopen_lock_file(&lock->lk, "w")) >> - return error("unable to fdopen %s: %s", >> - get_lock_file_path(&lock->lk), strerror(errno)); >> - update_symref_reflog(refs, lock, refname, target, logmsg); >> - /* 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)); > > The original in create_symref_locked() created a lockfile, called > update_symref_reflog(), and called commit_ref() to commit the thing. > > The "no error check" comment is about detecting an error while > writing into the lock file. It came from 370e5ad6 (create_symref: > use existing ref-lock code, 2015-12-29). Because the fprintf() call > was immediately followed by commit_ref(), and the code assumed that > commit_ref() will check ferror(), we do not bother checking if the > fprintf() call fails to write the contents correctly. > >> + ret = create_symref_lock(refs, lock, refname, target); >> + if (!ret) { >> + 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)); >> + } > > The new code lets create_symref_lock() to create a lockfile, and > does the rest here. commit_ref() does call commit_lock_file(), > which eventually passes the control to close_tempfile() and a > write error can be detected there. > > But the point of this patch is that the creation of the locked > symref file PLUS writing its new contents (which is done by > create_symref_lock()) can be done way ahead of the remainder that > eventually does commit_ref(). So it smells a bit[*] dubious that we > still leave the error from fprintf() ignored in the "early half" in > the rearranged code. > > Side note: it is a "bit", as it is unlikely that we will do > something to clear the ferror() from the (FILE *) in the > meantime. > You're right. I would say that perhaps it is a bit more than a 'bit dubious' since `commit_ref()` being called after, is no longer a guarantee. I'll go ahead and add error handling here. >> @@ -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; >> } > > This hunk shows the "original function was renamed; there is no > other changes visible to the caller" nature of this rearrangement. > > The extra blank line is probably a nice touch. Thanks. I'm sure it's not the best idea to introduce whitespace, but this felt more readable here.