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

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6127 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]