Re: [PATCH v3 05/44] refs.c: move update_ref to refs.c

[Michael Haggerty <mhagger@alum.mit.edu>]

On 10/12/2015 11:51 PM, David Turner wrote:
> From: Ronnie Sahlberg <sahlberg@google.com>
> 
> Move update_ref() to the refs.c file since this function does not
> contain any backend specific code.  Move the ref classifier functions
> as well, since update_ref depends on them.
> 
> Based on Ronnie Sahlberg's patch
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  refs-be-files.c | 117 +-------------------------------------------------------
>  refs.c          | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 116 insertions(+), 116 deletions(-)
> 
> diff --git a/refs-be-files.c b/refs-be-files.c
> index d0dfdfc..7fe4931 100644
> --- a/refs-be-files.c
> +++ b/refs-be-files.c
> [...]
> @@ -2879,77 +2877,6 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
> [...]
> -static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
> -			   const unsigned char *old_sha1, struct strbuf *err)
> -{
> -	const char *filename;
> -	int fd;
> -	static struct lock_file lock;
> -	struct strbuf buf = STRBUF_INIT;
> -	int ret = -1;
> -
> -	strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
> -
> -	filename = git_path("%s", pseudoref);
> -	fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
> -	if (fd < 0) {
> -		strbuf_addf(err, "Could not open '%s' for writing: %s",
> -			    filename, strerror(errno));
> -		return -1;
> -	}
> -
> -	if (old_sha1) {
> -		unsigned char actual_old_sha1[20];
> -
> -		if (read_ref(pseudoref, actual_old_sha1))
> -			die("could not read ref '%s'", pseudoref);
> -		if (hashcmp(actual_old_sha1, old_sha1)) {
> -			strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
> -			rollback_lock_file(&lock);
> -			goto done;
> -		}
> -	}
> -
> -	if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
> -		strbuf_addf(err, "Could not write to '%s'", filename);
> -		rollback_lock_file(&lock);
> -		goto done;
> -	}
> -
> -	commit_lock_file(&lock);
> -	ret = 0;
> -done:
> -	strbuf_release(&buf);
> -	return ret;
> -}
> -
> [...]
> diff --git a/refs.c b/refs.c
> index 77492ff..2d10708 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1,3 +1,118 @@
> [...]
> +static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
> +			   const unsigned char *old_sha1, struct strbuf *err)
> +{
> +	const char *filename;
> +	int fd;
> +	static struct lock_file lock;
> +	struct strbuf buf = STRBUF_INIT;
> +	int ret = -1;
> +
> +	strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
> +
> +	filename = git_path("%s", pseudoref);
> +	fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
> +	if (fd < 0) {
> +		strbuf_addf(err, "Could not open '%s' for writing: %s",
> +			    filename, strerror(errno));
> +		return -1;
> +	}
> +
> +	if (old_sha1) {
> +		unsigned char actual_old_sha1[20];
> +		read_ref(pseudoref, actual_old_sha1);

The original read

		if (read_ref(pseudoref, actual_old_sha1))
			die("could not read ref '%s'", pseudoref);

This seems like an important test. What happened to it?

If its removal was intentional, it deserves a careful explanation (and
should probably be done as a separate commit). If it was an accident,
please consider how this accident arose and try to think about whether
similar accidents might have happened elsewhere in this series.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu