Re: [PATCH 2/3] index-pack: no blobs during outgoing link check

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Patrick Steinhardt <ps@xxxxxx> writes:
> > This situation has not been observed yet - we have only noticed missing
> > commits, not missing trees or blobs. (In fact, if it were believed that
> > only missing commits are problematic, one could argue that we should
> > also exclude trees during the outgoing link check; but it is safer to
> > include them.)
> > 
> > Due to the rarity of the situation (it has not been observed to happen
> > in real life), and because the "penalty" in such a situation is merely
> > to refetch the missing blob when it's needed, the tradeoff seems
> > worth it.
> 
> So is this a one-off event that may happen once per blob, or would we
> eventually evict the refetched blob and run into the same situation
> repeatedly?

One-off, since when refetched, the blob is in a promisor pack (and
thus won't be GC-ed). I've added this to the code comment that I added
following your suggestion below.

> > diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> > index 8e7d14c17e..58d24540dc 100644
> > --- a/builtin/index-pack.c
> > +++ b/builtin/index-pack.c
> > @@ -830,8 +830,10 @@ static void do_record_outgoing_links(struct object *obj)
> >  			 * verified, so do not print any here.
> >  			 */
> >  			return;
> > -		while (tree_entry_gently(&desc, &entry))
> > -			record_outgoing_link(&entry.oid);
> > +		while (tree_entry_gently(&desc, &entry)) {
> > +			if (S_ISDIR(entry.mode))
> > +				record_outgoing_link(&entry.oid);
> > +		}
> 
> Without the context of the commit message this code snippet likely would
> not make any sense to a reader. The "correct" logic would be to record
> all objects, regardless of whether they are an object ID or not. But we
> explicitly choose not to as a tradeoff between performance and
> correctness.
> 
> All to say that we should have a comment here that explains what is
> going on.
> 
> Patrick

Makes sense. I had to move almost the entirety of the commit message
into a code comment - I don't think putting merely a part here would be
enough context.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux