* Re: Evaluation of ref-api branch status
From: Junio C Hamano @ 2011-12-06 6:57 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git discussion list
In-Reply-To: <4EDDAC4A.4030805@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 12/05/2011 07:26 PM, Junio C Hamano wrote:
> ...
>> 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.
>
> OK, then, I will try to re-roll the series on top of master, and build
> the equivalent of your quick-fix into the logical point in the series.
These quick-fixes were necessary _only_ because the queued topics predate
the real fix already in 1.7.8 (and I generally refuse to criss cross merge
from master to topics), so I expect you won't need their equivalents if
the topic is rebased on top of 1.7.8
> ... How much time to I have to work on this
> while still leaving enough time to comfortably integrate it into 1.7.9?
I am hoping we can have rc0 very early next year [*1*].
[Reference]
*1* https://www.google.com/calendar/embed?src=jfgbl2mrlipp4pb6ieih0qr3so%40group.calendar.google.com&ctz=America/Los_Angeles
^ permalink raw reply
* Re: Proposal: create meaningful aliases for git reset's hard/soft/mixed
From: Philippe Vaucher @ 2011-12-06 7:34 UTC (permalink / raw)
To: Phil Hord; +Cc: Junio C Hamano, git, Christian Couder
In-Reply-To: <CABURp0pmnsgE1ywW-W2+QFNci=3Lm=JKj9Y3U8zjh8+Cg_NA6Q@mail.gmail.com>
> Think about why you need to use git-reset. Why do new users need to
> use git-reset? What is it they are after?
Ok, so let's forget about git reset and let's focus on the features
instead. If I got it right you suggested the features that people
wants most often are uncommit, unadd/undelete and undo. Here's a new
proposal (based on your input):
uncommit: git jump <commit> (currently "git reset --soft <commit>")
unadd/undelete: git unstage file (currently "git reset --mixed file"
(with "git checkout file" for deleted files)
undo: git checkout --force --clean <commit> (currently "git reset
--hard <commit> && git clean -fd")
So, let's try out some scenarios:
1) Newbie user clones/pulls a repository from somewhere. He hacks
around and then things go bad, and he decides to scratch away
everything he did to make sure things are like they're supposed to be.
He'd then type "git checkout --force --clean master". If he didn't
introduce new files, he would simply type "git checkout --force
master"
2) Newbie adds some file to the index, then realise he added one too
many. He wants to remove it from being added. He'd then type "git
unstage file".
3) Average user creates a commit and suddenly realise he actually
wanted to split that commit in two (he cannot use --amend, and he's
not a rebase -i guru yet). Or he did a "temp" commit because he don't
know about "git stash" yet and wants to discard it. He wants to simply
go back to the previous state while keeping his changes in the index
and the worktree. He'd then type "git jump HEAD^1".
Feel free to add more scenarios!
Philippe
^ permalink raw reply
* Query on git commit amend
From: Viresh Kumar @ 2011-12-06 8:23 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Shiraz HASHIM
Hello,
Suppose i want to add few new changes to my last commit (HEAD).
The way i do it is
$ git add all_changed_files
$ git commit --amend
OR
$ git commit --amend -a
With both these ways, i get a screen to edit the message too.
I want to know if there is a way to skip this screen.
i.e.
$ git commit --amend -a -some_other_option
which simply adds new changes to existing commit, without asking to change
message.
If there is no such way, then can we add a patch for this, if it looks a valid
case.
--
viresh
^ permalink raw reply
* Re: Query on git commit amend
From: Konstantin Khomoutov @ 2011-12-06 9:01 UTC (permalink / raw)
To: Viresh Kumar; +Cc: git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <4EDDD0E4.6040003@st.com>
On Tue, 6 Dec 2011 13:53:00 +0530
Viresh Kumar <viresh.kumar@st.com> wrote:
> Suppose i want to add few new changes to my last commit (HEAD).
> The way i do it is
> $ git add all_changed_files
> $ git commit --amend
>
> OR
> $ git commit --amend -a
>
> With both these ways, i get a screen to edit the message too.
>
> I want to know if there is a way to skip this screen.
>
> i.e.
> $ git commit --amend -a -some_other_option
>
> which simply adds new changes to existing commit, without asking to
> change message.
>
> If there is no such way, then can we add a patch for this, if it
> looks a valid case.
git commit --amend -C HEAD
^ permalink raw reply
* Re: Query on git commit amend
From: Johannes Sixt @ 2011-12-06 9:11 UTC (permalink / raw)
To: Viresh Kumar; +Cc: git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <4EDDD0E4.6040003@st.com>
Am 12/6/2011 9:23, schrieb Viresh Kumar:
>
> Hello,
>
> Suppose i want to add few new changes to my last commit (HEAD).
> The way i do it is
> $ git add all_changed_files
> $ git commit --amend
>
> OR
> $ git commit --amend -a
>
> With both these ways, i get a screen to edit the message too.
>
> I want to know if there is a way to skip this screen.
>
> i.e.
> $ git commit --amend -a -some_other_option
>
> which simply adds new changes to existing commit, without asking to change
> message.
$ git commit --amend -a -C HEAD
But let's count keystrokes (after -a):
<BLANK>-<SHIFT>C<BLANK>HEAD<ENTER>
10 keystrokes (more if you release SHIFT before D)
But if vi pops up you have:
<ENTER><SHIFT>ZZ
4 keystrokes
Where is the advantage of the option?
-- Hannes
^ permalink raw reply
* Re: Query on git commit amend
From: Viresh Kumar @ 2011-12-06 9:20 UTC (permalink / raw)
To: Johannes Sixt, flatworm; +Cc: git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <4EDDDC38.8080108@viscovery.net>
On 12/6/2011 2:41 PM, Johannes Sixt wrote:
> Am 12/6/2011 9:23, schrieb Viresh Kumar:
> $ git commit --amend -a -C HEAD
>
> But let's count keystrokes (after -a):
>
> <BLANK>-<SHIFT>C<BLANK>HEAD<ENTER>
> 10 keystrokes (more if you release SHIFT before D)
>
> But if vi pops up you have:
>
> <ENTER><SHIFT>ZZ
> 4 keystrokes
>
> Where is the advantage of the option?
>
Thanks guys.
@Johannes: You are right but, i will make an alias for the entire command.
So keystrokes are same for me. :)
--
viresh
^ permalink raw reply
* Re: Query on git commit amend
From: Viresh Kumar @ 2011-12-06 9:22 UTC (permalink / raw)
To: Johannes Sixt; +Cc: flatworm, git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <4EDDDE5C.6040200@st.com>
On 12/6/2011 2:50 PM, Viresh Kumar wrote:
> On 12/6/2011 2:41 PM, Johannes Sixt wrote:
>> Am 12/6/2011 9:23, schrieb Viresh Kumar:
>> $ git commit --amend -a -C HEAD
>>
>> But let's count keystrokes (after -a):
>>
>> <BLANK>-<SHIFT>C<BLANK>HEAD<ENTER>
>> 10 keystrokes (more if you release SHIFT before D)
>>
>> But if vi pops up you have:
>>
>> <ENTER><SHIFT>ZZ
>> 4 keystrokes
>>
>> Where is the advantage of the option?
>>
>
> Thanks guys.
>
> @Johannes: You are right but, i will make an alias for the entire command.
> So keystrokes are same for me. :)
>
There is one more benefit, we don't have to wait for git. We can simply switch to
some other window and work. And can write further command on the same window, in
the time git processes commit --amend.
--
viresh
^ permalink raw reply
* Re: Roadmap for 1.7.9
From: Nguyen Thai Ngoc Duy @ 2011-12-06 11:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd3c2lr36.fsf@alter.siamese.dyndns.org>
On Tue, Dec 6, 2011 at 3:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I expect the following will not make much progress without further
> discussion:
>
> * Ignored vs Precious (Nguyen Thai Ngoc Duy)
I'm working on adding --exclude-standard and --exclude-from to
read-tree. Apart from that I don't plan on supporting precious files
because I don't use them.
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Nguyen Thai Ngoc Duy @ 2011-12-06 11:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8vmqi98f.fsf@alter.siamese.dyndns.org>
On Tue, Dec 6, 2011 at 12:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Here are the topics that have been cooking. Commits prefixed with '-' are
> only in 'pu' (proposed updates) while commits prefixed with '+' are in
> 'next'.
What can I do to get "build in repack" [1] series in or moved forward?
[1] http://thread.gmane.org/gmane.comp.version-control.git/185190
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Erik Faye-Lund @ 2011-12-06 11:22 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git Mailing List, Johannes Sixt, Stephan Beyer
In-Reply-To: <20111206055239.GA20671@sigill.intra.peff.net>
On Tue, Dec 6, 2011 at 6:52 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 05, 2011 at 09:01:52PM -0800, Junio C Hamano wrote:
>
>> * jk/credentials (2011-11-28) 20 commits
>> - fixup! 034c066e
>> - compat/getpass: add a /dev/tty implementation
>> - credential: use git_prompt instead of git_getpass
>> - prompt: add PROMPT_ECHO flag
>> - stub out getpass_echo function
>> - refactor git_getpass into generic prompt function
>> - move git_getpass to its own source file
>> - t: add test harness for external credential helpers
>> - credentials: add "store" helper
>> - strbuf: add strbuf_add*_urlencode
>> - credentials: add "cache" helper
>> - docs: end-user documentation for the credential subsystem
>> - credential: make relevance of http path configurable
>> - credential: add credential.*.username
>> - credential: apply helper config
>> - http: use credential API to get passwords
>> - credential: add function for parsing url components
>> - introduce credentials API
>> - t5550: fix typo
>> - test-lib: add test_config_global variant
>>
>> Expecting a reroll?
>
> Yes, I have a re-roll ready. I was holding back to see if some of the
> helper authors might comment, but got nothing. Perhaps the new version
> will spur some interest.
>
> Also, let's drop the top git_getpass bits from the topic for now (they
> will not be part of my rebase). They are a separate topic that can go on
> top, but I think there was some question from Erik of whether we should
> simply roll our own getpass().
>
>> * jk/upload-archive-use-start-command (2011-11-21) 1 commit
>> - upload-archive: use start_command instead of fork
>>
>> What's the status of this one?
>
> I think what you have in pu is good, but of course I didn't actually
> test it on Windows. Erik?
>
I tried to check out ee27ca4 and build, and got hit by a wall of warnings:
compat/mingw.h: In function 'waitpid':
compat/mingw.h:117: warning: pointer targets in passing argument 1 of
'_cwait' differ in signedness
d:\msysgit\mingw\bin\../lib/gcc/mingw32/4.4.0/../../../../include/process.h:60:
note: expected 'int *' but argument is of type 'unsigned int *'
This seems to be an issue that has been around since the birth of the
windows port, and can be fixed by making our waitpid signature
identical to what POSIX expects
(http://pubs.opengroup.org/onlinepubs/7908799/xsh/waitpid.html):
---8<---
diff --git a/compat/mingw.h b/compat/mingw.h
index a255898..2036013 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -111,7 +111,7 @@ static inline int mingw_unlink(const char *pathname)
}
#define unlink mingw_unlink
-static inline int waitpid(pid_t pid, unsigned *status, unsigned options)
+static inline int waitpid(pid_t pid, int *status, int options)
{
if (options == 0)
return _cwait(status, pid, 0);
---8<---
But then, there's a couple of much more scary warnings:
decorate.c: In function 'hash_obj':
decorate.c:11: warning: dereferencing type-punned pointer will break
strict-aliasing rules
<snip>
object.c: In function 'hash_obj':
object.c:48: warning: dereferencing type-punned pointer will break
strict-aliasing rules
<snip>
builtin-merge-recursive.c: In function 'cmd_merge_recursive':
builtin-merge-recursive.c:49: warning: unknown conversion type character 'z' in
format
builtin-merge-recursive.c:49: warning: format '%s' expects type 'char *', but ar
gument 2 has type 'unsigned int'
builtin-merge-recursive.c:49: warning: too many arguments for format
Looking at the code, the first two looks like the real thing, and not
just false positives.
IIRC, the strict aliasing rule is not a part of C89, but is in C99.
And GCC starts to generate code that depends on strict-aliasing rules
with -O3. We don't use -O3, so this might be theoretical, at least on
my machine at this time.
The last one is a usage if "%zu", which our printf doesn't support.
This comes from commit 73118f89 ("merge-recursive.c: Add more generic
merge_recursive_generic()"), and should probably be fixed by doing:
---8<---
diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 703045b..02242cc 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c
@@ -45,8 +45,9 @@ int cmd_merge_recursive(int argc, const char **argv,
const char *prefix)
bases[bases_count++] = sha;
}
else
- warning("Cannot handle more than %zu bases. "
- "Ignoring %s.", ARRAY_SIZE(bases)-1, argv[i]);
+ warning("Cannot handle more than %"PRIuMAX" bases. "
+ "Ignoring %s.",
+ (uintmax_t)ARRAY_SIZE(bases)-1, argv[i]);
}
if (argc - i != 3) /* "--" "<head>" "<remote>" */
die("Not handling anything other than two heads merge.");
---8<---
But no matter what, something has clearly happened to our warning
level, and that should probably be investigated.
Back to topic: after fixing these (apart from the strict aliasing
issues), t5000-tar-tree pass fine here. So I guess that's at least
something. I haven't tested from git-daemon yet, but I don't have time
for that right now...
I'll submit the first fix later tonight, as it's clearly an
improvement IMO... but perhaps it's better if Stephan just squash in
the latter in the next round of the series, since it seems to be in a
pu-only series so far?
^ permalink raw reply related
* Re: [PATCH] Set hard limit on delta chain depth
From: Erik Faye-Lund @ 2011-12-06 12:17 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1323068688-31481-1-git-send-email-pclouds@gmail.com>
2011/12/5 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> 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.
>
Wouldn't it make more sense to make the limit a config option rather
than a hard-coded value of 128 (which seems arbitrary to me)? After
all, different platforms have different stack-limitations...
^ permalink raw reply
* Re: [PATCH] Set hard limit on delta chain depth
From: Nguyen Thai Ngoc Duy @ 2011-12-06 12:32 UTC (permalink / raw)
To: kusmabite; +Cc: git, Junio C Hamano
In-Reply-To: <CABPQNSaE=TWGbBRMnthEuT181=XbOafPfgx88_JQnnQ6TiYyqw@mail.gmail.com>
2011/12/6 Erik Faye-Lund <kusmabite@gmail.com>:
> 2011/12/5 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>> 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.
>>
>
> Wouldn't it make more sense to make the limit a config option rather
> than a hard-coded value of 128 (which seems arbitrary to me)? After
> all, different platforms have different stack-limitations...
Then it'd make more sense to make a compile time config based on
platform. We could have a config option that can override the default,
but I really don't see the point of making long delta chains.
--
Duy
^ permalink raw reply
* Re: [PATCH] Set hard limit on delta chain depth
From: Erik Faye-Lund @ 2011-12-06 12:41 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <CACsJy8BuUn4htSR6jAJfbueOshE6-YVV5dZGSWTGXbkuA0HO=A@mail.gmail.com>
On Tue, Dec 6, 2011 at 1:32 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> 2011/12/6 Erik Faye-Lund <kusmabite@gmail.com>:
>> 2011/12/5 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>>> 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.
>>>
>>
>> Wouldn't it make more sense to make the limit a config option rather
>> than a hard-coded value of 128 (which seems arbitrary to me)? After
>> all, different platforms have different stack-limitations...
>
> Then it'd make more sense to make a compile time config based on
> platform.
Can how much stack each recursion use be calculated at compile-time?
If so, I agree with you.
> We could have a config option that can override the default,
> but I really don't see the point of making long delta chains.
Aha, I figured you _did_ see a point in this, because 128 seemed
excessive to me already. I was thinking more that some platforms can
have a much smaller stack than (I would expect to) fit in 128
recursions (I've worked relatively recently with platforms with as
small as a static 2k stack per process), so you might not be fixing
the issue for such platforms. But that's not really your
responsibility either ;)
^ permalink raw reply
* Re: [PATCH] Set hard limit on delta chain depth
From: Nguyen Thai Ngoc Duy @ 2011-12-06 12:48 UTC (permalink / raw)
To: kusmabite; +Cc: git, Junio C Hamano
In-Reply-To: <CABPQNSbOvVkSFAE32hXoZEZeHqg-+nHc93uGytfCPQMicu0uVg@mail.gmail.com>
On Tue, Dec 6, 2011 at 7:41 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>>> Wouldn't it make more sense to make the limit a config option rather
>>> than a hard-coded value of 128 (which seems arbitrary to me)? After
>>> all, different platforms have different stack-limitations...
>>
>> Then it'd make more sense to make a compile time config based on
>> platform.
>
> Can how much stack each recursion use be calculated at compile-time?
> If so, I agree with you.
No, but at least we know default stack size of each platform and can
make pretty good limit based on that.
>> We could have a config option that can override the default,
>> but I really don't see the point of making long delta chains.
>
> Aha, I figured you _did_ see a point in this, because 128 seemed
> excessive to me already. I was thinking more that some platforms can
> have a much smaller stack than (I would expect to) fit in 128
> recursions (I've worked relatively recently with platforms with as
> small as a static 2k stack per process), so you might not be fixing
> the issue for such platforms. But that's not really your
> responsibility either ;)
Ah, I was thinking of an option that extends the limit, not shortens
it. Yes it makes sense in this case.
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Nguyen Thai Ngoc Duy @ 2011-12-06 14:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8vmqi98f.fsf@alter.siamese.dyndns.org>
On Tue, Dec 6, 2011 at 12:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> * nd/resolve-ref (2011-12-05) 2 commits
> (merged to 'next' on 2011-12-05 at cc79e86)
> + Copy resolve_ref() return value for longer use
> + Convert many resolve_ref() calls to read_ref*() and ref_exists()
>
> Will merge to 'master'.
I thought this would be replaced by a new version [1] I posted a while
ago (and forgot to address comments). Do you want me to rebase that on
top of this or replace it?
[1] http://article.gmane.org/gmane.comp.version-control.git/185580
--
Duy
^ permalink raw reply
* Re: [PATCH 0/8] nd/resolve-ref v2
From: Nguyen Thai Ngoc Duy @ 2011-12-06 14:07 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Junio C Hamano, Jeff King, Michael Haggerty,
Ramkumar Ramachandra
In-Reply-To: <20111117103924.GA5277@elie.hsd1.il.comcast.net>
2011/11/17 Jonathan Nieder <jrnieder@gmail.com>:
> Hi,
>
> Nguyễn Thái Ngọc Duy wrote:
>
>> Nguyễn Thái Ngọc Duy (8):
>
> Here are some comments from a quick glance over the series, to avoid
> too much noise that would distract from reports about the upcoming
> release.
Sorry for the late reply. Somehow I managed to miss your mail.
>> Re-add resolve_ref() that always returns an allocated buffer
>
> Makes me nervous, since it would introduce memory leaks if some other
> patch in flight calls resolve_ref(). Why not call it ref_resolve() or
> something?
Yeah, I made the same mistake before and made it again.
>> Enable GIT_DEBUG_MEMCHECK on git_pathname()
>
> __VA_ARGS__ was introduced in C99. I suspect some compilers that
> currently can compile git don't support it. But if you need to use
> it, that wouldn't rule out doing so in some corner guarded with an
> #ifdef.
>
> Looks pleasant overall. I look forward to looking more closely at
> this later.
This patch is bonus anyway and I think I'll drop it. Keeping a bunch
of ifdefs looks really ugly. I think having git_pathname()'s static
buffer per callsite file would be a good thing to do instead.
--
Duy
^ permalink raw reply
* Re: [PATCH] Set hard limit on delta chain depth
From: Michael Haggerty @ 2011-12-06 14:54 UTC (permalink / raw)
To: kusmabite; +Cc: Nguyễn Thái Ngọc Duy, git, Junio C Hamano
In-Reply-To: <CABPQNSaE=TWGbBRMnthEuT181=XbOafPfgx88_JQnnQ6TiYyqw@mail.gmail.com>
On 12/06/2011 01:17 PM, Erik Faye-Lund wrote:
> 2011/12/5 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>> 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.
>
> Wouldn't it make more sense to make the limit a config option rather
> than a hard-coded value of 128 (which seems arbitrary to me)? After
> all, different platforms have different stack-limitations...
I'm confused: is the data only ever read by the same host that generated
it? Because if not, then the "creator" had better never be configured
to use a chain depth that the "reader" cannot handle. This in turn
imply that there should be a common limit that is supported by all git
clients and is a documented part of the protocol. (Or the code has to
be rewritten to use an explicit stack instead of recursion.)
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH] Set hard limit on delta chain depth
From: Junio C Hamano @ 2011-12-06 15:06 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1323068688-31481-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> 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.
Why should we treat this condition any differently from the case where the
sender of a pack used beefier machine than you have and stuffed a huge
object that the index-pack running on your box cannot hold in core,
causing xmalloc() to die on your machine?
I do not think this is the right way to handle the issue. Your other patch
to flatten the recursion to iteration looked a lot saner approach.
^ permalink raw reply
* Re: [PATCH] Set hard limit on delta chain depth
From: Nguyen Thai Ngoc Duy @ 2011-12-06 15:30 UTC (permalink / raw)
To: Michael Haggerty; +Cc: kusmabite, git, Junio C Hamano
In-Reply-To: <4EDE2C95.5040804@alum.mit.edu>
2011/12/6 Michael Haggerty <mhagger@alum.mit.edu>:
> On 12/06/2011 01:17 PM, Erik Faye-Lund wrote:
>> 2011/12/5 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>>> 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.
>>
>> Wouldn't it make more sense to make the limit a config option rather
>> than a hard-coded value of 128 (which seems arbitrary to me)? After
>> all, different platforms have different stack-limitations...
>
> I'm confused: is the data only ever read by the same host that generated
> it?
index-pack is called at server side as part of a push. So in theory
the sending side can generate very long delta chains and bring down
the server side.
> Because if not, then the "creator" had better never be configured
> to use a chain depth that the "reader" cannot handle.
Normal creators (i.e. C Git) use default depth 50 so we should be safe.
> This in turn
> imply that there should be a common limit that is supported by all git
> clients and is a documented part of the protocol. (Or the code has to
> be rewritten to use an explicit stack instead of recursion.)
It's the implementation limitation, not the protocol. If the server
propagates error messages from index-pack back to client (I'm not
sure), then users can adjust --depth to be accepted by server. We
could negotiate the limit over the protocol but not sure we would want
to go that route.
The troubled code could be rewritten to avoid recursion. However long
delta chains may degrade performance, I'd rather have support to split
long chains (which can be done with --depth at client already) instead
of just recursion elimination. This patch is simpler, so it would be
easier to back port it if someone wants to do so.
--
Duy
^ permalink raw reply
* Re: [PATCH] Set hard limit on delta chain depth
From: Nguyen Thai Ngoc Duy @ 2011-12-06 15:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vvcpthh97.fsf@alter.siamese.dyndns.org>
2011/12/6 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> 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.
>
> Why should we treat this condition any differently from the case where the
> sender of a pack used beefier machine than you have and stuffed a huge
> object that the index-pack running on your box cannot hold in core,
> causing xmalloc() to die on your machine?
That's interesting. First of all xmalloc() is controlled by us while
index-pack code might lead to stack overflow exploit (never done it,
not sure if it's really pratical to do in this case).
But can I really use up all memory at server side by sending a huge pack?
> I do not think this is the right way to handle the issue. Your other patch
> to flatten the recursion to iteration looked a lot saner approach.
It may take me some time as I'm not really familar with this code.
Anybody is welcome to step up and flatten the function.
--
Duy
^ permalink raw reply
* Re: Query on git commit amend
From: Vijay Lakshminarayanan @ 2011-12-06 15:46 UTC (permalink / raw)
To: Viresh Kumar; +Cc: git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <4EDDD0E4.6040003@st.com>
Viresh Kumar <viresh.kumar@st.com> writes:
> Hello,
>
> Suppose i want to add few new changes to my last commit (HEAD).
> The way i do it is
> $ git add all_changed_files
> $ git commit --amend
>
> OR
> $ git commit --amend -a
>
> With both these ways, i get a screen to edit the message too.
>
> I want to know if there is a way to skip this screen.
>
> i.e.
> $ git commit --amend -a -some_other_option
>
> which simply adds new changes to existing commit, without asking to change
> message.
>
> If there is no such way, then can we add a patch for this, if it looks a valid
> case.
I've found
$ GIT_EDITOR=cat git commit --amend
useful.
The benefit of this technique is that it even works for git-rebase -i.
In my typical git usage, I do a lot of git-commit --fixup's. After
reaching a level of stability, I change the history with:
GIT_EDITOR=cat git rebase -i --autosquash
and my history is adjusted without requiring manual intervention.
--
Cheers
~vijay
Gnus should be more complicated.
^ permalink raw reply
* [PATCH] userdiff: allow * between cpp funcname words
From: Thomas Rast @ 2011-12-06 16:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The cpp pattern, used for C and C++, would not match the start of a
declaration such as
static char *prepare_index(int argc,
because it did not allow for * anywhere between the various words that
constitute the modifiers, type and function name. Fix it.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
This is a really sneaky one-character bug that I cannot believe went
unnoticed for so long, seeing as there are plenty of instances within
git itself where it matters.
userdiff.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/userdiff.c b/userdiff.c
index 7c983c1..76109da 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -118,7 +118,7 @@
/* Jump targets or access declarations */
"!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:.*$\n"
/* C/++ functions/methods at top level */
- "^([A-Za-z_][A-Za-z_0-9]*([ \t]+[A-Za-z_][A-Za-z_0-9]*([ \t]*::[ \t]*[^[:space:]]+)?){1,}[ \t]*\\([^;]*)$\n"
+ "^([A-Za-z_][A-Za-z_0-9]*([ \t*]+[A-Za-z_][A-Za-z_0-9]*([ \t]*::[ \t]*[^[:space:]]+)?){1,}[ \t]*\\([^;]*)$\n"
/* compound type at top level */
"^((struct|class|enum)[^;]*)$",
/* -- */
--
1.7.8.424.gcb840
^ permalink raw reply related
* git svn rebase “out of memory” error after merging two tracking branches
From: Onsager @ 2011-12-06 16:40 UTC (permalink / raw)
To: git
@list
... more detailed explanation on stackoverflow:
http://stackoverflow.com/questions/8398405/git-svn-rebase-out-of-memory-error-after-merging-two-tracking-branches
Is there something, I can do about this issue as a common user?
Michael
^ permalink raw reply
* [PATCH 0/5] cache-tree revisited
From: Thomas Rast @ 2011-12-06 17:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Carlos Martín Nieto
Junio C Hamano wrote:
> 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.
Oh, I buried that. Let's try something other than the aggressive
strategy I had there: only compute cache-tree if
* we know we're going to need it soon, and we're about to write out
the index anyway (as in git-commit)
* we know we can rebuild it from a tree, and we're about to write out
the index anyway (as in git-reset)
Both of these are essentially free, so I'm not sure I should even
bother with timings, but I ran the silly script at the end on my
favourite linux v2.6.37-rc2 repo, and it takes
before-: real 0m29.103s user 0m17.335s sys 0m8.415s
before+: real 0m30.860s user 0m9.741s sys 0m7.287s
after-: real 0m28.798s user 0m17.367s sys 0m8.435s
after+: real 0m17.563s user 0m8.246s sys 0m7.122s
where the + signifies starting out with fully valid cache-tree data,
and - means starting out with none at all.
So it's really free when it's starting out in bad shape, and it's
much faster when it starts out with valid data.
If you insist on writing cache-tree at *every* (except --only) commit,
then we might make it so that it writes out the index again at the
very end if it didn't already update it earlier. It would (and does,
I've tried... ok ok, patch is at the end) of course give the +
performance in the - case, but it's not free so other operations may
be affected.
---- 8< ---- test script for the above timings ---- 8< ----
#!/bin/sh
for i in $(seq 1 100); do
echo $i > arch/alpha/boot/foo
git add arch/alpha/boot/foo
git commit -m$i
done
---- >8 ----
---- 8< ---- patch to let git-commit always write the index ---- 8< ----
diff --git i/builtin/commit.c w/builtin/commit.c
index 57d028e..8e0c773 100644
--- i/builtin/commit.c
+++ w/builtin/commit.c
@@ -333,6 +333,8 @@ static void refresh_cache_or_die(int refresh_flags)
die_resolve_conflict("commit");
}
+static int wrote_index_already = 0;
+
static char *prepare_index(int argc, const char **argv, const char *prefix,
const struct commit *current_head, int is_status)
{
@@ -395,6 +397,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
add_files_to_cache(also ? prefix : NULL, pathspec, 0);
refresh_cache_or_die(refresh_flags);
update_main_cache_tree(1);
+ wrote_index_already = 1;
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(&index_lock))
die(_("unable to write new_index file"));
@@ -415,6 +418,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
if (active_cache_changed) {
+ wrote_index_already = 1;
update_main_cache_tree(1);
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(&index_lock))
@@ -639,6 +643,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
const char *hook_arg2 = NULL;
int ident_shown = 0;
int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
+ int fd;
if (!no_verify && run_hook(index_file, "pre-commit", NULL))
return 0;
@@ -863,11 +868,17 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
* the editor and after we invoke run_status above.
*/
discard_cache();
+ if (!wrote_index_already)
+ fd = hold_locked_index(&index_lock, 1);
read_cache_from(index_file);
if (update_main_cache_tree(0)) {
error(_("Error building trees"));
return 0;
}
+ if (!wrote_index_already && commit_style != COMMIT_PARTIAL)
+ if (write_cache(fd, active_cache, active_nr) ||
+ close_lock_file(&index_lock))
+ die(_("unable to write new_index file"));
if (run_hook(index_file, "prepare-commit-msg",
git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
---- >8 ----
Thomas Rast (5):
Add test-scrap-cache-tree
Test the current state of the cache-tree optimization
Refactor cache_tree_update idiom from commit
commit: write cache-tree data when writing index anyway
reset: update cache-tree data when appropriate
.gitignore | 1 +
Makefile | 1 +
builtin/commit.c | 7 +--
builtin/reset.c | 7 +++
cache-tree.c | 19 +++++++--
cache-tree.h | 4 +-
merge-recursive.c | 2 +-
t/t0090-cache-tree.sh | 95 +++++++++++++++++++++++++++++++++++++++++++++++
test-dump-cache-tree.c | 2 +-
test-scrap-cache-tree.c | 17 ++++++++
10 files changed, 144 insertions(+), 11 deletions(-)
create mode 100755 t/t0090-cache-tree.sh
create mode 100644 test-scrap-cache-tree.c
--
1.7.8.425.ga639d3
^ permalink raw reply related
* [PATCH 1/5] Add test-scrap-cache-tree
From: Thomas Rast @ 2011-12-06 17:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Carlos Martín Nieto
In-Reply-To: <cover.1323191497.git.trast@student.ethz.ch>
A simple utility that invalidates all existing cache-tree data. We
need this for tests. (We don't need a tool to rebuild the cache-tree
data; git read-tree HEAD works for that.)
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
.gitignore | 1 +
Makefile | 1 +
test-scrap-cache-tree.c | 17 +++++++++++++++++
3 files changed, 19 insertions(+), 0 deletions(-)
create mode 100644 test-scrap-cache-tree.c
diff --git a/.gitignore b/.gitignore
index 8572c8c..122336c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -171,6 +171,7 @@
/test-date
/test-delta
/test-dump-cache-tree
+/test-scrap-cache-tree
/test-genrandom
/test-index-version
/test-line-buffer
diff --git a/Makefile b/Makefile
index cbb5601..6bfa10b 100644
--- a/Makefile
+++ b/Makefile
@@ -436,6 +436,7 @@ TEST_PROGRAMS_NEED_X += test-ctype
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
+TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
TEST_PROGRAMS_NEED_X += test-genrandom
TEST_PROGRAMS_NEED_X += test-index-version
TEST_PROGRAMS_NEED_X += test-line-buffer
diff --git a/test-scrap-cache-tree.c b/test-scrap-cache-tree.c
new file mode 100644
index 0000000..4728013
--- /dev/null
+++ b/test-scrap-cache-tree.c
@@ -0,0 +1,17 @@
+#include "cache.h"
+#include "tree.h"
+#include "cache-tree.h"
+
+static struct lock_file index_lock;
+
+int main(int ac, char **av)
+{
+ int fd = hold_locked_index(&index_lock, 1);
+ if (read_cache() < 0)
+ die("unable to read index file");
+ active_cache_tree = NULL;
+ if (write_cache(fd, active_cache, active_nr)
+ || commit_lock_file(&index_lock))
+ die("unable to write index file");
+ return 0;
+}
--
1.7.8.425.ga639d3
^ 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