git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries
@ 2025-03-07 11:17 Karthik Nayak
  2025-03-07 11:17 ` [PATCH 1/2] reflog: drop usage of global variables Karthik Nayak
                   ` (4 more replies)
  0 siblings, 5 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-07 11:17 UTC (permalink / raw)
  To: git; +Cc: Karthik Nayak

 Documentation/git-reflog.adoc |  6 ++++
 builtin/reflog.c              | 84 +++++++++++++++++++++++++++++++++++--------
 t/t1410-reflog.sh             | 55 ++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+), 15 deletions(-)

Karthik Nayak (2):
      reflog: drop usage of global variables
      reflog: implement subcommand to drop reflogs

This patch series adds a new 'drop' subcommand to git-reflog that allows
users to delete the reflog for a specified reference. Additionally, it
adds an '--all' flag to enable dropping all reflogs in a repository.
This is a followup to the discussion we had when I sent in a patch to
add '--no-reflog' option to 'git refs migrate' [1].

While 'git-reflog(1)' currently allows users to expire reflogs and
delete individual entries, it lacks functionality to completely remove
reflogs for specific references. This becomes problematic in
repositories where reflogs are not needed but continue to accumulate
entries despite setting 'core.logAllRefUpdates=false'.

The first patch is a small cleanup which ensures that 'builtin/reflog.c' no
longer uses global variables. The second patch add the required changes.

[1]: https://lore.kernel.org/all/xmqqa5aqu7g9.fsf@gitster.g/



base-commit: e969bc875963a10890d61ba84eab3a460bd9e535
change-id: 20250306-493-add-command-to-purge-reflog-entries-bd22547ad34a

