* Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"
From: Junio C Hamano @ 2007-09-06 23:17 UTC (permalink / raw)
To: René Scharfe
Cc: Johannes Schindelin, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <46E068BB.5080202@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> Just noticed: if the memcmp() above finds a difference, the code should
> *not* break out of the loop. Ahem. Perhaps I should first add memmem()
> after all...
That sounds like a sensible thing to do.
Will drop this copy and use updated 4/3 and 3.5/3; thanks.
^ permalink raw reply
* Subject: [PATCH] git-merge-pack
From: Junio C Hamano @ 2007-09-06 23:12 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Nicolas Pitre, Nix, Steven Grimm,
Git Mailing List
In-Reply-To: <alpine.LFD.0.999.0709061906010.5626@evo.linux-foundation.org>
This is a beginning of "git-merge-pack" that combines smaller
packs into one. Currently it does not actually create a new
pack, but pretends that it is a (dumb) "git-rev-list --objects"
that lists the objects in the affected packs. You have to pipe
its output to "git-pack-objects".
The command reads names of pack-*.pack files from the standard
input, outputs the objects' names in the order they are stored
in the original packs (i.e. the offset order). This sorting is
done in order to emulate the traversal order the original
"git-rev-list --objects" that was used to create the existing
pack listed the objects.
While this approach would give the resulting packfile very
similar locality of access as the original, it does not give the
"name" component you would see in "git-rev-list --objects"
output. This information is used as the clustering cue while
computing delta, and the lack of it means you can get horrible
delta selection. You do _not_ want to run the downstream
"git-pack-objects" without the optimization/heuristics to reuse
delta. IOW, do not run it with --no-reuse-delta.
To consolidate all packs that are smaller than a megabytes into
one, you would use it in its current form like this:
$ old=$(find .git/objects/pack -type f -name '*.pack' -size 1M)
$ new=$(echo "$old" | git merge-pack | git pack-objects pack)
$ for p in $old; do rm -f $p ${p%.pack}.idx; done
$ for s in pack idx; do mv pack-$new.$s .git/objects/pack/; done
An obvious next steps that can be done in parallel by interested
parties would be:
(1) come up with a way to give "name" aka "clustering cue" (I
think this is very hard);
(2) run the above four command sequence internally without
having to resort to shell wrapper (easy).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Linus Torvalds <torvalds@linux-foundation.org> writes:
> IOW, if you get lots of small incrmental packs, after a while you really
> *do* need to do "git gc" to get the real pack generated.
'auto' should do a lessor impact repack than the usual one.
Especially we do not want to lose objects that do not look like
they are reachable from this reopsitory, to help people with
alternate object stores, aka "repo.or.cz style _forked_
repositories". However, a full repack with "-a -d" discards
unreferenced objects that are only in packs.
We need a middle ground between "pack and prune-pack only loose
ones" and "full repack.
Here is one.
Makefile | 1 +
builtin-merge-pack.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++
builtin.h | 1 +
git.c | 1 +
4 files changed, 90 insertions(+), 0 deletions(-)
create mode 100644 builtin-merge-pack.c
diff --git a/Makefile b/Makefile
index dace211..cdff756 100644
--- a/Makefile
+++ b/Makefile
@@ -343,6 +343,7 @@ BUILTIN_OBJS = \
builtin-mailsplit.o \
builtin-merge-base.o \
builtin-merge-file.o \
+ builtin-merge-pack.o \
builtin-mv.o \
builtin-name-rev.o \
builtin-pack-objects.o \
diff --git a/builtin-merge-pack.c b/builtin-merge-pack.c
new file mode 100644
index 0000000..c98da80
--- /dev/null
+++ b/builtin-merge-pack.c
@@ -0,0 +1,87 @@
+#include "builtin.h"
+#include "cache.h"
+#include "pack.h"
+
+struct in_pack_object {
+ off_t offset;
+ const unsigned char *sha1;
+};
+
+static uint32_t get_packed_object_list(struct packed_git *p, struct in_pack_object *list, uint32_t loc)
+{
+ uint32_t n;
+
+ for (n = 0; n < p->num_objects; n++) {
+ list[loc].sha1 = nth_packed_object_sha1(p, n);
+ list[loc].offset = find_pack_entry_one(list[loc].sha1, p);
+ loc++;
+ }
+ return loc;
+}
+
+static int ofscmp(const void *a_, const void *b_)
+{
+ struct in_pack_object *a = (struct in_pack_object *)a_;
+ struct in_pack_object *b = (struct in_pack_object *)b_;
+ if (a->offset < b->offset)
+ return -1;
+ else if (a->offset > b->offset)
+ return 1;
+ else
+ return hashcmp(a->sha1, b->sha1);
+}
+
+int cmd_merge_pack(int ac, const char **av, const char *prefix)
+{
+ char filename[PATH_MAX];
+ struct packed_git **pack = NULL;
+ int pack_nr = 0;
+ int pack_alloc = 0;
+ uint32_t max_objs, cnt;
+ struct in_pack_object *objs;
+ int i;
+
+ while (fgets(filename, sizeof(filename), stdin) != NULL) {
+ int len = strlen(filename);
+ struct packed_git *p;
+
+ while (0 < len) {
+ if (filename[len-1] != '\n' &&
+ filename[len-1] != '\r')
+ break;
+ filename[--len] = '\0';
+ }
+ if (strcmp(filename + len - 5, ".pack"))
+ goto error;
+
+ /* add-packed-git wants the name of .idx file */
+ strcpy(filename + len - 5, ".idx");
+ len--;
+ p = add_packed_git(filename, len, 1);
+ if (!p)
+ goto error;
+ if (open_pack_index(p))
+ goto error;
+
+ if (pack_alloc <= pack_nr) {
+ pack_alloc = alloc_nr(pack_nr);
+ pack = xrealloc(pack, pack_alloc * sizeof(*pack));
+ }
+ pack[pack_nr++] = p;
+ continue;
+ error:
+ die("Cannot add a pack .idx file: %s", filename);
+ }
+
+ max_objs = 0;
+ for (i = 0; i < pack_nr; i++)
+ max_objs += pack[i]->num_objects;
+ objs = xmalloc(sizeof(*objs) * max_objs);
+ cnt = 0;
+ for (i = 0; i < pack_nr; i++)
+ cnt = get_packed_object_list(pack[i], objs, cnt);
+ qsort(objs, cnt, sizeof(*objs), ofscmp);
+ for (cnt = 0; cnt < max_objs; cnt++)
+ printf("%s\n", sha1_to_hex(objs[cnt].sha1));
+ return 0;
+}
diff --git a/builtin.h b/builtin.h
index bb72000..aff28ca 100644
--- a/builtin.h
+++ b/builtin.h
@@ -49,6 +49,7 @@ extern int cmd_mailinfo(int argc, const char **argv, const char *prefix);
extern int cmd_mailsplit(int argc, const char **argv, const char *prefix);
extern int cmd_merge_base(int argc, const char **argv, const char *prefix);
extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
+extern int cmd_merge_pack(int argc, const char **argv, const char *prefix);
extern int cmd_mv(int argc, const char **argv, const char *prefix);
extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index fd3d83c..69e86bc 100644
--- a/git.c
+++ b/git.c
@@ -353,6 +353,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "mailsplit", cmd_mailsplit },
{ "merge-base", cmd_merge_base, RUN_SETUP },
{ "merge-file", cmd_merge_file },
+ { "merge-pack", cmd_merge_pack },
{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
{ "name-rev", cmd_name_rev, RUN_SETUP },
{ "pack-objects", cmd_pack_objects, RUN_SETUP },
--
1.5.3.1.860.g2cce2
^ permalink raw reply related
* Re: [PATCH] git.__remotes_from_dir() should only return lists
From: Karl Hasselström @ 2007-09-06 23:11 UTC (permalink / raw)
To: Pavel Roskin; +Cc: Catalin Marinas, git
In-Reply-To: <1189082306.3695.5.camel@gx>
On 2007-09-06 08:38:26 -0400, Pavel Roskin wrote:
> On Thu, 2007-09-06 at 13:26 +0200, Karl Hasselström wrote:
>
> > Hmm. I don't believe I saw t1001 break without this patch (I run
> > the test suite before I push, but I might have made a mistake of
> > course). Does the user's environment leak into the test sandbox?
>
> I don't think it's the user environment, at least on my side. I'm
> using Fedora 7, which has python-2.5-12.fc7. That's the error from
> the t1001 before my patch:
Mmm, irritating. I really don't get the error, and debug printouts
confirm that it's because the directories .git/remotes and
.git/branches both exist.
Your patch is the right thing to do anyway, obviously.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: Help with git
From: Alex Riesen @ 2007-09-06 23:07 UTC (permalink / raw)
To: Jameson Chema Quinn; +Cc: devel, git
In-Reply-To: <faf2c12b0709042250s4f6a5a09o4299dbe06d6ce2be@mail.gmail.com>
Jameson Chema Quinn, Wed, Sep 05, 2007 07:50:25 +0200:
> I am having some problems with git - essentially, my .gitignore and
> core/editor config options are not working.
>
> If you can help, THANK YOU. Please respond to this problem on
> http://wiki.laptop.org/go/Talk:Git to avoid clogging up the list.
>
I answered on the page, but the one with .gitignore bothers me a bit:
this is an old piece of code, and a quite reliable one. Why did you
put .gitignore file?
^ permalink raw reply
* Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
From: René Scharfe @ 2007-09-06 22:54 UTC (permalink / raw)
To: Kristian Høgsberg, git
In-Reply-To: <20070906182746.GK8451@artemis.corp>
Pierre Habouzit schrieb:
> On Thu, Sep 06, 2007 at 06:18:34PM +0000, Kristian Høgsberg wrote:
>> On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:
>>> On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
>>>> On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
>>>>> - memcpy(path.buf, base, baselen);
>>>>> - memcpy(path.buf + baselen, filename, filenamelen);
>>>>> - path.len = baselen + filenamelen;
>>>>> - path.buf[path.len] = '\0';
>>>>> + strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
>>>>> + strbuf_reset(&path);
>>>> Does strbuf_reset() do anything here?
>>>>
>>>>> + strbuf_add(&path, base, baselen);
>>> Yes _reset() sets length to 0. so the add here will write at the start
>>> of the buffer again. It definitely is important !
>> But where was length set to non-zero? path is initialized on entry to
>> the function, and strbuf_grow() just increases the allocation, not
>> length, right?
>
> The path is static, hence when you reenter the function you have the
> last value in it. The fact that it's static may be questionable, but it
> was like it before, I kept it, I've supposed it was for performance
> reasons.
You're right, I've made it static to get the number of mallocs from
number-of-files-in-archive down to one. It was a ~10% speedup in the
warm cache case, so I could not resist. :)
René
^ permalink raw reply
* Re: rebase from ambiguous ref discards changes
From: Pierre Habouzit @ 2007-09-06 22:37 UTC (permalink / raw)
To: Keith Packard; +Cc: Git Mailing List
In-Reply-To: <1189115308.30308.9.camel@koto.keithp.com>
[-- Attachment #1: Type: text/plain, Size: 622 bytes --]
On Thu, Sep 06, 2007 at 09:48:28PM +0000, Keith Packard wrote:
> recovering my patch (having the ID in my terminal window from the
> commit), I named it 'master-with-fix'
Note that your patch was not lost, even if you had pruned, because it
would still be accessible from the reflog of your master branch. Not
that it's supposed to soften your disappointment but well, at least it
should make you a bit less uncomfortable, maybe :)
--
·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 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR" (take 2)
From: René Scharfe @ 2007-09-06 22:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <46E028B9.2090908@lsrfire.ath.cx>
As suggested by Johannes, --pretty=format: placeholders in specfiles
need to be wrapped in $Format:...$ now. This syntax change restricts
the expansion of placeholders and makes it easier to use with files
that contain non-placeholder percent signs.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
This replacement patch fixes a bug in the "$Format:" search logic. It
now uses memmem() and thus depends on patch 3.5 which introduces this
function. Patch 5 still applies unchanged.
Documentation/gitattributes.txt | 5 +++-
builtin-archive.c | 52 +++++++++++++++++++++++++++++++++++---
t/t5000-tar-tree.sh | 4 +-
3 files changed, 53 insertions(+), 8 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 47a621b..37b3be8 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -432,7 +432,10 @@ several placeholders when adding this file to an archive. The
expansion depends on the availability of a commit ID, i.e. if
gitlink:git-archive[1] has been given a tree instead of a commit or a
tag then no replacement will be done. The placeholders are the same
-as those for the option `--pretty=format:` of gitlink:git-log[1].
+as those for the option `--pretty=format:` of gitlink:git-log[1],
+except that they need to be wrapped like this: `$Format:PLACEHOLDERS$`
+in the file. E.g. the string `$Format:%H$` will be replaced by the
+commit hash.
GIT
diff --git a/builtin-archive.c b/builtin-archive.c
index faccce3..65bf9cb 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -81,14 +81,58 @@ static int run_remote_archiver(const char *remote, int argc,
return !!rv;
}
+static void *format_specfile(const struct commit *commit, const char *format,
+ unsigned long *sizep)
+{
+ unsigned long len = *sizep, result_len = 0;
+ const char *a = format;
+ char *result = NULL;
+
+ for (;;) {
+ const char *b, *c;
+ char *fmt, *formatted = NULL;
+ unsigned long a_len, fmt_len, formatted_len, allocated = 0;
+
+ b = memmem(a, len, "$Format:", 8);
+ if (!b || a + len < b + 9)
+ break;
+ c = memchr(b + 8, '$', len - 8);
+ if (!c)
+ break;
+
+ a_len = b - a;
+ fmt_len = c - b - 8;
+ fmt = xmalloc(fmt_len + 1);
+ memcpy(fmt, b + 8, fmt_len);
+ fmt[fmt_len] = '\0';
+
+ formatted_len = format_commit_message(commit, fmt, &formatted,
+ &allocated);
+ result = xrealloc(result, result_len + a_len + formatted_len);
+ memcpy(result + result_len, a, a_len);
+ memcpy(result + result_len + a_len, formatted, formatted_len);
+ result_len += a_len + formatted_len;
+ len -= c + 1 - a;
+ a = c + 1;
+ }
+
+ if (result && len) {
+ result = xrealloc(result, result_len + len);
+ memcpy(result + result_len, a, len);
+ result_len += len;
+ }
+
+ *sizep = result_len;
+
+ return result;
+}
+
static void *convert_to_archive(const char *path,
const void *src, unsigned long *sizep,
const struct commit *commit)
{
static struct git_attr *attr_specfile;
struct git_attr_check check[1];
- char *interpolated = NULL;
- unsigned long allocated = 0;
if (!commit)
return NULL;
@@ -102,9 +146,7 @@ static void *convert_to_archive(const char *path,
if (!ATTR_TRUE(check[0].value))
return NULL;
- *sizep = format_commit_message(commit, src, &interpolated, &allocated);
-
- return interpolated;
+ return format_specfile(commit, src, sizep);
}
void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 3d5d01b..6e89e07 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -36,7 +36,7 @@ test_expect_success \
echo simple textfile >a/a &&
mkdir a/bin &&
cp /bin/sh a/bin &&
- printf "%s" "$SPECFILEFORMAT" >a/specfile &&
+ printf "A\$Format:%s\$O" "$SPECFILEFORMAT" >a/specfile &&
ln -s a a/l1 &&
(p=long_path_to_a_file && cd a &&
for depth in 1 2 3 4 5; do mkdir $p && cd $p; done &&
@@ -119,7 +119,7 @@ test_expect_success \
test_expect_success \
'validate specfile contents' \
- 'git log --max-count=1 "--pretty=format:$SPECFILEFORMAT" HEAD \
+ 'git log --max-count=1 "--pretty=format:A${SPECFILEFORMAT}O" HEAD \
>f/a/specfile.expected &&
diff f/a/specfile.expected f/a/specfile'
--
1.5.3
^ permalink raw reply related
* [PATCH 3.5/3] add memmem()
From: René Scharfe @ 2007-09-06 22:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <46E028B9.2090908@lsrfire.ath.cx>
memmem() is a nice GNU extension for searching a length limited string
in another one.
This compat version is based on the version found in glibc 2.2 (GPL 2);
I only removed the optimization of checking the first char by hand, and
generally tried to keep the code simple. We can add it back if memcmp
shows up high in a profile, but for now I prefer to keep it (almost
trivially) simple.
Since I don't really know which platforms beside those with a glibc
have their own memmem(), I used a heuristic: if NO_STRCASESTR is set,
then NO_MEMMEM is set, too.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Makefile | 11 +++++++++++
compat/memmem.c | 29 +++++++++++++++++++++++++++++
git-compat-util.h | 6 ++++++
3 files changed, 46 insertions(+), 0 deletions(-)
create mode 100644 compat/memmem.c
diff --git a/Makefile b/Makefile
index 51af531..bae073f 100644
--- a/Makefile
+++ b/Makefile
@@ -28,6 +28,8 @@ all::
#
# Define NO_STRCASESTR if you don't have strcasestr.
#
+# Define NO_MEMMEM if you don't have memmem.
+#
# Define NO_STRLCPY if you don't have strlcpy.
#
# Define NO_STRTOUMAX if you don't have strtoumax in the C library.
@@ -402,6 +404,7 @@ ifeq ($(uname_S),SunOS)
NEEDS_NSL = YesPlease
SHELL_PATH = /bin/bash
NO_STRCASESTR = YesPlease
+ NO_MEMMEM = YesPlease
NO_HSTRERROR = YesPlease
ifeq ($(uname_R),5.8)
NEEDS_LIBICONV = YesPlease
@@ -424,6 +427,7 @@ ifeq ($(uname_O),Cygwin)
NO_D_TYPE_IN_DIRENT = YesPlease
NO_D_INO_IN_DIRENT = YesPlease
NO_STRCASESTR = YesPlease
+ NO_MEMMEM = YesPlease
NO_SYMLINK_HEAD = YesPlease
NEEDS_LIBICONV = YesPlease
NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
@@ -442,6 +446,7 @@ ifeq ($(uname_S),FreeBSD)
endif
ifeq ($(uname_S),OpenBSD)
NO_STRCASESTR = YesPlease
+ NO_MEMMEM = YesPlease
NEEDS_LIBICONV = YesPlease
BASIC_CFLAGS += -I/usr/local/include
BASIC_LDFLAGS += -L/usr/local/lib
@@ -456,6 +461,7 @@ ifeq ($(uname_S),NetBSD)
endif
ifeq ($(uname_S),AIX)
NO_STRCASESTR=YesPlease
+ NO_MEMMEM = YesPlease
NO_STRLCPY = YesPlease
NEEDS_LIBICONV=YesPlease
endif
@@ -467,6 +473,7 @@ ifeq ($(uname_S),IRIX64)
NO_IPV6=YesPlease
NO_SETENV=YesPlease
NO_STRCASESTR=YesPlease
+ NO_MEMMEM = YesPlease
NO_STRLCPY = YesPlease
NO_SOCKADDR_STORAGE=YesPlease
SHELL_PATH=/usr/gnu/bin/bash
@@ -661,6 +668,10 @@ ifdef NO_HSTRERROR
COMPAT_CFLAGS += -DNO_HSTRERROR
COMPAT_OBJS += compat/hstrerror.o
endif
+ifdef NO_MEMMEM
+ COMPAT_CFLAGS += -DNO_MEMMEM
+ COMPAT_OBJS += compat/memmem.o
+endif
ifeq ($(TCLTK_PATH),)
NO_TCLTK=NoThanks
diff --git a/compat/memmem.c b/compat/memmem.c
new file mode 100644
index 0000000..cd0d877
--- /dev/null
+++ b/compat/memmem.c
@@ -0,0 +1,29 @@
+#include "../git-compat-util.h"
+
+void *gitmemmem(const void *haystack, size_t haystack_len,
+ const void *needle, size_t needle_len)
+{
+ const char *begin = haystack;
+ const char *last_possible = begin + haystack_len - needle_len;
+
+ /*
+ * The first occurrence of the empty string is deemed to occur at
+ * the beginning of the string.
+ */
+ if (needle_len == 0)
+ return (void *)begin;
+
+ /*
+ * Sanity check, otherwise the loop might search through the whole
+ * memory.
+ */
+ if (haystack_len < needle_len)
+ return NULL;
+
+ for (; begin <= last_possible; begin++) {
+ if (!memcmp(begin, needle, needle_len))
+ return (void *)begin;
+ }
+
+ return NULL;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index ca0a597..1bfbdeb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -172,6 +172,12 @@ extern uintmax_t gitstrtoumax(const char *, char **, int);
extern const char *githstrerror(int herror);
#endif
+#ifdef NO_MEMMEM
+#define memmem gitmemmem
+void *gitmemmem(const void *haystack, size_t haystacklen,
+ const void *needle, size_t needlelen);
+#endif
+
extern void release_pack_memory(size_t, int);
static inline char* xstrdup(const char *str)
--
1.5.3
^ permalink raw reply related
* Re: [PATCH 2/2] git-commit: Add --no-status option
From: Alex Riesen @ 2007-09-06 22:15 UTC (permalink / raw)
To: Dmitry V. Levin; +Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <20070905234953.GB643@nomad.office.altlinux.org>
Dmitry V. Levin, Thu, Sep 06, 2007 01:49:53 +0200:
> By default, git-commit runs git-runstatus to print changes between
> the index and the working tree. This operation is very costly and
> is not always necessary. New option allows user to commit without
> running git-runstatus when appropriate.
Not a very good name for the option. Noone, except for a few, knows
that git-commit runs runstatus. The name makes no sense.
"git-commit --index", perhaps (the current state of index to be
commited). Or "git-commit --prepared" (the commit is prepared and
there can be stored immediately). Even a dumb "--fast" would be
better (because it is the purpose of the patch).
Besides, now when you disabled runstatus, you better check for changes
in the index with something like "git diff --quiet --cached".
^ permalink raw reply
* Re: [PATCH] git-svn: remove --first-parent, add --upstream
From: Lars Hjemli @ 2007-09-06 22:14 UTC (permalink / raw)
To: Eric Wong; +Cc: Junio C Hamano, git
In-Reply-To: <20070906213556.GA21234@soma>
On 9/6/07, Eric Wong <normalperson@yhbt.net> wrote:
> Wait, actually. --upstream won't ever populate the refs array in
> working_head_info for dcommit
Sorry, I didn't realize that working_head_info() collected commit-ids
later used by dcommit. But to implement --upstream we could maybe do
something like this:
sub working_head_info {
my ($head, $refs) = @_;
if (defined $_upstream) {
working_head_info_traverse($head, \$refs);
return working_head_info_traverse($_upstream, undef);
}
return working_head_info_traverse($head, \$refs);
}
sub working_head_info_traverse {
my ($head, $refs) = @_;
my ($fh, $ctx) = command_output_pipe('log', '--no-color',
'--first-parent', $head);
...
(This was written straight into firefox, late at night, by a perl
illiterate. Please be gentle...)
--
larsh
^ permalink raw reply
* rebase from ambiguous ref discards changes
From: Keith Packard @ 2007-09-06 21:48 UTC (permalink / raw)
To: Git Mailing List; +Cc: keithp
[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]
So, I started with a very simple repository
---*--- master
\
-- origin/master
From master, I did
$ git-rebase origin/master
warning: refname 'master' is ambiguous.
First, rewinding head to replay your work on top of it...
HEAD is now at 2a8592f... Fix G33 GTT stolen mem range
Fast-forwarded master to origin/master.
$
Note the lack of the usual 'Applying <patch>' messages.
checking the tree, I now had
---*
\
-- origin/master
master
with my patch lost.
recovering my patch (having the ID in my terminal window from the
commit), I named it 'master-with-fix'
---*--- master-with-fix
\
-- origin/master
master
Now the rebase from 'master-with-fix worked as expected:
$ git-rebase origin/master
First, rewinding head to replay your work on top of it...
HEAD is now at 2a8592f... Fix G33 GTT stolen mem range
Applying Switch to pci_device_map_range/pci_device_unmap_range APIs.
Adds trailing whitespace.
.dotest/patch:225:
Adds trailing whitespace.
.dotest/patch:226: if (IS_I965G(pI830))
Adds trailing whitespace.
.dotest/patch:446:
dev->regions[mmio_bar].size,
Adds trailing whitespace.
.dotest/patch:449:
warning: 4 lines add whitespace errors.
Wrote tree cd373666254d56a137d282deeb15a2ccaf8da22b
Committed: 286f5df0b62f571cbb4dbf120679d3af029b8775
$
And the tree looks right too:
--- origin/master --- master-with-fix
master
Seems like there's something going on when 'master' is ambiguous, or
perhaps some other problem.
This is all from version 1.5.3, but I think I've seen this on 1.5.2 as
well.
Git made me sad today; I'm not sure it's ever disappointed like this
before.
--
keith.packard@intel.com
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] git-svn: remove --first-parent, add --upstream
From: Eric Wong @ 2007-09-06 21:35 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <20070906210155.GA20938@soma>
Eric Wong <normalperson@yhbt.net> wrote:
> Lars Hjemli <hjemli@gmail.com> wrote:
> > This makes git-svn always issue the --first-parent option to git-log when
> > trying to establish the "base" subversion branch, so the --first-parent
> > option to git-svn is no longer needed. Instead a new option, --upstream
> > <revspec>, is introduced. When this is specified the search for embedded
> > git-svn-id lines in commit messages starts at the specified revision, if
> > not specified the search starts at HEAD.
> >
> > Signed-off-by: Lars Hjemli <hjemli@gmail.com>
>
> Looks good to me.
>
> Acked-by: Eric Wong <normalperson@yhbt.net>
Wait, actually. --upstream won't ever populate the refs array in
working_head_info for dcommit (but --first-parent should be great for
that).
But the rest of the commands is fine. I also just noticed cmd_find_rev
passes the @refs argument to working_head_info() unnecessarily.
--
Eric Wong
^ permalink raw reply
* Re: [PATCH 5/3] archive: rename attribute specfile to export-subst
From: Junio C Hamano @ 2007-09-06 21:03 UTC (permalink / raw)
To: Johannes Schindelin
Cc: René Scharfe, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <Pine.LNX.4.64.0709061811460.28586@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> The bigger question is now if these two patches should be folded back into
> your original patch series, or stand alone as commits of their own...
That is no brainer, as there is a simple and hard rule that any
topic already in 'next' are not to be rewound ever. Follow-up
patches are the right thing to do in this case.
^ permalink raw reply
* Re: [PATCH] git-svn: remove --first-parent, add --upstream
From: Eric Wong @ 2007-09-06 21:01 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <1189096669534-git-send-email-hjemli@gmail.com>
Lars Hjemli <hjemli@gmail.com> wrote:
> This makes git-svn always issue the --first-parent option to git-log when
> trying to establish the "base" subversion branch, so the --first-parent
> option to git-svn is no longer needed. Instead a new option, --upstream
> <revspec>, is introduced. When this is specified the search for embedded
> git-svn-id lines in commit messages starts at the specified revision, if
> not specified the search starts at HEAD.
>
> Signed-off-by: Lars Hjemli <hjemli@gmail.com>
Looks good to me.
Acked-by: Eric Wong <normalperson@yhbt.net>
> ---
> Documentation/git-svn.txt | 10 +++++-----
> git-svn.perl | 18 +++++++++---------
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
> index 42d7b82..2903777 100644
> --- a/Documentation/git-svn.txt
> +++ b/Documentation/git-svn.txt
> @@ -317,15 +317,15 @@ This is only used with the 'dcommit' command.
> Print out the series of git arguments that would show
> which diffs would be committed to SVN.
>
> ---first-parent::
> +--upstream=<revspec>::
>
> This is only used with the 'dcommit', 'rebase', 'log', 'find-rev' and
> 'show-ignore' commands.
>
> -These commands tries to detect the upstream subversion branch by means of
> -the embedded 'git-svn-id' line in commit messages. When --first-parent is
> -specified, git-svn only follows the first parent of each commit, effectively
> -ignoring commits brought into the current branch through merge-operations.
> +These commands tries to detect the upstream subversion branch by traversing
> +the first parent of each commit (starting at HEAD), looking for an embedded
> +'git-svn-id' line in the commit messages. When --upstream is specified,
> +git-svn starts the traversal at the specified commit instead of HEAD.
>
> --
>
> diff --git a/git-svn.perl b/git-svn.perl
> index d21eb7f..947a944 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -59,7 +59,7 @@ my ($_stdin, $_help, $_edit,
> $_template, $_shared,
> $_version, $_fetch_all, $_no_rebase,
> $_merge, $_strategy, $_dry_run, $_local,
> - $_prefix, $_no_checkout, $_verbose, $_first_parent);
> + $_prefix, $_no_checkout, $_verbose, $_upstream);
> $Git::SVN::_follow_parent = 1;
> my %remote_opts = ( 'username=s' => \$Git::SVN::Prompt::_username,
> 'config-dir=s' => \$Git::SVN::Ra::config_dir,
> @@ -119,14 +119,14 @@ my %cmd = (
> 'dry-run|n' => \$_dry_run,
> 'fetch-all|all' => \$_fetch_all,
> 'no-rebase' => \$_no_rebase,
> - 'first-parent' => \$_first_parent,
> + 'upstream=s' => \$_upstream,
> %cmt_opts, %fc_opts } ],
> 'set-tree' => [ \&cmd_set_tree,
> "Set an SVN repository to a git tree-ish",
> { 'stdin|' => \$_stdin, %cmt_opts, %fc_opts, } ],
> 'show-ignore' => [ \&cmd_show_ignore, "Show svn:ignore listings",
> { 'revision|r=i' => \$_revision,
> - 'first-parent' => \$_first_parent
> + 'upstream=s' => \$_upstream
> } ],
> 'multi-fetch' => [ \&cmd_multi_fetch,
> "Deprecated alias for $0 fetch --all",
> @@ -148,11 +148,11 @@ my %cmd = (
> 'authors-file|A=s' => \$_authors,
> 'color' => \$Git::SVN::Log::color,
> 'pager=s' => \$Git::SVN::Log::pager,
> - 'first-parent' => \$_first_parent
> + 'upstream=s' => \$_upstream
> } ],
> 'find-rev' => [ \&cmd_find_rev, "Translate between SVN revision numbers and tree-ish",
> {
> - 'first-parent' => \$_first_parent
> + 'upstream=s' => \$_upstream
> } ],
> 'rebase' => [ \&cmd_rebase, "Fetch and rebase your working directory",
> { 'merge|m|M' => \$_merge,
> @@ -160,7 +160,7 @@ my %cmd = (
> 'strategy|s=s' => \$_strategy,
> 'local|l' => \$_local,
> 'fetch-all|all' => \$_fetch_all,
> - 'first-parent' => \$_first_parent,
> + 'upstream=s' => \$_upstream,
> %fc_opts } ],
> 'commit-diff' => [ \&cmd_commit_diff,
> 'Commit a diff between two trees',
> @@ -818,9 +818,9 @@ sub cmt_metadata {
>
> sub working_head_info {
> my ($head, $refs) = @_;
> - my @args = ('log', '--no-color');
> - push @args, '--first-parent' if $_first_parent;
> - my ($fh, $ctx) = command_output_pipe(@args, $head);
> + my @args = ('log', '--no-color', '--first-parent');
> + push @args, ($_upstream ? $_upstream : $head);
> + my ($fh, $ctx) = command_output_pipe(@args);
> my $hash;
> my %max;
> while (<$fh>) {
> --
> 1.5.3.1.g0e33-dirty
>
--
Eric Wong
^ permalink raw reply
* Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
From: Pierre Habouzit @ 2007-09-06 20:54 UTC (permalink / raw)
To: David Kastrup; +Cc: Johannes Schindelin, git
In-Reply-To: <85hcm7a6t4.fsf@lola.goethe.zz>
[-- Attachment #1: Type: text/plain, Size: 957 bytes --]
On Thu, Sep 06, 2007 at 08:16:07PM +0000, David Kastrup wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > On Thu, Sep 06, 2007 at 02:05:36PM +0000, Johannes Schindelin wrote:
>
> >> You used spaces instead of tabs here.
> >
> > crap, and I did that in the 5th patch as well. well, I'll maybe send
> > privately a "fixed" version of the patch to junio then, to avoid
> > flooding the list with spacing issues.
> >
> > And I'll also set my vim to use tabs when I'm hacking on git.
>
> Just use Emacs and
> M-x c-set-style RET linux RET
I'll give you a hunch:
apt-cache showsrc vim|grep Uplo
…, Pierre Habouzit <madcoder@debian.org>, …
And it's not like vim doesn't has this either :)
Anyways, we're going a bit OT aren't we ?
--
·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: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"
From: René Scharfe @ 2007-09-06 20:53 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <46E0647A.10000@lsrfire.ath.cx>
René Scharfe schrieb:
> Johannes Schindelin schrieb:
>>> +
>>> + b = memchr(a, '$', len);
>>> + if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
>>> + break;
>> Wouldn't memmem(buffer, len, "$Format:", 8) be better here?
>
> Oh, that's a nice GNU extension, didn't know it before. We might import
> it to compat etc., but I think that's better left for a follow-up patch.
Just noticed: if the memcmp() above finds a difference, the code should
*not* break out of the loop. Ahem. Perhaps I should first add memmem()
after all...
René
^ permalink raw reply
* Re: [PATCH 5/3] archive: rename attribute specfile to export-subst
From: René Scharfe @ 2007-09-06 20:38 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano
Cc: Andreas Ericsson, Git Mailing List, Michael Gernoth,
Thomas Glanzmann
In-Reply-To: <Pine.LNX.4.64.0709061811460.28586@racer.site>
Johannes Schindelin schrieb:
> The bigger question is now if these two patches should be folded back
> into your original patch series, or stand alone as commits of their
> own...
Yes, indeed. I submitted them as add-on patches because I noticed the
first three are already in next, but I sure can make a fresh three-part
series based on master. Junio?
Thanks,
René
^ permalink raw reply
* Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"
From: René Scharfe @ 2007-09-06 20:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Andreas Ericsson, Git Mailing List,
Michael Gernoth, Thomas Glanzmann
In-Reply-To: <Pine.LNX.4.64.0709061803590.28586@racer.site>
Johannes Schindelin schrieb:
>> +static void *format_specfile(const struct commit *commit, const char *format,
>> + unsigned long *sizep)
>
> Should this not be "char *buffer" instead of "const char *format"? Or
> even better: a "struct strbuf *"?
const is correct, because the data in the buffer shouldn't be (and
isn't) modified.
"buffer" is a generic term; "format" on the other hand indicates the
meaning of the data within the buffer.
Input and output might indeed be passed along as struct strbuf, even
more so since the new API encourages its use as generic buffer (format
can contain NULs).
>> +{
>> + unsigned long len = *sizep, result_len = 0;
>> + const char *a = format;
>> + char *result = NULL;
>> +
>> + for (;;) {
>> + const char *b, *c;
>> + char *fmt, *formatted = NULL;
>> + unsigned long a_len, fmt_len, formatted_len, allocated = 0;
>
> Maybe initialise formatted_len, just to be on the safe side?
I wish I could use C99 syntax and move its declaration down to its first
use, then it would be obvious that formatted_len is never used without
initialization.
>> +
>> + b = memchr(a, '$', len);
>> + if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
>> + break;
>
> Wouldn't memmem(buffer, len, "$Format:", 8) be better here?
Oh, that's a nice GNU extension, didn't know it before. We might import
it to compat etc., but I think that's better left for a follow-up patch.
> A general comment: since you plan to output the result into a file anyway,
> it should be even easier to avoid realloc(), and do a
> print_formatted_specfile() instead of a format_specfile(), no?
Hmm, not sure what you mean. At least archive-tar needs the expanded
contents in a buffer (not immediately written to stdout) because it
tries to mimic a real tar and always writes in blocks of 10k and
therefore needs to buffer the output.
Thanks!
René
^ permalink raw reply
* Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
From: David Kastrup @ 2007-09-06 20:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <20070906171734.GG8451@artemis.corp>
Pierre Habouzit <madcoder@debian.org> writes:
> On Thu, Sep 06, 2007 at 02:05:36PM +0000, Johannes Schindelin wrote:
>> You used spaces instead of tabs here.
>
> crap, and I did that in the 5th patch as well. well, I'll maybe send
> privately a "fixed" version of the patch to junio then, to avoid
> flooding the list with spacing issues.
>
> And I'll also set my vim to use tabs when I'm hacking on git.
Just use Emacs and
M-x c-set-style RET linux RET
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: People unaware of the importance of "git gc"?
From: Steven Grimm @ 2007-09-06 18:29 UTC (permalink / raw)
To: Linus Torvalds
Cc: Junio C Hamano, Johannes Schindelin, Nicolas Pitre, Nix,
Git Mailing List
In-Reply-To: <alpine.LFD.0.999.0709061906010.5626@evo.linux-foundation.org>
Linus Torvalds wrote:
> IOW, if you get lots of small incrmental packs, after a while you really
> *do* need to do "git gc" to get the real pack generated.
>
I wonder if it makes sense to repack just the small incremental packs
into a large (but still incremental) pack, rather than repacking the
entire repository. Presumably that would be a lot faster than a full
"git gc", while still giving you reasonably good packing (at least, if
the threshold is set to a hugh enough number of small packs) and keeping
things fast. That could run as a second phase of "git gc --auto" -- it
should be quick enough to not be too terribly annoying since we're not
running it in the background.
Yeah, if you use the same repo for a long time, you'll accumulate a ton
of medium-sized packs this way, but (a) that's much better than the
situation we have today, and (b) it puts off the performance degradation
for long enough that it becomes more reasonable to expect people to find
out about running the full "git gc" in the meantime, or for git to
further evolve to not need it.
-Steve
^ permalink raw reply
* Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Pierre Habouzit @ 2007-09-06 18:27 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <1189102714.3423.22.camel@hinata.boston.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]
On Thu, Sep 06, 2007 at 06:18:34PM +0000, Kristian Høgsberg wrote:
> On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:
> > On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
> > > On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> > > > - memcpy(path.buf, base, baselen);
> > > > - memcpy(path.buf + baselen, filename, filenamelen);
> > > > - path.len = baselen + filenamelen;
> > > > - path.buf[path.len] = '\0';
> > > > + strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> > > > + strbuf_reset(&path);
> > >
> > > Does strbuf_reset() do anything here?
> > >
> > > > + strbuf_add(&path, base, baselen);
> >
> > Yes _reset() sets length to 0. so the add here will write at the start
> > of the buffer again. It definitely is important !
>
> But where was length set to non-zero? path is initialized on entry to
> the function, and strbuf_grow() just increases the allocation, not
> length, right?
The path is static, hence when you reenter the function you have the
last value in it. The fact that it's static may be questionable, but it
was like it before, I kept it, I've supposed it was for performance
reasons.
--
·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: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Kristian Høgsberg @ 2007-09-06 18:18 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <20070906180858.GJ8451@artemis.corp>
On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:
> On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
> > On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> > > + strbuf_grow(sb, len);
> > > + strbuf_addf(sb, "%u %s=", len, keyword);
> > > + strbuf_add(sb, value, valuelen);
> > > + strbuf_addch(sb, '\n');
> > > }
> >
> > This entire function can be collapsed to just:
> >
> > strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
>
> yes, but it's less efficient, because %.*s says that sprintf must copy
> at most valuelen bytes from value, but it still has to stop if it finds
> a \0 before. And the strbuf_grow has sense because the extend policy at
> snprintf time is optimistic: we try to write, and if it didn't fit, we
> try again. So there is a huge benefit if we have a clue of the final
> size.
Yeah, my strbuf_addf() suggestion doesn't work, since the format seems
to indicate the embedded NULs indeed is allowed (the %u is the length of
the value), so *.% could truncate data.
> > > - memcpy(path.buf, base, baselen);
> > > - memcpy(path.buf + baselen, filename, filenamelen);
> > > - path.len = baselen + filenamelen;
> > > - path.buf[path.len] = '\0';
> > > + strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> > > + strbuf_reset(&path);
> >
> > Does strbuf_reset() do anything here?
> >
> > > + strbuf_add(&path, base, baselen);
>
> Yes _reset() sets length to 0. so the add here will write at the start
> of the buffer again. It definitely is important !
But where was length set to non-zero? path is initialized on entry to
the function, and strbuf_grow() just increases the allocation, not
length, right?
Kristian
^ permalink raw reply
* Re: People unaware of the importance of "git gc"?
From: Linus Torvalds @ 2007-09-06 18:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Nicolas Pitre, Nix, Steven Grimm,
Git Mailing List
In-Reply-To: <7vk5r3adlx.fsf@gitster.siamese.dyndns.org>
On Thu, 6 Sep 2007, Junio C Hamano wrote:
>
> I thought the whole point of "gc --auto" was to have something
> that does not lose/prune any objects, even the ones that do not
> seem to be referenced from anywhere. That is why invocations of
> "git gc --auto" do not say --prune as you saw the second patch,
> and the repack command "gc --auto" runs is "repack -d -l"
> instead of "repack -a -d -l", which means that it does run
> git-prune-packed after repacking but not git-prune.
I think "repack -d -l" should be ok from a safety perspective, but I'd
also like to say that always running it incrementally is going to largely
suck after a time.
IOW, if you get lots of small incrmental packs, after a while you really
*do* need to do "git gc" to get the real pack generated.
In the case I saw, James really had hundreds of pack-files. That makes all
our object lookups suck. Yes, not having loose objects at all is a big
deal too, and yes, we try to start from the last pack-file we found (for
the locality that we hope is there), but it's still pretty bad from a
cache usage standpoint, and when we create a new object, we'll first
search (in vain) in all the hundreds of pack-files.
So would "git gc --auto" have helped James? I'm sure it would have. But he
already had lots of pack-files from doing "git fetch/pull", and while
doing the "git gc --auto" will likely *delay* the point where you need to
do a full repack, it doesn't make it go away.
We still need to tell people to do a full git gc at some point, or do it
for them. And the longer you delay doing it, the more expensive it's going
to get to do and/or the worse the final packing is going to be (especially
if it ends up reusing non-optimal packing decisions from the smaller
packs).
So I think the --auto stuff is still worth it, but it's really just
pushing the pain somewhat further out.
(In the kernel community, if you fetch my tree daily, you really *are*
going to have hundreds and hundreds of packfiles just from doing that).
So I'd really like us to also remind people to do a *real* and full "git
gc", not just the incremental ones.
Linus
^ permalink raw reply
* Re: Git's database structure
From: Steven Grimm @ 2007-09-06 18:14 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jon Smirl, Julian Phillips, Andreas Ericsson, Theodore Tso,
Junio C Hamano, Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0709061354180.28586@racer.site>
Johannes Schindelin wrote:
> But you can add _yet another_ index to it, which can be generated on the
> fly, so that Git only has to generate the information once, and then reuse
> it later. As a benefit of this method, the underlying well-tested
> structure needs no change at all.
>
And in fact, you can do this today, without modifying git-blame at all,
by (ab)using its "-S" option (which lets you specify a custom ancestry
chain to search). By coincidence, I was just showing some people at my
office how to do this yesterday. I'll cut-and-paste from the email I
sent them. I am not claiming this is nearly as desirable as a built-in,
auto-updated secondary index, but it proves the concept, anyway.
Fast-to-generate version:
git-rev-list HEAD -- main.c | awk '{if (last) print last " " $0;
last=$0;}' > /tmp/revlist
This speeds things up a lot, because git blame doesn't have to examine
other revisions:
time git blame main.c
1.56s user 0.30s system 99% cpu 1.868 total
time git blame -S /tmp/revlist main.c
0.21s user 0.03s system 96% cpu 0.249 total
The bad news is that generating that revision list is a bit slow, and if
you do it the naive way I suggested above, you can't use the rev list
with the -M option (to follow renames). The good news is that it's
possible to have that too if you generate a list of revisions that
includes the renames:
# Generate a list of all revisions in the right order (only need to do
this once, not once per file)
git rev-list HEAD > /tmp/all-revs
# Generate a list of the revisions that touched this file, following
copies/renames.
# Could do this in fewer commands but this is hopefully easier to follow.
git blame --porcelain -M main.c | \
egrep '^[0-9a-f]{40}' | \
cut -d' ' -f1 | \
fgrep -f - /tmp/all-revs | \
awk '{if (last) print last " " $0; last=$0;}' > /tmp/revlist
Then -M is fast too:
time git blame -M main.c
1.72s user 0.27s system 89% cpu 2.219 total
time git blame -M -S /tmp/revlist main.c
0.29s user 0.03s system 93% cpu 0.341 total
Oddly, if you use the -S option, "git blame -C" actually gets
significantly *slower*. I am not sure why.
-Steve
^ permalink raw reply
* Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Pierre Habouzit @ 2007-09-06 18:08 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <1189101569.3423.17.camel@hinata.boston.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2139 bytes --]
On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
> On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> > + strbuf_grow(sb, len);
> > + strbuf_addf(sb, "%u %s=", len, keyword);
> > + strbuf_add(sb, value, valuelen);
> > + strbuf_addch(sb, '\n');
> > }
>
> This entire function can be collapsed to just:
>
> strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
yes, but it's less efficient, because %.*s says that sprintf must copy
at most valuelen bytes from value, but it still has to stop if it finds
a \0 before. And the strbuf_grow has sense because the extend policy at
snprintf time is optimistic: we try to write, and if it didn't fit, we
try again. So there is a huge benefit if we have a clue of the final
size.
I would not change a thing.
> > + strbuf_init(&ext_header);
>
> Just use your STRBUF_INIT macro?
Many people dilike it, I'm not sure it's a good idea either, and the
performance hit should be negligible, if it's not, then we can still
make the _init() function an inline.
> > if (ext_header.len > 0) {
> > write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
> > - free(ext_header.buf);
> > }
>
> Remove excess braces?
bah, I don't like to strip braces so I won't do that, else you end up
with stupidities like:
if (foo)
// bar();
do_some_very_important_stuff();
Call me paranoid but well, it saved me so many times ...
> > - memcpy(path.buf, base, baselen);
> > - memcpy(path.buf + baselen, filename, filenamelen);
> > - path.len = baselen + filenamelen;
> > - path.buf[path.len] = '\0';
> > + strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> > + strbuf_reset(&path);
>
> Does strbuf_reset() do anything here?
>
> > + strbuf_add(&path, base, baselen);
Yes _reset() sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ 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