Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option

2015-02-10 Thread Junio C Hamano
Christian Couder christian.cou...@gmail.com writes:

 I think that very few new features are now needed to make it possible
 to use the code in other commands like commit, format-patch, am, etc,
 but this patch implements one of the needed features.

 - do trailer stuff by calling a central helper that does
   trailer stuff a pointer to the middle, trailers, struct.

   - when the trailer becomes a reusable subsystem, this central
 helper will become the primary entry point to the API.

   - trailer stuff will include adding new ones, removing
 existing ones, and rewriting lines.  What you do in the
 current process_command_line_args() and
 process_trailers_lists() [*1*] would come here.

 - write out the result, given the outermost struct.  This will
   become another entry point in the API.

 Structured that way, callers will supply that outermost structure,
 and can trust that the trailers subsystem will not molest
 message_proper or lines_after_trailers part.

 I don't think it is a big improvement because it is easy to see that
 the current code doesn't molest the part before and after the
 trailers.

You force the callers that want only the trailer thing to happen
to:

 - pass first and last around.

 - keep each line of the message body in separate strbuf and have
   it in the same array as the trailers

Neither of which is necessary.  I recall that during the review of
the previous rounds your own code had to work this around by first
concatenating lines (each of which are unnecessarily in separate
struct strbufs) into a single strbuf, only to call a helper that
takes a single strbuf to count what to ignore in it, and then
iterate over the array of strbuf to add up the lengths of them,
which would have been unnecessary if the underlying data structure
were saner.
--
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 1/3] trailer: add a trailer.trimEmpty config option

2015-02-10 Thread Junio C Hamano
Christian Couder christian.cou...@gmail.com writes:

 On Sat, Feb 7, 2015 at 9:20 PM, Junio C Hamano gits...@pobox.com wrote:

 Another problem I have with filter out during the output phase
 comes from the semantics/correctness in the resulting code, and I
 suspect that it would need to be done a lot earlier, before you
 allow the logic such as if I have X, do this, but if there is no X,
 do this other thing.  If you have an X that is empty in the input,
 trimming that before such logic kicks in and trimming that in the
 final output phase would give different results, and I think the
 latter would give a less intuitive result.

 I think it is much better in the output phase because it is very
 natural for some projects to have a template message with empty
 trailers like this:

That is exactly my point.  With empty trailers in the input, Add
this iff there is no existing one will be made useless.

I am *not* saying that we must not have a filter at the output
phrase.  If anything, it would allow people to be more sloppy and
hide the problem under the rug.  Your code may have a bug or design
problem that adds an empty one somewhere even when the user told you
that she does not want an empty one in the result.  The user may be
sloppy and say Add this key-value unconditionally, instead of
having to do What is the value I want to add?  Oh, it is not empty,
so I'll ask the trailer machiner to add this key-value there.

I am saying that not filtering the input and whatever intermediate
result you produce [*1*] will make the end result much less useful.

[Footnote]

*1* Filtering intermediate result of course can be done by making
sure you do not add an empty one in the first place.
--
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 1/3] trailer: add a trailer.trimEmpty config option

2015-02-09 Thread Junio C Hamano
Christian Couder christian.cou...@gmail.com writes:

 It is not designed like this because you only asked me to design it
 like this after the facts, when there was another email thread about
 conflicts blocks and one function you created could be used by the
 trailer code too.

 If you had asked this from the beginning I would certainly have done...

Because the process here is not somebody outlines the design and
gives it to laborer to implement, after the fact and from the
beginning are complaining to and asking for impossible.  You design
and implement and then it gets reviewed, not the other way around.

--
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 1/3] trailer: add a trailer.trimEmpty config option

2015-02-07 Thread Junio C Hamano
Christian Couder chrisc...@tuxfamily.org writes:

 This way people who always want trimed trailers
 don't need to specify it on the command line.

I sense that print_all() that cares about trimming illustrate a
design mistake in the original code structure.  Why isn't the entire
program structured like this instead?

