[BUG] 2024.05.09 tests failing on Alpine Linux

[stacyharper]

[BUG] 2024.05.09 tests failing on Alpine Linux

[stacyharper]

@krobelus Okay, I'm willing to pass. Let's forget this? Communication is hard, even more in open-source world. My last comment was rude too, so please accept my apology.

That said, in this specific case we expect a specific output format from git, and we assume this specific format will stay the default. It might be better to explicitely ask for it explicitely by adding a --format=... switch so that we do not break if git decides to change the default down the line.

I agree with this. If this is an easy to add parameter in the test suite? I can't find the correct place to add it on test/tools/git/.

[mawww]

Should be fixed by 8c2775f

[stacyharper]

Hey, thanks for fast reply. I continue to have 2 errors, not the same ones:

https://paste.sr.ht/~stacyharper/f1352d92ed7e2dcb5fd438e96b8e06c56f8f3df8

[stacyharper]

Okay this one is probably related to my personal git configs, or git wrappers. But it is susprise me that it affect the tests.

[krobelus]

something like git config --global format.pretty fuller is already ignored; it must be something else on your system (ancient git version?)

[stacyharper]

I got a wrap around git that might cause this: https://git.sr.ht/~stacyharper/dotfiles/tree/master/item/bin/git

[krobelus]

On Sat, May 11, 2024 at 07:51:35AM -0700, Willow wrote: I got a wrap around git that might cause this: https://git.sr.ht/~stacyharper/dotfiles/tree/master/item/bin/git

yeah don't redefine the meaning of `git show`. Better use `git config`, or an alias, or name it something other than `git`

[mawww]

We should probably export GIT_CONFIG_SYSTEM="" and GIT_CONFIG_GLOBAL="" to ensure user git config does not apply to tests

EDIT: seems we already do that, how come this did not work ? Although we use GIT_CONFIG_NOSYSTEM which has been replaced in git 2.32 with GIT_CONFIG_SYSTEM

[krobelus]

EDIT: seems we already do that, how come this did not work ?

It does work. But they overrode git with a wrapper. User error.

#!/bin/sh

case "$1" in
show)
shift
exec /usr/bin/git show --format=fuller --notes "$@"
;;
esac

exec /usr/bin/git "$@"

[mawww]

Understood, I dont think we can do anything on Kakoune's side for that. I believe the rest of the tests should be fixed now.

[stacyharper]

yeah don't redefine the meaning of git show. Better use git config, or an alias, or name it something other than git

IIRC my problem was that it is currently not possible to define a different format for different git subcommands

Understood, I dont think we can do anything on Kakoune's side for that. I believe the rest of the tests should be fixed now.

The only solution would be to use /usr/bin/git but this then make it impossible for the user to wrap it. Maybe it is possible to do so just for the test suite? (ex: use a specific PATH)

[krobelus]

yeah don't redefine the meaning of git show. Better use git config, or an alias, or name it something other than git

IIRC my problem was that it is currently not possible to define a different format for different git subcommands

No. That's not a problem, it's a feature. You must never completely override git to force valid commands to do something else. Git does not allow to define aliases for built-in commands like git show because of exactly this type of breakage.

You wouldn't override /bin/sh with different behavior and expect things to work.

Again, what you want is either a different name or configure Git to do what you want, like:

git config --global format.pretty fuller

This configuration mechanism does exactly the same thing except it's clearly scoped and can thus be feasibly overridden by our test suite which wants predictable behavior.

Understood, I dont think we can do anything on Kakoune's side for that.

It's not that we can't do anything but we should not encourage users to break their systems.

I believe the rest of the tests should be fixed now.

Does that mean that they pass for you?

The only solution would be to use /usr/bin/git but this then make it impossible for the user to wrap it. Maybe it is possible to do so just for the test suite? (ex: use a specific PATH)

We should not circumvent $PATH. A wrapper may do valid things (like add logging etc.).

[stacyharper]

Again, what you want is either a different name or configure Git to do what you want, like:

Really, don't tell people what they wants... I never asked for your opinion. I don't even ask for a fix. I just pointed that in those situations, kakoune tests fails.

It's not that we can't do anything but we should not encourage users to break their systems.

Really? Break their system?

Right, anyway I don't want to continue debating with you. You've been rude here, and on the IRC channel too.

[krobelus]

Again, what you want is either a different name or configure Git to do what you want, like:

Really, don't tell people what they wants...

That's a misunderstanding; in my book "what you want" means "this is how you can achieve the same thing without this problem" which I think is useful, factual information.

I never asked for your opinion.

I was trying to share facts, not opinions.

I don't even ask for a fix. I just pointed that in those situations, kakoune tests fails.

Ok. If I were in your shoes I'd want to fix it.

It's not that we can't do anything but we should not encourage users to break their systems.

Really? Break their system?

Yes.

Right, anyway I don't want to continue debating with you. You've been rude here,

The main reason I was yelling is because I didn't think I got my point across.

and on the IRC channel too.

This is the conversation

