"W. Trevor King" <wk...@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 <wk...@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.
