git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: jrnieder@gmail.com, git@vger.kernel.org, mfick@codeaurora.org
Subject: Re: [PATCH 0/3] git-describe deals gracefully with broken submodules
Date: Mon, 20 Mar 2017 23:28:27 -0700
Message-ID: <xmqq7f3jta7o.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170321001156.21915-1-sbeller@google.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.


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-21  0:11 Stefan Beller
2017-03-21  6:28 ` Junio C Hamano [this message]
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
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=xmqq7f3jta7o.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=mfick@codeaurora.org \
    --cc=sbeller@google.com \
    /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