* Re: [PATCH] git-checkout: Test for relative path use.
From: David Symonds @ 2007-11-09 1:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Andreas Ericsson
In-Reply-To: <7vtznwxl59.fsf@gitster.siamese.dyndns.org>
On Nov 9, 2007 12:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Symonds <dsymonds@gmail.com> writes:
>
> > Signed-off-by: David Symonds <dsymonds@gmail.com>
> > ---
> > Test 5 in this series fails because of a bug in git-ls-files, where
> > git-ls-files t/../
> > (with or without --full-name) returns no files.
>
> Heh, you shouldn't do that ;-)
>
> Seriously, that's a long standing limitation in the code, not to
> deal with arbitrary combination of ups and downs, but I do not
> think there is any fundamental reason to disallow something
> like:
>
> cd Documentation && git ls-files --full-name ../t
>
> Patches welcome.
So you're otherwise happy with my tests, despite one of them
triggering an (unrelated to git-checkout) bug? Or would you prefer I
remove that particular failure from the tests and resend?
Dave.
^ permalink raw reply
* Re: git rebase --skip
From: Johannes Schindelin @ 2007-11-08 23:01 UTC (permalink / raw)
To: Mike Hommey; +Cc: Daniel Barkalow, Björn Steinbrink, Jeff King, git
In-Reply-To: <20071108191601.GA22353@glandium.org>
Hi,
On Thu, 8 Nov 2007, Mike Hommey wrote:
> Maybe some commands such as commit should fail or at least emit warning
> when used during a rebase ?
No, that would disrupt comming (and valid!) workflow too much. Even a
warning.
Ciao,
Dscho
^ permalink raw reply
* Re: [BUG] git-rebase fails when a commit message contains a diff
From: Junio C Hamano @ 2007-11-09 1:51 UTC (permalink / raw)
To: Jonas Fonseca; +Cc: git
In-Reply-To: <7vlk98xkm3.fsf@gitster.siamese.dyndns.org>
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.
* There is no fundamental reason for rebase to use e-mail
format to express what "format-patch | am -3" pipeline does.
We do it currently because (1) it was expedient to reuse what
was already there, and because (2) the original target
audience of git are e-mail oriented communities, so there was
not strong incentive to make rebase independent of the
applymbox/mailinfo limitation (that is, even if you make
rebase able to handle such a patch, you cannot send out the
result over e-mail *anyway*).
This however does not mean we should always use merging
rebase. patch based approach "format-patch | am -3" pipeline
uses is often much faster. Instead of using "format-patch |
am -3", we could use more careful patch and message
generation, like git-rebase--interactive.sh:make_patch()
does.
^ permalink raw reply
* Re: [PATCH] git-checkout: Test for relative path use.
From: Junio C Hamano @ 2007-11-09 1:54 UTC (permalink / raw)
To: David Symonds; +Cc: Junio C Hamano, git, Johannes Schindelin, Andreas Ericsson
In-Reply-To: <ee77f5c20711081744p5d7b46fo88a582b9f5dbdab8@mail.gmail.com>
"David Symonds" <dsymonds@gmail.com> writes:
> On Nov 9, 2007 12:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Seriously, that's a long standing limitation in the code, not to
>> deal with arbitrary combination of ups and downs, but I do not
>> think there is any fundamental reason to disallow something
>> like:
>>
>> cd Documentation && git ls-files --full-name ../t
>>
>> Patches welcome.
>
> So you're otherwise happy with my tests, despite one of them
> triggering an (unrelated to git-checkout) bug? Or would you prefer I
> remove that particular failure from the tests and resend?
Are you really asking my preference? A patch to ls-files to
make the test pass is my preference, of course ;-).
Haven't read your tests, though, but I see capable people
already commented on the initial round so I do not expect it to
be problematic.
^ permalink raw reply
* Re: [PATCH] git-checkout: Test for relative path use.
From: David Symonds @ 2007-11-09 1:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Andreas Ericsson
In-Reply-To: <7vd4ukxjxn.fsf@gitster.siamese.dyndns.org>
On Nov 9, 2007 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Are you really asking my preference? A patch to ls-files to
> make the test pass is my preference, of course ;-).
>
> Haven't read your tests, though, but I see capable people
> already commented on the initial round so I do not expect it to
> be problematic.
I'm going after low-hanging fruit whilst I get familiar with Git's
internal structure. I can try to tackle this ls-files problem as a
separate thing.
Dave.
^ permalink raw reply
* Re: [BUG] git-rebase fails when a commit message contains a diff
From: Junio C Hamano @ 2007-11-09 2:18 UTC (permalink / raw)
To: Jonas Fonseca; +Cc: git, Johannes Schindelin
In-Reply-To: <7vhcjwxk1s.fsf@gitster.siamese.dyndns.org>
I wonder if this is a sensible thing to do, regardless of the
issue of commit log message that contains anything.
The patch replaces git-rebase with git-rebase--interactive. The
only difference from the existing "git-rebase -i" is if the
command is called without "-i" the initial "here is the to-do
list. please rearrange the lines, modify 'pick' to 'edit' or
whatever as appropriate" step is done without letting the user
edit the list.
---
Makefile | 2 +-
git-rebase--interactive.sh => git-rebase.sh | 14 ++++++++++----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/Makefile b/Makefile
index 0d5590f..f0e5e38 100644
--- a/Makefile
+++ b/Makefile
@@ -214,7 +214,7 @@ SCRIPT_SH = \
git-clean.sh git-clone.sh git-commit.sh \
git-ls-remote.sh \
git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
- git-pull.sh git-rebase.sh git-rebase--interactive.sh \
+ git-pull.sh git-rebase.sh \
git-repack.sh git-request-pull.sh \
git-sh-setup.sh \
git-am.sh \
diff --git a/git-rebase--interactive.sh b/git-rebase.sh
similarity index 98%
rename from git-rebase--interactive.sh
rename to git-rebase.sh
index 76dc679..1dd6f6d 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase.sh
@@ -11,7 +11,7 @@
# http://article.gmane.org/gmane.comp.version-control.git/22407
USAGE='(--continue | --abort | --skip | [--preserve-merges] [--verbose]
- [--onto <branch>] <upstream> [<branch>])'
+ [--interactive] [--onto <branch>] <upstream> [<branch>])'
. git-sh-setup
require_work_tree
@@ -25,6 +25,7 @@ REWRITTEN="$DOTEST"/rewritten
PRESERVE_MERGES=
STRATEGY=
VERBOSE=
+INTERACTIVE=
test -d "$REWRITTEN" && PRESERVE_MERGES=t
test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
test -f "$DOTEST"/verbose && VERBOSE=t
@@ -346,6 +347,9 @@ do_rest () {
while test $# != 0
do
case "$1" in
+ --interactive|-i)
+ INTERACTIVE=t
+ ;;
--continue)
comment_for_reflog continue
@@ -504,9 +508,11 @@ EOF
die_abort "Nothing to do"
cp "$TODO" "$TODO".backup
- git_editor "$TODO" ||
- die "Could not execute editor"
-
+ case "$INTERACTIVE" in
+ t)
+ git_editor "$TODO" || die "Could not execute editor"
+ ;;
+ esac
has_action "$TODO" ||
die_abort "Nothing to do"
^ permalink raw reply related
* Re: [BUG] git-rebase fails when a commit message contains a diff
From: Johannes Schindelin @ 2007-11-09 2:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonas Fonseca, git
In-Reply-To: <7v640cxitc.fsf@gitster.siamese.dyndns.org>
Hi,
On Thu, 8 Nov 2007, Junio C Hamano wrote:
> I wonder if this is a sensible thing to do, regardless of the issue of
> commit log message that contains anything.
>
> The patch replaces git-rebase with git-rebase--interactive. The only
> difference from the existing "git-rebase -i" is if the command is called
> without "-i" the initial "here is the to-do list. please rearrange the
> lines, modify 'pick' to 'edit' or whatever as appropriate" step is done
> without letting the user edit the list.
Hmm. I don't know, really. I had the impression that the "git
format-patch | git am" pipeline would be faster.
But if we were to do this, we'd get a progress meter for free. And bugs
exposed, no doubt.
> Makefile | 2 +-
> git-rebase--interactive.sh => git-rebase.sh | 14 ++++++++++----
> 2 files changed, 11 insertions(+), 5 deletions(-)
What about the existing git-rebase.sh?
> diff --git a/git-rebase--interactive.sh b/git-rebase.sh
> similarity index 98%
> rename from git-rebase--interactive.sh
> rename to git-rebase.sh
> index 76dc679..1dd6f6d 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase.sh
> @@ -346,6 +347,9 @@ do_rest () {
> while test $# != 0
> do
> case "$1" in
> + --interactive|-i)
> + INTERACTIVE=t
> + ;;
There is already a case for that, further down.
> @@ -504,9 +508,11 @@ EOF
> die_abort "Nothing to do"
>
> cp "$TODO" "$TODO".backup
> - git_editor "$TODO" ||
> - die "Could not execute editor"
> -
> + case "$INTERACTIVE" in
> + t)
> + git_editor "$TODO" || die "Could not execute editor"
> + ;;
> + esac
Would that not be easier to read as
test t = "$INTERACTIVE" &&
git_editor "$TODO" || die "Could not execute editor"
Hmm?
Ciao,
Dscho
^ permalink raw reply
* Re: [BUG] git-rebase fails when a commit message contains a diff
From: Junio C Hamano @ 2007-11-09 2:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jonas Fonseca, git
In-Reply-To: <Pine.LNX.4.64.0711090225110.4362@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Thu, 8 Nov 2007, Junio C Hamano wrote:
>
>> I wonder if this is a sensible thing to do, regardless of the issue of
>> commit log message that contains anything.
>>
>> The patch replaces git-rebase with git-rebase--interactive. The only
>> difference from the existing "git-rebase -i" is if the command is called
>> without "-i" the initial "here is the to-do list. please rearrange the
>> lines, modify 'pick' to 'edit' or whatever as appropriate" step is done
>> without letting the user edit the list.
>
> Hmm. I don't know, really. I had the impression that the "git
> format-patch | git am" pipeline would be faster.
Heh, I did not read rebase--interactive carefully enough.
Unless told to use merge with "rebase -m", rebase replays the
change by extracting and applying patches, and speed comparison
was about that vs merge based replaying; I thought make_patch
was done in order to avoid using cherry-pick (which is based on
merge-recursive) and doing patch application with three-way
fallback. Apparently that is not what "interactive" does.
Perhaps pick_one () could be taught to perform the 3-way
fallback dance git-am plays correctly. The patch I sent to make
git-rebase--interactive take over git-rebase would then become
quite reasonable, I would think.
^ permalink raw reply
* [PATCH 2/3] Make check-docs target detect removed commands
From: Junio C Hamano @ 2007-11-09 2:38 UTC (permalink / raw)
To: Jonas Fonseca; +Cc: Andreas Ericsson, Johannes Schindelin, git
In-Reply-To: <20071109002001.GB5082@diku.dk>
The maintainer should remember running "make check-docs" from
time to time.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Makefile | 30 +++++++++++++++++++++++++++---
1 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 0d5590f..d5445ea 100644
--- a/Makefile
+++ b/Makefile
@@ -1125,12 +1125,13 @@ endif
### Check documentation
#
check-docs::
- @for v in $(ALL_PROGRAMS) $(BUILT_INS) git$X gitk; \
+ @(for v in $(ALL_PROGRAMS) $(BUILT_INS) git gitk; \
do \
case "$$v" in \
git-merge-octopus | git-merge-ours | git-merge-recursive | \
- git-merge-resolve | git-merge-stupid | \
+ git-merge-resolve | git-merge-stupid | git-merge-subtree | \
git-add--interactive | git-fsck-objects | git-init-db | \
+ git-rebase--interactive | \
git-repo-config | git-fetch--tool ) continue ;; \
esac ; \
test -f "Documentation/$$v.txt" || \
@@ -1141,7 +1142,30 @@ check-docs::
git) ;; \
*) echo "no link: $$v";; \
esac ; \
- done | sort
+ done; \
+ ( \
+ sed -e '1,/^__DATA__/d' \
+ -e 's/[ ].*//' \
+ -e 's/^/listed /' Documentation/cmd-list.perl; \
+ ls -1 Documentation/git*txt | \
+ sed -e 's|Documentation/|documented |' \
+ -e 's/\.txt//'; \
+ ) | while read how cmd; \
+ do \
+ case "$$how,$$cmd" in \
+ *,git-citool | \
+ *,git-gui | \
+ documented,gitattributes | \
+ documented,gitignore | \
+ documented,gitmodules | \
+ documented,git-tools | \
+ sentinel,not,matching,is,ok ) continue ;; \
+ esac; \
+ case " $(ALL_PROGRAMS) $(BUILT_INS) git gitk " in \
+ *" $$cmd "*) ;; \
+ *) echo "removed but $$how: $$cmd" ;; \
+ esac; \
+ done ) | sort
### Make sure built-ins do not have dups and listed in git.c
#
--
1.5.3.5.1622.g41d10
^ permalink raw reply related
* Re: git rebase --skip
From: Junio C Hamano @ 2007-11-08 23:52 UTC (permalink / raw)
To: Jeff King; +Cc: Björn Steinbrink, Andreas Ericsson, Mike Hommey, git
In-Reply-To: <20071108231632.GC29840@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Personally, I don't see the point of a --force option; it turns your work
> flow from:
>
> 1. git-rebase --skip
> 2. Oops, I guess I have to reset.
> 3. git-reset --hard; git-rebase --skip
>
> to:
>
> 1. same as above
> 2. same as above
> 3. git-rebase --force --skip
I do not see it as improvement, either, for the same reason you
state.
> AIUI, Andreas's proposal is not so much DWIM as "do the obvious thing,
> but include a safety valve to prevent throwing away work." Is there
> actually a case where it would not have the desired effect?
The user is explicitly saying --skip, so I do not think it is
dangerous even if we unconditionally did "reset --hard" at that
point.
Or we could introduce a new option "--drop" (that's "drop the
current commit and continue") to do so, if people find that the
word "skip" does not sound like a scary destructive operation.
But I do not think that is needed.
^ permalink raw reply
* Re: [PATCH PARSEOPT 1/4] parse-options new features.
From: Junio C Hamano @ 2007-11-08 23:59 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <1194430832-6224-3-git-send-email-madcoder@debian.org>
Pierre Habouzit <madcoder@debian.org> writes:
> options flags:
> ~~~~~~~~~~~~~
> PARSE_OPT_NONEG allow the caller to disallow the negated option to exists.
Good addition; writing OPT_CALLBACK was tricky without this when
I tried to add --with=<commit> to git-branch.
s/to exists/to exist/
^ permalink raw reply
* Re: [PATCH 4/3] t3700: avoid racy git situation
From: Johannes Schindelin @ 2007-11-09 3:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, krh
In-Reply-To: <7vbqa431vj.fsf@gitster.siamese.dyndns.org>
Hi,
On Thu, 8 Nov 2007, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > The problem is that the index has the same timestamp as the file "foo".
> >
> > Therefore, git cannot tell if "foo" is up-to-date in the index, since
> > it could have been modified (and indeed is) just a fraction of a
> > second later than the index was last updated.
> >
> > And since diff-index, as called from the test script, does not
> > generate a diff, but really only determines if the index information
> > suggests that the files are up-to-date, there is not really much you
> > can do.
> >
> > This is our good old friend, the racy git problem.
>
> That sounds very wrong.
>
> What happened to the ce_smudge_racily_clean_entry() call that is
> done from write_index()?
And sure enough I am wrong. My patch assumes that it was the second
diff-index which failed, the one after "git add --refresh".
Alas, in shell "a && b && c && d || e" will execute e if a fails. So
after debugging this a bit more carefully, it seems that it is indeed the
missing update_index --refresh that Kristian mentioned, which leads to
this error.
And my patch is wrong.
Ciao,
Dscho
^ permalink raw reply
* Re: git rebase --skip
From: Jeff King @ 2007-11-09 3:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Björn Steinbrink, Andreas Ericsson, Mike Hommey, git
In-Reply-To: <7vir4cz45z.fsf@gitster.siamese.dyndns.org>
On Thu, Nov 08, 2007 at 03:52:08PM -0800, Junio C Hamano wrote:
> The user is explicitly saying --skip, so I do not think it is
> dangerous even if we unconditionally did "reset --hard" at that
> point.
Sure, I think the complaint is not that "reset --hard" is the wrong
behavior, but that people are prone to type --skip in error. Right now
we handle that error in a data-preserving way (we complain, and the user
has to think and issue a "throw away this data" command), but automatic
reset is less safe (even though there are fewer times when somebody
meant to commit instead of just reset, the consequences are harder to
recover from).
I've never personally run into this, but I think it is a reasonable
thing to think about, and if it is easy to add an additional safety
valve (either stashing the index/wt state, or checking before automatic
"reset --hard" whether any work has been done towards resolving), then
we probably should.
So I am fine with the original patch (unconditional reset --hard), but
it would be nice to see the people who care submit concrete proposals
for such a safety valve.
> Or we could introduce a new option "--drop" (that's "drop the
> current commit and continue") to do so, if people find that the
> word "skip" does not sound like a scary destructive operation.
I don't think the problem is "users don't realize how scary --skip can
be", but rather "I use --skip to resolve this situation 99% of the time,
so in the other 1%, I soetimes use it accidentally."
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] Add strchrnul()
From: Jeff King @ 2007-11-09 3:31 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>
On Fri, Nov 09, 2007 at 01:49:36AM +0100, 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.
Minor nit: a comment (in the code or even in the commit message)
describing the what the function does would be nice. On a GNU system,
one can look at the man page, but on others there is no indication of
what it does except for reading the code (and maybe the answer is "this
is a common library function, you idiot", but I am familiar with many
GNU extensions and didn't know this one).
-Peff
^ permalink raw reply
* [PATCH] stop t1400 hiding errors in tests
From: Alex Riesen @ 2007-11-08 23:41 UTC (permalink / raw)
To: git; +Cc: Kristian Høgsberg, Junio C Hamano
In-Reply-To: <20071108232751.GC4899@steel.home>
The last rm in the test was lacking an "&&" before it,
which caused the errors in the commands be silently hidden.
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Alex Riesen, Fri, Nov 09, 2007 00:27:52 +0100:
> Kristian Høgsberg, Thu, Nov 08, 2007 17:59:00 +0100:
> > This makes git commit a builtin and moves git-commit.sh to
>
> Applied instead of 00c8febf563da on Junio's pu it breaks t1400:
>
> * FAIL 29: git-commit logged updates
> diff expect .git/logs/refs/heads/master
>
>
> Which is not the test actually failed. The failed one is 28, but the
> last "rm -f M" killed the error because of missed "&&" before it.
just separated the fix into a patch of its own. Johannes already took
care of the problem with launch_editor not reading the message file.
t/t1400-update-ref.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index ce045b2..a90824b 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -205,7 +205,7 @@ test_expect_success \
echo $h_TEST >.git/MERGE_HEAD &&
GIT_AUTHOR_DATE="2005-05-26 23:45" \
GIT_COMMITTER_DATE="2005-05-26 23:45" git-commit -F M &&
- h_MERGED=$(git rev-parse --verify HEAD)
+ h_MERGED=$(git rev-parse --verify HEAD) &&
rm -f M'
cat >expect <<EOF
--
1.5.3.5.626.g8f33b
^ permalink raw reply related
* Re: [PATCH 2/3] rev-list: Introduce --no-output to avoid /dev/null redirects
From: Alex Riesen @ 2007-11-08 23:44 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20071108080052.GB16690@spearce.org>
Shawn O. Pearce, Thu, Nov 08, 2007 09:00:52 +0100:
> Some uses of git-rev-list are to run it with --objects to see if
> a range of objects between two or more commits is fully connected
> or not. In such a case the caller doesn't care about the actual
> object names or hash hints so formatting this data only for it to
> be dumped to /dev/null by a redirect is a waste of CPU time. If
> all the caller needs is the exit status then --no-output can be
> used to bypass the commit and object formatting.
We already have --quiet and even --exit-code.
^ permalink raw reply
* 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
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