git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Martin Fick <mfick@codeaurora.org>
Subject: Re: [PATCH] builtin/describe: introduce --broken flag
Date: Tue, 21 Mar 2017 15:27:56 -0700
Message-ID: <CAGZ79kbjsm1p+Ag5Q8fii3ncbxSsVLYRwGP=Va=btx8Tfy3aOA@mail.gmail.com> (raw)
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:
> 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

> 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

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-21  0:11 [PATCH 0/3] git-describe deals gracefully with broken submodules Stefan Beller
2017-03-21  0:11 ` [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2 Stefan Beller
2017-03-21  0:11 ` [PATCH 2/3] revision machinery: gentle submodule errors Stefan Beller
2017-03-21  0:11 ` [PATCH 3/3] builtin/describe: introduce --submodule-error-as-dirty flag Stefan Beller
2017-03-21  6:28 ` [PATCH 0/3] git-describe deals gracefully with broken submodules Junio C Hamano
2017-03-21  6:54   ` Junio C Hamano
2017-03-21 18:51     ` [PATCH] builtin/describe: introduce --broken flag Stefan Beller
2017-03-21 21:51       ` Junio C Hamano
2017-03-21 22:27         ` Stefan Beller [this message]
2017-03-21 22:41           ` Junio C Hamano
2017-03-21 22:50             ` Stefan Beller
2017-03-21 22:57               ` [PATCH v2] " Stefan Beller
2017-03-22 17:21                 ` Junio C Hamano
2017-03-22 21:50                 ` Jakub Narębski
2017-03-21 17:46   ` [PATCH 0/3] git-describe deals gracefully with broken submodules Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGZ79kbjsm1p+Ag5Q8fii3ncbxSsVLYRwGP=Va=btx8Tfy3aOA@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=mfick@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox