Display warning when flagging VCS packages

Message ID 20190525170733.25076-1-lfleischer@archlinux.org
State New
Headers show
Series
  • Display warning when flagging VCS packages
Related show

Commit Message

Lukas Fleischer May 25, 2019, 5:07 p.m. UTC
VCS packages should not be flagged out-of-date when the package version
does not match the most recent commit.

Implements FS#62733.

Signed-off-by: Lukas Fleischer <lfleischer@archlinux.org>
---
 web/html/css/aurweb.css      |  4 ++++
 web/html/pkgflag.php         |  5 +++++
 web/lib/pkgbasefuncs.inc.php | 21 +++++++++++++++++++++
 3 files changed, 30 insertions(+)

Comments

Bruno Pagani May 25, 2019, 5:46 p.m. UTC | #1
Le 25/05/2019 à 19:07, Lukas Fleischer a écrit :
> +		This is a VCS package. Please do <strong>not</strong> flag this package out-of-date when the package version in the AUR does not match the most recent commit.

Maybe we should also hint when it is appropriated to do so? E.g. missing
new dependency or old one to be removed, moved upstream…

> +/**
> + * Determine whether a package base is (or contains a) VCS package
> + *
> + * @param int $base_id The ID of the package base
> + *
> + * @return bool True if the package base is/contains a VCS package
> + */
> +function pkgbase_is_vcs($base_id) {
> +	$suffixes = array("-cvs", "-svn", "-git", "-hg", "-bzr", "-darcs");

What about false positives and missing items like -nightly’s? I think it
would be a good time to implement FS#56602, auto-seed the value
depending on your above list and let maintainers override this.

Regards,
Bruno
Lukas Fleischer May 25, 2019, 10:05 p.m. UTC | #2
On Sat, 25 May 2019 at 13:46:45, Bruno Pagani wrote:
> Le 25/05/2019 =C3=A0 19:07, Lukas Fleischer a =C3=A9crit=C2=A0:
> > +             This is a VCS package. Please do <strong>not</strong> fla=
g this package out-of-date when the package version in the AUR does not mat=
ch the most recent commit.
>=20
> Maybe we should also hint when it is appropriated to do so? E.g. missing
> new dependency or old one to be removed, moved upstream=E2=80=A6

There already is a message stating that package flagging should not be
used to report such bugs. Is it not obvious/precise enough?

>=20
> > +/**
> > + * Determine whether a package base is (or contains a) VCS package
> > + *
> > + * @param int $base_id The ID of the package base
> > + *
> > + * @return bool True if the package base is/contains a VCS package
> > + */
> > +function pkgbase_is_vcs($base_id) {
> > +     $suffixes =3D array("-cvs", "-svn", "-git", "-hg", "-bzr", "-darc=
s");
>=20
> What about false positives and missing items like -nightly=E2=80=99s? I t=
hink it
> would be a good time to implement FS#56602, auto-seed the value
> depending on your above list and let maintainers override this.

Yes, there are false positives and false negatives. That is why we only
display a warning and do not automatically disable the feature for VCS
packages. Read the comments in FS#62733 for details.
Bruno Pagani May 25, 2019, 10:33 p.m. UTC | #3
Le 26/05/2019 à 00:04, Lukas Fleischer a écrit :
> On Sat, 25 May 2019 at 13:46:45, Bruno Pagani wrote:
>> Le 25/05/2019 à 19:07, Lukas Fleischer a écrit :
>>> +             This is a VCS package. Please do <strong>not</strong> flag this package out-of-date when the package version in the AUR does not match the most recent commit.
>> Maybe we should also hint when it is appropriated to do so? E.g. missing
>> new dependency or old one to be removed, moved upstream…
> There already is a message stating that package flagging should not be
> used to report such bugs. Is it not obvious/precise enough?

Well I think it should instead be used for such bugs in the case of VCS
packages, because those are the only cases where they can be OOD, and in
contrary to normal packages those are valid OOD reasons. And that is the
case for the linked package in the FS ticket. But I acknowledge this is
not what we say currently, though I would use the opportunity of that
addition to change the guidelines regarding this. And then I’m in favour
of saying so in the message:

“This is a VCS package. Please do not flag it out-of-date if the package
version in the AUR does not match the most recent commit. Flagging this
package should only be done if the sources moved or changes in the
PKGBUILD are required because of recent upstream changes.”

>>> +/**
>>> + * Determine whether a package base is (or contains a) VCS package
>>> + *
>>> + * @param int $base_id The ID of the package base
>>> + *
>>> + * @return bool True if the package base is/contains a VCS package
>>> + */
>>> +function pkgbase_is_vcs($base_id) {
>>> +     $suffixes = array("-cvs", "-svn", "-git", "-hg", "-bzr", "-darcs");
>> What about false positives and missing items like -nightly’s? I think it
>> would be a good time to implement FS#56602, auto-seed the value
>> depending on your above list and let maintainers override this.
> Yes, there are false positives and false negatives. That is why we only
> display a warning and do not automatically disable the feature for VCS
> packages. Read the comments in FS#62733 for details.

All I’ve read was the same thing as before regarding the impossibility
to correctly detect all VCS packages and just them, but I did not see
why manual override wouldn’t be an option. ;) Regarding false positives,
without override possibility they will be misleading to users, so I
don’t agree on “it’s OK because we are not plainly disabling the
feature”. Also for me the strongest reason to not disable the feature
for VCS packages is rather because it is still useful even for those, as
stated by Eli. :)

Regards,
Bruno
Lukas Fleischer May 25, 2019, 10:52 p.m. UTC | #4
On Sat, 25 May 2019 at 18:33:55, Bruno Pagani wrote:
> Well I think it should instead be used for such bugs in the case of VCS
> packages, because those are the only cases where they can be OOD, and in
> contrary to normal packages those are valid OOD reasons. And that is the
> case for the linked package in the FS ticket. But I acknowledge this is
> not what we say currently, though I would use the opportunity of that
> addition to change the guidelines regarding this. And then I’m in favour
> of saying so in the message:
> 
> “This is a VCS package. Please do not flag it out-of-date if the package
> version in the AUR does not match the most recent commit. Flagging this
> package should only be done if the sources moved or changes in the
> PKGBUILD are required because of recent upstream changes.”

That message sounds good to me.

> 
> >>> +/**
> >>> + * Determine whether a package base is (or contains a) VCS package
> >>> + *
> >>> + * @param int $base_id The ID of the package base
> >>> + *
> >>> + * @return bool True if the package base is/contains a VCS package
> >>> + */
> >>> +function pkgbase_is_vcs($base_id) {
> >>> +     $suffixes = array("-cvs", "-svn", "-git", "-hg", "-bzr", "-darcs");
> >> What about false positives and missing items like -nightly’s? I think it
> >> would be a good time to implement FS#56602, auto-seed the value
> >> depending on your above list and let maintainers override this.
> > Yes, there are false positives and false negatives. That is why we only
> > display a warning and do not automatically disable the feature for VCS
> > packages. Read the comments in FS#62733 for details.
> 
> All I’ve read was the same thing as before regarding the impossibility
> to correctly detect all VCS packages and just them, but I did not see
> why manual override wouldn’t be an option. ;) Regarding false positives,
> without override possibility they will be misleading to users, so I
> don’t agree on “it’s OK because we are not plainly disabling the
> feature”. Also for me the strongest reason to not disable the feature
> for VCS packages is rather because it is still useful even for those, as
> stated by Eli. :)

We could tune the message and say "This seems to be a VCS package."

I would prefer to keep this very simple. That message is just for
convenience and not really an essential part of the AUR.
Bruno Pagani May 25, 2019, 10:56 p.m. UTC | #5
Le 26/05/2019 à 00:52, Lukas Fleischer a écrit :
>>>>> +/**
>>>>> + * Determine whether a package base is (or contains a) VCS package
>>>>> + *
>>>>> + * @param int $base_id The ID of the package base
>>>>> + *
>>>>> + * @return bool True if the package base is/contains a VCS package
>>>>> + */
>>>>> +function pkgbase_is_vcs($base_id) {
>>>>> +     $suffixes = array("-cvs", "-svn", "-git", "-hg", "-bzr", "-darcs");
>>>> What about false positives and missing items like -nightly’s? I think it
>>>> would be a good time to implement FS#56602, auto-seed the value
>>>> depending on your above list and let maintainers override this.
>>> Yes, there are false positives and false negatives. That is why we only
>>> display a warning and do not automatically disable the feature for VCS
>>> packages. Read the comments in FS#62733 for details.
>> All I’ve read was the same thing as before regarding the impossibility
>> to correctly detect all VCS packages and just them, but I did not see
>> why manual override wouldn’t be an option. ;) Regarding false positives,
>> without override possibility they will be misleading to users, so I
>> don’t agree on “it’s OK because we are not plainly disabling the
>> feature”. Also for me the strongest reason to not disable the feature
>> for VCS packages is rather because it is still useful even for those, as
>> stated by Eli. :)
> We could tune the message and say "This seems to be a VCS package."

