Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

2013-02-21 Thread Junio C Hamano 
Brandon Casey draf...@gmail.com writes:

 Teach append_signoff to detect whether a blank line exists at the position
 that the signed-off-by line will be added, and refrain from adding an
 additional one if one already exists.  Or, add an additional line if one
 is needed to make sure the new footer is separated from the message body
 by a blank line.

 Signed-off-by: Brandon Casey bca...@nvidia.com
 ---


 A slight tweak.  And I promise, no more are coming.

When I do

$ git commit -s

it should start my editor with this in the buffer:



Signed-off-by: Junio C Hamano gits...@pobox.com


and the cursor blinking at the beginning of the file.  Annoyingly
this step breaks it by removing the leading blank line.
--
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 v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

2013-02-21 Thread Brandon Casey 
On Thu, Feb 21, 2013 at 12:26 PM, Brandon Casey draf...@gmail.com wrote:

 But, this does not fix the same problem for 'cherry-pick --edit -s'
 when used to cherry-pick a commit without a sob.

Correction: when used to cherry-pick a commit with an empty commit message.
--
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 v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

2013-02-21 Thread Brandon Casey 
On Thu, Feb 21, 2013 at 10:51 AM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey draf...@gmail.com writes:

 Teach append_signoff to detect whether a blank line exists at the position
 that the signed-off-by line will be added, and refrain from adding an
 additional one if one already exists.  Or, add an additional line if one
 is needed to make sure the new footer is separated from the message body
 by a blank line.

 Signed-off-by: Brandon Casey bca...@nvidia.com
 ---


 A slight tweak.  And I promise, no more are coming.

 When I do

 $ git commit -s

 it should start my editor with this in the buffer:

 

 Signed-off-by: Junio C Hamano gits...@pobox.com
 

 and the cursor blinking at the beginning of the file.  Annoyingly
 this step breaks it by removing the leading blank line.

Yes.  The fix described by John Keeping restores the above behavior
for 'commit -s'.  Or the fix I described which inserts two preceding
newlines so it looks like this:

   


   Signed-off-by: Junio C Hamano gits...@pobox.com
   

So then the cursor would be placed on the first line and a space would
separate it from the sob which is arguably a better indication to the
user that a blank line should separate the commit message body from
the sob.

But, this does not fix the same problem for 'cherry-pick --edit -s'
when used to cherry-pick a commit without a sob.  The cherry-pick part
of it would add the extra preceding newlines, but then cherry-pick
passes the buffer to 'git commit' via .git/MERGE_MSG which then
cleans the buffer, removing the empty lines, in prepare_to_commit()
before allowing the editor to operate on it.

Using 'cherry-pick --edit -s' to cherry-pick a commit with an empty
commit message is going to be a pretty rare corner case.  It would be
nice to have the same behavior for it that we decide to have for
'commit -s', but it's probably not worth going through contortions to
make it happen.

-Brandon
--
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 v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

2013-02-21 Thread Junio C Hamano 
Brandon Casey draf...@gmail.com writes:

 Yes.  The fix described by John Keeping restores the above behavior
 for 'commit -s'.  Or the fix I described which inserts two preceding
 newlines so it looks like this:




Signed-off-by: Junio C Hamano gits...@pobox.com


 So then the cursor would be placed on the first line and a space would
 separate it from the sob which is arguably a better indication to the
 user that a blank line should separate the commit message body from
 the sob.

That sounds like an improvement to me.

 But, this does not fix the same problem for 'cherry-pick --edit -s'
 when used to cherry-pick a commit without a sob. ...
 Using 'cherry-pick --edit -s' to cherry-pick a commit with an empty
 commit message is going to be a pretty rare corner case

We actively discourage an empty commit message by requiring users to
say commit --allow-empty-message.  I think it is in line with the
philosophy for a Porcelain command git cherry-pick -s to punish
users by making them work harder to use a commit with an empty
message ;-).


--
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 v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

2013-02-17 Thread John Keeping 
On Fri, Feb 15, 2013 at 10:58:38AM -0800, Brandon Casey wrote:
 On Thu, Feb 14, 2013 at 9:58 AM, John Keeping j...@keeping.me.uk wrote:
  As Jonathan Nieder wondered before [1], this changes the behaviour when
  the commit message is empty.  Before this commit, there is an empty line
  followed by the S-O-B line; now the S-O-B is on the first line of the
  commit.
 
  The previous behaviour seems better to me since the empty line is
  hinting that the user should fill something in.  It looks particularly
  strange if your editor has syntax highlighting for commit messages such
  that the first line is in a different colour.
 
 Are you talking about the output produced by format-patch?  Or are you
 talking about what happens when you do 'commit --amend -s' for a
 commit with an empty commit message. (The email that you referenced
 was about the behavior of format-patch).

