Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions

[David Turner <dturner@twopensource.com>]

On Tue, 2015-08-11 at 15:47 -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > David Turner <dturner@twopensource.com> writes:
> >
> >> On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote:
> >>> I am sorry for being late to the review, I looked into coverity today as Duy
> >>> bugged me to fix the memory allocation stuff[1]
> >>
> >> Thanks. Junio, can you pleas substitute the attached patch instead?
> >
> > No.  The topic is already in 'next', no?
> 
> Yes, the topic is already in 'next'.  A follow-up fix would be good.
> 
> The patch didn't apply cleanly on top of 74ec19d^ to replace 74ec19d
> anyway, so I was about to discard it, but after conflict resolution,
> the interdiff turns out just these two hunks.
> 
> -- >8 --
> Subject: pseudoref: check return values from read_ref()
> From: David Turner <dturner@twopensource.com>
> Date: Wed, 15 Jul 2015 18:05:28 -0400
> 
> These codepaths attempt to compare the "expected" current value with
> the actual current value, but did not check if we successfully read
> the current value before comparison.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * It is likely that we would end up comparing the expected value with
>    garbage when the read fails, and the most likely outcome is that
>    they do not match and we fail the transaction, which is all fine.
> 
>    So in that sense, this is not all that urgent, but it is nice to
>    fix it when we know the code is not kosher.
> 
>  refs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 522b19b..1db3654 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2868,7 +2868,9 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
>  
>  	if (old_sha1) {
>  		unsigned char actual_old_sha1[20];
> -		read_ref(pseudoref, actual_old_sha1);
> +
> +		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);
> @@ -2904,7 +2906,8 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1
>  					       LOCK_DIE_ON_ERROR);
>  		if (fd < 0)
>  			die_errno(_("Could not open '%s' for writing"), filename);
> -		read_ref(pseudoref, actual_old_sha1);
> +		if (read_ref(pseudoref, actual_old_sha1))
> +			die("could not read ref '%s'", pseudoref);
>  		if (hashcmp(actual_old_sha1, old_sha1)) {
>  			warning("Unexpected sha1 when deleting %s", pseudoref);
>  			rollback_lock_file(&lock);
> 

LGTM.