diff mbox series

pack-redundant: gauge the usage before proposing its removal

Message ID xmqq1rjuz6n3.fsf_-_@gitster.c.googlers.com (mailing list archive)
State New, archived
Headers show
Series pack-redundant: gauge the usage before proposing its removal | expand

Commit Message

Junio C Hamano Aug. 25, 2020, 10:45 p.m. UTC 
The subcommand is unusably slow and the reason why nobody reports it
as a performance bug is suspected to be the absense of users.  Let's
show a big message that asks the user to tell us that they still
care about the command when an attempt is made to run the command,
with an escape hatch to override it with a command line option.

In a few releases, we may turn it into an error and keep it for a
few more releases before finally removing it (during the whole time,
the plan to remove it would be interrupted by end user raising hand).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

    Jeff King <peff@peff.net> writes:

    > A more gentle transition would I guess be:
    >
    >   1. Mention deprecation in release notes (hah, as if anybody reads
    >      them).
    >
    >   2. Issue a warning but continue to behave as normal. That might break
    >      scripts that care a lot about stderr, but otherwise is harmless. No
    >      clue if anybody would actually see the message or not.

    OK, so here is an update for the above.

 builtin/pack-redundant.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Taylor Blau Aug. 25, 2020, 11:09 p.m. UTC | #1 
Hi Junio,

On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:
> The subcommand is unusably slow and the reason why nobody reports it
> as a performance bug is suspected to be the absense of users.  Let's
> show a big message that asks the user to tell us that they still
> care about the command when an attempt is made to run the command,
> with an escape hatch to override it with a command line option.
>
> In a few releases, we may turn it into an error and keep it for a
> few more releases before finally removing it (during the whole time,
> the plan to remove it would be interrupted by end user raising hand).
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

Thanks. Peff's plan seems reasonable to me (and I'd like to add that I
am a frequent reader of the release notes ;-)), as does this patch.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor
Junio C Hamano Aug. 25, 2020, 11:22 p.m. UTC | #2 
Taylor Blau <me@ttaylorr.com> writes:

> On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:
>> The subcommand is unusably slow and the reason why nobody reports it
>> as a performance bug is suspected to be the absense of users.  Let's
>> show a big message that asks the user to tell us that they still
>> care about the command when an attempt is made to run the command,
>> with an escape hatch to override it with a command line option.
>>
>> In a few releases, we may turn it into an error and keep it for a
>> few more releases before finally removing it (during the whole time,
>> the plan to remove it would be interrupted by end user raising hand).
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>
> Thanks. Peff's plan seems reasonable to me (and I'd like to add that I
> am a frequent reader of the release notes ;-)), as does this patch.
>
>   Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks.  It needs updates to a test script, though.

diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh
index 6b4d1ca353..2dd2d67b9e 100755
--- a/t/t5323-pack-redundant.sh
+++ b/t/t5323-pack-redundant.sh
@@ -39,6 +39,8 @@ relationship between packs and objects is as follows:
 master_repo=master.git
 shared_repo=shared.git
 
+git_pack_redundant='git pack-redundant --i-still-use-this'
+
 # Create commits in <repo> and assign each commit's oid to shell variables
 # given in the arguments (A, B, and C). E.g.:
 #
@@ -154,7 +156,7 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '
 		EOF
 	(
 		cd "$master_repo" &&
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		test_must_be_empty out
 	)
 '
@@ -192,7 +194,7 @@ test_expect_success 'master: one of pack-2/pack-3 is redundant' '
 		cat >expect <<-EOF &&
 			P3:$P3
 			EOF
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		format_packfiles <out >actual &&
 		test_cmp expect actual
 	)
@@ -231,7 +233,7 @@ test_expect_success 'master: pack 2, 4, and 6 are redundant' '
 			P4:$P4
 			P6:$P6
 			EOF
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		format_packfiles <out >actual &&
 		test_cmp expect actual
 	)
@@ -266,7 +268,7 @@ test_expect_success 'master: pack-8 (subset of pack-1) is also redundant' '
 			P6:$P6
 			P8:$P8
 			EOF
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		format_packfiles <out >actual &&
 		test_cmp expect actual
 	)
@@ -284,9 +286,9 @@ test_expect_success 'master: clean loose objects' '
 test_expect_success 'master: remove redundant packs and pass fsck' '
 	(
 		cd "$master_repo" &&
-		git pack-redundant --all | xargs rm &&
+		$git_pack_redundant --all | xargs rm &&
 		git fsck &&
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		test_must_be_empty out
 	)
 '
@@ -304,7 +306,7 @@ test_expect_success 'setup shared.git' '
 test_expect_success 'shared: all packs are redundant, but no output without --alt-odb' '
 	(
 		cd "$shared_repo" &&
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		test_must_be_empty out
 	)
 '
@@ -343,7 +345,7 @@ test_expect_success 'shared: show redundant packs in stderr for verbose mode' '
 			P5:$P5
 			P7:$P7
 			EOF
-		git pack-redundant --all --verbose >out 2>out.err &&
+		$git_pack_redundant --all --verbose >out 2>out.err &&
 		test_must_be_empty out &&
 		grep "pack$" out.err | format_packfiles >actual &&
 		test_cmp expect actual
@@ -356,9 +358,9 @@ test_expect_success 'shared: remove redundant packs, no packs left' '
 		cat >expect <<-EOF &&
 			fatal: Zero packs found!
 			EOF
