Re: [RESEND PATCH] diff: recurse into nested submodules for inline diff

[Jacob Keller]

On Thu, May 4, 2017 at 2:43 PM, Stefan Beller  wrote:
> When fd47ae6a5b (diff: teach diff to display submodule difference with an
> inline diff, 2016-08-31) was introduced, we did not think of recursing
> into nested submodules.
>
> When showing the inline diff for submodules, automatically recurse
> into nested submodules as well with inline submodule diffs.
>
> Signed-off-by: Stefan Beller 
> ---
>
> This is a resend of
> https://public-inbox.org/git/20170331231733.11123-3-sbel...@google.com/
>
> In the cover letter of that patch, I said I would want to redo the tests with
> scrubbed output. However given the widespread use of unscrubbed output,
> I think this is fine as-is to include.
>
> Thanks,
> Stefan

Fix looks obviously correct to me. I would like to change the tests to
not use the unscrubbed output, but that's a problem for a separate
patch.

Thanks,
Jake

Re: [PATCH 3/3] protocol docs: explain receive-pack push options

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> Support for push options in the receive-pack protocol (and all Git
> components that speak it) have been added over a few commits, but not
> fully documented (especially its interaction with signed pushes). Update
> the protocol documentation to include the relevant details.

Thanks.  This could be combined with the previous patch, since that
patch is enforcing what this patch documents.

[...]
> -This list is followed by a flush-pkt. Then the push options are transmitted
> -one per packet followed by another flush-pkt. After that the packfile that
> -should contain all the objects that the server will need to complete the new
> -references will be sent.
> +This list is followed by a flush-pkt.

I think this removed more than intended.

Before c714e45f (receive-pack: implement advertising and receiving
push options, 2016-07-14), this said

This list is followed by a flush-pkt and then the packfile that should
contain all the objects that the server will need to complete the new
references.

which I think would work fine.

[...]
> -  update-request=  *shallow ( command-list | push-cert ) [packfile]
> +  update-request=  *shallow ( command-list | push-cert )

This seems confusing to me both before and after.  How does
update-request get used?  Is it supposed to include the packfile or not?

Where do push-options fit into an unsigned request?

[...]
> @@ -500,12 +497,35 @@ references will be sent.
> PKT-LINE("pusher" SP ident LF)
> PKT-LINE("pushee" SP url LF)
> PKT-LINE("nonce" SP nonce LF)
> +   *PKT-LINE("push-option" SP push-option LF)
> PKT-LINE(LF)
> *PKT-LINE(command LF)
> *PKT-LINE(gpg-signature-lines LF)
> PKT-LINE("push-cert-end" LF)
>  
> -  packfile  = "PACK" 28*(OCTET)
> +  push-option   =  1*CHAR
> +
> +
> +If the server has advertised the 'push-options' capability and the client has
> +specified 'push-options' as part of the capability list above, the client 
> then
> +sends its push options followed by a flush-pkt.
> +
> +
> +  push-options  =  *PKT-LINE(push-option) flush-pkt
> +
> +
> +For backwards compatibility with older Git servers, if the client sends a 
> push
> +cert and push options, it SHOULD send its push options both embedded within 
> the

This needs to be a MUST, not SHOULD.

> +push cert and after the push cert. (Note that the push options within the 
> cert
> +are prefixed, but the push options after the cert are not.) Both these lists
> +SHOULD be consistent.

Likewise this one.

What does it mean to be consistent?

> +
> +After that the packfile that
> +should contain all the objects that the server will need to complete the new
> +references will be sent.
> +
> +
> +  packfile  =  "PACK" 28*(OCTET)
>  

Thanks,
Jonathan

Re: [PATCH 2/3] receive-pack: verify push options in cert

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan  wrote:

>> This sets in stone the requirement that send-pack redundantly send its
>> push options in 2 places, but I think that this is better than the
>> alternatives. Sending push options only within the cert is
>> backwards-incompatible with existing Git servers (which read push
>> options only from outside the cert), and sending push options only
>> outside the cert means that the push options are not signed for.
>
> As the combination of push certs and push options are broken
> (at least when using JGit as well, which are used in Gerrit
> installations), I would not feel too bad about actually going
> the backwards-incompatible way.

Per offline discussion, what you're proposing is to omit the second
copy of push options from the client request, so the server does not
have to see two copies.  I agree that that would be a better protocol,
but I think that ship has sailed.

Current versions of git the client and git the server are able to
interoperate without trouble (though the server-side bug is ugly in
terms of what the push certs mean).

Current versions of JGit the server *require* that the push options be
omitted from the push certificate.  I don't think there exists a
sensible way to interoperate with that, so we can ignore that JGit
server behavior as a bug (like you've said).

If we want to omit the second copy of the push certs (which sounds
like a reasonable thing to want), that would require a new capability
to be added to the protocol to do so.  That way, interoperability with
existing versions of git the client wouldn't be broken.  That could
happen on top of this series --- it is not needed for fixing the bug
that this series fixes.

To summarize:

 1. I agree that we shouldn't feel bad about breaking compatibility
with the JGit server behavior at issue, since there is no
reasonable way to be compatible with it.  And that's what this
series does --- it breaks compatibility with the broken versions
of JGit server and picks what the Git client has been doing
instead.

 2. I don't think we should break new versions of Git's ability to
interoperate with current versions of the server, even though I
consider the current server behavior to be broken.

 3. If someone is interested in getting rid of the redundant second
copy of push options, we have a way to do so, by introducing a
new capability

Thanks,
Jonathan

Re: [PATCH 2/3] receive-pack: verify push options in cert

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
> was taught to include push options both within the signed cert (if the
> push is a signed push) and outside the signed cert; however,
> receive-pack ignores push options within the cert, only handling push
> options outside the cert.

Yikes.  Thanks for fixing it.

[...]
> --- a/Documentation/git-receive-pack.txt
> +++ b/Documentation/git-receive-pack.txt
> @@ -106,6 +106,16 @@ the following environment variables:
>   Also read about `receive.certNonceSlop` variable in
>   linkgit:git-config[1].
>  
> +`GIT_PUSH_CERT_OPTION_STATUS`::
> +`BAD`;;
> + For backwards compatibility reasons, when accepting a signed
> + push, receive-pack requires that push options be written both
> + inside and outside the certificate. ("git push" does this
> + automatically.) `BAD` is returned if they are inconsistent.

In this manual page the reader doesn't need to know it's for backword
compatibility.  It is simply what the protocol requires.

The protocol doc and especially the commit message are better places
to talk about rationale (as you already are doing nicely in patch 3).

> +`OK`;;
> + The push options inside and outside the certificate are
> + consistent.

Hm.  I would have thought it would be simpler to simply reject the
push without invoking hooks in the BAD case.  But
GIT_PUSH_CERT_NONCE_STATUS provides precedent for this, forcing me to
think longer.

Is the idea that invoking the hook despite a bad client is a way to
provide an opportunity to audit log the error?

... okay, I've thought longer.  I think this is a different kind of
error than a bad nonce: for example, a bad nonce might be the result
of a misconfiguration that makes the client get the wrong nonce or a
sign of a caller trying to brute-force a stable nonce.  For
comparison, this error is a more simple protocol violation, like a
malformed pkt-line.

When we see a malformed pkt-line, we error out and do not invoke the
pre-receive hook.  For this error I think we should behave the same
way: show an error to the user and abort the connection, without
invoking hooks.

[...]
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -124,6 +124,21 @@ test_expect_success GPG 'signed push sends push 
> certificate' '
>   test_cmp expect dst/push-cert-status
>  '
>  
> +test_expect_success GPG 'signed push reports push option status in receive 
> hook' '

Is there a straightforward way to test the error case?  (If there
isn't, I can live with that --- it would just be nice to have a test
that the behavior introduced here is preserved.)

Thanks and hope that helps,
Jonathan

Re: [PATCH 3/3] protocol docs: explain receive-pack push options

[Stefan Beller]

On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan  wrote:
> Support for push options in the receive-pack protocol (and all Git
> components that speak it) have been added over a few commits, but not
> fully documented (especially its interaction with signed pushes). Update
> the protocol documentation to include the relevant details.
>
> Signed-off-by: Jonathan Tan 
> ---
>  Documentation/technical/pack-protocol.txt | 32 
> +--
>  1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/technical/pack-protocol.txt 
> b/Documentation/technical/pack-protocol.txt
> index 5b0ba3ef2..cf7cb48c3 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the 
> obj-id currently on
>  the server, the obj-id the client would like to update it to and the name
>  of the reference.
>
> -This list is followed by a flush-pkt. Then the push options are transmitted
> -one per packet followed by another flush-pkt. After that the packfile that
> -should contain all the objects that the server will need to complete the new
> -references will be sent.
> +This list is followed by a flush-pkt.
>
>  
> -  update-request=  *shallow ( command-list | push-cert ) [packfile]
> +  update-request=  *shallow ( command-list | push-cert )
>
>shallow   =  PKT-LINE("shallow" SP obj-id)
>
> @@ -500,12 +497,35 @@ references will be sent.
>   PKT-LINE("pusher" SP ident LF)
>   PKT-LINE("pushee" SP url LF)
>   PKT-LINE("nonce" SP nonce LF)
> + *PKT-LINE("push-option" SP push-option LF)
>   PKT-LINE(LF)
>   *PKT-LINE(command LF)
>   *PKT-LINE(gpg-signature-lines LF)
>   PKT-LINE("push-cert-end" LF)
>
> -  packfile  = "PACK" 28*(OCTET)
> +  push-option   =  1*CHAR
> +
> +
> +If the server has advertised the 'push-options' capability and the client has
> +specified 'push-options' as part of the capability list above, the client 
> then
> +sends its push options followed by a flush-pkt.

The CHAR of the push-options SHOULD NOT include an LF. Well I guess that may
be obvious when looking at PKT-LINE("push-option" SP push-option LF),
not sure if we want to mention it here.

> +
> +
> +  push-options  =  *PKT-LINE(push-option) flush-pkt
> +
> +
> +For backwards compatibility with older Git servers, if the client sends a 
> push
> +cert and push options, it SHOULD send its push options both embedded within 
> the
> +push cert and after the push cert. (Note that the push options within the 
> cert
> +are prefixed, but the push options after the cert are not.) Both these lists
> +SHOULD be consistent.

s/SHOULD/MUST/ ?

> +
> +After that the packfile that
> +should contain all the objects that the server will need to complete the new
> +references will be sent.
> +
> +
> +  packfile  =  "PACK" 28*(OCTET)
>  
>
>  If the receiving end does not support delete-refs, the sending end MUST
> --
> 2.13.0.rc1.294.g07d810a77f-goog

Thanks for writing the docs.

Stefan


>

Re: [PATCH 2/3] receive-pack: verify push options in cert

[Brandon Williams]

On 05/05, Stefan Beller wrote:
> On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan  wrote:
> > In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
> > was taught to include push options both within the signed cert (if the
> > push is a signed push) and outside the signed cert; however,
> > receive-pack ignores push options within the cert, only handling push
> > options outside the cert.
> >
> > Teach receive-pack, in the case that push options are provided for a
> > signed push, to verify that the push options both within the cert and
> > outside the cert are consistent, and to provide the results of that
> > verification to the receive hooks as an environment variable (just like
> > it currently does for the nonce verification).
> >
> > This sets in stone the requirement that send-pack redundantly send its
> > push options in 2 places, but I think that this is better than the
> > alternatives. Sending push options only within the cert is
> > backwards-incompatible with existing Git servers (which read push
> > options only from outside the cert), and sending push options only
> > outside the cert means that the push options are not signed for.
> 
> As the combination of push certs and push options are broken
> (at least when using JGit as well, which are used in Gerrit
> installations), I would not feel too bad about actually going
> the backwards-incompatible way.

yeah just think of the bits that could be saved!

But in all seriousness, I'd agree that doing the backwards-incompatible
way would be cleaner in the longer run (esp since they are broken
currently).


-- 
Brandon Williams

Re: [PATCH 2/3] receive-pack: verify push options in cert

[Stefan Beller]

On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan  wrote:
> In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
> was taught to include push options both within the signed cert (if the
> push is a signed push) and outside the signed cert; however,
> receive-pack ignores push options within the cert, only handling push
> options outside the cert.
>
> Teach receive-pack, in the case that push options are provided for a
> signed push, to verify that the push options both within the cert and
> outside the cert are consistent, and to provide the results of that
> verification to the receive hooks as an environment variable (just like
> it currently does for the nonce verification).
>
> This sets in stone the requirement that send-pack redundantly send its
> push options in 2 places, but I think that this is better than the
> alternatives. Sending push options only within the cert is
> backwards-incompatible with existing Git servers (which read push
> options only from outside the cert), and sending push options only
> outside the cert means that the push options are not signed for.

As the combination of push certs and push options are broken
(at least when using JGit as well, which are used in Gerrit
installations), I would not feel too bad about actually going
the backwards-incompatible way.

>
> Signed-off-by: Jonathan Tan 
> ---
>  Documentation/git-receive-pack.txt | 10 
>  builtin/receive-pack.c | 49 
> ++
>  t/t5534-push-signed.sh | 15 
>  3 files changed, 70 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-receive-pack.txt 
> b/Documentation/git-receive-pack.txt
> index 86a4b32f0..f50ca0f29 100644
> --- a/Documentation/git-receive-pack.txt
> +++ b/Documentation/git-receive-pack.txt
> @@ -106,6 +106,16 @@ the following environment variables:
> Also read about `receive.certNonceSlop` variable in
> linkgit:git-config[1].
>
> +`GIT_PUSH_CERT_OPTION_STATUS`::
> +`BAD`;;
> +   For backwards compatibility reasons, when accepting a signed
> +   push, receive-pack requires that push options be written both
> +   inside and outside the certificate. ("git push" does this
> +   automatically.) `BAD` is returned if they are inconsistent.
> +`OK`;;
> +   The push options inside and outside the certificate are
> +   consistent.
> +
>  This hook is called before any refname is updated and before any
>  fast-forward checks are performed.
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index f96834f42..fe26c2f72 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -81,6 +81,9 @@ static long nonce_stamp_slop;
>  static unsigned long nonce_stamp_slop_limit;
>  static struct ref_transaction *transaction;
>
> +static const char *PUSH_OPTION_BAD = "BAD";
> +static const char *PUSH_OPTION_OK = "OK";
> +
>  static enum {
> KEEPALIVE_NEVER = 0,
> KEEPALIVE_AFTER_NUL,
> @@ -473,7 +476,8 @@ static char *prepare_push_cert_nonce(const char *path, 
> unsigned long stamp)
>   * after dropping "_commit" from its name and possibly moving it out
>   * of commit.c
>   */
> -static char *find_header(const char *msg, size_t len, const char *key)
> +static char *find_header(const char *msg, size_t len, const char *key,
> +const char **next_line)
>  {
> int key_len = strlen(key);
> const char *line = msg;
> @@ -486,6 +490,8 @@ static char *find_header(const char *msg, size_t len, 
> const char *key)
> if (line + key_len < eol &&
> !memcmp(line, key, key_len) && line[key_len] == ' ') {
> int offset = key_len + 1;
> +   if (next_line)
> +   *next_line = *eol ? eol + 1 : eol;
> return xmemdupz(line + offset, (eol - line) - offset);
> }
> line = *eol ? eol + 1 : NULL;
> @@ -495,7 +501,7 @@ static char *find_header(const char *msg, size_t len, 
> const char *key)
>
>  static const char *check_nonce(const char *buf, size_t len)
>  {
> -   char *nonce = find_header(buf, len, "nonce");
> +   char *nonce = find_header(buf, len, "nonce", NULL);
> unsigned long stamp, ostamp;
> char *bohmac, *expect = NULL;
> const char *retval = NONCE_BAD;
> @@ -575,9 +581,40 @@ static const char *check_nonce(const char *buf, size_t 
> len)
> return retval;
>  }
>
> -static void prepare_push_cert_sha1(struct child_process *proc)
> +static const char *check_push_option(const char *buf, size_t len,
> +const struct string_list *push_options)
> +{
> +   int options_seen = 0;
> +   char *option;
> +   const char *next_line;
> +   const char *retval = PUSH_OPTION_OK;
> +
> +   while ((option = find_header(buf, len, 

Re: [PATCH 1/3] docs: correct receive.advertisePushOptions default

[Jonathan Nieder]

Jonathan Tan wrote:

> In commit c714e45 ("receive-pack: implement advertising and receiving
> push options", 2016-07-14), receive-pack was taught to (among other
> things) advertise that it understood push options, depending on
> configuration. It was documented that it advertised such ability by
> default; however, it actually does not. (In that commit, notice that
> advertise_push_options defaults to 0, unlike advertise_atomic_push which
> defaults to 1.)
>
> Update the documentation to state that it does not advertise the ability
> by default.
>
> Signed-off-by: Jonathan Tan 
> ---
>  Documentation/config.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d5..f49a2f3cb 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
>   capability, set this variable to false.
>  
>  receive.advertisePushOptions::
> - By default, git-receive-pack will advertise the push options
> - capability to its clients. If you don't want to advertise this
> - capability, set this variable to false.
> + When set to true, git-receive-pack will advertise the push options
> + capability to its clients.

Good catch.

Reviewed-by: Jonathan Nieder 

Possible improvements:

- Should this also say "The default is false"?

- git-receive-pack(1) doesn't say anything about push options, so
  without more context it's hard for a new git admin taking over
  from someone who had set this up to understand what's going on.
  Should this have a pointer to the pre-receive + post-receive
  sections of githooks(5)?

- Speaking of which, should git-receive-pack(1) say something
  about push options (for example to also have a pointer to
  githooks(5))?

- git-push(1) has the same problem: when describing the -o option, it
  doesn't give a pointer to where to find more detail (though it's a
  little more helpful then this one since it includes the word "hook").

Thanks,
Jonathan

Re: [PATCH 1/3] docs: correct receive.advertisePushOptions default

[Stefan Beller]

On Fri, May 5, 2017 at 4:50 PM, Brandon Williams  wrote:
> On 05/05, Jonathan Tan wrote:
>> In commit c714e45 ("receive-pack: implement advertising and receiving
>> push options", 2016-07-14), receive-pack was taught to (among other
>> things) advertise that it understood push options, depending on
>> configuration. It was documented that it advertised such ability by
>> default; however, it actually does not. (In that commit, notice that
>> advertise_push_options defaults to 0, unlike advertise_atomic_push which
>> defaults to 1.)
>
> This looks like a good fix to the documentation as advertise_push_options
> does indeed default to 0.
>

Indeed.

Thanks,
Stefan

Re: [PATCH 1/3] docs: correct receive.advertisePushOptions default

[Brandon Williams]

On 05/05, Jonathan Tan wrote:
> In commit c714e45 ("receive-pack: implement advertising and receiving
> push options", 2016-07-14), receive-pack was taught to (among other
> things) advertise that it understood push options, depending on
> configuration. It was documented that it advertised such ability by
> default; however, it actually does not. (In that commit, notice that
> advertise_push_options defaults to 0, unlike advertise_atomic_push which
> defaults to 1.)

This looks like a good fix to the documentation as advertise_push_options
does indeed default to 0.

> 
> Update the documentation to state that it does not advertise the ability
> by default.
> 
> Signed-off-by: Jonathan Tan 
> ---
>  Documentation/config.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d5..f49a2f3cb 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
>   capability, set this variable to false.
>  
>  receive.advertisePushOptions::
> - By default, git-receive-pack will advertise the push options
> - capability to its clients. If you don't want to advertise this
> - capability, set this variable to false.
> + When set to true, git-receive-pack will advertise the push options
> + capability to its clients.
>  
>  receive.autogc::
>   By default, git-receive-pack will run "git-gc --auto" after
> -- 
> 2.13.0.rc1.294.g07d810a77f-goog
> 

-- 
Brandon Williams

[PATCH 2/3] receive-pack: verify push options in cert

[Jonathan Tan]

In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
was taught to include push options both within the signed cert (if the
push is a signed push) and outside the signed cert; however,
receive-pack ignores push options within the cert, only handling push
options outside the cert.

Teach receive-pack, in the case that push options are provided for a
signed push, to verify that the push options both within the cert and
outside the cert are consistent, and to provide the results of that
verification to the receive hooks as an environment variable (just like
it currently does for the nonce verification).

This sets in stone the requirement that send-pack redundantly send its
push options in 2 places, but I think that this is better than the
alternatives. Sending push options only within the cert is
backwards-incompatible with existing Git servers (which read push
options only from outside the cert), and sending push options only
outside the cert means that the push options are not signed for.

Signed-off-by: Jonathan Tan 
---
 Documentation/git-receive-pack.txt | 10 
 builtin/receive-pack.c | 49 ++
 t/t5534-push-signed.sh | 15 
 3 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-receive-pack.txt 
b/Documentation/git-receive-pack.txt
index 86a4b32f0..f50ca0f29 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -106,6 +106,16 @@ the following environment variables:
Also read about `receive.certNonceSlop` variable in
linkgit:git-config[1].
 
+`GIT_PUSH_CERT_OPTION_STATUS`::
+`BAD`;;
+   For backwards compatibility reasons, when accepting a signed
+   push, receive-pack requires that push options be written both
+   inside and outside the certificate. ("git push" does this
+   automatically.) `BAD` is returned if they are inconsistent.
+`OK`;;
+   The push options inside and outside the certificate are
+   consistent.
+
 This hook is called before any refname is updated and before any
 fast-forward checks are performed.
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f96834f42..fe26c2f72 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -81,6 +81,9 @@ static long nonce_stamp_slop;
 static unsigned long nonce_stamp_slop_limit;
 static struct ref_transaction *transaction;
 
+static const char *PUSH_OPTION_BAD = "BAD";
+static const char *PUSH_OPTION_OK = "OK";
+
 static enum {
KEEPALIVE_NEVER = 0,
KEEPALIVE_AFTER_NUL,
@@ -473,7 +476,8 @@ static char *prepare_push_cert_nonce(const char *path, 
unsigned long stamp)
  * after dropping "_commit" from its name and possibly moving it out
  * of commit.c
  */
-static char *find_header(const char *msg, size_t len, const char *key)
+static char *find_header(const char *msg, size_t len, const char *key,
+const char **next_line)
 {
int key_len = strlen(key);
const char *line = msg;
@@ -486,6 +490,8 @@ static char *find_header(const char *msg, size_t len, const 
char *key)
if (line + key_len < eol &&
!memcmp(line, key, key_len) && line[key_len] == ' ') {
int offset = key_len + 1;
+   if (next_line)
+   *next_line = *eol ? eol + 1 : eol;
return xmemdupz(line + offset, (eol - line) - offset);
}
line = *eol ? eol + 1 : NULL;
@@ -495,7 +501,7 @@ static char *find_header(const char *msg, size_t len, const 
char *key)
 
 static const char *check_nonce(const char *buf, size_t len)
 {
-   char *nonce = find_header(buf, len, "nonce");
+   char *nonce = find_header(buf, len, "nonce", NULL);
unsigned long stamp, ostamp;
char *bohmac, *expect = NULL;
const char *retval = NONCE_BAD;
@@ -575,9 +581,40 @@ static const char *check_nonce(const char *buf, size_t len)
return retval;
 }
 
-static void prepare_push_cert_sha1(struct child_process *proc)
+static const char *check_push_option(const char *buf, size_t len,
+const struct string_list *push_options)
+{
+   int options_seen = 0;
+   char *option;
+   const char *next_line;
+   const char *retval = PUSH_OPTION_OK;
+
+   while ((option = find_header(buf, len, "push-option", _line))) {
+   len -= (next_line - buf);
+   buf = next_line;
+   options_seen++;
+   if (options_seen > push_options->nr
+   || strcmp(option,
+ push_options->items[options_seen - 1].string)) {
+   retval = PUSH_OPTION_BAD;
+   goto leave;
+   }
+   free(option);
+   }
+
+   if (options_seen != push_options->nr)
+   

[PATCH 1/3] docs: correct receive.advertisePushOptions default

[Jonathan Tan]

In commit c714e45 ("receive-pack: implement advertising and receiving
push options", 2016-07-14), receive-pack was taught to (among other
things) advertise that it understood push options, depending on
configuration. It was documented that it advertised such ability by
default; however, it actually does not. (In that commit, notice that
advertise_push_options defaults to 0, unlike advertise_atomic_push which
defaults to 1.)

Update the documentation to state that it does not advertise the ability
by default.

Signed-off-by: Jonathan Tan 
---
 Documentation/config.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5..f49a2f3cb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
capability, set this variable to false.
 
 receive.advertisePushOptions::
-   By default, git-receive-pack will advertise the push options
-   capability to its clients. If you don't want to advertise this
-   capability, set this variable to false.
+   When set to true, git-receive-pack will advertise the push options
+   capability to its clients.
 
 receive.autogc::
By default, git-receive-pack will run "git-gc --auto" after
-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH 3/3] protocol docs: explain receive-pack push options

[Jonathan Tan]

Support for push options in the receive-pack protocol (and all Git
components that speak it) have been added over a few commits, but not
fully documented (especially its interaction with signed pushes). Update
the protocol documentation to include the relevant details.

Signed-off-by: Jonathan Tan 
---
 Documentation/technical/pack-protocol.txt | 32 +--
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt 
b/Documentation/technical/pack-protocol.txt
index 5b0ba3ef2..cf7cb48c3 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the 
obj-id currently on
 the server, the obj-id the client would like to update it to and the name
 of the reference.
 
-This list is followed by a flush-pkt. Then the push options are transmitted
-one per packet followed by another flush-pkt. After that the packfile that
-should contain all the objects that the server will need to complete the new
-references will be sent.
+This list is followed by a flush-pkt.
 
 
-  update-request=  *shallow ( command-list | push-cert ) [packfile]
+  update-request=  *shallow ( command-list | push-cert )
 
   shallow   =  PKT-LINE("shallow" SP obj-id)
 
@@ -500,12 +497,35 @@ references will be sent.
  PKT-LINE("pusher" SP ident LF)
  PKT-LINE("pushee" SP url LF)
  PKT-LINE("nonce" SP nonce LF)
+ *PKT-LINE("push-option" SP push-option LF)
  PKT-LINE(LF)
  *PKT-LINE(command LF)
  *PKT-LINE(gpg-signature-lines LF)
  PKT-LINE("push-cert-end" LF)
 
-  packfile  = "PACK" 28*(OCTET)
+  push-option   =  1*CHAR
+
+
+If the server has advertised the 'push-options' capability and the client has
+specified 'push-options' as part of the capability list above, the client then
+sends its push options followed by a flush-pkt.
+
+
+  push-options  =  *PKT-LINE(push-option) flush-pkt
+
+
+For backwards compatibility with older Git servers, if the client sends a push
+cert and push options, it SHOULD send its push options both embedded within the
+push cert and after the push cert. (Note that the push options within the cert
+are prefixed, but the push options after the cert are not.) Both these lists
+SHOULD be consistent.
+
+After that the packfile that
+should contain all the objects that the server will need to complete the new
+references will be sent.
+
+
+  packfile  =  "PACK" 28*(OCTET)
 
 
 If the receiving end does not support delete-refs, the sending end MUST
-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH 0/3] Clarify interaction between signed pushes and push options

[Jonathan Tan]

We noticed this when trying to use Git to make a signed push with push
options to a server using JGit (which rejects such pushes because the
Git client makes requests that are, strictly speaking, incompatible with
the documented protocol).

There have been several commits (see the commits linked in the commit
messages of the patches sent in reply to this e-mail) to support push
options (that are read by receive hooks) when using "git push", but the
interaction between push options and signed pushes are not very clear.
Here are some patches (containing both code and documentation) that
clarify this interaction.

I would appreciate a review of this - if we could make the protocol
clear, we could then update JGit to use the updated protocol and be no
longer incompatible with existing Git clients.

Jonathan Tan (3):
  docs: correct receive.advertisePushOptions default
  receive-pack: verify push options in cert
  protocol docs: explain receive-pack push options

 Documentation/config.txt  |  5 ++--
 Documentation/git-receive-pack.txt| 10 +++
 Documentation/technical/pack-protocol.txt | 32 
 builtin/receive-pack.c| 49 ---
 t/t5534-push-signed.sh| 15 ++
 5 files changed, 98 insertions(+), 13 deletions(-)

-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Daniel Ferreira (theiostream)]

On Fri, May 5, 2017 at 7:38 PM, Johannes Schindelin
 wrote:
> But maybe you want to keep the naming a little more consistent with the
> Perl script, e.g. instead of calling the function `print_modified()` call
> it already `list()` (and rename it later to `list_and_choose()` once you
> have taught it to ask for a choice)?

Actually, I named it print_modified() because I anticipated there
would be no list_and_choose() equivalent in C. I don't think the
builtin should have the interactive menu + modified list + untracked
list being handled by one function. Rather, I think a saner way to go
on with it would be to create functions like print_untracked();
choose_from_input(); print_menu() etc.

This is still pure speculation from what I've written until now and
from the roadmap I have in my head (I do not know how writing code
from now on will actually be), but I have a hunch that
list_and_choose() is already convoluted enough in Perl; in C it might
become a monster.

> Thank you for this pleasant read!

Thank *you* for the quick and thorough review :)

-- Daniel.

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Ævar Arnfjörð Bjarmason]

On Sat, May 6, 2017 at 1:13 AM, Daniel Ferreira (theiostream)
 wrote:
> On Fri, May 5, 2017 at 7:30 PM, Johannes Schindelin
>  wrote:
>>> +static int git_add_interactive_config(const char *var,
>>
>> Not git_add_interactive__helper_config()? ;-)
>
> I don't get if you mean this ironically (because of the verbosity) or
> if you do think this would be a good name ;P
>
>>> + for (i = 0; i < q->nr; i++) {
>>> + struct diff_filepair *p;
>>> + p = q->queue[i];
>>> + diff_flush_stat(p, options, );
>>> + }
>>> +
>>> + for (i = 0; i < stat.nr; i++) {
>>> + int file_index = s->file_count;
>>> + for (j = 0; j < s->file_count; j++) {
>>> + if (!strcmp(s->files[j].path, stat.files[i]->name)) {
>>> + file_index = j;
>>> + break;
>>> + }
>>> + }
>>
>> So basically, this is looking up in a list whether we saw the file in
>> question already, and the reason we have to do that is that we run the
>> entire shebang twice, once with the worktree and once with the index.
>>
>> I wonder whether it would not make sense to switch away s->files from a
>> list to a hashmap.
>> [...]
>> BTW in the first pass, we pretty much know that we only get unique names,
>> so the entire lookup is unnecessary and will just increase the time
>> complexity from O(n) to O(n^2). So let's avoid that.
>>
>> By moving to a hashmap, you can even get the second phase down to an
>> expected O(n).
>
> How would you go about implementing that hashmap (i.e. what should be
> the hash)? Does Git have any interface for it, or is there any example
> I can look after in the codebase?

Git has a hashmap API already. 7d4558c462 is a good example of using it.

>>> + printf(ADD_INTERACTIVE_HEADER_INDENT);
>>> + color_fprintf(stdout, header_color, modified_fmt, _("staged"),
>>> + _("unstaged"), _("path"));
>>
>> I think these _() need to become N_().
>
> I cannot find any call to N_() outside of Perl code. What should that
> even do differently?

N_() is to mark a string for later translation, not to return it. See
my "how to use getopt" patch for an example of that, but this doesn't
look like such a case, since _() is returning the string to
color_fprintf, surely...

$ git grep -P '\bN_\(' -- '*.c'

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Daniel Ferreira (theiostream)]

On Fri, May 5, 2017 at 7:30 PM, Johannes Schindelin
 wrote:
>> +static int git_add_interactive_config(const char *var,
>
> Not git_add_interactive__helper_config()? ;-)

I don't get if you mean this ironically (because of the verbosity) or
if you do think this would be a good name ;P

>> + for (i = 0; i < q->nr; i++) {
>> + struct diff_filepair *p;
>> + p = q->queue[i];
>> + diff_flush_stat(p, options, );
>> + }
>> +
>> + for (i = 0; i < stat.nr; i++) {
>> + int file_index = s->file_count;
>> + for (j = 0; j < s->file_count; j++) {
>> + if (!strcmp(s->files[j].path, stat.files[i]->name)) {
>> + file_index = j;
>> + break;
>> + }
>> + }
>
> So basically, this is looking up in a list whether we saw the file in
> question already, and the reason we have to do that is that we run the
> entire shebang twice, once with the worktree and once with the index.
>
> I wonder whether it would not make sense to switch away s->files from a
> list to a hashmap.
> [...]
> BTW in the first pass, we pretty much know that we only get unique names,
> so the entire lookup is unnecessary and will just increase the time
> complexity from O(n) to O(n^2). So let's avoid that.
>
> By moving to a hashmap, you can even get the second phase down to an
> expected O(n).

How would you go about implementing that hashmap (i.e. what should be
the hash)? Does Git have any interface for it, or is there any example
I can look after in the codebase?

> Apart from using PATH_MAX bytes for most likely only short names: [...]

If not PATH_MAX, what should I go for? Make it a strbuf? I tend to
believe keeping that on the stack would be simpler and more optimal.

> Now that I read this and remember that only WORKTREE and INDEX are handled
> in the callback function: is there actually a use for the NONE enum value?
> I.e. is current_mode read out in any other context than the callback
> function? If there is no other read, then the NONE enum value is just
> confusing.

I just preferred to have a declared non-handled value than leave
something undefined behind. I felt it might avoid headaches in the
future with petty segfaults.

> Why not collapse all three functions into one? It is not like they are
> totally unrelated nor super-long.

To me it is a matter of personal preference to keep them separate. If
there is, however, any technical or project-style-related reason to
get them together, I'll certainly do it.

>> +static void print_modified(void)
>> +{
>> + int i;
>> + struct add_interactive_status s;
>> + const char *modified_fmt = _("%12s %12s %s");
>
> We cannot really translate that...

Apparently, we can. Ævar covered that in his reply.

>> + printf(ADD_INTERACTIVE_HEADER_INDENT);
>> + color_fprintf(stdout, header_color, modified_fmt, _("staged"),
>> + _("unstaged"), _("path"));
>
> I think these _() need to become N_().

I cannot find any call to N_() outside of Perl code. What should that
even do differently?

>> +static void status_cmd(void)
>> +{
>> + print_modified();
>> +}
>
> As long as this function really only calls another function with no
> parameters, let's just drop it. We can call print_modified() instead of
> status_cmd() just as easily.

I thought calling status_cmd() would make that more clear, but I agree
-- the options already make it clear enough,

I agree with all points I did not directly address. And thank you for
the review :)

-- Daniel.

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Ævar Arnfjörð Bjarmason]

On Sat, May 6, 2017 at 12:30 AM, Johannes Schindelin
 wrote:
> Hi Daniel,
>
>
> On Fri, 5 May 2017, Daniel Ferreira wrote:
>> +static void print_modified(void)
>> +{
>> + int i;
>> + struct add_interactive_status s;
>> + const char *modified_fmt = _("%12s %12s %s");
>
> We cannot really translate that...

He copied this from the *.perl code:

# TRANSLATORS: you can adjust this to align "git add -i" status menu
my $status_fmt = __('%12s %12s %s');

And one translation at least makes use of that (and probably others should).

But porting the /* TRANSLATORS: ... */ comment over is missing, and
should be added.

Re: [PATCH] t7406: fix i18n expectation of error message

[Johannes Sixt]

Am 05.05.2017 um 19:50 schrieb Ævar Arnfjörð Bjarmason:

On Fri, May 5, 2017 at 7:38 PM, Stefan Beller  wrote:

The error message from "submodule update" is internationalized, which
makes sense. The test however did not check for the translated version,
but used a hardcoded string, which breaks the test when run with
GETTEXT_POISON.

Signed-off-by: Stefan Beller 
---
 t/t7406-submodule-update.sh | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 4ac386d98b..12f6435ab0 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -441,13 +441,11 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
test_i18ncmp actual expect
 '

+sq="'"
 test_expect_success 'submodule update - command run for initial population of 
submodule' '
-   cat >expect <<-EOF &&
-   Execution of '\''false $submodulesha1'\'' failed in submodule path 
'\''submodule'\''
-   EOF
rm -rf super/submodule &&
test_must_fail git -C super submodule update 2>actual &&
-   test_cmp expect actual &&
+   test_i18ngrep "Execution of ${sq}false $submodulesha1${sq} failed in submodule path 
${sq}submodule${sq}" actual &&
git -C super submodule update --checkout
 '


I have a fix for this in my gettext fixup series (so far lingering on
the list, not in pu):
https://public-inbox.org/git/20170421185757.28978-1-ava...@gmail.com/

The diff is quite a bit smaller than yours:

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 4ac386d98b..034914a14f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -447,7 +447,7 @@ test_expect_success 'submodule update - command
run for initial population of su
EOF
rm -rf super/submodule &&
test_must_fail git -C super submodule update 2>actual &&
-   test_cmp expect actual &&
+   test_i18ncmp expect actual &&
git -C super submodule update --checkout
 '

Do you prefer to use i18ngrep for whatever reason? Seems better to use
i18ncmp there.


With grep, you can focus on the important parts. Programs on different 
platforms or in different environments sometimes poison stderr with 
unexpected stuff (we've observed this recently with the nd/fopen-errors 
series, also sh -x output can end up in the >2 destination).


Also, grep allows to write

test_i18ngrep "Execution of .false.* failed in submodule path 
.submodule." actual


Note the lack of ugly single-quote hacks and volatile object names.

-- Hannes

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Johannes Schindelin]

Hi Daniel,

On Fri, 5 May 2017, Daniel Ferreira wrote:

> Hm, it looks like my GSoC project won't be in the Git organization,

Yeah, rumors have it that you were quite a popular student and several
organizations fought bloody wars over getting you... ;-)

> although I'm still interested in going for this so I guess I'll send
> patches to implement my proposal anyway (although certainly in a slower
> pace than I would if on the program).

Nice!

> This series introduces git-add-interactive--helper (or should it be
> called git-add--interactive--helper?) as a builtin capable of doing
> what the Perl script's status_cmd() would do.

Since it is a temporary thing, and not user-visible, I would opt for
git-add--helper. I may have mentioned that a couple of times now... ;-)

> I wish this patch series could have been smaller, although I don't
> think it would make sense to bring add-interactive "subunits" smaller
> than a command to the helper, and status_cmd (aside from probably
> add_untracked_cmd) was the simplest one to do after all -- and still
> required ~250 lines on the new builtin.

I think it's fine. There has been a patch series consisting of ~80
individual patches before, as long as you do not go close to that one, I
think you are fine.

> Another regret I had was not being able to retire any code from the Perl
> script yet (and will likely not be able to do so until all commands have
> been ported), but that is not such a big thing after all.

All in due time.

But maybe you want to keep the naming a little more consistent with the
Perl script, e.g. instead of calling the function `print_modified()` call
it already `list()` (and rename it later to `list_and_choose()` once you
have taught it to ask for a choice)?

> As for the new builtin, I think the color handling code is pretty
> straightforward. In fact, it was practically copied from places like
> clean.c or diff.c (which makes me wonder if some of that code could
> be generalized to avoid duplication). The same goes for the pretty
> simple option parsing code.

True. It does look awfully similar to other code handling colors.

Maybe put it on the backlog. Or the GSoC get-your-feet-wet projects for
next year.

> Bigger issues seem to arise when dealing with getting the numstat.
> While (as Junio anticipated on an RFC) some tasks like getting the
> actual diff and splitting it may require making the "diff
> machinery" write to a temporary file that we will read and do things
> with, I think it would be weird to do that for parsing a simple
> numstat from it.

I would actually think that the callback Junio talked about would be more
appropriate than to write temporary files. Even to split the diff. We can
do that much more efficiently in-memory.

> My first instinct was to create something like
> show_numstat_interactive() or something on diff.c (analogous to the
> other show_* functions). Doing that, however, would stumble upon another
> issue: we would not be able to print both a file's diff against the
> index (obtained from run_diff_index) and against the worktree (obtained
> from run_diff_files) in that function. The solution I came up with was
> to export the diffstat interface from diff.c into the world and allow
> our new builtin to use that and build our status_cmd output. Unless this
> breaks some rule of Git's API design, the result seems pretty reasonable
> to me.

It looks pretty reasonable to me, too.

Thank you for this pleasant read!
Johannes

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
> On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira  wrote:

>> This series introduces git-add-interactive--helper (or should it be
>> called git-add--interactive--helper?) as a builtin capable of doing
>> what the Perl script's status_cmd() would do.
>
> The existing script is git-add--interactive.perl, so
> git-add--interactive--helper.c would be consistent with that, since
> there's no git-add-interactive command.

Or git-add--interactive-helper.c, or git-add--helper.c.

The important thing is that there's a double-dash somewhere.

Thanks,
Jonathan

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Johannes Schindelin]

Hi,

On Fri, 5 May 2017, Ævar Arnfjörð Bjarmason wrote:

> On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira  wrote:
> > This series introduces git-add-interactive--helper (or should it be
> > called git-add--interactive--helper?) as a builtin capable of doing
> > what the Perl script's status_cmd() would do.
> 
> The existing script is git-add--interactive.perl, so
> git-add--interactive--helper.c would be consistent with that, since
> there's no git-add-interactive command.

Actually, to be consistent with the existing git-rebase--helper example
(which is also only helping the interactive version), it should be
git-add--helper.

Ciao,
Johannes

Re: [PATCH 3/3] add--interactive: use add-interactive--helper for status_cmd

[Johannes Schindelin]

Hi Daniel,


On Fri, 5 May 2017, Daniel Ferreira wrote:

> Call the newly introduced git-add-interactive--helper builtin on
> status_cmd() instead of relying on git-add--interactive's Perl
> functions to build print the numstat.
> 
> Signed-off-by: Daniel Ferreira 
> ---
>  git-add--interactive.perl | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 709a5f6..8617462 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -632,9 +632,7 @@ EOF
>  }
>  
>  sub status_cmd {
> - list_and_choose({ LIST_ONLY => 1, HEADER => $status_head },
> - list_modified());
> - print "\n";
> + system(qw(git add-interactive--helper --status));
>  }
>  
>  sub say_n_paths {

Obviously good!

Thanks,
Johannes

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Johannes Schindelin]

