[RFC] incremental rebase

[Ken Brown]

On 11/24/2014 3:19 PM, Ken Brown wrote:
> On 11/23/2014 3:17 PM, Achim Gratz wrote:
>>
>> In my attempt to implement post-postinstall scripts for Ken to use for
>> TeXlive it turned out that the same hack I used for the pre-postinstall
>> perpetual scripts wouldn't really work.  So I've implemented the
>> stratified postinstall already, at the moment limited to three strata:
>> 0, default and z; as well as perpetual and runonce scripts (on each
>> stratum).  No provisions for query and trigger scripts is made at the
>> moment, as these can be implemented with some manual effort with the two
>> script types that are already provided.  This is extensible, although at
>> the moment I have to configure the available strata twice, so there
>> needs to be some more refactoring in that area.
>>
>> Since I've coded this up on my new Linux box which already uses gcc-4.9
>> I had to do some preparatory fixes (the log macro clashing with the log
>> function) leading up to the meat of the implementation.  The patch
>> implementing the stratified postinstall is here:
>>
>> http://repo.or.cz/w/cygwin-setup.git/commitdiff/d1df7acc1dce40c97ddfaa2de38542a3a269004e
>>
>
> Thanks for doing this!  A couple of comments:
>
>> bool
>>  Script::isAScript (const std::string& file)
>>  {
>> -    /* file may be /etc/postinstall or etc/postinstall */
>> -    if (casecompare(file, ETCPostinstall, sizeof(ETCPostinstall)-1) &&
>> -       casecompare(file, ETCPostinstall+1, sizeof(ETCPostinstall)-2))
>> -      return false;
>> -    if (file.c_str()[file.size() - 1] == '/')
>> +    // is a directory path
>> +    if (file.size() == file.rfind('/'))
>        ^^^^^^^^^^^^^^^
> Shouldn't this be file.size() - 1?
>
>> +bool
>> +Script::match (const std::string& stratum, const std::string& type)
>>  {
>> -
>> +  if ("done" == scriptExtension)
>> +    return false;
>> +  bool matchedStratum = false;
>> +  if (stratum.size())
>> +    {
>> +      std::size_t found = stratum.find(scriptStratum);
>> +      matchedStratum = ((std::string::npos != found) &&
>> +                       (std::string::npos !=
>> stratum.substr(found,1).find_first_of(allowedStrata)));
>
> Is this last test necessary?  The Script constructor guarantees that the
> stratum is allowable.
>
>> +    }
>> +  else
>> +    matchedStratum = true;
>> +  bool matchedType = true;
>> +  if (type.size())
>> +    {
>> +      std::size_t found = type.find(scriptType);
>> +      matchedType = ((std::string::npos != found) &&
>> +                    (std::string::npos !=
>> type.substr(found,1).find_first_of(allowedTypes)));
>
> Ditto.

Two more comments:

1. You should do something to make sure that incremental rebase (or 
whatever version of autorebase gets chosen) is run first among the 
perpetual postinstall scripts in stratum 0.  One possibility would be to 
also allow stratum 1, and require that no packages other than 
incremental rebase use stratum 0.  Alternatively, give the incremental 
rebase script a name that guarantees that it comes first in whatever 
order the perpetual scripts are run in.

2. You've used the underscore for two unrelated purposes: On the one 
hand, it separates the prefix from the rest of the script name.  On the 
other hand, it denotes the default stratum.  This makes it impossible to 
create a perpetual postinstall script in the default stratum if someone 
would ever want to do this.  Maybe use a hyphen instead of underscore 
for the separator?

Ken