Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Charles Bailey]

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

[Charles Bailey]

From: Charles Bailey <cbaile...@bloomberg.net>

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, );
+   p->pcre1_extra_info = pcre_study(p->pcre1_regexp, 
GIT_PCRE_STUDY_JIT_COMPILE, );
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

[Charles Bailey]

From: Charles Bailey <cbaile...@bloomberg.net>

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 <cbaile...@bloomberg.net>
---
 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

[Charles Bailey]

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

[Charles Bailey]

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

[Charles Bailey]

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

[Charles Bailey]

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)

[Charles Bailey]

On Thu, Jul 07, 2016 at 02:21:28PM -0700, Junio C Hamano wrote:
> Charles Bailey <char...@hashpling.org> 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)

[Charles Bailey]

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

[Charles Bailey]

Signed-off-by: Charles Bailey <char...@hashpling.org>
---
 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

[Charles Bailey]

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

[Charles Bailey]

Signed-off-by: Charles Bailey <char...@hashpling.org>
---
 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

[Charles Bailey]

From: Charles Bailey <cbaile...@bloomberg.net>

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 <pclo...@gmail.com>
Signed-off-by: Charles Bailey <cbaile...@bloomberg.net>
---
 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 &&
+   git add -N intend-to-add

[PATCH v2 1/2] Fix duplicated test name

[Charles Bailey]

Signed-off-by: Charles Bailey <char...@hashpling.org>
---

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

[Charles Bailey]

From: Charles Bailey <cbaile...@bloomberg.net>

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 <pclo...@gmail.com>
Signed-off-by: Charles Bailey <cbaile...@bloomberg.net>
---

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

[Charles Bailey]

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

[Charles Bailey]

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

[Charles Bailey]

From: Charles Bailey <cbaile...@bloomberg.net>

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 <cbaile...@bloomberg.net>
---

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?

[Charles Bailey]

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: git branch in Bash subshell "$(git branch -a)" including ls output as part of return?

[Charles Bailey]

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: [PATCH] Fix quoting of redirect in test script

[Charles Bailey]

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 <cbaile...@bloomberg.net>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Fix quoting of redirect in test script

[Charles Bailey]

From: Charles Bailey <cbaile...@bloomberg.net>

---
 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] Fix detection of uname failure

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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] Fix detection of uname failure

[Charles Bailey]

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 v2] Fix detection of uname failure

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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

[PATCH] Fix definition of ARRAY_SIZE for non-gcc builds

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---

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

[PATCH v2] Fix definition of ARRAY_SIZE for non-gcc builds

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---

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

Re: [Question] Is it normal for accented characters to be shown as decomposed Unicode on GNU/Linux?

[Charles Bailey]

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 file... 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 dpota...@gmail.com
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

Re: [PATCH] Add list-all-objects command

[Charles Bailey]

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: [PATCH 8/7] cat-file: sort and de-dup output of --batch-all-objects

[Charles Bailey]

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: Improvements to integer option parsing

[Charles Bailey]

 On 22 Jun 2015, at 23:09, Junio C Hamano gits...@pobox.com wrote:
 
 Charles Bailey char...@hashpling.org 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] Add list-all-objects command

[Charles Bailey]

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

[PATCH 2/3] contrib/subtree: Fix broken -chains and revealed test error

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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

[PATCH 1/3] contrib/subtree: Use tabs consitently for indentation in tests

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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 '$(git rev-parse sub1)'
+   git subtree add --prefix=subdir/ FETCH_HEAD 
+   check_equal ''$(last_commit_message)'' Add ''subdir/'' from commit 
'$(git rev-parse sub1)'
 '
 
 # this shouldn't actually do anything, since FETCH_HEAD is already a parent
 test_expect_success 'merge fetched subproj' '
-git merge -m merge -s -ours -s ours FETCH_HEAD
+   git merge -m merge -s -ours -s ours FETCH_HEAD
 '
 
 test_expect_success 'add main-sub5' '
-create subdir/main-sub5 
-git commit -m main-sub5
+   create subdir/main-sub5 
+   git commit -m main-sub5
 '
 
 test_expect_success 'add main6' '
-create main6 
-git commit -m main6 boring
+   create main6 
+   git commit -m main6 boring
 '
 
 test_expect_success 'add main-sub7' '
-create subdir/main-sub7 
-git commit -m main-sub7
+   create subdir/main-sub7 
+   git commit -m main-sub7
 '
 
 test_expect_success 'fetch new subproj history' '
