mingw: handle absolute paths in expand_user_path() #66

dscho · 2018-11-06T14:41:12Z

In Git for Windows, we ran with a patch "in production" for quite a while where paths starting with a slash (i.e. looking like Unix paths, not like Windows paths) were interpreted as being relative to the runtime prefix, when expanded via expand_user_path().

This was sent to the Git mailing list as a discussion starter, and it was pointed out that this is neither portable nor unambiguous.

After the dust settled, I thought about the presented ideas for a while (quite a while...), ended up with a solution, then adapted it to Junio's preference: any path starting with %(prefix)/ is expanded. This is ambiguous because it could be a valid path. But then, it is unlikely, and if someone really wants to specify such a path, it is easy to slap a ./ in front and they're done.

Changes since v2:

Adjusted to Junio's preference %(...) over <...>. Of course, the preferred preference has the disadvantage of actually being allowed in Win32 filenames, but then, the workaround is as easy as on non-Windows platforms.
Since our convention of %(...) interpolation does not involve uppercase keywords, I now use a lowercase one.
Since this keyword is interpolated to the compiled-in prefix if built without runtime prefix support, I dropped the runtime part of the keyword.
Renamed the expand_user_path() to interpolate_path(), to remove the distraction as to the implementation detail which things get to be interpolated (because we extend it to interpolate more than just a home directory, which might well be unclear from the former name, anyway).
Adjusted the code comment above the interpolate_path() to remove a stale part, clarify another part, and to extend it to talk about the prefix expansion, too.

Changes since v1:

Included a test for the RUNTIME_PREFIX that I had meant to send for ages already, and based on which...
A test case was added to verify that this actually works as intended
It is no longer Windows-specific
I added some documentation

Cc: Ramsay Jones [email protected]
Cc: Duy Nguyen [email protected]
Cc: Junio C Hamano [email protected]
Cc: Johannes Sixt [email protected]
Cc: Jeff King [email protected]
Cc: Eric Sunshine [email protected]
cc: Fabian Stelzer [email protected]

dscho · 2018-11-06T14:50:46Z

/submit

gitgitgadget · 2018-11-06T14:54:01Z

Submitted as [email protected]

Originally, we refrained from adding a regression test in 7b6c649 (system_path(): Add prefix computation at runtime if RUNTIME_PREFIX set, 2008-08-10), and in 226c0dd (exec_cmd: RUNTIME_PREFIX on some POSIX systems, 2018-04-10). The reason was that it was deemed too tricky to test. Turns out that it is not tricky to test at all: we simply create a pseudo-root, copy the `git` executable into the `git/` subdirectory of that pseudo-root, then copy a script into the `libexec/git-core/` directory and expect that to be picked up. As long as the trash directory is in a location where binaries can be executed, this works. Signed-off-by: Johannes Schindelin <[email protected]>

dscho · 2021-07-01T16:01:57Z

/submit

gitgitgadget · 2021-07-01T16:03:20Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-66/dscho/mingw-expand-absolute-user-path-v2

To fetch this version to local tag pr-66/dscho/mingw-expand-absolute-user-path-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-66/dscho/mingw-expand-absolute-user-path-v2

gitgitgadget · 2021-07-01T17:45:45Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> In Git for Windows, we ran with a patch "in production" for quite a while
> where paths starting with a slash (i.e. looking like Unix paths, not like
> Windows paths) were interpreted as being relative to the runtime prefix,
> when expanded via expand_user_path().
>
> This was sent to the Git mailing list as a discussion starter, and it was
> pointed out that this is neither portable nor unambiguous.
>
> After the dust settled, I thought about the presented ideas for a while
> (quite a while...), and ended up with the following: any path starting with
> <RUNTIME-PREFIX>/ is expanded. This is ambiguous because it could be a valid
> path. But then, it is unlikely, and if someone really wants to specify such
> a path, it is easy to slap a ./ in front and they're done.

I just went back to briefly scan the discussion in late 2018.

I think the rough consensus back then was that 

 * It indeed is a problem that there is no syntax for users of
   "relocatable Git" to use to point at things that come as part of
   the "relocatable Git" package.

 * A change to expand_user_path() would be too broad, it makes sense
   for this feature to be in git_config_pathname();

 * We need to get the syntax right.

As to the last item, there were a handful of choices raised:

 - Use "~~"--the downside is that this is not extensible.  Use
   "~runtime-prefix/" would be a better choice (the likelyhood of
   <RUNTIME-PREFIX>, $RUNTIME_PREFIX, and any other random choice
   happens to be used as a valid directory name is just as slim as
   the likelyhood of "runtime-prefix" used as a valid username).

 - "$RUNTIME_PREFIX" to make it read like a variable---the downside
   was that it looked TOO MUCH like a variable and risked user
   confusion (e.g. it may be upsetting that "$HOME/.gitconfig" is
   not expanded like "~/.gitconfig" is).

 - %(RUNTIME-PREFIX) to make it similar to how Git replaces various
   tokens that are understood only in the context of Git.

 - <RUNTIME-PREFIX>---the downside is that this is an unnecessary
   new syntax we do not have to introduce.  If <RUNTIME-PREFIX> is
   unlikely to be used as a valid directory name, %(RUNTIME-PREFIX)
   is just as unlikely.

