* Re: [PATCH 2/2] --pretty=format: on-demand format expansion
From: Jeff King @ 2007-11-09 4:50 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Paul Mackerras, Git Mailing List, Pierre Habouzit,
Johannes Schindelin
In-Reply-To: <4733AEA6.1040802@lsrfire.ath.cx>
On Fri, Nov 09, 2007 at 01:49:42AM +0100, René Scharfe wrote:
> Another way is to use a callback based approach together with the
> strbuf library to keep allocations to a minimum and avoid string
> copies. That's what this patch does. It introduces a new strbuf
> function, strbuf_expand().
I think this is definitely the right approach, but there are some format
strings where the performance will be worse. Specifically:
- formatting expensive items multiple times will incur work
proportional to the number of times the item is used (in the old
code, it was calculated just once). e.g., "%h%h%h%h"
- formatting some items goes to some work that can be re-used by other
items (e.g., %ad and %ar both need to parse the author date)
And we could obviously overcome both by caching the results of expensive
operations. I'm not sure if these will be a problem in practice. For
the first one, the new code is so much faster that I needed to do
git-log --pretty=format:%h%h%h%h%h%h%h%h
to get a performance regression from the old code, which seems rather
unlikely. For the second, it is easy to imagine multiple "person" items
being used together, although their cost to produce is not all that
high. It looks like about .05 seconds to parse a date (over all commits
in git.git):
$ time ./git-log --pretty='format:' >/dev/null
real 0m0.441s
user 0m0.424s
sys 0m0.004s
$ time ./git-log --pretty='format:%ad' >/dev/null
real 0m0.477s
user 0m0.472s
sys 0m0.000s
$ time ./git-log --pretty='format:%ad %aD' >/dev/null
real 0m0.527s
user 0m0.520s
sys 0m0.004s
where the last two could probably end up costing about the same if we cached
the author parsing (but the caching will have a cost, too, so it might not end
up being a big win).
So it might make sense to cache some items as we figure them out. This
should be done by the calling code and not by strbuf_expand (since it
doesn't know which things are worth caching (and for fast things,
allocating memory for a cache entry is likely to be slower), or how the
expansions relate to each other).
A partial patch on top of yours is below (it caches commit and tree
abbreviations; parent abbreviations and person-parsing are probably
worth doing). Some timings:
Null format (these are average-looking runs; the differences got lost
in the noise):
# your patch
$ time git-log --pretty=format: >/dev/null
real 0m0.409s
user 0m0.384s
sys 0m0.012s
# with my patch
$ time ./git-log --pretty=format: >/dev/null
real 0m0.413s
user 0m0.404s
sys 0m0.004s
Single abbrev lookup (mine should be slightly slower because of
malloc/free of cache):
# your patch
$ time git-log --pretty=format:%h >/dev/null
real 0m0.536s
user 0m0.456s
sys 0m0.080s
# with my patch
$ time ./git-log --pretty=format:%h >/dev/null
real 0m0.553s
user 0m0.464s
sys 0m0.088s
Two abbrev lookups (I win by a little bit, but definitely not lost in
the noise):
# your patch
$ time git-log --pretty=format:%h%h >/dev/null
real 0m0.671s
user 0m0.496s
sys 0m0.144s
# my patch
$ time ./git-log --pretty=format:%h%h >/dev/null
real 0m0.567s
user 0m0.480s
sys 0m0.080s
And of course I can make pathological cases where mine wins hands down
(on "%h%h%h%h%h%h%h%h", mine stays the same but yours bumps to 1.2s).
So I think this is probably worth doing. Even if doubled work isn't the
common case,
1. It doesn't hurt the common case much at all (I think on average it
is slower, but the timings were totally lost in the noise)
2. It has a measurable impact on reasonable cases (like just using an
expensive substitution twice)
3. It has a huge impact on pathological cases (though I'm not sure we
care about those that much)
4. It's very little extra code, and it should be obvious to read. It
also documents the technique for other users of strbuf_expand,
where the "doubled" cases may be more common.
-Peff
---
pretty.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
1 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/pretty.c b/pretty.c
index 9fbd73f..8ae6fdd 100644
--- a/pretty.c
+++ b/pretty.c
@@ -282,6 +282,27 @@ static char *logmsg_reencode(const struct commit *commit,
return out;
}
+struct pretty_context {
+ const struct commit *commit;
+ char *abbrev_commit;
+ char *abbrev_tree;
+};
+
+static void pretty_context_init(struct pretty_context *p, const struct commit *c)
+{
+ p->commit = c;
+ p->abbrev_commit = NULL;
+ p->abbrev_tree = NULL;
+}
+
+static void pretty_context_free(struct pretty_context *p)
+{
+ if (p->abbrev_commit)
+ free(p->abbrev_commit);
+ if (p->abbrev_tree)
+ free(p->abbrev_tree);
+}
+
static void format_person_part(struct strbuf *sb, char part,
const char *msg, int len)
{
@@ -355,9 +376,10 @@ static void format_person_part(struct strbuf *sb, char part,
}
static void format_commit_item(struct strbuf *sb, const char *placeholder,
- void *context)
+ void *vcontext)
{
- const struct commit *commit = context;
+ struct pretty_context *context = vcontext;
+ const struct commit *commit = context->commit;
struct commit_list *p;
int i;
enum { HEADER, SUBJECT, BODY } state;
@@ -394,15 +416,23 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,
strbuf_addstr(sb, sha1_to_hex(commit->object.sha1));
return;
case 'h': /* abbreviated commit hash */
- strbuf_addstr(sb, find_unique_abbrev(commit->object.sha1,
- DEFAULT_ABBREV));
+ if (!context->abbrev_commit)
+ context->abbrev_commit = xstrdup(
+ find_unique_abbrev(
+ commit->object.sha1,
+ DEFAULT_ABBREV));
+ strbuf_addstr(sb, context->abbrev_commit);
return;
case 'T': /* tree hash */
strbuf_addstr(sb, sha1_to_hex(commit->tree->object.sha1));
return;
case 't': /* abbreviated tree hash */
- strbuf_addstr(sb, find_unique_abbrev(commit->tree->object.sha1,
- DEFAULT_ABBREV));
+ if (!context->abbrev_tree)
+ context->abbrev_tree = xstrdup(
+ find_unique_abbrev(
+ commit->tree->object.sha1,
+ DEFAULT_ABBREV));
+ strbuf_addstr(sb, context->abbrev_tree);
return;
case 'P': /* parent hashes */
for (p = commit->parents; p; p = p->next) {
@@ -505,7 +535,10 @@ void format_commit_message(const struct commit *commit,
"m", /* left/right/bottom */
NULL
};
- strbuf_expand(sb, format, placeholders, format_commit_item, (void *)commit);
+ struct pretty_context context;
+ pretty_context_init(&context, commit);
+ strbuf_expand(sb, format, placeholders, format_commit_item, &context);
+ pretty_context_free(&context);
}
static void pp_header(enum cmit_fmt fmt,
^ permalink raw reply related
* Re: [PATCH 2/2] --pretty=format: on-demand format expansion
From: Jeff King @ 2007-11-09 4:52 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Paul Mackerras, Git Mailing List, Pierre Habouzit,
Johannes Schindelin
In-Reply-To: <4733AEA6.1040802@lsrfire.ath.cx>
On Fri, Nov 09, 2007 at 01:49:42AM +0100, René Scharfe wrote:
> + strbuf_expand(sb, format, placeholders, format_commit_item, (void *)commit);
This void cast is pointless, since all pointers types convert implicitly
to void pointers anyway. At best, it does nothing, and at worst, it
hides an actual type error if the function signature or the type of
'commit' change.
(In the patch I just sent out, I had to change this line anyway, and
removed the cast).
-Peff
^ permalink raw reply
* Re: corrupt object on git-gc
From: Christian Couder @ 2007-11-09 5:13 UTC (permalink / raw)
To: Yossi Leybovich; +Cc: git
In-Reply-To: <6C2C79E72C305246B504CBA17B5500C9029A36A1@mtlexch01.mtl.com>
Le vendredi 9 novembre 2007, Yossi Leybovich a écrit :
>
> Unfortunately I can't get this object from backup directories as advise
> in the FAQ.
> Can this object manually fixed by any tool? (the object is attached) I
> don't even know to which file/tree/commit it belong how can I know that
> ?
Could you try something like:
git-cat-file -p 4b9458b3786228369c63936db65827de3cc06200
in your repository ?
Thanks,
Christian.
^ permalink raw reply
* git push failing, unpacker error
From: Jon Smirl @ 2007-11-09 5:55 UTC (permalink / raw)
To: Git Mailing List
Why is this push failing?
jonsmirl@terra:~/mpc5200b$ git push dreamhost
updating 'refs/remotes/linus/m24' using 'refs/heads/m24'
from 0000000000000000000000000000000000000000
to 06c52341cc9265b23e2d11eb631ff45e763215c0
updating 'refs/remotes/linus/m25' using 'refs/heads/m25'
from 0000000000000000000000000000000000000000
to bef47d2064dd1a848597246291ef8a7654387dde
updating 'refs/remotes/linus/m26' using 'refs/heads/m26'
from 0000000000000000000000000000000000000000
to 35200c360d85fb74da55e57dfb16cb9d50b253e9
updating 'refs/remotes/linus/m28' using 'refs/heads/m28'
from 0000000000000000000000000000000000000000
to a77cec0752aa8a00b95a44d1028d5f9b989354a4
updating 'refs/remotes/linus/m29' using 'refs/heads/m29'
from 0000000000000000000000000000000000000000
to 0c4bfa1cbed0070e5d0cc739ab0ac4c04290b8d3
Counting objects: 81156, done.
Compressing objects: 100% (16575/16575), done.
Writing objects: 100% (70133/70133), done.
Total 70133 (delta 57185), reused 66194 (delta 53490)
unpack index-pack abnormal exit
ng refs/remotes/linus/m24 n/a (unpacker error)
ng refs/remotes/linus/m25 n/a (unpacker error)
ng refs/remotes/linus/m26 n/a (unpacker error)
ng refs/remotes/linus/m28 n/a (unpacker error)
ng refs/remotes/linus/m29 n/a (unpacker error)
error: failed to push to 'ssh://jonsmirl1@git.digispeaker.com/~/mpc5200b.git'
jonsmirl@terra:~/mpc5200b$
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [BUG] git-rebase fails when a commit message contains a diff
From: Steffen Prohaska @ 2007-11-09 7:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonas Fonseca, git
In-Reply-To: <7vhcjwxk1s.fsf@gitster.siamese.dyndns.org>
On Nov 9, 2007, at 2:51 AM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> That's a known design limitation of applymbox/mailinfo. Any
>> line that looks like a beginning of a patch in e-mail ("^--- ",
>> "^---$", "^diff -", and "^Index: ") terminates the commit log.
>
> Ok, so that explains the symptom. What's the next step?
>
> * The applymbox/mailinfo pair should continue to split the
> commit log message at the first such line. There is no point
> breaking established workflow, and people in communities that
> exchange patches via e-mail already know to avoid this issue
> by indenting quoted diff snippet in the log message,
> e.g. 5be507fc.
I wasn't aware of this.
Maybe git-commit should validate commit messages? If people
have a notion of a well-formed commit message this should be
verified. The message should not contain strings that indicate
termination of the commit message when sent by email. If commit
did this the evil strings would never enter a commit message
in the first place.
Actually I sometimes use '---' as a separator in text. I'm
sure I'll forget at some point that I must not use it in a
commit message. I'd be happy if git saved me from this.
If I understand correctly, the problem will remain when
sending patches by emails; even if git-rebase was changed
to use merge. Or does git-format-patch quote evil strings in
commit messages?
Steffen
^ permalink raw reply
* [PATCH] git-am: -i does not take a string parameter.
From: Junio C Hamano @ 2007-11-09 7:12 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
$ git am -3 -s -i file
spewed the usage strings back at the user while
$ git am -3 -i -s file
didn't. This fixes it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-am.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index e5af955..4126f0e 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -9,7 +9,7 @@ git-am [options] --resolved
git-am [options] --skip
--
d,dotest= use <dir> and not .dotest
-i,interactive= run interactively
+i,interactive run interactively
b,binary pass --allo-binary-replacement to git-apply
3,3way allow fall back on 3way merging if needed
s,signoff add a Signed-off-by line to the commit message
--
1.5.3.5.1624.gc2f3b4
^ permalink raw reply related
* Re: [PATCH] git-checkout: Test for relative path use.
From: Johannes Sixt @ 2007-11-09 7:13 UTC (permalink / raw)
To: David Symonds; +Cc: Junio C Hamano, git, Johannes Schindelin, Andreas Ericsson
In-Reply-To: <11945685732608-git-send-email-dsymonds@gmail.com>
David Symonds schrieb:
> +test_expect_success 'remove and restore with relative path' '
> +
> + cd dir1 &&
> + rm ../file0 &&
> + git checkout HEAD -- ../file0 &&
> + test "base" = "$(cat ../file0)" &&
> + rm ../dir2/file2 &&
> + git checkout HEAD -- ../dir2/file2 &&
> + test "bonjour" = "$(cat ../dir2/file2)" &&
> + rm ../file0 ./file1 &&
> + git checkout HEAD -- .. &&
> + test "base" = "$(cat ../file0)" &&
> + test "hello" = "$(cat file1)" &&
> + cd -
What if this test fails? Then the rest of the tests run from the wrong
directory. You should put the test in parenthesis (and drop the cd -).
-- Hannes
^ permalink raw reply
* Re: [PATCH] git-checkout: Test for relative path use.
From: David Symonds @ 2007-11-09 7:24 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, Johannes Schindelin, Andreas Ericsson
In-Reply-To: <47340895.6000403@viscovery.net>
On Nov 9, 2007 6:13 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> David Symonds schrieb:
> > +test_expect_success 'remove and restore with relative path' '
> > +
> > + cd dir1 &&
> > + rm ../file0 &&
> > + git checkout HEAD -- ../file0 &&
> > + test "base" = "$(cat ../file0)" &&
> > + rm ../dir2/file2 &&
> > + git checkout HEAD -- ../dir2/file2 &&
> > + test "bonjour" = "$(cat ../dir2/file2)" &&
> > + rm ../file0 ./file1 &&
> > + git checkout HEAD -- .. &&
> > + test "base" = "$(cat ../file0)" &&
> > + test "hello" = "$(cat file1)" &&
> > + cd -
>
> What if this test fails? Then the rest of the tests run from the wrong
> directory. You should put the test in parenthesis (and drop the cd -).
Looking at the existing tests which, when they change directories,
don't cd back to where they were; they "cd .." at the start of the
next test. I'll add a "cd .." to the relevant bits of my tests.
Dave.
^ permalink raw reply
* Re: [PATCH 2/3] rev-list: Introduce --no-output to avoid /dev/null redirects
From: Junio C Hamano @ 2007-11-09 7:32 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20071108080052.GB16690@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> @@ -640,7 +656,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> }
> }
>
> - traverse_commit_list(&revs, show_commit, show_object);
> + traverse_commit_list(&revs,
> + nooutput ? noshow_commit : show_commit,
> + nooutput ? noshow_object : show_object);
>
> return 0;
> }
The function names noshow_xxx() looked a bit funny, but I do not
offhand have better alternatives to offer.
This allows "--bisect-vars --no-output" and "--bisect-all
--bisect-vars --no-output" but makes them behave differently. I
do not think --no-output is useful with bisection anyway but
maybe it makes sense to forbid the combination?
^ permalink raw reply
* [PATCH] git-checkout: Test for relative path use.
From: David Symonds @ 2007-11-09 7:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Andreas Ericsson, Johannes Sixt,
David Symonds
In-Reply-To: <ee77f5c20711082324s39a9d441tc05c5a27e6d39f3e@mail.gmail.com>
Signed-off-by: David Symonds <dsymonds@gmail.com>
---
Tests that change directories now change back at the start of the
next test. I don't know what to do about that last test, though.
t/t2008-checkout-subdir.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 81 insertions(+), 0 deletions(-)
create mode 100755 t/t2008-checkout-subdir.sh
diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh
new file mode 100755
index 0000000..41e76c9
--- /dev/null
+++ b/t/t2008-checkout-subdir.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 David Symonds
+
+test_description='git checkout from subdirectories'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+ echo "base" > file0 &&
+ git add file0 &&
+ mkdir dir1 &&
+ echo "hello" > dir1/file1 &&
+ git add dir1/file1 &&
+ mkdir dir2 &&
+ echo "bonjour" > dir2/file2 &&
+ git add dir2/file2 &&
+ test_tick &&
+ git commit -m "populate tree"
+
+'
+
+test_expect_success 'remove and restore with relative path' '
+
+ cd dir1 &&
+ rm ../file0 &&
+ git checkout HEAD -- ../file0 &&
+ test "base" = "$(cat ../file0)" &&
+ rm ../dir2/file2 &&
+ git checkout HEAD -- ../dir2/file2 &&
+ test "bonjour" = "$(cat ../dir2/file2)" &&
+ rm ../file0 ./file1 &&
+ git checkout HEAD -- .. &&
+ test "base" = "$(cat ../file0)" &&
+ test "hello" = "$(cat file1)"
+
+'
+
+# currently in dir1/
+test_expect_success 'checkout with empty prefix' '
+
+ cd .. &&
+ rm file0 &&
+ git checkout HEAD -- file0 &&
+ test "base" = "$(cat file0)"
+
+'
+
+test_expect_success 'checkout with simple prefix' '
+
+ rm dir1/file1 &&
+ git checkout HEAD -- dir1 &&
+ test "hello" = "$(cat dir1/file1)" &&
+ rm dir1/file1 &&
+ git checkout HEAD -- dir1/file1 &&
+ test "hello" = "$(cat dir1/file1)"
+
+'
+
+test_expect_success 'checkout with complex relative path' '
+
+ rm file1 &&
+ git checkout HEAD -- ../dir1/../dir1/file1 && test -f ./file1
+
+'
+
+test_expect_failure 'relative path outside tree should fail' \
+ 'git checkout HEAD -- ../../Makefile'
+
+test_expect_failure 'incorrect relative path to file should fail (1)' \
+ 'git checkout HEAD -- ../file0'
+
+test_expect_failure 'incorrect relative path should fail (2)' \
+ 'cd dir1 && git checkout HEAD -- ./file0'
+
+# currently in dir1/
+test_expect_failure 'incorrect relative path should fail (3)' \
+ 'git checkout HEAD -- ../../file0'
+
+test_done
--
1.5.3.1
^ permalink raw reply related
* Re: [BUG] git-rebase fails when a commit message contains a diff
From: Benoit Sigoure @ 2007-11-09 7:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Jonas Fonseca, git
In-Reply-To: <Pine.LNX.4.64.0711090225110.4362@racer.site>
[-- Attachment #1: Type: text/plain, Size: 562 bytes --]
On Nov 9, 2007, at 3:28 AM, Johannes Schindelin wrote:
> Would that not be easier to read as
>
> test t = "$INTERACTIVE" &&
> git_editor "$TODO" || die "Could not execute editor"
Hmm this will `die' if you're not running interactively.
Off topic question: why do you guys always do this instead of doing,
say, this:
INTERACTIVE=false
case $1 in
.
.
.
--interactive|-i)
INTERACTIVE=:
... ;;
esac
.
.
.
if $INTERACTIVE; then
git_editor "$TODO" || die ...
fi
?
--
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]
^ permalink raw reply
* Re: [PATCH] git-checkout: Test for relative path use.
From: Junio C Hamano @ 2007-11-09 8:04 UTC (permalink / raw)
To: David Symonds
Cc: Johannes Sixt, Junio C Hamano, git, Johannes Schindelin,
Andreas Ericsson
In-Reply-To: <ee77f5c20711082324s39a9d441tc05c5a27e6d39f3e@mail.gmail.com>
"David Symonds" <dsymonds@gmail.com> writes:
> Looking at the existing tests which, when they change directories,
> don't cd back to where they were; they "cd .." at the start of the
> next test. I'll add a "cd .." to the relevant bits of my tests.
Do not follow the bad examples, please.
^ permalink raw reply
* Re: corrupt object on git-gc
From: Alex Riesen @ 2007-11-09 8:10 UTC (permalink / raw)
To: Yossi Leybovich; +Cc: git
In-Reply-To: <6C2C79E72C305246B504CBA17B5500C9029A36A1@mtlexch01.mtl.com>
Yossi Leybovich, Fri, Nov 09, 2007 00:59:47 +0100:
> I wonder if someone can help in this error
> I tried to do git-gc and got error on corrupted object.
>
> I do the following:
>
> $ git-gc
> Generating pack...
> Done counting 3037 objects.
> Deltifying 3037 objects...
> error: corrupt loose object '4b9458b3786228369c63936db65827de3cc06200'
It is loose. Nothing uses it in this repository. What do you need to
repair it for?
> fatal: object 4b9458b3786228369c63936db65827de3cc06200 cannot be read
> error: failed to run repack
>
> sleybo@SLEYBO-LT /w/work/EMC/ib.071030.001/ib
> $ cd .git/objects/4b/
>
> sleybo@SLEYBO-LT /w/work/EMC/ib.071030.001/ib/.git/objects/4b
> $ git-fsck-objects.exe 9458b3786228369c63936db65827de3cc06200
> error: corrupt loose object '4b9458b3786228369c63936db65827de3cc06200'
> error: 4b9458b3786228369c63936db65827de3cc06200: object corrupt or
> missing
> error: invalid parameter: expected sha1, got
> '9458b3786228369c63936db65827de3cc06200'
> missing blob 4b9458b3786228369c63936db65827de3cc06200
the directories directly under .git/objects contain the first bytes of
sha1, to use filesystem in a more efficient way. git-fsck expects an
sha1 (or a reference).
Try running moving the corrupt object (with its *whole* name) some
place else and run git-fsck --all.
^ permalink raw reply
* Re: [PATCH 2/3] rev-list: Introduce --no-output to avoid /dev/null redirects
From: Alex Riesen @ 2007-11-09 8:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, git
In-Reply-To: <7vejezx4b2.fsf@gitster.siamese.dyndns.org>
Junio C Hamano, Fri, Nov 09, 2007 08:32:01 +0100:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
> > @@ -640,7 +656,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> > }
> > }
> >
> > - traverse_commit_list(&revs, show_commit, show_object);
> > + traverse_commit_list(&revs,
> > + nooutput ? noshow_commit : show_commit,
> > + nooutput ? noshow_object : show_object);
> >
> > return 0;
> > }
>
> The function names noshow_xxx() looked a bit funny, but I do not
> offhand have better alternatives to offer.
"hide", "skip", "ignore"?
^ permalink raw reply
* Re: [PATCH] git-checkout: Test for relative path use.
From: David Symonds @ 2007-11-09 8:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Johannes Schindelin, Andreas Ericsson
In-Reply-To: <7v7ikrx2st.fsf@gitster.siamese.dyndns.org>
On Nov 9, 2007 7:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "David Symonds" <dsymonds@gmail.com> writes:
>
> > Looking at the existing tests which, when they change directories,
> > don't cd back to where they were; they "cd .." at the start of the
> > next test. I'll add a "cd .." to the relevant bits of my tests.
>
> Do not follow the bad examples, please.
So what would you prefer? Bracketing the whole test in parentheses
looks ugly, but I can do that if that's the only option. If I look at
t5510-fetch.sh (one of yours, Junio), there is no directory
restoration in the case of test failure, as in my original patch.
Perhaps test_ok_ and test_failure_ in test-lib.sh should restore the directory?
Dave.
^ permalink raw reply
* Re: [BUG] git-rebase fails when a commit message contains a diff
From: Johannes Sixt @ 2007-11-09 8:57 UTC (permalink / raw)
To: Benoit Sigoure
Cc: Johannes Schindelin, Junio C Hamano, Jonas Fonseca,
Git Mailing List
In-Reply-To: <6FCE17E3-9FAA-4676-B12A-369B31743DA6@lrde.epita.fr>
Benoit Sigoure schrieb:
> Off topic question: why do you guys always do this instead of doing,
> say, this:
>
> INTERACTIVE=false
>
> case $1 in
> --interactive|-i)
> INTERACTIVE=:
> ... ;;
> esac
> if $INTERACTIVE; then
> git_editor "$TODO" || die ...
> fi
Because in some shells 'false' is not a built-in.
But then this might do it without the extra process:
INTERACTIVE="! :" # false
case $1 in
--interactive|-i)
INTERACTIVE=:
... ;;
esac
if $INTERACTIVE; then
git_editor "$TODO" || die ...
fi
-- Hannes
^ permalink raw reply
* Re: [PATCH 2/3] rev-list: Introduce --no-output to avoid /dev/null redirects
From: Junio C Hamano @ 2007-11-09 8:58 UTC (permalink / raw)
To: Alex Riesen; +Cc: Shawn O. Pearce, git
In-Reply-To: <20071109081204.GB2794@steel.home>
Alex Riesen <raa.lkml@gmail.com> writes:
> Junio C Hamano, Fri, Nov 09, 2007 08:32:01 +0100:
>> "Shawn O. Pearce" <spearce@spearce.org> writes:
>>
>> > @@ -640,7 +656,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>> > }
>> > }
>> >
>> > - traverse_commit_list(&revs, show_commit, show_object);
>> > + traverse_commit_list(&revs,
>> > + nooutput ? noshow_commit : show_commit,
>> > + nooutput ? noshow_object : show_object);
>> >
>> > return 0;
>> > }
>>
>> The function names noshow_xxx() looked a bit funny, but I do not
>> offhand have better alternatives to offer.
>
> "hide", "skip", "ignore"?
But look at what the functions do. The original show_xxx() was
to print and then process. Shawn splitted them into show_xxx()
and noshow_xxx(), leaft the printing part in the former, made
the former call the latter at the end, and moved the processing
to the latter. So it is not any of the three words.
^ permalink raw reply
* Re: [BUG] git-rebase fails when a commit message contains a diff
From: Ralf Wildenhues @ 2007-11-09 9:05 UTC (permalink / raw)
To: Johannes Sixt
Cc: Benoit Sigoure, Johannes Schindelin, Junio C Hamano,
Jonas Fonseca, Git Mailing List
In-Reply-To: <473420FE.7010807@viscovery.net>
* Johannes Sixt wrote on Fri, Nov 09, 2007 at 09:57:34AM CET:
> Benoit Sigoure schrieb:
>> Off topic question: why do you guys always do this instead of doing, say,
>> this:
>>
>> INTERACTIVE=false
[...]
>> if $INTERACTIVE; then
>> git_editor "$TODO" || die ...
>> fi
>
> Because in some shells 'false' is not a built-in.
[...]
> INTERACTIVE="! :" # false
Which shells do not have 'false' but do support '!' as process exit
status negation? For them, is it important to save another fork?
Solaris sh has neither (it will error on the '!'), but it's ruled out
for git anyway.
Cheers,
Ralf
^ permalink raw reply
* Re: [PATCH] git-checkout: Test for relative path use.
From: Junio C Hamano @ 2007-11-09 9:06 UTC (permalink / raw)
To: David Symonds
Cc: Junio C Hamano, Johannes Sixt, git, Johannes Schindelin,
Andreas Ericsson
In-Reply-To: <ee77f5c20711090014qfed56e7y446c014399e47a82@mail.gmail.com>
"David Symonds" <dsymonds@gmail.com> writes:
> So what would you prefer? Bracketing the whole test in parentheses
> looks ugly, but I can do that if that's the only option. If I look at
> t5510-fetch.sh (one of yours, Junio), there is no directory
> restoration in the case of test failure, as in my original patch.
Yes, that is what I was referring to as "bad examples". The way
t4116 goes down to different directory do not look ugly to me.
^ permalink raw reply
* Re: [BUG] git-rebase fails when a commit message contains a diff
From: Benoit Sigoure @ 2007-11-09 9:07 UTC (permalink / raw)
To: Johannes Sixt
Cc: Johannes Schindelin, Junio C Hamano, Jonas Fonseca,
Git Mailing List
In-Reply-To: <473420FE.7010807@viscovery.net>
[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]
On Nov 9, 2007, at 9:57 AM, Johannes Sixt wrote:
> Benoit Sigoure schrieb:
>> Off topic question: why do you guys always do this instead of
>> doing, say, this:
>> INTERACTIVE=false
>> case $1 in
>> --interactive|-i)
>> INTERACTIVE=:
>> ... ;;
>> esac
>> if $INTERACTIVE; then
>> git_editor "$TODO" || die ...
>> fi
>
> Because in some shells 'false' is not a built-in.
Can you name such a shell? (besides Solaris' brain-damaged, b0rken
and foobared /bin/sh which will most likely not work with Git anyway)
> But then this might do it without the extra process:
>
> INTERACTIVE="! :" # false
`!' is not portable either. In particular, I highly doubt `!' will
work on shells that don't have `false' as a builtin (Hello Mr Solaris).
>
> case $1 in
> --interactive|-i)
> INTERACTIVE=:
> ... ;;
> esac
> if $INTERACTIVE; then
> git_editor "$TODO" || die ...
> fi
Correct me if I'm wrong but I think that some shells don't have
`test' as a builtin either.
--
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]
^ permalink raw reply
* Re: [PATCH] git-checkout: Test for relative path use.
From: David Symonds @ 2007-11-09 9:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Johannes Schindelin, Andreas Ericsson
In-Reply-To: <7vfxzfvlch.fsf@gitster.siamese.dyndns.org>
On Nov 9, 2007 8:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "David Symonds" <dsymonds@gmail.com> writes:
>
> > So what would you prefer? Bracketing the whole test in parentheses
> > looks ugly, but I can do that if that's the only option. If I look at
> > t5510-fetch.sh (one of yours, Junio), there is no directory
> > restoration in the case of test failure, as in my original patch.
>
> Yes, that is what I was referring to as "bad examples". The way
> t4116 goes down to different directory do not look ugly to me.
Okay, thanks -- that's a useful example. I'll resend the patch shortly.
Dave.
^ permalink raw reply
* Re: [PATCH 2/3] rev-list: Introduce --no-output to avoid /dev/null redirects
From: Shawn O. Pearce @ 2007-11-09 9:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alex Riesen, git
In-Reply-To: <7vsl3fvlrb.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> Alex Riesen <raa.lkml@gmail.com> writes:
> > Junio C Hamano, Fri, Nov 09, 2007 08:32:01 +0100:
> >>
> >> The function names noshow_xxx() looked a bit funny, but I do not
> >> offhand have better alternatives to offer.
> >
> > "hide", "skip", "ignore"?
>
> But look at what the functions do. The original show_xxx() was
> to print and then process. Shawn splitted them into show_xxx()
> and noshow_xxx(), leaft the printing part in the former, made
> the former call the latter at the end, and moved the processing
> to the latter. So it is not any of the three words.
In my latest patch series I've gone with "finish_xxx()" as it is
finishing our usage of that object. I'll be resubmitting a new
series soon.
--
Shawn.
^ permalink raw reply
* [PATCH] git-checkout: Test for relative path use.
From: David Symonds @ 2007-11-09 9:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Andreas Ericsson, Johannes Sixt,
David Symonds
In-Reply-To: <ee77f5c20711090110s5d6c533et5e1e016a95fde943@mail.gmail.com>
Signed-off-by: David Symonds <dsymonds@gmail.com>
---
t/t2008-checkout-subdir.sh | 80 ++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 80 insertions(+), 0 deletions(-)
create mode 100755 t/t2008-checkout-subdir.sh
diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh
new file mode 100755
index 0000000..98d8eb3
--- /dev/null
+++ b/t/t2008-checkout-subdir.sh
@@ -0,0 +1,80 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 David Symonds
+
+test_description='git checkout from subdirectories'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+ echo "base" > file0 &&
+ git add file0 &&
+ mkdir dir1 &&
+ echo "hello" > dir1/file1 &&
+ git add dir1/file1 &&
+ mkdir dir2 &&
+ echo "bonjour" > dir2/file2 &&
+ git add dir2/file2 &&
+ test_tick &&
+ git commit -m "populate tree"
+
+'
+
+test_expect_success 'remove and restore with relative path' '
+
+ (
+ cd dir1 &&
+ rm ../file0 &&
+ git checkout HEAD -- ../file0 &&
+ test "base" = "$(cat ../file0)" &&
+ rm ../dir2/file2 &&
+ git checkout HEAD -- ../dir2/file2 &&
+ test "bonjour" = "$(cat ../dir2/file2)" &&
+ rm ../file0 ./file1 &&
+ git checkout HEAD -- .. &&
+ test "base" = "$(cat ../file0)" &&
+ test "hello" = "$(cat file1)"
+ )
+
+'
+
+test_expect_success 'checkout with empty prefix' '
+
+ rm file0 &&
+ git checkout HEAD -- file0 &&
+ test "base" = "$(cat file0)"
+
+'
+
+test_expect_success 'checkout with simple prefix' '
+
+ rm dir1/file1 &&
+ git checkout HEAD -- dir1 &&
+ test "hello" = "$(cat dir1/file1)" &&
+ rm dir1/file1 &&
+ git checkout HEAD -- dir1/file1 &&
+ test "hello" = "$(cat dir1/file1)"
+
+'
+
+test_expect_success 'checkout with complex relative path' '
+
+ rm file1 &&
+ git checkout HEAD -- ../dir1/../dir1/file1 && test -f ./file1
+
+'
+
+test_expect_failure 'relative path outside tree should fail' \
+ 'git checkout HEAD -- ../../Makefile'
+
+test_expect_failure 'incorrect relative path to file should fail (1)' \
+ 'git checkout HEAD -- ../file0'
+
+test_expect_failure 'incorrect relative path should fail (2)' \
+ '( cd dir1 && git checkout HEAD -- ./file0 )'
+
+test_expect_failure 'incorrect relative path should fail (3)' \
+ '( cd dir1 && git checkout HEAD -- ../../file0 )'
+
+test_done
--
1.5.3.1
^ permalink raw reply related
* [PATCH] tests: git push mirror mode tests V2
From: Andy Whitcroft @ 2007-11-09 10:21 UTC (permalink / raw)
To: git
In-Reply-To: <1194541305.0@pinky>
Add some basic tests for git push --mirror mode.
Signed-off-by: Andy Whitcroft <apw@shadowen.org>
---
Following the discussion on how tests which change directory
should use subshells to prevent loss of CWD and of how
! is not something we can rely on, here is an updates to
the tests.
---
t/t5517-push-mirror.sh | 125 ++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 125 insertions(+), 0 deletions(-)
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
new file mode 100755
index 0000000..a65d2f5
--- /dev/null
+++ b/t/t5517-push-mirror.sh
@@ -0,0 +1,125 @@
+#!/bin/sh
+
+test_description='pushing to a mirror repository'
+
+. ./test-lib.sh
+
+D=`pwd`
+
+invert () {
+ if "$@"; then
+ return 1
+ else
+ return 0
+ fi
+}
+
+mk_repo_pair () {
+ rm -rf master mirror &&
+ mkdir mirror &&
+ (
+ cd mirror &&
+ git init
+ ) &&
+ mkdir master &&
+ (
+ cd master &&
+ git init &&
+ git config remote.up.url ../mirror
+ )
+}
+
+
+test_expect_success 'push mirror does not create new branches' '
+
+ mk_repo_pair &&
+ (
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --mirror up
+ ) &&
+ master_master=$(cd master && git show-ref -s --verify refs/heads/master) &&
+ mirror_master=$(cd mirror && git show-ref -s --verify refs/heads/master) &&
+ test "$master_master" = "$mirror_master"
+
+'
+
+test_expect_success 'push mirror does not update existing branches' '
+
+ mk_repo_pair &&
+ (
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --mirror up &&
+ echo two >foo && git add foo && git commit -m two &&
+ git push --mirror up
+ ) &&
+ master_master=$(cd master && git show-ref -s --verify refs/heads/master) &&
+ mirror_master=$(cd mirror && git show-ref -s --verify refs/heads/master) &&
+ test "$master_master" = "$mirror_master"
+
+'
+
+test_expect_success 'push mirror does not force update existing branches' '
+
+ mk_repo_pair &&
+ (
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --mirror up &&
+ echo two >foo && git add foo && git commit -m two &&
+ git push --mirror up &&
+ git reset --hard HEAD^
+ git push --mirror up
+ ) &&
+ master_master=$(cd master && git show-ref -s --verify refs/heads/master) &&
+ mirror_master=$(cd mirror && git show-ref -s --verify refs/heads/master) &&
+ test "$master_master" = "$mirror_master"
+
+'
+
+test_expect_success 'push mirror does not remove branches' '
+
+ mk_repo_pair &&
+ (
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git branch remove master &&
+ git push --mirror up &&
+ git branch -D remove
+ git push --mirror up
+ ) &&
+ (
+ cd mirror &&
+ invert git show-ref -s --verify refs/heads/remove
+ )
+
+'
+
+test_expect_success 'push mirror does not add, update and remove together' '
+
+ mk_repo_pair &&
+ (
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git branch remove master &&
+ git push --mirror up &&
+ git branch -D remove &&
+ git branch add master &&
+ echo two >foo && git add foo && git commit -m two &&
+ git push --mirror up
+ ) &&
+ master_master=$(cd master && git show-ref -s --verify refs/heads/master) &&
+ master_add=$(cd master && git show-ref -s --verify refs/heads/add) &&
+ mirror_master=$(cd mirror && git show-ref -s --verify refs/heads/master) &&
+ mirror_add=$(cd mirror && git show-ref -s --verify refs/heads/add) &&
+ test "$master_master" = "$mirror_master" &&
+ test "$master_add" = "$mirror_add" &&
+ (
+ cd mirror &&
+ invert git show-ref -s --verify refs/heads/remove
+ )
+
+'
+
+test_done
^ permalink raw reply related
* Re: [PATCH 1/2] Add strchrnul()
From: Andreas Ericsson @ 2007-11-09 10:22 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Pierre Habouzit, Git Mailing List,
Johannes Schindelin
In-Reply-To: <4733AEA0.1060602@lsrfire.ath.cx>
René Scharfe wrote:
> As suggested by Pierre Habouzit, add strchrnul(). It's a useful GNU
> extension and can simplify string parser code. There are several
> places in git that can be converted to strchrnul(); as a trivial
> example, this patch introduces its usage to builtin-fetch--tool.c.
>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
>
> Makefile | 13 +++++++++++++
> builtin-fetch--tool.c | 8 ++------
> compat/strchrnul.c | 8 ++++++++
> git-compat-util.h | 5 +++++
> 4 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 0d5590f..578c999 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -30,6 +30,8 @@ all::
> #
> # Define NO_MEMMEM if you don't have memmem.
> #
> +# Define NO_STRCHRNUL if you don't have strchrnul.
> +#
This seems overly complicated. How about this instead?
From: Andreas Ericsson <ae@op5.se>
Subject: [PATCH] Add strchrnul()
As suggested by Pierre Habouzit, add strchrnul(). It's a useful GNU
extension and can simplify string parser code. There are several
places in git that can be converted to strchrnul(); as a trivial
example, this patch introduces its usage to builtin-fetch--tool.c.
strchrnul() was introduced in glibc in April 1999 and included in
glibc-2.1. Checking for that version means the majority of all git
users would get to use the optimized version in glibc. Of the
remaining few some might get to use a slightly slower version
than necessary but probably not slower than what we have today.
Original patch by Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
Signed-off-by: Andreas Ericsson <ae@op5.se>
---
I'm fairly much against forcing people to know what library
functions they have in order to get software to compile
properly. This is, imo, a neater solution, and also inlines
the function as suggested by Dscho.
diff --git a/git-compat-util.h b/git-compat-util.h
index 7b29d1b..9fedf33 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -105,6 +105,18 @@ extern void set_die_routine(void (*routine)(const char *err
extern void set_error_routine(void (*routine)(const char *err, va_list params))
extern void set_warn_routine(void (*routine)(const char *warn, va_list params))
+
+#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
+# define strchrnul(s, c) gitstrchrnul(s, c)
+static inline char *gitstrchrnul(const char *s, int c)
+{
+ while (*s && *s != c)
+ s++;
+
+ return (char *)s;
+}
+#endif
+
#ifdef NO_MMAP
#ifndef PROT_READ
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 6a78517..ed60847 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -435,9 +435,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
cp++;
if (!*cp)
break;
- np = strchr(cp, '\n');
- if (!np)
- np = cp + strlen(cp);
+ np = strchrnul(cp, '\n');
if (pass) {
lrr_list[i].line = cp;
lrr_list[i].name = cp + 41;
@@ -461,9 +459,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
rref++;
if (!*rref)
break;
- next = strchr(rref, '\n');
- if (!next)
- next = rref + strlen(rref);
+ next = strchrnul(rref, '\n');
rreflen = next - rref;
for (i = 0; i < lrr_count; i++) {
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply related
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