threads / patch / 6069

patch, 3 partsIntroduce a global level warn() function.

Subject: [PATCH 2/3] Introduce a global level warn() function.

## tl;dr

9 messages between Dec 22, 2006 and Dec 22, 2006. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Shawn O. Pearce· Dec 22, 2006, 00:48 UTC · lore

Like the existing error() function the new warn() function can be used to describe a situation that probably should not be occuring, but which the user (and Git) can continue to work around without running into too many problems.

An example situation is a bad commit SHA1 found in a reflog. Attempting to read this record out of the reflog isn't really an error as we have skipped over it in the past.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 git-compat-util.h |    2 ++
 usage.c           |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)
Show changes to 2 files +21 −0

git-compat-util.h, usage.c

diff --git a/git-compat-util.h b/git-compat-util.h
index 41fa7f6..304cdbf 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -71,10 +71,12 @@
 extern void usage(const char *err) NORETURN;
 extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern void warn(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
 extern void set_usage_routine(void (*routine)(const char *err) NORETURN);
 extern void set_die_routine(void (*routine)(const char *err, va_list params) NORETURN);
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
+extern void set_warn_routine(void (*routine)(const char *warn, va_list params));
 
 #ifdef NO_MMAP
 
diff --git a/usage.c b/usage.c
index 52c2e96..4dc5c77 100644
--- a/usage.c
+++ b/usage.c
@@ -29,12 +29,17 @@ static void error_builtin(const char *err, va_list params)
 	report("error: ", err, params);
 }
 
+static void warn_builtin(const char *warn, va_list params)
+{
+	report("warning: ", warn, params);
+}
 
 /* If we are in a dlopen()ed .so write to a global variable would segfault
  * (ugh), so keep things static. */
 static void (*usage_routine)(const char *err) NORETURN = usage_builtin;
 static void (*die_routine)(const char *err, va_list params) NORETURN = die_builtin;
 static void (*error_routine)(const char *err, va_list params) = error_builtin;
+static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
 
 void set_usage_routine(void (*routine)(const char *err) NORETURN)
 {
@@ -51,6 +56,11 @@ void set_error_routine(void (*routine)(const char *err, va_list params))
 	error_routine = routine;
 }
 
+void set_warn_routine(void (*routine)(const char *warn, va_list params))
+{
+	warn_routine = routine;
+}
+
 
 void usage(const char *err)
 {
@@ -75,3 +85,12 @@ int error(const char *err, ...)
 	va_end(params);
 	return -1;
 }
+
+void warn(const char *warn, ...)
+{
+	va_list params;
+
+	va_start(params, warn);
+	warn_routine(warn, params);
+	va_end(params);
+}
-- 
1.4.4.3.ga78a1
Shawn O. Pearce· Dec 22, 2006, 00:49 UTC · lore

[PATCH 3/3] Don't crash during repack of a reflog with pruned commits.

If the user has been using reflog for a long time (e.g. since its introduction) then it is very likely that an existing branch's reflog may still mention commits which have long since been pruned out of the repository.

Rather than aborting with a very useless error message during git-repack, pack as many valid commits as we can get from the reflog and let the user know that the branch's reflog contains already pruned commits. A future 'git reflog expire' (or whatever it finally winds up being called) can then be performed to expunge those reflog entries.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 revision.c |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)
Show changes to revision.c +19 −10
diff --git a/revision.c b/revision.c
index de2cbd8..52330cc 100644
--- a/revision.c
+++ b/revision.c
@@ -466,6 +466,7 @@ static void limit_list(struct rev_info *revs)
 
 struct all_refs_cb {
 	int all_flags;
+	int warned_bad_reflog;
 	struct rev_info *all_revs;
 	const char *name_for_errormsg;
 };
@@ -487,25 +488,33 @@ static void handle_all(struct rev_info *revs, unsigned flags)
 	for_each_ref(handle_one_ref, &cb);
 }
 
-static int handle_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1, char *detail, void *cb_data)
+static int handle_one_reflog_commit(unsigned char *sha1, void *cb_data)
 {
 	struct all_refs_cb *cb = cb_data;
-	struct object *object;
-
-	if (!is_null_sha1(osha1)) {
-		object = get_reference(cb->all_revs, cb->name_for_errormsg,
-				       osha1, cb->all_flags);
-		add_pending_object(cb->all_revs, object, "");
+	if (!is_null_sha1(sha1)) {
+		struct object *o = parse_object(sha1);
+		if (o) {
+			o->flags |= cb->all_flags;
+			add_pending_object(cb->all_revs, o, "");
+		} else if (!cb->warned_bad_reflog) {
+			warn("reflog of '%s' references pruned commits",
+				cb->name_for_errormsg);
+			cb->warned_bad_reflog = 1;
+		}
 	}
-	object = get_reference(cb->all_revs, cb->name_for_errormsg,
-			       nsha1, cb->all_flags);
-	add_pending_object(cb->all_revs, object, "");
+}
+
+static int handle_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1, char *detail, void *cb_data)
+{
+	handle_one_reflog_commit(osha1, cb_data);
+	handle_one_reflog_commit(nsha1, cb_data);
 	return 0;
 }
 
 static int handle_one_reflog(const char *path, const unsigned char *sha1, int flag, void *cb_data)
 {
 	struct all_refs_cb *cb = cb_data;
+	cb->warned_bad_reflog = 0;
 	cb->name_for_errormsg = path;
 	for_each_reflog_ent(path, handle_one_reflog_ent, cb_data);
 	return 0;
-- 
1.4.4.3.ga78a1
Junio C Hamano· Dec 22, 2006, 00:52 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 3/3] Don't crash during repack of a reflog with pruned commits.

"Shawn O. Pearce" <spearce@spearce.org> writes:
> If the user has been using reflog for a long time (e.g. since its
> introduction) then it is very likely that an existing branch's
> reflog may still mention commits which have long since been pruned
> out of the repository.

I've thought about this issue when I did the repack/prune; my take on this was you should prune reflog first then repack.

Shawn Pearce· Dec 22, 2006, 01:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] Don't crash during repack of a reflog with pruned commits.

Junio C Hamano <junkio@cox.net> wrote:
Show 9 quoted lines
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > If the user has been using reflog for a long time (e.g. since its
> > introduction) then it is very likely that an existing branch's
> > reflog may still mention commits which have long since been pruned
> > out of the repository.
> 
> I've thought about this issue when I did the repack/prune; my
> take on this was you should prune reflog first then repack.

OK, but we should suggest that to the user rather than just cryptically saying 'fatal: bad object refs/heads/build'.

-- 
Shawn.
Jakub Narebski· Dec 22, 2006, 01:18 UTC · re: Shawn Pearce · lore

Re: [PATCH 3/3] Don't crash during repack of a reflog with pruned commits.

Shawn Pearce wrote:
Show 13 quoted lines
> Junio C Hamano <junkio@cox.net> wrote:
>> "Shawn O. Pearce" <spearce@spearce.org> writes:
>> 
>>> If the user has been using reflog for a long time (e.g. since its
>>> introduction) then it is very likely that an existing branch's
>>> reflog may still mention commits which have long since been pruned
>>> out of the repository.
>> 
>> I've thought about this issue when I did the repack/prune; my
>> take on this was you should prune reflog first then repack.
> 
> OK, but we should suggest that to the user rather than just
> cryptically saying 'fatal: bad object refs/heads/build'.

I still think it is a good idea to allow user (experienced user) to set to not consider reflog for saving. Especially that there exist repositories which have reflogs with long pruned commits, and it would be nice to preserve the reflog info, even if some of information is not available.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Shawn Pearce· Dec 22, 2006, 01:23 UTC · re: Jakub Narebski · lore

Re: [PATCH 3/3] Don't crash during repack of a reflog with pruned commits.

Jakub Narebski <jnareb@gmail.com> wrote:
Show 5 quoted lines
> I still think it is a good idea to allow user (experienced user)
> to set to not consider reflog for saving. Especially that there
> exist repositories which have reflogs with long pruned commits,
> and it would be nice to preserve the reflog info, even if some of
> information is not available.

In my humble opinion there is *no* value in a reflog whose commits are "corrupt" (due to missing commit object or missing tree/blob it references) as you cannot get that commit back. And its hard to get any other data from it.

