[Kde-extra-gear] [PATCH] cvsExtract.sh

[Klas Kalass]

Am Freitag, 13. Februar 2004 11:12 schrieb Jeroen Wijnhout:
> On Thursday 12 February 2004 23:45, Klas Kalass wrote:
> > I noticed that you use pushd and popd - you can then remove the subshells
> > I opened (the simple braces) where you introduced pushd/popd.
>
> Done.
>
> > Nitpicking: For reading it would probably be nice if global variables
> > were set before they are used in function definitions. An example is
> > CVS_CHECKOUT_OPTIONS_I18N_BASE.
> >
> > By the way, that variable is the one I was referring to as not beeing
> > correctly set in the other mail.
> >
> > You only set it if USETAGFORI18NBASE is true, but the name of the
> > variable you replace is CVS_CHECKOUT_OPTIONS and not 'TAG_OR_BRANCH', so
> > you should not leave it unset if simply no Tag is requested for I18N. The
> > same goes
>
> Ok, it is initialized now.
Hmm, I think you did not understand me. If someone adds some more support for
CVS Options as a parameter (s)he will most certainly set CVS_CHECKOUT_OPTIONS 
in the command line handling.  Now, if the user chooses to use 
--noi18nbasetag  all those values will be lost. It is really easy to avoid 
this future bug by introducing $TAG_OR_BRANCH now and using it in addition to 
CVS_CHECKOUT_OPTIONS, which is more logical too.

The semantics of the --noi18nbasetag and --noadminbasetag is to "not use the 
specified tag or branch" for i18n and admin. You should implemented it like 
this, not make "--i18n-ignore-cvs-options" out of it.

Anyways, code is clearer than words so go ahead and commit. I will change 
it :-)

Regards,
  Klas

P.S: I am not that happy about the two options (admin and i18n). The tagging 
script should rather be completed and used to tag admin and i18n. IMHO at 
least i18n *should* be tagged, because you will want to change strings during 
development and will end up with different strings in branch and head 
eventually. 
When you release 2.0 and do a bugfix version 1.1.3 you can be sure that 
translations for 1.1.1 will be more incomplete than for 1.1.0. 

You might also be unlucky that your 1.1.3 suddenly does not build anymore 
because something fundamental was changed in the admin dir. 

But this is just a sidenote everybody should be aware of. I see the practical 
use of the options right now. And since this script is about beeing usefull 
*now* my concerns are not an objection.