Re: [PATCH v5 02/14] trailer: process trailers from file and arguments
From: Junio C Hamano gits...@pobox.com Junio C Hamano gits...@pobox.com writes: Having said all that, it appears that nobody seems to be able to come up with a saner arrangement that would not paint us into a tough corner that we would not be able to later escape from without being backward incompatible---I certainly didn't. So... let's take this from your earlier message: If we limit it to if_exists and if_missing, the user can remember that without things becoming too complex. and go with the semantics the posted patches (I believe I have the latest from you on 'pu') attempt to implement, at least for now. IOW, when re-rolling, let's not try changing the arrangement to use if-exists/if-missing (configuration variable names) for keys' existence and include chosen set of conditions on values as modifiers to the action (i.e. X in do_Y_in_X). Ok, will re-roll soon. Thanks, Christian. -- 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 v5 02/14] trailer: process trailers from file and arguments
Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: What is the right mental model the end-user needs to form when understanding these? Conditions on keys go on the left, and any other random conditions can come as a modifier after action e.g. add_if_same_value_is_not_at_the_end? Having said all that, it appears that nobody seems to be able to come up with a saner arrangement that would not paint us into a tough corner that we would not be able to later escape from without being backward incompatible---I certainly didn't. So... let's take this from your earlier message: If we limit it to if_exists and if_missing, the user can remember that without things becoming too complex. and go with the semantics the posted patches (I believe I have the latest from you on 'pu') attempt to implement, at least for now. IOW, when re-rolling, let's not try changing the arrangement to use if-exists/if-missing (configuration variable names) for keys' existence and include chosen set of conditions on values as modifiers to the action (i.e. X in do_Y_in_X). -- 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 v5 02/14] trailer: process trailers from file and arguments
From: Junio C Hamano gits...@pobox.com These numerous questions you have to ask are indications why choosing this condition goes to the left hand side of the equal sign (e.g. exists) and this condition goes to the right hand side (e.g. do-this-if_neighbor) is not working well. The user has to remember which conditions go to the variable name and which conditions go to the action part. If we limit it to if_exists and if_missing, the user can remember that without things becoming too complex. That is, not splitting the logic into two parts like you did, i.e. if_X = do_Y_if_Z, which invites why is it not if_true = do_Y_if_X_and_Z, if_X_and_Z = do_Y, if_Z = do_Y_if_X? It perhaps invite it, but there are reasons why splitting the logic too much is not a good idea and why limiting the split to if_exists and if_missing is a good tradeoff. One possible way to avoid the confusion is to say do_Y_if_X_and_Z without making the rule split into conditions and actions---I am NOT saying that is _better_, there may be other solutions to avoid this two-part logic in a cleaner way. This has been discussed first last November and I don't think that a solution better than what I implemented has been suggested. The problem is we not only want to say: action = do_Y_if_X_and_Z but we also want to say: action = do_Y_if_X_and_Z AND do_U_if_V For example some people might want: if_exists = overwrite if_missing = add while others might want: if_exists = overwrite if_missing = do_nothing and I don't see how we can say that with just: action = do_Y_if_X_and_Z Thanks, Christian. -- 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 v5 02/14] trailer: process trailers from file and arguments
Christian Couder chrisc...@tuxfamily.org writes: For example some people might want: if_exists = overwrite if_missing = add while others might want: if_exists = overwrite if_missing = do_nothing and I don't see how we can say that with just: action = do_Y_if_X_and_Z Yes, but then we go back to my original question: why exists and missing are so special, and why there aren't two kinds of exists (i.e. there exists an entry with the same key, value vs there exists an entry with the same key). I would have understood your this is not too hard to understand for users if you had three (i.e. missing, in addition to these two flavours of exists), but with only two, I do not see how it is useful in a hypothetical situation like above. For example, how would you express something like this only with if-exists vs if-missing? if_exists_exactly = ignore if_exists_with_different_value = append if_missng = prepend_to_the_beginning -- 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 v5 02/14] trailer: process trailers from file and arguments
From: Junio C Hamano gits...@pobox.com Christian Couder chrisc...@tuxfamily.org writes: For example some people might want: if_exists = overwrite if_missing = add while others might want: if_exists = overwrite if_missing = do_nothing and I don't see how we can say that with just: action = do_Y_if_X_and_Z Yes, but then we go back to my original question: why exists and missing are so special, Because they are completely disjoint, easy to understand, and they can avoid a lot of combinatorial explosion we would have if we used only one action variable, while still providing lot of expressiveness. They are just a good tradeoff for the special problem we have. and why there aren't two kinds of exists (i.e. there exists an entry with the same key, value vs there exists an entry with the same key). Because it doesn't improve expressiveness much, doesn't remove much combinatorial explosion and make it significantly more difficult to understand, compared to only if_exists and if_missing. I would have understood your this is not too hard to understand for users if you had three (i.e. missing, in addition to these two flavours of exists), but with only two, I do not see how it is useful in a hypothetical situation like above. You mean that you do not see why: if_exists = overwrite if_missing = do_nothing is simple and expressive? For example, how would you express something like this only with if-exists vs if-missing? if_exists_exactly = ignore if_exists_with_different_value = append if_missng = prepend_to_the_beginning First, previously in the discussion you said that you didn't want us to talk about the where = (after | before) part, because you could see that it was orthogonal to the other stuff, but now it looks like you want again to put that on the table. Then yes, it is not possible to express the above with what I implemented. But it could be possible with only if-exists vs if-missing like this: if_exists = append_if_different if_missing = prepend And yes I think it is much better than: if_exists_exactly = ignore if_exists_with_different_value = append if_missng = prepend_to_the_beginning because we can still easily express things like: if_exists = append_if_different_neighbor if_missing = prepend while it would be more difficult to understand with if_exists_exactly, if_exists_with_different_value and if_missing. Thanks, Christian. -- 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 v5 02/14] trailer: process trailers from file and arguments
For example, how would you express something like this only with if-exists vs if-missing? if_exists_exactly = ignore if_exists_with_different_value = append if_missng = prepend_to_the_beginning First, previously in the discussion you said that you didn't want us to talk about the where = (after | before) part, because you could see that it was orthogonal to the other stuff, but now it looks like you want again to put that on the table. Oh, then replace both append and prepend with append (it was a mistake). Can you express that without having two kinds of if-exists? But it could be possible with only if-exists vs if-missing like this: if_exists = append_if_different if_missing = prepend ... because we can still easily express things like: if_exists = append_if_different_neighbor The proliferation of these random if_X on the action part _is_ exactly what I find the proposal confusing. -- 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 v5 02/14] trailer: process trailers from file and arguments
Christian Couder chrisc...@tuxfamily.org writes: but we also want to say: action = do_Y_if_X_and_Z AND do_U_if_V For example some people might want: if_exists = overwrite if_missing = add while others might want: if_exists = overwrite if_missing = do_nothing and I don't see how we can say that with just: action = do_Y_if_X_and_Z That is a very relevant illustration that makes me realize why I found your if-exists/if-missing = do-Y-if-Z somewhat distasteful. Your if_exists = add_if_different says if the same key is there, add it if the value is different, but it also implicitly says donothing if the value is the same. That is, you are saying with the above if_exists = add_if_different AND ignore_if_same So you already have to support more than one actions depending on the condition, unless you want to limit the actions for all the cases other than X to be only ignore when you invent your next do_Y_if_X, X being different in this case, but you support (and need to support) different-neighbour and other random collections of conditions, I think. Which is essentially the same as saying that you need this: action = do_Y_if_X_and_Z AND do_U_if_V Again, unless all the U's are limited to ignore, that is. -- 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 v5 02/14] trailer: process trailers from file and arguments
Junio C Hamano gits...@pobox.com writes: That is, you are saying with the above if_exists = add_if_different AND ignore_if_same So you already have to support more than one actions depending on the condition, ... of conditions, I think. Which is essentially the same as saying that you need this: action = do_Y_if_X_and_Z AND do_U_if_V Again, unless all the U's are limited to ignore, that is. Oh by the way, don't get me wrong. I am not saying that the last one is necessarily better or worse. I am only saying that the semantics proposed seems to be hard to explain and we need to do find a way to do better. If you have these existing ones: Sob: A Sob: B Sob: C Sob: D and you have Sob: B at hand, Sob.if-missing would not fire (because if-exists/if-missing is only about keys) ans Sob.if-exists will. What happens is now up to the action part (i.e. what follows if_exists =, e.g. add_if_different). The conditional part of add_if_different action is explainable as a conditon on the value (as opposed to condition on keys, which is the left-hand-side). But what does a condition with neighbour in its name really mean? It is not a condition about the value, but also is a condition about the order of the existing records. What is the right mental model the end-user needs to form when understanding these? Conditions on keys go on the left, and any other random conditions can come as a modifier after action e.g. add_if_same_value_is_not_at_the_end? -- 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 v5 02/14] trailer: process trailers from file and arguments
On Mon, Feb 10, 2014 at 9:51 PM, Junio C Hamano gits...@pobox.com wrote: Christian Couder chrisc...@tuxfamily.org writes: Many entries with the same key but distinct values can be configured using: if_exists = add_if_different if_missing = add Many entries with the same key but values that can be the same can be configured using: if_exists = add if_missing = add While the above certainly can express the combinations, I am still puzzled about the value of having under this condition (i.e. if-exists/if-missing) and do this (i.e. add/add-if-different) as two separate concepts. It is because there are many possible conditions under which the user might want to do different things, and the syntax of our configuration files is not a programming language, so it is not well suited to nicely express what the user might want under different conditions. That's what I tried to show in my last message in the thread I sent the link to in my previous message. (http://thread.gmane.org/gmane.comp.version-control.git/237278/) So we have to choose a few conditions to avoid possible combinatorial explosion in the number of possible values. If you do not have an existing entry with the same key, no new entry can be the same as an existing one---therefore, the former allow only distinct values for the same key can just say trailer.Fixes.action = add_if_different or something, without any if_exists/if_missing. In a similar way, the latter duplicated values allowed for the same key can say trailer.Sob.action = add You can throw into the mix other actions like add_if_missing, you can also express only one value allowed for the same key---the first one wins, replace to mean replace if there is one with the same key, and replace_or_add to mean same as 'replace', but add if there is no existing entries with the same key. Will we lose expressiveness by simplifying the semantics, getting rid of this under this condition part and instead making do this part more intelligent? I don't think we will lose expressiveness, but I think there might be a combinatorial explosion in the number of choices. Right now there are 2 choices for if_missing and 5 choices for if_exists. This means that if we use only one action config variable and want to have the same expressiveness, we need 10 choices. It doesn't seem a big difference now, but, if we add more choices, then the difference will increase a lot. Even if we assume, for the sake of discussion, that it *is* a good idea to separate under this condition part and do this part, I do not think the above is the only or the best way to express distinct values allowed for the same key. How do we argue that this from your example if_exists = add_if_different if_missing = add is a better design than this, for example? if_key_value_exists = ignore ; exact matching key,val exists if_key_exists = add ; otherwise if_missing = add The question is what happens if we want to say if the same key, value pair exists but not near where we would add. With the solution I implemented, that is: if_exists = add_if_different_neighbor With the solution you suggest, should we have: if_key_value_exists_not_neighbor = add ; exact matching key,val exists but not near where we would add if_key_value_exists = ignore ; exact matching key,val exists if_key_exists = add ; otherwise if_missing = add or: if_key_value_exists = ignore_if_neighbor ; exact matching key,val exists if_key_exists = add ; otherwise if_missing = add And what happens if we want to say if key exists and value matches regex, how do we express that then? Or what happens when we want to say if key exists and command succeeds? With what I suggest, we can still say: if_exists = add_if_values_matches regex or if_exists = add_if_succeeds cmd With the solution you suggest, should it be: if_key_value_exists = ignore if_key_exists = add_if_values_matches regex and if_key_value_exists = ignore if_key_exists = add_if_succeeds cmd ? Doesn't it look like redondant between the 2 lines? And then people might ask if they can also use something like this: if_key_value_exists = add_if_succeeds cmd1 if_key_exists = add_if_succeeds cmd2 if_key_missing = add_if_succeeds cmd3 and it will look like we are trying too much to do what should be done in only one script. The latter splits remaining conditional logic from do this part (no more add_if_different that causes add action to see if there is the same key and act differently) and moves it to under this condition part. I am trying to illustrate that the line to split the under this condition and do this looks arbitrary and fuzzy here, and because of this arbitrary-ness and fuzziness, it is not very obvious to me why it is a good idea to have under this condition as a separate concept from do this settings. What is the advantage of having such an extra axis? Does it
Re: [PATCH v5 02/14] trailer: process trailers from file and arguments
Christian Couder christian.cou...@gmail.com writes: Even if we assume, for the sake of discussion, that it *is* a good idea to separate under this condition part and do this part, I do not think the above is the only or the best way to express distinct values allowed for the same key. How do we argue that this from your example if_exists = add_if_different if_missing = add is a better design than this, for example? if_key_value_exists = ignore ; exact matching key,val exists if_key_exists = add ; otherwise if_missing = add The question is what happens if we want to say if the same key, value pair exists but not near where we would add. With the solution I implemented, that is: ... With the solution you suggest, should we have: ... if_key_value_exists_not_neighbor = add ; exact matching key,val ... or: if_key_value_exists = ignore_if_neighbor ; exact matching key,val exists ... And what happens if we want to say if key exists and value matches regex, how do we express that then? Or what happens when we want to say if key exists and command succeeds? ... With what I suggest, we can still say: ... And then people might ask if they can also use something like this: ... and it will look like we are trying too much to do what should be done in only one script. I think the above illustrates my point very clearly. These numerous questions you have to ask are indications why choosing this condition goes to the left hand side of the equal sign (e.g. exists) and this condition goes to the right hand side (e.g. do-this-if_neighbor) is not working well. The user has to remember which conditions go to the variable name and which conditions go to the action part. And the made-up alternative you listed above is not a solution I suggest to begin with---it is a strawman if we assume such a splitting were a good idea in the first place ;-). What I was wondering if it is an better alternative was the below. The latter splits remaining conditional logic from do this part (no more add_if_different that causes add action to see if there is the same key and act differently) and moves it to under this condition part. ... I am trying to illustrate that the line to split the under this condition and do this looks arbitrary and fuzzy here, and because of this arbitrary-ness and fuzziness, it is not very obvious to me why it is a good idea to have under this condition as a separate concept from do this settings. That is, not splitting the logic into two parts like you did, i.e. if_X = do_Y_if_Z, which invites why is it not if_true = do_Y_if_X_and_Z, if_X_and_Z = do_Y, if_Z = do_Y_if_X? One possible way to avoid the confusion is to say do_Y_if_X_and_Z without making the rule split into conditions and actions---I am NOT saying that is _better_, there may be other solutions to avoid this two-part logic in a cleaner way. -- 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 v5 02/14] trailer: process trailers from file and arguments
Christian Couder chrisc...@tuxfamily.org writes: This is what if_exists and if_missing are all about. Either: the same key already exists regardless of the value and, in this case, what happens depends on what has been specified using the if_exists configuration variable. Or: the same key DOES NOT already exists regardless of the value and in this case, what happens depends on what has been specified using the if_missing configuration variable. Hmm, how should things like signed-off-by be handled? You may want to allow many entries with the same key with distinct values, but duplicated values may want to be handled differently (currently we only avoid to place a duplicate key, value consecutively, but keys with different semantics (e.g. fixes-bug: #bugid) may want to say unless the same key with the same value exists, append it at the end. -- 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 v5 02/14] trailer: process trailers from file and arguments
From: Junio C Hamano gits...@pobox.com Subject: Re: [PATCH v5 02/14] trailer: process trailers from file and arguments Date: Mon, 10 Feb 2014 10:14:34 -0800 Christian Couder chrisc...@tuxfamily.org writes: This is what if_exists and if_missing are all about. Either: the same key already exists regardless of the value and, in this case, what happens depends on what has been specified using the if_exists configuration variable. Or: the same key DOES NOT already exists regardless of the value and in this case, what happens depends on what has been specified using the if_missing configuration variable. Hmm, how should things like signed-off-by be handled? You may want to allow many entries with the same key with distinct values, but duplicated values may want to be handled differently (currently we only avoid to place a duplicate key, value consecutively, but keys with different semantics (e.g. fixes-bug: #bugid) may want to say unless the same key with the same value exists, append it at the end. Many entries with the same key but distinct values can be configured using: if_exists = add_if_different if_missing = add Many entries with the same key but values that can be the same can be configured using: if_exists = add if_missing = add The place where the trailers are added, if they should be added, can be selected using either where = after, the default, or where = before. where = after means just after trailers with the same key, or, if there are no trailers with the same key, after all the existing trailers. where = before means just before trailers with the same key, or, if there are no trailers with the same key, before all the existing trailers. I think that this is enough to handle most of the usual cases, but we can add other more complex options later, as I said in the last message of this thread: http://thread.gmane.org/gmane.comp.version-control.git/237278/ Thanks, Christian. -- 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 v5 02/14] trailer: process trailers from file and arguments
Christian Couder chrisc...@tuxfamily.org writes: Many entries with the same key but distinct values can be configured using: if_exists = add_if_different if_missing = add Many entries with the same key but values that can be the same can be configured using: if_exists = add if_missing = add While the above certainly can express the combinations, I am still puzzled about the value of having under this condition (i.e. if-exists/if-missing) and do this (i.e. add/add-if-different) as two separate concepts. If you do not have an existing entry with the same key, no new entry can be the same as an existing one---therefore, the former allow only distinct values for the same key can just say trailer.Fixes.action = add_if_different or something, without any if_exists/if_missing. In a similar way, the latter duplicated values allowed for the same key can say trailer.Sob.action = add You can throw into the mix other actions like add_if_missing, you can also express only one value allowed for the same key---the first one wins, replace to mean replace if there is one with the same key, and replace_or_add to mean same as 'replace', but add if there is no existing entries with the same key. Will we lose expressiveness by simplifying the semantics, getting rid of this under this condition part and instead making do this part more intelligent? Even if we assume, for the sake of discussion, that it *is* a good idea to separate under this condition part and do this part, I do not think the above is the only or the best way to express distinct values allowed for the same key. How do we argue that this from your example if_exists = add_if_different if_missing = add is a better design than this, for example? if_key_value_exists = ignore ; exact matching key,val exists if_key_exists = add ; otherwise if_missing = add The latter splits remaining conditional logic from do this part (no more add_if_different that causes add action to see if there is the same key and act differently) and moves it to under this condition part. I am trying to illustrate that the line to split the under this condition and do this looks arbitrary and fuzzy here, and because of this arbitrary-ness and fuzziness, it is not very obvious to me why it is a good idea to have under this condition as a separate concept from do this settings. What is the advantage of having such an extra axis? Does it make it easier to manage? Does it allow us to express more? I can see that the location where a new entry (or a duplicated one) is added (i.e. do we prepend? do we append?) can be orthogonal to any of the above; no need to bring up 'where' in the discussion. -- 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 v5 02/14] trailer: process trailers from file and arguments
From: Junio C Hamano gits...@pobox.com
Christian Couder chrisc...@tuxfamily.org writes:
+static void apply_arg_if_exist(struct trailer_item *infile_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len)
+{
+switch (arg_tok-conf-if_exist) {
+case EXIST_DO_NOTHING:
+free(arg_tok);
+break;
+case EXIST_OVERWRITE:
+free((char *)infile_tok-value);
+infile_tok-value = xstrdup(arg_tok-value);
+free(arg_tok);
+break;
+case EXIST_ADD:
+add_arg_to_infile(infile_tok, arg_tok);
+break;
+case EXIST_ADD_IF_DIFFERENT:
+if (check_if_different(infile_tok, arg_tok, alnum_len, 1))
+add_arg_to_infile(infile_tok, arg_tok);
+else
+free(arg_tok);
+break;
+case EXIST_ADD_IF_DIFFERENT_NEIGHBOR:
+if (check_if_different(infile_tok, arg_tok, alnum_len, 0))
+add_arg_to_infile(infile_tok, arg_tok);
+else
+free(arg_tok);
+break;
Makes me wonder if people want a rule to say if the same key
already exists, regardless of the value.
This is what if_exists and if_missing are all about.
Either:
the same key already exists regardless of the value
and, in this case, what happens depends on what has been specified using
the if_exists configuration variable.
Or:
the same key DOES NOT already exists regardless of the value
and in this case, what happens depends on what has been specified
using the if_missing configuration variable.
+static void remove_from_list(struct trailer_item *item,
+ struct trailer_item **first)
+{
+if (item-next)
+item-next-previous = item-previous;
+if (item-previous)
+item-previous-next = item-next;
+else
+*first = item-next;
+}
Will callers free the item that now is not on the list?
Yes, or the item will be inserted into another list.
+static struct trailer_item *remove_first(struct trailer_item **first)
+{
+struct trailer_item *item = *first;
+*first = item-next;
+if (item-next) {
+item-next-previous = NULL;
+item-next = NULL;
+}
+return item;
+}
+
+static void process_infile_tok(struct trailer_item *infile_tok,
+ struct trailer_item **arg_tok_first,
+ enum action_where where)
+{
+struct trailer_item *arg_tok;
+struct trailer_item *next_arg;
+
+int tok_alnum_len = alnum_len(infile_tok-token,
strlen(infile_tok-token));
+for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+next_arg = arg_tok-next;
+if (same_token(infile_tok, arg_tok, tok_alnum_len)
+arg_tok-conf-where == where) {
+remove_from_list(arg_tok, arg_tok_first);
+apply_arg_if_exist(infile_tok, arg_tok, tok_alnum_len);
+/*
+ * If arg has been added to infile,
+ * then we need to process it too now.
+ */
+if ((where == WHERE_AFTER ? infile_tok-next :
infile_tok-previous) == arg_tok)
+infile_tok = arg_tok;
+}
+}
+}
+
+static void update_last(struct trailer_item **last)
+{
+if (*last)
+while((*last)-next != NULL)
+*last = (*last)-next;
+}
+
+static void update_first(struct trailer_item **first)
+{
+if (*first)
+while((*first)-previous != NULL)
+*first = (*first)-previous;
+}
+
+static void apply_arg_if_missing(struct trailer_item **infile_tok_first,
+ struct trailer_item **infile_tok_last,
+ struct trailer_item *arg_tok)
+{
Makes me wonder if it would make the code simpler to keep an anchor
item struct trailer_item that is off heap, and pass that single
anchor item around, using its next/prev fields as the first and the
last. Wouldn't it let you remove the special cases for the first
and last item?
Yeah, that could work. On the other hand the other fields of this
special item would not be used for anything.
I will have a look at it.
+struct trailer_item **infile_tok;
+enum action_where where;
+
+switch (arg_tok-conf-if_missing) {
+case MISSING_DO_NOTHING:
+free(arg_tok);
+break;
+case MISSING_ADD:
+where = arg_tok-conf-where;
+infile_tok = (where == WHERE_AFTER) ? infile_tok_last :
infile_tok_first;
+if (*infile_tok) {
+add_arg_to_infile(*infile_tok, arg_tok);
+*infile_tok = arg_tok;
+} else {
+
Re: [PATCH v5 02/14] trailer: process trailers from file and arguments
Christian Couder chrisc...@tuxfamily.org writes:
This patch implements the logic that process trailers
from file and arguments.
At the beginning trailers from file are in their own
infile_tok doubly linked list, and trailers from
arguments are in their own arg_tok doubly linked list.
The lists are traversed and when an arg_tok should be
applied, it is removed from its list and inserted
into the infile_tok list.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
trailer.c | 187
++
1 file changed, 187 insertions(+)
diff --git a/trailer.c b/trailer.c
index f129b5a..ba0cfe0 100644
--- a/trailer.c
+++ b/trailer.c
@@ -46,3 +46,190 @@ static inline size_t alnum_len(const char *buf, int len)
while (--len = 0 !isalnum(buf[len]));
return (size_t) len + 1;
}
+
+static void add_arg_to_infile(struct trailer_item *infile_tok,
+ struct trailer_item *arg_tok)
+{
+ if (arg_tok-conf-where == WHERE_AFTER) {
+ arg_tok-next = infile_tok-next;
+ infile_tok-next = arg_tok;
+ arg_tok-previous = infile_tok;
+ if (arg_tok-next)
+ arg_tok-next-previous = arg_tok;
+ } else {
+ arg_tok-previous = infile_tok-previous;
+ infile_tok-previous = arg_tok;
+ arg_tok-next = infile_tok;
+ if (arg_tok-previous)
+ arg_tok-previous-next = arg_tok;
+ }
+}
+
+static int check_if_different(struct trailer_item *infile_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len, int check_all)
+{
+ enum action_where where = arg_tok-conf-where;
+ do {
+ if (!infile_tok)
+ return 1;
+ if (same_trailer(infile_tok, arg_tok, alnum_len))
+ return 0;
+ /*
+ * if we want to add a trailer after another one,
+ * we have to check those before this one
+ */
+ infile_tok = (where == WHERE_AFTER) ? infile_tok-previous :
infile_tok-next;
+ } while (check_all);
+ return 1;
+}
+
+static void apply_arg_if_exist(struct trailer_item *infile_tok,
+struct trailer_item *arg_tok,
+int alnum_len)
+{
+ switch (arg_tok-conf-if_exist) {
+ case EXIST_DO_NOTHING:
+ free(arg_tok);
+ break;
+ case EXIST_OVERWRITE:
+ free((char *)infile_tok-value);
+ infile_tok-value = xstrdup(arg_tok-value);
+ free(arg_tok);
+ break;
+ case EXIST_ADD:
+ add_arg_to_infile(infile_tok, arg_tok);
+ break;
+ case EXIST_ADD_IF_DIFFERENT:
+ if (check_if_different(infile_tok, arg_tok, alnum_len, 1))
+ add_arg_to_infile(infile_tok, arg_tok);
+ else
+ free(arg_tok);
+ break;
+ case EXIST_ADD_IF_DIFFERENT_NEIGHBOR:
+ if (check_if_different(infile_tok, arg_tok, alnum_len, 0))
+ add_arg_to_infile(infile_tok, arg_tok);
+ else
+ free(arg_tok);
+ break;
Makes me wonder if people want a rule to say if the same key
already exists, regardless of the value.
+ }
+}
+
+static void remove_from_list(struct trailer_item *item,
+ struct trailer_item **first)
+{
+ if (item-next)
+ item-next-previous = item-previous;
+ if (item-previous)
+ item-previous-next = item-next;
+ else
+ *first = item-next;
+}
Will callers free the item that now is not on the list?
+static struct trailer_item *remove_first(struct trailer_item **first)
+{
+ struct trailer_item *item = *first;
+ *first = item-next;
+ if (item-next) {
+ item-next-previous = NULL;
+ item-next = NULL;
+ }
+ return item;
+}
+
+static void process_infile_tok(struct trailer_item *infile_tok,
+struct trailer_item **arg_tok_first,
+enum action_where where)
+{
+ struct trailer_item *arg_tok;
+ struct trailer_item *next_arg;
+
+ int tok_alnum_len = alnum_len(infile_tok-token,
strlen(infile_tok-token));
+ for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+ next_arg = arg_tok-next;
+ if (same_token(infile_tok, arg_tok, tok_alnum_len)
+ arg_tok-conf-where == where) {
+ remove_from_list(arg_tok, arg_tok_first);
+ apply_arg_if_exist(infile_tok, arg_tok, tok_alnum_len);
+ /*
+ * If arg has been added to infile,
+ * then we need to process it
