On Sat, Feb 7, 2015 at 9:20 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <chriscool@xxxxxxxxxxxxx> 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 clear at all to me that the current API is not
reusable.
If it was so clear that an API similar as what you describe was so
important, why didn't you point out that at the very beginning when I
started working on git interpret-trailers? And it's not like you
didn't have time to think about what I was working on as we discussed
many other aspects of the design and the developement happened over
one year.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html