There might have been other ideas floated back then.  I have to say
that the one you chose in this round is the least favourite of mine,
and if you consulted me privately before redoing this round, I would
probably have said %(RUNTIME_PREFIX) would make the most sense among
the candidates.

Thanks.

In 395de25 (Expand ~ and ~user in core.excludesfile, commit.template, 2009-11-17), the `user_path()` function was refactored into the `expand_user_path()`. During that refactoring, the `buf` parameter was lost, but the code comment above said function still talks about it. Let's remove that stale part of the comment. Signed-off-by: Johannes Schindelin <[email protected]>

The `real_home` parameter only has an effect when expanding paths starting with `~/`, not when expanding paths starting with `~<user>/`. Let's make that clear. Signed-off-by: Johannes Schindelin <[email protected]>

It is not immediately clear what `expand_user_path()` means, so let's rename it to `interpolate_path()`. This also opens the path for interpolating more than just a home directory. Signed-off-by: Johannes Schindelin <[email protected]>

…efix Ever since Git learned to detect its install location at runtime, there was the slightly awkward problem that it was impossible to specify paths relative to said location. For example, if a version of Git was shipped with custom SSL certificates to use, there was no portable way to specify `http.sslCAInfo`. In Git for Windows, the problem was "solved" for years by interpreting paths starting with a slash as relative to the runtime prefix. However, this is not correct: such paths _are_ legal on Windows, and they are interpreted as absolute paths in the same drive as the current directory. After a lengthy discussion, and an even lengthier time to mull over the problem and its best solution, and then more discussions, we eventually decided to introduce support for the magic sequence `%(prefix)/`. If a path starts with this, the remainder is interpreted as relative to the detected (runtime) prefix. If built without runtime prefix support, Git will simply interpolate the compiled-in prefix. If a user _wants_ to specify a path starting with the magic sequence, they can prefix the magic sequence with `./` and voilà, the path won't be expanded. Signed-off-by: Johannes Schindelin <[email protected]>

dscho · 2021-07-24T22:06:06Z

/submit

gitgitgadget · 2021-07-24T22:07:08Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-66/dscho/mingw-expand-absolute-user-path-v3

To fetch this version to local tag pr-66/dscho/mingw-expand-absolute-user-path-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-66/dscho/mingw-expand-absolute-user-path-v3

gitgitgadget · 2021-07-26T17:59:18Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

>  * Since our convention of %(...) interpolation does not involve uppercase
>    keywords, I now use a lowercase one.

Makes sense.

>  * Since this keyword is interpolated to the compiled-in prefix if built
>    without runtime prefix support, I dropped the runtime part of the
>    keyword.

I have this nagging feeling that %(prefix) may be (mistakenly)
expected to interporate to $(git rev-parse --show-prefix).  Of
course, nobody would expect that in paths in the configuration
files, but because we are borrowing %(token) convention from
elsewhere, it is not outragous to imagine that either "for-each-ref"
family or "log" family of placeholders may want to use %(prefix)"
for such purpose (for that matter, they may also be helped to have
the runtime-prefix information available).

Perhaps %(installPrefix) or something may have less chance of making
us regret later.  I am just raising this as a possible problem; I
personally would not be confused if we settled on the %(prefix).

>  * Renamed the expand_user_path() to interpolate_path(), to remove the
>    distraction as to the implementation detail which things get to be
>    interpolated (because we extend it to interpolate more than just a home
>    directory, which might well be unclear from the former name, anyway).

Great.

>  * Adjusted the code comment above the interpolate_path() to remove a stale
>    part, clarify another part, and to extend it to talk about the prefix
>    expansion, too.

These looked good, too.

Thanks.

gitgitgadget · 2021-07-26T22:05:57Z

builtin/credential-cache.c

@@ -90,7 +90,7 @@ static char *get_socket_path(void)
 {


On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]> writes: > From: Johannes Schindelin <[email protected]> > > It is not immediately clear what `expand_user_path()` means, so let's > rename it to `interpolate_path()`. This also opens the path for > interpolating more than just a home directory. > > Signed-off-by: Johannes Schindelin <[email protected]> > --- > ... > diff --git a/cache.h b/cache.h > index ba04ff8bd36..87e4cbe9c5f 100644 > --- a/cache.h > +++ b/cache.h > @@ -1246,7 +1246,7 @@ typedef int create_file_fn(const char *path, void *cb); > int raceproof_create_file(const char *path, create_file_fn fn, void *cb); > > int mkdir_in_gitdir(const char *path); > -char *expand_user_path(const char *path, int real_home); > +char *interpolate_path(const char *path, int real_home); This of course breaks any topic in flight that adds more places to use expand_user_path(). I think Fabian's "ssh signing" is not as ready as this topic, and it can afford to wait by rebasing on top of this topic. By the time "ssh signing" gets into testable shape (right now, it does not pass tests when merged to 'seen'), hopefully the "expand install-prefix" topic may already be in 'next' if not in 'master'. In the meantime, I am adding this band-aid at the tip of this topic to help these two topics play together better. Thanks. diff --git a/cache.h b/cache.h index 87e4cbe9c5..679a27e17c 100644 --- a/cache.h +++ b/cache.h @@ -1247,6 +1247,8 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb); int mkdir_in_gitdir(const char *path); char *interpolate_path(const char *path, int real_home); +/* NEEDSWORK: remove this synonym once in-flight topics have migrated */ +#define expand_user_path interpolate_path const char *enter_repo(const char *path, int strict); static inline int is_absolute_path(const char *path) {