That could work, yes. Let’s do this. ;)

> I would prefer to keep this very simple. That message is just for
> convenience and not really an essential part of the AUR.

I definitively understand that. :)
Eli Schwartz May 26, 2019, 2:14 a.m. UTC | #6
On 5/25/19 6:52 PM, Lukas Fleischer wrote:
> On Sat, 25 May 2019 at 18:33:55, Bruno Pagani wrote:
>> Well I think it should instead be used for such bugs in the case of VCS
>> packages, because those are the only cases where they can be OOD, and in
>> contrary to normal packages those are valid OOD reasons. And that is the
>> case for the linked package in the FS ticket. But I acknowledge this is
>> not what we say currently, though I would use the opportunity of that
>> addition to change the guidelines regarding this. And then I’m in favour
>> of saying so in the message:
>>
>> “This is a VCS package. Please do not flag it out-of-date if the package
>> version in the AUR does not match the most recent commit. Flagging this
>> package should only be done if the sources moved or changes in the
>> PKGBUILD are required because of recent upstream changes.”
> 
> That message sounds good to me.

Me too!

>>>>> +/**
>>>>> + * Determine whether a package base is (or contains a) VCS package
>>>>> + *
>>>>> + * @param int $base_id The ID of the package base
>>>>> + *
>>>>> + * @return bool True if the package base is/contains a VCS package
>>>>> + */
>>>>> +function pkgbase_is_vcs($base_id) {
>>>>> +     $suffixes = array("-cvs", "-svn", "-git", "-hg", "-bzr", "-darcs");
>>>> What about false positives and missing items like -nightly’s? I think it
>>>> would be a good time to implement FS#56602, auto-seed the value
>>>> depending on your above list and let maintainers override this.
>>> Yes, there are false positives and false negatives. That is why we only
>>> display a warning and do not automatically disable the feature for VCS
>>> packages. Read the comments in FS#62733 for details.
>>
>> All I’ve read was the same thing as before regarding the impossibility
>> to correctly detect all VCS packages and just them, but I did not see
>> why manual override wouldn’t be an option. ;) Regarding false positives,
>> without override possibility they will be misleading to users, so I
>> don’t agree on “it’s OK because we are not plainly disabling the
>> feature”. Also for me the strongest reason to not disable the feature
>> for VCS packages is rather because it is still useful even for those, as
>> stated by Eli. :)
> 
> We could tune the message and say "This seems to be a VCS package."
> 
> I would prefer to keep this very simple. That message is just for
> convenience and not really an essential part of the AUR.

Agreed on all points. The revised patch seems good to me.

Patch

diff --git a/web/html/css/aurweb.css b/web/html/css/aurweb.css
index ef37bf5..81bf9ab 100644
--- a/web/html/css/aurweb.css
+++ b/web/html/css/aurweb.css
@@ -195,3 +195,7 @@  label.confirmation,
 .comments .more {
 	font-weight: normal;
 }
+
+.error {
+	color: red;
+}
diff --git a/web/html/pkgflag.php b/web/html/pkgflag.php
index 61346b9..2fba6bd 100644
--- a/web/html/pkgflag.php
+++ b/web/html/pkgflag.php
@@ -50,6 +50,11 @@  if (has_credential(CRED_PKGBASE_FLAG)): ?>
 		<li><?= htmlspecialchars($pkgname) ?></li>
 		<?php endforeach; ?>
 	</ul>
+	<?php if (pkgbase_is_vcs($base_id)): ?>
+	<p class="error">
+		This is a VCS package. Please do <strong>not</strong> flag this package out-of-date when the package version in the AUR does not match the most recent commit.
+	</p>
+	<?php endif; ?>
 	<p>
 		<?= __('Please do %snot%s use this form to report bugs. Use the package comments instead.',
 			'<strong>', '</strong>'); ?>
diff --git a/web/lib/pkgbasefuncs.inc.php b/web/lib/pkgbasefuncs.inc.php
index 1df21a2..a492589 100644
--- a/web/lib/pkgbasefuncs.inc.php
+++ b/web/lib/pkgbasefuncs.inc.php
@@ -367,6 +367,27 @@  function pkgbase_get_pkgnames($base_id) {
 	return $result->fetchAll(PDO::FETCH_COLUMN, 0);
 }
 
+/**
+ * Determine whether a package base is (or contains a) VCS package
+ *
+ * @param int $base_id The ID of the package base
+ *
+ * @return bool True if the package base is/contains a VCS package
+ */
+function pkgbase_is_vcs($base_id) {
+	$suffixes = array("-cvs", "-svn", "-git", "-hg", "-bzr", "-darcs");
+	$haystack = pkgbase_get_pkgnames($base_id);
+	array_push($haystack, pkgbase_name_from_id($base_id));
+	foreach ($haystack as $pkgname) {
+		foreach ($suffixes as $suffix) {
+			if (substr_compare($pkgname, $suffix, -strlen($suffix)) === 0) {
+				return true;
+			}
+		}
+	}
+	return false;
+}
+
 /**
  * Delete all packages belonging to a package base
  *