Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions

7 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:51:11

Jeff King [off-list ref] writes:
Two minor complaints on git-blame; maybe somebody can point out
something clever I've missed.
  1. blame's "-L" understands patterns already.
Teaching blame to take multiple -L options has been one of many
longstanding todo item for me.  Someday.
  2. Parsing the human-readable output blame output sucks. But parsing
     --porcelain is annoyingly complex for quick-and-dirty things like
     this. It doesn't repeat the commit information per-line.
Non-repetition was quite deliberate, as the reader was expected to have
memory proportional to the number of lines in the range, but I agree it is
not friendly for quick and dirty hack.

You should be able to add a command line option that disables the early
return at the beginning of emit_one_suspect_detail() with a 5-6 lines of
patch.

Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions

From: Jeff King <hidden>
Date: 2016-06-15 22:51:11

On Fri, May 06, 2011 at 03:27:08PM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
Two minor complaints on git-blame; maybe somebody can point out
something clever I've missed.
quoted
  1. blame's "-L" understands patterns already.
Teaching blame to take multiple -L options has been one of many
longstanding todo item for me.  Someday.
I think multiple -L is not quite enough. I want a single "-L" that
matches every instance of a pattern, like:

  -L "/ ()/,+0"
quoted
  2. Parsing the human-readable output blame output sucks. But parsing
     --porcelain is annoyingly complex for quick-and-dirty things like
     this. It doesn't repeat the commit information per-line.
Non-repetition was quite deliberate, as the reader was expected to have
memory proportional to the number of lines in the range, but I agree it is
not friendly for quick and dirty hack.

You should be able to add a command line option that disables the early
return at the beginning of emit_one_suspect_detail() with a 5-6 lines of
patch.
I tried that, and it is slightly more involved. You also need to break a
multi-line run of lines that blame to a single suspect into its
constituent lines. I am 75% of the way to such a patch if you are
interested. It's not a lot of code, but it takes some refactoring of
emit_porcelain.

-Peff

[PATCH 0/3] blame --line-porcelain

From: Jeff King <hidden>
Date: 2016-06-15 22:51:12

On Fri, May 06, 2011 at 06:29:51PM -0400, Jeff King wrote:
quoted
Non-repetition was quite deliberate, as the reader was expected to have
memory proportional to the number of lines in the range, but I agree it is
not friendly for quick and dirty hack.

You should be able to add a command line option that disables the early
return at the beginning of emit_one_suspect_detail() with a 5-6 lines of
patch.
I tried that, and it is slightly more involved. You also need to break a
multi-line run of lines that blame to a single suspect into its
constituent lines. I am 75% of the way to such a patch if you are
interested. It's not a lot of code, but it takes some refactoring of
emit_porcelain.
It turned out to not be too bad. Here's the series.

  [1/3]: add tests for various blame formats
  [2/3]: blame: refactor porcelain output
  [3/3]: blame: add --line-porcelain output format

-Peff

[PATCH 1/3] add tests for various blame formats

From: Jeff King <hidden>
Date: 2016-06-15 22:51:12

We don't seem to have any tests for "blame --porcelain".
Let's at least do a trivial test on a simple example.

Signed-off-by: Jeff King <redacted>
---
I always feel funny putting human-readable output in a test. Maybe it is
not worth including the "normal" output test below, but I assume it's
going to remain pretty stable.

 t/t8008-blame-formats.sh |   71 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)
 create mode 100755 t/t8008-blame-formats.sh
diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
new file mode 100755
index 0000000..387d1a6
--- /dev/null
+++ b/t/t8008-blame-formats.sh
@@ -0,0 +1,71 @@
+#!/bin/sh
+
+test_description='blame output in various formats on a simple case'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo a >file &&
+	git add file
+	test_tick &&
+	git commit -m one &&
+	echo b >>file &&
+	echo c >>file &&
+	echo d >>file &&
+	test_tick &&
+	git commit -a -m two
+'
+
+cat >expect <<'EOF'
+^baf5e0b (A U Thor 2005-04-07 15:13:13 -0700 1) a
+8825379d (A U Thor 2005-04-07 15:14:13 -0700 2) b
+8825379d (A U Thor 2005-04-07 15:14:13 -0700 3) c
+8825379d (A U Thor 2005-04-07 15:14:13 -0700 4) d
+EOF
+test_expect_success 'normal blame output' '
+	git blame file >actual &&
+	test_cmp expect actual
+'
+
+ID1=baf5e0b3869e0b2b2beb395a3720c7b51eac94fc
+COMMIT1='author A U Thor
+author-mail <author@example.com>
+author-time 1112911993
+author-tz -0700
+committer C O Mitter
+committer-mail <committer@example.com>
+committer-time 1112911993
+committer-tz -0700
+summary one
+boundary
+filename file'
+ID2=8825379dfb8a1267b58e8e5bcf69eec838f685ec
+COMMIT2='author A U Thor
+author-mail <author@example.com>
+author-time 1112912053
+author-tz -0700
+committer C O Mitter
+committer-mail <committer@example.com>
+committer-time 1112912053
+committer-tz -0700
+summary two
+previous baf5e0b3869e0b2b2beb395a3720c7b51eac94fc file
+filename file'
+
+cat >expect <<EOF
+$ID1 1 1 1
+$COMMIT1
+	a
+$ID2 2 2 3
+$COMMIT2
+	b
+$ID2 3 3
+	c
+$ID2 4 4
+	d
+EOF
+test_expect_success 'blame --porcelain output' '
+	git blame --porcelain file >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.7.5.rc2.8.gc085

[PATCH 2/3] blame: refactor porcelain output

From: Jeff King <hidden>
Date: 2016-06-15 22:51:12

This is in preparation for adding more porcelain output
options. The three changes are:

  1. emit_porcelain now receives the format option flags

  2. emit_one_suspect_detail takes an optional "repeat"
     parameter to suppress the "show only once" behavior

  3. The code for emitting porcelain suspect is factored
     into its own function for repeatability.

There should be no functional changes.

Signed-off-by: Jeff King <redacted>
---
I broke this out for readability. I can break each of the 3 out into a
separate patch if that helps, but it seemed excessive.

 builtin/blame.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 4242e4b..d74e18f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1484,13 +1484,14 @@ static void write_filename_info(const char *path)
 /*
  * Porcelain/Incremental format wants to show a lot of details per
  * commit.  Instead of repeating this every line, emit it only once,
- * the first time each commit appears in the output.
+ * the first time each commit appears in the output (unless the
+ * user has specifically asked for us to repeat).
  */
-static int emit_one_suspect_detail(struct origin *suspect)
+static int emit_one_suspect_detail(struct origin *suspect, int repeat)
 {
 	struct commit_info ci;
 
-	if (suspect->commit->object.flags & METAINFO_SHOWN)
+	if (!repeat && suspect->commit->object.flags & METAINFO_SHOWN)
 		return 0;
 
 	suspect->commit->object.flags |= METAINFO_SHOWN;
@@ -1529,7 +1530,7 @@ static void found_guilty_entry(struct blame_entry *ent)
 		printf("%s %d %d %d\n",
 		       sha1_to_hex(suspect->commit->object.sha1),
 		       ent->s_lno + 1, ent->lno + 1, ent->num_lines);
-		emit_one_suspect_detail(suspect);
+		emit_one_suspect_detail(suspect, 0);
 		write_filename_info(suspect->path);
 		maybe_flush_or_die(stdout, "stdout");
 	}
