print errors from git-update-ref

print errors from git-update-ref

[Alex Riesen]

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

...otherwise it not clear what happened when update-ref fails.

E.g., git checkout -b a/b/c HEAD would print nothing if refs/heads/a
exists and is a directory (it does return 1, so scripts checking for
return code should be ok).

I'm attaching two patches, because I'm not quite sure where it should
be done: git-checkout is the least intrusive, but only builtin-update-ref.c
has enough info to help user to resolve the problem (errno is ENOTDIR,
which is selfexplanatory). And I happen to use git-update-ref directly
sometimes.

[-- Attachment #2: 0001-update-ref-print-errors-otherwise-it-not-clear-what-happened.txt --]
[-- Type: text/plain, Size: 1049 bytes --]

From 5398f0ee6bab039701912fdaf784792f4cf76afe Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 18 Jul 2006 14:52:15 +0200
Subject: [PATCH] update-ref: print errors

otherwise it not clear what happened

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-update-ref.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index 83094ab..ad4a44d 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -50,10 +50,14 @@ int cmd_update_ref(int argc, const char 
 		die("%s: not a valid old SHA1", oldval);
 
 	lock = lock_any_ref_for_update(refname, oldval ? oldsha1 : NULL, 0);
-	if (!lock)
+	if (!lock) {
+		error("%s: %s", refname, strerror(errno));
 		return 1;
-	if (write_ref_sha1(lock, sha1, msg) < 0)
+        }
+	if (write_ref_sha1(lock, sha1, msg) < 0) {
+		error("%s: %s", refname, strerror(errno));
 		return 1;
+	}
 
 	/* write_ref_sha1 always unlocks the ref, no need to do it explicitly */
 	return 0;
-- 
1.4.2.rc1.g22734


[-- Attachment #3: 0001-git-checkout.sh-print-errors-otherwise-it-is-not-clear-what-happened.txt --]
[-- Type: text/plain, Size: 939 bytes --]

From 7ea3177aec909e333bacccd00693f223997e2613 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 18 Jul 2006 15:10:54 +0200
Subject: [PATCH] git-checkout.sh: print errors, otherwise it is not clear what happened

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 git-checkout.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-checkout.sh b/git-checkout.sh
index 5613bfc..7b335e5 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -198,7 +198,7 @@ if [ "$?" -eq 0 ]; then
 			mkdir -p $(dirname "$GIT_DIR/logs/refs/heads/$newbranch")
 			touch "$GIT_DIR/logs/refs/heads/$newbranch"
 		fi
-		git-update-ref -m "checkout: Created from $new_name" "refs/heads/$newbranch" $new || exit
+		git-update-ref -m "checkout: Created from $new_name" "refs/heads/$newbranch" $new || die "failed to create branch $newbranch"
 		branch="$newbranch"
 	fi
 	[ "$branch" ] &&
-- 
1.4.2.rc1.g22734

Re: print errors from git-update-ref

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> ...otherwise it not clear what happened when update-ref fails.
>
> E.g., git checkout -b a/b/c HEAD would print nothing if refs/heads/a
> exists and is a directory (it does return 1, so scripts checking for
> return code should be ok).
>
> I'm attaching two patches, because I'm not quite sure where it should
> be done: git-checkout is the least intrusive, but only builtin-update-ref.c
> has enough info to help user to resolve the problem (errno is ENOTDIR,
> which is selfexplanatory). And I happen to use git-update-ref directly
> sometimes.

My gut feeling is that complaining from update-ref is fine, but
I am still tired after a long week and not thinking straight, so
I will not be applying this tonight.

git-applypatch, git-am, and git-branch would be helped by
update-ref complaining.

Porcelains?

BTW, I wonder what happens when .git/logs/refs/a is a directory
(by mistake or malice), .git/refs/a does not exist, and the user
does "git checkout -b a/b/c HEAD".  Or when .git/logs/refs/a/b/c
does exist but is not writable.  My preference is just warn but
do not interrupt the primary operation, since ref-log is just an
optional part of the system, but that would probably lead to
confusion, so we might be better off erroring the caller out in
such a case.  Opinions?

git-resolve does not check exit value from update-ref, which is
*BAD*, but we should be deprecating it anyway.

git-reset has the same problem of not checking the exit status
from update ref.  Worse yet, it calls update-ref with wrong
parameter ($@ in its parameter should be $*).

Patches to fix these two and half problems should be trivial but
I won't be doing that myself tonight.

Re: print errors from git-update-ref

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> "Alex Riesen" <raa.lkml@gmail.com> writes:
> 
> > ...otherwise it not clear what happened when update-ref fails.
> >
> > E.g., git checkout -b a/b/c HEAD would print nothing if refs/heads/a
> > exists and is a directory (it does return 1, so scripts checking for
> > return code should be ok).
> 
> My gut feeling is that complaining from update-ref is fine, but
> I am still tired after a long week and not thinking straight, so
> I will not be applying this tonight.

So I looked into this issue tonight.  For starters I can't seem to
reproduce the situtation reported by Alex, and since he didn't
supply new test cases its difficult to actually fix it.

I did however find problems with git-update-ref a/b/c when a is
actually an existing ref.  This didn't report any error, so here's
a fix.  It may resolve Alex's problem - or maybe not.

-->8--
Display an error from update-ref if target ref name is invalid.

Alex Riesen (raa.lkml@gmail.com) recently observed that git branch
would fail with no error message due to unexpected situations with
regards to refs.  For example, if .git/refs/heads/gu is a file but
`git branch -b refs/heads/gu/fixa HEAD` was invoked by the user
it would fail silently due to refs/heads/gu being a file and not
a directory.

This change adds a test for trying to create a ref within a directory
that is actually currently a file, and adds error printing within
the ref locking routine should the resolve operation fail.

The error printing code probably belongs at this level of the library
as other failures within the ref locking, writing and logging code
are also currently at this level of the code.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 refs.c                |   33 +++++++++++++++++++++++++++++++++
 t/t1400-update-ref.sh |   12 ++++++++++++
 2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/refs.c b/refs.c
index 56db394..e99e9e4 100644
--- a/refs.c
+++ b/refs.c
@@ -290,10 +290,33 @@ static struct ref_lock *verify_lock(stru
 	return lock;
 }
 
+static char* not_a_directory (const char *orig_path)
+{
+	char *p = strdup(orig_path);
+	struct stat st;
+
+	do {
+		char * s = strrchr(p, '/');
+		if (s) {
+			*s = 0;
+			if (lstat(p, &st) == 0 && S_ISDIR(st.st_mode)) {
+				*s = '/';
+				break;
+			}
+		} else {
+			strcpy(p, orig_path);
+			break;
+		}
+	} while (errno == ENOTDIR);
+
+	return p;
+}
+
 static struct ref_lock *lock_ref_sha1_basic(const char *path,
 	int plen,
 	const unsigned char *old_sha1, int mustexist)
 {
+	const char *orig_path = path;
 	struct ref_lock *lock;
 	struct stat st;
 
@@ -303,7 +326,17 @@ static struct ref_lock *lock_ref_sha1_ba
 	plen = strlen(path) - plen;
 	path = resolve_ref(path, lock->old_sha1, mustexist);
 	if (!path) {
+		int last_errno = errno;
+		if (errno == ENOTDIR) {
+			char* p = not_a_directory(orig_path);
+			error("unable to resolve reference %s: %s",
+				p, strerror(errno));
+			free(p);
+		} else
+			error("unable to resolve reference %s: %s",
+				orig_path, strerror(errno));
 		unlock_ref(lock);
+		errno = last_errno;
 		return NULL;
 	}
 	lock->lk = xcalloc(1, sizeof(struct lock_file));
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 04fab26..e73827c 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -14,6 +14,8 @@ D=44444444444444444444444444444444444444
 E=5555555555555555555555555555555555555555
 F=6666666666666666666666666666666666666666
 m=refs/heads/master
+n_dir=refs/heads/gu
+n=$n_dir/fixes
 
 test_expect_success \
 	"create $m" \
@@ -26,6 +28,16 @@ test_expect_success \
 rm -f .git/$m
 
 test_expect_success \
+	"fail to create $n" \
+	'touch .git/$n_dir
+	 git-update-ref $n $A >out 2>err
+	 test $? = 1 &&
+	 test "" = "$(cat out)" &&
+	 grep "error: unable to resolve reference" err &&
+	 grep $n_dir err'
+rm -f .git/$n_dir out err
+
+test_expect_success \
 	"create $m (by HEAD)" \
 	'git-update-ref HEAD $A &&
 	 test $A = $(cat .git/$m)'
-- 
1.4.2.rc1.g802da

Re: print errors from git-update-ref

[Johannes Schindelin]

Hi,

On Wed, 26 Jul 2006, Shawn Pearce wrote:

> This change adds a test for trying to create a ref within a directory
> that is actually currently a file, and adds error printing within
> the ref locking routine should the resolve operation fail.

Why not just print an error message when the resolve operation fails, 
instead of special casing this obscure corner case? It is way shorter, 
too. The test should stay, though.

Ciao,
Dscho

Re: print errors from git-update-ref

[Shawn Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
> 
> On Wed, 26 Jul 2006, Shawn Pearce wrote:
> 
> > This change adds a test for trying to create a ref within a directory
> > that is actually currently a file, and adds error printing within
> > the ref locking routine should the resolve operation fail.
> 
> Why not just print an error message when the resolve operation fails, 
> instead of special casing this obscure corner case? It is way shorter, 
> too. The test should stay, though.

Did you read the patch?  If resolve_ref returns NULL then this
change prints an error (from errno) no matter what.  If errno is
ENOTDIR then it tries to figure out what part of the ref path wasn't
a directory (but was attempted to be used as such) and prints an
ENOTDIR error about that path instead of the one actually given
to the ref lock function

So I think I'm doing what you are suggesting...

-- 
Shawn.

Re: print errors from git-update-ref

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> Hi,
>> 
>> On Wed, 26 Jul 2006, Shawn Pearce wrote:
>> 
>> > This change adds a test for trying to create a ref within a directory
>> > that is actually currently a file, and adds error printing within
>> > the ref locking routine should the resolve operation fail.
>> 
>> Why not just print an error message when the resolve operation fails, 
>> instead of special casing this obscure corner case? It is way shorter, 
>> too. The test should stay, though.
>
> Did you read the patch?  If resolve_ref returns NULL then this
> change prints an error (from errno) no matter what.  If errno is
> ENOTDIR then it tries to figure out what part of the ref path wasn't
> a directory (but was attempted to be used as such) and prints an
> ENOTDIR error about that path instead of the one actually given
> to the ref lock function
>
> So I think I'm doing what you are suggesting...

Not quite.

+		int last_errno = errno;
+		if (errno == ENOTDIR) {
+			char* p = not_a_directory(orig_path);
+			error("unable to resolve reference %s: %s",
+				p, strerror(errno));
+			free(p);
+		} else
+			error("unable to resolve reference %s: %s",
+				orig_path, strerror(errno));
 		unlock_ref(lock);
+		errno = last_errno;

I know you are trying to be nice by pinpointing which component
of the directory hierarchy is offending, but at the same time
the nicety is hiding the orig_path given to the program from the
user.  Maybe showing orig_path _and_ p would be nicer.

But I suspect there is even more serious problem here.

 - lock_ref_sha1_basic() gets "path" from the user; you stash it
   away in "orig_path".

 - resolve_ref() tries to resolve both symbolic links and
   symrefs.  If it fails, it returns NULL.

 - When it returns NULL, you use orig_path (say, "a/b/c/d") to
   see which path component is not a directory (say, "a/b/c" is
   a file).

But the last step does not take into account what resolve_ref()
does, doesn't it?  What if orig_path is "HEAD", which is a
symref, which contained "ref: refs/heads/myhack/one" and
"refs/heads/myhack" is a file?  Ideally you may want to say
something like:

     '''while resolving %s, which points at %s,
        we found out %s is not a directory''' %
        ("HEAD", "refs/heads/myhack/one", "refs/heads/myhack")

So I tend to agree with Johannes's "why bother?" reaction.

Re: print errors from git-update-ref

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> Shawn Pearce <spearce@spearce.org> writes:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >> Hi,
> >> 
> >> On Wed, 26 Jul 2006, Shawn Pearce wrote:
> >> 
> >> > This change adds a test for trying to create a ref within a directory
> >> > that is actually currently a file, and adds error printing within
> >> > the ref locking routine should the resolve operation fail.
> >> 
> >> Why not just print an error message when the resolve operation fails, 
> >> instead of special casing this obscure corner case? It is way shorter, 
> >> too. The test should stay, though.
> >
> > Did you read the patch?  If resolve_ref returns NULL then this
> > change prints an error (from errno) no matter what.  If errno is
> > ENOTDIR then it tries to figure out what part of the ref path wasn't
> > a directory (but was attempted to be used as such) and prints an
> > ENOTDIR error about that path instead of the one actually given
> > to the ref lock function
> >
> > So I think I'm doing what you are suggesting...
>
[snip]
> But the last step does not take into account what resolve_ref()
> does, doesn't it?  What if orig_path is "HEAD", which is a
> symref, which contained "ref: refs/heads/myhack/one" and
> "refs/heads/myhack" is a file?  Ideally you may want to say
> something like:
> 
>      '''while resolving %s, which points at %s,
>         we found out %s is not a directory''' %
>         ("HEAD", "refs/heads/myhack/one", "refs/heads/myhack")
> 
> So I tend to agree with Johannes's "why bother?" reaction.

OK, that's a bug.  It would be horribly misleading.  So I'm taking
the shortcut here and just telling the user ENOTDIR and orig_path
rather than resolving it and finding that bad directory component.

-->8--
Display an error from update-ref if target ref name is invalid.

Alex Riesen (raa.lkml@gmail.com) recently observed that git branch
would fail with no error message due to unexpected situations with
regards to refs.  For example, if .git/refs/heads/gu is a file but
`git branch -b refs/heads/gu/fixa HEAD` was invoked by the user
it would fail silently due to refs/heads/gu being a file and not
a directory.

This change adds a test for trying to create a ref within a directory
that is actually currently a file, and adds error printing within
the ref locking routine should the resolve operation fail.

The error printing code probably belongs at this level of the library
as other failures within the ref locking, writing and logging code
are also currently at this level of the code.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 refs.c                |    5 +++++
 t/t1400-update-ref.sh |   12 ++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/refs.c b/refs.c
index 56db394..02850b6 100644
--- a/refs.c
+++ b/refs.c
@@ -294,6 +294,7 @@ static struct ref_lock *lock_ref_sha1_ba
 	int plen,
 	const unsigned char *old_sha1, int mustexist)
 {
+	const char *orig_path = path;
 	struct ref_lock *lock;
 	struct stat st;
 
@@ -303,7 +304,11 @@ static struct ref_lock *lock_ref_sha1_ba
 	plen = strlen(path) - plen;
 	path = resolve_ref(path, lock->old_sha1, mustexist);
 	if (!path) {
+		int last_errno = errno;
+		error("unable to resolve reference %s: %s",
+			orig_path, strerror(errno));
 		unlock_ref(lock);
+		errno = last_errno;
 		return NULL;
 	}
 	lock->lk = xcalloc(1, sizeof(struct lock_file));
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 04fab26..ddc80bb 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -14,6 +14,8 @@ D=44444444444444444444444444444444444444
 E=5555555555555555555555555555555555555555
 F=6666666666666666666666666666666666666666
 m=refs/heads/master
+n_dir=refs/heads/gu
+n=$n_dir/fixes
 
 test_expect_success \
 	"create $m" \
@@ -26,6 +28,16 @@ test_expect_success \
 rm -f .git/$m
 
 test_expect_success \
+	"fail to create $n" \
+	'touch .git/$n_dir
+	 git-update-ref $n $A >out 2>err
+	 test $? = 1 &&
+	 test "" = "$(cat out)" &&
+	 grep "error: unable to resolve reference" err &&
+	 grep $n err'
+rm -f .git/$n_dir out err
+
+test_expect_success \
 	"create $m (by HEAD)" \
 	'git-update-ref HEAD $A &&
 	 test $A = $(cat .git/$m)'
-- 
1.4.2.rc1.g802da