git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 4/5] t4202-log: add tests for --merges=

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Apr 7, 2015, 07:32 UTC
Message-ID
<20150407073257.GA45051@flurp.local>
In-Reply-To
<1428110521-31028-4-git-send-email-koosha@posteo.de>
On Sat, Apr 04, 2015 at 03:22:00AM +0200, Koosha Khajehmoogahi wrote:
Show 23 quoted lines
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 1b2e981..ceaaf4e 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -270,6 +270,90 @@ cat > expect <<\EOF
>  * initial
>  EOF
>  
> +test_expect_success 'setup --merges=' '
> +	git log --min-parents=2 --pretty=tformat:%s >expect_only &&
> +	git log --max-parents=1 --pretty=tformat:%s >expect_hide &&
> +	git log --min-parents=-1 --pretty=tformat:%s >expect_show
> +'
> +
> +test_expect_success 'log with config log.merges=show' '
> +	test_config log.merges show &&
> +	git log --pretty=tformat:%s >actual &&
> +	test_cmp expect_show actual
> +'
> +
[...much repetitious code omitted...]
Show 6 quoted lines
> +test_expect_success 'log with config log.merges=show with git log --no-merges' '
> +	test_config log.merges show &&
> +	git log --no-merges --pretty=tformat:%s >actual &&
> +	test_cmp expect_hide actual
> +'
> +
[...much repetitious code omitted...]
Show 9 quoted lines
> +test_log_merges() {
> +	test_config log.merges $2 &&
> +	git log --merges=$1 --pretty=tformat:%s >actual &&
> +	test_cmp $3 actual
> +}
> +
> +test_expect_success 'log --merges=show with config log.merges=hide' '
> +	test_log_merges show hide expect_show
> +'
[...much repetitious code omitted...]

In my previous review[1], I mentioned that the significant repetition in the implementation of the new tests increased potential for errors when composing them, and made it more difficult to review. To resolve the issue, I suggested factoring out the repeated code into a helper function, and gave an idea of its interface and how it could be applied to eliminate all the repetition.

In this version of the patch, you've made some progress at eliminating repetition, but much still remains. I had hoped to see much more aggressive repetition elimination, collapsing the code to the bare minimum while still testing all combinations of states. Here's what I had in mind:

---- 8< ----
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1b2e981..be6d34c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -270,6 +270,35 @@ cat > expect <<\EOF
 * initial
 EOF
 
+test_expect_success 'setup merges' '
+	git log --min-parents=2 --pretty=tformat:%s >expect_only &&
+	git log --max-parents=1 --pretty=tformat:%s >expect_hide &&
+	git log --min-parents=-1 --pretty=tformat:%s >expect_show
+'
+
+test_log_merges() {
+	expect=expect_$1
+	config=${2:+-c log.merges=$2}
+	option=${3:+--merges=$3}
+	option=${4:-$option}
+	test_expect_success "merges${config:+ $config}${option:+ $option}" "
+		git $config log $option --pretty=tformat:%s >actual &&
+		test_cmp $expect actual
+	"
+}
+
+states="show only hide"
+for c in '' $states
+do
+	for o in '' $states
+	do
+		test_log_merges ${o:-${c:-show}} "$c" "$o"
+	done
+done
+
+test_log_merges hide show '' --no-merges
+test_log_merges only hide '' '--merges --max-parents=2'
+
 test_expect_success 'log --graph with merge' '
 	git log --graph --date-order --pretty=tformat:%s |
 		sed "s/ *\$//" >actual &&
---- 8< ----

The output of the above test code looks like this:

---- 8< ----
ok 29 - setup merges
ok 30 - merges
ok 31 - merges --merges=show
ok 32 - merges --merges=only
ok 33 - merges --merges=hide
ok 34 - merges -c log.merges=show
ok 35 - merges -c log.merges=show --merges=show
ok 36 - merges -c log.merges=show --merges=only
ok 37 - merges -c log.merges=show --merges=hide
ok 38 - merges -c log.merges=only
ok 39 - merges -c log.merges=only --merges=show
ok 40 - merges -c log.merges=only --merges=only
ok 41 - merges -c log.merges=only --merges=hide
ok 42 - merges -c log.merges=hide
ok 43 - merges -c log.merges=hide --merges=show
ok 44 - merges -c log.merges=hide --merges=only
ok 45 - merges -c log.merges=hide --merges=hide
ok 46 - merges -c log.merges=show --no-merges
ok 47 - merges -c log.merges=hide --merges --max-parents=2
---- 8< ----

Hmm, it seems that I've rewritten pretty much the entire patch, so
perhaps the From: header ought to mention my name instead. Oops.

[1]: http://git.661346.n2.nabble.com/PATCH-1-5-Add-a-new-option-merges-to-revision-c-td7627662.html#a7627683
Previous: Koosha KhajehmoogahiNext: Koosha Khajehmoogahi
Message 12 of 14 in “revision: add --merges={show|only|hide} option”
  1. 1/5 revision: add --merges={show|only|hide} optionKoosha Khajehmoogahi, Apr 4, 2015
  2. 2/5 log: honor log.merges= optionKoosha Khajehmoogahi, Apr 4, 2015
  3. Junio C HamanoApr 4, 2015
  4. Koosha KhajehmoogahiApr 7, 2015
  5. Junio C HamanoApr 8, 2015
  6. Koosha KhajehmoogahiApr 8, 2015
  7. Junio C HamanoApr 13, 2015
  8. Eric SunshineApr 7, 2015
  9. 3/5 Documentation: add git-log --merges= option and log.merges config. varKoosha Khajehmoogahi, Apr 4, 2015
  10. Junio C HamanoApr 5, 2015
  11. 4/5 t4202-log: add tests for --merges=Koosha Khajehmoogahi, Apr 4, 2015
  12. Eric SunshineApr 7, 2015
  13. 5/5 bash-completion: add support for git-log --merges= and log.mergesKoosha Khajehmoogahi, Apr 4, 2015
  14. Eric SunshineApr 7, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.