* Re: Roadmap for 1.7.9
From: Ævar Arnfjörð Bjarmason @ 2011-12-06 0:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vr50ik55n.fsf@alter.siamese.dyndns.org>
On Mon, Dec 5, 2011 at 23:47, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Mon, Dec 5, 2011 at 21:07, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> Now, here are the biggies that we would want to try to have in reasonable
>>> shape before the next release.
>>
>> I'd like to get the i18n series into 1.7.9 as well. I think it's ready
>> as-is but some minor issues are sure to arise.
>
> Surely.
>
> My impression was that the part that can have interactions with the
> existing codebase is already in, and it is just the matter of updating
> what _() ans Q_() actually do, which can be reverted out quickly if it
> turns out to be necessary, and that was why I didn't count it as part of
> the "biggies" that make us worry.
That's the gist of it. But it's still a massive 1300 line patch,
which:
* Calls setlocale(LC_MESSAGES)
* Uses an evil trick to only call setlocale(LC_CTYPE) while loading
message catalogs
* Might have some portability problems on more obscure OS's.
* Has some tests I've only tested on Linux, FreeBSD and Solaris
So it might have some little surprises, and it would be nice to have
it in master early on so we'll give it plenty of time to cook and
discover any issues before 1.7.9.
^ permalink raw reply
* Yo dawg, I heard you like trees...
From: Sebastian Morr @ 2011-12-05 23:57 UTC (permalink / raw)
To: git
Just out of curiosity: Do you know of any attempts to construct a tree
object that contains itself, that is, references it's own SHA-1?
Sebastian
^ permalink raw reply
* Re: Roadmap for 1.7.9
From: Junio C Hamano @ 2011-12-05 22:47 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List
In-Reply-To: <CACBZZX6aC-E4DxaZzzhfGnK8ovBGCq_gNG3hPU7QjfAiNb3WrA@mail.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> On Mon, Dec 5, 2011 at 21:07, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Now, here are the biggies that we would want to try to have in reasonable
>> shape before the next release.
>
> I'd like to get the i18n series into 1.7.9 as well. I think it's ready
> as-is but some minor issues are sure to arise.
Surely.
My impression was that the part that can have interactions with the
existing codebase is already in, and it is just the matter of updating
what _() ans Q_() actually do, which can be reverted out quickly if it
turns out to be necessary, and that was why I didn't count it as part of
the "biggies" that make us worry.
^ permalink raw reply
* Re: [PATCH, v4] git-tag: introduce --cleanup option
From: Junio C Hamano @ 2011-12-05 22:41 UTC (permalink / raw)
To: Jeff King; +Cc: Kirill A. Shutemov, git
In-Reply-To: <20111205215148.GA22663@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> More importantly, though, this seems to break t6300 badly. I haven't
> looked into why yet, though.
Probably two issues.
- opt.message (and the original 'message') was misnamed and confused the
patch author what "if (!message && !buf->len)" meant.
- "opt" is a structure meant to be extensible, but is not initialized as
a whole, inviting future errors.
It still seems to be broken with respect to the primary thing the patch
wanted to do (t7400 "git tag -F commentsfile comments-annotated-tag" does
not seem to produce an expected result), so I'll kick it back to the
Kirill to look at.
Thanks.
builtin/tag.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 27a66a3..7883720 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -329,7 +329,7 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
}
struct create_tag_options {
- unsigned int message;
+ unsigned int message_given:1;
unsigned int sign;
enum {
CLEANUP_NONE,
@@ -364,7 +364,7 @@ static void create_tag(const unsigned char *object, const char *tag,
if (header_len > sizeof(header_buf) - 1)
die(_("tag header too big."));
- if (!opt->message) {
+ if (!opt->message_given) {
int fd;
/* write the template message before editing: */
@@ -393,7 +393,7 @@ static void create_tag(const unsigned char *object, const char *tag,
if (opt->cleanup_mode != CLEANUP_NONE)
stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);
- if (opt->message && !buf->len)
+ if (!opt->message_given && !buf->len)
die(_("no tag message?"));
strbuf_insert(buf, 0, header_buf, header_len);
@@ -486,7 +486,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
git_config(git_tag_config, NULL);
- opt.sign = 0;
+ memset(&opt, 0, sizeof(opt));
argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
@@ -552,10 +552,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
else if (!force)
die(_("tag '%s' already exists"), tag);
- opt.message = msg.given || msgfile;
+ opt.message_given = msg.given || msgfile;
if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
- opt.cleanup_mode = !opt.message ? CLEANUP_ALL : CLEANUP_SPACE;
+ opt.cleanup_mode = !opt.message_given ? CLEANUP_ALL : CLEANUP_SPACE;
else if (!strcmp(cleanup_arg, "verbatim"))
opt.cleanup_mode = CLEANUP_NONE;
else if (!strcmp(cleanup_arg, "whitespace"))
^ permalink raw reply related
* Re: [PATCH 2/2] builtin/apply.c: report error on failure to recognize input
From: Brandon Casey @ 2011-12-05 22:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, artem.bityutskiy
In-Reply-To: <7vzkf6lsyx.fsf@alter.siamese.dyndns.org>
On Mon, Dec 5, 2011 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> When git apply is passed something that is not a patch, it does not produce
>> an error message or exit with a non-zero status if it was not actually
>> "applying" the patch i.e. --check or --numstat etc were supplied on the
>> command line.
>>
>> Fix this by producing an error when apply fails to find any hunks whatsoever
>> while parsing the patch.
>>
>> This will cause some of the output formats (--numstat, --diffstat, etc) to
>> produce an error when they formerly would have reported zero changes and
>> exited successfully. That seems like the correct behavior though. Failure
>> to recognize the input as a patch should be an error.
>>
>> Plus, add a test.
>>
>> Reported-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
>> ---
>>
>> Initially, I was reluctant to change the error message, thinking that
>> error messages for plumbing commands were not supposed to change. But I
>> think I was wrong in that thought, so I changed the error message so it
>> was a more descriptive "unrecognized input".
>
> I am still reluctant to see
>
> $ git apply </dev/null
> error: unrecognized input
>
> instead of "error: No changes", though.
I'm not partial to "unrecognized input", but I thought it was more
descriptive of what happened than "No changes". This error message is
only printed out when absolutely no hunks were found while parsing the
input.
> "git apply --check" is about asking "do you see anything offending in the
> diff?" and it is not "git apply --dry-run" that asks "do you promise if I
> feed this for real to you you will apply it without complaint?".
>
> I am slightly in favor of answering "well you do not have a diff to begin
> with, which in itself is suspicious" to "do you see anything offending?"
> question, but I have to admit that it is an equally valid answer to say
> "no, there is nothing offending in the diff.", which is what we do with
> the current code.
>
> So, I dunno.
I think the current code is a little inconsistent with respect to
empty or bogus non-diff input.
It seems more consistent that if it is an error to tell git apply to
apply zero hunks, then it is also an error to --check zero hunks, or
--stat etc. In all cases the cause is the same: failure to find any
hunks in the input because the input was not a diff.
Also, the man page description of --check says that it checks "if the
patch is applicable to the current working tree and/or the index".
The new behavior would answer that with "no, this patch is not
applicable ... since no hunks were found", rather than "yes, because
no hunks were found". But I'm really arguing on the side of
"unrecognized input should be an error", since the new behavior would
also be an error for --stat, --numstat, etc.
-Brandon
^ permalink raw reply
* Re: [PATCH, v4] git-tag: introduce --cleanup option
From: Junio C Hamano @ 2011-12-05 22:30 UTC (permalink / raw)
To: Jeff King; +Cc: Kirill A. Shutemov, git
In-Reply-To: <20111205215148.GA22663@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> More importantly, though, this seems to break t6300 badly. I haven't
> looked into why yet, though.
Also breaks 7004 which is _about_ tags.
Rolling a broken patch in quick succession to v4 without ever running
tests (and not adding new test pieces to protect new feature) is not a
very productive way to use the reviewer bandwidth on this list.
^ permalink raw reply
* Re: [PATCH, v4] git-tag: introduce --cleanup option
From: Jeff King @ 2011-12-05 22:29 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: git, Junio C Hamano
In-Reply-To: <20111205222724.GA7603@sigill.intra.peff.net>
On Mon, Dec 05, 2011 at 05:27:24PM -0500, Jeff King wrote:
> I also looked at factoring out the "which cleanup mode to select" logic
> from builtin/commit.c, but it turned out to just make things harder to
> follow.
While I was doing that, I also noticed this minor fix:
-- >8 --
Subject: [PATCH] stripspace: fix outdated comment
The comment on top of stripspace() claims that the buffer
will no longer be NUL-terminated. However, this has not been
the case at least since the move to using strbuf in 2007.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/stripspace.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 4d3b93f..1288ffc 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -22,8 +22,6 @@ static size_t cleanup(char *line, size_t len)
* Remove empty lines from the beginning and end
* and also trailing spaces from every line.
*
- * Note that the buffer will not be NUL-terminated.
- *
* Turn multiple consecutive empty lines between paragraphs
* into just one empty line.
*
--
1.7.8.rc4.4.g884ec
^ permalink raw reply related
* Re: [PATCH, v4] git-tag: introduce --cleanup option
From: Jeff King @ 2011-12-05 22:27 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: git, Junio C Hamano
In-Reply-To: <1322972426-7591-1-git-send-email-kirill@shutemov.name>
On Sun, Dec 04, 2011 at 06:20:26AM +0200, Kirill A. Shutemov wrote:
> @@ -367,14 +390,15 @@ static void create_tag(const unsigned char *object, const char *tag,
> [...]
> - if (!message && !buf->len)
> + if (opt->message && !buf->len)
> die(_("no tag message?"));
Ah, this is the hunk that causes t6300 to fail. You accidentally removed
the negation when converting the "message" flag over.
This was much easier to find by splitting the refactoring (where the bug
is) away from the new feature (which is where I assumed the bug was).
Here's the first half of the "split". You can rebase your original patch
on top to get the second half.
I also looked at factoring out the "which cleanup mode to select" logic
from builtin/commit.c, but it turned out to just make things harder to
follow.
-- >8 --
From: "Kirill A. Shutemov" <kirill@shutemov.name>
Subject: [PATCH] tag: refactor passing tag creation options
Rather than continually adding parameters to the create_tag
function, we can put all of the flags in a struct.
Signed-off-by: Jeff King <peff@peff.net>
---
Actually, I'm not sure this really buys us much as a refactoring. It
saves a parameter in the function, but it's not like we end up passing
all those parameters to sub-functions, where something like this would
increase readability. I'm fine with this, but I'd also be fine with just
dropping this half and passing the cleanup_mode parameter directly.
builtin/tag.c | 32 ++++++++++++++++++++------------
1 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 9b6fd95..e5bd708 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -319,8 +319,13 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
return 0;
}
+struct create_tag_options {
+ unsigned int message;
+ unsigned int sign;
+};
+
static void create_tag(const unsigned char *object, const char *tag,
- struct strbuf *buf, int message, int sign,
+ struct strbuf *buf, struct create_tag_options *opt,
unsigned char *prev, unsigned char *result)
{
enum object_type type;
@@ -345,7 +350,7 @@ static void create_tag(const unsigned char *object, const char *tag,
if (header_len > sizeof(header_buf) - 1)
die(_("tag header too big."));
- if (!message) {
+ if (!opt->message) {
int fd;
/* write the template message before editing: */
@@ -369,12 +374,12 @@ static void create_tag(const unsigned char *object, const char *tag,
stripspace(buf, 1);
- if (!message && !buf->len)
+ if (!opt->message && !buf->len)
die(_("no tag message?"));
strbuf_insert(buf, 0, header_buf, header_len);
- if (build_tag_object(buf, sign, result) < 0) {
+ if (build_tag_object(buf, opt->sign, result) < 0) {
if (path)
fprintf(stderr, _("The tag message has been left in %s\n"),
path);
@@ -422,9 +427,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
unsigned char object[20], prev[20];
const char *object_ref, *tag;
struct ref_lock *lock;
-
- int annotate = 0, sign = 0, force = 0, lines = -1,
- list = 0, delete = 0, verify = 0;
+ struct create_tag_options opt;
+ int annotate = 0, force = 0, lines = -1, list = 0,
+ delete = 0, verify = 0;
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
@@ -442,7 +447,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', "message", &msg, "message",
"tag message", parse_msg_arg),
OPT_FILENAME('F', "file", &msgfile, "read message from file"),
- OPT_BOOLEAN('s', "sign", &sign, "annotated and GPG-signed tag"),
+ OPT_BOOLEAN('s', "sign", &opt.sign, "annotated and GPG-signed tag"),
OPT_STRING('u', "local-user", &keyid, "key-id",
"use another key to sign the tag"),
OPT__FORCE(&force, "replace the tag if exists"),
@@ -459,13 +464,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
git_config(git_tag_config, NULL);
+ opt.sign = 0;
+
argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
if (keyid) {
- sign = 1;
+ opt.sign = 1;
set_signingkey(keyid);
}
- if (sign)
+ if (opt.sign)
annotate = 1;
if (argc == 0 && !(delete || verify))
list = 1;
@@ -523,9 +530,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
else if (!force)
die(_("tag '%s' already exists"), tag);
+ opt.message = msg.given || msgfile;
+
if (annotate)
- create_tag(object, tag, &buf, msg.given || msgfile,
- sign, prev, object);
+ create_tag(object, tag, &buf, &opt, prev, object);
lock = lock_any_ref_for_update(ref.buf, prev, 0);
if (!lock)
--
1.7.8.rc4.4.g884ec
^ permalink raw reply related
* Re: [PATCH, v4] git-tag: introduce --cleanup option
From: Jeff King @ 2011-12-05 21:51 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: git, Junio C Hamano
In-Reply-To: <1322972426-7591-1-git-send-email-kirill@shutemov.name>
On Sun, Dec 04, 2011 at 06:20:26AM +0200, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
>
> Normally git tag stripes tag message lines starting with '#', trailing
> spaces from every line and empty lines from the beginning and end.
s/stripes/strips
> --cleanup allows to select different cleanup modes for tag message.
> It provides the same interface as --cleanup option in git-commit.
Thanks, I think this is better, though it would be better still if they
could share the code. As a minor nit, I think the patch would be a
little easier to read and review if you split the actual changes from
the refactoring to use the "struct create_tag_options".
More importantly, though, this seems to break t6300 badly. I haven't
looked into why yet, though.
-Peff
^ permalink raw reply
* Re: hooks that do not consume stdin sometimes crash git with SIGPIPE
From: Jeff King @ 2011-12-05 21:43 UTC (permalink / raw)
To: Joey Hess; +Cc: git, Lars Wirzenius
In-Reply-To: <20111205192930.GA32463@gnu.kitenet.net>
On Mon, Dec 05, 2011 at 03:29:30PM -0400, Joey Hess wrote:
> We had a weird problem where, after moving to a new, faster server,
> "git push" would sometimes fail like this:
>
> Unpacking objects: 100% (3/3), done.
> fatal: The remote end hung up unexpectedly
> fatal: The remote end hung up unexpectedly
>
> Turns out that git-receive-pack was dying due to an uncaught SIGPIPE.
> The SIGPIPE occurred when it tried to write to the pre-receive hook's
> stdin. The pre-receive hook, in this case, was able to do all the checks
> it needed to do[1] without the input, and so did exit(0) without
> consuming it.
Ugh. Yeah, receive-pack should probably just be ignoring SIGPIPE
entirely. It checks the return from write() properly where it matters,
so SIGPIPE is just an annoyance.
I would go so far as to say that git should be ignoring SIGPIPE 99% of
the time. We have crap like this sprinkled throughout the code base:
$ git grep -C1 SIGPIPE
builtin/tag.c- /* When the username signingkey is bad, program could be terminated
builtin/tag.c: * because gpg exits without reading and then write gets SIGPIPE. */
builtin/tag.c: signal(SIGPIPE, SIG_IGN);
[...]
upload-pack.c- * If rev-list --stdin encounters an unknown commit, it
upload-pack.c: * terminates, which will cause SIGPIPE in the write loop
upload-pack.c- */
upload-pack.c: sigchain_push(SIGPIPE, SIG_IGN);
but I find it highly unlikely that they are covering all of the cases.
You found one already, and these things can quite often be
race-condition heisenbugs.
The one place where SIGPIPE _is_ useful is for things like "git log"
which are just dumping to a pager over stdout. When the pager dies, we
can stop bothering to produce output.
It would be really nice if we could write a sigpipe handler that knew
which fd caused the the signal, and then we could do something like:
void sigpipe_handler(int sig)
{
/* If we're writing to a pager over stdout, then there is
* little use in writing more; nobody is interested in our
* output. */
if (get_fd_that_caused_sigpipe() == 1 && pager_in_use)
exit(1);
/* Otherwise, ignore it, as it's a write to some auxiliary
* process and we will be careful about checking the return
* code from write(). */
}
But I don't think such a function exists. We could just check
pager_in_use, which would cover most cases (e.g., programs like
receive-pack don't use a pager). But it would fail in the case of
something like "git log" using an auxiliary process that closes the pipe
early. Maybe that would be good enough, though. I dunno.
Another option is to just ignore SIGPIPE entirely, and convince programs
like log to actually bother checking the result of write(). It would be
a slight pain to check every printf() call we make in log-tree.c, but we
could do one of:
1. Make a set of stdio wrapper scripts that exit gracefully on EPIPE.
Using the wrapper scripts everywhere is a slight pain, but would
work pretty well, and adapts easily to other places like printing
lists of refs, etc.
2. Don't check every printf. After printing each commit, check
ferror(stdout) and exit as appropriate. This is a very small amount
of code, but you'd need to do it in several places (i.e., anywhere
that produces a lot of output).
> I think git should ignore SIGPIPE when writing to hooks. Otherwise,
> hooks may have to go out of their way to consume all input, and as I've
> seen, the races when they fail to do this can lurk undiscovered.
Yeah, certainly. The question to me is whether we should just stick a
SIG_IGN in the beginning of receive-pack, or whether we should try to
deal with this problem everywhere.
For example, I suspect the same problem exists in the credential helpers
I wrote recently. Generally they will read all of their input, but you
could do something like:
[credential "https://github.com"]
username = peff
helper = "!
f() {
# if we call this helper, we know we want the
# github password, or else this helper config
# would never have been triggered. so we
# don't even have to bother reading our stdin.
# We only handle getting the password.
test "$1" = "get" || return
# Presumably gpg-agent will ask for and cache
# your gpg password.
p=`gpg -qd --no-tty <~/.github-password.gpg`
echo "password=$p"
}; f"
This can racily fail if our write happens after the helper has already
finished (unlikely, but possible).
-Peff
^ permalink raw reply
* Re: Roadmap for 1.7.9
From: Ævar Arnfjörð Bjarmason @ 2011-12-05 21:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vd3c2lr36.fsf@alter.siamese.dyndns.org>
On Mon, Dec 5, 2011 at 21:07, Junio C Hamano <gitster@pobox.com> wrote:
> Now, here are the biggies that we would want to try to have in reasonable
> shape before the next release.
I'd like to get the i18n series into 1.7.9 as well. I think it's ready
as-is but some minor issues are sure to arise.
If time permits I'd also like to have a series of po/*.po files in as
well once it's in "master". Maybe as a submodule, which would be neat
in itself as we'd start dogfooding submodules.
^ permalink raw reply
* Re: [PATCH v2 0/3] grep multithreading and scaling
From: Thomas Rast @ 2011-12-05 20:16 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Eric Herman, git, Junio C Hamano
In-Reply-To: <201112051038.16423.trast@student.ethz.ch>
Thomas Rast wrote:
>
> I just found out that on Linux, there's mincore() that can tell us
> (racily, but who cares) whether a given file mapping is in memory.
[...]
> So that looks fairly promising, and the order would then be:
>
> - if stat-clean, and we have mincore(), and it tells us we can do it
> cheaply: grab file from tree
>
> - if it's a loose object: decompress it
>
> - if stat-clean: grab file from tree
>
> - access packs as usual
Just a small note, I tried two things:
* the simpler option of grabbing a loose object if it exists and is
mincore() turns out to massively slow down 'git log HEAD', probably
because only very few of these objects are loose in the first place
* doing this only under grep's use_threads, and dropping the lock
around unpack_sha1_file() [i.e., zlib decompression] still results
in a git-grep that is slower than without this, though not much
So no improvement here. Will have to look into the worktree trick
though.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Roadmap for 1.7.9
From: Junio C Hamano @ 2011-12-05 20:07 UTC (permalink / raw)
To: git
I do not do roadmaps, so the title of this message is not quite correct,
but I think I should post my current thinking on what have been cooking so
far that deserve to be polished further to be in the next release.
I've been aiming for a release cycle that lasts for 8 to 10 weeks, and the
last 2 or 3 weeks of a cycle is meant for release candidate testing that
we fix only regressions, which means we need to see how much we can do
within 5 to 8 weeks. The current cycle is expected to end at the end of
January next month [*1*].
I am not so worried about small and obvious fixes and enhancements, and
changes in the periphery areas. They can and will be merged as they come
and cook long enough in 'next' without issues reported by their users. I
expect what have been cooking in 'next' during the feature-freeze before
the 1.7.8 release, other than the ones listed below, to be more or less
ready already and they should be in 'master' shortly.
Now, here are the biggies that we would want to try to have in reasonable
shape before the next release. The list may be a bit too ambitious, given
that this cycle overlaps with end-of-this-year/beginning-of-new-year
holiday season in various cultures.
* Credentials and keychain (Peff)
* Pulling signed tag (me, Linus)
* Update "request-pull" script with information that matters (me)
* Revisiting threading of grep (Rene, Thomas Rast)
* Optimization of reading hierarchical refspace lazily (Michael Haggerty)
I expect the following will not make much progress without further
discussion:
* Signed commit (me)
* Ignored vs Precious (Nguyen Thai Ngoc Duy)
* Sequencer (Ram, Jonathan Nieder)
I think the following are too big to be ready by the end of this cycle
(polishing could and will continue as time permits).
* Large-files
- bulk check-in (me)
- "Chunked" encoding of large files (me)
[Reference]
*1* https://www.google.com/calendar/embed?src=jfgbl2mrlipp4pb6ieih0qr3so%40group.calendar.google.com&ctz=America/Los_Angeles
^ permalink raw reply
* hooks that do not consume stdin sometimes crash git with SIGPIPE
From: Joey Hess @ 2011-12-05 19:29 UTC (permalink / raw)
To: git; +Cc: Lars Wirzenius
In-Reply-To: <20110829203107.GA4946@gnu.kitenet.net>
[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]
We had a weird problem where, after moving to a new, faster server,
"git push" would sometimes fail like this:
Unpacking objects: 100% (3/3), done.
fatal: The remote end hung up unexpectedly
fatal: The remote end hung up unexpectedly
Turns out that git-receive-pack was dying due to an uncaught SIGPIPE.
The SIGPIPE occurred when it tried to write to the pre-receive hook's
stdin. The pre-receive hook, in this case, was able to do all the checks
it needed to do[1] without the input, and so did exit(0) without
consuming it.
Apparently that causes a race. Most of the time, git forks the hook,
writes output to the hook, and then the hook runs, ignores it, and exits.
But sometimes, on our new faster (and SMP) server, git forked the hook, and
it ran, and exited, before git got around to writing to it, resulting in
the SIGPIPE.
write(7, "c9f98c67d70a1cfeba382ec27d87644a"..., 100) = -1 EPIPE (Broken pipe)
--- SIGPIPE (Broken pipe) @ 0 (0) ---
I think git should ignore SIGPIPE when writing to hooks. Otherwise,
hooks may have to go out of their way to consume all input, and as I've
seen, the races when they fail to do this can lurk undiscovered.
Note that I encountered this same sort of problem from another direction
(involving smudge filters) not long ago, and sent a patch, in
<20110829203107.GA4946@gnu.kitenet.net>. That wasn't applied, and is
in different code than the case I outlined above.
--
see shy jo
[1] If you're wondering, it only needed to check that the push was
coming from a trusted UID. With an untrusted UID, it did further
checks that consumed the stdin.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] builtin/apply.c: report error on failure to recognize input
From: Junio C Hamano @ 2011-12-05 19:27 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, artem.bityutskiy
In-Reply-To: <1322944550-27344-2-git-send-email-drafnel@gmail.com>
Brandon Casey <drafnel@gmail.com> writes:
> When git apply is passed something that is not a patch, it does not produce
> an error message or exit with a non-zero status if it was not actually
> "applying" the patch i.e. --check or --numstat etc were supplied on the
> command line.
>
> Fix this by producing an error when apply fails to find any hunks whatsoever
> while parsing the patch.
>
> This will cause some of the output formats (--numstat, --diffstat, etc) to
> produce an error when they formerly would have reported zero changes and
> exited successfully. That seems like the correct behavior though. Failure
> to recognize the input as a patch should be an error.
>
> Plus, add a test.
>
> Reported-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> ---
>
> Initially, I was reluctant to change the error message, thinking that
> error messages for plumbing commands were not supposed to change. But I
> think I was wrong in that thought, so I changed the error message so it
> was a more descriptive "unrecognized input".
I am still reluctant to see
$ git apply </dev/null
error: unrecognized input
instead of "error: No changes", though.
"git apply --check" is about asking "do you see anything offending in the
diff?" and it is not "git apply --dry-run" that asks "do you promise if I
feed this for real to you you will apply it without complaint?".
I am slightly in favor of answering "well you do not have a diff to begin
with, which in itself is suspicious" to "do you see anything offending?"
question, but I have to admit that it is an equally valid answer to say
"no, there is nothing offending in the diff.", which is what we do with
the current code.
So, I dunno.
^ permalink raw reply
* Re: [PATCH] Documentation: fix formatting error in merge-options.txt
From: Junio C Hamano @ 2011-12-05 19:16 UTC (permalink / raw)
To: Jack Nagel; +Cc: git, Jay Soffian
In-Reply-To: <1323071607-29213-1-git-send-email-jacknagel@gmail.com>
Jack Nagel <jacknagel@gmail.com> writes:
> The first paragraph inside of a list item does not need a preceding line
> consisting of a single '+', and in fact this causes the text to be
> misrendered. Fix it.
Thanks.
>
> Signed-off-by: Jack Nagel <jacknagel@gmail.com>
> ---
> Documentation/merge-options.txt | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 6bd0b04..1a5c12e 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -9,7 +9,6 @@ inspect and further tweak the merge result before committing.
>
> --edit::
> -e::
> -+
> Invoke editor before committing successful merge to further
> edit the default merge message.
^ permalink raw reply
* Re: [bug?] checkout -m doesn't work without a base version
From: Junio C Hamano @ 2011-12-05 18:58 UTC (permalink / raw)
To: Pete Harlan; +Cc: git
In-Reply-To: <4EDBF4D5.6030908@pcharlan.com>
Pete Harlan <pgit@pcharlan.com> writes:
> But this only works if there's a base version; if foo.c was added in
> each branch, we get:
>
> error: path 'foo.c' does not have all three versions
>
> Git didn't need all three versions to create the original conflicted
> file, so why would it need them to recreate it?
Because the original "merge" was a bit more carefully written but
"checkout -m" was written without worrying too much about "both sides
added differently" corner case and still being defensive about not doing
random thing upon getting an unexpected input state.
IOW, being lazy ;-)
How does this look?
-- >8 --
checkout -m: no need to insist on having all 3 stages
The content level merge machinery ll_merge() is prepared to merge
correctly in "both sides added differently" case by using an empty blob as
if it were the common ancestor. "checkout -m" could do the same, but didn't
bother supporting it and instead insisted on having all three stages.
Reported-by: Pete Harlan
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/checkout.c | 60 +++++++++++++++++++++++++++++++--------------------
1 files changed, 36 insertions(+), 24 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a80772..923d040 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -114,16 +114,21 @@ static int check_stage(int stage, struct cache_entry *ce, int pos)
return error(_("path '%s' does not have their version"), ce->name);
}
-static int check_all_stages(struct cache_entry *ce, int pos)
+static int check_stages(unsigned stages, struct cache_entry *ce, int pos)
{
- if (ce_stage(ce) != 1 ||
- active_nr <= pos + 2 ||
- strcmp(active_cache[pos+1]->name, ce->name) ||
- ce_stage(active_cache[pos+1]) != 2 ||
- strcmp(active_cache[pos+2]->name, ce->name) ||
- ce_stage(active_cache[pos+2]) != 3)
- return error(_("path '%s' does not have all three versions"),
- ce->name);
+ unsigned seen = 0;
+ const char *name = ce->name;
+
+ while (pos < active_nr) {
+ ce = active_cache[pos];
+ if (strcmp(name, ce->name))
+ break;
+ seen |= (1 << ce_stage(ce));
+ pos++;
+ }
+ if ((stages & seen) != stages)
+ return error(_("path '%s' does not have all necessary versions"),
+ name);
return 0;
}
@@ -150,18 +155,27 @@ static int checkout_merged(int pos, struct checkout *state)
int status;
unsigned char sha1[20];
mmbuffer_t result_buf;
+ unsigned char threeway[3][20];
+ unsigned mode;
+
+ memset(threeway, 0, sizeof(threeway));
+ while (pos < active_nr) {
+ int stage;
+ stage = ce_stage(ce);
+ if (!stage || strcmp(path, ce->name))
+ break;
+ hashcpy(threeway[stage - 1], ce->sha1);
+ if (stage == 2)
+ mode = create_ce_mode(ce->ce_mode);
+ pos++;
+ ce = active_cache[pos];
+ }
+ if (is_null_sha1(threeway[1]) || is_null_sha1(threeway[2]))
+ return error(_("path '%s' does not have necessary versions"), path);
- if (ce_stage(ce) != 1 ||
- active_nr <= pos + 2 ||
- strcmp(active_cache[pos+1]->name, path) ||
- ce_stage(active_cache[pos+1]) != 2 ||
- strcmp(active_cache[pos+2]->name, path) ||
- ce_stage(active_cache[pos+2]) != 3)
- return error(_("path '%s' does not have all 3 versions"), path);
-
- read_mmblob(&ancestor, active_cache[pos]->sha1);
- read_mmblob(&ours, active_cache[pos+1]->sha1);
- read_mmblob(&theirs, active_cache[pos+2]->sha1);
+ read_mmblob(&ancestor, threeway[0]);
+ read_mmblob(&ours, threeway[1]);
+ read_mmblob(&theirs, threeway[2]);
/*
* NEEDSWORK: re-create conflicts from merges with
@@ -192,9 +206,7 @@ static int checkout_merged(int pos, struct checkout *state)
if (write_sha1_file(result_buf.ptr, result_buf.size,
blob_type, sha1))
die(_("Unable to add merge result for '%s'"), path);
- ce = make_cache_entry(create_ce_mode(active_cache[pos+1]->ce_mode),
- sha1,
- path, 2, 0);
+ ce = make_cache_entry(mode, sha1, path, 2, 0);
if (!ce)
die(_("make_cache_entry failed for path '%s'"), path);
status = checkout_entry(ce, state, NULL);
@@ -252,7 +264,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
} else if (stage) {
errs |= check_stage(stage, ce, pos);
} else if (opts->merge) {
- errs |= check_all_stages(ce, pos);
+ errs |= check_stages((1<<2) | (1<<3), ce, pos);
} else {
errs = 1;
error(_("path '%s' is unmerged"), ce->name);
^ permalink raw reply related
* Re: Evaluation of ref-api branch status
From: Junio C Hamano @ 2011-12-05 18:26 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git discussion list
In-Reply-To: <4EDAB62E.5070204@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Now that 1.7.8 is out, I wanted to figure out the status of the
> remaining ref-api changes that are in flight, including the differences
> between between my tree and yours.
I understand that the ultimate and largest objective of these series is to
make sure that the performance does not suffer from very many negative
lookups on the refs/replace hierarchy (which almost always is empty in a
sane repository), and I do want to see that happen. I also think it is
good that the series tries to make sure that the various codepaths we
create new refs do not let the user accidentally create refs with bad
names and/or contents.
When we see a questionable ref that is _already_ in the respository,
however, we may warn but it is wrong to declare the repository to be
broken and refuse to work. That would make it cumbersome or impossible to
even _fix_ such breakage. Regressions of that kind wer in the earlier part
of the series already in 1.7.8 and it was rather unpleasant having to
hotfix them. As our test suite does not deliberately create these
questionable refs and/or try to use them (and I personally do not in my
regular workflow either), I am afraid that it is rather hard to realize
what kind of refnames are what we intended to forbid even from earlier
days but did not _bother_ to check and enforce the rule against so far,
but are forbidden by the updated code, until we unleash the new logic to
the end users with various third party tools that created them [*1*]. I
would want to see us get this part right and solid, and include it in
1.7.9.
It would be nice if we can include also the bits to read hierarchies
lazily, but as they come on top of your B & C, it may end up in 1.7.10 and
I do not mind it at all. Reference resolution is one of the central things
in the user experience, and we cannot afford to ship anything that is
half-done-and-mostly-works over slow-but-correct.
Alternatively you _could_ swap the order of your B & C and D and try to
have your D early while giving B & C more time to cook, but I suspect the
order you chose would be better in the longer term.
> I understand that "next" will soon be re-rolled. Will the re-roll be
> based on the current "pu", or will you start from scratch?
As to the two quickfix patches that are on two remaining topics from you
in my tree, I did them merely as a short-term band-aid. I was expecting,
after 1.7.8 when we eject the topics out of 'next', that they will be
rebased on top of 'master' that already contains a proper fix before these
topics started touching the same codepath.
My reading of your summary suggests that it would be easiest to drop the
three mh/ref-api* topics from my tree, especially the 'refs: loosen
over-strict "format" check' band-aid patches, and re-queue a re-roll from
you.
Thanks.
[Footnote]
*1* Trying to be lenient when reading and being strict when writing as the
general principle would be the safe and sane way forward, and making sure
that we warn *loudly* when we think we are merely being lenient (i.e. we
think we found a bad ref with questionable name and/or contents) would be
a good way to catch our mistakes early. E.g. ".git/config does not record
a correct object name" glitch was caught only because you added the
warning so while it was painful to hotfix that late in the cycle, the
warning was worth it.
^ permalink raw reply
* Re: Debugging git-commit slowness on a large repo
From: Junio C Hamano @ 2011-12-05 17:38 UTC (permalink / raw)
To: Carlos Martín Nieto, Thomas Rast
Cc: Joshua Redstone, git@vger.kernel.org
In-Reply-To: <20111203002347.GB2950@centaur.lab.cmartin.tk>
Carlos Martín Nieto <cmn@elego.de> writes:
> ... At one
> point, commit forgot how to write the tree cache to the index (a
> performance optimisation). Do the times improve if you run 'git
> read-tree HEAD' between one commit and another? Note that this will
> reset the index to the last commit, though for the tests I image you
> use some variation of 'git commit -a'.
>
> Thomas Rast wrote a patch to re-teach commit to store the tree cache,
> but there were some issues and never got applied.
Ahh, I forgot all about that exchange.
http://thread.gmane.org/gmane.comp.version-control.git/178480/focus=178515
The cache-tree mechanism has traditionally been one of the more important
optimizations and it would be very nice if we can resurrect the behaviour
for "git commit" too.
^ permalink raw reply
* Re: [PATCH v2 0/3] grep multithreading and scaling
From: Thomas Rast @ 2011-12-05 9:38 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Eric Herman, git, Junio C Hamano
In-Reply-To: <20111202173400.GC23447@sigill.intra.peff.net>
Jeff King wrote:
>
> A quick perf run shows most of the time is spent inflating objects. The
> diff code has a sneaky trick to re-use worktree files when we know they
> are stat-clean (in diff's case it is to avoid writing a tempfile). I
> wonder if we should use the same trick here.
>
> It would hurt the cold cache case, though, as the compressed versions
> require fewer disk accesses, of course.
I just found out that on Linux, there's mincore() that can tell us
(racily, but who cares) whether a given file mapping is in memory. If
you would like to try it, see the source at the end, but I'm getting
things such as
# in a random collection of files, none of which I have accessed lately
$ ls -l
-rw-r--r-- 1 thomas users 116534 Jul 4 2010 IMG_4884.JPG
-rw-r--r-- 1 thomas users 7278081 Aug 25 2010 remoteserverrepo.zip
$ ./mincore IMG_4884.JPG
00000000000000000000000000000
$ cat IMG_4884.JPG > /dev/null
$ ./mincore IMG_4884.JPG
11111111111111111111111111111
$ ./mincore remoteserverrepo.zip
0000000000000000000000[...]
$ head -10 remoteserverrepo.zip >/dev/null
$ ./mincore remoteserverrepo.zip
1111000000000000000000[...]
So that looks fairly promising, and the order would then be:
- if stat-clean, and we have mincore(), and it tells us we can do it
cheaply: grab file from tree
- if it's a loose object: decompress it
- if stat-clean: grab file from tree
- access packs as usual
> PS I suspect your timings are somewhat affected by the simplicity of the
> regex you are asking for. The time to inflate the blobs dominates,
> because the search is just a memmem(). On my quad-core w/
> hyperthreading (i.e., 8 apparent cores):
>
> $ /usr/bin/time git grep INITRAMFS_ROOT_UID >/dev/null
> 0.42user 0.45system 0:00.15elapsed 578%CPU
> $ /usr/bin/time git grep 'a.*b' >/dev/null
> 14.68user 0.50system 0:02.00elapsed 758%CPU
> $ /usr/bin/time git grep --cached INITRAMFS_ROOT_UID >/dev/null
> 7.64user 0.41system 0:07.61elapsed 105%CPU
> $ /usr/bin/time git grep --cached 'a.*b' >/dev/null
> 23.46user 0.47system 0:08.42elapsed 284%CPU
>
> So I think there is value in parallelizing even --cached greps. But
> we could do so much better if blob inflation could be done in
> parallel.
Ok, I see, I missed that part. Perhaps the heuristic should then be
"if the regex boils down to memmem, disable threading", but let's see
what loose object decompression in parallel can give us.
---- 8< ---- mincore.c ---- 8< ----
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/mman.h>
#include <fcntl.h>
void die(const char *s)
{
perror(s);
exit(1);
}
int main (int argc, char *argv[])
{
void *mem;
size_t len;
struct stat st;
int fd;
unsigned char *vec;
int vsize;
int i;
size_t page = sysconf(_SC_PAGESIZE);
if (argc != 2) {
fprintf(stderr, "usage: %s <file>\n", argv[0]);
exit(2);
}
fd = open(argv[1], O_RDONLY);
if (fd == -1)
die("open failed");
if (fstat(fd, &st) == -1)
die("fstat failed");
mem = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
if (mem == (void*) -1)
die("mmap failed");
vsize = (st.st_size+page-1)/page;
vec = malloc(vsize);
if (!vec)
die("malloc failed");
if (mincore(mem, st.st_size, vec) == -1)
die("mincore failed");
for (i = 0; i < vsize; i++)
printf("%d", (int) vec[i]);
printf("\n");
return 0;
}
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH v2 3/3] grep: disable threading in all but worktree case
From: Thomas Rast @ 2011-12-05 9:02 UTC (permalink / raw)
To: René Scharfe; +Cc: Eric Herman, git, Junio C Hamano
In-Reply-To: <4ED8F9AE.8030605@lsrfire.ath.cx>
René Scharfe wrote:
> Am 02.12.2011 14:07, schrieb Thomas Rast:
> > Measuring grep performance showed that in all but the worktree case
> > (as opposed to --cached,<committish> or<treeish>), threading
> > actually slows things down. For example, on my dual-core
> > hyperthreaded i7 in a linux-2.6.git at v2.6.37-rc2, I got:
> >
> > Threads worktree case | --cached case
> > --------------------------------------------------------------------------
> > 8 (default) | 2.17user 0.15sys 0:02.20real | 0.11user 0.26sys 0:00.11real
> > 4 | 2.06user 0.17sys 0:02.08real | 0.11user 0.26sys 0:00.12real
> > 2 | 2.02user 0.25sys 0:02.08real | 0.15user 0.37sys 0:00.28real
> > NO_PTHREADS | 1.57user 0.05sys 0:01.64real | 0.09user 0.12sys 0:00.22real
>
> Are the columns mixed up?
Indeed, sorry.
In case you were wondering why this table is different from the
numbers given in the cover letter: I noticed at some point that I had
an incomplete checkout (apparently 'git checkout -- .' is really not
the same as 'git reset --hard'... sigh). Then I saw that while the
numbers were different, the conclusion was not, so I forgot to update
it.
> This is a bit radical. I think the underlying issue that
> read_sha1_file() is not thread-safe can be solved eventually and then
> we'd need to readd that code.
I'm also scared of sha1_file.c, especially when it gets down to
packfiles. But perhaps it wouldn't be *too* hard to do it in parallel
iff the object can be read from the loose object store.
> PS: Patches one and three missed a signoff.
Oops, thanks, turns out I had a misconfigured alias ...
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* [PATCH] Documentation: fix formatting error in merge-options.txt
From: Jack Nagel @ 2011-12-05 7:53 UTC (permalink / raw)
To: git, gitster; +Cc: jacknagel
The first paragraph inside of a list item does not need a preceding line
consisting of a single '+', and in fact this causes the text to be
misrendered. Fix it.
Signed-off-by: Jack Nagel <jacknagel@gmail.com>
---
Documentation/merge-options.txt | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 6bd0b04..1a5c12e 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -9,7 +9,6 @@ inspect and further tweak the merge result before committing.
--edit::
-e::
-+
Invoke editor before committing successful merge to further
edit the default merge message.
--
1.7.8
^ permalink raw reply related
* [PATCH] Set hard limit on delta chain depth
From: Nguyễn Thái Ngọc Duy @ 2011-12-05 7:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
Too deep delta chains can cause stack overflow in get_base_data(). Set
a hard limit so that index-pack does not run out of stack. Also stop
people from producing such a long delta chains using "pack-object
--depth=<too large>"
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
I used to make very long delta chains and triggered this in index-pack.
I did not care reporting because it's my fault anyway. Think again,
index-pack is called at server side and a malicious client can
trigger this. This patch does not improve the situation much, but at
least we won't get sigsegv at server side.
builtin/index-pack.c | 12 ++++++++++--
builtin/pack-objects.c | 3 +++
pack.h | 2 ++
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0945adb..cfb8cb2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -504,13 +504,16 @@ static int is_delta_type(enum object_type type)
return (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);
}
-static void *get_base_data(struct base_data *c)
+static void *get_base_data_1(struct base_data *c, int depth)
{
+ if (depth > MAX_DELTA_DEPTH)
+ die("index-pack: too long delta chain");
+
if (!c->data) {
struct object_entry *obj = c->obj;
if (is_delta_type(obj->type)) {
- void *base = get_base_data(c->base);
+ void *base = get_base_data_1(c->base, depth + 1);
void *raw = get_data_from_pack(obj);
c->data = patch_delta(
base, c->base->size,
@@ -530,6 +533,11 @@ static void *get_base_data(struct base_data *c)
return c->data;
}
+static void *get_base_data(struct base_data *c)
+{
+ return get_base_data_1(c, 0);
+}
+
static void resolve_delta(struct object_entry *delta_obj,
struct base_data *base, struct base_data *result)
{
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 824ecee..85ee04b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2508,6 +2508,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
if (keep_unreachable && unpack_unreachable)
die("--keep-unreachable and --unpack-unreachable are incompatible.");
+ if (depth > MAX_DELTA_DEPTH)
+ die("--depth exceeds %d", MAX_DELTA_DEPTH);
+
if (progress && all_progress_implied)
progress = 2;
diff --git a/pack.h b/pack.h
index 722a54e..b8f60c3 100644
--- a/pack.h
+++ b/pack.h
@@ -3,6 +3,8 @@
#include "object.h"
+#define MAX_DELTA_DEPTH 128
+
/*
* Packed object header
*/
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Re: Bug: git bus error (stack blowout)
From: Nguyen Thai Ngoc Duy @ 2011-12-05 4:58 UTC (permalink / raw)
To: vsrinivas; +Cc: git
In-Reply-To: <CAAcG=3NfvYSjhHDNb8aZ=_O4661bV7jkZBpmc77ZVCFDQQdHJw@mail.gmail.com>
On Sun, Dec 04, 2011 at 05:50:19PM -0500, Venkatesh Srinivas wrote:
> Hi,
>
> When using git 1.7.6.3 from NetBSD's pkgsrc, on this git tree:
> http://gitweb.dragonflybsd.org/pkgsrcv2.git, I got a bus error when
> switching from the pkgsrc-2011q3 branch to the master branch. I have a
> core file and the git binary if it'd be helpful; it looks like
> mark_parents_uninteresting() was called recursively entirely too many
> times (>60,000), originally from prepare_revision_walk(), from
> stat_tracking_info(), from format_tracking_info(),
> update_revs_for_switch(), from cmd_checkout().
This patch may fix it for you, although it'd be interesting to
understand how you get into this (I'm still cloning pkgsrcv2.git).
-- 8< --
Subject: [PATCH] Eliminate recursion in setting/clearing marks in commit list
Recursion in a DAG is generally a bad idea because it could be very
deep. Be defensive and avoid recursion in mark_parents_uninteresting()
and clear_commit_marks().
mark_parents_uninteresting() learns a trick from clear_commit_marks()
to avoid malloc() in (dorminant) single-parent case.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
commit.c | 31 ++++++++++++++++++++-----------
revision.c | 45 +++++++++++++++++++++++++++++----------------
2 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/commit.c b/commit.c
index 0775eec..cd19a69 100644
--- a/commit.c
+++ b/commit.c
@@ -423,22 +423,31 @@ struct commit *pop_most_recent_commit(struct commit_list **list,
void clear_commit_marks(struct commit *commit, unsigned int mark)
{
- while (commit) {
- struct commit_list *parents;
+ struct commit_list *list = NULL, *l;
+ commit_list_insert(commit, &list);
+ while (list) {
+ commit = list->item;
+ l = list;
+ list = list->next;
+ free(l);
- if (!(mark & commit->object.flags))
- return;
+ while (commit) {
+ struct commit_list *parents;
- commit->object.flags &= ~mark;
+ if (!(mark & commit->object.flags))
+ break;
- parents = commit->parents;
- if (!parents)
- return;
+ commit->object.flags &= ~mark;
+
+ parents = commit->parents;
+ if (!parents)
+ break;
- while ((parents = parents->next))
- clear_commit_marks(parents->item, mark);
+ while ((parents = parents->next))
+ commit_list_insert(parents->item, &list);
- commit = commit->parents->item;
+ commit = commit->parents->item;
+ }
}
}
diff --git a/revision.c b/revision.c
index 0aa3638..8d4069e 100644
--- a/revision.c
+++ b/revision.c
@@ -139,11 +139,32 @@ void mark_tree_uninteresting(struct tree *tree)
void mark_parents_uninteresting(struct commit *commit)
{
- struct commit_list *parents = commit->parents;
+ struct commit_list *parents = NULL, *l;
+
+ for (l = commit->parents; l; l = l->next)
+ commit_list_insert(l->item, &parents);
while (parents) {
struct commit *commit = parents->item;
- if (!(commit->object.flags & UNINTERESTING)) {
+ l = parents;
+ parents = parents->next;
+ free(l);
+
+ while (commit) {
+ /*
+ * A missing commit is ok iff its parent is marked
+ * uninteresting.
+ *
+ * We just mark such a thing parsed, so that when
+ * it is popped next time around, we won't be trying
+ * to parse it and get an error.
+ */
+ if (!has_sha1_file(commit->object.sha1))
+ commit->object.parsed = 1;
+
+ if (commit->object.flags & UNINTERESTING)
+ break;
+
commit->object.flags |= UNINTERESTING;
/*
@@ -154,21 +175,13 @@ void mark_parents_uninteresting(struct commit *commit)
* wasn't uninteresting), in which case we need
* to mark its parents recursively too..
*/
- if (commit->parents)
- mark_parents_uninteresting(commit);
- }
+ if (!commit->parents)
+ break;
- /*
- * A missing commit is ok iff its parent is marked
- * uninteresting.
- *
- * We just mark such a thing parsed, so that when
- * it is popped next time around, we won't be trying
- * to parse it and get an error.
- */
- if (!has_sha1_file(commit->object.sha1))
- commit->object.parsed = 1;
- parents = parents->next;
+ for (l = commit->parents->next; l; l = l->next)
+ commit_list_insert(l->item, &parents);
+ commit = commit->parents->item;
+ }
}
}
--
1.7.8.36.g69ee2
-- 8< --
--
Duy
^ permalink raw reply related
* [PATCH v2] git-p4: introduce skipSubmitEdit
From: Pete Wyckoff @ 2011-12-05 0:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Loren A. Linden Levy, Luke Diamand
Add a configuration variable to skip invoking the editor in the
submit path.
The existing variable skipSubmitEditCheck continues to make sure
that the submit template was indeed modified by the editor; but,
it is not considered if skipSubmitEdit is true.
Reported-by: Loren A. Linden Levy <lindenle@gmail.com>
Acked-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
This was talked about and reviewed back in September 2011.
I think it's ready to include in the next release.
Thanks,
-- Pete
contrib/fast-import/git-p4 | 59 ++++++++++++++++++----------
contrib/fast-import/git-p4.txt | 19 ++++++++-
t/t9805-skip-submit-edit.sh | 82 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 136 insertions(+), 24 deletions(-)
create mode 100755 t/t9805-skip-submit-edit.sh
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index b975d67..7fd8bf0 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -847,6 +847,38 @@ class P4Submit(Command, P4UserMap):
return template
+ def edit_template(self, template_file):
+ """Invoke the editor to let the user change the submission
+ message. Return true if okay to continue with the submit."""
+
+ # if configured to skip the editing part, just submit
+ if gitConfig("git-p4.skipSubmitEdit") == "true":
+ return True
+
+ # look at the modification time, to check later if the user saved
+ # the file
+ mtime = os.stat(template_file).st_mtime
+
+ # invoke the editor
+ if os.environ.has_key("P4EDITOR"):
+ editor = os.environ.get("P4EDITOR")
+ else:
+ editor = read_pipe("git var GIT_EDITOR").strip()
+ system(editor + " " + template_file)
+
+ # If the file was not saved, prompt to see if this patch should
+ # be skipped. But skip this verification step if configured so.
+ if gitConfig("git-p4.skipSubmitEditCheck") == "true":
+ return True
+
+ if os.stat(template_file).st_mtime <= mtime:
+ while True:
+ response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
+ if response == 'y':
+ return True
+ if response == 'n':
+ return False
+
def applyCommit(self, id):
print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
@@ -1001,7 +1033,7 @@ class P4Submit(Command, P4UserMap):
separatorLine = "######## everything below this line is just the diff #######\n"
- [handle, fileName] = tempfile.mkstemp()
+ (handle, fileName) = tempfile.mkstemp()
tmpFile = os.fdopen(handle, "w+")
if self.isWindows:
submitTemplate = submitTemplate.replace("\n", "\r\n")
@@ -1009,25 +1041,9 @@ class P4Submit(Command, P4UserMap):
newdiff = newdiff.replace("\n", "\r\n")
tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
tmpFile.close()
- mtime = os.stat(fileName).st_mtime
- if os.environ.has_key("P4EDITOR"):
- editor = os.environ.get("P4EDITOR")
- else:
- editor = read_pipe("git var GIT_EDITOR").strip()
- system(editor + " " + fileName)
- if gitConfig("git-p4.skipSubmitEditCheck") == "true":
- checkModTime = False
- else:
- checkModTime = True
-
- response = "y"
- if checkModTime and (os.stat(fileName).st_mtime <= mtime):
- response = "x"
- while response != "y" and response != "n":
- response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
-
- if response == "y":
+ if self.edit_template(fileName):
+ # read the edited message and submit
tmpFile = open(fileName, "rb")
message = tmpFile.read()
tmpFile.close()
@@ -1039,11 +1055,12 @@ class P4Submit(Command, P4UserMap):
if self.preserveUser:
if p4User:
# Get last changelist number. Cannot easily get it from
- # the submit command output as the output is unmarshalled.
+ # the submit command output as the output is
+ # unmarshalled.
changelist = self.lastP4Changelist()
self.modifyChangelistUser(changelist, p4User)
-
else:
+ # skip this patch
for f in editedFiles:
p4_revert(f)
for f in filesToAdd:
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index 52003ae..5044a12 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -202,11 +202,24 @@ able to find the relevant client. This client spec will be used to
both filter the files cloned by git and set the directory layout as
specified in the client (this implies --keep-path style semantics).
-git-p4.skipSubmitModTimeCheck
+git-p4.skipSubmitEdit
- git config [--global] git-p4.skipSubmitModTimeCheck false
+ git config [--global] git-p4.skipSubmitEdit false
-If true, submit will not check if the p4 change template has been modified.
+Normally, git-p4 invokes an editor after each commit is applied so
+that you can make changes to the submit message. Setting this
+variable to true will skip the editing step, submitting the change as is.
+
+git-p4.skipSubmitEditCheck
+
+ git config [--global] git-p4.skipSubmitEditCheck false
+
+After the editor is invoked, git-p4 normally makes sure you saved the
+change description, as an indication that you did indeed read it over
+and edit it. You can quit without saving to abort the submit (or skip
+this change and continue). Setting this variable to true will cause
+git-p4 not to check if you saved the change description. This variable
+only matters if git-p4.skipSubmitEdit has not been set to true.
git-p4.preserveUser
diff --git a/t/t9805-skip-submit-edit.sh b/t/t9805-skip-submit-edit.sh
new file mode 100755
index 0000000..734ccf2
--- /dev/null
+++ b/t/t9805-skip-submit-edit.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='git-p4 skipSubmitEdit config variables'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "change 1"
+ )
+'
+
+# this works because EDITOR is set to :
+test_expect_success 'no config, unedited, say yes' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ echo line >>file1 &&
+ git commit -a -m "change 2" &&
+ echo y | "$GITP4" submit &&
+ p4 changes //depot/... >wc &&
+ test_line_count = 2 wc
+ )
+'
+
+test_expect_success 'no config, unedited, say no' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ echo line >>file1 &&
+ git commit -a -m "change 3 (not really)" &&
+ printf "bad response\nn\n" | "$GITP4" submit
+ p4 changes //depot/... >wc &&
+ test_line_count = 2 wc
+ )
+'
+
+test_expect_success 'skipSubmitEdit' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ # will fail if editor is even invoked
+ git config core.editor /bin/false &&
+ echo line >>file1 &&
+ git commit -a -m "change 3" &&
+ "$GITP4" submit &&
+ p4 changes //depot/... >wc &&
+ test_line_count = 3 wc
+ )
+'
+
+test_expect_success 'skipSubmitEditCheck' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ echo line >>file1 &&
+ git commit -a -m "change 4" &&
+ "$GITP4" submit &&
+ p4 changes //depot/... >wc &&
+ test_line_count = 4 wc
+ )
+'
+
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.8.rc4.30.g3c9dc
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox