From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-4.1 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id DA5601FBEC for ; Tue, 7 Mar 2017 14:48:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932744AbdCGOsk (ORCPT ); Tue, 7 Mar 2017 09:48:40 -0500 Received: from cloud.peff.net ([104.130.231.41]:39820 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755773AbdCGOsf (ORCPT ); Tue, 7 Mar 2017 09:48:35 -0500 Received: (qmail 20861 invoked by uid 109); 7 Mar 2017 13:50:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.84) with SMTP; Tue, 07 Mar 2017 13:50:15 +0000 Received: (qmail 25721 invoked by uid 111); 7 Mar 2017 13:50:24 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.84) with SMTP; Tue, 07 Mar 2017 08:50:24 -0500 Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 07 Mar 2017 08:50:13 -0500 Date: Tue, 7 Mar 2017 08:50:13 -0500 From: Jeff King To: Horst Schirmeier Cc: git@vger.kernel.org Subject: Re: [PATCH 0/6] deadlock regression in v2.11.0 with failed mkdtemp Message-ID: <20170307135012.fdk6jhhl4zspdk2v@sigill.intra.peff.net> References: <20170307110328.GE7566@quickstop.soohrt.org> <20170307111406.GB1955@quickstop.soohrt.org> <20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Tue, Mar 07, 2017 at 08:34:37AM -0500, Jeff King wrote: > Yuck. In the original, the error was generated by the child index-pack, > and we relayed it over the sideband. But we don't even get as far as > running index-pack in the newer version; we fail trying to make the > tmpdir. The error ends up in the "unpack" protocol field, but the client > side does a bad job of showing that. With the rest of the patches, it > looks like: > > $ git push ~/tmp/foo.git HEAD > Counting objects: 210973, done. > Delta compression using up to 8 threads. > Compressing objects: 100% (52799/52799), done. > error: remote unpack failed: unable to create temporary object directory > error: failed to push some refs to '/home/peff/tmp/foo.git' There are two other options here I should mention. One is that when pack-objects dies, we suppress the ref-status table entirely. Which is fair, because it doesn't have anything interesting to say. But for a case like this where the other side stops reading our pack early but still produces status reports, we could actually read them all and have a status table like: $ git push ~/tmp/foo.git HEAD Counting objects: 209843, done. Delta compression using up to 8 threads. Compressing objects: 100% (52186/52186), done. error: remote unpack failed: unable to create temporary object directory To /home/peff/tmp/foo.git ! [remote rejected] HEAD -> jk/push-index-pack-deadlock (unpacker error) error: failed to push some refs to '/home/peff/tmp/foo.git' We'd have to take some care to make sure we handle the case when the remote _doesn't_ manage to give us the status (e.g., when there's a complete hangup). I don't know if it's worth the effort. There's no new information there. The second thing is that I think the design of the "unpack " report is a bit dated. These days everybody supports the sideband protocol, so it would probably make more sense to just issue the "" report via the sideband (at which point it gets a nice "remote: " prefix). We could do something like this: diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index f2c6953a3..6204d3d00 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1670,6 +1670,8 @@ static const char *unpack(int err_fd, struct shallow_info *si) if (!tmp_objdir) { if (err_fd > 0) close(err_fd); + rp_error("unable to create temporary object directory: %s", + strerror(errno)); return "unable to create temporary object directory"; } child.env = tmp_objdir_env(tmp_objdir); and drop the "try to show the unpack failure" parts of my series, and you'd end up with: $ git push ~/tmp/foo.git HEAD Counting objects: 209843, done. Delta compression using up to 8 threads. Compressing objects: 100% (52186/52186), done. remote: error: unable to create temporary object directory: Permission denied error: failed to push some refs to '/home/peff/tmp/foo.git' but in cases where pack-objects _doesn't_ fail, existing git versions do show the "unpack" error. So you'd see it twice. I don't know if it's worth trying to hack around. -Peff