Re: [PATCH] builtin/describe: introduce --broken flag

From: Stefan Beller <sbeller@google.com>
Date: Mar 21, 2017, 22:27 UTC
Message-ID: <CAGZ79kbjsm1p+Ag5Q8fii3ncbxSsVLYRwGP=Va=btx8Tfy3aOA@mail.gmail.com>
In-Reply-To: <xmqq7f3invr6.fsf@gitster.mtv.corp.google.com>

On Tue, Mar 21, 2017 at 2:51 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> This patch helps to fix the root cause in [1], which tries to work around
>> this situation.
>
> I do not necessarily think it is reasonable to give $version-dirty
> and proceed when a repository corruption is detected; if there is a
> breakage in the repository, "git describe" is correct to die when a
> populated submodule is broken.  IOW, I do not agree that [1] below
> is working with a sensible expectation.

ok, so I won't quote it in the commit message

>
> This is a tangent, but how does the Gerrit repository get corrupted
> in the way described in [1] in the first place?  That might be what
> needs to be corrected, perhaps?

AFAICT, someone is (was?) using a version of Git that doesn't contain f8eaa0ba98 (submodule--helper, module_clone: always operate on absolute paths, 2016-03-31). So then the submodule paths were made absolute paths on creation of the Gerrit repo.

And then someone moved the repo and the absolute paths broke.

Even after an upgrade of Git to its latest and greatest version, the underlying issue of having broken submodule paths remains in that case.

So there are a couple of ways forward
0) as an immediate fix, manually fix the absolute path or make them relative

1A) have more error resilient tools in Git
1B) have a tool in git (e.g. "git submodule fsck-setup") that rewrites
    the .git file link and the core.worktree setting to be relative and correct.

I think we should do both A and B, I decided to go with A first, specifically "git-describe" as that was reported to not work well in this situation with the given broken data

Show 11 quoted lines

> The wording for the "--dirty" is already awkward, but this one is
> even more so ("Describe the working tree. It means" conveys no
> useful information).  I however cannot come up with something much
> better.  This is the best I could come up with:
>
>         Describe the state of the working tree.  When the working
>         tree matches HEAD, the output is the same as "git describe
>         HEAD" and "-dirty" is appended to it if the working tree has
>         local modification.  When a repository is corrupt and Git
>         cannot determine if there is local modification, instead of
>         dying, append "-broken" instead.

ok, I'll reuse parts of it.

>> +static const char *append, *dirty, *broken;
>
> Perhaps call it "suffix" or something?

done

>> +                     argv_array_pushl(&args, "diff-index", "--quiet", "HEAD", "--", NULL);

..

>  Wouldn't argv_array_pushv() into these two different args
> array from the same template work better?

yes, looks much saner. Thanks