Thanks
- Karthik


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/2] reflog: drop usage of global variables
  2025-03-07 11:17 [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Karthik Nayak
@ 2025-03-07 11:17 ` Karthik Nayak
  2025-03-07 21:19   ` Junio C Hamano
  2025-03-07 11:17 ` [PATCH 2/2] reflog: implement subcommand to drop reflogs Karthik Nayak
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 30+ messages in thread
From: Karthik Nayak @ 2025-03-07 11:17 UTC (permalink / raw)
  To: git; +Cc: Karthik Nayak

The 'builtin/reflog.c' file uses the 'the_repository' global variable
directly and also via 'git_config()'. Since this is a builtin command
which has access to the 'struct repository', drop usage of the global
variable and use the available repository struct.

With this, remove the 'USE_THE_REPOSITORY_VARIABLE' macro from the file.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/reflog.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 95f264989b..f92258f6b6 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "builtin.h"
 #include "config.h"
 #include "gettext.h"
@@ -236,7 +234,7 @@ static int expire_total_callback(const struct option *opt,
 }
 
 static int cmd_reflog_show(int argc, const char **argv, const char *prefix,
-			   struct repository *repo UNUSED)
+			   struct repository *repo)
 {
 	struct option options[] = {
 		OPT_END()
@@ -246,7 +244,7 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix,
 		      PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0 |
 		      PARSE_OPT_KEEP_UNKNOWN_OPT);
 
-	return cmd_log_reflog(argc, argv, prefix, the_repository);
+	return cmd_log_reflog(argc, argv, prefix, repo);
 }
 
 static int show_reflog(const char *refname, void *cb_data UNUSED)
@@ -256,7 +254,7 @@ static int show_reflog(const char *refname, void *cb_data UNUSED)
 }
 
 static int cmd_reflog_list(int argc, const char **argv, const char *prefix,
-			   struct repository *repo UNUSED)
+			   struct repository *repo)
 {
 	struct option options[] = {
 		OPT_END()
@@ -268,13 +266,13 @@ static int cmd_reflog_list(int argc, const char **argv, const char *prefix,
 		return error(_("%s does not accept arguments: '%s'"),
 			     "list", argv[0]);
 
-	ref_store = get_main_ref_store(the_repository);
+	ref_store = get_main_ref_store(repo);
 
 	return refs_for_each_reflog(ref_store, show_reflog, NULL);
 }
 
 static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
-			     struct repository *repo UNUSED)
+			     struct repository *repo)
 {
 	struct cmd_reflog_expire_cb cmd = { 0 };
 	timestamp_t now = time(NULL);
@@ -310,7 +308,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 
 	default_reflog_expire_unreachable = now - 30 * 24 * 3600;
 	default_reflog_expire = now - 90 * 24 * 3600;
-	git_config(reflog_expire_config, NULL);
+	repo_config(repo, reflog_expire_config, NULL);
 
 	save_commit_buffer = 0;
 	do_all = status = 0;
@@ -332,7 +330,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 	if (cmd.stalefix) {
 		struct rev_info revs;
 
-		repo_init_revisions(the_repository, &revs, prefix);
+		repo_init_revisions(repo, &revs, prefix);
 		revs.do_not_die_on_missing_objects = 1;
 		revs.ignore_missing = 1;
 		revs.ignore_missing_links = 1;
@@ -368,7 +366,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 			};
 
 			set_reflog_expiry_param(&cb.cmd,  item->string);
-			status |= refs_reflog_expire(get_main_ref_store(the_repository),
+			status |= refs_reflog_expire(get_main_ref_store(repo),
 						     item->string, flags,
 						     reflog_expiry_prepare,
 						     should_prune_fn,
@@ -382,12 +380,12 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 		char *ref;
 		struct expire_reflog_policy_cb cb = { .cmd = cmd };
 
-		if (!repo_dwim_log(the_repository, argv[i], strlen(argv[i]), NULL, &ref)) {
+		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
 		}
 		set_reflog_expiry_param(&cb.cmd, ref);
-		status |= refs_reflog_expire(get_main_ref_store(the_repository),
+		status |= refs_reflog_expire(get_main_ref_store(repo),
 					     ref, flags,
 					     reflog_expiry_prepare,
 					     should_prune_fn,
@@ -430,7 +428,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix,
 }
 
 static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
-			     struct repository *repo UNUSED)
+			     struct repository *repo)
 {
 	struct option options[] = {
 		OPT_END()
@@ -445,7 +443,7 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
 	refname = argv[0];
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
 		die(_("invalid ref format: %s"), refname);
-	return !refs_reflog_exists(get_main_ref_store(the_repository),
+	return !refs_reflog_exists(get_main_ref_store(repo),
 				   refname);
 }
 

-- 
2.48.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/2] reflog: implement subcommand to drop reflogs
  2025-03-07 11:17 [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Karthik Nayak
  2025-03-07 11:17 ` [PATCH 1/2] reflog: drop usage of global variables Karthik Nayak
@ 2025-03-07 11:17 ` Karthik Nayak
  2025-03-07 11:50   ` Patrick Steinhardt
  2025-03-10  7:39 ` [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Kristoffer Haugsbakk
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 30+ messages in thread
From: Karthik Nayak @ 2025-03-07 11:17 UTC (permalink / raw)
  To: git; +Cc: Karthik Nayak

Add a new 'drop' subcommand to git-reflog that allows users to delete
the entire reflog for a specified reference. Include a '--all' flag to
enable dropping all reflogs in a repository.

While 'git-reflog(1)' currently allows users to expire reflogs and
delete individual entries, it lacks functionality to completely remove
reflogs for specific references. This becomes problematic in
repositories where reflogs are not needed but continue to accumulate
entries despite setting 'core.logAllRefUpdates=false'.

While here, remove an erranous newline in the file.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-reflog.adoc |  6 +++++
 builtin/reflog.c              | 58 ++++++++++++++++++++++++++++++++++++++++++-
 t/t1410-reflog.sh             | 55 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 118 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
index a929c52982..4ecee297de 100644
--- a/Documentation/git-reflog.adoc
+++ b/Documentation/git-reflog.adoc
@@ -17,6 +17,7 @@ SYNOPSIS
 'git reflog delete' [--rewrite] [--updateref]
 	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
 'git reflog exists' <ref>
+'git reflog drop' [--all | <refs>...]
 
 DESCRIPTION
 -----------
@@ -57,6 +58,11 @@ The "exists" subcommand checks whether a ref has a reflog.  It exits
 with zero status if the reflog exists, and non-zero status if it does
 not.
 
+The "drop" subcommand removes the reflog for the specified references.
+In contrast, "expire" can be used to prune all entries from a reflog,
+but the reflog itself will still exist for that reference. To fully
+remove the reflog for specific references, use the "drop" subcommand.
+
 OPTIONS
 -------
 
diff --git a/builtin/reflog.c b/builtin/reflog.c
index f92258f6b6..232602c1a6 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -27,6 +27,9 @@
 #define BUILTIN_REFLOG_EXISTS_USAGE \
 	N_("git reflog exists <ref>")
 
+#define BUILTIN_REFLOG_DROP_USAGE \
+	N_("git reflog drop [--all | <refs>...]")
+
 static const char *const reflog_show_usage[] = {
 	BUILTIN_REFLOG_SHOW_USAGE,
 	NULL,
@@ -52,12 +55,18 @@ static const char *const reflog_exists_usage[] = {
 	NULL,
 };
 
+static const char *const reflog_drop_usage[] = {
+	BUILTIN_REFLOG_DROP_USAGE,
+	NULL,
+};
+
 static const char *const reflog_usage[] = {
 	BUILTIN_REFLOG_SHOW_USAGE,
 	BUILTIN_REFLOG_LIST_USAGE,
 	BUILTIN_REFLOG_EXPIRE_USAGE,
 	BUILTIN_REFLOG_DELETE_USAGE,
 	BUILTIN_REFLOG_EXISTS_USAGE,
+	BUILTIN_REFLOG_DROP_USAGE,
 	NULL
 };
 
@@ -447,10 +456,56 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
 				   refname);
 }
 
+static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
+			   struct repository *repo)
+{
+	int i, ret, do_all;
+	const struct option options[] = {
+		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
+		OPT_END()
+	};
+
+	do_all = ret = 0;
+	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
+
+	if (do_all) {
+		struct worktree_reflogs collected = {
+			.reflogs = STRING_LIST_INIT_DUP,
+		};
+		struct string_list_item *item;
+		struct worktree **worktrees, **p;
+
+		worktrees = get_worktrees();
+		for (p = worktrees; *p; p++) {
+			collected.worktree = *p;
+			refs_for_each_reflog(get_worktree_ref_store(*p),
+					     collect_reflog, &collected);
+		}
+		free_worktrees(worktrees);
+
+		for_each_string_list_item(item, &collected.reflogs)
+			ret |= refs_delete_reflog(get_main_ref_store(repo),
+						     item->string);
+		string_list_clear(&collected.reflogs, 0);
+	}
+
+	for (i = 0; i < argc; i++) {
+		char *ref;
+		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
+			ret |= error(_("%s points nowhere!"), argv[i]);
+			continue;
+		}
+
+		ret |= refs_delete_reflog(get_main_ref_store(repo), ref);
+		free(ref);
+	}
+
+	return ret;
+}
+
 /*
  * main "reflog"
  */
-
 int cmd_reflog(int argc,
 	       const char **argv,
 	       const char *prefix,
@@ -463,6 +518,7 @@ int cmd_reflog(int argc,
 		OPT_SUBCOMMAND("expire", &fn, cmd_reflog_expire),
 		OPT_SUBCOMMAND("delete", &fn, cmd_reflog_delete),
 		OPT_SUBCOMMAND("exists", &fn, cmd_reflog_exists),
+		OPT_SUBCOMMAND("drop", &fn, cmd_reflog_drop),
 		OPT_END()
 	};
 
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 388fdf9ae5..b6e44ce6b9 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -551,4 +551,59 @@ test_expect_success 'reflog with invalid object ID can be listed' '
 	)
 '
 
+test_expect_success 'reflog drop non-existent ref' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_must_fail git reflog exists refs/heads/non-existent &&
+		test_must_fail git reflog drop refs/heads/non-existent
+	)
+'
+
+test_expect_success 'reflog drop' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch
+	)
+'
+
+test_expect_success 'reflog drop multiple references' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop refs/heads/main refs/heads/branch &&
+		test_must_fail git reflog exists refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/branch
+	)
+'
+
+test_expect_success 'reflog drop --all' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop --all &&
+		test_must_fail git reflog exists refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/branch
+	)
+'
+
 test_done

-- 
2.48.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] reflog: implement subcommand to drop reflogs
  2025-03-07 11:17 ` [PATCH 2/2] reflog: implement subcommand to drop reflogs Karthik Nayak
@ 2025-03-07 11:50   ` Patrick Steinhardt
  2025-03-07 12:53     ` Karthik Nayak
  2025-03-07 21:28     ` Junio C Hamano
  0 siblings, 2 replies; 30+ messages in thread
From: Patrick Steinhardt @ 2025-03-07 11:50 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git

On Fri, Mar 07, 2025 at 12:17:26PM +0100, Karthik Nayak wrote:
> Add a new 'drop' subcommand to git-reflog that allows users to delete
> the entire reflog for a specified reference. Include a '--all' flag to
> enable dropping all reflogs in a repository.
> 
> While 'git-reflog(1)' currently allows users to expire reflogs and
> delete individual entries, it lacks functionality to completely remove
> reflogs for specific references. This becomes problematic in
> repositories where reflogs are not needed but continue to accumulate
> entries despite setting 'core.logAllRefUpdates=false'.

I think the order of the two paragraphs should be switched: we tend to
first explain the problem before explaining how to address it.

> While here, remove an erranous newline in the file.

I suspet this should either be "extraneous" or "erroneous"? I cannot
quite tell which of both it shuld be :)

> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-reflog.adoc |  6 +++++
>  builtin/reflog.c              | 58 ++++++++++++++++++++++++++++++++++++++++++-
>  t/t1410-reflog.sh             | 55 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
> index a929c52982..4ecee297de 100644
> --- a/Documentation/git-reflog.adoc
> +++ b/Documentation/git-reflog.adoc
> @@ -17,6 +17,7 @@ SYNOPSIS
>  'git reflog delete' [--rewrite] [--updateref]
>  	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
>  'git reflog exists' <ref>
> +'git reflog drop' [--all | <refs>...]

Should we put the command next to `delete`?

>  DESCRIPTION
>  -----------
> @@ -57,6 +58,11 @@ The "exists" subcommand checks whether a ref has a reflog.  It exits
>  with zero status if the reflog exists, and non-zero status if it does
>  not.
>  
> +The "drop" subcommand removes the reflog for the specified references.
> +In contrast, "expire" can be used to prune all entries from a reflog,
> +but the reflog itself will still exist for that reference. To fully
> +remove the reflog for specific references, use the "drop" subcommand.

The last sentence feels like pointless duplication to me. We should
likely also point out how it is different from "delete". How about:

    The "drop" subcommand completely removes the reflog for the
    specified references. This is in contrast to "expire" and "delete",
    both of which can be used to delete reflog entries, but not the
    reflog itself.

It might also be useful to add a comment to "delete" to say that it
deletes entries, but not the reflog.

>  OPTIONS
>  -------
>  
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index f92258f6b6..232602c1a6 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -27,6 +27,9 @@
>  #define BUILTIN_REFLOG_EXISTS_USAGE \
>  	N_("git reflog exists <ref>")
>  
> +#define BUILTIN_REFLOG_DROP_USAGE \
> +	N_("git reflog drop [--all | <refs>...]")
> +
>  static const char *const reflog_show_usage[] = {
>  	BUILTIN_REFLOG_SHOW_USAGE,
>  	NULL,
> @@ -52,12 +55,18 @@ static const char *const reflog_exists_usage[] = {
>  	NULL,
>  };
>  
> +static const char *const reflog_drop_usage[] = {
> +	BUILTIN_REFLOG_DROP_USAGE,
> +	NULL,
> +};
> +
>  static const char *const reflog_usage[] = {
>  	BUILTIN_REFLOG_SHOW_USAGE,
>  	BUILTIN_REFLOG_LIST_USAGE,
>  	BUILTIN_REFLOG_EXPIRE_USAGE,
>  	BUILTIN_REFLOG_DELETE_USAGE,
>  	BUILTIN_REFLOG_EXISTS_USAGE,
> +	BUILTIN_REFLOG_DROP_USAGE,
>  	NULL
>  };
>  
> @@ -447,10 +456,56 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
>  				   refname);
>  }
>  
> +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
> +			   struct repository *repo)
> +{
> +	int i, ret, do_all;
> +	const struct option options[] = {
> +		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
> +		OPT_END()
> +	};
> +
> +	do_all = ret = 0;

Can't we initiailize the variables directly when declaring them?

> +	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
> +
> +	if (do_all) {

`do_all` and `argc > 0` should be mutually exclusive from my point of
view, as the combination does not make any sense. We should likely die
if we see both to be non-zero. Similarly, I think we should abort on
`!do_all && !argc`.

> +		struct worktree_reflogs collected = {
> +			.reflogs = STRING_LIST_INIT_DUP,
> +		};
> +		struct string_list_item *item;
> +		struct worktree **worktrees, **p;

Would it be useful to point out in the docs that we also prune logs of
worktrees?

> +		worktrees = get_worktrees();
> +		for (p = worktrees; *p; p++) {
> +			collected.worktree = *p;
> +			refs_for_each_reflog(get_worktree_ref_store(*p),
> +					     collect_reflog, &collected);
> +		}
> +		free_worktrees(worktrees);
> +
> +		for_each_string_list_item(item, &collected.reflogs)
> +			ret |= refs_delete_reflog(get_main_ref_store(repo),
> +						     item->string);
> +		string_list_clear(&collected.reflogs, 0);
> +	}
> +
> +	for (i = 0; i < argc; i++) {
> +		char *ref;
> +		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
> +			ret |= error(_("%s points nowhere!"), argv[i]);
> +			continue;
> +		}

Is there a particular reason why we have to double check that the reflog
that we just enumerated really exists? It feels rather unnecessary to
me.

> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index 388fdf9ae5..b6e44ce6b9 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -551,4 +551,59 @@ test_expect_success 'reflog with invalid object ID can be listed' '
>  	)
>  '
>  
> +test_expect_success 'reflog drop non-existent ref' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_must_fail git reflog exists refs/heads/non-existent &&
> +		test_must_fail git reflog drop refs/heads/non-existent

Do we want to check the error message of the latter command?

Patrick

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] reflog: implement subcommand to drop reflogs
  2025-03-07 11:50   ` Patrick Steinhardt
@ 2025-03-07 12:53     ` Karthik Nayak
  2025-03-07 12:59       ` Patrick Steinhardt
  2025-03-07 21:28     ` Junio C Hamano
  1 sibling, 1 reply; 30+ messages in thread
From: Karthik Nayak @ 2025-03-07 12:53 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 6951 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Mar 07, 2025 at 12:17:26PM +0100, Karthik Nayak wrote:
>> Add a new 'drop' subcommand to git-reflog that allows users to delete
>> the entire reflog for a specified reference. Include a '--all' flag to
>> enable dropping all reflogs in a repository.
>>
>> While 'git-reflog(1)' currently allows users to expire reflogs and
>> delete individual entries, it lacks functionality to completely remove
>> reflogs for specific references. This becomes problematic in
>> repositories where reflogs are not needed but continue to accumulate
>> entries despite setting 'core.logAllRefUpdates=false'.
>
> I think the order of the two paragraphs should be switched: we tend to
> first explain the problem before explaining how to address it.
>

That makes sense, let me do that.

>> While here, remove an erranous newline in the file.
>
> I suspet this should either be "extraneous" or "erroneous"? I cannot
> quite tell which of both it shuld be :)
>

I was thinking similar but decided to go with the latter (sans typo).
Will fix it.

>
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  Documentation/git-reflog.adoc |  6 +++++
>>  builtin/reflog.c              | 58 ++++++++++++++++++++++++++++++++++++++++++-
>>  t/t1410-reflog.sh             | 55 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
>> index a929c52982..4ecee297de 100644
>> --- a/Documentation/git-reflog.adoc
>> +++ b/Documentation/git-reflog.adoc
>> @@ -17,6 +17,7 @@ SYNOPSIS
>>  'git reflog delete' [--rewrite] [--updateref]
>>  	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
>>  'git reflog exists' <ref>
>> +'git reflog drop' [--all | <refs>...]
>
> Should we put the command next to `delete`?
>

I would have like it if they were actually alphabetically sorted, but I
guess this is good alternative to cluster similar sub-commands.

>>  DESCRIPTION
>>  -----------
>> @@ -57,6 +58,11 @@ The "exists" subcommand checks whether a ref has a reflog.  It exits
>>  with zero status if the reflog exists, and non-zero status if it does
>>  not.
>>
>> +The "drop" subcommand removes the reflog for the specified references.
>> +In contrast, "expire" can be used to prune all entries from a reflog,
>> +but the reflog itself will still exist for that reference. To fully
>> +remove the reflog for specific references, use the "drop" subcommand.
>
> The last sentence feels like pointless duplication to me. We should
> likely also point out how it is different from "delete". How about:
>
>     The "drop" subcommand completely removes the reflog for the
>     specified references. This is in contrast to "expire" and "delete",
>     both of which can be used to delete reflog entries, but not the
>     reflog itself.
>
> It might also be useful to add a comment to "delete" to say that it
> deletes entries, but not the reflog.
>

Thanks, this does look better and I agree, we should call out "delete"
too.

>>  OPTIONS
>>  -------
>>
>> diff --git a/builtin/reflog.c b/builtin/reflog.c
>> index f92258f6b6..232602c1a6 100644
>> --- a/builtin/reflog.c
>> +++ b/builtin/reflog.c
>> @@ -27,6 +27,9 @@
>>  #define BUILTIN_REFLOG_EXISTS_USAGE \
>>  	N_("git reflog exists <ref>")
>>
>> +#define BUILTIN_REFLOG_DROP_USAGE \
>> +	N_("git reflog drop [--all | <refs>...]")
>> +
>>  static const char *const reflog_show_usage[] = {
>>  	BUILTIN_REFLOG_SHOW_USAGE,
>>  	NULL,
>> @@ -52,12 +55,18 @@ static const char *const reflog_exists_usage[] = {
>>  	NULL,
>>  };
>>
>> +static const char *const reflog_drop_usage[] = {
>> +	BUILTIN_REFLOG_DROP_USAGE,
>> +	NULL,
>> +};
>> +
>>  static const char *const reflog_usage[] = {
>>  	BUILTIN_REFLOG_SHOW_USAGE,
>>  	BUILTIN_REFLOG_LIST_USAGE,
>>  	BUILTIN_REFLOG_EXPIRE_USAGE,
>>  	BUILTIN_REFLOG_DELETE_USAGE,
>>  	BUILTIN_REFLOG_EXISTS_USAGE,
>> +	BUILTIN_REFLOG_DROP_USAGE,
>>  	NULL
>>  };
>>
>> @@ -447,10 +456,56 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
>>  				   refname);
>>  }
>>
>> +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
>> +			   struct repository *repo)
>> +{
>> +	int i, ret, do_all;
>> +	const struct option options[] = {
>> +		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
>> +		OPT_END()
>> +	};
>> +
>> +	do_all = ret = 0;
>
> Can't we initiailize the variables directly when declaring them?
>

We can, let me fix it! I'll also move the initialization of 'i' down to
the loop while we're here.

>> +	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
>> +
>> +	if (do_all) {
>
> `do_all` and `argc > 0` should be mutually exclusive from my point of
> view, as the combination does not make any sense. We should likely die
> if we see both to be non-zero. Similarly, I think we should abort on
> `!do_all && !argc`.
>

Makes sense, let me add a 'die()' there.

>> +		struct worktree_reflogs collected = {
>> +			.reflogs = STRING_LIST_INIT_DUP,
>> +		};
>> +		struct string_list_item *item;
>> +		struct worktree **worktrees, **p;
>
> Would it be useful to point out in the docs that we also prune logs of
> worktrees?
>

Yes it would, will add.

>> +		worktrees = get_worktrees();
>> +		for (p = worktrees; *p; p++) {
>> +			collected.worktree = *p;
>> +			refs_for_each_reflog(get_worktree_ref_store(*p),
>> +					     collect_reflog, &collected);
>> +		}
>> +		free_worktrees(worktrees);
>> +
>> +		for_each_string_list_item(item, &collected.reflogs)
>> +			ret |= refs_delete_reflog(get_main_ref_store(repo),
>> +						     item->string);
>> +		string_list_clear(&collected.reflogs, 0);
>> +	}
>> +
>> +	for (i = 0; i < argc; i++) {
>> +		char *ref;
>> +		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
>> +			ret |= error(_("%s points nowhere!"), argv[i]);
>> +			continue;
>> +		}
>
> Is there a particular reason why we have to double check that the reflog
> that we just enumerated really exists? It feels rather unnecessary to
> me.
>

You mean we could directly do `(get_main_ref_store(repo), argv[i]);` ?
The issue is that this returns '0', even when the reflog doesn't exist.
So to notify the user correctly, we do this check.

>> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
>> index 388fdf9ae5..b6e44ce6b9 100755
>> --- a/t/t1410-reflog.sh
>> +++ b/t/t1410-reflog.sh
>> @@ -551,4 +551,59 @@ test_expect_success 'reflog with invalid object ID can be listed' '
>>  	)
>>  '
>>
>> +test_expect_success 'reflog drop non-existent ref' '
>> +	test_when_finished "rm -rf repo" &&
>> +	git init repo &&
>> +	(
>> +		cd repo &&
>> +		test_must_fail git reflog exists refs/heads/non-existent &&
>> +		test_must_fail git reflog drop refs/heads/non-existent
>
> Do we want to check the error message of the latter command?
>

That would be nice addittion, will add.

> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] reflog: implement subcommand to drop reflogs
  2025-03-07 12:53     ` Karthik Nayak
@ 2025-03-07 12:59       ` Patrick Steinhardt
  2025-03-07 13:28         ` Karthik Nayak
  2025-03-07 13:28         ` Karthik Nayak
  0 siblings, 2 replies; 30+ messages in thread
From: Patrick Steinhardt @ 2025-03-07 12:59 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git

On Fri, Mar 07, 2025 at 06:53:31AM -0600, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Fri, Mar 07, 2025 at 12:17:26PM +0100, Karthik Nayak wrote:
> >> @@ -447,10 +456,56 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
> >>  				   refname);
> >>  }
> >>
> >> +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
> >> +			   struct repository *repo)
> >> +{
> >> +	int i, ret, do_all;
> >> +	const struct option options[] = {
> >> +		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
> >> +		OPT_END()
> >> +	};
> >> +
> >> +	do_all = ret = 0;
> >
> > Can't we initiailize the variables directly when declaring them?
> >
> 
> We can, let me fix it! I'll also move the initialization of 'i' down to
> the loop while we're here.

You can also avoid declaring `i` here at all and just declare it inside
the loop.

Patrick

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] reflog: implement subcommand to drop reflogs
  2025-03-07 12:59       ` Patrick Steinhardt
@ 2025-03-07 13:28         ` Karthik Nayak
  2025-03-07 13:28         ` Karthik Nayak
  1 sibling, 0 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-07 13:28 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Mar 07, 2025 at 06:53:31AM -0600, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> > On Fri, Mar 07, 2025 at 12:17:26PM +0100, Karthik Nayak wrote:
>> >> @@ -447,10 +456,56 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
>> >>  				   refname);
>> >>  }
>> >>
>> >> +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
>> >> +			   struct repository *repo)
>> >> +{
>> >> +	int i, ret, do_all;
>> >> +	const struct option options[] = {
>> >> +		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
>> >> +		OPT_END()
>> >> +	};
>> >> +
>> >> +	do_all = ret = 0;
>> >
>> > Can't we initiailize the variables directly when declaring them?
>> >
>>
>> We can, let me fix it! I'll also move the initialization of 'i' down to
>> the loop while we're here.
>
> You can also avoid declaring `i` here at all and just declare it inside
> the loop.
>
> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] reflog: implement subcommand to drop reflogs
  2025-03-07 12:59       ` Patrick Steinhardt
  2025-03-07 13:28         ` Karthik Nayak
@ 2025-03-07 13:28         ` Karthik Nayak
  1 sibling, 0 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-07 13:28 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Mar 07, 2025 at 06:53:31AM -0600, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> > On Fri, Mar 07, 2025 at 12:17:26PM +0100, Karthik Nayak wrote:
>> >> @@ -447,10 +456,56 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
>> >>  				   refname);
>> >>  }
>> >>
>> >> +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
>> >> +			   struct repository *repo)
>> >> +{
>> >> +	int i, ret, do_all;
>> >> +	const struct option options[] = {
>> >> +		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
>> >> +		OPT_END()
>> >> +	};
>> >> +
>> >> +	do_all = ret = 0;
>> >
>> > Can't we initiailize the variables directly when declaring them?
>> >
>>
>> We can, let me fix it! I'll also move the initialization of 'i' down to
>> the loop while we're here.
>
> You can also avoid declaring `i` here at all and just declare it inside
> the loop.
>
> Patrick

Yup, this is what I was trying to convey too :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] reflog: drop usage of global variables
  2025-03-07 11:17 ` [PATCH 1/2] reflog: drop usage of global variables Karthik Nayak
