[bug#66592] [PATCH v3] scripts: archive: Check compatibility of command line options.

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

Hi,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Fixes <https://issues.guix.gnu.org/66358>.
> Reported by Perry, Daniel J <dperry45@gatech.edu>.
>
> * guix/scripts/archive.scm (guix-archive)[compatible-option]: New procedure.
> and use it.
> * tests/guix-archive.sh: Test it.
> ---
>  guix/scripts/archive.scm | 18 +++++++++++++++++-
>  tests/guix-archive.sh    |  5 +++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/guix/scripts/archive.scm b/guix/scripts/archive.scm
> index 2b5a55a23f..b9cf78f981 100644
> --- a/guix/scripts/archive.scm
> +++ b/guix/scripts/archive.scm
> @@ -1,6 +1,7 @@
>  ;;; GNU Guix --- Functional package management for GNU
>  ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
>  ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
> +;;; Copyright © 2023 Simon Tournier <zimon.toutoune@gmail.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -375,8 +376,23 @@ (define-command (guix-archive . args)
>            (loop (read-line port)
>                  (cons line result)))))
>  
> +  (define* (compatible-option options #:key actions)
> +  "Return the OPTIONS if it is compatible with the list of ACTIONS."

Sorry for not mentioning this in my first review, but re-reading this
code, I think it should be named like: (check-compatibility options
actions).  There's no point making actions an optional argument since
the only point of using this procedure is when you have actions to check
compatibility with, right?

The docstring should mention that it exits with a user message when the
compatibility validation fails.

> +  (let ((some-actions (filter (lambda (action)
> +                                (assoc-ref options action))
> +                              actions)))
> +    (match some-actions
> +      ((action)
> +       options)
> +      ((action other-actions ...)
> +       (leave (G_ "the options ~{'~s' ~}are exclusive~%") some-actions))
> +      (_
> +       options))))
> +
>    (with-error-handling
> -    (let ((opts (parse-command-line args %options (list %default-options))))
> +    (let* ((opts (parse-command-line args %options (list %default-options)))
> +           (opts (compatible-option opts
> +                                    #:actions (list 'authorize 'export 'import))))
>        (parameterize ((%graft? (assoc-ref opts 'graft?)))
>          (cond ((assoc-ref opts 'generate-key)
>                 =>
> diff --git a/tests/guix-archive.sh b/tests/guix-archive.sh
> index 0866b5a4d8..08c07684ad 100644
> --- a/tests/guix-archive.sh
> +++ b/tests/guix-archive.sh
> @@ -79,4 +79,9 @@ guix archive -t < "$archive" | grep "^r /share/guile.*/boot-9\.scm"
>  
>  echo foo | guix archive --authorize && false
>  
> +# Check incompatible command-line options
> +guix archive --authorize --export --import && false
> +guix archive --export guile-bootstrap --authorize > "$archive" && false
> +guix archive --authorize --import < "$archive" && false

I'm puzzled here, because that's the approach used in commit
37dd69b44511dc73eb04bdebe8d82c9a0386338e, but I don't understand how
these checks work: if the command fails, it won't get to execute the
part after && (&& only proceeds if the left part exited with status 0),
and the test will report a failure even though this is the expected
result, no?

OK, it was explained by Eric in <https://issues.guix.gnu.org/62406#6>:

--8<---------------cut here---------------start------------->8--- 'cmd
&& false' [...] has the desired semantics under 'set -e' [...]

If 'cmd' fails, the return status is ignored by 'set -e', which
considers only the return status of a command following the final '&&'
or '||'.  And because 'cmd' failed the statement short-circuits without
executing the 'false.  Otherwise, if 'cmd' succeeds, the 'false' is
executed and the shell exits immediately.
--8<---------------cut here---------------end--------------->8---

Interesting (yet confusing) shell hack.

Would you mind sending a v4 with the above suggestion?  Then I think
we'd be good.

-- 
Thanks,
Maxim