[PATCH setup] site.cc, site.h: code cleanup

[PATCH setup] site.cc, site.h: code cleanup

[Ken Brown]

Remove site_list_type::init(), which was introduced to work around a
problem with gcc-2.95.

Add a bool member 'is_official' to the site_list_type class.  Use it
to distinguish official mirrors (listed in mirrors.lst) from
user-added sites.  This replaces the (undocumented) use of
site_list_type::servername.size() for this purpose.

When registerSavedSite is called on a URL that's already in
'all_site_list', add the version from 'all_site_list' to 'site_list'
rather than adding a temporary version that contains no information
other than the URL.

Similarly, if the user adds a site that was already in
'all_site_list', don't replace the existent version with the new one
(which contains only the URL).
---
 site.cc | 41 +++++++++++++++++------------------------
 site.h  |  9 +++++----
 2 files changed, 22 insertions(+), 28 deletions(-)

diff --git a/site.cc b/site.cc
index c33da36..c9fac12 100644
--- a/site.cc
+++ b/site.cc
@@ -141,14 +141,17 @@ SiteSetting::~SiteSetting ()
     save ();
 }
 
-void
-site_list_type::init (const string &_url, const string &_servername,
-		      const string &_area, const string &_location)
+site_list_type::site_list_type (const string &_url,
+				const string &_servername,
+				const string &_area,
+				const string &_location,
+				bool _is_official)
 {
   url = _url;
   servername = _servername;
   area = _area;
   location = _location;
+  is_official = _is_official;
 
   /* Canonicalize URL to ensure it ends with a '/' */
   if (url.at(url.length()-1) != '/')
@@ -180,14 +183,6 @@ site_list_type::init (const string &_url, const string &_servername,
   key += url;
 }
 
-site_list_type::site_list_type (const string &_url,
-				const string &_servername,
-				const string &_area,
-				const string &_location)
-{
-  init (_url, _servername, _area, _location);
-}
-
 site_list_type::site_list_type (site_list_type const &rhs)
 {
   key = rhs.key;
@@ -195,6 +190,7 @@ site_list_type::site_list_type (site_list_type const &rhs)
   servername = rhs.servername;
   area = rhs.area;
   location = rhs.location;
+  is_official = rhs.is_official;
   displayed_url = rhs.displayed_url;
 }
 
@@ -206,6 +202,7 @@ site_list_type::operator= (site_list_type const &rhs)
   servername = rhs.servername;
   area = rhs.area;
   location = rhs.location;
+  is_official = rhs.is_official;
   displayed_url = rhs.displayed_url;
   return *this;
 }
@@ -243,6 +240,7 @@ save_dialog (HWND h)
     }
 }
 
+// This is called only for lists of official mirrors.
 void
 load_site_list (SiteList& theSites, char *theString)
 {
@@ -293,7 +291,7 @@ load_site_list (SiteList& theSites, char *theString)
 	  if (!semi || !semi2 || !semi3)
 	    continue;
 
-	  site_list_type newsite (bol, semi, semi2, semi3);
+	  site_list_type newsite (bol, semi, semi2, semi3, true);
 	  SiteList::iterator i = find (theSites.begin(),
 				       theSites.end(), newsite);
 	  if (i == theSites.end())
@@ -380,7 +378,7 @@ get_site_list (HINSTANCE h, HWND owner)
 void
 SiteSetting::registerSavedSite (const char * site)
 {
-  site_list_type tempSite(site, "", "", "");
+  site_list_type tempSite(site, "", "", "", false);
   SiteList::iterator i = find (all_site_list.begin(),
 			       all_site_list.end(), tempSite);
   if (i == all_site_list.end())
@@ -399,7 +397,7 @@ SiteSetting::registerSavedSite (const char * site)
       site_list.push_back (tempSite);
     }
   else
-    site_list.push_back (tempSite);
+    site_list.push_back (*i);
 }
 
 void