@@ -1619,7 +1620,15 @@ static const char *format_time(unsigned long time, const char *tz_str,
 #define OUTPUT_NO_AUTHOR       0200
 #define OUTPUT_SHOW_EMAIL	0400
 
-static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)
+static void emit_porcelain_details(struct origin *suspect, int repeat)
+{
+	if (emit_one_suspect_detail(suspect, repeat) ||
+	    (suspect->commit->object.flags & MORE_THAN_ONE_PATH))
+		write_filename_info(suspect->path);
+}
+
+static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
+			   int opt)
 {
 	int cnt;
 	const char *cp;
@@ -1633,9 +1642,7 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)
 	       ent->s_lno + 1,
 	       ent->lno + 1,
 	       ent->num_lines);
-	if (emit_one_suspect_detail(suspect) ||
-	    (suspect->commit->object.flags & MORE_THAN_ONE_PATH))
-		write_filename_info(suspect->path);
+	emit_porcelain_details(suspect, 0);
 
 	cp = nth_line(sb, ent->lno);
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
@@ -1756,7 +1763,7 @@ static void output(struct scoreboard *sb, int option)
 
 	for (ent = sb->ent; ent; ent = ent->next) {
 		if (option & OUTPUT_PORCELAIN)
-			emit_porcelain(sb, ent);
+			emit_porcelain(sb, ent, option);
 		else {
 			emit_other(sb, ent, option);
 		}
-- 
1.7.5.rc2.8.gc085

[PATCH 3/3] blame: add --line-porcelain output format

From: Jeff King <hidden>
Date: 2016-06-15 22:51:12

This is just like --porcelain, except that we always output
the commit information for each line, not just the first
time it is referenced. This can make quick and dirty scripts
much easier to write; see the example added to the blame
documentation.

Signed-off-by: Jeff King <redacted>
---
I'm not 100% happy with the name, but I couldn't think of anything
better. Something like --verbose-porcelain works, but is a little too
vague. Suggestions welcome.

 Documentation/blame-options.txt |    5 +++++
 Documentation/git-blame.txt     |   13 +++++++++++++
 builtin/blame.c                 |   10 ++++++++--
 t/t8008-blame-formats.sh        |   19 +++++++++++++++++++
 4 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 16e3c68..e76195a 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -52,6 +52,11 @@ of lines before or after the line given by <start>.
 --porcelain::
 	Show in a format designed for machine consumption.
 
+--line-porcelain::
+	Show the porcelain format, but output commit information for
+	each line, not just the first time a commit is referenced.
+	Implies --porcelain.
+
 --incremental::
 	Show the result incrementally in a format designed for
 	machine consumption.
diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index bb8edb4..9516914 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -105,6 +105,19 @@ The contents of the actual line is output after the above
 header, prefixed by a TAB. This is to allow adding more
 header elements later.
 
+The porcelain format generally suppresses commit information that has
+already been seen. For example, two lines that are blamed to the same
+commit will both be shown, but the details for that commit will be shown
+only once. This is more efficient, but may require more state be kept by
+the reader. The `--line-porcelain` option can be used to output full
+commit information for each line, allowing simpler (but less efficient)
+usage like:
+
+	# count the number of lines attributed to each author
+	git blame --line-porcelain file |
+	sed -n 's/^author //p' |
+	sort | uniq -c | sort -rn
+
 
 SPECIFYING RANGES
 -----------------
diff --git a/builtin/blame.c b/builtin/blame.c
index d74e18f..6c26672 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1619,6 +1619,7 @@ static const char *format_time(unsigned long time, const char *tz_str,
 #define OUTPUT_SHOW_SCORE      0100
 #define OUTPUT_NO_AUTHOR       0200
 #define OUTPUT_SHOW_EMAIL	0400
+#define OUTPUT_LINE_PORCELAIN 01000
 
 static void emit_porcelain_details(struct origin *suspect, int repeat)
 {
@@ -1630,6 +1631,7 @@ static void emit_porcelain_details(struct origin *suspect, int repeat)
 static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
 			   int opt)
 {
+	int repeat = opt & OUTPUT_LINE_PORCELAIN;
 	int cnt;
 	const char *cp;
 	struct origin *suspect = ent->suspect;
@@ -1642,15 +1644,18 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
 	       ent->s_lno + 1,
 	       ent->lno + 1,
 	       ent->num_lines);
-	emit_porcelain_details(suspect, 0);
+	emit_porcelain_details(suspect, repeat);
 
 	cp = nth_line(sb, ent->lno);
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
-		if (cnt)
+		if (cnt) {
 			printf("%s %d %d\n", hex,
 			       ent->s_lno + 1 + cnt,
 			       ent->lno + 1 + cnt);
+			if (repeat)
+				emit_porcelain_details(suspect, 1);
+		}
 		putchar('\t');
 		do {
 			ch = *cp++;
@@ -2307,6 +2312,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('f', "show-name", &output_option, "Show original filename (Default: auto)", OUTPUT_SHOW_NAME),
 		OPT_BIT('n', "show-number", &output_option, "Show original linenumber (Default: off)", OUTPUT_SHOW_NUMBER),
 		OPT_BIT('p', "porcelain", &output_option, "Show in a format designed for machine consumption", OUTPUT_PORCELAIN),
+		OPT_BIT(0, "line-porcelain", &output_option, "Show porcelain format with per-line commit information", OUTPUT_PORCELAIN|OUTPUT_LINE_PORCELAIN),
 		OPT_BIT('c', NULL, &output_option, "Use the same output mode as git-annotate (Default: off)", OUTPUT_ANNOTATE_COMPAT),
 		OPT_BIT('t', NULL, &output_option, "Show raw timestamp (Default: off)", OUTPUT_RAW_TIMESTAMP),
 		OPT_BIT('l', NULL, &output_option, "Show long commit SHA1 (Default: off)", OUTPUT_LONG_OBJECT_NAME),
diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
index 387d1a6..d15f8b3 100755
--- a/t/t8008-blame-formats.sh
+++ b/t/t8008-blame-formats.sh
@@ -68,4 +68,23 @@ test_expect_success 'blame --porcelain output' '
 	test_cmp expect actual
 '
 
+cat >expect <<EOF
+$ID1 1 1 1
+$COMMIT1
+	a
+$ID2 2 2 3
+$COMMIT2
+	b
+$ID2 3 3
+$COMMIT2
+	c
+$ID2 4 4
+$COMMIT2
+	d
+EOF
+test_expect_success 'blame --line-porcelain output' '
+	git blame --line-porcelain file >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.5.rc2.8.gc085

Re: [PATCH 2/3] blame: refactor porcelain output

From: Thiago Farina <hidden>
Date: 2016-06-15 22:51:12

On Mon, May 9, 2011 at 10:34 AM, Jeff King [off-list ref] wrote:
quoted hunk
This is in preparation for adding more porcelain output
options. The three changes are:

 1. emit_porcelain now receives the format option flags

 2. emit_one_suspect_detail takes an optional "repeat"
    parameter to suppress the "show only once" behavior

 3. The code for emitting porcelain suspect is factored
    into its own function for repeatability.

There should be no functional changes.

Signed-off-by: Jeff King <redacted>
---
I broke this out for readability. I can break each of the 3 out into a
separate patch if that helps, but it seemed excessive.

 builtin/blame.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 4242e4b..d74e18f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1484,13 +1484,14 @@ static void write_filename_info(const char *path)
 /*
 * Porcelain/Incremental format wants to show a lot of details per
 * commit.  Instead of repeating this every line, emit it only once,
- * the first time each commit appears in the output.
+ * the first time each commit appears in the output (unless the
+ * user has specifically asked for us to repeat).
 */
-static int emit_one_suspect_detail(struct origin *suspect)
+static int emit_one_suspect_detail(struct origin *suspect, int repeat)
 {
       struct commit_info ci;

-       if (suspect->commit->object.flags & METAINFO_SHOWN)
+       if (!repeat && suspect->commit->object.flags & METAINFO_SHOWN)
Maybe would be worth adding parentheses here:

if (!repeat && (...))
  return 0;

?

Probably is fine as is though.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help