Re: [PATCH] alloc: allow arbitrary repositories for alloc functions

[Junio C Hamano]

Stefan Beller  writes:

> We have to convert all of the alloc functions at once, because alloc_report
> uses a funky macro for reporting. It is better for the sake of mechanical
> conversion to convert multiple functions at once rather than changing the
> structure of the reporting function.
>
> We record all memory allocation in alloc.c, and free them in
> clear_alloc_state, which is called for all repositories except
> the_repository.
>
> Signed-off-by: Stefan Beller 
> Signed-off-by: Junio C Hamano 
> ---
>
> Notes:
>
> Eric, I have fixed s/relase/release/
>
>
> Jonathan,
>
>> This might seem a bit bikesheddy, but I wouldn't call it
>> free_tag_buffer(), since what's being freed is not the buffer of the
>> object itself, but just a string. If you want such a function, I would
>> just call it release_tag_memory() to match release_commit_memory().
>
> So you would replace the last commit with a patch like this?

Quite honestly, I do not see much difference either way between free
and release.  None of the resetting the .parsed bit, .index to 0,
and tagged to NULL is about releasing or freeing resources.  These
operations are about de-initializing the object, which may well
involve releasing resources directly associated with the object
itself, but even more.  The release_commit_memory() function may
want to do something to various commit slabs that refer back to the
commit, for example.

But as I said, I do not deeply care either way ;-)

Thanks, the patch looks sensible.

