git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Chen Jingpiao <chenjingpiao@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [GSoC][PATCH] commit: add a commit.signOff config variable
Date: Sun, 4 Feb 2018 06:31:35 -0500	[thread overview]
Message-ID: <20180204113135.GA28130@flurp.local> (raw)
In-Reply-To: <20180204020318.4363-1-chenjingpiao@gmail.com>

On Sun, Feb 04, 2018 at 10:03:18AM +0800, Chen Jingpiao wrote:
> Add the commit.signOff configuration variable to use the -s or --signoff
> option of git commit by default.
> 
> Signed-off-by: Chen Jingpiao <chenjingpiao@gmail.com>
> ---
> 
> Though we can configure signoff using format.signOff variable. Someone like to
> add Signed-off-by line by the committer.

This commentary explains why this feature is desirable, therefore it
would be a good idea to include this in the commit message itself.

> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> @@ -505,6 +505,75 @@ Myfooter: x" &&
> +test_expect_success "commit.signoff=true and --signoff omitted" '
> +	echo 7 >positive &&
> +	git add positive &&
> +	git -c commit.signoff=true commit -m "thank you" &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
> +	(
> +		echo thank you
> +		echo
> +		git var GIT_COMMITTER_IDENT |
> +		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
> +	) >expected &&
> +	test_cmp expected actual
> +'

The bodies of these test are quite noisy, doing a lot of work that isn't
really necessary, which makes it difficult to figure out what is really
being tested. Other tests in this script already check that the commit
message is properly formatted when Signed-off-by: is inserted so you
don't need to repeat all that boilerplate here.

Instead, you are interested only in whether or not Signed-off-by: has
been added to the message. For that purpose, you can use a simple 'grep'
expression.

The amount of copy/paste code in these six tests is also unfortunate.
Rather than merely repeating the same code over and over, you could
instead parameterize the test. For instance, you could run all six tests
via a simple for-loop:

--- >8 ---
for cfg in true false
do
    for opt in '' --signoff --no-signoff
    do
        case "$opt:$cfg" in
        --signoff:*|:true) expect= ;;
        --no-signoff:*|:false) expect=! ;;
        esac
        test_expect_success "commit.signoff=$cfg & ${opt:---signoff omitted}" '
            git -c commit.signoff=$cfg commit --allow-empty -m x $opt &&
            eval "$expect git log -1 --format=%B | grep ^Signed-off-by:"
        '
    done
done
--- >8 ---

A final consideration is that tests run slowly on Windows, and although
it's nice to be thorough by testing all six combinations, you can
probably exercise the new code sufficiently by instead testing just two
combinations. For instance, instead of all six combinations, test just
these two:

--- >8 ---
test_expect_success 'commit.signoff=true & --signoff omitted' '
    git -c commit.signoff=true commit --allow-empty -m x &&
    git log -1 --format=%B | grep ^Signed-off-by:
'

test_expect_success 'commit.signoff=true & --no-signoff' '
    git -c commit.signoff=true commit --allow-empty -m x --no-signoff &&
    ! git log -1 --format=%B | grep ^Signed-off-by:
'
--- >8 ---

> +test_expect_success "commit.signoff=true and --signoff" '
> +	echo 8 >positive &&
> +	git add positive &&
> +	git -c commit.signoff=true commit --signoff -m "thank you" &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
> +	(
> +		echo thank you
> +		echo
> +		git var GIT_COMMITTER_IDENT |
> +		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
> +	) >expected &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success "commit.signoff=true and --no-signoff" '
> +	echo 9 >positive &&
> +	git add positive &&
> +	git -c commit.signoff=true commit --no-signoff -m "thank you" &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
> +	echo thank you >expected &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success "commit.signoff=false and --signoff omitted" '
> +	echo 10 >positive &&
> +	git add positive &&
> +	git -c commit.signoff=false commit -m "thank you" &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
> +	echo thank you >expected &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success "commit.signoff=false and --signoff" '
> +	echo 11 >positive &&
> +	git add positive &&
> +	git -c commit.signoff=false commit --signoff -m "thank you" &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
> +	(
> +		echo thank you
> +		echo
> +		git var GIT_COMMITTER_IDENT |
> +		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
> +	) >expected &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success "commit.signoff=false and --no-signoff" '
> +	echo 12 >positive &&
> +	git add positive &&
> +	git -c commit.signoff=false commit --no-signoff -m "thank you" &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
> +	echo thank you >expected &&
> +	test_cmp expected actual
> +'

  reply	other threads:[~2018-02-04 11:31 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-04  2:03 [GSoC][PATCH] commit: add a commit.signOff config variable Chen Jingpiao
2018-02-04 11:31 ` Eric Sunshine [this message]
2018-02-04 18:59   ` Ævar Arnfjörð Bjarmason
2018-02-05 21:50 ` Stefan Beller
2018-02-06  7:01   ` Chen Jingpiao

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=20180204113135.GA28130@flurp.local \
    --to=sunshine@sunshineco.com \
    --cc=chenjingpiao@gmail.com \
    --cc=git@vger.kernel.org \
    /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).