On the Git mailing list, Fabian Stelzer wrote (reply to this):

On 27.07.21 00:05, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <[email protected]> > writes: > >> From: Johannes Schindelin <[email protected]> >> >> It is not immediately clear what `expand_user_path()` means, so let's >> rename it to `interpolate_path()`. This also opens the path for >> interpolating more than just a home directory. >> >> Signed-off-by: Johannes Schindelin <[email protected]> >> --- >> ... >> diff --git a/cache.h b/cache.h >> index ba04ff8bd36..87e4cbe9c5f 100644 >> --- a/cache.h >> +++ b/cache.h >> @@ -1246,7 +1246,7 @@ typedef int create_file_fn(const char *path, void *cb); >> int raceproof_create_file(const char *path, create_file_fn fn, void *cb); >> >> int mkdir_in_gitdir(const char *path); >> -char *expand_user_path(const char *path, int real_home); >> +char *interpolate_path(const char *path, int real_home); > This of course breaks any topic in flight that adds more places to > use expand_user_path(). > > I think Fabian's "ssh signing" is not as ready as this topic, and it > can afford to wait by rebasing on top of this topic. By the time > "ssh signing" gets into testable shape (right now, it does not pass > tests when merged to 'seen'), hopefully the "expand install-prefix" > topic may already be in 'next' if not in 'master'. I think the test problem is not due to my patch. Like Ævar wrote it's "hn/reftable probably interacting with my ab/refs-files-cleanup" [1] The failed tests i can see in [2] are either t0031-reftable.sh or a compile failure referencing reftable as well. If i merge the ssh code into the current seen branch everything works fine for me. Let me know if you have any other results / CI runs that might help. [1] https://lore.kernel.org/git/[email protected]/ [2] https://github.com/git/git/actions/runs/1053603028 > > In the meantime, I am adding this band-aid at the tip of this topic > to help these two topics play together better. > > Thanks. > > > diff --git a/cache.h b/cache.h > index 87e4cbe9c5..679a27e17c 100644 > --- a/cache.h > +++ b/cache.h > @@ -1247,6 +1247,8 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb); > > int mkdir_in_gitdir(const char *path); > char *interpolate_path(const char *path, int real_home); > +/* NEEDSWORK: remove this synonym once in-flight topics have migrated */ > +#define expand_user_path interpolate_path > const char *enter_repo(const char *path, int strict); > static inline int is_absolute_path(const char *path) > {

On the Git mailing list, Junio C Hamano wrote (reply to this):