│17:53:34   staceee | https://paste.sr.ht/~stacyharper/1afa4893867a586582b4d1e8a9d81f50b3402
│                   | afb
│17:53:47   staceee | a bit unreadable because of the vt colors..
│23:37:54  krobelus | staceee: on alpine/edge I can't reproduce the blame-jump failure; did
│                   | you install perl?  
│23:38:20  krobelus | I had always thought git depends on perl but not on alpine
...
│09:31:27  krobelus | staceee: I'm not sure why you get line 13 instead of 15. Can you share
│                   | the git show output?
│13:28:18   staceee | krobelus: I might have some patch above of origin/master
│13:39:28   staceee | I continue to have errors
│15:23:34  krobelus | staceee: again, that's not enough information for me
│15:27:39   staceee | I update the same github ticket
│15:27:42   staceee | krobelus ^
│15:34:27  krobelus | staceee: there's no steps to reproduce; what am I supposed to do?

If I had spent more time I would have phrased my requests for information in a more elaborate way.
But after my initial requests bore no useful reponse, I tend to be less motivated to do that.
Sorry if the ego is hurting. Mine is too when people don't seem to understand me.

[mawww]

I am sure we could work around your specific use case, but other wrapping issues would come out eventually and we would have to pile more and more brittle hacks to make those work.

I'd rather assume that if Kakoune calls perl it gets a binary that can interpret perl scripts and follows perl's documented switches.

That said, in this specific case we expect a specific output format from git, and we assume this specific format will stay the default. It might be better to explicitely ask for it explicitely by adding a --format=... switch so that we do not break if git decides to change the default down the line.

[krobelus]

Okay cool. The git-show command is defined in git.kak as $show_diff = "show $sha";
The test could set the format via git config or inject a wrapper around git.
We could also relax the test assertion by allowing any line number (this test doesn't care).
Unfortunately this naive attempt wil make failure messages much harder to read:

diff --git a/test/run b/test/run
index a65f53967..f98dae126 100755
--- a/test/run
+++ b/test/run
@@ -143,6 +143,17 @@ assert_eq() {
   fi
 }
 
+assert_matches() {
+  if ! expr match "$2" "$1" >/dev/null; then
+    fail_ifn
+    printf "${indent}  ${red}- regex  %s\n${indent}  ${green}+ actual %s${none}\n" "$1" "$2"
+  fi
+}
+
+regex_escape() {
+  printf %s "$1" | sed 's/[.[\\*|]/\\&/g'
+}
+
 show_diff() {
   diff -u $1 $2 | while IFS='' read -r line; do
     first_character=$(printf '%s\n' "$line" | cut -b 1)
diff --git a/test/tools/git/blame-jump-message/script b/test/tools/git/blame-jump-message/script
index 9f6fb6e0f..fc6bc9294 100644
--- a/test/tools/git/blame-jump-message/script
+++ b/test/tools/git/blame-jump-message/script
@@ -8,7 +8,7 @@ expected_subject=$(cat <<'EOF'
 EOF
 )
 expected_subject_json=\"$(printf '%s' "$expected_subject" | sed 's/"/\\"/g')\"
-expected_draw_status='{ "jsonrpc": "2.0", "method": "draw_status", "params": [[{ "face": { "fg": "black", "bg": "yellow", "underline": "default", "attributes": [] }, "contents": '"$expected_subject_json"' }], [{ "face": { "fg": "default", "bg": "default", "underline": "default", "attributes": [] }, "contents": "*git* 13:2 " }, { "face": { "fg": "black", "bg": "yellow", "underline": "default", "attributes": [] }, "contents": "[scratch]" }, { "face": { "fg": "default", "bg": "default", "underline": "default", "attributes": [] }, "contents": " " }, { "face": { "fg": "blue", "bg": "default", "underline": "default", "attributes": [] }, "contents": "1 sel" }, { "face": { "fg": "default", "bg": "default", "underline": "default", "attributes": [] }, "contents": " - client0@[kak-tests]" }], { "fg": "cyan", "bg": "default", "underline": "default", "attributes": [] }] }'
+expected_draw_status=$(regex_escape '{ "jsonrpc": "2.0", "method": "draw_status", "params": [[{ "face": { "fg": "black", "bg": "yellow", "underline": "default", "attributes": [] }, "contents": '"$expected_subject_json"' }], [{ "face": { "fg": "default", "bg": "default", "underline": "default", "attributes": [] }, "contents": "*git* ')'[0-9]*'$(regex_escape ':2 " }, { "face": { "fg": "black", "bg": "yellow", "underline": "default", "attributes": [] }, "contents": "[scratch]" }, { "face": { "fg": "default", "bg": "default", "underline": "default", "attributes": [] }, "contents": " " }, { "face": { "fg": "blue", "bg": "default", "underline": "default", "attributes": [] }, "contents": "1 sel" }, { "face": { "fg": "default", "bg": "default", "underline": "default", "attributes": [] }, "contents": " - client0@[kak-tests]" }], { "fg": "cyan", "bg": "default", "underline": "default", "attributes": [] }] }')
 
-assert_eq "$expected_draw_status" "$actual_draw_status"
+assert_matches "$expected_draw_status" "$actual_draw_status"
 ui_out -ignore 2