[PATCH 0/4] Miscellaneous Improvements

[PATCH 0/4] Miscellaneous Improvements

[Michael Witten]

Michael Witten (4):
  Typos: t/README
  Clean: Remove superfluous strbuf 'docs'
  Clean: Remove unnecessary `\' (line continuation)
  Clean: Remove useless parameters from both get_commit_info() functions

 builtin/blame.c     |   14 ++++----------
 reflog-walk.c       |   18 ++++--------------
 strbuf.h            |   37 +------------------------------------
 t/README            |   11 +++++------
 t/t8001-annotate.sh |    2 +-
 5 files changed, 15 insertions(+), 67 deletions(-)

-- 
1.7.4.18.g68fe8

[PATCH 1/4] Typos: t/README

[Michael Witten]

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 t/README |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/t/README b/t/README
index ccf6a53..39408b4 100644
--- a/t/README
+++ b/t/README
@@ -201,7 +201,7 @@ we are testing.
 If you create files under t/ directory (i.e. here) that is not
 the top-level test script, never name the file to match the above
 pattern.  The Makefile here considers all such files as the
-top-level test script and tries to run all of them.  A care is
+top-level test script and tries to run all of them.  Care is
 especially needed if you are creating a common test library
 file, similar to test-lib.sh, because such a library file may
 not be suitable for standalone execution.
@@ -248,7 +248,7 @@ This test harness library does the following things:
    consistently when command line arguments --verbose (or -v),
    --debug (or -d), and --immediate (or -i) is given.
 
-Do's, don'ts & things to keep in mind
+Dos, don'ts & things to keep in mind
 -------------------------------------
 
 Here are a few examples of things you probably should and shouldn't do
@@ -285,9 +285,8 @@ Do:
  - Check the test coverage for your tests. See the "Test coverage"
    below.
 
-   Don't blindly follow test coverage metrics, they're a good way to
-   spot if you've missed something. If a new function you added
-   doesn't have any coverage you're probably doing something wrong,
+   Don't blindly follow test coverage metrics; if a new function you added
+   doesn't have any coverage, then you're probably doing something wrong,
    but having 100% coverage doesn't necessarily mean that you tested
    everything.
 
@@ -431,7 +430,7 @@ library for your script to use.
  - test_tick
 
    Make commit and tag names consistent by setting the author and
-   committer times to defined stated.  Subsequent calls will
+   committer times to defined state.  Subsequent calls will
    advance the times by a fixed amount.
 
  - test_commit <message> [<filename> [<contents>]]
-- 
1.7.4.18.g68fe8

[PATCH 2/4] Clean: Remove superfluous strbuf 'docs'

[Michael Witten]

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 strbuf.h |   37 +------------------------------------
 1 files changed, 1 insertions(+), 36 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index f722331..07060ce 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,42 +1,7 @@
 #ifndef STRBUF_H
 #define STRBUF_H
 
-/*
- * Strbuf's can be use in many ways: as a byte array, or to store arbitrary
- * long, overflow safe strings.
- *
- * Strbufs has some invariants that are very important to keep in mind:
- *
- * 1. the ->buf member is always malloc-ed, hence strbuf's can be used to
- *    build complex strings/buffers whose final size isn't easily known.
- *
- *    It is NOT legal to copy the ->buf pointer away.
- *    `strbuf_detach' is the operation that detaches a buffer from its shell
- *    while keeping the shell valid wrt its invariants.
- *
- * 2. the ->buf member is a byte array that has at least ->len + 1 bytes
- *    allocated. The extra byte is used to store a '\0', allowing the ->buf
- *    member to be a valid C-string. Every strbuf function ensures this
- *    invariant is preserved.
- *
- *    Note that it is OK to "play" with the buffer directly if you work it
- *    that way:
- *
- *    strbuf_grow(sb, SOME_SIZE);
- *       ... Here, the memory array starting at sb->buf, and of length
- *       ... strbuf_avail(sb) is all yours, and you are sure that
- *       ... strbuf_avail(sb) is at least SOME_SIZE.
- *    strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
- *
- *    Of course, SOME_OTHER_SIZE must be smaller or equal to strbuf_avail(sb).
- *
- *    Doing so is safe, though if it has to be done in many places, adding the
- *    missing API to the strbuf module is the way to go.
- *
- *    XXX: do _not_ assume that the area that is yours is of size ->alloc - 1
- *         even if it's true in the current implementation. Alloc is somehow a
- *         "private" member that should not be messed with.
- */
+/* See Documentation/technical/api-strbuf.txt */
 
 #include <assert.h>
 