Fabian Stelzer <[email protected]> writes: >> I think Fabian's "ssh signing" is not as ready as this topic, and it >> can afford to wait by rebasing on top of this topic. By the time >> "ssh signing" gets into testable shape (right now, it does not pass >> tests when merged to 'seen'), hopefully the "expand install-prefix" >> topic may already be in 'next' if not in 'master'. > I think the test problem is not due to my patch. I've been seeing these test failers locally, every time fs/ssh-signing topic is merged to 'seen' (without the reftable thing). Test Summary Report ------------------- t5534-push-signed.sh (Wstat: 256 Tests: 13 Failed: 2) Failed tests: 8, 12 Non-zero exit status: 1 t7528-signed-commit-ssh.sh (Wstat: 256 Tests: 23 Failed: 2) Failed tests: 13, 17 Non-zero exit status: 1 When reftable thing is merged, either compilation fails or t0031 fails, and I suspect that these are not due to the ssh signing topic.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes: > Fabian Stelzer <[email protected]> writes: > >>> I think Fabian's "ssh signing" is not as ready as this topic, and it >>> can afford to wait by rebasing on top of this topic. By the time >>> "ssh signing" gets into testable shape (right now, it does not pass >>> tests when merged to 'seen'), hopefully the "expand install-prefix" >>> topic may already be in 'next' if not in 'master'. >> I think the test problem is not due to my patch. > > I've been seeing these test failers locally, every time > fs/ssh-signing topic is merged to 'seen' (without the reftable > thing). > > Test Summary Report > ------------------- > t5534-push-signed.sh (Wstat: 256 Tests: 13 Failed: 2) > Failed tests: 8, 12 > Non-zero exit status: 1 > t7528-signed-commit-ssh.sh (Wstat: 256 Tests: 23 Failed: 2) > Failed tests: 13, 17 > Non-zero exit status: 1 > > When reftable thing is merged, either compilation fails or t0031 > fails, and I suspect that these are not due to the ssh signing > topic. Interesting. It seems that the failure has some correlation with the use of --root=<trash directory> option. $ sh t5534-push-signed.sh -i ok 1 - setup ok 2 - unsigned push does not send push certificate ok 3 - talking with a receiver without push certificate support ok 4 - push --signed fails with a receiver without push certificate support ok 5 - push --signed=1 is accepted ok 6 - no certificate for a signed push with no update ok 7 - signed push sends push certificate ok 8 - ssh signed push sends push certificate ok 9 - inconsistent push options in signed push not allowed ok 10 - fail without key and heed user.signingkey ok 11 - fail without key and heed user.signingkey x509 ok 12 - fail without key and heed user.signingkey ssh ok 13 - failed atomic push does not execute GPG # passed all 13 test(s) 1..13 passes just fine, but $ TESTPEN=/dev/shm/testpen.$$ $ rm -fr "$TESTPEN" && mkdir "$TESTPEN" $ sh t5534-push-signed.sh --root=$TESTPEN -i -v dies like this: Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/.git/ expecting success of 5534.1 'setup': # main, ff and noff branches pointing at the same commit test_tick && git commit --allow-empty -m initial && git checkout -b noop && git checkout -b ff && git checkout -b noff && # noop stays the same, ff advances, noff rewrites test_tick && git commit --allow-empty --amend -m rewritten && git checkout ff && test_tick && git commit --allow-empty -m second [main (root-commit) 66fe8b3] initial Author: A U Thor <[email protected]> Switched to a new branch 'noop' Switched to a new branch 'ff' Switched to a new branch 'noff' [noff 6391b7f] rewritten Author: A U Thor <[email protected]> Date: Thu Apr 7 15:13:13 2005 -0700 Switched to branch 'ff' [ff 566fbd3] second Author: A U Thor <[email protected]> ok 1 - setup expecting success of 5534.2 'unsigned push does not send push certificate': prepare_dst && mkdir -p dst/.git/hooks && write_script dst/.git/hooks/post-receive <<-\EOF && # discard the update list cat >/dev/null # record the push certificate if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi EOF git push dst noop ff +noff && ! test -f dst/push-cert Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff To dst 66fe8b3..566fbd3 ff -> ff + 66fe8b3...6391b7f noff -> noff (forced update) ok 2 - unsigned push does not send push certificate expecting success of 5534.3 'talking with a receiver without push certificate support': prepare_dst && mkdir -p dst/.git/hooks && write_script dst/.git/hooks/post-receive <<-\EOF && # discard the update list cat >/dev/null # record the push certificate if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi EOF git push dst noop ff +noff && ! test -f dst/push-cert Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff To dst 66fe8b3..566fbd3 ff -> ff + 66fe8b3...6391b7f noff -> noff (forced update) ok 3 - talking with a receiver without push certificate support expecting success of 5534.4 'push --signed fails with a receiver without push certificate support': prepare_dst && mkdir -p dst/.git/hooks && test_must_fail git push --signed dst noop ff +noff 2>err && test_i18ngrep "the receiving end does not support" err Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff fatal: the receiving end does not support --signed push ok 4 - push --signed fails with a receiver without push certificate support expecting success of 5534.5 'push --signed=1 is accepted': prepare_dst && mkdir -p dst/.git/hooks && test_must_fail git push --signed=1 dst noop ff +noff 2>err && test_i18ngrep "the receiving end does not support" err Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff fatal: the receiving end does not support --signed push ok 5 - push --signed=1 is accepted checking prerequisite: GPG mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-GPG" && ( cd "$TRASH_DIRECTORY/prereq-test-dir-GPG" && gpg_version=$(gpg --version 2>&1) test $? != 127 || exit 1 # As said here: http://www.gnupg.org/documentation/faqs.html#q6.19 # the gpg version 1.0.6 did not parse trust packets correctly, so for # that version, creation of signed tags using the generated key fails. case "$gpg_version" in "gpg (GnuPG) 1.0.6"*) say "Your version of gpg (1.0.6) is too buggy for testing" exit 1 ;; *) # Available key info: # * Type DSA and Elgamal, size 2048 bits, no expiration date, # name and email: C O Mitter <[email protected]> # * Type RSA, size 2048 bits, no expiration date, # name and email: Eris Discordia <[email protected]> # No password given, to enable non-interactive operation. # To generate new key: # gpg --homedir /tmp/gpghome --gen-key # To write armored exported key to keyring: # gpg --homedir /tmp/gpghome --export-secret-keys \ # --armor 0xDEADBEEF >> lib-gpg/keyring.gpg # gpg --homedir /tmp/gpghome --export \ # --armor 0xDEADBEEF >> lib-gpg/keyring.gpg # To export ownertrust: # gpg --homedir /tmp/gpghome --export-ownertrust \ # > lib-gpg/ownertrust mkdir "$GNUPGHOME" && chmod 0700 "$GNUPGHOME" && (gpgconf --kill gpg-agent || : ) && gpg --homedir "${GNUPGHOME}" --import \ "$TEST_DIRECTORY"/lib-gpg/keyring.gpg && gpg --homedir "${GNUPGHOME}" --import-ownertrust \ "$TEST_DIRECTORY"/lib-gpg/ownertrust && gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null \ --sign -u [email protected] ;; esac ) gpg: keybox '/dev/shm/testpen.9441/trash directory.t5534-push-signed/gpghome/pubring.kbx' created gpg: /dev/shm/testpen.9441/trash directory.t5534-push-signed/gpghome/trustdb.gpg: trustdb created gpg: key 13B6F51ECDDE430D: public key "C O Mitter <[email protected]>" imported gpg: key 13B6F51ECDDE430D: secret key imported gpg: key 61092E85B7227189: public key "Eris Discordia <[email protected]>" imported gpg: key 61092E85B7227189: secret key imported gpg: key 13B6F51ECDDE430D: "C O Mitter <[email protected]>" not changed gpg: key 61092E85B7227189: "Eris Discordia <[email protected]>" not changed gpg: Total number processed: 4 gpg: imported: 2 gpg: unchanged: 2 gpg: secret keys read: 2 gpg: secret keys imported: 2 gpg: inserting ownertrust of 6 gpg: inserting ownertrust of 3 prerequisite GPG ok expecting success of 5534.6 'no certificate for a signed push with no update': prepare_dst && mkdir -p dst/.git/hooks && write_script dst/.git/hooks/post-receive <<-\EOF && if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi EOF git push dst noop && ! test -f dst/push-cert Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff Everything up-to-date ok 6 - no certificate for a signed push with no update expecting success of 5534.7 'signed push sends push certificate': prepare_dst && mkdir -p dst/.git/hooks && git -C dst config receive.certnonceseed sekrit && write_script dst/.git/hooks/post-receive <<-\EOF && # discard the update list cat >/dev/null # record the push certificate if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi && cat >../push-cert-status <<E_O_F SIGNER=${GIT_PUSH_CERT_SIGNER-nobody} KEY=${GIT_PUSH_CERT_KEY-nokey} STATUS=${GIT_PUSH_CERT_STATUS-nostatus} NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus} NONCE=${GIT_PUSH_CERT_NONCE-nononce} E_O_F EOF git push --signed dst noop ff +noff && ( cat <<-\EOF && SIGNER=C O Mitter <[email protected]> KEY=13B6F51ECDDE430D STATUS=G NONCE_STATUS=OK EOF sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert ) >expect && noop=$(git rev-parse noop) && ff=$(git rev-parse ff) && noff=$(git rev-parse noff) && grep "$noop $ff refs/heads/ff" dst/push-cert && grep "$noop $noff refs/heads/noff" dst/push-cert && test_cmp expect dst/push-cert-status Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff To dst 66fe8b3..566fbd3 ff -> ff + 66fe8b3...6391b7f noff -> noff (forced update) 66fe8b3f2df5c2a6e67944af865f3a0893093d69 566fbd34a75c18947f0bcd052512caf55e7144ba refs/heads/ff 66fe8b3f2df5c2a6e67944af865f3a0893093d69 6391b7f36bc1393eab3cad0aaf8c08cdacbe78fa refs/heads/noff ok 7 - signed push sends push certificate checking prerequisite: GPGSSH mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-GPGSSH" && ( cd "$TRASH_DIRECTORY/prereq-test-dir-GPGSSH" && ssh_version=$(ssh-keygen -Y find-principals -n "git" 2>&1) test $? != 127 || exit 1 echo $ssh_version | grep -q "find-principals:missing signature file" test $? = 0 || exit 1; mkdir -p "${GNUPGHOME}" && chmod 0700 "${GNUPGHOME}" && ssh-keygen -t ed25519 -N "" -f "${GNUPGHOME}/ed25519_ssh_signing_key" >/dev/null && ssh-keygen -t rsa -b 2048 -N "" -f "${GNUPGHOME}/rsa_2048_ssh_signing_key" >/dev/null && ssh-keygen -t ed25519 -N "super_secret" -f "${GNUPGHOME}/protected_ssh_signing_key" >/dev/null && find "${GNUPGHOME}" -name *ssh_signing_key.pub -exec cat {} \; | awk "{print \"\\\"principal with number \" NR \"\\\" \" \$0}" > "${GNUPGHOME}/ssh.all_valid.allowedSignersFile" && cat "${GNUPGHOME}/ssh.all_valid.allowedSignersFile" && ssh-keygen -t ed25519 -N "" -f "${GNUPGHOME}/untrusted_ssh_signing_key" >/dev/null ) "principal with number 1" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIK+hDFZT7sYN3M+1ciMGLTM6pu/xmJrNDTK/vN+QIVnq [email protected] "principal with number 2" ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDWLnqVpDYNstR7jCPKr1FaWzxt7NNw/kEV61GgKThW9S54/p/0WN+SCtUI0j7dCv/NkNhy87WNhohC9rCvZowPf/HRflHZ28vVk5d0DERCmlLBHnTq65Buyzj2R5xMtyVOBBMd+Io6tXk6GfJ6Dvq9l1wIJLv3OodOLBym3idV+8C+GOw9teTOlQWwkZD2hAt0n1Dy5WtaeGm3O2aUwRyg7KuxxmsgadBJ13a9thMYpwkS1McnwB+7oS81VXeeF0WcKAa2tGvxlg8NdBZuvvLJkl5vcISnpjt0NIuhqHzMlBIprpKujkZ5ze18YHXA52w6Y+9hG40n819EKjQfBVtD [email protected] "principal with number 3" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAgtFx51cu1d0gzZOjIdw4M9oBYgV+tX6Bsm2L+riv/Z [email protected] prerequisite GPGSSH ok expecting success of 5534.8 'ssh signed push sends push certificate': prepare_dst && mkdir -p dst/.git/hooks && git -C dst config gpg.ssh.allowedSignersFile "${SIGNING_ALLOWED_SIGNERS}" && git -C dst config receive.certnonceseed sekrit && write_script dst/.git/hooks/post-receive <<-\EOF && # discard the update list cat >/dev/null # record the push certificate if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi && cat >../push-cert-status <<E_O_F SIGNER=${GIT_PUSH_CERT_SIGNER-nobody} KEY=${GIT_PUSH_CERT_KEY-nokey} STATUS=${GIT_PUSH_CERT_STATUS-nostatus} NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus} NONCE=${GIT_PUSH_CERT_NONCE-nononce} E_O_F EOF test_config gpg.format ssh && test_config user.signingkey "${SIGNING_KEY_PRIMARY}" && FINGERPRINT=$(ssh-keygen -lf "${SIGNING_KEY_PRIMARY}" | awk "{print \$2;}") && git push --signed dst noop ff +noff && ( cat <<-\EOF && SIGNER=principal with number 1 KEY=FINGERPRINT STATUS=G NONCE_STATUS=OK EOF sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert ) | sed -e "s|FINGERPRINT|$FINGERPRINT|" >expect && noop=$(git rev-parse noop) && ff=$(git rev-parse ff) && noff=$(git rev-parse noff) && grep "$noop $ff refs/heads/ff" dst/push-cert && grep "$noop $noff refs/heads/noff" dst/push-cert && test_cmp expect dst/push-cert-status Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff To dst 66fe8b3..566fbd3 ff -> ff + 66fe8b3...6391b7f noff -> noff (forced update) 66fe8b3f2df5c2a6e67944af865f3a0893093d69 566fbd34a75c18947f0bcd052512caf55e7144ba refs/heads/ff 66fe8b3f2df5c2a6e67944af865f3a0893093d69 6391b7f36bc1393eab3cad0aaf8c08cdacbe78fa refs/heads/noff --- expect 2021-07-28 00:11:20.863019887 +0000 +++ dst/push-cert-status 2021-07-28 00:11:20.855019156 +0000 @@ -1,4 +1,4 @@ -SIGNER=principal with number 1 +SIGNER=principal with number 3 KEY=SHA256:Szd5rzYOrMBJFTR+gnRUu60YRVqg98UvpcSvmAm89rE STATUS=G NONCE_STATUS=OK not ok 8 - ssh signed push sends push certificate # # prepare_dst && # mkdir -p dst/.git/hooks && # git -C dst config gpg.ssh.allowedSignersFile "${SIGNING_ALLOWED_SIGNERS}" && # git -C dst config receive.certnonceseed sekrit && # write_script dst/.git/hooks/post-receive <<-\EOF && # # discard the update list # cat >/dev/null # # record the push certificate # if test -n "${GIT_PUSH_CERT-}" # then # git cat-file blob $GIT_PUSH_CERT >../push-cert # fi && # # cat >../push-cert-status <<E_O_F # SIGNER=${GIT_PUSH_CERT_SIGNER-nobody} # KEY=${GIT_PUSH_CERT_KEY-nokey} # STATUS=${GIT_PUSH_CERT_STATUS-nostatus} # NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus} # NONCE=${GIT_PUSH_CERT_NONCE-nononce} # E_O_F # # EOF # # test_config gpg.format ssh && # test_config user.signingkey "${SIGNING_KEY_PRIMARY}" && # FINGERPRINT=$(ssh-keygen -lf "${SIGNING_KEY_PRIMARY}" | awk "{print \$2;}") && # git push --signed dst noop ff +noff && # # ( # cat <<-\EOF && # SIGNER=principal with number 1 # KEY=FINGERPRINT # STATUS=G # NONCE_STATUS=OK # EOF # sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert # ) | sed -e "s|FINGERPRINT|$FINGERPRINT|" >expect && # # noop=$(git rev-parse noop) && # ff=$(git rev-parse ff) && # noff=$(git rev-parse noff) && # grep "$noop $ff refs/heads/ff" dst/push-cert && # grep "$noop $noff refs/heads/noff" dst/push-cert && # test_cmp expect dst/push-cert-status #

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes: > Junio C Hamano <[email protected]> writes: > >> Fabian Stelzer <[email protected]> writes: >> >>>> I think Fabian's "ssh signing" is not as ready as this topic, and it >>>> can afford to wait by rebasing on top of this topic. By the time >>>> "ssh signing" gets into testable shape (right now, it does not pass >>>> tests when merged to 'seen'), hopefully the "expand install-prefix" >>>> topic may already be in 'next' if not in 'master'. >>> I think the test problem is not due to my patch. >> >> I've been seeing these test failers locally, every time >> fs/ssh-signing topic is merged to 'seen' (without the reftable >> thing). >> >> Test Summary Report >> ------------------- >> t5534-push-signed.sh (Wstat: 256 Tests: 13 Failed: 2) >> Failed tests: 8, 12 >> Non-zero exit status: 1 >> t7528-signed-commit-ssh.sh (Wstat: 256 Tests: 23 Failed: 2) >> Failed tests: 13, 17 >> Non-zero exit status: 1 >> >> When reftable thing is merged, either compilation fails or t0031 >> fails, and I suspect that these are not due to the ssh signing >> topic. > > Interesting. It seems that the failure has some correlation with > the use of --root=<trash directory> option. > > $ sh t5534-push-signed.sh -i And 7528 fails with --root set to a /dev/shm/ trash directory. So,... this "principal with number #" differences come from the differences in location of the trash directory? Why? > --- expect 2021-07-28 00:11:20.863019887 +0000 > +++ dst/push-cert-status 2021-07-28 00:11:20.855019156 +0000 > @@ -1,4 +1,4 @@ > -SIGNER=principal with number 1 > +SIGNER=principal with number 3 > KEY=SHA256:Szd5rzYOrMBJFTR+gnRUu60YRVqg98UvpcSvmAm89rE > STATUS=G > NONCE_STATUS=OK

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes: > Junio C Hamano <[email protected]> writes: > >> Junio C Hamano <[email protected]> writes: >> >>> Fabian Stelzer <[email protected]> writes: >>> >>>>> I think Fabian's "ssh signing" is not as ready as this topic, and it >>>>> can afford to wait by rebasing on top of this topic. By the time >>>>> "ssh signing" gets into testable shape (right now, it does not pass >>>>> tests when merged to 'seen'), hopefully the "expand install-prefix" >>>>> topic may already be in 'next' if not in 'master'. >>>> I think the test problem is not due to my patch. >>> >>> I've been seeing these test failers locally, every time >>> fs/ssh-signing topic is merged to 'seen' (without the reftable >>> thing). >>> ... >> Interesting. It seems that the failure has some correlation with >> the use of --root=<trash directory> option. >> >> $ sh t5534-push-signed.sh -i > > And 7528 fails with --root set to a /dev/shm/ trash directory. An update. The same failure can be seen _without_ merging the topic to 'seen'. The topic by itself will fail t5534 and t7528 when run with --root= set to somewhere: $ make $ testpen=/dev/shm/testpen.$$ $ rm -fr "$testpen" && mkdir "$testpen" $ cd t $ sh t5534-*.sh --root=$testpen -i $ sh t7528-*.sh --root=$testpen -i on the branch itself, without getting interference by any other topic, should hopefully be an easy enough way to reproduce the problem. Thanks.

