[PATCH 0/2] 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 <redacted>

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.

Signed-off-by: Derrick Stolee <redacted>
---
 builtin/gc.c           |  6 ++++++
 t/t7900-maintenance.sh | 26 +++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index b315b2ad588..54bae7f0c4c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -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;

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 1074009cc05..f9031cbb44b 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh

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

From: Derrick Stolee <redacted>

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.

Signed-off-by: Derrick Stolee <redacted>
---
 t/t7900-maintenance.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index f9031cbb44b..6be9d42767a 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh

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

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

On Mon, Jan 18, 2021 at 03:23:35AM +0000, Derrick Stolee via GitGitGadget wrote:

OK; this is a good way to ensure that you're not constantly appending
'refs/prefetch' into the config.

I did notice that we have a 'remotes' string list just above, so I
suppose we could only ignore 'refs/prefetch/<remote>' for just the
remotes that we know about, but I doubt that this would be all that
useful. (I.e., are there really users that are using refs/prefetch
already and don't want to hide the parts of it that aren't managed by
maintenance? Doubtful.)

+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

Great, this test matches what I would expect. Thank you!

Thanks,
Taylor

On Mon, Jan 18, 2021 at 03:23:36AM +0000, Derrick Stolee via GitGitGadget wrote:

From: Derrick Stolee <redacted>

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.

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

Hmm. Makes sense, but this will certainly need to be updated to work
with reftables, and it would break if you ran 'git pack-refs'.

Perhaps instead:

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

?

Thanks,
Taylor

On 1/18/2021 11:04 AM, Taylor Blau wrote:

On Mon, Jan 18, 2021 at 03:23:36AM +0000, Derrick Stolee via GitGitGadget wrote:

quoted

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.

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

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

quoted

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.

Signed-off-by: Derrick Stolee <redacted>
---
 t/t7900-maintenance.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index f9031cbb44b..6be9d42767a 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh

@@ -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 &&

Hmm. Makes sense, but this will certainly need to be updated to work
with reftables, and it would break if you ran 'git pack-refs'.

Perhaps instead:

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

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

From: Derrick Stolee <redacted>

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.

Signed-off-by: Derrick Stolee <redacted>
---
 builtin/gc.c           |  6 ++++++
 t/t7900-maintenance.sh | 26 +++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index b315b2ad588..54bae7f0c4c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -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;

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 1074009cc05..f9031cbb44b 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh

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

From: Derrick Stolee <redacted>

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.

Helped-by: Taylor Blau [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 t/t7900-maintenance.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index f9031cbb44b..78ccf4b33f8 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh

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

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 [off-list ref]
          Signed-off-by: Derrick Stolee [off-list ref]
      
       ## 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

On Tue, Jan 19, 2021 at 12:52:02PM +0000, Derrick Stolee via GitGitGadget wrote:

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

The range-diff looks good to me, thanks!

  Reviewed-by: Taylor Blau [off-list ref]

Thanks,
Taylor