@ 2025-03-07 21:19   ` Junio C Hamano
  2025-03-10 11:41     ` Karthik Nayak
  0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-03-07 21:19 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git

Karthik Nayak <karthik.188@gmail.com> writes:

> The 'builtin/reflog.c' file uses the 'the_repository' global variable
> directly and also via 'git_config()'. Since this is a builtin command
> which has access to the 'struct repository', drop usage of the global
> variable and use the available repository struct.
>
> With this, remove the 'USE_THE_REPOSITORY_VARIABLE' macro from the file.

I suspect that this is not quite right.

    $ cd w/git.git/; make
    $ ./git-reflog list -h
    usage: git reflog list
    $ cd .. # not a repository
    $ git.git/git-reflog list -h
    fatal: not a git repository (or any of the parent directories): .git
    $ git.git/git-reflog -h
    usage: git reflog [show] ...

but I also suspect that it is mostly due to the original program
structure that uses OPT_SUBCOMMAND() that the subcommands fail to
respond to "-h" unlike the top-level command, so this may not be a
regression.  I do think however that this change is making it harder
to fix.

In any case, when you are adding a new feature, I would strongly
prefer you did *not* take it hostage to unrelated internal clean-up
with a dubious value.  For the library-ish parts of the system
(e.g. reflog.c at the top-level), not depending on the_repository is
absolutely the good thing to do, but the top level cmd_foo() are not
meant to be called as helper functions repeatedly with arbirary
repository instance, and a churn like this one, only to mollify the
USE_THE_REPOSITORY_VARIABLE macro, does not deserve to take a more
interesting (in the sense that it improves the life of end users)
change hostage by pretending to be its prerequisite clean-up.

Thanks.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] reflog: implement subcommand to drop reflogs
  2025-03-07 11:50   ` Patrick Steinhardt
  2025-03-07 12:53     ` Karthik Nayak
@ 2025-03-07 21:28     ` Junio C Hamano
  2025-03-10 11:28       ` Karthik Nayak
  1 sibling, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-03-07 21:28 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: Karthik Nayak, git

Patrick Steinhardt <ps@pks.im> writes:

>> +The "drop" subcommand removes the reflog for the specified references.
>> +In contrast, "expire" can be used to prune all entries from a reflog,
>> +but the reflog itself will still exist for that reference. To fully
>> +remove the reflog for specific references, use the "drop" subcommand.
>
> The last sentence feels like pointless duplication to me. We should
> likely also point out how it is different from "delete". How about:
>
>     The "drop" subcommand completely removes the reflog for the
>     specified references. This is in contrast to "expire" and "delete",
>     both of which can be used to delete reflog entries, but not the
>     reflog itself.
>
> It might also be useful to add a comment to "delete" to say that it
> deletes entries, but not the reflog.

Good.


>> +#define BUILTIN_REFLOG_DROP_USAGE \
>> +	N_("git reflog drop [--all | <refs>...]")
>> +

We need a matching change to Documentation/git-reflog.adoc file,
too, right?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries
  2025-03-07 11:17 [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Karthik Nayak
  2025-03-07 11:17 ` [PATCH 1/2] reflog: drop usage of global variables Karthik Nayak
  2025-03-07 11:17 ` [PATCH 2/2] reflog: implement subcommand to drop reflogs Karthik Nayak
@ 2025-03-10  7:39 ` Kristoffer Haugsbakk
  2025-03-10 12:34   ` Karthik Nayak
  2025-03-10 15:28   ` Junio C Hamano
  2025-03-10 12:36 ` [PATCH v2] reflog: implement subcommand to drop reflogs Karthik Nayak
  2025-03-14  8:40 ` [PATCH v3 0/2] " Karthik Nayak
  4 siblings, 2 replies; 30+ messages in thread
From: Kristoffer Haugsbakk @ 2025-03-10  7:39 UTC (permalink / raw)
  To: Karthik Nayak, git

On Fri, Mar 7, 2025, at 12:17, Karthik Nayak wrote:
> This patch series adds a new 'drop' subcommand to git-reflog that allows
> users to delete the reflog for a specified reference. Additionally, it
> adds an '--all' flag to enable dropping all reflogs in a repository.
> This is a followup to the discussion we had when I sent in a patch to
> add '--no-reflog' option to 'git refs migrate' [1].

I’ve been wanting a command to drop reflogs.  I use `always` and get a
lot of entries that I don’t care about.  But I don’t want to set it to
`true` because I care about some of them.

So this is great.

-- 
Kristoffer Haugsbakk



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] reflog: implement subcommand to drop reflogs
  2025-03-07 21:28     ` Junio C Hamano
@ 2025-03-10 11:28       ` Karthik Nayak
  0 siblings, 0 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-10 11:28 UTC (permalink / raw)
  To: Junio C Hamano, Patrick Steinhardt; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>>> +The "drop" subcommand removes the reflog for the specified references.
>>> +In contrast, "expire" can be used to prune all entries from a reflog,
>>> +but the reflog itself will still exist for that reference. To fully
>>> +remove the reflog for specific references, use the "drop" subcommand.
>>
>> The last sentence feels like pointless duplication to me. We should
>> likely also point out how it is different from "delete". How about:
>>
>>     The "drop" subcommand completely removes the reflog for the
>>     specified references. This is in contrast to "expire" and "delete",
>>     both of which can be used to delete reflog entries, but not the
>>     reflog itself.
>>
>> It might also be useful to add a comment to "delete" to say that it
>> deletes entries, but not the reflog.
>
> Good.
>
>
>>> +#define BUILTIN_REFLOG_DROP_USAGE \
>>> +	N_("git reflog drop [--all | <refs>...]")
>>> +
>
> We need a matching change to Documentation/git-reflog.adoc file,
> too, right?

I'm assuming you mean documentation specifically for the '--all' option,
I will include that in the next version.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] reflog: drop usage of global variables
  2025-03-07 21:19   ` Junio C Hamano
@ 2025-03-10 11:41     ` Karthik Nayak
  2025-03-10 15:24       ` Junio C Hamano
  0 siblings, 1 reply; 30+ messages in thread
From: Karthik Nayak @ 2025-03-10 11:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 4714 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The 'builtin/reflog.c' file uses the 'the_repository' global variable
>> directly and also via 'git_config()'. Since this is a builtin command
>> which has access to the 'struct repository', drop usage of the global
>> variable and use the available repository struct.
>>
>> With this, remove the 'USE_THE_REPOSITORY_VARIABLE' macro from the file.
>
> I suspect that this is not quite right.
>
>     $ cd w/git.git/; make
>     $ ./git-reflog list -h
>     usage: git reflog list
>     $ cd .. # not a repository
>     $ git.git/git-reflog list -h
>     fatal: not a git repository (or any of the parent directories): .git
>     $ git.git/git-reflog -h
>     usage: git reflog [show] ...
>
> but I also suspect that it is mostly due to the original program
> structure that uses OPT_SUBCOMMAND() that the subcommands fail to
> respond to "-h" unlike the top-level command, so this may not be a
> regression.  I do think however that this change is making it harder
> to fix.
>

Hmm. But this is the existing behavior, no?

  # Inside a git directory
  $ eza .git
  b4.template  branches  COMMIT_EDITMSG  config  description
FETCH_HEAD  filter-repo  HEAD  hooks  index  info  objects  refs
reftable  rr-cache

  $ git reflog -h
  usage: git reflog [show] [<log-options>] [<ref>]
     or: git reflog list
     or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
                           [--rewrite] [--updateref] [--stale-fix]
                           [--dry-run | -n] [--verbose] [--all
[--single-worktree] | <refs>...]
     or: git reflog delete [--rewrite] [--updateref]
                           [--dry-run | -n] [--verbose] <ref>@{<specifier>}...
     or: git reflog exists <ref>

  $ git reflog list -h
  usage: git reflog list

  $ ~/code/git/build/bin-wrappers/git reflog -h
  usage: git reflog [show] [<log-options>] [<ref>]
     or: git reflog list
     or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
                           [--rewrite] [--updateref] [--stale-fix]
                           [--dry-run | -n] [--verbose] [--all
[--single-worktree] | <refs>...]
     or: git reflog delete [--rewrite] [--updateref]
                           [--dry-run | -n] [--verbose] <ref>@{<specifier>}...
     or: git reflog exists <ref>
     or: git reflog drop [--all | <refs>...]

  $ ~/code/git/build/bin-wrappers/git reflog list -h
  usage: git reflog list

  # Outside a git repository
  $ eza .git
  ".git": No such file or directory (os error 2)

  $ git reflog -h
  usage: git reflog [show] [<log-options>] [<ref>]
     or: git reflog list
     or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
                           [--rewrite] [--updateref] [--stale-fix]
                           [--dry-run | -n] [--verbose] [--all
[--single-worktree] | <refs>...]
     or: git reflog delete [--rewrite] [--updateref]
                           [--dry-run | -n] [--verbose] <ref>@{<specifier>}...
     or: git reflog exists <ref>


  $ git reflog list -h
  fatal: not a git repository (or any of the parent directories): .git

  $ ~/code/git/build/bin-wrappers/git reflog -h
  usage: git reflog [show] [<log-options>] [<ref>]
     or: git reflog list
     or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
                           [--rewrite] [--updateref] [--stale-fix]
                           [--dry-run | -n] [--verbose] [--all
[--single-worktree] | <refs>...]
     or: git reflog delete [--rewrite] [--updateref]
                           [--dry-run | -n] [--verbose] <ref>@{<specifier>}...
     or: git reflog exists <ref>
     or: git reflog drop [--all | <refs>...]


  $ ~/code/git/build/bin-wrappers/git reflog list -h
  fatal: not a git repository (or any of the parent directories): .git

Seems like the behavior is the same with as with (git 2.48.1).

> In any case, when you are adding a new feature, I would strongly
> prefer you did *not* take it hostage to unrelated internal clean-up
> with a dubious value.  For the library-ish parts of the system
> (e.g. reflog.c at the top-level), not depending on the_repository is
> absolutely the good thing to do, but the top level cmd_foo() are not
> meant to be called as helper functions repeatedly with arbirary
> repository instance, and a churn like this one, only to mollify the
> USE_THE_REPOSITORY_VARIABLE macro, does not deserve to take a more
> interesting (in the sense that it improves the life of end users)
> change hostage by pretending to be its prerequisite clean-up.
>
> Thanks.

But point taken, I'll drop this patch in the next version! Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries
  2025-03-10  7:39 ` [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Kristoffer Haugsbakk
@ 2025-03-10 12:34   ` Karthik Nayak
  2025-03-10 15:28   ` Junio C Hamano
  1 sibling, 0 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-10 12:34 UTC (permalink / raw)
  To: Kristoffer Haugsbakk, git

[-- Attachment #1: Type: text/plain, Size: 814 bytes --]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> On Fri, Mar 7, 2025, at 12:17, Karthik Nayak wrote:
>> This patch series adds a new 'drop' subcommand to git-reflog that allows
>> users to delete the reflog for a specified reference. Additionally, it
>> adds an '--all' flag to enable dropping all reflogs in a repository.
>> This is a followup to the discussion we had when I sent in a patch to
>> add '--no-reflog' option to 'git refs migrate' [1].
>
> I’ve been wanting a command to drop reflogs.  I use `always` and get a
> lot of entries that I don’t care about.  But I don’t want to set it to
> `true` because I care about some of them.
>
> So this is great.
>

That's good to know! I was surprised that this didn't exist till date.

> --
> Kristoffer Haugsbakk

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2] reflog: implement subcommand to drop reflogs
  2025-03-07 11:17 [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Karthik Nayak
                   ` (2 preceding siblings ...)
  2025-03-10  7:39 ` [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Kristoffer Haugsbakk
@ 2025-03-10 12:36 ` Karthik Nayak
  2025-03-12  7:15   ` Patrick Steinhardt
  2025-03-14  8:40 ` [PATCH v3 0/2] " Karthik Nayak
  4 siblings, 1 reply; 30+ messages in thread
From: Karthik Nayak @ 2025-03-10 12:36 UTC (permalink / raw)
  To: git; +Cc: ps, kristofferhaugsbakk, gitster, Karthik Nayak

While 'git-reflog(1)' currently allows users to expire reflogs and
delete individual entries, it lacks functionality to completely remove
reflogs for specific references. This becomes problematic in
repositories where reflogs are not needed but continue to accumulate
entries despite setting 'core.logAllRefUpdates=false'.

Add a new 'drop' subcommand to git-reflog that allows users to delete
the entire reflog for a specified reference. Include a '--all' flag to
enable dropping all reflogs in a repository.

While here, remove an extraneous newline in the file.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v2:
- Rephrase the commit message to be clearer and fix typo.
- Move the documentation to be next to 'git reflog delete' and also
  add missing documentation for the '--all' flag.
- Ensure '--all' is not used with references and add a test.
- Cleanup variable assignment.
- Check for error message in the test.
- Drop the cleanup commit.
- Rebased on top of master a36e024e98 (Merge branch 'js/win-2.49-build-fixes',
  2025-03-06), this was to include the adoc changes which were breaking
  tests on the CI. 
- Link to v1: https://lore.kernel.org/r/20250307-493-add-command-to-purge-reflog-entries-v1-0-84ab8529cf9e@gmail.com

 Documentation/git-reflog.adoc | 18 +++++++++---
 builtin/reflog.c              | 60 +++++++++++++++++++++++++++++++++++++-
 t/t1410-reflog.sh             | 67 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+), 5 deletions(-)

Karthik Nayak (1):
      reflog: implement subcommand to drop reflogs

Range-diff versus v1:

