* [RFC] leaky()
From: Pierre Habouzit @ 2008-06-24 20:14 UTC (permalink / raw)
To: git; +Cc: gitster
In the parse-options thread, it appears that we may somethings leak some
tiny bits of memory on purpose. So that it makes life of people
searching for real memory leaks easier.
The second patch marks the leaks I'm responsible for in git (and that
I'm aware of) as leaky().
^ permalink raw reply
* [PATCH 1/2] Introduce leaky().
From: Pierre Habouzit @ 2008-06-24 20:14 UTC (permalink / raw)
To: git; +Cc: gitster, Pierre Habouzit
In-Reply-To: <1214338474-16822-1-git-send-email-madcoder@debian.org>
This can be used to mark allocated memory as a "leak". This can be used to
collect memory at the exit of the command, so that tools like valgrind can
be used to check for actual memory leak without noise.
COLLECT_LEAKS_AT_EXIT must be set to that purpose, else 'leaky' is the
transparent macro.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
Makefile | 5 +++++
alloc.c | 20 ++++++++++++++++++++
cache.h | 5 +++++
3 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/Makefile b/Makefile
index 9c16482..6be15c8 100644
--- a/Makefile
+++ b/Makefile
@@ -151,6 +151,8 @@ all::
# Define NO_EXTERNAL_GREP if you don't want "git grep" to ever call
# your external grep (e.g., if your system lacks grep, if its grep is
# broken, or spawning external process is slower than built-in grep git has).
+#
+# Define COLLECT_LEAKS_AT_EXIT if you want memory marked as leaky() at exit.
GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -947,6 +949,9 @@ endif
ifdef NO_EXTERNAL_GREP
BASIC_CFLAGS += -DNO_EXTERNAL_GREP
endif
+ifdef COLLECT_LEAKS_AT_EXIT
+ BASIC_CFLAGS += -DCOLLECT_LEAKS_AT_EXIT
+endif
ifeq ($(TCLTK_PATH),)
NO_TCLTK=NoThanks
diff --git a/alloc.c b/alloc.c
index 216c23a..1afe810 100644
--- a/alloc.c
+++ b/alloc.c
@@ -74,3 +74,23 @@ void alloc_report(void)
REPORT(commit);
REPORT(tag);
}
+
+#ifdef COLLECT_LEAKS_AT_EXIT
+static void **leaks;
+int leaknb, leaksz;
+
+static void release_leaks(void)
+{
+ while (leaknb-- > 0)
+ free(*leaks++);
+ free(leaks);
+}
+
+void *leaky(void *ptr)
+{
+ if (leaksz == 0)
+ atexit(&release_leaks);
+ ALLOC_GROW(leaks, leaknb + 1, leaksz);
+ return leaks[leaknb++] = ptr;
+}
+#endif
diff --git a/cache.h b/cache.h
index 101ead5..33603bb 100644
--- a/cache.h
+++ b/cache.h
@@ -777,6 +777,11 @@ int decode_85(char *dst, const char *line, int linelen);
void encode_85(char *buf, const unsigned char *data, int bytes);
/* alloc.c */
+#ifdef COLLECT_LEAKS_AT_EXIT
+extern void *leaky(void *);
+#else
+# define leaky(x) x
+#endif
extern void *alloc_blob_node(void);
extern void *alloc_tree_node(void);
extern void *alloc_commit_node(void);
--
1.5.6.120.g3adb8.dirty
^ permalink raw reply related
* [PATCH 2/2] git-rev-parse: use leaky().
From: Pierre Habouzit @ 2008-06-24 20:14 UTC (permalink / raw)
To: git; +Cc: gitster, Pierre Habouzit
In-Reply-To: <1214338474-16822-2-git-send-email-madcoder@debian.org>
Mark the mallocs that cmd_parseopt never frees as leaky so that nobody
loses time trying to fix them.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
builtin-rev-parse.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index a7860ed..02defbb 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -319,12 +319,12 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
s = strchr(sb.buf, ' ');
if (!s || *sb.buf == ' ') {
o->type = OPTION_GROUP;
- o->help = xstrdup(skipspaces(sb.buf));
+ o->help = leaky(xstrdup(skipspaces(sb.buf)));
continue;
}
o->type = OPTION_CALLBACK;
- o->help = xstrdup(skipspaces(s));
+ o->help = leaky(xstrdup(skipspaces(s)));
o->value = &parsed;
o->flags = PARSE_OPT_NOARG;
o->callback = &parseopt_dump;
@@ -349,10 +349,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
if (s - sb.buf == 1) /* short option only */
o->short_name = *sb.buf;
else if (sb.buf[1] != ',') /* long option only */
- o->long_name = xmemdupz(sb.buf, s - sb.buf);
+ o->long_name = leaky(xmemdupz(sb.buf, s - sb.buf));
else {
o->short_name = *sb.buf;
- o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
+ o->long_name = leaky(xmemdupz(sb.buf + 2, s - sb.buf - 2));
}
}
strbuf_release(&sb);
--
1.5.6.120.g3adb8.dirty
^ permalink raw reply related
* Re: why is git destructive by default? (i suggest it not be!)
From: David Jeske @ 2008-06-24 20:04 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Boaz Harrosh, git
In-Reply-To: <m31w2mlki4.fsf@localhost.localdomain>
> -- David Jeske wrote:
>> - improve the man page description of "reset --hard"
>> - standardize all the potentially destructive operations
>> (after gc) on "-f/--force" to override
>
> The thing is 'force' is not always the most descriptive word
> for the behavior that you propose enabling with --force.
I'm not talking about switching "git reset --hard" to "git reset -f". I'm
talking about requiring a "-f" option to "git reset --hard" when it would
destroy or dangle information.
(a) If you have a clean working directory which is fully checked in and has
another branch tag other than the current branch tag, then "git reset --hard
<commitish>" is non-destructive, and would complete happily.
(b) If you have local modifications to working files it would complain "hey,
your working files are dirty, 'reset --hard' will blow them away, either revert
them or use -f". This is what Boaz asked for, and I doubt it would change along
would alter workflow much for people who are using "git reset --hard" to toss
attempted patches (since they were fully committed anyhow), or even undo a
clone or pull operation. If people use it as a combo "revert and reset", they
would notice.
(c) If the current location is only pointed to by the current branch (which you
are going to move with 'reset --hard') tell the user that those changes will be
dangling and will be eligible for garbage collection if they move the branch.
What to do in this case seems more controversial. I would prefer for this to
error with "either label these changes with 'branch', or use 'reset --hard -f'
to force us to leave these in the reflog unnamed". --- Some here say that
being in the reflog is enough, and the -f is overkill here. If we define
destructive as dropping code-commits, then that's true. If we define
destructive as leaving code-commits unreferenced, then -f is warranted.
Personally, I'd rather git help me avoid dropping the NAMES to tips, because
even with GC-never, I don't really want to find myself crawling through SHA1
hashes and visualization trees to find them later, when git could have reminded
me to name a branch that would conveniently show up in 'git branch'. It's easy
enough to avoid dropping the names, or force git to not care with '-f'. I
personally would like to avoid dealing with reflog or SHA1 hashes 99% of the
time.
> 'gc' is another command that has been mentioned along
> with its '--aggressive' option.
This was an accident. When I made my "mv --aggressive" joke I was NOT intending
to reference "gc --aggressive", that is just a coincidence. I was trying to
make up another 'semi-dangerous sounding name that might or not might be
destructive". It's comical that it's in use for gc. I don't see any
relationship between "gc --aggressive" and destructive behavior.
However, there IS a situation to require a "-f" on a, because again, "-f" would
be required for operations which destroy commits. If we think commits being in
the reflog is good enough to hold onto them, and users are thinking that items
being in the reflog are 'safe', then a GC where reflog entry expiration is
going to cause DAG entries to be removed could print an error like:
error: the following entries are beyond the expiration time,
...<base branchname>/<commit-ish>: 17 commits, 78 lines, 3 authors
...use diff <commit-ish> , to see the changes
...use gc -f, to cause them to be deleted
This wouldn't happen very often, and would make "gc" a safe operation even on
trees with shorter expiration time. In fact, if this were the way it worked, I
might set my GC back from never to "30 days", because this would not only allow
me to safely cleanup junk, but it would also allow me to catch unnamed and
dangling references before they became so old I didn't remember what to name
them.
This would make a "non forced gc" safe from throwing away commits, but still
make it really easy to do so for people who want to. Likewise, we could make
any "auto-gc" that happens not forced by default.
^ permalink raw reply
* Re: why is git destructive by default? (i suggest it not be!)
From: David Jeske @ 2008-06-24 20:04 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Boaz Harrosh, git
In-Reply-To: <m31w2mlki4.fsf@localhost.localdomain>
> -- David Jeske wrote:
>> - improve the man page description of "reset --hard"
>> - standardize all the potentially destructive operations
>> (after gc) on "-f/--force" to override
>
> The thing is 'force' is not always the most descriptive word
> for the behavior that you propose enabling with --force.
I'm not talking about switching "git reset --hard" to "git reset -f". I'm
talking about requiring a "-f" option to "git reset --hard" when it would
destroy or dangle information.
(a) If you have a clean working directory which is fully checked in and has
another branch tag other than the current branch tag, then "git reset --hard
<commitish>" is non-destructive, and would complete happily.
(b) If you have local modifications to working files it would complain "hey,
your working files are dirty, 'reset --hard' will blow them away, either revert
them or use -f". This is what Boaz asked for, and I doubt it would change along
would alter workflow much for people who are using "git reset --hard" to toss
attempted patches (since they were fully committed anyhow), or even undo a
clone or pull operation. If people use it as a combo "revert and reset", they
would notice.
(c) If the current location is only pointed to by the current branch (which you
are going to move with 'reset --hard') tell the user that those changes will be
dangling and will be eligible for garbage collection if they move the branch.
What to do in this case seems more controversial. I would prefer for this to
error with "either label these changes with 'branch', or use 'reset --hard -f'
to force us to leave these in the reflog unnamed". --- Some here say that
being in the reflog is enough, and the -f is overkill here. If we define
destructive as dropping code-commits, then that's true. If we define
destructive as leaving code-commits unreferenced, then -f is warranted.
Personally, I'd rather git help me avoid dropping the NAMES to tips, because
even with GC-never, I don't really want to find myself crawling through SHA1
hashes and visualization trees to find them later, when git could have reminded
me to name a branch that would conveniently show up in 'git branch'. It's easy
enough to avoid dropping the names, or force git to not care with '-f'. I
personally would like to avoid dealing with reflog or SHA1 hashes 99% of the
time.
> 'gc' is another command that has been mentioned along
> with its '--aggressive' option.
This was an accident. When I made my "mv --aggressive" joke I was NOT intending
to reference "gc --aggressive", that is just a coincidence. I was trying to
make up another 'semi-dangerous sounding name that might or not might be
destructive". It's comical that it's in use for gc. I don't see any
relationship between "gc --aggressive" and destructive behavior.
However, there IS a situation to require a "-f" on a, because again, "-f" would
be required for operations which destroy commits. If we think commits being in
the reflog is good enough to hold onto them, and users are thinking that items
being in the reflog are 'safe', then a GC where reflog entry expiration is
going to cause DAG entries to be removed could print an error like:
error: the following entries are beyond the expiration time,
...<base branchname>/<commit-ish>: 17 commits, 78 lines, 3 authors
...use diff <commit-ish> , to see the changes
...use gc -f, to cause them to be deleted
This wouldn't happen very often, and would make "gc" a safe operation even on
trees with shorter expiration time. In fact, if this were the way it worked, I
might set my GC back from never to "30 days", because this would not only allow
me to safely cleanup junk, but it would also allow me to catch unnamed and
dangling references before they became so old I didn't remember what to name
them.
This would make a "non forced gc" safe from throwing away commits, but still
make it really easy to do so for people who want to. Likewise, we could make
any "auto-gc" that happens not forced by default.
^ permalink raw reply
* Re: [PATCH] pre-commit hook should ignore carriage returns at EOL
From: Jakub Narebski @ 2008-06-24 20:09 UTC (permalink / raw)
To: Ian Hilt; +Cc: Christian Holtje, git, gitster
In-Reply-To: <alpine.LFD.1.10.0806241548140.32759@sys-0.hiltweb.site>
On Tue, 24 Jun 2008, Ian Hilt wrote:
> On Tue, 24 Jun 2008 at 12:05pm -0700, Jakub Narebski wrote:
>
> > I don't think so, because you want next to test for whitespace
> > where it _doesn't_ end in \r, i.e. this condition is here
> > because of the 'else' clause. IIRC.
>
> What I'm suggesting is this,
>
> if (/\s\r$/) {
> bad_line("trailing whitespace", $_);
> } else {
> if (/\s$/) {
> bad_line("trailing whitespace", $_);
> }
> }
>
> Why only test for \r when all you want to know is whether there is
> whitespace before \r ? If there isn't whitespace and \r at the end
> of a line, then only test for whitespace at the end of a line.
Unfortunately \r matches \s (is whitespace), so if line ends with CR LF
("\r\n") it wouldn't match first regexp, so it would go to 'else'
clause, where it would match /\s$/ and it shouldn't.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [NON-TOY PATCH] git bisect: introduce 'fixed' and 'unfixed'
From: Michael Haggerty @ 2008-06-24 20:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git
In-Reply-To: <alpine.DEB.1.00.0806241808400.9925@racer>
Johannes Schindelin wrote:
> When you look for a fix instead of a regression, it can be quite hard
> to twist your brain into choosing the correct bisect command between
> 'git bisect bad' and 'git bisect good'.
>
> So introduce the commands 'git bisect fixed' and 'git bisect unfixed'.
It seems to me that your problem is that git-bisect requires the "good"
revision to be older than the "bad" one. If this requirement were
removed, would there still be a need for "fixed" vs. "unfixed"?
A bisection search doesn't care what labels are applied to the two
endpoints, as it only looks for transitions between the labels.
Therefore it should be easy to teach git-bisect to locate either kind of
transition, "bad" -> "good" or "good" -> "bad", depending only on where
the user places the original "good" and "bad" tags.
Michael
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: Jakub Narebski @ 2008-06-24 20:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Miklos Vajna, Pieter de Bie, Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0806242033570.9925@racer>
Hello!
Johannes Schindelin wrote:
> On Tue, 24 Jun 2008, Jakub Narebski wrote:
>
> > git-shell hackery won't solve problem, because not everybody is using
> > git-shell.
>
> The problem is not git-shell vs git potty.
>
> The problem is that not everybody magically updates their clients to ask
> for dash-less form.
What I meant here by "git-shell hackery" was for git-shell to
automagically redirect request for "git-receive-pack" to "git receive-pack"
(or "$GIT_EXEC_PATH/git-receive-pack").
But, as I said, it isn't complete solution. Leaving git-*-pack in $PATH
for the time being (what I said) is.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [NON-TOY PATCH] git bisect: introduce 'fixed' and 'unfixed'
From: SZEDER Gábor @ 2008-06-24 19:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git
In-Reply-To: <alpine.DEB.1.00.0806241808400.9925@racer>
On Tue, Jun 24, 2008 at 06:09:28PM +0100, Johannes Schindelin wrote:
> So introduce the commands 'git bisect fixed' and 'git bisect unfixed'.
And maybe this one squashed on it, to add completion support for the
new subcommands.
---
contrib/completion/git-completion.bash | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ebf7cde..014adab 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -511,7 +511,9 @@ _git_add ()
_git_bisect ()
{
- local subcommands="start bad good reset visualize replay log"
+ local subcommands="
+ start bad good reset visualize replay log fixed unfixed
+ "
local subcommand="$(__git_find_subcommand "$subcommands")"
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
--
1.5.6.64.g7dc1df
^ permalink raw reply related
* Re: [PATCH] pre-commit hook should ignore carriage returns at EOL
From: Ian Hilt @ 2008-06-24 19:54 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Ian Hilt, Christian Holtje, git, gitster
In-Reply-To: <m363rylknc.fsf@localhost.localdomain>
On Tue, 24 Jun 2008 at 12:05pm -0700, Jakub Narebski wrote:
> I don't think so, because you want next to test for whitespace
> where it _doesn't_ end in \r, i.e. this condition is here
> because of the 'else' clause. IIRC.
What I'm suggesting is this,
if (/\s\r$/) {
bad_line("trailing whitespace", $_);
} else {
if (/\s$/) {
bad_line("trailing whitespace", $_);
}
}
Why only test for \r when all you want to know is whether there is
whitespace before \r ? If there isn't whitespace and \r at the end of a
line, then only test for whitespace at the end of a line.
--
Ian Hilt
Ian.Hilt (at) gmx.com
GnuPG key: 0x4AFC1EE3
^ permalink raw reply
* Re: [PATCH 0/3] Manual editing for 'add' and 'add -p'
From: Miklos Vajna @ 2008-06-24 19:53 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jeff King, Johannes Schindelin, Junio C Hamano
In-Reply-To: <200806242108.08654.trast@student.ethz.ch>
[-- Attachment #1: Type: text/plain, Size: 407 bytes --]
On Tue, Jun 24, 2008 at 09:07:54PM +0200, Thomas Rast <trast@student.ethz.ch> wrote:
> I hope this series turns out right in mail; I'm struggling a bit to
> properly import the mails to KMail. Most annoyingly, format-patch
> apparently cannot turn other people's patches into mails by myself
> with an appropriate "From:" line. :-(
Why don't you just use git-send-email? It already handles the situation.
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Jeff King @ 2008-06-24 19:46 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806241036560.2926@woody.linux-foundation.org>
On Tue, Jun 24, 2008 at 10:44:24AM -0700, Linus Torvalds wrote:
> And to solve that _single_ problem, I wanted parse_options() to be able
> to:
>
> - stop at unknown options (so that I could hand-parse them)
>
> - ignore unknown options (so that I could parse all the ones I knew
> about, and then either hand-parse the rest, or just pass them on to
> _another_ function that used some arbitrary model to parse the parts it
> knew about)
>
> See? Single issue.
OK, fair enough. You are working towards a single goal. But I think
there is a flaw with that one sub-part of your goal (specifically your
second bullet point). I don't care about the other parts, and in fact I
even said "this is a fine way of doing that".
> And I even sent out a single patch for it. That single patch, btw, was
> even rather small.
>
> Did you ever look at that patch? Did you ever look at the code I was
> trying to have use parse_options()? No.
How can you even say something so ridiculous? Were you sitting at my
computer all day yesterday, making sure I didn't read your patch? No?
Funnily enough, I was there. And guess what I saw? Yes, it was me
READING YOUR PATCH (in a mirror, of course).
But I don't expect you to have a camera installed over my shoulder. So I
guess you would have had to just content yourself with THE EMAILS WHERE I
MENTION YOUR PATCH AND ITS EFFECTS.
> You constantly try to change the discussion to be about SOMETHIGN ELSE.
>
> For example, you keep on bringing up this TOTAL RED HERRING:
>
> > - It is impossible for that mechanism to be correct in all cases, due
> > to the syntactic rules for command lines. IOW, you cannot parse an
> > element until you know the function of the element to the left.
>
> NOBODY F*CKIGN CARES!
I care. Apparently Junio cared in the thread that I pointed out earlier.
Other commands that parse the options will care, if we are to ever port
them to parse_options. I understand that you don't care about those
things, and you only want git-blame to work. I am merely trying to help
amortize work that goes into fixing git-blame into helping to fix other
commands. To do that, I pointed out a flaw that might prevent the same
solution being used again.
I understand that you are interested in incremental change, and that
doing something now in git-blame is better than doing nothing while we
wait for a longer, more complete solution to arrive. But did I say "No,
we shouldn't apply this patch from Linus because it's not the optimal
solution?" No. Instead, I said "In the meantime, your patch does not
make this particular problem any worse."
> Because what builtin-blame.c *already* does is exactly that.
I KNOW AND I EVEN SAID THAT. But here it is in case you are hard of
hearing:
From 20080623195314.GA29569@sigill.intra.peff.net:
"In the meantime, I don't think your patch makes anything _worse_,
since we already have these sorts of bugs in the current parsing
code."
From 20080623183358.GA28941@sigill.intra.peff.net:
"It's worse than that...Try (with current git-blame...
$ git blame -n 1 git.gc
fatal: bad revision '1'
> This is what I'm complaining about with your totally IDIOTIC mails. You're
> ignoring reality, and talking about how things "ought to work", and never
> ever apparently looked at how things *do* work.
Again, how in the world can you say that I didn't look at how things do
work WHEN I CUT AND PASTED A SAMPLE TRANSCRIPT SHOWING HOW THEY DO WORK?
BTW, this is the third time you have mentioned the phrase "ought to
work" in a way that is totally disingenuous about what I actually said.
I can understand that perhaps I was not clear in the initial statement.
But I would have thought the other two times I explained it further
would have made it clear. So did you truly not understand my point, or
are you just being intentionally rude?
> The fact is, the one program I wanted to convert already does exactly what
> you claim is "impossible to be correct in all cases".
And it's not correct as it is now. And we have lived with it. Which is
why I didn't say "your proposal makes things worse." In fact, I said the
opposite.
> So either shut up, or send a patch to fix what you consider a bug. I'm
> waiting.
I already volunteered to work on it. I'm sorry that the patch didn't
materialize immediately, but I was working on other parts of git, just
as I mentioned when I said I would work on it.
In the meantime, I think Pierre has proposed an alternate approach that
also has promise, so I think it makes sense to see how he progresses
with that rather than potentially duplicate work.
And I will shut up now, because this is obviously getting nowhere. I
just find your responses so rude and misinformed not about technical
matters, but about my statements and my intent that I feel the need to
publicly defend myself.
-Peff
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Pierre Habouzit @ 2008-06-24 19:43 UTC (permalink / raw)
To: Linus Torvalds, Junio C Hamano, Jeff King, Johannes Schindelin,
Git Mailing List <git
In-Reply-To: <20080624193028.GC9189@artemis.madism.org>
[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]
On Tue, Jun 24, 2008 at 07:30:28PM +0000, Pierre Habouzit wrote:
> Though for the win32 port where fork is replaced with threads, well,
> it may cause some issues, so I was reluctant wrt them. Of course it's
> unlikely that it will cause problems, but one never knows ;)
OTOH if it's really a problem, we could easily use a custom allocator
in parse-options that registers a pthread_cleanup_push (or whichever
atexit() like pthread use, I'm not really into threads) that would
cleanup this[0]. So maybe just leaking is the simplest way.
As a consequence it makes the restriction about not keeping pointers
to argv during the parse go away, and I frankly don't like this
restriction, it's counterintuitive and very error prone, which arguably
means it's a really bad design.
[0] In fact we may even want to have some kind of
xmalloc_that_is_freed_at_exit that could be used where we
purposely leak things, and cleanse that on atexit() for POSIX
platforms and whatever win32 people like to use in their threads.
It has the nice property to avoid lots of false positives when you
are using valgrind and other memory checkers.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: Johannes Schindelin @ 2008-06-24 19:34 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Miklos Vajna, Pieter de Bie, Junio C Hamano, git
In-Reply-To: <m3wskek4wt.fsf@localhost.localdomain>
Hi,
On Tue, 24 Jun 2008, Jakub Narebski wrote:
> git-shell hackery won't solve problem, because not everybody is using
> git-shell.
The problem is not git-shell vs git potty.
The problem is that not everybody magically updates their clients to ask
for dash-less form.
Ciao,
Dscho
^ permalink raw reply
* Re: Segmentation fault on http clone, post-1.5.6
From: Mike Hommey @ 2008-06-24 19:34 UTC (permalink / raw)
To: Teemu Likonen; +Cc: Jeff King, git, Nicolas Pitre
In-Reply-To: <20080624185723.GA3368@mithlond.arda.local>
On Tue, Jun 24, 2008 at 09:57:23PM +0300, Teemu Likonen wrote:
> Jeff King wrote (2008-06-24 12:40 -0400):
>
> > On Tue, Jun 24, 2008 at 04:04:57PM +0300, Teemu Likonen wrote:
> >
> > > With the current "master" branch version (29b0d0191) I get
> > > segmentation fault when trying to clone a git repo with http
> > > protocol. Tried a couple of times and it's always reproducible. You
> > > can test with the following repository (about 5.5 MB):
> > >
> > > git clone http://www.iki.fi/tlikonen/voikko.git
> >
> > I can't reproduce the segfault here.
> >
> > > I also build git from the tag v1.5.6 and it seems to work fine, so
> > > I guess the bug was introduced after 1.5.6.
> >
> > That sounds like an excellent opportunity to learn about git-bisect.
> > Can you try bisecting the bug and reporting back the problematic
> > commit?
>
> Indeed. I have now officially bisected the problem and the first bad or
> problematic commit is 8eca0b47 "implement some resilience against pack
> corruptions" (hence Cc to Nicolas, the author). This is always
> reproducible in my Debian 4.0 box.
Does it crash in verify_packfile() ?
Mike
^ permalink raw reply
* Re: Segmentation fault on http clone, post-1.5.6
From: Nicolas Pitre @ 2008-06-24 19:34 UTC (permalink / raw)
To: Teemu Likonen; +Cc: Jeff King, git
In-Reply-To: <20080624185723.GA3368@mithlond.arda.local>
On Tue, 24 Jun 2008, Teemu Likonen wrote:
> Jeff King wrote (2008-06-24 12:40 -0400):
>
> > On Tue, Jun 24, 2008 at 04:04:57PM +0300, Teemu Likonen wrote:
> >
> > > With the current "master" branch version (29b0d0191) I get
> > > segmentation fault when trying to clone a git repo with http
> > > protocol. Tried a couple of times and it's always reproducible. You
> > > can test with the following repository (about 5.5 MB):
> > >
> > > git clone http://www.iki.fi/tlikonen/voikko.git
> >
> > I can't reproduce the segfault here.
> >
> > > I also build git from the tag v1.5.6 and it seems to work fine, so
> > > I guess the bug was introduced after 1.5.6.
> >
> > That sounds like an excellent opportunity to learn about git-bisect.
> > Can you try bisecting the bug and reporting back the problematic
> > commit?
>
> Indeed. I have now officially bisected the problem and the first bad or
> problematic commit is 8eca0b47 "implement some resilience against pack
> corruptions" (hence Cc to Nicolas, the author). This is always
> reproducible in my Debian 4.0 box.
I'm trying to reproduce your segfault with current master
(v1.5.6-56-g29b0d01) but I just can't.
Can you provide a gdb backtrace of the segfault? What my patch does is
to return NULL in all cases when an object exists but can't be read
instead of dying. So if some user of read_sha1_file() is not checking
for a returned null pointer then a null pointer dereference is most
likely.
Note that read_sha1_file() could have returned NULL even before my
patch, but not necessarily in all cases.
Nicolas
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: Jakub Narebski @ 2008-06-24 19:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Miklos Vajna, Pieter de Bie, Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0806242007150.9925@racer>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Tue, 24 Jun 2008, Miklos Vajna wrote:
> >
> > Using fc48199 ("Merge branch 'master' into next", which includes
> > ed99a225) on the server, v1.5.6 on the client, I get:
> >
> > $ git clone server:/home/vmiklos/git/test next
> > Initialize next/.git
> > Initialized empty Git repository in /home/vmiklos/scm/git/next/.git/
> > vmiklos@server's password:
> > bash: git-upload-pack: command not found
> > fatal: The remote end hung up unexpectedly
>
> Hmm. Probably the client needs to be newer, too. This is going to be
> painful. Junio?
It looks like git-upload-pack and git-receive-pack would
have to be left in $PATH, at least till old clients die
of old age ;-)
git-shell hackery won't solve problem, because not everybody is using
git-shell.
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Pierre Habouzit @ 2008-06-24 19:30 UTC (permalink / raw)
To: Linus Torvalds
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <alpine.LFD.1.10.0806241001140.2926@woody.linux-foundation.org>
[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]
On Tue, Jun 24, 2008 at 05:05:50PM +0000, Linus Torvalds wrote:
>
>
> On Tue, 24 Jun 2008, Pierre Habouzit wrote:
> >
> > Actually this doesn't work because it may point into the strbuf that
> > will be invalidated later. Our sole option here is to leak some memory I
> > fear.
>
> I think leaking memory is ok (it's obviously going to be bounded by the
> size of the arguments you pass into a program), but I also think you can
> just change the option strings in place.
>
> Yeah, I know - it's impolite, and we even marked things "const char", but
> "const" in C is just a politeness thing, we can just choose to have a
> function with a big comment that changes the string anyway. We'll have to
> make sure that the few places that actually create argument strings by
> hand (ie not from the ones supplied by a real "execve()") not do them so
> that they need splitting (but no current ones would need to, obviously,
> since splitting the argumens isn't even supported yet).
>
> Or, if people hate that, just leak a few malloc'ed areas. That's arguably
> the more straightforward way.
The problem is that we sometimes feed the option parser with hand
crafted const char *[] where strings are indeed in rodata, and well,
changing things is not only impolite, it tends to SIG{BUS,SEGV} ;)
But I think too that leaking is an option. git rev-parse --parseopt
already leaks in the same ways e.g.
Though for the win32 port where fork is replaced with threads, well,
it may cause some issues, so I was reluctant wrt them. Of course it's
unlikely that it will cause problems, but one never knows ;)
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [NON-TOY PATCH] git bisect: introduce 'fixed' and 'unfixed'
From: Johannes Schindelin @ 2008-06-24 19:26 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Jeff King, git
In-Reply-To: <alpine.LNX.1.00.0806241516440.19665@iabervon.org>
Hi,
On Tue, 24 Jun 2008, Daniel Barkalow wrote:
> On Tue, 24 Jun 2008, Jeff King wrote:
>
> > On Tue, Jun 24, 2008 at 06:09:28PM +0100, Johannes Schindelin wrote:
> >
> > > And this is my first attempt at a proper patch for it.
> > >
> > > Now with documentation, and hopefully all places where the
> > > user is being told about a "bad" commit.
> >
> > This looks reasonably sane to me. The only thing I can think of that
> > we're missing is that "git bisect visualize" will still show the refs as
> > "bisect/bad" and "bisect/good".
> >
> > To fix that, you'd have to ask people to start the bisect by saying "I
> > am bisecting to find a fix, not a breakage." And then you could change
> > the refnames and all of the messages as appropriate.
>
> That would also be a good way of taking care of the problem where someone
> gets distracted while running a slow test, forgets what they're looking
> for, and marks the result as "bad" instead of "unfixed".
Feel free to rework my patch.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.
From: Pierre Habouzit @ 2008-06-24 19:27 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, gitster, peff, Johannes.Schindelin
In-Reply-To: <alpine.LFD.1.10.0806241015390.2926@woody.linux-foundation.org>
[-- Attachment #1: Type: text/plain, Size: 994 bytes --]
On Tue, Jun 24, 2008 at 05:18:56PM +0000, Linus Torvalds wrote:
>
>
> On Tue, 24 Jun 2008, Pierre Habouzit wrote:
> >
> > This way, argv[0] isn't clobbered, to the cost of maybe not having a
> > resulting NULL terminated argv array.
>
> Umm. I think it's much easier to do by always having
>
> ctx->out = argv;
>
> and then just initializing cpix to 0 or 1:
>
> ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
>
> because now parse_options_end() doesn't need to play games any more. It
> doesn't need to care about PARSE_OPT_KEEP_ARGV0, it can just do exactly
> what it always used to do, because "ctx->cpidx + ctx->argc" automatically
> does the right thing (it is both the return value _and_ the index that you
> should fill with NULL.
Oh right, ack it's more elegant.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH 5/7] parse-opt: fake short strings for callers to believe in.
From: Pierre Habouzit @ 2008-06-24 19:26 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, gitster, peff, Johannes.Schindelin
In-Reply-To: <alpine.LFD.1.10.0806241019370.2926@woody.linux-foundation.org>
[-- Attachment #1: Type: text/plain, Size: 984 bytes --]
On Tue, Jun 24, 2008 at 05:20:28PM +0000, Linus Torvalds wrote:
>
>
> On Tue, 24 Jun 2008, Pierre Habouzit wrote:
> >
> > If we begin to parse -abc and that the parser knew about -a and -b, it
> > will fake a -c switch for the caller to deal with.
> >
> > Of course in the case of -acb (supposing -c is not taking an argument) the
> > caller will have to be especially clever to do the same thing. We could
> > think about exposing an API to do so if it's really needed, but oh well...
>
> Well, if the other parser is _also_ parse_options() (ie you just cascade
> them incrementally in a loop), then the other parser should get it right
> automatically. No?
Exactly. There are minor glitches wrt the help generation to deal
with, but for pure parsing issues yes, it will work.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: exit status 141 from git-svn
From: Frederik Hohlfeld @ 2008-06-24 19:24 UTC (permalink / raw)
To: git
In-Reply-To: <loom.20080623T145909-992@post.gmane.org>
Frederik Hohlfeld <frederik.hohlfeld <at> gmail.com> writes:
> I have init'ed a git repository with git svn init.
>
> Now I'm using git svn fetch, but it stops after just a few files/revisions. The
> exit status is 141.
>
> What does this 141 want to tell me?
No one?
Both git svn clone and git svn fetch stop after just a dozen or so revisions and
only continue after calling the command again. For yet another few revisions.
I'd like to get a clue what's happening. A git problem? A svn problem?
Current Git Windows version.
Thanks
Frederik Hohlfeld
^ permalink raw reply
* Re: [NON-TOY PATCH] git bisect: introduce 'fixed' and 'unfixed'
From: Daniel Barkalow @ 2008-06-24 19:22 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20080624174157.GB9500@sigill.intra.peff.net>
On Tue, 24 Jun 2008, Jeff King wrote:
> On Tue, Jun 24, 2008 at 06:09:28PM +0100, Johannes Schindelin wrote:
>
> > And this is my first attempt at a proper patch for it.
> >
> > Now with documentation, and hopefully all places where the
> > user is being told about a "bad" commit.
>
> This looks reasonably sane to me. The only thing I can think of that
> we're missing is that "git bisect visualize" will still show the refs as
> "bisect/bad" and "bisect/good".
>
> To fix that, you'd have to ask people to start the bisect by saying "I
> am bisecting to find a fix, not a breakage." And then you could change
> the refnames and all of the messages as appropriate.
That would also be a good way of taking care of the problem where someone
gets distracted while running a slow test, forgets what they're looking
for, and marks the result as "bad" instead of "unfixed".
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* [PATCH v2] pre-commit hook should ignore carriage returns at EOL
From: Christian Holtje @ 2008-06-24 19:21 UTC (permalink / raw)
To: git; +Cc: gitster
When commit files that use DOS style CRLF end-of-lines, the pre-commit
hook would raise an error. When combined with the fact that the hooks
get activated by default on windows, it makes life difficult for
people working with DOS files.
This patch causes the pre-commit hook to deal with crlf files
correctly.
Signed-off-by: Christian Höltje <docwhat@gmail.com>
---
t/t7503-template-hook--pre-commit.sh | 75 +++++++++++++++++++++++++
+++++++++
templates/hooks--pre-commit | 10 ++++-
2 files changed, 83 insertions(+), 2 deletions(-)
create mode 100755 t/t7503-template-hook--pre-commit.sh
diff --git a/t/t7503-template-hook--pre-commit.sh b/t/t7503-template-
hook--pre-commit.sh
new file mode 100755
index 0000000..c78a507
--- /dev/null
+++ b/t/t7503-template-hook--pre-commit.sh
@@ -0,0 +1,75 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Christian Höltje
+#
+
+test_description='t7503 templates-hooks--pre-commit
+
+This test verifies that the pre-commit hook shipped with
+git by default works correctly.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'verify that autocrlf is unset' '
+ if git config core.autocrlf
+ then
+ false
+ else
+ test $? -eq 1
+ fi
+'
+
+test_expect_success 'lf without hook' '
+
+ printf "foo" > lf.txt &&
+ git add lf.txt &&
+ git commit -m "lf without hook" lf.txt
+
+'
+
+test_expect_success 'crlf without hook' '
+
+ printf "foo\r" > crlf.txt &&
+ git add crlf.txt &&
+ git commit -m "crlf without hook" crlf.txt
+
+'
+
+# Set up the pre-commit hook.
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+mkdir -p "${HOOKDIR}"
+cp -r "${HOOKDIR}-disabled/pre-commit" "${HOOKDIR}/pre-commit"
+chmod +x "${HOOKDIR}/pre-commit"
+
+test_expect_success 'lf with hook' '
+
+ printf "bar" >> lf.txt &&
+ git add lf.txt &&
+ git commit -m "lf with hook" lf.txt
+
+'
+test_expect_success 'crlf with hook' '
+
+ printf "bar\r" >> crlf.txt &&
+ git add crlf.txt &&
+ git commit -m "crlf with hook" crlf.txt
+
+'
+
+test_expect_success 'lf with hook white-space failure' '
+
+ printf "bar " >> lf.txt &&
+ git add lf.txt &&
+ ! git commit -m "lf with hook" lf.txt
+
+'
+test_expect_success 'crlf with hook white-space failure' '
+
+ printf "bar \r" >> crlf.txt &&
+ git add crlf.txt &&
+ ! git commit -m "crlf with hook" crlf.txt
+
+'
+
+test_done
diff --git a/templates/hooks--pre-commit b/templates/hooks--pre-commit
index b25dce6..335ca09 100644
--- a/templates/hooks--pre-commit
+++ b/templates/hooks--pre-commit
@@ -55,8 +55,14 @@ perl -e '
if (s/^\+//) {
$lineno++;
chomp;
- if (/\s$/) {
- bad_line("trailing whitespace", $_);
+ if (/\r$/) {
+ if (/\s\r$/) {
+ bad_line("trailing whitespace", $_);
+ }
+ } else {
+ if (/\s$/) {
+ bad_line("trailing whitespace", $_);
+ }
}
if (/^\s* \t/) {
bad_line("indent SP followed by a TAB", $_);
--
1.5.5.4
^ permalink raw reply related
* Re: [PATCH] pre-commit hook should ignore carriage returns at EOL
From: Christian Holtje @ 2008-06-24 19:16 UTC (permalink / raw)
To: Ian Hilt; +Cc: git
In-Reply-To: <alpine.LFD.1.10.0806241418360.32759@sys-0.hiltweb.site>
On Jun 24, 2008, at 2:26 PM, Ian Hilt wrote:
> Perhaps you want to use printf "foo\r" instead? echo "foo\r" does not
> produce a CR on my system.
...
> Wouldn't it be less redundant to do a test for \s\r$ here instead of
> testing for \r$ and then \s\r$?
The code is checking for \r$ and then doing a different space check
depending on that, not one after another.
Thanks for the feedback. I'll put up v2 in a second.
Ciao!
^ permalink raw reply
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