Re: [PATCH v4 2/7] files-backend: extract out `create_symref_lock`

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> 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)`.

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?

> -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.

> @@ -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.