From: Jonathan Nieder <jrnieder@gmail.com>
To: Joey Hess <joey@kitenet.net>
Cc: git@vger.kernel.org, 554682@bugs.debian.org,
Adam Brewster <adambrewster@gmail.com>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: [RFC/PATCH 0/8] Re: bug: git-bundle create foo --stdin -> segfault
Date: Sat, 26 Jun 2010 01:17:35 -0500 [thread overview]
Message-ID: <20100626061735.GA15881@burratino> (raw)
In-Reply-To: <20100119002641.GA31434@gnu.kitenet.net>
Joey Hess wrote:
> I noticed that git bundle --stdin actually attempts to read from stdin
> past EOF.
Patch 5 below fixes that. Sadly it is not the whole story and I had
to introduce a memory leak for all users of --stdin to make
‘bundle --stdin’ actually work (patch 6).
In the case of bundle --stdin, I think the leak is unavoidable, while
in general, I suspect a parameter is needed to allow the caller to
indicate whether the leak is needed (analogous to save_commit_buffer).
The rest of the patches should be comparatively pleasant.
Patches 1, 3, and 4 give various parts of bundle creation their own
functions, to make the code easier to digest;
Patch 2 teaches the basis discovery code (which currently forks a
rev-list --boundary process) to use the revision walker directly, to
prepare for patch 5.
Patch 5 looked like it would be the main point of the series: it saves
the list of revs including those read from stdin and resets the
revision walker after the boundary is found. This way, stdin is only
read once and the underlying logic of the revision walk does not need
to be changed significantly.
Even with patch 5, bundle --stdin still finds no revisions to read.
The problem: to write the table of contents, the name passed on the
command line for each ref is needed. Unfortunately, names passed
through stdin are kept in a temporary buffer and then freed; the
random gibberish read instead does not look like a meaningful ref
name, so no valid toc entries are found.
Patch 6 fixes this, at the cost of a memory leak. Suggestions for
avoiding the leak would of course be welcome.
Patch 7 adopts a similar “fix” in a context where it is not needed.
This is just for illustration.
Patch 8 is an unrelated enhancement that came along the way.
Thanks to Joey for the report and Johannes for suggestions for fixing
it.
Thoughts? Tests to add?
Jonathan Nieder (8):
bundle: split basis discovery into its own function
bundle: use libified rev-list --boundary
bundle: split body of list_prerequisites() loop into its own function
bundle: split table of contents output into its own function
bundle: reuse setup_revisions result
revision: Keep ref names after reading them from stdin
bundle: Keep names of basis refs after discovery
bundle_create: Do not exit when given no revs to bundle
bundle.c | 172 ++++++++++++++++++++++++++++++++++-------------------
revision.c | 2 +-
t/t5704-bundle.sh | 4 +-
3 files changed, 113 insertions(+), 65 deletions(-)
next prev parent reply other threads:[~2010-06-26 6:18 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-01-19 0:26 bug: git-bundle create foo --stdin -> segfault Joey Hess
2010-01-19 23:52 ` Johannes Schindelin
2010-04-19 7:14 ` [PATCH 0/2] test bundle --stdin, fix objects_array_remove_duplicates() Jonathan Nieder
2010-04-19 8:03 ` [PATCH 1/2] t5704 (bundle): add tests for bundle --stdin Jonathan Nieder
2010-04-19 8:03 ` [PATCH 2/2] fix "bundle --stdin" segfault Jonathan Nieder
2010-04-20 5:16 ` [PATCH 0/2] test bundle --stdin, fix objects_array_remove_duplicates() Junio C Hamano
2010-06-26 6:17 ` Jonathan Nieder [this message]
2010-06-26 6:19 ` [PATCH 1/8] bundle: split basis discovery into its own function Jonathan Nieder
2010-06-26 6:20 ` [PATCH 2/8] bundle: use libified rev-list --boundary Jonathan Nieder
2010-06-30 17:57 ` Junio C Hamano
2010-06-30 20:34 ` Jonathan Nieder
2010-06-26 6:20 ` [PATCH 3/8] bundle: give list_prerequisites() loop body its own function Jonathan Nieder
2010-06-30 18:04 ` Junio C Hamano
2010-06-30 20:37 ` Jonathan Nieder
2010-06-26 6:21 ` [PATCH 4/8] bundle: split table of contents output into " Jonathan Nieder
2010-06-26 6:22 ` [PATCH 5/8] bundle: reuse setup_revisions result Jonathan Nieder
2010-06-26 6:28 ` [PATCH 6/8] Fix bundle --stdin Jonathan Nieder
2010-06-26 6:29 ` [RFC/PATCH 7/8] bundle: Keep names of basis refs after discovery Jonathan Nieder
2010-06-26 6:31 ` [PATCH 8/8] bundle_create: Do not exit when given no revs to bundle Jonathan Nieder
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=20100626061735.GA15881@burratino \
--to=jrnieder@gmail.com \
--cc=554682@bugs.debian.org \
--cc=Johannes.Schindelin@gmx.de \
--cc=adambrewster@gmail.com \
--cc=git@vger.kernel.org \
--cc=joey@kitenet.net \
/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 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 NNTP newsgroup(s).