threads / patch / 34459

patch, 6 partsfix blame -L regression; add tests

Subject: [PATCH 0/6] fix blame -L regression; add tests

## tl;dr

12 messages between Jul 17, 2013 and Jul 22, 2013. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Eric Sunshine· Jul 17, 2013, 21:25 UTC · lore

This series fixes a regression in "blame -L X,-N", adds blame -L tests, and makes minor documentation adjustments. The tests, in particular, were motivated by the desire to revisit and continue working on [1] which extends git-blame to accept multiple -L's. That topic will need to extend blame -L tests, of which there were essentially none.

Patches [2/6] (modernize style) and [3/6] (add blame -L tests) are intentionally independent of the "git log -L" topic (from earlier this year) to which the other patches are related. This independence should allow these two patches to graduate at their own pace without being tied to "git log -L".

[1]: http://thread.gmane.org/gmane.comp.version-control.git/229755/
Eric Sunshine (6):
  line-range: fix "blame -L X,-N" regression
  t8001/t8002 (blame): modernize style
  t8001/t8002 (blame): add blame -L tests
  t8001/t8002 (blame): add blame -L :funcname tests
  blame-options.txt: place each -L option variation on its own line
  blame-options.txt: explain that -L <start> and <end> are optional
 Documentation/blame-options.txt |  10 +-
 line-range.c                    |   7 +
 t/annotate-tests.sh             | 363 ++++++++++++++++++++++++++++------------
 t/t8001-annotate.sh             |   6 +-
 t/t8002-blame.sh                |  12 +-
 5 files changed, 283 insertions(+), 115 deletions(-)
-- 
1.8.3.3.1016.g4f0baba
Eric Sunshine· Jul 17, 2013, 21:25 UTC · re: Eric Sunshine · lore

[PATCH 1/6] line-range: fix "blame -L X,-N" regression

"blame -L X,-N" is documented as blaming "N lines ending at X". In practice, the behavior is achieved by swapping the two range endpoints if the second is less than the first. 25ed3412 (Refactor parse_loc; 2013-03-28) broke this interpretation by removing the swapping code from blame.c and failing to add it to line-range.c along with other code relocated from blame.c. Thus, such a range is effectively treated as empty. Fix this regression.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 line-range.c | 7 +++++++
 1 file changed, 7 insertions(+)
Show changes to line-range.c +7 −0
diff --git a/line-range.c b/line-range.c
index 8faf943..3942475 100644
--- a/line-range.c
+++ b/line-range.c
@@ -211,6 +211,8 @@ int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
 		    void *cb_data, long lines, long *begin, long *end,
 		    const char *path)
 {
+	*begin = *end = 0;
+
 	if (*arg == ':') {
 		arg = parse_range_funcname(arg, nth_line_cb, cb_data, lines, begin, end, path);
 		if (!arg || *arg)
@@ -226,6 +228,11 @@ int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
 	if (*arg)
 		return -1;
 
+	if (*begin && *end && *end < *begin) {
+		long tmp;
+		tmp = *end; *end = *begin; *begin = tmp;
+	}
+
 	return 0;
 }
 

-- 
1.8.3.3.1016.g4f0baba
Eric Sunshine· Jul 17, 2013, 21:25 UTC · re: Eric Sunshine · lore

[PATCH 2/6] t8001/t8002 (blame): modernize style

In particular,
- indent with tabs
- cuddle test description and opening body quote with test_expect_foo
- normalize test descriptions and case
- remove whitepsace following redirection operator
- use standardized filenames (such as "actual", "expected")
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/annotate-tests.sh | 221 +++++++++++++++++++++++++++-------------------------
 t/t8001-annotate.sh |   6 +-
 t/t8002-blame.sh    |  12 ++-
 3 files changed, 127 insertions(+), 112 deletions(-)
Show changes to 3 files +127 −112

t/annotate-tests.sh, t/t8001-annotate.sh, t/t8002-blame.sh

diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index c56a77d..3aa6964 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -2,11 +2,11 @@
 # sourced from t8001-annotate.sh and t8002-blame.sh.
 
 check_count () {
-	head=
-	case "$1" in -h) head="$2"; shift; shift ;; esac
-	echo "$PROG file $head" >&4
-	$PROG file $head >.result || return 1
-	cat .result | perl -e '
+	head= &&
+	case "$1" in -h) head="$2"; shift; shift ;; esac &&
+	echo "$PROG file $head" >&4 &&
+	$PROG file $head >actual &&
+	perl -e '
 		my %expect = (@ARGV);
 		my %count = map { $_ => 0 } keys %expect;
 		while (<STDIN>) {
@@ -31,107 +31,114 @@ check_count () {
 			print STDERR "Author $author (expected $value, attributed $count) $ok\n";
 		}
 		exit($bad);
-	' "$@"
+	' "$@" <actual
 }
 
