Two cleanups around 'prefetch' refs

Here are a couple things that caught my eye during a recent evaluation of the maintenance feature:

1. 'refs/prefetch/' refs show up in 'git log' decorations. Auto-hide these.
2. t7900-maintenance.sh had some scary warnings that end up being unimportant.

This is based on 'master' at 66e871b (The third batch, 2021-01-15).

Thanks, -Stolee

Derrick Stolee (2):
maintenance: set log.excludeDecoration durin prefetch
t7900: clean up some broken refs

 builtin/gc.c           |  6 ++++++
 t/t7900-maintenance.sh | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

base-commit: 66e871b6647ffea61a77a0f82c7ef3415f1ee79c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-838%2Fderrickstolee%2Fprefetch-refs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-838/derrickstolee/prefetch-refs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/838

gitgitgadget

From: Derrick Stolee <[email protected]>

The 'prefetch' task fetches refs from all remotes and places them in the refs/prefetch/<remote>/ refspace. As this task is intended to run in the background, this allows users to keep their local data very close to the remote servers' data while not updating the users' understanding of the remote refs in refs/remotes/<remote>/.

However, this can clutter 'git log' decorations with copies of the refs with the full name 'refs/prefetch/<remote>/<branch>'.

The log.excludeDecoration config option was added in a6be5e67 (log: add log.excludeDecoration config option, 2020-05-16) for exactly this purpose.

Ensure we set this only for users that would benefit from it by assigning it at the beginning of the prefetch task. Other alternatives would be during 'git maintenance register' or 'git maintenance start', but those might assign the config even when the prefetch task is disabled by existing config. Further, users could run 'git maintenance run --task=prefetch' using their own scripting or scheduling. This provides the best coverage to automatically update the config when valuable.

It is improbable, but possible, that users might want to run the prefetch task _and_ see these refs in their log decorations. This seems incredibly unlikely to me, but users can always opt-in on a command-by-command basis using --decorate-refs=refs/prefetch/.

Test that this works in a few cases. In particular, ensure that our assignment of log.excludeDecoration=refs/prefetch/ is additive to other existing exclusions. Further, ensure we do not add multiple copies in multiple runs.

---
 builtin/gc.c           |  6 ++++++
 t/t7900-maintenance.sh | 26 +++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

index b315b2ad588..54bae7f0c4c 100644
@@ -897,6 +897,12 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts)
 	struct string_list_item *item;
 	struct string_list remotes = STRING_LIST_INIT_DUP;
 
