* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Patrick Steinhardt @ 2023-11-10 10:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqq8r76zg1j.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]
On Fri, Nov 10, 2023 at 08:02:32AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
> >> process_subtree_split_trailer () {
> >> - assert test $# = 2 -o $# = 3
> >> + assert test $# -ge 2
> >> + assert test $# -le 3
> >
> > It took me a minute to figure out why we were swapping "=" for "-ge". It
> > is because we want to logical-OR the two conditions, but "assert"
> > requires that we test one at a time. I think that is probably worth
> > explaining in the commit message.
>
> I wish we could write something like
>
> assert test $# -ge 2 && test $# -le 3
>
> (and I'd allow double quoting the whole thing after assert if
> needed) but we cannot do so without tweaking the implementation of
> assert.
>
> >
> >> @@ -916,7 +919,7 @@ cmd_split () {
> >> if test $# -eq 0
> >> then
> >> rev=$(git rev-parse HEAD)
> >> - elif test $# -eq 1 -o $# -eq 2
> >> + elif test $# -eq 1 || test $# -eq 2
> >
> > OK, this one is a straight-forward use of "||".
>
> Yes, but why not consistently use the range notation like the
> earlier one here, or below?
I opted to go for the "obvious" conversion, if there was one easily
available, to make the diff easier to read. We could of course use a
range notation like this:
rev=$(git rev-parse HEAD)
- elif test $# -eq 1 || test $# -eq 2
+ elif test $# -ge 1 && test $# -le 2
then
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
Or :
rev=$(git rev-parse HEAD)
- elif test $# -eq 1 || test $# -eq 2
+ elif ! { test $# -lt 1 || test $# -gt 2; }
then
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
But both of these are not consistent with the other cases due to the
negation here, and both of them are harder to read in my opinion. So
I'm not sure whether we gain anything by trying to make this a bit more
consistent with the other conversions.
Patrick
> elif test $# -ge 1 && test $# -le 2
>
> >> cmd_merge () {
> >> - test $# -eq 1 -o $# -eq 2 ||
> >> + if test $# -lt 1 || test $# -gt 2
> >> ...
> > (I am OK with either, it just took me a minute to verify that your
> > conversion was correct. But that is a one-time issue now while
> > reviewing, and I think the code is readable going forward).
>
> Yeah, the end result looks good.
>
> Thanks, both.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Patrick Steinhardt @ 2023-11-10 10:18 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20231109185515.GD2711684@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2282 bytes --]
On Thu, Nov 09, 2023 at 01:55:15PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:
>
> > Functions in git-subtree.sh all assert that they are being passed the
> > correct number of arguments. In cases where we accept a variable number
> > of arguments we assert this via a single call to `test` with `-o`, which
> > is discouraged by our coding guidelines.
> >
> > Convert these cases to stop doing so.
>
> OK. I think these ones really are safe, because they're only expanding
> $#, but I agree with the principle to follow the guidelines.
>
> > # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
> > process_subtree_split_trailer () {
> > - assert test $# = 2 -o $# = 3
> > + assert test $# -ge 2
> > + assert test $# -le 3
>
> It took me a minute to figure out why we were swapping "=" for "-ge". It
> is because we want to logical-OR the two conditions, but "assert"
> requires that we test one at a time. I think that is probably worth
> explaining in the commit message.
I really hate to admit how long I've pondered over this patch series in
total, up to the point where I did a `git rebase --reset-author-date` at
the end just so that it's not obvious. So I totally get everyone who
needs to stop and think for a bit here.
Will adapt the commit message.
Patrick
> > @@ -916,7 +919,7 @@ cmd_split () {
> > if test $# -eq 0
> > then
> > rev=$(git rev-parse HEAD)
> > - elif test $# -eq 1 -o $# -eq 2
> > + elif test $# -eq 1 || test $# -eq 2
>
> OK, this one is a straight-forward use of "||".
>
> > cmd_merge () {
> > - test $# -eq 1 -o $# -eq 2 ||
> > + if test $# -lt 1 || test $# -gt 2
> > + then
> > die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
> > + fi
> > +
>
> But here we swap "-eq" for other operators. We have to because we went
> from "||" to an "if". I think what you have here is correct, but you
> could also write:
>
> if ! { test $# -eq 1 || test $# -eq 2; }
>
> (I am OK with either, it just took me a minute to verify that your
> conversion was correct. But that is a one-time issue now while
> reviewing, and I think the code is readable going forward).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Patrick Steinhardt @ 2023-11-10 10:18 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20231109184843.GC2711684@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]
On Thu, Nov 09, 2023 at 01:48:43PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:
>
> > > -elif test -d ${GIT_DIR:-.git} -o -f .git &&
> > > +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
> >
> > I do not think this is strictly necessary.
> >
> > Because the command line parser of "test" comes from left, notices
> > "-d" and takes the next one to check if it is a directory. There is
> > no value in ${GIT_DIR} can make "test -d ${GIT_DIR} -o ..." fail the
> > same way as the problem Peff pointed out during the discussion.
>
> I think this is one of the ambiguous cases. If $GIT_DIR is "=", then
> "test" cannot tell if you meant:
>
> var1=-d
> var2=-o
> test "$var1" = "$var2" ...
>
> or:
>
> var1="="
> test -d "$var1" -o ...
>
> With bash, for example:
>
> $ test -d /tmp -o -f .git; echo $?
> 0
> $ test -d = -o -f .git; echo $?
> bash: test: syntax error: `-f' unexpected
> 2
>
> Without "-o", it uses the number of arguments to disambiguate (though of
> course the lack of quotes around $GIT_DIR is another potential problem
> here).
Right, let me fix the missing quoting while at it.
> And I think the same is true of the other cases below using "-z", "-n",
> and so on.
>
> But IMHO it is worth getting rid of all -o/-a regardless. Even
> non-ambiguous cases make reasoning about the code harder, and we don't
> want to encourage people to think they're OK to use.
Agreed. I'll amend the commit message to say so.
> > I do not need a subshell for grouping, either. Plain {} should do
> > (but you may need a LF or semicolon after the statement)..
>
> This I definitely agree with. :)
Will fix.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 4/4] Makefile: stop using `test -o` when unlinking duplicate executables
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699609940.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]
When building executables we may end up with both `foo` and `foo.exe` in
the project's root directory. This can cause issues on Cygwin, which is
why we unlink the `foo` binary (see 6fc301bbf68 (Makefile: remove $foo
when $foo.exe is built/installed., 2007-01-10)). This step is skipped if
either:
- `foo` is a directory, which can happen when building Git on
Windows via MSVC (see ade2ca0ca9f (Do not try to remove
directories when removing old links, 2009-10-27)).
- `foo` is a hardlink to `foo.exe`, which can happen on Cygwin (see
0d768f7c8f1 (Makefile: building git in cygwin 1.7.0, 2008-08-15)).
These two conditions are currently chained together via `test -o`, which
is discouraged by our code style guide. Convert the recipe to instead
use an `if` statement with `&&`'d conditions, which both matches our
style guide and is easier to ready.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 03adcb5a480..1094a557711 100644
--- a/Makefile
+++ b/Makefile
@@ -2342,7 +2342,7 @@ profile-fast: profile-clean
all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
ifneq (,$X)
- $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
+ $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), if test ! -d '$p' && test ! '$p' -ef '$p$X'; then $(RM) '$p'; fi;)
endif
all::
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/4] contrib/subtree: convert subtree type check to use case statement
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699609940.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]
The `subtree_for_commit ()` helper function asserts that the subtree
identified by its parameters are either a commit or tree. This is done
via the `-o` parameter of test, which is discouraged.
Refactor the code to instead use a switch statement over the type.
Despite being aligned with our coding guidelines, the resulting code is
arguably also easier to read.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/subtree/git-subtree.sh | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8af0a81ba3f..3028029ac2d 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -641,10 +641,16 @@ subtree_for_commit () {
while read mode type tree name
do
assert test "$name" = "$dir"
- assert test "$type" = "tree" -o "$type" = "commit"
- test "$type" = "commit" && continue # ignore submodules
- echo $tree
- break
+
+ case "$type" in
+ commit)
+ continue;; # ignore submodules
+ tree)
+ echo $tree
+ break;;
+ *)
+ die "fatal: tree entry is of type ${type}, expected tree or commit";;
+ esac
done || exit $?
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699609940.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]
Functions in git-subtree.sh all assert that they are being passed the
correct number of arguments. In cases where we accept a variable number
of arguments we assert this via a single call to `test` with `-o`, which
is discouraged by our coding guidelines.
Convert these cases to stop doing so. This requires us to decompose
assertions of the style `assert test $# = 2 -o $# = 3` into two calls
because we have no easy way to logically chain statements passed to the
assert function.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/subtree/git-subtree.sh | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 43b5fec7320..8af0a81ba3f 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -373,7 +373,8 @@ try_remove_previous () {
# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
process_subtree_split_trailer () {
- assert test $# = 2 -o $# = 3
+ assert test $# -ge 2
+ assert test $# -le 3
b="$1"
sq="$2"
repository=""
@@ -402,7 +403,8 @@ process_subtree_split_trailer () {
# Usage: find_latest_squash DIR [REPOSITORY]
find_latest_squash () {
- assert test $# = 1 -o $# = 2
+ assert test $# -ge 1
+ assert test $# -le 2
dir="$1"
repository=""
if test "$#" = 2
@@ -455,7 +457,8 @@ find_latest_squash () {
# Usage: find_existing_splits DIR REV [REPOSITORY]
find_existing_splits () {
- assert test $# = 2 -o $# = 3
+ assert test $# -ge 2
+ assert test $# -le 3
debug "Looking for prior splits..."
local indent=$(($indent + 1))
@@ -916,7 +919,7 @@ cmd_split () {
if test $# -eq 0
then
rev=$(git rev-parse HEAD)
- elif test $# -eq 1 -o $# -eq 2
+ elif test $# -eq 1 || test $# -eq 2
then
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
@@ -1006,8 +1009,11 @@ cmd_split () {
# Usage: cmd_merge REV [REPOSITORY]
cmd_merge () {
- test $# -eq 1 -o $# -eq 2 ||
+ if test $# -lt 1 || test $# -gt 2
+ then
die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
+ fi
+
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
repository=""
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699609940.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4513 bytes --]
Our coding guidelines say to not use `test` with `-a` and `-o` because
it can easily lead to bugs. Convert trivial cases where we still use
these to instead instead concatenate multiple invocations of `test` via
`&&` and `||`, respectively.
While not all of the converted instances can cause ambiguity, it is
worth getting rid of all of them regardless:
- It becomes easier to reason about the code as we do not have to
argue why one use of `-a`/`-o` is okay while another one isn't.
- We don't encourage people to use these expressions.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
GIT-VERSION-GEN | 2 +-
configure.ac | 2 +-
contrib/subtree/git-subtree.sh | 4 ++--
t/perf/perf-lib.sh | 2 +-
t/perf/run | 9 +++++----
t/valgrind/valgrind.sh | 2 +-
6 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index e54492f8271..7246ab7c78c 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -11,7 +11,7 @@ LF='
if test -f version
then
VN=$(cat version) || VN="$DEF_VER"
-elif test -d ${GIT_DIR:-.git} -o -f .git &&
+elif { test -d "${GIT_DIR:-.git}" || test -f .git; } &&
VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
case "$VN" in
*$LF*) (exit 1) ;;
diff --git a/configure.ac b/configure.ac
index 276593cd9dd..d1a96da14eb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],
[AC_ARG_WITH([$1],
[AS_HELP_STRING([--with-$1=VALUE], $3)],
if test -n "$withval"; then
- if test "$withval" = "yes" -o "$withval" = "no"; then
+ if test "$withval" = "yes" || test "$withval" = "no"; then
AC_MSG_WARN([You likely do not want either 'yes' or 'no' as]
[a value for $1 ($2). Maybe you do...?])
fi
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index e0c5d3b0de6..43b5fec7320 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -489,13 +489,13 @@ find_existing_splits () {
;;
END)
debug "Main is: '$main'"
- if test -z "$main" -a -n "$sub"
+ if test -z "$main" && test -n "$sub"
then
# squash commits refer to a subtree
debug " Squash: $sq from $sub"
cache_set "$sq" "$sub"
fi
- if test -n "$main" -a -n "$sub"
+ if test -n "$main" && test -n "$sub"
then
debug " Prior: $main -> $sub"
cache_set $main $sub
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index e7786775a90..b952e5024b4 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM
GIT_CONFIG_SYSTEM="$TEST_DIRECTORY/perf/config"
export GIT_CONFIG_SYSTEM
-if test -n "$GIT_TEST_INSTALLED" -a -z "$PERF_SET_GIT_TEST_INSTALLED"
+if test -n "$GIT_TEST_INSTALLED" && test -z "$PERF_SET_GIT_TEST_INSTALLED"
then
error "Do not use GIT_TEST_INSTALLED with the perf tests.
diff --git a/t/perf/run b/t/perf/run
index 34115edec35..486ead21980 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -91,10 +91,10 @@ set_git_test_installed () {
run_dirs_helper () {
mydir=${1%/}
shift
- while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
shift
done
- if test $# -gt 0 -a "$1" = --; then
+ if test $# -gt 0 && test "$1" = --; then
shift
fi
@@ -124,7 +124,7 @@ run_dirs_helper () {
}
run_dirs () {
- while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
run_dirs_helper "$@"
shift
done
@@ -180,7 +180,8 @@ run_subsection () {
GIT_PERF_AGGREGATING_LATER=t
export GIT_PERF_AGGREGATING_LATER
- if test $# = 0 -o "$1" = -- -o -f "$1"; then
+ if test $# = 0 || test "$1" = -- || test -f "$1"
+ then
set -- . "$@"
fi
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 669ebaf68be..9fbf90cee7c 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -23,7 +23,7 @@ memcheck)
VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
test 3 -gt "$VALGRIND_MAJOR" ||
- test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
+ ( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||
TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
;;
*)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>`
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]
Hi,
this is the second version of my patch series that replaces all uses of
`test <expr> -o/a <expr`.
Changes compared to v1:
- I've expanded a bit on why we want to do these conversions in the
first place in the first commit message.
- Dropped a needless subshell and added missing quoting while at it.
- Explained why we need to decompose the asserts in the second patch
into two asserts.
Thanks!
Patrick
Patrick Steinhardt (4):
global: convert trivial usages of `test <expr> -a/-o <expr>`
contrib/subtree: stop using `-o` to test for number of args
contrib/subtree: convert subtree type check to use case statement
Makefile: stop using `test -o` when unlinking duplicate executables
GIT-VERSION-GEN | 2 +-
Makefile | 2 +-
configure.ac | 2 +-
contrib/subtree/git-subtree.sh | 34 +++++++++++++++++++++++-----------
t/perf/perf-lib.sh | 2 +-
t/perf/run | 9 +++++----
t/valgrind/valgrind.sh | 2 +-
7 files changed, 33 insertions(+), 20 deletions(-)
Range-diff against v1:
1: c5e627eb3fe ! 1: 2967c8ebb46 global: convert trivial usages of `test <expr> -a/-o <expr>`
@@ Commit message
these to instead instead concatenate multiple invocations of `test` via
`&&` and `||`, respectively.
+ While not all of the converted instances can cause ambiguity, it is
+ worth getting rid of all of them regardless:
+
+ - It becomes easier to reason about the code as we do not have to
+ argue why one use of `-a`/`-o` is okay while another one isn't.
+
+ - We don't encourage people to use these expressions.
+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## GIT-VERSION-GEN ##
@@ GIT-VERSION-GEN: LF='
then
VN=$(cat version) || VN="$DEF_VER"
-elif test -d ${GIT_DIR:-.git} -o -f .git &&
-+elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
++elif { test -d "${GIT_DIR:-.git}" || test -f .git; } &&
VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
case "$VN" in
*$LF*) (exit 1) ;;
2: b1ea45b8a88 ! 2: 977132d2236 contrib/subtree: stop using `-o` to test for number of args
@@ Commit message
of arguments we assert this via a single call to `test` with `-o`, which
is discouraged by our coding guidelines.
- Convert these cases to stop doing so.
+ Convert these cases to stop doing so. This requires us to decompose
+ assertions of the style `assert test $# = 2 -o $# = 3` into two calls
+ because we have no easy way to logically chain statements passed to the
+ assert function.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
3: 7c54d9070fa = 3: 761cde1b341 contrib/subtree: convert subtree type check to use case statement
4: bc9489ca5b8 = 4: 5326d86888a Makefile: stop using `test -o` when unlinking duplicate executables
base-commit: dadef801b365989099a9929e995589e455c51fed
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] glossary: add definitions for dereference & peel
From: Kristoffer Haugsbakk @ 2023-11-10 8:28 UTC (permalink / raw)
To: Josh Soref; +Cc: Patrick Steinhardt, Victoria Dye, git
In-Reply-To: <pull.1610.git.1699574277143.gitgitgadget@gmail.com>
On Fri, Nov 10, 2023, at 00:57, Victoria Dye via GitGitGadget wrote:
> +[[def_peel]]peel::
> + Synonym for object <<def_dereference,dereference>>. Most commonly used
> + in the context of tags, where it refers to the process of recursively
> + dereferencing a <<def_tag_object,tag object>> until the result object's
> + <<def_object_type,type>> is something other than "tag".
As a user I like that this is classified as a synonym. Because if I wanted
to ask StackOverflow about how to get to the commit that a tag points to
then I would use the term “dereference a tag”.
--
Kristoffer Haugsbakk
^ permalink raw reply
* [PATCH v4 3/3] t9164: fix inability to find basename(1) in Subversion hooks
From: Patrick Steinhardt @ 2023-11-10 8:17 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699596457.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]
Hooks executed by Subversion are spawned with an empty environment. By
default, not even variables like PATH will be propagated to them. In
order to ensure that we're still able to find required executables, we
thus write the current PATH variable into the hook script itself and
then re-export it in t9164.
This happens too late in the script though, as we already tried to
execute the basename(1) utility before exporting the PATH variable. This
tends to work on most platforms as the fallback value of PATH for Bash
(see `getconf PATH`) is likely to contain this binary. But on more
exotic platforms like NixOS this is not the case, and thus the test
fails.
While we could work around this issue by simply setting PATH earlier, it
feels fragile to inject a user-controlled value into the script and have
the shell interpret it. Instead, we can refactor the hook setup to write
a `hooks-env` file that configures PATH for us. Like this, Subversion
will know to set up the environment as expected for all hooks.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..d1dec89c3b7 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -46,6 +46,14 @@ setup_hook()
"passed to setup_hook" >&2 ; return 1; }
echo "cnt=$skip_revs" > "$hook_type-counter"
rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
+
+ # Subversion hooks run with an empty environment by default. We thus
+ # need to propagate PATH so that we can find executables.
+ cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
+ [default]
+ PATH = ${PATH}
+ EOF
+
hook="$rawsvnrepo/hooks/$hook_type"
cat > "$hook" <<- 'EOF1'
#!/bin/sh
@@ -63,7 +71,6 @@ EOF1
if [ "$hook_type" = "pre-commit" ]; then
echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
else
- echo "PATH=\"$PATH\"; export PATH" >>"$hook"
echo "svnconf=\"$svnconf\"" >>"$hook"
cat >>"$hook" <<- 'EOF2'
cd work-auto-commits.svn
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
From: Patrick Steinhardt @ 2023-11-10 8:17 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699596457.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]
When setting up httpd for our tests, we also install a passwd and
proxy-passwd file that contain the test user's credentials. These
credentials currently use crypt(3) as the password encryption schema.
This schema can be considered deprecated nowadays as it is not safe
anymore. Quoting Apache httpd's documentation [1]:
> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.
This is starting to cause issues in modern Linux distributions. glibc
has deprecated its libcrypt library that used to provide crypt(3) in
favor of the libxcrypt library. This newer replacement provides a
compile time switch to disable insecure password encryption schemata,
which causes crypt(3) to always return `EINVAL`. The end result is that
httpd tests that exercise authentication will fail on distros that use
libxcrypt without these insecure encryption schematas.
Regenerate the passwd files to instead use the default password
encryption schema, which is md5. While it feels kind of funny that an
MD5-based encryption schema should be more secure than anything else, it
is the current default and supported by all platforms. Furthermore, it
really doesn't matter all that much given that these files are only used
for testing purposes anyway.
[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-httpd/passwd | 2 +-
t/lib-httpd/proxy-passwd | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 0/3] t: improve compatibility with NixOS
From: Patrick Steinhardt @ 2023-11-10 8:16 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699428122.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3367 bytes --]
Hi,
this is the fourth version of my patch series to improve compatibility
of our test suite with NixOS.
Three changes compared to v3:
- Switched from `test <expr> -a <expr>` to `test <expr> && test
<expr>`.
- Improved the commit message to explain why the new
runtime-detected paths are only used as a fallback.
- Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
2023-11-09), which has been merged to `next` and caused conflicts.
Technically speaking this series also depends on 0763c3a2c4 (http:
update curl http/2 info matching for curl 8.3.0, 2023-09-15), without
which the tests will fail on NixOS machines with a recent libcurl.
Patrick
Patrick Steinhardt (3):
t/lib-httpd: dynamically detect httpd and modules path
t/lib-httpd: stop using legacy crypt(3) for authentication
t9164: fix inability to find basename(1) in Subversion hooks
t/lib-httpd.sh | 17 +++++++++++++----
t/lib-httpd/passwd | 2 +-
t/lib-httpd/proxy-passwd | 2 +-
t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++-
4 files changed, 23 insertions(+), 7 deletions(-)
Range-diff against v3:
1: e4c75c492dd ! 1: 41b9dada2e0 t/lib-httpd: dynamically detect httpd and modules path
@@ Commit message
- The module directory can (in many cases) be derived via the
`HTTPD_ROOT` compile-time variable.
- Amend the code to do so. The new runtime-detected paths will only be
- used in case none of the hardcoded paths are usable.
+ Amend the code to do so.
+
+ Note that the new runtime-detected paths will only be used as a fallback
+ in case none of the hardcoded paths are usable. For the PATH lookup this
+ is because httpd is typically installed into "/usr/sbin", which is often
+ not included in the user's PATH variable. And the module path detection
+ relies on a configured httpd installation and may thus not work in all
+ cases, either.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ t/lib-httpd.sh: fi
+ "$(command -v apache2)"
do
- if test -x "$DEFAULT_HTTPD_PATH"
-+ if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
++ if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
then
break
fi
@@ t/lib-httpd.sh: fi
'/usr/lib/apache2/modules' \
'/usr/lib64/httpd/modules' \
'/usr/lib/httpd/modules' \
-- '/usr/libexec/httpd'
-+ '/usr/libexec/httpd' \
+ '/usr/libexec/httpd' \
+- '/usr/lib/apache2'
++ '/usr/lib/apache2' \
+ "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
do
- if test -d "$DEFAULT_HTTPD_MODULE_PATH"
-+ if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
++ if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
break
fi
2: 3dce76da477 = 2: 7d28c32feaa t/lib-httpd: stop using legacy crypt(3) for authentication
3: 6891e254155 = 3: 6a7773aadba t9164: fix inability to find basename(1) in Subversion hooks
base-commit: 0e3b67e2aa25edb7e1a5c999c87b52a7b3a7649a
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v4 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-10 8:17 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699596457.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]
In order to set up the Apache httpd server, we need to locate both the
httpd binary and its default module path. This is done with a hardcoded
list of locations that we scan. While this works okayish with distros
that more-or-less follow the Filesystem Hierarchy Standard, it falls
apart on others like NixOS that don't.
While it is possible to specify these paths via `LIB_HTTPD_PATH` and
`LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
to figure out how to set those up. And in fact we can do better by
dynamically detecting both httpd and its module path at runtime:
- The httpd binary can be located via PATH.
- The module directory can (in many cases) be derived via the
`HTTPD_ROOT` compile-time variable.
Amend the code to do so.
Note that the new runtime-detected paths will only be used as a fallback
in case none of the hardcoded paths are usable. For the PATH lookup this
is because httpd is typically installed into "/usr/sbin", which is often
not included in the user's PATH variable. And the module path detection
relies on a configured httpd installation and may thus not work in all
cases, either.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-httpd.sh | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 9ea74927c40..f69d0da51d1 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,22 +55,31 @@ fi
HTTPD_PARA=""
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
+for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
+ '/usr/sbin/apache2' \
+ "$(command -v httpd)" \
+ "$(command -v apache2)"
do
- if test -x "$DEFAULT_HTTPD_PATH"
+ if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
then
break
fi
done
+if test -x "$DEFAULT_HTTPD_PATH"
+then
+ DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+fi
+
for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
'/usr/lib/apache2/modules' \
'/usr/lib64/httpd/modules' \
'/usr/lib/httpd/modules' \
'/usr/libexec/httpd' \
- '/usr/lib/apache2'
+ '/usr/lib/apache2' \
+ "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
do
- if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+ if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
break
fi
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [ANNOUNCE] Git v2.43.0-rc1
From: Andy Koppe @ 2023-11-10 7:59 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Linux Kernel, git-packagers
In-Reply-To: <xmqq8r785ev1.fsf@gitster.g>
On 08/11/2023 17:33, Junio C Hamano wrote:
> * "git log --format" has been taught the %(decorate) placeholder.
* "git log --format" has been taught the %(decorate) placeholder for
customizing the symbols used in ref decorations.
?
Regards,
Andy
^ permalink raw reply
* [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
From: Teng Long @ 2023-11-10 6:33 UTC (permalink / raw)
To: gitgitgadget; +Cc: chriscool, git, linusa
In-Reply-To: <0b9525db5a0787fdc71834abad9151aa03acc497.1694125210.git.gitgitgadget@gmail.com>
Linus Arver <linusa@google.com> writes:
> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 25433e1a1ff..418265f044d 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
> --------
> [verse]
> 'git interpret-trailers' [--in-place] [--trim-empty]
> - [(--trailer <token>[(=|:)<value>])...]
> + [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]
Maybe use 'key-alias' instead of 'keyAlias'? After checking the naming in some
other documents, it seems that they are basically in the former format, not
camel case format.
Thanks.
^ permalink raw reply
* Re: recover lost file
From: Junio C Hamano @ 2023-11-10 4:09 UTC (permalink / raw)
To: Malik Rumi; +Cc: git
In-Reply-To: <CAKd6oBxF8m09QnqZ46joVpcR3mrR4MRSO+kL8vELGwVhD=rgXg@mail.gmail.com>
Malik Rumi <malik.a.rumi@gmail.com> writes:
> I am looking for a lost file that was accidentally deleted about 18
> months ago. The docs I consulted are version 2.42.1. The git on my
> machine is version 2.34.1
>
> I followed the instructions on this page”
> https://git-scm.com/book/en/v2/Git-Tools-Searching , the section “Line
> Log Search”. But I got an error.
>
> malikarumi@Tetuoan2:~/Projects/hattie$ git grep titlesplit
> malikarumi@Tetuoan2:~/Projects/hattie$ git grep filesplit
> malikarumi@Tetuoan2:~/Projects/hattie$ git log -L :titlesplit
> fatal: -L argument not 'start,end:file' or ':funcname:file': :titlesplit
> malikarumi@Tetuoan2:~/Projects/hattie$ git log -L :titlesplit:
> fatal: -L argument not 'start,end:file' or ':funcname:file': :titlesplit:
>
> I don’t know the purpose of the colon. Is it a boundary marker? Does
> it belong at the front of the search object, the end, or both?
> 'start,end:file' sounds like the error message expects me to provide a
> start and an end, to which the obvious reply is - If I knew where it
> was, I wouldn’t be trying to find it.
Whatever documenttion that gave you "-L" perhaps did not give you a
good example.
$ git log -L"20,+5:helloworld.c"
$ git log -L"/^titlesplit(/,/^}/:helloworld.c"
$ git log -L":titlesplit:helloworld.c"
are ways to follow, how the range of lines in the helloworld.c file
of the CURRENTLY CHECKED OUT version, came about throughout the
history, going backwards. The way to specify the range in the
current version of helloworld.c file may be different (the examples
show "starting at line #20, for five lines", "starting at the line
that matches the pattern "^titlesplit(", and ending at the line that
matches the pattern "^}", or "the body of the 'titlesplit' function
(according to the xfuncname logic)"), but they share one important
detail that may make the feature unsuitable for your use case. You
need to exactly know where in the current version of which file the
line range you want to follow resides, but from your description I
am getting an impression that you do not even know the name of the
file.
You only said "a lost file" without giving any specifics, so it is
totally unclear to readers of your message how strings like
"titlesplit" and "filesplit" relate to what you are looking for. In
the above I randomly made a blind guess that it might be a function
name, but I may be totally off the mark.
- Do you mean "I think the file I removed had a name with either
titlesplit or filesplit in it?"
- Or do you mean "I know that the file had a definition of a
function whose name was either titlesplit or filesplit?"
- Or something completely different from the above two?
If I know all of the followings are true:
- I had the necessary contents committed in Git;
- I do not remember the filename at all, but I am sure I deleted it
and committed the deletion some time ago;
- I know the lost contents I am looking for had a string "frotz" in
it.
then I would probably try
$ git log -Sfrotz --diff-filter=D -p
which will look for all file deletions throughout the history,
limiting the output to those that had string "frotz" in them.
But again, it is unclear what useful clue you have to locate the
lost file from your description, so ...
^ permalink raw reply
* Re: recover lost file
From: Junio C Hamano @ 2023-11-10 5:27 UTC (permalink / raw)
To: Malik Rumi; +Cc: git
In-Reply-To: <xmqqmsvmw8oc.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> You only said "a lost file" without giving any specifics, so it is
> totally unclear to readers of your message how strings like
> "titlesplit" and "filesplit" relate to what you are looking for. In
> the above I randomly made a blind guess that it might be a function
> name, but I may be totally off the mark.
>
> - Do you mean "I think the file I removed had a name with either
> titlesplit or filesplit in it?"
>
> - Or do you mean "I know that the file had a definition of a
> function whose name was either titlesplit or filesplit?"
>
> - Or something completely different from the above two?
>
> If I know all of the followings are true:
>
> - I had the necessary contents committed in Git;
>
> - I do not remember the filename at all, but I am sure I deleted it
> and committed the deletion some time ago;
>
> - I know the lost contents I am looking for had a string "frotz" in
> it.
>
> then I would probably try
>
> $ git log -Sfrotz --diff-filter=D -p
>
> which will look for all file deletions throughout the history,
> limiting the output to those that had string "frotz" in them.
>
> But again, it is unclear what useful clue you have to locate the
> lost file from your description, so ...
If the scenario were
- I know the file were once committed in Git;
- I do not remember the filename, but I think its name had either
"frotz" or "nitfol" in it;
then I would try this instead:
$ git log --diff-filter=D --summary -- '*frotz*' '*nitfol*'
^ permalink raw reply
* Re: [PATCH] glossary: add definitions for dereference & peel
From: Junio C Hamano @ 2023-11-10 5:20 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, ps, Victoria Dye
In-Reply-To: <xmqq1qcyxxri.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I've never seen "peel" used for commits, though. So another
> improvement might be to say "peel" is "an act of dereferencing a
> tag" here.
I am reasonably sure I was the one who coined the term "peel", and
the picture I had in mind when I used it was to peel an onion, which
inherently was about unwrapping many levels repeatedly. I think
that is why it felt strange to see "peel" used in the context of
using a commit as a tree-ish, which (as your documentation update
clearly said) is doable only once.
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Junio C Hamano @ 2023-11-10 4:55 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King
In-Reply-To: <af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> + size_t chunk_size,
> + void *data)
> +{
> + struct pair_chunk_data *pcd = data;
> + if (chunk_size / pcd->record_size != pcd->record_nr)
> + return -1;
> + *pcd->p = chunk_start;
> + return 0;
> +}
I know one of the original places did the "divide the whole by
per-record size and see if it matches the number of records", the
same as we see above, but the check in the above could also be
if (chunk_size != st_mult(pcd->record_size, pcd->record_nr))
return -1;
which would also catch the case where chunk_size is not a multiple
of the record size. Your conversion of OOFF in midx.c loses this
protection as the original uses the multiplication-and-compare, but
the rewrite to call pair_chunk_expect would call the above and
checks with the truncating-divide-and-compare.
Does the distinction matter? I dunno. If the record/chunk
alignment is asserted elsewhere, then the distinction should not
matter, but even if it were, seeing a truncating division used in
any validation makes my skin tingle.
Other than that, the series was a pleasant read.
Thanks.
^ permalink raw reply
* recover lost file
From: Malik Rumi @ 2023-11-10 3:02 UTC (permalink / raw)
To: git
I am looking for a lost file that was accidentally deleted about 18
months ago. The docs I consulted are version 2.42.1. The git on my
machine is version 2.34.1
I followed the instructions on this page”
https://git-scm.com/book/en/v2/Git-Tools-Searching , the section “Line
Log Search”. But I got an error.
malikarumi@Tetuoan2:~/Projects/hattie$ git grep titlesplit
malikarumi@Tetuoan2:~/Projects/hattie$ git grep filesplit
malikarumi@Tetuoan2:~/Projects/hattie$ git log -L :titlesplit
fatal: -L argument not 'start,end:file' or ':funcname:file': :titlesplit
malikarumi@Tetuoan2:~/Projects/hattie$ git log -L :titlesplit:
fatal: -L argument not 'start,end:file' or ':funcname:file': :titlesplit:
I don’t know the purpose of the colon. Is it a boundary marker? Does
it belong at the front of the search object, the end, or both?
'start,end:file' sounds like the error message expects me to provide a
start and an end, to which the obvious reply is - If I knew where it
was, I wouldn’t be trying to find it.
What is the correct syntax?
Is there another search option?
Thanx
---
“None of you has faith until he loves for his brother or his neighbor
what he loves for himself.”
^ permalink raw reply
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
From: Joanna Wang @ 2023-11-10 2:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Joanna Wang
whoops no comments. thank you for the review and making the fixes!
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-10 0:51 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231109180308.GA2711684@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]
Hi Jeff,
On Thu, Nov 09, 2023 at 01:03:08PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 04:26:23PM +0100, Alejandro Colomar wrote:
>
> > I've tried something even simpler:
> >
> > ---8<---
> > #!/bin/sh
> >
> > mutt -H -;
> > --->8---
> >
> > I used it for sending a couple of patches to linux-man@, and it seems to
> > work. I don't have much experience with mutt, so maybe I'm missing some
> > corner cases. Do you expect it to not work for some case? Otherwise,
> > we might have a winner. :)
>
> Wow, I don't know how I missed that when I read the manual. That was
> exactly the feature I was thinking that mutt would need. ;)
>
> So yeah, that is obviously better than the "postponed" hackery I showed.
> I notice that "-H" even causes mutt to ignore "-i" (a sendmail flag that
> Git adds to sendemail.sendmailcmd). So you can just invoke it directly
> from your config like:
>
> git config sendemail.sendmailcmd "mutt -H -"
Having it directly in sendmailcmd causes some glitch: It repeats all CCs
in TO. See a log:
Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
OK. Log says:
Sendmail: mutt -H - -i kevin@8t8.us mutt-dev@mutt.org alx@kernel.org e.sovetkin@gmail.com neomutt-devel@neomutt.org
From: Alejandro Colomar <alx@kernel.org>
To: Kevin McCarthy <kevin@8t8.us>,
mutt-dev@mutt.org
Cc: Alejandro Colomar <alx@kernel.org>,
Jenya Sovetkin <e.sovetkin@gmail.com>,
neomutt-devel@neomutt.org
Subject: [PATCH] send.c: Allow crypto operations in batch and mailx modes.
Date: Fri, 10 Nov 2023 01:41:24 +0100
Message-ID: <20231110004128.5972-2-alx@kernel.org>
X-Mailer: git-send-email 2.42.0
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
The sent mail ended up being
From: Alejandro Colomar <alx@kernel.org>
To: Kevin McCarthy <kevin@8t8.us>, mutt-dev@mutt.org, alx@kernel.org,
e.sovetkin@gmail.com, neomutt-devel@neomutt.org
Cc: Alejandro Colomar <alx@kernel.org>,
Jenya Sovetkin <e.sovetkin@gmail.com>, neomutt-devel@neomutt.org
So maybe we need the wrapper script to ignore the arguments.
Cheers,
Alex
>
> Annoyingly, "-E" doesn't work when reading over stdin (I guess mutt
> isn't willing to re-open the tty itself). But if you're happy with not
> editing as they go through, then "-H" is then that's enough (in my
> workflow, I do the final proofread via mutt).
>
> -Peff
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] glossary: add definitions for dereference & peel
From: Junio C Hamano @ 2023-11-10 0:22 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, ps, Victoria Dye
In-Reply-To: <pull.1610.git.1699574277143.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -125,6 +124,24 @@ to point at the new commit.
> dangling object has no references to it from any
> reference or <<def_object,object>> in the <<def_repository,repository>>.
>
> +[[def_dereference]]dereference::
> + Referring to a <<def_symref,symbolic ref>>: the action of accessing the
> + <<def_ref,reference>> pointed at by a symbolic ref. Recursive
> + dereferencing involves repeating the aforementioned process on the
> + resulting ref until a non-symbolic reference is found.
> ++
> +Referring to a <<def_tag_object,tag object>>: the action of accessing the
> +<<def_object,object>> a tag points at. Tags are recursively dereferenced by
> +repeating the operation on the result object until the result has either a
> +specified <<def_object_type,object type>> (where applicable) or any non-"tag"
> +object type.
> ++
All of the above makes sense.
I would casually mention "peeling" here with cross reference,
if I were writing this section. There already is enough cross
reference in the other direction pointing this way.
> +Referring to a <<def_commit_object,commit object>>: the action of accessing
> +the commit's tree object. Commits cannot be dereferenced recursively.
I personally consider this is weird misuse of the verb and is rarely
used, but we see it in the description of tree-ish below.
> +Unless otherwise specified, "dereferencing" as it used in the context of Git
> +commands or protocols is implicitly recursive.
Nice to see this spelled out like this.
> @@ -444,6 +461,12 @@ exclude;;
> of the logical predecessor(s) in the line of development, i.e. its
> parents.
>
> +[[def_peel]]peel::
> + Synonym for object <<def_dereference,dereference>>. Most commonly used
> + in the context of tags, where it refers to the process of recursively
> + dereferencing a <<def_tag_object,tag object>> until the result object's
> + <<def_object_type,type>> is something other than "tag".
"object dereference" is not defined anywhere (yet). "Most commonly
used in the context of tags" implies that objects other than tags
can be "peeled" and "object dereference" is a word to refer to
peeling either "commit" or "tag", but we would want to be a bit more
clear and explicit. Let's either define "object dereference", or
better yet, avoid saying "object dereference" here and instead say
something like: "Synonym for dereference when used on tags and
commits".
I've never seen "peel" used for commits, though. So another
improvement might be to say "peel" is "an act of dereferencing a
tag" here.
Thanks.
^ permalink raw reply
* [PATCH] glossary: add definitions for dereference & peel
From: Victoria Dye via GitGitGadget @ 2023-11-09 23:57 UTC (permalink / raw)
To: git; +Cc: ps, Victoria Dye, Victoria Dye
From: Victoria Dye <vdye@github.com>
Add 'gitglossary' definitions for "dereference" (as it used for both symrefs
and objects) and "peel". These terms are used in options and documentation
throughout Git, but they are not clearly defined anywhere and the behavior
they refer to depends heavily on context. Provide explicit definitions to
clarify existing documentation to users and help contributors to use the
most appropriate terminology possible in their additions to Git.
Update other definitions in the glossary that use the term "dereference" to
link to 'def_dereference'.
Signed-off-by: Victoria Dye <vdye@github.com>
---
glossary: add definitions for dereference & peel
As promised in [1], this patch adds definitions for "peel" and
"dereference" in the glossary, based on how they're currently used
throughout Git. As a result, the definitions are somewhat broad
(although I did my best to explicitly describe the different contexts in
which they're used). My hope is that this will at least reduce confusion
around this terminology. These definitions can also serve as a starting
point if, in the future, another contributor wants to deprecate certain
usages of these terms to make them less ambiguous.
* Victoria
[1]
https://lore.kernel.org/git/21dfe606-39f5-4154-aaa4-695e5f6f784d@github.com/
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1610%2Fvdye%2Fvdye%2Fglossary-peel-dereference-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1610/vdye/vdye/glossary-peel-dereference-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1610
Documentation/glossary-content.txt | 50 +++++++++++++++++++++---------
1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 65c89e7b3eb..41dd5721def 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -98,9 +98,8 @@ to point at the new commit.
revision.
[[def_commit-ish]]commit-ish (also committish)::
- A <<def_commit_object,commit object>> or an
- <<def_object,object>> that can be recursively dereferenced to
- a commit object.
+ A <<def_commit_object,commit object>> or an <<def_object,object>> that
+ can be recursively <<def_dereference,dereferenced>> to a commit object.
The following are all commit-ishes:
a commit object,
a <<def_tag_object,tag object>> that points to a commit
@@ -125,6 +124,24 @@ to point at the new commit.
dangling object has no references to it from any
reference or <<def_object,object>> in the <<def_repository,repository>>.
+[[def_dereference]]dereference::
+ Referring to a <<def_symref,symbolic ref>>: the action of accessing the
+ <<def_ref,reference>> pointed at by a symbolic ref. Recursive
+ dereferencing involves repeating the aforementioned process on the
+ resulting ref until a non-symbolic reference is found.
++
+Referring to a <<def_tag_object,tag object>>: the action of accessing the
+<<def_object,object>> a tag points at. Tags are recursively dereferenced by
+repeating the operation on the result object until the result has either a
+specified <<def_object_type,object type>> (where applicable) or any non-"tag"
+object type.
++
+Referring to a <<def_commit_object,commit object>>: the action of accessing
+the commit's tree object. Commits cannot be dereferenced recursively.
++
+Unless otherwise specified, "dereferencing" as it used in the context of Git
+commands or protocols is implicitly recursive.
+
[[def_detached_HEAD]]detached HEAD::
Normally the <<def_HEAD,HEAD>> stores the name of a
<<def_branch,branch>>, and commands that operate on the
@@ -444,6 +461,12 @@ exclude;;
of the logical predecessor(s) in the line of development, i.e. its
parents.
+[[def_peel]]peel::
+ Synonym for object <<def_dereference,dereference>>. Most commonly used
+ in the context of tags, where it refers to the process of recursively
+ dereferencing a <<def_tag_object,tag object>> until the result object's
+ <<def_object_type,type>> is something other than "tag".
+
[[def_pickaxe]]pickaxe::
The term <<def_pickaxe,pickaxe>> refers to an option to the diffcore
routines that help select changes that add or delete a given text
@@ -620,12 +643,11 @@ The most notable example is `HEAD`.
copies of) commit objects of the contained submodules.
[[def_symref]]symref::
- Symbolic reference: instead of containing the <<def_SHA1,SHA-1>>
- id itself, it is of the format 'ref: refs/some/thing' and when
- referenced, it recursively dereferences to this reference.
- '<<def_HEAD,HEAD>>' is a prime example of a symref. Symbolic
- references are manipulated with the linkgit:git-symbolic-ref[1]
- command.
+ Symbolic reference: instead of containing the <<def_SHA1,SHA-1>> id
+ itself, it is of the format 'ref: refs/some/thing' and when referenced,
+ it recursively <<def_dereference,dereferences>> to this reference.
+ '<<def_HEAD,HEAD>>' is a prime example of a symref. Symbolic references
+ are manipulated with the linkgit:git-symbolic-ref[1] command.
[[def_tag]]tag::
A <<def_ref,ref>> under `refs/tags/` namespace that points to an
@@ -661,11 +683,11 @@ The most notable example is `HEAD`.
<<def_tree,tree>> is equivalent to a <<def_directory,directory>>.
[[def_tree-ish]]tree-ish (also treeish)::
- A <<def_tree_object,tree object>> or an <<def_object,object>>
- that can be recursively dereferenced to a tree object.
- Dereferencing a <<def_commit_object,commit object>> yields the
- tree object corresponding to the <<def_revision,revision>>'s
- top <<def_directory,directory>>.
+ A <<def_tree_object,tree object>> or an <<def_object,object>> that can
+ be recursively <<def_dereference,dereferenced>> to a tree object.
+ Dereferencing a <<def_commit_object,commit object>> yields the tree
+ object corresponding to the <<def_revision,revision>>'s top
+ <<def_directory,directory>>.
The following are all tree-ishes:
a <<def_commit-ish,commit-ish>>,
a tree object,
base-commit: dadef801b365989099a9929e995589e455c51fed
--
gitgitgadget
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)
From: Junio C Hamano @ 2023-11-09 23:33 UTC (permalink / raw)
To: Taylor Blau, Johannes Schindelin; +Cc: Jeff King, git
In-Reply-To: <ZUzcmsfJe6jk4fTk@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
>> * tb/merge-tree-write-pack (2023-10-23) 5 commits
> ...
> This series received a couple of LGTMs from you and Patrick:
>
> - https://lore.kernel.org/git/xmqqo7go7w63.fsf@gitster.g/#t
> - https://lore.kernel.org/git/ZTjKmcV5c_EFuoGo@tanuki/
Yup, I am aware of them.
> Johannes had posted some comments[1] about instead using a temporary
> object store where objects are written as loose that would extend to git
> replay....
I was hoping to hear from Johannes saying he agrees with the above.
It is not strictly required, but is much nice to have once we hear
"let's step back a bit---are we going in the right direction?" and
it has been responded.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox