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

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

[Shawn O. Pearce]

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

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

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

[Shawn O. Pearce]

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

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

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

[Junio C Hamano]

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

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

[Shawn Pearce]

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> 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.

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

[Shawn Pearce]

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

-- 
Shawn.

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

[Jakub Narebski]

Shawn Pearce wrote:

> 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

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

[Shawn Pearce]

Jakub Narebski <jnareb@gmail.com> wrote:
> 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.

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

[Junio C Hamano]

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

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,

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

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> 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.