git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Abhishek Kumar <abhishekkumar8222@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [GSoC Patch 1/5] lib-log-graph.sh: consolidate test_cmp_graph logic
Date: Sun, 16 Feb 2020 16:05:52 -0800	[thread overview]
Message-ID: <xmqqk14mm61r.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20200216134750.18947-1-abhishekkumar8222@gmail.com> (Abhishek Kumar's message of "Sun, 16 Feb 2020 19:17:46 +0530")

Abhishek Kumar <abhishekkumar8222@gmail.com> writes:

> Logic for comparing log graphs is duplicated across test scripts.
> ...
>  t/lib-log-graph.sh | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 t/lib-log-graph.sh

The presentation order of the patches may be less than ideal, in
that it introduces totally unused code in step 1/5 that is hard to
compare with what it will be used to replace with, and it is
impossible to tell if the potential issues readers see in this step
are merely inherited from existing tests or new issues introduced by
this series, before reading the later steps.

> diff --git a/t/lib-log-graph.sh b/t/lib-log-graph.sh
> new file mode 100644
> index 0000000000..999f2600de
> --- /dev/null
> +++ b/t/lib-log-graph.sh
> @@ -0,0 +1,39 @@
> +# Helpers shared by the test scripts for comparing log graphs.
> +
> +sanitize_output() {

One SP around both sides of ().  I suspect that all helper functions
in this patch has this style violation.

As a library-ish function that can be used outside individual test
script, "output" without any clarification is too broad a word to
act as an object of sanitizing.  Is this function to sanitize the
output from "git log"?  Perhaps at the minimum, it should be called
sanitize_log_output then.

> +	sed -e 's/ *$//' \
> +	    -e 's/commit [0-9a-f]*$/commit COMMIT_OBJECT_NAME/' \
> +	    -e 's/Merge: [ 0-9a-f]*$/Merge: MERGE_PARENTS/' \
> +	    -e 's/Merge tag.*/Merge HEADS DESCRIPTION/' \
> +	    -e 's/Merge commit.*/Merge HEADS DESCRIPTION/' \

These are understandable anonymization; so is the last "index" one.

> +	    -e 's/, 0 deletions(-)//' \
> +	    -e 's/, 0 insertions(+)//' \
> +	    -e 's/ 1 files changed, / 1 file changed, /' \
> +	    -e 's/, 1 deletions(-)/, 1 deletion(-)/' \
> +	    -e 's/, 1 insertions(+)/, 1 insertion(+)/' \

These might deserve comments.  IIUC, all of these are historical
accident and no longer necessary.

> +	    -e 's/index [0-9a-f]*\.\.[0-9a-f]*/index BEFORE..AFTER/'
> +}
> +
> +# Assume expected graph is in file `expect`
> +test_cmp_graph_file() {
> +	git log --graph "$@" >output &&
> +	sanitize_output >output.trimmed <output &&

Pay attention to the names.  If you are "sanitizing", then the
result is not "trimmed".  Call it "sanitized".

> +	test_i18ncmp expect output.trimmed
> +}
> +
> +test_cmp_graph() {
> +	cat >expect &&
> +	test_cmp_graph_file "$@"
> +}

I am not sure if this wrapper is useful or obscuring.  Open coding
the caller of this wrapper, i.e.

	cat >expect <<-\EOF &&
	expected pattern
	EOF
	test_cmp_graph_file $args

is not all that cumbersome, and it might make it more transparent to
the readers what is going on.  I'd need to see the callsites in
later steps to decide it is a good idea.

> +# Assume expected graph is in file `expect.colors`
> +test_cmp_colored_graph_file() {
> +	git log --graph --color=always "$@" >output.colors.raw &&
> +	test_decode_color <output.colors.raw | sed "s/ *\$//" >output.colors &&
> +	test_cmp expect.colors output.colors
> +}
> +
> +test_cmp_colored_graph() {
> +	cat >expect.colors &&
> +	test_cmp_colored_graph_file "$@"
> +}

So unlike test_cmp_graph family, colored counterparts do not
anonymize?  That sounds a bit harder to use, but we cannot really
tell if that is an issue before seeing the callsites in later steps.

Thanks.

  parent reply	other threads:[~2020-02-17  0:06 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-16 13:47 [GSoC Patch 1/5] lib-log-graph.sh: consolidate test_cmp_graph logic Abhishek Kumar
2020-02-16 13:47 ` [GSoC Patch 2/5] t3430: use lib-log-graph functions Abhishek Kumar
2020-02-19 17:23   ` Junio C Hamano
2020-02-16 13:47 ` [GSoC Patch 3/5] t4215: " Abhishek Kumar
2020-02-19 17:26   ` Junio C Hamano
2020-02-16 13:47 ` [GSoC Patch 4/5] t4214: " Abhishek Kumar
2020-02-19 17:29   ` Junio C Hamano
2020-02-16 13:47 ` [GSoC Patch 5/5] t4202: " Abhishek Kumar
2020-02-19 17:31   ` Junio C Hamano
2020-02-17  0:05 ` Junio C Hamano [this message]
2020-02-19 17:32   ` [GSoC Patch 1/5] lib-log-graph.sh: consolidate test_cmp_graph logic Junio C Hamano
2020-02-20  9:15 ` [GSoC PATCH v2 0/2] Consolidate " Abhishek Kumar
2020-02-20  9:15   ` [GSoC PATCH v2 1/2] lib-log-graph: consolidate " Abhishek Kumar
2020-02-20 17:49     ` Junio C Hamano
2020-02-20  9:15   ` [PATCH v2 2/2] lib-log-graph: consolidate colored graph cmp logic Abhishek Kumar
2020-02-24 13:38 ` [GSoC Patch v3 1/2] lib-log-graph: consolidate test_cmp_graph logic Abhishek Kumar
2020-02-24 13:38   ` [GSoC Patch v3 2/2] lib-log-graph: consolidate colored graph cmp logic Abhishek Kumar
2020-02-24 21:17   ` [GSoC Patch v3 1/2] lib-log-graph: consolidate test_cmp_graph logic Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqk14mm61r.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=abhishekkumar8222@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).