Subject: Re: [bug#75981] [PATCH (WIP) v1.5 3/4] Add 'guix fork update'.

From: Maxim Cournoyer <[email-masked]>
Date: Mon, 03 Feb 2025 01:21:38 +0900

Hi,

45mg <[email-masked]> writes:

> * guix/scripts/fork/update.scm: New file.
> * Makefile.am (MODULES): Add the new file.

Or, "Register it."

> * guix/scripts/fork.scm
> (show-help): Mention new command.
> (%sub-commands): Add new command.

OK.

[...]

> +(define %options
> +  ;; Specifications of the command-line options.
> +  (list (option '(#\h "help") #f #f
> +                (lambda args
> +                  (show-help)
> +                  (exit 0)))
> +        (option '(#\V "version") #f #f
> +                (lambda args
> +                  (show-version-and-exit "guix fork create")))
> +
> +        (option '( "fork-branch") #t #f

Extraneous space in list.

> +                (lambda (opt name arg result)
> +                  (alist-cons 'fork-branch-name arg result)))
> +        (option '(#\r "repository") #t #f
> +                (lambda (opt name arg result)
> +                  (alist-cons 'directory arg result)))))
> +
> +(define %default-options
> +  '())
> +
> +(define %usage
> +  (G_ "Usage: guix fork update [OPTIONS...]
> +Pull into this Guix fork's configured upstream branch, then apply new commits
> +onto the current branch.

I'd reword the beginning to "Pull into this Guix fork its configured
upstream branch [...]"

> +
> +  -r, --repository=DIRECTORY
> +                         Act in the Git repository in DIRECTORY

Maybe, "Work on the Git repository in DIRECTORY"

> +      --fork-branch=BRANCH
> +                         Apply new commits onto BRANCH instead of the current
> +                         branch
> +
> +  -h, --help             display this help and exit
> +  -V, --version          display version information and exit
> +"))
> +
> +(define (show-help)
> +  (display %usage)
> +  (newline)
> +  (show-bug-report-information))
> +
> +(define (missing-arguments)
> +    (leave (G_ "wrong number of arguments; \
> +required ~%")))
> +
> +
> +;;;
> +;;; Entry point.
> +;;;
> +
> +(define (guix-fork-update . args)
> +
> +  (define options
> +    (parse-command-line args %options (list %default-options)
> +                        #:build-options? #f))
> +
> +  (define (command-line-arguments lst)
> +    (reverse (filter-map (match-lambda
> +                           (('argument . arg) arg)
> +                           (_ #f))
> +                         lst)))
> +
> +  (define-syntax invoke-git
> +    (lambda (x)
> +      (syntax-case x ()
> +        ((_ args ...)
> +         #`(invoke "git" "-C" #,(datum->syntax x 'directory) args ...)))))
> +
> +  (define-syntax invoke-git/stdout
> +    (lambda (x)
> +      (syntax-case x ()
> +        ((_ args ...)
> +         #`(string-trim-right
> +            (invoke/stdout "git" "-C" #,(datum->syntax x 'directory) args ...))))))
> +
> +  (with-error-handling
> +    (let* ((directory (or (assoc-ref options 'directory) "."))
> +           (current-branch-name (invoke-git/stdout
> +                                 "branch"
> +                                 "--show-current"))
> +           (current-head-location (invoke-git/stdout
> +                                   "rev-parse"
> +                                   "HEAD"))
> +           (fork-branch-name (or (assoc-ref options 'fork-branch-name)
> +                                 (if (string= current-branch-name "")
> +                                     (leave (G_ "no current branch and --fork-branch not given"))

Too wide.  You can always break a string with a \ escape.

> +                                     current-branch-name)))
> +
> +           (repository (repository-open directory))
> +           (upstream-branch-name introduction-commit introduction-signer
> +                                 (if (fork-configured? repository)
> +                                     (fork-configured-introduction
> +                                      (repository-open directory))
> +                                     (leave (G_ "fork not fully configured.
> +(Did you remember to run `guix fork authenticate` first?)%~"))))

'leave' prints errors, which conventionally should be brief and not
complete sentence. I think you could get a nicer result by using a
compound condition combining a &message and &fix-hint conditions; which
the `with-error-handling' handler will correcly format with colors and
all.

> +           (upstream-branch-commit
> +            (invoke-git/stdout "rev-parse" upstream-branch-name))
> +           (new-upstream-branch-commit "")
> +           (config (repository-config repository))
> +           (signing-key
> +            (or
> +             (catch 'git-error
> +               (lambda ()
> +                 (config-entry-value
> +                  (config-get-entry config "user.signingkey")))
> +               (const #f))
> +             (begin
> +               (info (G_ "user.signingkey not set for this repository.~%"))
> +               (info (G_ "Will attempt to sign commits with fork introduction key.~%"))

Max width busted :-)

> +               introduction-signer))))
> +
> +      (info (G_ "Pulling into '~a'...~%") upstream-branch-name)
> +      (invoke-git "switch" upstream-branch-name)
> +      (invoke-git "pull")
> +      (set! new-upstream-branch-commit
> +            (invoke-git/stdout "rev-parse" upstream-branch-name))

I think you can use (define new-upstream-branch-commit ...) and avoid
its let-bound variable (set to the empty string).

> +
> +      (info (G_ "Rebasing commits from '~a' to '~a' onto fork branch '~a'...~%")
> +            upstream-branch-commit
> +            new-upstream-branch-commit
> +            fork-branch-name)
> +      (invoke-git "rebase" "--rebase-merges"
> +                  (string-append "--gpg-sign=" signing-key)
> +                  fork-branch-name new-upstream-branch-commit)
> +
> +      (info (G_ "Resetting fork branch '~a' to latest rebased commit...~%")
> +            fork-branch-name)
> +      (invoke-git "branch" "--force" fork-branch-name "HEAD")
> +
> +      (invoke-git "checkout" (or current-branch-name current-head-location))
> +
> +      (info (G_ "Successfully updated Guix fork in ~a~%")
> +            directory))))

Phew!  LGTM.  So the idea is to avoid rewriting the fork's introductory
commit and instead rewriting (rebasing) the Guix upstream commits on
top, which will resign them with the fork's authorized key, IIUC?

That's clever, but personally I much prefer to keep any work I've done
*rebased* on upstream so they are easily (re-)submitted, and it's clear
what extra work my fork has.  Seems like a good way for "forks" to hide
potentially bad commits hidden under thousands of rust commits,
obscuring them.

I think Liliana had that remark as well in the associated issue.

-- 
Thanks,
Maxim