[PATCH] doc: Add some guidelines for reviewing.

  • Done
  • quality assurance status badge
Details
5 participants
  • Clément Lassieur
  • Josselin Poiret
  • Ludovic Courtès
  • Maxim Cournoyer
  • Simon Tournier
Owner
unassigned
Submitted by
Maxim Cournoyer
Severity
normal
M
M
Maxim Cournoyer wrote on 10 Oct 2023 14:54
19f82d9bbef649c750ad067d23ebbaee6f9ae494.1696942467.git.maxim.cournoyer@gmail.com
* 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(-)

Toggle diff (103 lines)
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@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
L
L
Ludovic Courtès wrote on 10 Oct 2023 15:29
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 66436@debbugs.gnu.org)
87r0m2wqpq.fsf@gnu.org
Hi,

This looks great to me!

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

Toggle quote (6 lines)
> +@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’.
M
M
Maxim Cournoyer wrote on 11 Oct 2023 02:24
[PATCH v2] doc: Add some guidelines for reviewing.
(address . 66436@debbugs.gnu.org)
0a5e027af3c0b61f9ab9f3c66e73b7a769eef7fd.1696983695.git.maxim.cournoyer@gmail.com
* 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@gnu.org
Series-to: 66436@debbugs.gnu.org
Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
Co-authored-by: Ludovic Courtès <ludo@gnu.org>
---
v2: integrate guidelines suggested by Ludovic

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

