All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 0/5] parse_commit cleanups
Date: Tue, 8 Oct 2013 09:48:43 -0400	[thread overview]
Message-ID: <20131008134843.GA7530@sigill.intra.peff.net> (raw)
In-Reply-To: <20131006044856.GA12301@sigill.intra.peff.net>

On Sun, Oct 06, 2013 at 12:48:56AM -0400, Jeff King wrote:

> Instead of a segfault, let's print an error message and die
> a little more gracefully.
> [...]
> ---
> Not a huge deal, since we are terminating the program either way. There
> are other places in the code with a bare parse_commit that could
> probably use the same treatment. I didn't investigate them, but they
> could easily build on the parse_commit_or_die here if somebody wants to
> follow up.

Here are some follow-up patches that go on top. The first two are noop
cleanups. The third and fourth are not strictly noops, but are minor
behavior changes that should be strict improvements.

There are a number of unchecked parse_commit calls whose fallout is more
complicated. In many cases, we want to ignore an error (keeping in mind
that parse_commit will already have printed a message to stderr) and
keep going, in order to allow users to to get as much done as they can
in a broken repository.

The fifth patch deals with one of these cases. There are 8 more
unchecked calls after this series, some of which may want to die(), or
may want to be left alone; I didn't investigate deeply.

There are also some cases where we do not die, but perhaps should. In
general, I think it makes sense to let fetch-pack and rev-list work
around broken history, but probably not things like blame, which would
then provide subtly wrong answers. Still, I doubt it's a big deal in
practice, since corrupted repositories are relatively rare (and the
message to stderr does inform the user that something is wrong).

  [1/5]: assume parse_commit checks commit->object.parsed
  [2/5]: assume parse_commit checks for NULL commit
  [3/5]: use parse_commit_or_die instead of segfaulting
  [4/5]: use parse_commit_or_die instead of custom message
  [5/5]: checkout: do not die when leaving broken detached HEAD

-Peff

  reply	other threads:[~2013-10-09 16:37 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-06  4:48 [PATCH] log_tree_diff: die when we fail to parse a commit Jeff King
2013-10-08 13:48 ` Jeff King [this message]
2013-10-08 13:52   ` [PATCH 1/5] assume parse_commit checks commit->object.parsed Jeff King
2013-10-08 13:56   ` [PATCH 2/5] assume parse_commit checks for NULL commit Jeff King
2013-10-08 13:57   ` [PATCH 3/5] use parse_commit_or_die instead of segfaulting Jeff King
2013-10-08 13:57   ` [PATCH 4/5] use parse_commit_or_die instead of custom message Jeff King
2013-10-08 14:00   ` [PATCH 5/5] checkout: do not die when leaving broken detached HEAD Jeff King

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

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

  git send-email \
    --in-reply-to=20131008134843.GA7530@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.