[PATCH] change 'commands' to comments and improve wording

[Adrian]

---
 Documentation/git-stash.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 70191d06b69e..3899d36b2bab 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -233,7 +233,7 @@ return to your original branch to make the emergency fix, 
like this:
 $ git checkout -b my_wip
 $ git commit -a -m "WIP"
 $ git checkout master
-$ edit emergency fix
+# ... edit emergency fix ...
 $ git commit -a -m "Fix in a hurry"
 $ git checkout my_wip
 $ git reset --soft HEAD^
@@ -245,7 +245,7 @@ You can use 'git stash' to simplify the above, like this:
 
 # ... hack hack hack ...
 $ git stash
-$ edit emergency fix
+# ... edit emergency fix ...
 $ git commit -a -m "Fix in a hurry"
 $ git stash pop
 # ... continue hacking ...
@@ -261,11 +261,11 @@ each change before committing:
 # ... hack hack hack ...
 $ git add --patch foo# add just first part to the index
 $ git stash save --keep-index# save all other changes to the stash
-$ edit/build/test first part
+# ... edit, build and test first part ...
 $ git commit -m 'First part' # commit fully tested change
 $ git stash pop  # prepare to work on all other changes
 # ... repeat above five steps until one commit remains ...
-$ edit/build/test remaining parts
+# ... edit, build and test remaining parts ...
 $ git commit foo -m 'Remaining parts'
 
 

--
https://github.com/git/git/pull/361

Re: [PATCH v4 03/10] rebase -i: remove useless indentation

[liam Beguin]

Hi Stefan,

On 26/05/17 01:50 PM, Stefan Beller wrote:
> On Thu, May 25, 2017 at 8:15 PM, Liam Beguin  wrote:
>> Hi Johannes,
>>
>> Johannes Schindelin  writes:
>>> The commands used to be indented, and it is nice to look at, but when we
>>> transform the SHA-1s, the indentation is removed. So let's do away with it.
>>>
>>> For the moment, at least: when we will use the upcoming rebase--helper
>>> to transform the SHA-1s, we *will* keep the indentation and can
>>> reintroduce it. Yet, to be able to validate the rebase--helper against
>>> the output of the current shell script version, we need to remove the
>>> extra indentation.
>>>
>>> Signed-off-by: Johannes Schindelin 
>>> ---
>>>  git-rebase--interactive.sh | 14 +++---
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>>> index 609e150d38f..c40b1fd1d2e 100644
>>> --- a/git-rebase--interactive.sh
>>> +++ b/git-rebase--interactive.sh
>>> @@ -155,13 +155,13 @@ reschedule_last_action () {
>>>  append_todo_help () {
>>>   gettext "
>>>  Commands:
>>> - p, pick = use commit
>>> - r, reword = use commit, but edit the commit message
>>> - e, edit = use commit, but stop for amending
>>> - s, squash = use commit, but meld into previous commit
>>> - f, fixup = like \"squash\", but discard this commit's log message
>>> - x, exec = run command (the rest of the line) using shell
>>> - d, drop = remove commit
>>> +p, pick = use commit
>>> +r, reword = use commit, but edit the commit message
>>> +e, edit = use commit, but stop for amending
>>> +s, squash = use commit, but meld into previous commit
>>> +f, fixup = like \"squash\", but discard this commit's log message
>>> +x, exec = run command (the rest of the line) using shell
>>> +d, drop = remove commit
>>
>> do we also need to update all the translations since this is a `gettext`
>> function?
> 
> Translations are handled separately, later before a release is done.
> Separation of skills. ;)
> 
> As programming is quite complicated and involved we'd rather ask
> Johannes to only focus on the code in such a patch here and then later
> the translators will focus on getting the translation right. As the 
> translation
> tools are sophisticated, they will likely give the previous translation such
> that the translators see that there is only a white space change.

Thanks for the clarification, I was wondering how this part was handled.

> 
> But as the commit message hints at a later patch that will reintroduce the
> original indentation, maybe the translators won't even see that change?
> 
> For more information on how the translations work in the git workflow,
> see 951ea7656e (Merge tag 'l10n-2.13.0-rnd2.1' of
> git://github.com/git-l10n/git-po, 2017-05-09) or see
> https://public-inbox.org/git/canyiybgfdxj4jjtcd3ppxqsdn-twcc8dm8b9ov_3naszwsr...@mail.gmail.com/
> 

Liam 

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 26/05/17 18:07, Stefan Beller wrote:
> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
>  wrote:
>> Hmm, I'm not sure which documentation you are referring to,
> 
> Quite likely our fine manual pages. ;)
> 
>foreach [--recursive] 
>Evaluates an arbitrary shell command in each checked out submodule.
>The command has access to the variables $name, $path, $sha1 and
>$toplevel: $name is the name of the relevant submodule section in
>.gitmodules, $path is the name of the submodule directory relative
>to the superproject, $sha1 is the commit as recorded in the
>superproject, and $toplevel is the absolute path to the top-level
>of the superproject. Any submodules defined in the superproject but
>not checked out are ignored by this command. Unless given --quiet,
>foreach prints the name of each submodule before evaluating the
>command. If --recursive is given, submodules are traversed
>recursively (i.e. the given shell command is evaluated in nested
>submodules as well). A non-zero return from the command in any
>submodule causes the processing to terminate. This can be
>overridden by adding || : to the end of the command.

I suspected as much, but I was wondering specifically if $sm_path
had been documented anywhere. I didn't think so, but ...

> As $path is documented and $sm_path is not, we should care about
> $path first to be correct and either fix the documentation or the 
> implementation
> such that we have a consistent world view. :)

Sure, but what is that world view? :-D

I suspect that commit 091a6eb0fe did not intend (should not have)
used $sm_path in that test. If we were to 'fix' that test, would
it still work?

Back in 2012, the submodule list was generated by filtering the
output of 'git ls-files --error-unmatch --stage --'; but I don't
recall if (at that time) git-ls-files required being at the top
of the working tree, or if it would execute fine in a sub-directory.
So, it's possible that the documentation of $path was wrong all along.
;-)

At that time, by definition, $path == $sm_path. However, you know this
stuff much better than me (I don't use git-submodule), so ...

>> but if
>> $path != $sm_path then something is wrong. (unless their definition
>> has changed, of course).
> 
> I would lean in doing so (changing their definition):
> 
> $path (as documented) is the name of the submodule directory
> relative to the direct superproject (so in nested submodules you
> go up only one level).
> 
> $sm_path on the other hand is not documented at all and yields
> non-sense results in corner cases.

Hmm, at what point did '$sm_path yields non-sense results' start
being the case? (perhaps the corner cases need to be fixed first).

> With this patch it becomes less non-sensey and could be documented as:
> 
> $sm_path is the relative path from the current working directory
> to the submodule (ignoring relations to the superproject or nesting
> of submodules). 

OK.

>  This documentation also fits into the narrative of
> the test in t7407.

Hmm, does it?

ATB,
Ramsay Jones

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Johannes Sixt]

Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:

+   argv_array_pushf(_array, "path=%s", list_item->name);


Not good! On Windows, environment variables are case insensitive. The 
environment variable "path" has a very special purpose, although it is 
generally spelled "PATH" (actually "Path" on Windows).


