Re: print errors from git-update-ref

From: Junio C Hamano <junkio@cox.net>
Date: 2006-07-28 17:26:12
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.

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Received on Fri Jul 28 17:26:51 2006

This archive was generated by hypermail 2.1.8 : 2006-07-28 17:27:21 EST