Re: [PATCH] pull: pass --signoff/--no-signoff to "git merge"

[Junio C Hamano <gitster@pobox.com>]

"W. Trevor King" <wking@tremily.us> writes:

> Following 09c2cb87 (pull: pass --allow-unrelated-histories to "git
> merge", 2016-03-18) with the tests also drawing on 14d01b4f (merge:
> add a --signoff flag, 2017-07-04).

I cannot find a verb in the above.

> The order of options in merge-options.txt isn't clear to me, but I've
> put --signoff between --log and --stat as somewhat alphabetized and
> having an "add to the commit message" function like --log.
>
> The tests aren't as extensive as t7614-merge-signoff.sh, but they
> exercises both the --signoff and --no-signoff options.  There may be a
> more efficient way to set them up (like t7614-merge-signoff.sh's
> test_setup), but with all the pull options packed into a single test
> script it seemed easiest to just copy/paste the duplicate setup code.

The above two paragraphs read more like "requesting help for hints
to improve this patch" than commit log message.  Perhaps move them
below the three-dash line and instead describe what you actually did
here (if they were worth explaining, that is)?

> 09c2cb87 didn't motivate the addition of --allow-unrelated-histories
> to pull; only citing the reason from e379fdf3 (merge: refuse to create
> too cool a merge by default, 2016-03-18) gave for *not* including it.
> I like having both exposed in pull because while the fetch-and-merge
> approach might be a more popular way to judge "how well they fit
> together", you can also do that after an optimistic pull.  And in
> cases where an optimistic pull is likely to succeed, suggesting it is
> easier to explain to Git newbies than a FETCH_HEAD merge.

I find this paragraph totally unrelated to what the patch does.
Save it for the patch you add to pass --allow-unrelated-histories
given to pull down to underlying merge, perhaps?

>
> Signed-off-by: W. Trevor King <wking@tremily.us>
> ---
>  Documentation/git-merge.txt     |  8 --------
>  Documentation/merge-options.txt | 10 ++++++++++
>  builtin/pull.c                  |  8 ++++++++
>  t/t5521-pull-options.sh         | 43 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 61 insertions(+), 8 deletions(-)
> ...
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index ded8f98dbe..d95789ab8c 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -165,4 +165,47 @@ test_expect_success 'git pull --allow-unrelated-histories' '
>  	)
>  '
>  
> +test_expect_success 'git pull --signoff add a sign-off line' '
> +	test_when_finished "rm -fr src dst actual expected" &&
> +	cat >expected <<-EOF &&
> +		Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
> +	EOF

	echo "Signed-off-by: $GIT_COMMITER_NAME <$GIT_COMMITTER_EMAIL>" >expect

or

	git var GIT_COMMITTER_IDENT |
	sed -e 's/^\([^>]*>\).*/Signed-off-by: \1/' >expect

> +	git init src &&
> +	(
> +		cd src &&
> +		test_commit one
> +	) &&

I suspect somebody will suggest "test_commit -C" ;-)

> +	git clone src dst &&
> +	(
> +		cd src &&
> +		test_commit two
> +	) &&
> +	(
> +		cd dst &&
> +		git pull --signoff --no-ff &&
> +		git cat-file commit HEAD | tail -n1 >../actual

I think it makes it more robust to replace "tail" with "collect all
the signed-off-by lines" like the other test (below) does.  Perhaps
have a helper function and use it in both?

	get_signoff () {
		git cat-file commit "$1" | sed -n -e '/^Signed-off-by: /p'
	}

Some may say "cat-file can fail, and having it on the LHS of a pipe
hides its failure", advocating for something like:

	get_signoff () {
		git cat-file commit "$1" >sign-off-temp &&
		sed -n -e '/^Signed-off-by: /p' sign-off-temp
	}

> +	) &&
> +	test_cmp expected actual
> +'

> +test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '
> +	test_when_finished "rm -fr src dst actual" &&
> +	git init src &&
> +	(
> +		cd src &&
> +		test_commit one
> +	) &&
> +	git clone src dst &&
> +	(
> +		cd src &&
> +		test_commit two
> +	) &&
> +	(
> +		cd dst &&
> +		git pull --signoff --no-signoff --no-ff &&
> +		git cat-file commit HEAD | sed -n /Signed-off-by/p >../actual
> +	) &&
> +	test_must_be_empty actual
> +'
> +
>  test_done

I think "--signoff" and "--signoff --no-signoff" are reasonable
minimum things to test.  Two more cases, i.e. running it without
either and with "--no-signoff" alone, to ensure that the sign-off
mechanism does not kick in would make it even better.

Thanks.