On the Git mailing list, Fabian Stelzer wrote (reply to this):

On 28.07.21 07:43, Junio C Hamano wrote: > Junio C Hamano <[email protected]> writes: > >> Junio C Hamano <[email protected]> writes: >> >>> Junio C Hamano <[email protected]> writes: >>> >>>> Fabian Stelzer <[email protected]> writes: >>>> >>>>>> I think Fabian's "ssh signing" is not as ready as this topic, and it >>>>>> can afford to wait by rebasing on top of this topic. By the time >>>>>> "ssh signing" gets into testable shape (right now, it does not pass >>>>>> tests when merged to 'seen'), hopefully the "expand install-prefix" >>>>>> topic may already be in 'next' if not in 'master'. >>>>> I think the test problem is not due to my patch. >>>> I've been seeing these test failers locally, every time >>>> fs/ssh-signing topic is merged to 'seen' (without the reftable >>>> thing). >>>> ... >>> Interesting. It seems that the failure has some correlation with >>> the use of --root=<trash directory> option. >>> >>> $ sh t5534-push-signed.sh -i >> And 7528 fails with --root set to a /dev/shm/ trash directory. > An update. The same failure can be seen _without_ merging the topic > to 'seen'. The topic by itself will fail t5534 and t7528 when run > with --root= set to somewhere: > > $ make > $ testpen=/dev/shm/testpen.$$ > $ rm -fr "$testpen" && mkdir "$testpen" > $ cd t > $ sh t5534-*.sh --root=$testpen -i > $ sh t7528-*.sh --root=$testpen -i > > on the branch itself, without getting interference by any other > topic, should hopefully be an easy enough way to reproduce the > problem. > > Thanks. ok, funny issue. in the ssh test setup i generated a few ssh keys for testing and (wanting to be clever) concatenated them with a prefixed principal into an allowedSigners file using find & awk. Turns out the directory entries in /dev/shm are the other way around. touch ./t1 ./t2 /dev/shm/t1 /dev/shm/t2 find ./ -name 't[0-9]' results in: ./t1 ./t2 a find /dev/shm -name 't[0-9]' returns: /dev/shm/t2 /dev/shm/t1 I'll change the test setup code to do this statically for each key. Not such a good idea to rely on the file order in the dir anyway. Thanks

