Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT
On Mon, Nov 13, 2017 at 12:53:15PM +0900, Junio C Hamano wrote: > > Thanks. This patch needs a sign-off, by the way. Signed-off-by: cbaile...@bloomberg.net (I can resend the full patch if required or if anyone requests futher changes. > > But that we should take it anyway regardless of that since it'll *also* > > work on Linux with your patch, and this logic makes some sense whereas > > the other one clearly didn't and just worked by pure accident of some > > toolchain semantics that I haven't figured out yet. > > That is curious and would be nice to know the answer to. The error that I was getting - if I remember the details of the very brief debugging session that I performed - was an unaligned memory access causing a SIGBUS in PCRE code whose function name contained 'jit' and which was being called indirectly from pcre_study. My guess is that we are just exposing a pre-existing bug in our Solaris build of libpcre. Unaligned memory accesses on x86 / x86_64 "only" cause performance issues rather than fatal signals so even if the same bug exists on Linux it probably has no noticeable effect (or at least no noticed effect). Charles.
[PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT
From: Charles Bailey If you have a pcre1 library which is compiled with JIT enabled then PCRE_STUDY_JIT_COMPILE will be defined whether or not the NO_LIBPCRE1_JIT configuration is set. This means that we enable JIT functionality when calling pcre_study even if NO_LIBPCRE1_JIT has been explicitly set and we just use plain pcre_exec later. Fix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to PCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to 0 otherwise, as before. --- I was bisecting an issue with the PCRE support that was causing a test suite failure on our Solaris builds and reached fbaceaac47 ("grep: add support for the PCRE v1 JIT API"). It appeared to be a misaligned memory access somewhere inside the libpcre code. I tried disabling the use of JIT with NO_LIBPCRE1_JIT but it turned out that even with this set we were still triggering the JIT code path in the call to pcre_study. Yes, we probably should fix our PCRE1 library build on Solaris or move to PCRE2, but really NO_LIBPCRE1_JIT should have prevented us from triggering this crash. grep.c | 2 +- grep.h | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/grep.c b/grep.c index ce6a48e..d0b9b6c 100644 --- a/grep.c +++ b/grep.c @@ -387,7 +387,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt) if (!p->pcre1_regexp) compile_regexp_failed(p, error); - p->pcre1_extra_info = pcre_study(p->pcre1_regexp, PCRE_STUDY_JIT_COMPILE, &error); + p->pcre1_extra_info = pcre_study(p->pcre1_regexp, GIT_PCRE_STUDY_JIT_COMPILE, &error); if (!p->pcre1_extra_info && error) die("%s", error); diff --git a/grep.h b/grep.h index 52aecfa..399381c 100644 --- a/grep.h +++ b/grep.h @@ -7,11 +7,12 @@ #if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32 #ifndef NO_LIBPCRE1_JIT #define GIT_PCRE1_USE_JIT +#define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE #endif #endif #endif -#ifndef PCRE_STUDY_JIT_COMPILE -#define PCRE_STUDY_JIT_COMPILE 0 +#ifndef GIT_PCRE_STUDY_JIT_COMPILE +#define GIT_PCRE_STUDY_JIT_COMPILE 0 #endif #if PCRE_MAJOR <= 8 && PCRE_MINOR < 20 typedef int pcre_jit_stack; -- 2.10.2
[PATCH] Make t4201-shortlog.sh test more robust
From: Charles Bailey The test for '--abbrev' in t4201-shortlog.sh assumes that the commits generated in the test can always be uniquely abbreviated to 5 hex digits but this is not always the case. If you were unlucky and happened to run the test at (say) Thu Jun 22 03:04:49 2017 +, you would find that the first commit generated would collide with a tree object created later in the same test. This can be simulated in the version of t4201-shortlog.sh prior to this commit by setting GIT_COMMITTER_DATE and GIT_AUTHOR_DATE to 1498100689 after sourcing test-lib.sh. Change the test to test --abbrev=35 instead of --abbrev=5 to almost completely avoid the possibility of a partial collision and add a call to test_tick in the setup to make the test repeatable. Signed-off-by: Charles Bailey --- t/t4201-shortlog.sh | 5 +++-- t/test-lib.sh | 7 --- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh index 9df054b..da10478 100755 --- a/t/t4201-shortlog.sh +++ b/t/t4201-shortlog.sh @@ -9,6 +9,7 @@ test_description='git shortlog . ./test-lib.sh test_expect_success 'setup' ' + test_tick && echo 1 >a1 && git add a1 && tree=$(git write-tree) && @@ -59,7 +60,7 @@ fuzz() { file=$1 && sed " s/$_x40/OBJECT_NAME/g - s/$_x05/OBJID/g + s/$_x35/OBJID/g s/^ \{6\}[CTa].*/ SUBJECT/g s/^ \{8\}[^ ].*/CONTINUATION/g " <"$file" >"$file.fuzzy" && @@ -81,7 +82,7 @@ test_expect_success 'pretty format' ' test_expect_success '--abbrev' ' sed s/SUBJECT/OBJID/ expect.template >expect && - git shortlog --format="%h" --abbrev=5 HEAD >log && + git shortlog --format="%h" --abbrev=35 HEAD >log && fuzz log >log.predictable && test_cmp expect log.predictable ' diff --git a/t/test-lib.sh b/t/test-lib.sh index 9b61f16..116bd6a 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -175,9 +175,10 @@ esac # Convenience # -# A regexp to match 5 and 40 hexdigits +# A regexp to match 5, 35 and 40 hexdigits _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' -_x40="$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05" +_x35="$_x05$_x05$_x05$_x05$_x05$_x05$_x05" +_x40="$_x35$_x05" # Zero SHA-1 _z40= @@ -193,7 +194,7 @@ LF=' # when case-folding filenames u200c=$(printf '\342\200\214') -export _x05 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB +export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB # Each test should start with something like this, after copyright notices: # -- 2.10.2
Re: Handling of paths
On Thu, Jul 20, 2017 at 01:30:52PM -0700, Junio C Hamano wrote: > > I've read the function again and I think the attached patch covers > everything that ought to be a filename. > > By the way, to credit you, do you prefer your bloomberg or hashpling > address? The patch looks good to me. It's not critical which address you credit. I mark patches which result from my work at Bloomberg with my Bloomberg email address and anything that I do entirely outside of work with my hashpling address, although I will tend to use my hashpling email for all communications because it co-operates with the mailing list conventions a lot better. In this case, this is a follow on from a cbaile...@bloomberg.net patch so crediting that address seems the more appropriate option. Charles.
Re: Handling of paths
On Thu, Jul 20, 2017 at 12:42:40PM -0700, Junio C Hamano wrote: > Victor Toni writes: > > > What's unexpected is that paths used for sslKey or sslCert are treated > > differently insofar as they are expected to be absolute. > > Relative paths (whether with or without "~") don't work. > > It appears that only two of these among four were made aware of the > "~[username]/" prefix in bf9acba2 ("http: treat config options > sslCAPath and sslCAInfo as paths", 2015-11-23), but "sslkey" and > "sslcert" were still left as plain vanilla strings. I do not know > if that was an elaborate omission, or a mere oversight, as it seems > that it happened while I was away, so... It was more of an oversight than a deliberate omission, but more accurately I didn't actively consider whether the other http.ssl* variables were pathname-like or not. At the time I was trying to make a config which needed to set http.sslCAPath and/or http.sslCAInfo more portable between users and these were "obviously" pathname-like to me. Now that I read the help for http.sslCert and http.sslKey, I see no reason that they shouldn't also use git_config_pathname. If I'd been more thorough I would have proposed this at the time. Charles.
Git hackathon London 8th/9th July - call for mentors
As some of you may have inferred, we had to postpone our plans for a hackathon event last month but we are now moving ahead with plans for one in London in July. Thank you to everyone who expressed interest in being a mentor after my last request. So, similar to last time... Bloomberg going to host a Git hackathon over a weekend in London on the weekend of 8th-9th July 2017. Crucial to the success of the weekend will be having mentors available in both locations who can guide people on the project. Mentors should have some experience with developing for Git and should be familiar with the process and guidelines around contributing. If you are interested in being a mentor or have further questions, then please get in contact with me via email (either to this address or to cbaile...@bloomberg.net). Also, whether or not you are able to be at the weekend now would be a great time to think about any projects which you would like tackled at this event. Obviously, projects should be completable by small teams inside of a weekend. Charles. --- Git was the first project for which we hosted an "Open Source Day" and since then we've learned a lot and would like to revisit Git again. The event will involve volunteers who are usually competent programmers but who don't necessarily have experience with contributing to Git, working to contribute to the project over two days. Typically the type of tasks tackled would include documentation improvements, test case improvements and very simple bug fixes that have previously been identified as "low hanging fruit".
Git hackathon New York / London - call for mentors
Bloomberg would like to host a Git hackathon over a weekend in both New York and London, towards the end of April or the beginning of May. Crucial to the success of the weekend will be having mentors available in both locations who can guide people on the project. Mentors should have some experience with developing for Git and should be familiar with the process and guidelines around contributing. If you are interested in being a mentor or have further questions, then please get in contact with me via email (either to this address or to cbaile...@bloomberg.net) letting me know whether you are closer to New York or London and if you have any date restrictions. Charles. --- Git was the first project for which we hosted an "Open Source Day" and since then we've learned a lot and would like to revisit Git again. The event will involve volunteers who are usually competent programmers but who don't necessarily have experience with contributing to Git, working to contribute to the project over two days. Typically the type of tasks tackled would include documentation improvements, test case improvements and very simple bug fixes that have previously been identified as "low hanging fruit".
Re: What's cooking in git.git (Jul 2016, #02; Wed, 6)
On Thu, Jul 07, 2016 at 02:21:28PM -0700, Junio C Hamano wrote: > Charles Bailey writes: > > > I just wanted to clarify what was actually fixed. The actual bug that > > was reported and fixed was the fact that 'git grep' (without --cached) > > wasn't searching the contents of files in the working tree if the index > > entry had the "intent to add" bit set. > > Ouch, you are absolutely right. > > Git does not know what the contents in the index should be for a > path added with "git add -N" yet, so "git grep --cached" should not > show hits (or show lack of hits, with -L) in such a path, but that > logic does not apply to "git grep", i.e. searching in the working > tree files. But we did so by mistake, which has been corrected. > > perhaps? Yes, that reads like an accurate summary 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: What's cooking in git.git (Jul 2016, #02; Wed, 6)
On Wed, Jul 06, 2016 at 02:39:26PM -0700, Junio C Hamano wrote: > * nd/ita-cleanup (2016-07-01) 3 commits > (merged to 'next' on 2016-07-06 at f15aeba) > + grep: fix grepping for "intent to add" files > + t7810-grep.sh: fix a whitespace inconsistency > + t7810-grep.sh: fix duplicated test name > > Git does not know what the contents in the index should be for a > path added with "git add -N" yet, so "git grep --cached" should not > show hits (or show lack of hits, with -L) in such a path. But we > did by mistake, which has been corrected. > > Will merge to 'master'. I just wanted to clarify what was actually fixed. The actual bug that was reported and fixed was the fact that 'git grep' (without --cached) wasn't searching the contents of files in the working tree if the index entry had the "intent to add" bit set. My original proposed fix (reversion of the commit that introduced this change) caused the re-introduction of behaviour where "i-to-a" files would be reported with -L --cached which wasn't desired. I think because we spent most of our energy and discussion on fixing this in a better way we've lost the intent of the original patch. -- 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 v3 1/3] t7810-grep.sh: fix duplicated test name
Signed-off-by: Charles Bailey --- t/t7810-grep.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index 1e72971..c4302ed 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -353,7 +353,7 @@ test_expect_success 'grep -l -C' ' cat >expected <actual && test_cmp expected actual ' -- 2.8.2.311.gee88674 -- 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 v3 0/3] Grepping with intent to add
So I've got back around to this topic again. I've applied fixes to the tests as suggested by Eric and Junio. I came up with a test case that demonstrates a difference between the additional fix that Duy suggested and the alternative that Junio suggested. I've kept Duy's fix because I think it makes more sense, although it's a sufficiently obscure case that I don't feel strongly that it's definitely the best behavior. The fix ensures that if you have a file which is both "intend to add" and "assume unchanged" that it is not listed if you "grep -L" for for something. In effect, we are applying the "contents indeterminate" state of the index to the working tree file. Charles Bailey (3): t7810-grep.sh: fix duplicated test name t7810-grep.sh: fix a whitespace inconsistency grep: fix grepping for "intent to add" files builtin/grep.c | 4 ++-- t/t7810-grep.sh | 62 +++-- 2 files changed, 62 insertions(+), 4 deletions(-) -- 2.8.2.311.gee88674 -- 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 v3 2/3] t7810-grep.sh: fix a whitespace inconsistency
Signed-off-by: Charles Bailey --- t/t7810-grep.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index c4302ed..6e6eaa4 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -175,7 +175,7 @@ do test_expect_success "grep -c $L (no /dev/null)" ' ! git grep -c test $H | grep /dev/null -' + ' test_expect_success "grep --max-depth -1 $L" ' { -- 2.8.2.311.gee88674 -- 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 v3 3/3] grep: fix grepping for "intent to add" files
From: Charles Bailey This reverts commit 4d5520053 (grep: make it clear i-t-a entries are ignored, 2015-12-27) and adds an alternative fix to maintain the -L --cached behavior. 4d5520053 caused 'git grep' to no longer find matches in new files in the working tree where the corresponding index entry had the "intent to add" bit set, despite the fact that these files are tracked. The content in the index of a file for which the "intent to add" bit is set is considered indeterminate and not empty. For most grep queries we want these to behave the same, however for -L --cached (files without a match) we don't want to respond positively for "intent to add" files as their contents are indeterminate. This is in contrast to files with empty contents in the index (no lines implies no matches for any grep query expression) which should be reported in the output of a grep -L --cached invocation. Add tests to cover this case and a few related cases which previously lacked coverage. Helped-by: Nguyễn Thái Ngọc Duy Signed-off-by: Charles Bailey --- builtin/grep.c | 4 ++-- t/t7810-grep.sh | 58 + 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 462e607..ae73831 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int for (nr = 0; nr < active_nr; nr++) { const struct cache_entry *ce = active_cache[nr]; - if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce)) + if (!S_ISREG(ce->ce_mode)) continue; if (!ce_path_match(ce, pathspec, NULL)) continue; @@ -396,7 +396,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int * cache version instead */ if (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) { - if (ce_stage(ce)) + if (ce_stage(ce) || ce_intent_to_add(ce)) continue; hit |= grep_sha1(opt, ce->sha1, ce->name, 0, ce->name); } diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index 6e6eaa4..c18e954 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -1364,4 +1364,62 @@ test_expect_success 'grep --color -e A --and -e B -p with context' ' test_cmp expected actual ' +test_expect_success 'grep can find things only in the work tree' ' + : >work-tree-only && + git add work-tree-only && + test_when_finished "git rm -f work-tree-only" && + echo "find in work tree" >work-tree-only && + git grep --quiet "find in work tree" && + test_must_fail git grep --quiet --cached "find in work tree" && + test_must_fail git grep --quiet "find in work tree" HEAD +' + +test_expect_success 'grep can find things only in the work tree (i-t-a)' ' + echo "intend to add this" >intend-to-add && + git add -N intend-to-add && + test_when_finished "git rm -f intend-to-add" && + git grep --quiet "intend to add this" && + test_must_fail git grep --quiet --cached "intend to add this" && + test_must_fail git grep --quiet "intend to add this" HEAD +' + +test_expect_success 'grep does not search work tree with assume unchanged' ' + echo "intend to add this" >intend-to-add && + git add -N intend-to-add && + git update-index --assume-unchanged intend-to-add && + test_when_finished "git rm -f intend-to-add" && + test_must_fail git grep --quiet "intend to add this" && + test_must_fail git grep --quiet --cached "intend to add this" && + test_must_fail git grep --quiet "intend to add this" HEAD +' + +test_expect_success 'grep can find things only in the index' ' + echo "only in the index" >cache-this && + git add cache-this && + rm cache-this && + test_when_finished "git rm --cached cache-this" && + test_must_fail git grep --quiet "only in the index" && + git grep --quiet --cached "only in the index" && + test_must_fail git grep --quiet "only in the index" HEAD +' + +test_expect_success 'grep does not report i-t-a with -L --cached' ' + echo "intend to add this" >intend-to-add
[PATCH v2 1/2] Fix duplicated test name
Signed-off-by: Charles Bailey --- Spotted while testing t7810-grep and grep "i-t-a" fixes. t/t7810-grep.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index 1e72971..c4302ed 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -353,7 +353,7 @@ test_expect_success 'grep -l -C' ' cat >expected <actual && test_cmp expected actual ' -- 2.8.2.311.gee88674 -- 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 v2 2/2] grep: fix grepping for "intent to add" files
From: Charles Bailey This reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e and adds an alternative fix to maintain the -L --cached behavior. 4d5520053 caused 'git grep' to no longer find matches in new files in the working tree where the corresponding index entry had the "intent to add" bit set, despite the fact that these files are tracked. The content in the index of a file for which the "intent to add" bit is set is considered indeterminate and not empty. For most grep queries we want these to behave the same, however for -L --cached (files without a match) we don't want to respond positively for "intent to add" files as their contents are indeterminate. This is in contrast to files with empty contents in the index (no lines implies no matches for any grep query expression) which should be reported in the output of a grep -L --cached invocation. Add tests to cover this case and a few related cases which previously lacked coverage. Helped-by: Nguyễn Thái Ngọc Duy Signed-off-by: Charles Bailey --- Is "Helped-by" an appropriate attribution in this case? Personally, I could have gone either way on the -L --cached functionality but I know that Duy has put a lot more thought into "intent-to-add" entries so I trust his judgement. builtin/grep.c | 4 ++-- t/t7810-grep.sh | 38 ++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 462e607..ae73831 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int for (nr = 0; nr < active_nr; nr++) { const struct cache_entry *ce = active_cache[nr]; - if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce)) + if (!S_ISREG(ce->ce_mode)) continue; if (!ce_path_match(ce, pathspec, NULL)) continue; @@ -396,7 +396,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int * cache version instead */ if (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) { - if (ce_stage(ce)) + if (ce_stage(ce) || ce_intent_to_add(ce)) continue; hit |= grep_sha1(opt, ce->sha1, ce->name, 0, ce->name); } diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index c4302ed..6c7ccb3 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -1364,4 +1364,42 @@ test_expect_success 'grep --color -e A --and -e B -p with context' ' test_cmp expected actual ' +test_expect_success 'grep can find things only in the work tree' ' + touch work-tree-only && + git add work-tree-only && + echo "find in work tree" >work-tree-only && + git grep --quiet "find in work tree" && + test_must_fail git grep --quiet --cached "find in work tree" && + test_must_fail git grep --quiet "find in work tree" HEAD && + git rm -f work-tree-only +' + +test_expect_success 'grep can find things only in the work tree (i-t-a)' ' + echo "intend to add this" >intend-to-add && + git add -N intend-to-add && + git grep --quiet "intend to add this" && + test_must_fail git grep --quiet --cached "intend to add this" && + test_must_fail git grep --quiet "intend to add this" HEAD && + git rm -f intend-to-add +' + +test_expect_success 'grep can find things only in the index' ' + echo "only in the index" >cache-this && + git add cache-this && + rm cache-this && + test_must_fail git grep --quiet "only in the index" && + git grep --quiet --cached "only in the index" && + test_must_fail git grep --quiet "only in the index" HEAD && + git rm --cached cache-this +' + +test_expect_success 'grep does not report i-t-a with -L --cached' ' + echo "intend to add this" >intend-to-add && + git add -N intend-to-add && + git ls-files | grep -v "^intend-to-add\$" >expected && + git grep -L --cached "nonexistent_string" >actual && + test_cmp expected actual && + git rm -f intend-to-add +' + test_done -- 2.8.2.311.gee88674 -- 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] grep: fix grepping for "intent to add" files
On Thu, Jun 16, 2016 at 05:57:18PM +0700, Duy Nguyen wrote: > > "git grep --cached" searches file content that will be committed by > "git commit" (no -a). An i-t-a entry will not be committed (you would > need "git add" first, or do "git commit -a"). So if I say "search > among the to-be-committed file content, list files that do not match > abc" (git grep -l -v --cached abc), the i-t-a entry will show up > because its fake content is empty (i.e. not contain "abc"), even > though it's not in the "to-be-committed" list. So yeah, correctness > issue. OK, I think there is an issue there but it's not with "-l -v --cached" but rather with "-L --cached". If my understanding is correct, "-l -v" means "has a line that doesn't match" whereas "-L" means "has no line that matches". Does this sound correct? I'll try adding a new test. -- 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] grep: fix grepping for "intent to add" files
On Thu, Jun 16, 2016 at 02:47:09PM +0700, Duy Nguyen wrote: > I don't think revert is right. It rather needs a re-fix like below. > Basically we want grep_file() to run as normal, but grep_sha1() > (i.e. git grep --cached) should ignore i-t-a entries, because empty > SHA-1 is not the right content to grep. It does not matter in positive > matching, sure, but it may in -v cache. You don't think the revert is correct or you don't think the revert is sufficient? (I wasn't able to find a test case which proved that the change to line 399 was necessary, so perhaps I don't understand.) I would have thought that grepping the empty SHA-1 would be correct for with or without -v. An "intent to add" file has no content in the index so I would expect it to have zero matching and zero non-matching lines for any grep --cached query? Or is this an efficiency and not a correctness concern? Charles. -- 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] grep: fix grepping for "intent to add" files
From: Charles Bailey This reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e. This commit caused 'git grep' to no longer find matches in new files in the working tree where the corresponding index entry had the "intent to add" bit set. Add tests to cover this case and a few related cases which previously lacked coverage. Signed-off-by: Charles Bailey --- Originally discussed: http://thread.gmane.org/gmane.comp.version-control.git/272363/focus=276358 http://thread.gmane.org/gmane.comp.version-control.git/283001/focus=283002 Unless I've misunderstood the conversation and commit message, the referenced commit was supposed to be a "code as a comment" commit with no change in observable behavior however a user was surprised that 'git grep' couldn't find something that regular grep could, despite the file being tracked - albeit new and "intended to add". builtin/grep.c | 2 +- t/t7810-grep.sh | 29 + 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/builtin/grep.c b/builtin/grep.c index 462e607..d5aacba 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int for (nr = 0; nr < active_nr; nr++) { const struct cache_entry *ce = active_cache[nr]; - if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce)) + if (!S_ISREG(ce->ce_mode)) continue; if (!ce_path_match(ce, pathspec, NULL)) continue; diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index 1e72971..eae731a 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -1364,4 +1364,33 @@ test_expect_success 'grep --color -e A --and -e B -p with context' ' test_cmp expected actual ' +test_expect_success 'grep can find things only in the work tree' ' + touch work-tree-only && + git add work-tree-only && + echo "find in work tree" >work-tree-only && + git grep --quiet "find in work tree" && + test_must_fail git grep --quiet --cached "find in work tree" && + test_must_fail git grep --quiet "find in work tree" HEAD && + git rm -f work-tree-only +' + +test_expect_success 'grep can find things only in the work tree (i-t-a)' ' + echo "intend to add this" >intend-to-add && + git add -N intend-to-add && + git grep --quiet "intend to add this" && + test_must_fail git grep --quiet --cached "intend to add this" && + test_must_fail git grep --quiet "intend to add this" HEAD && + git rm -f intend-to-add +' + +test_expect_success 'grep can find things only in the index' ' + echo "only in the index" >cache-this && + git add cache-this && + rm cache-this && + test_must_fail git grep --quiet "only in the index" && + git grep --quiet --cached "only in the index" && + test_must_fail git grep --quiet "only in the index" HEAD && + git rm --cached cache-this +' + test_done -- 2.8.2.311.gee88674 -- 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: git branch in Bash subshell "$(git branch -a)" including ls output as part of return?
On Mon, Dec 07, 2015 at 04:58:10PM +, Charles Bailey wrote: > > Looking at the two outputs, you are seeing the shell's glob expansion of > the '*' current branch marker. You probably want to quote the command > expansion to prevent this: > > echo "$(git branch -a)" Pressing send has, of course, caused me to think further. You probably don't want to parse the output of a "porcelain" command such as "git branch" at all, but instead look at using something like "git for-each-ref", perhaps with the --format=%(refname) option, grepping out master and iterating through the rest. -- 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: git branch in Bash subshell "$(git branch -a)" including ls output as part of return?
On Mon, Dec 07, 2015 at 11:52:28AM -0500, Alex Jones wrote: > git branch -a output: > > ajonespro:Deploy_Script ajones$ git branch -a > > * DWH_concurrent_api > Email_No_Error_If_No_Old_Version > IT/configs_in_app_support > PHP_Build_Repo > master > remotes/origin/DWH_concurrent_api > remotes/origin/Email_No_Error_If_No_Old_Version > remotes/origin/IT/configs_in_app_support > remotes/origin/PHP_Build_Repo > remotes/origin/master > > echo $(git branch -a) output: > > ajonespro:Deploy_Script ajones$ echo $(git branch -a) > AppDeploy WebDeploy DWH_concurrent_api > Email_No_Error_If_No_Old_Version IT/configs_in_app_support > PHP_Build_Repo master remotes/origin/DWH_concurrent_api > remotes/origin/Email_No_Error_If_No_Old_Version > remotes/origin/IT/configs_in_app_support remotes/origin/PHP_Build_Repo > remotes/origin/master > > While it might be hard to see from that output, The first two > "branches" in the subshell's output are actually the directories > contained within the repo. Looking at the two outputs, you are seeing the shell's glob expansion of the '*' current branch marker. You probably want to quote the command expansion to prevent this: echo "$(git branch -a)" -- 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] Fix quoting of redirect in test script
On Fri, Dec 04, 2015 at 01:05:20PM -0800, Junio C Hamano wrote: > Do you want to sign-off this patch? > > Thanks. Oops, yes please. Signed-off-by: Charles Bailey -- 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] Fix quoting of redirect in test script
From: Charles Bailey --- t/t3404-rebase-interactive.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) If you are using bash (at least 4.3.30 or 4.3.42) this actually causes an error due to an "ambiguous redirect" because there is a space in "trash directory". diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 98eb49a..9067e02 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1234,7 +1234,7 @@ test_expect_success 'tabs and spaces are accepted in the todolist' ' # Turn single spaces into space/tab mix sed "1s/ / /g; 2s/ / /g; 3s/ //g" "$1" printf "\n\t# comment\n #more\n\t # comment\n" - ) >$1.new + ) >"$1.new" mv "$1.new" "$1" EOF test_set_editor "$(pwd)/add-indent.sh" && -- 2.4.0.53.g8440f74 -- 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 v2] Fix detection of uname failure
From: Charles Bailey According to POSIX specification uname must return -1 on failure and a non-negative value on success. Although many implementations do return 0 on success it is valid to return any positive value for success. In particular, Solaris returns 1. Signed-off-by: Charles Bailey --- dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dir.c b/dir.c index 8209f8b..1d42811 100644 --- a/dir.c +++ b/dir.c @@ -1848,7 +1848,7 @@ static const char *get_ident_string(void) if (sb.len) return sb.buf; - if (uname(&uts)) + if (uname(&uts) < 0) die_errno(_("failed to get kernel name and information")); strbuf_addf(&sb, "Location %s, system %s %s %s", get_git_work_tree(), uts.sysname, uts.release, uts.version); -- 2.4.0.53.g8440f74 -- 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] Fix detection of uname failure
On Fri, Jul 17, 2015 at 03:06:57PM +0200, Johannes Schindelin wrote: > > From a quick `git grep '== -1'` and another quick `git grep '< 0'` it appears > to me that we prefer the latter. Maybe you want to adjust it in the patch, > too? I did the same grep and found lots of examples of both. Many of the "< 0" applied to comparisons with variables and not API calls and many were internal (to git) calls and not POSIX or C library calls so I wasn't convinced to change my initial fix. Having said that and thought about it some more, I think '< 0' is probably better. In POSIX, we shouldn't ever get a negative value which isn't -1, but if we ever do it is probably safer to fail. I'll send and update. Charles. -- 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] Fix detection of uname failure
From: Charles Bailey According to POSIX specification uname must return -1 on failure and a non-negative value on success. Although many implementations do return 0 on success it is valid to return any positive value for success. In particular, Solaris returns 1. Signed-off-by: Charles Bailey --- dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dir.c b/dir.c index 8209f8b..52dbfd0 100644 --- a/dir.c +++ b/dir.c @@ -1848,7 +1848,7 @@ static const char *get_ident_string(void) if (sb.len) return sb.buf; - if (uname(&uts)) + if (uname(&uts) == -1) die_errno(_("failed to get kernel name and information")); strbuf_addf(&sb, "Location %s, system %s %s %s", get_git_work_tree(), uts.sysname, uts.release, uts.version); -- 2.4.0.53.g8440f74 -- 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 v2] Fix definition of ARRAY_SIZE for non-gcc builds
On Wed, Jun 24, 2015 at 11:12:07PM +0100, Charles Bailey wrote: > From: Charles Bailey > > The improved ARRAY_SIZE macro uses BARF_UNLESS_AN_ARRAY which is expands > to a valid check for recent gcc versions and to 0 for older gcc > versions but is not defined on non-gcc builds. Actually hitting send is a sure fire way to get me spot other errors: s/which is expands/which expands/ -- 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 v2] Fix definition of ARRAY_SIZE for non-gcc builds
From: Charles Bailey The improved ARRAY_SIZE macro uses BARF_UNLESS_AN_ARRAY which is expands to a valid check for recent gcc versions and to 0 for older gcc versions but is not defined on non-gcc builds. Non-gcc builds need this macro to expand to 0 as well. The current outer test (defined(__GNUC__) && (__GNUC__ >= 3)) is a strictly weaker condition than the inner test (GIT_GNUC_PREREQ(3, 1)) so we can omit the outer test and cause the BARF_UNLESS_AN_ARRAY macro to be defined correctly on non-gcc builds as well as gcc builds with older versions. Signed-off-by: Charles Bailey --- This resend fixes a copy and paste error in the outer test in the commit message. The patch remains the same. This fixes a build regression introduced in v2.4.4 so this patch is based off maint. git-compat-util.h | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index b45c75f..8c2b7aa 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -58,15 +58,13 @@ #define BUILD_ASSERT_OR_ZERO(cond) \ (sizeof(char [1 - 2*!(cond)]) - 1) -#if defined(__GNUC__) && (__GNUC__ >= 3) -# if GIT_GNUC_PREREQ(3, 1) +#if GIT_GNUC_PREREQ(3, 1) /* &arr[0] degrades to a pointer: a different type from an array */ # define BARF_UNLESS_AN_ARRAY(arr) \ BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \ __typeof__(&(arr)[0]))) -# else -# define BARF_UNLESS_AN_ARRAY(arr) 0 -# endif +#else +# define BARF_UNLESS_AN_ARRAY(arr) 0 #endif /* * ARRAY_SIZE - get the number of elements in a visible array -- 2.4.0.53.g8440f74 -- 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] Fix definition of ARRAY_SIZE for non-gcc builds
From: Charles Bailey The improved ARRAY_SIZE macro uses BARF_UNLESS_AN_ARRAY which is expands to a valid check for recent gcc versions and to 0 for older gcc versions but is not defined on non-gcc builds. Non-gcc builds need this macro to expand to 0 as well. The current outer test (defined(__GNUC__) && (__GNUC__)) is a strictly weaker condition than the inner test (GIT_GNUC_PREREQ(3, 1)) so we can omit the outer test and cause the BARF_UNLESS_AN_ARRAY macro to be defined correctly on non-gcc builds as well as gcc builds with older versions. Signed-off-by: Charles Bailey --- This fixes a build regression introduced in v2.4.4 so this patch is based off that tag. git-compat-util.h | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index b45c75f..8c2b7aa 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -58,15 +58,13 @@ #define BUILD_ASSERT_OR_ZERO(cond) \ (sizeof(char [1 - 2*!(cond)]) - 1) -#if defined(__GNUC__) && (__GNUC__ >= 3) -# if GIT_GNUC_PREREQ(3, 1) +#if GIT_GNUC_PREREQ(3, 1) /* &arr[0] degrades to a pointer: a different type from an array */ # define BARF_UNLESS_AN_ARRAY(arr) \ BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \ __typeof__(&(arr)[0]))) -# else -# define BARF_UNLESS_AN_ARRAY(arr) 0 -# endif +#else +# define BARF_UNLESS_AN_ARRAY(arr) 0 #endif /* * ARRAY_SIZE - get the number of elements in a visible array -- 2.4.0.53.g8440f74 -- 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: Improvements to integer option parsing
> On 22 Jun 2015, at 23:09, Junio C Hamano wrote: > > Charles Bailey writes: >> >> - marginally improved the opterror message on failed parses > > I'd queue with "s/a integer/a non-negative integer/". Ha! That's what I had before I submitted, but then the source line got quite long (which could have been split) and the generated message got quite long as well so I cropped it. This was probably the source of the grammar mistake. If you're happy with the longer message, I am happy with it too. -- To unsubscribe from this list: send the line "unsubscribe git" in
Re: [PATCH 8/7] cat-file: sort and de-dup output of --batch-all-objects
On Mon, Jun 22, 2015 at 07:06:32AM -0400, Jeff King wrote: > On Mon, Jun 22, 2015 at 06:33:21AM -0400, Jeff King wrote: > > > By the way, in addition to not showing objects in order, > > list-all-objects (and my cat-file option) may show duplicates. Do we > > want to "sort -u" for the user? It might be nice for them to always get > > a de-duped and sorted list. Aside from the CPU cost of sorting, it does > > mean we'll allocate ~80MB for the kernel to store the sha1s. I guess > > that's not too much when you are talking about the kernel repo. I took > > the coward's way out and just mentioned the limitation in the > > documentation, but I'm happy to be persuaded. > > The patch below does the sort/de-dup. I'd probably just squash it into > patch 7, though. Woah, 8 out of 7! Did you get a chance to measure the performance hit of the sort? If not, I may test it out when I next get the chance. -- To unsubscribe from this list: send the line "unsubscribe git" in
Re: [PATCH] Add list-all-objects command
On Mon, Jun 22, 2015 at 06:33:21AM -0400, Jeff King wrote: > On Mon, Jun 22, 2015 at 04:38:22AM -0400, Jeff King wrote: > > > > + prepare_packed_git(); > > > + for (p = packed_git; p; p = p->next) { > > > + open_pack_index(p); > > > + } > > > > Yikes. The fact that you need to do this means that > > for_each_packed_object is buggy, IMHO. I'll send a patch. > > Here's that patch. And since I did not want to pile work on Charles, I > went ahead and just implemented the patches I suggested in the other > email. I have to say that I think that adding this functionality to cat-file makes a lot of sense. If it only catted files it might be a stretch but as it's already grown --batch-check functionality, it now seems a reasonable extension. I'm not particularly attached to having a standalone "list-all-objects" command per se. -- To unsubscribe from this list: send the line "unsubscribe git" in
Re: [Question] Is it normal for accented characters to be shown as decomposed Unicode on GNU/Linux?
On Mon, Jun 22, 2015 at 03:17:40PM +0200, Bastien Traverse wrote: > test case: > $ mkdir accent-test && cd !$ > $ git init > $ touch rêve réunion > $ git status > On branch master > > Initial commit > > Untracked files: > (use "git add ..." to include in what will be committed) > > "r\303\251union" > "r\303\252ve" Note that these aren't "decomposed" (in the unicode decomposition sense) but are merely octal escaped representations of the utf-8 encoded file names. My understanding that this is normal and probably dates back (at least for status as far as: commit a734d0b10bd0f5554abb3acdf11426040cfc4df0 Author: Dmitry Potapov Date: Fri Mar 7 05:30:58 2008 +0300 Make private quote_path() in wt-status.c available as quote_path_relative() [...] The behaviour can be changed by setting the git config variable "core.quotePath" to false. -- To unsubscribe from this list: send the line "unsubscribe git" in
[PATCH 3/3] contrib/subtree: Small tidy-up to test
From: Charles Bailey There's no need to switch branches to parse another branch's ancestry. Signed-off-by: Charles Bailey --- contrib/subtree/t/t7900-subtree.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh index 001c604..bd3df97 100755 --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -221,9 +221,7 @@ test_expect_success 'check hash of split' ' spl1=$(git subtree split --prefix subdir) && git subtree split --prefix subdir --branch splitbr1test && check_equal ''"$(git rev-parse splitbr1test)"'' "$spl1" && - git checkout splitbr1test && - new_hash=$(git rev-parse HEAD~2) && - git checkout mainline && + new_hash=$(git rev-parse splitbr1test~2) && check_equal ''"$new_hash"'' "$subdir_hash" ' -- 2.4.0.53.g8440f74 -- To unsubscribe from this list: send the line "unsubscribe git" in
[PATCH 1/3] contrib/subtree: Use tabs consitently for indentation in tests
From: Charles Bailey Although subtrees tests uses more spaces for indentation than tabs, there are still quite a lot of lines indented with tabs. As tabs conform with Git coding guidelines resolve the inconsistency in favour of tabs. Signed-off-by: Charles Bailey --- contrib/subtree/t/t7900-subtree.sh | 294 ++--- 1 file changed, 147 insertions(+), 147 deletions(-) diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh index 6309d12..2c5bfc1 100755 --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -62,17 +62,17 @@ last_commit_message() } test_expect_success 'init subproj' ' -test_create_repo subproj + test_create_repo subproj ' # To the subproject! cd subproj test_expect_success 'add sub1' ' -create sub1 && -git commit -m "sub1" && -git branch sub1 && -git branch -m master subproj + create sub1 && + git commit -m "sub1" && + git branch sub1 && + git branch -m master subproj ' # Save this hash for testing later. @@ -80,133 +80,133 @@ test_expect_success 'add sub1' ' subdir_hash=$(git rev-parse HEAD) test_expect_success 'add sub2' ' -create sub2 && -git commit -m "sub2" && -git branch sub2 + create sub2 && + git commit -m "sub2" && + git branch sub2 ' test_expect_success 'add sub3' ' -create sub3 && -git commit -m "sub3" && -git branch sub3 + create sub3 && + git commit -m "sub3" && + git branch sub3 ' # Back to mainline cd .. test_expect_success 'add main4' ' -create main4 && -git commit -m "main4" && -git branch -m master mainline && -git branch subdir + create main4 && + git commit -m "main4" && + git branch -m master mainline && + git branch subdir ' test_expect_success 'fetch subproj history' ' -git fetch ./subproj sub1 && -git branch sub1 FETCH_HEAD + git fetch ./subproj sub1 && + git branch sub1 FETCH_HEAD ' test_expect_success 'no subtree exists in main tree' ' -test_must_fail git subtree merge --prefix=subdir sub1 + test_must_fail git subtree merge --prefix=subdir sub1 ' test_expect_success 'no pull from non-existant subtree' ' -test_must_fail git subtree pull --prefix=subdir ./subproj sub1 + test_must_fail git subtree pull --prefix=subdir ./subproj sub1 ' test_expect_success 'check if --message works for add' ' -git subtree add --prefix=subdir --message="Added subproject" sub1 && -check_equal ''"$(last_commit_message)"'' "Added subproject" && -undo + git subtree add --prefix=subdir --message="Added subproject" sub1 && + check_equal ''"$(last_commit_message)"'' "Added subproject" && + undo ' test_expect_success 'check if --message works as -m and --prefix as -P' ' -git subtree add -P subdir -m "Added subproject using git subtree" sub1 && -check_equal ''"$(last_commit_message)"'' "Added subproject using git subtree" && -undo + git subtree add -P subdir -m "Added subproject using git subtree" sub1 && + check_equal ''"$(last_commit_message)"'' "Added subproject using git subtree" && + undo ' test_expect_success 'check if --message works with squash too' ' -git subtree add -P subdir -m "Added subproject with squash" --squash sub1 && -check_equal ''"$(last_commit_message)"'' "Added subproject with squash" && -undo + git subtree add -P subdir -m "Added subproject with squash" --squash sub1 && + check_equal ''"$(last_commit_message)"'' "Added subproject with squash" && + undo ' test_expect_success 'add subproj to mainline' ' -git subtree add --prefix=subdir/ FETCH_HEAD && -check_equal ''"$(last_commit_message)"'' "Add '"'subdir/'"' from commit '"&
[PATCH 2/3] contrib/subtree: Fix broken &&-chains and revealed test error
From: Charles Bailey This fixes two instances where a &&-chain was broken in the subtree tests and fixes a test error that was revealed because of this. Many tests in t7900-subtree.sh make a commit and then use 'undo' to reset the state for the next test. In the 'check hash of split' test, an 'undo' was being invoked after a 'subtree split' even though the particular invocation of 'subtree split' did not actually make a commit. The subsequent check_equal was failing, but this failure was masked by that broken &&-chain. Removing this undo causes the failing check_equal to succeed but breaks the a check_equal later on in the same test. It turns out that an earlier test ('check if --message for merge works with squash too') makes a commit but doesn't 'undo' to the state expected by the remaining tests. None of the intervening tests cared enough about the state of the test repo to fail and the spurious 'undo' in 'check hash of split' restored the expected state for any remaining test that might care. Adding the missing 'undo' to 'check if --message for merge works with squash too' and removing the spurious one from 'check hash of split' fixes all tests once the &&-chains are completed. Signed-off-by: Charles Bailey --- contrib/subtree/t/t7900-subtree.sh | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh index 2c5bfc1..001c604 100755 --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -177,7 +177,8 @@ test_expect_success 'check if --message for merge works with squash too' ' test_expect_success 'merge new subproj history into subdir' ' git subtree merge --prefix=subdir FETCH_HEAD && git branch pre-split && - check_equal ''"$(last_commit_message)"'' "Merge commit '"'"'"$(git rev-parse sub2)"'"'"' into mainline" + check_equal ''"$(last_commit_message)"'' "Merge commit '"'"'"$(git rev-parse sub2)"'"'"' into mainline" && + undo ' test_expect_success 'Check that prefix argument is required for split' ' @@ -218,9 +219,8 @@ test_expect_success 'check split with --branch' ' test_expect_success 'check hash of split' ' spl1=$(git subtree split --prefix subdir) && - undo && git subtree split --prefix subdir --branch splitbr1test && - check_equal ''"$(git rev-parse splitbr1test)"'' "$spl1" + check_equal ''"$(git rev-parse splitbr1test)"'' "$spl1" && git checkout splitbr1test && new_hash=$(git rev-parse HEAD~2) && git checkout mainline && @@ -269,7 +269,7 @@ test_expect_success 'add sub9' ' cd .. test_expect_success 'split for sub8' ' - split2=''"$(git subtree split --annotate='"'*'"' --prefix subdir/ --rejoin)"'' + split2=''"$(git subtree split --annotate='"'*'"' --prefix subdir/ --rejoin)"'' && git branch split2 "$split2" ' -- 2.4.0.53.g8440f74 -- To unsubscribe from this list: send the line "unsubscribe git" in
Re: [PATCH] Add list-all-objects command
On Mon, Jun 22, 2015 at 04:38:22AM -0400, Jeff King wrote: > On Sun, Jun 21, 2015 at 08:20:31PM +0100, Charles Bailey wrote: > > > + prepare_packed_git(); > > + for (p = packed_git; p; p = p->next) { > > + open_pack_index(p); > > + } > > Yikes. The fact that you need to do this means that > for_each_packed_object is buggy, IMHO. I'll send a patch. I'm glad you said that; the interface did seem a bit warty at the time but as I "fixed" this early in my hacking I didn't remeber to revisit this and ask if it was actually intentional. -- To unsubscribe from this list: send the line "unsubscribe git" in
Fast enumeration of objects
This is a re-casting of my previous filter-objects command but without any of the filtering so it is now just "list-all-objects". I have retained the "--verbose" option which outputs the same format as the default "cat-file --batch-check" as it provides a useful performance gain to filtering though "cat-file" if this basic information is all that is needed. The motivating use case is to enable a script to quickly scan a large number of repositories for any large objects. I performed some test timings of some different commands on a clone of the Linux kernel which was completely packed. $ time git rev-list --all --objects | cut -d" " -f1 | git cat-file --batch-check | awk '{if ($3 >= 512000) { print $1 }}' | wc -l 958 real0m30.823s user0m41.904s sys 0m7.728s list-all-objects gives a significant improvement: $ time git list-all-objects | git cat-file --batch-check | awk '{if ($3 >= 512000) { print $1 }}' | wc -l 958 real0m9.585s user0m10.820s sys 0m4.960s skipping the cat-filter filter is a lesser but still significant improvement: $ time git list-all-objects -v | awk '{if ($3 >= 512000) { print $1 }}' | wc -l 958 real0m5.637s user0m6.652s sys 0m0.156s The old filter-objects could do the size filter a little be faster, but not by much: $ time git filter-objects --min-size=500k | wc -l 958 real0m4.564s user0m4.496s sys 0m0.064s -- To unsubscribe from this list: send the line "unsubscribe git" in
[PATCH] Add list-all-objects command
From: Charles Bailey list-all-objects is a command to print the ids of all objects in the object database of a repository. It is designed as a low overhead interface for scripts that want to analyse all objects but don't require the ordering implied by a revision walk. It will list all objects, loose and packed, and will include unreachable objects. list-all-objects is faster that "rev-list --all --objects" but there is no guarantee as to the order in which objects will be listed. Signed-off-by: Charles Bailey --- Documentation/git-list-all-objects.txt | 29 +++ Makefile | 1 + builtin.h | 1 + builtin/list-all-objects.c | 64 ++ git.c | 1 + t/t8100-list-all-objects.sh| 45 6 files changed, 141 insertions(+) create mode 100644 Documentation/git-list-all-objects.txt create mode 100644 builtin/list-all-objects.c create mode 100755 t/t8100-list-all-objects.sh diff --git a/Documentation/git-list-all-objects.txt b/Documentation/git-list-all-objects.txt new file mode 100644 index 000..5f28d41 --- /dev/null +++ b/Documentation/git-list-all-objects.txt @@ -0,0 +1,29 @@ +git-list-all-objects(1) +=== + +NAME + +git-list-all-objects - List all objects in the repository. + +SYNOPSIS + +[verse] +'git list-all-objects' [-v|--verbose] + +DESCRIPTION +--- +List the ids of all objects in a repository, including any unreachable objects. +If `--verbose` is specified then the object's type and size is printed out as +well as its id. + +OPTIONS +--- + +-v:: +--verbose:: + Output in the followin format instead of just printing object ids: +SP SP + +GIT +--- +Part of the linkgit:git[1] suite diff --git a/Makefile b/Makefile index 149f1c7..cf4f0c3 100644 --- a/Makefile +++ b/Makefile @@ -853,6 +853,7 @@ BUILTIN_OBJS += builtin/help.o BUILTIN_OBJS += builtin/index-pack.o BUILTIN_OBJS += builtin/init-db.o BUILTIN_OBJS += builtin/interpret-trailers.o +BUILTIN_OBJS += builtin/list-all-objects.o BUILTIN_OBJS += builtin/log.o BUILTIN_OBJS += builtin/ls-files.o BUILTIN_OBJS += builtin/ls-remote.o diff --git a/builtin.h b/builtin.h index b87df70..112bafb 100644 --- a/builtin.h +++ b/builtin.h @@ -74,6 +74,7 @@ extern int cmd_help(int argc, const char **argv, const char *prefix); extern int cmd_index_pack(int argc, const char **argv, const char *prefix); extern int cmd_init_db(int argc, const char **argv, const char *prefix); extern int cmd_interpret_trailers(int argc, const char **argv, const char *prefix); +extern int cmd_list_all_objects(int argc, const char **argv, const char *prefix); extern int cmd_log(int argc, const char **argv, const char *prefix); extern int cmd_log_reflog(int argc, const char **argv, const char *prefix); extern int cmd_ls_files(int argc, const char **argv, const char *prefix); diff --git a/builtin/list-all-objects.c b/builtin/list-all-objects.c new file mode 100644 index 000..3b43b02 --- /dev/null +++ b/builtin/list-all-objects.c @@ -0,0 +1,64 @@ +#include "cache.h" +#include "builtin.h" +#include "revision.h" +#include "parse-options.h" + +#include + +static int verbose; + +static int print_object(const unsigned char *sha1) +{ + if (verbose) { + unsigned long size; + int type = sha1_object_info(sha1, &size); + + if (type < 0) + return -1; + + printf("%s %s %lu\n", sha1_to_hex(sha1), typename(type), size); + } + else + printf("%s\n", sha1_to_hex(sha1)); + + return 0; +} + +static int check_loose_object(const unsigned char *sha1, + const char *path, + void *data) +{ + return print_object(sha1); +} + +static int check_packed_object(const unsigned char *sha1, + struct packed_git *pack, + uint32_t pos, + void *data) +{ + return print_object(sha1); +} + +static struct option builtin_filter_objects_options[] = { + OPT__VERBOSE(&verbose, "show object type and size"), + OPT_END() +}; + +int cmd_list_all_objects(int argc, const char **argv, const char *prefix) +{ + struct packed_git *p; + + argc = parse_options(argc, argv, prefix, builtin_filter_objects_options, +NULL, 0); + + for_each_loose_object(check_loose_object, NULL, 0); + + prepare_packed_git(); + for (p = packed_git; p; p = p->next) { + open_pack_index(p); + } + + for_each_packed_object(check_packed_object, NULL, 0); + + return 0; +} diff --git a/git.c b/git.c index 44374b1..81e8ae4 100644 --- a/git
Re: [PATCH 2/2] Move unsigned long option parsing out of pack-objects.c
On Sun, Jun 21, 2015 at 07:25:44PM +0100, Charles Bailey wrote: > From: Charles Bailey > > diff --git a/parse-options.c b/parse-options.c > index 80106c0..101b649 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -180,6 +180,23 @@ static int get_value(struct parse_opt_ctx_t *p, > return opterror(opt, "expects a numerical value", > flags); > return 0; > > + case OPTION_MAGNITUDE: > + if (unset) { > + *(unsigned long *)opt->value = 0; > + return 0; > + } > + if (opt->flags & PARSE_OPT_OPTARG && !p->opt) { > + *(unsigned long *)opt->value = opt->defval; > + return 0; > + } > + if (get_arg(p, opt, flags, &arg)) > + return -1; > + if (!git_parse_ulong(arg, opt->value)) > + return opterror(opt, > + "expects a integer value with an optional k/m/g > suffix", > + flags); > + return 0; > + Spotted after sending: s/expects a integer/expects an integer/ -- To unsubscribe from this list: send the line "unsubscribe git" in
[PATCH 1/2] Correct test-parse-options to handle negative ints
From: Charles Bailey Fix the printf specification to treat 'integer' as the signed type that it is and add a test that checks that we parse negative option arguments. Signed-off-by: Charles Bailey --- t/t0040-parse-options.sh | 2 ++ test-parse-options.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index b044785..372d521 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -132,6 +132,8 @@ test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt' +test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345' + cat > expect << EOF boolean: 2 integer: 1729 diff --git a/test-parse-options.c b/test-parse-options.c index 5dabce6..7c492cf 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -82,7 +82,7 @@ int main(int argc, char **argv) argc = parse_options(argc, (const char **)argv, prefix, options, usage, 0); printf("boolean: %d\n", boolean); - printf("integer: %u\n", integer); + printf("integer: %d\n", integer); printf("timestamp: %lu\n", timestamp); printf("string: %s\n", string ? string : "(not set)"); printf("abbrev: %d\n", abbrev); -- 2.4.0.53.g8440f74 -- To unsubscribe from this list: send the line "unsubscribe git" in
[PATCH 2/2] Move unsigned long option parsing out of pack-objects.c
From: Charles Bailey The unsigned long option parsing (including 'k'/'m'/'g' suffix parsing) is more widely applicable. Add support for OPT_MAGNITUDE to parse-options.h and change pack-objects.c use this support. The error behavior on parse errors follows that of OPT_INTEGER. The name of the option that failed to parse is reported with a brief message describing the expect format for the option argument and then the full usage message for the command invoked. This is differs from the previous behavior for OPT_ULONG used in pack-objects for --max-pack-size and --window-memory which used to display the value supplied in the error message and did not display the full usage message. Signed-off-by: Charles Bailey --- Documentation/technical/api-parse-options.txt | 6 builtin/pack-objects.c| 25 +++ parse-options.c | 17 ++ parse-options.h | 3 ++ t/t0040-parse-options.sh | 45 --- test-parse-options.c | 3 ++ 6 files changed, 73 insertions(+), 26 deletions(-) diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index 1f2db31..525cb2f 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -168,6 +168,12 @@ There are some macros to easily define options: Introduce an option with integer argument. The integer is put into `int_var`. +`OPT_MAGNITUDE(short, long, &unsigned_long_var, description)`:: + Introduce an option with a size argument. The argument must be a + non-negative integer and may include a suffix of 'k', 'm' or 'g' to + scale the provided value by 1024, 1024^2 or 1024^3 respectively. + The scaled value is put into `unsigned_long_var`. + `OPT_DATE(short, long, &int_var, description)`:: Introduce an option with date argument, see `approxidate()`. The timestamp is put into `int_var`. diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 80fe8c7..62cc16d 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2588,23 +2588,6 @@ static int option_parse_unpack_unreachable(const struct option *opt, return 0; } -static int option_parse_ulong(const struct option *opt, - const char *arg, int unset) -{ - if (unset) - die(_("option %s does not accept negative form"), - opt->long_name); - - if (!git_parse_ulong(arg, opt->value)) - die(_("unable to parse value '%s' for option %s"), - arg, opt->long_name); - return 0; -} - -#define OPT_ULONG(s, l, v, h) \ - { OPTION_CALLBACK, (s), (l), (v), "n", (h), \ - PARSE_OPT_NONEG, option_parse_ulong } - int cmd_pack_objects(int argc, const char **argv, const char *prefix) { int use_internal_rev_list = 0; @@ -2627,16 +2610,16 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) { OPTION_CALLBACK, 0, "index-version", NULL, N_("version[,offset]"), N_("write the pack index file in the specified idx format version"), 0, option_parse_index_version }, - OPT_ULONG(0, "max-pack-size", &pack_size_limit, - N_("maximum size of each output pack file")), + OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit, + N_("maximum size of each output pack file")), OPT_BOOL(0, "local", &local, N_("ignore borrowed objects from alternate object store")), OPT_BOOL(0, "incremental", &incremental, N_("ignore packed objects")), OPT_INTEGER(0, "window", &window, N_("limit pack window by objects")), - OPT_ULONG(0, "window-memory", &window_memory_limit, - N_("limit pack window by memory in addition to object limit")), + OPT_MAGNITUDE(0, "window-memory", &window_memory_limit, + N_("limit pack window by memory in addition to object limit")), OPT_INTEGER(0, "depth", &depth, N_("maximum length of delta chain allowed in the resulting pack")), OPT_BOOL(0, "reuse-delta", &reuse_delta, diff --git a/parse-options.c b/parse-options.c index 80106c0..101b649 100644 --- a/parse-options.c +++ b/parse-options.c @@ -180,6 +180,23 @@
Improvements to integer option parsing
This is a re-roll of the first two patches in my previous series which used to include "filter-objects" which is now a separate topic. [PATCH 1/2] Correct test-parse-options to handle negative ints The first one has changed only in that I've moved the additional test to a more logical place in the test file. [PATCH 2/2] Move unsigned long option parsing out of pack-objects.c I've made the following changes to the second commit: - renamed this OPT_MAGNITUDE to try and convey something that is both unsigned and might benefit from a 'scale' suffix. I'm expecting more discussion on the name! - fixed the enum ordering to put this close to OPT_INTEGER - added documentation to api-parse-options.txt - marginally improved the opterror message on failed parses - noted the change in behavior for the error messages generated for pack-objects' --max-pack-size and --window-memory in the commit message -- To unsubscribe from this list: send the line "unsubscribe git" in
Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c
On Fri, Jun 19, 2015 at 10:58:51AM -0700, Junio C Hamano wrote: > Charles Bailey writes: > > Please place it immediately after INTEGER, as they are conceptually > siblings---group similar things together. Sorry, this is a bad habit from working on projects where changing the value of existing enum identifiers cause bad things. > This used to be: > > > - die(_("unable to parse value '%s' for option %s"), > > - arg, opt->long_name); > > but opterror() talks about which option, so there is no information > loss by losing "for option %s" from here. That means there is only > one difference for pack-objects: > > $ git pack-objects --max-pack-size=1T > fatal: unable to parse value '1T' for option max-pack-size > $ ./git pack-objects --max-pack-size=1T > error: option `max-pack-size' expects a numerical value > usage: git pack-objects --stdout [options... > ... 30 more lines omitted ... > > Eh, make that two: > > * We no longer say what value we did not like. The user presumably >knows what he typed, so this is only a minor loss. > > * We used to stop without giving "usage", as the error message was >specific enough. We now spew descriptions on other options >unrelated to the specific error the user may want to concentrate >on. Perhaps this is a minor regression. > > I wonder if "expects a numerical value" is the best way to say this. I was aware that I was changing the error reporting for max-pack-size and window-memory but thought that by going with the existing behaviour of OPT_INTEGER I'd be going with a more established pattern. These observations also seem to apply to OPT_INTEGER handling. Would this be something that we'd want to fix too? Currently git package-objects --depth=5.5 prints: error: option `depth' expects a numerical value usage: git pack-objects --stdout [options... [... many more lines omitted ...] Obviously, changing this to skip the full usage report would affect many existing commands. Also, I preserved the PARSE_OPT_NONEG flag for OPT_ULONG but would this ever not make sense for an OPT_INTEGER option? -- To unsubscribe from this list: send the line "unsubscribe git" in
Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c
On Fri, Jun 19, 2015 at 01:03:25PM +0200, Remi Galan Alfonso wrote: > > It's trivial matter but the line: > > + > output 2> output.err && > should be written: > > + >output 2>output.err && > > It was incorrectly written before but since > you are modifying the line, it might be a > good thing to change it now. Yes, I can fold this in. I just changed the wrapping and didn't spot this style error. -- To unsubscribe from this list: send the line "unsubscribe git" in
Re: [PATCH 3/3] Add filter-objects command
On Fri, Jun 19, 2015 at 11:52:28AM +0100, John Keeping wrote: > On Fri, Jun 19, 2015 at 11:33:24AM +0100, Charles Bailey wrote: > > So, yes, performance is definitely an issue and I could have called this > > command "git magically-generate-all-object-for-scripts" but then, as it > > was so easy to provide exactly the filtering that I was looking for in > > the C code, I thought I would do that as well and then "filter-objects" > > ("filter-all-objects"?) seemed like a better name. > > By analogy with "git filter-branch", I don't think "filter-objects" is a > good name here. My preference would be "ls-objects". I like that because it emphasises why I wrote it, the very basic filtering is a nice additional feature. -- To unsubscribe from this list: send the line "unsubscribe git" in
Re: [PATCH 3/3] Add filter-objects command
On Fri, Jun 19, 2015 at 06:10:10AM -0400, Jeff King wrote: > On Fri, Jun 19, 2015 at 10:10:59AM +0100, Charles Bailey wrote: > > > filter-objects is a command to scan all objects in the object database > > for the repository and print the ids of those which match the given > > criteria. > > > > The current supported criteria are object type and the minimum size of > > the object. > > > > The guiding use case is to scan repositories quickly for large objects > > which may cause performance issues for users. The list of objects can > > then be used to guide some future remediating action. > > I've had to perform this exact same task. You can already do the > "filtering" part pretty easily and efficiently with cat-file and a perl > script, like: > > magically_generate_all_objects | > git cat-file --batch-check='%(objectsize) %(objectname)' | > perl -alne 'print $F[1] if $F[0] > 1234' > > That's not as friendly as your filter-objects, but it's a lot more > flexible (since you can ask cat-file for all sorts of information). > > Obviously I've glossed over the "how to get a list of objects" part. > If you truly want all objects (not just reachable ones), or if "rev-list > --objects" is too slow [...] So, yes, performance is definitely an issue and I could have called this command "git magically-generate-all-object-for-scripts" but then, as it was so easy to provide exactly the filtering that I was looking for in the C code, I thought I would do that as well and then "filter-objects" ("filter-all-objects"?) seemed like a better name. It's about an order of magnitude faster on the systems I've checked to do a parameterless filter-objects then rev-list --all --objects, although I understand they do different things. I am also thinking about another piece that answers the question: "which commits introduce any of (or the first of) this list of objects?". This can be done by parseing a diff --raw for commits but I think it should be possible to do this faster, too. Charles. -- 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/3] Correct test-parse-options to handle negative ints
From: Charles Bailey Fix the printf specification to treat 'integer' as the signed type that it is and add a test that checks that we parse negative option arguments. Signed-off-by: Charles Bailey --- t/t0040-parse-options.sh | 2 ++ test-parse-options.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index b044785..ecb7417 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -151,6 +151,8 @@ test_expect_success 'short options' ' test_must_be_empty output.err ' +test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345' + cat > expect << EOF boolean: 2 integer: 1729 diff --git a/test-parse-options.c b/test-parse-options.c index 5dabce6..7c492cf 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -82,7 +82,7 @@ int main(int argc, char **argv) argc = parse_options(argc, (const char **)argv, prefix, options, usage, 0); printf("boolean: %d\n", boolean); - printf("integer: %u\n", integer); + printf("integer: %d\n", integer); printf("timestamp: %lu\n", timestamp); printf("string: %s\n", string ? string : "(not set)"); printf("abbrev: %d\n", abbrev); -- 2.4.0.53.g8440f74 -- 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
Improvements to parse-options and a new filter-objects command
In my team we've been looking for a fast way to check a large number of repositories for large files, which are typically unintentionally checked in binaries, so that we can warn repository owners and help them tidy up as desired. There seem to be two main approaches to scripting this. The first is to do something revision-walk based such as `log --numstat` and the second is to scan pack files using `verify-pack -v` and either to ensure that everything is packed or scan loose objects separately. The revision walking tends to be slow and parsing verify-pack -v is awkward not only because of the need to take account of multiple packs and loose objects, but also because it is porcelainish. For example, at some point it gained a delta chain summary which needs to be snipped before the list of packed objects can be sorted and used. The third patch in this series adds a new built in which makes this simple and fast. While implementing it, I found a couple of other improvements which I think stand alone. [PATCH 1/3] Correct test-parse-options to handle negative ints I noticed that a printf in test-parse-options was using %u instead of %d for an int with the consequence that it wouldn't ever print a negative value correctly. I don't know that we do ever parse a negative integer as an option, but there's no reason that it shouldn't work so I fixed it and added a trivial test. [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c I wanted to be able to parse options like --min-size=500k in my new command so I started to add OPT_ULONG, only to realise that it already existed but was private to pack-objects. I added OPT_ULONG support to parse-options based on the existing OPT_INTEGER code, added new tests and changed pack-objects to use this instead. -- 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/3] Move unsigned long option parsing out of pack-objects.c
From: Charles Bailey The unsigned long option parsing (including 'k'/'m'/'g' suffix parsing) is more widely applicable. Add support for OPT_ULONG to parse-options.h and change pack-objects.c use this support. Signed-off-by: Charles Bailey --- builtin/pack-objects.c | 17 - parse-options.c | 15 +++ parse-options.h | 5 - t/t0040-parse-options.sh | 46 ++ test-parse-options.c | 3 +++ 5 files changed, 64 insertions(+), 22 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 80fe8c7..5de76db 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2588,23 +2588,6 @@ static int option_parse_unpack_unreachable(const struct option *opt, return 0; } -static int option_parse_ulong(const struct option *opt, - const char *arg, int unset) -{ - if (unset) - die(_("option %s does not accept negative form"), - opt->long_name); - - if (!git_parse_ulong(arg, opt->value)) - die(_("unable to parse value '%s' for option %s"), - arg, opt->long_name); - return 0; -} - -#define OPT_ULONG(s, l, v, h) \ - { OPTION_CALLBACK, (s), (l), (v), "n", (h), \ - PARSE_OPT_NONEG, option_parse_ulong } - int cmd_pack_objects(int argc, const char **argv, const char *prefix) { int use_internal_rev_list = 0; diff --git a/parse-options.c b/parse-options.c index 80106c0..76a5c3e 100644 --- a/parse-options.c +++ b/parse-options.c @@ -180,6 +180,21 @@ static int get_value(struct parse_opt_ctx_t *p, return opterror(opt, "expects a numerical value", flags); return 0; + case OPTION_ULONG: + if (unset) { + *(unsigned long *)opt->value = 0; + return 0; + } + if (opt->flags & PARSE_OPT_OPTARG && !p->opt) { + *(unsigned long *)opt->value = opt->defval; + return 0; + } + if (get_arg(p, opt, flags, &arg)) + return -1; + if (!git_parse_ulong(arg, opt->value)) + return opterror(opt, "expects a numerical value", flags); + return 0; + default: die("should not happen, someone must be hit on the forehead"); } diff --git a/parse-options.h b/parse-options.h index c71e9da..2ddb26f 100644 --- a/parse-options.h +++ b/parse-options.h @@ -18,7 +18,8 @@ enum parse_opt_type { OPTION_INTEGER, OPTION_CALLBACK, OPTION_LOWLEVEL_CALLBACK, - OPTION_FILENAME + OPTION_FILENAME, + OPTION_ULONG }; enum parse_opt_flags { @@ -129,6 +130,8 @@ struct option { #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) } #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) } +#define OPT_ULONG(s, l, v, h) { OPTION_ULONG, (s), (l), (v), N_("n"), \ + (h), PARSE_OPT_NONEG } #define OPT_STRING(s, l, v, a, h) { OPTION_STRING, (s), (l), (v), (a), (h) } #define OPT_STRING_LIST(s, l, v, a, h) \ { OPTION_CALLBACK, (s), (l), (v), (a), \ diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index ecb7417..55b3dba 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -19,6 +19,8 @@ usage: test-parse-options -i, --integer get a integer -j get a integer, too +-u, --unsigned-long + get an unsigned long --set23 set integer to 23 -t get timestamp of -L, --length get length of @@ -58,6 +60,7 @@ mv expect expect.err cat >expect.template < expect << EOF boolean: 2 integer: 1729 +unsigned long: 16384 timestamp: 0 string: 123 abbrev: 7 @@ -145,7 +171,7 @@ file: prefix/my.file EOF test_expect_success 'short options' ' - test-parse-options -s123 -b -i 1729 -b -vv -n -F my.file \ + test-parse-options -s123 -b -i 1729 -u 16k -b -vv -n -F my.file \ > output 2> output.err && test_cmp expect output && test_must_be_empty output.err @@ -156,6 +182,7 @@ test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345' cat > expect << EOF boolean: 2 integer: 1729 +unsigned long: 16384 timestamp: 0 string: 321 abbrev: 10 @@ -166,9 +193,10 @@ file: prefix/fi.le EOF test_expect_success 'long options' ' -
[PATCH 3/3] Add filter-objects command
From: Charles Bailey filter-objects is a command to scan all objects in the object database for the repository and print the ids of those which match the given criteria. The current supported criteria are object type and the minimum size of the object. The guiding use case is to scan repositories quickly for large objects which may cause performance issues for users. The list of objects can then be used to guide some future remediating action. Signed-off-by: Charles Bailey --- Documentation/git-filter-objects.txt | 38 +++ Makefile | 1 + builtin.h| 1 + builtin/filter-objects.c | 73 git.c| 1 + t/t8100-filter-objects.sh| 67 + 6 files changed, 181 insertions(+) create mode 100644 Documentation/git-filter-objects.txt create mode 100644 builtin/filter-objects.c create mode 100755 t/t8100-filter-objects.sh diff --git a/Documentation/git-filter-objects.txt b/Documentation/git-filter-objects.txt new file mode 100644 index 000..c10ca01 --- /dev/null +++ b/Documentation/git-filter-objects.txt @@ -0,0 +1,38 @@ +git-filter-objects(1) += + +NAME + +git-filter-objects - Scan through all objects in the repository and print those +matching a given filter + + +SYNOPSIS + +[verse] +'git filter-objects' [-t | --type=] [--min-size=] + [-v|--verbose] + +DESCRIPTION +--- +Scans all objects in a repository - including any unreachable objects - and +print out the ids of all matching objects. If `--verbose` is specified then +the object type and size is printed out as well as its id. + +OPTIONS +--- +-t:: +--type:: + Only list objects whose type matches . + +--min-size:: + Only list objects whose size exceeds bytes. + +-v:: +--verbose:: + Output in the followin format instead of just printing object ids: +SP SP + +GIT +--- +Part of the linkgit:git[1] suite diff --git a/Makefile b/Makefile index 149f1c7..a7c017f 100644 --- a/Makefile +++ b/Makefile @@ -842,6 +842,7 @@ BUILTIN_OBJS += builtin/diff.o BUILTIN_OBJS += builtin/fast-export.o BUILTIN_OBJS += builtin/fetch-pack.o BUILTIN_OBJS += builtin/fetch.o +BUILTIN_OBJS += builtin/filter-objects.o BUILTIN_OBJS += builtin/fmt-merge-msg.o BUILTIN_OBJS += builtin/for-each-ref.o BUILTIN_OBJS += builtin/fsck.o diff --git a/builtin.h b/builtin.h index b87df70..5a15693 100644 --- a/builtin.h +++ b/builtin.h @@ -62,6 +62,7 @@ extern int cmd_diff_tree(int argc, const char **argv, const char *prefix); extern int cmd_fast_export(int argc, const char **argv, const char *prefix); extern int cmd_fetch(int argc, const char **argv, const char *prefix); extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix); +extern int cmd_filter_objects(int argc, const char **argv, const char *prefix); extern int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix); extern int cmd_for_each_ref(int argc, const char **argv, const char *prefix); extern int cmd_format_patch(int argc, const char **argv, const char *prefix); diff --git a/builtin/filter-objects.c b/builtin/filter-objects.c new file mode 100644 index 000..c40d621 --- /dev/null +++ b/builtin/filter-objects.c @@ -0,0 +1,73 @@ +#include "cache.h" +#include "builtin.h" +#include "revision.h" +#include "parse-options.h" + +#include + +static int req_type; +static unsigned long min_size; +static int verbose; + +static int check_object(const unsigned char *sha1) +{ + unsigned long size; + int type = sha1_object_info(sha1, &size); + + if (type < 0) + return -1; + + if (size >= min_size && (!req_type || type == req_type)) { + if (verbose) + printf("%s %s %lu\n", sha1_to_hex(sha1), typename(type), size); + else + printf("%s\n", sha1_to_hex(sha1)); + } + + return 0; +} + +static int check_loose_object(const unsigned char *sha1, + const char *path, + void *data) +{ + return check_object(sha1); +} + +static int check_packed_object(const unsigned char *sha1, + struct packed_git *pack, + uint32_t pos, + void *data) +{ + return check_object(sha1); +} + +static char *opt_type; +static struct option builtin_filter_objects_options[] = { + OPT_ULONG(0, "min-size", &min_size, "minimum size of object to show"), + OPT_STRING('t', "type", &opt_type, NULL, "type of objects to show"), + OPT__VERBOSE(&verbose, "show object type and size"), + OPT_END() +
Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option
On Mon, Apr 20, 2015 at 09:22:47PM +0530, karthik nayak wrote: > > > On 04/20/2015 02:49 PM, Charles Bailey wrote: > >As far as I could tell - and please correct me if I've misunderstood, > >cat-file's literally is about dealing with unrecognized types whereas > >hash-object's --literally is about both creating objects with bad types > >and invalid objects of "recognized" types. This latter scenario is where > >the option name "literally" makes the most sense. > Yes. What you're saying is correct, but it also makes sense as we're asking > "cat-file" to give us information about the object irrespective of the type > of the > object, hence asking it to literally print the information. Also it stays as > a compliment > to "hash-object --literally", which is already existing. OK, I think you've hit the main point which I was trying to make. To me, "literally" means "without transformation" or "exactly as written/recorded/transmitted" (which -t/-s do anyway) and doesn't really encompass the "irrespective of type" meaning that it has been given here. In any case, I've made my point so I won't labour it any further. I think that --no-validation or --allow-any-type might be more accurate but if everyone else is happy enough with --literally then I'm happy to live with that too. -- 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 v8 2/4] cat-file: teach cat-file a '--literally' option
On Mon, Apr 20, 2015 at 02:27:44PM +0530, Karthik Nayak wrote: > Sorry, but I didn't get you, broken objects created using hash-object > --literally do not work with cat-file without the --literally option. Perhaps an example would help: I cannot create a bad tree without --literally: $ echo total garbage | ./git hash-object -t tree --stdin -w fatal: corrupt tree file $ echo total garbage | ./git hash-object -t tree --stdin -w --literally fa2905d47028d00baec739f6d73540bb2a75c6f7 but I can use cat-file without --literally to query the contents and information about the object as it stands. $ ./git cat-file tree fa2905d47028d00baec739f6d73540bb2a75c6f7 total garbage $ ./git cat-file -t fa2905d47028d00baec739f6d73540bb2a75c6f7 tree $ ./git cat-file -s fa2905d47028d00baec739f6d73540bb2a75c6f7 14 As far as I could tell - and please correct me if I've misunderstood, cat-file's literally is about dealing with unrecognized types whereas hash-object's --literally is about both creating objects with bad types and invalid objects of "recognized" types. This latter scenario is where the option name "literally" makes the most sense. Charles. -- 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 v8 2/4] cat-file: teach cat-file a '--literally' option
> On 20 Apr 2015, at 06:30, Junio C Hamano wrote: > Charles Bailey writes: > >> The option isn't a true opposite of hash-object's --literally because >> that also allows the creation of known types with invalid contents (e.g. >> corrupt trees) whereas cat-file is quite happy to show the _contents_ of >> such corrupt objects even without --literally. > > Not really. If you create an object with corrupt type string (e.g. "BLOB" > instead of "blob"), cat-file would not be happy. Sorry, the emphasis should have been on "complete" of "complete opposite". There are some types of bad objects that can be created only with hash-object --literally (malformed tag or tree), for which cat-file works with fine and there are other types (pun unintended - BLOB, wobble, etc.) for which --literally/--unchecked is required with cat-file. So I meant that cat-file's --literally is only a partial "opposite" or analogue of hash-object's. --allow-invalid-types? --force (in the sense of "suppress some possible errors")? It's not a big thing but I'm aware that if we can find a better name then now would be the best moment. If not, then --literally it is. -- 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 v8 2/4] cat-file: teach cat-file a '--literally' option
On Wed, Apr 15, 2015 at 10:29:34PM +0530, Karthik Nayak wrote: > Currently 'git cat-file' throws an error while trying to > print the type or size of a broken/corrupt object. This is > because these objects are usually of unknown types. > > Teach git cat-file a '--literally' option where it prints > the type or size of a broken/corrupt object without throwing > an error. I'm sorry to come in with such a fundamental question at such a late revision of this patch series, but am I the only person to wonder about the choice of option name? To me, cat-file already output objects "literally" (without -p) as opposed to show. From the description, it feels more like it should be "--unchecked" or perhaps something better that I haven't thought of? The option isn't a true opposite of hash-object's --literally because that also allows the creation of known types with invalid contents (e.g. corrupt trees) whereas cat-file is quite happy to show the _contents_ of such corrupt objects even without --literally. Charles. -- 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] dumb-http: do not pass NULL path to parse_pack_index
On Tue, Jan 27, 2015 at 03:02:27PM -0500, Jeff King wrote: > Discovery and tests by Charles Bailey . > > Signed-off-by: Jeff King > --- > I'm happy to flip the authorship on this. You have more lines in it than > I do. :) No, I'm happy with you taking the blame/praise for this, it's definitely your fix. -- 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] Add failing test for fetching from multiple packs over dumb httpd
On Tue, Jan 27, 2015 at 01:12:21PM -0500, Jeff King wrote: > On Tue, Jan 27, 2015 at 03:20:41PM +0000, Charles Bailey wrote: > > > From: Charles Bailey > > > > When objects are spread across multiple packs, if an initial fetch does > > require all pack files, a subsequent fetch for objects in packs not > > retrieved in the initial fetch will fail. > > s/does/does not/, I think? Yes, that's definitely what I meant to write. [...] > It looks like the culprit is 7b64469 (Allow parse_pack_index on > temporary files, 2010-04-19). It added a new "idx_path" parameter to > parse_pack_index, which we pass as NULL. That causes its call to > check_packed_git_idx to fail (because it has no idea what file we are > talking about!). That change looks like it went into 1.7.1.1. I cannot confirm this working before then but we've definitely seen the bug in 1.7.12.3 and more recent versions. > This seems to fix it: > > diff --git a/sha1_file.c b/sha1_file.c > index 30995e6..eda4d90 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -1149,6 +1149,9 @@ struct packed_git *parse_pack_index(unsigned char > *sha1, const char *idx_path) > const char *path = sha1_pack_name(sha1); > struct packed_git *p = alloc_packed_git(strlen(path) + 1); > > + if (!idx_path) > + idx_path = sha1_pack_index_name(sha1); > + > strcpy(p->pack_name, path); > hashcpy(p->sha1, sha1); > if (check_packed_git_idx(idx_path, p)) { It certainly fixes my test script and I can give this patch a test in the 'real' world. -- 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] Add failing test for fetching from multiple packs over dumb httpd
From: Charles Bailey When objects are spread across multiple packs, if an initial fetch does require all pack files, a subsequent fetch for objects in packs not retrieved in the initial fetch will fail. --- I'm not very familiar with the http client code so this analysis is based purely on observed behaviour. When fetching only some refs from a repository served over dumb httpd Git appears to download all of the index files for the available packs but then only chooses the pack files that help it resolve the objects which we need. If we then later try to fetch an object which is in a pack file for which Git has previously downloaded an index file, it seems to trip because it believes it already has the object locally due to the presence of the index file but doesn't actually have it because it never retrieved the corresponding pack file. It reports an error of the form "Cannot obtain needed object ...". Manually deleting index files which have no corresponding local pack file will allow a repeat of the failed fetch to succeed. t/t5550-http-fetch-dumb.sh | 18 ++ 1 file changed, 18 insertions(+) diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index ac71418..cf2362a 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -165,6 +165,24 @@ test_expect_success 'fetch notices corrupt idx' ' ) ' +test_expect_failure 'fetch packed branches' ' + git checkout --orphan branch1 && + echo base >file && + git add file && + git commit -m base && + git --bare init "$HTTPD_DOCUMENT_ROOT_PATH"/repo_packed_branches.git && + git push "$HTTPD_DOCUMENT_ROOT_PATH"/repo_packed_branches.git branch1 && + git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH"/repo_packed_branches.git repack -d && + git checkout -b branch2 branch1 && + echo b2 >>file && + git commit -a -m b2 && + git push "$HTTPD_DOCUMENT_ROOT_PATH"/repo_packed_branches.git branch2 && + git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH"/repo_packed_branches.git repack -d && + git --bare init clone_packed_branches.git && + git --git-dir=clone_packed_branches.git fetch "$HTTPD_URL"/dumb/repo_packed_branches.git branch1:branch1 && + git --git-dir=clone_packed_branches.git fetch "$HTTPD_URL"/dumb/repo_packed_branches.git branch2:branch2 +' + test_expect_success 'did not use upload-pack service' ' grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act : >exp -- 2.0.2.611.g8c85416 -- 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: git ls-files -o seems to ignore .gitignore
On Mon, Oct 27, 2014 at 07:16:49AM +0100, Richard PALO wrote: > Hash: SHA1 > > I'm having an issue in that 'git ls-files -o' seems to ignore > [parts of] .gitignore whereas other commands, such as 'git status' > seem fine. This is, as far as I am aware, by design. If you want to apply the standard ignore rules to the output of ls-files -o then you can use the --exclude-standard option. -- 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 v2 1/2] mergetool: don't require a work tree for --tool-help
> On 11 Oct 2014, at 09:29, David Aguilar wrote: > > Thanks for the heads-up. > > I tested mergetool and it seems fine but indeed there's an > `if test -e "$GIT_DIR/MERGE_RR"` in there that is surely not > working as intended. > > One solution would be to move the work done in the test -z "$NONGIT_OK" > block in git-sh-setup into a function e.g. git_dir_init () so > that we can defer the GIT_DIR initialization until after > require_work_tree has been called. I believe I had a very similar idea but the vast number of things that would potentially be affected by changing git-sh-setup.sh made me put things on hold in case I had any other ideas. I haven't so I think this is probably the best approach.-- 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 v2 1/2] mergetool: don't require a work tree for --tool-help
On 10 Oct 2014, at 09:51, David Aguilar wrote: > Changes since v1: > > NONGIT_OK=Yes was added to make it actually work outside of a git repo. Does this actually work? The reason that I haven't got around to resending my re-roll is that I found that I needed changes to git-sh-setup.sh because doing NONGIT_OK and then require_work_tree didn't correctly set GIT_DIR when it wasn't already explicitly set in the environment. (I believe the rest of mergetool relies on it.) Perhaps I misunderstood, though.-- 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] mergetool: use more conservative temporary filenames
While you have the lid of this section of code, should we consider (optionally?) using a tmpdir to alleviate the eclipse issue where it wants temporary merge files to be the canonical locations for definitions of things that it finds when scanning source files in the project tree? [Apologies for this email client's long lines.]-- 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: What's cooking in git.git (Aug 2014, #02; Fri, 8)
On Fri, Aug 08, 2014 at 03:18:11PM -0700, Junio C Hamano wrote: > * cb/mergetool-difftool (2014-07-21) 2 commits > - difftool: don't assume that default sh is sane > - mergetool: don't require a work tree for --tool-help > > Update the way the "difftool --help" shows the help message that is > shared with the "mergetool" to reduce one shell dependency. > > Will merge to 'next'. Can you hold off on merging this? I think I want to have another go at making this neater. Specifically, --tool-help doesn't need a working tree but I hadn't spotted that it still requires a GIT_DIR / --git-dir when it really shouldn't. -- 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: Apple violating git LGPL?
On Wed, Aug 06, 2014 at 02:10:08PM -0400, Robert P Fischer wrote: > > 3. The version of git I ran is clearly NOT a plain vanilla "official" > git, it is a derivative work. Has Apple provided the source code of > the special version that I just ran? If not, that would seem to be a > violation of the LGPL. I found the source code of Apple's Git builds here: http://www.opensource.apple.com/source/Git/ Mine happens to be Git-48. I'm not sure how to tell what version you have if it is prompting you to accept a licence first. Charles. -- 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] difftool: don't assume that default sh is sane
On Sat, Jul 19, 2014 at 06:21:32PM +0100, John Keeping wrote: > > What's the reason for forcing `--tool-help` to be the last option? > Wouldn't it be simpler to just change the top-level case statement to: > > --tool-help=*) > TOOL_MODE=${1#--tool-help=} > show_tool_help > ;; > --tool-help) > show_tool_help > ;; It doesn't make sense to use --tool-help with other parameters so issuing an error made sense to me at the time. You've pointed out to me that I don't error when those other options come first so I'm now unsure how valuable this behaviour is, now. (I can't immediately see a really neat way to give a diagnostic if other options do come first.) Your version is good, obviously simpler. -- 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/2] difftool: don't assume that default sh is sane
From: Charles Bailey git-difftool used to create a command list script containing $( ... ) and explicitly call "sh -c" with this list. Instead, allow mergetool --tool-help to take a mode parameter and call mergetool directly to invoke the show_tool_help function. This mode parameter is intented for use solely by difftool. Signed-off-by: Charles Bailey --- Another issue for Solaris. Originally I had a fix for this that substituted "@SHELL_PATH@" even inside perl scripts but I felt that having an interface for show_tool_help was a little neater all round but I welcome alternative views. git-difftool.perl | 6 +- git-mergetool.sh | 12 +++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/git-difftool.perl b/git-difftool.perl index 18ca61e..598fcc2 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -47,13 +47,9 @@ sub find_worktree sub print_tool_help { - my $cmd = 'TOOL_MODE=diff'; - $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"'; - $cmd .= ' && show_tool_help'; - # See the comment at the bottom of file_diff() for the reason behind # using system() followed by exit() instead of exec(). - my $rc = system('sh', '-c', $cmd); + my $rc = system(qw(git mergetool --tool-help=diff)); exit($rc | ($rc >> 8)); } diff --git a/git-mergetool.sh b/git-mergetool.sh index e969dd0..d32b663 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -320,7 +320,17 @@ guessed_merge_tool=false while test $# != 0 do case "$1" in - --tool-help) + --tool-help*) + case "$#,$1" in + 1,*=*) + TOOL_MODE=$(expr "z$1" : 'z-[^=]*=\(.*\)') + ;; + 1,--tool-help) + ;; + *) + usage + ;; + esac show_tool_help ;; -t|--tool*) -- 2.0.2.611.g8c85416 -- 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/2] mergetool: don't require a work tree for --tool-help
From: Charles Bailey Signed-off-by: Charles Bailey --- You can call git difftool --tool-help outside of a work tree but not mergetool --tool-help but there's not real reason for this restriction and it can be easily relaxed by deferring the require_work_tree call until after the options have been parsed. git-mergetool.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-mergetool.sh b/git-mergetool.sh index 9a046b7..e969dd0 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -14,7 +14,6 @@ OPTIONS_SPEC= TOOL_MODE=merge . git-sh-setup . git-mergetool--lib -require_work_tree # Returns true if the mode reflects a symlink is_symlink () { @@ -372,6 +371,8 @@ prompt_after_failed_merge () { done } +require_work_tree + if test -z "$merge_tool" then # Check if a merge tool has been configured -- 2.0.2.611.g8c85416 -- 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] Fix contrib/subtree Makefile to patch #! line
From: Charles Bailey Signed-off-by: Charles Bailey --- The main Git Makefile does this, but not the contrib/subtree Makefile. SHELL_PATH should be respected if set (especially on Solaris where /bin/sh is very legacy). The sed command is a copy of one of the ones used for installing .sh files by the main Git Makefile. contrib/subtree/Makefile | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile index d888d45..d9a0ce2 100644 --- a/contrib/subtree/Makefile +++ b/contrib/subtree/Makefile @@ -18,6 +18,11 @@ RM ?= rm -f ASCIIDOC = asciidoc XMLTO= xmlto +ifndef SHELL_PATH + SHELL_PATH = /bin/sh +endif +SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH)) + ASCIIDOC_CONF = ../../Documentation/asciidoc.conf MANPAGE_XSL = ../../Documentation/manpage-normal.xsl @@ -32,7 +37,8 @@ GIT_SUBTREE_HTML := git-subtree.html all: $(GIT_SUBTREE) $(GIT_SUBTREE): $(GIT_SUBTREE_SH) - cp $< $@ && chmod +x $@ + sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' $< >$@ + chmod +x $@ doc: $(GIT_SUBTREE_DOC) $(GIT_SUBTREE_HTML) -- 2.0.2.611.g8c85416 -- 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] Fix filter-branch to eliminate duplicate mapped parents
On Mon, Jun 30, 2014 at 10:20:27PM +0100, Charles Bailey wrote: > From: Charles Bailey > > This change ensure that duplicate parents are pruned before the parent > filter and ensures that --prune-empty is idempotent, removing all > empty non-merge commits in a singe pass. s/change ensure/change ensures/ -- 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] Fix filter-branch to eliminate duplicate mapped parents
From: Charles Bailey When multiple parents of a merge commit get mapped to the same commit, filter-branch used to pass all instances of the parent commit to the parent and commit filters and to "git commit-tree" or "git_commit_non_empty_tree". This can often happen when extracting a small project from a large repository; merges can join history with no commits on any branch which affect the paths being retained. Once the intermediate commits have been filtered out, all the immediate parents of the merge commit can end up being mapped to the same commit - either the original merge-base or an ancestor of it. "git commit-tree" would display an error but write the commit with the normalized parents in any case. "git_commit_non_empty_tree" would fail to notice that the commit being made was in fact a non-merge commit and would retain it even if a further pass with --prune-empty would discard the commit as empty. This change ensure that duplicate parents are pruned before the parent filter and ensures that --prune-empty is idempotent, removing all empty non-merge commits in a singe pass. Signed-off-by: Charles Bailey --- I worked on this after discovering that --prune-empty often left some apparently empty commits that I was wasn't expecting it to leave and that running filter-branch --prune-empty in a loop would often do many passes where it was still pruning empty former merge commits. The test is a simple example of such a case. A non-ff merge of a commit that only changes a file that is to be pruned gets squashed into an empty non-merge commit that should be pruned. git-filter-branch.sh | 8 +++- t/t7003-filter-branch.sh | 11 +++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/git-filter-branch.sh b/git-filter-branch.sh index 86d6994..c5b82a8 100755 --- a/git-filter-branch.sh +++ b/git-filter-branch.sh @@ -332,7 +332,13 @@ while read commit parents; do parentstr= for parent in $parents; do for reparent in $(map "$parent"); do - parentstr="$parentstr -p $reparent" + case "$parentstr" in + *" -p $reparent"*) + ;; + *) + parentstr="$parentstr -p $reparent" + ;; + esac done done if [ "$filter_parent" ]; then diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh index 9496736..3741f51 100755 --- a/t/t7003-filter-branch.sh +++ b/t/t7003-filter-branch.sh @@ -308,6 +308,17 @@ test_expect_success 'Prune empty commits' ' test_cmp expect actual ' +test_expect_success 'Prune empty collapsed merges' ' + test_config merge.ff false && + git rev-list HEAD > expect && + test_commit to_remove_2 && + git reset --hard HEAD^ && + test_merge non-ff to_remove_2 && + git filter-branch -f --index-filter "git update-index --remove to_remove_2.t" --prune-empty HEAD && + git rev-list HEAD > actual && + test_cmp expect actual +' + test_expect_success '--remap-to-ancestor with filename filters' ' git checkout master && git reset --hard A && -- 1.9.0 -- 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] Detect endianness on more platforms that don't use BYTE_ORDER
On Fri, May 02, 2014 at 12:43:32PM -0700, Junio C Hamano wrote: > So,... I am inclined to queue this on top of your patch at least for > now, before I go into incommunicado-mode to finish preparing -rc2. Yes, I'd agree with that. -- 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] Detect endianness on more platforms that don't use BYTE_ORDER
On Fri, May 02, 2014 at 09:48:58AM -0700, Junio C Hamano wrote: > Charles Bailey writes: > > > --- > > Please sign-off your patches ;-) Oops! Please consider this patch... Signed-off-by: Charles Bailey > This swaps the precedence of BYTE_ORDER and __BYTE_ORDER from the > original, which we may not want to. It is easy for me to swap the > order of if/elif to restore it, so it is not a big deal, though. I think I swapped the precedence (semi-deliberately) because I found a proposal to standardize the BYTE_ORDER variant. I claim that any platform which provides both but with differing senses is somewhat broken so I cannot see the precedence mattering much. I don't mind either way. Charles. -- 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] Detect endianness on more platforms that don't use BYTE_ORDER
--- compat/bswap.h | 33 - 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/compat/bswap.h b/compat/bswap.h index 120c6c1..f08a9fe 100644 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -101,19 +101,34 @@ static inline uint64_t git_bswap64(uint64_t x) #undef ntohll #undef htonll -#if !defined(__BYTE_ORDER) -# if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN) -# define __BYTE_ORDER BYTE_ORDER -# define __LITTLE_ENDIAN LITTLE_ENDIAN -# define __BIG_ENDIAN BIG_ENDIAN +#if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN) + +# define GIT_BYTE_ORDER BYTE_ORDER +# define GIT_LITTLE_ENDIAN LITTLE_ENDIAN +# define GIT_BIG_ENDIAN BIG_ENDIAN + +#elif defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && defined(__BIG_ENDIAN) + +# define GIT_BYTE_ORDER __BYTE_ORDER +# define GIT_LITTLE_ENDIAN __LITTLE_ENDIAN +# define GIT_BIG_ENDIAN __BIG_ENDIAN + +#else + +# define GIT_BIG_ENDIAN 4321 +# define GIT_LITTLE_ENDIAN 1234 + +# if defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN) +# define GIT_BYTE_ORDER GIT_BIG_ENDIAN +# elif defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN) +# define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN +# else +# error "Cannot determine endianness" # endif -#endif -#if !defined(__BYTE_ORDER) -# error "Cannot determine endianness" #endif -#if __BYTE_ORDER == __BIG_ENDIAN +#if GIT_BYTE_ORDER == GIT_BIG_ENDIAN # define ntohll(n) (n) # define htonll(n) (n) #else -- 1.9.0 -- 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] Add extra logic required to detect endianness on Solaris
On Thu, May 01, 2014 at 11:58:26AM -0700, Junio C Hamano wrote: > > This patch seems to address two unrelated issues in that. > > (1) The existing support does not help a platform where the > convention is to define either _BIG_ENDIAN (with one leading > underscore) or _LITTLE_ENDIAN and not both, which is Solaris > but there may be others. > > (2) There may be __LITTLE_ENDIAN and __BIG_ENDIAN macros already > defined on the platform. Or these may not have been defined at > all. You avoid unconditionally redefing these. > > I find the latter iffy. Yes, you are right. I think I was uncomfortable defining macros with names reserved for the implementation even if the implementation didn't seem to be using them. I think I made my patch less correct as a result. Looking at the rest of the git source code we don't seem to use any of these macros anywhere else so perhaps we could use macros specific to GIT? Let me follow up with an alternative patch. Charles. -- 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] Add extra logic required to detect endianness on Solaris
Signed-off-by: Charles Bailey --- The endian detection added in 7e3dae494 isn't sufficient for the Solaris Studio compilers. This adds some fallback logic which works for Solaris but would also work for AIX and Linux if it were needed. compat/bswap.h | 21 + 1 file changed, 21 insertions(+) diff --git a/compat/bswap.h b/compat/bswap.h index 120c6c1..5a41311 100644 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -110,6 +110,27 @@ static inline uint64_t git_bswap64(uint64_t x) #endif #if !defined(__BYTE_ORDER) +/* Known to be needed on Solaris but designed to potentially more portable */ + +#if !defined(__BIG_ENDIAN) +#define __BIG_ENDIAN 4321 +#endif + +#if !defined(__LITTLE_ENDIAN) +#define __LITTLE_ENDIAN 1234 +#endif + +#if defined(_BIG_ENDIAN) +#define __BYTE_ORDER __BIG_ENDIAN +#endif + +#if defined(_LITTLE_ENDIAN) +#define __BYTE_ORDER __LITTLE_ENDIAN +#endif + +#endif /* !defined(__BYTE_ORDER) */ + +#if !defined(__BYTE_ORDER) # error "Cannot determine endianness" #endif -- 1.9.0 -- 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] mergetool: run prompt only if guessed tool
On Tue, Apr 22, 2014 at 02:56:22AM -0500, Felipe Contreras wrote: > An explicitly set mergetool.prompt = true would override the default. See the > patch. I have had a chance to test the patch now and it looks good. I think when glancing at it before I missed the change that dropped "|| echo true" from prompt=$(git config --bool mergetool.prompt || echo true) so I wasn't sure where the implicit true / explicit true difference was handled. > I looked, the documentation doesn't mention any default. We could add it, but > I > don't think it's necesarily part of this patch. The bit of documentation that I was thinking of is in Documentation/git-mergetool.txt where it states that "--prompt" is the default which is now only partially true. -- 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] mergetool: run prompt only if guessed tool
On Tue, Apr 22, 2014 at 01:53:46AM -0500, Felipe Contreras wrote: > Charles Bailey wrote: > > On Tue, Apr 22, 2014 at 01:24:09AM -0500, Felipe Contreras wrote: > > > > > > This is what I get when a tool is not working: > > > > > > Documentation/config.txt seems unchanged. > > > Was the merge successful? [y/n] > > > > Does this happen now even with merge tools for which we do trust the > > exit code? If so, my original concern is addressed. > > Which tools are those? > I didn't remember off hand, but checking the mergetools directory suggests that kdiff3 is one (there's no call to check_unchanged). I stopped checking after I found one. > You don't see anything wrong with asking the user *every single time* he runs > `git mergetool`, even though he *already told us* which tool to use? > > If so, I'm pretty sure everybody else disagrees with you. I think that you may have misunderstood me. As I said, I've no particular objections to changing the default (subject a few concerns that may or may not still apply). Having said that, the fact that the user has configured the merge tool doesn't mean that he necessarily doesn't want to see the prompt. In a part of my reply which you snipped, I said that it's sometimes the particular file that's due to be resolved that might prompt a user to want to skip launching the tool. Either way, I think we shouldn't unconditionally override an explicitly set mergetool.prompt and if we are (effectively) changing the default we should probably update the documentation to say so as well. (Yes, I didn't introduce the first "no prompt" option patch and yes, I have since changed my mind about whether I have it set, but that's just a personal preference.) -- 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] mergetool: run prompt only if guessed tool
On Tue, Apr 22, 2014 at 01:24:09AM -0500, Felipe Contreras wrote: > > This is what I get when a tool is not working: > > Documentation/config.txt seems unchanged. > Was the merge successful? [y/n] Does this happen now even with merge tools for which we do trust the exit code? If so, my original concern is addressed. > > Personally, I leave mergetool.prompt unset (default true) because one > > extra click in a complex merge resolution is relatively low overhead and > > to catch myself when I forget that I'm in a no-X environment. > > > > I glanced at the patch and it seems to subvert this intent for users > > who have configured a merge tool. Is my understanding correct? > > Yes, that's correct. If the user has *manually* configured a tool, why would > you ask him again? We are annoying the overwhelming the vast majority of users > who already configured the right tool in order to avoid issues with a minute > minority that might potentially but unlikely have a problem once or twice. > > That's not productive. We're asking to user to signal that he's happy to launch the mergetool and implicitly giving him an opportunity to break out of the session. I don't see anything wrong with having this behaviour. So long as we don't hit the loop-with-lots-of-error-trace for users who haven't set mergetool.prompt I've no strong objections to changing the default so long as an explictly set mergetool.prompt is respected. Ideally, I think I'd like the prompt to accept a "launch/skip/abort" range of options but that's a wider scoped thing. Sometimes when I'm resolving a lot of things and not specifying any paths I come across something that know I don't want to attempt a resolve with my currently configured tool and I just want to skip it for now. Currently, I have to either abort the session or let the mergetool fire up and close it again neither of which are optimal. -- 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] mergetool: run prompt only if guessed tool
On Mon, Apr 21, 2014 at 09:59:52PM -0700, David Aguilar wrote: > [Cc:ing Charles in case he has an opinion, this behavior dates back to the > original MT] > > On Sun, Apr 20, 2014 at 07:17:34PM -0500, Felipe Contreras wrote: > > It's annoying to see the prompt: > > > > Hit return to start merge resolution tool (foo): > > > > Every time the user does 'git mergetool' even if the user already > > configured 'foo' as the wanted tool. > > > > Display this prompt only when the user hasn't explicitly configured a > > tool. > > I agree this is probably helpful. > Most users I've met end up configuring mergetool.prompt=false. >From my memory, the reason that we choose to prompt by default is to help new users or users who have just changed their choice of mergetool. Because we never use the exit code of the tool to determine whether a tool "worked", if we don't prompt and the tool fails (bad custom command, requires X when no X available, etc.) then we'll repeatedly run the command for all paths requiring resolution leading to, potentially, a lot of trace with whatever error the tool might happen to report. We prompt by default to give users a chance to abort the mergetool session at the first opportunity that they see that the configured tool is not working. Personally, I leave mergetool.prompt unset (default true) because one extra click in a complex merge resolution is relatively low overhead and to catch myself when I forget that I'm in a no-X environment. I glanced at the patch and it seems to subvert this intent for users who have configured a merge tool. Is my understanding correct? -- 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] Don't rely on strerror text when testing rmdir failure
On Sat, Mar 29, 2014 at 04:48:44PM +0100, Jens Lehmann wrote: > Am 29.03.2014 16:39, schrieb Charles Bailey: > > diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh > > index 3d30581..23eed17 100755 > > --- a/t/t3600-rm.sh > > +++ b/t/t3600-rm.sh > > @@ -709,10 +709,9 @@ test_expect_success 'checking out a commit after > > submodule removal needs manual > > git commit -m "submodule removal" submod && > > git checkout HEAD^ && > > git submodule update && > > - git checkout -q HEAD^ 2>actual && > > + git checkout -q HEAD^ 2>/dev/null && > > Isn't this unrelated to the strerror issue you are fixing here? > Why not just drop the redirection completely? But maybe I'm just > being to pedantic here ;-) Well, it's a write to 'actual' and the next thing that tests the contents of 'actual' is the thing that I'm fixing so it's almost related. (See context kept below.) I changed the redirection here while investigating the bug. The redirected output was being overwritten immediately and this was a more explicit way to write "I don't care about whatever goes to stderr from this command" which confused me less that merely overwriting the file on the next line, but perhaps simply not redirecting is better. I really didn't give it much thought. > > > git checkout -q master 2>actual && > > - echo "warning: unable to rmdir submod: Directory not empty" >expected && > > - test_i18ncmp expected actual && > > + test_i18ngrep "^warning: unable to rmdir submod:" actual && Charles. -- 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
AIX fixes
[PATCH 1/2] Remove inline from git_fnmatch in dir.c There are currently a few issues with building on AIX. These two patches address two of them. The first removes 'inline' from a function in dir.c. The function has grown such that I don't really see a benefit in explicitly encouraging the compiler to inline. (As it is in a .c file, it's only going to be inlined for sophisticated toolchains doing LTO or similar for other translation units and with this sophistication the inline hinting behaviour is probably not so important.) The problem with having this function declared inline is a compliance issue. 6.7.4 of C99 says: > An inline definition of a function with external linkage shall not > contain a definition of a modifiable object with static storage > duration, and shall not contain a reference to an identifier with > internal linkage. git_fnmatch contains calls to ps_strncmp and ps_strcmp which are all declared static so violate this and xlC complains about this. [PATCH 2/2] Don't rely on strerror text when testing rmdir failure The second issue is that AIX doesn't distinguish between EEXIST and WNOTEMPTY so two tests that rely on the exact text of strerror for rmdir's failure to remove a non-empty directory fail. My personal take was that the exact text of strerror was not too important but we can test the leading portion of the error message which is under the control of Git and verifies that the readdir function reported a failure. I also have an issue where two low level tests (t0020-crlf and t0022-crlf-rename) fail (and perhaps later tests, too) unless I de-inline ce_path_match and dir_path_match from dir.h but as I cannot yet explain what the issue is or why this is a fix, I'm holding onto this one for now. Charles. -- 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/2] Don't rely on strerror text when testing rmdir failure
AIX doesn't make a distiction between EEXIST and ENOTEMPTY so relying on the strerror string for the rmdir failure is fragile. Just test that the start of the string matches the Git controlled "failed to rmdir..." error. The exact text of the OS generated error string isn't important to the test. Signed-off-by: Charles Bailey --- t/t3600-rm.sh | 5 ++--- t/t7001-mv.sh | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 3d30581..23eed17 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -709,10 +709,9 @@ test_expect_success 'checking out a commit after submodule removal needs manual git commit -m "submodule removal" submod && git checkout HEAD^ && git submodule update && - git checkout -q HEAD^ 2>actual && + git checkout -q HEAD^ 2>/dev/null && git checkout -q master 2>actual && - echo "warning: unable to rmdir submod: Directory not empty" >expected && - test_i18ncmp expected actual && + test_i18ngrep "^warning: unable to rmdir submod:" actual && git status -s submod >actual && echo "?? submod/" >expected && test_cmp expected actual && diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 215d43d..34fb1af 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -447,8 +447,7 @@ test_expect_success 'checking out a commit before submodule moved needs manual u git mv sub sub2 && git commit -m "moved sub to sub2" && git checkout -q HEAD^ 2>actual && - echo "warning: unable to rmdir sub2: Directory not empty" >expected && - test_i18ncmp expected actual && + test_i18ngrep "^warning: unable to rmdir sub2:" actual && git status -s sub2 >actual && echo "?? sub2/" >expected && test_cmp expected actual && -- 1.8.5.1.2.ge5d1dab -- 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/2] Remove inline from git_fnmatch in dir.c
Now that it calls a static inline function, it cannot be an inline definition with external linkage. Remove inline and make it an external definition. Signed-off-by: Charles Bailey --- dir.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dir.c b/dir.c index 99f5303..eb6f581 100644 --- a/dir.c +++ b/dir.c @@ -54,9 +54,9 @@ int fnmatch_icase(const char *pattern, const char *string, int flags) NULL); } -inline int git_fnmatch(const struct pathspec_item *item, - const char *pattern, const char *string, - int prefix) +int git_fnmatch(const struct pathspec_item *item, + const char *pattern, const char *string, + int prefix) { if (prefix > 0) { if (ps_strncmp(item, pattern, string, prefix)) -- 1.8.5.1.2.ge5d1dab -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 05:57:41PM -0400, Jeff King wrote: > Hmm, so the year you got is actually: 1623969404. That still seems off > to me by a factor 20. I don't know if this is really worth digging into > that much further, but I wonder what you would get for timestamps of: > > 9 > > 999 > etc. > AIX goes negative at about the same time Linux and Solaris segfault: 999 Sun Apr 26 10:46:39 1970 -0700 Sat Mar 3 02:46:39 1973 -0700 9 Sat Sep 8 18:46:39 2001 -0700 99 Sat Nov 20 10:46:39 2286 -0700 999 Wed Nov 16 02:46:39 5138 -0700 Thu Sep 26 18:46:39 33658 -0700 9 Sun May 20 10:46:39 318857 -0700 99 Sat Nov 7 02:46:39 3170843 -0700 999 Sat Jul 4 18:46:39 31690708 -0700 Sat Jan 25 10:46:39 316889355 -0700 9 Wed Sep 6 02:46:39 -1126091476 -0700 99 Thu Oct 24 18:46:39 1623969404 -0700 So, very bogus values. Charles. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > That being said, is the AIX value actually right? I did not look closely > at first, but just assumed that it was vaguely right. But: > > 99 / (86400 * 365) > > is something like 31 billion years in the future, not 160 million. > A real date calculation will have a few tweaks (leap years, etc), but > that is orders of magnitude off. Well, this is embarrassing, while moving this through the corporate firewall (aka typing on one machine while looking at another), I munged the date. It still doesn't seem right but at least you can now see the actual data. I stopped the test with --immediate and found the dangling commit that the test created and dumped it with the previous version of Git (well a 1.8.5.5 build) ibm: trash directory.t4212-log-corrupt $ git log -1 --pretty=raw 1fc17e734e4487c31bdfe05bb3d15618b69c4dca commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca tree 64fd3796c57084e7b8cbae358ce37970b8e954f6 author A U Thor 99 -0700 committer C O Mitter 1112911993 -0700 foo ibm: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca Author: A U Thor Date: Thu Oct 24 18:46:39 1623969404 -0700 foo Same commit but dumped from a linux machine: linux: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca Author: A U Thor Date: (null) foo Charles. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 04:38:30PM -0400, Jeff King wrote: > > By the way, can you confirm that this is a 64-bit system? On a 32-bit > system, we should be triggering different code paths (we fail at the > strtoul level). Those should be checked by the previous tests, but I'd > like to make sure. Yes, we're only building 64-bit at the moment. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:40:43PM -0400, Jeff King wrote: > On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > > That being said, is the AIX value actually right? I did not look closely > > at first, but just assumed that it was vaguely right. But: > > > > 99 / (86400 * 365) > > > > is something like 31 billion years in the future, not 160 million. > > A real date calculation will have a few tweaks (leap years, etc), but > > that is orders of magnitude off. > > Assuming my math is right, then here is the most sensible patch, IMHO. > Perhaps hold onto this one for a little while longer. Splitting things out from the test is giving me some inconsistent results, there may be something else going wrong in our environment here. Charles. -- 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 5/5] log: do not segfault on gmtime errors
On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote: > +# date is within 2^63-1, but enough to choke glibc's gmtime > +test_expect_success 'absurdly far-in-future dates produce sentinel' ' > + commit=$(munge_author_date HEAD 99) && > + echo "Thu Jan 1 00:00:00 1970 +" >expect && > + git log -1 --format=%ad $commit >actual && > + test_cmp expect actual > +' Git on AIX seems happy to convert this to Thu Oct 24 18:46:39 162396404 -0700. I don't know if this is correct for the given input but the test fails. Charles. -- 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: [RFC 0/3] Make git more user-friendly during a merge conflict
On Fri, Feb 28, 2014 at 03:01:53AM -0600, Stephen Leake wrote: > Jonathan Nieder writes: > > And for experienced users, this would be a bad regression. > > Backward incompatibility is a real concern. > > It might be best if "git reset" (with _no_ option) be made to error out, > so all users have to specify what they want. This is just as much of a regression (if less dangerous) as changing the default behaviour of git reset to touch the working tree. 'git reset' is a very, very common action for me and simply means 'reset [my index] [to HEAD]'. I frequently find myself resetting so that I can stage something a bit different to what I had originally intended. -- 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: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges
On Thu, May 09, 2013 at 03:17:30PM -0700, David Aguilar wrote: > Generally, "mergetool..cmd" is not general enough since we've > always special cased the base vs. no-base code paths and we run > different commands depending on whether a base is available. Then this is a deficiency of the ".cmd" mechanism which should provide an "if all else fails" way to do things, even if ugly. We could simply add a BASELESS variable to the eval environment of the custom command. (Do we always create an empty file for $BASE, now?) > We could drop --auto altogether, which maybe is a better course of > action since it makes the behavior predictable and un-surprising, but > I do not know if anyone has come to rely on kdiff3's "auto-merge" > feature (hence the extended Cc: list). I disagree, I think that a mergetool should be allowed to be as helpful as possible and if it can resolve the merge unaided then this is no bad thing. I've worked with a number of people who were very happy with the current kdiff3 behaviour. Nothing prevents you from verifying the merge and fixing it up if it wasn't done perfectly by the tool, although I haven't ever encountered this with git+kdiff3. Charles. -- 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: What should mergetool do with --no-prompt?
On Tue, Aug 14, 2012 at 08:06:56AM -0700, Junio C Hamano wrote: > > Could it be that the calling user or script does not even have a > terminal but still can spawn the chosen mergetool backend and > interact with the user via its GUI? Or it may have a terminal that > is hard for the user to interact with, and the prompt and "read ans" > may get stuck. It could be, although this certainly wasn't considered in the original design. I know that we removed explicit references to /dev/tty and replaced them with exec n>&m juggling which made things generally more robust and allowed some basic shell tests to work more reliably. I don't object to handling non-interactive mode better but it feels unsatisfactory to only be able to resolve some types of conflict and have to skip others. > In such an environment, the ideal behaviour for the "git mergetool" > frontend may be not to interact via the terminal at all and instead > run its interaction to choose the resolution using a matching GUI > interface. I see when "read ans" fails (e.g. the standard input to > the mergetool is closed), resolve_{symlink,deleted}_merge will not > get stuck but instead fail, so perhaps David's issue could be solved > by running "git mergetool --tool=... http://vger.kernel.org/majordomo-info.html
Re: What should mergetool do with --no-prompt?
On Tue, Aug 14, 2012 at 12:07:26AM -0700, David Aguilar wrote: > Right now there are two code paths, resolving deletion conflicts > and resolving symlink conflicts, in git-mergetool that do not > honor --no-prompt. They force user-interaction with the shell > even though the caller (such as a program) said that they do > not want to be prompted. > > This was an oversight from when this option was first added. > > I think a simple and sensible thing to do would be for mergetool > to skip over these entries when --no-prompt is supplied. > > Does this sound like a good idea? --no-prompt is designed to remove the prompt before launching a mergetool. This is because it is mostly pointless but does provide a convenient point to interrupt (C-c) a large multifile conflict resolution. It was never supposed to be a batch mode switch. By it's very nature mergetool is interactive so I don't see any advantage to pretending otherwise. If the documentation indicates otherwise then it's my opinion that this is what needs to be fixed. Charles. -- 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