Should git-prune-script warn about dircache?

Should git-prune-script warn about dircache?

[Junio C Hamano]

When the user has a change that is recorded in the dircache but
the change has not been committed, git-fsck-cache --unreachable
would report blobs and trees (if you have a subdirectory) that
are involved.

    $ git-init-db
    defaulting to local storage area
    $ date >foo
    $ git-update-cache --add foo
    $ date | git-commit-tree $(git-write-tree) >.git/HEAD
    Committing initial tree 9061076c3ec10fef339c84f4e0c9e06576d3b7db
    $ date >foo
    $ git-update-cache foo
    $ git-fsck-cache --unreachable $(cat .git/HEAD)
    unreachable blob d8c61e975a591ad5905c7defc567a77fca58f092
    $ git-ls-files --stage
    100644 d8c61e975a591ad5905c7defc567a77fca58f092 0 foo

At this point, if we run git-prune-script with the current head,
we would remove the blob, and after that, code that assumes the
objects pointed by SHA1s recorded in the dircache are locally
available start failing.

    $ git-prune-script
    $ touch foo
    $ git-diff-files -p
    .git/objects/d8/c61e975a591....: No such file or directory
    fatal: unable to read blob object for foo (d8c61e975a591....)

I have a feeling somewhere in the chain from git-prune-script we
should have a mechanism to prevent us from losing such blob and
trees.  I know it should not be in git-fsck-cache, but I have
not decided what my suggestions would be for us to do yet.

Maybe a big warning in red ugly bold blinking typeface somewhere
in the doc?

Re: Should git-prune-script warn about dircache?

[Linus Torvalds]

On Sun, 1 May 2005, Junio C Hamano wrote:
> 
> Maybe a big warning in red ugly bold blinking typeface somewhere
> in the doc?

How about making git-prune-script first run "git-update-cache --refresh",
and checking the return value of it (this, of course, assumes that
git-update-cache --refresh would return non-zero if it can't refresh a
file, which is currently not true, but should be easily fixable).

		Linus

Re: Should git-prune-script warn about dircache?

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> How about making git-prune-script first run "git-update-cache --refresh",
LT> and checking the return value of it (this, of course, assumes that
LT> git-update-cache --refresh would return non-zero if it can't refresh a
LT> file, which is currently not true, but should be easily fixable).

Or just check if it sees anything in the output, especially
"needs update" line.

I do not think it is such a big deal [*1*] but I should point
out that, "git-update-cache --refresh" needs to be run on all of
them if the user (or the porcelain layer) is using more than one
GIT_INDEX_FILEs [*2*].


[Footnotes]

*1* Because git-prune-script is just an example and it already
assumes it knows where the valid heads are; right now it looks
only at .git/HEAD and not .git/refs/*/*.  Each Porcelain layer
implementation should provide its own prune script anyway.

*2* I do not do this anymore but an earlier incarnation of my
little SCM on GIT [*3*] allowed a user to keep snapshots of work
tree state and switch between them by juggling multiple
GIT_INDEX_FILE.  I just create commits off of the current state
when making a snapshot in the latest version so it is not a
problem anymore for me.

*3* (PLUG) found in http://members.cox.net/junkio/.

Re: Should git-prune-script warn about dircache?

[Junio C Hamano]

>>>>> "JCH" == Junio C Hamano <junkio@cox.net> writes:

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:
LT> How about making git-prune-script first run "git-update-cache --refresh",
LT> and checking the return value of it (this, of course, assumes that
LT> git-update-cache --refresh would return non-zero if it can't refresh a
LT> file, which is currently not true, but should be easily fixable).

JCH> Or just check if it sees anything in the output, especially
JCH> "needs update" line.

Well, we were both wrong.  The problem is not about the work
tree changes since the last git-update-cache, but about the
blobs recorded in the cache but still not committed.

I think we should do something like this.

    git-ls-files --cached | "sed to SHA1 only" | sort >,,1
    git-fsck-cache --unreachable | "sed to SHA1 only" | sort >,,2
    comm -13 ,,1 ,,2 | "sed to .git/object/ path" | xargs -r rm -f

[PATCH] Make git-prune-script a bit more careful.

[Junio C Hamano]

As discussed on the git list, this patch makes the sample script
git-prune-script a bit more careful by keeping the objects
referenced from the current cache from being reclaimed.

At the same time it also fixes and enhances the following:

 - Optional command line parameters can specify which commit
   heads to start reachablity test from.  Earlier we had a
   hardcoded .git/HEAD.  It now defaults to the contents of
   .git/HEAD and .git/refs/*/* files.

 - It does not assume SHA1_FILE_DIRECTORY is .git/objects/ but
   uses the value from the environment if there is one.

 - It runs xargs with "-r" option to prevent it from making "rm"
   command barf if there is nothing to remove.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

