Re: [PATCH 3/3] sub-process: allocate argv on the heap

[Johannes Sixt]

Am 04.10.2017 um 06:59 schrieb Junio C Hamano:

Johannes Sixt  writes:


Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:

diff --git a/sub-process.c b/sub-process.c
index 6dde5062be..4680af8193 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct 
subprocess_entry *entry, co
   {
int err;
struct child_process *process;
-   const char *argv[] = { cmd, NULL };
+   const char **argv = xmalloc(2 * sizeof(char *));
+   argv[0] = cmd;
+   argv[1] = NULL;
entry->cmd = cmd;
process = >process;



Perhaps this should become

argv_array_push(>args, cmd);

so that there is no new memory leak?


Sounds like a good idea (if I am not grossly mistaken as to what is
being suggested).

Here is what I am planning to queue.

-- >8 --
From: Johannes Sixt 
Date: Tue, 3 Oct 2017 22:24:57 +0200
Subject: [PATCH] sub-process: use child_process.args instead of 
child_process.argv

Currently the argv is only allocated on the stack, and then assigned to
process->argv.  When the start_subprocess function goes out of scope,
the local argv variable is eliminated from the stack, but the pointer is
still kept around in process->argv.

Much later when we try to access the same process->argv in
finish_command, this leads us to access a memory location that no longer
contains what we want.  As argv0 is only used for printing errors, this
is not easily noticed in normal git operations.  However when running
t0021-conversion.sh through valgrind, valgrind rightfully complains:

==21024== Invalid read of size 8
==21024==at 0x2ACF64: finish_command (run-command.c:869)
==21024==by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)
==21024==by 0x2AB41E: cleanup_children (run-command.c:45)
==21024==by 0x2AB526: cleanup_children_on_exit (run-command.c:81)
==21024==by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)
==21024==by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)
==21024==by 0x11A9EF: handle_builtin (git.c:550)
==21024==by 0x11ABCC: run_argv (git.c:602)
==21024==by 0x11AD8E: cmd_main (git.c:679)
==21024==by 0x1BF125: main (common-main.c:43)
==21024==  Address 0x1ffeffec00 is on thread 1's stack
==21024==  1504 bytes below stack pointer
==21024==

These days, the child_process structure has its own args array, and
the standard way to set up its argv[] is to use that one, instead of
assigning to process->argv to point at an array that is outside.
Use that facility automatically fixes this issue.

Reported-by: Thomas Gummerer 
Signed-off-by: Johannes Sixt 
Signed-off-by: Junio C Hamano 
---
  sub-process.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sub-process.c b/sub-process.c
index fcc4832c14..648b3a3943 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -74,13 +74,12 @@ int subprocess_start(struct hashmap *hashmap, struct 
subprocess_entry *entry, co
  {
int err;
struct child_process *process;
-   const char *argv[] = { cmd, NULL };
  
  	entry->cmd = cmd;

process = >process;
  
  	child_process_init(process);

-   process->argv = argv;
+   argv_array_push(>args, cmd);
process->use_shell = 1;
process->in = -1;
process->out = -1;



Thank you very much! That looks good. Just to be on the safe side:

Signed-off-by: Johannes Sixt 

-- Hannes

Re: [PATCH 3/3] sub-process: allocate argv on the heap

[Jeff King]

On Wed, Oct 04, 2017 at 01:59:31PM +0900, Junio C Hamano wrote:

> > Perhaps this should become
> >
> > argv_array_push(>args, cmd);
> >
> > so that there is no new memory leak?
> 
> Sounds like a good idea (if I am not grossly mistaken as to what is
> being suggested).
> 
> Here is what I am planning to queue.
> 
> -- >8 --
> From: Johannes Sixt 
> Date: Tue, 3 Oct 2017 22:24:57 +0200
> Subject: [PATCH] sub-process: use child_process.args instead of 
> child_process.argv

This looks good (and is exactly the type of case for which I added
"args" to the child_process in the first place). The commit message
is well-explained and the patch looks obviously correct.

-Peff

Re: [PATCH v2] strbuf doc: reuse after strbuf_release is fine

[Jeff King]

On Tue, Oct 03, 2017 at 07:39:54PM -0700, Jonathan Nieder wrote:

> > I think it's actually OK to use the string buffer after this function.
> > It's just an empty string.
> >
> > Perhaps we should be more explicit: this releases any resources and
> > resets to a pristine, empty state. I suspect strbuf_detach() probably
> > should make the same claim.
> 
> Like this?

Yes, perfect. Thanks!

-Peff

Re: [PATCH 2/3] http-push: fix construction of hex value from path

[Jeff King]

On Wed, Oct 04, 2017 at 02:20:05PM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Jeff King  writes:
> >
> >>> Moreover, this is in the webdav-based "dumb http" push code path,
> >>> which I do not trust much at all.  I wonder if we could retire it
> >>> completely (or at least provide an option to turn it off).
> >>
> >> I would really like that, too. It has been the cause of a lot of pain
> >> when working with the smart code, and I am not at all surprised to find
> >> a bug of this magnitude lurking in it. I'd _hoped_ this could show that
> >> the system has been unusably broken for years, which would give us
> >> confidence to turn it off. :) But per your paragraph above, people could
> >> very easily still have been happily using it in the meantime.
> >
> > Same here.  Perhaps we should deliberately and silently break it and
> > see who screams?
> 
> Hopefully it should be obvious but just for people with unreasonable
> expectations, I should clarify that the above needs a smiley ;-).

Yes, I was surprised to see it without one. :)

More seriously, is there any interest in marking it as deprecated in the
release notes and issuing a warning when it's used for a few cycles?

-Peff

Re: [PATCH 1/3] path.c: fix uninitialized memory access

[Jeff King]

On Wed, Oct 04, 2017 at 01:47:29PM +0900, Junio C Hamano wrote:

> Jonathan Nieder  writes:
> 
> > Jeff King wrote:
> >> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:
> >
> >>> In other words, an alternative fix would be
> >>> 
> >>>   if (*path == '.' && path[1] == '/') {
> >>>   ...
> >>>   }
> >>> 
> >>> which would not require passing in 'len' or switching to index-based
> >>> arithmetic.  I think I prefer it.  What do you think?
> >>
> >> Yes, I think that approach is much nicer. I think you could even use
> >> skip_prefix. Unfortunately you have to play a few games with const-ness,
> >> but I think the resulting signature for cleanup_path() is an
> >> improvement:
> 
> To tie the loose end, here is what I'll queue.

Thank you, I was planning to get to this later tonight, but now I don't
have to. :)

FWIW, I wondered if we could get rid of the extra cast by switching
cleanup_path() to return an offset rather than a string (which also
makes its interface more foolproof, since it's clear that the return
value is a subset of the original string).

But it ends up being a bit clunky I think (patch below for reference).

I guess it's possible that `cleanup_path` could learn to do other
cleanup, too (e.g., to clean up doubled slashes in the middle of the
path), in which case it really would want a non-const buffer. But since
it has remained unchanged since 26c8a533af (Add "mkpath()" helper
function, 2005-07-08), I'm happy to assume it will remain so for another
12 years.

All of which is to say:

> -- >8 --
> From: Jeff King 
> Date: Tue, 3 Oct 2017 19:30:40 -0400
> Subject: [PATCH] path.c: fix uninitialized memory access

Looks good to me.

-Peff

-- >8 --
diff --git a/path.c b/path.c
index 5aa9244eb2..eaeb9d9a17 100644
--- a/path.c
+++ b/path.c
@@ -34,22 +34,24 @@ static struct strbuf *get_pathname(void)
return sb;
 }
 
-static char *cleanup_path(char *path)
+static size_t cleanup_path(const char *path)
 {
-   /* Clean it up */
-   if (!memcmp(path, "./", 2)) {
-   path += 2;
-   while (*path == '/')
-   path++;
-   }
-   return path;
+   const char *clean;
+
+   if (!skip_prefix(path, "./", ))
+   return 0;
+
+   while (*clean == '/')
+   clean++;
+
+   return clean - path;
 }
 
 static void strbuf_cleanup_path(struct strbuf *sb)
 {
-   char *path = cleanup_path(sb->buf);
-   if (path > sb->buf)
-   strbuf_remove(sb, 0, path - sb->buf);
+   size_t s = cleanup_path(sb->buf);
+   if (s)
+   strbuf_remove(sb, 0, s);
 }
 
 char *mksnpath(char *buf, size_t n, const char *fmt, ...)
@@ -64,7 +66,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
strlcpy(buf, bad_path, n);
return buf;
}
-   return cleanup_path(buf);
+   return buf + cleanup_path(buf);
 }
 
 static int dir_prefix(const char *buf, const char *dir)

Re: [PATCH 2/3] http-push: fix construction of hex value from path

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>>> Moreover, this is in the webdav-based "dumb http" push code path,
>>> which I do not trust much at all.  I wonder if we could retire it
>>> completely (or at least provide an option to turn it off).
>>
>> I would really like that, too. It has been the cause of a lot of pain
>> when working with the smart code, and I am not at all surprised to find
>> a bug of this magnitude lurking in it. I'd _hoped_ this could show that
>> the system has been unusably broken for years, which would give us
>> confidence to turn it off. :) But per your paragraph above, people could
>> very easily still have been happily using it in the meantime.
>
> Same here.  Perhaps we should deliberately and silently break it and
> see who screams?

Hopefully it should be obvious but just for people with unreasonable
expectations, I should clarify that the above needs a smiley ;-).

Re: [PATCH v2] strbuf doc: reuse after strbuf_release is fine

[Junio C Hamano]

Jonathan Nieder  writes:

> strbuf_release leaves the strbuf in a valid, initialized state, so
> there is no need to call strbuf_init after it.
>
> Moreover, this is not likely to change in the future: strbuf_release
> leaving the strbuf in a valid state has been easy to maintain and has
> been very helpful for Git's robustness and simplicity (e.g.,
> preventing use-after-free vulnerabilities).
>
> Document the semantics so the next generation of Git developers can
> become familiar with them without reading the implementation.  It is
> still not advisable to call strbuf_release too often because it is
> wasteful, so add a note pointing to strbuf_reset for that.
>
> The same semantics apply to strbuf_detach.  Add a similar note to its
> docstring to make that clear.
>
> Improved-by: Jeff King 
> Signed-off-by: Jonathan Nieder 
> ---
> Jeff King wrote:
>
>> I think it's actually OK to use the string buffer after this function.
>> It's just an empty string.
>>
>> Perhaps we should be more explicit: this releases any resources and
>> resets to a pristine, empty state. I suspect strbuf_detach() probably
>> should make the same claim.
>
> Like this?

Looks good to me.


>
> Thanks,
> Jonathan
>
>  strbuf.h | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index 7496cb8ec5..249df86711 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -82,8 +82,12 @@ extern char strbuf_slopbuf[];
>  extern void strbuf_init(struct strbuf *, size_t);
>  
>  /**
> - * Release a string buffer and the memory it used. You should not use the
> - * string buffer after using this function, unless you initialize it again.
> + * Release a string buffer and the memory it used. After this call, the
> + * strbuf points to an empty string that does not need to be free()ed, as
> + * if it had been set to `STRBUF_INIT` and never modified.
> + *
> + * To clear a strbuf in preparation for further use without the overhead
> + * of free()ing and malloc()ing again, use strbuf_reset() instead.
>   */
>  extern void strbuf_release(struct strbuf *);
>  
> @@ -91,6 +95,9 @@ extern void strbuf_release(struct strbuf *);
>   * Detach the string from the strbuf and returns it; you now own the
>   * storage the string occupies and it is your responsibility from then on
>   * to release it with `free(3)` when you are done with it.
> + *
> + * The strbuf that previously held the string is reset to `STRBUF_INIT` so
> + * it can be reused after calling this function.
>   */
>  extern char *strbuf_detach(struct strbuf *, size_t *);

Re: [PATCH 3/3] sub-process: allocate argv on the heap

[Junio C Hamano]

Johannes Sixt  writes:

> Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:
>> diff --git a/sub-process.c b/sub-process.c
>> index 6dde5062be..4680af8193 100644
>> --- a/sub-process.c
>> +++ b/sub-process.c
>> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct 
>> subprocess_entry *entry, co
>>   {
>>  int err;
>>  struct child_process *process;
>> -const char *argv[] = { cmd, NULL };
>> +const char **argv = xmalloc(2 * sizeof(char *));
>> +argv[0] = cmd;
>> +argv[1] = NULL;
>>  entry->cmd = cmd;
>>  process = >process;
>>
>
> Perhaps this should become
>
>   argv_array_push(>args, cmd);
>
> so that there is no new memory leak?

Sounds like a good idea (if I am not grossly mistaken as to what is
being suggested).

Here is what I am planning to queue.

-- >8 --
From: Johannes Sixt 
Date: Tue, 3 Oct 2017 22:24:57 +0200
Subject: [PATCH] sub-process: use child_process.args instead of 
child_process.argv

Currently the argv is only allocated on the stack, and then assigned to
process->argv.  When the start_subprocess function goes out of scope,
the local argv variable is eliminated from the stack, but the pointer is
still kept around in process->argv.

Much later when we try to access the same process->argv in
finish_command, this leads us to access a memory location that no longer
contains what we want.  As argv0 is only used for printing errors, this
is not easily noticed in normal git operations.  However when running
t0021-conversion.sh through valgrind, valgrind rightfully complains:

==21024== Invalid read of size 8
==21024==at 0x2ACF64: finish_command (run-command.c:869)
==21024==by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)
==21024==by 0x2AB41E: cleanup_children (run-command.c:45)
==21024==by 0x2AB526: cleanup_children_on_exit (run-command.c:81)
==21024==by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)
==21024==by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)
==21024==by 0x11A9EF: handle_builtin (git.c:550)
==21024==by 0x11ABCC: run_argv (git.c:602)
==21024==by 0x11AD8E: cmd_main (git.c:679)
==21024==by 0x1BF125: main (common-main.c:43)
==21024==  Address 0x1ffeffec00 is on thread 1's stack
==21024==  1504 bytes below stack pointer
==21024==

These days, the child_process structure has its own args array, and
the standard way to set up its argv[] is to use that one, instead of
assigning to process->argv to point at an array that is outside.
Use that facility automatically fixes this issue.

Reported-by: Thomas Gummerer 
Signed-off-by: Johannes Sixt 
Signed-off-by: Junio C Hamano 
---
 sub-process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sub-process.c b/sub-process.c
index fcc4832c14..648b3a3943 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -74,13 +74,12 @@ int subprocess_start(struct hashmap *hashmap, struct 
subprocess_entry *entry, co
 {
int err;
struct child_process *process;
-   const char *argv[] = { cmd, NULL };
 
entry->cmd = cmd;
process = >process;
 
child_process_init(process);
-   process->argv = argv;
+   argv_array_push(>args, cmd);
process->use_shell = 1;
process->in = -1;
process->out = -1;
-- 
2.14.2-889-gd2948f6aa6

Re: [PATCH 2/3] http-push: fix construction of hex value from path

[Junio C Hamano]

Jeff King  writes:

>> Moreover, this is in the webdav-based "dumb http" push code path,
>> which I do not trust much at all.  I wonder if we could retire it
>> completely (or at least provide an option to turn it off).
>
> I would really like that, too. It has been the cause of a lot of pain
> when working with the smart code, and I am not at all surprised to find
> a bug of this magnitude lurking in it. I'd _hoped_ this could show that
> the system has been unusably broken for years, which would give us
> confidence to turn it off. :) But per your paragraph above, people could
> very easily still have been happily using it in the meantime.

Same here.  Perhaps we should deliberately and silently break it and
see who screams?

Re: [PATCH 1/3] path.c: fix uninitialized memory access

[Junio C Hamano]

Jonathan Nieder  writes:

> Jeff King wrote:
>> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:
>
>>> In other words, an alternative fix would be
>>> 
>>> if (*path == '.' && path[1] == '/') {
>>> ...
>>> }
>>> 
>>> which would not require passing in 'len' or switching to index-based
>>> arithmetic.  I think I prefer it.  What do you think?
>>
>> Yes, I think that approach is much nicer. I think you could even use
>> skip_prefix. Unfortunately you have to play a few games with const-ness,
>> but I think the resulting signature for cleanup_path() is an
>> improvement:

To tie the loose end, here is what I'll queue.

-- >8 --
From: Jeff King 
Date: Tue, 3 Oct 2017 19:30:40 -0400
Subject: [PATCH] path.c: fix uninitialized memory access

In cleanup_path we're passing in a char array, run a memcmp on it, and
run through it without ever checking if something is in the array in the
first place.  This can lead us to access uninitialized memory, for
example in t5541-http-push-smart.sh test 7, when run under valgrind:

==4423== Conditional jump or move depends on uninitialised value(s)
==4423==at 0x242FA9: cleanup_path (path.c:35)
==4423==by 0x242FA9: mkpath (path.c:456)
==4423==by 0x256CC7: refname_match (refs.c:364)
==4423==by 0x26C181: count_refspec_match (remote.c:1015)
==4423==by 0x26C181: match_explicit_lhs (remote.c:1126)
==4423==by 0x26C181: check_push_refs (remote.c:1409)
==4423==by 0x2ABB4D: transport_push (transport.c:870)
==4423==by 0x186703: push_with_options (push.c:332)
==4423==by 0x18746D: do_push (push.c:409)
==4423==by 0x18746D: cmd_push (push.c:566)
==4423==by 0x1183E0: run_builtin (git.c:352)
==4423==by 0x11973E: handle_builtin (git.c:539)
==4423==by 0x11973E: run_argv (git.c:593)
==4423==by 0x11973E: main (git.c:698)
==4423==  Uninitialised value was created by a heap allocation
==4423==at 0x4C2CD8F: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4423==by 0x4C2F195: realloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4423==by 0x2C196B: xrealloc (wrapper.c:137)
==4423==by 0x29A30B: strbuf_grow (strbuf.c:66)
==4423==by 0x29A30B: strbuf_vaddf (strbuf.c:277)
==4423==by 0x242F9F: mkpath (path.c:454)
==4423==by 0x256CC7: refname_match (refs.c:364)
==4423==by 0x26C181: count_refspec_match (remote.c:1015)
==4423==by 0x26C181: match_explicit_lhs (remote.c:1126)
==4423==by 0x26C181: check_push_refs (remote.c:1409)
==4423==by 0x2ABB4D: transport_push (transport.c:870)
==4423==by 0x186703: push_with_options (push.c:332)
==4423==by 0x18746D: do_push (push.c:409)
==4423==by 0x18746D: cmd_push (push.c:566)
==4423==by 0x1183E0: run_builtin (git.c:352)
==4423==by 0x11973E: handle_builtin (git.c:539)
==4423==by 0x11973E: run_argv (git.c:593)
==4423==by 0x11973E: main (git.c:698)
==4423==

Avoid this by using skip_prefix(), which knows not to go beyond the
end of the string.

Reported-by: Thomas Gummerer 
Signed-off-by: Jeff King 
Reviewed-by: Jonathan Nieder 
Signed-off-by: Junio C Hamano 
---
 path.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index e50d2befcf..2fecf854fe 100644
--- a/path.c
+++ b/path.c
@@ -33,11 +33,10 @@ static struct strbuf *get_pathname(void)
return sb;
 }
 
-static char *cleanup_path(char *path)
+static const char *cleanup_path(const char *path)
 {
/* Clean it up */
-   if (!memcmp(path, "./", 2)) {
-   path += 2;
+   if (skip_prefix(path, "./", )) {
while (*path == '/')
path++;
}
@@ -46,7 +45,7 @@ static char *cleanup_path(char *path)
 
 static void strbuf_cleanup_path(struct strbuf *sb)
 {
-   char *path = cleanup_path(sb->buf);
+   const char *path = cleanup_path(sb->buf);
if (path > sb->buf)
strbuf_remove(sb, 0, path - sb->buf);
 }
@@ -63,7 +62,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
strlcpy(buf, bad_path, n);
return buf;
}
-   return cleanup_path(buf);
+   return (char *)cleanup_path(buf);
 }
 
 static int dir_prefix(const char *buf, const char *dir)
-- 
2.14.2-889-gd2948f6aa6

Re: [PATCH] run-command.c: add hint when hook is not executable

[Junio C Hamano]

Damien  writes:

> ---

Please explain why this change makes sense to those who find this
commit in "git log" output six months down the line, without having
read the original thread that motivated you to add this feature
above this line with three dashes.  Use your full name on the From:
header of your mail (or alternatively, you can use an in-body "From:"
to override what your MUA places there) and add sign-off with the
same name (cf. Documentation/SubmittingPatches).

>  Documentation/config.txt   | 2 ++
>  advice.c   | 2 ++
>  advice.h   | 1 +
>  contrib/completion/git-completion.bash | 1 +
>  run-command.c  | 4 
>  5 files changed, 10 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1ac0ae6adb046..83b1884cf22fc 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -351,6 +351,8 @@ advice.*::
>   addEmbeddedRepo::
>   Advice on what to do when you've accidentally added one
>   git repo inside of another.
> + hookNotExecutable::
> + Shown when an hook is there but not executable.
>  --

"Shown when" does not tell readers what is shown; many of the other
entries in this list does so, and it is helpful to choose from the
list when a user encounters one of these advice messages and wants
to squelch it.

> diff --git a/advice.c b/advice.c
> diff --git a/advice.h b/advice.h
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash

The changes to these files looked good.