On the Git mailing list, Junio C Hamano wrote (reply to this):

Fabian Stelzer <[email protected]> writes: > ok, funny issue. in the ssh test setup i generated a few ssh keys for > testing and (wanting to be clever) concatenated them with a prefixed > principal into an allowedSigners file using find & awk. > > Turns out the directory entries in /dev/shm are the other way around. Good finding. Yes, relying on the order "find" discovers filesystem entities is asking for trouble. > I'll change the test setup code to do this statically for each > key. Not such a good idea to rely on the file order in the dir anyway. Thanks.

gitgitgadget · 2021-07-27T07:59:56Z

User Fabian Stelzer <[email protected]> has been added to the cc: list.

dscho · 2021-08-05T12:17:03Z

Integrated into next as of b95a81a

dscho · 2021-08-25T07:49:15Z

Integrated into master as of aab0eea

dscho mentioned this pull request Nov 6, 2018

Pay down technical debt git-for-windows/git#1884

Closed

35 tasks

dscho added the needs-work These patches have pending issues that need to be resolved before submitting label Dec 15, 2018

dscho mentioned this pull request Jul 1, 2021

Allow specifying at runtime which POSIX shell to use git-for-windows/git#3301

Open

dscho force-pushed the mingw-expand-absolute-user-path branch 2 times, most recently from 8ed8f69 to 66df56f Compare July 1, 2021 13:14