+	git_config_set_multivar_gently("log.excludedecoration",
+					"refs/prefetch/",
+					"refs/prefetch/",
+					CONFIG_FLAGS_FIXED_VALUE |
+					CONFIG_FLAGS_MULTI_REPLACE);
+
 	if (for_each_remote(append_remote, &remotes)) {
 		error(_("failed to fill remotes"));
 		result = 1;

index 1074009cc05..f9031cbb44b 100755
@@ -149,7 +149,31 @@ test_expect_success 'prefetch multiple remotes' '
 	git log prefetch/remote2/two &&
 	git fetch --all &&
 	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remote1/one &&
-	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remote2/two
+	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remote2/two &&
+
+	test_cmp_config refs/prefetch/ log.excludedecoration &&
+	git log --oneline --decorate --all >log &&
+	! grep "prefetch" log
+'
+
+test_expect_success 'prefetch and existing log.excludeDecoration values' '
+	git config --unset-all log.excludeDecoration &&
+	git config log.excludeDecoration refs/remotes/remote1/ &&
+	git maintenance run --task=prefetch &&
+
+	git config --get-all log.excludeDecoration >out &&
+	grep refs/remotes/remote1/ out &&
+	grep refs/prefetch/ out &&
+
+	git log --oneline --decorate --all >log &&
+	! grep "prefetch" log &&
+	! grep "remote1" log &&
+	grep "remote2" log &&
+
+	# a second run does not change the config
+	git maintenance run --task=prefetch &&
+	git log --oneline --decorate --all >log2 &&
+	test_cmp log log2
 '
 
 test_expect_success 'loose-objects task' '

gitgitgadget

Not related to the patch below, but in the subject line: s/durin/\0g/.

From: Derrick Stolee <[email protected]>

The tests for the 'prefetch' task create remotes and fetch refs into 'refs/prefetch/<remote>/' and tags into 'refs/tags/'. These tests use the remotes to create objects not intended to be seen by the "local" repository.

In that sense, the incrmental-repack tasks did not have these objects and refs in mind. That test replaces the object directory with a specific pack-file layout for testing the batch-size logic. However, this causes some operations to start showing warnings such as:

error: refs/prefetch/remote1/one does not point to a valid object!
error: refs/tags/one does not point to a valid object!

This only shows up if you run the tests verbosely and watch the output. It caught my eye and I _thought_ that there was a bug where 'git gc' or 'git repack' wouldn't check 'refs/prefetch/' before pruning objects. That is incorrect. Those commands do handle 'refs/prefetch/' correctly.

All that is left is to clean up the tests in t7900-maintenance.sh to remove these tags and refs that are not being repacked for the incremental-repack tests.

---
 t/t7900-maintenance.sh | 5 +++++
 1 file changed, 5 insertions(+)

index f9031cbb44b..6be9d42767a 100755
@@ -256,6 +256,11 @@ test_expect_success 'incremental-repack task' '
 	HEAD
 	^HEAD~1
 	EOF
+
+	# Replace the object directory with this pack layout.
+	# However, it does not include all objects from the remotes.
+	rm -rf .git/refs/prefetch &&
+	rm -rf .git/refs/tags &&
 	rm -f $packDir/pack-* &&
 	rm -f $packDir/loose-* &&
 	ls $packDir/*.pack >packs-before &&

gitgitgadget

Do you think it would be worth checking that 'does not point to a valid object' doesn't appear in the output?

  git for-each-ref --format='delete %(refname)' \
    refs/prefetch refs/tags >refs &&
  git update-ref --stdin <refs

Perhaps, but I think this isn't an actually interesting behavior to test. It's really the test that was broken, not Git itself.

Deleting refs in a clean way is probably good to do. I'm guessing you didn't use a pipe because it can be hard to diagnose a failure in the chain? That's probably reasonable.

Thanks,
-Stolee

Here are a couple things that caught my eye during a recent evaluation of the maintenance feature:

1. 'refs/prefetch/' refs show up in 'git log' decorations. Auto-hide these.
2. t7900-maintenance.sh had some scary warnings that end up being unimportant.

This is based on 'master' at 66e871b (The third batch, 2021-01-15).

Update in v2: deleting refs more safely for alternate ref backends. (Thanks, Taylor!)

Thanks, -Stolee

Derrick Stolee (2):
maintenance: set log.excludeDecoration durin prefetch
t7900: clean up some broken refs

 builtin/gc.c           |  6 ++++++
 t/t7900-maintenance.sh | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

base-commit: 66e871b6647ffea61a77a0f82c7ef3415f1ee79c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-838%2Fderrickstolee%2Fprefetch-refs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-838/derrickstolee/prefetch-refs-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/838

Range-diff vs v1:

 1:  5b2ce9049a6 = 1:  5b2ce9049a6 maintenance: set log.excludeDecoration durin prefetch
 2:  616b73a6556 ! 2:  35038dfd037 t7900: clean up some broken refs

@@ Commit message

All that is left is to clean up the tests in t7900-maintenance.sh to remove these tags and refs that are not being repacked for the
- incremental-repack tests.
+ incremental-repack tests. Use update-ref to ensure this works with all
+ ref backends.

+ Helped-by: Taylor Blau <[email protected]>

## t/t7900-maintenance.sh ##
@@ t/t7900-maintenance.sh: test_expect_success 'incremental-repack task' '
^HEAD~1
EOF
+
++ # Delete refs that have not been repacked in these packs.
++ git for-each-ref --format="delete %(refname)" \
++ refs/prefetch refs/tags >refs &&
++ git update-ref --stdin <refs &&
++
+ # Replace the object directory with this pack layout.
-+ # However, it does not include all objects from the remotes.
-+ rm -rf .git/refs/prefetch &&
-+ rm -rf .git/refs/tags &&
rm -f $packDir/pack-* &&
rm -f $packDir/loose-* &&
ls $packDir/*.pack >packs-before &&

gitgitgadget

From: Derrick Stolee <[email protected]>

The 'prefetch' task fetches refs from all remotes and places them in the refs/prefetch/<remote>/ refspace. As this task is intended to run in the background, this allows users to keep their local data very close to the remote servers' data while not updating the users' understanding of the remote refs in refs/remotes/<remote>/.

However, this can clutter 'git log' decorations with copies of the refs with the full name 'refs/prefetch/<remote>/<branch>'.

The log.excludeDecoration config option was added in a6be5e67 (log: add log.excludeDecoration config option, 2020-05-16) for exactly this purpose.

Ensure we set this only for users that would benefit from it by assigning it at the beginning of the prefetch task. Other alternatives would be during 'git maintenance register' or 'git maintenance start', but those might assign the config even when the prefetch task is disabled by existing config. Further, users could run 'git maintenance run --task=prefetch' using their own scripting or scheduling. This provides the best coverage to automatically update the config when valuable.

It is improbable, but possible, that users might want to run the prefetch task _and_ see these refs in their log decorations. This seems incredibly unlikely to me, but users can always opt-in on a command-by-command basis using --decorate-refs=refs/prefetch/.

Test that this works in a few cases. In particular, ensure that our assignment of log.excludeDecoration=refs/prefetch/ is additive to other existing exclusions. Further, ensure we do not add multiple copies in multiple runs.

---
 builtin/gc.c           |  6 ++++++
 t/t7900-maintenance.sh | 26 +++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

index b315b2ad588..54bae7f0c4c 100644
@@ -897,6 +897,12 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts)
 	struct string_list_item *item;
 	struct string_list remotes = STRING_LIST_INIT_DUP;
 
+	git_config_set_multivar_gently("log.excludedecoration",
+					"refs/prefetch/",
+					"refs/prefetch/",
+					CONFIG_FLAGS_FIXED_VALUE |
+					CONFIG_FLAGS_MULTI_REPLACE);
+
 	if (for_each_remote(append_remote, &remotes)) {
 		error(_("failed to fill remotes"));
 		result = 1;

index 1074009cc05..f9031cbb44b 100755
@@ -149,7 +149,31 @@ test_expect_success 'prefetch multiple remotes' '
 	git log prefetch/remote2/two &&
 	git fetch --all &&
 	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remote1/one &&
-	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remote2/two
+	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remote2/two &&
+
+	test_cmp_config refs/prefetch/ log.excludedecoration &&
+	git log --oneline --decorate --all >log &&
+	! grep "prefetch" log
+'
+
+test_expect_success 'prefetch and existing log.excludeDecoration values' '
+	git config --unset-all log.excludeDecoration &&
+	git config log.excludeDecoration refs/remotes/remote1/ &&
+	git maintenance run --task=prefetch &&
+
+	git config --get-all log.excludeDecoration >out &&
+	grep refs/remotes/remote1/ out &&
+	grep refs/prefetch/ out &&
+
+	git log --oneline --decorate --all >log &&
+	! grep "prefetch" log &&
+	! grep "remote1" log &&
+	grep "remote2" log &&
+
+	# a second run does not change the config
+	git maintenance run --task=prefetch &&
+	git log --oneline --decorate --all >log2 &&
+	test_cmp log log2
 '
 
 test_expect_success 'loose-objects task' '

gitgitgadget

OK. I think those users who keep distance from "git maintenance" are different story but all others cannot be using refs/prefetch/ hierarchy for purposes other than "git maintenance" dictates, so "git maintenance [register|start]", or even when the user first runs "git maintenance" for that matter, would be acceptable point to add the configuration, but at the beginning of a prefetch task, we know the hierarchy is being used for what "git maintenance" wants to do, so it is a good place to do so.

But playing devil's advocate, I do not think throwing refs/prefetch into a "hardcoded list of hierarchies that would never be used for decoration purposes" would upset any end-users in practice. The only reason why I do not make such a suggestion is it is more work (such a hardcoded reject list does not exist, if I recall correctly).

It is not a viable workaround to add "--decorate-refs=refs/prefetch" that is a mouthful; configuration options are to reduce such typing, not to force more of them. But because I agree with that "incredibly unlikely" assessment, I do not care.

Thanks.

From: Derrick Stolee <[email protected]>

The tests for the 'prefetch' task create remotes and fetch refs into 'refs/prefetch/<remote>/' and tags into 'refs/tags/'. These tests use the remotes to create objects not intended to be seen by the "local" repository.

In that sense, the incrmental-repack tasks did not have these objects and refs in mind. That test replaces the object directory with a specific pack-file layout for testing the batch-size logic. However, this causes some operations to start showing warnings such as:

error: refs/prefetch/remote1/one does not point to a valid object!
error: refs/tags/one does not point to a valid object!

This only shows up if you run the tests verbosely and watch the output. It caught my eye and I _thought_ that there was a bug where 'git gc' or 'git repack' wouldn't check 'refs/prefetch/' before pruning objects. That is incorrect. Those commands do handle 'refs/prefetch/' correctly.

All that is left is to clean up the tests in t7900-maintenance.sh to remove these tags and refs that are not being repacked for the incremental-repack tests. Use update-ref to ensure this works with all ref backends.

---
 t/t7900-maintenance.sh | 7 +++++++
 1 file changed, 7 insertions(+)

index f9031cbb44b..78ccf4b33f8 100755
@@ -256,6 +256,13 @@ test_expect_success 'incremental-repack task' '
 	HEAD
 	^HEAD~1
 	EOF
+
+	# Delete refs that have not been repacked in these packs.
+	git for-each-ref --format="delete %(refname)" \
+		refs/prefetch refs/tags >refs &&
+	git update-ref --stdin <refs &&
+
+	# Replace the object directory with this pack layout.
 	rm -f $packDir/pack-* &&
 	rm -f $packDir/loose-* &&
 	ls $packDir/*.pack >packs-before &&

gitgitgadget

So we use prefetch refs but craft a repacking strategy in such a way that these prefetch refs do not act as anchors, deliberately breaking the repository and invalidating the prefetch refs so to speak?

Wow, that sounds like an evil and brittle test set-up.

The range-diff looks good to me, thanks!

Thanks,
Taylor

Thanks, both.