* Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks
From: Jeff King @ 2023-11-08 17:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <361f1bd9c88e3e6b7b135ba67b39d3bf4d70e322.1699455383.git.ps@pks.im>
On Wed, Nov 08, 2023 at 03:57:27PM +0100, Patrick Steinhardt wrote:
> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index c8e6c0733f4..3b64022dc57 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -47,9 +47,15 @@ setup_hook()
> echo "cnt=$skip_revs" > "$hook_type-counter"
> rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
> hook="$rawsvnrepo/hooks/$hook_type"
> - cat > "$hook" <<- 'EOF1'
> + cat >"$hook" <<-EOF
> #!/bin/sh
> set -e
> +
> + PATH=\"$PATH\"
> + export PATH
> + EOF
The quoting here is weird. The original escaped the double-quotes
because it was echo-ing and interpolated string:
> - echo "PATH=\"$PATH\"; export PATH" >>"$hook"
But the here-doc interpolation does not treat double-quotes as special,
and you end up with actual double-quote characters at the beginning and
end of your $PATH variable, wrecking those entries (but I guess it works
some of the time depending on whether those parts of your $PATH are
important).
It also interpolates PATH while making the here-doc. That's what you
want to convince the hook's inner shell to use the same PATH as the
outer shell, but it also means that the inner shell will parse and
evaluate it. I don't think anyone is going to inject '"; rm -rf /' into
their own test runs, but it does mean we'd do the wrong thing if their
$PATH contains double-quotes or backslashes.
Certainly the original suffered from the same issue, so it may not be
worth worrying about. But the more robust way to do it is:
# export ORIG_PATH, which presumably is not cleared the same way PATH
# is, so that the hook can access it
ORIG_PATH=$PATH &&
export ORIG_PATH &&
cat >"$hook" <<EOF
# pull the original path from the caller
PATH=$ORIG_PATH
export PATH
...do other stuff...
EOF
That's assuming that environment variables make it intact to the hook at
all (it is not clear to me why the original $PATH doesn't). If they
don't, you'd probably have to stash it in a file like:
echo "$PATH" >orig-path
cat >"$hook" <<EOF
PATH="$(cat orig-path)"
export PATH
EOF
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
From: Jeff King @ 2023-11-08 17:02 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <f4c6c754f1e519b655d45572bd7566183d98138b.1699455383.git.ps@pks.im>
On Wed, Nov 08, 2023 at 03:57:23PM +0100, Patrick Steinhardt wrote:
> 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.
Thanks for doing this. I died inside a little while adding the
proxy-passwd one recently in 29ae2c9e74 (add basic http proxy tests,
2023-02-16). There I mused about moving to bcrypt in a separate patch,
which I think is probably the least-bad option from a security
perspective. But I agree that md5 is more likely to be available
everywhere, and we certainly don't care about security here.
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Jeff King @ 2023-11-08 16:54 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <cee8fbebf84422f73c38d55b5730053121b74e0f.1699455383.git.ps@pks.im>
On Wed, Nov 08, 2023 at 03:57:19PM +0100, Patrick Steinhardt wrote:
> 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. The new runtime-detected paths will only be
> used in case none of the hardcoded paths are usable.
If these improve detection on your platform, I think that is a good
thing and they are worth doing. But as a generic mechanism, I have two
comments:
> -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)"
> do
> if test -x "$DEFAULT_HTTPD_PATH"
> then
The binary goes under the name "httpd", but also "apache2". But the PATH
search only looks for "httpd". Should we check "command -v apache2" as
well?
This also means we may run "test -x" on an empty string, but that is
probably OK in practice (we could guard it with "test -n", though).
> +if test -x "$DEFAULT_HTTPD_PATH"
> +then
> + DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> +fi
I was really pleased to see this and hoped it could replace the
kitchen-sink list of paths in the hunk below. But sadly I think it
depends on having a configured apache setup. On my Debian system, for
example, I have the "apache2-bin" package installed but not "apache"
(because only the latter actually sets up a system apache daemon, which
I don't want). And thus there is no config:
$ /usr/sbin/apache2 -D HTTPD_ROOT
apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory
So without a system config file to act as a template for our custom
config, I think we are stuck with guessing where the installer might
have put them.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Jeff King @ 2023-11-08 16:44 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Junio C Hamano, git
In-Reply-To: <ZUtmAyVHO4ROhLEq@tanuki>
On Wed, Nov 08, 2023 at 11:42:11AM +0100, Patrick Steinhardt wrote:
> I was a bit torn myself when writing this. I can also see a potential
> future where we would drop the hardcoded list of locations altogether in
> favor of always using PATH. After all we already rely on PATH to resolve
> other tools as well, so why should httpd be special there?
httpd/apache2 is often in sbin, which is not as commonly found in the
PATH of regular users. E.g., this is the case on Debian (it is
/sbin/apache2, and a newly created user will not have that in their
PATH).
-Peff
^ permalink raw reply
* On November 8, 2023, a notice for fee distribution was issued.FKHDY22679379347of Plan
From: gjtuityikjgjgijfjhjgjfcggkgjku @ 2023-11-08 14:58 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 71 bytes --]
To/For : git@vger.kernel.org
Renewal E:bill : 276459774
Many Thanks
[-- Attachment #2: Bill22679379347.pdf --]
[-- Type: application/octet-stream, Size: 65460 bytes --]
^ permalink raw reply
* [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks
From: Patrick Steinhardt @ 2023-11-08 14:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1699455383.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]
The post-commit hook in t9164 is executed with a default environment.
To work around this issue we already write the current `PATH` value into
the hook script. But this is done at a point where we already tried to
execute non-built-in commands, namely basename(1). While this seems to
work alright on most platforms, it fails on NixOS.
Set the `PATH` variable earlier to fix this issue. Note that we do this
via two separate heredocs. This is done because in the first one we do
want to expand variables, whereas in the second one we don't.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..3b64022dc57 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -47,9 +47,15 @@ setup_hook()
echo "cnt=$skip_revs" > "$hook_type-counter"
rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
hook="$rawsvnrepo/hooks/$hook_type"
- cat > "$hook" <<- 'EOF1'
+ cat >"$hook" <<-EOF
#!/bin/sh
set -e
+
+ PATH=\"$PATH\"
+ export PATH
+ EOF
+
+ cat >>"$hook" <<-'EOF'
cd "$1/.." # "$1" is repository location
exec >> svn-hook.log 2>&1
hook="$(basename "$0")"
@@ -59,11 +65,11 @@ setup_hook()
cnt="$(($cnt - 1))"
echo "cnt=$cnt" > ./$hook-counter
[ "$cnt" = "0" ] || exit 0
-EOF1
+ EOF
+
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 v2 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
From: Patrick Steinhardt @ 2023-11-08 14:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1699455383.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 v2 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-08 14:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1699455383.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1954 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. The new runtime-detected paths will only be
used in case none of the hardcoded paths are usable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-httpd.sh | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 5fe3c8ab69d..26c1632ea9a 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,7 +55,7 @@ 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)"
do
if test -x "$DEFAULT_HTTPD_PATH"
then
@@ -63,11 +63,17 @@ do
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/libexec/httpd' \
+ "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
do
if test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 0/3] t: improve compatibility with NixOS
From: Patrick Steinhardt @ 2023-11-08 14:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1699428122.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4332 bytes --]
Hi,
this is the second version of my patch series to improve compatibility
of our tests with NixOS.
There's only a single change compared to v1. As suggested by Junio, we
now use the executable discovered via PATH and the modules detected via
`HTTPD_ROOT` as fallback values if none of our hardcoded paths were
detected. This should further ensure that the refactoring doesn't cause
any issues on platforms that follow the FHS more closely than NixOS
does. It also has the benefit that the change is now a lot more straight
forward.
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 hooks
t/lib-httpd.sh | 10 ++++++++--
t/lib-httpd/passwd | 2 +-
t/lib-httpd/proxy-passwd | 2 +-
t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
4 files changed, 19 insertions(+), 7 deletions(-)
Range-diff against v1:
1: ac059db8fed ! 1: cee8fbebf84 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.
- Refactor the code to do so. If runtime detection of the paths fails we
- continue to fall back to the hardcoded list of paths.
+ Amend the code to do so. The new runtime-detected paths will only be
+ used in case none of the hardcoded paths are usable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ t/lib-httpd.sh: fi
HTTPD_PARA=""
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
--do
-- if test -x "$DEFAULT_HTTPD_PATH"
-- then
-- break
-- fi
--done
--
--for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
-- '/usr/lib/apache2/modules' \
-- '/usr/lib64/httpd/modules' \
-- '/usr/lib/httpd/modules' \
-- '/usr/libexec/httpd'
--do
-- if test -d "$DEFAULT_HTTPD_MODULE_PATH"
-+DEFAULT_HTTPD_PATH="$(command -v httpd)"
-+if test -z "$DEFAULT_HTTPD_PATH" -o ! -x "$DEFAULT_HTTPD_PATH"
-+then
-+ for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
-+ do
-+ if test -x "$DEFAULT_HTTPD_PATH"
-+ then
-+ break
-+ fi
-+ done
-+fi
-+
-+if test -x "$DEFAULT_HTTPD_PATH"
-+then
-+ HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
-+ if test -n "$HTTPD_ROOT" -a -d "$HTTPD_ROOT/modules"
++for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
+ do
+ if test -x "$DEFAULT_HTTPD_PATH"
then
-- break
-+ DEFAULT_HTTPD_MODULE_PATH="$HTTPD_ROOT/modules"
+@@ t/lib-httpd.sh: do
fi
--done
-+ unset HTTPD_ROOT
-+fi
-+
-+if test -z "$DEFAULT_HTTPD_MODULE_PATH" -o ! -d "$DEFAULT_HTTPD_MODULE_PATH"
+ done
+
++if test -x "$DEFAULT_HTTPD_PATH"
+then
-+ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
-+ '/usr/lib/apache2/modules' \
-+ '/usr/lib64/httpd/modules' \
-+ '/usr/lib/httpd/modules' \
-+ '/usr/libexec/httpd'
-+ do
-+ if test -d "$DEFAULT_HTTPD_MODULE_PATH"
-+ then
-+ break
-+ fi
-+ done
++ DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+fi
-
- case $(uname) in
- Darwin)
++
+ 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/libexec/httpd' \
++ "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
+ do
+ if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+ then
2: 23835763002 = 2: f4c6c754f1e t/lib-httpd: stop using legacy crypt(3) for authentication
3: b50e625f967 = 3: 361f1bd9c88 t9164: fix inability to find basename(1) in hooks
base-commit: 98009afd24e2304bf923a64750340423473809ff
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* (no subject)
From: ابوعمار العكيدي @ 2023-11-08 13:40 UTC (permalink / raw)
To: 4d8e3fd30710040527j61152b2dh1b073504ba19d490, alhmsyabwmar72, git,
paolo.ciarrocchi, vmiklos
^ permalink raw reply
* Re: [PATCH v6 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-08 12:47 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <20231102135151.843758-1-christian.couder@gmail.com>
Hi Christian,
On Thu, 2 Nov 2023, Christian Couder wrote:
> # Range-diff between v5 and v6
>
> (Sorry it's very long mostly due to doc and test changes over a number
> of patches.)
I am grateful for the detailed range-diff that let's me focus exclusively
on what changed between iterations.
> 1: 72c34a0eba = 1: fac0a9dff4 t6429: remove switching aspects of fast-rebase
> 2: f85e6c823c ! 2: 8a605ddef8 replay: introduce new builtin
> @@ Commit message
> For now, this is just a rename from `t/helper/test-fast-rebase.c` into
> `builtin/replay.c` with minimal changes to make it build appropriately.
>
> + There is a stub documentation and a stub test script though.
> +
> Subsequent commits will flesh out its capabilities and make it a more
> standard regular builtin.
>
> @@ .gitignore
> /git-rerere
> /git-reset
>
> + ## Documentation/git-replay.txt (new) ##
> +@@
> ++git-replay(1)
> ++=============
> ++
> ++NAME
> ++----
> ++git-replay - EXPERIMENTAL: Replay commits on a new base, works with bare repos too
> ++
> ++
> ++SYNOPSIS
> ++--------
> ++[verse]
> ++'git replay' --onto <newbase> <revision-range>... # EXPERIMENTAL
Technically, at this stage `git replay` requires precisely 5 arguments, so
the `<revision>...` is incorrect and should be `<upstream> <branch>`
instead. But it's not worth a new iteration to fix this.
> ++
> ++DESCRIPTION
> ++-----------
> ++
> ++Takes a range of commits and replays them onto a new location.
> ++
> ++THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> ++
> ++OPTIONS
> ++-------
> ++
> ++--onto <newbase>::
> ++ Starting point at which to create the new commits. May be any
> ++ valid commit, and not just an existing branch name.
> ++
Traditionally, this would be a place to describe the `<revision>` argument
(or, in this patch, to reflect the current state of `builtin/replay.c`,
the `<upstream> <branch>` arguments instead).
> ++EXIT STATUS
> ++-----------
> ++
> ++For a successful, non-conflicted replay, the exit status is 0. When
> ++the replay has conflicts, the exit status is 1. If the replay is not
> ++able to complete (or start) due to some kind of error, the exit status
> ++is something other than 0 or 1.
> ++
> ++GIT
> ++---
> ++Part of the linkgit:git[1] suite
> +
> ## Makefile ##
> [... snipping non-controversial changes ...]
> 6: 37d545d5d6 ! 6: edafe4846f replay: change rev walking options
> [... snipping changes Elijah already talked about ...]
> 7: 2943f08926 = 7: b81574744a replay: add an important FIXME comment about gpg signing
> 8: f81962ba41 = 8: b08ad2b2f1 replay: remove progress and info output
> 9: 236747497e ! 9: 5099c94d2e replay: remove HEAD related sanity check
> {... snipping non-controversial changes ...]
Thank you!
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v5 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-08 12:25 UTC (permalink / raw)
To: Christian Couder
Cc: Elijah Newren, git, Junio C Hamano, Patrick Steinhardt, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <CAP8UFD09dZbrbebRFZvarY71q5Vc0YBfRQHbTg7A3H-qM2g8fg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]
Hi Christian,
On Thu, 2 Nov 2023, Christian Couder wrote:
> On Thu, Oct 26, 2023 at 3:44 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > In addition, I am still a bit uneasy with introducing both the manual page
> > and the test script in this commit (see my comments in
> > https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/).
> > It would be better to uphold our high standard and introduce scaffolds for
> > both files in the first commit, then populate the file contents
> > incrementally in the same the patches that introduce the corresponding
> > options/features/changes.
>
> I have tried to improve on that in the v6 I just sent, but there are
> many patches implementing changes in behavior that I think weren't
> worth documenting and testing in `test-tool fast-rebase` (which had no
> doc and no test) and that aren't worth documenting and testing
> specifically in `git replay` either.
Thank you for putting in the effort. I appreciate it very much.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v5 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-08 12:25 UTC (permalink / raw)
To: Christian Couder
Cc: Elijah Newren, git, Junio C Hamano, Patrick Steinhardt, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <CAP8UFD04PbOLgep3-s-_4_fMbqtrWMR87K1cG-DsKnbyHEBT_w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2028 bytes --]
Hi Christian,
On Thu, 2 Nov 2023, Christian Couder wrote:
> On Sun, Oct 29, 2023 at 7:02 AM Elijah Newren <newren@gmail.com> wrote:
>
> > On Thu, Oct 26, 2023 at 6:44 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > As mentioned in
> > > https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/,
> > > I would like the `EXPERIMENTAL` label to be shown prominently here.
> > > Probably not only the `SYNOPSIS` as I had originally suggested but
> > > also in the `NAME`.
>
> Ok, I have made changes in the v6 I just sent, so that there is
> EXPERIMENTAL both in the NAME and SYNOPSIS.
>
> > > Otherwise we may end up with the same situation as with the (from my
> > > perspective, failed) `git switch`/`git restore` experiment, where we
> > > wanted to explore a better user experience than the overloaded `git
> > > checkout` command, only to now be stuck with having to maintain
> > > backward-compatibility for `git switch`/`git restore` command-line options
> > > that were not meant to be set in stone but to be iterated on, instead. A
> > > real-life demonstration of [Hyrum's Law](hyrumslaw.com/), if you like. Or,
> > > from a different angle, we re-enacted https://xkcd.com/927/ in a way.
>
> Nit: Hyrum's Law says:
>
> "With a sufficient number of users of an API,
> it does not matter what you promise in the contract:
> all observable behaviors of your system
> will be depended on by somebody."
>
> The doc is part of the contract, which according to this law doesn't
> matter. So I don't see why you use this law to suggest a doc change.
You're right. In addition to the documentation (where we definitely need
to state the experimental nature of the command), we may want to consider
adding the `EXPERIMENTAL` label not only to the output of `git replay -h`,
but show also a warning for every `git replay` invocation cautioning users
against depending on the current command-line options of this command.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v6 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-08 12:19 UTC (permalink / raw)
To: Elijah Newren
Cc: Christian Couder, git, Junio C Hamano, Patrick Steinhardt,
John Cai, Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <CABPp-BE6G2qaF50bhz-CwxSsvxGDHzwvsWtfQO4zVcX6ERppLw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]
Hi,
On Mon, 6 Nov 2023, Elijah Newren wrote:
> On Thu, Nov 2, 2023 at 6:52 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> [...]
>
> > @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> > -
> > strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
> >
> > ++ /*
> > ++ * TODO: For now, let's warn when we see an option that we are
> > ++ * going to override after setup_revisions() below. In the
> > ++ * future we might want to either die() or allow them if we
> > ++ * think they could be useful though.
> > ++ */
> > ++ for (i = 0; i < argc; i++) {
> > ++ if (!strcmp(argv[i], "--reverse") || !strcmp(argv[i], "--date-order") ||
> > ++ !strcmp(argv[i], "--topo-order") || !strcmp(argv[i], "--author-date-order") ||
> > ++ !strcmp(argv[i], "--full-history"))
> > ++ warning(_("option '%s' will be overridden"), argv[i]);
> > ++ }
> > ++
>
> [...] This seems like an inefficient way to provide this warning;
Not only inefficient: think about the false positive when looking at a
command-line containing `--grep --reverse`. In this instance, `--reverse`
is an argument of the `--grep` option, but would be mistaken for an option
in its own right.
Ciao,
Johannes
^ permalink raw reply
* Re: Error when "git mv" file in a sparsed checkout
From: Josef Wolf @ 2023-11-08 11:36 UTC (permalink / raw)
To: git
In-Reply-To: <CABPp-BEAU8rPeNHphut0ZxcLdH0pzjh+Z_CF+rg2uhvVZoZfxg@mail.gmail.com>
Thanks for the reply, Elijah!
On Tue, Nov 07, 2023 at 06:21:00PM -0800, Elijah Newren wrote:
> On Tue, Nov 7, 2023 at 5:32 AM Josef Wolf <jw@raven.inka.de> wrote:
> > I have used the procedure described below for many years. In fact,
> > this procedure is part of a script which I am using for about 10 years.
> > This procedure was definitely working with git-2-25-1 and git-2.26.2.
> >
> > Now, with git-2.34.1 (on a freshly installed ubuntu-22.04), this
> > procedure fails.
> >
> > Here is what I do:
> >
> > I want to rename a file on a branch which is not currently checked out
> > without messing/touching my current working directory.
> >
> > For this, I first create a clone of the repo with shared git-directory:
> >
> > $ SANDBOX=/var/tmp/manage-scans-X1pKZQiey
> > $ WT=$SANDBOX/wt
> > $ GIT=$SANDBOX/git
> >
> > $ mkdir -p $SANDBOX
> > $ git --work-tree $WT --git-dir $GIT clone -qns -n ~/upstream-repo $GIT
> >
> > Then, I do a sparse checkout in this clone, containing only the file
> > that is to be renamed:
> >
> > $ cd $WT
> > $ echo 'path/to/old-filename' >>$GIT/info/sparse-checkout
> > $ git --work-tree $WT --git-dir $GIT config core.sparsecheckout true
> > $ git --work-tree $WT --git-dir $GIT checkout -b the-branch remotes/origin/the-branch
> > Switched to a new branch 'the-branch'
> >
> > Next step would be to "git mv" the file:
> >
> > $ mkdir -p /path/to # already exists, but should do no harm
> > $ git --work-tree $WT --git-dir $GIT mv path/to/old-filename path/to/new-filename
>
> sparse checkouts are designed such that only files matching the
> patterns in the sparse-checkout file should be present in the working
> tree, so renaming to a path that should not be present is problematic.
> We could possibly have "git-mv" immediately remove the path from the
> working tree (while leaving the new pathname in the index), but that's
> problematic in that users often overlook the index and only look at
> the working tree and might think the file was deleted instead of
> renamed. Not immediately removing it is potentially even worse,
> because any subsequent operation (particularly ones like checkout,
> reset, merge, rebase, etc.) are likely to nuke the file from the
> working tree and the fact that the removal is delayed makes it much
> harder for users to understand and diagnose.
>
> So, Stolee fixed this to make it throw an error; see
> https://lore.kernel.org/git/pull.1018.v4.git.1632497954.gitgitgadget@gmail.com/
> for details. His description did focus on cone mode, but you'll note
> that none of my explanation here did. The logic for making this an
> error fully applies to non-cone mode for all the same reasons.
>
> If you want to interact with `path/to/new-filename` as a path within
> your sparse checkout (as suggested by your git-mv command), then that
> path should actually be part of your sparse checkout. In other words,
> you should add `path/to/new-filename` to $GIT/info/sparse-checkout and
> do so _before_ attempting your `git mv` command. If you don't like
> that for some reason, you are allowed to instead ignore the
> problematic consequences of renaming outside the sparse-checkout by
> providing the `--sparse` flag. Both of these possibilities are
> documented in the hints provided along with the error message you
> showed below:
>
> > The following paths and/or pathspecs matched paths that exist
> > outside of your sparse-checkout definition, so will not be
> > updated in the index:
> > path/to/new-filename
> > hint: If you intend to update such entries, try one of the following:
> > hint: * Use the --sparse option.
> > hint: * Disable or modify the sparsity rules.
> > hint: Disable this message with "git config advice.updateSparsePath false"
> >
> > This error is something I have not expected.
> >
> > Error message suggests, there already exists a file named "new-filename". This
> > is not true at all. There is no file named "new-filename" in the entire
> > repository. Not in any directory of any branch.
>
> You are correct; the wording of the error message here is suboptimal
> and seems to have been focused more on the git-add case (the error
> message is shared by git-add, git-mv, and git-rm). Thanks for
> pointing it out! We could improve that wording, perhaps with
> something like:
>
> The following paths and/or pathspecs match paths that are
> outside of your sparse-checkout definition, so will not be
> updated:
>
> Which is still slightly slanted towards git-add and git-rm cases, but
> I hope it works better than the current message. Thoughts?
Yes, the wording was pretty much confusing me, since i could not find a file
named "new-file" anywhere in the repo.
There are more things confusing concerning sparse mode:
- It is not clear from git-sparse-checkout(1) when changes to
$GIT_DIR/info/sparse-checkout are catched up. In my case: would it be enough
to add the new pathname just before git-mv or would a fresh git-checkout be
needed after modifying $GIT_DIR/info/sparse-checkout? You have clarified
this in your response, but shouldn't this be clear from the manpage?
- git-sparse-checkout(1) refers to "skip-worktree bit". This concept is
potentially not very familiar to the average git user which uses mostly
porcelain. Thus, edge cases remain to be unclear.
- The pathspecs refers to .gitignore (which by itself is not very clear). But
there are differences:
1. giignore is relative to containing directoy, which don't seem to make
much sense for sparse mode
2. sparse specs are the opposite of gitignore, which seems to have different
meaning in some edge-cases.
- For cone, it is not clear how the two "accepted patterns" look like what the
semantics are. I understand that specifying a directory adds siblings
recursively. But what does the "Parent" mode mean exactly and when/how is
this recognized? I guess, this is just a mis-namer? IMHO, parent of /a/b/c would
be /a/b and not /a/b/c/* (as git-sparse-chekout(1) suggests).
I guess all this is very clear to the core-developers. But for the occasional
user like myself, all this is pretty much confusing.
--
Josef Wolf
jw@raven.inka.de
^ permalink raw reply
* Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-08 10:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqcywk7k0d.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]
On Wed, Nov 08, 2023 at 04:59:46PM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > 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.
> >
> > Refactor the code to do so. If runtime detection of the paths fails we
> > continue to fall back to the hardcoded list of paths.
>
> Hmph.
>
> I do not think we would want to punish the distros that follow the
> FHS that was created explicitly to help developers by standardizing
> locations of various things, with an approach this patch takes that
> throws everthing with bathwater and rely on $PATH first.
>
> Would it be sufficient to please NixOS if we simply append $(command
> -v apache) or whatever after the well known candidate locations?
I was a bit torn myself when writing this. I can also see a potential
future where we would drop the hardcoded list of locations altogether in
favor of always using PATH. After all we already rely on PATH to resolve
other tools as well, so why should httpd be special there?
But in the end I opted to use the more conservative approach of using
both PATH and the static list as I didn't want to break other distros. I
don't mind to make this even more conservative and resolve via PATH as a
last resort, only.
Patrick
> I know "command -v" is in POSIX, and on both bash and dash (the two
> shells most distros use), it works as this patch expects, but its
> portability is also a bit worrysome, especially because the whole
> point of this patch is to support a platform that is, eh, on the
> fringe.
>
> So, I dunno.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/9] for-each-ref: clarify interaction of --omit-empty & --count
From: Kristoffer Haugsbakk @ 2023-11-08 10:00 UTC (permalink / raw)
To: Øystein Walle; +Cc: Josh Soref, git, Victoria Dye
In-Reply-To: <CAFaJEquUBYUO=scjHw2qrUyP-4wZJWtdmWAtRrW0mH9x9PbZZw@mail.gmail.com>
On Wed, Nov 8, 2023, at 08:53, Øystein Walle wrote:
> On Tue, 7 Nov 2023 at 20:30, Victoria Dye <vdye@github.com> wrote:
>
>> Since the interaction isn't clearly defined at the moment, we could probably
>> still update it to work like you're describing here. I'm happy to drop this
>> patch and implement your recommendation in a follow-up series. Let me know
>> what you think!
>
> Regardless of whether the logic is changed in a follow-up series or not
> I think the current behavior is worth documenting even if it doesn't
> exist for much longer in the tree. So I am favor of having this patch as
> part of this series.
The funny thing though is that once it’s documented then you also kind of
commit yourself to it, right? That it’s how it’s supposed to behave.[1] If
you instead change the behavior (to the correct one) and document it in
the same series then there is no in-between time when people can claim to
rely on it via the documentation.
[1] Modulo “subject to change” hedging, but it seems that even
experimental commands who are documented as that are now resistant to
change in practice.
^ permalink raw reply
* Re: [PATCH 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
From: Junio C Hamano @ 2023-11-08 8:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <23835763002c5f5cd68db7bdc9e4c083dda3558f.1699428122.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> 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.
This step makes quite a lot of sense, as we are changing this not at
all for security but for portability ;-)
>
> [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.
^ permalink raw reply
* Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Junio C Hamano @ 2023-11-08 7:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ac059db8fedc6493c64f703814c7db11adb4385e.1699428122.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> 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.
>
> Refactor the code to do so. If runtime detection of the paths fails we
> continue to fall back to the hardcoded list of paths.
Hmph.
I do not think we would want to punish the distros that follow the
FHS that was created explicitly to help developers by standardizing
locations of various things, with an approach this patch takes that
throws everthing with bathwater and rely on $PATH first.
Would it be sufficient to please NixOS if we simply append $(command
-v apache) or whatever after the well known candidate locations?
I know "command -v" is in POSIX, and on both bash and dash (the two
shells most distros use), it works as this patch expects, but its
portability is also a bit worrysome, especially because the whole
point of this patch is to support a platform that is, eh, on the
fringe.
So, I dunno.
^ permalink raw reply
* Re: [PATCH 2/9] for-each-ref: clarify interaction of --omit-empty & --count
From: Øystein Walle @ 2023-11-08 7:53 UTC (permalink / raw)
To: Victoria Dye; +Cc: gitgitgadget, git
In-Reply-To: <e166abeb-3566-4acf-a252-bc493ee37f41@github.com>
On Tue, 7 Nov 2023 at 20:30, Victoria Dye <vdye@github.com> wrote:
> Since the interaction isn't clearly defined at the moment, we could probably
> still update it to work like you're describing here. I'm happy to drop this
> patch and implement your recommendation in a follow-up series. Let me know
> what you think!
Regardless of whether the logic is changed in a follow-up series or not
I think the current behavior is worth documenting even if it doesn't
exist for much longer in the tree. So I am favor of having this patch as
part of this series.
I am also in favor of changing the behavior but that warrants a separate
series and discussion.
Øsse
^ permalink raw reply
* Re: first-class conflicts?
From: Elijah Newren @ 2023-11-08 7:31 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: phillip.wood, Sandra Snan, git, Randall S. Becker
In-Reply-To: <CAESOdVDmQ85-des6Au-LH0fkUB9BZBZho0r-5=8MkPLJVA5WQQ@mail.gmail.com>
Hi Martin,
On Tue, Nov 7, 2023 at 9:38 AM Martin von Zweigbergk
<martinvonz@google.com> wrote:
>
[...]
> > One thing to think about if we ever want to implement this is what other
> > data we need to store along with the conflict trees to preserve the
> > context in which the conflict was created. For example the files that
> > are read by "git commit" when it commits a conflict resolution. For a
> > single cherry-pick/revert it would probably be fairly straight forward
> > to store CHERRY_PICK_HEAD/REVERT_HEAD and add it as a parent so it gets
> > transferred along with the conflicts. For a sequence of cherry-picks or
> > a rebase it is more complicated to preserve the context of the conflict.
> > Even "git merge" can create several files in addition to MERGE_HEAD
> > which are read when the conflict resolution is committed.
>
> Good point. We actually don't store any extra data in jj. The old
> per-path conflict model was prepared for having some label associated
> with each term of the conflict but we never actually used it.
>
> If we add such metadata, it would probably have to be something that
> makes sense even after pushing the conflict to another repo, so it
> probably shouldn't be commit ids, unless we made sure to also push
> those commits. Also note that if you `jj restore --from <commit with
> conflict>`, you can get a conflict into a commit that didn't have
> conflicts previously. Or if you already had conflicts in the
> destination commit, your root trees (the multiple root trees
> constituting the conflict) will now have conflicts that potentially
> were created by two completely unrelated operations, so you would kind
> of need different labels for different paths.
>
> https://github.com/martinvonz/jj/issues/1176 has some more discussion
> about this.
Interesting link; thanks for sharing.
I am curious more about the data you do store. My fuzzy memory is
that you store a commit header involving something of the form "A + B
- C", where those are all commit IDs. Is that correct? Is this in
addition to a normal "tree" header as in Git, or are one of A or B
found in the tree header? I think you said there was also the
possibility for more than three terms. Are those for when a
conflicted commit is merged with another branch that adds more
conflicts, or are there other cases too? (Octopus merges?)
What about recursive merges, i.e. merges where the two sides do not
have a unique merge base. What is the form of those? (Would "- C" be
replaced by "- C1 - C2 - ... - Cn"? Or would we create the virtual
merge base V and then do a " - V"? Or do we only have "A + B"?)
You previously mentioned that if someone goes to edit a commit with
conflicts, and resolves the conflicts in just one file, then you can
modify each of the trees A, B, and C such that a merging of those
trees gives the partially resolved result. How does one do that with
special conflicts, such as:
* User modifies file D on both sides of history, in conflicting
ways, and also renames D -> E on one side of history. User checks out
this conflicted commit and fixes the conflicts in E (but not other
files) and does a "git add E". When they go to commit, does the
machinery need a mapping to figure out that it needs to adjust "D" in
two of the trees while adjusting "E" in the other?
* Similar to the above, but the side that doesn't rename D renames
olddir/ -> newdir/, and the side that renames D instead renames
D->olddir/E. For this case, the file will end up at newdir/E; do we
need the backward mapping from newdir/E to both olddir/E and D?
* Slightly different than the above: User renames D -> E on one
side of history, and D -> F on the other. That's a rename/rename
(1to2) conflict. User checks out this conflicted commit and does a
"git add F", marking it as okay, but leaving E conflicted. How can
one adjust the tree such that no conflict for F appears, but one still
appears for E?
* Similar to above with an extra wrinkle: User renames D -> E on
one side of history, and on the other side both renames D -> F and
adds a slightly different file named E. That's both a rename/rename
(1to2) conflict for E & F, and an add/add conflict for E. Users
checks out this conflicted commit and resolves textual conflict in E
(in favor of the "other side"), and does a "git add E", marking it as
resolved. When they go to commit, we not only need to worry about
making sure a conflict for F appears, we also need to figure out how
to adjust the tree such that the merge result gives you the expected
value in E without affecting F. How can that be done?
On the first two bullet points, there's no such thing as a reverse
mapping from conflicted files to original files from previous commits
in current Git. Creating one, if possible, would be a fair amount of
work. But, I'm not so sure it's even possible, due to the fact that
conflicts and files do not always have one-to-one (or even one-to-many
or many-to-one) relationships; many-to-many relationship can exist, as
I've started alluding to in the last two bullet points (see also
https://github.com/git/git/blob/98009afd24e2304bf923a64750340423473809ff/Documentation/git-merge-tree.txt#L266-L271).
In fact, they can get even more complicated (e.g.
https://github.com/git/git/blob/master/t/t6422-merge-rename-corner-cases.sh#L1017-L1022).
> > > But we'd also have to be careful and think through usecases, including
> > > in the surrounding community. People would probably want to ensure
> > > that e.g. "Protected" or "Integration" branches don't get accept
> > > fetches or pushes of conflicted commits,
> >
> > I think this is a really important point, while it can be useful to
> > share conflicts so they can be collaboratively resolved we don't want to
> > propagate them into "stable" or production branches. I wonder how 'jj'
> > handles this.
>
> Agreed. `jj git push` refuses to push commits with conflicts, because
> it's very unlikely that the remote will be able to make any sense of
> it. Our commit backend at Google does support conflicts, so users can
> check out each other's conflicted commits there (except that we
> haven't even started dogfooding yet).
I'm curious to hear what happens when you do start dogfooding, on
projects with many developers and which are jj-only. Do commits with
conflicts accidentally end up in mainline branches, or are there good
ways to make sure they don't hit anything considered stable?
> > > git status would probably
> > > need some special warnings or notices, git checkout would probably
> > > benefit from additional warnings/notices checks for those cases, git
> > > log should probably display conflicted commits differently, we'd need
> > > to add special handling for higher order conflicts (e.g. a merge with
> > > conflicts is itself involved in a merge) probably similar to what jj
> > > has done, and audit a lot of other code paths to see what would be
> > > needed.
> >
> > As you point out there is a lot more to this than just being able to
> > store the conflict data in a commit - in many ways I think that is the
> > easiest part of the solution to sharing conflicts.
>
> Yes, I think it would be a very large project. Unlike jj, Git of
> course has to worry about backwards compatibility. For example, you
> would have to decide if your goal - even in the long term - is to make
> `git rebase` etc. not get interrupted due to conflicts.
...and whether to copy jj's other feature in this area in some form:
auto-rebasing any descendants when you checkout and amend an old
commit (e.g. to resolve conflicts). :-)
^ permalink raw reply
* [PATCH 3/3] t9164: fix inability to find basename(1) in hooks
From: Patrick Steinhardt @ 2023-11-08 7:30 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1699428122.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]
The post-commit hook in t9164 is executed with a default environment.
To work around this issue we already write the current `PATH` value into
the hook script. But this is done at a point where we already tried to
execute non-built-in commands, namely basename(1). While this seems to
work alright on most platforms, it fails on NixOS.
Set the `PATH` variable earlier to fix this issue. Note that we do this
via two separate heredocs. This is done because in the first one we do
want to expand variables, whereas in the second one we don't.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..3b64022dc57 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -47,9 +47,15 @@ setup_hook()
echo "cnt=$skip_revs" > "$hook_type-counter"
rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
hook="$rawsvnrepo/hooks/$hook_type"
- cat > "$hook" <<- 'EOF1'
+ cat >"$hook" <<-EOF
#!/bin/sh
set -e
+
+ PATH=\"$PATH\"
+ export PATH
+ EOF
+
+ cat >>"$hook" <<-'EOF'
cd "$1/.." # "$1" is repository location
exec >> svn-hook.log 2>&1
hook="$(basename "$0")"
@@ -59,11 +65,11 @@ setup_hook()
cnt="$(($cnt - 1))"
echo "cnt=$cnt" > ./$hook-counter
[ "$cnt" = "0" ] || exit 0
-EOF1
+ EOF
+
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 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
From: Patrick Steinhardt @ 2023-11-08 7:30 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1699428122.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 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-08 7:29 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1699428122.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2623 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.
Refactor the code to do so. If runtime detection of the paths fails we
continue to fall back to the hardcoded list of paths.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-httpd.sh | 51 +++++++++++++++++++++++++++++++++-----------------
1 file changed, 34 insertions(+), 17 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 2fb1b2ae561..06038f72607 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,25 +55,42 @@ fi
HTTPD_PARA=""
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
-do
- if test -x "$DEFAULT_HTTPD_PATH"
- then
- break
- fi
-done
-
-for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
- '/usr/lib/apache2/modules' \
- '/usr/lib64/httpd/modules' \
- '/usr/lib/httpd/modules' \
- '/usr/libexec/httpd'
-do
- if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+DEFAULT_HTTPD_PATH="$(command -v httpd)"
+if test -z "$DEFAULT_HTTPD_PATH" -o ! -x "$DEFAULT_HTTPD_PATH"
+then
+ for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
+ do
+ if test -x "$DEFAULT_HTTPD_PATH"
+ then
+ break
+ fi
+ done
+fi
+
+if test -x "$DEFAULT_HTTPD_PATH"
+then
+ HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+ if test -n "$HTTPD_ROOT" -a -d "$HTTPD_ROOT/modules"
then
- break
+ DEFAULT_HTTPD_MODULE_PATH="$HTTPD_ROOT/modules"
fi
-done
+ unset HTTPD_ROOT
+fi
+
+if test -z "$DEFAULT_HTTPD_MODULE_PATH" -o ! -d "$DEFAULT_HTTPD_MODULE_PATH"
+then
+ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
+ '/usr/lib/apache2/modules' \
+ '/usr/lib64/httpd/modules' \
+ '/usr/lib/httpd/modules' \
+ '/usr/libexec/httpd'
+ do
+ if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+ then
+ break
+ fi
+ done
+fi
case $(uname) in
Darwin)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/3] t: improve compatibility with NixOS
From: Patrick Steinhardt @ 2023-11-08 7:29 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]
Hi,
this patch series improves compatibility of our test suite with NixOS.
NixOS is somewhat special compared to more conventional Linux distros
because it doesn't follow the Filesystem Hierarchy Specification for
most of the part. Instead, packages are installed into the Nix store
in `/nix` with hashed paths that change frequently when upgrading the
system to a newer generation. Consequentially, paths cannot be hardcoded
and must instead be computed at runtime.
We have two such issues in our test harness right now:
- t/lib-httpd searches Apache httpd and its modules directory in a
list of hardcoded paths.
- t9164 doesn't propagate PATH to a script and thus cannot find the
basename(1) utility.
Both of these issues are fixed in this patch series. In addition, this
patch series fixes an upcoming issue in httpd's passwd files caused by
the deprecation of the crypt(3) function.
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 hooks
t/lib-httpd.sh | 51 ++++++++++++++++++---------
t/lib-httpd/passwd | 2 +-
t/lib-httpd/proxy-passwd | 2 +-
t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++--
4 files changed, 45 insertions(+), 22 deletions(-)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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