From: "Martin Ågren" <martin.agren@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
Git Mailing List <git@vger.kernel.org>,
Christian Couder <christian.couder@gmail.com>
Subject: Re: [PATCH] pack-objects: handle island check for "external" delta base
Date: Wed, 19 Sep 2018 20:34:05 +0200 [thread overview]
Message-ID: <CAN0heSpMRfDd7=E_kYL6ucL9bL2VWMGLTAkT72BUrVwR6YTezQ@mail.gmail.com> (raw)
In-Reply-To: <20180919034907.GA7626@sigill.intra.peff.net>
On Wed, 19 Sep 2018 at 05:49, Jeff King <peff@peff.net> wrote:
> This is tricky to do inside a single "if" statement. And
> after the merge in f3504ea3dd (Merge branch
> 'cc/delta-islands', 2018-09-17), that "if" condition is
> already getting pretty unwieldy. So this patch moves the
> logic into a helper function, where we can easily use
> multiple return paths. The result is a bit longer, but the
> logic should be much easier to follow.
> +static int can_reuse_delta(const unsigned char *base_sha1,
> + struct object_entry *delta,
> + struct object_entry **base_out)
> +{
> + struct object_entry *base;
> +
> + if (!base_sha1)
> + return 0;
So this corresponds to "if (base_ref &&".
> + /*
> + * First see if we're already sending the base (or it's explicitly in
> + * our "excluded" list.
> + */
Missing ')'.
> + base = packlist_find(&to_pack, base_sha1, NULL);
> + if (base) {
> + if (!in_same_island(&delta->idx.oid, &base->idx.oid))
> + return 0;
This logic matches the removed code...
> + *base_out = base;
> + return 1;
> + }
> +
> + /*
> + * Otherwise, reachability bitmaps may tell us if the receiver has it,
> + * even if it was buried too deep in history to make it into the
> + * packing list.
> + */
> + if (thin && bitmap_has_sha1_in_uninteresting(bitmap_git, base_sha1)) {
This matches...
> + if (use_delta_islands) {
> + struct object_id base_oid;
> + hashcpy(base_oid.hash, base_sha1);
> + if (!in_same_island(&delta->idx.oid, &base_oid))
> + return 0;
This does some extra juggling to avoid using `base->idx.oid`, which
would have been the moral equivalent of the original code, but which
won't fly since `base` is NULL.
> + }
> + *base_out = NULL;
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> static void check_object(struct object_entry *entry)
> {
> unsigned long canonical_size;
> @@ -1556,22 +1607,7 @@ static void check_object(struct object_entry *entry)
> break;
> }
>
> - if (base_ref && (
> - (base_entry = packlist_find(&to_pack, base_ref, NULL)) ||
> - (thin &&
> - bitmap_has_sha1_in_uninteresting(bitmap_git, base_ref))) &&
> - in_same_island(&entry->idx.oid, &base_entry->idx.oid)) {
Yeah, the new function looks much simpler than this. We have
if (A && (B1 || B2) && C) {.
Knowing what to look for, it can be seen that we can -- under the right
circumstances -- have A and B2, but not B1, and try to evalute C by
dereferencing `base_entry` which will be NULL.
> + if (can_reuse_delta(base_ref, entry, &base_entry)) {
> oe_set_type(entry, entry->in_pack_type);
> SET_SIZE(entry, in_pack_size); /* delta size */
> SET_DELTA_SIZE(entry, in_pack_size);
Without being at all familiar with this code, this looks sane to me.
Just had a small nit about the missing closing ')'.
Martin
next prev parent reply other threads:[~2018-09-19 18:34 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-14 21:56 What's cooking in git.git (Sep 2018, #03; Fri, 14) Junio C Hamano
2018-09-15 20:17 ` Antonio Ospite
2018-09-17 15:11 ` Junio C Hamano
2018-09-16 6:39 ` Duy Nguyen
2018-09-16 15:09 ` Ævar Arnfjörð Bjarmason
2018-09-17 2:39 ` Jeff King
2018-09-17 17:51 ` Junio C Hamano
2018-09-17 18:22 ` Jeff King
2018-09-17 18:26 ` Jonathan Nieder
2018-09-16 12:01 ` brian m. carlson
2018-09-17 15:10 ` Junio C Hamano
2018-09-17 18:35 ` Derrick Stolee
2018-09-17 19:04 ` Junio C Hamano
2018-09-17 22:54 ` Junio C Hamano
2018-09-19 3:49 ` [PATCH] pack-objects: handle island check for "external" delta base Jeff King
2018-09-19 18:34 ` Martin Ågren [this message]
2018-09-19 18:43 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAN0heSpMRfDd7=E_kYL6ucL9bL2VWMGLTAkT72BUrVwR6YTezQ@mail.gmail.com' \
--to=martin.agren@gmail.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).