-git fetch ./subproj sub2 
-git branch sub2 FETCH_HEAD
+   git fetch

[PATCH 3/3] contrib/subtree: Small tidy-up to test

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

There's no need to switch branches to parse another branch's ancestry.

Signed-off-by: Charles Bailey cbaile...@bloomberg.net
---
 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

Re: [PATCH 2/2] Move unsigned long option parsing out of pack-objects.c

[Charles Bailey]

On Sun, Jun 21, 2015 at 07:25:44PM +0100, Charles Bailey wrote:
 From: Charles Bailey cbaile...@bloomberg.net
 
 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

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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 @@ 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

Improvements to integer option parsing

[Charles Bailey]

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

Fast enumeration of objects

[Charles Bailey]

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

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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:
+   sha1 SP type SP size
+
+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 stdio.h
+
+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.c
+++ b/git.c
@@ -417,6 +417,7 @@ static struct cmd_struct commands

Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c

[Charles Bailey]

On Fri, Jun 19, 2015 at 10:58:51AM -0700, Junio C Hamano wrote:
 Charles Bailey char...@hashpling.org 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

[PATCH 3/3] Add filter-objects command

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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 | --type=type] [--min-size=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 type.
+
+--min-size::
+   Only list objects whose size exceeds size bytes.
+
+-v::
+--verbose::
+   Output in the followin format instead of just printing object ids:
+   sha1 SP type SP size
+
+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 stdio.h
+
+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()
+};
+
+int cmd_filter_objects(int argc, const char **argv, const char *prefix)
+{
+   struct packed_git *p;
+
+   argc

Re: [PATCH 3/3] Add filter-objects command

[Charles Bailey]

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

Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c

[Charles Bailey]

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 2output.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

[PATCH 2/3] Move unsigned long option parsing out of pack-objects.c

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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 options
 
 -i, --integer n get a integer
 -j nget a integer, too
+-u, --unsigned-long n
+  get an unsigned long
 --set23   set integer to 23
 -t time get timestamp of time
 -L, --length strget length of str
@@ -58,6 +60,7 @@ mv expect expect.err
 cat expect.template EOF
 boolean: 0
 integer: 0
+unsigned long: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
@@ -132,9 +135,32 @@ 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'
+
+test_expect_success 'OPT_ULONG() simple' '
+   check unsigned long: 2345678 -u 2345678
+'
+
+test_expect_success 'OPT_ULONG() kilo' '
+   check unsigned long: 239616 -u 234k
+'
+
+test_expect_success 'OPT_ULONG() mega' '
+   check unsigned long: 104857600 -u 100m
+'
+
+test_expect_success 'OPT_ULONG() giga' '
+   check unsigned long: 1073741824 -u 1g
+'
+
+test_expect_success 'OPT_ULONG() 3giga' '
+   check unsigned long: 3221225472 -u 3g
+'
+
 cat  expect  EOF
 boolean: 2
 integer: 1729
+unsigned long: 16384
 timestamp: 0
 string: 123
 abbrev: 7
@@ -145,7 +171,7 @@ file

Improvements to parse-options and a new filter-objects command

[Charles Bailey]

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 1/3] Correct test-parse-options to handle negative ints

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
 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

Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option

[Charles Bailey]

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

[Charles Bailey]

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

[Charles Bailey]

 On 20 Apr 2015, at 06:30, Junio C Hamano gits...@pobox.com wrote:
 Charles Bailey char...@hashpling.org 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

[Charles Bailey]

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] Add failing test for fetching from multiple packs over dumb httpd

[Charles Bailey]

On Tue, Jan 27, 2015 at 01:12:21PM -0500, Jeff King wrote:
 On Tue, Jan 27, 2015 at 03:20:41PM +, Charles Bailey wrote:
 
  From: Charles Bailey cbaile...@bloomberg.net
  
  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

Re: [PATCH] dumb-http: do not pass NULL path to parse_pack_index

[Charles Bailey]

On Tue, Jan 27, 2015 at 03:02:27PM -0500, Jeff King wrote:
 Discovery and tests by Charles Bailey char...@hashpling.org.
 
 Signed-off-by: Jeff King p...@peff.net
 ---
 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

[PATCH] Add failing test for fetching from multiple packs over dumb httpd

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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

[Charles Bailey]

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

