Re: [PATCHv2 2/6] send-pack: Invert the return value of ref_update_to_be_sent
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 16, 2014, 19:14 UTC
- Message-ID
- <xmqq7fxrbe4w.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1418755747-22506-2-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 10 quoted lines
> 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.
Show 31 quoted lines
> 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.
Show 29 quoted lines
> }
>
> @@ -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);