- read input and split that into three parts:

struct {
struct strbuf messsage_proper;

struct trailers {
int nr, alloc;
struct strbuf *array_of_trailers[];
} trailers;

struct strbuf lines_after_trailers;
};

- do trailer stuff by calling a central helper that does
  trailer stuff a pointer to the middle, trailers, struct.

  - when the trailer becomes a reusable subsystem, this central
helper will become the primary entry point to the API.

  - trailer stuff will include adding new ones, removing
existing ones, and rewriting lines.  What you do in the
current process_command_line_args() and
process_trailers_lists() [*1*] would come here.

- write out the result, given the outermost struct.  This will
  become another entry point in the API.

Structured that way, callers will supply that outermost structure,
and can trust that the trailers subsystem will not molest
message_proper or lines_after_trailers part.  They can even process
the parts that the trailer subsystem would not touch, e.g. running
stripspace to the text in message_proper.

Viewed that way, it would be clear that strip empty ones should be
a new feature in the trailer stuff, not just filter out only
during the output phase.  Having it in the output phase does not
feel that the labor/responsibility is split in the right way.

Another problem I have with filter out during the output phase
comes from the semantics/correctness in the resulting code, and I
suspect that it would need to be done a lot earlier, before you
allow the logic such as if I have X, do this, but if there is no X,
do this other thing.  If you have an X that is empty in the input,
trimming that before such logic kicks in and trimming that in the
final output phase would give different results, and I think the
latter would give a less intuitive result.