I'm talking about plain 'commit -s' which seems to use the same code
path.

 I'm thinking you must be talking about the 'commit --amend -s'
 behavior since you mentioned your editor.  Is there another case that
 is affected by this?  Normally, any extra blank lines that precede or
 follow a commit message are removed before the commit object is
 created.  So, I guess it wouldn't hurt to insert a newline (or maybe
 it should be two?) before the signoff in this case.  Would this
 provide an improvement or change for any other commands than 'commit
 --amend -s'?
 
 If we want to do this, then I'd probably do it like this:
 
 -   if (len  msgbuf-buf[len - 1] != '\n')
 +   if (!len || msgbuf-buf[len - 1] != '\n')
 append_newlines = \n\n;
 -   else if (len  1  msgbuf-buf[len - 2] != '\n')
 +   else if (len == 1 || msgbuf-buf[len - 2] != '\n')
 append_newlines = \n;
 
 This would ensure there were two newlines preceding the sob.  The
 editor would place its cursor on the top line where the user should
 begin typing in a commit message.  If an editor was not opened up
 (e.g. if 'git cherry-pick -s --allow-empty-message ...' was used) then
 the normal mechanism that removes extra blank lines would trigger to
 remove the extra blank lines.
 
 I think that's reasonable.

Two blank lines seems like an improvement to me, FWIW.


John
--
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 v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

2013-02-15 Thread Brandon Casey 
On Thu, Feb 14, 2013 at 9:58 AM, John Keeping j...@keeping.me.uk wrote:
 On Tue, Feb 12, 2013 at 02:33:42AM -0800, Brandon Casey wrote:
 Teach append_signoff to detect whether a blank line exists at the position
 that the signed-off-by line will be added, and refrain from adding an
 additional one if one already exists.  Or, add an additional line if one
 is needed to make sure the new footer is separated from the message body
 by a blank line.

 Signed-off-by: Brandon Casey bca...@nvidia.com
 ---

 As Jonathan Nieder wondered before [1], this changes the behaviour when
 the commit message is empty.  Before this commit, there is an empty line
 followed by the S-O-B line; now the S-O-B is on the first line of the
 commit.

 The previous behaviour seems better to me since the empty line is
 hinting that the user should fill something in.  It looks particularly
 strange if your editor has syntax highlighting for commit messages such
 that the first line is in a different colour.

 [1] http://article.gmane.org/gmane.comp.version-control.git/214796

 diff --git a/sequencer.c b/sequencer.c
 index 3364faa..084573b 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int 
 ignore_footer, unsigned flag)
   else
   has_footer = has_conforming_footer(msgbuf, sob, 
 ignore_footer);

 - if (!has_footer)
 - strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0, \n, 1);
 + if (!has_footer) {
 + const char *append_newlines = NULL;
 + size_t len = msgbuf-len - ignore_footer;
 +
 + if (len  msgbuf-buf[len - 1] != '\n')
 + append_newlines = \n\n;
 + else if (len  1  msgbuf-buf[len - 2] != '\n')
 + append_newlines = \n;

 To restore the old behaviour this needs something like this:

 else if (!len)
 append_newlines = \n;

 + if (append_newlines)
 + strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0,
 + append_newlines, strlen(append_newlines));
 + }

   if (has_footer != 3  (!no_dup_sob || has_footer != 2))
   strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0,

Are you talking about the output produced by format-patch?  Or are you
talking about what happens when you do 'commit --amend -s' for a
commit with an empty commit message. (The email that you referenced
was about the behavior of format-patch).

I'm thinking you must be talking about the 'commit --amend -s'
behavior since you mentioned your editor.  Is there another case that
is affected by this?  Normally, any extra blank lines that precede or
follow a commit message are removed before the commit object is
created.  So, I guess it wouldn't hurt to insert a newline (or maybe
it should be two?) before the signoff in this case.  Would this
provide an improvement or change for any other commands than 'commit
--amend -s'?

If we want to do this, then I'd probably do it like this:

-   if (len  msgbuf-buf[len - 1] != '\n')
+   if (!len || msgbuf-buf[len - 1] != '\n')
append_newlines = \n\n;
-   else if (len  1  msgbuf-buf[len - 2] != '\n')
+   else if (len == 1 || msgbuf-buf[len - 2] != '\n')
append_newlines = \n;