-test_expect_success \
-    'prepare reference tree' \
-    'echo "1A quick brown fox jumps over the" >file &&
-     echo "lazy dog" >>file &&
-     git add file &&
-     GIT_AUTHOR_NAME="A" GIT_AUTHOR_EMAIL="A@test.git" git commit -a -m "Initial."'
-
-test_expect_success \
-    'check all lines blamed on A' \
-    'check_count A 2'
-
-test_expect_success \
-    'Setup new lines blamed on B' \
-    'echo "2A quick brown fox jumps over the" >>file &&
-     echo "lazy dog" >> file &&
-     GIT_AUTHOR_NAME="B" GIT_AUTHOR_EMAIL="B@test.git" git commit -a -m "Second."'
-
-test_expect_success \
-    'Two lines blamed on A, two on B' \
-    'check_count A 2 B 2'
-
-test_expect_success \
-    'merge-setup part 1' \
-    'git checkout -b branch1 master &&
-     echo "3A slow green fox jumps into the" >> file &&
-     echo "well." >> file &&
-     GIT_AUTHOR_NAME="B1" GIT_AUTHOR_EMAIL="B1@test.git" git commit -a -m "Branch1-1"'
-
-test_expect_success \
-    'Two lines blamed on A, two on B, two on B1' \
-    'check_count A 2 B 2 B1 2'
-
-test_expect_success \
-    'merge-setup part 2' \
-    'git checkout -b branch2 master &&
-     sed -e "s/2A quick brown/4A quick brown lazy dog/" < file > file.new &&
-     mv file.new file &&
-     GIT_AUTHOR_NAME="B2" GIT_AUTHOR_EMAIL="B2@test.git" git commit -a -m "Branch2-1"'
-
-test_expect_success \
-    'Two lines blamed on A, one on B, one on B2' \
-    'check_count A 2 B 1 B2 1'
-
-test_expect_success \
-    'merge-setup part 3' \
-    'git pull . branch1'
-
-test_expect_success \
-    'Two lines blamed on A, one on B, two on B1, one on B2' \
-    'check_count A 2 B 1 B1 2 B2 1'
-
-test_expect_success \
-    'Annotating an old revision works' \
-    'check_count -h master A 2 B 2'
-
-test_expect_success \
-    'Annotating an old revision works' \
-    'check_count -h master^ A 2'
-
-test_expect_success \
-    'merge-setup part 4' \
-    'echo "evil merge." >>file &&
-     git commit -a --amend'
-
-test_expect_success \
-    'Two lines blamed on A, one on B, two on B1, one on B2, one on A U Thor' \
-    'check_count A 2 B 1 B1 2 B2 1 "A U Thor" 1'
-
-test_expect_success \
-    'an incomplete line added' \
-    'echo "incomplete" | tr -d "\\012" >>file &&
-    GIT_AUTHOR_NAME="C" GIT_AUTHOR_EMAIL="C@test.git" git commit -a -m "Incomplete"'
-
-test_expect_success \
-    'With incomplete lines.' \
-    'check_count A 2 B 1 B1 2 B2 1 "A U Thor" 1 C 1'
-
-test_expect_success \
-    'some edit' \
-    'mv file file.orig &&
-    {
-	cat file.orig &&
-	echo
-    } | sed -e "s/^3A/99/" -e "/^1A/d" -e "/^incomplete/d" > file &&
-    echo "incomplete" | tr -d "\\012" >>file &&
-    GIT_AUTHOR_NAME="D" GIT_AUTHOR_EMAIL="D@test.git" git commit -a -m "edit"'
-
-test_expect_success \
-    'some edit' \
-    'check_count A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1'
-
-test_expect_success \
-    'an obfuscated email added' \
-    'echo "No robots allowed" > file.new &&
-     cat file >> file.new &&
-     mv file.new file &&
-     GIT_AUTHOR_NAME="E" GIT_AUTHOR_EMAIL="E at test dot git" git commit -a -m "norobots"'
-
-test_expect_success \
-    'obfuscated email parsed' \
-    'check_count A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1 E 1'
+test_expect_success 'setup A lines' '
+	echo "1A quick brown fox jumps over the" >file &&
+	echo "lazy dog" >>file &&
+	git add file &&
+	GIT_AUTHOR_NAME="A" GIT_AUTHOR_EMAIL="A@test.git" \
+	git commit -a -m "Initial."
+'
+
+test_expect_success 'blame 1 author' '
+	check_count A 2
+'
+
+test_expect_success 'setup B lines' '
+	echo "2A quick brown fox jumps over the" >>file &&
+	echo "lazy dog" >>file &&
+	GIT_AUTHOR_NAME="B" GIT_AUTHOR_EMAIL="B@test.git" \
+	git commit -a -m "Second."
+'
+
+test_expect_success 'blame 2 authors' '
+	check_count A 2 B 2
+'
+
+test_expect_success 'setup B1 lines (branch1)' '
+	git checkout -b branch1 master &&
+	echo "3A slow green fox jumps into the" >>file &&
+	echo "well." >>file &&
+	GIT_AUTHOR_NAME="B1" GIT_AUTHOR_EMAIL="B1@test.git" \
+	git commit -a -m "Branch1-1"
+'
+
+test_expect_success 'blame 2 authors + 1 branch1 author' '
+	check_count A 2 B 2 B1 2
+'
+
+test_expect_success 'setup B2 lines (branch2)' '
+	git checkout -b branch2 master &&
+	sed -e "s/2A quick brown/4A quick brown lazy dog/" <file >file.new &&
+	mv file.new file &&
+	GIT_AUTHOR_NAME="B2" GIT_AUTHOR_EMAIL="B2@test.git" \
+	git commit -a -m "Branch2-1"
+'
+
+test_expect_success 'blame 2 authors + 1 branch2 author' '
+	check_count A 2 B 1 B2 1
+'
+
+test_expect_success 'merge branch1 & branch2' '
+	git pull . branch1
+'
+
+test_expect_success 'blame 2 authors + 2 merged-in authors' '
+	check_count A 2 B 1 B1 2 B2 1
+'
+
+test_expect_success 'blame ancestor' '
+	check_count -h master A 2 B 2
+'
+
+test_expect_success 'blame great-ancestor' '
+	check_count -h master^ A 2
+'
+
+test_expect_success 'setup evil merge' '
+	echo "evil merge." >>file &&
+	git commit -a --amend
+'
+
+test_expect_success 'blame evil merge' '
+	check_count A 2 B 1 B1 2 B2 1 "A U Thor" 1
+'
+
+test_expect_success 'setup incomplete line' '
+	echo "incomplete" | tr -d "\\012" >>file &&
+	GIT_AUTHOR_NAME="C" GIT_AUTHOR_EMAIL="C@test.git" \
+	git commit -a -m "Incomplete"
+'
+
+test_expect_success 'blame incomplete line' '
+	check_count A 2 B 1 B1 2 B2 1 "A U Thor" 1 C 1
+'
+
+test_expect_success 'setup edits' '
+	mv file file.orig &&
+	{
+		cat file.orig &&
+		echo
+	} | sed -e "s/^3A/99/" -e "/^1A/d" -e "/^incomplete/d" >file &&
+	echo "incomplete" | tr -d "\\012" >>file &&
+	GIT_AUTHOR_NAME="D" GIT_AUTHOR_EMAIL="D@test.git" \
+	git commit -a -m "edit"
+'
+
+test_expect_success 'blame edits' '
+	check_count A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1
+'
+
+test_expect_success 'setup obfuscated email' '
+	echo "No robots allowed" >file.new &&
+	cat file >>file.new &&
+	mv file.new file &&
+	GIT_AUTHOR_NAME="E" GIT_AUTHOR_EMAIL="E at test dot git" \
+	git commit -a -m "norobots"
+'
+
+test_expect_success 'blame obfuscated email' '
+	check_count A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1 E 1
+'
diff --git a/t/t8001-annotate.sh b/t/t8001-annotate.sh
index 41962f0..72176e4 100755
--- a/t/t8001-annotate.sh
+++ b/t/t8001-annotate.sh
@@ -6,9 +6,9 @@ test_description='git annotate'
 PROG='git annotate'
 . "$TEST_DIRECTORY"/annotate-tests.sh
 
