Re: [PATCHv2 2/6] send-pack: Invert the return value of ref_update_to_be_sent

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

Stefan Beller <sbeller@google.com> writes:

> Having the return value inverted we can have different values for
> the error codes. This is useful in a later patch when we want to
> know if we hit the REF_STATUS_REJECT_* case.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
>     New in the series. For consistency with all the other patches
>     it also reads v2.

Sorry, but ECANNOTPARSE especially "it also read v2" part.

>  send-pack.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/send-pack.c b/send-pack.c
> index 2a513f4..1c4ac75 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -190,10 +190,10 @@ static void advertise_shallow_grafts_buf(struct strbuf *sb)
>  	for_each_commit_graft(advertise_shallow_grafts_cb, sb);
>  }
>  
> -static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args)
> +static int no_ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args)
>  {
>  	if (!ref->peer_ref && !args->send_mirror)
> -		return 0;
> +		return 1;
>  
>  	/* Check for statuses set by set_ref_status_for_push() */
>  	switch (ref->status) {
> @@ -203,10 +203,11 @@ static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_a
>  	case REF_STATUS_REJECT_NEEDS_FORCE:
>  	case REF_STATUS_REJECT_STALE:
>  	case REF_STATUS_REJECT_NODELETE:
> +		return 2;
>  	case REF_STATUS_UPTODATE:
> -		return 0;
> +		return 3;
>  	default:
> -		return 1;
> +		return 0;

Hmmm.  It used to be "will we send this one?"  It is fine if the
function wants to differenciate various kinds of error, but

 (1) unexplained 1, 2 and 3 looks cryptic and unmaintainable;

 (2) no_ prefix in the function name is usually a bad idea, because
     it easily invites "if (!no_foo(...))" double negation; and

 (3) unless there is a good reason to do otherwise, a function that
     returns 0 on success should signal an error with a negative
     return.

"static int check_to_send_update()" or something, perhaps?

	if (check_to_send_update() < 0)
        	/* skip as this is an error */
                continue;

does not look too bad.

>  }
>  
> @@ -250,7 +251,7 @@ static int generate_push_cert(struct strbuf *req_buf,
>  	strbuf_addstr(&cert, "\n");
>  
>  	for (ref = remote_refs; ref; ref = ref->next) {
> -		if (!ref_update_to_be_sent(ref, args))
> +		if (no_ref_update_to_be_sent(ref, args))
>  			continue;
>  		update_seen = 1;
>  		strbuf_addf(&cert, "%s %s %s\n",
> @@ -370,7 +371,7 @@ int send_pack(struct send_pack_args *args,
>  	 * the pack data.
>  	 */
>  	for (ref = remote_refs; ref; ref = ref->next) {
> -		if (!ref_update_to_be_sent(ref, args))
> +		if (no_ref_update_to_be_sent(ref, args))
>  			continue;
>  
>  		if (!ref->deletion)
> @@ -391,7 +392,7 @@ int send_pack(struct send_pack_args *args,
>  		if (args->dry_run || args->push_cert)
>  			continue;
>  
> -		if (!ref_update_to_be_sent(ref, args))
> +		if (no_ref_update_to_be_sent(ref, args))
>  			continue;
>  
>  		old_hex = sha1_to_hex(ref->old_sha1);