From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>,
David Turner <dturner@twopensource.com>
Cc: "Jeff King" <peff@peff.net>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Ramsay Jones" <ramsay@ramsayjones.plus.com>,
git@vger.kernel.org
Subject: Re: [PATCH v2 00/33] Yet more preparation for reference backends
Date: Fri, 10 Jun 2016 14:50:31 +0200 [thread overview]
Message-ID: <575AB797.809@alum.mit.edu> (raw)
In-Reply-To: <cover.1462550456.git.mhagger@alum.mit.edu>
On 05/06/2016 06:13 PM, Michael Haggerty wrote:
> [...]
> This patch series is also available from my GitHub repo [2] as branch
> "split-under-lock".
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/292772
> [2] https://github.com/mhagger/git
I was reading this area of the code again, and I found a problem in this
patch series. Now that ref_updates can be added to ref_transactions
while they are being processed, it is not correct to store a pointer to
transaction->updates in ref_transaction_commit() here [1], because the
array might be moved by realloc() if it grows during the function. The
problem wasn't detected during testing because an added commit would
have to cross an ALLOC_GROW boundary to trigger the bug.
The fix is obvious but it is textually quite a few lines. For good
measure, the same fix should be made in initial_ref_transaction_commit()
here [2].
The most logical place to fix this is by expanding commit
"ref_transaction_commit(): remove local variable n" [3], so I've done
that and force pushed the result to my GitHub account [4] as branch
"split-under-lock".
Junio, if you want to incorporate this revised version of the branch in
your big rewind of next, then we can pretend that the bug was never
there :-) Otherwise, tell me in what form you would like the fix and I
will be happy to provide it.
Sorry for finding this problem so late in the process.
Michael
[1]
https://github.com/mhagger/git/blob/088c8f756c86581ff25371983ef409044b348bb9/refs/files-backend.c#L3559
[2]
https://github.com/mhagger/git/blob/088c8f756c86581ff25371983ef409044b348bb9/refs/files-backend.c#L3725
[3] http://article.gmane.org/gmane.comp.version-control.git/293801
[4] https://github.com/mhagger/git
next prev parent reply other threads:[~2016-06-10 12:50 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-06 16:13 [PATCH v2 00/33] Yet more preparation for reference backends Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 01/33] t1404: demonstrate a bug resolving references Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 02/33] commit_ref(): if there is an empty dir in the way, delete it Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 03/33] read_raw_ref(): don't get confused by an empty directory Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 04/33] safe_create_leading_directories(): improve docstring Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 05/33] remove_dir_recursively(): add docstring Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 06/33] refname_is_safe(): use skip_prefix() Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 07/33] refname_is_safe(): don't allow the empty string Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 08/33] refname_is_safe(): insist that the refname already be normalized Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 09/33] commit_ref_update(): write error message to *err, not stderr Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 10/33] rename_ref(): remove unneeded local variable Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 11/33] ref_transaction_commit(): remove local variable n Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 12/33] read_raw_ref(): rename flags argument to type Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 13/33] read_raw_ref(): clear *type at start of function Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 14/33] read_raw_ref(): rename symref argument to referent Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 15/33] read_raw_ref(): improve docstring Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 16/33] read_raw_ref(): move docstring to header file Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 17/33] lock_ref_sha1_basic(): remove unneeded local variable Michael Haggerty
2016-05-06 16:13 ` [PATCH v2 18/33] refs: make error messages more consistent Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 19/33] ref_transaction_create(): disallow recursive pruning Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 20/33] ref_transaction_commit(): correctly report close_ref() failure Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 21/33] delete_branches(): use resolve_refdup() Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 22/33] refs: allow log-only updates Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 23/33] refs: don't dereference on rename Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 24/33] verify_refname_available(): adjust constness in declaration Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 25/33] add_update(): initialize the whole ref_update Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 26/33] lock_ref_for_update(): new function Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 27/33] unlock_ref(): move definition higher in the file Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 28/33] ref_transaction_update(): check refname_is_safe() at a minimum Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 29/33] refs: resolve symbolic refs first Michael Haggerty
2016-05-12 7:45 ` Jeff King
2016-05-12 8:25 ` Jeff King
2016-05-13 12:33 ` Michael Haggerty
2016-05-13 12:35 ` [PATCH v3 " Michael Haggerty
2016-05-13 12:58 ` Jeff King
2016-05-13 12:51 ` [PATCH v2 " Jeff King
2016-05-14 9:02 ` Torsten Bögershausen
2016-05-06 16:14 ` [PATCH v2 30/33] lock_ref_for_update(): don't re-read non-symbolic references Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 31/33] lock_ref_for_update(): don't resolve symrefs Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 32/33] commit_ref_update(): remove the flags parameter Michael Haggerty
2016-05-06 16:14 ` [PATCH v2 33/33] lock_ref_sha1_basic(): only handle REF_NODEREF mode Michael Haggerty
2016-05-09 20:12 ` [PATCH v2 00/33] Yet more preparation for reference backends David Turner
2016-05-09 21:05 ` Junio C Hamano
2016-05-09 21:50 ` Michael Haggerty
2016-05-09 22:04 ` Junio C Hamano
2016-05-12 7:55 ` Jeff King
2016-05-12 16:10 ` Junio C Hamano
2016-05-10 21:32 ` Junio C Hamano
2016-06-10 12:50 ` Michael Haggerty [this message]
2016-06-10 15:43 ` Junio C Hamano
2016-06-13 9:55 ` [ADDENDUM v4] " Michael Haggerty
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=575AB797.809@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=dturner@twopensource.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--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.
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.