* [PROPER PATCH 1/1] Make read_patch_file work on a strbuf.
From: Pierre Habouzit @ 2007-09-27 11:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20070927112204.GE10289@artemis.corp>
So that we don't need to use strbuf_detach.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
Sorry, in the hurry I sent a wrong patch before, this one works :)
builtin-apply.c | 27 +++++++++++----------------
1 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 833b142..1f0a672 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -178,12 +178,9 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, c
#define CHUNKSIZE (8192)
#define SLOP (16)
-static void *read_patch_file(int fd, size_t *sizep)
+static void read_patch_file(struct strbuf *sb, int fd)
{
- struct strbuf buf;
-
- strbuf_init(&buf, 0);
- if (strbuf_read(&buf, fd, 0) < 0)
+ if (strbuf_read(sb, fd, 0) < 0)
die("git-apply: read returned %s", strerror(errno));
/*
@@ -191,9 +188,8 @@ static void *read_patch_file(int fd, size_t *sizep)
* so that we can do speculative "memcmp" etc, and
* see to it that it is NUL-filled.
*/
- strbuf_grow(&buf, SLOP);
- memset(buf.buf + buf.len, 0, SLOP);
- return strbuf_detach(&buf, sizep);
+ strbuf_grow(sb, SLOP);
+ memset(sb->buf + sb->len, 0, SLOP);
}
static unsigned long linelen(const char *buffer, unsigned long size)
@@ -2648,22 +2644,22 @@ static void prefix_patches(struct patch *p)
static int apply_patch(int fd, const char *filename, int inaccurate_eof)
{
- unsigned long offset, size;
- char *buffer = read_patch_file(fd, &size);
+ size_t offset;
+ struct strbuf buf;
struct patch *list = NULL, **listp = &list;
int skipped_patch = 0;
+ strbuf_init(&buf, 0);
patch_input_file = filename;
- if (!buffer)
- return -1;
+ read_patch_file(&buf, fd);
offset = 0;
- while (size > 0) {
+ while (offset < buf.len) {
struct patch *patch;
int nr;
patch = xcalloc(1, sizeof(*patch));
patch->inaccurate_eof = inaccurate_eof;
- nr = parse_chunk(buffer + offset, size, patch);
+ nr = parse_chunk(buf.buf + offset, buf.len, patch);
if (nr < 0)
break;
if (apply_in_reverse)
@@ -2681,7 +2677,6 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
skipped_patch++;
}
offset += nr;
- size -= nr;
}
if (whitespace_error && (new_whitespace == error_on_whitespace))
@@ -2716,7 +2711,7 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
if (summary)
summary_patch_list(list);
- free(buffer);
+ strbuf_release(&buf);
return 0;
}
--
1.5.3.2.1102.gefa87-dirty
^ permalink raw reply related
* Re: Use of strbuf.buf when strbuf.len == 0
From: Pierre Habouzit @ 2007-09-27 11:37 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20070927112204.GE10289@artemis.corp>
[-- Attachment #1: Type: text/plain, Size: 646 bytes --]
On Thu, Sep 27, 2007 at 11:22:04AM +0000, Pierre Habouzit wrote:
> (it's arguable > that it's a right thing to assume though)
I find this ugly, so I've even checked if we did assume that, which is
easy now that such places all use strbuf_detach. One place seemed to,
but wasn't, so I patched it so that it's not the case anymore.
All other places that use strbuf_detach don't use the fact that it can
return NULL to detect error cases, so we are safe.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH 1/1] Make read_patch_file work on a strbuf.
From: Pierre Habouzit @ 2007-09-27 11:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20070927112204.GE10289@artemis.corp>
So that we don't need to use strbuf_detach.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
builtin-apply.c | 18 ++++++++----------
1 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 833b142..b0c6e60 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -178,7 +178,7 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, c
#define CHUNKSIZE (8192)
#define SLOP (16)
-static void *read_patch_file(int fd, size_t *sizep)
+static void read_patch_file(struct strbuf *sb, int fd)
{
struct strbuf buf;
@@ -193,7 +193,6 @@ static void *read_patch_file(int fd, size_t *sizep)
*/
strbuf_grow(&buf, SLOP);
memset(buf.buf + buf.len, 0, SLOP);
- return strbuf_detach(&buf, sizep);
}
static unsigned long linelen(const char *buffer, unsigned long size)
@@ -2648,22 +2647,22 @@ static void prefix_patches(struct patch *p)
static int apply_patch(int fd, const char *filename, int inaccurate_eof)
{
- unsigned long offset, size;
- char *buffer = read_patch_file(fd, &size);
+ size_t offset;
+ struct strbuf buf;
struct patch *list = NULL, **listp = &list;
int skipped_patch = 0;
+ strbuf_init(&buf, 0);
patch_input_file = filename;
- if (!buffer)
- return -1;
+ read_patch_file(&buf, fd);
offset = 0;
- while (size > 0) {
+ while (offset < buf.len) {
struct patch *patch;
int nr;
patch = xcalloc(1, sizeof(*patch));
patch->inaccurate_eof = inaccurate_eof;
- nr = parse_chunk(buffer + offset, size, patch);
+ nr = parse_chunk(buf.buf + offset, buf.len, patch);
if (nr < 0)
break;
if (apply_in_reverse)
@@ -2681,7 +2680,6 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
skipped_patch++;
}
offset += nr;
- size -= nr;
}
if (whitespace_error && (new_whitespace == error_on_whitespace))
@@ -2716,7 +2714,7 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
if (summary)
summary_patch_list(list);
- free(buffer);
+ strbuf_release(&buf);
return 0;
}
--
1.5.3.2.1101.g89e60-dirty
^ permalink raw reply related
* Re: Use of strbuf.buf when strbuf.len == 0
From: Pierre Habouzit @ 2007-09-27 11:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vir5wy6fv.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]
On Thu, Sep 27, 2007 at 06:21:24AM +0000, Junio C Hamano wrote:
> It would be appreciated if somebody with a fresh pair of eyes
> can go over the strbuf series one more time to make sure that we
> do not try to blindly use buf.buf, assuming buf.buf[0] is NUL if
> (buf.len == 0).
Like said in the 2/2 patch, I think it's better if people could be
able to always assume that and be done with it, else you have to know
this internal duality of the empty strbuf and it sucks.
Instead, what is important, is that people that initialized a strbuf,
then want to go back in the char* world gets a NULL if nothing was
allocated. It is a semantics that is used in a few places (it's arguable
that it's a right thing to assume though). For those, making
strbuf_detach use mandatory, and dealing with the special ->alloc == 0
case is the easiest way.
And as I don't trust my eyes to be fresh, I've used the aid of the
compiler to bust any place where we were using .buf members directly,
possibly doing something stupid.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH 2/2] strbuf change: be sure ->buf is never ever NULL.
From: Pierre Habouzit @ 2007-09-27 10:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20070927101300.GD10289@artemis.corp>
For that purpose, the ->buf is always initialized with a char * buf living
in the strbuf module. It is made a char * so that we can sloppily accept
things that perform: sb->buf[0] = '\0', and because you can't pass "" as an
initializer for ->buf without making gcc unhappy for very good reasons.
strbuf_init/_detach/_grow have been fixed to trust ->alloc and not ->buf
anymore.
as a consequence strbuf_detach is _mandatory_ to detach a buffer, copying
->buf isn't an option anymore, if ->buf is going to escape from the scope,
and eventually be free'd.
API changes:
* strbuf_setlen now always works, so just make strbuf_reset a convenience
macro.
* strbuf_detatch takes a size_t* optional argument (meaning it can be
NULL) to copy the buffer's len, as it was needed for this refactor to
make the code more readable, and working like the callers.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
I've written this patch using the following method:
(1) I've renamed the ->buf member into sb_buf (I grepped before it wasn't
a string used in the project) and made the changes I describe in
strbuf.[hc]
(2) I've built the project, and renamed every ->buf into sb_buf, and
applied the needed semantics changes. It's doing that that I found the
issue I fix in the patch 1/2.
(3) I've then run the testsuite, it passes.
(4) I've sed -i -e 's/\<sb_buf\>/buf/g' *.h *.c
Nobody was directly using the fact that a strbuf that wasn't touched had
its pointer NULL, though people detach'ing them do, and strbuf_detach
complies with that. That's why I think, despite the somehow tasteless
"strbuf_slopbuf" it's the best way to go.
builtin-apply.c | 16 +++++++---------
builtin-archive.c | 5 ++---
builtin-fetch--tool.c | 2 +-
commit.c | 2 +-
convert.c | 4 ++--
diff.c | 14 ++++++--------
entry.c | 3 +--
fast-import.c | 2 +-
imap-send.c | 2 +-
quote.c | 2 +-
sha1_file.c | 3 +--
strbuf.c | 27 ++++++++++++++++-----------
strbuf.h | 10 +++++-----
13 files changed, 45 insertions(+), 47 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 450f0a8..833b142 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -178,14 +178,13 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, c
#define CHUNKSIZE (8192)
#define SLOP (16)
-static void *read_patch_file(int fd, unsigned long *sizep)
+static void *read_patch_file(int fd, size_t *sizep)
{
struct strbuf buf;
strbuf_init(&buf, 0);
if (strbuf_read(&buf, fd, 0) < 0)
die("git-apply: read returned %s", strerror(errno));
- *sizep = buf.len;
/*
* Make sure that we have some slop in the buffer
@@ -194,7 +193,7 @@ static void *read_patch_file(int fd, unsigned long *sizep)
*/
strbuf_grow(&buf, SLOP);
memset(buf.buf + buf.len, 0, SLOP);
- return strbuf_detach(&buf);
+ return strbuf_detach(&buf, sizep);
}
static unsigned long linelen(const char *buffer, unsigned long size)
@@ -253,7 +252,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
*/
strbuf_remove(&name, 0, cp - name.buf);
free(def);
- return name.buf;
+ return strbuf_detach(&name, NULL);
}
}
strbuf_release(&name);
@@ -607,7 +606,7 @@ static char *git_header_name(char *line, int llen)
if (strcmp(cp + 1, first.buf))
goto free_and_fail1;
strbuf_release(&sp);
- return first.buf;
+ return strbuf_detach(&first, NULL);
}
/* unquoted second */
@@ -618,7 +617,7 @@ static char *git_header_name(char *line, int llen)
if (line + llen - cp != first.len + 1 ||
memcmp(first.buf, cp, first.len))
goto free_and_fail1;
- return first.buf;
+ return strbuf_detach(&first, NULL);
free_and_fail1:
strbuf_release(&first);
@@ -655,7 +654,7 @@ static char *git_header_name(char *line, int llen)
isspace(name[len])) {
/* Good */
strbuf_remove(&sp, 0, np - sp.buf);
- return sp.buf;
+ return strbuf_detach(&sp, NULL);
}
free_and_fail2:
@@ -1968,8 +1967,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
if (apply_fragments(&buf, patch) < 0)
return -1; /* note with --reject this succeeds. */
- patch->result = buf.buf;
- patch->resultsize = buf.len;
+ patch->result = strbuf_detach(&buf, &patch->resultsize);
if (0 < patch->is_delete && patch->resultsize)
return error("removal patch leaves file contents");
diff --git a/builtin-archive.c b/builtin-archive.c
index 843a9e3..04385de 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -89,7 +89,7 @@ static void format_subst(const struct commit *commit,
struct strbuf fmt;
if (src == buf->buf)
- to_free = strbuf_detach(buf);
+ to_free = strbuf_detach(buf, NULL);
strbuf_init(&fmt, 0);
for (;;) {
const char *b, *c;
@@ -153,8 +153,7 @@ void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
strbuf_attach(&buf, buffer, *sizep, *sizep + 1);
convert_to_working_tree(path, buf.buf, buf.len, &buf);
convert_to_archive(path, buf.buf, buf.len, &buf, commit);
- *sizep = buf.len;
- buffer = buf.buf;
+ buffer = strbuf_detach(&buf, sizep);
}
return buffer;
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 349b59c..1e43d79 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -10,7 +10,7 @@ static char *get_stdin(void)
if (strbuf_read(&buf, 0, 1024) < 0) {
die("error reading standard input: %s", strerror(errno));
}
- return strbuf_detach(&buf);
+ return strbuf_detach(&buf, NULL);
}
static void show_new(enum object_type type, unsigned char *sha1_new)
diff --git a/commit.c b/commit.c
index 1e391e6..20fb220 100644
--- a/commit.c
+++ b/commit.c
@@ -663,7 +663,7 @@ static char *replace_encoding_header(char *buf, const char *encoding)
len - strlen("encoding \n"),
encoding, strlen(encoding));
}
- return tmp.buf;
+ return strbuf_detach(&tmp, NULL);
}
static char *logmsg_reencode(const struct commit *commit,
diff --git a/convert.c b/convert.c
index 79c9df2..0d5e909 100644
--- a/convert.c
+++ b/convert.c
@@ -168,7 +168,7 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,
/* are we "faking" in place editing ? */
if (src == buf->buf)
- to_free = strbuf_detach(buf);
+ to_free = strbuf_detach(buf, NULL);
strbuf_grow(buf, len + stats.lf - stats.crlf);
for (;;) {
@@ -464,7 +464,7 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
/* are we "faking" in place editing ? */
if (src == buf->buf)
- to_free = strbuf_detach(buf);
+ to_free = strbuf_detach(buf, NULL);
hash_sha1_file(src, len, "blob", sha1);
strbuf_grow(buf, len + cnt * 43);
diff --git a/diff.c b/diff.c
index 4a7f1e1..0bd7e24 100644
--- a/diff.c
+++ b/diff.c
@@ -197,7 +197,7 @@ static char *quote_two(const char *one, const char *two)
strbuf_addstr(&res, one);
strbuf_addstr(&res, two);
}
- return res.buf;
+ return strbuf_detach(&res, NULL);
}
static const char *external_diff(void)
@@ -662,7 +662,7 @@ static char *pprint_rename(const char *a, const char *b)
quote_c_style(a, &name, NULL, 0);
strbuf_addstr(&name, " => ");
quote_c_style(b, &name, NULL, 0);
- return name.buf;
+ return strbuf_detach(&name, NULL);
}
/* Find common prefix */
@@ -710,7 +710,7 @@ static char *pprint_rename(const char *a, const char *b)
strbuf_addch(&name, '}');
strbuf_add(&name, a + len_a - sfx_length, sfx_length);
}
- return name.buf;
+ return strbuf_detach(&name, NULL);
}
struct diffstat_t {
@@ -827,7 +827,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
strbuf_init(&buf, 0);
if (quote_c_style(file->name, &buf, NULL, 0)) {
free(file->name);
- file->name = buf.buf;
+ file->name = strbuf_detach(&buf, NULL);
} else {
strbuf_release(&buf);
}
@@ -1519,8 +1519,7 @@ static int populate_from_stdin(struct diff_filespec *s)
strerror(errno));
s->should_munmap = 0;
- s->size = buf.len;
- s->data = strbuf_detach(&buf);
+ s->data = strbuf_detach(&buf, &s->size);
s->should_free = 1;
return 0;
}
@@ -1612,8 +1611,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
if (convert_to_git(s->path, s->data, s->size, &buf)) {
munmap(s->data, s->size);
s->should_munmap = 0;
- s->data = buf.buf;
- s->size = buf.len;
+ s->data = strbuf_detach(&buf, &s->size);
s->should_free = 1;
}
}
diff --git a/entry.c b/entry.c
index 4a8c73b..98f5f6d 100644
--- a/entry.c
+++ b/entry.c
@@ -120,8 +120,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
strbuf_init(&buf, 0);
if (convert_to_working_tree(ce->name, new, size, &buf)) {
free(new);
- new = buf.buf;
- size = buf.len;
+ new = strbuf_detach(&buf, &size);
}
if (to_tempfile) {
diff --git a/fast-import.c b/fast-import.c
index a870a44..e9c80be 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1562,7 +1562,7 @@ static int read_next_command(void)
} else {
struct recent_command *rc;
- strbuf_detach(&command_buf);
+ strbuf_detach(&command_buf, NULL);
stdin_eof = strbuf_getline(&command_buf, stdin, '\n');
if (stdin_eof)
return EOF;
diff --git a/imap-send.c b/imap-send.c
index e95cdde..a429a76 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1180,7 +1180,7 @@ read_message( FILE *f, msg_data_t *msg )
} while (!feof(f));
msg->len = buf.len;
- msg->data = strbuf_detach(&buf);
+ msg->data = strbuf_detach(&buf, NULL);
return msg->len;
}
diff --git a/quote.c b/quote.c
index 800fd88..482be05 100644
--- a/quote.c
+++ b/quote.c
@@ -22,7 +22,7 @@ void sq_quote_buf(struct strbuf *dst, const char *src)
char *to_free = NULL;
if (dst->buf == src)
- to_free = strbuf_detach(dst);
+ to_free = strbuf_detach(dst, NULL);
strbuf_addch(dst, '\'');
while (*src) {
diff --git a/sha1_file.c b/sha1_file.c
index f1377fb..83a06a7 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2340,8 +2340,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
strbuf_init(&nbuf, 0);
if (convert_to_git(path, buf, size, &nbuf)) {
munmap(buf, size);
- size = nbuf.len;
- buf = nbuf.buf;
+ buf = strbuf_detach(&nbuf, &size);
re_allocated = 1;
}
}
diff --git a/strbuf.c b/strbuf.c
index d5e92ee..450110d 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,27 +1,30 @@
#include "cache.h"
+/* used as the default ->buf value, so that people can always assume buf is
+ non NULL and ->buf[0] is '\0' */
+char strbuf_slopbuf[32];
+
void strbuf_init(struct strbuf *sb, size_t hint)
{
- memset(sb, 0, sizeof(*sb));
+ sb->alloc = sb->len = 0;
+ sb->buf = strbuf_slopbuf;
if (hint)
strbuf_grow(sb, hint);
}
void strbuf_release(struct strbuf *sb)
{
- free(sb->buf);
- memset(sb, 0, sizeof(*sb));
-}
-
-void strbuf_reset(struct strbuf *sb)
-{
- if (sb->len)
- strbuf_setlen(sb, 0);
+ if (sb->alloc) {
+ free(sb->buf);
+ strbuf_init(sb, 0);
+ }
}
-char *strbuf_detach(struct strbuf *sb)
+char *strbuf_detach(struct strbuf *sb, size_t *sz)
{
- char *res = sb->buf;
+ char *res = sb->alloc ? sb->buf : NULL;
+ if (sz)
+ *sz = sb->len;
strbuf_init(sb, 0);
return res;
}
@@ -40,6 +43,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra)
{
if (sb->len + extra + 1 <= sb->len)
die("you want to use way too much memory");
+ if (!sb->alloc)
+ sb->buf = NULL;
ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
}
diff --git a/strbuf.h b/strbuf.h
index fd68389..a92222b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -10,8 +10,7 @@
* 1. the ->buf member is always malloc-ed, hence strbuf's can be used to
* build complex strings/buffers whose final size isn't easily known.
*
- * It is legal to copy the ->buf pointer away. Though if you want to reuse
- * the strbuf after that, setting ->buf to NULL isn't legal.
+ * It is NOT legal to copy the ->buf pointer away.
* `strbuf_detach' is the operation that detachs a buffer from its shell
* while keeping the shell valid wrt its invariants.
*
@@ -41,19 +40,19 @@
#include <assert.h>
+extern char strbuf_slopbuf[];
struct strbuf {
size_t alloc;
size_t len;
char *buf;
};
-#define STRBUF_INIT { 0, 0, NULL }
+#define STRBUF_INIT { 0, 0, strbuf_slopbuf }
/*----- strbuf life cycle -----*/
extern void strbuf_init(struct strbuf *, size_t);
extern void strbuf_release(struct strbuf *);
-extern void strbuf_reset(struct strbuf *);
-extern char *strbuf_detach(struct strbuf *);
+extern char *strbuf_detach(struct strbuf *, size_t *);
extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);
static inline void strbuf_swap(struct strbuf *a, struct strbuf *b) {
struct strbuf tmp = *a;
@@ -75,6 +74,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
sb->len = len;
sb->buf[len] = '\0';
}
+#define strbuf_reset(sb) strbuf_setlen(sb, 0)
/*----- content related -----*/
extern void strbuf_rtrim(struct strbuf *);
--
1.5.3.2.1100.g015a-dirty
^ permalink raw reply related
* [PATCH 1/2] double free in builtin-update-index.c
From: Pierre Habouzit @ 2007-09-27 10:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20070927101300.GD10289@artemis.corp>
path_name is either ptr that should not be freed, or a pointer to a strbuf
buffer that is deallocated when exiting the loop. Don't do that !
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
builtin-update-index.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/builtin-update-index.c b/builtin-update-index.c
index c76879e..e1a938d 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -377,8 +377,6 @@ static void read_index_info(int line_termination)
die("git-update-index: unable to update %s",
path_name);
}
- if (path_name != ptr)
- free(path_name);
continue;
bad_line:
--
1.5.3.2.1100.g015a-dirty
^ permalink raw reply related
* Re: [PATCH] Add --no-rename to git-apply
From: Johannes Schindelin @ 2007-09-27 10:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Robin Rosenberg, git
In-Reply-To: <7vbqbozo7t.fsf@gitster.siamese.dyndns.org>
Hi,
On Wed, 26 Sep 2007, Junio C Hamano wrote:
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
>
> > With this option git-apply can apply a patch with a rename
> > onto the original file(s).
>
> This is troubling from both design and implementation point of
> view.
>
> * Why would this be useful? What's the point of producing the
> renaming patch if you know you would want to apply while
> ignoring the rename?
Robin said in a follow-up mail that he needs it for a payed-for SCM
(let's describe it as TransparentBox here), which insists on explicit
renames.
But I suggest a simple script here which extracts from the diff the
renames, which outputs a script which renames the file(s) back and then
uses the TransparentBox' mv command:
sed -n -e "/^rename from/N" \
-e "s/^rename from \(.*\)\nrename to \(.*\)/mv \2 \1 \&\& tb mv \1 \2/p" \
< diff.patch
Ciao,
Dscho
^ permalink raw reply
* Re: Use of strbuf.buf when strbuf.len == 0
From: Pierre Habouzit @ 2007-09-27 10:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vir5wy6fv.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]
On Thu, Sep 27, 2007 at 06:21:24AM +0000, Junio C Hamano wrote:
> It might be an easier and safer fix to define that strbuf_init()
> to always have allocation. Use of a strbuf in the code _and_
> not adding any contents to the buffer should be an exception and
> avoiding malloc()/free() for that special case feels optimizing
> for the wrong case.
>
> However, there are strbuf instances that are not initialized
> (i.e. in BSS or initialized by declaring with STRBUF_INIT), so
> we still need to handle (.len == 0 && .alloc == 0) case
> specially anyway.
I can see a way, that would need special proof-reading of the strbuf
module, but should not harm its users, that would be to change
STRBUF_INIT to work this way:
{ .buf = "", .len = 0, .alloc = 0 }
It needs to make strbuf_grow and strbuf_release check for ->alloc
before doing anything stupid.
Though we may have some bits of code that rely on .buf being NULL if
nothing happened. I tried to track them down, but some may remain.
If you agree with this change, that would solve most of the issues
with almost no cost, then I'll propose a new patch with this change.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: Equivalent of `svn switch` for git-svn?
From: Pierre Habouzit @ 2007-09-27 10:06 UTC (permalink / raw)
To: Adam Roben; +Cc: git
In-Reply-To: <46FB5086.7070408@apple.com>
[-- Attachment #1: Type: text/plain, Size: 857 bytes --]
On Thu, Sep 27, 2007 at 06:41:10AM +0000, Adam Roben wrote:
> Hi all-
> I've recently been informed that the Subversion server I and several
> others have been tracking with git-svn will be switching from using the
> svn+ssh scheme to the http scheme. To handle this, users of svn will be
> running `svn switch` to move their working copies to the new repository
> URL. Is there some way to do the same for git-svn? I suspect the biggest
> complication will come from the git-svn-id: lines in the commit logs,
> since changing that line would require changing the commit hash as well.
edit your .git/config, in the section [svn-remote "svn"], change url =
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* git-quiltimport and non-existent patches
From: Geert Uytterhoeven @ 2007-09-27 9:59 UTC (permalink / raw)
To: git; +Cc: Geert Uytterhoeven, Geoff Levand
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1071 bytes --]
Hi,
Unlike quilt itself, git-quiltimport doesn't ignore non-existent patches.
Instead it bails out badly, leaving a .dotest directory that must be removed
manually. This is with git 1.5.3.2.
It would be nice if git-quiltimport would just warn about non-existent patches,
just like quilt. This will make it work with `markers' we put in our quilt
series files. Commenting out the markers is no solution as `quilt series'
doesn't show commented-out patches.
Thanks!
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
^ permalink raw reply
* Re: [PATCH 2/4] This exports the update() function from builtin-add.c as
From: Junio C Hamano @ 2007-09-27 9:05 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <1190868632-29287-2-git-send-email-krh@redhat.com>
Kristian Høgsberg <krh@redhat.com> writes:
> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
Huh? -EPARSE_TITLE_STRING
[PATCH 1/4] Add a simple option parser for use by builtin-commit.c.
[PATCH 2/4] This exports the update() function from builtin-add.c as
[PATCH 3/4] Implement git commit as a builtin command.
[PATCH 4/4] Move launch_editor() and stripspace() to new file editor.c.
Let's step back a bit. The whole series organization is very
screwy. Especially I do not think 4/4 should be at the end.
Reviewing your old series...
* Enable wt-status output to a given FILE pointer.
* Enable wt-status to run against non-standard index file.
Let's have the above two from the previous series in 'next'.
Now the following five that have been in 'pu' are from the older
series:
* Introduce entry point for launching add--interactive.
* Clean up stripspace a bit, use strbuf even more.
* Add strbuf_read_file().
* Export rerere() and launch_editor().
* Implement git commit as a builtin command.
The changes to stripspace and strbuf_read_file() are unrelated
to making the commit command a builtin. I have extended the
strbuf topic with these two and merged the result to 'next'.
I think the right organization for the "builtin-commit" series
should be:
* merge strbuf topic in kh/commit topic, in order to get the
stripspace updates and strbuf_read_file();
* add--interactive entry point change (respin the one from the
old series);
* rename update() to add_files_to_cache() and export (respin
this [2/4] with a better commit message);
* create a separate rerere() function and export (respin part
of old series, with proper refactoring);
I am not happy with builtin-foo.c calling into something from
builtin-bar.c, though. We probably would want to move
rerere() and add_files_to_cache() somewhere else.
* move launch_editor() and stripspace() to create editor.c (new
[4/4]);
* add option parser in parse-options.[ch] (new [1/4]);
* finally, create builtin-commit that uses the groundwork laid
out above (new [3/4]).
I ended up doing the above up to the rerere() one myself, but
haven't done the rest.
I probably would start more aggressively asking the original
author to clean up and resubmit from now on. I haven't managed
to scrape enough time for myself to code anything meaningful for
git recently, and instead spent too much time fixing up other
peoples code.
^ permalink raw reply
* Re: [PATCH] git-commit --amend: respect grafted parents.
From: David Kastrup @ 2007-09-27 8:39 UTC (permalink / raw)
To: git
In-Reply-To: <Pine.LNX.4.64.0709262039250.28395@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi,
>
> On Wed, 26 Sep 2007, Junio C Hamano wrote:
>
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>
>> > Johannes Schindelin schrieb:
>> >> The reason why I insist on not putting this into --amend is that I
>> >> think this is not really an amend, but actively a rewrite of the
>> >> merge commit.
>> >
>> > You have a point here. I'm convinced. Scrap the patch.
>>
>> I am slow today. Since when --amend is not about "a rewrite of commit"?
>
> Technically, you are right, of course. Commit objects are immutable. But
> from a _porcelain_ view I maintain that "amending" is about changes _to_ a
> commit. It is not about redefining the (a) parent.
Well, if you already branched off the commit, the changes "to the
commit" will not register on the branch. So my view is that amending
is about changes to HEAD, not to the HEAD commit. And since branching
is certainly a porcelain operation and is clearly not affected by
amending commits, I think that the "rewrite of a commit" wording
strikes a good balance between "the original commit remains" and "it
is functionally replaced in the HEAD".
--
David Kastrup
^ permalink raw reply
* Re: [PATCH] Create .dotest-merge after validating options.
From: Junio C Hamano @ 2007-09-27 7:29 UTC (permalink / raw)
To: Matt Kraai; +Cc: git
In-Reply-To: <1190770213-8651-1-git-send-email-kraai@ftbfs.org>
Matt Kraai <kraai@ftbfs.org> writes:
> Creating .dotest-merge before validating the options prevents both
> --continue and --interactive from working if the options are invalid,
> so only create it after validating the options.
Thanks. Will apply with a minor fixup. Next time, please make
sure the testsuite passes before submitting.
^ permalink raw reply
* Re: grafts not appearing in manual pages
From: Junio C Hamano @ 2007-09-27 7:25 UTC (permalink / raw)
To: Johannes Sixt; +Cc: J. Bruce Fields, Mike Hommey, git
In-Reply-To: <46FB4C23.8010400@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
> J. Bruce Fields schrieb:
> ...
>> It could go in Documentation/user-manual.txt, but I don't know where.
>
> IMHO grafts should not be made known to a wide audience until
> send-pack, pack-objects, and prune are fixed so that you cannot
> corrupt your repository when there are grafts.
I mildly have to disagree.
Documenting the current semantics (in short, "grafts are
strictly local matter") and the implications is important.
Here are some of the points you would want to mention:
- if you graft, prune and fsck will honor that fake ancestry,
- if you _add_ parent by grafting you will not lose the history
that is otherwise disconnected, but on the other hand, once
having pruned that way and you remove that graft, prune will
discard that discontiguous history away.
- if you _hide_ parent by grafting, you will be able to lose
the hidden subbranch away, but you will get complaints from
fsck if you remove that graft after pruning your history.
- if you try to fetch/push across repositories with different
notion of ancestry (because of different grafts), things can
break in expected ways (and you can keep both halves ;-).
For example, if the sending side has extra parents to a
commit compared to the receiving side, and if the receiving
side claims to have that commit, objects reachable from the
extra parents might be missing from the reciving end but the
sender will not be able to notice.
> See http://thread.gmane.org/gmane.comp.version-control.git/37744
> in particular http://article.gmane.org/gmane.comp.version-control.git/37866
> on a sketch how to fix the issues.
IIRC, there discussions were more about what the issues are and
what the potential semantics could be. First the desired
semantics need to be defined.
^ permalink raw reply
* Re: git-svn and branches
From: Eric Wong @ 2007-09-27 7:24 UTC (permalink / raw)
To: Steven Walter; +Cc: git
In-Reply-To: <20070927021252.GA23777@dervierte>
Steven Walter <stevenrwalter@gmail.com> wrote:
> I'm using git-svn to track a rather large subversion repository with a
> non-standard layout. In the past, I've only cared about trunk, but now
> I need to occasionally use branches, too. By adding a second git-svn
> remote with the branch URL, I can fetch the branch, and git-svn is even
> intelligent enough to notice that the branch was copied off of trunk.
>
> However, git-svn also does a complete checkout for the first revision of
> the branch. By this, I mean it goes through shows "A file" for every
> file in the repository. This takes quite a while, and seems rather
> unnecessary given that git-svn already noticed that the branch shares a
> history with trunk, which is already fetched.
>
> Knowing just enough of what git-svn is doing to be dangerous, I whipped
> up a short little patch. This patch seems to work for the common case,
> and avoids fetching every file from subversion. It does break
> sometimes, however, and I don't understand why.
>
> Maybe someone with a better grasps of the code can see what I did wrong,
> or suggest a better means to my end?
I believe your case handles where a branch is created directly from a
trunk copy with no file modifications in the branch, but not when a
branch is created and files are modified in the trunk (or branch) within
the same revision. Is this what's happening?
Additionally, I think this breaks when an entire trunk or branch is
moved around because the original directory has moved or gone away:
/trunk => /project-a/trunk
Anyways, as Sam said, newer SVN (1.4.4+) has a working do_switch()
function and that code path will never be hit at all.
> diff --git a/git-svn.perl b/git-svn.perl
> index 484b057..1bc92b6 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1848,9 +1848,10 @@ sub find_parent_branch {
> $self->full_url, $ed)
> or die "SVN connection failed somewhere...\n";
> } else {
> + $self->assert_index_clean($parent);
> print STDERR "Following parent with do_update\n";
> $ed = SVN::Git::Fetcher->new($self);
> - $self->ra->gs_do_update($rev, $rev, $self, $ed)
> + $self->ra->gs_do_update($rev, $r0, $self, $ed)
> or die "SVN connection failed somewhere...\n";
> }
> print STDERR "Successfully followed parent\n";
--
Eric Wong
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: David Kastrup @ 2007-09-27 6:43 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <854phgfxn7.fsf@lola.goethe.zz>
David Kastrup <dak@gnu.org> writes:
> Jeff King <peff@peff.net> writes:
>
>> On Wed, Sep 26, 2007 at 01:05:59PM -0700, Junio C Hamano wrote:
>>
>>> * jk/diff-rename (Tue Sep 25 15:29:42 2007 -0400) 1 commit
>>> + diffcore-rename: cache file deltas
>>>
>>> Parked in 'next' for now but is 'master' material.
>>
>> My tests after this patch show that spanhash_find is responsible for
>> a large portion of the processing time in large renames, so I am going
>> to look into speeding that up.
>
> In itself, it does not look like there is all too much room for
> optimization. One can remove the temporary pointer "optimization" and
> see whether this makes strength reduction possible for the compiler.
> Making this an endless loop wrapped around a loop on bucket might also
> help the compiler in that effect.
>
> But there is really not all too much leeway, and it might be better
> spent in the caller. For example, the search will take something like
> r/(1-r) iterations on average where r is the fill ratio of the hash
> array. So one would not want to, say, let r grow above 0.75 or
> something like that.
Ok, here is some suggestion:
Here is the inner loop for this stuff:
for (i = 0; i < ssz; i++) {
struct spanhash *s = &(src_count->data[i]);
struct spanhash *d;
unsigned dst_cnt, src_cnt;
if (!s->cnt)
continue;
src_cnt = s->cnt;
d = spanhash_find(dst_count, s->hashval);
dst_cnt = d ? d->cnt : 0;
if (src_cnt < dst_cnt) {
la += dst_cnt - src_cnt;
sc += src_cnt;
}
else
sc += dst_cnt;
}
Now here is how one could optimize the data structures: The hash
structures are with linear probing, and we try to find any hash
matches from source to destination. If we sort all hashes indexed to
a given first hash bucket by their full hash value, then one could
basically use passes similar to list merges for figuring the 1:1
relations. That cuts down the O(l n) cost (where n is the number of
elements and l their average run length) to O(n).
Of course, making l close to 1 by keeping the hash utilization
reasonably low is much simpler.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Equivalent of `svn switch` for git-svn?
From: Adam Roben @ 2007-09-27 6:41 UTC (permalink / raw)
To: git
Hi all-
I've recently been informed that the Subversion server I and several
others have been tracking with git-svn will be switching from using the
svn+ssh scheme to the http scheme. To handle this, users of svn will be
running `svn switch` to move their working copies to the new repository
URL. Is there some way to do the same for git-svn? I suspect the biggest
complication will come from the git-svn-id: lines in the commit logs,
since changing that line would require changing the commit hash as well.
Thanks for any advice!
-Adam
^ permalink raw reply
* Re: git-svn and branches
From: Sam Vilain @ 2007-09-27 6:36 UTC (permalink / raw)
To: Steven Walter; +Cc: git, normalperson
In-Reply-To: <20070927021252.GA23777@dervierte>
Steven Walter wrote:
> Knowing just enough of what git-svn is doing to be dangerous, I
> whipped up a short little patch. This patch seems to work for the
> common case, and avoids fetching every file from subversion. It does
> break sometimes, however, and I don't understand why.
>
> Maybe someone with a better grasps of the code can see what I did
> wrong, or suggest a better means to my end?
Try also with the SVN trunk - the do_switch API has recently been added
and it also avoids this excess checkout. I'm not sure why a solution
like you post isn't used, perhaps Eric can comment further.
Sam.
> diff --git a/git-svn.perl b/git-svn.perl
> index 484b057..1bc92b6 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1848,9 +1848,10 @@ sub find_parent_branch {
> $self->full_url, $ed)
> or die "SVN connection failed somewhere...\n";
> } else {
> + $self->assert_index_clean($parent);
> print STDERR "Following parent with do_update\n";
> $ed = SVN::Git::Fetcher->new($self);
> - $self->ra->gs_do_update($rev, $rev, $self, $ed)
> + $self->ra->gs_do_update($rev, $r0, $self, $ed)
> or die "SVN connection failed somewhere...\n";
> }
> print STDERR "Successfully followed parent\n";
^ permalink raw reply
* Re: grafts not appearing in manual pages
From: Johannes Sixt @ 2007-09-27 6:22 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Mike Hommey, git
In-Reply-To: <20070926210102.GF26099@fieldses.org>
J. Bruce Fields schrieb:
> On Wed, Sep 26, 2007 at 10:24:41PM +0200, Mike Hommey wrote:
>> Hi,
>>
>> The only occurrence of grafts in the manual pages is in the
>> git-filter-branch one. I somehow feel this is wrong not to see it
>> described more "formally" in the manual pages.
>>
>> I wouldn't mind writing a small something, except I have no idea what
>> would be the most appropriate place to talk about it... Does anyone have
>> such an idea ?
>
> It could go in Documentation/user-manual.txt, but I don't know where.
IMHO grafts should not be made known to a wide audience until send-pack,
pack-objects, and prune are fixed so that you cannot corrupt your repository
when there are grafts.
See http://thread.gmane.org/gmane.comp.version-control.git/37744
in particular http://article.gmane.org/gmane.comp.version-control.git/37866
on a sketch how to fix the issues.
-- Hannes
^ permalink raw reply
* Use of strbuf.buf when strbuf.len == 0
From: Junio C Hamano @ 2007-09-27 6:21 UTC (permalink / raw)
To: git
I am starting to hate some parts of the strbuf API.
Here is an example. Can you spot what goes wrong?
static int handle_file(const char *path,
unsigned char *sha1, const char *output)
{
SHA_CTX ctx;
char buf[1024];
int hunk = 0, hunk_no = 0;
struct strbuf one, two;
...
if (sha1)
SHA1_Init(&ctx);
strbuf_init(&one, 0);
strbuf_init(&two, 0);
while (fgets(buf, sizeof(buf), f)) {
if (!prefixcmp(buf, "<<<<<<< "))
hunk = 1;
else if (!prefixcmp(buf, "======="))
hunk = 2;
else if (!prefixcmp(buf, ">>>>>>> ")) {
int cmp = strbuf_cmp(&one, &two);
hunk_no++;
hunk = 0;
if (cmp > 0) {
strbuf_swap(&one, &two);
}
if (out) {
fputs("<<<<<<<\n", out);
fwrite(one.buf, one.len, 1, out);
fputs("=======\n", out);
fwrite(two.buf, two.len, 1, out);
fputs(">>>>>>>\n", out);
}
if (sha1) {
SHA1_Update(&ctx, one.buf, one.len + 1);
SHA1_Update(&ctx, two.buf, two.len + 1);
}
strbuf_reset(&one);
strbuf_reset(&two);
} else if (hunk == 1)
strbuf_addstr(&one, buf);
else if (hunk == 2)
strbuf_addstr(&two, buf);
else if (out)
fputs(buf, out);
}
strbuf_release(&one);
strbuf_release(&two);
...
}
When one or two are empty and the caller asked for checksumming,
the code still relies on one.buf being an allocated memory with
an extra NUL termination and tries to feed the NULL pointer to
SHA1_Update() with length of 1!
An obvious workaround is to say "strbuf_init(&one, !!sha1)" to
force the allocation. However, because the second parameter to
strbuf_init() is defined to be merely a hint, we should not rely
on strbuf_init() with non-zero hint to have allocation from the
beginning. It is assuming too much.
A more defensive way would be for the user to do something like:
SHA1_Update(&ctx, one.buf ? one.buf : "", one.len + 1);
SHA1_Update(&ctx, two.buf ? two.buf : "", two.len + 1);
which I am leaning towards, but this looks ugly.
I was already bitten by another bug in strbuf_setlen() that
shared the same roots; an empty strbuf can have two internal
representations ("alloc == 0 && buf == NULL" vs "alloc != 0 &&
buf != NULL") and they behave differently.
For example, this happens to be Ok but the validity of it is
subtle. If a or b is empty, memcmp may get a NULL pointer but
we ask only 0 byte comparison.
int strbuf_cmp(struct strbuf *a, struct strbuf *b)
{
int cmp;
if (a->len < b->len) {
cmp = memcmp(a->buf, b->buf, a->len);
return cmp ? cmp : -1;
} else {
cmp = memcmp(a->buf, b->buf, b->len);
return cmp ? cmp : a->len != b->len;
}
}
It might be an easier and safer fix to define that strbuf_init()
to always have allocation. Use of a strbuf in the code _and_
not adding any contents to the buffer should be an exception and
avoiding malloc()/free() for that special case feels optimizing
for the wrong case.
However, there are strbuf instances that are not initialized
(i.e. in BSS or initialized by declaring with STRBUF_INIT), so
we still need to handle (.len == 0 && .alloc == 0) case
specially anyway.
It would be appreciated if somebody with a fresh pair of eyes
can go over the strbuf series one more time to make sure that we
do not try to blindly use buf.buf, assuming buf.buf[0] is NUL if
(buf.len == 0).
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: David Kastrup @ 2007-09-27 6:08 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20070927023633.GA28902@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Sep 26, 2007 at 01:05:59PM -0700, Junio C Hamano wrote:
>
>> * jk/diff-rename (Tue Sep 25 15:29:42 2007 -0400) 1 commit
>> + diffcore-rename: cache file deltas
>>
>> Parked in 'next' for now but is 'master' material.
>
> My tests after this patch show that spanhash_find is responsible for
> a large portion of the processing time in large renames, so I am going
> to look into speeding that up.
In itself, it does not look like there is all too much room for
optimization. One can remove the temporary pointer "optimization" and
see whether this makes strength reduction possible for the compiler.
Making this an endless loop wrapped around a loop on bucket might also
help the compiler in that effect.
But there is really not all too much leeway, and it might be better
spent in the caller. For example, the search will take something like
r/(1-r) iterations on average where r is the fill ratio of the hash
array. So one would not want to, say, let r grow above 0.75 or
something like that.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* [StGit PATCH] Let "stg assimilate" handle missing patches
From: Karl Hasselström @ 2007-09-27 5:50 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git
In-Reply-To: <20070927054833.GA12207@diana.vm.bytemark.co.uk>
If a patch was not mentioned in the applied/unapplied files,
"assimilate" used to ignore it. Now it won't. The only information
loss ensuing from this sequence of commands
$ rm .git/patches/<branch>/{un,}applied
$ touch .git/patches/<branch>/{un,}applied
$ stg assimilate
is now the order of the unapplied patches.
Signed-off-by: Karl Hasselström <kha@treskal.com>
---
stgit/commands/assimilate.py | 32 ++++++++++++++++++++------------
1 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/stgit/commands/assimilate.py b/stgit/commands/assimilate.py
index ab2264a..43672fd 100644
--- a/stgit/commands/assimilate.py
+++ b/stgit/commands/assimilate.py
@@ -90,7 +90,7 @@ def read_commit_dag(branch):
for line in Run('git-show-ref').output_lines():
id, ref = line.split()
m = re.match(r'^refs/patches/%s/(.+)$' % branch, ref)
- if m:
+ if m and not m.group(1).endswith('.log'):
c = commits[id]
c.patch = m.group(1)
patches.add(c)
@@ -173,17 +173,25 @@ def func(parser, options, args):
# Write the applied/unapplied files.
out.start('Checking patch appliedness')
+ unapplied = patches - set(applied)
applied_name_set = set(p.patch for p in applied)
- unapplied_names = []
- for name in orig_applied:
- if not name in applied_name_set:
- out.info('%s is now unapplied' % name)
- unapplied_names.append(name)
- for name in orig_unapplied:
- if name in applied_name_set:
- out.info('%s is now applied' % name)
- else:
- unapplied_names.append(name)
+ unapplied_name_set = set(p.patch for p in unapplied)
+ patches_name_set = set(p.patch for p in patches)
+ orig_patches = orig_applied + orig_unapplied
+ orig_applied_name_set = set(orig_applied)
+ orig_unapplied_name_set = set(orig_unapplied)
+ orig_patches_name_set = set(orig_patches)
+ for name in orig_patches_name_set - patches_name_set:
+ out.info('%s is gone' % name)
+ for name in applied_name_set - orig_applied_name_set:
+ out.info('%s is now applied' % name)
+ for name in unapplied_name_set - orig_unapplied_name_set:
+ out.info('%s is now unapplied' % name)
+ orig_order = dict(zip(orig_patches, xrange(len(orig_patches))))
+ def patchname_cmp(p1, p2):
+ i1 = orig_order.get(p1, len(orig_order))
+ i2 = orig_order.get(p2, len(orig_order))
+ return cmp((i1, p1), (i2, p2))
crt_series.set_applied(p.patch for p in applied)
- crt_series.set_unapplied(unapplied_names)
+ crt_series.set_unapplied(sorted(unapplied_name_set, cmp = patchname_cmp))
out.done()
^ permalink raw reply related
* Re: [StGit PATCH 0/2] "stg assimilate" on steroids
From: Karl Hasselström @ 2007-09-27 5:48 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git
In-Reply-To: <20070926020911.1202.2580.stgit@yoghurt>
On 2007-09-26 04:15:02 +0200, Karl Hasselström wrote:
> If you have been using that, manually change the stack format
> version back to 2, and create empty "applied" and "unapplied" files,
> and run the new assimilate. That should fix it. I hope.
Not quite. You need this fix first.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: History over-simplification
From: Junio C Hamano @ 2007-09-27 5:34 UTC (permalink / raw)
To: linux; +Cc: git, spearce
In-Reply-To: <20070927045640.31040.qmail@science.horizon.com>
linux@horizon.com writes:
> These seem like roundabout ways of expressing the requirement.
> The desired rule is "no silent changes in the simplified history".
> E.g. if the revision history of a particular file is:
>
> +-C-+
> / \
> A---B B---D
> \ /
> +-B-+
>
> Then the normal code will notice that there are no changes on the
> lower branch, prune the merge entirely, and simplify to A---B---C---D.
> This is, however, misleading; the true history is A---B---C---B---D.
Do you denote the same content with the same letter in the
above?
If so, wouldn't the simplified history be just A-B-D?
^ permalink raw reply
* Re: History over-simplification
From: linux @ 2007-09-27 4:56 UTC (permalink / raw)
To: git, spearce; +Cc: gitster, linux
>> b) The resulting path matches one of the parents but not one of
>> the others. In such a case the merge should still be output if
>> a 3-way read-tree would not have chosen this result by default.
> I am not sure (b) is useful in general. Merging two branches
> that fix the same issue but in different ways (think: 'maint'
> and 'master' have different infrastructure and a fix initially
> made on 'master' was backported to 'maint', and then later
> 'maint' needed to be merged to 'master' to carry forward other
> fixes) is a norm, and in such cases taking the version from the
> merged-to branch is almost always what happens.
>
> Also it sounds to me by "if read-tree would not have chosen this
> result by default" you mean this feature would not just need to
> run merge-base but also recursive merge-base synthesis, and also
> recreate the structural merge (aka "rename detection") there as
> well. Even if (b) is useful, it sounds like a very expensive
> option, and the current merge-recursive code is structured in
> such a way to be easily reused for this purpose.
These seem like roundabout ways of expressing the requirement.
The desired rule is "no silent changes in the simplified history".
E.g. if the revision history of a particular file is:
+-C-+
/ \
A---B B---D
\ /
+-B-+
Then the normal code will notice that there are no changes on the
lower branch, prune the merge entirely, and simplify to A---B---C---D.
This is, however, misleading; the true history is A---B---C---B---D.
A merge must be shown unless it matches a *non-eliminated* ancestor.
The point is that this isn't expressed in terms of what merge-base would
do, but in terms of what the path limiter is in the process of doing.
Now, I haven't dived into the Deep Magic of revision.c to figure out
where to put this into the code, unfortunately.
Another way to say it is that the desired simplified history
is achieved when you have exhausted the following two rules:
- You may delete any revision which has only one ancestor and
is identical (after path-limiting) to that ancestor.
(Descendants are implicitly linked to that ancestor.)
- You may delete any ancestor link A---B if there is an
alternative directed path between A and B.
(Fast-forward rule.)
Given unlabeled ancestor links, there is a unique fixed point.
The current code is willing to delete a revision that is
identical to *any* ancestor, even deleted ones.
^ 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