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: Ayushman Tripathi
Subject: Re: [Savannah-hackers-public] Final-stage work/changes on adding the Git homepage source code web browsing option.
Date: Mon, 15 Jan 2024 20:23:56 +0530
User-agent: Mozilla Thunderbird

Hello! Ineiev.

Thank you for the feedback.

Yes, I'll remove the default value. $attr = [] I added it as I was experimenting with a new method.

I used this quote `groups` because in the latest MySQL version, groups is a reserved keyword. So, when I was trying to set up a development environment, I got so many errors related to this. MySQL :: MySQL 8.0 Reference Manual :: 9.3 Keywords and Reserved Words <https://dev.mysql.com/doc/refman/8.0/en/keywords.html> That's why I changed groups to `groups` with a quote. I fixed this issue in numerous PHP files, but I refrained from adding them to the patch to avoid redundancy. Additionally, I was uncertain about the necessity of including them since the problem was specific to my local development environment and mysql version.

Yeah, I will remove the isArtifactUrlSet; actually, I wanted some feedback before I got to the optimization and refactoring phases.

Yes, definitely. I will implement a single field to store the homepage URL for any VCS. I didn't do that before, as I was wondering if it would break some convention, codebase style, or pattern by doing that.

I'll try to make these changes very shortly.

And yes, I'm working with the Savannah volunteers to implement its backend, which is to update the sv_groups.in <https://git.savannah.gnu.org/cgit/administration/savane.git/tree/backend/accounts/sv_groups.in> and Git.pm.in, <https://git.savannah.gnu.org/cgit/administration/savane.git/tree/lib/Savane/Git.pm.in>and later to update the table groups to add the new column `homepage_vcs`.

I set an independent database field, `homepage_vcs`, which stores the value of the vcs used, which can be git, cvs, etc. With this approach, if in the future we want to add other VCS support, it will be much easier and more clear. I did this by keeping in mind that it would be much easier to write a backend Perl script using it. We can easily check if this flag is set to git and switch it accordingly.

Any further suggestions are welcome.
Thank you once again.

On 15-01-2024 00:59, Ineiev wrote:
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.


reply via email to

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