Lowercase "path" may have worked as long as it was only used in a shell 
script (and perhaps only by lucky coincidence), but this I can pretty 
much guarantee to fail. (I haven't tested it, though.)


The correct fix can only be to rename this variable here and in shell 
scripts that need the value that is set here.


-- Hannes

[PATCH] wildmatch test: remove redundant duplicate test

[Ævar Arnfjörð Bjarmason]

Remove a test line that's exactly the same as the preceding
line.

This was brought in in commit feabcc173b ("Integrate wildmatch to
git", 2012-10-15), these tests are originally copied from rsync.git,
but the duplicate line was never present there, so must have just
snuck in during integration with git by accident.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t3070-wildmatch.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index ef509df351..7ca69f4bed 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -135,7 +135,6 @@ match 1 x '5' '[[:xdigit:]]'
 match 1 x 'f' '[[:xdigit:]]'
 match 1 x 'D' '[[:xdigit:]]'
 match 1 x '_' 
'[[:alnum:][:alpha:][:blank:][:cntrl:][:digit:][:graph:][:lower:][:print:][:punct:][:space:][:upper:][:xdigit:]]'
-match 1 x '_' 
'[[:alnum:][:alpha:][:blank:][:cntrl:][:digit:][:graph:][:lower:][:print:][:punct:][:space:][:upper:][:xdigit:]]'
 match 1 x '.' 
'[^[:alnum:][:alpha:][:blank:][:cntrl:][:digit:][:lower:][:space:][:upper:][:xdigit:]]'
 match 1 x '5' '[a-c[:digit:]x-z]'
 match 1 x 'b' '[a-c[:digit:]x-z]'
-- 
2.13.0.303.g4ebf302169

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Brandon Williams]

On 05/26, Johannes Sixt wrote:
> Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:
> >+argv_array_pushf(_array, "path=%s", list_item->name);
> 
> Not good! On Windows, environment variables are case insensitive.
> The environment variable "path" has a very special purpose, although
> it is generally spelled "PATH" (actually "Path" on Windows).
> 
> Lowercase "path" may have worked as long as it was only used in a
> shell script (and perhaps only by lucky coincidence), but this I can
> pretty much guarantee to fail. (I haven't tested it, though.)
> 
> The correct fix can only be to rename this variable here and in
> shell scripts that need the value that is set here.
> 
> -- Hannes

Nice catch, the only reason why this would have worked before was
because it was a shell script before...

-- 
Brandon Williams

[PATCH 1/1] diff.c: color moved lines differently

[Stefan Beller]

When a patch consists mostly of moving blocks of code around, it can
be quite tedious to ensure that the blocks are moved verbatim, and not
undesirably modified in the move. To that end, color blocks that are
moved within the same patch differently. For example (OM, del, add,
and NM are different colors):

[OM]  -void sensitive_stuff(void)
[OM]  -{
[OM]  -if (!is_authorized_user())
[OM]  -die("unauthorized");
[OM]  -sensitive_stuff(spanning,
[OM]  -multiple,
[OM]  -lines);
[OM]  -}

   void another_function()
   {
[del] -printf("foo");
[add] +printf("bar");
   }

[NM]  +void sensitive_stuff(void)
[NM]  +{
[NM]  +if (!is_authorized_user())
[NM]  +die("unauthorized");
[NM]  +sensitive_stuff(spanning,
[NM]  +multiple,
[NM]  +lines);
[NM]  +}

However adjacent blocks may be problematic. For example, in this
potentially malicious patch, the swapping of blocks can be spotted:

[OM]  -void sensitive_stuff(void)
[OM]  -{
[OMA] -if (!is_authorized_user())
[OMA] -die("unauthorized");
[OM]  -sensitive_stuff(spanning,
[OM]  -multiple,
[OM]  -lines);
[OMA] -}

   void another_function()
   {
[del] -printf("foo");
[add] +printf("bar");
   }

[NM]  +void sensitive_stuff(void)
[NM]  +{
[NMA] +sensitive_stuff(spanning,
[NMA] +multiple,
[NMA] +lines);
[NM]  +if (!is_authorized_user())
[NM]  +die("unauthorized");
[NMA] +}

If the moved code is larger, it is easier to hide some permutation in the
code, which is why some alternative coloring is needed.

As the reviewers attention should be brought to the places, where the
difference is introduced to the moved code, we cannot just have one new
color for all of moved code.

First I implemented an alternative design, which would try to fingerprint
a line by its neighbors to detect if we are in a block or at the boundary.
This idea iss error prone as it inspected each line and its neighboring
lines to determine if the line was (a) moved and (b) if was deep inside
a hunk by having matching neighboring lines. This is unreliable as the
we can construct hunks which have equal neighbors that just exceed the
number of lines inspected. (Think of 'AXYZBXYZCXYZD..' with each letter
as a line, that is permutated to AXYZCXYZBXYZD..').

Instead this provides a dynamic programming greedy algorithm that finds
the largest moved hunk and then has several modes on highlighting bounds.

A note on the options '--submodule=diff' and '--color-words/--word-diff':
In the conversion to use emit_line in the prior patches both submodules
as well as word diff output carefully chose to call emit_line with sign=0.
All output with sign=0 is ignored for move detection purposes in this
patch, such that no weird looking output will be generated for these
cases. This leads to another thought: We could pass on '--color-moved' to
submodules such that they color up moved lines for themselves. If we'd do
so only line moves within a repository boundary are marked up.

Helped-by: Jonathan Tan 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt   |  10 +-
 Documentation/diff-options.txt |  32 
 color.h|   2 +
 diff.c | 342 +++--
 diff.h |  15 +-
 t/t4015-diff-whitespace.sh | 373 +
 6 files changed, 760 insertions(+), 14 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d51..73511a4603 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1051,14 +1051,20 @@ This does not affect linkgit:git-format-patch[1] or the
 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the
 command line with the `--color[=]` option.
 
+diff.colorMoved::
+   If set moved lines in a diff are colored differently,
+   for details see '--color-moved' in linkgit:git-diff[1].
+
 color.diff.::
Use customized color for diff colorization.  `` specifies
which part of the patch to use the specified color, and is one
of `context` (context text - `plain` is a historical synonym),
`meta` (metainformation), `frag`
(hunk header), 'func' (function in hunk header), `old` (removed lines),
-   `new` (added lines), `commit` (commit headers), or `whitespace`
-   (highlighting whitespace errors).
+   `new` (added lines), `commit` (commit 

Re: [PATCHv4 00/17] Diff machine: highlight moved lines.

[Jacob Keller]

On Mon, May 22, 2017 at 7:40 PM, Stefan Beller  wrote:
> v4:
> * interdiff to v3 (what is currently origin/sb/diff-color-move) below.
> * renamed the "buffered_patch_line" to "diff_line". Originally I planned
>   to not carry the "line" part as it can be a piece of a line as well.
>   But for the intended functionality it is best to keep the name.
>   If we'd want to add more functionality to say have a move detection
>   for words as well, we'd rename the struct to have a better name then.
>   For now diff_line is the best. (Thanks Jonathan Nieder!)
> * tests to demonstrate it doesn't mess with --color-words as well as
>   submodules. (Thanks Jonathan Tan!)
> * added in the statics (Thanks Ramsay!)
> * smaller scope for the hashmaps (Thanks Jonathan Tan!)
> * some commit messages were updated, prior patch 4-7 is squashed into one
>   (Thanks Jonathan Tan!)
> * the tests added revealed an actual fault: now that the submodule process
>   is not attached to a dupe of our stdout, it would stop coloring the
>   output. We need to pass on use-color explicitly.
> * updated the NEEDSWORK comment in the second last patch.
>
> Thanks for bearing,
> Stefan
>

One thing to note when I was playing around with what's on pu right
now, I noticed that the oldMovedAlternative and newMovedAlternative
are the first moved colors to be used if there is only one move. (Ie:
a simple case of literally one section moved) This is a bit weird that
the alternative colors are used before the "main" colors. I would have
thought it would be the other way.

I noticed this because the default colors do not work well for my
terminal color scheme and I had to configure but realized that I
needed to configure the alternative ones to make a difference in the
simple diff I was viewing.

Thanks,
Jake

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Ramsay Jones]

On 26/05/17 22:54, Johannes Sixt wrote:
> Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:
>> +argv_array_pushf(_array, "path=%s", list_item->name);
> 
> Not good! On Windows, environment variables are case insensitive. The 
> environment variable "path" has a very special purpose, although it is 
> generally spelled "PATH" (actually "Path" on Windows).
> 
> Lowercase "path" may have worked as long as it was only used in a shell 
> script (and perhaps only by lucky coincidence), but this I can pretty much 
> guarantee to fail. (I haven't tested it, though.)
> 
> The correct fix can only be to rename this variable here and in shell scripts 
> that need the value that is set here.

Yeah, I already pointed to commit 64394e3ae9 (but it seems not
to have registered!), but ...

I tried provoking a failure on cygwin, and I couldn't get it to fail!
Since Johannes told me about Gfw fork of the msys-runtime, I didn't
even attempt to try and provoke a failure on MSYS2/MinGW.

So, maybe it's fixed (no I'm not convinced either) ...

ATB,
Ramsay Jones

Re: [Non-Bug] cloning a repository with a default MASTER branch tries to check out the master branch

[Philip Oakley]

adding and updating an example..
From: "Philip Oakley" 

been trying to keep up...

From: "Jeff King" 

On Thu, May 25, 2017 at 11:59:24AM -0400, Jeff King wrote:


The just-HEAD case could look like:


This patch does work, in the sense that upload-pack advertises the
unborn HEAD symref. But the client doesn't actually do anything with it.
The capability parsing happens in get_remote_heads(), which passes the
data out by adding an annotation to the "struct ref" list. But of course
we have no HEAD ref to annotate.

So either get_remote_heads() would have to start returning a bogus HEAD
ref (with a null sha1, I guess, which all callers would have to
recognize). Or clone (and probably "remote set-head -a") would have to
start reaching across the transport-module boundary and asking for any
symref values for "HEAD". I'm not excited about more special-casing of
"HEAD", though. In theory we'd want this for other symrefs in the long
run, and it would be nice if clients were ready to handle that (even if
the protocol isn't quite there).

I dunno. I was thinking there might be a quick tweak, but I'm wondering
if this arcane case is worth the restructuring we'd have to do to
support it. It only comes up when you've moved the server repo's HEAD to
an unborn branch _and_ you have other refs (since otherwise we don't
even send capabilities at all!).

-Peff


My original comment regarding Felix's report was based on when I was 
looking at the bundle code's disambiguation of refs which matched HEAD's 
sha1. Hence I had a mis-remembered impression that the HEAD - symref 
matching was avaibable.


At that time, Junio had suggested that, at least in the bundle file, the 
HEAD symref could be advertised immediately after a nul on the ref line.


At least for regular git, the sha1and symref value would included in the 
read line, and the current string processing [1] would not notice the 
extra symref data. This extra data could then be read (if present) from 
the end of the line, and the HEAD symref set.


What I then noticed (but didn't report to the list) was the option of 
adding that extra info to the PKLINE protocol.



In technical\pack-protocol.txt #L136;158-160
Reference Discovery:

If HEAD is a valid ref, HEAD MUST appear as the first advertised
ref.  If HEAD is not a valid ref, HEAD MUST NOT appear in the
advertisement list at all, but other refs may still appear.
-

So, (for both upload pack, and bundle refs) the place to hide the HEAD is 
after the later ref that HEAD points to.

e.g.(updating the example at #L147):
  00441d3fcd5ced445d1abc402225c0b8a1299641f497 
refs/heads/integration\0HEAD[LF]


The potential issue is if there is a passed ref that is HEAD, but that 
HEAD itself isn't passed (especially for bundle)

<\from my notes>
--

However given the discussion about an unborn HEAD, the option here would 
be to also pass the NULL sha for the symref and then add the annotation 
'HEAD' after an extra \0, in the same way that an active symref could be 
annotated with the '\0HEAD'. This would kill two birds with one stone!


These are still protocol changes but should squeeze into the existing 
processing using the \0 trick.


In the absence of the information, and the multi-use of the warning 
function, the current message is possible the best we can get.


Philip

[1] the question would be whether other git versions also use the same 
string processing so could be 'fooled' in the same way? I'd be interested 
to know if that was a possibility.


Updating the original example with the suggestion of adding the unborn ref 
and a \0HEAD marker (sort order may be an issue if it is the first entry 
which 'clashes' with the capability string... - I've been lenient here)


 $ git ls-remote git://github.com/passcod/UPPERCASE-NPM.git
 efc7dbfd6ca155d5d19ce67eb98603896062f35a refs/heads/MASTER
  refs/heads/master\0HEAD
 e60ea8e6ec45ec45ff44ac8939cb4105b16477da refs/pull/1/head
 f35a73dcb151d336dc3d30c9a2c7423ecdb7bd1c refs/pull/2/head
 0d9b3a1268ff39350e04a7183af0add912b686e6 refs/tags/V1.0.0
 efc7dbfd6ca155d5d19ce67eb98603896062f35a refs/tags/V1.0.1

--
Philip

[PATCH 0/1] Diff machine: highlight moved lines.

[Stefan Beller]

This is a new version of the latest patch of sb/diff-color-move.

It seems as if different people prefer different settings for the
boundary/alternate coloring, so let's have all of them. (As the
block detection is rather simple we do not need a lot of overhead
to support multiple modes).

With more tests:
 * for each mode;
 * check that diff.colorMoved is respected
 
And it is documented as well.

Thanks,
Stefan

Stefan Beller (1):
  diff.c: color moved lines differently

 Documentation/config.txt   |  10 +-
 Documentation/diff-options.txt |  32 
 color.h|   2 +
 diff.c | 342 +++--
 diff.h |  15 +-
 t/t4015-diff-whitespace.sh | 373 +
 6 files changed, 760 insertions(+), 14 deletions(-)

Interdiff to what Junio has queued:
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 902d017c3b..73511a4603 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1051,12 +1051,9 @@ This does not affect linkgit:git-format-patch[1] or the
 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the
 command line with the `--color[=]` option.
 
-color.moved::
-   A boolean value, whether a diff should color moved lines
-   differently. The moved lines are searched for in the diff only.
-   Duplicated lines from somewhere in the project that are not
-   part of the diff are not colored as moved.
-   Defaults to false.
+diff.colorMoved::
+   If set moved lines in a diff are colored differently,
+   for details see '--color-moved' in linkgit:git-diff[1].
 
 color.diff.::
Use customized color for diff colorization.  `` specifies
@@ -1065,10 +1062,9 @@ color.diff.::
`meta` (metainformation), `frag`
(hunk header), 'func' (function in hunk header), `old` (removed lines),
`new` (added lines), `commit` (commit headers), `whitespace`
-   (highlighting whitespace errors), `oldMoved` (removed lines that
-   reappear), `newMoved` (added lines that were removed elsewhere),
-   `oldMovedAlternative` and `newMovedAlternative` (as a fallback to
-   cover adjacent blocks of moved code)
+   (highlighting whitespace errors), `oldMoved`, `newMoved`,
+   `oldMovedAlternative` and `newMovedAlternative` (See the ''
+   setting of '--color-moved' in linkgit:git-diff[1] for details).
 
 color.decorate.::
Use customized color for 'git log --decorate' output.  `` is one
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 89cc0f48de..25259dbbc3 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -231,6 +231,38 @@ ifdef::git-diff[]
 endif::git-diff[]
It is the same as `--color=never`.
 
+--color-moved[=]::
+   Moved lines of code are colored differently.
+ifdef::git-diff[]
+   It can be changed by the `diff.colorMoved` configuration setting.
+endif::git-diff[]
+   The  defaults to 'no' if the option is not given
+   and to 'adjacentbounds' if the option with no mode is given.
+   The mode must be one of:
++
+--
+no::
+   Moved lines are not highlighted.
+nobounds::
+   Any line that is added in on location and was removed
+   in another location will be colored with 'color.diff.newmoved'.
+   Any line that is removed in on location and was added
+   in another location will be colored with 'color.diff.oldmoved'.
+allbounds::
+   Based on 'nobounds'. Additionally blocks of moved code are
+   detected and the first and last line of a block will be highlighted
+   using 'color.diff.newMovedAlternate' or
+   'color.diff.oldMovedAlternate'.
+adjacentbounds::
+   The same as 'allbounds' except that highlighting is only performed
+   at adjacent block boundaries of blocks that have the same sign.
+alternate::
+   Based on 'nobounds'. Additionally blocks of moved code are
+   detected. If moved blocks are adjacent mark one of them with the
+   alternative move color using 'color.diff.newMovedAlternate' or
+   'color.diff.oldMovedAlternate'.
+--
+
 --word-diff[=]::
Show a word diff, using the  to delimit changed words.
By default, words are delimited by whitespace; see
diff --git a/color.h b/color.h
index 90627650fc..04b3b87929 100644
--- a/color.h
+++ b/color.h
@@ -42,6 +42,8 @@ struct strbuf;
 #define GIT_COLOR_BG_BLUE  "\033[44m"
 #define GIT_COLOR_BG_MAGENTA   "\033[45m"
 #define GIT_COLOR_BG_CYAN  "\033[46m"
+#define GIT_COLOR_DI_IT_CYAN   "\033[2;3;36m"
+#define GIT_COLOR_DI_IT_MAGENTA"\033[2;3;35m"
 
 /* A special value meaning "no color selected" */
 #define GIT_COLOR_NIL "NIL"
diff --git a/diff.c b/diff.c
index 40cccafb67..efd2530a89 100644
--- a/diff.c
+++ b/diff.c
@@ -56,10 +56,10 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_YELLOW,   /* COMMIT */

Re: [Non-Bug] cloning a repository with a default MASTER branch tries to check out the master branch

[Philip Oakley]

been trying to keep up...

From: "Jeff King" 

On Thu, May 25, 2017 at 11:59:24AM -0400, Jeff King wrote:


The just-HEAD case could look like:


This patch does work, in the sense that upload-pack advertises the
unborn HEAD symref. But the client doesn't actually do anything with it.
The capability parsing happens in get_remote_heads(), which passes the
data out by adding an annotation to the "struct ref" list. But of course
we have no HEAD ref to annotate.

So either get_remote_heads() would have to start returning a bogus HEAD
ref (with a null sha1, I guess, which all callers would have to
recognize). Or clone (and probably "remote set-head -a") would have to
start reaching across the transport-module boundary and asking for any
symref values for "HEAD". I'm not excited about more special-casing of
"HEAD", though. In theory we'd want this for other symrefs in the long
run, and it would be nice if clients were ready to handle that (even if
the protocol isn't quite there).

I dunno. I was thinking there might be a quick tweak, but I'm wondering
if this arcane case is worth the restructuring we'd have to do to
support it. It only comes up when you've moved the server repo's HEAD to
an unborn branch _and_ you have other refs (since otherwise we don't
even send capabilities at all!).

-Peff


My original comment regarding Felix's report was based on when I was looking 
at the bundle code's disambiguation of refs which matched HEAD's sha1. Hence 
I had a mis-remembered impression that the HEAD - symref matching was 
avaibable.


At that time, Junio had suggested that, at least in the bundle file, the 
HEAD symref could be advertised immediately after a nul on the ref line.


At least for regular git, the sha1and symref value would included in the 
read line, and the current string processing [1] would not notice the extra 
symref data. This extra data could then be read (if present) from the end of 
the line, and the HEAD symref set.


What I then noticed (but didn't report to the list) was the option of adding 
that extra info to the PKLINE protocol.



In technical\pack-protocol.txt #L136;158-160
Reference Discovery:

If HEAD is a valid ref, HEAD MUST appear as the first advertised
ref.  If HEAD is not a valid ref, HEAD MUST NOT appear in the
advertisement list at all, but other refs may still appear.
-

So, (for both upload pack, and bundle refs) the place to hide the HEAD is 
after the later ref that HEAD points to.

e.g.(updating the example at #L147):
  00441d3fcd5ced445d1abc402225c0b8a1299641f497 
refs/heads/integration\0HEAD[LF]


The potential issue is if there is a passed ref that is HEAD, but that HEAD 
itself isn't passed (especially for bundle)

<\from my notes>
--

However given the discussion about an unborn HEAD, the option here would be 
to also pass the NULL sha for the symref and then add the annotation 'HEAD' 
after an extra \0, in the same way that an active symref could be annotated 
with the '\0HEAD'. This would kill two birds with one stone!


These are still protocol changes but should squeeze into the existing 
processing using the \0 trick.


In the absence of the information, and the multi-use of the warning 
function, the current message is possible the best we can get.


Philip

[1] the question would be whether other git versions also use the same 
string processing so could be 'fooled' in the same way? I'd be interested to 
know if that was a possibility.

Re: [PATCHv5 00/17] Diff machine: highlight moved lines.

[Stefan Beller]

On Thu, May 25, 2017 at 6:20 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> As you turn on/off normal coloring via "color.diff" and this only extends
>> the coloring scheme, I have the impression "color" is the right section.
>> Maybe color.diffStyle=[normal/enhanced] and "enhanced" means this
>> feature for now?
>
> Hmph, I thought the intent of color.diff is "is the diff command
> itself is colored?"  In other words, color.diff=false should give
> you monochrome if you say "diff --word-diff", etc.

Yes, but in my understanding the "diff" doesn't apply to
the command, but the part of the output. I arrived at that
understanding as other commands (show/log/..) also respect
that setting, so the "diff" in color.diff is not the command, but
referring to something else, the output being the closest match. :)

And by that understanding color.diffStyle is just natural?

But I think that setting would be a bad idea as we'd rather have
multiple uncorrelated settings for coloring, which a style argument
would not.

>> The only option in the "diff" section related to color is 
>> diff.wsErrorHighlight
>> which has a very similar purpose, so "diff.colorMoved" would fit in that
>> scheme.

With the above reasoning, this may be missnamed and should rather be
color.wsErrorHighlight as it applies to more than just the diff command.

Note: The average user may not aware that diff/show/log are tiny wrappers
around the same backend for the heavy lifting.

> I didn't have "should diff output highlight whitespace errors?" in
> mind when I wrote the message you are responding to, but yes, that
> is quite similar to "should diff output show lines moved and lines
> deleted/added differently?".
>
>> So with these questions, I wonder if we want to color moved lines
>> as "color.diff.context" (i.e. plain white text in the normal coloring scheme)
>> This would serve the intended purpose of
>> dimming the attention to moved lines.
>
> Yes, but two points.
>
>  (1) We want to do so while making it obvious where the boundary
>  between two moved blocks of text whose destination (for
>  moved-deleted lines) or source (for moved-added lines) is.

Yes, that is what the boundary color would accomplish.
Any two adjacent blocks with the same sign would have
their boundary line colored this way.

>  (2) My message was an impression from using the code to review a
>  patch that is meant to be "move without changing other things".

Ok, glad you found it somewhat useful already.

>  For other purposes, there may be cases where moved ones may
>  want to be highlighted, not dimmed.

I wonder what these use cases would be?
(barring a --find-copies harder extension that would not just search the
current diff, but the whole tree)

That hints at just having an extra slot for the moved block, but the default
color could be the same as color.diff.context for dimming.

By now I also think we may not need different colors for additions
or removals, but keeping two colors is easy enough.

>> Regarding the last point of marking up adjacent blocks (which would
>> indicate that there is a coherency issue or just moving from different
>> places), we could highlight the last line of the previous block
>> and first line of the next block in their "normal" colors (i.e.
>> color.diff.old and color.diff.new).
>
> Hmm.  That is an interesting thought.

I'll try the implementation for that and see if it looks good.

Thanks,
Stefan

[PATCH 7/8] builtin/push.c: respect 'submodule.recurse' option

[Stefan Beller]

The closest mapping from the boolean 'submodule.recurse' set to "yes"
to the variety of submodule push modes is "on-demand", so implement that.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/push.c |  4 
 t/t5531-deep-submodule-push.sh | 21 +
 2 files changed, 25 insertions(+)

diff --git a/builtin/push.c b/builtin/push.c
index a597759d8f..258648d5fd 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -498,6 +498,10 @@ static int git_push_config(const char *k, const char *v, 
void *cb)
const char *value;
if (!git_config_get_value("push.recursesubmodules", ))
recurse_submodules = 
parse_push_recurse_submodules_arg(k, value);
+   } else if (!strcmp(k, "submodule.recurse")) {
+   int val = git_config_bool(k, v) ?
+   RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
+   recurse_submodules = val;
}
 
return git_default_config(k, v, NULL);
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 57ba322628..712c595fd8 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -126,6 +126,27 @@ test_expect_success 'push succeeds if submodule commit not 
on remote but using o
)
 '
 
+test_expect_success 'push succeeds if submodule commit not on remote but using 
auto-on-demand via submodule.recurse config' '
+   (
+   cd work/gar/bage &&
+   >recurse-on-demand-from-submodule-recurse-config &&
+   git add recurse-on-demand-from-submodule-recurse-config &&
+   git commit -m "Recurse submodule.recurse from config junk"
+   ) &&
+   (
+   cd work &&
+   git add gar/bage &&
+   git commit -m "Recurse submodule.recurse from config for 
gar/bage" &&
+   git -c submodule.recurse push ../pub.git master &&
+   # Check that the supermodule commit got there
+   git fetch ../pub.git &&
+   git diff --quiet FETCH_HEAD master &&
+   # Check that the submodule commit got there too
+   cd gar/bage &&
+   git diff --quiet origin/master master
+   )
+'
+
 test_expect_success 'push recurse-submodules on command line overrides config' 
'
(
cd work/gar/bage &&
-- 
2.13.0.17.g582985b1e4

[PATCH 2/8] submodule test invocation: only pass additional arguments

[Stefan Beller]

In a later patch we want to introduce a config option to trigger the
submodule recursing by default. As this option should be available and
uniform across all commands that deal with submodules we'd want to test
for this option in the submodule update library.

So instead of calling the whole test set again for
"git -c submodule.recurse foo" instead of "git foo --recurse-submodules",
we'd only want to introduce one basic test that tests if the option is
recognized and respected to not overload the test suite.

Change the test functions by taking only the argument and assemble the
command inside the test function by embedding the arguments into the
command that is "git $arguments --recurse-submodules".

It would be nice to do this for all functions in lib-submodule-update,
but we cannot do that for the non-recursing tests, as there we do not
just pass in a git command but whole functions. (See t3426 for example)

Signed-off-by: Stefan Beller 
---
 t/lib-submodule-update.sh  | 10 ++
 t/t1013-read-tree-submodule.sh |  4 ++--
 t/t2013-checkout-submodule.sh  |  4 ++--
 t/t7112-reset-submodule.sh |  4 ++--
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index f0b1b18206..0272c4d8ca 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -781,8 +781,9 @@ test_submodule_forced_switch () {
 # - Removing a submodule with a git directory absorbs the submodules
 #   git directory first into the superproject.
 
-test_submodule_switch_recursing () {
-   command="$1"
+test_submodule_switch_recursing_with_args () {
+   cmd_args="$1"
+   command="git $cmd_args --recurse-submodules"
RESULTDS=success
if test "$KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS" = 1
then
@@ -1021,8 +1022,9 @@ test_submodule_switch_recursing () {
 # Test that submodule contents are updated when switching between commits
 # that change a submodule, but throwing away local changes in
 # the superproject as well as the submodule is allowed.
-test_submodule_forced_switch_recursing () {
-   command="$1"
+test_submodule_forced_switch_recursing_with_args () {
+   cmd_args="$1"
+   command="git $cmd_args --recurse-submodules"
RESULT=success
if test "$KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS" = 1
then
diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh
index de1ba02dc5..2c8d620324 100755
--- a/t/t1013-read-tree-submodule.sh
+++ b/t/t1013-read-tree-submodule.sh
@@ -9,9 +9,9 @@ KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
 KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1
 
-test_submodule_switch_recursing "git read-tree --recurse-submodules -u -m"
+test_submodule_switch_recursing_with_args "read-tree -u -m"
 
-test_submodule_forced_switch_recursing "git read-tree --recurse-submodules -u 
--reset"
+test_submodule_forced_switch_recursing_with_args "read-tree -u --reset"
 
 test_submodule_switch "git read-tree -u -m"
 
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index e8f70b806f..c962a02277 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -65,9 +65,9 @@ test_expect_success '"checkout " honors 
submodule.*.ignore from .git/
 
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
 KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
-test_submodule_switch_recursing "git checkout --recurse-submodules"
+test_submodule_switch_recursing_with_args "checkout"
 
-test_submodule_forced_switch_recursing "git checkout -f --recurse-submodules"
+test_submodule_forced_switch_recursing_with_args "checkout -f"
 
 test_submodule_switch "git checkout"
 
diff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh
index f86ccdf215..a1cb9ff858 100755
--- a/t/t7112-reset-submodule.sh
+++ b/t/t7112-reset-submodule.sh
@@ -9,9 +9,9 @@ KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
 KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1
 
-test_submodule_switch_recursing "git reset --recurse-submodules --keep"
+test_submodule_switch_recursing_with_args "reset --keep"
 
-test_submodule_forced_switch_recursing "git reset --hard --recurse-submodules"
+test_submodule_forced_switch_recursing_with_args "reset --hard"
 
 test_submodule_switch "git reset --keep"
 
-- 
2.13.0.17.g582985b1e4

[PATCH 6/8] builtin/grep.c: respect 'submodule.recurse' option

[Stefan Beller]

In builtin/grep.c we parse the config before evaluating the command line
options. This makes the task of teaching grep to respect the new config
option 'submodule.recurse' very easy by just parsing that option.

As an alternative I had implemented a similar structure to treat
submodules as the fetch/push command have, including
* aligning the meaning of the 'recurse_submodules' to possible submodule
  values RECURSE_SUBMODULES_* as defined in submodule.h.
* having a callback to parse the value and
* reacting to the RECURSE_SUBMODULES_DEFAULT state that was the initial
  state.

However all this is not needed for a true boolean value, so let's keep
it simple. However this adds another place where "submodule.recurse" is
parsed.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/grep.c |  3 +++
 t/t7814-grep-recurse-submodules.sh | 18 ++
 2 files changed, 21 insertions(+)

diff --git a/builtin/grep.c b/builtin/grep.c
index b1095362fb..454e263820 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -302,6 +302,9 @@ static int grep_cmd_config(const char *var, const char 
*value, void *cb)
 #endif
}
 
+   if (!strcmp(var, "submodule.recurse"))
+   recurse_submodules = git_config_bool(var, value);
+
return st;
 }
 
diff --git a/t/t7814-grep-recurse-submodules.sh 
b/t/t7814-grep-recurse-submodules.sh
index 3a58197f47..7184113b9b 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -33,6 +33,24 @@ test_expect_success 'grep correctly finds patterns in a 
submodule' '
test_cmp expect actual
 '
 
+test_expect_success 'grep finds patterns in a submodule via config' '
+   test_config submodule.recurse true &&
+   # expect from previous test
+   git grep -e "(3|4)" >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'grep --no-recurse-submodules overrides config' '
+   test_config submodule.recurse true &&
+   cat >expect <<-\EOF &&
+   a:(1|2)d(3|4)
+   b/b:(3|4)
+   EOF
+
+   git grep -e "(3|4)" --no-recurse-submodules >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'grep and basic pathspecs' '
cat >expect <<-\EOF &&
submodule/a:(1|2)d(3|4)
-- 
2.13.0.17.g582985b1e4

[PATCH 3/8] reset/checkout/read-tree: unify config callback for submodule recursion

[Stefan Beller]

The callback function is essentially duplicated 3 times. Remove all
of them and offer a new callback function, that lives in submodule.c

By putting the callback function there, we no longer need the function
'set_config_update_recurse_submodules', nor duplicate the global variable
in each builtin as well as submodule.c

In the three builtins we have different 2 ways how to load the .gitmodules
and config file, which are slightly different. git-checkout has to load
the submodule config all the time due to 23b4c7bcc5 (checkout: Use
submodule.*.ignore settings from .git/config and .gitmodules, 2010-08-28)

git-reset and git-read-tree do not respect these diff settings, so loading
the submodule configuration is optional. Also put that into submodule.c
for code deduplication.

Signed-off-by: Stefan Beller 
---
 builtin/checkout.c  | 27 +--
 builtin/read-tree.c | 28 +++-
 builtin/reset.c | 27 ++-
 submodule.c | 33 +++--
 submodule.h |  6 +-
 5 files changed, 38 insertions(+), 83 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0fd57672cc..acff6039d6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -21,31 +21,12 @@
 #include "submodule-config.h"
 #include "submodule.h"
 
-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
-
 static const char * const checkout_usage[] = {
N_("git checkout [] "),
N_("git checkout [] [] -- ..."),
NULL,
 };
 
-static int option_parse_recurse_submodules(const struct option *opt,
-  const char *arg, int unset)
-{
-   if (unset) {
-   recurse_submodules = RECURSE_SUBMODULES_OFF;
-   return 0;
-   }
-   if (arg)
-   recurse_submodules =
-   parse_update_recurse_submodules_arg(opt->long_name,
-   arg);
-   else
-   recurse_submodules = RECURSE_SUBMODULES_ON;
-
-   return 0;
-}
-
 struct checkout_opts {
int patch_mode;
int quiet;
@@ -1183,7 +1164,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
 N_("do not check if another worktree is holding the 
given ref")),
{ OPTION_CALLBACK, 0, "recurse-submodules", NULL,
"checkout", "control recursive updating of 
submodules",
-   PARSE_OPT_OPTARG, option_parse_recurse_submodules },
+   PARSE_OPT_OPTARG, 
option_parse_recurse_submodules_worktree_updater },
OPT_BOOL(0, "progress", _progress, N_("force progress 
reporting")),
OPT_END(),
};
@@ -1214,12 +1195,6 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
}
 
-   if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
-   git_config(submodule_config, NULL);
-   if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
-   
set_config_update_recurse_submodules(recurse_submodules);
-   }
-
if ((!!opts.new_branch + !!opts.new_branch_force + 
!!opts.new_orphan_branch) > 1)
die(_("-b, -B and --orphan are mutually exclusive"));
 
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 2a1b8a530e..8a889ef4c3 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -21,7 +21,6 @@
 static int nr_trees;
 static int read_empty;
 static struct tree *trees[MAX_UNPACK_TREES];
-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 
 static int list_tree(unsigned char *sha1)
 {
@@ -99,23 +98,6 @@ static int debug_merge(const struct cache_entry * const 
*stages,
return 0;
 }
 
-static int option_parse_recurse_submodules(const struct option *opt,
-  const char *arg, int unset)
-{
-   if (unset) {
-   recurse_submodules = RECURSE_SUBMODULES_OFF;
-   return 0;
-   }
-   if (arg)
-   recurse_submodules =
-   parse_update_recurse_submodules_arg(opt->long_name,
-   arg);
-   else
-   recurse_submodules = RECURSE_SUBMODULES_ON;
-
-   return 0;
-}
-
 static struct lock_file lock_file;
 
 int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
@@ -159,7 +141,7 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
 N_("debug unpack-trees")),
{ OPTION_CALLBACK, 0, "recurse-submodules", NULL,
"checkout", "control recursive updating of 
submodules",
-   PARSE_OPT_OPTARG, option_parse_recurse_submodules },
+   

[PATCH 5/8] Introduce 'submodule.recurse' option for worktree manipulators

[Stefan Beller]

Any command that understands '--recurse-submodules' can have its
default changed to true, by setting the new 'submodule.recurse'
option.

This patch includes read-tree/checkout/reset for working tree
manipulating commands. Later patches will cover other commands.

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt  |  5 +
 builtin/checkout.c|  2 +-
 builtin/read-tree.c   | 10 +-
 builtin/reset.c   | 10 +-
 submodule.c   | 23 +--
 submodule.h   |  1 +
 t/lib-submodule-update.sh | 12 
 7 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d51..e367becf72 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3063,6 +3063,11 @@ submodule.active::
submodule's path to determine if the submodule is of interest to git
commands.
 
+submodule.recurse::
+   Specifies if commands recurse into submodules by default. This
+   applies to all commands that have a `--recurse-submodules` option.
+   Defaults to false.
+
 submodule.fetchJobs::
Specifies how many submodules are fetched/cloned at the same time.
A positive integer allows up to that number of submodules fetched
diff --git a/builtin/checkout.c b/builtin/checkout.c
index acff6039d6..9ccc4a1d52 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -854,7 +854,7 @@ static int git_checkout_config(const char *var, const char 
*value, void *cb)
}
 
if (starts_with(var, "submodule."))
-   return parse_submodule_config_option(var, value);
+   return submodule_config(var, value, NULL);
 
return git_xmerge_config(var, value, NULL);
 }
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 8a889ef4c3..6dd70cd430 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -98,6 +98,14 @@ static int debug_merge(const struct cache_entry * const 
*stages,
return 0;
 }
 
+int git_read_tree_config(const char *var, const char *value, void *cb)
+{
+   if (!strcmp(var, "submodule.recurse"))
+   return git_default_submodule_config(var, value, cb);
+
+   return git_default_config(var, value, cb);
+}
+
 static struct lock_file lock_file;
 
 int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
@@ -150,7 +158,7 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
opts.src_index = _index;
opts.dst_index = _index;
 
-   git_config(git_default_config, NULL);
+   git_config(git_read_tree_config, NULL);
 
argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 read_tree_usage, 0);
diff --git a/builtin/reset.c b/builtin/reset.c
index 6f89dc5494..8ccdb7437e 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -266,6 +266,14 @@ static int reset_refs(const char *rev, const struct 
object_id *oid)
return update_ref_status;
 }
 
+int git_reset_config(const char *var, const char *value, void *cb)
+{
+   if (!strcmp(var, "submodule.recurse"))
+   return git_default_submodule_config(var, value, cb);
+
+   return git_default_config(var, value, cb);
+}
+
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
int reset_type = NONE, update_ref_status = 0, quiet = 0;
@@ -294,7 +302,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   git_config(git_default_config, NULL);
+   git_config(git_reset_config, NULL);
 
argc = parse_options(argc, argv, prefix, options, git_reset_usage,
PARSE_OPT_KEEP_DASHDASH);
diff --git a/submodule.c b/submodule.c
index 78cccb7563..2b157dc995 100644
--- a/submodule.c
+++ b/submodule.c
@@ -16,6 +16,7 @@
 #include "quote.h"
 #include "remote.h"
 #include "worktree.h"
+#include "parse-options.h"
 
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
@@ -170,10 +171,28 @@ static int git_modules_config(const char *var, const char 
*value, void *cb)
return 0;
 }
 
-/* Loads all submodule settings from the config */
+/* Loads all submodule settings from the config. */
 int submodule_config(const char *var, const char *value, void *cb)
 {
-   return git_modules_config(var, value, cb);
+   if (!strcmp(var, "submodule.recurse")) {
+   int v = git_config_bool(var, value) ?
+   RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
+   config_update_recurse_submodules = v;
+   return 0;
+   } else {
+   return git_modules_config(var, value, cb);
+   }
+}
+
+/* Cheap function that only determines if we're interested in submodules at 
all */
+int git_default_submodule_config(const char 

[PATCH 5/6] verify_filename(): treat ":(magic)" as a pathspec

[Jeff King]

For commands that take revisions and pathspecs, magic
pathspecs like ":(exclude)foo" require the user to specify
a disambiguating "--", since they do not match a file in the
filesystem, like:

  git grep foo -- :(exclude)bar

This makes them more annoying to use than they need to be.
We loosened the rules for wildcards in 28fcc0b71 (pathspec:
avoid the need of "--" when wildcard is used, 2015-05-02).
Let's do the same for pathspecs with long-form magic.

We already handle the short-forms ":/" and ":^" specially in
check_filename(), so we don't need to handle them here. And
in fact, we could do the same with long-form magic, parsing
out the actual filename and making sure it exists. But there
are a few reasons not to do it that way:

  - the parsing gets much more complicated, and we'd want to
hand it off to the pathspec code. But that code isn't
ready to do this kind of speculative parsing (it's happy
to die() when it sees a syntactically invalid pathspec).

  - not all pathspec magic maps to a filesystem path. E.g.,
:(attr) should be treated as a pathspec regardless of
what is in the filesystem

  - we can be a bit looser with ":(" than with the
short-form ":/", because it is much less likely to have
a false positive. Whereas ":/" also means "search for a
commit with this regex".

Note that because the change is in verify_filename() and not
in its helper check_filename(), this doesn't affect the
verify_non_filename() case. I.e., if an item that matches
our new rule doesn't resolve as an object, we may fallback
to treating it as a pathspec (rather than complaining it
doesn't exist). But if it does resolve (e.g., as a file in
the index that starts with an open-paren), we won't then
complain that it's also a valid pathspec. This matches the
wildcard-exception behavior.

And of course in either case, one can always insert the "--"
to get more precise results.

Signed-off-by: Jeff King 
---
 setup.c   | 20 +++-
 t/t4208-log-magic-pathspec.sh | 13 +
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 86bb7c9a9..89fcc12ab 100644
--- a/setup.c
+++ b/setup.c
@@ -185,6 +185,24 @@ static void NORETURN die_verify_filename(const char 
*prefix,
 
 }
 
+/*
+ * Check for arguments that don't resolve as actual files,
+ * but which look sufficiently like pathspecs that we'll consider
+ * them such for the purposes of rev/pathspec DWIM parsing.
+ */
+static int looks_like_pathspec(const char *arg)
+{
+   /* anything with a wildcard character */
+   if (!no_wildcard(arg))
+   return 1;
+
+   /* long-form pathspec magic */
+   if (starts_with(arg, ":("))
+   return 1;
+
+   return 0;
+}
+
 /*
  * Verify a filename that we got as an argument for a pathspec
  * entry. Note that a filename that begins with "-" never verifies
@@ -211,7 +229,7 @@ void verify_filename(const char *prefix,
 {
if (*arg == '-')
die("bad flag '%s' used after filename", arg);
-   if (check_filename(prefix, arg) || !no_wildcard(arg))
+   if (check_filename(prefix, arg) || looks_like_pathspec(arg))
return;
die_verify_filename(prefix, arg, diagnose_misspelt_rev);
 }
diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
index 67250ebdc..25129dfa0 100755
--- a/t/t4208-log-magic-pathspec.sh
+++ b/t/t4208-log-magic-pathspec.sh
@@ -65,6 +65,19 @@ test_expect_success  '"git log :!sub" behaves the same as 
:^' '
test_must_fail git log :!does-not-exist
 '
 
+test_expect_success '"git log :(exclude)sub" is not ambiguous' '
+   git log ":(exclude)sub"
+'
+
+test_expect_success '"git log :(exclude)sub --" must resolve as an object' '
+   test_must_fail git log ":(exclude)sub" --
+'
+
+test_expect_success '"git log :(unknown-magic) complains of bogus magic' '
+   test_must_fail git log ":(unknown-magic)" 2>error &&
+   test_i18ngrep pathspec.magic error
+'
+
 test_expect_success 'command line pathspec parsing for "git log"' '
git reset --hard &&
>a &&
-- 
2.13.0.496.ge44ba89db

[PATCHv2 0/8] A reroll of sb/submodule-blanket-recursive

[Stefan Beller]

v2:
* A reroll of sb/submodule-blanket-recursive.
* This requires ab/grep-preparatory-cleanup 
* It changed a lot from v1, as in v1 the tests did not work,
  hence the code was broken. Now it actually works.
* it also includes grep, fetch, push in addition to plain working tree
  manipulators.

Thanks,
Stefan

Stefan Beller (8):
  submodule recursing: do not write a config variable twice
  submodule test invocation: only pass additional arguments
  reset/checkout/read-tree: unify config callback for submodule
recursion
  submodule loading: separate code path for .gitmodules and config
overlay
  Introduce 'submodule.recurse' option for worktree manipulators
  builtin/grep.c: respect 'submodule.recurse' option
  builtin/push.c: respect 'submodule.recurse' option
  builtin/fetch.c: respect 'submodule.recurse' option

 Documentation/config.txt   |  5 +++
 builtin/checkout.c | 31 ++
 builtin/fetch.c|  7 +
 builtin/grep.c |  3 ++
 builtin/push.c |  4 +++
 builtin/read-tree.c| 32 ++-
 builtin/reset.c| 39 +++
 submodule.c| 64 +-
 submodule.h|  7 -
 t/lib-submodule-update.sh  | 22 ++---
 t/t1013-read-tree-submodule.sh |  4 +--
 t/t2013-checkout-submodule.sh  |  4 +--
 t/t5526-fetch-submodules.sh| 10 ++
 t/t5531-deep-submodule-push.sh | 21 +
 t/t7112-reset-submodule.sh |  4 +--
 t/t7814-grep-recurse-submodules.sh | 18 +++
 16 files changed, 178 insertions(+), 97 deletions(-)

-- 
2.13.0.17.g582985b1e4

[PATCH 8/8] builtin/fetch.c: respect 'submodule.recurse' option

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/fetch.c |  7 +++
 t/t5526-fetch-submodules.sh | 10 ++
 2 files changed, 17 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5f2c2ab23e..c1ec3b03c3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -73,6 +73,13 @@ static int git_fetch_config(const char *k, const char *v, 
void *cb)
fetch_prune_config = git_config_bool(k, v);
return 0;
}
+
+   if (!strcmp(k, "submodule.recurse")) {
+   int r = git_config_bool(k, v) ?
+   RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
+   recurse_submodules = r;
+   }
+
return git_default_config(k, v, cb);
 }
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index f3b0a8d30a..162baf101f 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -71,6 +71,16 @@ test_expect_success "fetch --recurse-submodules recurses 
into submodules" '
test_i18ncmp expect.err actual.err
 '
 
+test_expect_success "submodule.recurse option triggers recursive fetch" '
+   add_upstream_commit &&
+   (
+   cd downstream &&
+   git -c submodule.recurse fetch >../actual.out 2>../actual.err
+   ) &&
+   test_must_be_empty actual.out &&
+   test_i18ncmp expect.err actual.err
+'
+
 test_expect_success "fetch --recurse-submodules -j2 has the same output 
behaviour" '
add_upstream_commit &&
(
-- 
2.13.0.17.g582985b1e4

[PATCH 6/6] verify_filename(): flip order of checks

[Jeff King]

The looks_like_pathspec() check is much cheaper than
check_filename(), which actually stats the file. Since
either is sufficient for our return value, we should do the
cheaper one first, potentially short-circuiting the other.

Signed-off-by: Jeff King 
---
Probably doesn't matter, but it bugged me and it was subtle enough that
I pulled it out into its own commit.

 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 89fcc12ab..1de87ed84 100644
--- a/setup.c
+++ b/setup.c
@@ -229,7 +229,7 @@ void verify_filename(const char *prefix,
 {
if (*arg == '-')
die("bad flag '%s' used after filename", arg);
-   if (check_filename(prefix, arg) || looks_like_pathspec(arg))
+   if (looks_like_pathspec(arg) || check_filename(prefix, arg))
return;
die_verify_filename(prefix, arg, diagnose_misspelt_rev);
 }
-- 
2.13.0.496.ge44ba89db

[PATCH 1/8] submodule recursing: do not write a config variable twice

[Stefan Beller]

The command line option for '--recurse-submodules' is implemented
using an OPTION_CALLBACK, which takes both the callback (that sets
the file static global variable) as well as passes the same file
static global variable to the option parsing machinery to assign it.
This is fixed in this commit by passing NULL as the variable. The
callback sets it instead

Signed-off-by: Stefan Beller 
---
 builtin/checkout.c  | 2 +-
 builtin/read-tree.c | 2 +-
 builtin/reset.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f33..0fd57672cc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1181,7 +1181,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
N_("second guess 'git checkout 
'")),
OPT_BOOL(0, "ignore-other-worktrees", 
_other_worktrees,
 N_("do not check if another worktree is holding the 
given ref")),
-   { OPTION_CALLBACK, 0, "recurse-submodules", _submodules,
+   { OPTION_CALLBACK, 0, "recurse-submodules", NULL,
"checkout", "control recursive updating of 
submodules",
PARSE_OPT_OPTARG, option_parse_recurse_submodules },
OPT_BOOL(0, "progress", _progress, N_("force progress 
reporting")),
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 23e212ee8c..2a1b8a530e 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -157,7 +157,7 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
 N_("skip applying sparse checkout filter")),
OPT_BOOL(0, "debug-unpack", _unpack,
 N_("debug unpack-trees")),
-   { OPTION_CALLBACK, 0, "recurse-submodules", _submodules,
+   { OPTION_CALLBACK, 0, "recurse-submodules", NULL,
"checkout", "control recursive updating of 
submodules",
PARSE_OPT_OPTARG, option_parse_recurse_submodules },
OPT_END()
diff --git a/builtin/reset.c b/builtin/reset.c
index 5ce27fcaed..1e5f85b1fb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -304,7 +304,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
N_("reset HEAD, index and working tree"), 
MERGE),
OPT_SET_INT(0, "keep", _type,
N_("reset HEAD but keep local changes"), KEEP),
-   { OPTION_CALLBACK, 0, "recurse-submodules", _submodules,
+   { OPTION_CALLBACK, 0, "recurse-submodules", NULL,
"reset", "control recursive updating of submodules",
PARSE_OPT_OPTARG, option_parse_recurse_submodules },
OPT_BOOL('p', "patch", _mode, N_("select hunks 
interactively")),
-- 
2.13.0.17.g582985b1e4

[PATCH 4/8] submodule loading: separate code path for .gitmodules and config overlay

[Stefan Beller]

The .gitmodules file is not supposed to have all the options available,
that are available in the configuration so separate it out.

A configuration option such as the hypothetical submodule.color.diff
that determines in which color a submodule change is printed,
is a very user specific thing, that the .gitmodules file should
not tamper with.

The .gitmodules file should only be used for settings that required
to setup the project in which the .gitmodules file is tracked. As the
minimum this would only include the name<->path mapping of the
submodule and its URL and branch.

Any further setting (such as 'fetch.recursesubmodules' or
'submodule..{update, ignore, shallow}') is not specific
to the project setup requirements, but rather is a distribution
of suggested developer configurations.  In other areas of Git
a suggested developer configuration is not transported in-tree
but via other means.  In an organisation this could be done
by deploying an opinionated system wide config (/etc/gitconfig)
or by putting the settings in the users home directory when
they start at the organisation. In open source projects this
is often accomplished via extensive READMEs (cf. our
SubmittingPatches/CodingGuidlines).

As a later patch in this series wants to introduce
a generic submodule recursion option, we want to make
sure that switch is not exposed via the gitmodules file.

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

diff --git a/submodule.c b/submodule.c
index c9e764b519..78cccb7563 100644
--- a/submodule.c
+++ b/submodule.c
@@ -153,7 +153,8 @@ void set_diffopt_flags_from_submodule_config(struct 
diff_options *diffopt,
}
 }
 
-int submodule_config(const char *var, const char *value, void *cb)
+/* For loading from the .gitmodules file. */
+static int git_modules_config(const char *var, const char *value, void *cb)
 {
if (!strcmp(var, "submodule.fetchjobs")) {
parallel_jobs = git_config_int(var, value);
@@ -169,6 +170,12 @@ int submodule_config(const char *var, const char *value, 
void *cb)
return 0;
 }
 
+/* Loads all submodule settings from the config */
+int submodule_config(const char *var, const char *value, void *cb)
+{
+   return git_modules_config(var, value, cb);
+}
+
 int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 const char *arg, int unset)
 {
@@ -222,7 +229,8 @@ void gitmodules_config(void)
}
 
if (!gitmodules_is_unmerged)
-   git_config_from_file(submodule_config, 
gitmodules_path.buf, NULL);
+   git_config_from_file(git_modules_config,
+   gitmodules_path.buf, NULL);
strbuf_release(_path);
}
 }
@@ -233,7 +241,7 @@ void gitmodules_config_sha1(const unsigned char 
*commit_sha1)
unsigned char sha1[20];
 
if (gitmodule_sha1_from_commit(commit_sha1, sha1, )) {
-   git_config_from_blob_sha1(submodule_config, rev.buf,
+   git_config_from_blob_sha1(git_modules_config, rev.buf,
  sha1, NULL);
}
strbuf_release();
-- 
2.13.0.17.g582985b1e4

[PATCH 4/6] check_filename(): handle ":^" path magic

[Jeff King]

We special-case "git log :/foo" to work when "foo" exists in
the working tree. But :^ (and its alias :!) do not get the
same treatment, requiring the user to supply a
disambiguating "--". Let's make them work without requiring
the user to type the "--".

Signed-off-by: Jeff King 
---
 setup.c   |  4 
 t/t4208-log-magic-pathspec.sh | 13 +
 2 files changed, 17 insertions(+)

diff --git a/setup.c b/setup.c
index f2a8070bd..86bb7c9a9 100644
--- a/setup.c
+++ b/setup.c
@@ -141,6 +141,10 @@ int check_filename(const char *prefix, const char *arg)
if (!*arg) /* ":/" is root dir, always exists */
return 1;
prefix = NULL;
+   } else if (skip_prefix(arg, ":!", ) ||
+  skip_prefix(arg, ":^", )) {
+   if (!*arg) /* excluding everything is silly, but allowed */
+   return 1;
}
 
if (prefix)
diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
index 70bc64100..67250ebdc 100755
--- a/t/t4208-log-magic-pathspec.sh
+++ b/t/t4208-log-magic-pathspec.sh
@@ -52,6 +52,19 @@ test_expect_success 'git log HEAD -- :/' '
test_cmp expected actual
 '
 
+test_expect_success '"git log :^sub" is not ambiguous' '
+   git log :^sub
+'
+
+test_expect_success '"git log :^does-not-exist" does not match anything' '
+   test_must_fail git log :^does-not-exist
+'
+
+test_expect_success  '"git log :!" behaves the same as :^' '
+   git log :!sub &&
+   test_must_fail git log :!does-not-exist
+'
+
 test_expect_success 'command line pathspec parsing for "git log"' '
git reset --hard &&
>a &&
-- 
2.13.0.496.ge44ba89db

[PATCH 2/6] check_filename(): refactor ":/" handling

[Jeff King]

We handle arguments with the ":/" pathspec magic specially,
making sure the name exists at the top-level.  We'll want to
handle more pathspec magic in future patches, so let's do a
little rearranging to make that easier.

Instead of relying on an if/else cascade to avoid the
prefix_filename() call, we'll just set prefix to NULL.
Likewise, we'll get rid of the "name" variable entirely, and
just push the "arg" pointer forward to skip past the magic.
That means by the time we get to the prefix-handling, we're
set up appropriately whether we saw ":/" or not.

Note that this does impact the final error message we
produce when stat() fails, as it shows "arg" (which we'll
have modified to skip magic and include the prefix). This is
a good thing; the original message would say something like
"failed to stat ':/foo'", which is confusing (we tried to
stat "foo").

Signed-off-by: Jeff King 
---
 setup.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/setup.c b/setup.c
index 0309c2782..000ffa810 100644
--- a/setup.c
+++ b/setup.c
@@ -134,19 +134,20 @@ int path_inside_repo(const char *prefix, const char *path)
 
 int check_filename(const char *prefix, const char *arg)
 {
-   const char *name;
char *to_free = NULL;
struct stat st;
 
if (starts_with(arg, ":/")) {
if (arg[2] == '\0') /* ":/" is root dir, always exists */
return 1;
-   name = arg + 2;
-   } else if (prefix)
-   name = to_free = prefix_filename(prefix, arg);
-   else
-   name = arg;
-   if (!lstat(name, )) {
+   arg += 2;
+   prefix = NULL;
+   }
+
+   if (prefix)
+   arg = to_free = prefix_filename(prefix, arg);
+
+   if (!lstat(arg, )) {
free(to_free);
return 1; /* file exists */
}
-- 
2.13.0.496.ge44ba89db

[PATCH 3/6] check_filename(): use skip_prefix

[Jeff King]

This avoids some magic numbers (and we'll be adding more
similar calls in a minute).

Signed-off-by: Jeff King 
---
 setup.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/setup.c b/setup.c
index 000ffa810..f2a8070bd 100644
--- a/setup.c
+++ b/setup.c
@@ -137,10 +137,9 @@ int check_filename(const char *prefix, const char *arg)
char *to_free = NULL;
struct stat st;
 
-   if (starts_with(arg, ":/")) {
-   if (arg[2] == '\0') /* ":/" is root dir, always exists */
+   if (skip_prefix(arg, ":/", )) {
+   if (!*arg) /* ":/" is root dir, always exists */
return 1;
-   arg += 2;
prefix = NULL;
}
 
-- 
2.13.0.496.ge44ba89db

[PATCH 1/6] t4208: add check for ":/" without matching file

[Jeff King]

The DWIM magic in check_filename() doesn't just recognize
":/". It actually makes sure that the file it points to
exists. t4208 checks only the case where the path is
present, not the opposite. Since the next patches will be
touching this area, let's add a test to make sure it
continues working.

Signed-off-by: Jeff King 
---
 t/t4208-log-magic-pathspec.sh | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
index 001343e2f..70bc64100 100755
--- a/t/t4208-log-magic-pathspec.sh
+++ b/t/t4208-log-magic-pathspec.sh
@@ -29,6 +29,12 @@ test_expect_success '"git log -- :/a" should not be 
ambiguous' '
git log -- :/a
 '
 
+# This differs from the ":/a" check above in that :/in looks like a pathspec,
+# but doesn't match an actual file.
+test_expect_success '"git log :/in" should not be ambiguous' '
+   git log :/in
+'
+
 test_expect_success '"git log :" should be ambiguous' '
test_must_fail git log : 2>error &&
test_i18ngrep ambiguous error
-- 
2.13.0.496.ge44ba89db

[PATCH 0/6] make "^:foo" work without disambiguating "--"

[Jeff King]

On Fri, May 26, 2017 at 09:24:32AM -0400, Jeff King wrote:

> The middle ground would be to replicate a simple subset of the rules in
> verify_filename(). If we assume that all arguments with ":(" are
> pathspecs (which seem rather unlikely to have false positives), then
> that leaves only a few short-magic patterns: :/, :!, and :^.
> 
> We already specially handle :/ here. So it would really just be adding
> the other two (which are just aliases of each other). It's not that much
> code. The dirty thing is just that we're replicating some of the
> pathspec logic, and any new magic would have to be updated here, too.
> 
> I'll see if I can work up a patch.

So here's what I came up with. TBH, I could live without patch 5. What I
care most about is making the ":^" work. But I don't really see a
downside to it.

  [1/6]: t4208: add check for ":/" without matching file
  [2/6]: check_filename(): refactor ":/" handling
  [3/6]: check_filename(): use skip_prefix
  [4/6]: check_filename(): handle ":^" path magic
  [5/6]: verify_filename(): treat ":(magic)" as a pathspec
  [6/6]: verify_filename(): flip order of checks

 setup.c   | 42 --
 t/t4208-log-magic-pathspec.sh | 32 
 2 files changed, 64 insertions(+), 10 deletions(-)

-Peff

Re: [PATCH] doc: filter-branch does not require re-export of vars

[Jeff King]

On Fri, May 26, 2017 at 07:36:54PM +0200, Andreas Heiduk wrote:

> The function `set_ident` in `filter-branch` exported the variables
> GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007.
> Therefore the filter scripts don't need to re-eport them again.

Some old shells keep separate values for the internal and exporter
versions of variables. I.e., this:

  foo=one
  export foo
  foo=two

would continue to export $foo as "one", even though it is "two" inside
the script.

However, I think POSIX mandates the behavior you'd expect. And the only
shell I know that misbehaves in this way is Solaris /bin/sh, which we
have already declared too broken to support. According to

  
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Limitations-of-Builtins.html#export

it sounds like there are some other antique shells which may do the
same (it doesn't cover this case explicitly, but the "coexist" cases it
mentions are likely to behave in this way).

At this point, I'd be inclined to remove the text as you suggest and
either make a small note at the bottom of the page, or just omit it
entirely and assume that anybody on an old non-POSIX shell can fend for
themselves.

-Peff

git-2.13.0: log --date=format:%z not working

[Ulrich Mueller]

The following commands work as expected (using commit b06d364310
in the git://git.kernel.org/pub/scm/git/git.git repo as test case):

$ export TZ=Europe/Berlin
$ git --no-pager log -1 --pretty="%ad" --date=iso b06d364310
2017-05-09 23:26:02 +0900
$ git --no-pager log -1 --pretty="%ad" --date=iso-local b06d364310
2017-05-09 16:26:02 +0200

However, if I use explicit format: then the output of the time zone is
wrong:

$ git --no-pager log -1 --pretty="%ad" --date="format:%F %T %z" b06d364310
2017-05-09 23:26:02 +
$ git --no-pager log -1 --pretty="%ad" --date="format-local:%F %T %z" b06d364310
2017-05-09 16:26:02 +

I would expect the output to be the same as in the first two examples.

Re: [PATCH v4 03/10] rebase -i: remove useless indentation

[Stefan Beller]

On Thu, May 25, 2017 at 8:15 PM, Liam Beguin  wrote:
> Hi Johannes,
>
> Johannes Schindelin  writes:
>> The commands used to be indented, and it is nice to look at, but when we
>> transform the SHA-1s, the indentation is removed. So let's do away with it.
>>
>> For the moment, at least: when we will use the upcoming rebase--helper
>> to transform the SHA-1s, we *will* keep the indentation and can
>> reintroduce it. Yet, to be able to validate the rebase--helper against
>> the output of the current shell script version, we need to remove the
>> extra indentation.
>>
>> Signed-off-by: Johannes Schindelin 
>> ---
>>  git-rebase--interactive.sh | 14 +++---
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 609e150d38f..c40b1fd1d2e 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -155,13 +155,13 @@ reschedule_last_action () {
>>  append_todo_help () {
>>   gettext "
>>  Commands:
>> - p, pick = use commit
>> - r, reword = use commit, but edit the commit message
>> - e, edit = use commit, but stop for amending
>> - s, squash = use commit, but meld into previous commit
>> - f, fixup = like \"squash\", but discard this commit's log message
>> - x, exec = run command (the rest of the line) using shell
>> - d, drop = remove commit
>> +p, pick = use commit
>> +r, reword = use commit, but edit the commit message
>> +e, edit = use commit, but stop for amending
>> +s, squash = use commit, but meld into previous commit
>> +f, fixup = like \"squash\", but discard this commit's log message
>> +x, exec = run command (the rest of the line) using shell
>> +d, drop = remove commit
>
> do we also need to update all the translations since this is a `gettext`
> function?

Translations are handled separately, later before a release is done.
Separation of skills. ;)

As programming is quite complicated and involved we'd rather ask
Johannes to only focus on the code in such a patch here and then later
the translators will focus on getting the translation right. As the translation
tools are sophisticated, they will likely give the previous translation such
that the translators see that there is only a white space change.

But as the commit message hints at a later patch that will reintroduce the
original indentation, maybe the translators won't even see that change?

For more information on how the translations work in the git workflow,
see 951ea7656e (Merge tag 'l10n-2.13.0-rnd2.1' of
git://github.com/git-l10n/git-po, 2017-05-09) or see
https://public-inbox.org/git/canyiybgfdxj4jjtcd3ppxqsdn-twcc8dm8b9ov_3naszwsr...@mail.gmail.com/

[PATCH] doc: filter-branch does not require re-export of vars

[Andreas Heiduk]

The function `set_ident` in `filter-branch` exported the variables
GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007.
Therefore the filter scripts don't need to re-eport them again.

Signed-off-by: Andreas Heiduk 
---
 Documentation/git-filter-branch.txt | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/git-filter-branch.txt 
b/Documentation/git-filter-branch.txt
index 6e4bb0220..7b695dbb7 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -86,8 +86,7 @@ OPTIONS
This filter may be used if you only need to modify the environment
in which the commit will be performed.  Specifically, you might
want to rewrite the author/committer name/email/time environment
-   variables (see linkgit:git-commit-tree[1] for details).  Do not forget
-   to re-export the variables.
+   variables (see linkgit:git-commit-tree[1] for details).
 
 --tree-filter ::
This is the filter for rewriting the tree and its contents.
@@ -340,12 +339,10 @@ git filter-branch --env-filter '
if test "$GIT_AUTHOR_EMAIL" = "root@localhost"
then
GIT_AUTHOR_EMAIL=j...@example.com
-   export GIT_AUTHOR_EMAIL
fi
if test "$GIT_COMMITTER_EMAIL" = "root@localhost"
then
GIT_COMMITTER_EMAIL=j...@example.com
-   export GIT_COMMITTER_EMAIL
fi
 ' -- --all
 
-- 
2.13.0

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Stefan Beller]

On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
 wrote:
>
>
> On 26/05/17 16:17, Prathamesh Chavan wrote:
>> According to the documentation about git-submodule foreach subcommand's
>> $path variable:
>> $path is the name of the submodule directory relative to the superproject
>>
>> But it was observed when the value of the $path value deviates from this
>> for the nested submodules when the  is run from a subdirectory.
>> This patch aims for its correction.
>>
>> Mentored-by: Christian Couder 
>> Mentored-by: Stefan Beller 
>> Signed-off-by: Prathamesh Chavan 
>> ---
>> This series of patch is based on gitster/jk/bug-to-abort for untilizing its
>> BUG() macro.
>>
>> The observation made was as follows:
>> For a project - super containing dir (not a submodule) and a submodule sub
>> which contains another submodule subsub. When we run a command from 
>> super/dir:
>>
>> git submodule foreach "echo \$path-\$sm_path"
>>
>> actual results:
>> Entering '../sub'
>> ../sub-../sub
>> Entering '../sub/subsub'
>> ../subsub-../subsub
>>
>> expected result wrt documentation and current test suite:
>> Entering '../sub'
>> sub-../sub
>> Entering '../sub/subsub'
>> subsub-../sub/subsub
>>
>> This make the value of $path confusing and I also feel it deviates from its
>> documentation:
>> $path is the name of the submodule directory relative to the superproject.
>> Hence, this patch corrects the value assigned to the $path and $sm_path.
>>
>>  git-submodule.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index c0d0e9a4c..ea6f56337 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -344,9 +344,9 @@ cmd_foreach()
>>   prefix="$prefix$sm_path/"
>>   sanitize_submodule_env
>>   cd "$sm_path" &&
>> - sm_path=$(git submodule--helper relative-path 
>> "$sm_path" "$wt_prefix") &&
>>   # we make $path available to scripts ...
>>   path=$sm_path &&
>> + sm_path=$displaypath &&
>>   if test $# -eq 1
>>   then
>>   eval "$1"
>>
>
> Hmm, I'm not sure which documentation you are referring to,

Quite likely our fine manual pages. ;)

   foreach [--recursive] 
   Evaluates an arbitrary shell command in each checked out submodule.
   The command has access to the variables $name, $path, $sha1 and
   $toplevel: $name is the name of the relevant submodule section in
   .gitmodules, $path is the name of the submodule directory relative
   to the superproject, $sha1 is the commit as recorded in the
   superproject, and $toplevel is the absolute path to the top-level
   of the superproject. Any submodules defined in the superproject but
   not checked out are ignored by this command. Unless given --quiet,
   foreach prints the name of each submodule before evaluating the
   command. If --recursive is given, submodules are traversed
   recursively (i.e. the given shell command is evaluated in nested
   submodules as well). A non-zero return from the command in any
   submodule causes the processing to terminate. This can be
   overridden by adding || : to the end of the command.

As $path is documented and $sm_path is not, we should care about
$path first to be correct and either fix the documentation or the implementation
such that we have a consistent world view. :)

> but if
> $path != $sm_path then something is wrong. (unless their definition
> has changed, of course).

I would lean in doing so (changing their definition):

$path (as documented) is the name of the submodule directory
relative to the direct superproject (so in nested submodules you
go up only one level).

$sm_path on the other hand is not documented at all and yields
non-sense results in corner cases.

With this patch it becomes less non-sensey and could be documented as:

$sm_path is the relative path from the current working directory
to the submodule (ignoring relations to the superproject or nesting
of submodules). This documentation also fits into the narrative of
the test in t7407.

Thanks,
Stefan

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Brandon Williams]

On 05/26, Prathamesh Chavan wrote:
> This aims to make git-submodule foreach a builtin. This is the very
> first step taken in this direction. Hence, 'foreach' is ported to
> submodule--helper, and submodule--helper is called from git-submodule.sh.
> The code is split up to have one function to obtain all the list of
> submodules. This function acts as the front-end of git-submodule foreach
> subcommand. It calls the function for_each_submodule_list, which basically
> loops through the list and calls function fn, which in this case is
> runcommand_in_submodule. This third function is a calling function that
> takes care of running the command in that submodule, and recursively
> perform the same when --recursive is flagged.
> 
> The first function module_foreach first parses the options present in
> argv, and then with the help of module_list_compute, generates the list of
> submodules present in the current working tree.
> 
> The second function for_each_submodule_list traverses through the
> list, and calls function fn (which in case of submodule subcommand
> foreach is runcommand_in_submodule) is called for each entry.
> 
> The third function runcommand_in_submodule, generates a submodule struct sub
> for $name, value and then later prepends name=sub->name; and other
> value assignment to the env argv_array structure of a child_process.
> Also the  of submodule-foreach is push to args argv_array
> structure and finally, using run_command the commands are executed
> using a shell.
> 
> The third function also takes care of the recursive flag, by creating
> a separate child_process structure and prepending "--super-prefix 
> displaypath",
> to the args argv_array structure. Other required arguments and the
> input  of submodule-foreach is also appended to this argv_array.
> 
> The commit 1c4fb136db (submodule foreach: skip eval for more than one
> argument, 2013-09-27), which explains that why for the case when argc>1,
> we do not use eval. But since in this patch, we are calling the
> command in a separate shell itself for all values of argc, this case
> is not considered separately.
> 
> Both env variable $path and $sm_path were added since both are used in
> tests in t7407.
> 
> Helped-by: Brandon Williams 
> Mentored-by: Christian Couder 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
> These series of patches passes the complete test suite.
> Its build report is available at:
> https://travis-ci.org/pratham-pc/git/builds
> Branch: submodule-foreach
> Build #71
> 
>  builtin/submodule--helper.c | 148 
> 
>  git-submodule.sh|  39 +---
>  2 files changed, 149 insertions(+), 38 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 566a5b6a6..343b6269c 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -13,6 +13,8 @@
>  #include "refs.h"
>  #include "connect.h"
>  
> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item, 
> void *cb_data);
> +
>  static char *get_default_remote(void)
>  {
>   char *dest = NULL, *ret;
> @@ -219,6 +221,26 @@ static int resolve_relative_url_test(int argc, const 
> char **argv, const char *pr
>   return 0;
>  }
>  
> +static char *get_submodule_displaypath(const char *path, const char *prefix)
> +{
> + const char *super_prefix = get_super_prefix();
> +
> + if (prefix && super_prefix) {
> + BUG("cannot have prefix '%s' and superprefix '%s'",
> + prefix, super_prefix);
> + } else if (prefix) {
> + struct strbuf sb = STRBUF_INIT;
> + char *displaypath;
> + displaypath = xstrdup(relative_path(path, prefix, ));

These can probably go on the same line:
  
  char *displaypath = xstrdup(relative_path(path, prefix, ));

> + strbuf_release();
> + return displaypath;
> + } else if (super_prefix) {
> + return xstrfmt("%s/%s", super_prefix, path);
> + } else {
> + return xstrdup(path);
> + }
> +}
> +
>  struct module_list {
>   const struct cache_entry **entries;
>   int alloc, nr;
> @@ -331,6 +353,14 @@ static int module_list(int argc, const char **argv, 
> const char *prefix)
>   return 0;
>  }
>  
> +static void for_each_submodule_list(const struct module_list list,
> + submodule_list_func_t fn, void *cb_data)
> +{
> + int i;
> + for (i = 0; i < list.nr; i++)
> + fn(list.entries[i], cb_data);
> +}
> +
>  static void init_submodule(const char *path, const char *prefix, int quiet)
>  {
>   const struct submodule *sub;
> @@ -487,6 +517,123 @@ static int module_name(int argc, const char **argv, 
> const char *prefix)
>   return 0;
>  }
>  
> +struct cb_foreach {
> + int argc;
> + const char **argv;
> + 

Re: [GSoC][PATCH v5 2/3] t7407: test "submodule foreach --recursive" from subdirectory added

[Brandon Williams]

On 05/26, Prathamesh Chavan wrote:
> Additional test cases added to the submodule-foreach test suite
> to check the submodule foreach --recursive behavior from a
> subdirectory as this was missing from the test suite.
> 
> Helped-by: Brandon Williams 
> Mentored-by: Christian Couder 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
> Additional test added to check the bug fixed in the [PATCH v5 1/3] of
> this patch series.
> 
>  t/t7407-submodule-foreach.sh | 34 ++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 6ba5daf42..1c8d132d8 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -197,6 +197,40 @@ test_expect_success 'test messages from "foreach 
> --recursive" from subdirectory'
>   test_i18ncmp expect actual
>  '
>  
> +sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)
> +sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)
> +sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)
> +nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)
> +nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)
> +nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)
> +submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse 
> HEAD)
> +
> +cat >expect < +Entering '../nested1'
> +$pwd/clone2-nested1-../nested1-$nested1sha1
> +Entering '../nested1/nested2'
> +$pwd/clone2/nested1-nested2-../nested1/nested2-$nested2sha1
> +Entering '../nested1/nested2/nested3'
> +$pwd/clone2/nested1/nested2-nested3-../nested1/nested2/nested3-$nested3sha1
> +Entering '../nested1/nested2/nested3/submodule'
> +$pwd/clone2/nested1/nested2/nested3-submodule-../nested1/nested2/nested3/submodule-$submodulesha1
> +Entering '../sub1'
> +$pwd/clone2-foo1-../sub1-$sub1sha1
> +Entering '../sub2'
> +$pwd/clone2-foo2-../sub2-$sub2sha1
> +Entering '../sub3'
> +$pwd/clone2-foo3-../sub3-$sub3sha1
> +EOF
> +
> +test_expect_success 'test "submodule foreach --recursive" from subdirectory' 
> '
> + (
> + cd clone2 &&
> + cd untracked &&
> + git submodule foreach --recursive "echo 
> \$toplevel-\$name-\$sm_path-\$sha1" >../../actual
> + ) &&

