All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: Michael Haggerty <mhagger@alum.mit.edu>,
	Jeff King <peff@peff.net>,
	"git\@vger.kernel.org" <git@vger.kernel.org>,
	Loic Dachary <loic@dachary.org>
Subject: Re: [PATCHv3 6/6] refs.c: enable large transactions
Date: Mon, 26 Jan 2015 19:17:07 -0800	[thread overview]
Message-ID: <xmqqd260nc98.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CAGZ79kbQiVQJyZC8mKaSUnOpY6YJc0TYdX=msuZDXLd7DxmTmQ@mail.gmail.com> (Stefan Beller's message of "Mon, 26 Jan 2015 11:30:56 -0800")

Stefan Beller <sbeller@google.com> writes:

> I do not see the problem in the code itself, but rather in understanding
> the code. I will send a follow up patch which makes it easier to follow
> by removing the early exit with no problem away.


Taken as a whole the code may function correctly but the division of
roles of individual functions seems screwed up.  write_ref_sha1()
sometimes unlocks, and sometimes leaves the unlocking to the caller,
and the caller cannot even tell if it is expected to do the unlocking
for it from the return value because both cases return 0 (success).

I am not sure if it is sensible to call that "correct but hard to
understand".  I'd rather see us admit that its behaviour is screwey
and needs fixing for better code health longer term.

      parent reply	other threads:[~2015-01-27  3:17 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-22  2:32 [PATCHv2 0/5] Fix bug in large transactions Stefan Beller
2015-01-22  2:32 ` [PATCHv2 1/5] update-ref: test handling large transactions properly Stefan Beller
2015-01-22 10:54   ` Michael Haggerty
2015-01-22 13:07     ` Jeff King
2015-01-22  2:32 ` [PATCHv2 2/5] t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE Stefan Beller
2015-01-22  2:32 ` [PATCHv2 3/5] refs.c: remove lock_fd from struct ref_lock Stefan Beller
2015-01-22  2:32 ` [PATCHv2 4/5] refs.c: have a write_sha1_to_lock_file wrapper Stefan Beller
2015-01-22  2:32 ` [PATCHv2 5/5] refs.c: enable large transactions Stefan Beller
2015-01-22 11:24   ` Michael Haggerty
2015-01-22 13:10     ` Jeff King
2015-01-22 16:33       ` Michael Haggerty
2015-01-22 19:24         ` Stefan Beller
2015-01-22 23:11     ` [RFC PATCH 0/5] So you dislike the sequence of system calls? Stefan Beller
2015-01-22 23:11       ` [PATCH 1/5] fixup for "refs.c: enable large transactions" Stefan Beller
2015-01-22 23:11       ` [PATCH 2/5] refs.c: remove unlock_ref from write_ref_sha1 Stefan Beller
2015-01-22 23:11       ` [PATCH 3/5] refs.c: move static functions to close and commit refs Stefan Beller
2015-01-22 23:11       ` [PATCH 4/5] refs.c: remove committing the ref from write_ref_sha1 Stefan Beller
2015-01-22 23:11       ` [PATCH 5/5] refs.c: write values to lock files early for committing Stefan Beller
2015-01-22 12:59   ` [PATCHv2 5/5] refs.c: enable large transactions Ramsay Jones
2015-01-22 19:16     ` Stefan Beller
2015-01-22 19:51       ` Ramsay Jones
2015-01-22 20:13         ` Ramsay Jones
2015-01-22 20:20           ` Stefan Beller
2015-01-22 20:59             ` Ramsay Jones
2015-01-22 12:05 ` [PATCHv2 0/5] Fix bug in " Michael Haggerty
2015-01-23 20:03   ` [PATCHv3 0/6] " Stefan Beller
2015-01-23 20:03     ` [PATCHv3 1/6] update-ref: test handling large transactions properly Stefan Beller
2015-01-23 20:03     ` [PATCHv3 2/6] t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE Stefan Beller
2015-01-23 20:03     ` [PATCHv3 3/6] refs.c: remove lock_fd from struct ref_lock Stefan Beller
2015-01-23 20:04     ` [PATCHv3 4/6] refs.c: move static functions to close and commit refs Stefan Beller
2015-01-23 20:04     ` [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1 Stefan Beller
2015-01-23 23:57       ` Junio C Hamano
2015-01-24  0:22         ` Stefan Beller
2015-01-24  0:39           ` Junio C Hamano
2015-01-24  1:04             ` Stefan Beller
2015-01-24  1:29               ` Junio C Hamano
2015-01-23 20:04     ` [PATCHv3 6/6] refs.c: enable large transactions Stefan Beller
2015-01-24  0:14       ` Junio C Hamano
2015-01-24  0:24         ` Stefan Beller
2015-01-24  0:38           ` Junio C Hamano
2015-01-26 19:30             ` Stefan Beller
2015-01-26 21:10               ` [PATCH] refs.c: clean up write_ref_sha1 returns Stefan Beller
2015-01-27  3:22                 ` Junio C Hamano
2015-01-28 21:35                   ` Stefan Beller
2015-01-27  3:17               ` Junio C Hamano [this message]

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=xmqqd260nc98.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=loic@dachary.org \
    --cc=mhagger@alum.mit.edu \
    --cc=peff@peff.net \
    --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
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.