Re: [PATCH 0/3] git-describe deals gracefully with broken submodules

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> Our own version generation in GIT-VERSION-GEN is somewhat sane by testing
> if we have a .git dir, and use that as a signal whether the obtained
> copy of git was obtained using git (clone/fetch) or if it is just a
> downloaded tar ball.
>
> Other scripts to generate a version are not as cautious and just run
> "git describe". An error from git-describe is treated as a sufficient
> signal to assume it is not a git repository.
>
> When submodules come into play, this is not true, as a submodule
> may be damaged instead, such that we're still in a git repository
> but error out for the sake of reporting a severly broken submodule.
>
> Add a flag to git-describe that instructs it to treat severe submodule
> errors as "dirty" instead of erroring out.

I do not have a strong preference for or against the "treat a broken
repository as if nothing is wrong with the revision, but just mark
it as dirty" idea.  I would be more receptive if it substituted the
"-dirty" marker with something else, e.g. "-broken", though.

My knee-jerk reaction to the code change is that treating submodule
as something very special is probably not a good idea.  Even if you
do not use submodules, if some of the objects referenced from your
index and/or HEAD are damaged or otherwise causes some error while
accessing, the diff machinery would die, wouldn't it?

I saw that some new symbolic constants in the code to tell the
machinery to "gracefully die" (or "hide the breakage under the rug")
are named with SUBMODULE in them, which is probably a bad sign that
the design is being too centric to submodules.  The implementation
that covers only breakages in submodule as its first step may be OK
(you have to start somewhere, after all), but I think the aspiration
should be to cover all kinds of breakages in the end and turn them
to be "graceful", and if you had that goal in mind, you wouldn't be
naming these constants with SUBMODULE in them.

If "treat a broken repository as just being 'dirty'" were a good
idea, I'd suspect that we would want to see all breakages, not just
ones related to submodules, to be treated the same way.  

But it is possible there may be a reason why submodules are special.
I do not think the third paragraph quoted above is a good
justification.  A repository with broken submodule is just as broken
and untrustworthy as a broken repository without a submodule, and if
you want to allow such a checkout with broken submodule to call
itself v2.0-dirty, you would also want to allow a broken checkout
without any submodule to do so, too.