-test_expect_success 'Annotating an old revision works' '
-	git annotate file master >result &&
-	awk "{ print \$3; }" <result >authors &&
+test_expect_success 'annotate old revision' '
+	git annotate file master >actual &&
+	awk "{ print \$3; }" <actual >authors &&
 	test 2 = $(grep A <authors | wc -l) &&
 	test 2 = $(grep B <authors | wc -l)
 '
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index e2896cf..5cdf3f1 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -7,8 +7,16 @@ PROG='git blame -c'
 . "$TEST_DIRECTORY"/annotate-tests.sh
 
 PROG='git blame -c -e'
-test_expect_success 'Blame --show-email works' '
-    check_count "<A@test.git>" 1 "<B@test.git>" 1 "<B1@test.git>" 1 "<B2@test.git>" 1 "<author@example.com>" 1 "<C@test.git>" 1 "<D@test.git>" 1 "<E at test dot git>" 1
+test_expect_success 'blame --show-email' '
+	check_count \
+		"<A@test.git>" 1 \
+		"<B@test.git>" 1 \
+		"<B1@test.git>" 1 \
+		"<B2@test.git>" 1 \
+		"<author@example.com>" 1 \
+		"<C@test.git>" 1 \
+		"<D@test.git>" 1 \
+		"<E at test dot git>" 1
 '
 
 test_done

