Re: [PATCH v5 5/5] clean: improve performance when removing lots of directories

From: erik elfström <erik.elfstrom@gmail.com>
Date: Apr 28, 2015, 20:31 UTC
Message-ID: <CAMpP7Nbm9Gy6AgULBCCwgeEUBWU4M5r8+SC22iuu8VmaUtst_w@mail.gmail.com>
In-Reply-To: <20150428062422.GM24580@peff.net>

On Tue, Apr 28, 2015 at 8:24 AM, Jeff King <peff@peff.net> wrote:

Show 8 quoted lines

>
> This iteration looks reasonable overall to me.
>
> Should this is_git_repository() helper be available to other files? I
> think there are other calls to resolve_gitlink_ref() that would want the
> same treatment (e.g., I think "git status" may have a similar issue).
>
> -Peff

Yes that is probably the direction to go in but I think the current version is too tailored to the clean case to be generally useful (although I haven't check the status case). Right now this is more of a is_this_a_repo_clean_should_care_about_ignoring_some_cornercase_quirks than a true is_git_repository check.

I would think a general version would look more like this:

static int is_git_repository(struct strbuf *path)
{
    if (is_bare_repository(path))
        return BARE_REPO;
    if (is_submodule(path))
        return SUBMODULE;
    if (is_git_director(path))
        return REPO;
    return 0;
}

probably also communicating any errors back to the caller.

To sum it up I think you are correct in the direction but I suspect that the current version is not sufficient for the general case and that it would be best to leave the generalization and making it public for future work.

/Erik