GNU bug report logs - #66436
[PATCH] doc: Add some guidelines for reviewing.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 66436 in the body.
You can then email your comments to 66436 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: guix-patches <at> gnu.org,
	maxim.cournoyer <at> gmail.com
Subject: [PATCH] doc: Add some guidelines for reviewing.
Date: Tue, 10 Oct 2023 08:54:27 -0400

* doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
section.
* doc/contributing.texi (Debbugs Usertags): Expound with Emacs Debbugs
information and document the 'reviewed-looks-good' usertag.

Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
---
 doc/contributing.texi | 53 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 864190b119..b09e9cc299 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -29,6 +29,7 @@ Contributing
 * Submitting Patches::          Share your work.
 * Tracking Bugs and Changes::   Keeping it all organized.
 * Commit Access::               Pushing to the official repository.
+* Reviewing the Work of Others::  Some guidelines for sharing reviews.
 * Updating the Guix Package::   Updating the Guix package definition.
 * Writing Documentation::       Improving documentation in GNU Guix.
 * Translating Guix::            Make Guix speak your native language.
@@ -605,7 +606,7 @@ Packaging Guidelines
 * Version Numbers::             When the name is not enough.
 * Synopses and Descriptions::   Helping users find the right package.
 * Snippets versus Phases::      Whether to use a snippet, or a build phase.
-* Cyclic Module Dependencies::   Going full circle.
+* Cyclic Module Dependencies::  Going full circle.
 * Emacs Packages::              Your Elisp fix.
 * Python Modules::              A touch of British comedy.
 * Perl Modules::                Little pearls.
@@ -1972,7 +1973,12 @@ Debbugs Usertags
 tag any bug with an arbitrary label.  Bugs can be searched by usertag,
 so this is a handy way to organize bugs <at> footnote{The list of usertags is
 public information, and anyone can modify any user's list of usertags,
-so keep that in mind if you choose to use this feature.}.
+so keep that in mind if you choose to use this feature.}.  If you use
+Emacs Debbugs, the entry-point to consult existing usertags is the
+@samp{C-u M-x debbugs-gnu-usertags} procedure.  To set a usertag, press
+@samp{C} while consulting a bug within the *Guix-Patches* buffer opened
+with @samp{C-u M-x debbugs-gnu-bugs} buffer, then select @code{usertag}
+and follow the instructions.
 
 For example, to view all the bug reports (or patches, in the case of
 @code{guix-patches}) tagged with the usertag @code{powerpc64le-linux}
@@ -1985,9 +1991,9 @@ Debbugs Usertags
 to interact with Debbugs.
 
 In Guix, we are experimenting with usertags to keep track of
-architecture-specific issues.  To facilitate collaboration, all our
-usertags are associated with the single user @code{guix}.  The following
-usertags currently exist for that user:
+architecture-specific issues, as well as reviewed ones.  To facilitate
+collaboration, all our usertags are associated with the single user
+@code{guix}.  The following usertags currently exist for that user:
 
 @table @code
 
@@ -2005,6 +2011,9 @@ Debbugs Usertags
 appropriate to assign this usertag to a bug report for a package that
 fails to build reproducibly.
 
+@item reviewed-looks-good
+You have reviewed the series and it looks good to you (LGTM).
+
 @end table
 
 If you're a committer and you want to add a usertag, just start using it
@@ -2237,6 +2246,40 @@ Commit Access
 you're welcome to use your expertise and commit rights to help other
 contributors, too!
 
+@node Reviewing the Work of Others
+@section Reviewing the Work of Others
+
+Perhaps the biggest action you can do to help GNU Guix grow as a project
+is to review the work contributed by others.  You do not need to be a
+committer to do so; applying, building, linting and running other
+people's series and sharing your comments about your experience will
+give some confidence to committers that should result in the proposed
+change being merged faster.
+
+@cindex LGTM, Looks Good To Me
+@cindex reviewing, guidelines
+Review comments should be unambiguous; be as clear and explicit as you
+can about what you think should be changed, ensuring the author can take
+action on it.  The following well understood/codified @acronym{LGTM,
+Looks Good To Me} phrases should be used to sign off as a reviewer,
+meaning you have reviewed the change and that it looks good to you:
+
+@enumerate
+@item
+If the @emph{whole} series (containing multiple commits) looks good to
+you, reply with a @samp{This series LGTM!} to the cover page if it has
+one, or to the last patch of the series otherwise.
+
+@item
+If you instead want to mark a @emph{single commit} as reviewed (but not
+the whole series), simply reply with @samp{LGTM!} to that commit
+message.
+@end enumerate
+
+If you are not a committer, you can help others find a @emph{series} you
+have reviewed more easily by adding a @code{reviewed-looks-good} usertag
+for the @code{guix} user (@pxref{Debbugs Usertags}).
+
 @node Updating the Guix Package
 @section Updating the Guix Package
 

base-commit: 619ff2fa1d5b60b5fe33216ca2d6219c04a5515b
-- 
2.41.0

Message #8 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org
Subject: Re: [bug#66436] [PATCH] doc: Add some guidelines for reviewing.
Date: Tue, 10 Oct 2023 15:29:37 +0200

Hi,

This looks great to me!

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> +@cindex LGTM, Looks Good To Me
> +@cindex reviewing, guidelines
> +Review comments should be unambiguous; be as clear and explicit as you
> +can about what you think should be changed, ensuring the author can take
> +action on it.

I’d add a few guidelines here, and perhaps we can make it a bullet
list:

  1. Be clear and explicit about changes you are suggesting, ensuring
     the author can take action on it.  In particular, it is a good idea
     to explicitly ask for new revisions when you want it.

  2. Remain focused: do not change the scope of the work being reviewed.
     For example, if the contribution touches a code that follows a
     pattern deemed unwieldy, it would be unfair to ask the submitter to
     fix all occurrences of that pattern in the code; to put it simply,
     if an problem unrelated to the patch at hand was already there, do
     not ask the submitter to fix it.

  3. Ensure progress.  As they respond to review, submitters may submit
     new revisions of their changes; avoid requesting changes that you
     did not request in the previous round of comments.  Overall, the
     submitter should get a clear sense of progress; the number of items
     open for discussion should clearly decrease over time.

  4. Review is a discussion.  The submitter's and reviewer's views on
     how to achieve a particular change may not always be aligned.  As a
     reviewer, try hard to explain the rationale for suggestions you
     make, and to understand and take into account the submitter's
     motivation for doing things in a certain way.

  5. Aim for finalization.  Reviewing code is time-consuming.  Your goal
     as a reviewer is to put the process on a clear path towards
     integration, possibly with agreed-upon changes, or rejection, with
     a clear and mutually-understood reasoning.  Avoid leaving the
     review process in a lingering state with no clear way out.

I just made it up but I’m sure there are good review guidelines out
there that we could use as an example.

My 2¢,
Ludo’.

Message #11 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 66436 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH v2] doc: Add some guidelines for reviewing.
Date: Tue, 10 Oct 2023 20:24:05 -0400

* doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
section.
* doc/contributing.texi (Debbugs Usertags): Expound with Emacs Debbugs
information and document the 'reviewed-looks-good' usertag.

Series-version: 2
Series-cc: ludo <at> gnu.org
Series-to: 66436 <at> debbugs.gnu.org
Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
Co-authored-by: Ludovic Courtès <ludo <at> gnu.org>
---
v2: integrate guidelines suggested by Ludovic

 doc/contributing.texi | 93 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 88 insertions(+), 5 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 864190b119..b8a0839c7f 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -29,6 +29,7 @@ Contributing
 * Submitting Patches::          Share your work.
 * Tracking Bugs and Changes::   Keeping it all organized.
 * Commit Access::               Pushing to the official repository.
+* Reviewing the Work of Others::  Some guidelines for sharing reviews.
 * Updating the Guix Package::   Updating the Guix package definition.
 * Writing Documentation::       Improving documentation in GNU Guix.
 * Translating Guix::            Make Guix speak your native language.
@@ -605,7 +606,7 @@ Packaging Guidelines
 * Version Numbers::             When the name is not enough.
 * Synopses and Descriptions::   Helping users find the right package.
 * Snippets versus Phases::      Whether to use a snippet, or a build phase.
-* Cyclic Module Dependencies::   Going full circle.
+* Cyclic Module Dependencies::  Going full circle.
 * Emacs Packages::              Your Elisp fix.
 * Python Modules::              A touch of British comedy.
 * Perl Modules::                Little pearls.
@@ -1972,7 +1973,12 @@ Debbugs Usertags
 tag any bug with an arbitrary label.  Bugs can be searched by usertag,
 so this is a handy way to organize bugs <at> footnote{The list of usertags is
 public information, and anyone can modify any user's list of usertags,
-so keep that in mind if you choose to use this feature.}.
+so keep that in mind if you choose to use this feature.}.  If you use
+Emacs Debbugs, the entry-point to consult existing usertags is the
+@samp{C-u M-x debbugs-gnu-usertags} procedure.  To set a usertag, press
+@samp{C} while consulting a bug within the *Guix-Patches* buffer opened
+with @samp{C-u M-x debbugs-gnu-bugs} buffer, then select @code{usertag}
+and follow the instructions.
 
 For example, to view all the bug reports (or patches, in the case of
 @code{guix-patches}) tagged with the usertag @code{powerpc64le-linux}
