Re: [PATCH] clone: error specifically with --local and symlinked objects

[Taylor Blau <me@xxxxxxxxxxxx>]

On Wed, Apr 05, 2023 at 09:48:22AM -0700, Glen Choo wrote:
> Since this is an error code path, I think the extra lstat() is probably
> worth it since it lets us be really specific about the error. Maybe:

FWIW, I don't totally think it's necessary for us to report back to the
user that they're trying to clone a repository with `--local` whose
`$GIT_DIR/objects` either is or contains a symbolic link. Especially so
since you're already updating the documentation here.

But I think it's a compelling argument in the name of improving the user
experience, so I think that this is a reasonable direction. And I agree,
that while the extra lstat() is unfortunate, I don't think there is a
convenient way to do it otherwise.

We *could* teach the dir-iterator API to return a specialized error code either
through a pointer, like:

    struct dir_iterator *dir_iterator_begin(const char *path,
                                            unsigned int flags, int *error)

and set error to something like -DIR_ITERATOR_IS_SYMLINK when error is
non-NULL.

Or we could do something like this:

    int dir_iterator_begin(struct dir_iterator **it,
                           const char *path, unsigned int flags)

and have the `dir_iterator_begin()` function return its specialized
error and initialize the dir iterator through a pointer. Between these
two, I prefer the latter, but I think it's up to individual taste.

> 	if (!iter) {
> 		struct stat st;
>
>     if (errno == ENOTDIR && lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))

Couple of nit-picks here. Instead of writing "lstat(src->buf, &st ==
0)", we should write "!lstat(src->buf, &st)" to match
Documentation/CodingGuidelines.

But note that that call to lstat() will clobber your errno value, so
you'd want to save it off beforehand. If you end up going this route,
I'd probably do something like:

    if (!iter) {
      int saved_errno = errno;
      if (errno == ENOTDIR) {
        struct stat st;

        if (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))
          die(_("'%s' is a symlink, refusing to clone with `--local`"),
              src->buf);
      }
      errno = saved_errno;

      die_errno(_("failed to start iterator over '%s'"), src->buf);
    }

Thanks,
Taylor