About API code comments and some behavior.

[Mads Kiilerich]

On 30/10/2022 13:00, toras wrote:
> > Yes. Can you confirm this will solve the code problems you reported:
> > 
> https://kallithea-scm.org/repos/kallithea-incoming/changeset/088551a24485247af328f0b8e395fdbbcd62a700 
> ?
>
> 1. Modify create_repo()
>   I don't think this change is correct.
>   I overwrote api.py and called create_repo, but 
> enable_downloads/enable_statistics seems to have no effect.
>
>   I think the call flow during API execution is as follows.
>
>   api.py : ApiController.create_repo()
>     repo.py : RepoModel.create()
>       repo.py : (@celerylib.task) create_repo()
>
>   I think that the third function does not refer to form_data, but 
> refers to defs to obtain the default value and use it.
> https://kallithea-scm.org/repos/kallithea/files/2dd317e9cc4b5ff94c1ddae7ee772d20b61259a9/kallithea/model/repo.py#L717
>   I think this is probably implemented in conjunction with the web 
> front end.
>
>   I see two options for changes to the create_repo API.
>   I think this requires a policy decision.
>
>   Scenario 1:
>     The API will no longer accept that parameter.
>     Attachment: 1-scenario1.patch
>
>   Scenario 2:
>     Support parameter specification in RepoModel.
>     Attachment: 1-scenario2.patch


Right. Thanks. I will go with scenario2 ... but take it further and make 
repo_enable_downloads/repo_enable_statistics mandatory in the internal 
API and expose them in the UI form as well. As a part of that, I will 
also add better test coverage and clean up the UI forms.


> 2. Modify update_repo_group()
>   I think there are two issues with parent handling.
>
>   The first is.
>   Other api parameters can basically target entities by name.
>   It seems to me that as the parent_group_id parameter to 
> RepoGroupModel.update(), the API parameter should be passed with name 
> resolution instead of passing it as is.
>     Attachment: 2-parent_id.patch


Agreed. I will do that ... but with slight changes.


>   The second is still unresolved and not yet known in detail.
>   The problem is that changing the parent group via the API will leave 
> the database and file system in an inconsistent state.
>   At that time, new_path does not seem to indicate the correct path in 
> RepoGroupModel.update(). So the folder is not moved.
>   It seems correct when run from the web front end. I still don't know 
> what the difference is.

...

> I have found the cause of the problem with the update_repo_group API 
> behavior that I wrote about in my previous email.
> The implementation of RepoGroupModel.update() does not seem to work 
> correctly unless it includes a valid group_name in args, even if the 
> name is not changed.
> If no new name is specified for the API parameter, it may be necessary 
> to pass the original name.
> I have created a patch to api.py based on 088551a24485 from 
> kallithea-imcoming.
>     Attachment: fix-update_repo_group-1.patch

Agreed, but I think the problem is in RepoGroupModel.update , where it 
failed to update group_name to the new full path path after moving *if* 
group_name wasn't specified. I will fix that instead.


> Also, I thought it was not good to have inconsistent results depending 
> on the call parameters, so I refactored the implementation of 
> RepoGroupModel.update().
> It also includes changes that correct incorrect logging output. 
> However, this is not required for API operation.
>     Attachment: fix-update_repo_group-2.patch

Lots of things happening in that patch. Can you explain in more details 
what is changed and why ... and perhaps do it in several simpler changesets?

For example:

What inconsistent results did you see?

What logging was incorrect? (But to avoid pointless formatting of log 
strings that will be ignored because of the logging level, we generally 
prefer to avoid using log.debug('%s' % x) but prefer log.debug('%s', x) .)

Also note that repo_group.parent_group is a convenience thing in the 
Object Relationship Mapper to automatically fetch whatever 
repo_group.parent_group_id refers to. When one of them are changed, the 
other one should change too.


> Supplementation.
> It may not be necessary, but I will publish the docker files and 
> scripts that I used in the confirmation work.
> https://github.com/toras9000/kallithea_api_test


Thank you. There are so many ways to do Docker. Great that your setup 
can cover this and provide additional testing compared to what is in the 
upstream repo.


I took a bit of a chance and pushed the changes to the stable branch. I 
hope you can confirm it works for you now.

/Mads