Shawn Pearce <spearce@xxxxxxxxxxx> writes:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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.
-
: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html