Re: [PATCH v3] blame: prevent error if range ends past end of file
On Fri, Oct 27, 2017 at 2:18 AM, Isabella Stephens
wrote:
> On 27/10/17 12:58 pm, Junio C Hamano wrote:
>> There should be an "is the range sensible?" check after all the
>> tweaking to bottom and top are done, I think.
>
> My mistake. I missed that case. I think this section of code is a little
> hard to read because it avoids treating an empty file as a special case.
> Why not do something like this:
>
> for (range_i = 0; range_i < range_list.nr; ++range_i) {
> long bottom, top;
> if (!lno)
> die(_("file is empty"));
No need for this conditional to reside within the loop. Hoisting it
outside the loop would (IMO) make the intent even clearer:
if (range_list.nr && !lno)
die(_("file is empty; -L has no effect"));
for (...) {
...
On the other hand, one could argue that "-L," (where comma is the sole
argument to -L), which specifies the entire file, should be allowed
even on an empty file without complaining that the file is empty.
Although it may not seem sensible for a human to specify "-L," perhaps
a tool would do so to override an earlier more restrictive -L range.
However, that may be too esoteric a case to worry about.
> if (parse_range_arg(range_list.items[range_i].string,
> nth_line_cb, &sb, lno, anchor,
> &bottom, &top, sb.path))
> usage(blame_usage);
> if (bottom < 1)
> bottom = 1;
> if (lno < top)
> top = lno;
> if (top < 0 || lno < bottom)
>die(Q_("file %s has only %lu line",
> "file %s has only %lu lines",
> lno), path, lno);
> bottom--;
> range_set_append_unsafe(&ranges, bottom, top);
> anchor = top + 1;
Re: [PATCH v3] blame: prevent error if range ends past end of file
On 27/10/17 12:58 pm, Junio C Hamano wrote:
> 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, &sb, lno, anchor,
>> &bottom, &top, 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.
My mistake. I missed that case. I think this section of code is a little
hard to read because it avoids treating an empty file as a special case.
Why not do something like this:
for (range_i = 0; range_i < range_list.nr; ++range_i) {
long bottom, top;
if (!lno)
die(_("file is empty"));
if (parse_range_arg(range_list.items[range_i].string,
nth_line_cb, &sb, lno, anchor,
&bottom, &top, sb.path))
usage(blame_usage);
if (bottom < 1)
bottom = 1;
if (lno < top)
top = lno;
if (top < 0 || lno < bottom)
die(Q_("file %s has only %lu line",
"file %s has only %lu lines",
lno), path, lno);
bottom--;
range_set_append_unsafe(&ranges, bottom, top);
anchor = top + 1;
We'd also need to change parse_range_arg to always make bottom less than top:
- if (*begin && *end && *end < *begin) {
+ if (*end < *begin) {
SWAP(*end, *begin);
}
This also fixes the case where the given range is n,-(n+1) (e.g. -L1,-2). At
the moment that will blame from n to the end of the file. My suggested change
would instead blame the first n lines, which makes a lot more sense IMO.
Happy to leave as is if you aren't happy with this suggestion, however.
Re* Consequences of CRLF in index?
Junio C Hamano writes:
> Stefan Beller writes:
>
>> (1<<5) is taken twice now.
>
> Good eyes. I think we use bits #1-#8 now (bit #0 is vacant, so are
> #9-#31).
Let's do this bit-shuffling as a preliminary clean-up.
-- >8 --
Subject: [PATCH] xdiff: reassign xpparm_t.flags bits
We have packed the bits too tightly in such a way that it is not
easy to add a new type of whitespace ignoring option, a new type
of LCS algorithm, or a new type of post-cleanup heuristics.
Reorder bits a bit to give room for these three classes of options
to grow.
While at it, add a comment in front of the bit definitions to
clarify in which structure these defined bits may appear.
Signed-off-by: Junio C Hamano
---
xdiff/xdiff.h | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index b090ad8eac..457cac32d8 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -27,22 +27,24 @@
extern "C" {
#endif /* #ifdef __cplusplus */
+/* xpparm_t.flags */
+#define XDF_NEED_MINIMAL (1 << 0)
-#define XDF_NEED_MINIMAL (1 << 1)
#define XDF_IGNORE_WHITESPACE (1 << 2)
#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
-#define XDF_PATIENCE_DIFF (1 << 5)
-#define XDF_HISTOGRAM_DIFF (1 << 6)
+#define XDF_IGNORE_BLANK_LINES (1 << 7)
+
+#define XDF_PATIENCE_DIFF (1 << 14)
+#define XDF_HISTOGRAM_DIFF (1 << 15)
#define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
#define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
-#define XDF_IGNORE_BLANK_LINES (1 << 7)
-
-#define XDF_INDENT_HEURISTIC (1 << 8)
+#define XDF_INDENT_HEURISTIC (1 << 23)
+/* xdemitconf_t.flags */
#define XDL_EMIT_FUNCNAMES (1 << 0)
#define XDL_EMIT_FUNCCONTEXT (1 << 2)
--
2.15.0-rc2-266-g8f92d095f4
Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()
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
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
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 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 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(&ranges, 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
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, &sb, lno, anchor,
> &bottom, &top, 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(&ranges, 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
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
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
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?
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)
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
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, &sb, lno, anchor,
&bottom, &top, 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(&ranges, 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
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, &sb, lno, anchor,
>> &bottom, &top, 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(&ranges, 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
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?
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
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
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
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
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
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
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
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)
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?
Thank you for the clarification, it's much appreciated. -- Hannes
Re: [PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman
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?
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
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(&trace_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?
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
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
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 = &sorting;
struct ref_format format = REF_FORMAT_INIT;
int icase = 0;
+ int no_gpg_sign = 0;
struct option options[] = {
OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, &filter.lines, 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(&force, N_("replace the tag if exists")),
OPT_BOOL(0, "create-reflog", &create_reflog, N_("create a
reflog")),
+ OPT_BOOL(0, "no-gpg-sign", &no_gpg_sign, N_("do not GPG-sign
tag")),
OPT_GROUP(N_("Tag listing options")),
OPT_COLUMN(0, "column", &colopts, 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(&colopts, -1);
@@ -536,8 +548,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
create_reflog_msg(&object, &reflog_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(&object, tag, &buf, &opt, &prev, &object);
Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree
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(&cp, query_result, 1024); }
Re: Consequences of CRLF in index?
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?
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
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
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
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
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, &type, &sz);
+ if (!data || type != OBJ_BLOB) {
+ free(data);
+ return -1;
+ }
+
+ if (sha1_stat) {
+ memset(&sha1_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, &type, &sz);
- if (!data || type != OBJ_BLOB) {
- free(data);
- return NULL;
- }
- *size = xsize_t(sz);
- if (sha1_stat) {
- memset(&sha1_stat->stat, 0, sizeof(sha1_stat->stat));
- hashcpy(sha1_stat->sha1, istate->cache[pos]->oid.hash);
- }
- return data;
+ return -1;
+
+ return do_read_blob(&istate->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, &st) < 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 = read_skip_workt
Re: [PATCH 0/2] color-moved: ignore all space changes by default
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?
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
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?
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
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
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
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?
+ 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)
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?
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?
> 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]
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?
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
> 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, &sb, lno, anchor,
> &bottom, &top, 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(&ranges, 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
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?
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?
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:
I have a business proposal 15,500,000 GBP.
Felicitări
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
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?
> 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?
> 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,
-- 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
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
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
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
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, &sb, lno, anchor,
> &bottom, &top, 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(&ranges, 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
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
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
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
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
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
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, &referent, &type) ||
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
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?
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
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
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, &sb, lno, anchor,
&bottom, &top, 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(&ranges, 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
