Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()

[Christian Couder]

On Fri, Oct 27, 2017 at 4:57 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> It is fine to leave the original code broken at this step while we
>> purely move the lines around, and hopefully this will be corrected
>> in a later step in the series (I am responding as I read on, so I do
>> not yet know at which patch that happens in this series).
>
> Actually, I think you'd probably be better off if you fixed these
> broken comparisions that does (@list1 eq @list2) very early in the
> series, perhaps as [PATCH 0.8/6].

Yeah, it's better to fix the comparisons first.

> I am sure Perl experts among us on the list can come up with a
> cleaner and better implementation of compare_lists sub I am adding
> here, but in the meantime, here is what I would start with if I were
> working on this topic.

Thanks for the patch,
Christian.

Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Ben Peart]

On 10/26/2017 5:27 PM, Alex Vandiver wrote:

On Thu, 26 Oct 2017, Ben Peart wrote:

On 10/25/2017 9:31 PM, Alex Vandiver wrote:

diff --git a/fsmonitor.c b/fsmonitor.c
index 7c1540c05..0d26ff34f 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t
last_update, struct strbuf *que
argv[3] = NULL;
cp.argv = argv;
cp.use_shell = 1;
+cp.dir = get_git_work_tree();
   


I'm not sure what would trigger this problem but I don't doubt that it is
possible.  Better to be safe than sorry. This is a better/faster solution than
you're previous patch.  Thank you!


See my response on the v1 of this series -- I'm curious how you're
_not_ seeing it, actually.  Can  you not replicate just by running
`git status` from differing parts of the working tree?
  - Alex



I saw your response but actually can't replicate it locally.  I've been 
running with Watchman integration on all my repos for months and my 
"watchman watch-list" command only shows the root of the various working 
directories - no subdirectories.

