Re: [PATCH 1/4] check_connected(): delay opening new_pack

>
> In check_connected(), if the transport tells us we got a single packfile
> that has already been verified as self-contained and connected, then we
> can skip checking connectivity for any tips that are mentioned in that
> pack. This goes back to c6807a40dc (clone: open a shortcut for
> connectivity check, 2013-05-26).
>
> We don't need to open that pack until we are about to start sending oids
> to our child rev-list process, since that's when we check whether they
> are in the self-contained pack. Let's push the opening of that pack
> further down in the function. That saves us from having to clean it up
> when we leave the function early (and by the time have opened the
> rev-list process, we never leave the function early, since we have to
> clean up the child process).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> One thing I noticed here is that for a clone with a single
> self-contained pack, we could probably skip running rev-list entirely. I
> don't know if it matters much, though, as a noop rev-list process is not
> that expensive compared to the cost of a clone. And in the worst case,
> it would involve calling find_pack_entry() on each proposed ref tip an
> extra time only to find that at least one does need to be sent. Though
> that is also not very expensive.
>
> I left it out of this series, though it would involve moving the
> new_pack opening up above the start_command() invocation again.
>
> I also wondered if this whole thing out to be written to avoid a one-off
> packed_git in the first place, like:
>
>   - call reprepare_packed_git() to re-scan objects/pack
>
>   - find the pack by name in the packed_git list
>
>   - don't clean it up; it's owned by the repository struct now
>
> But that's a somewhat bigger change, and I'm not sure it really buys us
> that much.

>
>  connected.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/connected.c b/connected.c
> index 79403108dd..530357de54 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -45,20 +45,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>                 return err;
>         }
>
> -       if (transport && transport->smart_options &&
> -           transport->smart_options->self_contained_and_connected &&
> -           transport->pack_lockfiles.nr == 1 &&
> -           strip_suffix(transport->pack_lockfiles.items[0].string,
> -                        ".keep", &base_len)) {
> -               struct strbuf idx_file = STRBUF_INIT;
> -               strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,
> -                          base_len);
> -               strbuf_addstr(&idx_file, ".idx");
> -               new_pack = add_packed_git(the_repository, idx_file.buf,
> -                                         idx_file.len, 1);
> -               strbuf_release(&idx_file);
> -       }
> -
>         if (repo_has_promisor_remote(the_repository)) {
>                 /*
>                  * For partial clones, we don't want to have to do a regular
> @@ -90,7 +76,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>  promisor_pack_found:
>                         ;
>                 } while ((oid = fn(cb_data)) != NULL);
> -               free(new_pack);
>                 return 0;
>         }
>
> @@ -127,15 +112,27 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>         else
>                 rev_list.no_stderr = opt->quiet;
>
> -       if (start_command(&rev_list)) {
> -               free(new_pack);
> +       if (start_command(&rev_list))
>                 return error(_("Could not run 'git rev-list'"));
> -       }
>
>         sigchain_push(SIGPIPE, SIG_IGN);
>
>         rev_list_in = xfdopen(rev_list.in, "w");
>
> +       if (transport && transport->smart_options &&
> +           transport->smart_options->self_contained_and_connected &&
> +           transport->pack_lockfiles.nr == 1 &&
> +           strip_suffix(transport->pack_lockfiles.items[0].string,
> +                        ".keep", &base_len)) {
> +               struct strbuf idx_file = STRBUF_INIT;
> +               strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,
> +                          base_len);
> +               strbuf_addstr(&idx_file, ".idx");
> +               new_pack = add_packed_git(the_repository, idx_file.buf,
> +                                         idx_file.len, 1);
> +               strbuf_release(&idx_file);
> +       }
> +
>         do {
>                 /*
>                  * If index-pack already checked that:
> --
> 2.53.0.786.g466665faa3
>
>