dscho removed the needs-work These patches have pending issues that need to be resolved before submitting label Jul 1, 2021

dscho added 4 commits July 24, 2021 23:25

expand_user_path(): clarify the role of the real_home parameter

7b79ba6

The `real_home` parameter only has an effect when expanding paths starting with `~/`, not when expanding paths starting with `~<user>/`. Let's make that clear. Signed-off-by: Johannes Schindelin <[email protected]>

dscho force-pushed the mingw-expand-absolute-user-path branch from 66df56f to d286583 Compare July 24, 2021 21:36

gitgitgadget bot reviewed Jul 26, 2021

View reviewed changes

dscho added next seen labels Aug 5, 2021

dscho mentioned this pull request Aug 14, 2021

Respect http.sslCAInfo when set and validate TLS certs against it git-ecosystem/git-credential-manager#242

Closed

dscho closed this Aug 25, 2021

dscho deleted the mingw-expand-absolute-user-path branch August 25, 2021 07:49

dscho added the master label Aug 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mingw: handle absolute paths in expand_user_path() #66

mingw: handle absolute paths in expand_user_path() #66

dscho commented Nov 6, 2018 •

edited by gitgitgadget bot

Loading

dscho commented Nov 6, 2018

gitgitgadget bot commented Nov 6, 2018

