Hi,

On Wed, 5 Mar 2008, Junio C Hamano wrote:

quoted text
> 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?
> 
> [...]
>
> 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.

Yes, exactly.  That was the usage I had in mind.

The problem with the two others you suggested (basically that the reviewer 
herself sends out another patch series) is a little fragile, since a 
malicous reviewer could add/modify code, but for nice reviewers, it is 
better in that you know which code was reviewed.

So I'd rather have it not resent by the reviewer as a patch series, but I 
think that pulling it is fine, since it is much easier to verify (with log 
--cherry-pick) that no code was tampered with by the reviewer.

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

Frankly, I did not make up my mind on the other workflows, I only wanted 
to make the maintainer's life (yours) easier.

I have no problem posting several revisions of this patch, or scrap it 
altogether... as somebody recently said, we even joke around here via 
patches ;-)

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

I am always unsure how to embed newlines in strings in a shell script 
(except with single-ticks), and besides, I do not like breaking the 
indentation.  But I could make it a temporary file... Hmm?

Ciao,
Dscho

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html