Re: [PATCH v2 04/10] sequencer.c: recognize (cherry picked from ... as part of s-o-b footer

2013-01-27 Thread Jonathan Nieder
Hi,

Brandon Casey wrote:

 Let's detect (cherry picked from...) as part of the footer so that we
 will produce this:

Signed-off-by: A U Thor aut...@example.com
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C O Mmitter commit...@example.com

 instead of this:

Signed-off-by: A U Thor aut...@example.com
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)

Signed-off-by: C O Mmitter commit...@example.com

My last review was based on a misunderstanding of
ends_rfc2822_footer().  This time I can unreservedly say I like this.

[...]
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -18,6 +18,7 @@
  #define GIT_REFLOG_ACTION GIT_REFLOG_ACTION
  
  const char sign_off_header[] = Signed-off-by: ;
 +static const char cherry_picked_prefix[] = (cherry picked from commit ;

The static/nonstatic mismatch looks strange.  Not about this patch,
but probably rest_is_empty() from builtin-commit.c should move here
some day.

[...]
 @@ -1021,11 +1022,36 @@ int sequencer_pick_revisions(struct replay_opts *opts)
[...]
 +static int is_cherry_picked_from_line(const char *buf, int len)
 +{
 + /*
 +  * We only care that it looks roughly like (cherry picked from ...)
 +  */
 + return !prefixcmp(buf, cherry_picked_prefix) 
 + (buf[len - 1] == ')' ||
 +  (buf[len - 1] == '\n'  buf[len - 2] == ')'));

These two cases are based on whether the commit message ended with
a '(cherry picked from' with no trailing newline, I guess?

I wonder if switching the convention for 'k' would make this simpler:

const char *line, *eol;

line = buf + i;
eol = memchr(buf, '\n', len - i);
if (!eol)
eol = buf + len;

if (!is_rfc2822_field(line, eol - line) 
!is_cherry_picked_from_line(line, eol - line))
return 0;

I notice you just sent a new version of the series.  I'll try this out
on top of that.

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


Re: [PATCH v2 04/10] sequencer.c: recognize (cherry picked from ... as part of s-o-b footer

2013-01-22 Thread Jonathan Nieder
Brandon Casey wrote:

 Let's detect (cherry picked from...) as part of the footer so that we
 will produce this:

Signed-off-by: A U Thor aut...@example.com
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C O Mmitter commit...@example.com

 instead of this:

Signed-off-by: A U Thor aut...@example.com
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)

Signed-off-by: C O Mmitter commit...@example.com

Yes, looks sane.

A downside is that this produces an arguably worse result when using
-x -s for a commit that does not already have a sign-off.  Before,
we had:

test: do something great

Do something fantastic in a clean and elegant way that
only takes two lines of explanation.
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)

Signed-off-by: C H Errypicker cherry-pic...@example.com

Afterwards, we will have:

test: do something great

Do something fantastic in a clean and elegant way that
only takes two lines of explanation.
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C H Errypicker cherry-pic...@example.com

An ideal result would be completely different:

test: do something great

commit da39a3ee5e6b4b0d3255bfef95601890afd80709 upstream.

Do something fantastic in a clean and elegant way that
only takes two lines of explanation.

Signed-off-by: C H Errypicker cherry-pic...@example.com

In other words, the -x output format that puts the commit id at the
end with odd spacing seems to be of questionable taste anyway.  But
given the constraint of leaving that alone, cramming together the
sign-off like this seems like the best we can do, so for what it's
worth,
Acked-by: Jonathan Nieder jrnie...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html