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

[Stefan Beller <sbeller@google.com>]

On Mon, Mar 20, 2017 at 11:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

ok. That was my initial plan. I plan on also incorporating this into
git-status.

>
> 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?

Yes; ideally we would want to have a generic "error with working tree"
for all these kinds of errors. Note that the probability of submodule errors
is way higher. (This has a couple of reasons: (1) a submodule has more
complexity than a file, (2) that opens up new areas in which errors can occur
and (3) submodules are less popular so we have fewer
bug reports and people working on it, hence less mature code.

As an example for (2), we had relative/absolute path issues in Git 2.7
for submodules, which is what Martin was complaining about.

> 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.

ok

>
> 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.

agreed.

> I probably would
>
> - introduce a new "git describe --possibly-broken" option;
>
> - instead of running "diff-index" internally to decide the "-dirty"
>   state, spawn "diff-index" as a separate process;
>
> - observe the exit status from "diff-index" and add "-dirty" suffix
>   when it is _known_ to be dirty, add "-broken" suffix when it
>   died, and leave out the suffix when we know that the checkout is
>   clean.
>
> That way, we wouldn't have to contaminate the generic codepath with
> a "treat broken and modified states as if they are the same" logic,
> only to support such a niche feature that we wouldn't want to use
> anywhere else.

I planned on a follow-up that adds similar behavior to git-status,
such that git-status can be used to get the big picture before
diagnosing the individual breakages of a submodule.

That is the reason why I started with the first patch porting
'is_submodule_modified' to porcelain2 of git-status, as that
allows way better reporting on submodules. Sorry for intermixing
these two things.

Thanks,
Stefan