[Charles Bailey]

 On 11 Oct 2014, at 09:29, David Aguilar dav...@gmail.com 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] mergetool: use more conservative temporary filenames

[Charles Bailey]

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: [PATCH v2 1/2] mergetool: don't require a work tree for --tool-help

[Charles Bailey]

On 10 Oct 2014, at 09:51, David Aguilar dav...@gmail.com 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: What's cooking in git.git (Aug 2014, #02; Fri, 8)

[Charles Bailey]

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?

[Charles Bailey]

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

[PATCH] Fix contrib/subtree Makefile to patch #! line

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

Signed-off-by: Charles Bailey cbaile...@bloomberg.net
---
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

[PATCH 1/2] mergetool: don't require a work tree for --tool-help

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

Signed-off-by: Charles Bailey cbaile...@bloomberg.net
---
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 2/2] difftool: don't assume that default sh is sane

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---
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

Re: [PATCH 2/2] difftool: don't assume that default sh is sane

[Charles Bailey]

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

Re: [PATCH] Fix filter-branch to eliminate duplicate mapped parents

[Charles Bailey]

On Mon, Jun 30, 2014 at 10:20:27PM +0100, Charles Bailey wrote:
 From: Charles Bailey cbaile...@bloomberg.net
 
 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

[Charles Bailey]

From: Charles Bailey cbaile...@bloomberg.net

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 cbaile...@bloomberg.net
---

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] Add extra logic required to detect endianness on Solaris

[Charles Bailey]

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] Detect endianness on more platforms that don't use BYTE_ORDER

[Charles Bailey]

---
 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] Detect endianness on more platforms that don't use BYTE_ORDER

[Charles Bailey]

On Fri, May 02, 2014 at 09:48:58AM -0700, Junio C Hamano wrote:
 Charles Bailey cbaile...@bloomberg.net writes:
 
  ---
 
 Please sign-off your patches ;-)

Oops! Please consider this patch...

Signed-off-by: Charles Bailey cbaile...@bloomberg.net

 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

Re: [PATCH] Detect endianness on more platforms that don't use BYTE_ORDER

[Charles Bailey]

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

[PATCH] Add extra logic required to detect endianness on Solaris

[Charles Bailey]

Signed-off-by: Charles Bailey cbaile...@bloomberg.net
---

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

[Charles Bailey]

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

[Charles Bailey]

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] mergetool: run prompt only if guessed tool

[Charles Bailey]

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

[Charles Bailey]

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

[PATCH 2/2] Don't rely on strerror text when testing rmdir failure

[Charles Bailey]

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 cbaile...@bloomberg.net
---
 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^ 2actual 
+   git checkout -q HEAD^ 2/dev/null 
git checkout -q master 2actual 
-   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^ 2actual 
-   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

[Charles Bailey]

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 cbaile...@bloomberg.net
---
 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

AIX fixes

[Charles Bailey]

[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

Re: [PATCH 2/2] Don't rely on strerror text when testing rmdir failure

[Charles Bailey]

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^ 2actual 
  +   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 2actual 
  -   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

Re: [PATCH 5/5] log: do not segfault on gmtime errors

[Charles Bailey]

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: [PATCH] t4212: handle systems with post-apocalyptic gmtime

[Charles Bailey]

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] t4212: handle systems with post-apocalyptic gmtime

[Charles Bailey]

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

[Charles Bailey]

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 aut...@example.com 99 -0700
  committer C O Mitter commit...@example.com 1112911993 -0700

  foo

  ibm: trash directory.t4212-log-corrupt $ git log -1 
1fc17e734e4487c31bdfe05bb3d15618b69c4dca
  commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca
  Author: A U Thor aut...@example.com
  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 aut...@example.com
  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

[Charles Bailey]

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: [RFC 0/3] Make git more user-friendly during a merge conflict

[Charles Bailey]

On Fri, Feb 28, 2014 at 03:01:53AM -0600, Stephen Leake wrote:
 Jonathan Nieder jrnie...@gmail.com 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

[Charles Bailey]

On Thu, May 09, 2013 at 03:17:30PM -0700, David Aguilar wrote:
 Generally, mergetool.tool.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?

[Charles Bailey]

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

Re: What should mergetool do with --no-prompt?

[Charles Bailey]

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 nm 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=... /dev/null or something?

To be honest, I wasn't sure what David's issue was, other than I
spotted this could/should it be fixed?. Is it a real world issue?

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