Hi, On Wed, 5 Mar 2008, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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. > 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 ;-) > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