Re: [PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman

[Ben Peart]

On 10/26/2017 5:29 PM, Alex Vandiver wrote:

On Thu, 26 Oct 2017, Ben Peart wrote:

On 10/25/2017 9:31 PM, Alex Vandiver wrote:

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index a3e30bf54..79f24325c 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -50,7 +50,7 @@ launch_watchman();
 sub launch_watchman {
   -my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
+   my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')


Since this is a test script performance isn't critical.  This version of the
integration script logs the response to a file in .git/watchman-response.json
and is much more human readable without the "--no-pretty."  As such, I'd leave
this one pretty.


This would be the first delta between the test file and the template
file.  It seems quite important to me to attempt to ensure that we're
testing the _same_ contents that we're suggesting users set up.  In
fact, it makes more sense to me to just turn this into a symlink to the
sample template.
  - Alex



If you look closer (actually diff the two files) you will see that the 
test version includes logging that the sample doesn't include.  I found 
the logging very helpful during testing of the feature and debugging 
Watchman issues but don't want end users to pay the performance penalty 
of the logging.


t/t7519/fsmonitor-watchman isn't actually used by any of the automated 
tests as they can't assume Watchman is available. It is just provided as 
a convenience to enable manual testing and debugging.

Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()

[Junio C Hamano]

Junio C Hamano  writes:

> It is fine to leave the original code broken at this step while we
> purely move the lines around, and hopefully this will be corrected
> in a later step in the series (I am responding as I read on, so I do
> not yet know at which patch that happens in this series).

Actually, I think you'd probably be better off if you fixed these
broken comparisions that does (@list1 eq @list2) very early in the
series, perhaps as [PATCH 0.8/6].  

I am sure Perl experts among us on the list can come up with a
cleaner and better implementation of compare_lists sub I am adding
here, but in the meantime, here is what I would start with if I were
working on this topic.

Thanks.

 t/t0021/rot13-filter.pl | 35 ---
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index ad685d92f8..9bf5a756af 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -107,21 +107,42 @@ sub packet_flush {
STDOUT->flush();
 }
 
+sub compare_lists {
+   my ($expect, @result) = @_;
+   my $ix;
+   if (scalar @$expect != scalar @result) {
+   return undef;
+   }
+   for ($ix = 0; $ix < $#result; $ix++) {
+   if ($expect->[$ix] ne $result[$ix]) {
+   return undef;
+   }
+   }
+   return 1;
+}
+
 print $debug "START\n";
 $debug->flush();
 
-( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
-( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
-( packet_bin_read() eq ( 1, "" ) )  || die "bad version end";
+compare_lists([0, "git-filter-client"], packet_txt_read()) ||
+   die "bad initialize";
+compare_lists([0, "version=2"], packet_txt_read()) ||
+   die "bad version";
+compare_lists([1, ""], packet_bin_read()) ||
+   die "bad version end";
 
 packet_txt_write("git-filter-server");
 packet_txt_write("version=2");
 packet_flush();
 
-( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
-( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
-( packet_txt_read() eq ( 0, "capability=delay" ) )  || die "bad capability";
-( packet_bin_read() eq ( 1, "" ) )  || die "bad capability 
end";
+compare_lists([0, "capability=clean"], packet_txt_read()) ||
+   die "bad capability";
+compare_lists([0, "capability=smudge"], packet_txt_read()) ||
+   die "bad capability";
+compare_lists([0, "capability=delay"], packet_txt_read()) ||
+   die "bad capability";
+compare_lists([1, ""], packet_bin_read()) ||
+   die "bad capability end";
 
 foreach (@capabilities) {
packet_txt_write( "capability=" . $_ );

Re: [PATCH v3] blame: prevent error if range ends past end of file

[Junio C Hamano]

Junio C Hamano  writes:

> For example, with an empty file (i.e. lno == 0), you can ask "git
> blame -L1,-4 ("i.e. "at most four lines, ending at line #1") and the
> code silently accepts the input without noticing that the request is
> an utter nonsense; "file X has only 0 lines" error is given a chance

s/is given/is not given/; obviously.  Sorry for a typo coming from
laggy ssh connection.

> to kick in.
>
> There should be an "is the range sensible?" check after all the
> tweaking to bottom and top are done, I think.
>
>>  bottom--;
>>  range_set_append_unsafe(, bottom, top);
>> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
>> index 661f9d430..728209fa3 100755
>> --- a/t/t8003-blame-corner-cases.sh
>> +++ b/t/t8003-blame-corner-cases.sh
>> @@ -216,14 +216,13 @@ test_expect_success 'blame -L with invalid start' '
>>  '
>>  
>>  test_expect_success 'blame -L with invalid end' '
>> -test_must_fail git blame -L1,5 tres 2>errors &&
>> -test_i18ngrep "has only 2 lines" errors
>> +git blame -L1,5 tres >out &&
>> +test_line_count = 2 out
>>  '
>>  
>>  test_expect_success 'blame parses  part of -L' '
>>  git blame -L1,1 tres >out &&
>> -cat out &&
>> -test $(wc -l < out) -eq 1
>> +test_line_count = 1 out
>>  '
>>  
>>  test_expect_success 'indent of line numbers, nine lines' '

Re: [PATCH v3] blame: prevent error if range ends past end of file

[Junio C Hamano]

Isabella Stephens  writes:

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 67adaef4d..b5b9db147 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -878,13 +878,13 @@ int cmd_blame(int argc, const char **argv, const char 
> *prefix)
>   nth_line_cb, , lno, anchor,
>   , , sb.path))
>   usage(blame_usage);
> - if (lno < top || ((lno || bottom) && lno < bottom))
> + if ((lno || bottom) && lno < bottom)
>   die(Q_("file %s has only %lu line",
>  "file %s has only %lu lines",
>  lno), path, lno);
>   if (bottom < 1)
>   bottom = 1;
> - if (top < 1)
> + if (top < 1 || lno < top)
>   top = lno;

This section sanity-checks first and then tweaks the values it
allowed to pass the check.  Because it wants to later fix up an
overly large "top" by capping to "lno" (i.e. total line number), the
patch needs to loosen the early sanity-check.  And the "fixed up"
values are never checked if they are sane.

For example, with an empty file (i.e. lno == 0), you can ask "git
blame -L1,-4 ("i.e. "at most four lines, ending at line #1") and the
code silently accepts the input without noticing that the request is
an utter nonsense; "file X has only 0 lines" error is given a chance
to kick in.

There should be an "is the range sensible?" check after all the
tweaking to bottom and top are done, I think.

>   bottom--;
>   range_set_append_unsafe(, bottom, top);
> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index 661f9d430..728209fa3 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -216,14 +216,13 @@ test_expect_success 'blame -L with invalid start' '
>  '
>  
>  test_expect_success 'blame -L with invalid end' '
> - test_must_fail git blame -L1,5 tres 2>errors &&
> - test_i18ngrep "has only 2 lines" errors
> + git blame -L1,5 tres >out &&
> + test_line_count = 2 out
>  '
>  
>  test_expect_success 'blame parses  part of -L' '
>   git blame -L1,1 tres >out &&
> - cat out &&
> - test $(wc -l < out) -eq 1
> + test_line_count = 1 out
>  '
>  
>  test_expect_success 'indent of line numbers, nine lines' '

Re: [PATCH] rev-list-options.txt: use correct directional reference

[Junio C Hamano]

SZEDER Gábor  writes:

> The descriptions of the options '--parents', '--children' and
> '--graph' say "see 'History Simplification' below", although the
> referred section is in fact above the description of these options.
>
> Send readers in the right direction by saying "above" instead of
> "below".
>
> Signed-off-by: SZEDER Gábor 
> ---

Thanks.  

It turns out that this is not a recent regression, but was done at
f98fd436 ("git-log.txt,rev-list-options.txt: put option blocks in
proper order", 2011-03-08), which moved Commit Formatting and Diff
Formatting sections, which used to appear very early, to near the
end of the sequence.

>  Documentation/rev-list-options.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt 
> b/Documentation/rev-list-options.txt
> index 7d860bfca..13501e155 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -799,11 +799,11 @@ endif::git-rev-list[]
>  
>  --parents::
>   Print also the parents of the commit (in the form "commit parent...").
> - Also enables parent rewriting, see 'History Simplification' below.
> + Also enables parent rewriting, see 'History Simplification' above.
>  
>  --children::
>   Print also the children of the commit (in the form "commit child...").
> - Also enables parent rewriting, see 'History Simplification' below.
> + Also enables parent rewriting, see 'History Simplification' above.
>  
>  ifdef::git-rev-list[]
>  --timestamp::
> @@ -846,7 +846,7 @@ you would get an output like this:
>   to be drawn properly.
>   Cannot be combined with `--no-walk`.
>  +
> -This enables parent rewriting, see 'History Simplification' below.
> +This enables parent rewriting, see 'History Simplification' above.
>  +
>  This implies the `--topo-order` option by default, but the
>  `--date-order` option may also be specified.

Re: [PATCH] dir: allow exclusions from blob in addition to file

[Junio C Hamano]

Jeff Hostetler  writes:

> From: Jeff Hostetler 
>
> Refactor add_excludes() to separate the reading of the
> exclude file into a buffer and the parsing of the buffer
> into exclude_list items.
>
> Add add_excludes_from_blob_to_list() to allow an exclude
> file be specified with an OID without assuming a local
> worktree or index exists.
>
> Refactor read_skip_worktree_file_from_index() and add
> do_read_blob() to eliminate duplication of preliminary
> processing of blob contents.
>
> Signed-off-by: Jeff Hostetler 
> ---

Yeah, with a separate do_read_blob() helper, this one looks a lot
easier to follow, at least to me---as the author, you might find the
earlier one just as easy, I suspect, though ;-)

Thanks.  Will queue.

Re: [PATCH] t0000: check whether the shell supports the "local" keyword

[Junio C Hamano]

Jacob Keller  writes:

> I don't think you're missing anything. I think the idea here is: "do
> any users who actively run the test suite care if we start using
> local". I don't think the goal is to allow use of local in non-test
> suite code. At least, that's not how I interpreted it.
>
> Thus it's fine to be only as part of a test and see if anyone
> complains, since the only people affected would be those which
> actually run the test suite...
>
> Changing our requirement for regular shell scripts we ship seems a lot
> trickier to gauge.

Yup, that matches my expectations for what we would gain out of this
change.

Re: Consequences of CRLF in index?

[Junio C Hamano]

Ross Kabus  writes:

> Is "* -text" in any way different than "-text" (without the * asterisk)? All
> of my .gitattributes files have "-text" (no * asterisk) and I haven't noticed
> any difference but could I be missing something subtle?
>
> ~Ross

A line in the .gitattibute file is of the form

   ...

i.e. a pattern to match paths, with a list of attribute definitions.
The asterisk they are showing in their description is the 
part, i.e. "apply the '-text' thing to paths that match '*'", which
is equivalent to saying "set text attribute to false for all paths".

Re: What's cooking in git.git (Oct 2017, #05; Tue, 24)

[Junio C Hamano]

Ramsay Jones  writes:

> On 24/10/17 06:28, Junio C Hamano wrote:
> [snip]> 
>> * tg/deprecate-stash-save (2017-10-23) 3 commits
>>  - stash: remove now superfluos help for "stash push"
>>  - mark git stash push deprecated in the man page
>>  - replace git stash save with git stash push in the documentation
>> 
>>  "git stash save" has been deprecated in favour of "git stash push".
>> 
>>  Will merge to 'next'.
>
> If you don't intend to include this in v2.15.0, when re-building
> the next branch after release (the above is now in 'next'), could
> we please remember to update one of the commit message subject line.
>
> In particular, commit 742d6ce35b ("mark git stash push deprecated
> in the man page", 22-10-2017), is marking 'git stash *save*' as
> deprecated, not *push*.
>
> [Sorry for not spotting this earlier; I only noticed when doing an
> 'git log --oneline' display which, naturally, puts focus on the
> subject lines. ;-) ]

Thanks for spotting.  I can always revert the merge into 'next' and
then merge a rewritten copy of the series back into 'next' again to
preserve the fast-forwardness of the integration branch, so that I
do not have to remember ;-)

[PATCH v3] blame: prevent error if range ends past end of file

[Isabella Stephens]

If the -L option is used to specify a line range in git blame, and the
end of the range is past the end of the file, at present git will fail
with a fatal error. This commit prevents such behavior - instead the
blame is displayed for existing lines within the specified range.
Blaming a range that is entirely outside the bounds of the file will
still fail.

This commit also ammends the relevant test and adds clarification to
the documentation.

Signed-off-by: Isabella Stephens 
---
 Documentation/git-blame.txt   | 10 ++
 builtin/blame.c   |  4 ++--
 t/t8003-blame-corner-cases.sh |  7 +++
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index fdc3aea30..8a28b4114 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -154,6 +154,16 @@ Also you can use a regular expression to specify the line 
range:
 
 which limits the annotation to the body of the `hello` subroutine.
 
+A range that begins or ends outside the bounds of the file will
+blame the relevant lines. For example:
+
+   git blame -L 10,-20 foo
+   git blame -L 10,+20 foo
+
+will respectively blame the first 10 and last 11 lines of a
+20 line file. However, blaming a line range that is entirely
+outside the bounds of the file will fail.
+
 When you are not interested in changes older than version
 v2.6.18, or changes older than 3 weeks, you can use revision
 range specifiers  similar to 'git rev-list':
diff --git a/builtin/blame.c b/builtin/blame.c
index 67adaef4d..b5b9db147 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -878,13 +878,13 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
nth_line_cb, , lno, anchor,
, , sb.path))
usage(blame_usage);
-   if (lno < top || ((lno || bottom) && lno < bottom))
+   if ((lno || bottom) && lno < bottom)
die(Q_("file %s has only %lu line",
   "file %s has only %lu lines",
   lno), path, lno);
if (bottom < 1)
bottom = 1;
-   if (top < 1)
+   if (top < 1 || lno < top)
top = lno;
bottom--;
range_set_append_unsafe(, bottom, top);
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index 661f9d430..728209fa3 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -216,14 +216,13 @@ test_expect_success 'blame -L with invalid start' '
 '
 
 test_expect_success 'blame -L with invalid end' '
-   test_must_fail git blame -L1,5 tres 2>errors &&
-   test_i18ngrep "has only 2 lines" errors
+   git blame -L1,5 tres >out &&
+   test_line_count = 2 out
 '
 
 test_expect_success 'blame parses  part of -L' '
git blame -L1,1 tres >out &&
-   cat out &&
-   test $(wc -l < out) -eq 1
+   test_line_count = 1 out
 '
 
 test_expect_success 'indent of line numbers, nine lines' '
-- 
2.14.1

Re: [PATCH v2] blame: prevent error if range ends past end of file

[Isabella Stephens]

On 27/10/17 2:31 am, SZEDER Gábor wrote:
>> If the -L option is used to specify a line range in git blame, and the
>> end of the range is past the end of the file, at present git will fail
>> with a fatal error. This commit prevents such behaviour - instead the
>> blame is display for any existing lines within the specified range.
> 
> s/is display/is displayed/ ?
Oops.

> 
> 'git log' has a very similar -L option, which errors out, too, if the
> end of the line range is past the end of the file.  IMHO the
> interpretation of the line range -L, should be kept
> consistent in the two commands, and 'git log' shouldn't error out,
> either.
Good suggestion. I'll submit a separate patch for git log.

> 
>> Signed-off-by: Isabella Stephens 
>> ---
>>  builtin/blame.c   | 4 ++--
>>  t/t8003-blame-corner-cases.sh | 5 +++--
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index 67adaef4d..b5b9db147 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -878,13 +878,13 @@ int cmd_blame(int argc, const char **argv, const char 
>> *prefix)
>>  nth_line_cb, , lno, anchor,
>>  , , sb.path))
>>  usage(blame_usage);
>> -if (lno < top || ((lno || bottom) && lno < bottom))
>> +if ((lno || bottom) && lno < bottom)
>>  die(Q_("file %s has only %lu line",
>> "file %s has only %lu lines",
>> lno), path, lno);
>>  if (bottom < 1)
>>  bottom = 1;
>> -if (top < 1)
>> +if (top < 1 || lno < top)
>>  top = lno;
>>  bottom--;
>>  range_set_append_unsafe(, bottom, top);
>> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
>> index 661f9d430..32b3788fe 100755
>> --- a/t/t8003-blame-corner-cases.sh
>> +++ b/t/t8003-blame-corner-cases.sh
>> @@ -216,8 +216,9 @@ test_expect_success 'blame -L with invalid start' '
>>  '
>>  
>>  test_expect_success 'blame -L with invalid end' '
>> -test_must_fail git blame -L1,5 tres 2>errors &&
>> -test_i18ngrep "has only 2 lines" errors
>> +git blame -L1,5 tres >out &&
>> +cat out &&
>> +test $(wc -l < out) -eq 2
> 
> Please use the test_line_count helper instead, as it will output a
> helpful error message on failure, making the 'cat out' above
> unnecessary.
> Thanks for pointing that out.

>>  '
>>  
>>  test_expect_success 'blame parses  part of -L' '
>> -- 
>> 2.14.1
>>
>>

Re: [PATCH v2] blame: prevent error if range ends past end of file

[Isabella Stephens]

On 26/10/17 7:48 pm, Jacob Keller wrote:
> On Thu, Oct 26, 2017 at 12:01 AM, Isabella Stephens
>  wrote:
>> If the -L option is used to specify a line range in git blame, and the
>> end of the range is past the end of the file, at present git will fail
>> with a fatal error. This commit prevents such behaviour - instead the
>> blame is display for any existing lines within the specified range.
>>
>> Signed-off-by: Isabella Stephens 
>> ---
> 
> I like this change. We might want to document L to indicate that an L
> that is outside the range of lines will show all lines that do match.
> 
> Maybe we also want it to only succeed if at least some lines are
> blamed? Could we make it so that it fails if no lines are within the
> range? (ie: the start point is too far in? or does it already do
> such?)
> 
> Thanks,
> Jake

Yep, that is exactly how it behaves now - if a range intersects the
file at all it will annotate the relevant lines, otherwise it will fail.

I'll add a clarification to the documentation in my next revision.
Thanks for reviewing!

Re: Consequences of CRLF in index?

[Ross Kabus]

Is "* -text" in any way different than "-text" (without the * asterisk)? All
of my .gitattributes files have "-text" (no * asterisk) and I haven't noticed
any difference but could I be missing something subtle?

~Ross

gitk --version and --help

[Jonny Grant]

Would be useful if these missing options could be added.

Was just checking the version using regular git when I noticed.

$ git --version
git version 2.7.4

I'm not a member of the list, so please cc me in any replies.

Cheers, Jonny

Re: [PATCH] tag: add tag.gpgSign config option to force all tags be GPG-signed

[Stefan Beller]

On Thu, Oct 26, 2017 at 2:33 PM, Jonathan Nieder  wrote:
> Hi again,
>
> Mkrtchyan, Tigran wrote:
>> Jonathan Nieder wrote:
>>> Tigran Mkrtchyan wrote:
>
 In some workflows we have no control on how git command is executed,
 however a signed tags are required.
>>>
>>> Don't leave me hanging: this leaves me super curious.  Can you tell me
>>> more about these workflows?
>>
>> Well, this is a build/release process where we can't pass additional
>> command line options to git. TO be hones, is case of annotated tags
>> there is already option tag.forceSignAnnotated. However, non annotated
>> tags are not forced to be signed.
>>
>> Additionally, the proposed option is symmetric with commit.gpgSign.
>
> Now I'm even more curious.

I started digging and found
https://public-inbox.org/git/20131105112840.gz4...@mars-attacks.org/
which is an answer to "Why do we have commit.gpgSign?" which is
a very similar question to begin with.

Maybe the answer is also similar (bonus points if the answer also touches
when to prefer one over the other)?

Re: [PATCH] tag: add tag.gpgSign config option to force all tags be GPG-signed

[Jonathan Nieder]

Hi again,

Mkrtchyan, Tigran wrote:
> Jonathan Nieder wrote:
>> Tigran Mkrtchyan wrote:

>>> In some workflows we have no control on how git command is executed,
>>> however a signed tags are required.
>>
>> Don't leave me hanging: this leaves me super curious.  Can you tell me
>> more about these workflows?
>
> Well, this is a build/release process where we can't pass additional
> command line options to git. TO be hones, is case of annotated tags
> there is already option tag.forceSignAnnotated. However, non annotated
> tags are not forced to be signed.
>
> Additionally, the proposed option is symmetric with commit.gpgSign.

Now I'm even more curious.

I don't think we have the full picture to understand whether this
change is needed.  When adding a configuration item, we need to be
able to explain to users what the configuration item is for, and so
far the only answer I am hearing is "because we do not want to patch
our build/release script, though we could in principle".  That doesn't
sound like a compelling reason.

On the other hand, perhaps the answer is "our build/release script
does not have a --sign option for the following reason, and this is a
better interface for configuring it".

Or perhaps there is an answer that does not involve the build/release
script.

But with no answer at all, it is hard to see why we should move
forward on this patch.

To be clear, I am not saying that writing the patch is wasted effort.
E.g. you can continue to use it internally, and it means that once we
have a clear reason to add this configuration, the patch is there and
ready to use to do so.

Thanks again,
Jonathan

Re: [PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman

[Alex Vandiver]

On Thu, 26 Oct 2017, Ben Peart wrote:
> On 10/25/2017 9:31 PM, Alex Vandiver wrote:
> > diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
> > index a3e30bf54..79f24325c 100755
> > --- a/t/t7519/fsmonitor-watchman
> > +++ b/t/t7519/fsmonitor-watchman
> > @@ -50,7 +50,7 @@ launch_watchman();
> > sub launch_watchman {
> >   - my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
> > +   my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
> 
> Since this is a test script performance isn't critical.  This version of the
> integration script logs the response to a file in .git/watchman-response.json
> and is much more human readable without the "--no-pretty."  As such, I'd leave
> this one pretty.

This would be the first delta between the test file and the template
file.  It seems quite important to me to attempt to ensure that we're
testing the _same_ contents that we're suggesting users set up.  In
fact, it makes more sense to me to just turn this into a symlink to the
sample template.
 - Alex

Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Alex Vandiver]

On Thu, 26 Oct 2017, Ben Peart wrote:
> On 10/25/2017 9:31 PM, Alex Vandiver wrote:
> > diff --git a/fsmonitor.c b/fsmonitor.c
> > index 7c1540c05..0d26ff34f 100644
> > --- a/fsmonitor.c
> > +++ b/fsmonitor.c
> > @@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t
> > last_update, struct strbuf *que
> > argv[3] = NULL;
> > cp.argv = argv;
> > cp.use_shell = 1;
> > +cp.dir = get_git_work_tree();
> >   
>
> I'm not sure what would trigger this problem but I don't doubt that it is
> possible.  Better to be safe than sorry. This is a better/faster solution than
> you're previous patch.  Thank you!

See my response on the v1 of this series -- I'm curious how you're
_not_ seeing it, actually.  Can  you not replicate just by running
`git status` from differing parts of the working tree?
 - Alex

Re: [PATCH] tag: add tag.gpgSign config option to force all tags be GPG-signed

[Mkrtchyan, Tigran]

Well, this is a build/release process where we can't pass additional
command line options to git. TO be hones, is case of annotated tags
there is already option tag.forceSignAnnotated. However, non annotated
tags are not forced to be signed.

Additionally, the proposed option is symmetric with commit.gpgSign.

Tigran.

- Original Message -
> From: "Jonathan Nieder" 
> To: "Tigran Mkrtchyan" 
> Cc: git@vger.kernel.org
> Sent: Thursday, October 26, 2017 10:55:09 PM
> Subject: Re: [PATCH] tag: add tag.gpgSign config option to force all tags be 
> GPG-signed

> Hi,
> 
> Tigran Mkrtchyan wrote:
> 
>> In some workflows we have no control on how git command is executed,
>> however a signed tags are required.
> 
> Don't leave me hanging: this leaves me super curious.  Can you tell me
> more about these workflows?
> 
> Thanks and hope that helps,
> Jonathan

Re: [PATCH] tag: add tag.gpgSign config option to force all tags be GPG-signed

[Jonathan Nieder]

Hi,

Tigran Mkrtchyan wrote:

> In some workflows we have no control on how git command is executed,
> however a signed tags are required.

Don't leave me hanging: this leaves me super curious.  Can you tell me
more about these workflows?

Thanks and hope that helps,
Jonathan

Re: What's cooking in git.git (Oct 2017, #05; Tue, 24)

[Thomas Gummerer]

On 10/26, Ramsay Jones wrote:
> 
> 
> On 24/10/17 06:28, Junio C Hamano wrote:
> [snip]> 
> > * tg/deprecate-stash-save (2017-10-23) 3 commits
> >  - stash: remove now superfluos help for "stash push"
> >  - mark git stash push deprecated in the man page
> >  - replace git stash save with git stash push in the documentation
> > 
> >  "git stash save" has been deprecated in favour of "git stash push".
> > 
> >  Will merge to 'next'.
> 
> If you don't intend to include this in v2.15.0, when re-building
> the next branch after release (the above is now in 'next'), could
> we please remember to update one of the commit message subject line.
> 
> In particular, commit 742d6ce35b ("mark git stash push deprecated
> in the man page", 22-10-2017), is marking 'git stash *save*' as
> deprecated, not *push*.

Sorry about this.  Would indeed be great if we could still fix it.
Thanks for catching this.

> [Sorry for not spotting this earlier; I only noticed when doing an
> 'git log --oneline' display which, naturally, puts focus on the
> subject lines. ;-) ]
> 
> ATB,
> Ramsay Jones
> 
> 

Re: Consequences of CRLF in index?

[Johannes Sixt]

Thank you for the clarification, it's much appreciated.

-- Hannes

Re: [PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman

[Ben Peart]

On 10/26/2017 4:05 PM, Ben Peart wrote:



On 10/25/2017 9:31 PM, Alex Vandiver wrote:

This provides modest performance savings.  Benchmarking with the
following program, with and without `--no-pretty`, we find savings of
23% (0.316s -> 0.242s) in the git repository, and savings of 8% (5.24s
-> 4.86s) on a large repository with 580k files in the working copy.



Given this patch series is all about speed, it's good to see *any* wins 
- especially those that don't impact functionality at all.  The 
performance win of --no-pretty is greater than I expected.



 #!/usr/bin/perl

 use strict;
 use warnings;
 use IPC::Open2;

 my $pid = open2(\*CHLD_OUT, \*CHLD_IN, "watchman -j @ARGV")
 or die "open2() failed: $!\n" .
 "Falling back to scanning...\n";

 my $query = qq|["query", "$ENV{PWD}", {}]|;

 print CHLD_IN $query;
 close CHLD_IN;
 my $response = do {local $/; };

 my $json_pkg;
 eval {
 require JSON::XSomething;
 $json_pkg = "JSON::XSomething";
 1;
 } or do {
 require JSON::PP;
 $json_pkg = "JSON::PP";
 };

 my $o = $json_pkg->new->utf8->decode($response);

Signed-off-by: Alex Vandiver 
---
  t/t7519/fsmonitor-watchman | 2 +-
  templates/hooks--fsmonitor-watchman.sample | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index a3e30bf54..79f24325c 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -50,7 +50,7 @@ launch_watchman();
  sub launch_watchman {
-    my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
+    my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')


Since this is a test script performance isn't critical.  This version of 
the integration script logs the response to a file in 
.git/watchman-response.json and is much more human readable without the 
"--no-pretty."  As such, I'd leave this one pretty.


I didn't see anything (including this) worth another roll so only 
address it if something else comes up.





  or die "open2() failed: $!\n" .
  "Falling back to scanning...\n";
diff --git a/templates/hooks--fsmonitor-watchman.sample 
b/templates/hooks--fsmonitor-watchman.sample

index 9eba8a740..9a082f278 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -49,7 +49,7 @@ launch_watchman();
  sub launch_watchman {
-    my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
+    my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')


No human will see this response so the faster --no-pretty option makes 
sense.



  or die "open2() failed: $!\n" .
  "Falling back to scanning...\n";

Re: Consequences of CRLF in index?

[Jonathan Nieder]

Johannes Sixt wrote:
> Am 26.10.2017 um 13:01 schrieb Lars Schneider:

>> * -text
>> *.sh   text eol=lf
>
> Why would that be necessary? I cannot have CRLF in shell scripts etc., not
> even on Windows. (And in addition I do not mind CR in C++ source code.) All
> I want is: Git, please, by all means, do not mess with my files, ever. Isn't
> the simplest way to achieve that to set *nothing* at all? Not even
> core.autocrlf?

That's why Lars suggests "* -text" in .gitattributes.  That way, if
some user of the repository *does* set core.autocrlf, you still get
the "do not mess with my files" semantics you want.

If you control the configuration of all users of the repository, you
don't need that, but it doesn't hurt, either.

With that "* -text", you do not need "*.sh text eol=lf", but maybe
you'd want it in order to get some warnings when someone changes the
line endings by mistake without running tests.  (Better to have a
culture of running tests, you might say.  Fair enough, but as with the
other .gitattributes line, it doesn't hurt.)

Jonathan

Re: [PATCH v2 4/4] fsmonitor: Delay updating state until after split index is merged

[Ben Peart]

On 10/25/2017 9:31 PM, Alex Vandiver wrote:

If the fsmonitor extension is used in conjunction with the split index
extension, the set of entries in the index when it is first loaded is
only a subset of the real index.  This leads to only the non-"base"
index being marked as CE_FSMONITOR_VALID.

Delay the expansion of the ewah bitmap until after tweak_split_index
has been called to merge in the base index as well.

The new fsmonitor_dirty is kept from being leaked by dint of being
cleaned up in post_read_index_from, which is guaranteed to be called
after do_read_index in read_index_from.

Signed-off-by: Alex Vandiver 
---
  cache.h |  1 +
  fsmonitor.c | 39 ---
  2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/cache.h b/cache.h
index 25adcf681..0a4f43ec2 100644
--- a/cache.h
+++ b/cache.h
@@ -348,6 +348,7 @@ struct index_state {
unsigned char sha1[20];
struct untracked_cache *untracked;
uint64_t fsmonitor_last_update;
+   struct ewah_bitmap *fsmonitor_dirty;
  };
  
  extern struct index_state the_index;

diff --git a/fsmonitor.c b/fsmonitor.c
index 0d26ff34f..fad9c6b13 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate, 
const void *data,
ewah_free(fsmonitor_dirty);
return error("failed to parse ewah bitmap reading fsmonitor index 
extension");
}
-
-   if (git_config_get_fsmonitor()) {
-   /* Mark all entries valid */
-   for (i = 0; i < istate->cache_nr; i++)
-   istate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;
-
-   /* Mark all previously saved entries as dirty */
-   ewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);
-
-   /* Now mark the untracked cache for fsmonitor usage */
-   if (istate->untracked)
-   istate->untracked->use_fsmonitor = 1;
-   }
-   ewah_free(fsmonitor_dirty);
+   istate->fsmonitor_dirty = fsmonitor_dirty;
  
  	trace_printf_key(_fsmonitor, "read fsmonitor extension successful");

return 0;
@@ -239,7 +226,29 @@ void remove_fsmonitor(struct index_state *istate)
  
  void tweak_fsmonitor(struct index_state *istate)

  {
-   switch (git_config_get_fsmonitor()) {
+   int i;
+   int fsmonitor_enabled = git_config_get_fsmonitor();
+


The logic looks good this time.  It is nice to know this will now be 
optimal when split index is also turned on.  Thank you.



+   if (istate->fsmonitor_dirty) {
+   if (fsmonitor_enabled) {
+   /* Mark all entries valid */
+   for (i = 0; i < istate->cache_nr; i++) {
+   istate->cache[i]->ce_flags |= 
CE_FSMONITOR_VALID;
+   }
+
+   /* Mark all previously saved entries as dirty */
+   ewah_each_bit(istate->fsmonitor_dirty, 
fsmonitor_ewah_callback, istate);
+
+   /* Now mark the untracked cache for fsmonitor usage */
+   if (istate->untracked)
+   istate->untracked->use_fsmonitor = 1;
+   }
+
+   ewah_free(istate->fsmonitor_dirty);
+   istate->fsmonitor_dirty = NULL;
+   }
+
+   switch (fsmonitor_enabled) {
case -1: /* keep: do nothing */
break;
case 0: /* false */

Re: Consequences of CRLF in index?

[Johannes Sixt]

Am 26.10.2017 um 13:01 schrieb Lars Schneider:

On 26 Oct 2017, at 09:09, Johannes Sixt  wrote:
Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:

I envy you for the blessing of such a clean C++ source that you do not
have any, say, Unix shell script in it. Try this, and weep:
$ printf 'echo \\\r\n\t123\r\n' >a1
$ sh a1
a1: 2: a1: 123: not found


I was bitten by that, too. For this reason, I ensure that shell scripts and 
Makefiles begin their life on Linux. Fortunately, modern editors on Windows, 
includ^Wand vi, do not force CRLF line breaks, and such files can be edited on 
Windows, too.


Wouldn't this kind of .gitattributes setup solve the problem?


But there is no problem. Don't have CRs in shell scripts and be done 
with it.




* -text
*.sh   text eol=lf


Why would that be necessary? I cannot have CRLF in shell scripts etc., 
not even on Windows. (And in addition I do not mind CR in C++ source 
code.) All I want is: Git, please, by all means, do not mess with my 
files, ever. Isn't the simplest way to achieve that to set *nothing* at 
all? Not even core.autocrlf?


-- Hannes

Re: [PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman

[Ben Peart]

On 10/25/2017 9:31 PM, Alex Vandiver wrote:

This provides modest performance savings.  Benchmarking with the
following program, with and without `--no-pretty`, we find savings of
23% (0.316s -> 0.242s) in the git repository, and savings of 8% (5.24s
-> 4.86s) on a large repository with 580k files in the working copy.



Given this patch series is all about speed, it's good to see *any* wins 
- especially those that don't impact functionality at all.  The 
performance win of --no-pretty is greater than I expected.



 #!/usr/bin/perl

 use strict;
 use warnings;
 use IPC::Open2;

 my $pid = open2(\*CHLD_OUT, \*CHLD_IN, "watchman -j @ARGV")
 or die "open2() failed: $!\n" .
 "Falling back to scanning...\n";

 my $query = qq|["query", "$ENV{PWD}", {}]|;

 print CHLD_IN $query;
 close CHLD_IN;
 my $response = do {local $/; };

 my $json_pkg;
 eval {
 require JSON::XSomething;
 $json_pkg = "JSON::XSomething";
 1;
 } or do {
 require JSON::PP;
 $json_pkg = "JSON::PP";
 };

 my $o = $json_pkg->new->utf8->decode($response);

Signed-off-by: Alex Vandiver 
---
  t/t7519/fsmonitor-watchman | 2 +-
  templates/hooks--fsmonitor-watchman.sample | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index a3e30bf54..79f24325c 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -50,7 +50,7 @@ launch_watchman();
  
  sub launch_watchman {
  
-	my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')

+   my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')


Since this is a test script performance isn't critical.  This version of 
the integration script logs the response to a file in 
.git/watchman-response.json and is much more human readable without the 
"--no-pretty."  As such, I'd leave this one pretty.



or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
  
diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample

index 9eba8a740..9a082f278 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -49,7 +49,7 @@ launch_watchman();
  
  sub launch_watchman {
  
-	my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')

+   my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')


No human will see this response so the faster --no-pretty option makes 
sense.



or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
  

[PATCH] tag: add tag.gpgSign config option to force all tags be GPG-signed

[Tigran Mkrtchyan]

In some workflows we have no control on how git command is executed,
however a signed tags are required.

The new config-file option tag.gpgSign enforces signed tags. Additional
command line option --no-gpg-sign is added to disable such behavior if
needed. E.g.:

$ git tag -m "commit message"

will generate a GPG signed tag if tag.gpgSign option is true, while

$ git tag --no-gpg-sign -m "commit message"

will skip the signing step.

Signed-off-by: Tigran Mkrtchyan 
---
 Documentation/config.txt   |  4 
 Documentation/git-tag.txt  |  4 
 builtin/tag.c  | 18 +++---
 contrib/completion/git-completion.bash |  1 +
 t/t7004-tag.sh | 21 +
 5 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6ad..fa6694bec 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3161,6 +3161,10 @@ tag.forceSignAnnotated::
If `--annotate` is specified on the command line, it takes
precedence over this option.
 
+tag.gpgSign::
+
+   A boolean to specify whether all tags should be GPG signed.
+
 tag.sort::
This variable controls the sort ordering of tags when displayed by
linkgit:git-tag[1]. Without the "--sort=" option provided, the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f..1dd43f18b 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -181,6 +181,10 @@ This option is only applicable when listing tags without 
annotation lines.
`--create-reflog`, but currently does not negate the setting of
`core.logAllRefUpdates`.
 
+--no-gpg-sign::
+   Countermand `tag.gpgSign` configuration variable that is
+   set to force each and every tag to be signed.
+
 ::
The name of the tag to create, delete, or describe.
The new tag name must pass all checks defined by
diff --git a/builtin/tag.c b/builtin/tag.c
index b38329b59..d9060a404 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -31,6 +31,7 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 static int force_sign_annotate;
+static int sign_tag;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 struct ref_format *format)
@@ -141,6 +142,11 @@ static int git_tag_config(const char *var, const char 
*value, void *cb)
int status;
struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
 
+   if (!strcmp(var, "tag.gpgsign")) {
+   sign_tag = git_config_bool(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "tag.sort")) {
if (!value)
return config_error_nonbool(var);
@@ -372,6 +378,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
static struct ref_sorting *sorting = NULL, **sorting_tail = 
struct ref_format format = REF_FORMAT_INIT;
int icase = 0;
+   int no_gpg_sign = 0;
struct option options[] = {
OPT_CMDMODE('l', "list", , N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, , N_("n"),
@@ -393,6 +400,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
N_("use another key to sign the tag")),
OPT__FORCE(, N_("replace the tag if exists")),
OPT_BOOL(0, "create-reflog", _reflog, N_("create a 
reflog")),
+   OPT_BOOL(0, "no-gpg-sign", _gpg_sign, N_("do not GPG-sign 
tag")),
 
OPT_GROUP(N_("Tag listing options")),
OPT_COLUMN(0, "column", , N_("show tag list in 
columns")),
@@ -426,6 +434,10 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
 
argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
+   if (no_gpg_sign) {
+   sign_tag = 0;
+   }
+
if (keyid) {
opt.sign = 1;
set_signing_key(keyid);
@@ -444,7 +456,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (cmdmode == 'l')
setup_auto_pager("tag", 1);
 
-   if ((create_tag_object || force) && (cmdmode != 0))
+   if ((create_tag_object || force || no_gpg_sign) && (cmdmode != 0))
usage_with_options(git_tag_usage, options);
 
finalize_colopts(, -1);
@@ -536,8 +548,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
create_reflog_msg(, _msg);
 
-   if (create_tag_object) {
-   if (force_sign_annotate && !annotate)
+   if (create_tag_object || sign_tag) {
+   if (sign_tag || (force_sign_annotate && !annotate))
opt.sign = 1;
create_tag(, tag, , , , );
}
diff --git a/contrib/completion/git-completion.bash 

Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Ben Peart]

On 10/25/2017 9:31 PM, Alex Vandiver wrote:

The fsmonitor command inherits the PWD of its caller, which may be
anywhere in the working copy.  This makes is difficult for the
fsmonitor command to operate on the whole repository.  Specifically,
for the watchman integration, this causes each subdirectory to get its
own watch entry.

Set the CWD to the top of the working directory, for consistency.

Signed-off-by: Alex Vandiver 
---
  fsmonitor.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/fsmonitor.c b/fsmonitor.c
index 7c1540c05..0d26ff34f 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t 
last_update, struct strbuf *que
argv[3] = NULL;
cp.argv = argv;
cp.use_shell = 1;
+cp.dir = get_git_work_tree();
  


I'm not sure what would trigger this problem but I don't doubt that it 
is possible.  Better to be safe than sorry. This is a better/faster 
solution than you're previous patch.  Thank you!



return capture_command(, query_result, 1024);
  }

Re: Consequences of CRLF in index?

[Torsten Bögershausen]

On Thu, Oct 26, 2017 at 01:01:25PM +0200, Lars Schneider wrote:
> 
> > On 26 Oct 2017, at 09:09, Johannes Sixt  wrote:
> > 
> > Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:
> >> I envy you for the blessing of such a clean C++ source that you do not
> >> have any, say, Unix shell script in it. Try this, and weep:
> >>$ printf 'echo \\\r\n\t123\r\n' >a1
> >>$ sh a1
> >>a1: 2: a1: 123: not found
> > 
> > I was bitten by that, too. For this reason, I ensure that shell scripts and 
> > Makefiles begin their life on Linux. Fortunately, modern editors on 
> > Windows, includ^Wand vi, do not force CRLF line breaks, and such files can 
> > be edited on Windows, too.
> 
> Wouldn't this kind of .gitattributes setup solve the problem?
> 
> * -text
> *.sh   text eol=lf
> 

Yes, exactly. and for the snake-lovers:

*.py   text eol=lf

Re: Consequences of CRLF in index?

[Torsten Bögershausen]

On Wed, Oct 25, 2017 at 10:13:57AM -0700, Jonathan Nieder wrote:
> Hi again,
> 
> Lars Schneider wrote:
> >> On 24 Oct 2017, at 20:14, Jonathan Nieder  wrote:
> 
> >> In any event, you also probably want to declare what you're doing
> >> using .gitattributes.  By checking in the files as CRLF, you are
> >> declaring that you do *not* want Git to treat them as text files
> >> (i.e., you do not want Git to change the line endings), so something
> >> as simple as
> >>
> >>* -text
> >
> > That's sounds good. Does "-text" have any other implications?
> > For whatever reason I always thought this is the way to tell
> > Git that a particular file is binary with the implication that
> > Git should not attempt to diff it.
> 
> No other implications.  You're thinking of "-diff".  There is also a
> shortcut "binary" which simply means "-text -diff".

Not 100% the same, as far as I know.
"binary" means: Don't convert line endings, and there is now way to
do a readable diff.
The only thing to tell the user is: The binary blobs are different.

Then we have "text". The "old" version of "text" was "crlf", which
for some people was more intuitive, and less intuitive for others.
"* crlf" is the same as "* text" and means please convert line endings.
And yes, the file is still line oriented.
"* -crlf" means don't touch the line endings, the file is
line-orinted and diff and  merge will work.
"* -text" is the same as "* -crlf"

> 
> Ideas for wording improvements to gitattributes(5) on this subject?

None from me at the moment.

> 
> Thanks,
> Jonathan

Discrepancy between git-check-ignore and git-status for negated pattern

[Guido Flohr]

This .gitignore file does not work as expected with git-check-ignore:

*.o
!a.o

It seems that “git check-ignore a.o” ignores that “a.o” was re-included.  This 
is what happens:

$ git --version
git version 2.15.0.rc2.48.g4e40fb302
$ mkdir repo
$ cd repo
$ git init
Initialized empty Git repository in /path/to/repo/.git/
$ echo '*.o' >.gitignore
$ echo '!a.o' >>.gitignore
$ >a.o
$ >b.o
$ git check-ignore a.o b.o
a.o
b.o
$ git status --short
?? a.o

According to git-check-ignore, “a.o” is ignored.  According to git-status, it 
is not.

Is that a bug or did I miss something because of my limited search capabilities?

Tested on a Mac with current master from git.  The 2.13.4 binary from Mac Ports 
exhibits the same behavior. 

Thanks!

Re: [PATCH 01/13] dir: allow exclusions from blob in addition to file

[Jeff Hostetler]

On 10/25/2017 11:47 PM, Junio C Hamano wrote:

Jeff Hostetler  writes:


The existing code handles use cases where you want to read the
exclusion list from a pathname in the worktree -- or from blob
named in the index when the pathname is not populated (presumably
because of the skip-worktree bit).

I was wanting to add a more general case (and perhaps my commit
message should be improved).  I want to be able to read it from
a blob not necessarily associated with the current commit or
not necessarily available on the local client, but yet known to
exist.


Oh, I understand the above two paragraphs perfectly well, and I
agree with you that such a helper to read from an arbitrary blob is
a worthy thing to have.  I was merely commenting on the fact that
such a helper that is meant to be able to handle more general cases
is not used to help the more specific case that we already have,
which was a bit curious.

I guess the reason why it is not done is (besides expediency)
because the model the new helper operates in would not fit well with
the existing logic flow, where everything is loaded into core
(either from the filesystem or from a blob) and then a common code
parses and registers; the helper wants to do the reading (only) from
the blob, the parsing and the registration all by itself, so there
is not much that can be shared even if the existing code wanted to
reuse what the helper offers.

The new helper mimicks the read_skip_worktree_file_from_index()
codepath to massage the data it reads from the blob to buf[] but not
really (e.g. even though it copies and pastes a lot, it forgets to
call skip_utf8_bom(), for example).  We may still want to see if we
can share more so that we do not have to worry about these tiny
differences between codepaths.


I'm going to extract this commit, refactor it to try to share
more code with the existing read_skip_worktree_file_from_index()
and submit it as a separate patch series so that we can discuss
it in isolation without the rest of the partial-clone code getting
in the way.

The call to skip_utf8_bom() was captured in the new
add_excludes_from_buffer() routine that both add_excludes()
and my new add_excludes_from_blob_to_list() call.




With my "add_excludes_from_blob_to_list()", we can request a
blob-ish expression, such as "master:enlistments/foo".  In my
later commits associated with clone and fetch, we can use this
mechanism to let the client ask the server to filter using the
blob associated with this blob-ish.  If the client has the blob
(such as during a later fetch) and can resolve it, then it can
and send the server the OID, but it can also send the blob-ish
to the server and let it resolve it.


Security-minded people may want to keep an eye or two open for these
later patches---extended SHA-1 expressions is a new attack surface
we would want to carefully polish and protect.

[PATCH] dir: allow exclusions from blob in addition to file

[Jeff Hostetler]

From: Jeff Hostetler 

I pulled commit 01/13 from Tuesday's partial clone part 1 patch series [1]
and refactored the changes in dir.c to try to address Junio's comments in [2]
WRT sharing more code with the existing read_skip_worktree_file_from_index().

This patch can be discussed independently of the partial clone series.

[1] https://public-inbox.org/git/20171024185332.57261-2-...@jeffhostetler.com/
[2] https://public-inbox.org/git/xmqqpo9afu3s@gitster.mtv.corp.google.com/

Jeff Hostetler (1):
  dir: allow exclusions from blob in addition to file

 dir.c | 132 ++
 dir.h |   3 ++
 2 files changed, 104 insertions(+), 31 deletions(-)

-- 
2.9.3

[PATCH] dir: allow exclusions from blob in addition to file

[Jeff Hostetler]

From: Jeff Hostetler 

Refactor add_excludes() to separate the reading of the
exclude file into a buffer and the parsing of the buffer
into exclude_list items.

Add add_excludes_from_blob_to_list() to allow an exclude
file be specified with an OID without assuming a local
worktree or index exists.

Refactor read_skip_worktree_file_from_index() and add
do_read_blob() to eliminate duplication of preliminary
processing of blob contents.

Signed-off-by: Jeff Hostetler 
---
 dir.c | 132 ++
 dir.h |   3 ++
 2 files changed, 104 insertions(+), 31 deletions(-)

diff --git a/dir.c b/dir.c
index 1d17b80..1962374 100644
--- a/dir.c
+++ b/dir.c
@@ -220,6 +220,57 @@ int within_depth(const char *name, int namelen,
return 1;
 }
 
+/*
+ * Read the contents of the blob with the given OID into a buffer.
+ * Append a trailing LF to the end if the last line doesn't have one.
+ *
+ * Returns:
+ *-1 when the OID is invalid or unknown or does not refer to a blob.
+ * 0 when the blob is empty.
+ * 1 along with { data, size } of the (possibly augmented) buffer
+ *   when successful.
+ *
+ * Optionally updates the given sha1_stat with the given OID (when valid).
+ */
+static int do_read_blob(const struct object_id *oid,
+   struct sha1_stat *sha1_stat,
+   size_t *size_out,
+   char **data_out)
+{
+   enum object_type type;
+   unsigned long sz;
+   char *data;
+
+   *size_out = 0;
+   *data_out = NULL;
+
+   data = read_sha1_file(oid->hash, , );
+   if (!data || type != OBJ_BLOB) {
+   free(data);
+   return -1;
+   }
+
+   if (sha1_stat) {
+   memset(_stat->stat, 0, sizeof(sha1_stat->stat));
+   hashcpy(sha1_stat->sha1, oid->hash);
+   }
+
+   if (sz == 0) {
+   free(data);
+   return 0;
+   }
+
+   if (data[sz - 1] != '\n') {
+   data = xrealloc(data, st_add(sz, 1));
+   data[sz++] = '\n';
+   }
+
+   *size_out = xsize_t(sz);
+   *data_out = data;
+
+   return 1;
+}
+
 #define DO_MATCH_EXCLUDE   (1<<0)
 #define DO_MATCH_DIRECTORY (1<<1)
 #define DO_MATCH_SUBMODULE (1<<2)
@@ -600,32 +651,22 @@ void add_exclude(const char *string, const char *base,
x->el = el;
 }
 
-static void *read_skip_worktree_file_from_index(const struct index_state 
*istate,
-   const char *path, size_t *size,
-   struct sha1_stat *sha1_stat)
+static int read_skip_worktree_file_from_index(const struct index_state *istate,
+ const char *path,
+ size_t *size_out,
+ char **data_out,
+ struct sha1_stat *sha1_stat)
 {
int pos, len;
-   unsigned long sz;
-   enum object_type type;
-   void *data;
 
len = strlen(path);
pos = index_name_pos(istate, path, len);
if (pos < 0)
-   return NULL;
+   return -1;
if (!ce_skip_worktree(istate->cache[pos]))
-   return NULL;
-   data = read_sha1_file(istate->cache[pos]->oid.hash, , );
-   if (!data || type != OBJ_BLOB) {
-   free(data);
-   return NULL;
-   }
-   *size = xsize_t(sz);
-   if (sha1_stat) {
-   memset(_stat->stat, 0, sizeof(sha1_stat->stat));
-   hashcpy(sha1_stat->sha1, istate->cache[pos]->oid.hash);
-   }
-   return data;
+   return -1;
+
+   return do_read_blob(>cache[pos]->oid, sha1_stat, size_out, 
data_out);
 }
 
 /*
@@ -739,6 +780,10 @@ static void invalidate_directory(struct untracked_cache 
*uc,
dir->dirs[i]->recurse = 0;
 }
 
+static int add_excludes_from_buffer(char *buf, size_t size,
+   const char *base, int baselen,
+   struct exclude_list *el);
+
 /*
  * Given a file with name "fname", read it (either from disk, or from
  * an index if 'istate' is non-null), parse it and store the
@@ -754,9 +799,10 @@ static int add_excludes(const char *fname, const char 
*base, int baselen,
struct sha1_stat *sha1_stat)
 {
struct stat st;
-   int fd, i, lineno = 1;
+   int r;
+   int fd;
size_t size = 0;
-   char *buf, *entry;
+   char *buf;
 
fd = open(fname, O_RDONLY);
if (fd < 0 || fstat(fd, ) < 0) {
@@ -764,17 +810,13 @@ static int add_excludes(const char *fname, const char 
*base, int baselen,
warn_on_fopen_errors(fname);
else
close(fd);
-   if (!istate ||
-   (buf = 

Re: [PATCH 0/2] color-moved: ignore all space changes by default

[Stefan Beller]

On Thu, Oct 26, 2017 at 1:42 AM, Jacob Keller  wrote:

>>
>> Hello,
>>
>> I'm not sure if this is a good default. I think it's not obvious
>> that moved code gets treated differently than regular changes. I
>> wouldn't expect git diff to ignore whitespace changes (without me
>> telling it to) and so when I see moved code I expect they were
>> moved as is.
>>
>> And there are languages where indentation is relevant (e.g.
>> Python, YAML) and as color-moved is also treated as review tool
>> to detect unwanted changes this new default can be dangerous.

That makes sense.

>>
>> The new options sound like a good addition but I don't think the
>> defaults should change. However unrelated to this decision,
>> please add config settings in addition to these new options so
>> users can globally configure the behavior they want.
>>
>> Regards
>> Simon
>> --
>
> Even languages which are indentation sensitive often move blocks of
> lines between indentation levels a lot. I personally think the default
> could change.

A safe default for such languages would be when the
change in whitespace across lines is taking into account, i.e.
when lots of lines are copied, but all of them miss two tab indentation,
then it is probably fine to color it all the same. However if there would
be a couple of lines in that moved block of code that have
a different indentation than most other lines, this could indeed
change the program flow in python.

So ideally we'd compare the whitespace delta across lines.
Note sure if that is easy. Maybe instead of ignoring whitespaces
completely we'd rather make up a "white space delta", e.g. by using
word-diff between the old and new line, and then keeping a hash of that
space delta for each line. The move detection would then also compare
the hashes of adjacent moved lines.

> However, I would suspect the best path forward is leave the default
> "exact match" and allow users who care and know about the feature to
> change their config settings.

I think that is what I'll do for now as it seems simple enough.

Thanks,
Stefan

Re: grep vs git grep performance?

[Stefan Beller]

On Thu, Oct 26, 2017 at 10:41 AM, Joe Perches  wrote:
> On Thu, 2017-10-26 at 09:58 -0700, Stefan Beller wrote:
>> + Avar who knows a thing about pcre (I assume the regex compilation
>> has impact on grep speed)
>>
>> On Thu, Oct 26, 2017 at 8:02 AM, Joe Perches  wrote:
>> > Comparing a cache warm git grep vs command line grep
>> > shows significant differences in cpu & wall clock.
>> >
>> > Any ideas how to improve this?
>> >
>> > $ time git grep "\bseq_.*%p\W" | wc -l
>> > 112
>> >
>> > real0m4.271s
>> > user0m15.520s
>> > sys 0m0.395s
>> >
>> > $ time grep -r --include=*.[ch] "\bseq_.*%p\W" * | wc -l
>> > 112
>> >
>> > real0m1.164s
>> > user0m0.847s
>> > sys 0m0.314s
>> >
>>
>> I wonder how much is algorithmic advantage vs coding/micro
>> optimization that we can do.
>
> As do I.  I presume this is libpcre related.
>
> For instance, git grep performance is better than grep for:
>
> $ time git grep -w "seq_printf" -- "*.[ch]" | wc -l
> 8609
>
> real0m0.301s
> user0m0.548s
> sys 0m0.372s
>
> $ time grep -w -r --include=*.[ch] "seq_printf" * | wc -l
> 8609
>
> real0m0.706s
> user0m0.396s
> sys 0m0.309s
>

One important piece of information is what version of Git you are running,


$ git tag --contains origin/ab/pcre-v2
v2.14.0
...

(and the version of pcre, see the numbers)
https://git.kernel.org/pub/scm/git/git.git/commit/?id=94da9193a6eb8f1085d611c04ff8bbb4f5ae1e0a

Re: [PATCH 2/2] diff.c: get rid of duplicate implementation

[René Scharfe]

Am 25.10.2017 um 20:49 schrieb Stefan Beller:
> The implementations in diff.c to detect moved lines needs to compare
> strings and hash strings, which is implemented in that file, as well
> as in the xdiff library.
> 
> Remove the rather recent implementation in diff.c and rely on the well
> exercised code in the xdiff lib.
> 
> With this change the hash used for bucketing the strings for the moved
> line detection changes from FNV32 (that is provided via the hashmaps
> memhash) to DJB2 (which is used internally in xdiff).  Benchmarks found
> on the web[1] do not indicate that these hashes are different in
> performance for readable strings.
> 
> [1] 
> https://softwareengineering.stackexchange.com/questions/49550/which-hashing-algorithm-is-best-for-uniqueness-and-speed

Awesome comparison!  It calls the variant used in libxdiff DJB2a (which
uses xor to mix in the new char) instead of DJB2 (which uses addition).

There's also https://www.strchr.com/hash_functions, which lists DJB2
as Bernstein.  Both functions rank somewhere in the middle of that list.

> Signed-off-by: Stefan Beller 
> ---
>   diff.c | 82 
> --
>   1 file changed, 4 insertions(+), 78 deletions(-)

Very nice.

René

Re: grep vs git grep performance?

[Joe Perches]

On Thu, 2017-10-26 at 09:58 -0700, Stefan Beller wrote:
> + Avar who knows a thing about pcre (I assume the regex compilation
> has impact on grep speed)
> 
> On Thu, Oct 26, 2017 at 8:02 AM, Joe Perches  wrote:
> > Comparing a cache warm git grep vs command line grep
> > shows significant differences in cpu & wall clock.
> > 
> > Any ideas how to improve this?
> > 
> > $ time git grep "\bseq_.*%p\W" | wc -l
> > 112
> > 
> > real0m4.271s
> > user0m15.520s
> > sys 0m0.395s
> > 
> > $ time grep -r --include=*.[ch] "\bseq_.*%p\W" * | wc -l
> > 112
> > 
> > real0m1.164s
> > user0m0.847s
> > sys 0m0.314s
> > 
> 
> I wonder how much is algorithmic advantage vs coding/micro
> optimization that we can do.

As do I.  I presume this is libpcre related.

For instance, git grep performance is better than grep for:

$ time git grep -w "seq_printf" -- "*.[ch]" | wc -l
8609

real0m0.301s
user0m0.548s
sys 0m0.372s

$ time grep -w -r --include=*.[ch] "seq_printf" * | wc -l
8609

real0m0.706s
user0m0.396s
sys 0m0.309s

Re: [PATCH] docs: fix formatting of rev-parse's --show-superproject-working-tree

[Stefan Beller]

On Thu, Oct 26, 2017 at 4:53 AM, Sebastian Schuberth
 wrote:
> Signed-off-by: Sebastian Schuberth 
> ---
>  Documentation/git-rev-parse.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index 0917b8207b9d6..95326b85ff68e 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -264,7 +264,7 @@ print a message to stderr and exit with nonzero status.
>  --show-toplevel::
> Show the absolute path of the top-level directory.
>
> ---show-superproject-working-tree
> +--show-superproject-working-tree::

Thanks for this fix!
Stefan

Re: [PATCH 1/2] xdiff-interface: export comparing and hashing strings

[René Scharfe]

Am 25.10.2017 um 20:49 schrieb Stefan Beller:
> +/*
> + * Compare the strings l1 with l2 which are of size s1 and s2 respectively.
> + * Returns 1 if the strings are deemed equal, 0 otherwise.
> + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces
> + * are treated for the comparision.
> + */
> +extern int xdiff_compare_lines(const char *l1, long s1,
> +const char *l2, long s2, long flags);

With the added comment it's OK here.

Still, I find the tendency in libxdiff to use the shortest possible
variable names to be hard on the eyes.  That math-like notation may have
its place in that external library, but I think we should be careful
lest it spreads.

René

Re: [PATCH 2/4] xdiff-interface: export comparing and hashing strings

[René Scharfe]

Am 24.10.2017 um 22:42 schrieb Stefan Beller:
> On Tue, Oct 24, 2017 at 1:23 PM, René Scharfe  wrote:
> 
>> xdl_recmatch() is already exported; why not use it without this
>> wrapper?
> 
> It is exported in xdiff/xutils.h, to be used by various xdiff/*.c files, but
> not outside of xdiff/. This one makes it available to the outside, too.

Ah, right, somehow I mixed that up with xdiff/xdiff.h, which is already
included by two builtins.

René

Re: grep vs git grep performance?

[Stefan Beller]

+ Avar who knows a thing about pcre (I assume the regex compilation
has impact on grep speed)

On Thu, Oct 26, 2017 at 8:02 AM, Joe Perches  wrote:
> Comparing a cache warm git grep vs command line grep
> shows significant differences in cpu & wall clock.
>
> Any ideas how to improve this?
>
> $ time git grep "\bseq_.*%p\W" | wc -l
> 112
>
> real0m4.271s
> user0m15.520s
> sys 0m0.395s
>
> $ time grep -r --include=*.[ch] "\bseq_.*%p\W" * | wc -l
> 112
>
> real0m1.164s
> user0m0.847s
> sys 0m0.314s
>

I wonder how much is algorithmic advantage vs coding/micro
optimization that we can do.

Re: What's cooking in git.git (Oct 2017, #05; Tue, 24)

[Ramsay Jones]

On 24/10/17 06:28, Junio C Hamano wrote:
[snip]> 
> * tg/deprecate-stash-save (2017-10-23) 3 commits
>  - stash: remove now superfluos help for "stash push"
>  - mark git stash push deprecated in the man page
>  - replace git stash save with git stash push in the documentation
> 
>  "git stash save" has been deprecated in favour of "git stash push".
> 
>  Will merge to 'next'.

If you don't intend to include this in v2.15.0, when re-building
the next branch after release (the above is now in 'next'), could
we please remember to update one of the commit message subject line.

In particular, commit 742d6ce35b ("mark git stash push deprecated
in the man page", 22-10-2017), is marking 'git stash *save*' as
deprecated, not *push*.

[Sorry for not spotting this earlier; I only noticed when doing an
'git log --oneline' display which, naturally, puts focus on the
subject lines. ;-) ]

ATB,
Ramsay Jones

Re: grep vs git grep performance?

[Joe Perches]

On Thu, 2017-10-26 at 18:13 +0200, SZEDER Gábor wrote:
> > Comparing a cache warm git grep vs command line grep
> > shows significant differences in cpu & wall clock.
> > 
> > Any ideas how to improve this?
> > 
> > $ time git grep "\bseq_.*%p\W" | wc -l
> > 112
> > 
> > real0m4.271s
> > user0m15.520s
> > sys 0m0.395s
> > 
> > $ time grep -r --include=*.[ch] "\bseq_.*%p\W" * | wc -l
> > 112
> > 
> > real0m1.164s
> > user0m0.847s
> > sys 0m0.314s
> 
> Note that this "regular" grep is limited to *.c and *.h files, while
> the above git grep invocation isn't and has to look at all tracked
> files.  How does
> 
>   git grep "\bseq_.*%p\W" "*.[ch]"
> 
> fare?

Same-ish

$ time git grep "\bseq_.*%p\W" -- "*.[ch]" | wc -l
112

real0m4.225s
user0m14.485s
sys 0m0.413s

Re: grep vs git grep performance?

[SZEDER Gábor]

> Comparing a cache warm git grep vs command line grep
> shows significant differences in cpu & wall clock.
> 
> Any ideas how to improve this?
> 
> $ time git grep "\bseq_.*%p\W" | wc -l
> 112
> 
> real  0m4.271s
> user  0m15.520s
> sys   0m0.395s
> 
> $ time grep -r --include=*.[ch] "\bseq_.*%p\W" * | wc -l
> 112
> 
> real  0m1.164s
> user  0m0.847s
> sys   0m0.314s

Note that this "regular" grep is limited to *.c and *.h files, while
the above git grep invocation isn't and has to look at all tracked
files.  How does

  git grep "\bseq_.*%p\W" "*.[ch]"

fare?

[no subject]

[Financial Pvt Ltd]

We can help you with a genuine loan to meet your needs.Do you need a personal 
or business loan without stress andQuick approval?Do you need an urgent loan 
today? No Credit Checks* LOAN APPROVAL IN 60MINS !!* GUARANTEED SAME DAY 
TRANSFER !!* 100% APPROVAL RATE !!* LOW INTEREST RATE !!RegardsMrs. Hijab 
Mohammed

Re: grep vs git grep performance?

[Joe Perches]

On Thu, 2017-10-26 at 17:11 +0200, Han-Wen Nienhuys wrote:
> On Thu, Oct 26, 2017 at 5:02 PM, Joe Perches  wrote:
> > Comparing a cache warm git grep vs command line grep
> > shows significant differences in cpu & wall clock.
> > 
> > Any ideas how to improve this?
> 
> Is git-grep multithreaded?

Yes, at least according to the documentation

$ git grep --help
[]
   grep.threads
   Number of grep worker threads to use. If unset (or set to 0), 8
   threads are used by default (for now).

> IIRC, grep -r uses multiple threads. (Do
> you have a 4-core machine?)

I have a 2 core machine with hyperthreading

$ cat /proc/cpuinfo
[]
model name  : Intel(R) Core(TM) i5-6200U CPU @ 2.30GHz
stepping: 3
microcode   : 0xba
cpu MHz : 2400.000
cache size  : 3072 KB
physical id : 0
siblings: 4
core id : 0
cpu cores   : 2

Re: [PATCH v2] blame: prevent error if range ends past end of file

[SZEDER Gábor]

> If the -L option is used to specify a line range in git blame, and the
> end of the range is past the end of the file, at present git will fail
> with a fatal error. This commit prevents such behaviour - instead the
> blame is display for any existing lines within the specified range.

s/is display/is displayed/ ?

'git log' has a very similar -L option, which errors out, too, if the
end of the line range is past the end of the file.  IMHO the
interpretation of the line range -L, should be kept
consistent in the two commands, and 'git log' shouldn't error out,
either.

> Signed-off-by: Isabella Stephens 
> ---
>  builtin/blame.c   | 4 ++--
>  t/t8003-blame-corner-cases.sh | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 67adaef4d..b5b9db147 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -878,13 +878,13 @@ int cmd_blame(int argc, const char **argv, const char 
> *prefix)
>   nth_line_cb, , lno, anchor,
>   , , sb.path))
>   usage(blame_usage);
> - if (lno < top || ((lno || bottom) && lno < bottom))
> + if ((lno || bottom) && lno < bottom)
>   die(Q_("file %s has only %lu line",
>  "file %s has only %lu lines",
>  lno), path, lno);
>   if (bottom < 1)
>   bottom = 1;
> - if (top < 1)
> + if (top < 1 || lno < top)
>   top = lno;
>   bottom--;
>   range_set_append_unsafe(, bottom, top);
> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index 661f9d430..32b3788fe 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -216,8 +216,9 @@ test_expect_success 'blame -L with invalid start' '
>  '
>  
>  test_expect_success 'blame -L with invalid end' '
> - test_must_fail git blame -L1,5 tres 2>errors &&
> - test_i18ngrep "has only 2 lines" errors
> + git blame -L1,5 tres >out &&
> + cat out &&
> + test $(wc -l < out) -eq 2

Please use the test_line_count helper instead, as it will output a
helpful error message on failure, making the 'cat out' above
unnecessary.

>  '
>  
>  test_expect_success 'blame parses  part of -L' '
> -- 
> 2.14.1
> 
> 

[PATCH] rev-list-options.txt: use correct directional reference

[SZEDER Gábor]

The descriptions of the options '--parents', '--children' and
'--graph' say "see 'History Simplification' below", although the
referred section is in fact above the description of these options.

Send readers in the right direction by saying "above" instead of
"below".

Signed-off-by: SZEDER Gábor 
---
 Documentation/rev-list-options.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index 7d860bfca..13501e155 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -799,11 +799,11 @@ endif::git-rev-list[]
 
 --parents::
Print also the parents of the commit (in the form "commit parent...").
-   Also enables parent rewriting, see 'History Simplification' below.
+   Also enables parent rewriting, see 'History Simplification' above.
 
 --children::
Print also the children of the commit (in the form "commit child...").
-   Also enables parent rewriting, see 'History Simplification' below.
+   Also enables parent rewriting, see 'History Simplification' above.
 
 ifdef::git-rev-list[]
 --timestamp::
@@ -846,7 +846,7 @@ you would get an output like this:
to be drawn properly.
Cannot be combined with `--no-walk`.
 +
-This enables parent rewriting, see 'History Simplification' below.
+This enables parent rewriting, see 'History Simplification' above.
 +
 This implies the `--topo-order` option by default, but the
 `--date-order` option may also be specified.
-- 
2.15.0.rc2.80.g094badb02

Re: grep vs git grep performance?

[Han-Wen Nienhuys]

On Thu, Oct 26, 2017 at 5:02 PM, Joe Perches  wrote:
> Comparing a cache warm git grep vs command line grep
> shows significant differences in cpu & wall clock.
>
> Any ideas how to improve this?

Is git-grep multithreaded? IIRC, grep -r uses multiple threads. (Do
you have a 4-core machine?)

--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

grep vs git grep performance?

[Joe Perches]

Comparing a cache warm git grep vs command line grep
shows significant differences in cpu & wall clock.

Any ideas how to improve this?

$ time git grep "\bseq_.*%p\W" | wc -l
112

real0m4.271s
user0m15.520s
sys 0m0.395s

$ time grep -r --include=*.[ch] "\bseq_.*%p\W" * | wc -l
112

real0m1.164s
user0m0.847s
sys 0m0.314s

RE:

[John Galvan]

I have a business proposal 15,500,000 GBP.

Felicitări

[Euromillionen / Google Promo]

Felicitări ați câștigat de 650.000€.00 Milioane de Euro/Google Promo 
extrageri lunare a avut loc Pe 1 octombrie 2017.

Contactați pretinde agent de e-Mail: janosiklubos...@gmail.com
1. Nume complet:
2. Adresa:
3. Sex:
4. Varsta:
5. Ocupația:
6. Telefon:

Robert Avtandiltayn
Online coordonatorul

[PATCH] docs: fix formatting of rev-parse's --show-superproject-working-tree

[Sebastian Schuberth]

Signed-off-by: Sebastian Schuberth 
---
 Documentation/git-rev-parse.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 0917b8207b9d6..95326b85ff68e 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -264,7 +264,7 @@ print a message to stderr and exit with nonzero status.
 --show-toplevel::
Show the absolute path of the top-level directory.
 
---show-superproject-working-tree
+--show-superproject-working-tree::
Show the absolute path of the root of the superproject's
working tree (if exists) that uses the current repository as
its submodule.  Outputs nothing if the current repository is

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

Re: Consequences of CRLF in index?

[Lars Schneider]

> On 25 Oct 2017, at 19:13, Jonathan Nieder  wrote:
> 
> Hi again,
> 
> Lars Schneider wrote:
>>> On 24 Oct 2017, at 20:14, Jonathan Nieder  wrote:
> 
>>> In any event, you also probably want to declare what you're doing
>>> using .gitattributes.  By checking in the files as CRLF, you are
>>> declaring that you do *not* want Git to treat them as text files
>>> (i.e., you do not want Git to change the line endings), so something
>>> as simple as
>>> 
>>> * -text
>> 
>> That's sounds good. Does "-text" have any other implications?
>> For whatever reason I always thought this is the way to tell
>> Git that a particular file is binary with the implication that
>> Git should not attempt to diff it.
> 
> No other implications.  You're thinking of "-diff".  There is also a
> shortcut "binary" which simply means "-text -diff".

Yeah. Well, when I read "-text" then I think "no text" and that makes
me think "is binary".


> Ideas for wording improvements to gitattributes(5) on this subject?

I think the wording in the docs is good. It is just the "text" keyword
that confused me. Maybe this could have been names "eolnorm" and
"-eolnorm" or something. But it is too late for that now I guess :-)

Thanks,
Lars

Re: Consequences of CRLF in index?

[Lars Schneider]

> On 26 Oct 2017, at 09:09, Johannes Sixt  wrote:
> 
> Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:
>> I envy you for the blessing of such a clean C++ source that you do not
>> have any, say, Unix shell script in it. Try this, and weep:
>>  $ printf 'echo \\\r\n\t123\r\n' >a1
>>  $ sh a1
>>  a1: 2: a1: 123: not found
> 
> I was bitten by that, too. For this reason, I ensure that shell scripts and 
> Makefiles begin their life on Linux. Fortunately, modern editors on Windows, 
> includ^Wand vi, do not force CRLF line breaks, and such files can be edited 
> on Windows, too.

Wouldn't this kind of .gitattributes setup solve the problem?

* -text
*.sh   text eol=lf

Thanks,
Lars

Please read carefully and get back to me,

[Mrs AzeezahLawala]

-- 
Assalamu Alaikum.

I am Mrs Azeezah Lawal from France, I am 58 years old woman
suffering from Endometrial cancer and I live in west Africa, please i
want you help me create a charitable project with the money that I
inherited from my deceased husband, I crave your indulgence as a God
fearing individual and as someone who cares for the less-privileged as
much as i do, please take it upon yourself and use this fund for the
above mentioned purposes, As soon as I receive your reply and personal
information's as listed below, I shall give you the official contact
of the officials as well as my lawyer to enable you contact the Bank
without delays and i will also issue you with a letter of
authorization for proof, kindly contact me through my private email
address (azeezahlawa...@gmail.com)

Regards.
Mrs Azeezah Lawal.
Please reply to my private Email:azeezahlawa...@gmail.com

Re: [PATCH 0/6] Create Git/Packet.pm

[Bryan Turner]

On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller  wrote:
> On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano  wrote:
>> Johannes Schindelin  writes:
>>
>>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
>>> the line
>>>
>>>   use lib (split(/:/, $ENV{GITPERLLIB}));
>>>
>>> splits off the drive letter from the rest of the path. Obviously, this
>>> fails to Do The Right Thing, and simply points to Yet Another Portability
>>> Problem with Git's reliance on Unix scripting.
>>
>> In our C code, we have "#define PATH_SEP ';'", and encourage our
>> code to be careful and use it.  Is there something similar for Perl
>> scripts, I wonder.
>>
>> I notice that t/{t0202,t9000,t9700}/test.pl share the same
>> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
>> the non-platform convention to accomodate the use of split(/:/)
>> certainly is a workaround, but it does feel dirty.
>>
>> It is hard to imagine that we were the first people who wants to
>> split the value of a variable into a list, where the value is a list
>> of paths, concatenated into a single string with a delimiter that
>> may be platform specific.  I wonder if we are going against a best
>> practice established in the Perl world, simply because we don't know
>> about it (i.e. basically, it would say "don't split at a colon
>> because not all world is Unix; use $this_module instead", similar to
>> "don't split at a slash, use File::Spec instead to extract path
>> components").
>>
>
> I thought there was a way to do this in File::Spec, but that's only
> for splitting regular paths, and not for splitting a list of paths
> separated by ":" or ";"
>
> We probably should find a better solution to allow this to work with
> windows style paths...? I know that python provides os.pathsep, but I
> haven't seen an equivalent for perl yet.
>
> The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
> maybe we should be using this?

I was testing this recently on the Perl included with Git for Windows
and it returns : for the path separator even though it's on Windows,
so I don't think that would work. The Perl in Git for Windows seems to
want UNIX-style inputs (something Dscho seemed to allude to in his
response earlier.). I'm not sure why it's that way, but he probably
knows.

Bryan

(Pardon my previous blank message; Gmail fail.)

>
> Thanks,
> Jake
>
> [1] https://perldoc.perl.org/Env.html
> [2] https://perldoc.perl.org/Config.html

Re: [PATCH 0/6] Create Git/Packet.pm

[Bryan Turner]

On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller  wrote:
> On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano  wrote:
>> Johannes Schindelin  writes:
>>
>>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
>>> the line
>>>
>>>   use lib (split(/:/, $ENV{GITPERLLIB}));
>>>
>>> splits off the drive letter from the rest of the path. Obviously, this
>>> fails to Do The Right Thing, and simply points to Yet Another Portability
>>> Problem with Git's reliance on Unix scripting.
>>
>> In our C code, we have "#define PATH_SEP ';'", and encourage our
>> code to be careful and use it.  Is there something similar for Perl
>> scripts, I wonder.
>>
>> I notice that t/{t0202,t9000,t9700}/test.pl share the same
>> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
>> the non-platform convention to accomodate the use of split(/:/)
>> certainly is a workaround, but it does feel dirty.
>>
>> It is hard to imagine that we were the first people who wants to
>> split the value of a variable into a list, where the value is a list
>> of paths, concatenated into a single string with a delimiter that
>> may be platform specific.  I wonder if we are going against a best
>> practice established in the Perl world, simply because we don't know
>> about it (i.e. basically, it would say "don't split at a colon
>> because not all world is Unix; use $this_module instead", similar to
>> "don't split at a slash, use File::Spec instead to extract path
>> components").
>>
>
> I thought there was a way to do this in File::Spec, but that's only
> for splitting regular paths, and not for splitting a list of paths
> separated by ":" or ";"
>
> We probably should find a better solution to allow this to work with
> windows style paths...? I know that python provides os.pathsep, but I
> haven't seen an equivalent for perl yet.
>
> The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
> maybe we should be using this?
>
> Thanks,
> Jake
>
> [1] https://perldoc.perl.org/Env.html
> [2] https://perldoc.perl.org/Config.html

Re: [PATCH 0/6] Create Git/Packet.pm

[Jacob Keller]

On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano  wrote:
> Johannes Schindelin  writes:
>
>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
>> the line
>>
>>   use lib (split(/:/, $ENV{GITPERLLIB}));
>>
>> splits off the drive letter from the rest of the path. Obviously, this
>> fails to Do The Right Thing, and simply points to Yet Another Portability
>> Problem with Git's reliance on Unix scripting.
>
> In our C code, we have "#define PATH_SEP ';'", and encourage our
> code to be careful and use it.  Is there something similar for Perl
> scripts, I wonder.
>
> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> the non-platform convention to accomodate the use of split(/:/)
> certainly is a workaround, but it does feel dirty.
>
> It is hard to imagine that we were the first people who wants to
> split the value of a variable into a list, where the value is a list
> of paths, concatenated into a single string with a delimiter that
> may be platform specific.  I wonder if we are going against a best
> practice established in the Perl world, simply because we don't know
> about it (i.e. basically, it would say "don't split at a colon
> because not all world is Unix; use $this_module instead", similar to
> "don't split at a slash, use File::Spec instead to extract path
> components").
>

I thought there was a way to do this in File::Spec, but that's only
for splitting regular paths, and not for splitting a list of paths
separated by ":" or ";"

We probably should find a better solution to allow this to work with
windows style paths...? I know that python provides os.pathsep, but I
haven't seen an equivalent for perl yet.

The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
maybe we should be using this?

Thanks,
Jake

[1] https://perldoc.perl.org/Env.html
[2] https://perldoc.perl.org/Config.html

Re: [PATCH v2] blame: prevent error if range ends past end of file

[Jacob Keller]

On Thu, Oct 26, 2017 at 12:01 AM, Isabella Stephens
 wrote:
> If the -L option is used to specify a line range in git blame, and the
> end of the range is past the end of the file, at present git will fail
> with a fatal error. This commit prevents such behaviour - instead the
> blame is display for any existing lines within the specified range.
>
> Signed-off-by: Isabella Stephens 
> ---

I like this change. We might want to document L to indicate that an L
that is outside the range of lines will show all lines that do match.

Maybe we also want it to only succeed if at least some lines are
blamed? Could we make it so that it fails if no lines are within the
range? (ie: the start point is too far in? or does it already do
such?)

Thanks,
Jake

>  builtin/blame.c   | 4 ++--
>  t/t8003-blame-corner-cases.sh | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 67adaef4d..b5b9db147 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -878,13 +878,13 @@ int cmd_blame(int argc, const char **argv, const char 
> *prefix)
> nth_line_cb, , lno, anchor,
> , , sb.path))
> usage(blame_usage);
> -   if (lno < top || ((lno || bottom) && lno < bottom))
> +   if ((lno || bottom) && lno < bottom)
> die(Q_("file %s has only %lu line",
>"file %s has only %lu lines",
>lno), path, lno);
> if (bottom < 1)
> bottom = 1;
> -   if (top < 1)
> +   if (top < 1 || lno < top)
> top = lno;
> bottom--;
> range_set_append_unsafe(, bottom, top);
> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index 661f9d430..32b3788fe 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -216,8 +216,9 @@ test_expect_success 'blame -L with invalid start' '
>  '
>
>  test_expect_success 'blame -L with invalid end' '
> -   test_must_fail git blame -L1,5 tres 2>errors &&
> -   test_i18ngrep "has only 2 lines" errors
> +   git blame -L1,5 tres >out &&
> +   cat out &&
> +   test $(wc -l < out) -eq 2
>  '
>
>  test_expect_success 'blame parses  part of -L' '
> --
> 2.14.1
>

Re: [PATCH] t0000: check whether the shell supports the "local" keyword

[Michael Haggerty]

On 10/26/2017 10:40 AM, Jacob Keller wrote:
> On Thu, Oct 26, 2017 at 1:28 AM, Eric Sunshine  
> wrote:
>> On Thu, Oct 26, 2017 at 4:18 AM, Michael Haggerty  
>> wrote:
>>> Add a test balloon to see if we get complaints from anybody who is
>>> using a shell that doesn't support the "local" keyword. If so, this
>>> test can be reverted. If not, we might want to consider using "local"
>>> in shell code throughout the git code base.
>>
>> I would guess that the number of people who actually run the Git test
>> suite is microscopic compared to the number of people who use Git
>> itself. It is not clear, therefore, that lack of reports of failure of
>> the new test would imply that "local" can safely be used throughout
>> the Git code base. At best, it might indicate that "local" can be used
>> in the tests.
>>
>> Or, am I missing something?
>>
> 
> I don't think you're missing anything. I think the idea here is: "do
> any users who actively run the test suite care if we start using
> local". I don't think the goal is to allow use of local in non-test
> suite code. At least, that's not how I interpreted it.
> 
> Thus it's fine to be only as part of a test and see if anyone
> complains, since the only people affected would be those which
> actually run the test suite...
> 
> Changing our requirement for regular shell scripts we ship seems a lot
> trickier to gauge.

Actually, I would hope that if this experiment is successful that we can
use "local" in production code, too.

The proper question isn't "what fraction of Git users run the test
suite?", because I agree with Eric that that is microscopic. The correct
question is "on what fraction of platforms where Git will be run has the
test suite been run by *somebody*?", and I think (I hope!) that that
fraction is quite high.

Really...if you are compiling Git on a platform that is so deviant or
archaic that it doesn't have a reasonable Shell, and you don't even
bother running the test suite, you kindof deserve your fate, don't you?

Michael

Re: [PATCH 0/2] color-moved: ignore all space changes by default

[Jacob Keller]

On Thu, Oct 26, 2017 at 12:22 AM, Simon Ruderich  wrote:
> On Wed, Oct 25, 2017 at 03:46:18PM -0700, Stefan Beller wrote:
>> On Mon, Oct 23, 2017 at 7:52 PM, Stefan Beller wrote[1]:
>>> On Mon, Oct 23, 2017 at 6:54 PM, Junio C Hamano wrote:

  * As moved-lines display is mostly a presentation thing, I wonder
if it makes sense to always match loosely wrt whitespace
differences.
>>>
>>> Well, sometimes the user wants to know if it is byte-for-byte identical
>>> (unlikely to be code, but maybe column oriented data for input;
>>> think of all our FORTRAN users. ;)
>>
>> ... and this is the implementation and the flip of the default setting
>> to ignore all white space for the move detection.
>
> Hello,
>
> I'm not sure if this is a good default. I think it's not obvious
> that moved code gets treated differently than regular changes. I
> wouldn't expect git diff to ignore whitespace changes (without me
> telling it to) and so when I see moved code I expect they were
> moved as is.
>
> And there are languages where indentation is relevant (e.g.
> Python, YAML) and as color-moved is also treated as review tool
> to detect unwanted changes this new default can be dangerous.
>
> The new options sound like a good addition but I don't think the
> defaults should change. However unrelated to this decision,
> please add config settings in addition to these new options so
> users can globally configure the behavior they want.
>
> Regards
> Simon
> --

Even languages which are indentation sensitive often move blocks of
lines between indentation levels a lot. I personally think the default
could change.

However, I would suspect the best path forward is leave the default
"exact match" and allow users who care and know about the feature to
change their config settings.

Thanks,
Jake

Re: [PATCH] t0000: check whether the shell supports the "local" keyword

[Jacob Keller]

On Thu, Oct 26, 2017 at 1:28 AM, Eric Sunshine  wrote:
> On Thu, Oct 26, 2017 at 4:18 AM, Michael Haggerty  
> wrote:
>> Add a test balloon to see if we get complaints from anybody who is
>> using a shell that doesn't support the "local" keyword. If so, this
>> test can be reverted. If not, we might want to consider using "local"
>> in shell code throughout the git code base.
>
> I would guess that the number of people who actually run the Git test
> suite is microscopic compared to the number of people who use Git
> itself. It is not clear, therefore, that lack of reports of failure of
> the new test would imply that "local" can safely be used throughout
> the Git code base. At best, it might indicate that "local" can be used
> in the tests.
>
> Or, am I missing something?
>

I don't think you're missing anything. I think the idea here is: "do
any users who actively run the test suite care if we start using
local". I don't think the goal is to allow use of local in non-test
suite code. At least, that's not how I interpreted it.

Thus it's fine to be only as part of a test and see if anyone
complains, since the only people affected would be those which
actually run the test suite...

Changing our requirement for regular shell scripts we ship seems a lot
trickier to gauge.

Thanks,
Jake

Re: [PATCH] t0000: check whether the shell supports the "local" keyword

[Eric Sunshine]

On Thu, Oct 26, 2017 at 4:18 AM, Michael Haggerty  wrote:
> Add a test balloon to see if we get complaints from anybody who is
> using a shell that doesn't support the "local" keyword. If so, this
> test can be reverted. If not, we might want to consider using "local"
> in shell code throughout the git code base.

I would guess that the number of people who actually run the Git test
suite is microscopic compared to the number of people who use Git
itself. It is not clear, therefore, that lack of reports of failure of
the new test would imply that "local" can safely be used throughout
the Git code base. At best, it might indicate that "local" can be used
in the tests.

Or, am I missing something?

> Signed-off-by: Michael Haggerty 
> ---
> This has been discussed on the mailing list [1,2].
>
> Michael
>
> [1] 
> https://public-inbox.org/git/CAPig+cRLB=dGD=+Af=yyl3m709lrpeurtvcdlo9ibkyy2ha...@mail.gmail.com/
> [2] https://public-inbox.org/git/20160601163747.ga10...@sigill.intra.peff.net/
>
>  t/t-basic.sh | 25 +
>  1 file changed, 25 insertions(+)
>
> diff --git a/t/t-basic.sh b/t/t-basic.sh
> index 1aa5093f36..7fd87dd544 100755
> --- a/t/t-basic.sh
> +++ b/t/t-basic.sh
> @@ -20,6 +20,31 @@ modification *should* take notice and update the test 
> vectors here.
>
>  . ./test-lib.sh
>
> +try_local_x () {
> +   local x="local" &&
> +   echo "$x"
> +}
> +
> +# This test is an experiment to check whether any Git users are using
> +# Shells that don't support the "local" keyword. "local" is not
> +# POSIX-standard, but it is very widely supported by POSIX-compliant
> +# shells, and if it doesn't cause problems for people, we would like
> +# to be able to use it in Git code.
> +#
> +# For now, this is the only test that requires "local". If your shell
> +# fails this test, you can ignore the failure, but please report the
> +# problem to the Git mailing list , as it might
> +# convince us to continue avoiding the use of "local".
> +test_expect_success 'verify that the running shell supports "local"' '
> +   x="notlocal" &&
> +   echo "local" >expected1 &&
> +   try_local_x >actual1 &&
> +   test_cmp expected1 actual1 &&
> +   echo "notlocal" >expected2 &&
> +   echo "$x" >actual2 &&
> +   test_cmp expected2 actual2
> +'
> +
>  
>  # git init has been done in an empty repository.
>  # make sure it is empty.
> --
> 2.14.1

[PATCH] t0000: check whether the shell supports the "local" keyword

[Michael Haggerty]

Add a test balloon to see if we get complaints from anybody who is
using a shell that doesn't support the "local" keyword. If so, this
test can be reverted. If not, we might want to consider using "local"
in shell code throughout the git code base.

Signed-off-by: Michael Haggerty 
---
This has been discussed on the mailing list [1,2].

Michael

[1] 
https://public-inbox.org/git/CAPig+cRLB=dGD=+Af=yyl3m709lrpeurtvcdlo9ibkyy2ha...@mail.gmail.com/
[2] https://public-inbox.org/git/20160601163747.ga10...@sigill.intra.peff.net/

 t/t-basic.sh | 25 +
 1 file changed, 25 insertions(+)

diff --git a/t/t-basic.sh b/t/t-basic.sh
index 1aa5093f36..7fd87dd544 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -20,6 +20,31 @@ modification *should* take notice and update the test 
vectors here.
 
 . ./test-lib.sh
 
+try_local_x () {
+   local x="local" &&
+   echo "$x"
+}
+
+# This test is an experiment to check whether any Git users are using
+# Shells that don't support the "local" keyword. "local" is not
+# POSIX-standard, but it is very widely supported by POSIX-compliant
+# shells, and if it doesn't cause problems for people, we would like
+# to be able to use it in Git code.
+#
+# For now, this is the only test that requires "local". If your shell
+# fails this test, you can ignore the failure, but please report the
+# problem to the Git mailing list , as it might
+# convince us to continue avoiding the use of "local".
+test_expect_success 'verify that the running shell supports "local"' '
+   x="notlocal" &&
+   echo "local" >expected1 &&
+   try_local_x >actual1 &&
+   test_cmp expected1 actual1 &&
+   echo "notlocal" >expected2 &&
+   echo "$x" >actual2 &&
+   test_cmp expected2 actual2
+'
+
 
 # git init has been done in an empty repository.
 # make sure it is empty.
-- 
2.14.1

Re: [PATCH 2/2] files-backend: don't rewrite the `packed-refs` file unnecessarily

[Michael Haggerty]

On 10/26/2017 08:46 AM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> Even when we are deleting references, we needn't overwrite the
>> `packed-refs` file if the references that we are deleting are not
>> present in that file. Implement this optimization as follows:
> 
> This goal I can understand.  files-transaction-prepare may see a
> deletion and in order to avoid a deletion of a ref from the
> file-backend to expose a stale entry in the packed-refs file, it
> prepares a transaction to remove the same ref that might exist in
> it.  If it turns out that there is no such ref in the packed-refs
> file, then we can leave the packed-refs file as-is without
> rewriting.
> 
>> * Add a function `is_packed_transaction_noop()`, which checks whether
>>   a given packed-refs transaction doesn't actually have to do
>>   anything. This function must be called while holding the
>>   `packed-refs` lock to avoid races.
> 
> This checks three things only to cover the most trivial case (I am
> perfectly happy that it punts on more complex cases).  If an update
> 
>  - checks the old value,
> 
>  - sets a new value, or
> 
>  - deletes a ref that is not on the filesystem,
> 
> it is not a trivial case.  I can understand the latter two (i.e. We
> are special casing the deletion, and an update with a new value is
> not.  If removing a file from the filesystem is not sufficient to
> delete, we may have to rewrite the packed-refs), but not the first
> one.  Is it because currently we do not say "delete this ref only
> when its current value is X"?

It wouldn't be too hard to allow updates with REF_HAVE_OLD to be
optimized away too. I didn't do it because

1. We currently only create updates for that packed_refs backend with
REF_HAVE_OLD turned off, so such could would be unreachable (and
untestable).

2. I wanted to keep the patch as simple as possible in case you decide
to merge it into 2.15.

There would also be a little bit of a leveling violation (or maybe the
function name is not ideal). `is_packed_transaction_noop()` could check
that `old_oid` values are correct, and if they all are, declare the
transaction a NOOP. (It wasn't *really* a NOOP, but its OP, namely
checking old values, has already been carried out.) But what if it finds
that an `old_oid` was incorrect? Right now
`is_packed_transaction_noop()` has no way to report something like a
TRANSACTION_GENERIC_ERROR.

It could be that long-term it makes more sense for this optimization to
happen in `packed_transaction_prepare()`. Except that function is
sometimes called for its side-effects, like sorting an existing file, in
which case overwriting the `packed-refs` file shouldn't be optimized away.

So overall it seemed easier to punt on this optimization at this point.

> Also it is not immediately obvious how the "is this noop" helper is
> checking the absence of the same-named ref in the current
> packed-refs file, which is what matters for the correctness of the
> optimization.

The check is in the second loop in `is_packed_transaction_noop()`:

if (!refs_read_raw_ref(ref_store, update->refname,
   oid.hash, , ) ||
errno != ENOENT) {
/*
 * We might have to actually delete that
 * reference -> not a NOOP.
 */
ret = 0;
break;
}

If the refname doesn't exist, then `refs_read_raw_ref()` returns -1 and
sets errno to ENOENT, and the loop continues. Otherwise we exit with a
value of 0, meaning that the transaction is not a NOOP.

There are a lot of double-negatives here. It might be easier to follow
the logic if the sense of the function were inverted:
`is_packed_transaction_needed()`. Let me know if you have a strong
feeling about it.

Michael

Re: [PATCH 0/2] color-moved: ignore all space changes by default

[Simon Ruderich]

On Wed, Oct 25, 2017 at 03:46:18PM -0700, Stefan Beller wrote:
> On Mon, Oct 23, 2017 at 7:52 PM, Stefan Beller wrote[1]:
>> On Mon, Oct 23, 2017 at 6:54 PM, Junio C Hamano wrote:
>>>
>>>  * As moved-lines display is mostly a presentation thing, I wonder
>>>if it makes sense to always match loosely wrt whitespace
>>>differences.
>>
>> Well, sometimes the user wants to know if it is byte-for-byte identical
>> (unlikely to be code, but maybe column oriented data for input;
>> think of all our FORTRAN users. ;)
>
> ... and this is the implementation and the flip of the default setting
> to ignore all white space for the move detection.

Hello,

I'm not sure if this is a good default. I think it's not obvious
that moved code gets treated differently than regular changes. I
wouldn't expect git diff to ignore whitespace changes (without me
telling it to) and so when I see moved code I expect they were
moved as is.

And there are languages where indentation is relevant (e.g.
Python, YAML) and as color-moved is also treated as review tool
to detect unwanted changes this new default can be dangerous.

The new options sound like a good addition but I don't think the
defaults should change. However unrelated to this decision,
please add config settings in addition to these new options so
users can globally configure the behavior they want.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: Consequences of CRLF in index?

[Johannes Sixt]

Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:

I envy you for the blessing of such a clean C++ source that you do not
have any, say, Unix shell script in it. Try this, and weep:

$ printf 'echo \\\r\n\t123\r\n' >a1

$ sh a1

a1: 2: a1: 123: not found


I was bitten by that, too. For this reason, I ensure that shell scripts 
and Makefiles begin their life on Linux. Fortunately, modern editors on 
Windows, includ^Wand vi, do not force CRLF line breaks, and such files 
can be edited on Windows, too.


Of course, I do not set core.autocrlf anywhere to avoid any changes 
behind my back.



For the same reason (Unix shell not handling CR/LF gracefull), I went
through that painful work that finally landed as 00ddc9d13ca (Fix build
with core.autocrlf=true, 2017-05-09).


That's much appreciated!

-- Hannes

Re: [PATCH] blame: add --fuzzy-lines command line option

[Isabella Stephens]

On 26/10/17 5:15 pm, Junio C Hamano wrote:
> Isabella Stephens  writes:
> 
>> If the -L option is used to specify a line range in git blame, and the
>> end of the range is past the end of the file, git will fail with a fatal
>> error. It may instead be desirable to perform a git blame for the line
>> numbers in the intersection of the file and the specified line range.
> 
> Even though erroring out upon such input was done as a deliberate
> design decision, in retrospect, I do not think the design decision
> made much sense.
> 
> The code already takes a nonsense input and tries to make best sense
> of it, e.g. "-L10,6" is interpreted as "-L6,10" instead of erroring
> out.  So if we were to do this kind of change, I suspect that it may
> be better to do so unconditionally without introducing a new option.
> 
> Thanks.
> 

Thanks for following up. I've sent through a version 2 of the patch 
without the command line option.

[PATCH v2] blame: prevent error if range ends past end of file

[Isabella Stephens]

If the -L option is used to specify a line range in git blame, and the
end of the range is past the end of the file, at present git will fail
with a fatal error. This commit prevents such behaviour - instead the
blame is display for any existing lines within the specified range.

Signed-off-by: Isabella Stephens 
---
 builtin/blame.c   | 4 ++--
 t/t8003-blame-corner-cases.sh | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 67adaef4d..b5b9db147 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -878,13 +878,13 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
nth_line_cb, , lno, anchor,
, , sb.path))
usage(blame_usage);
-   if (lno < top || ((lno || bottom) && lno < bottom))
+   if ((lno || bottom) && lno < bottom)
die(Q_("file %s has only %lu line",
   "file %s has only %lu lines",
   lno), path, lno);
if (bottom < 1)
bottom = 1;
-   if (top < 1)
+   if (top < 1 || lno < top)
top = lno;
bottom--;
range_set_append_unsafe(, bottom, top);
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index 661f9d430..32b3788fe 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -216,8 +216,9 @@ test_expect_success 'blame -L with invalid start' '
 '
 
 test_expect_success 'blame -L with invalid end' '
-   test_must_fail git blame -L1,5 tres 2>errors &&
-   test_i18ngrep "has only 2 lines" errors
+   git blame -L1,5 tres >out &&
+   cat out &&
+   test $(wc -l < out) -eq 2
 '
 
 test_expect_success 'blame parses  part of -L' '
-- 
2.14.1

Re: [PATCH 2/2] files-backend: don't rewrite the `packed-refs` file unnecessarily

[Junio C Hamano]

Michael Haggerty  writes:

> Even when we are deleting references, we needn't overwrite the
> `packed-refs` file if the references that we are deleting are not
> present in that file. Implement this optimization as follows:

This goal I can understand.  files-transaction-prepare may see a
deletion and in order to avoid a deletion of a ref from the
file-backend to expose a stale entry in the packed-refs file, it
prepares a transaction to remove the same ref that might exist in
it.  If it turns out that there is no such ref in the packed-refs
file, then we can leave the packed-refs file as-is without
rewriting.

> * Add a function `is_packed_transaction_noop()`, which checks whether
>   a given packed-refs transaction doesn't actually have to do
>   anything. This function must be called while holding the
>   `packed-refs` lock to avoid races.

This checks three things only to cover the most trivial case (I am
perfectly happy that it punts on more complex cases).  If an update

 - checks the old value,

 - sets a new value, or

 - deletes a ref that is not on the filesystem,

it is not a trivial case.  I can understand the latter two (i.e. We
are special casing the deletion, and an update with a new value is
not.  If removing a file from the filesystem is not sufficient to
delete, we may have to rewrite the packed-refs), but not the first
one.  Is it because currently we do not say "delete this ref only
when its current value is X"?

Also it is not immediately obvious how the "is this noop" helper is
checking the absence of the same-named ref in the current
packed-refs file, which is what matters for the correctness of the
optimization.

Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts

[Jeff King]

On Wed, Oct 25, 2017 at 10:03:09AM +0200, Michael Haggerty wrote:

> > Yeah. It's supported by dash and many other shells, but we do try to
> > avoid it[1]. I think in this case we could just drop it (but keep
> > setting the "local foo" ones to empty with "foo=".
> 
> I do wish that we could allow "local", as it avoids a lot of headaches
> and potential breakage. According to [1],

Agreed.

> > However, every single POSIX-compliant shell I've tested implements the
> > 'local' keyword, which lets you declare variables that won't be
> > returned from the current function. So nowadays you can safely count
> > on it working.
> 
> He mentions that ksh93 doesn't support "local", but that it differs from
> POSIX in many other ways, too.

Yes, the conclusion we came to in the thread I linked earlier is the
same: ksh is affected, but that shell is a problem for other reasons. I
don't know if anybody tested with "modern" ksh like mksh, though. Should
be easy enough:

  cat >foo.sh <<\EOF
  fun() {
local x=3
echo inside: $x
  }
  x=2
  fun
  echo after: $x
  EOF

  mksh foo.sh

which produces the expected:

  inside: 3
  after: 2

So that's good.

> Perhaps we could slip in a couple of "local" as a compatibility test to
> see if anybody complains, like we did with a couple of C features recently.

That sounds reasonable to me. But from the earlier conversation, beware
that:

  local x
  ...
  x=5

is not necessarily enough to notice the problem on broken shells (they
may complain that "local" is not a command, and quietly stomp on the
global). I think:

  local x=5

would be enough (though depend on how you use $x, the failure mode might
be pretty subtle). Or we could even add an explicit test in t like
the example above.

> But I agree that this bug fix is not the correct occasion to change
> policy on something like this, so I'll reroll without "local".

Also agreed.

-Peff

N/A

[Maureend David Kaltschmidt]

Claim Donation

Re: [PATCH] blame: add --fuzzy-lines command line option

[Junio C Hamano]

Isabella Stephens  writes:

> If the -L option is used to specify a line range in git blame, and the
> end of the range is past the end of the file, git will fail with a fatal
> error. It may instead be desirable to perform a git blame for the line
> numbers in the intersection of the file and the specified line range.

Even though erroring out upon such input was done as a deliberate
design decision, in retrospect, I do not think the design decision
made much sense.

The code already takes a nonsense input and tries to make best sense
of it, e.g. "-L10,6" is interpreted as "-L6,10" instead of erroring
out.  So if we were to do this kind of change, I suspect that it may
be better to do so unconditionally without introducing a new option.

Thanks.