Re: print errors from git-update-ref

From: Junio C Hamano <junkio@cox.net>
Date: Jul 28, 2006, 07:26 UTC
Message-ID: <7vejw6mbqj.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20060728062720.GC30783@spearce.org>

Shawn Pearce <spearce@spearce.org> writes:

Show 21 quoted lines

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