Toggle diff (143 lines)
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@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
M
M
Maxim Cournoyer wrote on 12 Oct 2023 04:48
[PATCH 0/2] Add support for Git Large File Storage (LFS).
(address . 66436@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
cover.1697078526.git.maxim.cournoyer@gmail.com
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
M
M
Maxim Cournoyer wrote on 12 Oct 2023 04:48
[PATCH 1/2] gnu: git-lfs: Patch /bin/sh references.
(address . 66436@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
2343e018a06ef7e1ce7d9d7cc2e6d30c76027277.1697078865.git.maxim.cournoyer@gmail.com
* 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(+)

Toggle diff (18 lines)
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
M
M
Maxim Cournoyer wrote on 12 Oct 2023 04:48
[PATCH 2/2] git-download: Add support for Git Large File Storage (LFS).
(address . 66436@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
58269ccea06b016c38d3bea8678608c8cccce1ca.1697078865.git.maxim.cournoyer@gmail.com
* 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(-)

Toggle diff (347 lines)
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
M
M
Maxim Cournoyer wrote on 12 Oct 2023 04:51
Re: [bug#66436] [PATCH 0/2] Add support for Git Large File Storage (LFS).
(address . 66436@debbugs.gnu.org)
878r88d03f.fsf@gmail.com
Hello,

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

Toggle quote (14 lines)
> 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
L
L
Ludovic Courtès wrote on 14 Oct 2023 19:04
Re: [bug#66436] [PATCH 1/2] gnu: git-lfs: Patch /bin/sh references.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 66436@debbugs.gnu.org)
87h6mtdtjn.fsf@gnu.org
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

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

LGTM!
L
L
Ludovic Courtès wrote on 14 Oct 2023 19:12
Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large File Storage (LFS).
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
877cnpdt6d.fsf@gnu.org
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (21 lines)
> * 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…

Toggle quote (8 lines)
> +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.

Toggle quote (10 lines)
> +(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’.
J
J
Josselin Poiret wrote on 15 Oct 2023 11:55
Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
87jzroyzuk.fsf@jpoiret.xyz
Hi Maxim,

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

Toggle quote (18 lines)
> +@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

Best,
--
Josselin Poiret
-----BEGIN PGP SIGNATURE-----

iQHEBAEBCgAuFiEEOSSM2EHGPMM23K8vUF5AuRYXGooFAmUrtwMQHGRldkBqcG9p
cmV0Lnh5egAKCRBQXkC5FhcaiqBCC/4zDX0BupR681wVirKoJuOctDso8gn+obJQ
7T6ftm50NNzywQ1cwAb6cHlVptX2FpyKOcCPq1MhhyAuw2BgqCP0OeciiJWBcDO4
NE5jqjYovr47Au02v7Fez1PJ3hsJF4a/pSwvkGsHonjv3w46rT1NJla2TKw74ihP
vYpBza6eKaso3XNMDMs+ur2D2EJ0UpAFdJurGpHtZ6pJgobrpRjnLpu9TI//FEBr
V3Ijb0+alwMCzSApnGO/8xeO2VzUS7xOwyibt2NOBA9vH+05GNo53koU24kL3SGd
9NMhRCvJHRUod7MIBZfgsCFjuZF6G4fQ+hr1+BBoU8RSuO+PZTmStARfAf16OlPY
JWN5FpPc3tPCmYYM72hT9Zc0a5Ymfp/mVsFDx6Q7R/epB1u8vEGN+7n5iCGGwhoi
9t8Ld0Tya3Si2dn1qglcuHsN+c6a6s5JYf2nk0pdLTzX1tBFVRCnQhmz9y7jKyHI
u8qrlUL0e5lHjv7v0hNwEbua/4+RNTU=
=8Qxd
-----END PGP SIGNATURE-----

M
M
Maxim Cournoyer wrote on 16 Oct 2023 16:02
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
87fs2azmvv.fsf@gmail.com
Hello,

Josselin Poiret <dev@jpoiret.xyz> writes:

Toggle quote (29 lines)
> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@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
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
M
M
Maxim Cournoyer wrote on 16 Oct 2023 16:46
Re: [bug#66436] [PATCH 1/2] gnu: git-lfs: Patch /bin/sh references.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 66436@debbugs.gnu.org)
87a5sizktz.fsf@gmail.com
Hi,

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

Toggle quote (7 lines)
> Maxim Cournoyer <maxim.cournoyer@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
M
M
Maxim Cournoyer wrote on 16 Oct 2023 17:10
Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large File Storage (LFS).
(name . Ludovic Courtès)(address . ludo@gnu.org)
87sf6ay55z.fsf@gmail.com
Hi Ludovic,

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

Toggle quote (28 lines)
> Maxim Cournoyer <maxim.cournoyer@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.

Toggle quote (15 lines)
> 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.

Toggle quote (26 lines)
> 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
S
S
Simon Tournier wrote on 16 Oct 2023 17:15
87fs2ar42s.fsf@gmail.com
Hi,

I reorder Ludo’s answers for clarity.

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

Toggle quote (3 lines)
> 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.

Toggle quote (4 lines)
> 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
M
M
Maxim Cournoyer wrote on 16 Oct 2023 18:23
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
87fs2awn6r.fsf@gmail.com
Hi,

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

Toggle quote (22 lines)
> Hi,
>
> I reorder Ludo’s answers for clarity.
>
> On Sat, 14 Oct 2023 at 19:12, Ludovic Courtès <ludo@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
C
C
Clément Lassieur wrote on 20 Oct 2023 10:12
Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87h6mlzp9g.fsf@lassieur.org
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
M
M
Maxim Cournoyer wrote on 21 Oct 2023 01:01
(name . Clément Lassieur)(address . clement@lassieur.org)
87r0logap6.fsf@gmail.com
Hi,

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

Toggle quote (16 lines)
> 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.

Toggle quote (10 lines)
> 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.

Toggle quote (4 lines)
> 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:

Toggle snippet (8 lines)
+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").

--
Thanks,
Maxim
C
C
Clément Lassieur wrote on 22 Oct 2023 22:03
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87il6ybf27.fsf@lassieur.org
Hi Maxim,

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

Toggle quote (17 lines)
>> 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.

Toggle quote (19 lines)
>> 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
M
M
Maxim Cournoyer wrote on 23 Oct 2023 03:55
(name . Clément Lassieur)(address . clement@lassieur.org)
8734y29k5x.fsf@gmail.com
Hi,

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

Toggle quote (24 lines)
> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@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

--
Thanks,
Maxim
S
S
Simon Tournier wrote on 24 Oct 2023 10:49
86il6wv200.fsf@gmail.com
Hi,

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

Toggle quote (15 lines)
>> 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
S
S
Simon Tournier wrote on 24 Oct 2023 10:59
86bkcov1j2.fsf@gmail.com
Re,

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

Toggle quote (9 lines)
>> --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---

[...]

Toggle quote (9 lines)
> 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
L
L
Ludovic Courtès wrote on 24 Oct 2023 17:54
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 66436@debbugs.gnu.org)
87a5s86mos.fsf@gnu.org
Hi,

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

Toggle quote (3 lines)
> * 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.

Toggle quote (3 lines)
> +@node Reviewing the Work of Others
> +@section Reviewing the Work of Others

[...]

Toggle quote (6 lines)
> +@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:

[...]

Toggle quote (8 lines)
> +@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@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’.
S
S
Simon Tournier wrote on 25 Oct 2023 15:53
(address . 66436@debbugs.gnu.org)
87o7gmhkqn.fsf@gmail.com
Hi

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

Toggle quote (8 lines)
>> +@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…

Toggle quote (2 lines)
> Another item came to mind, that could be phrased like this:

…while this new is unclear…

Toggle quote (11 lines)
> 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@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:

Toggle snippet (13 lines)
@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@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.

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
J
J
Josselin Poiret wrote on 29 Oct 2023 15:52
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
878r7l4h22.fsf@jpoiret.xyz
Hi Maxim,

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

Toggle quote (2 lines)
> Do you use such a tool when applying patches working with Guix review?

Yes, it's very useful.

Toggle quote (4 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iQHEBAEBCgAuFiEEOSSM2EHGPMM23K8vUF5AuRYXGooFAmU+caUQHGRldkBqcG9p
cmV0Lnh5egAKCRBQXkC5FhcailN7C/9JkUFR8VDrWk5Fn+o4wmiRpJUBHVl1Zj7D
NoB2OvcgUXm7z3tXWHcwKJUPdXzScQiwt6FfolomAtbxWwqrdYLCn4LwWkGuadxc
U0yiMFydcREFCvlynkuYpXbCN/+gkKVWrFU6YY+QKFanxxiq5Tk4mF9oivrooQ0H
mSQTDzgWkC/PmtLrTvTqkUpfxf9JRwpdl0vqxkPwbVr3W+5ltmlnraA4oWqLKtsU
v3mSM3X8VK0A+MeGKjlg4pZ0VFirPkSo9WDSOei1uymOE5ukantDEVjDXjlmuJ2q
K7TOGixajvRvL6+vXYSdC8VOXSRmwE2xqxmhguduH4FQtRQf1lvZY1VaNEpwmpwQ
4mKajByo7frie2IJ+8o6hdYsSFgW93mpNBoXuPlIFEBp5YYFMPmwL4gTbKzQ/FnD
+8nFJBrjwunLmZrku35YB2+csUp7bhSmEZxGdvYdQz9xdwjVIw9L5aAmKlzxpXtV
Od7Hzs5AQue5JrnXhHLxRS8HRS9WWj0=
=u3NA
-----END PGP SIGNATURE-----

M
M
Maxim Cournoyer wrote on 31 Oct 2023 19:53
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
87jzr2pqrq.fsf@gmail.com
Hi Simon,

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

Toggle quote (28 lines)
> Re,
>
> On Tue, 24 Oct 2023 at 10:49, Simon Tournier <zimon.toutoune@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
S
S
Simon Tournier wrote on 31 Oct 2023 20:03
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
CAJ3okZ0Zn2kCrtqA5FEKP6eczQmYo+K67hJEqSs83fpj=SN_7w@mail.gmail.com
Hi Maxim,

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

Toggle quote (2 lines)
> >> IMHO, it is worth to clearly state:

[...]

Toggle quote (2 lines)
> That's already mentioned in 'Submitting Patches':

[...]

Toggle quote (3 lines)
> 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
M
M
Maxim Cournoyer wrote on 1 Nov 2023 20:23
[PATCH v3] doc: Add some guidelines for reviewing.
(address . 66436@debbugs.gnu.org)
ee8182cae8ec18eedbef416483701e60da123e6a.1698866603.git.maxim.cournoyer@gmail.com
* 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(-)

Toggle diff (169 lines)
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@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@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@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@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@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@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
L
L
Ludovic Courtès wrote on 5 Nov 2023 15:51
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87fs1k1cej.fsf@gnu.org
Hello,

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

Toggle quote (14 lines)
> * 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!

Toggle quote (7 lines)
> +[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’.
M
M
Maxim Cournoyer wrote on 7 Nov 2023 17:14
(name . Ludovic Courtès)(address . ludo@gnu.org)
87edh1k0c7.fsf@gmail.com
Hello,

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

Toggle quote (20 lines)
> Hello,
>
> Maxim Cournoyer <maxim.cournoyer@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.

Toggle quote (10 lines)
>> +[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
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 66436@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 66436
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch