threads / patch / 51348

patch, 2 partsAdd OBJECT_INFO_NO_FETCH_IF_MISSING flag

Subject: [PATCH 0/2] Add OBJECT_INFO_NO_FETCH_IF_MISSING flag

## tl;dr

10 messages between Jun 20, 2019 and Jun 21, 2019. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Christian Couder· Jun 20, 2019, 08:30 UTC · lore

In a review[1] of my "many promisor remotes" patch series[2] and in the following thread, it was suggested that a flag should be passed to tell oid_object_info_extended() that it should not fetch objects from promisor remotes if they are missing, instead of using the ugly fetch_if_missing global.

It looks like the OBJECT_INFO_FOR_PREFETCH flag already exists but unfortunately conflates 2 separate things.

This patch series introduces OBJECT_INFO_NO_FETCH_IF_MISSING to disambiguate the different meanings and then uses it instead of OBJECT_INFO_FOR_PREFETCH where it makes sense.

1: https://public-inbox.org/git/b4d69d2b-dc0d-fffb-2909-c54060fe9cd1@gmail.com/ 2: https://public-inbox.org/git/20190409161116.30256-1-chriscool@tuxfamily.org/

Christian Couder (2):
  object-store: introduce OBJECT_INFO_NO_FETCH_IF_MISSING
  sha1-file: use OBJECT_INFO_NO_FETCH_IF_MISSING
 object-store.h | 9 +++++++--
 sha1-file.c    | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)
-- 
2.22.0
Christian Couder· Jun 20, 2019, 08:30 UTC · re: Christian Couder · lore

[PATCH 1/2] object-store: introduce OBJECT_INFO_NO_FETCH_IF_MISSING

The fetch_if_missing global variable should be replaced as much as possible by passing a flag to oid_object_info_extended().

The existing OBJECT_INFO_FOR_PREFETCH unfortunately conflates both a "no fetch if missing" meaning with OBJECT_INFO_QUICK which is about retrying packed storage.

Let's disambiguate that by adding a new explicit OBJECT_INFO_NO_FETCH_IF_MISSING flag.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 object-store.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
Show changes to object-store.h +7 −2
diff --git a/object-store.h b/object-store.h
index 272e01e452..02c1795d50 100644
--- a/object-store.h
+++ b/object-store.h
@@ -277,10 +277,15 @@ struct object_info {
 #define OBJECT_INFO_IGNORE_LOOSE 16
 /*
  * Do not attempt to fetch the object if missing (even if fetch_is_missing is
- * nonzero). This is meant for bulk prefetching of missing blobs in a partial
+ * nonzero).
+ */
+#define OBJECT_INFO_NO_FETCH_IF_MISSING 32
+/*
+ * This is meant for bulk prefetching of missing blobs in a partial
  * clone. Implies OBJECT_INFO_QUICK.
  */
-#define OBJECT_INFO_FOR_PREFETCH (32 + OBJECT_INFO_QUICK)
+#define OBJECT_INFO_FOR_PREFETCH \
+  (OBJECT_INFO_NO_FETCH_IF_MISSING + OBJECT_INFO_QUICK)
 
 int oid_object_info_extended(struct repository *r,
 			     const struct object_id *,

-- 
2.22.0
Jeff King· Jun 20, 2019, 08:48 UTC · re: Christian Couder · lore

Re: [PATCH 1/2] object-store: introduce OBJECT_INFO_NO_FETCH_IF_MISSING

On Thu, Jun 20, 2019 at 10:30:25AM +0200, Christian Couder wrote:
Show 9 quoted lines
> The fetch_if_missing global variable should be replaced as much
> as possible by passing a flag to oid_object_info_extended().
> 
> The existing OBJECT_INFO_FOR_PREFETCH unfortunately conflates
> both a "no fetch if missing" meaning with OBJECT_INFO_QUICK
> which is about retrying packed storage.
> 
> Let's disambiguate that by adding a new explicit
> OBJECT_INFO_NO_FETCH_IF_MISSING flag.

I think this patch is obsoleted by Stolee's 31f5256c82 (sha1-file: split OBJECT_INFO_FOR_PREFETCH, 2019-05-28), which I think just made it to master in the last day or so.

There it's called OBJECT_INFO_SKIP_FETCH_OBJECT, so your 2/2 will need to be tweaked.

-Peff
Christian Couder· Jun 20, 2019, 08:30 UTC · re: Christian Couder · lore

[PATCH 2/2] sha1-file: use OBJECT_INFO_NO_FETCH_IF_MISSING

Currently the OBJECT_INFO_FOR_PREFETCH flag is used to check if we should fetch objects from promisor remotes when we haven't found them elsewhere.

Now that OBJECT_INFO_NO_FETCH_IF_MISSING exists, let's use it instead to be more correct in case this new flag is ever used without OBJECT_INFO_QUICK.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 sha1-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to sha1-file.c +1 −1
diff --git a/sha1-file.c b/sha1-file.c
index ed5c50dac4..2116ff1e70 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1379,7 +1379,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,
 		/* Check if it is a missing object */
 		if (fetch_if_missing && repository_format_partial_clone &&
 		    !already_retried && r == the_repository &&
-		    !(flags & OBJECT_INFO_FOR_PREFETCH)) {
+		    !(flags & OBJECT_INFO_NO_FETCH_IF_MISSING)) {
 			/*
 			 * TODO Investigate having fetch_object() return
 			 * TODO error/success and stopping the music here.

-- 
2.22.0
Jeff King· Jun 20, 2019, 08:50 UTC · re: Christian Couder · lore

Re: [PATCH 2/2] sha1-file: use OBJECT_INFO_NO_FETCH_IF_MISSING

On Thu, Jun 20, 2019 at 10:30:26AM +0200, Christian Couder wrote:
Show 7 quoted lines
> Currently the OBJECT_INFO_FOR_PREFETCH flag is used to check
> if we should fetch objects from promisor remotes when we
> haven't found them elsewhere.
> 
> Now that OBJECT_INFO_NO_FETCH_IF_MISSING exists, let's use
> it instead to be more correct in case this new flag is ever
> used without OBJECT_INFO_QUICK.

I said earlier that this one would need to be tweaked for the new upstream name. But actually, I think it is not necessary after Stolee's patch.

-Peff
Derrick Stolee· Jun 20, 2019, 12:39 UTC · re: Jeff King · lore

Re: [PATCH 2/2] sha1-file: use OBJECT_INFO_NO_FETCH_IF_MISSING

On 6/20/2019 4:50 AM, Jeff King wrote:
Show 13 quoted lines
> On Thu, Jun 20, 2019 at 10:30:26AM +0200, Christian Couder wrote:
> 
>> Currently the OBJECT_INFO_FOR_PREFETCH flag is used to check
>> if we should fetch objects from promisor remotes when we
>> haven't found them elsewhere.
>>
>> Now that OBJECT_INFO_NO_FETCH_IF_MISSING exists, let's use
>> it instead to be more correct in case this new flag is ever
>> used without OBJECT_INFO_QUICK.
> 
> I said earlier that this one would need to be tweaked for the new
> upstream name. But actually, I think it is not necessary after Stolee's
> patch.

Yes, I believe that 31f5256c82 does an equivalent thing to the combination of these patches.

Thanks, -Stolee

Christian Couder· Jun 20, 2019, 14:08 UTC · re: Derrick Stolee · lore

Re: [PATCH 2/2] sha1-file: use OBJECT_INFO_NO_FETCH_IF_MISSING

On Thu, Jun 20, 2019 at 2:39 PM Derrick Stolee <stolee@gmail.com> wrote:
Show 18 quoted lines
>
> On 6/20/2019 4:50 AM, Jeff King wrote:
> > On Thu, Jun 20, 2019 at 10:30:26AM +0200, Christian Couder wrote:
> >
> >> Currently the OBJECT_INFO_FOR_PREFETCH flag is used to check
> >> if we should fetch objects from promisor remotes when we
> >> haven't found them elsewhere.
> >>
> >> Now that OBJECT_INFO_NO_FETCH_IF_MISSING exists, let's use
> >> it instead to be more correct in case this new flag is ever
> >> used without OBJECT_INFO_QUICK.
> >
> > I said earlier that this one would need to be tweaked for the new
> > upstream name. But actually, I think it is not necessary after Stolee's
> > patch.
>
> Yes, I believe that 31f5256c82  does an equivalent thing to the
> combination of these patches.

Yeah, I agree. Thanks Stolee for having already fixed that, and sorry for bothering everyone with this.

Christian.
Junio C Hamano· Jun 20, 2019, 20:57 UTC · re: Christian Couder · lore

Re: [PATCH 2/2] sha1-file: use OBJECT_INFO_NO_FETCH_IF_MISSING

Christian Couder <christian.couder@gmail.com> writes:
Show 22 quoted lines
> On Thu, Jun 20, 2019 at 2:39 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 6/20/2019 4:50 AM, Jeff King wrote:
>> > On Thu, Jun 20, 2019 at 10:30:26AM +0200, Christian Couder wrote:
>> >
>> >> Currently the OBJECT_INFO_FOR_PREFETCH flag is used to check
>> >> if we should fetch objects from promisor remotes when we
>> >> haven't found them elsewhere.
>> >>
>> >> Now that OBJECT_INFO_NO_FETCH_IF_MISSING exists, let's use
>> >> it instead to be more correct in case this new flag is ever
>> >> used without OBJECT_INFO_QUICK.
>> >
>> > I said earlier that this one would need to be tweaked for the new
>> > upstream name. But actually, I think it is not necessary after Stolee's
>> > patch.
>>
>> Yes, I believe that 31f5256c82  does an equivalent thing to the
>> combination of these patches.
>
> Yeah, I agree. Thanks Stolee for having already fixed that, and sorry
> for bothering everyone with this.
OK, I'll stop looking at this patch.
Junio C Hamano· Jun 20, 2019, 20:52 UTC · re: Christian Couder · lore

Re: [PATCH 0/2] Add OBJECT_INFO_NO_FETCH_IF_MISSING flag

Christian Couder <christian.couder@gmail.com> writes:
Show 15 quoted lines
> In a review[1] of my "many promisor remotes" patch series[2] and in
> the following thread, it was suggested that a flag should be passed to
> tell oid_object_info_extended() that it should not fetch objects from
> promisor remotes if they are missing, instead of using the ugly
> fetch_if_missing global.
>
> It looks like the OBJECT_INFO_FOR_PREFETCH flag already exists but
> unfortunately conflates 2 separate things.
>
> This patch series introduces OBJECT_INFO_NO_FETCH_IF_MISSING to
> disambiguate the different meanings and then uses it instead of
> OBJECT_INFO_FOR_PREFETCH where it makes sense.
>
> 1: https://public-inbox.org/git/b4d69d2b-dc0d-fffb-2909-c54060fe9cd1@gmail.com/
> 2: https://public-inbox.org/git/20190409161116.30256-1-chriscool@tuxfamily.org/
What commit did you base these two patches on?
Show 8 quoted lines
>
> Christian Couder (2):
>   object-store: introduce OBJECT_INFO_NO_FETCH_IF_MISSING
>   sha1-file: use OBJECT_INFO_NO_FETCH_IF_MISSING
>
>  object-store.h | 9 +++++++--
>  sha1-file.c    | 2 +-
>  2 files changed, 8 insertions(+), 3 deletions(-)
Christian Couder· Jun 21, 2019, 10:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Add OBJECT_INFO_NO_FETCH_IF_MISSING flag

On Thu, Jun 20, 2019 at 10:52 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > In a review[1] of my "many promisor remotes" patch series[2] and in
> > the following thread, it was suggested that a flag should be passed to
> > tell oid_object_info_extended() that it should not fetch objects from
> > promisor remotes if they are missing, instead of using the ugly
> > fetch_if_missing global.
> >
> > It looks like the OBJECT_INFO_FOR_PREFETCH flag already exists but
> > unfortunately conflates 2 separate things.
> >
> > This patch series introduces OBJECT_INFO_NO_FETCH_IF_MISSING to
> > disambiguate the different meanings and then uses it instead of
> > OBJECT_INFO_FOR_PREFETCH where it makes sense.
> >
> > 1: https://public-inbox.org/git/b4d69d2b-dc0d-fffb-2909-c54060fe9cd1@gmail.com/
> > 2: https://public-inbox.org/git/20190409161116.30256-1-chriscool@tuxfamily.org/
>
> What commit did you base these two patches on?

They were based on master at v2.22.0 when I worked on them, but I didn't send them right away. And I didn't rebase them before I later sent them.

← back to recent threads