>
> Thanks,
> Stefan
>
> diff to what is currently queued:
>
> diff --git c/commit.c w/commit.c
> index 612ccf7b053..5eb4d2f08f8 100644
> --- c/commit.c
> +++ w/commit.c
> @@ -299,9 +299,13 @@ void free_commit_buffer(struct commit *commit)
> 
>  void release_commit_memory(struct commit *c)
>  {
> + c->tree = NULL;
> + c->index = 0;
>   free_commit_buffer(c);
>   free_commit_list(c->parents);
>   /* TODO: what about commit->util? */
> +
> + c->object.parsed = 0;
>  }
> 
>  const void *detach_commit_buffer(struct commit *commit, unsigned long 
> *sizep)
> diff --git c/object.c w/object.c
> index 9d5b10d5a20..8e29f63bf23 100644
> --- c/object.c
> +++ w/object.c
> @@ -528,7 +528,7 @@ void parsed_object_pool_clear(struct 
> parsed_object_pool *o)
>   else if (obj->type == OBJ_COMMIT)
>   release_commit_memory((struct commit*)obj);
>   else if (obj->type == OBJ_TAG)
> - free_tag_buffer((struct tag*)obj);
> + release_tag_memory((struct tag*)obj);
>   }
> 
>   FREE_AND_NULL(o->obj_hash);
> diff --git c/tag.c w/tag.c
> index 254352c30c6..7c12426b4ea 100644
> --- c/tag.c
> +++ w/tag.c
> @@ -116,9 +116,12 @@ static timestamp_t parse_tag_date(const char *buf, 
> const char *tail)
>   return parse_timestamp(dateptr, NULL, 10);
>  }
> 
> -void free_tag_buffer(struct tag *t)
> +void release_tag_memory(struct tag *t)
>  {
>   free(t->tag);
> + t->tagged = NULL;
> + t->object.parsed = 0;
> + t->date = 0;
>  }
> 
>  int parse_tag_buffer(struct tag *item, const void *data, unsigned long 
> size)
> diff --git c/tag.h w/tag.h
> index b241fe67bc5..9057d76a506 100644
> --- c/tag.h
> +++ w/tag.h
> @@ -15,7 +15,7 @@ struct tag {
>  extern struct tag *lookup_tag(const struct object_id *oid);
>  extern int parse_tag_buffer(struct tag *item, const void *data, unsigned 
> long size);
>  extern int parse_tag(struct tag *item);
> -extern void free_tag_buffer(struct tag *t);
> +extern void release_tag_memory(struct tag *t);
>  extern struct object *deref_tag(struct object *, const char *, int);
>  extern struct object *deref_tag_noverify(struct object *);
>  extern int gpg_verify_tag(const struct object_id *oid,
>
>  alloc.c   | 65 ++-
>  alloc.h   | 19 ++
>  blame.c   |  1 +
>  blob.c|  1 +
>  cache.h   | 16 
>  commit.c  | 12 +
>  commit.h  |  6 +
>  merge-recursive.c |  1 +
>  object.c  | 42 --
>  object.h  |  8 ++
>  tag.c |  9 +++
>  tag.h |  1 +
>  tree.c|  1 +
>  13 files changed, 140 insertions(+), 42 deletions(-)
>  create mode 100644 alloc.h
>
> diff --git a/alloc.c b/alloc.c
> index 277dadd221b..714df633169 100644
> --- a/alloc.c
> +++ b/alloc.c
> @@ -4,8 +4,7 @@
>   * Copyright (C) 2006 Linus Torvalds
>   *
>   * The standard malloc/free wastes too much space for objects, partly because
> - * it maintains all the allocation infrastructure (which isn't needed, since
> - * we never free an object descriptor anyway), but even more because it ends
> + * it 

[PATCH v3 15/28] t4007: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs and uses the
ZERO_OID variable instead of using hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4007-rename-3.sh | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh
index dae327fabb..b187b7f6c6 100755
--- a/t/t4007-rename-3.sh
+++ b/t/t4007-rename-3.sh
@@ -17,6 +17,7 @@ test_expect_success 'prepare reference tree' '
echo $tree
 '
 
+blob=$(git hash-object "$TEST_DIRECTORY/diff-lib/COPYING")
 test_expect_success 'prepare work tree' '
cp path0/COPYING path1/COPYING &&
git update-index --add --remove path0/COPYING path1/COPYING
@@ -26,8 +27,8 @@ test_expect_success 'prepare work tree' '
 # path1 both have COPYING and the latter is a copy of path0/COPYING.
 # Comparing the full tree with cache should tell us so.
 
-cat >expected <<\EOF
-:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 
6ff87c4664981e4397625791c8ea3bbb5f2279a3 C100  path0/COPYING   path1/COPYING
+cat >expected expected expected expected <

[PATCH v3 20/28] t4029: fix test indentation

[brian m. carlson]

We typically indent our tests with a single tab, partially so that we
can take advantage of indented heredocs.  Make this change and move the
quote marks to be in the typical position for our tests.

Signed-off-by: brian m. carlson 
---
 t/t4029-diff-trailing-space.sh | 33 -
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/t/t4029-diff-trailing-space.sh b/t/t4029-diff-trailing-space.sh
index 3ccc237a8d..f4e18cb8d3 100755
--- a/t/t4029-diff-trailing-space.sh
+++ b/t/t4029-diff-trailing-space.sh
@@ -18,22 +18,21 @@ index 5f6a263..8cb8bae 100644
 EOF
 exit 1
 
-test_expect_success \
-"$test_description" \
-'printf "\nx\n" > f &&
- git add f &&
- git commit -q -m. f &&
- printf "\ny\n" > f &&
- git config --bool diff.suppressBlankEmpty true &&
- git diff f > actual &&
- test_cmp exp actual &&
- perl -i.bak -p -e "s/^\$/ /" exp &&
- git config --bool diff.suppressBlankEmpty false &&
- git diff f > actual &&
- test_cmp exp actual &&
- git config --bool --unset diff.suppressBlankEmpty &&
- git diff f > actual &&
- test_cmp exp actual
- '
+test_expect_success "$test_description" '
+   printf "\nx\n" > f &&
+   git add f &&
+   git commit -q -m. f &&
+   printf "\ny\n" > f &&
+   git config --bool diff.suppressBlankEmpty true &&
+   git diff f > actual &&
+   test_cmp exp actual &&
+   perl -i.bak -p -e "s/^\$/ /" exp &&
+   git config --bool diff.suppressBlankEmpty false &&
+   git diff f > actual &&
+   test_cmp exp actual &&
+   git config --bool --unset diff.suppressBlankEmpty &&
+   git diff f > actual &&
+   test_cmp exp actual
+'
 
 test_done

[PATCH v3 18/28] t4020: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4020-diff-external.sh | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 49d3f54b29..e009826fcb 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -13,6 +13,8 @@ test_expect_success setup '
 
test_tick &&
echo second >file &&
+   before=$(git hash-object file) &&
+   before=$(git rev-parse --short $before) &&
git add file &&
git commit -m second &&
 
@@ -180,9 +182,13 @@ test_expect_success 'no diff with -diff' '
 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file
 
 test_expect_success 'force diff with "diff"' '
+   after=$(git hash-object file) &&
+   after=$(git rev-parse --short $after) &&
echo >.gitattributes "file diff" &&
git diff >actual &&
-   test_cmp "$TEST_DIRECTORY"/t4020/diff.NUL actual
+   sed -e "s/^index .*/index $before..$after 100644/" \
+   "$TEST_DIRECTORY"/t4020/diff.NUL >expected-diff &&
+   test_cmp expected-diff actual
 '
 
 test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '
@@ -237,7 +243,7 @@ test_expect_success 'diff --cached' '
git update-index --assume-unchanged file &&
echo second >file &&
git diff --cached >actual &&
-   test_cmp "$TEST_DIRECTORY"/t4020/diff.NUL actual
+   test_cmp expected-diff actual
 '
 
 test_expect_success 'clean up crlf leftovers' '

[PATCH v3 19/28] t4022: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4022-diff-rewrite.sh | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh
index cb51d9f9d4..6d1c3d949c 100755
--- a/t/t4022-diff-rewrite.sh
+++ b/t/t4022-diff-rewrite.sh
@@ -13,6 +13,8 @@ test_expect_success setup '
  "nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM" \
  <"$TEST_DIRECTORY"/../COPYING >test &&
echo "to be deleted" >test2 &&
+   blob=$(git hash-object test2) &&
+   blob=$(git rev-parse --short $blob) &&
git add test2
 
 '
@@ -27,7 +29,7 @@ test_expect_success 'detect rewrite' '
 cat >expect 

[PATCH v3 13/28] t3702: abstract away SHA-1-specific constants

[brian m. carlson]

Strip out the index lines in the diff before comparing them, as these
will differ between hash algorithms.  This leads to a smaller, simpler
change than editing the index line.

Signed-off-by: brian m. carlson 
---
 t/t3702-add-edit.sh | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t3702-add-edit.sh b/t/t3702-add-edit.sh
index 3cb74ca296..1be5cd5756 100755
--- a/t/t3702-add-edit.sh
+++ b/t/t3702-add-edit.sh
@@ -40,7 +40,6 @@ test_expect_success 'setup' '
 
 cat > expected-patch << EOF
 diff --git a/file b/file
-index b9834b5..9020acb 100644
 --- a/file
 +++ b/file
 @@ -1,11 +1,6 @@
@@ -80,7 +79,6 @@ EOF
 
 cat > expected << EOF
 diff --git a/file b/file
-index b9834b5..ef6e94c 100644
 --- a/file
 +++ b/file
 @@ -1,10 +1,12 @@
@@ -100,7 +98,7 @@ EOF
 
 echo "#!$SHELL_PATH" >fake-editor.sh
 cat >> fake-editor.sh <<\EOF
-mv -f "$1" orig-patch &&
+egrep -v '^index' "$1" >orig-patch &&
 mv -f patch "$1"
 EOF
 
@@ -113,7 +111,8 @@ test_expect_success 'add -e' '
git add -e &&
test_cmp second-part file &&
test_cmp orig-patch expected-patch &&
-   git diff --cached > out &&
+   git diff --cached >actual &&
+   egrep -v "^index " actual >out &&
test_cmp out expected
 
 '

[PATCH v3 12/28] t3103: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it uses variables and command substitution for
trees instead of hard-coded hashes.  This also has the benefit of making
it more obvious how the test works.

Signed-off-by: brian m. carlson 
---
 t/t3103-ls-tree-misc.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh
index 09dcf043fd..14520913af 100755
--- a/t/t3103-ls-tree-misc.sh
+++ b/t/t3103-ls-tree-misc.sh
@@ -17,7 +17,8 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '
-   rm -f .git/objects/5f/cffbd6e4c5c5b8d81f5e9314b20e338e35 &&
+   tree=$(git rev-parse HEAD:a) &&
+   rm -f .git/objects/$(echo $tree | sed -e "s,^\(..\),\1/,") &&
test_must_fail git ls-tree -r HEAD
 '
 

[PATCH v3 08/28] t1512: skip test if not using SHA-1

[brian m. carlson]

This test relies on objects with colliding short names which are
necessarily dependent on the hash used.  Skip the test if we're not
using SHA-1.

Signed-off-by: brian m. carlson 
---
 t/t1512-rev-parse-disambiguation.sh | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/t/t1512-rev-parse-disambiguation.sh 
b/t/t1512-rev-parse-disambiguation.sh
index 711704ba5a..6537f30c9e 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -22,6 +22,12 @@ one tagged as v1.0.0.  They all have one regular file each.
 
 . ./test-lib.sh
 
+if ! test_have_prereq SHA1
+then
+   skip_all='not using SHA-1 for objects'
+   test_done
+fi
+
 test_expect_success 'blob and tree' '
test_tick &&
(

[PATCH v3 06/28] t0000: annotate with SHA1 prerequisite

[brian m. carlson]

Since this is a core test that tests basic functionality, annotate the
assertions that have dependencies on SHA-1 with the appropriate
prerequisite.

Signed-off-by: brian m. carlson 
---
 t/t-basic.sh | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/t-basic.sh b/t/t-basic.sh
index 7fd87dd544..af61d083b4 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -839,7 +839,7 @@ test_expect_success 'writing tree out with git write-tree' '
 '
 
 # we know the shape and contents of the tree and know the object ID for it.
-test_expect_success 'validate object ID of a known tree' '
+test_expect_success SHA1 'validate object ID of a known tree' '
test "$tree" = 7bb943559a305bdd6bdee2cef6e5df2413c3d30a
 '
 
@@ -882,7 +882,7 @@ test_expect_success 'showing stage with git ls-files 
--stage' '
git ls-files --stage >current
 '
 
-test_expect_success 'validate git ls-files output for a known tree' '
+test_expect_success SHA1 'validate git ls-files output for a known tree' '
cat >expected <<-\EOF &&
100644 f87290f8eb2cbbea7857214459a0739927eab154 0   path0
12 15a98433ae33114b085f3eb3bb03b832b3180a01 0   path0sym
@@ -900,7 +900,7 @@ test_expect_success 'writing tree out with git write-tree' '
tree=$(git write-tree)
 '
 
-test_expect_success 'validate object ID for a known tree' '
+test_expect_success SHA1 'validate object ID for a known tree' '
test "$tree" = 087704a96baf1c2d1c869a8b084481e121c88b5b
 '
 
@@ -908,7 +908,7 @@ test_expect_success 'showing tree with git ls-tree' '
 git ls-tree $tree >current
 '
 
-test_expect_success 'git ls-tree output for a known tree' '
+test_expect_success SHA1 'git ls-tree output for a known tree' '
cat >expected <<-\EOF &&
100644 blob f87290f8eb2cbbea7857214459a0739927eab154path0
12 blob 15a98433ae33114b085f3eb3bb03b832b3180a01path0sym
@@ -924,7 +924,7 @@ test_expect_success 'showing tree with git ls-tree -r' '
git ls-tree -r $tree >current
 '
 
-test_expect_success 'git ls-tree -r output for a known tree' '
+test_expect_success SHA1 'git ls-tree -r output for a known tree' '
cat >expected <<-\EOF &&
100644 blob f87290f8eb2cbbea7857214459a0739927eab154path0
12 blob 15a98433ae33114b085f3eb3bb03b832b3180a01path0sym
@@ -943,7 +943,7 @@ test_expect_success 'showing tree with git ls-tree -r -t' '
git ls-tree -r -t $tree >current
 '
 
-test_expect_success 'git ls-tree -r output for a known tree' '
+test_expect_success SHA1 'git ls-tree -r output for a known tree' '
cat >expected <<-\EOF &&
100644 blob f87290f8eb2cbbea7857214459a0739927eab154path0
12 blob 15a98433ae33114b085f3eb3bb03b832b3180a01path0sym
@@ -964,7 +964,7 @@ test_expect_success 'writing partial tree out with git 
write-tree --prefix' '
ptree=$(git write-tree --prefix=path3)
 '
 
-test_expect_success 'validate object ID for a known tree' '
+test_expect_success SHA1 'validate object ID for a known tree' '
test "$ptree" = 21ae8269cacbe57ae09138dcc3a2887f904d02b3
 '
 
@@ -972,7 +972,7 @@ test_expect_success 'writing partial tree out with git 
write-tree --prefix' '
ptree=$(git write-tree --prefix=path3/subp3)
 '
 
-test_expect_success 'validate object ID for a known tree' '
+test_expect_success SHA1 'validate object ID for a known tree' '
test "$ptree" = 3c5e5399f3a333eddecce7a9b9465b63f65f51e2
 '
 
@@ -1006,7 +1006,7 @@ test_expect_success 'git read-tree followed by write-tree 
should be idempotent'
test "$newtree" = "$tree"
 '
 
-test_expect_success 'validate git diff-files output for a know cache/work tree 
state' '
+test_expect_success SHA1 'validate git diff-files output for a know cache/work 
tree state' '
cat >expected <<\EOF &&
 :100644 100644 f87290f8eb2cbbea7857214459a0739927eab154 
 M path0
 :12 12 15a98433ae33114b085f3eb3bb03b832b3180a01 
 M path0sym
@@ -1033,21 +1033,21 @@ test_expect_success 'no diff after checkout and git 
update-index --refresh' '
 
 P=087704a96baf1c2d1c869a8b084481e121c88b5b
 
-test_expect_success 'git commit-tree records the correct tree in a commit' '
+test_expect_success SHA1 'git commit-tree records the correct tree in a 
commit' '
commit0=$(echo NO | git commit-tree $P) &&
tree=$(git show --pretty=raw $commit0 |
 sed -n -e "s/^tree //p" -e "/^author /q") &&
test "z$tree" = "z$P"
 '
 
-test_expect_success 'git commit-tree records the correct parent in a commit' '
+test_expect_success SHA1 'git commit-tree records the correct parent in a 
commit' '
commit1=$(echo NO | git commit-tree $P -p $commit0) &&
parent=$(git show --pretty=raw $commit1 |
  

[PATCH v3 16/28] t4008: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4008-diff-break-rewrite.sh | 59 +++
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh
index 9dd1bc5e16..b1ccd4102e 100755
--- a/t/t4008-diff-break-rewrite.sh
+++ b/t/t4008-diff-break-rewrite.sh
@@ -27,29 +27,32 @@ Further, with -B and -M together, these should turn into 
two renames.
 test_expect_success setup '
cat "$TEST_DIRECTORY"/diff-lib/README >file0 &&
cat "$TEST_DIRECTORY"/diff-lib/COPYING >file1 &&
+   blob0_id=$(git hash-object file0) &&
+   blob1_id=$(git hash-object file1) &&
git update-index --add file0 file1 &&
git tag reference $(git write-tree)
 '
 
 test_expect_success 'change file1 with copy-edit of file0 and remove file0' '
sed -e "s/git/GIT/" file0 >file1 &&
+   blob2_id=$(git hash-object file1) &&
rm -f file0 &&
git update-index --remove file0 file1
 '
 
 test_expect_success 'run diff with -B (#1)' '
git diff-index -B --cached reference >current &&
-   cat >expect <<-\EOF &&
-   :100644 00 548142c327a6790ff8821d67c2ee1eff7a656b52 
 D  file0
-   :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 
2fbedd0b5d4b8126e4750c3bee305e8ff79f80ec M100   file1
+   cat >expect <<-EOF &&
+   :100644 00 $blob0_id $ZERO_OID Dfile0
+   :100644 100644 $blob1_id $blob2_id M100 file1
EOF
compare_diff_raw expect current
 '
 
 test_expect_success 'run diff with -B and -M (#2)' '
git diff-index -B -M reference >current &&
-   cat >expect <<-\EOF &&
-   :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 
2fbedd0b5d4b8126e4750c3bee305e8ff79f80ec R100   file0   file1
+   cat >expect <<-EOF &&
+   :100644 100644 $blob0_id $blob2_id R100 file0   file1
EOF
compare_diff_raw expect current
 '
@@ -66,18 +69,18 @@ test_expect_success 'swap file0 and file1' '
 
 test_expect_success 'run diff with -B (#3)' '
git diff-index -B reference >current &&
-   cat >expect <<-\EOF &&
-   :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 
6ff87c4664981e4397625791c8ea3bbb5f2279a3 M100   file0
-   :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 
548142c327a6790ff8821d67c2ee1eff7a656b52 M100   file1
+   cat >expect <<-EOF &&
+   :100644 100644 $blob0_id $blob1_id M100 file0
+   :100644 100644 $blob1_id $blob0_id M100 file1
EOF
compare_diff_raw expect current
 '
 
 test_expect_success 'run diff with -B and -M (#4)' '
git diff-index -B -M reference >current &&
-   cat >expect <<-\EOF &&
-   :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 
6ff87c4664981e4397625791c8ea3bbb5f2279a3 R100   file1   file0
-   :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 
548142c327a6790ff8821d67c2ee1eff7a656b52 R100   file0   file1
+   cat >expect <<-EOF &&
+   :100644 100644 $blob1_id $blob1_id R100 file1   file0
+   :100644 100644 $blob0_id $blob0_id R100 file0   file1
EOF
compare_diff_raw expect current
 '
@@ -85,14 +88,15 @@ test_expect_success 'run diff with -B and -M (#4)' '
 test_expect_success 'make file0 into something completely different' '
rm -f file0 &&
test_ln_s_add frotz file0 &&
+   slink_id=$(printf frotz | git hash-object --stdin) &&
git update-index file1
 '
 
 test_expect_success 'run diff with -B (#5)' '
git diff-index -B reference >current &&
-   cat >expect <<-\EOF &&
-   :100644 12 548142c327a6790ff8821d67c2ee1eff7a656b52 
67be421f88824578857624f7b3dc75e99a8a1481 T  file0
-   :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 
548142c327a6790ff8821d67c2ee1eff7a656b52 M100   file1
+   cat >expect <<-EOF &&
+   :100644 12 $blob0_id $slink_id Tfile0
+   :100644 100644 $blob1_id $blob0_id M100 file1
EOF
compare_diff_raw expect current
 '
@@ -103,9 +107,9 @@ test_expect_success 'run diff with -B -M (#6)' '
# file0 changed from regular to symlink.  file1 is the same as the 
preimage
# of file0.  Because the change does not make file0 disappear, file1 is
# denoted as a copy of file0
-   cat >expect <<-\EOF &&
-   :100644 12 548142c327a6790ff8821d67c2ee1eff7a656b52 
67be421f88824578857624f7b3dc75e99a8a1481 T  file0
-   :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 
548142c327a6790ff8821d67c2ee1eff7a656b52 C  file0   file1
+   cat >expect <<-EOF &&
+   :100644 12 $blob0_id $slink_id Tfile0
+   :100644 100644 $blob0_id $blob0_id Cfile0   file1
EOF
compare_diff_raw expect current
 '
@@ -115,9 +119,9 @@ test_expect_success 'run diff with -M 

[PATCH v3 25/28] t4042: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4042-diff-textconv-caching.sh | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 04a44d5c61..bf33aedf4b 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -15,9 +15,13 @@ test_expect_success 'setup' '
echo bar content 1 >bar.bin &&
git add . &&
git commit -m one &&
+   foo1=$(git rev-parse --short HEAD:foo.bin) &&
+   bar1=$(git rev-parse --short HEAD:bar.bin) &&
echo foo content 2 >foo.bin &&
echo bar content 2 >bar.bin &&
git commit -a -m two &&
+   foo2=$(git rev-parse --short HEAD:foo.bin) &&
+   bar2=$(git rev-parse --short HEAD:bar.bin) &&
echo "*.bin diff=magic" >.gitattributes &&
git config diff.magic.textconv ./helper &&
git config diff.magic.cachetextconv true
@@ -25,14 +29,14 @@ test_expect_success 'setup' '
 
 cat >expect 

[PATCH v3 22/28] t4030: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4030-diff-textconv.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index aad6c7f78d..4cb9f0e523 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -148,7 +148,8 @@ test_expect_success 'diffstat does not run textconv' '
 # restore working setup
 echo file diff=foo >.gitattributes
 
-cat >expect.typechange <<'EOF'
+symlink=$(git rev-parse --short $(printf frotz | git hash-object --stdin))
+cat >expect.typechange 

[PATCH v3 24/28] t4205: sort log output in a hash-independent way

[brian m. carlson]

This test enumerates log entries and then sorts them.  For SHA-1, this
produces results that happen to sort in the order specified in the test,
but for other hash algorithms they sort differently.  Ensure we sort the
log entries in a hash-independent way by sorting on the ref name instead
of the object ID.

Signed-off-by: brian m. carlson 
---
 t/t4205-log-pretty-formats.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 591f35daaf..2052cadb11 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -516,22 +516,22 @@ test_expect_success 'log decoration properly follows tag 
chain' '
git commit --amend -m shorter &&
git log --no-walk --tags --pretty="%H %d" --decorate=full >actual &&
cat <<-EOF >expected &&
-   $head1  (tag: refs/tags/tag2)
$head2  (tag: refs/tags/message-one)
$old_head1  (tag: refs/tags/message-two)
+   $head1  (tag: refs/tags/tag2)
EOF
-   sort actual >actual1 &&
+   sort -k3 actual >actual1 &&
test_cmp expected actual1
 '
 
 test_expect_success 'clean log decoration' '
git log --no-walk --tags --pretty="%H %D" --decorate=full >actual &&
cat >expected <<-EOF &&
-   $head1 tag: refs/tags/tag2
$head2 tag: refs/tags/message-one
$old_head1 tag: refs/tags/message-two
+   $head1 tag: refs/tags/tag2
EOF
-   sort actual >actual1 &&
+   sort -k3 actual >actual1 &&
test_cmp expected actual1
 '
 

[PATCH v3 28/28] t5300: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for object IDs instead of
using hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t5300-pack-object.sh | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 65ff60f2ee..9e66637a19 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -466,9 +466,11 @@ test_expect_success !PTHREADS,C_LOCALE_OUTPUT 
'pack-objects --threads=N or pack.
 
 test_expect_success \
 'fake a SHA1 hash collision' \
-'test -f   .git/objects/c8/2de19312b6c3695c0c18f70709a6c535682a67 &&
- cp -f .git/objects/9d/235ed07cd19811a6ceb342de82f190e49c9f68 \
-   .git/objects/c8/2de19312b6c3695c0c18f70709a6c535682a67'
+'long_a=$(git hash-object a | sed -e "s!^..!&/!") &&
+ long_b=$(git hash-object b | sed -e "s!^..!&/!") &&
+ test -f   .git/objects/$long_b &&
+ cp -f .git/objects/$long_a \
+   .git/objects/$long_b'
 
 test_expect_success \
 'make sure index-pack detects the SHA1 collision' \

[PATCH v3 26/28] t4045: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4045-diff-relative.sh | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 6471a68701..36f8ed8a81 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -8,6 +8,7 @@ test_expect_success 'setup' '
echo content >file1 &&
mkdir subdir &&
echo other content >subdir/file2 &&
+   blob=$(git hash-object subdir/file2) &&
git add . &&
git commit -m one
 '
@@ -17,10 +18,11 @@ check_diff () {
shift
expect=$1
shift
+   short_blob=$(git rev-parse --short $blob)
cat >expected <<-EOF
diff --git a/$expect b/$expect
new file mode 100644
-   index 000..25c05ef
+   index 000..$short_blob
--- /dev/null
+++ b/$expect
@@ -0,0 +1 @@
@@ -68,7 +70,7 @@ check_raw () {
expect=$1
shift
cat >expected <<-EOF
-   :00 100644  
25c05ef3639d2d270e7fe765a67668f098092bc5 A  $expect
+   :00 100644  $blob A $expect
EOF
test_expect_success "--raw $*" "
git -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual &&

[PATCH v3 23/28] t/lib-diff-alternative: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test code so that it computes variables for blobs instead of
using hard-coded hashes.  This makes t4033 and t4050 (the patience and
histogram tests) pass.

Signed-off-by: brian m. carlson 
---
 t/lib-diff-alternative.sh | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/lib-diff-alternative.sh b/t/lib-diff-alternative.sh
index 8b4dbf22d2..8d1e408bb5 100644
--- a/t/lib-diff-alternative.sh
+++ b/t/lib-diff-alternative.sh
@@ -59,9 +59,11 @@ int main(int argc, char **argv)
 }
 EOF
 
-   cat >expect <<\EOF
+   file1=$(git rev-parse --short $(git hash-object file1))
+   file2=$(git rev-parse --short $(git hash-object file2))
+   cat >expect expect <

[PATCH v3 27/28] t4208: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for object IDs instead of
using hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4208-log-magic-pathspec.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
index a1705f70cf..62f335b2d9 100755
--- a/t/t4208-log-magic-pathspec.sh
+++ b/t/t4208-log-magic-pathspec.sh
@@ -45,8 +45,9 @@ test_expect_success 'git log -- :' '
 '
 
 test_expect_success 'git log HEAD -- :/' '
+   initial=$(git rev-parse --short HEAD^) &&
cat >expected <<-EOF &&
-   24b24cf initial
+   $initial initial
EOF
(cd sub && git log --oneline HEAD -- :/ >../actual) &&
test_cmp expected actual

[PATCH v3 17/28] t4014: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes values for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4014-format-patch.sh | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index dac3f349a3..349029f43b 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -578,7 +578,11 @@ test_expect_success 'excessive subject' '
 
rm -rf patches/ &&
git checkout side &&
+   before=$(git hash-object file) &&
+   before=$(git rev-parse --short $before) &&
for i in 5 6 1 2 3 A 4 B C 7 8 9 10 D E F; do echo "$i"; done >>file &&
+   after=$(git hash-object file) &&
+   after=$(git rev-parse --short $after) &&
git update-index file &&
git commit -m "This is an excessively long subject line for a message 
due to the habit some projects have of not having a short, one-line subject at 
the start of the commit message, but rather sticking a whole paragraph right at 
the start as the only thing in the commit message. It had better not become the 
filename for the patch." &&
git format-patch -o patches/ master..side &&
@@ -586,7 +590,6 @@ test_expect_success 'excessive subject' '
 '
 
 test_expect_success 'cover-letter inherits diff options' '
-
git mv file foo &&
git commit -m foo &&
git format-patch --no-renames --cover-letter -1 &&
@@ -616,7 +619,7 @@ test_expect_success 'shortlog of cover-letter wraps 
overly-long onelines' '
 '
 
 cat > expect << EOF
-index 40f36c6..2dc5c23 100644
+index $before..$after 100644
 --- a/file
 +++ b/file
 @@ -13,4 +13,20 @@ C
@@ -640,7 +643,7 @@ test_expect_success 'format-patch respects -U' '
 cat > expect << EOF
 
 diff --git a/file b/file
-index 40f36c6..2dc5c23 100644
+index $before..$after 100644
 --- a/file
 +++ b/file
 @@ -14,3 +14,19 @@ C

[PATCH v3 00/28] Hash-independent tests

[brian m. carlson]

This is part 2 in the series to make tests hash independent.

This series introduces an SHA1 prerequisite which checks if the hash in
use is SHA-1, and can be used to skip the test if it is not.
Additionally, because NewHash will be 256-bit, I introduced aliases for
the test constants $_x40 and $_z40 which will be less confusing when the
hash isn't 40 hex characters long.  I opted to leave the old names in
place for the moment to prevent any potential conflicts with other
series and will clean up any stragglers later.

This version addresses the concerns raised about robustness in case git
hash-object fails in an unexpected way.  We now have better error
handling for that case by performing git rev-parse and git hash-object
in separate steps.

Changes from v2:
* Split out git rev-parse --short and git hash-object into separate
  stages for better error handling.

Changes from v1:
* Amend commit message to indicate that we *will* be updating tests
  annotated with the SHA1 prerequisite to work with NewHash.
* Rename FULL_HEX to OID_REGEX.
* Regenerate patch for OID_REGEX.
* Update variable name from "link_oid" to "slink_id" for consistency
  while still preserving alignment.
* Restore blank line between tests.

tbdiff output below.

brian m. carlson (28):
  t/test-lib: add an SHA1 prerequisite
  t/test-lib: introduce ZERO_OID
  t: switch $_z40 to $ZERO_OID
  t/test-lib: introduce OID_REGEX
  t: switch $_x40 to $OID_REGEX
  t: annotate with SHA1 prerequisite
  t1007: annotate with SHA1 prerequisite
  t1512: skip test if not using SHA-1
  t4044: skip test if not using SHA-1
  t: skip pack tests if not using SHA-1
  t2203: abstract away SHA-1-specific constants
  t3103: abstract away SHA-1-specific constants
  t3702: abstract away SHA-1-specific constants
  t3905: abstract away SHA-1-specific constants
  t4007: abstract away SHA-1-specific constants
  t4008: abstract away SHA-1-specific constants
  t4014: abstract away SHA-1-specific constants
  t4020: abstract away SHA-1-specific constants
  t4022: abstract away SHA-1-specific constants
  t4029: fix test indentation
  t4029: abstract away SHA-1-specific constants
  t4030: abstract away SHA-1-specific constants
  t/lib-diff-alternative: abstract away SHA-1-specific constants
  t4205: sort log output in a hash-independent way
  t4042: abstract away SHA-1-specific constants
  t4045: abstract away SHA-1-specific constants
  t4208: abstract away SHA-1-specific constants
  t5300: abstract away SHA-1-specific constants

 t/diff-lib.sh   |  4 +-
 t/lib-diff-alternative.sh   | 12 --
 t/t-basic.sh| 24 ++--
 t/t0090-cache-tree.sh   |  2 +-
 t/t1000-read-tree-m-3way.sh |  2 +-
 t/t1001-read-tree-m-2way.sh |  2 +-
 t/t1002-read-tree-m-u-2way.sh   |  2 +-
 t/t1006-cat-file.sh |  8 ++--
 t/t1007-hash-object.sh  | 16 
 t/t1012-read-tree-df.sh |  2 +-
 t/t1400-update-ref.sh   |  2 +-
 t/t1407-worktree-ref-store.sh   |  8 ++--
 t/t1450-fsck.sh |  4 +-
 t/t1501-work-tree.sh|  6 +--
 t/t1512-rev-parse-disambiguation.sh |  6 +++
 t/t1601-index-bogus.sh  |  2 +-
 t/t1700-split-index.sh  |  2 +-
 t/t2011-checkout-invalid-head.sh|  2 +-
 t/t2025-worktree-add.sh |  8 ++--
 t/t2027-worktree-list.sh|  2 +-
 t/t2107-update-index-basic.sh   |  4 +-
 t/t2201-add-update-typechange.sh| 16 
 t/t2203-add-intent.sh   | 14 +++
 t/t3100-ls-tree-restrict.sh |  2 +-
 t/t3101-ls-tree-dirname.sh  |  2 +-
 t/t3103-ls-tree-misc.sh |  3 +-
 t/t3200-branch.sh   |  4 +-
 t/t3510-cherry-pick-sequence.sh |  8 ++--
 t/t3702-add-edit.sh |  7 ++--
 t/t3905-stash-include-untracked.sh  | 11 --
 t/t4002-diff-basic.sh   |  2 +-
 t/t4006-diff-mode.sh|  2 +-
 t/t4007-rename-3.sh | 17 +
 t/t4008-diff-break-rewrite.sh   | 59 -
 t/t4014-format-patch.sh | 13 ---
 t/t4020-diff-external.sh| 20 ++
 t/t4022-diff-rewrite.sh |  6 ++-
 t/t4027-diff-submodule.sh   |  6 +--
 t/t4029-diff-trailing-space.sh  | 40 ++-
 t/t4030-diff-textconv.sh|  5 ++-
 t/t4042-diff-textconv-caching.sh| 16 +---
 t/t4044-diff-index-unique-abbrev.sh |  6 +++
 t/t4045-diff-relative.sh|  6 ++-
 t/t4046-diff-unmerged.sh| 14 +++
 t/t4054-diff-bogus-tree.sh  | 12 +++---
 t/t4058-diff-duplicates.sh  | 12 +++---
 t/t4150-am.sh   |  4 +-
 t/t4200-rerere.sh   |  2 +-
 t/t4201-shortlog.sh |  2 +-
 t/t4205-log-pretty-formats.sh   |  8 ++--
 t/t4208-log-magic-pathspec.sh   |  3 +-
 t/t5150-request-pull.sh |  2 +-
 

[PATCH v3 07/28] t1007: annotate with SHA1 prerequisite

[brian m. carlson]

Since this is a core test that tests basic functionality, annotate the
assertions that have dependencies on SHA-1 with the appropriate
prerequisite.

Signed-off-by: brian m. carlson 
---
 t/t1007-hash-object.sh | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 532682f51c..a37753047e 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -9,13 +9,13 @@ echo_without_newline() {
 }
 
 test_blob_does_not_exist() {
-   test_expect_success 'blob does not exist in database' "
+   test_expect_success SHA1 'blob does not exist in database' "
test_must_fail git cat-file blob $1
"
 }
 
 test_blob_exists() {
-   test_expect_success 'blob exists in database' "
+   test_expect_success SHA1 'blob exists in database' "
git cat-file blob $1
"
 }
@@ -73,19 +73,19 @@ test_expect_success "Can't use --path with --no-filters" '
 
 push_repo
 
-test_expect_success 'hash a file' '
+test_expect_success SHA1 'hash a file' '
test $hello_sha1 = $(git hash-object hello)
 '
 
 test_blob_does_not_exist $hello_sha1
 
-test_expect_success 'hash from stdin' '
+test_expect_success SHA1 'hash from stdin' '
test $example_sha1 = $(git hash-object --stdin < example)
 '
 
 test_blob_does_not_exist $example_sha1
 
-test_expect_success 'hash a file and write to database' '
+test_expect_success SHA1 'hash a file and write to database' '
test $hello_sha1 = $(git hash-object -w hello)
 '
 
@@ -161,7 +161,7 @@ pop_repo
 for args in "-w --stdin" "--stdin -w"; do
push_repo
 
-   test_expect_success "hash from stdin and write to database ($args)" '
+   test_expect_success SHA1 "hash from stdin and write to database 
($args)" '
test $example_sha1 = $(git hash-object $args < example)
'
 
@@ -176,14 +176,14 @@ example"
 sha1s="$hello_sha1
 $example_sha1"
 
-test_expect_success "hash two files with names on stdin" '
+test_expect_success SHA1 "hash two files with names on stdin" '
test "$sha1s" = "$(echo_without_newline "$filenames" | git hash-object 
--stdin-paths)"
 '
 
 for args in "-w --stdin-paths" "--stdin-paths -w"; do
push_repo
 
-   test_expect_success "hash two files with names on stdin and write to 
database ($args)" '
+   test_expect_success SHA1 "hash two files with names on stdin and write 
to database ($args)" '
test "$sha1s" = "$(echo_without_newline "$filenames" | git 
hash-object $args)"
'
 

[PATCH v3 02/28] t/test-lib: introduce ZERO_OID

[brian m. carlson]

Currently we have a variable, $_z40, which contains the all-zero object
ID.  However, with NewHash, we'll have an all-zero object ID which is
longer than 40 hex characters.  In such a case, $_z40 will be a
confusing name.  Create a $ZERO_OID variable which will always reflect
the all-zeros object ID, regardless of the length of the current hash.

Signed-off-by: brian m. carlson 
---
 t/test-lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index fce728d2ea..58c2ea52c6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -184,6 +184,7 @@ _x40="$_x35$_x05"
 # Zero SHA-1
 _z40=
 
+ZERO_OID=$_z40
 EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
 EMPTY_BLOB=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
 
@@ -195,7 +196,7 @@ LF='
 # when case-folding filenames
 u200c=$(printf '\342\200\214')
 
-export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB
+export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB ZERO_OID
 
 # Each test should start with something like this, after copyright notices:
 #

[PATCH v3 10/28] t: skip pack tests if not using SHA-1

[brian m. carlson]

These tests rely on creating packs with specially named objects which
are necessarily dependent on the hash used.  Skip these tests if we're
not using SHA-1.

Signed-off-by: brian m. carlson 
---
 t/t5308-pack-detect-duplicates.sh | 6 ++
 t/t5309-pack-delta-cycles.sh  | 6 ++
 2 files changed, 12 insertions(+)

diff --git a/t/t5308-pack-detect-duplicates.sh 
b/t/t5308-pack-detect-duplicates.sh
index 156ae9e9d3..6845c1f3c3 100755
--- a/t/t5308-pack-detect-duplicates.sh
+++ b/t/t5308-pack-detect-duplicates.sh
@@ -4,6 +4,12 @@ test_description='handling of duplicate objects in incoming 
packfiles'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-pack.sh
 
+if ! test_have_prereq SHA1
+then
+   skip_all='not using SHA-1 for objects'
+   test_done
+fi
+
 # The sha1s we have in our pack. It's important that these have the same
 # starting byte, so that they end up in the same fanout section of the index.
 # That lets us make sure we are exercising the binary search with both sets.
diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh
index 3e7861b075..491556dad9 100755
--- a/t/t5309-pack-delta-cycles.sh
+++ b/t/t5309-pack-delta-cycles.sh
@@ -4,6 +4,12 @@ test_description='test index-pack handling of delta cycles in 
packfiles'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-pack.sh
 
+if ! test_have_prereq SHA1
+then
+   skip_all='not using SHA-1 for objects'
+   test_done
+fi
+
 # Two similar-ish objects that we have computed deltas between.
 A=01d7713666f4de822776c7622c10f1b07de280dc
 B=e68fe8129b546b101aee9510c5328e7f21ca1d18

[PATCH v3 04/28] t/test-lib: introduce OID_REGEX

[brian m. carlson]

Currently we have a variable, $_x40, which contains a regex that matches
a full 40-character hex constant.  However, with NewHash, we'll have
object IDs that are longer than 40 characters.  In such a case, $_x40
will be a confusing name.  Create a $OID_REGEX variable which will
always reflect a regex matching the appropriate object ID, regardless of
the length of the current hash.

Signed-off-by: brian m. carlson 
---
 t/test-lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 58c2ea52c6..fed21c3dfc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -184,6 +184,7 @@ _x40="$_x35$_x05"
 # Zero SHA-1
 _z40=
 
+OID_REGEX="$_x40"
 ZERO_OID=$_z40
 EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
 EMPTY_BLOB=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
@@ -196,7 +197,7 @@ LF='
 # when case-folding filenames
 u200c=$(printf '\342\200\214')
 
-export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB ZERO_OID
+export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB ZERO_OID OID_REGEX
 
 # Each test should start with something like this, after copyright notices:
 #

[PATCH v3 03/28] t: switch $_z40 to $ZERO_OID

[brian m. carlson]

Switch all uses of $_z40 to $ZERO_OID so that they work correctly with
larger hashes.  This commit was created by using the following sed
command to modify all files in the t directory except t/test-lib.sh:

  sed -i 's/\$_z40/$ZERO_OID/g'

Signed-off-by: brian m. carlson 
---
 t/t1006-cat-file.sh|  8 ++---
 t/t1400-update-ref.sh  |  2 +-
 t/t1407-worktree-ref-store.sh  |  8 ++---
 t/t1450-fsck.sh|  4 +--
 t/t1501-work-tree.sh   |  6 ++--
 t/t1601-index-bogus.sh |  2 +-
 t/t1700-split-index.sh |  2 +-
 t/t2011-checkout-invalid-head.sh   |  2 +-
 t/t2025-worktree-add.sh|  8 ++---
 t/t2027-worktree-list.sh   |  2 +-
 t/t2107-update-index-basic.sh  |  4 +--
 t/t2201-add-update-typechange.sh   | 16 -
 t/t2203-add-intent.sh  |  6 ++--
 t/t3200-branch.sh  |  4 +--
 t/t4002-diff-basic.sh  |  2 +-
 t/t4014-format-patch.sh|  2 +-
 t/t4020-diff-external.sh   | 10 +++---
 t/t4027-diff-submodule.sh  |  6 ++--
 t/t4046-diff-unmerged.sh   | 14 
 t/t4054-diff-bogus-tree.sh | 12 +++
 t/t4058-diff-duplicates.sh | 12 +++
 t/t4150-am.sh  |  4 +--
 t/t4200-rerere.sh  |  2 +-
 t/t5516-fetch-push.sh  | 22 ++--
 t/t5527-fetch-odd-refs.sh  |  2 +-
 t/t5571-pre-push-hook.sh   |  8 ++---
 t/t6120-describe.sh|  2 +-
 t/t6300-for-each-ref.sh|  2 +-
 t/t6301-for-each-ref-errors.sh |  2 +-
 t/t7009-filter-branch-null-sha1.sh |  2 +-
 t/t7011-skip-worktree-reading.sh   |  2 +-
 t/t7064-wtstatus-pv2.sh| 58 +++---
 32 files changed, 119 insertions(+), 119 deletions(-)

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 2ac3b940c6..13dd510b2e 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -236,8 +236,8 @@ test_expect_success "--batch-check for an empty line" '
 '
 
 test_expect_success 'empty --batch-check notices missing object' '
-   echo "$_z40 missing" >expect &&
-   echo "$_z40" | git cat-file --batch-check="" >actual &&
+   echo "$ZERO_OID missing" >expect &&
+   echo "$ZERO_OID" | git cat-file --batch-check="" >actual &&
test_cmp expect actual
 '
 
@@ -294,8 +294,8 @@ test_expect_success 'setup blobs which are likely to delta' 
'
 
 test_expect_success 'confirm that neither loose blob is a delta' '
cat >expect <<-EOF &&
-   $_z40
-   $_z40
+   $ZERO_OID
+   $ZERO_OID
EOF
git cat-file --batch-check="%(deltabase)" actual &&
test_cmp expect actual
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 664a3a4e4e..f6dc05654e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -6,7 +6,7 @@
 test_description='Test git update-ref and basic ref logging'
 . ./test-lib.sh
 
-Z=$_z40
+Z=$ZERO_OID
 
 m=refs/heads/master
 n_dir=refs/heads/gu
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index 2211f9831f..4623ae15c4 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -50,13 +50,13 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' 
'
 '
 
 test_expect_success 'for_each_reflog()' '
-   echo $_z40 > .git/logs/PSEUDO-MAIN &&
+   echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
mkdir -p .git/logs/refs/bisect &&
-   echo $_z40 > .git/logs/refs/bisect/random &&
+   echo $ZERO_OID > .git/logs/refs/bisect/random &&
 
-   echo $_z40 > .git/worktrees/wt/logs/PSEUDO-WT &&
+   echo $ZERO_OID > .git/worktrees/wt/logs/PSEUDO-WT &&
mkdir -p .git/worktrees/wt/logs/refs/bisect &&
-   echo $_z40 > .git/worktrees/wt/logs/refs/bisect/wt-random &&
+   echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
 
$RWT for-each-reflog | cut -c 42- | sort >actual &&
cat >expected <<-\EOF &&
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index cb4b66e29d..91fd71444d 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -713,7 +713,7 @@ test_expect_success 'fsck notices dangling objects' '
 
 test_expect_success 'fsck $name notices bogus $name' '
test_must_fail git fsck bogus &&
-   test_must_fail git fsck $_z40
+   test_must_fail git fsck $ZERO_OID
 '
 
 test_expect_success 'bogus head does not fallback to all heads' '
@@ -723,7 +723,7 @@ test_expect_success 'bogus head does not fallback to all 
heads' '
blob=$(git rev-parse :foo) &&
test_when_finished "git rm --cached foo" &&
remove_object $blob &&
-   test_must_fail git fsck $_z40 >out 2>&1 &&
+   test_must_fail git fsck $ZERO_OID >out 2>&1 &&
! grep $blob out
 '
 
diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index 9c0bc65250..afcdfafe45 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ 

[PATCH v3 09/28] t4044: skip test if not using SHA-1

[brian m. carlson]

This test relies on objects with colliding short names which are
necessarily dependent on the hash used.  Skip the test if we're not
using SHA-1.

Signed-off-by: brian m. carlson 
---
 t/t4044-diff-index-unique-abbrev.sh | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/t/t4044-diff-index-unique-abbrev.sh 
b/t/t4044-diff-index-unique-abbrev.sh
index d5ce72be63..647905e01f 100755
--- a/t/t4044-diff-index-unique-abbrev.sh
+++ b/t/t4044-diff-index-unique-abbrev.sh
@@ -3,6 +3,12 @@
 test_description='test unique sha1 abbreviation on "index from..to" line'
 . ./test-lib.sh
 
+if ! test_have_prereq SHA1
+then
+   skip_all='not using SHA-1 for objects'
+   test_done
+fi
+
 cat >expect_initial <

[PATCH v3 01/28] t/test-lib: add an SHA1 prerequisite

[brian m. carlson]

There are some basic tests in our codebase that test that we get fixed
SHA-1 values.  These are valuable because they make sure that our SHA-1
implementation is free of bugs, but obviously these tests will fail with
a different hash.

There are also tests which intentionally produce objects that have
collisions when truncated to a certain length to test our handling of
these cases.  These tests, too, will fail with a different hash.

Add an SHA1 prerequisite to annotate both of these types of tests and
disable them when we're using a different hash.  In the future, we will
create versions of these tests which handle both SHA-1 and NewHash.

Signed-off-by: brian m. carlson 
---
 t/test-lib.sh | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ea2bbaaa7a..fce728d2ea 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1212,3 +1212,10 @@ test_lazy_prereq TIME_T_IS_64BIT 'test-tool date 
time_t-is64bit'
 test_lazy_prereq CURL '
curl --version
 '
+
+# SHA1 is a test if the hash algorithm in use is SHA-1.  This is both for tests
+# which will not work with other hash algorithms and tests that work but don't
+# test anything meaningful (e.g. special values which cause short collisions).
+test_lazy_prereq SHA1 '
+   test $(git hash-object /dev/null) = 
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
+'

[PATCH v3 21/28] t4029: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t4029-diff-trailing-space.sh | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/t4029-diff-trailing-space.sh b/t/t4029-diff-trailing-space.sh
index f4e18cb8d3..32b6e9a4e7 100755
--- a/t/t4029-diff-trailing-space.sh
+++ b/t/t4029-diff-trailing-space.sh
@@ -6,7 +6,7 @@ test_description='diff honors config option, 
diff.suppressBlankEmpty'
 
 . ./test-lib.sh
 
-cat <<\EOF > exp ||
+cat <<\EOF >expected ||
 diff --git a/f b/f
 index 5f6a263..8cb8bae 100644
 --- a/f
@@ -20,9 +20,14 @@ exit 1
 
 test_expect_success "$test_description" '
printf "\nx\n" > f &&
+   before=$(git hash-object f) &&
+   before=$(git rev-parse --short $before) &&
git add f &&
git commit -q -m. f &&
printf "\ny\n" > f &&
+   after=$(git hash-object f) &&
+   after=$(git rev-parse --short $after) &&
+   sed -e "s/^index .*/index $before..$after 100644/" expected >exp &&
git config --bool diff.suppressBlankEmpty true &&
git diff f > actual &&
test_cmp exp actual &&

[PATCH v3 05/28] t: switch $_x40 to $OID_REGEX

[brian m. carlson]

Switch all uses of $_x40 to $OID_REGEX so that they work correctly with
larger hashes.  This commit was created by using the following sed
command to modify all files in the t directory except t/test-lib.sh:

  sed -i 's/\$_x40/$OID_REGEX/g'

Signed-off-by: brian m. carlson 
---
 t/diff-lib.sh   |  4 ++--
 t/t0090-cache-tree.sh   |  2 +-
 t/t1000-read-tree-m-3way.sh |  2 +-
 t/t1001-read-tree-m-2way.sh |  2 +-
 t/t1002-read-tree-m-u-2way.sh   |  2 +-
 t/t1012-read-tree-df.sh |  2 +-
 t/t3100-ls-tree-restrict.sh |  2 +-
 t/t3101-ls-tree-dirname.sh  |  2 +-
 t/t3510-cherry-pick-sequence.sh |  8 
 t/t4002-diff-basic.sh   |  2 +-
 t/t4006-diff-mode.sh|  2 +-
 t/t4014-format-patch.sh |  2 +-
 t/t4201-shortlog.sh |  2 +-
 t/t5150-request-pull.sh |  2 +-
 t/t6006-rev-list-format.sh  |  4 ++--
 t/t6012-rev-list-simplify.sh|  2 +-
 t/t6111-rev-list-treesame.sh|  2 +-
 t/t7506-status-submodule.sh |  2 +-
 t/t9010-svn-fe.sh   | 14 +++---
 t/t9300-fast-import.sh  |  6 +++---
 20 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/t/diff-lib.sh b/t/diff-lib.sh
index c211dc40ee..2de880f7a5 100644
--- a/t/diff-lib.sh
+++ b/t/diff-lib.sh
@@ -1,6 +1,6 @@
 :
 
-sanitize_diff_raw='/^:/s/ '"\($_x40\)"' '"\($_x40\)"' \([A-Z]\)[0-9]*  / \1 \2 
\3# /'
+sanitize_diff_raw='/^:/s/ '"\($OID_REGEX\)"' '"\($OID_REGEX\)"' 
\([A-Z]\)[0-9]*/ \1 \2 \3# /'
 compare_diff_raw () {
 # When heuristics are improved, the score numbers would change.
 # Ignore them while comparing.
@@ -12,7 +12,7 @@ compare_diff_raw () {
 test_cmp .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
 }
 
-sanitize_diff_raw_z='/^:/s/ '"$_x40"' '"$_x40"' \([A-Z]\)[0-9]*$/ X X \1#/'
+sanitize_diff_raw_z='/^:/s/ '"$OID_REGEX"' '"$OID_REGEX"' \([A-Z]\)[0-9]*$/ X 
X \1#/'
 compare_diff_raw_z () {
 # When heuristics are improved, the score numbers would change.
 # Ignore them while comparing.
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 4ae0995cd9..0c61268fd2 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -9,7 +9,7 @@ cache-tree extension.
 
 cmp_cache_tree () {
test-tool dump-cache-tree | sed -e '/#(ref)/d' >actual &&
-   sed "s/$_x40/SHA/" filtered &&
+   sed "s/$OID_REGEX/SHA/" filtered &&
test_cmp "$1" filtered
 }
 
diff --git a/t/t1000-read-tree-m-3way.sh b/t/t1000-read-tree-m-3way.sh
index 3c4d2d6045..013c5a7bc3 100755
--- a/t/t1000-read-tree-m-3way.sh
+++ b/t/t1000-read-tree-m-3way.sh
@@ -128,7 +128,7 @@ cat >expected <<\EOF
 EOF
 
 check_result () {
-   git ls-files --stage | sed -e 's/ '"$_x40"' / X /' >current &&
+   git ls-files --stage | sed -e 's/ '"$OID_REGEX"' / X /' >current &&
test_cmp expected current
 }
 
diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 5ededd8e40..1057a96b24 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -30,7 +30,7 @@ read_tree_twoway () {
 compare_change () {
sed -n >current \
-e '/^--- /d; /^+++ /d; /^@@ /d;' \
-   -e 's/^\([-+][0-7][0-7][0-7][0-7][0-7][0-7]\) '"$_x40"' /\1 X /p' \
+   -e 's/^\([-+][0-7][0-7][0-7][0-7][0-7][0-7]\) '"$OID_REGEX"' /\1 X 
/p' \
"$1"
test_cmp expected current
 }
diff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh
index 7ca2e65d10..9c05f5e1f5 100755
--- a/t/t1002-read-tree-m-u-2way.sh
+++ b/t/t1002-read-tree-m-u-2way.sh
@@ -16,7 +16,7 @@ compare_change () {
-e '1{/^diff --git /d;}' \
-e '2{/^index /d;}' \
-e '/^--- /d; /^+++ /d; /^@@ /d;' \
-   -e 's/^\(.[0-7][0-7][0-7][0-7][0-7][0-7]\) '"$_x40"' /\1 X /' "$1"
+   -e 's/^\(.[0-7][0-7][0-7][0-7][0-7][0-7]\) '"$OID_REGEX"' /\1 X /' 
"$1"
test_cmp expected current
 }
 
diff --git a/t/t1012-read-tree-df.sh b/t/t1012-read-tree-df.sh
index a6a04b6b90..57f0770df1 100755
--- a/t/t1012-read-tree-df.sh
+++ b/t/t1012-read-tree-df.sh
@@ -32,7 +32,7 @@ settree () {
 
 checkindex () {
git ls-files -s |
-   sed "s|^[0-7][0-7]* $_x40 \([0-3]\) |\1 |" >current &&
+   sed "s|^[0-7][0-7]* $OID_REGEX \([0-3]\)|\1 |" >current &&
cat >expect &&
test_cmp expect current
 }
diff --git a/t/t3100-ls-tree-restrict.sh b/t/t3100-ls-tree-restrict.sh
index 325114f8fe..18baf49a49 100755
--- a/t/t3100-ls-tree-restrict.sh
+++ b/t/t3100-ls-tree-restrict.sh
@@ -32,7 +32,7 @@ test_expect_success \
  echo $tree'
 
 test_output () {
-sed -e "s/ $_x40   / X /" check
+sed -e "s/ $OID_REGEX  / X /" check
 test_cmp expected check
 }
 
diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh
index 327ded4000..12bf31022a 100755
--- a/t/t3101-ls-tree-dirname.sh
+++ b/t/t3101-ls-tree-dirname.sh
@@ -40,7 +40,7 @@ test_expect_success 'setup' 

[PATCH v3 11/28] t2203: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t2203-add-intent.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 1797f946b9..04d840a544 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -27,8 +27,8 @@ test_expect_success 'git status' '
 
 test_expect_success 'git status with porcelain v2' '
git status --porcelain=v2 | grep -v "^?" >actual &&
-   nam1=d00491fd7e5bb6fa28c517a0bb32b8b506539d4d &&
-   nam2=ce013625030ba8dba906f756967f9e9ca394464a &&
+   nam1=$(echo 1 | git hash-object --stdin) &&
+   nam2=$(git hash-object elif) &&
cat >expect <<-EOF &&
1 DA N... 100644 00 100644 $nam1 $ZERO_OID 1.t
1 A. N... 00 100644 100644 $ZERO_OID $nam2 elif
@@ -181,7 +181,7 @@ test_expect_success 'rename detection finds the right 
names' '
EOF
test_cmp expected.2 actual.2 &&
 
-   hash=12f00e90b6ef79117ce6e650416b8cf517099b78 &&
+   hash=$(git hash-object third) &&
git status --porcelain=v2 | grep -v "^?" >actual.3 &&
cat >expected.3 <<-EOF &&
2 .R N... 100644 100644 100644 $hash $hash R100 third   first
@@ -212,7 +212,7 @@ test_expect_success 'double rename detection in status' '
EOF
test_cmp expected.2 actual.2 &&
 
-   hash=12f00e90b6ef79117ce6e650416b8cf517099b78 &&
+   hash=$(git hash-object third) &&
git status --porcelain=v2 | grep -v "^?" >actual.3 &&
cat >expected.3 <<-EOF &&
2 R. N... 100644 100644 100644 $hash $hash R100 second  first

[PATCH v3 14/28] t3905: abstract away SHA-1-specific constants

[brian m. carlson]

Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
 t/t3905-stash-include-untracked.sh | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/t/t3905-stash-include-untracked.sh 
b/t/t3905-stash-include-untracked.sh
index 3ea5b9bb3f..597b0637d1 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -35,24 +35,26 @@ test_expect_success 'stash save --include-untracked cleaned 
the untracked files'
test_cmp expect actual
 '
 
+tracked=$(git rev-parse --short $(echo 1 | git hash-object --stdin))
+untracked=$(git rev-parse --short $(echo untracked | git hash-object --stdin))
 cat > expect.diff < expect <

Re: [PATCH 3/3] Inform about fast-forwarding of submodules during merge

[Junio C Hamano]

Stefan Beller  writes:

> From: Leif Middelschulte 
>
> Inform the user about an automatically fast-forwarded submodule. The
> silent merge behavior was introduced by commit 68d03e4a6e44 ("Implement
> automatic fast-forward merge for submodules", 2010-07-07)).

Oh, another thing I forgot to mention.

These three lines do not convey much useful information.  The first
sentence can be read from the patch text, and the rest can be read
from "git blame" and "git log" output.

It is correct that the silent behaviour was introduced long time
ago.  The proposed log message does not even say if that silent
behaviour is bad in any way, let alone why it is bad and need to be
changed.

Perhaps Leif can elaborate why this change is a good idea in the
first place?

Thanks.

Re: [PATCH 3/3] Inform about fast-forwarding of submodules during merge

[Junio C Hamano]

Stefan Beller  writes:

> From: Leif Middelschulte 
>

Subject: merge-recursive: give notice when submodule commit gets fast-forwarded

perhaps?

>   /* Case #1: a is contained in b or vice versa */
>   if (in_merge_bases(commit_a, commit_b)) {
>   oidcpy(result, b);
> + if (show(o, 3)) {
> + output(o, 1, _("Fast-forwarding submodule %s to the 
> following commit:"), path);
> + output_commit_title(o, commit_b);
> + } else if (show(o, 2))
> + output(o, 2, _("Fast-forwarding submodule %s to %s"), 
> path, oid_to_hex(b));
> + else
> + ; /* no output */
> +

merge.verbosity::
Controls the amount of output shown by the recursive merge
strategy.  Level 0 outputs nothing except a final error
message if conflicts were detected. Level 1 outputs only
conflicts, 2 outputs conflicts and file changes.  Level 5 and
above outputs debugging information.  The default is level 2.
Can be overridden by the `GIT_MERGE_VERBOSITY` environment variable.

So, by default, we report the fact that we update submodule to a
particular commit, which is quite similar to how we report auto
merged paths using the content level 3-way merge; when you squint
your eyes, the "fast-forward" of submodules look somewhat like a
content-level 3-way merge anyway ;-)

And at level 3, which currently is used to report a non-event that
does not change the result of the merge from what was naturally
expected, we give a bit more detail by citing the commit the
submodule gets fast-forwarded to [*1*].

Sort of makes sense.


[Footnote]

*1* I wonder if that is really necessary, though---we do not give
"here is a diff" or "this is the new contents" after a path gets
merged for normal files.  And if it is needed perhaps because
submodules are so special, I wonder if we also need to give the
commit the submodule gets fast-forwarded from, i.e. the original
one, the same way.

Re: [PATCH 3/3] Inform about fast-forwarding of submodules during merge

[Elijah Newren]

On Tue, May 15, 2018 at 1:00 PM, Stefan Beller  wrote:
> From: Leif Middelschulte 
>
> Inform the user about an automatically fast-forwarded submodule. The
> silent merge behavior was introduced by commit 68d03e4a6e44 ("Implement
> automatic fast-forward merge for submodules", 2010-07-07)).
>
> Signed-off-by: Leif Middelschulte 
> Signed-off-by: Stefan Beller 
> ---
>  merge-recursive.c | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 0571919ee0a..29a430c418a 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1093,10 +1093,26 @@ static int merge_submodule(struct merge_options *o,
> /* Case #1: a is contained in b or vice versa */
> if (in_merge_bases(commit_a, commit_b)) {
> oidcpy(result, b);
> +   if (show(o, 3)) {
> +   output(o, 1, _("Fast-forwarding submodule %s to the 
> following commit:"), path);

Seems slightly odd to mix 3 and 1 here; although it'll work just fine,
I would have expected use of 3 in both places (much like you did with
the 2 and 2 below).

> +   output_commit_title(o, commit_b);
> +   } else if (show(o, 2))
> +   output(o, 2, _("Fast-forwarding submodule %s to %s"), 
> path, oid_to_hex(b));
> +   else
> +   ; /* no output */
> +
> return 1;
> }
> if (in_merge_bases(commit_b, commit_a)) {
> oidcpy(result, a);
> +   if (show(o, 3)) {
> +   output(o, 1, _("Fast-forwarding submodule %s to the 
> following commit:"), path);

Same.

> +   output_commit_title(o, commit_a);
> +   } else if (show(o, 2))
> +   output(o, 2, _("Fast-forwarding submodule %s to %s"), 
> path, oid_to_hex(a));
> +   else
> +   ; /* no output */
> +
> return 1;
> }
>
> --
> 2.17.0.582.gccdcbd54c44.dirty

Other than that nit-pick, looks good to me.

Re: [PATCH 2/3] merge-recursive: i18n submodule merge output and respect verbosity

[Junio C Hamano]

Stefan Beller  writes:

> +static int merge_submodule(struct merge_options *o,
> +struct object_id *result, const char *path,
>  const struct object_id *base, const struct object_id 
> *a,
> -const struct object_id *b, int search)
> +const struct object_id *b)
>  {
>   struct commit *commit_base, *commit_a, *commit_b;
>   int parent_count;
>   struct object_array merges;
>  
>   int i;
> + int search = !o->call_depth;

I kind of like this "while at it" change in this patch ;-)

Re: [PATCH 01/35] refspec: move refspec parsing logic into its own file

[Junio C Hamano]

Brandon Williams  writes:

> On 05/15, Junio C Hamano wrote:
>> Brandon Williams  writes:
>> 
>> > In preperation for performing a refactor on refspec related code, move
>> 
>> Preparation?
>
> Oops, I'll fix that.

Thanks.  And sorry for forcing you to page thru 600+ lines only to
find that I commented on only that line.  I'll try harder to make
sure I trim quotes from my responses.

Re: [PATCH] grep: handle corrupt index files early

[Junio C Hamano]

Stefan Beller  writes:

> Any other caller of 'repo_read_index' dies upon a negative return of
> it, so grep should, too.

Makes sense.  When the function returns a failure, there is no
sensible fallback action anyway, so dying here is alright.

Will queue; thanks.

>
> Signed-off-by: Stefan Beller 
> ---
>
> Found while reviewing the series
> https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/
>
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 6e7bc76785a..69f0743619f 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct 
> repository *repo,
>   strbuf_addstr(, repo->submodule_prefix);
>   }
>  
> - repo_read_index(repo);
> + if (repo_read_index(repo) < 0)
> + die("index file corrupt");
>  
>   for (nr = 0; nr < repo->index->cache_nr; nr++) {
>   const struct cache_entry *ce = repo->index->cache[nr];

[PATCH 04/19] commit: add repository argument to register_commit_graft

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow callers of register_commit_graft to
be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 builtin/blame.c | 3 ++-
 commit.c| 4 ++--
 commit.h| 3 ++-
 shallow.c   | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 0ffd1d443ea..7a07bff2423 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -8,6 +8,7 @@
 #include "cache.h"
 #include "config.h"
 #include "builtin.h"
+#include "repository.h"
 #include "commit.h"
 #include "diff.h"
 #include "revision.h"
@@ -491,7 +492,7 @@ static int read_ancestry(const char *graft_file)
/* The format is just "Commit Parent1 Parent2 ...\n" */
struct commit_graft *graft = read_graft_line();
if (graft)
-   register_commit_graft(graft, 0);
+   register_commit_graft(the_repository, graft, 0);
}
fclose(fp);
strbuf_release();
diff --git a/commit.c b/commit.c
index 2cd5b8a0b96..4e8d3488425 100644
--- a/commit.c
+++ b/commit.c
@@ -112,7 +112,7 @@ static int commit_graft_pos_the_repository(const unsigned 
char *sha1)
commit_graft_sha1_access);
 }
 
-int register_commit_graft(struct commit_graft *graft, int ignore_dups)
+int register_commit_graft_the_repository(struct commit_graft *graft, int 
ignore_dups)
 {
int pos = commit_graft_pos(the_repository, graft->oid.hash);
 
@@ -188,7 +188,7 @@ static int read_graft_file(const char *graft_file)
struct commit_graft *graft = read_graft_line();
if (!graft)
continue;
-   if (register_commit_graft(graft, 1))
+   if (register_commit_graft(the_repository, graft, 1))
error("duplicate graft data: %s", buf.buf);
}
fclose(fp);
diff --git a/commit.h b/commit.h
index 2d764ab7d8e..40a5b5e2585 100644
--- a/commit.h
+++ b/commit.h
@@ -174,7 +174,8 @@ struct commit_graft {
 typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
 
 struct commit_graft *read_graft_line(struct strbuf *line);
-int register_commit_graft(struct commit_graft *, int);
+#define register_commit_graft(r, g, i) register_commit_graft_##r(g, i)
+int register_commit_graft_the_repository(struct commit_graft *, int);
 struct commit_graft *lookup_commit_graft(const struct object_id *oid);
 
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit 
*rev2);
diff --git a/shallow.c b/shallow.c
index c2f81a5a5a8..ef802deed41 100644
--- a/shallow.c
+++ b/shallow.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "repository.h"
 #include "tempfile.h"
 #include "lockfile.h"
 #include "object-store.h"
@@ -38,7 +39,7 @@ int register_shallow(const struct object_id *oid)
graft->nr_parent = -1;
if (commit && commit->object.parsed)
commit->parents = NULL;
-   return register_commit_graft(graft, 0);
+   return register_commit_graft(the_repository, graft, 0);
 }
 
 int is_repository_shallow(void)
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 03/19] commit: add repository argument to commit_graft_pos

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow callers of commit_graft_pos to be
more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 commit.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/commit.c b/commit.c
index a0c9eb05c82..2cd5b8a0b96 100644
--- a/commit.c
+++ b/commit.c
@@ -104,7 +104,8 @@ static const unsigned char *commit_graft_sha1_access(size_t 
index, void *table)
return commit_graft_table[index]->oid.hash;
 }
 
-static int commit_graft_pos(const unsigned char *sha1)
+#define commit_graft_pos(r, s) commit_graft_pos_##r(s)
+static int commit_graft_pos_the_repository(const unsigned char *sha1)
 {
return sha1_pos(sha1, the_repository->parsed_objects->grafts,
the_repository->parsed_objects->grafts_nr,
@@ -113,7 +114,7 @@ static int commit_graft_pos(const unsigned char *sha1)
 
 int register_commit_graft(struct commit_graft *graft, int ignore_dups)
 {
-   int pos = commit_graft_pos(graft->oid.hash);
+   int pos = commit_graft_pos(the_repository, graft->oid.hash);
 
if (0 <= pos) {
if (ignore_dups)
@@ -213,7 +214,7 @@ struct commit_graft *lookup_commit_graft(const struct 
object_id *oid)
 {
int pos;
prepare_commit_graft();
-   pos = commit_graft_pos(oid->hash);
+   pos = commit_graft_pos(the_repository, oid->hash);
if (pos < 0)
return NULL;
return the_repository->parsed_objects->grafts[pos];
@@ -229,7 +230,7 @@ int for_each_commit_graft(each_commit_graft_fn fn, void 
*cb_data)
 
 int unregister_shallow(const struct object_id *oid)
 {
-   int pos = commit_graft_pos(oid->hash);
+   int pos = commit_graft_pos(the_repository, oid->hash);
if (pos < 0)
return -1;
if (pos + 1 < the_repository->parsed_objects->grafts_nr)
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 11/19] shallow: add repository argument to is_repository_shallow

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/fetch.c| 2 +-
 builtin/pack-objects.c | 4 ++--
 builtin/prune.c| 2 +-
 builtin/rev-parse.c| 3 ++-
 commit.c   | 2 +-
 commit.h   | 3 ++-
 fetch-pack.c   | 4 ++--
 send-pack.c| 6 +++---
 shallow.c  | 8 
 upload-pack.c  | 2 +-
 10 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c1f2df97965..55140671ef3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1445,7 +1445,7 @@ int cmd_fetch(int argc, const char **argv, const char 
*prefix)
if (unshallow) {
if (depth)
die(_("--depth and --unshallow cannot be used 
together"));
-   else if (!is_repository_shallow())
+   else if (!is_repository_shallow(the_repository))
die(_("--unshallow on a complete repository does not 
make sense"));
else
depth = xstrfmt("%d", INFINITE_DEPTH);
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 97a5963efb6..0f1eec2eecd 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2857,7 +2857,7 @@ static void get_object_list(int ac, const char **av)
setup_revisions(ac, av, , NULL);
 
/* make sure shallows are read */
-   is_repository_shallow();
+   is_repository_shallow(the_repository);
 
while (fgets(line, sizeof(line), stdin) != NULL) {
int len = strlen(line);
@@ -3142,7 +3142,7 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
use_bitmap_index = use_bitmap_index_default;
 
/* "hard" reasons not to use bitmaps; these just won't work at all */
-   if (!use_internal_rev_list || (!pack_to_stdout && write_bitmap_index) 
|| is_repository_shallow())
+   if (!use_internal_rev_list || (!pack_to_stdout && write_bitmap_index) 
|| is_repository_shallow(the_repository))
use_bitmap_index = 0;
 
if (pack_to_stdout || !rev_list_all)
diff --git a/builtin/prune.c b/builtin/prune.c
index 8cc8659612f..70ec35aa058 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -160,7 +160,7 @@ int cmd_prune(int argc, const char **argv, const char 
*prefix)
remove_temporary_files(s);
free(s);
 
-   if (is_repository_shallow())
+   if (is_repository_shallow(the_repository))
prune_shallow(show_only);
 
return 0;
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 36b20877828..a8a9b506ff6 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -879,7 +879,8 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
continue;
}
if (!strcmp(arg, "--is-shallow-repository")) {
-   printf("%s\n", is_repository_shallow() ? "true"
+   printf("%s\n",
+   
is_repository_shallow(the_repository) ? "true"
: "false");
continue;
}
diff --git a/commit.c b/commit.c
index c832133f055..684eeaa2ccd 100644
--- a/commit.c
+++ b/commit.c
@@ -208,7 +208,7 @@ static void prepare_commit_graft_the_repository(void)
graft_file = get_graft_file();
read_graft_file(the_repository, graft_file);
/* make sure shallows are read */
-   is_repository_shallow();
+   is_repository_shallow(the_repository);
commit_graft_prepared = 1;
 }
 
diff --git a/commit.h b/commit.h
index 59346de5512..c7f25d6490a 100644
--- a/commit.h
+++ b/commit.h
@@ -195,7 +195,8 @@ struct ref;
 extern int register_shallow_the_repository(const struct object_id *oid);
 extern int unregister_shallow(const struct object_id *oid);
 extern int for_each_commit_graft(each_commit_graft_fn, void *);
-extern int is_repository_shallow(void);
+#define is_repository_shallow(r) is_repository_shallow_##r()
+extern int is_repository_shallow_the_repository(void);
 extern struct commit_list *get_shallow_commits(struct object_array *heads,
int depth, int shallow_flag, int not_shallow_flag);
 extern struct commit_list *get_shallow_commits_by_rev_list(
diff --git a/fetch-pack.c b/fetch-pack.c
index e3e99e44962..90befd370fe 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -397,7 +397,7 @@ static int find_common(struct fetch_pack_args *args,
return 1;
}
 
-   if (is_repository_shallow())
+   if (is_repository_shallow(the_repository))
write_shallow_commits(_buf, 1, NULL);
if (args->depth > 0)
packet_buf_write(_buf, "deepen %d", args->depth);
@@ -986,7 +986,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
*args,
sort_ref_list(, ref_compare_name);

[PATCH 10/19] shallow: add repository argument to check_shallow_file_for_update

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 shallow.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/shallow.c b/shallow.c
index 0fadd5330d2..0028e4ea776 100644
--- a/shallow.c
+++ b/shallow.c
@@ -217,7 +217,8 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, 
const char **av,
return result;
 }
 
-static void check_shallow_file_for_update(void)
+#define check_shallow_file_for_update(r) check_shallow_file_for_update_##r()
+static void check_shallow_file_for_update_the_repository(void)
 {
if (is_shallow == -1)
die("BUG: shallow must be initialized by now");
@@ -319,7 +320,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 
fd = hold_lock_file_for_update(shallow_lock, git_path_shallow(),
   LOCK_DIE_ON_ERROR);
-   check_shallow_file_for_update();
+   check_shallow_file_for_update(the_repository);
if (write_shallow_commits(, 0, extra)) {
if (write_in_full(fd, sb.buf, sb.len) < 0)
die_errno("failed to write to %s",
@@ -366,7 +367,7 @@ void prune_shallow(int show_only)
}
fd = hold_lock_file_for_update(_lock, git_path_shallow(),
   LOCK_DIE_ON_ERROR);
-   check_shallow_file_for_update();
+   check_shallow_file_for_update(the_repository);
if (write_shallow_commits_1(, 0, NULL, SEEN_ONLY)) {
if (write_in_full(fd, sb.buf, sb.len) < 0)
die_errno("failed to write to %s",
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 07/19] commit: add repository argument to lookup_commit_graft

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow callers of lookup_commit_graft to
be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

The included coccinelle semantic patch will adapt any new callers in
the diff produced by `make coccicheck`.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 commit.c  | 4 ++--
 commit.h  | 3 ++-
 fsck.c| 2 +-
 shallow.c | 5 +++--
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/commit.c b/commit.c
index a0400b93a1e..c832133f055 100644
--- a/commit.c
+++ b/commit.c
@@ -212,7 +212,7 @@ static void prepare_commit_graft_the_repository(void)
commit_graft_prepared = 1;
 }
 
-struct commit_graft *lookup_commit_graft(const struct object_id *oid)
+struct commit_graft *lookup_commit_graft_the_repository(const struct object_id 
*oid)
 {
int pos;
prepare_commit_graft(the_repository);
@@ -359,7 +359,7 @@ int parse_commit_buffer(struct commit *item, const void 
*buffer, unsigned long s
bufptr += tree_entry_len + 1; /* "tree " + "hex sha1" + "\n" */
pptr = >parents;
 
-   graft = lookup_commit_graft(>object.oid);
+   graft = lookup_commit_graft(the_repository, >object.oid);
while (bufptr + parent_entry_len < tail && !memcmp(bufptr, "parent ", 
7)) {
struct commit *new_parent;
 
diff --git a/commit.h b/commit.h
index 40a5b5e2585..f6746125766 100644
--- a/commit.h
+++ b/commit.h
@@ -176,7 +176,8 @@ typedef int (*each_commit_graft_fn)(const struct 
commit_graft *, void *);
 struct commit_graft *read_graft_line(struct strbuf *line);
 #define register_commit_graft(r, g, i) register_commit_graft_##r(g, i)
 int register_commit_graft_the_repository(struct commit_graft *, int);
-struct commit_graft *lookup_commit_graft(const struct object_id *oid);
+#define lookup_commit_graft(r, o) lookup_commit_graft_##r(o)
+struct commit_graft *lookup_commit_graft_the_repository(const struct object_id 
*oid);
 
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit 
*rev2);
 extern struct commit_list *get_merge_bases_many(struct commit *one, int n, 
struct commit **twos);
diff --git a/fsck.c b/fsck.c
index 59b0c7d640e..104c3c0a434 100644
--- a/fsck.c
+++ b/fsck.c
@@ -738,7 +738,7 @@ static int fsck_commit_buffer(struct commit *commit, const 
char *buffer,
buffer += 41;
parent_line_count++;
}
-   graft = lookup_commit_graft(>object.oid);
+   graft = lookup_commit_graft(the_repository, >object.oid);
parent_count = commit_list_count(commit->parents);
if (graft) {
if (graft->nr_parent == -1 && !parent_count)
diff --git a/shallow.c b/shallow.c
index ef802deed41..ca360c700c5 100644
--- a/shallow.c
+++ b/shallow.c
@@ -109,7 +109,7 @@ struct commit_list *get_shallow_commits(struct object_array 
*heads, int depth,
cur_depth++;
if ((depth != INFINITE_DEPTH && cur_depth >= depth) ||
(is_repository_shallow() && !commit->parents &&
-(graft = lookup_commit_graft(>object.oid)) != NULL 
&&
+(graft = lookup_commit_graft(the_repository, 
>object.oid)) != NULL &&
 graft->nr_parent < 0)) {
commit_list_insert(commit, );
commit->object.flags |= shallow_flag;
@@ -398,7 +398,8 @@ void prepare_shallow_info(struct shallow_info *info, struct 
oid_array *sa)
for (i = 0; i < sa->nr; i++) {
if (has_object_file(sa->oid + i)) {
struct commit_graft *graft;
-   graft = lookup_commit_graft(>oid[i]);
+   graft = lookup_commit_graft(the_repository,
+   >oid[i]);
if (graft && graft->nr_parent < 0)
continue;
info->ours[info->nr_ours++] = i;
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 17/19] shallow: migrate shallow information into the object parser

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 commit.h  |  9 +++--
 object.c  |  3 +++
 object.h  |  4 
 shallow.c | 50 --
 4 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/commit.h b/commit.h
index d04bbed81cf..45114a95b25 100644
--- a/commit.h
+++ b/commit.h
@@ -190,18 +190,15 @@ extern struct commit_list 
*get_merge_bases_many_dirty(struct commit *one, int n,
 
 struct oid_array;
 struct ref;
-#define register_shallow(r, o) register_shallow_##r(o);
-extern int register_shallow_the_repository(const struct object_id *oid);
+extern int register_shallow(struct repository *r, const struct object_id *oid);
 extern int unregister_shallow(const struct object_id *oid);
 extern int for_each_commit_graft(each_commit_graft_fn, void *);
-#define is_repository_shallow(r) is_repository_shallow_##r()
-extern int is_repository_shallow_the_repository(void);
+extern int is_repository_shallow(struct repository *r);
 extern struct commit_list *get_shallow_commits(struct object_array *heads,
int depth, int shallow_flag, int not_shallow_flag);
 extern struct commit_list *get_shallow_commits_by_rev_list(
int ac, const char **av, int shallow_flag, int 
not_shallow_flag);
-#define set_alternate_shallow_file(r, p, o) set_alternate_shallow_file_##r(p, 
o)
-extern void set_alternate_shallow_file_the_repository(const char *path, int 
override);
+extern void set_alternate_shallow_file(struct repository *r, const char *path, 
int override);
 extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 const struct oid_array *extra);
 extern void setup_alternate_shallow(struct lock_file *shallow_lock,
diff --git a/object.c b/object.c
index 0116ed6529a..30b8a721cf6 100644
--- a/object.c
+++ b/object.c
@@ -464,6 +464,9 @@ struct parsed_object_pool *parsed_object_pool_new(void)
o->tag_state = allocate_alloc_state();
o->object_state = allocate_alloc_state();
 
+   o->is_shallow = -1;
+   o->shallow_stat = xcalloc(1, sizeof(*o->shallow_stat));
+
return o;
 }
 
diff --git a/object.h b/object.h
index ec908f9bcc1..a314331acaf 100644
--- a/object.h
+++ b/object.h
@@ -16,6 +16,10 @@ struct parsed_object_pool {
/* parent substitutions from .git/info/grafts and .git/shallow */
struct commit_graft **grafts;
int grafts_alloc, grafts_nr;
+
+   int is_shallow;
+   struct stat_validity *shallow_stat;
+   char *alternate_shallow_file;
 };
 
 struct parsed_object_pool *parsed_object_pool_new(void);
diff --git a/shallow.c b/shallow.c
index a0e338459f9..560329d53a8 100644
--- a/shallow.c
+++ b/shallow.c
@@ -14,22 +14,21 @@
 #include "commit-slab.h"
 #include "revision.h"
 #include "list-objects.h"
+#include "repository.h"
 
-static int is_shallow = -1;
-static struct stat_validity shallow_stat;
-static char *alternate_shallow_file;
+struct stat_validity the_repository_shallow_stat;
 
-void set_alternate_shallow_file_the_repository(const char *path, int override)
+void set_alternate_shallow_file(struct repository *r, const char *path, int 
override)
 {
-   if (is_shallow != -1)
+   if (r->parsed_objects->is_shallow != -1)
die("BUG: is_repository_shallow must not be called before 
set_alternate_shallow_file");
-   if (alternate_shallow_file && !override)
+   if (r->parsed_objects->alternate_shallow_file && !override)
return;
-   free(alternate_shallow_file);
-   alternate_shallow_file = xstrdup_or_null(path);
+   free(r->parsed_objects->alternate_shallow_file);
+   r->parsed_objects->alternate_shallow_file = xstrdup_or_null(path);
 }
 
-int register_shallow_the_repository(const struct object_id *oid)
+int register_shallow(struct repository *r, const struct object_id *oid)
 {
struct commit_graft *graft =
xmalloc(sizeof(struct commit_graft));
@@ -39,41 +38,41 @@ int register_shallow_the_repository(const struct object_id 
*oid)
graft->nr_parent = -1;
if (commit && commit->object.parsed)
commit->parents = NULL;
-   return register_commit_graft(the_repository, graft, 0);
+   return register_commit_graft(r, graft, 0);
 }
 
-int is_repository_shallow_the_repository(void)
+int is_repository_shallow(struct repository *r)
 {
FILE *fp;
char buf[1024];
-   const char *path = alternate_shallow_file;
+   const char *path = r->parsed_objects->alternate_shallow_file;
 
-   if (is_shallow >= 0)
-   return is_shallow;
+   if (r->parsed_objects->is_shallow >= 0)
+   return r->parsed_objects->is_shallow;
 
if (!path)
-   path = git_path_shallow(the_repository);
+   path = git_path_shallow(r);
/*
 * fetch-pack sets '--shallow-file ""' as an indicator that no
 * shallow file should be used. We could just open it and it

[PATCH 13/19] commit: convert register_commit_graft to handle arbitrary repositories

[Stefan Beller]

From: Brandon Williams 

Signed-off-by: Brandon Williams 
Signed-off-by: Stefan Beller 
---
 commit.c | 29 +++--
 commit.h |  3 +--
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/commit.c b/commit.c
index 0ec3d22813a..8a2ab53fc67 100644
--- a/commit.c
+++ b/commit.c
@@ -111,30 +111,31 @@ static int commit_graft_pos(struct repository *r, const 
unsigned char *sha1)
commit_graft_sha1_access);
 }
 
-int register_commit_graft_the_repository(struct commit_graft *graft, int 
ignore_dups)
+int register_commit_graft(struct repository *r, struct commit_graft *graft,
+ int ignore_dups)
 {
-   int pos = commit_graft_pos(the_repository, graft->oid.hash);
+   int pos = commit_graft_pos(r, graft->oid.hash);
 
if (0 <= pos) {
if (ignore_dups)
free(graft);
else {
-   free(the_repository->parsed_objects->grafts[pos]);
-   the_repository->parsed_objects->grafts[pos] = graft;
+   free(r->parsed_objects->grafts[pos]);
+   r->parsed_objects->grafts[pos] = graft;
}
return 1;
}
pos = -pos - 1;
-   ALLOC_GROW(the_repository->parsed_objects->grafts,
-  the_repository->parsed_objects->grafts_nr + 1,
-  the_repository->parsed_objects->grafts_alloc);
-   the_repository->parsed_objects->grafts_nr++;
-   if (pos < the_repository->parsed_objects->grafts_nr)
-   memmove(the_repository->parsed_objects->grafts + pos + 1,
-   the_repository->parsed_objects->grafts + pos,
-   (the_repository->parsed_objects->grafts_nr - pos - 1) *
-   sizeof(*the_repository->parsed_objects->grafts));
-   the_repository->parsed_objects->grafts[pos] = graft;
+   ALLOC_GROW(r->parsed_objects->grafts,
+  r->parsed_objects->grafts_nr + 1,
+  r->parsed_objects->grafts_alloc);
+   r->parsed_objects->grafts_nr++;
+   if (pos < r->parsed_objects->grafts_nr)
+   memmove(r->parsed_objects->grafts + pos + 1,
+   r->parsed_objects->grafts + pos,
+   (r->parsed_objects->grafts_nr - pos - 1) *
+   sizeof(*r->parsed_objects->grafts));
+   r->parsed_objects->grafts[pos] = graft;
return 0;
 }
 
diff --git a/commit.h b/commit.h
index c7f25d6490a..d04bbed81cf 100644
--- a/commit.h
+++ b/commit.h
@@ -174,8 +174,7 @@ struct commit_graft {
 typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
 
 struct commit_graft *read_graft_line(struct strbuf *line);
-#define register_commit_graft(r, g, i) register_commit_graft_##r(g, i)
-int register_commit_graft_the_repository(struct commit_graft *, int);
+int register_commit_graft(struct repository *r, struct commit_graft *, int);
 #define lookup_commit_graft(r, o) lookup_commit_graft_##r(o)
 struct commit_graft *lookup_commit_graft_the_repository(const struct object_id 
*oid);
 
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 08/19] shallow: add repository argument to set_alternate_shallow_file

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 commit.h  | 3 ++-
 environment.c | 2 +-
 git.c | 2 +-
 shallow.c | 2 +-
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/commit.h b/commit.h
index f6746125766..f88c854e2f6 100644
--- a/commit.h
+++ b/commit.h
@@ -199,7 +199,8 @@ extern struct commit_list *get_shallow_commits(struct 
object_array *heads,
int depth, int shallow_flag, int not_shallow_flag);
 extern struct commit_list *get_shallow_commits_by_rev_list(
int ac, const char **av, int shallow_flag, int 
not_shallow_flag);
-extern void set_alternate_shallow_file(const char *path, int override);
+#define set_alternate_shallow_file(r, p, o) set_alternate_shallow_file_##r(p, 
o)
+extern void set_alternate_shallow_file_the_repository(const char *path, int 
override);
 extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 const struct oid_array *extra);
 extern void setup_alternate_shallow(struct lock_file *shallow_lock,
diff --git a/environment.c b/environment.c
index b991fc0a87c..87d9e52ffde 100644
--- a/environment.c
+++ b/environment.c
@@ -189,7 +189,7 @@ void setup_git_env(const char *git_dir)
git_namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));
shallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT);
if (shallow_file)
-   set_alternate_shallow_file(shallow_file, 0);
+   set_alternate_shallow_file(the_repository, shallow_file, 0);
 }
 
 int is_bare_repository(void)
diff --git a/git.c b/git.c
index 3a89893712e..5e8903681e6 100644
--- a/git.c
+++ b/git.c
@@ -207,7 +207,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
} else if (!strcmp(cmd, "--shallow-file")) {
(*argv)++;
(*argc)--;
-   set_alternate_shallow_file((*argv)[0], 1);
+   set_alternate_shallow_file(the_repository, (*argv)[0], 
1);
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "-C")) {
diff --git a/shallow.c b/shallow.c
index ca360c700c5..73cb11a9162 100644
--- a/shallow.c
+++ b/shallow.c
@@ -19,7 +19,7 @@ static int is_shallow = -1;
 static struct stat_validity shallow_stat;
 static char *alternate_shallow_file;
 
-void set_alternate_shallow_file(const char *path, int override)
+void set_alternate_shallow_file_the_repository(const char *path, int override)
 {
if (is_shallow != -1)
die("BUG: is_repository_shallow must not be called before 
set_alternate_shallow_file");
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 14/19] commit: convert read_graft_file to handle arbitrary repositories

[Stefan Beller]

From: Brandon Williams 

Signed-off-by: Brandon Williams 
Signed-off-by: Stefan Beller 
---
 commit.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/commit.c b/commit.c
index 8a2ab53fc67..3fcb2fd66ce 100644
--- a/commit.c
+++ b/commit.c
@@ -177,8 +177,7 @@ struct commit_graft *read_graft_line(struct strbuf *line)
return NULL;
 }
 
-#define read_graft_file(r, f) read_graft_file_##r(f)
-static int read_graft_file_the_repository(const char *graft_file)
+static int read_graft_file(struct repository *r, const char *graft_file)
 {
FILE *fp = fopen_or_warn(graft_file, "r");
struct strbuf buf = STRBUF_INIT;
@@ -189,7 +188,7 @@ static int read_graft_file_the_repository(const char 
*graft_file)
struct commit_graft *graft = read_graft_line();
if (!graft)
continue;
-   if (register_commit_graft(the_repository, graft, 1))
+   if (register_commit_graft(r, graft, 1))
error("duplicate graft data: %s", buf.buf);
}
fclose(fp);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 01/19] object-store: move object access functions to object-store.h

[Stefan Beller]

This should make these functions easier to find and cache.h less
overwhelming to read.

In particular, this moves:
- read_object_file
- oid_object_info
- write_object_file

As a result, most of the codebase needs to #include object-store.h.
In this patch the #include is only added to files that would fail to
compile otherwise.  It would be better to #include wherever
identifiers from the header are used.  That can happen later
when we have better tooling for it.

Signed-off-by: Stefan Beller 
---
 apply.c  |   1 +
 archive-tar.c|   1 +
 archive-zip.c|   1 +
 archive.c|   1 +
 blame.c  |   1 +
 builtin/blame.c  |   1 +
 builtin/cat-file.c   |   1 +
 builtin/checkout.c   |   1 +
 builtin/clone.c  |   1 +
 builtin/commit-tree.c|   1 +
 builtin/describe.c   |   1 +
 builtin/difftool.c   |   1 +
 builtin/fast-export.c|   1 +
 builtin/fetch.c  |   1 +
 builtin/fmt-merge-msg.c  |   1 +
 builtin/hash-object.c|   1 +
 builtin/log.c|   1 +
 builtin/ls-tree.c|   1 +
 builtin/merge-tree.c |   1 +
 builtin/mktag.c  |   1 +
 builtin/mktree.c |   1 +
 builtin/notes.c  |   1 +
 builtin/prune.c  |   1 +
 builtin/receive-pack.c   |   1 +
 builtin/reflog.c |   1 +
 builtin/remote.c |   1 +
 builtin/rev-list.c   |   1 +
 builtin/show-ref.c   |   1 +
 builtin/tag.c|   1 +
 builtin/unpack-file.c|   1 +
 builtin/unpack-objects.c |   1 +
 builtin/verify-commit.c  |   1 +
 bulk-checkin.c   |   1 +
 bundle.c |   1 +
 cache-tree.c |   1 +
 cache.h  | 117 ---
 combine-diff.c   |   1 +
 commit.c |   1 +
 config.c |   1 +
 convert.c|   1 +
 diff.c   |   1 +
 diffcore-rename.c|   1 +
 dir.c|   1 +
 entry.c  |   1 +
 fetch-pack.c |   1 +
 fsck.c   |   1 +
 grep.c   |   1 +
 list-objects-filter.c|   1 +
 list-objects.c   |   1 +
 log-tree.c   |   1 +
 mailmap.c|   1 +
 match-trees.c|   1 +
 merge-blobs.c|   1 +
 merge-recursive.c|   1 +
 notes-cache.c|   1 +
 notes-merge.c|   1 +
 notes.c  |   1 +
 object-store.h   | 117 +++
 object.c |   1 +
 pack-bitmap-write.c  |   1 +
 packfile.h   |   5 ++
 read-cache.c |   1 +
 ref-filter.c |   1 +
 refs.c   |   1 +
 remote-testsvn.c |   1 +
 remote.c |   1 +
 rerere.c |   1 +
 revision.c   |   1 +
 send-pack.c  |   1 +
 sequencer.c  |   1 +
 shallow.c|   1 +
 submodule-config.c   |   1 +
 tag.c|   1 +
 tree-walk.c  |   1 +
 tree.c   |   1 +
 unpack-trees.c   |   1 +
 upload-pack.c|   1 +
 walker.c |   1 +
 xdiff-interface.c|   1 +
 79 files changed, 198 insertions(+), 117 deletions(-)

diff --git a/apply.c b/apply.c
index 7e5792c996f..cbc45fa1b0e 100644
--- a/apply.c
+++ b/apply.c
@@ -9,6 +9,7 @@
 
 #include "cache.h"
 #include "config.h"
+#include "object-store.h"
 #include "blob.h"
 #include "delta.h"
 #include "diff.h"
diff --git a/archive-tar.c b/archive-tar.c
index f93409324f9..e38435eb4ef 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -5,6 +5,7 @@
 #include "config.h"
 #include "tar.h"
 #include "archive.h"
+#include "object-store.h"
 #include "streaming.h"
 #include "run-command.h"
 
diff --git a/archive-zip.c b/archive-zip.c
index 74f3fe91034..abc556e5a75 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -6,6 +6,7 @@
 #include "archive.h"
 #include "streaming.h"
 #include "utf8.h"
+#include "object-store.h"
 #include "userdiff.h"
 #include "xdiff-interface.h"
 
diff --git a/archive.c b/archive.c
index 93ab175b0b4..9da1e3664a6 100644
--- a/archive.c
+++ b/archive.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "config.h"
 #include "refs.h"
+#include "object-store.h"
 #include "commit.h"
 #include "tree-walk.h"
 #include "attr.h"
diff --git a/blame.c b/blame.c
index 3a11f1ce52b..f689bde31cd 100644
--- a/blame.c
+++ b/blame.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "refs.h"
+#include "object-store.h"
 #include "cache-tree.h"
 #include "mergesort.h"
 #include "diff.h"
diff --git a/builtin/blame.c b/builtin/blame.c
index bfdf7cc1325..0ffd1d443ea 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -22,6 +22,7 @@
 #include "line-log.h"
 #include "dir.h"
 #include "progress.h"
+#include "object-store.h"
 #include "blame.h"
 
 static char blame_usage[] = N_("git blame [] [] [] 
[--] ");
diff --git 

[PATCH 18/19] commit: allow prepare_commit_graft to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 commit.c | 14 ++
 object.h |  2 ++
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/commit.c b/commit.c
index 24028fd257a..eef1675d692 100644
--- a/commit.c
+++ b/commit.c
@@ -196,19 +196,17 @@ static int read_graft_file(struct repository *r, const 
char *graft_file)
return 0;
 }
 
-#define prepare_commit_graft(r) prepare_commit_graft_##r()
-static void prepare_commit_graft_the_repository(void)
+static void prepare_commit_graft(struct repository *r)
 {
-   static int commit_graft_prepared;
char *graft_file;
 
-   if (commit_graft_prepared)
+   if (r->parsed_objects->commit_graft_prepared)
return;
-   graft_file = get_graft_file(the_repository);
-   read_graft_file(the_repository, graft_file);
+   graft_file = get_graft_file(r);
+   read_graft_file(r, graft_file);
/* make sure shallows are read */
-   is_repository_shallow(the_repository);
-   commit_graft_prepared = 1;
+   is_repository_shallow(r);
+   r->parsed_objects->commit_graft_prepared = 1;
 }
 
 struct commit_graft *lookup_commit_graft_the_repository(const struct object_id 
*oid)
diff --git a/object.h b/object.h
index a314331acaf..4af499ab03e 100644
--- a/object.h
+++ b/object.h
@@ -20,6 +20,8 @@ struct parsed_object_pool {
int is_shallow;
struct stat_validity *shallow_stat;
char *alternate_shallow_file;
+
+   int commit_graft_prepared;
 };
 
 struct parsed_object_pool *parsed_object_pool_new(void);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 19/19] commit: allow lookup_commit_graft to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 commit.c | 8 
 commit.h | 3 +--
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/commit.c b/commit.c
index eef1675d692..b01cc0c3e0c 100644
--- a/commit.c
+++ b/commit.c
@@ -209,14 +209,14 @@ static void prepare_commit_graft(struct repository *r)
r->parsed_objects->commit_graft_prepared = 1;
 }
 
-struct commit_graft *lookup_commit_graft_the_repository(const struct object_id 
*oid)
+struct commit_graft *lookup_commit_graft(struct repository *r, const struct 
object_id *oid)
 {
int pos;
-   prepare_commit_graft(the_repository);
-   pos = commit_graft_pos(the_repository, oid->hash);
+   prepare_commit_graft(r);
+   pos = commit_graft_pos(r, oid->hash);
if (pos < 0)
return NULL;
-   return the_repository->parsed_objects->grafts[pos];
+   return r->parsed_objects->grafts[pos];
 }
 
 int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)
diff --git a/commit.h b/commit.h
index 45114a95b25..6de6f10cd04 100644
--- a/commit.h
+++ b/commit.h
@@ -175,8 +175,7 @@ typedef int (*each_commit_graft_fn)(const struct 
commit_graft *, void *);
 
 struct commit_graft *read_graft_line(struct strbuf *line);
 int register_commit_graft(struct repository *r, struct commit_graft *, int);
-#define lookup_commit_graft(r, o) lookup_commit_graft_##r(o)
-struct commit_graft *lookup_commit_graft_the_repository(const struct object_id 
*oid);
+struct commit_graft *lookup_commit_graft(struct repository *r, const struct 
object_id *oid);
 
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit 
*rev2);
 extern struct commit_list *get_merge_bases_many(struct commit *one, int n, 
struct commit **twos);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 12/19] commit: convert commit_graft_pos() to handle arbitrary repositories

[Stefan Beller]

From: Brandon Williams 

Signed-off-by: Brandon Williams 
Signed-off-by: Stefan Beller 
---
 commit.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/commit.c b/commit.c
index 684eeaa2ccd..0ec3d22813a 100644
--- a/commit.c
+++ b/commit.c
@@ -104,11 +104,10 @@ static const unsigned char 
*commit_graft_sha1_access(size_t index, void *table)
return commit_graft_table[index]->oid.hash;
 }
 
-#define commit_graft_pos(r, s) commit_graft_pos_##r(s)
-static int commit_graft_pos_the_repository(const unsigned char *sha1)
+static int commit_graft_pos(struct repository *r, const unsigned char *sha1)
 {
-   return sha1_pos(sha1, the_repository->parsed_objects->grafts,
-   the_repository->parsed_objects->grafts_nr,
+   return sha1_pos(sha1, r->parsed_objects->grafts,
+   r->parsed_objects->grafts_nr,
commit_graft_sha1_access);
 }
 
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 15/19] cache: convert get_graft_file to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 cache.h   | 2 +-
 commit.c  | 2 +-
 environment.c | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index ab716011b7e..cb1aeb1dcbf 100644
--- a/cache.h
+++ b/cache.h
@@ -476,7 +476,7 @@ extern const char *get_git_dir(void);
 extern const char *get_git_common_dir(void);
 extern char *get_object_directory(void);
 extern char *get_index_file(void);
-extern char *get_graft_file(void);
+extern char *get_graft_file(struct repository *r);
 extern int set_git_dir(const char *path);
 extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
diff --git a/commit.c b/commit.c
index 3fcb2fd66ce..24028fd257a 100644
--- a/commit.c
+++ b/commit.c
@@ -204,7 +204,7 @@ static void prepare_commit_graft_the_repository(void)
 
if (commit_graft_prepared)
return;
-   graft_file = get_graft_file();
+   graft_file = get_graft_file(the_repository);
read_graft_file(the_repository, graft_file);
/* make sure shallows are read */
is_repository_shallow(the_repository);
diff --git a/environment.c b/environment.c
index 87d9e52ffde..ab42346e563 100644
--- a/environment.c
+++ b/environment.c
@@ -316,11 +316,11 @@ char *get_index_file(void)
return the_repository->index_file;
 }
 
-char *get_graft_file(void)
+char *get_graft_file(struct repository *r)
 {
-   if (!the_repository->graft_file)
+   if (!r->graft_file)
BUG("git environment hasn't been setup");
-   return the_repository->graft_file;
+   return r->graft_file;
 }
 
 int set_git_dir(const char *path)
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 16/19] path.c: migrate git_path_ to take a repository argument

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 blame.c  |  8 +---
 branch.c | 14 +++---
 builtin/commit.c | 38 +++---
 builtin/fetch.c  |  4 ++--
 builtin/merge.c  | 37 +++--
 builtin/pull.c   |  4 ++--
 builtin/reset.c  |  2 +-
 fetch-pack.c |  2 +-
 path.c   | 18 +-
 path.h   | 40 +++-
 repository.h |  5 +
 rerere.c |  7 ---
 sequencer.c  | 37 +++--
 shallow.c| 12 +++-
 wt-status.c  |  8 
 15 files changed, 135 insertions(+), 101 deletions(-)

diff --git a/blame.c b/blame.c
index f689bde31cd..c22184c2dad 100644
--- a/blame.c
+++ b/blame.c
@@ -112,17 +112,19 @@ static void append_merge_parents(struct commit_list 
**tail)
int merge_head;
struct strbuf line = STRBUF_INIT;
 
-   merge_head = open(git_path_merge_head(), O_RDONLY);
+   merge_head = open(git_path_merge_head(the_repository), O_RDONLY);
if (merge_head < 0) {
if (errno == ENOENT)
return;
-   die("cannot open '%s' for reading", git_path_merge_head());
+   die("cannot open '%s' for reading",
+   git_path_merge_head(the_repository));
}
 
while (!strbuf_getwholeline_fd(, merge_head, '\n')) {
struct object_id oid;
if (line.len < GIT_SHA1_HEXSZ || get_oid_hex(line.buf, ))
-   die("unknown line in '%s': %s", git_path_merge_head(), 
line.buf);
+   die("unknown line in '%s': %s",
+   git_path_merge_head(the_repository), line.buf);
tail = append_parent(tail, );
}
close(merge_head);
diff --git a/branch.c b/branch.c
index 2672054f0b5..9b2742de32a 100644
--- a/branch.c
+++ b/branch.c
@@ -339,13 +339,13 @@ void create_branch(const char *name, const char 
*start_name,
 
 void remove_branch_state(void)
 {
-   unlink(git_path_cherry_pick_head());
-   unlink(git_path_revert_head());
-   unlink(git_path_merge_head());
-   unlink(git_path_merge_rr());
-   unlink(git_path_merge_msg());
-   unlink(git_path_merge_mode());
-   unlink(git_path_squash_msg());
+   unlink(git_path_cherry_pick_head(the_repository));
+   unlink(git_path_revert_head(the_repository));
+   unlink(git_path_merge_head(the_repository));
+   unlink(git_path_merge_rr(the_repository));
+   unlink(git_path_merge_msg(the_repository));
+   unlink(git_path_merge_mode(the_repository));
+   unlink(git_path_squash_msg(the_repository));
 }
 
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
diff --git a/builtin/commit.c b/builtin/commit.c
index 37fcb55ab0a..7c22879777d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -145,9 +145,9 @@ static int opt_parse_m(const struct option *opt, const char 
*arg, int unset)
 
 static void determine_whence(struct wt_status *s)
 {
-   if (file_exists(git_path_merge_head()))
+   if (file_exists(git_path_merge_head(the_repository)))
whence = FROM_MERGE;
-   else if (file_exists(git_path_cherry_pick_head())) {
+   else if (file_exists(git_path_cherry_pick_head(the_repository))) {
whence = FROM_CHERRY_PICK;
if (file_exists(git_path_seq_dir()))
sequencer_in_use = 1;
@@ -696,21 +696,21 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
if (have_option_m)
strbuf_addbuf(, );
hook_arg1 = "message";
-   } else if (!stat(git_path_merge_msg(), )) {
+   } else if (!stat(git_path_merge_msg(the_repository), )) {
/*
 * prepend SQUASH_MSG here if it exists and a
 * "merge --squash" was originally performed
 */
-   if (!stat(git_path_squash_msg(), )) {
-   if (strbuf_read_file(, git_path_squash_msg(), 0) < 0)
+   if (!stat(git_path_squash_msg(the_repository), )) {
+   if (strbuf_read_file(, 
git_path_squash_msg(the_repository), 0) < 0)
die_errno(_("could not read SQUASH_MSG"));
hook_arg1 = "squash";
} else
hook_arg1 = "merge";
-   if (strbuf_read_file(, git_path_merge_msg(), 0) < 0)
+   if (strbuf_read_file(, git_path_merge_msg(the_repository), 
0) < 0)
die_errno(_("could not read MERGE_MSG"));
-   } else if (!stat(git_path_squash_msg(), )) {
-   if (strbuf_read_file(, git_path_squash_msg(), 0) < 0)
+   } else if (!stat(git_path_squash_msg(the_repository), )) {
+   if (strbuf_read_file(, git_path_squash_msg(the_repository), 
0) < 0)

[PATCH 09/19] shallow: add repository argument to register_shallow

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/pack-objects.c | 2 +-
 builtin/receive-pack.c | 2 +-
 commit.h   | 3 ++-
 fetch-pack.c   | 2 +-
 shallow.c  | 4 ++--
 upload-pack.c  | 7 ---
 6 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d65eb4a9478..97a5963efb6 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2875,7 +2875,7 @@ static void get_object_list(int ac, const char **av)
struct object_id oid;
if (get_oid_hex(line + 10, ))
die("not an SHA-1 '%s'", line + 10);
-   register_shallow();
+   register_shallow(the_repository, );
use_bitmap_index = 0;
continue;
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 36906fd5e98..c666820b69a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -906,7 +906,7 @@ static int update_shallow_ref(struct command *cmd, struct 
shallow_info *si)
 * not lose these new roots..
 */
for (i = 0; i < extra.nr; i++)
-   register_shallow([i]);
+   register_shallow(the_repository, [i]);
 
si->shallow_ref[cmd->index] = 0;
oid_array_clear();
diff --git a/commit.h b/commit.h
index f88c854e2f6..59346de5512 100644
--- a/commit.h
+++ b/commit.h
@@ -191,7 +191,8 @@ extern struct commit_list 
*get_merge_bases_many_dirty(struct commit *one, int n,
 
 struct oid_array;
 struct ref;
-extern int register_shallow(const struct object_id *oid);
+#define register_shallow(r, o) register_shallow_##r(o);
+extern int register_shallow_the_repository(const struct object_id *oid);
 extern int unregister_shallow(const struct object_id *oid);
 extern int for_each_commit_graft(each_commit_graft_fn, void *);
 extern int is_repository_shallow(void);
diff --git a/fetch-pack.c b/fetch-pack.c
index a1535b37b9b..e3e99e44962 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -428,7 +428,7 @@ static int find_common(struct fetch_pack_args *args,
if (skip_prefix(line, "shallow ", )) {
if (get_oid_hex(arg, ))
die(_("invalid shallow line: %s"), 
line);
-   register_shallow();
+   register_shallow(the_repository, );
continue;
}
if (skip_prefix(line, "unshallow ", )) {
diff --git a/shallow.c b/shallow.c
index 73cb11a9162..0fadd5330d2 100644
--- a/shallow.c
+++ b/shallow.c
@@ -29,7 +29,7 @@ void set_alternate_shallow_file_the_repository(const char 
*path, int override)
alternate_shallow_file = xstrdup_or_null(path);
 }
 
-int register_shallow(const struct object_id *oid)
+int register_shallow_the_repository(const struct object_id *oid)
 {
struct commit_graft *graft =
xmalloc(sizeof(struct commit_graft));
@@ -70,7 +70,7 @@ int is_repository_shallow(void)
struct object_id oid;
if (get_oid_hex(buf, ))
die("bad shallow line: %s", buf);
-   register_shallow();
+   register_shallow(the_repository, );
}
fclose(fp);
return is_shallow;
diff --git a/upload-pack.c b/upload-pack.c
index a11c6d192ce..4e4ac0f0d95 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -663,7 +663,7 @@ static void send_shallow(struct commit_list *result)
if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
packet_write_fmt(1, "shallow %s",
 oid_to_hex(>oid));
-   register_shallow(>oid);
+   register_shallow(the_repository, >oid);
shallow_nr++;
}
result = result->next;
@@ -700,7 +700,7 @@ static void send_unshallow(const struct object_array 
*shallows)
add_object_array(object, NULL, _edge_obj);
}
/* make sure commit traversal conforms to client */
-   register_shallow(>oid);
+   register_shallow(the_repository, >oid);
}
 }
 
@@ -912,7 +912,8 @@ static void receive_needs(void)
if (shallows.nr > 0) {
int i;
for (i = 0; i < shallows.nr; i++)
-   
register_shallow([i].item->oid);
+   register_shallow(the_repository,
+
[i].item->oid);
}
 
shallow_nr += shallows.nr;
-- 
2.17.0.582.gccdcbd54c44.dirty

[RFC PATCH 00/19] object store: grafts and shallow.

[Stefan Beller]

This applies on top of sb/object-store-alloc[1], and is the next part of the
object store series. 

I think we're getting close to actually being done in the object store series,
as the next series to build on top of this will convert the
lookup_{object, commit, ...} functions.

However I marked this series as RFC, as I expect heavy conflicts with the
code base. sb/object-store-alloc builds on an older base of the code base
and there have been some series that will conflict with this one.
For example the patch to migrate path functions into a repository world
will collide with one of Dschos series as he added another path helper
function there. I am happy to reroll on top of that, but for now I chose
sb/object-store-alloc to keep the momentum in the object-store series'.

There is another object store series that is not part of the critical path:
There is a mem leak in the pack files stored in the raw object store.
Upon free'ing the repository we do not free its pack files. We cannot
free the packfiles, as otherwise the bitmap code would have dangling
pointers into packfiles that no longer exists, leading to segfaults.
So we'll need to have an object store series covering the bitmap code.
I have something local, which I'll send out shortly.

Thanks,
Stefan

[1] with the latest patch replaced as in
https://public-inbox.org/git/20180515214842.108713-1-sbel...@google.com/

Brandon Williams (3):
  commit: convert commit_graft_pos() to handle arbitrary repositories
  commit: convert register_commit_graft to handle arbitrary repositories
  commit: convert read_graft_file to handle arbitrary repositories

Jonathan Nieder (6):
  object: move grafts to object parser
  commit: add repository argument to commit_graft_pos
  commit: add repository argument to register_commit_graft
  commit: add repository argument to read_graft_file
  commit: add repository argument to prepare_commit_graft
  commit: add repository argument to lookup_commit_graft

Stefan Beller (10):
  object-store: move object access functions to object-store.h
  shallow: add repository argument to set_alternate_shallow_file
  shallow: add repository argument to register_shallow
  shallow: add repository argument to check_shallow_file_for_update
  shallow: add repository argument to is_repository_shallow
  cache: convert get_graft_file to handle arbitrary repositories
  path.c: migrate git_path_ to take a repository argument
  shallow: migrate shallow information into the object parser
  commit: allow prepare_commit_graft to handle arbitrary repositories
  commit: allow lookup_commit_graft to handle arbitrary repositories

 apply.c  |   1 +
 archive-tar.c|   1 +
 archive-zip.c|   1 +
 archive.c|   1 +
 blame.c  |   9 ++-
 branch.c |  14 ++---
 builtin/blame.c  |   4 +-
 builtin/cat-file.c   |   1 +
 builtin/checkout.c   |   1 +
 builtin/clone.c  |   1 +
 builtin/commit-tree.c|   1 +
 builtin/commit.c |  38 ++---
 builtin/describe.c   |   1 +
 builtin/difftool.c   |   1 +
 builtin/fast-export.c|   1 +
 builtin/fetch.c  |   7 ++-
 builtin/fmt-merge-msg.c  |   1 +
 builtin/hash-object.c|   1 +
 builtin/log.c|   1 +
 builtin/ls-tree.c|   1 +
 builtin/merge-tree.c |   1 +
 builtin/merge.c  |  37 ++--
 builtin/mktag.c  |   1 +
 builtin/mktree.c |   1 +
 builtin/notes.c  |   1 +
 builtin/pack-objects.c   |   6 +-
 builtin/prune.c  |   3 +-
 builtin/pull.c   |   4 +-
 builtin/receive-pack.c   |   3 +-
 builtin/reflog.c |   1 +
 builtin/remote.c |   1 +
 builtin/reset.c  |   2 +-
 builtin/rev-list.c   |   1 +
 builtin/rev-parse.c  |   3 +-
 builtin/show-ref.c   |   1 +
 builtin/tag.c|   1 +
 builtin/unpack-file.c|   1 +
 builtin/unpack-objects.c |   1 +
 builtin/verify-commit.c  |   1 +
 bulk-checkin.c   |   1 +
 bundle.c |   1 +
 cache-tree.c |   1 +
 cache.h  | 119 +--
 combine-diff.c   |   1 +
 commit.c |  77 +
 commit.h |  10 ++--
 config.c |   1 +
 convert.c|   1 +
 diff.c   |   1 +
 diffcore-rename.c|   1 +
 dir.c|   1 +
 entry.c  |   1 +
 environment.c|   8 +--
 fetch-pack.c |   9 +--
 fsck.c   |   3 +-
 git.c|   2 +-
 grep.c   |   1 +
 list-objects-filter.c|   1 +
 list-objects.c   |   1 +
 log-tree.c   |   1 +
 mailmap.c|   1 +
 match-trees.c|   1 +
 merge-blobs.c|   1 +
 merge-recursive.c|   1 +
 notes-cache.c|   1 +
 notes-merge.c|   1 +
 notes.c  

[PATCH 05/19] commit: add repository argument to read_graft_file

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow the caller of read_graft_file to be
more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 commit.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 4e8d3488425..b5c0aec24a0 100644
--- a/commit.c
+++ b/commit.c
@@ -177,7 +177,8 @@ struct commit_graft *read_graft_line(struct strbuf *line)
return NULL;
 }
 
-static int read_graft_file(const char *graft_file)
+#define read_graft_file(r, f) read_graft_file_##r(f)
+static int read_graft_file_the_repository(const char *graft_file)
 {
FILE *fp = fopen_or_warn(graft_file, "r");
struct strbuf buf = STRBUF_INIT;
@@ -204,7 +205,7 @@ static void prepare_commit_graft(void)
if (commit_graft_prepared)
return;
graft_file = get_graft_file();
-   read_graft_file(graft_file);
+   read_graft_file(the_repository, graft_file);
/* make sure shallows are read */
is_repository_shallow();
commit_graft_prepared = 1;
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 02/19] object: move grafts to object parser

[Stefan Beller]

From: Jonathan Nieder 

Grafts are only meaningful in the context of a single repository.
Therefore they cannot be global.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 commit.c | 42 +++---
 object.h |  4 
 2 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/commit.c b/commit.c
index b053f07f305..a0c9eb05c82 100644
--- a/commit.c
+++ b/commit.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "tag.h"
 #include "commit.h"
+#include "repository.h"
 #include "object-store.h"
 #include "pkt-line.h"
 #include "utf8.h"
@@ -97,9 +98,6 @@ static timestamp_t parse_commit_date(const char *buf, const 
char *tail)
return parse_timestamp(dateptr, NULL, 10);
 }
 
-static struct commit_graft **commit_graft;
-static int commit_graft_alloc, commit_graft_nr;
-
 static const unsigned char *commit_graft_sha1_access(size_t index, void *table)
 {
struct commit_graft **commit_graft_table = table;
@@ -108,7 +106,8 @@ static const unsigned char *commit_graft_sha1_access(size_t 
index, void *table)
 
 static int commit_graft_pos(const unsigned char *sha1)
 {
-   return sha1_pos(sha1, commit_graft, commit_graft_nr,
+   return sha1_pos(sha1, the_repository->parsed_objects->grafts,
+   the_repository->parsed_objects->grafts_nr,
commit_graft_sha1_access);
 }
 
@@ -120,18 +119,22 @@ int register_commit_graft(struct commit_graft *graft, int 
ignore_dups)
if (ignore_dups)
free(graft);
else {
-   free(commit_graft[pos]);
-   commit_graft[pos] = graft;
+   free(the_repository->parsed_objects->grafts[pos]);
+   the_repository->parsed_objects->grafts[pos] = graft;
}
return 1;
}
pos = -pos - 1;
-   ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);
-   commit_graft_nr++;
-   if (pos < commit_graft_nr)
-   MOVE_ARRAY(commit_graft + pos + 1, commit_graft + pos,
-  commit_graft_nr - pos - 1);
-   commit_graft[pos] = graft;
+   ALLOC_GROW(the_repository->parsed_objects->grafts,
+  the_repository->parsed_objects->grafts_nr + 1,
+  the_repository->parsed_objects->grafts_alloc);
+   the_repository->parsed_objects->grafts_nr++;
+   if (pos < the_repository->parsed_objects->grafts_nr)
+   memmove(the_repository->parsed_objects->grafts + pos + 1,
+   the_repository->parsed_objects->grafts + pos,
+   (the_repository->parsed_objects->grafts_nr - pos - 1) *
+   sizeof(*the_repository->parsed_objects->grafts));
+   the_repository->parsed_objects->grafts[pos] = graft;
return 0;
 }
 
@@ -213,14 +216,14 @@ struct commit_graft *lookup_commit_graft(const struct 
object_id *oid)
pos = commit_graft_pos(oid->hash);
if (pos < 0)
return NULL;
-   return commit_graft[pos];
+   return the_repository->parsed_objects->grafts[pos];
 }
 
 int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)
 {
int i, ret;
-   for (i = ret = 0; i < commit_graft_nr && !ret; i++)
-   ret = fn(commit_graft[i], cb_data);
+   for (i = ret = 0; i < the_repository->parsed_objects->grafts_nr && 
!ret; i++)
+   ret = fn(the_repository->parsed_objects->grafts[i], cb_data);
return ret;
 }
 
@@ -229,10 +232,11 @@ int unregister_shallow(const struct object_id *oid)
int pos = commit_graft_pos(oid->hash);
if (pos < 0)
return -1;
-   if (pos + 1 < commit_graft_nr)
-   MOVE_ARRAY(commit_graft + pos, commit_graft + pos + 1,
-  commit_graft_nr - pos - 1);
-   commit_graft_nr--;
+   if (pos + 1 < the_repository->parsed_objects->grafts_nr)
+   MOVE_ARRAY(the_repository->parsed_objects->grafts + pos,
+  the_repository->parsed_objects->grafts + pos + 1,
+  the_repository->parsed_objects->grafts_nr - pos - 1);
+   the_repository->parsed_objects->grafts_nr--;
return 0;
 }
 
diff --git a/object.h b/object.h
index 7916edb4edf..ec908f9bcc1 100644
--- a/object.h
+++ b/object.h
@@ -12,6 +12,10 @@ struct parsed_object_pool {
struct alloc_state *tag_state;
struct alloc_state *object_state;
unsigned commit_count;
+
+   /* parent substitutions from .git/info/grafts and .git/shallow */
+   struct commit_graft **grafts;
+   int grafts_alloc, grafts_nr;
 };
 
 struct parsed_object_pool *parsed_object_pool_new(void);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 06/19] commit: add repository argument to prepare_commit_graft

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow the caller of prepare_commit_graft
to be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 commit.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index b5c0aec24a0..a0400b93a1e 100644
--- a/commit.c
+++ b/commit.c
@@ -197,7 +197,8 @@ static int read_graft_file_the_repository(const char 
*graft_file)
return 0;
 }
 
-static void prepare_commit_graft(void)
+#define prepare_commit_graft(r) prepare_commit_graft_##r()
+static void prepare_commit_graft_the_repository(void)
 {
static int commit_graft_prepared;
char *graft_file;
@@ -214,7 +215,7 @@ static void prepare_commit_graft(void)
 struct commit_graft *lookup_commit_graft(const struct object_id *oid)
 {
int pos;
-   prepare_commit_graft();
+   prepare_commit_graft(the_repository);
pos = commit_graft_pos(the_repository, oid->hash);
if (pos < 0)
return NULL;
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH] alloc: allow arbitrary repositories for alloc functions

[Stefan Beller]

We have to convert all of the alloc functions at once, because alloc_report
uses a funky macro for reporting. It is better for the sake of mechanical
conversion to convert multiple functions at once rather than changing the
structure of the reporting function.

We record all memory allocation in alloc.c, and free them in
clear_alloc_state, which is called for all repositories except
the_repository.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---

Notes:

Eric, I have fixed s/relase/release/


Jonathan,

> This might seem a bit bikesheddy, but I wouldn't call it
> free_tag_buffer(), since what's being freed is not the buffer of the
> object itself, but just a string. If you want such a function, I would
> just call it release_tag_memory() to match release_commit_memory().

So you would replace the last commit with a patch like this?

Thanks,
Stefan

diff to what is currently queued:

diff --git c/commit.c w/commit.c
index 612ccf7b053..5eb4d2f08f8 100644
--- c/commit.c
+++ w/commit.c
@@ -299,9 +299,13 @@ void free_commit_buffer(struct commit *commit)

 void release_commit_memory(struct commit *c)
 {
+   c->tree = NULL;
+   c->index = 0;
free_commit_buffer(c);
free_commit_list(c->parents);
/* TODO: what about commit->util? */
+
+   c->object.parsed = 0;
 }

 const void *detach_commit_buffer(struct commit *commit, unsigned long 
*sizep)
diff --git c/object.c w/object.c
index 9d5b10d5a20..8e29f63bf23 100644
--- c/object.c
+++ w/object.c
@@ -528,7 +528,7 @@ void parsed_object_pool_clear(struct parsed_object_pool 
*o)
else if (obj->type == OBJ_COMMIT)
release_commit_memory((struct commit*)obj);
else if (obj->type == OBJ_TAG)
-   free_tag_buffer((struct tag*)obj);
+   release_tag_memory((struct tag*)obj);
}

FREE_AND_NULL(o->obj_hash);
diff --git c/tag.c w/tag.c
index 254352c30c6..7c12426b4ea 100644
--- c/tag.c
+++ w/tag.c
@@ -116,9 +116,12 @@ static timestamp_t parse_tag_date(const char *buf, 
const char *tail)
return parse_timestamp(dateptr, NULL, 10);
 }

-void free_tag_buffer(struct tag *t)
+void release_tag_memory(struct tag *t)
 {
free(t->tag);
+   t->tagged = NULL;
+   t->object.parsed = 0;
+   t->date = 0;
 }

 int parse_tag_buffer(struct tag *item, const void *data, unsigned long 
size)
diff --git c/tag.h w/tag.h
index b241fe67bc5..9057d76a506 100644
--- c/tag.h
+++ w/tag.h
@@ -15,7 +15,7 @@ struct tag {
 extern struct tag *lookup_tag(const struct object_id *oid);
 extern int parse_tag_buffer(struct tag *item, const void *data, unsigned 
long size);
 extern int parse_tag(struct tag *item);
-extern void free_tag_buffer(struct tag *t);
+extern void release_tag_memory(struct tag *t);
 extern struct object *deref_tag(struct object *, const char *, int);
 extern struct object *deref_tag_noverify(struct object *);
 extern int gpg_verify_tag(const struct object_id *oid,

 alloc.c   | 65 ++-
 alloc.h   | 19 ++
 blame.c   |  1 +
 blob.c|  1 +
 cache.h   | 16 
 commit.c  | 12 +
 commit.h  |  6 +
 merge-recursive.c |  1 +
 object.c  | 42 --
 object.h  |  8 ++
 tag.c |  9 +++
 tag.h |  1 +
 tree.c|  1 +
 13 files changed, 140 insertions(+), 42 deletions(-)
 create mode 100644 alloc.h

diff --git a/alloc.c b/alloc.c
index 277dadd221b..714df633169 100644
--- a/alloc.c
+++ b/alloc.c
@@ -4,8 +4,7 @@
  * Copyright (C) 2006 Linus Torvalds
  *
  * The standard malloc/free wastes too much space for objects, partly because
- * it maintains all the allocation infrastructure (which isn't needed, since
- * we never free an object descriptor anyway), but even more because it ends
+ * it maintains all the allocation infrastructure, but even more because it 
ends
  * up with maximal alignment because it doesn't know what the object alignment
  * for the new allocation is.
  */
@@ -15,6 +14,7 @@
 #include "tree.h"
 #include "commit.h"
 #include "tag.h"
+#include "alloc.h"
 
 #define BLOCKING 1024
 
@@ -30,8 +30,27 @@ struct alloc_state {
int count; /* total number of nodes allocated */
int nr;/* number of nodes left in current allocation */
void *p;   /* first free node in current allocation */
+
+   /* bookkeeping of allocations */
+   void **slabs;
+   int slab_nr, slab_alloc;
 };
 
+void *allocate_alloc_state(void)
+{
+   return xcalloc(1, sizeof(struct alloc_state));
+}
+
+void clear_alloc_state(struct alloc_state *s)
+{
+   while (s->slab_nr > 0) 

What would travis do?

[Stefan Beller]

Hi,

in a context unrelated to Git, I found https://github.com/grosser/wwtd
("What would travis do?"), which may be interesting for those who
tweak the travis file.

Just for your information,
Stefan

Re: [PATCH v2 08/12] commit-graph: verify commit contents against odb

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:

I finally sat down today and familiarized myself with the commit-graph
code a little. My biggest surprise was when I noticed that there is a
hash checksum at the end of the commit-graph-file. That in combination
with the tests where you flip some bytes...

It turns out, if my reading is right, that the hash value is written as
the commit-graph is generated, but that it is not verified as the
commit-graph is later read. I could not find any mention of your plans
here -- I understand why you would not want to verify the hash in
`load_commit_graph_one()`, at least not in every run. Anyway, this is
just an observation. Verifying the hash would affect the tests this
series adds. They might need to rewrite the hash or set some magic
environment variable. :-/ But that's for another day.

> +   for (i = 0; i < g->num_commits; i++) {
> +   struct commit *graph_commit, *odb_commit;
> +   struct commit_list *graph_parents, *odb_parents;
> +   int num_parents = 0;
> +   hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);

`num_commits` was derived as the commit-graph was loaded. It was derived
from offsets which were verified to be in the mmap-ed memory. So this
source address is guaranteed to be so, as well. Ok.

(Once brian's latest series hits master, this could use `oidread(...)`.)

> +   graph_commit = lookup_commit(_oid);

Do we know this comes from the graph? Even with a more-or-less-messed-up
commit graph? See below.

> +   odb_commit = (struct commit *)create_object(cur_oid.hash, 
> alloc_commit_node());
> +   if (parse_commit_internal(odb_commit, 0, 0)) {
> +   graph_report("failed to parse %s from object 
> database", oid_to_hex(_oid));
> +   continue;
> +   }
> +
> +   if (oidcmp(_commit_tree_in_graph_one(g, 
> graph_commit)->object.oid,
> +  get_commit_tree_oid(odb_commit)))

`get_commit_tree_in_graph_one()` will BUG rather than return NULL. So
this will not dereference NULL. Good. But might it hit the BUG? That is,
can we trust the commit coming out of `lookup_commit()` not to have
`graph_pos == COMMIT_NOT_FROM_GRAPH`?

> +   graph_report("root tree object ID for commit %s in 
> commit-graph is %s != %s",
> +oid_to_hex(_oid),
> +
> oid_to_hex(get_commit_tree_oid(graph_commit)),
> +
> oid_to_hex(get_commit_tree_oid(odb_commit)));
> +
> +   if (graph_commit->date != odb_commit->date)
> +   graph_report("commit date for commit %s in 
> commit-graph is %"PRItime" != %"PRItime"",
> +oid_to_hex(_oid),
> +graph_commit->date,
> +odb_commit->date);
> +
> +

(Extra blank line?)

> +   graph_parents = graph_commit->parents;
> +   odb_parents = odb_commit->parents;
> +
> +   while (graph_parents) {
> +   num_parents++;
> +
> +   if (odb_parents == NULL)
> +   graph_report("commit-graph parent list for 
> commit %s is too long (%d)",
> +oid_to_hex(_oid),
> +num_parents);
> +
> +   if (oidcmp(_parents->item->object.oid, 
> _parents->item->object.oid))
> +   graph_report("commit-graph parent for %s is 
> %s != %s",
> +oid_to_hex(_oid),
> +
> oid_to_hex(_parents->item->object.oid),
> +
> oid_to_hex(_parents->item->object.oid));
> +
> +   graph_parents = graph_parents->next;
> +   odb_parents = odb_parents->next;
> +   }
> +
> +   if (odb_parents != NULL)
> +   graph_report("commit-graph parent list for commit %s 
> terminates early",
> +oid_to_hex(_oid));

Ok, ensure the lists are equally long and compare the entries.

> +
> +   if (graph_commit->generation) {

If the commit has a generation number (not an old commit graph)...

> +   uint32_t max_generation = 0;
> +   graph_parents = graph_commit->parents;
> +
> +   while (graph_parents) {
> +   if (graph_parents->item->generation == 
> GENERATION_NUMBER_ZERO ||
> +   graph_parents->item->generation == 
> GENERATION_NUMBER_INFINITY)
> +   graph_report("commit-graph has valid 
> generation for %s but not 

Re: [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive

[Stefan Beller]

On Tue, May 15, 2018 at 1:15 PM, Leif Middelschulte
 wrote:
> Hello Stefan,
>
> thank you once again for your effort.
>
> Am 15. Mai 2018 um 22:00:34, Stefan Beller
> (sbel...@google.com(mailto:sbel...@google.com)) schrieb:
>
>> This rerolls the two commits found at [1] with the feedback of Eliah
>> and puts Leifs patch[2] on top, that I edited according to Eliahs feedback,
>> but kept Leifs ownership.
>>
>> This has addressed all of Eliahs feedback AFAICT.
>> You'll find a branch-diff below[3], which lacks
>> the new patch of Leif in that series, but is part of the reroll?
>>
>> Leif, what do you think?
>
> Seems great to me. Thank you for picking up and improving my changes :)
> One Question though: Shouldn’t an enum (like
> NOTES_MERGE_VERBOSITY_DEFAULT) be used instead of numbers?

Hah! I did not know that existed.

$ git grep NOTES_MERGE_VERBOSITY_DEFAULT
builtin/notes.c:810:o.verbosity = verbosity + NOTES_MERGE_VERBOSITY_DEFAULT;
notes-merge.c:22:   o->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT;
notes-merge.h:9:NOTES_MERGE_VERBOSITY_DEFAULT = 2,

It doesn't seem to be used much, as opposed to numbers:

$ git grep show -- merge-recursive.c
merge-recursive.c:201:static int show(struct merge_options *o, int v)
merge-recursive.c:211:  if (!show(o, v))
merge-recursive.c:570:  opts.show_rename_progress = o->show_rename_progress;
merge-recursive.c:1096: if (show(o, 3)) {
merge-recursive.c:1099: } else if (show(o, 2))
merge-recursive.c:1108: if (show(o, 3)) {
merge-recursive.c:: } else if (show(o, 2))
merge-recursive.c:2178: if (show(o, 4) || o->call_depth)
merge-recursive.c:2275: if (show(o, 4)) {
merge-recursive.c:2286: if (show(o, 5)) {
merge-recursive.c:2351: if (show(o, 2))

(The first two are the implementation of show/output, third is
somewhat unrelated to show() and all the rest is numbers).

If we'd want to use  NOTES_MERGE_VERBOSITY_DEFAULT,
I would suggest to send a followup series on top of this?

I would think numbers are fine for now.

Thanks,
Stefan

Re: [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive

[Leif Middelschulte]

Hello Stefan,

thank you once again for your effort.

Am 15. Mai 2018 um 22:00:34, Stefan Beller
(sbel...@google.com(mailto:sbel...@google.com)) schrieb:

> This rerolls the two commits found at [1] with the feedback of Eliah
> and puts Leifs patch[2] on top, that I edited according to Eliahs feedback,
> but kept Leifs ownership.
>
> This has addressed all of Eliahs feedback AFAICT.
> You'll find a branch-diff below[3], which lacks
> the new patch of Leif in that series, but is part of the reroll?
>
> Leif, what do you think?

Seems great to me. Thank you for picking up and improving my changes :)
One Question though: Shouldn’t an enum (like
NOTES_MERGE_VERBOSITY_DEFAULT) be used instead of numbers?


Cheers,


Leif

>
> Thanks,
> Stefan
>
> [1] https://public-inbox.org/git/20180510211917.138518-1-sbel...@google.com/
> [2] 
> https://public-inbox.org/git/20180514205737.21313-2-leif.middelschu...@gmail.com/
> [3] git branch-diff 
> origin/master..origin/sb/submodule-merge-in-merge-recursive 
> origin/master..HEAD >>-cover-letter.patch
>
> Leif Middelschulte (1):
> Inform about fast-forwarding of submodules during merge
>
> Stefan Beller (2):
> submodule.c: move submodule merging to merge-recursive.c
> merge-recursive: i18n submodule merge output and respect verbosity
>
> merge-recursive.c | 185 +-
> submodule.c | 168 +
> submodule.h | 6 +-
> 3 files changed, 186 insertions(+), 173 deletions(-)
>
> --
> 2.17.0.582.gccdcbd54c44.dirty
>
>
>
> 1: e022c7976ae ! 1: 3b638ccac64 submodule.c: move submodule merging to 
> merge-recursive.c
> @@ -20,7 +20,6 @@
> This commit is best viewed with --color-moved.
>
> Signed-off-by: Stefan Beller
> - Signed-off-by: Junio C Hamano
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> --- a/merge-recursive.c
> 2: 2c02ece7e01 ! 2: eb43110df9d merge-recursive: i18n submodule merge output 
> and respect verbosity
> @@ -7,7 +7,6 @@
> internationalisation as well as the verbosity setting.
>
> Signed-off-by: Stefan Beller
> - Signed-off-by: Junio C Hamano
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> --- a/merge-recursive.c
> @@ -73,10 +72,10 @@
> - fprintf(stderr, "Found a possible merge resolution "
> - "for the submodule:\n");
> + output(o, 1, _("Failed to merge submodule %s (not fast-forward)"), path);
> -+ output(o, 1, _("Found a possible merge resolution for the submodule:\n"));
> ++ output(o, 2, _("Found a possible merge resolution for the submodule:\n"));
> print_commit((struct commit *) merges.objects[0].item);
> - fprintf(stderr,
> -+ output(o, 1, _(
> ++ output(o, 2, _(
> "If this is correct simply add it to the index "
> "for example\n"
> "by using:\n\n"
> -: --- > 3: 4a3bc435023 Inform about fast-forwarding of submodules 
> during merge

Re: [PATCH] git-submodule.sh: try harder to fetch a submodule

[Jonathan Nieder]

Stefan Beller wrote:

> I'll resend it with a warning (using say()).

Thanks, makes sense.

> I think we have 2 bugs and this is merely fixing the second bug.

I'm fearing that there are more than two.

[...]
>   $ git init confused-head
>   $ (cd confused-head && git branch test \
> $(git commit-tree $(git write-tree) -m test))
>   $ git clone --no-checkout  --depth=1 \
> --separate-git-dir=test.git confused-head/.git test
> Cloning into 'test'...
> warning: --depth is ignored in local clones; use file:// instead.
> done.
>
>   $ git -C test.git config remote.origin.fetch
>   $ echo $?
> 1
>
> (A) Despite the warning of --depth having no impact, the
>   omission thereof changes the repository state.
> (B) There is no remote.origin.fetch configuration, which
>   is weird. See builtin/clone.c:830, that states for this case:

I can reproduce the issue without submodules and without --local,
as follows:

git init --bare empty.git
git init --bare almost-empty.git
git -C ~/src/git push $(pwd)/almost-empty HEAD:refs/heads/upstream

git clone --single-branch file://$(pwd)/empty.git
git clone --single-branch file://$(pwd)/almost-empty.git

git -C almost-empty.git branch -D upstream

git -C empty fetch
git -C almost-empty fetch

Expected result:
Both fetches succeed.

Actual result:
First fetch succeeds, second produces
"fatal: Couldn't find remote ref HEAD".

Note that empty.git and almost-empty.git are basically identical.
The difference instead lies in the clones' .git/config files:

diff --git 1/empty/.git/config 2/almost-empty/.git/config
index b51bb0d..ee21198 100644
--- 1/empty/.git/config
+++ 2/almost-empty/.git/config
@@ -4,7 +4,4 @@
bare = false
logallrefupdates = true
 [remote "origin"]
-   url = file:///tmp/t/empty.git
-[branch "master"]
-   remote = origin
-   merge = refs/heads/master
+   url = file:///tmp/t/almost-empty.git

Thanks,
Jonathan

Re: [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive

[Stefan Beller]

And I resent two of my earlier patches, please ignore those
(0001-grep-handle-corrupt-index-files-early.patch and
0001-git-submodule.sh-try-harder-to-fetch-a-submodule.patch)

Stefan

[PATCH 1/3] submodule.c: move submodule merging to merge-recursive.c

[Stefan Beller]

In a later patch we want to improve submodule merging by using the output()
function in merge-recursive.c for submodule merges to deliver a consistent
UI to users.

To do so we could either make the output() function globally available
so we can use it in submodule.c#merge_submodule(), or we could integrate
the submodule merging into the merging code. Choose the later as we
generally want to move submodules closer into the core.

Therefore we move any function related to merging submodules
(merge_submodule(), find_first_merges() and print_commit) to
merge-recursive.c.  We'll keep add_submodule_odb() in submodule.c as it
is used by other submodule functions. While at it, add a TODO note that
we do not really like the function add_submodule_odb().

This commit is best viewed with --color-moved.

Signed-off-by: Stefan Beller 
---
 merge-recursive.c | 166 +
 submodule.c   | 168 +-
 submodule.h   |   6 +-
 3 files changed, 170 insertions(+), 170 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 0c0d48624da..700ba15bf88 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -23,6 +23,7 @@
 #include "merge-recursive.h"
 #include "dir.h"
 #include "submodule.h"
+#include "revision.h"
 
 struct path_hashmap_entry {
struct hashmap_entry e;
@@ -977,6 +978,171 @@ static int merge_3way(struct merge_options *o,
return merge_status;
 }
 
+static int find_first_merges(struct object_array *result, const char *path,
+   struct commit *a, struct commit *b)
+{
+   int i, j;
+   struct object_array merges = OBJECT_ARRAY_INIT;
+   struct commit *commit;
+   int contains_another;
+
+   char merged_revision[42];
+   const char *rev_args[] = { "rev-list", "--merges", "--ancestry-path",
+  "--all", merged_revision, NULL };
+   struct rev_info revs;
+   struct setup_revision_opt rev_opts;
+
+   memset(result, 0, sizeof(struct object_array));
+   memset(_opts, 0, sizeof(rev_opts));
+
+   /* get all revisions that merge commit a */
+   xsnprintf(merged_revision, sizeof(merged_revision), "^%s",
+   oid_to_hex(>object.oid));
+   init_revisions(, NULL);
+   rev_opts.submodule = path;
+   /* FIXME: can't handle linked worktrees in submodules yet */
+   revs.single_worktree = path != NULL;
+   setup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, , _opts);
+
+   /* save all revisions from the above list that contain b */
+   if (prepare_revision_walk())
+   die("revision walk setup failed");
+   while ((commit = get_revision()) != NULL) {
+   struct object *o = &(commit->object);
+   if (in_merge_bases(b, commit))
+   add_object_array(o, NULL, );
+   }
+   reset_revision_walk();
+
+   /* Now we've got all merges that contain a and b. Prune all
+* merges that contain another found merge and save them in
+* result.
+*/
+   for (i = 0; i < merges.nr; i++) {
+   struct commit *m1 = (struct commit *) merges.objects[i].item;
+
+   contains_another = 0;
+   for (j = 0; j < merges.nr; j++) {
+   struct commit *m2 = (struct commit *) 
merges.objects[j].item;
+   if (i != j && in_merge_bases(m2, m1)) {
+   contains_another = 1;
+   break;
+   }
+   }
+
+   if (!contains_another)
+   add_object_array(merges.objects[i].item, NULL, result);
+   }
+
+   object_array_clear();
+   return result->nr;
+}
+
+static void print_commit(struct commit *commit)
+{
+   struct strbuf sb = STRBUF_INIT;
+   struct pretty_print_context ctx = {0};
+   ctx.date_mode.type = DATE_NORMAL;
+   format_commit_message(commit, " %h: %m %s", , );
+   fprintf(stderr, "%s\n", sb.buf);
+   strbuf_release();
+}
+
+#define MERGE_WARNING(path, msg) \
+   warning("Failed to merge submodule %s (%s)", path, msg);
+
+static int merge_submodule(struct object_id *result, const char *path,
+  const struct object_id *base, const struct object_id 
*a,
+  const struct object_id *b, int search)
+{
+   struct commit *commit_base, *commit_a, *commit_b;
+   int parent_count;
+   struct object_array merges;
+
+   int i;
+
+   /* store a in result in case we fail */
+   oidcpy(result, a);
+
+   /* we can not handle deletion conflicts */
+   if (is_null_oid(base))
+   return 0;
+   if (is_null_oid(a))
+   return 0;
+   if (is_null_oid(b))
+   return 0;
+
+   if (add_submodule_odb(path)) {
+   MERGE_WARNING(path, "not checked out");
+   

[PATCH] grep: handle corrupt index files early

[Stefan Beller]

Any other caller of 'repo_read_index' dies upon a negative return of
it, so grep should, too.

Signed-off-by: Stefan Beller 
---

Found while reviewing the series
https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/

 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 6e7bc76785a..69f0743619f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct 
repository *repo,
strbuf_addstr(, repo->submodule_prefix);
}
 
-   repo_read_index(repo);
+   if (repo_read_index(repo) < 0)
+   die("index file corrupt");
 
for (nr = 0; nr < repo->index->cache_nr; nr++) {
const struct cache_entry *ce = repo->index->cache[nr];
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 2/3] merge-recursive: i18n submodule merge output and respect verbosity

[Stefan Beller]

The submodule merge code now uses the output() function that is used by
all the rest of the merge-recursive-code. This allows for respecting
internationalisation as well as the verbosity setting.

Signed-off-by: Stefan Beller 
---
 merge-recursive.c | 33 +++--
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 700ba15bf88..0571919ee0a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1048,18 +1048,17 @@ static void print_commit(struct commit *commit)
strbuf_release();
 }
 
-#define MERGE_WARNING(path, msg) \
-   warning("Failed to merge submodule %s (%s)", path, msg);
-
-static int merge_submodule(struct object_id *result, const char *path,
+static int merge_submodule(struct merge_options *o,
+  struct object_id *result, const char *path,
   const struct object_id *base, const struct object_id 
*a,
-  const struct object_id *b, int search)
+  const struct object_id *b)
 {
struct commit *commit_base, *commit_a, *commit_b;
int parent_count;
struct object_array merges;
 
int i;
+   int search = !o->call_depth;
 
/* store a in result in case we fail */
oidcpy(result, a);
@@ -1073,21 +1072,21 @@ static int merge_submodule(struct object_id *result, 
const char *path,
return 0;
 
if (add_submodule_odb(path)) {
-   MERGE_WARNING(path, "not checked out");
+   output(o, 1, _("Failed to merge submodule %s (not checked 
out)"), path);
return 0;
}
 
if (!(commit_base = lookup_commit_reference(base)) ||
!(commit_a = lookup_commit_reference(a)) ||
!(commit_b = lookup_commit_reference(b))) {
-   MERGE_WARNING(path, "commits not present");
+   output(o, 1, _("Failed to merge submodule %s (commits not 
present)"), path);
return 0;
}
 
/* check whether both changes are forward */
if (!in_merge_bases(commit_base, commit_a) ||
!in_merge_bases(commit_base, commit_b)) {
-   MERGE_WARNING(path, "commits don't follow merge-base");
+   output(o, 1, _("Failed to merge submodule %s (commits don't 
follow merge-base)"), path);
return 0;
}
 
@@ -1116,25 +1115,24 @@ static int merge_submodule(struct object_id *result, 
const char *path,
parent_count = find_first_merges(, path, commit_a, commit_b);
switch (parent_count) {
case 0:
-   MERGE_WARNING(path, "merge following commits not found");
+   output(o, 1, _("Failed to merge submodule %s (merge following 
commits not found)"), path);
break;
 
case 1:
-   MERGE_WARNING(path, "not fast-forward");
-   fprintf(stderr, "Found a possible merge resolution "
-   "for the submodule:\n");
+   output(o, 1, _("Failed to merge submodule %s (not 
fast-forward)"), path);
+   output(o, 2, _("Found a possible merge resolution for the 
submodule:\n"));
print_commit((struct commit *) merges.objects[0].item);
-   fprintf(stderr,
+   output(o, 2, _(
"If this is correct simply add it to the index "
"for example\n"
"by using:\n\n"
"  git update-index --cacheinfo 16 %s \"%s\"\n\n"
-   "which will accept this suggestion.\n",
+   "which will accept this suggestion.\n"),
oid_to_hex([0].item->oid), path);
break;
 
default:
-   MERGE_WARNING(path, "multiple merges found");
+   output(o, 1, _("Failed to merge submodule %s (multiple merges 
found)"), path);
for (i = 0; i < merges.nr; i++)
print_commit((struct commit *) merges.objects[i].item);
}
@@ -1205,12 +1203,11 @@ static int merge_file_1(struct merge_options *o,
return ret;
result->clean = (merge_status == 0);
} else if (S_ISGITLINK(a->mode)) {
-   result->clean = merge_submodule(>oid,
+   result->clean = merge_submodule(o, >oid,
   one->path,
   >oid,
   >oid,
-  >oid,
-  !o->call_depth);
+  >oid);
} else if (S_ISLNK(a->mode)) {
switch (o->recursive_variant) {
 

[PATCH 3/3] Inform about fast-forwarding of submodules during merge

[Stefan Beller]

From: Leif Middelschulte 

Inform the user about an automatically fast-forwarded submodule. The
silent merge behavior was introduced by commit 68d03e4a6e44 ("Implement
automatic fast-forward merge for submodules", 2010-07-07)).

Signed-off-by: Leif Middelschulte 
Signed-off-by: Stefan Beller 
---
 merge-recursive.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index 0571919ee0a..29a430c418a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1093,10 +1093,26 @@ static int merge_submodule(struct merge_options *o,
/* Case #1: a is contained in b or vice versa */
if (in_merge_bases(commit_a, commit_b)) {
oidcpy(result, b);
+   if (show(o, 3)) {
+   output(o, 1, _("Fast-forwarding submodule %s to the 
following commit:"), path);
+   output_commit_title(o, commit_b);
+   } else if (show(o, 2))
+   output(o, 2, _("Fast-forwarding submodule %s to %s"), 
path, oid_to_hex(b));
+   else
+   ; /* no output */
+
return 1;
}
if (in_merge_bases(commit_b, commit_a)) {
oidcpy(result, a);
+   if (show(o, 3)) {
+   output(o, 1, _("Fast-forwarding submodule %s to the 
following commit:"), path);
+   output_commit_title(o, commit_a);
+   } else if (show(o, 2))
+   output(o, 2, _("Fast-forwarding submodule %s to %s"), 
path, oid_to_hex(a));
+   else
+   ; /* no output */
+
return 1;
}
 
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH] git-submodule.sh: try harder to fetch a submodule

[Stefan Beller]

This is the logical continuum of fb43e31f2b4 (submodule: try harder to
fetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as
some assumptions were not correct.

The commit states:
> If $sha1 was not part of the default fetch ... fail ourselves here
> assumes that the fetch_in_submodule only fails when the serverside does
> not support fetching by sha1.

There are other failures, why such a fetch may fail, such as
fatal: Couldn't find remote ref HEAD
which can happen if the remote side doesn't advertise HEAD and we do not
have a local fetch refspec.

Not advertising HEAD is allowed by the protocol spec and would happen,
if HEAD points at an unborn branch for example.

Not having a local fetch refspec can happen when submodules are fetched
shallowly, as then git-clone doesn't setup a fetch refspec.

So do try even harder for a submodule by ignoring the exit code of the
first fetch and rather relying on the following is_tip_reachable to
see if we try fetching again.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963ca2..00fcd69138f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -614,7 +614,7 @@ cmd_update()
# is not reachable from a ref.
is_tip_reachable "$sm_path" "$sha1" ||
fetch_in_submodule "$sm_path" $depth ||
-   die "$(eval_gettext "Unable to fetch in 
submodule path '\$displaypath'")"
+   say "$(eval_gettext "Unable to fetch in 
submodule path '\$displaypath'")"
 
# Now we tried the usual fetch, but $sha1 may
# not be reachable from any of the refs
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive

[Stefan Beller]

This rerolls the two commits found at [1] with the feedback of Eliah
and puts Leifs patch[2] on top, that I edited according to Eliahs feedback,
but kept Leifs ownership. 

This has addressed all of Eliahs feedback AFAICT.
You'll find a branch-diff below[3], which lacks
the new patch of Leif in that series, but is part of the reroll?

Leif, what do you think?

Thanks,
Stefan

[1] https://public-inbox.org/git/20180510211917.138518-1-sbel...@google.com/
[2] 
https://public-inbox.org/git/20180514205737.21313-2-leif.middelschu...@gmail.com/
[3] git branch-diff origin/master..origin/sb/submodule-merge-in-merge-recursive 
origin/master..HEAD  >>-cover-letter.patch

Leif Middelschulte (1):
  Inform about fast-forwarding of submodules during merge

Stefan Beller (2):
  submodule.c: move submodule merging to merge-recursive.c
  merge-recursive: i18n submodule merge output and respect verbosity

 merge-recursive.c | 185 +-
 submodule.c   | 168 +
 submodule.h   |   6 +-
 3 files changed, 186 insertions(+), 173 deletions(-)

-- 
2.17.0.582.gccdcbd54c44.dirty



1:  e022c7976ae ! 1:  3b638ccac64 submodule.c: move submodule merging to 
merge-recursive.c
@@ -20,7 +20,6 @@
 This commit is best viewed with --color-moved.
 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
 diff --git a/merge-recursive.c b/merge-recursive.c
 --- a/merge-recursive.c
2:  2c02ece7e01 ! 2:  eb43110df9d merge-recursive: i18n submodule merge output 
and respect verbosity
@@ -7,7 +7,6 @@
 internationalisation as well as the verbosity setting.
 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
 diff --git a/merge-recursive.c b/merge-recursive.c
 --- a/merge-recursive.c
@@ -73,10 +72,10 @@
 -  fprintf(stderr, "Found a possible merge resolution "
 -  "for the submodule:\n");
 +  output(o, 1, _("Failed to merge submodule %s (not 
fast-forward)"), path);
-+  output(o, 1, _("Found a possible merge resolution for the 
submodule:\n"));
++  output(o, 2, _("Found a possible merge resolution for the 
submodule:\n"));
print_commit((struct commit *) merges.objects[0].item);
 -  fprintf(stderr,
-+  output(o, 1, _(
++  output(o, 2, _(
"If this is correct simply add it to the index "
"for example\n"
"by using:\n\n"
-:  --- > 3:  4a3bc435023 Inform about fast-forwarding of submodules 
during merge

[PATCH] git-submodule.sh: try harder to fetch a submodule

[Stefan Beller]

This is the logical continuum of fb43e31f2b4 (submodule: try harder to
fetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as
some assumptions were not correct.

The commit states:
> If $sha1 was not part of the default fetch ... fail ourselves here
> assumes that the fetch_in_submodule only fails when the serverside does
> not support fetching by sha1.

There are other failures, why such a fetch may fail, such as
fatal: Couldn't find remote ref HEAD
which can happen if the remote side doesn't advertise HEAD and we do not
have a local fetch refspec.

Not advertising HEAD is allowed by the protocol spec and would happen,
if HEAD points at an unborn branch for example.

Not having a local fetch refspec can happen when submodules are fetched
shallowly, as then git-clone doesn't setup a fetch refspec.

So do try even harder for a submodule by ignoring the exit code of the
first fetch and rather relying on the following is_tip_reachable to
see if we try fetching again.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963ca2..00fcd69138f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -614,7 +614,7 @@ cmd_update()
# is not reachable from a ref.
is_tip_reachable "$sm_path" "$sha1" ||
fetch_in_submodule "$sm_path" $depth ||
-   die "$(eval_gettext "Unable to fetch in 
submodule path '\$displaypath'")"
+   say "$(eval_gettext "Unable to fetch in 
submodule path '\$displaypath'")"
 
# Now we tried the usual fetch, but $sha1 may
# not be reachable from any of the refs
-- 
2.17.0.582.gccdcbd54c44.dirty

Re: [PATCH] fast-export: avoid NULL pointer arithmetic

[René Scharfe]

Am 14.05.2018 um 03:37 schrieb Junio C Hamano:
> René Scharfe  writes:
> 
>> Storing integer values in pointers is a trick that seems to have worked
>> so far for fast-export.  A portable way to avoid that trick without
>> requiring more memory would be to use a union.
>>
>> Or we could roll our own custom hash map, as I mused in an earlier post.
>> That would duplicate quite a bit of code; are there reusable pieces
>> hidden within that could be extracted into common functions?
> 
> Hmm, this together with your follow-up does not look too bad, but it
> does introduce quite a lot of code that could be refactored, so I am
> not sure if I really like it or not.

Putting keys and values into separate arrays probably causes stores and
lookups to hit (at least) two cache lines instead of just one.  Not sure
how much of an impact that has on the overall performance (probably not
much), but we'd need at least a perf test for that.

And we have enough hash map implementations already.

Casting should be good enough for now, to avoid the compiler warning.

René

Re: [PATCH] git-submodule.sh: try harder to fetch a submodule

[Stefan Beller]

On Fri, May 11, 2018 at 5:03 PM, Junio C Hamano  wrote:
>> A more typical example would be if the ref simply doesn't exist (i.e.,
>> is a branch yet to be born).
>
> Indeed this is interesting.  At first glance I thought this was
> about underlying "git clone" failing to grab things from a
> repository with unborn HEAD, but that part works perfectly OK.

ok.

> So it probably should be more like
>
> guard1 || action1 || warn
> guard2 || action2 || die
>
> so that no matter what the outcome of the action1 is, the second set
> gets executed.
>

I'll resend it with a warning (using say()).

I think we have 2 bugs and this is merely fixing the second bug.

The first bug:
We had a call chain as
  git clone --recurse-submodules=
-> git submodule update --init --recursive $(pathspec)
  -> git submodule--helper update-clone # will clone
-> git submodule helper clone
  -> git clone --no-checkout --separate-git-dir ...

The call to the "git clone" produces an interesting
submodule state:

  $ git init confused-head
  $ (cd confused-head && git branch test \
$(git commit-tree $(git write-tree) -m test))
  $ git clone --no-checkout  --depth=1 \
--separate-git-dir=test.git confused-head/.git test
Cloning into 'test'...
warning: --depth is ignored in local clones; use file:// instead.
done.

  $ git -C test.git config remote.origin.fetch
  $ echo $?
1

(A) Despite the warning of --depth having no impact, the
  omission thereof changes the repository state.
(B) There is no remote.origin.fetch configuration, which
  is weird. See builtin/clone.c:830, that states for this case:

/*
 * otherwise, the next "git fetch" will
 * simply fetch from HEAD without updating
 * any remote-tracking branch, which is what
 * we want.
 */

I disagree as the next fetch will be confused
(HEAD is not advertised on the next ls-remote)

The patch that is under discussion here is merely
papering over the effect of having no fetch spec,
by allowing the second fetch (fetching the sha1 directly)
to run, which ignores the configuration as a refspec is
given.

However it is still a bug, as such repositories are out there,
which is why I said there are 2 bugs initially. It's just that
the first bug enables the second bug.

Thanks,
Stefan

Re: [PATCH 04/35] refspec: introduce struct refspec

[Brandon Williams]

On 05/15, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > diff --git a/refspec.h b/refspec.h
> > index 173cea882..f6fb251f3 100644
> > --- a/refspec.h
> > +++ b/refspec.h
> > @@ -20,4 +20,29 @@ struct refspec_item *parse_push_refspec(int nr_refspec, 
> > const char **refspec);
> >  
> >  void free_refspec(int nr_refspec, struct refspec_item *refspec);
> >  
> > +#define REFSPEC_FETCH 1
> > +#define REFSPEC_PUSH 0
> 
> The reversed order of these two definitions looks somewhat unusual.
> I guess the reason why you made FETCH true and PUSH false is
> probably because quite a lot of places in the existing code we do

Yeah I really just made the choice based on what the existing logic did
(parse_refspec takes in a parameter 'fetch' which is 1 if we are parsing
the refspec for a fetch and 0 for push).  So it wouldn't be too
difficult to go and make this change here since all future callers use
the #defines themselves, because it is significantly easier to read
'REFSPEC_PUSH' than to read a 0 like you point out below :)

> 
>   if (fetch)
>   do the fetch thing;
>   else
>   do the push thing;
> 
> i.e. "fetch" variable is used as "is this a fetch: yes/no?"
> boolean, and a caller that mysteriously passes "1" (or "0")
> is harder to read than necessary.  Being able to pass REFSPEC_PUSH
> instead of "0" would certainly make the caller easier to read.  But
> as long as the variable is understood as "is_fetch? Yes/no", the
> caller can pass Yes or No and be still descriptive enough.
> 
> I think the way to make such a code more readable would not be to
> rewrite the above to
> 
>   if (fetch_or_push)
>   do the fetch thing;
>   else
>   do the push thing;
> 
> Rather it would be 
> 
>   if (fetch_or_push == REFSPEC_FETCH)
>   do the fetch thing;
>   else
>   do the push thing;
> 
> And once you have gone that far, the actual "enum" value assignment
> no longer makes difference.
> 
> > +#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH }
> > +#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH }
> > +
> > +struct refspec {
> > +   struct refspec_item *items;
> > +   int alloc;
> > +   int nr;
> > +
> > +   const char **raw;
> > +   int raw_alloc;
> > +   int raw_nr;
> > +
> > +   int fetch;
> > +};
> > +
> > +void refspec_item_init(struct refspec_item *item, const char *refspec, int 
> > fetch);
> > +void refspec_item_clear(struct refspec_item *item);
> > +void refspec_init(struct refspec *rs, int fetch);
> > +void refspec_append(struct refspec *rs, const char *refspec);
> > +void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
> > +void refspec_clear(struct refspec *rs);
> > +
> >  #endif /* REFSPEC_H */

-- 
Brandon Williams

Re: [PATCH 03/35] refspec: rename struct refspec to struct refspec_item

[Brandon Williams]

On 05/15, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > In preperation for introducing an abstraction around a collection of
> > refspecs (much like how a 'struct pathspec' is a collection of 'struct
> > pathspec_item's) rename the existing 'struct refspec' to 'struct
> > refspec_item'.
> 
> Makes sense.
> 
> >  /* See TAG_REFSPEC for the string version */
> > -const struct refspec *tag_refspec = _tag_refspec;
> > +const struct refspec_item *tag_refspec = _tag_refspec;
> >  
> >  /*
> >   * Parses 'refspec' and populates 'rs'.  returns 1 if successful and 0 if 
> > the
> >   * refspec is invalid.
> >   */
> > -static int parse_refspec(struct refspec *rs, const char *refspec, int 
> > fetch)
> > +static int parse_refspec(struct refspec_item *rs, const char *refspec, int 
> > fetch)
> 
> The new comment given to the function in the previous step talks
> in terms of the variable name and not type, so technically it is
> still valid after this change without updating.
> 
> I however wonder if it makes more sense to go one step further and
> rename the "const char *" variable that is a string form of a single
> refspec item to something other than "refspec".  Which would
> invalidate the commit you gave to the function and make it necessary
> to be updated in this step.
> 
> I wonder if the early part of the series becomes easier to read if
> patches 2 and 3 are swapped.  That may result in less code/comment
> churn.

I'll go ahead and switch these two patches and fixup the comment and
function name.

-- 
Brandon Williams

Re: git diff: meaning of ^M at line ends ?

[Frank Schäfer]

Am 14.05.2018 um 20:13 schrieb Torsten Bögershausen:
> ^M is the representation of a "Carriage Return" or CR.
> Under Linux/Unix/Mac OS X a line is terminated with a single
> "line feed", LF.
>
> Windows typically uses CRLF at the end of the line.
> "git diff" uses the LF to detect the end of line,
> leaving the CR alone.
>
> Nothing to worry about.
Thanks, I already suspected something like that.
Has this behavior been changed/added recently ?
I didn't observe it before, although the project I'm currently looking
into has always been using CR+LF...

Why does the ^M only show up in '+' lines ?
When changing the line end from CR+LF to LF, the diff looks like this:

-blahblah
+blahblah

But I would expect it to be

-blahblah^M
+blahblah


Regards,
Frank

> If you want, you can commit those files with
> CRLF in the working tree, and LF in the repo.
>
> More information may be found here:
>
> https://git-scm.com/docs/gitattributes
>
> (Or ask more questions here, if needed)

Re: [PATCH 00/35] refactoring refspecs

[Brandon Williams]

On 05/15, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, May 14 2018, Brandon Williams wrote:
> 
> > When working on protocol v2 I noticed that working with refspecs was a
> > little difficult because of the various api's that existed.  Some
> > functions expected an array of "const char *" while others expected an
> > array of "struct refspec".  In all cases a length parameter needed to be
> > passed as a parameter as well.  This makes working with refspecs a
> > little challenging because of the different expectations different parts
> > of the code base have.
> >
> > This series refactors how refspecs are handled through out the code base
> > by renaming the existing struct refspec to refspec_item and introducing
> > a new 'struct refspec' which is a container of refspec_items, much like
> > how a pathspec contains pathspec_items.  This simplifies many callers
> > and makes handling pathspecs a bit easier.
> 
> This looks really good to me. The API you're replacing is one of the
> worst I've had a chance to encounter in git.git (as noted in my
> https://public-inbox.org/git/87in7p2ucb@evledraar.gmail.com/ but
> maybe I haven't looked widely enough), and now it's really nice.

Thanks! Yeah its one of the rougher edges I've worked with and I'm glad
I finally got around to fixing it.

> 
> > I have some follow on work which I'll build on top of this series, but
> > since this was already getting a bit lengthy at 35 patches I'll save
> > that for another time.
> 
> In addition to my other suggestions for stuff to put on top, which I see
> now you may have just had in your local tree but didn't submit, I think
> this makes sense:

Yes these changes make sense, though I'll need to tweak them to avoid
some memory leaks.  I'll probably go ahead and squash it into the two
patches which effect those two functions.

Thanks for catching this.

> 
> diff --git a/remote.c b/remote.c
> index 946b95d18d..cb97e662e8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -77,16 +77,6 @@ static const char *alias_url(const char *url, struct 
> rewrites *r)
>   return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len);
>  }
> 
> -static void add_push_refspec(struct remote *remote, const char *ref)
> -{
> - refspec_append(>push, ref);
> -}
> -
> -static void add_fetch_refspec(struct remote *remote, const char *ref)
> -{
> - refspec_append(>fetch, ref);
> -}
> -
>  static void add_url(struct remote *remote, const char *url)
>  {
>   ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
> @@ -261,9 +251,9 @@ static void read_remotes_file(struct remote *remote)
>   if (skip_prefix(buf.buf, "URL:", ))
>   add_url_alias(remote, xstrdup(skip_spaces(v)));
>   else if (skip_prefix(buf.buf, "Push:", ))
> - add_push_refspec(remote, xstrdup(skip_spaces(v)));
> + refspec_append(>push, xstrdup(skip_spaces(v)));
>   else if (skip_prefix(buf.buf, "Pull:", ))
> - add_fetch_refspec(remote, xstrdup(skip_spaces(v)));
> + refspec_append(>fetch, xstrdup(skip_spaces(v)));
>   }
>   strbuf_release();
>   fclose(f);
> @@ -302,14 +292,14 @@ static void read_branches_file(struct remote 
> *remote)
>   frag = "master";
> 
>   add_url_alias(remote, strbuf_detach(, NULL));
> - add_fetch_refspec(remote, xstrfmt("refs/heads/%s:refs/heads/%s",
> -   frag, remote->name));
> + refspec_append(>fetch, xstrfmt("refs/heads/%s:refs/heads/%s",
> +frag, remote->name));
> 
>   /*
>* Cogito compatible push: push current HEAD to remote #branch
>* (master if missing)
>*/
> - add_push_refspec(remote, xstrfmt("HEAD:refs/heads/%s", frag));
> + refspec_append(>push, xstrfmt("HEAD:refs/heads/%s", frag));
>   remote->fetch_tags = 1; /* always auto-follow */
>  }
> 
> @@ -395,12 +385,12 @@ static int handle_config(const char *key, const 
> char *value, void *cb)
>   const char *v;
>   if (git_config_string(, key, value))
>   return -1;
> - add_push_refspec(remote, v);
> + refspec_append(>push, v);
>   } else if (!strcmp(subkey, "fetch")) {
>   const char *v;
>   if (git_config_string(, key, value))
>   return -1;
> - add_fetch_refspec(remote, v);
> + refspec_append(>fetch, v);
>   } else if (!strcmp(subkey, "receivepack")) {
>   const char *v;
>   if (git_config_string(, key, value))
> 
> I.e. the reason we have add_{push,fetch}_refspec() in the first place is
> because without your series it's tricky to add new ones, but now it's
> trivial, so let's not leave behind wrapper static functions whose sole
> purpose 

Re: [PATCH 14/35] remote: convert fetch refspecs to struct refspec

[Brandon Williams]

On 05/15, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, May 14 2018, Brandon Williams wrote:
> 
> >  void add_prune_tags_to_fetch_refspec(struct remote *remote)
> >  {
> > -   int nr = remote->fetch_refspec_nr;
> > -   int bufsize = nr  + 1;
> > -   int size = sizeof(struct refspec_item);
> > -
> > -   remote->fetch = xrealloc(remote->fetch, size  * bufsize);
> > -   memcpy(>fetch[nr], tag_refspec, size);
> > -   add_fetch_refspec(remote, xstrdup(TAG_REFSPEC));
> > +   refspec_append(>fetch, TAG_REFSPEC);
> >  }
> 
> Thanks for fixing the hack I needed to put in place in 97716d217c
> ("fetch: add a --prune-tags option and fetch.pruneTags config",
> 2018-02-09).
> 
> I'm not sure where it belongs in this series, but I think this makes
> sense on top of the whole thing:

This actually would work well immediately after this patch, so I'll add
it here :)

Thanks!

> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index af7064dce3..9a523249f5 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1383,7 +1383,8 @@ static int fetch_one(struct remote *remote, int 
> argc, const char **argv, int pru
> 
> maybe_prune_tags = prune_tags_ok && prune_tags;
> if (maybe_prune_tags && remote_via_config)
> -   add_prune_tags_to_fetch_refspec(remote);
> +   refspec_append(>fetch, TAG_REFSPEC);
> +
> 
> if (maybe_prune_tags && (argc || !remote_via_config))
> refspec_append(, TAG_REFSPEC);
> diff --git a/remote.c b/remote.c
> index 8e6522f4d0..946b95d18d 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -87,11 +87,6 @@ static void add_fetch_refspec(struct remote *remote, 
> const char *ref)
> refspec_append(>fetch, ref);
>  }
> 
> -void add_prune_tags_to_fetch_refspec(struct remote *remote)
> -{
> -   refspec_append(>fetch, TAG_REFSPEC);
> -}
> -
>  static void add_url(struct remote *remote, const char *url)
>  {
> ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
> diff --git a/remote.h b/remote.h
> index 9014f707f0..62a6566594 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -289,6 +289,4 @@ extern int parseopt_push_cas_option(const struct 
> option *, const char *arg, int
>  extern int is_empty_cas(const struct push_cas_option *);
>  void apply_push_cas(struct push_cas_option *, struct remote *, struct 
> ref *);
> 
> -void add_prune_tags_to_fetch_refspec(struct remote *remote);
> -
>  #endif
> 
> I.e. the whole reason we have this function is because of my above
> commit where I had to very carefully hack around the fact that we didn't
> have something which could ALLOW_GROW() the structure after it had been
> created.
> 
> So I added the add_prune_tags_to_fetch_refspec() function to very
> carefully do *only* that so others wouldn't be tempted to use this hack
> more generally.
> 
> But now we have a nice API for it, so we can just throw away the
> wrapper, and use the same API everywhere. You already did the other half
> of that in your e69b54f53a ("fetch: convert fetch_one to use struct
> refspec", 2018-05-11).

-- 
Brandon Williams

Re: [PATCH] grep: handle corrupt index files early

[Brandon Williams]

On 05/14, Stefan Beller wrote:
> Any other caller of 'repo_read_index' dies upon a negative return of
> it, so grep should, too.
> 
> Signed-off-by: Stefan Beller 
> ---
> 
> Found while reviewing the series
> https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/
> 
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 6e7bc76785a..69f0743619f 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct 
> repository *repo,
>   strbuf_addstr(, repo->submodule_prefix);
>   }
>  
> - repo_read_index(repo);
> + if (repo_read_index(repo) < 0)
> + die("index file corrupt");

Looks good to me!

>  
>   for (nr = 0; nr < repo->index->cache_nr; nr++) {
>   const struct cache_entry *ce = repo->index->cache[nr];
> -- 
> 2.17.0.582.gccdcbd54c44.dirty
> 

-- 
Brandon Williams

Re: [PATCH 35/35] submodule: convert push_unpushed_submodules to take a struct refspec

[Brandon Williams]

On 05/15, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, May 14 2018, Brandon Williams wrote:
> 
> > Convert 'push_unpushed_submodules()' to take a 'struct refspec' as a
> > parameter instead of an array of 'const char *'.
> > [...]
> > diff --git a/submodule.h b/submodule.h
> > index e5526f6aa..aae0c9c8f 100644
> > --- a/submodule.h
> > +++ b/submodule.h
> > @@ -100,9 +100,10 @@ extern int submodule_touches_in_range(struct object_id 
> > *a,
> >  extern int find_unpushed_submodules(struct oid_array *commits,
> > const char *remotes_name,
> > struct string_list *needs_pushing);
> > +struct refspec;
> >  extern int push_unpushed_submodules(struct oid_array *commits,
> > const struct remote *remote,
> > -   const char **refspec, int refspec_nr,
> > +   const struct refspec *rs,
> > const struct string_list *push_options,
> > int dry_run);
> >  /*
> 
> Why do you prefer doing this to having this on top?:
> 
> diff --git a/submodule.h b/submodule.h
> index aae0c9c8ff..c3f206ce17 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -1,5 +1,6 @@
>  #ifndef SUBMODULE_H
>  #define SUBMODULE_H
> +#include "refspec.h"
>  
>  struct repository;
>  struct diff_options;
> @@ -100,7 +101,6 @@ extern int submodule_touches_in_range(struct 
> object_id *a,
>  extern int find_unpushed_submodules(struct oid_array *commits,
> const char *remotes_name,
> struct string_list *needs_pushing);
> -struct refspec;
>  extern int push_unpushed_submodules(struct oid_array *commits,
> const struct remote *remote,
> const struct refspec *rs,

Basically for the reason that stefan pointed out, though in practice I
don't know how much that would actually impact compile times given we
already are including cache.h and a bunch of others everywhere.

-- 
Brandon Williams

Re: [PATCH 35/35] submodule: convert push_unpushed_submodules to take a struct refspec

[Stefan Beller]

On Tue, May 15, 2018 at 1:11 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Mon, May 14 2018, Brandon Williams wrote:
>
>> Convert 'push_unpushed_submodules()' to take a 'struct refspec' as a
>> parameter instead of an array of 'const char *'.
>> [...]
>> diff --git a/submodule.h b/submodule.h
>> index e5526f6aa..aae0c9c8f 100644
>> --- a/submodule.h
>> +++ b/submodule.h
>> @@ -100,9 +100,10 @@ extern int submodule_touches_in_range(struct object_id 
>> *a,
>>  extern int find_unpushed_submodules(struct oid_array *commits,
>>   const char *remotes_name,
>>   struct string_list *needs_pushing);
>> +struct refspec;
>>  extern int push_unpushed_submodules(struct oid_array *commits,
>>   const struct remote *remote,
>> - const char **refspec, int refspec_nr,
>> + const struct refspec *rs,
>>   const struct string_list *push_options,
>>   int dry_run);
>>  /*
>
> Why do you prefer doing this to having this on top?:

The fewer includes in header files the better, as then the headers
themselves don't have dependencies. (Otherwise we'd end up
multiplying cache.h ;)

In the source files we have to include all needed headers, but for
the headers, it is better if we can just get away with declaring the
existence of a struct.

This way we reduce compile time, so I'd am not keen on your patch
on top.

This is discussed a lot on stackoverflow, e.g.:
https://softwareengineering.stackexchange.com/questions/195806/forward-declaration-vs-include
https://stackoverflow.com/a/15828094

Stefan

Re: [PATCH 01/35] refspec: move refspec parsing logic into its own file

[Brandon Williams]

On 05/15, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > In preperation for performing a refactor on refspec related code, move
> 
> Preparation?

Oops, I'll fix that.


-- 
Brandon Williams

Re: [PATCH] grep: handle corrupt index files early

[Stefan Beller]

On Tue, May 15, 2018 at 6:13 AM, Duy Nguyen  wrote:
> On Tue, May 15, 2018 at 3:04 AM, Stefan Beller  wrote:
>> Any other caller of 'repo_read_index' dies upon a negative return of
>> it, so grep should, too.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>
>> Found while reviewing the series
>> https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/
>>
>>  builtin/grep.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index 6e7bc76785a..69f0743619f 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct 
>> repository *repo,
>> strbuf_addstr(, repo->submodule_prefix);
>> }
>>
>> -   repo_read_index(repo);
>> +   if (repo_read_index(repo) < 0)
>> +   die("index file corrupt");
>
> _() the string (and maybe reuse an existing phrase if found to reduce
> workload on translators)

sbeller@sbeller:/u/git$ git grep -A 1 repo_read_index
builtin/grep.c:491: if (repo_read_index(repo) < 0)
builtin/grep.c-492- die("index file corrupt");
--
builtin/ls-files.c:213: if (repo_read_index() < 0)
builtin/ls-files.c-214- die("index file corrupt");
--
builtin/ls-files.c:582: if (repo_read_index(the_repository) < 0)
builtin/ls-files.c-583- die("index file corrupt");
--
dir.c:3028: if (repo_read_index() < 0)
dir.c-3029- die("index file corrupt in repo %s", subrepo.gitdir);
--
repository.c:245:int repo_read_index(struct repository *repo)
repository.c-246-{
--
repository.h:70: * 'repo_read_index()' can be used to populate 'index'.
repository.h-71- */
--
repository.h:119:extern int repo_read_index(struct repository *repo);
repository.h-120-
--
submodule-config.c:583: if (repo_read_index(repo) < 0)
submodule-config.c-584- return;
--
submodule.c:1336:   if (repo_read_index(r) < 0)
submodule.c-1337-   die("index file corrupt");

I think this is as good as it gets for using an existing phrase.
None of them are translated, which I would defer to a follow up patch
that translates all(?) of them or just the porcelains.

Thanks,
Stefan

Re: [PATCH] grep: handle corrupt index files early

[Duy Nguyen]

On Tue, May 15, 2018 at 3:04 AM, Stefan Beller  wrote:
> Any other caller of 'repo_read_index' dies upon a negative return of
> it, so grep should, too.
>
> Signed-off-by: Stefan Beller 
> ---
>
> Found while reviewing the series
> https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/
>
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 6e7bc76785a..69f0743619f 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct 
> repository *repo,
> strbuf_addstr(, repo->submodule_prefix);
> }
>
> -   repo_read_index(repo);
> +   if (repo_read_index(repo) < 0)
> +   die("index file corrupt");

_() the string (and maybe reuse an existing phrase if found to reduce
workload on translators)

>
> for (nr = 0; nr < repo->index->cache_nr; nr++) {
> const struct cache_entry *ce = repo->index->cache[nr];
> --
> 2.17.0.582.gccdcbd54c44.dirty
>



-- 
Duy

[no subject]

[Joseph Obieke]

¿Todo lo que necesito es una persona honesta en quien pueda confiar?
¿Puedo confiar en que transfiera la suma de $ 12.500.000.00 millones
de dólares estadounidenses a su cuenta si es posible, contáctenos para
obtener más detalles a través de mi dirección de correo electrónico
privada jobiekeoff...@gmail.com
Trate muy urgente y confidencialmente con confianza y honestamente y
usted compartirá el fondo una vez que se transfiera a su cuenta porque
el gobierno está haciendo un avión para confiscar el fondo.
Esperando con urgencia para saber de ti.
Mr Joseph Obieke

Re: [PATCH] grep: handle corrupt index files early

[Johannes Schindelin]

Hi Stefan,

On Mon, 14 May 2018, Stefan Beller wrote:

> Any other caller of 'repo_read_index' dies upon a negative return of
> it, so grep should, too.
> 
> Signed-off-by: Stefan Beller 
> ---
> 
> Found while reviewing the series
> https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/
> 
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 6e7bc76785a..69f0743619f 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct 
> repository *repo,
>   strbuf_addstr(, repo->submodule_prefix);
>   }
>  
> - repo_read_index(repo);
> + if (repo_read_index(repo) < 0)
> + die("index file corrupt");

Looks obviously correct, thanks!

Ciao,
Dscho

[PATCHv3 1/1] git-p4: add unshelve command

[Luke Diamand]

This can be used to "unshelve" a shelved P4 commit into
a git commit.

For example:

  $ git p4 unshelve 12345

The resulting commit ends up in the branch:
   refs/remotes/p4/unshelved/12345

If that branch already exists, it is renamed - for example
the above branch would be saved as p4/unshelved/12345.1.

git-p4 checks that the shelved changelist is based on files
which are at the same Perforce revision as the origin branch
being used for the unshelve (HEAD by default). If they are not,
it will refuse to unshelve. This is to ensure that the unshelved
change does not contain other changes mixed-in.

The reference branch can be changed manually with the "--origin"
option.

The change adds a new Unshelve command class. This just runs the
existing P4Sync code tweaked to handle a shelved changelist.

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt |  32 ++
 git-p4.py| 207 ---
 t/t9832-unshelve.sh  | 153 +
 3 files changed, 356 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..d3cb249fc2 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -164,6 +164,31 @@ $ git p4 submit --shelve
 $ git p4 submit --update-shelve 1234 --update-shelve 2345
 
 
+
+Unshelve
+
+Unshelving will take a shelved P4 changelist, and produce the equivalent git 
commit
+in the branch refs/remotes/p4/unshelved/.
+
+The git commit is created relative to the current origin revision (HEAD by 
default).
+If the shelved changelist's parent revisions differ, git-p4 will refuse to 
unshelve;
+you need to be unshelving onto an equivalent tree.
+
+The origin revision can be changed with the "--origin" option.
+
+If the target branch in refs/remotes/p4/unshelved already exists, the old one 
will
+be renamed.
+
+
+$ git p4 sync
+$ git p4 unshelve 12345
+$ git show refs/remotes/p4/unshelved/12345
+
+$ git p4 unshelve 12345
+
+
+
+
 OPTIONS
 ---
 
@@ -337,6 +362,13 @@ These options can be used to modify 'git p4 rebase' 
behavior.
 --import-labels::
Import p4 labels.
 
+Unshelve options
+
+
+--origin::
+Sets the git refspec against which the shelved P4 changelist is compared.
+Defaults to p4/master.
+
 DEPOT PATH SYNTAX
 -
 The p4 depot path argument to 'git p4 sync' and 'git p4 clone' can
diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc69..dcff31a1d4 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -316,12 +316,17 @@ def p4_last_change():
 results = p4CmdList(["changes", "-m", "1"], skip_info=True)
 return int(results[0]['change'])
 
-def p4_describe(change):
+def p4_describe(change, shelved=False):
 """Make sure it returns a valid result by checking for
the presence of field "time".  Return a dict of the
results."""
 
-ds = p4CmdList(["describe", "-s", str(change)], skip_info=True)
+cmd = ["describe", "-s"]
+if shelved:
+cmd += ["-S"]
+cmd += [str(change)]
+
+ds = p4CmdList(cmd, skip_info=True)
 if len(ds) != 1:
 die("p4 describe -s %d did not return 1 result: %s" % (change, 
str(ds)))
 
@@ -662,6 +667,12 @@ def gitBranchExists(branch):
 stderr=subprocess.PIPE, stdout=subprocess.PIPE);
 return proc.wait() == 0;
 
+def gitUpdateRef(ref, newvalue):
+subprocess.check_call(["git", "update-ref", ref, newvalue])
+
+def gitDeleteRef(ref):
+subprocess.check_call(["git", "update-ref", "-d", ref])
+
 _gitConfig = {}
 
 def gitConfig(key, typeSpecifier=None):
@@ -2411,6 +2422,7 @@ class P4Sync(Command, P4UserMap):
 self.tempBranches = []
 self.tempBranchLocation = "refs/git-p4-tmp"
 self.largeFileSystem = None
+self.suppress_meta_comment = False
 
 if gitConfig('git-p4.largeFileSystem'):
 largeFileSystemConstructor = 
globals()[gitConfig('git-p4.largeFileSystem')]
@@ -2421,6 +2433,18 @@ class P4Sync(Command, P4UserMap):
 if gitConfig("git-p4.syncFromOrigin") == "false":
 self.syncWithOrigin = False
 
+self.depotPaths = []
+self.changeRange = ""
+self.previousDepotPaths = []
+self.hasOrigin = False
+
+# map from branch depot path to parent branch
+self.knownBranches = {}
+self.initialParents = {}
+
+self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 
3600) / 60))
+self.labels = {}
+
 # Force a checkpoint in fast-import and wait for it to finish
 def checkpoint(self):
 self.gitStream.write("checkpoint\n\n")
@@ -2429,7 +2453,20 @@ class P4Sync(Command, P4UserMap):
 if self.verbose:
 print "checkpoint finished: " + out
 
-def extractFilesFromCommit(self, commit):
+def cmp_shelved(self, path, filerev, revision):
+""" Determine if a path at revision 

[PATCHv3 0/1] git-p4 unshelve

[Luke Diamand]

This is a git-p4 unshelve command which detects when extra
changes are going to be included, and refuses to unhshelve.

As in my earlier unshelve change, this uses git fast-import
to do the actual delta generation, but now checks to see
if the files unshelved are based on the same revisions that
fast-import will be using, using the revision numbers in
the "p4 describe -S" command output. If they're different,
it refuses to unshelve.

That makes it safe to use, rather than potentially generating
deltas that contain bits from other changes.

I have added a test for this case.

Luke Diamand (1):
  git-p4: add unshelve command

 Documentation/git-p4.txt |  32 ++
 git-p4.py| 207 ---
 t/t9832-unshelve.sh  | 153 +
 3 files changed, 356 insertions(+), 36 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

-- 
2.17.0.392.gdeb1a6e9b7

Re: [RFC] Other chunks for commit-graph, part 1 - Bloom filters, topo order, etc.

[Johannes Schindelin]

Hi Kuba,

On Mon, 14 May 2018, Jakub Narebski wrote:

> [... lots and lots of discussions...]
>
> All right.
> 
> Here is my [allegedly] improved version, which assumes that we always
> want to start from commit with maximum generation number (there may be
> more than one such commit).
> 
> Let's add one more data structure:
> 
>   RRQ : A queue, for storing remaining commits from R (starting point).
>   It is a priority queue ordered by generation number.
> 
> 
> InitializeTopoOrder(R):
>     Initialize IDQ, IDV, TOQ, RRQ
> 
>     max_generation = 0
>     visit_order = 0
> 
>     for each commit c in R:
>         max_generation = max(max_generation, generation(c))
>         unless IsRedundantFilter(R / c, c): # optional
>             RRQ.Enqueue(c, generation(c))
> 
>     while RRQ.Max == max_generation:
>         c = RRQ.Dequeue()
>         IDV[c] = 0  # commits with max gen have in-degree of 0
>         IDQ.Enqueue(c, generation(c))
> 
>     # this would be almost no-op
>     AdvanceInDegreeWalk(IDQ, IDV, max_generation)
> 
>     for each commit c in reversed R:
>         if generation(c) == max_generation: # then IDV[c] == 0
>             TOQ.Enqueue(c, visit_order++)
> 
>     return (IDQ, IDV, TOQ, RRQ, visit_order)

Isn't it premature to throw out code at this stage? My impression from the
side lines is that Stolee is trying to get concrete code implemented, code
that can be tested against concrete repositories, so that the question of
singularly overall importance can be asked: is it worth it, is it really
faster? And by how much?

It may not be the best idea to distract Stolee from said implementation
(i.e. step 1) by getting ahead ourselves with steps 17, 18 or 19. The next
steps would seem to be step 2 and 3, and I am sure that Stolee would
appreciate your help in implementing them.

So at this point it would maybe make a lot more sense to help Stolee e.g.
by refactoring the rev-list code that is a *mess* around the topo order,
so that all kinds of cute algorithms can be implemented *directly* in Git,
and put to the ultimate test.

Ciao,
Dscho

Re: [PATCH 05/35] refspec: convert valid_fetch_refspec to use parse_refspec

[Junio C Hamano]

Brandon Williams  writes:

> Convert 'valid_fetch_refspec()' to use the new 'parse_refspec()'
> function to only parse a single refspec an eliminate an allocation.

s/an/and/, perhaps?

> -int valid_fetch_refspec(const char *fetch_refspec_str)
> -{
> - struct refspec_item *refspec;
> -
> - refspec = parse_refspec_internal(1, _refspec_str, 1, 1);
> - free_refspec(1, refspec);
> - return !!refspec;
> -}
> -
>  struct refspec_item *parse_fetch_refspec(int nr_refspec, const char 
> **refspec)
>  {
>   return parse_refspec_internal(nr_refspec, refspec, 1, 0);
> @@ -242,3 +233,11 @@ void refspec_clear(struct refspec *rs)
>  
>   rs->fetch = 0;
>  }
> +
> +int valid_fetch_refspec(const char *fetch_refspec_str)
> +{
> + struct refspec_item refspec;
> + int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
> + refspec_item_clear();
> + return ret;
> +}

Makes quite a lot of sense.  The function name may eventually want
to become parse_refspec_item(), though?

Re: [PATCH 04/35] refspec: introduce struct refspec

[Junio C Hamano]

Brandon Williams  writes:

> diff --git a/refspec.h b/refspec.h
> index 173cea882..f6fb251f3 100644
> --- a/refspec.h
> +++ b/refspec.h
> @@ -20,4 +20,29 @@ struct refspec_item *parse_push_refspec(int nr_refspec, 
> const char **refspec);
>  
>  void free_refspec(int nr_refspec, struct refspec_item *refspec);
>  
> +#define REFSPEC_FETCH 1
> +#define REFSPEC_PUSH 0

The reversed order of these two definitions looks somewhat unusual.
I guess the reason why you made FETCH true and PUSH false is
probably because quite a lot of places in the existing code we do

if (fetch)
do the fetch thing;
else
do the push thing;

i.e. "fetch" variable is used as "is this a fetch: yes/no?"
boolean, and a caller that mysteriously passes "1" (or "0")
is harder to read than necessary.  Being able to pass REFSPEC_PUSH
instead of "0" would certainly make the caller easier to read.  But
as long as the variable is understood as "is_fetch? Yes/no", the
caller can pass Yes or No and be still descriptive enough.

I think the way to make such a code more readable would not be to
rewrite the above to

if (fetch_or_push)
do the fetch thing;
else
do the push thing;

Rather it would be 

if (fetch_or_push == REFSPEC_FETCH)
do the fetch thing;
else
do the push thing;

And once you have gone that far, the actual "enum" value assignment
no longer makes difference.

> +#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH }
> +#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH }
> +
> +struct refspec {
> + struct refspec_item *items;
> + int alloc;
> + int nr;
> +
> + const char **raw;
> + int raw_alloc;
> + int raw_nr;
> +
> + int fetch;
> +};
> +
> +void refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch);
> +void refspec_item_clear(struct refspec_item *item);
> +void refspec_init(struct refspec *rs, int fetch);
> +void refspec_append(struct refspec *rs, const char *refspec);
> +void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
> +void refspec_clear(struct refspec *rs);
> +
>  #endif /* REFSPEC_H */

Re: [PATCH v1 2/8] Add initial odb remote support

[Ævar Arnfjörð Bjarmason]

On Sun, May 13 2018, Christian Couder wrote:

> "sha1_file.c"[...]

sha1-file.c these days :)

Re: [PATCH 00/35] refactoring refspecs

[Ævar Arnfjörð Bjarmason]

On Mon, May 14 2018, Brandon Williams wrote:

> When working on protocol v2 I noticed that working with refspecs was a
> little difficult because of the various api's that existed.  Some
> functions expected an array of "const char *" while others expected an
> array of "struct refspec".  In all cases a length parameter needed to be
> passed as a parameter as well.  This makes working with refspecs a
> little challenging because of the different expectations different parts
> of the code base have.
>
> This series refactors how refspecs are handled through out the code base
> by renaming the existing struct refspec to refspec_item and introducing
> a new 'struct refspec' which is a container of refspec_items, much like
> how a pathspec contains pathspec_items.  This simplifies many callers
> and makes handling pathspecs a bit easier.

This looks really good to me. The API you're replacing is one of the
worst I've had a chance to encounter in git.git (as noted in my
https://public-inbox.org/git/87in7p2ucb@evledraar.gmail.com/ but
maybe I haven't looked widely enough), and now it's really nice.

> I have some follow on work which I'll build on top of this series, but
> since this was already getting a bit lengthy at 35 patches I'll save
> that for another time.

In addition to my other suggestions for stuff to put on top, which I see
now you may have just had in your local tree but didn't submit, I think
this makes sense:

diff --git a/remote.c b/remote.c
index 946b95d18d..cb97e662e8 100644
--- a/remote.c
+++ b/remote.c
@@ -77,16 +77,6 @@ static const char *alias_url(const char *url, struct 
rewrites *r)
return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len);
 }

-static void add_push_refspec(struct remote *remote, const char *ref)
-{
-   refspec_append(>push, ref);
-}
-
-static void add_fetch_refspec(struct remote *remote, const char *ref)
-{
-   refspec_append(>fetch, ref);
-}
-
 static void add_url(struct remote *remote, const char *url)
 {
ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
@@ -261,9 +251,9 @@ static void read_remotes_file(struct remote *remote)
if (skip_prefix(buf.buf, "URL:", ))
add_url_alias(remote, xstrdup(skip_spaces(v)));
else if (skip_prefix(buf.buf, "Push:", ))
-   add_push_refspec(remote, xstrdup(skip_spaces(v)));
+   refspec_append(>push, xstrdup(skip_spaces(v)));
else if (skip_prefix(buf.buf, "Pull:", ))
-   add_fetch_refspec(remote, xstrdup(skip_spaces(v)));
+   refspec_append(>fetch, xstrdup(skip_spaces(v)));
}
strbuf_release();
fclose(f);
@@ -302,14 +292,14 @@ static void read_branches_file(struct remote *remote)
frag = "master";

add_url_alias(remote, strbuf_detach(, NULL));
-   add_fetch_refspec(remote, xstrfmt("refs/heads/%s:refs/heads/%s",
- frag, remote->name));
+   refspec_append(>fetch, xstrfmt("refs/heads/%s:refs/heads/%s",
+  frag, remote->name));

/*
 * Cogito compatible push: push current HEAD to remote #branch
 * (master if missing)
 */
-   add_push_refspec(remote, xstrfmt("HEAD:refs/heads/%s", frag));
+   refspec_append(>push, xstrfmt("HEAD:refs/heads/%s", frag));
remote->fetch_tags = 1; /* always auto-follow */
 }

@@ -395,12 +385,12 @@ static int handle_config(const char *key, const char 
*value, void *cb)
const char *v;
if (git_config_string(, key, value))
return -1;
-   add_push_refspec(remote, v);
+   refspec_append(>push, v);
} else if (!strcmp(subkey, "fetch")) {
const char *v;
if (git_config_string(, key, value))
return -1;
-   add_fetch_refspec(remote, v);
+   refspec_append(>fetch, v);
} else if (!strcmp(subkey, "receivepack")) {
const char *v;
if (git_config_string(, key, value))

I.e. the reason we have add_{push,fetch}_refspec() in the first place is
because without your series it's tricky to add new ones, but now it's
trivial, so let's not leave behind wrapper static functions whose sole
purpose is to just call another exported API as-is.

I've pushed all the patches I quoted inline in this review at
avar-bwill/refspec in github.com/avar/git, consider them all signed-off,
and depending on whether you agree/disagree etc. please squash
them/adapt them/drop them however you see fit.

Re: [PATCH 14/35] remote: convert fetch refspecs to struct refspec

[Ævar Arnfjörð Bjarmason]

On Mon, May 14 2018, Brandon Williams wrote:

>  void add_prune_tags_to_fetch_refspec(struct remote *remote)
>  {
> - int nr = remote->fetch_refspec_nr;
> - int bufsize = nr  + 1;
> - int size = sizeof(struct refspec_item);
> -
> - remote->fetch = xrealloc(remote->fetch, size  * bufsize);
> - memcpy(>fetch[nr], tag_refspec, size);
> - add_fetch_refspec(remote, xstrdup(TAG_REFSPEC));
> + refspec_append(>fetch, TAG_REFSPEC);
>  }

Thanks for fixing the hack I needed to put in place in 97716d217c
("fetch: add a --prune-tags option and fetch.pruneTags config",
2018-02-09).

I'm not sure where it belongs in this series, but I think this makes
sense on top of the whole thing:

diff --git a/builtin/fetch.c b/builtin/fetch.c
index af7064dce3..9a523249f5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1383,7 +1383,8 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv, int pru

maybe_prune_tags = prune_tags_ok && prune_tags;
if (maybe_prune_tags && remote_via_config)
-   add_prune_tags_to_fetch_refspec(remote);
+   refspec_append(>fetch, TAG_REFSPEC);
+

if (maybe_prune_tags && (argc || !remote_via_config))
refspec_append(, TAG_REFSPEC);
diff --git a/remote.c b/remote.c
index 8e6522f4d0..946b95d18d 100644
--- a/remote.c
+++ b/remote.c
@@ -87,11 +87,6 @@ static void add_fetch_refspec(struct remote *remote, 
const char *ref)
refspec_append(>fetch, ref);
 }

-void add_prune_tags_to_fetch_refspec(struct remote *remote)
-{
-   refspec_append(>fetch, TAG_REFSPEC);
-}
-
 static void add_url(struct remote *remote, const char *url)
 {
ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
diff --git a/remote.h b/remote.h
index 9014f707f0..62a6566594 100644
--- a/remote.h
+++ b/remote.h
@@ -289,6 +289,4 @@ extern int parseopt_push_cas_option(const struct option 
*, const char *arg, int
 extern int is_empty_cas(const struct push_cas_option *);
 void apply_push_cas(struct push_cas_option *, struct remote *, struct ref 
*);

-void add_prune_tags_to_fetch_refspec(struct remote *remote);
-
 #endif

I.e. the whole reason we have this function is because of my above
commit where I had to very carefully hack around the fact that we didn't
have something which could ALLOW_GROW() the structure after it had been
created.

So I added the add_prune_tags_to_fetch_refspec() function to very
carefully do *only* that so others wouldn't be tempted to use this hack
more generally.

But now we have a nice API for it, so we can just throw away the
wrapper, and use the same API everywhere. You already did the other half
of that in your e69b54f53a ("fetch: convert fetch_one to use struct
refspec", 2018-05-11).

Re: could `git merge --no-ff origin/master` be made more useful?

[demerphq]

On 15 May 2018 at 00:58, Ævar Arnfjörð Bjarmason  wrote:
>
> On Mon, May 14 2018, demerphq wrote:
>
>> The first time I tried to use --no-ff I tried to do something like this:
>>
>>   git checkout master
>>   git commit -a -m'whatever'
>>   git commit -a -m'whatever2'
>>   git merge --no-ff origin/master
>>
>> and was disappointed when "it didn't work" and git told me there was
>> nothing to do as the branch was up to date. (Which I found a bit
>> confusing.)
>>
>> I realize now my expectations were incorrect, and that the argument to
>> merge needs to resolve to a commit that is ahead of the current
>> commit, and in the above sequence it is the other way around. So to do
>> what I want I can do:
>>
>>   git checkout master
>>   git checkout -b topic
>>   git commit -a -m'whatever'
>>   git commit -a -m'whatever2'
>>   git checkout master
>>   git merge --no-ff topic
>>
>> and iiuir this works because 'master' would be behind 'topic' in this case.
>>
>> But I have a few questions, 1) is there is an argument to feed to git
>> merge to make the first recipe work like the second? And 2) is this
>> asymmetry necessary with --no-ff?
>
> I've been bitten my this myself, but found that it's documented as the
> very first thing in git-merge:
>
> Incorporates changes from the named commits (since the time their
> histories diverged from the current branch) into the current
> branch[...].
>
> Since origin/master hasn't diverged from your current branch (unlike the
> other way around), the merge with --no-ff is a noop.

Yeah, I got it, but only after rereading a lot of times.

>
>> More specifically would something horrible break if --no-ff
>> origin/trunk detected that the current branch was ahead of the named
>> branch and "swapped"  the implicit order of the two so that the first
>> recipe could behave like the second
>
> If it worked like that then the user who sets merge.ff=false in his
> config and issues a "git pull" after making a commit on his local master
> would create a merge commit.
>
> This old E-Mail of Junio's discusses that edge case & others in detail:
> https://public-inbox.org/git/7vty1zfwmd@alter.siamese.dyndns.org/

Thanks I skimmed, but it is long so I will review later.

I see the point about the config option for no-ff.

But what about an option like --reverse? Assuming we are on a local
branch master then

  git merge --no-ff --reverse origin/master

would treat origin/master as the "current" branch, and "master" as the
merged in branch, and create the appropriate merge commit. Which as
far as I can tell is tree-wise identical to creating a topic branch
instead of hacking on the local master.

>> Anyway, even if the above makes no sense, would it be hard to make the
>> message provided by git merge in the first recipe a bit more
>> suggestive of what is going on? For instance if it had said "Cannot
>> --no-ff merge, origin/master is behind master" it would have been much
>> more clear what was going on.
>
> I can't spot any reason for why we couldn't have something like this POC
> (would be properly done through advice.c):
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 9db5a2cf16..920f67d9f8 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1407,6 +1407,8 @@ int cmd_merge(int argc, const char **argv, const 
> char *prefix)
>  * but first the most common case of merging one remote.
>  */
> finish_up_to_date(_("Already up to date."));
> +   if (fast_forward == FF_NO)
> +   fprintf(stderr, "did you mean this the other way 
> around?\n");
> goto done;
> } else if (fast_forward != FF_NO && !remoteheads->next &&
> !common->next &&
>
> But that should probably be reworked to be smart about whether --no-ff
> or merge.ff=false was specified, i.e. do we want to yell this at the
> user who's just set that at his config default, or the user who's
> specified --no-ff explicitly, or both? I don't know.

Yes, all those points make sense.

Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: [PATCH 03/35] refspec: rename struct refspec to struct refspec_item

[Junio C Hamano]

Brandon Williams  writes:

> In preperation for introducing an abstraction around a collection of
> refspecs (much like how a 'struct pathspec' is a collection of 'struct
> pathspec_item's) rename the existing 'struct refspec' to 'struct
> refspec_item'.

Makes sense.

>  /* See TAG_REFSPEC for the string version */
> -const struct refspec *tag_refspec = _tag_refspec;
> +const struct refspec_item *tag_refspec = _tag_refspec;
>  
>  /*
>   * Parses 'refspec' and populates 'rs'.  returns 1 if successful and 0 if the
>   * refspec is invalid.
>   */
> -static int parse_refspec(struct refspec *rs, const char *refspec, int fetch)
> +static int parse_refspec(struct refspec_item *rs, const char *refspec, int 
> fetch)

The new comment given to the function in the previous step talks
in terms of the variable name and not type, so technically it is
still valid after this change without updating.

I however wonder if it makes more sense to go one step further and
rename the "const char *" variable that is a string form of a single
refspec item to something other than "refspec".  Which would
invalidate the commit you gave to the function and make it necessary
to be updated in this step.

I wonder if the early part of the series becomes easier to read if
patches 2 and 3 are swapped.  That may result in less code/comment
churn.

Re: [PATCH 35/35] submodule: convert push_unpushed_submodules to take a struct refspec

[Ævar Arnfjörð Bjarmason]

On Mon, May 14 2018, Brandon Williams wrote:

> Convert 'push_unpushed_submodules()' to take a 'struct refspec' as a
> parameter instead of an array of 'const char *'.
> [...]
> diff --git a/submodule.h b/submodule.h
> index e5526f6aa..aae0c9c8f 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -100,9 +100,10 @@ extern int submodule_touches_in_range(struct object_id 
> *a,
>  extern int find_unpushed_submodules(struct oid_array *commits,
>   const char *remotes_name,
>   struct string_list *needs_pushing);
> +struct refspec;
>  extern int push_unpushed_submodules(struct oid_array *commits,
>   const struct remote *remote,
> - const char **refspec, int refspec_nr,
> + const struct refspec *rs,
>   const struct string_list *push_options,
>   int dry_run);
>  /*

Why do you prefer doing this to having this on top?:

diff --git a/submodule.h b/submodule.h
index aae0c9c8ff..c3f206ce17 100644
--- a/submodule.h
+++ b/submodule.h
@@ -1,5 +1,6 @@
 #ifndef SUBMODULE_H
 #define SUBMODULE_H
+#include "refspec.h"
 
 struct repository;
 struct diff_options;
@@ -100,7 +101,6 @@ extern int submodule_touches_in_range(struct object_id 
*a,
 extern int find_unpushed_submodules(struct oid_array *commits,
const char *remotes_name,
struct string_list *needs_pushing);
-struct refspec;
 extern int push_unpushed_submodules(struct oid_array *commits,
const struct remote *remote,
const struct refspec *rs,

Re: [PATCH 01/35] refspec: move refspec parsing logic into its own file

[Junio C Hamano]

Brandon Williams  writes:

> In preperation for performing a refactor on refspec related code, move

Preparation?

> the refspec parsing logic into its own file.
>
> Signed-off-by: Brandon Williams 
> ---
>  Makefile|   1 +
>  branch.c|   1 +
>  builtin/clone.c |   1 +
>  builtin/fast-export.c   |   1 +
>  builtin/fetch.c |   1 +
>  builtin/merge.c |   1 +
>  builtin/pull.c  |   1 +
>  builtin/push.c  |   1 +
>  builtin/remote.c|   1 +
>  builtin/submodule--helper.c |   1 +
>  checkout.c  |   1 +
>  refspec.c   | 168 
>  refspec.h   |  23 +
>  remote.c| 165 +--
>  remote.h|  20 -
>  transport-helper.c  |   1 +
>  transport.c |   1 +
>  17 files changed, 205 insertions(+), 184 deletions(-)
>  create mode 100644 refspec.c
>  create mode 100644 refspec.h
>
> diff --git a/Makefile b/Makefile
> index ad880d1fc..4bca65383 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -928,6 +928,7 @@ LIB_OBJS += refs/files-backend.o
>  LIB_OBJS += refs/iterator.o
>  LIB_OBJS += refs/packed-backend.o
>  LIB_OBJS += refs/ref-cache.o
> +LIB_OBJS += refspec.o
>  LIB_OBJS += ref-filter.o
>  LIB_OBJS += remote.o
>  LIB_OBJS += replace-object.o
> diff --git a/branch.c b/branch.c
> index 2672054f0..32ccefc6b 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -3,6 +3,7 @@
>  #include "config.h"
>  #include "branch.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "remote.h"
>  #include "commit.h"
>  #include "worktree.h"
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 84f1473d1..6d1614ed3 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -14,6 +14,7 @@
>  #include "parse-options.h"
>  #include "fetch-pack.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "tree.h"
>  #include "tree-walk.h"
>  #include "unpack-trees.h"
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 530df12f0..a13b7c8ef 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -7,6 +7,7 @@
>  #include "cache.h"
>  #include "config.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "commit.h"
>  #include "object.h"
>  #include "tag.h"
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 7ee83ac0f..1fce68e9a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -5,6 +5,7 @@
>  #include "config.h"
>  #include "repository.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "commit.h"
>  #include "builtin.h"
>  #include "string-list.h"
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 9db5a2cf1..c362cfe90 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -14,6 +14,7 @@
>  #include "run-command.h"
>  #include "diff.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "commit.h"
>  #include "diffcore.h"
>  #include "revision.h"
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 71aac5005..6247c956d 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -15,6 +15,7 @@
>  #include "remote.h"
>  #include "dir.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "revision.h"
>  #include "submodule.h"
>  #include "submodule-config.h"
> diff --git a/builtin/push.c b/builtin/push.c
> index ac3705370..fa65999b2 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -4,6 +4,7 @@
>  #include "cache.h"
>  #include "config.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "run-command.h"
>  #include "builtin.h"
>  #include "remote.h"
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 8708e584e..c49513995 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -7,6 +7,7 @@
>  #include "strbuf.h"
>  #include "run-command.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "argv-array.h"
>  
>  static const char * const builtin_remote_usage[] = {
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c2403a915..6ab032acb 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -12,6 +12,7 @@
>  #include "run-command.h"
>  #include "remote.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "connect.h"
>  #include "revision.h"
>  #include "diffcore.h"
> diff --git a/checkout.c b/checkout.c
> index ac42630f7..193ba8567 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -1,5 +1,6 @@
>  #include "cache.h"
>  #include "remote.h"
> +#include "refspec.h"
>  #include "checkout.h"
>  
>  struct tracking_name_data {
> diff --git a/refspec.c b/refspec.c
> new file mode 100644
> index 0..ecb0bdff3
> --- /dev/null
> +++ b/refspec.c
> @@ -0,0 +1,168 @@
> +#include "cache.h"
> +#include "refs.h"
> +#include "refspec.h"
> +
> +static struct refspec s_tag_refspec = {
> + 0,
> + 1,
> + 0,
> + 0,
> + "refs/tags/*",
> + 

Re: [PATCH 00/35] refactoring refspecs

[Junio C Hamano]

Brandon Williams  writes:

> This series refactors how refspecs are handled through out the code base
> by renaming the existing struct refspec to refspec_item and introducing
> a new 'struct refspec' which is a container of refspec_items, much like
> how a pathspec contains pathspec_items.  This simplifies many callers
> and makes handling pathspecs a bit easier.

Good.  IOW, a refspec is a set of refspec items, just like a
pathspec is a set of pathspec items.  The two level abstraction
would naturally let us work on individual items as well as the whole
thing as a single set, which is quite nice.

Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Junio C Hamano]

Elijah Newren  writes:

> Hi Leif,
>
> On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte
>  wrote:
>
> Thanks for updating the patch on top of Stefan's series.  :-)
>
>> /* Case #1: a is contained in b or vice versa */
>> if (in_merge_bases(commit_a, commit_b)) {
>> oidcpy(result, b);
>> +   output(o, 1, _("Note: Fast-forwarding submodule %s to the 
>> following commit"), path);
>> +   output_commit_title(o, commit_b);
>
> Level 1 is for conflicts; I don't think this message should have
> higher priority than "Auto-merging $PATH" for normal files, so it
> needs to be 2 (or maybe 3, see below) rather than 1.  (The default
> output level is 2, so it'd still be shown, but we do allow people to
> remove informational message and just get conflicts by setting
> GIT_MERGE_VERBOSITY to 1, or request extra information by setting it
> higher)
>
> Also, this two-line message seems somewhat verbose compared to the
> other messages in merge_submdoule(), and when compared to the simple
> "Auto-merging $PATH" we do for normal files.  The multi-line nature of
> it particularly strikes me; the merge-recursive code has generally
> avoided multi-line messages even for conflicts.
>
> In comparison, your original patch just had ("Fast-forwarding
> submodule %s", path).

FWIW, I share both of your surprises.  Between level 2 and 3, after
skimming merge-recursive.c for existing use of output levels, I
think the situation for non-submodule merges that is closest to
these two cases the patch covers for submodules is probably the
message given when content merge happened to end up with what we
already had.  It is part of a normal merge operation that is not a
singificant event in the larger picture, yet it is rather rare and
interesting when you are curious on events that occur infrequently.

So a one-liner message as everybody else emitted at level 3 or more
verbose would probably be a good balance with the remainder of the
system, I would think.

Thanks for a review.