All of lore.kernel.org
 help / color / mirror / Atom feed
From: Derrick Stolee <derrickstolee@github.com>
To: "Jeff King" <peff@peff.net>, 程洋 <chengyang@xiaomi.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	何浩 <hehao@xiaomi.com>, "Xin7 Ma 马鑫" <maxin7@xiaomi.com>,
	石奉兵 <shifengbing@xiaomi.com>, 凡军辉 <fanjunhui@xiaomi.com>,
	王汉基 <wanghanji@xiaomi.com>
Subject: Re: [PATCH 1/3] parse_object(): allow skipping hash check
Date: Wed, 7 Sep 2022 10:15:37 -0400	[thread overview]
Message-ID: <f79b0ccd-3e36-f447-0dbb-6e40ad547c8d@github.com> (raw)
In-Reply-To: <YxfRTubqh7aFvNJs@coredump.intra.peff.net>

On 9/6/2022 7:01 PM, Jeff King wrote:
> I'm sorry, I know the argument here is really hand-wavy. But I really
> think this isn't making anything much less safe.

I agree with you that this is the safest way to move forward here.
 
> I was actually tempted to rip out the blob hash-check entirely by
> default!  Anybody who really cares about checking the bits can do so
> with read_object_file(). That's what fsck does, and we could pretty
> easily convert "rev-list --verify-objects" to do so, too. So this is the
> less extreme version of the patch. ;)

A quick search shows many uses of parse_object() across the codebase.
It would certainly be nice if they all suddenly got faster by avoiding
this hashing, but I also suppose that most of the calls are using
parse_object() only because they are unsure if they are parsing a
commit or a tag and would never parse a large blob.

I think this approach of making parse_object_with_flags() is the best
way to incrementally approach things here. If we decide that we need
the _with_flags() version specifically to avoid this hash check, then
we could probably take the second approach: remove the hash check from
parse_object() and swap the places that care to use read_object_file()
instead. My guess is that in the long term there will be fewer swaps
to read_object_file() than to parse_object_with_flags().

However, this is a good first step to make progress without doing the
time-consuming audit of every caller to parse_object().

Thanks,
-Stolee

  reply	other threads:[~2022-09-07 14:15 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-11  8:09 Partial-clone cause big performance impact on server 程洋
2022-08-11 17:22 ` Jonathan Tan
2022-08-13  7:55   ` 回复: [External Mail]Re: " 程洋
2022-08-13 11:41     ` 程洋
2022-08-15  5:16     ` ZheNing Hu
2022-08-15 13:15       ` 程洋
2022-08-12 12:21 ` Derrick Stolee
2022-08-14  6:48 ` Jeff King
2022-08-15 13:18   ` Derrick Stolee
2022-08-15 14:50     ` [External Mail]Re: " 程洋
2022-08-17 10:22     ` 程洋
2022-08-17 13:41       ` Derrick Stolee
2022-08-18  5:49         ` Jeff King
2022-09-01  6:53   ` 程洋
2022-09-01 16:19     ` Jeff King
2022-09-05 11:17       ` 程洋
2022-09-06 18:38         ` Jeff King
2022-09-06 22:58           ` [PATCH 0/3] speeding up on-demand fetch for blobs in partial clone Jeff King
2022-09-06 23:01             ` [PATCH 1/3] parse_object(): allow skipping hash check Jeff King
2022-09-07 14:15               ` Derrick Stolee [this message]
2022-09-07 20:44                 ` Jeff King
2022-09-06 23:05             ` [PATCH 2/3] upload-pack: skip parse-object re-hashing of "want" objects Jeff King
2022-09-07 14:36               ` Derrick Stolee
2022-09-07 14:45                 ` Derrick Stolee
2022-09-07 20:50                   ` Jeff King
2022-09-07 19:26               ` Junio C Hamano
2022-09-07 20:36                 ` Jeff King
2022-09-07 20:48                   ` [BUG] t1800: Fails for error text comparison rsbecker
2022-09-07 21:55                     ` Junio C Hamano
2022-09-07 22:23                       ` rsbecker
2022-09-07 21:02                   ` [PATCH 2/3] upload-pack: skip parse-object re-hashing of "want" objects Jeff King
2022-09-07 22:07                     ` Junio C Hamano
2022-09-08  5:04                       ` Jeff King
2022-09-08 16:41                         ` Junio C Hamano
2022-09-06 23:06             ` [PATCH 3/3] parse_object(): check commit-graph when skip_hash set Jeff King
2022-09-07 14:46               ` Derrick Stolee
2022-09-07 19:31               ` Junio C Hamano
2022-09-08 10:39                 ` [External Mail]Re: " 程洋
2022-09-08 18:42                   ` Jeff King
2022-09-07 14:48             ` [PATCH 0/3] speeding up on-demand fetch for blobs in partial clone Derrick Stolee

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f79b0ccd-3e36-f447-0dbb-6e40ad547c8d@github.com \
    --to=derrickstolee@github.com \
    --cc=chengyang@xiaomi.com \
    --cc=fanjunhui@xiaomi.com \
    --cc=git@vger.kernel.org \
    --cc=hehao@xiaomi.com \
    --cc=maxin7@xiaomi.com \
    --cc=peff@peff.net \
    --cc=shifengbing@xiaomi.com \
    --cc=wanghanji@xiaomi.com \
    /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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.