* [PATCH] fetch-pack: in partial clone, pass --promisor
@ 2020-08-20 17:51 Jonathan Tan
2020-08-20 20:11 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Tan @ 2020-08-20 17:51 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
When fetching a pack from a promisor remote, the corresponding .promisor
file needs to be created. "fetch-pack" originally did this by passing
"--promisor" to "index-pack", but in 5374a290aa ("fetch-pack: write
fetched refs to .promisor", 2019-10-16), "fetch-pack" was taught to do
this itself instead, because it needed to store ref information in the
.promisor file.
This causes a problem with superprojects when transfer.fsckobjects is
set, because in the current implementation, it is "index-pack" that
calls fsck_finish() to check the objects; before 5374a290aa,
fsck_finish() would see that .gitmodules is a promisor object and
tolerate it being missing, but after, there is no .promisor file (at the
time of the invocation of fsck_finish() by "index-pack") to tell it that
.gitmodules is a promisor object, so it returns an error.
Therefore, teach "fetch-pack" to pass "--promisor" to index pack once
again. "fetch-pack" will subsequently overwrite this file with the ref
information.
An alternative is to instead move object checking to "fetch-pack", and
let "index-pack" only index the files. However, since "index-pack" has
to inflate objects in order to index them, it seems reasonable to also
let it check the objects (which also require inflated files).
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
fetch-pack.c | 17 ++++++++++-------
t/t5616-partial-clone.sh | 16 ++++++++++++++++
2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 7f20eca4f8..d467edc24e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -866,13 +866,16 @@ static int get_pack(struct fetch_pack_args *args,
* have this responsibility.
*/
args->check_self_contained_and_connected = 0;
- /*
- * If we're obtaining the filename of a lockfile, we'll use
- * that filename to write a .promisor file with more
- * information below. If not, we need index-pack to do it for
- * us.
- */
- if (!(do_keep && pack_lockfiles) && args->from_promisor)
+
+ if (args->from_promisor)
+ /*
+ * write_promisor_file() may be called afterwards but
+ * we still need index-pack to know that this is a
+ * promisor pack. For example, if transfer.fsckobjects
+ * is true, index-pack needs to know that .gitmodules
+ * is a promisor object (so that it won't complain if
+ * it is missing).
+ */
strvec_push(&cmd.args, "--promisor");
}
else {
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 8827c2ed18..5a01466db4 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -163,6 +163,22 @@ test_expect_success 'manual prefetch of missing objects' '
test_line_count = 0 observed.oids
'
+test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '
+ test_create_repo submodule &&
+ test_commit -C submodule mycommit &&
+
+ test_create_repo src_with_sub &&
+ test_config -C src_with_sub uploadpack.allowfilter 1 &&
+ test_config -C src_with_sub uploadpack.allowanysha1inwant 1 &&
+
+ git -C src_with_sub submodule add "file://$(pwd)/submodule" mysub &&
+ git -C src_with_sub commit -m "commit with submodule" &&
+
+ git -c transfer.fsckobjects=1 \
+ clone --filter="blob:none" "file://$(pwd)/src_with_sub" dst &&
+ test_when_finished rm -rf dst
+'
+
test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack --fsck-objects' '
git init src &&
test_commit -C src x &&
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fetch-pack: in partial clone, pass --promisor
2020-08-20 17:51 [PATCH] fetch-pack: in partial clone, pass --promisor Jonathan Tan
@ 2020-08-20 20:11 ` Junio C Hamano
2020-08-21 21:08 ` Jonathan Tan
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2020-08-20 20:11 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
> When fetching a pack from a promisor remote, the corresponding .promisor
> file needs to be created. "fetch-pack" originally did this by passing
> "--promisor" to "index-pack", but in 5374a290aa ("fetch-pack: write
> fetched refs to .promisor", 2019-10-16), "fetch-pack" was taught to do
> this itself instead, because it needed to store ref information in the
> .promisor file.
So is this patch a fix for a regression in v2.25?
> This causes a problem ...
> ...
> An alternative is to instead move object checking to "fetch-pack", and
> let "index-pack" only index the files. However, since "index-pack" has
> to inflate objects in order to index them, it seems reasonable to also
> let it check the objects (which also require inflated files).
I can see why it might feel attractive to draw the line to divide
the labor between the two that way, but I 100% agree with your
reasoning---index-pack needs to inspect the objects in order for it
to be able to index, and in order for us to be able to trust the
resulting index, it should be validating the objects it identified
while it was indexing the pack stream.
> +
> + if (args->from_promisor)
> + /*
> + * write_promisor_file() may be called afterwards but
> + * we still need index-pack to know that this is a
> + * promisor pack. For example, if transfer.fsckobjects
> + * is true, index-pack needs to know that .gitmodules
> + * is a promisor object (so that it won't complain if
> + * it is missing).
> + */
> strvec_push(&cmd.args, "--promisor");
> }
> else {
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 8827c2ed18..5a01466db4 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -163,6 +163,22 @@ test_expect_success 'manual prefetch of missing objects' '
> test_line_count = 0 observed.oids
> '
>
> +test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '
> + test_create_repo submodule &&
> + test_commit -C submodule mycommit &&
> +
> + test_create_repo src_with_sub &&
> + test_config -C src_with_sub uploadpack.allowfilter 1 &&
> + test_config -C src_with_sub uploadpack.allowanysha1inwant 1 &&
> +
> + git -C src_with_sub submodule add "file://$(pwd)/submodule" mysub &&
> + git -C src_with_sub commit -m "commit with submodule" &&
> +
> + git -c transfer.fsckobjects=1 \
> + clone --filter="blob:none" "file://$(pwd)/src_with_sub" dst &&
> + test_when_finished rm -rf dst
> +'
> +
> test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack --fsck-objects' '
> git init src &&
> test_commit -C src x &&
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fetch-pack: in partial clone, pass --promisor
2020-08-20 20:11 ` Junio C Hamano
@ 2020-08-21 21:08 ` Jonathan Tan
2020-08-21 21:42 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Tan @ 2020-08-21 21:08 UTC (permalink / raw)
To: gitster; +Cc: jonathantanmy, git
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> > When fetching a pack from a promisor remote, the corresponding .promisor
> > file needs to be created. "fetch-pack" originally did this by passing
> > "--promisor" to "index-pack", but in 5374a290aa ("fetch-pack: write
> > fetched refs to .promisor", 2019-10-16), "fetch-pack" was taught to do
> > this itself instead, because it needed to store ref information in the
> > .promisor file.
>
> So is this patch a fix for a regression in v2.25?
Yes. (Just checked with "git merge-base" that 5374a290aa is in v2.25.0
but not v2.24.0.)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fetch-pack: in partial clone, pass --promisor
2020-08-21 21:08 ` Jonathan Tan
@ 2020-08-21 21:42 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2020-08-21 21:42 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>> > When fetching a pack from a promisor remote, the corresponding .promisor
>> > file needs to be created. "fetch-pack" originally did this by passing
>> > "--promisor" to "index-pack", but in 5374a290aa ("fetch-pack: write
>> > fetched refs to .promisor", 2019-10-16), "fetch-pack" was taught to do
>> > this itself instead, because it needed to store ref information in the
>> > .promisor file.
>>
>> So is this patch a fix for a regression in v2.25?
>
> Yes. (Just checked with "git merge-base" that 5374a290aa is in v2.25.0
> but not v2.24.0.)
Thanks.
I wonder how we missed the breakage back then, but better late than
never ;-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] fetch-pack: in partial clone, pass --promisor
2020-08-24 19:16 [PATCH 0/7] Better threaded delta resolution in index-pack (another try) Jonathan Tan
@ 2020-08-24 19:16 ` Jonathan Tan
2020-08-24 19:36 ` Jonathan Tan
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Tan @ 2020-08-24 19:16 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
When fetching a pack from a promisor remote, the corresponding .promisor
file needs to be created. "fetch-pack" originally did this by passing
"--promisor" to "index-pack", but in 5374a290aa ("fetch-pack: write
fetched refs to .promisor", 2019-10-16), "fetch-pack" was taught to do
this itself instead, because it needed to store ref information in the
.promisor file.
This causes a problem with superprojects when transfer.fsckobjects is
set, because in the current implementation, it is "index-pack" that
calls fsck_finish() to check the objects; before 5374a290aa,
fsck_finish() would see that .gitmodules is a promisor object and
tolerate it being missing, but after, there is no .promisor file (at the
time of the invocation of fsck_finish() by "index-pack") to tell it that
.gitmodules is a promisor object, so it returns an error.
Therefore, teach "fetch-pack" to pass "--promisor" to index pack once
again. "fetch-pack" will subsequently overwrite this file with the ref
information.
An alternative is to instead move object checking to "fetch-pack", and
let "index-pack" only index the files. However, since "index-pack" has
to inflate objects in order to index them, it seems reasonable to also
let it check the objects (which also require inflated files).
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
fetch-pack.c | 17 ++++++++++-------
t/t5616-partial-clone.sh | 16 ++++++++++++++++
2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 7f20eca4f8..d467edc24e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -866,13 +866,16 @@ static int get_pack(struct fetch_pack_args *args,
* have this responsibility.
*/
args->check_self_contained_and_connected = 0;
- /*
- * If we're obtaining the filename of a lockfile, we'll use
- * that filename to write a .promisor file with more
- * information below. If not, we need index-pack to do it for
- * us.
- */
- if (!(do_keep && pack_lockfiles) && args->from_promisor)
+
+ if (args->from_promisor)
+ /*
+ * write_promisor_file() may be called afterwards but
+ * we still need index-pack to know that this is a
+ * promisor pack. For example, if transfer.fsckobjects
+ * is true, index-pack needs to know that .gitmodules
+ * is a promisor object (so that it won't complain if
+ * it is missing).
+ */
strvec_push(&cmd.args, "--promisor");
}
else {
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 8827c2ed18..5a01466db4 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -163,6 +163,22 @@ test_expect_success 'manual prefetch of missing objects' '
test_line_count = 0 observed.oids
'
+test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '
+ test_create_repo submodule &&
+ test_commit -C submodule mycommit &&
+
+ test_create_repo src_with_sub &&
+ test_config -C src_with_sub uploadpack.allowfilter 1 &&
+ test_config -C src_with_sub uploadpack.allowanysha1inwant 1 &&
+
+ git -C src_with_sub submodule add "file://$(pwd)/submodule" mysub &&
+ git -C src_with_sub commit -m "commit with submodule" &&
+
+ git -c transfer.fsckobjects=1 \
+ clone --filter="blob:none" "file://$(pwd)/src_with_sub" dst &&
+ test_when_finished rm -rf dst
+'
+
test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack --fsck-objects' '
git init src &&
test_commit -C src x &&
--
2.28.0.297.g1956fa8f8d-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fetch-pack: in partial clone, pass --promisor
2020-08-24 19:16 ` [PATCH] fetch-pack: in partial clone, pass --promisor Jonathan Tan
@ 2020-08-24 19:36 ` Jonathan Tan
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Tan @ 2020-08-24 19:36 UTC (permalink / raw)
To: jonathantanmy; +Cc: git
> When fetching a pack from a promisor remote, the corresponding .promisor
> file needs to be created. "fetch-pack" originally did this by passing
> "--promisor" to "index-pack", but in 5374a290aa ("fetch-pack: write
> fetched refs to .promisor", 2019-10-16), "fetch-pack" was taught to do
> this itself instead, because it needed to store ref information in the
> .promisor file.
Oops...please ignore this. This has already been sent out as
https://lore.kernel.org/git/20200820175116.3889786-1-jonathantanmy@google.com/
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-24 19:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-20 17:51 [PATCH] fetch-pack: in partial clone, pass --promisor Jonathan Tan
2020-08-20 20:11 ` Junio C Hamano
2020-08-21 21:08 ` Jonathan Tan
2020-08-21 21:42 ` Junio C Hamano
-- strict thread matches above, loose matches on Subject: below --
2020-08-24 19:16 [PATCH 0/7] Better threaded delta resolution in index-pack (another try) Jonathan Tan
2020-08-24 19:16 ` [PATCH] fetch-pack: in partial clone, pass --promisor Jonathan Tan
2020-08-24 19:36 ` Jonathan Tan
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).