[PATCH] t: make many tests depend less on the refs being files

[Christian Couder]

From: David Turner 

So that they work under alternate ref storage backends.

This will be really needed when such alternate ref storage backends are
developed. But this could already help by making clear to readers that
some tests do not depend on which ref backend is used.

This patch just takes care of many low hanging fruits. It does not try
to completely solves the issue.

Signed-off-by: David Turner 
Signed-off-by: Christian Couder 
---

Thanks for all the great feedback regarding implementing reftable [1].

Looking at David Turner's tests in [2], it seems that they could indeed
be already valuable, so let's start by extracting most of the simple
improvements they make. 

[1] 
https://public-inbox.org/git/cap8ufd0ppzsjbnxca7ez91vbuatchkq+juwvtd1ihcxzpbj...@mail.gmail.com/
[2] https://github.com/dturner-tw/git/tree/dturner/pluggable-backends

 t/lib-t6000.sh   |  5 ++---
 t/t1401-symbolic-ref.sh  |  2 +-
 t/t3200-branch.sh| 18 +-
 t/t3903-stash.sh |  2 +-
 t/t5500-fetch-pack.sh| 10 +-
 t/t5510-fetch.sh |  6 +++---
 t/t6010-merge-base.sh|  2 +-
 t/t7201-co.sh|  2 +-
 t/t9104-git-svn-follow-parent.sh |  3 ++-
 t/t9903-bash-prompt.sh   |  2 +-
 10 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/t/lib-t6000.sh b/t/lib-t6000.sh
index 3f2d873fec..b8567cdf94 100644
--- a/t/lib-t6000.sh
+++ b/t/lib-t6000.sh
@@ -4,11 +4,10 @@ mkdir -p .git/refs/tags
 
 >sed.script
 
-# Answer the sha1 has associated with the tag. The tag must exist in 
.git/refs/tags
+# Answer the sha1 has associated with the tag. The tag must exist under 
refs/tags
 tag () {
_tag=$1
-   test -f ".git/refs/tags/$_tag" || error "tag: \"$_tag\" does not exist"
-   cat ".git/refs/tags/$_tag"
+   git rev-parse --verify "refs/tags/$_tag" || error "tag: \"$_tag\" does 
not exist"
 }
 
 # Generate a commit using the text specified to make it unique and the tree
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index 9e782a8122..a4ebb0b65f 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -65,7 +65,7 @@ reset_to_sane
 test_expect_success 'symbolic-ref fails to delete real ref' '
echo "fatal: Cannot delete refs/heads/foo, not a symbolic ref" >expect 
&&
test_must_fail git symbolic-ref -d refs/heads/foo >actual 2>&1 &&
-   test_path_is_file .git/refs/heads/foo &&
+   git rev-parse --verify refs/heads/foo &&
test_cmp expect actual
 '
 reset_to_sane
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index c0ef946811..222dc2c377 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -234,34 +234,34 @@ test_expect_success 'git branch -M master2 master2 should 
work when master is ch
 
 test_expect_success 'git branch -v -d t should work' '
git branch t &&
-   test_path_is_file .git/refs/heads/t &&
+   git rev-parse --verify refs/heads/t &&
git branch -v -d t &&
-   test_path_is_missing .git/refs/heads/t
+   test_must_fail git rev-parse --verify refs/heads/t
 '
 
 test_expect_success 'git branch -v -m t s should work' '
git branch t &&
-   test_path_is_file .git/refs/heads/t &&
+   git rev-parse --verify refs/heads/t &&
git branch -v -m t s &&
-   test_path_is_missing .git/refs/heads/t &&
-   test_path_is_file .git/refs/heads/s &&
+   test_must_fail git rev-parse --verify refs/heads/t &&
+   git rev-parse --verify refs/heads/s &&
git branch -d s
 '
 
 test_expect_success 'git branch -m -d t s should fail' '
git branch t &&
-   test_path_is_file .git/refs/heads/t &&
+   git rev-parse refs/heads/t &&
test_must_fail git branch -m -d t s &&
git branch -d t &&
-   test_path_is_missing .git/refs/heads/t
+   test_must_fail git rev-parse refs/heads/t
 '
 
 test_expect_success 'git branch --list -d t should fail' '
git branch t &&
-   test_path_is_file .git/refs/heads/t &&
+   git rev-parse refs/heads/t &&
test_must_fail git branch --list -d t &&
git branch -d t &&
-   test_path_is_missing .git/refs/heads/t
+   test_must_fail git rev-parse refs/heads/t
 '
 
 test_expect_success 'git branch --list -v with --abbrev' '
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aefde7b172..1f871d3cca 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -726,7 +726,7 @@ test_expect_success 'store updates stash ref and reflog' '
git reset --hard &&
! grep quux bazzy &&
git stash store -m quuxery $STASH_ID &&
-   test $(cat .git/refs/stash) = $STASH_ID &&
+   test $(git rev-parse stash) = $STASH_ID &&
git reflog --format=%H stash| grep $STASH_ID &&
git stash pop &&
grep quux bazzy
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh

Re: [PATCH v3 01/15] commit-slab.h: code split

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> The struct declaration and implementation macros are moved to
> commit-slab-hdr.h and commit-slab-impl.h respectively.

s/hdr/decl/;

>
> This right now is not needed for current users but if we make a public
> commit-slab type, we may want to avoid including the slab
> implementation in a header file which gets replicated in every c file
> that includes it.

s/c file/C file/; 

Also s/may want to/need to/; after all this is not about avoid
bloating the header and slowing down compilation.  Rather, the
duplicated implementation will cause linkage failures so we want
only a single implementation that is referenced from many places.

> ---

Missing sign off.

>  commit-slab-decl.h |  30 
>  commit-slab-impl.h |  91 +++
>  commit-slab.h  | 115 +++--
>  3 files changed, 127 insertions(+), 109 deletions(-)
>  create mode 100644 commit-slab-decl.h
>  create mode 100644 commit-slab-impl.h

Other than that, looking good.

Re: [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Junio C Hamano]

I've been using both branch-diff and tbdiff while comparing rerolled
topics before accepting them.  One obvious difference between the
two is that the speed to compute pairing is quite different but that
is expected ;-)

Another difference that is somewhat irritating is that a SP that
leads a context line in a diff hunk that is introduced in the newer
iteration is often painted in reverse, and I do not know what aspect
of that single SP on these lines the branch-diff wants to pull my
attention to.

https://pasteboard.co/Hm9ujI7F.png

In the picture, the three pre-context lines that are all indented by
a HT are prefixed by a SP, and that is prefixed by a '+' sign of the
outer diff.

We can use --dual-color mode to unsee these but it is somewhat
frustrating not to know what the branch-diff program wants to tell
me by highlighting the single SPs on these lines.

Thanks.

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

[Junio C Hamano]

Elijah Newren  writes:

> Thanks for continuing to push on this.  This looks good so far (to
> me), but I was also hoping to see the analogy between these messages
> and "Auto-merging $FILE" for regular files mentioned.  Both Junio[1]
> and I[2] pointed out this similarity, and I think this
> similarity/analogy is useful additional motivation for making this
> change.

... meaning that it should be discussed and named as the primary
reason why this change is a good idea?

Re-reading what Leif wrote in the first paragraph, I tend to think
that "the more recent version may break us" Leif gives is not a
particularly convincing one.  After all, if we did not change the
commit bound at a submodule since we forked, while they changed it
to something else (either old or new), even though our changes may
have been fully tested with the version of the submodule we have
been testing with, it may break with the version the merged branch
has been using.  Such an update is cleanly and silently resolved at
the tree-level three-way merge, but the risk of breakage is no
different to the case this patch adds new notices to.

More importantly, the same "the changes we made may get broken by
changes in areas that are textually unrelated they made" will happen
without submodules.  Content-level three-way merges that resolves
cleanly at the textual level may need to get semantic adjustment.
Do we treat clean 3-way content merges as suspicious and give a
similar warning?  That smells like madness.

But as you said, we give "Auto-merging $FILE" notice to clean 3-way
merge at the content-level for normal files, and there is no good
reason why we should not do the same for submodules when one
fast-forwards to the other, which is an analogue to the
content-level 3-way merge where one branch's version is a superset
of the other ones.  And that is quite a convincing reason why a new
"Auto-merging $SUBMODULE" notice is a good idea.

> ...
> Also, by analogy to the "Auto-merging $FILE" comparison, the "to %s"
> on these two lines feels out of place.  Users can just look at the
> submodule to see what it was updated to.  In a sea of output from
> merging, this extra detail feels like noise for the standard use-case,
> unless I'm misunderstanding how submodules are special.

Now you meantion it, that part of the message does look more like a
debugging aid than a feature that helps actual end-users.  After
all, if our side did not change the commit recorded for the
submodule while their side changed, we do not report the result of
such a tree-level three-way merge that takes what commit they had at
their tip.

Re: [PATCH 2/2] apply: add --intent-to-add

[Junio C Hamano]

Duy Nguyen  writes:

>> >if (state->check_index && is_not_gitdir)
>> >return error(_("--index outside a repository"));
>> > +  if (state->set_ita && is_not_gitdir)
>> > +  state->set_ita = 0;
>> 
>> I think this should error out, just like one line above does.
>> "I-t-a" is impossible without having the index, just like "--index"
>> is impossible without having the index.
>
> I was hoping to put this in an alias that works both with or without a
> repository. Do you feel strongly about this? 

"git apply --index" that silently applied only to the filesystem
files without telling me that I am in a wrong directory by mistake
is a UI disaster, and that is what the existing error we see in the
pre context is about.  I do not think of a good reason why "git
apply --i-t-a" should behave any differently.

Re: Re*: [PATCH v4 3/4] string-list: provide `string_list_appendf()`

[Jeff King]

On Mon, May 21, 2018 at 09:25:01AM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > I have a feeling that argv_array might be a better fit for the
> > purpose of keeping track of to_free[] strings in the context of this
> > series.  Moving away from string_list would allow us to sidestep the
> > storage ownership issues the API has, and we do not need the .util
> > thing string_list gives us (which is one distinct advantage string_list
> > has over argv_array, if the application needs that feature).
> >
> > We would need to make _pushf() and friends return "const char *" if
> > we go that route to make the resulting API more useful, though.
> 
> ... and redoing the 4/4 patch using argv_array_pushf() makes the
> result look like this, which does not look too bad.

Agreed.

-Peff

Re: Re*: [PATCH v4 3/4] string-list: provide `string_list_appendf()`

[Jeff King]

On Mon, May 21, 2018 at 09:01:05AM +0900, Junio C Hamano wrote:

> Jacob Keller  writes:
> 
> > On Sun, May 20, 2018 at 3:17 AM, Martin Ågren  
> > wrote:
> >> +/**
> >> + * Add formatted string to the end of `list`. This function ignores
> >> + * the value of `list->strdup_strings` and always appends a freshly
> >> + * allocated string, so you will probably not want to use it with
> >> + * `strdup_strings = 0`.
> >> + */
> >> +struct string_list_item *string_list_appendf(struct string_list *list,
> >> +const char *fmt, ...);
> >> +
> >
> > Would it make sense to verify that strdup_strings == 0? I guess we'd
> > have to use die or BUG(), but that would mean that the program could
> > crash..
> 
> It probably is clear to readers that any reasonable implementation
> of *_appendf() will create a new and unique string, as the point of
> *f() is to give a customized instantiation of fmt string for given
> parameters.  So it would be natural to expect that the storage that
> holds the generated string will belong to the list.  We _could_ make
> it honor strdup_strings and make one extra copy when strdup_strings
> is set to true, but the only effect such a stupid implementation has
> is to unnecessarily leak ;-)
> 
> I think it is probably OK to check and BUG() when strdup_strings==0,
> but such a check means that we now declare that a string list must
> either borrow all of its strings from elsewhere or own all of its
> strings itself, and mixture is not allowed.
>
> The (overly) flexible string_list API could be used to mix both
> borrowed and owned strings (an obvious strategy to do this without
> leaking and crashing is to use the .util field to mark which ones
> are owned and which ones are borrowed), so there might already be
> current users of the API that violates that rule.

IMHO such a mixed use is mildly crazy. At any rate, we would know that
anybody using appendf() would not have this problem, since we are just
introducing it now.

> I have a feeling that argv_array might be a better fit for the
> purpose of keeping track of to_free[] strings in the context of this
> series.  Moving away from string_list would allow us to sidestep the
> storage ownership issues the API has, and we do not need the .util
> thing string_list gives us (which is one distinct advantage string_list
> has over argv_array, if the application needs that feature).

I do agree that argv_array is generally a better fit for most cases.
Didn't we want to rename it to strarray or something? That's probably
too much yak-shaving for this series, though. :)

> We would need to make _pushf() and friends return "const char *" if
> we go that route to make the resulting API more useful, though.

This is the first time I think that's been suggested, but I agree it's
the only sensible thing for the functions to return.

> -- >8 --
> Subject: argv-array: return the pushed string from argv_push*()
> 
> Such an API change allows us to use an argv_array this way:
> 
>   struct argv_array to_free = ARGV_ARRAY_INIT;
> const char *msg;
> 
> if (some condition) {
>   msg = "constant string message";
>   ... other logic ...
>   } else {
>   msg = argv_pushf(_free, "format %s", var);
>   }
>   ... use "msg" ...
>   ... do other things ...
>   argv_clear(_free);
> 
> Note that argv_array_pushl() and argv_array_pushv() are used to push
> one or more strings with a single call, so we do not return any one
> of these strings from these two functions in order to reduce the
> chance to misuse the API.
> 
> Signed-off-by: Junio C Hamano 
> ---

Yup, this looks good to me.

-Peff

