Re: [PATCH 1/5] chainlint: match arbitrary here-docs tags rather than hard-coded names

[Eric Sunshine]

On Wed, Aug 8, 2018 at 6:50 PM Jeff King  wrote:
> On Tue, Aug 07, 2018 at 04:21:31AM -0400, Eric Sunshine wrote:
> > +# Swallowing here-docs with arbitrary tags requires a bit of finesse. When 
> > a
> > +# line such as "cat  > front
> > +# of the line enclosed in angle brackets as a sentinel, giving "cat 
> > >out".
>
> Gross, but OK, as long as we would not get confused by a line that
> actually started with  at the start.

It can't get confused by such a line. There here-doc swallower
prepends that when it starts the swallowing process and removes it add
the end. Even if a line actually started with that, it would become
"cmd" while swallowing the here-doc, and be restored to
"cmd" at the end. Stripping the "" is done non-greedily, so
it wouldn't remove both of them. Likewise, non-greedy matching is used
for pulling the "EOF" out of the "<...>" when trying to match against
the terminating "EOF" line, so there can be no confusion.

> > +/<<[ ]*[-\\]*[A-Z0-9_][A-Z0-9_]*/ {
> > + s/^\(.*\)<<[]*[-\\]*\([A-Z0-9_][A-Z0-9_]*\)/<\2>\1< > + s/[ ]*<
> Here-docs can use lowercase, too, though I'd personally frown on that
> from a style perspective.

Yeah, I was going with the tighter uppercase-only which Jonathan
suggested[1], but I guess it wouldn't hurt to re-roll to allow
lowercase too.

[1]: 
https://public-inbox.org/git/20180730205914.ge156...@aiede.svl.corp.google.com/

> It looks like this doesn't catch:
>
>   cat <<'EOF'
>   EOF
>
> either. I think we prefer the backslash style, but there are quite a few
> <<-'EOF' hits. Is it covered somewhere else?

No. I've gotten so used to \EOF in this codebase that it didn't occur
to me to even think about 'EOF', but a re-roll could add that, as
well.

Re: Help with "fatal: unable to read ...." error during GC?

[Paul Smith]

On Wed, 2018-08-08 at 14:24 -0400, Jeff King wrote:
> If so, can you try running it under gdb and getting a stack trace?
> Something like:
> 
>   gdb git
>   [and then inside gdb...]
>   set args pack-objects --all --reflog --indexed-objects foobreak die
>   run
>   bt
> 
> That might give us a clue where the broken object reference is coming

Here we go.  I can rebuild with -Og or -O0 if more detailed debugging
is needed; most everything appears to be optimized out:

  ...
Compressing objects: 100% (10/10), done.
Writing objects:  54% (274416/508176)   
Thread 1 "git" hit Breakpoint 1, die (err=err@entry=0x5a373a "unable to read 
%s") at usage.c:119
119 {
(gdb) bt
#0  die (err=err@entry=0x5a373a "unable to read %s") at usage.c:119
#1  0x004563f3 in get_delta (entry=) at 
builtin/pack-objects.c:143
#2  write_no_reuse_object () at builtin/pack-objects.c:308
#3  0x00456592 in write_reuse_object (usable_delta=, 
limit=, entry=, f=) at 
builtin/pack-objects.c:516
#4  write_object (write_offset=, entry=0x7fffc9a8d940, 
f=0x198fb70) at builtin/pack-objects.c:518
#5  write_one () at builtin/pack-objects.c:576
#6  0x004592f0 in write_pack_file () at builtin/pack-objects.c:849
#7  cmd_pack_objects (argc=, argv=, 
prefix=) at builtin/pack-objects.c:3354
#8  0x00404f06 in run_builtin (argv=, argc=, p=) at git.c:417
#9  handle_builtin (argc=, argv=) at git.c:632
#10 0x00405f21 in run_argv (argv=0x7fffe210, argcp=0x7fffe21c) 
at git.c:761
#11 cmd_main (argc=, argc@entry=6, argv=, 
argv@entry=0x7fffe448) at git.c:761
#12 0x00404b15 in main (argc=6, argv=0x7fffe448) at common-main.c:45

Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted

[brian m. carlson]

On Wed, Aug 08, 2018 at 05:59:43PM -0700, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> 
> >> FWIW, I'm on board with returning non-zero in any case where gpg would.
> >
> > I think that's probably the best solution overall.
> 
> FWIW, I am not married to the current behaviour.  I would not be
> surprised if it mostly came by accident and not designed.

Since apparently I was the author of the commit that changed the
behavior originally, let me simply say that I was not aware that gpg
signalled the correctness of a signature by its exit status when I wrote
that patch.  If I had known that, I would have deferred to gpg in my
change.  My goal was consistency between verify-tag and verify-commit,
and in retrospect I probably made the wrong decision.

