* Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs
From: Ian Kumlien @ 2012-02-24 16:16 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330086201-13916-2-git-send-email-pclouds@gmail.com>
On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
> This command unpacks every non-delta objects in order to:
>
> 1. calculate sha-1
> 2. do byte-to-byte sha-1 collision test if we happen to have objects
> with the same sha-1
> 3. validate object content in strict mode
>
> All this requires the entire object to stay in memory, a bad news for
> giant blobs. This patch lowers memory consumption by not saving the
> object in memory whenever possible, calculating SHA-1 while unpacking
> the object.
>
> This patch assumes that the collision test is rarely needed. The
> collision test will be done later in second pass if necessary, which
> puts the entire object back to memory again (We could even do the
> collision test without putting the entire object back in memory, by
> comparing as we unpack it).
>
> In strict mode, it always keeps non-blob objects in memory for
> validation (blobs do not need data validation). "--strict --verify"
> also keeps blobs in memory.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Writing objects: 100% (1425/1425), 56.06 MiB | 4.62 MiB/s, done.
Total 1425 (delta 790), reused 1425 (delta 790)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
To ../test_data/
! [remote rejected] master -> master (missing necessary objects)
! [remote rejected] origin/HEAD -> origin/HEAD (missing necessary objects)
! [remote rejected] origin/master -> origin/master (missing necessary objects)
error: failed to push some refs to '../test_data/'
So there are additional code paths to look at... =(
^ permalink raw reply
* Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs
From: Ian Kumlien @ 2012-02-24 15:37 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330086201-13916-2-git-send-email-pclouds@gmail.com>
On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
> This command unpacks every non-delta objects in order to:
>
> 1. calculate sha-1
> 2. do byte-to-byte sha-1 collision test if we happen to have objects
> with the same sha-1
> 3. validate object content in strict mode
>
> All this requires the entire object to stay in memory, a bad news for
> giant blobs. This patch lowers memory consumption by not saving the
> object in memory whenever possible, calculating SHA-1 while unpacking
> the object.
>
> This patch assumes that the collision test is rarely needed. The
> collision test will be done later in second pass if necessary, which
> puts the entire object back to memory again (We could even do the
> collision test without putting the entire object back in memory, by
> comparing as we unpack it).
>
> In strict mode, it always keeps non-blob objects in memory for
> validation (blobs do not need data validation). "--strict --verify"
> also keeps blobs in memory.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Finally, reapplied the patches and so on:
remote: Counting objects: 1425, done.
remote: Compressing objects: 100% (617/617), done.
remote: Total 1425 (delta 790), reused 1425 (delta 790)
Receiving objects: 100% (1425/1425), 56.06 MiB | 3.97 MiB/s, done.
Resolving deltas: 100% (790/790), done.
real 1m57.742s
user 0m29.950s
sys 0m6.308s
*YAY*
I wonder how the hell i could have missed several parts of the patch =(
But there seems to be some issue in gerrit 2.1.8, will have to check
against a newer gerrit to verify if it's still a problem.
FYI - it seems to hang doing nothing.
As for your patches:
Tested-by: Ian Kumlien <pomac@vapor.com>
;)
^ permalink raw reply
* Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs
From: Ian Kumlien @ 2012-02-24 14:40 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330086201-13916-2-git-send-email-pclouds@gmail.com>
On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
> This command unpacks every non-delta objects in order to:
>
> 1. calculate sha-1
> 2. do byte-to-byte sha-1 collision test if we happen to have objects
> with the same sha-1
> 3. validate object content in strict mode
>
> All this requires the entire object to stay in memory, a bad news for
> giant blobs. This patch lowers memory consumption by not saving the
> object in memory whenever possible, calculating SHA-1 while unpacking
> the object.
>
> This patch assumes that the collision test is rarely needed. The
> collision test will be done later in second pass if necessary, which
> puts the entire object back to memory again (We could even do the
> collision test without putting the entire object back in memory, by
> comparing as we unpack it).
>
> In strict mode, it always keeps non-blob objects in memory for
> validation (blobs do not need data validation). "--strict --verify"
> also keeps blobs in memory.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Actually, nevermind my last report - i had missed a merge :(
And now that i merged that part it seems like it doesn't do much..
(No real output for 2+ minutes)
I think i should reapply the patches again and verify that everything is
correct before reporting any additional progress.
But, this might not be before monday, unfortunately... But *thanks* for
posting the patches!
^ permalink raw reply
* FW: question about merge in 1.7.10
From: Marlene Cote @ 2012-02-24 14:33 UTC (permalink / raw)
To: git@vger.kernel.org
I used merge with -no-commit -no-ff. So, I assume I won't see any change in behavior, since merge is not performing commits, right?
------------------
Regards,
Marlene Cote, Release Engineering
978-268-0821
^ permalink raw reply
* Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs
From: Ian Kumlien @ 2012-02-24 14:30 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330086201-13916-2-git-send-email-pclouds@gmail.com>
On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
> This command unpacks every non-delta objects in order to:
>
> 1. calculate sha-1
> 2. do byte-to-byte sha-1 collision test if we happen to have objects
> with the same sha-1
> 3. validate object content in strict mode
>
> All this requires the entire object to stay in memory, a bad news for
> giant blobs. This patch lowers memory consumption by not saving the
> object in memory whenever possible, calculating SHA-1 while unpacking
> the object.
>
> This patch assumes that the collision test is rarely needed. The
> collision test will be done later in second pass if necessary, which
> puts the entire object back to memory again (We could even do the
> collision test without putting the entire object back in memory, by
> comparing as we unpack it).
>
> In strict mode, it always keeps non-blob objects in memory for
> validation (blobs do not need data validation). "--strict --verify"
> also keeps blobs in memory.
I applied both patches to git master, with some manual tinkering so i
might have missed some change that caused this to break.
But i get a segmentation fault and i just thought that i'd send you a
small trace before i even start trying to look in to this:
0xb7eb5b43 in SHA1_Update () from /lib/i686/cmov/libcrypto.so.0.9.8
(gdb) bt
#0 0xb7eb5b43 in SHA1_Update () from /lib/i686/cmov/libcrypto.so.0.9.8
#1 0x08116a2d in write_sha1_file_prepare
#2 0x08116a83 in hash_sha1_file
#3 0x0807c2a6 in sha1_object
#4 0x0807d74a in parse_pack_objects
#5 0x0807de6f in cmd_index_pack
#6 0x0804be97 in run_builtin
#7 handle_internal_command
#8 0x0804c0ad in run_argv
#9 main
Sorry about the censorship but i don't know how sensetive this data
is...
sha1_file.c:2343
---
static void write_sha1_file_prepare(const void *buf, unsigned long len,
const char *type, unsigned char *sha1,
char *hdr, int *hdrlen)
{
git_SHA_CTX c;
/* Generate the header */
*hdrlen = sprintf(hdr, "%s %lu", type, len)+1;
/* Sha1.. */
git_SHA1_Init(&c);
git_SHA1_Update(&c, hdr, *hdrlen);
git_SHA1_Update(&c, buf, len); <== this line fails.
git_HA1_Final(sha1, &c);
}
---
Just keep sending patches, i have atleast one git to test it on. ;)
^ permalink raw reply
* cvs2git multiple session for repository migration...
From: supadhyay @ 2012-02-24 14:08 UTC (permalink / raw)
To: git
Hi All,
I am migrating my CVS repository to GIt. As I have multiple repository I
want to start multiple session for cvs2git,but it failed to start. I
received cvs2svn.lock file error ? Is there any workaround to start multiple
cvs2git session (which migrate the CVS directory) ??
Thanks in advance...
--
View this message in context: http://git.661346.n2.nabble.com/cvs2git-multiple-session-for-repository-migration-tp7314909p7314909.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: Submodule status inside nested submodule fails
From: Charles Brossollet @ 2012-02-24 13:41 UTC (permalink / raw)
To: git
In-Reply-To: <loom.20120224T104003-230@post.gmane.org>
Charles Brossollet <chbrosso <at> lltech.fr> writes:
> Searching for message in git source I found the test triggering the message to
> be an empty result for "git rev-parse --show-cdup". Running the command in
> ext/submodule strangely returns the *absolute* path of ext/submodule, which
> should not happen because this command returns path of current dir relative to
> the containing working tree.
Things are OK if I run the commands from Windows' CMD, but it fails when run in
MinGW shell, so I believe the problem is related to MsysGit, discussion follows
up on their list.
^ permalink raw reply
* Re: [FYI] very large text files and their problems.
From: Ian Kumlien @ 2012-02-24 12:55 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8Aq=ofaHo3RkOBWTYP3eehZAU0E=HeDoUpR0nh+seKxzA@mail.gmail.com>
On Fri, Feb 24, 2012 at 06:14:46PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Fri, Feb 24, 2012 at 5:11 PM, Ian Kumlien <pomac@vapor.com> wrote:
> > I'm uncertain if you got my reply since i did it out of bounds - so i'll
> > repeat myself - sorry... =)
>
> yes I received it, just too busy this week.
Ah good, you never know what anti-spam measures people applies these
days... =)
And i have the same, so i totally understand.
> >> > git needs to have atleast the same ammount of memory as the largest
> >> > file free... Couldn't this be worked around?
> >> >
> >> > On a (32 bit) machine with 4GB memory - results in:
> >> > fatal: Out of memory, malloc failed (tried to allocate 3310214313 bytes)
> >> >
> >> > (and i see how this could be a problem, but couldn't it be mitigated? or
> >> > is it bydesign and intended behaviour?)
> >>
> >> I think that it's delta resolving that hogs all your memory. If your
> >> files are smaller than 512M, try lower core.bigFileThreshold. The
> >> topic jc/split-blob, which stores a big file are several smaller
> >> pieces, might solve your problem. Unfortunately the topic is not
> >> complete yet.
> >
> > Well, in this case it's just stream unpacking gzip data to disk, i
> > understand if delta would be a problem... But wouldn't delta be a
> > problem in the sence of <size_of_change>+<size_of_subdata>+<result> ?
> >
> > Ie, if the file is mmapped - it shouldn't have to be allocated, right?
>
> We should not delta large files. I was worried that the large file
> check could go wrong, But I guess your blob's not deltified in this
> case.
That would be correct
> When you receive a pack during a clone, the pack is streamed to
> index-pack, not mmapped, and index-pack checks every object in there
> in uncompressed form. I think I have found a way to avoid allocating
> that much. Need some more check, then send out.
Ah! That explains alot - do you have a publicly available version i
could look at?
> --
> Duy
^ permalink raw reply
* [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs
From: Nguyễn Thái Ngọc Duy @ 2012-02-24 12:23 UTC (permalink / raw)
To: git; +Cc: Ian Kumlien, Nguyễn Thái Ngọc Duy
In-Reply-To: <1330086201-13916-1-git-send-email-pclouds@gmail.com>
This command unpacks every non-delta objects in order to:
1. calculate sha-1
2. do byte-to-byte sha-1 collision test if we happen to have objects
with the same sha-1
3. validate object content in strict mode
All this requires the entire object to stay in memory, a bad news for
giant blobs. This patch lowers memory consumption by not saving the
object in memory whenever possible, calculating SHA-1 while unpacking
the object.
This patch assumes that the collision test is rarely needed. The
collision test will be done later in second pass if necessary, which
puts the entire object back to memory again (We could even do the
collision test without putting the entire object back in memory, by
comparing as we unpack it).
In strict mode, it always keeps non-blob objects in memory for
validation (blobs do not need data validation). "--strict --verify"
also keeps blobs in memory.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Does anybody do "git index-pack --stdin < .git/objects/pack/something"?
builtin/index-pack.c | 74 +++++++++++++++++++++++++++++++++++++++++---------
1 files changed, 61 insertions(+), 13 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index cee83b9..0c1f915 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -277,30 +277,60 @@ static void unlink_base_data(struct base_data *c)
free_base_data(c);
}
-static void *unpack_entry_data(unsigned long offset, unsigned long size)
+static void *unpack_entry_data(unsigned long offset, unsigned long size,
+ enum object_type type, unsigned char *sha1)
{
+ static char fixed_buf[8192];
int status;
git_zstream stream;
- void *buf = xmalloc(size);
+ void *buf;
+ git_SHA_CTX c;
+
+ if (sha1) { /* do hash_sha1_file internally */
+ char hdr[32];
+ int hdrlen = sprintf(hdr, "%s %lu", typename(type), size)+1;
+ git_SHA1_Init(&c);
+ git_SHA1_Update(&c, hdr, hdrlen);
+
+ buf = fixed_buf;
+ } else {
+ buf = xmalloc(size);
+ }
memset(&stream, 0, sizeof(stream));
git_inflate_init(&stream);
stream.next_out = buf;
- stream.avail_out = size;
+ stream.avail_out = buf == fixed_buf ? sizeof(fixed_buf) : size;
do {
stream.next_in = fill(1);
stream.avail_in = input_len;
status = git_inflate(&stream, 0);
use(input_len - stream.avail_in);
+ if (sha1) {
+ git_SHA1_Update(&c, buf, stream.next_out - (unsigned char *)buf);
+ stream.next_out = buf;
+ stream.avail_out = sizeof(fixed_buf);
+ }
} while (status == Z_OK);
if (stream.total_out != size || status != Z_STREAM_END)
bad_object(offset, "inflate returned %d", status);
git_inflate_end(&stream);
+ if (sha1) {
+ git_SHA1_Final(sha1, &c);
+ buf = NULL;
+ }
return buf;
}
-static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_base)
+static int is_delta_type(enum object_type type)
+{
+ return (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);
+}
+
+static void *unpack_raw_entry(struct object_entry *obj,
+ union delta_base *delta_base,
+ unsigned char *sha1)
{
unsigned char *p;
unsigned long size, c;
@@ -360,7 +390,17 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_
}
obj->hdr_size = consumed_bytes - obj->idx.offset;
- data = unpack_entry_data(obj->idx.offset, obj->size);
+ /*
+ * --verify --strict: sha1_object() does all collision test
+ * --strict: sha1_object() does all except blobs,
+ * blobs tested in second pass
+ * --verify : no collision test
+ * : all in second pass
+ */
+ if (is_delta_type(obj->type) ||
+ (strict && (verify || obj->type != OBJ_BLOB)))
+ sha1 = NULL; /* save unpacked object */
+ data = unpack_entry_data(obj->idx.offset, obj->size, obj->type, sha1);
obj->idx.crc32 = input_crc32;
return data;
}
@@ -461,8 +501,9 @@ static void find_delta_children(const union delta_base *base,
static void sha1_object(const void *data, unsigned long size,
enum object_type type, unsigned char *sha1)
{
- hash_sha1_file(data, size, typename(type), sha1);
- if ((strict || !verify) && has_sha1_file(sha1)) {
+ if (data)
+ hash_sha1_file(data, size, typename(type), sha1);
+ if (data && has_sha1_file(sha1)) {
void *has_data;
enum object_type has_type;
unsigned long has_size;
@@ -511,11 +552,6 @@ static void sha1_object(const void *data, unsigned long size,
}
}
-static int is_delta_type(enum object_type type)
-{
- return (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);
-}
-
/*
* This function is part of find_unresolved_deltas(). There are two
* walkers going in the opposite ways.
@@ -702,7 +738,7 @@ static void parse_pack_objects(unsigned char *sha1)
nr_objects);
for (i = 0; i < nr_objects; i++) {
struct object_entry *obj = &objects[i];
- void *data = unpack_raw_entry(obj, &delta->base);
+ void *data = unpack_raw_entry(obj, &delta->base, obj->idx.sha1);
obj->real_type = obj->type;
if (is_delta_type(obj->type)) {
nr_deltas++;
@@ -744,6 +780,9 @@ static void parse_pack_objects(unsigned char *sha1)
* - if used as a base, uncompress the object and apply all deltas,
* recursively checking if the resulting object is used as a base
* for some more deltas.
+ * - if the same object exists in repository and we're not in strict
+ * mode, we skipped the sha-1 collision test in the first pass.
+ * Do it now.
*/
if (verbose)
progress = start_progress("Resolving deltas", nr_deltas);
@@ -753,6 +792,15 @@ static void parse_pack_objects(unsigned char *sha1)
if (is_delta_type(obj->type))
continue;
+
+ if (((!strict && !verify) ||
+ (strict && !verify && obj->type == OBJ_BLOB)) &&
+ has_sha1_file(obj->idx.sha1)) {
+ void *data = get_data_from_pack(obj);
+ sha1_object(data, obj->size, obj->type, obj->idx.sha1);
+ free(data);
+ }
+
base_obj->obj = obj;
base_obj->data = NULL;
find_unresolved_deltas(base_obj);
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 1/2] Skip SHA-1 collision test on "index-pack --verify"
From: Nguyễn Thái Ngọc Duy @ 2012-02-24 12:23 UTC (permalink / raw)
To: git; +Cc: Ian Kumlien, Nguyễn Thái Ngọc Duy
index-pack --verify (or verify-pack) is about verifying the pack
itself. SHA-1 collision test is about outside (probably malicious)
objects with the same SHA-1 entering current repo.
SHA-1 collision test is currently done unconditionally. Which means if
you verify an in-repo pack, all objects from the pack will be checked
against objects in repo, which are themselves.
Skip this test for --verify, unless --strict is also specified.
linux-2.6 $ ls -sh .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack
401M .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack
Without the patch (and with another patch to cut out second pass in
index-pack):
linux-2.6 $ time ~/w/git/old index-pack -v --verify .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack
Indexing objects: 100% (1944656/1944656), done.
fatal: pack has 1617280 unresolved deltas
real 1m1.223s
user 0m55.028s
sys 0m0.828s
With the patch:
linux-2.6 $ time ~/w/git/git index-pack -v --verify .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack
Indexing objects: 100% (1944656/1944656), done.
fatal: pack has 1617280 unresolved deltas
real 0m41.714s
user 0m40.994s
sys 0m0.550s
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/index-pack.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index dd1c5c9..cee83b9 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -62,6 +62,7 @@ static int nr_resolved_deltas;
static int from_stdin;
static int strict;
+static int verify;
static int verbose;
static struct progress *progress;
@@ -461,7 +462,7 @@ static void sha1_object(const void *data, unsigned long size,
enum object_type type, unsigned char *sha1)
{
hash_sha1_file(data, size, typename(type), sha1);
- if (has_sha1_file(sha1)) {
+ if ((strict || !verify) && has_sha1_file(sha1)) {
void *has_data;
enum object_type has_type;
unsigned long has_size;
@@ -1078,7 +1079,7 @@ static void show_pack_info(int stat_only)
int cmd_index_pack(int argc, const char **argv, const char *prefix)
{
- int i, fix_thin_pack = 0, verify = 0, stat_only = 0, stat = 0;
+ int i, fix_thin_pack = 0, stat_only = 0, stat = 0;
const char *curr_pack, *curr_index;
const char *index_name = NULL, *pack_name = NULL;
const char *keep_name = NULL, *keep_msg = NULL;
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Re: [FYI] very large text files and their problems.
From: Nguyen Thai Ngoc Duy @ 2012-02-24 11:14 UTC (permalink / raw)
To: Ian Kumlien; +Cc: git
In-Reply-To: <20120224101121.GA9526@pomac.netswarm.net>
On Fri, Feb 24, 2012 at 5:11 PM, Ian Kumlien <pomac@vapor.com> wrote:
> I'm uncertain if you got my reply since i did it out of bounds - so i'll
> repeat myself - sorry... =)
yes I received it, just too busy this week.
>> > git needs to have atleast the same ammount of memory as the largest
>> > file free... Couldn't this be worked around?
>> >
>> > On a (32 bit) machine with 4GB memory - results in:
>> > fatal: Out of memory, malloc failed (tried to allocate 3310214313 bytes)
>> >
>> > (and i see how this could be a problem, but couldn't it be mitigated? or
>> > is it bydesign and intended behaviour?)
>>
>> I think that it's delta resolving that hogs all your memory. If your
>> files are smaller than 512M, try lower core.bigFileThreshold. The
>> topic jc/split-blob, which stores a big file are several smaller
>> pieces, might solve your problem. Unfortunately the topic is not
>> complete yet.
>
> Well, in this case it's just stream unpacking gzip data to disk, i
> understand if delta would be a problem... But wouldn't delta be a
> problem in the sence of <size_of_change>+<size_of_subdata>+<result> ?
>
> Ie, if the file is mmapped - it shouldn't have to be allocated, right?
We should not delta large files. I was worried that the large file
check could go wrong, But I guess your blob's not deltified in this
case.
When you receive a pack during a clone, the pack is streamed to
index-pack, not mmapped, and index-pack checks every object in there
in uncompressed form. I think I have found a way to avoid allocating
that much. Need some more check, then send out.
--
Duy
^ permalink raw reply
* Re: [PATCH] pretty: add '*' modifier to add LF after non-empty
From: Luc Pionchon @ 2012-02-24 10:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7gzd498r.fsf@alter.siamese.dyndns.org>
On Thu, Feb 23, 2012 at 21:53, Junio C Hamano <gitster@pobox.com> wrote:
>
> Luc Pionchon <pionchon.luc@gmail.com> writes:
>
> > Add the '*' modifier, similar to the '+' modifier,
> > to add a line-feed after a non-empty placeholder.
>
> Hrm, I thought I designed the plus and minus fairly carefully so that
> nobody needs to add this later.
>
> Wouldn't it be sufficient to write
>
> Foo%n%-d
>
>
> that says "We usually have LF after Foo, and write %d after that, but we
> might not have anything interesting in %d at all, in which case we don't
> add that %n"?
What I want is a LF after the non empty, rather than before.
It seems to me that %n%-d is equivalent to %+d,
or did I miss something?
See the 3 examples below, with %n%-d, %+d and %*d
$ ./git log -5 --graph --pretty=format:"%C(yellow bold)%n%-d %C(reset)%s"
*
|\ (HEAD, origin/master, origin/HEAD, master) Sync with 1.7.9.2
| *
| | (v1.7.9.2, origin/maint) Git 1.7.9.2
| * completion: use tabs for indentation
| * completion: remove stale "to submit patches" documentation
* | git-p4: the option to specify 'host' is -H, not -h
$ ./git log -5 --graph --pretty=format:"%C(yellow bold)%+d %C(reset)%s"
*
|\ (HEAD, origin/master, origin/HEAD, master) Sync with 1.7.9.2
| *
| | (v1.7.9.2, origin/maint) Git 1.7.9.2
| * completion: use tabs for indentation
| * completion: remove stale "to submit patches" documentation
* | git-p4: the option to specify 'host' is -H, not -h
$ ./git log -5 --graph --pretty=format:"%C(yellow bold)%*d %C(reset)%s"
* (HEAD, origin/master, origin/HEAD, master)
|\ Sync with 1.7.9.2
| * (v1.7.9.2, origin/maint)
| | Git 1.7.9.2
| * completion: use tabs for indentation
| * completion: remove stale "to submit patches" documentation
* | git-p4: the option to specify 'host' is -H, not -h
Also as a side note, I noticed that color is lost after new lines.
>
> > +test_expect_success 'add LF after non-empty (1) (empty)' '
> > + git show -s --pretty=format:"%*d%s%nfoo%n" HEAD^^ >actual &&
>
> Shouldn't this be equivalent to "%n%-d%s%nfoo%n", which in turn is covered
> by one of the previous tests (del LF before empty)?
I think the later is equivalent to %+d%s%nfoo%n, am I wrong?
^ permalink raw reply
* [BugReport] git tag -a / git show
From: Romain Vimont (®om) @ 2012-02-24 10:24 UTC (permalink / raw)
To: git
$ git --version
git version 1.7.5.4
$ git log --pretty=online
0ef41513d0b6d0ad28f21d0ac1da7096ad1dc6ff This is the last commit
a4702c69c28484d357179166cf3b116764da20a4 This is a commit
Now, I edit some files (for example in a config file "mock_data=true"),
then I want to tag without commiting this change.
$ git tag -a v0.1 -m 'My v0.1 with mock data'
$ git show v0.1
tag v0.1
Tagger: Me <me@me.me>
Date: Fri Feb 24 11:23:38 2012 +0100
My v0.1 with mock data
commit 0ef41513d0b6d0ad28f21d0ac1da7096ad1dc6ff
Author: Me <me@me.me>
Date: Fri Feb 24 11:14:19 2012 +0100
This is the last commit
diff --git a/myfile b/myfile
index 8430bf6..20feeb6 100644
...
And it shows the diff between a4702c69c28484d357179166cf3b116764da20a4
and 0ef41513d0b6d0ad28f21d0ac1da7096ad1dc6ff (the two last commits).
Instead, it should show the diff between
0ef41513d0b6d0ad28f21d0ac1da7096ad1dc6ff (the last commit) and v0.1 (the
tag).
Best regards,
®om
^ permalink raw reply
* Re: [FYI] very large text files and their problems.
From: Ian Kumlien @ 2012-02-24 10:11 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8Bdbegs7QdztvsFnKPcpAX5UL7s7uc37wF3_nF4kJQjrQ@mail.gmail.com>
I'm uncertain if you got my reply since i did it out of bounds - so i'll
repeat myself - sorry... =)
On Wed, Feb 22, 2012 at 11:18:19PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Wed, Feb 22, 2012 at 10:49 PM, Ian Kumlien <pomac@vapor.com> wrote:
> > Hi,
> >
> > We just saw a interesting issue, git compressed a ~3.4 gb project to ~57 mb.
>
> How big are those files? How many of them? How often do they change?
This was the first check in, there is no deltas yet.
The file in question is ~3.3 gb in size - ie exactly: 3310214313 bytes
(as seen below in the malloc failure)
git show <blob sha1 id> |wc -c gives the same exact result.
> > But when we tried to clone it on a big machine we got:
> >
> > fatal: Out of memory, malloc failed (tried to allocate
> > 18446744072724798634 bytes)
> >
> > This is already fixed in the 1.7.10 mainline - but it also seems like
>
> Does 1.7.9 have this problem?
Only tested 1.7.8 and 1.7.9.1 - works in mainline git (pre-1.7.10)
> > git needs to have atleast the same ammount of memory as the largest
> > file free... Couldn't this be worked around?
> >
> > On a (32 bit) machine with 4GB memory - results in:
> > fatal: Out of memory, malloc failed (tried to allocate 3310214313 bytes)
> >
> > (and i see how this could be a problem, but couldn't it be mitigated? or
> > is it bydesign and intended behaviour?)
>
> I think that it's delta resolving that hogs all your memory. If your
> files are smaller than 512M, try lower core.bigFileThreshold. The
> topic jc/split-blob, which stores a big file are several smaller
> pieces, might solve your problem. Unfortunately the topic is not
> complete yet.
Well, in this case it's just stream unpacking gzip data to disk, i
understand if delta would be a problem... But wouldn't delta be a
problem in the sence of <size_of_change>+<size_of_subdata>+<result> ?
Ie, if the file is mmapped - it shouldn't have to be allocated, right?
> --
> Duy
^ permalink raw reply
* Submodule status inside nested submodule fails
From: Charles Brossollet @ 2012-02-24 9:53 UTC (permalink / raw)
To: git
Hello there,
Using MSysGit 1.7.9 on Win7 (64 bit), I have a repo with the following
structure:
main/
src/
ext/
submodule/
modules/module1
modules/module2
submodule is... a submodule, having itself submodules.
When I query submodule status --recursive in main/, no problem.
But when I query submodule status in ext/submodule, I get error "You need to
run this command from the toplevel of the working tree.", which is a wrong
error message.
Searching for message in git source I found the test triggering the message to
be an empty result for "git rev-parse --show-cdup". Running the command in
ext/submodule strangely returns the *absolute* path of ext/submodule, which
should not happen because this command returns path of current dir relative to
the containing working tree.
Is there something wrong with my repo? Or have I found a bug? Any help
appreciated
Charles
^ permalink raw reply
* Improving merge messages for 1.7.10 and making "pull" easier
From: Ævar Arnfjörð Bjarmason @ 2012-02-24 9:59 UTC (permalink / raw)
To: Git Mailing List; +Cc: Thomas Rast, Junio C Hamano, Linus Torvalds
With 1.7.10 we'll get merge edit messages by default (which I like)
but I wonder if we could better clarify this to users when they see a
merge message pop up.
Firstly (and as a more general thing) I think we should add a mention
of "git merge --abort" to the message, just saving an empty file is
not sufficient to fully clear the merge state:
diff --git a/builtin/merge.c b/builtin/merge.c
index d3e1e8d..9169c74 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -889,8 +889,8 @@ static const char merge_editor_comment[] =
N_("Please enter a commit message to explain why this merge is
necessary,\n"
"especially if it merges an updated upstream into a topic branch.\n"
"\n"
- "Lines starting with '#' will be ignored, and an empty message aborts\n"
- "the commit.\n");
+ "Lines starting with '#' will be ignored, and an empty message
followed\n"
+ "by 'git merge --abort' the merge.\n");
static void prepare_to_commit(void)
{
Additionally, perhaps it would be a good idea to:
* Detect if the user didn't run this explicitly but implicitly from a
"git pull". We could pass some env var along or another option
(e.g. --internal-from-porcelain=pull) and add this:
You've merged implicitly via a "git pull", if you're just
updating some local work in progress to keep up with upstream
you may want to use "git pull --rebase" instead (or set the
pull.rebase configuration variable) to rebase instead of merge.
* Explicitly check if we're merging an updated upstream into the
work-in-progress topic, then mentioning some succinct version of
Junio's #1 point at
http://git-blame.blogspot.com/2012/02/anticipating-git-1710.html
I work with a lot of inexperienced git users and a lot of them are
going to be very confused by this change. I still think it's a good
change to make, but we could do a lot more to mitigate the inevitable
confusion.
One thing that would help these users in particular would be to have
some easy to use replacement for their frequent use of "git
pull".
They don't often commit their work (because of git inexperience) so
rebasing will error out because the tree is unclean. I tried to hack
support for unclean trees into git-rebase(1) the other day but it was
difficult because at various points in the process it'll do a "git
reset --hard".
Maybe having an option to "git pull" that would make it do:
git stash save --include-untracked -- "Automatically created stash
by \"git pull\" at $(date)"
git pull --rebase
git stash pop
Would make things easier. It could be called "git pull --rebase
--stash".
^ permalink raw reply
* git-svn: Please improve confusing error message "needs update" (when actually commit is needed) in rebase
From: Jakub Holy @ 2012-02-24 9:55 UTC (permalink / raw)
To: git
In-Reply-To: <CAFXq=J7hoUtb06LBAPhrbdmtHB8FGWpmUoz2yF-UJz8G=_smPA@mail.gmail.com>
Hi,
I hope this is the right way to submit this enhancement request.
When I run rebase with uncommited local changes, I get rather unclear error
message:
$ git svn rebase -v
mymodule/src/main/java/example/MyModifiedFile.java: needs update
rights-lib/src/main/java/example/AnotherModifiedFile.java: needs update
update-index --refresh: command returned error: 1
If the message was something like "You have uncommited changes in your
workspace, commit them before running rebase." (followed by a list of the
files) it would be much easier for newcomers like me to understand what has
to be done.
I tried to understand what has to be changed in the code but it exceeds my
low level of knowledge :-(
Thank you!
Best regards, Jakub Holy
--
Jakub Holy
Solutions Engineer | +47 966 23 666
Iterate AS | www.iterate.no
The Lean Software Development Consultancy
--
Jakub Holy
Solutions Engineer | +47 966 23 666
Iterate AS | www.iterate.no
The Lean Software Development Consultancy
^ permalink raw reply
* Re: git log -z doesn't separate commits with NULs
From: Jeff King @ 2012-02-24 9:52 UTC (permalink / raw)
To: Nikolaj Shurkaev; +Cc: Junio C Hamano, Jakub Narebski, git
In-Reply-To: <4F475689.4040203@gmail.com>
On Fri, Feb 24, 2012 at 12:21:13PM +0300, Nikolaj Shurkaev wrote:
> I'll propose to put something like this into git documentation
> --------------------------------------------------------------------------------------------
> diff --git a/Documentation/git-format-patch.txt
> b/Documentation/git-format-patch.txt
> index 6ea9be7..63267c6 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -23,6 +23,7 @@ SYNOPSIS
> [--cover-letter] [--quiet]
> [<common diff options>]
> [ <since> | <revision range> ]
> + [[\--] <path>...]
>
> DESCRIPTION
> -----------
> @@ -219,6 +220,12 @@ you can use `--suffix=-patch` to get
> `0001-description-of-my-change-patch`.
> range are always formatted as creation patches, independently
> of this flag.
>
> +[\--] <path>...::
> + Put in patches only those modifications that affect specified files
> + and folders. It's important to understand that log message of the
> + commit may become inappropriate because some parts of patch may be
> + cut off.
> +
I think that text looks OK. But to my mind, it is not that format-patch
accepts a path parameter, but rather that it takes arbitrary log-like
arguments. So you could do "git format-patch --grep=whatever", or even
something like "git format-patch --cherry".
I don't know how well tested every option is, though, so maybe it's not
a good idea to encourage the use of random options.
-Peff
^ permalink raw reply
* [PATCH 2/2] do not stream large files to pack when filters are in use
From: Jeff King @ 2012-02-24 9:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20120224093924.GA11680@sigill.intra.peff.net>
Because git's object format requires us to specify the
number of bytes in the object in its header, we must know
the size before streaming a blob into the object database.
This is not a problem when adding a regular file, as we can
get the size from stat(). However, when filters are in use
(such as autocrlf, or the ident, filter, or eol
gitattributes), we have no idea what the ultimate size will
be.
The current code just punts on the whole issue and ignores
filter configuration entirely for files larger than
core.bigfilethreshold. This can generate confusing results
if you use filters for large binary files, as the filter
will suddenly stop working as the file goes over a certain
size. Rather than try to handle unknown input sizes with
streaming, this patch just turns off the streaming
optimization when filters are in use.
This has a slight performance regression in a very specific
case: if you have autocrlf on, but no gitattributes, a large
binary file will avoid the streaming code path because we
don't know beforehand whether it will need conversion or
not. But if you are handling large binary files, you should
be marking them as such via attributes (or at least not
using autocrlf, and instead marking your text files as
such). And the flip side is that if you have a large
_non_-binary file, there is a correctness improvement;
before we did not apply the conversion at all.
The first half of the new t1051 script covers these failures
on input. The second half tests the matching output code
paths. These already work correctly, and do not need any
adjustment.
Signed-off-by: Jeff King <peff@peff.net>
---
I'm on the fence about the performance regression above. On the one
hand, if you're telling git to autocrlf your gigantic binary files, you
should fix your attributes, and expect not to use the streaming
optimization until you do. On the other hand, while I can see a remote
possibility of using filter.*.clean on a large binary file, the
probability that autocrlf would apply seems quite absurdly unlikely.
So rather than be pessimistic that filters will be used in the "auto"
case, perhaps we should be optimistic that they would not be. Or another
way of thinking about it is: from the CRLF code's perspective, anything
larger than core.bigfilethreshold should be considered binary without
actually looking at it, and we can say for sure (not the "might" from
the last patch) that such large things would not be converted in the
CRLF_GUESS case.
sha1_file.c | 3 +-
t/t1051-large-conversion.sh | 86 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+), 1 deletion(-)
create mode 100755 t/t1051-large-conversion.sh
diff --git a/sha1_file.c b/sha1_file.c
index f9f8d5e..61f597b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2720,7 +2720,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,
if (!S_ISREG(st->st_mode))
ret = index_pipe(sha1, fd, type, path, flags);
- else if (size <= big_file_threshold || type != OBJ_BLOB)
+ else if (size <= big_file_threshold || type != OBJ_BLOB ||
+ convert_to_git(path, NULL, 0, NULL, 0))
ret = index_core(sha1, fd, size, type, path, flags);
else
ret = index_stream(sha1, fd, size, type, path, flags);
diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh
new file mode 100755
index 0000000..8b7640b
--- /dev/null
+++ b/t/t1051-large-conversion.sh
@@ -0,0 +1,86 @@
+#!/bin/sh
+
+test_description='test conversion filters on large files'
+. ./test-lib.sh
+
+set_attr() {
+ test_when_finished 'rm -f .gitattributes' &&
+ echo "* $*" >.gitattributes
+}
+
+check_input() {
+ git read-tree --empty &&
+ git add small large &&
+ git cat-file blob :small >small.index &&
+ git cat-file blob :large | head -n 1 >large.index &&
+ test_cmp small.index large.index
+}
+
+check_output() {
+ rm -f small large &&
+ git checkout small large &&
+ head -n 1 large >large.head &&
+ test_cmp small large.head
+}
+
+test_expect_success 'setup input tests' '
+ printf "\$Id: foo\$\\r\\n" >small &&
+ cat small small >large &&
+ git config core.bigfilethreshold 20 &&
+ git config filter.test.clean "sed s/.*/CLEAN/"
+'
+
+test_expect_success 'autocrlf=true converts on input' '
+ test_config core.autocrlf true &&
+ check_input
+'
+
+test_expect_success 'eol=crlf converts on input' '
+ set_attr eol=crlf &&
+ check_input
+'
+
+test_expect_success 'ident converts on input' '
+ set_attr ident &&
+ check_input
+'
+
+test_expect_success 'user-defined filters convert on input' '
+ set_attr filter=test &&
+ check_input
+'
+
+test_expect_success 'setup output tests' '
+ echo "\$Id\$" >small &&
+ cat small small >large &&
+ git add small large &&
+ git config core.bigfilethreshold 7 &&
+ git config filter.test.smudge "sed s/.*/SMUDGE/"
+'
+
+test_expect_success 'autocrlf=true converts on output' '
+ test_config core.autocrlf true &&
+ check_output
+'
+
+test_expect_success 'eol=crlf converts on output' '
+ set_attr eol=crlf &&
+ check_output
+'
+
+test_expect_success 'user-defined filters convert on output' '
+ set_attr filter=test &&
+ check_output
+'
+
+test_expect_success 'ident converts on output' '
+ set_attr ident &&
+ rm -f small large &&
+ git checkout small large &&
+ sed -n "s/Id: .*/Id: SHA/p" <small >small.clean &&
+ head -n 1 large >large.head &&
+ sed -n "s/Id: .*/Id: SHA/p" <large.head >large.clean &&
+ test_cmp small.clean large.clean
+'
+
+test_done
--
1.7.9.9.g04d94
^ permalink raw reply related
* [PATCH 1/2] teach convert_to_git a "dry run" mode
From: Jeff King @ 2012-02-24 9:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20120224093924.GA11680@sigill.intra.peff.net>
Some callers may want to know whether convert_to_git will do
any conversion without actually feeding file data to it
(e.g., if you are using the decision to decide how to
acquire the data that might be converted). Rather than
replicate convert_to_git's logic in a separate function,
this patch lets callers pass a NULL buffer. The function
runs through its usual logic, except that it does not do any
actual conversion; the return value, instead of specifying
whether conversion happened, indicates whether conversion
might occur.
Note the use of the word "might" above. We cannot know for
sure whether conversion will occur without seeing the buffer
itself (because CRLF conversion may decide the file is
binary and do nothing). However, "might" is close enough for
callers which are trying to find out whether filters are in
used; they can be conservative and assume that the filters
will be used in such a case.
Signed-off-by: Jeff King <peff@peff.net>
---
convert.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/convert.c b/convert.c
index 12868ed..33aa72f 100644
--- a/convert.c
+++ b/convert.c
@@ -195,9 +195,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
char *dst;
if (crlf_action == CRLF_BINARY ||
- (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) || !len)
+ (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) ||
+ (src && !len))
return 0;
+ if (!src)
+ return 1;
+
gather_stats(src, len, &stats);
if (crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS) {
@@ -391,6 +395,9 @@ static int apply_filter(const char *path, const char *src, size_t len,
if (!cmd)
return 0;
+ if (!src)
+ return 1;
+
memset(&async, 0, sizeof(async));
async.proc = filter_buffer;
async.data = ¶ms;
@@ -522,9 +529,12 @@ static int ident_to_git(const char *path, const char *src, size_t len,
{
char *dst, *dollar;
- if (!ident || !count_ident(src, len))
+ if (!ident || (src && !count_ident(src, len)))
return 0;
+ if (!src)
+ return 1;
+
/* only grow if not in place */
if (strbuf_avail(buf) + buf->len < len)
strbuf_grow(buf, len - buf->len);
@@ -754,13 +764,13 @@ int convert_to_git(const char *path, const char *src, size_t len,
filter = ca.drv->clean;
ret |= apply_filter(path, src, len, dst, filter);
- if (ret) {
+ if (ret && src) {
src = dst->buf;
len = dst->len;
}
ca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);
ret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);
- if (ret) {
+ if (ret && src) {
src = dst->buf;
len = dst->len;
}
--
1.7.9.9.g04d94
^ permalink raw reply related
* Re: [BUG?] bulk checkin does not respect filters
From: Jeff King @ 2012-02-24 9:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20120224082803.GB18688@sigill.intra.peff.net>
On Fri, Feb 24, 2012 at 03:28:03AM -0500, Jeff King wrote:
> I don't like repeating all of the convert_to_git policy logic. Perhaps
> if you pass a NULL buffer to convert_to_git, it should run through its
> usual logic, stopping just short of actually writing anything, and
> return a flag indicating whether it _would_ convert...
So here's a series that does that.
[1/2]: teach convert_to_git a "dry run" mode
[2/2]: do not stream large files to pack when filters are in use
-Peff
^ permalink raw reply
* Re: git log -z doesn't separate commits with NULs
From: Nikolaj Shurkaev @ 2012-02-24 9:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jakub Narebski, git
In-Reply-To: <7vy5rt2u0c.fsf@alter.siamese.dyndns.org>
Hello.
Thank you for the hint.
git format-patch HEAD~3..HEAD -- SomePath
does exactly what I need. But that way of usage is not described in git
documentation thus I thought there is no way to do that. I've just
double checked
git format-patch --help
doesn't describe that. I'll propose to put something like this into git
documentation
--------------------------------------------------------------------------------------------
diff --git a/Documentation/git-format-patch.txt
b/Documentation/git-format-patch.txt
index 6ea9be7..63267c6 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -23,6 +23,7 @@ SYNOPSIS
[--cover-letter] [--quiet]
[<common diff options>]
[ <since> | <revision range> ]
+ [[\--] <path>...]
DESCRIPTION
-----------
@@ -219,6 +220,12 @@ you can use `--suffix=-patch` to get
`0001-description-of-my-change-patch`.
range are always formatted as creation patches, independently
of this flag.
+[\--] <path>...::
+ Put in patches only those modifications that affect specified files
+ and folders. It's important to understand that log message of the
+ commit may become inappropriate because some parts of patch may be
+ cut off.
+
CONFIGURATION
-------------
You can specify extra mail header lines to be added to each message,
--------------------------------------------------------------------------------------------
--
Nikolaj.
23.02.2012 23:07, Junio C Hamano пишет:
> Jeff King<peff@peff.net> writes:
>
>> On Thu, Feb 23, 2012 at 04:48:43PM +0300, Nikolaj Shurkaev wrote:
>>
>> Doesn't:
>>
>> git format-patch HEAD~3..HEAD SomePath
>>
>> do what you want? It is certainly designed to, and it seems to work for
>> me.
> It is not quite "designed to", though.
>
> It happens to work that way, and I do not think we want to forbid its use,
> but we would want to discourage anybody from blindly using it without
> thinking if the end results suits his/her purpose (and the reason should
> be obvious to those who think, the hint is "log message").
>
^ permalink raw reply related
* Re: [BUG?] bulk checkin does not respect filters
From: Jeff King @ 2012-02-24 8:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vvcmw2a3m.fsf@alter.siamese.dyndns.org>
On Thu, Feb 23, 2012 at 07:17:49PM -0800, Junio C Hamano wrote:
> > Thoughts? Was this intentional, or just overlooked?
>
> This is intentional in the sense it is not worth worrying about (I
> personally consider large and binary gunk equivalent and something nobody
> should care about anything more than 1. what the exact contents it had at
> each point in history and 2. if it did or did not change between two
> versions, but definitely not 3. how different these two versions were),
> and does not deserve the complexity to support filtering and textconv'ing.
We're purely in the hypothetical devil's advocate realm here, as I do
not use the filtering feature myself. But what if I had a filter that
canonicalized large binary files in some way (e.g., by re-zipping large
zipfiles using deterministic parameters). Even though they are large
binary gunk and you would never want to diff two versions, the important
thing is getting an identical sha1 when the underlying data has not
changed. I.e., your point 2 above, but applied to the "clean" repository
versions of files.
> If somebody strongly feels about lifting the limitation with a clean patch
> that does not harm the common case codepaths, patches are welcome, but I
> am not planning to do it myself ;-)
Perhaps something like this:
---
convert.c | 18 ++++++++++++++++++
convert.h | 1 +
sha1_file.c | 3 ++-
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/convert.c b/convert.c
index 12868ed..254301a 100644
--- a/convert.c
+++ b/convert.c
@@ -742,6 +742,24 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
}
}
+int want_convert_to_git(const char *path)
+{
+ struct conv_attrs ca;
+ convert_attrs(&ca, path);
+
+ if (ca.drv && ca.drv->clean)
+ return 1;
+ if (ca.ident)
+ return 1;
+
+ ca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);
+ if (ca.crlf_action == CRLF_BINARY)
+ return 0;
+ if (ca.crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE)
+ return 0;
+ return 1;
+}
+
int convert_to_git(const char *path, const char *src, size_t len,
struct strbuf *dst, enum safe_crlf checksafe)
{
diff --git a/convert.h b/convert.h
index d799a16..4d5936d 100644
--- a/convert.h
+++ b/convert.h
@@ -36,6 +36,7 @@ extern enum eol core_eol;
/* returns 1 if *dst was used */
extern int convert_to_git(const char *path, const char *src, size_t len,
struct strbuf *dst, enum safe_crlf checksafe);
+extern int want_convert_to_git(const char *path);
extern int convert_to_working_tree(const char *path, const char *src,
size_t len, struct strbuf *dst);
extern int renormalize_buffer(const char *path, const char *src, size_t len,
diff --git a/sha1_file.c b/sha1_file.c
index f9f8d5e..6c0e05c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2720,7 +2720,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,
if (!S_ISREG(st->st_mode))
ret = index_pipe(sha1, fd, type, path, flags);
- else if (size <= big_file_threshold || type != OBJ_BLOB)
+ else if (size <= big_file_threshold || type != OBJ_BLOB ||
+ want_convert_to_git(path))
ret = index_core(sha1, fd, size, type, path, flags);
else
ret = index_stream(sha1, fd, size, type, path, flags);
There should be no performance impact, as the new code only kicks in for
files that exceed big_file_threshold (and even then, it is just an extra
duplicate attr lookup, which the check_attr code caches anyway).
I don't like repeating all of the convert_to_git policy logic. Perhaps
if you pass a NULL buffer to convert_to_git, it should run through its
usual logic, stopping just short of actually writing anything, and
return a flag indicating whether it _would_ convert (this can't be 100%
accurate, as sometimes conversion depends on looking at the actual
contents of the buffer, but it could at least tell us "yes, I might
convert" versus "no, I will definitely not convert").
-Peff
^ permalink raw reply related
* Re: [BUG?] bulk checkin does not respect filters
From: Jeff King @ 2012-02-24 7:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vr4xk28z0.fsf@alter.siamese.dyndns.org>
On Thu, Feb 23, 2012 at 07:42:11PM -0800, Junio C Hamano wrote:
> It is a bit curious that anything filtered even goes to the streaming
> codepath, given this piece of code in write_entry() in entry.c:
>
> if (ce_mode_s_ifmt == S_IFREG) {
> struct stream_filter *filter = get_stream_filter(path, ce->sha1);
> if (filter &&
> !streaming_write_entry(ce, path, filter,
> state, to_tempfile,
> &fstat_done, &st))
> goto finish;
> }
>
> and get_stream_filter() in convert.c has an explicit exception for this
> case at the very beginning:
I think it is because we don't follow that code path at all. The stack trace
for "git add" looks on a large file looks like:
#0 stream_to_pack (...) at bulk-checkin.c:101
#1 deflate_to_pack (...) at bulk-checkin.c:219
#2 index_bulk_checkin (...) at bulk-checkin.c:258
#3 index_stream (...) at sha1_file.c:2712
#4 index_fd (...) at sha1_file.c:2726
#5 index_path (...) at sha1_file.c:2742
#6 add_to_index (...) at read-cache.c:644
#7 add_file_to_index (...) at read-cache.c:673
#8 add_files (...) at builtin/add.c:363
#9 cmd_add (...) at builtin/add.c:474
#10 run_builtin (...) at git.c:324
#11 handle_internal_command (...) at git.c:484
#12 run_argv (...) at git.c:530
#13 main (...) at git.c:605
Isn't write_entry the _other_ side of things. I.e., checking out, not
checking in? That side works fine (making the large file handling
for git-add even more odd. We will fail to apply the "clean" filter when
adding it, but we will apply the smudge filter when we write it out).
-Peff
^ permalink raw reply
* Re: [PATCH] index-pack: remove real_type from struct object_entry
From: Nguyen Thai Ngoc Duy @ 2012-02-24 6:08 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1330051320-19043-1-git-send-email-pclouds@gmail.com>
2012/2/24 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> @@ -581,7 +580,6 @@ static void resolve_delta(struct object_entry *delta_obj,
> {
> void *base_data, *delta_data;
>
> - delta_obj->real_type = base->obj->real_type;
> delta_obj->delta_depth = base->obj->delta_depth + 1;
> if (deepest_delta < delta_obj->delta_depth)
> deepest_delta = delta_obj->delta_depth;
This is wrong. Sorry for the noise.
--
Duy
^ 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