On Thu, Oct 12, 2017 at 10:17:51AM +0900, Junio C Hamano wrote:
> "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.

I'd meant it as either a continuation of the subject line, or with an
implicit leading “I did this…” :p.  I can reword if you like, maybe
just “Following” → “Follow”?  Something more drastic?

> > 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)?

I think something about merge-options.txt ordering should end up in
the history of that content.  Reading through:

  $ git log Documentation/merge-options.txt

only turned up 690b2975 (Documentation/merge-options.txt: group "ff"
related options together, 2012-02-22) discussing option order (it
suggested grouping similar options together, although --ff and
--ff-only would also be close alphabetically).

I agree that the first paragraph you quote above doesn't have me
coming down firmly in favor of a particular ordering strategy, but I
think having something like it in the Git history will help whoever
ends up giving merge-options.txt a well-defined strategy by showing I
didn't have any strong opinions to account for ;).  Silence can mean
“doesn't have a strong opinion”, but sometimes it means “feels the
choice is so obvious that it doesn't need explicit motivation”.

I'm fine moving the second paragraph you quote below the fold in a v2,
although you're calling for more tests below, and it won't apply
anymore once I've added those :).

> > 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?

09c2cb87 is your commit in master (v2.9.0-rc0~88^2) that is doing just
that.  I haven't gone through the list history to figure out why it
ended up getting landed with its current commit message; “Prepare a
patch to make it a reality, just in case it is needed” sounds more
like it was “here's the code in case folks want it, I'll reroll the
motivation if they do”.  This paragraph was aiming to motivate both
the --signoff pass-through I'm adding here and (retroactively) the
--allow-unrelated-histories pass-through you added there.  I'll add
more context in v2 to try to make that more clear.

> > +	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

Much nicer, thanks.  I'll add a patch to v2 to make the same change to
t7614.

> > +	git init src &&
> > +	(
> > +		cd src &&
> > +		test_commit one
> > +	) &&
> 
> I suspect somebody will suggest "test_commit -C" ;-)

Sounds good.  I'll add a patch to v2 to make the same change to the
existing t5521 --allow-unrelated-histories test.

> > +	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
> 	}

There are several existing consumers using grep and sed for this:

  wking@ullr ~/src/git/git $ git grep Signed-off-by v2.15.0-rc1 -- 't/*.sh' | grep 'grep\|sed'
  …
  v2.15.0-rc1:t/t3501-revert-cherry-pick.sh:      git cat-file commit HEAD | grep ^Signed-off-by: >signoff &&
  v2.15.0-rc1:t/t3507-cherry-pick-conflict.sh:    test_i18ngrep -e "Signed-off-by" .git/MERGE_MSG
  v2.15.0-rc1:t/t3507-cherry-pick-conflict.sh:    test $(git show -s |grep -c "Signed-off-by") = 1
  v2.15.0-rc1:t/t3507-cherry-pick-conflict.sh:    grep -e "^# Conflicts:" -e '^Signed-off-by' <.git/COMMIT_EDITMSG >actual &&
  v2.15.0-rc1:t/t3507-cherry-pick-conflict.sh:    grep -e "^Conflicts:" -e '^Signed-off-by' <.git/COMMIT_EDITMSG >actual &&
  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    ! grep "Signed-off-by:" initial_msg &&
  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    grep "Signed-off-by:" unrelatedpick_msg &&
  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    ! grep "Signed-off-by:" picked_msg &&
  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    grep "Signed-off-by:" anotherpick_msg
  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    ! grep Signed-off-by: msg
  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    ! grep Signed-off-by: msg
  v2.15.0-rc1:t/t4014-format-patch.sh:    grep "^Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" out
  v2.15.0-rc1:t/t4014-format-patch.sh:    ! sed "/^Signed-off-by: /q" out | grep "test message" &&
  v2.15.0-rc1:t/t4014-format-patch.sh:    sed "1,/^Signed-off-by: /d" out | grep "test message" &&
  v2.15.0-rc1:t/t4153-am-resume-override-opts.sh: git cat-file commit HEAD^ | grep "Signed-off-by:" >actual &&
  v2.15.0-rc1:t/t4153-am-resume-override-opts.sh: test $(git cat-file commit HEAD | grep -c "Signed-off-by:") -eq 0
  v2.15.0-rc1:t/t7501-commit.sh:          sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
  v2.15.0-rc1:t/t7501-commit.sh:          sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
  v2.15.0-rc1:t/t7501-commit.sh:          sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
  v2.15.0-rc1:t/t7501-commit.sh:          sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
  v2.15.0-rc1:t/t7502-commit.sh:  actual=$(git cat-file commit HEAD | sed -ne "s/Signed-off-by: //p") &&
  v2.15.0-rc1:t/t7614-merge-signoff.sh:Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")

Perhaps we want something like:

  test_has_trailer $OBJECT $TOKEN $VALUE

and:

  test_has_no_trailer $OBJECT $TOKEN

in test-lib-functions.sh that we can use in all of these cases?

> 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.

Sounds good, I'll add those in v2.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy