diff mbox series

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

Message ID 20240426152449.228860-3-knayak@gitlab.com (mailing list archive)
State New, archived
Headers show
Series add symref-* commands to 'git-update-ref --stdin' | expand

Commit Message

karthik nayak April 26, 2024, 3:24 p.m. UTC 
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(-)

Comments

Junio C Hamano April 26, 2024, 9:39 p.m. UTC | #1 
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.
karthik nayak April 28, 2024, 7:57 p.m. UTC | #2 
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.
diff mbox series

Patch

diff --git a/refs/files-backend.c b/refs/files-backend.c
index e4d0aa3d41..2420dac2aa 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;
 }