git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] logging branch deletion to help recovering from mistakes
@ 2010-12-06 21:16 Junio C Hamano
  2010-12-06 21:55 ` Štěpán Němec
                   ` (3 more replies)
  0 siblings, 4 replies; 28+ messages in thread
From: Junio C Hamano @ 2010-12-06 21:16 UTC (permalink / raw)
  To: git

This adds core.logrefdeletion configuration variable (enabled by default
in a repository with a work tree, just like core.logallrefupdates), and
logs deletion of refs via "update-ref -d", "branch -d", etc.

"git branch" learns a new "--list-deleted" option to help users view the
names of branches and the commit objects that were at the tip of them when
the branches were deleted.

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

 * This recently came up at $dayjob.  The new option is not '--undelete'
   and this is deliberate, as we do not have any information other than
   the tip of the branch to recreate tracking and other frills.

 Documentation/config.txt             |    6 +
 Documentation/git-branch.txt         |    6 +
 builtin/branch.c                     |   21 ++++-
 cache.h                              |    1 +
 config.c                             |    5 +
 environment.c                        |    1 +
 refs.c                               |  167 +++++++++++++++++++++++++---------
 refs.h                               |    2 +
 t/t1400-update-ref.sh                |   53 ++++++++++-
 t/t7701-repack-unpack-unreachable.sh |    4 +
 10 files changed, 219 insertions(+), 47 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d82c0da..bdf90eb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -351,6 +351,12 @@ This value is true by default in a repository that has
 a working directory associated with it, and false by
 default in a bare repository.
 
+core.logRefDeletion::
+	Enable logging of eletion of refs (e.g. branches), allowing `git
+	branch --list-deleted` to help you recover branches lost by
+	running `git branch -d` by mistake.  This is enabled in a
+	repository that has a working tree associated with it by default.
+
 core.repositoryFormatVersion::
 	Internal variable identifying the repository format and layout
 	version.
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 1940256..07ec47b 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git branch' (-m | -M) [<oldbranch>] <newbranch>
 'git branch' (-d | -D) [-r] <branchname>...
+'git branch' --list-deleted [<pattern>...]
 
 DESCRIPTION
 -----------
@@ -70,6 +71,11 @@ OPTIONS
 -D::
 	Delete a branch irrespective of its merged status.
 
+--list-deleted::
+	List names of recently deleted branches together with the object
+	names of the commits that were at the tip of them.  Glob patterns
+	can be used to limit the branches that are shown.
+
 -l::
 	Create the branch's reflog.  This activates recording of
 	all changes made to the branch ref, enabling use of date
diff --git a/builtin/branch.c b/builtin/branch.c
index 87976f0..68604c2 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -15,6 +15,7 @@
 #include "branch.h"
 #include "diff.h"
 #include "revision.h"