-- 
1.7.4.18.g68fe8

[PATCH 3/4] Clean: Remove unnecessary `\' (line continuation)

[Michael Witten]

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 t/t8001-annotate.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t8001-annotate.sh b/t/t8001-annotate.sh
index 45cb60e..68ac828 100755
--- a/t/t8001-annotate.sh
+++ b/t/t8001-annotate.sh
@@ -8,7 +8,7 @@ PROG='git annotate'
 
 test_expect_success \
     'Annotating an old revision works' \
-    '[ $(git annotate file master | awk "{print \$3}" | grep -c "^A$") -eq 2 ] && \
+    '[ $(git annotate file master | awk "{print \$3}" | grep -c "^A$") -eq 2 ] &&
      [ $(git annotate file master | awk "{print \$3}" | grep -c "^B$") -eq 2 ]'
 
 
-- 
1.7.4.18.g68fe8

[PATCH 4/4] Clean: Remove useless parameters from both get_commit_info() functions

[Michael Witten]

Both `builtin/blame.c' and `reflog-walk.c' have static get_commit_info()
functions:

    $ echo; git grep get_commit_info 7811d9600f02e70c9f835719c71156c967a684f7 | cut -c42-

    builtin/blame.c:static void get_commit_info(struct commit *commit,
    builtin/blame.c:	get_commit_info(suspect->commit, &ci, 1);
    builtin/blame.c:	get_commit_info(suspect->commit, &ci, 1);
    builtin/blame.c:			get_commit_info(suspect->commit, &ci, 1);
    reflog-walk.c:static struct commit_info *get_commit_info(struct commit *commit,
    reflog-walk.c:		get_commit_info(commit, &info->reflogs, 0);

Every time one of the get_commit_info() functions is called, the
last parameter is invariably set to the same constant, rendering that
parameter effectively useless.

This commit removes those last parameters and updates the function bodies
and calls accordingly.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 builtin/blame.c |   14 ++++----------
 reflog-walk.c   |   18 ++++--------------
 2 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index f6b03f7..5b5fc6a 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1413,8 +1413,7 @@ static void get_ac_line(const char *inbuf, const char *what,
 }
 
 static void get_commit_info(struct commit *commit,
-			    struct commit_info *ret,
-			    int detailed)
+			    struct commit_info *ret)
 {
 	int len;
 	const char *subject;
@@ -1447,11 +1446,6 @@ static void get_commit_info(struct commit *commit,
 		    sizeof(author_mail), author_mail,
 		    &ret->author_time, &ret->author_tz);
 
-	if (!detailed) {
-		free(reencoded);
-		return;
-	}
-
 	ret->committer = committer_name;
 	ret->committer_mail = committer_mail;
 	get_ac_line(message, "\ncommitter ",
@@ -1493,7 +1487,7 @@ static int emit_one_suspect_detail(struct origin *suspect)
 		return 0;
 
 	suspect->commit->object.flags |= METAINFO_SHOWN;
-	get_commit_info(suspect->commit, &ci, 1);
+	get_commit_info(suspect->commit, &ci);
 	printf("author %s\n", ci.author);
 	printf("author-mail %s\n", ci.author_mail);
 	printf("author-time %lu\n", ci.author_time);
@@ -1664,7 +1658,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
 	char hex[41];
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
 
-	get_commit_info(suspect->commit, &ci, 1);
+	get_commit_info(suspect->commit, &ci);
 	strcpy(hex, sha1_to_hex(suspect->commit->object.sha1));
 
 	cp = nth_line(sb, ent->lno);
@@ -1850,7 +1844,7 @@ static void find_alignment(struct scoreboard *sb, int *option)
 			longest_file = num;
 		if (!(suspect->commit->object.flags & METAINFO_SHOWN)) {
 			suspect->commit->object.flags |= METAINFO_SHOWN;
-			get_commit_info(suspect->commit, &ci, 1);
+			get_commit_info(suspect->commit, &ci);
 			if (*option & OUTPUT_SHOW_EMAIL)
 				num = utf8_strwidth(ci.author_mail);
 			else
diff --git a/reflog-walk.c b/reflog-walk.c
index 5d81d39..3357331 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -87,22 +87,12 @@ struct commit_info_lifo {
 };
 
 static struct commit_info *get_commit_info(struct commit *commit,
-		struct commit_info_lifo *lifo, int pop)
+		struct commit_info_lifo *lifo)
 {
 	int i;
 	for (i = 0; i < lifo->nr; i++)
-		if (lifo->items[i].commit == commit) {
-			struct commit_info *result = &lifo->items[i];
-			if (pop) {
-				if (i + 1 < lifo->nr)
-					memmove(lifo->items + i,
-						lifo->items + i + 1,
-						(lifo->nr - i) *
-						sizeof(struct commit_info));
-				lifo->nr--;
-			}
-			return result;
-		}
+		if (lifo->items[i].commit == commit)
+			return &lifo->items[i];
 	return NULL;
 }
 
@@ -214,7 +204,7 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)
 {
 	struct commit_info *commit_info =
-		get_commit_info(commit, &info->reflogs, 0);
+		get_commit_info(commit, &info->reflogs);
 	struct commit_reflog *commit_reflog;
 	struct reflog_info *reflog;
 
-- 
1.7.4.18.g68fe8

Re: [PATCH 3/4] Clean: Remove unnecessary `\' (line continuation)

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
>  t/t8001-annotate.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/t/t8001-annotate.sh b/t/t8001-annotate.sh
> index 45cb60e..68ac828 100755
> --- a/t/t8001-annotate.sh
> +++ b/t/t8001-annotate.sh
> @@ -8,7 +8,7 @@ PROG='git annotate'
>  
>  test_expect_success \
>      'Annotating an old revision works' \
> -    '[ $(git annotate file master | awk "{print \$3}" | grep -c "^A$") -eq 2 ] && \
> +    '[ $(git annotate file master | awk "{print \$3}" | grep -c "^A$") -eq 2 ] &&
>       [ $(git annotate file master | awk "{print \$3}" | grep -c "^B$") -eq 2 ]'

While this is not wrong per-se, I don't want to take too much half-way
churning.

If we were to properly do this, we should first rewrite it to use the more
modern style:

	test_expect_success 'Annotating an old revision works' '
		... test script comes here ...
        '

and just run annotate once without having any downstream pipe, i.e.

	git annotate file master >result &&
	awk "{ print \$3; }" <result >authors &&
	test 2 = $(grep A <authors | wc -l) &&
	test 2 = $(grep B <authors | wc -l)

so that we can catch breakage in "git annotate" itself more reliably
(e.g. even if the command showed two lines for each author, it is a
failure if the command itself did not exit with status 0).

Re: [PATCH 1/4] Typos: t/README

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> @@ -248,7 +248,7 @@ This test harness library does the following things:
>     consistently when command line arguments --verbose (or -v),
>     --debug (or -d), and --immediate (or -i) is given.
>  
> -Do's, don'ts & things to keep in mind
> +Dos, don'ts & things to keep in mind
>  -------------------------------------

A quick googling seem to indicate that both forms are accepted and widely
used (72k hits for "Do's and Don'ts" vs 45k hits for "Dos and Don'ts" in
www.google.com/books search), so I'd rather drop this hunk.

But all others are unambiguous improvements.  Thanks.

Re: [PATCH 1/4] Typos: t/README

[Michael Witten]

On Wed, Mar 30, 2011 at 13:58, Junio C Hamano <gitster@pobox.com> wrote:
> A quick googling seem to indicate that both forms are accepted and widely
> used (72k hits for "Do's and Don'ts" vs 45k hits for "Dos and Don'ts" in
> www.google.com/books search), so I'd rather drop this hunk.

*grrrrumble grumble grumble* :-)

OK.