git-prune-script |   32 +++++++++++++++++++++++++++++++-
1 files changed, 31 insertions(+), 1 deletion(-)

# - date handling: handle "AM"/"PM" on time
# + [PATCH] Make git-prune-script a bit more careful.
--- k/git-prune-script
+++ l/git-prune-script
@@ -1,2 +1,32 @@
 #!/bin/sh
-git-fsck-cache --unreachable $(cat .git/HEAD ) | grep unreachable | cut -d' ' -f3 | sed 's:^\(..\):.git/objects/\1/:' | xargs rm
+
+tmp=.git-prune-script-$$
+trap 'rm -f $tmp-*' 0 1 2 3 15 
+
+# Defaulting to include .git/refs/*/* may be debatable from the
+# purist POV but power users can always give explicit parameters
+# to the script anyway.
+case "$#" in
+0) set x $(cat .git/HEAD .git/refs/*/*); shift ;;
+esac
+
+git-fsck-cache --unreachable "$@" |
+sed -ne 's/unreachable [^ ][^ ]* //p' |
+sort >$tmp-unreachable
+
+# This makes extra objects to be kept if the cache has an entry
+# with an unusual name like "this\n0 0123...abcdef 0 file", but
+# we are trying not to discard information and keeping extra in
+# an unusual situation would be OK.
+git-ls-files --stage |
+sed -ne 's|^[0-7][0-7]* \([0-9a-f][0-9a-f]*\) [0-3] .*|\1|p' |
+sort >$tmp-keep
+
+comm -23 $tmp-unreachable $tmp-keep |
+sed -e 's|\(..\)|\1/|' | {
+	case "$SHA1_FILE_DIRECTORY" in
+	'') cd .git/objects/ ;;
+	*) cd "$SHA1_FILE_DIRECTORY" ;;
+	esac || exit
+	xargs -r echo rm -f
+}

[PATCH] Make git-update-cache --refresh fail if update/merge needed.

[Junio C Hamano]

Scripts may find it useful if they do not have to parse the
output from the command but just can rely on its exit status.

Earlier both Linus and myself thought this would be necessary to
make git-prune-script safer but it turns out that the issue was
somewhere else and not related to what this patch addresses.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

update-cache.c |   15 ++++++++++-----
1 files changed, 10 insertions(+), 5 deletions(-)

# - date handling: handle "AM"/"PM" on time
# + [PATCH] Make git-prune-script a bit more careful.
--- k/update-cache.c
+++ l/update-cache.c
@@ -212,15 +212,17 @@ static struct cache_entry *refresh_entry
 	return updated;
 }
 
-static void refresh_cache(void)
+static int refresh_cache(void)
 {
 	int i;
+	int has_errors = 0;
 
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce, *new;
 		ce = active_cache[i];
 		if (ce_stage(ce)) {
 			printf("%s: needs merge\n", ce->name);
+			has_errors = 1;
 			while ((i < active_nr) &&
 			       ! strcmp(active_cache[i]->name, ce->name))
 				i++;
@@ -230,12 +232,15 @@ static void refresh_cache(void)
 
 		new = refresh_entry(ce);
 		if (IS_ERR(new)) {
-			if (!(not_new && PTR_ERR(new) == -ENOENT))
+			if (!(not_new && PTR_ERR(new) == -ENOENT)) {
 				printf("%s: needs update\n", ce->name);
+				has_errors = 1;
+			}
 			continue;
 		}
 		active_cache[i] = new;
 	}
+	return has_errors;
 }
 
 /*
@@ -307,7 +312,7 @@ static void remove_lock_file_on_signal(i
 
 int main(int argc, char **argv)
 {
-	int i, newfd, entries;
+	int i, newfd, entries, has_errors = 0;
 	int allow_options = 1;
 	static char lockfile[MAXPATHLEN+1];
 	const char *indexfile = get_index_file();
@@ -343,7 +348,7 @@ int main(int argc, char **argv)
 				continue;
 			}
 			if (!strcmp(path, "--refresh")) {
-				refresh_cache();
+				has_errors |= refresh_cache();
 				continue;
 			}
 			if (!strcmp(path, "--cacheinfo")) {
@@ -369,5 +374,5 @@ int main(int argc, char **argv)
 		die("Unable to write new cachefile");
 
 	lockfile_name = NULL;
-	return 0;
+	return has_errors;
 }