* [PATCH v2 1/3] contrib/subtree: Store subtree sources in .gitsubtree and use for push/pull
From: Paul Campbell @ 2013-02-15 22:42 UTC (permalink / raw)
To: git
Cc: Adam Tkac, David A. Greene, Jesper L. Nielsen, Michael Schubert,
Techlive Zheng
Add the prefix, repository and refspec in the file .gitsubtree when
git subtree add is used. Then when a git subtree push or pull is needed
the repository and refspec from .gitsubtree are used as the default
values.
Having to remember what subtree came from what source is a waste of
developer memory and doesn't transfer easily to other developers.
git subtree push/pull operations would typically be to/from the same
source that the original subtree was cloned from with git subtree add.
The <repository> and <refspec>, or <commit>, used in the git subtree add
operation are stored in .gitsubtree. One line each, delimited by a space:
"<prefix> <repository> <refspec>" or "<prefix> . <commit>".
Subsequent git subtree push/pull operations now default to the values
stored in .gitsubtree, unless overridden from the command line.
The .gitsubtree file should be tracked as part of the repo as it
describes where parts of the tree came from and can be used to update
to and from that source.
Signed-off-by: Paul Campbell <pcampbell@kemitix.net>
---
Reworked my previous patch paying closer attention to the coding style
documentation and renamed my new functions to make more sense.
contrib/subtree/git-subtree.sh | 75 +++++++++++++++++++++++++++++++++++-------
1 file changed, 64 insertions(+), 11 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 51146bd..6dc8999 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -11,8 +11,8 @@ OPTS_SPEC="\
git subtree add --prefix=<prefix> <commit>
git subtree add --prefix=<prefix> <repository> <commit>
git subtree merge --prefix=<prefix> <commit>
-git subtree pull --prefix=<prefix> <repository> <refspec...>
-git subtree push --prefix=<prefix> <repository> <refspec...>
+git subtree pull --prefix=<prefix> [<repository> <refspec...>]
+git subtree push --prefix=<prefix> [<repository> <refspec...>]
git subtree split --prefix=<prefix> <commit...>
--
h,help show the help
@@ -489,6 +489,28 @@ ensure_clean()
fi
}
+add_subtree () {
+ if ( grep "^$dir " .gitsubtree )
+ then
+ # remove $dir from .gitsubtree - there's probably a clever way to
do this with sed
+ grep -v "^$dir " .gitsubtree > .gitsubtree.temp
+ rm .gitsubtree
+ mv .gitsubtree.temp .gitsubtree
+ fi
+ if test $# -eq 1
+ then
+ # Only a commit provided, thus use the current repository
+ echo "$dir . $@" >> .gitsubtree
+ elif test $# -eq 2
+ then
+ echo "$dir $@" >> .gitsubtree
+ fi
+}
+
+get_subtree () {
+ grep "^$dir " .gitsubtree || die "Subtree not found in .gitsubtree: " "$dir"
+}
+
cmd_add()
{
if [ -e "$dir" ]; then
@@ -497,6 +519,8 @@ cmd_add()
ensure_clean
+ add_subtree "$@"
+
if [ $# -eq 1 ]; then
git rev-parse -q --verify "$1^{commit}" >/dev/null ||
die "'$1' does not refer to a commit"
@@ -700,7 +724,23 @@ cmd_merge()
cmd_pull()
{
ensure_clean
- git fetch "$@" || exit $?
+ if test $# -eq 0
+ then
+ subtree=($(get_subtree))
+ repository=${subtree[1]}
+ refspec=${subtree[2]}
+ if test "$repository" == "."
+ then
+ echo "Pulling into $dir from branch $refspec"
+ else
+ echo "Pulling into '$dir' from '$repository' '$refspec'"
+ fi
+ echo "git fetch using: " $repository $refspec
+ git fetch "$repository" "$refspec" || exit $?
+ else
+ echo "git fetch using: $@"
+ git fetch "$@" || exit $?
+ fi
revs=FETCH_HEAD
set -- $revs
cmd_merge "$@"
@@ -708,16 +748,29 @@ cmd_pull()
cmd_push()
{
- if [ $# -ne 2 ]; then
- die "You must provide <repository> <refspec>"
+ repository=$1
+ refspec=$2
+ if test $# -eq 0
+ then
+ subtree=($(get_subtree))
+ repository=${subtree[1]}
+ refspec=${subtree[2]}
+ if test "$repository" == "."
+ then
+ echo "Pushing from $dir into branch $refspec"
+ else
+ echo "Pushing from $dir into $repository $refspec"
+ fi
+ elif test $# -ne 2
+ then
+ die "You must provide <repository> <refspec>, or a <prefix> listed
in .gitsubtree"
fi
- if [ -e "$dir" ]; then
- repository=$1
- refspec=$2
- echo "git push using: " $repository $refspec
- git push $repository $(git subtree split
--prefix=$prefix):refs/heads/$refspec
+ if test -e "$dir"
+ then
+ echo "git push using: " $repository $refspec
+ git push $repository $(git subtree split
--prefix=$prefix):refs/heads/$refspec
else
- die "'$dir' must already exist. Try 'git subtree add'."
+ die "'$dir' must already exist. Try 'git subtree add'."
fi
}
--
1.8.1.3.605.g02339dd
^ permalink raw reply related
* [PATCH 2/3] contrib/subtree/t: Added tests for .gitsubtree support
From: Paul Campbell @ 2013-02-15 22:50 UTC (permalink / raw)
To: git
Cc: Adam Tkac, David A. Greene, Jesper L. Nielsen, Michael Schubert,
Techlive Zheng
add: ensure details are added to .gitsubtree
push: check for a SHA1 update line
pull: add a file on one subtree, push it to a branch, then pull into
another subtree containing the same branch and confirm the files match
add: ensure stale .gitsubtree entry is replaced
Signed-off-by: Paul Campbell <pcampbell@kemitix.net>
---
contrib/subtree/t/t7900-subtree.sh | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/contrib/subtree/t/t7900-subtree.sh
b/contrib/subtree/t/t7900-subtree.sh
index 80d3399..4437dc6 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -465,4 +465,34 @@ test_expect_success 'verify one file change per commit' '
))
'
+# return to mainline
+cd ../..
+
+# .gitsubtree
+test_expect_success 'added repository appears in .gitsubtree' '
+ git subtree add --prefix=copy0 sub1 &&
+ grep "^copy0 \. sub1$" .gitsubtree
+'
+
+test_expect_success 'change in subtree is pushed okay' '
+ cd copy0 && create new_file && git commit -m"Added new_file" &&
+ cd .. && git subtree push --prefix=copy0 2>&1 | \
+ grep
"^\s\{3\}[0-9a-f]\{7\}\.\.[0-9a-f]\{7\}\s\s[0-9a-f]\{40\}\s->\ssub1$"
+'
+
+test_expect_success 'pull into subtree okay' '
+ git subtree add --prefix=copy1 sub1 &&
+ git subtree add --prefix=copy2 sub1 &&
+ cd copy1 && create new_file_in_copy1 && git commit -m"Added
new_file_in_copy1" &&
+ cd .. && git subtree push --prefix=copy1 &&
+ git subtree pull --prefix=copy2 | grep "^ create mode 100644
copy2/new_file_in_copy1$"
+'
+
+test_expect_success 'replace outdated entry in .gitsubtree' '
+ echo "copy3 . sub2" >> .gitsubtree &&
+ git subtree add --prefix=copy3 sub1 &&
+ (grep "^copy3 . sub2$" .gitsubtree && die || true) &&
+ grep "^copy3 . sub1$" .gitsubtree
+'
+
test_done
--
1.8.1.3.605.g02339dd
^ permalink raw reply related
* [PATCH 3/3] contrib/subtree: update documentation
From: Paul Campbell @ 2013-02-15 22:54 UTC (permalink / raw)
To: git
Cc: Adam Tkac, David A. Greene, Jesper L. Nielsen, Michael Schubert,
Techlive Zheng
Indicate that repository and refspec are now optional on push and pull.
Add notes to add, push and pull about storing values in .gitsubtree
and their use as default values.
Signed-off-by: Paul Campbell <pcampbell@kemitix.net>
---
contrib/subtree/git-subtree.txt | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/contrib/subtree/git-subtree.txt b/contrib/subtree/git-subtree.txt
index 7ba853e..2ad9278 100644
--- a/contrib/subtree/git-subtree.txt
+++ b/contrib/subtree/git-subtree.txt
@@ -11,8 +11,8 @@ SYNOPSIS
[verse]
'git subtree' add -P <prefix> <refspec>
'git subtree' add -P <prefix> <repository> <refspec>
-'git subtree' pull -P <prefix> <repository> <refspec...>
-'git subtree' push -P <prefix> <repository> <refspec...>
+'git subtree' pull -P [<prefix> <repository> <refspec...>]
+'git subtree' push -P [<prefix> <repository> <refspec...>]
'git subtree' merge -P <prefix> <commit>
'git subtree' split -P <prefix> [OPTIONS] [<commit>]
@@ -72,7 +72,9 @@ add::
A new commit is created automatically, joining the imported
project's history with your own. With '--squash', imports
only a single commit from the subproject, rather than its
- entire history.
+ entire history. Details of the <prefix> are added to the
+ .gitsubtree file, where they will be used as defaults for
+ 'push' and 'pull'.
merge::
Merge recent changes up to <commit> into the <prefix>
@@ -91,13 +93,16 @@ merge::
pull::
Exactly like 'merge', but parallels 'git pull' in that
it fetches the given commit from the specified remote
- repository.
+ repository. Default values for <repository> and <refspec>
+ are taken from the .gitsubtree file.
push::
Does a 'split' (see below) using the <prefix> supplied
and then does a 'git push' to push the result to the
repository and refspec. This can be used to push your
subtree to different branches of the remote repository.
+ Default values for <repository> and <refspec> are taken
+ from the .gitsubtree file.
split::
Extract a new, synthetic project history from the
--
1.8.1.3.605.g02339dd
^ permalink raw reply related
* Re: [PATCH 2/3] contrib/subtree/t: Added tests for .gitsubtree support
From: Jonathan Nieder @ 2013-02-15 22:56 UTC (permalink / raw)
To: Paul Campbell
Cc: git, Adam Tkac, David A. Greene, Jesper L. Nielsen,
Michael Schubert, Techlive Zheng
In-Reply-To: <CALeLG_=ir-kBTYpsRr_Hf8q2UY2ZtjShbTkO_tH=YiWSskfPOw@mail.gmail.com>
Hi Paul,
Paul Campbell wrote:
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -465,4 +465,34 @@ test_expect_success 'verify one file change per commit' '
[...]
> +test_expect_success 'change in subtree is pushed okay' '
> + cd copy0 && create new_file && git commit -m"Added new_file" &&
> + cd .. && git subtree push --prefix=copy0 2>&1 | \
If it possible to restrict the chdirs to subshells, that can make the
test more resiliant to early failures without breaking later tests.
That is:
(
cd copy0 &&
create new_file &&
test_tick &&
git commit -m "add new_file"
) &&
git subtree push --prefix=copy0 >output 2>&1 &&
grep "..." output
> + grep "^\s\{3\}[0-9a-f]\{7\}\.\.[0-9a-f]\{7\}\s\s[0-9a-f]\{40\}\s->\ssub1$"
This might not be portable if I understand
Documentation/CodingGuidelines correctly.
[...]
> + (grep "^copy3 . sub2$" .gitsubtree && die || true) &&
! grep "^copy3 . sub2\$" .gitsubtree &&
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH 2/3] contrib/subtree/t: Added tests for .gitsubtree support
From: Paul Campbell @ 2013-02-15 23:16 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Adam Tkac, David A. Greene, Jesper L. Nielsen,
Michael Schubert, Techlive Zheng
In-Reply-To: <20130215225624.GB21165@google.com>
Hi Jonathan,
On Fri, Feb 15, 2013 at 10:56 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi Paul,
>
> Paul Campbell wrote:
>
>> --- a/contrib/subtree/t/t7900-subtree.sh
>> +++ b/contrib/subtree/t/t7900-subtree.sh
>> @@ -465,4 +465,34 @@ test_expect_success 'verify one file change per commit' '
> [...]
>> +test_expect_success 'change in subtree is pushed okay' '
>> + cd copy0 && create new_file && git commit -m"Added new_file" &&
>> + cd .. && git subtree push --prefix=copy0 2>&1 | \
>
> If it possible to restrict the chdirs to subshells, that can make the
> test more resiliant to early failures without breaking later tests.
>
> That is:
>
> (
> cd copy0 &&
> create new_file &&
> test_tick &&
> git commit -m "add new_file"
> ) &&
> git subtree push --prefix=copy0 >output 2>&1 &&
> grep "..." output
>
Adding them in.
>> + grep "^\s\{3\}[0-9a-f]\{7\}\.\.[0-9a-f]\{7\}\s\s[0-9a-f]\{40\}\s->\ssub1$"
>
> This might not be portable if I understand
> Documentation/CodingGuidelines correctly.
>
And it's ugly. But I believe it fits the "don't use grep -E"
condition. Unless I missed something else.
Is there was a better way to verify that the push operation succeeds
then grepping for a SHA1?
> [...]
>> + (grep "^copy3 . sub2$" .gitsubtree && die || true) &&
>
> ! grep "^copy3 . sub2\$" .gitsubtree &&
>
> Hope that helps,
> Jonathan
Thanks. That's a much neater way to do it.
--
Paul [W] Campbell
^ permalink raw reply
* Re: [PATCH] contrib/subtree: remove contradicting use options on echo wrapper
From: Paul Campbell @ 2013-02-15 23:18 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Adam Tkac, David A. Greene, Jesper L. Nielsen,
Michael Schubert, Techlive Zheng
In-Reply-To: <7vtxpdfbhx.fsf@alter.siamese.dyndns.org>
On Fri, Feb 15, 2013 at 10:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Campbell <pcampbell@kemitix.net> writes:
>
>> Remove redundant -n option and raw ^M in call to echo.
>>
>> Call to 'say' function, a wrapper of 'echo', passed the parameter -n, then
>> included a raw ^M newline in the end of the last parameter. Yet the -n option
>> is meant to suppress the addition of new line by echo.
>>
>> Signed-off-by: Paul Campbell <pcampbell@kemitix.net>
>
> I generally do not comment on comment on contrib/ material, and I am
> not familiar with subtree myself, but
>
> for count in $(seq 0 $total)
> do
> echo -n "$count/$total^M"
> ... do heavy lifting ...
> done
> echo "Done "
>
> is an idiomatic way to implement a progress meter without scrolling
> more important message you gave earlier to the user before entering
> the loop away. The message appears, carrige-return moves the cursor
> to the beginning of the line without going to the next line, and the
> next iteration overwrites the previous count. Finally, the progress
> meter is overwritten with the "Done" message. Alternatively you can
> wrap it up with
>
> echo
> echo Done
>
> if you want to leave the final progress "100/100" before saying "Done."
>
> Isn't that what this piece of code trying to do?
>
>> ---
>> contrib/subtree/git-subtree.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index 8a23f58..51146bd 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -592,7 +592,7 @@ cmd_split()
>> eval "$grl" |
>> while read rev parents; do
>> revcount=$(($revcount + 1))
>> - say -n "$revcount/$revmax ($createcount)
>> "
>> + say "$revcount/$revmax ($createcount)"
>> debug "Processing commit: $rev"
>> exists=$(cache_get $rev)
>> if [ -n "$exists" ]; then
[Apologies for resending this Junio. Forgot to hit reply all.]
Ah. I've not seen that done in shell before. In other languages I've
seen and used '\r' for this purpose, rather than a raw ^M.
I was getting frustrated with it as my apparently braindead text
editor was converting it to a normal unix newline, which would then
keep getting picked up by git diff.
Please ignore my patch.
--
Paul [W] Campbell
^ permalink raw reply
* Re: [PATCH v4+ 3/4] count-objects: report garbage files in pack directory too
From: Junio C Hamano @ 2013-02-15 23:20 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1360930030-21211-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> prepare_packed_git_one() is modified to allow count-objects to hook a
> report function to so we don't need to duplicate the pack searching
> logic in count-objects.c. When report_pack_garbage is NULL, the
> overhead is insignificant.
>
> The garbage is reported with warning() instead of error() in packed
> garbage case because it's not an error to have garbage. Loose garbage
> is still reported as errors and will be converted to warnings later.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
Will replace the one from the other day and advance the topic to 'next'.
Thanks.
^ permalink raw reply
* supports diff.context config for git-diff-tree
From: 乙酸鋰 @ 2013-02-15 23:52 UTC (permalink / raw)
To: git
Dear Sir,
In git 1.8.1, git-diff supports diff.context config.
However, git-diff-tree does not support this.
Could you also add this to git-diff-tree?
Regards,
ch3cooli
^ permalink raw reply
* Re: supports diff.context config for git-diff-tree
From: Junio C Hamano @ 2013-02-16 0:00 UTC (permalink / raw)
To: 乙酸鋰; +Cc: git
In-Reply-To: <CAHtLG6TPs=Z2i8s3_dd_igztuvuqE5L93cTtBM4q1zDCzpU55w@mail.gmail.com>
乙酸鋰 <ch3cooli@gmail.com> writes:
> In git 1.8.1, git-diff supports diff.context config.
> However, git-diff-tree does not support this.
> Could you also add this to git-diff-tree?
That's more or less deliberate, isn't it?
Cosmetic details of the output from plumbing commands should not be
affected by random configuration variables the user happens to have
and break the assumption your script that reads their output makes.
If your custom Porcelain that is built using the plumbing commands
wants to honor such UI level configuration variables, "git config"
is there for that exact purpose.
^ permalink raw reply
* Re: [BUG] Git clone of a bundle fails, but works (somewhat) when run with strace
From: Alain Kalker @ 2013-02-16 0:03 UTC (permalink / raw)
To: git
In-Reply-To: <kfmclb$4ro$2@ger.gmane.org>
On Fri, 15 Feb 2013 22:25:47 +0000, Alain Kalker wrote:
> On Fri, 15 Feb 2013 20:33:24 +0100, Alain Kalker wrote:
>
>> tl;dr:
>>
>> - `git bundle create` without <git-rev-list-args> gives git rev-list
>> help, then dies.
>> Should point out missing <git-rev-list-args> instead.
>> - `git clone <bundle> <dir> gives "ERROR: Repository not found."
>> - `strace ... git clone <bundle> <dir>` (magically) appears to work but
>> cannot checkout files b/c of nonexistent ref.
>> - Heisenbug? Race condition?
>> - Zaphod Beeblebrox has left the building, sulking.
>>
>> Full description:
>>
>> When I try to clone from a bundle created from a local repository, `git
>> clone <bundle> <dir>` fails with: "ERROR: Repository not found. fatal:
>> Could not read from remote repository." unless I run it with strace.
After trying to bisect this using `bisect start; bisect good v1.5.1; git
bisect bad HEAD; git bisect run ..test.sh`:
---test.sh---
#!/bin/sh
make clean
make || return 125
GIT=$(pwd)/git
cd /tmp
rm -rf testrepo
mkdir testrepo
cd testrepo
$GIT init
echo test > test.txt
$GIT add test.txt
$GIT commit -m "Add test.txt"
$GIT bundle create ../testrepo.bundle master || return 125
cd ..
rm -rf testrepofrombundle
$GIT clone testrepo.bundle testrepofrombundle || return 1
---
I was unable to find a bad revision.
After a lot more searching I found that I had `git` aliased to `hub`, a
tool used to make Github actions easier.
Eliminating `hub` from the equation resolved most problems.
The only ones remaining are the confusing error message from `git bundle
create` and the "missing HEAD" (you can interpret that in different
ways) ;-)
P.S. I hereby promise to _never_ _ever_ alias `git` to something else and
then post a Git bug about that "something else" on this ML.
Sorry to have wasted your time,
Alain
^ permalink raw reply
* Re: [BUG] Git clone of a bundle fails, but works (somewhat) when run with strace
From: Junio C Hamano @ 2013-02-16 0:09 UTC (permalink / raw)
To: Alain Kalker; +Cc: git
In-Reply-To: <kfmide$4ro$3@ger.gmane.org>
Alain Kalker <a.c.kalker@gmail.com> writes:
> P.S. I hereby promise to _never_ _ever_ alias `git` to something else and
> then post a Git bug about that "something else" on this ML.
>
> Sorry to have wasted your time,
Thanks.
People around here tend to be quiet until they sufficiently have dug
the issue themselves; unless the initial report grossly lack
necessary level of details, you may not hear "does not reproduce for
me" for some time, so some may still have been scratching their
head, and your honestly following-up on your message will save time
for them. Thanks again, and happy Gitting ;-)
^ permalink raw reply
* Re: [PATCH] read_directory: avoid invoking exclude machinery on tracked files
From: Duy Nguyen @ 2013-02-16 3:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Karsten Blees, kusmabite, Ramkumar Ramachandra, Robert Zeh,
finnag
In-Reply-To: <7vd2w1gyok.fsf@alter.siamese.dyndns.org>
On Sat, Feb 16, 2013 at 2:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> If you consider read_directory_recursive alone, there is a regression.
>> The return value of r_d_r depends on path_handled/path_ignored. With
>> this patch, the return value will be different.
>
> That is exactly what was missing from the proposed log message, and
> made me ask "Do all the callers that reach this function in their
> callgraph, when they get path_ignored for a path in the index,
> behave as if the difference between path_ignored and path_handled
> does not matter?"
I'll add it the the log message. Although I'm thinking some
restructuring to separate tracked file handling from the rest may make
it clearer (and less error prone in future).
--
Duy
^ permalink raw reply
* Re: [BUG] Git clone of a bundle fails, but works (somewhat) when run with strace
From: Jeff King @ 2013-02-16 4:01 UTC (permalink / raw)
To: Alain Kalker; +Cc: git
In-Reply-To: <kfmide$4ro$3@ger.gmane.org>
On Sat, Feb 16, 2013 at 12:03:58AM +0000, Alain Kalker wrote:
> ---test.sh---
> #!/bin/sh
>
> make clean
> make || return 125
> GIT=$(pwd)/git
>
> cd /tmp
> rm -rf testrepo
> mkdir testrepo
> cd testrepo
> $GIT init
> echo test > test.txt
> $GIT add test.txt
> $GIT commit -m "Add test.txt"
> $GIT bundle create ../testrepo.bundle master || return 125
> cd ..
>
> rm -rf testrepofrombundle
> $GIT clone testrepo.bundle testrepofrombundle || return 1
> ---
> I was unable to find a bad revision.
> After a lot more searching I found that I had `git` aliased to `hub`, a
> tool used to make Github actions easier.
> Eliminating `hub` from the equation resolved most problems.
Great.
> The only ones remaining are the confusing error message from `git bundle
> create` and the "missing HEAD" (you can interpret that in different
> ways) ;-)
I do not see any odd message from "bundle create" in the recipe above.
Mine says:
$ git bundle create ../repo.bundle master
Counting objects: 3, done.
Writing objects: 100% (3/3), 209 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
What you _might_ be seeing is the fact that the invocation above is
likely to be running two different versions of git under the hood. "git
bundle" will invoke "git rev-list", and it will use the first git in
your PATH, even if it is not $GIT. The proper way to test an un-installed
version of git is to use $YOUR_GIT_BUILD/bin-wrappers/git, which will
set up environment variables sufficient to make sure all sub-gits are
from the same version. Sometimes mixing versions can have weird results
(e.g., the new "git bundle" expects "rev-list" to have a particular
option, but the older version does not have it).
Secondly, I do get the same warning about HEAD:
$ git clone repo.bundle repofrombundle
Cloning into 'repofrombundle'...
Receiving objects: 100% (3/3), done.
warning: remote HEAD refers to nonexistent ref, unable to checkout.
but that warning makes sense. You did not create a bundle that contains
HEAD, therefore when we clone it, we do not know what to point HEAD to.
You probably wanted "git bundle create ../repo.bundle --all" which
includes both "master" and "HEAD".
It would be slightly more accurate to say "the remote HEAD does not
exist", rather than "refers to nonexistent ref". It would perhaps be
nicer still for "git clone" to make a guess about the correct HEAD when
one is not present (especially in the single-branch case, it is easy to
make the right guess).
Patches welcome. In the meantime, you can clone with "-b master" to tell
it explicitly, or you can "git checkout master" inside the newly-cloned
repository.
-Peff
^ permalink raw reply
* [PATCH 1/4] difftool: silence uninitialized variable warning
From: David Aguilar @ 2013-02-16 5:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Git::config() returns `undef` when given keys that do not exist.
Check that the $guitool value is defined to prevent a noisy
"Use of uninitialized variable $guitool in length" warning.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-difftool.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 0a90de4..12231fb 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -336,7 +336,7 @@ sub main
}
if ($opts{gui}) {
my $guitool = Git::config('diff.guitool');
- if (length($guitool) > 0) {
+ if (defined($guitool) && length($guitool) > 0) {
$ENV{GIT_DIFF_TOOL} = $guitool;
}
}
--
1.8.1.3.623.g622c8fc
^ permalink raw reply related
* [PATCH 2/4] t7800: Update copyright notice
From: David Aguilar @ 2013-02-16 5:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1360993666-81308-1-git-send-email-davvid@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
t/t7800-difftool.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index eb1d3f8..bb3158a 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -1,6 +1,6 @@
#!/bin/sh
#
-# Copyright (c) 2009, 2010 David Aguilar
+# Copyright (c) 2009, 2010, 2012 David Aguilar
#
test_description='git-difftool
--
1.8.1.3.623.g622c8fc
^ permalink raw reply related
* [PATCH 3/4] t7800: modernize tests
From: David Aguilar @ 2013-02-16 5:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1360993666-81308-2-git-send-email-davvid@gmail.com>
Eliminate a lot of redundant work by using test_config().
Chain everything together by using sane_unset() and a
simpler difftool_test_setup().
The original tests relied upon restore_test_defaults()
from the previous test to provide the next test with a sane
environment. Make the tests do their own setup so that they
are not dependent on the success of the previous test.
The end result is shorter tests and better test isolation.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
t/t7800-difftool.sh | 160 +++++++++++++++++++++-------------------------------
1 file changed, 63 insertions(+), 97 deletions(-)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb3158a..2d1ba8d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -10,29 +10,11 @@ Testing basic diff tool invocation
. ./test-lib.sh
-remove_config_vars()
+difftool_test_setup()
{
- # Unset all config variables used by git-difftool
- git config --unset diff.tool
- git config --unset diff.guitool
- git config --unset difftool.test-tool.cmd
- git config --unset difftool.prompt
- git config --unset merge.tool
- git config --unset mergetool.test-tool.cmd
- git config --unset mergetool.prompt
- return 0
-}
-
-restore_test_defaults()
-{
- # Restores the test defaults used by several tests
- remove_config_vars
- unset GIT_DIFF_TOOL
- unset GIT_DIFFTOOL_PROMPT
- unset GIT_DIFFTOOL_NO_PROMPT
- git config diff.tool test-tool &&
- git config difftool.test-tool.cmd 'cat $LOCAL'
- git config difftool.bogus-tool.cmd false
+ test_config diff.tool test-tool &&
+ test_config difftool.test-tool.cmd 'cat $LOCAL' &&
+ test_config difftool.bogus-tool.cmd false
}
prompt_given()
@@ -65,36 +47,33 @@ test_expect_success PERL 'setup' '
# Configure a custom difftool.<tool>.cmd and use it
test_expect_success PERL 'custom commands' '
- restore_test_defaults &&
- git config difftool.test-tool.cmd "cat \$REMOTE" &&
-
+ difftool_test_setup &&
+ test_config difftool.test-tool.cmd "cat \$REMOTE" &&
diff=$(git difftool --no-prompt branch) &&
test "$diff" = "master" &&
- restore_test_defaults &&
+ test_config difftool.test-tool.cmd "cat \$LOCAL" &&
diff=$(git difftool --no-prompt branch) &&
test "$diff" = "branch"
'
# Ensures that a custom difftool.<tool>.cmd overrides built-ins
test_expect_success PERL 'custom commands override built-ins' '
- restore_test_defaults &&
- git config difftool.defaults.cmd "cat \$REMOTE" &&
+ test_config difftool.defaults.cmd "cat \$REMOTE" &&
diff=$(git difftool --tool defaults --no-prompt branch) &&
- test "$diff" = "master" &&
-
- git config --unset difftool.defaults.cmd
+ test "$diff" = "master"
'
# Ensures that git-difftool ignores bogus --tool values
test_expect_success PERL 'difftool ignores bad --tool values' '
diff=$(git difftool --no-prompt --tool=bad-tool branch)
test "$?" = 1 &&
- test "$diff" = ""
+ test -z "$diff"
'
test_expect_success PERL 'difftool forwards arguments to diff' '
+ difftool_test_setup &&
>for-diff &&
git add for-diff &&
echo changes>for-diff &&
@@ -106,178 +85,165 @@ test_expect_success PERL 'difftool forwards arguments to diff' '
'
test_expect_success PERL 'difftool honors --gui' '
- git config merge.tool bogus-tool &&
- git config diff.tool bogus-tool &&
- git config diff.guitool test-tool &&
+ difftool_test_setup &&
+ test_config merge.tool bogus-tool &&
+ test_config diff.tool bogus-tool &&
+ test_config diff.guitool test-tool &&
diff=$(git difftool --no-prompt --gui branch) &&
- test "$diff" = "branch" &&
-
- restore_test_defaults
+ test "$diff" = "branch"
'
test_expect_success PERL 'difftool --gui last setting wins' '
- git config diff.guitool bogus-tool &&
- git difftool --no-prompt --gui --no-gui &&
+ difftool_test_setup &&
- git config merge.tool bogus-tool &&
- git config diff.tool bogus-tool &&
- git config diff.guitool test-tool &&
- diff=$(git difftool --no-prompt --no-gui --gui branch) &&
- test "$diff" = "branch" &&
+ diff=$(git difftool --no-prompt --gui --no-gui) &&
+ test -z "$diff" &&
- restore_test_defaults
+ test_config merge.tool bogus-tool &&
+ test_config diff.tool bogus-tool &&
+ test_config diff.guitool test-tool &&
+
+ diff=$(git difftool --no-prompt --no-gui --gui branch) &&
+ test "$diff" = "branch"
'
test_expect_success PERL 'difftool --gui works without configured diff.guitool' '
- git config diff.tool test-tool &&
+ difftool_test_setup &&
diff=$(git difftool --no-prompt --gui branch) &&
- test "$diff" = "branch" &&
-
- restore_test_defaults
+ test "$diff" = "branch"
'
# Specify the diff tool using $GIT_DIFF_TOOL
test_expect_success PERL 'GIT_DIFF_TOOL variable' '
- test_might_fail git config --unset diff.tool &&
+ difftool_test_setup &&
+ git config --unset diff.tool &&
+
GIT_DIFF_TOOL=test-tool &&
export GIT_DIFF_TOOL &&
diff=$(git difftool --no-prompt branch) &&
test "$diff" = "branch" &&
-
- restore_test_defaults
+ sane_unset GIT_DIFF_TOOL
'
# Test the $GIT_*_TOOL variables and ensure
# that $GIT_DIFF_TOOL always wins unless --tool is specified
test_expect_success PERL 'GIT_DIFF_TOOL overrides' '
- git config diff.tool bogus-tool &&
- git config merge.tool bogus-tool &&
-
+ difftool_test_setup &&
+ test_config diff.tool bogus-tool &&
+ test_config merge.tool bogus-tool &&
GIT_DIFF_TOOL=test-tool &&
export GIT_DIFF_TOOL &&
diff=$(git difftool --no-prompt branch) &&
test "$diff" = "branch" &&
+ test_config diff.tool bogus-tool &&
+ test_config merge.tool bogus-tool &&
GIT_DIFF_TOOL=bogus-tool &&
export GIT_DIFF_TOOL &&
diff=$(git difftool --no-prompt --tool=test-tool branch) &&
test "$diff" = "branch" &&
-
- restore_test_defaults
+ sane_unset GIT_DIFF_TOOL
'
# Test that we don't have to pass --no-prompt to difftool
# when $GIT_DIFFTOOL_NO_PROMPT is true
test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '
+ difftool_test_setup &&
GIT_DIFFTOOL_NO_PROMPT=true &&
export GIT_DIFFTOOL_NO_PROMPT &&
diff=$(git difftool branch) &&
test "$diff" = "branch" &&
-
- restore_test_defaults
+ sane_unset GIT_DIFFTOOL_NO_PROMPT
'
# git-difftool supports the difftool.prompt variable.
# Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false
test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '
- git config difftool.prompt false &&
+ difftool_test_setup &&
+ test_config difftool.prompt false &&
GIT_DIFFTOOL_PROMPT=true &&
export GIT_DIFFTOOL_PROMPT &&
prompt=$(echo | git difftool branch | tail -1) &&
prompt_given "$prompt" &&
-
- restore_test_defaults
+ sane_unset GIT_DIFFTOOL_PROMPT
'
# Test that we don't have to pass --no-prompt when difftool.prompt is false
test_expect_success PERL 'difftool.prompt config variable is false' '
- git config difftool.prompt false &&
+ difftool_test_setup &&
+ test_config difftool.prompt false &&
diff=$(git difftool branch) &&
- test "$diff" = "branch" &&
-
- restore_test_defaults
+ test "$diff" = "branch"
'
# Test that we don't have to pass --no-prompt when mergetool.prompt is false
test_expect_success PERL 'difftool merge.prompt = false' '
+ difftool_test_setup &&
test_might_fail git config --unset difftool.prompt &&
- git config mergetool.prompt false &&
+ test_config mergetool.prompt false &&
diff=$(git difftool branch) &&
- test "$diff" = "branch" &&
-
- restore_test_defaults
+ test "$diff" = "branch"
'
# Test that the -y flag can override difftool.prompt = true
test_expect_success PERL 'difftool.prompt can overridden with -y' '
- git config difftool.prompt true &&
+ difftool_test_setup &&
+ test_config difftool.prompt true &&
diff=$(git difftool -y branch) &&
- test "$diff" = "branch" &&
-
- restore_test_defaults
+ test "$diff" = "branch"
'
# Test that the --prompt flag can override difftool.prompt = false
test_expect_success PERL 'difftool.prompt can overridden with --prompt' '
- git config difftool.prompt false &&
+ difftool_test_setup &&
+ test_config difftool.prompt false &&
prompt=$(echo | git difftool --prompt branch | tail -1) &&
- prompt_given "$prompt" &&
-
- restore_test_defaults
+ prompt_given "$prompt"
'
# Test that the last flag passed on the command-line wins
test_expect_success PERL 'difftool last flag wins' '
+ difftool_test_setup &&
diff=$(git difftool --prompt --no-prompt branch) &&
test "$diff" = "branch" &&
- restore_test_defaults &&
-
prompt=$(echo | git difftool --no-prompt --prompt branch | tail -1) &&
- prompt_given "$prompt" &&
-
- restore_test_defaults
+ prompt_given "$prompt"
'
# git-difftool falls back to git-mergetool config variables
# so test that behavior here
test_expect_success PERL 'difftool + mergetool config variables' '
- remove_config_vars &&
- git config merge.tool test-tool &&
- git config mergetool.test-tool.cmd "cat \$LOCAL" &&
+ test_config merge.tool test-tool &&
+ test_config mergetool.test-tool.cmd "cat \$LOCAL" &&
diff=$(git difftool --no-prompt branch) &&
test "$diff" = "branch" &&
# set merge.tool to something bogus, diff.tool to test-tool
- git config merge.tool bogus-tool &&
- git config diff.tool test-tool &&
+ test_config merge.tool bogus-tool &&
+ test_config diff.tool test-tool &&
diff=$(git difftool --no-prompt branch) &&
- test "$diff" = "branch" &&
-
- restore_test_defaults
+ test "$diff" = "branch"
'
test_expect_success PERL 'difftool.<tool>.path' '
- git config difftool.tkdiff.path echo &&
+ test_config difftool.tkdiff.path echo &&
diff=$(git difftool --tool=tkdiff --no-prompt branch) &&
- git config --unset difftool.tkdiff.path &&
lines=$(echo "$diff" | grep file | wc -l) &&
- test "$lines" -eq 1 &&
-
- restore_test_defaults
+ test "$lines" -eq 1
'
test_expect_success PERL 'difftool --extcmd=cat' '
--
1.8.1.3.623.g622c8fc
^ permalink raw reply related
* [PATCH 4/4] t7800: "defaults" is no longer a builtin tool name
From: David Aguilar @ 2013-02-16 5:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1360993666-81308-3-git-send-email-davvid@gmail.com>
073678b8e6324a155fa99f40eee0637941a70a34 reworked the
mergetools/ directory so that every file corresponds to a
difftool-supported tool. When this happened the "defaults"
file went away as it was no longer needed by mergetool--lib.
t7800 tests that configured commands can override builtins,
but this test was not adjusted when the "defaults" file was
removed because the test continued to pass.
Adjust the test to use the everlasting "vimdiff" tool name
instead of "defaults" so that it correctly tests against a tool
that is known by mergetool--lib.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
t/t7800-difftool.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 2d1ba8d..6307c36 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -59,9 +59,9 @@ test_expect_success PERL 'custom commands' '
# Ensures that a custom difftool.<tool>.cmd overrides built-ins
test_expect_success PERL 'custom commands override built-ins' '
- test_config difftool.defaults.cmd "cat \$REMOTE" &&
+ test_config difftool.vimdiff.cmd "cat \$REMOTE" &&
- diff=$(git difftool --tool defaults --no-prompt branch) &&
+ diff=$(git difftool --tool vimdiff --no-prompt branch) &&
test "$diff" = "master"
'
--
1.8.1.3.623.g622c8fc
^ permalink raw reply related
* Re: [PATCH 2/4] t7800: Update copyright notice
From: David Aguilar @ 2013-02-16 6:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1360993666-81308-2-git-send-email-davvid@gmail.com>
On Fri, Feb 15, 2013 at 9:47 PM, David Aguilar <davvid@gmail.com> wrote:
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> t/t7800-difftool.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index eb1d3f8..bb3158a 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -1,6 +1,6 @@
> #!/bin/sh
> #
> -# Copyright (c) 2009, 2010 David Aguilar
> +# Copyright (c) 2009, 2010, 2012 David Aguilar
Oh boy, I'm still living in the past.
This should also include 2013. It gets me every time! ;-)
I'll wait and see if there are other review comments before I resend.
--
David
^ permalink raw reply
* [PATCH 0/3] make smart-http more robust against bogus server input
From: Jeff King @ 2013-02-16 6:44 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
For the most part, smart-http just passes data to fetch-pack and
send-pack, which take care of the heavy lifting. However, I did find a
few corner cases around truncated data from the server, one of which can
actually cause a deadlock.
I found these because I was trying to figure out what was going on with
some hung git processes which were in a deadlock like the one described
in patch 3. But having experimented and read the code, I don't think
that it is triggerable from a normal clone, but rather only when you
poke git-remote-curl in the right way. So it may or may not be my
culprit, but these patches do make remote-curl more robust, which is a
good thing.
[1/3]: pkt-line: teach packet_get_line a no-op mode
[2/3]: remote-curl: verify smart-http metadata lines
[3/3]: remote-curl: sanity check ref advertisement from server
-Peff
^ permalink raw reply
* [PATCH 1/3] pkt-line: teach packet_get_line a no-op mode
From: Jeff King @ 2013-02-16 6:46 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <20130216064455.GA27063@sigill.intra.peff.net>
You can use packet_get_line to parse a single packet out of
a stream and into a buffer. However, if you just want to
throw away a set of packets from the stream, there's no need
to even bother copying the bytes. This patch treats a NULL
output buffer as a hint that the caller does not even want
to see the output.
We have to tweak the packet_trace call, too, since it showed
the trace from the copied buffer, which now might not exist.
The new code is actually more correct, though, as it shows
just what we parsed, not any cruft that may have been in the
output buffer before (it never mattered, though, because all
callers gave us a fresh buffer).
Signed-off-by: Jeff King <peff@peff.net>
---
pkt-line.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index eaba15f..7f28701 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -234,9 +234,10 @@ int packet_get_line(struct strbuf *out,
*src_len -= 4;
len -= 4;
- strbuf_add(out, *src_buf, len);
+ if (out)
+ strbuf_add(out, *src_buf, len);
+ packet_trace(*src_buf, len, 0);
*src_buf += len;
*src_len -= len;
- packet_trace(out->buf, out->len, 0);
return len;
}
--
1.8.1.2.11.g1a2f572
^ permalink raw reply related
* [PATCH 2/3] remote-curl: verify smart-http metadata lines
From: Jeff King @ 2013-02-16 6:47 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <20130216064455.GA27063@sigill.intra.peff.net>
A smart http ref advertisement starts with a packet
containing the service header, followed by an arbitrary
number of packets containing other metadata headers,
followed by a flush packet.
We don't currently recognize any other metadata headers, so
we just parse through any extra packets, throwing away their
contents. However, we don't do so very carefully, and just
stop at the first error or flush packet.
Let's flag any errors we see here, which might be a sign of
truncated or corrupted output. Since the rest of the data
should be the ref advertisement, and since we pass that
along to our helper programs (like fetch-pack), they will
probably notice the error, as whatever cruft is in the
buffer will not parse. However, it's nice to note problems
as early as possible, which can help in debugging the root
cause.
Signed-off-by: Jeff King <peff@peff.net>
---
remote-curl.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 933c69a..73134f5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -90,6 +90,17 @@ static void free_discovery(struct discovery *d)
}
}
+static int read_packets_until_flush(char **buf, size_t *len)
+{
+ while (1) {
+ int r = packet_get_line(NULL, buf, len);
+ if (r < 0)
+ return -1;
+ if (r == 0)
+ return 0;
+ }
+}
+
static struct discovery* discover_refs(const char *service)
{
struct strbuf exp = STRBUF_INIT;
@@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char *service)
/* The header can include additional metadata lines, up
* until a packet flush marker. Ignore these now, but
- * in the future we might start to scan them.
+ * in the future we might start to scan them. However, we do
+ * still check to make sure we are getting valid packet lines,
+ * ending with a flush.
*/
- strbuf_reset(&buffer);
- while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
- strbuf_reset(&buffer);
+ if (read_packets_until_flush(&last->buf, &last->len) < 0)
+ die("smart-http metadata lines are invalid at %s",
+ refs_url);
last->proto_git = 1;
}
--
1.8.1.2.11.g1a2f572
^ permalink raw reply related
* [PATCH 3/3] remote-curl: sanity check ref advertisement from server
From: Jeff King @ 2013-02-16 6:49 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <20130216064455.GA27063@sigill.intra.peff.net>
If the smart HTTP response from the server is truncated for
any reason, we will get an incomplete ref advertisement. If
we then feed this incomplete list to "fetch-pack", one of a
few things may happen:
1. If the truncation is in a packet header, fetch-pack
will notice the bogus line and complain.
2. If the truncation is inside a packet, fetch-pack will
keep waiting for us to send the rest of the packet,
which we never will.
3. If the truncation is at a packet boundary, fetch-pack
will keep waiting for us to send the next packet, which
we never will.
As a result, fetch-pack hangs, waiting for input. However,
remote-curl believes it has sent all of the advertisement,
and therefore waits for fetch-pack to speak. The two
processes end up in a deadlock.
This fortunately doesn't happen in the normal fetching
workflow, because git-fetch first uses the "list" command,
which feeds the refs to get_remote_heads, which does notice
the error. However, you can trigger it by sending a direct
"fetch" to the remote-curl helper.
We can make this more robust by verifying that the packet
stream we got from the server does indeed parse correctly
and ends with a flush packet, which means that what
fetch-pack receives will at least be syntactically correct.
The normal non-stateless-rpc case does not have to deal with
this problem; it detects a truncation by getting EOF on the
file descriptor before it has read all data. So it is
tempting to think that we can solve this by closing the
descriptor after relaying the server's advertisement.
Unfortunately, in the stateless rpc case, we need to keep
the descriptor to fetch-pack open in order to pass more data
to it.
We could solve that by using two descriptors, but our
run-command interface does not support that (and modifying
it to create more pipes would make life hard for the Windows
port of git).
Signed-off-by: Jeff King <peff@peff.net>
---
remote-curl.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/remote-curl.c b/remote-curl.c
index 73134f5..c7647c7 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -101,6 +101,15 @@ static int read_packets_until_flush(char **buf, size_t *len)
}
}
+static int verify_ref_advertisement(char *buf, size_t len)
+{
+ /*
+ * Our function parameters are copies, so we do not
+ * have to care that read_packets will increment our pointers.
+ */
+ return read_packets_until_flush(&buf, &len);
+}
+
static struct discovery* discover_refs(const char *service)
{
struct strbuf exp = STRBUF_INIT;
@@ -174,6 +183,9 @@ static struct discovery* discover_refs(const char *service)
die("smart-http metadata lines are invalid at %s",
refs_url);
+ if (verify_ref_advertisement(last->buf, last->len) < 0)
+ die("ref advertisement is invalid at %s", refs_url);
+
last->proto_git = 1;
}
--
1.8.1.2.11.g1a2f572
^ permalink raw reply related
* [PATCH v2] read_directory: avoid invoking exclude machinery on tracked files
From: Nguyễn Thái Ngọc Duy @ 2013-02-16 7:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Karsten Blees, kusmabite, Ramkumar Ramachandra,
Robert Zeh, finnag, Nguyễn Thái Ngọc Duy
In-Reply-To: <1360937848-4426-1-git-send-email-pclouds@gmail.com>
read_directory() (and its friendly wrapper fill_directory) collects
untracked/ignored files by traversing through the whole worktree,
feeding every entry to treat_one_path(), where each entry is checked
against .gitignore patterns.
One may see that tracked files can't be excluded and we do not need to
run them through exclude machinery. On repos where there are many
.gitignore patterns and/or a lot of tracked files, this unnecessary
processing can become expensive.
This patch avoids it mostly for normal cases. Directories are still
processed as before. DIR_SHOW_IGNORED and DIR_COLLECT_IGNORED are not
normally used unless some options are given (e.g. "checkout
--overwrite-ignore", "add -f"...)
treat_one_path's behavior changes when taking this shortcut. With
current code, when a non-directory path is not excluded,
treat_one_path calls treat_file, which returns the initial value of
exclude_file and causes treat_one_path to return path_handled. With
this patch, on the same conditions, treat_one_path returns
path_ignored.
read_directory_recursive() cares about this difference. Check out the
snippet:
while (...) {
switch (treat_path(...)) {
case path_ignored:
continue;
case path_handled:
break;
}
contents++;
if (check_only)
break;
dir_add_name(dir, path.buf, path.len);
}
If path_handled is returned, contents goes up. And if check_only is
true, the loop could be broken early. These will not happen when
treat_one_path (and its wrapper treat_path) returns
path_ignored. dir_add_name internally does a cache_name_exists() check
so it makes no difference.
To avoid this behavior change, treat_one_path is instructed to skip
the optimization when check_only or contents is used.
Finally some numbers (best of 20 runs) that shows why it's worth all
the hassle:
git status | webkit linux-2.6 libreoffice-core gentoo-x86
-------------+----------------------------------------------
before | 1.097s 0.208s 0.399s 0.539s
after | 0.736s 0.159s 0.248s 0.501s
nr. patterns | 89 376 19 0
nr. tracked | 182k 40k 63k 101k
Tracked-down-by: Karsten Blees <karsten.blees@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Instead of relying on the surrounding code happening to not trigger
the behavior change in treat_one_path, this round ensures such
triggers will disable the optimization and fall back to normal code
path.
There are no big differences in measured numbers, which indicate
incorrect triggers do not happen, at least in my tests.
dir.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 52 insertions(+), 27 deletions(-)
diff --git a/dir.c b/dir.c
index 57394e4..a5fe0a0 100644
--- a/dir.c
+++ b/dir.c
@@ -17,8 +17,11 @@ struct path_simplify {
const char *path;
};
-static int read_directory_recursive(struct dir_struct *dir, const char *path, int len,
- int check_only, const struct path_simplify *simplify);
+static void read_directory_recursive(struct dir_struct *dir,
+ const char *path, int len,
+ int check_only,
+ const struct path_simplify *simplify,
+ int *contents);
static int get_dtype(struct dirent *de, const char *path, int len);
/* helper string functions with support for the ignore_case flag */
@@ -1034,6 +1037,7 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
const char *dirname, int len, int exclude,
const struct path_simplify *simplify)
{
+ int contents = 0;
/* The "len-1" is to strip the final '/' */
switch (directory_exists_in_index(dirname, len-1)) {
case index_directory:
@@ -1065,19 +1069,19 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
* check if it contains only ignored files
*/
if ((dir->flags & DIR_SHOW_IGNORED) && !exclude) {
- int ignored;
dir->flags &= ~DIR_SHOW_IGNORED;
dir->flags |= DIR_HIDE_EMPTY_DIRECTORIES;
- ignored = read_directory_recursive(dir, dirname, len, 1, simplify);
+ read_directory_recursive(dir, dirname, len, 1, simplify, &contents);
dir->flags &= ~DIR_HIDE_EMPTY_DIRECTORIES;
dir->flags |= DIR_SHOW_IGNORED;
- return ignored ? ignore_directory : show_directory;
+ return contents ? ignore_directory : show_directory;
}
if (!(dir->flags & DIR_SHOW_IGNORED) &&
!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
return show_directory;
- if (!read_directory_recursive(dir, dirname, len, 1, simplify))
+ read_directory_recursive(dir, dirname, len, 1, simplify, &contents);
+ if (!contents)
return ignore_directory;
return show_directory;
}
@@ -1242,9 +1246,23 @@ enum path_treatment {
static enum path_treatment treat_one_path(struct dir_struct *dir,
struct strbuf *path,
const struct path_simplify *simplify,
- int dtype, struct dirent *de)
+ int dtype, struct dirent *de,
+ int exclude_shortcut_ok)
{
- int exclude = is_excluded(dir, path->buf, &dtype);
+ int exclude;
+
+ if (dtype == DT_UNKNOWN)
+ dtype = get_dtype(de, path->buf, path->len);
+
+ if (exclude_shortcut_ok &&
+ !(dir->flags & DIR_SHOW_IGNORED) &&
+ !(dir->flags & DIR_COLLECT_IGNORED) &&
+ dtype != DT_DIR &&
+ cache_name_exists(path->buf, path->len, ignore_case))
+ return path_ignored;
+
+ exclude = is_excluded(dir, path->buf, &dtype);
+
if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
&& exclude_matches_pathspec(path->buf, path->len, simplify))
dir_add_ignored(dir, path->buf, path->len);
@@ -1256,9 +1274,6 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
if (exclude && !(dir->flags & DIR_SHOW_IGNORED))
return path_ignored;
- if (dtype == DT_UNKNOWN)
- dtype = get_dtype(de, path->buf, path->len);
-
switch (dtype) {
default:
return path_ignored;
@@ -1290,7 +1305,8 @@ static enum path_treatment treat_path(struct dir_struct *dir,
struct dirent *de,
struct strbuf *path,
int baselen,
- const struct path_simplify *simplify)
+ const struct path_simplify *simplify,
+ int exclude_shortcut_ok)
{
int dtype;
@@ -1302,7 +1318,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
return path_ignored;
dtype = DTYPE(de);
- return treat_one_path(dir, path, simplify, dtype, de);
+ return treat_one_path(dir, path, simplify, dtype, de, exclude_shortcut_ok);
}
/*
@@ -1314,13 +1330,13 @@ static enum path_treatment treat_path(struct dir_struct *dir,
* Also, we ignore the name ".git" (even if it is not a directory).
* That likely will not change.
*/
-static int read_directory_recursive(struct dir_struct *dir,
- const char *base, int baselen,
- int check_only,
- const struct path_simplify *simplify)
+static void read_directory_recursive(struct dir_struct *dir,
+ const char *base, int baselen,
+ int check_only,
+ const struct path_simplify *simplify,
+ int *contents)
{
DIR *fdir;
- int contents = 0;
struct dirent *de;
struct strbuf path = STRBUF_INIT;
@@ -1331,18 +1347,29 @@ static int read_directory_recursive(struct dir_struct *dir,
goto out;
while ((de = readdir(fdir)) != NULL) {
- switch (treat_path(dir, de, &path, baselen, simplify)) {
+ switch (treat_path(dir, de, &path, baselen,
+ simplify,
+ !check_only && !contents)) {
case path_recurse:
- contents += read_directory_recursive(dir, path.buf,
- path.len, 0,
- simplify);
+ read_directory_recursive(dir, path.buf,
+ path.len, 0,
+ simplify,
+ contents);
continue;
case path_ignored:
continue;
case path_handled:
break;
}
- contents++;
+ /*
+ * Update the last argument to treat_path if anything
+ * else is done after this point. This is because if
+ * treat_path's exclude_shortcut_ok is true, it may
+ * incorrectly return path_ignored (and never reaches
+ * this part) instead of path_handled.
+ */
+ if (contents)
+ (*contents)++;
if (check_only)
break;
dir_add_name(dir, path.buf, path.len);
@@ -1350,8 +1377,6 @@ static int read_directory_recursive(struct dir_struct *dir,
closedir(fdir);
out:
strbuf_release(&path);
-
- return contents;
}
static int cmp_name(const void *p1, const void *p2)
@@ -1420,7 +1445,7 @@ static int treat_leading_path(struct dir_struct *dir,
if (simplify_away(sb.buf, sb.len, simplify))
break;
if (treat_one_path(dir, &sb, simplify,
- DT_DIR, NULL) == path_ignored)
+ DT_DIR, NULL, 0) == path_ignored)
break; /* do not recurse into it */
if (len <= baselen) {
rc = 1;
@@ -1440,7 +1465,7 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const char
simplify = create_simplify(pathspec);
if (!len || treat_leading_path(dir, path, len, simplify))
- read_directory_recursive(dir, path, len, 0, simplify);
+ read_directory_recursive(dir, path, len, 0, simplify, NULL);
free_simplify(simplify);
qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), cmp_name);
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* Re: [PATCH v2] read_directory: avoid invoking exclude machinery on tracked files
From: Pete Wyckoff @ 2013-02-16 18:11 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Karsten Blees, kusmabite,
Ramkumar Ramachandra, Robert Zeh, finnag
In-Reply-To: <1360999078-27196-1-git-send-email-pclouds@gmail.com>
pclouds@gmail.com wrote on Sat, 16 Feb 2013 14:17 +0700:
> Finally some numbers (best of 20 runs) that shows why it's worth all
> the hassle:
>
> git status | webkit linux-2.6 libreoffice-core gentoo-x86
> -------------+----------------------------------------------
> before | 1.097s 0.208s 0.399s 0.539s
> after | 0.736s 0.159s 0.248s 0.501s
> nr. patterns | 89 376 19 0
> nr. tracked | 182k 40k 63k 101k
Thanks for this work. I repeated some of the tests across NFS,
where I'd expect to see bigger differences. Best of 20 values
reported in "min ...".
webkit
Stock min 9.61 avg 11.61 +/- 1.35 max 14.26
Duy min 6.91 avg 7.67 +/- 0.46 max 8.71
linux
Stock min 2.27 avg 3.16 +/- 0.56 max 4.49
Duy min 2.04 avg 3.12 +/- 0.69 max 4.87
libreoffice-core
Stock min 4.56 avg 5.79 +/- 0.79 max 7.08
Duy min 3.96 avg 5.25 +/- 0.95 max 6.95
Similar 30%-ish speedup on webkit. And an absolute gain
of 2.7 seconds is quite nice.
-- Pete
^ permalink raw reply
* Re: [PATCH v2] read_directory: avoid invoking exclude machinery on tracked files
From: Duy Nguyen @ 2013-02-17 4:39 UTC (permalink / raw)
To: Pete Wyckoff
Cc: git, Junio C Hamano, Karsten Blees, kusmabite,
Ramkumar Ramachandra, Robert Zeh, finnag
In-Reply-To: <20130216181110.GA27233@padd.com>
On Sun, Feb 17, 2013 at 1:11 AM, Pete Wyckoff <pw@padd.com> wrote:
> pclouds@gmail.com wrote on Sat, 16 Feb 2013 14:17 +0700:
>> Finally some numbers (best of 20 runs) that shows why it's worth all
>> the hassle:
>>
>> git status | webkit linux-2.6 libreoffice-core gentoo-x86
>> -------------+----------------------------------------------
>> before | 1.097s 0.208s 0.399s 0.539s
>> after | 0.736s 0.159s 0.248s 0.501s
>> nr. patterns | 89 376 19 0
>> nr. tracked | 182k 40k 63k 101k
>
> Thanks for this work. I repeated some of the tests across NFS,
> where I'd expect to see bigger differences.
This is about reducing CPU processing time, not I/O time. So no bigger
differences is expected. I/O time can be reduced with inotify, or fam
in nfs case because inotify does not support nfs.
> Best of 20 values reported in "min ...".
>
> webkit
> Stock min 9.61 avg 11.61 +/- 1.35 max 14.26
> Duy min 6.91 avg 7.67 +/- 0.46 max 8.71
>
> linux
> Stock min 2.27 avg 3.16 +/- 0.56 max 4.49
> Duy min 2.04 avg 3.12 +/- 0.69 max 4.87
>
> libreoffice-core
> Stock min 4.56 avg 5.79 +/- 0.79 max 7.08
> Duy min 3.96 avg 5.25 +/- 0.95 max 6.95
>
> Similar 30%-ish speedup on webkit. And an absolute gain
> of 2.7 seconds is quite nice.
>
> -- Pete
--
Duy
^ 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