savannah-hackers-public
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Savannah-hackers-public] Final-stage work/changes on adding the Git


From: Ineiev
Subject: Re: [Savannah-hackers-public] Final-stage work/changes on adding the Git homepage source code web browsing option.
Date: Sun, 14 Jan 2024 19:29:49 +0000

Hello, Ayushman;

On Fri, Jan 12, 2024 at 01:55:15AM +0530, Ayushman Tripathi wrote:
> Hello! I'm reaching the final stage of the process of implementing the
> option for the Git webpage repository.
> This patch basically covers the PHP changes.

What about the other end, linking the checkout of the repository
to website and updating it on the web server?

> -function form_radio ($name, $value, $attr)
> +function form_radio ($name, $value, $attr = [])

The default value is never used in your patch, and in fact,
form_radio makes little sense without at least a label, which has
no reasonable default.

> -      db_execute ("SELECT * FROM groups WHERE group_id = ?", [$id]);
> +      db_execute ("SELECT * FROM `groups` WHERE group_id = ?", [$id]);

Current Savane code doesn't use this kind of quoting; irrespectively
of whether they may improve the program, it seems to me it's
pointless to introduce this kind of inconsistency in an unrelated
modification.

> +  function isArtifactUrlSet ($artifact)
> +  {
> +    # Returns true if the artifact url is set for this group.
> +    return $this->data_array["url_$artifact"] != "";
> +  }

This function is never used, and I don't think it would be really
helpful.

> +  function getHomepageVcs ()
> +  {
> +    return $this->data_array['homepage_vcs'];
> +  }

The code implies a new field in the groups table, and in many
cases of access to data of the group it will not be used.
That alone is a good reason against it, and the compatibility
issues are even more important.

It's OK to keep this value in $this->data_array, but the code to
extract and update it should be different.

> @@ -743,7 +754,7 @@ function group_get_artifact_url ($artifact, $hostname = 1)
>    global $project, $sys_home;
>    $type_urls = [
>      "homepage", "download", "cvs_viewcvs", "cvs_viewcvs_homepage",
> -    "arch_viewcvs", "svn_viewcvs", "git_viewcvs", "hg_viewcvs", "bzr_viewcvs"
> +    "arch_viewcvs", "svn_viewcvs", "git_viewcvs", "git_viewcvs_homepage", 
> "hg_viewcvs", "bzr_viewcvs"

Savane follows GNU recommendations for style.  Please keep
the length of source lines to 79 characters or less,
https://www.gnu.org/prep/standards/html_node/Formatting.html

Also, these VCS URLs are in fact the same for all group types; we'd
better decommission these useless fields that make group types
unmanageable rather than add yet one more.

> -  if (!pagemenu_url_is_set ($group, "cvs_viewcvs_homepage"))
> +  if (!pagemenu_url_is_set ($group, "cvs_viewcvs_homepage") && 
> !pagemenu_url_is_set ($group, "git_viewcvs_homepage"))

The group will use no more than one VCS for its homepage; a new
field 'url_git_viewcvs_homepage' will only add unneeded complexity.

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]