Re: [PATCH] branch.c: simplify chain of if statements

2014-03-18 Thread Eric Sunshine
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

2014-03-17 Thread Matthieu Moy
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

2014-03-17 Thread Eric Sunshine
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

2014-03-17 Thread Eric Sunshine
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

2014-03-17 Thread Dragos Foianu
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

2014-03-17 Thread Ævar Arnfjörð Bjarmason
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