-- 
1.8.3.3.1016.g4f0baba
Eric Sunshine· Jul 17, 2013, 21:25 UTC · re: Eric Sunshine · lore

[PATCH 3/6] t8001/t8002 (blame): add blame -L tests

With the exception of a couple "corner case" checks in t8003 (and some indirect tests in t4211 of -L parsing code shared by log -L), there is no systematic checking of blame -L. Add tests to check blame -L directly.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/annotate-tests.sh | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 3 deletions(-)
Show changes to t/annotate-tests.sh +101 −3
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 3aa6964..b6a7478 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -3,9 +3,17 @@
 
 check_count () {
 	head= &&
-	case "$1" in -h) head="$2"; shift; shift ;; esac &&
-	echo "$PROG file $head" >&4 &&
-	$PROG file $head >actual &&
+	options= &&
+	while :
+	do
+		case "$1" in
+		-h) head="$2"; shift; shift ;;
+		-*) options="$options $1"; shift ;;
+		*) break ;;
+		esac
+	done &&
+	echo "$PROG $options file $head" >&4 &&
+	$PROG $options file $head >actual &&
 	perl -e '
 		my %expect = (@ARGV);
 		my %count = map { $_ => 0 } keys %expect;
@@ -142,3 +150,93 @@ test_expect_success 'setup obfuscated email' '
 test_expect_success 'blame obfuscated email' '
 	check_count A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1 E 1
 '
