* Re: Re: Segmentation fault with latest git (070c57df)
From: Jongman Heo @ 2013-01-31 7:27 UTC (permalink / raw)
To: git, Antoine Pelisse, Jeff King
On Thu, Jan 31, 2013 at 7:49 AM, Jeff King wrote:
> On Thu, Jan 31, 2013 at 01:35:21AM +0000, Jongman Heo wrote:
>
>> Looks like following commit causes a segmentation fault in my machine
>> (when running git pull or git fetch);
>>
>> commit 8dd5afc926acb9829ebf56e9b78826a5242cd638
>> Author: Junio C Hamano
>> Date: Mon Jan 7 12:24:55 2013 -0800
>>
>> string-list: allow case-insensitive string list
>>
>>
>> In my case, list->cmp (at get_entry_index() function) has an invalid
>> address, obviously not an address of string comparision function,
>> instead it points to 1.
>
> Can you show us a stack trace? The string-list functions are generic and
> get called in a lot of places. It would be useful to know which list is
> causing the problem.
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
FYI, gdb backtrace and valgrind output attached below, Thanks.
(gdb) run fetch
Starting program: /home/hjongman/repos/git/git fetch
warning: .dynamic section for "/lib/libc.so.6" is not at the expected address
warning: difference appears to be caused by prelink, adjusting expectations
[Thread debugging using libthread_db enabled]
Program received signal SIGSEGV, Segmentation fault.
0x00000001 in ?? ()
(gdb) bt
#0 0x00000001 in ?? ()
#1 0x0812b457 in get_entry_index (list=0xbfffe7c0, string=0x821ec3c "refs/remotes/origin/HEAD", exact_match=0xbfffe568)
at string-list.c:14
#2 0x0812bd60 in add_entry (list=0xbfffe7c0, insert_at=-1, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:33
#3 string_list_insert_at_index (list=0xbfffe7c0, insert_at=-1, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:63
#4 0x0812bda0 in string_list_insert (list=0xbfffe7c0, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:57
#5 0x08071838 in add_existing (refname=0x821ec3c "refs/remotes/origin/HEAD",
sha1=0x821ec14 "\a\fW\337B\352N\255\314C\320Em\021E`\022C&", <incomplete sequence \303>, flag=1, cbdata=0xbfffe7c0)
at builtin/fetch.c:570
#6 0x0810af97 in do_one_ref (base=<value optimized out>, fn=0x8071820 <add_existing>, trim=0, flags=<value optimized out>,
cb_data=0xbfffe7c0, entry=0x821ec10) at refs.c:525
#7 0x0810bd9f in do_for_each_ref_in_dirs (dir1=0x8215d54, dir2=0x821ea44, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0,
flags=0, cb_data=0xbfffe7c0) at refs.c:627
#8 0x0810bc8e in do_for_each_ref_in_dirs (dir1=0x8215cac, dir2=0x8226954, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0,
flags=0, cb_data=0xbfffe7c0) at refs.c:597
#9 0x0810bc8e in do_for_each_ref_in_dirs (dir1=0x8215c0c, dir2=0x8215a54, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0,
flags=0, cb_data=0xbfffe7c0) at refs.c:597
#10 0x0810bc8e in do_for_each_ref_in_dirs (dir1=0x8215a1c, dir2=0x821862c, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0,
flags=0, cb_data=0xbfffe7c0) at refs.c:597
#11 0x0810c304 in do_for_each_ref (submodule=<value optimized out>, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, flags=0,
cb_data=0xbfffe7c0) at refs.c:1295
#12 0x0810c63b in for_each_ref (fn=0x8071820 <add_existing>, cb_data=0xbfffe7c0) at refs.c:1343
#13 0x0807390a in do_fetch (remote=<value optimized out>, argc=0, argv=0xbfffe9f8) at builtin/fetch.c:699
#14 fetch_one (remote=<value optimized out>, argc=0, argv=0xbfffe9f8) at builtin/fetch.c:949
#15 0x08074251 in cmd_fetch (argc=1, argv=0xbfffe9f8, prefix=0x0) at builtin/fetch.c:992
#16 0x0804b60b in run_builtin (argc=1, argv=0xbfffe9f8) at git.c:281
#17 handle_internal_command (argc=1, argv=0xbfffe9f8) at git.c:443
#18 0x0804ba51 in run_argv (argc=1, argv=0xbfffe9f8) at git.c:489
#19 main (argc=1, argv=0xbfffe9f8) at git.c:564
$ valgrind git fetch
==2195== Memcheck, a memory error detector
==2195== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==2195== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info
==2195== Command: git fetch
==2195==
==2195== Conditional jump or move depends on uninitialised value(s)
==2195== at 0x812B41F: get_entry_index (string-list.c:10)
==2195== by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
==2195== by 0x812BD9F: string_list_insert (string-list.c:57)
==2195== by 0x8071837: add_existing (fetch.c:570)
==2195== by 0x810AF96: do_one_ref (refs.c:525)
==2195== by 0x810BB20: do_for_each_ref_in_dir (refs.c:551)
==2195== by 0x810BD34: do_for_each_ref_in_dirs (refs.c:623)
==2195== by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195== by 0x810C303: do_for_each_ref (refs.c:1295)
==2195== by 0x810C63A: for_each_ref (refs.c:1343)
==2195== by 0x8073909: fetch_one (fetch.c:699)
==2195== by 0x8074250: cmd_fetch (fetch.c:992)
==2195==
==2195== Use of uninitialised value of size 4
==2195== at 0x812B454: get_entry_index (string-list.c:14)
==2195== by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
==2195== by 0x812BD9F: string_list_insert (string-list.c:57)
==2195== by 0x8071837: add_existing (fetch.c:570)
==2195== by 0x810AF96: do_one_ref (refs.c:525)
==2195== by 0x810BD9E: do_for_each_ref_in_dirs (refs.c:627)
==2195== by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195== by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195== by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195== by 0x810C303: do_for_each_ref (refs.c:1295)
==2195== by 0x810C63A: for_each_ref (refs.c:1343)
==2195== by 0x8073909: fetch_one (fetch.c:699)
==2195==
==2195==
==2195== Process terminating with default action of signal 11 (SIGSEGV)
==2195== Access not within mapped region at address 0x0
==2195== at 0x1: ???
==2195== by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
==2195== by 0x812BD9F: string_list_insert (string-list.c:57)
==2195== by 0x8071837: add_existing (fetch.c:570)
==2195== by 0x810AF96: do_one_ref (refs.c:525)
==2195== by 0x810BD9E: do_for_each_ref_in_dirs (refs.c:627)
==2195== by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195== by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195== by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195== by 0x810C303: do_for_each_ref (refs.c:1295)
==2195== by 0x810C63A: for_each_ref (refs.c:1343)
==2195== by 0x8073909: fetch_one (fetch.c:699)
==2195== If you believe this happened as a result of a stack
==2195== overflow in your program's main thread (unlikely but
==2195== possible), you can try to increase the size of the
==2195== main thread stack using the --main-stacksize= flag.
==2195== The main thread stack size used in this run was 10485760.
==2195==
==2195== HEAP SUMMARY:
==2195== in use at exit: 325,604 bytes in 3,579 blocks
==2195== total heap usage: 3,687 allocs, 108 frees, 518,058 bytes allocated
==2195==
==2195== LEAK SUMMARY:
==2195== definitely lost: 72 bytes in 2 blocks
==2195== indirectly lost: 0 bytes in 0 blocks
==2195== possibly lost: 31,347 bytes in 460 blocks
==2195== still reachable: 294,185 bytes in 3,117 blocks
==2195== suppressed: 0 bytes in 0 blocks
==2195== Rerun with --leak-check=full to see details of leaked memory
==2195==
==2195== For counts of detected and suppressed errors, rerun with: -v
==2195== Use --track-origins=yes to see where uninitialised values come from
==2195== ERROR SUMMARY: 3 errors from 2 contexts (suppressed: 17 from 8)
Segmentation fault
^ permalink raw reply
* Re: Segmentation fault with latest git (070c57df)
From: Jeff King @ 2013-01-31 7:35 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: Jongman Heo, git
In-Reply-To: <CALWbr2y8veWYRwEGrbGq-PhxZjXbBF5HMgQXrRr=1JzGQ9Y-tQ@mail.gmail.com>
On Thu, Jan 31, 2013 at 08:02:06AM +0100, Antoine Pelisse wrote:
> In "clean.c" we have a "string_list" created on the stack with
> "STRING_LIST_INIT_NODUP" (there are probably others, I stopped at the
> first occurrence).
> But, "STRING_LIST_INIT_NODUP" doesn't init the "list->cmp" pointer
> which can thus be random.
I don't think that is the problem. Extra struct members that are not
mentioned by an initializer will get set to 0 or NULL.
A code path that tried to initialize each member individually would run
into problems, but I could not find any such code path (all of them
either use an initializer, are static, or memset the struct to
all-zeroes).
-Peff
^ permalink raw reply
* Re: Re: Segmentation fault with latest git (070c57df)
From: Jeff King @ 2013-01-31 7:55 UTC (permalink / raw)
To: Jongman Heo; +Cc: git, Antoine Pelisse
In-Reply-To: <15825158.309231359617223702.JavaMail.weblogic@epml26>
On Thu, Jan 31, 2013 at 07:27:04AM +0000, Jongman Heo wrote:
> FYI, gdb backtrace and valgrind output attached below, Thanks.
Thanks, that's helpful.
> #4 0x0812bda0 in string_list_insert (list=0xbfffe7c0, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:57
> #5 0x08071838 in add_existing (refname=0x821ec3c "refs/remotes/origin/HEAD",
> sha1=0x821ec14 "\a\fW\337B\352N\255\314C\320Em\021E`\022C&", <incomplete sequence \303>, flag=1, cbdata=0xbfffe7c0)
> at builtin/fetch.c:570
So we are inserting the string from add_existing, which gets the list
from a callback parameter. Which comes from...
> #13 0x0807390a in do_fetch (remote=<value optimized out>, argc=0, argv=0xbfffe9f8) at builtin/fetch.c:699
...here, which does this:
struct string_list existing_refs = STRING_LIST_INIT_NODUP;
[...]
for_each_ref(add_existing, &existing_refs);
And yet we get:
> ==2195== Conditional jump or move depends on uninitialised value(s)
> ==2195== at 0x812B41F: get_entry_index (string-list.c:10)
> ==2195== by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
> ==2195== by 0x812BD9F: string_list_insert (string-list.c:57)
> ==2195== by 0x8071837: add_existing (fetch.c:570)
> ==2195== by 0x810AF96: do_one_ref (refs.c:525)
> ==2195== by 0x810BB20: do_for_each_ref_in_dir (refs.c:551)
> ==2195== by 0x810BD34: do_for_each_ref_in_dirs (refs.c:623)
> ==2195== by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
> ==2195== by 0x810C303: do_for_each_ref (refs.c:1295)
> ==2195== by 0x810C63A: for_each_ref (refs.c:1343)
> ==2195== by 0x8073909: fetch_one (fetch.c:699)
> ==2195== by 0x8074250: cmd_fetch (fetch.c:992)
which seems odd. cmp should be initialized to NULL, and then we never
touch it (and even if we did, it wouldn't be unitialized, but rather
have the value we put in it).
It's almost like the compiler is getting the initializer wrong. It's a
long shot, but I wonder if the presence of the bitfield could be
triggering a compiler bug (or there is a subtle C rule about bitfield
initializations that I do not know). Just for the sake of my sanity,
what does the following program output for you?
-- >8 --
#include <stdio.h>
#include <stdlib.h>
typedef int (*compare_fn)(const char *, const char *);
struct foo {
char **items;
unsigned int nr, alloc;
unsigned int bitfield:1;
compare_fn cmp;
};
int main(void)
{
struct foo f = { NULL, 0, 0, 0 };
printf("cmp is %lu\n", (unsigned long)f.cmp);
return 0;
}
^ permalink raw reply
* Re: Re: Re: Segmentation fault with latest git (070c57df)
From: 허종만 @ 2013-01-31 8:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Antoine Pelisse
> It's almost like the compiler is getting the initializer wrong. It's a
> long shot, but I wonder if the presence of the bitfield could be
> triggering a compiler bug (or there is a subtle C rule about bitfield
> initializations that I do not know). Just for the sake of my sanity,
> what does the following program output for you?
Hi,
just "cmp is 0" is printed.
$ gcc --version
gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-48)
Copyright (C) 2006 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Best regards,
Jongman Heo.
^ permalink raw reply
* Re: Segmentation fault with latest git (070c57df)
From: Thomas Rast @ 2013-01-31 8:42 UTC (permalink / raw)
To: Jeff King; +Cc: Jongman Heo, git, Antoine Pelisse
In-Reply-To: <20130131075511.GB5342@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Jan 31, 2013 at 07:27:04AM +0000, Jongman Heo wrote:
>
>> FYI, gdb backtrace and valgrind output attached below, Thanks.
>
> Thanks, that's helpful.
>
>> #4 0x0812bda0 in string_list_insert (list=0xbfffe7c0,
>> string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:57
>> #5 0x08071838 in add_existing (refname=0x821ec3c "refs/remotes/origin/HEAD",
>> sha1=0x821ec14 "\a\fW\337B\352N\255\314C\320Em\021E`\022C&",
>> <incomplete sequence \303>, flag=1, cbdata=0xbfffe7c0)
>> at builtin/fetch.c:570
>
> So we are inserting the string from add_existing, which gets the list
> from a callback parameter. Which comes from...
>
>> #13 0x0807390a in do_fetch (remote=<value optimized out>, argc=0,
>> argv=0xbfffe9f8) at builtin/fetch.c:699
>
> ...here, which does this:
>
> struct string_list existing_refs = STRING_LIST_INIT_NODUP;
> [...]
> for_each_ref(add_existing, &existing_refs);
>
> And yet we get:
>
>> ==2195== Conditional jump or move depends on uninitialised value(s)
>> ==2195== at 0x812B41F: get_entry_index (string-list.c:10)
>> ==2195== by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
>> ==2195== by 0x812BD9F: string_list_insert (string-list.c:57)
>> ==2195== by 0x8071837: add_existing (fetch.c:570)
>> ==2195== by 0x810AF96: do_one_ref (refs.c:525)
>> ==2195== by 0x810BB20: do_for_each_ref_in_dir (refs.c:551)
>> ==2195== by 0x810BD34: do_for_each_ref_in_dirs (refs.c:623)
>> ==2195== by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
>> ==2195== by 0x810C303: do_for_each_ref (refs.c:1295)
>> ==2195== by 0x810C63A: for_each_ref (refs.c:1343)
>> ==2195== by 0x8073909: fetch_one (fetch.c:699)
>> ==2195== by 0x8074250: cmd_fetch (fetch.c:992)
>
> which seems odd. cmp should be initialized to NULL, and then we never
> touch it (and even if we did, it wouldn't be unitialized, but rather
> have the value we put in it).
>
> It's almost like the compiler is getting the initializer wrong. It's a
> long shot, but I wonder if the presence of the bitfield could be
> triggering a compiler bug (or there is a subtle C rule about bitfield
> initializations that I do not know). Just for the sake of my sanity,
> what does the following program output for you?
>
> -- >8 --
> #include <stdio.h>
> #include <stdlib.h>
>
> typedef int (*compare_fn)(const char *, const char *);
>
> struct foo {
> char **items;
> unsigned int nr, alloc;
> unsigned int bitfield:1;
> compare_fn cmp;
> };
>
> int main(void)
> {
> struct foo f = { NULL, 0, 0, 0 };
> printf("cmp is %lu\n", (unsigned long)f.cmp);
> return 0;
> }
I doubt that would help because that stack region would be 0 anyway due
to kernel initialization of new pages. You'd have to somehow trample
over it first, like below.
Or perhaps something in the build process went wrong, and fetch.c didn't
get the memo about the new field in the struct. Depending on stack
layout, the next variable might be the 'int i' right before the
'string_list list' in the code, which could explain the value of 1.
---- 8< ----
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
typedef int (*compare_fn)(const char *, const char *);
struct foo {
char **items;
unsigned int nr, alloc;
unsigned int bitfield:1;
compare_fn cmp;
};
void scramble()
{
char foo[256];
memset(foo, 0x42, 256);
}
void init()
{
struct foo f = { NULL, 0, 0, 0 };
printf("cmp is %lu\n", (unsigned long)f.cmp);
}
int main(void)
{
scramble();
init();
return 0;
}
^ permalink raw reply
* Re: Segmentation fault with latest git (070c57df)
From: Jeff King @ 2013-01-31 8:54 UTC (permalink / raw)
To: Thomas Rast; +Cc: Jongman Heo, git, Antoine Pelisse
In-Reply-To: <87wqutvj4w.fsf@pctrast.inf.ethz.ch>
On Thu, Jan 31, 2013 at 09:42:07AM +0100, Thomas Rast wrote:
> > int main(void)
> > {
> > struct foo f = { NULL, 0, 0, 0 };
> > printf("cmp is %lu\n", (unsigned long)f.cmp);
> > return 0;
> > }
>
> I doubt that would help because that stack region would be 0 anyway due
> to kernel initialization of new pages. You'd have to somehow trample
> over it first, like below.
Good point. Unfortunately, I can't get either yours or mine to fail,
neither with a recent version of gcc nor with gcc-4.1. But I can't
convince git to fail, either. The only gcc-4.1 I have is Debian's
4.1.3 release, which is not quite what the OP has.
> Or perhaps something in the build process went wrong, and fetch.c didn't
> get the memo about the new field in the struct. Depending on stack
> layout, the next variable might be the 'int i' right before the
> 'string_list list' in the code, which could explain the value of 1.
Yeah, that would make sense to me with respect to the behavior we are
seeing, but that part of the Makefile should be pretty simple and
bug-free, I'd think (and from the original report, it seems like he was
able to reproduce it well enough to bisect). Still, trying a "make clean
&& make" might be worth it just to rule that out.
Puzzled...
-Peff
^ permalink raw reply
* Re: How to identify the users?
From: Sitaram Chamarty @ 2013-01-31 10:12 UTC (permalink / raw)
To: Scott Yan; +Cc: Tomas Carnecky, git@vger.kernel.org
In-Reply-To: <CACkbei+_dJowH-odL+UCS3hQwOwFZ7B5_6sxw=ZZg1V4=upSKg@mail.gmail.com>
On 01/31/2013 12:23 PM, Scott Yan wrote:
> Sitaram:
>
> It seems I must host my central repo on Gitolite first...
There is no "must" but yes it is a decent solution and can, in
principle, do the kind of checking you want if you set it up to do that.
Please note that I don't use that mode and, as my rant would have
indicated, I don't think it's a smart thing to do.
> I don't know Gitolite much, but you are right, maybe I should use
> Gitolite as my git server.
> I'll find more documents about gitolite these days,
> can you give me some suggestion which tutorial should I read? Thanks!
> ps: my OS is windows.
Try
http://therightstuff.de/CommentView,guid,b969ea4d-8d2c-42af-9806-de3631f4df68.aspx
I normally don't mention blog posts (favouring instead the official
documentation) but Windows is an exception. Hence the link.
Good luck.
^ permalink raw reply
* Re: [PATCH 4/6] introduce a commit metapack
From: Duy Nguyen @ 2013-01-31 11:06 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <CACsJy8Bqg6knVtddwaGSqtiXzVDgbO1JjbFOPMbF_RqrxM2rFQ@mail.gmail.com>
On Wed, Jan 30, 2013 at 09:16:29PM +0700, Duy Nguyen wrote:
> Perhaps we could store abbrev sha-1 instead of full sha-1. Nice
> space/time trade-off.
Following the on-disk format experiment yesterday, I changed the
format to:
- a list a _short_ SHA-1 of cached commits
- a list of cache entries, each (5 uint32_t) consists of:
- uint32_t for the index in .idx sha-1 table to get full SHA-1 of
the commit
- uint32_t for timestamp
- uint32_t for tree, 1st and 2nd parents for the index in .idx
table
The length of SHA-1 is chosen to be able to unambiguously identify any
cached commits. Full SHA-1 check is done after to catch false
positives. For linux-2.6, SHA-1 length is 6 bytes, git and many
moderate-sized projects are 4 bytes. So it's 26 bytes per commit for
linux-2.6.git, or 8MB .commits file. Not as good as revindex approach
(5.5MB) but way better than the current one (27MB). And still good
enough for caching 2 parents unconditionally.
Performance seems to improve a tiny bit, probably because of more
compact search space: 0.6s with my patch vs 0.7s without (vs 3.4s
without cache).
-- 8< --
diff --git a/cache.h b/cache.h
index 1f96f65..8048d5b 100644
--- a/cache.h
+++ b/cache.h
@@ -1069,6 +1069,7 @@ extern struct packed_git *add_packed_git(const char *, int, int);
extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);
extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);
extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);
+extern int find_pack_entry_pos(const unsigned char *, struct packed_git *);
extern int is_pack_valid(struct packed_git *);
extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);
extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
diff --git a/commit-metapack.c b/commit-metapack.c
index 2c19f48..c984b8e 100644
--- a/commit-metapack.c
+++ b/commit-metapack.c
@@ -4,11 +4,21 @@
#include "commit.h"
#include "sha1-lookup.h"
+struct commit_entry {
+ uint32_t commit; /* nth_packed_object_sha1 to get own SHA-1 */
+ uint32_t timestamp;
+ uint32_t tree; /* nth_packed_object_sha1 to get tree SHA-1 */
+ uint32_t parent1; /* nth_packed_object_sha1 to get 1st parent SHA-1 */
+ uint32_t parent2; /* nth_packed_object_sha1 to get 2nd parent SHA-1 */
+};
+
struct commit_metapack {
struct metapack mp;
uint32_t nr;
+ uint32_t abbrev_len;
+ struct packed_git *pack;
unsigned char *index;
- unsigned char *data;
+ struct commit_entry *data;
struct commit_metapack *next;
};
static struct commit_metapack *commit_metapacks;
@@ -41,20 +51,23 @@ static struct commit_metapack *alloc_commit_metapack(struct packed_git *pack)
}
memcpy(&it->nr, it->mp.data, 4);
it->nr = ntohl(it->nr);
+ memcpy(&it->abbrev_len, it->mp.data + 4, 4);
+ it->abbrev_len = ntohl(it->abbrev_len);
+ it->pack = pack;
/*
- * We need 84 bytes for each entry: sha1(20), date(4), tree(20),
- * parents(40).
+ * We need 20+abbrev_len bytes for each entry: abbrev sha-1,
+ * date(4), tree index(4), parent indexes(8).
*/
- if (it->mp.len < (84 * it->nr + 4)) {
+ if (it->mp.len < ((sizeof(*it->data) + it->abbrev_len) * it->nr + 8)) {
warning("commit metapack for '%s' is truncated", pack->pack_name);
metapack_close(&it->mp);
free(it);
return NULL;
}
- it->index = it->mp.data + 4;
- it->data = it->index + 20 * it->nr;
+ it->index = it->mp.data + 8;
+ it->data = (struct commit_entry*)(it->index + it->abbrev_len * it->nr);
return it;
}
@@ -83,29 +96,51 @@ static void prepare_commit_metapacks(void)
int commit_metapack(unsigned char *sha1,
uint32_t *timestamp,
- unsigned char **tree,
- unsigned char **parent1,
- unsigned char **parent2)
+ const unsigned char **tree,
+ const unsigned char **parent1,
+ const unsigned char **parent2)
{
struct commit_metapack *p;
prepare_commit_metapacks();
for (p = commit_metapacks; p; p = p->next) {
- unsigned char *data;
- int pos = sha1_entry_pos(p->index, 20, 0, 0, p->nr, p->nr, sha1);
+ struct commit_entry *data;
+ uint32_t p1, p2;
+ unsigned lo, hi, mi;
+ int pos;
+
+ /* sha1_entry_pos does not work with abbreviated sha-1 */
+ lo = 0;
+ hi = p->nr;
+ pos = -1;
+ do {
+ unsigned mi = (lo + hi) / 2;
+ int cmp = memcmp(p->index + mi * p->abbrev_len, sha1, p->abbrev_len);
+
+ if (!cmp) {
+ pos = mi;
+ break;
+ }
+ if (cmp > 0)
+ hi = mi;
+ else
+ lo = mi+1;
+ } while (lo < hi);
if (pos < 0)
continue;
- /* timestamp(4) + tree(20) + parents(40) */
- data = p->data + 64 * pos;
- *timestamp = *(uint32_t *)data;
- *timestamp = ntohl(*timestamp);
- data += 4;
- *tree = data;
- data += 20;
- *parent1 = data;
- data += 20;
- *parent2 = data;
+ data = p->data + pos;
+
+ /* full sha-1 check again */
+ if (hashcmp(nth_packed_object_sha1(p->pack, ntohl(data->commit)), sha1))
+ continue;
+
+ *timestamp = ntohl(data->timestamp);
+ *tree = nth_packed_object_sha1(p->pack, ntohl(data->tree));
+ p1 = ntohl(data->parent1);
+ *parent1 = nth_packed_object_sha1(p->pack, p1);
+ p2 = ntohl(data->parent2);
+ *parent2 = p1 == p2 ? null_sha1 : nth_packed_object_sha1(p->pack, p2);
return 0;
}
@@ -113,13 +148,20 @@ int commit_metapack(unsigned char *sha1,
return -1;
}
+struct write_cb {
+ struct commit_list **tail;
+ int abbrev_len;
+ const unsigned char *last_sha1;
+};
+
static void get_commits(struct metapack_writer *mw,
const unsigned char *sha1,
void *data)
{
- struct commit_list ***tail = data;
+ struct write_cb *write_cb = (struct write_cb *)data;
enum object_type type = sha1_object_info(sha1, NULL);
struct commit *c;
+ int p1, p2;
if (type != OBJ_COMMIT)
return;
@@ -128,47 +170,95 @@ static void get_commits(struct metapack_writer *mw,
if (!c || parse_commit(c))
die("unable to read commit %s", sha1_to_hex(sha1));
+ if (c->buffer) {
+ free(c->buffer);
+ c->buffer = NULL;
+ }
+
/*
* Our fixed-size parent list cannot represent root commits, nor
* octopus merges. Just skip those commits, as we can fallback
* in those rare cases to reading the actual commit object.
*/
if (!c->parents ||
- (c->parents && c->parents->next && c->parents->next->next))
+ (c->parents && c->parents->next && c->parents->next->next) ||
+ /* edge commits are out too */
+ find_pack_entry_pos(c->tree->object.sha1, mw->pack) == -1 ||
+ (p1 = find_pack_entry_pos(c->parents->item->object.sha1, mw->pack)) == -1 ||
+ (c->parents->next &&
+ (p2 = find_pack_entry_pos(c->parents->next->item->object.sha1, mw->pack)) == -1) ||
+ /*
+ * we set the 2nd parent the same as 1st parent as an
+ * indication that 2nd parent does not exist. Normal
+ * commits should never have two same parents, but just in
+ * case..
+ */
+ p1 == p2)
return;
- *tail = &commit_list_insert(c, *tail)->next;
+ /*
+ * Make sure we store the abbr sha-1 long enough to
+ * unambiguously identify any cached commits in the pack.
+ */
+ while (write_cb->abbrev_len < 20 &&
+ write_cb->last_sha1 &&
+ !memcmp(write_cb->last_sha1, sha1, write_cb->abbrev_len))
+ write_cb->abbrev_len++;
+ /*
+ * A bit sensitive to metapack_writer_foreach. "sha1" must not
+ * be changed even after this function exits.
+ */
+ write_cb->last_sha1 = sha1;
+
+ write_cb->tail = &commit_list_insert(c, write_cb->tail)->next;
}
void commit_metapack_write(const char *idx)
{
struct metapack_writer mw;
struct commit_list *commits = NULL, *p;
- struct commit_list **tail = &commits;
+ struct write_cb write_cb;
uint32_t nr = 0;
metapack_writer_init(&mw, idx, "commits", 1);
+ write_cb.tail = &commits;
+ write_cb.abbrev_len = 1;
+ write_cb.last_sha1 = NULL;
+
/* Figure out how many eligible commits we've got in this pack. */
- metapack_writer_foreach(&mw, get_commits, &tail);
+ metapack_writer_foreach(&mw, get_commits, &write_cb);
for (p = commits; p; p = p->next)
nr++;
+
metapack_writer_add_uint32(&mw, nr);
+ metapack_writer_add_uint32(&mw, write_cb.abbrev_len);
/* Then write an index of commit sha1s */
for (p = commits; p; p = p->next)
- metapack_writer_add(&mw, p->item->object.sha1, 20);
+ metapack_writer_add(&mw, p->item->object.sha1, write_cb.abbrev_len);
/* Followed by the actual date/tree/parents data */
for (p = commits; p; p = p->next) {
struct commit *c = p->item;
+ int pos;
+
+ pos = find_pack_entry_pos(c->object.sha1, mw.pack);
+ metapack_writer_add_uint32(&mw, pos);
+
metapack_writer_add_uint32(&mw, c->date);
- metapack_writer_add(&mw, c->tree->object.sha1, 20);
- metapack_writer_add(&mw, c->parents->item->object.sha1, 20);
- metapack_writer_add(&mw,
- c->parents->next ?
- c->parents->next->item->object.sha1 :
- null_sha1, 20);
+
+ pos = find_pack_entry_pos(c->tree->object.sha1, mw.pack);
+ metapack_writer_add_uint32(&mw, pos);
+
+ pos = find_pack_entry_pos(c->parents->item->object.sha1, mw.pack);
+ metapack_writer_add_uint32(&mw, pos);
+
+ if (c->parents->next) {
+ struct object *o = &c->parents->next->item->object;
+ pos = find_pack_entry_pos(o->sha1, mw.pack);
+ }
+ metapack_writer_add_uint32(&mw, pos);
}
metapack_writer_finish(&mw);
diff --git a/commit-metapack.h b/commit-metapack.h
index 4684573..caf85be 100644
--- a/commit-metapack.h
+++ b/commit-metapack.h
@@ -3,9 +3,9 @@
int commit_metapack(unsigned char *sha1,
uint32_t *timestamp,
- unsigned char **tree,
- unsigned char **parent1,
- unsigned char **parent2);
+ const unsigned char **tree,
+ const unsigned char **parent1,
+ const unsigned char **parent2);
void commit_metapack_write(const char *idx_file);
diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..1acab8c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1978,8 +1978,8 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
}
}
-off_t find_pack_entry_one(const unsigned char *sha1,
- struct packed_git *p)
+int find_pack_entry_pos(const unsigned char *sha1,
+ struct packed_git *p)
{
const uint32_t *level1_ofs = p->index_data;
const unsigned char *index = p->index_data;
@@ -1992,7 +1992,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
if (!index) {
if (open_pack_index(p))
- return 0;
+ return -1;
level1_ofs = p->index_data;
index = p->index_data;
}
@@ -2019,9 +2019,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
if (use_lookup) {
int pos = sha1_entry_pos(index, stride, 0,
lo, hi, p->num_objects, sha1);
- if (pos < 0)
- return 0;
- return nth_packed_object_offset(p, pos);
+ return pos;
}
do {
@@ -2032,15 +2030,24 @@ off_t find_pack_entry_one(const unsigned char *sha1,
printf("lo %u hi %u rg %u mi %u\n",
lo, hi, hi - lo, mi);
if (!cmp)
- return nth_packed_object_offset(p, mi);
+ return mi;
if (cmp > 0)
hi = mi;
else
lo = mi+1;
} while (lo < hi);
- return 0;
+ return -1;
}
+off_t find_pack_entry_one(const unsigned char *sha1,
+ struct packed_git *p)
+{
+ int pos = find_pack_entry_pos(sha1, p);
+ if (pos < 0)
+ return 0;
+ else
+ return nth_packed_object_offset(p, pos);
+}
int is_pack_valid(struct packed_git *p)
{
/* An already open pack is known to be valid. */
-- 8< --
^ permalink raw reply related
* [feature request] git-daemon http connection filtering of client types
From: @ 2013-01-31 12:46 UTC (permalink / raw)
To: git
Hey folks,
When I checked for false positives in my spam this morning, I spotted
an interesting malformed img link at the top of a spam message.
{snip}
> <http://git.{snip}.n2.nabble.com/file/{snip}/t3.jpg>
>
> Employ a medal tiffany bracelet <{snip}> a is
{snip}
So, apparently git-daemon's http features are being used by spammers.
In most cases, spam filters will correctly identify this junk.
I wonder if there is a better way... In my mental sandbox, git-daemon
http could have a set of deny/allow rules for incoming connection
client types.
e.g.:
git: allow
git-http: allow
thunderbird: deny
outlook express: replace linked file with rickroll.jpg
and so on.. An out-of-the-box install probably should default to
allow all to keep backward compatibility.
While I'd love a chance to hack something out, I sadly doubt I'll ever
have the time for it. Perhaps there is a student hacker looking for a
project.
Cheers!
-phil
p.s. appologies to anyone who now has Astley's song stuck in their
head. This was not intentional.
^ permalink raw reply
* [PATCH] push: fix segfault when HEAD points nowhere
From: Fraser Tweedale @ 2013-01-31 12:22 UTC (permalink / raw)
To: git; +Cc: Fraser Tweedale
When reporting the outcome of a push, a segfault occurs if HEAD does
not point somewhere. Check that HEAD points somewhere before trying
to strcmp it.
Signed-off-by: Fraser Tweedale <frase@frase.id.au>
---
transport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/transport.c b/transport.c
index 9932f40..b9306ef 100644
--- a/transport.c
+++ b/transport.c
@@ -741,7 +741,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,
n += print_one_push_status(ref, dest, n, porcelain);
if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD &&
*nonfastforward != NON_FF_HEAD) {
- if (!strcmp(head, ref->name))
+ if (head != NULL && !strcmp(head, ref->name))
*nonfastforward = NON_FF_HEAD;
else
*nonfastforward = NON_FF_OTHER;
--
1.8.1.1
^ permalink raw reply related
* Re: [feature request] git-daemon http connection filtering of client types
From: Erik Faye-Lund @ 2013-01-31 13:22 UTC (permalink / raw)
To: porpen+git; +Cc: git
In-Reply-To: <CAANzHtT83JXhQ8XRifdK5ah7NrnD6hvrCjBO4PnPx=qC=DTT9A@mail.gmail.com>
On Thu, Jan 31, 2013 at 1:46 PM, <porpen+git@gmail.com> wrote:
> Hey folks,
>
> When I checked for false positives in my spam this morning, I spotted
> an interesting malformed img link at the top of a spam message.
>
> {snip}
>> <http://git.{snip}.n2.nabble.com/file/{snip}/t3.jpg>
>>
>> Employ a medal tiffany bracelet <{snip}> a is
> {snip}
>
> So, apparently git-daemon's http features are being used by spammers.
Not at all. You appear to be referring to the message from
http://git.661346.n2.nabble.com/tiffany-bracelet-On-your-Significant-other-td7575440.html
This isn't a running instance of git-daemon, it's a web front-end for
the mailing list. It seems nabble allows image-attachments, and that's
what you're seeing; an attached image to a spam-email that was sent to
the git-mailing list through nabble.
The message contains HTML to display the image, and the git mailing
list rejects HTML messages. So the only ones who should be able to get
these spam-emails are users who subscribe through nabble. If you
subscribe through vger instead
(http://vger.kernel.org/vger-lists.html#git), you should get less
spam.
> In most cases, spam filters will correctly identify this junk.
>
> I wonder if there is a better way... In my mental sandbox, git-daemon
> http could have a set of deny/allow rules for incoming connection
> client types.
> e.g.:
>
> git: allow
> git-http: allow
> thunderbird: deny
> outlook express: replace linked file with rickroll.jpg
>
> and so on.. An out-of-the-box install probably should default to
> allow all to keep backward compatibility.
>
Git-daemon doesn't have an http-feature. You are probably thinking
about git-http-backend, but that's an CGI; the http-daemon invoking it
should already be able to filter connections. So, I don't think
there's anything that needs to be done to be able to block spammers
from git-servers. Blocking spammers from nabble is a different manner,
and is something you'll have to take up with the nabble staff.
^ permalink raw reply
* Re: [PATCH 1/1] Introduce new build variables INSTALL_MODE_EXECUTABLE and INSTALL_MODE_DATA.
From: Jeff Epler @ 2013-01-31 13:25 UTC (permalink / raw)
To: TJ; +Cc: git
In-Reply-To: <5109D230.2030101@iam.tj>
I was not familiar with this behavior of 'install -d' that it tries to change
the mode of an existing directory, but GNU coreutils 8.12.197-032bb
certainly behaves as TJ reports.
As a possible alternative, what about
[ -d $(DESTDIR)$(main1dir) ] || $(INSTALL) -d -m 755 $(DESTDIR)$(man1dir)
so that $(INSTALL) is not called when the target directory exists
already.
Jeff
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Ted Zlatanov @ 2013-01-31 15:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Michal Nazarewicz, git, Krzysztof Mazur,
Michal Nazarewicz
In-Reply-To: <7v7gmumzo6.fsf@alter.siamese.dyndns.org>
On Wed, 30 Jan 2013 07:57:29 -0800 Junio C Hamano <gitster@pobox.com> wrote:
JCH> Jeff King <peff@peff.net> writes:
>> But it would probably make sense for send-email to support the existing
>> git-credential subsystem, so that it can take advantage of secure
>> system-specific storage. And that is where we should be pointing new
>> users. I think contrib/mw-to-git even has credential support written in
>> perl, so it would just need to be factored out to Git.pm.
JCH> Yeah, that sounds like a neat idea.
Jeff, is there a way for git-credential to currently support
authinfo/netrc parsing? I assume that's the right way, instead of using
Michal's proposal to parse internally?
I'd like to add that, plus support for the 'string' and "string"
formats, and authinfo.gpg decoding through GPG. I'd write it in Perl,
if there's a choice.
Ted
^ permalink raw reply
* Re: [PATCH 1/1] Introduce new build variables INSTALL_MODE_EXECUTABLE and INSTALL_MODE_DATA.
From: Junio C Hamano @ 2013-01-31 15:48 UTC (permalink / raw)
To: Jeff Epler; +Cc: TJ, git
In-Reply-To: <20130131132526.GA10622@unpythonic.net>
Jeff Epler <jepler@unpythonic.net> writes:
> I was not familiar with this behavior of 'install -d' that it tries to change
> the mode of an existing directory, but GNU coreutils 8.12.197-032bb
> certainly behaves as TJ reports.
>
> As a possible alternative, what about
> [ -d $(DESTDIR)$(main1dir) ] || $(INSTALL) -d -m 755 $(DESTDIR)$(man1dir)
> so that $(INSTALL) is not called when the target directory exists
> already.
That can cut both ways, though. If it exists and its permission is
too tight (say 0750), $(INSTALL) -d -m 755 ought to loosen for
others, but with the additional test, it won't be given a chance to
do so.
^ permalink raw reply
* Re: [PATCH 1/1] Introduce new build variables INSTALL_MODE_EXECUTABLE and INSTALL_MODE_DATA.
From: Junio C Hamano @ 2013-01-31 15:51 UTC (permalink / raw)
To: TJ; +Cc: git
In-Reply-To: <5109D230.2030101@iam.tj>
TJ <git@iam.tj> writes:
> + $(INSTALL) -d -m $(INSTALL_MODE_EXECUTABLE) $(DESTDIR)$(man1dir)
> + $(INSTALL) -d -m $(INSTALL_MODE_EXECUTABLE) $(DESTDIR)$(man5dir)
> + $(INSTALL) -d -m $(INSTALL_MODE_EXECUTABLE) $(DESTDIR)$(man7dir)
> + $(INSTALL) -m $(INSTALL_MODE_DATA) $(DOC_MAN1) $(DESTDIR)$(man1dir)
> + $(INSTALL) -m $(INSTALL_MODE_DATA) $(DOC_MAN5) $(DESTDIR)$(man5dir)
> + $(INSTALL) -m $(INSTALL_MODE_DATA) $(DOC_MAN7) $(DESTDIR)$(man7dir)
I'm tempted to suggest
INSTALL_DIR = $(INSTALL) -d -m 755
INSTALL_DATA = $(INSTALL) -m 644
INSTALL_PROGRAM = $(INSTALL) -m 755
The number of lines the patch needs to touch will be the same, but
the resulting lines will not have many $(INSTALL_MODE_BLAH) shouting
at us.
Besides, you would want to differentiate the two kinds of 755 anyway
(I'd prefer INSTALL_PROGRAM to use -m 555 personally, for example).
^ permalink raw reply
* Re: [PATCH 0/2] improve "git branch --contains=<commit> <pattern>"
From: Junio C Hamano @ 2013-01-31 15:53 UTC (permalink / raw)
To: Jeff King; +Cc: Peter Wu, git
In-Reply-To: <20130131064357.GA24660@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That being said, we could be much more helpful. It seems like --contains
> should imply listing mode, since it is nonsensical in other modes. The
> second patch below adjusts that, and makes the command above do what you
> expect.
>
> [1/2]: docs: clarify git-branch --list behavior
> [2/2]: branch: let branch filters imply --list
Thanks. Looks good.
^ permalink raw reply
* Re: [PATCH] push: fix segfault when HEAD points nowhere
From: Junio C Hamano @ 2013-01-31 16:06 UTC (permalink / raw)
To: Fraser Tweedale; +Cc: git
In-Reply-To: <1359634971-79036-1-git-send-email-frase@frase.id.au>
Fraser Tweedale <frase@frase.id.au> writes:
> When reporting the outcome of a push, a segfault occurs if HEAD does
> not point somewhere. Check that HEAD points somewhere before trying
> to strcmp it.
>
> Signed-off-by: Fraser Tweedale <frase@frase.id.au>
> ---
> transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/transport.c b/transport.c
> index 9932f40..b9306ef 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -741,7 +741,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,
> n += print_one_push_status(ref, dest, n, porcelain);
> if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD &&
> *nonfastforward != NON_FF_HEAD) {
> - if (!strcmp(head, ref->name))
> + if (head != NULL && !strcmp(head, ref->name))
> *nonfastforward = NON_FF_HEAD;
> else
> *nonfastforward = NON_FF_OTHER;
Wow.
This is a bug that is hard to trigger by just using the software
(you have to be doubly insane to be on an unborn branch and pushing
out a branch that is not the one you are currently on) and one has
to look at the code to hunt for it.
The fix looks correct. Thank you very much for spotting.
^ permalink raw reply
* Re: [PATCH 2/2] branch: let branch filters imply --list
From: Peter Wu @ 2013-01-31 16:13 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130131064611.GB25315@sigill.intra.peff.net>
On Thursday 31 January 2013 01:46:11 Jeff King wrote:
> Currently, a branch filter like `--contains`, `--merged`, or
> `--no-merged` is ignored when we are not in listing mode.
> For example:
>
> git branch --contains=foo bar
>
> will create the branch "bar" from the current HEAD, ignoring
> the `--contains` argument entirely. This is not very
> helpful. There are two reasonable behaviors for git here:
>
> 1. Flag an error; the arguments do not make sense.
>
> 2. Implicitly go into `--list` mode
>
> This patch chooses the latter, as it is more convenient, and
> there should not be any ambiguity with attempting to create
> a branch; using `--contains` and not wanting to list is
> nonsensical.
>
> That leaves the case where an explicit modification option
> like `-d` is given. We already catch the case where
> `--list` is given alongside `-d` and flag an error. With
> this patch, we will also catch the use of `--contains` and
> other filter options alongside `-d`.
>
> Signed-off-by: Jeff King <peff@peff.net>
Tested-by: Peter Wu <lekensteyn@gmail.com>
I have tested this patch on top of 1.8.1.2 and it seems to work.
One note, the following command spits out master without complaining about the
non-existing branch name:
git branch --contains <id> master <non-existant branch name>
(the order of branches doesn't affect the result.)
Regards,
Peter
^ permalink raw reply
* Re: [PATCH 2/3] run-command: Be more informative about what failed
From: Junio C Hamano @ 2013-01-31 16:24 UTC (permalink / raw)
To: Stephen Boyd; +Cc: git
In-Reply-To: <1359597666-10108-3-git-send-email-sboyd@codeaurora.org>
Stephen Boyd <sboyd@codeaurora.org> writes:
> While debugging an error with verify_signed_buffer() the error
> messages from run-command weren't very useful:
>
> error: cannot create pipe for gpg: Too many open files
> error: could not run gpg.
>
> because they didn't indicate *which* pipe couldn't be created.
For the message emitted here with your update (or without for that
matter) to be useful, it has to hold that there is a single leaker,
that leaker fails in this codepath, and that there is nobody else
involved. Otherwise, you may be able to tell that one caller could
not create its stdin, but the reason it couldn't may be because
somebody else consumed all the available file descriptors.
I am not opposed to this change per-se, but I am not sure that
saying "stdin" etc. makes the message more useful for the purpose of
debugging.
> For example, the above error now prints:
>
> error: cannot create stderr pipe for gpg: Too many open files
> error: could not run gpg.
I'd prefer to see these names spelled out (e.g. "standard error")
in any case.
Thanks.
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
> run-command.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 12d4ddb..016dd05 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -274,6 +274,7 @@ int start_command(struct child_process *cmd)
> int need_in, need_out, need_err;
> int fdin[2], fdout[2], fderr[2];
> int failed_errno = failed_errno;
> + char *str;
>
> /*
> * In case of errors we must keep the promise to close FDs
> @@ -286,6 +287,7 @@ int start_command(struct child_process *cmd)
> failed_errno = errno;
> if (cmd->out > 0)
> close(cmd->out);
> + str = "stdin";
> goto fail_pipe;
> }
> cmd->in = fdin[1];
> @@ -301,6 +303,7 @@ int start_command(struct child_process *cmd)
> close_pair(fdin);
> else if (cmd->in)
> close(cmd->in);
> + str = "stdout";
> goto fail_pipe;
> }
> cmd->out = fdout[0];
> @@ -318,9 +321,10 @@ int start_command(struct child_process *cmd)
> close_pair(fdout);
> else if (cmd->out)
> close(cmd->out);
> + str = "stderr";
> fail_pipe:
> - error("cannot create pipe for %s: %s",
> - cmd->argv[0], strerror(failed_errno));
> + error("cannot create %s pipe for %s: %s",
> + str, cmd->argv[0], strerror(failed_errno));
> errno = failed_errno;
> return -1;
> }
^ permalink raw reply
* Re: [PATCH 0/2] optimizing pack access on "read only" fetch repos
From: Shawn Pearce @ 2013-01-31 16:47 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130129211932.GA17377@sigill.intra.peff.net>
On Tue, Jan 29, 2013 at 1:19 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 29, 2013 at 07:58:01AM -0800, Junio C Hamano wrote:
>
>> The point is not about space. Disk is cheap, and it is not making
>> it any worse than what happens to your target audience, that is a
>> fetch-only repository with only "gc --auto" in it, where nobody
>> passes "-f" to "repack" to cause recomputation of delta.
>>
>> What I was trying to seek was a way to reduce the runtime penalty we
>> pay every time we run git in such a repository.
>>
>> - Object look-up cost will become log2(50*n) from 50*log2(n), which
>> is about 50/log2(50) improvement;
>
> Yes and no. Our heuristic is to look at the last-used pack for an
> object. So assuming we have locality of requests, we should quite often
> get "lucky" and find the object in the first log2 search. Even if we
> don't assume locality, a situation with one large pack and a few small
> packs will have the large one as "last used" more often than the others,
> and it will also have the looked-for object more often than the others
Opening all of those files does impact performance. It depends on how
slow your open(2) syscall is. I know on Mac OS X that its not the
fastest function we get from the C library. Performing ~40 opens to
look through the most recent pack files and finally find the "real"
pack that contains that tag you asked `git show` for isn't that quick.
Some of us also use Git on filesystems that are network based, and
slow compared to local disk Linux ext2/3/4 with gobs of free RAM.
> So I can see how it is something we could potentially optimize, but I
> could also see it being surprisingly not a big deal. I'd be very
> interested to see real measurements, even of something as simple as a
> "master index" which can reference multiple packfiles.
I actually tried this many many years ago. There are threads in the
archive about it. Its slower. We ruled it out.
>> - System resource cost we incur by having to keep 50 file
>> descriptors open and maintaining 50 mmap windows will reduce by
>> 50 fold.
>
> I wonder how measurable that is (and if it matters on Linux versus less
> efficient platforms).
It does matter. We know it has a negative impact on JGit even on Linux
for example. You don't want 300 packs in a repository. 50 might be
tolerable. 300 is not.
^ permalink raw reply
* Re: [PATCH 1/1] Introduce new build variables INSTALL_MODE_EXECUTABLE and INSTALL_MODE_DATA.
From: TJ @ 2013-01-31 16:55 UTC (permalink / raw)
To: git
In-Reply-To: <7vtxpxic5l.fsf@alter.siamese.dyndns.org>
On 31/01/13 15:51, Junio C Hamano wrote:
> TJ <git@iam.tj> writes:
>
>> + $(INSTALL) -d -m $(INSTALL_MODE_EXECUTABLE) $(DESTDIR)$(man1dir)
>> + $(INSTALL) -d -m $(INSTALL_MODE_EXECUTABLE) $(DESTDIR)$(man5dir)
>> + $(INSTALL) -d -m $(INSTALL_MODE_EXECUTABLE) $(DESTDIR)$(man7dir)
>> + $(INSTALL) -m $(INSTALL_MODE_DATA) $(DOC_MAN1) $(DESTDIR)$(man1dir)
>> + $(INSTALL) -m $(INSTALL_MODE_DATA) $(DOC_MAN5) $(DESTDIR)$(man5dir)
>> + $(INSTALL) -m $(INSTALL_MODE_DATA) $(DOC_MAN7) $(DESTDIR)$(man7dir)
>
> I'm tempted to suggest
>
> INSTALL_DIR = $(INSTALL) -d -m 755
> INSTALL_DATA = $(INSTALL) -m 644
> INSTALL_PROGRAM = $(INSTALL) -m 755
>
> The number of lines the patch needs to touch will be the same, but
> the resulting lines will not have many $(INSTALL_MODE_BLAH) shouting
> at us.
I did contemplate that but was concerned it might be seen as interfering unduly with
the tool name/path settings, as opposed to their options.
> Besides, you would want to differentiate the two kinds of 755 anyway
> (I'd prefer INSTALL_PROGRAM to use -m 555 personally, for example).
Yes, I think I lost that one in the mists of sed-land when making the changes :)
I'll revise the patch based on received comments and post the revision tomorrow.
^ permalink raw reply
* Re: [PATCH 2/2] branch: let branch filters imply --list
From: Junio C Hamano @ 2013-01-31 17:02 UTC (permalink / raw)
To: Peter Wu; +Cc: Jeff King, git
In-Reply-To: <1504250.y8BAS4sG2O@al>
Peter Wu <lekensteyn@gmail.com> writes:
> One note, the following command spits out master without complaining about the
> non-existing branch name:
>
> git branch --contains <id> master <non-existant branch name>
>
> (the order of branches doesn't affect the result.)
That is perfectly normal.
What you gave after "--contains <id>" are *not* branch names. They
are patterns against branch names that fits the given criteria (in
this case "--contains <id>") are matched, and the branches that do
not match any of the patterns will not appear in the result.
^ permalink raw reply
* Re: [PATCH 4/6] introduce a commit metapack
From: Shawn Pearce @ 2013-01-31 17:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Duy Nguyen
In-Reply-To: <7vboc6mzpf.fsf@alter.siamese.dyndns.org>
On Wed, Jan 30, 2013 at 7:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>>>From this:
>>
>>> Then it will be very natural for the extension data that store the
>>> commit metainfo to name objects in the pack the .idx file describes
>>> by the offset in the SHA-1 table.
>>
>> I guess your argument is that putting it all in the same file makes it
>> more natural for there to be a data dependency.
>
> It is more about the "I am torn on this one" I mentioned earlier.
>
> It would be more "logical" if this weren't tied to a particular
> pack, as the properties of a commit you record in this series do not
> depend on which pack the commit is in, and such a repository-global
> file by definition cannot be inside anybody's .idx.
>
> But if we split the information into separate pieces and store one
> piece per .idx for implementation reasons, it is crazy not to at
> least consider it a longer term goal to put it inside .idx file.
>
> Of course, it is more convenient to store this kind of things in a
> separate file while experimenting and improving the mechanism, but I
> do not think we want to see each packfile in a repository comes with
> 47 auxiliary files with different suffixes 5 years down the road.
Arrrrgggh.
Right now we are in the middle of refactoring the JGit reachability
bitmap implementation to store it into a separate file and get it out
of the .idx file. This work is nearly completed. So this thread is
great timing. :-)
I think Junio is right about not wanting 47 different tiny auxiliary
files for a single pack. We are unlikely to create that many, but
right now there are proposals floating around for at least 2 new
auxiliary files (commit cache and reachability bitmaps). So its not
impossible that another will be discovered in the future.
Junio may be right about the hole in the index file for git-core. I
haven't checked the JGit implementation, but I suspect it does not
have this hole. IIRC JGit consumes the index sections and then expects
the pack trailer SHA-1 to be present immediately after the last table.
This happens because JGit doesn't use mmap() to load the index, it
streams the file into memory and does some reformatting on the tables
to make search faster.
If we are going to change the index to support extension sections and
I have to modify JGit to grok this new format, it needs to be index v3
not index v2. If we are making index v3 we should just put index v3 on
the end of the pack file.
^ permalink raw reply
* Re: [PATCH 2/7] Undocument deprecated alias 'push.default=tracking'
From: Ævar Arnfjörð Bjarmason @ 2013-01-31 17:10 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster, Jeff King, Michael Haggerty
In-Reply-To: <1335170284-30768-3-git-send-email-Matthieu.Moy@imag.fr>
On Mon, Apr 23, 2012 at 10:37 AM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> It's been deprecated since 53c4031 (Johan Herland, Wed Feb 16 2011,
> push.default: Rename 'tracking' to 'upstream'), so it's OK to remove it
> from documentation (even though it's still supported) to make the
> explanations more readable.
I don't think this was a good move for the documentation. Now every
time I find an old repo with "push.default=tracking" I end up
wondering what it was a synonym for again, and other users who don't
know what it does will just assume it's an invalid value or something.
We can't treat existing config values we still support as any other
deprecated feature. They still exist in files we have no control over,
and in people's brains who are reading "man git-config" trying to
remember what it meant.
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> Feel free to squash into previous one if needed.
>
> Documentation/config.txt | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e38fab1..ddf6043 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1693,7 +1693,6 @@ push.default::
> makes `git push` and `git pull` symmetrical in the sense that `push`
> will update the same remote ref as the one which is merged by
> `git pull`.
> -* `tracking` - deprecated synonym for `upstream`.
> * `current` - push the current branch to a branch of the same name.
> +
> The `current` and `upstream` modes are for those who want to
> --
> 1.7.10.234.ge65dd.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH/RFC 0/6] commit caching
From: Shawn Pearce @ 2013-01-31 17:14 UTC (permalink / raw)
To: Jeff King; +Cc: git, Duy Nguyen
In-Reply-To: <20130129091434.GA6975@sigill.intra.peff.net>
On Tue, Jan 29, 2013 at 1:14 AM, Jeff King <peff@peff.net> wrote:
> This is the cleaned-up version of the commit caching patches I mentioned
> here:
>
> http://article.gmane.org/gmane.comp.version-control.git/212329
...
> The short of it is that for an extra 31M of disk
> space (~4%), I get a warm-cache speedup for "git rev-list --all" of
> ~4.2s to ~0.66s.
I have to admit, this is a nice gain. I don't think users often dig
through all commits to the root but I can see how this might improve
git log with a path filter.
> Coupled with using compression level 0 for trees (which do not compress
> well at all, and yield only a 2% increase in size when left
> uncompressed), my "git rev-list --objects --all" time drops from ~40s to
> ~25s.
This uhm.... is nice?
But consider reachability bitmaps. ~40s to ~80ms. :-)
> Perf reveals that we're spending most of the remaining time in
> lookup_object. I've spent a fair bit of time trying to optimize that,
> but with no luck; I think it's fairly close to optimal. The problem is
> just that we call it a very large number of times, since it is the
> mechanism by which we recognize that we have already processed each
> sha1.
Yup. I have also futzed with the one in JGit for quite a while now. I
pull some tricks there like making it a 2 level directory to reduce
the need to find a contiguous array of 8M entries when processing the
Linux kernel, and I try to preallocate the first level table based on
the number of objects in pack-*.idx files. But the bottleneck is
basically the cache lookups and hits, these happen like 100M times on
2M objects, because its every link in nearly every tree.
Reachability bitmaps basically let you skip this. So they go fast. But
I have another that you could try.
If we modified pack-objects' delta compressor for tree objects to only
generate delta instructions at tree record boundaries, a delta-encoded
tree can be processed without inflating the full content of that tree.
Because of the way deltas are created, "most" tree deltas should have
their delta base scanned by the object traversal before the delta is
considered. This means the tree delta just needs to consider the much
smaller records that are inserted into the base. We know these are
different SHA-1s than what was there before, so they are more likely
to be new to the lookup_object table.
So the --objects traversal algorithm can change to get the delta base
SHA-1 and raw tree delta from the pack storage. Perform a
lookup_object on the base to see if it has been scanned. If it has,
just scan the delta insert instructions. If the base has not yet been
scanned, inflate the tree to its normal format and scan the entire
tree.
This is an approximation of what Nico and I were talking about doing
for pack v4. But doesn't require a file format change. :-)
^ 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