small nit: You can either merge the two cd commands to 'cd clone2/untracked' or
better you can even avoid the subshell entirely by doing the following:

  git -C clone2/untracked submodule foreach --recursive \
"echo \$toplevel-\$name-\$sm_path-\$sha1" >actual

Or something akin to that.

> + test_i18ncmp expect actual
> +'
> +
>  cat > expect <  nested1-nested1
>  nested2-nested2
> -- 
> 2.11.0
> 

-- 
Brandon Williams

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 26/05/17 16:17, Prathamesh Chavan wrote:
> According to the documentation about git-submodule foreach subcommand's
> $path variable:
> $path is the name of the submodule directory relative to the superproject
> 
> But it was observed when the value of the $path value deviates from this
> for the nested submodules when the  is run from a subdirectory.
> This patch aims for its correction.
> 
> Mentored-by: Christian Couder 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
> This series of patch is based on gitster/jk/bug-to-abort for untilizing its 
> BUG() macro.
> 
> The observation made was as follows:
> For a project - super containing dir (not a submodule) and a submodule sub 
> which contains another submodule subsub. When we run a command from super/dir:
> 
> git submodule foreach "echo \$path-\$sm_path"
> 
> actual results:
> Entering '../sub'
> ../sub-../sub
> Entering '../sub/subsub'
> ../subsub-../subsub
> 
> expected result wrt documentation and current test suite:
> Entering '../sub'
> sub-../sub
> Entering '../sub/subsub'
> subsub-../sub/subsub
> 
> This make the value of $path confusing and I also feel it deviates from its 
> documentation:
> $path is the name of the submodule directory relative to the superproject.
> Hence, this patch corrects the value assigned to the $path and $sm_path.
> 
>  git-submodule.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index c0d0e9a4c..ea6f56337 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -344,9 +344,9 @@ cmd_foreach()
>   prefix="$prefix$sm_path/"
>   sanitize_submodule_env
>   cd "$sm_path" &&
> - sm_path=$(git submodule--helper relative-path 
> "$sm_path" "$wt_prefix") &&
>   # we make $path available to scripts ...
>   path=$sm_path &&
> + sm_path=$displaypath &&
>   if test $# -eq 1
>   then
>   eval "$1"
> 