-		git pack-redundant --all --alt-odb | xargs rm &&
+		$git_pack_redundant --all --alt-odb | xargs rm &&
 		git fsck &&
-		test_must_fail git pack-redundant --all --alt-odb >actual 2>&1 &&
+		test_must_fail $git_pack_redundant --all --alt-odb >actual 2>&1 &&
 		test_cmp expect actual
 	)
 '
@@ -386,7 +388,7 @@ test_expect_success 'shared: create new objects and packs' '
 test_expect_success 'shared: no redundant without --alt-odb' '
 	(
 		cd "$shared_repo" &&
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		test_must_be_empty out
 	)
 '
@@ -417,7 +419,7 @@ test_expect_success 'shared: no redundant without --alt-odb' '
 test_expect_success 'shared: one pack is redundant with --alt-odb' '
 	(
 		cd "$shared_repo" &&
-		git pack-redundant --all --alt-odb >out &&
+		$git_pack_redundant --all --alt-odb >out &&
 		format_packfiles <out >actual &&
 		test_line_count = 1 actual
 	)
@@ -454,7 +456,7 @@ test_expect_success 'shared: ignore unique objects and all two packs are redunda
 			Px1:$Px1
 			Px2:$Px2
 			EOF
-		git pack-redundant --all --alt-odb >out <<-EOF &&
+		$git_pack_redundant --all --alt-odb >out <<-EOF &&
 			$X
 			$Y
 			$Z
Jeff King Aug. 28, 2020, 9:14 a.m. UTC | #3 
On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:

> The subcommand is unusably slow and the reason why nobody reports it
> as a performance bug is suspected to be the absense of users.  Let's
> show a big message that asks the user to tell us that they still
> care about the command when an attempt is made to run the command,
> with an escape hatch to override it with a command line option.

I was looking at the history here and noticed this topic, which I
somehow missed when it happened:

  $ git show -s cf0879f7e98d2213503622f780d2ab0dd3f93477
  commit cf0879f7e98d2213503622f780d2ab0dd3f93477
  Merge: 3710f60a80 0e37abd2e8
  Author: Junio C Hamano <gitster@pobox.com>
  Date:   Thu Mar 7 09:59:54 2019 +0900
  
      Merge branch 'sc/pack-redundant'
      
      Update the implementation of pack-redundant for performance in a
      repository with many packfiles.
      
      * sc/pack-redundant:
        pack-redundant: consistent sort method
        pack-redundant: rename pack_list.all_objects
        pack-redundant: new algorithm to find min packs
        pack-redundant: delete redundant code
        pack-redundant: delay creation of unique_objects
        t5323: test cases for git-pack-redundant

So it sounds like:

  - somebody does care enough to use it

  - it may not be horrifically slow anymore

So it may not be worth trying to follow through on the deprecation
(though the fact that neither of us realized this makes me worried for
the general state of maintenance of this code).

-Peff
Junio C Hamano Aug. 28, 2020, 10:45 p.m. UTC | #4 
Jeff King <peff@peff.net> writes:

> On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:
>
>> The subcommand is unusably slow and the reason why nobody reports it
>> as a performance bug is suspected to be the absense of users.  Let's
>> show a big message that asks the user to tell us that they still
>> care about the command when an attempt is made to run the command,
>> with an escape hatch to override it with a command line option.
>
> I was looking at the history here and noticed this topic, which I
> somehow missed when it happened:
>
>   $ git show -s cf0879f7e98d2213503622f780d2ab0dd3f93477
>   commit cf0879f7e98d2213503622f780d2ab0dd3f93477
>   Merge: 3710f60a80 0e37abd2e8
>   Author: Junio C Hamano <gitster@pobox.com>
>   Date:   Thu Mar 7 09:59:54 2019 +0900
>   
>       Merge branch 'sc/pack-redundant'
>       
>       Update the implementation of pack-redundant for performance in a
>       repository with many packfiles.
>       
>       * sc/pack-redundant:
>         pack-redundant: consistent sort method
>         pack-redundant: rename pack_list.all_objects
>         pack-redundant: new algorithm to find min packs
>         pack-redundant: delete redundant code
>         pack-redundant: delay creation of unique_objects
>         t5323: test cases for git-pack-redundant
>
> So it sounds like:
>
>   - somebody does care enough to use it
>
>   - it may not be horrifically slow anymore
>
> So it may not be worth trying to follow through on the deprecation
> (though the fact that neither of us realized this makes me worried for
> the general state of maintenance of this code).

OK.  Just dropping the topic is the easiest ;-)  Thanks.
diff mbox series

Patch

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 178e3409b7..b94c2f2423 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -554,6 +554,7 @@  static void load_all(void)
 int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
 {
 	int i;
+	int i_still_use_this = 0;
 	struct pack_list *min = NULL, *red, *pl;
 	struct llist *ignore;
 	struct object_id *oid;
@@ -580,12 +581,24 @@  int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
 			alt_odb = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--i-still-use-this")) {
+			i_still_use_this = 1;
+			continue;
+		}
 		if (*arg == '-')
 			usage(pack_redundant_usage);
 		else
 			break;
 	}
 
+	if (!i_still_use_this) {
+		fputs(_("'git pack-redundant' is nominated for removal.\n"
+			"If you still use this command, please add an extra\n"
+			"option, '--i-still-use-this', on the command line\n"
+			"and let us know you still use it by sending an e-mail\n"
+			"to <git@vger.kernel.org>.  Thanks.\n"), stderr);
+	}
+
 	if (load_all_packs)
 		load_all();
 	else