[PATCH 0/2] fixing parse_object() check for type mismatch

From: Jeff King <peff@peff.net>
Date: Nov 17, 2022, 22:37 UTC
Message-ID: <Y3a3qcqNG8W3ueeb@coredump.intra.peff.net>
In-Reply-To: <20221116211419.439356-1-jonathantanmy@google.com>

On Wed, Nov 16, 2022 at 01:14:18PM -0800, Jonathan Tan wrote:

Show 12 quoted lines

> > But there may be some subtlety I'm missing. I'm cc-ing Jonathan Tan, who
> > added has_object(), and who added the top call to repo_has_object_file()
> > via df11e19648 (rev-list: support termination at promisor objects,
> > 2017-12-08).
> 
> Thanks for CC-ing me on this. Looking at that commit and the code at that time,
> I'm not sure why I added that call either. My best guess is that I was worried
> that the streaming interface wouldn't support missing objects, but both then
> and now, a call to istream_source() is made before any streaming occurs (which
> does perform the lazy fetch).
> 
> So yes, I also think that you can remove these calls.

Thanks. After staring at this a bit, I noticed there is an even more subtle issue with the case you touched back then, which is that it fails to notice when a think we expect to be a blob isn't one. Your old patch didn't make anything worse there, but it also wasn't sufficient to catch the problem. See patch 2 for details.

I'm adding Taylor to the cc as the author of t6102, when we were tracking down all of these "oops, it's not really a blob" cases. This fixes one of the lingering cases from that test script.

  [1/2]: parse_object(): drop extra "has" check before checking object type
  [2/2]: parse_object(): check on-disk type of suspected blob

 object.c                               | 5 ++---
 t/t6102-rev-list-unexpected-objects.sh | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

-Peff