dscho commented Jul 1, 2021

gitgitgadget bot commented Jul 1, 2021

gitgitgadget bot commented Jul 1, 2021

dscho commented Jul 24, 2021

gitgitgadget bot commented Jul 24, 2021

gitgitgadget bot commented Jul 26, 2021

gitgitgadget bot Jul 26, 2021

gitgitgadget bot Jul 27, 2021

gitgitgadget bot Jul 27, 2021

gitgitgadget bot Jul 28, 2021

gitgitgadget bot Jul 28, 2021

gitgitgadget bot Jul 28, 2021

gitgitgadget bot Jul 28, 2021

gitgitgadget bot Jul 28, 2021

gitgitgadget bot commented Jul 27, 2021

dscho commented Aug 5, 2021

dscho commented Aug 25, 2021

mingw: handle absolute paths in expand_user_path() #66

mingw: handle absolute paths in expand_user_path() #66

Conversation

dscho commented Nov 6, 2018 • edited by gitgitgadget bot Loading

dscho commented Nov 6, 2018

gitgitgadget bot commented Nov 6, 2018

dscho commented Jul 1, 2021

gitgitgadget bot commented Jul 1, 2021

gitgitgadget bot commented Jul 1, 2021

dscho commented Jul 24, 2021

gitgitgadget bot commented Jul 24, 2021

gitgitgadget bot commented Jul 26, 2021

gitgitgadget bot Jul 26, 2021

Choose a reason for hiding this comment

gitgitgadget bot Jul 27, 2021

Choose a reason for hiding this comment

gitgitgadget bot Jul 27, 2021

Choose a reason for hiding this comment

gitgitgadget bot Jul 28, 2021

Choose a reason for hiding this comment

gitgitgadget bot Jul 28, 2021

Choose a reason for hiding this comment

gitgitgadget bot Jul 28, 2021

Choose a reason for hiding this comment

gitgitgadget bot Jul 28, 2021

Choose a reason for hiding this comment

gitgitgadget bot Jul 28, 2021

Choose a reason for hiding this comment

gitgitgadget bot commented Jul 27, 2021

dscho commented Aug 5, 2021

dscho commented Aug 25, 2021

dscho commented Nov 6, 2018 •

edited by gitgitgadget bot

Loading