@@ -1985,9 +1991,9 @@ Debbugs Usertags
 to interact with Debbugs.
 
 In Guix, we are experimenting with usertags to keep track of
-architecture-specific issues.  To facilitate collaboration, all our
-usertags are associated with the single user @code{guix}.  The following
-usertags currently exist for that user:
+architecture-specific issues, as well as reviewed ones.  To facilitate
+collaboration, all our usertags are associated with the single user
+@code{guix}.  The following usertags currently exist for that user:
 
 @table @code
 
@@ -2005,6 +2011,9 @@ Debbugs Usertags
 appropriate to assign this usertag to a bug report for a package that
 fails to build reproducibly.
 
+@item reviewed-looks-good
+You have reviewed the series and it looks good to you (LGTM).
+
 @end table
 
 If you're a committer and you want to add a usertag, just start using it
@@ -2237,6 +2246,80 @@ Commit Access
 you're welcome to use your expertise and commit rights to help other
 contributors, too!
 
+@node Reviewing the Work of Others
+@section Reviewing the Work of Others
+
+Perhaps the biggest action you can do to help GNU Guix grow as a project
+is to review the work contributed by others.  You do not need to be a
+committer to do so; applying, reading the source, building, linting and
+running other people's series and sharing your comments about your
+experience will give some confidence to committers, and should result in
+the proposed change being merged faster.
+
+@cindex reviewing, guidelines
+Review comments should be unambiguous; be as clear and explicit as you
+can about what you think should be changed, ensuring the author can take
+action on it.  Please try to keep the following guidelines in mind
+during review:
+
+@enumerate
+@item
+@emph{Be clear and explicit about changes you are suggesting}, ensuring
+the author can take action on it.  In particular, it is a good idea to
+explicitly ask for new revisions when you want it.
+
+@item
+@emph{Remain focused: do not change the scope of the work being
+reviewed.}  For example, if the contribution touches code that follows a
+pattern deemed unwieldy, it would be unfair to ask the submitter to fix
+all occurrences of that pattern in the code; to put it simply, if a
+problem unrelated to the patch at hand was already there, do not ask the
+submitter to fix it.
+
+@item
+@emph{Ensure progress.}  As they respond to review, submitters may
+submit new revisions of their changes; avoid requesting changes that you
+did not request in the previous round of comments.  Overall, the
+submitter should get a clear sense of progress; the number of items open
+for discussion should clearly decrease over time.
+
+@item
+@emph{Review is a discussion.}  The submitter's and reviewer's views on
+how to achieve a particular change may not always be aligned.  As a
+reviewer, try hard to explain the rationale for suggestions you make,
+and to understand and take into account the submitter's motivation for
+doing things in a certain way.
+
+@item
+@emph{Aim for finalization.}  Reviewing code is time-consuming.  Your
+goal as a reviewer is to put the process on a clear path towards
+integration, possibly with agreed-upon changes, or rejection, with a
+clear and mutually-understood reasoning.  Avoid leaving the review
+process in a lingering state with no clear way out.
+@end enumerate
+
+@cindex LGTM, Looks Good To Me
+When you deem the proposed change adequate and ready for inclusion
+within Guix, the following well understood/codified @acronym{LGTM, Looks
+Good To Me} phrases should be used to sign off as a reviewer, meaning
+you have reviewed the change and that it looks good to you:
+
+@itemize
+@item
+If the @emph{whole} series (containing multiple commits) looks good to
+you, reply with a @samp{This series LGTM!} to the cover page if it has
+one, or to the last patch of the series otherwise.
+
+@item
+If you instead want to mark a @emph{single commit} as reviewed (but not
+the whole series), simply reply with @samp{LGTM!} to that commit
+message.
+@end itemize
+
+If you are not a committer, you can help others find a @emph{series} you
+have reviewed more easily by adding a @code{reviewed-looks-good} usertag
+for the @code{guix} user (@pxref{Debbugs Usertags}).
+
 @node Updating the Guix Package
 @section Updating the Guix Package
 

base-commit: 5a8f9d32f5196263bc50c2059bac4c4226784a59
-- 
2.41.0

Message #14 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 66436 <at> debbugs.gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH 0/2] Add support for Git Large File Storage (LFS).
Date: Wed, 11 Oct 2023 22:48:40 -0400

While updating orcus to its latest version (0.19.0), I stumbled on a test a
failure, which ended up being caused by the lack of test files in the
repository checkout.  These files are stored using the LFS extension in the
repo; when not using LFS, empty text stubs with some metadata are left in
their place.

I've opted to keep the dependency on git-lfs optional for now for the daemon.
The git in-band downloader will only add the git-lfs dependency when the
git-reference object has its lfs? field set to true.

Maxim Cournoyer (2):
  gnu: git-lfs: Patch /bin/sh references.
  git-download: Add support for Git Large File Storage (LFS).

 configure.ac                      | 10 ++++++
 doc/guix.texi                     |  9 +++++
 gnu/packages/version-control.scm  |  5 +++
 guix/build/git.scm                | 18 +++++++---
 guix/config.scm.in                |  4 +++
 guix/git-download.scm             | 58 ++++++++++++++++++++-----------
 guix/scripts/perform-download.scm |  3 ++
 guix/self.scm                     | 10 +++++-
 8 files changed, 92 insertions(+), 25 deletions(-)


base-commit: d6d706a58b8159748d3a46fa97cae18850487c8a
--
2.41.0

Message #17 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 66436 <at> debbugs.gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH 1/2] gnu: git-lfs: Patch /bin/sh references.
Date: Wed, 11 Oct 2023 22:48:41 -0400

* gnu/packages/version-control.scm (git-lfs)
[arguments]: Add patch-/bin/sh phase.

Change-Id: I2d455e683e4f6e30cd32f5b1fdaccac71616826c
---
 gnu/packages/version-control.scm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gnu/packages/version-control.scm b/gnu/packages/version-control.scm