Hmm, I'm not sure which documentation you are referring to, but if
$path != $sm_path then something is wrong. (unless their definition
has changed, of course). commit 091a6eb0fe may have muddied the water
a little by using $sm_path in the test in t7407, since (as far as I
know) $path is the user-facing variable (NOT $sm_path).

ATB,
Ramsay Jones

Re: persistent-https, url insteadof, and `git submodule`

[Elliott Cable]

Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list
etiquette? Feel free to yell at with a direct reply!). For whatever it's
worth, as a random user, here's my thoughts:

On Sat, May 20, 2017 at 2:07 AM, Jeff King  wrote:
> On Fri, May 19, 2017 at 11:55:34PM +0200, Dennis Kaarsemaker wrote:
>> > On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:
>> > > Presumably this isn't intended behaviour?
>> >
>> > It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which
>> > makes git not trust any urls except http(s), git, ssh and file urls
>> > unless you explicitely configure git to allow it. See the
>> > GIT_ALLOW_PROTOCOL section in man git and the git-config section it
>> > links to.
>>
>> 33cfccbbf3 (submodule: allow only certain protocols for submodule
>> fetches, 2015-09-16) says:
>> [...]
>> But doing it this way is
>> simpler, and makes it much less likely that we would miss a
>> case. And since such protocols should be an exception
>> (especially because nobody who clones from them will be able
>> to update the submodules!), it's not likely to inconvenience
>> anyone in practice.
>
> The other approach is to declare that a url rewrite resets the
> protocol-from-user flag to 1. IOW, since the "persistent-https" protocol
> comes from our local config, it's not dangerous and we should behave as
> if the user themselves gave it to us. That makes Elliott's case work out
> of the box.

