Re: [PATCH 4/4] sequencer: use trailer's trailer layout

[Junio C Hamano]

Jonathan Tan  writes:

>>>  9:I want to mention about Signed-off-by: here.
>> ...
>> This seems a bit weird.
>
> This is because the "I want to mention" block has 100% trailer lines
> (since its only line contains a colon). We could forbid spaces in
> trailer field names, but as you said [1], it might be better to allow
> them since users might include them.

That merely means that the implementation of the wish expressed in
[1] was overly loose and needs a bit of tightening, isn't it?

> The original sequencer.c interpreted this block as not a trailer
> block, because it only accepted alphanumeric characters or '-' before
> the colon (and no spaces) - hence the difference in behavior.

That sounds more sensible to me.  Would there be an easy way to
still allow misspelled "Thanks to:" but not be fooled by an obvious
nonsense like this example, without going deep into natural language
processing?  If not, we may want to tighten it back.

>
> [1] 

Re: [PATCH 4/4] sequencer: use trailer's trailer layout

[Jonathan Tan]

On 10/31/2016 06:11 PM, Junio C Hamano wrote:

Jonathan Tan  writes:

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba4902d..635b394 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1277,8 +1277,7 @@ EOF
 4:Subject: [PATCH] subject
 8:
 9:I want to mention about Signed-off-by: here.
-10:
-11:Signed-off-by: C O Mitter 
+10:Signed-off-by: C O Mitter 
 EOF
test_cmp expected actual
 '


The original log message is a single-liner subject line, blank, "I
want to mention..." and when asked to append S-o-b:, we would want
to see a blank before the added S-o-b, no?

This seems a bit weird.


This is because the "I want to mention" block has 100% trailer lines 
(since its only line contains a colon). We could forbid spaces in 
trailer field names, but as you said [1], it might be better to allow 
them since users might include them.


The original sequencer.c interpreted this block as not a trailer block, 
because it only accepted alphanumeric characters or '-' before the colon 
(and no spaces) - hence the difference in behavior.


[1] 

Re: [PATCH 4/4] sequencer: use trailer's trailer layout

[Junio C Hamano]

Jonathan Tan  writes:

> @@ -26,30 +27,6 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
>  static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
>  static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
>  
> -static int is_rfc2822_line(const char *buf, int len)
> -{
> - int i;
> -
> - for (i = 0; i < len; i++) {
> - int ch = buf[i];
> - if (ch == ':')
> - return 1;
> - if (!isalnum(ch) && ch != '-')
> - break;
> - }
> -
> - return 0;
> -}
> -
> -static int is_cherry_picked_from_line(const char *buf, int len)
> -{
> - /*
> -  * We only care that it looks roughly like (cherry picked from ...)
> -  */
> - return len > strlen(cherry_picked_prefix) + 1 &&
> - starts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';
> -}

We lost two helper functions here, one to detect "Mail-header: like"
line, the other to detect "(cherry picked from") line.  Let's see
how the updated caller can do without these.  We know that both of
these can be caught if we grabbed the block of trailer block.

> @@ -59,49 +36,25 @@ static int is_cherry_picked_from_line(const char *buf, 
> int len)
>  static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
>   int ignore_footer)
>  {
> - char prev;
> - int i, k;
> - int len = sb->len - ignore_footer;
> - const char *buf = sb->buf;
> - int found_sob = 0;
> -
> - /* footer must end with newline */
> - if (!len || buf[len - 1] != '\n')
> - return 0;
> + struct trailer_info info;
> + int i;
> + int found_sob = 0, found_sob_last = 0;
>  
> - prev = '\0';
> - for (i = len - 1; i > 0; i--) {
> - char ch = buf[i];
> - if (prev == '\n' && ch == '\n') /* paragraph break */
> - break;
> - prev = ch;
> - }
> + trailer_info_get(, sb->buf);
>  
> - /* require at least one blank line */
> - if (prev != '\n' || buf[i] != '\n')
> + if (info.trailer_start == info.trailer_end)
>   return 0;

So we feed the thing to trailer_info_get() which will find the
trailer block.  If there is no trailer block, start and end will
point at the same place, which is trivial.

>  
> - /* advance to start of last paragraph */
> - while (i < len - 1 && buf[i] == '\n')
> - i++;
> -
> - for (; i < len; i = k) {
> - int found_rfc2822;
> -
> - for (k = i; k < len && buf[k] != '\n'; k++)
> - ; /* do nothing */
> - k++;
> + for (i = 0; i < info.trailer_nr; i++)
> + if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
> + found_sob = 1;
> + if (i == info.trailer_nr - 1)
> + found_sob_last = 1;
> + }
>  
> - found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
> - if (found_rfc2822 && sob &&
> - !strncmp(buf + i, sob->buf, sob->len))
> - found_sob = k;

Then we scan the trailer block and see if we are looking at the same
s-o-b line as we are asked to look for, and if it is at the last
logical line in the trailer block.

> + trailer_info_release();
>  
> - if (!(found_rfc2822 ||
> -   is_cherry_picked_from_line(buf + i, k - i - 1)))
> - return 0;

We used to reject a "last paragraph" that has "cruft" other than
"Mail-header: like" line or "(cherry-picked from" line.  By reusing
the trailer code, we are getting consistently looser with its logic.

> - }
> - if (found_sob == i)
> + if (found_sob_last)
>   return 3;
>   if (found_sob)
>   return 2;

I found it surprising that you said "commit -s", "cherry-pick -x"
and "format-patch -s" are covered by this patch and saw a change
only to sequencer.c.

It turns out append_signoff() is the central function that is shared
among builtin/commit.c::prepare_to_commit() that does "commit -s",
log-tree.c::show_log() that is called by "format-patch -s", and
sequencer.c::do_recursive_merge() that do_pick_commit() hence "git
cherry-pick -s" uses.  And that function decides where to put a new
S-o-b by calling this has_conforming_footer() function.

In addition, do_pick_commit() also calls has_conforming_footer() to
decide where to add "(cherry picked from".

Whoever did the sequencer.c should be proud to structure the code to
make this change so easy to make (I know it is not me, and you
Jonathan know it is not you).  

Nicely done by both of you.

> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
> index 9cce5ae..bf0a5c9 100755
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor "
>  
>  mesg_broken_footer="$mesg_no_footer
>  
> -The signed-off-by string