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