Well, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER`
and `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect
`insteadOf` to disable that behaviour. Instead, one of two things seems
like a more ideal solution:

1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`
   explicitly in the documentation of/near `insteadOf`, most
   particularly in the README for `contrib/persistent-https`.

2. Possibly, special-case “higher-security” porcelain (like
   `git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`
   rewrite-rules without additional, special configuration. This way,
   `git-submodule` works for ignorant users (like me) out of the box,
   just as it previously did, and there's no possible security
   compramise.

Just my 2¢ — thanks for your tireless contributions, loves. <3


⁓ ELLIOTTCABLE — fly safe.
  http://ell.io/tt

Re: [GSoC][PATCH v5 2/3] t7407: test "submodule foreach --recursive" from subdirectory added

[Stefan Beller]

On Fri, May 26, 2017 at 8:17 AM, Prathamesh Chavan  wrote:
> Additional test cases added to the submodule-foreach test suite
> to check the submodule foreach --recursive behavior from a
> subdirectory as this was missing from the test suite.

As this demonstrates the fixture of the first patch,
this could be squashed into the first commit.

Reason: When someone is looking at that first commit, they
may wonder if there is no test that demonstrates the fix. (as fixing
a bug with no test is bad style. ;) And given the data structures of
Git it is only easy to find the previous commit, but hard to find the
next commit (this one) later on.

I think with only the minor nit in patch 3, the foreach is tackled. :)

Thanks,
Stefan

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Stefan Beller]

On Fri, May 26, 2017 at 8:17 AM, Prathamesh Chavan  wrote:
> This aims to make git-submodule foreach a builtin.

Cool. I reviewed the code and only have one minor nit.

> +static void runcommand_in_submodule(const struct cache_entry *list_item,
> +   void *cb_data)
> +{

> +   /* Only loads from .gitmodules, no overlay with .git/config */
> +   gitmodules_config();

Performance nit: We only need to load the gitmodules file once instead
of foreach submodule separately, so we could move this to module_foreach().

Thanks,
Stefan

Re: [PATCH v2 6/6] fsmonitor: add a sample query-fsmonitor hook script for Watchman

[Ben Peart]

On 5/26/2017 5:47 AM, Ævar Arnfjörð Bjarmason wrote:

On Wed, May 24, 2017 at 3:12 PM, Christian Couder
 wrote:

On Thu, May 18, 2017 at 10:13 PM, Ben Peart  wrote:

This hook script integrates the new fsmonitor capabilities of git with
the cross platform Watchman file watching service. To use the script:

Download and install Watchman from https://facebook.github.io/watchman/
and instruct Watchman to watch your working directory for changes
('watchman watch-project /usr/src/git').

Rename the sample integration hook from query-fsmonitor.sample to
query-fsmonitor.

Configure git to use the extension ('git config core.fsmonitor true')
and optionally turn on the untracked cache for optimal performance
('git config core.untrackedcache true').


Ok, it looks like the untracked cache can be used, but it could work without it.


Signed-off-by: Ben Peart 
Signed-off-by: Johannes Schindelin 
---
  templates/hooks--query-fsmonitor.sample | 27 +++
  1 file changed, 27 insertions(+)
  create mode 100644 templates/hooks--query-fsmonitor.sample

diff --git a/templates/hooks--query-fsmonitor.sample 
b/templates/hooks--query-fsmonitor.sample
new file mode 100644
index 00..4bd22f21d8
--- /dev/null
+++ b/templates/hooks--query-fsmonitor.sample
@@ -0,0 +1,27 @@
+#!/bin/sh
+#
+# An example hook script to integrate Watchman
+# (https://facebook.github.io/watchman/) with git to provide fast
+# git status.
+#
+# The hook is passed a time_t formatted as a string and outputs to
+# stdout all files that have been modified since the given time.
+# Paths must be relative to the root of the working tree and
+# separated by a single NUL.
+#
+# To enable this hook, rename this file to "query-fsmonitor"
+
+# Convert unix style paths to escaped Windows style paths
+case "$(uname -s)" in
+MINGW*|MSYS_NT*)
+  GIT_WORK_TREE="$(cygpath -aw "$PWD" | sed 's,\\,,g')"
+  ;;
+*)
+  GIT_WORK_TREE="$PWD"
+  ;;
+esac
+
+# Query Watchman for all the changes since the requested time
+echo "[\"query\", \"$GIT_WORK_TREE\", {\"since\": $1, \"fields\":[\"name\"]}]" 
| \
+watchman -j | \
+perl -e 'use JSON::PP; my $o = JSON::PP->new->utf8->decode(join("", <>)); die "Watchman: $o->{'error'}.\nFalling back 
to scanning...\n" if defined($o->{"error"}); print(join("\0", @{$o->{"files"}}));'


Maybe put the above small perl script on multiple lines for improved
readability.



I'll pick up the suggestions from Ævar on the perl script.  I believe 
that fixes all the issues you have raised.


I've also tested the various error cases of watchman not installed and 
when watchman returns an error and they are all properly handled by 1) 
giving an error message and 2) falling back to the git codepath without 
fsmonitor so that the git command succeeds.



Is JSON::PP always available in Perl >= 5.8?


No, it's shipped with perl as of 5.14.0, which came out in May 2011.

I think depending on that is fine. FWIW we bumped the required core
dependency (but you might still need to install modules) in 2010 in my
d48b284183 ("perl: bump the required Perl version to 5.8 from
5.6.[21]", 2010-09-24). Maybe we should be bumping that again...


What happens if watchman is not installed or not in the PATH?
It seems to me that the git process will not die, and will just work
as if the hook does not exist, except that it will call the hook which
will probably output error messages.


I think a good solution to this is to just add "set -euo pipefail" to
that script.

Then we'll print out on stderr that we couldn't find the watchman
command. Right now (with my patch) it'll make its way to the perl
program and get empty input.



With or without "set -euo pipefail" the output if watchman is not 
installed is:


.git/hooks/query-fsmonitor: line 37: watchman: command not found
Watchman: command returned no output.
Falling back to scanning...
On branch fsmonitor
Your branch is up-to-date with 'benpeart/fsmonitor'.


To try this out on Mac, you can install the package maintained by 
MacPorts by running "sudo port install watchman"


https://facebook.github.io/watchman/docs/install.html

[GSoC][PATCH v5 2/3] t7407: test "submodule foreach --recursive" from subdirectory added

[Prathamesh Chavan]

Additional test cases added to the submodule-foreach test suite
to check the submodule foreach --recursive behavior from a
subdirectory as this was missing from the test suite.

Helped-by: Brandon Williams 
Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
Additional test added to check the bug fixed in the [PATCH v5 1/3] of
this patch series.

 t/t7407-submodule-foreach.sh | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42..1c8d132d8 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -197,6 +197,40 @@ test_expect_success 'test messages from "foreach 
--recursive" from subdirectory'
test_i18ncmp expect actual
 '
 
+sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)
+sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)
+sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)
+nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)
+nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)
+nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)
+submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse 
HEAD)
+
+cat >expect <../../actual
+   ) &&
+   test_i18ncmp expect actual
+'
+
 cat > expect <

[GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Prathamesh Chavan]

This aims to make git-submodule foreach a builtin. This is the very
first step taken in this direction. Hence, 'foreach' is ported to
submodule--helper, and submodule--helper is called from git-submodule.sh.
The code is split up to have one function to obtain all the list of
submodules. This function acts as the front-end of git-submodule foreach
subcommand. It calls the function for_each_submodule_list, which basically
loops through the list and calls function fn, which in this case is
runcommand_in_submodule. This third function is a calling function that
takes care of running the command in that submodule, and recursively
perform the same when --recursive is flagged.

The first function module_foreach first parses the options present in
argv, and then with the help of module_list_compute, generates the list of
submodules present in the current working tree.

The second function for_each_submodule_list traverses through the
list, and calls function fn (which in case of submodule subcommand
foreach is runcommand_in_submodule) is called for each entry.

The third function runcommand_in_submodule, generates a submodule struct sub
for $name, value and then later prepends name=sub->name; and other
value assignment to the env argv_array structure of a child_process.
Also the  of submodule-foreach is push to args argv_array
structure and finally, using run_command the commands are executed
using a shell.

The third function also takes care of the recursive flag, by creating
a separate child_process structure and prepending "--super-prefix displaypath",
to the args argv_array structure. Other required arguments and the
input  of submodule-foreach is also appended to this argv_array.

The commit 1c4fb136db (submodule foreach: skip eval for more than one
argument, 2013-09-27), which explains that why for the case when argc>1,
we do not use eval. But since in this patch, we are calling the
command in a separate shell itself for all values of argc, this case
is not considered separately.

Both env variable $path and $sm_path were added since both are used in
tests in t7407.

Helped-by: Brandon Williams 
Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
These series of patches passes the complete test suite.
Its build report is available at:
https://travis-ci.org/pratham-pc/git/builds
Branch: submodule-foreach
Build #71

 builtin/submodule--helper.c | 148 
 git-submodule.sh|  39 +---
 2 files changed, 149 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..343b6269c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -13,6 +13,8 @@
 #include "refs.h"
 #include "connect.h"
 
+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item, 
void *cb_data);
+
 static char *get_default_remote(void)
 {
char *dest = NULL, *ret;
@@ -219,6 +221,26 @@ static int resolve_relative_url_test(int argc, const char 
**argv, const char *pr
return 0;
 }
 
+static char *get_submodule_displaypath(const char *path, const char *prefix)
+{
+   const char *super_prefix = get_super_prefix();
+
+   if (prefix && super_prefix) {
+   BUG("cannot have prefix '%s' and superprefix '%s'",
+   prefix, super_prefix);
+   } else if (prefix) {
+   struct strbuf sb = STRBUF_INIT;
+   char *displaypath;
+   displaypath = xstrdup(relative_path(path, prefix, ));
+   strbuf_release();
+   return displaypath;
+   } else if (super_prefix) {
+   return xstrfmt("%s/%s", super_prefix, path);
+   } else {
+   return xstrdup(path);
+   }
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -331,6 +353,14 @@ static int module_list(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+static void for_each_submodule_list(const struct module_list list,
+   submodule_list_func_t fn, void *cb_data)
+{
+   int i;
+   for (i = 0; i < list.nr; i++)
+   fn(list.entries[i], cb_data);
+}
+
 static void init_submodule(const char *path, const char *prefix, int quiet)
 {
const struct submodule *sub;
@@ -487,6 +517,123 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct cb_foreach {
+   int argc;
+   const char **argv;
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int recursive: 1;
+};
+#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }
+
+static void runcommand_in_submodule(const struct cache_entry *list_item,
+   void *cb_data)
+{
+   struct cb_foreach *info = cb_data;
+   char 

[GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Prathamesh Chavan]

According to the documentation about git-submodule foreach subcommand's
$path variable:
$path is the name of the submodule directory relative to the superproject

But it was observed when the value of the $path value deviates from this
for the nested submodules when the  is run from a subdirectory.
This patch aims for its correction.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
This series of patch is based on gitster/jk/bug-to-abort for untilizing its 
BUG() macro.

The observation made was as follows:
For a project - super containing dir (not a submodule) and a submodule sub 
which contains another submodule subsub. When we run a command from super/dir:

git submodule foreach "echo \$path-\$sm_path"

actual results:
Entering '../sub'
../sub-../sub
Entering '../sub/subsub'
../subsub-../subsub

expected result wrt documentation and current test suite:
Entering '../sub'
sub-../sub
Entering '../sub/subsub'
subsub-../sub/subsub

This make the value of $path confusing and I also feel it deviates from its 
documentation:
$path is the name of the submodule directory relative to the superproject.
Hence, this patch corrects the value assigned to the $path and $sm_path.

 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index c0d0e9a4c..ea6f56337 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -344,9 +344,9 @@ cmd_foreach()
prefix="$prefix$sm_path/"
sanitize_submodule_env
cd "$sm_path" &&
-   sm_path=$(git submodule--helper relative-path 
"$sm_path" "$wt_prefix") &&
# we make $path available to scripts ...
path=$sm_path &&
+   sm_path=$displaypath &&
if test $# -eq 1
then
eval "$1"
-- 
2.11.0

Re: [PATCHv3 2/4] Documentation/clone: document ignored configuration variables

[SZEDER Gábor]

On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor  wrote:
> Due to limitations/bugs in the current implementation, some
> configuration variables specified via 'git clone -c var=val' (or 'git
> -c var=val clone') are ignored during the initial fetch and checkout.
>
> Let the users know which configuration variables are known to be
> ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the
> documentation of 'git clone -c'.
>
> Signed-off-by: SZEDER Gábor 
> ---
>  Documentation/git-clone.txt | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index ec41d3d69..4f1e7d4ba 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -186,6 +186,10 @@ objects from the source repository into a pack in the 
> cloned repository.
> values are given for the same key, each value will be written to
> the config file. This makes it safe, for example, to add
> additional fetch refspecs to the origin remote.
> +   Note that due to limitations of the current implementation some
> +   configuration variables don't take effect during the initial
> +   fetch and checkout.  Configuration variables known to not take
> +   effect are: `remote..mirror` and `remote..tagOpt`.

A few notes to this patch, because I didn't like that "known to not
take effect" expression, and looked into how some configuration
variables are handled during the initial fetch and checkout.  Far from
comprehensive, but here they are anyway:

Concerning the initial fetch:

 - Configuration variables influencing the refspecs to be fetched are
   currently ignored.  These are the fetch refspecs, of course, and
   remote..{mirror,tagOpt}.
   This series makes fetch refspecs work.  The other two are mentioned
   in this patch, and are less urgent, because their functionality is
   or will soon be available through command line options (--mirror
   and --no-tags).
 - remote..url is strange.  It's not just ignored, it's ignored
   so much that it isn't even written to the config file when
   specified as 'git clone -c ...'.  Nonetheless, specifying it for
   'git clone' doesn't make much sense in the first place, does it?
   So I think it's actually good that it's ignored and it isn't worth
   mentioning it in the docs.
 - Some fetch-related config variables, e.g.
   remote..{prune,skipDefaultUpdate,skipFetchAll} or
   fetch.{prune,output}, don't matter during the initial fetch.
 - Other fetch-related config variables, e.g.
   fetch.{fsckObjects,unpackLimit}, are handled deep down in
   fetch-pack and work properly.
 - Transport-specific config variables, e.g. url..insteadOf,
   remote..{proxy,uploadpack,vcs}, or http.*, if applicable, are
   handled in the transport layer or remote helper.
 - I'm not sure about submodule-related config variables, but there
   are command line options for that.

I'm not sure about the initial checkout, in particular I'm not sure
how many configuration variables there are that could/should influence
the initial checkout (or any checkout, for that matter).

 - core.autocrlf works properly, we even have a test for that.
 - filter..smudge is read during initial checkout, but I'm not
   sure whether that should do anything, since no attributes files
   exist at that point.
 - I glanced through builtin/checkout.c to see whether it looks at any
   configuration variables that clone doesn't, and while it does so,
   it seems to only look at variables that are either irrelevant
   during the initial checkout, e.g. 'diff.ignoresubmodules' and
   'merge.conflictstyle', or are submodule-specific, and about those I
   have no idea.


Like I said, far from comprehensive, but I think at least the fetch
part is well covered.

Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King]

On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:

> > but given that it lazy-parses in
> > some cases, it feels a little dangerous.
> 
> I'm not sure about lazy parsing.
> remote_get() returns a fully-parsed, cached struct remote instance
> without re-reading the configuration, so all fields directly
> corresponding to configuration variables stay the same.  However, it
> does parse fetch and push refspecs on every invocation.  So if it were
> to be called to return the origin remote more than once during
> cloning, then the default refspec would get lost on subsequent
> invocations.  Is this what you meant with dangerous?

More or less. I actually didn't look far enough to see under what
circumstances we might re-parse (or might not have parsed when we add
our extra refspec), but that's definitely the sort of thing I was
worried about.

> (Sidenote: and it would leak some memory, too, because it re-parses
> the refspecs without free()ing the results of the previous
> invocation.)

Yes, I'd argue that the current code is buggy, since:

  x = remote_get("foo");
  y = remote_get("foo");

is a guaranteed leak. It seems like remote_get_1() should protect the
call to parse_fetch_refspec() by checking whether ret->fetch is NULL
(and ditto for ret->push).

> Your proposed function to add a refspec as a string would eliminate
> this danger.

Yeah, I think it's not that hard to support. I'd just rather have the
logic inside remote.c, rather than infecting the caller with the
complexity.

> It certainly looks better, see the patch below the scissors for
> reference, and I thought it works because until last night I only run
> the corresponding test script (t5611-clone-config), though I know very
> well that "Thou shalt always run the full test suite!" :)
> 
> Unfortunately, putting the default refspec into this temporary
> configuration environment breaks a few submodule tests
> (t5614-clone-submodules or t5614-clone-submodules-shallow (it's got
> renamed between this topic and master), t7407-submodule-foreach,
> t7410-submodule-checkout-to), because it "leaks" to the submodule
> environment.

Doh, of course. I didn't think of that. That's probably a bad direction,
then, as there's no "just for this process" global config.

-Peff

Re: [RFC/PATCH] recognize pathspec magic without "--" disambiguation

[Jeff King]

On Fri, May 26, 2017 at 11:13:55AM +0900, Junio C Hamano wrote:

> >- git log :^foo
> >   (when "^foo" exists in your index)
> >
> >   The same logic applies; it would continue to work. And
> >   ditto for any other weird filenames in your index like
> >   "(attr)foo".
> 
> "git show :$path" where $path happens to be "^foo" would grab the
> contents of the $path out of the index and I think that is what you
> meant, but use of "git log" in the example made me scratch my head
> greatly.

Yeah, sorry about that confusion. My original motivation for this was
for use with git-grep, but since the existing tests all used "git log",
I tried to switch to that for consistency. But index paths obviously
make no sense with "git log" (we _do_ still parse it the same as we
would for git-show, etc, even though git-log doesn't do anything with
it).

As an aside, I wonder if git-log should issue a warning when there are
pending objects that it doesn't handle.

> >- git log :/foo
> >   (when "foo" does _not_ match a commit message)
> > ...
> >   This same downside actually exists currently when you
> >   have an asterisk in your regex. E.g.,
> >
> > git log :/foo.*bar
> >
> >   will be treated as a pathspec (if it doesn't match a
> >   commit message) due to the wildcard matching in
> >   28fcc0b71.
> 
> In other words, we are not making things worse?

We're making them slightly worse. The "problem" used to trigger with
":/foo.*bar", and now it would trigger with ":/foobar", too. I don't
know if that's a meaningful distinction, though.

> Unlike "git log builtin-checkout.c" that errors out (only because
> there is no such file in the checkout of the current version) and
> makes its solution obvious to the users, this change has the risk of
> silently accepting an ambiguous input and computing result that is
> different from what the user intended to.  So I am not sure.  

Right, that's what I was trying to catalogue in the commit message.

> As you pointedout, ":/" especially does look like a likely point of
> failure, in that both "this is path at the top" pathspec magic and
> "the commit with this string" are not something that we can say with
> confidence that are rarely used because they are so esoteric.
> 
> As to "is it OK to build a rule that we cannot explain easily?", I
> think it is OK to say "if it is not a rev, and if it is not a
> pathname in the current working tree, you must disambiguate, but Git
> helps by guessing in some cases---if you want to have more control
> (e.g. you are a script), explicitly disambiguate and you'd be OK",
> and leave the "some cases" vague, as long as we are only making
> reasonably conservative guesses.

I don't know. That is punting on the issue by not making any promises.
But that's a small consolation to somebody who does:

  $ git log :^foo
  [ok, works great...]
  $ git log :/foo
  fatal: ambiguous argument ':/foo': unknown revision or path not in the 
working tree.
  [stupid git! why don't you work?]

An explanation like "it's complicated, and the rules are meant to be
conservative and avoid confusion" does not really help Git's reputation
for being arcane.

I dunno. The one saving grace of ":/" is that we actually do handle
the ":/foo" case completely in check_filename(). So going back to my
:/foobar example: if you have a file "foobar" in your root directory, it
_is_ considered ambiguous.

So if that was the one exception, it would probably be OK in practice.

Which again makes me feel a bit like the right solution is doing the
existence checks on the non-magic portion of the pathspec for more magic
besides ":/".

Unfortunately, since writing my last message I did look into asking the
pathspec code to parse the arguments for us, but I think it would take
quite a bit of refactoring. It's very ready to die() or emit warnings on
bogus pathspecs, so it's not a good match for this kind of speculative
"is it a pathspec" test.

The middle ground would be to replicate a simple subset of the rules in
verify_filename(). If we assume that all arguments with ":(" are
pathspecs (which seem rather unlikely to have false positives), then
that leaves only a few short-magic patterns: :/, :!, and :^.

We already specially handle :/ here. So it would really just be adding
the other two (which are just aliases of each other). It's not that much
code. The dirty thing is just that we're replicating some of the
pathspec logic, and any new magic would have to be updated here, too.

I'll see if I can work up a patch.

-Peff

GREETINGS,

[mis.sbort...@ono.com]

GREETINGS,

I AM BORTE ,MY DOCTOR JUST CONFIRM THAT I HAVE OVARIAN CANCER AND I 
HAVE FEW WEEKS TO LIVE, SO I HAVE DECIDED TO DONATE EVERYTHING I HAVE 
TO THE ORPHANAGE AND THE POOR WIDOWS THROUGH YOU IN YOUR AREA.PLEASE 
KINDLY REPLY  ME ONLY ON MY  EMAIL ADDRES HERE (missboteogotai@gmail.
com)  AS SOON AS POSIBLE TO ENABLE ME GIVE YOU MORE INFORMATION ON HOW 
TO GO ABOUT IT .

THANKS 

MISS BORTE

Re: .Setting Up Charity Foundation !

[.Sarah Edward]

***.

Peace be with you,

Gladly seek for your sincere help in setting up a Charitable
Organization to help the less privileged, old aged and vulnerable
people under your care,

It is my desire to use my late husband wealth of $3,000,000.00 to set
up a charity foundation to help the needy and the less privileged ones
in your country under your care, Can you help to build this Charity
project in your country?  All i need from you is your sincerity to use
this funds to do this project as i desired to use it for less
privileged ones, orphanage homes and  vulnerable people,

We can make the world a better place when we help one another
sincerely from our good heart,

Let me read from you today and know your opinion in doing this Noble Project,

Best Regard,

Mrs.Sarah Edward J

Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch

[SZEDER Gábor]

On Tue, May 16, 2017 at 1:07 AM, Jeff King  wrote:
>> @@ -989,6 +994,10 @@ int cmd_clone(int argc, const char **argv, const char 
>> *prefix)
>>   strbuf_reset();
>>
>>   remote = remote_get(option_origin);
>> + REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr + 1);
>> + memcpy(remote->fetch+remote->fetch_refspec_nr, refspec,
>> +sizeof(*refspec));
>
> Here we append to remote->fetch. We are assuming then that
> remote->fetch_refspec has already been parsed into remote->fetch. Which
> I think it always is by remote_get(),

Right.

> but given that it lazy-parses in
> some cases, it feels a little dangerous.

I'm not sure about lazy parsing.
remote_get() returns a fully-parsed, cached struct remote instance
without re-reading the configuration, so all fields directly
corresponding to configuration variables stay the same.  However, it
does parse fetch and push refspecs on every invocation.  So if it were
to be called to return the origin remote more than once during
cloning, then the default refspec would get lost on subsequent
invocations.  Is this what you meant with dangerous?

(Sidenote: and it would leak some memory, too, because it re-parses
the refspecs without free()ing the results of the previous
invocation.)

Your proposed function to add a refspec as a string would eliminate
this danger.

> I think in the earlier discussion you mentioned there are some
> ordering
> problems with writing out the new on-disk config. But could we add
> it to
> the temporary environment, like:
>
>   strbuf_addf(, "remote.%s.fetch=%s", option_origin,
>   refspec_pattern);
>   git_config_push_parameter(key.buf);
>
> ?

> If all that's correct, then I think the push_parameter() thing would
> work. It does feel like a round-a-bout way to solve the problem, but
> it's at least manipulating solid, public APIs.

It certainly looks better, see the patch below the scissors for
reference, and I thought it works because until last night I only run
the corresponding test script (t5611-clone-config), though I know very
well that "Thou shalt always run the full test suite!" :)

Unfortunately, putting the default refspec into this temporary
configuration environment breaks a few submodule tests
(t5614-clone-submodules or t5614-clone-submodules-shallow (it's got
renamed between this topic and master), t7407-submodule-foreach,
t7410-submodule-checkout-to), because it "leaks" to the submodule
environment.


 -- >8 --

Subject: [PATCH] clone: respect additional configured fetch refspecs during
 initial fetch

The initial fetch during a clone doesn't transfer refs matching
additional fetch refspecs given on the command line as configuration
variables.  This contradicts the documentation stating that
configuration variables specified via 'git clone -c = ...'
"take effect immediately after the repository is initialized, but
before the remote history is fetched" and the given example
specifically mentions "adding additional fetch refspecs to the origin
remote".  Furthermore, one-shot configuration variables specified via
'git -c = clone ...', though not written to the newly
created repository's config file, live during the lifetime of the
'clone' command, including the initial fetch.  All this implies that
any fetch refspecs specified this way should already be taken into
account during the initial fetch.

The reason for this is that the initial fetch is not a fully fledged
'git fetch' but a bunch of direct calls into the fetch/transport
machinery with clone's own refs-to-refspec matching logic, which
bypasses parts of 'git fetch' processing configured fetch refspecs.
This logic only considers a single default refspec, potentially
influenced by options like '--single-branch' and '--mirror'.  The
configured refspecs are, however, already read and parsed properly
when clone calls remote.c:remote_get(), but it never looks at the
parsed refspecs in the resulting 'struct remote'.

Modify clone to take the configured fetch refspecs into account to
retrieve all matching refs during the initial fetch.  Note that the
configuration at that point only includes the fetch refspecs specified
by the user, but it doesn't include the default fetch refspec.
To keep the code simple and parsing and memory management of the
refspecs in one place, add the default fetch refspec to the temporary
configuration environment, so remote_get() can parse it along with all
other refspecs that might have been specified on the command line.

Add tests to check that refspecs given both via 'git clone -c ...' and
'git -c ... clone' retrieve all refs matching either the default or
the additional refspecs, and that it works even when the user
specifies an alternative remote name via '--origin='.

Signed-off-by: SZEDER Gábor 
---
 builtin/clone.c | 32 
 t/t5611-clone-config.sh | 44 
 2 files changed, 60 insertions(+), 16 

Re: [PATCH v2 1/7] grep: don't redundantly compile throwaway patterns under threading

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> To be clear the point of my mail was not to say "I can't think of a
> way to support both of these things, help!", obviously we can continue
> to maintain two codepaths. The point was to raise the idea that we
> could simply remove the more complex & doomed to forever be slow
> codepath.

To be clear, the point of my response was that these features must
remain.  As long as they are more convenient than sifting through
output produced by pattern matching engine that is less powerful
(which forces the user to give wider pattern than desired, to avoid
false negatives) with eyeball, having to match each pattern one by
one, instead of being able to use a combined and more efficient
single pattern, is still more efficient for the end user, which is
the point of using a computer.

Re: [PATCH v2 6/6] fsmonitor: add a sample query-fsmonitor hook script for Watchman

[Ævar Arnfjörð Bjarmason]

On Wed, May 24, 2017 at 3:12 PM, Christian Couder
 wrote:
> On Thu, May 18, 2017 at 10:13 PM, Ben Peart  wrote:
>> This hook script integrates the new fsmonitor capabilities of git with
>> the cross platform Watchman file watching service. To use the script:
>>
>> Download and install Watchman from https://facebook.github.io/watchman/
>> and instruct Watchman to watch your working directory for changes
>> ('watchman watch-project /usr/src/git').
>>
>> Rename the sample integration hook from query-fsmonitor.sample to
>> query-fsmonitor.
>>
>> Configure git to use the extension ('git config core.fsmonitor true')
>> and optionally turn on the untracked cache for optimal performance
>> ('git config core.untrackedcache true').
>
> Ok, it looks like the untracked cache can be used, but it could work without 
> it.
>
>> Signed-off-by: Ben Peart 
>> Signed-off-by: Johannes Schindelin 
>> ---
>>  templates/hooks--query-fsmonitor.sample | 27 +++
>>  1 file changed, 27 insertions(+)
>>  create mode 100644 templates/hooks--query-fsmonitor.sample
>>
>> diff --git a/templates/hooks--query-fsmonitor.sample 
>> b/templates/hooks--query-fsmonitor.sample
>> new file mode 100644
>> index 00..4bd22f21d8
>> --- /dev/null
>> +++ b/templates/hooks--query-fsmonitor.sample
>> @@ -0,0 +1,27 @@
>> +#!/bin/sh
>> +#
>> +# An example hook script to integrate Watchman
>> +# (https://facebook.github.io/watchman/) with git to provide fast
>> +# git status.
>> +#
>> +# The hook is passed a time_t formatted as a string and outputs to
>> +# stdout all files that have been modified since the given time.
>> +# Paths must be relative to the root of the working tree and
>> +# separated by a single NUL.
>> +#
>> +# To enable this hook, rename this file to "query-fsmonitor"
>> +
>> +# Convert unix style paths to escaped Windows style paths
>> +case "$(uname -s)" in
>> +MINGW*|MSYS_NT*)
>> +  GIT_WORK_TREE="$(cygpath -aw "$PWD" | sed 's,\\,,g')"
>> +  ;;
>> +*)
>> +  GIT_WORK_TREE="$PWD"
>> +  ;;
>> +esac
>> +
>> +# Query Watchman for all the changes since the requested time
>> +echo "[\"query\", \"$GIT_WORK_TREE\", {\"since\": $1, 
>> \"fields\":[\"name\"]}]" | \
>> +watchman -j | \
>> +perl -e 'use JSON::PP; my $o = JSON::PP->new->utf8->decode(join("", <>)); 
>> die "Watchman: $o->{'error'}.\nFalling back to scanning...\n" if 
>> defined($o->{"error"}); print(join("\0", @{$o->{"files"}}));'
>
> Maybe put the above small perl script on multiple lines for improved
> readability.
>
> Is JSON::PP always available in Perl >= 5.8?

No, it's shipped with perl as of 5.14.0, which came out in May 2011.

I think depending on that is fine. FWIW we bumped the required core
dependency (but you might still need to install modules) in 2010 in my
d48b284183 ("perl: bump the required Perl version to 5.8 from
5.6.[21]", 2010-09-24). Maybe we should be bumping that again...

> What happens if watchman is not installed or not in the PATH?
> It seems to me that the git process will not die, and will just work
> as if the hook does not exist, except that it will call the hook which
> will probably output error messages.

I think a good solution to this is to just add "set -euo pipefail" to
that script.

Then we'll print out on stderr that we couldn't find the watchman
command. Right now (with my patch) it'll make its way to the perl
program and get empty input.

Re: Hide decorations in git log

[Ævar Arnfjörð Bjarmason]

On Wed, May 24, 2017 at 4:22 PM, Robert Dailey  wrote:
> Is it possible to hide decorated refs in `git log` even if they are
> reachable from the refs I'm actually interested in seeing the logs of?
>
> For example, if I do `git log --graph --decorate --oneline --branches
> 'feature/*'`, I'd like to *only* see refnames that match the glob
> pattern. However, you'll see tags and other branches that do not match
> the glob if they are reachable from the result set.
>
> This is purely a visual thing, and shouldn't impact the search results
> I'd think.
>
> This is especially useful in cases where I do --simplify-by-decoration
> to get a better understanding of the topology of just a couple of
> select branches. Without some sort of "decoration exclusion", I am
> getting ton of results including tags, etc which obfuscates the
> information I'm really interested in.

I don't think there's a way to do this, but it could be added. The
glob just matches the refs now, the decoration happens as an unrelated
step which doesn't care about the glob.

Re: [PATCH v2 1/7] grep: don't redundantly compile throwaway patterns under threading

[Ævar Arnfjörð Bjarmason]

On Fri, May 26, 2017 at 2:58 AM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason  writes:
>
>> I think it's a pointless distraction to start speculating in this
>> commit message what we're going to do with --debug it if it ever
>> starts emitting some debugging information at pattern execution time.
>
> OK.
>
>> As an aside, I'd very much like to remove both --debug and the
>> --and/--or/--all-match, gives some very rough edges in the UI and how
>> easy it is to make that feature error or segfault, I suspect you might
>> be the only one using it.
>
> I agree that rewriting "grep -e A -e B" to "grep -e A|B" as an
> optimization is an interesting possibility to look into, and I can
> understand that having to support "--and" and "--not" would
> make such an optimization harder to implement. "-e A --and -e B"
> must become "-e A.*B|B.*A" and as you get more terms your unified
> pattern will grow combinatorial, at which point you would be better
> off matching N patterns and combining the result.
>
> Ever saw a user run "ps | grep rogue | grep -v grep" to find a rogue
> process to kill?  That would not work if the rogue process's command
> line has a word "grep".  Because "git grep" is often run on files in
> order to find the location the patterns appear in, "git grep -e
> pattern | grep -v unwanted" shares the same issue--the unwanted
> pattern may appear in the filename, and the downstream "grep -v" may
> filter out a valid hit.  This is why "--not" exists [*1*].  I agree
> that emulating it within the same "concatenate patterns into one"
> optimization you are envisioning may be hard.
>
> Attempting to optimize "--all-match" would share similar difficulty
> with "--and", but your matching now must be done with the entire
> buffer and not go line-by-line.  It was meant to make it possible to
> say "find commits that avarab@ talks about both regex and log", i.e.
>
> $ git log --author=avarab@ --all-match --grep=log --grep=regex
>
> This is not something you can emulate by piping an output of grep to
> another grep.
>
> But none of the above means you have to give up optimizing.
>
> You can choose not to combine them into a single pattern if certain
> constructions are hard, and do only the easy ones.  If you think
> that harder combinations are not used very often, the result would
> be faster for many cases while not losing useful features, which is
> what we want.

To be clear the point of my mail was not to say "I can't think of a
way to support both of these things, help!", obviously we can continue
to maintain two codepaths. The point was to raise the idea that we
could simply remove the more complex & doomed to forever be slow
codepath.

Obviously there are caveats with the likes of "grep foo | grep bar"
that don't exist with "grep -e foo --and -e bar". I'm less interested
in whether we can come up with cases that wouldn't be possible if this
were removed, than if anyone's using them in practice.

I suspect that to the extent anyone uses this for common things it
could be emulated by --single-line --perl-regexp and e.g. 'foo.*bar'
instead of 'foo' --and 'bar'. I.e. we could offer to AND together your
regexes and match them over the entire content.

If someone needed something more complex we could just show an example
of piping e.g. \0-delimited commit messages into an arbitrary perl
script you provide.

Anyway, I've only looked this over a tiny bit, and I don't know
whether it's worth it to remove this, right now I was just interested
in some reports of what it was used for. I.e. whether anyone uses it
for N-level deep mixed AND/OR branches, or whether it's really just a
lazy way to concat regexes and get around the current limitation of
not being able to match across lines.

> [Footnote]
>
> *1* For human consumption, lack of "--not" may not hurt in the sense
> that there are workarounds (i.e. you can do without "| grep -v
> unwanted" and filter irrelevant ones by eyeballing).  But it is
> essential while scripting and trying to be precise.

Re: Bug report: Corrupt pack file after committing a large file (>4 GB?)

[Torsten Bögershausen]

On 2017-05-26 07:51, Yu-Hsuan Chen wrote:
> Dear maintainer,
> 
> There is a bug where committing a large file corrupts the pack file in
> Windows. Steps to recreate are:
> 
> 1. git init
> 2. stage and commit a file larger than 4 GB (not entirely sure about this 
> size)
> 3. git checkout -f
> 
> The file checked out is much smaller than the original file size.
> 
> This behavior is surprising. If git does not support large files, I
> would at least expect an error message when staging or committing. I
> have post a question on StackOverflow regrading this issue, and has
> been confirmed by another user. (question id: 44022897)
> 
> Best regards,
> 
> David Chen
> 
Issues for Git for Windows should, in general, be reported here:
https://github.com/git-for-windows/git/

After 2 seconds of searching, we can find that the 4Gb problem
has already been reported:
https://github.com/git-for-windows/git/issues/1063

And, to my knowledge, it has not been fixed, since it is a
lot of effort to replace the "long" (or unsigned long) in the
Git code base with a better data type.

In other words, thanks for reminding us, more help is needed.

Re: Bug report: Corrupt pack file after committing a large file (>4 GB?)

[Konstantin Khomoutov]

On Fri, May 26, 2017 at 01:51:34PM +0800, Yu-Hsuan Chen wrote:

> There is a bug where committing a large file corrupts the pack file in
> Windows. Steps to recreate are:
> 
> 1. git init
> 2. stage and commit a file larger than 4 GB (not entirely sure about this 
> size)
> 3. git checkout -f
> 
> The file checked out is much smaller than the original file size.

For a bug report to be at least marginally useful, please state your Git
version (run "git --version") and the version of your Windows
installation (including whether it's 32- or 64-bit install).

Please also make sure you're really using Git for Windows (that is, you
got it from [1] or [2] and not, say, from Cygwin.

The best course of actions is to download the most recent available
version from the locations mentioned above and verify the problem still
manifests itself.

1. https://git-scm.com/download/win
2. https://git-for-windows.github.io/