[PATCH 0/2] Add OBJECT_INFO_NO_FETCH_IF_MISSING flag

[PATCH 0/2] Add OBJECT_INFO_NO_FETCH_IF_MISSING flag

[Christian Couder]

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

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

[Christian Couder]

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(-)

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

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

[Jeff King]

On Thu, Jun 20, 2019 at 10:30:25AM +0200, Christian Couder wrote:

> 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

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

[Christian Couder]

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(-)

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

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

[Jeff King]

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.

-Peff

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

[Derrick Stolee]

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.

Thanks,
-Stolee

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

[Christian Couder]

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.

Christian.

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

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> 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.

Re: [PATCH 0/2] Add OBJECT_INFO_NO_FETCH_IF_MISSING flag

[Junio C Hamano]

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?

>
> 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(-)

Re: [PATCH 0/2] Add OBJECT_INFO_NO_FETCH_IF_MISSING flag

[Christian Couder]

On Thu, Jun 20, 2019 at 10:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.