> > There's a bug report
> > in Debian (https://bugs.debian.org/895048) that requests that behavior
> > instead of the status quo, and also it's the behavior that's documented:
> 
> The last bit is a bit questionable; I think you are reading too much
> into the description.
> 
> A substitute for gpg.program MUST signal good (or not good)
> signature the same way as gpg would with its exit code---that is all
> the description says.  It does not say anything about how that exit
> code affects the exit status of "tag --verify" and friends that
> called gpg.program.

I agree that the description doesn't specifically say that.  In fact, it
doesn't explicitly say that it must exit nonzero on a bad signature,
although I agree with your interpretation that that would be logical
(and, AIUI, the behavior of gpg).

However, I would assert that we do want Git's verification to exit
successfully on a good signature and unsuccessfully on a bad signature,
and deferring to gpg may be the most robust way of ensuring that.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted

[Junio C Hamano]

"brian m. carlson"  writes:

>> FWIW, I'm on board with returning non-zero in any case where gpg would.
>
> I think that's probably the best solution overall.

FWIW, I am not married to the current behaviour.  I would not be
surprised if it mostly came by accident and not designed.

> There's a bug report
> in Debian (https://bugs.debian.org/895048) that requests that behavior
> instead of the status quo, and also it's the behavior that's documented:

The last bit is a bit questionable; I think you are reading too much
into the description.

A substitute for gpg.program MUST signal good (or not good)
signature the same way as gpg would with its exit code---that is all
the description says.  It does not say anything about how that exit
code affects the exit status of "tag --verify" and friends that
called gpg.program.

>gpg.program
>Use this custom program instead of "gpg" found on $PATH when
>making or verifying a PGP signature. The program must support
>the same command-line interface as GPG, namely, to verify a
>detached signature, "gpg --verify $file - <$signature" is
>run, and the program is expected to signal a good signature
>by exiting with code 0, and to generate an ASCII-armored
>detached signature, the standard input of "gpg -bsau $key" is
>fed with the contents to be signed, and the program is
>expected to send the result to its standard output.

Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs

[Brandon Williams]

On 08/08, Stefan Beller wrote:
> On Wed, Aug 8, 2018 at 3:33 PM Brandon Williams  wrote:
> >
> > Introduce a helper function "submodule_name_to_gitdir()" (and the
> > submodule--helper subcommand "gitdir") which constructs a path to a
> > submodule's gitdir, located in the provided repository's "modules"
> > directory.
> 
> Makes sense.
> 
> >
> > This consolidates the logic needed to build up a path into a
> > repository's "modules" directory, abstracting away the fact that
> > submodule git directories are stored in a repository's common gitdir.
> > This makes it easier to adjust how submodules gitdir are stored in the
> > "modules" directory in a future patch.
> 
> and yet, all places that we touch were and still are broken for old-style
> submodules that have their git directory inside the working tree?
> Do we need to pay attention to those, too?

This series only tries to address the issues with submodules stored in
$GITDIR/modules/ and places in our codebase that explicitly reference
submodules stored there.

For those old-old-style submodules, wouldn't the absorb submodule
functions handle that migration?

> 
> 
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 8b5ad59bde..053747d290 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> 
> > @@ -577,7 +578,7 @@ cmd_update()
> > die "$(eval_gettext "Unable to find current 
> > \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
> > fi
> >
> > -   if ! $(git config -f "$(git rev-parse 
> > --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
> > +   if ! $(git config -f "$(git submodule--helper gitdir 
> > "$name")/config" core.worktree) 2>/dev/null
> 
> This will collide with origin/sb/submodule-update-in-c specifically
> 1c866b9831d (submodule--helper: replace connect-gitdir-workingtree
> by ensure-core-worktree, 2018-08-03), but as that removes these lines,
> it should be easy to resolve the conflict.

-- 
Brandon Williams

Re: [PATCH 0/5] chainlint: improve robustness against "unusual" shell coding

[Junio C Hamano]

Jeff King  writes:

> I had two minor comments on the first patch. I'll admit my eyes glazed
> over looking at the rest of them, and to make any kind of intelligent
> review I'd need to spend an hour understanding how the sed script works.
> Which frankly, I'm not sure is worth it.

Didn't I make this prediction when we started the "text inspection"
approach that it quickly go downhill resulting in unmaintainable
mess rather quickly ;-)?

> Given the empirical results
> (both on the real code base and the new tests you add) and the low-risk
> nature (it's linting our tests, after all, not code users run), I'd be
> inclined to say it's not making anything worse, and probably making
> things better. We can find out about any further short-comings in the
> wild.

Amen to that.

Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems

[Junio C Hamano]

Jeff King  writes:

> I think we really want to avoid doing that normalization ourselves if we
> can. There are just too many filesystem-specific rules.

Exactly; not having to learn these rules is the major (if not whole)
point of the "let checkout notice the collision and then deal with
it" approach.  Let's not forget that.

> If we have an equivalence-class hashmap and feed it inodes (or again,
> some system equivalent) as the keys, we should get buckets of
> collisions.

I guess one way to get "some system equivalent" that can be used as
the last resort, when there absolutely is no inum equivalent, is to
rehash the working tree file that shouldn't be there when we detect
a collision.

If we found that there is something when we tried to write out
"Foo.txt", if we open "Foo.txt" on the working tree and hash-object
it, we should find the matching blob somewhere in the index _before_
"Foo.txt".  On a case-insensitive filesytem, it may well be
"foo.txt", but we do not even have to know "foo.txt" and "Foo.txt"
only differ in case.

Of course, that's really the last resort, as it would be costly, but
this is something that only need to happen on the "unusual" case in
the error codepath, so...

Re: [RFC PATCH] packfile: iterate packed objects in pack order

[Jeff King]

On Wed, Aug 08, 2018 at 04:12:10PM -0700, Jonathan Tan wrote:

> Many invocations of for_each_object_in_pack() and
> for_each_packed_object() (which invokes the former) subsequently check
> at least the type of the packed object, necessitating accessing the
> packfile itself. For locality reasons, it is thus better to iterate in
> pack order, instead of index order. Teach for_each_object_in_pack() to
> iterate in pack order by first creating a reverse index.
> 
> This is based off work by Jeff King.
> 
> Signed-off-by: Jonathan Tan 
> ---
> After writing this patch and looking at it further, I'm not sure if this
> is a clear benefit, but here's the patch anyway. In particular,
> builtin/fsck.c and builtin/cat-file.c just deal with the OID directly
> and does not access the packfile at all (at least at the time of
> invoking for_each_packed_object). And revision.c, if we are excluding
> promisor objects, parses each packed promisor object, but it seems
> possible to avoid doing that (replacing the parse_object() by
> lookup_unknown_object() still passes tests).

Even if you just use the oid to do a separate lookup in the object
database, there's still a benefit in accessing the objects in pack
order. The case in cat-file needs more than this, though, since it
separately sorts the output (it has to, because it has to merge and
de-dup the output from several packs plus loose objects).

With the patch below on top of yours, I get:

  $ time git.v2.18.0 cat-file --batch-all-objects --buffer --batch | wc -c
  6938365964

  real  0m44.686s
  user  0m42.932s
  sys   0m5.283s

  $ time git.compile cat-file --batch-all-objects --buffer --batch | wc -c
  8289859070

  real  0m7.007s
  user  0m5.542s
  sys   0m4.005s

But:

  - it needs to de-duplicate using a hashmap (which is why the output is
so much bigger in the second case)

  - it probably needs to be enabled explicitly by the user, since
cat-file is plumbing and callers may be relying on the existing sort
order

I can try to pick this up and carry the cat-file bits to completion if
you want, but probably not until tomorrow or Friday.

-Peff

Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs

[Stefan Beller]

On Wed, Aug 8, 2018 at 3:33 PM Brandon Williams  wrote:
>
> Introduce a helper function "submodule_name_to_gitdir()" (and the
> submodule--helper subcommand "gitdir") which constructs a path to a
> submodule's gitdir, located in the provided repository's "modules"
> directory.

Makes sense.

>
> This consolidates the logic needed to build up a path into a
> repository's "modules" directory, abstracting away the fact that
> submodule git directories are stored in a repository's common gitdir.
> This makes it easier to adjust how submodules gitdir are stored in the
> "modules" directory in a future patch.

and yet, all places that we touch were and still are broken for old-style
submodules that have their git directory inside the working tree?
Do we need to pay attention to those, too?


> diff --git a/git-submodule.sh b/git-submodule.sh
> index 8b5ad59bde..053747d290 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh

> @@ -577,7 +578,7 @@ cmd_update()
> die "$(eval_gettext "Unable to find current 
> \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
> fi
>
> -   if ! $(git config -f "$(git rev-parse 
> --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
> +   if ! $(git config -f "$(git submodule--helper gitdir 
> "$name")/config" core.worktree) 2>/dev/null

This will collide with origin/sb/submodule-update-in-c specifically
1c866b9831d (submodule--helper: replace connect-gitdir-workingtree
by ensure-core-worktree, 2018-08-03), but as that removes these lines,
it should be easy to resolve the conflict.

Re: abstracting commit signing/verify to support other signing schemes

[Jeff King]

On Mon, Aug 06, 2018 at 08:24:25PM +, Tacitus Aedifex wrote:

> the older patch set suggested the idea of using PEM strings to match up the
> signature payload with a certain signing tool.  i can't tell if they mean
> the 'pre-ecapsulation boundary' (e.g. '-BEGIN FOO-') or if they mean
> the encapsulated headers; both as defined in RFC 1421 [0].

It was the pre-encapsulation boundary (we didn't use that word, but it
was definitely the "-BEGIN" line ;) ).

And that was the sticking point: there was an open question of what
support for something like signify would look like exactly, and what the
matching rules would need to be. My thought was to allow multiple
matching types, and "PEM type" (by which I meant that pre-encapsulation
boundary) would be the first such type.

But that got punted on, since we didn't have a real-world example to
look at, and we really only cared about gpgsm in the near-term anyway.
And that obviously does PEM. So the gpg.* tools all require PEM, but if
we add a generic signingtool config, it doesn't have to.

> the newer patch set looks specifically at the pre-encapsulation boundary to
> switch behaviors. that works assuming that the signing tools all understand
> PEM. in the case of signify, it doesn't, so the driver code in git will have
> to translate to/from PEM.

Right. It might be fine to encapsulate it, though I prefer not inventing
new micro-formats if we can avoid it.

There actually _is_ an interesting micro-format already in Git, which is
that commit signatures (not tags) are held in the "gpgsig" header of the
commit. Which would be an awkward name for storing a non-gpg tool. We
may want to live with that for historical reasons, or we could switch to
a more generic name.

The actual PEM detection is for tags, where the signature is in the tag
body itself.

In either case, we could use some object header to indicate the
signature type (on the other hand, it could be possible to have multiple
signatures of different types).

> i suggest that we switch to a standard format for all signatures that is
> similar to the commit message format with colon ':' separated fields
> followed by a payload.  the colon separated fields would specify the signing
> tool used to generate the signature and the tool specific data followed by
> the signature blob like so:
> 
>  signing-tool: gpg
>  gpg-keyid: 0123456789ABCDEF
>  -BEGIN PGP SIGNATURE-
>  
>  -END PGP SIGNATURE-
> 
> by adopting this format, git will be fully abstracted from the underlying
> signing tool and the user can specify multiple signing tools in their config
> and git will be able to map the signature to the tool when verifying (e.g.
> git log --show-signature).

One problem with that for the signatures in tag bodies is that
"signing-tool: gpg" looks like something a human might right (as opposed
to the PEM boundary, which is a bit more obvious).

If we're going to make up a micro-format, it may be simpler to just have
something PEM-like in the first place, and shove signify into that.

> > So _if_ we knew what it would take to support signify, we could
> > potentially adjust what's going into 2.19 in order to skip straight to
> > the more generic interface. But on the OTOH, it may not be worth
> > rushing, and there is already a vague plan of how the gpg..*
> > config would interact with a more generic config.
> 
> there's no rush, but i would prefer that the newer patch set get changed to
> use the more generic 'signingtool..*' instead of 'gpg..*'. if
> you all agree, i'll follow up with a patch to change that part of what is
> going into 2.19.

I'm on the fence for that myself. The best way to get people to comment
would be to make a patch, and cc people involved in the earlier
discussions.

-Peff

Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted

[brian m. carlson]

On Wed, Aug 08, 2018 at 07:04:56PM -0400, Jeff King wrote:
> On Sat, Aug 04, 2018 at 10:43:46AM +0200, Karel Kočí wrote:
> > I have a solution for my problem (calling git verify-* twice and grep). 
> > That is
> > not the point of this email nor this contribution. The point is that 
> > although
> > GPG's behavior of exiting with 0 code when trust level is unknown is 
> > unexpected
> > but in the end understandable, git's behavior of exiting with 0 code even 
> > if key
> > is explicitly untrusted is just counterintuitive. I think that few people 
> > are
> > still going to get nasty surprise when I consider that this change was 
> > introduced
> > mid 2014 just after v2.4.0 and Ubuntu 14.04 lts (running even on part of our
> > infrastructure) still contains version 1.9.1 and in that release it was
> > acknowledging GPG exit code.
> 
> FWIW, I'm on board with returning non-zero in any case where gpg would.

I think that's probably the best solution overall.  There's a bug report
in Debian (https://bugs.debian.org/895048) that requests that behavior
instead of the status quo, and also it's the behavior that's documented:

   gpg.program
   Use this custom program instead of "gpg" found on $PATH when
   making or verifying a PGP signature. The program must support
   the same command-line interface as GPG, namely, to verify a
   detached signature, "gpg --verify $file - <$signature" is
   run, and the program is expected to signal a good signature
   by exiting with code 0, and to generate an ASCII-armored
   detached signature, the standard input of "gpg -bsau $key" is
   fed with the contents to be signed, and the program is
   expected to send the result to its standard output.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[RFC PATCH] packfile: iterate packed objects in pack order

[Jonathan Tan]

Many invocations of for_each_object_in_pack() and
for_each_packed_object() (which invokes the former) subsequently check
at least the type of the packed object, necessitating accessing the
packfile itself. For locality reasons, it is thus better to iterate in
pack order, instead of index order. Teach for_each_object_in_pack() to
iterate in pack order by first creating a reverse index.

This is based off work by Jeff King.

Signed-off-by: Jonathan Tan 
---
After writing this patch and looking at it further, I'm not sure if this
is a clear benefit, but here's the patch anyway. In particular,
builtin/fsck.c and builtin/cat-file.c just deal with the OID directly
and does not access the packfile at all (at least at the time of
invoking for_each_packed_object). And revision.c, if we are excluding
promisor objects, parses each packed promisor object, but it seems
possible to avoid doing that (replacing the parse_object() by
lookup_unknown_object() still passes tests).
---
 packfile.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/packfile.c b/packfile.c
index 6974903e5..371b64e9b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -15,6 +15,7 @@
 #include "tree-walk.h"
 #include "tree.h"
 #include "object-store.h"
+#include "pack-revindex.h"
 
 char *odb_pack_name(struct strbuf *buf,
const unsigned char *sha1,
@@ -1890,14 +1891,17 @@ int for_each_object_in_pack(struct packed_git *p, 
each_packed_object_fn cb, void
uint32_t i;
int r = 0;
 
+   load_pack_revindex(p);
+
for (i = 0; i < p->num_objects; i++) {
+   uint32_t pack_nr = p->revindex[i].nr;
struct object_id oid;
 
-   if (!nth_packed_object_oid(, p, i))
+   if (!nth_packed_object_oid(, p, pack_nr))
return error("unable to get sha1 of object %u in %s",
-i, p->pack_name);
+pack_nr, p->pack_name);
 
-   r = cb(, p, i, data);
+   r = cb(, p, pack_nr, data);
if (r)
break;
}
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted

[Jeff King]

On Sat, Aug 04, 2018 at 10:43:46AM +0200, Karel Kočí wrote:

> > I think the only sensible thing is to err on the conservative side, and
> > return non-zero if we saw _any_ invalid signature.
> > 
> > I will note, though, that just checking the exit code of `verify-tag`
> > isn't really that thorough. It shows that there was _a_ signature, but
> > we don't know:
> > 
> >   - if it was an identity the user would expect to be signing tags
> > 
> >   - if it even matches the refname we used to find the tag
> > 
> > So I'd argue that any real verification needs to either have a human in
> > the loop, or implement a custom policy based on reading the full output.
> > 
> > I know we (and you specifically Santiago) talked about this a while ago,
> > and we ended up providing ways to get more information out of
> > verify-tag, so that a tool could sit on top of that and implement more
> > project-specific policy. I don't know offhand of any reusable tools that
> > do so, though.
> 
> I think that it would be even legit to exit on first tag verification 
> failure. If
> someone wants to really verify all tags then it can be done with simple for 
> loop.
> git that way does not have to solve problem of error combination.

Yeah, I'd be fine with that.

> >   - if it was an identity the user would expect to be signing tags
> That can be done just by using trust levels.

I may be showing my PGP cluelessness here, but I thought trust levels
were about saying "to what degree do I think this uid matches this key".
Or are you proposing feeding gpg a fixed trust-db pre-seeded with keys
that are allowed to sign?

I suppose that would work, but:

  - is it much easier than just verifying the uid from gpg output
against a trusted list?

  - it mixes authentication and authorization. I.e., you lose the
ability to know about a case of "yes, this signature is valid by
this person, but they are not an authorized tagger".

Definitely for some use cases that is fine (and easier still is to just
not even have disallowed keys in your keyring). But I don't think it's a
general solution.

> >   - if it even matches the refname we used to find the tag
> Can you explain this more? You mean that string (such as v1.1) used to lookup 
> tag
> object is not verified as part of that object?

Yes. The signature is just over the tag object contents itself. That
object does contain a "tag" field, but it may or may not be the tag
you're expecting. Git _could_ confirm that when you looked up a tag via
refs/tags/v1.1 that the tag object we found has "tag v1.1" in it. But
that's not always possible (you might feed Git a resolved name, for
example).

> OK I thing that it was enough of abstract concepts from me. Let me explain you
> what am I trying to achieve. I am implementing feeds (in other words git
> repositories with packages) and package sources verification for OpenWRT. We
> (project Turris by CZ.NIC) are signing all our commits and all our tags. Now 
> we
> are using small script that is verifying our repositories just before we run
> build. That is against keyring maintained on our server. I am trying to extend
> that to whole OpenWRT tree. That introduces problem of having a lot of keys 
> and a
> lot of packages sharing same allowed keys. Fetching all allowed keys for every
> package from key servers is just slow because of that I have to share those
> between packages. In general there are two options. First one is to have 
> cache of
> already fetched keys in armor format. Second one is to have one keyring and by
> setting all keys explicitly as never trusted with package given exception.
> Unfortunately first option can't be used because of one other request that is 
> from
> our team. We don't want to be forced to update list of allowed contributors 
> to our
> projects every time we have new colleague. Solution we come up with is to have
> central PGP key that signs our whole team and then verification is done by
> allowing GPG to fetch additional keys with max-cert-depth 1. That brings me 
> to git
> verify-commit/tag that won't exit with zero code when signature is not 
> trusted.

OK, that makes some sense (and I guess answers my "how would you use the
trustdb" question above).

> I have a solution for my problem (calling git verify-* twice and grep). That 
> is
> not the point of this email nor this contribution. The point is that although
> GPG's behavior of exiting with 0 code when trust level is unknown is 
> unexpected
> but in the end understandable, git's behavior of exiting with 0 code even if 
> key
> is explicitly untrusted is just counterintuitive. I think that few people are
> still going to get nasty surprise when I consider that this change was 
> introduced
> mid 2014 just after v2.4.0 and Ubuntu 14.04 lts (running even on part of our
> infrastructure) still contains version 1.9.1 and in that release it was
> acknowledging GPG exit code.

FWIW, I'm on board with returning non-zero in any case where gpg 

Re: [PATCH 0/5] chainlint: improve robustness against "unusual" shell coding

[Jeff King]

On Tue, Aug 07, 2018 at 04:21:30AM -0400, Eric Sunshine wrote:

> This series improves chainlint's robustness when faced with the sort of
> unusual shell coding in contrib/subtree/t7900 which triggered a
> false-positive, as reported by Jonathan[1]. Jonathan has already
> rewritten[2] that code to be cleaner and more easily understood (and,
> consequently, to avoid triggering the false-positive), thus the
> improvements in this series are not strictly necessary.
> 
> Nevertheless, it seems prudent to make chainlint more robust against
> such unusual coding as an aid to future less-experienced test writers,
> making it less likely for them to trigger a false-positive and waste
> time trying to decipher a non-existent problem (in their code).
> 
> In [3], I said that 'sed' couldn't "be coerced" into dealing with nested
> here-docs with arbitrary tag names (explaining why it recognized only a
> "blessed" set of hard-coded names). However, I put a bit of thought into
> it and figured out how to do it. Patch 1/5 is the result.
> 
> This applies atop 'master'.

I had two minor comments on the first patch. I'll admit my eyes glazed
over looking at the rest of them, and to make any kind of intelligent
review I'd need to spend an hour understanding how the sed script works.
Which frankly, I'm not sure is worth it. Given the empirical results
(both on the real code base and the new tests you add) and the low-risk
nature (it's linting our tests, after all, not code users run), I'd be
inclined to say it's not making anything worse, and probably making
things better. We can find out about any further short-comings in the
wild.

-Peff

Re: [PATCH 1/5] chainlint: match arbitrary here-docs tags rather than hard-coded names

[Jeff King]

On Tue, Aug 07, 2018 at 04:21:31AM -0400, Eric Sunshine wrote:

> diff --git a/t/chainlint.sed b/t/chainlint.sed
> index 5f0882cb38..bd76c5d181 100644
> --- a/t/chainlint.sed
> +++ b/t/chainlint.sed
> @@ -61,6 +61,22 @@
>  # "else", and "fi" in if-then-else likewise must not end with "&&", thus
>  # receives similar treatment.
>  #
> +# Swallowing here-docs with arbitrary tags requires a bit of finesse. When a
> +# line such as "cat  front
> +# of the line enclosed in angle brackets as a sentinel, giving "cat 
> >out".
> +# As each subsequent line is read, it is appended to the target line and a
> +# (whitespace-loose) back-reference match /^<(.*)>\n\1$/ is attempted to see 
> if
> +# the content inside "<...>" matches the entirety of the newly-read line. For
> +# instance, if the next line read is "some data", when concatenated with the
> +# target line, it becomes "cat >out\nsome data", and a match is 
> attempted
> +# to see if "EOF" matches "some data". Since it doesn't, the next line is
> +# attempted. When a line consisting of only "EOF" (and possible whitespace) 
> is
> +# encountered, it is appended to the target line giving "cat >out\nEOF",
> +# in which case the "EOF" inside "<...>" does match the text following the
> +# newline, thus the closing here-doc tag has been found. The closing tag line
> +# and the "<...>" prefix on the target line are then discarded, leaving just
> +# the target line "cat >out".

Gross, but OK, as long as we would not get confused by a line that
actually started with  at the start.

> +/<<[ ]*[-\\]*[A-Z0-9_][A-Z0-9_]*/ {
> + s/^\(.*\)<<[]*[-\\]*\([A-Z0-9_][A-Z0-9_]*\)/<\2>\1< + s/[ ]*<

Re: [PATCH] update-index: there no longer is `apply --index-info`

[Jeff King]

On Wed, Aug 08, 2018 at 02:35:18PM -0700, Junio C Hamano wrote:

> Back when we removed `git apply --index-info` in 2007, we forgot to
> adjust the documentation for update-index that reads its output.
> 
> Let's reorder the description of three formats to present the other
> two formats that are still generated by git commands before this
> format, and stop mentioning `git apply --index-info`.

Yep, this makes sense.

> ---
>  Documentation/git-update-index.txt | 15 ++-
>  1 file changed, 6 insertions(+), 9 deletions(-)

For fun, I tried out my "doc-diff" on it, and it looks good. :)

-Peff

Re: What's cooking in git.git (Aug 2018, #02; Mon, 6)

[Junio C Hamano]

Junio C Hamano  writes:

> Hmph, it came from this message (most headers omitted)
>
> To: =?iso-8859-1?Q?=C6var_Arnfj=F6r=F0?= Bjarmason 
> Message-ID: <20180804085247.ge55...@aiede.svl.corp.google.com>
> Content-Type: text/plain; charset=iso-8859-1
> Content-Disposition: inline
> Content-Transfer-Encoding: 8bit
>
> Subject: doc hash-function-transition: pick SHA-256 as NewHash
>
> ...
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
>
> and the body seems to be correct iso-8859-1.  "od -cx" tells me that
> the file stores 0xf0 for that D looking thing, for example.  Could it
> be mailinfo that screws up, I wonder.  A quick check with "git mailinfo"
> does not give me anything suspicous---the info contents emitted to
> its standard output is correctly converted to UTF-8.  Puzzled...
>
> I read from public-inbox/git over nntp, if that matters.

Just to close the loop, this turned out to be caused by my use of
Gnus/Emacs.

You can stop reading if you are not interested in reading and applying
patches from inside Gnus.

I am used to type '|' (gnus-summary-pipe-output) to pipe the current
article into "git am -s[c3]", and it works fine when the payload is
UTF-8.  But the command decodes, and strips certain e-mail headers,
before feeding it to the command.

While the payload is converted to UTF-8 (presumably because that is
what I use by default), "Content-type" is *not* among the e-mail
headers that are stripped, so "am" ends up seeing UTF-8 bytestream
that (still) claims to be "iso-8859-1" when processing the above
message.

I need to get used to typing M-i r | (that is, to use the 'r'
"symbolic prefix") to force the message piped as-is to the command.

Again, thanks for noticing and giving me a chance to correct the
result before it got too late.

[PATCH v2 1/2] repack: refactor setup of pack-objects cmd

[Jonathan Tan]

A subsequent patch will teach repack to run pack-objects with some same
and some different arguments if repacking of promisor objects is
required. Refactor the setup of the pack-objects cmd so that setting up
the arguments common to both is done in a function.

Signed-off-by: Jonathan Tan 
---
 builtin/repack.c | 99 +++-
 1 file changed, 55 insertions(+), 44 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 6c636e159..f8cae7d66 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -138,6 +138,47 @@ static void remove_redundant_pack(const char *dir_name, 
const char *base_name)
strbuf_release();
 }
 
+struct pack_objects_args {
+   const char *window;
+   const char *window_memory;
+   const char *depth;
+   const char *threads;
+   const char *max_pack_size;
+   int no_reuse_delta;
+   int no_reuse_object;
+   int quiet;
+   int local;
+};
+
+static void prepare_pack_objects(struct child_process *cmd,
+const struct pack_objects_args *args)
+{
+   argv_array_push(>args, "pack-objects");
+   if (args->window)
+   argv_array_pushf(>args, "--window=%s", args->window);
+   if (args->window_memory)
+   argv_array_pushf(>args, "--window-memory=%s", 
args->window_memory);
+   if (args->depth)
+   argv_array_pushf(>args, "--depth=%s", args->depth);
+   if (args->threads)
+   argv_array_pushf(>args, "--threads=%s", args->threads);
+   if (args->max_pack_size)
+   argv_array_pushf(>args, "--max-pack-size=%s", 
args->max_pack_size);
+   if (args->no_reuse_delta)
+   argv_array_pushf(>args, "--no-reuse-delta");
+   if (args->no_reuse_object)
+   argv_array_pushf(>args, "--no-reuse-object");
+   if (args->local)
+   argv_array_push(>args,  "--local");
+   if (args->quiet)
+   argv_array_push(>args,  "--quiet");
+   if (delta_base_offset)
+   argv_array_push(>args,  "--delta-base-offset");
+   argv_array_push(>args, packtmp);
+   cmd->git_cmd = 1;
+   cmd->out = -1;
+}
+
 #define ALL_INTO_ONE 1
 #define LOOSEN_UNREACHABLE 2
 
@@ -165,15 +206,9 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
int delete_redundant = 0;
const char *unpack_unreachable = NULL;
int keep_unreachable = 0;
-   const char *window = NULL, *window_memory = NULL;
-   const char *depth = NULL;
-   const char *threads = NULL;
-   const char *max_pack_size = NULL;
struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
-   int no_reuse_delta = 0, no_reuse_object = 0;
int no_update_server_info = 0;
-   int quiet = 0;
-   int local = 0;
+   struct pack_objects_args po_args = {NULL};
 
struct option builtin_repack_options[] = {
OPT_BIT('a', NULL, _everything,
@@ -183,14 +218,14 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
   LOOSEN_UNREACHABLE | ALL_INTO_ONE),
OPT_BOOL('d', NULL, _redundant,
N_("remove redundant packs, and run 
git-prune-packed")),
-   OPT_BOOL('f', NULL, _reuse_delta,
+   OPT_BOOL('f', NULL, _args.no_reuse_delta,
N_("pass --no-reuse-delta to 
git-pack-objects")),
-   OPT_BOOL('F', NULL, _reuse_object,
+   OPT_BOOL('F', NULL, _args.no_reuse_object,
N_("pass --no-reuse-object to 
git-pack-objects")),
OPT_BOOL('n', NULL, _update_server_info,
N_("do not run git-update-server-info")),
-   OPT__QUIET(, N_("be quiet")),
-   OPT_BOOL('l', "local", ,
+   OPT__QUIET(_args.quiet, N_("be quiet")),
+   OPT_BOOL('l', "local", _args.local,
N_("pass --local to git-pack-objects")),
OPT_BOOL('b', "write-bitmap-index", _bitmaps,
N_("write bitmap index")),
@@ -198,15 +233,15 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
N_("with -A, do not loosen objects older than 
this")),
OPT_BOOL('k', "keep-unreachable", _unreachable,
N_("with -a, repack unreachable objects")),
-   OPT_STRING(0, "window", , N_("n"),
+   OPT_STRING(0, "window", _args.window, N_("n"),
N_("size of the window used for delta 
compression")),
-   OPT_STRING(0, "window-memory", _memory, N_("bytes"),
+   OPT_STRING(0, "window-memory", _args.window_memory, 
N_("bytes"),
N_("same as the above, but limit memory size 
instead of entries count")),
-   OPT_STRING(0, 

[PATCH v2 0/2] Repacking of promisor packfiles

[Jonathan Tan]

Changes from v1:
 - add NEEDSWORK stating that input to pack-objects could be removed
 - run pack-objects to repack promisor objects only if there is at least
   one of them - this exposed a possible bug where the later part of
   cmd_repack() requires a correct packed_git (this was mostly noticed
   by tests that deal with corrupt packfiles), so insert a call to
   reprepare_packed_git() after all packfile manipulation is done
 - rename prepare_packed_objects() to prepare_pack_objects() to better
   reflect that we are preparing a call to pack-objects, not preparing
   existing packed objects

Peff raised the possibility of making for_each_packed_object() use pack
order instead of index order - I'll also take a look at that, separately
from this patch series.

Jonathan Tan (2):
  repack: refactor setup of pack-objects cmd
  repack: repack promisor objects if -a or -A is set

 Documentation/git-repack.txt |   5 +
 builtin/repack.c | 182 ++-
 t/t0410-partial-clone.sh |  85 +---
 3 files changed, 213 insertions(+), 59 deletions(-)

-- 
2.18.0.597.ga71716f1ad-goog

[PATCH v2 2/2] repack: repack promisor objects if -a or -A is set

[Jonathan Tan]

Currently, repack does not touch promisor packfiles at all, potentially
causing the performance of repositories that have many such packfiles to
drop. Therefore, repack all promisor objects if invoked with -a or -A.

This is done by an additional invocation of pack-objects on all promisor
objects individually given, which takes care of deduplication and allows
the resulting packfiles to respect flags such as --max-pack-size.

Signed-off-by: Jonathan Tan 
---
 Documentation/git-repack.txt |  5 +++
 builtin/repack.c | 83 +--
 t/t0410-partial-clone.sh | 85 +++-
 3 files changed, 158 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index d90e7907f..d05625096 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -40,6 +40,11 @@ OPTIONS
 Note that users fetching over dumb protocols will have to fetch the
 whole new pack in order to get any contained object, no matter how many
 other objects in that pack they already have locally.
++
+Promisor packfiles are repacked separately: if there are packfiles that
+have an associated ".promisor" file, these packfiles will be repacked
+into another separate pack, and an empty ".promisor" file corresponding
+to the new separate pack will be written.
 
 -A::
Same as `-a`, unless `-d` is used.  Then any unreachable
diff --git a/builtin/repack.c b/builtin/repack.c
index f8cae7d66..5c97dec3d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -8,6 +8,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 #include "argv-array.h"
+#include "packfile.h"
 
 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;
@@ -83,7 +84,7 @@ static void remove_pack_on_signal(int signo)
 
 /*
  * Adds all packs hex strings to the fname list, which do not
- * have a corresponding .keep or .promisor file. These packs are not to
+ * have a corresponding .keep file. These packs are not to
  * be kept if we are going to pack everything into one file.
  */
 static void get_non_kept_pack_filenames(struct string_list *fname_list,
@@ -111,8 +112,7 @@ static void get_non_kept_pack_filenames(struct string_list 
*fname_list,
 
fname = xmemdupz(e->d_name, len);
 
-   if (!file_exists(mkpath("%s/%s.keep", packdir, fname)) &&
-   !file_exists(mkpath("%s/%s.promisor", packdir, fname)))
+   if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
string_list_append_nodup(fname_list, fname);
else
free(fname);
@@ -122,7 +122,7 @@ static void get_non_kept_pack_filenames(struct string_list 
*fname_list,
 
 static void remove_redundant_pack(const char *dir_name, const char *base_name)
 {
-   const char *exts[] = {".pack", ".idx", ".keep", ".bitmap"};
+   const char *exts[] = {".pack", ".idx", ".keep", ".bitmap", ".promisor"};
int i;
struct strbuf buf = STRBUF_INIT;
size_t plen;
@@ -179,6 +179,76 @@ static void prepare_pack_objects(struct child_process *cmd,
cmd->out = -1;
 }
 
+/*
+ * Write oid to the given struct child_process's stdin, starting it first if
+ * necessary.
+ */
+static int write_oid(const struct object_id *oid, struct packed_git *pack,
+uint32_t pos, void *data)
+{
+   struct child_process *cmd = data;
+
+   if (cmd->in == -1) {
+   if (start_command(cmd))
+   die("Could not start pack-objects to repack promisor 
objects");
+   }
+
+   xwrite(cmd->in, oid_to_hex(oid), GIT_SHA1_HEXSZ);
+   xwrite(cmd->in, "\n", 1);
+   return 0;
+}
+
+static void repack_promisor_objects(const struct pack_objects_args *args,
+   struct string_list *names)
+{
+   struct child_process cmd = CHILD_PROCESS_INIT;
+   FILE *out;
+   struct strbuf line = STRBUF_INIT;
+
+   prepare_pack_objects(, args);
+   cmd.in = -1;
+
+   /*
+* NEEDSWORK: Giving pack-objects only the OIDs without any ordering
+* hints may result in suboptimal deltas in the resulting pack. See if
+* the OIDs can be sent with fake paths such that pack-objects can use a
+* {type -> existing pack order} ordering when computing deltas instead
+* of a {type -> size} ordering, which may produce better deltas.
+*/
+   for_each_packed_object(write_oid, ,
+  FOR_EACH_OBJECT_PROMISOR_ONLY);
+
+   if (cmd.in == -1)
+   /* No packed objects; cmd was never started */
+   return;
+
+   close(cmd.in);
+
+   out = xfdopen(cmd.out, "r");
+   while (strbuf_getline_lf(, out) != EOF) {
+   char *promisor_name;
+   int fd;
+   if (line.len != 40)
+   die("repack: Expecting 40 character sha1 lines only 
from pack-objects.");
+

[PATCH 2/2] submodule: munge paths to submodule git directories

[Brandon Williams]

Commit 0383bbb901 (submodule-config: verify submodule names as paths,
2018-04-30) introduced some checks to ensure that submodule names don't
include directory traversal components (e.g. "../").

This addresses the vulnerability identified in 0383bbb901 but the root
cause is that we use submodule names to construct paths to the
submodule's git directory.  What we really should do is munge the
submodule name before using it to construct a path.

Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url
encoding it) before using it to build a path to the submodule's gitdir.

Signed-off-by: Brandon Williams 
---
 submodule.c | 14 ++
 t/t7400-submodule-basic.sh  | 32 +++-
 t/t7406-submodule-update.sh | 21 ++---
 3 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/submodule.c b/submodule.c
index 24eced34e7..4854d88ce8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1965,6 +1965,20 @@ int submodule_to_gitdir(struct strbuf *buf, const char 
*submodule)
 void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,
  const char *submodule_name)
 {
+   size_t modules_len;
+
strbuf_git_common_path(buf, r, "modules/");
+   modules_len = buf->len;
strbuf_addstr(buf, submodule_name);
+
+   /*
+* If the submodule gitdir already exists using the old-fashioned
+* location (which uses the submodule name as-is, without munging it)
+* then return that.
+*/
+   if (!access(buf->buf, F_OK))
+   return;
+
+   strbuf_setlen(buf, modules_len);
+   strbuf_addstr_urlencode(buf, submodule_name, 1);
 }
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2c2c97e144..963693332c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -933,7 +933,7 @@ test_expect_success 'recursive relative submodules stay 
relative' '
cd clone2 &&
git submodule update --init --recursive &&
echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect &&
-   echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" 
>./sub3/dirdir/subsub/.git_expect
+   echo "gitdir: 
../../../.git/modules/sub3/modules/dirdir%2fsubsub" 
>./sub3/dirdir/subsub/.git_expect
) &&
test_cmp clone2/sub3/.git_expect clone2/sub3/.git &&
test_cmp clone2/sub3/dirdir/subsub/.git_expect 
clone2/sub3/dirdir/subsub/.git
@@ -1324,4 +1324,34 @@ test_expect_success 'recursive clone respects -q' '
test_must_be_empty actual
 '
 
+test_expect_success 'resolve submodule gitdir in superprojects modules 
directory' '
+   test_when_finished "rm -rf superproject submodule" &&
+
+   # Create a superproject with a submodule which contains a "/"
+   test_create_repo submodule &&
+   test_commit -C submodule one &&
+   test_create_repo superproject &&
+   git -C superproject submodule add ../submodule sub/module &&
+   git -C superproject commit -m "add submodule" &&
+
+   # "/" characters in submodule names are properly urlencoded before
+   # being used to construct a path to the submodules gitdir.
+   cat >expect <<-EOF &&
+   $(git -C superproject rev-parse --git-common-dir)/modules/sub%2fmodule
+   EOF
+   git -C superproject submodule--helper gitdir "sub/module" >actual &&
+   test_cmp expect actual &&
+   test_path_is_dir "superproject/.git/modules/sub%2fmodule" &&
+
+   # Test the old-fashioned way of storing submodules in the
+   # "modules" directory by directly renaming the submodules gitdir
+   mkdir superproject/.git/modules/sub/ &&
+   mv superproject/.git/modules/sub%2fmodule 
superproject/.git/modules/sub/module &&
+   cat >expect <<-EOF &&
+   $(git -C superproject rev-parse --git-common-dir)/modules/sub/module
+   EOF
+   git -C superproject submodule--helper gitdir "sub/module" >actual &&
+   test_cmp expect actual
+'
+
 test_done
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a72..fb744c5c39 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -777,12 +777,8 @@ test_expect_success 'submodule add places git-dir in 
superprojects git-dir' '
(cd super &&
 mkdir deeper &&
 git submodule add ../submodule deeper/submodule &&
-(cd deeper/submodule &&
- git log > ../../expected
-) &&
-(cd .git/modules/deeper/submodule &&
- git log > ../../../../actual
-) &&
+git -C deeper/submodule log >expected &&
+git -C .git/modules/deeper%2fsubmodule log >actual &&
 test_cmp actual expected
)
 '
@@ -795,12 +791,9 @@ test_expect_success 'submodule update places git-dir in 
superprojects git-dir' '
(cd super2 &&
 git submodule init deeper/submodule &&
 git 

[PATCH 1/2] submodule: create helper to build paths to submodule gitdirs

[Brandon Williams]

Introduce a helper function "submodule_name_to_gitdir()" (and the
submodule--helper subcommand "gitdir") which constructs a path to a
submodule's gitdir, located in the provided repository's "modules"
directory.

This consolidates the logic needed to build up a path into a
repository's "modules" directory, abstracting away the fact that
submodule git directories are stored in a repository's common gitdir.
This makes it easier to adjust how submodules gitdir are stored in the
"modules" directory in a future patch.

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c  | 28 +++--
 dir.c|  2 +-
 git-submodule.sh |  7 +++--
 repository.c |  3 +-
 submodule.c  | 53 
 submodule.h  |  7 +
 t/t7410-submodule-checkout-to.sh |  6 ++--
 7 files changed, 75 insertions(+), 31 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a3c4564c6c..5bfd2d0be9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -906,6 +906,21 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+static int module_gitdir(int argc, const char **argv, const char *prefix)
+{
+   struct strbuf gitdir = STRBUF_INIT;
+
+   if (argc != 2)
+   usage(_("git submodule--helper gitdir "));
+
+   submodule_name_to_gitdir(, the_repository, argv[1]);
+
+   printf("%s\n", gitdir.buf);
+
+   strbuf_release();
+   return 0;
+}
+
 struct sync_cb {
const char *prefix;
unsigned int flags;
@@ -1268,18 +1283,24 @@ static int add_possible_reference_from_superproject(
 * standard layout with .git/(modules/)+/objects
 */
if (ends_with(alt->path, "/objects")) {
+   struct repository alternate;
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
strbuf_add(, alt->path, strlen(alt->path) - 
strlen("objects"));
 
+   repo_init(, sb.buf, NULL);
+
/*
 * We need to end the new path with '/' to mark it as a dir,
 * otherwise a submodule name containing '/' will be broken
 * as the last part of a missing submodule reference would
 * be taken as a file name.
 */
-   strbuf_addf(, "modules/%s/", sas->submodule_name);
+   strbuf_reset();
+   submodule_name_to_gitdir(, , sas->submodule_name);
+   strbuf_addch(, '/');
+   repo_clear();
 
sm_alternate = compute_alternate_path(sb.buf, );
if (sm_alternate) {
@@ -1392,7 +1413,7 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
usage_with_options(git_submodule_helper_usage,
   module_clone_options);
 
-   strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
+   submodule_name_to_gitdir(, the_repository, name);
sm_gitdir = absolute_pathdup(sb.buf);
strbuf_reset();
 
@@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const 
char **argv, const char *p
name = argv[1];
path = argv[2];
 
-   strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
+   submodule_name_to_gitdir(, the_repository, name);
sm_gitdir = absolute_pathdup(sb.buf);
 
connect_work_tree_and_git_dir(path, sm_gitdir, 0);
@@ -2040,6 +2061,7 @@ struct cmd_struct {
 static struct cmd_struct commands[] = {
{"list", module_list, 0},
{"name", module_name, 0},
+   {"gitdir", module_gitdir, 0},
{"clone", module_clone, 0},
{"update-clone", update_clone, 0},
{"connect-gitdir-workingtree", connect_gitdir_workingtree, 0},
diff --git a/dir.c b/dir.c
index 21e6f2520a..7a9827ea4b 100644
--- a/dir.c
+++ b/dir.c
@@ -3053,7 +3053,7 @@ static void connect_wt_gitdir_in_nested(const char 
*sub_worktree,
strbuf_reset(_wt);
strbuf_reset(_gd);
strbuf_addf(_wt, "%s/%s", sub_worktree, sub->path);
-   strbuf_addf(_gd, "%s/modules/%s", sub_gitdir, sub->name);
+   submodule_name_to_gitdir(_gd, , sub->name);
 
connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 1);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index 8b5ad59bde..053747d290 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -252,12 +252,13 @@ Use -f if you really want to add it." >&2
fi
 
else
-   if test -d ".git/modules/$sm_name"
+   sm_gitdir="$(git submodule--helper gitdir "$sm_name")"
+   if test -d "$sm_gitdir"
then
if test -z "$force"
then
   

[PATCH 0/2] munge submodule names

[Brandon Williams]

Here's a more polished series taking into account some of the feedback
on the RFC.  As Junio pointed out URL encoding makes the directories
much more human readable, but I'm open to other ideas if we don't think
URL encoding is the right thing to do.

Brandon Williams (2):
  submodule: create helper to build paths to submodule gitdirs
  submodule: munge paths to submodule git directories

 builtin/submodule--helper.c  | 28 +++--
 dir.c|  2 +-
 git-submodule.sh |  7 ++--
 repository.c |  3 +-
 submodule.c  | 67 ++--
 submodule.h  |  7 
 t/t7400-submodule-basic.sh   | 32 ++-
 t/t7406-submodule-update.sh  | 21 +++---
 t/t7410-submodule-checkout-to.sh |  6 ++-
 9 files changed, 126 insertions(+), 47 deletions(-)

-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems

[Jeff King]

On Wed, Aug 08, 2018 at 03:48:04PM -0400, Jeff Hostetler wrote:

> > ce_match_stat() may not be a very good measure to see if two paths
> > refer to the same file, though.  After a fresh checkout, I would not
> > be surprised if two completely unrelated paths have the same size
> > and have same mtime/ctime.  In its original use case, i.e. "I have
> > one specific path in mind and took a snapshot of its metadata
> > earlier.  Is it still the same, or has it been touched?", that may
> > be sufficient to detect that the path has been _modified_, but
> > without reliable inum, it may be a poor measure to say two paths
> > refer to the same.
> 
> I agree with Junio on this one.  The mtime values are sloppy at best.
> On FAT file systems, they have 2 second resolution.  Even NTFS IIRC
> has only 100ns resolution, so we might get a lot of false matches
> using this technique, right?

Yeah, I think anything less than inode (or some system equivalent) is
going to be too flaky.

> It might be better to build an equivalence-class hash-map for the
> colliding entries.  Compute a "normalized" version of the pathname
> (such as convert to lowercase, strip final-dots/spaces, strip the
> digits following tilda of a shortname, and etc for the MAC's UTF-isms).
> Then when you rescan the index entries to find the matches, apply the
> equivalence operator on the pathname and do the hashmap lookup.
> When you find a match, you have a "potential" collider pair (I say
> potential only because of the ambiguity of shortnames).  Then we
> can use inum/file-index/whatever to see if they actually collide.

I think we really want to avoid doing that normalization ourselves if we
can. There are just too many filesystem-specific rules.

If we have an equivalence-class hashmap and feed it inodes (or again,
some system equivalent) as the keys, we should get buckets of
collisions. I started to write a "something like this..." earlier, but
got bogged down in boilerplate around the C hashmap.

But here it is in perl. ;)

-- >8 --
# pretend we have these paths in our index
paths='foo FOO and some other paths'

# create them; this will make a single path on a case-insensitive system
for i in $paths; do
  echo $i >$i
done

# now find the duplicates
perl -le '
  for my $path (@ARGV) {
# this would be an ntfs unique-id on Windows
my $inode = (lstat($path))[1];
push @{$h{$inode}}, $path;
  }

  for my $group (grep { @$_ > 1 } values(%h)) {
print "group:";
print "  ", $_ for (@$group);
  }
' $paths
-- >8 --

which should show the obvious pair (it does for me on vfat-on-linux,
though where it gets those inodes from, I have no idea ;) ).

-Peff

Re: [PATCH 1/1] pull --rebase=: allow single-letter abbreviations for the type

[Junio C Hamano]

Junio C Hamano  writes:

> Ævar Arnfjörð Bjarmason  writes:
>
>>> -   else if (!strcmp(value, "preserve"))
>>> +   else if (!strcmp(value, "preserve") || !strcmp(value, "p"))
>>> return REBASE_PRESERVE;
>>> -   else if (!strcmp(value, "merges"))
>>> +   else if (!strcmp(value, "merges") || !strcmp(value, "m"))
>>> return REBASE_MERGES;
>>> -   else if (!strcmp(value, "interactive"))
>>> +   else if (!strcmp(value, "interactive") || !strcmp(value, "i"))
>>> return REBASE_INTERACTIVE;
>>
>> Here 3 special cases are added...
>> ...
>>> +test_expect_success 'pull --rebase=i' '
>>> ...
>>> +'
>>> +
>>>  test_expect_success 'pull.rebase=invalid fails' '
>>> git reset --hard before-preserve-rebase &&
>>> test_config pull.rebase invalid &&
>>
>> ...but this test is only for 1/3. I haven't run this, but it looks like
>> the tests will still pass if we remove --rebase=p and --rebase=m.
>
> Good eyes.  It's not like that parsing these three is implemented
> with one thing; in other words, it is not hard to break one without
> breaking the other two.

Having said that, that can be done as a follow-up "oops, the
original was sloppy" patch.  It's not as bad compared to "oops, the
original was totally borked and here is a fix", so I am OK with that
;-)

[PATCH 10/10] fetch: retry fetching submodules if sha1 were not fetched

[Stefan Beller]

Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (and some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.

This works surprisingly well in some workflows (such as using submodules
as a third party library), while not so well in other scenarios, such
as in a Gerrit topic-based workflow, that can tie together changes
(potentially across repositories) on the server side. One of the parts
of such a Gerrit workflow is to download a change when wanting to examine
it, and you'd want to have its submodule changes that are in the same
topic downloaded as well. However these submodule changes reside in their
own repository in their on ref (refs/changes/).

Retry fetching a submodule if the object id that the superproject points
to, cannot be found.

Note: This is an RFC and doesn't support fetching to FETCH_HEAD yet, but
only into a local branch. To make fetching into FETCH_HEAD work, we need
some refactoring in builtin/fetch.c to adjust the calls to
'check_for_new_submodule_commits'.

Signed-off-by: Stefan Beller 
---
 builtin/fetch.c |  9 ++---
 submodule.c | 79 -
 t/t5526-fetch-submodules.sh | 16 
 3 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 34d2bd123b3..9396e6a44c6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -700,8 +700,7 @@ static int update_local_ref(struct ref *ref,
what = _("[new ref]");
}
 
-   if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
-   (recurse_submodules != RECURSE_SUBMODULES_ON))
+   if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
@@ -716,8 +715,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid, 
DEFAULT_ABBREV);
strbuf_addstr(, "..");
strbuf_add_unique_abbrev(, >new_oid, 
DEFAULT_ABBREV);
-   if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
-   (recurse_submodules != RECURSE_SUBMODULES_ON))
+   if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
@@ -731,8 +729,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid, 
DEFAULT_ABBREV);
strbuf_addstr(, "...");
strbuf_add_unique_abbrev(, >new_oid, 
DEFAULT_ABBREV);
-   if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
-   (recurse_submodules != RECURSE_SUBMODULES_ON))
+   if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
diff --git a/submodule.c b/submodule.c
index ec7ea6f8c2d..6cbd0b1a470 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1127,6 +1127,7 @@ struct submodule_parallel_fetch {
int result;
 
struct string_list changed_submodule_names;
+   struct string_list retry;
 };
 #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, 
STRING_LIST_INIT_DUP }
 
@@ -1259,8 +1260,10 @@ static int get_next_submodule(struct child_process *cp,
 {
int ret = 0;
struct submodule_parallel_fetch *spf = data;
+   struct string_list_item *retry_it;
 
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
+   int recurse_config;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
const char *git_dir, *default_argv;
@@ -1280,7 +1283,9 @@ static int get_next_submodule(struct child_process *cp,
}
}
 
-   switch (get_fetch_recurse_config(submodule, spf))
+   recurse_config = get_fetch_recurse_config(submodule, spf);
+
+   switch (recurse_config)
{
default:
case RECURSE_SUBMODULES_DEFAULT:
@@ -1318,9 +1323,46 @@ static int get_next_submodule(struct child_process *cp,
strbuf_release(_prefix);
if (ret) {
spf->count++;
+   if (submodule != _submodule)
+   /* discard const-ness: */
+   *task_cb = (void*)submodule;
return 1;
}

[PATCH 09/10] submodule: fetch in submodules git directory instead of in worktree

[Stefan Beller]

This patch started as a refactoring to make 'get_next_submodule' more
readable, but upon doing so, I realized that git-fetch actually doesn't
need to be run in the worktree. So let's run it in the git dir instead.

That should pave the way towards fetching submodules that are currently
not checked out.

Signed-off-by: Stefan Beller 
---
 submodule.c | 43 ++---
 t/t5526-fetch-submodules.sh |  7 +-
 2 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/submodule.c b/submodule.c
index 21757e32908..ec7ea6f8c2d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -481,6 +481,12 @@ void prepare_submodule_repo_env(struct argv_array *out)
 DEFAULT_GIT_DIR_ENVIRONMENT);
 }
 
+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)
+{
+   prepare_submodule_repo_env_no_git_dir(out);
+   argv_array_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
+}
+
 /* Helper function to display the submodule header line prior to the full
  * summary output. If it can locate the submodule objects directory it will
  * attempt to lookup both the left and right commits and put them into the
@@ -1227,6 +1233,27 @@ static int get_fetch_recurse_config(const struct 
submodule *submodule,
return spf->default_option;
 }
 
+static const char *get_submodule_git_dir(struct repository *r, const char 
*path)
+{
+   struct repository subrepo;
+   const char *ret;
+
+   if (repo_submodule_init(, r, path)) {
+   /* no entry in .gitmodules? */
+   struct strbuf gitdir = STRBUF_INIT;
+   strbuf_repo_worktree_path(, r, "%s/.git", path);
+   if (repo_init(, gitdir.buf, NULL)) {
+   strbuf_release();
+   return NULL;
+   }
+   }
+
+   ret = xstrdup(subrepo.gitdir);
+   repo_clear();
+
+   return ret;
+}
+
 static int get_next_submodule(struct child_process *cp,
  struct strbuf *err, void *data, void **task_cb)
 {
@@ -1234,8 +1261,6 @@ static int get_next_submodule(struct child_process *cp,
struct submodule_parallel_fetch *spf = data;
 
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
-   struct strbuf submodule_path = STRBUF_INIT;
-   struct strbuf submodule_git_dir = STRBUF_INIT;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
const char *git_dir, *default_argv;
@@ -1273,16 +1298,12 @@ static int get_next_submodule(struct child_process *cp,
continue;
}
 
-   strbuf_repo_worktree_path(_path, spf->r, "%s", 
ce->name);
-   strbuf_addf(_git_dir, "%s/.git", submodule_path.buf);
strbuf_addf(_prefix, "%s%s/", spf->prefix, ce->name);
-   git_dir = read_gitfile(submodule_git_dir.buf);
-   if (!git_dir)
-   git_dir = submodule_git_dir.buf;
-   if (is_directory(git_dir)) {
+   git_dir = get_submodule_git_dir(spf->r, ce->name);
+   if (git_dir) {
child_process_init(cp);
-   cp->dir = strbuf_detach(_path, NULL);
-   prepare_submodule_repo_env(>env_array);
+   prepare_submodule_repo_env_in_gitdir(>env_array);
+   cp->dir = git_dir;
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
@@ -1294,8 +1315,6 @@ static int get_next_submodule(struct child_process *cp,
argv_array_push(>args, submodule_prefix.buf);
ret = 1;
}
-   strbuf_release(_path);
-   strbuf_release(_git_dir);
strbuf_release(_prefix);
if (ret) {
spf->count++;
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 0f730d77815..4437cb17698 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -567,7 +567,12 @@ test_expect_success 'fetching submodule into a broken 
repository' '
 
test_must_fail git -C dst status &&
test_must_fail git -C dst diff &&
-   test_must_fail git -C dst fetch --recurse-submodules
+
+   # git-fetch cannot find the git directory of the submodule,
+   # so it will do nothing, successfully, as it cannot distinguish between
+   # this broken submodule and a submodule that was just set active but
+   # not cloned yet
+   git -C dst fetch --recurse-submodules
 '
 
 test_expect_success "fetch new commits when submodule got renamed" '
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 07/10] submodule: move global changed_submodule_names into fetch submodule struct

[Stefan Beller]

The `changed_submodule_names` are only used for fetching, so let's make it
part of the struct that is passed around for fetching submodules.

Signed-off-by: Stefan Beller 
---
 submodule.c | 42 +++---
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/submodule.c b/submodule.c
index 89a46b8af50..92988239f6b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -24,7 +24,7 @@
 #include "object-store.h"
 
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
+
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;
@@ -1110,7 +1110,22 @@ void check_for_new_submodule_commits(struct object_id 
*oid)
oid_array_append(_tips_after_fetch, oid);
 }
 
-static void calculate_changed_submodule_paths(void)
+struct submodule_parallel_fetch {
+   int count;
+   struct argv_array args;
+   struct repository *r;
+   const char *prefix;
+   int command_line_option;
+   int default_option;
+   int quiet;
+   int result;
+
+   struct string_list changed_submodule_names;
+};
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, 
STRING_LIST_INIT_DUP }
+
+static void calculate_changed_submodule_paths(
+   struct submodule_parallel_fetch *spf)
 {
struct argv_array argv = ARGV_ARRAY_INIT;
struct string_list changed_submodules = STRING_LIST_INIT_DUP;
@@ -1148,7 +1163,8 @@ static void calculate_changed_submodule_paths(void)
continue;
 
if (!submodule_has_commits(path, commits))
-   string_list_append(_submodule_names, 
name->string);
+   string_list_append(>changed_submodule_names,
+  name->string);
}
 
free_submodules_oids(_submodules);
@@ -1185,18 +1201,6 @@ int submodule_touches_in_range(struct object_id 
*excl_oid,
return ret;
 }
 
-struct submodule_parallel_fetch {
-   int count;
-   struct argv_array args;
-   struct repository *r;
-   const char *prefix;
-   int command_line_option;
-   int default_option;
-   int quiet;
-   int result;
-};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
-
 static int get_fetch_recurse_config(const struct submodule *submodule,
struct submodule_parallel_fetch *spf)
 {
@@ -1256,7 +1260,7 @@ static int get_next_submodule(struct child_process *cp,
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
if (!submodule || !string_list_lookup(
-   _submodule_names,
+   >changed_submodule_names,
submodule->name))
continue;
default_argv = "on-demand";
@@ -1348,8 +1352,8 @@ int fetch_populated_submodules(struct repository *r,
argv_array_push(, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
 
-   calculate_changed_submodule_paths();
-   string_list_sort(_submodule_names);
+   calculate_changed_submodule_paths();
+   string_list_sort(_submodule_names);
run_processes_parallel(max_parallel_jobs,
   get_next_submodule,
   fetch_start_failure,
@@ -1358,7 +1362,7 @@ int fetch_populated_submodules(struct repository *r,
 
argv_array_clear();
 out:
-   string_list_clear(_submodule_names, 1);
+   string_list_clear(_submodule_names, 1);
return spf.result;
 }
 
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 08/10] submodule.c: do not copy around submodule list

[Stefan Beller]

'calculate_changed_submodule_paths' uses a local list to compute the
changed submodules, and then produces the result by copying appropriate
items into the result list.

Instead use the result list directly and prune items afterwards
using string_list_remove_empty_items.

As a side effect, we'll have access to the util pointer for longer that
contains the commits that we need to fetch.

Signed-off-by: Stefan Beller 
---
 submodule.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/submodule.c b/submodule.c
index 92988239f6b..21757e32908 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1128,8 +1128,7 @@ static void calculate_changed_submodule_paths(
struct submodule_parallel_fetch *spf)
 {
struct argv_array argv = ARGV_ARRAY_INIT;
-   struct string_list changed_submodules = STRING_LIST_INIT_DUP;
-   const struct string_list_item *name;
+   struct string_list_item *name;
 
/* No need to check if there are no submodules configured */
if (!submodule_from_path(the_repository, NULL, NULL))
@@ -1146,9 +1145,9 @@ static void calculate_changed_submodule_paths(
 * Collect all submodules (whether checked out or not) for which new
 * commits have been recorded upstream in "changed_submodule_names".
 */
-   collect_changed_submodules(_submodules, );
+   collect_changed_submodules(>changed_submodule_names, );
 
-   for_each_string_list_item(name, _submodules) {
+   for_each_string_list_item(name, >changed_submodule_names) {
struct oid_array *commits = name->util;
const struct submodule *submodule;
const char *path = NULL;
@@ -1162,12 +1161,14 @@ static void calculate_changed_submodule_paths(
if (!path)
continue;
 
-   if (!submodule_has_commits(path, commits))
-   string_list_append(>changed_submodule_names,
-  name->string);
+   if (submodule_has_commits(path, commits)) {
+   oid_array_clear(commits);
+   *name->string = '\0';
+   }
}
 
-   free_submodules_oids(_submodules);
+   string_list_remove_empty_items(>changed_submodule_names, 1);
+
argv_array_clear();
oid_array_clear(_tips_before_fetch);
oid_array_clear(_tips_after_fetch);
@@ -1362,7 +1363,7 @@ int fetch_populated_submodules(struct repository *r,
 
argv_array_clear();
 out:
-   string_list_clear(_submodule_names, 1);
+   free_submodules_oids(_submodule_names);
return spf.result;
 }
 
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 05/10] submodule.c: fix indentation

[Stefan Beller]

The submodule subsystem is really bad at staying within 80 characters.
Fix it while we are here.

Signed-off-by: Stefan Beller 
---
 submodule.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index 5b4e5227d90..bceeba13217 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1244,7 +1244,8 @@ static int get_next_submodule(struct child_process *cp,
if (!submodule) {
const char *name = default_name_or_path(ce->name);
if (name) {
-   default_submodule.path = default_submodule.name 
= name;
+   default_submodule.path = name;
+   default_submodule.name = name;
submodule = _submodule;
}
}
@@ -1254,8 +1255,9 @@ static int get_next_submodule(struct child_process *cp,
default:
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
-   if (!submodule || 
!unsorted_string_list_lookup(_submodule_names,
-submodule->name))
+   if (!submodule || !unsorted_string_list_lookup(
+   _submodule_names,
+   submodule->name))
continue;
default_argv = "on-demand";
break;
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 04/10] submodule.c: convert submodule_move_head new argument to object id

[Stefan Beller]

All callers use oid_to_hex to convert the desired oid to a string before
calling submodule_move_head. Defer the conversion to the
submodule_move_head as it will turn out to be useful in a bit.

Signed-off-by: Stefan Beller 
---
 entry.c|  6 +++---
 submodule.c| 12 ++--
 submodule.h|  2 +-
 unpack-trees.c | 13 +
 4 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/entry.c b/entry.c
index b5d1d3cf231..4b34dfd30df 100644
--- a/entry.c
+++ b/entry.c
@@ -358,7 +358,7 @@ static int write_entry(struct cache_entry *ce,
sub = submodule_from_ce(ce);
if (sub)
return submodule_move_head(ce->name,
-   NULL, oid_to_hex(>oid),
+   NULL, >oid,
state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);
break;
 
@@ -438,10 +438,10 @@ int checkout_entry(struct cache_entry *ce,
unlink_or_warn(ce->name);
 
return submodule_move_head(ce->name,
-   NULL, oid_to_hex(>oid), 0);
+   NULL, >oid, 0);
} else
return submodule_move_head(ce->name,
-   "HEAD", oid_to_hex(>oid),
+   "HEAD", >oid,
state->force ? 
SUBMODULE_MOVE_HEAD_FORCE : 0);
}
 
diff --git a/submodule.c b/submodule.c
index 6e14547e9e0..5b4e5227d90 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1597,9 +1597,9 @@ static void submodule_reset_index(const char *path)
  * pass NULL for old or new respectively.
  */
 int submodule_move_head(const char *path,
-const char *old_head,
-const char *new_head,
-unsigned flags)
+   const char *old_head,
+   const struct object_id *new_oid,
+   unsigned flags)
 {
int ret = 0;
struct child_process cp = CHILD_PROCESS_INIT;
@@ -1679,7 +1679,7 @@ int submodule_move_head(const char *path,
if (!(flags & SUBMODULE_MOVE_HEAD_FORCE))
argv_array_push(, old_head ? old_head : 
empty_tree_oid_hex());
 
-   argv_array_push(, new_head ? new_head : empty_tree_oid_hex());
+   argv_array_push(, new_oid ? oid_to_hex(new_oid) : 
empty_tree_oid_hex());
 
if (run_command()) {
ret = error(_("Submodule '%s' could not be updated."), path);
@@ -1687,7 +1687,7 @@ int submodule_move_head(const char *path,
}
 
if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
-   if (new_head) {
+   if (new_oid) {
child_process_init();
/* also set the HEAD accordingly */
cp.git_cmd = 1;
@@ -1696,7 +1696,7 @@ int submodule_move_head(const char *path,
 
prepare_submodule_repo_env(_array);
argv_array_pushl(, "update-ref", "HEAD",
-"--no-deref", new_head, NULL);
+"--no-deref", oid_to_hex(new_oid), 
NULL);
 
if (run_command()) {
ret = -1;
diff --git a/submodule.h b/submodule.h
index 4644683e6cb..d1cceabb9b7 100644
--- a/submodule.h
+++ b/submodule.h
@@ -118,7 +118,7 @@ int submodule_to_gitdir(struct strbuf *buf, const char 
*submodule);
 #define SUBMODULE_MOVE_HEAD_FORCE   (1<<1)
 extern int submodule_move_head(const char *path,
   const char *old,
-  const char *new_head,
+  const struct object_id *new_oid,
   unsigned flags);
 
 void submodule_unset_core_worktree(const struct submodule *sub);
diff --git a/unpack-trees.c b/unpack-trees.c
index f9efee0836a..6c76bd6162a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -256,7 +256,7 @@ static void display_error_msgs(struct unpack_trees_options 
*o)
 
 static int check_submodule_move_head(const struct cache_entry *ce,
 const char *old_id,
-const char *new_id,
+const struct object_id *new_id,
 struct unpack_trees_options *o)
 {
unsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;
@@ -1513,7 +1513,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,
 
if (submodule_from_ce(ce)) {
int r = check_submodule_move_head(ce,
-   "HEAD", oid_to_hex(>oid), o);
+   "HEAD", >oid, o);
if (r)
return o->gently ? -1 :
   

[PATCH 03/10] sha1-array: provide oid_array_remove_if

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 sha1-array.c | 39 +++
 sha1-array.h |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/sha1-array.c b/sha1-array.c
index 265941fbf40..10eb08b425e 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -77,3 +77,42 @@ int oid_array_for_each_unique(struct oid_array *array,
}
return 0;
 }
+
+int oid_array_remove_if(struct oid_array *array,
+   for_each_oid_fn fn,
+   void *data)
+{
+   int i, j;
+   char *to_remove = xcalloc(array->nr, sizeof(char));
+
+   /* No oid_array_sort() here! See the api-oid-array.txt docs! */
+
+   for (i = 0; i < array->nr; i++) {
+   int ret = fn(array->oid + i, data);
+   if (ret)
+   to_remove[i] = 1;
+   }
+
+   i = 0, j = 0;
+   while (i < array->nr && j < array->nr) {
+   while (i < array->nr && !to_remove[i])
+   i++;
+   /* i at first marked for deletion or out */
+   if (j < i)
+   j = i;
+   while (j < array->nr && to_remove[j])
+   j++;
+   /* j > i; j at first valid after first deletion range or out */
+   if (i < array->nr && j < array->nr)
+   oidcpy(>oid[i], >oid[j]);
+   else if (i >= array->nr)
+   assert(j >= array->nr);
+   /* no pruning happened, keep original array->nr */
+   else if (j >= array->nr)
+   array->nr = i;
+   }
+
+   free(to_remove);
+
+   return 0;
+}
diff --git a/sha1-array.h b/sha1-array.h
index 232bf950172..151c7ad7f30 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -22,5 +22,8 @@ int oid_array_for_each(struct oid_array *array,
 int oid_array_for_each_unique(struct oid_array *array,
  for_each_oid_fn fn,
  void *data);
+int oid_array_remove_if(struct oid_array *array,
+   for_each_oid_fn fn,
+   void *data);
 
 #endif /* SHA1_ARRAY_H */
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 06/10] submodule.c: sort changed_submodule_names before searching it

[Stefan Beller]

Instead of sorting it after we created an unsorted list, we could insert
correctly into the list.  As the unsorted append is in order of cache entry
names, this is already sorted if names were equal to paths for submodules.

As submodule names are often the same as their path, the input is sorted
pretty well already, so let's just do the sort afterwards.

Signed-off-by: Stefan Beller 
---
 submodule.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index bceeba13217..89a46b8af50 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1255,7 +1255,7 @@ static int get_next_submodule(struct child_process *cp,
default:
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
-   if (!submodule || !unsorted_string_list_lookup(
+   if (!submodule || !string_list_lookup(
_submodule_names,
submodule->name))
continue;
@@ -1349,6 +1349,7 @@ int fetch_populated_submodules(struct repository *r,
/* default value, "--submodule-prefix" and its value are added later */
 
calculate_changed_submodule_paths();
+   string_list_sort(_submodule_names);
run_processes_parallel(max_parallel_jobs,
   get_next_submodule,
   fetch_start_failure,
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 01/10] string_list: print_string_list to use trace_printf

[Stefan Beller]

It is a debugging aid, so it should print to the debugging channel.

Signed-off-by: Stefan Beller 
---
 string-list.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/string-list.c b/string-list.c
index 771c4550980..9f651bb4294 100644
--- a/string-list.c
+++ b/string-list.c
@@ -200,9 +200,9 @@ void print_string_list(const struct string_list *p, const 
char *text)
 {
int i;
if ( text )
-   printf("%s\n", text);
+   trace_printf("%s\n", text);
for (i = 0; i < p->nr; i++)
-   printf("%s:%p\n", p->items[i].string, p->items[i].util);
+   trace_printf("%s:%p\n", p->items[i].string, p->items[i].util);
 }
 
 struct string_list_item *string_list_append_nodup(struct string_list *list,
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 02/10] string-list.h: add string_list_pop function.

[Stefan Beller]

A string list can be used as a stack, but should we? A later patch shows
how useful this will be.

Signed-off-by: Stefan Beller 
---
 string-list.c | 8 
 string-list.h | 6 ++
 2 files changed, 14 insertions(+)

diff --git a/string-list.c b/string-list.c
index 9f651bb4294..ea80afc8a0c 100644
--- a/string-list.c
+++ b/string-list.c
@@ -80,6 +80,14 @@ void string_list_remove(struct string_list *list, const char 
*string,
}
 }
 
+struct string_list_item *string_list_pop(struct string_list *list)
+{
+   if (list->nr == 0)
+   return 0;
+
+   return >items[--list->nr];
+}
+
 int string_list_has_string(const struct string_list *list, const char *string)
 {
int exact_match;
diff --git a/string-list.h b/string-list.h
index ff8f6094a33..a1a41bc961a 100644
--- a/string-list.h
+++ b/string-list.h
@@ -191,6 +191,12 @@ extern void string_list_remove(struct string_list *list, 
const char *string,
  */
 struct string_list_item *string_list_lookup(struct string_list *list, const 
char *string);
 
+/**
+ * Returns the last item inserted and removes it from the list.
+ * If the list is empty, return NULL.
+ */
+struct string_list_item *string_list_pop(struct string_list *list);
+
 /*
  * Remove all but the first of consecutive entries with the same
  * string value.  If free_util is true, call free() on the util
-- 
2.18.0.597.ga71716f1ad-goog

[RFC PATCH 00/10] fetch: make sure submodule oids are fetched

[Stefan Beller]

Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (and some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.

This works surprisingly well in some workflows, not so well in others,
which this series aims to fix.

The first patches provide new basic functionality and do some refactoring;
the interesting part is in the two last patches.

Thanks,
Stefan

Stefan Beller (10):
  string_list: print_string_list to use trace_printf
  string-list.h: add string_list_pop function.
  sha1-array: provide oid_array_remove_if
  submodule.c: convert submodule_move_head new argument to object id
  submodule.c: fix indentation
  submodule.c: sort changed_submodule_names before searching it
  submodule: move global changed_submodule_names into fetch submodule
struct
  submodule.c: do not copy around submodule list
  submodule: fetch in submodules git directory instead of in worktree
  fetch: retry fetching submodules if sha1 were not fetched

 builtin/fetch.c |   9 +-
 entry.c |   6 +-
 sha1-array.c|  39 
 sha1-array.h|   3 +
 string-list.c   |  12 ++-
 string-list.h   |   6 ++
 submodule.c | 194 +++-
 submodule.h |   2 +-
 t/t5526-fetch-submodules.sh |  23 -
 unpack-trees.c  |  13 +--
 10 files changed, 241 insertions(+), 66 deletions(-)

-- 
2.18.0.597.ga71716f1ad-goog

Re: Help with "fatal: unable to read ...." error during GC?

[Paul Smith]

On Wed, 2018-08-08 at 14:24 -0400, Jeff King wrote:
> Let's narrow it down first and make sure we're dying where I expect.
> Can
> you try:
> 
>   GIT_TRACE=1 git gc
> 
> and confirm the program running when the fatal error is produced?
> 
> From what you've shown it's going to be git-repack, but what I'm not
> clear on is whether it is repack itself that is complaining, or the
> pack-objects process it spawns. I'd guess the latter.

You are correct:

15:27:24.264161 git.c:415   trace: built-in: git pack-
objects --keep-true-parents --honor-pack-keep --non-empty --all --
reflog --indexed-objects --unpack-unreachable=2.weeks.ago --local --
delta-base-offset .git/objects/pack/.tmp-17617-pack

> If so, can you try running it under gdb and getting a stack trace?

I would... but I discovered all my Git binaries are stripped to the max
and no symbols available.

I'll do a quick rebuild with some debug info and get back to you.

Thanks for the pointers!

[PATCH] update-index: there no longer is `apply --index-info`

[Junio C Hamano]

Back when we removed `git apply --index-info` in 2007, we forgot to
adjust the documentation for update-index that reads its output.

Let's reorder the description of three formats to present the other
two formats that are still generated by git commands before this
format, and stop mentioning `git apply --index-info`.

Signed-off-by: Junio C Hamano 
---
 Documentation/git-update-index.txt | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index 4e8e762e68..c62a683648 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -268,23 +268,20 @@ USING --INDEX-INFO
 multiple entry definitions from the standard input, and designed
 specifically for scripts.  It can take inputs of three formats:
 
-. mode SP sha1  TAB path
-+
-The first format is what "git-apply --index-info"
-reports, and used to reconstruct a partial tree
-that is used for phony merge base tree when falling
-back on 3-way merge.
-
 . mode SP type SP sha1  TAB path
 +
-The second format is to stuff 'git ls-tree' output
-into the index file.
+This format is to stuff `git ls-tree` output into the index.
 
 . mode SP sha1 SP stage TAB path
 +
 This format is to put higher order stages into the
 index file and matches 'git ls-files --stage' output.
 
+. mode SP sha1  TAB path
++
+This format is no longer produced by any Git command, but is
+and will continue to be supported by `update-index --index-info`.
+
 To place a higher stage entry to the index, the path should
 first be removed by feeding a mode=0 entry for the path, and
 then feeding necessary input lines in the third format.

Re: [PATCH] git-update-index.txt: reword possibly confusing example

[Junio C Hamano]

Elijah Newren  writes:

> The following phrase could be interpreted multiple ways:
>   "To pretend you have a file with mode and sha1 at path"
>
> In particular, I can think of two:
>   1. Pretend we have some new file, which happens to have a given mode
>  and sha1
>   2. Pretend one of the files we are already tracking has a different
>  mode and sha1 than what it really does
>
> I think people could easily assume either case while reading, but the
> example command provided doesn't actually handle the first case, which
> caused some minor frustration to at least one user.  Modify the example
> command so that it correctly handles both cases, and re-order the
> wording in a way that makes it more likely folks will assume the first
> interpretation.

I do not think the rephrasing loses those who want to update an
existing path, and is a good one.

> -To pretend you have a file with mode and sha1 at path, say:
> +To pretend you have a file at path with mode and sha1, say:
>  
>  
> -$ git update-index --cacheinfo ,,
> +$ git update-index --add --cacheinfo ,,
>  

Re: [PATCH v2 0/4] Speed up unpack_trees()

[Ben Peart]

On 8/1/2018 12:38 PM, Duy Nguyen wrote:

On Tue, Jul 31, 2018 at 01:31:31PM -0400, Ben Peart wrote:



On 7/31/2018 12:50 PM, Ben Peart wrote:



On 7/31/2018 11:31 AM, Duy Nguyen wrote:





In the performance game of whack-a-mole, that call to repair cache-tree
is now looking quite expensive...


Yeah and I think we can whack that mole too. I did some measurement.
Best case possible, we just need to scan through two indexes (one with
many good cache-tree, one with no cache-tree), compare and copy
cache-tree over. The scanning takes like 1% time of current repair
step and I suspect it's the hashing that takes most of the time. Of
course real world won't have such nice numbers, but I guess we could
maybe half cache-tree update/repair time.



I have some great profiling tools available so will take a look at this
next and see exactly where the time is being spent.


Good instincts.  In cache_tree_update, the heavy hitter is definitely
hash_object_file followed by has_object_file.

NameInc %Inc
+ git!cache_tree_update  12.4  4,935
|+ git!update_one11.8  4,706
| + git!update_one   11.8  4,706
|  + git!hash_object_file 6.1  2,406
|  + git!has_object_file  2.0813
|  + OTHER <>   0.5203
|  + git!strbuf_addf  0.4155
|  + git!strbuf_release   0.4143
|  + git!strbuf_add   0.3121
|  + OTHER <>   0.2 93
|  + git!strbuf_grow  0.1 25


Ben, if you work on this, this could be a good starting point. I will
not work on this because I still have some other things to catch up
and follow through. You can have my sign off if you reuse something
from this patch

Even if it's a naive implementation, the initial numbers look pretty
good. Without the patch we have

18:31:05.970621 unpack-trees.c:1437 performance: 0.01029 s: copy
18:31:05.975729 unpack-trees.c:1444 performance: 0.005082004 s: update

And with the patch

18:31:13.295655 unpack-trees.c:1437 performance: 0.000198017 s: copy
18:31:13.296757 unpack-trees.c:1444 performance: 0.001075935 s: update

Time saving is about 80% by the look of this (best possible case
because only the top tree needs to be hashed and written out).

-- 8< --
diff --git a/cache-tree.c b/cache-tree.c
index 6b46711996..67a4a93100 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -440,6 +440,147 @@ int cache_tree_update(struct index_state *istate, int 
flags)
return 0;
  }
  
+static int same(const struct cache_entry *a, const struct cache_entry *b)

+{
+   if (ce_stage(a) || ce_stage(b))
+   return 0;
+   if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED)
+   return 0;
+   return a->ce_mode == b->ce_mode &&
+  !oidcmp(>oid, >oid);
+}
+
+static int cache_tree_name_pos(const struct index_state *istate,
+  const struct strbuf *path)
+{
+   int pos;
+
+   if (!path->len)
+   return 0;
+
+   pos = index_name_pos(istate, path->buf, path->len);
+   if (pos >= 0)
+   BUG("No no no, directory path must not exist in index");
+   return -pos - 1;
+}
+
+/*
+ * Locate the same cache-tree in two separate indexes. Check the
+ * cache-tree is still valid for the "to" index (i.e. it contains the
+ * same set of entries in the "from" index).
+ */
+static int verify_one_cache_tree(const struct index_state *to,
+const struct index_state *from,
+const struct cache_tree *it,
+const struct strbuf *path)
+{
+   int i, spos, dpos;
+
+   spos = cache_tree_name_pos(from, path);
+   if (spos + it->entry_count > from->cache_nr)
+   return -1;
+
+   dpos = cache_tree_name_pos(to, path);
+   if (dpos + it->entry_count > to->cache_nr)
+   return -1;
+
+   /* Can we quickly check head and tail and bail out early */
+   if (!same(from->cache[spos], to->cache[spos]) ||
+   !same(from->cache[spos + it->entry_count - 1],
+ to->cache[spos + it->entry_count - 1]))
+   return -1;
+
+   for (i = 1; i < it->entry_count - 1; i++)
+   if (!same(from->cache[spos + i],
+ to->cache[dpos + i]))
+   return -1;
+
+   return 0;
+}
+
+static int verify_and_invalidate(struct index_state *to,
+const struct index_state *from,
+struct cache_tree *it,
+struct strbuf *path)
+{
+   /*
+* Optimistically verify the current tree first. Alternatively
+* we could verify all the subtrees first then do this
+* last. Any invalid subtree would also invalidates its
+* 

[PATCH] git-update-index.txt: reword possibly confusing example

[Elijah Newren]

The following phrase could be interpreted multiple ways:
  "To pretend you have a file with mode and sha1 at path"

In particular, I can think of two:
  1. Pretend we have some new file, which happens to have a given mode
 and sha1
  2. Pretend one of the files we are already tracking has a different
 mode and sha1 than what it really does

I think people could easily assume either case while reading, but the
example command provided doesn't actually handle the first case, which
caused some minor frustration to at least one user.  Modify the example
command so that it correctly handles both cases, and re-order the
wording in a way that makes it more likely folks will assume the first
interpretation.  I believe the new example shouldn't pose any obstacles
to those wanting the second interpretation (at worst, they pass an
unnecessary extra flag).

Signed-off-by: Elijah Newren 
---
 Documentation/git-update-index.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index 4e8e762e68..a9753e6557 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -245,10 +245,10 @@ USING --CACHEINFO OR --INFO-ONLY
 current working directory.  This is useful for minimum-checkout
 merging.
 
-To pretend you have a file with mode and sha1 at path, say:
+To pretend you have a file at path with mode and sha1, say:
 
 
-$ git update-index --cacheinfo ,,
+$ git update-index --add --cacheinfo ,,
 
 
 `--info-only` is used to register files without placing them in the object
-- 
2.18.0.556.g1604670984

Re: [PATCH 0/3] Resending sb/config-write-fix

[Junio C Hamano]

Stefan Beller  writes:

> This is a resend of sb/config-write-fix, with a slightly
> better commit message and a renamed variable.
>
> Thanks,
> Stefan

Thanks.  Let's declare victory and mark it to be merged to 'next'.

Re: [GSoC][PATCH v7 05/26] stash: convert apply to builtin

[Junio C Hamano]

Paul-Sebastian Ungureanu  writes:

> From: Joel Teichroeb 
>
> Add a builtin helper for performing stash commands. Converting
> all at once proved hard to review, so starting with just apply
> lets conversion get started without the other commands being
> finished.
>
> The helper is being implemented as a drop in replacement for
> stash so that when it is complete it can simply be renamed and
> the shell script deleted.
>
> Delete the contents of the apply_stash shell function and replace
> it with a call to stash--helper apply until pop is also
> converted.
>
> Signed-off-by: Joel Teichroeb 
> Signed-off-by: Paul-Sebastian Ungureanu 

Good to see that the right way to forward a patch from another
person is used, but is this a GSoC project?

> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> new file mode 100644
> index 0..ef6a9d30d
> --- /dev/null
> +++ b/builtin/stash--helper.c
> @@ -0,0 +1,452 @@
> +#include "builtin.h"
> +#include "config.h"
> +#include "parse-options.h"
> +#include "refs.h"
> +#include "lockfile.h"
> +#include "cache-tree.h"
> +#include "unpack-trees.h"
> +#include "merge-recursive.h"
> +#include "argv-array.h"
> +#include "run-command.h"
> +#include "dir.h"
> +#include "rerere.h"

Wow, "apply" is a biggie, as you'd pretty much have to do
everything, like merging and updating the index and asking rerere to
auto-resolve.  Quite a many include files.

> +static const char * const git_stash_helper_usage[] = {
> + N_("git stash--helper apply [--index] [-q|--quiet] []"),
> + NULL
> +};
> +
> +static const char * const git_stash_helper_apply_usage[] = {
> + N_("git stash--helper apply [--index] [-q|--quiet] []"),
> + NULL
> +};
> + ...
> +static void assert_stash_like(struct stash_info *info, const char * revision)

This inherits an unfortunate name from the scripted version (it does
more than asserting), but it is OK to keep the original name,
especially in this early step in the series.

Lose the SP in "* revision"; the asterisk sticks to the variable/parameter name.

> +{
> + if (get_oidf(>b_commit, "%s^1", revision) ||
> + get_oidf(>w_tree, "%s:", revision) ||
> + get_oidf(>b_tree, "%s^1:", revision) ||
> + get_oidf(>i_tree, "%s^2:", revision)) {
> + free_stash_info(info);
> + error(_("'%s' is not a stash-like commit"), revision);
> + exit(128);
> + }
> +}

> +static int reset_tree(struct object_id *i_tree, int update, int reset)
> +{
> ...
> +}

Kind-a surprising that there is no helper function to do this
already.  The implementation looked OK, though.

> +static int apply_cached(struct strbuf *out)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> +
> + /*
> +  * Apply currently only reads either from stdin or a file, thus
> +  * apply_all_patches would have to be updated to optionally take a
> +  * buffer.
> +  */
> + cp.git_cmd = 1;
> + argv_array_pushl(, "apply", "--cached", NULL);
> + return pipe_command(, out->buf, out->len, NULL, 0, NULL, 0);
> +}

Applying and updating the index is more resource intensive than
spawning a process, and not having to worry about the process dying
is a benefit, so overall, making this into an internal call would be
a lot lower priority, I would guess.

> +static int reset_head(const char *prefix)
> +{

This is resetting the index to the HEAD, right?  reset_head sounds
as if it takes a commit-ish and moves HEAD there.

Re: [PATCH 1/1] pull --rebase=: allow single-letter abbreviations for the type

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>> -else if (!strcmp(value, "preserve"))
>> +else if (!strcmp(value, "preserve") || !strcmp(value, "p"))
>>  return REBASE_PRESERVE;
>> -else if (!strcmp(value, "merges"))
>> +else if (!strcmp(value, "merges") || !strcmp(value, "m"))
>>  return REBASE_MERGES;
>> -else if (!strcmp(value, "interactive"))
>> +else if (!strcmp(value, "interactive") || !strcmp(value, "i"))
>>  return REBASE_INTERACTIVE;
>
> Here 3 special cases are added...
> ...
>> +test_expect_success 'pull --rebase=i' '
>> ...
>> +'
>> +
>>  test_expect_success 'pull.rebase=invalid fails' '
>>  git reset --hard before-preserve-rebase &&
>>  test_config pull.rebase invalid &&
>
> ...but this test is only for 1/3. I haven't run this, but it looks like
> the tests will still pass if we remove --rebase=p and --rebase=m.

Good eyes.  It's not like that parsing these three is implemented
with one thing; in other words, it is not hard to break one without
breaking the other two.

[PATCH 2/3] config: fix case sensitive subsection names on writing

[Stefan Beller]

A user reported a submodule issue regarding a section mix-up,
but it could be boiled down to the following test case:

  $ git init test  && cd test
  $ git config foo."Bar".key test
  $ git config foo."bar".key test
  $ tail -n 3 .git/config
  [foo "Bar"]
key = test
key = test

Sub sections are case sensitive and we have a test for correctly reading
them. However we do not have a test for writing out config correctly with
case sensitive subsection names, which is why this went unnoticed in
6ae996f2acf (git_config_set: make use of the config parser's event
stream, 2018-04-09)

Unfortunately we have to make a distinction between old style configuration
that looks like

  [foo.Bar]
key = test

and the new quoted style as seen above. The old style is documented as
case-agnostic, hence we need to keep 'strncasecmp'; although the
resulting setting for the old style config differs from the configuration.
That will be fixed in a follow up patch.

Reported-by: JP Sugarbroad 
Signed-off-by: Stefan Beller 
---
 config.c  | 12 +++-
 t/t1300-config.sh |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 02050529788..27e800c7ce8 100644
--- a/config.c
+++ b/config.c
@@ -35,6 +35,7 @@ struct config_source {
int eof;
struct strbuf value;
struct strbuf var;
+   unsigned subsection_case_sensitive : 1;
 
int (*do_fgetc)(struct config_source *c);
int (*do_ungetc)(int c, struct config_source *conf);
@@ -603,6 +604,7 @@ static int get_value(config_fn_t fn, void *data, struct 
strbuf *name)
 
 static int get_extended_base_var(struct strbuf *name, int c)
 {
+   cf->subsection_case_sensitive = 0;
do {
if (c == '\n')
goto error_incomplete_line;
@@ -639,6 +641,7 @@ static int get_extended_base_var(struct strbuf *name, int c)
 
 static int get_base_var(struct strbuf *name)
 {
+   cf->subsection_case_sensitive = 1;
for (;;) {
int c = get_next_char();
if (cf->eof)
@@ -2328,14 +2331,21 @@ static int store_aux_event(enum config_event_t type,
store->parsed[store->parsed_nr].type = type;
 
if (type == CONFIG_EVENT_SECTION) {
+   int (*cmpfn)(const char *, const char *, size_t);
+
if (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.')
return error("invalid section name '%s'", cf->var.buf);
 
+   if (cf->subsection_case_sensitive)
+   cmpfn = strncasecmp;
+   else
+   cmpfn = strncmp;
+
/* Is this the section we were looking for? */
store->is_keys_section =
store->parsed[store->parsed_nr].is_keys_section =
cf->var.len - 1 == store->baselen &&
-   !strncasecmp(cf->var.buf, store->key, store->baselen);
+   !cmpfn(cf->var.buf, store->key, store->baselen);
if (store->is_keys_section) {
store->section_seen = 1;
ALLOC_GROW(store->seen, store->seen_nr + 1,
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index c15c19bf78d..77c5899d000 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1260,6 +1260,7 @@ test_expect_success 'setting different case sensitive 
subsections ' '
Qc = v2
[d "e"]
f = v1
+   [d "E"]
Qf = v2
EOF
# exact match
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 1/3] t1300: document current behavior of setting options

[Stefan Beller]

This documents current behavior of the config machinery, when changing
the value of some settings. This patch just serves to provide a baseline
for the follow up that will fix some issues with the current behavior.

Signed-off-by: Stefan Beller 
---
 t/t1300-config.sh | 86 +++
 1 file changed, 86 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e43982a9c1f..c15c19bf78d 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1188,6 +1188,92 @@ test_expect_success 'last one wins: three level vars' '
test_cmp expect actual
 '
 
+test_expect_success 'old-fashioned settings are case insensitive' '
+   test_when_finished "rm -f testConfig testConfig_expect 
testConfig_actual" &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V.A]
+   r = value1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V.A]
+   Qr = value2
+   EOF
+   git config -f testConfig_actual "v.a.r" value2 &&
+   test_cmp testConfig_expect testConfig_actual &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V.A]
+   r = value1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V.A]
+   QR = value2
+   EOF
+   git config -f testConfig_actual "V.a.R" value2 &&
+   test_cmp testConfig_expect testConfig_actual &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V.A]
+   r = value1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V.A]
+   r = value1
+   Qr = value2
+   EOF
+   git config -f testConfig_actual "V.A.r" value2 &&
+   test_cmp testConfig_expect testConfig_actual &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V.A]
+   r = value1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V.A]
+   r = value1
+   Qr = value2
+   EOF
+   git config -f testConfig_actual "v.A.r" value2 &&
+   test_cmp testConfig_expect testConfig_actual
+'
+
+test_expect_success 'setting different case sensitive subsections ' '
+   test_when_finished "rm -f testConfig testConfig_expect 
testConfig_actual" &&
+
+   cat >testConfig_actual <<-EOF &&
+   [V "A"]
+   R = v1
+   [K "E"]
+   Y = v1
+   [a "b"]
+   c = v1
+   [d "e"]
+   f = v1
+   EOF
+   q_to_tab >testConfig_expect <<-EOF &&
+   [V "A"]
+   Qr = v2
+   [K "E"]
+   Qy = v2
+   [a "b"]
+   Qc = v2
+   [d "e"]
+   f = v1
+   Qf = v2
+   EOF
+   # exact match
+   git config -f testConfig_actual a.b.c v2 &&
+   # match section and subsection, key is cased differently.
+   git config -f testConfig_actual K.E.y v2 &&
+   # section and key are matched case insensitive, but subsection needs
+   # to match; When writing out new values only the key is adjusted
+   git config -f testConfig_actual v.A.r v2 &&
+   # subsection is not matched:
+   git config -f testConfig_actual d.E.f v2 &&
+   test_cmp testConfig_expect testConfig_actual
+'
+
 for VAR in a .a a. a.0b a."b c". a."b c".0d
 do
test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax

[Stefan Beller]

The bug was noticed when writing the previous patch; a fix for this bug
is not easy though: If we choose to ignore the case of the subsection
(and revert most of the code of the previous patch, just keeping
s/strncasecmp/strcmp/), then we'd introduce new sections using the
new syntax, such that

 
   [section.subsection]
 key = value1
 

  git config section.Subsection.key value2

would result in

 
   [section.subsection]
 key = value1
   [section.Subsection]
 key = value2
 

which is even more confusing. A proper fix would replace the first
occurrence of 'key'. As the syntax is deprecated, let's prefer to not
spend time on fixing the behavior and just document it instead.

Signed-off-by: Stefan Beller 
---
 Documentation/git-config.txt | 21 +
 1 file changed, 21 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 14da5fc157e..1ac2eab9482 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -430,6 +430,27 @@ http.sslverify false
 
 include::config.txt[]
 
+BUGS
+
+When using the deprecated `[section.subsection]` syntax, changing a value
+will result in adding a multi-line key instead of a change, if the subsection
+is given with at least one uppercase character. For example when the config
+looks like
+
+
+  [section.subsection]
+key = value1
+
+
+and running `git config section.Subsection.key value2` will result in
+
+
+  [section.subsection]
+key = value1
+key = value2
+
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 0/3] Resending sb/config-write-fix

[Stefan Beller]

This is a resend of sb/config-write-fix, with a slightly
better commit message and a renamed variable.

Thanks,
Stefan


Stefan Beller (3):
  t1300: document current behavior of setting options
  config: fix case sensitive subsection names on writing
  git-config: document accidental multi-line setting in deprecated
syntax

 Documentation/git-config.txt | 21 +
 config.c | 12 -
 t/t1300-config.sh| 87 
 3 files changed, 119 insertions(+), 1 deletion(-)

./git-range-diff origin/sb/config-write-fix...HEAD >>-cover-letter.patch 
2.18.0.597.ga71716f1ad-goog

1:  999d9026272 ! 1:  e40f57f3da1 t1300: document current behavior of setting 
options
@@ -7,7 +7,6 @@
 for the follow up that will fix some issues with the current behavior.
 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
  diff --git a/t/t1300-config.sh b/t/t1300-config.sh
  --- a/t/t1300-config.sh
2:  c667e555066 ! 2:  f01cb1d9dae config: fix case sensitive subsection names 
on writing
@@ -2,8 +2,8 @@
 
 config: fix case sensitive subsection names on writing
 
-A use reported a submodule issue regarding strange case indentation
-issues, but it could be boiled down to the following test case:
+A user reported a submodule issue regarding a section mix-up,
+but it could be boiled down to the following test case:
 
   $ git init test  && cd test
   $ git config foo."Bar".key test
@@ -32,7 +32,6 @@
 
 Reported-by: JP Sugarbroad 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
  diff --git a/config.c b/config.c
  --- a/config.c
@@ -41,7 +40,7 @@
int eof;
struct strbuf value;
struct strbuf var;
-+  unsigned section_name_old_dot_style : 1;
++  unsigned subsection_case_sensitive : 1;
  
int (*do_fgetc)(struct config_source *c);
int (*do_ungetc)(int c, struct config_source *conf);
@@ -49,7 +48,7 @@
  
  static int get_extended_base_var(struct strbuf *name, int c)
  {
-+  cf->section_name_old_dot_style = 0;
++  cf->subsection_case_sensitive = 0;
do {
if (c == '\n')
goto error_incomplete_line;
@@ -57,7 +56,7 @@
  
  static int get_base_var(struct strbuf *name)
  {
-+  cf->section_name_old_dot_style = 1;
++  cf->subsection_case_sensitive = 1;
for (;;) {
int c = get_next_char();
if (cf->eof)
@@ -70,7 +69,7 @@
if (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.')
return error("invalid section name '%s'", cf->var.buf);
  
-+  if (cf->section_name_old_dot_style)
++  if (cf->subsection_case_sensitive)
 +  cmpfn = strncasecmp;
 +  else
 +  cmpfn = strncmp;
3:  6749bb283a8 ! 3:  6b5ad773490 git-config: document accidental multi-line 
setting in deprecated syntax
@@ -29,7 +29,6 @@
 spend time on fixing the behavior and just document it instead.
 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
  diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
  --- a/Documentation/git-config.txt

Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems

[Jeff Hostetler]

On 8/7/2018 3:31 PM, Junio C Hamano wrote:

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


  One nice thing about this is we don't need platform specific code for
  detecting the duplicate entries. I think ce_match_stat() works even
  on Windows. And it's now equally expensive on all platforms :D


ce_match_stat() may not be a very good measure to see if two paths
refer to the same file, though.  After a fresh checkout, I would not
be surprised if two completely unrelated paths have the same size
and have same mtime/ctime.  In its original use case, i.e. "I have
one specific path in mind and took a snapshot of its metadata
earlier.  Is it still the same, or has it been touched?", that may
be sufficient to detect that the path has been _modified_, but
without reliable inum, it may be a poor measure to say two paths
refer to the same.


I agree with Junio on this one.  The mtime values are sloppy at best.
On FAT file systems, they have 2 second resolution.  Even NTFS IIRC
has only 100ns resolution, so we might get a lot of false matches
using this technique, right?

It might be better to build an equivalence-class hash-map for the
colliding entries.  Compute a "normalized" version of the pathname
(such as convert to lowercase, strip final-dots/spaces, strip the
digits following tilda of a shortname, and etc for the MAC's UTF-isms).
Then when you rescan the index entries to find the matches, apply the
equivalence operator on the pathname and do the hashmap lookup.
When you find a match, you have a "potential" collider pair (I say
potential only because of the ambiguity of shortnames).  Then we
can use inum/file-index/whatever to see if they actually collide.


Jeff

Re: [PATCH 04/11] builtin rebase: support --quiet

[Junio C Hamano]

Stefan Beller  writes:

> On Wed, Aug 8, 2018 at 6:51 AM Pratik Karki  wrote:
>>
>> This commit introduces a rebase option `--quiet`. While `--quiet` is
>> commonly perceived as opposite to `--verbose`, this is not the case for
>> the rebase command: both `--quiet` and `--verbose` default to `false` if
>> neither `--quiet` nor `--verbose` is present.
>>
>> This commit goes further and introduces `--no-quiet` which is the
>> contrary of `--quiet` and it's introduction doesn't modify any
>> behaviour.

Why?  Is it for completeness (i.e. does the scripted version take
such an option and addition of --no-quiet makes the C rewrite behave
the same)?

>> Note: The `flags` field in `rebase_options` will accumulate more bits in
>> subsequent commits, in particular a verbose and a diffstat flag. And as
>> --quoet inthe shell scripted version of the rebase command switches off
>
>   --quote in the
>
> (in case a resend is needed)

Meaning --quiet?

Re: [PATCH 03/11] builtin rebase: handle the pre-rebase hook (and add --no-verify)

[Junio C Hamano]

Pratik Karki  writes:

> This commit converts the equivalent part of the shell script
> `git-legacy-rebase.sh` to run the pre-rebase hook (unless disabled), and
> to interrupt the rebase with error if the hook fails.
>
> Signed-off-by: Pratik Karki 
> ---

Introduction of upstream_arg in this step looked a bit
surprising, but the hook invocation is the only thing that uses it,
so it is understandable.

"rebase: handle the re-rebase hook and --no-verify" would have been
sufficient, without "add" or parentheses.

>  builtin/rebase.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 38c496dd10..b79f9b0a9f 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -70,6 +70,7 @@ struct rebase_options {
>   const char *state_dir;
>   struct commit *upstream;
>   const char *upstream_name;
> + const char *upstream_arg;
>   char *head_name;
>   struct object_id orig_head;
>   struct commit *onto;
> @@ -310,6 +311,7 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>   };
>   const char *branch_name;
>   int ret, flags;
> + int ok_to_skip_pre_rebase = 0;
>   struct strbuf msg = STRBUF_INIT;
>   struct strbuf revisions = STRBUF_INIT;
>   struct object_id merge_base;
> @@ -317,6 +319,8 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>   OPT_STRING(0, "onto", _name,
>  N_("revision"),
>  N_("rebase onto given branch instead of upstream")),
> + OPT_BOOL(0, "no-verify", _to_skip_pre_rebase,
> +  N_("allow pre-rebase hook to run")),

Do we need to catch "--no-no-verify" ourselves with NONEG bit, or is
this sufficient to tell parse_options() machinery to take care of
it?

>   OPT_END(),
>   };
>  
> @@ -382,6 +386,7 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>   options.upstream = peel_committish(options.upstream_name);
>   if (!options.upstream)
>   die(_("invalid upstream '%s'"), options.upstream_name);
> + options.upstream_arg = options.upstream_name;
>   } else
>   die("TODO: upstream for --root");
>  
> @@ -430,6 +435,12 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>   die(_("Could not resolve HEAD to a revision"));
>   }
>  
> + /* If a hook exists, give it a chance to interrupt*/
> + if (!ok_to_skip_pre_rebase &&
> + run_hook_le(NULL, "pre-rebase", options.upstream_arg,
> + argc ? argv[0] : NULL, NULL))
> + die(_("The pre-rebase hook refused to rebase."));
> +
>   strbuf_addf(, "rebase: checkout %s", options.onto_name);
>   if (reset_head(>object.oid, "checkout", NULL, 1))
>   die(_("Could not detach HEAD"));

Re: [PATCH 02/11] builtin rebase: support `git rebase --onto A...B`

[Junio C Hamano]

Pratik Karki  writes:

> This commit implements support for an --onto argument that is actually a
> "symmetric range" i.e. `...`.
>
> The equivalent shell script version of the code offers two different
> error messages for the cases where there is no merge base vs more than
> one merge base. Though following the similar approach would be nice,
> this would create more complexity than it is of current. Currently, for

Sorry, but it is unclear what you mean by "than it is of current."
Do you mean we leave it broken at this step in the series for now
for expediency, with the intention to later revisit and fix it, or
do you mean something else?

> simple convenience, the `get_oid_mb()` function is used whose return
> value does not discern between those two error conditions.
>
> Signed-off-by: Pratik Karki 
> ...
> @@ -387,7 +389,11 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>   if (!options.onto_name)
>   options.onto_name = options.upstream_name;
>   if (strstr(options.onto_name, "...")) {
> - die("TODO");
> + if (get_oid_mb(options.onto_name, _base) < 0)
> + die(_("'%s': need exactly one merge base"),
> + options.onto_name);
> + options.onto = lookup_commit_or_die(_base,
> + options.onto_name);

The original is slightly sloppy in that it will misparse

rebase --onto 'master^{/log ... message}'

and this shares the same, which I think is probably OK.  When this
actually becomes problematic, the original can easily be salvaged by
making it to fall back to the same peel_committish in its else
clause; I am not sure if this C rewrite is as easily be fixed the
same way, though.

>   } else {
>   options.onto = peel_committish(options.onto_name);
>   if (!options.onto)

Re: [PATCH 01/11] builtin rebase: support --onto

[Junio C Hamano]

Pratik Karki  writes:

> The `--onto` option is important, as it allows to rebase a range of
> commits onto a different base commit (which gave the command its odd
> name: "rebase").

Is there anything unimportant?  A rhetorical question, of course.

The quite casual and natural use of "to rebase" as a verb in the
first sentence contradicts with what the parenthetical "its odd
name" comment says.  Perhaps drop everything after "(which..."?

i.e.

The `--onto` option allows to rebase a range of commits onto
a different base commit.  Port the support for the option to
the C re-implementation.

> This commit introduces options parsing so that different options can
> be added in future commits.

We usually do not say "This commit does X", or (worse) "I do X in
this commit".  Instead, order the codebase to be like so, e.g.
"Support command line options by adding a call to parse_options();
later commits will add more options by building on top." or
something like that.

> @@ -318,13 +334,22 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>   BUG("sane_execvp() returned???");
>   }
>  
> - if (argc != 2)
> - die(_("Usage: %s "), argv[0]);
> + if (argc == 2 && !strcmp(argv[1], "-h"))
> + usage_with_options(builtin_rebase_usage,
> +builtin_rebase_options);
> +
>   prefix = setup_git_directory();
>   trace_repo_setup(prefix);
>   setup_work_tree();
>
>   git_config(git_default_config, NULL);
> + argc = parse_options(argc, argv, prefix,
> +  builtin_rebase_options,
> +  builtin_rebase_usage, 0);
> +
> + if (argc > 2)
> + usage_with_options(builtin_rebase_usage,
> +builtin_rebase_options);

OK.  This correctly calls the parser after repository setup.

>   switch (options.type) {
>   case REBASE_MERGE:
> @@ -343,10 +368,10 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>   }
>  
>   if (!options.root) {
> - if (argc < 2)
> + if (argc < 1)
>   die("TODO: handle @{upstream}");
>   else {
> - options.upstream_name = argv[1];
> + options.upstream_name = argv[0];
>   argc--;
>   argv++;
>   if (!strcmp(options.upstream_name, "-"))
> @@ -377,7 +402,7 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>* orig_head -- commit object name of tip of the branch before rebasing
>* head_name -- refs/heads/ or "detached HEAD"
>*/
> - if (argc > 1)
> + if (argc > 0)
>die("TODO: handle switch_to");
>   else {
>   /* Do not need to switch branches, we are already on it. */

Re: Page content is wider than view window

[Brady Trainor]

Jeff King  writes:

> Yes, there's work happening already for a visual refresh of the site,
> [...]
>
> -Peff

Awesome! Looking forward to it!

-- 
Brady

[GSoC][PATCH v7 11/26] stash: change `git stash show` usage text and documentation

[Paul-Sebastian Ungureanu]

It is already stated in documentation that it will accept any
option known to `git diff`, but not in the usage text and some
parts of the documentation.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 Documentation/git-stash.txt | 4 ++--
 builtin/stash--helper.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 7ef8c4791..e31ea7d30 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 
 [verse]
 'git stash' list []
-'git stash' show []
+'git stash' show [] []
 'git stash' drop [-q|--quiet] []
 'git stash' ( pop | apply ) [--index] [-q|--quiet] []
 'git stash' branch  []
@@ -106,7 +106,7 @@ stash@{1}: On master: 9cc0589... Add git-stash
 The command takes options applicable to the 'git log'
 command to control what is shown and how. See linkgit:git-log[1].
 
-show []::
+show [] []::
 
Show the changes recorded in the stash entry as a diff between the
stashed contents and the commit back when the stash entry was first
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index e764cd33e..0c1efca6b 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,7 +13,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
-   N_("git stash--helper show []"),
+   N_("git stash--helper show [] []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
N_("git stash--helper branch  []"),
@@ -27,7 +27,7 @@ static const char * const git_stash_helper_list_usage[] = {
 };
 
 static const char * const git_stash_helper_show_usage[] = {
-   N_("git stash--helper show []"),
+   N_("git stash--helper show [] []"),
NULL
 };
 
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 07/26] stash: convert branch to builtin

[Paul-Sebastian Ungureanu]

From: Joel Teichroeb 

Add stash branch to the helper and delete the apply_to_branch
function from the shell script.

Checkout does not currently provide a function for checking out
a branch as cmd_checkout does a large amount of sanity checks
first that we require here.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 44 +
 git-stash.sh| 17 ++--
 2 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index ae719b7fc..1e4d07295 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -14,6 +14,7 @@
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
NULL
 };
@@ -28,6 +29,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_branch_usage[] = {
+   N_("git stash--helper branch  []"),
+   NULL
+};
+
 static const char * const git_stash_helper_clear_usage[] = {
N_("git stash--helper clear"),
NULL
@@ -535,6 +541,42 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+   const char *branch = NULL;
+   int ret;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct stash_info info;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_branch_usage, 0);
+
+   if (argc == 0)
+   return error(_("No branch name specified"));
+
+   branch = argv[0];
+
+   if (get_stash_info(, argc - 1, argv + 1))
+   return -1;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "checkout", "-b", NULL);
+   argv_array_push(, branch);
+   argv_array_push(, oid_to_hex(_commit));
+   ret = run_command();
+   if (!ret)
+   ret = do_apply_stash(prefix, , 1);
+   if (!ret && info.is_stash_ref)
+   ret = do_drop_stash(prefix, );
+
+   free_stash_info();
+
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -561,6 +603,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
return !!drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "branch"))
+   return !!branch_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index a99d5dc9e..29d9f4425 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
clear_stash
 }
 
-apply_to_branch () {
-   test -n "$1" || die "$(gettext "No branch name specified")"
-   branch=$1
-   shift 1
-
-   set -- --index "$@"
-   assert_stash_like "$@"
-
-   git checkout -b $branch $REV^ &&
-   apply_stash "$@" && {
-   test -z "$IS_STASH_REF" || drop_stash "$@"
-   }
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -673,7 +659,8 @@ pop)
;;
 branch)
shift
-   apply_to_branch "$@"
+   cd "$START_DIR"
+   git stash--helper branch "$@"
;;
 *)
case $# in
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 03/26] stash: update test cases conform to coding guidelines

[Paul-Sebastian Ungureanu]

Removed whitespaces after redirection operators.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 t/t3903-stash.sh | 120 ---
 1 file changed, 61 insertions(+), 59 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index af7586d43..de6cab1fe 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -8,22 +8,22 @@ test_description='Test git stash'
 . ./test-lib.sh
 
 test_expect_success 'stash some dirty working directory' '
-   echo 1 > file &&
+   echo 1 >file &&
git add file &&
echo unrelated >other-file &&
git add other-file &&
test_tick &&
git commit -m initial &&
-   echo 2 > file &&
+   echo 2 >file &&
git add file &&
-   echo 3 > file &&
+   echo 3 >file &&
test_tick &&
git stash &&
git diff-files --quiet &&
git diff-index --cached --quiet HEAD
 '
 
-cat > expect << EOF
+cat >expect < output &&
+   git diff stash^2..stash >output &&
test_cmp output expect
 '
 
@@ -74,7 +74,7 @@ test_expect_success 'apply stashed changes' '
 
 test_expect_success 'apply stashed changes (including index)' '
git reset --hard HEAD^ &&
-   echo 6 > other-file &&
+   echo 6 >other-file &&
git add other-file &&
test_tick &&
git commit -m other-file &&
@@ -99,12 +99,12 @@ test_expect_success 'stash drop complains of extra options' 
'
 
 test_expect_success 'drop top stash' '
git reset --hard &&
-   git stash list > stashlist1 &&
-   echo 7 > file &&
+   git stash list >expected &&
+   echo 7 >file &&
git stash &&
git stash drop &&
-   git stash list > stashlist2 &&
-   test_cmp stashlist1 stashlist2 &&
+   git stash list >actual &&
+   test_cmp expected actual &&
git stash apply &&
test 3 = $(cat file) &&
test 1 = $(git show :file) &&
@@ -113,9 +113,9 @@ test_expect_success 'drop top stash' '
 
 test_expect_success 'drop middle stash' '
git reset --hard &&
-   echo 8 > file &&
+   echo 8 >file &&
git stash &&
-   echo 9 > file &&
+   echo 9 >file &&
git stash &&
git stash drop stash@{1} &&
test 2 = $(git stash list | wc -l) &&
@@ -160,7 +160,7 @@ test_expect_success 'stash pop' '
test 0 = $(git stash list | wc -l)
 '
 
-cat > expect << EOF
+cat >expect < expect1 << EOF
+cat >expect1 < expect2 << EOF
+cat >expect2 < file &&
+   echo foo >file &&
git commit file -m first &&
-   echo bar > file &&
-   echo bar2 > file2 &&
+   echo bar >file &&
+   echo bar2 >file2 &&
git add file2 &&
git stash &&
-   echo baz > file &&
+   echo baz >file &&
git commit file -m second &&
git stash branch stashbranch &&
test refs/heads/stashbranch = $(git symbolic-ref HEAD) &&
test $(git rev-parse HEAD) = $(git rev-parse master^) &&
-   git diff --cached > output &&
+   git diff --cached >output &&
test_cmp output expect &&
-   git diff > output &&
+   git diff >output &&
test_cmp output expect1 &&
git add file &&
git commit -m alternate\ second &&
-   git diff master..stashbranch > output &&
+   git diff master..stashbranch >output &&
test_cmp output expect2 &&
test 0 = $(git stash list | wc -l)
 '
 
 test_expect_success 'apply -q is quiet' '
-   echo foo > file &&
+   echo foo >file &&
git stash &&
-   git stash apply -q > output.out 2>&1 &&
+   git stash apply -q >output.out 2>&1 &&
test_must_be_empty output.out
 '
 
 test_expect_success 'save -q is quiet' '
-   git stash save --quiet > output.out 2>&1 &&
+   git stash save --quiet >output.out 2>&1 &&
test_must_be_empty output.out
 '
 
 test_expect_success 'pop -q is quiet' '
-   git stash pop -q > output.out 2>&1 &&
+   git stash pop -q >output.out 2>&1 &&
test_must_be_empty output.out
 '
 
 test_expect_success 'pop -q --index works and is quiet' '
-   echo foo > file &&
+   echo foo >file &&
git add file &&
git stash save --quiet &&
-   git stash pop -q --index > output.out 2>&1 &&
+   git stash pop -q --index >output.out 2>&1 &&
test foo = "$(git show :file)" &&
test_must_be_empty output.out
 '
 
 test_expect_success 'drop -q is quiet' '
git stash &&
-   git stash drop -q > output.out 2>&1 &&
+   git stash drop -q >output.out 2>&1 &&
test_must_be_empty output.out
 '
 
 test_expect_success 'stash -k' '
-   echo bar3 > file &&
-   echo bar4 > file2 &&
+   echo bar3 >file &&
+   echo bar4 >file2 &&
git add file2 &&
git stash -k &&
test bar,bar4 = $(cat file),$(cat file2)
 '
 
 test_expect_success 'stash --no-keep-index' '
-   echo bar33 > file &&
-   echo bar44 > file2 &&
+   echo bar33 >file &&
+   echo 

[GSoC][PATCH v7 17/26] stash: avoid spawning a "diff-index" process

[Paul-Sebastian Ungureanu]

This commits replaces spawning `diff-index` child process by using
the already existing `diff` API
---
 builtin/stash--helper.c | 56 ++---
 1 file changed, 42 insertions(+), 14 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 887b78d05..f905d3908 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
 #include "rerere.h"
 #include "revision.h"
 #include "log-tree.h"
+#include "diffcore.h"
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
@@ -297,6 +298,18 @@ static int reset_head(const char *prefix)
return run_command();
 }
 
+static void add_diff_to_buf(struct diff_queue_struct *q,
+   struct diff_options *options,
+   void *data)
+{
+   int i;
+   for (i = 0; i < q->nr; i++) {
+   struct diff_filepair *p = q->queue[i];
+   strbuf_addstr(data, p->one->path);
+   strbuf_addch(data, '\n');
+   }
+}
+
 static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)
 {
struct child_process cp = CHILD_PROCESS_INIT;
@@ -981,14 +994,16 @@ static int stash_patch(struct stash_info *info, const 
char **argv)
return ret;
 }
 
-static int stash_working_tree(struct stash_info *info, const char **argv)
+static int stash_working_tree(struct stash_info *info,
+ const char **argv, const char *prefix)
 {
int ret = 0;
-   struct child_process cp1 = CHILD_PROCESS_INIT;
struct child_process cp2 = CHILD_PROCESS_INIT;
struct child_process cp3 = CHILD_PROCESS_INIT;
-   struct strbuf out1 = STRBUF_INIT;
struct strbuf out3 = STRBUF_INIT;
+   struct argv_array args = ARGV_ARRAY_INIT;
+   struct strbuf diff_output = STRBUF_INIT;
+   struct rev_info rev;
 
set_alternate_index_output(stash_index_path.buf);
if (reset_tree(>i_tree, 0, 0)) {
@@ -997,26 +1012,36 @@ static int stash_working_tree(struct stash_info *info, 
const char **argv)
}
set_alternate_index_output(".git/index");
 
-   cp1.git_cmd = 1;
-   argv_array_pushl(, "diff-index", "--name-only", "-z",
-   "HEAD", "--", NULL);
+   argv_array_push(, "dummy");
if (argv)
-   argv_array_pushv(, argv);
-   argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
+   argv_array_pushv(, argv);
+   git_config(git_diff_basic_config, NULL);
+   init_revisions(, prefix);
+   args.argc = setup_revisions(args.argc, args.argv, , NULL);
+
+   rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+   rev.diffopt.format_callback = add_diff_to_buf;
+   rev.diffopt.format_callback_data = _output;
+
+   if (read_cache_preload() < 0) {
+   ret = -1;
+   goto done;
+   }
 
-   if (pipe_command(, NULL, 0, , 0, NULL, 0)) {
+   add_pending_object(, parse_object(the_repository, >b_commit), 
"");
+   if (run_diff_index(, 0)) {
ret = -1;
goto done;
}
 
cp2.git_cmd = 1;
-   argv_array_pushl(, "update-index", "-z", "--add",
+   argv_array_pushl(, "update-index", "--add",
 "--remove", "--stdin", NULL);
argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
 stash_index_path.buf);
 
-   if (pipe_command(, out1.buf, out1.len, NULL, 0, NULL, 0)) {
+   if (pipe_command(, diff_output.buf, diff_output.len,
+NULL, 0, NULL, 0)) {
ret = -1;
goto done;
}
@@ -1033,8 +1058,11 @@ static int stash_working_tree(struct stash_info *info, 
const char **argv)
get_oid_hex(out3.buf, >w_tree);
 
 done:
-   strbuf_release();
+   UNLEAK(rev);
strbuf_release();
+   argv_array_clear();
+   object_array_clear();
+   strbuf_release(_output);
remove_path(stash_index_path.buf);
return ret;
 }
@@ -1112,7 +1140,7 @@ static int do_create_stash(int argc, const char **argv, 
const char *prefix,
goto done;
}
} else {
-   if (stash_working_tree(info, argv)) {
+   if (stash_working_tree(info, argv, prefix)) {
printf_ln("Cannot save the current worktree state");
ret = -1;
goto done;
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 15/26] stash: convert create to builtin

[Paul-Sebastian Ungureanu]

Add stash create to the helper.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 406 
 git-stash.sh|   2 +-
 2 files changed, 407 insertions(+), 1 deletion(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 5ff810f8c..a4e57899b 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -21,6 +21,7 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
N_("git stash--helper store [-m|--message ] [-q|--quiet] 
"),
+   N_("git stash--helper create []"),
NULL
 };
 
@@ -64,6 +65,11 @@ static const char * const git_stash_helper_store_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_create_usage[] = {
+   N_("git stash--helper create []"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -781,6 +787,404 @@ static int store_stash(int argc, const char **argv, const 
char *prefix)
return do_store_stash(argv[0], stash_msg, quiet);
 }
 
+/*
+ * `out` will be filled with the names of untracked files. The return value is:
+ *
+ * < 0 if there was a bug (any arg given outside the repo will be detected
+ * by `setup_revision()`)
+ * = 0 if there are not any untracked files
+ * > 0 if there are untracked files
+ */
+static int get_untracked_files(const char **argv, int line_term,
+  int include_untracked, struct strbuf *out)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+   cp.git_cmd = 1;
+   argv_array_pushl(, "ls-files", "-o", NULL);
+   if (line_term)
+   argv_array_push(, "-z");
+   if (include_untracked != 2)
+   argv_array_push(, "--exclude-standard");
+   argv_array_push(, "--");
+   if (argv)
+   argv_array_pushv(, argv);
+
+   if (pipe_command(, NULL, 0, out, 0, NULL, 0))
+   return -1;
+   return out->len;
+}
+
+/*
+ * The return value of `check_changes()` can be:
+ *
+ * < 0 if there was an error
+ * = 0 if there are no changes.
+ * > 0 if there are changes.
+ */
+static int check_changes(const char **argv, int include_untracked,
+const char *prefix)
+{
+   int result;
+   int ret = 0;
+   struct rev_info rev;
+   struct object_id dummy;
+   struct strbuf out = STRBUF_INIT;
+   struct argv_array args = ARGV_ARRAY_INIT;
+
+   init_revisions(, prefix);
+   parse_pathspec(_data, 0, PATHSPEC_PREFER_FULL,
+  prefix, argv);
+
+   rev.diffopt.flags.quick = 1;
+   rev.diffopt.flags.ignore_submodules = 1;
+   rev.abbrev = 0;
+
+   /* No initial commit. */
+   if (get_oid("HEAD", )) {
+   ret = -1;
+   goto done;
+   }
+
+   add_head_to_pending();
+   diff_setup_done();
+
+   if (read_cache() < 0) {
+   ret = -1;
+   goto done;
+   }
+   result = run_diff_index(, 1);
+   if (diff_result_code(, result)) {
+   ret = 1;
+   goto done;
+   }
+
+   object_array_clear();
+   result = run_diff_files(, 0);
+   if (diff_result_code(, result)) {
+   ret = 1;
+   goto done;
+   }
+
+   if (include_untracked && get_untracked_files(argv, 0,
+include_untracked, ))
+   ret = 1;
+
+done:
+   strbuf_release();
+   argv_array_clear();
+   return ret;
+}
+
+static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
+   struct strbuf *out)
+{
+   int ret = 0;
+   struct strbuf untracked_msg = STRBUF_INIT;
+   struct strbuf out2 = STRBUF_INIT;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct child_process cp2 = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "update-index", "-z", "--add",
+"--remove", "--stdin", NULL);
+   argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
+stash_index_path.buf);
+
+   strbuf_addf(_msg, "untracked files on %s\n", msg->buf);
+   if (pipe_command(, out->buf, out->len, NULL, 0, NULL, 0)) {
+   ret = -1;
+   goto done;
+   }
+
+   cp2.git_cmd = 1;
+   argv_array_push(, "write-tree");
+   argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
+stash_index_path.buf);
+   if (pipe_command(, NULL, 0, , 0,NULL, 0)) {
+   ret = -1;
+   goto done;
+   }
+   get_oid_hex(out2.buf, >u_tree);
+
+   if (commit_tree(untracked_msg.buf, untracked_msg.len,
+   >u_tree, NULL, >u_commit, NULL, NULL)) {
+   ret = -1;
+   goto done;
+   }
+
+done:
+   

[GSoC][PATCH v7 26/26] stash: replace all "git apply" child processes with API calls

[Paul-Sebastian Ungureanu]

`apply_all_patches()` does not provide a method to apply patches
from strbuf. Because of this, this commit introduces a new
function `apply_patch_from_buf()` which applies a patch from buf.
It works by saving the strbuf as a file. This way we can call
`apply_all_patches()`. Before returning, the created file is
removed.
---
 builtin/stash.c | 61 +++--
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 46e76a34e..74eda822c 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -13,6 +13,7 @@
 #include "revision.h"
 #include "log-tree.h"
 #include "diffcore.h"
+#include "apply.h"
 
 static const char * const git_stash_usage[] = {
N_("git stash list []"),
@@ -277,10 +278,6 @@ static int diff_tree_binary(struct strbuf *out, struct 
object_id *w_commit)
struct child_process cp = CHILD_PROCESS_INIT;
const char *w_commit_hex = oid_to_hex(w_commit);
 
-   /*
-* Diff-tree would not be very hard to replace with a native function,
-* however it should be done together with apply_cached.
-*/
cp.git_cmd = 1;
argv_array_pushl(, "diff-tree", "--binary", NULL);
argv_array_pushf(, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
@@ -288,18 +285,36 @@ static int diff_tree_binary(struct strbuf *out, struct 
object_id *w_commit)
return pipe_command(, NULL, 0, out, 0, NULL, 0);
 }
 
-static int apply_cached(struct strbuf *out)
+static int apply_patch_from_buf(struct strbuf *patch, int cached, int reverse,
+   int check_index)
 {
-   struct child_process cp = CHILD_PROCESS_INIT;
+   int ret = 0;
+   struct apply_state state;
+   struct argv_array args = ARGV_ARRAY_INIT;
+   const char *patch_path = ".git/stash_patch.patch";
+   FILE *patch_file;
 
-   /*
-* Apply currently only reads either from stdin or a file, thus
-* apply_all_patches would have to be updated to optionally take a
-* buffer.
-*/
-   cp.git_cmd = 1;
-   argv_array_pushl(, "apply", "--cached", NULL);
-   return pipe_command(, out->buf, out->len, NULL, 0, NULL, 0);
+   if (init_apply_state(, NULL))
+   return -1;
+
+   state.cached = cached;
+   state.apply_in_reverse = reverse;
+   state.check_index = check_index;
+   if (state.cached)
+   state.check_index = 1;
+   if (state.check_index)
+   state.unsafe_paths = 0;
+
+   patch_file = fopen(patch_path, "w");
+   strbuf_write(patch, patch_file);
+   fclose(patch_file);
+
+   argv_array_push(, patch_path);
+   ret = apply_all_patches(, args.argc, args.argv, 0);
+
+   remove_path(patch_path);
+   clear_apply_state();
+   return ret;
 }
 
 static int reset_head(const char *prefix)
@@ -418,7 +433,7 @@ static int do_apply_stash(const char *prefix, struct 
stash_info *info,
return -1;
}
 
-   ret = apply_cached();
+   ret = apply_patch_from_buf(, 1, 0, 0);
strbuf_release();
if (ret)
return -1;
@@ -1341,7 +1356,6 @@ static int do_push_stash(int argc, const char **argv, 
const char *prefix,
int i;
struct child_process cp1 = CHILD_PROCESS_INIT;
struct child_process cp2 = CHILD_PROCESS_INIT;
-   struct child_process cp3 = CHILD_PROCESS_INIT;
struct strbuf out = STRBUF_INIT;
 
cp1.git_cmd = 1;
@@ -1365,11 +1379,9 @@ static int do_push_stash(int argc, const char **argv, 
const char *prefix,
if (pipe_command(, NULL, 0, , 0, NULL, 0))
return -1;
 
-   cp3.git_cmd = 1;
-   argv_array_pushl(, "apply", "--index", "-R",
-NULL);
-   if (pipe_command(, out.buf, out.len, NULL, 0, NULL,
-0))
+   discard_cache();
+   read_cache();
+   if (apply_patch_from_buf(, 0, 1, 1))
return -1;
} else {
struct child_process cp = CHILD_PROCESS_INIT;
@@ -1405,12 +1417,7 @@ static int do_push_stash(int argc, const char **argv, 
const char *prefix,
return -1;
}
} else {
-   struct child_process cp = CHILD_PROCESS_INIT;
-
-   cp.git_cmd = 1;
-   argv_array_pushl(, "apply", "-R", NULL);
-
-   if (pipe_command(, patch.buf, patch.len, NULL, 0, NULL, 0)) {
+   if (apply_patch_from_buf(, 0, 1, 0)) {
if (!quiet)

Re: [PATCH 09/11] builtin rebase: start a new rebase only if none is in progress

[Stefan Beller]

On Wed, Aug 8, 2018 at 6:51 AM Pratik Karki  wrote:
>
> To run a new rebase, there needs to be a check to assure that no other
> rebase is in progress. New rebase operation cannot start until an
> ongoing rebase operation completes or is terminated.
>
> Signed-off-by: Pratik Karki 
> ---
>  builtin/rebase.c | 48 +++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 8a7bf3d468..a261f552f1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -87,6 +87,7 @@ struct rebase_options {
> REBASE_VERBOSE = 1<<1,
> REBASE_DIFFSTAT = 1<<2,
> REBASE_FORCE = 1<<3,
> +   REBASE_INTERACTIVE_EXPLICIT = 1<<4,
> } flags;
> struct strbuf git_am_opt;
>  };
> @@ -392,10 +393,11 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
> .git_am_opt = STRBUF_INIT,
> };
> const char *branch_name;
> -   int ret, flags;
> +   int ret, flags, in_progress = 0;
> int ok_to_skip_pre_rebase = 0;
> struct strbuf msg = STRBUF_INIT;
> struct strbuf revisions = STRBUF_INIT;
> +   struct strbuf buf = STRBUF_INIT;
> struct object_id merge_base;
> struct option builtin_rebase_options[] = {
> OPT_STRING(0, "onto", _name,
> @@ -447,6 +449,30 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>
> git_config(rebase_config, );
>
> +   if (is_directory(apply_dir())) {
> +   options.type = REBASE_AM;
> +   options.state_dir = apply_dir();
> +   } else if (is_directory(merge_dir())) {
> +   strbuf_reset();
> +   strbuf_addf(, "%s/rewritten", merge_dir());
> +   if (is_directory(buf.buf)) {
> +   options.type = REBASE_PRESERVE_MERGES;
> +   options.flags |= REBASE_INTERACTIVE_EXPLICIT;
> +   } else {
> +   strbuf_reset();
> +   strbuf_addf(, "%s/interactive", merge_dir());
> +   if(file_exists(buf.buf)) {
> +   options.type = REBASE_INTERACTIVE;
> +   options.flags |= REBASE_INTERACTIVE_EXPLICIT;
> +   } else
> +   options.type = REBASE_MERGE;
> +   }
> +   options.state_dir = merge_dir();
> +   }
> +
> +   if (options.type != REBASE_UNSPECIFIED)
> +   in_progress = 1;
> +
> argc = parse_options(argc, argv, prefix,
>  builtin_rebase_options,
>  builtin_rebase_usage, 0);
> @@ -455,6 +481,26 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
> usage_with_options(builtin_rebase_usage,
>builtin_rebase_options);
>
> +   /* Make sure no rebase is in progress */

The faithful conversion doesn't even stop at the comments. ;-)
I shortly wondered if this is the best place for this comment,
but let's just keep it here to have the 1:1 rewrite.


> +   if (in_progress) {
[...]
> +   state_dir_base, cmd_live_rebase,buf.buf);

In case a resend is needed, add a whitespace after the
comma and buf.buf, please.

So far I have not seen anything major that would warrant a resend.

Thanks,
Stefan

[GSoC][PATCH v7 04/26] stash: renamed test cases to be more descriptive

[Paul-Sebastian Ungureanu]

Renamed some test cases' labels to be more descriptive and under 80
characters per line.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 t/t3903-stash.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index de6cab1fe..8d002a7f2 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -604,7 +604,7 @@ test_expect_success 'stash show -p - no stashes on stack, 
stash-like argument' '
test_cmp expected actual
 '
 
-test_expect_success 'stash drop - fail early if specified stash is not a stash 
reference' '
+test_expect_success 'drop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -618,7 +618,7 @@ test_expect_success 'stash drop - fail early if specified 
stash is not a stash r
git reset --hard HEAD
 '
 
-test_expect_success 'stash pop - fail early if specified stash is not a stash 
reference' '
+test_expect_success 'pop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -682,7 +682,7 @@ test_expect_success 'invalid ref of the form "n", n >= N' '
git stash drop
 '
 
-test_expect_success 'stash branch should not drop the stash if the branch 
exists' '
+test_expect_success 'branch: should not drop the stash if the branch exists' '
git stash clear &&
echo foo >file &&
git add file &&
@@ -693,7 +693,7 @@ test_expect_success 'stash branch should not drop the stash 
if the branch exists
git rev-parse stash@{0} --
 '
 
-test_expect_success 'stash branch should not drop the stash if the apply 
fails' '
+test_expect_success 'branch: should not drop the stash if the apply fails' '
git stash clear &&
git reset HEAD~1 --hard &&
echo foo >file &&
@@ -707,7 +707,7 @@ test_expect_success 'stash branch should not drop the stash 
if the apply fails'
git rev-parse stash@{0} --
 '
 
-test_expect_success 'stash apply shows status same as git status (relative to 
current directory)' '
+test_expect_success 'apply: shows same status as git status (relative to ./)' '
git stash clear &&
echo 1 >subdir/subfile1 &&
echo 2 >subdir/subfile2 &&
@@ -1048,7 +1048,7 @@ test_expect_success 'stash push -p with pathspec shows no 
changes only once' '
test_i18ncmp expect actual
 '
 
-test_expect_success 'stash push with pathspec shows no changes when there are 
none' '
+test_expect_success 'push:  shows no changes when there are none' '
>foo &&
git add foo &&
git commit -m "tmp" &&
@@ -1058,7 +1058,7 @@ test_expect_success 'stash push with pathspec shows no 
changes when there are no
test_i18ncmp expect actual
 '
 
-test_expect_success 'stash push with pathspec not in the repository errors 
out' '
+test_expect_success 'push:  not in the repository errors out' '
>untracked &&
test_must_fail git stash push untracked &&
test_path_is_file untracked
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 12/26] stash: refactor `show_stash()` to use the diff API

[Paul-Sebastian Ungureanu]

Currently, `show_stash()` uses `cmd_diff()` to generate
the output. After this commit, the output will be generated
using the internal API.

Before this commit, `git stash show --quiet` would act like
`git diff` and error out if the stash is not empty. Now, the
`--quiet` option does not error out given an empty stash.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 73 +
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 0c1efca6b..ec8c38c6f 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -10,6 +10,8 @@
 #include "run-command.h"
 #include "dir.h"
 #include "rerere.h"
+#include "revision.h"
+#include "log-tree.h"
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
@@ -662,56 +664,71 @@ static int git_stash_config(const char *var, const char 
*value, void *cb)
 
 static int show_stash(int argc, const char **argv, const char *prefix)
 {
-   int i, ret = 0;
-   struct child_process cp = CHILD_PROCESS_INIT;
-   struct argv_array args_refs = ARGV_ARRAY_INIT;
+   int i;
+   int flags = 0;
struct stash_info info;
+   struct rev_info rev;
+   struct argv_array stash_args = ARGV_ARRAY_INIT;
struct option options[] = {
OPT_END()
};
 
-   argc = parse_options(argc, argv, prefix, options,
-git_stash_helper_show_usage,
-PARSE_OPT_KEEP_UNKNOWN);
+   init_diff_ui_defaults();
+   git_config(git_diff_ui_config, NULL);
 
-   cp.git_cmd = 1;
-   argv_array_push(, "diff");
+   init_revisions(, prefix);
 
-   /* Push arguments which are not options into args_refs. */
-   for (i = 0; i < argc; ++i) {
-   if (argv[i][0] == '-')
-   argv_array_push(, argv[i]);
+   /* Push arguments which are not options into stash_args. */
+   for (i = 1; i < argc; ++i) {
+   if (argv[i][0] != '-')
+   argv_array_push(_args, argv[i]);
else
-   argv_array_push(_refs, argv[i]);
-   }
-
-   if (get_stash_info(, args_refs.argc, args_refs.argv)) {
-   child_process_clear();
-   argv_array_clear(_refs);
-   return -1;
+   flags++;
}
 
/*
 * The config settings are applied only if there are not passed
 * any flags.
 */
-   if (cp.args.argc == 1) {
+   if (!flags) {
git_config(git_stash_config, NULL);
if (show_stat)
-   argv_array_push(, "--stat");
+   rev.diffopt.output_format |= DIFF_FORMAT_DIFFSTAT;
+   if (show_patch) {
+   rev.diffopt.output_format = ~DIFF_FORMAT_NO_OUTPUT;
+   rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+   }
+   }
 
-   if (show_patch)
-   argv_array_push(, "-p");
+   if (get_stash_info(, stash_args.argc, stash_args.argv)) {
+   argv_array_clear(_args);
+   return -1;
}
 
-   argv_array_pushl(, oid_to_hex(_commit),
-oid_to_hex(_commit), NULL);
+   argc = setup_revisions(argc, argv, , NULL);
+   if (!rev.diffopt.output_format)
+   rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+   diff_setup_done();
+   rev.diffopt.flags.recursive = 1;
+   setup_diff_pager();
 
-   ret = run_command();
+   /*
+* We can return early if there was any option not recognised by
+* `diff_opt_parse()`, besides the word `stash`.
+*/
+   if (argc > 1) {
+   free_stash_info();
+   argv_array_clear(_args);
+   usage_with_options(git_stash_helper_show_usage, options);
+   }
+
+   /* Do the diff thing. */
+   diff_tree_oid(_commit, _commit, "", );
+   log_tree_diff_flush();
 
free_stash_info();
-   argv_array_clear(_refs);
-   return ret;
+   argv_array_clear(_args);
+   return 0;
 }
 
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 18/26] stash: convert push to builtin

[Paul-Sebastian Ungureanu]

Add stash push to the helper.
---
 builtin/stash--helper.c | 209 
 git-stash.sh|   6 +-
 2 files changed, 213 insertions(+), 2 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index f905d3908..c26cad3d5 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -23,6 +23,9 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper clear"),
N_("git stash--helper store [-m|--message ] [-q|--quiet] 
"),
N_("git stash--helper create []"),
+   N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+  "  [-u|--include-untracked] [-a|--all] [-m|--message 
]\n"
+  "  [--] [...]]"),
NULL
 };
 
@@ -71,6 +74,13 @@ static const char * const git_stash_helper_create_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_push_usage[] = {
+   N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+  "  [-u|--include-untracked] [-a|--all] [-m|--message 
]\n"
+  "  [--] [...]]"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -1210,6 +1220,203 @@ static int create_stash(int argc, const char **argv, 
const char *prefix)
return ret < 0;
 }
 
+static int do_push_stash(int argc, const char **argv, const char *prefix,
+int keep_index, int patch_mode, int include_untracked,
+int quiet, const char *stash_msg)
+{
+   int ret = 0;
+   struct pathspec ps;
+   struct stash_info info;
+   if (patch_mode && keep_index == -1)
+   keep_index = 1;
+
+   if (patch_mode && include_untracked) {
+   fprintf_ln(stderr, "Can't use --patch and --include-untracked 
or --all at the same time");
+   return -1;
+   }
+
+   parse_pathspec(, 0, PATHSPEC_PREFER_FULL, prefix, argv);
+
+   if (read_cache() < 0)
+   die(_("index file corrupt"));
+
+   if (!include_untracked && ps.nr) {
+   int i;
+   char *ps_matched = xcalloc(ps.nr, 1);
+
+   for (i = 0; i < active_nr; ++i) {
+   const struct cache_entry *ce = active_cache[i];
+   if (!ce_path_match(ce, , ps_matched))
+   continue;
+   }
+
+   if (report_path_error(ps_matched, , prefix)) {
+   fprintf_ln(stderr, "Did you forget to 'git add'?");
+   return -1;
+   }
+   }
+
+   read_cache_preload(NULL);
+   if (refresh_cache(REFRESH_QUIET))
+   return -1;
+
+   if (!check_changes(argv, include_untracked, prefix)) {
+   fprintf_ln(stdout, "No local changes to save");
+   return 0;
+   }
+
+   if (!reflog_exists(ref_stash) && do_clear_stash()) {
+   fprintf_ln(stderr, "Cannot initialize stash");
+   return -1;
+   }
+
+   if ((ret = do_create_stash(argc, argv, prefix, _msg,
+  include_untracked, patch_mode, )))
+   return ret;
+
+   if (do_store_stash(oid_to_hex(_commit), stash_msg, 1)) {
+   fprintf(stderr, "Cannot save the current status");
+   return -1;
+   }
+
+   fprintf(stdout, "Saved working directory and index state %s", 
stash_msg);
+
+   if (!patch_mode) {
+   if (include_untracked && ps.nr == 0) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "clean", "--force",
+"--quiet", "-d", NULL);
+   if (include_untracked == 2)
+   argv_array_push(, "-x");
+   if (run_command())
+   return -1;
+   }
+   if (argc != 0) {
+   int i;
+   struct child_process cp1 = CHILD_PROCESS_INIT;
+   struct child_process cp2 = CHILD_PROCESS_INIT;
+   struct child_process cp3 = CHILD_PROCESS_INIT;
+   struct strbuf out = STRBUF_INIT;
+
+   cp1.git_cmd = 1;
+   argv_array_push(, "add");
+   if (!include_untracked)
+   argv_array_push(, "-u");
+   if (include_untracked == 2)
+   argv_array_push(, "--force");
+   argv_array_push(, "--");
+   for (i = 0; i < ps.nr; ++i)
+   argv_array_push(, ps.items[i].match);
+   if (run_command())
+   return 

[GSoC][PATCH v7 06/26] stash: convert drop and clear to builtin

[Paul-Sebastian Ungureanu]

From: Joel Teichroeb 

Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.

We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 115 
 git-stash.sh|   4 +-
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index ef6a9d30d..ae719b7fc 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,7 +12,14 @@
 #include "rerere.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper clear"),
+   NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
NULL
 };
 
@@ -21,6 +28,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_clear_usage[] = {
+   N_("git stash--helper clear"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -140,6 +152,31 @@ static int get_stash_info(struct stash_info *info, int 
argc, const char **argv)
return !(ret == 0 || ret == 1);
 }
 
+static int do_clear_stash(void)
+{
+   struct object_id obj;
+   if (get_oid(ref_stash, ))
+   return 0;
+
+   return delete_ref(NULL, ref_stash, , 0);
+}
+
+static int clear_stash(int argc, const char **argv, const char *prefix)
+{
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_clear_usage,
+PARSE_OPT_STOP_AT_NON_OPTION);
+
+   if (argc != 0)
+   return error(_("git stash--helper clear with parameters is 
unimplemented"));
+
+   return do_clear_stash();
+}
+
 static int reset_tree(struct object_id *i_tree, int update, int reset)
 {
struct unpack_trees_options opts;
@@ -424,6 +461,80 @@ static int apply_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int do_drop_stash(const char *prefix, struct stash_info *info)
+{
+   struct child_process cp_reflog = CHILD_PROCESS_INIT;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   int ret;
+
+   /*
+* reflog does not provide a simple function for deleting refs. One will
+* need to be added to avoid implementing too much reflog code here
+*/
+
+   cp_reflog.git_cmd = 1;
+   argv_array_pushl(_reflog.args, "reflog", "delete", "--updateref",
+"--rewrite", NULL);
+   argv_array_push(_reflog.args, info->revision.buf);
+   ret = run_command(_reflog);
+   if (!ret) {
+   if (!quiet)
+   printf(_("Dropped %s (%s)\n"), info->revision.buf,
+  oid_to_hex(>w_commit));
+   } else {
+   return error(_("%s: Could not drop stash entry"),
+info->revision.buf);
+   }
+
+   /*
+* This could easily be replaced by get_oid, but currently it will throw
+* a fatal error when a reflog is empty, which we can not recover from.
+*/
+   cp.git_cmd = 1;
+   /* Even though --quiet is specified, rev-parse still outputs the hash */
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_pushf(, "%s@{0}", ref_stash);
+   ret = run_command();
+
+   /* do_clear_stash if we just dropped the last stash entry */
+   if (ret)
+   do_clear_stash();
+
+   return 0;
+}
+
+static void assert_stash_ref(struct stash_info *info)
+{
+   if (!info->is_stash_ref) {
+   free_stash_info(info);
+   error(_("'%s' is not a stash reference"), info->revision.buf);
+   exit(128);
+   }
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+   struct stash_info info;
+   int ret;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_drop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   assert_stash_ref();
+
+   ret = do_drop_stash(prefix, );
+   free_stash_info();
+   return ret;
+}
+
 int 

[GSoC][PATCH v7 01/26] sha1-name.c: added 'get_oidf', which acts like 'get_oid'

[Paul-Sebastian Ungureanu]

Compared to 'get_oid', 'get_oidf' has as parameters a
printf format string and the additional arguments. This
will help simplify the code in subsequent commits.

Original-idea-by: Johannes Schindelin 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 cache.h |  1 +
 sha1-name.c | 19 +++
 2 files changed, 20 insertions(+)

diff --git a/cache.h b/cache.h
index 8dc7134f0..4ec09b336 100644
--- a/cache.h
+++ b/cache.h
@@ -1321,6 +1321,7 @@ struct object_context {
GET_OID_BLOB)
 
 extern int get_oid(const char *str, struct object_id *oid);
+extern int get_oidf(struct object_id *oid, const char *fmt, ...);
 extern int get_oid_commit(const char *str, struct object_id *oid);
 extern int get_oid_committish(const char *str, struct object_id *oid);
 extern int get_oid_tree(const char *str, struct object_id *oid);
diff --git a/sha1-name.c b/sha1-name.c
index c9cc1318b..261b960bb 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1471,6 +1471,25 @@ int get_oid(const char *name, struct object_id *oid)
return get_oid_with_context(name, 0, oid, );
 }
 
+/*
+ * This returns a non-zero value if the string (built using printf
+ * format and the given arguments) is not a valid object.
+ */
+int get_oidf(struct object_id *oid, const char *fmt, ...)
+{
+   va_list ap;
+   int ret;
+   struct strbuf sb = STRBUF_INIT;
+
+   va_start(ap, fmt);
+   strbuf_vaddf(, fmt, ap);
+   va_end(ap);
+
+   ret = get_oid(sb.buf, oid);
+   strbuf_release();
+
+   return ret;
+}
 
 /*
  * Many callers know that the user meant to name a commit-ish by
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 13/26] stash: update `git stash show` documentation

[Paul-Sebastian Ungureanu]

Add in documentation about the change of behavior regarding
the `--quiet` option, which was introduced in the last commit.
(the `--quiet` option does not exit anymore with erorr if it
is given an empty stash as argument)

Signed-off-by: Paul-Sebastian Ungureanu 
---
 Documentation/git-stash.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index e31ea7d30..d60ebdb96 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -117,6 +117,9 @@ show [] []::
You can use stash.showStat and/or stash.showPatch config variables
to change the default behavior.
 
+   It accepts any option known to `git diff`, but acts different on
+   `--quiet` option and exit with zero regardless of differences.
+
 pop [--index] [-q|--quiet] []::
 
Remove a single stashed state from the stash list and apply it
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 23/26] stash: convert `stash--helper.c` into `stash.c`

[Paul-Sebastian Ungureanu]

The old shell script `git-stash.sh`  was removed and replaced
entirely by `builtin/stash.c`. In order to do that, `create` and
`push` were adapted to work without `stash.sh`. For example, before
this commit, `git stash create` called `git stash--helper create
--message "$*"`. If it called `git stash--helper create "$@"`, then
some of these changes wouldn't have been necessary.

This commit also removes the word `helper` since now stash is
called directly and not by a shell script.
---
 .gitignore   |   1 -
 Makefile |   3 +-
 builtin.h|   2 +-
 builtin/{stash--helper.c => stash.c} | 132 ---
 git-stash.sh | 153 ---
 git.c|   2 +-
 6 files changed, 74 insertions(+), 219 deletions(-)
 rename builtin/{stash--helper.c => stash.c} (91%)
 delete mode 100755 git-stash.sh

diff --git a/.gitignore b/.gitignore
index 3abbba81a..3284a1e9b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -156,7 +156,6 @@
 /git-show-ref
 /git-stage
 /git-stash
-/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index af82bc7ee..c41e31ad8 100644
--- a/Makefile
+++ b/Makefile
@@ -612,7 +612,6 @@ SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
@@ -1083,7 +1082,7 @@ BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-index.o
 BUILTIN_OBJS += builtin/show-ref.o
-BUILTIN_OBJS += builtin/stash--helper.o
+BUILTIN_OBJS += builtin/stash.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 55ceb1574..e8102d825 100644
--- a/builtin.h
+++ b/builtin.h
@@ -222,7 +222,7 @@ extern int cmd_show(int argc, const char **argv, const char 
*prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_show_index(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
-extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
+extern int cmd_stash(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash.c
similarity index 91%
rename from builtin/stash--helper.c
rename to builtin/stash.c
index f54a476e3..0ef88408a 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash.c
@@ -14,77 +14,77 @@
 #include "log-tree.h"
 #include "diffcore.h"
 
-static const char * const git_stash_helper_usage[] = {
-   N_("git stash--helper list []"),
-   N_("git stash--helper show [] []"),
-   N_("git stash--helper drop [-q|--quiet] []"),
-   N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
-   N_("git stash--helper branch  []"),
-   N_("git stash--helper clear"),
-   N_("git stash--helper store [-m|--message ] [-q|--quiet] 
"),
-   N_("git stash--helper create []"),
-   N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+static const char * const git_stash_usage[] = {
+   N_("git stash list []"),
+   N_("git stash show [] []"),
+   N_("git stash drop [-q|--quiet] []"),
+   N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+   N_("git stash branch  []"),
+   N_("git stash clear"),
+   N_("git stash store [-m|--message ] [-q|--quiet] "),
+   N_("git stash create []"),
+   N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
   "  [-u|--include-untracked] [-a|--all] [-m|--message 
]\n"
   "  [--] [...]]"),
-   N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+   N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
   "  [-u|--include-untracked] [-a|--all] []"),
NULL
 };
 
-static const char * const git_stash_helper_list_usage[] = {
-   N_("git stash--helper list []"),
+static const char * const git_stash_list_usage[] = {
+   N_("git stash list []"),
NULL
 };
 
-static const char * const git_stash_helper_show_usage[] = {
-   N_("git stash--helper show [] []"),
+static const char * const git_stash_show_usage[] = {
+   N_("git stash show [] []"),
NULL
 };
 
-static const char * const git_stash_helper_drop_usage[] = {
-   N_("git stash--helper drop [-q|--quiet] []"),
+static const char * const git_stash_drop_usage[] = {
+   N_("git 

[GSoC][PATCH v7 00/26] Convert "git stash" to C builtin

[Paul-Sebastian Ungureanu]

Hello,

Here is the whole `git stash` C version. Some of the previous
patches were already reviewed (up to and including "stash: convert
store to builtin"), but there are some which were not
(starting with "stash: convert create to builtin").

In order to see the difference between the shell version and
the C version, I ran `time` on:

* git test suite (t3903-stash.sh, t3904-stash-patch.sh,
t3905-stash-include-untracked.sh and t3906-stash-submodule.sh)

t3903-stash.sh:
** SHELL: 12,69s user 9,95s system 109% cpu 20,730 total
** C:  2,67s user 2,84s system 105% cpu  5,206 total

t3904-stash-patch.sh:
** SHELL: 1,43s user 0,94s system 106% cpu 2,242 total
** C: 1,01s user 0,58s system 104% cpu 1,530 total

t3905-stash-include-untracked.sh
** SHELL: 2,22s user 1,73s system 110% cpu 3,569 total
** C: 0,59s user 0,57s system 106% cpu 1,085 total

t3906-stash-submodule.sh
** SHELL: 2,89s user 2,99s system 106% cpu 5,527 total
** C: 2,21s user 2,61s system 105% cpu 4,568 total

TOTAL:
** SHELL: 19.23s user 15.61s system
** C:  6.48s user  6.60s system

* a git repository with 4000 files: 1000 not changed,
1000 staged files, 1000 unstaged files, 1000 untracked.
In this case I ran some of the most used commands:

git stash push:

** SHELL: 0,12s user 0,21s system 101% cpu 0,329 total
** C: 0,06s user 0,13s system 105% cpu 0,185 total

git stash push -u:

** SHELL: 0,18s user 0,27s system  108% cpu 0,401 total
** C: 0,09s user 0,19s system  103% cpu 0,267 total

git stash pop:

** SHELL: 0,16s user 0,26s system 103% cpu 0,399 total
** C: 0,13s user 0,19s system 102% cpu 0,308 total

Best regards,
Paul Ungureanu


Joel Teichroeb (5):
  stash: improve option parsing test coverage
  stash: convert apply to builtin
  stash: convert drop and clear to builtin
  stash: convert branch to builtin
  stash: convert pop to builtin

Paul-Sebastian Ungureanu (21):
  sha1-name.c: added 'get_oidf', which acts like 'get_oid'
  stash: update test cases conform to coding guidelines
  stash: renamed test cases to be more descriptive
  stash: implement the "list" command in the builtin
  stash: convert show to builtin
  stash: change `git stash show` usage text and documentation
  stash: refactor `show_stash()` to use the diff API
  stash: update `git stash show` documentation
  stash: convert store to builtin
  stash: convert create to builtin
  stash: replace spawning a "read-tree" process
  stash: avoid spawning a "diff-index" process
  stash: convert push to builtin
  stash: make push to be quiet
  stash: add tests for `git stash push -q`
  stash: replace spawning `git ls-files` child process
  stash: convert save to builtin
  stash: convert `stash--helper.c` into `stash.c`
  stash: optimize `get_untracked_files()` and `check_changes()`
  stash: replace all `write-tree` child processes with API calls
  stash: replace all "git apply" child processes with API calls

 Documentation/git-stash.txt |7 +-
 Makefile|2 +-
 builtin.h   |1 +
 builtin/stash.c | 1562 +++
 cache.h |1 +
 git-stash.sh|  752 -
 git.c   |1 +
 sha1-name.c |   19 +
 t/t3903-stash.sh|  190 +++--
 9 files changed, 1714 insertions(+), 821 deletions(-)
 create mode 100644 builtin/stash.c
 delete mode 100755 git-stash.sh

-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 05/26] stash: convert apply to builtin

[Paul-Sebastian Ungureanu]

From: Joel Teichroeb 

Add a builtin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
lets conversion get started without the other commands being
finished.

The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.

Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 452 
 git-stash.sh|  78 +--
 git.c   |   1 +
 6 files changed, 463 insertions(+), 71 deletions(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/.gitignore b/.gitignore
index 3284a1e9b..3abbba81a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -156,6 +156,7 @@
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index bc4fc8eea..af82bc7ee 100644
--- a/Makefile
+++ b/Makefile
@@ -1083,6 +1083,7 @@ BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-index.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce2..55ceb1574 100644
--- a/builtin.h
+++ b/builtin.h
@@ -222,6 +222,7 @@ extern int cmd_show(int argc, const char **argv, const char 
*prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_show_index(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 0..ef6a9d30d
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,452 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+#include "rerere.h"
+
+static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet;
+static struct strbuf stash_index_path = STRBUF_INIT;
+
+/*
+ * w_commit is set to the commit containing the working tree
+ * b_commit is set to the base commit
+ * i_commit is set to the commit containing the index tree
+ * u_commit is set to the commit containing the untracked files tree
+ * w_tree is set to the working tree
+ * b_tree is set to the base tree
+ * i_tree is set to the index tree
+ * u_tree is set to the untracked files tree
+ */
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   struct strbuf revision;
+   int is_stash_ref;
+   int has_u;
+};
+
+static void free_stash_info(struct stash_info *info)
+{
+   strbuf_release(>revision);
+}
+
+static void assert_stash_like(struct stash_info *info, const char * revision)
+{
+   if (get_oidf(>b_commit, "%s^1", revision) ||
+   get_oidf(>w_tree, "%s:", revision) ||
+   get_oidf(>b_tree, "%s^1:", revision) ||
+   get_oidf(>i_tree, "%s^2:", revision)) {
+   free_stash_info(info);
+   error(_("'%s' is not a stash-like commit"), revision);
+   exit(128);
+   }
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+   struct strbuf symbolic = STRBUF_INIT;
+   int ret;
+   const char *revision;
+   const char *commit = NULL;
+   char *end_of_rev;
+   char *expanded_ref;
+   struct object_id dummy;
+
+   if (argc > 1) {
+   int i;
+   struct strbuf refs_msg = STRBUF_INIT;
+   for (i = 0; i < argc; ++i)
+   strbuf_addf(_msg, " '%s'", 

[GSoC][PATCH v7 09/26] stash: implement the "list" command in the builtin

[Paul-Sebastian Ungureanu]

Add stash list to the helper and delete the list_stash function
from the shell script.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 31 +++
 git-stash.sh|  7 +--
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index d6bd468e0..daa4d0034 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
 #include "rerere.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper list []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
N_("git stash--helper branch  []"),
@@ -19,6 +20,11 @@ static const char * const git_stash_helper_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_list_usage[] = {
+   N_("git stash--helper list []"),
+   NULL
+};
+
 static const char * const git_stash_helper_drop_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
NULL
@@ -609,6 +615,29 @@ static int branch_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int list_stash(int argc, const char **argv, const char *prefix)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_list_usage,
+PARSE_OPT_KEEP_UNKNOWN);
+
+   if (!ref_exists(ref_stash))
+   return 0;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "log", "--format=%gd: %gs", "-g",
+"--first-parent", "-m", NULL);
+   argv_array_pushv(, argv);
+   argv_array_push(, ref_stash);
+   argv_array_push(, "--");
+   return run_command();
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -639,6 +668,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
return !!branch_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "list"))
+   return !!list_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 8f2640fe9..6052441aa 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -382,11 +382,6 @@ have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
 }
 
-list_stash () {
-   have_stash || return 0
-   git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash --
-}
-
 show_stash () {
ALLOW_UNKNOWN_FLAGS=t
assert_stash_like "$@"
@@ -574,7 +569,7 @@ test -n "$seen_non_option" || set "push" "$@"
 case "$1" in
 list)
shift
-   list_stash "$@"
+   git stash--helper list "$@"
;;
 show)
shift
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 10/26] stash: convert show to builtin

[Paul-Sebastian Ungureanu]

Add stash show to the helper and delete the show_stash, have_stash,
assert_stash_like, is_stash_like and parse_flags_and_rev functions
from the shell script now that they are no longer needed.

Before this commit, `git stash show` would ignore `--index` and
`--quiet` options. Now, `git stash show` errors out on `--index`
and does not display any message on `--quiet`, but errors out
if the stash is not empty.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c |  78 
 git-stash.sh| 132 +---
 2 files changed, 79 insertions(+), 131 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index daa4d0034..e764cd33e 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,6 +13,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
+   N_("git stash--helper show []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
N_("git stash--helper branch  []"),
@@ -25,6 +26,11 @@ static const char * const git_stash_helper_list_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_show_usage[] = {
+   N_("git stash--helper show []"),
+   NULL
+};
+
 static const char * const git_stash_helper_drop_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
NULL
@@ -638,6 +644,76 @@ static int list_stash(int argc, const char **argv, const 
char *prefix)
return run_command();
 }
 
+static int show_stat = 1;
+static int show_patch;
+
+static int git_stash_config(const char *var, const char *value, void *cb)
+{
+   if (!strcmp(var, "stash.showStat")) {
+   show_stat = git_config_bool(var, value);
+   return 0;
+   }
+   if (!strcmp(var, "stash.showPatch")) {
+   show_patch = git_config_bool(var, value);
+   return 0;
+   }
+   return git_default_config(var, value, cb);
+}
+
+static int show_stash(int argc, const char **argv, const char *prefix)
+{
+   int i, ret = 0;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct argv_array args_refs = ARGV_ARRAY_INIT;
+   struct stash_info info;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_show_usage,
+PARSE_OPT_KEEP_UNKNOWN);
+
+   cp.git_cmd = 1;
+   argv_array_push(, "diff");
+
+   /* Push arguments which are not options into args_refs. */
+   for (i = 0; i < argc; ++i) {
+   if (argv[i][0] == '-')
+   argv_array_push(, argv[i]);
+   else
+   argv_array_push(_refs, argv[i]);
+   }
+
+   if (get_stash_info(, args_refs.argc, args_refs.argv)) {
+   child_process_clear();
+   argv_array_clear(_refs);
+   return -1;
+   }
+
+   /*
+* The config settings are applied only if there are not passed
+* any flags.
+*/
+   if (cp.args.argc == 1) {
+   git_config(git_stash_config, NULL);
+   if (show_stat)
+   argv_array_push(, "--stat");
+
+   if (show_patch)
+   argv_array_push(, "-p");
+   }
+
+   argv_array_pushl(, oid_to_hex(_commit),
+oid_to_hex(_commit), NULL);
+
+   ret = run_command();
+
+   free_stash_info();
+   argv_array_clear(_refs);
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -670,6 +746,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!branch_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "list"))
return !!list_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "show"))
+   return !!show_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 6052441aa..0d05cbc1e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -378,35 +378,6 @@ save_stash () {
fi
 }
 
-have_stash () {
-   git rev-parse --verify --quiet $ref_stash >/dev/null
-}
-
-show_stash () {
-   ALLOW_UNKNOWN_FLAGS=t
-   assert_stash_like "$@"
-
-   if test -z "$FLAGS"
-   then
-   if test "$(git config --bool stash.showStat || echo true)" = 
"true"
-   then
-   FLAGS=--stat
-   fi
-
-   if test "$(git config --bool stash.showPatch || echo false)" = 
"true"
-   then
-   FLAGS=${FLAGS}${FLAGS:+ }-p
-   fi
-
- 

[GSoC][PATCH v7 08/26] stash: convert pop to builtin

[Paul-Sebastian Ungureanu]

From: Joel Teichroeb 

Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref functions from the shell script now that they
are no longer needed.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 36 ++-
 git-stash.sh| 47 ++---
 2 files changed, 37 insertions(+), 46 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 1e4d07295..d6bd468e0 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,7 +13,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
-   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
NULL
@@ -24,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_pop_usage[] = {
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
+   NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
NULL
@@ -541,6 +546,33 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+   int index = 0, ret;
+   struct stash_info info;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_BOOL(0, "index", ,
+   N_("attempt to recreate the index")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_pop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   assert_stash_ref();
+   if ((ret = do_apply_stash(prefix, , index)))
+   printf_ln(_("The stash entry is kept in case you need it 
again."));
+   else
+   ret = do_drop_stash(prefix, );
+
+   free_stash_info();
+   return ret;
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
const char *branch = NULL;
@@ -603,6 +635,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
return !!drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "pop"))
+   return !!pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
return !!branch_stash(argc, argv, prefix);
 
diff --git a/git-stash.sh b/git-stash.sh
index 29d9f4425..8f2640fe9 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -554,50 +554,6 @@ assert_stash_like() {
}
 }
 
-is_stash_ref() {
-   is_stash_like "$@" && test -n "$IS_STASH_REF"
-}
-
-assert_stash_ref() {
-   is_stash_ref "$@" || {
-   args="$*"
-   die "$(eval_gettext "'\$args' is not a stash reference")"
-   }
-}
-
-apply_stash () {
-   cd "$START_DIR"
-   git stash--helper apply "$@"
-   res=$?
-   cd_to_toplevel
-   return $res
-}
-
-pop_stash() {
-   assert_stash_ref "$@"
-
-   if apply_stash "$@"
-   then
-   drop_stash "$@"
-   else
-   status=$?
-   say "$(gettext "The stash entry is kept in case you need it 
again.")"
-   exit $status
-   fi
-}
-
-drop_stash () {
-   assert_stash_ref "$@"
-
-   git reflog delete --updateref --rewrite "${REV}" &&
-   say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
-   die "$(eval_gettext "\${REV}: Could not drop stash entry")"
-
-   # clear_stash if we just dropped the last stash entry
-   git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
-   clear_stash
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -655,7 +611,8 @@ drop)
;;
 pop)
shift
-   pop_stash "$@"
+   cd "$START_DIR"
+   git stash--helper pop "$@"
;;
 branch)
shift
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 24/26] stash: optimize `get_untracked_files()` and `check_changes()`

[Paul-Sebastian Ungureanu]

This commits introduces a optimization by avoiding calling the
same functions again. For example, `git stash push -u`
would call at some points the following functions:

 * `check_changes()`
 * `do_create_stash()`, which calls: `check_changes()` and
`get_untracked_files()`

Note that `check_changes()` also calls `get_untracked_files()`.
So, `check_changes()` is called 2 times and `get_untracked_files()`
3 times. By checking at the beginning of the function if we already
performed a check, we can avoid making useless calls.
---
 builtin/stash.c | 50 +++--
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 0ef88408a..4d5c0d16e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -819,13 +819,23 @@ static int store_stash(int argc, const char **argv, const 
char *prefix)
 }
 
 /*
- * `out` will be filled with the names of untracked files. The return value is:
+ * `has_untracked_files` is:
+ * -2 if `get_untracked_files()` hasn't been called
+ * -1 if there were errors
+ *  0 if there are no untracked files
+ *  1 if there are untracked files
+ *
+ * `untracked_files` will be filled with the names of untracked files.
+ * The return value is:
  *
  * = 0 if there are not any untracked files
  * > 0 if there are untracked files
  */
+static struct strbuf untracked_files = STRBUF_INIT;
+static int has_untracked_files = -2;
+
 static int get_untracked_files(const char **argv, const char *prefix,
-  int include_untracked, struct strbuf *out)
+  int include_untracked)
 {
int max_len;
int i;
@@ -833,6 +843,9 @@ static int get_untracked_files(const char **argv, const 
char *prefix,
struct dir_struct dir;
struct pathspec pathspec;
 
+   if (has_untracked_files != -2)
+   return has_untracked_files;
+
memset(, 0, sizeof(dir));
if (include_untracked != 2)
setup_standard_excludes();
@@ -849,7 +862,7 @@ static int get_untracked_files(const char **argv, const 
char *prefix,
free(ent);
continue;
}
-   strbuf_addf(out, "%s\n", ent->name);
+   strbuf_addf(_files, "%s\n", ent->name);
free(ent);
}
 
@@ -857,16 +870,25 @@ static int get_untracked_files(const char **argv, const 
char *prefix,
free(dir.ignored);
clear_directory();
free(seen);
-   return out->len;
+   has_untracked_files = untracked_files.len;
+   return untracked_files.len;
 }
 
 /*
+ * `changes` is:
+ * -2 if `check_changes()` hasn't been called
+ * -1 if there were any errors
+ *  0 if there are no changes
+ *  1 if there are changes
+ *
  * The return value of `check_changes()` can be:
  *
  * < 0 if there was an error
  * = 0 if there are no changes.
  * > 0 if there are changes.
  */
+static int changes = -2;
+
 static int check_changes(const char **argv, int include_untracked,
 const char *prefix)
 {
@@ -874,9 +896,11 @@ static int check_changes(const char **argv, int 
include_untracked,
int ret = 0;
struct rev_info rev;
struct object_id dummy;
-   struct strbuf out = STRBUF_INIT;
struct argv_array args = ARGV_ARRAY_INIT;
 
+   if (changes != -2)
+   return changes;
+
init_revisions(, prefix);
parse_pathspec(_data, 0, PATHSPEC_PREFER_FULL,
   prefix, argv);
@@ -912,17 +936,16 @@ static int check_changes(const char **argv, int 
include_untracked,
}
 
if (include_untracked && get_untracked_files(argv, prefix,
-include_untracked, ))
+include_untracked))
ret = 1;
 
 done:
-   strbuf_release();
+   changes = ret;
argv_array_clear();
return ret;
 }
 
-static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
-   struct strbuf *out)
+static int save_untracked_files(struct stash_info *info, struct strbuf *msg)
 {
int ret = 0;
struct strbuf untracked_msg = STRBUF_INIT;
@@ -937,7 +960,8 @@ static int save_untracked_files(struct stash_info *info, 
struct strbuf *msg,
 stash_index_path.buf);
 
strbuf_addf(_msg, "untracked files on %s\n", msg->buf);
-   if (pipe_command(, out->buf, out->len, NULL, 0, NULL, 0)) {
+   if (pipe_command(, untracked_files.buf, untracked_files.len,
+NULL, 0, NULL, 0)) {
ret = -1;
goto done;
}
@@ -1116,7 +1140,6 @@ static int do_create_stash(int argc, const char **argv, 
const char *prefix,
struct commit_list *parents = NULL;
struct strbuf msg = STRBUF_INIT;
struct strbuf commit_tree_label = STRBUF_INIT;
-   

[GSoC][PATCH v7 19/26] stash: make push to be quiet

[Paul-Sebastian Ungureanu]

There is a change in behaviour with this commit. When there was
no initial commit, the shell version of stash would still display
a message. This commit makes `push` to not display any message if
`--quiet` or `-q` is specified.
---
 builtin/stash--helper.c | 41 +++--
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index c26cad3d5..4fd79532c 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -1079,7 +1079,7 @@ static int stash_working_tree(struct stash_info *info,
 
 static int do_create_stash(int argc, const char **argv, const char *prefix,
   const char **stash_msg, int include_untracked,
-  int patch_mode, struct stash_info *info)
+  int patch_mode, struct stash_info *info, int quiet)
 {
int untracked_commit_option = 0;
int ret = 0;
@@ -1105,7 +1105,8 @@ static int do_create_stash(int argc, const char **argv, 
const char *prefix,
}
 
if (get_oid("HEAD", >b_commit)) {
-   fprintf_ln(stderr, "You do not have the initial commit yet");
+   if (!quiet)
+   fprintf_ln(stderr, "You do not have the initial commit 
yet");
ret = -1;
goto done;
} else {
@@ -1127,7 +1128,8 @@ static int do_create_stash(int argc, const char **argv, 
const char *prefix,
if (write_cache_as_tree(>i_tree, 0, NULL) ||
commit_tree(commit_tree_label.buf, commit_tree_label.len,
>i_tree, parents, >i_commit, NULL, NULL)) {
-   fprintf_ln(stderr, "Cannot save the current index state");
+   if (!quiet)
+   fprintf_ln(stderr, "Cannot save the current index 
state");
ret = -1;
goto done;
}
@@ -1135,7 +1137,8 @@ static int do_create_stash(int argc, const char **argv, 
const char *prefix,
if (include_untracked && get_untracked_files(argv, 1,
 include_untracked, )) {
if (save_untracked_files(info, , )) {
-   printf_ln("Cannot save the untracked files");
+   if (!quiet)
+   printf_ln("Cannot save the untracked files");
ret = -1;
goto done;
}
@@ -1144,14 +1147,16 @@ static int do_create_stash(int argc, const char **argv, 
const char *prefix,
if (patch_mode) {
ret = stash_patch(info, argv);
if (ret < 0) {
-   printf_ln("Cannot save the current worktree state");
+   if (!quiet)
+   printf_ln("Cannot save the current worktree 
state");
goto done;
} else if (ret > 0) {
goto done;
}
} else {
if (stash_working_tree(info, argv, prefix)) {
-   printf_ln("Cannot save the current worktree state");
+   if (!quiet)
+   printf_ln("Cannot save the current worktree 
state");
ret = -1;
goto done;
}
@@ -1176,7 +1181,8 @@ static int do_create_stash(int argc, const char **argv, 
const char *prefix,
 
if (commit_tree(*stash_msg, strlen(*stash_msg), >w_tree,
parents, >w_commit, NULL, NULL)) {
-   printf_ln("Cannot record working tree state");
+   if (!quiet)
+   printf_ln("Cannot record working tree state");
ret = -1;
goto done;
}
@@ -1208,7 +1214,7 @@ static int create_stash(int argc, const char **argv, 
const char *prefix)
 0);
 
ret = do_create_stash(argc, argv, prefix, _msg,
- include_untracked, 0, );
+ include_untracked, 0, , 0);
 
if (!ret)
printf_ln("%s", oid_to_hex(_commit));
@@ -1261,25 +1267,31 @@ static int do_push_stash(int argc, const char **argv, 
const char *prefix,
return -1;
 
if (!check_changes(argv, include_untracked, prefix)) {
-   fprintf_ln(stdout, "No local changes to save");
+   if (!quiet)
+   fprintf_ln(stdout, "No local changes to save");
return 0;
}
 
if (!reflog_exists(ref_stash) && do_clear_stash()) {
-   fprintf_ln(stderr, "Cannot initialize stash");
+   if (!quiet)
+   fprintf_ln(stderr, "Cannot initialize stash");
return -1;
}
 
if ((ret = do_create_stash(argc, argv, prefix, _msg,
-  include_untracked, patch_mode, )))
+   

[GSoC][PATCH v7 20/26] stash: add tests for `git stash push -q`

[Paul-Sebastian Ungureanu]

This commit introduces more tests for the quiet option of
`git stash push`.
---
 t/t3903-stash.sh | 21 +
 1 file changed, 21 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 8d002a7f2..b78db74ae 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1064,6 +1064,27 @@ test_expect_success 'push:  not in the 
repository errors out' '
test_path_is_file untracked
 '
 
+test_expect_success 'push: -q is quiet with changes' '
+   >foo &&
+   git stash push -q >output 2>&1 &&
+   test_must_be_empty output
+'
+
+test_expect_success 'push: -q is quiet with no changes' '
+   git stash push -q >output 2>&1 &&
+   test_must_be_empty output
+'
+
+test_expect_success 'push: -q is quiet even if there is no initial commit' '
+   git init foo_dir &&
+   cd foo_dir &&
+   touch bar &&
+   test_must_fail git stash push -q >output 2>&1 &&
+   test_must_be_empty output &&
+   cd .. &&
+   rm -rf foo_dir
+'
+
 test_expect_success 'untracked files are left in place when -u is not given' '
>file &&
git add file &&
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 22/26] stash: convert save to builtin

[Paul-Sebastian Ungureanu]

Add stash save to the helper and delete functions which are no
longer needed (`show_help()`, `save_stash()`, `push_stash()`,
`create_stash()`, `clear_stash()`, `untracked_files()` and
`no_changes()`).
---
 builtin/stash--helper.c |  48 +++
 git-stash.sh| 311 +---
 2 files changed, 50 insertions(+), 309 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 5c27f5dcf..f54a476e3 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -26,6 +26,8 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
   "  [-u|--include-untracked] [-a|--all] [-m|--message 
]\n"
   "  [--] [...]]"),
+   N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+  "  [-u|--include-untracked] [-a|--all] []"),
NULL
 };
 
@@ -81,6 +83,12 @@ static const char * const git_stash_helper_push_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_save_usage[] = {
+   N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+  "  [-u|--include-untracked] [-a|--all] []"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -1445,6 +1453,44 @@ static int push_stash(int argc, const char **argv, const 
char *prefix)
 include_untracked, quiet, stash_msg);
 }
 
+static int save_stash(int argc, const char **argv, const char *prefix)
+{
+   int i;
+   int keep_index = -1;
+   int patch_mode = 0;
+   int include_untracked = 0;
+   int quiet = 0;
+   char *stash_msg = NULL;
+   struct strbuf alt_stash_msg = STRBUF_INIT;
+   struct option options[] = {
+   OPT_SET_INT('k', "keep-index", _index,
+   N_("keep index"), 1),
+   OPT_BOOL('p', "patch", _mode,
+   N_("stash in patch mode")),
+   OPT_BOOL('q', "quiet", ,
+   N_("quiet mode")),
+   OPT_BOOL('u', "include-untracked", _untracked,
+N_("include untracked files in stash")),
+   OPT_SET_INT('a', "all", _untracked,
+   N_("include ignore files"), 2),
+   OPT_STRING('m', "message", _msg, N_("message"),
+N_("stash message")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_save_usage,
+0);
+
+   for (i = 0; i < argc; ++i)
+   strbuf_addf(_stash_msg, "%s ", argv[i]);
+
+   stash_msg = strbuf_detach(_stash_msg, NULL);
+
+   return do_push_stash(0, NULL, prefix, keep_index, patch_mode,
+include_untracked, quiet, stash_msg);
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -1485,6 +1531,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!create_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "push"))
return !!push_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "save"))
+   return !!save_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index c3146f62a..695f1feba 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -36,314 +36,6 @@ else
reset_color=
 fi
 
-no_changes () {
-   git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
-   git diff-files --quiet --ignore-submodules -- "$@" &&
-   (test -z "$untracked" || test -z "$(untracked_files "$@")")
-}
-
-untracked_files () {
-   if test "$1" = "-z"
-   then
-   shift
-   z=-z
-   else
-   z=
-   fi
-   excl_opt=--exclude-standard
-   test "$untracked" = "all" && excl_opt=
-   git ls-files -o $z $excl_opt -- "$@"
-}
-
-clear_stash () {
-   if test $# != 0
-   then
-   die "$(gettext "git stash clear with parameters is 
unimplemented")"
-   fi
-   if current=$(git rev-parse --verify --quiet $ref_stash)
-   then
-   git update-ref -d $ref_stash $current
-   fi
-}
-
-create_stash () {
-   stash_msg=
-   untracked=
-   while test $# != 0
-   do
-   case "$1" in
-   -m|--message)
-   shift
-   stash_msg=${1?"BUG: create_stash () -m requires an 
argument"}
-   ;;
-   -m*)
-   stash_msg=${1#-m}
-   ;;
-   

[GSoC][PATCH v7 14/26] stash: convert store to builtin

[Paul-Sebastian Ungureanu]

Add stash store to the helper and delete the store_stash function
from the shell script.

Add the usage string which was forgotten in the shell script.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 52 +
 git-stash.sh| 43 ++
 2 files changed, 54 insertions(+), 41 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index ec8c38c6f..5ff810f8c 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -20,6 +20,7 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
+   N_("git stash--helper store [-m|--message ] [-q|--quiet] 
"),
NULL
 };
 
@@ -58,6 +59,11 @@ static const char * const git_stash_helper_clear_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_store_usage[] = {
+   N_("git stash--helper store [-m|--message ] [-q|--quiet] 
"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -731,6 +737,50 @@ static int show_stash(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+static int do_store_stash(const char *w_commit, const char *stash_msg,
+ int quiet)
+{
+   int ret = 0;
+   struct object_id obj;
+
+   if (!stash_msg)
+   stash_msg  = xstrdup("Created via \"git stash--helper 
store\".");
+
+   ret = get_oid(w_commit, );
+   if (!ret) {
+   ret = update_ref(stash_msg, ref_stash, , NULL,
+REF_FORCE_CREATE_REFLOG,
+quiet ? UPDATE_REFS_QUIET_ON_ERR :
+UPDATE_REFS_MSG_ON_ERR);
+   }
+   if (ret && !quiet)
+   fprintf_ln(stderr, _("Cannot update %s with %s"),
+  ref_stash, w_commit);
+
+   return ret;
+}
+
+static int store_stash(int argc, const char **argv, const char *prefix)
+{
+   const char *stash_msg = NULL;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_STRING('m', "message", _msg, "message", N_("stash 
message")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_store_usage,
+PARSE_OPT_KEEP_UNKNOWN);
+
+   if (argc != 1) {
+   fprintf(stderr, _("\"git stash--helper store\" requires one 
 argument\n"));
+   return -1;
+   }
+
+   return do_store_stash(argv[0], stash_msg, quiet);
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -765,6 +815,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!list_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "show"))
return !!show_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "store"))
+   return !!store_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0d05cbc1e..5739c5152 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -191,45 +191,6 @@ create_stash () {
die "$(gettext "Cannot record working tree state")"
 }
 
-store_stash () {
-   while test $# != 0
-   do
-   case "$1" in
-   -m|--message)
-   shift
-   stash_msg="$1"
-   ;;
-   -m*)
-   stash_msg=${1#-m}
-   ;;
-   --message=*)
-   stash_msg=${1#--message=}
-   ;;
-   -q|--quiet)
-   quiet=t
-   ;;
-   *)
-   break
-   ;;
-   esac
-   shift
-   done
-   test $# = 1 ||
-   die "$(eval_gettext "\"$dashless store\" requires one  
argument")"
-
-   w_commit="$1"
-   if test -z "$stash_msg"
-   then
-   stash_msg="Created via \"git stash store\"."
-   fi
-
-   git update-ref --create-reflog -m "$stash_msg" $ref_stash $w_commit
-   ret=$?
-   test $ret != 0 && test -z "$quiet" &&
-   die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
-   return $ret
-}
-
 push_stash () {
keep_index=
patch_mode=
@@ -308,7 +269,7 @@ push_stash () {
clear_stash || die "$(gettext "Cannot initialize stash")"
 
create_stash -m "$stash_msg" -u "$untracked" -- "$@"
-   store_stash -m 

[GSoC][PATCH v7 25/26] stash: replace all `write-tree` child processes with API calls

[Paul-Sebastian Ungureanu]

This commit replaces spawning `git write-tree` with API calls.
---
 builtin/stash.c | 40 
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 4d5c0d16e..46e76a34e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -949,9 +949,8 @@ static int save_untracked_files(struct stash_info *info, 
struct strbuf *msg)
 {
int ret = 0;
struct strbuf untracked_msg = STRBUF_INIT;
-   struct strbuf out2 = STRBUF_INIT;
struct child_process cp = CHILD_PROCESS_INIT;
-   struct child_process cp2 = CHILD_PROCESS_INIT;
+   struct index_state state = { NULL };
 
cp.git_cmd = 1;
argv_array_pushl(, "update-index", "--add",
@@ -966,15 +965,11 @@ static int save_untracked_files(struct stash_info *info, 
struct strbuf *msg)
goto done;
}
 
-   cp2.git_cmd = 1;
-   argv_array_push(, "write-tree");
-   argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
-   if (pipe_command(, NULL, 0, , 0,NULL, 0)) {
+   if (write_index_as_tree(>u_tree, , stash_index_path.buf, 0,
+   NULL)) {
ret = -1;
goto done;
}
-   get_oid_hex(out2.buf, >u_tree);
 
if (commit_tree(untracked_msg.buf, untracked_msg.len,
>u_tree, NULL, >u_commit, NULL, NULL)) {
@@ -984,7 +979,6 @@ static int save_untracked_files(struct stash_info *info, 
struct strbuf *msg)
 
 done:
strbuf_release(_msg);
-   strbuf_release();
remove_path(stash_index_path.buf);
return ret;
 }
@@ -994,11 +988,10 @@ static struct strbuf patch = STRBUF_INIT;
 static int stash_patch(struct stash_info *info, const char **argv)
 {
int ret = 0;
-   struct strbuf out2 = STRBUF_INIT;
struct child_process cp0 = CHILD_PROCESS_INIT;
struct child_process cp1 = CHILD_PROCESS_INIT;
-   struct child_process cp2 = CHILD_PROCESS_INIT;
struct child_process cp3 = CHILD_PROCESS_INIT;
+   struct index_state state = { NULL };
 
remove_path(stash_index_path.buf);
 
@@ -1023,17 +1016,12 @@ static int stash_patch(struct stash_info *info, const 
char **argv)
goto done;
}
 
-   cp2.git_cmd = 1;
-   argv_array_push(, "write-tree");
-   argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
-   if (pipe_command(, NULL, 0, , 0,NULL, 0)) {
+   if (write_index_as_tree(>w_tree, , stash_index_path.buf, 0,
+   NULL)) {
ret = -1;
goto done;
}
 
-   get_oid_hex(out2.buf, >w_tree);
-
cp3.git_cmd = 1;
argv_array_pushl(, "diff-tree", "-p", "HEAD",
 oid_to_hex(>w_tree), "--", NULL);
@@ -1046,7 +1034,6 @@ static int stash_patch(struct stash_info *info, const 
char **argv)
}
 
 done:
-   strbuf_release();
remove_path(stash_index_path.buf);
return ret;
 }
@@ -1056,11 +1043,10 @@ static int stash_working_tree(struct stash_info *info,
 {
int ret = 0;
struct child_process cp2 = CHILD_PROCESS_INIT;
-   struct child_process cp3 = CHILD_PROCESS_INIT;
-   struct strbuf out3 = STRBUF_INIT;
struct argv_array args = ARGV_ARRAY_INIT;
struct strbuf diff_output = STRBUF_INIT;
struct rev_info rev;
+   struct index_state state = { NULL };
 
set_alternate_index_output(stash_index_path.buf);
if (reset_tree(>i_tree, 0, 0)) {
@@ -1103,20 +1089,18 @@ static int stash_working_tree(struct stash_info *info,
goto done;
}
 
-   cp3.git_cmd = 1;
-   argv_array_push(, "write-tree");
-   argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
-   if (pipe_command(, NULL, 0, , 0,NULL, 0)) {
+   if (write_index_as_tree(>w_tree, , stash_index_path.buf, 0,
+   NULL)) {
+
ret = -1;
goto done;
}
 
-   get_oid_hex(out3.buf, >w_tree);
+   discard_cache();
+   read_cache();
 
 done:
UNLEAK(rev);
-   strbuf_release();
argv_array_clear();
object_array_clear();
strbuf_release(_output);
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 16/26] stash: replace spawning a "read-tree" process

[Paul-Sebastian Ungureanu]

Instead of spawning a child process, make use of `reset_tree()`
function already implemented in `stash-helper.c`.
---
 builtin/stash--helper.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index a4e57899b..887b78d05 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -984,21 +984,18 @@ static int stash_patch(struct stash_info *info, const 
char **argv)
 static int stash_working_tree(struct stash_info *info, const char **argv)
 {
int ret = 0;
-   struct child_process cp0 = CHILD_PROCESS_INIT;
struct child_process cp1 = CHILD_PROCESS_INIT;
struct child_process cp2 = CHILD_PROCESS_INIT;
struct child_process cp3 = CHILD_PROCESS_INIT;
struct strbuf out1 = STRBUF_INIT;
struct strbuf out3 = STRBUF_INIT;
 
-   cp0.git_cmd = 1;
-   argv_array_push(, "read-tree");
-   argv_array_pushf(, "--index-output=%s", stash_index_path.buf);
-   argv_array_pushl(, "-m", oid_to_hex(>i_tree), NULL);
-   if (run_command()) {
+   set_alternate_index_output(stash_index_path.buf);
+   if (reset_tree(>i_tree, 0, 0)) {
ret = -1;
goto done;
}
+   set_alternate_index_output(".git/index");
 
cp1.git_cmd = 1;
argv_array_pushl(, "diff-index", "--name-only", "-z",
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 21/26] stash: replace spawning `git ls-files` child process

[Paul-Sebastian Ungureanu]

This commit replaces spawning `git ls-files` child process with
API calls to get the untracked files.
---
 builtin/stash--helper.c | 49 +++--
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 4fd79532c..5c27f5dcf 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -813,27 +813,42 @@ static int store_stash(int argc, const char **argv, const 
char *prefix)
 /*
  * `out` will be filled with the names of untracked files. The return value is:
  *
- * < 0 if there was a bug (any arg given outside the repo will be detected
- * by `setup_revision()`)
  * = 0 if there are not any untracked files
  * > 0 if there are untracked files
  */
-static int get_untracked_files(const char **argv, int line_term,
+static int get_untracked_files(const char **argv, const char *prefix,
   int include_untracked, struct strbuf *out)
 {
-   struct child_process cp = CHILD_PROCESS_INIT;
-   cp.git_cmd = 1;
-   argv_array_pushl(, "ls-files", "-o", NULL);
-   if (line_term)
-   argv_array_push(, "-z");
+   int max_len;
+   int i;
+   char *seen;
+   struct dir_struct dir;
+   struct pathspec pathspec;
+
+   memset(, 0, sizeof(dir));
if (include_untracked != 2)
-   argv_array_push(, "--exclude-standard");
-   argv_array_push(, "--");
-   if (argv)
-   argv_array_pushv(, argv);
+   setup_standard_excludes();
 
-   if (pipe_command(, NULL, 0, out, 0, NULL, 0))
-   return -1;
+   parse_pathspec(, 0,
+  PATHSPEC_PREFER_FULL,
+  prefix, argv);
+   seen = xcalloc(pathspec.nr, 1);
+
+   max_len = fill_directory(, the_repository->index, );
+   for (i = 0; i < dir.nr; i++) {
+   struct dir_entry *ent = dir.entries[i];
+   if (!dir_path_match(ent, , max_len, seen)) {
+   free(ent);
+   continue;
+   }
+   strbuf_addf(out, "%s\n", ent->name);
+   free(ent);
+   }
+
+   free(dir.entries);
+   free(dir.ignored);
+   clear_directory();
+   free(seen);
return out->len;
 }
 
@@ -888,7 +903,7 @@ static int check_changes(const char **argv, int 
include_untracked,
goto done;
}
 
-   if (include_untracked && get_untracked_files(argv, 0,
+   if (include_untracked && get_untracked_files(argv, prefix,
 include_untracked, ))
ret = 1;
 
@@ -908,7 +923,7 @@ static int save_untracked_files(struct stash_info *info, 
struct strbuf *msg,
struct child_process cp2 = CHILD_PROCESS_INIT;
 
cp.git_cmd = 1;
-   argv_array_pushl(, "update-index", "-z", "--add",
+   argv_array_pushl(, "update-index", "--add",
 "--remove", "--stdin", NULL);
argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
 stash_index_path.buf);
@@ -1134,7 +1149,7 @@ static int do_create_stash(int argc, const char **argv, 
const char *prefix,
goto done;
}
 
-   if (include_untracked && get_untracked_files(argv, 1,
+   if (include_untracked && get_untracked_files(argv, prefix,
 include_untracked, )) {
if (save_untracked_files(info, , )) {
if (!quiet)
-- 
2.18.0.573.g56500d98f

[GSoC][PATCH v7 02/26] stash: improve option parsing test coverage

[Paul-Sebastian Ungureanu]

From: Joel Teichroeb 

In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many parameters, or too few.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 t/t3903-stash.sh | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 1f871d3cc..af7586d43 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,36 @@ test_expect_failure 'stash file to directory' '
test foo = "$(cat file/file)"
 '
 
+test_expect_success 'giving too many ref arguments does not modify files' '
+   git stash clear &&
+   test_when_finished "git reset --hard HEAD" &&
+   echo foo >file2 &&
+   git stash &&
+   echo bar >file2 &&
+   git stash &&
+   test-tool chmtime =123456789 file2 &&
+   for type in apply pop "branch stash-branch"
+   do
+   test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+   test_i18ngrep "Too many revisions" err &&
+   test 123456789 = $(test-tool chmtime -g file2) || return 1
+   done
+'
+
+test_expect_success 'drop: too many arguments errors out (does nothing)' '
+   git stash list >expect &&
+   test_must_fail git stash drop stash@{0} stash@{1} 2>err &&
+   test_i18ngrep "Too many revisions" err &&
+   git stash list >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'show: too many arguments errors out (does nothing)' '
+   test_must_fail git stash show stash@{0} stash@{1} 2>err 1>out &&
+   test_i18ngrep "Too many revisions" err &&
+   test_must_be_empty out
+'
+
 test_expect_success 'stash create - no changes' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
@@ -479,6 +509,11 @@ test_expect_success 'stash branch - stashes on stack, 
stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
 '
 
+test_expect_success 'stash branch complains with no arguments' '
+   test_must_fail git stash branch 2>err &&
+   test_i18ngrep "No branch name specified" err
+'
+
 test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
-- 
2.18.0.573.g56500d98f

Re: [PATCH 11/11] builtin rebase: support `git rebase `

[Johannes Schindelin]

Hi Duy,

On Wed, 8 Aug 2018, Duy Nguyen wrote:

> On Wed, Aug 8, 2018 at 3:55 PM Pratik Karki  wrote:
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 63634210c0..b2ddfa8dbf 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -585,7 +602,8 @@ int cmd_rebase(int argc, const char **argv, const char 
> > *prefix)
> > }
> > if (get_oid("HEAD", _head))
> > die(_("Could not resolve HEAD to a revision"));
> > -   }
> > +   } else
> > +   BUG("unexpected number of arguments left to parse");
> 
> Does this mean "git base one two three" triggers this BUG? If so, this
> should be a die() instead. I did not real the full source code, so
> maybe this case is already caught higher up.

As you can see from

https://github.com/git/git/blob/v2.18.0/git-rebase.sh#L615

the original, Unix shell script version of `git rebase` also says "BUG"
here. And if you care to look at

https://github.com/git/git/blob/3358abdcb/builtin/rebase.c#L870-L872

you will see that there is a proper check for the correct amount of
command-line parameters.

So at this point, it would indeed indicate a bug if the `argc` had an
unexpected value.

Ciao,
Dscho

Re: [PATCH 08/11] builtin rebase: support --force-rebase

[Stefan Beller]

On Wed, Aug 8, 2018 at 6:51 AM Pratik Karki  wrote:

> @@ -551,10 +560,21 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
[...]
> ; /* be quiet */
> else if (!strcmp(branch_name, "HEAD") &&
> -   resolve_ref_unsafe("HEAD", 0, NULL, ))
> +resolve_ref_unsafe("HEAD", 0, NULL, ))

This line is changing only the indentation whitespace?
Would it make sense to have it in the previous patch?

Re: [PATCH v2 4/4] unpack-trees: cheaper index update when walking by cache-tree

[Elijah Newren]

On Sun, Jul 29, 2018 at 3:36 AM Nguyễn Thái Ngọc Duy  wrote:
>
> With the new cache-tree, we could mostly avoid I/O (due to odb access)
> the code mostly becomes a loop of "check this, check that, add the
> entry to the index". We could skip a couple checks in this giant loop
> to go faster:
>
> - We know here that we're copying entries from the source index to the
>   result one. All paths in the source index must have been validated
>   at load time already (and we're not taking strange paths from tree
>   objects) which means we can skip verify_path() without compromise.
>
> - We also know that D/F conflicts can't happen for all these entries
>   (since cache-tree and all the trees are the same) so we can skip
>   that as well.
>
> This gives rather nice speedups for "unpack trees" rows where "unpack
> trees" time is now cut in half compared to when
> traverse_by_cache_tree() is added, or 1/7 of the original "unpack
> trees" time.
>
>baseline  cache-treethis patch
>  
>0.018239226   0.019365414   0.020519621 s: read cache .git/index
>0.052541655   0.049605548   0.048814384 s: preload index
>0.001537598   0.001571695   0.001575382 s: refresh index
>0.168167768   0.049677212   0.024719308 s: unpack trees
>0.002897186   0.002845256   0.00280 s: update worktree after a merge
>0.131661745   0.136597522   0.134891617 s: repair cache-tree
>0.075389117   0.075422517   0.074832291 s: write index, changed mask = 2a
>0.111702023   0.032813253   0.008616479 s: unpack trees
>0.23245   0.22002   0.26630 s: update worktree after a merge
>0.111793866   0.032933140   0.008714071 s: diff-index
>0.587933288   0.398924370   0.380452871 s: git command: 
> /home/pclouds/w/git/git
>
> Total saving of this new patch looks even less impressive, now that
> time spent in unpacking trees is so small. Which is why the next
> attempt should be on that "repair cache-tree" line.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  cache.h|  1 +
>  read-cache.c   |  3 ++-
>  unpack-trees.c | 27 +++
>  unpack-trees.h |  1 +
>  4 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/cache.h b/cache.h
> index 8b447652a7..e6f7ee4b64 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -673,6 +673,7 @@ extern int index_name_pos(const struct index_state *, 
> const char *name, int name
>  #define ADD_CACHE_JUST_APPEND 8/* Append only; 
> tree.c::read_tree() */
>  #define ADD_CACHE_NEW_ONLY 16  /* Do not replace existing ones */
>  #define ADD_CACHE_KEEP_CACHE_TREE 32   /* Do not invalidate cache-tree */
> +#define ADD_CACHE_SKIP_VERIFY_PATH 64  /* Do not verify path */
>  extern int add_index_entry(struct index_state *, struct cache_entry *ce, int 
> option);
>  extern void rename_index_entry_at(struct index_state *, int pos, const char 
> *new_name);
>
> diff --git a/read-cache.c b/read-cache.c
> index e865254bea..b0b5df5de7 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1170,6 +1170,7 @@ static int add_index_entry_with_check(struct 
> index_state *istate, struct cache_e
> int ok_to_add = option & ADD_CACHE_OK_TO_ADD;
> int ok_to_replace = option & ADD_CACHE_OK_TO_REPLACE;
> int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
> +   int skip_verify_path = option & ADD_CACHE_SKIP_VERIFY_PATH;
> int new_only = option & ADD_CACHE_NEW_ONLY;
>
> if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
> @@ -1210,7 +1211,7 @@ static int add_index_entry_with_check(struct 
> index_state *istate, struct cache_e
>
> if (!ok_to_add)
> return -1;
> -   if (!verify_path(ce->name, ce->ce_mode))
> +   if (!skip_verify_path && !verify_path(ce->name, ce->ce_mode))
> return error("Invalid path '%s'", ce->name);
>
> if (!skip_df_check &&
> diff --git a/unpack-trees.c b/unpack-trees.c
> index c33ebaf001..dc62afd968 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -201,6 +201,7 @@ static int do_add_entry(struct unpack_trees_options *o, 
> struct cache_entry *ce,
>
> ce->ce_flags = (ce->ce_flags & ~clear) | set;
> return add_index_entry(>result, ce,
> +  o->extra_add_index_flags |
>ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
>  }
>
> @@ -701,6 +702,24 @@ static int traverse_by_cache_tree(int pos, int 
> nr_entries, int nr_names,
> if (!o->merge)
> BUG("We need cache-tree to do this optimization");
>
> +   /*
> +* Try to keep add_index_entry() as fast as possible since
> +* we're going to do a lot of them.
> +*
> +* Skipping verify_path() should totally be safe because these
> +* paths are from the source index, which must have been
> +* verified.
> +*
> +* Skipping D/F and cache-tree validation checks 

Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set

[Jonathan Tan]

> But what to delta against what else is determined by the pathname
> info, which is now lost by enumerating objects without tree/history
> walking.  By giving phoney pathnames to objects while enumerating
> them in offset order and giving similar pathnames to objects closer
> to each other, I was hoping that better bases will likely to be in
> the same window.  The order in which objects are prepared and fed to
> try_delta() is "group by type, and then sort by path-hash (major
> key) and size (descending order, used as minor key)", so that
> the largest among similarly named blobs is stored as base and other
> blobs with similar name try to become delta against that one.

Thanks for the patient explanation - I think I see it now. If we
enumerate in pack order and pass " " instead of
"" to pack-objects in such a way that we make pack-objects sort
by {type -> pack-order -> size} instead of {type -> size}, we can
hopefully get better deltas. I'll write a NEEDSWORK similar to this.

I'll send a reroll later today.

Re: Page content is wider than view window

[Brady Trainor]

Eric Sunshine  writes:

> The git-scm.com website is maintained as a distinct project[1] at
> Github; it is not directly related to the Git project itself (to which
> you sent this email). A good way to voice a concern or make a
> suggestion about the website is either to open an issue[2] or submit a
> pull request[3] if you have a specific change in mind.
>
> The Pro Git book (if that's what you're reading) is also a distinct
> project[4], not directly related to the Git project. You can likewise
> open an issue[5] or submit a pull request[6] if needed.
>
> [...]

Ah, that was completely discoverable through the website. My mistake,
thank you for redirect.

-- 
Brady

Re: [PATCH v3 0/4] Speed up unpack_trees()

[Junio C Hamano]

Junio C Hamano  writes:

> Junio C Hamano  writes:
>
>> not used behind is *not* OK.  And lack of restoring the bottom in
>> the new codepath makes me suspect exactly such a bug _after_ the
>> traversal exits the subtree we are using this new optimization in
>> and moves on.
>
> Hmph, thinking about this further, I cannot convince myself that
> lack of bottom adjustment can lead to a triggerable bug.  The only
> case that a subtree traversal need to skip some unpacked entries in
> the index and then revisit them by rewinding, e.g. entries "t-i" and
> "t-j" that are left unprocessed while entries "t/1", "t/2", etc. are
> processed, in the illustration of da165f47 ("unpack-trees.c: prepare
> for looking ahead in the index", 2010-01-07), is when one of the
> trees have a non-tree with the same name as the subtree we are
> trying to descend into, and as long as we know all trees have the
> thing as a tree, I do not think of a case where such ordering
> inversion would get in the way.

One more, and hopefully the final, note.

A paranoid may be soothed by a simple "cache_bottom must match pos
at this point" at the beginning of the optimized traversal.  Just
like you already have an assert to ensure that pos points at the
first entry in the directory in index_pos_by_traverse_info(), there
should not be any unused entry in the index before that entry and
the bottom pointer must be pointing at it.  It is a cheap check, and
if violated, would indicate that the above "I do not think of a
case ..." was incomplete.

> That was the only thing I found questionable in 2/4, which is the
> most important piece in the series, so we probably are OK.
>
> Thanks for working on this one.

Re: [PATCH 04/11] builtin rebase: support --quiet

[Stefan Beller]

On Wed, Aug 8, 2018 at 6:51 AM Pratik Karki  wrote:
>
> This commit introduces a rebase option `--quiet`. While `--quiet` is
> commonly perceived as opposite to `--verbose`, this is not the case for
> the rebase command: both `--quiet` and `--verbose` default to `false` if
> neither `--quiet` nor `--verbose` is present.
>
> This commit goes further and introduces `--no-quiet` which is the
> contrary of `--quiet` and it's introduction doesn't modify any
> behaviour.
>
> Note: The `flags` field in `rebase_options` will accumulate more bits in
> subsequent commits, in particular a verbose and a diffstat flag. And as
> --quoet inthe shell scripted version of the rebase command switches off

  --quote in the

(in case a resend is needed)

Re: [PATCH v3 3/4] unpack-trees: reduce malloc in cache-tree walk

[Elijah Newren]

On Fri, Aug 3, 2018 at 10:39 PM Nguyễn Thái Ngọc Duy  wrote:
>
> This is a micro optimization that probably only shines on repos with
> deep directory structure. Instead of allocating and freeing a new
> cache_entry in every iteration, we reuse the last one and only update
> the parts that are new each iteration.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> Signed-off-by: Junio C Hamano 
> ---
>  unpack-trees.c | 29 -
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index ba3d2e947e..c8defc2015 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -694,6 +694,8 @@ static int traverse_by_cache_tree(int pos, int 
> nr_entries, int nr_names,
>  {
> struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
> struct unpack_trees_options *o = info->data;
> +   struct cache_entry *tree_ce = NULL;
> +   int ce_len = 0;
> int i, d;
>
> if (!o->merge)
> @@ -708,30 +710,39 @@ static int traverse_by_cache_tree(int pos, int 
> nr_entries, int nr_names,
>  * in the first place.
>  */
> for (i = 0; i < nr_entries; i++) {
> -   struct cache_entry *tree_ce;
> -   int len, rc;
> +   int new_ce_len, len, rc;
>
> src[0] = o->src_index->cache[pos + i];
>
> len = ce_namelen(src[0]);
> -   tree_ce = xcalloc(1, cache_entry_size(len));
> +   new_ce_len = cache_entry_size(len);
> +
> +   if (new_ce_len > ce_len) {
> +   new_ce_len <<= 1;
> +   tree_ce = xrealloc(tree_ce, new_ce_len);
> +   memset(tree_ce, 0, new_ce_len);
> +   ce_len = new_ce_len;
> +
> +   tree_ce->ce_flags = create_ce_flags(0);
> +
> +   for (d = 1; d <= nr_names; d++)
> +   src[d] = tree_ce;
> +   }
>
> tree_ce->ce_mode = src[0]->ce_mode;
> -   tree_ce->ce_flags = create_ce_flags(0);
> tree_ce->ce_namelen = len;
> oidcpy(_ce->oid, [0]->oid);
> memcpy(tree_ce->name, src[0]->name, len + 1);
>
> -   for (d = 1; d <= nr_names; d++)
> -   src[d] = tree_ce;
> -
> rc = call_unpack_fn((const struct cache_entry * const *)src, 
> o);
> -   free(tree_ce);
> -   if (rc < 0)
> +   if (rc < 0) {
> +   free(tree_ce);
> return rc;
> +   }
>
> mark_ce_used(src[0], o);
> }
> +   free(tree_ce);
> if (o->debug_unpack)
> printf("Unpacked %d entries from %s to %s using cache-tree\n",
>nr_entries,
> --
> 2.18.0.656.gda699b98b3

Seems reasonable, when we really do have to invoke call_unpack_fn.
I'm still curious if there are reasons why we couldn't just skip that
call (at least when o->fn is one of {oneway_merge, twoway_merge,
threeway_merge, bind_merge}), but I already brought that up in my
comments on patch 2.

Re: Help with "fatal: unable to read ...." error during GC?

[Jeff King]

On Wed, Aug 08, 2018 at 01:35:30PM -0400, Paul Smith wrote:

> Thanks for the note!  Unhappily for me none of these operations seem to
> find any actionable problems...
> [...]

Drat.

One other option is that it _could_ be related to the "old unreachable
objects that are reachable from recent unreachable objects should be
kept" code. That's supposed to quietly ignore broken links in
unreachable objects, but there could be a bug.

Let's narrow it down first and make sure we're dying where I expect. Can
you try:

  GIT_TRACE=1 git gc

and confirm the program running when the fatal error is produced?

>From what you've shown it's going to be git-repack, but what I'm not
clear on is whether it is repack itself that is complaining, or the
pack-objects process it spawns. I'd guess the latter.

If so, can you try running it under gdb and getting a stack trace?
Something like:

  gdb git
  [and then inside gdb...]
  set args pack-objects --all --reflog --indexed-objects foo 

Re: [PATCH v3 2/4] unpack-trees: optimize walking same trees with cache-tree

[Elijah Newren]

On Fri, Aug 3, 2018 at 10:39 PM Nguyễn Thái Ngọc Duy  wrote:
> From: Duy Nguyen 
>
> In order to merge one or many trees with the index, unpack-trees code
> walks multiple trees in parallel with the index and performs n-way
> merge. If we find out at start of a directory that all trees are the
> same (by comparing OID) and cache-tree happens to be available for
> that directory as well, we could avoid walking the trees because we
> already know what these trees contain: it's flattened in what's called
> "the index".

This is cool.

> The upside is of course a lot less I/O since we can potentially skip
> lots of trees (think subtrees). We also save CPU because we don't have
> to inflate and the apply deltas. The downside is of course more

s/and the apply/and apply the/

> fragile code since the logic in some functions are now duplicated
> elsewhere.
>
> "checkout -" with this patch on gcc.git:
>
> baseline  new
>   
> 0.018239226   0.019365414 s: read cache .git/index
> 0.052541655   0.049605548 s: preload index
> 0.001537598   0.001571695 s: refresh index
> 0.168167768   0.049677212 s: unpack trees
> 0.002897186   0.002845256 s: update worktree after a merge
> 0.131661745   0.136597522 s: repair cache-tree
> 0.075389117   0.075422517 s: write index, changed mask = 2a
> 0.111702023   0.032813253 s: unpack trees
> 0.23245   0.22002 s: update worktree after a merge
> 0.111793866   0.032933140 s: diff-index
> 0.587933288   0.398924370 s: git command: /home/pclouds/w/git/git
>
> Another measurement from Ben's running "git checkout" with over 500k
> trees (on the whole series):
>
> baselinenew
>   --
> 0.535510167 0.556558733 s: read cache .git/index
> 0.3057373   0.3147105   s: initialize name hash
> 0.0184082   0.023558433 s: preload index
> 0.086910967 0.089085967 s: refresh index
> 7.889590767 2.191554433 s: unpack trees
> 0.120760833 0.131941267 s: update worktree after a merge
> 2.2583504   2.572663167 s: repair cache-tree
> 0.8916137   0.959495233 s: write index, changed mask = 28
> 3.405199233 0.2710663   s: unpack trees
> 0.000999667 0.0021554   s: update worktree after a merge
> 3.4063306   0.273318333 s: diff-index
> 16.9524923  9.462943133 s: git command: git.exe checkout
>
> This command calls unpack_trees() twice, the first time on 2way merge
> and the second 1way merge. In both times, "unpack trees" time is
> reduced to one third. Overall time reduction is not that impressive of
> course because index operations take a big chunk. And there's that
> repair cache-tree line.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  unpack-trees.c | 117 +
>  1 file changed, 117 insertions(+)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index a32ddee159..ba3d2e947e 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -644,6 +644,102 @@ static inline int are_same_oid(struct name_entry 
> *name_j, struct name_entry *nam
> return name_j->oid && name_k->oid && !oidcmp(name_j->oid, 
> name_k->oid);
>  }
>
> +static int all_trees_same_as_cache_tree(int n, unsigned long dirmask,
> +   struct name_entry *names,
> +   struct traverse_info *info)
> +{
> +   struct unpack_trees_options *o = info->data;
> +   int i;
> +
> +   if (!o->merge || dirmask != ((1 << n) - 1))
> +   return 0;
> +
> +   for (i = 1; i < n; i++)
> +   if (!are_same_oid(names, names + i))
> +   return 0;
> +
> +   return cache_tree_matches_traversal(o->src_index->cache_tree, names, 
> info);
> +}

I was curious whether this could also be extended in the case of a
merge; as long as HEAD and MERGE have the same tree, even if the base
commit doesn't match, we can still just use the tree from HEAD which
should be in the current index/cache_tree.  However, it'd be a
somewhat odd history for HEAD and MERGE to match on some significantly
sized tree when the base commit doesn't also match.

> +
> +static int index_pos_by_traverse_info(struct name_entry *names,
> + struct traverse_info *info)
> +{
> +   struct unpack_trees_options *o = info->data;
> +   int len = traverse_path_len(info, names);
> +   char *name = xmalloc(len + 1 /* slash */ + 1 /* NUL */);
> +   int pos;
> +
> +   make_traverse_path(name, info, names);
> +   name[len++] = '/';
> +   name[len] = '\0';
> +   pos = index_name_pos(o->src_index, name, len);
> +   if (pos >= 0)
> +   BUG("This is a directory and should not exist in index");
> +   pos = -pos - 1;
> + 

Re: [PATCH v3 0/4] Speed up unpack_trees()

[Junio C Hamano]

Junio C Hamano  writes:

> not used behind is *not* OK.  And lack of restoring the bottom in
> the new codepath makes me suspect exactly such a bug _after_ the
> traversal exits the subtree we are using this new optimization in
> and moves on.

Hmph, thinking about this further, I cannot convince myself that
lack of bottom adjustment can lead to a triggerable bug.  The only
case that a subtree traversal need to skip some unpacked entries in
the index and then revisit them by rewinding, e.g. entries "t-i" and
"t-j" that are left unprocessed while entries "t/1", "t/2", etc. are
processed, in the illustration of da165f47 ("unpack-trees.c: prepare
for looking ahead in the index", 2010-01-07), is when one of the
trees have a non-tree with the same name as the subtree we are
trying to descend into, and as long as we know all trees have the
thing as a tree, I do not think of a case where such ordering
inversion would get in the way.

That was the only thing I found questionable in 2/4, which is the
most important piece in the series, so we probably are OK.

Thanks for working on this one.

Re: Help with "fatal: unable to read ...." error during GC?

[Paul Smith]

On Wed, 2018-08-08 at 12:06 -0400, Jeff King wrote:
> I'd have expected fsck to find it, too. However, looking at the code,
> I'm not convinced that fsck is actually considering detached worktree
> heads properly, either. Try:
> 
>   git rev-list --all --reflog --objects >/dev/null
> 
> which I know checks worktrees correctly. I'd expect that to fail.
> 
> If it does, then we need to narrow down which worktree is corrupt.
> Perhaps something like:
> 
>   git worktree list |
>   while read worktree head junk; do
> git rev-list --objects $head >/dev/null ||
> echo "$worktree seems corrupt"
>   done

Thanks for the note!  Unhappily for me none of these operations seem to
find any actionable problems...

$ git rev-list --all --reflog --objects >/dev/null
warning: reflog of 'HEAD' references pruned commits
warning: reflog of 'HEAD' references pruned commits
warning: reflog of 'HEAD' references pruned commits
warning: reflog of 'HEAD' references pruned commits
warning: reflog of 'HEAD' references pruned commits
warning: reflog of 'HEAD' references pruned commits
warning: reflog of 'HEAD' references pruned commits
warning: reflog of 'HEAD' references pruned commits
warning: reflog of 'HEAD' references pruned commits
warning: reflog of 'HEAD' references pruned commits
$ echo $?
0

$ git worktree list | while read wt head junk; do \
  git rev-list --objects $head >/dev/null || echo "$wt seems corrupt"; \
  done
$

Just to be sure I updated the loop above to echo $wt and $head and they
were correct.  I also re-ran git gc after the above and still got the
original error output so it didn't magically fix itself :).

Re: [PATCH v3 0/4] Speed up unpack_trees()

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Aug 6, 2018 at 5:48 PM Junio C Hamano  wrote:
>> > I've also checked about the lookahead thing in unpack_trees() to see
>> > if we accidentally break something there, which is my biggest worry.
>> > See [1] and [2] for context, but I believe since we can't have D/F
>> > conflicts, the situation where lookahead is needed will not occur. So
>> > we should be safe.

I think you would want the same "switch cache-bottom before
descending into a subdirectory, and then restore cache-bottom after
traversal comes back" dance that is done for the normal tree
traversal case to happen.

bottom = switch_cache_bottom();
ret = traverse_trees(n, t, );
restore_cache_bottom(, bottom);

During your walk of the index and the trees that are known to be in
sync, there is little reason to worry about the cache_bottom, which
is advanced by calling mark_ce_used() in traverse_by_cache_tree().
Where it matters is what happens after the traversal comes back out
of the subtree.  find_cache_pos() uses the bottom pointer so that it
does not have to go back to far to find an index entry that has not
been used to match with the entries from the trees (which are not
sorted exactly the same way as the index, unfortunately), so
forgetting to advance the bottom pointer while correctly marking a
ce as "used" is OK (i.e. hurts performance but not correctness), but
advancing the bottom pointer too much and leaving entries that are
not used behind is *not* OK.  And lack of restoring the bottom in
the new codepath makes me suspect exactly such a bug _after_ the
traversal exits the subtree we are using this new optimization in
and moves on.

Imagine we are iterating over the top-level of the trees, and found
a subtree in them.  There may be some index entries before the first
path in this subtree that are not yet marked as "used", the earliest
of which is pointed at by the cache_bottom pointer.

Before descending into the subtree (and start consuming the entry
with the first in this subtree from the index), we stash away the
current cache_bottom, and then start walking the subtree.  While we
are in that subtree, the cache_bottom starts from the first name in
the subtree and increments, as we _know_ the entry at the old
cache_bottom is outside this subtree and will not match any entry
form the subtree.  Then when the traversal returns, all index
entries within the "subtree/" path will be marked "used".  At that
point, when we continue to scan the top-level of the trees, we need
to restore the cache_bottom, so that we do not forget entries that
we knew we needed to scan eventually, if there was any.

Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike

[Stefan Beller]

On Wed, Aug 8, 2018 at 6:05 AM Johannes Schindelin
 wrote:

> > [...]
> > >   diff --git a/Makefile b/Makefile
> > >   --- a/Makefile
> > >   +++ b/Makefile
> >
> > The line starting with --- is red (default removed color) and the line
> > with +++ is green (default add color).
> >
> > Ideally these two lines and the third line above starting with "diff --git"
> > would render in GIT_COLOR_BOLD ("METAINFO").
>
> I agree that is not the best coloring here, but as you remarked elsewhere,
> it would require content-aware dual coloring, and I am loathe to try to
> implement that for two reasons: 1) it would take most likely a long time
> to design and implement that, and 2) I don't have that time.
>
> So I would like to declare that good enough is good enough in this case.

I anticipated this answer, so I wrote some patches myself, starting at
https://public-inbox.org/git/20180804015317.182683-1-sbel...@google.com/
specifically
https://public-inbox.org/git/20180804015317.182683-5-sbel...@google.com/

I plan on resending these on top of your resend (if any) at a later convenient
time for both you and Junio, as noted in
https://public-inbox.org/git/cagz79kznvesvpicnu7lxkrchurqgvesfvg3dl5o_2kpvyrw...@mail.gmail.com/


>
> > >   3:  076e1192d !  3:  4e3fb47a1 range-diff: first rudimentary 
> > > implementation
> > >  @@ -4,7 +4,7 @@
> > >
> > >   At this stage, `git range-diff` can determine corresponding 
> > > commits
> > >   of two related commit ranges. This makes use of the recently 
> > > introduced
> > >  -implementation of the Hungarian algorithm.
> > >  +implementation of the linear assignment algorithm.
> > >
> > >   The core of this patch is a straight port of the ideas of 
> > > tbdiff, the
> > >   apparently dormant project at https://github.com/trast/tbdiff.
> > >  @@ -51,19 +51,17 @@
> > >   + int res = 0;
> > >   + struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
> > >
> > >  -- argc = parse_options(argc, argv, NULL, options,
> > >  --  builtin_range_diff_usage, 0);
> > >  -+ argc = parse_options(argc, argv, NULL, options, 
> > > builtin_range_diff_usage,
> > >  -+  0);
> > >  +  argc = parse_options(argc, argv, NULL, options,
> > >  +   builtin_range_diff_usage, 0);
> >
> > This is really nice in colors when viewed locally.
> >
> > >  16:  dfa7b1e71 <  -:  - range-diff --dual-color: work around 
> > > bogus white-space warning
> > >   -:  - > 16:  f4252f2b2 range-diff --dual-color: fix bogus 
> > > white-space warning
> >
> > Ah; here my initial assumption of only reviewing the range-diff breaks down 
> > now.
> > I'll dig into patch 16 separately.
>
> Right. This was an almost complete rewrite, and then next iteration will
> hopefully bring another complete rewrite: disabling whitespace warnings in
> dual color mode.
>
> > Maybe it is worth having an option to expand all "new" patches.
>
> Sure.
>
> And I also have a use case for --left-only/--right-only.
>
> And I also have a strong use case (and so does Junio, it seems, or for
> that matter, anybody contributing to Git due to Junio's insistence on
> signing off on each patch, rather than on the merge commit) for something
> like --ignore-lines=.
>
> And you probably guess what I will say next: these features will make for
> really fantastic patch series *on top* of mine. There really is no good
> reason to delay the current patch series just to cram more features into
> it that had not been planned in the first place.

Yes, I agree. I am unsure about the current state of your series, though;

Junio thinks (expects?) a resend, whereas you seem to call it good enough
but also said (some time back) that you want to resend due to Thomas
feedback.

I do have 2 series on top of the current range-diff.
* The first (queued by Junio as origin/sb/range-diff-colors)
   adds a basic test for colors and improves diff.c readability
* The second (linked above) changes colors for some lines.

I do not want to build more on top as long as I do not know if
you resend (and how much it'll change)

Thanks,
Stefan

Re: [PATCH v3 0/4] Speed up unpack_trees()

[Ben Peart]

On 8/6/2018 2:59 PM, Junio C Hamano wrote:

Duy Nguyen  writes:


We require the unpacked entry from all input trees to be a tree
objects (the dirmask thing), so if one tree has 't' as a file,


Ah, OK, this is still part of that "all the trees match cache tree
so we walk the index instead" optimization.  I forgot about that.



I ran this set of patches through the VFS For Git set of functional 
tests as well as our performance test suite (my earlier perf numbers 
were from manual testing).  All the functional tests pass and the 
performance tests are looking _very_ promising.


Checkout times are impacted most and on average drop from 20.96	seconds 
to 11.63 seconds for a 45% savings.


Merge times drop from 19.44 seconds to 12.88 for a 34% savings.

Rebase times drop from 26.78 seconds to 20.72 for a 23% savings.

Overall, I'm looking forward to a good review of the patches and seeing 
them get merged as soon as they are ready.

Re: [PATCH 2/2] remote-curl: remove spurious period

[Elijah Newren]

On Wed, Aug 8, 2018 at 4:52 AM Johannes Schindelin via GitGitGadget
 wrote:
>
> We should not interrupt. sentences in the middle.

I love this. commit message.  :-)

...
> /* The client backend isn't giving us compressed data so
> -* we can try to deflate it ourselves, this may save on.
> +* we can try to deflate it ourselves, this may save on
>  * the transfer time.

Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set

[Junio C Hamano]

Jonathan Tan  writes:

>> Just a note (and a request-for-sanity-check) and not meant to be a
>> request to update the code, but with a still-in-pu 4b757a40 ("Use
>> remote_odb_get_direct() and has_remote_odb()", 2018-08-02) in
>> flight, repository_format_partial_clone is now gone.
>> ...
> Thanks for the heads-up. This makes me realize that even the current
> precondition (repository_format_partial_clone) is not an exact one - I
> should only be doing this if I know that there is at least one promisor
> object (if not, in the rare case that a repo is configured to be partial
> but does not have any promisor objects, repack will generate an empty
> packfile). In the next reroll, I'll take care of this, which should
> avoid this merge issue too.

I think that it is a good change, regardless of the semantic
conflict with any other topic, to look at the .promisor packs and
not look at the partial clone extension for this "do we want to
coalesce .promisor packs into one?" application.  You only want to
do so when you have two or more such packs; being in partial clone
might be a preconditon for having such packs, but it is overly loose.

In any case, remote-odb topic is turning out to be more trouble than
I originally thought; in the same change that removes the variable,
it actually removes the support for a repository configured with a
partial clone extension, and causes a hard error in t0410, breaking
test of 'pu'.  I do not know how many people actually use partial
clone right now, but their repositories would be broken the same way
when they update Git, if we merge that topic in its current shape.

I am dropping it from 'pu' for today's integration cycle.

[PATCHv4 0/5] Simple fixes to t7406

[Elijah Newren]

Changes since v3, all suggested/endorsed by Junio (range-diff at the end):
  - Moved the actual fix from being last patch in the series to the first
(other patches in this series are just test code cleanups)
  - Anchored regexes to avoid matching another filename as a substring
  - Add test_path_exists() to test-lib-function.sh and use it (we had
test_path_is_dir, test_path_is_file, and test_path_is_missing, but
not simple test_path_exists)

Elijah Newren (5):
  t7406: fix call that was failing for the wrong reason
  t7406: simplify by using diff --name-only instead of diff --raw
  t7406: avoid having git commands upstream of a pipe
  t7406: prefer test_* helper functions to test -[feds]
  t7406: avoid using test_must_fail for commands other than git

 t/t7406-submodule-update.sh | 37 +++--
 t/test-lib-functions.sh |  8 
 2 files changed, 31 insertions(+), 14 deletions(-)

-:  -- > 1:  5f257af6c8 t7406: fix call that was failing for the wrong 
reason
1:  3c369bf73d ! 2:  9e5400a1ad t7406: simplify by using diff --name-only 
instead of diff --raw
@@ -16,10 +16,10 @@
  compare_head
 ) &&
 -   git diff --raw | grep "submodule" &&
-+   git diff --name-only | grep submodule &&
++   git diff --name-only | grep ^submodule$ &&
 git submodule update &&
 -   git diff --raw | grep "submodule" &&
-+   git diff --name-only | grep submodule &&
++   git diff --name-only | grep ^submodule$ &&
 (cd submodule &&
  compare_head
 ) &&
@@ -28,10 +28,12 @@
  compare_head
 ) &&
 -   git diff --raw | grep "submodule" &&
-+   git diff --name-only | grep submodule &&
++   git diff --name-only | grep ^submodule$ &&
 git submodule update --checkout &&
--   test_must_fail git diff --raw \| grep "submodule" &&
-+   test_must_fail git diff --name-only \| grep submodule &&
+-   git diff --raw >out &&
+-   ! grep "   submodule" out &&
++   git diff --name-only >out &&
++   ! grep ^submodule$ out &&
 (cd submodule &&
  test_must_fail compare_head
 ) &&
2:  ba50d6b0f3 ! 3:  4e8cdf60f4 t7406: avoid having git commands upstream of a 
pipe
@@ -26,13 +26,13 @@
  git checkout master &&
  compare_head
 ) &&
--   git diff --name-only | grep submodule &&
+-   git diff --name-only | grep ^submodule$ &&
 +   git diff --name-only >out &&
-+   grep submodule out &&
++   grep ^submodule$ out &&
 git submodule update &&
--   git diff --name-only | grep submodule &&
+-   git diff --name-only | grep ^submodule$ &&
 +   git diff --name-only >out &&
-+   grep submodule out &&
++   grep ^submodule$ out &&
 (cd submodule &&
  compare_head
 ) &&
@@ -40,12 +40,12 @@
  git checkout master &&
  compare_head
 ) &&
--   git diff --name-only | grep submodule &&
+-   git diff --name-only | grep ^submodule$ &&
 +   git diff --name-only >out &&
-+   grep submodule out &&
++   grep ^submodule$ out &&
 git submodule update --checkout &&
-test_must_fail git diff --name-only \| grep submodule &&
-(cd submodule &&
+git diff --name-only >out &&
+! grep ^submodule$ out &&
 @@
 H=$(git rev-parse --short HEAD) &&
 git commit -am "pre move" &&
3:  42f7b7f225 ! 4:  f171cbcc9a t7406: prefer test_* helper functions to test 
-[feds]
@@ -6,13 +6,12 @@
 test failures, so use the test_* helper functions from
 test-lib-functions.sh.
 
-Note: The use of 'test_path_is_file submodule/.git' may look odd, but
-it is a file which is populated with a
+Also, add test_path_exists() to test-lib-function.sh while at it, so
+that we don't need to worry whether submodule/.git is a file or a
+directory.  It currently is a file with contents of the form
gitdir: ../.git/modules/submodule
-directive.  If, in the future, handling of the submodule is changed and
-submodule/.git becomes a directory we can change this to
-test_path_is_dir (or perhaps write a test_path_exists helper function
-that doesn't care whether the path is a file or a directory).
+but it could be changed in the future to be a directory; this test
+only really cares that it exists.
 
 Signed-off-by: Elijah Newren 
 
@@ -34,8 +33,27 @@
 git submodule update --init &&
 -   test -e submodule/.git &&
 -   test_must_fail test -e none/.git
-+   test_path_is_file submodule/.git &&
++   test_path_exists submodule/.git &&
 +   test_path_is_missing none/.git
)
  '
  
+
+diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
+--- a/t/test-lib-functions.sh