Re: [PATCH] branch.c: simplify chain of if statements
On Mon, Mar 17, 2014 at 7:46 AM, Dragos Foianu dragos.foi...@gmail.com wrote: Eric Sunshine sunshine at sunshineco.com writes: Matthieu already mentioned [2] that this sort of lego string construction is not internationalization-friendly. See section 4.3 [3] of the gettext manual for details. I was hoping to get away with using less memory by having only four entries in the table. I suppose that is not possible. The rebasing check can still be moved outside of the four if statements and calculate the index correctly. The strings would then have to be arranged in such a way to make this work. Using a multiple-dimension array as suggested in other submissions for this particular microproject would probably be better, but it has already been done. If a multi-dimension table is indeed better than other alternatives, then that's a good reason to choose it, even if others have already used that approach in their submissions. It's more important that the code is clean and easy to understand and maintain than to be clever. If you're really interested in trying an approach not already submitted by others, take a look at Jonathan's idea [1]. If you play around with it and find that it actually does make the code clearer and simpler, then perhaps it's worth submitting. If not, then not. [1]: http://thread.gmane.org/gmane.comp.version-control.git/198882/focus=198902 These hard-coded indexing constants (0, 1, 2, 3) are fragile and convey little meaning to the reader. Try to consider how to compute the index into verbose_prints[] based upon the values of 'remote_is_branch' and 'origin'. What are the different ways you could do so? I was going to do something like this: if !remote_is_branch the index goes incremented by 2, because the first two entries are of no interest and if !origin, the index is incremented by 1. This would correctly compute the index. It should also work with the rebasing check if the four rebasing-specific messages are at the end of the table and when rebasing the index is set to start at those messages. The reason I did not go with this is because I would still need the four ifs in order to keep the bug check part of the code. I might be able to find a work-around for it on the second attempt. Since the result is just a number, its possible to compute it directly without conditionals, however, it does start resembling a magical incantation. (I'll comment further in your v2 submission.) I have seen N_() used in other code but I wasn't sure what its purpose was. Thank you very much for the review. -- 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] branch.c: simplify chain of if statements
Dragos Foianu dragos.foi...@gmail.com writes: + const char *verbose_prints[4] = { + Branch %s set up to track remote branch %s from %s%s, + Branch %s set up to track local branch %s%s, + Branch %s set up to track remote ref %s%s, + Branch %s set up to track local ref %s%s + }; + char *verbose_rebasing = rebasing ? by rebasing. : .; + This seems to be a lego construct that makes translation harder: are you sure that the by rebasing will be at the end of the sentence in any languages? Also, this lacks the _() on verbose_rebasing, which isn't translatable anymore after your patch. I personnally think that the table-driven approach is wrong here, it makes the code shorter but much harder to read. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] branch.c: simplify chain of if statements
On Mon, Mar 17, 2014 at 3:23 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Dragos Foianu dragos.foi...@gmail.com writes: + const char *verbose_prints[4] = { + Branch %s set up to track remote branch %s from %s%s, + Branch %s set up to track local branch %s%s, + Branch %s set up to track remote ref %s%s, + Branch %s set up to track local ref %s%s + }; + char *verbose_rebasing = rebasing ? by rebasing. : .; + This seems to be a lego construct that makes translation harder: are you sure that the by rebasing will be at the end of the sentence in any languages? Read this thread [1] for more details about why this approach is problematic. [1]: http://thread.gmane.org/gmane.comp.version-control.git/243793/focus=243824 Also, this lacks the _() on verbose_rebasing, which isn't translatable anymore after your patch. I personnally think that the table-driven approach is wrong here, it makes the code shorter but much harder to read. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] branch.c: simplify chain of if statements
Thanks for the submission. Comments below to give you a taste of the Git review process... On Sun, Mar 16, 2014 at 5:22 PM, Dragos Foianu dragos.foi...@gmail.com wrote: This patch uses a table-driven approach in order to make the code cleaner. In fact, this change is not table-driven (emphasis on *driven*). It merely moves the strings into a table, but all the logic is still in the code. To be table-driven, the logic would be encoded in the table as well, and that logic would *drive* the code. This is not to say that the code must be table-driven. The GSoC microproject merely asks if doing so would make sense. Whether it would is for you to decide, and then to explain to reviewers the reason(s) why you did or did not make it so. Although not necessary, it helps code reability by not forcing the user to read the print message when trying to understand what the code does. The if-chain is just sufficiently complex that the print messages may actually help the reader understand the logic of the code, so this argument seems specious. The rebase check has been moved to the verbose if statement to avoid making the same check in each of the four if statements. Signed-off-by: Dragos Foianu dragos.foi...@gmail.com --- Overall, the patch appears to be properly constructed and you seem to have digested Documentation/SubmittingPatches. Good. branch.c | 32 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/branch.c b/branch.c index 723a36b..e2fe455 100644 --- a/branch.c +++ b/branch.c @@ -54,6 +54,14 @@ void install_branch_config(int flag, const char *local, const char *origin, cons struct strbuf key = STRBUF_INIT; int rebasing = should_setup_rebase(origin); + const char *verbose_prints[4] = { No need to hard-code 4. 'const char *verbose_prints[]' is sufficient. On this project, it is preferred (though not consistent) to name arrays using singular form, such as verbose_print[], so that accessing a single element, such as verbose_print[42], reads more grammatically correct. verbose_prints[] is not as descriptive as it could be. Perhaps something like verbose_messages[] would be more informative (though awfully long; maybe just messages[]). + Branch %s set up to track remote branch %s from %s%s, Even though you are correctly accessing these strings via _() in the printf_ln() invocation, you still need to mark them translatable here with N_(). See section 4.7 [1] of the gettext manual. [1]: http://www.gnu.org/software/gettext/manual/gettext.html#Special-cases + Branch %s set up to track local branch %s%s, + Branch %s set up to track remote ref %s%s, + Branch %s set up to track local ref %s%s + }; + char *verbose_rebasing = rebasing ? by rebasing. : .; This should be 'const char *'. Matthieu already mentioned [2] that this sort of lego string construction is not internationalization-friendly. See section 4.3 [3] of the gettext manual for details. [2]: http://thread.gmane.org/gmane.comp.version-control.git/244210/focus=244226 [3]: http://www.gnu.org/software/gettext/manual/gettext.html#Preparing-Strings if (remote_is_branch !strcmp(local, shortname) !origin) { @@ -78,25 +86,17 @@ void install_branch_config(int flag, const char *local, const char *origin, cons if (flag BRANCH_CONFIG_VERBOSE) { if (remote_is_branch origin) - printf_ln(rebasing ? - _(Branch %s set up to track remote branch %s from %s by rebasing.) : - _(Branch %s set up to track remote branch %s from %s.), - local, shortname, origin); + printf_ln(_(verbose_prints[0]), + local, shortname, origin, verbose_rebasing); else if (remote_is_branch !origin) - printf_ln(rebasing ? - _(Branch %s set up to track local branch %s by rebasing.) : - _(Branch %s set up to track local branch %s.), - local, shortname); + printf_ln(_(verbose_prints[1]), + local, shortname, verbose_rebasing); else if (!remote_is_branch origin) - printf_ln(rebasing ? - _(Branch %s set up to track remote ref %s by rebasing.) : - _(Branch %s set up to track remote ref %s.), - local, remote); + printf_ln(_(verbose_prints[2]), + local, remote, verbose_rebasing); else if (!remote_is_branch !origin) - printf_ln(rebasing ? -
Re: [PATCH] branch.c: simplify chain of if statements
Eric Sunshine sunshine at sunshineco.com writes: In fact, this change is not table-driven (emphasis on *driven*). It merely moves the strings into a table, but all the logic is still in the code. To be table-driven, the logic would be encoded in the table as well, and that logic would *drive* the code. This is not to say that the code must be table-driven. The GSoC microproject merely asks if doing so would make sense. Whether it would is for you to decide, and then to explain to reviewers the reason(s) why you did or did not make it so. The if-chain is just sufficiently complex that the print messages may actually help the reader understand the logic of the code, so this argument seems specious. I understand now after reading your review. I will try to fix this in a future attempt. Matthieu already mentioned [2] that this sort of lego string construction is not internationalization-friendly. See section 4.3 [3] of the gettext manual for details. I was hoping to get away with using less memory by having only four entries in the table. I suppose that is not possible. The rebasing check can still be moved outside of the four if statements and calculate the index correctly. The strings would then have to be arranged in such a way to make this work. Using a multiple-dimension array as suggested in other submissions for this particular microproject would probably be better, but it has already been done. These hard-coded indexing constants (0, 1, 2, 3) are fragile and convey little meaning to the reader. Try to consider how to compute the index into verbose_prints[] based upon the values of 'remote_is_branch' and 'origin'. What are the different ways you could do so? I was going to do something like this: if !remote_is_branch the index goes incremented by 2, because the first two entries are of no interest and if !origin, the index is incremented by 1. This would correctly compute the index. It should also work with the rebasing check if the four rebasing-specific messages are at the end of the table and when rebasing the index is set to start at those messages. The reason I did not go with this is because I would still need the four ifs in order to keep the bug check part of the code. I might be able to find a work-around for it on the second attempt. I have seen N_() used in other code but I wasn't sure what its purpose was. Thank you very much for the review. -- 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] branch.c: simplify chain of if statements
On Mon, Mar 17, 2014 at 12:46 PM, Dragos Foianu dragos.foi...@gmail.com wrote: The reason I did not go with this is because I would still need the four ifs in order to keep the bug check part of the code. I might be able to find a work-around for it on the second attempt. I have seen N_() used in other code but I wasn't sure what its purpose was. Aside from other comments here, more generally if you see code that looks odd it helps to see why it was introduced initially. In this case if you'd ran e.g.: git log --reverse -p -G'Branch %s set up to track remote branch %s from %s by rebasing' -- branch.c or otherwise searched for the first occurrence of that odd-looking code you'd have gotten: commit d53a3503 Author: Nguyễn Thái Ngọc Duy pclo...@gmail.com Date: Thu Jun 7 19:05:10 2012 +0700 Remove i18n legos in notifying new branch tracking setup Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com And searching for that commit has plenty of context for why that was done: https://www.google.com/search?q=%22Remove%20i18n%20legos%20in%20notifying%20new%20branch%20tracking%20setup%22 -- 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