Re: [PATCH 9/9] trailer: add tests for git interpret-trailers

2013-12-30 Thread Junio C Hamano
Christian Couder chrisc...@tuxfamily.org writes:

 +# Do not remove trailing spaces below!
 +cat complex_message_trailers 'EOF'
 +Fixes: 
 +Acked-by: 
 +Reviewed-by: 
 +Signed-off-by: 
 +EOF

Just a hint.  I think it is far safer and robust over time to do
something like this:

sed -e 's/ Z$/ /' -\EOF
Fixes: Z
Acked-by: Z
EOF

instead of a comment, which can warn human developers but does not
do anything to prevent their editors' auto-fix features from kicking
in.
--
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 9/9] trailer: add tests for git interpret-trailers

2013-12-30 Thread Josh Triplett
On Mon, Dec 30, 2013 at 09:19:55AM -0800, Junio C Hamano wrote:
 Christian Couder chrisc...@tuxfamily.org writes:
 
  +# Do not remove trailing spaces below!
  +cat complex_message_trailers 'EOF'
  +Fixes: 
  +Acked-by: 
  +Reviewed-by: 
  +Signed-off-by: 
  +EOF
 
 Just a hint.  I think it is far safer and robust over time to do
 something like this:
 
   sed -e 's/ Z$/ /' -\EOF
 Fixes: Z
 Acked-by: Z
 EOF
 
 instead of a comment, which can warn human developers but does not
 do anything to prevent their editors' auto-fix features from kicking
 in.

This, but for simplicity, since every line needs the trailing space, why
not just use 's/$/ /' and drop the ' Z' on every line?

/bikeshed

- Josh Triplett
--
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 9/9] trailer: add tests for git interpret-trailers

2013-12-30 Thread Junio C Hamano
Josh Triplett j...@joshtriplett.org writes:

 On Mon, Dec 30, 2013 at 09:19:55AM -0800, Junio C Hamano wrote:
 Christian Couder chrisc...@tuxfamily.org writes:
 
  +# Do not remove trailing spaces below!
  +cat complex_message_trailers 'EOF'
  +Fixes: 
  +Acked-by: 
  +Reviewed-by: 
  +Signed-off-by: 
  +EOF
 
 Just a hint.  I think it is far safer and robust over time to do
 something like this:
 
  sed -e 's/ Z$/ /' -\EOF
 Fixes: Z
 Acked-by: Z
 EOF
 
 instead of a comment, which can warn human developers but does not
 do anything to prevent their editors' auto-fix features from kicking
 in.

 This, but for simplicity, since every line needs the trailing space, why
 not just use 's/$/ /' and drop the ' Z' on every line?

 /bikeshed

 - Josh Triplett

A few reasons:

 - The everybody will have a single SP at the end may or may not
   last forever;

 - With your scheme, if you already had _one_ trailing SPs in the
   input, it would be hard to spot in the source;

 - It makes it visually very clear that we expect a SP after these
   colons.  This is especially true if you replace 'Z' with
   something more readable (e.g. '|').

--
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 9/9] trailer: add tests for git interpret-trailers

2013-12-30 Thread Josh Triplett
On Mon, Dec 30, 2013 at 12:46:33PM -0800, Junio C Hamano wrote:
 Josh Triplett j...@joshtriplett.org writes:
 
  On Mon, Dec 30, 2013 at 09:19:55AM -0800, Junio C Hamano wrote:
  Christian Couder chrisc...@tuxfamily.org writes:
  
   +# Do not remove trailing spaces below!
   +cat complex_message_trailers 'EOF'
   +Fixes: 
   +Acked-by: 
   +Reviewed-by: 
   +Signed-off-by: 
   +EOF
  
  Just a hint.  I think it is far safer and robust over time to do
  something like this:
  
 sed -e 's/ Z$/ /' -\EOF
  Fixes: Z
  Acked-by: Z
  EOF
  
  instead of a comment, which can warn human developers but does not
  do anything to prevent their editors' auto-fix features from kicking
  in.
 
  This, but for simplicity, since every line needs the trailing space, why
  not just use 's/$/ /' and drop the ' Z' on every line?
 
  /bikeshed
 
  - Josh Triplett
 
 A few reasons:
 
  - The everybody will have a single SP at the end may or may not
last forever;

Trivially fixed if that ever changes, but given the nature of all of
these, that seems unlikely.

  - With your scheme, if you already had _one_ trailing SPs in the
input, it would be hard to spot in the source;

Git makes them quite difficult to miss. :)

- Josh Triplett
--
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 9/9] trailer: add tests for git interpret-trailers

2013-12-30 Thread Junio C Hamano
Josh Triplett j...@joshtriplett.org writes:

  - The everybody will have a single SP at the end may or may not
last forever;

 Trivially fixed if that ever changes, but given the nature of all of
 these, that seems unlikely.

Why?  Because we encourage to write tests that are expected to find
breakages, some of these test vector lines may have to show a broken
line that lacks SP after label + colon.

  - With your scheme, if you already had _one_ trailing SPs in the
input, it would be hard to spot in the source;

 Git makes them quite difficult to miss. :)

That is irrelevant, isn't it?

This is about protecting the source in the editor, before you run
git show --whitespace=trailing-space, git diff --check, etc.


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