Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>>
>> 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)`.
>
> 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.
Attachment:
signature.asc
Description: PGP signature
