* [PATCH 08/11] index-pack: reduce memory usage when the pack has large blobs
From: Nguyễn Thái Ngọc Duy @ 2012-02-27 7:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1330329315-11407-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>
---
builtin/index-pack.c | 74 +++++++++++++++++++++++++++++++++++++++++---------
t/t1050-large.sh | 4 +-
2 files changed, 63 insertions(+), 15 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index e3cb684..86de813 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 && (strict || !verify) && 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.
@@ -690,10 +726,22 @@ static int compare_delta_entry(const void *a, const void *b)
* - 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.
*/
static void second_pass(struct object_entry *obj)
{
struct base_data *base_obj = alloc_base_data();
+
+ 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);
@@ -719,7 +767,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++;
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index 66acb3b..7e78c72 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -123,7 +123,7 @@ test_expect_success 'git-show a large file' '
'
-test_expect_failure 'clone' '
+test_expect_success 'clone' '
git clone -n file://"$PWD"/.git new &&
(
cd new &&
@@ -132,7 +132,7 @@ test_expect_failure 'clone' '
)
'
-test_expect_failure 'fetch updates' '
+test_expect_success 'fetch updates' '
echo modified >> large1 &&
git commit -q -a -m updated &&
(
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* [PATCH 07/11] index-pack: split second pass obj handling into own function
From: Nguyễn Thái Ngọc Duy @ 2012-02-27 7:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1330329315-11407-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/index-pack.c | 31 ++++++++++++++++++-------------
1 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index cee83b9..e3cb684 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -683,6 +683,23 @@ static int compare_delta_entry(const void *a, const void *b)
objects[delta_b->obj_no].type);
}
+/*
+ * Second pass:
+ * - for all non-delta objects, look if it is used as a base for
+ * deltas;
+ * - 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.
+ */
+static void second_pass(struct object_entry *obj)
+{
+ struct base_data *base_obj = alloc_base_data();
+ base_obj->obj = obj;
+ base_obj->data = NULL;
+ find_unresolved_deltas(base_obj);
+ display_progress(progress, nr_resolved_deltas);
+}
+
/* Parse all objects and return the pack content SHA1 hash */
static void parse_pack_objects(unsigned char *sha1)
{
@@ -737,26 +754,14 @@ static void parse_pack_objects(unsigned char *sha1)
qsort(deltas, nr_deltas, sizeof(struct delta_entry),
compare_delta_entry);
- /*
- * Second pass:
- * - for all non-delta objects, look if it is used as a base for
- * deltas;
- * - 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 (verbose)
progress = start_progress("Resolving deltas", nr_deltas);
for (i = 0; i < nr_objects; i++) {
struct object_entry *obj = &objects[i];
- struct base_data *base_obj = alloc_base_data();
if (is_delta_type(obj->type))
continue;
- base_obj->obj = obj;
- base_obj->data = NULL;
- find_unresolved_deltas(base_obj);
- display_progress(progress, nr_resolved_deltas);
+ second_pass(obj);
}
}
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* [PATCH 09/11] pack-check: do not unpack blobs
From: Nguyễn Thái Ngọc Duy @ 2012-02-27 7:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1330329315-11407-1-git-send-email-pclouds@gmail.com>
blob content is not used by verify_pack caller (currently only fsck),
we only need to make sure blob sha-1 signature matches its
content. unpack_entry() is taught to hash pack entry as it is
unpacked, eliminating the need to keep whole blob in memory.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache.h | 2 +-
fast-import.c | 2 +-
pack-check.c | 21 ++++++++++++++++++++-
sha1_file.c | 45 +++++++++++++++++++++++++++++++++++----------
t/t1050-large.sh | 2 +-
5 files changed, 58 insertions(+), 14 deletions(-)
diff --git a/cache.h b/cache.h
index 6ce691b..33bfb69 100644
--- a/cache.h
+++ b/cache.h
@@ -1065,7 +1065,7 @@ 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 is_pack_valid(struct packed_git *);
-extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);
+extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *, unsigned char *);
extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
extern int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);
diff --git a/fast-import.c b/fast-import.c
index 6cd19e5..5e94a64 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1303,7 +1303,7 @@ static void *gfi_unpack_entry(
*/
p->pack_size = pack_size + 20;
}
- return unpack_entry(p, oe->idx.offset, &type, sizep);
+ return unpack_entry(p, oe->idx.offset, &type, sizep, NULL);
}
static const char *get_mode(const char *str, uint16_t *modep)
diff --git a/pack-check.c b/pack-check.c
index 63a595c..1920bdb 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -105,6 +105,7 @@ static int verify_packfile(struct packed_git *p,
void *data;
enum object_type type;
unsigned long size;
+ off_t curpos = entries[i].offset;
if (p->index_version > 1) {
off_t offset = entries[i].offset;
@@ -116,7 +117,25 @@ static int verify_packfile(struct packed_git *p,
sha1_to_hex(entries[i].sha1),
p->pack_name, (uintmax_t)offset);
}
- data = unpack_entry(p, entries[i].offset, &type, &size);
+ type = unpack_object_header(p, w_curs, &curpos, &size);
+ unuse_pack(w_curs);
+ if (type == OBJ_BLOB) {
+ unsigned char sha1[20];
+ data = unpack_entry(p, entries[i].offset, &type, &size, sha1);
+ if (!data) {
+ if (hashcmp(entries[i].sha1, sha1))
+ err = error("packed %s from %s is corrupt",
+ sha1_to_hex(entries[i].sha1), p->pack_name);
+ else if (fn) {
+ int eaten = 0;
+ fn(entries[i].sha1, type, size, NULL, &eaten);
+ }
+ if (((base_count + i) & 1023) == 0)
+ display_progress(progress, base_count + i);
+ continue;
+ }
+ }
+ data = unpack_entry(p, entries[i].offset, &type, &size, NULL);
if (!data)
err = error("cannot unpack %s from %s at offset %"PRIuMAX"",
sha1_to_hex(entries[i].sha1), p->pack_name,
diff --git a/sha1_file.c b/sha1_file.c
index a77ef0a..d68a5b0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1653,28 +1653,51 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,
}
static void *unpack_compressed_entry(struct packed_git *p,
- struct pack_window **w_curs,
- off_t curpos,
- unsigned long size)
+ struct pack_window **w_curs,
+ off_t curpos,
+ unsigned long size,
+ enum object_type type,
+ unsigned char *sha1)
{
+ static unsigned char fixed_buf[8192];
int st;
git_zstream stream;
unsigned char *buffer, *in;
+ 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);
+
+ buffer = fixed_buf;
+ } else {
+ buffer = xmallocz(size);
+ }
- buffer = xmallocz(size);
memset(&stream, 0, sizeof(stream));
stream.next_out = buffer;
- stream.avail_out = size + 1;
+ stream.avail_out = buffer == fixed_buf ? sizeof(fixed_buf) : size + 1;
git_inflate_init(&stream);
do {
in = use_pack(p, w_curs, curpos, &stream.avail_in);
stream.next_in = in;
st = git_inflate(&stream, Z_FINISH);
- if (!stream.avail_out)
+ if (sha1) {
+ git_SHA1_Update(&c, buffer, stream.next_out - (unsigned char *)buffer);
+ stream.next_out = buffer;
+ stream.avail_out = sizeof(fixed_buf);
+ }
+ else if (!stream.avail_out)
break; /* the payload is larger than it should be */
curpos += stream.next_in - in;
} while (st == Z_OK || st == Z_BUF_ERROR);
+ if (sha1) {
+ git_SHA1_Final(sha1, &c);
+ buffer = NULL;
+ }
git_inflate_end(&stream);
if ((st != Z_STREAM_END) || stream.total_out != size) {
free(buffer);
@@ -1727,7 +1750,7 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
ret = ent->data;
if (!ret || ent->p != p || ent->base_offset != base_offset)
- return unpack_entry(p, base_offset, type, base_size);
+ return unpack_entry(p, base_offset, type, base_size, NULL);
if (!keep_cache) {
ent->data = NULL;
@@ -1844,7 +1867,7 @@ static void *unpack_delta_entry(struct packed_git *p,
return NULL;
}
- delta_data = unpack_compressed_entry(p, w_curs, curpos, delta_size);
+ delta_data = unpack_compressed_entry(p, w_curs, curpos, delta_size, OBJ_NONE, NULL);
if (!delta_data) {
error("failed to unpack compressed delta "
"at offset %"PRIuMAX" from %s",
@@ -1883,7 +1906,8 @@ static void write_pack_access_log(struct packed_git *p, off_t obj_offset)
int do_check_packed_object_crc;
void *unpack_entry(struct packed_git *p, off_t obj_offset,
- enum object_type *type, unsigned long *sizep)
+ enum object_type *type, unsigned long *sizep,
+ unsigned char *sha1)
{
struct pack_window *w_curs = NULL;
off_t curpos = obj_offset;
@@ -1917,7 +1941,8 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
case OBJ_TREE:
case OBJ_BLOB:
case OBJ_TAG:
- data = unpack_compressed_entry(p, &w_curs, curpos, *sizep);
+ data = unpack_compressed_entry(p, &w_curs, curpos,
+ *sizep, *type, sha1);
break;
default:
data = NULL;
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index 7e78c72..c749ecb 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -141,7 +141,7 @@ test_expect_success 'fetch updates' '
)
'
-test_expect_failure 'fsck' '
+test_expect_success 'fsck' '
git fsck --full
'
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* [PATCH 10/11] archive: support streaming large files to a tar archive
From: Nguyễn Thái Ngọc Duy @ 2012-02-27 7:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1330329315-11407-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
archive-tar.c | 35 ++++++++++++++++++++++++++++-------
archive-zip.c | 9 +++++----
archive.c | 51 ++++++++++++++++++++++++++++++++++-----------------
archive.h | 11 +++++++++--
t/t1050-large.sh | 2 +-
5 files changed, 77 insertions(+), 31 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index 20af005..5bffe49 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -5,6 +5,7 @@
#include "tar.h"
#include "archive.h"
#include "run-command.h"
+#include "streaming.h"
#define RECORDSIZE (512)
#define BLOCKSIZE (RECORDSIZE * 20)
@@ -123,9 +124,29 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)
return i;
}
+static void write_file(struct git_istream *stream, const void *buffer,
+ unsigned long size)
+{
+ if (!stream) {
+ write_blocked(buffer, size);
+ return;
+ }
+ for (;;) {
+ char buf[1024 * 16];
+ ssize_t readlen;
+
+ readlen = read_istream(stream, buf, sizeof(buf));
+
+ if (!readlen)
+ break;
+ write_blocked(buf, readlen);
+ }
+}
+
static int write_tar_entry(struct archiver_args *args,
- const unsigned char *sha1, const char *path, size_t pathlen,
- unsigned int mode, void *buffer, unsigned long size)
+ const unsigned char *sha1, const char *path,
+ size_t pathlen, unsigned int mode, void *buffer,
+ struct git_istream *stream, unsigned long size)
{
struct ustar_header header;
struct strbuf ext_header = STRBUF_INIT;
@@ -200,14 +221,14 @@ static int write_tar_entry(struct archiver_args *args,
if (ext_header.len > 0) {
err = write_tar_entry(args, sha1, NULL, 0, 0, ext_header.buf,
- ext_header.len);
+ NULL, ext_header.len);
if (err)
return err;
}
strbuf_release(&ext_header);
write_blocked(&header, sizeof(header));
- if (S_ISREG(mode) && buffer && size > 0)
- write_blocked(buffer, size);
+ if (S_ISREG(mode) && size > 0)
+ write_file(stream, buffer, size);
return err;
}
@@ -219,7 +240,7 @@ static int write_global_extended_header(struct archiver_args *args)
strbuf_append_ext_header(&ext_header, "comment", sha1_to_hex(sha1), 40);
err = write_tar_entry(args, NULL, NULL, 0, 0, ext_header.buf,
- ext_header.len);
+ NULL, ext_header.len);
strbuf_release(&ext_header);
return err;
}
@@ -308,7 +329,7 @@ static int write_tar_archive(const struct archiver *ar,
if (args->commit_sha1)
err = write_global_extended_header(args);
if (!err)
- err = write_archive_entries(args, write_tar_entry);
+ err = write_archive_entries(args, write_tar_entry, 1);
if (!err)
write_trailer();
return err;
diff --git a/archive-zip.c b/archive-zip.c
index 02d1f37..4a1e917 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -120,9 +120,10 @@ static void *zlib_deflate(void *data, unsigned long size,
return buffer;
}
-static int write_zip_entry(struct archiver_args *args,
- const unsigned char *sha1, const char *path, size_t pathlen,
- unsigned int mode, void *buffer, unsigned long size)
+int write_zip_entry(struct archiver_args *args,
+ const unsigned char *sha1, const char *path,
+ size_t pathlen, unsigned int mode, void *buffer,
+ struct git_istream *stream, unsigned long size)
{
struct zip_local_header header;
struct zip_dir_header dirent;
@@ -271,7 +272,7 @@ static int write_zip_archive(const struct archiver *ar,
zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
zip_dir_size = ZIP_DIRECTORY_MIN_SIZE;
- err = write_archive_entries(args, write_zip_entry);
+ err = write_archive_entries(args, write_zip_entry, 0);
if (!err)
write_zip_trailer(args->commit_sha1);
diff --git a/archive.c b/archive.c
index 1ee837d..257eadf 100644
--- a/archive.c
+++ b/archive.c
@@ -5,6 +5,7 @@
#include "archive.h"
#include "parse-options.h"
#include "unpack-trees.h"
+#include "streaming.h"
static char const * const archive_usage[] = {
"git archive [options] <tree-ish> [<path>...]",
@@ -59,26 +60,35 @@ static void format_subst(const struct commit *commit,
free(to_free);
}
-static void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
- unsigned int mode, enum object_type *type,
- unsigned long *sizep, const struct commit *commit)
+void sha1_file_to_archive(void **buffer, struct git_istream **stream,
+ const char *path, const unsigned char *sha1,
+ unsigned int mode, enum object_type *type,
+ unsigned long *sizep,
+ const struct commit *commit)
{
- void *buffer;
+ if (stream) {
+ struct stream_filter *filter;
+ filter = get_stream_filter(path, sha1);
+ if (!commit && S_ISREG(mode) && is_null_stream_filter(filter)) {
+ *buffer = NULL;
+ *stream = open_istream(sha1, type, sizep, NULL);
+ return;
+ }
+ *stream = NULL;
+ }
- buffer = read_sha1_file(sha1, type, sizep);
- if (buffer && S_ISREG(mode)) {
+ *buffer = read_sha1_file(sha1, type, sizep);
+ if (*buffer && S_ISREG(mode)) {
struct strbuf buf = STRBUF_INIT;
size_t size = 0;
- strbuf_attach(&buf, buffer, *sizep, *sizep + 1);
+ strbuf_attach(&buf, *buffer, *sizep, *sizep + 1);
convert_to_working_tree(path, buf.buf, buf.len, &buf);
if (commit)
format_subst(commit, buf.buf, buf.len, &buf);
- buffer = strbuf_detach(&buf, &size);
+ *buffer = strbuf_detach(&buf, &size);
*sizep = size;
}
-
- return buffer;
}
static void setup_archive_check(struct git_attr_check *check)
@@ -97,6 +107,7 @@ static void setup_archive_check(struct git_attr_check *check)
struct archiver_context {
struct archiver_args *args;
write_archive_entry_fn_t write_entry;
+ int stream_ok;
};
static int write_archive_entry(const unsigned char *sha1, const char *base,
@@ -109,6 +120,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
write_archive_entry_fn_t write_entry = c->write_entry;
struct git_attr_check check[2];
const char *path_without_prefix;
+ struct git_istream *stream = NULL;
int convert = 0;
int err;
enum object_type type;
@@ -133,25 +145,29 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
strbuf_addch(&path, '/');
if (args->verbose)
fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
- err = write_entry(args, sha1, path.buf, path.len, mode, NULL, 0);
+ err = write_entry(args, sha1, path.buf, path.len, mode, NULL, NULL, 0);
if (err)
return err;
return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
}
- buffer = sha1_file_to_archive(path_without_prefix, sha1, mode,
- &type, &size, convert ? args->commit : NULL);
- if (!buffer)
+ sha1_file_to_archive(&buffer, c->stream_ok ? &stream : NULL,
+ path_without_prefix, sha1, mode,
+ &type, &size, convert ? args->commit : NULL);
+ if (!buffer && !stream)
return error("cannot read %s", sha1_to_hex(sha1));
if (args->verbose)
fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
- err = write_entry(args, sha1, path.buf, path.len, mode, buffer, size);
+ err = write_entry(args, sha1, path.buf, path.len, mode, buffer, stream, size);
+ if (stream)
+ close_istream(stream);
free(buffer);
return err;
}
int write_archive_entries(struct archiver_args *args,
- write_archive_entry_fn_t write_entry)
+ write_archive_entry_fn_t write_entry,
+ int stream_ok)
{
struct archiver_context context;
struct unpack_trees_options opts;
@@ -167,13 +183,14 @@ int write_archive_entries(struct archiver_args *args,
if (args->verbose)
fprintf(stderr, "%.*s\n", (int)len, args->base);
err = write_entry(args, args->tree->object.sha1, args->base,
- len, 040777, NULL, 0);
+ len, 040777, NULL, NULL, 0);
if (err)
return err;
}
context.args = args;
context.write_entry = write_entry;
+ context.stream_ok = stream_ok;
/*
* Setup index and instruct attr to read index only
diff --git a/archive.h b/archive.h
index 2b0884f..370cca9 100644
--- a/archive.h
+++ b/archive.h
@@ -27,9 +27,16 @@ extern void register_archiver(struct archiver *);
extern void init_tar_archiver(void);
extern void init_zip_archiver(void);
-typedef int (*write_archive_entry_fn_t)(struct archiver_args *args, const unsigned char *sha1, const char *path, size_t pathlen, unsigned int mode, void *buffer, unsigned long size);
+struct git_istream;
+typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,
+ const unsigned char *sha1,
+ const char *path, size_t pathlen,
+ unsigned int mode,
+ void *buffer,
+ struct git_istream *stream,
+ unsigned long size);
-extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);
+extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry, int stream_ok);
extern int write_archive(int argc, const char **argv, const char *prefix, int setup_prefix, const char *name_hint, int remote);
const char *archive_format_from_filename(const char *filename);
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index c749ecb..1e64692 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -149,7 +149,7 @@ test_expect_success 'repack' '
git repack -ad
'
-test_expect_failure 'tar achiving' '
+test_expect_success 'tar achiving' '
git archive --format=tar HEAD >/dev/null
'
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* [PATCH 11/11] fsck: use streaming interface for writing lost-found blobs
From: Nguyễn Thái Ngọc Duy @ 2012-02-27 7:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1330329315-11407-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/fsck.c | 8 ++------
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 8c479a7..319b5c7 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -236,13 +236,9 @@ static void check_unreachable_object(struct object *obj)
if (!(f = fopen(filename, "w")))
die_errno("Could not open '%s'", filename);
if (obj->type == OBJ_BLOB) {
- enum object_type type;
- unsigned long size;
- char *buf = read_sha1_file(obj->sha1,
- &type, &size);
- if (buf && fwrite(buf, 1, size, f) != size)
+ if (streaming_write_sha1(fileno(f), 1,
+ obj->sha1, OBJ_BLOB, NULL))
die_errno("Could not write '%s'", filename);
- free(buf);
} else
fprintf(f, "%s\n", sha1_to_hex(obj->sha1));
if (fclose(f))
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* Re: [PATCH v6 00/11] Column display
From: Nguyen Thai Ngoc Duy @ 2012-02-27 8:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vipisbtw0.fsf@alter.siamese.dyndns.org>
On Mon, Feb 27, 2012 at 2:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Come to think of it, that is really the same distaste I had against both
> "diff --no-index" and "grep --no-index". We know adding our enhancements
> over non-git versions of these programs is a better gift to the outside
> world, but we instead added the --no-index mode to Git to only keep the
> benefit to ourselves, because it is far easier for us to do so.
Exactly. Some people like me also benefit from the popularity of Git.
If it's (likely) installed, I get many extra features for free. There
are machines that I'm not allowed to install new stuff on. If it comes
with BSD "column", not too bad. If not arghhh.
--
Duy
^ permalink raw reply
* Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-
From: Thomas Rast @ 2012-02-27 8:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, git, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <7vy5rpcgrk.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I would naïvely expect that it would be sufficient to update an existing
> definition for "--no-frotz" that uses PARSE_OPT_NONEG to instead define
> "--frotz" that by itself is a no-op, and "--no-frotz" would cause whatever
> the option currently means, with an update to the help text that says
> something to the effect that "--frotz by itself is meaningless and is
> always used as --no-frotz".
Doesn't that last quote already answer your question? It would be
rather awkward to see, in 'git apply -h',
--add Also apply additions in the patch. This is the
default; use --no-add to disable it.
Compare to the current concise wording
--no-add ignore additions made by the patch
which lists the main (or mainly used) form in the left column, and
doesn't have to implicitly mention the 'no-' convention again to make
the help display useful.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: cvs2git multiple session for repository migration...
From: supadhyay @ 2012-02-27 9:50 UTC (permalink / raw)
To: git
In-Reply-To: <4F48FACC.30800@alum.mit.edu>
Hi Michael,
I apologize. I have not realized if you already inform me before this. next
onwards I will move to cvs2svn mailing list.
Thanks for your reply.
--
View this message in context: http://git.661346.n2.nabble.com/cvs2git-multiple-session-for-repository-migration-tp7314909p7321288.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* git-cherries
From: Thien-Thi Nguyen @ 2012-02-27 10:56 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 385 bytes --]
For my personal use, i wrote git-cherries, attached.
It commits each hunk of every modified file separately
(creating cherries to cherry-pick later, you see).
I am writing to ask if this is already in Git somewhere,
and if not, for tips on how to make it faster / more elegant.
Please cc me in replies, as i am not subscribed.
Thanks!
_____________________________________________
[-- Attachment #2: git-cherries --]
[-- Type: application/octet-stream, Size: 454 bytes --]
#!/bin/sh
blurb="${1-cherry}"
git status --short \
| sed '/^ M /!d;s///' \
| while read filename
do
printf '\nprocessing: %s\n' $filename
n=0
while [ "$(git status --short -- $filename)" ]
do
n=$(expr 1 + $n)
printf 'y\nq\n' | git add --patch $filename >/dev/null
printf '%5d -- ' $n
git commit -m"$n $filename ($blurb)" \
| sed '1d;s/.* changed, //;s/[^0-9,]//g;s/,/& /'
done
done
^ permalink raw reply
* Re: [PATCH v4] Display change history as a diff between two dirs
From: Roland Kaufmann @ 2012-02-27 11:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tim Henigan, git
In-Reply-To: <7vlinpcewj.fsf@alter.siamese.dyndns.org>
On Mon, Feb 27, 2012 at 01:13, Junio C Hamano <gitster@pobox.com> wrote:
> How does this compare with Tim Henigan's "diffall"? I think the problem
> these two topics try to address is the same, and their approach may be
> similar enough that having one consolidated effort might be worth it.
Yes, I certainly agree. From a quick glance it seems to do exactly the
same thing (and both take inspirations from scripts already floating
around on the net); I guess that is a sign that it is an idea whose time
has come! :-)
The major difference is that Tim's version generate the list of files
and then extract them, whereas my current version piggybacks on
the external diff interface (as you suggested in one of the previous
reviews). Which of these approaches -- if any -- has the most merit
I think depends on the plans for that interface.
But until that's decided upon, it would have been nice if a version
could hang out in contrib/ to gather some experience.
--
Roland.
^ permalink raw reply
* [BUG] git branch --merged $unknown_checksum segfaults
From: Bernhard Reutner-Fischer @ 2012-02-27 12:26 UTC (permalink / raw)
To: git
Hi,
I hope this minor buglet is not known already..
I did not look further.
Seen with
$ dpkg-query -W git
git 1:1.7.2.5-3
and
git 1:1.7.9-1
cheers,
git init
git branch --merged 0000000000000000000000000000000000000000
Segmentation fault
touch f
git commit -m one f
[master (root-commit) f11ad60] one
0 files changed, 0 insertions(+), 0 deletions(-)
create mode 100644 f
git branch --merged $((echo "obase=16;ibase=16;$(git log -n1 --format='%H'| tr '[[:lower:]]' '[[:upper:]]')+1") | bc)
Segmentation fault
^ permalink raw reply
* Re: sha-1 check in rev-list --verify-objects redundant?
From: Nguyen Thai Ngoc Duy @ 2012-02-27 13:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <CACsJy8BUeedTZSq_ay=JmqUt3wrnm6n1eOcFt0WPkEo2B-1zwA@mail.gmail.com>
On Sun, Feb 26, 2012 at 06:11:30PM +0700, Nguyen Thai Ngoc Duy wrote:
> "rev-list --objects" does check for blob existence, in finish_object().
Eck.. I think "--quiet --verify-objects" becomes "--quiet --objects"
because of this code:
-- 8< --
traverse_commit_list(&revs,
quiet ? finish_commit : show_commit,
quiet ? finish_object : show_object,
&info);
-- 8< --
Unless that's intentional, shouldn't we apply this patch? --quiet's
interfering with rev-list's business sounds weird to me.
-- 8< --
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 264e3ae..95fb605 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -172,19 +172,18 @@ static void finish_object(struct object *obj,
const struct name_path *path, const char *name,
void *cb_data)
{
+ struct rev_list_info *info = cb_data;
if (obj->type == OBJ_BLOB && !has_sha1_file(obj->sha1))
die("missing blob object '%s'", sha1_to_hex(obj->sha1));
+ if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
+ parse_object(obj->sha1);
}
static void show_object(struct object *obj,
const struct name_path *path, const char *component,
void *cb_data)
{
- struct rev_list_info *info = cb_data;
-
finish_object(obj, path, component, cb_data);
- if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
- parse_object(obj->sha1);
show_object_with_name(stdout, obj, path, component);
}
-- 8< --
--
Duy
^ permalink raw reply related
* [PATCH] branch: don't assume the merge filter ref exists
From: Carlos Martín Nieto @ 2012-02-27 15:11 UTC (permalink / raw)
To: Bernhard Reutner-Fischer; +Cc: git
In-Reply-To: <20120227122609.GA26981@mx.loc>
print_ref_list looks up the merge_filter_ref and assumes that a valid
pointer is returned. When the object doesn't exist, it tries to
dereference a NULL pointer. This can be the case when git branch
--merged is given an argument that isn't a valid commit name.
Check whether the lookup returns a NULL pointer and die with an error
if it does. Add a test, while we're at it.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
It certainly looks like --merged was only ever supposed to be used
with branch names, as it assumed that get_sha1() would catch the
errors.
I'm not sure if "bad object" or "invalid object" fits better. "bad
object" might have a stronger implication that it exists but is
corrupt.
builtin/branch.c | 3 +++
t/t3200-branch.sh | 4 ++++
2 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index cb17bc3..b63d5fe 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -530,6 +530,9 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
if (merge_filter != NO_FILTER) {
struct commit *filter;
filter = lookup_commit_reference_gently(merge_filter_ref, 0);
+ if (!filter)
+ die("bad object %s", sha1_to_hex(merge_filter_ref));
+
filter->object.flags |= UNINTERESTING;
add_pending_object(&ref_list.revs,
(struct object *) filter, "");
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dd1aceb..9fe1d8f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -653,4 +653,8 @@ test_expect_success 'refuse --edit-description on unborn branch for now' '
)
'
+test_expect_success '--merged catches invalid object names' '
+ test_must_fail git branch --merged 0000000000000000000000000000000000000000
+'
+
test_done
--
1.7.9.rc0
^ permalink raw reply related
* Re: git compiled on same distro, different versions
From: Holger Hellmuth @ 2012-02-27 15:19 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: Junio C Hamano, Neal Kreitzinger, git
In-Reply-To: <4F4825CA.1060506@gmail.com>
On 25.02.2012 01:05, Neal Kreitzinger wrote:
> "high-level" question:
> If I compile git 1.7.9.2 (from git.git source) on RHEL6 test-box and
> test it and conclude that it "works right" is that sufficient for me to
> then go ahead and compile git 1.7.9.2 on RHEL5 real-box and
> expect/assume that it will also "work right"? IOW, will they produce the
> same results? Because if not then I have just potentially broken the
> real-box.
Depends on your thoroughness. I.e. you have to assume that your testing
does actually test aspects that are not covered by all the tests
included with git (and not detected by hundreds of other users on that
platform). In that case you can't guarantee anything, even applying
routine security patches to the operating system could potentially break
things (and some companies really test every single patch they apply to
mission-critical systems).
> "low-level" question:
> I suspect git calls linux commands alot. Git has "plumbing" commands
> that are not supposed to "break" scripts. Does linux also have
> "plumbing" commands that are not supposed to "break" scripts? Does git
> only use linux "plumbing" commands? Because if git commands uses linux
> "porcelain" then the linux "porcelain" change could cause git to change
> (not necessarily "break"). Maybe git-porcelain only uses
> linux-porcelain, and git-plumbing only uses linux-plumbing.
As far as I know there is no plumbing/porcelain distinction in the linux
kernel. But while the internal interfaces in the kernel change a lot,
the external interface (to user space programs like git) is relatively
fixed. You can assume that git is adapted to incompatible changes very
fast. But nobody can guarantee you that bugs won't make a difference
going from one platform to another.
^ permalink raw reply
* [PATCH] Perform cheaper connectivity check when pack is used as medium
From: Nguyễn Thái Ngọc Duy @ 2012-02-27 15:39 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
When we fetch or push, usually "git rev-list --verify-objects --not
--all --stdin" is used to make sure that there are no gaps between
existing refs and new refs. --verify-objects calls parse_object(),
which internally calls check_sha1_signature() to verify object content
matches its SHA-1 signature.
check_sha1_signature() is an expensive operation, especially when new
refs are far away from existing ones because all objects in between
are re-hashed. However, if we receive new objects by pack, we can
skip the operation because packs themselves do not contain SHA-1
signatures. All signatures are recreated by index-pack's hashing the
pack, which we can trust.
Detect pack transfer cases and turn --verify-objects to --objects.
--objects is similar to --verify-objects except that it does not call
check_sha1_signature().
As an (extreme) example, a repository is created with only one commit:
e83c516 (Initial revision of "git", the information manager from hell
- 2005-04-07). The rest of git.git is fetched on top. Without the
patch:
$ time git fetch file:///home/pclouds/w/git/.git
remote: Counting objects: 125638, done.
remote: Compressing objects: 100% (33201/33201), done.
remote: Total 125638 (delta 92568), reused 123517 (delta 90743)
Receiving objects: 100% (125638/125638), 34.58 MiB | 8.07 MiB/s, done.
Resolving deltas: 100% (92568/92568), done.
From file:///home/pclouds/w/git/
* branch HEAD -> FETCH_HEAD
real 1m30.972s
user 1m31.410s
sys 0m1.757s
With the patch:
$ time git fetch file:///home/pclouds/w/git/.git
remote: Counting objects: 125647, done.
remote: Compressing objects: 100% (33209/33209), done.
remote: Total 125647 (delta 92576), reused 123516 (delta 90744)
Receiving objects: 100% (125647/125647), 34.58 MiB | 7.99 MiB/s, done.
Resolving deltas: 100% (92576/92576), done.
From file:///home/pclouds/w/git/
* branch HEAD -> FETCH_HEAD
real 0m51.456s
user 0m52.737s
sys 0m1.548s
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/fetch.c | 12 +++++++-----
builtin/receive-pack.c | 4 ++--
connected.c | 5 ++++-
connected.h | 3 ++-
transport.c | 5 +++++
transport.h | 1 +
6 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 65f5f9b..2b62f42 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -368,7 +368,7 @@ static int iterate_ref_map(void *cb_data, unsigned char sha1[20])
}
static int store_updated_refs(const char *raw_url, const char *remote_name,
- struct ref *ref_map)
+ struct ref *ref_map, int pack_transport)
{
FILE *fp;
struct commit *commit;
@@ -389,7 +389,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
url = xstrdup("foreign");
rm = ref_map;
- if (check_everything_connected(iterate_ref_map, 0, &rm)) {
+ if (check_everything_connected(iterate_ref_map, 0,
+ pack_transport ? 0 : 1, &rm)) {
rc = error(_("%s did not send all necessary objects\n"), url);
goto abort;
}
@@ -516,7 +517,7 @@ static int quickfetch(struct ref *ref_map)
*/
if (depth)
return -1;
- return check_everything_connected(iterate_ref_map, 1, &rm);
+ return check_everything_connected(iterate_ref_map, 1, 0, &rm);
}
static int fetch_refs(struct transport *transport, struct ref *ref_map)
@@ -526,8 +527,9 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
ret |= store_updated_refs(transport->url,
- transport->remote->name,
- ref_map);
+ transport->remote->name,
+ ref_map,
+ is_pack_transport(transport));
transport_unlock_pack(transport);
return ret;
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0afb8b2..5935751 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -669,7 +669,7 @@ static void set_connectivity_errors(struct command *commands)
for (cmd = commands; cmd; cmd = cmd->next) {
struct command *singleton = cmd;
if (!check_everything_connected(command_singleton_iterator,
- 0, &singleton))
+ 0, 0, &singleton))
continue;
cmd->error_string = "missing necessary objects";
}
@@ -705,7 +705,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
cmd = commands;
if (check_everything_connected(iterate_receive_command_list,
- 0, &cmd))
+ 0, 0, &cmd))
set_connectivity_errors(commands);
if (run_receive_hook(commands, pre_receive_hook, 0)) {
diff --git a/connected.c b/connected.c
index d762423..6ebd330 100644
--- a/connected.c
+++ b/connected.c
@@ -14,7 +14,8 @@
*
* Returns 0 if everything is connected, non-zero otherwise.
*/
-int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)
+int check_everything_connected(sha1_iterate_fn fn, int quiet,
+ int strict, void *cb_data)
{
struct child_process rev_list;
const char *argv[] = {"rev-list", "--verify-objects",
@@ -26,6 +27,8 @@ int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)
if (fn(cb_data, sha1))
return err;
+ if (!strict)
+ argv[1] = "--objects";
if (quiet)
argv[5] = "--quiet";
diff --git a/connected.h b/connected.h
index 7e4585a..1f191da 100644
--- a/connected.h
+++ b/connected.h
@@ -15,6 +15,7 @@ typedef int (*sha1_iterate_fn)(void *, unsigned char [20]);
*
* Return 0 if Ok, non zero otherwise (i.e. some missing objects)
*/
-extern int check_everything_connected(sha1_iterate_fn, int quiet, void *cb_data);
+extern int check_everything_connected(sha1_iterate_fn, int quiet,
+ int strict, void *cb_data);
#endif /* CONNECTED_H */
diff --git a/transport.c b/transport.c
index 181f8f2..cd5e0ca 100644
--- a/transport.c
+++ b/transport.c
@@ -1248,3 +1248,8 @@ void for_each_alternate_ref(alternate_ref_fn fn, void *data)
cb.data = data;
foreach_alt_odb(refs_from_alternate_cb, &cb);
}
+
+int is_pack_transport(const struct transport *transport)
+{
+ return transport->fetch == fetch_refs_via_pack;
+}
diff --git a/transport.h b/transport.h
index ce99ef8..7cf72ff 100644
--- a/transport.h
+++ b/transport.h
@@ -150,6 +150,7 @@ int transport_disconnect(struct transport *transport);
char *transport_anonymize_url(const char *url);
void transport_take_over(struct transport *transport,
struct child_process *child);
+int is_pack_transport(const struct transport *transport);
int transport_connect(struct transport *transport, const char *name,
const char *exec, int fd[2]);
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH v4] contrib: added git-diffall
From: Tim Henigan @ 2012-02-24 19:48 UTC (permalink / raw)
To: git, gitster; +Cc: davvid, stefano.lattarini, tim.henigan
The 'git difftool' command allows the user to view diffs using an
external tool. It runs a separate instance of the tool for each
file in the diff. This makes it tedious to review changes spanning
multiple files.
The 'git-diffall' script instead prepares temporary directories
with the files to be compared and launches a single instance of
the external diff tool to view them (i.e. a directory diff).
The 'diff.tool' or 'merge.tool' configuration variable is used
to specify which external tool is used.
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
This script has been hosted on GitHub [1] since April 2010. Enough people
have found it useful that I hope it will be considered for inclusion in
the standard git install, either in contrib or as a new core command.
Changes in v4:
- Corrected location of hard-coded tmp dir
- Renamed 'path_sep' to 'dashdash_seen'
- Documented '--extcmd' in the README
- Changed test for '--extcmd=<command>' to use $#
- Documented that '--' is mandatory if a pathspec is given
v4 matches commit 251177b8e4 on GitHub [1].
Changes in v3:
- Fixed a bug that caused failures if file names included spaces
- Added unique suffix to tmp dir name (tmp/git-diffall-tmp.$$)
- Renamed "common_ancestor" to "merge_base"
- Cleaned up README to be more accurate
- Added useful error message if --extcmd is final option, but no
command was specified
- Removed spaces after redirection operators
v3 matches commit f36e4881e5 on GitHub [1].
Changes in v2:
- Changed to #!/bin/sh
- Eliminated use of 'which' statements
- Fixed trap function to actually run on abnormal exit
- Simplified path concatenation logic ($IFS)
- Corrected indentation errors
- Improved readability of while loop
- Cleaned up quoting of variables
v2 matches commit 5d4b90de3 on GitHub [1].
[1]: https://github.com/thenigan/git-diffall
contrib/diffall/README | 31 +++++
contrib/diffall/git-diffall | 261 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 292 insertions(+), 0 deletions(-)
create mode 100644 contrib/diffall/README
create mode 100755 contrib/diffall/git-diffall
diff --git a/contrib/diffall/README b/contrib/diffall/README
new file mode 100644
index 0000000..507f17d
--- /dev/null
+++ b/contrib/diffall/README
@@ -0,0 +1,31 @@
+The git-diffall script provides a directory based diff mechanism
+for git.
+
+To determine what diff viewer is used, the script requires either
+the 'diff.tool' or 'merge.tool' configuration option to be set.
+
+This script is compatible with most common forms used to specify a
+range of revisions to diff:
+
+ 1. git diffall: shows diff between working tree and staged changes
+ 2. git diffall --cached [<commit>]: shows diff between staged
+ changes and HEAD (or other named commit)
+ 3. git diffall <commit>: shows diff between working tree and named
+ commit
+ 4. git diffall <commit> <commit>: show diff between two named commits
+ 5. git diffall <commit>..<commit>: same as above
+ 6. git diffall <commit>...<commit>: show the changes on the branch
+ containing and up to the second, starting at a common ancestor
+ of both <commit>
+
+Note: all forms take an optional path limiter [-- <path>*]
+
+The '--extcmd=<command>' option allows the user to specify a custom
+command for viewing diffs. When given, configured defaults are
+ignored and the script runs $command $LOCAL $REMOTE. Additionally,
+$BASE is set in the environment.
+
+This script is based on an example provided by Thomas Rast on the
+Git list [1]:
+
+[1] http://thread.gmane.org/gmane.comp.version-control.git/124807
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
new file mode 100755
index 0000000..b9d51ca
--- /dev/null
+++ b/contrib/diffall/git-diffall
@@ -0,0 +1,261 @@
+#!/bin/sh
+# Copyright 2010 - 2012, Tim Henigan <tim.henigan@gmail.com>
+#
+# Perform a directory diff between commits in the repository using
+# the external diff or merge tool specified in the user's config.
+
+USAGE='[--cached] [--copy-back] [-x|--extcmd=<command>] <commit>{0,2} [-- <path>*]
+
+ --cached Compare to the index rather than the working tree.
+
+ --copy-back Copy files back to the working tree when the diff
+ tool exits (in case they were modified by the
+ user). This option is only valid if the diff
+ compared with the working tree.
+
+ -x=<command>
+ --extcmd=<command> Specify a custom command for viewing diffs.
+ git-diffall ignores the configured defaults and
+ runs $command $LOCAL $REMOTE when this option is
+ specified. Additionally, $BASE is set in the
+ environment.
+'
+
+SUBDIRECTORY_OK=1
+. "$(git --exec-path)/git-sh-setup"
+
+TOOL_MODE=diff
+. "$(git --exec-path)/git-mergetool--lib"
+
+merge_tool="$(get_merge_tool)"
+if test -z "$merge_tool"
+then
+ echo "Error: Either the 'diff.tool' or 'merge.tool' option must be set."
+ usage
+fi
+
+start_dir=$(pwd)
+
+# needed to access tar utility
+cdup=$(git rev-parse --show-cdup) &&
+cd "$cdup" || {
+ echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
+ exit 1
+}
+
+# mktemp is not available on all platforms (missing from msysgit)
+# Use a hard-coded tmp dir if it is not available
+tmp="$(mktemp -d -t tmp.XXXXXX 2>/dev/null)" || {
+ tmp=/tmp/git-diffall-tmp.$$
+ mkdir "$tmp" || exit 1
+}
+
+trap 'rm -rf "$tmp" 2>/dev/null' EXIT
+
+left=
+right=
+paths=
+dashdash_seen=
+compare_staged=
+merge_base=
+left_dir=
+right_dir=
+diff_tool=
+copy_back=
+
+while test $# != 0
+do
+ case "$1" in
+ -h|--h|--he|--hel|--help)
+ usage
+ ;;
+ --cached)
+ compare_staged=1
+ ;;
+ --copy-back)
+ copy_back=1
+ ;;
+ -x|--e|--ex|--ext|--extc|--extcm|--extcmd)
+ if test $# == 1
+ then
+ echo You must specify the tool for use with --extcmd
+ usage
+ else
+ diff_tool=$2
+ shift
+ fi
+ ;;
+ --)
+ dashdash_seen=1
+ ;;
+ -*)
+ echo Invalid option: "$1"
+ usage
+ ;;
+ *)
+ # could be commit, commit range or path limiter
+ case "$1" in
+ *...*)
+ left=${1%...*}
+ right=${1#*...}
+ merge_base=1
+ ;;
+ *..*)
+ left=${1%..*}
+ right=${1#*..}
+ ;;
+ *)
+ if test -n "$dashdash_seen"
+ then
+ paths="$paths$1 "
+ elif test -z "$left"
+ then
+ left=$1
+ elif test -z "$right"
+ then
+ right=$1
+ else
+ paths="$paths$1 "
+ fi
+ ;;
+ esac
+ ;;
+ esac
+ shift
+done
+
+# Determine the set of files which changed
+if test -n "$left" && test -n "$right"
+then
+ left_dir="cmt-$(git rev-parse --short $left)"
+ right_dir="cmt-$(git rev-parse --short $right)"
+
+ if test -n "$compare_staged"
+ then
+ usage
+ elif test -n "$merge_base"
+ then
+ git diff --name-only "$left"..."$right" -- $paths >"$tmp/filelist"
+ else
+ git diff --name-only "$left" "$right" -- $paths >"$tmp/filelist"
+ fi
+elif test -n "$left"
+then
+ left_dir="cmt-$(git rev-parse --short $left)"
+
+ if test -n "$compare_staged"
+ then
+ right_dir="staged"
+ git diff --name-only --cached "$left" -- $paths >"$tmp/filelist"
+ else
+ right_dir="working_tree"
+ git diff --name-only "$left" -- $paths >"$tmp/filelist"
+ fi
+else
+ left_dir="HEAD"
+
+ if test -n "$compare_staged"
+ then
+ right_dir="staged"
+ git diff --name-only --cached -- $paths >"$tmp/filelist"
+ else
+ right_dir="working_tree"
+ git diff --name-only -- $paths >"$tmp/filelist"
+ fi
+fi
+
+# Exit immediately if there are no diffs
+if test ! -s "$tmp/filelist"
+then
+ exit 0
+fi
+
+if test -n "$copy_back" && test "$right_dir" != "working_tree"
+then
+ echo "--copy-back is only valid when diff includes the working tree."
+ exit 1
+fi
+
+# Create the named tmp directories that will hold the files to be compared
+mkdir -p "$tmp/$left_dir" "$tmp/$right_dir"
+
+# Populate the tmp/right_dir directory with the files to be compared
+if test -n "$right"
+then
+ while read name
+ do
+ ls_list=$(git ls-tree $right "$name")
+ if test -n "$ls_list"
+ then
+ mkdir -p "$tmp/$right_dir/$(dirname "$name")"
+ git show "$right":"$name" >"$tmp/$right_dir/$name" || true
+ fi
+ done < "$tmp/filelist"
+elif test -n "$compare_staged"
+then
+ while read name
+ do
+ ls_list=$(git ls-files -- "$name")
+ if test -n "$ls_list"
+ then
+ mkdir -p "$tmp/$right_dir/$(dirname "$name")"
+ git show :"$name" >"$tmp/$right_dir/$name"
+ fi
+ done < "$tmp/filelist"
+else
+ # Mac users have gnutar rather than tar
+ (tar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && tar -x)) || {
+ gnutar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && gnutar -x)
+ }
+fi
+
+# Populate the tmp/left_dir directory with the files to be compared
+while read name
+do
+ if test -n "$left"
+ then
+ ls_list=$(git ls-tree $left "$name")
+ if test -n "$ls_list"
+ then
+ mkdir -p "$tmp/$left_dir/$(dirname "$name")"
+ git show "$left":"$name" >"$tmp/$left_dir/$name" || true
+ fi
+ else
+ if test -n "$compare_staged"
+ then
+ ls_list=$(git ls-tree HEAD "$name")
+ if test -n "$ls_list"
+ then
+ mkdir -p "$tmp/$left_dir/$(dirname "$name")"
+ git show HEAD:"$name" >"$tmp/$left_dir/$name"
+ fi
+ else
+ mkdir -p "$tmp/$left_dir/$(dirname "$name")"
+ git show :"$name" >"$tmp/$left_dir/$name"
+ fi
+ fi
+done < "$tmp/filelist"
+
+cd "$tmp"
+LOCAL="$left_dir"
+REMOTE="$right_dir"
+
+if test -n "$diff_tool"
+then
+ export BASE
+ eval $diff_tool '"$LOCAL"' '"$REMOTE"'
+else
+ run_merge_tool "$merge_tool" false
+fi
+
+# Copy files back to the working dir, if requested
+if test -n "$copy_back" && test "$right_dir" = "working_tree"
+then
+ cd "$start_dir"
+ git_top_dir=$(git rev-parse --show-toplevel)
+ find "$tmp/$right_dir" -type f |
+ while read file
+ do
+ cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
+ done
+fi
--
1.7.9.1
^ permalink raw reply related
* [PATCH] send-email: document the --smtp-debug option
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-27 16:22 UTC (permalink / raw)
To: git, gitster; +Cc: greened, Zbigniew Jędrzejewski-Szmek
In-Reply-To: <87ehtiu5dg.fsf@smith.obbligato.org>
The option was already shown in -h output, so it should be documented
in the man page.
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Suggested-by: David A. Greene <greened@obbligato.org>
---
David Greene wrote:
> I don't think --smtp-debug is documented in the man pages. Was that a
> deliberate decision or an oversight?
Documentation/git-send-email.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 327233c..3241170 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -198,6 +198,10 @@ must be used for each option.
if a username is not specified (with '--smtp-user' or 'sendemail.smtpuser'),
then authentication is not attempted.
+--smtp-debug=0|1::
+ Enable (1) or disable (0) debug output. If enabled, SMTP
+ commands and replies will be printed. Useful to debug TLS
+ connection and authentication problems.
Automating
~~~~~~~~~~
--
1.7.9.2.378.g4d260
^ permalink raw reply related
* [PATCH] grep -P: add tests for matching ^ and $
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-27 16:45 UTC (permalink / raw)
To: git, gitster; +Cc: michal.kiedrowicz, Zbigniew Jędrzejewski-Szmek
In-Reply-To: <7vlinpdxsu.fsf@alter.siamese.dyndns.org>
From: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
When a fix for matching ^ and $ with -P was commited to master in
fba4f1 (grep -P: Fix matching ^ and $), the tests were missing the
LIBPCRE prerequisite check and were dropped from the patch. Here are
the tests guarded with LIBPCRE.
Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
t/t7810-grep.sh | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 75f4716..d9ad633 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -47,6 +47,13 @@ test_expect_success setup '
echo vvv >t/v &&
mkdir t/a &&
echo vvv >t/a/v &&
+ {
+ echo "line without leading space1"
+ echo " line with leading space1"
+ echo " line with leading space2"
+ echo " line with leading space3"
+ echo "line without leading space2"
+ } >space &&
git add . &&
test_tick &&
git commit -m initial
@@ -893,4 +900,20 @@ test_expect_success 'mimic ack-grep --group' '
test_cmp expected actual
'
+cat >expected <<EOF
+space: line with leading space1
+space: line with leading space2
+space: line with leading space3
+EOF
+
+test_expect_success LIBPCRE 'grep -E "^ "' '
+ git grep -E "^ " space >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success LIBPCRE 'grep -P "^ "' '
+ git grep -P "^ " space >actual &&
+ test_cmp expected actual
+'
+
test_done
--
1.7.9.2.396.ga883d.dirty
^ permalink raw reply related
* Re: [RFC/PATCH] Make git-{pull,rebase} no-tracking message friendlier
From: Carlos Martín Nieto @ 2012-02-27 17:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhayh2t15.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]
On Thu, 2012-02-23 at 12:28 -0800, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
>
> > The current message is too long and at too low a level for anybody to
> > understand it if they don't know about the configuration format
> > already.
> >
> > Reformat it to show the commands a user would be expected to use,
> > instead of the contents of the configuration file.
> > ---
>
> Sounds like a change going in the right direction. I am unsure if it is a
> good idea to remove "See git-config...", but otherwise I like the updated
> text much better.
We're already telling them to go look at the git-pull or git-rebase
manpages, where there's already a lot of information and adding another
RTFM seemed a bit unfriendly (and not that useful, as they'd be
manipulating the config via other commands and shouldn't need to care
about config that much)
>
> But of course I am not the target audience, so let's see what we hear from
> others.
Sure, I'll see if I can come up with some better wording and send a
signed-off patch.
cmn
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-
From: Junio C Hamano @ 2012-02-27 17:18 UTC (permalink / raw)
To: Thomas Rast
Cc: René Scharfe, git, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <87d390smpa.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@inf.ethz.ch> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I would naïvely expect that it would be sufficient to update an existing
>> definition for "--no-frotz" that uses PARSE_OPT_NONEG to instead define
>> "--frotz" that by itself is a no-op, and "--no-frotz" would cause whatever
>> the option currently means, with an update to the help text that says
>> something to the effect that "--frotz by itself is meaningless and is
>> always used as --no-frotz".
>
> Doesn't that last quote already answer your question?
Yes, but only partly. I would agree with the awkwardness in
> It would be rather awkward to see, in 'git apply -h',
>
> --add Also apply additions in the patch. This is the
> default; use --no-add to disable it.
but it feeels somewhat questionable that the solution to get this:
>
> Compare to the current concise wording
>
> --no-add ignore additions made by the patch
is to define OPT_BOOL("no-add") that does not have any hint (other than
the fact that the option name begins with 3 character "no-") that this is
an already negated boolean and the "no-" negation can be removed.
This means an option "no-$foo" can never mean anything but "not foo". Not
that we would have to or necessarily want to support an option to give the
number of foo as --no-foo=47, as --num-foo=47 is a perfectly good spelling
for such an option.
If it were OPT_BOOL("no-foo", OPT_ISNEG | ...) that signals the parser
that:
- the option name is already negative;
- the leading "no-" is to be removed to negate it; and
- no extra leading "no-", i.e. "--no-no-foo", is accepted.
I probably wouldn't have felt this uneasy iffiness.
But it is just a minor issue.
^ permalink raw reply
* Re: [PATCH 02/11] Factor out and export large blob writing code to arbitrary file handle
From: Junio C Hamano @ 2012-02-27 17:29 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330329315-11407-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> cache.h | 3 +++
> entry.c | 39 ++++++++++++++++++++++++++-------------
> 2 files changed, 29 insertions(+), 13 deletions(-)
It was the goal of the original streaming output topic to helping more
callers stream the data out directly from the object store in order to
reduce memory pressure, and this series is very much in line with its
spirit.
The static version of streaming_write_entry() in entry.c was very specific
to writing out an index entry out to the working tree, and it made perfect
sense to have the function in that file, but its interface was limited to
the original context the function was used in.
The whole point of your refactoring in this patch is to make it available
for callers outside that original context; e.g. archive that finds blob
SHA-1 from a tree and writes the blob out to its standard output. They
should not have to work with an API that takes a cache-entry and writes to
a working tree file. And your result is much more generic.
So I think the external declaration and the definition should move to a
more generic place, namely streaming.[ch]. It does not belong to entry.c
anymore.
Thanks for working on this.
^ permalink raw reply
* [PATCH] git-p4: missing she-bang line in t9804 confuses prove
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-27 17:40 UTC (permalink / raw)
To: git, gitster; +Cc: luke, Zbigniew Jędrzejewski-Szmek
Without the magic line, prove shows lots and lots of errors:
% prove ./t9804-git-p4-label.sh
./t9804-git-p4-label.sh .. syntax error at ./t9804-git-p4-label.sh line 3, near ". ."
...
When #!/bin/sh is added, tests are skipped (I have no p4d).
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
t/t9804-git-p4-label.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/t9804-git-p4-label.sh b/t/t9804-git-p4-label.sh
index 80d01ea..a9e04ef 100755
--- a/t/t9804-git-p4-label.sh
+++ b/t/t9804-git-p4-label.sh
@@ -1,3 +1,5 @@
+#!/bin/sh
+
test_description='git-p4 p4 label tests'
. ./lib-git-p4.sh
--
1.7.9.2.396.ga883d.dirty
^ permalink raw reply related
* [PATCH 1/2] CodingGuidelines: Add a note about spaces after redirection
From: Tim Henigan @ 2012-02-24 21:12 UTC (permalink / raw)
To: git, gitster; +Cc: tim.henigan
During code review of some patches, it was noted that redirection operators
should have space before, but no space after them.
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
Documentation/CodingGuidelines | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 4830086..a4ffe7c 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -35,6 +35,11 @@ For shell scripts specifically (not exhaustive):
- Case arms are indented at the same depth as case and esac lines.
+ - Redirection operators should be written with space before, but
+ no space after them. For example:
+ 'echo test >$file' is preferred over
+ 'echo test > $file'
+
- We prefer $( ... ) for command substitution; unlike ``, it
properly nests. It should have been the way Bourne spelled
it from day one, but unfortunately isn't.
--
1.7.9.1
^ permalink raw reply related
* [PATCH 2/2] CodingGuidelines: Add note forbidding use of 'which' in shell scripts
From: Tim Henigan @ 2012-02-24 21:12 UTC (permalink / raw)
To: git, gitster; +Cc: tim.henigan
In-Reply-To: <1330117921-8257-1-git-send-email-tim.henigan@gmail.com>
During the code review of a recent patch, it was noted that shell scripts
must not use 'which'. The output of the command is not machine parseable
and its exit code is not reliable across platforms.
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
Documentation/CodingGuidelines | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index a4ffe7c..3505a4b 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -44,6 +44,10 @@ For shell scripts specifically (not exhaustive):
properly nests. It should have been the way Bourne spelled
it from day one, but unfortunately isn't.
+ - The use of 'which' is not allowed. The output of 'which' is not
+ machine parseable and its exit code is not reliable across
+ platforms.
+
- We use POSIX compliant parameter substitutions and avoid bashisms;
namely:
--
1.7.9.1
^ permalink raw reply related
* Re: [PATCH 03/11] cat-file: use streaming interface to print blobs
From: Junio C Hamano @ 2012-02-27 17:44 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330329315-11407-4-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/cat-file.c | 22 ++++++++++++++++++++++
> t/t1050-large.sh | 2 +-
> 2 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 8ed501f..3f3b558 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -82,6 +82,24 @@ static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long
> write_or_die(1, cp, endp - cp);
> }
>
> +static int write_blob(const unsigned char *sha1)
> +{
> + unsigned char new_sha1[20];
> +
> + if (sha1_object_info(sha1, NULL) == OBJ_TAG) {
This smells bad. Why in the world could an API be sane if lets a caller
call "write_blob()" with something that can be a tag?
Both of your callsites call this function when (type == OBJ_BLOB), but the
"case 0:" arm in the large switch in cat_one_file() only checks "expected
type" which may not match the real type at all, so it is wrong to switch
on that in the first place. In addition, that call site alone needs to
deref tag to the requested/expected type.
This block does not belong to this function, but to only one of its
callers among two.
> + enum object_type type;
> + unsigned long size;
> + char *buffer = read_sha1_file(sha1, &type, &size);
> + if (memcmp(buffer, "object ", 7) ||
> + get_sha1_hex(buffer + 7, new_sha1))
> + die("%s not a valid tag", sha1_to_hex(sha1));
> + sha1 = new_sha1;
> + free(buffer);
> + }
> +
> + return streaming_write_sha1(1, 0, sha1, OBJ_BLOB, NULL);
I do not think your previous refactoring added a fall-back codepath to the
function you are calling here. In the original context, the caller of
streaming_write_entry() made sure that the blob is suitable for streaming
write by getting an istream, and called the function only when that is the
case. Blobs unsuitable for streaming (e.g. an deltified object in a pack)
were handled by the caller that decided not to call
streaming_write_entry() with the conventional "read to core and then write
it out" codepath.
And I do not think your updated caller in cat_one_file() is equipped to do
so at all.
So it looks to me that this patch totally breaks the cat-file. What am I
missing?
> +}
> +
> static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> {
> unsigned char sha1[20];
> @@ -127,6 +145,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> return cmd_ls_tree(2, ls_args, NULL);
> }
>
> + if (type == OBJ_BLOB)
> + return write_blob(sha1);
> buf = read_sha1_file(sha1, &type, &size);
> if (!buf)
> die("Cannot read object %s", obj_name);
> @@ -149,6 +169,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> break;
>
> case 0:
> + if (type_from_string(exp_type) == OBJ_BLOB)
> + return write_blob(sha1);
> buf = read_object_with_reference(sha1, exp_type, &size, NULL);
> break;
>
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index f245e59..39a3e77 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -114,7 +114,7 @@ test_expect_success 'hash-object' '
> git hash-object large1
> '
>
> -test_expect_failure 'cat-file a large file' '
> +test_expect_success 'cat-file a large file' '
> git cat-file blob :large1 >/dev/null
> '
^ 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