+
+test_expect_success 'blame -L 1 (all)' '
+	check_count -L1 A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1 E 1
+'
+
+test_expect_success 'blame -L , (all)' '
+	check_count -L, A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1 E 1
+'
+
+test_expect_success 'blame -L X (X to end)' '
+	check_count -L5 B1 1 C 1 D 1 "A U Thor" 1
+'
+
+test_expect_success 'blame -L X, (X to end)' '
+	check_count -L5, B1 1 C 1 D 1 "A U Thor" 1
+'
+
+test_expect_success 'blame -L ,Y (up to Y)' '
+	check_count -L,3 A 1 B2 1 E 1
+'
+
+test_expect_success 'blame -L X,X' '
+	check_count -L3,3 B2 1
+'
+
+test_expect_success 'blame -L X,Y' '
+	check_count -L3,6 B 1 B1 1 B2 1 D 1
+'
+
+test_expect_success 'blame -L Y,X (undocumented)' '
+	check_count -L6,3 B 1 B1 1 B2 1 D 1
+'
+
+test_expect_success 'blame -L X,+1' '
+	check_count -L3,+1 B2 1
+'
+
+test_expect_success 'blame -L X,+N' '
+	check_count -L3,+4 B 1 B1 1 B2 1 D 1
+'
+
+test_expect_success 'blame -L X,-1' '
+	check_count -L3,-1 B2 1
+'
+
+test_expect_success 'blame -L X,-N' '
+	check_count -L6,-4 B 1 B1 1 B2 1 D 1
+'
+
+test_expect_success 'blame -L /RE/ (RE to end)' '
+	check_count -L/evil/ C 1 "A U Thor" 1
+'
+
+test_expect_success 'blame -L /RE/,/RE2/' '
+	check_count -L/robot/,/green/ A 1 B 1 B2 1 D 1 E 1
+'
+
+test_expect_success 'blame -L X,/RE/' '
+	check_count -L5,/evil/ B1 1 D 1 "A U Thor" 1
+'
+
+test_expect_success 'blame -L /RE/,Y' '
+	check_count -L/99/,7 B1 1 D 1 "A U Thor" 1
+'
+
+test_expect_success 'blame -L /RE/,+N' '
+	check_count -L/99/,+3 B1 1 D 1 "A U Thor" 1
+'
+
+test_expect_success 'blame -L /RE/,-N' '
+	check_count -L/99/,-3 B 1 B2 1 D 1
+'
+
+test_expect_success 'blame -L X (X > nlines)' '
+	test_must_fail $PROG -L12345 file
+'
+
+test_expect_success 'blame -L ,Y (Y > nlines)' '
+	test_must_fail $PROG -L,12345 file
+'
+
+test_expect_success 'blame -L bogus' '
+	test_must_fail $PROG -L file &&
+	test_must_fail $PROG -L1,+ file &&
+	test_must_fail $PROG -L1,- file &&
+	test_must_fail $PROG -LX file &&
+	test_must_fail $PROG -L1,X file &&
+	test_must_fail $PROG -L1,+N file &&
+	test_must_fail $PROG -L1,-N file
+'

-- 
1.8.3.3.1016.g4f0baba
Eric Sunshine· Jul 17, 2013, 21:25 UTC · re: Eric Sunshine · lore

[PATCH 4/6] t8001/t8002 (blame): add blame -L :funcname tests

