git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Ramsay Jones <ramsay@ramsayjones.plus.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Jason@zx2c4.com, GIT Mailing-list <git@vger.kernel.org>
Subject: Re: [PATCH] fsck: check skiplist for object in fsck_blob()
Date: Fri, 13 Jul 2018 15:38:00 -0400	[thread overview]
Message-ID: <20180713193759.GB10354@sigill.intra.peff.net> (raw)
In-Reply-To: <2ad1b00c-70ff-c4b2-8cbc-9ef55c174221@ramsayjones.plus.com>

On Wed, Jul 11, 2018 at 08:31:34PM +0100, Ramsay Jones wrote:

> >> Simply, I have found (for many different reasons) that, if there
> >> is no good reason to execute some code, it is _far_ better to not
> >> do so! ;-)
> > 
> > Heh. I also agree with that as a guiding principle. But I _also_ like
> > the principle of "if you do not need to do add this code, do not add
> > it". So the two are a little at odds here. :)
> 
> I agree with that also! ;-) However, in this case, I can't
> imagine having to do less, to do nothing - if you see what
> I mean! So, I think "don't execute code you don't need to"
> trumps "don't add code you don't need to" here.

Fair enough. I'm OK with it either way, then.

> > @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
> >  	FUNC(NUL_IN_COMMIT, WARN) \
> >  	/* infos (reported as warnings, but ignored by default) */ \
> >  	FUNC(BAD_TAG_NAME, INFO) \
> > +	FUNC(GITMODULES_PARSE, INFO) \
> >  	FUNC(MISSING_TAGGER_ENTRY, INFO)
> >  
> >  #define MSG_ID(id, msg_type) FSCK_MSG_##id,
> > 
> 
> So, just squinting at this in my email client, if this allowed
> a push/fetch to succeed (along with an 'info' message), while
> providing an admin the means to configure it to loudly deny
> the push/fetch - then I think we have a winner! ;-)
> 
> Sorry for not testing the patch.

No problem. I didn't get back to it until today. And indeed, the patch
works as advertised, but there's one additional bit needed (in the
preparatory patch below).

So here's what I came up with, which I think is pretty reasonable. The
commit message for the second one is quite long, but I tried to lay out
the pros and cons from our discussion. And I think what we discussed
here may end up being the blueprint for how we consider similar cases in
the future, so I tried to be exhaustive.

I built these two on top of my earlier four patches (which Junio has
queued as jk/fsck-gitmodules-gently). The code change itself is
orthogonal to silencing the config code, but I built on top of the test
added earlier. If we want to back-port this to v2.17 or earlier, I can
build it the other way around.

If this is what we decide to do upstream, then I'll lobby to flip
GitHub's defaults to match. Other hosters (especially ones using other
implementations) may consider doing the same.

  [1/2]: fsck: split ".gitmodules too large" error from parse failure
  [2/2]: fsck: downgrade gitmodulesParse default to "info"

 fsck.c                     | 5 +++--
 t/t7415-submodule-names.sh | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

-Peff

  parent reply	other threads:[~2018-07-13 19:38 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-27 18:39 [PATCH] fsck: check skiplist for object in fsck_blob() Ramsay Jones
2018-06-28 11:49 ` Jeff King
2018-06-28 16:39   ` Junio C Hamano
2018-06-28 17:30     ` Jeff King
2018-06-28 16:56   ` Ramsay Jones
2018-06-28 17:28     ` Junio C Hamano
2018-06-28 17:45     ` Jeff King
2018-06-28 18:53       ` Ramsay Jones
2018-06-28 22:03         ` Jeff King
2018-06-28 22:05           ` [PATCH 1/4] config: turn die_on_error into caller-facing enum Jeff King
2018-06-28 22:05           ` [PATCH 2/4] config: add CONFIG_ERROR_SILENT handler Jeff King
2018-06-28 22:05           ` [PATCH 3/4] config: add options parameter to git_config_from_mem Jeff King
2018-06-28 22:06           ` [PATCH 4/4] fsck: silence stderr when parsing .gitmodules Jeff King
2018-06-28 22:12             ` Jeff King
2018-06-29  1:14               ` Ramsay Jones
2018-06-29  1:10           ` [PATCH] fsck: check skiplist for object in fsck_blob() Ramsay Jones
2018-07-03 14:34             ` Jeff King
2018-07-04  0:12               ` Ramsay Jones
2018-07-07  1:32                 ` Jeff King
2018-07-11 19:31                   ` Ramsay Jones
2018-07-13 19:37                     ` Ramsay Jones
2018-07-13 19:41                       ` Jeff King
2018-07-13 19:46                         ` Jeff King
2018-07-13 20:08                           ` Ramsay Jones
2018-07-13 19:38                     ` Jeff King [this message]
2018-07-13 19:39                       ` [PATCH 1/2] fsck: split ".gitmodules too large" error from parse failure Jeff King
2018-07-13 19:39                       ` [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info" Jeff King
2018-07-13 20:21                         ` Stefan Beller
2018-07-16 18:04                         ` Junio C Hamano
2018-07-16 18:30                           ` Jeff King
2018-07-16 21:08                             ` Junio C Hamano

Reply instructions:

You may reply publicly 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=20180713193759.GB10354@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Jason@zx2c4.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ramsay@ramsayjones.plus.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).