[PATCH 0 of 7 WIPv2] Introduction of kallithea-cli command

[Mads Kiilerich]

On 10/8/18 9:57 PM, Thomas De Schampheleire wrote:
> Hi,
>
> This is v2 of the WIP.


Thanks. This looks good. I guess I would have applied it if you didn't 
call it WIP. Now, see if you want to make additional tweaks based on my 
minor feedback. I look forward to the final non-WIP series. Perhaps with 
the slightly more experimental front-end command on top.


> Specific questions about remaining commands:
>
> - gearbox update-repoinfo has following options:
>          parser.add_argument('--update-only',
>                             action='store',
>                             dest='repo_update_list',
>                             help="Specifies a comma separated list of repositories "
>                                  "to update last commit info for. OPTIONAL")
>          parser.add_argument('--invalidate-cache',
>                             action='store_true',
>                             dest='invalidate_cache',
>                             help="Trigger cache invalidation event for repos. "
>                                  "OPTIONAL")
>
>    In fact, this command can do two different things:
>    1. update the changeset cache (seemingly a JSON array with changeset
>    information)
>    2. invalidate the 'cache' of the repository. But this is a different cache
>    than the changeset cache above. What the difference exactly is, is unclear
>    to me. Enabling this action is steered via '--invalidate-cache'.
>
>    For the new CLI, I would create two separate commands as these things are
>    seemingly unrelated, or alternatively group them into one command without
>    --invalidate-cache option, i.e. do both 'cache' updates/invalidation
>    actions in one go. The latter approach would be good if there is no good
>    reason to want one but not the other.
>
>    What do you prefer? How would you name these commands?


One cache is actually caching the latest change from the file system 
repo into the database, mainly to be shown in repo listings (showing a 
repo group) without having to touch each repo each time. It could be 
named something with "latest" or "tip" or "listing info".

The other cache is actually in memory, trying to reuse repo VCS objects 
in memory ... at the cost of a lot of extra database hits. The command 
is only about invalidating the caches so they can be re-populated 
automatically. It could be named something with "invalidate internal cache".

(I guess this "beaker" cache could be ripped out without significant 
negative performance impact, and then perhaps replaced by something 
simpler and more efficient.)

Perhaps most important: While they are different at the low level, they 
are mostly implementation details. At the high level, it makes sense to 
have to tell the system if repos have changed externally, but I don't 
really see any good use cases for updating one without the other? So 
perhaps instead just make it one command and always run both? Or really 
and essentially keep it as one command, but with "secret" options for 
only running one or the other. From the user perspective, it is really 
about "repo changed" or "update repo info", so it might belong as a sub 
command under "repo"?


> - gearbox make-index has following options:
>
>          parser.add_argument('--repo-location',
>                            action='store',
>                            dest='repo_location',
>                            help="Specifies repositories location to index OPTIONAL",
>                            )
>          parser.add_argument('--index-only',
>                            action='store',
>                            dest='repo_list',
>                            help="Specifies a comma separated list of repositories "
>                                  "to build index on. If not given all repositories "
>                                  "are scanned for indexing. OPTIONAL",
>                            )
>          parser.add_argument('--update-only',
>                            action='store',
>                            dest='repo_update_list',
>                            help="Specifies a comma separated list of repositories "
>                                  "to re-build index on. OPTIONAL",
>                            )
>          parser.add_argument('-f',
>                            action='store_true',
>                            dest='full_index',
>                            help="Specifies that index should be made full i.e"
>                                  " destroy old and build from scratch",
>                            default=False)
>
>    Do you know how this command works and what each option does? Could you
>    clarify?
>    And how should this be untangled in the new CLI?


I didn't look much at indexing. By default, I guess we just should try 
to keep it until we have good understanding and good reasons to change it.

The thing about specifying a set of repos seems like a common thing for 
several repo commands. But comma separation is odd and fragile. Instead, 
it should probably take them as separate arguments somehow.

Repo location seems odd - I would expect that to be controlled 100% from 
the .ini file.

So I would say: Just try to make minimal changes and avoid breaking 
anything. It will need a big clean-up ... but we don't want to do that now.


/Mads