As this is the second time I have to point out that the data
structure used by the current code to hold the trailers and other
stuff to work on smells fishy, I would seriously consider cleaning
up the existing code to make it easier to allow us to later create
a reusable API and trailer subsystem out of it, before piling new
features on top of it, if I were you.

 Signed-off-by: Christian Couder chrisc...@tuxfamily.org
 ---
  builtin/interpret-trailers.c |  2 +-
  trailer.c| 13 ++---
  trailer.h|  2 +-
  3 files changed, 12 insertions(+), 5 deletions(-)

 diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
 index 46838d2..1adf814 100644
 --- a/builtin/interpret-trailers.c
 +++ b/builtin/interpret-trailers.c
 @@ -18,7 +18,7 @@ static const char * const git_interpret_trailers_usage[] = {
  
  int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
  {
 - int trim_empty = 0;
 + int trim_empty = -1;
   struct string_list trailers = STRING_LIST_INIT_DUP;
  
   struct option options[] = {
 diff --git a/trailer.c b/trailer.c
 index 623adeb..7614182 100644
 --- a/trailer.c
 +++ b/trailer.c
 @@ -36,6 +36,8 @@ static struct trailer_item *first_conf_item;
  
  static char *separators = :;
  
 +static int trim_empty;
 +
  #define TRAILER_ARG_STRING $ARG
  
  static int after_or_end(enum action_where where)
 @@ -120,7 +122,7 @@ static void print_tok_val(const char *tok, const char 
 *val)
   printf(%s%c %s\n, tok, separators[0], val);
  }
  
 -static void print_all(struct trailer_item *first, int trim_empty)
 +static void print_all(struct trailer_item *first)
  {
   struct trailer_item *item;
   for (item = first; item; item = item-next) {
 @@ -509,6 +511,8 @@ static int git_trailer_default_config(const char 
 *conf_key, const char *value, v
   value, conf_key);
   } else if (!strcmp(trailer_item, separators)) {
   separators = xstrdup(value);
 + } else if (!strcmp(trailer_item, trimempty)) {
 + trim_empty = git_config_bool(conf_key, value);
   }
   }
   return 0;
 @@ -842,7 +846,7 @@ static void free_all(struct trailer_item **first)
   }
  }
  
 -void process_trailers(const char *file, int trim_empty, struct string_list 
 *trailers)
 +void process_trailers(const char *file, int trim, struct string_list 
 *trailers)
  {
   struct trailer_item *in_tok_first = NULL;
   struct trailer_item *in_tok_last = NULL;
 @@ -854,6 +858,9 @@ void process_trailers(const char *file, int trim_empty, 
 

Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option

2015-02-07 Thread Christian Couder
On Sat, Feb 7, 2015 at 9:20 PM, Junio C Hamano gits...@pobox.com wrote:
 Christian Couder chrisc...@tuxfamily.org writes:

 This way people who always want trimed trailers
 don't need to specify it on the command line.

 I sense that print_all() that cares about trimming illustrate a
 design mistake in the original code structure.  Why isn't the entire
 program structured like this instead?

 - read input and split that into three parts:

 struct {
 struct strbuf messsage_proper;

 struct trailers {
 int nr, alloc;
 struct strbuf *array_of_trailers[];
 } trailers;

 struct strbuf lines_after_trailers;
 };

It is not designed like this because you only asked me to design it
like this after the facts, when there was another email thread about
conflicts blocks and one function you created could be used by the
trailer code too.

If you had asked this from the beginning I would certainly have done
it more like this (though I think the struct trailers part is too
simplistic). Rearchitecturing the code now would bring only small
performance improvements and a lot of code churn. And the performance
is not needed anyway if the code is not used in the first place, so
I'd rather first make sure that the code can be used.

I think that very few new features are now needed to make it possible
to use the code in other commands like commit, format-patch, am, etc,
but this patch implements one of the needed features.

 - do trailer stuff by calling a central helper that does
   trailer stuff a pointer to the middle, trailers, struct.

   - when the trailer becomes a reusable subsystem, this central
 helper will become the primary entry point to the API.

   - trailer stuff will include adding new ones, removing
 existing ones, and rewriting lines.  What you do in the
 current process_command_line_args() and
 process_trailers_lists() [*1*] would come here.

 - write out the result, given the outermost struct.  This will
   become another entry point in the API.

 Structured that way, callers will supply that outermost structure,
 and can trust that the trailers subsystem will not molest
 message_proper or lines_after_trailers part.

I don't think it is a big improvement because it is easy to see that
the current code doesn't molest the part before and after the
trailers.

 They can even process
 the parts that the trailer subsystem would not touch, e.g. running
 stripspace to the text in message_proper.

That could be worth the rearchitecturing if people really wanted that,
but I think for now more people have been interested in having ways to
change the trailer part, so I prefer to work on this first.

 Viewed that way, it would be clear that strip empty ones should be
 a new feature in the trailer stuff, not just filter out only
 during the output phase.  Having it in the output phase does not
 feel that the labor/responsibility is split in the right way.

My opinion is that having it in the output phase is best.

 Another problem I have with filter out during the output phase
 comes from the semantics/correctness in the resulting code, and I
 suspect that it would need to be done a lot earlier, before you
 allow the logic such as if I have X, do this, but if there is no X,
 do this other thing.  If you have an X that is empty in the input,
 trimming that before such logic kicks in and trimming that in the
 final output phase would give different results, and I think the
 latter would give a less intuitive result.

I think it is much better in the output phase because it is very
natural for some projects to have a template message with empty
trailers like this:

Signed-off-by:
Reviewed-by:

Such a template message can for example remind contributors to the
project that they need to sign off their work and that they need to
have it reviewed by at least one person, and that to make it simpler
for everyone to review patches, the Signed-off-by trailers should
come before the Reviewed-by trailers in the commit message.

In this case, if you trim before you process command line trailers,
then, when you process some command line trailers that add sign offs,
the meaning of where=after cannot be based any more on the existing
empty Signed-off-by: in the template message.

 As this is the second time I have to point out that the data
 structure used by the current code to hold the trailers and other
 stuff to work on smells fishy, I would seriously consider cleaning
 up the existing code to make it easier to allow us to later create
 a reusable API and trailer subsystem out of it, before piling new
 features on top of it, if I were you.

I am not so sure that it would make it easier to allow us to later
create a reusable API and trailer subsystem out of it.
It is very difficult to predict what will be really needed in the
future and it is not