* 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 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] 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] 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
* [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
* [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 2/4] git-mergetool: remove redundant assignment
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>
TOOL_MODE is set at the top of git-mergetool.sh so there is no need to
set it again in show_tool_help. Removing this lets us re-use
show_tool_help in git-difftool.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-mergetool--lib.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 89a857f..1748315 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -175,7 +175,6 @@ list_merge_tool_candidates () {
}
show_tool_help () {
- TOOL_MODE=merge
list_merge_tool_candidates
unavailable= available= LF='
'
--
1.8.1
^ permalink raw reply related
* [PATCH 1/4] git-mergetool: move show_tool_help to mergetool--lib
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>
This is the first step in unifying "git difftool --tool-help" and
"git mergetool --tool-help".
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-mergetool--lib.sh | 37 +++++++++++++++++++++++++++++++++++++
git-mergetool.sh | 37 -------------------------------------
2 files changed, 37 insertions(+), 37 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f013a03..89a857f 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -174,6 +174,43 @@ list_merge_tool_candidates () {
esac
}
+show_tool_help () {
+ TOOL_MODE=merge
+ list_merge_tool_candidates
+ unavailable= available= LF='
+'
+ for i in $tools
+ do
+ merge_tool_path=$(translate_merge_tool_path "$i")
+ if type "$merge_tool_path" >/dev/null 2>&1
+ then
+ available="$available$i$LF"
+ else
+ unavailable="$unavailable$i$LF"
+ fi
+ done
+ if test -n "$available"
+ then
+ echo "'git mergetool --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."
+ fi
+ if test -n "$unavailable"
+ then
+ echo
+ echo 'The following tools are valid, but not currently available:'
+ echo "$unavailable" | sort | sed -e 's/^/ /'
+ fi
+ if test -n "$unavailable$available"
+ then
+ echo
+ echo "Some of the tools listed above only work in a windowed"
+ echo "environment. If run in a terminal-only session, they will fail."
+ fi
+ exit 0
+}
+
guess_merge_tool () {
list_merge_tool_candidates
echo >&2 "merge tool candidates: $tools"
diff --git a/git-mergetool.sh b/git-mergetool.sh
index c50e18a..c0ee9aa 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -315,43 +315,6 @@ merge_file () {
return 0
}
-show_tool_help () {
- TOOL_MODE=merge
- list_merge_tool_candidates
- unavailable= available= LF='
-'
- for i in $tools
- do
- merge_tool_path=$(translate_merge_tool_path "$i")
- if type "$merge_tool_path" >/dev/null 2>&1
- then
- available="$available$i$LF"
- else
- unavailable="$unavailable$i$LF"
- fi
- done
- if test -n "$available"
- then
- echo "'git mergetool --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."
- fi
- if test -n "$unavailable"
- then
- echo
- echo 'The following tools are valid, but not currently available:'
- echo "$unavailable" | sort | sed -e 's/^/ /'
- fi
- if test -n "$unavailable$available"
- then
- echo
- echo "Some of the tools listed above only work in a windowed"
- echo "environment. If run in a terminal-only session, they will fail."
- fi
- exit 0
-}
-
prompt=$(git config --bool mergetool.prompt || echo true)
while test $# != 0
--
1.8.1
^ permalink raw reply related
* [PATCH 0/4] Fix "git difftool --tool-help"
From: John Keeping @ 2013-01-24 19:55 UTC (permalink / raw)
To: git; +Cc: David Aguilar
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.
This series fixes this by changing it to simply delegate to a function
in git-mergetool--lib.
The first three patches are just refactorings to move the show_tool_help
function from git-mergetool to git-mergetool--lib and make it usable by
git-difftool. The final patch switches git-difftool to use this
function.
John Keeping (4):
git-mergetool: move show_tool_help to mergetool--lib
git-mergetool: remove redundant assignment
git-mergetool: don't hardcode 'mergetool' in show_tool_help
git-difftool: use git-mergetool--lib for "--tool-help"
git-difftool.perl | 57 ++++++++-------------------------------------------
git-mergetool--lib.sh | 38 ++++++++++++++++++++++++++++++++++
git-mergetool.sh | 37 ---------------------------------
3 files changed, 46 insertions(+), 86 deletions(-)
--
1.8.1
^ permalink raw reply
* Re: [PATCH] mergetool: clean up temp files when aborted
From: Junio C Hamano @ 2013-01-24 19:54 UTC (permalink / raw)
To: Phil Hord; +Cc: git, phil.hord, Theodore Ts'o
In-Reply-To: <1359051829-21331-1-git-send-email-hordp@cisco.com>
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.
>
> Remove the temporary files by calling the cleanup_temp_files when the
> user chooses to abort the mergetool operation.
>
> It looks like 'cleanup_temp_files' without the --save-backups option is
> the correct thing to do, and this is how this commit is implemented. But
> some other paths do use --save-backups resulting in a foo.c.orig file
> being left behind. That seems to be a different bug, though.
>
> Signed-off-by: Phil Hord <hordp@cisco.com>
> ---
> git-mergetool.sh | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index c50e18a..bb93b70 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -90,6 +90,7 @@ resolve_symlink_merge () {
> return 0
> ;;
> [aA]*)
> + cleanup_temp_files
> return 1
> ;;
> esac
> @@ -118,6 +119,7 @@ resolve_deleted_merge () {
> return 0
> ;;
> [aA]*)
> + cleanup_temp_files
> return 1
> ;;
> esac
^ permalink raw reply
* Re: [PATCH] mergetools: Add tortoisegitmerge helper
From: Junio C Hamano @ 2013-01-24 19:51 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, Sebastian Schuberth, davvid, Jeff King
In-Reply-To: <50FCFBBB.2080305@tu-clausthal.de>
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? It seems that you are
forcing all the users to reconfigure or retrain their fingers. Is
that the best we can do? Is it too cumbersome to autodetect the
presense of tortoisegitmerge and redirect a request for tortoisemerge
to it, perhaps using translate_merge_tool_path (cf. mergetools/bc3)?
Assuming that people that have both variants will always want
mergetool to use tortoisegitmerge, that is. If there are some
features missing from or extra bugs in tortoisegitmerge that makes
some people favor tortoisemerge, then giving two choices like your
patch does may make more sense. I only know the difference between
the two from your four-line description above, but it does not look
like it is the case.
> 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.
^ permalink raw reply
* [PATCH] git-cvsimport.txt: cvsps-2 is deprecated
From: John Keeping @ 2013-01-24 19:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric S. Raymond, Chris Rorvick
In-Reply-To: <7vip6ndveb.fsf@alter.siamese.dyndns.org>
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.
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.
--
1.8.1
^ permalink raw reply related
* Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects
From: Junio C Hamano @ 2013-01-24 19:15 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jens Lehmann
In-Reply-To: <1359016940-18849-6-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> read-cache.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index fda78bc..4579215 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1720,6 +1720,26 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,
> ce->name + common, ce_namelen(ce) - common);
> }
>
> + if (object_database_contaminated) {
> + struct object_info oi;
> + switch (ce->ce_mode) {
> + case S_IFGITLINK:
> + break;
> + case S_IFDIR:
> + if (sha1_object_info_extended(ce->sha1, &oi) != OBJ_TREE ||
This case should never happen. Do we store any tree object in the
index in the first place?
> + (oi.alt && oi.alt->external))
> + die("cannot create index referring to an external tree %s",
> + sha1_to_hex(ce->sha1));
> + break;
> + default:
> + if (sha1_object_info_extended(ce->sha1, &oi) != OBJ_BLOB ||
> + (oi.alt && oi.alt->external))
> + die("cannot create index referring to an external blob %s",
> + sha1_to_hex(ce->sha1));
> + break;
> + }
> + }
> +
> result = ce_write(c, fd, ondisk, size);
> free(ondisk);
> return result;
I think the check you want to add is to the cache-tree code; before
writing the new index out, if you suspect you might have called
cache_tree_update() before, invalidate any hierarchy that is
recorded to produce a tree object that refers to an object that is
only available through external object store.
As to the logic to check if a object is something that we should
refuse to create a new dependent, I think you should not butcher
sha1_object_info_extended(), and instead add a new call that checks
if a given SHA-1 yields an object if you ignore alternate object
databases that are marked as "external", whose signature may
resemble:
int has_sha1_file_proper(const unsigned char *sha1);
or something.
This is a tangent, but in addition, you may want to add an even
narrower variant that checks the same but ignoring _all_ alternate
object databases, "external" or not:
int has_sha1_file_local(const unsigned char *sha1);
That may be useful if we want to later make the alternate safer to
use; instead of letting the codepath to create an object ask
has_sha1_file() to see if it already exists and refrain from writing
a new copy, we switch to ask has_sha1_file_locally() and even if an
alternate object database we borrow from has it, we keep our own
copy in our repository.
Not tested beyond syntax, but rebasing something like this patch on
top of your "mark external sources" change may take us closer to
that.
cache.h | 2 ++
sha1_file.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 1f96f65..8d651b6 100644
--- a/cache.h
+++ b/cache.h
@@ -766,6 +766,8 @@ extern int move_temp_to_file(const char *tmpfile, const char *filename);
extern int has_sha1_pack(const unsigned char *sha1);
extern int has_sha1_file(const unsigned char *sha1);
+extern int has_sha1_file_proper(const unsigned char *sha1);
+extern int has_sha1_file_local(const unsigned char *sha1);
extern int has_loose_object_nonlocal(const unsigned char *sha1);
extern int has_pack_index(const unsigned char *sha1);
diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..1a3192a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -420,11 +420,18 @@ static int has_loose_object_local(const unsigned char *sha1)
return !access(name, F_OK);
}
-int has_loose_object_nonlocal(const unsigned char *sha1)
+enum odb_origin {
+ odb_default = 0, odb_proper, odb_local
+};
+
+static int has_loose_object_nonlocal_limited(const unsigned char *sha1,
+ enum odb_origin origin)
{
struct alternate_object_database *alt;
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
+ if (origin == odb_proper && 0 /* alt->external */)
+ continue;
fill_sha1_path(alt->name, sha1);
if (!access(alt->base, F_OK))
return 1;
@@ -432,6 +439,11 @@ int has_loose_object_nonlocal(const unsigned char *sha1)
return 0;
}
+int has_loose_object_nonlocal(const unsigned char *sha1)
+{
+ return has_loose_object_nonlocal_limited(sha1, odb_default);
+}
+
static int has_loose_object(const unsigned char *sha1)
{
return has_loose_object_local(sha1) ||
@@ -2062,12 +2074,28 @@ int is_pack_valid(struct packed_git *p)
return !open_packed_git(p);
}
+static int limit_pack_odb_origin(enum odb_origin origin, struct packed_git *p)
+{
+ switch (origin) {
+ default:
+ return 0;
+ case odb_proper:
+ return 0; /* p->external */
+ case odb_local:
+ return !p->pack_local;
+ }
+}
+
static int fill_pack_entry(const unsigned char *sha1,
struct pack_entry *e,
- struct packed_git *p)
+ struct packed_git *p,
+ enum odb_origin origin)
{
off_t offset;
+ if (limit_pack_odb_origin(origin, p))
+ return 0;
+
if (p->num_bad_objects) {
unsigned i;
for (i = 0; i < p->num_bad_objects; i++)
@@ -2096,7 +2124,8 @@ static int fill_pack_entry(const unsigned char *sha1,
return 1;
}
-static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
+static int find_pack_entry_limited(const unsigned char *sha1, struct pack_entry *e,
+ enum odb_origin origin)
{
struct packed_git *p;
@@ -2104,11 +2133,11 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
if (!packed_git)
return 0;
- if (last_found_pack && fill_pack_entry(sha1, e, last_found_pack))
+ if (last_found_pack && fill_pack_entry(sha1, e, last_found_pack, origin))
return 1;
for (p = packed_git; p; p = p->next) {
- if (p == last_found_pack || !fill_pack_entry(sha1, e, p))
+ if (p == last_found_pack || !fill_pack_entry(sha1, e, p, origin))
continue;
last_found_pack = p;
@@ -2117,6 +2146,11 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
return 0;
}
+static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
+{
+ return find_pack_entry_limited(sha1, e, odb_default);
+}
+
struct packed_git *find_sha1_pack(const unsigned char *sha1,
struct packed_git *packs)
{
@@ -2630,6 +2664,22 @@ int has_sha1_file(const unsigned char *sha1)
return has_loose_object(sha1);
}
+int has_sha1_file_local(const unsigned char *sha1)
+{
+ struct pack_entry e;
+ if (find_pack_entry_limited(sha1, &e, odb_local))
+ return 1;
+ return has_loose_object_local(sha1);
+}
+
+int has_sha1_file_proper(const unsigned char *sha1)
+{
+ struct pack_entry e;
+ return (!!find_pack_entry_limited(sha1, &e, odb_proper) ||
+ has_loose_object_local(sha1) ||
+ has_loose_object_nonlocal_limited(sha1, odb_proper));
+}
+
static void check_tree(const void *buf, size_t size)
{
struct tree_desc desc;
^ permalink raw reply related
* Re: [PATCH] send-email: Honor multi-part email messages
From: Junio C Hamano @ 2013-01-24 19:11 UTC (permalink / raw)
To: Shumkin Alexey; +Cc: git
In-Reply-To: <CAEFUfsE-Ca=4jAyoaCtoeVYX0Etsn2Cj42OJm+nu4042JPJaBg@mail.gmail.com>
Shumkin Alexey <alex.crezoff@gmail.com> writes:
> I've meant you did not reply to my last message in this thread
> http://thread.gmane.org/gmane.comp.version-control.git/181791
Please repost the patch (with possible updates) for review when
attempting to resurrect an ancient topic, instead of forcing people
to hunt for an ancient message.
I think your patch is wrong. What happens when we see a Subject:
line with a non-ascii on it that causes an early return of the loop
before your new code has a chance to see Content-Type: header?
^ permalink raw reply
* Re: [PATCH] send-email: Honor multi-part email messages
From: Shumkin Alexey @ 2013-01-24 18:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v622mcuyb.fsf@alter.siamese.dyndns.org>
I've meant you did not reply to my last message in this thread
http://thread.gmane.org/gmane.comp.version-control.git/181791
2013/1/24 Junio C Hamano <gitster@pobox.com>
>
> Alexey Shumkin <alex.crezoff@gmail.com> writes:
>
> > Bump
>
> Bump what???
^ permalink raw reply
* [PATCH] mergetool: clean up temp files when aborted
From: Phil Hord @ 2013-01-24 18:23 UTC (permalink / raw)
To: git; +Cc: phil.hord, Theodore Ts'o, Junio C Hamano, Phil Hord
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.
Remove the temporary files by calling the cleanup_temp_files when the
user chooses to abort the mergetool operation.
It looks like 'cleanup_temp_files' without the --save-backups option is
the correct thing to do, and this is how this commit is implemented. But
some other paths do use --save-backups resulting in a foo.c.orig file
being left behind. That seems to be a different bug, though.
Signed-off-by: Phil Hord <hordp@cisco.com>
---
git-mergetool.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index c50e18a..bb93b70 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -90,6 +90,7 @@ resolve_symlink_merge () {
return 0
;;
[aA]*)
+ cleanup_temp_files
return 1
;;
esac
@@ -118,6 +119,7 @@ resolve_deleted_merge () {
return 0
;;
[aA]*)
+ cleanup_temp_files
return 1
;;
esac
--
1.8.1.1.ga649ac9.dirty
^ permalink raw reply related
* Re: [PATCH v3 01/10] wildmatch: fix "**" special case
From: Junio C Hamano @ 2013-01-24 18:22 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8CtV-ngy4iGm3Vn3bu9XwpSrZ_AeWpPxTC2TY_qXv=Cxw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> I don't think we need to support two different sets of wildcards in
> the long run. I'm thinking of adding ":(glob)" with WM_PATHNAME.
> Pathspec without :(glob) still uses the current wildcards (i.e. no
> FNM_PATHNAME). At some point, like 2.0, we either switch the behavior
> of patterns-without-:(glob) to WM_PATHNAME, or just disable wildcards
> when :(glob) is not present.
Yeah, I think that is sensible.
I am meaning to merge your retire-fnmatch topic to 'master'. What
should the Release Notes say for the upcoming release?
You can build with USE_WILDMATCH=YesPlease to use a replacement
implementation of pattern matching logic used for pathname-like
things, e.g. refnames and paths in the repository. The new
implementation is not expected change the existing behaviour of
Git at all, except for "git for-each-ref" where you can now say
"refs/**/master" and match with both refs/heads/master and
refs/remotes/origin/master. In future versions of Git, we plan
to use this new implementation in wider places (e.g. "git log
'**/t*.sh'" may find commits that touch a shell script whose
name begins with "t" at any level), but we are not there yet.
By building with USE_WILDMATCH, using the resulting Git daily
and reporting when you find breakages, you can help us get
closer to that goal.
^ permalink raw reply
* Re: [PATCH] send-email: Honor multi-part email messages
From: Junio C Hamano @ 2013-01-24 18:11 UTC (permalink / raw)
To: Alexey Shumkin; +Cc: git
In-Reply-To: <20130124143816.7c33fc1c@ashu.dyn1.rarus.ru>
Alexey Shumkin <alex.crezoff@gmail.com> writes:
> Bump
Bump what???
^ permalink raw reply
* Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from
From: Junio C Hamano @ 2013-01-24 17:45 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jens Lehmann
In-Reply-To: <1359016940-18849-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> We currently know if an object is loose or packed. We do not know if
> it's from the repo's object database, or via alternates
> mechanism. With this patch, sha1_object_info_extended() can tell if an
> object comes from alternates source (and which one).
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> How about this way instead: we keep track of where objects come from
> so we can verify object source when we create or update something
> that contains SHA-1.
The overall approach taken by this series may be worth considering, but
I do not think the check implemented here is correct.
An object may be found in an alternate odb but we may also have our
own copy of the same object. You need to prove that a suspicious
object is visible to us *ONLY* through add_submodule_odb().
Once you do add_submodule_odb() to contaminate our object pool, you
make everything a suspicious object that needs to be checked; that
is the worst part of the story.
If I understand the test case that triggers the issue correctly, it
makes a merge in the top-level superproject, and while doing so,
tries to three-way merge gitlink changes, so that it can detect
fast-forward situation inside the submodule. It should just be done
with running "git merge-base A B" there; after all, we are in the
tree-wide merge of the top-level superproject, which is already a
heavy-weight operation. I do not see a reason to make it more brittle
than necessary.
If we rip out add_submodule_odb() we do not have to ever worry about
this kind of object database breakages that are hard to track down.
It is an optimization to allow the code become more brittle, violate
the project boundary, and break the object database faster as a
result. That is not a good optimization at all.
^ permalink raw reply
* Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement
From: Junio C Hamano @ 2013-01-24 17:25 UTC (permalink / raw)
To: Jeff King; +Cc: git, git-dev
In-Reply-To: <20130124075008.GA3249@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> And yeah, this should use lookup_unknown_object to extend the
> optimization to mark_our_ref (and avoid removing it for the
> ref-advertisement case, of course).
Thanks for sanity checking. Here is what is queued at the bottom of
the hide-refs topic.
-- >8 --
Date: Fri, 18 Jan 2013 15:48:49 -0800
Subject: [PATCH] upload-pack: share more code
We mark the objects pointed at our refs with "OUR_REF" flag in two
functions (mark_our_ref() and send_ref()), but we can just use the
former as a helper for the latter.
Update the way mark_our_ref() prepares in-core object to use
lookup_unknown_object() to delay reading the actual object data,
just like we did in 435c833 (upload-pack: use peel_ref for ref
advertisements, 2012-10-04).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
upload-pack.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 95d8313..3dd220d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -722,15 +722,28 @@ static void receive_needs(void)
free(shallows.objects);
}
+static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
+{
+ struct object *o = lookup_unknown_object(sha1);
+ if (!o)
+ die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1));
+ if (!(o->flags & OUR_REF)) {
+ o->flags |= OUR_REF;
+ nr_our_refs++;
+ }
+ return 0;
+}
+
static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
{
static const char *capabilities = "multi_ack thin-pack side-band"
" side-band-64k ofs-delta shallow no-progress"
" include-tag multi_ack_detailed";
- struct object *o = lookup_unknown_object(sha1);
const char *refname_nons = strip_namespace(refname);
unsigned char peeled[20];
+ mark_our_ref(refname, sha1, flag, cb_data);
+
if (capabilities)
packet_write(1, "%s %s%c%s%s agent=%s\n",
sha1_to_hex(sha1), refname_nons,
@@ -740,27 +753,11 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
else
packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
capabilities = NULL;
- if (!(o->flags & OUR_REF)) {
- o->flags |= OUR_REF;
- nr_our_refs++;
- }
if (!peel_ref(refname, peeled))
packet_write(1, "%s %s^{}\n", sha1_to_hex(peeled), refname_nons);
return 0;
}
-static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
-{
- struct object *o = parse_object(sha1);
- if (!o)
- die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1));
- if (!(o->flags & OUR_REF)) {
- o->flags |= OUR_REF;
- nr_our_refs++;
- }
- return 0;
-}
-
static void upload_pack(void)
{
if (advertise_refs || !stateless_rpc) {
--
1.8.1.1.525.gdace530
^ permalink raw reply related
* Re: [PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
From: Junio C Hamano @ 2013-01-24 17:19 UTC (permalink / raw)
To: Jeff King; +Cc: git, Chris Rorvick
In-Reply-To: <20130124065842.GC610@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I wonder if we can reword it to explain more about why we do not have
> the object, without getting too inaccurate. Something like:
>
> Updates were rejected because the remote contains objects that you do
> not have locally. This is usually caused by another repository pushing
> to the same ref. You may want to first merge the remote changes (e.g.,
> 'git pull') before pushing again.
>
> I was also tempted to s/objects/work/, which is more vague, but is less
> jargon-y for new users who do not know how git works.
After all this is "hint", and there is a value in being more
approachable at the cost of being less accurate, over being
impenetrable to achieve perfect correctness.
> Also, how should this interact with the checkout-then-pull-then-push
> advice? We make a distinction for the non-fastforward case between HEAD
> and other refs. Should we be making the same distinction here?
Possibly, but I am not among the people who cared most about the
distinction there; with the default behaviour switching to 'simple',
that distinction will start mattering even less, I suspect.
^ permalink raw reply
* Re: [PATCH] parse_object: clear "parsed" when freeing buffers
From: Junio C Hamano @ 2013-01-24 16:51 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathon Mah, git
In-Reply-To: <20130124063259.GA610@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The ensure_commit_buffer function could look something like:
>
> int ensure_commit_buffer(struct commit *item)
> {
> enum object_type type;
> unsigned long size;
>
> if (!item)
> return -1;
> if (!item->object.parsed)
> return parse_commit(item);
> if (item->buffer)
> return 0;
>
> item->buffer = read_sha1_file(item->object.sha1, &type, &size);
> if (!item->buffer)
> return error("Could not read %s",
> sha1_to_hex(item->object.sha1);
> return 0;
> }
The more important issue is when to release them.
Only the subcommands that know that what they are doing do not need
to access commit->buffer and they operate solely on parsed data are
suppposed to flip the save_commit_buffer bit off to reclaim the
memory early, so new callers that violate that assumption may want
to call ensure_*, but if they did so when somebody cares about the
memory pressure enough to turn the bit off need to free the buffer
themselves after they use it. Otherwise the resulting program may
still be "correct" but we will end up keeping buffer that nobody
will use, just in case it is asked again.
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Duy Nguyen @ 2013-01-24 14:14 UTC (permalink / raw)
To: Stefan Näwe; +Cc: Jeff King, Armin, git@vger.kernel.org
In-Reply-To: <51013FDD.5030004@atlas-elektronik.com>
On Thu, Jan 24, 2013 at 9:06 PM, Stefan Näwe
<stefan.naewe@atlas-elektronik.com> wrote:
> Am Donnerstag, 24. Januar 2013 14:40:47 schrieb Duy Nguyen:
>> On Thu, Jan 24, 2013 at 7:11 PM, Stefan Näwe
>> <stefan.naewe@atlas-elektronik.com> wrote:
>>>> Does it fail with older versions of git? If so, can you bisect?
>>>
>>> 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.
--
Duy
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Stefan Näwe @ 2013-01-24 14:06 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, Armin, git@vger.kernel.org
In-Reply-To: <CACsJy8B9O=A26_=sv1JEYdtazsWa4khZkqpTgFSSTs_RGGPZqQ@mail.gmail.com>
Am Donnerstag, 24. Januar 2013 14:40:47 schrieb Duy Nguyen:
> On Thu, Jan 24, 2013 at 7:11 PM, Stefan Näwe
> <stefan.naewe@atlas-elektronik.com> wrote:
>>> Does it fail with older versions of git? If so, can you bisect?
>>
>> 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!
> Also you did not encode commits in any specific encoding,
We're using Git for Windows and some commits contain 'umlauts' (äöü).
But those characters should be encoded in UTF-8, shouldn't they?
But the 'git log...' only crashes on a Debian/Linux machine.
> nor set i18n.logOutputEncoding?
It's not set.
(only i18n.filesEncoding is set to utf-8 on my machine)
Oh, and it's not crashing if I do:
git log -p --submodule |cat
Stefan
--
----------------------------------------------------------------
/dev/random says: Dumb luck beats sound planning every time. Trust me.
python -c "print
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Duy Nguyen @ 2013-01-24 13:40 UTC (permalink / raw)
To: Stefan Näwe; +Cc: Jeff King, Armin, git@vger.kernel.org
In-Reply-To: <510124F5.9090505@atlas-elektronik.com>
On Thu, Jan 24, 2013 at 7:11 PM, Stefan Näwe
<stefan.naewe@atlas-elektronik.com> wrote:
>> Does it fail with older versions of git? If so, can you bisect?
>
> 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))
??
Also you did not encode commits in any specific encoding, nor set
i18n.logOutputEncoding?
--
Duy
^ 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