1:  7c37f97eb0 < -:  ---------- reflog: drop usage of global variables
2:  d321bf14ae ! 1:  cac494d5cc reflog: implement subcommand to drop reflogs
    @@ Metadata
      ## Commit message ##
         reflog: implement subcommand to drop reflogs
     
    -    Add a new 'drop' subcommand to git-reflog that allows users to delete
    -    the entire reflog for a specified reference. Include a '--all' flag to
    -    enable dropping all reflogs in a repository.
    -
         While 'git-reflog(1)' currently allows users to expire reflogs and
         delete individual entries, it lacks functionality to completely remove
         reflogs for specific references. This becomes problematic in
         repositories where reflogs are not needed but continue to accumulate
         entries despite setting 'core.logAllRefUpdates=false'.
     
    -    While here, remove an erranous newline in the file.
    +    Add a new 'drop' subcommand to git-reflog that allows users to delete
    +    the entire reflog for a specified reference. Include a '--all' flag to
    +    enable dropping all reflogs in a repository.
    +
    +    While here, remove an extraneous newline in the file.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## Documentation/git-reflog.adoc ##
     @@ Documentation/git-reflog.adoc: SYNOPSIS
    + 	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
      'git reflog delete' [--rewrite] [--updateref]
      	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
    - 'git reflog exists' <ref>
     +'git reflog drop' [--all | <refs>...]
    + 'git reflog exists' <ref>
      
      DESCRIPTION
    - -----------
    -@@ Documentation/git-reflog.adoc: The "exists" subcommand checks whether a ref has a reflog.  It exits
    +@@ Documentation/git-reflog.adoc: and not reachable from the current tip, are removed from the reflog.
    + This is typically not used directly by end users -- instead, see
    + linkgit:git-gc[1].
    + 
    +-The "delete" subcommand deletes single entries from the reflog. Its
    +-argument must be an _exact_ entry (e.g. "`git reflog delete
    +-master@{2}`"). This subcommand is also typically not used directly by
    +-end users.
    ++The "delete" subcommand deletes single entries from the reflog, but
    ++not the reflog itself. Its argument must be an _exact_ entry (e.g. "`git
    ++reflog delete master@{2}`"). This subcommand is also typically not used
    ++directly by end users.
    + 
    + The "exists" subcommand checks whether a ref has a reflog.  It exits
      with zero status if the reflog exists, and non-zero status if it does
      not.
      
    -+The "drop" subcommand removes the reflog for the specified references.
    -+In contrast, "expire" can be used to prune all entries from a reflog,
    -+but the reflog itself will still exist for that reference. To fully
    -+remove the reflog for specific references, use the "drop" subcommand.
    ++The "drop" subcommand completely removes the reflog for the specified
    ++references. This is in contrast to "expire" and "delete", both of which
    ++can be used to delete reflog entries, but not the reflog itself.
     +
      OPTIONS
      -------
      
    +@@ Documentation/git-reflog.adoc: Options for `delete`
    + `--dry-run`, and `--verbose`, with the same meanings as when they are
    + used with `expire`.
    + 
    ++Options for `drop`
    ++~~~~~~~~~~~~~~~~~~~~
    ++
    ++--all::
    ++	Drop the reflogs of all references from all worktrees.
    + 
    + GIT
    + ---
     
      ## builtin/reflog.c ##
     @@
    @@ builtin/reflog.c: static const char *const reflog_exists_usage[] = {
      	BUILTIN_REFLOG_LIST_USAGE,
      	BUILTIN_REFLOG_EXPIRE_USAGE,
      	BUILTIN_REFLOG_DELETE_USAGE,
    - 	BUILTIN_REFLOG_EXISTS_USAGE,
     +	BUILTIN_REFLOG_DROP_USAGE,
    + 	BUILTIN_REFLOG_EXISTS_USAGE,
      	NULL
      };
    - 
     @@ builtin/reflog.c: static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
      				   refname);
      }
    @@ builtin/reflog.c: static int cmd_reflog_exists(int argc, const char **argv, cons
     +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
     +			   struct repository *repo)
     +{
    -+	int i, ret, do_all;
    ++	int ret = 0, do_all = 0;
     +	const struct option options[] = {
     +		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
     +		OPT_END()
     +	};
     +
    -+	do_all = ret = 0;
     +	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
     +
    ++	if (argc && do_all)
    ++		die(_("references specified along with --all"));
    ++
     +	if (do_all) {
     +		struct worktree_reflogs collected = {
     +			.reflogs = STRING_LIST_INIT_DUP,
    @@ builtin/reflog.c: static int cmd_reflog_exists(int argc, const char **argv, cons
     +		string_list_clear(&collected.reflogs, 0);
     +	}
     +
    -+	for (i = 0; i < argc; i++) {
    ++	for (int i = 0; i < argc; i++) {
     +		char *ref;
     +		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
     +			ret |= error(_("%s points nowhere!"), argv[i]);
    @@ t/t1410-reflog.sh: test_expect_success 'reflog with invalid object ID can be lis
     +	(
     +		cd repo &&
     +		test_must_fail git reflog exists refs/heads/non-existent &&
    -+		test_must_fail git reflog drop refs/heads/non-existent
    ++		test_must_fail git reflog drop refs/heads/non-existent 2>stderr &&
    ++		test_grep "error: refs/heads/non-existent points nowhere!" stderr
     +	)
     +'
     +
    @@ t/t1410-reflog.sh: test_expect_success 'reflog with invalid object ID can be lis
     +		test_must_fail git reflog exists refs/heads/branch
     +	)
     +'
    ++
    ++test_expect_success 'reflog drop --all with reference' '
    ++	test_when_finished "rm -rf repo" &&
    ++	git init repo &&
    ++	(
    ++		cd repo &&
    ++		test_commit A &&
    ++		test_must_fail git reflog drop --all refs/heads/main 2>stderr &&
    ++		test_grep "fatal: references specified along with --all" stderr
    ++	)
    ++'
     +
      test_done


base-commit: a36e024e989f4d35f35987a60e3af8022cac3420
change-id: 20250306-493-add-command-to-purge-reflog-entries-bd22547ad34a

Thanks
- Karthik
---

---
 Documentation/git-reflog.adoc | 18 +++++++++---
 builtin/reflog.c              | 60 +++++++++++++++++++++++++++++++++++++-
 t/t1410-reflog.sh             | 67 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
index a929c52982..6ed98ddaef 100644
--- a/Documentation/git-reflog.adoc
+++ b/Documentation/git-reflog.adoc
@@ -16,6 +16,7 @@ SYNOPSIS
 	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
 'git reflog delete' [--rewrite] [--updateref]
 	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
+'git reflog drop' [--all | <refs>...]
 'git reflog exists' <ref>
 
 DESCRIPTION
@@ -48,15 +49,19 @@ and not reachable from the current tip, are removed from the reflog.
 This is typically not used directly by end users -- instead, see
 linkgit:git-gc[1].
 
-The "delete" subcommand deletes single entries from the reflog. Its
-argument must be an _exact_ entry (e.g. "`git reflog delete
-master@{2}`"). This subcommand is also typically not used directly by
-end users.
+The "delete" subcommand deletes single entries from the reflog, but
+not the reflog itself. Its argument must be an _exact_ entry (e.g. "`git
+reflog delete master@{2}`"). This subcommand is also typically not used
+directly by end users.
 
 The "exists" subcommand checks whether a ref has a reflog.  It exits
 with zero status if the reflog exists, and non-zero status if it does
 not.
 
+The "drop" subcommand completely removes the reflog for the specified
+references. This is in contrast to "expire" and "delete", both of which
+can be used to delete reflog entries, but not the reflog itself.
+
 OPTIONS
 -------
 
@@ -132,6 +137,11 @@ Options for `delete`
 `--dry-run`, and `--verbose`, with the same meanings as when they are
 used with `expire`.
 
+Options for `drop`
+~~~~~~~~~~~~~~~~~~~~
+
+--all::
+	Drop the reflogs of all references from all worktrees.
 
 GIT
 ---
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 95f264989b..cd93a0bef9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -29,6 +29,9 @@
 #define BUILTIN_REFLOG_EXISTS_USAGE \
 	N_("git reflog exists <ref>")
 
+#define BUILTIN_REFLOG_DROP_USAGE \
+	N_("git reflog drop [--all | <refs>...]")
+
 static const char *const reflog_show_usage[] = {
 	BUILTIN_REFLOG_SHOW_USAGE,
 	NULL,
@@ -54,11 +57,17 @@ static const char *const reflog_exists_usage[] = {
 	NULL,
 };
 
+static const char *const reflog_drop_usage[] = {
+	BUILTIN_REFLOG_DROP_USAGE,
+	NULL,
+};
+
 static const char *const reflog_usage[] = {
 	BUILTIN_REFLOG_SHOW_USAGE,
 	BUILTIN_REFLOG_LIST_USAGE,
 	BUILTIN_REFLOG_EXPIRE_USAGE,
 	BUILTIN_REFLOG_DELETE_USAGE,
+	BUILTIN_REFLOG_DROP_USAGE,
 	BUILTIN_REFLOG_EXISTS_USAGE,
 	NULL
 };
@@ -449,10 +458,58 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
 				   refname);
 }
 
+static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
+			   struct repository *repo)
+{
+	int ret = 0, do_all = 0;
+	const struct option options[] = {
+		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
+
+	if (argc && do_all)
+		die(_("references specified along with --all"));
+
+	if (do_all) {
+		struct worktree_reflogs collected = {
+			.reflogs = STRING_LIST_INIT_DUP,
+		};
+		struct string_list_item *item;
+		struct worktree **worktrees, **p;
+
+		worktrees = get_worktrees();
+		for (p = worktrees; *p; p++) {
+			collected.worktree = *p;
+			refs_for_each_reflog(get_worktree_ref_store(*p),
+					     collect_reflog, &collected);
+		}
+		free_worktrees(worktrees);
+
+		for_each_string_list_item(item, &collected.reflogs)
+			ret |= refs_delete_reflog(get_main_ref_store(repo),
+						     item->string);
+		string_list_clear(&collected.reflogs, 0);
+	}
+
+	for (int i = 0; i < argc; i++) {
+		char *ref;
+		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
+			ret |= error(_("%s points nowhere!"), argv[i]);
+			continue;
+		}
+
+		ret |= refs_delete_reflog(get_main_ref_store(repo), ref);
+		free(ref);
+	}
+
+	return ret;
+}
+
 /*
  * main "reflog"
  */
-
 int cmd_reflog(int argc,
 	       const char **argv,
 	       const char *prefix,
@@ -465,6 +522,7 @@ int cmd_reflog(int argc,
 		OPT_SUBCOMMAND("expire", &fn, cmd_reflog_expire),
 		OPT_SUBCOMMAND("delete", &fn, cmd_reflog_delete),
 		OPT_SUBCOMMAND("exists", &fn, cmd_reflog_exists),
+		OPT_SUBCOMMAND("drop", &fn, cmd_reflog_drop),
 		OPT_END()
 	};
 
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 388fdf9ae5..251caaf9a4 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -551,4 +551,71 @@ test_expect_success 'reflog with invalid object ID can be listed' '
 	)
 '
 
+test_expect_success 'reflog drop non-existent ref' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_must_fail git reflog exists refs/heads/non-existent &&
+		test_must_fail git reflog drop refs/heads/non-existent 2>stderr &&
+		test_grep "error: refs/heads/non-existent points nowhere!" stderr
+	)
+'
+
+test_expect_success 'reflog drop' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch
+	)
+'
+
+test_expect_success 'reflog drop multiple references' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop refs/heads/main refs/heads/branch &&
+		test_must_fail git reflog exists refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/branch
+	)
+'
+
+test_expect_success 'reflog drop --all' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop --all &&
+		test_must_fail git reflog exists refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/branch
+	)
+'
+
+test_expect_success 'reflog drop --all with reference' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_must_fail git reflog drop --all refs/heads/main 2>stderr &&
+		test_grep "fatal: references specified along with --all" stderr
+	)
+'
+
 test_done




^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] reflog: drop usage of global variables
  2025-03-10 11:41     ` Karthik Nayak
@ 2025-03-10 15:24       ` Junio C Hamano
  2025-03-13 13:30         ` Karthik Nayak
  0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-03-10 15:24 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git

Karthik Nayak <karthik.188@gmail.com> writes:

>> but I also suspect that it is mostly due to the original program
>> structure that uses OPT_SUBCOMMAND() that the subcommands fail to
>> respond to "-h" unlike the top-level command, so this may not be a
>> regression.  I do think however that this change is making it harder
>> to fix.
>>
>
> Hmm. But this is the existing behavior, no?

Didn't I said I also suspect?

> But point taken, I'll drop this patch in the next version! Thanks

Yup.  Take your time, as it is already deep in prerelease feature
freeze.  I'd prefer to see us leaving spare capacity in our minds to
fix regressions introduced during this period once they are noticed,
without getting distracted by shiny new toys.

Thanks.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries
  2025-03-10  7:39 ` [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Kristoffer Haugsbakk
  2025-03-10 12:34   ` Karthik Nayak
@ 2025-03-10 15:28   ` Junio C Hamano
  1 sibling, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2025-03-10 15:28 UTC (permalink / raw)
  To: Kristoffer Haugsbakk; +Cc: Karthik Nayak, git

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> I’ve been wanting a command to drop reflogs.  I use `always` and get a
> lot of entries that I don’t care about.  But I don’t want to set it to
> `true` because I care about some of them.

I am not sure what these references to always and true are about,
but nevertheless, instead of having to release entries one by one
with "git reflog delete", a command that lets you discard all the
reflog entries for a given ref would be a great addition to the
toolset.

Thanks for commenting.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] reflog: implement subcommand to drop reflogs
  2025-03-10 12:36 ` [PATCH v2] reflog: implement subcommand to drop reflogs Karthik Nayak
@ 2025-03-12  7:15   ` Patrick Steinhardt
  2025-03-13 14:24     ` Karthik Nayak
  0 siblings, 1 reply; 30+ messages in thread
From: Patrick Steinhardt @ 2025-03-12  7:15 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, kristofferhaugsbakk, gitster

On Mon, Mar 10, 2025 at 01:36:25PM +0100, Karthik Nayak wrote:
> diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
> index a929c52982..6ed98ddaef 100644
> --- a/Documentation/git-reflog.adoc
> +++ b/Documentation/git-reflog.adoc
> @@ -16,6 +16,7 @@ SYNOPSIS
>  	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
>  'git reflog delete' [--rewrite] [--updateref]
>  	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
> +'git reflog drop' [--all | <refs>...]
>  'git reflog exists' <ref>
>  
>  DESCRIPTION
> @@ -48,15 +49,19 @@ and not reachable from the current tip, are removed from the reflog.
>  This is typically not used directly by end users -- instead, see
>  linkgit:git-gc[1].
>  
> -The "delete" subcommand deletes single entries from the reflog. Its
> -argument must be an _exact_ entry (e.g. "`git reflog delete
> -master@{2}`"). This subcommand is also typically not used directly by
> -end users.
> +The "delete" subcommand deletes single entries from the reflog, but
> +not the reflog itself. Its argument must be an _exact_ entry (e.g. "`git
> +reflog delete master@{2}`"). This subcommand is also typically not used
> +directly by end users.
>  
>  The "exists" subcommand checks whether a ref has a reflog.  It exits
>  with zero status if the reflog exists, and non-zero status if it does
>  not.
>  
> +The "drop" subcommand completely removes the reflog for the specified
> +references. This is in contrast to "expire" and "delete", both of which
> +can be used to delete reflog entries, but not the reflog itself.
> +

I guess this paragraph should also moved between "delete" and "exists"
now.

> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 95f264989b..cd93a0bef9 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -449,10 +458,58 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
>  				   refname);
>  }
>  
> +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
> +			   struct repository *repo)
> +{
> +	int ret = 0, do_all = 0;
> +	const struct option options[] = {
> +		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
> +
> +	if (argc && do_all)
> +		die(_("references specified along with --all"));

We should probably use `usage()` instead of `die()` here.

> +	if (do_all) {
> +		struct worktree_reflogs collected = {
> +			.reflogs = STRING_LIST_INIT_DUP,
> +		};
> +		struct string_list_item *item;
> +		struct worktree **worktrees, **p;
> +
> +		worktrees = get_worktrees();
> +		for (p = worktrees; *p; p++) {
> +			collected.worktree = *p;
> +			refs_for_each_reflog(get_worktree_ref_store(*p),
> +					     collect_reflog, &collected);
> +		}
> +		free_worktrees(worktrees);
> +
> +		for_each_string_list_item(item, &collected.reflogs)
> +			ret |= refs_delete_reflog(get_main_ref_store(repo),
> +						     item->string);
> +		string_list_clear(&collected.reflogs, 0);
> +	}

I noticed that `git reflog expire` has the same arguments to specify
which reflogs to expire:

    [--all [--single-worktree] | <refs>...]

The only exception is that they also support `--single-worktree` to only
expire relfogs from the current worktree. Supporting it should probably
not be too much work, so do we want to do so to have feature parity
regarding the reflog selection?

> +	for (int i = 0; i < argc; i++) {
> +		char *ref;
> +		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
> +			ret |= error(_("%s points nowhere!"), argv[i]);

As a user I wouldn't know what this error is trying to tell me. Does the
reflog exist but it's a symreflog that points to another reflog that
does not exist? Do its entries point nowhere?

How about: `error(_("reflog could not be found: '%s'"))` instead? And
seeing that you copied the error message from the "expire" subcommand
we could also adapt it in a preparatory commit.

> +			continue;
> +		}
> +
> +		ret |= refs_delete_reflog(get_main_ref_store(repo), ref);
> +		free(ref);
> +	}

The code is correct, but do we want to maybe wrap this loop in the
`else` branch to guide the reader and make it blindingly obvious that
the loop does nothing `if (do_all)`?

> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index 388fdf9ae5..251caaf9a4 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -551,4 +551,71 @@ test_expect_success 'reflog with invalid object ID can be listed' '
>  	)
>  '
>  
> +test_expect_success 'reflog drop non-existent ref' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_must_fail git reflog exists refs/heads/non-existent &&
> +		test_must_fail git reflog drop refs/heads/non-existent 2>stderr &&
> +		test_grep "error: refs/heads/non-existent points nowhere!" stderr
> +	)
> +'

One edge case that I haven't seen is to try and drop multiple
references, some of which exist and some of which don't. The loops you
have seem to explicitly allow for deletion of only a subset, so it would
be nice to verify that the logic works as expected.

Patrick

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] reflog: drop usage of global variables
  2025-03-10 15:24       ` Junio C Hamano
@ 2025-03-13 13:30         ` Karthik Nayak
  0 siblings, 0 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-13 13:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> but I also suspect that it is mostly due to the original program
>>> structure that uses OPT_SUBCOMMAND() that the subcommands fail to
>>> respond to "-h" unlike the top-level command, so this may not be a
>>> regression.  I do think however that this change is making it harder
>>> to fix.
>>>
>>
>> Hmm. But this is the existing behavior, no?
>
> Didn't I said I also suspect?
>

Yes, I misunderstood!

>> But point taken, I'll drop this patch in the next version! Thanks
>
> Yup.  Take your time, as it is already deep in prerelease feature
> freeze.  I'd prefer to see us leaving spare capacity in our minds to
> fix regressions introduced during this period once they are noticed,
> without getting distracted by shiny new toys.
>
> Thanks.

Fair enough. I only pushed this new topic, because I had mentioned to
you that I would pick it up and I didn't want to forget about it.

Either ways, I will take it slow here! :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] reflog: implement subcommand to drop reflogs
  2025-03-12  7:15   ` Patrick Steinhardt
@ 2025-03-13 14:24     ` Karthik Nayak
  2025-03-13 14:45       ` Patrick Steinhardt
  0 siblings, 1 reply; 30+ messages in thread
From: Karthik Nayak @ 2025-03-13 14:24 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, kristofferhaugsbakk, gitster

[-- Attachment #1: Type: text/plain, Size: 5809 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Mar 10, 2025 at 01:36:25PM +0100, Karthik Nayak wrote:
>> diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
>> index a929c52982..6ed98ddaef 100644
>> --- a/Documentation/git-reflog.adoc
>> +++ b/Documentation/git-reflog.adoc
>> @@ -16,6 +16,7 @@ SYNOPSIS
>>  	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
>>  'git reflog delete' [--rewrite] [--updateref]
>>  	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
>> +'git reflog drop' [--all | <refs>...]
>>  'git reflog exists' <ref>
>>
>>  DESCRIPTION
>> @@ -48,15 +49,19 @@ and not reachable from the current tip, are removed from the reflog.
>>  This is typically not used directly by end users -- instead, see
>>  linkgit:git-gc[1].
>>
>> -The "delete" subcommand deletes single entries from the reflog. Its
>> -argument must be an _exact_ entry (e.g. "`git reflog delete
>> -master@{2}`"). This subcommand is also typically not used directly by
>> -end users.
>> +The "delete" subcommand deletes single entries from the reflog, but
>> +not the reflog itself. Its argument must be an _exact_ entry (e.g. "`git
>> +reflog delete master@{2}`"). This subcommand is also typically not used
>> +directly by end users.
>>
>>  The "exists" subcommand checks whether a ref has a reflog.  It exits
>>  with zero status if the reflog exists, and non-zero status if it does
>>  not.
>>
>> +The "drop" subcommand completely removes the reflog for the specified
>> +references. This is in contrast to "expire" and "delete", both of which
>> +can be used to delete reflog entries, but not the reflog itself.
>> +
>
> I guess this paragraph should also moved between "delete" and "exists"
> now.
>

Yeah, make sense.

>> diff --git a/builtin/reflog.c b/builtin/reflog.c
>> index 95f264989b..cd93a0bef9 100644
>> --- a/builtin/reflog.c
>> +++ b/builtin/reflog.c
>> @@ -449,10 +458,58 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
>>  				   refname);
>>  }
>>
>> +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
>> +			   struct repository *repo)
>> +{
>> +	int ret = 0, do_all = 0;
>> +	const struct option options[] = {
>> +		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
>> +		OPT_END()
>> +	};
>> +
>> +	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
>> +
>> +	if (argc && do_all)
>> +		die(_("references specified along with --all"));
>
> We should probably use `usage()` instead of `die()` here.
>

Good point.

>> +	if (do_all) {
>> +		struct worktree_reflogs collected = {
>> +			.reflogs = STRING_LIST_INIT_DUP,
>> +		};
>> +		struct string_list_item *item;
>> +		struct worktree **worktrees, **p;
>> +
>> +		worktrees = get_worktrees();
>> +		for (p = worktrees; *p; p++) {
>> +			collected.worktree = *p;
>> +			refs_for_each_reflog(get_worktree_ref_store(*p),
>> +					     collect_reflog, &collected);
>> +		}
>> +		free_worktrees(worktrees);
>> +
>> +		for_each_string_list_item(item, &collected.reflogs)
>> +			ret |= refs_delete_reflog(get_main_ref_store(repo),
>> +						     item->string);
>> +		string_list_clear(&collected.reflogs, 0);
>> +	}
>
> I noticed that `git reflog expire` has the same arguments to specify
> which reflogs to expire:
>
>     [--all [--single-worktree] | <refs>...]
>
> The only exception is that they also support `--single-worktree` to only
> expire relfogs from the current worktree. Supporting it should probably
> not be too much work, so do we want to do so to have feature parity
> regarding the reflog selection?
>

I think it would make sense to add support for `--single-worktree`. Let
me add that in, since it mostly a single-line change.

>> +	for (int i = 0; i < argc; i++) {
>> +		char *ref;
>> +		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
>> +			ret |= error(_("%s points nowhere!"), argv[i]);
>
> As a user I wouldn't know what this error is trying to tell me. Does the
> reflog exist but it's a symreflog that points to another reflog that
> does not exist? Do its entries point nowhere?
>
> How about: `error(_("reflog could not be found: '%s'"))` instead? And
> seeing that you copied the error message from the "expire" subcommand
> we could also adapt it in a preparatory commit.
>

Thanks! let change it in both places.

>> +			continue;
>> +		}
>> +
>> +		ret |= refs_delete_reflog(get_main_ref_store(repo), ref);
>> +		free(ref);
>> +	}
>
> The code is correct, but do we want to maybe wrap this loop in the
> `else` branch to guide the reader and make it blindingly obvious that
> the loop does nothing `if (do_all)`?
>

Wouldn't it be simpler to return at the end of the `if (do_all)`? I've
added that, but if feel strongly about this form, happy to change it.

>> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
>> index 388fdf9ae5..251caaf9a4 100755
>> --- a/t/t1410-reflog.sh
>> +++ b/t/t1410-reflog.sh
>> @@ -551,4 +551,71 @@ test_expect_success 'reflog with invalid object ID can be listed' '
>>  	)
>>  '
>>
>> +test_expect_success 'reflog drop non-existent ref' '
>> +	test_when_finished "rm -rf repo" &&
>> +	git init repo &&
>> +	(
>> +		cd repo &&
>> +		test_must_fail git reflog exists refs/heads/non-existent &&
>> +		test_must_fail git reflog drop refs/heads/non-existent 2>stderr &&
>> +		test_grep "error: refs/heads/non-existent points nowhere!" stderr
>> +	)
>> +'
>
> One edge case that I haven't seen is to try and drop multiple
> references, some of which exist and some of which don't. The loops you
> have seem to explicitly allow for deletion of only a subset, so it would
> be nice to verify that the logic works as expected.
>

Good catch, I also noticed that I didn't have a test for worktrees. So
will add that in too.

> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] reflog: implement subcommand to drop reflogs
  2025-03-13 14:24     ` Karthik Nayak
@ 2025-03-13 14:45       ` Patrick Steinhardt
  0 siblings, 0 replies; 30+ messages in thread
From: Patrick Steinhardt @ 2025-03-13 14:45 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, kristofferhaugsbakk, gitster

On Thu, Mar 13, 2025 at 09:24:16AM -0500, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Mon, Mar 10, 2025 at 01:36:25PM +0100, Karthik Nayak wrote:
> >> +			continue;
> >> +		}
> >> +
> >> +		ret |= refs_delete_reflog(get_main_ref_store(repo), ref);
> >> +		free(ref);
> >> +	}
> >
> > The code is correct, but do we want to maybe wrap this loop in the
> > `else` branch to guide the reader and make it blindingly obvious that
> > the loop does nothing `if (do_all)`?
> >
> 
> Wouldn't it be simpler to return at the end of the `if (do_all)`? I've
> added that, but if feel strongly about this form, happy to change it.

I don't feel overly strong about it. I'm not a huge fan of early
returns as I think it's easier to reason about functions if they have a
common exit path. But I can live with whatever solution you come up
with.

Patrick

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 0/2] reflog: implement subcommand to drop reflogs
  2025-03-07 11:17 [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Karthik Nayak
                   ` (3 preceding siblings ...)
  2025-03-10 12:36 ` [PATCH v2] reflog: implement subcommand to drop reflogs Karthik Nayak
@ 2025-03-14  8:40 ` Karthik Nayak
  2025-03-14  8:40   ` [PATCH v3 1/2] reflog: improve error for when reflog is not found Karthik Nayak
  2025-03-14  8:40   ` [PATCH v3 2/2] reflog: implement subcommand to drop reflogs Karthik Nayak
  4 siblings, 2 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-14  8:40 UTC (permalink / raw)
  To: git; +Cc: ps, kristofferhaugsbakk, gitster, Karthik Nayak

While 'git-reflog(1)' currently allows users to expire reflogs and
delete individual entries, it lacks functionality to completely remove
reflogs for specific references. This becomes problematic in
repositories where reflogs are not needed but continue to accumulate
entries despite setting 'core.logAllRefUpdates=false'.

Add a new 'drop' subcommand to git-reflog that allows users to delete
the entire reflog for a specified reference. Include a '--all' flag to
enable dropping all reflogs from all worktrees and an addon flag
'--single-worktree', to drop all reflogs from the current worktree.

The first patch is a small cleanup in 'git refs expire' which improves
the error message used when there is no reflog present for a given
reference.

Changes in v3:
- Add a preparatory commit to fix the error message in 'git reflog
  expire' when a non-existent ref is provided.
- Add support for '--single-worktree' to provide feature parity with
  'git reflog expire'.
- Improved error message and small code fixes.
- Added some additional tests.
- Link to v2: https://lore.kernel.org/r/20250310-493-add-command-to-purge-reflog-entries-v2-1-05caa92e0bfa@gmail.com

Changes in v2:
- Rephrase the commit message to be clearer and fix typo.
- Move the documentation to be next to 'git reflog delete' and also
  add missing documentation for the '--all' flag.
- Ensure '--all' is not used with references and add a test.
- Cleanup variable assignment.
- Check for error message in the test.
- Drop the cleanup commit.
- Rebased on top of master a36e024e98 (Merge branch 'js/win-2.49-build-fixes',
  2025-03-06), this was to include the adoc changes which were breaking
  tests on the CI.
- Link to v1: https://lore.kernel.org/r/20250307-493-add-command-to-purge-reflog-entries-v1-0-84ab8529cf9e@gmail.com

 Documentation/git-reflog.adoc |  23 ++++++--
 builtin/reflog.c              |  68 ++++++++++++++++++++++-
 t/t1410-reflog.sh             | 126 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 209 insertions(+), 8 deletions(-)

Karthik Nayak (2):
      reflog: improve error for when reflog is not found
      reflog: implement subcommand to drop reflogs

Range-diff versus v2:

-:  ---------- > 1:  d4c4bf8c99 reflog: improve error for when reflog is not found
1:  9405eaacb7 ! 2:  a7f88d71da reflog: implement subcommand to drop reflogs
    @@ Commit message
         entries despite setting 'core.logAllRefUpdates=false'.
     
         Add a new 'drop' subcommand to git-reflog that allows users to delete
    -    the entire reflog for a specified reference. Include a '--all' flag to
    -    enable dropping all reflogs in a repository.
    +    the entire reflog for a specified reference. Include an '--all' flag to
    +    enable dropping all reflogs from all worktrees and an addon flag
    +    '--single-worktree', to only drop all reflogs from the current worktree.
     
         While here, remove an extraneous newline in the file.
     
    @@ Documentation/git-reflog.adoc: SYNOPSIS
      	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
      'git reflog delete' [--rewrite] [--updateref]
      	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
    -+'git reflog drop' [--all | <refs>...]
    ++'git reflog drop' [--all [--single-worktree] | <refs>...]
      'git reflog exists' <ref>
      
      DESCRIPTION
    @@ Documentation/git-reflog.adoc: and not reachable from the current tip, are remov
     +not the reflog itself. Its argument must be an _exact_ entry (e.g. "`git
     +reflog delete master@{2}`"). This subcommand is also typically not used
     +directly by end users.
    - 
    - The "exists" subcommand checks whether a ref has a reflog.  It exits
    - with zero status if the reflog exists, and non-zero status if it does
    - not.
    - 
    ++
     +The "drop" subcommand completely removes the reflog for the specified
     +references. This is in contrast to "expire" and "delete", both of which
     +can be used to delete reflog entries, but not the reflog itself.
    -+
    - OPTIONS
    - -------
      
    + The "exists" subcommand checks whether a ref has a reflog.  It exits
    + with zero status if the reflog exists, and non-zero status if it does
     @@ Documentation/git-reflog.adoc: Options for `delete`
      `--dry-run`, and `--verbose`, with the same meanings as when they are
      used with `expire`.
    @@ Documentation/git-reflog.adoc: Options for `delete`
     +
     +--all::
     +	Drop the reflogs of all references from all worktrees.
    ++
    ++--single-worktree::
    ++	By default when `--all` is specified, reflogs from all working
    ++	trees are dropped. This option limits the processing to reflogs
    ++	from the current working tree only.
      
      GIT
      ---
    @@ builtin/reflog.c
      	N_("git reflog exists <ref>")
      
     +#define BUILTIN_REFLOG_DROP_USAGE \
    -+	N_("git reflog drop [--all | <refs>...]")
    ++	N_("git reflog drop [--all [--single-worktree] | <refs>...]")
     +
      static const char *const reflog_show_usage[] = {
      	BUILTIN_REFLOG_SHOW_USAGE,
    @@ builtin/reflog.c: static int cmd_reflog_exists(int argc, const char **argv, cons
     +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
     +			   struct repository *repo)
     +{
    -+	int ret = 0, do_all = 0;
    ++	int ret = 0, do_all = 0, single_worktree = 0;
     +	const struct option options[] = {
    -+		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
    ++		OPT_BOOL(0, "all", &do_all, N_("drop the reflogs of all references")),
    ++		OPT_BOOL(0, "single-worktree", &single_worktree,
    ++			 N_("drop reflogs from the current worktree only")),
     +		OPT_END()
     +	};
     +
     +	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
     +
     +	if (argc && do_all)
    -+		die(_("references specified along with --all"));
    ++		usage(_("references specified along with --all"));
     +
     +	if (do_all) {
     +		struct worktree_reflogs collected = {
    @@ builtin/reflog.c: static int cmd_reflog_exists(int argc, const char **argv, cons
     +
     +		worktrees = get_worktrees();
     +		for (p = worktrees; *p; p++) {
    ++			if (single_worktree && !(*p)->is_current)
    ++				continue;
     +			collected.worktree = *p;
     +			refs_for_each_reflog(get_worktree_ref_store(*p),
     +					     collect_reflog, &collected);
    @@ builtin/reflog.c: static int cmd_reflog_exists(int argc, const char **argv, cons
     +			ret |= refs_delete_reflog(get_main_ref_store(repo),
     +						     item->string);
     +		string_list_clear(&collected.reflogs, 0);
    ++
    ++		return ret;
     +	}
     +
     +	for (int i = 0; i < argc; i++) {
     +		char *ref;
     +		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
    -+			ret |= error(_("%s points nowhere!"), argv[i]);
    ++			ret |= error(_("reflog could not be found: '%s'"), argv[i]);
     +			continue;
     +		}
     +
    @@ t/t1410-reflog.sh: test_expect_success 'reflog with invalid object ID can be lis
     +		cd repo &&
     +		test_must_fail git reflog exists refs/heads/non-existent &&
     +		test_must_fail git reflog drop refs/heads/non-existent 2>stderr &&
    -+		test_grep "error: refs/heads/non-existent points nowhere!" stderr
    ++		test_grep "error: reflog could not be found: ${SQ}refs/heads/non-existent${SQ}" stderr
     +	)
     +'
     +
    @@ t/t1410-reflog.sh: test_expect_success 'reflog with invalid object ID can be lis
     +	)
     +'
     +
    ++test_expect_success 'reflog drop multiple references some non-existent' '
    ++	test_when_finished "rm -rf repo" &&
    ++	git init repo &&
    ++	(
    ++		cd repo &&
    ++		test_commit A &&
    ++		test_commit_bulk --ref=refs/heads/branch 1 &&
    ++		git reflog exists refs/heads/main &&
    ++		git reflog exists refs/heads/branch &&
    ++		test_must_fail git reflog exists refs/heads/non-existent &&
    ++		test_must_fail git reflog drop refs/heads/main refs/heads/non-existent refs/heads/branch 2>stderr &&
    ++		test_must_fail git reflog exists refs/heads/main &&
    ++		test_must_fail git reflog exists refs/heads/branch &&
    ++		test_must_fail git reflog exists refs/heads/non-existent &&
    ++		test_grep "error: reflog could not be found: ${SQ}refs/heads/non-existent${SQ}" stderr
    ++	)
    ++'
    ++
     +test_expect_success 'reflog drop --all' '
     +	test_when_finished "rm -rf repo" &&
     +	git init repo &&
    @@ t/t1410-reflog.sh: test_expect_success 'reflog with invalid object ID can be lis
     +	)
     +'
     +
    ++test_expect_success 'reflog drop --all multiple worktrees' '
    ++	test_when_finished "rm -rf repo" &&
    ++	test_when_finished "rm -rf wt" &&
    ++	git init repo &&
    ++	(
    ++		cd repo &&
    ++		test_commit A &&
    ++		git worktree add ../wt &&
    ++		test_commit_bulk -C ../wt --ref=refs/heads/branch 1 &&
    ++		git reflog exists refs/heads/main &&
    ++		git reflog exists refs/heads/branch &&
    ++		git reflog drop --all &&
    ++		test_must_fail git reflog exists refs/heads/main &&
    ++		test_must_fail git reflog exists refs/heads/branch
    ++	)
    ++'
    ++
    ++test_expect_success 'reflog drop --all --single-worktree' '
    ++	test_when_finished "rm -rf repo" &&
    ++	test_when_finished "rm -rf wt" &&
    ++	git init repo &&
    ++	(
    ++		cd repo &&
    ++		test_commit A &&
    ++		git worktree add ../wt &&
    ++		test_commit -C ../wt foobar &&
    ++		git reflog exists refs/heads/main &&
    ++		git reflog exists refs/heads/wt &&
    ++		test-tool ref-store worktree:wt reflog-exists HEAD &&
    ++		git reflog drop --all --single-worktree &&
    ++		test_must_fail git reflog exists refs/heads/main &&
    ++		test_must_fail git reflog exists refs/heads/wt &&
    ++		test_must_fail test-tool ref-store worktree:main reflog-exists HEAD &&
    ++		test-tool ref-store worktree:wt reflog-exists HEAD
    ++	)
    ++'
    ++
     +test_expect_success 'reflog drop --all with reference' '
     +	test_when_finished "rm -rf repo" &&
     +	git init repo &&
    @@ t/t1410-reflog.sh: test_expect_success 'reflog with invalid object ID can be lis
     +		cd repo &&
     +		test_commit A &&
     +		test_must_fail git reflog drop --all refs/heads/main 2>stderr &&
    -+		test_grep "fatal: references specified along with --all" stderr
    ++		test_grep "usage: references specified along with --all" stderr
     +	)
     +'
     +


base-commit: a36e024e989f4d35f35987a60e3af8022cac3420
change-id: 20250306-493-add-command-to-purge-reflog-entries-bd22547ad34a

Thanks
- Karthik


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 1/2] reflog: improve error for when reflog is not found
  2025-03-14  8:40 ` [PATCH v3 0/2] " Karthik Nayak
@ 2025-03-14  8:40   ` Karthik Nayak
  2025-03-14  8:40   ` [PATCH v3 2/2] reflog: implement subcommand to drop reflogs Karthik Nayak
  1 sibling, 0 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-14  8:40 UTC (permalink / raw)
  To: git; +Cc: ps, kristofferhaugsbakk, gitster, Karthik Nayak

The 'git reflog expire' prints the error message '<ref> points nowhere!'
when used with a non-existent ref. This message is a bit confusing and
vague. Modify the message to be more clear and direct.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/reflog.c  | 2 +-
 t/t1410-reflog.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 95f264989b..762719315e 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -383,7 +383,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 		struct expire_reflog_policy_cb cb = { .cmd = cmd };
 
 		if (!repo_dwim_log(the_repository, argv[i], strlen(argv[i]), NULL, &ref)) {
-			status |= error(_("%s points nowhere!"), argv[i]);
+			status |= error(_("reflog could not be found: '%s'"), argv[i]);
 			continue;
 		}
 		set_reflog_expiry_param(&cb.cmd, ref);
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 388fdf9ae5..1f7249be76 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -315,9 +315,9 @@ test_expect_success 'git reflog expire unknown reference' '
 	test_config gc.reflogexpireunreachable never &&
 
 	test_must_fail git reflog expire main@{123} 2>stderr &&
-	test_grep "points nowhere" stderr &&
+	test_grep "error: reflog could not be found: ${SQ}main@{123}${SQ}" stderr &&
 	test_must_fail git reflog expire does-not-exist 2>stderr &&
-	test_grep "points nowhere" stderr
+	test_grep "error: reflog could not be found: ${SQ}does-not-exist${SQ}" stderr
 '
 
 test_expect_success 'checkout should not delete log for packed ref' '

-- 
2.48.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v3 2/2] reflog: implement subcommand to drop reflogs
  2025-03-14  8:40 ` [PATCH v3 0/2] " Karthik Nayak
  2025-03-14  8:40   ` [PATCH v3 1/2] reflog: improve error for when reflog is not found Karthik Nayak
@ 2025-03-14  8:40   ` Karthik Nayak
  2025-03-18 14:01     ` Christian Couder
  2025-03-18 15:56     ` Toon Claes
  1 sibling, 2 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-14  8:40 UTC (permalink / raw)
  To: git; +Cc: ps, kristofferhaugsbakk, gitster, Karthik Nayak

While 'git-reflog(1)' currently allows users to expire reflogs and
delete individual entries, it lacks functionality to completely remove
reflogs for specific references. This becomes problematic in
repositories where reflogs are not needed but continue to accumulate
entries despite setting 'core.logAllRefUpdates=false'.

Add a new 'drop' subcommand to git-reflog that allows users to delete
the entire reflog for a specified reference. Include an '--all' flag to
enable dropping all reflogs from all worktrees and an addon flag
'--single-worktree', to only drop all reflogs from the current worktree.

While here, remove an extraneous newline in the file.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-reflog.adoc |  23 ++++++--
 builtin/reflog.c              |  66 ++++++++++++++++++++++-
 t/t1410-reflog.sh             | 122 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
index a929c52982..b55c060569 100644
--- a/Documentation/git-reflog.adoc
+++ b/Documentation/git-reflog.adoc
@@ -16,6 +16,7 @@ SYNOPSIS
 	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
 'git reflog delete' [--rewrite] [--updateref]
 	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
+'git reflog drop' [--all [--single-worktree] | <refs>...]
 'git reflog exists' <ref>
 
 DESCRIPTION
@@ -48,10 +49,14 @@ and not reachable from the current tip, are removed from the reflog.
 This is typically not used directly by end users -- instead, see
 linkgit:git-gc[1].
 
-The "delete" subcommand deletes single entries from the reflog. Its
-argument must be an _exact_ entry (e.g. "`git reflog delete
-master@{2}`"). This subcommand is also typically not used directly by
-end users.
+The "delete" subcommand deletes single entries from the reflog, but
+not the reflog itself. Its argument must be an _exact_ entry (e.g. "`git
+reflog delete master@{2}`"). This subcommand is also typically not used
+directly by end users.
+
+The "drop" subcommand completely removes the reflog for the specified
+references. This is in contrast to "expire" and "delete", both of which
+can be used to delete reflog entries, but not the reflog itself.
 
 The "exists" subcommand checks whether a ref has a reflog.  It exits
 with zero status if the reflog exists, and non-zero status if it does
@@ -132,6 +137,16 @@ Options for `delete`
 `--dry-run`, and `--verbose`, with the same meanings as when they are
 used with `expire`.
 
+Options for `drop`
+~~~~~~~~~~~~~~~~~~~~
+
+--all::
+	Drop the reflogs of all references from all worktrees.
+
+--single-worktree::
+	By default when `--all` is specified, reflogs from all working
+	trees are dropped. This option limits the processing to reflogs
+	from the current working tree only.
 
 GIT
 ---
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 762719315e..a3652e69f1 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -29,6 +29,9 @@
 #define BUILTIN_REFLOG_EXISTS_USAGE \
 	N_("git reflog exists <ref>")
 
+#define BUILTIN_REFLOG_DROP_USAGE \
+	N_("git reflog drop [--all [--single-worktree] | <refs>...]")
+
 static const char *const reflog_show_usage[] = {
 	BUILTIN_REFLOG_SHOW_USAGE,
 	NULL,
@@ -54,11 +57,17 @@ static const char *const reflog_exists_usage[] = {
 	NULL,
 };
 
+static const char *const reflog_drop_usage[] = {
+	BUILTIN_REFLOG_DROP_USAGE,
+	NULL,
+};
+
 static const char *const reflog_usage[] = {
 	BUILTIN_REFLOG_SHOW_USAGE,
 	BUILTIN_REFLOG_LIST_USAGE,
 	BUILTIN_REFLOG_EXPIRE_USAGE,
 	BUILTIN_REFLOG_DELETE_USAGE,
+	BUILTIN_REFLOG_DROP_USAGE,
 	BUILTIN_REFLOG_EXISTS_USAGE,
 	NULL
 };
@@ -449,10 +458,64 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
 				   refname);
 }
 
+static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
+			   struct repository *repo)
+{
+	int ret = 0, do_all = 0, single_worktree = 0;
+	const struct option options[] = {
+		OPT_BOOL(0, "all", &do_all, N_("drop the reflogs of all references")),
+		OPT_BOOL(0, "single-worktree", &single_worktree,
+			 N_("drop reflogs from the current worktree only")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
+
+	if (argc && do_all)
+		usage(_("references specified along with --all"));
+
+	if (do_all) {
+		struct worktree_reflogs collected = {
+			.reflogs = STRING_LIST_INIT_DUP,
+		};
+		struct string_list_item *item;
+		struct worktree **worktrees, **p;
+
+		worktrees = get_worktrees();
+		for (p = worktrees; *p; p++) {
+			if (single_worktree && !(*p)->is_current)
+				continue;
+			collected.worktree = *p;
+			refs_for_each_reflog(get_worktree_ref_store(*p),
+					     collect_reflog, &collected);
+		}
+		free_worktrees(worktrees);
+
+		for_each_string_list_item(item, &collected.reflogs)
+			ret |= refs_delete_reflog(get_main_ref_store(repo),
+						     item->string);
+		string_list_clear(&collected.reflogs, 0);
+
+		return ret;
+	}
+
+	for (int i = 0; i < argc; i++) {
+		char *ref;
+		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
+			ret |= error(_("reflog could not be found: '%s'"), argv[i]);
+			continue;
+		}
+
+		ret |= refs_delete_reflog(get_main_ref_store(repo), ref);
+		free(ref);
+	}
+
+	return ret;
+}
+
 /*
  * main "reflog"
  */
-
 int cmd_reflog(int argc,
 	       const char **argv,
 	       const char *prefix,
@@ -465,6 +528,7 @@ int cmd_reflog(int argc,
 		OPT_SUBCOMMAND("expire", &fn, cmd_reflog_expire),
 		OPT_SUBCOMMAND("delete", &fn, cmd_reflog_delete),
 		OPT_SUBCOMMAND("exists", &fn, cmd_reflog_exists),
+		OPT_SUBCOMMAND("drop", &fn, cmd_reflog_drop),
 		OPT_END()
 	};
 
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 1f7249be76..42b501f163 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -551,4 +551,126 @@ test_expect_success 'reflog with invalid object ID can be listed' '
 	)
 '
 
+test_expect_success 'reflog drop non-existent ref' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_must_fail git reflog exists refs/heads/non-existent &&
+		test_must_fail git reflog drop refs/heads/non-existent 2>stderr &&
+		test_grep "error: reflog could not be found: ${SQ}refs/heads/non-existent${SQ}" stderr
+	)
+'
+
+test_expect_success 'reflog drop' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch
+	)
+'
+
+test_expect_success 'reflog drop multiple references' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop refs/heads/main refs/heads/branch &&
+		test_must_fail git reflog exists refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/branch
+	)
+'
+
+test_expect_success 'reflog drop multiple references some non-existent' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		test_must_fail git reflog exists refs/heads/non-existent &&
+		test_must_fail git reflog drop refs/heads/main refs/heads/non-existent refs/heads/branch 2>stderr &&
+		test_must_fail git reflog exists refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/branch &&
+		test_must_fail git reflog exists refs/heads/non-existent &&
+		test_grep "error: reflog could not be found: ${SQ}refs/heads/non-existent${SQ}" stderr
+	)
+'
+
+test_expect_success 'reflog drop --all' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit_bulk --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop --all &&
+		test_must_fail git reflog exists refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/branch
+	)
+'
+
+test_expect_success 'reflog drop --all multiple worktrees' '
+	test_when_finished "rm -rf repo" &&
+	test_when_finished "rm -rf wt" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		git worktree add ../wt &&
+		test_commit_bulk -C ../wt --ref=refs/heads/branch 1 &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/branch &&
+		git reflog drop --all &&
+		test_must_fail git reflog exists refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/branch
+	)
+'
+
+test_expect_success 'reflog drop --all --single-worktree' '
+	test_when_finished "rm -rf repo" &&
+	test_when_finished "rm -rf wt" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		git worktree add ../wt &&
+		test_commit -C ../wt foobar &&
+		git reflog exists refs/heads/main &&
+		git reflog exists refs/heads/wt &&
+		test-tool ref-store worktree:wt reflog-exists HEAD &&
+		git reflog drop --all --single-worktree &&
+		test_must_fail git reflog exists refs/heads/main &&
+		test_must_fail git reflog exists refs/heads/wt &&
+		test_must_fail test-tool ref-store worktree:main reflog-exists HEAD &&
+		test-tool ref-store worktree:wt reflog-exists HEAD
+	)
+'
+
+test_expect_success 'reflog drop --all with reference' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_must_fail git reflog drop --all refs/heads/main 2>stderr &&
+		test_grep "usage: references specified along with --all" stderr
+	)
+'
+
 test_done

-- 
2.48.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 2/2] reflog: implement subcommand to drop reflogs
  2025-03-14  8:40   ` [PATCH v3 2/2] reflog: implement subcommand to drop reflogs Karthik Nayak
@ 2025-03-18 14:01     ` Christian Couder
  2025-03-18 17:44       ` Junio C Hamano
  2025-03-18 15:56     ` Toon Claes
  1 sibling, 1 reply; 30+ messages in thread
From: Christian Couder @ 2025-03-18 14:01 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, ps, kristofferhaugsbakk, gitster

On Fri, Mar 14, 2025 at 9:41 AM Karthik Nayak <karthik.188@gmail.com> wrote:

> +Options for `drop`
> +~~~~~~~~~~~~~~~~~~~~
> +
> +--all::
> +       Drop the reflogs of all references from all worktrees.
> +
> +--single-worktree::
> +       By default when `--all` is specified, reflogs from all working
> +       trees are dropped. This option limits the processing to reflogs
> +       from the current working tree only.

It seems to me that "--current-worktree" would have been clearer than
"--single-worktree", but I understand that it would have been
confusing to have a different name for basically the same option in
`git reflog expire` and `git reflog drop`.

> +       argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
> +
> +       if (argc && do_all)
> +               usage(_("references specified along with --all"));
> +
> +       if (do_all) {
> +               struct worktree_reflogs collected = {
> +                       .reflogs = STRING_LIST_INIT_DUP,
> +               };
> +               struct string_list_item *item;
> +               struct worktree **worktrees, **p;
> +
> +               worktrees = get_worktrees();
> +               for (p = worktrees; *p; p++) {
> +                       if (single_worktree && !(*p)->is_current)

It looks like 'single_worktree' is only used here. This means that if
a user forgets to add --all and only uses --single-worktree, nothing
will happen and it seems to me that the command will exit with code 0.
Even if `git reflog expire` already works like that, I think this is a
bit unfortunate.

Otherwise this patch series looks very well done to me.

Thanks!

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 2/2] reflog: implement subcommand to drop reflogs
  2025-03-14  8:40   ` [PATCH v3 2/2] reflog: implement subcommand to drop reflogs Karthik Nayak
  2025-03-18 14:01     ` Christian Couder
@ 2025-03-18 15:56     ` Toon Claes
  2025-03-19  9:16       ` Karthik Nayak
  1 sibling, 1 reply; 30+ messages in thread
From: Toon Claes @ 2025-03-18 15:56 UTC (permalink / raw)
  To: Karthik Nayak, git; +Cc: ps, kristofferhaugsbakk, gitster, Karthik Nayak

Karthik Nayak <karthik.188@gmail.com> writes:

> While 'git-reflog(1)' currently allows users to expire reflogs and
> delete individual entries, it lacks functionality to completely remove
> reflogs for specific references. This becomes problematic in
> repositories where reflogs are not needed but continue to accumulate
> entries despite setting 'core.logAllRefUpdates=false'.
>
> Add a new 'drop' subcommand to git-reflog that allows users to delete
> the entire reflog for a specified reference. Include an '--all' flag to
> enable dropping all reflogs from all worktrees and an addon flag
> '--single-worktree', to only drop all reflogs from the current worktree.
>
> While here, remove an extraneous newline in the file.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-reflog.adoc |  23 ++++++--
>  builtin/reflog.c              |  66 ++++++++++++++++++++++-
>  t/t1410-reflog.sh             | 122 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 206 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
> index a929c52982..b55c060569 100644
> --- a/Documentation/git-reflog.adoc
> +++ b/Documentation/git-reflog.adoc
> @@ -16,6 +16,7 @@ SYNOPSIS
>  	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
>  'git reflog delete' [--rewrite] [--updateref]
>  	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
> +'git reflog drop' [--all [--single-worktree] | <refs>...]
>  'git reflog exists' <ref>
>  
>  DESCRIPTION
> @@ -48,10 +49,14 @@ and not reachable from the current tip, are removed from the reflog.
>  This is typically not used directly by end users -- instead, see
>  linkgit:git-gc[1].
>  
> -The "delete" subcommand deletes single entries from the reflog. Its
> -argument must be an _exact_ entry (e.g. "`git reflog delete
> -master@{2}`"). This subcommand is also typically not used directly by
> -end users.
> +The "delete" subcommand deletes single entries from the reflog, but
> +not the reflog itself. Its argument must be an _exact_ entry (e.g. "`git
> +reflog delete master@{2}`"). This subcommand is also typically not used
> +directly by end users.
> +
> +The "drop" subcommand completely removes the reflog for the specified
> +references. This is in contrast to "expire" and "delete", both of which
> +can be used to delete reflog entries, but not the reflog itself.
>  
>  The "exists" subcommand checks whether a ref has a reflog.  It exits
>  with zero status if the reflog exists, and non-zero status if it does
> @@ -132,6 +137,16 @@ Options for `delete`
>  `--dry-run`, and `--verbose`, with the same meanings as when they are
>  used with `expire`.
>  
> +Options for `drop`
> +~~~~~~~~~~~~~~~~~~~~
> +
> +--all::
> +	Drop the reflogs of all references from all worktrees.
> +
> +--single-worktree::
> +	By default when `--all` is specified, reflogs from all working
> +	trees are dropped. This option limits the processing to reflogs
> +	from the current working tree only.
>  
>  GIT
>  ---
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 762719315e..a3652e69f1 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -29,6 +29,9 @@
>  #define BUILTIN_REFLOG_EXISTS_USAGE \
>  	N_("git reflog exists <ref>")
>  
> +#define BUILTIN_REFLOG_DROP_USAGE \
> +	N_("git reflog drop [--all [--single-worktree] | <refs>...]")
> +
>  static const char *const reflog_show_usage[] = {
>  	BUILTIN_REFLOG_SHOW_USAGE,
>  	NULL,
> @@ -54,11 +57,17 @@ static const char *const reflog_exists_usage[] = {
>  	NULL,
>  };
>  
> +static const char *const reflog_drop_usage[] = {
> +	BUILTIN_REFLOG_DROP_USAGE,
> +	NULL,
> +};
> +
>  static const char *const reflog_usage[] = {
>  	BUILTIN_REFLOG_SHOW_USAGE,
>  	BUILTIN_REFLOG_LIST_USAGE,
>  	BUILTIN_REFLOG_EXPIRE_USAGE,
>  	BUILTIN_REFLOG_DELETE_USAGE,
> +	BUILTIN_REFLOG_DROP_USAGE,
>  	BUILTIN_REFLOG_EXISTS_USAGE,
>  	NULL
>  };
> @@ -449,10 +458,64 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
>  				   refname);
>  }
>  
> +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
> +			   struct repository *repo)
> +{
> +	int ret = 0, do_all = 0, single_worktree = 0;
> +	const struct option options[] = {
> +		OPT_BOOL(0, "all", &do_all, N_("drop the reflogs of all references")),
> +		OPT_BOOL(0, "single-worktree", &single_worktree,
> +			 N_("drop reflogs from the current worktree only")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
> +
> +	if (argc && do_all)
> +		usage(_("references specified along with --all"));

What is the intended behavior when both `--all` and `<refs>` are
omitted? It seems nothing happens at the moment. And no error nor
warning is printed, that feels a bit odd to me.

Now, when you do `git reflog expire --expire=all` it also seems to be
doing nothing at all. I also think this is weird. And I don't see any
test coverage for `git reflog expire` without `--all`.

But what is the expected behavior when you omit `--all` and `<refs>`?
Should it give an error or warning? Should it use HEAD, just like `git
reflog show` does?

> +
> +	if (do_all) {
> +		struct worktree_reflogs collected = {
> +			.reflogs = STRING_LIST_INIT_DUP,
> +		};
> +		struct string_list_item *item;
> +		struct worktree **worktrees, **p;
> +
> +		worktrees = get_worktrees();
> +		for (p = worktrees; *p; p++) {
> +			if (single_worktree && !(*p)->is_current)
> +				continue;
> +			collected.worktree = *p;
> +			refs_for_each_reflog(get_worktree_ref_store(*p),
> +					     collect_reflog, &collected);
> +		}
> +		free_worktrees(worktrees);
> +
> +		for_each_string_list_item(item, &collected.reflogs)
> +			ret |= refs_delete_reflog(get_main_ref_store(repo),
> +						     item->string);
> +		string_list_clear(&collected.reflogs, 0);
> +
> +		return ret;
> +	}
> +
> +	for (int i = 0; i < argc; i++) {
> +		char *ref;
> +		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
> +			ret |= error(_("reflog could not be found: '%s'"), argv[i]);
> +			continue;
> +		}
> +
> +		ret |= refs_delete_reflog(get_main_ref_store(repo), ref);
> +		free(ref);
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * main "reflog"
>   */
> -
>  int cmd_reflog(int argc,
>  	       const char **argv,
>  	       const char *prefix,
> @@ -465,6 +528,7 @@ int cmd_reflog(int argc,
>  		OPT_SUBCOMMAND("expire", &fn, cmd_reflog_expire),
>  		OPT_SUBCOMMAND("delete", &fn, cmd_reflog_delete),
>  		OPT_SUBCOMMAND("exists", &fn, cmd_reflog_exists),
> +		OPT_SUBCOMMAND("drop", &fn, cmd_reflog_drop),
>  		OPT_END()
>  	};
>  
> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index 1f7249be76..42b501f163 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -551,4 +551,126 @@ test_expect_success 'reflog with invalid object ID can be listed' '
>  	)
>  '
>  
> +test_expect_success 'reflog drop non-existent ref' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_must_fail git reflog exists refs/heads/non-existent &&
> +		test_must_fail git reflog drop refs/heads/non-existent 2>stderr &&
> +		test_grep "error: reflog could not be found: ${SQ}refs/heads/non-existent${SQ}" stderr
> +	)
> +'
> +
> +test_expect_success 'reflog drop' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit A &&
> +		test_commit_bulk --ref=refs/heads/branch 1 &&
> +		git reflog exists refs/heads/main &&
> +		git reflog exists refs/heads/branch &&
> +		git reflog drop refs/heads/main &&
> +		test_must_fail git reflog exists refs/heads/main &&
> +		git reflog exists refs/heads/branch
> +	)
> +'
> +
> +test_expect_success 'reflog drop multiple references' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit A &&
> +		test_commit_bulk --ref=refs/heads/branch 1 &&
> +		git reflog exists refs/heads/main &&
> +		git reflog exists refs/heads/branch &&
> +		git reflog drop refs/heads/main refs/heads/branch &&
> +		test_must_fail git reflog exists refs/heads/main &&
> +		test_must_fail git reflog exists refs/heads/branch
> +	)
> +'
> +
> +test_expect_success 'reflog drop multiple references some non-existent' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit A &&
> +		test_commit_bulk --ref=refs/heads/branch 1 &&
> +		git reflog exists refs/heads/main &&
> +		git reflog exists refs/heads/branch &&
> +		test_must_fail git reflog exists refs/heads/non-existent &&
> +		test_must_fail git reflog drop refs/heads/main refs/heads/non-existent refs/heads/branch 2>stderr &&
> +		test_must_fail git reflog exists refs/heads/main &&
> +		test_must_fail git reflog exists refs/heads/branch &&
> +		test_must_fail git reflog exists refs/heads/non-existent &&
> +		test_grep "error: reflog could not be found: ${SQ}refs/heads/non-existent${SQ}" stderr
> +	)
> +'
> +
> +test_expect_success 'reflog drop --all' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit A &&
> +		test_commit_bulk --ref=refs/heads/branch 1 &&
> +		git reflog exists refs/heads/main &&
> +		git reflog exists refs/heads/branch &&
> +		git reflog drop --all &&
> +		test_must_fail git reflog exists refs/heads/main &&
> +		test_must_fail git reflog exists refs/heads/branch

Should we test output of `git reflog list`?

> +	)
> +'
> +
> +test_expect_success 'reflog drop --all multiple worktrees' '
> +	test_when_finished "rm -rf repo" &&
> +	test_when_finished "rm -rf wt" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit A &&
> +		git worktree add ../wt &&
> +		test_commit_bulk -C ../wt --ref=refs/heads/branch 1 &&
> +		git reflog exists refs/heads/main &&
> +		git reflog exists refs/heads/branch &&
> +		git reflog drop --all &&
> +		test_must_fail git reflog exists refs/heads/main &&
> +		test_must_fail git reflog exists refs/heads/branch

Shall we test HEAD in both worktrees does not exists?

> +	)
> +'
> +
> +test_expect_success 'reflog drop --all --single-worktree' '
> +	test_when_finished "rm -rf repo" &&
> +	test_when_finished "rm -rf wt" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit A &&
> +		git worktree add ../wt &&
> +		test_commit -C ../wt foobar &&
> +		git reflog exists refs/heads/main &&
> +		git reflog exists refs/heads/wt &&
> +		test-tool ref-store worktree:wt reflog-exists HEAD &&
> +		git reflog drop --all --single-worktree &&
> +		test_must_fail git reflog exists refs/heads/main &&
> +		test_must_fail git reflog exists refs/heads/wt &&
> +		test_must_fail test-tool ref-store worktree:main reflog-exists HEAD &&
> +		test-tool ref-store worktree:wt reflog-exists HEAD

Naive question: why is `test-tool ref-store` used and not
`git -C ../wt reflog exist`?

> +	)
> +'
> +
> +test_expect_success 'reflog drop --all with reference' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit A &&
> +		test_must_fail git reflog drop --all refs/heads/main 2>stderr &&
> +		test_grep "usage: references specified along with --all" stderr
> +	)
> +'
> +
>  test_done
>
> -- 
> 2.48.1

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 2/2] reflog: implement subcommand to drop reflogs
  2025-03-18 14:01     ` Christian Couder
@ 2025-03-18 17:44       ` Junio C Hamano
  2025-03-19  8:17         ` Christian Couder
  0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-03-18 17:44 UTC (permalink / raw)
  To: Christian Couder; +Cc: Karthik Nayak, git, ps, kristofferhaugsbakk

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

> It looks like 'single_worktree' is only used here. This means that if
> a user forgets to add --all and only uses --single-worktree, nothing
> will happen and it seems to me that the command will exit with code 0.
> Even if `git reflog expire` already works like that, I think this is a
> bit unfortunate.
>
> Otherwise this patch series looks very well done to me.

In the thread Toon too seems to have noticed the same "what if there
is no --all and --single-worktree is given?" gotcha.  Together with
the "current would be better name than single", we can consider that
these funnies are to be "consistent" with the "expire" thing, and I
am OK to see us move on.  An alternative is to "fix" the behaviour
and naming of the "expire" first, and then use the same improved
behaviour and naming when adding "drop" as a new thing.

Thanks.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 2/2] reflog: implement subcommand to drop reflogs
  2025-03-18 17:44       ` Junio C Hamano
@ 2025-03-19  8:17         ` Christian Couder
  2025-03-19  9:06           ` Karthik Nayak
  0 siblings, 1 reply; 30+ messages in thread
From: Christian Couder @ 2025-03-19  8:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Karthik Nayak, git, ps, kristofferhaugsbakk

On Tue, Mar 18, 2025 at 6:44 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > It looks like 'single_worktree' is only used here. This means that if
> > a user forgets to add --all and only uses --single-worktree, nothing
> > will happen and it seems to me that the command will exit with code 0.
> > Even if `git reflog expire` already works like that, I think this is a
> > bit unfortunate.
> >
> > Otherwise this patch series looks very well done to me.
>
> In the thread Toon too seems to have noticed the same "what if there
> is no --all and --single-worktree is given?" gotcha.  Together with
> the "current would be better name than single", we can consider that
> these funnies are to be "consistent" with the "expire" thing, and I
> am OK to see us move on.

I am OK with moving on too. We can "fix" the behavior and naming later
in a dedicated separate patch series.

> An alternative is to "fix" the behaviour
> and naming of the "expire" first, and then use the same improved
> behaviour and naming when adding "drop" as a new thing.

I would be OK with that too.

Thanks.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 2/2] reflog: implement subcommand to drop reflogs
  2025-03-19  8:17         ` Christian Couder
@ 2025-03-19  9:06           ` Karthik Nayak
  0 siblings, 0 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-19  9:06 UTC (permalink / raw)
  To: Christian Couder, Junio C Hamano; +Cc: git, ps, kristofferhaugsbakk

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

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

> On Tue, Mar 18, 2025 at 6:44 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>> > It looks like 'single_worktree' is only used here. This means that if
>> > a user forgets to add --all and only uses --single-worktree, nothing
>> > will happen and it seems to me that the command will exit with code 0.
>> > Even if `git reflog expire` already works like that, I think this is a
>> > bit unfortunate.
>> >
>> > Otherwise this patch series looks very well done to me.
>>
>> In the thread Toon too seems to have noticed the same "what if there
>> is no --all and --single-worktree is given?" gotcha.  Together with
>> the "current would be better name than single", we can consider that
>> these funnies are to be "consistent" with the "expire" thing, and I
>> am OK to see us move on.
>
> I am OK with moving on too. We can "fix" the behavior and naming later
> in a dedicated separate patch series.
>

Seems good, let's do that then. I'll see if I can follow up but this
could also be #leftoverbits if someone wants to pick it up!

>> An alternative is to "fix" the behaviour
>> and naming of the "expire" first, and then use the same improved
>> behaviour and naming when adding "drop" as a new thing.
>
> I would be OK with that too.
>
> Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 2/2] reflog: implement subcommand to drop reflogs
  2025-03-18 15:56     ` Toon Claes
@ 2025-03-19  9:16       ` Karthik Nayak
  0 siblings, 0 replies; 30+ messages in thread
From: Karthik Nayak @ 2025-03-19  9:16 UTC (permalink / raw)
  To: Toon Claes, git; +Cc: ps, kristofferhaugsbakk, gitster

[-- Attachment #1: Type: text/plain, Size: 4001 bytes --]

Toon Claes <toon@iotcl.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>

[snip]

>> +static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
>> +			   struct repository *repo)
>> +{
>> +	int ret = 0, do_all = 0, single_worktree = 0;
>> +	const struct option options[] = {
>> +		OPT_BOOL(0, "all", &do_all, N_("drop the reflogs of all references")),
>> +		OPT_BOOL(0, "single-worktree", &single_worktree,
>> +			 N_("drop reflogs from the current worktree only")),
>> +		OPT_END()
>> +	};
>> +
>> +	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
>> +
>> +	if (argc && do_all)
>> +		usage(_("references specified along with --all"));
>
> What is the intended behavior when both `--all` and `<refs>` are
> omitted? It seems nothing happens at the moment. And no error nor
> warning is printed, that feels a bit odd to me.
>
> Now, when you do `git reflog expire --expire=all` it also seems to be
> doing nothing at all. I also think this is weird. And I don't see any
> test coverage for `git reflog expire` without `--all`.
>
> But what is the expected behavior when you omit `--all` and `<refs>`?
> Should it give an error or warning? Should it use HEAD, just like `git
> reflog show` does?
>

As discussed in the other thread [1], ideally this should be raised as
an error. I'm leaving it for now.

[snip]

>> +
>> +test_expect_success 'reflog drop --all' '
>> +	test_when_finished "rm -rf repo" &&
>> +	git init repo &&
>> +	(
>> +		cd repo &&
>> +		test_commit A &&
>> +		test_commit_bulk --ref=refs/heads/branch 1 &&
>> +		git reflog exists refs/heads/main &&
>> +		git reflog exists refs/heads/branch &&
>> +		git reflog drop --all &&
>> +		test_must_fail git reflog exists refs/heads/main &&
>> +		test_must_fail git reflog exists refs/heads/branch
>
> Should we test output of `git reflog list`?
>

I don't see why, we're concerned with individual reflogs and 'exists'
help check against those individual reflogs.

>> +	)
>> +'
>> +
>> +test_expect_success 'reflog drop --all multiple worktrees' '
>> +	test_when_finished "rm -rf repo" &&
>> +	test_when_finished "rm -rf wt" &&
>> +	git init repo &&
>> +	(
>> +		cd repo &&
>> +		test_commit A &&
>> +		git worktree add ../wt &&
>> +		test_commit_bulk -C ../wt --ref=refs/heads/branch 1 &&
>> +		git reflog exists refs/heads/main &&
>> +		git reflog exists refs/heads/branch &&
>> +		git reflog drop --all &&
>> +		test_must_fail git reflog exists refs/heads/main &&
>> +		test_must_fail git reflog exists refs/heads/branch
>
> Shall we test HEAD in both worktrees does not exists?
>

I think it would be a good addition, but I'm not sure if its worthy of a
re-roll.

>> +	)
>> +'
>> +
>> +test_expect_success 'reflog drop --all --single-worktree' '
>> +	test_when_finished "rm -rf repo" &&
>> +	test_when_finished "rm -rf wt" &&
>> +	git init repo &&
>> +	(
>> +		cd repo &&
>> +		test_commit A &&
>> +		git worktree add ../wt &&
>> +		test_commit -C ../wt foobar &&
>> +		git reflog exists refs/heads/main &&
>> +		git reflog exists refs/heads/wt &&
>> +		test-tool ref-store worktree:wt reflog-exists HEAD &&
>> +		git reflog drop --all --single-worktree &&
>> +		test_must_fail git reflog exists refs/heads/main &&
>> +		test_must_fail git reflog exists refs/heads/wt &&
>> +		test_must_fail test-tool ref-store worktree:main reflog-exists HEAD &&
>> +		test-tool ref-store worktree:wt reflog-exists HEAD
>
> Naive question: why is `test-tool ref-store` used and not
> `git -C ../wt reflog exist`?
>

That should work too :)

>> +	)
>> +'
>> +
>> +test_expect_success 'reflog drop --all with reference' '
>> +	test_when_finished "rm -rf repo" &&
>> +	git init repo &&
>> +	(
>> +		cd repo &&
>> +		test_commit A &&
>> +		test_must_fail git reflog drop --all refs/heads/main 2>stderr &&
>> +		test_grep "usage: references specified along with --all" stderr
>> +	)
>> +'
>> +
>>  test_done
>>
>> --
>> 2.48.1

[1]: CAOLa=ZSj11TSTs6CywSX1Q9AAfW28zssS2yrGf8PmBOgd06Etg@mail.gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2025-03-19  9:16 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-07 11:17 [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Karthik Nayak
2025-03-07 11:17 ` [PATCH 1/2] reflog: drop usage of global variables Karthik Nayak
2025-03-07 21:19   ` Junio C Hamano
2025-03-10 11:41     ` Karthik Nayak
2025-03-10 15:24       ` Junio C Hamano
2025-03-13 13:30         ` Karthik Nayak
2025-03-07 11:17 ` [PATCH 2/2] reflog: implement subcommand to drop reflogs Karthik Nayak
2025-03-07 11:50   ` Patrick Steinhardt
2025-03-07 12:53     ` Karthik Nayak
2025-03-07 12:59       ` Patrick Steinhardt
2025-03-07 13:28         ` Karthik Nayak
2025-03-07 13:28         ` Karthik Nayak
2025-03-07 21:28     ` Junio C Hamano
2025-03-10 11:28       ` Karthik Nayak
2025-03-10  7:39 ` [PATCH 0/2] EDITME: cover title for 493-add-command-to-purge-reflog-entries Kristoffer Haugsbakk
2025-03-10 12:34   ` Karthik Nayak
2025-03-10 15:28   ` Junio C Hamano
2025-03-10 12:36 ` [PATCH v2] reflog: implement subcommand to drop reflogs Karthik Nayak
2025-03-12  7:15   ` Patrick Steinhardt
2025-03-13 14:24     ` Karthik Nayak
2025-03-13 14:45       ` Patrick Steinhardt
2025-03-14  8:40 ` [PATCH v3 0/2] " Karthik Nayak
2025-03-14  8:40   ` [PATCH v3 1/2] reflog: improve error for when reflog is not found Karthik Nayak
2025-03-14  8:40   ` [PATCH v3 2/2] reflog: implement subcommand to drop reflogs Karthik Nayak
2025-03-18 14:01     ` Christian Couder
2025-03-18 17:44       ` Junio C Hamano
2025-03-19  8:17         ` Christian Couder
2025-03-19  9:06           ` Karthik Nayak
2025-03-18 15:56     ` Toon Claes
2025-03-19  9:16       ` Karthik Nayak

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 NNTP newsgroup(s).