+#include "string-list.h"
 
 static const char * const builtin_branch_usage[] = {
 	"git branch [options] [-r | -a] [--merged | --no-merged]",
@@ -144,6 +145,19 @@ static int branch_merged(int kind, const char *name,
 	return merged;
 }
 
+static int show_deleted(struct string_list_item *item, void *cb_data)
+{
+	printf("%s %s\n", (char *)item->util, item->string);
+	return 0;
+}
+
+static int list_deleted_branches(const char **argv)
+{
+	struct string_list *deleted = list_deleted_refs("refs/heads/", argv);
+	for_each_string_list(deleted, show_deleted, NULL);
+	return 0;
+}
+
 static int delete_branches(int argc, const char **argv, int force, int kinds)
 {
 	struct commit *rev, *head_rev = NULL;
@@ -612,7 +626,7 @@ static int opt_parse_merge_filter(const struct option *opt, const char *arg, int
 
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
-	int delete = 0, rename = 0, force_create = 0;
+	int delete = 0, rename = 0, force_create = 0, list_deleted = 0;
 	int verbose = 0, abbrev = DEFAULT_ABBREV, detached = 0;
 	int reflog = 0;
 	enum branch_track track;
@@ -652,6 +666,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BIT('M', NULL, &rename, "move/rename a branch, even if target exists", 2),
 		OPT_BOOLEAN('l', NULL, &reflog, "create the branch's reflog"),
 		OPT_BOOLEAN('f', "force", &force_create, "force creation (when already exists)"),
+		OPT_BOOLEAN(0, "list-deleted", &list_deleted, "list deleted branches"),
 		{
 			OPTION_CALLBACK, 0, "no-merged", &merge_filter_ref,
 			"commit", "print only not merged branches",
@@ -689,11 +704,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
-	if (!!delete + !!rename + !!force_create > 1)
+	if (!!list_deleted + !!delete + !!rename + !!force_create > 1)
 		usage_with_options(builtin_branch_usage, options);
 
 	if (delete)
 		return delete_branches(argc, argv, delete > 1, kinds);
+	else if (list_deleted)
+		return list_deleted_branches(argv);
 	else if (argc == 0)
 		return print_ref_list(kinds, detached, verbose, abbrev, with_commit);
 	else if (rename && (argc == 1))
diff --git a/cache.h b/cache.h
index 2ef2fa3..0a82612 100644
--- a/cache.h
+++ b/cache.h
@@ -537,6 +537,7 @@ extern int ignore_case;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
+extern int log_ref_deletion;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
 extern const char *apply_default_whitespace;
diff --git a/config.c b/config.c
index 4b0a820..cfa162a 100644
--- a/config.c
+++ b/config.c
@@ -509,6 +509,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.logrefdeletion")) {
+		log_ref_deletion = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.warnambiguousrefs")) {
 		warn_ambiguous_refs = git_config_bool(var, value);
 		return 0;
diff --git a/environment.c b/environment.c
index 2d0c315..12166d9 100644
--- a/environment.c
+++ b/environment.c
@@ -20,6 +20,7 @@ int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
+int log_ref_deletion = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int repository_format_version;
 const char *git_commit_encoding;
diff --git a/refs.c b/refs.c
index e3c0511..afdd634 100644
--- a/refs.c
+++ b/refs.c
@@ -3,11 +3,14 @@
 #include "object.h"
 #include "tag.h"
 #include "dir.h"
+#include "string-list.h"
 
 /* ISSYMREF=01 and ISPACKED=02 are public interfaces */
 #define REF_KNOWS_PEELED 04
 #define REF_BROKEN 010
 
+#define BRANCH_DELETION_LOG "DELETED-REFS"
+
 struct ref_list {
 	struct ref_list *next;
 	unsigned char flag; /* ISSYMREF? ISPACKED? */
@@ -1137,44 +1140,6 @@ static int repack_without_ref(const char *refname)
 	return commit_lock_file(&packlock);
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
-{
-	struct ref_lock *lock;
-	int err, i = 0, ret = 0, flag = 0;
-
-	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
-	if (!lock)
-		return 1;
-	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
-		/* loose */
-		const char *path;
-
-		if (!(delopt & REF_NODEREF)) {
-			i = strlen(lock->lk->filename) - 5; /* .lock */
-			lock->lk->filename[i] = 0;
-			path = lock->lk->filename;
-		} else {
-			path = git_path("%s", refname);
-		}
-		err = unlink_or_warn(path);
-		if (err && errno != ENOENT)
-			ret = 1;
-
-		if (!(delopt & REF_NODEREF))
-			lock->lk->filename[i] = '.';
-	}
-	/* removing the loose one could have resurrected an earlier
-	 * packed one.  Also, if it was not loose we need to repack
-	 * without it.
-	 */
-	ret |= repack_without_ref(refname);
-
-	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	invalidate_cached_refs();
-	unlock_ref(lock);
-	return ret;
-}
-
 /*
  * People using contrib's git-new-workdir have .git/logs/refs ->
  * /some/other/path/.git/logs/refs, and that may live on another device.
@@ -1361,11 +1326,13 @@ int log_ref_setup(const char *ref_name, char *logfile, int bufsize)
 	int logfd, oflags = O_APPEND | O_WRONLY;
 
 	git_snpath(logfile, bufsize, "logs/%s", ref_name);
-	if (log_all_ref_updates &&
-	    (!prefixcmp(ref_name, "refs/heads/") ||
-	     !prefixcmp(ref_name, "refs/remotes/") ||
-	     !prefixcmp(ref_name, "refs/notes/") ||
-	     !strcmp(ref_name, "HEAD"))) {
+	if ((log_all_ref_updates &&
+	     (!prefixcmp(ref_name, "refs/heads/") ||
+	      !prefixcmp(ref_name, "refs/remotes/") ||
+	      !prefixcmp(ref_name, "refs/notes/") ||
+	      !strcmp(ref_name, "HEAD"))) ||
+	    (log_ref_deletion &&
+	     !strcmp(ref_name, BRANCH_DELETION_LOG))) {
 		if (safe_create_leading_directories(logfile) < 0)
 			return error("unable to create directory for %s",
 				     logfile);
@@ -1407,6 +1374,8 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
+	if (log_ref_deletion < 0)
+		log_ref_deletion = !is_bare_repository();
 
 	result = log_ref_setup(ref_name, log_file, sizeof(log_file));
 	if (result)
@@ -1432,6 +1401,118 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 	return 0;
 }
 
+struct filter_deleted {
+	const char *pfx;
+	const char **pattern;
+	size_t pfxlen;
+	struct string_list *list;
+};
+
+static int collect_deleted(unsigned char *osha1, unsigned char *nsha1,
+			   const char *ident,
+			   unsigned long timestamp, int tz,
+			   const char *msg, void *cb_data)
+{
+	struct filter_deleted *filter = cb_data;
+	struct string_list_item *item;
+	const char *nameloc;
+	char *namebody;
+	char namebody_buf[1024];
+	size_t namebodylen;
+
+	if (prefixcmp(msg, "delete ") ||
+	    memcmp(msg + 7, filter->pfx, filter->pfxlen) ||
+	    !is_null_sha1(nsha1))
+		return 0;
+	nameloc = msg + 7 + filter->pfxlen;
+	namebodylen = strlen(nameloc); /* counts final LF */
+	if (!namebodylen || nameloc[namebodylen - 1] != '\n')
+		return 0;
+	if (sizeof(namebody_buf) <= namebodylen)
+		namebody = xmalloc(namebodylen);
+	else
+		namebody = namebody_buf;
+	memcpy(namebody, nameloc, namebodylen);
+	namebody[namebodylen - 1] = '\0';
+	if (filter->pattern[0]) {
+		int i, matches;
+		for (i = matches = 0; !matches && filter->pattern[i]; i++)
+			if (fnmatch(filter->pattern[i], namebody, 0))
+				matches = 1;
+		if (!matches)
+			goto free_return;
+	}
+	item = string_list_insert(filter->list, namebody);
+	if (!item->util)
+		item->util = xmalloc(41);
+	strcpy(item->util, sha1_to_hex(osha1));
+free_return:
+	if (namebody != namebody_buf)
+		free(namebody);
+	return 0;
+}
+
+struct string_list *list_deleted_refs(const char *pfx, const char **pattern)
+{
+	struct filter_deleted filter = { pfx, pattern };
+	filter.list = xcalloc(1, sizeof(*(filter.list)));
+	filter.list->strdup_strings = 1;
+	filter.pfxlen = strlen(pfx);
+
+	for_each_reflog_ent(BRANCH_DELETION_LOG, collect_deleted, &filter);
+	return filter.list;
+}
+
+int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
+{
+	struct ref_lock *lock;
+	int err, i = 0, ret = 0, flag = 0;
+	struct strbuf logmsg = STRBUF_INIT;
+
+	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
+	if (!lock)
+		return 1;
+	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
+		/* loose */
+		const char *path;
+
+		if (!(delopt & REF_NODEREF)) {
+			i = strlen(lock->lk->filename) - 5; /* .lock */
+			lock->lk->filename[i] = 0;
+			path = lock->lk->filename;
+		} else {
+			path = git_path("%s", refname);
+		}
+		err = unlink_or_warn(path);
+		if (err && errno != ENOENT)
+			ret = 1;
+
+		if (!(delopt & REF_NODEREF))
+			lock->lk->filename[i] = '.';
+	}
+	/*
+	 * removing the loose one could have resurrected an earlier
+	 * packed one.  Also, if it was not loose we need to repack
+	 * without it.
+	 */
+	ret |= repack_without_ref(refname);
+
+	unlink_or_warn(git_path("logs/%s", lock->ref_name));
+	invalidate_cached_refs();
+
+	strbuf_addf(&logmsg, "delete %s", refname);
+	if (log_ref_write(BRANCH_DELETION_LOG,
+			  lock->old_sha1, null_sha1, logmsg.buf))
+		/*
+		 * there isn't much we can do at this point upon
+		 * failing to record the branch deletion, and an error
+		 * messages have been already issued.
+		 */
+		;
+	unlock_ref(lock);
+	return ret;
+}
+
 static int is_branch(const char *refname)
 {
 	return !strcmp(refname, "HEAD") || !prefixcmp(refname, "refs/heads/");
diff --git a/refs.h b/refs.h
index 5e7a9a5..9fcf7ac 100644
--- a/refs.h
+++ b/refs.h
@@ -87,6 +87,8 @@ typedef int each_reflog_ent_fn(unsigned char *osha1, unsigned char *nsha1, const
 int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data);
 int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long, void *cb_data);
 
+struct string_list *list_deleted_refs(const char *prefix, const char **pattern);
+
 /*
  * Calls the specified function for each reflog file until it returns nonzero,
  * and returns the value
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 54ba3df..ad9f461 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -9,6 +9,7 @@ test_description='Test git update-ref and basic ref logging'
 Z=0000000000000000000000000000000000000000
 
 test_expect_success setup '
+	git config core.logrefdeletion false &&
 
 	for name in A B C D E F
 	do
@@ -38,7 +39,8 @@ test_expect_success "fail to delete $m with stale ref" '
 '
 test_expect_success "delete $m" '
 	git update-ref -d $m $B &&
-	! test -f .git/$m
+	! test -f .git/$m &&
+	! test -f .git/logs/DELETED-REFS
 '
 rm -f .git/$m
 
@@ -46,7 +48,8 @@ test_expect_success "delete $m without oldvalue verification" "
 	git update-ref $m $A &&
 	test $A = \$(cat .git/$m) &&
 	git update-ref -d $m &&
-	! test -f .git/$m
+	! test -f .git/$m &&
+	! test -f .git/logs/DELETED-REFS
 "
 rm -f .git/$m
 
@@ -285,4 +288,50 @@ test_expect_success \
 	'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
 	'test OTHER = $(git cat-file blob "master@{2005-05-26 23:42}:F")'
 
+test_expect_success 'reflog for deletion' '
+	git config core.logrefdeletion yes &&
+
+	git branch frotz HEAD &&
+	git branch nitfol $A &&
+	git branch xyzzy $B &&
+
+	git branch -d frotz &&
+	git branch -D nitfol &&
+	git update-ref -d refs/heads/xyzzy &&
+	{
+		echo "$(git rev-parse HEAD) frotz"
+		echo "$A nitfol"
+		echo "$B xyzzy"
+	} >expect &&
+	git branch --list-deleted >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'reflog for deletion disabled by default in a bare repo' '
+	rm -fr test &&
+	mkdir test &&
+	(
+		cd test && git --bare init &&
+		git fetch .. HEAD:refs/heads/master HEAD:refs/heads/slave &&
+		git update-ref -d refs/heads/slave &&
+		git branch --list-deleted >actual &&
+		>expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'reflog for deletion can be enabled in a bare repo' '
+	rm -fr test &&
+	mkdir test &&
+	(
+		cd test && git --bare init &&
+		git config core.logrefdeletion yes &&
+		git fetch .. HEAD:refs/heads/master HEAD:refs/heads/slave &&
+		git update-ref -d refs/heads/slave &&
+		git branch --list-deleted >actual &&
+		echo "$(git rev-parse master) slave" >expect &&
+		test_cmp expect actual
+	)
+'
+
 test_done
diff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh
index 200ab61..f6209d2 100755
--- a/t/t7701-repack-unpack-unreachable.sh
+++ b/t/t7701-repack-unpack-unreachable.sh
@@ -9,6 +9,10 @@ csha1=
 tsha1=
 
 test_expect_success '-A with -d option leaves unreachable objects unpacked' '
+	# The test expects branch removal to lose the last reference to
+	# lost objects.  Disable branch deletion log to achieve this.
+	git config core.logrefdeletion false &&
+
 	echo content > file1 &&
 	git add . &&
 	test_tick &&

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-06 21:16 [PATCH] logging branch deletion to help recovering from mistakes Junio C Hamano
@ 2010-12-06 21:55 ` Štěpán Němec
  2010-12-06 23:14 ` Andreas Schwab
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 28+ messages in thread
From: Štěpán Němec @ 2010-12-06 21:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

[...]

Just two cosmetic nits I noticed:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d82c0da..bdf90eb 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -351,6 +351,12 @@ This value is true by default in a repository that has
>  a working directory associated with it, and false by
>  default in a bare repository.
>  
> +core.logRefDeletion::
> +	Enable logging of eletion of refs (e.g. branches), allowing `git
                          ^^^^^^^

[...]

> diff --git a/refs.c b/refs.c
> index e3c0511..afdd634 100644
> --- a/refs.c
> +++ b/refs.c

[...]

> -	/* removing the loose one could have resurrected an earlier
> -	 * packed one.  Also, if it was not loose we need to repack
> -	 * without it.
> -	 */

Could also the first sentence start with a capital letter?

[...]

> +		if (!(delopt & REF_NODEREF))
> +			lock->lk->filename[i] = '.';
> +	}
> +	/*
> +	 * removing the loose one could have resurrected an earlier
> +	 * packed one.  Also, if it was not loose we need to repack
> +	 * without it.
> +	 */

Same here.

Thank you,

  Štěpán

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-06 21:16 [PATCH] logging branch deletion to help recovering from mistakes Junio C Hamano
  2010-12-06 21:55 ` Štěpán Němec
@ 2010-12-06 23:14 ` Andreas Schwab
  2010-12-07  1:18 ` Nguyen Thai Ngoc Duy
  2010-12-07 16:23 ` Casey Dahlin
  3 siblings, 0 replies; 28+ messages in thread
From: Andreas Schwab @ 2010-12-06 23:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

> +core.logRefDeletion::
> +	Enable logging of eletion of refs (e.g. branches), allowing `git

                         +d

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-06 21:16 [PATCH] logging branch deletion to help recovering from mistakes Junio C Hamano
  2010-12-06 21:55 ` Štěpán Němec
  2010-12-06 23:14 ` Andreas Schwab
@ 2010-12-07  1:18 ` Nguyen Thai Ngoc Duy
  2010-12-07  6:28   ` Junio C Hamano
  2010-12-07 16:23 ` Casey Dahlin
  3 siblings, 1 reply; 28+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-12-07  1:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Tue, Dec 7, 2010 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> +#define BRANCH_DELETION_LOG "DELETED-REFS"
> +

Should this special log be mentioned in git-update-ref.txt or
gitrepository-layout.txt?
-- 
Duy

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07  1:18 ` Nguyen Thai Ngoc Duy
@ 2010-12-07  6:28   ` Junio C Hamano
  2010-12-07 11:37     ` Nguyen Thai Ngoc Duy
  2010-12-07 17:06     ` Jeff King
  0 siblings, 2 replies; 28+ messages in thread
From: Junio C Hamano @ 2010-12-07  6:28 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: git

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Tue, Dec 7, 2010 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> +#define BRANCH_DELETION_LOG "DELETED-REFS"
>> +
>
> Should this special log be mentioned in git-update-ref.txt or
> gitrepository-layout.txt?

Perhaps, but I wasn't sure if this patch itself is a good idea to begin
with.  Not the problem it tries to solve, but its approach.

For example, this cannot be shown with "reflog show" or "log -g" due to
the way these frontends locate the reflog file to read (the logic wants to
have an underlying ref).

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07  6:28   ` Junio C Hamano
@ 2010-12-07 11:37     ` Nguyen Thai Ngoc Duy
  2010-12-07 15:25       ` Michael J Gruber
  2010-12-07 16:22       ` Jakub Narebski
  2010-12-07 17:06     ` Jeff King
  1 sibling, 2 replies; 28+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-12-07 11:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Tue, Dec 7, 2010 at 1:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Tue, Dec 7, 2010 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> +#define BRANCH_DELETION_LOG "DELETED-REFS"
>>> +
>>
>> Should this special log be mentioned in git-update-ref.txt or
>> gitrepository-layout.txt?
>
> Perhaps, but I wasn't sure if this patch itself is a good idea to begin
> with.  Not the problem it tries to solve, but its approach.
>
> For example, this cannot be shown with "reflog show" or "log -g" due to
> the way these frontends locate the reflog file to read (the logic wants to
> have an underlying ref).
>

I think you have thought of this. What's wrong with keeping reflog
when a branch is removed and appending "delete" line to the said
reflog? I don't know how reflogs are managed, but those reflogs
without associated branch will (or should) be cleaned when they are
expired.

I stick with this idea because I also want to archive old branches and
am thinking those reflogs ending with "archive" line will be kept
forever, or until I feel like digging up them again.
-- 
Duy

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 11:37     ` Nguyen Thai Ngoc Duy
@ 2010-12-07 15:25       ` Michael J Gruber
  2010-12-07 16:25         ` Nguyen Thai Ngoc Duy
  2010-12-07 16:22       ` Jakub Narebski
  1 sibling, 1 reply; 28+ messages in thread
From: Michael J Gruber @ 2010-12-07 15:25 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git

Nguyen Thai Ngoc Duy venit, vidit, dixit 07.12.2010 12:37:
> On Tue, Dec 7, 2010 at 1:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>
>>> On Tue, Dec 7, 2010 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> +#define BRANCH_DELETION_LOG "DELETED-REFS"
>>>> +
>>>
>>> Should this special log be mentioned in git-update-ref.txt or
>>> gitrepository-layout.txt?
>>
>> Perhaps, but I wasn't sure if this patch itself is a good idea to begin
>> with.  Not the problem it tries to solve, but its approach.
>>
>> For example, this cannot be shown with "reflog show" or "log -g" due to
>> the way these frontends locate the reflog file to read (the logic wants to
>> have an underlying ref).
>>
> 
> I think you have thought of this. What's wrong with keeping reflog
> when a branch is removed and appending "delete" line to the said
> reflog? I don't know how reflogs are managed, but those reflogs
> without associated branch will (or should) be cleaned when they are
> expired.

The problem is the following:

Say, you delete a branch and its reflog is kept (with a "delete" line
appended).

Then you create a new branch under the same name. What is supposed to
happen to the reflog? If you simply append, then old (unrelated) entries
will not expire through the imagined "expire branch reflogs" mechanism.

Now, you rename that branch. We should really split the reflog in two
now, keeping the old name for the old parts and moving only the newer
parts to the reflog with the new name.

This is all workable in principle but hints at a design flaw.

Maybe it's easier to teach "git reflog" about "DELETED_REFS"?

Michael

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 11:37     ` Nguyen Thai Ngoc Duy
  2010-12-07 15:25       ` Michael J Gruber
@ 2010-12-07 16:22       ` Jakub Narebski
  2010-12-07 16:26         ` Nguyen Thai Ngoc Duy
  1 sibling, 1 reply; 28+ messages in thread
From: Jakub Narebski @ 2010-12-07 16:22 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On Tue, Dec 7, 2010 at 1:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>
>>> On Tue, Dec 7, 2010 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> +#define BRANCH_DELETION_LOG "DELETED-REFS"
>>>> +
>>>
>>> Should this special log be mentioned in git-update-ref.txt or
>>> gitrepository-layout.txt?
>>
>> Perhaps, but I wasn't sure if this patch itself is a good idea to begin
>> with.  Not the problem it tries to solve, but its approach.
>>
>> For example, this cannot be shown with "reflog show" or "log -g" due to
>> the way these frontends locate the reflog file to read (the logic wants to
>> have an underlying ref).
>>
> 
> I think you have thought of this. What's wrong with keeping reflog
> when a branch is removed and appending "delete" line to the said
> reflog? I don't know how reflogs are managed, but those reflogs
> without associated branch will (or should) be cleaned when they are
> expired.
>
> I stick with this idea because I also want to archive old branches and
> am thinking those reflogs ending with "archive" line will be kept
> forever, or until I feel like digging up them again.

The problem with this idea is deleting branch 'foo' and creating 'foo/bar',
or deleting branch 'foo/bar' and creating branch 'foo'.  Old reflog with
"delete" line would block creating reflog for new branch.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-06 21:16 [PATCH] logging branch deletion to help recovering from mistakes Junio C Hamano
                   ` (2 preceding siblings ...)
  2010-12-07  1:18 ` Nguyen Thai Ngoc Duy
@ 2010-12-07 16:23 ` Casey Dahlin
  2010-12-07 17:45   ` Jonathan Nieder
  3 siblings, 1 reply; 28+ messages in thread
From: Casey Dahlin @ 2010-12-07 16:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Mon, Dec 06, 2010 at 01:16:18PM -0800, Junio C Hamano wrote:
> This adds core.logrefdeletion configuration variable (enabled by default
> in a repository with a work tree, just like core.logallrefupdates), and
> logs deletion of refs via "update-ref -d", "branch -d", etc.
> 
> "git branch" learns a new "--list-deleted" option to help users view the
> names of branches and the commit objects that were at the tip of them when
> the branches were deleted.
> 

Could commits made onto a detached head also show up here? Or is that
better thwarted with another mechanism?

--CJD

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 15:25       ` Michael J Gruber
@ 2010-12-07 16:25         ` Nguyen Thai Ngoc Duy
  0 siblings, 0 replies; 28+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-12-07 16:25 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: Junio C Hamano, git

On Tue, Dec 7, 2010 at 10:25 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Nguyen Thai Ngoc Duy venit, vidit, dixit 07.12.2010 12:37:
>> On Tue, Dec 7, 2010 at 1:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>>
>>>> On Tue, Dec 7, 2010 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> +#define BRANCH_DELETION_LOG "DELETED-REFS"
>>>>> +
>>>>
>>>> Should this special log be mentioned in git-update-ref.txt or
>>>> gitrepository-layout.txt?
>>>
>>> Perhaps, but I wasn't sure if this patch itself is a good idea to begin
>>> with.  Not the problem it tries to solve, but its approach.
>>>
>>> For example, this cannot be shown with "reflog show" or "log -g" due to
>>> the way these frontends locate the reflog file to read (the logic wants to
>>> have an underlying ref).
>>>
>>
>> I think you have thought of this. What's wrong with keeping reflog
>> when a branch is removed and appending "delete" line to the said
>> reflog? I don't know how reflogs are managed, but those reflogs
>> without associated branch will (or should) be cleaned when they are
>> expired.
>
> The problem is the following:
>
> Say, you delete a branch and its reflog is kept (with a "delete" line
> appended).
>
> Then you create a new branch under the same name. What is supposed to
> happen to the reflog? If you simply append, then old (unrelated) entries
> will not expire through the imagined "expire branch reflogs" mechanism.
>
> Now, you rename that branch. We should really split the reflog in two
> now, keeping the old name for the old parts and moving only the newer
> parts to the reflog with the new name.

I don't see any problems with that. If I happen to create a branch
with the same name, most of the time, there is something related,
unless for very generic names like "tmp". We can always notify users
about the accident resurrection of an old branch at branch creation,
so they can remove the old reflog if they want.
-- 
Duy

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 16:22       ` Jakub Narebski
@ 2010-12-07 16:26         ` Nguyen Thai Ngoc Duy
  0 siblings, 0 replies; 28+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-12-07 16:26 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Junio C Hamano, git

On Tue, Dec 7, 2010 at 11:22 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>> On Tue, Dec 7, 2010 at 1:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>>
>>>> On Tue, Dec 7, 2010 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> +#define BRANCH_DELETION_LOG "DELETED-REFS"
>>>>> +
>>>>
>>>> Should this special log be mentioned in git-update-ref.txt or
>>>> gitrepository-layout.txt?
>>>
>>> Perhaps, but I wasn't sure if this patch itself is a good idea to begin
>>> with.  Not the problem it tries to solve, but its approach.
>>>
>>> For example, this cannot be shown with "reflog show" or "log -g" due to
>>> the way these frontends locate the reflog file to read (the logic wants to
>>> have an underlying ref).
>>>
>>
>> I think you have thought of this. What's wrong with keeping reflog
>> when a branch is removed and appending "delete" line to the said
>> reflog? I don't know how reflogs are managed, but those reflogs
>> without associated branch will (or should) be cleaned when they are
>> expired.
>>
>> I stick with this idea because I also want to archive old branches and
>> am thinking those reflogs ending with "archive" line will be kept
>> forever, or until I feel like digging up them again.
>
> The problem with this idea is deleting branch 'foo' and creating 'foo/bar',
> or deleting branch 'foo/bar' and creating branch 'foo'.  Old reflog with
> "delete" line would block creating reflog for new branch.

Thanks. That makes sense.
-- 
Duy

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07  6:28   ` Junio C Hamano
  2010-12-07 11:37     ` Nguyen Thai Ngoc Duy
@ 2010-12-07 17:06     ` Jeff King
  2010-12-07 18:14       ` Shawn Pearce
  2010-12-07 19:21       ` Junio C Hamano
  1 sibling, 2 replies; 28+ messages in thread
From: Jeff King @ 2010-12-07 17:06 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nguyen Thai Ngoc Duy, git

On Mon, Dec 06, 2010 at 10:28:53PM -0800, Junio C Hamano wrote:

> > Should this special log be mentioned in git-update-ref.txt or
> > gitrepository-layout.txt?
> 
> Perhaps, but I wasn't sure if this patch itself is a good idea to begin
> with.  Not the problem it tries to solve, but its approach.
> 
> For example, this cannot be shown with "reflog show" or "log -g" due to
> the way these frontends locate the reflog file to read (the logic wants to
> have an underlying ref).

Yeah, I think this is not _quite_ what people want in this area. A base
requirement from past discussions, I think, is that the whole reflog of
the deleted branch be saved rather than just the tip. And then "reflog
show" would make a lot more sense on such saved reflogs.

I'm not sure in practice how important that distinction is, as we are
not saving deleted branch reflogs _at all_ right now, so the
requirements are mostly speculation at this point.

The most recent discussion I recall is this one:

  http://thread.gmane.org/gmane.comp.version-control.git/144250/focus=145353

where the general idea was to just keep deleted reflogs around, append
to them if the branch was recreated, and use a consistent renaming
scheme to avoid D/F naming conflicts (e.g., "foo" is a deleted ref, and
you create "foo/bar").

-Peff

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 16:23 ` Casey Dahlin
@ 2010-12-07 17:45   ` Jonathan Nieder
  2010-12-07 17:54     ` Casey Dahlin
  0 siblings, 1 reply; 28+ messages in thread
From: Jonathan Nieder @ 2010-12-07 17:45 UTC (permalink / raw)
  To: Casey Dahlin; +Cc: Junio C Hamano, git

Casey Dahlin wrote:

> Could commits made onto a detached head also show up here? Or is that
> better thwarted with another mechanism?

I think that's better thwarted with the HEAD reflog:

	$ git log -g HEAD

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 17:45   ` Jonathan Nieder
@ 2010-12-07 17:54     ` Casey Dahlin
  2010-12-07 18:02       ` Jonathan Nieder
  2010-12-07 18:12       ` Jeff King
  0 siblings, 2 replies; 28+ messages in thread
From: Casey Dahlin @ 2010-12-07 17:54 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Casey Dahlin, Junio C Hamano, git

On Tue, Dec 07, 2010 at 11:45:20AM -0600, Jonathan Nieder wrote:
> Casey Dahlin wrote:
> 
> > Could commits made onto a detached head also show up here? Or is that
> > better thwarted with another mechanism?
> 
> I think that's better thwarted with the HEAD reflog:
> 
> 	$ git log -g HEAD

I was more worried about changes that were made onto a detached head,
and then the head was reattached, leaving the new commits dangling.

The end result is identical to a deleted branch, just wondering if we
should note it in the same place.

--CJD

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 17:54     ` Casey Dahlin
@ 2010-12-07 18:02       ` Jonathan Nieder
  2010-12-07 18:26         ` Casey Dahlin
  2010-12-07 20:25         ` Junio C Hamano
  2010-12-07 18:12       ` Jeff King
  1 sibling, 2 replies; 28+ messages in thread
From: Jonathan Nieder @ 2010-12-07 18:02 UTC (permalink / raw)
  To: Casey Dahlin; +Cc: Junio C Hamano, git

Casey Dahlin wrote:
> On Tue, Dec 07, 2010 at 11:45:20AM -0600, Jonathan Nieder wrote:
>> Casey Dahlin wrote:

>>> Could commits made onto a detached head also show up here? Or is that
>>> better thwarted with another mechanism?
>>
>> I think that's better thwarted with the HEAD reflog:
>> 
>> 	$ git log -g HEAD
>
> I was more worried about changes that were made onto a detached head,
> and then the head was reattached, leaving the new commits dangling.

But isn't that exactly what a detached HEAD is for?  If one wants
the experiments one does on detached HEAD to be kept around "just
in case", wouldn't it make more sense to give them a (branch) name so
they can be separated from one another?

In other words, I do not see the connection yet.  Maybe it would be
best to propose another patch on top to do that?  (Patches often come
with documentation, which means clear explanation of use cases, which
would address my worry here.)

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 17:54     ` Casey Dahlin
  2010-12-07 18:02       ` Jonathan Nieder
@ 2010-12-07 18:12       ` Jeff King
  1 sibling, 0 replies; 28+ messages in thread
From: Jeff King @ 2010-12-07 18:12 UTC (permalink / raw)
  To: Casey Dahlin; +Cc: Jonathan Nieder, Junio C Hamano, git

On Tue, Dec 07, 2010 at 12:54:18PM -0500, Casey Dahlin wrote:

> On Tue, Dec 07, 2010 at 11:45:20AM -0600, Jonathan Nieder wrote:
> > Casey Dahlin wrote:
> > 
> > > Could commits made onto a detached head also show up here? Or is that
> > > better thwarted with another mechanism?
> > 
> > I think that's better thwarted with the HEAD reflog:
> > 
> > 	$ git log -g HEAD
> 
> I was more worried about changes that were made onto a detached head,
> and then the head was reattached, leaving the new commits dangling.
> 
> The end result is identical to a deleted branch, just wondering if we
> should note it in the same place.

We have enough information in the HEAD reflog already to reconstruct
those sorts of things.

You can detect entering and leaving the detached HEAD in the reflog. The
reflog comments look something like this:

  checkout: moving from $SOME_BRANCH to $SOME_SHA1
  commit: $SOME_COMMIT_MESSAGE
  checkout: moving from $SOME_OTHER_SHA1 to $BRANCH|$SHA1

So from that you can see that we entered a detached HEAD state, made a
commit, and that commit became dangling when we moved. One could write a
script to search for these cases (but note that most detached instances
just involve rebasing, which is probably not interesting, as we install
the result into the branch tip at the end).

I don't think this belongs in the same realm as "deleted branches", but
I do think we could have a special option to "git fsck" to stick these
into lost-found.

-Peff

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 17:06     ` Jeff King
@ 2010-12-07 18:14       ` Shawn Pearce
  2010-12-07 18:20         ` Jeff King
  2010-12-07 19:21       ` Junio C Hamano
  1 sibling, 1 reply; 28+ messages in thread
From: Shawn Pearce @ 2010-12-07 18:14 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Nguyen Thai Ngoc Duy, git

On Tue, Dec 7, 2010 at 9:06 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 06, 2010 at 10:28:53PM -0800, Junio C Hamano wrote:
>
>> > Should this special log be mentioned in git-update-ref.txt or
>> > gitrepository-layout.txt?
>>
>> Perhaps, but I wasn't sure if this patch itself is a good idea to begin
>> with.  Not the problem it tries to solve, but its approach.
>>
>> For example, this cannot be shown with "reflog show" or "log -g" due to
>> the way these frontends locate the reflog file to read (the logic wants to
>> have an underlying ref).
>
> Yeah, I think this is not _quite_ what people want in this area. A base
> requirement from past discussions, I think, is that the whole reflog of
> the deleted branch be saved rather than just the tip. And then "reflog
> show" would make a lot more sense on such saved reflogs.

Yup, that's what I recall too, folks (including myself) want to
save the reflog of the deleted branch, so it can be recovered if
the branch itself were to be recovered with an --undelete option.

> I'm not sure in practice how important that distinction is, as we are
> not saving deleted branch reflogs _at all_ right now, so the
> requirements are mostly speculation at this point.
>
> The most recent discussion I recall is this one:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/144250/focus=145353
>
> where the general idea was to just keep deleted reflogs around, append
> to them if the branch was recreated, and use a consistent renaming
> scheme to avoid D/F naming conflicts (e.g., "foo" is a deleted ref, and
> you create "foo/bar").

Per check-ref-format, ref names cannot contain two dots.  We could
archive ref logs by renaming them, $GIT_DIR/logs/refs/heads/foo
becomes $GIT_DIR/logs/refs/heads/foo..deleted-1.  If foo is created
and deleted again, it becomes foo..deleted-2.

This still causes problems for git reflog show / git log -g because
they want a current ref to enumerate the log of.


A different approach might be to have $GIT_DIR/logs/refs/REF_ATTIC,
and special case that in git reflog show / git log -g.  When a
ref is deleted, append its entire log onto REF_ATTIC, between two
specially formatted marker lines.  When recovering a branch, copy
out the region from the REF_ATTIC log.

-- 
Shawn.

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 18:14       ` Shawn Pearce
@ 2010-12-07 18:20         ` Jeff King
  2010-12-07 18:23           ` Shawn O. Pearce
  0 siblings, 1 reply; 28+ messages in thread
From: Jeff King @ 2010-12-07 18:20 UTC (permalink / raw)
  To: Shawn Pearce; +Cc: Junio C Hamano, Nguyen Thai Ngoc Duy, git

On Tue, Dec 07, 2010 at 10:14:19AM -0800, Shawn O. Pearce wrote:

> Per check-ref-format, ref names cannot contain two dots.  We could
> archive ref logs by renaming them, $GIT_DIR/logs/refs/heads/foo
> becomes $GIT_DIR/logs/refs/heads/foo..deleted-1.  If foo is created
> and deleted again, it becomes foo..deleted-2.
> 
> This still causes problems for git reflog show / git log -g because
> they want a current ref to enumerate the log of.

That seems reasonable to me. The "reflog show" limitation is just a
matter of a simple code fix, though, isn't it? Is there a good reason
for this restriction to exist? And even if there is, it would be simple
to special case it for ..deleted-* branches.

> A different approach might be to have $GIT_DIR/logs/refs/REF_ATTIC,
> and special case that in git reflog show / git log -g.  When a
> ref is deleted, append its entire log onto REF_ATTIC, between two
> specially formatted marker lines.  When recovering a branch, copy
> out the region from the REF_ATTIC log.

That seems a lot less efficient, as we have to linearly search all of
REF_ATTIC to get:

  1. the reflog for one deleted branch

  2. the list of deleted branches

Neither of those is probably particularly performance critical, but it
just seems like keeping the logs in files indexed by the original ref
names is a more natural fit.

-Peff

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 18:20         ` Jeff King
@ 2010-12-07 18:23           ` Shawn O. Pearce
  2010-12-07 18:35             ` Jeff King
  0 siblings, 1 reply; 28+ messages in thread
From: Shawn O. Pearce @ 2010-12-07 18:23 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Nguyen Thai Ngoc Duy, git

Jeff King <peff@peff.net> wrote:
> On Tue, Dec 07, 2010 at 10:14:19AM -0800, Shawn O. Pearce wrote:
> 
> > Per check-ref-format, ref names cannot contain two dots.  We could
> > archive ref logs by renaming them, $GIT_DIR/logs/refs/heads/foo
> > becomes $GIT_DIR/logs/refs/heads/foo..deleted-1.  If foo is created
> > and deleted again, it becomes foo..deleted-2.
...
> > A different approach might be to have $GIT_DIR/logs/refs/REF_ATTIC,
> 
> That seems a lot less efficient, as we have to linearly search all of
> REF_ATTIC to get:
> 
>   1. the reflog for one deleted branch
> 
>   2. the list of deleted branches
> 
> Neither of those is probably particularly performance critical, but it
> just seems like keeping the logs in files indexed by the original ref
> names is a more natural fit.

Yea, I'm leaning more towards the foo..deleted-n idea too, for the
same reasons.  It also makes it easier to GC a deleted branch's
reflog, we can examine the last record's timestamp in a reasonable
time bound and unlink the log if its really freaking old.

-- 
Shawn.

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 18:02       ` Jonathan Nieder
@ 2010-12-07 18:26         ` Casey Dahlin
  2010-12-07 20:25         ` Junio C Hamano
  1 sibling, 0 replies; 28+ messages in thread
From: Casey Dahlin @ 2010-12-07 18:26 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Casey Dahlin, Junio C Hamano, git

On Tue, Dec 07, 2010 at 12:02:36PM -0600, Jonathan Nieder wrote:
> Casey Dahlin wrote:
> > On Tue, Dec 07, 2010 at 11:45:20AM -0600, Jonathan Nieder wrote:
> >> Casey Dahlin wrote:
> 
> >>> Could commits made onto a detached head also show up here? Or is that
> >>> better thwarted with another mechanism?
> >>
> >> I think that's better thwarted with the HEAD reflog:
> >> 
> >> 	$ git log -g HEAD
> >
> > I was more worried about changes that were made onto a detached head,
> > and then the head was reattached, leaving the new commits dangling.
> 
> But isn't that exactly what a detached HEAD is for?  If one wants
> the experiments one does on detached HEAD to be kept around "just
> in case", wouldn't it make more sense to give them a (branch) name so
> they can be separated from one another?
> 

An experienced git user who's paying attention to what he's doing would
do things that way, but detached heads and what happens to commits
thereupon are one of those concepts that tends to puzzle newbies, so
making it harder to make mistakes with them is probably a good idea.

Even still, there's always going to be those "Oh, I actually wanted to
keep some of that" moments, and that's what I'm talking about preparing
for.

--CJD

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 18:23           ` Shawn O. Pearce
@ 2010-12-07 18:35             ` Jeff King
  2010-12-07 18:37               ` Shawn O. Pearce
  0 siblings, 1 reply; 28+ messages in thread
From: Jeff King @ 2010-12-07 18:35 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Junio C Hamano, Nguyen Thai Ngoc Duy, git

On Tue, Dec 07, 2010 at 10:23:42AM -0800, Shawn O. Pearce wrote:

> Yea, I'm leaning more towards the foo..deleted-n idea too, for the
> same reasons.  It also makes it easier to GC a deleted branch's
> reflog, we can examine the last record's timestamp in a reasonable
> time bound and unlink the log if its really freaking old.

Do we need to actually do that? Shouldn't the entries in the reflog get
expired as part of the regular reflog gc? In that case, we would just
delete the file when it had zero entries.

-Peff

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 18:35             ` Jeff King
@ 2010-12-07 18:37               ` Shawn O. Pearce
  2010-12-07 18:39                 ` Jeff King
  0 siblings, 1 reply; 28+ messages in thread
From: Shawn O. Pearce @ 2010-12-07 18:37 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Nguyen Thai Ngoc Duy, git

Jeff King <peff@peff.net> wrote:
> On Tue, Dec 07, 2010 at 10:23:42AM -0800, Shawn O. Pearce wrote:
> 
> > Yea, I'm leaning more towards the foo..deleted-n idea too, for the
> > same reasons.  It also makes it easier to GC a deleted branch's
> > reflog, we can examine the last record's timestamp in a reasonable
> > time bound and unlink the log if its really freaking old.
> 
> Do we need to actually do that? Shouldn't the entries in the reflog get
> expired as part of the regular reflog gc? In that case, we would just
> delete the file when it had zero entries.

Yes, you are right.  We should instead let the normal reflog expire
action do its work here, and delete the empty log file when it is
finally empty.

I guess we also need repack and prune to enumerate these deleted
reflogs and retain the objects their records point to.

-- 
Shawn.

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 18:37               ` Shawn O. Pearce
@ 2010-12-07 18:39                 ` Jeff King
  2010-12-07 18:41                   ` Shawn O. Pearce
  0 siblings, 1 reply; 28+ messages in thread
From: Jeff King @ 2010-12-07 18:39 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Junio C Hamano, Nguyen Thai Ngoc Duy, git

On Tue, Dec 07, 2010 at 10:37:39AM -0800, Shawn O. Pearce wrote:

> Yes, you are right.  We should instead let the normal reflog expire
> action do its work here, and delete the empty log file when it is
> finally empty.
> 
> I guess we also need repack and prune to enumerate these deleted
> reflogs and retain the objects their records point to.

Definitely. I sort of assumed all of those things just traversed
.git/logs blindly without regard to whether there was a ref, which would
handle this automagically. But maybe that is not the case.

Is there a reason to require that each log is specifically tied to a
ref?

-Peff

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 18:39                 ` Jeff King
@ 2010-12-07 18:41                   ` Shawn O. Pearce
  0 siblings, 0 replies; 28+ messages in thread
From: Shawn O. Pearce @ 2010-12-07 18:41 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Nguyen Thai Ngoc Duy, git

Jeff King <peff@peff.net> wrote:
> On Tue, Dec 07, 2010 at 10:37:39AM -0800, Shawn O. Pearce wrote:
> 
> > Yes, you are right.  We should instead let the normal reflog expire
> > action do its work here, and delete the empty log file when it is
> > finally empty.
> > 
> > I guess we also need repack and prune to enumerate these deleted
> > reflogs and retain the objects their records point to.
> 
> Definitely. I sort of assumed all of those things just traversed
> .git/logs blindly without regard to whether there was a ref, which would
> handle this automagically. But maybe that is not the case.

I think those enumerate the logs of refs that are also being
traversed.  Which means we would need to add new logic to enumerate
the deleted reflogs.
 
> Is there a reason to require that each log is specifically tied to a
> ref?

Historical bad assumptions?

I mean, no, there really isn't a good reason that each log is
tied to a ref.  Its probably reasonable to just enumerate the logs
directory separate from the refs directory enumeration.  Its just
some more code.  Right now we discover logs by just relying on the
ref directory traversal code.

-- 
Shawn.

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 17:06     ` Jeff King
  2010-12-07 18:14       ` Shawn Pearce
@ 2010-12-07 19:21       ` Junio C Hamano
  2010-12-07 19:38         ` Jeff King
  1 sibling, 1 reply; 28+ messages in thread
From: Junio C Hamano @ 2010-12-07 19:21 UTC (permalink / raw)
  To: Jeff King; +Cc: Nguyen Thai Ngoc Duy, git

Jeff King <peff@peff.net> writes:

> On Mon, Dec 06, 2010 at 10:28:53PM -0800, Junio C Hamano wrote:
>
> Yeah, I think this is not _quite_ what people want in this area. A base
> requirement from past discussions, I think, is that the whole reflog of
> the deleted branch be saved rather than just the tip. And then "reflog
> show" would make a lot more sense on such saved reflogs.

I am more worried about stuff in branch.<name>.* that are discarded upon
"branch -d".  Without the config items, you won't have a working:

    $ branch -d frotz
    $ branch --undelete frotz
    $ git checkout frotz
    $ git pull

I would say it is fine to discard old reflog for "frotz" branch and tell
users of --undelete that even though their branch is undeleted, the reflog
for it is already expired when they deleted it the first time, but it is
impossible to implement "branch --undelete" without stashing away stuff
other than reflog.

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 19:21       ` Junio C Hamano
@ 2010-12-07 19:38         ` Jeff King
  0 siblings, 0 replies; 28+ messages in thread
From: Jeff King @ 2010-12-07 19:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nguyen Thai Ngoc Duy, git

On Tue, Dec 07, 2010 at 11:21:46AM -0800, Junio C Hamano wrote:

> I am more worried about stuff in branch.<name>.* that are discarded upon
> "branch -d".  Without the config items, you won't have a working:
> 
>     $ branch -d frotz
>     $ branch --undelete frotz
>     $ git checkout frotz
>     $ git pull

Hmm, yeah, I didn't think about that. Two possible solutions:

  1. Just leave it in .git/config. It is not hurting anything if the
     branch does not exist, but it is cruft in a file the user might
     look at.

  2. Drop it into .git/config.dead/<branch_name>. When resurrecting a
     branch, copy it back into .git/config.

In both cases, when the reflog for the deleted branch is pruned to
nothing, we delete the relevant config, too.

In the second case, I think you would have to take special care for
something like:

  $ git branch frotz origin/master
  $ git branch -d frotz
  $ git remote rename origin foo
  $ git branch --undelete frotz

In the non-deleted case, this transparently renames branch.frotz.remote
from "origin" to "foo". In the deleted case, we would need to make sure
the dead config is updated, too.


To be honest, I have never been that interested in a "branch --undelete"
feature. I much more care about leaving the reflogs of deleted branches
around, so I can "git checkout -b foo bar@{1}" later on[1]. That is, to
me branch undeletion is not about bringing a branch back wholesale, but
rather remembering commits so I can start a new branch there.

But I guess others might disagree.

-Peff

[1] Well, that and just piece of mind from knowing that "branch -d" is
    not totally unrecoverable. Specifically, if we kept deleted reflogs
    around, it would be safe(r) to turn on auto-prune on fetch, the lack
    of which is something that seems to confuse new users.

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 18:02       ` Jonathan Nieder
  2010-12-07 18:26         ` Casey Dahlin
@ 2010-12-07 20:25         ` Junio C Hamano
  2010-12-07 20:55           ` Jonathan Nieder
  1 sibling, 1 reply; 28+ messages in thread
From: Junio C Hamano @ 2010-12-07 20:25 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Casey Dahlin, Junio C Hamano, git

Jonathan Nieder <jrnieder@gmail.com> writes:

> Casey Dahlin wrote:
>> On Tue, Dec 07, 2010 at 11:45:20AM -0600, Jonathan Nieder wrote:
>>> Casey Dahlin wrote:
>
>>>> Could commits made onto a detached head also show up here? Or is that
>>>> better thwarted with another mechanism?
>>>
>>> I think that's better thwarted with the HEAD reflog:
>>> 
>>> 	$ git log -g HEAD
>>
>> I was more worried about changes that were made onto a detached head,
>> and then the head was reattached, leaving the new commits dangling.
>
> But isn't that exactly what a detached HEAD is for?  If one wants
> the experiments one does on detached HEAD to be kept around "just
> in case", wouldn't it make more sense to give them a (branch) name so
> they can be separated from one another?

What are you arguing after giving a correct answer.  "git log -g HEAD"
keeps track of what was at the tip of HEAD, be it pointing at a branch or
pointing diretly at a commit in a detached state, no?

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

* Re: [PATCH] logging branch deletion to help recovering from mistakes
  2010-12-07 20:25         ` Junio C Hamano
@ 2010-12-07 20:55           ` Jonathan Nieder
  0 siblings, 0 replies; 28+ messages in thread
From: Jonathan Nieder @ 2010-12-07 20:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Casey Dahlin, git

Junio C Hamano wrote:

> "git log -g HEAD"
> keeps track of what was at the tip of HEAD, be it pointing at a branch or
> pointing diretly at a commit in a detached state, no?

Yes.

1. Imagine I have an interesting branch and delete it:

	$ git branch interesting $(lots of hard work)
	$ git branch -D interesting

Oops.  If I want to recover that branch, I may have a lot of digging
to do in the HEAD reflog.  It may not be there are all.  Your patch
mitigates that by allowing a simple "I didn't mean that" command.

	$ git branch --undelete interesting

2. Great.  Another way to lose a line of development, as Casey
mentioned, is to not give it a branch name in the first place:

	$ git checkout HEAD^0
	...
	$ git checkout something-else

Oops.  Well, not so bad.  If I want to recover my old work, I can
simply use

	$ git checkout HEAD@{1}

3. Now suppose I was not paying attention and made the mistake
from (1) or (2) a week ago and didn't realize it.  Now I want to
get back that code.

If it was situation (1), I can remember the name of the branch
and do

	$ git branch --undelete interesting

No problem [1].  If it was situation (2), I need to dig through the
HEAD reflog.  As Jeff explained, it is possible to script something up
to help organize the search.  I think Casey was suggesting doing that
work at HEAD-reattachment time instead, so you could do

	$ git branch --undelete-detached-head=old-head

to recover the last line of development made without a branch;
my response was that if this ends up frequently being useful
then I suspect something is wrong with the workflow.

Hoping that is clearer,
Jonathan

[1] as long as the branch name was not reused

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

end of thread, other threads:[~2010-12-07 20:55 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-06 21:16 [PATCH] logging branch deletion to help recovering from mistakes Junio C Hamano
2010-12-06 21:55 ` Štěpán Němec
2010-12-06 23:14 ` Andreas Schwab
2010-12-07  1:18 ` Nguyen Thai Ngoc Duy
2010-12-07  6:28   ` Junio C Hamano
2010-12-07 11:37     ` Nguyen Thai Ngoc Duy
2010-12-07 15:25       ` Michael J Gruber
2010-12-07 16:25         ` Nguyen Thai Ngoc Duy
2010-12-07 16:22       ` Jakub Narebski
2010-12-07 16:26         ` Nguyen Thai Ngoc Duy
2010-12-07 17:06     ` Jeff King
2010-12-07 18:14       ` Shawn Pearce
2010-12-07 18:20         ` Jeff King
2010-12-07 18:23           ` Shawn O. Pearce
2010-12-07 18:35             ` Jeff King
2010-12-07 18:37               ` Shawn O. Pearce
2010-12-07 18:39                 ` Jeff King
2010-12-07 18:41                   ` Shawn O. Pearce
2010-12-07 19:21       ` Junio C Hamano
2010-12-07 19:38         ` Jeff King
2010-12-07 16:23 ` Casey Dahlin
2010-12-07 17:45   ` Jonathan Nieder
2010-12-07 17:54     ` Casey Dahlin
2010-12-07 18:02       ` Jonathan Nieder
2010-12-07 18:26         ` Casey Dahlin
2010-12-07 20:25         ` Junio C Hamano
2010-12-07 20:55           ` Jonathan Nieder
2010-12-07 18:12       ` Jeff King

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).