git-blame inherited "-L :funcname" support when "-L :funcname:file" was implemented for git-log. Add tests.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/annotate-tests.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)
Show changes to t/annotate-tests.sh +46 −2
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index b6a7478..0bfee00 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -3,17 +3,19 @@
 
 check_count () {
 	head= &&
+	file='file' &&
 	options= &&
 	while :
 	do
 		case "$1" in
 		-h) head="$2"; shift; shift ;;
+		-f) file="$2"; shift; shift ;;
 		-*) options="$options $1"; shift ;;
 		*) break ;;
 		esac
 	done &&
-	echo "$PROG $options file $head" >&4 &&
-	$PROG $options file $head >actual &&
+	echo "$PROG $options $file $head" >&4 &&
+	$PROG $options $file $head >actual &&
 	perl -e '
 		my %expect = (@ARGV);
 		my %count = map { $_ => 0 } keys %expect;
@@ -231,6 +233,48 @@ test_expect_success 'blame -L ,Y (Y > nlines)' '
 	test_must_fail $PROG -L,12345 file
 '
 
+test_expect_success 'setup -L :regex' '
+	tr Q "\\t" >hello.c <<-\EOF &&
+	int main(int argc, const char *argv[])
+	{
+	Qputs("hello");
+	}
+	EOF
+	git add hello.c &&
+	GIT_AUTHOR_NAME="F" GIT_AUTHOR_EMAIL="F@test.git" \
+	git commit -m "hello" &&
+
+	mv hello.c hello.orig &&
+	sed -e "/}/i\\
+	Qputs(\"goodbye\");" <hello.orig | tr Q "\\t" >hello.c &&
+	GIT_AUTHOR_NAME="G" GIT_AUTHOR_EMAIL="G@test.git" \
+	git commit -a -m "goodbye" &&
+
+	mv hello.c hello.orig &&
+	echo "#include <stdio.h>" >hello.c &&
+	cat hello.orig >>hello.c &&
+	tr Q "\\t" >>hello.c <<-\EOF
+	void mail()
+	{
+	Qputs("mail");
+	}
+	EOF
+	GIT_AUTHOR_NAME="H" GIT_AUTHOR_EMAIL="H@test.git" \
+	git commit -a -m "mail"
+'
+
+test_expect_success 'blame -L :literal' '
+	check_count -f hello.c -L:main F 4 G 1
+'
+
+test_expect_success 'blame -L :regex' '
+	check_count -f hello.c "-L:m[a-z][a-z]l" H 4
+'
+
+test_expect_success 'blame -L :nomatch' '
+	test_must_fail $PROG -L:nomatch hello.c
+'
+
 test_expect_success 'blame -L bogus' '
 	test_must_fail $PROG -L file &&
 	test_must_fail $PROG -L1,+ file &&

-- 
1.8.3.3.1016.g4f0baba
Eric Sunshine· Jul 17, 2013, 21:25 UTC · re: Eric Sunshine · lore

[PATCH 5/6] blame-options.txt: place each -L option variation on its own line

Standard practice in Git documentation is for each variation of an option (such as: -p / --porcelain) to be placed on its own line in the OPTIONS table. The -L option does not follow suit. It cuddles "-L <start>,<end>" and "-L :<regex>", separated by a comma. This is inconsistent and potentially confusing since the comma separating them is typeset the same as the comma in "<start>,<end>". Fix this by placing each variation on its own line.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/blame-options.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Documentation/blame-options.txt +2 −1
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index e9f984b..624b353 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -9,7 +9,8 @@
 --show-stats::
 	Include additional statistics at the end of blame output.
 
--L <start>,<end>, -L :<regex>::
+-L <start>,<end>::
+-L :<regex>::
 	Annotate only the given line range.  <start> and <end> can take
 	one of these forms:
 

-- 
1.8.3.3.1016.g4f0baba
Thomas Rast· Jul 18, 2013, 11:56 UTC · re: Eric Sunshine · lore

Re: [PATCH 5/6] blame-options.txt: place each -L option variation on its own line

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 7 quoted lines
> Standard practice in Git documentation is for each variation of an
> option (such as: -p / --porcelain) to be placed on its own line in the
> OPTIONS table. The -L option does not follow suit. It cuddles
> "-L <start>,<end>" and "-L :<regex>", separated by a comma. This is
> inconsistent and potentially confusing since the comma separating them
> is typeset the same as the comma in "<start>,<end>". Fix this by placing
> each variation on its own line.
Ok, but why not fix them all in one go?
Edited to remove the false positives:

$ git grep -n '^.*,.*::$' Documentation/*.txt Documentation/blame-options.txt:12:-L <start>,<end>, -L :<regex>:: Documentation/config.txt:1252:gitcvs.dbuser, gitcvs.dbpass:: Documentation/config.txt:1513:http.lowSpeedLimit, http.lowSpeedTime:: Documentation/git-add.txt:90:-e, \--edit:: Documentation/git-check-attr.txt:22:-a, --all:: Documentation/git-check-ignore.txt:26:-q, --quiet:: Documentation/git-check-ignore.txt:30:-v, --verbose:: Documentation/git-check-ignore.txt:42:-n, --non-matching:: Documentation/git-log.txt:65:-L <start>,<end>:<file>, -L :<regex>:<file>:: Documentation/git-log.txt:156:git log -L '/int main/',/^}/:main.c:: Documentation/git-p4.txt:171:--verbose, -v:: Documentation/git-p4.txt:282:--dry-run, -n::

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Jul 18, 2013, 17:51 UTC · re: Thomas Rast · lore

Re: [PATCH 5/6] blame-options.txt: place each -L option variation on its own line

Thomas Rast <tr@thomasrast.ch> writes:
Show 27 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Standard practice in Git documentation is for each variation of an
>> option (such as: -p / --porcelain) to be placed on its own line in the
>> OPTIONS table. The -L option does not follow suit. It cuddles
>> "-L <start>,<end>" and "-L :<regex>", separated by a comma. This is
>> inconsistent and potentially confusing since the comma separating them
>> is typeset the same as the comma in "<start>,<end>". Fix this by placing
>> each variation on its own line.
>
> Ok, but why not fix them all in one go?
>
> Edited to remove the false positives:
>
> $ git grep -n '^.*,.*::$' Documentation/*.txt
> Documentation/blame-options.txt:12:-L <start>,<end>, -L :<regex>::
> Documentation/config.txt:1252:gitcvs.dbuser, gitcvs.dbpass::
> Documentation/config.txt:1513:http.lowSpeedLimit, http.lowSpeedTime::
> Documentation/git-add.txt:90:-e, \--edit::
> Documentation/git-check-attr.txt:22:-a, --all::
> Documentation/git-check-ignore.txt:26:-q, --quiet::
> Documentation/git-check-ignore.txt:30:-v, --verbose::
> Documentation/git-check-ignore.txt:42:-n, --non-matching::
> Documentation/git-log.txt:65:-L <start>,<end>:<file>, -L :<regex>:<file>::
> Documentation/git-log.txt:156:git log -L '/int main/',/^}/:main.c::
> Documentation/git-p4.txt:171:--verbose, -v::
> Documentation/git-p4.txt:282:--dry-run, -n::

Thanks, but I think it is cleaner to fix the others in a separate "treewide clean-up" patch.

Also I wonder if this one Documentation/git-log.txt:156
    git log -L '/int main/',/^}/:main.c
should be more like one of these:
    git log -L '/int main/,/^}/':main.c
    git log -L '/int main/,/^}/:main.c'

I find it somewhat hard to spot the closing sq in the middle of a string.

Thomas Rast· Jul 22, 2013, 10:38 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/6] blame-options.txt: place each -L option variation on its own line

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> Also I wonder if this one Documentation/git-log.txt:156
>
>     git log -L '/int main/',/^}/:main.c
>
> should be more like one of these:
>
>     git log -L '/int main/,/^}/':main.c
>     git log -L '/int main/,/^}/:main.c'
>
> I find it somewhat hard to spot the closing sq in the middle of a
> string.

There's a distinct advantage to keeping the sq before the colon, at least in my config (and I think I haven't changed the defaults as far as this goes): bash will try to complete a filename if you type

  git log -L '...':mai<TAB>
but not if you instead do
  git log -L '...:'mai<TAB>
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Jul 22, 2013, 17:25 UTC · re: Thomas Rast · lore

Re: [PATCH 5/6] blame-options.txt: place each -L option variation on its own line

Thomas Rast <trast@inf.ethz.ch> writes:
Show 23 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Also I wonder if this one Documentation/git-log.txt:156
>>
>>     git log -L '/int main/',/^}/:main.c
>>
>> should be more like one of these:
>>
>>     git log -L '/int main/,/^}/':main.c
>>     git log -L '/int main/,/^}/:main.c'
>>
>> I find it somewhat hard to spot the closing sq in the middle of a
>> string.
>
> There's a distinct advantage to keeping the sq before the colon, at
> least in my config (and I think I haven't changed the defaults as far as
> this goes): bash will try to complete a filename if you type
>
>   git log -L '...':mai<TAB>
>
> but not if you instead do
>
>   git log -L '...:'mai<TAB>
Interesting.
Eric Sunshine· Jul 17, 2013, 21:25 UTC · re: Eric Sunshine · lore

[PATCH 6/6] blame-options.txt: explain that -L <start> and <end> are optional

The ability to omit either end of the -L range is a handy but undocumented shortcut, and is thus not easily discovered. Fix this shortcoming.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/blame-options.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Show changes to Documentation/blame-options.txt +5 −2
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 624b353..4e55b15 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -11,8 +11,11 @@
 
 -L <start>,<end>::
 -L :<regex>::
-	Annotate only the given line range.  <start> and <end> can take
-	one of these forms:
+	Annotate only the given line range.  <start> and <end> are optional.
+	``-L <start>'' or ``-L <start>,'' spans from <start> to end of file.
+	``-L ,<end>'' spans from start of file to <end>.
++
+<start> and <end> can take one of these forms:
 
 include::line-range-format.txt[]
 

-- 
1.8.3.3.1016.g4f0baba
Thomas Rast· Jul 18, 2013, 12:04 UTC · re: Eric Sunshine · lore

Re: [PATCH 0/6] fix blame -L regression; add tests

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 21 quoted lines
> This series fixes a regression in "blame -L X,-N", adds blame -L tests,
> and makes minor documentation adjustments. The tests, in particular,
> were motivated by the desire to revisit and continue working on [1]
> which extends git-blame to accept multiple -L's. That topic will need to
> extend blame -L tests, of which there were essentially none.
>
> Patches [2/6] (modernize style) and [3/6] (add blame -L tests) are
> intentionally independent of the "git log -L" topic (from earlier this
> year) to which the other patches are related.  This independence should
> allow these two patches to graduate at their own pace without being tied
> to "git log -L".
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/229755/
>
> Eric Sunshine (6):
>   line-range: fix "blame -L X,-N" regression
>   t8001/t8002 (blame): modernize style
>   t8001/t8002 (blame): add blame -L tests
>   t8001/t8002 (blame): add blame -L :funcname tests
>   blame-options.txt: place each -L option variation on its own line
>   blame-options.txt: explain that -L <start> and <end> are optional
Thanks, and except for the comment I just sent out,
Acked-by: Thomas Rast <trast@inf.ethz.ch>

In case it wasn't obvious to anyone else: the tests do actually verify that the right lines were picked, by counting how often each author is blamed.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

← back to recent threads