> diff --git a/run-command.c b/run-command.c
> index b5e6eb37c0eb3..95d93a23bf87e 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1174,6 +1174,10 @@ const char *find_hook(const char *name)
>   if (access(path.buf, X_OK) >= 0)
>   return path.buf;
>  #endif
> + if (advice_hook_not_executable) {
> + advise(_("The '%s' hook was ignored because it's not 
> set as executable."
> + "\nYou can disable this warning with `git 
> config advice.hookNotExecutable false`"), name);
> + }
>   return NULL;

As to the string constant, it is somewhat strange to see it is
chomped into two and the LF in the middle is given to the latter
half.  If you are going to chomp anyway, perhaps you'd want to chomp
it further to avoid making the source line too long.

But more importantly, is this checking the right thing?  Before the
pre-context of this hunk is:

if (access(path.buf, X_OK) < 0) {

so we know access(X_OK) failed.  And we didn't have to care how/why
it failed because the only thing we did was to return NULL when we
decide there is no executable hook.

But for the purpose of your patch, you now do care.  access(X_OK)
may have failed with EACCESS (i.e. you have no permission to run the
script), in which case your new advise() call may make sense.  But
it may have failed with ENOENT or ENOTDIR.  And your new advise()
call is a disaster, if the user didn't even have such a hook.

Writing a test would have helped notice this, I would think.  You'd
need at least the following variations to cover possibilities:

 - Without the advise.* configuration, install an executable hook
   for a command and try to run the command.  Make sure you do not
   see any advise message shown.

 - Drop the executable bit from the hook from the above and run the
   same command.  Make sure you do see the advise message.  You'd
   probably need to protect this test piece with POSIXPERM
   prerequisite.

 - Set advise.* configuration to squelch and run the same command.
   Make sure you do not see the advise message.

 - Remove advise.* configuration and the hook, and run the same
   command.  Make sure you do not see the advise message.

Re: [PATCH v2] branch: change the error messages to be more meaningful

[Junio C Hamano]

Kaartic Sivaraam  writes:

> I interpreted the "not .. too bad" as a "it makes little sense". So,
> pinged the thread.

Thanks.  I think what the patch does (sort of) makes sense.  

It is a bit dissapointing that we do not need to touch tests, as it
indicates that the logic to diagnose extra arguments as an error has
no coverage.

Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Junio C Hamano]

Ramsay Jones  writes:

> On 03/10/17 04:51, Junio C Hamano wrote:
>> 
>> It seems that Pranit needs a bit more work to take known fixes from
>> your efforts and we should wait for the series to be rerolled?
> 
> This series is just the first few patches from the original 28/29
> patch series; in particular, patches 1-5 and 8 of that series.
> ...
> So, the major differences and bug fixes are in later patches.

OK.  So these are good to go separately (as prerequistes for the
follow-on work)--thanks for clarification.

Re: new contributors presentation

[Junio C Hamano]

Nathan PAYRE  writes:

> Hi all,
> me and my two other partner (Daniel and Timothee) have make the choice
> to contribute to gitHub for a university project supervised by Mattieu
> Moy.

First things first.  I suspect that you are trying to contribute to
the Git project (GitHub is totally a different animal, even though
they benefit greatly from our presence, and we theirs).

And if you are dipping your toes to the Git project's development
community, then, "Welcome!" ;-).

> The principal project is to improve the git-send-email function, for
> example we will try to implement the possibility to answer to a email
> by keeping the recipient list or quote properly the email body!
> Do you think that it's will be usefull ?

I do not know about others, but I cannot quite tell what you are
propsing from that three-line description to tell if it would be
useful or not.  Let's wait to hear more from you guys.

Re: What means "git config bla ~/"?

[Junio C Hamano]

Matthieu Moy  writes:

> "Junio C Hamano"  writes:
>
>> Jonathan Nieder  writes:
>> 
 what's with that "git config bla ~/"? is this some config keyword
 or something?
>>> ...
>>  git config section.var ~/some/thing
>> ...
>
> I prefer your "section.var" proposal above. I think people who really 
> understand shell quoting do not need the explanations, and others probably 
> need the example.
>
> While we're there, the formatting is also wrong ('' quoting, while we 
> normally use `` quoting for shell commands).
>
> Sounds like a nice microproject for my students :-). A patch should follow 
> soon.

OK, thanks.  Lest we forget, this is #leftoverbits for now.

Re: [PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL

[Junio C Hamano]

René Scharfe  writes:

> Am 03.10.2017 um 14:51 schrieb René Scharfe:
>> Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:
>>> Furthermore, fsck.c:fsck_walk_tree() does the same "immediately
>>> reference the object member in lookup_blob()'s and lookup_tree()'s
>>> return value" thing.  I think those should receive the same treatment
>>> as well.
>> 
>> Hmm, are put_object_name() and all the walk() implementations ready for
>> a NULL object handed to them?  Or would we rather need to error out
>> right there?
> How about this?
>
> -- >8 --
> lookup_blob() and lookup_tree() can return NULL if they find an object
> of an unexpected type.  Error out of fsck_walk_tree() in that case, like
> we do when encountering a bad file mode.  An error message is already
> shown by object_as_type(), which gets called by the lookup functions.

The result from options->walk() is checked, and among the callbacks
that are assigned to the .walk field:

 - mark_object() in builtin/fsck.c gives its own error message to diagnose 
broken link
   and returns 1;

 - mark_used() in builtin/fsck.c silently returns 1;

 - mark_link() in builtin/index-pack.c does the same; and

 - check_object() in builtin/unpack-objects.c does the same,

when they see a NULL object.

This patch may avoid the "unexpected behaviour" coming from
expecting that &((struct tree *)NULL)->object == NULL the current
code does, but it also changes the behaviour.  The loop used to
diagnose a fishy entry in the tree we are walking, and kept checking
the remaining entries in the tree.  You now immediately return, not
seeing if the later entries in the tree are good and losing objects
that are referenced by these entries as dangling.

I am not sure if this is a good change.  I suspect that the "bad
mode" handling should be made less severe instead.

>
> Signed-off-by: Rene Scharfe 
> ---
>  fsck.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 2ad00fc4d0..561a13ac27 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -358,14 +358,20 @@ static int fsck_walk_tree(struct tree *tree, void 
> *data, struct fsck_options *op
>   continue;
>  
>   if (S_ISDIR(entry.mode)) {
> - obj = _tree(entry.oid)->object;
> + struct tree *tree = lookup_tree(entry.oid);
> + if (!tree)
> + return -1;
> + obj = >object;
>   if (name)
>   put_object_name(options, obj, "%s%s/", name,
>   entry.path);
>   result = options->walk(obj, OBJ_TREE, data, options);
>   }
>   else if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {
> - obj = _blob(entry.oid)->object;
> + struct blob *blob = lookup_blob(entry.oid);
> + if (!blob)
> + return -1;
> + obj = >object;
>   if (name)
>   put_object_name(options, obj, "%s%s", name,
>   entry.path);

Re: Updated to v2.14.2 on macOS; git add --patch broken

[Junio C Hamano]

Jeff King  writes:

> Here's what I came up with on the "color.ui=always is nonsense that we
> should not offer" front. The number of patches may be a little daunting,
> but most of them are just removing cases of "git -c color.ui=always"
> from the tests.
>
>   [01/12]: test-terminal: set TERM=vt100
>   [02/12]: t4015: prefer --color to -c color.diff=always
>   [03/12]: t4015: use --color with --color-moved
>   [04/12]: t3701: use test-terminal to collect color output
>   [05/12]: t7508: use test_terminal for color output
>   [06/12]: t7502: use diff.noprefix for --verbose test
>   [07/12]: t6006: drop "always" color config tests
>   [08/12]: t3203: drop "always" color test
>   [09/12]: t7301: use test_terminal to check color
>   [10/12]: t3205: use --color instead of color.branch=always
>   [11/12]: provide --color option for all ref-filter users
>   [12/12]: color: make "always" the same as "auto" in config

I'm shuffling these so that everything except 03/12 and 09/12 goes
on top of jk/ref-filter-colors (to be merged later for v2.14.x) to
create jk/ui-color-always-to-auto-maint branch.

Another branch jk/ui-color-always-to-auto would fork from 'master'
and have 03/12 and 09/12 (with a tweak to use vt100 explicitly, as
we lack 01/12 at that point) applied, and then merge the above one.
And then queuing another one that drops "env TERM=vt100" tweak added
to 09/12 would bring us to the same state as applying your 12 patches
directly on top.  That will cook in 'next' down to 'master' to make
sure we do not regress in 2.15.

Thanks.

[PATCH v2] strbuf doc: reuse after strbuf_release is fine

[Jonathan Nieder]

strbuf_release leaves the strbuf in a valid, initialized state, so
there is no need to call strbuf_init after it.

Moreover, this is not likely to change in the future: strbuf_release
leaving the strbuf in a valid state has been easy to maintain and has
been very helpful for Git's robustness and simplicity (e.g.,
preventing use-after-free vulnerabilities).

Document the semantics so the next generation of Git developers can
become familiar with them without reading the implementation.  It is
still not advisable to call strbuf_release too often because it is
wasteful, so add a note pointing to strbuf_reset for that.

The same semantics apply to strbuf_detach.  Add a similar note to its
docstring to make that clear.

Improved-by: Jeff King 
Signed-off-by: Jonathan Nieder 
---
Jeff King wrote:

> I think it's actually OK to use the string buffer after this function.
> It's just an empty string.
>
> Perhaps we should be more explicit: this releases any resources and
> resets to a pristine, empty state. I suspect strbuf_detach() probably
> should make the same claim.

Like this?

Thanks,
Jonathan

 strbuf.h | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 7496cb8ec5..249df86711 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -82,8 +82,12 @@ extern char strbuf_slopbuf[];
 extern void strbuf_init(struct strbuf *, size_t);
 
 /**
- * Release a string buffer and the memory it used. You should not use the
- * string buffer after using this function, unless you initialize it again.
+ * Release a string buffer and the memory it used. After this call, the
+ * strbuf points to an empty string that does not need to be free()ed, as
+ * if it had been set to `STRBUF_INIT` and never modified.
+ *
+ * To clear a strbuf in preparation for further use without the overhead
+ * of free()ing and malloc()ing again, use strbuf_reset() instead.
  */
 extern void strbuf_release(struct strbuf *);
 
@@ -91,6 +95,9 @@ extern void strbuf_release(struct strbuf *);
  * Detach the string from the strbuf and returns it; you now own the
  * storage the string occupies and it is your responsibility from then on
  * to release it with `free(3)` when you are done with it.
+ *
+ * The strbuf that previously held the string is reset to `STRBUF_INIT` so
+ * it can be reused after calling this function.
  */
 extern char *strbuf_detach(struct strbuf *, size_t *);
 
-- 
2.14.2.920.gcf0c67979c

Re: Git for Windows: mingw.c's strange usage of creation time as ctime?

[Jonathan Nieder]

+git-for-windows
Hi Marc,

Marc Strapetz wrote:

> compat/mingw.c assigns the Windows file creation time [1] to Git's
> ctime fields at line 591 and at line 705:
>
> buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
>
> ftCreationTime > ftLastWriteTime is actually possible after copying
> a file, so it makes sense to include this timestamp somehow in the
> Index, but I think it would be better to use the maximum of
> ftLastWriteTime and ftCreationTime here which is less confusing and
> closer to Unix's ctime:
>
> buf->st_ctime = max(buf->st_mtime,
> filetime_to_time_t(&(fdata.ftCreationTime));

Can you say more about the practical effect?  Is this causing a bug in
practice, is it a bug waiting to happen, or is it making the code
difficult to understand without any ill effect expected at run time?
(Any of those three is a valuable kind of feedback to offer --- I'm
just trying to figure out which one you mean.)

By the way, do you have core.trustctime set to true or false?

Thanks,
Jonathan

> -Marc
>
> [1] 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365739(v=vs.85).aspx

Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Ramsay Jones]

On 03/10/17 04:51, Junio C Hamano wrote:
> Ramsay Jones  writes:
> 
>> On 02/10/17 14:44, Pranit Bauva wrote:
>> [snip]
>>> ...
>> Yes, I also meant to tidy that up by removing some, now
>> redundant, initialisation later in that function.
>>
>> Note, that wasn't the only bug! (I have probably forgotten
>> some of them, but look at 964f4e2b0, for example).
> 
> It seems that Pranit needs a bit more work to take known fixes from
> your efforts and we should wait for the series to be rerolled?

This series is just the first few patches from the original 28/29
patch series; in particular, patches 1-5 and 8 of that series.
If I compare just the first 5 patches, then the differences are
small and (maybe) not worth a re-roll. For some reason, I changed
the message passed to the delete_refs() call from "bisect: remove"
to "bisect: clean", we both added "terms" to the 'builtin command'
list but in different positions and some calls to die() have
been replaced with 'return error(...)'.

Viz:

  $ git log --oneline v2.14.0..bisect~34
  d8ce077c8 t6030: explicitly test for bisection cleanup
  1fd8e44a1 bisect--helper: `bisect_clean_state` shell function in C
  5aa9d18eb bisect--helper: `write_terms` shell function in C
  1b94b2ff1 bisect: rewrite `check_term_format` shell function in C
  a97a96ccf bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  $ 
  $ git log --oneline v2.14.0..pranit~1
  6012123c7 t6030: explicitly test for bisection cleanup
  7c945c391 bisect--helper: `bisect_clean_state` shell function in C
  9240c3962 bisect--helper: `write_terms` shell function in C
  ba496589c bisect--helper: rewrite `check_term_format` shell function in C
  f1563a33f bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  $ 

Note that the subject line of patch #2 has also been corrected
in this new series (bisect: -> bisect--helper:). I haven't
compared the commit messages.

  $ git diff bisect~34 pranit~1
  diff --git a/bisect.c b/bisect.c
  index b19311ca7..2838d672d 100644
  --- a/bisect.c
  +++ b/bisect.c
  @@ -1066,7 +1066,7 @@ int bisect_clean_state(void)
struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
for_each_ref_in("refs/bisect", mark_for_removal, (void *) 
_for_removal);
string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
  - result = delete_refs("bisect: clean", _for_removal, REF_NODEREF);
  + result = delete_refs("bisect: remove", _for_removal, REF_NODEREF);
refs_for_removal.strdup_strings = 1;
string_list_clear(_for_removal, 0);
unlink_or_warn(git_path_bisect_expected_rev());
  diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
  index 2af024f60..0f4d4e41c 100644
  --- a/builtin/bisect--helper.c
  +++ b/builtin/bisect--helper.c
  @@ -43,8 +43,8 @@ static int check_term_format(const char *term, const char 
*orig_term)
if (res)
return error(_("'%s' is not a valid term"), term);
   
  - if (one_of(term, "help", "start", "terms", "skip", "next", "reset",
  - "visualize", "replay", "log", "run", NULL))
  + if (one_of(term, "help", "start", "skip", "next", "reset",
  + "visualize", "replay", "log", "run", "terms", NULL))
return error(_("can't use the builtin command '%s' as a term"), 
term);
   
/*
  @@ -111,14 +111,14 @@ int cmd_bisect__helper(int argc, const char **argv, 
const char *prefix)
return bisect_next_all(prefix, no_checkout);
case WRITE_TERMS:
if (argc != 2)
  - die(_("--write-terms requires two arguments"));
  + return error(_("--write-terms requires two arguments"));
return write_terms(argv[0], argv[1]);
case BISECT_CLEAN_STATE:
if (argc != 0)
  - die(_("--bisect-clean-state requires no arguments"));
  + return error(_("--bisect-clean-state requires no 
arguments"));
return bisect_clean_state();
default:
  - die("BUG: unknown subcommand '%d'", cmdmode);
  + return error("BUG: unknown subcommand '%d'", cmdmode);
}
return 0;
   }
  diff --git a/git-bisect.sh b/git-bisect.sh
  index f1202dfb4..045830c39 100755
  --- a/git-bisect.sh
  +++ b/git-bisect.sh
  @@ -209,7 +209,7 @@ bisect_start() {
eval "$eval true" &&
if test $must_write_terms -eq 1
then
  - git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
  + git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || 
exit
fi &&
echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
#
  $ 

So, the major differences and bug fixes are in later patches.

ATB,
Ramsay Jones

Re: [PATCH] branch: reset instead of release a strbuf

[Junio C Hamano]

Jeff King  writes:

>>  /**
>>   * Release a string buffer and the memory it used. You should not use the
>> - * string buffer after using this function, unless you initialize it again.
>> + * string buffer after using this function.
>>   */
>>  extern void strbuf_release(struct strbuf *);
>
> I think it's actually OK to use the string buffer after this function.
> It's just an empty string.
>
> Perhaps we should be more explicit: this releases any resources and
> resets to a pristine, empty state. I suspect strbuf_detach() probably
> should make the same claim.
>
> Earlier you mentioned:
>
>> It is still not advisable to call strbuf_release until done using a
>> strbuf because it is wasteful, so keep that part of the advice.
>
> Is this what you meant? If so, I think we should probably be more
> explicit in giving people a hint to use strbuf_reset() for efficiency.

Yes, "should not use" above is simply misleading.  Either drop it
altogether, or say something like

If you find yourself reusing the same strbuf in a loop and
calling strbuf_release() each iteration, you may want to
consider if it makes more sense to use strbuf_reset()
instead in each iteration and calling strbuf_release() at
the end.

perhaps.

Re: [PATCH v8 00/12] Fast git status via a file system watcher

[Junio C Hamano]

Ben Peart  writes:

> Well, rats. I found one more issue that applies to two of the
> commits. Can you squash this in as well or do you want it in some
> other form?

Rats indeed.  Let's go incremental as promised, perhaps like this
(but please supply a better description if you have one).

-- >8 --
From: Ben Peart 
Subject: fsmonitor: MINGW support for watchman integration

Instead of just taking $ENV{'PWD'}, use the same logic that converts
PWD to $git_work_tree on MSYS_NT in the watchman integration hook
script also on MINGW.

Signed-off-by: Ben Peart 
Signed-off-by: Junio C Hamano 
---
 t/t7519/fsmonitor-watchman | 2 +-
 templates/hooks--fsmonitor-watchman.sample | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index 7ceb32dc18..cca3d71e90 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -36,7 +36,7 @@ my $system = `uname -s`;
 $system =~ s/[\r\n]+//g;
 my $git_work_tree;
 
-if ($system =~ m/^MSYS_NT/) {
+if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree = `cygpath -aw "\$PWD"`;
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;
diff --git a/templates/hooks--fsmonitor-watchman.sample 
b/templates/hooks--fsmonitor-watchman.sample
index 870a59d237..c68038ef00 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -35,7 +35,7 @@ my $system = `uname -s`;
 $system =~ s/[\r\n]+//g;
 my $git_work_tree;
 
-if ($system =~ m/^MSYS_NT/) {
+if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree = `cygpath -aw "\$PWD"`;
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;

Re: Line ending normalization doesn't work as expected

[Junio C Hamano]

Torsten Bögershausen  writes:

>> $ git rm -r --cached . && git add .
>
> (Both should work)
>
> To be honest, from the documentation, I can't figure out the difference 
> between
> $ git read-tree --empty
> and
> $ git rm -r --cached .
>
> Does anybody remember the discussion, why we ended up with read-tree ?

We used to use neither, and considered it fine to "rm .git/index" if
you wanted to empty the on-disk index file in the old world.  In the
modern world, folks want you to avoid touching filesystem directly
and instead want you to use Git tools, and the above are two obvious
ways to do so.

"git read-tree" (without any parameter, i.e. "read these 0 trees and
populate the index with it") and its modern and preferred synonym
"git read-tree --empty" (i.e. "I am giving 0 trees and I know the
sole effect of this command is to empty the index.") are more direct
ways to express "I want the index emptied" between the two.

The other one, "git rm -r --cached .", in the end gives you the same
state because it tells Git to "iterate over all the entries in the
index, find the ones that match pathspec '.', and remove them from
the index.".  It is not wrong per-se, but conceptually it is a bit
roundabout way to say that "I want the index emptied", I would
think.

I wouldn't be surprised if the "rm -r --cached ." were a lot slower,
due to the overhead of having to do the pathspec filtering that ends
up to be a no-op, but there shouldn't be a difference in the end
result.

Re: [PATCH v4] technical doc: add a design doc for hash function transition

[Junio C Hamano]

Jonathan Nieder  writes:

> +Alternatives considered
> +---
> +Upgrading everyone working on a particular project on a flag day
> +
> ...
> +Using hash functions in parallel
> +
> ...

Good that we are not doing these ;-)

> +Lazily populated translation table
> +~~
> +Some of the work of building the translation table could be deferred to
> +push time, but that would significantly complicate and slow down pushes.
> +Calculating the sha1-name at object creation time at the same time it is
> +being streamed to disk and having its newhash-name calculated should be
> +an acceptable cost.

And the version described in the body of the document hopefully
would be simpler.  It certainly would be, when SHA-1 content and
NewHash content are the same (i.e. blob).

THanks.

Git for Windows: mingw.c's strange usage of creation time as ctime?

[Marc Strapetz]

compat/mingw.c assigns the Windows file creation time [1] to Git's ctime 
fields at line 591 and at line 705:


buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));

ftCreationTime > ftLastWriteTime is actually possible after copying a 
file, so it makes sense to include this timestamp somehow in the Index, 
but I think it would be better to use the maximum of ftLastWriteTime and 
ftCreationTime here which is less confusing and closer to Unix's ctime:


buf->st_ctime = max(buf->st_mtime,
filetime_to_time_t(&(fdata.ftCreationTime));

-Marc

[1] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365739(v=vs.85).aspx

Re: [PATCH v2] oidmap: map with OID as key

[Jonathan Tan]

On Mon, Oct 2, 2017 at 11:31 PM, Jeff King  wrote:
> Right, I kind of wonder if this has fallen into an uncanny value where
> we have this almost-hashmap infrastructure, but the end result is not
> significantly easier to use than a plain-old hashmap.
>
> I.e., it looks like you still have to declare something like:
>
>   struct my_data {
> struct oidmap_entry oid;
> int value; /* mapping to an int */
>   };
>
> and handle the allocation of the entry yourself. If we instead just
> adding an oidhash() and oidcmpfn(), then callers could those directly.

I thought of something like that, but it seems that you have to
remember quite a few things:
- your entry must have "struct oidmap_entry" at the start, not "struct
hashmap_entry"
- initialize your hashmap with oidcmpfn()
- when getting, hashmap_get_from_hash(map, oidhash(), ) (and
oid might be longer e.g. ref->old_oid)

> The invocations are a _little_ longer with a raw hashmap, but not much
> (as you can see from the actual oidmap implementation, and the changes
> to oidset).

About the invocation of hashmap_get_from_hash(), I felt that it would
get annoying quickly enough that I would want an oidmap_get(const
struct hashmap *, const struct object_id *) but it might be strange
that the "get" method is named differently from the rest. If we
tolerate oidmap_get(), and tolerate the fact that the user must both
declare "struct oidmap_entry" instead of "struct hashmap_entry" and
initialize the hashmap with oidcmpfn() (so that the invocation to
hashmap_get_from_hash() within oidmap_get() sends the correct
keydata), we can avoid the thin wrapper issue where callers can no
longer use other methods of hashmap. At this point I decided that I
prefer the thin wrapper, but the "light touch" (struct oidmap_entry,
oidcmpfn(), oidmap_get() only) still better than the status quo.

Re: [PATCH v6 09/40] Add initial external odb support

[Jonathan Tan]

On Tue, Oct 3, 2017 at 2:45 AM, Christian Couder
 wrote:
> Yeah, some people need the faster solution, but my opinion is that
> many other people would prefer the single shot protocol.
> If all you want to do is a simple resumable clone using bundles for
> example, then the long running process solution is very much overkill.
>
> For example with filters there are people using them to do keyword
> expansion (maybe to emulate the way Subversion and CVS substitutes
> keywords like $Id$, $Author$ and so on). It would be really bad to
> deprecate the single shot filters and tell those people they now have
> to use long running processes because we don't want to maintain the
> small code that make single shot filters work.
>
> The Microsoft GVFS use case is just one use case that is very far from
> what most people need. And my opinion is that many more people could
> benefit from the single shot protocol. For example many people and
> admins could benefit from resumable clones using bundles and, if I
> remove the single shot protocol, this use case will be unnecessarily
> more difficult to implement in the same way as keyword expansion would
> be unnecessarily more difficult to implement if we removed the single
> shot filters.

The idea that some users will prefer writing to the single-shot
protocol is reasonable to me, but I think that providing a contrib/
Perl script that wraps something that speaks the single-shot protocol
is sufficient. This results in less C code, and a better separation of
concerns (I prefer 1 exit point and 1 adapter over 2 exit points).

> I agree that your patch set already includes some infrastructure that
> could be used by my work, and your patch sets are perhaps implementing
> some of this infrastructure better than in my work (I haven't taken a
> deep look). But I really think that the right approach is to focus
> first on designing a flexible protocol between Git and external
> stores. Then the infrastructure work should be related to improving or
> enabling the flexible protocol and the communication between Git and
> external stores.
>
> Doing infrastructure work first and improving things on top of this
> new infrastructure without relying first on a design of the protocol
> between Git and external stores is not the best approach as I think we
> might over engineer some infrastructure work or base some user
> interfaces on the infrastructure work and not on the end goal.
>
> For example if we improve the current protocol, which is not
> necessarily a bad thing in itself, we might forget that for resumable
> clone it is much better if we just let external stores and helpers
> handle the transfer.
>
> I am not saying that doing infrastructure work is bad or will not in
> the end let us reach our goals, but I see it as something that is
> potentially distracting, or misleading, from focusing first on the
> protocol between Git and external stores.

I think that the infrastructure really needs to be considered when
designing the protocol. In particular, we had to consider the needs of
the connectivity check in fsck and the repacking in GC when designing
what the promisor remote (or ODB, in this case) needs to tell us and
what, if any, postprocessing needs to be done. In the end, I settled
on tracking which objects came from the promisor remote and which did
not, which works in my design (which I have tried to ensure that it
fits in our and Microsoft's use case). But that design won't work in
what I understand to be the ODB case, at least from what I understand,
because (at least) (i) you can have multiple ODBs, and (ii) Git does
not have direct access to the objects stored within the ODBs. So some
more design needs to be done.

Dearest

[Asana Hajraf]

Dearest,
I am Mrs. Asana Hajraf and I am married to Mr. Hassan Hajraf from kuwait for 19 
years without a child and my husband died in 2014. I am contacting you to let 
you know my desire to donate the sum of $4.5 million to charities in your 
country which I inherited from my late husband. Due to my illness of cancer 
were confirmed out of just eight months, so it is my desire to see that this 
money is invested to any organization of your choice and distributed each year 
among the charity organizations, motherless babies home, schools and support 
for the men and women homeless or what you may consider to be the benefit of 
those less fortunate. I do not want this fund to be invested in a manner 
without God. As soon as I receive your reply confirming your acceptance of the 
work I tell you, I will give all relevant information to authorize the release 
and transfer the money to you as my duly appointed representative.
Thanks,
Mrs Asana Hajraf.

Re: [PATCH 0/3] fixes for running the test suite with --valgrind

[Jeff King]

On Tue, Oct 03, 2017 at 04:50:58PM -0700, Jonathan Nieder wrote:

> > I think using SANITIZE=memory would catch these, but it needs some
> > suppressions tuning. The weird "zlib reads uninitialized memory" error
> > is a problem (valgrind sees this, too, but we have suppressions).
> 
> What version of zlib do you use?  I've heard some good things about
> v1.2.11 improving matters, though I haven't checked yet.

1.2.8.dfsg-5, from Debian unstable. I'm happy to try v1.2.11 once it
hits unstable or experimental. :)

-Peff

Re: [bug] git version 2.4.12 color.ui = always breaks git add -p

[Jeff King]

On Tue, Oct 03, 2017 at 02:47:48PM -0700, Jonathan Nieder wrote:

> Hi Christian,
> 
> Christian Rebischke wrote:
> 
> > I played around with my gitconfig and I think I found a bug while doing
> > so. I set the following lines in my gitconfig:
> >
> > [color]
> > ui = always
> >
> > When I try to use `git add -p ` I don't see the 'Stage
> > this hunk'-dialog anymore. First I thought it's some other configuration
> > but now I can definitly say that this configuration breaks `git add -p`
> > interactive mode.
> 
> Do the patches at
> https://public-inbox.org/git/20171003133713.ccxv6clrmuuhh...@sigill.intra.peff.net/
> help?

If it makes it any easier to test, they are also available as a fetch
from:

  https://github.com/peff/git jk/color-no-always

-Peff

Re: [PATCH 0/3] fixes for running the test suite with --valgrind

[Jonathan Nieder]

Jeff King wrote:

> I think using SANITIZE=memory would catch these, but it needs some
> suppressions tuning. The weird "zlib reads uninitialized memory" error
> is a problem (valgrind sees this, too, but we have suppressions).

What version of zlib do you use?  I've heard some good things about
v1.2.11 improving matters, though I haven't checked yet.

Thanks,
Jonathan

Re: [PATCH] branch: reset instead of release a strbuf

[Jeff King]

On Tue, Oct 03, 2017 at 03:24:14PM -0700, Jonathan Nieder wrote:

> Here's a patch to address the surprising strbuf.h advice.
> 
> -- >8 --
> Subject: strbuf: do not encourage init-after-release
> 
> strbuf_release already leaves the strbuf in a valid, initialized
> state, so there is not need to call strbuf_init after it.
> 
> Moreover, this is not likely to change in the future: strbuf_release
> leaving the strbuf in a valid state has been easy to maintain and has
> been very helpful for Git's robustness and simplicity (e.g.,
> preventing use-after-free vulnerabilities).

Thanks for picking this up. Like you, I was quite surprised when I saw
Stefan's original patch.

> diff --git a/strbuf.h b/strbuf.h
> index 7496cb8ec5..6e175c3694 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -83,7 +83,7 @@ extern void strbuf_init(struct strbuf *, size_t);
>  
>  /**
>   * Release a string buffer and the memory it used. You should not use the
> - * string buffer after using this function, unless you initialize it again.
> + * string buffer after using this function.
>   */
>  extern void strbuf_release(struct strbuf *);

I think it's actually OK to use the string buffer after this function.
It's just an empty string.

Perhaps we should be more explicit: this releases any resources and
resets to a pristine, empty state. I suspect strbuf_detach() probably
should make the same claim.

Earlier you mentioned:

> It is still not advisable to call strbuf_release until done using a
> strbuf because it is wasteful, so keep that part of the advice.

Is this what you meant? If so, I think we should probably be more
explicit in giving people a hint to use strbuf_reset() for efficiency.

-Peff

Re: [PATCH 0/3] fixes for running the test suite with --valgrind

[Jeff King]

On Tue, Oct 03, 2017 at 08:57:10PM +0100, Thomas Gummerer wrote:

> I recently tried to run the git test suite with --valgrind.
> Unfortunately it didn't come back completely clean.  This patch series
> fixes a bunch of these errors, although unfortunately not quite all of
> them yet.

Thanks for trying that. I've largely switched to ASan over valgrind
because it runs _so_ much faster. Unfortunately it seems to miss
these uninitialized-memory cases.

I think using SANITIZE=memory would catch these, but it needs some
suppressions tuning. The weird "zlib reads uninitialized memory" error
is a problem (valgrind sees this, too, but we have suppressions).

-Peff

Re: [PATCH 00/18] Partial clone (from clone to lazy fetch in 18 patches)

[Jonathan Tan]

On Tue, Oct 3, 2017 at 7:39 AM, Jeff Hostetler  wrote:
>
> As I see it there are the following major parts to partial clone:
> 1. How to let git-clone (and later git-fetch) specify the desired
>subset of objects that it wants?  (A ref-relative request.)
> 2. How to let the server and git-pack-objects build that incomplete
>packfile?
> 3. How to remember in the local config that a partial clone (or
>fetch) was used and that missing object should be expected?
> 4. How to dynamically fetch individual missing objects individually?
> (Not a ref-relative request.)
> 5. How to augment the local ODB with partial clone information and
>let git-fsck (and friends) perform limited consistency checking?
> 6. Methods to bulk fetching missing objects (whether in a pre-verb
>hook or in unpack-tree)
> 7. Miscellaneous issues (e.g. fixing places that accidentally cause
>a missing object to be fetched that don't really need it).

Thanks for the enumeration.

> As was suggested above, I think we should merge our efforts:
> using my filtering for 1 and 2 and Jonathan's code for 3, 4, and 5.
> I would need to eliminate the "relax" options in favor of his
> is_promised() functionality for index-pack and similar.  And omit
> his blob-max-bytes changes from pack-objects, the protocol and
> related commands.
>
> That should be a good first step.

This sounds good to me. Jeff Hostetler's filtering (all blobs, blobs
by size, blobs by sparse checkout specification) is more comprehensive
than mine, so removing blob-max-bytes from my code is not a problem.

> We both have thoughts on bulk fetching (mine in pre-verb hooks and
> his in unpack-tree).  We don't need this immediately, but can wait
> until the above is working to revisit.

Agreed.

Re: [PATCH 1/3] path.c: fix uninitialized memory access

[Jonathan Nieder]

Jeff King wrote:
> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:

>> In other words, an alternative fix would be
>> 
>>  if (*path == '.' && path[1] == '/') {
>>  ...
>>  }
>> 
>> which would not require passing in 'len' or switching to index-based
>> arithmetic.  I think I prefer it.  What do you think?
>
> Yes, I think that approach is much nicer. I think you could even use
> skip_prefix. Unfortunately you have to play a few games with const-ness,
> but I think the resulting signature for cleanup_path() is an
> improvement:

Ooh!

For what it's worth, if you add a commit message with Thomas's
Reported-by then this lgtm.

Thanks,
Jonathan

> diff --git a/path.c b/path.c
> index 00ec04e7a5..2e09a7bce0 100644
> --- a/path.c
> +++ b/path.c
> @@ -34,11 +34,10 @@ static struct strbuf *get_pathname(void)
>   return sb;
>  }
>  
> -static char *cleanup_path(char *path)
> +static const char *cleanup_path(const char *path)
>  {
>   /* Clean it up */
> - if (!memcmp(path, "./", 2)) {
> - path += 2;
> + if (skip_prefix(path, "./", )) {
>   while (*path == '/')
>   path++;
>   }
> @@ -47,7 +46,7 @@ static char *cleanup_path(char *path)
>  
>  static void strbuf_cleanup_path(struct strbuf *sb)
>  {
> - char *path = cleanup_path(sb->buf);
> + const char *path = cleanup_path(sb->buf);
>   if (path > sb->buf)
>   strbuf_remove(sb, 0, path - sb->buf);
>  }
> @@ -64,7 +63,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
>   strlcpy(buf, bad_path, n);
>   return buf;
>   }
> - return cleanup_path(buf);
> + return (char *)cleanup_path(buf);
>  }
>  
>  static int dir_prefix(const char *buf, const char *dir)

Re: [PATCH 2/3] http-push: fix construction of hex value from path

[Jeff King]

On Tue, Oct 03, 2017 at 03:53:15PM -0700, Jonathan Nieder wrote:

> Thomas Gummerer wrote:
> 
> > The get_oid_hex_from_objpath takes care of creating a oid from a
> > pathname.  It does this by memcpy'ing the first two bytes of the path to
> > the "hex" string, then skipping the '/', and then copying the rest of the
> > path to the "hex" string.  Currently it fails to increase the pointer to
> > the hex string, so the second memcpy invocation just mashes over what
> > was copied in the first one, and leaves the last two bytes in the string
> > uninitialized.
> 
> Wow.  The fix is obviously correct.

Agreed. This is brown-paper-bag worthy. Thanks, Thomas, for cleaning up
my mess.

> I think the problem is that when it fails, we end up thinking that
> there are *fewer* objects than are actually present remotely so the
> only ill effect is pushing too much.  So this should be observable in
> server logs (i.e. it is testable) but it's not a catastrophic failure
> which means it's harder to test than it would be otherwise.

And thank you, Jonathan, for this analysis. I had also wondered how such
a frequently-triggered bug could have gone completely unnoticed, but
this explains it.

> Moreover, this is in the webdav-based "dumb http" push code path,
> which I do not trust much at all.  I wonder if we could retire it
> completely (or at least provide an option to turn it off).

I would really like that, too. It has been the cause of a lot of pain
when working with the smart code, and I am not at all surprised to find
a bug of this magnitude lurking in it. I'd _hoped_ this could show that
the system has been unusably broken for years, which would give us
confidence to turn it off. :) But per your paragraph above, people could
very easily still have been happily using it in the meantime.

-Peff

Re: [PATCH 1/3] path.c: fix uninitialized memory access

[Jeff King]

On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:

> When I first read the above, I thought it was going to be about a
> NUL-terminated string that was missing a NUL.  But in fact, the issue
> is that strlen(path) can be < 2.
> 
> In other words, an alternative fix would be
> 
>   if (*path == '.' && path[1] == '/') {
>   ...
>   }
> 
> which would not require passing in 'len' or switching to index-based
> arithmetic.  I think I prefer it.  What do you think?

Yes, I think that approach is much nicer. I think you could even use
skip_prefix. Unfortunately you have to play a few games with const-ness,
but I think the resulting signature for cleanup_path() is an
improvement:

diff --git a/path.c b/path.c
index 00ec04e7a5..2e09a7bce0 100644
--- a/path.c
+++ b/path.c
@@ -34,11 +34,10 @@ static struct strbuf *get_pathname(void)
return sb;
 }
 
-static char *cleanup_path(char *path)
+static const char *cleanup_path(const char *path)
 {
/* Clean it up */
-   if (!memcmp(path, "./", 2)) {
-   path += 2;
+   if (skip_prefix(path, "./", )) {
while (*path == '/')
path++;
}
@@ -47,7 +46,7 @@ static char *cleanup_path(char *path)
 
 static void strbuf_cleanup_path(struct strbuf *sb)
 {
-   char *path = cleanup_path(sb->buf);
+   const char *path = cleanup_path(sb->buf);
if (path > sb->buf)
strbuf_remove(sb, 0, path - sb->buf);
 }
@@ -64,7 +63,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
strlcpy(buf, bad_path, n);
return buf;
}
-   return cleanup_path(buf);
+   return (char *)cleanup_path(buf);
 }
 
 static int dir_prefix(const char *buf, const char *dir)

Re: "man git-config", "--list" option misleadingly refers to "config file" (singular)

[Jeff King]

On Tue, Oct 03, 2017 at 04:54:32PM -0400, Robert P. J. Day wrote:

> > It does that by default, or it lists the contents of a specific file
> > if given (either by --file, or with --system, --global, or --local).
> >
> > So I agree it's not quite accurate, but you probably want some
> > phrasing that leaves this unsaid (the actual rules are described
> > earlier in the description section). Maybe just refer to it as the
> > "config source" or something?
> 
>   i think the simplest phrasing is, "List all variables set in the
> current configuration, along with their values."
> 
>   sound fair?

Yes, sounds reasonable to me.

-Peff

Re: [PATCH] test-stringlist: avoid buffer underrun when sorting nothing

[Jonathan Nieder]

René Scharfe wrote:

> Check if the strbuf containing data to sort is empty before attempting
> to trim a trailing newline character.
>
> Signed-off-by: Rene Scharfe 
> ---
>  t/helper/test-string-list.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks for fixing it.
Reviewed-by: Jonathan Nieder 

Cash Statement of Account 10/03/2017

[Front Desk]

Kindly find attached today's cash payments statement of account. 10/3/2017

Reems Exchange Company


SOA-03-10-2017.pdf
Description: SOA-03-10-2017.pdf

Re: [PATCH 2/3] http-push: fix construction of hex value from path

[Jonathan Nieder]

Hi,

Thomas Gummerer wrote:

> The get_oid_hex_from_objpath takes care of creating a oid from a
> pathname.  It does this by memcpy'ing the first two bytes of the path to
> the "hex" string, then skipping the '/', and then copying the rest of the
> path to the "hex" string.  Currently it fails to increase the pointer to
> the hex string, so the second memcpy invocation just mashes over what
> was copied in the first one, and leaves the last two bytes in the string
> uninitialized.

Wow.  The fix is obviously correct.

> This breaks valgrind in t5540, although the test passes without
> valgrind:
[...]
> Signed-off-by: Thomas Gummerer 
> ---
>  http-push.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Would it be straightforward to add a correctness test for this?  It
seems like this code path didn't work at all and no one noticed.

This is the code path in http-push.c which says

 /*
  * NEEDSWORK: remote_ls() ignores info/refs on the remote side.  But it
  * should _only_ heed the information from that file, instead of trying to
  * determine the refs from the remote file system (badly: it does not even
  * know about packed-refs).
  */
 static void remote_ls(const char *path, int flags,

I think the problem is that when it fails, we end up thinking that
there are *fewer* objects than are actually present remotely so the
only ill effect is pushing too much.  So this should be observable in
server logs (i.e. it is testable) but it's not a catastrophic failure
which means it's harder to test than it would be otherwise.

Moreover, this is in the webdav-based "dumb http" push code path,
which I do not trust much at all.  I wonder if we could retire it
completely (or at least provide an option to turn it off).

> diff --git a/http-push.c b/http-push.c
> index e4c9b065ce..e9a01ec4da 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1018,7 +1018,7 @@ static int get_oid_hex_from_objpath(const char *path, 
> struct object_id *oid)
>   memcpy(hex, path, 2);
>   path += 2;
>   path++; /* skip '/' */
> - memcpy(hex, path, GIT_SHA1_HEXSZ - 2);
> + memcpy(hex + 2, path, GIT_SHA1_HEXSZ - 2);
>  
>   return get_oid_hex(hex, oid);

Thanks,
Jonathan

Re: [PATCH 1/3] path.c: fix uninitialized memory access

[Jonathan Nieder]

Hi,

Thomas Gummerer wrote:

> In cleanup_path we're passing in a char array, run a memcmp on it, and
> run through it without ever checking if something is in the array in the
> first place.  This can lead us to access uninitialized memory, for
> example in t5541-http-push-smart.sh test 7, when run under valgrind:
[...]
> Avoid this by checking passing in the length of the string in the char
> array, and checking that we never run over it.
>
> Signed-off-by: Thomas Gummerer 
> ---
>  path.c | 19 ++-
>  1 file changed, 10 insertions(+), 9 deletions(-)

When I first read the above, I thought it was going to be about a
NUL-terminated string that was missing a NUL.  But in fact, the issue
is that strlen(path) can be < 2.

In other words, an alternative fix would be

if (*path == '.' && path[1] == '/') {
...
}

which would not require passing in 'len' or switching to index-based
arithmetic.  I think I prefer it.  What do you think?

Thanks and hope that helps,
Jonathan

diff --git i/path.c w/path.c
index b533ec938d..3a1fbee1e0 100644
--- i/path.c
+++ w/path.c
@@ -37,7 +37,7 @@ static struct strbuf *get_pathname(void)
 static char *cleanup_path(char *path)
 {
/* Clean it up */
-   if (!memcmp(path, "./", 2)) {
+   if (*path == '.' && path[1] == '/') {
path += 2;
while (*path == '/')
path++;

Re: [PATCH] branch: reset instead of release a strbuf

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Our documentation advises to not re-use a strbuf, after strbuf_release
> has been called on it. Use the proper reset instead.
>
> Reviewed-by: Jonathan Nieder 

This is indeed
Reviewed-by: Jonathan Nieder 

Thank you.

> Signed-off-by: Stefan Beller 
> ---
>  builtin/branch.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Here's a patch to address the surprising strbuf.h advice.

-- >8 --
Subject: strbuf: do not encourage init-after-release

strbuf_release already leaves the strbuf in a valid, initialized
state, so there is not need to call strbuf_init after it.

Moreover, this is not likely to change in the future: strbuf_release
leaving the strbuf in a valid state has been easy to maintain and has
been very helpful for Git's robustness and simplicity (e.g.,
preventing use-after-free vulnerabilities).

It is still not advisable to call strbuf_release until done using a
strbuf because it is wasteful, so keep that part of the advice.

Reported-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 7496cb8ec5..6e175c3694 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -83,7 +83,7 @@ extern void strbuf_init(struct strbuf *, size_t);
 
 /**
  * Release a string buffer and the memory it used. You should not use the
- * string buffer after using this function, unless you initialize it again.
+ * string buffer after using this function.
  */
 extern void strbuf_release(struct strbuf *);
 
-- 
2.14.2.920.gcf0c67979c

[PATCH] branch: reset instead of release a strbuf

[Stefan Beller]

Our documentation advises to not re-use a strbuf, after strbuf_release
has been called on it. Use the proper reset instead.

Currently 'strbuf_release' releases and re-initializes the strbuf, so it
is safe, but slow. 'strbuf_reset' only resets the internal length variable,
such that this could also be accounted for as a micro-optimization.

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

diff --git a/builtin/branch.c b/builtin/branch.c
index b998e16d0c..71ed1c7036 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -217,7 +217,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
if (!head_rev)
die(_("Couldn't look up commit object for HEAD"));
}
-   for (i = 0; i < argc; i++, strbuf_release()) {
+   for (i = 0; i < argc; i++, strbuf_reset()) {
char *target = NULL;
int flags = 0;
 
@@ -282,8 +282,9 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
}
 
free(name);
+   strbuf_release();
 
-   return(ret);
+   return ret;
 }
 
 static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
-- 
2.14.0.rc0.3.g6c2e499285

[PATCH] test-list-objects: mark file-local symbols as static

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Derrick,

If you need to re-roll your 'ds/find-unique-abbrev-optim' branch,
could you please squash this into the relevant patch (commit 3792c78ba0,
"test-list-objects: list a subset of object ids", 01-10-2017).

Thanks!

ATB,
Ramsay Jones

 t/helper/test-list-objects.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/t/helper/test-list-objects.c b/t/helper/test-list-objects.c
index 22bc9b4e6..5c5d3c03f 100644
--- a/t/helper/test-list-objects.c
+++ b/t/helper/test-list-objects.c
@@ -6,43 +6,43 @@ struct count {
int select_mod;
 };
 
-int count_loose(const struct object_id *oid,
-   const char *path,
-   void *data)
+static int count_loose(const struct object_id *oid,
+  const char *path,
+  void *data)
 {
((struct count*)data)->total++;
return 0;
 }
 
-int count_packed(const struct object_id *oid,
-struct packed_git *pack,
-uint32_t pos,
-void* data)
+static int count_packed(const struct object_id *oid,
+   struct packed_git *pack,
+   uint32_t pos,
+   void* data)
 {
((struct count*)data)->total++;
return 0;
 }
 
-void output(const struct object_id *oid,
-   struct count *c)
+static void output(const struct object_id *oid,
+  struct count *c)
 {
if (!(c->total % c->select_mod))
printf("%s\n", oid_to_hex(oid));
c->total--;
 }
 
-int output_loose(const struct object_id *oid,
-const char *path,
-void *data)
+static int output_loose(const struct object_id *oid,
+   const char *path,
+   void *data)
 {
output(oid, (struct count*)data);
return 0;
 }
 
-int output_packed(const struct object_id *oid,
- struct packed_git *pack,
- uint32_t pos,
- void* data)
+static int output_packed(const struct object_id *oid,
+struct packed_git *pack,
+uint32_t pos,
+void* data)
 {
output(oid, (struct count*)data);
return 0;
-- 
2.14.0

contact my email (wang.jian...@yandex.com)

[Alma Gyogyszertar Orient]

I intend to give you a portion of my wealth as a free-will financial donation 
to you, Respond to partake.contact my email (wang.jian...@yandex.com)
Wang Jianlin
Wanda Group

Re: [bug] git version 2.4.12 color.ui = always breaks git add -p

[Jonathan Nieder]

Hi Christian,

Christian Rebischke wrote:

> I played around with my gitconfig and I think I found a bug while doing
> so. I set the following lines in my gitconfig:
>
> [color]
> ui = always
>
> When I try to use `git add -p ` I don't see the 'Stage
> this hunk'-dialog anymore. First I thought it's some other configuration
> but now I can definitly say that this configuration breaks `git add -p`
> interactive mode.

Do the patches at
https://public-inbox.org/git/20171003133713.ccxv6clrmuuhh...@sigill.intra.peff.net/
help?

Thanks and hope that helps,
Jonathan

Re: [PATCH] branch: reset instead of release a strbuf

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Our documentation advises to not re-use a strbuf, after strbuf_release
> has been called on it. Use the proper reset instead.

I'm super surprised at this documentation.  strbuf_release maintains
the invariant that a strbuf is always usable (i.e., that we do not have
to fear use-after-free problems).

On second thought, though, strbuf_release is a more expensive operation
than strbuf_reset --- constantly free()-ing and re-malloc()ing is
unnecessary churn in malloc data structures.  So maybe that is the
motivation here?

> Signed-off-by: Stefan Beller 
> ---
>
> Maybe one of the #leftoverbits is to remove the re-init call in release
> and see what breaks? (And then fixing up more of such cases as presented
> in this patch)

As mentioned above: please no.  That would be problematic both for
ease of development and for the risk of security bugs.

>  builtin/branch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index b998e16d0c..9758012390 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -217,7 +217,7 @@ static int delete_branches(int argc, const char **argv, 
> int force, int kinds,
>   if (!head_rev)
>   die(_("Couldn't look up commit object for HEAD"));
>   }
> - for (i = 0; i < argc; i++, strbuf_release()) {
> + for (i = 0; i < argc; i++, strbuf_reset()) {
>   char *target = NULL;
>   int flags = 0;

Should there be a strbuf_release (or UNLEAK if you are very very sure)
call at the end of the function to replace this?

With that change (but not without it),
Reviewed-by: Jonathan Nieder 

Thanks.

Re: [PATCH v3 10/10] ssh: introduce a 'simple' ssh variant

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> When using the 'ssh' transport, the '-o' option is used to specify an
> environment variable which should be set on the remote end.  This allows
> git to send additional information when contacting the server,
> requesting the use of a different protocol version via the
> 'GIT_PROTOCOL' environment variable like so: "-o SendEnv=GIT_PROTOCOL"
>
> Unfortunately not all ssh variants support the sending of environment
> variables to the remote end.  To account for this, only use the '-o'
> option for ssh variants which are OpenSSH compliant.  This is done by
> checking that the basename of the ssh command is 'ssh' or the ssh
> variant is overridden to be 'ssh' (via the ssh.variant config).

This also affects -p (port), right?

What happens if I specify a ssh://host:port/path URL and the SSH
implementation is of 'simple' type?

> Previously if an ssh command's basename wasn't 'plink' or

Git's commit messages use the present tense to describe the current
state of the code, so this is "Currently". :)

> 'tortoiseplink' git assumed that the command was an OpenSSH variant.
> Since user configured ssh commands may not be OpenSSH compliant, tighten
> this constraint and assume a variant of 'simple' if the basename of the
> command doesn't match the variants known to git.  The new ssh variant
> 'simple' will only have the host and command to execute ([username@]host
> command) passed as parameters to the ssh command.
>
> Update the Documentation to better reflect the command-line options sent
> to ssh commands based on their variant.
>
> Reported-by: Jeffrey Yasskin 
> Signed-off-by: Brandon Williams 

Thanks for working on this.

For background, the GIT_SSH implementation that motivated this is
https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c56b068bac0a77c049/lib/dpl/provider.rb#L215,
which does not support -p or -4/-6, either.

> ---
>  Documentation/config.txt |  27 ++--
>  Documentation/git.txt|   9 ++--
>  connect.c| 107 
> ++-
>  t/t5601-clone.sh |   9 ++--
>  t/t5700-protocol-v1.sh   |   2 +
>  5 files changed, 95 insertions(+), 59 deletions(-)
[...]
> --- a/connect.c
> +++ b/connect.c
> @@ -776,37 +776,44 @@ static const char *get_ssh_command(void)
[...]
> +static enum ssh_variant determine_ssh_variant(const char *ssh_command,
> +   int is_cmdline)
[...]
> - if (!strcasecmp(variant, "plink") ||
> - !strcasecmp(variant, "plink.exe"))
> - *port_option = 'P';
> + if (!strcasecmp(variant, "ssh"))
> + ssh_variant = VARIANT_SSH;

Could this handle ssh.exe, too?

[...]
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh

Can this get tests for the new defaulting behavior?  E.g.

 - default is "simple"
 - how "simple" treats an ssh://host:port/path URL
 - how "simple" treats ipv4/ipv6 switching
 - ssh defaults to "ssh"
 - if GIT_SSH=ssh, can set ssh.variant to "simple" to force the "simple"
   mode

One other worry: this (intentionally) changes the behavior of a
previously-working GIT_SSH=ssh-wrapper that wants to support
OpenSSH-style options but does not declare ssh.variant=ssh.  When
discovering this change, what should the author of such an ssh-wrapper
do?

They could instruct their users to set ssh.variant or GIT_SSH_VARIANT
to "ssh", but then they are at the mercy of future additional options
supported by OpenSSH we may want to start using in the future (e.g.,
maybe we will start passing "--" to separate options from the
hostname).  So this is not a futureproof option for them.

They could take the new default behavior or instruct their users to
set ssh.variant or GIT_SSH_VARIANT to "simple", but then they lose
support for handling alternate ports, ipv4/ipv6, and specifying -o
SendEnv to propagate GIT_PROTOCOL or other envvars.  They can handle
GIT_PROTOCOL propagation manually, but losing port support seems like
a heavy cost.

They could send a patch to define yet another variant that is
forward-compatible, for example using an interface similar to what
git-credential(1) uses.  Then they can set GIT_SSH to their
OpenSSH-style helper and GIT_FANCY_NEW_SSH to their more modern
helper, so that old Git versions could use GIT_SSH and new Git
versions could use GIT_FANCY_NEW_SSH.  This might be their best
option.  It feels odd to say that their only good way forward is to
send a patch, but on the other hand someone with such an itch is
likely to be in the best position to define an appropriate interface.

They could send a documentation patch to make more promises about the
commandline used in OpenSSH mode: e.g. setting a rule in advance about
which options can take an argument so that they can properly parse an
OpenSSH command line in a future-compatible way.

Or they could send a patch to allow passing the port in "simple"
mode, for example using an environment variable.

Am 

[bug] git version 2.4.12 color.ui = always breaks git add -p

[Christian Rebischke]

Hello everybody,
I played around with my gitconfig and I think I found a bug while doing
so. I set the following lines in my gitconfig:

[color]
ui = always

When I try to use `git add -p ` I don't see the 'Stage
this hunk'-dialog anymore. First I thought it's some other configuration
but now I can definitly say that this configuration breaks `git add -p`
interactive mode.

best regards

chris


signature.asc
Description: PGP signature

[PATCH] branch: reset instead of release a strbuf

[Stefan Beller]

Our documentation advises to not re-use a strbuf, after strbuf_release
has been called on it. Use the proper reset instead.

Signed-off-by: Stefan Beller 
---

Maybe one of the #leftoverbits is to remove the re-init call in release
and see what breaks? (And then fixing up more of such cases as presented
in this patch)

Thanks,
Stefan

 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b998e16d0c..9758012390 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -217,7 +217,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
if (!head_rev)
die(_("Couldn't look up commit object for HEAD"));
}
-   for (i = 0; i < argc; i++, strbuf_release()) {
+   for (i = 0; i < argc; i++, strbuf_reset()) {
char *target = NULL;
int flags = 0;
 
-- 
2.14.0.rc0.3.g6c2e499285

Re: "man git-config", "--list" option misleadingly refers to "config file" (singular)

[Robert P. J. Day]

On Tue, 3 Oct 2017, Jeff King wrote:

> On Tue, Oct 03, 2017 at 06:34:34AM -0400, rpj...@crashcourse.ca wrote:
>
> >   (i suppose that if i'm going to continue whining about stuff, i might
> > as well clone the git source and start submitting patches.)
>
> Yes, please. :)
>
> >   in "man git-config":
> >
> > -l
> > --list
> >   List all variables set in config file, along with their values.
> >
> >
> > except that, AIUI, "git config --list" will list the combination
> > of all config values in (if it exists) /etc/gitconfig, then the
> > user's global settings, and finally the repo's config settings if
> > one happens to be in a working directory, so technically, it lists
> > the contents of *all* of the config files (plural), no?
>
> It does that by default, or it lists the contents of a specific file
> if given (either by --file, or with --system, --global, or --local).
>
> So I agree it's not quite accurate, but you probably want some
> phrasing that leaves this unsaid (the actual rules are described
> earlier in the description section). Maybe just refer to it as the
> "config source" or something?

  i think the simplest phrasing is, "List all variables set in the
current configuration, along with their values."

  sound fair?

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Project Proposal

[Joseph Taylorl]

Dear sir,

I am Barrister Joseph Taylor, a legal Solicitor. I was the Personal
Attorney and legal adviser to Mr. John ALBIN, a national of your country,
who was an expatriate engineer to British Petroleum oil Company. My
client, his wife, and their three children were involved in the ill fated
Kenya Airways crash in the coasts of Abidjan in January 2000 in which all
passengers on board died. Since then I have made several inquiries to your
embassy to locate any of my clients extended relatives, this has proved
unsuccessful.

After these several unsuccessful attempts, I decided to trace his
relatives over the Internet, to locate any member of his family but of no
avail, hence I contacted you. I have contacted you to assist in
repatriating the fund and property left behind by my client before the
bank diverts the fund to government treasury.

I seek your consent to present you as a relative to the deceased since you
are from the same country so that his fund valued at Twenty Million United
States Dollars (US$20,000,000.00) can be transferred to you by the bank. I
will procure all the necessary claim documents from the court to make your
claim most legal and legitimate. I will also present you officially to the
bank since I am in a position to do so being the executor of his will.

We shall decide on the sharing formula once I hear from you. However, I
will expect to hear from you urgently because time is running out on us to
claim the fund.

I am Waiting for your urgent response.

Best Regards,
Joseph Taylor

Re: [PATCH 3/3] sub-process: allocate argv on the heap

[Stefan Beller]

On Tue, Oct 3, 2017 at 12:57 PM, Thomas Gummerer  wrote:
> Currently the argv is only allocated on the stack, and then assigned to
> process->argv.  When the start_subprocess function goes out of scope,
> the local argv variable is eliminated from the stack, but the pointer is
> still kept around in process->argv.
>
> Much later when we try to access the same process->argv in
> finish_command, this leads us to access a memory location that no longer
> contains what we want.  As argv0 is only used for printing errors, this
> is not easily noticed in normal git operations.  However when running
> t0021-conversion.sh through valgrind, valgrind rightfully complains:
>
> ==21024== Invalid read of size 8
> ==21024==at 0x2ACF64: finish_command (run-command.c:869)
> ==21024==by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)
> ==21024==by 0x2AB41E: cleanup_children (run-command.c:45)
> ==21024==by 0x2AB526: cleanup_children_on_exit (run-command.c:81)
> ==21024==by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)
> ==21024==by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)
> ==21024==by 0x11A9EF: handle_builtin (git.c:550)
> ==21024==by 0x11ABCC: run_argv (git.c:602)
> ==21024==by 0x11AD8E: cmd_main (git.c:679)
> ==21024==by 0x1BF125: main (common-main.c:43)
> ==21024==  Address 0x1ffeffec00 is on thread 1's stack
> ==21024==  1504 bytes below stack pointer
> ==21024==
>
> Fix this by allocating the memory on properly on the heap.  This memory
> is allocated on the heap, and never free'd.  However the same seems to be
> true for struct child_process, so it should be fine to just let the
> memory be free'd when the process terminates.

Uh. :( The broken window theory at work.

The patch below seems correct, but as you eluded to, now we'd be
leaking memory. The run_command API has two fields 'char **argv'
and 'argv_array args'. The argv is kept around for historical reasons
as well as when the caller wants to be in control of the array (the caller
needs to free the memory, but could also just reuse it for a slightly
different invocation), whereas the args argument is owned by the child
process, such that the memory is freed by finish_command.

As we're doing a memory allocation now anyway, how about:

-   const char *argv[] = { cmd, NULL };
...
child_process_init(process);
+argv_array_push(process.args, cmd);

Re: [PATCH 3/3] sub-process: allocate argv on the heap

[Johannes Sixt]

Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:

diff --git a/sub-process.c b/sub-process.c
index 6dde5062be..4680af8193 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct 
subprocess_entry *entry, co
  {
int err;
struct child_process *process;
-   const char *argv[] = { cmd, NULL };
+   const char **argv = xmalloc(2 * sizeof(char *));
+   argv[0] = cmd;
+   argv[1] = NULL;
  
  	entry->cmd = cmd;

process = >process;



Perhaps this should become

argv_array_push(>args, cmd);

so that there is no new memory leak?

-- Hannes

[PATCH v3 06/10] connect: teach client to recognize v1 server response

[Brandon Williams]

Teach a client to recognize that a server understands protocol v1 by
looking at the first pkt-line the server sends in response.  This is
done by looking for the response "version 1" send by upload-pack or
receive-pack.

Signed-off-by: Brandon Williams 
---
 connect.c | 30 ++
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/connect.c b/connect.c
index 8e2e276b6..a5e708a61 100644
--- a/connect.c
+++ b/connect.c
@@ -12,6 +12,7 @@
 #include "sha1-array.h"
 #include "transport.h"
 #include "strbuf.h"
+#include "protocol.h"
 
 static char *server_capabilities;
 static const char *parse_feature_value(const char *, const char *, int *);
@@ -129,9 +130,23 @@ static int read_remote_ref(int in, char **src_buf, size_t 
*src_len,
return len;
 }
 
-#define EXPECTING_FIRST_REF 0
-#define EXPECTING_REF 1
-#define EXPECTING_SHALLOW 2
+#define EXPECTING_PROTOCOL_VERSION 0
+#define EXPECTING_FIRST_REF 1
+#define EXPECTING_REF 2
+#define EXPECTING_SHALLOW 3
+
+/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. */
+static int process_protocol_version(void)
+{
+   switch (determine_protocol_version_client(packet_buffer)) {
+   case protocol_v1:
+   return 1;
+   case protocol_v0:
+   return 0;
+   default:
+   die("server is speaking an unknown protocol");
+   }
+}
 
 static void process_capabilities(int *len)
 {
@@ -224,12 +239,19 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t src_len,
 */
int responded = 0;
int len;
-   int state = EXPECTING_FIRST_REF;
+   int state = EXPECTING_PROTOCOL_VERSION;
 
*list = NULL;
 
while ((len = read_remote_ref(in, _buf, _len, ))) {
switch (state) {
+   case EXPECTING_PROTOCOL_VERSION:
+   if (process_protocol_version()) {
+   state = EXPECTING_FIRST_REF;
+   break;
+   }
+   state = EXPECTING_FIRST_REF;
+   /* fallthrough */
case EXPECTING_FIRST_REF:
process_capabilities();
if (process_dummy_ref()) {
-- 
2.14.2.920.gcf0c67979c-goog

[PATCH v3 10/10] ssh: introduce a 'simple' ssh variant

[Brandon Williams]

When using the 'ssh' transport, the '-o' option is used to specify an
environment variable which should be set on the remote end.  This allows
git to send additional information when contacting the server,
requesting the use of a different protocol version via the
'GIT_PROTOCOL' environment variable like so: "-o SendEnv=GIT_PROTOCOL"

Unfortunately not all ssh variants support the sending of environment
variables to the remote end.  To account for this, only use the '-o'
option for ssh variants which are OpenSSH compliant.  This is done by
checking that the basename of the ssh command is 'ssh' or the ssh
variant is overridden to be 'ssh' (via the ssh.variant config).

Previously if an ssh command's basename wasn't 'plink' or
'tortoiseplink' git assumed that the command was an OpenSSH variant.
Since user configured ssh commands may not be OpenSSH compliant, tighten
this constraint and assume a variant of 'simple' if the basename of the
command doesn't match the variants known to git.  The new ssh variant
'simple' will only have the host and command to execute ([username@]host
command) passed as parameters to the ssh command.

Update the Documentation to better reflect the command-line options sent
to ssh commands based on their variant.

Reported-by: Jeffrey Yasskin 
Signed-off-by: Brandon Williams 
---
 Documentation/config.txt |  27 ++--
 Documentation/git.txt|   9 ++--
 connect.c| 107 ++-
 t/t5601-clone.sh |   9 ++--
 t/t5700-protocol-v1.sh   |   2 +
 5 files changed, 95 insertions(+), 59 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b78747abc..0460af37e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2084,12 +2084,31 @@ ssh.variant::
Depending on the value of the environment variables `GIT_SSH` or
`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
auto-detects whether to adjust its command-line parameters for use
-   with plink or tortoiseplink, as opposed to the default (OpenSSH).
+   with ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
+   (simple).
 +
 The config variable `ssh.variant` can be set to override this auto-detection;
-valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
-will be treated as normal ssh. This setting can be overridden via the
-environment variable `GIT_SSH_VARIANT`.
+valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any
+other value will be treated as normal ssh. This setting can be overridden via
+the environment variable `GIT_SSH_VARIANT`.
++
+The current command-line parameters used for each variant are as
+follows:
++
+--
+
+* `ssh` - [-p port] [-4] [-6] [-o option] [username@]host command
+
+* `simple` - [username@]host command
+
+* `plink` or `putty` - [-P port] [-4] [-6] [username@]host command
+
+* `tortoiseplink` - [-P port] [-4] [-6] -batch [username@]host command
+
+--
++
+Except for the `simple` variant, command-line parameters are likely to
+change as git gains new features.
 
 i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7518ea3af..8bc3f2147 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -518,11 +518,10 @@ other
If either of these environment variables is set then 'git fetch'
and 'git push' will use the specified command instead of 'ssh'
when they need to connect to a remote system.
-   The command will be given exactly two or four arguments: the
-   'username@host' (or just 'host') from the URL and the shell
-   command to execute on that remote system, optionally preceded by
-   `-p` (literally) and the 'port' from the URL when it specifies
-   something other than the default SSH port.
+   The command-line parameters passed to the configured command are
+   determined by the ssh variant.  See `ssh.variant` option in
+   linkgit:git-config[1] for details.
+
 +
 `$GIT_SSH_COMMAND` takes precedence over `$GIT_SSH`, and is interpreted
 by the shell, which allows additional arguments to be included.
diff --git a/connect.c b/connect.c
index b8695a2fa..65cee49b6 100644
--- a/connect.c
+++ b/connect.c
@@ -776,37 +776,44 @@ static const char *get_ssh_command(void)
return NULL;
 }
 
-static int override_ssh_variant(int *port_option, int *needs_batch)
+enum ssh_variant {
+   VARIANT_SIMPLE,
+   VARIANT_SSH,
+   VARIANT_PLINK,
+   VARIANT_PUTTY,
+   VARIANT_TORTOISEPLINK,
+};
+
+static int override_ssh_variant(enum ssh_variant *ssh_variant)
 {
-   char *variant;
+   const char *variant = getenv("GIT_SSH_VARIANT");
 
-   variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
-   if (!variant &&
-   git_config_get_string("ssh.variant", ))
+  

[PATCH v3 07/10] connect: tell server that the client understands v1

[Brandon Williams]

Teach the connection logic to tell a serve that it understands protocol
v1.  This is done in 2 different ways for the builtin transports.

1. git://
   A normal request to git-daemon is structured as
   "command path/to/repo\0host=..\0" and due to a bug introduced in
   49ba83fb6 (Add virtualization support to git-daemon, 2006-09-19) we
   aren't able to place any extra arguments (separated by NULs) besides
   the host otherwise the parsing of those arguments would enter an
   infinite loop.  This bug was fixed in 73bb33a94 (daemon: Strictly
   parse the "extra arg" part of the command, 2009-06-04) but a check
   was put in place to disallow extra arguments so that new clients
   wouldn't trigger this bug in older servers.

   In order to get around this limitation git-daemon was taught to
   recognize additional request arguments hidden behind a second
   NUL byte.  Requests can then be structured like:
   "command path/to/repo\0host=..\0\0version=1\0key=value\0".
   git-daemon can then parse out the extra arguments and set
   'GIT_PROTOCOL' accordingly.

   By placing these extra arguments behind a second NUL byte we can
   skirt around both the infinite loop bug in 49ba83fb6 (Add
   virtualization support to git-daemon, 2006-09-19) as well as the
   explicit disallowing of extra arguments introduced in 73bb33a94
   (daemon: Strictly parse the "extra arg" part of the command,
   2009-06-04) because both of these versions of git-daemon check for a
   single NUL byte after the host argument before terminating the
   argument parsing.

2. ssh://, file://
   Set 'GIT_PROTOCOL' environment variable with the desired protocol
   version.  With the file:// transport, 'GIT_PROTOCOL' can be set
   explicitly in the locally running git-upload-pack or git-receive-pack
   processes.  With the ssh:// transport and OpenSSH compliant ssh
   programs, 'GIT_PROTOCOL' can be sent across ssh by using '-o
   SendEnv=GIT_PROTOCOL' and having the server whitelist this
   environment variable.

Signed-off-by: Brandon Williams 
---
 connect.c  |  37 ++--
 t/t5700-protocol-v1.sh | 223 +
 2 files changed, 255 insertions(+), 5 deletions(-)
 create mode 100755 t/t5700-protocol-v1.sh

diff --git a/connect.c b/connect.c
index a5e708a61..b8695a2fa 100644
--- a/connect.c
+++ b/connect.c
@@ -871,6 +871,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
} else if (protocol == PROTO_GIT) {
+   struct strbuf request = STRBUF_INIT;
/*
 * Set up virtual host information based on where we will
 * connect, unless the user has overridden us in
@@ -898,13 +899,25 @@ struct child_process *git_connect(int fd[2], const char 
*url,
 * Note: Do not add any other headers here!  Doing so
 * will cause older git-daemon servers to crash.
 */
-   packet_write_fmt(fd[1],
-"%s %s%chost=%s%c",
-prog, path, 0,
-target_host, 0);
+   strbuf_addf(,
+   "%s %s%chost=%s%c",
+   prog, path, 0,
+   target_host, 0);
+
+   /* If using a new version put that stuff here after a second 
null byte */
+   if (get_protocol_version_config() > 0) {
+   strbuf_addch(, '\0');
+   strbuf_addf(, "version=%d%c",
+   get_protocol_version_config(), '\0');
+   }
+
+   packet_write(fd[1], request.buf, request.len);
+
free(target_host);
+   strbuf_release();
} else {
struct strbuf cmd = STRBUF_INIT;
+   const char *const *var;
 
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
@@ -917,7 +930,9 @@ struct child_process *git_connect(int fd[2], const char 
*url,
sq_quote_buf(, path);
 
/* remove repo-local variables from the environment */
-   conn->env = local_repo_env;
+   for (var = local_repo_env; *var; var++)
+   argv_array_push(>env_array, *var);
+
conn->use_shell = 1;
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
@@ -971,6 +986,14 @@ struct child_process *git_connect(int fd[2], const char 
*url,
}
 
argv_array_push(>args, ssh);
+
+   if (get_protocol_version_config() > 0) {
+   argv_array_push(>args, "-o");
+   argv_array_push(>args, "SendEnv=" 
GIT_PROTOCOL_ENVIRONMENT);
+   

[PATCH v3 08/10] http: tell server that the client understands v1

[Brandon Williams]

Tell a server that protocol v1 can be used by sending the http header
'Git-Protocol' indicating this.

Also teach the apache http server to pass through the 'Git-Protocol'
header as an environment variable 'GIT_PROTOCOL'.

Signed-off-by: Brandon Williams 
---
 cache.h |  2 ++
 http.c  | 18 +
 t/lib-httpd/apache.conf |  7 +
 t/t5700-protocol-v1.sh  | 69 +
 4 files changed, 96 insertions(+)

diff --git a/cache.h b/cache.h
index c74b73671..3a6b869c2 100644
--- a/cache.h
+++ b/cache.h
@@ -452,6 +452,8 @@ static inline enum object_type object_type(unsigned int 
mode)
  * ignored.
  */
 #define GIT_PROTOCOL_ENVIRONMENT "GIT_PROTOCOL"
+/* HTTP header used to handshake the wire protocol */
+#define GIT_PROTOCOL_HEADER "Git-Protocol"
 
 /*
  * This environment variable is expected to contain a boolean indicating
diff --git a/http.c b/http.c
index 9e40a465f..ffb719216 100644
--- a/http.c
+++ b/http.c
@@ -12,6 +12,7 @@
 #include "gettext.h"
 #include "transport.h"
 #include "packfile.h"
+#include "protocol.h"
 
 static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
 #if LIBCURL_VERSION_NUM >= 0x070a08
@@ -897,6 +898,21 @@ static void set_from_env(const char **var, const char 
*envname)
*var = val;
 }
 
+static void protocol_http_header(void)
+{
+   if (get_protocol_version_config() > 0) {
+   struct strbuf protocol_header = STRBUF_INIT;
+
+   strbuf_addf(_header, GIT_PROTOCOL_HEADER ": 
version=%d",
+   get_protocol_version_config());
+
+
+   extra_http_headers = curl_slist_append(extra_http_headers,
+  protocol_header.buf);
+   strbuf_release(_header);
+   }
+}
+
 void http_init(struct remote *remote, const char *url, int proactive_auth)
 {
char *low_speed_limit;
@@ -927,6 +943,8 @@ void http_init(struct remote *remote, const char *url, int 
proactive_auth)
if (remote)
var_override(_proxy_authmethod, 
remote->http_proxy_authmethod);
 
+   protocol_http_header();
+
pragma_header = curl_slist_append(http_copy_default_headers(),
"Pragma: no-cache");
no_pragma_header = curl_slist_append(http_copy_default_headers(),
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 0642ae7e6..df1943631 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -67,6 +67,9 @@ LockFile accept.lock
 
LoadModule unixd_module modules/mod_unixd.so
 
+
+   LoadModule setenvif_module modules/mod_setenvif.so
+
 
 
 PassEnv GIT_VALGRIND
@@ -76,6 +79,10 @@ PassEnv ASAN_OPTIONS
 PassEnv GIT_TRACE
 PassEnv GIT_CONFIG_NOSYSTEM
 
+= 2.4>
+   SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
+
+
 Alias /dumb/ www/
 Alias /auth/dumb/ www/auth/dumb/
 
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index 6551932da..b0779d362 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -220,4 +220,73 @@ test_expect_success 'push with ssh:// using protocol v1' '
grep "push< version 1" log
 '
 
+# Test protocol v1 with 'http://' transport
+#
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'create repo to be served by http:// transport' '
+   git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+   git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" config http.receivepack 
true &&
+   test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one
+'
+
+test_expect_success 'clone with http:// using protocol v1' '
+   GIT_TRACE_PACKET=1 GIT_TRACE_CURL=1 git -c protocol.version=1 \
+   clone "$HTTPD_URL/smart/http_parent" http_child 2>log &&
+
+   git -C http_child log -1 --format=%s >actual &&
+   git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s 
>expect &&
+   test_cmp expect actual &&
+
+   # Client requested to use protocol v1
+   grep "Git-Protocol: version=1" log &&
+   # Server responded using protocol v1
+   grep "git< version 1" log
+'
+
+test_expect_success 'fetch with http:// using protocol v1' '
+   test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" two &&
+
+   GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
+   fetch 2>log &&
+
+   git -C http_child log -1 --format=%s origin/master >actual &&
+   git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s 
>expect &&
+   test_cmp expect actual &&
+
+   # Server responded using protocol v1
+   grep "git< version 1" log
+'
+
+test_expect_success 'pull with http:// using protocol v1' '
+   GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
+   pull 2>log &&
+
+   git -C http_child log -1 --format=%s >actual &&
+   git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s 
>expect &&
+   test_cmp expect actual &&
+

[PATCH v3 02/10] pkt-line: add packet_write function

[Brandon Williams]

Add a function which can be used to write the contents of an arbitrary
buffer.  This makes it easy to build up data in a buffer before writing
the packet instead of formatting the entire contents of the packet using
'packet_write_fmt()'.

Signed-off-by: Brandon Williams 
---
 pkt-line.c | 6 ++
 pkt-line.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index 647bbd3bc..c025d0332 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -188,6 +188,12 @@ static int packet_write_gently(const int fd_out, const 
char *buf, size_t size)
return 0;
 }
 
+void packet_write(const int fd_out, const char *buf, size_t size)
+{
+   if (packet_write_gently(fd_out, buf, size))
+   die_errno("packet write failed");
+}
+
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 {
va_list args;
diff --git a/pkt-line.h b/pkt-line.h
index 66ef610fc..d9e9783b1 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -22,6 +22,7 @@
 void packet_flush(int fd);
 void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format 
(printf, 2, 3)));
 void packet_buf_flush(struct strbuf *buf);
+void packet_write(const int fd_out, const char *buf, size_t size);
 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)));
-- 
2.14.2.920.gcf0c67979c-goog

[PATCH v3 01/10] connect: in ref advertisement, shallows are last

[Brandon Williams]

From: Jonathan Tan 

Currently, get_remote_heads() parses the ref advertisement in one loop,
allowing refs and shallow lines to intersperse, despite this not being
allowed by the specification. Refactor get_remote_heads() to use two
loops instead, enforcing that refs come first, and then shallows.

This also makes it easier to teach get_remote_heads() to interpret other
lines in the ref advertisement, which will be done in a subsequent
patch.

As part of this change, this patch interprets capabilities only on the
first line in the ref advertisement, printing a warning message when
encountering capabilities on other lines.

Signed-off-by: Jonathan Tan 
Signed-off-by: Brandon Williams 
---
 connect.c | 189 --
 1 file changed, 123 insertions(+), 66 deletions(-)

diff --git a/connect.c b/connect.c
index df56c0cbf..8e2e276b6 100644
--- a/connect.c
+++ b/connect.c
@@ -11,6 +11,7 @@
 #include "string-list.h"
 #include "sha1-array.h"
 #include "transport.h"
+#include "strbuf.h"
 
 static char *server_capabilities;
 static const char *parse_feature_value(const char *, const char *, int *);
@@ -107,6 +108,104 @@ static void annotate_refs_with_symref_info(struct ref 
*ref)
string_list_clear(, 0);
 }
 
+/*
+ * Read one line of a server's ref advertisement into packet_buffer.
+ */
+static int read_remote_ref(int in, char **src_buf, size_t *src_len,
+  int *responded)
+{
+   int len = packet_read(in, src_buf, src_len,
+ packet_buffer, sizeof(packet_buffer),
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+   const char *arg;
+   if (len < 0)
+   die_initial_contact(*responded);
+   if (len > 4 && skip_prefix(packet_buffer, "ERR ", ))
+   die("remote error: %s", arg);
+
+   *responded = 1;
+
+   return len;
+}
+
+#define EXPECTING_FIRST_REF 0
+#define EXPECTING_REF 1
+#define EXPECTING_SHALLOW 2
+
+static void process_capabilities(int *len)
+{
+   int nul_location = strlen(packet_buffer);
+   if (nul_location == *len)
+   return;
+   server_capabilities = xstrdup(packet_buffer + nul_location + 1);
+   *len = nul_location;
+}
+
+static int process_dummy_ref(void)
+{
+   struct object_id oid;
+   const char *name;
+
+   if (parse_oid_hex(packet_buffer, , ))
+   return 0;
+   if (*name != ' ')
+   return 0;
+   name++;
+
+   return !oidcmp(_oid, ) && !strcmp(name, "capabilities^{}");
+}
+
+static void check_no_capabilities(int len)
+{
+   if (strlen(packet_buffer) != len)
+   warning("Ignoring capabilities after first line '%s'",
+   packet_buffer + strlen(packet_buffer));
+}
+
+static int process_ref(int len, struct ref ***list, unsigned int flags,
+  struct oid_array *extra_have)
+{
+   struct object_id old_oid;
+   const char *name;
+
+   if (parse_oid_hex(packet_buffer, _oid, ))
+   return 0;
+   if (*name != ' ')
+   return 0;
+   name++;
+
+   if (extra_have && !strcmp(name, ".have")) {
+   oid_array_append(extra_have, _oid);
+   } else if (!strcmp(name, "capabilities^{}")) {
+   die("protocol error: unexpected capabilities^{}");
+   } else if (check_ref(name, flags)) {
+   struct ref *ref = alloc_ref(name);
+   oidcpy(>old_oid, _oid);
+   **list = ref;
+   *list = >next;
+   }
+   check_no_capabilities(len);
+   return 1;
+}
+
+static int process_shallow(int len, struct oid_array *shallow_points)
+{
+   const char *arg;
+   struct object_id old_oid;
+
+   if (!skip_prefix(packet_buffer, "shallow ", ))
+   return 0;
+
+   if (get_oid_hex(arg, _oid))
+   die("protocol error: expected shallow sha-1, got '%s'", arg);
+   if (!shallow_points)
+   die("repository on the other end cannot be shallow");
+   oid_array_append(shallow_points, _oid);
+   check_no_capabilities(len);
+   return 1;
+}
+
 /*
  * Read all the refs from the other end
  */
@@ -123,76 +222,34 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t src_len,
 * willing to talk to us.  A hang-up before seeing any
 * response does not necessarily mean an ACL problem, though.
 */
-   int saw_response;
-   int got_dummy_ref_with_capabilities_declaration = 0;
+   int responded = 0;
+   int len;
+   int state = EXPECTING_FIRST_REF;
 
*list = NULL;
-   for (saw_response = 0; ; saw_response = 1) {
-   struct ref *ref;
-   struct object_id old_oid;
-   char *name;
-   int len, name_len;
-   char *buffer = 

[PATCH v3 05/10] upload-pack, receive-pack: introduce protocol version 1

[Brandon Williams]

Teach upload-pack and receive-pack to understand and respond using
protocol version 1, if requested.

Protocol version 1 is simply the original and current protocol (what I'm
calling version 0) with the addition of a single packet line, which
precedes the ref advertisement, indicating the protocol version being
spoken.

Signed-off-by: Brandon Williams 
---
 builtin/receive-pack.c | 15 +++
 upload-pack.c  | 18 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index dd06b3fb4..94b7d29ea 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -24,6 +24,7 @@
 #include "tmp-objdir.h"
 #include "oidset.h"
 #include "packfile.h"
+#include "protocol.h"
 
 static const char * const receive_pack_usage[] = {
N_("git receive-pack "),
@@ -1963,6 +1964,20 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
else if (0 <= receive_unpack_limit)
unpack_limit = receive_unpack_limit;
 
+   switch (determine_protocol_version_server()) {
+   case protocol_v1:
+   if (advertise_refs || !stateless_rpc)
+   packet_write_fmt(1, "version 1\n");
+   /*
+* v1 is just the original protocol with a version string,
+* so just fall through after writing the version string.
+*/
+   case protocol_v0:
+   break;
+   default:
+   BUG("unknown protocol version");
+   }
+
if (advertise_refs || !stateless_rpc) {
write_head_info();
}
diff --git a/upload-pack.c b/upload-pack.c
index 7efff2fbf..ef438e9c2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -18,6 +18,7 @@
 #include "parse-options.h"
 #include "argv-array.h"
 #include "prio-queue.h"
+#include "protocol.h"
 
 static const char * const upload_pack_usage[] = {
N_("git upload-pack [] "),
@@ -1067,6 +1068,21 @@ int cmd_main(int argc, const char **argv)
die("'%s' does not appear to be a git repository", dir);
 
git_config(upload_pack_config, NULL);
-   upload_pack();
+
+   switch (determine_protocol_version_server()) {
+   case protocol_v1:
+   if (advertise_refs || !stateless_rpc)
+   packet_write_fmt(1, "version 1\n");
+   /*
+* v1 is just the original protocol with a version string,
+* so just fall through after writing the version string.
+*/
+   case protocol_v0:
+   upload_pack();
+   break;
+   default:
+   BUG("unknown protocol version");
+   }
+
return 0;
 }
-- 
2.14.2.920.gcf0c67979c-goog

[PATCH v3 09/10] i5700: add interop test for protocol transition

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 t/interop/i5700-protocol-transition.sh | 68 ++
 1 file changed, 68 insertions(+)
 create mode 100755 t/interop/i5700-protocol-transition.sh

diff --git a/t/interop/i5700-protocol-transition.sh 
b/t/interop/i5700-protocol-transition.sh
new file mode 100755
index 0..97e8e580e
--- /dev/null
+++ b/t/interop/i5700-protocol-transition.sh
@@ -0,0 +1,68 @@
+#!/bin/sh
+
+VERSION_A=.
+VERSION_B=v2.0.0
+
+: ${LIB_GIT_DAEMON_PORT:=5700}
+LIB_GIT_DAEMON_COMMAND='git.b daemon'
+
+test_description='clone and fetch by client who is trying to use a new 
protocol'
+. ./interop-lib.sh
+. "$TEST_DIRECTORY"/lib-git-daemon.sh
+
+start_git_daemon --export-all
+
+repo=$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo
+
+test_expect_success "create repo served by $VERSION_B" '
+   git.b init "$repo" &&
+   git.b -C "$repo" commit --allow-empty -m one
+'
+
+test_expect_success "git:// clone with $VERSION_A and protocol v1" '
+   GIT_TRACE_PACKET=1 git.a -c protocol.version=1 clone 
"$GIT_DAEMON_URL/repo" child 2>log &&
+   git.a -C child log -1 --format=%s >actual &&
+   git.b -C "$repo" log -1 --format=%s >expect &&
+   test_cmp expect actual &&
+   grep "version=1" log
+'
+
+test_expect_success "git:// fetch with $VERSION_A and protocol v1" '
+   git.b -C "$repo" commit --allow-empty -m two &&
+   git.b -C "$repo" log -1 --format=%s >expect &&
+
+   GIT_TRACE_PACKET=1 git.a -C child -c protocol.version=1 fetch 2>log &&
+   git.a -C child log -1 --format=%s FETCH_HEAD >actual &&
+
+   test_cmp expect actual &&
+   grep "version=1" log &&
+   ! grep "version 1" log
+'
+
+stop_git_daemon
+
+test_expect_success "create repo served by $VERSION_B" '
+   git.b init parent &&
+   git.b -C parent commit --allow-empty -m one
+'
+
+test_expect_success "file:// clone with $VERSION_A and protocol v1" '
+   GIT_TRACE_PACKET=1 git.a -c protocol.version=1 clone 
--upload-pack="git.b upload-pack" parent child2 2>log &&
+   git.a -C child2 log -1 --format=%s >actual &&
+   git.b -C parent log -1 --format=%s >expect &&
+   test_cmp expect actual &&
+   ! grep "version 1" log
+'
+
+test_expect_success "file:// fetch with $VERSION_A and protocol v1" '
+   git.b -C parent commit --allow-empty -m two &&
+   git.b -C parent log -1 --format=%s >expect &&
+
+   GIT_TRACE_PACKET=1 git.a -C child2 -c protocol.version=1 fetch 
--upload-pack="git.b upload-pack" 2>log &&
+   git.a -C child2 log -1 --format=%s FETCH_HEAD >actual &&
+
+   test_cmp expect actual &&
+   ! grep "version 1" log
+'
+
+test_done
-- 
2.14.2.920.gcf0c67979c-goog

[PATCH v3 04/10] daemon: recognize hidden request arguments

[Brandon Williams]

A normal request to git-daemon is structured as
"command path/to/repo\0host=..\0" and due to a bug introduced in
49ba83fb6 (Add virtualization support to git-daemon, 2006-09-19) we
aren't able to place any extra arguments (separated by NULs) besides the
host otherwise the parsing of those arguments would enter an infinite
loop.  This bug was fixed in 73bb33a94 (daemon: Strictly parse the
"extra arg" part of the command, 2009-06-04) but a check was put in
place to disallow extra arguments so that new clients wouldn't trigger
this bug in older servers.

In order to get around this limitation teach git-daemon to recognize
additional request arguments hidden behind a second NUL byte.  Requests
can then be structured like:
"command path/to/repo\0host=..\0\0version=1\0key=value\0".  git-daemon
can then parse out the extra arguments and set 'GIT_PROTOCOL'
accordingly.

By placing these extra arguments behind a second NUL byte we can skirt
around both the infinite loop bug in 49ba83fb6 (Add virtualization
support to git-daemon, 2006-09-19) as well as the explicit disallowing
of extra arguments introduced in 73bb33a94 (daemon: Strictly parse the
"extra arg" part of the command, 2009-06-04) because both of these
versions of git-daemon check for a single NUL byte after the host
argument before terminating the argument parsing.

Signed-off-by: Brandon Williams 
---
 daemon.c | 68 +++-
 1 file changed, 59 insertions(+), 9 deletions(-)

diff --git a/daemon.c b/daemon.c
index 30747075f..36cc794c9 100644
--- a/daemon.c
+++ b/daemon.c
@@ -282,7 +282,7 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
return NULL;/* Fallthrough. Deny by default */
 }
 
-typedef int (*daemon_service_fn)(void);
+typedef int (*daemon_service_fn)(const struct argv_array *env);
 struct daemon_service {
const char *name;
const char *config_name;
@@ -363,7 +363,7 @@ static int run_access_hook(struct daemon_service *service, 
const char *dir,
 }
 
 static int run_service(const char *dir, struct daemon_service *service,
-  struct hostinfo *hi)
+  struct hostinfo *hi, const struct argv_array *env)
 {
const char *path;
int enabled = service->enabled;
@@ -422,7 +422,7 @@ static int run_service(const char *dir, struct 
daemon_service *service,
 */
signal(SIGTERM, SIG_IGN);
 
-   return service->fn();
+   return service->fn(env);
 }
 
 static void copy_to_log(int fd)
@@ -462,25 +462,34 @@ static int run_service_command(struct child_process *cld)
return finish_command(cld);
 }
 
-static int upload_pack(void)
+static int upload_pack(const struct argv_array *env)
 {
struct child_process cld = CHILD_PROCESS_INIT;
argv_array_pushl(, "upload-pack", "--strict", NULL);
argv_array_pushf(, "--timeout=%u", timeout);
+
+   argv_array_pushv(_array, env->argv);
+
return run_service_command();
 }
 
-static int upload_archive(void)
+static int upload_archive(const struct argv_array *env)
 {
struct child_process cld = CHILD_PROCESS_INIT;
argv_array_push(, "upload-archive");
+
+   argv_array_pushv(_array, env->argv);
+
return run_service_command();
 }
 
-static int receive_pack(void)
+static int receive_pack(const struct argv_array *env)
 {
struct child_process cld = CHILD_PROCESS_INIT;
argv_array_push(, "receive-pack");
+
+   argv_array_pushv(_array, env->argv);
+
return run_service_command();
 }
 
@@ -574,7 +583,7 @@ static void canonicalize_client(struct strbuf *out, const 
char *in)
 /*
  * Read the host as supplied by the client connection.
  */
-static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
+static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
 {
char *val;
int vallen;
@@ -602,6 +611,43 @@ static void parse_host_arg(struct hostinfo *hi, char 
*extra_args, int buflen)
if (extra_args < end && *extra_args)
die("Invalid request");
}
+
+   return extra_args;
+}
+
+static void parse_extra_args(struct hostinfo *hi, struct argv_array *env,
+char *extra_args, int buflen)
+{
+   const char *end = extra_args + buflen;
+   struct strbuf git_protocol = STRBUF_INIT;
+
+   /* First look for the host argument */
+   extra_args = parse_host_arg(hi, extra_args, buflen);
+
+   /* Look for additional arguments places after a second NUL byte */
+   for (; extra_args < end; extra_args += strlen(extra_args) + 1) {
+   const char *arg = extra_args;
+
+   /*
+* Parse the extra arguments, adding most to 'git_protocol'
+* which will be used to set the 'GIT_PROTOCOL' envvar in the
+* service that will be run.
+*
+ 

[PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Brandon Williams]

Create protocol.{c,h} and provide functions which future servers and
clients can use to determine which protocol to use or is being used.

Also introduce the 'GIT_PROTOCOL' environment variable which will be
used to communicate a colon separated list of keys with optional values
to a server.  Unknown keys and values must be tolerated.  This mechanism
is used to communicate which version of the wire protocol a client would
like to use with a server.

Signed-off-by: Brandon Williams 
---
 Documentation/config.txt | 17 +++
 Documentation/git.txt|  6 
 Makefile |  1 +
 cache.h  |  8 +
 protocol.c   | 79 
 protocol.h   | 33 
 6 files changed, 144 insertions(+)
 create mode 100644 protocol.c
 create mode 100644 protocol.h

diff --git a/Documentation/config.txt b/Documentation/config.txt
index dc4e3f58a..b78747abc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2517,6 +2517,23 @@ The protocol names currently used by git are:
 `hg` to allow the `git-remote-hg` helper)
 --
 
+protocol.version::
+   Experimental. If set, clients will attempt to communicate with a
+   server using the specified protocol version.  If unset, no
+   attempt will be made by the client to communicate using a
+   particular protocol version, this results in protocol version 0
+   being used.
+   Supported versions:
++
+--
+
+* `0` - the original wire protocol.
+
+* `1` - the original wire protocol with the addition of a version string
+  in the initial response from the server.
+
+--
+
 pull.ff::
By default, Git does not create an extra merge commit when merging
a commit that is a descendant of the current commit. Instead, the
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6e3a6767e..7518ea3af 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -697,6 +697,12 @@ of clones and fetches.
which feed potentially-untrusted URLS to git commands.  See
linkgit:git-config[1] for more details.
 
+`GIT_PROTOCOL`::
+   For internal use only.  Used in handshaking the wire protocol.
+   Contains a colon ':' separated list of keys with optional values
+   'key[=value]'.  Presence of unknown keys and values must be
+   ignored.
+
 Discussion[[Discussion]]
 
 
diff --git a/Makefile b/Makefile
index ed4ca438b..9ce68cded 100644
--- a/Makefile
+++ b/Makefile
@@ -842,6 +842,7 @@ LIB_OBJS += pretty.o
 LIB_OBJS += prio-queue.o
 LIB_OBJS += progress.o
 LIB_OBJS += prompt.o
+LIB_OBJS += protocol.o
 LIB_OBJS += quote.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
diff --git a/cache.h b/cache.h
index 49b083ee0..c74b73671 100644
--- a/cache.h
+++ b/cache.h
@@ -445,6 +445,14 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define GIT_ICASE_PATHSPECS_ENVIRONMENT "GIT_ICASE_PATHSPECS"
 #define GIT_QUARANTINE_ENVIRONMENT "GIT_QUARANTINE_PATH"
 
+/*
+ * Environment variable used in handshaking the wire protocol.
+ * Contains a colon ':' separated list of keys with optional values
+ * 'key[=value]'.  Presence of unknown keys and values must be
+ * ignored.
+ */
+#define GIT_PROTOCOL_ENVIRONMENT "GIT_PROTOCOL"
+
 /*
  * This environment variable is expected to contain a boolean indicating
  * whether we should or should not treat:
diff --git a/protocol.c b/protocol.c
new file mode 100644
index 0..43012b7eb
--- /dev/null
+++ b/protocol.c
@@ -0,0 +1,79 @@
+#include "cache.h"
+#include "config.h"
+#include "protocol.h"
+
+static enum protocol_version parse_protocol_version(const char *value)
+{
+   if (!strcmp(value, "0"))
+   return protocol_v0;
+   else if (!strcmp(value, "1"))
+   return protocol_v1;
+   else
+   return protocol_unknown_version;
+}
+
+enum protocol_version get_protocol_version_config(void)
+{
+   const char *value;
+   if (!git_config_get_string_const("protocol.version", )) {
+   enum protocol_version version = parse_protocol_version(value);
+
+   if (version == protocol_unknown_version)
+   die("unknown value for config 'protocol.version': %s",
+   value);
+
+   return version;
+   }
+
+   return protocol_v0;
+}
+
+enum protocol_version determine_protocol_version_server(void)
+{
+   const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);
+   enum protocol_version version = protocol_v0;
+
+   /*
+* Determine which protocol version the client has requested.  Since
+* multiple 'version' keys can be sent by the client, indicating that
+* the client is okay to speak any of them, select the greatest version
+* that the client has requested.  This is due to the assumption that
+* the most recent protocol version will be 

[PATCH v3 00/10] protocol transition

[Brandon Williams]

Changes in v3:
 * added a new ssh variant 'simple' and update documentation to better reflect
   the command-line parameters passed to the ssh command.
 * updated various commit messages based on feedback.
 * tighten the wording for 'GIT_PROTOCOL' to indicate that both unknown keys
   and values must be ignored.
 * added API comments for functions in protocol.h
 * updated various tests in t5700 based on reviewer feedback

Brandon Williams (9):
  pkt-line: add packet_write function
  protocol: introduce protocol extention mechanisms
  daemon: recognize hidden request arguments
  upload-pack, receive-pack: introduce protocol version 1
  connect: teach client to recognize v1 server response
  connect: tell server that the client understands v1
  http: tell server that the client understands v1
  i5700: add interop test for protocol transition
  ssh: introduce a 'simple' ssh variant

Jonathan Tan (1):
  connect: in ref advertisement, shallows are last

 Documentation/config.txt   |  44 +++-
 Documentation/git.txt  |  15 +-
 Makefile   |   1 +
 builtin/receive-pack.c |  15 ++
 cache.h|  10 +
 connect.c  | 353 ++---
 daemon.c   |  68 ++-
 http.c |  18 ++
 pkt-line.c |   6 +
 pkt-line.h |   1 +
 protocol.c |  79 
 protocol.h |  33 +++
 t/interop/i5700-protocol-transition.sh |  68 +++
 t/lib-httpd/apache.conf|   7 +
 t/t5601-clone.sh   |   9 +-
 t/t5700-protocol-v1.sh | 294 +++
 upload-pack.c  |  18 +-
 17 files changed, 900 insertions(+), 139 deletions(-)
 create mode 100644 protocol.c
 create mode 100644 protocol.h
 create mode 100755 t/interop/i5700-protocol-transition.sh
 create mode 100755 t/t5700-protocol-v1.sh

--- interdiff with 'origin/bw/protocol-v1'

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b78747abc..0460af37e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2084,12 +2084,31 @@ ssh.variant::
Depending on the value of the environment variables `GIT_SSH` or
`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
auto-detects whether to adjust its command-line parameters for use
-   with plink or tortoiseplink, as opposed to the default (OpenSSH).
+   with ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
+   (simple).
 +
 The config variable `ssh.variant` can be set to override this auto-detection;
-valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
-will be treated as normal ssh. This setting can be overridden via the
-environment variable `GIT_SSH_VARIANT`.
+valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any
+other value will be treated as normal ssh. This setting can be overridden via
+the environment variable `GIT_SSH_VARIANT`.
++
+The current command-line parameters used for each variant are as
+follows:
++
+--
+
+* `ssh` - [-p port] [-4] [-6] [-o option] [username@]host command
+
+* `simple` - [username@]host command
+
+* `plink` or `putty` - [-P port] [-4] [-6] [username@]host command
+
+* `tortoiseplink` - [-P port] [-4] [-6] -batch [username@]host command
+
+--
++
+Except for the `simple` variant, command-line parameters are likely to
+change as git gains new features.
 
 i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 299f75c7b..8bc3f2147 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -518,11 +518,10 @@ other
If either of these environment variables is set then 'git fetch'
and 'git push' will use the specified command instead of 'ssh'
when they need to connect to a remote system.
-   The command will be given exactly two or four arguments: the
-   'username@host' (or just 'host') from the URL and the shell
-   command to execute on that remote system, optionally preceded by
-   `-p` (literally) and the 'port' from the URL when it specifies
-   something other than the default SSH port.
+   The command-line parameters passed to the configured command are
+   determined by the ssh variant.  See `ssh.variant` option in
+   linkgit:git-config[1] for details.
+
 +
 `$GIT_SSH_COMMAND` takes precedence over `$GIT_SSH`, and is interpreted
 by the shell, which allows additional arguments to be included.
@@ -700,7 +699,8 @@ of clones and fetches.
 `GIT_PROTOCOL`::
For internal use only.  Used in handshaking the wire protocol.
Contains a colon ':' separated list of keys with optional values
-   

[PATCH 3/3] sub-process: allocate argv on the heap

[Thomas Gummerer]

Currently the argv is only allocated on the stack, and then assigned to
process->argv.  When the start_subprocess function goes out of scope,
the local argv variable is eliminated from the stack, but the pointer is
still kept around in process->argv.

Much later when we try to access the same process->argv in
finish_command, this leads us to access a memory location that no longer
contains what we want.  As argv0 is only used for printing errors, this
is not easily noticed in normal git operations.  However when running
t0021-conversion.sh through valgrind, valgrind rightfully complains:

==21024== Invalid read of size 8
==21024==at 0x2ACF64: finish_command (run-command.c:869)
==21024==by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)
==21024==by 0x2AB41E: cleanup_children (run-command.c:45)
==21024==by 0x2AB526: cleanup_children_on_exit (run-command.c:81)
==21024==by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)
==21024==by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)
==21024==by 0x11A9EF: handle_builtin (git.c:550)
==21024==by 0x11ABCC: run_argv (git.c:602)
==21024==by 0x11AD8E: cmd_main (git.c:679)
==21024==by 0x1BF125: main (common-main.c:43)
==21024==  Address 0x1ffeffec00 is on thread 1's stack
==21024==  1504 bytes below stack pointer
==21024==

Fix this by allocating the memory on properly on the heap.  This memory
is allocated on the heap, and never free'd.  However the same seems to be
true for struct child_process, so it should be fine to just let the
memory be free'd when the process terminates.

Signed-off-by: Thomas Gummerer 
---
 sub-process.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sub-process.c b/sub-process.c
index 6dde5062be..4680af8193 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct 
subprocess_entry *entry, co
 {
int err;
struct child_process *process;
-   const char *argv[] = { cmd, NULL };
+   const char **argv = xmalloc(2 * sizeof(char *));
+   argv[0] = cmd;
+   argv[1] = NULL;
 
entry->cmd = cmd;
process = >process;
-- 
2.14.1.480.gb18f417b89

[PATCH 0/3] fixes for running the test suite with --valgrind

[Thomas Gummerer]

I recently tried to run the git test suite with --valgrind.
Unfortunately it didn't come back completely clean.  This patch series
fixes a bunch of these errors, although unfortunately not quite all of
them yet.

The remaining failures are in

t0021.15 - This one is not actually valgrind complaining about
something, but the clean/smudge script not writing debug.log for some
reason.  I'm not quite sure what exactly is going on here.
t0021.25, t0021.26 - This is an actual uninitialized memory usage:

==4751== Conditional jump or move depends on uninitialised value(s)
==4751==at 0x2796D5: fill_stat_cache_info (read-cache.c:153)
==4751==by 0x2218A2: write_entry (entry.c:359)
==4751==by 0x221D42: checkout_entry (entry.c:458)
==4751==by 0x2EB627: check_updates (unpack-trees.c:382)
==4751==by 0x2EDBA1: unpack_trees (unpack-trees.c:1380)
==4751==by 0x13797E: checkout (clone.c:750)
==4751==by 0x138FF4: cmd_clone (clone.c:1194)
==4751==by 0x11A6F2: run_builtin (git.c:342)
==4751==by 0x11A9E8: handle_builtin (git.c:550)
==4751==by 0x11ABCC: run_argv (git.c:602)
==4751==by 0x11AD8E: cmd_main (git.c:679)
==4751==by 0x1BF125: main (common-main.c:43)
==4751==  Uninitialised value was created by a stack allocation
==4751==at 0x2212B4: write_entry (entry.c:254)
==4751== 

So far I've tracked this one down to the lstat call in write_entry
failing, and thus not filling struct stat_info.  I'm not quite sure
yet about the best workaround for that (and I'm not very familiar with
the clean/smudge code).  I'll keep digging what the problem there is.

There's also one test that's unexpectedly passing when the test suite
is run under valgrind (t6410.) but I haven't dug into what's up with
that yet.

These patches could be applied by themselves as well, but since they
work toward the same goal, and a cover letter would explain where
these are coming from I decided to make them into a patch series.

Thomas Gummerer (3):
  path.c: fix uninitialized memory access
  http-push: fix construction of hex value from path
  sub-process: allocate argv on the heap

 http-push.c   |  2 +-
 path.c| 19 ++-
 sub-process.c |  4 +++-
 3 files changed, 14 insertions(+), 11 deletions(-)

-- 
2.14.1.480.gb18f417b89

[PATCH 1/3] path.c: fix uninitialized memory access

[Thomas Gummerer]

In cleanup_path we're passing in a char array, run a memcmp on it, and
run through it without ever checking if something is in the array in the
first place.  This can lead us to access uninitialized memory, for
example in t5541-http-push-smart.sh test 7, when run under valgrind:

==4423== Conditional jump or move depends on uninitialised value(s)
==4423==at 0x242FA9: cleanup_path (path.c:35)
==4423==by 0x242FA9: mkpath (path.c:456)
==4423==by 0x256CC7: refname_match (refs.c:364)
==4423==by 0x26C181: count_refspec_match (remote.c:1015)
==4423==by 0x26C181: match_explicit_lhs (remote.c:1126)
==4423==by 0x26C181: check_push_refs (remote.c:1409)
==4423==by 0x2ABB4D: transport_push (transport.c:870)
==4423==by 0x186703: push_with_options (push.c:332)
==4423==by 0x18746D: do_push (push.c:409)
==4423==by 0x18746D: cmd_push (push.c:566)
==4423==by 0x1183E0: run_builtin (git.c:352)
==4423==by 0x11973E: handle_builtin (git.c:539)
==4423==by 0x11973E: run_argv (git.c:593)
==4423==by 0x11973E: main (git.c:698)
==4423==  Uninitialised value was created by a heap allocation
==4423==at 0x4C2CD8F: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4423==by 0x4C2F195: realloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4423==by 0x2C196B: xrealloc (wrapper.c:137)
==4423==by 0x29A30B: strbuf_grow (strbuf.c:66)
==4423==by 0x29A30B: strbuf_vaddf (strbuf.c:277)
==4423==by 0x242F9F: mkpath (path.c:454)
==4423==by 0x256CC7: refname_match (refs.c:364)
==4423==by 0x26C181: count_refspec_match (remote.c:1015)
==4423==by 0x26C181: match_explicit_lhs (remote.c:1126)
==4423==by 0x26C181: check_push_refs (remote.c:1409)
==4423==by 0x2ABB4D: transport_push (transport.c:870)
==4423==by 0x186703: push_with_options (push.c:332)
==4423==by 0x18746D: do_push (push.c:409)
==4423==by 0x18746D: cmd_push (push.c:566)
==4423==by 0x1183E0: run_builtin (git.c:352)
==4423==by 0x11973E: handle_builtin (git.c:539)
==4423==by 0x11973E: run_argv (git.c:593)
==4423==by 0x11973E: main (git.c:698)
==4423==

Avoid this by checking passing in the length of the string in the char
array, and checking that we never run over it.

Signed-off-by: Thomas Gummerer 
---
 path.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/path.c b/path.c
index b533ec938d..12f41ee877 100644
--- a/path.c
+++ b/path.c
@@ -34,20 +34,21 @@ static struct strbuf *get_pathname(void)
return sb;
 }
 
-static char *cleanup_path(char *path)
+static char *cleanup_path(char *path, int len)
 {
/* Clean it up */
-   if (!memcmp(path, "./", 2)) {
-   path += 2;
-   while (*path == '/')
-   path++;
+   int skip = 0;
+   if (len >= 2 && !memcmp(path, "./", 2)) {
+   skip += 2;
+   while (skip < len && *(path + skip) == '/')
+   skip++;
}
-   return path;
+   return path + skip;
 }
 
 static void strbuf_cleanup_path(struct strbuf *sb)
 {
-   char *path = cleanup_path(sb->buf);
+   char *path = cleanup_path(sb->buf, sb->len);
if (path > sb->buf)
strbuf_remove(sb, 0, path - sb->buf);
 }
@@ -64,7 +65,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
strlcpy(buf, bad_path, n);
return buf;
}
-   return cleanup_path(buf);
+   return cleanup_path(buf, n);
 }
 
 static int dir_prefix(const char *buf, const char *dir)
@@ -494,7 +495,7 @@ const char *mkpath(const char *fmt, ...)
va_start(args, fmt);
strbuf_vaddf(pathname, fmt, args);
va_end(args);
-   return cleanup_path(pathname->buf);
+   return cleanup_path(pathname->buf, pathname->len);
 }
 
 const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
-- 
2.14.1.480.gb18f417b89

[PATCH 2/3] http-push: fix construction of hex value from path

[Thomas Gummerer]

The get_oid_hex_from_objpath takes care of creating a oid from a
pathname.  It does this by memcpy'ing the first two bytes of the path to
the "hex" string, then skipping the '/', and then copying the rest of the
path to the "hex" string.  Currently it fails to increase the pointer to
the hex string, so the second memcpy invocation just mashes over what
was copied in the first one, and leaves the last two bytes in the string
uninitialized.

This breaks valgrind in t5540, although the test passes without
valgrind:

==5490== Use of uninitialised value of size 8
==5490==at 0x13C6B5: hexval (cache.h:1238)
==5490==by 0x13C6DB: hex2chr (cache.h:1247)
==5490==by 0x13C734: get_sha1_hex (hex.c:42)
==5490==by 0x13C78E: get_oid_hex (hex.c:53)
==5490==by 0x118BDA: get_oid_hex_from_objpath (http-push.c:1023)
==5490==by 0x118C92: process_ls_object (http-push.c:1038)
==5490==by 0x118E5B: handle_remote_ls_ctx (http-push.c:1077)
==5490==by 0x118227: xml_end_tag (http-push.c:815)
==5490==by 0x50C1448: ??? (in /usr/lib/libexpat.so.1.6.6)
==5490==by 0x50C221B: ??? (in /usr/lib/libexpat.so.1.6.6)
==5490==by 0x50BFBF2: ??? (in /usr/lib/libexpat.so.1.6.6)
==5490==by 0x50C0B24: ??? (in /usr/lib/libexpat.so.1.6.6)
==5490==  Uninitialised value was created by a stack allocation
==5490==at 0x118B63: get_oid_hex_from_objpath (http-push.c:1012)
==5490==

Fix this by correctly incrementing the pointer to the "hex" variable, so
the first two bytes are left untouched by the memcpy call, and the last
two bytes are correctly initialized.

Signed-off-by: Thomas Gummerer 
---
 http-push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http-push.c b/http-push.c
index e4c9b065ce..e9a01ec4da 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1018,7 +1018,7 @@ static int get_oid_hex_from_objpath(const char *path, 
struct object_id *oid)
memcpy(hex, path, 2);
path += 2;
path++; /* skip '/' */
-   memcpy(hex, path, GIT_SHA1_HEXSZ - 2);
+   memcpy(hex + 2, path, GIT_SHA1_HEXSZ - 2);
 
return get_oid_hex(hex, oid);
 }
-- 
2.14.1.480.gb18f417b89

Re: [PATCH v8 00/12] Fast git status via a file system watcher

[Ben Peart]

On 10/1/2017 4:24 AM, Junio C Hamano wrote:

Ben Peart  writes:


I had accumulated the same set of changes with one addition of removing
a duplicate "the" from a comment in the fsmonitor.h file:

diff --git a/fsmonitor.h b/fsmonitor.h
index 8eb6163455..0de644e01a 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -4,7 +4,7 @@
  extern struct trace_key trace_fsmonitor;
  
  /*

- * Read the the fsmonitor index extension and (if configured) restore the
+ * Read the fsmonitor index extension and (if configured) restore the
   * CE_FSMONITOR_VALID state.
   */
  extern int read_fsmonitor_extension(struct index_state *istate, const void 
*data, unsigned long sz);



Thanks.


OK, now my copy has the same, so we are in sync.  Unless there is no
more comment that benefits from a reroll of the series, let's run
with this version for now and merge it to 'next'.  Further updates
can be done incrementally on top.

Thanks.



Well, rats. I found one more issue that applies to two of the commits. 
Can you squash this in as well or do you want it in some other form?



diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index 7ceb32dc18..cca3d71e90 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -36,7 +36,7 @@ my $system = `uname -s`;
 $system =~ s/[\r\n]+//g;
 my $git_work_tree;

-if ($system =~ m/^MSYS_NT/) {
+if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree = `cygpath -aw "\$PWD"`;
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;
diff --git a/templates/hooks--fsmonitor-watchman.sample 
b/templates/hooks--fsmonitor-watchman.sample

index 870a59d237..c68038ef00 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -35,7 +35,7 @@ my $system = `uname -s`;
 $system =~ s/[\r\n]+//g;
 my $git_work_tree;

-if ($system =~ m/^MSYS_NT/) {
+if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree = `cygpath -aw "\$PWD"`;
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;

Re: git add -p stops working when setting color.ui = always

[Kevin Daudt]

On Mon, Oct 02, 2017 at 01:38:17PM +0300, Tsvi Mostovicz wrote:
> Hi,
> 
> When setting "color.ui = always" in the last few versions (not sure
> exactly when this started, but definitely exists in 2.14.1 and
> 2.14.2), git add -p stops working as expected. Instead of prompting
> the user, it skips through the prompts and doesn't allow selecting a
> hunk.
> 
> Don't know why I had color.ui = always set in my .gitconfig.
> 
> git version 2.14.2.666.gea220ee40
> 
> Thanks,
> 

Hello Tsvi,

This is being discussed just now[0]. Setting the value to auto should
fix it (setting it to always does not make much sense in your config
file).

Kevin.

[0}:https://public-inbox.org/git/20171003093157.gq7za2fwcqsou...@sigill.intra.peff.net/T/

Re: Line ending normalization doesn't work as expected

[Torsten Bögershausen]

On 2017-10-03 19:23, Robert Dailey wrote:
> On Tue, Oct 3, 2017 at 11:26 AM, Torsten Bögershausen  wrote:
>> The short version is, that the instructions on Github are outdated.
>> This is the official procedure (since 2016, Git v2.12 or so)
>> But it should work even with older version of Git.
>>
>> $ echo "* text=auto" >.gitattributes
>> $ git read-tree --empty   # Clean index, force re-scan of working directory
>> $ git add .
>> $ git status# Show files that will be normalized
>> $ git commit -m "Introduce end-of-line normalization"
> 
> Is the way I did it that worked also a valid solution? Or did it only
> work accidentally? Again the command I ran that worked is:
> 
> $ git rm -r --cached . && git add .

(Both should work)

To be honest, from the documentation, I can't figure out the difference between
$ git read-tree --empty
and
$ git rm -r --cached .

Does anybody remember the discussion, why we ended up with read-tree ?

Re: [PATCH v2] branch: change the error messages to be more meaningful

[Kaartic Sivaraam]

On Tue, 2017-10-03 at 09:21 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> I do not even recall what the patches did and if I thought what they
> wanted to do made sense, 


I thought you did or may be I misinterpreted the following statement,


On Thursday 17 August 2017 12:58 AM, Junio C Hamano wrote:
> I do not find the s/branch/parameter/ too bad (although I would have
> said "arguments" instead).
>

I interpreted the "not .. too bad" as a "it makes little sense". So,
pinged the thread.

---
Kaartic

Re: Line ending normalization doesn't work as expected

[Robert Dailey]

On Tue, Oct 3, 2017 at 11:26 AM, Torsten Bögershausen  wrote:
> The short version is, that the instructions on Github are outdated.
> This is the official procedure (since 2016, Git v2.12 or so)
> But it should work even with older version of Git.
>
> $ echo "* text=auto" >.gitattributes
> $ git read-tree --empty   # Clean index, force re-scan of working directory
> $ git add .
> $ git status# Show files that will be normalized
> $ git commit -m "Introduce end-of-line normalization"

Is the way I did it that worked also a valid solution? Or did it only
work accidentally? Again the command I ran that worked is:

$ git rm -r --cached . && git add .

Re: [PATCH v3 5/5] sha1_name: Minimize OID comparisons during disambiguation

[Derrick Stolee]

On 10/3/2017 11:55 AM, Stefan Beller wrote:

@@ -505,6 +506,65 @@ static int extend_abbrev_len(const struct object_id *oid, 
void *cb_data)
 return 0;
  }

+static void find_abbrev_len_for_pack(struct packed_git *p,
+struct min_abbrev_data *mad)
+{
+   int match = 0;
+   uint32_t num, last, first = 0;
+   struct object_id oid;
+
+   open_pack_index(p);

coverity complained here with
 Calling "open_pack_index" without checking return value
 (as is done elsewhere 13 out of 15 times).
Good catch! This same line is repeated in unique_in_pack() in this same 
file, so if this is worth fixing then we should probably fix it there, too.

I think the easiest way out is just a

 if (open_pack_index(p))
 die(_("Cannot open existing pack idx file for '%s'"), p);

or is there another good approach?

You probably intended to have p->pack_name in the die();

Using `cat *.c | grep -A 2 "if (open_pack_index("` and `cat */*.c | grep 
-A 2 "if (open_pack_index("` I see a few places that return error codes 
or quietly fail. The cases that use die() are inside builtin/ so I don't 
think die() is the right choice here.


Since find_abbrev_len_for_pack() is intended to extend the abbreviation 
length when necessary, I think a silent return is best here:


    if (open_pack_index(p))
        return;

Thanks,
-Stolee

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Stefan Beller]

On Tue, Oct 3, 2017 at 3:15 AM, Marius Paliga  wrote:
> There is a need to pass predefined push-option during "git push"
> without need to specify it explicitly.
>
> In another words we need to have a new "git config" variable to
> specify string that will be automatically passed as "--push-option"
> when pushing to remote.
>
> Something like the following:
>
> git config push.optionDefault AllowMultipleCommits
>
> and then command
>   git push
> would silently run
>   git push --push-option "AllowMultipleCommits"

We would need to
* design this feature (seems like you already have a good idea what you need)
* implement it (see builtin/push.c):
 - move "struct string_list push_options = STRING_LIST_INIT_DUP;"
  to be a file-static variable, such that we have access to it outside
of cmd_push.
 - In git_push_config in builtin/push.c that parses the config, we'd
need to check
  for "push.optionDefault" and add these to the push_options (I assume multiple
  are allowed)
* document it (Documentation/git-push.txt)
* add a test for it ? (t/t5545-push-options.sh)

Care to write a patch? Otherwise I'd mark it up as part of
#leftoverbits for now,
as it seems like a good starter project.

Thanks,
Stefan

[PATCH] emacs: work with remote filesystems

[Enrico Scholz]

With this patch, it is possible to work on remote filesystems which
were made accessible by tramp.

For example, 'M-x git-status /remote-host:/repository' will show the
status of /repository on 'remote-host' and usual operations like add
or commit are supported there.

First part of the is patch is trivial and replaces 'call-process' with
the network transparent 'process-file'.

The second one is more extensive and implements a tramp wrapper for
'call-process-region'.

Signed-off-by: Enrico Scholz 
---
 contrib/emacs/git.el | 32 ++--
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
index 5ffc506..3d9d691 100644
--- a/contrib/emacs/git.el
+++ b/contrib/emacs/git.el
@@ -190,8 +190,8 @@ if there is already one that displays the same directory."
   (mapcar (lambda (entry) (concat (car entry) "=" (cdr entry))) env))
 
 (defun git-call-process (buffer  args)
-  "Wrapper for call-process that sets environment strings."
-  (apply #'call-process "git" nil buffer nil args))
+  "Wrapper for process-file that sets environment strings."
+  (apply #'process-file "git" nil buffer nil args))
 
 (defun git-call-process-display-error ( args)
   "Wrapper for call-process that displays error messages."
@@ -221,14 +221,34 @@ the process output as a string, or nil if the git command 
failed."
   (display-message-or-buffer (current-buffer))
   nil)))
 
+(defun git-tramp-call-process-region (start end program
+ delete buffer display
+ args)
+  "call-process-region variant for tramp"
+  (let ((tmpfile (tramp-compat-make-temp-file "")))
+(unwind-protect
+(progn
+  (write-region start end tmpfile)
+  (when delete (delete-region start end))
+  (apply #'process-file program tmpfile buffer display args))
+  (delete-file tmpfile
+
 (defun git-run-process-region (buffer start end program args)
   "Run a git process with a buffer region as input."
-  (let ((output-buffer (current-buffer))
-(dir default-directory))
+  (let ((dir default-directory)
+(fh (find-file-name-handler default-directory 'call-process-region))
+(fnargs (apply 'list start end program
+   nil (list (current-buffer) t) nil args)))
 (with-current-buffer buffer
   (cd dir)
-  (apply #'call-process-region start end program
- nil (list output-buffer t) nil args
+  (case fh
+;; special handling for tramp
+(tramp-file-name-handler
+ (apply #'git-tramp-call-process-region fnargs))
+;; the default (local-file) handler
+((nil) (apply #'call-process-region fnargs))
+;; else, when there is a handler, call it
+(t (apply fh #'call-process-region fnargs))
 
 (defun git-run-command-buffer (buffer-name  args)
   "Run a git command, sending the output to a buffer named BUFFER-NAME."
-- 
2.9.5

Re: Line ending normalization doesn't work as expected

[Torsten Bögershausen]

On 2017-10-03 17:00, Robert Dailey wrote:
> I'm on Windows using Git for Windows v2.13.1. Following github's
> recommended process for fixing line endings after adding a
> `.gitattributes` file[1], I run the following:
> 
> $ rm .git/index && git reset
> 
> Once I run `git status`, I see that no files have changed. Note that I
> know for a fact in my repository, files were committed using CRLF line
> endings (the files in question are C# code files, and no
> .gitattributes was present at the time).
> 
> I also tried this:
> 
> $ git rm -r --cached . && git reset --hard
> 
> However, again `git status` shows no working copy modifications. The
> one thing that *did* work (and I tried this on accident actually) is:
> 
> $ git rm -r --cached . && git add .
> 
> This properly showed all files in my index with line ending
> modifications (I ran `git diff --cached -R` to be sure; the output
> shows `^M` at the end of each line in the diff in this case). Note
> that my global git config has `core.autocrlf` set to `false`, but I
> also tried the top 2 commands above with it set to `true` but it made
> no difference.
> 
> So my question is: Why do the top 2 commands not work, but the third
> one does? To me this all feels like magic / nondeterministic, so I'm
> hoping someone here knows what is going on and can explain the logic
> of it. Also if this is a git config issue, I'm not sure what it could
> be. Note my `.gitattributes` just has this in it:

The short version is, that the instructions on Github are outdated.
This is the official procedure (since 2016, Git v2.12 or so)
But it should work even with older version of Git.

$ echo "* text=auto" >.gitattributes
$ git read-tree --empty   # Clean index, force re-scan of working directory
$ git add .
$ git status# Show files that will be normalized
$ git commit -m "Introduce end-of-line normalization"


Could you open an issue on Github ?
(Or can someone @github fix this ?)

> 
> * text=auto
> 
> Thanks in advance.
> 
> 
> [1]: https://help.github.com/articles/dealing-with-line-endings/
> 

Re: [PATCH v3 5/5] sha1_name: Minimize OID comparisons during disambiguation

[Stefan Beller]

> @@ -505,6 +506,65 @@ static int extend_abbrev_len(const struct object_id 
> *oid, void *cb_data)
> return 0;
>  }
>
> +static void find_abbrev_len_for_pack(struct packed_git *p,
> +struct min_abbrev_data *mad)
> +{
> +   int match = 0;
> +   uint32_t num, last, first = 0;
> +   struct object_id oid;
> +
> +   open_pack_index(p);

coverity complained here with
Calling "open_pack_index" without checking return value
(as is done elsewhere 13 out of 15 times).

I think the easiest way out is just a

if (open_pack_index(p))
die(_("Cannot open existing pack idx file for '%s'"), p);

or is there another good approach?

Re: Security of .git/config and .git/hooks

[Stefan Beller]

So once upon a time we compared Gits security model with a
web browser. A web browser lets you execute 3rd party code
(e.g. javascript) and it is supposedly safe to look at malicious sites.

Currently Git only promises to have the clone/fetch operation safe,
not the "here is a zip of my whole repo" case, which sounds more
like the web browser experience ("here is a site with js, even zipped
in transfer"). Tightening the security model of Git towards this seems
like a good idea to me.

>>  1. Introduce a (configurable) list of "safe" configuration items that
>> can be set in .git/config and don't respect any others.
>
> A whitelist is obviously safer than a blacklist. Though I also feel like
> some of the options may give an unexpectedly wide attack surface. I.e.,
> I wouldn't be surprised if some innocent-looking option ends up being
> used in a tricky way to gain more access. E.g., submodule config
> pointing to paths outside of the repository.
>
> Do you plan to run in safe-mode all the time? What if safe-mode was a
> lot more extreme, and simply avoided reading repo-level config at all
> (except for check_repository_format(), which should be pretty innocent).
>
> I have a feeling there are some features (like submodules) that would
> simply be broken in safe-mode.

I would think that the essential submodule things would be "safe"
to look at. But e.g. submodule..update = "!rm -rf /" would be
not ok, hence the .update configuration would be in the unsafe space.

Any unsafe config option would need to be set outside the actual
repository (~/.config/git//config ?)

>
>>  2. But what if I want to set a different pager per-repository?
>> I think we could do this using configuration "profiles".
>> My ~/.config/git/profiles/ directory would contain git-style
>> config files for repositories to include.  Repositories could
>> then contain
>>
>>   [include]
>>   path = ~/.config/git/profiles/fancy-log-pager
>>
>> to make use of those settings.  The facility (1) would
>> special-case this directory to allow it to set "unsafe" settings
>> since files there are assumed not to be under the control of an
>> attacker.
>
> You can do something quite similar already:
>
>   git config --global \
> include.gitdir:/path/to/specific/repo.path
> .gitconfig-fancy-log-pager
>
> The main difference is that the profile inclusion is done by path rather
> than riding along with the repository directory if it gets moved. In
> practice I doubt that matters much, and I think the security model for
> include.gitdir is a lot simpler.

I am not sure if this works so well for the submodule..update
config (that we want to deprecate anyway, but still)

>> For backward compatibility, this facility would be controlled by a
>> global configuration setting.  If that setting is not enabled, then
>> the current, less safe behavior would remain.
>
> Are config and symlinks everything we need to care about? I can't think
> of anything else, but Git really has quite a large attack surface when
> accessing a local repo. Right now the safest thing you can do is "git
> clone --no-local" an untrusted repo and then look only at the clone. Of
> course nobody _actually_ does that, so any "safe" mode seems like it
> would be an improvement. But would claiming to have a "safe" mode
> encourage people to use it to look at risky repositories, exacerbating
> any holes (e.g., exploiting a bug in the index format)? I don't know.

Good point. Though we only care about the case of breaking out and
executing untrusted code; most of the index exploits would rather
trigger a segfault or infinite lop (in my imagination at least).

>
> -Peff

Line ending normalization doesn't work as expected

[Robert Dailey]

I'm on Windows using Git for Windows v2.13.1. Following github's
recommended process for fixing line endings after adding a
`.gitattributes` file[1], I run the following:

$ rm .git/index && git reset

Once I run `git status`, I see that no files have changed. Note that I
know for a fact in my repository, files were committed using CRLF line
endings (the files in question are C# code files, and no
.gitattributes was present at the time).

I also tried this:

$ git rm -r --cached . && git reset --hard

However, again `git status` shows no working copy modifications. The
one thing that *did* work (and I tried this on accident actually) is:

$ git rm -r --cached . && git add .

This properly showed all files in my index with line ending
modifications (I ran `git diff --cached -R` to be sure; the output
shows `^M` at the end of each line in the diff in this case). Note
that my global git config has `core.autocrlf` set to `false`, but I
also tried the top 2 commands above with it set to `true` but it made
no difference.

So my question is: Why do the top 2 commands not work, but the third
one does? To me this all feels like magic / nondeterministic, so I'm
hoping someone here knows what is going on and can explain the logic
of it. Also if this is a git config issue, I'm not sure what it could
be. Note my `.gitattributes` just has this in it:

* text=auto

Thanks in advance.


[1]: https://help.github.com/articles/dealing-with-line-endings/

Re: [PATCH 00/18] Partial clone (from clone to lazy fetch in 18 patches)

[Jeff Hostetler]

On 10/3/2017 4:50 AM, Junio C Hamano wrote:

Christian Couder  writes:


Could you give a bit more details about the use cases this is designed for?
It seems that when people review my work they want a lot of details
about the use cases, so I guess they would also be interesting in
getting this kind of information for your work too.

Could this support users who would be interested in lazily cloning
only one kind of files, for example *.jpeg?


I do not know about others, but the reason why I was not interested
in finding out "use cases" is because the value of this series is
use-case agnostic.

At least to me, the most interesting part of the series is that it
allows you to receive a set of objects transferred from the other
side that lack some of objects that would otherwise be required to
be here for connectivity purposes, and it introduces a mechanism to
allow object transfer layer, gc and fsck to work well together in
the resulting repository that deliberately lacks some objects.  The
transfer layer marks the objects obtained from a specific remote as
such, and gc and fsck are taught not to try to follow a missing link
or diagnose a missing link as an error, if a missing link is
expected using the mark the transfer layer left.

and it does so in such a way that it is use-case agnostic.  The
mechanism does not care how the objects to be omitted were chosen,
and how the omission criteria were negotiated between the sender and
the receiver of the pack.

I think the series comes with one filter that is size-based, but I
view it as a technology demonstration.  It does not have to be the
primary use case.  IOW, I do not think the series is meant as a
declaration that size-based filtering is the most important thing
and other omission criteria are less important.

You should be able to build path based omission (i.e. narrow clone)
or blobtype based omission.  Depending on your needs, you may want
different object omission criteria.  It is something you can build
on top.  And the work done on transfer/gc/fsck in this series does
not have to change to accommodate these different "use cases".



Agreed.

There are lots of reasons for wanting partial clones (and we've been
flinging lots of RFCs at each other that each seem to have a different
base assumption (small-blobs-only vs sparse-checkout vs ))
and not reaching consensus or closure.

The main thing is to allow the client to use partial clone to request
a "subset", let the server deliver that "subset", and let the client
tooling deal with an incomplete view of the repo.

As I see it there are the following major parts to partial clone:
1. How to let git-clone (and later git-fetch) specify the desired
   subset of objects that it wants?  (A ref-relative request.)
2. How to let the server and git-pack-objects build that incomplete
   packfile?
3. How to remember in the local config that a partial clone (or
   fetch) was used and that missing object should be expected?
4. How to dynamically fetch individual missing objects individually?
(Not a ref-relative request.)
5. How to augment the local ODB with partial clone information and
   let git-fsck (and friends) perform limited consistency checking?
6. Methods to bulk fetching missing objects (whether in a pre-verb
   hook or in unpack-tree)
7. Miscellaneous issues (e.g. fixing places that accidentally cause
   a missing object to be fetched that don't really need it).

My proposal [1] includes a generic filtering mechanism that handles 3
types of filtering and makes it easy to add other techniques as we
see fit.  It slips in at the list-objects / traverse_commit_list
level and hides all of the details from rev-list and pack-objects.
I have a follow on proposal [2] that extends the filtering parameter
handling to git-clone, git-fetch, git-fetch-pack, git-upload-pack
and the pack protocol.  That takes care of items 1 and 2 above.

Jonathan's proposal [3] includes code to update the local config,
dynamically fetch individual objects, and handle the local ODB and
fsck consistency checking.  That takes care of items 3, 4, and 5.

As was suggested above, I think we should merge our efforts:
using my filtering for 1 and 2 and Jonathan's code for 3, 4, and 5.
I would need to eliminate the "relax" options in favor of his
is_promised() functionality for index-pack and similar.  And omit
his blob-max-bytes changes from pack-objects, the protocol and
related commands.

That should be a good first step.

We both have thoughts on bulk fetching (mine in pre-verb hooks and
his in unpack-tree).  We don't need this immediately, but can wait
until the above is working to revisit.

[1] https://github.com/jeffhostetler/git/pull/3
[2]https://github.com/jeffhostetler/git/pull/4
[3] https://github.com/jonathantanmy/git/tree/partialclone3

Thoughts?

Thanks,
Jeff

[PATCH] test-stringlist: avoid buffer underrun when sorting nothing

[René Scharfe]

Check if the strbuf containing data to sort is empty before attempting
to trim a trailing newline character.

Signed-off-by: Rene Scharfe 
---
 t/helper/test-string-list.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index c502fa16d3..829ec3d7d2 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -108,7 +108,7 @@ int cmd_main(int argc, const char **argv)
 * Split by newline, but don't create a string_list item
 * for the empty string after the last separator.
 */
-   if (sb.buf[sb.len - 1] == '\n')
+   if (sb.len && sb.buf[sb.len - 1] == '\n')
strbuf_setlen(, sb.len - 1);
string_list_split_in_place(, sb.buf, '\n', -1);
 
-- 
2.14.2

Re: "man git-config", "--list" option misleadingly refers to "config file" (singular)

[Jeff King]

On Tue, Oct 03, 2017 at 06:34:34AM -0400, rpj...@crashcourse.ca wrote:

>   (i suppose that if i'm going to continue whining about stuff, i might
> as well clone the git source and start submitting patches.)

Yes, please. :)

>   in "man git-config":
> 
> -l
> --list
>   List all variables set in config file, along with their values.
>
> 
> except that, AIUI, "git config --list" will list the combination of all
> config values in (if it exists) /etc/gitconfig, then the user's global
> settings, and finally the repo's config settings if one happens to be
> in a working directory, so technically, it lists the contents of *all* of
> the config files (plural), no?

It does that by default, or it lists the contents of a specific file if
given (either by --file, or with --system, --global, or --local).

So I agree it's not quite accurate, but you probably want some phrasing
that leaves this unsaid (the actual rules are described earlier in the
description section). Maybe just refer to it as the "config source" or
something?

-Peff

[PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL

[René Scharfe]

Am 03.10.2017 um 14:51 schrieb René Scharfe:
> Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:
>> Furthermore, fsck.c:fsck_walk_tree() does the same "immediately
>> reference the object member in lookup_blob()'s and lookup_tree()'s
>> return value" thing.  I think those should receive the same treatment
>> as well.
> 
> Hmm, are put_object_name() and all the walk() implementations ready for
> a NULL object handed to them?  Or would we rather need to error out
> right there?
How about this?

-- >8 --
lookup_blob() and lookup_tree() can return NULL if they find an object
of an unexpected type.  Error out of fsck_walk_tree() in that case, like
we do when encountering a bad file mode.  An error message is already
shown by object_as_type(), which gets called by the lookup functions.

Signed-off-by: Rene Scharfe 
---
 fsck.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fsck.c b/fsck.c
index 2ad00fc4d0..561a13ac27 100644
--- a/fsck.c
+++ b/fsck.c
@@ -358,14 +358,20 @@ static int fsck_walk_tree(struct tree *tree, void *data, 
struct fsck_options *op
continue;
 
if (S_ISDIR(entry.mode)) {
-   obj = _tree(entry.oid)->object;
+   struct tree *tree = lookup_tree(entry.oid);
+   if (!tree)
+   return -1;
+   obj = >object;
if (name)
put_object_name(options, obj, "%s%s/", name,
entry.path);
result = options->walk(obj, OBJ_TREE, data, options);
}
else if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {
-   obj = _blob(entry.oid)->object;
+   struct blob *blob = lookup_blob(entry.oid);
+   if (!blob)
+   return -1;
+   obj = >object;
if (name)
put_object_name(options, obj, "%s%s", name,
entry.path);
-- 
2.14.2

[PATCH 12/12] color: make "always" the same as "auto" in config

[Jeff King]

It can be handy to use `--color=always` (or it's synonym
`--color`) on the command-line to convince a command to
produce color even if it's stdout isn't going to the
terminal or a pager.

What's less clear is whether it makes sense to set config
variables like color.ui to `always`. For a one-shot like:

  git -c color.ui=always ...

it's potentially useful (especially if the command doesn't
directly support the `--color` option). But setting `always`
in your on-disk config is much muddier, as you may be
surprised when piped commands generate colors (and send them
to whatever is consuming the pipe downstream).

Some people have done this anyway, because:

  1. The documentation for color.ui makes it sound like
 using `always` is a good idea, when you almost
 certainly want `auto`.

  2. Traditionally not every command (and especially not
 plumbing) respected color.ui in the first place. So
 the confusion came up less frequently than it might
 have.

The situation changed in 136c8c8b8f (color: check color.ui
in git_default_config(), 2017-07-13), which negated point
(2): now scripts using only plumbing commands (like
add-interactive) are broken by this setting.

That commit was fixing real issues (e.g., by making
`color.ui=never` work, since `auto` is the default), so we
don't want to just revert it.  We could turn `always` into a
noop in plumbing commands, but that creates a hard-to-explain
inconsistency between the plumbing and other commands.

Instead, let's just turn `always` into `auto` for all config.
This does break the "one-shot" config shown above, but again,
we're probably better to have simple and consistent rules than
to try to special-case command-line config.

There is one place where `always` should retain its meaning:
on the command line, `--color=always` should continue to be
the same as `--color`, overriding any isatty checks. Since the
command-line parser also depends on git_config_colorbool(), we
can use the existence of the "var" string to deterine whether
we are serving the command-line or the config.

Signed-off-by: Jeff King 
---
 Documentation/config.txt   | 35 +--
 color.c|  2 +-
 t/t3701-add-interactive.sh | 10 ++
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6adb..b53c994d0a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1058,10 +1058,10 @@ clean.requireForce::
 
 color.branch::
A boolean to enable/disable color in the output of
-   linkgit:git-branch[1]. May be set to `always`,
-   `false` (or `never`) or `auto` (or `true`), in which case colors are 
used
-   only when the output is to a terminal. If unset, then the
-   value of `color.ui` is used (`auto` by default).
+   linkgit:git-branch[1]. May be set to `false` (or `never`) to
+   disable color entirely, `auto` (or `true` or `always`) in which
+   case colors are used only when the output is to a terminal.  If
+   unset, then the value of `color.ui` is used (`auto` by default).
 
 color.branch.::
Use customized color for branch coloration. `` is one of
@@ -1072,12 +1072,11 @@ color.branch.::
 
 color.diff::
Whether to use ANSI escape sequences to add color to patches.
-   If this is set to `always`, linkgit:git-diff[1],
+   If this is set to `true` or `auto`, linkgit:git-diff[1],
linkgit:git-log[1], and linkgit:git-show[1] will use color
-   for all patches.  If it is set to `true` or `auto`, those
-   commands will only use color when output is to the terminal.
-   If unset, then the value of `color.ui` is used (`auto` by
-   default).
+   when output is to the terminal. The value `always` is a
+   historical synonym for `auto`.  If unset, then the value of
+   `color.ui` is used (`auto` by default).
 +
 This does not affect linkgit:git-format-patch[1] or the
 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the
@@ -1141,12 +1140,12 @@ color.grep.::
 --
 
 color.interactive::
-   When set to `always`, always use colors for interactive prompts
+   When set to `true` or `auto`, use colors for interactive prompts
and displays (such as those used by "git-add --interactive" and
-   "git-clean --interactive"). When false (or `never`), never.
-   When set to `true` or `auto`, use colors only when the output is
-   to the terminal. If unset, then the value of `color.ui` is
-   used (`auto` by default).
+   "git-clean --interactive") when the output is to the terminal.
+   When false (or `never`), never show colors. The value `always`
+   is a historical synonym for `auto`.  If unset, then the value of
+   `color.ui` is used (`auto` by default).
 
 color.interactive.::
Use customized color for 'git add --interactive' and 'git clean
@@ -1193,10 +1192,10 @@ 

[PATCH 11/12] provide --color option for all ref-filter users

[Jeff King]

When ref-filter learned about want_color() in 11b087adfd
(ref-filter: consult want_color() before emitting colors,
2017-07-13), it became useful to be able to turn colors off
and on for specific commands. For git-branch, you can do so
with --color/--no-color.

But for git-for-each-ref and git-tag, the other users of
ref-filter, you have no option except to tweak the
"color.ui" config setting. Let's give both of these commands
the usual color command-line options.

This is a bit more obvious as a method for overriding the
config. And it also prepares us for the behavior of "always"
changing (so that we are still left with a way of forcing
color when our output goes to a non-terminal).

Signed-off-by: Jeff King 
---
 Documentation/git-for-each-ref.txt | 5 +
 Documentation/git-tag.txt  | 5 +
 builtin/for-each-ref.c | 1 +
 builtin/tag.c  | 1 +
 t/t6300-for-each-ref.sh| 4 ++--
 t/t7004-tag.sh | 4 ++--
 6 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 66b4e0a405..cbd0a6212a 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -57,6 +57,11 @@ OPTIONS
`xx`; for example `%00` interpolates to `\0` (NUL),
`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
 
+--color[=]:
+   Respect any colors specified in the `--format` option. The
+   `` field must be one of `always`, `never`, or `auto` (if
+   `` is absent, behave as if `always` was given).
+
 --shell::
 --perl::
 --python::
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 95e9f391d8..956fc019f9 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -115,6 +115,11 @@ options for details.
variable if it exists, or lexicographic order otherwise. See
linkgit:git-config[1].
 
+--color[=]:
+   Respect any colors specified in the `--format` option. The
+   `` field must be one of `always`, `never`, or `auto` (if
+   `` is absent, behave as if `always` was given).
+
 -i::
 --ignore-case::
Sorting and filtering tags are case insensitive.
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 5d7c921a77..e931be9ce4 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -36,6 +36,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char 
*prefix)
OPT_GROUP(""),
OPT_INTEGER( 0 , "count", , N_("show only  matched 
refs")),
OPT_STRING(  0 , "format", , N_("format"), 
N_("format to use for the output")),
+   OPT__COLOR(_color, N_("respect format colors")),
OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
N_("field name to sort on"), 
_opt_ref_sorting),
OPT_CALLBACK(0, "points-at", _at,
diff --git a/builtin/tag.c b/builtin/tag.c
index c627794181..12dbbc56d9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -411,6 +411,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
},
OPT_STRING(  0 , "format", , N_("format"),
   N_("format to use for the output")),
+   OPT__COLOR(_color, N_("respect format colors")),
OPT_BOOL('i', "ignore-case", , N_("sorting and filtering 
are case insensitive")),
OPT_END()
};
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index d6bffe6273..6358134805 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -435,8 +435,8 @@ test_expect_success '%(color) does not show color without 
tty' '
test_cmp expected.bare actual
 '
 
-test_expect_success 'color.ui=always can override tty check' '
-   git -c color.ui=always for-each-ref --format="$color_format" 
>actual.raw &&
+test_expect_success '--color can override tty check' '
+   git for-each-ref --color --format="$color_format" >actual.raw &&
test_decode_color actual &&
test_cmp expected.color actual
 '
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 0a86f8ea39..4e62c505fc 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1912,8 +1912,8 @@ test_expect_success TTY '%(color) present with tty' '
test_cmp expect.color actual
 '
 
-test_expect_success 'color.ui=always overrides auto-color' '
-   git -c color.ui=always tag $color_args >actual.raw &&
+test_expect_success '--color overrides auto-color' '
+   git tag --color $color_args >actual.raw &&
test_decode_color actual &&
test_cmp expect.color actual
 '
-- 
2.14.2.1079.gce6b466188

[PATCH 10/12] t3205: use --color instead of color.branch=always

[Jeff King]

To test the color output, we must convince "git branch" to
write colors to a non-terminal. We do that now by setting
the color config to "always".  In preparation for the
behavior of "always" changing, let's switch to using the
"--color" command-line option, which is more direct.

Signed-off-by: Jeff King 
---
 t/t3205-branch-color.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t3205-branch-color.sh b/t/t3205-branch-color.sh
index 9343550f50..4f1e16bb44 100755
--- a/t/t3205-branch-color.sh
+++ b/t/t3205-branch-color.sh
@@ -12,7 +12,6 @@ test_expect_success 'set up some sample branches' '
 # choose non-default colors to make sure config
 # is taking effect
 test_expect_success 'set up some color config' '
-   git config color.branch always &&
git config color.branch.local blue &&
git config color.branch.remote yellow &&
git config color.branch.current cyan
@@ -24,7 +23,7 @@ test_expect_success 'regular output shows colors' '
  other
  remotes/origin/master
EOF
-   git branch -a >actual.raw &&
+   git branch --color -a >actual.raw &&
test_decode_color actual &&
test_cmp expect actual
 '
@@ -36,7 +35,7 @@ test_expect_success 'verbose output shows colors' '
  other $oid foo
  remotes/origin/master $oid foo
EOF
-   git branch -v -a >actual.raw &&
+   git branch --color -v -a >actual.raw &&
test_decode_color actual &&
test_cmp expect actual
 '
-- 
2.14.2.1079.gce6b466188

[PATCH 09/12] t7301: use test_terminal to check color

[Jeff King]

This test wants to confirm that "clean -i" shows color
output. Using test_terminal gives us a more realistic
environment than "color.ui=always", and prepares us for the
behavior of "always" changing in a future patch.

Signed-off-by: Jeff King 
---
 t/t7301-clean-interactive.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7301-clean-interactive.sh b/t/t7301-clean-interactive.sh
index 556e1850e2..1bf9789c8a 100755
--- a/t/t7301-clean-interactive.sh
+++ b/t/t7301-clean-interactive.sh
@@ -3,6 +3,7 @@
 test_description='git clean -i basic tests'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'setup' '
 
@@ -472,10 +473,10 @@ test_expect_success 'git clean -id with prefix and path 
(ask)' '
 
 '
 
-test_expect_success 'git clean -i paints the header in HEADER color' '
+test_expect_success TTY 'git clean -i paints the header in HEADER color' '
>a.out &&
echo q |
-   git -c color.ui=always clean -i |
+   test_terminal git clean -i |
test_decode_color |
head -n 1 >header &&
# not i18ngrep
-- 
2.14.2.1079.gce6b466188

[PATCH 07/12] t6006: drop "always" color config tests

[Jeff King]

We test the %C() format placeholders with a variety of
color-inducing options, including "--color" and
"-c color.ui=always". In preparation for the behavior of
"always" changing, we need to do something with those
"always" tests.

We can drop ones that expect "always" to turn on color even
to a file, as that will become a synonym for "auto", which
is already tested.

For the "--no-color" test, we need to make sure that color
would otherwise be shown. To do this, we can use
test_terminal, which enables colors in the default setup.

Signed-off-by: Jeff King 
---
 t/t6006-rev-list-format.sh | 20 +---
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 98be78b4a2..25a9c65dc5 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -208,26 +208,11 @@ do
has_no_color actual
'
 
-   test_expect_success "$desc enables colors for color.diff" '
-   git -c color.diff=always log --format=$color -1 >actual &&
-   has_color actual
-   '
-
-   test_expect_success "$desc enables colors for color.ui" '
-   git -c color.ui=always log --format=$color -1 >actual &&
-   has_color actual
-   '
-
test_expect_success "$desc respects --color" '
git log --format=$color -1 --color >actual &&
has_color actual
'
 
-   test_expect_success "$desc respects --no-color" '
-   git -c color.ui=always log --format=$color -1 --no-color 
>actual &&
-   has_no_color actual
-   '
-
test_expect_success TTY "$desc respects --color=auto (stdout is tty)" '
test_terminal git log --format=$color -1 --color=auto >actual &&
has_color actual
@@ -240,6 +225,11 @@ do
has_no_color actual
)
'
+
+   test_expect_success TTY "$desc respects --no-color" '
+   test_terminal git log --format=$color -1 --no-color >actual &&
+   has_no_color actual
+   '
 done
 
 test_expect_success '%C(always,...) enables color even without tty' '
-- 
2.14.2.1079.gce6b466188

[PATCH 08/12] t3203: drop "always" color test

[Jeff King]

In preparation for the behavior of "always" changing to
match "auto", we can simply drop this test. We already check
other forms (like "--color") independently.

Signed-off-by: Jeff King 
---
 t/t3203-branch-output.sh | 6 --
 1 file changed, 6 deletions(-)

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 86286f263d..ee6787614c 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -258,12 +258,6 @@ test_expect_success TTY '%(color) present with tty' '
test_cmp expect.color actual
 '
 
-test_expect_success 'color.branch=always overrides auto-color' '
-   git -c color.branch=always branch $color_args >actual.raw &&
-   test_decode_color actual &&
-   test_cmp expect.color actual
-'
-
 test_expect_success '--color overrides auto-color' '
git branch --color $color_args >actual.raw &&
test_decode_color actual &&
-- 
2.14.2.1079.gce6b466188

[PATCH 04/12] t3701: use test-terminal to collect color output

[Jeff King]

When testing whether "add -p" can generate colors, we set
color.ui to "always". This isn't a very good test, as in the
real-world a user typically has "auto" coupled with stdout
going to a terminal (and it's plausible that this could mask
a real bug in add--interactive if we depend on plumbing's
isatty check).

Let's switch to test_terminal, which gives us a more
realistic environment. This also prepare us for future
changes to the "always" color option.

Signed-off-by: Jeff King 
---
 t/t3701-add-interactive.sh | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 2f3e7cea64..39d0130f88 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -2,6 +2,7 @@
 
 test_description='add -i basic tests'
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 if ! test_have_prereq PERL
 then
@@ -380,14 +381,11 @@ test_expect_success 'patch mode ignores unmerged entries' 
'
test_cmp expected diff
 '
 
-test_expect_success 'diffs can be colorized' '
+test_expect_success TTY 'diffs can be colorized' '
git reset --hard &&
 
-   # force color even though the test script has no terminal
-   test_config color.ui always &&
-
echo content >test &&
-   printf y | git add -p >output 2>&1 &&
+   printf y | test_terminal git add -p >output 2>&1 &&
 
# We do not want to depend on the exact coloring scheme
# git uses for diffs, so just check that we saw some kind of color.
-- 
2.14.2.1079.gce6b466188

[PATCH 05/12] t7508: use test_terminal for color output

[Jeff King]

This script tests the output of status with various formats
when color is enabled. It uses the "always" setting so that
the output is valid even though we capture it in a file.
Using test_terminal gives us a more realistic environment,
and prepares us for the behavior of "always" changing.

Arguably we are testing less than before, since "auto" is
already the default, and we can no longer tell if the config
is actually doing anything.

Signed-off-by: Jeff King 
---
I noticed that "status" does not have a "--color" option. I think it
might be worth adding one for completeness, though I still prefer the
test_terminal solution here.

 t/t7508-status.sh | 41 +
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 43d19a9b22..a3d760e63a 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -6,6 +6,7 @@
 test_description='git status'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'status -h in broken repository' '
git config --global advice.statusuoption false &&
@@ -667,7 +668,7 @@ test_expect_success 'setup unique colors' '
 
 '
 
-test_expect_success 'status with color.ui' '
+test_expect_success TTY 'status with color.ui' '
cat >expect <<\EOF &&
 On branch master
 Your branch and '\''upstream'\'' have diverged,
@@ -694,14 +695,14 @@ Untracked files:
untracked
 
 EOF
-   test_config color.ui always &&
-   git status | test_decode_color >output &&
+   test_config color.ui auto &&
+   test_terminal git status | test_decode_color >output &&
test_i18ncmp expect output
 '
 
-test_expect_success 'status with color.status' '
-   test_config color.status always &&
-   git status | test_decode_color >output &&
+test_expect_success TTY 'status with color.status' '
+   test_config color.status auto &&
+   test_terminal git status | test_decode_color >output &&
test_i18ncmp expect output
 '
 
@@ -714,19 +715,19 @@ cat >expect <<\EOF
 ?? untracked
 EOF
 
-test_expect_success 'status -s with color.ui' '
+test_expect_success TTY 'status -s with color.ui' '
 
-   git config color.ui always &&
-   git status -s | test_decode_color >output &&
+   git config color.ui auto &&
+   test_terminal git status -s | test_decode_color >output &&
test_cmp expect output
 
 '
 
-test_expect_success 'status -s with color.status' '
+test_expect_success TTY 'status -s with color.status' '
 
git config --unset color.ui &&
-   git config color.status always &&
-   git status -s | test_decode_color >output &&
+   git config color.status auto &&
+   test_terminal git status -s | test_decode_color >output &&
test_cmp expect output
 
 '
@@ -741,9 +742,9 @@ cat >expect <<\EOF
 ?? untracked
 EOF
 
-test_expect_success 'status -s -b with color.status' '
+test_expect_success TTY 'status -s -b with color.status' '
 
-   git status -s -b | test_decode_color >output &&
+   test_terminal git status -s -b | test_decode_color >output &&
test_i18ncmp expect output
 
 '
@@ -757,20 +758,20 @@ A  dir2/added
 ?? untracked
 EOF
 
-test_expect_success 'status --porcelain ignores color.ui' '
+test_expect_success TTY 'status --porcelain ignores color.ui' '
 
git config --unset color.status &&
-   git config color.ui always &&
-   git status --porcelain | test_decode_color >output &&
+   git config color.ui auto &&
+   test_terminal git status --porcelain | test_decode_color >output &&
test_cmp expect output
 
 '
 
-test_expect_success 'status --porcelain ignores color.status' '
+test_expect_success TTY 'status --porcelain ignores color.status' '
 
git config --unset color.ui &&
-   git config color.status always &&
-   git status --porcelain | test_decode_color >output &&
+   git config color.status auto &&
+   test_terminal git status --porcelain | test_decode_color >output &&
test_cmp expect output
 
 '
-- 
2.14.2.1079.gce6b466188

[PATCH 06/12] t7502: use diff.noprefix for --verbose test

[Jeff King]

To check that "status -v" respects diff config, we set
"color.diff" and look at the output of "status". We could
equally well use any diff config. Since color output depends
on a lot of other factors (like whether stdout is a tty, and
how we interpret "always"), let's use a more mundane option.

Signed-off-by: Jeff King 
---
 t/t7502-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 725687d5d5..d33a3cb331 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -171,9 +171,9 @@ test_expect_success 'verbose' '
 
 test_expect_success 'verbose respects diff config' '
 
-   test_config color.diff always &&
+   test_config diff.noprefix true &&
git status -v >actual &&
-   grep "\[1mdiff --git" actual
+   grep "diff --git negative negative" actual
 '
 
 mesg_with_comment_and_newlines='
-- 
2.14.2.1079.gce6b466188

[PATCH 01/12] test-terminal: set TERM=vt100

[Jeff King]

The point of the test-terminal script is to simulate in the
test scripts an environment where output is going to a real
terminal.

But since test-lib.sh also sets TERM=dumb, the simulation
isn't very realistic. The color code will skip auto-coloring
for TERM=dumb, leading to us liberally sprinkling

  test_terminal env TERM=vt100 git ...

through the test suite to convince the tests to actually
generate colors. Let's set TERM for programs run under
test_terminal, which is one less thing for test-writers to
remember.

In most cases the callers can be simplified, but note there
is one interesting case in t4202. It uses test_terminal to
check the auto-enabling of --decorate, but the expected
output _doesn't_ contain colors (because TERM=dumb
suppresses them). Using TERM=vt100 is closer to what the
real world looks like; adjust the expected output to match.

Signed-off-by: Jeff King 
---
Not strictly necessary for this series, but after banging my shins on
this nuisance for the umpteenth time, I decided to finally do something
about it.

 t/t3203-branch-output.sh   | 2 +-
 t/t4202-log.sh | 2 +-
 t/t6006-rev-list-format.sh | 3 +--
 t/t6300-for-each-ref.sh| 3 +--
 t/t7004-tag.sh | 2 +-
 t/t7006-pager.sh   | 6 +++---
 t/test-terminal.perl   | 1 +
 7 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index d2aec0f38b..86286f263d 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -253,7 +253,7 @@ test_expect_success '%(color) omitted without tty' '
 '
 
 test_expect_success TTY '%(color) present with tty' '
-   test_terminal env TERM=vt100 git branch $color_args >actual.raw &&
+   test_terminal git branch $color_args >actual.raw &&
test_decode_color actual &&
test_cmp expect.color actual
 '
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 36d120c969..8f155da7a5 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -750,7 +750,7 @@ test_expect_success 'log.decorate config parsing' '
 '
 
 test_expect_success TTY 'log output on a TTY' '
-   git log --oneline --decorate >expect.short &&
+   git log --color --oneline --decorate >expect.short &&
 
test_terminal git log --oneline >actual &&
test_cmp expect.short actual
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index b326d550f3..98be78b4a2 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -229,8 +229,7 @@ do
'
 
test_expect_success TTY "$desc respects --color=auto (stdout is tty)" '
-   test_terminal env TERM=vt100 \
-   git log --format=$color -1 --color=auto >actual &&
+   test_terminal git log --format=$color -1 --color=auto >actual &&
has_color actual
'
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b733..d6bffe6273 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -425,8 +425,7 @@ test_expect_success 'set up color tests' '
 '
 
 test_expect_success TTY '%(color) shows color with a tty' '
-   test_terminal env TERM=vt100 \
-   git for-each-ref --format="$color_format" >actual.raw &&
+   test_terminal git for-each-ref --format="$color_format" >actual.raw &&
test_decode_color actual &&
test_cmp expected.color actual
 '
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b545c33f83..0a86f8ea39 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1907,7 +1907,7 @@ test_expect_success '%(color) omitted without tty' '
 '
 
 test_expect_success TTY '%(color) present with tty' '
-   test_terminal env TERM=vt100 git tag $color_args >actual.raw &&
+   test_terminal git tag $color_args >actual.raw &&
test_decode_color actual &&
test_cmp expect.color actual
 '
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 9128ec5acd..f0f1abd1c2 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -239,7 +239,7 @@ test_expect_success 'no color when stdout is a regular 
file' '
 test_expect_success TTY 'color when writing to a pager' '
rm -f paginated.out &&
test_config color.ui auto &&
-   test_terminal env TERM=vt100 git log &&
+   test_terminal git log &&
colorful paginated.out
 '
 
@@ -247,7 +247,7 @@ test_expect_success TTY 'colors are suppressed by 
color.pager' '
rm -f paginated.out &&
test_config color.ui auto &&
test_config color.pager false &&
-   test_terminal env TERM=vt100 git log &&
+   test_terminal git log &&
! colorful paginated.out
 '
 
@@ -266,7 +266,7 @@ test_expect_success 'color when writing to a file intended 
for a pager' '
 test_expect_success TTY 'colors are sent to pager for external commands' '
test_config alias.externallog "!git log" &&
test_config color.ui auto &&
-   test_terminal env TERM=vt100 git -p externallog &&
+   test_terminal 

Re: Updated to v2.14.2 on macOS; git add --patch broken

[Jeff King]

On Tue, Oct 03, 2017 at 07:38:24PM +0900, Junio C Hamano wrote:

> That's an argument to say color.ui=always is not a useful thing to
> use and Git is wrong to offer such a nonsense option.  I agree with
> both of them.
> 
> But it is a different matter that plumbing commands are now doing
> the "color" thing without being asked.  Reading the configuration
> that is meant for human interaction adds insult to injury.  I think
> the earlier "everybody is colored by default" that forgot to make
> sure the change does not affect plumbing was the main culprit, and
> we were merely lucky that thanks to the auto-detection of "auto" we
> did not break scripts.

Yes, I agree that the "everybody is colored by default" is the root of
the problem. And in that sense all of this discussion is band-aid fixes
on that.

At the same time, I have a suspicion that "even plumbing is color=auto
by default" may actually be _helping_ in many scripts. Because they to
use the plumbing to show output to the user, respecting the user's
normal color preferences. That's just a hunch, though.

I also think trying to revert that "default" patch (4c7f1819) may be a
pretty big pain at this point.

  Side note: Sorry, I noticed while writing this that I got my commit
  references mixed up earlier between 4c7f1819 (which turned the default
  to auto even for plumbing) and 136c8c8b, my recent change to parse
  color.ui in more places. When I said the problem was exacerbated/made
  worse by 4c7f1819, I really meant 136c8c8b. Hopefully that didn't
  confuse the discussion too much.

> Having said all that, unless we revert the entire series (and
> possibly other things that happened after the series was merged on
> 'master' that assumes that now default_config would read the
> color.ui thing), we won't be able to get back to the state before
> the "add -p" regression, it seems.  As -rc freeze period for the
> next cycle is approaching fast, I wanted to make sure that we have a
> plan to address the regression (which does not have to solve what
> the reverted commit tried to solve).  If you think we can get a
> workable code in 2.14.x and 'master' that essentially castrates
> "always" and makes it the same as "auto" in several days tops, then
> I'd prefer such a solution, as honoring "color.ui=always" does not
> feel all that important.

Here's what I came up with on the "color.ui=always is nonsense that we
should not offer" front. The number of patches may be a little daunting,
but most of them are just removing cases of "git -c color.ui=always"
from the tests.

  [01/12]: test-terminal: set TERM=vt100
  [02/12]: t4015: prefer --color to -c color.diff=always
  [03/12]: t4015: use --color with --color-moved
  [04/12]: t3701: use test-terminal to collect color output
  [05/12]: t7508: use test_terminal for color output
  [06/12]: t7502: use diff.noprefix for --verbose test
  [07/12]: t6006: drop "always" color config tests
  [08/12]: t3203: drop "always" color test
  [09/12]: t7301: use test_terminal to check color
  [10/12]: t3205: use --color instead of color.branch=always
  [11/12]: provide --color option for all ref-filter users
  [12/12]: color: make "always" the same as "auto" in config

 Documentation/config.txt   | 35 -
 Documentation/git-for-each-ref.txt |  5 
 Documentation/git-tag.txt  |  5 
 builtin/for-each-ref.c |  1 +
 builtin/tag.c  |  1 +
 color.c|  2 +-
 t/t3203-branch-output.sh   |  8 +-
 t/t3205-branch-color.sh|  5 ++--
 t/t3701-add-interactive.sh | 18 +
 t/t4015-diff-whitespace.sh | 53 +++---
 t/t4202-log.sh |  2 +-
 t/t6006-rev-list-format.sh | 23 +
 t/t6300-for-each-ref.sh|  7 +++--
 t/t7004-tag.sh |  6 ++---
 t/t7006-pager.sh   |  6 ++---
 t/t7301-clean-interactive.sh   |  5 ++--
 t/t7502-commit.sh  |  4 +--
 t/t7508-status.sh  | 41 +++--
 t/test-terminal.perl   |  1 +
 19 files changed, 115 insertions(+), 113 deletions(-)

-Peff

[PATCH 03/12] t4015: use --color with --color-moved

[Jeff King]

The tests for --color-moved write their output to a file,
but doing so suppresses color output under "auto". Right now
this is solved by running the whole script under
"color.diff=always". In preparation for the behavior of
"always" changing, let's explicitly enable color.

Signed-off-by: Jeff King 
---
I kind of think `--color-moved` should imply `--color`, as
`--color-words` seems to. But that seemed like a potential rabbit-hole,
and this series is already contentious enough.

 t/t4015-diff-whitespace.sh | 25 -
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index b9df886e37..3bca958863 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -802,7 +802,6 @@ test_expect_success 'combined diff with autocrlf 
conversion' '
 # Start testing the colored format for whitespace checks
 
 test_expect_success 'setup diff colors' '
-   git config color.diff always &&
git config color.diff.plain normal &&
git config color.diff.meta bold &&
git config color.diff.frag cyan &&
@@ -986,7 +985,7 @@ test_expect_success 'detect moved code, complete file' '
git mv test.c main.c &&
test_config color.diff.oldMoved "normal red" &&
test_config color.diff.newMoved "normal green" &&
-   git diff HEAD --color-moved=zebra --no-renames | test_decode_color 
>actual &&
+   git diff HEAD --color-moved=zebra --color --no-renames | 
test_decode_color >actual &&
cat >expected <<-\EOF &&
diff --git a/main.c b/main.c
new file mode 100644
@@ -1087,7 +1086,7 @@ test_expect_success 'detect malicious moved code, inside 
file' '
bar();
}
EOF
-   git diff HEAD --no-renames --color-moved=zebra| test_decode_color 
>actual &&
+   git diff HEAD --no-renames --color-moved=zebra --color | 
test_decode_color >actual &&
cat <<-\EOF >expected &&
diff --git a/main.c b/main.c
index 27a619c..7cf9336 100644
@@ -1136,7 +1135,7 @@ test_expect_success 'plain moved code, inside file' '
test_config color.diff.oldMovedAlternative "blue" &&
test_config color.diff.newMovedAlternative "yellow" &&
# needs previous test as setup
-   git diff HEAD --no-renames --color-moved=plain| test_decode_color 
>actual &&
+   git diff HEAD --no-renames --color-moved=plain --color | 
test_decode_color >actual &&
cat <<-\EOF >expected &&
diff --git a/main.c b/main.c
index 27a619c..7cf9336 100644
@@ -1227,7 +1226,7 @@ test_expect_success 'detect permutations inside moved 
code -- dimmed_zebra' '
test_config color.diff.newMovedDimmed "normal cyan" &&
test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
-   git diff HEAD --no-renames --color-moved=dimmed_zebra |
+   git diff HEAD --no-renames --color-moved=dimmed_zebra --color |
grep -v "index" |
test_decode_color >actual &&
cat <<-\EOF >expected &&
@@ -1271,7 +1270,7 @@ test_expect_success 'cmd option assumes configured 
colored-moved' '
test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
test_config diff.colorMoved zebra &&
-   git diff HEAD --no-renames --color-moved |
+   git diff HEAD --no-renames --color-moved --color |
grep -v "index" |
test_decode_color >actual &&
cat <<-\EOF >expected &&
@@ -1343,7 +1342,7 @@ line 4
 EOF
test_config color.diff.oldMoved "magenta" &&
test_config color.diff.newMoved "cyan" &&
-   git diff HEAD --no-renames --color-moved |
+   git diff HEAD --no-renames --color-moved --color |
grep -v "index" |
test_decode_color >actual &&
cat <<-\EOF >expected &&
@@ -1364,7 +1363,7 @@ EOF
EOF
test_cmp expected actual &&
 
-   git diff HEAD --no-renames -w --color-moved |
+   git diff HEAD --no-renames -w --color-moved --color |
grep -v "index" |
test_decode_color >actual &&
cat <<-\EOF >expected &&
@@ -1403,7 +1402,7 @@ test_expect_success '--color-moved block at end of diff 
output respects MIN_ALNU
irrelevant_line
EOF
 
-   git diff HEAD --color-moved=zebra --no-renames |
+   git diff HEAD --color-moved=zebra --color --no-renames |
grep -v "index" |
test_decode_color >actual &&
cat >expected <<-\EOF &&
@@ -1442,7 +1441,7 @@ test_expect_success '--color-moved respects 
MIN_ALNUM_COUNT' '
nineteen chars 456789
EOF
 
-   git diff HEAD --color-moved=zebra --no-renames |
+   git diff HEAD --color-moved=zebra --color --no-renames |
grep -v 

[PATCH 02/12] t4015: prefer --color to -c color.diff=always

[Jeff King]

t4015 contains many color-related tests which need to
override the "is stdout a tty" check. They do so by setting
the color.diff config, but we can accomplish the same with
the --color option. Besides being shorter to type, switching
will prepare us for upcoming changes to "always" when see it
in config.

Signed-off-by: Jeff King 
---
 t/t4015-diff-whitespace.sh | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 12d182dc1b..b9df886e37 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -821,7 +821,7 @@ test_expect_success 'diff that introduces a line with only 
tabs' '
echo "test" >x &&
git commit -m "initial" x &&
echo "{NTN}" | tr "NT" "\n\t" >>x &&
-   git -c color.diff=always diff | test_decode_color >current &&
+   git diff --color | test_decode_color >current &&
 
cat >expected <<-\EOF &&
diff --git a/x b/x
@@ -851,7 +851,7 @@ test_expect_success 'diff that introduces and removes ws 
breakages' '
echo "2. and a new line "
} >x &&
 
-   git -c color.diff=always diff |
+   git diff --color |
test_decode_color >current &&
 
cat >expected <<-\EOF &&
@@ -923,15 +923,15 @@ test_expect_success 'ws-error-highlight test setup' '
 
 test_expect_success 'test --ws-error-highlight option' '
 
-   git -c color.diff=always diff --ws-error-highlight=default,old |
+   git diff --color --ws-error-highlight=default,old |
test_decode_color >current &&
test_cmp expect.default-old current &&
 
-   git -c color.diff=always diff --ws-error-highlight=all |
+   git diff --color --ws-error-highlight=all |
test_decode_color >current &&
test_cmp expect.all current &&
 
-   git -c color.diff=always diff --ws-error-highlight=none |
+   git diff --color --ws-error-highlight=none |
test_decode_color >current &&
test_cmp expect.none current
 
@@ -939,15 +939,15 @@ test_expect_success 'test --ws-error-highlight option' '
 
 test_expect_success 'test diff.wsErrorHighlight config' '
 
-   git -c color.diff=always -c diff.wsErrorHighlight=default,old diff |
+   git -c diff.wsErrorHighlight=default,old diff --color |
test_decode_color >current &&
test_cmp expect.default-old current &&
 
-   git -c color.diff=always -c diff.wsErrorHighlight=all diff |
+   git -c diff.wsErrorHighlight=all diff --color |
test_decode_color >current &&
test_cmp expect.all current &&
 
-   git -c color.diff=always -c diff.wsErrorHighlight=none diff |
+   git -c diff.wsErrorHighlight=none diff --color |
test_decode_color >current &&
test_cmp expect.none current
 
@@ -955,18 +955,18 @@ test_expect_success 'test diff.wsErrorHighlight config' '
 
 test_expect_success 'option overrides diff.wsErrorHighlight' '
 
-   git -c color.diff=always -c diff.wsErrorHighlight=none \
-   diff --ws-error-highlight=default,old |
+   git -c diff.wsErrorHighlight=none \
+   diff --color --ws-error-highlight=default,old |
test_decode_color >current &&
test_cmp expect.default-old current &&
 
-   git -c color.diff=always -c diff.wsErrorHighlight=default \
-   diff --ws-error-highlight=all |
+   git -c diff.wsErrorHighlight=default \
+   diff --color --ws-error-highlight=all |
test_decode_color >current &&
test_cmp expect.all current &&
 
-   git -c color.diff=always -c diff.wsErrorHighlight=all \
-   diff --ws-error-highlight=none |
+   git -c diff.wsErrorHighlight=all \
+   diff --color --ws-error-highlight=none |
test_decode_color >current &&
test_cmp expect.none current
 
-- 
2.14.2.1079.gce6b466188

I am waiting for your Reply

[Mr.Patrick Joseph]

Dear Friend,

Good Day. I know this message might meet you in utmost surprise;
however, it's just my urgent need for a foreign partner that made me
to contact you for this mutual benefiting business when searching for
a good and reliable and trust worthy person. I got your contact email
address from a Business directory and decided to connect you to this
transaction that is based on trust and your understanding.

I am Mr.Patrick Joseph I work with the African Development Bank ADB
Ouagadougou Burkina-Faso, I would like to know if this proposal will
be worthwhile for your acceptance have a Foreign Customer, Paul-Louis
Halley from France who was an Investor, Crude Oil Merchant and Federal
Government Contractor, he was a victim with Socata TBM 700 killing 6
people crashed on 6 December 2003 near Paris leaving a closing balance
of $28.5 Million United States Dollars in one of his Private US dollar
Account that was been managed by me as his Customer's Account Officer.

Base on my security report, these funds can be claimed without any
hitches as no one is aware of the funds and it's closing balance
except me and the customer who is (Now Deceased) therefore, I can
present you as the Next of Kin and we will work out the modalities for
the claiming of the funds in accordance with the law.

Now, if you are interested and really sure of your trustworthy,
accountability and confidentiality on this transaction without
disappointment, you can contact me using my private email, and our
sharing ratio will be 50% for me and 40% for you, While 10% will be
for the necessary expenses that might occur along the line.

I expect your reply.

Sincerely,
Mr.Patrick Joseph.