[Aptitude-devel] Review: 001-build-skeleton

[Piotr Galiszewski]

2010/7/10 Daniel Burrows <dburrows at debian.org>:
> On Sat, Jul 10, 2010 at 06:23:22PM +0200, Piotr Galiszewski <piotr at galiszewski.pl> was heard to say:
>> 2010/7/10 Daniel Burrows <dburrows at debian.org>:
>> >> +  # required version of Qt libraries
>> >> +  QTCORE_REQUIRED=4.6.0
>> >> +  QTGUI_REQUIRED=4.6.0
>> >
>> >  (snip)
>> >
>> >  Are these versions actually required by our code?
>> >
>>
>> Actually not, but I am not developing with this versions in my system.
>> Also this versions are available on testing and unstable, so I thought
>> that it is reasonable requirement. Of course I can try to not use
>> newer methods, and lower requirements
>
>  I don't think we need to avoid newer methods.  How about just
> setting this to the earliest version that you *know* we require (I
> assume we need at least 4.0, for instance), and we can tighten it as
> we become aware of the need to do so.  All this does is move errors to
> an earlier step of the compile process, so we might as well not put
> unnecessary obstacles in front of users with older versions of Qt.
>

OK. I will check it tomorrow

>> I did not check PKG_CHECK_MODULES macro. Now, I have removed all
>> manual PKG_CONFIG checking. I have also resigned from  fallback
>> values. It is hard to find good values, and probably it will be better
>> to add --with-qt-dir option in the future :)
>
>  Sounds good.
>
>> >  Could you add "// For the Qt version" here?
>> >
>>
>> done
>
>  Sorry, one more change request -- instead say "To get the Qt version
> number".
>

done

>> >>    {"log-level", 1, &getopt_result, OPTION_LOG_LEVEL},
>> >>    {"log-file", 1, &getopt_result, OPTION_LOG_FILE},
>> >>    {"log-config-file", 1, &getopt_result, OPTION_LOG_CONFIG_FILE},
>> >> @@ -656,6 +678,11 @@ int main(int argc, char *argv[])
>> >>    bool use_new_gui = false;
>> >>  #endif
>> >
>> >> +#ifdef HAVE_QT
>> >> +  // Use Qt frontend.
>> >> +  bool use_qt_gui = false;
>> >> +#endif
>> >
>> >  Could you change "gui" to "use_gtk_gui" and "use_new_gui" to
>> > "use_new_gtk_gui"?  I'd also like to change the configuration option
>> > to control starting the GUI by default, but we can do that later
>> > (Aptitude::Gui=none|gtk|qt, deprecate Aptitude::Start-Gui).
>> >
>>
>> done
>
>  This change broke the indentation of some lines:
>
>            case OPTION_GUI:
> -             gui = true;
> +        use_gtk_gui = true;
>              break;
>            case OPTION_NO_GUI:
> -             gui = false;
> +        use_gtk_gui = false;
>              break;
>  #else
>            case OPTION_NO_GUI:
>              // Recognize it as a NOP.
>              break;
>  #endif
> +
>            case OPTION_LOG_LEVEL:
>
>  Also, there's an extra blank line at the end of that block.
>
>  Indentation is also wrote for these case blocks (the first one is
> correct):
>
>            case OPTION_NEW_GUI:
> #ifdef HAVE_GTK
>              use_new_gtk_gui = true;
> #endif
>              break;
>
> #ifdef HAVE_QT
>      case OPTION_QT_GUI:
>        use_qt_gui = true;
>        break;
>
>      case OPTION_NO_QT_GUI:
>        use_qt_gui = false;
>        break;
> #endif
>

I am sorry about that. That was a problem with invalid settings of my
editor. Visually it looked correct on him. Both problems should be
fixed now

(snip)

>
>> On the other hand, I have learned today, that using git rebase -i with
>> commit in first branch could be "a little" problematic for other
>> branches ;)
>
>  This gave me a little trouble the first time I tried it too.  You
> need to use the "--onto" option if you're transplanting patches onto
> an unrelated branch.  Here's the relevant diagram from the manual:
>
> ----- Cut here -----
>
>       Here is how you would transplant a topic branch based on one
>       branch to another, to pretend that you forked the topic
>       branch from the latter branch, using rebase --onto.
>
>       First let’s assume your topic is based on branch next. For
>       example, a feature developed in topic depends on some
>       functionality which is found in next.
>
>               o---o---o---o---o  master
>                    \
>                     o---o---o---o---o  next
>                                      \
>                                       o---o---o  topic
>       We want to make topic forked from branch master; for example,
>       because the functionality on which topic depends was merged
>       into the more stable master branch. We want our tree to look
>       like this:
>
>               o---o---o---o---o  master
>                   |            \
>                   |             o'--o'--o'  topic
>                    \
>                     o---o---o---o---o  next
>
>
>       We can get this using the following command:
>
>           git rebase --onto master next topic
>
> ----- Cut here -----
>
>  The important thing to remember with this form of rebase is that
> "next" is the most recent patch that *isn't* included in the rebasing.
> I usually just run gitk and copy SHA values out of it when I'm doing
> this.
>
>  I think the same logic applies when you're using "--interactive",
> but I've only really used that for in-place rebasing (rewriting the
> most recent commits on a branch, but leaving them attached to the same
> parent).
>

Thanks for the informations. I will learn more about it. I have used
git rebase -i + cherry-picking newer commits, Probably onto will be
better ;)

>  Daniel
>

-- 
Regards
Piotr Galiszewski