@@ -510,7 +508,7 @@ int check_dropped_mirrors (HWND h)
     {
       SiteList::iterator i = find (all_site_list.begin(), all_site_list.end(),
 				   *n);
-      if (i == all_site_list.end() || !i->servername.size())
+      if (i == all_site_list.end() || !i->is_official)
 	{
 	  SiteList::iterator j = find (cached_site_list.begin(),
 				       cached_site_list.end(), *n);
@@ -540,7 +538,7 @@ void write_cache_list (io_stream *f, const SiteList& theSites)
   string s;
   for (SiteList::const_iterator n = theSites.begin ();
        n != theSites.end (); ++n)
-    if (n->servername.size())
+    if (n->is_official)
       *f << (n->url + ";" + n->servername + ";" + n->area + ";"
 	     + n->location);
 }
@@ -700,22 +698,17 @@ bool SitePage::OnMessageCmd (int id, HWND hwndctl, UINT code)
 	    std::string other_url = egetString (GetHWND (), IDC_EDIT_USER_URL);
 	    if (other_url.size())
 	    {
-	    site_list_type newsite (other_url, "", "", "");
+	    site_list_type newsite (other_url, "", "", "", false);
 	    SiteList::iterator i = find (all_site_list.begin(),
 					 all_site_list.end(), newsite);
 	    if (i == all_site_list.end())
 	      {
 		all_site_list.push_back (newsite);
 		Log (LOG_BABBLE) << "Adding site: " << other_url << endLog;
+		site_list.push_back (newsite);
 	      }
 	    else
-	      {
-		*i = newsite;
-		Log (LOG_BABBLE) << "Replacing site: " << other_url << endLog;
-	      }
-
-	    // Assume the user wants to use it and select it for him.
-	    site_list.push_back (newsite);
+	      site_list.push_back (*i);
 
 	    // Update the list box.
 	    PopulateListBox ();
diff --git a/site.h b/site.h
index 457aaee..48fd271 100644
--- a/site.h
+++ b/site.h
@@ -50,17 +50,18 @@ public:
   site_list_type () : url (), displayed_url (), key () {};
   site_list_type (const site_list_type &);
   site_list_type (const std::string& , const std::string& ,
-                  const std::string& , const std::string& );
-  /* workaround for missing placement new in gcc 2.95 */
-  void init (const std::string& , const std::string& ,
-             const std::string& , const std::string& );
+                  const std::string& , const std::string&, bool);
   ~site_list_type () {};
   site_list_type &operator= (const site_list_type &);
   std::string url;
+  // provided by mirrors.lst but not used
   std::string servername;
   std::string area;
   std::string location;
+  // did this site come from mirrors.lst?
+  bool is_official;
   std::string displayed_url;
+  // sort key
   std::string key;
   bool operator == (const site_list_type &) const;
   bool operator != (const site_list_type &) const;
-- 
2.15.0

Re: [PATCH setup] site.cc, site.h: code cleanup

[Jon Turney]

On 20/11/2017 01:26, Ken Brown wrote:
> Remove site_list_type::init(), which was introduced to work around a
> problem with gcc-2.95.

Please tell me we're not actually using a placement new for these things :S

> Add a bool member 'is_official' to the site_list_type class.  Use it
> to distinguish official mirrors (listed in mirrors.lst) from
> user-added sites.  This replaces the (undocumented) use of
> site_list_type::servername.size() for this purpose.
> 
> When registerSavedSite is called on a URL that's already in
> 'all_site_list', add the version from 'all_site_list' to 'site_list'
> rather than adding a temporary version that contains no information
> other than the URL.
> 
> Similarly, if the user adds a site that was already in
> 'all_site_list', don't replace the existent version with the new one
> (which contains only the URL).

This is a nice bit of cleanup.

My only concern is with the terminology "is_offical".

In general, this code suffers from confusion between (i) package 
repository [a set of packages], and (ii) mirrors [a set of sites 
offering the same package repository]

(See also the musings in [1]. I have a vague recollection that I 
actually started writing some code to do some of that, but that would be 
in an old CVS checkout somewhere)

We have the concept of a label for the package set ('release:' in 
setup.ini [2])

To me "is_official" gives the idea that this flag means "the package set 
at this URL is the package set with the label 'cygwin'", but actually it 
just means "this URL was read from mirrors.lst", so maybe it could be 
named something like that?

Oh, you actually have a comment to that effect...

[1] https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html
[2] https://sourceware.org/cygwin-apps/setup.ini.html

Re: [PATCH setup] site.cc, site.h: code cleanup

[Ken Brown]

On 11/22/2017 12:02 PM, Jon Turney wrote:
> On 20/11/2017 01:26, Ken Brown wrote:
>> Remove site_list_type::init(), which was introduced to work around a
>> problem with gcc-2.95.
> 
> Please tell me we're not actually using a placement new for these things :S
> 
>> Add a bool member 'is_official' to the site_list_type class.  Use it
>> to distinguish official mirrors (listed in mirrors.lst) from
>> user-added sites.  This replaces the (undocumented) use of
>> site_list_type::servername.size() for this purpose.
>>
>> When registerSavedSite is called on a URL that's already in
>> 'all_site_list', add the version from 'all_site_list' to 'site_list'
>> rather than adding a temporary version that contains no information
>> other than the URL.
>>
>> Similarly, if the user adds a site that was already in
>> 'all_site_list', don't replace the existent version with the new one
>> (which contains only the URL).
> 
> This is a nice bit of cleanup.
> 
> My only concern is with the terminology "is_offical".
> 
> In general, this code suffers from confusion between (i) package 
> repository [a set of packages], and (ii) mirrors [a set of sites 
> offering the same package repository]
> 
> (See also the musings in [1]. I have a vague recollection that I 
> actually started writing some code to do some of that, but that would be 
> in an old CVS checkout somewhere)

I agree with what you wrote there.  A lot of things would be clearer if 
we distinguished between mirrors of the Cygwin repo and general package 
sources.  Trying to use the two simultaneously is what's giving Brian a 
headache in the "make setup mirror list..." thread.

> We have the concept of a label for the package set ('release:' in 
> setup.ini [2])
> 
> To me "is_official" gives the idea that this flag means "the package set 
> at this URL is the package set with the label 'cygwin'", but actually it 
> just means "this URL was read from mirrors.lst", so maybe it could be 
> named something like that?

Yeah, maybe "from_mirrors_lst"?

Ken