This would ensure there were two newlines preceding the sob.  The
editor would place its cursor on the top line where the user should
begin typing in a commit message.  If an editor was not opened up
(e.g. if 'git cherry-pick -s --allow-empty-message ...' was used) then
the normal mechanism that removes extra blank lines would trigger to
remove the extra blank lines.

I think that's reasonable.

It seems 'git cherry-pick -s --edit' follows a different code path,
and the commit message is stripped of newlines by 'git commit' before
it is passed to the editor.  'cherry-pick -s --edit' and 'commit
--amend -s' should probably have the same behavior and present the
same buffer to the user for editing when they encounter a commit with
an empty message.

Maybe something like this is enough(?):

diff --git a/builtin/commit.c b/builtin/commit.c
index 7b9e2ac..0796412 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -124,8 +124,10 @@ static int opt_parse_m(const struct option *opt, const char
if (unset)
strbuf_setlen(buf, 0);
else {
+   if (buf-len)
+   strbuf_addch(buf, '\n');
strbuf_addstr(buf, arg);
-   strbuf_addstr(buf, \n\n);
+   strbuf_complete_line(buf);
}
return 0;
 }
@@ -673,9 +675,6 @@ static int prepare_to_commit(const char *index_file, const c
if (s-fp == NULL)
die_errno(_(could not open '%s'), git_path(commit_editmsg));

-   if (clean_message_contents)
-   stripspace(sb, 0);
-
if (signoff) {

Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

2013-02-14 Thread John Keeping 
On Tue, Feb 12, 2013 at 02:33:42AM -0800, Brandon Casey wrote:
 Teach append_signoff to detect whether a blank line exists at the position
 that the signed-off-by line will be added, and refrain from adding an
 additional one if one already exists.  Or, add an additional line if one
 is needed to make sure the new footer is separated from the message body
 by a blank line.
 
 Signed-off-by: Brandon Casey bca...@nvidia.com
 ---

As Jonathan Nieder wondered before [1], this changes the behaviour when
the commit message is empty.  Before this commit, there is an empty line
followed by the S-O-B line; now the S-O-B is on the first line of the
commit.

The previous behaviour seems better to me since the empty line is
hinting that the user should fill something in.  It looks particularly
strange if your editor has syntax highlighting for commit messages such
that the first line is in a different colour.

[1] http://article.gmane.org/gmane.comp.version-control.git/214796

 diff --git a/sequencer.c b/sequencer.c
 index 3364faa..084573b 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int 
 ignore_footer, unsigned flag)
   else
   has_footer = has_conforming_footer(msgbuf, sob, ignore_footer);
  
 - if (!has_footer)
 - strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0, \n, 1);
 + if (!has_footer) {
 + const char *append_newlines = NULL;
 + size_t len = msgbuf-len - ignore_footer;
 +
 + if (len  msgbuf-buf[len - 1] != '\n')
 + append_newlines = \n\n;
 + else if (len  1  msgbuf-buf[len - 2] != '\n')
 + append_newlines = \n;

To restore the old behaviour this needs something like this:

else if (!len)
append_newlines = \n;

 + if (append_newlines)
 + strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0,
 + append_newlines, strlen(append_newlines));
 + }
  
   if (has_footer != 3  (!no_dup_sob || has_footer != 2))
   strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0,
--
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

[PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

2013-02-12 Thread Brandon Casey 
Teach append_signoff to detect whether a blank line exists at the position
that the signed-off-by line will be added, and refrain from adding an
additional one if one already exists.  Or, add an additional line if one
is needed to make sure the new footer is separated from the message body
by a blank line.

Signed-off-by: Brandon Casey bca...@nvidia.com
---


A slight tweak.  And I promise, no more are coming.

-Brandon


 sequencer.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3364faa..084573b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int 
ignore_footer, unsigned flag)
else
has_footer = has_conforming_footer(msgbuf, sob, ignore_footer);
 
-   if (!has_footer)
-   strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0, \n, 1);
+   if (!has_footer) {
+   const char *append_newlines = NULL;
+   size_t len = msgbuf-len - ignore_footer;
+
+   if (len  msgbuf-buf[len - 1] != '\n')
+   append_newlines = \n\n;
+   else if (len  1  msgbuf-buf[len - 2] != '\n')
+   append_newlines = \n;
+
+   if (append_newlines)
+   strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0,
+   append_newlines, strlen(append_newlines));
+   }
 
if (has_footer != 3  (!no_dup_sob || has_footer != 2))
strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0,
-- 
1.8.1.1.252.gdb33759

--
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