My local git.git repository got killed during a repack because I was missing 1 blob (an old version of builtin-blame.c) from Junio's pu branch. Apparently I had a reflog on that tracking branch which went back pretty far and although the commits in that log were still available, the blob wasn't.

-- 
Shawn.
Shawn Pearce· Dec 22, 2006, 00:56 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 3/3] Don't crash during repack of a reflog with pruned commits.

"Shawn O. Pearce" <spearce@spearce.org> wrote:
Show 11 quoted lines
> If the user has been using reflog for a long time (e.g. since its
> introduction) then it is very likely that an existing branch's
> reflog may still mention commits which have long since been pruned
> out of the repository.
> 
> Rather than aborting with a very useless error message during
> git-repack, pack as many valid commits as we can get from the
> reflog and let the user know that the branch's reflog contains
> already pruned commits.  A future 'git reflog expire' (or whatever
> it finally winds up being called) can then be performed to expunge
> those reflog entries.

If its not obvious from the patch, this doesn't entirely fix the problem.

Just because the commit has not been pruned does not mean that a blob or tree referenced by that commit has not been pruned. Right now I am missing 1 blob and cannot repack because of it.

At least with this series of 3 patches the error message resulting from this one missing blob is clearer.

-- 
Shawn.
Junio C Hamano· Dec 22, 2006, 08:16 UTC · re: Shawn Pearce · lore

Re: [PATCH 3/3] Don't crash during repack of a reflog with pruned commits.

Shawn Pearce <spearce@spearce.org> writes:
> Just because the commit has not been pruned does not mean that a blob
> or tree referenced by that commit has not been pruned.
True.  How about this?
---
 builtin-reflog.c |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 40 insertions(+), 3 deletions(-)
Show changes to builtin-reflog.c +40 −3
diff --git a/builtin-reflog.c b/builtin-reflog.c
index d4f7353..4097c32 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -3,7 +3,7 @@
 #include "commit.h"
 #include "refs.h"
 #include "dir.h"
-#include <time.h>
+#include "tree-walk.h"
 
 struct expire_reflog_cb {
 	FILE *newlog;
@@ -13,13 +13,50 @@ struct expire_reflog_cb {
 	unsigned long expire_unreachable;
 };
 
+static int tree_is_complete(const unsigned char *sha1)
+{
+	struct tree_desc desc;
+	void *buf;
+	char type[20];
+
+	buf = read_sha1_file(sha1, type, &desc.size);
+	if (!buf)
+		return 0;
+	desc.buf = buf;
+	while (desc.size) {
+		const unsigned char *elem;
+		const char *name;
+		unsigned mode;
+
+		elem = tree_entry_extract(&desc, &name, &mode);
+		if (!has_sha1_file(elem) ||
+		    (S_ISDIR(mode) && !tree_is_complete(elem))) {
+			free(buf);
+			return 0;
+		}
+		update_tree_entry(&desc);
+	}
+	free(buf);
+	return 1;
+}
+
 static int keep_entry(struct commit **it, unsigned char *sha1)
 {
+	struct commit *commit;
+
 	*it = NULL;
 	if (is_null_sha1(sha1))
 		return 1;
-	*it = lookup_commit_reference_gently(sha1, 1);
-	return (*it != NULL);
+	commit = lookup_commit_reference_gently(sha1, 1);
+	if (!commit)
+		return 0;
+
+	/* Make sure everything in this commit exists. */
+	parse_object(commit->object.sha1);
+	if (!tree_is_complete(commit->tree->object.sha1))
+		return 0;
+	*it = commit;
+	return 1;
 }
 
 static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
Shawn Pearce· Dec 22, 2006, 08:22 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] Don't crash during repack of a reflog with pruned commits.

Junio C Hamano <junkio@cox.net> wrote:
Show 6 quoted lines
> Shawn Pearce <spearce@spearce.org> writes:
> 
> > Just because the commit has not been pruned does not mean that a blob
> > or tree referenced by that commit has not been pruned.
> 
> True.  How about this?
Yea, that's a good way to deal with it.  Ack'd.
-- 
Shawn.

← back to recent threads