Hi Daniel,


On Fri, 5 May 2017, Daniel Ferreira wrote:

> Create a builtin helper for git-add--interactive, which right now is
> only able to reproduce git-add--interactive.perl's status_cmd()
> function, providing a summarized diff numstat to the user.
> 
> This is the first step in an effort to convert git-add--interactive.perl
> to a C builtin, in search for better portability, expressibility and
> performance (specially on non-POSIX systems like Windows).
> 
> Additionally, an eventual complete port of git-add--interactive would
> remove the last "big" Git script to have Perl as a dependency, allowing
> most Git users to have a NOPERL build running without big losses.
> 
> Signed-off-by: Daniel Ferreira 

Good. I would think that git-add--helper is a better name. It is an
internal command only, anyway, and hopefully it will go away soon (i.e.
hopefully all of add -i will be builtin soon and can then be even moved
into a separate file outside builtin/, say, add-interactive.c).

> diff --git a/.gitignore b/.gitignore
> index 833ef3b..0d6cfe4 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -11,6 +11,7 @@
>  /git
>  /git-add
>  /git-add--interactive
> +/git-add-interactive--helper

Personally, I would prefer a separate commit adding the helper, before it
learns about any functionality. Somehow I have an easier time reviewing
patches if they tease such things apart.

> diff --git a/builtin/add-interactive--helper.c 
> b/builtin/add-interactive--helper.c
> new file mode 100644
> index 000..97ca1b3
> --- /dev/null
> +++ b/builtin/add-interactive--helper.c
> @@ -0,0 +1,258 @@
> +#include "builtin.h"
> +#include "color.h"
> +#include "diff.h"
> +#include "diffcore.h"
> +#include "revision.h"
> +
> +#define ADD_INTERACTIVE_HEADER_INDENT "  "

This definition is local to this helper, and we already know that this is
all about add -i. So maybe not repeat it? HEADER_INDENT would be catchier
and just as expressive.

> +enum add_interactive_collection_mode {
> + COLLECTION_MODE_NONE,
> + COLLECTION_MODE_WORKTREE,
> + COLLECTION_MODE_INDEX
> +};

Likewise, maybe we do not want to have this COLLECTION_MODE_ prefix
littering the source code... NONE, WORKTREE and INDEX should be short and
sweet.

> +struct add_interactive_file_status {

Just struct file_status, maybe?

From a cursory glance, it also would appear that the only need for this
struct to be named is the memset() call when initializing a new item,
right? If that is the case, it would be better to use sizeof(*s->files)
and keep it unnamed, as part of the enclosing struct.

> + int selected;
> +
> + char path[PATH_MAX];

That is a bit wasteful. `char *name` is what diffstat_t uses...

> + int lines_added_index;
> + int lines_deleted_index;
> + int lines_added_worktree;
> + int lines_deleted_worktree;

Maybe for better readability:

struct {
uintmax_t added, deleted;
} index, worktree;

> +};
>
> +struct add_interactive_status {
> + enum add_interactive_collection_mode current_mode;

I am scared of future patches where you cannot wrap the code to the
required <= 80 columns/row because the data type or variable name is too
long... ;-)

> +
> + const char *reference;
> + struct pathspec pathspec;
> +
> + int file_count;
> + struct add_interactive_file_status *files;

The convention for growable arrays in Git is to have nr, alloc and the
array, and use ALLOC_GROW() to avoid reallocating for every single new
item.

> +};
> +
> +static int add_interactive_use_color = -1;
> +enum color_add_interactive {
> + ADD_INTERACTIVE_PROMPT,
> + ADD_INTERACTIVE_HEADER,
> + ADD_INTERACTIVE_HELP,
> + ADD_INTERACTIVE_ERROR
> +};

Again, if we were talking about a declaration in a .h file, that prefix
would make sense, but here we are safely in file-local territory where I
think this is just unnecessary churn.

> +static char add_interactive_colors[][COLOR_MAXLEN] = {
> + GIT_COLOR_BOLD_BLUE, /* Prompt */
> + GIT_COLOR_BOLD,  /* Header */
> + GIT_COLOR_BOLD_RED,  /* Help */
> + GIT_COLOR_BOLD_RED   /* Error */
> +};
> +
> +static const char *add_interactive_get_color(enum color_add_interactive ix)
> +{
> + if (want_color(add_interactive_use_color))
> + return add_interactive_colors[ix];
> + return "";
> +}
> +
> +static int parse_add_interactive_color_slot(const char *slot)
> +{
> + if (!strcasecmp(slot, "prompt"))
> + return ADD_INTERACTIVE_PROMPT;
> + if (!strcasecmp(slot, "header"))
> + return ADD_INTERACTIVE_HEADER;
> + if (!strcasecmp(slot, "help"))
> + return ADD_INTERACTIVE_HELP;
> + if (!strcasecmp(slot, "error"))
> + return ADD_INTERACTIVE_ERROR;
> +
> + return -1;
> +}
> +
> +static int git_add_interactive_config(const char *var,

Not git_add_interactive__helper_config()? ;-)

> + const char *value, void *cbdata)
> +{
> +

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Ævar Arnfjörð Bjarmason]

On Fri, May 5, 2017 at 11:21 PM, Johannes Schindelin
 wrote:
> Hi,
>
> On Fri, 5 May 2017, Ævar Arnfjörð Bjarmason wrote:
>
>>  int cmd_add_interactive__helper(int argc, const char **argv, const char 
>> *prefix)
>>  {
>> - int i, found_opt = 0;
>> -
>> - git_config(git_add_interactive_config, NULL);
>> + int opt_status = 0;
>>
>> - for (i = 1; i < argc; i++) {
>> - const char *arg = argv[i];
>> + struct option options[] = {
>> + OPT_BOOL(0, "status", _status,
>> +  N_("print status information with diffstat")),
>
> Please use OPT_CMDMODE() instead; it was invented exactly for this kind of
> scenario.

Indeed. Forgot about that. That's much better.

>> --
>> 2.11.0
>
> Really? ;-)

CC-ing the Debian stretch release team with your complaint [not really].

I don't bother to 'make install' the git I'm hacking on, so E-Mail
gets sent with whatever's in Debian testing.

Attn: fund owner Payment Notification,

[Secretary Ad-hoc committee]

Dear Sir,

Please be informed that your payment $6.5 million US dollars has been approved.
You are expected to contact Charles Anthony (Mr.) on: (officemail...@gmail.com)
with the following for proper verification and claim processing Telephone;
+1925-231-8472 on any development thus Note that you have no financial
obligation in this arrangement which would be perfected from the
United States next week .

Yours sincerely,
Mrs. Anita Benson.
Secretary Ad-hoc committee.

Re: [PATCH 1/3] diff: export diffstat interface

[Johannes Schindelin]

Hi Daniel,

On Fri, 5 May 2017, Daniel Ferreira wrote:

> Make the diffstat interface (namely, the diffstat_t struct and
> diff_flush_stat) no longer be internal to diff.c and allow it to be used
> by other parts of git.
> 
> This is helpful for code that may want to easily extract information
> from files using the diff machinery, while flushing it differently from
> how the show_* functions used by diff_flush() do it.
> 
> Signed-off-by: Daniel Ferreira 

Maybe add one more paragraph with the motivation, i.e. the conversion of
add -i into a builtin?

The patch is straight-forward, and I do not even see any names that may
need to be adjusted for a more visible API than a file-local function.

> -- 
> 2.7.4 (Apple Git-66)

I guess I take my comment about Ævar's Git version back ;-)

Ciao,
Johannes

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Johannes Schindelin]

Hi,

On Fri, 5 May 2017, Ævar Arnfjörð Bjarmason wrote:

>  int cmd_add_interactive__helper(int argc, const char **argv, const char 
> *prefix)
>  {
> - int i, found_opt = 0;
> -
> - git_config(git_add_interactive_config, NULL);
> + int opt_status = 0;
>  
> - for (i = 1; i < argc; i++) {
> - const char *arg = argv[i];
> + struct option options[] = {
> + OPT_BOOL(0, "status", _status,
> +  N_("print status information with diffstat")),

Please use OPT_CMDMODE() instead; it was invented exactly for this kind of
scenario.

> -- 
> 2.11.0

Really? ;-)

Ciao,
Dscho

Re: How to `git status' without scrambling modified with new, etc

[Samuel Lijin]

On Fri, May 5, 2017 at 9:24 AM, Ævar Arnfjörð Bjarmason
 wrote:
> On Fri, May 5, 2017 at 4:02 PM, Harry Putnam  wrote:
>> This is probably what everyone sees:
>>
>> When I run `git status'; I see  modified and newfiles scrambled together
>>
>> Is there a trick or technique to make that output show each category
>> separately?

You can use `git status -s` and match on the modification type (M
corresponds to modified, A to new files). See the man page for more
details on the interface.

>> Or do folks just a throw a `sort' in there (git status|sort) and lose
>> the color ouput?
>
> If you set color.ui=auto it'll disable coloring when it detects that
> the output isn't to a terminal, i.e. being piped.
>
> It sounds like you want:
>
> git -c color.ui=always status --short|sort
>
> But there's no native option to sort the status output, but that'll do
> it for you.

Re: Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity

[Johannes Schindelin]

Hi Stefan,


On Fri, 28 Apr 2017, Johannes Schindelin wrote:

> On Fri, 28 Apr 2017, Stefan Beller wrote:
> 
> > On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin
> >  wrote:
> > 
> > > I still have to find the time to figure out one more detail: how to
> > > download and extract the Coverity tool (the .zip archive has a
> > > variable name for the top-level directory), and doing that only
> > > every once in a while, say, only when there is no previously
> > > unpacked tool, or it is already 4 weeks old.
> > 
> > That is an interesting problem, which I ignored as the older versions
> > of their tools still works once they release new versions. So I just
> > manually check every once in a while if they have new versions out
> > there.
> > 
> > So if you find a nice solution to that problem, let me know, please.
> 
> I think I have a working idea (jotting it down in the editor, untested):
> 
> [... totally untested snippet ...]

And now I edited it and tested it. The code is now part of the script I
use for pretty much all administrative (i.e. recurring and boring) tasks
in the Git for Windows project:

https://github.com/git-for-windows/build-extra/commit/05b5342128

May it be useful to you, too, at least as a starting point,
Dscho

Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Ævar Arnfjörð Bjarmason]

On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira  wrote:
> Create a builtin helper for git-add--interactive, which right now is
> only able to reproduce git-add--interactive.perl's status_cmd()
> function, providing a summarized diff numstat to the user.

I'm not a user of add -i and didn't review the main logic in detail,
but I ran this, and aside from two issues this LGTM:

 * You missed a trailing \n in the output, so your formatting is
   different from the current behavior.

 * You should be using the getopt API instead of rolling your own.

Fixes for both below, thanks a lot for hacking on this. As you pointed
out this doesn't help with removing any of the perl code for now, but
after replacing a few more command modes we can start chopping away at
the perl code.

diff --git a/builtin/add-interactive--helper.c 
b/builtin/add-interactive--helper.c
index 97ca1b38dc..ea0f790bd3 100644
--- a/builtin/add-interactive--helper.c
+++ b/builtin/add-interactive--helper.c
@@ -226,6 +226,7 @@ static void print_modified(void)
printf(modified_fmt, index_changes, worktree_changes, f.path);
printf("\n");
}
+   printf("\n");
 }
 
 static void status_cmd(void)
@@ -233,26 +234,31 @@ static void status_cmd(void)
print_modified();
 }
 
-static const char add_interactive_helper_usage[] =
-"git add-interactive--helper ";
+static const char * const builtin_add_interactive_helper_usage[] = {
+   N_("git add-interactive--helper "),
+   NULL
+};
 
 int cmd_add_interactive__helper(int argc, const char **argv, const char 
*prefix)
 {
-   int i, found_opt = 0;
-
-   git_config(git_add_interactive_config, NULL);
+   int opt_status = 0;
 
-   for (i = 1; i < argc; i++) {
-   const char *arg = argv[i];
+   struct option options[] = {
+   OPT_BOOL(0, "status", _status,
+N_("print status information with diffstat")),
+   OPT_END()
+   };
 
-   if (!strcmp(arg, "--status")) {
-   status_cmd();
-   found_opt = 1;
-   }
-   }
-
-   if (!found_opt)
-   usage(add_interactive_helper_usage);
+   git_config(git_add_interactive_config, NULL);
+   argc = parse_options(argc, argv, NULL, options,
+builtin_add_interactive_helper_usage,
+PARSE_OPT_KEEP_ARGV0);
+
+   if (opt_status)
+   status_cmd();
+   else
+   usage_with_options(builtin_add_interactive_helper_usage,
+  options);
 
return 0;
 }
-- 
2.11.0

[RFC 00/14] convert dir.c to take an index parameter

[Brandon Williams]

One of the things brought up on the list in the past few days has been
migrating away from using the index compatibility macros.  One of the issues
brought up in that thread was how simply doing that conversion doesn't
eliminate the reliance on global state (specifically the_index).  If one day we
want to have a 'repository object' passed around then we first need to convert
different subsystems to be prepared to handle that.  This series provides a
first step, converting the code in dir.c to take a 'struct index_state' and
using that instead of implicitly using 'the_index'.

Brandon Williams (14):
  dir: stop using the index compatibility macros
  dir: convert read_skip_worktree_file_from_index to take an index
  dir: convert directory_exists_in_index to take index
  dir: convert get_dtype to take index
  dir: convert dir_add* to take an index
  dir: convert last_exclude_matching_from_list to take an index
  dir: convert is_excluded_from_list to take an index
  dir: convert add_excludes to take an index
  dir: convert prep_exclude to take an index
  dir: convert is_excluded to take an index
  dir: convert open_cached_dir to take an index
  dir: convert read_directory_recursive to take an index
  dir: convert read_directory to take an index
  dir: convert fill_directory to take an index

 builtin/add.c  |   7 +-
 builtin/check-ignore.c |   3 +-
 builtin/clean.c|   4 +-
 builtin/grep.c |   2 +-
 builtin/ls-files.c |   4 +-
 dir.c  | 200 -
 dir.h  |  27 +--
 unpack-trees.c |  10 +--
 wt-status.c|   2 +-
 9 files changed, 151 insertions(+), 108 deletions(-)

-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 09/14] dir: convert prep_exclude to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/dir.c b/dir.c
index b86d02ff9..50b5e720e 100644
--- a/dir.c
+++ b/dir.c
@@ -1046,7 +1046,9 @@ static struct exclude 
*last_exclude_matching_from_lists(struct dir_struct *dir,
  * Loads the per-directory exclude list for the substring of base
  * which has a char length of baselen.
  */
-static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
+static void prep_exclude(struct dir_struct *dir,
+struct index_state *istate,
+const char *base, int baselen)
 {
struct exclude_list_group *group;
struct exclude_list *el;
@@ -1125,7 +1127,7 @@ static void prep_exclude(struct dir_struct *dir, const 
char *base, int baselen)
int dt = DT_DIR;
dir->basebuf.buf[stk->baselen - 1] = 0;
dir->exclude = last_exclude_matching_from_lists(dir,
-   
_index,
+   istate,
dir->basebuf.buf, stk->baselen - 1,
dir->basebuf.buf + current, );
dir->basebuf.buf[stk->baselen - 1] = '/';
@@ -1167,7 +1169,7 @@ static void prep_exclude(struct dir_struct *dir, const 
char *base, int baselen)
strbuf_addbuf(, >basebuf);
strbuf_addstr(, dir->exclude_per_dir);
el->src = strbuf_detach(, NULL);
-   add_excludes(el->src, el->src, stk->baselen, el, 
_index,
+   add_excludes(el->src, el->src, stk->baselen, el, istate,
 untracked ? _stat : NULL);
}
/*
@@ -1209,7 +1211,7 @@ struct exclude *last_exclude_matching(struct dir_struct 
*dir,
const char *basename = strrchr(pathname, '/');
basename = (basename) ? basename+1 : pathname;
 
-   prep_exclude(dir, pathname, basename-pathname);
+   prep_exclude(dir, _index, pathname, basename-pathname);
 
if (dir->exclude)
return dir->exclude;
@@ -1695,10 +1697,10 @@ static int valid_cached_dir(struct dir_struct *dir,
 */
if (path->len && path->buf[path->len - 1] != '/') {
strbuf_addch(path, '/');
-   prep_exclude(dir, path->buf, path->len);
+   prep_exclude(dir, _index, path->buf, path->len);
strbuf_setlen(path, path->len - 1);
} else
-   prep_exclude(dir, path->buf, path->len);
+   prep_exclude(dir, _index, path->buf, path->len);
 
/* hopefully prep_exclude() haven't invalidated this entry... */
return untracked->valid;
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 03/14] dir: convert directory_exists_in_index to take index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/dir.c b/dir.c
index 44307b963..5c635198d 100644
--- a/dir.c
+++ b/dir.c
@@ -1264,14 +1264,15 @@ enum exist_status {
  * the directory name; instead, use the case insensitive
  * directory hash.
  */
-static enum exist_status directory_exists_in_index_icase(const char *dirname, 
int len)
+static enum exist_status directory_exists_in_index_icase(struct index_state 
*istate,
+const char *dirname, 
int len)
 {
struct cache_entry *ce;
 
-   if (index_dir_exists(_index, dirname, len))
+   if (index_dir_exists(istate, dirname, len))
return index_directory;
 
-   ce = index_file_exists(_index, dirname, len, ignore_case);
+   ce = index_file_exists(istate, dirname, len, ignore_case);
if (ce && S_ISGITLINK(ce->ce_mode))
return index_gitdir;
 
@@ -1285,18 +1286,19 @@ static enum exist_status 
directory_exists_in_index_icase(const char *dirname, in
  * the files it contains) will sort with the '/' at the
  * end.
  */
-static enum exist_status directory_exists_in_index(const char *dirname, int 
len)
+static enum exist_status directory_exists_in_index(struct index_state *istate,
+  const char *dirname, int len)
 {
int pos;
 
if (ignore_case)
-   return directory_exists_in_index_icase(dirname, len);
+   return directory_exists_in_index_icase(istate, dirname, len);
 
-   pos = index_name_pos(_index, dirname, len);
+   pos = index_name_pos(istate, dirname, len);
if (pos < 0)
pos = -pos-1;
-   while (pos < the_index.cache_nr) {
-   const struct cache_entry *ce = the_index.cache[pos++];
+   while (pos < istate->cache_nr) {
+   const struct cache_entry *ce = istate->cache[pos++];
unsigned char endchar;
 
if (strncmp(ce->name, dirname, len))
@@ -1351,7 +1353,7 @@ static enum path_treatment treat_directory(struct 
dir_struct *dir,
const struct pathspec *pathspec)
 {
/* The "len-1" is to strip the final '/' */
-   switch (directory_exists_in_index(dirname, len-1)) {
+   switch (directory_exists_in_index(_index, dirname, len-1)) {
case index_directory:
return path_recurse;
 
@@ -1554,7 +1556,7 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
if ((dir->flags & DIR_COLLECT_KILLED_ONLY) &&
(dtype == DT_DIR) &&
!has_path_in_index &&
-   (directory_exists_in_index(path->buf, path->len) == 
index_nonexistent))
+   (directory_exists_in_index(_index, path->buf, path->len) == 
index_nonexistent))
return path_none;
 
exclude = is_excluded(dir, path->buf, );
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 08/14] dir: convert add_excludes to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c  | 29 +++--
 dir.h  |  2 +-
 unpack-trees.c |  2 +-
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/dir.c b/dir.c
index 497a2db85..b86d02ff9 100644
--- a/dir.c
+++ b/dir.c
@@ -730,7 +730,7 @@ static void invalidate_directory(struct untracked_cache *uc,
 
 /*
  * Given a file with name "fname", read it (either from disk, or from
- * the index if "check_index" is non-zero), parse it and store the
+ * an index if 'istate' is non-null), parse it and store the
  * exclude rules in "el".
  *
  * If "ss" is not NULL, compute SHA-1 of the exclude file and fill
@@ -738,7 +738,8 @@ static void invalidate_directory(struct untracked_cache *uc,
  * ss_valid is non-zero, "ss" must contain good value as input.
  */
 static int add_excludes(const char *fname, const char *base, int baselen,
-   struct exclude_list *el, int check_index,
+   struct exclude_list *el,
+   struct index_state *istate,
struct sha1_stat *sha1_stat)
 {
struct stat st;
@@ -752,8 +753,8 @@ static int add_excludes(const char *fname, const char 
*base, int baselen,
warn_on_inaccessible(fname);
if (0 <= fd)
close(fd);
-   if (!check_index ||
-   (buf = read_skip_worktree_file_from_index(_index, 
fname, , sha1_stat)) == NULL)
+   if (!istate ||
+   (buf = read_skip_worktree_file_from_index(istate, fname, 
, sha1_stat)) == NULL)
return -1;
if (size == 0) {
free(buf);
@@ -785,15 +786,15 @@ static int add_excludes(const char *fname, const char 
*base, int baselen,
if (sha1_stat) {
int pos;
if (sha1_stat->valid &&
-   !match_stat_data_racy(_index, _stat->stat, 
))
+   !match_stat_data_racy(istate, _stat->stat, 
))
; /* no content change, ss->sha1 still good */
-   else if (check_index &&
-(pos = index_name_pos(_index, fname, 
strlen(fname))) >= 0 &&
-!ce_stage(the_index.cache[pos]) &&
-ce_uptodate(the_index.cache[pos]) &&
+   else if (istate &&
+(pos = index_name_pos(istate, fname, 
strlen(fname))) >= 0 &&
+!ce_stage(istate->cache[pos]) &&
+ce_uptodate(istate->cache[pos]) &&
 !would_convert_to_git(fname))
hashcpy(sha1_stat->sha1,
-   the_index.cache[pos]->oid.hash);
+   istate->cache[pos]->oid.hash);
else
hash_sha1_file(buf, size, "blob", 
sha1_stat->sha1);
fill_stat_data(_stat->stat, );
@@ -824,9 +825,9 @@ static int add_excludes(const char *fname, const char 
*base, int baselen,
 
 int add_excludes_from_file_to_list(const char *fname, const char *base,
   int baselen, struct exclude_list *el,
-  int check_index)
+  struct index_state *istate)
 {
-   return add_excludes(fname, base, baselen, el, check_index, NULL);
+   return add_excludes(fname, base, baselen, el, istate, NULL);
 }
 
 struct exclude_list *add_exclude_list(struct dir_struct *dir,
@@ -858,7 +859,7 @@ static void add_excludes_from_file_1(struct dir_struct 
*dir, const char *fname,
if (!dir->untracked)
dir->unmanaged_exclude_files++;
el = add_exclude_list(dir, EXC_FILE, fname);
-   if (add_excludes(fname, "", 0, el, 0, sha1_stat) < 0)
+   if (add_excludes(fname, "", 0, el, NULL, sha1_stat) < 0)
die("cannot use %s as an exclude file", fname);
 }
 
@@ -1166,7 +1167,7 @@ static void prep_exclude(struct dir_struct *dir, const 
char *base, int baselen)
strbuf_addbuf(, >basebuf);
strbuf_addstr(, dir->exclude_per_dir);
el->src = strbuf_detach(, NULL);
-   add_excludes(el->src, el->src, stk->baselen, el, 1,
+   add_excludes(el->src, el->src, stk->baselen, el, 
_index,
 untracked ? _stat : NULL);
}
/*
diff --git a/dir.h b/dir.h
index 64254c7e7..1bcda0d23 100644
--- a/dir.h
+++ b/dir.h
@@ -243,7 +243,7 @@ extern int is_excluded(struct dir_struct *dir, const char 
*name, int *dtype);
 extern struct exclude_list *add_exclude_list(struct dir_struct *dir,
 int 

[RFC 11/14] dir: convert open_cached_dir to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index a15da672c..f08d30ee4 100644
--- a/dir.c
+++ b/dir.c
@@ -1664,6 +1664,7 @@ static void add_untracked(struct untracked_cache_dir 
*dir, const char *name)
 
 static int valid_cached_dir(struct dir_struct *dir,
struct untracked_cache_dir *untracked,
+   struct index_state *istate,
struct strbuf *path,
int check_only)
 {
@@ -1678,7 +1679,7 @@ static int valid_cached_dir(struct dir_struct *dir,
return 0;
}
if (!untracked->valid ||
-   match_stat_data_racy(_index, >stat_data, )) {
+   match_stat_data_racy(istate, >stat_data, )) {
if (untracked->valid)
invalidate_directory(dir->untracked, untracked);
fill_stat_data(>stat_data, );
@@ -1699,10 +1700,10 @@ static int valid_cached_dir(struct dir_struct *dir,
 */
if (path->len && path->buf[path->len - 1] != '/') {
strbuf_addch(path, '/');
-   prep_exclude(dir, _index, path->buf, path->len);
+   prep_exclude(dir, istate, path->buf, path->len);
strbuf_setlen(path, path->len - 1);
} else
-   prep_exclude(dir, _index, path->buf, path->len);
+   prep_exclude(dir, istate, path->buf, path->len);
 
/* hopefully prep_exclude() haven't invalidated this entry... */
return untracked->valid;
@@ -1711,12 +1712,13 @@ static int valid_cached_dir(struct dir_struct *dir,
 static int open_cached_dir(struct cached_dir *cdir,
   struct dir_struct *dir,
   struct untracked_cache_dir *untracked,
+  struct index_state *istate,
   struct strbuf *path,
   int check_only)
 {
memset(cdir, 0, sizeof(*cdir));
cdir->untracked = untracked;
-   if (valid_cached_dir(dir, untracked, path, check_only))
+   if (valid_cached_dir(dir, untracked, istate, path, check_only))
return 0;
cdir->fdir = opendir(path->len ? path->buf : ".");
if (dir->untracked)
@@ -1789,7 +1791,7 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
 
strbuf_add(, base, baselen);
 
-   if (open_cached_dir(, dir, untracked, , check_only))
+   if (open_cached_dir(, dir, untracked, _index, , 
check_only))
goto out;
 
if (untracked)
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 14/14] dir: convert fill_directory to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/add.c  | 2 +-
 builtin/clean.c| 2 +-
 builtin/grep.c | 2 +-
 builtin/ls-files.c | 2 +-
 dir.c  | 6 --
 dir.h  | 4 +++-
 wt-status.c| 2 +-
 7 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 0b52aeced..36cad00ae 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -400,7 +400,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
 
/* This picks up the paths that are not tracked */
-   baselen = fill_directory(, );
+   baselen = fill_directory(, _index, );
if (pathspec.nr)
seen = prune_directory(, , baselen);
}
diff --git a/builtin/clean.c b/builtin/clean.c
index 39866afab..329b68c40 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -930,7 +930,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
   PATHSPEC_PREFER_CWD,
   prefix, argv);
 
-   fill_directory(, );
+   fill_directory(, _index, );
 
for (i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
diff --git a/builtin/grep.c b/builtin/grep.c
index 65070c52f..ca1344133 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -866,7 +866,7 @@ static int grep_directory(struct grep_opt *opt, const 
struct pathspec *pathspec,
if (exc_std)
setup_standard_excludes();
 
-   fill_directory(, pathspec);
+   fill_directory(, _index, pathspec);
for (i = 0; i < dir.nr; i++) {
if (!dir_path_match(dir.entries[i], pathspec, 0, NULL))
continue;
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index ccba227e2..239c18a1f 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -326,7 +326,7 @@ static void show_files(struct dir_struct *dir)
if (show_others || show_killed) {
if (!show_others)
dir->flags |= DIR_COLLECT_KILLED_ONLY;
-   fill_directory(dir, );
+   fill_directory(dir, _index, );
if (show_others)
show_other_files(dir);
if (show_killed)
diff --git a/dir.c b/dir.c
index 4eb8cb6a2..3f3167e55 100644
--- a/dir.c
+++ b/dir.c
@@ -177,7 +177,9 @@ char *common_prefix(const struct pathspec *pathspec)
return len ? xmemdupz(pathspec->items[0].match, len) : NULL;
 }
 
-int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
+int fill_directory(struct dir_struct *dir,
+  struct index_state *istate,
+  const struct pathspec *pathspec)
 {
const char *prefix;
size_t prefix_len;
@@ -190,7 +192,7 @@ int fill_directory(struct dir_struct *dir, const struct 
pathspec *pathspec)
prefix = prefix_len ? pathspec->items[0].match : "";
 
/* Read the directory and prune it */
-   read_directory(dir, _index, prefix, prefix_len, pathspec);
+   read_directory(dir, istate, prefix, prefix_len, pathspec);
 
return prefix_len;
 }
diff --git a/dir.h b/dir.h
index a23bcd005..17d110693 100644
--- a/dir.h
+++ b/dir.h
@@ -214,7 +214,9 @@ extern int match_pathspec(const struct pathspec *pathspec,
 extern int report_path_error(const char *ps_matched, const struct pathspec 
*pathspec, const char *prefix);
 extern int within_depth(const char *name, int namelen, int depth, int 
max_depth);
 
-extern int fill_directory(struct dir_struct *dir, const struct pathspec 
*pathspec);
+extern int fill_directory(struct dir_struct *dir,
+ struct index_state *istate,
+ const struct pathspec *pathspec);
 extern int read_directory(struct dir_struct *, struct index_state *istate,
  const char *path, int len,
  const struct pathspec *pathspec);
diff --git a/wt-status.c b/wt-status.c
index 308cf3779..9e2d03cd9 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -652,7 +652,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
dir.untracked = the_index.untracked;
setup_standard_excludes();
 
-   fill_directory(, >pathspec);
+   fill_directory(, _index, >pathspec);
 
for (i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 10/14] dir: convert is_excluded to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/add.c  |  2 +-
 builtin/check-ignore.c |  3 ++-
 builtin/clean.c|  2 +-
 builtin/ls-files.c |  2 +-
 dir.c  | 16 +---
 dir.h  |  5 -
 unpack-trees.c |  2 +-
 7 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index bf5e676e4..0b52aeced 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -436,7 +436,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 !file_exists(path))) {
if (ignore_missing) {
int dtype = DT_UNKNOWN;
-   if (is_excluded(, path, ))
+   if (is_excluded(, _index, path, 
))
dir_add_ignored(, 
_index,
path, 
pathspec.items[i].len);
} else
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 1d73d3ca3..d2293b22e 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -101,7 +101,8 @@ static int check_ignore(struct dir_struct *dir,
full_path = pathspec.items[i].match;
exclude = NULL;
if (!seen[i]) {
-   exclude = last_exclude_matching(dir, full_path, );
+   exclude = last_exclude_matching(dir, _index,
+   full_path, );
}
if (!quiet && (exclude || show_non_matching))
output_exclude(pathspec.items[i].original, exclude);
diff --git a/builtin/clean.c b/builtin/clean.c
index d861f836a..39866afab 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -683,7 +683,7 @@ static int filter_by_patterns_cmd(void)
for_each_string_list_item(item, _list) {
int dtype = DT_UNKNOWN;
 
-   if (is_excluded(, item->string, )) {
+   if (is_excluded(, _index, item->string, 
)) {
*item->string = '\0';
changed++;
}
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d449e46db..ccba227e2 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -315,7 +315,7 @@ static void show_ru_info(void)
 static int ce_excluded(struct dir_struct *dir, const struct cache_entry *ce)
 {
int dtype = ce_to_dtype(ce);
-   return is_excluded(dir, ce->name, );
+   return is_excluded(dir, _index, ce->name, );
 }
 
 static void show_files(struct dir_struct *dir)
diff --git a/dir.c b/dir.c
index 50b5e720e..a15da672c 100644
--- a/dir.c
+++ b/dir.c
@@ -1204,19 +1204,20 @@ static void prep_exclude(struct dir_struct *dir,
  * undecided.
  */
 struct exclude *last_exclude_matching(struct dir_struct *dir,
-const char *pathname,
-int *dtype_p)
+ struct index_state *istate,
+ const char *pathname,
+ int *dtype_p)
 {
int pathlen = strlen(pathname);
const char *basename = strrchr(pathname, '/');
basename = (basename) ? basename+1 : pathname;
 
-   prep_exclude(dir, _index, pathname, basename-pathname);
+   prep_exclude(dir, istate, pathname, basename-pathname);
 
if (dir->exclude)
return dir->exclude;
 
-   return last_exclude_matching_from_lists(dir, _index, pathname, 
pathlen,
+   return last_exclude_matching_from_lists(dir, istate, pathname, pathlen,
basename, dtype_p);
 }
 
@@ -1225,10 +1226,11 @@ struct exclude *last_exclude_matching(struct dir_struct 
*dir,
  * scans all exclude lists to determine whether pathname is excluded.
  * Returns 1 if true, otherwise 0.
  */
-int is_excluded(struct dir_struct *dir, const char *pathname, int *dtype_p)
+int is_excluded(struct dir_struct *dir, struct index_state *istate,
+   const char *pathname, int *dtype_p)
 {
struct exclude *exclude =
-   last_exclude_matching(dir, pathname, dtype_p);
+   last_exclude_matching(dir, istate, pathname, dtype_p);
if (exclude)
return exclude->flags & EXC_FLAG_NEGATIVE ? 0 : 1;
return 0;
@@ -1573,7 +1575,7 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
(directory_exists_in_index(_index, path->buf, path->len) == 
index_nonexistent))
return path_none;
 
-   exclude = is_excluded(dir, path->buf, );
+   exclude = is_excluded(dir, _index, path->buf, );
 
/*
 * Excluded? If we don't explicitly want to show
diff --git a/dir.h b/dir.h
index 

[RFC 07/14] dir: convert is_excluded_from_list to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c  | 5 +++--
 dir.h  | 6 --
 unpack-trees.c | 4 ++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/dir.c b/dir.c
index 077b756c2..497a2db85 100644
--- a/dir.c
+++ b/dir.c
@@ -1010,10 +1010,11 @@ static struct exclude 
*last_exclude_matching_from_list(const char *pathname,
  */
 int is_excluded_from_list(const char *pathname,
  int pathlen, const char *basename, int *dtype,
- struct exclude_list *el)
+ struct exclude_list *el, struct index_state *istate)
 {
struct exclude *exclude;
-   exclude = last_exclude_matching_from_list(pathname, pathlen, basename, 
dtype, el, _index);
+   exclude = last_exclude_matching_from_list(pathname, pathlen, basename,
+ dtype, el, istate);
if (exclude)
return exclude->flags & EXC_FLAG_NEGATIVE ? 0 : 1;
return -1; /* undecided */
diff --git a/dir.h b/dir.h
index a9f809983..64254c7e7 100644
--- a/dir.h
+++ b/dir.h
@@ -217,8 +217,10 @@ extern int within_depth(const char *name, int namelen, int 
depth, int max_depth)
 extern int fill_directory(struct dir_struct *dir, const struct pathspec 
*pathspec);
 extern int read_directory(struct dir_struct *, const char *path, int len, 
const struct pathspec *pathspec);
 
-extern int is_excluded_from_list(const char *pathname, int pathlen, const char 
*basename,
-int *dtype, struct exclude_list *el);
+extern int is_excluded_from_list(const char *pathname, int pathlen,
+const char *basename, int *dtype,
+struct exclude_list *el,
+struct index_state *istate);
 struct dir_entry *dir_add_ignored(struct dir_struct *dir,
  struct index_state *istate,
  const char *pathname, int len);
diff --git a/unpack-trees.c b/unpack-trees.c
index 6b7356dab..9bcb13e4f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1040,7 +1040,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, 
int nr,
struct cache_entry **cache_end;
int dtype = DT_DIR;
int ret = is_excluded_from_list(prefix->buf, prefix->len,
-   basename, , el);
+   basename, , el, _index);
int rc;
 
strbuf_addch(prefix, '/');
@@ -1143,7 +1143,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, 
int nr,
/* Non-directory */
dtype = ce_to_dtype(ce);
ret = is_excluded_from_list(ce->name, ce_namelen(ce),
-   name, , el);
+   name, , el, _index);
if (ret < 0)
ret = defval;
if (ret > 0)
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 13/14] dir: convert read_directory to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c  | 16 
 dir.h  |  4 +++-
 unpack-trees.c |  2 +-
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/dir.c b/dir.c
index 3318ebbcb..4eb8cb6a2 100644
--- a/dir.c
+++ b/dir.c
@@ -190,7 +190,7 @@ int fill_directory(struct dir_struct *dir, const struct 
pathspec *pathspec)
prefix = prefix_len ? pathspec->items[0].match : "";
 
/* Read the directory and prune it */
-   read_directory(dir, prefix, prefix_len, pathspec);
+   read_directory(dir, _index, prefix, prefix_len, pathspec);
 
return prefix_len;
 }
@@ -2071,8 +2071,8 @@ static struct untracked_cache_dir 
*validate_untracked_cache(struct dir_struct *d
return root;
 }
 
-int read_directory(struct dir_struct *dir, const char *path,
-  int len, const struct pathspec *pathspec)
+int read_directory(struct dir_struct *dir, struct index_state *istate,
+  const char *path, int len, const struct pathspec *pathspec)
 {
struct untracked_cache_dir *untracked;
 
@@ -2086,8 +2086,8 @@ int read_directory(struct dir_struct *dir, const char 
*path,
 * e.g. prep_exclude()
 */
dir->untracked = NULL;
-   if (!len || treat_leading_path(dir, _index, path, len, pathspec))
-   read_directory_recursive(dir, _index, path, len, untracked, 
0, pathspec);
+   if (!len || treat_leading_path(dir, istate, path, len, pathspec))
+   read_directory_recursive(dir, istate, path, len, untracked, 0, 
pathspec);
QSORT(dir->entries, dir->nr, cmp_name);
QSORT(dir->ignored, dir->ignored_nr, cmp_name);
if (dir->untracked) {
@@ -2101,12 +2101,12 @@ int read_directory(struct dir_struct *dir, const char 
*path,
 dir->untracked->gitignore_invalidated,
 dir->untracked->dir_invalidated,
 dir->untracked->dir_opened);
-   if (dir->untracked == the_index.untracked &&
+   if (dir->untracked == istate->untracked &&
(dir->untracked->dir_opened ||
 dir->untracked->gitignore_invalidated ||
 dir->untracked->dir_invalidated))
-   the_index.cache_changed |= UNTRACKED_CHANGED;
-   if (dir->untracked != the_index.untracked) {
+   istate->cache_changed |= UNTRACKED_CHANGED;
+   if (dir->untracked != istate->untracked) {
free(dir->untracked);
dir->untracked = NULL;
}
diff --git a/dir.h b/dir.h
index b745f1e5d..a23bcd005 100644
--- a/dir.h
+++ b/dir.h
@@ -215,7 +215,9 @@ extern int report_path_error(const char *ps_matched, const 
struct pathspec *path
 extern int within_depth(const char *name, int namelen, int depth, int 
max_depth);
 
 extern int fill_directory(struct dir_struct *dir, const struct pathspec 
*pathspec);
-extern int read_directory(struct dir_struct *, const char *path, int len, 
const struct pathspec *pathspec);
+extern int read_directory(struct dir_struct *, struct index_state *istate,
+ const char *path, int len,
+ const struct pathspec *pathspec);
 
 extern int is_excluded_from_list(const char *pathname, int pathlen,
 const char *basename, int *dtype,
diff --git a/unpack-trees.c b/unpack-trees.c
index abdd2922b..48d9cf921 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1564,7 +1564,7 @@ static int verify_clean_subdirectory(const struct 
cache_entry *ce,
memset(, 0, sizeof(d));
if (o->dir)
d.exclude_per_dir = o->dir->exclude_per_dir;
-   i = read_directory(, pathbuf, namelen+1, NULL);
+   i = read_directory(, _index, pathbuf, namelen+1, NULL);
if (i)
return o->gently ? -1 :
add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 02/14] dir: convert read_skip_worktree_file_from_index to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/dir.c b/dir.c
index 97eadd45e..44307b963 100644
--- a/dir.c
+++ b/dir.c
@@ -588,7 +588,8 @@ void add_exclude(const char *string, const char *base,
x->el = el;
 }
 
-static void *read_skip_worktree_file_from_index(const char *path, size_t *size,
+static void *read_skip_worktree_file_from_index(const struct index_state 
*istate,
+   const char *path, size_t *size,
struct sha1_stat *sha1_stat)
 {
int pos, len;
@@ -597,12 +598,12 @@ static void *read_skip_worktree_file_from_index(const 
char *path, size_t *size,
void *data;
 
len = strlen(path);
-   pos = index_name_pos(_index, path, len);
+   pos = index_name_pos(istate, path, len);
if (pos < 0)
return NULL;
-   if (!ce_skip_worktree(the_index.cache[pos]))
+   if (!ce_skip_worktree(istate->cache[pos]))
return NULL;
-   data = read_sha1_file(the_index.cache[pos]->oid.hash, , );
+   data = read_sha1_file(istate->cache[pos]->oid.hash, , );
if (!data || type != OBJ_BLOB) {
free(data);
return NULL;
@@ -610,7 +611,7 @@ static void *read_skip_worktree_file_from_index(const char 
*path, size_t *size,
*size = xsize_t(sz);
if (sha1_stat) {
memset(_stat->stat, 0, sizeof(sha1_stat->stat));
-   hashcpy(sha1_stat->sha1, the_index.cache[pos]->oid.hash);
+   hashcpy(sha1_stat->sha1, istate->cache[pos]->oid.hash);
}
return data;
 }
@@ -751,7 +752,7 @@ static int add_excludes(const char *fname, const char 
*base, int baselen,
if (0 <= fd)
close(fd);
if (!check_index ||
-   (buf = read_skip_worktree_file_from_index(fname, , 
sha1_stat)) == NULL)
+   (buf = read_skip_worktree_file_from_index(_index, 
fname, , sha1_stat)) == NULL)
return -1;
if (size == 0) {
free(buf);
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 04/14] dir: convert get_dtype to take index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/dir.c b/dir.c
index 5c635198d..4515f0083 100644
--- a/dir.c
+++ b/dir.c
@@ -48,7 +48,8 @@ struct cached_dir {
 static enum path_treatment read_directory_recursive(struct dir_struct *dir,
const char *path, int len, struct untracked_cache_dir *untracked,
int check_only, const struct pathspec *pathspec);
-static int get_dtype(struct dirent *de, const char *path, int len);
+static int get_dtype(struct dirent *de, struct index_state *istate,
+const char *path, int len);
 
 int fspathcmp(const char *a, const char *b)
 {
@@ -975,7 +976,7 @@ static struct exclude 
*last_exclude_matching_from_list(const char *pathname,
 
if (x->flags & EXC_FLAG_MUSTBEDIR) {
if (*dtype == DT_UNKNOWN)
-   *dtype = get_dtype(NULL, pathname, pathlen);
+   *dtype = get_dtype(NULL, _index, pathname, 
pathlen);
if (*dtype != DT_DIR)
continue;
}
@@ -1459,12 +1460,13 @@ static int exclude_matches_pathspec(const char *path, 
int pathlen,
return 0;
 }
 
-static int get_index_dtype(const char *path, int len)
+static int get_index_dtype(struct index_state *istate,
+  const char *path, int len)
 {
int pos;
const struct cache_entry *ce;
 
-   ce = index_file_exists(_index, path, len, 0);
+   ce = index_file_exists(istate, path, len, 0);
if (ce) {
if (!ce_uptodate(ce))
return DT_UNKNOWN;
@@ -1478,12 +1480,12 @@ static int get_index_dtype(const char *path, int len)
}
 
/* Try to look it up as a directory */
-   pos = index_name_pos(_index, path, len);
+   pos = index_name_pos(istate, path, len);
if (pos >= 0)
return DT_UNKNOWN;
pos = -pos-1;
-   while (pos < the_index.cache_nr) {
-   ce = the_index.cache[pos++];
+   while (pos < istate->cache_nr) {
+   ce = istate->cache[pos++];
if (strncmp(ce->name, path, len))
break;
if (ce->name[len] > '/')
@@ -1497,14 +1499,15 @@ static int get_index_dtype(const char *path, int len)
return DT_UNKNOWN;
 }
 
-static int get_dtype(struct dirent *de, const char *path, int len)
+static int get_dtype(struct dirent *de, struct index_state *istate,
+const char *path, int len)
 {
int dtype = de ? DTYPE(de) : DT_UNKNOWN;
struct stat st;
 
if (dtype != DT_UNKNOWN)
return dtype;
-   dtype = get_index_dtype(path, len);
+   dtype = get_index_dtype(istate, path, len);
if (dtype != DT_UNKNOWN)
return dtype;
if (lstat(path, ))
@@ -1529,7 +1532,7 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
int has_path_in_index = !!index_file_exists(_index, path->buf, 
path->len, ignore_case);
 
if (dtype == DT_UNKNOWN)
-   dtype = get_dtype(de, path->buf, path->len);
+   dtype = get_dtype(de, _index, path->buf, path->len);
 
/* Always exclude indexed files */
if (dtype != DT_DIR && has_path_in_index)
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 05/14] dir: convert dir_add* to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/add.c |  3 ++-
 dir.c | 18 +++---
 dir.h |  4 +++-
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 9f53f020d..bf5e676e4 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -437,7 +437,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (ignore_missing) {
int dtype = DT_UNKNOWN;
if (is_excluded(, path, ))
-   dir_add_ignored(, path, 
pathspec.items[i].len);
+   dir_add_ignored(, 
_index,
+   path, 
pathspec.items[i].len);
} else
die(_("pathspec '%s' did not match any 
files"),
pathspec.items[i].original);
diff --git a/dir.c b/dir.c
index 4515f0083..a508e8076 100644
--- a/dir.c
+++ b/dir.c
@@ -1236,18 +1236,22 @@ static struct dir_entry *dir_entry_new(const char 
*pathname, int len)
return ent;
 }
 
-static struct dir_entry *dir_add_name(struct dir_struct *dir, const char 
*pathname, int len)
+static struct dir_entry *dir_add_name(struct dir_struct *dir,
+ struct index_state *istate,
+ const char *pathname, int len)
 {
-   if (index_file_exists(_index, pathname, len, ignore_case))
+   if (index_file_exists(istate, pathname, len, ignore_case))
return NULL;
 
ALLOC_GROW(dir->entries, dir->nr+1, dir->alloc);
return dir->entries[dir->nr++] = dir_entry_new(pathname, len);
 }
 
-struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char 
*pathname, int len)
+struct dir_entry *dir_add_ignored(struct dir_struct *dir,
+ struct index_state *istate,
+ const char *pathname, int len)
 {
-   if (!index_name_is_other(_index, pathname, len))
+   if (!index_name_is_other(istate, pathname, len))
return NULL;
 
ALLOC_GROW(dir->ignored, dir->ignored_nr+1, dir->ignored_alloc);
@@ -1819,18 +1823,18 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
switch (state) {
case path_excluded:
if (dir->flags & DIR_SHOW_IGNORED)
-   dir_add_name(dir, path.buf, path.len);
+   dir_add_name(dir, _index, path.buf, 
path.len);
else if ((dir->flags & DIR_SHOW_IGNORED_TOO) ||
((dir->flags & DIR_COLLECT_IGNORED) &&
exclude_matches_pathspec(path.buf, path.len,
 pathspec)))
-   dir_add_ignored(dir, path.buf, path.len);
+   dir_add_ignored(dir, _index, path.buf, 
path.len);
break;
 
case path_untracked:
if (dir->flags & DIR_SHOW_IGNORED)
break;
-   dir_add_name(dir, path.buf, path.len);
+   dir_add_name(dir, _index, path.buf, path.len);
if (cdir.fdir)
add_untracked(untracked, path.buf + baselen);
break;
diff --git a/dir.h b/dir.h
index bf23a470a..a9f809983 100644
--- a/dir.h
+++ b/dir.h
@@ -219,7 +219,9 @@ extern int read_directory(struct dir_struct *, const char 
*path, int len, const
 
 extern int is_excluded_from_list(const char *pathname, int pathlen, const char 
*basename,
 int *dtype, struct exclude_list *el);
-struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char 
*pathname, int len);
+struct dir_entry *dir_add_ignored(struct dir_struct *dir,
+ struct index_state *istate,
+ const char *pathname, int len);
 
 /*
  * these implement the matching logic for dir.c:excluded_from_list and
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 06/14] dir: convert last_exclude_matching_from_list to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index a508e8076..077b756c2 100644
--- a/dir.c
+++ b/dir.c
@@ -961,7 +961,8 @@ static struct exclude 
*last_exclude_matching_from_list(const char *pathname,
   int pathlen,
   const char *basename,
   int *dtype,
-  struct exclude_list *el)
+  struct exclude_list *el,
+  struct index_state 
*istate)
 {
struct exclude *exc = NULL; /* undecided */
int i;
@@ -976,7 +977,7 @@ static struct exclude 
*last_exclude_matching_from_list(const char *pathname,
 
if (x->flags & EXC_FLAG_MUSTBEDIR) {
if (*dtype == DT_UNKNOWN)
-   *dtype = get_dtype(NULL, _index, pathname, 
pathlen);
+   *dtype = get_dtype(NULL, istate, pathname, 
pathlen);
if (*dtype != DT_DIR)
continue;
}
@@ -1012,13 +1013,14 @@ int is_excluded_from_list(const char *pathname,
  struct exclude_list *el)
 {
struct exclude *exclude;
-   exclude = last_exclude_matching_from_list(pathname, pathlen, basename, 
dtype, el);
+   exclude = last_exclude_matching_from_list(pathname, pathlen, basename, 
dtype, el, _index);
if (exclude)
return exclude->flags & EXC_FLAG_NEGATIVE ? 0 : 1;
return -1; /* undecided */
 }
 
 static struct exclude *last_exclude_matching_from_lists(struct dir_struct *dir,
+   struct index_state 
*istate,
const char *pathname, int pathlen, const char *basename,
int *dtype_p)
 {
@@ -1030,7 +1032,7 @@ static struct exclude 
*last_exclude_matching_from_lists(struct dir_struct *dir,
for (j = group->nr - 1; j >= 0; j--) {
exclude = last_exclude_matching_from_list(
pathname, pathlen, basename, dtype_p,
-   >el[j]);
+   >el[j], istate);
if (exclude)
return exclude;
}
@@ -1121,6 +1123,7 @@ static void prep_exclude(struct dir_struct *dir, const 
char *base, int baselen)
int dt = DT_DIR;
dir->basebuf.buf[stk->baselen - 1] = 0;
dir->exclude = last_exclude_matching_from_lists(dir,
+   
_index,
dir->basebuf.buf, stk->baselen - 1,
dir->basebuf.buf + current, );
dir->basebuf.buf[stk->baselen - 1] = '/';
@@ -1209,7 +1212,7 @@ struct exclude *last_exclude_matching(struct dir_struct 
*dir,
if (dir->exclude)
return dir->exclude;
 
-   return last_exclude_matching_from_lists(dir, pathname, pathlen,
+   return last_exclude_matching_from_lists(dir, _index, pathname, 
pathlen,
basename, dtype_p);
 }
 
-- 
2.13.0.rc1.294.g07d810a77f-goog

[RFC 12/14] dir: convert read_directory_recursive to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 dir.c | 52 +---
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/dir.c b/dir.c
index f08d30ee4..3318ebbcb 100644
--- a/dir.c
+++ b/dir.c
@@ -46,7 +46,8 @@ struct cached_dir {
 };
 
 static enum path_treatment read_directory_recursive(struct dir_struct *dir,
-   const char *path, int len, struct untracked_cache_dir *untracked,
+   struct index_state *istate, const char *path, int len,
+   struct untracked_cache_dir *untracked,
int check_only, const struct pathspec *pathspec);
 static int get_dtype(struct dirent *de, struct index_state *istate,
 const char *path, int len);
@@ -1362,12 +1363,13 @@ static enum exist_status 
directory_exists_in_index(struct index_state *istate,
  *  (c) otherwise, we recurse into it.
  */
 static enum path_treatment treat_directory(struct dir_struct *dir,
+   struct index_state *istate,
struct untracked_cache_dir *untracked,
const char *dirname, int len, int baselen, int exclude,
const struct pathspec *pathspec)
 {
/* The "len-1" is to strip the final '/' */
-   switch (directory_exists_in_index(_index, dirname, len-1)) {
+   switch (directory_exists_in_index(istate, dirname, len-1)) {
case index_directory:
return path_recurse;
 
@@ -1392,7 +1394,7 @@ static enum path_treatment treat_directory(struct 
dir_struct *dir,
 
untracked = lookup_untracked(dir->untracked, untracked,
 dirname + baselen, len - baselen);
-   return read_directory_recursive(dir, dirname, len,
+   return read_directory_recursive(dir, istate, dirname, len,
untracked, 1, pathspec);
 }
 
@@ -1536,16 +1538,17 @@ static int get_dtype(struct dirent *de, struct 
index_state *istate,
 
 static enum path_treatment treat_one_path(struct dir_struct *dir,
  struct untracked_cache_dir *untracked,
+ struct index_state *istate,
  struct strbuf *path,
  int baselen,
  const struct pathspec *pathspec,
  int dtype, struct dirent *de)
 {
int exclude;
-   int has_path_in_index = !!index_file_exists(_index, path->buf, 
path->len, ignore_case);
+   int has_path_in_index = !!index_file_exists(istate, path->buf, 
path->len, ignore_case);
 
if (dtype == DT_UNKNOWN)
-   dtype = get_dtype(de, _index, path->buf, path->len);
+   dtype = get_dtype(de, istate, path->buf, path->len);
 
/* Always exclude indexed files */
if (dtype != DT_DIR && has_path_in_index)
@@ -1572,10 +1575,10 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
if ((dir->flags & DIR_COLLECT_KILLED_ONLY) &&
(dtype == DT_DIR) &&
!has_path_in_index &&
-   (directory_exists_in_index(_index, path->buf, path->len) == 
index_nonexistent))
+   (directory_exists_in_index(istate, path->buf, path->len) == 
index_nonexistent))
return path_none;
 
-   exclude = is_excluded(dir, _index, path->buf, );
+   exclude = is_excluded(dir, istate, path->buf, );
 
/*
 * Excluded? If we don't explicitly want to show
@@ -1589,7 +1592,7 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
return path_none;
case DT_DIR:
strbuf_addch(path, '/');
-   return treat_directory(dir, untracked, path->buf, path->len,
+   return treat_directory(dir, istate, untracked, path->buf, 
path->len,
   baselen, exclude, pathspec);
case DT_REG:
case DT_LNK:
@@ -1600,6 +1603,7 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
 static enum path_treatment treat_path_fast(struct dir_struct *dir,
   struct untracked_cache_dir 
*untracked,
   struct cached_dir *cdir,
+  struct index_state *istate,
   struct strbuf *path,
   int baselen,
   const struct pathspec *pathspec)
@@ -1618,7 +1622,7 @@ static enum path_treatment treat_path_fast(struct 
dir_struct *dir,
 * to its bottom. Verify again the same set of directories
 * with check_only set.
 */
-   return read_directory_recursive(dir, path->buf, path->len,
+   return read_directory_recursive(dir, istate, path->buf, 
path->len,
cdir->ucd, 1, 

[RFC 01/14] dir: stop using the index compatibility macros

[Brandon Williams]

In order to make it clearer where the_index is being referenced, stop
using the index compatibility macros in dir.c.  This is to make it
easier to identify the functions which need to be convert to taking in a
'struct index_state' as a parameter.

The end goal would be to eliminate the need to reference global index
state in dir.c.

Signed-off-by: Brandon Williams 
---
 dir.c | 41 +
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/dir.c b/dir.c
index f451bfa48..97eadd45e 100644
--- a/dir.c
+++ b/dir.c
@@ -7,6 +7,7 @@
  * Copyright (C) Linus Torvalds, 2005-2006
  *  Junio Hamano, 2005-2006
  */
+#define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "dir.h"
 #include "attr.h"
@@ -596,12 +597,12 @@ static void *read_skip_worktree_file_from_index(const 
char *path, size_t *size,
void *data;
 
len = strlen(path);
-   pos = cache_name_pos(path, len);
+   pos = index_name_pos(_index, path, len);
if (pos < 0)
return NULL;
-   if (!ce_skip_worktree(active_cache[pos]))
+   if (!ce_skip_worktree(the_index.cache[pos]))
return NULL;
-   data = read_sha1_file(active_cache[pos]->oid.hash, , );
+   data = read_sha1_file(the_index.cache[pos]->oid.hash, , );
if (!data || type != OBJ_BLOB) {
free(data);
return NULL;
@@ -609,7 +610,7 @@ static void *read_skip_worktree_file_from_index(const char 
*path, size_t *size,
*size = xsize_t(sz);
if (sha1_stat) {
memset(_stat->stat, 0, sizeof(sha1_stat->stat));
-   hashcpy(sha1_stat->sha1, active_cache[pos]->oid.hash);
+   hashcpy(sha1_stat->sha1, the_index.cache[pos]->oid.hash);
}
return data;
 }
@@ -785,12 +786,12 @@ static int add_excludes(const char *fname, const char 
*base, int baselen,
!match_stat_data_racy(_index, _stat->stat, 
))
; /* no content change, ss->sha1 still good */
else if (check_index &&
-(pos = cache_name_pos(fname, strlen(fname))) 
>= 0 &&
-!ce_stage(active_cache[pos]) &&
-ce_uptodate(active_cache[pos]) &&
+(pos = index_name_pos(_index, fname, 
strlen(fname))) >= 0 &&
+!ce_stage(the_index.cache[pos]) &&
+ce_uptodate(the_index.cache[pos]) &&
 !would_convert_to_git(fname))
hashcpy(sha1_stat->sha1,
-   active_cache[pos]->oid.hash);
+   the_index.cache[pos]->oid.hash);
else
hash_sha1_file(buf, size, "blob", 
sha1_stat->sha1);
fill_stat_data(_stat->stat, );
@@ -1235,7 +1236,7 @@ static struct dir_entry *dir_entry_new(const char 
*pathname, int len)
 
 static struct dir_entry *dir_add_name(struct dir_struct *dir, const char 
*pathname, int len)
 {
-   if (cache_file_exists(pathname, len, ignore_case))
+   if (index_file_exists(_index, pathname, len, ignore_case))
return NULL;
 
ALLOC_GROW(dir->entries, dir->nr+1, dir->alloc);
@@ -1244,7 +1245,7 @@ static struct dir_entry *dir_add_name(struct dir_struct 
*dir, const char *pathna
 
 struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char 
*pathname, int len)
 {
-   if (!cache_name_is_other(pathname, len))
+   if (!index_name_is_other(_index, pathname, len))
return NULL;
 
ALLOC_GROW(dir->ignored, dir->ignored_nr+1, dir->ignored_alloc);
@@ -1266,10 +1267,10 @@ static enum exist_status 
directory_exists_in_index_icase(const char *dirname, in
 {
struct cache_entry *ce;
 
-   if (cache_dir_exists(dirname, len))
+   if (index_dir_exists(_index, dirname, len))
return index_directory;
 
-   ce = cache_file_exists(dirname, len, ignore_case);
+   ce = index_file_exists(_index, dirname, len, ignore_case);
if (ce && S_ISGITLINK(ce->ce_mode))
return index_gitdir;
 
@@ -1290,11 +1291,11 @@ static enum exist_status 
directory_exists_in_index(const char *dirname, int len)
if (ignore_case)
return directory_exists_in_index_icase(dirname, len);
 
-   pos = cache_name_pos(dirname, len);
+   pos = index_name_pos(_index, dirname, len);
if (pos < 0)
pos = -pos-1;
-   while (pos < active_nr) {
-   const struct cache_entry *ce = active_cache[pos++];
+   while (pos < the_index.cache_nr) {
+   const struct cache_entry *ce = the_index.cache[pos++];
unsigned char endchar;
 
if (strncmp(ce->name, dirname, len))
@@ 

Re: [PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Ævar Arnfjörð Bjarmason]

On Fri, May 5, 2017 at 8:43 PM, Daniel Ferreira  wrote:
> This series introduces git-add-interactive--helper (or should it be
> called git-add--interactive--helper?) as a builtin capable of doing
> what the Perl script's status_cmd() would do.

The existing script is git-add--interactive.perl, so
git-add--interactive--helper.c would be consistent with that, since
there's no git-add-interactive command.

Re: [PATCH v1 1/2] travis-ci: setup "prove cache" in "script" step

[Ævar Arnfjörð Bjarmason]

On Fri, May 5, 2017 at 5:40 PM, Lars Schneider  wrote:
> The command that made the "prove cache" persistent across builds was
> executed in the "before_install" step. Consequently, every job that
> wanted to make use of the cache had to run this step.
>
> The "prove cache" is only used in the "script" step for the
> "make test" command. Therefore, we should configure the "prove cache"
> in this step.
>
> This change is useful for a subsequent patch that adds a job which does
> not need the "before_install" step but wants to run the "script" step to
> execute the tests.
>
> Signed-off-by: Lars Schneider 
> ---
>  .travis.yml | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index 48cb00a581..aa03f8eb82 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -135,12 +135,14 @@ before_install:
>  p4 -V | grep Rev.;
>  echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)";
>  git-lfs version;
> -mkdir -p $HOME/travis-cache;
> -ln -s $HOME/travis-cache/.prove t/.prove;
>
>  before_script: make --jobs=2
>
> -script: make --quiet test
> +script:
> +  - >
> +mkdir -p $HOME/travis-cache;
> +ln -s $HOME/travis-cache/.prove t/.prove;
> +make --quiet test;
>
>  after_failure:
>- >

This reminded me to submit a patch to prove itself to allow for
customizing the location of the .prove file:
https://github.com/Perl-Toolchain-Gang/Test-Harness/pull/66

Hopefully in the not too distant future we can then do away with this
minor hack. Doesn't matter much to us, but just inflexible that such
symlink hacks are needed.

[PATCH 0/3] Port git-add--interactive.perl:status_cmd to C

[Daniel Ferreira]

Hm, it looks like my GSoC project won't be in the Git organization,
although I'm still interested in going for this so I guess I'll send
patches to implement my proposal anyway (although certainly in a slower
pace than I would if on the program).

This series introduces git-add-interactive--helper (or should it be
called git-add--interactive--helper?) as a builtin capable of doing
what the Perl script's status_cmd() would do.

I wish this patch series could have been smaller, although I don't
think it would make sense to bring add-interactive "subunits" smaller
than a command to the helper, and status_cmd (aside from probably
add_untracked_cmd) was the simplest one to do after all -- and still
required ~250 lines on the new builtin.

Another regret I had was not being able to retire any code from the
Perl script yet (and will likely not be able to do so until all
commands have been ported), but that is not such a big thing after
all.

As for the new builtin, I think the color handling code is pretty
straightforward. In fact, it was practically copied from places like
clean.c or diff.c (which makes me wonder if some of that code could
be generalized to avoid duplication). The same goes for the pretty
simple option parsing code.

Bigger issues seem to arise when dealing with getting the numstat.
While (as Junio anticipated on an RFC) some tasks like getting the
actual diff and splitting it may require making the "diff
machinery" write to a temporary file that we will read and do things
with, I think it would be weird to do that for parsing a simple
numstat from it. My first instinct was to create something like
show_numstat_interactive() or something on diff.c (analogous to the
other show_* functions). Doing that, however, would stumble upon
another issue: we would not be able to print both a file's diff
against the index (obtained from run_diff_index) and against the
worktree (obtained from run_diff_files) in that function. The solution
I came up with was to export the diffstat interface from diff.c into
the world and allow our new builtin to use that and build our status_cmd
output. Unless this breaks some rule of Git's API design, the result
seems pretty reasonable to me.

Travis CI build: https://travis-ci.org/theiostream/git/builds/229232202

Looking forward for feedback,
Daniel.

Daniel Ferreira (3):
  diff: export diffstat interface
  add--interactive: add builtin helper for interactive add
  add--interactive: use add-interactive--helper for status_cmd

 .gitignore|   1 +
 Makefile  |   1 +
 builtin.h |   1 +
 builtin/add-interactive--helper.c | 258 ++
 diff.c|  17 +--
 diff.h|  19 ++-
 git-add--interactive.perl |   4 +-
 git.c |   1 +
 8 files changed, 282 insertions(+), 20 deletions(-)
 create mode 100644 builtin/add-interactive--helper.c

--
2.7.4 (Apple Git-66)

[PATCH 3/3] add--interactive: use add-interactive--helper for status_cmd

[Daniel Ferreira]

Call the newly introduced git-add-interactive--helper builtin on
status_cmd() instead of relying on git-add--interactive's Perl
functions to build print the numstat.

Signed-off-by: Daniel Ferreira 
---
 git-add--interactive.perl | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 709a5f6..8617462 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -632,9 +632,7 @@ EOF
 }
 
 sub status_cmd {
-   list_and_choose({ LIST_ONLY => 1, HEADER => $status_head },
-   list_modified());
-   print "\n";
+   system(qw(git add-interactive--helper --status));
 }
 
 sub say_n_paths {
-- 
2.7.4 (Apple Git-66)

[PATCH 2/3] add--interactive: add builtin helper for interactive add

[Daniel Ferreira]

Create a builtin helper for git-add--interactive, which right now is
only able to reproduce git-add--interactive.perl's status_cmd()
function, providing a summarized diff numstat to the user.

This is the first step in an effort to convert git-add--interactive.perl
to a C builtin, in search for better portability, expressibility and
performance (specially on non-POSIX systems like Windows).

Additionally, an eventual complete port of git-add--interactive would
remove the last "big" Git script to have Perl as a dependency, allowing
most Git users to have a NOPERL build running without big losses.

Signed-off-by: Daniel Ferreira 
---
 .gitignore|   1 +
 Makefile  |   1 +
 builtin.h |   1 +
 builtin/add-interactive--helper.c | 258 ++
 git.c |   1 +
 5 files changed, 262 insertions(+)
 create mode 100644 builtin/add-interactive--helper.c

diff --git a/.gitignore b/.gitignore
index 833ef3b..0d6cfe4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -11,6 +11,7 @@
 /git
 /git-add
 /git-add--interactive
+/git-add-interactive--helper
 /git-am
 /git-annotate
 /git-apply
diff --git a/Makefile b/Makefile
index e35542e..842fce2 100644
--- a/Makefile
+++ b/Makefile
@@ -873,6 +873,7 @@ LIB_OBJS += xdiff-interface.o
 LIB_OBJS += zlib.o
 
 BUILTIN_OBJS += builtin/add.o
+BUILTIN_OBJS += builtin/add-interactive--helper.o
 BUILTIN_OBJS += builtin/am.o
 BUILTIN_OBJS += builtin/annotate.o
 BUILTIN_OBJS += builtin/apply.o
diff --git a/builtin.h b/builtin.h
index 9e4a898..3d6a0ab 100644
--- a/builtin.h
+++ b/builtin.h
@@ -30,6 +30,7 @@ extern int textconv_object(const char *path, unsigned mode, 
const struct object_
 extern int is_builtin(const char *s);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
+extern int cmd_add_interactive__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_am(int argc, const char **argv, const char *prefix);
 extern int cmd_annotate(int argc, const char **argv, const char *prefix);
 extern int cmd_apply(int argc, const char **argv, const char *prefix);
diff --git a/builtin/add-interactive--helper.c 
b/builtin/add-interactive--helper.c
new file mode 100644
index 000..97ca1b3
--- /dev/null
+++ b/builtin/add-interactive--helper.c
@@ -0,0 +1,258 @@
+#include "builtin.h"
+#include "color.h"
+#include "diff.h"
+#include "diffcore.h"
+#include "revision.h"
+
+#define ADD_INTERACTIVE_HEADER_INDENT "  "
+
+enum add_interactive_collection_mode {
+   COLLECTION_MODE_NONE,
+   COLLECTION_MODE_WORKTREE,
+   COLLECTION_MODE_INDEX
+};
+
+struct add_interactive_file_status {
+   int selected;
+
+   char path[PATH_MAX];
+
+   int lines_added_index;
+   int lines_deleted_index;
+   int lines_added_worktree;
+   int lines_deleted_worktree;
+};
+
+struct add_interactive_status {
+   enum add_interactive_collection_mode current_mode;
+
+   const char *reference;
+   struct pathspec pathspec;
+
+   int file_count;
+   struct add_interactive_file_status *files;
+};
+
+static int add_interactive_use_color = -1;
+enum color_add_interactive {
+   ADD_INTERACTIVE_PROMPT,
+   ADD_INTERACTIVE_HEADER,
+   ADD_INTERACTIVE_HELP,
+   ADD_INTERACTIVE_ERROR
+};
+
+static char add_interactive_colors[][COLOR_MAXLEN] = {
+   GIT_COLOR_BOLD_BLUE, /* Prompt */
+   GIT_COLOR_BOLD,  /* Header */
+   GIT_COLOR_BOLD_RED,  /* Help */
+   GIT_COLOR_BOLD_RED   /* Error */
+};
+
+static const char *add_interactive_get_color(enum color_add_interactive ix)
+{
+   if (want_color(add_interactive_use_color))
+   return add_interactive_colors[ix];
+   return "";
+}
+
+static int parse_add_interactive_color_slot(const char *slot)
+{
+   if (!strcasecmp(slot, "prompt"))
+   return ADD_INTERACTIVE_PROMPT;
+   if (!strcasecmp(slot, "header"))
+   return ADD_INTERACTIVE_HEADER;
+   if (!strcasecmp(slot, "help"))
+   return ADD_INTERACTIVE_HELP;
+   if (!strcasecmp(slot, "error"))
+   return ADD_INTERACTIVE_ERROR;
+
+   return -1;
+}
+
+static int git_add_interactive_config(const char *var,
+   const char *value, void *cbdata)
+{
+   const char *name;
+
+   if (!strcmp(var, "color.interactive")) {
+   add_interactive_use_color = git_config_colorbool(var, value);
+   return 0;
+   }
+
+   if (skip_prefix(var, "color.interactive", )) {
+   int slot = parse_add_interactive_color_slot(name);
+   if (slot < 0)
+   return 0;
+   if (!value)
+   return config_error_nonbool(var);
+   return color_parse(value, add_interactive_colors[slot]);
+   }
+
+   return git_default_config(var, value, cbdata);
+}
+
+static void 

[PATCH 1/3] diff: export diffstat interface

[Daniel Ferreira]

Make the diffstat interface (namely, the diffstat_t struct and
diff_flush_stat) no longer be internal to diff.c and allow it to be used
by other parts of git.

This is helpful for code that may want to easily extract information
from files using the diff machinery, while flushing it differently from
how the show_* functions used by diff_flush() do it.

Signed-off-by: Daniel Ferreira 
---
 diff.c | 17 +
 diff.h | 19 ++-
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/diff.c b/diff.c
index 74283d9..7c073c2 100644
--- a/diff.c
+++ b/diff.c
@@ -1460,21 +1460,6 @@ static char *pprint_rename(const char *a, const char *b)
return strbuf_detach(, NULL);
 }
 
-struct diffstat_t {
-   int nr;
-   int alloc;
-   struct diffstat_file {
-   char *from_name;
-   char *name;
-   char *print_name;
-   unsigned is_unmerged:1;
-   unsigned is_binary:1;
-   unsigned is_renamed:1;
-   unsigned is_interesting:1;
-   uintmax_t added, deleted;
-   } **files;
-};
-
 static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat,
  const char *name_a,
  const char *name_b)
@@ -4310,7 +4295,7 @@ static void diff_flush_patch(struct diff_filepair *p, 
struct diff_options *o)
run_diff(p, o);
 }
 
-static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
+void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
struct diffstat_t *diffstat)
 {
if (diff_unmodified_pair(p))
diff --git a/diff.h b/diff.h
index 5be1ee7..4cdc72d 100644
--- a/diff.h
+++ b/diff.h
@@ -188,6 +188,21 @@ struct diff_options {
int diff_path_counter;
 };
 
+struct diffstat_t {
+   int nr;
+   int alloc;
+   struct diffstat_file {
+   char *from_name;
+   char *name;
+   char *print_name;
+   unsigned is_unmerged:1;
+   unsigned is_binary:1;
+   unsigned is_renamed:1;
+   unsigned is_interesting:1;
+   uintmax_t added, deleted;
+   } **files;
+};
+
 enum color_diff {
DIFF_RESET = 0,
DIFF_CONTEXT = 1,
@@ -206,7 +221,6 @@ const char *diff_get_color(int diff_use_color, enum 
color_diff ix);
 
 const char *diff_line_prefix(struct diff_options *);
 
-
 extern const char mime_boundary_leader[];
 
 extern struct combine_diff_path *diff_tree_paths(
@@ -262,6 +276,9 @@ extern void diff_change(struct diff_options *,
 
 extern struct diff_filepair *diff_unmerge(struct diff_options *, const char 
*path);
 
+void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
+   struct diffstat_t *diffstat);
+
 #define DIFF_SETUP_REVERSE 1
 #define DIFF_SETUP_USE_CACHE   2
 #define DIFF_SETUP_USE_SIZE_CACHE  4
-- 
2.7.4 (Apple Git-66)

[PATCH v2] tests: fix tests broken under GETTEXT_POISON=YesPlease

[Ævar Arnfjörð Bjarmason]

The GETTEXT_POISON=YesPlease compile-time testing option added in my
bb946bba76 ("i18n: add GETTEXT_POISON to simulate unfriendly
translator", 2011-02-22) has been slowly bitrotting as strings have
been marked for translation, and new tests have been added without
running it.

I brought this up on the list ("[BUG] test suite broken with
GETTEXT_POISON=YesPlease", [1]) asking whether this mode was useful at
all anymore. At least one person occasionally uses it, and Lars
Schneider offered to change one of the the Travis builds to run in
this mode, so fix up the failing ones.

My test setup runs most of the tests, with the notable exception of
skipping all the p4 tests, so it's possible that there's still some
lurking regressions I haven't fixed.

1. 

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

On Fri, May 5, 2017 at 5:40 PM, Lars Schneider  wrote:
> Hi,
>
> this adds GETTEXT_POISON tests to TravisCI. Patch 1/2 is preparation
> and 2/2 adds the build job.
>
> You can see a test run here:
> https://travis-ci.org/larsxschneider/git/jobs/229120495
>
> On "next" this generates a bunch of failures (see below).
>
> @Ævar: Are your GETTEXT_POISON fixes in already or are these failures 
> expected?

My v1 in <20170421185757.28978-1-ava...@gmail.com> didn't get picked
up, so these are all known failures.

In addition the t7508-status.sh test was broken on pu. I've just based
this version of the patch on pu, it's identical on master except for
the addition of the t7508-status.sh fix, meaning if this lands on pu
GETTEXT_POISON runs will be clean.

Junio: I think between Travis now testing for this & the scary i18n
reflog regression (not that poison caught that, but that was lack of
testing, poisoining catches that class of issue) it makes sense to
discard my patch for removing GETTEXT_POISON & queue this up instead.

 t/t1309-early-config.sh  |  2 +-
 t/t1430-bad-ref-name.sh  |  2 +-
 t/t3203-branch-output.sh |  2 +-
 t/t3404-rebase-interactive.sh| 14 +++---
 t/t3415-rebase-autosquash.sh | 10 +-
 t/t3903-stash.sh |  4 ++--
 t/t4205-log-pretty-formats.sh|  4 ++--
 t/t5316-pack-delta-depth.sh  |  8 ++--
 t/t6134-pathspec-in-submodule.sh |  4 ++--
 t/t7004-tag.sh   |  4 ++--
 t/t7406-submodule-update.sh  |  2 +-
 t/t7508-status.sh|  6 +++---
 t/t7509-commit.sh|  4 ++--
 t/t7800-difftool.sh  |  4 ++--
 14 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
index 1af8c454cf..3dda215e8e 100755
--- a/t/t1309-early-config.sh
+++ b/t/t1309-early-config.sh
@@ -77,7 +77,7 @@ test_with_config () {
 
 test_expect_success 'ignore .git/ with incompatible repository version' '
test_with_config "[core]repositoryformatversion = 99" 2>err &&
-   grep "warning:.* Expected git repo version <= [1-9]" err
+   test_i18ngrep "warning:.* Expected git repo version <= [1-9]" err
 '
 
 test_expect_failure 'ignore .git/ with invalid repository version' '
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 8937e25e49..e88349c8a0 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -122,7 +122,7 @@ test_expect_success 'push cannot create a badly named ref' '
! grep -e "broken\.\.\.ref" output
 '
 
-test_expect_failure 'push --mirror can delete badly named ref' '
+test_expect_failure C_LOCALE_OUTPUT 'push --mirror can delete badly named ref' 
'
top=$(pwd) &&
git init src &&
git init dest &&
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 5778c0afe1..a428ae6703 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -236,7 +236,7 @@ test_expect_success 'git branch --format option' '
Refname is refs/heads/ref-to-remote
EOF
git branch --format="Refname is %(refname)" >actual &&
-   test_cmp expect actual
+   test_i18ncmp expect actual
 '
 
 test_done
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 61113be08a..3b411ea8f1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -366,7 +366,7 @@ test_expect_success 'verbose flag is heeded, even after 
--continue' '
grep "^ file1 | 2 +-$" output
 '
 
-test_expect_success 'multi-squash only fires up editor once' '
+test_expect_success C_LOCALE_OUTPUT 'multi-squash only fires up editor once' '
base=$(git rev-parse HEAD~4) &&
set_fake_editor &&
FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 squash 2 squash 3 squash 4" \
@@ -376,7 +376,7 @@ test_expect_success 'multi-squash only fires up editor 
once' '
test 1 = $(git show | grep ONCE | wc -l)
 '
 
-test_expect_success 'multi-fixup does not fire up editor' '
+test_expect_success C_LOCALE_OUTPUT 'multi-fixup 

Re: [RFC PATCH 00/10] Add blame to libgit

[Junio C Hamano]

Jeff Smith  writes:

> Jeff Smith (10):
>   Remove unneeded dependency on blob.h from blame
>   Move textconv_object to be with other textconv methods
>   Add some missing definitions to header files
>   Remove unused parameter from get_origin()
>   Split blame origin into its own file
>   Move fake_working_tree_commit() to lib
>   Break out scoreboard a little better
>   Split blame scoreboard into its own file
>   Break out scoreboard init and setup
>   Move scoreboard init and setup to lib

Make sure these shortlog entries, when eventually appear along with
other people's changes, let readers know what these are about
easily, and formatted similarly to others'.  Write the ": "
prefix in front is a good pratice to help that, e.g.

blame: remove unneeded dependency on blob.h

In general I am OK with the idea of splitting builtin/blame.c into
the blame Porcelain part (i.e. cmd_blame()) and some form of
reusable "blame library".

However, you'd need to be cafreful about names.  Are contents of
"commit-fake", "origin" and "scoreboard" generally useful outside
the context of running a "blame"?  They are not good filenames if
saying "origin.c" in the context of the entire Git codebase, anybody
would understand that it is about "the origin" concept used by the
blame engine (similarly for other new files).  I would imagine that
having blame.[ch] at the toplevel instead of creating that many
"blame library" files would be a good way to go (mirrors how other
parts of the system is laid out, e.g. diff.[ch] and diff-*.[ch] are
library-ish files, while builtin/diff*.c implements the Porcelain).

Have you renamed the names of structures, functions and variables
that used to be private to builtin/blame.c and now shared between
builtin/blame.c and these new files so that they are specific enough
and are obvious that they are about blame infrastructure?  Names
that were perfectly acceptable and appropriate as private ones
inside a subsystem are often too vague or generic when exposed to
outside world as part of the library-ish code.

Re: [PATCH] t7406: fix i18n expectation of error message

[Stefan Beller]

On Fri, May 5, 2017 at 10:50 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> I have a fix for this in my gettext fixup series (so far lingering on
> the list, not in pu):
> https://public-inbox.org/git/20170421185757.28978-1-ava...@gmail.com/

Oh, should have checked the list more closely before.

>
> The diff is quite a bit smaller than yours:

I agree that this is the way to fix it best.

Sorry for the noise,
Stefan

Re: [RFC PATCH 07/10] Break out scoreboard a little better

[Junio C Hamano]

Jeff Smith  writes:

This blank space is for you to explain what you mean by "a little
better".  What makes the result better?

It seems to do too many things in a single patch.  For example, I
guess it may be a good idea that you made various knobs like
"show_root" encapsulated in the scoreboard; that change can and
should be on its own single step.  It is unclear what the pupose of
blame-sort-final thing is, but even if that change were a good idea
(I am not saying I think it is a bad idea; I am saying I do not see
sufficient explanation to judge its goodness), it is a change that
is unrelated to moving "show_root" and friends into the scoreboard,
hence should be a separate patch.

Re: [RFC PATCH 02/10] Move textconv_object to be with other textconv methods

[Stefan Beller]

On Fri, May 5, 2017 at 10:44 AM, Junio C Hamano  wrote:
> Jeff Smith  writes:
>
>> Signed-off-by: Jeff Smith 
>> ---
>>  builtin.h  |  2 --
>>  builtin/blame.c| 28 
>>  builtin/cat-file.c |  1 +
>>  diff.c | 23 +++
>>  diff.h |  7 +++
>>  5 files changed, 31 insertions(+), 30 deletions(-)
>
> This change makes sense regardless of your primary goal of the
> series.  It was not good that one builtin borrowing a helper from
> another.  The common helper should live outside builtin/ as a common
> code, and in this case, diff.[ch] may be an OK place for it.
>

My kneejerk reaction to this is would be:
Please don't grow diff.[ch] even more. It has
5k lines which is a lot IMHO. Although it might be
ok for the compiler and from a logical point of view,
I'd rather prefer to deal with lots of small files, than
with such large files. I am undecided if this hints at
bad tooling on my side or at how I think of code
and their location.

I guess it is ok for now and in this series, but we may want
to split up diff.[ch] in the future into multiple finer grained files.

Thanks,
Stefan

Re: [PATCH] t7406: fix i18n expectation of error message

[Ævar Arnfjörð Bjarmason]

On Fri, May 5, 2017 at 7:38 PM, Stefan Beller  wrote:
> The error message from "submodule update" is internationalized, which
> makes sense. The test however did not check for the translated version,
> but used a hardcoded string, which breaks the test when run with
> GETTEXT_POISON.
>
> Signed-off-by: Stefan Beller 
> ---
>  t/t7406-submodule-update.sh | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 4ac386d98b..12f6435ab0 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -441,13 +441,11 @@ test_expect_success 'submodule update - command in 
> .git/config catches failure -
> test_i18ncmp actual expect
>  '
>
> +sq="'"
>  test_expect_success 'submodule update - command run for initial population 
> of submodule' '
> -   cat >expect <<-EOF &&
> -   Execution of '\''false $submodulesha1'\'' failed in submodule path 
> '\''submodule'\''
> -   EOF
> rm -rf super/submodule &&
> test_must_fail git -C super submodule update 2>actual &&
> -   test_cmp expect actual &&
> +   test_i18ngrep "Execution of ${sq}false $submodulesha1${sq} failed in 
> submodule path ${sq}submodule${sq}" actual &&
> git -C super submodule update --checkout
>  '

I have a fix for this in my gettext fixup series (so far lingering on
the list, not in pu):
https://public-inbox.org/git/20170421185757.28978-1-ava...@gmail.com/

The diff is quite a bit smaller than yours:

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 4ac386d98b..034914a14f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -447,7 +447,7 @@ test_expect_success 'submodule update - command
run for initial population of su
EOF
rm -rf super/submodule &&
test_must_fail git -C super submodule update 2>actual &&
-   test_cmp expect actual &&
+   test_i18ncmp expect actual &&
git -C super submodule update --checkout
 '

Do you prefer to use i18ngrep for whatever reason? Seems better to use
i18ncmp there.

Re: [RFC PATCH 02/10] Move textconv_object to be with other textconv methods

[Junio C Hamano]

Jeff Smith  writes:

> Signed-off-by: Jeff Smith 
> ---
>  builtin.h  |  2 --
>  builtin/blame.c| 28 
>  builtin/cat-file.c |  1 +
>  diff.c | 23 +++
>  diff.h |  7 +++
>  5 files changed, 31 insertions(+), 30 deletions(-)

This change makes sense regardless of your primary goal of the
series.  It was not good that one builtin borrowing a helper from
another.  The common helper should live outside builtin/ as a common
code, and in this case, diff.[ch] may be an OK place for it.


>
> diff --git a/builtin.h b/builtin.h
> index 9e4a898..498ac80 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -25,8 +25,6 @@ struct fmt_merge_msg_opts {
>  extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
>struct fmt_merge_msg_opts *);
>  
> -extern int textconv_object(const char *path, unsigned mode, const struct 
> object_id *oid, int oid_valid, char **buf, unsigned long *buf_size);
> -
>  extern int is_builtin(const char *s);
>  
>  extern int cmd_add(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 42c56eb..c419981 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -147,34 +147,6 @@ static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b,
>  }
>  
>  /*
> - * Prepare diff_filespec and convert it using diff textconv API
> - * if the textconv driver exists.
> - * Return 1 if the conversion succeeds, 0 otherwise.
> - */
> -int textconv_object(const char *path,
> - unsigned mode,
> - const struct object_id *oid,
> - int oid_valid,
> - char **buf,
> - unsigned long *buf_size)
> -{
> - struct diff_filespec *df;
> - struct userdiff_driver *textconv;
> -
> - df = alloc_filespec(path);
> - fill_filespec(df, oid->hash, oid_valid, mode);
> - textconv = get_textconv(df);
> - if (!textconv) {
> - free_filespec(df);
> - return 0;
> - }
> -
> - *buf_size = fill_textconv(textconv, df, buf);
> - free_filespec(df);
> - return 1;
> -}
> -
> -/*
>   * Given an origin, prepare mmfile_t structure to be used by the
>   * diff machinery
>   */
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 1890d7a..79a2c82 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -5,6 +5,7 @@
>   */
>  #include "cache.h"
>  #include "builtin.h"
> +#include "diff.h"
>  #include "parse-options.h"
>  #include "userdiff.h"
>  #include "streaming.h"
> diff --git a/diff.c b/diff.c
> index 11eef1c..a62e989 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5270,6 +5270,29 @@ size_t fill_textconv(struct userdiff_driver *driver,
>   return size;
>  }
>  
> +int textconv_object(const char *path,
> + unsigned mode,
> + const struct object_id *oid,
> + int oid_valid,
> + char **buf,
> + unsigned long *buf_size)
> +{
> + struct diff_filespec *df;
> + struct userdiff_driver *textconv;
> +
> + df = alloc_filespec(path);
> + fill_filespec(df, oid->hash, oid_valid, mode);
> + textconv = get_textconv(df);
> + if (!textconv) {
> + free_filespec(df);
> + return 0;
> + }
> +
> + *buf_size = fill_textconv(textconv, df, buf);
> + free_filespec(df);
> + return 1;
> +}
> +
>  void setup_diff_pager(struct diff_options *opt)
>  {
>   /*
> diff --git a/diff.h b/diff.h
> index 5be1ee7..52ebd54 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -385,6 +385,13 @@ extern size_t fill_textconv(struct userdiff_driver 
> *driver,
>   */
>  extern struct userdiff_driver *get_textconv(struct diff_filespec *one);
>  
> +/*
> + * Prepare diff_filespec and convert it using diff textconv API
> + * if the textconv driver exists.
> + * Return 1 if the conversion succeeds, 0 otherwise.
> + */
> +extern int textconv_object(const char *path, unsigned mode, const struct 
> object_id *oid, int oid_valid, char **buf, unsigned long *buf_size);
> +
>  extern int parse_rename_score(const char **cp_p);
>  
>  extern long parse_algorithm_value(const char *value);

Re: [RFC PATCH 03/10] Add some missing definitions to header files

[Junio C Hamano]

Jeff Smith  writes:

> Signed-off-by: Jeff Smith 
> ---
>  object.h| 2 ++
>  pathspec.h  | 4 
>  refs.h  | 3 +++
>  tree-walk.h | 2 ++
>  4 files changed, 11 insertions(+)

As the coding rule of this codebase is not to include system headers
in anything other than git-compat-util.h which should be the first
thing to be included in the *.c files (cache.h and other selected
and well-known header files can be the first one in place of
git-compat-util, as they include it as the first thing), all changes
in this patch, except possibly the one to tree-walk.h, are wrong.

>
> diff --git a/object.h b/object.h
> index f52957d..9737582 100644
> --- a/object.h
> +++ b/object.h
> @@ -1,6 +1,8 @@
>  #ifndef OBJECT_H
>  #define OBJECT_H
>  
> +#include "cache.h"
> +
>  struct object_list {
>   struct object *item;
>   struct object_list *next;
> diff --git a/pathspec.h b/pathspec.h
> index 55e9769..ea18e24 100644
> --- a/pathspec.h
> +++ b/pathspec.h
> @@ -1,6 +1,10 @@
>  #ifndef PATHSPEC_H
>  #define PATHSPEC_H
>  
> +#include 
> +#include 
> +#include 
> +
>  /* Pathspec magic */
>  #define PATHSPEC_FROMTOP (1<<0)
>  #define PATHSPEC_MAXDEPTH(1<<1)
> diff --git a/refs.h b/refs.h
> index 07cf4cd..e9d19fd 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -1,6 +1,9 @@
>  #ifndef REFS_H
>  #define REFS_H
>  
> +#include 
> +#include 
> +
>  struct object_id;
>  struct ref_store;
>  struct strbuf;
> diff --git a/tree-walk.h b/tree-walk.h
> index 68bb78b..8b0fccd 100644
> --- a/tree-walk.h
> +++ b/tree-walk.h
> @@ -1,6 +1,8 @@
>  #ifndef TREE_WALK_H
>  #define TREE_WALK_H
>  
> +struct strbuf;
> +
>  struct name_entry {
>   const struct object_id *oid;
>   const char *path;

[PATCH] t7406: fix i18n expectation of error message

[Stefan Beller]

The error message from "submodule update" is internationalized, which
makes sense. The test however did not check for the translated version,
but used a hardcoded string, which breaks the test when run with
GETTEXT_POISON.

Signed-off-by: Stefan Beller 
---
 t/t7406-submodule-update.sh | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 4ac386d98b..12f6435ab0 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -441,13 +441,11 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
test_i18ncmp actual expect
 '
 
+sq="'"
 test_expect_success 'submodule update - command run for initial population of 
submodule' '
-   cat >expect <<-EOF &&
-   Execution of '\''false $submodulesha1'\'' failed in submodule path 
'\''submodule'\''
-   EOF
rm -rf super/submodule &&
test_must_fail git -C super submodule update 2>actual &&
-   test_cmp expect actual &&
+   test_i18ngrep "Execution of ${sq}false $submodulesha1${sq} failed in 
submodule path ${sq}submodule${sq}" actual &&
git -C super submodule update --checkout
 '
 
-- 
2.13.0.rc2.dirty

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Brandon Williams]

On 05/05, Junio C Hamano wrote:
> Jonathan Nieder  writes:
> 
> > That is, one way to do what this series attempts would be the
> > following:
> >
> >  1. rename variables that shadow the_index.
> 
> No question about this one.  It is a good thing to do.
> 
> >  2. add coccinelle patches (or one coccinelle patch) to
> > contrib/coccinelle implementing *_cache -> *_index migration.
> > Is there a way to do this without making it fail "make coccicheck"?
> 
> Quite honestly, I do not see much value in this, but take it merely
> as my knee-jerk reaction.  The only scenario I can think of in which
> dropping *_cache() macros is an improvement as the end result is
> when our goal is to completely drop the singleton index_state
> instance, aka "the_index".  I actually think that it may be a
> worthwhile goal to eradicate "the_index".
> 
> I wonder if somebody can take a small example codepath and make it
> not to rely on the existence of "the_index" from start to end?  Have
> an instance of index_state on the stack of cmd_foo(), have it call
> read_index() into it where it currently calls read_cache(), update
> the support functions it calls so that it can pass the pointer to
> its index_info throughout the callchain, and see how involved the
> necessary changes of all of the above are.  Start from something
> simple and small, e.g. "ls-files".  The infrastructure code updated
> for such an experiment may be NO_THE_INDEX_COMPATIBILITY_MACROS
> clean.

I've mentioned elsewhere but I've been working on this for 'ls-files'.
There's quite a few "gotchas" but its given me a good idea of which
sections of code need to be converted to taking in a
'struct index_state'.  I'll send some of this conversion out later today
as a RFC and see what people think about it and if its worth while to
continue.

-- 
Brandon Williams

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Junio C Hamano]

Jonathan Nieder  writes:

> That is, one way to do what this series attempts would be the
> following:
>
>  1. rename variables that shadow the_index.

No question about this one.  It is a good thing to do.

>  2. add coccinelle patches (or one coccinelle patch) to
> contrib/coccinelle implementing *_cache -> *_index migration.
> Is there a way to do this without making it fail "make coccicheck"?

Quite honestly, I do not see much value in this, but take it merely
as my knee-jerk reaction.  The only scenario I can think of in which
dropping *_cache() macros is an improvement as the end result is
when our goal is to completely drop the singleton index_state
instance, aka "the_index".  I actually think that it may be a
worthwhile goal to eradicate "the_index".

I wonder if somebody can take a small example codepath and make it
not to rely on the existence of "the_index" from start to end?  Have
an instance of index_state on the stack of cmd_foo(), have it call
read_index() into it where it currently calls read_cache(), update
the support functions it calls so that it can pass the pointer to
its index_info throughout the callchain, and see how involved the
necessary changes of all of the above are.  Start from something
simple and small, e.g. "ls-files".  The infrastructure code updated
for such an experiment may be NO_THE_INDEX_COMPATIBILITY_MACROS
clean.

Perhaps we can remove the existence of the_index from the system by
going that route; needless to say, when that goal is achieved, by
definition *_cache() macros will be completely useless and must be
removed, as there is nobody who relies on the existence of and who
uses "the_index".

>  3. migrate library code (but not builtins) using that semantic patch

I do think this is going backwards.  The only thinng replacing
*_cache() to *_index(_index) buys you is newbies not having to
know both, but instead they will be exposed to the pattern of
repeated use of *_index(_index); i.e. reliance of the existence
of the singleton "the_index" is not reduced, but is stressed which
is a big downside when we are trying to eventually get rid of it.

Also quite honestly, I do not think we want newbies to be touching
the things that needs *_index() interface while this update is going
on.

The remainder of your enumeration goes in a direction different from
getting rid of the_index, so I do not know---I wrote the above under
the assumption that the total removal of "the_index" might be a good
thing to do, and where that assumption would lead us to (e.g. an
obvious side-effect of no longer having the_index is that *_cache()
macros cannot exist), and I am undecided if the assumption is a good
one (yet).

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Brandon Williams]

On 05/05, Johannes Schindelin wrote:
> Hi Stefan & Junio,
> 
> On Thu, 4 May 2017, Stefan Beller wrote:
> 
> > So instead of a mechanical replacement, we'd rather want to
> > see "the_index" not appearing at all outside of builtins, which
> > implies two things:
> >
> > * If done properly we can move the macros from cache.h to
> >   e.g. builtin.h. That way future developers are less tempted
> >   to use the cache_* macros in the library code.
> 
> Ye!
> 
> > * we'd have to pass through the_index from the builtin function
> >   down to the library code, potentially going through multiple
> >   function. For this it is unclear if we want to start this now, or wait
> >   until Brandon presents his initial repository object struct, which
> >   may be suited better for passing-around.
> 
> Or the other way round. I guess passing a struct index_state can be a
> first step, and we can later convert it to struct repository. I fathom
> that more places will need a struct repository parameter than a struct
> index_state parameter. That is, if you first identify all the places where
> the index_state parameter is required, it should make the struct
> repository change easier.

Exactly this.  I have a local series which converts ls-files to use a
repository struct but it turns out, for that to work, dir.c needs to be
converted to take in an index_state struct for fill_directory().  So I
then started working on doing that conversion and hopefully will have
something clean enough to send out later today for people to comment on.

-- 
Brandon Williams

Re: [PATCH] l10n: de.po: translate 4 new messages

[Matthias Rüster]

Acked-by: Matthias Rüster 


Am 05.05.2017 um 18:17 schrieb Ralf Thielow:
> Translate 4 new messages came from git.pot update in 28e1aaa48 (l10n:
> git.pot: v2.13.0 round 2 (4 new, 7 removed)).
> 
> Signed-off-by: Ralf Thielow 
> ---
>  po/de.po | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/po/de.po b/po/de.po
> index 12c3d36a0..679f8f472 100644
> --- a/po/de.po
> +++ b/po/de.po
> @@ -2493,7 +2493,7 @@ msgstr ""
>  "Verwende Version %i"
>  
>  #: read-cache.c:2375 sequencer.c:1350 sequencer.c:2048
> -#, fuzzy, c-format
> +#, c-format
>  msgid "could not stat '%s'"
>  msgstr "Konnte '%s' nicht lesen."
>  
> @@ -2514,7 +2514,7 @@ msgstr "Konnte '%s' nicht zum Schreiben öffnen."
>  
>  #: refs.c:1667
>  msgid "ref updates forbidden inside quarantine environment"
> -msgstr ""
> +msgstr "Aktualisierungen von Referenzen ist innerhalb der 
> Quarantäne-Umgebung verboten."
>  
>  #: refs/files-backend.c:1631
>  #, c-format
> @@ -14135,9 +14135,8 @@ msgid "populate the new working tree"
>  msgstr "das neue Arbeitsverzeichnis auschecken"
>  
>  #: builtin/worktree.c:335
> -#, fuzzy
>  msgid "keep the new working tree locked"
> -msgstr "das neue Arbeitsverzeichnis auschecken"
> +msgstr "das neue Arbeitsverzeichnis gesperrt lassen"
>  
>  #: builtin/worktree.c:343
>  msgid "-b, -B, and --detach are mutually exclusive"
> @@ -14243,7 +14242,7 @@ msgstr ""
>  #: http.c:336
>  #, c-format
>  msgid "negative value for http.postbuffer; defaulting to %d"
> -msgstr ""
> +msgstr "negativer Wert für http.postbuffer; benutze Standardwert %d"
>  
>  #: http.c:357
>  msgid "Delegation control is not supported with cURL < 7.22.0"
> 

[PATCH] l10n: de.po: translate 4 new messages

[Ralf Thielow]

Translate 4 new messages came from git.pot update in 28e1aaa48 (l10n:
git.pot: v2.13.0 round 2 (4 new, 7 removed)).

Signed-off-by: Ralf Thielow 
---
 po/de.po | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/po/de.po b/po/de.po
index 12c3d36a0..679f8f472 100644
--- a/po/de.po
+++ b/po/de.po
@@ -2493,7 +2493,7 @@ msgstr ""
 "Verwende Version %i"
 
 #: read-cache.c:2375 sequencer.c:1350 sequencer.c:2048
-#, fuzzy, c-format
+#, c-format
 msgid "could not stat '%s'"
 msgstr "Konnte '%s' nicht lesen."
 
@@ -2514,7 +2514,7 @@ msgstr "Konnte '%s' nicht zum Schreiben öffnen."
 
 #: refs.c:1667
 msgid "ref updates forbidden inside quarantine environment"
-msgstr ""
+msgstr "Aktualisierungen von Referenzen ist innerhalb der Quarantäne-Umgebung 
verboten."
 
 #: refs/files-backend.c:1631
 #, c-format
@@ -14135,9 +14135,8 @@ msgid "populate the new working tree"
 msgstr "das neue Arbeitsverzeichnis auschecken"
 
 #: builtin/worktree.c:335
-#, fuzzy
 msgid "keep the new working tree locked"
-msgstr "das neue Arbeitsverzeichnis auschecken"
+msgstr "das neue Arbeitsverzeichnis gesperrt lassen"
 
 #: builtin/worktree.c:343
 msgid "-b, -B, and --detach are mutually exclusive"
@@ -14243,7 +14242,7 @@ msgstr ""
 #: http.c:336
 #, c-format
 msgid "negative value for http.postbuffer; defaulting to %d"
-msgstr ""
+msgstr "negativer Wert für http.postbuffer; benutze Standardwert %d"
 
 #: http.c:357
 msgid "Delegation control is not supported with cURL < 7.22.0"
-- 
2.13.0.rc0.207.gb44265493

Re: [PATCH v1 2/2] travis-ci: add job to run tests with GETTEXT_POISON

[Jonathan Nieder]

Hi,

Lars Schneider wrote:

> Add a job to run Git tests with GETTEXT_POISON. In this job we don't run
> the git-p4, git-svn, and HTTPD tests to save resources/time (those tests
> are already executed in other jobs). Since we don't run these tests, we
> can also skip the "before_install" step (which would install the
> necessary dependencies) with an empty override.
>
> Signed-off-by: Lars Schneider 
> ---
>  .travis.yml | 5 +
>  1 file changed, 5 insertions(+)

Yay!  I like this.

What I like most about GETTEXT_POISON is that it verifies that
translatable strings are not affecting other functionality of Git.
It's a valuable thing to test continuously.

For what it's worth,
Reviewed-by: Jonathan Nieder 

Thanks.

[PATCH v1 0/2] run GETTEXT_POISON on TravisCI

[Lars Schneider]

Hi,

this adds GETTEXT_POISON tests to TravisCI. Patch 1/2 is preparation
and 2/2 adds the build job.

You can see a test run here:
https://travis-ci.org/larsxschneider/git/jobs/229120495

On "next" this generates a bunch of failures (see below).

@Ævar: Are your GETTEXT_POISON fixes in already or are these failures expected?

Cheers,
Lars


t5316-pack-delta-depth.sh(Wstat: 256 Tests: 3 Failed: 2)
  Failed tests:  2-3
  Non-zero exit status: 1
t6134-pathspec-in-submodule.sh   (Wstat: 256 Tests: 3 Failed: 2)
  Failed tests:  2-3
  Non-zero exit status: 1
t3415-rebase-autosquash.sh   (Wstat: 256 Tests: 19 Failed: 
6)
  Failed tests:  13-17, 19
  Non-zero exit status: 1
t7509-commit.sh  (Wstat: 256 Tests: 12 Failed: 
2)
  Failed tests:  6-7
  Non-zero exit status: 1
t1309-early-config.sh(Wstat: 256 Tests: 8 Failed: 1)
  Failed test:  6
  Non-zero exit status: 1
t3203-branch-output.sh   (Wstat: 256 Tests: 23 Failed: 
1)
  Failed test:  23
  Non-zero exit status: 1
t7800-difftool.sh(Wstat: 256 Tests: 71 Failed: 
1)
  Failed test:  1
  Non-zero exit status: 1
t3404-rebase-interactive.sh  (Wstat: 256 Tests: 95 Failed: 
46)
  Failed tests:  26-38, 40-43, 45, 47-74
  Non-zero exit status: 1
t3903-stash.sh   (Wstat: 256 Tests: 75 Failed: 
2)
  Failed tests:  70-71
  Non-zero exit status: 1
t7406-submodule-update.sh(Wstat: 256 Tests: 52 Failed: 
1)
  Failed test:  23
  Non-zero exit status: 1
t7508-status.sh  (Wstat: 256 Tests: 102 Failed: 
3)
  Failed tests:  18-19, 43
  Non-zero exit status: 1


Base Ref: next
Web-Diff: https://github.com/larsxschneider/git/commit/a835cd4775
Checkout: git fetch https://github.com/larsxschneider/git travisci/poison-v1 && 
git checkout a835cd4775

Lars Schneider (2):
  travis-ci: setup "prove cache" in "script" step
  travis-ci: add job to run tests with GETTEXT_POISON

 .travis.yml | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)


base-commit: 813ba54fc35ef2b9c03fe84a4803e4365c22cf49
--
2.12.2

[PATCH v1 1/2] travis-ci: setup "prove cache" in "script" step

[Lars Schneider]

The command that made the "prove cache" persistent across builds was
executed in the "before_install" step. Consequently, every job that
wanted to make use of the cache had to run this step.

The "prove cache" is only used in the "script" step for the
"make test" command. Therefore, we should configure the "prove cache"
in this step.

This change is useful for a subsequent patch that adds a job which does
not need the "before_install" step but wants to run the "script" step to
execute the tests.

Signed-off-by: Lars Schneider 
---
 .travis.yml | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 48cb00a581..aa03f8eb82 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -135,12 +135,14 @@ before_install:
 p4 -V | grep Rev.;
 echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)";
 git-lfs version;
-mkdir -p $HOME/travis-cache;
-ln -s $HOME/travis-cache/.prove t/.prove;
 
 before_script: make --jobs=2
 
-script: make --quiet test
+script:
+  - >
+mkdir -p $HOME/travis-cache;
+ln -s $HOME/travis-cache/.prove t/.prove;
+make --quiet test;
 
 after_failure:
   - >
-- 
2.12.2

[PATCH v1 2/2] travis-ci: add job to run tests with GETTEXT_POISON

[Lars Schneider]

Add a job to run Git tests with GETTEXT_POISON. In this job we don't run
the git-p4, git-svn, and HTTPD tests to save resources/time (those tests
are already executed in other jobs). Since we don't run these tests, we
can also skip the "before_install" step (which would install the
necessary dependencies) with an empty override.

Signed-off-by: Lars Schneider 
---
 .travis.yml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index aa03f8eb82..278943d14a 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,6 +39,11 @@ env:
 
 matrix:
   include:
+- env: GETTEXT_POISON=YesPlease
+  os: linux
+  compiler:
+  addons:
+  before_install:
 - env: Windows
   os: linux
   compiler:
-- 
2.12.2

[PATCH v7 10/10] convert: Update subprocess_read_status to not die on EOF

[Ben Peart]

Enable sub-processes to gracefully handle when the process dies by
updating subprocess_read_status to return an error on EOF instead of
dying.

Update apply_multi_file_filter to take advantage of the revised
subprocess_read_status.

Signed-off-by: Ben Peart 
---
 convert.c | 10 --
 sub-process.c | 10 +++---
 sub-process.h |  2 +-
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/convert.c b/convert.c
index acf51416d1..85238e4976 100644
--- a/convert.c
+++ b/convert.c
@@ -635,7 +635,10 @@ static int apply_multi_file_filter(const char *path, const 
char *src, size_t len
if (err)
goto done;
 
-   subprocess_read_status(process->out, _status);
+   err = subprocess_read_status(process->out, _status);
+   if (err)
+   goto done;
+
err = strcmp(filter_status.buf, "success");
if (err)
goto done;
@@ -644,7 +647,10 @@ static int apply_multi_file_filter(const char *path, const 
char *src, size_t len
if (err)
goto done;
 
-   subprocess_read_status(process->out, _status);
+   err = subprocess_read_status(process->out, _status);
+   if (err)
+   goto done;
+
err = strcmp(filter_status.buf, "success");
 
 done:
diff --git a/sub-process.c b/sub-process.c
index 536b60cced..92f8aea70a 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -21,13 +21,15 @@ struct subprocess_entry *subprocess_find_entry(struct 
hashmap *hashmap, const ch
return hashmap_get(hashmap, , NULL);
 }
 
-void subprocess_read_status(int fd, struct strbuf *status)
+int subprocess_read_status(int fd, struct strbuf *status)
 {
struct strbuf **pair;
char *line;
+   int len;
+
for (;;) {
-   line = packet_read_line(fd, NULL);
-   if (!line)
+   len = packet_read_line_gently(fd, NULL, );
+   if ((len < 0) || !line)
break;
pair = strbuf_split_str(line, '=', 2);
if (pair[0] && pair[0]->len && pair[1]) {
@@ -39,6 +41,8 @@ void subprocess_read_status(int fd, struct strbuf *status)
}
strbuf_list_free(pair);
}
+
+   return (len < 0) ? len : 0;
 }
 
 void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry)
diff --git a/sub-process.h b/sub-process.h
index a88e782bfc..7d451e1cde 100644
--- a/sub-process.h
+++ b/sub-process.h
@@ -44,6 +44,6 @@ static inline struct child_process 
*subprocess_get_child_process(
  * key/value pairs and return the value from the last "status" packet
  */
 
-void subprocess_read_status(int fd, struct strbuf *status);
+int subprocess_read_status(int fd, struct strbuf *status);
 
 #endif
-- 
2.12.2.gvfs.2.20.g3624a68d62.dirty

[PATCH v7 04/10] convert: move packet_write_line() into pkt-line as packet_writel()

[Ben Peart]

Add packet_writel() which writes multiple lines in a single call and
then calls packet_flush_gently(). Update convert.c to use the new
packet_writel() function from pkt-line.

Signed-off-by: Ben Peart 
---
 convert.c  | 23 ++-
 pkt-line.c | 19 +++
 pkt-line.h |  1 +
 3 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/convert.c b/convert.c
index bdd528086f..1f18d947b9 100644
--- a/convert.c
+++ b/convert.c
@@ -521,25 +521,6 @@ static struct cmd2process 
*find_multi_file_filter_entry(struct hashmap *hashmap,
return hashmap_get(hashmap, , NULL);
 }
 
-static int packet_write_list(int fd, const char *line, ...)
-{
-   va_list args;
-   int err;
-   va_start(args, line);
-   for (;;) {
-   if (!line)
-   break;
-   if (strlen(line) > LARGE_PACKET_DATA_MAX)
-   return -1;
-   err = packet_write_fmt_gently(fd, "%s\n", line);
-   if (err)
-   return err;
-   line = va_arg(args, const char*);
-   }
-   va_end(args);
-   return packet_flush_gently(fd);
-}
-
 static void read_multi_file_filter_status(int fd, struct strbuf *status)
 {
struct strbuf **pair;
@@ -616,7 +597,7 @@ static struct cmd2process *start_multi_file_filter(struct 
hashmap *hashmap, cons
 
sigchain_push(SIGPIPE, SIG_IGN);
 
-   err = packet_write_list(process->in, "git-filter-client", "version=2", 
NULL);
+   err = packet_writel(process->in, "git-filter-client", "version=2", 
NULL);
if (err)
goto done;
 
@@ -632,7 +613,7 @@ static struct cmd2process *start_multi_file_filter(struct 
hashmap *hashmap, cons
if (err)
goto done;
 
-   err = packet_write_list(process->in, "capability=clean", 
"capability=smudge", NULL);
+   err = packet_writel(process->in, "capability=clean", 
"capability=smudge", NULL);
 
for (;;) {
cap_buf = packet_read_line(process->out, NULL);
diff --git a/pkt-line.c b/pkt-line.c
index 7db9119573..9d845ecc3c 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
return status;
 }
 
+int packet_writel(int fd, const char *line, ...)
+{
+   va_list args;
+   int err;
+   va_start(args, line);
+   for (;;) {
+   if (!line)
+   break;
+   if (strlen(line) > LARGE_PACKET_DATA_MAX)
+   return -1;
+   err = packet_write_fmt_gently(fd, "%s\n", line);
+   if (err)
+   return err;
+   line = va_arg(args, const char*);
+   }
+   va_end(args);
+   return packet_flush_gently(fd);
+}
+
 static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 {
static char packet_write_buffer[LARGE_PACKET_MAX];
diff --git a/pkt-line.h b/pkt-line.h
index 66ef610fc4..b2965869ad 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
 int packet_flush_gently(int fd);
 int packet_write_fmt_gently(int fd, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
+int packet_writel(int fd, const char *line, ...);
 int write_packetized_from_fd(int fd_in, int fd_out);
 int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
 
-- 
2.12.2.gvfs.2.20.g3624a68d62.dirty

[PATCH v7 08/10] convert: rename reusable sub-process functions

[Ben Peart]

Do a mechanical rename of the functions that will become the reusable
sub-process module.

Signed-off-by: Ben Peart 
---
 convert.c | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/convert.c b/convert.c
index 7d05dcb4aa..84c4ff8a01 100644
--- a/convert.c
+++ b/convert.c
@@ -507,8 +507,8 @@ struct cmd2process {
unsigned int supported_capabilities;
 };
 
-static int cmd_process_map_initialized;
-static struct hashmap cmd_process_map;
+static int subprocess_map_initialized;
+static struct hashmap subprocess_map;
 
 static int cmd2process_cmp(const struct subprocess_entry *e1,
   const struct subprocess_entry *e2,
@@ -517,7 +517,7 @@ static int cmd2process_cmp(const struct subprocess_entry 
*e1,
return strcmp(e1->cmd, e2->cmd);
 }
 
-static struct subprocess_entry *find_multi_file_filter_entry(struct hashmap 
*hashmap, const char *cmd)
+static struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, 
const char *cmd)
 {
struct subprocess_entry key;
 
@@ -526,7 +526,7 @@ static struct subprocess_entry 
*find_multi_file_filter_entry(struct hashmap *has
return hashmap_get(hashmap, , NULL);
 }
 
-static void read_multi_file_filter_status(int fd, struct strbuf *status)
+static void subprocess_read_status(int fd, struct strbuf *status)
 {
struct strbuf **pair;
char *line;
@@ -546,7 +546,7 @@ static void read_multi_file_filter_status(int fd, struct 
strbuf *status)
}
 }
 
-static void kill_multi_file_filter(struct hashmap *hashmap, struct 
subprocess_entry *entry)
+static void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry 
*entry)
 {
if (!entry)
return;
@@ -558,10 +558,10 @@ static void kill_multi_file_filter(struct hashmap 
*hashmap, struct subprocess_en
hashmap_remove(hashmap, entry, NULL);
 }
 
-static void stop_multi_file_filter(struct child_process *process)
+static void subprocess_exit_handler(struct child_process *process)
 {
sigchain_push(SIGPIPE, SIG_IGN);
-   /* Closing the pipe signals the filter to initiate a shutdown. */
+   /* Closing the pipe signals the subprocess to initiate a shutdown. */
close(process->in);
close(process->out);
sigchain_pop(SIGPIPE);
@@ -630,7 +630,7 @@ static int start_multi_file_filter_fn(struct 
subprocess_entry *subprocess)
 }
 
 typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
-int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry 
*entry, const char *cmd,
+int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, 
const char *cmd,
subprocess_start_fn startfn)
 {
int err;
@@ -646,11 +646,11 @@ int start_multi_file_filter(struct hashmap *hashmap, 
struct subprocess_entry *en
process->in = -1;
process->out = -1;
process->clean_on_exit = 1;
-   process->clean_on_exit_handler = stop_multi_file_filter;
+   process->clean_on_exit_handler = subprocess_exit_handler;
 
err = start_command(process);
if (err) {
-   error("cannot fork to run external filter '%s'", cmd);
+   error("cannot fork to run subprocess '%s'", cmd);
return err;
}
 
@@ -658,8 +658,8 @@ int start_multi_file_filter(struct hashmap *hashmap, struct 
subprocess_entry *en
 
err = startfn(entry);
if (err) {
-   error("initialization for external filter '%s' failed", cmd);
-   kill_multi_file_filter(hashmap, entry);
+   error("initialization for subprocess '%s' failed", cmd);
+   subprocess_stop(hashmap, entry);
return err;
}
 
@@ -678,12 +678,12 @@ static int apply_multi_file_filter(const char *path, 
const char *src, size_t len
struct strbuf filter_status = STRBUF_INIT;
const char *filter_type;
 
-   if (!cmd_process_map_initialized) {
-   cmd_process_map_initialized = 1;
-   hashmap_init(_process_map, (hashmap_cmp_fn) 
cmd2process_cmp, 0);
+   if (!subprocess_map_initialized) {
+   subprocess_map_initialized = 1;
+   hashmap_init(_map, (hashmap_cmp_fn) cmd2process_cmp, 
0);
entry = NULL;
} else {
-   entry = (struct cmd2process 
*)find_multi_file_filter_entry(_process_map, cmd);
+   entry = (struct cmd2process 
*)subprocess_find_entry(_map, cmd);
}
 
fflush(NULL);
@@ -692,7 +692,7 @@ static int apply_multi_file_filter(const char *path, const 
char *src, size_t len
entry = xmalloc(sizeof(*entry));
entry->supported_capabilities = 0;
 
-   if (start_multi_file_filter(_process_map, 
>subprocess, cmd, start_multi_file_filter_fn)) {
+   if (subprocess_start(_map, >subprocess, cmd, 
start_multi_file_filter_fn)) {
   

[PATCH v7 05/10] convert: split start_multi_file_filter() into two separate functions

[Ben Peart]

To enable future reuse of the filter..process infrastructure,
split start_multi_file_filter() into two separate parts.

start_multi_file_filter() will now only contain the generic logic to
manage the creation and tracking of the child process in a hashmap.

start_multi_file_filter_fn() is a protocol specific initialization
function that will negotiate the multi-file-filter interface version
and capabilities.

Signed-off-by: Ben Peart 
---
 convert.c | 58 ++
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/convert.c b/convert.c
index 1f18d947b9..4e1d018577 100644
--- a/convert.c
+++ b/convert.c
@@ -565,35 +565,14 @@ static void stop_multi_file_filter(struct child_process 
*process)
finish_command(process);
 }
 
-static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, 
const char *cmd)
+static int start_multi_file_filter_fn(struct cmd2process *entry)
 {
int err;
-   struct cmd2process *entry;
-   struct child_process *process;
-   const char *argv[] = { cmd, NULL };
struct string_list cap_list = STRING_LIST_INIT_NODUP;
char *cap_buf;
const char *cap_name;
-
-   entry = xmalloc(sizeof(*entry));
-   entry->cmd = cmd;
-   entry->supported_capabilities = 0;
-   process = >process;
-
-   child_process_init(process);
-   process->argv = argv;
-   process->use_shell = 1;
-   process->in = -1;
-   process->out = -1;
-   process->clean_on_exit = 1;
-   process->clean_on_exit_handler = stop_multi_file_filter;
-
-   if (start_command(process)) {
-   error("cannot fork to run external filter '%s'", cmd);
-   return NULL;
-   }
-
-   hashmap_entry_init(entry, strhash(cmd));
+   struct child_process *process = >process;
+   const char *cmd = entry->cmd;
 
sigchain_push(SIGPIPE, SIG_IGN);
 
@@ -642,6 +621,37 @@ static struct cmd2process *start_multi_file_filter(struct 
hashmap *hashmap, cons
 done:
sigchain_pop(SIGPIPE);
 
+   return err;
+}
+
+static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, 
const char *cmd)
+{
+   int err;
+   struct cmd2process *entry;
+   struct child_process *process;
+   const char *argv[] = { cmd, NULL };
+
+   entry = xmalloc(sizeof(*entry));
+   entry->cmd = cmd;
+   entry->supported_capabilities = 0;
+   process = >process;
+
+   child_process_init(process);
+   process->argv = argv;
+   process->use_shell = 1;
+   process->in = -1;
+   process->out = -1;
+   process->clean_on_exit = 1;
+   process->clean_on_exit_handler = stop_multi_file_filter;
+
+   if (start_command(process)) {
+   error("cannot fork to run external filter '%s'", cmd);
+   return NULL;
+   }
+
+   hashmap_entry_init(entry, strhash(cmd));
+
+   err = start_multi_file_filter_fn(entry);
if (err) {
error("initialization for external filter '%s' failed", cmd);
kill_multi_file_filter(hashmap, entry);
-- 
2.12.2.gvfs.2.20.g3624a68d62.dirty

[PATCH v7 02/10] pkt-line: fix packet_read_line() to handle len < 0 errors

[Ben Peart]

Update packet_read_line() to test for len > 0 to avoid potential bug
if read functions return lengths less than zero to indicate errors.

Signed-off-by: Ben Peart 
Found/Fixed-by: Lars Schneider 
---
 pkt-line.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pkt-line.c b/pkt-line.c
index d4b6bfe076..6f05b1a4a8 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -315,7 +315,7 @@ static char *packet_read_line_generic(int fd,
  PACKET_READ_CHOMP_NEWLINE);
if (dst_len)
*dst_len = len;
-   return len ? packet_buffer : NULL;
+   return (len > 0) ? packet_buffer : NULL;
 }
 
 char *packet_read_line(int fd, int *len_p)
-- 
2.12.2.gvfs.2.20.g3624a68d62.dirty

[PATCH v7 01/10] convert: remove erroneous tests for errno == EPIPE

[Ben Peart]

start_multi_file_filter() and apply_multi_file_filter() currently test
for errno == EPIPE but treating EPIPE as an error is already happening
from one of the packet_write() functions.

Signed-off-by: Ben Peart 
Found/Fixed-by: Jeff King 
Acked-by: Lars Schneider 
---
 convert.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/convert.c b/convert.c
index 4e17e45ed2..bdd528086f 100644
--- a/convert.c
+++ b/convert.c
@@ -661,7 +661,7 @@ static struct cmd2process *start_multi_file_filter(struct 
hashmap *hashmap, cons
 done:
sigchain_pop(SIGPIPE);
 
-   if (err || errno == EPIPE) {
+   if (err) {
error("initialization for external filter '%s' failed", cmd);
kill_multi_file_filter(hashmap, entry);
return NULL;
@@ -752,7 +752,7 @@ static int apply_multi_file_filter(const char *path, const 
char *src, size_t len
 done:
sigchain_pop(SIGPIPE);
 
-   if (err || errno == EPIPE) {
+   if (err) {
if (!strcmp(filter_status.buf, "error")) {
/* The filter signaled a problem with the file. */
} else if (!strcmp(filter_status.buf, "abort")) {
-- 
2.12.2.gvfs.2.20.g3624a68d62.dirty

[PATCH v7 06/10] convert: Separate generic structures and variables from the filter specific ones

[Ben Peart]

To enable future reuse of the filter..process infrastructure,
split the cmd2process structure into two separate parts.

subprocess_entry will now contain the generic data required to manage
the creation and tracking of the child process in a hashmap.

cmd2process is a filter protocol specific structure that is used to
track the negotiated capabilities of the filter.

Signed-off-by: Ben Peart 
---
 convert.c | 35 ---
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/convert.c b/convert.c
index 4e1d018577..5876218347 100644
--- a/convert.c
+++ b/convert.c
@@ -496,26 +496,31 @@ static int apply_single_file_filter(const char *path, 
const char *src, size_t le
 #define CAP_CLEAN(1u<<0)
 #define CAP_SMUDGE   (1u<<1)
 
-struct cmd2process {
+struct subprocess_entry {
struct hashmap_entry ent; /* must be the first member! */
-   unsigned int supported_capabilities;
const char *cmd;
struct child_process process;
 };
 
+struct cmd2process {
+   struct subprocess_entry subprocess; /* must be the first member! */
+   unsigned int supported_capabilities;
+};
+
 static int cmd_process_map_initialized;
 static struct hashmap cmd_process_map;
 
-static int cmd2process_cmp(const struct cmd2process *e1,
-  const struct cmd2process *e2,
+static int cmd2process_cmp(const struct subprocess_entry *e1,
+  const struct subprocess_entry *e2,
   const void *unused)
 {
return strcmp(e1->cmd, e2->cmd);
 }
 
-static struct cmd2process *find_multi_file_filter_entry(struct hashmap 
*hashmap, const char *cmd)
+static struct subprocess_entry *find_multi_file_filter_entry(struct hashmap 
*hashmap, const char *cmd)
 {
-   struct cmd2process key;
+   struct subprocess_entry key;
+
hashmap_entry_init(, strhash(cmd));
key.cmd = cmd;
return hashmap_get(hashmap, , NULL);
@@ -541,7 +546,7 @@ static void read_multi_file_filter_status(int fd, struct 
strbuf *status)
}
 }
 
-static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process 
*entry)
+static void kill_multi_file_filter(struct hashmap *hashmap, struct 
subprocess_entry *entry)
 {
if (!entry)
return;
@@ -571,8 +576,8 @@ static int start_multi_file_filter_fn(struct cmd2process 
*entry)
struct string_list cap_list = STRING_LIST_INIT_NODUP;
char *cap_buf;
const char *cap_name;
-   struct child_process *process = >process;
-   const char *cmd = entry->cmd;
+   struct child_process *process = >subprocess.process;
+   const char *cmd = entry->subprocess.cmd;
 
sigchain_push(SIGPIPE, SIG_IGN);
 
@@ -632,9 +637,9 @@ static struct cmd2process *start_multi_file_filter(struct 
hashmap *hashmap, cons
const char *argv[] = { cmd, NULL };
 
entry = xmalloc(sizeof(*entry));
-   entry->cmd = cmd;
+   entry->subprocess.cmd = cmd;
entry->supported_capabilities = 0;
-   process = >process;
+   process = >subprocess.process;
 
child_process_init(process);
process->argv = argv;
@@ -654,7 +659,7 @@ static struct cmd2process *start_multi_file_filter(struct 
hashmap *hashmap, cons
err = start_multi_file_filter_fn(entry);
if (err) {
error("initialization for external filter '%s' failed", cmd);
-   kill_multi_file_filter(hashmap, entry);
+   kill_multi_file_filter(hashmap, >subprocess);
return NULL;
}
 
@@ -678,7 +683,7 @@ static int apply_multi_file_filter(const char *path, const 
char *src, size_t len
hashmap_init(_process_map, (hashmap_cmp_fn) 
cmd2process_cmp, 0);
entry = NULL;
} else {
-   entry = find_multi_file_filter_entry(_process_map, cmd);
+   entry = (struct cmd2process 
*)find_multi_file_filter_entry(_process_map, cmd);
}
 
fflush(NULL);
@@ -688,7 +693,7 @@ static int apply_multi_file_filter(const char *path, const 
char *src, size_t len
if (!entry)
return 0;
}
-   process = >process;
+   process = >subprocess.process;
 
if (!(wanted_capability & entry->supported_capabilities))
return 0;
@@ -759,7 +764,7 @@ static int apply_multi_file_filter(const char *path, const 
char *src, size_t len
 * Force shutdown and restart if another blob requires 
filtering.
 */
error("external filter '%s' failed", cmd);
-   kill_multi_file_filter(_process_map, entry);
+   kill_multi_file_filter(_process_map, 
>subprocess);
}
} else {
strbuf_swap(dst, );
-- 
2.12.2.gvfs.2.20.g3624a68d62.dirty

[PATCH v7 07/10] convert: Update generic functions to only use generic data structures

[Ben Peart]

Update all functions that are going to be moved into a reusable module
so that they only work with the reusable data structures.  Move code
that is specific to the filter out into the filter specific functions.

Signed-off-by: Ben Peart 
---
 convert.c | 41 +++--
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/convert.c b/convert.c
index 5876218347..7d05dcb4aa 100644
--- a/convert.c
+++ b/convert.c
@@ -556,7 +556,6 @@ static void kill_multi_file_filter(struct hashmap *hashmap, 
struct subprocess_en
finish_command(>process);
 
hashmap_remove(hashmap, entry, NULL);
-   free(entry);
 }
 
 static void stop_multi_file_filter(struct child_process *process)
@@ -570,14 +569,15 @@ static void stop_multi_file_filter(struct child_process 
*process)
finish_command(process);
 }
 
-static int start_multi_file_filter_fn(struct cmd2process *entry)
+static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
 {
int err;
+   struct cmd2process *entry = (struct cmd2process *)subprocess;
struct string_list cap_list = STRING_LIST_INIT_NODUP;
char *cap_buf;
const char *cap_name;
-   struct child_process *process = >subprocess.process;
-   const char *cmd = entry->subprocess.cmd;
+   struct child_process *process = >process;
+   const char *cmd = subprocess->cmd;
 
sigchain_push(SIGPIPE, SIG_IGN);
 
@@ -629,17 +629,16 @@ static int start_multi_file_filter_fn(struct cmd2process 
*entry)
return err;
 }
 
-static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, 
const char *cmd)
+typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
+int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry 
*entry, const char *cmd,
+   subprocess_start_fn startfn)
 {
int err;
-   struct cmd2process *entry;
struct child_process *process;
const char *argv[] = { cmd, NULL };
 
-   entry = xmalloc(sizeof(*entry));
-   entry->subprocess.cmd = cmd;
-   entry->supported_capabilities = 0;
-   process = >subprocess.process;
+   entry->cmd = cmd;
+   process = >process;
 
child_process_init(process);
process->argv = argv;
@@ -649,22 +648,23 @@ static struct cmd2process *start_multi_file_filter(struct 
hashmap *hashmap, cons
process->clean_on_exit = 1;
process->clean_on_exit_handler = stop_multi_file_filter;
 
-   if (start_command(process)) {
+   err = start_command(process);
+   if (err) {
error("cannot fork to run external filter '%s'", cmd);
-   return NULL;
+   return err;
}
 
hashmap_entry_init(entry, strhash(cmd));
 
-   err = start_multi_file_filter_fn(entry);
+   err = startfn(entry);
if (err) {
error("initialization for external filter '%s' failed", cmd);
-   kill_multi_file_filter(hashmap, >subprocess);
-   return NULL;
+   kill_multi_file_filter(hashmap, entry);
+   return err;
}
 
hashmap_add(hashmap, entry);
-   return entry;
+   return 0;
 }
 
 static int apply_multi_file_filter(const char *path, const char *src, size_t 
len,
@@ -689,9 +689,13 @@ static int apply_multi_file_filter(const char *path, const 
char *src, size_t len
fflush(NULL);
 
if (!entry) {
-   entry = start_multi_file_filter(_process_map, cmd);
-   if (!entry)
+   entry = xmalloc(sizeof(*entry));
+   entry->supported_capabilities = 0;
+
+   if (start_multi_file_filter(_process_map, 
>subprocess, cmd, start_multi_file_filter_fn)) {
+   free(entry);
return 0;
+   }
}
process = >subprocess.process;
 
@@ -765,6 +769,7 @@ static int apply_multi_file_filter(const char *path, const 
char *src, size_t len
 */
error("external filter '%s' failed", cmd);
kill_multi_file_filter(_process_map, 
>subprocess);
+   free(entry);
}
} else {
strbuf_swap(dst, );
-- 
2.12.2.gvfs.2.20.g3624a68d62.dirty

[PATCH v7 09/10] sub-process: move sub-process functions into separate files

[Ben Peart]

Move the sub-proces functions into sub-process.h/c.  Add documentation
for the new module in Documentation/technical/api-sub-process.txt

Signed-off-by: Ben Peart 
---
 Documentation/technical/api-sub-process.txt |  59 
 Makefile|   1 +
 convert.c   | 104 +---
 sub-process.c   | 102 +++
 sub-process.h   |  49 +
 5 files changed, 212 insertions(+), 103 deletions(-)
 create mode 100644 Documentation/technical/api-sub-process.txt
 create mode 100644 sub-process.c
 create mode 100644 sub-process.h

diff --git a/Documentation/technical/api-sub-process.txt 
b/Documentation/technical/api-sub-process.txt
new file mode 100644
index 00..793508cf3e
--- /dev/null
+++ b/Documentation/technical/api-sub-process.txt
@@ -0,0 +1,59 @@
+sub-process API
+===
+
+The sub-process API makes it possible to run background sub-processes
+for the entire lifetime of a Git invocation. If Git needs to communicate
+with an external process multiple times, then this can reduces the process
+invocation overhead. Git and the sub-process communicate through stdin and
+stdout.
+
+The sub-processes are kept in a hashmap by command name and looked up
+via the subprocess_find_entry function.  If an existing instance can not
+be found then a new process should be created and started.  When the
+parent git command terminates, all sub-processes are also terminated.
+
+This API is based on the run-command API.
+
+Data structures
+---
+
+* `struct subprocess_entry`
+
+The sub-process structure.  Members should not be accessed directly.
+
+Types
+-
+
+'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::
+
+   User-supplied function to initialize the sub-process.  This is
+   typically used to negotiate the interface version and capabilities.
+
+
+Functions
+-
+
+`cmd2process_cmp`::
+
+   Function to test two subprocess hashmap entries for equality.
+
+`subprocess_start`::
+
+   Start a subprocess and add it to the subprocess hashmap.
+
+`subprocess_stop`::
+
+   Kill a subprocess and remove it from the subprocess hashmap.
+
+`subprocess_find_entry`::
+
+   Find a subprocess in the subprocess hashmap.
+
+`subprocess_get_child_process`::
+
+   Get the underlying `struct child_process` from a subprocess.
+
+`subprocess_read_status`::
+
+   Helper function to read packets looking for the last "status="
+   key/value pair.
diff --git a/Makefile b/Makefile
index 47827aa129..3239dcbf0a 100644
--- a/Makefile
+++ b/Makefile
@@ -832,6 +832,7 @@ LIB_OBJS += streaming.o
 LIB_OBJS += string-list.o
 LIB_OBJS += submodule.o
 LIB_OBJS += submodule-config.o
+LIB_OBJS += sub-process.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += tempfile.o
diff --git a/convert.c b/convert.c
index 84c4ff8a01..acf51416d1 100644
--- a/convert.c
+++ b/convert.c
@@ -4,6 +4,7 @@
 #include "quote.h"
 #include "sigchain.h"
 #include "pkt-line.h"
+#include "sub-process.h"
 
 /*
  * convert.c - convert a file when checking it out and checking it in.
@@ -496,12 +497,6 @@ static int apply_single_file_filter(const char *path, 
const char *src, size_t le
 #define CAP_CLEAN(1u<<0)
 #define CAP_SMUDGE   (1u<<1)
 
-struct subprocess_entry {
-   struct hashmap_entry ent; /* must be the first member! */
-   const char *cmd;
-   struct child_process process;
-};
-
 struct cmd2process {
struct subprocess_entry subprocess; /* must be the first member! */
unsigned int supported_capabilities;
@@ -510,65 +505,6 @@ struct cmd2process {
 static int subprocess_map_initialized;
 static struct hashmap subprocess_map;
 
-static int cmd2process_cmp(const struct subprocess_entry *e1,
-  const struct subprocess_entry *e2,
-  const void *unused)
-{
-   return strcmp(e1->cmd, e2->cmd);
-}
-
-static struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, 
const char *cmd)
-{
-   struct subprocess_entry key;
-
-   hashmap_entry_init(, strhash(cmd));
-   key.cmd = cmd;
-   return hashmap_get(hashmap, , NULL);
-}
-
-static void subprocess_read_status(int fd, struct strbuf *status)
-{
-   struct strbuf **pair;
-   char *line;
-   for (;;) {
-   line = packet_read_line(fd, NULL);
-   if (!line)
-   break;
-   pair = strbuf_split_str(line, '=', 2);
-   if (pair[0] && pair[0]->len && pair[1]) {
-   /* the last "status=" line wins */
-   if (!strcmp(pair[0]->buf, "status=")) {
-   strbuf_reset(status);
-   strbuf_addbuf(status, pair[1]);
-   }
-   }
-   strbuf_list_free(pair);
-

[PATCH v7 03/10] pkt-line: add packet_read_line_gently()

[Ben Peart]

Add packet_read_line_gently() to enable reading a line without dying on
EOF.

Signed-off-by: Ben Peart 
---
 pkt-line.c | 12 
 pkt-line.h | 11 +++
 2 files changed, 23 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index 6f05b1a4a8..7db9119573 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -323,6 +323,18 @@ char *packet_read_line(int fd, int *len_p)
return packet_read_line_generic(fd, NULL, NULL, len_p);
 }
 
+int packet_read_line_gently(int fd, int *dst_len, char **dst_line)
+{
+   int len = packet_read(fd, NULL, NULL,
+ packet_buffer, sizeof(packet_buffer),
+ 
PACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);
+   if (dst_len)
+   *dst_len = len;
+   if (dst_line)
+   *dst_line = (len > 0) ? packet_buffer : NULL;
+   return len;
+}
+
 char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
 {
return packet_read_line_generic(-1, src, src_len, dst_len);
diff --git a/pkt-line.h b/pkt-line.h
index 18eac64830..66ef610fc4 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -74,6 +74,17 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, 
char
 char *packet_read_line(int fd, int *size);
 
 /*
+ * Convenience wrapper for packet_read that sets the PACKET_READ_GENTLE_ON_EOF
+ * and CHOMP_NEWLINE options. The return value specifies the number of bytes
+ * read into the buffer or -1 on truncated input. If the *dst_line parameter
+ * is not NULL it will return NULL for a flush packet or when the number of
+ * bytes copied is zero and otherwise points to a static buffer (that may be
+ * overwritten by subsequent calls). If the size parameter is not NULL, the
+ * length of the packet is written to it.
+ */
+int packet_read_line_gently(int fd, int *size, char **dst_line);
+
+/*
  * Same as packet_read_line, but read from a buf rather than a descriptor;
  * see packet_read for details on how src_* is used.
  */
-- 
2.12.2.gvfs.2.20.g3624a68d62.dirty

[PATCH v7 00/10] refactor the filter process code into a reusable module

[Ben Peart]

Changes from V6 include:

convert: remove erroneous tests for errno == EPIPE
 - split into separate patch to fix a preexisting bug discovered in the review 
process

pkt-line: Update packet_read_line() to test for len > 0
 - split into separate patch to deal with errors that return negative lengths

pkt-line: add packet_read_line_gently()
 - update documentation to clarify return values
 - update white space in function definition


Refactor the filter..process code into a separate sub-process
module that can be used to reduce the cost of starting up a sub-process
for multiple commands.  It does this by keeping the external process
running and processing all commands by communicating over standard input
and standard output using the packet format (pkt-line) based protocol.
Full documentation is in Documentation/technical/api-sub-process.txt.

This code is refactored from:

Commit edcc85814c ("convert: add filter..process option", 
2016-10-16)
keeps the external process running and processes all commands

Ben Peart (10):
  convert: remove erroneous tests for errno == EPIPE
  pkt-line: fix packet_read_line() to handle len < 0 errors
  pkt-line: add packet_read_line_gently()
  convert: move packet_write_line() into pkt-line as packet_writel()
  convert: split start_multi_file_filter() into two separate functions
  convert: Separate generic structures and variables from the filter
specific ones
  convert: Update generic functions to only use generic data structures
  convert: rename reusable sub-process functions
  sub-process: move sub-process functions into separate files
  convert: Update subprocess_read_status to not die on EOF

 Documentation/technical/api-sub-process.txt |  59 ++
 Makefile|   1 +
 convert.c   | 161 ++--
 pkt-line.c  |  33 +-
 pkt-line.h  |  12 +++
 sub-process.c   | 106 ++
 sub-process.h   |  49 +
 7 files changed, 292 insertions(+), 129 deletions(-)
 create mode 100644 Documentation/technical/api-sub-process.txt
 create mode 100644 sub-process.c
 create mode 100644 sub-process.h

-- 
2.12.2.gvfs.2.20.g3624a68d62.dirty

Re: not uptodate. Cannot merge

[Torsten Bögershausen]

On 2017-05-04 23:40, G. Sylvie Davies wrote:
> Hi,
> 
> My little bitbucket "cherry-pick" button is failing on Windows from a
> "git reset --hard" blowing up.
> 
> My situation:  Git-2.10.2.windows.1 / Bitbucket-4.14.3 / Windows
> 10-10.0-amd64.   But I suspect even more recent Git will have the same
> problem.
> 
> Now, I'm pretty far from Kansas here as you'll see from my "git clone"
> invocation:
> 
> git.exe clone -c core.checkStat=minimal -c core.fileMode=false -c
> core.ignoreStat=true -c core.trustctime=false -c
> core.logAllRefUpdates=false --shared
> C:\Users\gsylvie\dev\bb\target\bitbucket\home\shared\data\repositories\1
> C:\Users\gsylvie\dev\bb\target\bitbucket\home\caches\bbClones\1
> 
> 
> Right after cloning I create a ".git/info/attributes" file containing
> just this one line:
> 
> * -text
This -may- be part of the problem.
In general, it is possible to add attributes on your local copy like
this, but it is not recommendet, at least not from me.
In general, the project should have a .gitattributes file, which
belongs to the project and which travels together with push and pull.
And of course, files should have been "normalized" and have LF in the repo.

In your case:
What does
git config core.autocrlf
say?

[]

Re: not uptodate. Cannot merge

[Johannes Schindelin]

Hi Sylvie,

On Thu, 4 May 2017, G. Sylvie Davies wrote:

> My situation:  Git-2.10.2.windows.1 / Bitbucket-4.14.3 / Windows
> 10-10.0-amd64.   But I suspect even more recent Git will have the same
> problem.

In contrast, I suspect that recent Git for Windows versions have tons of
CR/LF-related fixes... ;-)

> Right after cloning I create a ".git/info/attributes" file containing
> just this one line:
> 
> * -text
> 
> 
> After the clone, here's the sequence of commands leading up to the bad
> "git reset --hard".  These are all fine (well, the "--aborts" whine a
> little, but that's expected):
> 
> git.exe branch --unset-upstream
> git.exe update-index --refresh
> git.exe rebase --abort
> git.exe cherry-pick --abort
> 
> 
> And here's the "git reset --hard" that fails:
> 
> git.exe reset --hard --quiet d6edcbf924697ab811a867421dab60d954ccad99 --
> 
> ---
> Exit=128
> error: Entry 'basic_branching/file.txt' not uptodate. Cannot merge.
> fatal: Could not reset index file to revision
> 'd6edcbf924697ab811a867421dab60d954ccad99'.
> ---

This smells very much like a problem I vaguely remember has been addressed
recently: Torsten Bögershausen was working on issues where files checked
out with one line ending, and then "retroactively" become dirty by
changing the line ending convention (which your -text seems to do) and Git
not really noticing this until the `reset --hard` call that simply cannot
cope with "this kind of dirty".

In essence, I am fairly certain that v2.12.2(2) should *not* display this
behavior.

If my hunch is wrong, please do fill out a full bug report at
https://github.com/git-for-windows/git/issues/new, preferably with a short
and sweet script to reproduce the problem elsewhere.

Thanks,
Johannes

[PATCH 1/2] split-index: add and use unshare_split_index()

[Christian Couder]

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

When split-index is being used, we have two cache_entry arrays in
index_state->cache[] and index_state->split_index->base->cache[].

index_state->cache[] may share the same entries with base->cache[] so
we can quickly determine what entries are shared. This makes memory
management tricky, we can't free base->cache[] until we know
index_state->cache[] does not point to any of those entries.

unshare_split_index() is added for this purpose, to find shared
entries and either duplicate them in index_state->cache[], or discard
them. Either way it should be safe to free base->cache[] after
unshare_split_index().

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 read-cache.c  | 10 ++
 split-index.c | 57 -
 split-index.h |  1 +
 3 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 0d0081a11b..8da84ae2d1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1877,15 +1877,9 @@ int discard_index(struct index_state *istate)
 {
int i;
 
-   for (i = 0; i < istate->cache_nr; i++) {
-   if (istate->cache[i]->index &&
-   istate->split_index &&
-   istate->split_index->base &&
-   istate->cache[i]->index <= 
istate->split_index->base->cache_nr &&
-   istate->cache[i] == 
istate->split_index->base->cache[istate->cache[i]->index - 1])
-   continue;
+   unshare_split_index(istate, 1);
+   for (i = 0; i < istate->cache_nr; i++)
free(istate->cache[i]);
-   }
resolve_undo_clear_index(istate);
istate->cache_nr = 0;
istate->cache_changed = 0;
diff --git a/split-index.c b/split-index.c
index f519e60f87..49bd197f71 100644
--- a/split-index.c
+++ b/split-index.c
@@ -73,10 +73,17 @@ void move_cache_to_base_index(struct index_state *istate)
int i;
 
/*
-* do not delete old si->base, its index entries may be shared
-* with istate->cache[]. Accept a bit of leaking here because
-* this code is only used by short-lived update-index.
+* If "si" is shared with another index_state (e.g. by
+* unpack-trees code), we will need to duplicate split_index
+* struct. It's not happening now though, luckily.
 */
+   assert(si->refcount <= 1);
+
+   unshare_split_index(istate, 0);
+   if (si->base) {
+   discard_index(si->base);
+   free(si->base);
+   }
si->base = xcalloc(1, sizeof(*si->base));
si->base->version = istate->version;
/* zero timestamp disables racy test in ce_write_index() */
@@ -275,11 +282,41 @@ void finish_writing_split_index(struct index_state 
*istate)
istate->cache_nr = si->saved_cache_nr;
 }
 
+void unshare_split_index(struct index_state *istate, int discard)
+{
+   struct split_index *si = istate->split_index;
+   int i;
+
+   if (!si || !si->base)
+   return;
+
+   for (i = 0; i < istate->cache_nr; i++) {
+   struct cache_entry *ce = istate->cache[i];
+   struct cache_entry *new = NULL;
+
+   if (!ce->index ||
+   ce->index > si->base->cache_nr ||
+   ce != si->base->cache[ce->index - 1])
+   continue;
+
+   if (!discard) {
+   int len = ce_namelen(ce);
+   new = xcalloc(1, cache_entry_size(len));
+   copy_cache_entry(new, ce);
+   memcpy(new->name, ce->name, len);
+   new->index = 0;
+   }
+   istate->cache[i] = new;
+   }
+}
+
+
 void discard_split_index(struct index_state *istate)
 {
struct split_index *si = istate->split_index;
if (!si)
return;
+   unshare_split_index(istate, 0);
istate->split_index = NULL;
si->refcount--;
if (si->refcount)
@@ -328,14 +365,8 @@ void add_split_index(struct index_state *istate)
 
 void remove_split_index(struct index_state *istate)
 {
-   if (istate->split_index) {
-   /*
-* can't discard_split_index(_index); because that
-* will destroy split_index->base->cache[], which may
-* be shared with the_index.cache[]. So yeah we're
-* leaking a bit here.
-*/
-   istate->split_index = NULL;
-   istate->cache_changed |= SOMETHING_CHANGED;
-   }
+   if (!istate->split_index)
+   return;
+   discard_split_index(istate);
+   istate->cache_changed |= SOMETHING_CHANGED;
 }
diff --git a/split-index.h b/split-index.h
index df91c1bda8..65c0f09b2b 100644
--- a/split-index.h
+++ b/split-index.h
@@ -33,5 +33,6 @@ void finish_writing_split_index(struct index_state *istate);
 void 

[PATCH 0/2] split index extra bits

[Christian Couder]

This patch series contains 2 patches that have already been sent to
the list but have felt through the cracks. It would be nice if they
could be considered for v2.13.0.

Patch 1/2 (split-index: add and use unshare_split_index()) by Duy
fixes a memory leak when a new shared index file is created and memory
used by the old one should be freed.

Patch 2/2 (p3400: add perf tests for rebasing many changes) adds a
test in t/perf that can help see what performance improvements can be
gained by using the split index feature. For example using this patch
at one point I got the following results on a Linux machine on the Git
repository:

Testv2.10.0 
v2.11.0  v2.12.0  origin/master 

3400.2: rebase on top of a lot of unrelated changes 2.62(2.19+0.55) 
2.54(2.15+0.49) -3.1%2.59(2.19+0.49) -1.1%2.66(2.27+0.52) +1.5% 
3400.4: rebase a lot of unrelated changes without split-index   
14.08(10.12+3.92)   7.20(5.85+1.36) -48.9%   8.06(6.54+1.60) -42.8%   
7.53(6.40+1.12) -46.5%
3400.6: rebase a lot of unrelated changes with split-index  
14.10(10.11+3.94)   6.94(5.71+1.27) -50.8%   7.99(6.49+1.57) -43.3%   
6.10(5.12+1.07) -56.7%

On bigger repositories results are likely to be better.

Christian Couder (1):
  p3400: add perf tests for rebasing many changes

Nguyễn Thái Ngọc Duy (1):
  split-index: add and use unshare_split_index()

 read-cache.c   | 10 ++---
 split-index.c  | 57 ++
 split-index.h  |  1 +
 t/perf/p3400-rebase.sh | 22 ++-
 4 files changed, 68 insertions(+), 22 deletions(-)

-- 
2.13.0.rc1.83.g83955d3ecd.dirty

[PATCH 2/2] p3400: add perf tests for rebasing many changes

[Christian Couder]

Rebasing onto many changes is interesting, but it's also
interesting to see what happens when rebasing many changes.

And while at it, let's also look at the impact of using a
split index.

Signed-off-by: Christian Couder 
---
 t/perf/p3400-rebase.sh | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh
index b3e7d525d2..ce271ca4c1 100755
--- a/t/perf/p3400-rebase.sh
+++ b/t/perf/p3400-rebase.sh
@@ -5,7 +5,7 @@ test_description='Tests rebase performance'
 
 test_perf_default_repo
 
-test_expect_success 'setup' '
+test_expect_success 'setup rebasing on top of a lot of changes' '
git checkout -f -b base &&
git checkout -b to-rebase &&
git checkout -b upstream &&
@@ -33,4 +33,24 @@ test_perf 'rebase on top of a lot of unrelated changes' '
git rebase --onto base HEAD^
 '
 
+test_expect_success 'setup rebasing many changes without split-index' '
+   git config core.splitIndex false &&
+   git checkout -b upstream2 to-rebase &&
+   git checkout -b to-rebase2 upstream
+'
+
+test_perf 'rebase a lot of unrelated changes without split-index' '
+   git rebase --onto upstream2 base &&
+   git rebase --onto base upstream2
+'
+
+test_expect_success 'setup rebasing many changes with split-index' '
+   git config core.splitIndex true
+'
+
+test_perf 'rebase a lot of unrelated changes with split-index' '
+   git rebase --onto upstream2 base &&
+   git rebase --onto base upstream2
+'
+
 test_done
-- 
2.13.0.rc1.83.g83955d3ecd.dirty

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Johannes Schindelin]

Hi Stefan & Junio,

On Thu, 4 May 2017, Stefan Beller wrote:

> So instead of a mechanical replacement, we'd rather want to
> see "the_index" not appearing at all outside of builtins, which
> implies two things:
>
> * If done properly we can move the macros from cache.h to
>   e.g. builtin.h. That way future developers are less tempted
>   to use the cache_* macros in the library code.

Ye!

> * we'd have to pass through the_index from the builtin function
>   down to the library code, potentially going through multiple
>   function. For this it is unclear if we want to start this now, or wait
>   until Brandon presents his initial repository object struct, which
>   may be suited better for passing-around.

Or the other way round. I guess passing a struct index_state can be a
first step, and we can later convert it to struct repository. I fathom
that more places will need a struct repository parameter than a struct
index_state parameter. That is, if you first identify all the places where
the index_state parameter is required, it should make the struct
repository change easier.

> So if I want to further look into refactoring, I'll have a look into
> the object store and hold off sending a series that drops the_index.
> 
> > Also, dropping compatibility macros at the end of the series is
> > unfriendly to fellow developers with WIPs that depend on the
> > traditional way of doing things.  It is doubly insulting to them to
> > send such a series during the feature freeze period, when it is more
> > likely that they are holding off their WIP in an attempt to allow us
> > to concentrate more on what we should be, i.e. finding and fixing
> > possible regressions at the tip of 'master' to polish the upcoming
> > release and help the end users.
> 
> Personally I have not noticed large variations of patch volume
> correlated to pre-release times.

Speaking for myself, I also use this "slow" time to prepare patch series
that should be submitted directly after a major version bump, patch series
like the timestamp_t one (with which I failed miserably: it got to a
usable state only now, very short *before* a major version bump).

But yes, part of why I set aside a substantial chunk of time to work on
the Coverity report was to prepare for the major version, to make sure
that we did not have anything blatant (like the difftool use-after-free,
which was embarrassing) in v2.13.0.

It may look as if the coverity patches do not focus on critical fixes, but
that's only because I did not find anything major in the Coverity report
(I looked primarily for Git for Windows-specific stuff, though, to be
honest).

Maybe it is a similar situation for other patch contributors: they tried
to focus on critical fixes and ended up not finding anything really
critical?

Having said that, I did see a little shift toward cppcheck & sparse
related patches ;-)

Ciao,
Dscho

Re: How to `git status' without scrambling modified with new, etc

[Ævar Arnfjörð Bjarmason]

On Fri, May 5, 2017 at 4:02 PM, Harry Putnam  wrote:
> This is probably what everyone sees:
>
> When I run `git status'; I see  modified and newfiles scrambled together
>
> Is there a trick or technique to make that output show each category
> separately?
>
> Or do folks just a throw a `sort' in there (git status|sort) and lose
> the color ouput?

If you set color.ui=auto it'll disable coloring when it detects that
the output isn't to a terminal, i.e. being piped.

It sounds like you want:

git -c color.ui=always status --short|sort

But there's no native option to sort the status output, but that'll do
it for you.

Re: [RFC PATCH 01/10] Remove unneeded dependency on blob.h from blame

[Jeffrey Smith]

While it was added there (it was part of builtin pickaxe which was
renamed builtin blame), it was actually needed then, but I do see
what you are getting at.

It was no longer needed in commit 21666f1 ("convert object type
handling from a string to a number", 2007-02-26) with the removed
use of blob_type.  I will update the commit message accordingly.

Thanks

On Fri, May 5, 2017 at 8:52 AM, Jeffrey Smith  wrote:
> While it was added there (it was part of builtin pickaxe which was
> renamed builtin blame), it was actually needed then, but I do see
> what you are getting at.
>
> It was no longer needed in commit 21666f1 ("convert object type
> handling from a string to a number", 2007-02-26) with the removed
> use of blob_type.  I will update the commit message accordingly.
>
> Thanks
>
> P.S. Sorry for the duplicate message Ævar. I forgot to reply all
> the first time.
>
>
> On Fri, May 5, 2017 at 2:07 AM, Ævar Arnfjörð Bjarmason 
> wrote:
>>
>> For commit message: This was originally added in commit acca687fa9
>> ("git-pickaxe: retire pickaxe", 2006-11-08), but has never been
>> needed.
>>
>> On Fri, May 5, 2017 at 7:27 AM, Jeff Smith  wrote:
>> > Signed-off-by: Jeff Smith 
>> > ---
>> >  builtin/blame.c | 1 -
>> >  1 file changed, 1 deletion(-)
>> >
>> > diff --git a/builtin/blame.c b/builtin/blame.c
>> > index 07506a3..42c56eb 100644
>> > --- a/builtin/blame.c
>> > +++ b/builtin/blame.c
>> > @@ -8,7 +8,6 @@
>> >  #include "cache.h"
>> >  #include "refs.h"
>> >  #include "builtin.h"
>> > -#include "blob.h"
>> >  #include "commit.h"
>> >  #include "tag.h"
>> >  #include "tree-walk.h"
>> > --
>> > 2.9.3
>> >
>
>

How to `git status' without scrambling modified with new, etc

[Harry Putnam]

This is probably what everyone sees:

When I run `git status'; I see  modified and newfiles scrambled together

Is there a trick or technique to make that output show each category
separately?

Or do folks just a throw a `sort' in there (git status|sort) and lose
the color ouput?

git client debug with customer ssh client

[Pierre J. Ludwick]

Hi,

Git client has been ported to HPE NonStop. This OS is Posix called OSS.

Currently this git client works with openSSH ssh client also ported to 
OSS.


 When we try to use our "native" ssh client, a custom written ssh 
client, using GIT_SSH= to point to it we are running into some 
problems. We are this custom ssh developers.


The problem is there is establish connection to Gitlab, the server, 
and some work is done between git client and git server, but then it 
seems git client just sits waiting. We can't figure out what it 
waiting for not knowing this client/server protocol and would need 
some way of knowing to more productively debug our custom ssh client 
code.


We are not an actor in the communication between git client and git 
server, we just transport data packets transparently with ssh. So 
either one of the communication partners do not go on with the 
protocol or some data is stuck in ssh (or the pipe server on the OS). 
We can only know the details if we know what the git server 
sent/received and what we sent/received. From our side the only thing 
we can do is to dump the data from the git server when we receive it. 
Beyond that we need the data dump from the git server and the git 
client. Additional state info from the git client and server would 
help to understand what went wrong, which data is missing, which state 
it is in.


How can we get more info from git client? Any helps suggestions welcomed?


Here is the output of a clone cmd, with tracing turned on in git client:
- 



root-pl@BWNS02-ZTC0:~$ cat ~/bin/plsh_sshgit

#!/bin/sh
run -name=/G/plsh3 /home/pl/bin/ssh -oIdentity=rsa2k -S \$plssh -S 
\$plssh -Q -Z "$@"




cmd & output:
--

GIT_SSH=/home/pl/bin/plsh_sshgit GIT_TRACE=true GIT_TRACE_PACKET=true 
GIT_TRACE_PACK_ACCESS=true GIT_TRACE_SETUP=true git clone 
git@dev-ssh-debian.comforte.local:pl/test.git


Cloning into 'test'...
warning: templates not found /usr/local/share/git-core/templates
06:04:25.030966 trace: run_command: '/home/pl/bin/plsh_sshgit' 
'git@dev-ssh-debian.comforte.local' 'git-upload-pack '\''pl/test.git'

\'''
06:05:10.517100 packet:clone< 
d789ced47cd9cca76409656507ea5bdc646c4051 HEAD\0multi_ack thin-pack 
side-band side-band-64k ofs
-delta shallow no-progress include-tag multi_ack_detailed 
symref=HEAD:refs/heads/master agent=git/2.10.2
06:05:10.524625 packet:clone< 
7bdce38446aae35c92dbab790257313c4d9d950f refs/heads/18-management-fix
06:05:10.524782 packet:clone< 
bafcadca5e47b0949ce3135312f587fbc80db237 refs/heads/2452-pw-reset-email
06:05:10.524937 packet:clone< 
a927a7c30759c4ad7461525ee55e1a7aed1d490c refs/heads/add-build-signals
06:05:10.525094 packet:clone< 
717faa92a7ef00c5cfca7f56dbb0658751e421e5 refs/heads/add-call-to-theme-js
06:05:10.525271 packet:clone< 
299ea53cf39c670399d3d09ff30be5efab2749c0 refs/heads/add-docsearch-url
06:05:10.525433 packet:clone< 
beed6029b7035ed9c47ed604bb0d19372a7c720b refs/heads/add-gold-projects
06:05:10.525627 packet:clone< 
ef5c7090095c4004f052d816b759ef441d3f68d0 
refs/heads/add-page-suffix-support
06:05:10.525801 packet:clone< 
0384faf59f796a8c6617239f0abe1604416c9acb refs/heads/add-payment-form
06:05:10.525997 packet:clone< 
277e9c0701d14375e2a83ba4821545add35789f9 
refs/heads/add-project-container-image
06:05:10.526150 packet:clone< 
28d240bbef04b55fe4a6b856f146e5003f1d540e refs/heads/add-wipe-button-to-ui
06:05:10.526356 packet:clone< 
bf0363cd0a566f8077ceb159094e87019a9c8d7b refs/heads/admin-icon
06:05:10.526516 packet:clone< 
5aa5151470bc8029b027d207fd71e81808361f40 refs/heads/api-v2-docs
06:05:10.526736 packet:clone< 
399f924f8067595ad5d2296898ad0761e7a60a02 refs/heads/auth-cas-dev
06:05:10.526888 packet:clone< 
249421d0b13c335a37b5ed695274070da8dd5e32 refs/heads/auto-import
06:05:10.527531 packet:clone< 
c922688c4d88dc831541d40e7251c74a11a783d1 refs/heads/backport-style
06:05:10.527680 packet:clone< 
fa501c59dd1a96009ad2f7ee1a577662699348d9 refs/heads/badge-v2
06:05:10.527897 packet:clone< 
4b1453954179d21ded05479f3fe469851fd1b815 refs/heads/bitbucket-oauth2
06:05:10.528054 packet:clone< 
a1f4b96f2a521c3214cef9f4f54a8f15750fadc2 
refs/heads/break-out-core-urls-views
06:05:10.528267 packet:clone< 
9dfa0de18ec8c2a0f5a3e2dad0bb3aebf39b73b0 refs/heads/build-command-shebang
06:05:10.528421 packet:clone< 
e7a86d67695d0224629abdccf9cf4c732b7915c0 refs/heads/build-complete-signal
06:05:10.528638 packet:clone< 
a5be71f425379ac722c8f507355b070f46fc4d29 refs/heads/build-failure-email
06:05:10.528882 packet:clone< 
dd7db9e1f5524e14215ede8e58ecd21ebe3cd223 refs/heads/build-filtering
06:05:10.529066 packet:clone< 
5ee3f2ba081531c98f3a37220f8c4bc8f24e2b7b refs/heads/build-indexes
06:05:10.529219 packet:clone< 

Re: git error

[Johannes Schindelin]

Hi Ada,

On Fri, 5 May 2017, ada wrote:

> I used the command below,there's an error,please help me to solve it !
> Thank you ~
> git push

I am sorry to hear that you encountered a problem using Git.

Having said that, this description does not allow anybody to help you, as
not even the error message has been reproduced.

There are too many possible problems to make guessing a sensible options.

Please see https://stackoverflow.com/help/mcve how to craft a question in
a manner that gives anybody else a chance to assist you.

Ciao,
Johannes

Re: git error

[Kevin Daudt]

On Fri, May 05, 2017 at 10:52:13AM +0800, ada wrote:
> hi,
> 
> I used the command below,there's an error,please help me to solve it !
> Thank you ~
> git push
> 
> 
> Best Regards~
> ada Wang
> 

Hello Ada,

In order for people to be able to help you, you should provide more
details about your problem.

This typically includes what exact command you ran, what the output /
error message you got and any other relevant details.

Kind regards, Kevin.

[PATCH] doc: replace a couple of broken gmane links

[Ævar Arnfjörð Bjarmason]

Replace a couple of broken links to gmane with links to other
archives. See commit 54471fdcc3 ("README: replace gmane link with
public-inbox", 2016-12-15) for prior art.

With this change there's still 4 references left in the code:

$ git grep -E '(article|thread)\.gmane.org' -- |grep -v RelNotes|wc -l
4

I couldn't find alternative links for those.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

Perhaps someone else will have better luck with the other ones, which
are:

Documentation/git-bisect-lk2009.txt-1355-- [[[6]]] 
https://lwn.net/Articles/277872/[Jonathan Corbet. 'Bisection divides users and 
developers'. LWN.net.]
Documentation/git-bisect-lk2009.txt:1356:- [[[7]]] 
http://article.gmane.org/gmane.linux.scsi/36652/[Ingo Molnar. 'Re: BUG 
2.6.23-rc3 can't see sd partitions on Alpha'. Gmane.]
Documentation/git-bisect-lk2009.txt-1357-- [[[8]]] 
https://www.kernel.org/pub/software/scm/git/docs/git-bisect.html[Junio C Hamano 
and the git-list. 'git-bisect(1) Manual Page'. Linux Kernel Archives.]
--
git-rebase--interactive.sh-7-# The original idea comes from Eric W. 
Biederman, in
git-rebase--interactive.sh:8:# 
http://article.gmane.org/gmane.comp.version-control.git/22407
git-rebase--interactive.sh-9-#
--
t/t4038-diff-combined.sh-356-# Test for a bug reported at
t/t4038-diff-combined.sh:357:# 
http://thread.gmane.org/gmane.comp.version-control.git/224410
t/t4038-diff-combined.sh-358-# where a delete lines were missing from 
combined diff output when they
--
tree-walk.c-1077-* in future, see
tree-walk.c:1078:* 
http://thread.gmane.org/gmane.comp.version-control.git/163757/focus=163840
tree-walk.c-1079-*/

 Documentation/CodingGuidelines  | 2 +-
 Documentation/git-bisect-lk2009.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index a4191aa388..2248cf7324 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -24,7 +24,7 @@ code.  For Git in general, a few rough rules are:
 
"Once it _is_ in the tree, it's not really worth the patch noise to
go and fix it up."
-   Cf. http://article.gmane.org/gmane.linux.kernel/943020
+   Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html
 
 Make your code readable and sensible, and don't try to be clever.
 
diff --git a/Documentation/git-bisect-lk2009.txt 
b/Documentation/git-bisect-lk2009.txt
index 8ac75fcc25..b5d5e8b544 100644
--- a/Documentation/git-bisect-lk2009.txt
+++ b/Documentation/git-bisect-lk2009.txt
@@ -1350,7 +1350,7 @@ References
 - [[[1]]] 
https://www.nist.gov/sites/default/files/documents/director/planning/report02-3.pdf['The
 Economic Impacts of Inadequate Infratructure for Software Testing'.  Nist 
Planning Report 02-3], see Executive Summary and Chapter 8.
 - [[[2]]] http://www.oracle.com/technetwork/java/codeconvtoc-136057.html['Code 
Conventions for the Java Programming Language'. Sun Microsystems.]
 - [[[3]]] https://en.wikipedia.org/wiki/Software_maintenance['Software 
maintenance'. Wikipedia.]
-- [[[4]]] http://article.gmane.org/gmane.comp.version-control.git/45195/[Junio 
C Hamano. 'Automated bisect success story'. Gmane.]
+- [[[4]]] 
https://public-inbox.org/git/7vps5xsbwp.fsf...@assigned-by-dhcp.cox.net/[Junio 
C Hamano. 'Automated bisect success story'.]
 - [[[5]]] https://lwn.net/Articles/317154/[Christian Couder. 'Fully automated 
bisecting with "git bisect run"'. LWN.net.]
 - [[[6]]] https://lwn.net/Articles/277872/[Jonathan Corbet. 'Bisection divides 
users and developers'. LWN.net.]
 - [[[7]]] http://article.gmane.org/gmane.linux.scsi/36652/[Ingo Molnar. 'Re: 
BUG 2.6.23-rc3 can't see sd partitions on Alpha'. Gmane.]
-- 
2.11.0

Re: [RFC PATCH 01/10] Remove unneeded dependency on blob.h from blame

[Ævar Arnfjörð Bjarmason]

For commit message: This was originally added in commit acca687fa9
("git-pickaxe: retire pickaxe", 2006-11-08), but has never been
needed.

On Fri, May 5, 2017 at 7:27 AM, Jeff Smith  wrote:
> Signed-off-by: Jeff Smith 
> ---
>  builtin/blame.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 07506a3..42c56eb 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -8,7 +8,6 @@
>  #include "cache.h"
>  #include "refs.h"
>  #include "builtin.h"
> -#include "blob.h"
>  #include "commit.h"
>  #include "tag.h"
>  #include "tree-walk.h"
> --
> 2.9.3
>

[PATCH v2 8/7] fixup! compat/regex: update the gawk regex engine from upstream

[Ævar Arnfjörð Bjarmason]

---

On Fri, May 5, 2017 at 7:54 AM, Johannes Sixt  wrote:
> GNU C Library 2016? Is this GPL v3 code? that would be incompatible with GPL
> v2, I think.

No, it's all under LGPL 2.1, the glibc license has not changed since
2010.

As noted in patch 02 one amendmend to one of the LGPL 2.1 files from
glibc requires a GPLv3 header, this is a change made by Gawk in
importing it from glibc, but I removed that dependency.

But I forgot to subsequently git rm the relevant header file & amend
the monkeypatch. Here's a !fixup on top for that.

Junio: Just sending this one patch of v2, if you'd like to fetch the
whole thing it's import-2017-upstream-gawk-regex-engine-2 on
github.com/avar/git

 .../0001-Add-notice-at-top-of-copied-files.patch   |  15 --
 compat/regex/verify.h  | 286 -
 2 files changed, 301 deletions(-)
 delete mode 100644 compat/regex/verify.h

diff --git a/compat/regex/patches/0001-Add-notice-at-top-of-copied-files.patch 
b/compat/regex/patches/0001-Add-notice-at-top-of-copied-files.patch
index 4b4acc45ba..4f82185174 100644
--- a/compat/regex/patches/0001-Add-notice-at-top-of-copied-files.patch
+++ b/compat/regex/patches/0001-Add-notice-at-top-of-copied-files.patch
@@ -103,18 +103,3 @@ index c8f11e52e7..c79ff38b1c 100644
  /* Extended regular expression matching and search library.
 Copyright (C) 2002-2016 Free Software Foundation, Inc.
 This file is part of the GNU C Library.
-diff --git a/compat/regex/verify.h b/compat/regex/verify.h
-index 5c8381d290..e865af5298 100644
 a/compat/regex/verify.h
-+++ b/compat/regex/verify.h
-@@ -1,3 +1,10 @@
-+/*
-+ * This is git.git's copy of gawk.git's regex engine. Please see that
-+ * project for the latest version & to submit patches to this code,
-+ * and git.git's compat/regex/README for information on how git's copy
-+ * of this code is maintained.
-+ */
-+
- /* Compile-time assert-like macros.
- 
-Copyright (C) 2005-2006, 2009-2016 Free Software Foundation, Inc.
diff --git a/compat/regex/verify.h b/compat/regex/verify.h
deleted file mode 100644
index e865af5298..00
--- a/compat/regex/verify.h
+++ /dev/null
@@ -1,286 +0,0 @@
-/*
- * This is git.git's copy of gawk.git's regex engine. Please see that
- * project for the latest version & to submit patches to this code,
- * and git.git's compat/regex/README for information on how git's copy
- * of this code is maintained.
- */
-
-/* Compile-time assert-like macros.
-
-   Copyright (C) 2005-2006, 2009-2016 Free Software Foundation, Inc.
-
-   This program is free software: you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 3 of the License, or
-   (at your option) any later version.
-
-   This program is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU General Public License for more details.
-
-   You should have received a copy of the GNU General Public License
-   along with this program.  If not, see .  */
-
-/* Written by Paul Eggert, Bruno Haible, and Jim Meyering.  */
-
-#ifndef _GL_VERIFY_H
-#define _GL_VERIFY_H
-
-
-/* Define _GL_HAVE__STATIC_ASSERT to 1 if _Static_assert works as per C11.
-   This is supported by GCC 4.6.0 and later, in C mode, and its use
-   here generates easier-to-read diagnostics when verify (R) fails.
-
-   Define _GL_HAVE_STATIC_ASSERT to 1 if static_assert works as per C++11.
-   This will likely be supported by future GCC versions, in C++ mode.
-
-   Use this only with GCC.  If we were willing to slow 'configure'
-   down we could also use it with other compilers, but since this
-   affects only the quality of diagnostics, why bother?  */
-#if (4 < __GNUC__ + (6 <= __GNUC_MINOR__) \
- && (201112L <= __STDC_VERSION__  || !defined __STRICT_ANSI__) \
- && !defined __cplusplus)
-# define _GL_HAVE__STATIC_ASSERT 1
-#endif
-/* The condition (99 < __GNUC__) is temporary, until we know about the
-   first G++ release that supports static_assert.  */
-#if (99 < __GNUC__) && defined __cplusplus
-# define _GL_HAVE_STATIC_ASSERT 1
-#endif
-
-/* FreeBSD 9.1 , included by  and lots of other
-   system headers, defines a conflicting _Static_assert that is no
-   better than ours; override it.  */
-#ifndef _GL_HAVE_STATIC_ASSERT
-# include 
-# undef _Static_assert
-#endif
-
-/* Each of these macros verifies that its argument R is nonzero.  To
-   be portable, R should be an integer constant expression.  Unlike
-   assert (R), there is no run-time overhead.
-
-   If _Static_assert works, verify (R) uses it directly.  Similarly,
-   _GL_VERIFY_TRUE works by packaging a _Static_assert inside a struct
-   that is an operand of sizeof.
-
-   The code below uses several ideas for C++ compilers, and for C
-   compilers that do not