index d9c53af71c..04b52f2a85 100644
--- a/gnu/packages/version-control.scm
+++ b/gnu/packages/version-control.scm
@@ -3175,6 +3175,11 @@ (define-public git-lfs
       #:install-source? #f
       #:phases
       #~(modify-phases %standard-phases
+          (add-after 'unpack 'patch-/bin/sh
+            (lambda* (#:key inputs #:allow-other-keys)
+              (substitute* "src/github.com/git-lfs/git-lfs/lfs/hook.go"
+                (("/bin/sh")
+                 (search-input-file inputs "bin/sh")))))
           (add-after 'unpack 'fix-embed-x-net
             (lambda _
               (delete-file-recursively "src/golang.org/x/net/publicsuffix/data")
-- 
2.41.0

Message #20 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 66436 <at> debbugs.gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH 2/2] git-download: Add support for Git Large File Storage
 (LFS).
Date: Wed, 11 Oct 2023 22:48:42 -0400

* guix/git-download.scm (<git-reference>) [lfs?]: New field.
(git-fetch/in-band): New #:git-lfs argument.
<inputs>: Remove labels.  Conditionally add git-lfs.
<build>: Read "git lfs?" environment
variable and pass its value to the #:lfs? argument of git-fetch-with-fallback.
Use INPUTS directly; update comment.
<gexp->derivation>: Add "git lfs?" to #:env-vars.
(git-fetch/built-in): Add "lfs?" to #:env-vars.
* guix/build/git.scm (git-fetch) [lfs?]: New argument, doc and setup code.
(git-fetch-with-fallback) [lfs?]: New argument.  Pass it to git-fetch.
* guix/scripts/perform-download.scm (perform-git-download): Honor the 'lfs?'
environment variable.
* doc/guix.texi (origin Reference) <git-reference>: Document the new 'lfs?'
field.
 (Requirements): Mention the optional 'git-lfs' dependency.
* configure.ac: Add a check for the 'git-lfs' command.
* guix/config.scm.in (%git-lfs): New variable.
* guix/self.scm (%packages): Add git-lfs.
(compiled-guix): Add git-lfs to guix-config.
(make-config.scm): New #:git-lfs argument.

Change-Id: I5b233b8642a7bdb8737b9d9b740e7254a89ccb25
---
 configure.ac                      | 10 ++++++
 doc/guix.texi                     |  9 +++++
 guix/build/git.scm                | 18 +++++++---
 guix/config.scm.in                |  4 +++
 guix/git-download.scm             | 58 ++++++++++++++++++++-----------
 guix/scripts/perform-download.scm |  3 ++
 guix/self.scm                     | 10 +++++-
 7 files changed, 87 insertions(+), 25 deletions(-)

diff --git a/configure.ac b/configure.ac
index d817f620cf..5342c0f80e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -208,6 +208,16 @@ if test "x$GIT" = "x"; then
 fi
 AC_SUBST([GIT])
 
+dnl Git Large File Storage is an optional dependency for the
+dnl "builtin:git-download" derivation builder.
+AC_PATH_PROG([GIT_LFS], [git-lfs])
+if test "x$GIT_LFS" = "x"; then
+  AC_MSG_WARN([Git Large File Storage (git-lfs) is missing;
+  The builtin:git-download derivation builder of the Guix daemon will
+  not be able to use it.])
+fi
+AC_SUBST([GIT_LFS])
+
 LIBGCRYPT_LIBDIR="no"
 LIBGCRYPT_PREFIX="no"
 
diff --git a/doc/guix.texi b/doc/guix.texi
index dc16ec1d15..89faaa7b90 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -1020,6 +1020,11 @@ Requirements
 The following dependencies are optional:
 
 @itemize
+@item
+The daemon will be able to fetch from Git repositories using the
+@uref{https://git-lfs.com/, Git Large File Storage} extension when the
+@command{git-lfs} command is available.
+
 @item
 @c Note: We need at least 0.13.0 for #:nodelay.
 Support for build offloading (@pxref{Daemon Offload Setup}) and
@@ -8499,6 +8504,10 @@ origin Reference
 @command{git describe} style identifier such as
 @code{v1.0.1-10-g58d7909c97}.
 
+@item @code{lfs?} (default: @code{#f})
+This Boolean indicates whether to fetch Git large file storage (LFS)
+files.
+
 @item @code{recursive?} (default: @code{#f})
 This Boolean indicates whether to recursively fetch Git sub-modules.
 @end table
diff --git a/guix/build/git.scm b/guix/build/git.scm
index 0ff263c81b..82c31fabf1 100644
--- a/guix/build/git.scm
+++ b/guix/build/git.scm
@@ -33,10 +33,13 @@ (define-module (guix build git)
 ;;; Code:
 
 (define* (git-fetch url commit directory
-                    #:key (git-command "git") recursive?)
+                    #:key (git-command "git")
+                    lfs? recursive?)
   "Fetch COMMIT from URL into DIRECTORY.  COMMIT must be a valid Git commit
-identifier.  When RECURSIVE? is true, all the sub-modules of URL are fetched,
-recursively.  Return #t on success, #f otherwise."
+identifier.  When LFS? is true, configure Git to also fetch Large File
+Storage (LFS) files; it assumes that the @code{git-lfs} extension is available
+in the environment.  When RECURSIVE? is true, all the sub-modules of URL are
+fetched, recursively.  Return #t on success, #f otherwise."
 
   ;; Disable TLS certificate verification.  The hash of the checkout is known
   ;; in advance anyway.
@@ -57,6 +60,11 @@ (define* (git-fetch url commit directory
     (with-directory-excursion directory
       (invoke git-command "init" "--initial-branch=main")
       (invoke git-command "remote" "add" "origin" url)
+
+      (when lfs?
+        (setenv "HOME" "/tmp")
+        (invoke git-command "lfs" "install"))
+
       (if (zero? (system* git-command "fetch" "--depth" "1" "origin" commit))
           (invoke git-command "checkout" "FETCH_HEAD")
           (begin
@@ -81,11 +89,13 @@ (define* (git-fetch url commit directory
 
 
 (define* (git-fetch-with-fallback url commit directory
-                                  #:key (git-command "git") recursive?)
+                                  #:key (git-command "git")
+                                  lfs? recursive?)
   "Like 'git-fetch', fetch COMMIT from URL into DIRECTORY, but fall back to
 alternative methods when fetching from URL fails: attempt to download a nar,
 and if that also fails, download from the Software Heritage archive."
   (or (git-fetch url commit directory
+                 #:lfs? lfs?
                  #:recursive? recursive?
                  #:git-command git-command)
       (download-nar directory)
diff --git a/guix/config.scm.in b/guix/config.scm.in
index 62e15dd713..4997a1740e 100644
--- a/guix/config.scm.in
+++ b/guix/config.scm.in
@@ -36,6 +36,7 @@ (define-module (guix config)
 
             %system
             %git
+            %git-lfs
             %gzip
             %bzip2
             %xz))
@@ -113,6 +114,9 @@ (define %system
 (define %git
   "@GIT@")
 
+(define %git-lfs
+  "@GIT_LFS@")
+
 (define %gzip
   "@GZIP@")
 
diff --git a/guix/git-download.scm b/guix/git-download.scm
index 5d5d73dc6b..6feeea6428 100644
--- a/guix/git-download.scm
+++ b/guix/git-download.scm
@@ -51,6 +51,7 @@ (define-module (guix git-download)
             git-reference?
             git-reference-url
             git-reference-commit
+            git-reference-lfs?
             git-reference-recursive?
 
             git-fetch
@@ -71,7 +72,9 @@ (define-record-type* <git-reference>
   git-reference?
   (url        git-reference-url)
   (commit     git-reference-commit)
-  (recursive? git-reference-recursive?   ; whether to recurse into sub-modules
+  (lfs?       git-reference-lfs?        ;whether to fetch LFS files
+              (default #f))
+  (recursive? git-reference-recursive?  ;whether to recurse into sub-modules
               (default #f)))
 
 (define (git-package)
@@ -79,11 +82,17 @@ (define (git-package)
   (let ((distro (resolve-interface '(gnu packages version-control))))
     (module-ref distro 'git-minimal)))
 
+(define (git-lfs-package)
+  "Return the default 'git-lfs' package."
+  (let ((distro (resolve-interface '(gnu packages version-control))))
+    (module-ref distro 'git-lfs)))
+
 (define* (git-fetch/in-band ref hash-algo hash
                             #:optional name
                             #:key (system (%current-system))
                             (guile (default-guile))
-                            (git (git-package)))
+                            (git (git-package))
+                            (git-lfs (git-lfs-package)))
   "Return a fixed-output derivation that performs a Git checkout of REF, using
 GIT and GUILE (thus, said derivation depends on GIT and GUILE).
 
@@ -91,18 +100,22 @@ (define* (git-fetch/in-band ref hash-algo hash
 It will be removed when versions of guix-daemon implementing
 \"builtin:git-download\" will be sufficiently widespread."
   (define inputs
-    `(("git" ,(or git (git-package)))
-
-      ;; When doing 'git clone --recursive', we need sed, grep, etc. to be
-      ;; available so that 'git submodule' works.
+    `(,(or git (git-package))
+      ,@(if (git-reference-lfs? ref)
+            (list (or git-lfs (git-lfs-package)))
+            '())
       ,@(if (git-reference-recursive? ref)
-            (standard-packages)
+            ;; TODO: remove (standard-packages) after
+            ;; 48e528a26f9c019eeaccf5e3de3126aa02c98d3b is merged into master;
+            ;; currently when doing 'git clone --recursive', we need sed, grep,
+            ;; etc. to be available so that 'git submodule' works.
+            (map second (standard-packages))
 
             ;; The 'swh-download' procedure requires tar and gzip.
-            `(("gzip" ,(module-ref (resolve-interface '(gnu packages compression))
-                                   'gzip))
-              ("tar" ,(module-ref (resolve-interface '(gnu packages base))
-                                  'tar))))))
+            (list (module-ref (resolve-interface '(gnu packages compression))
+                              'gzip)
+                  (module-ref (resolve-interface '(gnu packages base))
+                              'tar)))))
 
   (define guile-json
     (module-ref (resolve-interface '(gnu packages guile)) 'guile-json-4))
@@ -126,7 +139,7 @@ (define* (git-fetch/in-band ref hash-algo hash
 
   (define build
     (with-imported-modules modules
-      (with-extensions (list guile-json gnutls    ;for (guix swh)
+      (with-extensions (list guile-json gnutls ;for (guix swh)
                              guile-lzlib)
         #~(begin
             (use-modules (guix build git)
@@ -134,6 +147,9 @@ (define* (git-fetch/in-band ref hash-algo hash
                           #:select (set-path-environment-variable))
                          (ice-9 match))
 
+            (define lfs?
+              (call-with-input-string (getenv "git lfs?") read))
+
             (define recursive?
               (call-with-input-string (getenv "git recursive?") read))
 
@@ -144,18 +160,17 @@ (define* (git-fetch/in-band ref hash-algo hash
                     #+(file-append glibc-locales "/lib/locale"))
             (setlocale LC_ALL "en_US.utf8")
 
-            ;; The 'git submodule' commands expects Coreutils, sed,
-            ;; grep, etc. to be in $PATH.
-            (set-path-environment-variable "PATH" '("bin")
-                                           (match '#+inputs
-                                             (((names dirs outputs ...) ...)
-                                              dirs)))
+            ;; The 'git submodule' commands expects Coreutils, sed, grep,
+            ;; etc. to be in $PATH.  This also ensures that git extensions are
+            ;; found.
+            (set-path-environment-variable "PATH" '("bin") '#+inputs)
 
             (setvbuf (current-output-port) 'line)
             (setvbuf (current-error-port) 'line)
 
             (git-fetch-with-fallback (getenv "git url") (getenv "git commit")
                                      #$output
+                                     #:lfs? lfs?
                                      #:recursive? recursive?
                                      #:git-command "git")))))
 
@@ -175,13 +190,15 @@ (define* (git-fetch/in-band ref hash-algo hash
                                          (git-reference-url ref))))
                         ("git commit" . ,(git-reference-commit ref))
                         ("git recursive?" . ,(object->string
-                                              (git-reference-recursive? ref))))
+                                              (git-reference-recursive? ref)))
+                        ("git lfs?" . ,(object->string
+                                        (git-reference-lfs? ref))))
                       #:leaked-env-vars '("http_proxy" "https_proxy"
                                           "LC_ALL" "LC_MESSAGES" "LANG"
                                           "COLUMNS")
 
                       #:system system
-                      #:local-build? #t           ;don't offload repo cloning
+                      #:local-build? #t ;don't offload repo cloning
                       #:hash-algo hash-algo
                       #:hash hash
                       #:recursive? #t
@@ -209,6 +226,7 @@ (define* (git-fetch/built-in ref hash-algo hash
                                  (_
                                   (git-reference-url ref)))))
                     ("commit" . ,(git-reference-commit ref))
+                    ("lfs?" . ,(object->string (git-reference-lfs? ref)))
                     ("recursive?" . ,(object->string
                                       (git-reference-recursive? ref))))
                   #:leaked-env-vars '("http_proxy" "https_proxy"
diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-download.scm
index 9aa0e61e9d..37904941d1 100644
--- a/guix/scripts/perform-download.scm
+++ b/guix/scripts/perform-download.scm
@@ -96,6 +96,7 @@ (define* (perform-git-download drv output
 'bmRepair' builds."
   (derivation-let drv ((url "url")
                        (commit "commit")
+                       (lfs? "lfs?")
                        (recursive? "recursive?"))
     (unless url
       (leave (G_ "~a: missing Git URL~%") (derivation-file-name drv)))
@@ -103,6 +104,7 @@ (define* (perform-git-download drv output
       (leave (G_ "~a: missing Git commit~%") (derivation-file-name drv)))
 
     (let* ((url        (call-with-input-string url read))
+           (lfs?       (and lfs? (call-with-input-string lfs? read)))
            (recursive? (and recursive?
                             (call-with-input-string recursive? read)))
            (drv-output (assoc-ref (derivation-outputs drv) "out"))
@@ -115,6 +117,7 @@ (define* (perform-git-download drv output
       (setenv "PATH" "/run/current-system/profile/bin:/bin:/usr/bin")
 
       (git-fetch-with-fallback url commit output
+                               #:lfs? lfs?
                                #:recursive? recursive?
                                #:git-command %git))))
 
diff --git a/guix/self.scm b/guix/self.scm
index a1f235659d..96021be6f6 100644
--- a/guix/self.scm
+++ b/guix/self.scm
@@ -69,6 +69,7 @@ (define %packages
       ("gzip"               . ,(ref 'compression 'gzip))
       ("bzip2"              . ,(ref 'compression 'bzip2))
       ("xz"                 . ,(ref 'compression 'xz))
+      ("git-lfs"            . ,(ref 'version-control 'git-lfs))
       ("git-minimal"        . ,(ref 'version-control 'git-minimal))
       ("po4a"               . ,(ref 'gettext 'po4a))
       ("gettext-minimal"    . ,(ref 'gettext 'gettext-minimal))
@@ -830,6 +831,9 @@ (define* (compiled-guix source #:key
   (define git
     (specification->package "git-minimal"))
 
+  (define git-lfs
+    (specification->package "git-lfs"))
+
   (define dependencies
     (append-map transitive-package-dependencies
                 (list guile-gcrypt guile-gnutls guile-git guile-avahi
@@ -1004,6 +1008,7 @@ (define* (compiled-guix source #:key
                                          #:bzip2 bzip2
                                          #:xz xz
                                          #:git git
+                                         #:git-lfs git-lfs
                                          #:package-name
                                          %guix-package-name
                                          #:package-version
@@ -1109,7 +1114,7 @@ (define %default-config-variables
     (%storedir . "/gnu/store")
     (%sysconfdir . "/etc")))
 
-(define* (make-config.scm #:key gzip xz bzip2 git
+(define* (make-config.scm #:key gzip xz bzip2 git git-lfs
                           (package-name "GNU Guix")
                           (package-version "0")
                           (channel-metadata #f)
@@ -1140,6 +1145,7 @@ (define* (make-config.scm #:key gzip xz bzip2 git
                                %store-database-directory
                                %config-directory
                                %git
+                               %git-lfs
                                %gzip
                                %bzip2
                                %xz))
@@ -1184,6 +1190,8 @@ (define* (make-config.scm #:key gzip xz bzip2 git
 
                    (define %git
                      #+(and git (file-append git "/bin/git")))
+                   (define %git-lfs
+                     #+(and git-lfs (file-append git-lfs "/bin/git-lfs")))
                    (define %gzip
                      #+(and gzip (file-append gzip "/bin/gzip")))
                    (define %bzip2
-- 
2.41.0

Message #23 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 66436 <at> debbugs.gnu.org
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#66436] [PATCH 0/2] Add support for Git Large File Storage
 (LFS).
Date: Wed, 11 Oct 2023 22:51:48 -0400

Hello,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:

> While updating orcus to its latest version (0.19.0), I stumbled on a test a
> failure, which ended up being caused by the lack of test files in the
> repository checkout.  These files are stored using the LFS extension in the
> repo; when not using LFS, empty text stubs with some metadata are left in
> their place.
>
> I've opted to keep the dependency on git-lfs optional for now for the daemon.
> The git in-band downloader will only add the git-lfs dependency when the
> git-reference object has its lfs? field set to true.
>
> Maxim Cournoyer (2):
>   gnu: git-lfs: Patch /bin/sh references.
>   git-download: Add support for Git Large File Storage (LFS).

Apologies for sending this series twice; the first time I sent it
erroneously to 66436 after forgetting to run 'mumi new'.

-- 
Thanks,
Maxim

Message #26 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org
Subject: Re: [bug#66436] [PATCH 1/2] gnu: git-lfs: Patch /bin/sh references.
Date: Sat, 14 Oct 2023 19:04:44 +0200

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> * gnu/packages/version-control.scm (git-lfs)
> [arguments]: Add patch-/bin/sh phase.

LGTM!

Message #29 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 66436 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large
 File Storage (LFS).
Date: Sat, 14 Oct 2023 19:12:42 +0200

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> * guix/git-download.scm (<git-reference>) [lfs?]: New field.
> (git-fetch/in-band): New #:git-lfs argument.
> <inputs>: Remove labels.  Conditionally add git-lfs.
> <build>: Read "git lfs?" environment
> variable and pass its value to the #:lfs? argument of git-fetch-with-fallback.
> Use INPUTS directly; update comment.
> <gexp->derivation>: Add "git lfs?" to #:env-vars.
> (git-fetch/built-in): Add "lfs?" to #:env-vars.
> * guix/build/git.scm (git-fetch) [lfs?]: New argument, doc and setup code.
> (git-fetch-with-fallback) [lfs?]: New argument.  Pass it to git-fetch.
> * guix/scripts/perform-download.scm (perform-git-download): Honor the 'lfs?'
> environment variable.
> * doc/guix.texi (origin Reference) <git-reference>: Document the new 'lfs?'
> field.
>  (Requirements): Mention the optional 'git-lfs' dependency.
> * configure.ac: Add a check for the 'git-lfs' command.
> * guix/config.scm.in (%git-lfs): New variable.
> * guix/self.scm (%packages): Add git-lfs.
> (compiled-guix): Add git-lfs to guix-config.
> (make-config.scm): New #:git-lfs argument.

I wonder whether this is a desirable feature, in the sense that the
store is not designed to hold large amounts of data: just like Git has
Git-LFS and Git-Annex, we’d need Guix-Annex (in fact, the GWL is kinda
doing that already!).

Furthermore…

> +dnl Git Large File Storage is an optional dependency for the
> +dnl "builtin:git-download" derivation builder.
> +AC_PATH_PROG([GIT_LFS], [git-lfs])
> +if test "x$GIT_LFS" = "x"; then
> +  AC_MSG_WARN([Git Large File Storage (git-lfs) is missing;
> +  The builtin:git-download derivation builder of the Guix daemon will
> +  not be able to use it.])

… I don’t think we want to spend more words on the effect of increasing
the closure size and getting locked with Git (libgit2 doesn’t implement
LFS I suppose.)  To me this part is a showstopper; we should just make
it clear that “builtin:git-download” does not implement LFS.

Also, it is crucial for the “builtin:git-download” semantics to be the
same across all installations and to be very stable.

> +(define* (make-config.scm #:key gzip xz bzip2 git git-lfs
>                            (package-name "GNU Guix")
>                            (package-version "0")
>                            (channel-metadata #f)
> @@ -1140,6 +1145,7 @@ (define* (make-config.scm #:key gzip xz bzip2 git
>                                 %store-database-directory
>                                 %config-directory
>                                 %git
> +                               %git-lfs

This I’d like to avoid, too (for the size issue).

Overall, I feel like LFS support, if needed, should be “on the side”,
with a custom ‘git-fetch/lfs’ or something along these lines (just like
we have ‘url-fetch/tarbomb’).

We might still need to change (guix build git) to implement it, but I
would make sure that “builtin:git-download” remains unaffected and never
fetches LFS stuff, even if ‘git-lfs’ happens to be on $PATH or
something.

WDYT?

Ludo’.

Message #32 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, 66436 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Sun, 15 Oct 2023 11:55:15 +0200

[Message part 1 (text/plain, inline)]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:

> +@cindex LGTM, Looks Good To Me
> +When you deem the proposed change adequate and ready for inclusion
> +within Guix, the following well understood/codified @acronym{LGTM, Looks
> +Good To Me} phrases should be used to sign off as a reviewer, meaning
> +you have reviewed the change and that it looks good to you:
> +
> +@itemize
> +@item
> +If the @emph{whole} series (containing multiple commits) looks good to
> +you, reply with a @samp{This series LGTM!} to the cover page if it has
> +one, or to the last patch of the series otherwise.
> +
> +@item
> +If you instead want to mark a @emph{single commit} as reviewed (but not
> +the whole series), simply reply with @samp{LGTM!} to that commit
> +message.
> +@end itemize

Some mbox -> patches tools (like b4) are able to automatically apply
trailers like "Reviewed-by: some ident <...>" found as replies to patch
series [1].  I also like the very clear meaning of it: there's no way
you would type this without meaning "Yes, I officially vouch that this
patch series has been reviewed by me".  Maybe this could be codified
here?

Otherwise, seem good to me.

[1] `b4 am`'s -t option, see
https://b4.docs.kernel.org/en/latest/maintainer/am-shazam.html

Best,
-- 
Josselin Poiret

[signature.asc (application/pgp-signature, inline)]

Message #35 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Mon, 16 Oct 2023 10:02:12 -0400

Hello,

Josselin Poiret <dev <at> jpoiret.xyz> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:
>
>> +@cindex LGTM, Looks Good To Me
>> +When you deem the proposed change adequate and ready for inclusion
>> +within Guix, the following well understood/codified @acronym{LGTM, Looks
>> +Good To Me} phrases should be used to sign off as a reviewer, meaning
>> +you have reviewed the change and that it looks good to you:
>> +
>> +@itemize
>> +@item
>> +If the @emph{whole} series (containing multiple commits) looks good to
>> +you, reply with a @samp{This series LGTM!} to the cover page if it has
>> +one, or to the last patch of the series otherwise.
>> +
>> +@item
>> +If you instead want to mark a @emph{single commit} as reviewed (but not
>> +the whole series), simply reply with @samp{LGTM!} to that commit
>> +message.
>> +@end itemize
>
> Some mbox -> patches tools (like b4) are able to automatically apply
> trailers like "Reviewed-by: some ident <...>" found as replies to patch
> series [1].  I also like the very clear meaning of it: there's no way
> you would type this without meaning "Yes, I officially vouch that this
> patch series has been reviewed by me".  Maybe this could be codified
> here?

Reading about it at
<https://b4.docs.kernel.org/en/latest/maintainer/am-shazam.html>, it
seems 'b4 -t' is about adding the 'Review-by' git trailers found in
replies to *cover letters*, and inserting them on each commit of the
series when applied.

Do you use such a tool when applying patches working with Guix review?
I'm not too fond of adding tool-specific bits to the otherwise
tool-agnostic section above, but perhaps we could add a note or
something.

-- 
Thanks,
Maxim

Message #38 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 66436 <at> debbugs.gnu.org
Subject: Re: [bug#66436] [PATCH 1/2] gnu: git-lfs: Patch /bin/sh references.
Date: Mon, 16 Oct 2023 10:46:32 -0400

Hi,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> * gnu/packages/version-control.scm (git-lfs)
>> [arguments]: Add patch-/bin/sh phase.
>
> LGTM!

I've installed this commit, thanks.

-- 
Thanks,
Maxim

Message #41 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 66436 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large
 File Storage (LFS).
Date: Mon, 16 Oct 2023 11:10:16 -0400

Hi Ludovic,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> * guix/git-download.scm (<git-reference>) [lfs?]: New field.
>> (git-fetch/in-band): New #:git-lfs argument.
>> <inputs>: Remove labels.  Conditionally add git-lfs.
>> <build>: Read "git lfs?" environment
>> variable and pass its value to the #:lfs? argument of git-fetch-with-fallback.
>> Use INPUTS directly; update comment.
>> <gexp->derivation>: Add "git lfs?" to #:env-vars.
>> (git-fetch/built-in): Add "lfs?" to #:env-vars.
>> * guix/build/git.scm (git-fetch) [lfs?]: New argument, doc and setup code.
>> (git-fetch-with-fallback) [lfs?]: New argument.  Pass it to git-fetch.
>> * guix/scripts/perform-download.scm (perform-git-download): Honor the 'lfs?'
>> environment variable.
>> * doc/guix.texi (origin Reference) <git-reference>: Document the new 'lfs?'
>> field.
>>  (Requirements): Mention the optional 'git-lfs' dependency.
>> * configure.ac: Add a check for the 'git-lfs' command.
>> * guix/config.scm.in (%git-lfs): New variable.
>> * guix/self.scm (%packages): Add git-lfs.
>> (compiled-guix): Add git-lfs to guix-config.
>> (make-config.scm): New #:git-lfs argument.
>
> I wonder whether this is a desirable feature, in the sense that the
> store is not designed to hold large amounts of data: just like Git has
> Git-LFS and Git-Annex, we’d need Guix-Annex (in fact, the GWL is kinda
> doing that already!).

Interesting thoughts, though in the case of running the 'orcus' package
test suite, the "large" files are simply .ods files (Open Document
spreadsheet).  It seems LFS is useful to store binary data files of any
kind, notwithstanding their size.

> Furthermore…
>
>> +dnl Git Large File Storage is an optional dependency for the
>> +dnl "builtin:git-download" derivation builder.
>> +AC_PATH_PROG([GIT_LFS], [git-lfs])
>> +if test "x$GIT_LFS" = "x"; then
>> +  AC_MSG_WARN([Git Large File Storage (git-lfs) is missing;
>> +  The builtin:git-download derivation builder of the Guix daemon will
>> +  not be able to use it.])
>
> … I don’t think we want to spend more words on the effect of increasing
> the closure size and getting locked with Git (libgit2 doesn’t implement
> LFS I suppose.)  To me this part is a showstopper; we should just make
> it clear that “builtin:git-download” does not implement LFS.

I thought we already did get locked with having git as a hard
dependency.  I guess you are keeping some hope for that to be reversed
in the future, pending new libgit2 developments such as gaining garbage
collection (git gc) support?  git-lfs would increase the closure of guix
from 688 to 700 MiB.

> Also, it is crucial for the “builtin:git-download” semantics to be the
> same across all installations and to be very stable.
>
>> +(define* (make-config.scm #:key gzip xz bzip2 git git-lfs
>>                            (package-name "GNU Guix")
>>                            (package-version "0")
>>                            (channel-metadata #f)
>> @@ -1140,6 +1145,7 @@ (define* (make-config.scm #:key gzip xz bzip2 git
>>                                 %store-database-directory
>>                                 %config-directory
>>                                 %git
>> +                               %git-lfs
>
> This I’d like to avoid, too (for the size issue).
>
> Overall, I feel like LFS support, if needed, should be “on the side”,
> with a custom ‘git-fetch/lfs’ or something along these lines (just like
> we have ‘url-fetch/tarbomb’).
>
> We might still need to change (guix build git) to implement it, but I
> would make sure that “builtin:git-download” remains unaffected and never
> fetches LFS stuff, even if ‘git-lfs’ happens to be on $PATH or
> something.
>
> WDYT?

Your arguments make sense.  I'll try to see how git-fetch/lfs could be
implemented.

-- 
Thanks,
Maxim

Message #44 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>, Maxim Cournoyer
 <maxim.cournoyer <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 66436 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large
 File Storage (LFS).
Date: Mon, 16 Oct 2023 17:15:39 +0200

Hi,

I reorder Ludo’s answers for clarity.

On Sat, 14 Oct 2023 at 19:12, Ludovic Courtès <ludo <at> gnu.org> wrote:

> Also, it is crucial for the “builtin:git-download” semantics to be the
> same across all installations and to be very stable.

I agree and that’s one strong argument for me.

> Overall, I feel like LFS support, if needed, should be “on the side”,
> with a custom ‘git-fetch/lfs’ or something along these lines (just like
> we have ‘url-fetch/tarbomb’).

I agree with this direction.  Most of the fetching methods should not be
implemented with “builtin“ but the converse, git-fetch/lfs.

    ( Even, I think the current git-fetch using out-of-band builtin
recently introduced should be a ’git-fetch/bootstrap’ method, and
git-fetch using in-band should be the default method. )


Cheers,
simon

Message #47 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 66436 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large
 File Storage (LFS).
Date: Mon, 16 Oct 2023 12:23:56 -0400

Hi,

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

> Hi,
>
> I reorder Ludo’s answers for clarity.
>
> On Sat, 14 Oct 2023 at 19:12, Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>> Also, it is crucial for the “builtin:git-download” semantics to be the
>> same across all installations and to be very stable.
>
> I agree and that’s one strong argument for me.
>
>> Overall, I feel like LFS support, if needed, should be “on the side”,
>> with a custom ‘git-fetch/lfs’ or something along these lines (just like
>> we have ‘url-fetch/tarbomb’).
>
> I agree with this direction.  Most of the fetching methods should not be
> implemented with “builtin“ but the converse, git-fetch/lfs.
>
>     ( Even, I think the current git-fetch using out-of-band builtin
> recently introduced should be a ’git-fetch/bootstrap’ method, and
> git-fetch using in-band should be the default method. )

Thanks for tipping in, I'll look into adding a whole new 'git-fetch/lfs'
procedure.

-- 
Thanks,
Maxim

Message #50 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Fri, 20 Oct 2023 10:12:11 +0200

Hi,

These are a few questions regarding reviewing.

1. What should the reviewer do with old-style patches, like the ones
   that don't use G-Expressions?  Should we tell the submitter to use
   them when possible or is it only a matter of style that is up to the
   submitter?  Obviously they are hard to grasp for newcomers.

   It's probably good for newcomers if we teach them how to use
   G-Expressions but we don't really have time to do so, given the
   number of patches waiting to be reviewed.

   This question could be extended to style issues.  Like using %var
   versus var.

2. What should the reviewer do when only small changes are required?
   The reviewer could do these changes in seconds whereas asking for a
   new revision could take days.  These changes could be indentation
   fixes, removing of unused code, but they could also be more
   substantial, like adding a missing `file-name` field.  Or changing
   old-style to G-Expressions?

   If the reviewer makes such changes and pushes them right away, I
   imagine they should be documented and explained.

3. Should the reviewer run the program being packaged?  The above
   guidelines speak about applying, reading, building and linting but
   not running.  (Making sure it works as expected.)

Thanks,
Clément

Message #53 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Clément Lassieur <clement <at> lassieur.org>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Fri, 20 Oct 2023 19:01:41 -0400

Hi,

Clément Lassieur <clement <at> lassieur.org> writes:

> Hi,
>
> These are a few questions regarding reviewing.
>
> 1. What should the reviewer do with old-style patches, like the ones
>    that don't use G-Expressions?  Should we tell the submitter to use
>    them when possible or is it only a matter of style that is up to the
>    submitter?  Obviously they are hard to grasp for newcomers.
>
>    It's probably good for newcomers if we teach them how to use
>    G-Expressions but we don't really have time to do so, given the
>    number of patches waiting to be reviewed.
>
>    This question could be extended to style issues.  Like using %var
>    versus var.

I think we should now make sure all new submissions use the current
style; if they aren't we can demand of the contributors to adjust it.
There is a blog post and enough examples in the code base already that
should make this not too difficult.

> 2. What should the reviewer do when only small changes are required?
>    The reviewer could do these changes in seconds whereas asking for a
>    new revision could take days.  These changes could be indentation
>    fixes, removing of unused code, but they could also be more
>    substantial, like adding a missing `file-name` field.  Or changing
>    old-style to G-Expressions?
>
>    If the reviewer makes such changes and pushes them right away, I
>    imagine they should be documented and explained.

In general, I think it's best to let the contributor know about the
small problems so they can submit a v2, as they'll learn to pay
attention to these.  For first submissions, we can make the experience
easier by adjusting locally and pushing directly for small things.  This
also holds for very old contributions where the original author is no
longer around.

I think these two points could be expound as new subsections of the
'Coding Style' section; the current scope is about codifying the human
interactions more than the technical details.

> 3. Should the reviewer run the program being packaged?  The above
>    guidelines speak about applying, reading, building and linting but
>    not running.  (Making sure it works as expected.)

From the diff:

--8<---------------cut here---------------start------------->8---
+Perhaps the biggest action you can do to help GNU Guix grow as a project
+is to review the work contributed by others.  You do not need to be a
+committer to do so; applying, reading the source, building, linting and
+running other people's series and sharing your comments about your
+experience will give some confidence to committers, and should result in
+the proposed change being merged faster.
--8<---------------cut here---------------end--------------->8---

So it does mention trying out the software ("running").

-- 
Thanks,
Maxim

Message #56 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Sun, 22 Oct 2023 22:03:12 +0200

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:

>> 1. What should the reviewer do with old-style patches, like the ones
>>    that don't use G-Expressions?  Should we tell the submitter to use
>>    them when possible or is it only a matter of style that is up to the
>>    submitter?  Obviously they are hard to grasp for newcomers.
>>
>>    It's probably good for newcomers if we teach them how to use
>>    G-Expressions but we don't really have time to do so, given the
>>    number of patches waiting to be reviewed.
>>
>>    This question could be extended to style issues.  Like using %var
>>    versus var.
>
> I think we should now make sure all new submissions use the current
> style; if they aren't we can demand of the contributors to adjust it.
> There is a blog post and enough examples in the code base already that
> should make this not too difficult.

Are you referring to this one?
https://guix.gnu.org/en/blog/2023/dissecting-guix-part-3-g-expressions/

>> 2. What should the reviewer do when only small changes are required?
>>    The reviewer could do these changes in seconds whereas asking for a
>>    new revision could take days.  These changes could be indentation
>>    fixes, removing of unused code, but they could also be more
>>    substantial, like adding a missing `file-name` field.  Or changing
>>    old-style to G-Expressions?
>>
>>    If the reviewer makes such changes and pushes them right away, I
>>    imagine they should be documented and explained.
>
> +Perhaps the biggest action you can do to help GNU Guix grow as a project
> +is to review the work contributed by others.  You do not need to be a
> +committer to do so; applying, reading the source, building, linting and
> +running other people's series and sharing your comments about your
> +experience will give some confidence to committers, and should result in
> +the proposed change being merged faster.
>
> So it does mention trying out the software ("running").

Yes indeed!  I didn't see it.

Thanks for your reply!
Clément

Message #59 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Clément Lassieur <clement <at> lassieur.org>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Sun, 22 Oct 2023 21:55:54 -0400

Hi,

Clément Lassieur <clement <at> lassieur.org> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:
>
>>> 1. What should the reviewer do with old-style patches, like the ones
>>>    that don't use G-Expressions?  Should we tell the submitter to use
>>>    them when possible or is it only a matter of style that is up to the
>>>    submitter?  Obviously they are hard to grasp for newcomers.
>>>
>>>    It's probably good for newcomers if we teach them how to use
>>>    G-Expressions but we don't really have time to do so, given the
>>>    number of patches waiting to be reviewed.
>>>
>>>    This question could be extended to style issues.  Like using %var
>>>    versus var.
>>
>> I think we should now make sure all new submissions use the current
>> style; if they aren't we can demand of the contributors to adjust it.
>> There is a blog post and enough examples in the code base already that
>> should make this not too difficult.
>
> Are you referring to this one?
> https://guix.gnu.org/en/blog/2023/dissecting-guix-part-3-g-expressions/

Rather to the one corresponding to the 1.4.0 release, which introduced
these new changes: <https://guix.gnu.org/en/blog/2022/gnu-guix-1.4.0-released/>.

-- 
Thanks,
Maxim

Message #62 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Clément
 Lassieur <clement <at> lassieur.org>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Tue, 24 Oct 2023 10:49:35 +0200

Hi,

On Fri, 20 Oct 2023 at 19:01, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> wrote:

>> 3. Should the reviewer run the program being packaged?  The above
>>    guidelines speak about applying, reading, building and linting but
>>    not running.  (Making sure it works as expected.)

> --8<---------------cut here---------------start------------->8---
> +Perhaps the biggest action you can do to help GNU Guix grow as a project
> +is to review the work contributed by others.  You do not need to be a
> +committer to do so; applying, reading the source, building, linting and
> +running other people's series and sharing your comments about your
> +experience will give some confidence to committers, and should result in
> +the proposed change being merged faster.
> --8<---------------cut here---------------end--------------->8---
>
> So it does mention trying out the software ("running").

If LGTM also implies “I run it and it is OK”, then submitter should
provide how to run it.

Otherwise, for what it is worth, I will stop to review stuff that I do
not use myself because reviewing is asking me too much: read some doc
about how to run something that I do not care.

    ( Similarly, if LGTM also implies “I have read the source code and
it is OK”, it appears to me too much. )

Well, “running” and “trying out the software” seems ambiguous.  What
does it mean “run IceCat” or “run Gmsh” or else?

What I like with the proposal is that it makes better defined what are
the expectations behind LGTM.  But what means “running” is not clear for
me.

IMHO, it is worth to clearly state:

 1. what helps the review process:

    « applying, reading the source, building, linting and running other
    people's series and sharing your comments about your experience will
    give some confidence to committers, and should result in the
    proposed change being merged faster. »

 2. what means LGTM, from my understanding: applying, building, linting,
    carefully checking the code that is merged to Guix.

Cheers,
simon

Message #65 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Clément
 Lassieur <clement <at> lassieur.org>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Tue, 24 Oct 2023 10:59:45 +0200

Re,

On Tue, 24 Oct 2023 at 10:49, Simon Tournier <zimon.toutoune <at> gmail.com> wrote:

>> --8<---------------cut here---------------start------------->8---
>> +Perhaps the biggest action you can do to help GNU Guix grow as a project
>> +is to review the work contributed by others.  You do not need to be a
>> +committer to do so; applying, reading the source, building, linting and
>> +running other people's series and sharing your comments about your
>> +experience will give some confidence to committers, and should result in
>> +the proposed change being merged faster.
>> --8<---------------cut here---------------end--------------->8---

[...]

> IMHO, it is worth to clearly state:
>
>  1. what helps the review process:
>
>     « applying, reading the source, building, linting and running other
>     people's series and sharing your comments about your experience will
>     give some confidence to committers, and should result in the
>     proposed change being merged faster. »

Although I do not know if it is the right place, I would mention that
“building” also means that the dependants still build or clearly
indicate if something breaks.


Cheers,
simon

Message #68 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Tue, 24 Oct 2023 17:54:11 +0200

Hi,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> * doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
> section.
> * doc/contributing.texi (Debbugs Usertags): Expound with Emacs Debbugs
    ^
Nitpick: no need to repeat the file name.

> +@node Reviewing the Work of Others
> +@section Reviewing the Work of Others

[...]

> +@cindex reviewing, guidelines
> +Review comments should be unambiguous; be as clear and explicit as you
> +can about what you think should be changed, ensuring the author can take
> +action on it.  Please try to keep the following guidelines in mind
> +during review:

[...]

> +@item
> +@emph{Remain focused: do not change the scope of the work being
> +reviewed.}  For example, if the contribution touches code that follows a
> +pattern deemed unwieldy, it would be unfair to ask the submitter to fix
> +all occurrences of that pattern in the code; to put it simply, if a
> +problem unrelated to the patch at hand was already there, do not ask the
> +submitter to fix it.

Another item came to mind, that could be phrased like this:

  Spend time proportional to the stakes.  Ensure the discussion focuses
  on important aspects of the changes; do not let it be derailed by
  objectively unimportant issues <at> footnote{This situation is often
  referred to as ``bikeshedding'', where much time is spent discussing
  each one's preference for the color of the shed at the expense
  progress made on the project to keep bikes dry.}.  In particular,
  issues such as code formatting and other conventions can be dealt with
  through automation---e.g., @command{guix lint} and @command{guix
  style}---or through documentation (@pxref{Packaging Guidelines}, as an
  example).

Maybe that makes the guidelines too long though; your call.

Otherwise LGTM!

Ludo’.

Message #71 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>, Maxim Cournoyer
 <maxim.cournoyer <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Wed, 25 Oct 2023 15:53:04 +0200

Hi

On Tue, 24 Oct 2023 at 17:54, Ludovic Courtès <ludo <at> gnu.org> wrote:

>> +@item
>> +@emph{Remain focused: do not change the scope of the work being
>> +reviewed.}  For example, if the contribution touches code that follows a
>> +pattern deemed unwieldy, it would be unfair to ask the submitter to fix
>> +all occurrences of that pattern in the code; to put it simply, if a
>> +problem unrelated to the patch at hand was already there, do not ask the
>> +submitter to fix it.

For me this item is clear…

> Another item came to mind, that could be phrased like this:

…while this new is unclear…

>   Spend time proportional to the stakes.  Ensure the discussion focuses
>   on important aspects of the changes; do not let it be derailed by
>   objectively unimportant issues <at> footnote{This situation is often
>   referred to as ``bikeshedding'', where much time is spent discussing
>   each one's preference for the color of the shed at the expense
>   progress made on the project to keep bikes dry.}.  In particular,
>   issues such as code formatting and other conventions can be dealt with
>   through automation---e.g., @command{guix lint} and @command{guix
>   style}---or through documentation (@pxref{Packaging Guidelines}, as an
>   example).

…especially in the light of these other items:

        +@item
        +@emph{Review is a discussion.}  The submitter's and reviewer's views on
        +how to achieve a particular change may not always be aligned.  As a
        +reviewer, try hard to explain the rationale for suggestions you make,
        +and to understand and take into account the submitter's motivation for
        +doing things in a certain way.

        +@item
        +@emph{Aim for finalization.}  Reviewing code is time-consuming.  Your
        +goal as a reviewer is to put the process on a clear path towards
        +integration, possibly with agreed-upon changes, or rejection, with a
        +clear and mutually-understood reasoning.  Avoid leaving the review
        +process in a lingering state with no clear way out.


Well, I do not like: « discussion focuses on important aspects of the
changes; do not let it be derailed by objectively unimportant issues »
because it is not clear for me what means “important aspects” or
“objectively unimportant issues”.  How do I gauge?

Sometimes, what does not appear to me “important” at first has then, at
the end, an impact that cannot be neglected.  This new item appears to
me as: it is not a open discussion and you should refrain from
commenting if you are not sure your point is *absolutely* important.

Instead of this new item – where my understanding is: avoid bikeshed :-)
and I agree – I propose to amend the item @emph{Review is a discussion.}
by explicitly refer to the 3 other items; which are, IMHO, the guards
against bikeshedding.  Something along:

--8<---------------cut here---------------start------------->8---
@item
@emph{Review is a discussion.}  The submitter's and reviewer's views on
how to achieve a particular change may not always be aligned.  The
discussion is lead by remain focused, ensure progress and aim for
finalization; spend time proportional to the stakes <at> footnote{This
situation is often referred to as ``bikeshedding'', where much time is
spent discussing each one's preference for the color of the shed at the
expense progress made on the project to keep bikes dry.}.  As a
reviewer, try hard to explain the rationale for suggestions you make,
and to understand and take into account the submitter's motivation for
doing things in a certain way.
--8<---------------cut here---------------end--------------->8---

WDYT?  Does it capture the idea?

If yes, I would pick this order for the enumeration:

 1. Be clear and explicit about changes you are suggesting 
 2. Remain focused
 3. Ensure progress
 4. Aim for finalization
 5. Review is a discussion


Cheers,
simon

Message #74 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Sun, 29 Oct 2023 15:52:21 +0100

[Message part 1 (text/plain, inline)]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:

> Do you use such a tool when applying patches working with Guix review?

Yes, it's very useful.

> I'm not too fond of adding tool-specific bits to the otherwise
> tool-agnostic section above, but perhaps we could add a note or
> something.

You can view this as a tool-agnostic but clear way way of formally
saying "I've reviewed this".  Otherwise, it's not entirely clear when
someone has reviewed a patch series IMO.

Best,
-- 
Josselin Poiret

[signature.asc (application/pgp-signature, inline)]

Message #77 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>,
 Clément Lassieur <clement <at> lassieur.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Tue, 31 Oct 2023 14:53:45 -0400

Hi Simon,

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

> Re,
>
> On Tue, 24 Oct 2023 at 10:49, Simon Tournier <zimon.toutoune <at> gmail.com> wrote:
>
>>> --8<---------------cut here---------------start------------->8---
>>> +Perhaps the biggest action you can do to help GNU Guix grow as a project
>>> +is to review the work contributed by others.  You do not need to be a
>>> +committer to do so; applying, reading the source, building, linting and
>>> +running other people's series and sharing your comments about your
>>> +experience will give some confidence to committers, and should result in
>>> +the proposed change being merged faster.
>>> --8<---------------cut here---------------end--------------->8---
>
> [...]
>
>> IMHO, it is worth to clearly state:
>>
>>  1. what helps the review process:
>>
>>     « applying, reading the source, building, linting and running other
>>     people's series and sharing your comments about your experience will
>>     give some confidence to committers, and should result in the
>>     proposed change being merged faster. »
>
> Although I do not know if it is the right place, I would mention that
> “building” also means that the dependants still build or clearly
> indicate if something breaks.

That's already mentioned in 'Submitting Patches':

  9. Check that dependent packages (if applicable) are not affected by
     the change; ‘guix refresh --list-dependent PACKAGE’ will help you
     do that (*note Invoking guix refresh::).

I don't think we should repeat it here :-) (also, we now have CI, which
should be more apt at catching breakage here).

--
Thanks,
Maxim

Message #80 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>,
 Clément Lassieur <clement <at> lassieur.org>
Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Tue, 31 Oct 2023 20:03:47 +0100

Hi Maxim,

On Tue, 31 Oct 2023 at 19:53, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> wrote:

> >> IMHO, it is worth to clearly state:

[...]

> That's already mentioned in 'Submitting Patches':

[...]

> I don't think we should repeat it here :-) (also, we now have CI, which
> should be more apt at catching breakage here).

We are listing the expectations for the Review process, therefore
repeat that the dependencies need to be checked at Review time makes
sense to me.  It can be a sentence as: "make sure CI does not report
new error" or something like that.

Cheers,
simon

Message #83 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 66436 <at> debbugs.gnu.org
Cc: dev <at> jpoiret.xyz, ludo <at> gnu.org, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,
 zimon.toutoune <at> gmail.com
Subject: [PATCH v3] doc: Add some guidelines for reviewing.
Date: Wed,  1 Nov 2023 15:23:23 -0400

* doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
section.
(Debbugs Usertags): Expound with Emacs Debbugs information and document the
'reviewed-looks-good' usertag.

Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
---

Changes in v3:
 - Replace LGTM with Reviewed-by Git tag
 - Add b4 config
 - Link to the Submitting Patches section for check list
 - Fuse further suggestions by both Ludovic and Simon

 doc/contributing.texi | 111 ++++++++++++++++++++++++++++++++++++++++--
 etc/git/gitconfig     |   7 +++
 2 files changed, 114 insertions(+), 4 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 30876447d4..023478c98d 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -29,6 +29,7 @@ Contributing
 * Submitting Patches::          Share your work.
 * Tracking Bugs and Changes::   Keeping it all organized.
 * Commit Access::               Pushing to the official repository.
+* Reviewing the Work of Others::  Some guidelines for sharing reviews.
 * Updating the Guix Package::   Updating the Guix package definition.
 * Writing Documentation::       Improving documentation in GNU Guix.
 * Translating Guix::            Make Guix speak your native language.
@@ -1981,7 +1982,12 @@ Debbugs Usertags
 tag any bug with an arbitrary label.  Bugs can be searched by usertag,
 so this is a handy way to organize bugs <at> footnote{The list of usertags is
 public information, and anyone can modify any user's list of usertags,
-so keep that in mind if you choose to use this feature.}.
+so keep that in mind if you choose to use this feature.}.  If you use
+Emacs Debbugs, the entry-point to consult existing usertags is the
+@samp{C-u M-x debbugs-gnu-usertags} procedure.  To set a usertag, press
+@samp{C} while consulting a bug within the *Guix-Patches* buffer opened
+with @samp{C-u M-x debbugs-gnu-bugs} buffer, then select @code{usertag}
+and follow the instructions.
 
 For example, to view all the bug reports (or patches, in the case of
 @code{guix-patches}) tagged with the usertag @code{powerpc64le-linux}
@@ -1994,9 +2000,9 @@ Debbugs Usertags
 to interact with Debbugs.
 
 In Guix, we are experimenting with usertags to keep track of
-architecture-specific issues.  To facilitate collaboration, all our
-usertags are associated with the single user @code{guix}.  The following
-usertags currently exist for that user:
+architecture-specific issues, as well as reviewed ones.  To facilitate
+collaboration, all our usertags are associated with the single user
+@code{guix}.  The following usertags currently exist for that user:
 
 @table @code
 
@@ -2014,6 +2020,9 @@ Debbugs Usertags
 appropriate to assign this usertag to a bug report for a package that
 fails to build reproducibly.
 
+@item reviewed-looks-good
+You have reviewed the series and it looks good to you (LGTM).
+
 @end table
 
 If you're a committer and you want to add a usertag, just start using it
@@ -2283,6 +2292,100 @@ Commit Access
 you're welcome to use your expertise and commit rights to help other
 contributors, too!
 
+@node Reviewing the Work of Others
+@section Reviewing the Work of Others
+
+Perhaps the biggest action you can do to help GNU Guix grow as a project
+is to review the work contributed by others.  You do not need to be a
+committer to do so; applying, reading the source, building, linting and
+running other people's series and sharing your comments about your
+experience will give some confidence to committers.  Basically, you gmust
+ensure the check list found in the @ref{Submitting Patches} section has
+been correctly followed.  A reviewed patch series should give the best
+chances for the proposed change to be merged faster, so if a change you
+would like to see merged hasn't yet been reviewed, this is the most
+appropriate thing to do!
+
+@cindex reviewing, guidelines
+Review comments should be unambiguous; be as clear and explicit as you
+can about what you think should be changed, ensuring the author can take
+action on it.  Please try to keep the following guidelines in mind
+during review:
+
+@enumerate
+@item
+@emph{Be clear and explicit about changes you are suggesting}, ensuring
+the author can take action on it.  In particular, it is a good idea to
+explicitly ask for new revisions when you want it.
+
+@item
+@emph{Remain focused: do not change the scope of the work being
+reviewed.}  For example, if the contribution touches code that follows a
+pattern deemed unwieldy, it would be unfair to ask the submitter to fix
+all occurrences of that pattern in the code; to put it simply, if a
+problem unrelated to the patch at hand was already there, do not ask the
+submitter to fix it.
+
+@item
+@emph{Ensure progress.}  As they respond to review, submitters may
+submit new revisions of their changes; avoid requesting changes that you
+did not request in the previous round of comments.  Overall, the
+submitter should get a clear sense of progress; the number of items open
+for discussion should clearly decrease over time.
+
+@item
+@emph{Aim for finalization.}  Reviewing code is time-consuming.  Your
+goal as a reviewer is to put the process on a clear path towards
+integration, possibly with agreed-upon changes, or rejection, with a
+clear and mutually-understood reasoning.  Avoid leaving the review
+process in a lingering state with no clear way out.
+
+@item
+@emph{Review is a discussion.}  The submitter's and reviewer's views on
+how to achieve a particular change may not always be aligned.  To lead
+the discussion, remain focused, ensure progress and aim for
+finalization, spending time proportional to the stakes <at> footnote{The
+tendency to discuss minute details at length is often referred to as
+``bikeshedding'', where much time is spent discussing each one's
+preference for the color of the shed at the expense of progress made on
+the project to keep bikes dry.}.  As a reviewer, try hard to explain the
+rationale for suggestions you make, and to understand and take into
+account the submitter's motivation for doing things in a certain way.
+@end enumerate
+
+@cindex LGTM, Looks Good To Me
+@cindex review tags
+@cindex Reviewed-by, git trailer
+When you deem the proposed change adequate and ready for inclusion
+within Guix, the following well understood/codified
+@samp{Reviewed-by:@tie{}Your <at> tie{}Name<your-email@@example.com>}
+@footnote{The @samp{Reviewed-by} Git trailer is used by other projects
+such as Linux, and is understood by third-party tools such as the
+@samp{b4 am} sub-command, which is able to retrieve the complete
+submission email thread from a public-inbox instance and add the Git
+trailers found in replies to the commit patches.} line should be used to
+sign off as a reviewer, meaning you have reviewed the change and that it
+looks good to you:
+
+@itemize
+@item
+If the @emph{whole} series (containing multiple commits) looks good to
+you, reply with @samp{Reviewed-by:@tie{}Your <at> tie{}Name<your-email@@example.com>}
+to the cover page if it has one, or to the last patch of the series
+otherwise, adding another @samp{(for the whole series)} comment on the
+line below to explicit this fact.
+
+@item
+If you instead want to mark a @emph{single commit} as reviewed (but not
+the whole series), simply reply with
+@samp{Reviewed-by:@tie{}Your <at> tie{}Name<your-email@@example.com>} to that
+commit message.
+@end itemize
+
+If you are not a committer, you can help others find a @emph{series} you
+have reviewed more easily by adding a @code{reviewed-looks-good} usertag
+for the @code{guix} user (@pxref{Debbugs Usertags}).
+
 @node Updating the Guix Package
 @section Updating the Guix Package
 
diff --git a/etc/git/gitconfig b/etc/git/gitconfig
index 907ad01804..654a630b18 100644
--- a/etc/git/gitconfig
+++ b/etc/git/gitconfig
@@ -16,3 +16,10 @@
         to = guix-patches <at> gnu.org
         headerCmd = etc/teams.scm cc-members-header-cmd
         thread = no
+
+[b4]
+        attestation-check-dkim = off
+        attestation-policy = off
+        linkmask = https://yhetil.org/guix/%s
+        linktrailermask = https://yhetil.org/guix/%s
+        midmask = https://yhetil.org/guix/%s

base-commit: a96f1c1bc0fa186414359890025e8acacbb1de02
-- 
2.41.0

Message #86 received at 66436 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 66436 <at> debbugs.gnu.org, dev <at> jpoiret.xyz, zimon.toutoune <at> gmail.com
Subject: Re: [PATCH v3] doc: Add some guidelines for reviewing.
Date: Sun, 05 Nov 2023 15:51:32 +0100

Hello,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> * doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
> section.
> (Debbugs Usertags): Expound with Emacs Debbugs information and document the
> 'reviewed-looks-good' usertag.
>
> Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
> ---
>
> Changes in v3:
>  - Replace LGTM with Reviewed-by Git tag
>  - Add b4 config
>  - Link to the Submitting Patches section for check list
>  - Fuse further suggestions by both Ludovic and Simon

Could you mention the b4 change in the commit log?  Otherwise LGTM!

> +[b4]
> +        attestation-check-dkim = off
> +        attestation-policy = off
> +        linkmask = https://yhetil.org/guix/%s
> +        linktrailermask = https://yhetil.org/guix/%s
> +        midmask = https://yhetil.org/guix/%s

Really cool to have the b4 workflow documented and a default config in
place!

Ludo’.

Message #91 received at 66436-done <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 66436-done <at> debbugs.gnu.org, dev <at> jpoiret.xyz, zimon.toutoune <at> gmail.com
Subject: Re: [PATCH v3] doc: Add some guidelines for reviewing.
Date: Tue, 07 Nov 2023 11:14:00 -0500

Hello,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hello,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> * doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
>> section.
>> (Debbugs Usertags): Expound with Emacs Debbugs information and document the
>> 'reviewed-looks-good' usertag.
>>
>> Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
>> ---
>>
>> Changes in v3:
>>  - Replace LGTM with Reviewed-by Git tag
>>  - Add b4 config
>>  - Link to the Submitting Patches section for check list
>>  - Fuse further suggestions by both Ludovic and Simon
>
> Could you mention the b4 change in the commit log?  Otherwise LGTM!

Done, thanks for the review.

>> +[b4]
>> +        attestation-check-dkim = off
>> +        attestation-policy = off
>> +        linkmask = https://yhetil.org/guix/%s
>> +        linktrailermask = https://yhetil.org/guix/%s
>> +        midmask = https://yhetil.org/guix/%s
>
> Really cool to have the b4 workflow documented and a default config in
> place!

Now we could document more extensively some successful workflows in the
Cookbook (Debbugs/Gnus-based, QA-based, b4-based, etc.) :-).

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.