Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 4:48 AM, Jeff King p...@peff.net wrote: - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0; + return fnmatch_icase_mem(pattern, patternlen, +name, namelen, +FNM_PATHNAME) == 0; } I think you (or Junio) should rebase this on maint. Since c41244e (included in maint), this call is turned to wildmatch(WM_PATHNAME) and WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like foo/**/bar. A diff between next and pu shows me that WM_PATHNAME is incorrectly converted to FNM_PATHNAME. I hope that is the cause of all breakages Junio found out on pu. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 3:45 PM, Duy Nguyen pclo...@gmail.com wrote: On Fri, Mar 29, 2013 at 4:48 AM, Jeff King p...@peff.net wrote: - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0; + return fnmatch_icase_mem(pattern, patternlen, +name, namelen, +FNM_PATHNAME) == 0; } I think you (or Junio) should rebase this on maint. Since c41244e (included in maint), this call is turned to wildmatch(WM_PATHNAME) and WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like foo/**/bar. A diff between next and pu shows me that WM_PATHNAME is incorrectly converted to FNM_PATHNAME. I hope that is the cause of all breakages Junio found out on pu. Just tested. t0003 and t3001 on 'pu' work for me because I have USE_WILDMATCH on (which turns FNM_PATHNAME to WM_PATHNAME). Both break without USE_WILDMATCH. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] difftool improvements
This is a consolidated series replacing jk/t7800-modernize. The patches here have been rebased on master. Patch 1 is new and moves the test added by commit 02c5631 (difftool --dir-diff: symlink all files matching the working tree) to the end of the test file. This means that once this is applied the second patch and the final three patches can become independent branches. Patch 2 is a fix for a long standing deficiency, but the potential for this to happen has been increased by the commit mentioned above. It has already been through a couple of iterations [1]. The final three patches are the same as the current jk/t7800-modernize, with one tweaked commit message. [1] http://article.gmane.org/gmane.comp.version-control.git/219100 John Keeping (5): t7800: move '--symlinks' specific test to the end difftool: don't overwrite modified files t7800: don't hide grep output t7800: fix tests when difftool uses --no-symlinks t7800: run --dir-diff tests with and without symlinks git-difftool.perl | 73 +--- t/t7800-difftool.sh | 105 ++-- 2 files changed, 127 insertions(+), 51 deletions(-) -- 1.8.2.411.g65a544e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] t7800: move '--symlinks' specific test to the end
This will group the tests more logically when we introduce a helper to run most --dir-diff tests with both --symlinks and --no-symlinks. Signed-off-by: John Keeping j...@keeping.me.uk --- t/t7800-difftool.sh | 30 +++--- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index c6d6b1c..e6a16cd 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -340,6 +340,21 @@ test_expect_success PERL 'difftool --dir-diff' ' stdin_contains file output ' +test_expect_success PERL 'difftool --dir-diff ignores --prompt' ' + git difftool --dir-diff --prompt --extcmd ls branch output + stdin_contains sub output + stdin_contains file output +' + +test_expect_success PERL 'difftool --dir-diff from subdirectory' ' + ( + cd sub + git difftool --dir-diff --extcmd ls branch output + stdin_contains sub output + stdin_contains file output + ) +' + write_script .git/CHECK_SYMLINKS \EOF for f in file file2 sub/sub do @@ -362,19 +377,4 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage test_cmp actual expect ' -test_expect_success PERL 'difftool --dir-diff ignores --prompt' ' - git difftool --dir-diff --prompt --extcmd ls branch output - stdin_contains sub output - stdin_contains file output -' - -test_expect_success PERL 'difftool --dir-diff from subdirectory' ' - ( - cd sub - git difftool --dir-diff --extcmd ls branch output - stdin_contains sub output - stdin_contains file output - ) -' - test_done -- 1.8.2.411.g65a544e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] difftool: don't overwrite modified files
After running the user's diff tool, git-difftool will copy any files
that differ between the working tree and the temporary tree. This is
useful when the user edits the file in their diff tool but is wrong if
they edit the working tree file while examining the diff.
Instead of copying unconditionally when the files differ, create and
index from the working tree files and only copy the temporary file back
if it was modified and the working tree file was not. If both files
have been modified, print a warning and exit with an error.
Note that we cannot use an existing index in git-difftool since those
contain the modified files that need to be checked out but here we are
looking at those files which are copied from the working tree and not
checked out. These are precisely the files which are not in the
existing indices.
Signed-off-by: John Keeping j...@keeping.me.uk
---
Changes since v2:
- Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails
git-difftool.perl | 73 +++--
t/t7800-difftool.sh | 30 ++
2 files changed, 89 insertions(+), 14 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 663640d..844f619 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,9 +13,9 @@
use 5.008;
use strict;
use warnings;
+use Error qw(:try);
use File::Basename qw(dirname);
use File::Copy;
-use File::Compare;
use File::Find;
use File::stat;
use File::Path qw(mkpath rmtree);
@@ -88,14 +88,45 @@ sub use_wt_file
my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
my $null_sha1 = '0' x 40;
- if ($sha1 eq $null_sha1) {
- return 1;
- } elsif (not $symlinks) {
+ if ($sha1 ne $null_sha1 and not $symlinks) {
return 0;
}
my $wt_sha1 = $repo-command_oneline('hash-object', $workdir/$file);
- return $sha1 eq $wt_sha1;
+ my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+ return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+ my ($repo_path, $index, $worktree) = @_;
+ $ENV{GIT_INDEX_FILE} = $index;
+ $ENV{GIT_WORK_TREE} = $worktree;
+ my $must_unset_git_dir = 0;
+ if (not defined($ENV{GIT_DIR})) {
+ $must_unset_git_dir = 1;
+ $ENV{GIT_DIR} = $repo_path;
+ }
+
+ my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+ my @gitargs = qw/diff-files --name-only -z/;
+ try {
+ Git::command_oneline(@refreshargs);
+ } catch Git::Error::Command with {};
+
+ my $line = Git::command_oneline(@gitargs);
+ my @files;
+ if (defined $line) {
+ @files = split('\0', $line);
+ } else {
+ @files = ();
+ }
+
+ delete($ENV{GIT_INDEX_FILE});
+ delete($ENV{GIT_WORK_TREE});
+ delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+ return map { $_ = 1 } @files;
}
sub setup_dir_diff
@@ -121,6 +152,7 @@ sub setup_dir_diff
my $null_sha1 = '0' x 40;
my $lindex = '';
my $rindex = '';
+ my $wtindex = '';
my %submodule;
my %symlink;
my @working_tree = ();
@@ -174,8 +206,12 @@ EOF
}
if ($rmode ne $null_mode) {
- if (use_wt_file($repo, $workdir, $dst_path, $rsha1,
$symlinks)) {
- push(@working_tree, $dst_path);
+ my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+ $dst_path, $rsha1,
+ $symlinks);
+ if ($use) {
+ push @working_tree, $dst_path;
+ $wtindex .= $rmode $wt_sha1\t$dst_path\0;
} else {
$rindex .= $rmode $rsha1\t$dst_path\0;
}
@@ -218,6 +254,12 @@ EOF
$rc = system('git', 'checkout-index', '--all', --prefix=$rdir/);
exit_cleanup($tmpdir, $rc) if $rc != 0;
+ $ENV{GIT_INDEX_FILE} = $tmpdir/wtindex;
+ ($inpipe, $ctx) =
+ $repo-command_input_pipe(qw(update-index --info-only -z
--index-info));
+ print($inpipe $wtindex);
+ $repo-command_close_pipe($inpipe, $ctx);
+
# If $GIT_DIR was explicitly set just for the update/checkout
# commands, then it should be unset before continuing.
delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
@@ -390,19 +432,22 @@ sub dir_diff
# should be copied back to the working tree.
# Do not copy back files when symlinks are used and the
# external tool did not replace the original link with a file.
+ my %wt_modified = changed_files($repo-repo_path(),
+ $tmpdir/wtindex, $workdir);
+ my %tmp_modified = changed_files($repo-repo_path(),
+ $tmpdir/wtindex, $b);
[PATCH 3/5] t7800: don't hide grep output
Remove the stdin_contains and stdin_doesnt_contain helper functions
which add nothing but hide the output of grep, hurting debugging.
Suggested-by: Johannes Sixt j.s...@viscovery.net
Signed-off-by: John Keeping j...@keeping.me.uk
---
t/t7800-difftool.sh | 44 +---
1 file changed, 17 insertions(+), 27 deletions(-)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 017f55a..9fd09db 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,16 +23,6 @@ prompt_given ()
test $prompt = Launch 'test-tool' [Y/n]: branch
}
-stdin_contains ()
-{
- grep /dev/null $1
-}
-
-stdin_doesnot_contain ()
-{
- ! stdin_contains $1
-}
-
# Create a file on master and change it on branch
test_expect_success PERL 'setup' '
echo master file
@@ -296,24 +286,24 @@ test_expect_success PERL 'setup with 2 files different' '
test_expect_success PERL 'say no to the first file' '
(echo n echo) input
git difftool -x cat branch input output
- stdin_contains m2 output
- stdin_contains br2 output
- stdin_doesnot_contain master output
- stdin_doesnot_contain branch output
+ grep m2 output
+ grep br2 output
+ ! grep master output
+ ! grep branch output
'
test_expect_success PERL 'say no to the second file' '
(echo echo n) input
git difftool -x cat branch input output
- stdin_contains master output
- stdin_contains branch output
- stdin_doesnot_contain m2 output
- stdin_doesnot_contain br2 output
+ grep master output
+ grep branch output
+ ! grep m2 output
+ ! grep br2 output
'
test_expect_success PERL 'difftool --tool-help' '
git difftool --tool-help output
- stdin_contains tool output
+ grep tool output
'
test_expect_success PERL 'setup change in subdirectory' '
@@ -330,28 +320,28 @@ test_expect_success PERL 'setup change in subdirectory' '
test_expect_success PERL 'difftool -d' '
git difftool -d --extcmd ls branch output
- stdin_contains sub output
- stdin_contains file output
+ grep sub output
+ grep file output
'
test_expect_success PERL 'difftool --dir-diff' '
git difftool --dir-diff --extcmd ls branch output
- stdin_contains sub output
- stdin_contains file output
+ grep sub output
+ grep file output
'
test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
git difftool --dir-diff --prompt --extcmd ls branch output
- stdin_contains sub output
- stdin_contains file output
+ grep sub output
+ grep file output
'
test_expect_success PERL 'difftool --dir-diff from subdirectory' '
(
cd sub
git difftool --dir-diff --extcmd ls branch output
- stdin_contains sub output
- stdin_contains file output
+ grep sub output
+ grep file output
)
'
--
1.8.2.411.g65a544e
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] t7800: fix tests when difftool uses --no-symlinks
When 'git difftool --dir-diff' is using --no-symlinks (either explicitly or implicitly because it's running on Windows), any working tree files that have been copied to the temporary directory are copied back after the difftool completes. Because an earlier test uses git add ., the output file used by tests is tracked by Git and the following sequence occurs during some tests: 1) the shell opens output to redirect the difftool output 2) difftool copies the empty output to the temporary directory 3) difftool runs ls which writes to output 4) difftool copies the empty output file back over the output of the command 5) the output file doesn't contain the expected output, causing the test to fail Instead of adding all changes, explicitly add only the files that the test is using, allowing later tests to write their result files into the working tree. Signed-off-by: John Keeping j...@keeping.me.uk --- Changes since v2: - Fix grammar issues in commit message - Remove the final paragraph of the commit message since this is now addressed by patch 2/5 in this series t/t7800-difftool.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 9fd09db..df443a9 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -314,7 +314,7 @@ test_expect_success PERL 'setup change in subdirectory' ' git commit -m added sub/sub echo test file echo test sub/sub - git add . + git add file sub/sub git commit -m modified both ' -- 1.8.2.411.g65a544e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] t7800: run --dir-diff tests with and without symlinks
Currently the difftool --dir-diff tests may or may not use symlinks
depending on the operating system on which they are run. In one case
this has caused a test failure to be noticed only on Windows when the
test also fails on Linux when difftool is invoked with --no-symlinks.
Rewrite these tests so that they do not depend on the environment but
run explicitly with both --symlinks and --no-symlinks, protecting the
--symlinks version with a SYMLINKS prerequisite.
Signed-off-by: John Keeping j...@keeping.me.uk
---
t/t7800-difftool.sh | 27 +++
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index df443a9..a6bd99e 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -318,28 +318,39 @@ test_expect_success PERL 'setup change in subdirectory' '
git commit -m modified both
'
-test_expect_success PERL 'difftool -d' '
- git difftool -d --extcmd ls branch output
+run_dir_diff_test () {
+ test_expect_success PERL $1 --no-symlinks
+ symlinks=--no-symlinks
+ $2
+
+ test_expect_success PERL,SYMLINKS $1 --symlinks
+ symlinks=--symlinks
+ $2
+
+}
+
+run_dir_diff_test 'difftool -d' '
+ git difftool -d $symlinks --extcmd ls branch output
grep sub output
grep file output
'
-test_expect_success PERL 'difftool --dir-diff' '
- git difftool --dir-diff --extcmd ls branch output
+run_dir_diff_test 'difftool --dir-diff' '
+ git difftool --dir-diff $symlinks --extcmd ls branch output
grep sub output
grep file output
'
-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
- git difftool --dir-diff --prompt --extcmd ls branch output
+run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
+ git difftool --dir-diff $symlinks --prompt --extcmd ls branch output
grep sub output
grep file output
'
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+run_dir_diff_test 'difftool --dir-diff from subdirectory' '
(
cd sub
- git difftool --dir-diff --extcmd ls branch output
+ git difftool --dir-diff $symlinks --extcmd ls branch output
grep sub output
grep file output
)
--
1.8.2.411.g65a544e
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
On 29.03.13 11:03, Duy Nguyen wrote: On Fri, Mar 29, 2013 at 3:45 PM, Duy Nguyen pclo...@gmail.com wrote: On Fri, Mar 29, 2013 at 4:48 AM, Jeff King p...@peff.net wrote: - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0; + return fnmatch_icase_mem(pattern, patternlen, +name, namelen, +FNM_PATHNAME) == 0; } I think you (or Junio) should rebase this on maint. Since c41244e (included in maint), this call is turned to wildmatch(WM_PATHNAME) and WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like foo/**/bar. A diff between next and pu shows me that WM_PATHNAME is incorrectly converted to FNM_PATHNAME. I hope that is the cause of all breakages Junio found out on pu. Just tested. t0003 and t3001 on 'pu' work for me because I have USE_WILDMATCH on (which turns FNM_PATHNAME to WM_PATHNAME). Both break without USE_WILDMATCH. Hm, tested what? diff --git a/dir.c b/dir.c index 73a08af..0b63167 100644 --- a/dir.c +++ b/dir.c @@ -564,7 +564,7 @@ int match_pathname(const char *pathname, int pathlen, return fnmatch_icase_mem(pattern, patternlen, name, namelen, -FNM_PATHNAME) == 0; +WM_PATHNAME) == 0; } Gives only one breakage, so we are coming closer. *** t3001-ls-files-others-exclude.sh *** [snip] not ok 17 - ls-files with ** patterns -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 6:32 PM, Torsten Bögershausen tbo...@web.de wrote: Just tested. t0003 and t3001 on 'pu' work for me because I have USE_WILDMATCH on (which turns FNM_PATHNAME to WM_PATHNAME). Both break without USE_WILDMATCH. Hm, tested what? Tested t0003 and t3001 with and without USE_WILDMATCH, which is pretty much like you patch, except that wildmatch is used instead of fnmatch. diff --git a/dir.c b/dir.c index 73a08af..0b63167 100644 --- a/dir.c +++ b/dir.c @@ -564,7 +564,7 @@ int match_pathname(const char *pathname, int pathlen, return fnmatch_icase_mem(pattern, patternlen, name, namelen, -FNM_PATHNAME) == 0; +WM_PATHNAME) == 0; } Gives only one breakage, so we are coming closer. *** t3001-ls-files-others-exclude.sh *** [snip] not ok 17 - ls-files with ** patterns -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 03:45:35PM +0700, Nguyen Thai Ngoc Duy wrote: On Fri, Mar 29, 2013 at 4:48 AM, Jeff King p...@peff.net wrote: - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0; + return fnmatch_icase_mem(pattern, patternlen, +name, namelen, +FNM_PATHNAME) == 0; } I think you (or Junio) should rebase this on maint. Since c41244e (included in maint), this call is turned to wildmatch(WM_PATHNAME) and WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like foo/**/bar. A diff between next and pu shows me that WM_PATHNAME is incorrectly converted to FNM_PATHNAME. I hope that is the cause of all breakages Junio found out on pu. I don't think we want to rebase; the regression is in the v1.8.1 series, and I suspected that Junio was planning to ship a v1.8.1.6 with the fix. The wildmatch code comes in v1.8.2. So we would want to do any adjustment to the fix when we merge up to maint. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 08:05:40AM -0400, Jeff King wrote:
On Fri, Mar 29, 2013 at 03:45:35PM +0700, Nguyen Thai Ngoc Duy wrote:
On Fri, Mar 29, 2013 at 4:48 AM, Jeff King p...@peff.net wrote:
- return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;
+ return fnmatch_icase_mem(pattern, patternlen,
+name, namelen,
+FNM_PATHNAME) == 0;
}
I think you (or Junio) should rebase this on maint. Since c41244e
(included in maint), this call is turned to wildmatch(WM_PATHNAME) and
WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like
foo/**/bar. A diff between next and pu shows me that WM_PATHNAME is
incorrectly converted to FNM_PATHNAME. I hope that is the cause of all
breakages Junio found out on pu.
I don't think we want to rebase; the regression is in the v1.8.1 series,
and I suspected that Junio was planning to ship a v1.8.1.6 with the fix.
The wildmatch code comes in v1.8.2.
So we would want to do any adjustment to the fix when we merge up to
maint.
OK. Then Junio, you may need to resolve the conflict with something
like this. Originally match_basename uses fnmatch, not wildmatch. But
using wildmatch there too should be fine, now that both
match_{base,path}name share fnmatch_icase_mem().
-- 8 --
diff --git a/dir.c b/dir.c
index 73a08af..84744df 100644
--- a/dir.c
+++ b/dir.c
@@ -81,7 +81,9 @@ static int fnmatch_icase_mem(const char *pattern, int
patternlen,
use_str = str_buf.buf;
}
- match_status = fnmatch_icase(use_pat, use_str, flags);
+ if (ignore_case)
+ flags |= WM_CASEFOLD;
+ match_status = wildmatch(use_pat, use_str, flags, NULL);
strbuf_release(pat_buf);
strbuf_release(str_buf);
@@ -564,7 +566,7 @@ int match_pathname(const char *pathname, int pathlen,
return fnmatch_icase_mem(pattern, patternlen,
name, namelen,
-FNM_PATHNAME) == 0;
+WM_PATHNAME) == 0;
}
/*
-- 8 --
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] rev-list: preallocate object hash table in --all --objects
Every time the object hash table grows, all entries must be
rearranged. The few last times could be really expensive when the
table contains a lot of entries.
When we do --all --objects we know in advance we may need to hash
all objects. Just prepare the hash table big enough, so there won't be
any resizing later on. The hash table is resized a couple times before
prehash_objects() is called. But that's ok because there aren't many
objects by that time (unless you have tons of refs, likely tags..)
On linux-2.6.git:
before after
real0m34.402s0m33.288s
user0m34.111s0m32.863s
sys 0m0.205s 0m0.352s
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
object.c | 21 +++--
object.h | 2 ++
revision.c | 59 +++
3 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/object.c b/object.c
index 20703f5..bcfd2c6 100644
--- a/object.c
+++ b/object.c
@@ -88,10 +88,10 @@ struct object *lookup_object(const unsigned char *sha1)
return obj;
}
-static void grow_object_hash(void)
+void grow_object_hash_to(unsigned long nr)
{
int i;
- int new_hash_size = obj_hash_size 32 ? 32 : 2 * obj_hash_size;
+ int new_hash_size = nr 32 ? 32 : nr * 2;
struct object **new_hash;
new_hash = xcalloc(new_hash_size, sizeof(struct object *));
@@ -106,6 +106,11 @@ static void grow_object_hash(void)
obj_hash_size = new_hash_size;
}
+static void grow_object_hash(void)
+{
+ grow_object_hash_to(obj_hash_size);
+}
+
void *create_object(const unsigned char *sha1, int type, void *o)
{
struct object *obj = o;
@@ -307,3 +312,15 @@ void clear_object_flags(unsigned flags)
obj-flags = ~flags;
}
}
+
+int has_object_flags(unsigned flags)
+{
+ int i;
+
+ for (i = 0; i obj_hash_size; i++) {
+ struct object *obj = obj_hash[i];
+ if (obj (obj-flags flags))
+ return 1;
+ }
+ return 0;
+}
diff --git a/object.h b/object.h
index 97d384b..1e8fee8 100644
--- a/object.h
+++ b/object.h
@@ -52,6 +52,7 @@ extern struct object *get_indexed_object(unsigned int);
*/
struct object *lookup_object(const unsigned char *sha1);
+extern void grow_object_hash_to(unsigned long nr);
extern void *create_object(const unsigned char *sha1, int type, void *obj);
/*
@@ -87,6 +88,7 @@ void add_object_array(struct object *obj, const char *name,
struct object_array
void add_object_array_with_mode(struct object *obj, const char *name, struct
object_array *array, unsigned mode);
void object_array_remove_duplicates(struct object_array *);
+int has_object_flags(unsigned flags);
void clear_object_flags(unsigned flags);
#endif /* OBJECT_H */
diff --git a/revision.c b/revision.c
index 71e62d8..f9ea2d1 100644
--- a/revision.c
+++ b/revision.c
@@ -1665,8 +1665,9 @@ static int for_each_good_bisect_ref(const char
*submodule, each_ref_fn fn, void
}
static int handle_revision_pseudo_opt(const char *submodule,
- struct rev_info *revs,
- int argc, const char **argv, int *flags)
+ struct rev_info *revs,
+ int argc, const char **argv,
+ int *flags, int *all)
{
const char *arg = argv[0];
const char *optarg;
@@ -1685,6 +1686,7 @@ static int handle_revision_pseudo_opt(const char
*submodule,
if (!strcmp(arg, --all)) {
handle_refs(submodule, revs, *flags, for_each_ref_submodule);
handle_refs(submodule, revs, *flags, head_ref_submodule);
+ *all = 1;
} else if (!strcmp(arg, --branches)) {
handle_refs(submodule, revs, *flags,
for_each_branch_ref_submodule);
} else if (!strcmp(arg, --bisect)) {
@@ -1738,6 +1740,49 @@ static int handle_revision_pseudo_opt(const char
*submodule,
return 1;
}
+static void preallocate_hash_table(void)
+{
+ unsigned long cnt = 0;
+ struct packed_git *p;
+ int i;
+
+ if (has_object_flags(UNINTERESTING))
+ /*
+* nope this is not simply --all --objects
+* not worth preallocation.
+*/
+ return;
+
+ for (i = 0; i 256; i++) {
+ struct dirent *ent;
+ DIR *d = opendir(git_path(objects/%02x, i));
+ if (!d)
+ continue;
+ while ((ent = readdir(d)) != NULL)
+ /*
+* We only worry about insufficient size which
+* leads to expensive growths later on. A few
+* extra slots in the hash table would not hurt.
+*/
+ cnt++;
+ closedir(d);
+ }
+
+ if
[Toy PATCH] Avoid spilling collided entries in object hash table to the next slots
If a position in object hash table is taken, we currently check out
the next one. This could potentially create long object chains. We
could create linked lists instead and leave the next slot alone.
This patch relies on the fact that pointers are usually aligned more
than one byte and it uses the first bit as object vs linked list
indicator. Architectures that don't support this can fall back to
original implementation.
The saving is real, although not ground breaking. I'm just not sure it
is worth the ugliness that comes with this patch. Although we could
avoid the alignment problem by saving that bit in a separate bitmap.
git rev-list --objects --all on linux-2.6.git:
before after
real0m33.407s0m31.732s
user0m32.926s0m31.460s
sys 0m0.407s 0m0.202s
lookup_object() goes down from 30% to 27% in perf reports.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
object.c | 42 ++
1 file changed, 42 insertions(+)
diff --git a/object.c b/object.c
index bcfd2c6..7b013a0 100644
--- a/object.c
+++ b/object.c
@@ -8,6 +8,17 @@
static struct object **obj_hash;
static int nr_objs, obj_hash_size;
+#ifdef USE_LINKED_LIST
+struct obj_list {
+ struct object *obj;
+ struct obj_list *next;
+};
+
+#define IS_LST(x) ((intptr_t)(x) 1)
+#define MAKE_LST(x) (struct object *)((intptr_t)(x) | 1)
+#define GET_LST(x) (struct obj_list *)((intptr_t)(x) ~1)
+#endif
+
unsigned int get_max_object_index(void)
{
return obj_hash_size;
@@ -53,13 +64,30 @@ static unsigned int hash_obj(struct object *obj, unsigned
int n)
static void insert_obj_hash(struct object *obj, struct object **hash, unsigned
int size)
{
unsigned int j = hash_obj(obj, size);
+#ifdef USE_LINKED_LIST
+ struct obj_list *lst, *o;
+
+ if (!hash[j]) {
+ hash[j] = obj;
+ return;
+ }
+ o = xmalloc(sizeof(*o));
+ o-obj = obj;
+
+ if (IS_LST(hash[j]))
+ o-next = GET_LST(hash[j]);
+ else
+ o-next = NULL;
+ hash[j] = MAKE_LST(o);
+#else
while (hash[j]) {
j++;
if (j = size)
j = 0;
}
hash[j] = obj;
+#endif
}
static unsigned int hashtable_index(const unsigned char *sha1)
@@ -78,6 +106,19 @@ struct object *lookup_object(const unsigned char *sha1)
return NULL;
i = hashtable_index(sha1);
+#ifdef USE_LINKED_LIST
+ if (IS_LST(obj_hash[i])) {
+ struct obj_list *lst;
+ for (lst = GET_LST(obj_hash[i]); lst; lst = lst-next) {
+ if (!hashcmp(lst-obj-sha1, sha1))
+ return lst-obj;
+ }
+ return NULL;
+ } else {
+ struct object *obj = obj_hash[i];
+ return obj !hashcmp(sha1, obj-sha1) ? obj : NULL;
+ }
+#else
while ((obj = obj_hash[i]) != NULL) {
if (!hashcmp(sha1, obj-sha1))
break;
@@ -86,6 +127,7 @@ struct object *lookup_object(const unsigned char *sha1)
i = 0;
}
return obj;
+#endif
}
void grow_object_hash_to(unsigned long nr)
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-list: preallocate object hash table in --all --objects
On Fri, Mar 29, 2013 at 08:20:10PM +0700, Nguyen Thai Ngoc Duy wrote:
Every time the object hash table grows, all entries must be
rearranged. The few last times could be really expensive when the
table contains a lot of entries.
When we do --all --objects we know in advance we may need to hash
all objects. Just prepare the hash table big enough, so there won't be
any resizing later on. The hash table is resized a couple times before
prehash_objects() is called. But that's ok because there aren't many
objects by that time (unless you have tons of refs, likely tags..)
On linux-2.6.git:
before after
real0m34.402s0m33.288s
user0m34.111s0m32.863s
sys 0m0.205s 0m0.352s
This feels weirdly specific, and like we should just be tuning our hash
table growth better. You show a 3.2% speedup here. I was able to get a
2.8% speedup just by doing this:
diff --git a/object.c b/object.c
index 20703f5..8e5e12c 100644
--- a/object.c
+++ b/object.c
@@ -91,7 +91,7 @@ static void grow_object_hash(void)
static void grow_object_hash(void)
{
int i;
- int new_hash_size = obj_hash_size 32 ? 32 : 2 * obj_hash_size;
+ int new_hash_size = obj_hash_size 32 ? 32 : 3 * obj_hash_size;
struct object **new_hash;
new_hash = xcalloc(new_hash_size, sizeof(struct object *));
It might be worth trying to figure out what the optimium growth rate is
first, which would help this use case and others. With less fragile
code.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-list: preallocate object hash table in --all --objects
On Fri, Mar 29, 2013 at 10:12 PM, Jeff King p...@peff.net wrote: This feels weirdly specific, and like we should just be tuning our hash table growth better. You show a 3.2% speedup here. I was able to get a 2.8% speedup just by doing this: It also uses a lot more memory. 5.8m entries for .. * 2 and 8.8m for ... * 3. Probably no big deal for modern machines.. It might be worth trying to figure out what the optimium growth rate is first, which would help this use case and others. With less fragile code. Agreed. Although I think it's getting out of my domain. I'm not even sure how many factors are involved. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cherry-pick -x: improve handling of one-liner commit messages
git cherry-pick -x normally just appends the cherry picked from commit
line at the end of the message, which is fine. However, in case the
original commit message had only one line, first append a newline,
otherwise the second line won't be empty, which is against
recommendations.
---
sequencer.c | 10 ++
t/t3501-revert-cherry-pick.sh | 8
2 files changed, 18 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index aef5e8a..1ae0e43 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -496,6 +496,16 @@ static int do_pick_commit(struct commit *commit, struct
replay_opts *opts)
}
if (opts-record_origin) {
+
+ /*
+* If this the message is a one-liner, append a
+* newline, so the second line will be empty, as
+* recommended.
+*/
+ p = strstr(msgbuf.buf, \n\n);
+ if (!p)
+ strbuf_addch(msgbuf, '\n');
+
strbuf_addstr(msgbuf, (cherry picked from commit );
strbuf_addstr(msgbuf,
sha1_to_hex(commit-object.sha1));
strbuf_addstr(msgbuf, )\n);
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 6f489e2..858c744 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -70,6 +70,14 @@ test_expect_success 'cherry-pick after renaming branch' '
'
+test_expect_success 'cherry-pick -x of one-liner commit message' '
+
+ git checkout rename2
+ git cherry-pick -x added
+ git show -s --pretty=format:%s | test_must_fail grep cherry picked
+
+'
+
test_expect_success 'revert after renaming branch' '
git checkout rename1
--
1.8.1.4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-list: preallocate object hash table in --all --objects
Jeff King p...@peff.net writes:
This feels weirdly specific, and like we should just be tuning our hash
table growth better. You show a 3.2% speedup here. I was able to get a
2.8% speedup just by doing this:
diff --git a/object.c b/object.c
index 20703f5..8e5e12c 100644
--- a/object.c
+++ b/object.c
@@ -91,7 +91,7 @@ static void grow_object_hash(void)
static void grow_object_hash(void)
{
int i;
- int new_hash_size = obj_hash_size 32 ? 32 : 2 * obj_hash_size;
+ int new_hash_size = obj_hash_size 32 ? 32 : 3 * obj_hash_size;
struct object **new_hash;
new_hash = xcalloc(new_hash_size, sizeof(struct object *));
It might be worth trying to figure out what the optimium growth rate is
first, which would help this use case and others. With less fragile
code.
I agree with the general principle to avoid heuristics that is too
specific to the use case. Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
Duy Nguyen pclo...@gmail.com writes:
So we would want to do any adjustment to the fix when we merge up to
maint.
OK. Then Junio, you may need to resolve the conflict with something
like this. Originally match_basename uses fnmatch, not wildmatch. But
using wildmatch there too should be fine, now that both
match_{base,path}name share fnmatch_icase_mem().
Thanks.
The result still smells somewhat funny, though.
fnmatch_icase_mem() is meant to be a wrapper of fnmatch_icase() for
counted strings and its matching semantics should be the same as
fnmatch_icase().
With the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but
fnmatch_icase() still calls into fnmatch().
The latter's flags are meant to be taken from FNM_* family, but the
former takes flags from WM_* family of bits, no?
I think you are running with USE_WILDMATCH which may make the
differences harder to notice, but the name fnmatch_icase_mem() that
is not in the same family as fnmatch but is from the wildmatch()
family smells like an accident waiting to happen.
I tend to think in the longer term it may be a good idea to build
with USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so
in the end this may not matter that much, but before that happens,
soon after we merge the regression fix with this merge-fix, we may
want to update the codebase as if we applied a series that were
based on 'maint' as you suggested, i.e. using raw wildmatch()
consistently in the match_{base,path}name() codepath.
Opinions?
-- 8 --
diff --git a/dir.c b/dir.c
index 73a08af..84744df 100644
--- a/dir.c
+++ b/dir.c
@@ -81,7 +81,9 @@ static int fnmatch_icase_mem(const char *pattern, int
patternlen,
use_str = str_buf.buf;
}
- match_status = fnmatch_icase(use_pat, use_str, flags);
+ if (ignore_case)
+ flags |= WM_CASEFOLD;
+ match_status = wildmatch(use_pat, use_str, flags, NULL);
strbuf_release(pat_buf);
strbuf_release(str_buf);
@@ -564,7 +566,7 @@ int match_pathname(const char *pathname, int pathlen,
return fnmatch_icase_mem(pattern, patternlen,
name, namelen,
- FNM_PATHNAME) == 0;
+ WM_PATHNAME) == 0;
}
/*
-- 8 --
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config
On Fri, Mar 29, 2013 at 09:29:14AM -0700, Phil Haack wrote: If you run the following set of commands: git checkout -b some-branch git push origin some-branch git branch --set-upstream-to origin/some-branch git branch --unset-upstream some-branch git branch --set-upstream-to origin/some-branch You get the following result in the .git\config file [branch some-branch] [branch some-branch] remote = origin merge = refs/heads/some-branch My expectation is that the very last call to: git branch --set-upstream-to would not add a new config section, but would instead insert this information into the existing [branch some-branch]. Yes, this is a known issue[1] in the config-editing code. There are actually two separate problems. Try this: $ git config -f foo section.one 1 $ cat foo [section] one = 1 Looking good so far... $ git config -f foo --unset section.one $ cat foo [section] Oops, it would have been nice to clean up that now-empty section. Now let's re-add... $ git config -f foo section.two 2 $ cat foo [section] [section] two = 2 Oops, it would have been nice to use the existing section. What happens if we add again... $ git config -f foo section.three 3 $ cat foo [section] [section] two = 2 three = 3 Now that one worked. I think what happens is that the config editor runs through the files linearly, munging whatever lines necessary for the requested operation, and leaving everything else untouched (as it must, to leave comments and whitespace intact). But it does not keep a look-behind buffer to realize that a section name is now obsolete (which we don't know until we get to the next section, or to EOF). In the worst case, this buffer can grow arbitrarily large, like: [foo] # the above section is now empty # but we have to read through all of # these comments to actually # realize it [bar] In practice it should not be a big deal (and I do not think it is an interesting attack vector for somebody trying to run you out of memory). That brings up another point, which is that comments may get misplaced by deletion. But that's the case for any modification we do to the file, since we cannot understand the semantics of comments that say things like the line below does X. So that's the first bug. The second one is, I suspect, just laziness. We notice that section.two is set, and put section.three after it. But we do not make the same trigger for just section. That's probably much easier to fix. In any case, from what I understand the current behavior is technically valid and I haven't encountered any adverse effects. It was just something I noticed while testing. Yes, the config files generated by these operations parse as you would expect them to. I think it's purely an aesthetic concern. -Peff [1] http://thread.gmane.org/gmane.comp.version-control.git/208113 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 09:44:32AM -0700, Junio C Hamano wrote:
Duy Nguyen pclo...@gmail.com writes:
So we would want to do any adjustment to the fix when we merge up to
maint.
OK. Then Junio, you may need to resolve the conflict with something
like this. Originally match_basename uses fnmatch, not wildmatch. But
using wildmatch there too should be fine, now that both
match_{base,path}name share fnmatch_icase_mem().
Thanks.
The result still smells somewhat funny, though.
fnmatch_icase_mem() is meant to be a wrapper of fnmatch_icase() for
counted strings and its matching semantics should be the same as
fnmatch_icase().
With the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but
fnmatch_icase() still calls into fnmatch().
The latter's flags are meant to be taken from FNM_* family, but the
former takes flags from WM_* family of bits, no?
Yeah, that does not seem right. If match_pathname has learned to call
into wildmatch instead of fnmatch_icase in the interim, then the right
resolution is to convert its call to fnmatch_icase_mem to a new
wildmatch_mem. Presumably that can be done by either tweaking
fnmatch_icase_mem, or, if wildmatch is ready to take counted strings,
calling into it with the right options.
I think you are running with USE_WILDMATCH which may make the
differences harder to notice, but the name fnmatch_icase_mem() that is
not in the same family as fnmatch but is from the wildmatch() family
smells like an accident waiting to happen.
Agreed.
I tend to think in the longer term it may be a good idea to build with
USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so in the
end this may not matter that much
Yeah, I think that is a sane long-term goal.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Toy PATCH] Avoid spilling collided entries in object hash table to the next slots
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: If a position in object hash table is taken, we currently check out the next one. This could potentially create long object chains. We could create linked lists instead and leave the next slot alone. In the current code, not just the logic in lookup_object(), but the logic to enforce load factor when create_object() decides to call grow_object_hash() and object enumeration implemented by get_max_object_index() and get_indexed_object() are closely tied to the open addressing scheme. If you want to switch to any other method (e.g. separate chaining) these need to be updated quite a bit. I do not see get_max_object_index() and get_indexed_object() updated at all. Do fsck, index-pack, name-rev and upload-pack still work? This particular implementation that uses a fake union is a bit ugly, but in general, it may be worth rethinking the object hash implementation. I vaguely recall trying cuckoo in the vicinity of this code. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config
Jeff King p...@peff.net writes:
I think what happens is that the config editor runs
through the files linearly, munging whatever lines necessary for the
requested operation, and leaving everything else untouched (as it must,
to leave comments and whitespace intact). But it does not keep a
look-behind buffer to realize that a section name is now obsolete (which
we don't know until we get to the next section, or to EOF). In the worst
case, this buffer can grow arbitrarily large, like:
[foo]
# the above section is now empty
# but we have to read through all of
# these comments to actually
# realize it
[bar]
If we treat this case as having a bunch of comments that make the
section non-empty, then we both avoid needing an arbitrarily large
lookbehind and deleting the user's precious comments...
I.e. the rule would be that we only delete the section if there is
nothing but whitespace until the next section header.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config
On Fri, Mar 29, 2013 at 06:20:28PM +0100, Thomas Rast wrote: Jeff King p...@peff.net writes: I think what happens is that the config editor runs through the files linearly, munging whatever lines necessary for the requested operation, and leaving everything else untouched (as it must, to leave comments and whitespace intact). But it does not keep a look-behind buffer to realize that a section name is now obsolete (which we don't know until we get to the next section, or to EOF). In the worst case, this buffer can grow arbitrarily large, like: [foo] # the above section is now empty # but we have to read through all of # these comments to actually # realize it [bar] If we treat this case as having a bunch of comments that make the section non-empty, then we both avoid needing an arbitrarily large lookbehind and deleting the user's precious comments... I.e. the rule would be that we only delete the section if there is nothing but whitespace until the next section header. I think that is sane. Technically it does not remove the need for the buffer, unless we are also collapsing whitespace (which I think is probably a sane thing to do, too). I'm looking at a patch now... -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cherry-pick -x: improve handling of one-liner commit messages
Miklos Vajna vmik...@suse.cz writes:
git cherry-pick -x normally just appends the cherry picked from commit
line at the end of the message, which is fine. However, in case the
original commit message had only one line, first append a newline,
otherwise the second line won't be empty, which is against
recommendations.
---
Sign-off?
I think this is part of the bc/append-signed-off-by topic that is
about to graduate to 'master'; more specifically, b971e04f54e7
(sequencer.c: always separate (cherry picked from from commit
body, 2013-02-12) does the equivalent, no?
sequencer.c | 10 ++
t/t3501-revert-cherry-pick.sh | 8
2 files changed, 18 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index aef5e8a..1ae0e43 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -496,6 +496,16 @@ static int do_pick_commit(struct commit *commit, struct
replay_opts *opts)
}
if (opts-record_origin) {
+
+ /*
+ * If this the message is a one-liner, append a
+ * newline, so the second line will be empty, as
+ * recommended.
+ */
+ p = strstr(msgbuf.buf, \n\n);
+ if (!p)
+ strbuf_addch(msgbuf, '\n');
+
strbuf_addstr(msgbuf, (cherry picked from commit );
strbuf_addstr(msgbuf,
sha1_to_hex(commit-object.sha1));
strbuf_addstr(msgbuf, )\n);
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 6f489e2..858c744 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -70,6 +70,14 @@ test_expect_success 'cherry-pick after renaming branch' '
'
+test_expect_success 'cherry-pick -x of one-liner commit message' '
+
+ git checkout rename2
+ git cherry-pick -x added
+ git show -s --pretty=format:%s | test_must_fail grep cherry picked
+
+'
+
test_expect_success 'revert after renaming branch' '
git checkout rename1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config
Phil Haack haac...@gmail.com writes: Hello there! Please /cc me with responses as I'm not on the mailing list. I'm using git version 1.8.1.msysgit.1 and I ran into a very minor issue. It doesn't actually seem to affect operations, but I thought I'd report it in case someone felt it was worth cleaning up. I do not think this is limited to branch --set-whatever. If you run the following set of commands: git checkout -b some-branch git push origin some-branch git branch --set-upstream-to origin/some-branch git branch --unset-upstream some-branch This step causes the removal of the last variable in a configuration section, which leaves an empty section. As the code to add a new variable to the configuration, trying to reuse an existing section header, does not pay attention to an empty section, you end up with an empty section, followed by a new one. Either removal of configuration variable should be taught to remove the section header when a section becomes empty, or addition of configuration variable should be taught to notice an empty section header. git branch --set-upstream-to origin/some-branch You get the following result in the .git\config file [branch some-branch] [branch some-branch] remote = origin merge = refs/heads/some-branch -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
Jeff King p...@peff.net writes: On Fri, Mar 29, 2013 at 09:44:32AM -0700, Junio C Hamano wrote: ... With the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but fnmatch_icase() still calls into fnmatch(). The latter's flags are meant to be taken from FNM_* family, but the former takes flags from WM_* family of bits, no? Yeah, that does not seem right. If match_pathname has learned to call into wildmatch instead of fnmatch_icase in the interim, then the right resolution is to convert its call to fnmatch_icase_mem to a new wildmatch_mem. Presumably that can be done by either tweaking fnmatch_icase_mem, or, if wildmatch is ready to take counted strings, calling into it with the right options. I think you are running with USE_WILDMATCH which may make the differences harder to notice, but the name fnmatch_icase_mem() that is not in the same family as fnmatch but is from the wildmatch() family smells like an accident waiting to happen. Agreed. This may be just the matter of naming. It smelled wrong to me only because the fnmatch in the helper fnmatch_icase_mem() told me that it should forever use fnmatch semantics. After reading its (only) two callsites, they are both the caller has transformed the inputs to this lowest level pathname vs pattern matching function in order to reduce the cost of matching, and now it is time to exercise the matcher. The only thing they care about is that they are calling the lowest level pathname vs pattern matching function. If we pronounce fnmatch_icase_mem() as match_path_with_pattern() or something in the original series, the problem may go away ;-) Does any caller pass FNM_* bits to a callchain that reach the new *_mem() function? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 10:35:17AM -0700, Junio C Hamano wrote: This may be just the matter of naming. It smelled wrong to me only because the fnmatch in the helper fnmatch_icase_mem() told me that it should forever use fnmatch semantics. After reading its (only) two callsites, they are both the caller has transformed the inputs to this lowest level pathname vs pattern matching function in order to reduce the cost of matching, and now it is time to exercise the matcher. The only thing they care about is that they are calling the lowest level pathname vs pattern matching function. If we pronounce fnmatch_icase_mem() as match_path_with_pattern() or something in the original series, the problem may go away ;-) Yes, since there are only the two new added callers, if they both want to switch to wildmatch, then it is fine for the helper function to switch. The danger is if some other caller wants to start using it; I added it with the name I did figuring that other spots might want to use it eventually. But if all of fnmatch is going to go away in favor of wildmatch eventually, then that helper is not all that useful (or it would be even more useful as wildmatch_mem or similar). Does any caller pass FNM_* bits to a callchain that reach the new *_mem() function? The series only adds two callers, and they provide constant flags; match_basename passes no flags, and should be OK. match_pathname uses FNM_PATHNAME, and would need to be converted to use WM_PATHNAME as part of the conflict resolution. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
gitdiffbinstat - git diff --shortstat -like output for changes in binary files
I use git mostly for game-development which means I have to deal with a lot of binary files (images, sound files etc). When I came to a point where I had run image optimization on a branch, I wanted to know of course how much smaller the new branch was in comparison to master. Problem was that 'git diff --stat' would only summerize per-binary-file size changes and 'git diff --shortstat' did skip the binary files entirely. To solve this problem, I wrote a script (gitdiffbinstat) which basically runs 'git diff --stat' and summerizes the output. The script can be found here: https://github.com/matthiaskrgr/gitdiffbinstat/blob/master/gitdiffbinstat.sh Screenshot of example output is attached. I wondered what you guys thought about the script, is there a chance to perhaps get it included as some kind of helper script into the official git repo? Regards, Matthias attachment: screenshot.png
Re: [Toy PATCH] Avoid spilling collided entries in object hash table to the next slots
Junio C Hamano gits...@pobox.com writes:
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
If a position in object hash table is taken, we currently check out
the next one. This could potentially create long object chains. We
could create linked lists instead and leave the next slot alone.
In the current code, not just the logic in lookup_object(), but the
logic to enforce load factor when create_object() decides to call
grow_object_hash() and object enumeration implemented by
get_max_object_index() and get_indexed_object() are closely tied to
the open addressing scheme. If you want to switch to any other
method (e.g. separate chaining) these need to be updated quite a
bit.
I do not see get_max_object_index() and get_indexed_object() updated
at all. Do fsck, index-pack, name-rev and upload-pack still work?
You may want to start with a bit more abstraction around the
hashtable API. Perhaps like this?
The idea is to let your object enumerator to be not just a simple
unsigned int into the flat hashtable, but be something like
typedef struct {
unsigned int slot;
struct obj_list *list;
} object_enumerator;
You store the current index in obj_hash[] to enu.slot, and if that
is IS_LST(), the linked-list element you are looking at in enu.list.
When you increment the iterator in object_enumerator_next(), you
increment enu.slot only after you reach the tail of the enu.list.
builtin/fsck.c | 17 ++---
builtin/index-pack.c | 11 +++
builtin/name-rev.c | 20 +++-
object.c | 22 +++---
object.h | 8 ++--
upload-pack.c| 23 ++-
6 files changed, 67 insertions(+), 34 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index bb9a2cd..5688cad 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -270,22 +270,25 @@ static void check_object(struct object *obj)
static void check_connectivity(void)
{
- int i, max;
+ int max;
+ object_enumerator enu;
/* Traverse the pending reachable objects */
traverse_reachable();
/* Look up all the requirements, warn about missing objects.. */
- max = get_max_object_index();
+ max = begin_object_enumeration(enu);
if (verbose)
fprintf(stderr, Checking connectivity (%d objects)\n, max);
- for (i = 0; i max; i++) {
- struct object *obj = get_indexed_object(i);
-
- if (obj)
- check_object(obj);
+ if (max) {
+ do {
+ struct object *obj = get_enumerated_object(enu);
+ if (obj)
+ check_object(obj);
+ } while (object_enumeration_next(enu));
}
+ end_object_enumeration(enu);
}
static int fsck_obj(struct object *obj)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index ef62124..1d5b65c 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -195,11 +195,14 @@ static void check_object(struct object *obj)
static void check_objects(void)
{
- unsigned i, max;
+ object_enumerator enu;
- max = get_max_object_index();
- for (i = 0; i max; i++)
- check_object(get_indexed_object(i));
+ if (begin_object_enumeration(enu)) {
+ do {
+ check_object(get_enumerated_object(enu));
+ } while (object_enumeration_next(enu));
+ }
+ end_object_enumeration(enu);
}
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 6238247..239c3ef 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -286,16 +286,18 @@ int cmd_name_rev(int argc, const char **argv, const char
*prefix)
name_rev_line(p, data);
}
} else if (all) {
- int i, max;
-
- max = get_max_object_index();
- for (i = 0; i max; i++) {
- struct object *obj = get_indexed_object(i);
- if (!obj || obj-type != OBJ_COMMIT)
- continue;
- show_name(obj, NULL,
- always, allow_undefined, data.name_only);
+ object_enumerator enu;
+
+ if (begin_object_enumeration(enu)) {
+ do {
+ struct object *obj =
get_enumerated_object(enu);
+ if (!obj || obj-type != OBJ_COMMIT)
+ continue;
+ show_name(obj, NULL,
+ always, allow_undefined,
data.name_only);
+ } while (object_enumeration_next(enu));
}
+ end_object_enumeration(enu);
} else {
int i;
for (i = 0; i revs.nr; i++)
Re: gitdiffbinstat - git diff --shortstat -like output for changes in binary files
On Fri, Mar 29, 2013 at 07:07:32PM +0100, Matthias Krüger wrote: I use git mostly for game-development which means I have to deal with a lot of binary files (images, sound files etc). When I came to a point where I had run image optimization on a branch, I wanted to know of course how much smaller the new branch was in comparison to master. Problem was that 'git diff --stat' would only summerize per-binary-file size changes and 'git diff --shortstat' did skip the binary files entirely. Have you tried --summary? Combined with --stat (or --shortstat) I wonder if it would get you closer to what you want. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t1300: document some aesthetic failures of the config editor
Jeff King p...@peff.net writes: diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 3c96fda..d62facb 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1087,4 +1087,36 @@ test_expect_success 'barf on incomplete string' ' grep line 3 error ' +# good section hygiene +test_expect_failure 'unsetting the last key in a section removes header' ' + cat .git/config -\EOF + [section] + # some intervening lines + # that should be saved + key = value + EOF + + cat expect -\EOF + # some intervening lines + # that should be saved + EOF I do not know if I agree with this expectation. Most likely these comments are about the section, and possibly even are specific to section.key, not applicable to the section in general). If we _were_ to remove the section header at this point, we should be removing the comment two out of three cases (if it is about section.key, it should go when section.key goes; if it is about section, it should go when section goes; if it is a more generic comment about this configuration file, it should stay). A better approach may be to only insist on the when adding, reuse an empty section header side of the coin. Then we do not have to worry about we keep cruft that talks about some section but what the comment says is illegible now the crucial bit of information, section name the comment talks about, is gone. + + git config --unset section.key + test_cmp expect .git/config +' + +test_expect_failure 'adding a key into an empty section reuses header' ' + cat .git/config -\EOF + [section] + EOF + + q_to_tab expect -\EOF + [section] + Qkey = value + EOF + + git config section.key value + test_cmp expect .git/config +' This side I would agree it is unconditionally a good thing to do. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gitdiffbinstat - git diff --shortstat -like output for changes in binary files
Jeff King p...@peff.net writes: I use git mostly for game-development which means I have to deal with a lot of binary files (images, sound files etc). When I came to a point where I had run image optimization on a branch, I wanted to know of course how much smaller the new branch was in comparison to master. Problem was that 'git diff --stat' would only summerize per-binary-file size changes and 'git diff --shortstat' did skip the binary files entirely. Have you tried --summary? Combined with --stat (or --shortstat) I wonder if it would get you closer to what you want. diff is not about how much did my project grow or shink. If you moved one block of lines up or down in the same file, you will see double the number of lines moved in the statistics. On the other hand, the use case to measure how much it helped to run png optimizers only cares about the total size. I do not think it is a good match to present the binary stat next to the textual diff stats in the first place. Adding two numbers do not give us any meaningful number. It is an interesting use case, but it just is not a problem core Git, which is a source code management system, particularly wants to bolt on a solution for, that does not mesh well with other parts of the system. We do want to make sure that people who want to deal with binaries have access to raw statistics, so that they can build their solution on top of, though. ls-tree -r -l gives byte-size of each blob, and the attribute system can let you tell which paths are binaries. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t1300: document some aesthetic failures of the config editor
On Fri, Mar 29, 2013 at 11:51:51AM -0700, Junio C Hamano wrote: + cat expect -\EOF + # some intervening lines + # that should be saved + EOF I do not know if I agree with this expectation. Most likely these comments are about the section, and possibly even are specific to section.key, not applicable to the section in general). If we _were_ to remove the section header at this point, we should be removing the comment two out of three cases (if it is about section.key, it should go when section.key goes; if it is about section, it should go when section goes; if it is a more generic comment about this configuration file, it should stay). I agree that probably makes more sense (I actually wrote the test before responding to Thomas, and then got bogged down in the code change and forgot to update it when I decided to give up). A better approach may be to only insist on the when adding, reuse an empty section header side of the coin. Then we do not have to worry about we keep cruft that talks about some section but what the comment says is illegible now the crucial bit of information, section name the comment talks about, is gone. I think they are two separate problems. They happen to combine to produce the behavior that Phil reported, but I would still expect --unset not to leave cruft. It makes sense to document to me to document both via tests; even if we end up tweaking the expected behavior when the fix is actually implemented, the presence of the test still serves as a reminder of the issue. Here it is with the updated expectation. I don't care _that_ much, so if you feel strongly and want to drop the first test, feel free. -- 8 -- Subject: [PATCH] t1300: document some aesthetic failures of the config editor The config-editing code used by git config var value is built around the regular config callback parser, whose only triggerable item is an actual key. As a result, it does not know anything about section headers, which can result in unnecessarily ugly output: 1. When we delete the last key in a section, we should be able to delete the section header. 2. When we add a key into a section, we should be able to reuse the same section header, even if that section did not have any keys in it already. Unfortunately, fixing these is not trivial with the current code. It would involve the config parser recording and passing back information on each item it finds, including headers, keys, and even comments (or even better, generating an actual in-memory parse-tree). Since these behaviors do not cause any functional problems (i.e., the resulting config parses as expected, it is just uglier than one would like), fixing them can wait until somebody feels like substantially refactoring the parsing code. In the meantime, let's document them as known issues with some tests. Signed-off-by: Jeff King p...@peff.net --- t/t1300-repo-config.sh | 30 ++ 1 file changed, 30 insertions(+) diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 3c96fda..213e5a8 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1087,4 +1087,34 @@ test_expect_success 'barf on incomplete string' ' grep line 3 error ' +# good section hygiene +test_expect_failure 'unsetting the last key in a section removes header' ' + cat .git/config -\EOF + [section] + # some intervening lines + # that should also be dropped + + key = value + EOF + + expect + + git config --unset section.key + test_cmp expect .git/config +' + +test_expect_failure 'adding a key into an empty section reuses header' ' + cat .git/config -\EOF + [section] + EOF + + q_to_tab expect -\EOF + [section] + Qkey = value + EOF + + git config section.key value + test_cmp expect .git/config +' + test_done -- 1.8.2.rc3.27.g5c55d5c -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t1300: document some aesthetic failures of the config editor
Junio C Hamano gits...@pobox.com writes: ... If we _were_ to remove the section header at this point, we should be removing the comment two out of three cases (if it is about section.key, it should go when section.key goes; if it is about section, it should go when section goes; if it is a more generic comment about this configuration file, it should stay). The last case should end more like this: ..., it should stay, but why are you writing a comment that is not specific to this section _inside_ the section in the first place??? Another case we have to worry about, if we were to remove an empty section header is this case: # Now, let's list all the remotes I interact with. # This is where I push all the separate topics. [remote github] # fetch like everybody else without auth url = git://github.com/user/git # push with auth pushURL = github.com:user/git # publish the state of my primary working area as-is mirror # Traditional canonical one [remote korg] url = k.org:/pub/scm/user/git If I were to retire github account, removing the section while keeping the comments would be confusing (I do not push all the separate topics to korg). Removing the section while removing the comments that pertain to the section is impossible; Now, let's list all the remotes should stay, This is where I push should go. Removing only the keys and keeping the skelton around would give us: # Now, let's list all the remotes I interact with. # This is where I push all the separate topics. [remote github] # fetch like everybody else without auth # push with auth # publish the state of my primary working area as-is # Traditional canonical one [remote korg] url = k.org:/pub/scm/user/git which is still confusing, but at least the confusion is not spread to adjacent sections. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] difftool: don't overwrite modified files
John Keeping j...@keeping.me.uk writes:
After running the user's diff tool, git-difftool will copy any files
that differ between the working tree and the temporary tree. This is
useful when the user edits the file in their diff tool but is wrong if
they edit the working tree file while examining the diff.
Thanks.
I should drop the t7800-modernize topic and queue this under a
better name. Perhaps difftool-no-overwrite-on-copyback, or
something.
Instead of copying unconditionally when the files differ, create and
index from the working tree files and only copy the temporary file back
if it was modified and the working tree file was not. If both files
have been modified, print a warning and exit with an error.
Note that we cannot use an existing index in git-difftool since those
contain the modified files that need to be checked out but here we are
looking at those files which are copied from the working tree and not
checked out. These are precisely the files which are not in the
existing indices.
Signed-off-by: John Keeping j...@keeping.me.uk
---
Changes since v2:
- Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails
git-difftool.perl | 73
+++--
t/t7800-difftool.sh | 30 ++
2 files changed, 89 insertions(+), 14 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 663640d..844f619 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,9 +13,9 @@
use 5.008;
use strict;
use warnings;
+use Error qw(:try);
use File::Basename qw(dirname);
use File::Copy;
-use File::Compare;
use File::Find;
use File::stat;
use File::Path qw(mkpath rmtree);
@@ -88,14 +88,45 @@ sub use_wt_file
my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
my $null_sha1 = '0' x 40;
- if ($sha1 eq $null_sha1) {
- return 1;
- } elsif (not $symlinks) {
+ if ($sha1 ne $null_sha1 and not $symlinks) {
return 0;
}
my $wt_sha1 = $repo-command_oneline('hash-object', $workdir/$file);
- return $sha1 eq $wt_sha1;
+ my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+ return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+ my ($repo_path, $index, $worktree) = @_;
+ $ENV{GIT_INDEX_FILE} = $index;
+ $ENV{GIT_WORK_TREE} = $worktree;
+ my $must_unset_git_dir = 0;
+ if (not defined($ENV{GIT_DIR})) {
+ $must_unset_git_dir = 1;
+ $ENV{GIT_DIR} = $repo_path;
+ }
+
+ my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+ my @gitargs = qw/diff-files --name-only -z/;
+ try {
+ Git::command_oneline(@refreshargs);
+ } catch Git::Error::Command with {};
+
+ my $line = Git::command_oneline(@gitargs);
+ my @files;
+ if (defined $line) {
+ @files = split('\0', $line);
+ } else {
+ @files = ();
+ }
+
+ delete($ENV{GIT_INDEX_FILE});
+ delete($ENV{GIT_WORK_TREE});
+ delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+ return map { $_ = 1 } @files;
}
sub setup_dir_diff
@@ -121,6 +152,7 @@ sub setup_dir_diff
my $null_sha1 = '0' x 40;
my $lindex = '';
my $rindex = '';
+ my $wtindex = '';
my %submodule;
my %symlink;
my @working_tree = ();
@@ -174,8 +206,12 @@ EOF
}
if ($rmode ne $null_mode) {
- if (use_wt_file($repo, $workdir, $dst_path, $rsha1,
$symlinks)) {
- push(@working_tree, $dst_path);
+ my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+ $dst_path, $rsha1,
+ $symlinks);
+ if ($use) {
+ push @working_tree, $dst_path;
+ $wtindex .= $rmode $wt_sha1\t$dst_path\0;
} else {
$rindex .= $rmode $rsha1\t$dst_path\0;
}
@@ -218,6 +254,12 @@ EOF
$rc = system('git', 'checkout-index', '--all', --prefix=$rdir/);
exit_cleanup($tmpdir, $rc) if $rc != 0;
+ $ENV{GIT_INDEX_FILE} = $tmpdir/wtindex;
+ ($inpipe, $ctx) =
+ $repo-command_input_pipe(qw(update-index --info-only -z
--index-info));
+ print($inpipe $wtindex);
+ $repo-command_close_pipe($inpipe, $ctx);
+
# If $GIT_DIR was explicitly set just for the update/checkout
# commands, then it should be unset before continuing.
delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
@@ -390,19 +432,22 @@ sub dir_diff
# should be copied back to the working tree.
# Do not copy back files when symlinks are used and the
# external tool did not replace the original link with a file.
+ my %wt_modified =
Re: [PATCH] t1300: document some aesthetic failures of the config editor
Jeff King p...@peff.net writes: Here it is with the updated expectation. I don't care _that_ much, so if you feel strongly and want to drop the first test, feel free. As long as we are adding expect_failure without an immediate fix, let's document the ideal, with this patch on top. t/t1300-repo-config.sh | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 213e5a8..133f26d 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1089,16 +1089,20 @@ test_expect_success 'barf on incomplete string' ' # good section hygiene test_expect_failure 'unsetting the last key in a section removes header' ' cat .git/config -\EOF + # some generic comment on the configuration file itself + # a comment specific to this section section. [section] # some intervening lines # that should also be dropped key = value EOF - expect + cat expect -\EOF + # some generic comment on the configuration file itself + EOF git config --unset section.key test_cmp expect .git/config ' -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cherry-pick -x: improve handling of one-liner commit messages
Hi, On Fri, Mar 29, 2013 at 10:41:17AM -0700, Brandon Casey draf...@gmail.com wrote: Sign-off? Indeed, I forgot about it, my bad. I think this is part of the bc/append-signed-off-by topic that is about to graduate to 'master'; more specifically, b971e04f54e7 (sequencer.c: always separate (cherry picked from from commit body, 2013-02-12) does the equivalent, no? Yeah, I think this case is already handled. Miklos, can you check out next and see if your problem case is handled? I just checked next and right, that solves the problem I was fixing. So -- sorry for the noise. :-) Miklos signature.asc Description: Digital signature
Re: [PATCH] cherry-pick -x: improve handling of one-liner commit messages
Miklos Vajna vmik...@suse.cz writes: On Fri, Mar 29, 2013 at 10:41:17AM -0700, Brandon Casey draf...@gmail.com wrote: I think this is part of the bc/append-signed-off-by topic that is about to graduate to 'master'; more specifically, b971e04f54e7 (sequencer.c: always separate (cherry picked from from commit body, 2013-02-12) does the equivalent, no? Yeah, I think this case is already handled. Miklos, can you check out next and see if your problem case is handled? I just checked next and right, that solves the problem I was fixing. So -- sorry for the noise. :-) Don't be sorry. People noticing an issue, trying 'master' and 'next' to see that it has already been resolved, is the only way for us to know we are helping people other than those who had the original itch. The ideal would be for more people to run 'next' and report both problems and successes ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] optimize set_shared_perm()
Torsten Bögershausen tbo...@web.de writes: optimize set_shared_perm() in path.c: - sometimes the chown() function is called even when not needed. (This can be provoced by running t1301, and adding some debug code) Save a chmod from 400 to 400, or from 600-600 on these files: .git/info/refs+ .git/objects/info/packs+ Save chmod on directories from 2770 to 2770: .git/refs .git/refs/heads .git/refs/tags - as all callers use mode == 0 when caling set_shared_perm(), the function can be simplified. - all callers use the macro adjust_shared_perm(path) from cache.h Convert adjust_shared_perm() from a macro into a function prototype The last two points can become a separate preparation step. The result would be easier to read. Your updated adjust_shared_perm() does not begin with: if (!shared_repository) return 0; as the original, but it always first calls to get_st_mode_bits() which makes a call to lstat(2). That smells like a huge regression for !shared_repository case, unless you have updated the existing callers of adjust_shared_perm() not to call it when !shared_repository. diff --git a/path.c b/path.c index 2fdccc2..4bc918a 100644 --- a/path.c +++ b/path.c @@ -1,14 +1,5 @@ /* - * I'm tired of doing vsnprintf() etc just to open a - * file, so here's a return static buffer with printf - * interface for paths. - * - * It's obviously not thread-safe. Sue me. But it's quite - * useful for doing things like - * - * f = open(mkpath(%s/%s.git, base, name), O_RDONLY); - * - * which is what it's designed for. + * Different utilitiy functions for path and path names */ Removing the stale I'm tired of is good; you do not have to move it anywhere. A single-liner /* Utilities for paths and pathnames */ should suffice. Will discard this step (but will keep PATCH 1/2). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] difftool: don't overwrite modified files
On Fri, Mar 29, 2013 at 01:20:50PM -0700, Junio C Hamano wrote:
John Keeping j...@keeping.me.uk writes:
After running the user's diff tool, git-difftool will copy any files
that differ between the working tree and the temporary tree. This is
useful when the user edits the file in their diff tool but is wrong if
they edit the working tree file while examining the diff.
Thanks.
I should drop the t7800-modernize topic and queue this under a
better name. Perhaps difftool-no-overwrite-on-copyback, or
something.
Instead of copying unconditionally when the files differ, create and
index from the working tree files and only copy the temporary file back
if it was modified and the working tree file was not. If both files
have been modified, print a warning and exit with an error.
Note that we cannot use an existing index in git-difftool since those
contain the modified files that need to be checked out but here we are
looking at those files which are copied from the working tree and not
checked out. These are precisely the files which are not in the
existing indices.
Signed-off-by: John Keeping j...@keeping.me.uk
---
Changes since v2:
- Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails
git-difftool.perl | 73
+++--
t/t7800-difftool.sh | 30 ++
2 files changed, 89 insertions(+), 14 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 663640d..844f619 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,9 +13,9 @@
use 5.008;
use strict;
use warnings;
+use Error qw(:try);
use File::Basename qw(dirname);
use File::Copy;
-use File::Compare;
use File::Find;
use File::stat;
use File::Path qw(mkpath rmtree);
@@ -88,14 +88,45 @@ sub use_wt_file
my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
my $null_sha1 = '0' x 40;
- if ($sha1 eq $null_sha1) {
- return 1;
- } elsif (not $symlinks) {
+ if ($sha1 ne $null_sha1 and not $symlinks) {
return 0;
}
my $wt_sha1 = $repo-command_oneline('hash-object', $workdir/$file);
- return $sha1 eq $wt_sha1;
+ my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+ return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+ my ($repo_path, $index, $worktree) = @_;
+ $ENV{GIT_INDEX_FILE} = $index;
+ $ENV{GIT_WORK_TREE} = $worktree;
+ my $must_unset_git_dir = 0;
+ if (not defined($ENV{GIT_DIR})) {
+ $must_unset_git_dir = 1;
+ $ENV{GIT_DIR} = $repo_path;
+ }
+
+ my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+ my @gitargs = qw/diff-files --name-only -z/;
+ try {
+ Git::command_oneline(@refreshargs);
+ } catch Git::Error::Command with {};
+
+ my $line = Git::command_oneline(@gitargs);
+ my @files;
+ if (defined $line) {
+ @files = split('\0', $line);
+ } else {
+ @files = ();
+ }
+
+ delete($ENV{GIT_INDEX_FILE});
+ delete($ENV{GIT_WORK_TREE});
+ delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+ return map { $_ = 1 } @files;
}
sub setup_dir_diff
@@ -121,6 +152,7 @@ sub setup_dir_diff
my $null_sha1 = '0' x 40;
my $lindex = '';
my $rindex = '';
+ my $wtindex = '';
my %submodule;
my %symlink;
my @working_tree = ();
@@ -174,8 +206,12 @@ EOF
}
if ($rmode ne $null_mode) {
- if (use_wt_file($repo, $workdir, $dst_path, $rsha1,
$symlinks)) {
- push(@working_tree, $dst_path);
+ my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+ $dst_path, $rsha1,
+ $symlinks);
+ if ($use) {
+ push @working_tree, $dst_path;
+ $wtindex .= $rmode $wt_sha1\t$dst_path\0;
} else {
$rindex .= $rmode $rsha1\t$dst_path\0;
}
@@ -218,6 +254,12 @@ EOF
$rc = system('git', 'checkout-index', '--all', --prefix=$rdir/);
exit_cleanup($tmpdir, $rc) if $rc != 0;
+ $ENV{GIT_INDEX_FILE} = $tmpdir/wtindex;
+ ($inpipe, $ctx) =
+ $repo-command_input_pipe(qw(update-index --info-only -z
--index-info));
+ print($inpipe $wtindex);
+ $repo-command_close_pipe($inpipe, $ctx);
+
# If $GIT_DIR was explicitly set just for the update/checkout
# commands, then it should be unset before continuing.
delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
@@ -390,19 +432,22 @@ sub dir_diff
# should be copied back to the working tree.
# Do not copy back files when symlinks are used and the
# external tool
[PATCH 2/5 v2] difftool: don't overwrite modified files
After running the user's diff tool, git-difftool will copy any files
that differ between the working tree and the temporary tree. This is
useful when the user edits the file in their diff tool but is wrong if
they edit the working tree file while examining the diff.
Instead of copying unconditionally when the files differ, create and
index from the working tree files and only copy the temporary file back
if it was modified and the working tree file was not. If both files
have been modified, print a warning and exit with an error.
Note that we cannot use an existing index in git-difftool since those
contain the modified files that need to be checked out but here we are
looking at those files which are copied from the working tree and not
checked out. These are precisely the files which are not in the
existing indices.
Signed-off-by: John Keeping j...@keeping.me.uk
---
Changes since v1:
- Lazily initialize *_modified hashes to avoid executing Git commands
in the common case where symlinks are in use and the diff tool writes
to the file through the symlink
- Use exists consistently when checking *_modified hashes
git-difftool.perl | 85 -
t/t7800-difftool.sh | 30 +++
2 files changed, 101 insertions(+), 14 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 663640d..6780292 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,9 +13,9 @@
use 5.008;
use strict;
use warnings;
+use Error qw(:try);
use File::Basename qw(dirname);
use File::Copy;
-use File::Compare;
use File::Find;
use File::stat;
use File::Path qw(mkpath rmtree);
@@ -88,14 +88,45 @@ sub use_wt_file
my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
my $null_sha1 = '0' x 40;
- if ($sha1 eq $null_sha1) {
- return 1;
- } elsif (not $symlinks) {
+ if ($sha1 ne $null_sha1 and not $symlinks) {
return 0;
}
my $wt_sha1 = $repo-command_oneline('hash-object', $workdir/$file);
- return $sha1 eq $wt_sha1;
+ my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+ return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+ my ($repo_path, $index, $worktree) = @_;
+ $ENV{GIT_INDEX_FILE} = $index;
+ $ENV{GIT_WORK_TREE} = $worktree;
+ my $must_unset_git_dir = 0;
+ if (not defined($ENV{GIT_DIR})) {
+ $must_unset_git_dir = 1;
+ $ENV{GIT_DIR} = $repo_path;
+ }
+
+ my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+ my @gitargs = qw/diff-files --name-only -z/;
+ try {
+ Git::command_oneline(@refreshargs);
+ } catch Git::Error::Command with {};
+
+ my $line = Git::command_oneline(@gitargs);
+ my @files;
+ if (defined $line) {
+ @files = split('\0', $line);
+ } else {
+ @files = ();
+ }
+
+ delete($ENV{GIT_INDEX_FILE});
+ delete($ENV{GIT_WORK_TREE});
+ delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+ return map { $_ = 1 } @files;
}
sub setup_dir_diff
@@ -121,6 +152,7 @@ sub setup_dir_diff
my $null_sha1 = '0' x 40;
my $lindex = '';
my $rindex = '';
+ my $wtindex = '';
my %submodule;
my %symlink;
my @working_tree = ();
@@ -174,8 +206,12 @@ EOF
}
if ($rmode ne $null_mode) {
- if (use_wt_file($repo, $workdir, $dst_path, $rsha1,
$symlinks)) {
- push(@working_tree, $dst_path);
+ my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+ $dst_path, $rsha1,
+ $symlinks);
+ if ($use) {
+ push @working_tree, $dst_path;
+ $wtindex .= $rmode $wt_sha1\t$dst_path\0;
} else {
$rindex .= $rmode $rsha1\t$dst_path\0;
}
@@ -218,6 +254,12 @@ EOF
$rc = system('git', 'checkout-index', '--all', --prefix=$rdir/);
exit_cleanup($tmpdir, $rc) if $rc != 0;
+ $ENV{GIT_INDEX_FILE} = $tmpdir/wtindex;
+ ($inpipe, $ctx) =
+ $repo-command_input_pipe(qw(update-index --info-only -z
--index-info));
+ print($inpipe $wtindex);
+ $repo-command_close_pipe($inpipe, $ctx);
+
# If $GIT_DIR was explicitly set just for the update/checkout
# commands, then it should be unset before continuing.
delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
@@ -390,19 +432,34 @@ sub dir_diff
# should be copied back to the working tree.
# Do not copy back files when symlinks are used and the
# external tool did not replace the original link with a file.
+ #
+ # These hashes are
What's cooking in git.git (Mar 2013, #08; Fri, 29)
What's cooking in git.git (Mar 2013, #08; Fri, 29) -- Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. A handful of topics that have been stalled for quite a while have been discarded; for those that are not superseded by something else, interested parties can still resubmit a reroll, but without any advances, we do not get any benefit from carrying them in my tree. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to master] * ap/combine-diff-ignore-whitespace (2013-03-14) 1 commit (merged to 'next' on 2013-03-19 at dfb2c98) + Allow combined diff to ignore white-spaces (this branch is used by ap/combine-diff-coalesce-lost.) Originally merged to 'next' on 2013-03-18 Teach diff --cc output to honor options to ignore various forms of whitespace changes. * jc/remove-treesame-parent-in-simplify-merges (2013-01-17) 1 commit (merged to 'next' on 2013-03-26 at 7999dbe) + simplify-merges: drop merge from irrelevant side branch The --simplify-merges logic did not cull irrelevant parents from a merge that is otherwise not interesting with respect to the paths we are following. This touches a fairly core part of the revision traversal infrastructure; even though I think this change is correct, please report immediately if you find any unintended side effect. * jk/checkout-attribute-lookup (2013-03-20) 3 commits (merged to 'next' on 2013-03-20 at 43a89e8) + t2003: work around path mangling issue on Windows (merged to 'next' on 2013-03-19 at b063a55) + entry: fix filter lookup + t2003: modernize style Codepath to stream blob object contents directly from the object store to filesystem did not use the correct path to find conversion filters when writing to temporary files. * jk/difftool-dir-diff-edit-fix (2013-03-14) 3 commits (merged to 'next' on 2013-03-19 at e68014a) + difftool --dir-diff: symlink all files matching the working tree + difftool: avoid double slashes in symlink targets + git-difftool(1): fix formatting of --symlink description Originally merged to 'next' on 2013-03-15 git difftool --dir-diff made symlinks to working tree files when preparing a temporary directory structure, so that accidental edits of these files in the difftool are reflected back to the working tree, but the logic to decide when to do so was not quite right. * kk/revwalk-slop-too-many-commit-within-a-second (2013-03-22) 1 commit (merged to 'next' on 2013-03-26 at ea90e75) + Fix revision walk for commits with the same dates Allow the revision slop code to look deeper while commits with exactly the same timestamps come next to each other (which can often happen after a large am and rebase session). * rr/tests-dedup-test-config (2013-03-19) 1 commit (merged to 'next' on 2013-03-26 at d314299) + t4018,7810,7811: remove test_config() redefinition * rs/archive-zip-raw-compression (2013-03-16) 1 commit (merged to 'next' on 2013-03-19 at 8cc1cb3) + archive-zip: use deflateInit2() to ask for raw compressed data Originally merged to 'next' on 2013-03-18 * yd/doc-is-in-asciidoc (2013-03-21) 1 commit (merged to 'next' on 2013-03-26 at a980af2) + CodingGuidelines: our documents are in AsciiDoc * yd/doc-merge-annotated-tag (2013-03-21) 1 commit (merged to 'next' on 2013-03-26 at a11162f) + Documentation: merging a tag is a special case Document the 1.7.9 feature to merge a signed tag and keep that in the mergetag header in the resulting commit better. * yd/use-test-config-unconfig (2013-03-25) 12 commits (merged to 'next' on 2013-03-26 at 55b69a9) + t7600: use test_config to set/unset git config variables + t7502: remove clear_config + t7502: use test_config to set/unset git config variables + t9500: use test_config to set/unset git config variables + t7508: use test_config to set/unset git config variables + t7500: use test_config to set/unset git config variables + t5541: use test_config to set/unset git config variables + t5520: use test_config to set/unset git config variables + t4202: use test_config/test_unconfig to set/unset git config variables + t4034: use test_config/test_unconfig to set/unset git config variables + t4304: use test_config to set/unset git config variables + t3400: use test_config to set/unset git config variables Bulk-update of the test suite. -- [New Topics] * js/log-gpg (2013-03-27) 1 commit - log: read gpg settings for signed commit verification Teach show/log honor gpg.program configuration just like other parts of the code that use GnuPG. Will merge to 'next'. * jc/t5516-pushInsteadOf-vs-pushURL (2013-03-28) 1 commit - t5516: test
Reading remote reflogs
Is it possible to somehow fetch the reflog of a remote? I would like to delegate some post-receive actions to an automatically mirrored clone of some repositories. Mirrored repositories don't maintain a reflog, even with core.logAllRefUpdates = true, so to be able to know what was pushed per push, it would need to somehow know the reflog of the origin. Of course a post-receive hook can send this information downstream, but I'd like to keep the origin 'dumb' and not do that. -- Dennis Kaarsemaker www.kaarsemaker.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Reading remote reflogs
Dennis Kaarsemaker den...@kaarsemaker.net writes: ... Mirrored repositories don't maintain a reflog, even with core.logAllRefUpdates = true,... Are you sure about this? When log_all_ref_updates is not set, by default we do not log for bare repositories, but other than that, we do not do anything special with respect to reflogs. $ (cd .. git clone --no-local --mirror git.git victim) $ git checkout next $ EDITOR=: git commit --amend $ cd ../victim $ git config core.logallrefupdates true $ git fetch $ find logs logs logs/HEAD logs/refs logs/refs/heads logs/refs/heads/next It seems to record how a branch at its origin was updated just fine, at least to me. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Reading remote reflogs
On vr, 2013-03-29 at 15:45 -0700, Junio C Hamano wrote: Dennis Kaarsemaker den...@kaarsemaker.net writes: ... Mirrored repositories don't maintain a reflog, even with core.logAllRefUpdates = true,... Are you sure about this? When log_all_ref_updates is not set, by default we do not log for bare repositories, but other than that, we do not do anything special with respect to reflogs. I was, as I tried the recipe below, though with a different repo. Must have goofed something up, as it works now. Thanks for the braincheck :) That gives me a reasonable approximation of distinct pushes if I pull the mirror often enough. It's always going to be an approximation though, so the original question still stands. -- Dennis Kaarsemaker www.kaarsemaker.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Reading remote reflogs
Dennis Kaarsemaker den...@kaarsemaker.net writes: On vr, 2013-03-29 at 15:45 -0700, Junio C Hamano wrote: Dennis Kaarsemaker den...@kaarsemaker.net writes: ... Mirrored repositories don't maintain a reflog, even with core.logAllRefUpdates = true,... Are you sure about this? When log_all_ref_updates is not set, by default we do not log for bare repositories, but other than that, we do not do anything special with respect to reflogs. I was, as I tried the recipe below, though with a different repo. Must have goofed something up, as it works now. Thanks for the braincheck :) That gives me a reasonable approximation of distinct pushes if I pull the mirror often enough. Instead of polling, why not git push --mirror whenever the original gets updated? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Reading remote reflogs
On vr, 2013-03-29 at 15:58 -0700, Junio C Hamano wrote: Dennis Kaarsemaker den...@kaarsemaker.net writes: On vr, 2013-03-29 at 15:45 -0700, Junio C Hamano wrote: Dennis Kaarsemaker den...@kaarsemaker.net writes: ... Mirrored repositories don't maintain a reflog, even with core.logAllRefUpdates = true,... Are you sure about this? When log_all_ref_updates is not set, by default we do not log for bare repositories, but other than that, we do not do anything special with respect to reflogs. I was, as I tried the recipe below, though with a different repo. Must have goofed something up, as it works now. Thanks for the braincheck :) That gives me a reasonable approximation of distinct pushes if I pull the mirror often enough. Instead of polling, why not git push --mirror whenever the original gets updated? I considered that, but it has two downsides: - Slows down the push as it needs to wait for this to complete - Only works if you control the master, so it won't work with e.g. github hosted repositories -- Dennis Kaarsemaker www.kaarsemaker.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/5] Verify GPG signatures when merging and extend %G? pretty string
I hope I did not introduce more problems than I fixed in this revision ;)
On 03/28/2013 11:33 PM, Junio C Hamano wrote:
It would be much easier to read if it were unless we are not
looking at the very first byte, the previous byte must be LF, i.e.
if (found != buf found[-1] != '\n')
Is that continue correct? Don't you want to retry from the end of
the line that contains the mistaken hit?
Actually it is not. Sorry.
The \n at the beginning anchors the expected string for quicker
multi-line scan done with strstr(). If you really want to lose that
LF and still write this function correctly and clearly, I think you
would need to iterate over the buffer line by line.
The function now does that and calls prefixcmp on each line.
On 03/28/2013 11:33 PM, Junio C Hamano wrote: Sebastian Götte
ja...@physik.tu-berlin.de writes:
+if (verify_signatures) {
+/* Verify the commit signatures */
+for (p = remoteheads; p; p = p-next) {
+struct commit *commit = p-item;
+struct signature signature;
+signature.check_result = 0;
[...]
By the way, I think this variable and type should be called more
like signature_check or something with check in its name, not
signature. After all it is _not_ a signature itself.
I renamed it signature_check. I put that into the commit moving the code to
commit.c. I also renamed the array/struct containing the GPG status output
strings since that was originally called signature_check.
+grep does not have a GPG signature mergeerror
Do we plan to make this message localized? If so I think you would
need to do this with test_i18ngrep.
Yes, this message should be localized since it is normal status output. I
fixed the test case, however I noticed a possible problem with the git log
--show-signature test case (t/t7510-signed-commit.sh): Here, grep is used on
git output, but this git output is actually just passed-through GPG output, and
GPG localizes that output. Are the tests alwasy run with LANG=en_US.utf-8,
LANG=C etc.?
+test_expect_success GPG 'merge commit with bad signature with verification'
'
+test_must_fail git merge --ff-only --verify-signatures $(cat
forged.commit) 2 mergeerror
+grep has a bad GPG signature mergeerror
+'
+
+test_expect_success GPG 'merge signed commit with verification' '
+git merge -v --ff-only --verify-signatures side-signed mergeoutput
+grep has a good GPG signature mergeoutput
+'
Hmph.
So the caller needs to check both the standard output and the
standard error to get the whole picture? Is there a reason why we
are not sending everything to the standard output consistently?
If --verify-signatures is given, everything but a good signature results in
die(_(foo)), with _(foo) being printed on stderr. I clarified that point
in merge-options.txt. If additionally --verbose is given on the command line,
git will print _(Commit %s has a good GPG signature by %s (key fingerprint
%s)\n) on stdout for each good commit. I think it is ok that way because
the caller only needs to check the exit code to get a picture. The good GPG
signature-message is only meant to convey the signer's name and key
fingerprint in case the caller is interested. If the caller does not want to
abort the merge in case of GPG trouble, git merge may be called without
--verify-signatures followed by a git log --show-signature on the commits
that have been merged.
Sebastian Götte (5):
Move commit GPG signature verification to commit.c
commit.c/GPG signature verification: Also look at the first GPG status
line
merge/pull: verify GPG signatures of commits being merged
merge/pull Check for untrusted good GPG signatures
pretty printing: extend %G? to include 'N' and 'U'
Documentation/merge-options.txt| 5 ++
Documentation/pretty-formats.txt | 3 +-
builtin/merge.c| 35 +-
commit.c | 67 ++
commit.h | 10
git-pull.sh| 10 +++-
gpg-interface.h| 8
pretty.c | 93 ++---
t/lib-gpg/pubring.gpg | Bin 1164 - 2359 bytes
t/lib-gpg/random_seed | Bin 600 - 600 bytes
t/lib-gpg/secring.gpg | Bin 1237 - 3734 bytes
t/lib-gpg/trustdb.gpg | Bin 1280 - 1360 bytes
t/t7612-merge-verify-signatures.sh | 61
13 files changed, 210 insertions(+), 82 deletions(-)
create mode 100755 t/t7612-merge-verify-signatures.sh
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/5] commit.c/GPG signature verification: Also look at the first GPG status line
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
commit.c | 34 --
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/commit.c b/commit.c
index e94d122..48f09e9 100644
--- a/commit.c
+++ b/commit.c
@@ -1027,27 +1027,33 @@ static struct {
char result;
const char *check;
} sigcheck_gpg_status[] = {
- { 'G', \n[GNUPG:] GOODSIG },
- { 'B', \n[GNUPG:] BADSIG },
+ { 'G', [GNUPG:] GOODSIG },
+ { 'B', [GNUPG:] BADSIG },
};
static void parse_gpg_output(struct signature_check *sigc)
{
- const char *buf = sigc-gpg_status;
int i;
+ /* Iterate over all search strings */
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
- const char *found = strstr(buf, sigcheck_gpg_status[i].check);
- const char *next;
- if (!found)
- continue;
- sigc-check_result = sigcheck_gpg_status[i].result;
- found += strlen(sigcheck_gpg_status[i].check);
- sigc-key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc-signer = xmemdupz(found, next - found);
- break;
+ const char *found = sigc-gpg_status;
+
+ /* Iterate over all lines */
+ do {
+ if (!prefixcmp(found, sigcheck_gpg_status[i].check)) {
+ const char *next;
+
+ found += strlen(sigcheck_gpg_status[i].check);
+ sigc-check_result =
sigcheck_gpg_status[i].result;
+ sigc-key = xmemdupz(found, 16);
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc-signer = xmemdupz(found, next - found);
+ return;
+ }
+ found = strchr(found, '\n')+1;
+ } while(found-1);
}
}
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/5] Move commit GPG signature verification to commit.c
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
commit.c| 59 +
commit.h| 9 ++
gpg-interface.h | 8 +
pretty.c| 91 +
4 files changed, 89 insertions(+), 78 deletions(-)
diff --git a/commit.c b/commit.c
index e8eb0ae..e94d122 100644
--- a/commit.c
+++ b/commit.c
@@ -1023,6 +1023,65 @@ free_return:
free(buf);
}
+static struct {
+ char result;
+ const char *check;
+} sigcheck_gpg_status[] = {
+ { 'G', \n[GNUPG:] GOODSIG },
+ { 'B', \n[GNUPG:] BADSIG },
+};
+
+static void parse_gpg_output(struct signature_check *sigc)
+{
+ const char *buf = sigc-gpg_status;
+ int i;
+
+ for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
+ const char *found = strstr(buf, sigcheck_gpg_status[i].check);
+ const char *next;
+ if (!found)
+ continue;
+ sigc-check_result = sigcheck_gpg_status[i].result;
+ found += strlen(sigcheck_gpg_status[i].check);
+ sigc-key = xmemdupz(found, 16);
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc-signer = xmemdupz(found, next - found);
+ break;
+ }
+}
+
+void check_commit_signature(const struct commit* commit, struct
signature_check *sigc)
+{
+ struct strbuf payload = STRBUF_INIT;
+ struct strbuf signature = STRBUF_INIT;
+ struct strbuf gpg_output = STRBUF_INIT;
+ struct strbuf gpg_status = STRBUF_INIT;
+ int status;
+
+ sigc-check_result = 'N';
+
+ if (parse_signed_commit(commit-object.sha1,
+ payload, signature) = 0)
+ goto out;
+ status = verify_signed_buffer(payload.buf, payload.len,
+ signature.buf, signature.len,
+ gpg_output, gpg_status);
+ if (status !gpg_output.len)
+ goto out;
+ sigc-gpg_output = strbuf_detach(gpg_output, NULL);
+ sigc-gpg_status = strbuf_detach(gpg_status, NULL);
+ parse_gpg_output(sigc);
+
+ out:
+ strbuf_release(gpg_status);
+ strbuf_release(gpg_output);
+ strbuf_release(payload);
+ strbuf_release(signature);
+}
+
+
+
void append_merge_tag_headers(struct commit_list *parents,
struct commit_extra_header ***tail)
{
diff --git a/commit.h b/commit.h
index 4138bb4..941f7f3 100644
--- a/commit.h
+++ b/commit.h
@@ -5,6 +5,7 @@
#include tree.h
#include strbuf.h
#include decorate.h
+#include gpg-interface.h
struct commit_list {
struct commit *item;
@@ -230,4 +231,12 @@ extern void print_commit_list(struct commit_list *list,
const char *format_cur,
const char *format_last);
+/*
+ * Check the signature of the given commit. The result of the check is stored
in
+ * sig-check_result, 'G' for a good signature, 'B' for a bad signature and 'N'
+ * for no signature at all.
+ * This may allocate memory for sig-gpg_output, sig-gpg_status, sig-signer
and sig-key.
+ */
+extern void check_commit_signature(const struct commit* commit, struct
signature_check *sigc);
+
#endif /* COMMIT_H */
diff --git a/gpg-interface.h b/gpg-interface.h
index cf99021..44f70aa 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -1,6 +1,14 @@
#ifndef GPG_INTERFACE_H
#define GPG_INTERFACE_H
+struct signature_check {
+ char *gpg_output;
+ char *gpg_status;
+ char check_result; /* 0 (not checked), N (checked but no further
result), G (good) or B (bad) */
+ char *signer;
+ char *key;
+};
+
extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const
char *signing_key);
extern int verify_signed_buffer(const char *payload, size_t payload_size,
const char *signature, size_t signature_size, struct strbuf *gpg_output, struct
strbuf *gpg_status);
extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index b57adef..da3c53c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -756,14 +756,7 @@ struct format_commit_context {
const struct pretty_print_context *pretty_ctx;
unsigned commit_header_parsed:1;
unsigned commit_message_parsed:1;
- unsigned commit_signature_parsed:1;
- struct {
- char *gpg_output;
- char *gpg_status;
- char good_bad;
- char *signer;
- char *key;
- } signature;
+ struct signature_check signature_check;
char *message;
size_t width, indent1, indent2;
@@ -946,64 +939,6 @@ static void rewrap_message_tail(struct strbuf *sb,
c-indent2 = new_indent2;
}
-static struct {
- char result;
- const char *check;
-} signature_check[] = {
- { 'G', \n[GNUPG:]
[PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
When --verify-signatures is specified, abort the merge in case a good
GPG signature from an untrusted key is encountered.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/merge-options.txt| 4 ++--
builtin/merge.c| 2 ++
commit.c | 2 ++
commit.h | 9 +
gpg-interface.h| 2 +-
t/lib-gpg/pubring.gpg | Bin 1164 - 2359 bytes
t/lib-gpg/random_seed | Bin 600 - 600 bytes
t/lib-gpg/secring.gpg | Bin 1237 - 3734 bytes
t/lib-gpg/trustdb.gpg | Bin 1280 - 1360 bytes
t/t7612-merge-verify-signatures.sh | 9 +
10 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 31f1067..a0f022b 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -85,8 +85,8 @@ option can be used to override --squash.
--verify-signatures::
--no-verify-signatures::
- Verify that the commits being merged have good GPG signatures and abort
the
- merge in case they do not.
+ Verify that the commits being merged have good and trusted GPG
signatures
+ and abort the merge in case they do not.
--summary::
--no-summary::
diff --git a/builtin/merge.c b/builtin/merge.c
index cb3e9ea..5b2ece1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1251,6 +1251,8 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
if (verbosity = 0)
printf(_(Commit %s has a good
GPG signature by %s (key fingerprint %s)\n), hex, signature_check.signer,
signature_check.key);
break;
+ case 'U':
+ die(_(Commit %s has a good GPG
signature allegedly by %s, albeit from an untrusted key (fingerprint %s).),
hex, signature_check.signer, signature_check.key);
case 'B':
die(_(Commit %s has a bad GPG
signature allegedly by %s (key fingerprint %s).), hex, signature_check.signer,
signature_check.key);
default: /* 'N' */
diff --git a/commit.c b/commit.c
index 48f09e9..b132c15 100644
--- a/commit.c
+++ b/commit.c
@@ -1027,6 +1027,8 @@ static struct {
char result;
const char *check;
} sigcheck_gpg_status[] = {
+ { 'U', [GNUPG:] TRUST_UNDEFINED },
+ { 'U', [GNUPG:] TRUST_NEVER },
{ 'G', [GNUPG:] GOODSIG },
{ 'B', [GNUPG:] BADSIG },
};
diff --git a/commit.h b/commit.h
index 941f7f3..27d9b36 100644
--- a/commit.h
+++ b/commit.h
@@ -232,10 +232,11 @@ extern void print_commit_list(struct commit_list *list,
const char *format_last);
/*
- * Check the signature of the given commit. The result of the check is stored
in
- * sig-check_result, 'G' for a good signature, 'B' for a bad signature and 'N'
- * for no signature at all.
- * This may allocate memory for sig-gpg_output, sig-gpg_status, sig-signer
and sig-key.
+ * Check the signature of the given commit. The result of the check is stored
+ * in sig-check_result, 'G' for a good signature, 'U' for a good signature
+ * from an untrusted signer, 'B' for a bad signature and 'N' for no signature
+ * at all. This may allocate memory for sig-gpg_output, sig-gpg_status,
+ * sig-signer and sig-key.
*/
extern void check_commit_signature(const struct commit* commit, struct
signature_check *sigc);
diff --git a/gpg-interface.h b/gpg-interface.h
index 44f70aa..d2e512d 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -4,7 +4,7 @@
struct signature_check {
char *gpg_output;
char *gpg_status;
- char check_result; /* 0 (not checked), N (checked but no further
result), G (good) or B (bad) */
+ char check_result; /* 0 (not checked), N (checked but no further
result), U (untrusted, good), G (good) or B (bad) */
char *signer;
char *key;
};
diff --git a/t/lib-gpg/pubring.gpg b/t/lib-gpg/pubring.gpg
index
83855fa4e1c6c37afe550c17afa1e7971042ded5..1a3c2d487c2fda9169751a3068fa51e853a1e519
100644
GIT binary patch
delta 1212
zcmV;t1Vj6b3AYlkj0As~0SyFEOqNFh2mr~8{AU1PG9!Ku9w|}@vpZJPg*#s86v-*O
zhafj(DLlFA0)`tvC$E@WHJ2r~0{0ZCh1kHo$b9ih^aD*~)oVvKyC1(yi)6x_y
zF8V3JpbIY^ZYQUk#j*ja0`;jw^J+5~!h3qc)ej%g1;Wb0U8cXSuLKdXkx2PUtB4
zzqQO;r+6QaVFd?ZkA#8Ch(2p+QaoKR;K0{BU;l_DqGU2YCt6Fr4psgB*tPou
z8C@tnj=CqkffSLs=a-5G+h;Y8bkws-iIgT{$f^j!)l`hmkz}$nZPxBoPmwU;zIH^
zLN~K=bOhGBes9{Fcqzh%Wj2h^{ZjI01*KI0kkAVa%poQL}_zlZ*pX5VIVwYX((5
za%4bdcwudDY-KKPWpqA?0XPH`0RjLb1p-k_mPY~`0|pBT2nPcK1{DYb2?`4Y76JnS
z0v-VZ7k~f?2@qikE`_%uafw)?2m1%-{uDP0Rs1|(F{OVhOSKoH$k!o)x3CO5Z^@El
zxGq=+xQGRr@3$S0@SUu@@UuxUH%pD3Q-D`?37Vg*xmo%+KD)(U~kXU^b+=70V
zFr?b3DvlFq^B*d%lz=lr!ch6nQbvcwWCD}n2DT3`r?RDkq=7r}qFvEhdfApA7yR;
[PATCH v5 5/5] pretty printing: extend %G? to include 'N' and 'U'
Expand %G? in pretty format strings to 'N' in case of no GPG signature
and 'U' in case of a good but untrusted GPG signature in addition to
the previous 'G'ood and 'B'ad. This eases writing anyting parsing
git-log output.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/pretty-formats.txt | 3 ++-
pretty.c | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 2939655..afac703 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -131,7 +131,8 @@ The placeholders are:
- '%B': raw body (unwrapped subject and body)
- '%N': commit notes
- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show either G for Good or B for Bad for a signed commit
+- '%G?': show G for a Good signature, B for a Bad signature, U for a
good,
+ untrusted signature and N for no signature
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}`
diff --git a/pretty.c b/pretty.c
index da3c53c..bbe521b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1135,6 +1135,8 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
switch (c-signature_check.check_result) {
case 'G':
case 'B':
+ case 'U':
+ case 'N':
strbuf_addch(sb,
c-signature_check.check_result);
}
break;
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/5] merge/pull: verify GPG signatures of commits being merged
When --verify-signatures is specified on the command-line of git-merge
or git-pull, check whether the commits being merged have good gpg
signatures and abort the merge in case they do not. This allows e.g.
auto-deployment from untrusted repo hosts.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/merge-options.txt| 5
builtin/merge.c| 33 +++-
git-pull.sh| 10 ++--
t/t7612-merge-verify-signatures.sh | 52 ++
4 files changed, 97 insertions(+), 3 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..31f1067 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -83,6 +83,11 @@ option can be used to override --squash.
Pass merge strategy specific option through to the merge
strategy.
+--verify-signatures::
+--no-verify-signatures::
+ Verify that the commits being merged have good GPG signatures and abort
the
+ merge in case they do not.
+
--summary::
--no-summary::
Synonyms to --stat and --no-stat; these are deprecated and will be
diff --git a/builtin/merge.c b/builtin/merge.c
index 7c8922c..cb3e9ea 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -49,7 +49,7 @@ static const char * const builtin_merge_usage[] = {
static int show_diffstat = 1, shortlog_len = -1, squash;
static int option_commit = 1, allow_fast_forward = 1;
static int fast_forward_only, option_edit = -1;
-static int allow_trivial = 1, have_message;
+static int allow_trivial = 1, have_message, verify_signatures;
static int overwrite_ignore = 1;
static struct strbuf merge_msg = STRBUF_INIT;
static struct strategy **use_strategies;
@@ -199,6 +199,8 @@ static struct option builtin_merge_options[] = {
OPT_BOOLEAN(0, ff-only, fast_forward_only,
N_(abort if fast-forward is not possible)),
OPT_RERERE_AUTOUPDATE(allow_rerere_auto),
+ OPT_BOOLEAN(0, verify-signatures, verify_signatures,
+ N_(Verify that the named commit has a valid GPG signature)),
OPT_CALLBACK('s', strategy, use_strategies, N_(strategy),
N_(merge strategy to use), option_parse_strategy),
OPT_CALLBACK('X', strategy-option, xopts, N_(option=value),
@@ -1233,6 +1235,35 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_merge_usage,
builtin_merge_options);
+ if (verify_signatures) {
+ /* Verify the commit signatures */
+ for (p = remoteheads; p; p = p-next) {
+ struct commit *commit = p-item;
+ char hex[41];
+ struct signature_check signature_check;
+ memset(signature_check, 0, sizeof(signature_check));
+
+ check_commit_signature(commit, signature_check);
+
+ strcpy(hex, find_unique_abbrev(commit-object.sha1,
DEFAULT_ABBREV));
+ switch(signature_check.check_result){
+ case 'G':
+ if (verbosity = 0)
+ printf(_(Commit %s has a good
GPG signature by %s (key fingerprint %s)\n), hex, signature_check.signer,
signature_check.key);
+ break;
+ case 'B':
+ die(_(Commit %s has a bad GPG
signature allegedly by %s (key fingerprint %s).), hex, signature_check.signer,
signature_check.key);
+ default: /* 'N' */
+ die(_(Commit %s does not have a good
GPG signature. In fact, commit %s does not have a GPG signature at all.), hex,
hex);
+ }
+
+ free(signature_check.gpg_output);
+ free(signature_check.gpg_status);
+ free(signature_check.signer);
+ free(signature_check.key);
+ }
+ }
+
strbuf_addstr(buf, merge);
for (p = remoteheads; p; p = p-next)
strbuf_addf(buf, %s, merge_remote_util(p-item)-name);
diff --git a/git-pull.sh b/git-pull.sh
index 266e682..705940d 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -39,7 +39,7 @@ test -z $(git ls-files -u) || die_conflict
test -f $GIT_DIR/MERGE_HEAD die_merge
strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
-log_arg= verbosity= progress= recurse_submodules=
+log_arg= verbosity= progress= recurse_submodules= verify_signatures=
merge_args= edit=
curr_branch=$(git symbolic-ref -q HEAD)
curr_branch_short=${curr_branch#refs/heads/}
@@ -125,6 +125,12 @@ do
--no-recurse-submodules)
recurse_submodules=--no-recurse-submodules
;;
+
Re: [PATCH] t1300: document some aesthetic failures of the config editor
On Fri, Mar 29, 2013 at 01:35:22PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: Here it is with the updated expectation. I don't care _that_ much, so if you feel strongly and want to drop the first test, feel free. As long as we are adding expect_failure without an immediate fix, let's document the ideal, with this patch on top. [...] test_expect_failure 'unsetting the last key in a section removes header' ' cat .git/config -\EOF + # some generic comment on the configuration file itself + # a comment specific to this section section. [section] # some intervening lines # that should also be dropped key = value EOF - expect + cat expect -\EOF + # some generic comment on the configuration file itself + EOF I think we may not attain that ideal without some natural language processing of the comments. But hey, no reason not to shoot for the stars. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Toy PATCH] Avoid spilling collided entries in object hash table to the next slots
On Sat, Mar 30, 2013 at 12:15 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: If a position in object hash table is taken, we currently check out the next one. This could potentially create long object chains. We could create linked lists instead and leave the next slot alone. In the current code, not just the logic in lookup_object(), but the logic to enforce load factor when create_object() decides to call grow_object_hash() and object enumeration implemented by get_max_object_index() and get_indexed_object() are closely tied to the open addressing scheme. If you want to switch to any other method (e.g. separate chaining) these need to be updated quite a bit. I do not see get_max_object_index() and get_indexed_object() updated at all. Do fsck, index-pack, name-rev and upload-pack still work? No, apparently not. I should have been hit hard by not updating grow_object_hash(). But I think it was ok because I was on top of my preallocation patch and there probably were not many chains before that patch kicked in. This particular implementation that uses a fake union is a bit ugly, but in general, it may be worth rethinking the object hash implementation. I vaguely recall trying cuckoo in the vicinity of this code. Yeah I saw that. Need to read and maybe try again some time. Still playing with the linked list idea. If every time we find something in a chain, we swap it to top (with hope it'll be accessed more often), then the number of traversing in chains goes down from 33m times to 21.5m times. If we just push it up one node in the linked list, it's 21.3m times. Interesting. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 09:44:32AM -0700, Junio C Hamano wrote: I tend to think in the longer term it may be a good idea to build with USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so in the end this may not matter that much I was thinking about that yesterday. After all, it's the purpose of USE_WILDMATCH and nd/retire-fnmatch series. So how about this patch to enable USE_WILDMATCH for a wider audience? I think it could get merged down to master. If wildmatch's bug history is messy by the time we enter rc cycles, we could revert the patch and cut new releases with fnmatch. If not, after 1.9 is released, I'll submit a patch to remove fnmatch compatiblity later and we will be on wildmatch only. -- 8 -- diff --git a/Makefile b/Makefile index 598d631..8a740f8 100644 --- a/Makefile +++ b/Makefile @@ -106,7 +106,7 @@ all:: # Define NO_FNMATCH_CASEFOLD if your fnmatch function doesn't have the # FNM_CASEFOLD GNU extension. # -# Define USE_WILDMATCH if you want to use Git's wildmatch +# Define NO_USE_WILDMATCH if you do not want to use Git's wildmatch # implementation as fnmatch # # Define NO_GECOS_IN_PWENT if you don't have pw_gecos in struct passwd @@ -1255,7 +1255,7 @@ ifdef NO_FNMATCH_CASEFOLD COMPAT_OBJS += compat/fnmatch/fnmatch.o endif endif -ifdef USE_WILDMATCH +ifndef NO_USE_WILDMATCH COMPAT_CFLAGS += -DUSE_WILDMATCH endif ifdef NO_SETENV -- 8 -- -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] commit.c/GPG signature verification: Also look at the first GPG status line
Sebastian Götte ja...@physik.tu-berlin.de writes:
static void parse_gpg_output(struct signature_check *sigc)
{
- const char *buf = sigc-gpg_status;
int i;
+ /* Iterate over all search strings */
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
+ const char *found = sigc-gpg_status;
+
+ /* Iterate over all lines */
+ do {
+ if (!prefixcmp(found, sigcheck_gpg_status[i].check)) {
+ const char *next;
+
+ found += strlen(sigcheck_gpg_status[i].check);
+ sigc-check_result =
sigcheck_gpg_status[i].result;
+ sigc-key = xmemdupz(found, 16);
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc-signer = xmemdupz(found, next - found);
+ return;
+ }
+ found = strchr(found, '\n')+1;
+ } while(found-1);
Yuck. That termination condition is horrible.
Honestly speaking, I find the one I suggested the other day (which
has been queued on 'pu') much nicer than this loop.
If you really really want to do a line at a time, discarding the
allow strstr() to scan over multiple lines optimization, it is
more natural to iterate over buffer one line at a time, and check
for each expected output with an inner loop, perhaps like this:
const char *cp = buf;
while (*cp) {
for (i = 0; i ARRAY_SIZE(sig_check); i++) {
if (!prefixcmp(cp, sig_check[i].check)
parse_gpg_status(sigc, cp, sig_check[i]))
return;
}
cp = strchrnul(cp, '\n');
if (*cp)
cp++;
}
But I do not see much point in doing so.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/5] Move commit GPG signature verification to commit.c
Sebastian Götte ja...@physik.tu-berlin.de writes:
@@ -230,4 +231,12 @@ extern void print_commit_list(struct commit_list *list,
const char *format_cur,
const char *format_last);
+/*
+ * Check the signature of the given commit. The result of the check is
stored in
+ * sig-check_result, 'G' for a good signature, 'B' for a bad signature and
'N'
+ * for no signature at all.
+ * This may allocate memory for sig-gpg_output, sig-gpg_status,
sig-signer and sig-key.
+ */
How wide is your terminal? These lines are a tad wider than our
standard. We tend to keep function decls in *.h files on a single
long line (primarily to help people who grep them without using
CTAGS/ETAGS), but everywhere else we try to keep most of our lines
comfortably fit on 80-column terminals.
+extern void check_commit_signature(const struct commit* commit, struct
signature_check *sigc);
+
#endif /* COMMIT_H */
diff --git a/gpg-interface.h b/gpg-interface.h
index cf99021..44f70aa 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -1,6 +1,14 @@
#ifndef GPG_INTERFACE_H
#define GPG_INTERFACE_H
+struct signature_check {
+ char *gpg_output;
+ char *gpg_status;
+ char check_result; /* 0 (not checked), N (checked but no further
result), G (good) or B (bad) */
Listing the possible values is a good idea, but not on a single
line.
Also now the structure screams with its name that it is about
checking, I do not see a reason for its field to repeat check.
Calling it result (or result_code) would avoid stuttering when
you use them, e.g.
struct signature_check signature_check;
switch (signature_check.check_result) {
...
would be less nice than
switch (signature_check.result) {
...
no?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/5] merge/pull: verify GPG signatures of commits being merged
Sebastian Götte ja...@physik.tu-berlin.de writes:
+ OPT_BOOLEAN(0, verify-signatures, verify_signatures,
+ N_(Verify that the named commit has a valid GPG signature)),
Please use OPT_BOOL() in new code. Verifying existing OPT_BOOLEAN()
can safely converted to OPT_BOOL() and doing so would be a separate
matter and should not be part of this series.
@@ -1233,6 +1235,35 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_merge_usage,
builtin_merge_options);
+ if (verify_signatures) {
+ /* Verify the commit signatures */
This boolean variable is named clearly enough that you do not need
this comment.
+ for (p = remoteheads; p; p = p-next) {
+ struct commit *commit = p-item;
+ char hex[41];
+ struct signature_check signature_check;
+ memset(signature_check, 0, sizeof(signature_check));
+
+ check_commit_signature(commit, signature_check);
+
+ strcpy(hex, find_unique_abbrev(commit-object.sha1,
DEFAULT_ABBREV));
+ switch(signature_check.check_result){
+ case 'G':
+ if (verbosity = 0)
+ printf(_(Commit %s has a good
GPG signature by %s (key fingerprint %s)\n), hex, signature_check.signer,
signature_check.key);
+ break;
+ case 'B':
+ die(_(Commit %s has a bad GPG
signature allegedly by %s (key fingerprint %s).), hex,
signature_check.signer, signature_check.key);
+ default: /* 'N' */
+ die(_(Commit %s does not have a good
GPG signature. In fact, commit %s does not have a GPG signature at all.),
hex, hex);
+ }
Style.
switch (expr) {
case 'G':
do_something_for_G();
break;
...
}
Also avoid overlong lines, both in the source, but pay extra
attention to what we show the user. For example:
Commit %s has a bad GPG signature allegedly by %s (key fingerprint %s).
The first %s will expand to 40 places, the other two are likely to
be around 20-30 places.
Commit %s does not have a good GPG signature. In fact, commit %s does not
have a GPG signature at all.
Drop everything from the beginning up to In fact, , perhaps:
Commit '%s' does not have any GPG signature.
is sufficient? You may also want to consider
die(_(Commit '%.*s...' does not have any GPG signature.),
8, hex);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in git rev-parse --verify
On 03/28/2013 05:52 PM, Junio C Hamano wrote:
You could force rev-parse to resolve the input to an existing
object, with something like this:
git rev-parse --verify $ARG^{}
It will unwrap a tag, so the output may end up pointing at a object
that is different from $ARG in such a case.
Yes, if unwrapping tags is OK then this would work.
But what is the purpose of turning a random string to a random
40-hex in the first place?
In non-trivial scripts, it makes sense to convert user input into a
known and verified quantity (SHA1) once, while processing external
inputs, and not have to think about it afterwards. Verifying and
converting to pure-SHA1s as soon as possible has several advantages:
1. An SHA1 is a canonical representation of the argument, useful for
example as the key in a hash map for for looking for the presence of a
commit in a rev-list output.
2. An SHA1 is persistent. For example, I use them when caching
benchmark results across versions. Moreover, they are safe for use in
filenames. The persistence also makes scripts more robust against
simultaneous changes to the repo by other processes, whereas if I use a
string like branch^ multiple times, there is no guarantee that it
always refers to the same commit.
3. Verifying arguments at one spot centralizes error-checking at the
start of a script and eliminates one reason for random git commands to
fail later (when error recovery is perhaps more difficult).
4. Converting once avoids the overhead of repeated conversion from a
free-form committish into an object name if the argument needs to be
passed to multiple git commands (though presumably the overhead is
insignificant in most cases).
Undoubtedly in many cases this practice of
early-verification-and-conversion is unnecessary, or the same benefit
could be had from either verifying or converting without doing both.
But verifying-and-converting is easy, cheap, and means not having to
decide on a case-by-case basis whether it could have been avoided.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in git rev-parse --verify
Michael Haggerty mhag...@alum.mit.edu writes:
1. An SHA1 is a canonical representation of the argument, useful for
example as the key in a hash map for for looking for the presence of a
commit in a rev-list output.
2. An SHA1 is persistent. For example, I use them when caching
benchmark results across versions. Moreover, they are safe for use in
filenames. The persistence also makes scripts more robust against
simultaneous changes to the repo by other processes, whereas if I use a
string like branch^ multiple times, there is no guarantee that it
always refers to the same commit.
These two are half-irrelevant; they only call for use of the current
rev-parse --verify that always gives you 40-hex. The more
important one is the next one.
3. Verifying arguments at one spot centralizes error-checking at the
start of a script and eliminates one reason for random git commands to
fail later (when error recovery is perhaps more difficult).
Not necessarily, unless your script is a very narrow corner case
that is satisfied with any kind of object goes. When a parameter
has to be commit for some purpose and another parameter can be any
tree-ish, you would want to validate them _with_ type requirement.
If you are using the object name persistency to create a cache of
how many times the word 'gogopuff' appears in the entire tree?,
you want to cache the result keyed with the object name of the tree,
whether your user gives you v1.8.0 (tag), v1.8.0^0 (commit), or
v1.8.0^{tree} (tree), and you want to unwrap the user input down to
tree object name to look up the pre-computed result for the cache to
be any useful.
4. Converting once avoids the overhead of repeated conversion from a
free-form committish into an object name if the argument needs to be
passed to multiple git commands (though presumably the overhead is
insignificant in most cases).
True.
So why not verify arguments while making sure of their type early
with 'rev-parse --verify $userinput^{desiredtype}?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in git rev-parse --verify
Junio C Hamano gits...@pobox.com writes: 3. Verifying arguments at one spot centralizes error-checking at the start of a script and eliminates one reason for random git commands to fail later (when error recovery is perhaps more difficult). Not necessarily, unless your script is a very narrow corner case that is satisfied with any kind of object goes. Clarification. I meant A single central point to check is a useful concept, but centrally checking 'does this exist? I do not care what it is' is not necessarily useful and I suspect it is not useful more often than it is. When you care if something exists, you often want to know what it is and other aspects of that thing at the same time. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
You might Select links of london charm bracelet In between An array of Searching
Kochi along with Trivandrum are the actual popular websites meant for searching with this specific The lord's Distinctive Condition. Searching Along with Tamil Nadu. This particular handicrafts associated with Tamil Nadu may get rid of the drop inside your budget. Towards the top of superb along with [b][url=http://buylinksoflondonstore.co.uk/]links of london friendship bracelet[/url][/b] elegance, this particular projects tend to be certainly thrilling along with tempting. [b] http://buylinksoflondonstore.co.uk/[/b] -- View this message in context: http://git.661346.n2.nabble.com/You-might-Select-links-of-london-charm-bracelet-In-between-An-array-of-Searching-tp7581098.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in git rev-parse --verify
On 03/30/2013 05:09 AM, Junio C Hamano wrote:
So why not verify arguments while making sure of their type early
with 'rev-parse --verify $userinput^{desiredtype}?
Yes, that's a better solution in almost all cases. Thanks for the tip.
(It doesn't change my opinion that the documentation for rev-parse
--verify is misleading, but given that you don't appear to want to
change its behavior I will submit a documentation patch.)
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
thomas sabo sale Are Verified When Obtaining Well-known To Its Avidnes
This is a reasonably irritating thing should you ever're unable to choose a great reward * thomas sabo charms http://www.cheapthomassaboukshop.co.uk/ * goods in your personalized buddy and also cherished one particular, becoming transferred in nearly special day. The initial thomas sabo charms listing which comes to your mind and body after all this should be Jones Sabo. This identification has become incredible into a great style of modern and stylish necklaces around the globe. Due to this states history the appropriate method for anyone stress on the options associated with a present. *http://www.cheapthomassaboukshop.co.uk/* -- View this message in context: http://git.661346.n2.nabble.com/thomas-sabo-sale-Are-Verified-When-Obtaining-Well-known-To-Its-Avidnes-tp7581100.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
