On Fri, May 20, 2022 at 01:24:28PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, May 20 2022, Frantisek Hrbata wrote:
>
> > Remove the big indented block for push_refs() check in transport vtable
> > and let's just return error immediately. Hopefully this makes the code
> > more readable.
>
> s/push_refs/transport_push/, push_refs is the name of the field in the
> callback (and there's more than just this function).
uf, I will fix that, thank you for noticing.
>
> This looks good to me....
>
> > Is there a reason to return 1 instead of -1 when push_refs() is not
> > set in transport vtable? Looking at the code I think it might return
> > -1 also and make it more consistent.
>
> No, looking at it (I tried renaming the function) the only user is
> builtin/push.c, which we can easily see doesn't care about the 1 v.s. -1 here.
>
> Perhaps it's worthwhile to add this in-between the two patches you have:
>
> diff --git a/transport.c b/transport.c
> index 0b9c5a427d7..5348fac36ef 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1283,22 +1283,23 @@ int transport_push(struct repository *r,
> int quiet = (transport->verbose < 0);
> int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
> int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
> - int push_ret, ret, err;
> + int push_ret, err;
> + int ret = -1;
> struct transport_ls_refs_options transport_options =
> TRANSPORT_LS_REFS_OPTIONS_INIT;
>
> *reject_reasons = 0;
>
> if (transport_color_config() < 0)
> - return -1;
> + goto done;
>
> if (!transport->vtable->push_refs)
> - return 1;
> + goto done;
>
> local_refs = get_local_heads();
>
> if (check_push_refs(local_refs, rs) < 0)
> - return -1;
> + goto done;
>
> refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
>
> @@ -1319,7 +1320,7 @@ int transport_push(struct repository *r,
> match_flags |= MATCH_REFS_FOLLOW_TAGS;
>
> if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
> - return -1;
> + goto done;
>
> if (transport->smart_options &&
> transport->smart_options->cas &&
> @@ -1333,7 +1334,7 @@ int transport_push(struct repository *r,
>
> if (!(flags & TRANSPORT_PUSH_NO_HOOK))
> if (run_pre_push_hook(transport, remote_refs))
> - return -1;
> + goto done;
>
> if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
> TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
> @@ -1417,6 +1418,7 @@ int transport_push(struct repository *r,
> else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
> fprintf(stderr, "Everything up-to-date\n");
>
> +done:
> return ret;
> }
>
> Which would make your new 3/3 truly trivial, i.e. just adding the
> free_refs() for the two.
I was thinking about this too. Just use the done label as a single exit point.
Let me incorporate your suggestions in v3. I will add a 2/3 patch e.g.
"unify return values and exit point for transport_push()"
>
> *Maybe* (but probably not) it would then make sense to do this as that 3/3:
>
> diff --git a/transport.c b/transport.c
> index 5348fac36ef..d4952bf5f6a 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1291,15 +1291,17 @@ int transport_push(struct repository *r,
> *reject_reasons = 0;
>
> if (transport_color_config() < 0)
> - goto done;
> + return ret;
>
> if (!transport->vtable->push_refs)
> - goto done;
> + return ret;
>
> local_refs = get_local_heads();
>
> - if (check_push_refs(local_refs, rs) < 0)
> + if (check_push_refs(local_refs, rs) < 0) {
> + remote_refs = NULL;
> goto done;
> + }
>
> refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
>
> @@ -1419,6 +1421,8 @@ int transport_push(struct repository *r,
> fprintf(stderr, "Everything up-to-date\n");
>
> done:
> + free_refs(local_refs);
> + free_refs(remote_refs);
> return ret;
> }
>
> I.e. entirely skip the NULL assignment for the two, which helps the
> compiler catch if we don't init it before the later goto's, but because
> of that we'd need to "return" instead of "goto done" for the early ones,
> and set it for the check_push_refs() failure case.
>
> So nah, probably best just to keep it as you have it, i.e. always "goto
> done".
I agree. Let me prepare v3 and let's see what happens :)
Thank you for your input!
--
Frantisek Hrbata
