Re: [PATCH 2/2] format-patch: add --reviewed-by=<ident>

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>  'git-format-patch' [-k] [-o <dir> | --stdout] [--thread]
>  		   [--attach[=<boundary>] | --inline[=<boundary>]]
> +		   [--reviewed-by=<ident>]
>  		   [-s | --signoff] [<common diff options>]
>  		   [-n | --numbered | -N | --no-numbered]
>  		   [--start-number <n>] [--numbered-files]

What's the expected workflow this patch intends to help?

 - You see a patch by somebody, you look at it deeply, you apply to your
   tree (presumably with your own Signed-off-by).

 - You inspect the result further, and decide it is good.

 - You format-patch with the option, which would now have a Reviewed-by:
   too.

 - You send it out.

If so, it might make sense to simply always use the committer ident.

If the person who adds the reviewed-by is trusted so much that her
reviewed-by counts, the commits might even be transfered with "Please
pull".  In such a case, the workflow might become:

 - You see a patch by somebody, you look at it deeply, you apply to your
   tree (presumably with your own Signed-off-by).

 - You inspect the result further, and decide it is good.

 - You run "rebase --add-reviewed-by" to prepare a series on a branch to
   be pulled from.

 - You send a request-pull.

In that workflow, it would also make sense to use the committer ident.

I am trying to come up with a plausible workflow that wants to add
somebody else's reviewed-by.

 - You send out your patch to the list.  People give comments, you reroll,
   you get more comments, eventually people say "Ah, that's good, Ack."
   and/or "I am not the primary person who knows this area, but I reviewed
   it and I know my reviewed-by would count, so here is my Ok".

 - You format-patch the final version, with Acked-by and Reviewed-by
   adding other people's names.

Then I think it makes sense to take names of other people if that is the
case.

You probably meant that that is the expected workflow, as you can give
more than one of these options.

But people who read the documentation should not have to guess.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index b2b7a8d..e2ff94f 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -230,4 +230,30 @@ test_expect_success 'shortlog of cover-letter wraps overly-long onelines' '
>  
>  '
>  
> +cat > expect << EOF
> +
> +Reviewed-by: Ken Robinson
> +
> +Reviewed-by: Sergey Rachmaninov
> +
> +Reviewed-by: Ronny O Sullivan
> +Reviewed-by: Mickey Mouse
> +Reviewed-by: Mahatma Gandhi
> +EOF
> +
> +test_expect_success '--reviewed-by' '
> +
> +	echo reviewed > foo &&
> +	test_tick &&
> +	git commit -m "Reviewed" -m "Reviewed-by: Ken Robinson" \
> +		-m "Reviewed-by: Sergey Rachmaninov" \
> +		-m "Reviewed-by: Ronny O Sullivan" foo &&
> +	git format-patch --reviewed-by="Mickey Mouse" \
> +		--reviewed-by="Sergey Rachmaninov" \
> +		--reviewed-by="Mahatma Gandhi" -1 HEAD &&
> +	sed -e "1,/^Cc: /d" -e "/^---/,\$d" < 0001-Reviewed.patch > output &&
> +	git diff expect output
> +
> +'

Why not use a single -m for the first three reviewed-bys, instead of
making them into separate paragraphs using multiple -m?