On Tue, Sep 02, 2008 at 11:38:38AM -0700, Junio C Hamano wrote: > (1) Addition of strerror(errno) is a good thing, but it is a separate > topic; Yes indeed. There are many more places that lack proper error reporting. I wonder if we should introduce some wrapper functions like mkdir_or_die, so the caller doesn't have check for errors. Such a function could (optionally?) create leading directories as well. > (2) I always thought that it was a clever feature to allow callers that > would want to prepare a directory in advance to ask for "xyzzy/" and > cause the whole path created. You are breaking it, which may or may > not be a bad thing per-se, because I do not think any existing caller > depends on this behaviour; Yes, I was afraid of that. So I checked all calls to c_l_d and it's not used that way anywhere. > (3) If you *are* to break that feature, then I think you should also > handle a user input that is broken in the same fashion as your clone > example, namely, "git clone <repo> path//". It does not make much > senseto say "path/" as the last parameter to clone is not a user > error but "path//" is. True enough. > As a "bugfix" patch meant to apply to 'maint', I'd prefer a fix to the > caller (builtin-clone.c that calls the function), which should be of much > less impact. It is fine to include the change to add strerror(errno) in > that patch, whose title would be "clone: fix creation of explicitly named > target directory". Unfortunately, if we simply add strerror to the error message, in place of fatal: could not create work tree dir 'path/'. the new version would print fatal: could not create work tree dir 'path/': File exists. which makes things worse IMO. We could of course strip trailing slashes in builtin-clone.c for now and revert that as soon as the cleanup patch is in, but I think it's not worth the trouble. I suggest we live with the "bug" for now. The error reporting cleanups should be done at a greater scope anyways. > > - if (!pos) > > + if (!pos || !*(pos + 1)) > > (minor nit) I think > > if (!pos || !pos[1]) > > is shorter and easier on the eye. Will be fixed in the patch to follow. Clemens -- To unsubscribe from this list: 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