[PATCH v4 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 

Re: git apply does not honor diff.noprefix config setting

[Junio C Hamano]

hIpPy  writes:

> If I disable mnemonic prefix,
>
> $ git config --global diff.noprefix true
>
> and do a round-trip of format-patch and apply,

Setting diff.noprefix does not disable "mnemonic prefix".  It asks
"diff" family of commands to use no prefix, not even the normal,
non-mnemonic, prefix.

> $ git format-patch -1 @
> $ git apply 
>
> git apply fails with,
>
> error: git diff header lacks filename information when removing 1
> leading pathname component (line 16)

Totally expected.

> Without 'diff.noprefix' config setting, git apply works. It seems git
> apply does not honor the diff.noprefix config setting.

Yes, and because "diff" and "format-patch" are for producers of, and
"apply" and "am" are for consumers of a patch, which are likely to
be different people, "apply" or "am" should never pay attention to
"diff.noprefix".

It is a different issue if we should have

 - format-patch.noprefix, which defaults to the same as
   diff.noprefix, but allows people to configure "format-patch"
   differently from "diff" and "show"

 - am.pvalue, which defaults to 1, but can be set to e.g. 0 to
   accept format-patch output from those who set
   format-patch.noprefix to true

I haven't thought things through, but offhand I do not see why we
shouldn't.  But I am reasonably firm on that diff.noprefix should
never affect anything on the "apply/am" side.

[PATCH v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 <

[PATCH v4 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 v4 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 v4 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..c6af7f82b5 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 &&
+   grep -v index actual >out &&
test_cmp out expected
 
 '

[PATCH v4 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 v4 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 v4 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 v3:
* Switch a use of egrep to grep.

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 +-
 

[PATCH v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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 v4 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:
 #

Re: What's cooking in git.git (May 2018, #02; Thu, 17)

[brian m. carlson]

On Thu, May 17, 2018 at 03:01:40PM +0900, Junio C Hamano wrote:
> * bc/hash-independent-tests (2018-05-16) 28 commits
>  - t5300: abstract away SHA-1-specific constants
>  - t4208: abstract away SHA-1-specific constants
>  - t4045: abstract away SHA-1-specific constants
>  - t4042: abstract away SHA-1-specific constants
>  - t4205: sort log output in a hash-independent way
>  - t/lib-diff-alternative: abstract away SHA-1-specific constants
>  - t4030: abstract away SHA-1-specific constants
>  - t4029: abstract away SHA-1-specific constants
>  - t4029: fix test indentation
>  - t4022: abstract away SHA-1-specific constants
>  - t4020: abstract away SHA-1-specific constants
>  - t4014: abstract away SHA-1-specific constants
>  - t4008: abstract away SHA-1-specific constants
>  - t4007: abstract away SHA-1-specific constants
>  - t3905: abstract away SHA-1-specific constants
>  - t3702: abstract away SHA-1-specific constants
>  - t3103: abstract away SHA-1-specific constants
>  - t2203: abstract away SHA-1-specific constants
>  - t: skip pack tests if not using SHA-1
>  - t4044: skip test if not using SHA-1
>  - t1512: skip test if not using SHA-1
>  - t1007: annotate with SHA1 prerequisite
>  - t: annotate with SHA1 prerequisite
>  - t: switch $_x40 to $OID_REGEX
>  - t/test-lib: introduce OID_REGEX
>  - t: switch $_z40 to $ZERO_OID
>  - t/test-lib: introduce ZERO_OID
>  - t/test-lib: add an SHA1 prerequisite
> 
>  Many tests hardcode the raw object names, which would change once
>  we migrate away from SHA-1.  While some of them must test against
>  exact object names, most of them do not have to use hardcoded
>  constants in the test.  The latter kind of tests have been updated
>  to test the moral equivalent of the original without hardcoding the
>  actual object names.
> 
>  Will merge to 'next'.

I think there was one minor change Stefan wanted out of this series.
I'll send a reroll (and tbdiff) with just that change.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 13/14] completion: reduce completable command list

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> The following commands are removed from the complete list:
>
> - interpreter-trailers not for interactive use

Typo here.  see below.

> -git-interpret-trailers  purehelpers 
> complete
> +git-interpret-trailers  purehelpers

Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers

[Junio C Hamano]

René Scharfe  writes:

> How about this instead?
>
> -- >8 --
> Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process
>
> Avoid magic array sizes and indexes by constructing the fsmonitor
> command line using the embedded argv_array of the child_process.  The
> resulting code is shorter and easier to extend.
>
> Getting rid of the snprintf() calls is a bonus -- even though the
> buffers were big enough here to avoid truncation -- as it makes auditing
> the remaining callers easier.
> ...
> - if (!(argv[0] = core_fsmonitor))
> + if (!core_fsmonitor)
>   return -1;
>  
> - snprintf(ver, sizeof(ver), "%d", version);

I really like this change, as this exact line used to take
sizeof(version) instead of sizeof(ver), which has been corrected
recently.

> - snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
> - argv[1] = ver;
> - argv[2] = date;
> - argv[3] = NULL;
> - cp.argv = argv;
> + argv_array_push(, core_fsmonitor);
> + argv_array_pushf(, "%d", version);

If it were written like this from the beginning, such a bug would
never have happened ;-)

> + argv_array_pushf(, "%" PRIuMAX, (uintmax_t)last_update);
>   cp.use_shell = 1;
>   cp.dir = get_git_work_tree();

Re: Re*: [PATCH v4 3/4] string-list: provide `string_list_appendf()`

[Junio C Hamano]

Junio C Hamano  writes:

> I have a feeling that argv_array might be a better fit for the
> purpose of keeping track of to_free[] strings in the context of this
> series.  Moving away from string_list would allow us to sidestep the
> storage ownership issues the API has, and we do not need the .util
> thing string_list gives us (which is one distinct advantage string_list
> has over argv_array, if the application needs that feature).
>
> We would need to make _pushf() and friends return "const char *" if
> we go that route to make the resulting API more useful, though.

... and redoing the 4/4 patch using argv_array_pushf() makes the
result look like this, which does not look too bad.

-- >8 --
From: Junio C Hamano 
Subject: [PATCH] unpack_trees_options: keep track of owned messages with 
argv_array

Instead of the string_list API, which is overly flexible and require
callers to be careful about memory ownership issues, use the
argv_array API that always takes ownership to redo the earlier
commit.

Signed-off-by: Junio C Hamano 
---
 unpack-trees.c | 16 ++--
 unpack-trees.h |  4 ++--
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 86046b987a..b28f0c6e9d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1,5 +1,6 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "argv-array.h"
 #include "repository.h"
 #include "config.h"
 #include "dir.h"
@@ -103,11 +104,7 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
const char **msgs = opts->msgs;
const char *msg;
 
-   /*
-* As we add strings using `...appendf()`, this does not matter,
-* but when we clear the string list, we want them to be freed.
-*/
-   opts->msgs_to_free.strdup_strings = 1;
+   argv_array_init(>msgs_to_free);
 
if (!strcmp(cmd, "checkout"))
msg = advice_commit_before_merge
@@ -125,7 +122,7 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
  "Please commit your changes or stash them before you 
%s.")
  : _("Your local changes to the following files would be 
overwritten by %s:\n%%s");
msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
-   string_list_appendf(>msgs_to_free, msg, cmd, cmd)->string;
+   argv_array_pushf(>msgs_to_free, msg, cmd, cmd);
 
msgs[ERROR_NOT_UPTODATE_DIR] =
_("Updating the following directories would lose untracked 
files in them:\n%s");
@@ -146,7 +143,7 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
  "Please move or remove them before you %s.")
  : _("The following untracked working tree files would be 
removed by %s:\n%%s");
msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] =
-   string_list_appendf(>msgs_to_free, msg, cmd, cmd)->string;
+   argv_array_pushf(>msgs_to_free, msg, cmd, cmd);
 
if (!strcmp(cmd, "checkout"))
msg = advice_commit_before_merge
@@ -164,7 +161,7 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
  "Please move or remove them before you %s.")
  : _("The following untracked working tree files would be 
overwritten by %s:\n%%s");
msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] =
-   string_list_appendf(>msgs_to_free, msg, cmd, cmd)->string;
+   argv_array_pushf(>msgs_to_free, msg, cmd, cmd);
 
/*
 * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
@@ -189,8 +186,7 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
 
 void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
 {
-   string_list_clear(>msgs_to_free, 0);
-   memset(opts->msgs, 0, sizeof(opts->msgs));
+   argv_array_clear(>msgs_to_free);
 }
 
 static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
diff --git a/unpack-trees.h b/unpack-trees.h
index 5a84123a40..c2b434c606 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -2,7 +2,7 @@
 #define UNPACK_TREES_H
 
 #include "tree-walk.h"
-#include "string-list.h"
+#include "argv-array.h"
 
 #define MAX_UNPACK_TREES 8
 
@@ -62,7 +62,7 @@ struct unpack_trees_options {
struct pathspec *pathspec;
merge_fn_t fn;
const char *msgs[NB_UNPACK_TREES_ERROR_TYPES];
-   struct string_list msgs_to_free;
+   struct argv_array msgs_to_free;
/*
 * Store error messages in an array, each case
 * corresponding to a error message type
-- 
2.17.0-582-gccdcbd54c4

Re*: [PATCH v4 3/4] string-list: provide `string_list_appendf()`

[Junio C Hamano]

Jacob Keller  writes:

> On Sun, May 20, 2018 at 3:17 AM, Martin Ågren  wrote:
>> +/**
>> + * Add formatted string to the end of `list`. This function ignores
>> + * the value of `list->strdup_strings` and always appends a freshly
>> + * allocated string, so you will probably not want to use it with
>> + * `strdup_strings = 0`.
>> + */
>> +struct string_list_item *string_list_appendf(struct string_list *list,
>> +const char *fmt, ...);
>> +
>
> Would it make sense to verify that strdup_strings == 0? I guess we'd
> have to use die or BUG(), but that would mean that the program could
> crash..

It probably is clear to readers that any reasonable implementation
of *_appendf() will create a new and unique string, as the point of
*f() is to give a customized instantiation of fmt string for given
parameters.  So it would be natural to expect that the storage that
holds the generated string will belong to the list.  We _could_ make
it honor strdup_strings and make one extra copy when strdup_strings
is set to true, but the only effect such a stupid implementation has
is to unnecessarily leak ;-)

I think it is probably OK to check and BUG() when strdup_strings==0,
but such a check means that we now declare that a string list must
either borrow all of its strings from elsewhere or own all of its
strings itself, and mixture is not allowed.

The (overly) flexible string_list API could be used to mix both
borrowed and owned strings (an obvious strategy to do this without
leaking and crashing is to use the .util field to mark which ones
are owned and which ones are borrowed), so there might already be
current users of the API that violates that rule.

I have a feeling that argv_array might be a better fit for the
purpose of keeping track of to_free[] strings in the context of this
series.  Moving away from string_list would allow us to sidestep the
storage ownership issues the API has, and we do not need the .util
thing string_list gives us (which is one distinct advantage string_list
has over argv_array, if the application needs that feature).

We would need to make _pushf() and friends return "const char *" if
we go that route to make the resulting API more useful, though.

-- >8 --
Subject: argv-array: return the pushed string from argv_push*()

Such an API change allows us to use an argv_array this way:

struct argv_array to_free = ARGV_ARRAY_INIT;
const char *msg;

if (some condition) {
msg = "constant string message";
... other logic ...
} else {
msg = argv_pushf(_free, "format %s", var);
}
... use "msg" ...
... do other things ...
argv_clear(_free);

Note that argv_array_pushl() and argv_array_pushv() are used to push
one or more strings with a single call, so we do not return any one
of these strings from these two functions in order to reduce the
chance to misuse the API.

Signed-off-by: Junio C Hamano 
---
 argv-array.c | 6 --
 argv-array.h | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/argv-array.c b/argv-array.c
index 5d370fa336..449dfc105a 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -21,12 +21,13 @@ static void argv_array_push_nodup(struct argv_array *array, 
const char *value)
array->argv[array->argc] = NULL;
 }
 
-void argv_array_push(struct argv_array *array, const char *value)
+const char *argv_array_push(struct argv_array *array, const char *value)
 {
argv_array_push_nodup(array, xstrdup(value));
+   return array->argv[array->argc - 1];
 }
 
-void argv_array_pushf(struct argv_array *array, const char *fmt, ...)
+const char *argv_array_pushf(struct argv_array *array, const char *fmt, ...)
 {
va_list ap;
struct strbuf v = STRBUF_INIT;
@@ -36,6 +37,7 @@ void argv_array_pushf(struct argv_array *array, const char 
*fmt, ...)
va_end(ap);
 
argv_array_push_nodup(array, strbuf_detach(, NULL));
+   return array->argv[array->argc - 1];
 }
 
 void argv_array_pushl(struct argv_array *array, ...)
diff --git a/argv-array.h b/argv-array.h
index 29056e49a1..715c93b246 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -12,9 +12,9 @@ struct argv_array {
 #define ARGV_ARRAY_INIT { empty_argv, 0, 0 }
 
 void argv_array_init(struct argv_array *);
-void argv_array_push(struct argv_array *, const char *);
+const char *argv_array_push(struct argv_array *, const char *);
 __attribute__((format (printf,2,3)))
-void argv_array_pushf(struct argv_array *, const char *fmt, ...);
+const char *argv_array_pushf(struct argv_array *, const char *fmt, ...);
 LAST_ARG_MUST_BE_NULL
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pushv(struct argv_array *, const char **);

Proposal

[Miss Zeliha Omer Faruk]

Hello

Greetings to you please i have a business proposal for you contact me
for more detailes asap thanks.

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turkey

Proposal

[Zeliha Omer Faruk]

--
Hello

Greetings to you please i have a business proposal for you contact me
for more detailes asap thanks.

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turkey

[RFC/PATCH 5/7] rerere: only return whether a path has conflicts or not

[Thomas Gummerer]

We currently return the exact number of conflict hunks a certain path
has from the 'handle_paths' function.  However all of its callers only
care whether there are conflicts or not or if there is an error.
Return only that information, and document that only that information
is returned.  This will simplify the code in the subsequent steps.

Signed-off-by: Thomas Gummerer 
---
 rerere.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/rerere.c b/rerere.c
index 49ace8e108..f3e658e374 100644
--- a/rerere.c
+++ b/rerere.c
@@ -393,12 +393,13 @@ static int is_cmarker(char *buf, int marker_char, int 
marker_size)
  * one side of the conflict, NUL, the other side of the conflict,
  * and NUL concatenated together.
  *
- * Return the number of conflict hunks found.
+ * Return 1 if conflict hunks are found, 0 if there are no conflict
+ * hunks and -1 if an error occured.
  */
 static int handle_path(unsigned char *sha1, struct rerere_io *io, int 
marker_size)
 {
git_SHA_CTX ctx;
-   int hunk_no = 0;
+   int has_conflicts = 0;
enum {
RR_CONTEXT = 0, RR_SIDE_1, RR_SIDE_2, RR_ORIGINAL
} hunk = RR_CONTEXT;
@@ -426,7 +427,7 @@ static int handle_path(unsigned char *sha1, struct 
rerere_io *io, int marker_siz
goto bad;
if (strbuf_cmp(, ) > 0)
strbuf_swap(, );
-   hunk_no++;
+   has_conflicts = 1;
hunk = RR_CONTEXT;
rerere_io_putconflict('<', marker_size, io);
rerere_io_putmem(one.buf, one.len, io);
@@ -462,7 +463,7 @@ static int handle_path(unsigned char *sha1, struct 
rerere_io *io, int marker_siz
git_SHA1_Final(sha1, );
if (hunk != RR_CONTEXT)
return -1;
-   return hunk_no;
+   return has_conflicts;
 }
 
 /*
@@ -471,7 +472,7 @@ static int handle_path(unsigned char *sha1, struct 
rerere_io *io, int marker_siz
  */
 static int handle_file(const char *path, unsigned char *sha1, const char 
*output)
 {
-   int hunk_no = 0;
+   int has_conflicts = 0;
struct rerere_io_file io;
int marker_size = ll_merge_marker_size(path);
 
@@ -491,7 +492,7 @@ static int handle_file(const char *path, unsigned char 
*sha1, const char *output
}
}
 
-   hunk_no = handle_path(sha1, (struct rerere_io *), marker_size);
+   has_conflicts = handle_path(sha1, (struct rerere_io *), marker_size);
 
fclose(io.input);
if (io.io.wrerror)
@@ -500,14 +501,14 @@ static int handle_file(const char *path, unsigned char 
*sha1, const char *output
if (io.io.output && fclose(io.io.output))
io.io.wrerror = error_errno(_("Failed to flush %s"), path);
 
-   if (hunk_no < 0) {
+   if (has_conflicts < 0) {
if (output)
unlink_or_warn(output);
return error(_("Could not parse conflict hunks in %s"), path);
}
if (io.io.wrerror)
return -1;
-   return hunk_no;
+   return has_conflicts;
 }
 
 /*
@@ -955,7 +956,7 @@ static int handle_cache(const char *path, unsigned char 
*sha1, const char *outpu
mmfile_t mmfile[3] = {{NULL}};
mmbuffer_t result = {NULL, 0};
const struct cache_entry *ce;
-   int pos, len, i, hunk_no;
+   int pos, len, i, has_conflicts;
struct rerere_io_mem io;
int marker_size = ll_merge_marker_size(path);
 
@@ -1009,11 +1010,11 @@ static int handle_cache(const char *path, unsigned char 
*sha1, const char *outpu
 * Grab the conflict ID and optionally write the original
 * contents with conflict markers out.
 */
-   hunk_no = handle_path(sha1, (struct rerere_io *), marker_size);
+   has_conflicts = handle_path(sha1, (struct rerere_io *), marker_size);
strbuf_release();
if (io.io.output)
fclose(io.io.output);
-   return hunk_no;
+   return has_conflicts;
 }
 
 static int rerere_forget_one_path(const char *path, struct string_list *rr)
-- 
2.17.0.588.g4d217cdf8e.dirty

[RFC/PATCH 6/7] rerere: factor out handle_conflict function

[Thomas Gummerer]

Factor out the handle_conflict function, which handles a single
conflict in a path.  This is a preparation for the next step, where
this function will be re-used.  No functional changes intended.

Signed-off-by: Thomas Gummerer 
---
 rerere.c | 143 +--
 1 file changed, 65 insertions(+), 78 deletions(-)

diff --git a/rerere.c b/rerere.c
index f3e658e374..f3cfd1c09b 100644
--- a/rerere.c
+++ b/rerere.c
@@ -302,38 +302,6 @@ static void rerere_io_putstr(const char *str, struct 
rerere_io *io)
ferr_puts(str, io->output, >wrerror);
 }
 
-/*
- * Write a conflict marker to io->output (if defined).
- */
-static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
-{
-   char buf[64];
-
-   while (size) {
-   if (size <= sizeof(buf) - 2) {
-   memset(buf, ch, size);
-   buf[size] = '\n';
-   buf[size + 1] = '\0';
-   size = 0;
-   } else {
-   int sz = sizeof(buf) - 1;
-
-   /*
-* Make sure we will not write everything out
-* in this round by leaving at least 1 byte
-* for the next round, giving the next round
-* a chance to add the terminating LF.  Yuck.
-*/
-   if (size <= sz)
-   sz -= (sz - size) + 1;
-   memset(buf, ch, sz);
-   buf[sz] = '\0';
-   size -= sz;
-   }
-   rerere_io_putstr(buf, io);
-   }
-}
-
 static void rerere_io_putmem(const char *mem, size_t sz, struct rerere_io *io)
 {
if (io->output)
@@ -384,37 +352,25 @@ static int is_cmarker(char *buf, int marker_char, int 
marker_size)
return isspace(*buf);
 }
 
-/*
- * Read contents a file with conflicts, normalize the conflicts
- * by (1) discarding the common ancestor version in diff3-style,
- * (2) reordering our side and their side so that whichever sorts
- * alphabetically earlier comes before the other one, while
- * computing the "conflict ID", which is just an SHA-1 hash of
- * one side of the conflict, NUL, the other side of the conflict,
- * and NUL concatenated together.
- *
- * Return 1 if conflict hunks are found, 0 if there are no conflict
- * hunks and -1 if an error occured.
- */
-static int handle_path(unsigned char *sha1, struct rerere_io *io, int 
marker_size)
+static void rerere_strbuf_putconflict(struct strbuf *buf, int ch, size_t size)
+{
+   strbuf_addchars(buf, ch, size);
+   strbuf_addch(buf, '\n');
+}
+
+static int handle_conflict(struct strbuf *out, struct rerere_io *io,
+  int marker_size, git_SHA_CTX *ctx)
 {
-   git_SHA_CTX ctx;
-   int has_conflicts = 0;
enum {
-   RR_CONTEXT = 0, RR_SIDE_1, RR_SIDE_2, RR_ORIGINAL
-   } hunk = RR_CONTEXT;
+   RR_SIDE_1 = 0, RR_SIDE_2, RR_ORIGINAL
+   } hunk = RR_SIDE_1;
struct strbuf one = STRBUF_INIT, two = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
-
-   if (sha1)
-   git_SHA1_Init();
-
+   int has_conflicts = 1;
while (!io->getline(, io)) {
-   if (is_cmarker(buf.buf, '<', marker_size)) {
-   if (hunk != RR_CONTEXT)
-   goto bad;
-   hunk = RR_SIDE_1;
-   } else if (is_cmarker(buf.buf, '|', marker_size)) {
+   if (is_cmarker(buf.buf, '<', marker_size))
+   goto bad;
+   else if (is_cmarker(buf.buf, '|', marker_size)) {
if (hunk != RR_SIDE_1)
goto bad;
hunk = RR_ORIGINAL;
@@ -427,42 +383,73 @@ static int handle_path(unsigned char *sha1, struct 
rerere_io *io, int marker_siz
goto bad;
if (strbuf_cmp(, ) > 0)
strbuf_swap(, );
-   has_conflicts = 1;
-   hunk = RR_CONTEXT;
-   rerere_io_putconflict('<', marker_size, io);
-   rerere_io_putmem(one.buf, one.len, io);
-   rerere_io_putconflict('=', marker_size, io);
-   rerere_io_putmem(two.buf, two.len, io);
-   rerere_io_putconflict('>', marker_size, io);
-   if (sha1) {
-   git_SHA1_Update(, one.buf ? one.buf : "",
+   rerere_strbuf_putconflict(out, '<', marker_size);
+   strbuf_addbuf(out, );
+   rerere_strbuf_putconflict(out, '=', marker_size);
+   strbuf_addbuf(out, );
+   rerere_strbuf_putconflict(out, '>', 

[RFC/PATCH 4/7] rerere: fix crash when conflict goes unresolved

[Thomas Gummerer]

Currently when a user doesn't resolve a conflict in a file, but
commits the file with the conflict markers, and later the file ends up
in a state in which rerere can't handle it, subsequent rerere
operations that are interested in that path, such as 'rerere clear' or
'rerere forget ' will fail, or even worse in the case of 'rerere
clear' segfault.

Such states include nested conflicts, or an extra conflict marker that
doesn't have any match.

This is because the first 'git rerere' when there was only one
conflict in the file leaves an entry in the MERGE_RR file behind.  The
next 'git rerere' will then pick the rerere ID for that file up, and
not assign a new ID as it can't successfully calculate one.  It will
however still try to do the rerere operation, because of the existing
ID.  As the handle_file function fails, it will remove the 'preimage'
for the ID in the process, while leaving the ID in the MERGE_RR file.

Now when 'rerere clear' for example is run, it will segfault in
'has_rerere_resolution', because status is NULL.

To fix this, remove the rerere ID from the MERGE_RR file in case we
can't handle it, and remove the folder for the ID.  Removing it
unconditionally is fine here, because if the user would have resolved
the conflict and ran rerere, the entry would no longer be in the
MERGE_RR file, so we wouldn't have this problem in the first place,
while if the conflict was not resolved, the only thing that's left in
the folder is the 'preimage', which by itself will be regenerated by
git if necessary, so the user won't loose any work.

Signed-off-by: Thomas Gummerer 
---

I realize the test here may not be as complete as we would want it to
be.  But I first wanted to get some feedback on the approach, before
spending too much time on a proper test (I did test it manually, and
the test does show that the original problem is fixed, but it probably
deserves some cleanup).

 rerere.c  | 12 +++-
 t/t4200-rerere.sh | 25 +
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/rerere.c b/rerere.c
index a02a38e072..49ace8e108 100644
--- a/rerere.c
+++ b/rerere.c
@@ -824,10 +824,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)
struct rerere_id *id;
unsigned char sha1[20];
const char *path = conflict.items[i].string;
-   int ret;
-
-   if (string_list_has_string(rr, path))
-   continue;
+   int ret, has_string;
 
/*
 * Ask handle_file() to scan and assign a
@@ -835,7 +832,12 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 * yet.
 */
ret = handle_file(path, sha1, NULL);
-   if (ret < 1)
+   has_string = string_list_has_string(rr, path);
+   if (ret < 0 && has_string) {
+   remove_variant(string_list_lookup(rr, path)->util);
+   string_list_remove(rr, path, 1);
+   }
+   if (ret < 1 || has_string)
continue;
 
id = new_rerere_id(sha1);
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index eaf18c81cb..27f8afc0b4 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -580,4 +580,29 @@ test_expect_success 'multiple identical conflicts' '
count_pre_post 0 0
 '
 
+test_expect_success 'rerere with extra conflict markers keeps working' '
+   git reset --hard &&
+
+   git checkout -b branch-1 master &&
+   echo "bar" >test &&
+   git add test &&
+   git commit -q -m two &&
+   echo "baz" >test &&
+   git add test &&
+   git commit -q -m three &&
+
+   git reset --hard &&
+   git checkout -b branch-2 master &&
+   echo "foo" >test &&
+   git add test &&
+   git commit -q -a -m one &&
+
+   test_must_fail git merge branch-1~ &&
+   git add test &&
+   git commit -q -m "will solve conflicts later" &&
+   test_must_fail git merge branch-1 &&
+
+   git rerere clear
+'
+
 test_done
-- 
2.17.0.588.g4d217cdf8e.dirty

[RFC/PATCH 7/7] rerere: teach rerere to handle nested conflicts

[Thomas Gummerer]

Currently rerere can't handle nested conflicts and will error out when
it encounters such conflicts.  Do that by recursively calling the
'handle_conflict' function to normalize the conflict.

The conflict ID calculation here deserves some explanation:

As we are using the same handle_conflict function, the nested conflict
is normalized the same way as for non-nested conflicts, which means
the ancestor in the diff3 case is stripped out, and the parts of the
conflict are ordered alphabetically.

The conflict ID is however is only calculated in the top level
handle_conflict call, so it will include the markers that 'rerere'
adds to the output.  e.g. say there's the following conflict:

<<< HEAD
1
===
<<< HEAD
3
===
2
>>> branch-2
>>> branch-3~

it would be reordered as follows in the preimage:

<<<
1
===
<<<
2
===
3
>>>
>>>

and the conflict ID would be calculated as

sha1(1<<<
2
===
3
>>>)

Stripping out vs. leaving the conflict markers in place should have no
practical impact, but it simplifies the implementation.

Signed-off-by: Thomas Gummerer 
---

No automated test for this yet.  As mentioned in the cover letter as
well, I'm not sure if this is common enough for us to actually
consider this use case.  I don't know how nested conflicts could
actually be created apart from committing a file with conflict
markers, but maybe I'm just lacking imagination, so if someone has an
example for that I would be very grateful :)  If we decide to do this,
it probably also merits a mention in
Documentation/technical/rerere.txt.

 rerere.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/rerere.c b/rerere.c
index f3cfd1c09b..45e2bd6ff1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -365,12 +365,18 @@ static int handle_conflict(struct strbuf *out, struct 
rerere_io *io,
RR_SIDE_1 = 0, RR_SIDE_2, RR_ORIGINAL
} hunk = RR_SIDE_1;
struct strbuf one = STRBUF_INIT, two = STRBUF_INIT;
-   struct strbuf buf = STRBUF_INIT;
+   struct strbuf buf = STRBUF_INIT, conflict = STRBUF_INIT;
int has_conflicts = 1;
while (!io->getline(, io)) {
-   if (is_cmarker(buf.buf, '<', marker_size))
-   goto bad;
-   else if (is_cmarker(buf.buf, '|', marker_size)) {
+   if (is_cmarker(buf.buf, '<', marker_size)) {
+   if (handle_conflict(, io, marker_size, NULL) < 
0)
+   goto bad;
+   if (hunk == RR_SIDE_1)
+   strbuf_addbuf(, );
+   else
+   strbuf_addbuf(, );
+   strbuf_release();
+   } else if (is_cmarker(buf.buf, '|', marker_size)) {
if (hunk != RR_SIDE_1)
goto bad;
hunk = RR_ORIGINAL;
-- 
2.17.0.588.g4d217cdf8e.dirty

[RFC/PATCH 3/7] rerere: add some documentation

[Thomas Gummerer]

Add some documentation for the logic behind the conflict normalization
in rerere.  Also describe a bug that happens because we just linearly
scan for conflict markers.

Signed-off-by: Thomas Gummerer 
---

This documents my understanding of the rerere conflict normalization
and conflict ID computation logic.  Writing this down helped me
understand the logic, and I thought it may be useful to have this as
documentation in Documentation/technical as well.  Junio: as you wrote
the original NEEDSWORK comment, did you have something more in mind
here that should be documented?

 Documentation/technical/rerere.txt | 43 ++
 rerere.c   |  4 ---
 2 files changed, 43 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/technical/rerere.txt

diff --git a/Documentation/technical/rerere.txt 
b/Documentation/technical/rerere.txt
new file mode 100644
index 00..94cc6a7ef0
--- /dev/null
+++ b/Documentation/technical/rerere.txt
@@ -0,0 +1,43 @@
+Rerere
+==
+
+This document describes the rerere logic.
+
+Conflict normalization
+--
+
+To try and re-do a conflict resolution, even when different merge
+strategies are used, 'rerere' computes a conflict ID for each
+conflict in the file.
+
+This is done by discarding the common ancestor version in the
+diff3-style, and re-ordering the two sides of the conflict, in
+alphabetic order.
+
+Using this technique a conflict that looks as follows when for example
+'master' was merged into a topic branch:
+
+<<< HEAD
+foo
+===
+bar
+>>> master
+
+and the opposite way when the topic branch is merged into 'master':
+
+<<< HEAD
+bar
+===
+foo
+>>> topic
+
+can be recognized as the same conflict, and can automatically be
+re-resolved by 'rerere', as the SHA-1 sum of the two conflicts would
+be calculated from 'barfoo' in both cases.
+
+If there are multiple conflicts in one file, they are all appended to
+one another, both in the 'preimage' file as well as in the conflict
+ID.
+
+This is currently implemented by simply scanning through the file and
+looking for conflict markers.
diff --git a/rerere.c b/rerere.c
index af5e6179a9..a02a38e072 100644
--- a/rerere.c
+++ b/rerere.c
@@ -394,10 +394,6 @@ static int is_cmarker(char *buf, int marker_char, int 
marker_size)
  * and NUL concatenated together.
  *
  * Return the number of conflict hunks found.
- *
- * NEEDSWORK: the logic and theory of operation behind this conflict
- * normalization may deserve to be documented somewhere, perhaps in
- * Documentation/technical/rerere.txt.
  */
 static int handle_path(unsigned char *sha1, struct rerere_io *io, int 
marker_size)
 {
-- 
2.17.0.588.g4d217cdf8e.dirty

[RFC/PATCH 2/7] rerere: mark strings for translation

[Thomas Gummerer]

'git rerere' is considered a plumbing command and as such its output
should be translated.  Its functionality is also only enabled through
a config setting, so scripts really shouldn't rely on its output
either way.

Signed-off-by: Thomas Gummerer 
---
 rerere.c | 68 
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/rerere.c b/rerere.c
index 4b4869662d..af5e6179a9 100644
--- a/rerere.c
+++ b/rerere.c
@@ -212,7 +212,7 @@ static void read_rr(struct string_list *rr)
 
/* There has to be the hash, tab, path and then NUL */
if (buf.len < 42 || get_sha1_hex(buf.buf, sha1))
-   die("corrupt MERGE_RR");
+   die(_("corrupt MERGE_RR"));
 
if (buf.buf[40] != '.') {
variant = 0;
@@ -221,10 +221,10 @@ static void read_rr(struct string_list *rr)
errno = 0;
variant = strtol(buf.buf + 41, , 10);
if (errno)
-   die("corrupt MERGE_RR");
+   die(_("corrupt MERGE_RR"));
}
if (*(path++) != '\t')
-   die("corrupt MERGE_RR");
+   die(_("corrupt MERGE_RR"));
buf.buf[40] = '\0';
id = new_rerere_id_hex(buf.buf);
id->variant = variant;
@@ -259,12 +259,12 @@ static int write_rr(struct string_list *rr, int out_fd)
rr->items[i].string, 0);
 
if (write_in_full(out_fd, buf.buf, buf.len) < 0)
-   die("unable to write rerere record");
+   die(_("unable to write rerere record"));
 
strbuf_release();
}
if (commit_lock_file(_lock) != 0)
-   die("unable to write rerere record");
+   die(_("unable to write rerere record"));
return 0;
 }
 
@@ -484,12 +484,12 @@ static int handle_file(const char *path, unsigned char 
*sha1, const char *output
io.input = fopen(path, "r");
io.io.wrerror = 0;
if (!io.input)
-   return error_errno("Could not open %s", path);
+   return error_errno(_("Could not open %s"), path);
 
if (output) {
io.io.output = fopen(output, "w");
if (!io.io.output) {
-   error_errno("Could not write %s", output);
+   error_errno(_("Could not write %s"), output);
fclose(io.input);
return -1;
}
@@ -499,15 +499,15 @@ static int handle_file(const char *path, unsigned char 
*sha1, const char *output
 
fclose(io.input);
if (io.io.wrerror)
-   error("There were errors while writing %s (%s)",
+   error(_("There were errors while writing %s (%s)"),
  path, strerror(io.io.wrerror));
if (io.io.output && fclose(io.io.output))
-   io.io.wrerror = error_errno("Failed to flush %s", path);
+   io.io.wrerror = error_errno(_("Failed to flush %s"), path);
 
if (hunk_no < 0) {
if (output)
unlink_or_warn(output);
-   return error("Could not parse conflict hunks in %s", path);
+   return error(_("Could not parse conflict hunks in %s"), path);
}
if (io.io.wrerror)
return -1;
@@ -568,7 +568,7 @@ static int find_conflict(struct string_list *conflict)
 {
int i;
if (read_cache() < 0)
-   return error("index file corrupt");
+   return error(_("index file corrupt"));
 
for (i = 0; i < active_nr;) {
int conflict_type;
@@ -601,7 +601,7 @@ int rerere_remaining(struct string_list *merge_rr)
if (setup_rerere(merge_rr, RERERE_READONLY))
return 0;
if (read_cache() < 0)
-   return error("index file corrupt");
+   return error(_("index file corrupt"));
 
for (i = 0; i < active_nr;) {
int conflict_type;
@@ -684,17 +684,17 @@ static int merge(const struct rerere_id *id, const char 
*path)
 * Mark that "postimage" was used to help gc.
 */
if (utime(rerere_path(id, "postimage"), NULL) < 0)
-   warning_errno("failed utime() on %s",
+   warning_errno(_("failed utime() on %s"),
  rerere_path(id, "postimage"));
 
/* Update "path" with the resolution */
f = fopen(path, "w");
if (!f)
-   return error_errno("Could not open %s", path);
+   return error_errno(_("Could not open %s"), path);
if (fwrite(result.ptr, result.size, 1, f) != 1)
-   error_errno("Could not write %s", path);
+   error_errno(_("Could not 

[RFC/PATCH 0/7] rerere: handle nested conflicts

[Thomas Gummerer]

I started this whole patch series when I did a git rebase, and was too
lazy to resolve a conflict and just added the file with the conflict
markers and continued.  Once I got nested conflicts in the file, I
decided to abort the rebase with 'git rebase --abort' and got a
segfault in 'git rerere clear'.

Even if we can't handle the conflict, we shouldn't end with crashing
'git rerere clear'.  While trying to understand how 'git rerere' works
internally I noticed some other improvements that could be made, such
as marking the strings for translation and adding some docs on how
rerere works, since I had to find out from the code, and reading some
documentation would definitely have been helpful.

The next patches are more related to the actual problem I encountered,
first fixing the the possible crashing of 'git rerere clear' when we
can't handle conflicts in a file, and then actually trying to handle
nested conflicts.

I don't know if it's actually worth trying to handle nested conflicts,
as they are more than likely a very rare use-case, but on the other
hand resolving such conflicts is especially painful, so only having to
do it once would be much nicer.

This whole patch series is marked as RFC/PATCH, as this is my first
time touching the rerere code, so I may well misunderstand some bits
of the code.

Thomas Gummerer (7):
  rerere: unify error message when read_cache fails
  rerere: mark strings for translation
  rerere: add some documentation
  rerere: fix crash when conflict goes unresolved
  rerere: only return whether a path has conflicts or not
  rerere: factor out handle_conflict function
  rerere: teach rerere to handle nested conflicts

 Documentation/technical/rerere.txt |  43 +
 rerere.c   | 244 ++---
 t/t4200-rerere.sh  |  25 +++
 3 files changed, 186 insertions(+), 126 deletions(-)
 create mode 100644 Documentation/technical/rerere.txt

-- 
2.17.0.588.g4d217cdf8e.dirty

[RFC/PATCH 1/7] rerere: unify error message when read_cache fails

[Thomas Gummerer]

We have multiple different variants of the error message we show to
the user if 'read_cache' fails.  The "Could not read index" variant we
are using in 'rerere.c' is currently not used anywhere in translated
form.

As a subsequent commit will mark all output that comes from 'rerere.c'
for translation, make the life of the translators a little bit easier
by using a string that is used elsewhere, and marked for translation
there, and thus most likely already translated.

"index file corrupt" seems to be the most common error message we show
when 'read_cache' fails, so use that here as well.

Signed-off-by: Thomas Gummerer 
---

"index file corrupt" is also what Stefan chose for his series unifying
these error messages (and 'die'ing, which I'm not sure is the right
thing to do here as also mentioned in my reply to [1]).  I'm happy to
drop this if we decide to go with that series.

[1]: <20180516222118.233868-8-sbel...@google.com>

 rerere.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/rerere.c b/rerere.c
index 18cae2d11c..4b4869662d 100644
--- a/rerere.c
+++ b/rerere.c
@@ -568,7 +568,7 @@ static int find_conflict(struct string_list *conflict)
 {
int i;
if (read_cache() < 0)
-   return error("Could not read index");
+   return error("index file corrupt");
 
for (i = 0; i < active_nr;) {
int conflict_type;
@@ -601,7 +601,7 @@ int rerere_remaining(struct string_list *merge_rr)
if (setup_rerere(merge_rr, RERERE_READONLY))
return 0;
if (read_cache() < 0)
-   return error("Could not read index");
+   return error("index file corrupt");
 
for (i = 0; i < active_nr;) {
int conflict_type;
@@ -1104,7 +1104,7 @@ int rerere_forget(struct pathspec *pathspec)
struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
if (read_cache() < 0)
-   return error("Could not read index");
+   return error("index file corrupt");
 
fd = setup_rerere(_rr, RERERE_NOAUTOUPDATE);
if (fd < 0)
-- 
2.17.0.588.g4d217cdf8e.dirty

Re: [PATCH v2 02/12] commit-graph: verify file header information

[Jakub Narebski]

Derrick Stolee  writes:

> During a run of 'git commit-graph verify', list the issues with the
> header information in the commit-graph file. Some of this information
> is inferred from the loaded 'struct commit_graph'. Some header
> information is checked as part of load_commit_graph_one().
>
> Signed-off-by: Derrick Stolee 
> ---
>  commit-graph.c | 32 +++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index b25aaed128..d2db20e49a 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -818,7 +818,37 @@ void write_commit_graph(const char *obj_dir,
>   oids.nr = 0;
>  }
>  
> +static int verify_commit_graph_error;
> +
> +static void graph_report(const char *fmt, ...)
> +{
> + va_list ap;
> + struct strbuf sb = STRBUF_INIT;
> + verify_commit_graph_error = 1;
> +
> + va_start(ap, fmt);
> + strbuf_vaddf(, fmt, ap);
> +
> + fprintf(stderr, "%s\n", sb.buf);
> + strbuf_release();
> + va_end(ap);
> +}
> +
>  int verify_commit_graph(struct commit_graph *g)
>  {
> - return !g;
> + if (!g) {
> + graph_report("no commit-graph file loaded");
> + return 1;
> + }

I won't be repeating what Martin said, but I agree with it.  Well, that
or make it a separate patch.

> +
> + verify_commit_graph_error = 0;
> +

A quick reminder for myself.  The load_commit_graph_one() that is used
to fill the commit_graph parameter alreaady verifies that:
 - file is not too small, i.e. smaller than GRAPH_MIN_SIZE
 - it has correct signature
 - it has correct graph version
 - it has correct hash version
 - chunks [offsets] all fit within file
 - that OID Fanout, OID Lookup, Commit Data and Large Edges chunks are
   not repeated, though not that all required chunks are present

> + if (!g->chunk_oid_fanout)
> + graph_report("commit-graph is missing the OID Fanout chunk");
> + if (!g->chunk_oid_lookup)
> + graph_report("commit-graph is missing the OID Lookup chunk");
> + if (!g->chunk_commit_data)
> + graph_report("commit-graph is missing the Commit Data chunk");

This one checks that all chunks that needs to be present are present.
Nice.

There are a few more things that we can check about CHUNK LOOKUP part.
For example we would detect if file was truncated because the offset of
some chunk would be pointing outside the file... unless the truncation
falls within the last chunk.  We don't check that terminating label
(chunk "\0\0\0\0" offset) is outside file, I think.

We also don't check that positions of subsequent chunks are sorted
(increasing offsets), so that each chunk length is positive.


I also wonder if we shouldn't at least _warn_ about unknown chunks.

> +
> + return verify_commit_graph_error;
>  }

Nice trick to be able to check as much as possible without segfaulting,
while still returning correct error result.


Testing newly intruduced functionality would be hard, unless relying on
hand-crafted files, or on some helper to produce invalid commit-graph
files.

-- 
Jakub Narębski

Re: [GSoC] A blog about 'git stash' project

[Paul-Sebastian Ungureanu]

Hi

On 17.05.2018 13:29, Kaartic Sivaraam wrote:

On Thursday 17 May 2018 02:39 PM, Johannes Schindelin wrote:

I have great empathy for the desire to see these bugs fixed. The
conversion must come first, though, and in the interest of making it
easier on me and other reviewers, I must insist on keeping the conversion
free of any changes, much in the way as we try to avoid evil merges (i.e.
merge commits that introduce changes that were not present in any of their
parents).



Of course, the conversion should be separate from the bug fixes :-)

When I mentioned "while porting it to C", I actually meant the "thought
process of creating a foundation for `git-stash` in C". I thought
hinting at some of the existing and unsolved `git-stash` bugs would
allow the person who would be doing the port of `git-stash` to C to
consider how to avoid this while implementing the basic foundation. I
should have been more explicit about this in my previous mails.



Thank you! I will keep in mind to fix those bugs. I actually wrote a 
short paragraph about one of them (the one regarding -p option) on the 
blog (the first post).


Best,
Paul

Re: [PATCH v4 3/4] string-list: provide `string_list_appendf()`

[Jacob Keller]

On Sun, May 20, 2018 at 3:17 AM, Martin Ågren  wrote:
> +/**
> + * Add formatted string to the end of `list`. This function ignores
> + * the value of `list->strdup_strings` and always appends a freshly
> + * allocated string, so you will probably not want to use it with
> + * `strdup_strings = 0`.
> + */
> +struct string_list_item *string_list_appendf(struct string_list *list,
> +const char *fmt, ...);
> +

Would it make sense to verify that strdup_strings == 0? I guess we'd
have to use die or BUG(), but that would mean that the program could
crash..

I doubt this could be verified at compilation time

Thanks,
Jake

[GSoC] GSoC with git, week 3

[Alban Gruin]

Hi,

I published a new post about this week. You can read it here:

https://blog.pa1ch.fr/posts/2018/05/20/en/gsoc2018-week-3.html

Feel free to give me your feedback! :)

Cheers,
Alban

[PATCH v2 17/17] completion: allow to customize the completable command list

[Nguyễn Thái Ngọc Duy]

By default we show porcelain, external commands and a couple others
that are also popular. If you are not happy with this list, you can
now customize it a new config variable.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/config.txt   |  8 +++
 Documentation/git.txt  |  3 ++-
 contrib/completion/git-completion.bash |  2 +-
 git.c  |  2 ++
 help.c | 33 ++
 help.h |  1 +
 6 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..9e81dcf867 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1343,6 +1343,14 @@ credential..*::
 credentialCache.ignoreSIGHUP::
Tell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.
 
+completion.commands::
+   This is only used by git-completion.bash to add or remove
+   commands from the list of completed commands. Normally only
+   porcelain commands and a few select others are completed. You
+   can add more commands, separated by space, in this
+   variable. Prefixing the command with '-' will remove it from
+   the existing list.
+
 include::diff-config.txt[]
 
 difftool..path::
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 75f50d2379..6f7eddf847 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -170,7 +170,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
parse-options), main (all commands in libexec directory),
others (all other commands in `$PATH` that have git- prefix),
list- (see categories in command-list.txt),
-   nohelpers (exclude helper commands) and alias.
+   nohelpers (exclude helper commands), alias and config
+   (retrieve command list from config variable completion.commands)
 
 GIT COMMANDS
 
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 98f278fb9a..e5b2ccbdd2 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3012,7 +3012,7 @@ __git_main ()
then
__gitcomp "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
else
-   __gitcomp "$(git 
--list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete)"
+   __gitcomp "$(git 
--list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
fi
;;
esac
diff --git a/git.c b/git.c
index 63acd9ea81..447dac0e71 100644
--- a/git.c
+++ b/git.c
@@ -77,6 +77,8 @@ static int list_cmds(const char *spec)
exclude_helpers_from_list();
else if (match_token(spec, len, "alias"))
list_aliases();
+   else if (match_token(spec, len, "config"))
+   list_cmds_by_config();
else if (len > 5 && !strncmp(spec, "list-", 5)) {
struct strbuf sb = STRBUF_INIT;
 
diff --git a/help.c b/help.c
index 23924dd300..abf87205b2 100644
--- a/help.c
+++ b/help.c
@@ -366,6 +366,39 @@ void list_cmds_by_category(struct string_list *list,
}
 }
 
+void list_cmds_by_config(struct string_list *list)
+{
+   const char *cmd_list;
+
+   /*
+* There's no actual repository setup at this point (and even
+* if there is, we don't really care; only global config
+* matters). If we accidentally set up a repository, it's ok
+* too since the caller (git --list-cmds=) should exit shortly
+* anyway.
+*/
+   if (git_config_get_string_const("completion.commands", _list))
+   return;
+
+   string_list_sort(list);
+   string_list_remove_duplicates(list, 0);
+
+   while (*cmd_list) {
+   struct strbuf sb = STRBUF_INIT;
+   const char *p = strchrnul(cmd_list, ' ');
+
+   strbuf_add(, cmd_list, p - cmd_list);
+   if (*cmd_list == '-')
+   string_list_remove(list, cmd_list + 1, 0);
+   else
+   string_list_insert(list, sb.buf);
+   strbuf_release();
+   while (*p == ' ')
+   p++;
+   cmd_list = p;
+   }
+}
+
 void list_common_guides_help(void)
 {
struct category_description catdesc[] = {
diff --git a/help.h b/help.h
index b2293e99be..3b38292a1b 100644
--- a/help.h
+++ b/help.h
@@ -26,6 +26,7 @@ extern void list_all_main_cmds(struct string_list *list);
 extern void list_all_other_cmds(struct string_list *list);
 extern void list_cmds_by_category(struct string_list *list,
  const char *category);
+extern void list_cmds_by_config(struct 

[PATCH v2 12/17] completion: let git provide the completable command list

[Nguyễn Thái Ngọc Duy]

Instead of maintaining a separate list of command classification,
which often could go out of date, let's centralize the information
back in git.

While the function in git-completion.bash implies "list porcelain
commands", that's not exactly what it does. It gets all commands (aka
--list-cmds=main,others) then exclude certain non-porcelain ones. We
could almost recreate this list two lists list-mainporcelain and
others. The non-porcelain-but-included-anyway is added by the third
category list-complete.

Note that the current completion script incorrectly classifies
filter-branch as porcelain and t9902 tests this behavior. We keep it
this way in t9902 because this test does not really care which
particular command is porcelain or plumbing, they're just names.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 command-list.txt   |  53 +--
 contrib/completion/git-completion.bash | 119 ++---
 t/t9902-completion.sh  |   5 +-
 3 files changed, 58 insertions(+), 119 deletions(-)

diff --git a/command-list.txt b/command-list.txt
index a2f360eab9..dcf1907a54 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -47,12 +47,12 @@
 # command name  category [category] [category]
 git-add mainporcelain   worktree
 git-am  mainporcelain
-git-annotateancillaryinterrogators
-git-apply   plumbingmanipulators
+git-annotateancillaryinterrogators  
complete
+git-apply   plumbingmanipulators
complete
 git-archimport  foreignscminterface
 git-archive mainporcelain
 git-bisect  mainporcelain   info
-git-blame   ancillaryinterrogators
+git-blame   ancillaryinterrogators  
complete
 git-branch  mainporcelain   history
 git-bundle  mainporcelain
 git-cat-fileplumbinginterrogators
@@ -62,7 +62,7 @@ git-check-mailmap   purehelpers
 git-checkoutmainporcelain   history
 git-checkout-index  plumbingmanipulators
 git-check-ref-formatpurehelpers
-git-cherry  ancillaryinterrogators
+git-cherry  ancillaryinterrogators  
complete
 git-cherry-pick mainporcelain
 git-citool  mainporcelain
 git-clean   mainporcelain
@@ -70,7 +70,7 @@ git-clone   mainporcelain 
  init
 git-column  purehelpers
 git-commit  mainporcelain   history
 git-commit-tree plumbingmanipulators
-git-config  ancillarymanipulators
+git-config  ancillarymanipulators   
complete
 git-count-objects   ancillaryinterrogators
 git-credential  purehelpers
 git-credential-cachepurehelpers
@@ -84,30 +84,30 @@ git-diffmainporcelain   
history
 git-diff-files  plumbinginterrogators
 git-diff-index  plumbinginterrogators
 git-diff-tree   plumbinginterrogators
-git-difftoolancillaryinterrogators
+git-difftoolancillaryinterrogators  
complete
 git-fast-export ancillarymanipulators
 git-fast-import ancillarymanipulators
 git-fetch   mainporcelain   remote
 git-fetch-pack  synchingrepositories
-git-filter-branch   ancillarymanipulators
+git-filter-branch   ancillarymanipulators   
complete
 git-fmt-merge-msg   purehelpers
 git-for-each-refplumbinginterrogators
 git-format-patchmainporcelain
-git-fsckancillaryinterrogators
+git-fsckancillaryinterrogators  
complete
 git-gc  mainporcelain
-git-get-tar-commit-id   ancillaryinterrogators
+git-get-tar-commit-id   ancillaryinterrogators  
complete
 git-grepmainporcelain   info
 git-gui mainporcelain
 git-hash-object plumbingmanipulators
-git-help

[PATCH v2 15/17] completion: add and use --list-cmds=nohelpers

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git.txt  |  3 ++-
 contrib/completion/git-completion.bash | 20 
 git.c  | 14 ++
 3 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index c423810eac..820ab77fcb 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -169,7 +169,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
groups are: builtins, parseopt (builtin commands that use
parse-options), main (all commands in libexec directory),
others (all other commands in `$PATH` that have git- prefix),
-   list- (see categories in command-list.txt)
+   list- (see categories in command-list.txt),
+   nohelpers (exclude helper commands).
 
 GIT COMMANDS
 
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index cd1d8e553f..217c8a3d3b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -843,7 +843,7 @@ __git_commands () {
then
printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
else
-   git --list-cmds=list-mainporcelain,others,list-complete
+   git 
--list-cmds=list-mainporcelain,others,nohelpers,list-complete
fi
;;
all)
@@ -851,27 +851,15 @@ __git_commands () {
then
printf "%s" "$GIT_TESTING_ALL_COMMAND_LIST"
else
-   git --list-cmds=main,others
+   git --list-cmds=main,others,nohelpers
fi
;;
esac
 }
 
-__git_list_commands ()
-{
-   local i IFS=" "$'\n'
-   for i in $(__git_commands $1)
-   do
-   case $i in
-   *--*) : helper pattern;;
-   *) echo $i;;
-   esac
-   done
-}
-
 __git_list_all_commands ()
 {
-   __git_list_commands all
+   __git_commands all
 }
 
 __git_all_commands=
@@ -883,7 +871,7 @@ __git_compute_all_commands ()
 
 __git_list_porcelain_commands ()
 {
-   __git_list_commands porcelain
+   __git_commands porcelain
 }
 
 __git_porcelain_commands=
diff --git a/git.c b/git.c
index 19f73b3fa3..f6ae79ffaf 100644
--- a/git.c
+++ b/git.c
@@ -39,6 +39,18 @@ static int use_pager = -1;
 
 static void list_builtins(struct string_list *list, unsigned int 
exclude_option);
 
+static void exclude_helpers_from_list(struct string_list *list)
+{
+   int i = 0;
+
+   while (i < list->nr) {
+   if (strstr(list->items[i].string, "--"))
+   unsorted_string_list_delete_item(list, i, 0);
+   else
+   i++;
+   }
+}
+
 static int match_token(const char *spec, int len, const char *token)
 {
int token_len = strlen(token);
@@ -61,6 +73,8 @@ static int list_cmds(const char *spec)
list_all_main_cmds();
else if (match_token(spec, len, "others"))
list_all_other_cmds();
+   else if (match_token(spec, len, "nohelpers"))
+   exclude_helpers_from_list();
else if (len > 5 && !strncmp(spec, "list-", 5)) {
struct strbuf sb = STRBUF_INIT;
 
-- 
2.17.0.705.g3525833791

[PATCH v2 05/17] git.c: convert --list-* to --list-cmds=*

[Nguyễn Thái Ngọc Duy]

Even if these are hidden options, let's make them a bit more generic
since we're introducing more listing types shortly. The code is
structured to allow combining multiple listing types together because
we will soon add more types the 'builtins'.

'parseopt' remains separate because it has separate (SPC) to match
git-completion.bash needs and will not combine with others.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git.txt  |  6 +
 contrib/completion/git-completion.bash |  2 +-
 git.c  | 37 +-
 t/t0012-help.sh|  2 +-
 4 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..2800e3d188 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -163,6 +163,12 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
Do not perform optional operations that require locks. This is
equivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.
 
+--list-cmds=group[,group...]::
+   List commands by group. This is an internal/experimental
+   option and may change or be removed in the future. Supported
+   groups are: builtins, parseopt (builtin commands that use
+   parse-options).
+
 GIT COMMANDS
 
 
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index a757073945..3556838759 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3049,7 +3049,7 @@ __git_complete_common () {
 __git_cmds_with_parseopt_helper=
 __git_support_parseopt_helper () {
test -n "$__git_cmds_with_parseopt_helper" ||
-   __git_cmds_with_parseopt_helper="$(__git 
--list-parseopt-builtins)"
+   __git_cmds_with_parseopt_helper="$(__git --list-cmds=parseopt)"
 
case " $__git_cmds_with_parseopt_helper " in
*" $1 "*)
diff --git a/git.c b/git.c
index 3a89893712..cd85355d81 100644
--- a/git.c
+++ b/git.c
@@ -38,6 +38,30 @@ static int use_pager = -1;
 
 static void list_builtins(unsigned int exclude_option, char sep);
 
+static int match_token(const char *spec, int len, const char *token)
+{
+   int token_len = strlen(token);
+
+   return len == token_len && !strncmp(spec, token, token_len);
+}
+
+static int list_cmds(const char *spec)
+{
+   while (*spec) {
+   const char *sep = strchrnul(spec, ',');
+   int len = sep - spec;
+
+   if (match_token(spec, len, "builtins"))
+   list_builtins(0, '\n');
+   else
+   die(_("unsupported command listing type '%s'"), spec);
+   spec += len;
+   if (*spec == ',')
+   spec++;
+   }
+   return 0;
+}
+
 static void commit_pager_choice(void) {
switch (use_pager) {
case 0:
@@ -223,12 +247,13 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
}
(*argv)++;
(*argc)--;
-   } else if (!strcmp(cmd, "--list-builtins")) {
-   list_builtins(0, '\n');
-   exit(0);
-   } else if (!strcmp(cmd, "--list-parseopt-builtins")) {
-   list_builtins(NO_PARSEOPT, ' ');
-   exit(0);
+   } else if (skip_prefix(cmd, "--list-cmds=", )) {
+   if (!strcmp(cmd, "parseopt")) {
+   list_builtins(NO_PARSEOPT, ' ');
+   exit(0);
+   } else {
+   exit(list_cmds(cmd));
+   }
} else {
fprintf(stderr, _("unknown option: %s\n"), cmd);
usage(git_usage_string);
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index c096f33505..3c91a9024a 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -59,7 +59,7 @@ test_expect_success 'git help' '
 '
 
 test_expect_success 'generate builtin list' '
-   git --list-builtins >builtins
+   git --list-cmds=builtins >builtins
 '
 
 while read builtin
-- 
2.17.0.705.g3525833791

[PATCH v2 09/17] help: add "-a --verbose" to list all commands with synopsis

[Nguyễn Thái Ngọc Duy]

This lists all recognized commands [1] by category. The group order
follows closely git.txt.

[1] We may actually show commands that are not built (e.g. if you set
NO_PERL you don't have git-instaweb but it's still listed here). I
ignore the problem because on Linux a git package could be split
anyway. The "git-core" package may not contain git-instaweb even if
it's built because it may end up in a separate package. We can't know
anyway.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-help.txt |  4 +++-
 builtin/help.c |  7 +++
 help.c | 16 
 help.h |  2 ++
 t/t0012-help.sh|  9 +
 5 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 40d328a4b3..a40fc38d8b 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 
 [verse]
-'git help' [-a|--all] [-g|--guide]
+'git help' [-a|--all [--verbose]] [-g|--guide]
   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
 
 DESCRIPTION
@@ -42,6 +42,8 @@ OPTIONS
 --all::
Prints all the available commands on the standard output. This
option overrides any given command or guide name.
+   When used with `--verbose` print description for all recognized
+   commands.
 
 -g::
 --guides::
diff --git a/builtin/help.c b/builtin/help.c
index 598867cfea..0e0af8426a 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -36,6 +36,7 @@ static const char *html_path;
 
 static int show_all = 0;
 static int show_guides = 0;
+static int verbose;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
@@ -48,6 +49,7 @@ static struct option builtin_help_options[] = {
HELP_FORMAT_WEB),
OPT_SET_INT('i', "info", _format, N_("show info page"),
HELP_FORMAT_INFO),
+   OPT__VERBOSE(, N_("print command description")),
OPT_END(),
 };
 
@@ -463,6 +465,11 @@ int cmd_help(int argc, const char **argv, const char 
*prefix)
 
if (show_all) {
git_config(git_help_config, NULL);
+   if (verbose) {
+   setup_pager();
+   list_all_cmds_help();
+   return 0;
+   }
printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
load_command_list("git-", _cmds, _cmds);
list_commands(colopts, _cmds, _cmds);
diff --git a/help.c b/help.c
index 1117f7d1d1..c7df1d2338 100644
--- a/help.c
+++ b/help.c
@@ -27,6 +27,17 @@ static struct category_description common_categories[] = {
{ CAT_remote, N_("collaborate (see also: git help workflows)") },
{ 0, NULL }
 };
+static struct category_description main_categories[] = {
+   { CAT_mainporcelain, N_("Main Porcelain Commands") },
+   { CAT_ancillarymanipulators, N_("Ancillary Commands / Manipulators") },
+   { CAT_ancillaryinterrogators, N_("Ancillary Commands / Interrogators") 
},
+   { CAT_foreignscminterface, N_("Interacting with Others") },
+   { CAT_plumbingmanipulators, N_("Low-level Commands / Manipulators") },
+   { CAT_plumbinginterrogators, N_("Low-level Commands / Interrogators") },
+   { CAT_synchingrepositories, N_("Low-level Commands / Synching 
Repositories") },
+   { CAT_purehelpers, N_("Low-level Commands / Internal Helpers") },
+   { 0, NULL }
+};
 
 static const char *drop_prefix(const char *name)
 {
@@ -352,6 +363,11 @@ void list_cmds_by_category(struct string_list *list,
}
 }
 
+void list_all_cmds_help(void)
+{
+   print_cmd_by_category(main_categories);
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index 734bba32d3..40917fc38c 100644
--- a/help.h
+++ b/help.h
@@ -19,6 +19,8 @@ static inline void mput_char(char c, unsigned int num)
 }
 
 extern void list_common_cmds_help(void);
+extern void list_all_cmds_help(void);
+
 extern void list_all_main_cmds(struct string_list *list);
 extern void list_all_other_cmds(struct string_list *list);
 extern void list_cmds_by_category(struct string_list *list,
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 3c91a9024a..060df24c2d 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -25,6 +25,15 @@ test_expect_success "setup" '
EOF
 '
 
+# make sure to exercise these code paths, the output is a bit tricky
+# to verify
+test_expect_success 'basic help commands' '
+   git help >/dev/null &&
+   git help -a >/dev/null &&
+   git help -g >/dev/null &&
+   git help -av >/dev/null
+'
+
 test_expect_success "works for commands and guides by default" '
configure_help &&
git help status &&
-- 
2.17.0.705.g3525833791

[PATCH v2 11/17] command-list.txt: documentation and guide line

[Nguyễn Thái Ngọc Duy]

This is intended to help anybody who needs to update command-list.txt.
It gives a brief introduction of all attributes a command can take.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 command-list.txt | 45 +
 1 file changed, 45 insertions(+)

diff --git a/command-list.txt b/command-list.txt
index 99ddc231c1..a2f360eab9 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -1,3 +1,48 @@
+# Command classification list
+# ---
+# All supported commands, builtin or external, must be described in
+# here. This info is used to list commands in various places. Each
+# command is on one line followed by one or more attributes.
+#
+# The first attribute group is mandatory and indicates the command
+# type. This group includes:
+#
+#   mainporcelain
+#   ancillarymanipulators
+#   ancillaryinterrogators
+#   foreignscminterface
+#   plumbingmanipulators
+#   plumbinginterrogators
+#   synchingrepositories
+#   synchelpers
+#   purehelpers
+#
+# The type names are self explanatory. But if you want to see what
+# command belongs to what group to get a better picture, have a look
+# at "git" man page, "GIT COMMANDS" section.
+#
+# Commands of type mainporcelain can also optionally have one of these
+# attributes:
+#
+#   init
+#   worktree
+#   info
+#   history
+#   remote
+#
+# These commands are considered "common" and will show up in "git
+# help" output in groups. Uncommon porcelain commands must not
+# specify any of these attributes.
+#
+# "complete" attribute is used to mark that the command should be
+# completable by git-completion.bash. Note that by default,
+# mainporcelain commands are completable so you don't need this
+# attribute.
+#
+# As part of the Git man page list, the man(5/7) guides are also
+# specified here, which can only have "guide" attribute and nothing
+# else.
+#
 ### command list (do not change this line, also do not change alignment)
 # command name  category [category] [category]
 git-add mainporcelain   worktree
-- 
2.17.0.705.g3525833791

[PATCH v2 10/17] help: use command-list.txt for the source of guides

[Nguyễn Thái Ngọc Duy]

The help command currently hard codes the list of guides and their
summary in C. Let's move this list to command-list.txt. This lets us
extract summary lines from Documentation/git*.txt. This also
potentially lets us list guides in git.txt, but I'll leave that for
now.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/gitattributes.txt|  2 +-
 Documentation/gitmodules.txt   |  2 +-
 Documentation/gitrevisions.txt |  2 +-
 Makefile   |  2 +-
 builtin/help.c | 32 --
 command-list.txt   | 16 +
 contrib/completion/git-completion.bash | 15 
 help.c | 21 +
 help.h |  1 +
 t/t0012-help.sh|  6 +
 10 files changed, 54 insertions(+), 45 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 1094fe2b5b..083c2f380d 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -3,7 +3,7 @@ gitattributes(5)
 
 NAME
 
-gitattributes - defining attributes per path
+gitattributes - Defining attributes per path
 
 SYNOPSIS
 
diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index db5d47eb19..4d63def206 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt
@@ -3,7 +3,7 @@ gitmodules(5)
 
 NAME
 
-gitmodules - defining submodule properties
+gitmodules - Defining submodule properties
 
 SYNOPSIS
 
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 27dec5b91d..1f6cceaefb 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -3,7 +3,7 @@ gitrevisions(7)
 
 NAME
 
-gitrevisions - specifying revisions and ranges for Git
+gitrevisions - Specifying revisions and ranges for Git
 
 SYNOPSIS
 
diff --git a/Makefile b/Makefile
index a60a78ee67..1efb751e46 100644
--- a/Makefile
+++ b/Makefile
@@ -1937,7 +1937,7 @@ $(BUILT_INS): git$X
 
 command-list.h: generate-cmdlist.sh command-list.txt
 
-command-list.h: $(wildcard Documentation/git-*.txt)
+command-list.h: $(wildcard Documentation/git*.txt)
$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ 
&& mv $@+ $@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
diff --git a/builtin/help.c b/builtin/help.c
index 0e0af8426a..5727fb5e51 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -402,38 +402,6 @@ static void show_html_page(const char *git_cmd)
open_html(page_path.buf);
 }
 
-static struct {
-   const char *name;
-   const char *help;
-} common_guides[] = {
-   { "attributes", N_("Defining attributes per path") },
-   { "everyday", N_("Everyday Git With 20 Commands Or So") },
-   { "glossary", N_("A Git glossary") },
-   { "ignore", N_("Specifies intentionally untracked files to ignore") },
-   { "modules", N_("Defining submodule properties") },
-   { "revisions", N_("Specifying revisions and ranges for Git") },
-   { "tutorial", N_("A tutorial introduction to Git (for version 1.5.1 or 
newer)") },
-   { "workflows", N_("An overview of recommended workflows with Git") },
-};
-
-static void list_common_guides_help(void)
-{
-   int i, longest = 0;
-
-   for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
-   if (longest < strlen(common_guides[i].name))
-   longest = strlen(common_guides[i].name);
-   }
-
-   puts(_("The common Git guides are:\n"));
-   for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
-   printf("   %s   ", common_guides[i].name);
-   mput_char(' ', longest - strlen(common_guides[i].name));
-   puts(_(common_guides[i].help));
-   }
-   putchar('\n');
-}
-
 static const char *check_git_cmd(const char* cmd)
 {
char *alias;
diff --git a/command-list.txt b/command-list.txt
index 3bd23201a6..99ddc231c1 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -139,3 +139,19 @@ gitweb  
ancillaryinterrogators
 git-whatchanged ancillaryinterrogators
 git-worktreemainporcelain
 git-write-tree  plumbingmanipulators
+gitattributes   guide
+gitcli  guide
+gitcore-tutorialguide
+gitcvs-migrationguide
+gitdiffcore guide
+giteveryday guide
+gitglossary guide
+githooksguide
+gitignore   guide
+gitmodules  guide
+gitnamespaces   guide
+gitrepository-layoutguide
+gitrevisions

[PATCH v2 16/17] completion: add and use --list-cmds=alias

[Nguyễn Thái Ngọc Duy]

By providing aliases via --list-cmds=, we could simplify command
collection code in the script. We only issue one git command. Before
this patch that is "git config", after it's "git --list-cmds=". In
"git help" completion case we actually reduce one "git" process (for
getting guides) but that call was added in this series so it does not
really count.

A couple of bash functions are removed because they are not needed
anymore. __git_compute_all_commands() and $__git_all_commands stay
because they are still needed for completing pager.* config and
without "alias" group, the result is still cacheable.

There is a slight (good) change in _git_help() with this patch: before
"git help " shows external commands (as in _not_ part of git) as
well as part of $__git_all_commands. We have finer control over
command listing now and can exclude that because we can't provide a
man page for external commands anyway.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git.txt  |  2 +-
 alias.c| 21 +++-
 alias.h|  3 ++
 contrib/completion/git-completion.bash | 75 ++
 git.c  |  2 +
 t/t9902-completion.sh  | 18 ---
 6 files changed, 40 insertions(+), 81 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 820ab77fcb..75f50d2379 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -170,7 +170,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
parse-options), main (all commands in libexec directory),
others (all other commands in `$PATH` that have git- prefix),
list- (see categories in command-list.txt),
-   nohelpers (exclude helper commands).
+   nohelpers (exclude helper commands) and alias.
 
 GIT COMMANDS
 
diff --git a/alias.c b/alias.c
index e9726ce8c5..a7e4e57130 100644
--- a/alias.c
+++ b/alias.c
@@ -1,10 +1,12 @@
 #include "cache.h"
 #include "alias.h"
 #include "config.h"
+#include "string-list.h"
 
 struct config_alias_data {
const char *alias;
char *v;
+   struct string_list *list;
 };
 
 static int config_alias_cb(const char *key, const char *value, void *d)
@@ -12,8 +14,16 @@ static int config_alias_cb(const char *key, const char 
*value, void *d)
struct config_alias_data *data = d;
const char *p;
 
-   if (skip_prefix(key, "alias.", ) && !strcasecmp(p, data->alias))
-   return git_config_string((const char **)>v, key, value);
+   if (!skip_prefix(key, "alias.", ))
+   return 0;
+
+   if (data->alias) {
+   if (!strcasecmp(p, data->alias))
+   return git_config_string((const char **)>v,
+key, value);
+   } else if (data->list) {
+   string_list_append(data->list, p);
+   }
 
return 0;
 }
@@ -27,6 +37,13 @@ char *alias_lookup(const char *alias)
return data.v;
 }
 
+void list_aliases(struct string_list *list)
+{
+   struct config_alias_data data = { NULL, NULL, list };
+
+   read_early_config(config_alias_cb, );
+}
+
 #define SPLIT_CMDLINE_BAD_ENDING 1
 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
 static const char *split_cmdline_errors[] = {
diff --git a/alias.h b/alias.h
index fbf1d22a94..79933f2457 100644
--- a/alias.h
+++ b/alias.h
@@ -1,9 +1,12 @@
 #ifndef __ALIAS_H__
 #define __ALIAS_H__
 
+struct string_list;
+
 char *alias_lookup(const char *alias);
 int split_cmdline(char *cmdline, const char ***argv);
 /* Takes a negative value returned by split_cmdline */
 const char *split_cmdline_strerror(int cmdline_errno);
+void list_aliases(struct string_list *list);
 
 #endif
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 217c8a3d3b..98f278fb9a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -834,51 +834,11 @@ __git_complete_strategy ()
return 1
 }
 
-# __git_commands requires 1 argument:
-# 1: the command group, either "all" or "porcelain"
-__git_commands () {
-   case "$1" in
-   porcelain)
-   if test -n "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
-   then
-   printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
-   else
-   git 
--list-cmds=list-mainporcelain,others,nohelpers,list-complete
-   fi
-   ;;
-   all)
-   if test -n "$GIT_TESTING_ALL_COMMAND_LIST"
-   then
-   printf "%s" "$GIT_TESTING_ALL_COMMAND_LIST"
-   else
-   git --list-cmds=main,others,nohelpers
-   fi
-   ;;
-   esac
-}
-
-__git_list_all_commands ()
-{
-   __git_commands all
-}
-
 __git_all_commands=
 __git_compute_all_commands ()
 {
test -n 

[PATCH v2 04/17] Remove common-cmds.h

[Nguyễn Thái Ngọc Duy]

After the last patch, common-cmds.h is no longer used (and it was
actually broken). Remove all related code. command-list.h will take
its place from now on.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .gitignore  |  1 -
 Makefile| 17 ++---
 generate-cmdlist.sh | 46 +++--
 3 files changed, 9 insertions(+), 55 deletions(-)

diff --git a/.gitignore b/.gitignore
index d4c3914167..0836083992 100644
--- a/.gitignore
+++ b/.gitignore
@@ -179,7 +179,6 @@
 /gitweb/gitweb.cgi
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
-/common-cmds.h
 /command-list.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index 5c58b0b692..a60a78ee67 100644
--- a/Makefile
+++ b/Makefile
@@ -757,7 +757,7 @@ LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
-GENERATED_H += common-cmds.h command-list.h
+GENERATED_H += command-list.h
 
 LIB_H = $(shell $(FIND) . \
-name .git -prune -o \
@@ -1914,9 +1914,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS)
 
-help.sp help.s help.o: common-cmds.h command-list.h
+help.sp help.s help.o: command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h command-list.h 
GIT-PREFIX
+builtin/help.sp builtin/help.s builtin/help.o: command-list.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
@@ -1935,11 +1935,6 @@ $(BUILT_INS): git$X
ln -s $< $@ 2>/dev/null || \
cp $< $@
 
-common-cmds.h: generate-cmdlist.sh command-list.txt
-
-common-cmds.h: $(wildcard Documentation/git-*.txt)
-   $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt COMMON 
>$@+ && mv $@+ $@
-
 command-list.h: generate-cmdlist.sh command-list.txt
 
 command-list.h: $(wildcard Documentation/git-*.txt)
@@ -2153,7 +2148,7 @@ else
 # Dependencies on header files, for platforms that do not support
 # the gcc -MMD option.
 #
-# Dependencies on automatically generated headers such as common-cmds.h or 
command-list.h
+# Dependencies on automatically generated headers such as command-list.h
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
@@ -2532,7 +2527,7 @@ sparse: $(SP_OBJ)
 style:
git clang-format --style file --diff --extensions c,h
 
-check: common-cmds.h command-list.h
+check: command-list.h
@if sparse; \
then \
echo >&2 "Use 'make sparse' instead"; \
@@ -2780,7 +2775,7 @@ clean: profile-clean coverage-clean
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
$(RM) -r bin-wrappers $(dep_dirs)
$(RM) -r po/build/
-   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h command-list.h 
$(ETAGS_TARGET) tags cscope*
+   $(RM) *.pyc *.pyo */*.pyc */*.pyo command-list.h $(ETAGS_TARGET) tags 
cscope*
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 9eb22c4ef1..3bcc1ee57d 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -68,46 +68,6 @@ struct cmdname_help {
uint32_t category;
 };
 "
-if test -z "$2"
-then
-   define_categories "$1"
-   echo
-   print_command_list "$1"
-   exit 0
-fi
-
-echo "static const char *common_cmd_groups[] = {"
-
-grps=grps$$.tmp
-match=match$$.tmp
-trap "rm -f '$grps' '$match'" 0 1 2 3 15
-
-sed -n '
-   1,/^### common groups/b
-   /^### command list/q
-   /^#/b
-   /^[ ]*$/b
-   h;s/^[^ ][^ ]*[ ][  ]*\(.*\)/   N_("\1"),/p
-   g;s/^\([^   ][^ ]*\)[   ].*/\1/w '$grps'
-   ' "$1"
-printf '};\n\n'
-
-n=0
-substnum=
-while read grp
-do
-   echo "^git-..*[ ]$grp"
-   substnum="$substnum${substnum:+;}s/[]$grp/$n/"
-   n=$(($n+1))
-done <"$grps" >"$match"
-
-printf 'static struct cmdname_help common_cmds[] = {\n'
-grep -f "$match" "$1" |
-sed 's/^git-//' |
-sort |
-while read cmd tags
-do
-   tag=$(echo "$tags" | sed "$substnum; s/[^0-9]//g")
-   echo "  {\"$cmd\", $(get_synopsis git-$cmd), $tag},"
-done
-echo "};"
+define_categories "$1"
+echo
+print_command_list "$1"
-- 
2.17.0.705.g3525833791

[PATCH v2 08/17] git: support --list-cmds=list-

[Nguyễn Thái Ngọc Duy]

This allows us to select any group of commands by a category defined
in command-list.txt. This is an internal/hidden option so we don't
have to be picky about the category name or worried about exposing too
much.

This will be used later by git-completion.bash to retrieve certain
command groups.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git.txt |  3 ++-
 generate-cmdlist.sh   | 17 +
 git.c |  7 +++
 help.c| 23 +++
 help.h|  2 ++
 5 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index c01477ab5e..c423810eac 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -168,7 +168,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
option and may change or be removed in the future. Supported
groups are: builtins, parseopt (builtin commands that use
parse-options), main (all commands in libexec directory),
-   others (all other commands in `$PATH` that have git- prefix).
+   others (all other commands in `$PATH` that have git- prefix),
+   list- (see categories in command-list.txt)
 
 GIT COMMANDS
 
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 3bcc1ee57d..8d6d8b45ce 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -45,6 +45,21 @@ define_categories () {
test "$bit" -gt 32 && die "Urgh.. too many categories?"
 }
 
+define_category_names () {
+   echo
+   echo "/* Category names */"
+   echo "static const char *category_names[] = {"
+   bit=0
+   category_list "$1" |
+   while read cat
+   do
+   echo "  \"$cat\", /* (1UL << $bit) */"
+   bit=$(($bit+1))
+   done
+   echo "  NULL"
+   echo "};"
+}
+
 print_command_list () {
echo "static struct cmdname_help command_list[] = {"
 
@@ -70,4 +85,6 @@ struct cmdname_help {
 "
 define_categories "$1"
 echo
+define_category_names "$1"
+echo
 print_command_list "$1"
diff --git a/git.c b/git.c
index 10907f7266..4d5b8a9931 100644
--- a/git.c
+++ b/git.c
@@ -60,6 +60,13 @@ static int list_cmds(const char *spec)
list_all_main_cmds();
else if (match_token(spec, len, "others"))
list_all_other_cmds();
+   else if (len > 5 && !strncmp(spec, "list-", 5)) {
+   struct strbuf sb = STRBUF_INIT;
+
+   strbuf_add(, spec + 5, len - 5);
+   list_cmds_by_category(, sb.buf);
+   strbuf_release();
+   }
else
die(_("unsupported command listing type '%s'"), spec);
spec += len;
diff --git a/help.c b/help.c
index d5ce9dfcbb..1117f7d1d1 100644
--- a/help.c
+++ b/help.c
@@ -329,6 +329,29 @@ void list_all_other_cmds(struct string_list *list)
clean_cmdnames(_cmds);
 }
 
+void list_cmds_by_category(struct string_list *list,
+  const char *cat)
+{
+   int i, n = ARRAY_SIZE(command_list);
+   uint32_t cat_id = 0;
+
+   for (i = 0; category_names[i]; i++) {
+   if (!strcmp(cat, category_names[i])) {
+   cat_id = 1UL << i;
+   break;
+   }
+   }
+   if (!cat_id)
+   die(_("unsupported command listing type '%s'"), cat);
+
+   for (i = 0; i < n; i++) {
+   struct cmdname_help *cmd = command_list + i;
+
+   if (cmd->category & cat_id)
+   string_list_append(list, drop_prefix(cmd->name));
+   }
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index 97e6c0965e..734bba32d3 100644
--- a/help.h
+++ b/help.h
@@ -21,6 +21,8 @@ static inline void mput_char(char c, unsigned int num)
 extern void list_common_cmds_help(void);
 extern void list_all_main_cmds(struct string_list *list);
 extern void list_all_other_cmds(struct string_list *list);
+extern void list_cmds_by_category(struct string_list *list,
+ const char *category);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
  struct cmdnames *main_cmds,
-- 
2.17.0.705.g3525833791

[PATCH v2 06/17] git --list-cmds: collect command list in a string_list

[Nguyễn Thái Ngọc Duy]

Instead of printing the command directly one by one, keep them in a
list and print at the end. This allows more modification before we
print out (e.g. sorting, removing duplicates or even excluding some
items).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 git.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/git.c b/git.c
index cd85355d81..376a59b97f 100644
--- a/git.c
+++ b/git.c
@@ -36,7 +36,7 @@ const char git_more_info_string[] =
 
 static int use_pager = -1;
 
-static void list_builtins(unsigned int exclude_option, char sep);
+static void list_builtins(struct string_list *list, unsigned int 
exclude_option);
 
 static int match_token(const char *spec, int len, const char *token)
 {
@@ -47,18 +47,24 @@ static int match_token(const char *spec, int len, const 
char *token)
 
 static int list_cmds(const char *spec)
 {
+   struct string_list list = STRING_LIST_INIT_DUP;
+   int i;
+
while (*spec) {
const char *sep = strchrnul(spec, ',');
int len = sep - spec;
 
if (match_token(spec, len, "builtins"))
-   list_builtins(0, '\n');
+   list_builtins(, 0);
else
die(_("unsupported command listing type '%s'"), spec);
spec += len;
if (*spec == ',')
spec++;
}
+   for (i = 0; i < list.nr; i++)
+   puts(list.items[i].string);
+   string_list_clear(, 0);
return 0;
 }
 
@@ -249,7 +255,13 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
(*argc)--;
} else if (skip_prefix(cmd, "--list-cmds=", )) {
if (!strcmp(cmd, "parseopt")) {
-   list_builtins(NO_PARSEOPT, ' ');
+   struct string_list list = STRING_LIST_INIT_DUP;
+   int i;
+
+   list_builtins(, NO_PARSEOPT);
+   for (i = 0; i < list.nr; i++)
+   printf("%s ", list.items[i].string);
+   string_list_clear(, 0);
exit(0);
} else {
exit(list_cmds(cmd));
@@ -533,14 +545,14 @@ int is_builtin(const char *s)
return !!get_builtin(s);
 }
 
-static void list_builtins(unsigned int exclude_option, char sep)
+static void list_builtins(struct string_list *out, unsigned int exclude_option)
 {
int i;
for (i = 0; i < ARRAY_SIZE(commands); i++) {
if (exclude_option &&
(commands[i].option & exclude_option))
continue;
-   printf("%s%c", commands[i].cmd, sep);
+   string_list_append(out, commands[i].cmd);
}
 }
 
-- 
2.17.0.705.g3525833791

[PATCH v2 02/17] generate-cmds.sh: export all commands to command-list.h

[Nguyễn Thái Ngọc Duy]

The current generate-cmds.sh generates just enough to print "git help"
output. That is, it only extracts help text for common commands.

The script is now updated to extract help text for all commands and
keep command classification a new file, command-list.h. This will be
useful later:

- "git help -a" could print a short summary of all commands instead of
  just the common ones.

- "git" could produce a list of commands of one or more category. One
  of its use is to reduce another command classification embedded in
  git-completion.bash.

The new file can be generated but is not used anywhere yet. The plan
is we migrate away from common-cmds.h. Then we can kill off
common-cmds.h build rules and generation code (and also delete
duplicate content in command-list.h which we keep for now to not mess
generate-cmds.sh up too much).

PS. The new fixed column requirement on command-list.txt is
technically not needed. But it helps simplify the code a bit at this
stage. We could lift this restriction later if we want to.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .gitignore  |  1 +
 Makefile| 13 ++---
 command-list.txt|  4 +--
 generate-cmdlist.sh | 67 ++---
 4 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..d4c3914167 100644
--- a/.gitignore
+++ b/.gitignore
@@ -180,6 +180,7 @@
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
 /common-cmds.h
+/command-list.h
 *.tar.gz
 *.dsc
 *.deb
diff --git a/Makefile b/Makefile
index f181687250..2a8913ea21 100644
--- a/Makefile
+++ b/Makefile
@@ -757,7 +757,7 @@ LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
-GENERATED_H += common-cmds.h
+GENERATED_H += common-cmds.h command-list.h
 
 LIB_H = $(shell $(FIND) . \
-name .git -prune -o \
@@ -1938,6 +1938,11 @@ $(BUILT_INS): git$X
 common-cmds.h: generate-cmdlist.sh command-list.txt
 
 common-cmds.h: $(wildcard Documentation/git-*.txt)
+   $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt COMMON 
>$@+ && mv $@+ $@
+
+command-list.h: generate-cmdlist.sh command-list.txt
+
+command-list.h: $(wildcard Documentation/git-*.txt)
$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ 
&& mv $@+ $@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
@@ -2148,7 +2153,7 @@ else
 # Dependencies on header files, for platforms that do not support
 # the gcc -MMD option.
 #
-# Dependencies on automatically generated headers such as common-cmds.h
+# Dependencies on automatically generated headers such as common-cmds.h or 
command-list.h
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
@@ -2527,7 +2532,7 @@ sparse: $(SP_OBJ)
 style:
git clang-format --style file --diff --extensions c,h
 
-check: common-cmds.h
+check: common-cmds.h command-list.h
@if sparse; \
then \
echo >&2 "Use 'make sparse' instead"; \
@@ -2775,7 +2780,7 @@ clean: profile-clean coverage-clean
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
$(RM) -r bin-wrappers $(dep_dirs)
$(RM) -r po/build/
-   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags 
cscope*
+   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h command-list.h 
$(ETAGS_TARGET) tags cscope*
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
diff --git a/command-list.txt b/command-list.txt
index a1fad28fd8..786536aba0 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -8,8 +8,8 @@ info examine the history and state (see also: git help 
revisions)
 history  grow, mark and tweak your common history
 remote   collaborate (see also: git help workflows)
 
-### command list (do not change this line)
-# command name  category [deprecated] [common]
+### command list (do not change this line, also do not change alignment)
+# command name  category [category] [category]
 git-add mainporcelain   worktree
 git-am  mainporcelain
 git-annotateancillaryinterrogators
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 31b6d886cb..870d3b626a 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -1,5 +1,27 @@
 #!/bin/sh
 
+die () {
+   echo "$@" >&2
+   exit 1
+}
+
+command_list () {
+   sed '1,/^### command list/d;/^#/d' "$1"
+}
+
+get_categories () {
+   tr ' ' '\n'|
+   grep -v '^$' |
+   sort |
+   uniq
+}
+
+category_list () {
+   command_list "$1" |
+   cut -c 40- |
+   get_categories
+}
+
 get_synopsis () {
sed -n '
/^NAME/,/'"$1"'/H
@@ -10,14 +32,51 @@ get_synopsis () {
}' 

[PATCH v2 07/17] completion: implement and use --list-cmds=main,others

[Nguyễn Thái Ngọc Duy]

This is part of the effort to break down and provide commands by
category in machine-readable form. This could be helpful later on when
completion script switches to use --list-cmds for selecting
completable commands. It would be much easier for the user to choose
to complete _all_ commands instead of the default selection by passing
different values to --list-cmds in git-completino.bash.

While at there, replace "git help -a" in git-completion.bash with
--list-cmds since it's better suited for this task.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git.txt  |  3 ++-
 contrib/completion/git-completion.bash |  2 +-
 git.c  |  4 
 help.c | 32 ++
 help.h |  4 
 5 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2800e3d188..c01477ab5e 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -167,7 +167,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
List commands by group. This is an internal/experimental
option and may change or be removed in the future. Supported
groups are: builtins, parseopt (builtin commands that use
-   parse-options).
+   parse-options), main (all commands in libexec directory),
+   others (all other commands in `$PATH` that have git- prefix).
 
 GIT COMMANDS
 
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 3556838759..62ca8641f4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -839,7 +839,7 @@ __git_commands () {
then
printf "%s" "${GIT_TESTING_COMMAND_COMPLETION}"
else
-   git help -a|egrep '^  [a-zA-Z0-9]'
+   git --list-cmds=main,others
fi
 }
 
diff --git a/git.c b/git.c
index 376a59b97f..10907f7266 100644
--- a/git.c
+++ b/git.c
@@ -56,6 +56,10 @@ static int list_cmds(const char *spec)
 
if (match_token(spec, len, "builtins"))
list_builtins(, 0);
+   else if (match_token(spec, len, "main"))
+   list_all_main_cmds();
+   else if (match_token(spec, len, "others"))
+   list_all_other_cmds();
else
die(_("unsupported command listing type '%s'"), spec);
spec += len;
diff --git a/help.c b/help.c
index 2d6a3157f8..d5ce9dfcbb 100644
--- a/help.c
+++ b/help.c
@@ -297,6 +297,38 @@ void list_common_cmds_help(void)
print_cmd_by_category(common_categories);
 }
 
+void list_all_main_cmds(struct string_list *list)
+{
+   struct cmdnames main_cmds, other_cmds;
+   int i;
+
+   memset(_cmds, 0, sizeof(main_cmds));
+   memset(_cmds, 0, sizeof(other_cmds));
+   load_command_list("git-", _cmds, _cmds);
+
+   for (i = 0; i < main_cmds.cnt; i++)
+   string_list_append(list, main_cmds.names[i]->name);
+
+   clean_cmdnames(_cmds);
+   clean_cmdnames(_cmds);
+}
+
+void list_all_other_cmds(struct string_list *list)
+{
+   struct cmdnames main_cmds, other_cmds;
+   int i;
+
+   memset(_cmds, 0, sizeof(main_cmds));
+   memset(_cmds, 0, sizeof(other_cmds));
+   load_command_list("git-", _cmds, _cmds);
+
+   for (i = 0; i < other_cmds.cnt; i++)
+   string_list_append(list, other_cmds.names[i]->name);
+
+   clean_cmdnames(_cmds);
+   clean_cmdnames(_cmds);
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index b21d7c94e8..97e6c0965e 100644
--- a/help.h
+++ b/help.h
@@ -1,6 +1,8 @@
 #ifndef HELP_H
 #define HELP_H
 
+struct string_list;
+
 struct cmdnames {
int alloc;
int cnt;
@@ -17,6 +19,8 @@ static inline void mput_char(char c, unsigned int num)
 }
 
 extern void list_common_cmds_help(void);
+extern void list_all_main_cmds(struct string_list *list);
+extern void list_all_other_cmds(struct string_list *list);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
  struct cmdnames *main_cmds,
-- 
2.17.0.705.g3525833791

[PATCH v2 03/17] help: use command-list.h for common command list

[Nguyễn Thái Ngọc Duy]

The previous commit added code generation for all_cmd_desc[] which
includes almost everything we need to generate common command list.
Convert help code to use that array instead and drop common_cmds[] array.

The description of each common command group is removed from
command-list.txt. This keeps this file format simpler. common-cmds.h
will not be generated correctly after this change due to the
command-list.txt format change. But it does not matter and
common-cmds.h will be removed.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Makefile|   4 +-
 command-list.txt|  10 ---
 generate-cmdlist.sh |   4 +-
 help.c  | 145 +---
 t/t0012-help.sh |   9 +++
 5 files changed, 122 insertions(+), 50 deletions(-)

diff --git a/Makefile b/Makefile
index 2a8913ea21..5c58b0b692 100644
--- a/Makefile
+++ b/Makefile
@@ -1914,9 +1914,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS)
 
-help.sp help.s help.o: common-cmds.h
+help.sp help.s help.o: common-cmds.h command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h command-list.h 
GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
diff --git a/command-list.txt b/command-list.txt
index 786536aba0..3bd23201a6 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -1,13 +1,3 @@
-# common commands are grouped by themes
-# these groups are output by 'git help' in the order declared here.
-# map each common command in the command list to one of these groups.
-### common groups (do not change this line)
-init start a working area (see also: git help tutorial)
-worktree work on the current change (see also: git help everyday)
-info examine the history and state (see also: git help revisions)
-history  grow, mark and tweak your common history
-remote   collaborate (see also: git help workflows)
-
 ### command list (do not change this line, also do not change alignment)
 # command name  category [category] [category]
 git-add mainporcelain   worktree
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 870d3b626a..9eb22c4ef1 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -6,7 +6,7 @@ die () {
 }
 
 command_list () {
-   sed '1,/^### command list/d;/^#/d' "$1"
+   grep -v '^#' "$1"
 }
 
 get_categories () {
@@ -65,7 +65,7 @@ echo "/* Automatically generated by generate-cmdlist.sh */
 struct cmdname_help {
const char *name;
const char *help;
-   uint32_t group;
+   uint32_t category;
 };
 "
 if test -z "$2"
diff --git a/help.c b/help.c
index 60071a9bea..2d6a3157f8 100644
--- a/help.c
+++ b/help.c
@@ -5,13 +5,114 @@
 #include "run-command.h"
 #include "levenshtein.h"
 #include "help.h"
-#include "common-cmds.h"
+#include "command-list.h"
 #include "string-list.h"
 #include "column.h"
 #include "version.h"
 #include "refs.h"
 #include "parse-options.h"
 
+struct category_description {
+   uint32_t category;
+   const char *desc;
+};
+static uint32_t common_mask =
+   CAT_init | CAT_worktree | CAT_info |
+   CAT_history | CAT_remote;
+static struct category_description common_categories[] = {
+   { CAT_init, N_("start a working area (see also: git help tutorial)") },
+   { CAT_worktree, N_("work on the current change (see also: git help 
everyday)") },
+   { CAT_info, N_("examine the history and state (see also: git help 
revisions)") },
+   { CAT_history, N_("grow, mark and tweak your common history") },
+   { CAT_remote, N_("collaborate (see also: git help workflows)") },
+   { 0, NULL }
+};
+
+static const char *drop_prefix(const char *name)
+{
+   const char *new_name;
+
+   if (skip_prefix(name, "git-", _name))
+   return new_name;
+   return name;
+
+}
+
+static void extract_cmds(struct cmdname_help **p_cmds, uint32_t mask)
+{
+   int i, nr = 0;
+   struct cmdname_help *cmds;
+
+   if (ARRAY_SIZE(command_list) == 0)
+   BUG("empty command_list[] is a sign of broken 
generate-cmdlist.sh");
+
+   ALLOC_ARRAY(cmds, ARRAY_SIZE(command_list) + 1);
+
+   for (i = 0; i < ARRAY_SIZE(command_list); i++) {
+   const struct cmdname_help *cmd = command_list + i;
+
+   if (!(cmd->category & mask))
+   continue;
+
+   cmds[nr] = *cmd;
+   cmds[nr].name = drop_prefix(cmd->name);
+
+   nr++;
+   }
+   cmds[nr].name = NULL;
+   *p_cmds = cmds;
+}
+
+static void print_command_list(const struct cmdname_help *cmds,
+  uint32_t 

[PATCH v2 13/17] completion: reduce completable command list

[Nguyễn Thái Ngọc Duy]

The following commands are removed from the complete list:

- annotate obsolete, discouraged to use
- filter-branchnot often used
- get-tar-commit-idnot often used
- imap-sendnot often used
- interpreter-trailers not for interactive use
- name-rev plumbing, just use git-describe
- p4   too short and probably not often used (*)
- svn  same category as p4 (*)
- verify-commitnot often used

(*) to be fair, send-email command which is in the same foreignscminterface
group as svn and p4 does get completion, just because it's used by git
and kernel development. So maybe we should include them.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 command-list.txt | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/command-list.txt b/command-list.txt
index dcf1907a54..e505a1e34c 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -47,7 +47,7 @@
 # command name  category [category] [category]
 git-add mainporcelain   worktree
 git-am  mainporcelain
-git-annotateancillaryinterrogators  
complete
+git-annotateancillaryinterrogators
 git-apply   plumbingmanipulators
complete
 git-archimport  foreignscminterface
 git-archive mainporcelain
@@ -89,13 +89,13 @@ git-fast-export 
ancillarymanipulators
 git-fast-import ancillarymanipulators
 git-fetch   mainporcelain   remote
 git-fetch-pack  synchingrepositories
-git-filter-branch   ancillarymanipulators   
complete
+git-filter-branch   ancillarymanipulators
 git-fmt-merge-msg   purehelpers
 git-for-each-refplumbinginterrogators
 git-format-patchmainporcelain
 git-fsckancillaryinterrogators  
complete
 git-gc  mainporcelain
-git-get-tar-commit-id   ancillaryinterrogators  
complete
+git-get-tar-commit-id   ancillaryinterrogators
 git-grepmainporcelain   info
 git-gui mainporcelain
 git-hash-object plumbingmanipulators
@@ -103,11 +103,11 @@ git-help
ancillaryinterrogators  complete
 git-http-backendsynchingrepositories
 git-http-fetch  synchelpers
 git-http-push   synchelpers
-git-imap-send   foreignscminterface 
complete
+git-imap-send   foreignscminterface
 git-index-pack  plumbingmanipulators
 git-initmainporcelain   init
 git-instawebancillaryinterrogators  
complete
-git-interpret-trailers  purehelpers 
complete
+git-interpret-trailers  purehelpers
 gitkmainporcelain
 git-log mainporcelain   info
 git-ls-filesplumbinginterrogators
@@ -125,9 +125,9 @@ git-merge-tree  
ancillaryinterrogators
 git-mktag   plumbingmanipulators
 git-mktree  plumbingmanipulators
 git-mv  mainporcelain   worktree
-git-name-revplumbinginterrogators   
complete
+git-name-revplumbinginterrogators
 git-notes   mainporcelain
-git-p4  foreignscminterface 
complete
+git-p4  foreignscminterface
 git-pack-objectsplumbingmanipulators
 git-pack-redundant  plumbinginterrogators
 git-pack-refs   ancillarymanipulators
@@ -167,7 +167,7 @@ git-stage   
complete
 git-status  mainporcelain   info
 git-stripspace  purehelpers
 git-submodule   mainporcelain
-git-svn foreignscminterface 
complete
+git-svn foreignscminterface
 git-symbolic-refplumbingmanipulators
 git-tag mainporcelain   history
 git-unpack-file plumbinginterrogators
@@ 

[PATCH v2 01/17] generate-cmds.sh: factor out synopsis extract code

[Nguyễn Thái Ngọc Duy]

This makes it easier to reuse the same code in another place (very
soon).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 generate-cmdlist.sh | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index eeea4b67ea..31b6d886cb 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -1,5 +1,15 @@
 #!/bin/sh
 
+get_synopsis () {
+   sed -n '
+   /^NAME/,/'"$1"'/H
+   ${
+   x
+   s/.*'"$1"' - \(.*\)/N_("\1")/
+   p
+   }' "Documentation/$1.txt"
+}
+
 echo "/* Automatically generated by generate-cmdlist.sh */
 struct cmdname_help {
char name[16];
@@ -39,12 +49,6 @@ sort |
 while read cmd tags
 do
tag=$(echo "$tags" | sed "$substnum; s/[^0-9]//g")
-   sed -n '
-   /^NAME/,/git-'"$cmd"'/H
-   ${
-   x
-   s/.*git-'"$cmd"' - \(.*\)/  {"'"$cmd"'", N_("\1"), 
'$tag'},/
-   p
-   }' "Documentation/git-$cmd.txt"
+   echo "  {\"$cmd\", $(get_synopsis git-$cmd), $tag},"
 done
 echo "};"
-- 
2.17.0.705.g3525833791

[PATCH v2 14/17] Move declaration for alias.c to alias.h

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 alias.c | 1 +
 alias.h | 9 +
 builtin/help.c  | 1 +
 builtin/merge.c | 1 +
 cache.h | 5 -
 connect.c   | 1 +
 git.c   | 1 +
 pager.c | 1 +
 sequencer.c | 1 +
 shell.c | 1 +
 10 files changed, 17 insertions(+), 5 deletions(-)
 create mode 100644 alias.h

diff --git a/alias.c b/alias.c
index bf146e5263..e9726ce8c5 100644
--- a/alias.c
+++ b/alias.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "alias.h"
 #include "config.h"
 
 struct config_alias_data {
diff --git a/alias.h b/alias.h
new file mode 100644
index 00..fbf1d22a94
--- /dev/null
+++ b/alias.h
@@ -0,0 +1,9 @@
+#ifndef __ALIAS_H__
+#define __ALIAS_H__
+
+char *alias_lookup(const char *alias);
+int split_cmdline(char *cmdline, const char ***argv);
+/* Takes a negative value returned by split_cmdline */
+const char *split_cmdline_strerror(int cmdline_errno);
+
+#endif
diff --git a/builtin/help.c b/builtin/help.c
index 5727fb5e51..6b4b3df90d 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -9,6 +9,7 @@
 #include "run-command.h"
 #include "column.h"
 #include "help.h"
+#include "alias.h"
 
 #ifndef DEFAULT_HELP_FORMAT
 #define DEFAULT_HELP_FORMAT "man"
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..e3681cd850 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -34,6 +34,7 @@
 #include "string-list.h"
 #include "packfile.h"
 #include "tag.h"
+#include "alias.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
diff --git a/cache.h b/cache.h
index bbaf5c349a..16ea13 100644
--- a/cache.h
+++ b/cache.h
@@ -1835,11 +1835,6 @@ extern int ws_blank_line(const char *line, int len, 
unsigned ws_rule);
 void overlay_tree_on_index(struct index_state *istate,
   const char *tree_name, const char *prefix);
 
-char *alias_lookup(const char *alias);
-int split_cmdline(char *cmdline, const char ***argv);
-/* Takes a negative value returned by split_cmdline */
-const char *split_cmdline_strerror(int cmdline_errno);
-
 /* setup.c */
 struct startup_info {
int have_repository;
diff --git a/connect.c b/connect.c
index c3a014c5ba..ff078d28dc 100644
--- a/connect.c
+++ b/connect.c
@@ -13,6 +13,7 @@
 #include "transport.h"
 #include "strbuf.h"
 #include "protocol.h"
+#include "alias.h"
 
 static char *server_capabilities;
 static const char *parse_feature_value(const char *, const char *, int *);
diff --git a/git.c b/git.c
index 4d5b8a9931..19f73b3fa3 100644
--- a/git.c
+++ b/git.c
@@ -3,6 +3,7 @@
 #include "exec_cmd.h"
 #include "help.h"
 #include "run-command.h"
+#include "alias.h"
 
 #define RUN_SETUP  (1<<0)
 #define RUN_SETUP_GENTLY   (1<<1)
diff --git a/pager.c b/pager.c
index 92b23e6cd1..1f4688fa03 100644
--- a/pager.c
+++ b/pager.c
@@ -2,6 +2,7 @@
 #include "config.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "alias.h"
 
 #ifndef DEFAULT_PAGER
 #define DEFAULT_PAGER "less"
diff --git a/sequencer.c b/sequencer.c
index 667f35ebdf..1288a36ebd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -23,6 +23,7 @@
 #include "hashmap.h"
 #include "notes-utils.h"
 #include "sigchain.h"
+#include "alias.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
diff --git a/shell.c b/shell.c
index 234b2d4f16..3ce77b8e34 100644
--- a/shell.c
+++ b/shell.c
@@ -3,6 +3,7 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 #include "run-command.h"
+#include "alias.h"
 
 #define COMMAND_DIR "git-shell-commands"
 #define HELP_COMMAND COMMAND_DIR "/help"
-- 
2.17.0.705.g3525833791

[PATCH v2 00/17] nd/command-list updates

[Nguyễn Thái Ngọc Duy]

It turns out this series is not done as I thought it was :)

v2 makes it possible to write

gitcomp "$(git --list-cmds=...)"

which is really nice and very close to what gitcomp_builtin does.
Other changes are

- support --list-cmds=alias and --list-cmds=nohelpers so that we could
  do the above
- a bit more document about --list-cmds
- $GIT_COMPLETION_CMD_GROUPS is dropped. I think it just causes more
  confusion and complete.commands should be enough in most cases
- name-rev is no longer compleable
- "git help " does not show truly external commands anymore
  (those that are in $PATH but not in libexec and very unlikely to
  have a man page)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 91f7eaed7b..9e81dcf867 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1345,13 +1345,11 @@ credentialCache.ignoreSIGHUP::
 
 completion.commands::
This is only used by git-completion.bash to add or remove
-   commands from the complete list. Normally only porcelain
-   commands and a few select others are in the complete list. You
+   commands from the list of completed commands. Normally only
+   porcelain commands and a few select others are completed. You
can add more commands, separated by space, in this
variable. Prefixing the command with '-' will remove it from
the existing list.
-+
-This variable should not be per-repository.
 
 include::diff-config.txt[]
 
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..6f7eddf847 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -163,6 +163,16 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
Do not perform optional operations that require locks. This is
equivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.
 
+--list-cmds=group[,group...]::
+   List commands by group. This is an internal/experimental
+   option and may change or be removed in the future. Supported
+   groups are: builtins, parseopt (builtin commands that use
+   parse-options), main (all commands in libexec directory),
+   others (all other commands in `$PATH` that have git- prefix),
+   list- (see categories in command-list.txt),
+   nohelpers (exclude helper commands), alias and config
+   (retrieve command list from config variable completion.commands)
+
 GIT COMMANDS
 
 
diff --git a/alias.c b/alias.c
index bf146e5263..a7e4e57130 100644
--- a/alias.c
+++ b/alias.c
@@ -1,9 +1,12 @@
 #include "cache.h"
+#include "alias.h"
 #include "config.h"
+#include "string-list.h"
 
 struct config_alias_data {
const char *alias;
char *v;
+   struct string_list *list;
 };
 
 static int config_alias_cb(const char *key, const char *value, void *d)
@@ -11,8 +14,16 @@ static int config_alias_cb(const char *key, const char 
*value, void *d)
struct config_alias_data *data = d;
const char *p;
 
-   if (skip_prefix(key, "alias.", ) && !strcasecmp(p, data->alias))
-   return git_config_string((const char **)>v, key, value);
+   if (!skip_prefix(key, "alias.", ))
+   return 0;
+
+   if (data->alias) {
+   if (!strcasecmp(p, data->alias))
+   return git_config_string((const char **)>v,
+key, value);
+   } else if (data->list) {
+   string_list_append(data->list, p);
+   }
 
return 0;
 }
@@ -26,6 +37,13 @@ char *alias_lookup(const char *alias)
return data.v;
 }
 
+void list_aliases(struct string_list *list)
+{
+   struct config_alias_data data = { NULL, NULL, list };
+
+   read_early_config(config_alias_cb, );
+}
+
 #define SPLIT_CMDLINE_BAD_ENDING 1
 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
 static const char *split_cmdline_errors[] = {
diff --git a/alias.h b/alias.h
new file mode 100644
index 00..79933f2457
--- /dev/null
+++ b/alias.h
@@ -0,0 +1,12 @@
+#ifndef __ALIAS_H__
+#define __ALIAS_H__
+
+struct string_list;
+
+char *alias_lookup(const char *alias);
+int split_cmdline(char *cmdline, const char ***argv);
+/* Takes a negative value returned by split_cmdline */
+const char *split_cmdline_strerror(int cmdline_errno);
+void list_aliases(struct string_list *list);
+
+#endif
diff --git a/builtin/help.c b/builtin/help.c
index 5727fb5e51..6b4b3df90d 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -9,6 +9,7 @@
 #include "run-command.h"
 #include "column.h"
 #include "help.h"
+#include "alias.h"
 
 #ifndef DEFAULT_HELP_FORMAT
 #define DEFAULT_HELP_FORMAT "man"
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..e3681cd850 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -34,6 +34,7 @@
 #include "string-list.h"
 #include "packfile.h"
 #include "tag.h"
+#include "alias.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
diff --git a/cache.h b/cache.h
index 

Re: [PATCH 07/11] rerere: use repo_read_index_or_die

[Thomas Gummerer]

On 05/16, Stefan Beller wrote:
> By switching to repo_read_index_or_die, we'll get a slightly different
> error message ("index file corrupt") as well as localization of it.

Apart from the slightly different error message, and the localization
(both of which I think are a good thing), I notice that this also
turns a "return error(...)" into a "die(...)".  I thought we were
going more towards not 'die'ing in libgit.a code, and letting the
caller handling the errors?  Either way I think this change should be
described in the commit message.

Also all other messages in 'rerere.c' are currently not translated.
I'm currently working on a series that includes a patch to do just
that (amongst some other changes to 'rerere'), which I'm hoping to
send soon-ish, but the textual conflicts should we still want this
patch should be easy to solve.

> Signed-off-by: Stefan Beller 
> ---
>  rerere.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/rerere.c b/rerere.c
> index 18cae2d11c9..5f35dd66f90 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -10,6 +10,7 @@
>  #include "attr.h"
>  #include "pathspec.h"
>  #include "sha1-lookup.h"
> +#include "repository.h"
>  
>  #define RESOLVED 0
>  #define PUNTED 1
> @@ -567,8 +568,7 @@ static int check_one_conflict(int i, int *type)
>  static int find_conflict(struct string_list *conflict)
>  {
>   int i;
> - if (read_cache() < 0)
> - return error("Could not read index");
> + repo_read_index_or_die(the_repository);
>  
>   for (i = 0; i < active_nr;) {
>   int conflict_type;
> @@ -600,8 +600,7 @@ int rerere_remaining(struct string_list *merge_rr)
>   int i;
>   if (setup_rerere(merge_rr, RERERE_READONLY))
>   return 0;
> - if (read_cache() < 0)
> - return error("Could not read index");
> + repo_read_index_or_die(the_repository);
>  
>   for (i = 0; i < active_nr;) {
>   int conflict_type;
> @@ -1103,8 +1102,7 @@ int rerere_forget(struct pathspec *pathspec)
>   struct string_list conflict = STRING_LIST_INIT_DUP;
>   struct string_list merge_rr = STRING_LIST_INIT_DUP;
>  
> - if (read_cache() < 0)
> - return error("Could not read index");
> + repo_read_index_or_die(the_repository);
>  
>   fd = setup_rerere(_rr, RERERE_NOAUTOUPDATE);
>   if (fd < 0)
> -- 
> 2.17.0.582.gccdcbd54c44.dirty
> 

Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers

[Jeff King]

On Sat, May 19, 2018 at 10:27:46AM +0200, René Scharfe wrote:

> Am 19.05.2018 um 03:57 schrieb Jeff King:
> > These formatted integers should always fit into their
> > 64-byte buffers. Let's use xsnprintf() to add an assertion
> > that this is the case, which makes auditing for other
> > unchecked snprintfs() easier.
> 
> How about this instead?
> 
> -- >8 --
> Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process

Yeah, I agree that is much nicer (I was so focused on the snprintfs I
didn't bother to look at the context for a better solution).

Thanks, getting a review in the form of a complete patch is my favorite
kind of review. :)

> Inspired-by: Jeff King 

Heh.

-Peff

Re: [PATCH 8/9] completion: support case-insensitive config vars just a bit

[SZEDER Gábor]

> config variable names are technically case-insensitive while this case
> construct is by default case-sensitive. Moving it to case-insensitive
> could be a lot more work.

Bash v4 supports the case modification expansion in the form of
${prev,,}.  Alas OSX (or older LTS/Enterprise Linux releases) ships
Bash v3.2, which doesn't yet support this syntax, and there is ZSH,
which doesn't understand it either (though I suspect it has its own
weird syntax for case modification).  Supporting them could look
something like this:

  local varname
  
  if [ "${BASH_VERSINFO[0]:-0}" -ge 4 ]; then
  varname="${prev,,}"
  else
  varname="$(echo "$prev" |tr A-Z a-z)"
  fi
  
  case "$varname" in
  <>

Not pretty, but I think the additional complexity and overhead is
acceptable.  Yeah, on some platforms/shells we would run an extra
command substitution and an external command, but I suppose on those
platforms the overhead of the extra processes is not that critical.
And where this additional overhead matters the most is Windows, but
luckily Git for Windows ships with Bash v4.


> Instead let's just accept the form that is
> more likely to show up in practice.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  contrib/completion/git-completion.bash | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 8d3257c4de..e7829b5b24 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2084,7 +2084,7 @@ __git_compute_config_vars ()
>  _git_config ()
>  {
>   case "$prev" in
> - branch.*.remote|branch.*.pushremote)
> + branch.*.remote|branch.*.push[Rr]emote)
>   __gitcomp_nl "$(__git_remotes)"
>   return
>   ;;
> @@ -2096,7 +2096,7 @@ _git_config ()
>   __gitcomp "false true preserve interactive"
>   return
>   ;;
> - remote.pushdefault)
> + remote.push[Dd]efault)
>   __gitcomp_nl "$(__git_remotes)"
>   return
>   ;;
> @@ -2162,7 +2162,7 @@ _git_config ()
>   __gitcomp "$__git_send_email_suppresscc_options"
>   return
>   ;;
> - sendemail.transferencoding)
> + sendemail.transfer[Ee]ncoding)
>   __gitcomp "7bit 8bit quoted-printable base64"
>   return
>   ;;

'git help config' shows 'color.showBranch' and
'sendemail.aliasesfiletype' in camelCase as well.

Re: [PATCH 7/9] completion: drop the hard coded list of config vars

[SZEDER Gábor]

> The new help option --config-for-completion is a machine friendlier
> version of --config where all the placeholders and wildcards are
> dropped, leaving only the good, completable prefixes for
> git-completion.bash to consume.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  builtin/help.c |  13 +-
>  contrib/completion/git-completion.bash | 334 +
>  help.c |  30 ++-
>  help.h |   2 +-
>  4 files changed, 47 insertions(+), 332 deletions(-)

Oh, this diffstat is fantastic! :)

I'm glad to see that enormous hardcoded list go.

Re: [PATCH 12/14] completion: let git provide the completable command list

[Duy Nguyen]

On Sun, May 20, 2018 at 3:20 PM, SZEDER Gábor  wrote:
> On Sat, May 19, 2018 at 6:27 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> Instead of maintaining a separate list of command classification,
>> which often could go out of date, let's centralize the information
>> back in git.
>>
>> While the function in git-completion.bash implies "list porcelain
>> commands", that's not exactly what it does. It gets all commands (aka
>> --list-cmds=main,others) then exclude certain non-porcelain ones. We
>> could almost recreate this list two lists list-mainporcelain and
>> others. The non-porcelain-but-included-anyway is added by the third
>> category list-complete.
>>
>> list-complete does not recreate exactly the command list before this
>> patch though. The following commands will disappear from the complete
>> list because they are not in command-list.txt and it's not worth
>> adding them back: lost-found, peek-remote and tar-tree.
>
> These commands have been removed long ago, see the topic leading to
> 577aed296a (Merge branch 'jk/remove-deprecated', 2013-12-12).  Perhaps
> you saw them only because they are still somewhere on your $PATH or
> $GIT_EXEC_PATH?

Right. Old commands remain in my libexec dir. Will clean up this commit message.
-- 
Duy

Re: [PATCH 14/14] completion: allow to customize the completable command list

[Duy Nguyen]

On Sun, May 20, 2018 at 4:27 PM, SZEDER Gábor  wrote:
> On Sat, May 19, 2018 at 6:27 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> By default we show porcelain, external commands and a couple others
>> that are also popular. If you are not happy with this list, you can
>> now customize it. See the big comment block for details.
>>
>> PS. perhaps I should make aliases a group too, which makes it possible
>> to _not_ complete aliases by omitting this special group in
>> $GIT_COMPLETION_CMD_GROUPS
>
> Note that the completion script reads the configured aliases each time
> the user attempts to complete commands.  So if the user adds or
> removes an alias, then it will automatically be taken into account the
> next time after 'git '.  By turning aliases into a group listed
> by 'git help' they would be cached like all other commands, so this
> would no longer be the case.

Maybe we can stop caching "git --list-cmds=..." then. We achieve the
same effect for completion.commands (changes take effect immediately)
and don't add any more overhead (still one git call per completion)?
This can also avoid the per-repository limitation below.

>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  Documentation/config.txt   | 10 
>>  contrib/completion/git-completion.bash | 28 +-
>>  git.c  |  2 ++
>>  help.c | 33 ++
>>  help.h |  1 +
>>  5 files changed, 73 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 2659153cb3..91f7eaed7b 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1343,6 +1343,16 @@ credential..*::
>>  credentialCache.ignoreSIGHUP::
>> Tell git-credential-cache--daemon to ignore SIGHUP, instead of 
>> quitting.
>>
>> +completion.commands::
>> +   This is only used by git-completion.bash to add or remove
>> +   commands from the complete list. Normally only porcelain
>
> s/complete list/list of completed commands/ perhaps?
>
>> +   commands and a few select others are in the complete list. You
>
> s/in the complete list/completed/
>
>> +   can add more commands, separated by space, in this
>> +   variable. Prefixing the command with '-' will remove it from
>> +   the existing list.
>> ++
>> +This variable should not be per-repository.
>
> I think this should also mention that changing the value of this
> config variable will not immediately affect the commands listed after
> 'git ', but the user will have to re-dot-source the completion
> script first.
>
> The way I understand the rest of the patch, this config variable
> doesn't have any effect if $GIT_COMPLETION_CMD_GROUPS doesn't contain
> "config".  If that is indeed the case, then that should be mentioned
> here as well.
>
> Having said that, I wonder whether we should really require "config"
> in $GIT_COMPLETION_CMD_GROUPS.  Isn't having 'completion.commands' set
> in the config a clear enough indication in itself that the user wants
> to customize the listed commands?

$GIT_COMPLETION_CMD_GROUPS is for really specific customization. I
initially added it because "config" was not default, and because
completion.commands could not exclude commands, only include them. Now
that is possible, perhaps just completion.commands (no
$GIT_COMPLETINO_CMD_GROUPS) would suffice for most cases?

>
>> +
>>  include::diff-config.txt[]
>>
>>  difftool..path::
>> diff --git a/contrib/completion/git-completion.bash 
>> b/contrib/completion/git-completion.bash
>> index cd1d8e553f..f237eb0ff4 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -38,6 +38,29 @@
>>  #
>>  # When set to "1", do not include "DWIM" suggestions in git-checkout
>>  # completion (e.g., completing "foo" when "origin/foo" exists).
>> +#
>> +#   GIT_COMPLETION_CMD_GROUPS
>> +#
>> +# When set, "git --list-cmds=$GIT_COMPLETION_CMD_GROUPS" will be
>> +# used to get the list of completable commands. The default is
>> +# "mainporcelain,others,list-complete" (in English: all porcelain
>
> Mental note #1: "mainporcelain"
>
>> +# commands and external ones are included. Certain non-porcelain
>> +# commands are also marked for completion in command-list.txt).
>> +# You could for example complete all commands with
>> +#
>> +# GIT_COMPLETION_CMD_GROUPS=main,others
>
> Mental note #2: "main"
>
>> +#
>> +# Or you could go with main porcelain only and extra commands in
>> +# the configuration variable completion.commands with
>> +#
>> +# GIT_COMPLETION_CMD_GROUPS=mainporcelain,config
>> +#
>> +# Or go completely custom group with
>> +#
>> +# GIT_COMPLETION_CMD_GROUPS=config
>> +#
>> +# Or you could even play with other command categories found in
>> +# 

[PATCH v2] Use OPT_SET_INT_F() for cmdline option specification

[Nguyễn Thái Ngọc Duy]

The only thing these commands need is extra parseopt flag which can be
passed in by OPT_SET_INT_F() and it is a bit more compact than full
struct initialization.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Changes from v1

diff --git a/builtin/am.c b/builtin/am.c
index 666287b497..a1ff235fbc 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2235,7 +2235,7 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
N_("pass --keep-cr flag to git-mailsplit for mbox 
format"),
1, PARSE_OPT_NONEG),
OPT_SET_INT_F(0, "no-keep-cr", _cr,
-   N_("do not pass --keep-cr flag to git-mailsplit for 
mbox format"),
+   N_("do not pass --keep-cr flag to git-mailsplit 
independent of am.keepcr"),
0, PARSE_OPT_NONEG),
OPT_BOOL('c', "scissors", ,
N_("strip everything before a scissors line")),

 archive.c  |  6 ++
 builtin/am.c   | 12 ++--
 builtin/branch.c   |  4 ++--
 builtin/difftool.c |  9 -
 builtin/fetch.c|  6 +++---
 builtin/grep.c |  6 +++---
 builtin/log.c  |  6 +++---
 builtin/ls-files.c |  6 +++---
 builtin/merge.c|  6 +++---
 builtin/notes.c| 12 ++--
 builtin/pack-objects.c | 24 
 11 files changed, 47 insertions(+), 50 deletions(-)

diff --git a/archive.c b/archive.c
index 93ab175b0b..4fe7bec60c 100644
--- a/archive.c
+++ b/archive.c
@@ -411,11 +411,9 @@ static void parse_treeish_arg(const char **argv,
 }
 
 #define OPT__COMPR(s, v, h, p) \
-   { OPTION_SET_INT, (s), NULL, (v), NULL, (h), \
- PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (p) }
+   OPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)
 #define OPT__COMPR_HIDDEN(s, v, p) \
-   { OPTION_SET_INT, (s), NULL, (v), NULL, "", \
- PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }
+   OPT_SET_INT_F(s, NULL, v, "", p, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN)
 
 static int parse_archive_args(int argc, const char **argv,
const struct archiver **ar, struct archiver_args *args,
diff --git a/builtin/am.c b/builtin/am.c
index d834f9e62b..a1ff235fbc 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2231,12 +2231,12 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
N_("pass -b flag to git-mailinfo"), KEEP_NON_PATCH),
OPT_BOOL('m', "message-id", _id,
N_("pass -m flag to git-mailinfo")),
-   { OPTION_SET_INT, 0, "keep-cr", _cr, NULL,
- N_("pass --keep-cr flag to git-mailsplit for mbox format"),
- PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},
-   { OPTION_SET_INT, 0, "no-keep-cr", _cr, NULL,
- N_("do not pass --keep-cr flag to git-mailsplit independent 
of am.keepcr"),
- PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
+   OPT_SET_INT_F(0, "keep-cr", _cr,
+   N_("pass --keep-cr flag to git-mailsplit for mbox 
format"),
+   1, PARSE_OPT_NONEG),
+   OPT_SET_INT_F(0, "no-keep-cr", _cr,
+   N_("do not pass --keep-cr flag to git-mailsplit 
independent of am.keepcr"),
+   0, PARSE_OPT_NONEG),
OPT_BOOL('c', "scissors", ,
N_("strip everything before a scissors line")),
OPT_PASSTHRU_ARGV(0, "whitespace", _apply_opts, 
N_("action"),
diff --git a/builtin/branch.c b/builtin/branch.c
index efc9ac1922..cc089f9efb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -592,8 +592,8 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT__QUIET(, N_("suppress informational messages")),
OPT_SET_INT('t', "track",  , N_("set up tracking mode 
(see git-pull(1))"),
BRANCH_TRACK_EXPLICIT),
-   { OPTION_SET_INT, 0, "set-upstream", , NULL, N_("do not 
use"),
-   PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 
BRANCH_TRACK_OVERRIDE },
+   OPT_SET_INT_F(0, "set-upstream", , N_("do not use"),
+   BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN),
OPT_STRING('u', "set-upstream-to", _upstream, 
N_("upstream"), N_("change the upstream info")),
OPT_BOOL(0, "unset-upstream", _upstream, N_("Unset the 
upstream info")),
OPT__COLOR(_use_color, N_("use colored output")),
diff --git a/builtin/difftool.c b/builtin/difftool.c
index aad0e073ee..c439b64207 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -695,12 +695,11 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
 N_("use `diff.guitool` instead of `diff.tool`")),
OPT_BOOL('d', "dir-diff", _diff,
 

Re: [PATCH] Use OPT_SET_INT_F() for cmdline option specification

[Duy Nguyen]

On Sun, May 20, 2018 at 11:15 AM, Martin Ågren  wrote:
> On 20 May 2018 at 10:12, Nguyễn Thái Ngọc Duy  wrote:
>> The only thing these commands need is extra parseopt flags which can be
>> passed in by OPT_SET_INT_F() and it is a bit more compact than full
>> struct initialization.
>
>> diff --git a/archive.c b/archive.c
>> index 93ab175b0b..4fe7bec60c 100644
>> --- a/archive.c
>> +++ b/archive.c
>> @@ -411,11 +411,9 @@ static void parse_treeish_arg(const char **argv,
>>  }
>>
>>  #define OPT__COMPR(s, v, h, p) \
>> -   { OPTION_SET_INT, (s), NULL, (v), NULL, (h), \
>> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (p) }
>> +   OPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)
>>  #define OPT__COMPR_HIDDEN(s, v, p) \
>> -   { OPTION_SET_INT, (s), NULL, (v), NULL, "", \
>> - PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }
>> +   OPT_SET_INT_F(s, NULL, v, "", p, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN)
>
> Right. We have NULLs in the fifth and the second-to-last positions, and
> we use PARSE_OPT_NOARG.  By switching to OPT_SET_INT_F we get those for
> free.
>
> Do we want to keep "(s)" instead of "s", just to be safe? And same for
> "(v)", "(p)". Macro expansion always makes me paranoid.

They are still wrapped in () in the end by OPT_SET_INT_F() so the
expanded struct initialization  is the same. I think we're fine here.

>> diff --git a/builtin/am.c b/builtin/am.c
>> index d834f9e62b..666287b497 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -2231,12 +2231,12 @@ int cmd_am(int argc, const char **argv, const char 
>> *prefix)
>> N_("pass -b flag to git-mailinfo"), KEEP_NON_PATCH),
>> OPT_BOOL('m', "message-id", _id,
>> N_("pass -m flag to git-mailinfo")),
>> -   { OPTION_SET_INT, 0, "keep-cr", _cr, NULL,
>> - N_("pass --keep-cr flag to git-mailsplit for mbox format"),
>> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},
>> -   { OPTION_SET_INT, 0, "no-keep-cr", _cr, NULL,
>> - N_("do not pass --keep-cr flag to git-mailsplit 
>> independent of am.keepcr"),
>> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
>> +   OPT_SET_INT_F(0, "keep-cr", _cr,
>> +   N_("pass --keep-cr flag to git-mailsplit for mbox 
>> format"),
>> +   1, PARSE_OPT_NONEG),
>> +   OPT_SET_INT_F(0, "no-keep-cr", _cr,
>> +   N_("do not pass --keep-cr flag to git-mailsplit for 
>> mbox format"),
>> +   0, PARSE_OPT_NONEG),
>
> I found `-w` and `--word-diff` useful. You actually change the N_("...")
> for `--no-keep-cr` here: [-independent of am.keepcr-]{+for mbox format+}
> Copy-paste mistake?

Oops. You're correct. Fixed in v2.

>
> Other than that, `--word-diff` has a very structured appearance and
> nothing stood out. The ordering is different (f goes at the end in the
> post-image), which makes the diff busier than it would have had to be.
> (That's obviously nothing this patch can do anything about.)
>
> Martin
-- 
Duy

git push => git: 'credential-winstore' is not a git command.

[Chris]

Hi,

Windows 10
git version 2.17.0.windows.1

I'm having a problem very similar to this one:
https://stackoverflow.com/questions/11693074/git-credential-cache-is-not-a-git-command
One of the comments on the question suggests this command:

git config --global --unset credential.helper


This did help me, because previously Git was trying to authenticate me
with the Microsoft account I use to log into my Windows, which is
unrelated to the account I need to use to push code. And it removed
one of the two "git: 'credential-winstore' is not a git command."
messages I was receiving.

But I still get one of them, so I tried reinstalling Git for Windows
with the credential helper disabled, but that didn't help. Then I ran
this command:

git config -e


And couldn't find any mention of [credential].

What can I do to get rid of this annoying message (and, for all I
know, potential symptom of a larger problem)?

Thanks,
Chris

Re: [PATCHv4 1/1] git-p4: add unshelve command

[SZEDER Gábor]

> diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
> new file mode 100755
> index 00..cca2dec536
> --- /dev/null
> +++ b/t/t9832-unshelve.sh
> @@ -0,0 +1,153 @@
> +#!/bin/sh
> +
> +last_shelved_change() {

Style nit: space between function name and ()

> + p4 changes -s shelved -m1 | cut -d " " -f 2
> +}
> +
> +test_description='git p4 unshelve'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> + start_p4d
> +'
> +
> +test_expect_success 'init depot' '
> + (
> + cd "$cli" &&
> + echo file1 >file1 &&
> + p4 add file1 &&
> + p4 submit -d "change 1"

Broken && chain.

> + : >file_to_delete &&
> + p4 add file_to_delete &&
> + p4 submit -d "file to delete"
> + )
> +'
> +
> +test_expect_success 'initial clone' '
> + git p4 clone --dest="$git" //depot/@all
> +'
> +
> +test_expect_success 'create shelved changelist' '
> + (
> + cd "$cli" &&
> + p4 edit file1 &&
> + echo "a change" >>file1 &&
> + echo "new file" >file2 &&
> + p4 add file2 &&
> + p4 delete file_to_delete &&
> + p4 opened &&
> + p4 shelve -i < +Change: new
> +Description:
> + Test commit
> +
> + Further description
> +Files:
> + //depot/file1
> + //depot/file2
> + //depot/file_to_delete
> +EOF
> +
> + ) &&
> + (
> + cd "$git" &&
> + change=$(last_shelved_change) &&
> + git p4 unshelve $change &&
> + git show refs/remotes/p4/unshelved/$change | grep -q "Further 
> description" &&
> + git cherry-pick refs/remotes/p4/unshelved/$change &&
> + test_path_is_file file2 &&
> + test_cmp file1 "$cli"/file1 &&
> + test_cmp file2 "$cli"/file2 &&
> + test_path_is_missing file_to_delete
> + )
> +'
> +
> +test_expect_success 'update shelved changelist and re-unshelve' '
> + test_when_finished cleanup_git &&
> + (
> + cd "$cli" &&
> + change=$(last_shelved_change) &&
> + echo "file3" >file3 &&
> + p4 add -c $change file3 &&
> + p4 shelve -i -r < +Change: $change
> +Description:
> + Test commit
> +
> + Further description
> +Files:
> + //depot/file1
> + //depot/file2
> + //depot/file3
> + //depot/file_to_delete
> +EOF
> + p4 describe $change
> + ) &&
> + (
> + cd "$git" &&
> + change=$(last_shelved_change) &&
> + git p4 unshelve $change &&
> + git diff refs/remotes/p4/unshelved/$change.0 
> refs/remotes/p4/unshelved/$change | grep -q file3
> + )
> +'
> +
> +# This is the tricky case where the shelved changelist base revision doesn't
> +# match git-p4's idea of the base revision
> +#
> +# We will attempt to unshelve a change that is based on a change one commit
> +# ahead of p4/master
> +
> +test_expect_success 'create shelved changelist based on p4 change ahead of 
> p4/master' '
> + git p4 clone --dest="$git" //depot/@all &&
> + (
> + cd "$cli" &&
> + p4 revert ... &&
> + p4 edit file1 &&
> + echo "foo" >>file1 &&
> + p4 submit -d "change:foo" &&
> + p4 edit file1 &&
> + echo "bar" >>file1 &&
> + p4 shelve -i < +Change: new
> +Description:
> + Change to be unshelved
> +Files:
> + //depot/file1
> +EOF
> + change=$(last_shelved_change) &&
> + p4 describe -S $change | grep -q "Change to be unshelved"
> + )
> +'
> +
> +diff_adds_line() {
> + text="$1" &&
> + file="$2" &&
> + grep -q "^+$text" $file || (echo "expected \"text\" $text not found in 
> $file" && exit 1)
> +}
> +
> +diff_excludes_line() {
> + text="$1" &&
> + file="$2" &&
> + if grep -q "^+$text" $file; then
> + echo "unexpected text \"$text\" found in $file" &&
> + exit 1
> + fi
> +}

It appears that these two function aren't used anywhere.

> +
> +# Now try to unshelve it. git-p4 should refuse to do so.
> +test_expect_success 'try to unshelve the change' '
> + test_when_finished cleanup_git &&
> + (
> + change=$(last_shelved_change) &&
> + cd "$git" &&
> + ! git p4 unshelve $change >out.txt 2>&1 &&
> + grep -q "cannot unshelve" out.txt

Please use 'test_must_fail' instead of '!'; the latter would report
success even if git were to segfault.

Furthermore, don't combine stdout and stderr, but look for the message
only in the stream where it is expected to appear.

> + )
> +'
> +
> +test_expect_success 'kill p4d' '
> + kill_p4d
> +'
> +
> +test_done
> -- 
> 2.17.0.392.gdeb1a6e9b7
> 
> 

Re: [PATCH 14/14] completion: allow to customize the completable command list

[SZEDER Gábor]

On Sat, May 19, 2018 at 6:27 AM, Nguyễn Thái Ngọc Duy  wrote:
> By default we show porcelain, external commands and a couple others
> that are also popular. If you are not happy with this list, you can
> now customize it. See the big comment block for details.
>
> PS. perhaps I should make aliases a group too, which makes it possible
> to _not_ complete aliases by omitting this special group in
> $GIT_COMPLETION_CMD_GROUPS

Note that the completion script reads the configured aliases each time
the user attempts to complete commands.  So if the user adds or
removes an alias, then it will automatically be taken into account the
next time after 'git '.  By turning aliases into a group listed
by 'git help' they would be cached like all other commands, so this
would no longer be the case.

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Documentation/config.txt   | 10 
>  contrib/completion/git-completion.bash | 28 +-
>  git.c  |  2 ++
>  help.c | 33 ++
>  help.h |  1 +
>  5 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2659153cb3..91f7eaed7b 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1343,6 +1343,16 @@ credential..*::
>  credentialCache.ignoreSIGHUP::
> Tell git-credential-cache--daemon to ignore SIGHUP, instead of 
> quitting.
>
> +completion.commands::
> +   This is only used by git-completion.bash to add or remove
> +   commands from the complete list. Normally only porcelain

s/complete list/list of completed commands/ perhaps?

> +   commands and a few select others are in the complete list. You

s/in the complete list/completed/

> +   can add more commands, separated by space, in this
> +   variable. Prefixing the command with '-' will remove it from
> +   the existing list.
> ++
> +This variable should not be per-repository.

I think this should also mention that changing the value of this
config variable will not immediately affect the commands listed after
'git ', but the user will have to re-dot-source the completion
script first.

The way I understand the rest of the patch, this config variable
doesn't have any effect if $GIT_COMPLETION_CMD_GROUPS doesn't contain
"config".  If that is indeed the case, then that should be mentioned
here as well.

Having said that, I wonder whether we should really require "config"
in $GIT_COMPLETION_CMD_GROUPS.  Isn't having 'completion.commands' set
in the config a clear enough indication in itself that the user wants
to customize the listed commands?

> +
>  include::diff-config.txt[]
>
>  difftool..path::
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index cd1d8e553f..f237eb0ff4 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -38,6 +38,29 @@
>  #
>  # When set to "1", do not include "DWIM" suggestions in git-checkout
>  # completion (e.g., completing "foo" when "origin/foo" exists).
> +#
> +#   GIT_COMPLETION_CMD_GROUPS
> +#
> +# When set, "git --list-cmds=$GIT_COMPLETION_CMD_GROUPS" will be
> +# used to get the list of completable commands. The default is
> +# "mainporcelain,others,list-complete" (in English: all porcelain

Mental note #1: "mainporcelain"

> +# commands and external ones are included. Certain non-porcelain
> +# commands are also marked for completion in command-list.txt).
> +# You could for example complete all commands with
> +#
> +# GIT_COMPLETION_CMD_GROUPS=main,others

Mental note #2: "main"

> +#
> +# Or you could go with main porcelain only and extra commands in
> +# the configuration variable completion.commands with
> +#
> +# GIT_COMPLETION_CMD_GROUPS=mainporcelain,config
> +#
> +# Or go completely custom group with
> +#
> +# GIT_COMPLETION_CMD_GROUPS=config
> +#
> +# Or you could even play with other command categories found in
> +# command-list.txt.
>
>  case "$COMP_WORDBREAKS" in
>  *:*) : great ;;
> @@ -842,8 +865,11 @@ __git_commands () {
> if test -n "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
> then
> printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
> +   elif test -n "$GIT_COMPLETION_CMD_GROUPS"
> +   then
> +   git --list-cmds="$GIT_COMPLETION_CMD_GROUPS"
> else
> -   git 
> --list-cmds=list-mainporcelain,others,list-complete
> +   git 
> --list-cmds=list-mainporcelain,others,list-complete,config

So first it was "mainporcelain", then simply "main", then
"mainporcelain" again, and now "list-mainporcelain"?!
You've lost me here.

Are the possible values 

Re: [PATCH 13/14] completion: reduce completable command list

[SZEDER Gábor]

On Sat, May 19, 2018 at 6:27 AM, Nguyễn Thái Ngọc Duy  wrote:
> The following commands are removed from the complete list:
>
> - annotate obsolete, discouraged to use
> - filter-branchnot often used
> - get-tar-commit-idnot often used
> - imap-sendnot often used
> - interpreter-trailers not for interactive use
> - p4   too short and probably not often used (*)
> - svn  same category as p4 (*)
> - verify-commitnot often used
>

> @@ -127,7 +127,7 @@ git-mktree  
> plumbingmanipulators
>  git-mv  mainporcelain   worktree
>  git-name-revplumbinginterrogators   
> complete

Since 'git name-rev' is plumbing, and since it's functionality is
fully covered by the 'git describe' porcelain, shouldn't it be removed
as well?

Re: [PATCH 12/14] completion: let git provide the completable command list

[SZEDER Gábor]

On Sat, May 19, 2018 at 6:27 AM, Nguyễn Thái Ngọc Duy  wrote:
> Instead of maintaining a separate list of command classification,
> which often could go out of date, let's centralize the information
> back in git.
>
> While the function in git-completion.bash implies "list porcelain
> commands", that's not exactly what it does. It gets all commands (aka
> --list-cmds=main,others) then exclude certain non-porcelain ones. We
> could almost recreate this list two lists list-mainporcelain and
> others. The non-porcelain-but-included-anyway is added by the third
> category list-complete.
>
> list-complete does not recreate exactly the command list before this
> patch though. The following commands will disappear from the complete
> list because they are not in command-list.txt and it's not worth
> adding them back: lost-found, peek-remote and tar-tree.

These commands have been removed long ago, see the topic leading to
577aed296a (Merge branch 'jk/remove-deprecated', 2013-12-12).  Perhaps
you saw them only because they are still somewhere on your $PATH or
$GIT_EXEC_PATH?

> Note that the current completion script incorrectly classifies
> filter-branch as porcelain and t9902 tests this behavior. We keep it
> this way in t9902 because this test does not really care which
> particular command is porcelain or plubmbing, they're just names.

s/plubmbing/plumbing/

Re: [PATCH v2 01/12] commit-graph: add 'verify' subcommand

[Jakub Narebski]

Derrick Stolee  writes:

> If the commit-graph file becomes corrupt, we need a way to verify
> that its contents match the object database. In the manner of
> 'git fsck' we will implement a 'git commit-graph verify' subcommand
> to report all issues with the file.
>
> Add the 'verify' subcommand to the 'commit-graph' builtin and its
> documentation. The subcommand is currently a no-op except for
> loading the commit-graph into memory, which may trigger run-time
> errors that would be caught by normal use. Add a simple test that
> ensures the command returns a zero error code.
>
> If no commit-graph file exists, this is an acceptable state. Do
> not report any errors.

All right.  Nice introductory patch.

>
> During review, we noticed that a FREE_AND_NULL(graph_name) was
> placed after a possible 'return', and this pattern was also in
> graph_read(). Fix that case, too.

This should probably be a separate [micro-]patch.  Especially as Martin
Ågren noticed it is not correct...

>
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/git-commit-graph.txt |  6 ++
>  builtin/commit-graph.c | 40 
> +-
>  commit-graph.c |  5 +
>  commit-graph.h |  2 ++
>  t/t5318-commit-graph.sh| 10 ++
>  5 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index 4c97b555cc..a222cfab08 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  
>  [verse]
>  'git commit-graph read' [--object-dir ]
> +'git commit-graph verify' [--object-dir ]
>  'git commit-graph write'  [--object-dir ]
>  
>  
> @@ -52,6 +53,11 @@ existing commit-graph file.
>  Read a graph file given by the commit-graph file and output basic
>  details about the graph file. Used for debugging purposes.
>  
> +'verify'::
> +
> +Read the commit-graph file and verify its contents against the object
> +database. Used to check for corrupted data.

I wonder if it would be useful to have an option to verify commit-graph
file without accessing the object database, checking just that it is
well formed.

Anyway, it could be added later, if needed.

> +
>  
>  EXAMPLES
>  
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 37420ae0fd..af3101291f 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -8,10 +8,16 @@
>  static char const * const builtin_commit_graph_usage[] = {
>   N_("git commit-graph [--object-dir ]"),
>   N_("git commit-graph read [--object-dir ]"),
> + N_("git commit-graph verify [--object-dir ]"),
>   N_("git commit-graph write [--object-dir ] [--append] 
> [--stdin-packs|--stdin-commits]"),
>   NULL
>  };
>  
> +static const char * const builtin_commit_graph_verify_usage[] = {
> + N_("git commit-graph verify [--object-dir ]"),
> + NULL
> +};
> +
>  static const char * const builtin_commit_graph_read_usage[] = {
>   N_("git commit-graph read [--object-dir ]"),
>   NULL
> @@ -29,6 +35,36 @@ static struct opts_commit_graph {
>   int append;
>  } opts;
>  
> +
> +static int graph_verify(int argc, const char **argv)

A reminder for myself: exit code 0 means no errors.

> +{
> + struct commit_graph *graph = 0;
> + char *graph_name;
> +
> + static struct option builtin_commit_graph_verify_options[] = {
> + OPT_STRING(0, "object-dir", _dir,
> +N_("dir"),
> +N_("The object directory to store the graph")),
> + OPT_END(),
> + };
> +
> + argc = parse_options(argc, argv, NULL,
> +  builtin_commit_graph_verify_options,
> +  builtin_commit_graph_verify_usage, 0);
> +
> + if (!opts.obj_dir)
> + opts.obj_dir = get_object_directory();

All right, simple handling of a subcommand and its options.


I still think that '--object-dir=' should be a git wrapper option,
like '--git-dir=' and '--work-tree=' (and
'--namespace=') are.  It would be command-line option equivalent
to the GIT_OBJECT_DIRECTORY environment variable, just like
--git-dir= is for GIT_DIR, and --work-tree= is for
GIT_WORK_TREE, etc.

This way the code would be implemented once for all commands, and there
would be no duplicated code for each git-commit-graph subcommand.

But that may be a matter of a separate patch.

> +
> + graph_name = get_commit_graph_filename(opts.obj_dir);

This returns full path of commit-graph file, allocating it.

> + graph = load_commit_graph_one(graph_name);

This reads the file (no checking if core.commitGraph is set, no handling
of alternatives), and verifies that:
 - file is not too small, i.e. smaller than GRAPH_MIN_SIZE
 - it has correct signature
 - it has correct graph version
 - it has correct hash 

Re:Second Email Notification!!

[E Charity Grant]

Hello Lucky Beneficiary,

You have been selected to receive (€950,000.00) as a donation from the Emirates 
Foundation Award 2018. Kindly reply back for more details;

Best Regards,
Abdullah bin Zayed Al Nahyan.
Contact E-mail: emiratesfor...@asia.com

President of the Emirates Foundation.

[PATCH] regex: do not call `regfree()` if compilation fails

[Martin Ågren]

It is apparently undefined behavior to call `regfree()` on a regex where
`regcomp()` failed. The language in [1] is a bit muddy, at least to me,
but the clearest hint is this (`preg` is the `regex_t *`):

Upon successful completion, the regcomp() function shall return 0.
Otherwise, it shall return an integer value indicating an error as
described in , and the content of preg is undefined.

Funnily enough, there is also the `regerror()` function which should be
given a pointer to such a "failed" `regex_t` -- the content of which
would supposedly be undefined -- and which may investigate it to come up
with a detailed error message.

In any case, the example in that document shows how `regfree()` is not
called after `regcomp()` fails.

We have quite a few users of this API and most get this right. These
three users do not.

Several implementations can handle this just fine [2] and these code paths
supposedly have not wreaked havoc or we'd have heard about it. (These
are all in code paths where git got bad input and is just about to die
anyway.) But let's just avoid the issue altogether.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html

[2] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html

Researched-by: Eric Sunshine 
Signed-off-byi Martin Ågren 
---

On 14 May 2018 at 05:05, Eric Sunshine  wrote:
> My research (for instance [1,2]) seems to indicate that it would be
> better to avoid regfree() upon failed regcomp(), even though such a
> situation is handled sanely in some implementations.
>
> [1]: https://www.redhat.com/archives/libvir-list/2013-September/msg00276.html
> [2]: https://www.redhat.com/archives/libvir-list/2013-September/msg00273.html

Thank you for researching this. I think it would make sense to get rid
of the few places we have where we get this wrong (if our understanding
of this being undefined is right).

 diffcore-pickaxe.c | 1 -
 grep.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 239ce5122b..800a899c86 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char 
*needle, int cflags)
/* The POSIX.2 people are surely sick */
char errbuf[1024];
regerror(err, regex, errbuf, 1024);
-   regfree(regex);
die("invalid regex: %s", errbuf);
}
 }
diff --git a/grep.c b/grep.c
index 65b90c10a3..5e4f3f9a9d 100644
--- a/grep.c
+++ b/grep.c
@@ -636,7 +636,6 @@ static void compile_fixed_regexp(struct grep_pat *p, struct 
grep_opt *opt)
if (err) {
char errbuf[1024];
regerror(err, >regexp, errbuf, sizeof(errbuf));
-   regfree(>regexp);
compile_regexp_failed(p, errbuf);
}
 }
@@ -701,7 +700,6 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
if (err) {
char errbuf[1024];
regerror(err, >regexp, errbuf, 1024);
-   regfree(>regexp);
compile_regexp_failed(p, errbuf);
}
 }
-- 
2.17.0.840.g5d83f92caf

[PATCH v2 3/3] config: let `config_store_data_clear()` handle `key`

[Martin Ågren]

Instead of remembering to free `key` in each code path, let
`config_store_data_clear()` handle that.

We still need to free it before replacing it, though. Move that freeing
closer to the replacing to be safe. Note that in that same part of the
code, we can no longer set `key` to the original pointer, but need to
`xstrdup()` it.

Signed-off-by: Martin Ågren 
---
 config.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index ac71f3f2e1..339a92235d 100644
--- a/config.c
+++ b/config.c
@@ -2335,6 +2335,7 @@ struct config_store_data {
 
 static void config_store_data_clear(struct config_store_data *store)
 {
+   free(store->key);
if (store->value_regex != NULL &&
store->value_regex != CONFIG_REGEX_NONE) {
regfree(store->value_regex);
@@ -2679,7 +2680,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
fd = hold_lock_file_for_update(, config_filename, 0);
if (fd < 0) {
error_errno("could not lock config file %s", config_filename);
-   free(store.key);
ret = CONFIG_NO_LOCK;
goto out_free;
}
@@ -2689,8 +2689,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
 */
in_fd = open(config_filename, O_RDONLY);
if ( in_fd < 0 ) {
-   free(store.key);
-
if ( ENOENT != errno ) {
error_errno("opening %s", config_filename);
ret = CONFIG_INVALID_FILE; /* same as "invalid config 
file" */
@@ -2702,7 +2700,8 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
goto out_free;
}
 
-   store.key = (char *)key;
+   free(store.key);
+   store.key = xstrdup(key);
if (write_section(fd, key, ) < 0 ||
write_pair(fd, key, value, ) < 0)
goto write_err_out;
@@ -2752,13 +2751,10 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
  config_filename,
  , )) {
error("invalid config file %s", config_filename);
-   free(store.key);
ret = CONFIG_INVALID_FILE;
goto out_free;
}
 
-   free(store.key);
-
/* if nothing to unset, or too many matches, error out */
if ((store.seen_nr == 0 && value == NULL) ||
(store.seen_nr > 1 && multi_replace == 0)) {
-- 
2.17.0.840.g5d83f92caf

[PATCH v2 2/3] config: let `config_store_data_clear()` handle `value_regex`

[Martin Ågren]

Instead of duplicating the logic for clearing up `value_regex`, let
`config_store_data_clear()` handle that.

When `regcomp()` fails, the current code does not call `regfree()`. Make
sure we do the same by immediately invalidating `value_regex`. Some
implementations are able to handle such an extra `regfree()`-call [1],
but from the example in [2], we should not do so. (The language itself
in [2] is not super-clear on this.)

[1] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html

[2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html

Researched-by: Eric Sunshine 
Signed-off-by: Martin Ågren 
---
 config.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index b3282f7193..ac71f3f2e1 100644
--- a/config.c
+++ b/config.c
@@ -2335,6 +2335,11 @@ struct config_store_data {
 
 static void config_store_data_clear(struct config_store_data *store)
 {
+   if (store->value_regex != NULL &&
+   store->value_regex != CONFIG_REGEX_NONE) {
+   regfree(store->value_regex);
+   free(store->value_regex);
+   }
free(store->parsed);
free(store->seen);
memset(store, 0, sizeof(*store));
@@ -2722,7 +2727,7 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
if (regcomp(store.value_regex, value_regex,
REG_EXTENDED)) {
error("invalid pattern: %s", value_regex);
-   free(store.value_regex);
+   FREE_AND_NULL(store.value_regex);
ret = CONFIG_INVALID_PATTERN;
goto out_free;
}
@@ -2748,21 +2753,11 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
  , )) {
error("invalid config file %s", config_filename);
free(store.key);
-   if (store.value_regex != NULL &&
-   store.value_regex != CONFIG_REGEX_NONE) {
-   regfree(store.value_regex);
-   free(store.value_regex);
-   }
ret = CONFIG_INVALID_FILE;
goto out_free;
}
 
free(store.key);
-   if (store.value_regex != NULL &&
-   store.value_regex != CONFIG_REGEX_NONE) {
-   regfree(store.value_regex);
-   free(store.value_regex);
-   }
 
/* if nothing to unset, or too many matches, error out */
if ((store.seen_nr == 0 && value == NULL) ||
-- 
2.17.0.840.g5d83f92caf

[PATCH v2 1/3] config: free resources of `struct config_store_data`

[Martin Ågren]

Commit fee8572c6d (config: avoid using the global variable `store`,
2018-04-09) dropped the staticness of a certain struct, instead letting
the users create an instance on the stack and pass around a pointer.

We do not free all the memory that the struct tracks. When the struct
was static, the memory would always be reachable. Now that we keep the
struct on the stack, though, as soon as we return, it goes out of scope
and we leak the memory it points to. In particular, we leak the memory
pointed to by the `parsed` and `seen` fields.

Introduce and use a helper function `config_store_data_clear()` to plug
these leaks. The memory tracked here is config parser events. Once the
users (`git_config_set_multivar_in_file_gently()` and
`git_config_copy_or_rename_section_in_file()` at the moment) are done,
no-one should be holding on to a pointer into this memory.

There are two more members of the struct that are candidates for freeing
in this new function (`key` and `value_regex`). Those are actually
already being taken care of. The next couple of patches will move their
freeing into the function we are adding here.

Signed-off-by: Martin Ågren 
---
 config.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/config.c b/config.c
index 6f8f1d8c11..b3282f7193 100644
--- a/config.c
+++ b/config.c
@@ -2333,6 +2333,13 @@ struct config_store_data {
unsigned int key_seen:1, section_seen:1, is_keys_section:1;
 };
 
+static void config_store_data_clear(struct config_store_data *store)
+{
+   free(store->parsed);
+   free(store->seen);
+   memset(store, 0, sizeof(*store));
+}
+
 static int matches(const char *key, const char *value,
   const struct config_store_data *store)
 {
@@ -2887,6 +2894,7 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
munmap(contents, contents_sz);
if (in_fd >= 0)
close(in_fd);
+   config_store_data_clear();
return ret;
 
 write_err_out:
@@ -3127,6 +3135,7 @@ static int 
git_config_copy_or_rename_section_in_file(const char *config_filename
rollback_lock_file();
 out_no_rollback:
free(filename_buf);
+   config_store_data_clear();
return ret;
 }
 
-- 
2.17.0.840.g5d83f92caf

[PATCH v2 0/3] config: free resources of `struct config_store_data`

[Martin Ågren]

On 14 May 2018 at 05:03, Eric Sunshine  wrote:
> On Sun, May 13, 2018 at 5:58 AM, Martin Ågren  wrote:
>>
>> How about the following two patches as patches 2/3 and 3/3? I would also
>> need to mention in the commit message of this patch (1/3) that the
>> function will soon learn to clean up more members.
>>
>
> Yep, making this a multi-part patch series and updating the commit
> message of the patch which introduces config_store_data_clear(), as
> you suggest, makes sense. The patch series could be organized
> differently -- such as first moving freeing of 'value_regex' into new
> config_store_data_clear(), then freeing additional fields in later
> patches -- but I don't think it matters much in practice, so the
> current organization is likely good enough.

I tried such a re-ordering but wasn't entirely happy about the result
(maybe I didn't try hard enough), so here are these patches again, as a
proper series and with improved commit messages.

Martin

Martin Ågren (3):
  config: free resources of `struct config_store_data`
  config: let `config_store_data_clear()` handle `value_regex`
  config: let `config_store_data_clear()` handle `key`

 config.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

-- 
2.17.0.840.g5d83f92caf

[PATCH v4 2/4] merge-recursive: provide pair of `unpack_trees_{start,finish}()`

[Martin Ågren]

From: Elijah Newren 

Rename `git_merge_trees()` to `unpack_trees_start()` and extract the
call to `discard_index()` into a new function `unpack_trees_finish()`.
As a result, these are called early resp. late in `merge_trees()`,
making the resource handling clearer. A later commit will expand on
that, teaching `..._finish()` to free more memory. (So rather than
moving the FIXME-comment, just drop it, since it will be addressed soon
enough.)

Also call `..._finish()` when `merge_trees()` returns early.

Signed-off-by: Elijah Newren 
Signed-off-by: Martin Ågren 
---
 merge-recursive.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 680e01226b..ddb0fa7369 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,10 +337,10 @@ static void init_tree_desc_from_tree(struct tree_desc 
*desc, struct tree *tree)
init_tree_desc(desc, tree->buffer, tree->size);
 }
 
-static int git_merge_trees(struct merge_options *o,
-  struct tree *common,
-  struct tree *head,
-  struct tree *merge)
+static int unpack_trees_start(struct merge_options *o,
+ struct tree *common,
+ struct tree *head,
+ struct tree *merge)
 {
int rc;
struct tree_desc t[3];
@@ -379,6 +379,11 @@ static int git_merge_trees(struct merge_options *o,
return rc;
 }
 
+static void unpack_trees_finish(struct merge_options *o)
+{
+   discard_index(>orig_index);
+}
+
 struct tree *write_tree_from_memory(struct merge_options *o)
 {
struct tree *result = NULL;
@@ -3088,13 +3093,14 @@ int merge_trees(struct merge_options *o,
return 1;
}
 
-   code = git_merge_trees(o, common, head, merge);
+   code = unpack_trees_start(o, common, head, merge);
 
if (code != 0) {
if (show(o, 4) || o->call_depth)
err(o, _("merging of trees %s and %s failed"),
oid_to_hex(>object.oid),
oid_to_hex(>object.oid));
+   unpack_trees_finish(o);
return -1;
}
 
@@ -3147,20 +3153,15 @@ int merge_trees(struct merge_options *o,
 
hashmap_free(>current_file_dir_set, 1);
 
-   if (clean < 0)
+   if (clean < 0) {
+   unpack_trees_finish(o);
return clean;
+   }
}
else
clean = 1;
 
-   /* Free the extra index left from git_merge_trees() */
-   /*
-* FIXME: Need to also free data allocated by
-* setup_unpack_trees_porcelain() tucked away in o->unpack_opts.msgs,
-* but the problem is that only half of it refers to dynamically
-* allocated data, while the other half points at static strings.
-*/
-   discard_index(>orig_index);
+   unpack_trees_finish(o);
 
if (o->call_depth && !(*result = write_tree_from_memory(o)))
return -1;
-- 
2.17.0.840.g5d83f92caf

[PATCH v4 3/4] string-list: provide `string_list_appendf()`

[Martin Ågren]

Add a function `string_list_appendf(list, fmt, ...)` to the string-list
API. The next commit will add a user.

This function naturally ignores the `strdup_strings`-setting and always
appends a freshly allocated string. Thus, using this function with
`strdup_strings = 0` risks making ownership unclear and leaking memory.
With `strdup_strings = 1` on the other hand, we can easily add formatted
strings without going through `string_list_append_nodup()` or playing
with `strdup_strings`.

Suggested-by: Jeff King 
Signed-off-by: Martin Ågren 
---
 string-list.h |  9 +
 string-list.c | 13 +
 2 files changed, 22 insertions(+)

diff --git a/string-list.h b/string-list.h
index ff8f6094a3..3a73b86ffa 100644
--- a/string-list.h
+++ b/string-list.h
@@ -208,6 +208,15 @@ void string_list_remove_duplicates(struct string_list 
*sorted_list, int free_uti
  */
 struct string_list_item *string_list_append(struct string_list *list, const 
char *string);
 
+/**
+ * Add formatted string to the end of `list`. This function ignores
+ * the value of `list->strdup_strings` and always appends a freshly
+ * allocated string, so you will probably not want to use it with
+ * `strdup_strings = 0`.
+ */
+struct string_list_item *string_list_appendf(struct string_list *list,
+const char *fmt, ...);
+
 /**
  * Like string_list_append(), except string is never copied.  When
  * list->strdup_strings is set, this function can be used to hand
diff --git a/string-list.c b/string-list.c
index a0cf0cfe88..b54d31c1cf 100644
--- a/string-list.c
+++ b/string-list.c
@@ -224,6 +224,19 @@ struct string_list_item *string_list_append(struct 
string_list *list,
list->strdup_strings ? xstrdup(string) : (char 
*)string);
 }
 
+struct string_list_item *string_list_appendf(struct string_list *list,
+const char *fmt, ...)
+{
+   struct string_list_item *retval;
+   va_list ap;
+
+   va_start(ap, fmt);
+   retval = string_list_append_nodup(list, xstrvfmt(fmt, ap));
+   va_end(ap);
+
+   return retval;
+}
+
 static int cmp_items(const void *a, const void *b, void *ctx)
 {
compare_strings_fn cmp = ctx;
-- 
2.17.0.840.g5d83f92caf

[PATCH v4 0/4] unpack_trees_options: free messages when done

[Martin Ågren]

This is v4 of my series for taking care of the memory allocated by
`setup_unpack_trees_porcelain()`. As before, this is based on
bp/merge-rename-config.

On 19 May 2018 at 08:13, Martin Ågren  wrote:
> On 19 May 2018 at 03:02, Jeff King  wrote:
>>
>>> > would become:
>>> >
>>> >   msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOUPTODATE_FILE] =
>>> > string_list_appendf(>msgs_to_free, msg, cmd, cmd)->string;
>>> >
>>> > I don't know if that's worth it or not (I suspect that there are other
>>> > places where appendf would be handy, but I didn't poke around).
>
> I'll look into this over the weekend. Thanks for the suggestion.

The difference to v3 is indeed the new patch 3/4, which introduces
`string_list_appendf()`. I think that makes patch 4/4 clearer and the
resulting code less surprising.

There is an obvious candidate for using this new function in bisect.c,
but I refrained from doing that conversion in this series. While
converting that user to use this new function would be trivial and safe,
such a change might not look entirely sane on its own. The reason is
that the user does the whole `strdup_strings`-dance that I did in v3.

I think it would be much better to do that conversion as a part of a
"let's not play with strdup_strings"-patch. I have one prepared and it
looks quite ok to me. I should be able to be able to collect more
`strdup_string`-cleanups soonish and submit a series later (say, when/if
this here series has matured).

Elijah Newren (1):
  merge-recursive: provide pair of `unpack_trees_{start,finish}()`

Martin Ågren (3):
  merge: setup `opts` later in `checkout_fast_forward()`
  string-list: provide `string_list_appendf()`
  unpack_trees_options: free messages when done

 string-list.h  |  9 +
 unpack-trees.h |  6 ++
 builtin/checkout.c |  1 +
 merge-recursive.c  | 30 --
 merge.c| 35 ---
 string-list.c  | 13 +
 unpack-trees.c | 20 +---
 7 files changed, 82 insertions(+), 32 deletions(-)

-- 
2.17.0.840.g5d83f92caf

[PATCH v4 1/4] merge: setup `opts` later in `checkout_fast_forward()`

[Martin Ågren]

After we initialize the various fields in `opts` but before we actually
use them, we might return early. Move the initialization further down,
to immediately before we use `opts`.

This limits the scope of `opts` and will help a later commit fix a
memory leak without having to worry about those early returns.

This patch is best viewed using something like this (note the tab!):
--color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 merge.c | 34 ++
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/merge.c b/merge.c
index f06a4773d4..f123658e58 100644
--- a/merge.c
+++ b/merge.c
@@ -94,23 +94,7 @@ int checkout_fast_forward(const struct object_id *head,
return -1;
 
memset(, 0, sizeof(trees));
-   memset(, 0, sizeof(opts));
memset(, 0, sizeof(t));
-   if (overwrite_ignore) {
-   memset(, 0, sizeof(dir));
-   dir.flags |= DIR_SHOW_IGNORED;
-   setup_standard_excludes();
-   opts.dir = 
-   }
-
-   opts.head_idx = 1;
-   opts.src_index = _index;
-   opts.dst_index = _index;
-   opts.update = 1;
-   opts.verbose_update = 1;
-   opts.merge = 1;
-   opts.fn = twoway_merge;
-   setup_unpack_trees_porcelain(, "merge");
 
trees[nr_trees] = parse_tree_indirect(head);
if (!trees[nr_trees++]) {
@@ -126,6 +110,24 @@ int checkout_fast_forward(const struct object_id *head,
parse_tree(trees[i]);
init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
}
+
+   memset(, 0, sizeof(opts));
+   if (overwrite_ignore) {
+   memset(, 0, sizeof(dir));
+   dir.flags |= DIR_SHOW_IGNORED;
+   setup_standard_excludes();
+   opts.dir = 
+   }
+
+   opts.head_idx = 1;
+   opts.src_index = _index;
+   opts.dst_index = _index;
+   opts.update = 1;
+   opts.verbose_update = 1;
+   opts.merge = 1;
+   opts.fn = twoway_merge;
+   setup_unpack_trees_porcelain(, "merge");
+
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
return -1;
-- 
2.17.0.840.g5d83f92caf

[PATCH v4 4/4] unpack_trees_options: free messages when done

[Martin Ågren]

The strings allocated in `setup_unpack_trees_porcelain()` are never
freed. Provide a function `clear_unpack_trees_porcelain()` to do so and
call it where we use `setup_unpack_trees_porcelain()`. The only
non-trivial user is `unpack_trees_start()`, where we should place the
new call in `unpack_trees_finish()`.

We keep the string pointers in an array, mixing pointers to static
memory and memory that we allocate on the heap. We also keep several
copies of the individual pointers. So we need to make sure that we do
not free what we must not free and that we do not double-free. Keep the
unique, heap-allocated pointers in a separate string list, to make the
freeing safe and future-proof.

Zero the whole array of string pointers to make sure that we do not
leave any dangling pointers.

Note that we only take responsibility for the memory allocated in
`setup_unpack_trees_porcelain()` and not any other members of the
`struct unpack_trees_options`.

Helped-by: Junio C Hamano 
Helped-by: Jeff King 
Signed-off-by: Martin Ågren 
---
 unpack-trees.h |  6 ++
 builtin/checkout.c |  1 +
 merge-recursive.c  |  1 +
 merge.c|  3 +++
 unpack-trees.c | 20 +---
 5 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.h b/unpack-trees.h
index 41178ada94..5a84123a40 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -33,6 +33,11 @@ enum unpack_trees_error_types {
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
  const char *cmd);
 
+/*
+ * Frees resources allocated by setup_unpack_trees_porcelain().
+ */
+void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
+
 struct unpack_trees_options {
unsigned int reset,
 merge,
@@ -57,6 +62,7 @@ struct unpack_trees_options {
struct pathspec *pathspec;
merge_fn_t fn;
const char *msgs[NB_UNPACK_TREES_ERROR_TYPES];
+   struct string_list msgs_to_free;
/*
 * Store error messages in an array, each case
 * corresponding to a error message type
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..5cebe170fc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -526,6 +526,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
init_tree_desc([1], tree->buffer, tree->size);
 
ret = unpack_trees(2, trees, );
+   clear_unpack_trees_porcelain();
if (ret == -1) {
/*
 * Unpack couldn't do a trivial merge; either
diff --git a/merge-recursive.c b/merge-recursive.c
index ddb0fa7369..338f63a952 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -382,6 +382,7 @@ static int unpack_trees_start(struct merge_options *o,
 static void unpack_trees_finish(struct merge_options *o)
 {
discard_index(>orig_index);
+   clear_unpack_trees_porcelain(>unpack_opts);
 }
 
 struct tree *write_tree_from_memory(struct merge_options *o)
diff --git a/merge.c b/merge.c
index f123658e58..b433291d0c 100644
--- a/merge.c
+++ b/merge.c
@@ -130,8 +130,11 @@ int checkout_fast_forward(const struct object_id *head,
 
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
+   clear_unpack_trees_porcelain();
return -1;
}
+   clear_unpack_trees_porcelain();
+
if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
return 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index 79fd97074e..86046b987a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -103,6 +103,12 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
const char **msgs = opts->msgs;
const char *msg;
 
+   /*
+* As we add strings using `...appendf()`, this does not matter,
+* but when we clear the string list, we want them to be freed.
+*/
+   opts->msgs_to_free.strdup_strings = 1;
+
if (!strcmp(cmd, "checkout"))
msg = advice_commit_before_merge
  ? _("Your local changes to the following files would be 
overwritten by checkout:\n%%s"
@@ -119,7 +125,7 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
  "Please commit your changes or stash them before you 
%s.")
  : _("Your local changes to the following files would be 
overwritten by %s:\n%%s");
msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
-   xstrfmt(msg, cmd, cmd);
+   string_list_appendf(>msgs_to_free, msg, cmd, cmd)->string;
 
msgs[ERROR_NOT_UPTODATE_DIR] =
_("Updating the following directories would lose untracked 
files in them:\n%s");
@@ -139,7 +145,8 @@ void setup_unpack_trees_porcelain(struct 

Re: [RFC 00/14] Allow fetch-pack to send ref names (globs allowed)

[Apinan Ponchan]

ส่งจาก iPhone ของฉัน