* [PATCH 3/4] git-mergetool: don't hardcode 'mergetool' in show_tool_help
From: John Keeping @ 2013-01-24 19:55 UTC (permalink / raw)
To: git; +Cc: David Aguilar
In-Reply-To: <cover.1359057056.git.john@keeping.me.uk>
When using show_tool_help from git-difftool we will want it to print
"git difftool" not "git mergetool" so use "git ${TOOL_MODE}tool".
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-mergetool--lib.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 1748315..4c1e129 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -188,12 +188,14 @@ show_tool_help () {
unavailable="$unavailable$i$LF"
fi
done
+
+ cmd_name=${TOOL_MODE}tool
if test -n "$available"
then
- echo "'git mergetool --tool=<tool>' may be set to one of the following:"
+ echo "'git $cmd_name --tool=<tool>' may be set to one of the following:"
echo "$available" | sort | sed -e 's/^/ /'
else
- echo "No suitable tool for 'git mergetool --tool=<tool>' found."
+ echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
fi
if test -n "$unavailable"
then
--
1.8.1
^ permalink raw reply related
* [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
From: John Keeping @ 2013-01-24 19:55 UTC (permalink / raw)
To: git; +Cc: David Aguilar
In-Reply-To: <cover.1359057056.git.john@keeping.me.uk>
The "--tool-help" option to git-difftool currently displays incorrect
output since it uses the names of the files in
"$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
git-mergetool--lib.
Fix this by simply delegating the "--tool-help" argument to the
show_tool_help function in git-mergetool--lib.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-difftool.perl | 57 ++++++++-----------------------------------------------
1 file changed, 8 insertions(+), 49 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index edd0493..0a90de4 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -59,57 +59,16 @@ sub find_worktree
return $worktree;
}
-sub filter_tool_scripts
-{
- my ($tools) = @_;
- if (-d $_) {
- if ($_ ne ".") {
- # Ignore files in subdirectories
- $File::Find::prune = 1;
- }
- } else {
- if ((-f $_) && ($_ ne "defaults")) {
- push(@$tools, $_);
- }
- }
-}
-
sub print_tool_help
{
- my ($cmd, @found, @notfound, @tools);
- my $gitpath = Git::exec_path();
-
- find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools");
-
- foreach my $tool (@tools) {
- $cmd = "TOOL_MODE=diff";
- $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
- $cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1";
- $cmd .= " && can_diff >/dev/null 2>&1";
- if (system('sh', '-c', $cmd) == 0) {
- push(@found, $tool);
- } else {
- push(@notfound, $tool);
- }
- }
-
- print << 'EOF';
-'git difftool --tool=<tool>' may be set to one of the following:
-EOF
- print "\t$_\n" for (sort(@found));
-
- print << 'EOF';
-
-The following tools are valid, but not currently available:
-EOF
- print "\t$_\n" for (sort(@notfound));
-
- print << 'EOF';
-
-NOTE: Some of the tools listed above only work in a windowed
-environment. If run in a terminal-only session, they will fail.
-EOF
- exit(0);
+ my $cmd = 'TOOL_MODE=diff';
+ $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
+ $cmd .= ' && show_tool_help';
+
+ # See the comment at the bottom of file_diff() for the reason behind
+ # using system() followed by exit() instead of exec().
+ my $rc = system('sh', '-c', $cmd);
+ exit($rc | ($rc >> 8));
}
sub exit_cleanup
--
1.8.1
^ permalink raw reply related
* Re: [PATCH] git-cvsimport.txt: cvsps-2 is deprecated
From: Junio C Hamano @ 2013-01-24 20:04 UTC (permalink / raw)
To: John Keeping; +Cc: git, Eric S. Raymond, Chris Rorvick
In-Reply-To: <20130124191845.GS7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> git-cvsimport relies on version 2 of cvsps and does not work with the
> new version 3. Since cvsps 3.x does not currently work as well as
> version 2 for incremental import, document this fact.
>
> Specifically, there is no way to make new git-cvsimport that supports
> cvsps 3.x and have a seamless transition for existing users since cvsps
> 3.x needs a time from which to continue importing and git-cvsimport does
> not save the time of the last import or import into a specific namespace
> so there is no safe way to calculate the time of the last import.
Isn't the whole "and git-cvsimport does not save the time..." part
something that can be fixed in the new cvsimport that reads the
output from cvsps3?
To me, it sounds more like
cvsps2 + cvsimport has an unfixable bugs and there have been an
effort to rewrite cvsps2 from scratch. The resulting cvsp3
currently is unusable with cvsimport, especially when importing
the history incrementally, and it isn't expected that it will
ever be usable with cvsimport again.
There are other tools that analyse the original history better
and emits more correct output when used to convert the whole
history, and hopefully cvsps3 + fast-import would become one of
them. Suggest users to use them instead of cvsimport when they
are not doing an incremental import.
By the way, do we want to make any recommendation to the distro
folks which cvsps they should ship? It appears that not shipping
cvsps2 would be a major regression if cvsps3 does not plan to
support incrementals, so shipping both might be the safest way for
them to support their users with different needs.
Thanks. I think the patch text itself is good.
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> On Wed, Jan 23, 2013 at 09:04:12PM -0800, Junio C Hamano wrote:
>> Care to roll a proper patch with a log message? I'll discard the
>> topic for now and replace it with your documentation update.
>
> Here it is.
>
> Documentation/git-cvsimport.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
> index 9d5353e..f059ea9 100644
> --- a/Documentation/git-cvsimport.txt
> +++ b/Documentation/git-cvsimport.txt
> @@ -18,6 +18,12 @@ SYNOPSIS
>
> DESCRIPTION
> -----------
> +*WARNING:* `git cvsimport` uses cvsps version 2, which is considered
> +deprecated; it does not work with cvsps version 3 and later. If you are
> +performing a one-shot import of a CVS repository consider using
> +link:http://cvs2svn.tigris.org/cvs2git.html[cvs2git] or
> +link:https://github.com/BartMassey/parsecvs[parsecvs].
> +
> Imports a CVS repository into git. It will either create a new
> repository, or incrementally import into an existing one.
^ permalink raw reply
* Re: [PATCH] git-cvsimport.txt: cvsps-2 is deprecated
From: John Keeping @ 2013-01-24 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric S. Raymond, Chris Rorvick
In-Reply-To: <7v7gn2bb5w.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 12:04:11PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > git-cvsimport relies on version 2 of cvsps and does not work with the
> > new version 3. Since cvsps 3.x does not currently work as well as
> > version 2 for incremental import, document this fact.
> >
> > Specifically, there is no way to make new git-cvsimport that supports
> > cvsps 3.x and have a seamless transition for existing users since cvsps
> > 3.x needs a time from which to continue importing and git-cvsimport does
> > not save the time of the last import or import into a specific namespace
> > so there is no safe way to calculate the time of the last import.
>
> Isn't the whole "and git-cvsimport does not save the time..." part
> something that can be fixed in the new cvsimport that reads the
> output from cvsps3?
Yes it can be fixed there (and I have patches to do that) - my argument
here is that there cannot be a seamless upgrade for people who are
currently using git-cvsimport incrementally. If you don't have that
file then how do you create it to reflect the current state of your
repository?
> To me, it sounds more like
>
> cvsps2 + cvsimport has an unfixable bugs and there have been an
> effort to rewrite cvsps2 from scratch. The resulting cvsp3
> currently is unusable with cvsimport, especially when importing
> the history incrementally, and it isn't expected that it will
> ever be usable with cvsimport again.
cvsps3 isn't a re-write, it's cvsps2 with a lot of things ripped out and
a fast-export mode added. And in fast-export mode it cannot inspect the
Git repository in the same way that git-cvsimport does.
> There are other tools that analyse the original history better
> and emits more correct output when used to convert the whole
> history, and hopefully cvsps3 + fast-import would become one of
> them. Suggest users to use them instead of cvsimport when they
> are not doing an incremental import.
Yes. The consensus seems to be that cvs2git is the most correct.
> By the way, do we want to make any recommendation to the distro
> folks which cvsps they should ship? It appears that not shipping
> cvsps2 would be a major regression if cvsps3 does not plan to
> support incrementals, so shipping both might be the safest way for
> them to support their users with different needs.
I agree. cvsps is only one binary and a man page so I don't think it
would be too hard to ship both.
John
^ permalink raw reply
* Re: [PATCH v4 1/4] t6006 (rev-list-format): don't hardcode SHA-1 in expected outputs
From: Junio C Hamano @ 2013-01-24 20:29 UTC (permalink / raw)
To: Alexey Shumkin; +Cc: git
In-Reply-To: <cee3610fde1626c2854eb5b821529ab22a06e4bf.1359018188.git.Alex.Crezoff@gmail.com>
Alexey Shumkin <alex.crezoff@gmail.com> writes:
> The expected SHA-1 digests are always available in variables. Use
> them instead of hardcoding.
>
> Signed-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>
> ---
> t/t6006-rev-list-format.sh | 130 +++++++++++++++++++++++++--------------------
> 1 file changed, 72 insertions(+), 58 deletions(-)
>
> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> index f94f0c4..c248509 100755
> --- a/t/t6006-rev-list-format.sh
> +++ b/t/t6006-rev-list-format.sh
> @@ -6,8 +6,19 @@ test_description='git rev-list --pretty=format test'
>
> test_tick
> test_expect_success 'setup' '
> -touch foo && git add foo && git commit -m "added foo" &&
> - echo changed >foo && git commit -a -m "changed foo"
> + touch foo &&
This is inherited from the original, but these days we try to avoid
touch, unless it is about setting a new file timestamp. The
canonical (in the script we write) way to create an empty file is:
: >foo
with or without ": ", it does not matter that much.
> + git add foo &&
> + git commit -m "added foo" &&
> + head1=$(git rev-parse --verify HEAD) &&
> + head1_7=$(echo $head1 | cut -c1-7) &&
Why do we want "whatever_7" variables and use "cut -c1-7" to produce
them? Is "7" something we care deeply about?
I think what we care a lot more than "7" that happens to be the
current default value is to make sure that, if we ever update the
default abbreviation length to a larger value, the abbreviation
shown with --format=%h is consistent with the abbreviation that is
given by rev-parse --short.
head1_short=$(git rev-parse --short $head1)
perhaps?
> + echo changed >foo &&
> + git commit -a -m "changed foo" &&
> + head2=$(git rev-parse --verify HEAD) &&
> + head2_7=$(echo $head2 | cut -c1-7) &&
> + head2_parent=$(git cat-file -p HEAD | grep parent | cut -f 2 -d" ") &&
Do not use "cat-file -p" that is for human consumption in scripts,
unless you are testing how the format for human consumption should
look like (we may later add more pretty-print to them), which is not
the case here.
Also be careful and pay attention to the end of the header; you do
not want to pick up a random "parent" string in the middle of a log
message.
head2_parent=$(git cat-file commit HEAD | sed -n -e "s/^parent //p" -e "/^$/q")
would be much better.
> + head2_parent_7=$(echo $head2_parent | cut -c1-7) &&
> + tree2=$(git cat-file -p HEAD | grep tree | cut -f 2 -d" ") &&
Likewise.
> + tree2_7=$(echo $tree2 | cut -c1-7)
Likewise.
> @@ -131,39 +142,42 @@ This commit message is much longer than the others,
> and it will be encoded in iso8859-1. We should therefore
> include an iso8859 character: ¡bueno!
> EOF
> +
> test_expect_success 'setup complex body' '
> -git config i18n.commitencoding iso8859-1 &&
> - echo change2 >foo && git commit -a -F commit-msg
> + git config i18n.commitencoding iso8859-1 &&
> + echo change2 >foo && git commit -a -F commit-msg &&
> + head3=$(git rev-parse --verify HEAD) &&
> + head3_7=$(echo $head3 | cut -c1-7)
> '
Likewise.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 2/4] t7102 (reset): refactoring: don't hardcode SHA-1 in expected outputs
From: Junio C Hamano @ 2013-01-24 20:30 UTC (permalink / raw)
To: Alexey Shumkin; +Cc: git
In-Reply-To: <63637506fa940f6dd5fc050c7fccfa5ef41993dd.1359018188.git.Alex.Crezoff@gmail.com>
Alexey Shumkin <alex.crezoff@gmail.com> writes:
> The expected SHA-1 digests are always available in variables. Use
> them instead of hardcoding.
>
> Signed-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>
> ---
Looks good (" refactoring:" in the title may not want to be there,
though).
Thanks.
> t/t7102-reset.sh | 41 +++++++++++++++++++++--------------------
> 1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index b096dc8..cf492f4 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -28,7 +28,8 @@ test_expect_success 'creating initial files and commits' '
>
> echo "1st line 2nd file" >secondfile &&
> echo "2nd line 2nd file" >>secondfile &&
> - git commit -a -m "modify 2nd file"
> + git commit -a -m "modify 2nd file" &&
> + head5=$(git rev-parse --verify HEAD)
> '
> # git log --pretty=oneline # to see those SHA1 involved
>
> @@ -56,7 +57,7 @@ test_expect_success 'giving a non existing revision should fail' '
> test_must_fail git reset --mixed aaaaaa &&
> test_must_fail git reset --soft aaaaaa &&
> test_must_fail git reset --hard aaaaaa &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
> '
>
> test_expect_success 'reset --soft with unmerged index should fail' '
> @@ -74,7 +75,7 @@ test_expect_success \
> test_must_fail git reset --hard -- first &&
> test_must_fail git reset --soft HEAD^ -- first &&
> test_must_fail git reset --hard HEAD^ -- first &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
> '
>
> test_expect_success 'giving unrecognized options should fail' '
> @@ -86,7 +87,7 @@ test_expect_success 'giving unrecognized options should fail' '
> test_must_fail git reset --soft -o &&
> test_must_fail git reset --hard --other &&
> test_must_fail git reset --hard -o &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
> '
>
> test_expect_success \
> @@ -110,7 +111,7 @@ test_expect_success \
>
> git checkout master &&
> git branch -D branch1 branch2 &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
> '
>
> test_expect_success \
> @@ -133,27 +134,27 @@ test_expect_success \
>
> git checkout master &&
> git branch -D branch3 branch4 &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
> '
>
> test_expect_success \
> 'resetting to HEAD with no changes should succeed and do nothing' '
> git reset --hard &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
> git reset --hard HEAD &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
> git reset --soft &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
> git reset --soft HEAD &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
> git reset --mixed &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
> git reset --mixed HEAD &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
> git reset &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
> git reset HEAD &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
> '
>
> >.diff_expect
> @@ -176,7 +177,7 @@ test_expect_success '--soft reset only should show changes in diff --cached' '
> git reset --soft HEAD^ &&
> check_changes d1a4bc3abce4829628ae2dcb0d60ef3d1a78b1c4 &&
> test "$(git rev-parse ORIG_HEAD)" = \
> - 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + $head5
> '
>
> >.diff_expect
> @@ -193,7 +194,7 @@ test_expect_success \
> git commit -a -C ORIG_HEAD &&
> check_changes 3d3b7be011a58ca0c179ae45d94e6c83c0b0cd0d &&
> test "$(git rev-parse ORIG_HEAD)" = \
> - 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + $head5
> '
>
> >.diff_expect
> @@ -303,7 +304,7 @@ test_expect_success 'redoing the last two commits should succeed' '
> echo "1st line 2nd file" >secondfile &&
> echo "2nd line 2nd file" >>secondfile &&
> git commit -a -m "modify 2nd file" &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
> '
>
> >.diff_expect
> @@ -341,15 +342,15 @@ EOF
> test_expect_success \
> '--hard reset to ORIG_HEAD should clear a fast-forward merge' '
> git reset --hard HEAD^ &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
>
> git pull . branch1 &&
> git reset --hard ORIG_HEAD &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
>
> git checkout master &&
> git branch -D branch1 branch2 &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
> '
>
> cat > expect << EOF
^ permalink raw reply
* Re: [PATCH v4 3/4] pretty: Add failing tests: user format ignores i18n.logOutputEncoding setting
From: Junio C Hamano @ 2013-01-24 20:44 UTC (permalink / raw)
To: Alexey Shumkin; +Cc: git
In-Reply-To: <6de583a2d281b1614c69d5e7b6f5c4495488f6a3.1359018188.git.Alex.Crezoff@gmail.com>
Alexey Shumkin <alex.crezoff@gmail.com> writes:
> The following two commands are expected to give the same output to a terminal:
>
> $ git log --oneline --no-color
> $ git log --pretty=format:'%h %s'
>
> However, the former pays attention to i18n.logOutputEncoding
> configuration, while the latter does not when it format "%s".
> Log messages written in an encoding i18n.commitEncoding which differs
> from terminal encoding are shown corrupted with the latter even
> when i18n.logOutputEncoding and terminal encoding are the same.
>
> The same corruption is true for
> $ git diff --submodule=log
> and
> $ git rev-list --pretty=format:%s HEAD
> and
> $ git reset --hard
>
> Signed-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>
> ---
> t/t4041-diff-submodule-option.sh | 33 ++++++++-------
> t/t4205-log-pretty-formats.sh | 43 +++++++++++++++----
> t/t6006-rev-list-format.sh | 90 ++++++++++++++++++++++++++--------------
> t/t7102-reset.sh | 32 +++++++++++---
> 4 files changed, 138 insertions(+), 60 deletions(-)
>
> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> index 32d4a60..e7d6363 100755
> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -1,6 +1,7 @@
> #!/bin/sh
> #
> # Copyright (c) 2009 Jens Lehmann, based on t7401 by Ping Yin
> +# Copyright (c) 2011 Alexey Shumkin (+ non-UTF-8 commit encoding tests)
> #
>
> test_description='Support for verbose submodule differences in git diff
> @@ -10,6 +11,7 @@ This test tries to verify the sanity of the --submodule option of git diff.
>
> . ./test-lib.sh
>
> +added=$(printf "\320\264\320\276\320\261\320\260\320\262\320\273\320\265\320\275")
Please have an in-code comment before this line to explain what this
variable is about, e.g.
# String "added" in Russian, encoded in UTF-8, used in
# sample commit log messages in add_file() function below.
added=$(printf "...")
> add_file () {
> (
> cd "$1" &&
> @@ -19,7 +21,8 @@ add_file () {
> echo "$name" >"$name" &&
> git add "$name" &&
> test_tick &&
> - git commit -m "Add $name" || exit
> + msg_added_cp1251=$(echo "Add $name ($added $name)" | iconv -f utf-8 -t cp1251) &&
> + git -c 'i18n.commitEncoding=cp1251' commit -m "$msg_added_cp1251"
> done >/dev/null &&
> git rev-parse --short --verify HEAD
> )
Does this patch make the all tests in this script fail for people
without cp1251 locale installed? We already have tests that depend
on 8859-1 and some other locales, and we'd be better off limiting
such dependency to the minimum.
Same comment applies to the changes to other test scripts.
Thanks.
^ permalink raw reply
* Re: [PATCH] git-cvsimport.txt: cvsps-2 is deprecated
From: Junio C Hamano @ 2013-01-24 20:58 UTC (permalink / raw)
To: John Keeping; +Cc: git, Eric S. Raymond, Chris Rorvick
In-Reply-To: <20130124201330.GT7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> On Thu, Jan 24, 2013 at 12:04:11PM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>> > git-cvsimport relies on version 2 of cvsps and does not work with the
>> > new version 3. Since cvsps 3.x does not currently work as well as
>> > version 2 for incremental import, document this fact.
>> >
>> > Specifically, there is no way to make new git-cvsimport that supports
>> > cvsps 3.x and have a seamless transition for existing users since cvsps
>> > 3.x needs a time from which to continue importing and git-cvsimport does
>> > not save the time of the last import or import into a specific namespace
>> > so there is no safe way to calculate the time of the last import.
>>
>> Isn't the whole "and git-cvsimport does not save the time..." part
>> something that can be fixed in the new cvsimport that reads the
>> output from cvsps3?
>
> Yes it can be fixed there (and I have patches to do that) - my argument
> here is that there cannot be a seamless upgrade for people who are
> currently using git-cvsimport incrementally. If you don't have that
> file then how do you create it to reflect the current state of your
> repository?
I am not disagreeing with your patch text to warn the current users
that cvsimport will be made unusable if they lose cvsps2, and they
are better off using other tools when doing a whole-history import.
I was trying to make sure that my understanding of the current
situation and future possibilities matches yours.
Thanks, I think you clarified the situation better with your
response.
^ permalink raw reply
* Re: [PATCH v4 3/4] pretty: Add failing tests: user format ignores i18n.logOutputEncoding setting
From: Junio C Hamano @ 2013-01-24 21:02 UTC (permalink / raw)
To: Alexey Shumkin; +Cc: git
In-Reply-To: <6de583a2d281b1614c69d5e7b6f5c4495488f6a3.1359018188.git.Alex.Crezoff@gmail.com>
Alexey Shumkin <alex.crezoff@gmail.com> writes:
> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> index c248509..4db43a4 100755
> --- a/t/t6006-rev-list-format.sh
> +++ b/t/t6006-rev-list-format.sh
> ...
> @@ -112,12 +133,12 @@ commit $head2
> commit $head1
> EOF
>
> -test_format raw-body %B <<'EOF'
> -commit 131a310eb913d107dd3c09a65d1651175898735d
> -changed foo
> +test_format failure raw-body %B <<EOF
> +commit $head2
> +$changed
>
> -commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
> -added foo
> +commit $head1
> +$added
>
> EOF
It may have been easier to follow if you did this "Don't hardcode"
as a separate preparatory patch, like your first two patches.
> @@ -135,16 +156,16 @@ commit $head1
> foo
> EOF
>
> -cat >commit-msg <<'EOF'
> +iconv -f utf-8 -t cp1251 > commit-msg <<EOF
> Test printing of complex bodies
>
> This commit message is much longer than the others,
> -and it will be encoded in iso8859-1. We should therefore
> -include an iso8859 character: ¡bueno!
> +and it will be encoded in cp1251. We should therefore
> +include an cp1251 character: так вот!
> EOF
>
> test_expect_success 'setup complex body' '
> - git config i18n.commitencoding iso8859-1 &&
> + git config i18n.commitencoding cp1251 &&
What is going on here?
Is this an example that shows that i18n.commitencoding works
correctly with iso8859-1 but not with cp1251?
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index cf492f4..699c824 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> ...
> @@ -192,7 +214,7 @@ test_expect_success \
> 'changing files and redo the last commit should succeed' '
> echo "3rd line 2nd file" >>secondfile &&
> git commit -a -C ORIG_HEAD &&
> - check_changes 3d3b7be011a58ca0c179ae45d94e6c83c0b0cd0d &&
> + check_changes f06f78b8dd468c722952b77569dd0db212442c25 &&
> test "$(git rev-parse ORIG_HEAD)" = \
> $head5
> '
This and remaining hunks to this script shows that it would be
helped by the same love you gave to other scripts with your first
two patches before you add the "non-unicode" tests, no?
^ permalink raw reply
* Re: [PATCH 0/4] Fix "git difftool --tool-help"
From: Junio C Hamano @ 2013-01-24 21:07 UTC (permalink / raw)
To: John Keeping; +Cc: git, David Aguilar
In-Reply-To: <cover.1359057056.git.john@keeping.me.uk>
Nice code reduction ;-)
Thanks, will queue.
^ permalink raw reply
* Re: [PATCH] parse_object: clear "parsed" when freeing buffers
From: Jonathon Mah @ 2013-01-24 21:16 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130124070715.GD610@sigill.intra.peff.net>
On 2013-01-23, at 23:07, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:
>
>> Several areas of code would free buffers for object structs that
>> contained them ("struct tree" and "struct commit"), but without clearing
>> the "parsed" flag. parse_object would clear the flag for "struct tree",
>> but commits would remain in an invalid state (marked as parsed but with
>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
>> invalid objects stay around and can lead to bad behavior.
>>
>> In particular, running pickaxe on all refs in a repository with a cached
>> textconv could segfault. If the textconv cache ref was evaluated first
>> by cmd_log_walk, a subsequent notes_cache_match_validity call would
>> dereference NULL.
>
> Just to be sure I understand, what is going on is something like this?
>
> Log reads all refs, including notes. After showing the notes commit,
> log frees the buffer to save memory. Later, doing the diff on a
> commit causes us to use the notes_cache mechanism. That will look at
> the commit at the tip of the notes ref, which log has already
> processed. It will call parse_commit, but that will do nothing, as the
> parsed flag is already set, but the commit buffer remains NULL.
>
> I wonder if this could be the source of the segfault here, too:
>
> http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355
Indeed, that description matches what I see. The other segfault seems to be in the same place too.
The segfault hits with the following stack (optimization off):
0 git-log parse_commit_header + 39 (pretty.c:738)
1 git-log format_commit_one + 3102 (pretty.c:1138)
2 git-log format_commit_item + 177 (pretty.c:1203)
3 git-log strbuf_expand + 172 (strbuf.c:247)
4 git-log format_commit_message + 196 (pretty.c:1263)
5 git-log notes_cache_match_validity + 215 (notes-cache.c:22)
6 git-log notes_cache_init + 212 (notes-cache.c:41)
7 git-log userdiff_get_textconv + 176 (userdiff.c:301)
8 git-log get_textconv + 66 (diff.c:2233)
9 git-log has_changes + 53 (diffcore-pickaxe.c:205)
10 git-log pickaxe + 299 (diffcore-pickaxe.c:40)
11 git-log diffcore_pickaxe_count + 344 (diffcore-pickaxe.c:275)
12 git-log diffcore_pickaxe + 58 (diffcore-pickaxe.c:289)
13 git-log diffcore_std + 162 (diff.c:4673)
14 git-log log_tree_diff_flush + 43 (log-tree.c:721)
15 git-log log_tree_diff + 566 (log-tree.c:826)
16 git-log log_tree_commit + 63 (log-tree.c:847)
17 git-log cmd_log_walk + 172 (log.c:301)
18 git-log cmd_log + 186 (log.c:568)
19 git-log run_builtin + 475 (git.c:273)
20 git-log handle_internal_command + 199 (git.c:434)
21 git-log main + 149 (git.c:523)
22 libdyld.dylib start + 1
commit->message was freed and nulled earlier in a call to cmd_log_walk. This test reproduces it reliably on my machine:
diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..d7e26ca 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces correct results' '
test_cmp expect actual
'
+cat >expect <<EOF
+./helper other (refs/notes/textconv/magic)
+one
+EOF
+# add empty commit on master to make bug more reproducible
+test_expect_success 'pickaxe with cached textconv' '
+ git commit --allow-empty -m another &&
+ git log -S other --pretty=tformat:%s%d --all >actual &&
+ test_cmp expect actual
+'
+
test_done
Jonathon Mah
me@JonathonMah.com
^ permalink raw reply related
* [PATCH] t9902: Instruct git-completion.bash about a test mode
From: Jean-Noël AVILA @ 2013-01-24 21:20 UTC (permalink / raw)
To: git
In test mode, git completion should propose commands only if they
belong to the list of authorized commands.
Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
---
Better show some code than try to explain. Here is what I mean by
"filter the output git help -a".
contrib/completion/git-completion.bash | 16 +++++++++++++++-
t/t9902-completion.sh | 4 ++--
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 14dd5e7..6490553 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,6 +531,20 @@ __git_complete_strategy ()
return 1
}
+if test -z "$AUTHORIZED_CMD_LIST"; then
+ __git_cmdlist ()
+ {
+ echo $1;
+ }
+else
+ __git_cmdlist ()
+ {
+ if [[ " $AUTHORIZED_CMD_LIST " =~ " $1 " ]] ; then
+ echo $1
+ fi
+ }
+fi
+
__git_list_all_commands ()
{
local i IFS=" "$'\n'
@@ -538,7 +552,7 @@ __git_list_all_commands ()
do
case $i in
*--*) : helper pattern;;
- *) echo $i;;
+ *) __git_cmdlist $i;;
esac
done
}
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..5e7d81e 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -12,8 +12,8 @@ complete ()
# do nothing
return 0
}
-
-. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+AUTHORIZED_CMD_LIST=" checkout show add filter-branch ls-files send-email describe"
+. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
# We don't need this function to actually join words or do anything special.
# Also, it's cleaner to avoid touching bash's internal completion variables.
--
1.8.1.1.271.g02f55e6
^ permalink raw reply related
* Re: [PATCH] mergetool: clean up temp files when aborted
From: Phil Hord @ 2013-01-24 21:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phil Hord, git@vger.kernel.org, Theodore Ts'o
In-Reply-To: <7vbocebblo.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <hordp@cisco.com> writes:
>
>> When handling a symlink conflict or a deleted-file conflict, mergetool
>> stops to ask the user what to do. If the user chooses any option besides
>> "(a)bort", then the temporary files which mergetool created in
>> preparation for handling the conflict are removed. But these temporary
>> files are not removed when the user chooses to abort the operation.
>>
>> $ git cherry-pick other/branch
>> error: could not apply 4e43581... Fix foo.c
>>
>> $ git status --short
>> DU foo.c
>>
>> $ git mergetool
>> Merging:
>> foo.c
>>
>> Deleted merge conflict for 'foo.c':
>> {local}: deleted
>> {remote}: modified file
>> Use (m)odified or (d)eleted file, or (a)bort? a
>> Continue merging other unresolved paths (y/n) ? n
>>
>> $ git status --short
>> DU foo.c
>> ?? foo.c.BACKUP.16929.c
>> ?? foo.c.BASE.16929.c
>> ?? foo.c.LOCAL.16929.c
>> ?? foo.c.REMOTE.16929.c
>>
>> These temporary files should not remain after the mergetool operation is
>> completed.
>
> Aren't there cases where people "abort" so that they can have a
> chance inspect them outside mergetool program? If there are no such
> cases, then I would agree with your claim "should not remain", but
> the proposed log message does not explay why it is sure that there
> are no such use cases.
You may be right about other people's workflows which I forgot to
consider. I have noticed a lot of inconsistency in this tool wrt to
mergetool.keepBackup and mergetool.keepTemporaries. Perhaps this
change should hinge on the latter.
Would you agree with this rephrased statement (accompanied by matching logic)?
When mergetool.keepTemporaries is false or not set, these temporary files
should not remain when the mergetool operation is aborted.
Here is the wording from git help config:
mergetool.keepBackup
After performing a merge, the original file with conflict
markers can be saved as a file with a .orig extension. If this
variable is set to false then this file
is not preserved. Defaults to true (i.e. keep the backup files).
mergetool.keepTemporaries
When invoking a custom merge tool, git uses a set of
temporary files to pass to the tool. If the tool returns an error and
this variable is set to true, then
these temporary files will be preserved, otherwise they
will be removed after the tool has exited. Defaults to false.
I have another commit in the works to clean up consistency around the
keepBackup. I'll re-roll this one at the same time.
Phil
^ permalink raw reply
* Re: [PATCH] mergetool: clean up temp files when aborted
From: Junio C Hamano @ 2013-01-24 21:41 UTC (permalink / raw)
To: Phil Hord; +Cc: Phil Hord, git@vger.kernel.org, Theodore Ts'o
In-Reply-To: <CABURp0qxj0HVHbYOR1Enfx_A1-ALPDGhLriGp=gab-LBigwt8g@mail.gmail.com>
Phil Hord <phil.hord@gmail.com> writes:
> On Thu, Jan 24, 2013 at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Phil Hord <hordp@cisco.com> writes:
>> ...
>>> These temporary files should not remain after the mergetool operation is
>>> completed.
>>
>> Aren't there cases where people "abort" so that they can have a
>> chance inspect them outside mergetool program? If there are no such
>> cases, then I would agree with your claim "should not remain", but
>> the proposed log message does not explay why it is sure that there
>> are no such use cases.
>
> You may be right about other people's workflows which I forgot to
> consider. I have noticed a lot of inconsistency in this tool wrt to
> mergetool.keepBackup and mergetool.keepTemporaries. Perhaps this
> change should hinge on the latter.
>
> Would you agree with this rephrased statement (accompanied by matching logic)?
>
> When mergetool.keepTemporaries is false or not set, these temporary files
> should not remain when the mergetool operation is aborted.
That is perfectly sensible (and should apply equally to non-"abort"
cases, I think).
Thanks.
^ permalink raw reply
* Re: [PATCH] t9902: Instruct git-completion.bash about a test mode
From: Junio C Hamano @ 2013-01-24 21:46 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: git, Jeff King
In-Reply-To: <201301242220.09067.jn.avila@free.fr>
"Jean-Noël AVILA" <jn.avila@free.fr> writes:
> In test mode, git completion should propose commands only if they
> belong to the list of authorized commands.
>
> Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
> ---
>
> Better show some code than try to explain. Here is what I mean by
> "filter the output git help -a".
Why do you have to make an extra shell function call for each and
every possible Git subcommand to slow down everybody's work when not
in testing mode?
Comparing it with Peff's suggestion, it is fairly clear which one we
should pick, I think.
> contrib/completion/git-completion.bash | 16 +++++++++++++++-
> t/t9902-completion.sh | 4 ++--
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 14dd5e7..6490553 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -531,6 +531,20 @@ __git_complete_strategy ()
> return 1
> }
>
> +if test -z "$AUTHORIZED_CMD_LIST"; then
> + __git_cmdlist ()
> + {
> + echo $1;
> + }
> +else
> + __git_cmdlist ()
> + {
> + if [[ " $AUTHORIZED_CMD_LIST " =~ " $1 " ]] ; then
> + echo $1
> + fi
> + }
> +fi
> +
> __git_list_all_commands ()
> {
> local i IFS=" "$'\n'
> @@ -538,7 +552,7 @@ __git_list_all_commands ()
> do
> case $i in
> *--*) : helper pattern;;
> - *) echo $i;;
> + *) __git_cmdlist $i;;
> esac
> done
> }
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 3cd53f8..5e7d81e 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -12,8 +12,8 @@ complete ()
> # do nothing
> return 0
> }
> -
> -. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
> +AUTHORIZED_CMD_LIST=" checkout show add filter-branch ls-files send-email describe"
> +. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
>
> # We don't need this function to actually join words or do anything special.
> # Also, it's cleaner to avoid touching bash's internal completion variables.
^ permalink raw reply
* Re: [PATCH] t9902: Instruct git-completion.bash about a test mode
From: Jean-Noël AVILA @ 2013-01-24 22:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vtxq68day.fsf@alter.siamese.dyndns.org>
Le jeudi 24 janvier 2013 22:46:13, Junio C Hamano a écrit :
> "Jean-Noël AVILA" <jn.avila@free.fr> writes:
>
> > In test mode, git completion should propose commands only if they
> > belong to the list of authorized commands.
> >
> > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
> > ---
> >
> > Better show some code than try to explain. Here is what I mean by
> > "filter the output git help -a".
>
> Why do you have to make an extra shell function call for each and
> every possible Git subcommand to slow down everybody's work when not
> in testing mode?
>
My rational was to be sure to put the environment variable out of the
way once the script has been sourced. I can make two alternative
definitions of __git_list_all_commands () depending on the presence of
$AUTHORIZED_CMD_LIST if you are worried about performance.
> Comparing it with Peff's suggestion, it is fairly clear which one we
> should pick, I think.
>
>
>
> > contrib/completion/git-completion.bash | 16 +++++++++++++++-
> > t/t9902-completion.sh | 4 ++--
> > 2 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 14dd5e7..6490553 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -531,6 +531,20 @@ __git_complete_strategy ()
> > return 1
> > }
> >
> > +if test -z "$AUTHORIZED_CMD_LIST"; then
> > + __git_cmdlist ()
> > + {
> > + echo $1;
> > + }
> > +else
> > + __git_cmdlist ()
> > + {
> > + if [[ " $AUTHORIZED_CMD_LIST " =~ " $1 " ]] ; then
> > + echo $1
> > + fi
> > + }
> > +fi
> > +
> > __git_list_all_commands ()
> > {
> > local i IFS=" "$'\n'
> > @@ -538,7 +552,7 @@ __git_list_all_commands ()
> > do
> > case $i in
> > *--*) : helper pattern;;
> > - *) echo $i;;
> > + *) __git_cmdlist $i;;
> > esac
> > done
> > }
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index 3cd53f8..5e7d81e 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -12,8 +12,8 @@ complete ()
> > # do nothing
> > return 0
> > }
> > -
> > -. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
> > +AUTHORIZED_CMD_LIST=" checkout show add filter-branch ls-files send-email describe"
> > +. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
> >
> > # We don't need this function to actually join words or do anything special.
> > # Also, it's cleaner to avoid touching bash's internal completion variables.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] mergetools: Add tortoisegitmerge helper
From: Sven Strickroth @ 2013-01-24 22:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Sebastian Schuberth, davvid, Jeff King
In-Reply-To: <7vfw1qbbr4.fsf@alter.siamese.dyndns.org>
Am 24.01.2013 20:51 schrieb Junio C Hamano:
> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>
>> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>> (starting with 1.8.0) in order to make clear that this one has special
>> support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>> version.
>
> Wouldn't it make more sense in such a situation if your users can
> keep using the old "tortoisemerge" configured in their configuration
> and when the renamed one is found the mergetool automatically used
> it, rather than the way your patch is done?
That was also my first idea, however, TortoiseMerge uses parameters as
follows: '-base:"$BASE"'. TortoiseGitMerge uses values separated by
space from keys: '-base "$BASE"'. So both are incompatible (the first
approach has problems with spaces in filenames, the TortoiseGitMerge
approach fixes this).
>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>> index 4314ad0..13cbe5b 100644
>> --- a/Documentation/diff-config.txt
>> +++ b/Documentation/diff-config.txt
>> @@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
>> diff.tool::
>> The diff tool to be used by linkgit:git-difftool[1]. This
>> option overrides `merge.tool`, and has the same valid built-in
>> - values as `merge.tool` minus "tortoisemerge" and plus
>> - "kompare". Any other value is treated as a custom diff tool,
>> + values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
>> + plus "kompare". Any other value is treated as a custom diff tool,
>> and there must be a corresponding `difftool.<tool>.cmd`
>> option.
>
> So in short, two tortoises and kompare are only valid as mergetool
> but cannot be used as difftool? No, I am reading it wrong.
> merge.tool can be used for both, kompare can be used as difftool,
> and two tortoises can only be used as mergetool.
>
> This paragraph needs to be rewritten to unconfuse readers. The
> original is barely intelligible, and it becomes unreadable as the
> set of tools subtracted by "minus" and added by "plus" grows.
But I think this should not be part of this patch.
--
Best regards,
Sven Strickroth
PGP key id F5A9D4C4 @ any key-server
^ permalink raw reply
* Re: [PATCH] mergetools: Add tortoisegitmerge helper
From: Junio C Hamano @ 2013-01-24 22:15 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, Sebastian Schuberth, davvid, Jeff King
In-Reply-To: <5101B0A5.1020308@tu-clausthal.de>
Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> Am 24.01.2013 20:51 schrieb Junio C Hamano:
>> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>>
>>> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>>> (starting with 1.8.0) in order to make clear that this one has special
>>> support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>>> version.
>>
>> Wouldn't it make more sense in such a situation if your users can
>> keep using the old "tortoisemerge" configured in their configuration
>> and when the renamed one is found the mergetool automatically used
>> it, rather than the way your patch is done?
>
> That was also my first idea, however, TortoiseMerge uses parameters as
> follows: '-base:"$BASE"'. TortoiseGitMerge uses values separated by
> space from keys: '-base "$BASE"'. So both are incompatible (the first
> approach has problems with spaces in filenames, the TortoiseGitMerge
> approach fixes this).
OK. Please unconfuse future readers of "git log" by saying why such
a unification does not work in the proposed log message.
>>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>>> index 4314ad0..13cbe5b 100644
>>> --- a/Documentation/diff-config.txt
>>> +++ b/Documentation/diff-config.txt
>>> @@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
>>> diff.tool::
>>> The diff tool to be used by linkgit:git-difftool[1]. This
>>> option overrides `merge.tool`, and has the same valid built-in
>>> - values as `merge.tool` minus "tortoisemerge" and plus
>>> - "kompare". Any other value is treated as a custom diff tool,
>>> + values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
>>> + plus "kompare". Any other value is treated as a custom diff tool,
>>> and there must be a corresponding `difftool.<tool>.cmd`
>>> option.
>>
>> So in short, two tortoises and kompare are only valid as mergetool
>> but cannot be used as difftool? No, I am reading it wrong.
>> merge.tool can be used for both, kompare can be used as difftool,
>> and two tortoises can only be used as mergetool.
>>
>> This paragraph needs to be rewritten to unconfuse readers. The
>> original is barely intelligible, and it becomes unreadable as the
>> set of tools subtracted by "minus" and added by "plus" grows.
>
> But I think this should not be part of this patch.
I agree that it can be done (and it is better to be done) as a
preparatory step. The current text is barely readable, but with
this patch there will be two "minus", and the result becomes
unreadable at that point.
It also could be done as a follow-up documentation readability fix.
^ permalink raw reply
* Re: [PATCH] t9902: Instruct git-completion.bash about a test mode
From: Junio C Hamano @ 2013-01-24 22:29 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: git, Jeff King
In-Reply-To: <201301242304.16078.jn.avila@free.fr>
"Jean-Noël AVILA" <jn.avila@free.fr> writes:
> My rational was to be sure to put the environment variable out of the
> way once the script has been sourced. I can make two alternative
> definitions of __git_list_all_commands () depending on the presence of
> $AUTHORIZED_CMD_LIST if you are worried about performance.
Actually, it does not matter, as once __git_list_all_commands is
run, its result is kept in a variable.
So Peff's
if test -z "$FAKE_COMMAND_LIST"; then
__git_cmdlist() {
git help -a | egrep '^ [a-zA-Z0-9]'
}
else
__git_cmdlist() {
printf '%s' "$FAKE_COMMAND_LIST"
}
fi
would be just as simple if not simpler, does the same thing, and is
sufficient, I think.
The t9902 test is only interested in making sure that the completion
works, and we do not want "git help -a" that omits a subcommand from
its output that is not built in your particular environment to get
in the way, which will not be an issue with this approach.
^ permalink raw reply
* Re: [PATCH v4 1/3] push: further clean up fields of "struct ref"
From: Eric Sunshine @ 2013-01-24 22:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Chris Rorvick
In-Reply-To: <1358978130-12216-2-git-send-email-gitster@pobox.com>
On Wed, Jan 23, 2013 at 4:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The "nonfastforward" and "update" fields are only used while
> deciding what value to assign to the "status" locally in a single
> function. Remove them from the "struct ref".
>
> The "requires_force" field is not used to decide if the proposed
> update requires a --force option to succeed, or to record such a
> decision made elsewhere. It is used by status reporting code that
> the particular update was "forced". Rename it to "forced_udpate",
s/udpate/update/
^ permalink raw reply
* Re: [PATCH] t9902: Instruct git-completion.bash about a test mode
From: Jean-Noël AVILA @ 2013-01-24 22:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vlibi8baq.fsf@alter.siamese.dyndns.org>
Le jeudi 24 janvier 2013 23:29:33, Junio C Hamano a écrit :
> "Jean-Noël AVILA" <jn.avila@free.fr> writes:
>
> > My rational was to be sure to put the environment variable out of the
> > way once the script has been sourced. I can make two alternative
> > definitions of __git_list_all_commands () depending on the presence of
> > $AUTHORIZED_CMD_LIST if you are worried about performance.
>
> Actually, it does not matter, as once __git_list_all_commands is
> run, its result is kept in a variable.
>
> So Peff's
>
> if test -z "$FAKE_COMMAND_LIST"; then
> __git_cmdlist() {
> git help -a | egrep '^ [a-zA-Z0-9]'
> }
> else
> __git_cmdlist() {
> printf '%s' "$FAKE_COMMAND_LIST"
> }
> fi
>
> would be just as simple if not simpler, does the same thing, and is
> sufficient, I think.
>
> The t9902 test is only interested in making sure that the completion
> works, and we do not want "git help -a" that omits a subcommand from
> its output that is not built in your particular environment to get
> in the way, which will not be an issue with this approach.
>
Ah. I totally missed the point. I though that the requested change was
to intersect the list needed for the test with the one provided by the
standard completion.
^ permalink raw reply
* [PATCH] t9902: protect test from stray build artifacts
From: Junio C Hamano @ 2013-01-24 23:07 UTC (permalink / raw)
To: Jeff King; +Cc: Jean-Noël AVILA, git
In-Reply-To: <20130122003954.GA23297@sigill.intra.peff.net>
When you have random build artifacts in your build directory, left
behind by running "make" while on another branch, the "git help -a"
command run by __git_list_all_commands in the completion script that
is being tested does not have a way to know that they are not part
of the subcommands this build will ship. Such extra subcommands may
come from the user's $PATH. They will interfere with the tests that
expect a certain prefix to uniquely expand to a known completion.
Instrument the completion script and give it a way for us to tell
what (subset of) subcommands we are going to ship.
Also add a test to "git --help <prefix><TAB>" expansion. It needs
to show not just commands but some selected documentation pages.
Based on an idea by Jeff King.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
contrib/completion/git-completion.bash | 11 ++++++++++-
t/t9902-completion.sh | 25 ++++++++++++++++++++++++-
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a4c48e1..6139b50 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,10 +531,19 @@ __git_complete_strategy ()
return 1
}
+__git_commands () {
+ if test -n "${GIT_TESTING_COMMAND_COMPLETION:-}"
+ then
+ printf "%s" "${GIT_TESTING_COMMAND_COMPLETION}"
+ else
+ git help -a|egrep '^ [a-zA-Z0-9]'
+ fi
+}
+
__git_list_all_commands ()
{
local i IFS=" "$'\n'
- for i in $(git help -a|egrep '^ [a-zA-Z0-9]')
+ for i in $(__git_commands)
do
case $i in
*--*) : helper pattern;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..adc1372 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -13,6 +13,25 @@ complete ()
return 0
}
+# Be careful when updating this list:
+#
+# (1) The build tree may have build artifact from different branch, or
+# the user's $PATH may have a random executable that may begin
+# with "git-check" that are not part of the subcommands this build
+# will ship, e.g. "check-ignore". The tests for completion for
+# subcommand names tests how "check" is expanded; we limit the
+# possible candidates to "checkout" and "check-attr" to make sure
+# "check-attr", which is known by the filter function as a
+# subcommand to be thrown out, while excluding other random files
+# that happen to begin with "check" to avoid letting them get in
+# the way.
+#
+# (2) A test makes sure that common subcommands are included in the
+# completion for "git <TAB>", and a plumbing is excluded. "add",
+# "filter-branch" and "ls-files" are listed for this.
+
+GIT_TESTING_COMMAND_COMPLETION='add checkout check-attr filter-branch ls-files'
+
. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
# We don't need this function to actually join words or do anything special.
@@ -196,7 +215,6 @@ test_expect_success 'general options plus command' '
test_completion "git --paginate check" "checkout " &&
test_completion "git --git-dir=foo check" "checkout " &&
test_completion "git --bare check" "checkout " &&
- test_completion "git --help des" "describe " &&
test_completion "git --exec-path=foo check" "checkout " &&
test_completion "git --html-path check" "checkout " &&
test_completion "git --no-pager check" "checkout " &&
@@ -207,6 +225,11 @@ test_expect_success 'general options plus command' '
test_completion "git --no-replace-objects check" "checkout "
'
+test_expect_success 'git --help completion' '
+ test_completion "git --help ad" "add " &&
+ test_completion "git --help core" "core-tutorial "
+'
+
test_expect_success 'setup for ref completion' '
echo content >file1 &&
echo more >file2 &&
^ permalink raw reply related
* [regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails
From: Jonathan Nieder @ 2013-01-24 23:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Angus Hammond, git, Mihai Rusu
In-Reply-To: <20120521231017.GJ10981@sigill.intra.peff.net>
Hi Jeff,
Jeff King wrote:
> When we pull the user's name from the GECOS field of the
> passwd file (or generate an email address based on their
> username and hostname), we put the result into a
> static buffer.
[...]
> The conversion is mostly mechanical: replace string copies
> with strbuf equivalents, and access the strbuf.buf directly.
> There are a few exceptions:
This broke /etc/mailname handling. Before:
$ git var GIT_COMMITTER_IDENT
Jonathan Nieder <jrn@mailname.example.com> 1359069165 -0800
After:
$ git var GIT_COMMITTER_IDENT
Jonathan Nieder <mailname.example.com> 1359069192 -0800
The cause:
[...]
> --- a/ident.c
> +++ b/ident.c
> @@ -19,42 +18,27 @@ int user_ident_explicitly_given;
[...]
> -static int add_mailname_host(char *buf, size_t len)
> +static int add_mailname_host(struct strbuf *buf)
> {
> FILE *mailname;
>
> @@ -65,7 +49,7 @@ static int add_mailname_host(char *buf, size_t len)
> strerror(errno));
> return -1;
> }
> - if (!fgets(buf, len, mailname)) {
> + if (strbuf_getline(buf, mailname, '\n') == EOF) {
This clears the strbuf.
How about something like this as a quick fix?
Reported-by: Mihai Rusu <dizzy@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
diff --git a/ident.c b/ident.c
index 73a06a1..cabd73f 100644
--- a/ident.c
+++ b/ident.c
@@ -41,6 +41,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf *name)
static int add_mailname_host(struct strbuf *buf)
{
FILE *mailname;
+ struct strbuf mailnamebuf = STRBUF_INIT;
mailname = fopen("/etc/mailname", "r");
if (!mailname) {
@@ -49,14 +50,17 @@ static int add_mailname_host(struct strbuf *buf)
strerror(errno));
return -1;
}
- if (strbuf_getline(buf, mailname, '\n') == EOF) {
+ if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
if (ferror(mailname))
warning("cannot read /etc/mailname: %s",
strerror(errno));
+ strbuf_release(&mailnamebuf);
fclose(mailname);
return -1;
}
/* success! */
+ strbuf_addbuf(buf, &mailnamebuf);
+ strbuf_release(&mailnamebuf);
fclose(mailname);
return 0;
}
^ permalink raw reply related
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Jeff King @ 2013-01-24 23:27 UTC (permalink / raw)
To: Duy Nguyen
Cc: Stefan Näwe, Armin, Junio C Hamano, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <CACsJy8CEofqi9S8-SDx_O+Ko0i56aRZ4KEJrVnbFum6zzsJrJg@mail.gmail.com>
On Thu, Jan 24, 2013 at 09:14:47PM +0700, Nguyen Thai Ngoc Duy wrote:
> >>> I did. My bisection told me this is the suspect:
> >>>
> >>> ccdc603 (parse_object: try internal cache before reading object db)
> >>
> >> diff --git a/object.c b/object.c
> >> index d8d09f9..6b06297 100644
> >> --- a/object.c
> >> +++ b/object.c
> >> @@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char *sha1)
> >> enum object_type type;
> >> int eaten;
> >> const unsigned char *repl = lookup_replace_object(sha1);
> >> - void *buffer = read_sha1_file(sha1, &type, &size);
> >> + void *buffer;
> >> + struct object *obj;
> >> +
> >> + obj = lookup_object(sha1);
> >> + if (obj && obj->parsed)
> >> + return obj;
> >>
> >> Any chance obj->parsed is 1 but ((struct commit*)obj)->buffer is NULL?
> >> What if you change that "if" to
> >>
> >> if (obj && obj->parsed && (obj->type != OBJ_COMMIT || ((struct commit
> >> *)obj)->buffer))
> >>
> >
> > No more segfault!
>
> Sweet. I have no idea how that fixes it. Maybe Jeff can give some
> explanation after he wakes up.
Ugh. I think I know why it fixes it. We free the commit's buffer as part
of the log traversal, but then later want to access it as part of the
diff. We presumably call parse_object somewhere in the middle to make
sure it is parsed.
Before ccdc603, a side effect of parse_object is that even for a parsed
object, we would fill in the buffer field of a commit or tree. See
parse_object_buffer:
} else if (type == OBJ_COMMIT) {
struct commit *commit = lookup_commit(sha1);
if (commit) {
if (parse_commit_buffer(commit, buffer, size))
return NULL;
if (!commit->buffer) {
commit->buffer = buffer;
eaten = 1;
}
obj = &commit->object;
}
When this patch was originally proposed, I wrote[1]:
On Thu, Jan 05, 2012 at 01:55:22PM -0800, Junio C Hamano wrote:
> > So I think it is safe short of somebody doing some horrible manual
> > munging of a "struct object".
>
> Yeah, I was worried about codepaths like commit-pretty-printing
> might be mucking with the contents of commit->buffer, perhaps
> reencoding the text and then calling parse_object() to get the
> unmodified original back, or something silly like that. But the
> lookup_object() call at the beginning of the parse_object() already
> prevents us from doing such a thing, so we should be OK, I would
> think.
[...]
What saves you is that the parse_*_buffer functions all do nothing
when the object.parsed flag is set, and the code I added makes sure
that object.parsed is set in the object that lookup_object returns.
So yeah, anytime you tweak the contents of commit->buffer but don't
unset the "parsed" flag, you are asking for trouble.
Which is true, but obviously I missed that in addition to calling
parse_*_buffer, which will be a no-op, we _also_ set the buffer
independently. So parse_object was functioning in a belt-and-suspenders
for that case. And I think this is probably the same root cause as the
segfault which came up here:
http://thread.gmane.org/gmane.comp.version-control.git/214366
So what to do?
We can revert ccdc603, but I do not think we need to. We can catch the
problematic cases with something like your patch, but still get the
optimization when the buffer really is already filled in. I think we'd
need to extend your patch to handle trees, too, to be totally correct.
But there are still some loose ends that I note:
1. Making such a change would be parse_object erring on the side of
providing the buffer. But it doesn't actually know if the buffer is
desired or not. For instance, upload-pack benefited from this
optimization, but does not need save_commit_buffer on at all. So
commit->buffer is _always_ NULL there, and that's just fine; we
really don't need to read the object.
Now this may be a bad example, because due to my follow-on patches,
we avoid calling parse_object at all in most cases, so I don't
think it matters any longer to upload-pack. But I suspect there are
other places with similar circumstances. Fundamentally parse_object
doesn't know what the caller is interested in.
2. This means that parse_commit and parse_object behave differently in
this regard. The former will leave the buffer unfilled. Meaning we
may still have issues with code paths that munge the buffer without
resetting the parsed flag, independent of ccdc603 and fixing this.
To me, these highlight that our commit->buffer management is fragile and
is largely about guessing in various circumstances whether somebody will
later want the buffer. I'm not sure of the right solution, though. It
seems like something that inherently blurs the lines between bits of
code (e.g., how should "log" know that a submodule diff might later want
to see the same entry? Should we optimistically free and then make it
easier for the later user to reliably ensure the buffer is primed? Or
should we err on the side of keeping it in place?).
-Peff
[1] http://article.gmane.org/gmane.comp.version-control.git/188000
^ permalink raw reply
* Re: [PATCH] parse_object: clear "parsed" when freeing buffers
From: Jonathon Mah @ 2013-01-24 23:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130124070715.GD610@sigill.intra.peff.net>
On 2013-01-23, at 23:07, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:
>
>> Several areas of code would free buffers for object structs that
>> contained them ("struct tree" and "struct commit"), but without clearing
>> the "parsed" flag. parse_object would clear the flag for "struct tree",
>> but commits would remain in an invalid state (marked as parsed but with
>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
>> invalid objects stay around and can lead to bad behavior.
>>
>> In particular, running pickaxe on all refs in a repository with a cached
>> textconv could segfault. If the textconv cache ref was evaluated first
>> by cmd_log_walk, a subsequent notes_cache_match_validity call would
>> dereference NULL.
>
> Just to be sure I understand, what is going on is something like this?
>
> Log reads all refs, including notes. After showing the notes commit,
> log frees the buffer to save memory. Later, doing the diff on a
> commit causes us to use the notes_cache mechanism. That will look at
> the commit at the tip of the notes ref, which log has already
> processed. It will call parse_commit, but that will do nothing, as the
> parsed flag is already set, but the commit buffer remains NULL.
>
> I wonder if this could be the source of the segfault here, too:
>
> http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355
Indeed, that description matches what I see. The other segfault seems to be in the same place too.
The segfault hits with the following stack (optimization off):
0 git-log parse_commit_header + 39 (pretty.c:738)
1 git-log format_commit_one + 3102 (pretty.c:1138)
2 git-log format_commit_item + 177 (pretty.c:1203)
3 git-log strbuf_expand + 172 (strbuf.c:247)
4 git-log format_commit_message + 196 (pretty.c:1263)
5 git-log notes_cache_match_validity + 215 (notes-cache.c:22)
6 git-log notes_cache_init + 212 (notes-cache.c:41)
7 git-log userdiff_get_textconv + 176 (userdiff.c:301)
8 git-log get_textconv + 66 (diff.c:2233)
9 git-log has_changes + 53 (diffcore-pickaxe.c:205)
10 git-log pickaxe + 299 (diffcore-pickaxe.c:40)
11 git-log diffcore_pickaxe_count + 344 (diffcore-pickaxe.c:275)
12 git-log diffcore_pickaxe + 58 (diffcore-pickaxe.c:289)
13 git-log diffcore_std + 162 (diff.c:4673)
14 git-log log_tree_diff_flush + 43 (log-tree.c:721)
15 git-log log_tree_diff + 566 (log-tree.c:826)
16 git-log log_tree_commit + 63 (log-tree.c:847)
17 git-log cmd_log_walk + 172 (log.c:301)
18 git-log cmd_log + 186 (log.c:568)
19 git-log run_builtin + 475 (git.c:273)
20 git-log handle_internal_command + 199 (git.c:434)
21 git-log main + 149 (git.c:523)
22 libdyld.dylib start + 1
commit->message was freed and nulled earlier in a call to cmd_log_walk. This test reproduces it reliably on my machine:
diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..d7e26ca 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces correct results' '
test_cmp expect actual
'
+cat >expect <<EOF
+./helper other (refs/notes/textconv/magic)
+one
+EOF
+# add empty commit on master to make bug more reproducible
+test_expect_success 'pickaxe with cached textconv' '
+ git commit --allow-empty -m another &&
+ git log -S other --pretty=tformat:%s%d --all >actual &&
+ test_cmp expect actual
+'
+
test_done
Jonathon Mah
me@JonathonMah.com
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox