search

git-for-windows / git

A fork of Git containing Windows-specific patches.
http://gitforwindows.org/
Other
8.43k stars 2.56k forks source link

git gui 2.25 - stage to commit: popup error "wrong # args: should be "ui_status msg" #2469

Closed msuencksen closed 4 years ago

msuencksen commented 4 years ago

Setup

git version 2.25.0.windows.1
cpu: x86_64
built from commit: 7c71c859c97853ed057da5cbab12f3c13b5554df
sizeof-long: 4

Microsoft Windows [Version 10.0.18362.535]
Editor Option: VIM
Custom Editor Path:
Path Option: BashOnly
SSH Option: OpenSSH
Tortoise Option: false
CURL Option: WinSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Disabled

Details

Windows taskbar 

git gui, clicking on menu item "Stage to commit"

The selected files are moved to "Staged changes (will commit)"

A popup error dialog appeared, The "details" button said:

wrong # args: should be "ui_status msg"
wrong # args: should be "ui_status msg"
    while executing
"ui_status [mc "Ready to commit."] ui_ready"
    ("eval" body line 1)
    invoked from within
"eval $callback"
    (procedure "read_diff" line 181)
    invoked from within
"read_diff file3e52170 7 {{} {reshow_diff; ui_status [mc "Ready to commit."] ui_ready}}"

Screenshot:

git 2-25

dscho commented 4 years ago

git gui, clicking on menu item "Stage to commit"

Works over here. Both with the menu item Commit>Stage to commit and with the keyboard shortcut Ctrl+T.

There must be something else going on on your side that does not happen over here. Maybe it depends on the file? Can you reproduce in a totally new worktree with a totally new file?

msuencksen commented 4 years ago

I found out, it occurs only for non-binary files. For text files, I usually get the message "Updating the Git index failed. A rescan will be automatically started to resynchronize git-gui.", for which I always clicked "Continue".

After just reading this, I now have I configured: git config --global core.autocrlf false

That made go away both the index error and the error in this issue.

dscho commented 4 years ago

That's a good find. If I were you, I would report this to the Git mailing list (git@vger.kernel.org, no HTML part please or it will be rejected, and Cc: Pratyush Yadav <me@yadavpratyush.com> AKA @prati0100 i.e. the Git GUI maintainer).

asmwarrior commented 4 years ago

I see this issue in git-for-windows 2.25.0, but I just uninstall this version and install git-for-windows 2.24.2, and I don't see the "Application error dialog". Though I still see the dialog saying "Updating the Git index failed. A rescan will be automatically started to resynchronize git-gui."

See my report here: https://groups.google.com/d/msgid/git-for-windows/f3e8a9d2-b927-437a-9f08-b95904b2925f%40googlegroups.com?utm_medium=email&utm_source=footer

dscho commented 4 years ago

I still see the dialog saying "Updating the Git index failed. A rescan will be automatically started to resynchronize git-gui."

I would be surprised if the underlying issue was introduced recently.

See my report here: https://groups.google.com/d/msgid/git-for-windows/f3e8a9d2-b927-437a-9f08-b95904b2925f%40googlegroups.com?utm_medium=email&utm_source=footer

That is most likely the wrong audience. While the CR/LF issues are traditionally Windows-specific, the underlying cause (worktree/index mismatch) is not. So at least the Git mailing list needs to be involved, more likely even the Git GUI maintainer, @prati0100.

prati0100 commented 4 years ago

I would be surprised if the underlying issue was introduced recently.

We did recently change how ui_status works behind the scenes so I suspect that refactor could be the reason for this.

I'll take a closer look when I can.

prati0100 commented 4 years ago

Hi @msuencksen

Does the below patch fix the problem?

diff --git a/lib/index.tcl b/lib/index.tcl
index 1254145..1179089 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -366,7 +366,7 @@ proc add_helper {txt paths} {
        update_index \
            $txt \
            $path_list \
-           [concat $after {ui_status [mc "Ready to commit."]}]
+           [concat $after {ui_status [mc "Ready to commit."];}]
    }
 }

I haven't tested it but I think this is the bug that causes the error dialog to pop up.

This bug has been lying around for years and it got masked because ui_status used to take another argument called test that was used to only update the status if the current status is equal to test. So this led to ui_ready being passed as the argument test instead of actually being executed like it should have been. ui_status was recently reworked to not have that argument, and this got uncovered.

As for why we even go into rescan failure I'm not sure and will have to look further into it.

The error comes from going into the procedure rescan_on_error. This happens when we fail to close the file descriptor used for reading/writing to the index (which comes from something like the git checkout-index or the git update-index command.

The control flow around the error handler was also recently changed, so that might be the cause of the problem. But since changing crlf settings makes this problem go away it might be something else entirely.

asmwarrior commented 4 years ago

Hi @msuencksen

Does the below patch fix the problem?

diff --git a/lib/index.tcl b/lib/index.tcl
index 1254145..1179089 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -366,7 +366,7 @@ proc add_helper {txt paths} {
      update_index \
          $txt \
          $path_list \
-         [concat $after {ui_status [mc "Ready to commit."]}]
+         [concat $after {ui_status [mc "Ready to commit."];}]
  }
 }

I haven't tested it but I think this is the bug that causes the error dialog to pop up.

I just uninstall the 2.24.1.2 version, and install the 2.25.0 version again, and change the file: C:\Program Files\Git\mingw64\share\git-gui\lib\index.tcl by the above patch, in-fact, only a semicolon is added here. And I see the pop up of the messsage like wrong # args: should be "ui_ready"... is still there.

msuencksen commented 4 years ago

Hello, thank you for investigating!

With the above patch, the "wrong # args" error does not occur for me anymore.

msuencksen commented 4 years ago

As the above change worked for me, but apparently not for asmwarrior (see above), I might add that I didn't use the patch utility to apply the change. I copy-pasted (without the "+") the change into Notepad++ using administrator mode and saved. I am just telling, because I don't know how picky TCL scripting is about newlines or tab formatting.

asmwarrior commented 4 years ago

As the above change worked for me, but apparently not for asmwarrior (see above), I might add that I didn't use the patch utility to apply the change. I copy-pasted (without the "+") the change into Notepad++ using administrator mode and saved. I am just telling, because I don't know how picky TCL scripting is about newlines or tab formatting.

I also did not use the patch utility, I just modify the source file and save it.

prati0100 commented 4 years ago

@asmwarrior

Does the below patch fix the problem?

diff --git a/lib/index.tcl b/lib/index.tcl
index 1254145..1179089 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -366,7 +366,7 @@ proc add_helper {txt paths} {
        update_index \
            $txt \
            $path_list \
-           [concat $after {ui_status [mc "Ready to commit."]}]
+           [concat $after {ui_status [mc "Ready to commit."];}]
    }
 }

I haven't tested it but I think this is the bug that causes the error dialog to pop up.

I just uninstall the 2.24.1.2 version, and install the 2.25.0 version again, and change the file: C:\Program Files\Git\mingw64\share\git-gui\lib\index.tcl by the above patch, in-fact, only a semicolon is added here.

Can you re-check if you put the semicolon in the correct place? Or if possible just use the patch utility.

And I see the pop up of the messsage like wrong # args: should be "ui_ready"... is still there.

If the error is still there, please post the full error log.

asmwarrior commented 4 years ago

Can you re-check if you put the semicolon in the correct place? Or if possible just use the patch utility.

OK, I have re-checked, and this problem is still there. I can reproduce this issue in my two PCs which are all running Windows 7(64bit). The wrong # args: should be "ui_ready"... dialog always happens after the Updating the Git index failed. A rescan will be automatically started to resynchronize git-gui. dialog, whether this small patch is applied or not.

By the way, the Updating the Git index failed. A rescan will be automatically started to resynchronize git-gui. dialog is not always popped up when I have changed some source files which have CRLF EOL. For example, I change some files, and adding the hunk to the index does not pop up such dialog, thus no wrong # args: should be "ui_ready"... dialog pop up further.

And I see the pop up of the messsage like wrong # args: should be "ui_ready"... is still there.

If the error is still there, please post the full error log.

Here is the log when the wrong # args: should be "ui_ready"... dialog pops up.

wrong # args: should be "ui_ready"
wrong # args: should be "ui_ready"
    while executing
"ui_ready ui_ready"
    ("eval" body line 1)
    invoked from within
"eval $callback"
    (procedure "read_diff" line 181)
    invoked from within
"read_diff file43b9560 7 {{} {next_diff; ui_ready ui_ready}}"

The log is the same whether I applied the small patch or not.

asmwarrior commented 4 years ago

I know little about Tcl language. while I see the log says something like:

{{} {next_diff; ui_ready ui_ready}}

then, I guess there is a semicolon missing between the two ui_ready.

I just debugged a while, and I think this line is related to the bug:

in the index.tcl, around line 63:

    $::main_status stop_all
    unlock_index
        rescan [concat $after [list ui_ready]] 0

The last line above, I think there is a semicolon here, so I change the line to:

        rescan [concat $after "; " [list ui_ready]] 0

Then, I don't see the error message now.

prati0100 commented 4 years ago

The last line above, I think there is a semicolon here, so I change the line to

Yes, the missing semicolon seems to be the problem, though you fixed it in a less than ideal way. Yes, that statement should have a semicolon, but it should be something like:

rescan [concat $after {ui_ready;}] 0

And then we should look for another ui_ready that is missing a semicolon.

All that said, I think the larger problem to tackle is to figure out why the index operation is failing in the first place. Does this happen when staging, unstaging, or both?

asmwarrior commented 4 years ago

The last line above, I think there is a semicolon here, so I change the line to

Yes, the missing semicolon seems to be the problem, though you fixed it in a less than ideal way. Yes, that statement should have a semicolon, but it should be something like:

rescan [concat $after {ui_ready;}] 0

The change suggested by you is not correct, it will gives such error log:

wrong # args: should be "ui_ready"
wrong # args: should be "ui_ready"
    while executing
"ui_ready ui_ready"
    ("eval" body line 1)
    invoked from within
"eval $callback"
    (procedure "read_diff" line 181)
    invoked from within
"read_diff file357a980 7 {{} {next_diff; ui_ready ui_ready;}}"

You can see that the semicolon should be between $after and ui_ready. Not at the end of them. So, this works OK.

rescan [concat $after {; ui_ready;}] 0

And then we should look for another ui_ready that is missing a semicolon.

All that said, I think the larger problem to tackle is to figure out why the index operation is failing in the first place. Does this happen when staging, unstaging, or both?

The wrong # args: should be "ui_ready"... error message only happens in the staging process, it only happens after the Updating the Git index failed. message. Both the two messages do not happen in the unstaging process.

prati0100 commented 4 years ago

The change suggested by you is not correct, it will gives such error log

Yes, I know it will give the error. Maybe I didn't express my thoughts properly. While this line itself is incorrect, the real cause of the problem is another similar statement that concats just ui_ready instead of ui_ready;. Once we update both the instances in the same way this error should go away.

The wrong # args: should be "ui_ready"... error message only happens in the staging process, it only happens after the Updating the Git index failed. message. Both the two messages do not happen in the unstaging process.

Thanks for reporting. I'll see if I can figure something out over the weekend. I don't have a dev environment set up on Windows so I'm not sure how successful I'll be though.

Still, I'll send a patch to fix the missing semicolons.

pilot51 commented 4 years ago

I've been having this issue consistently with my work projects since 2.25.0 on two Windows 10 computers.

I was able to predictably reproduce with a clean repository by using Notepad++ to switch line endings (Edit -> EOL Conversion) in a test.txt file with at least one newline and staging. The error occurs with LF, but not CR LF or CR.

Edit: Just installed 2.25.1, which still has this issue.

elion commented 4 years ago

I upgraded to 2.26.0 and still the same problem.

asmwarrior commented 4 years ago

We should wait the maintainer @prati0100 to fix the issue. I have post the workaround in previous comments in this issue.

Isengart commented 4 years ago

I think I found all ui_ready appends which miss a ";" fix.patch.txt (In a git-for-windows install git-gui.sh is located here mingw64\libexec\git-core\git-gui.tcl and the other tcl files here mingw64\share\git-gui\lib)

After fixing them at least for me the error dialog is not shown anymore.

dscho commented 4 years ago

@Isengart how about

That will allow you to submit the patch to the Git mailing list via /submit, and I am sure that @prati0100 will be happy to review the patch there.

If your patch gets accepted, we can always fast-track it into Git for Windows.

Isengart commented 4 years ago

@dscho I followed your guide(thx for this step by step guide) and would now need someones "/allow" for sending it to the mailing list. https://github.com/gitgitgadget/git/pull/607

dscho commented 4 years ago

Done!

prati0100 commented 4 years ago

The proposed fix is here https://github.com/prati0100/git-gui/commit/19195fbd73994d05abaa0a2976143da96b320f47

Some testing reports would be nice before I merge it in.

asmwarrior commented 4 years ago

The proposed fix is here prati0100/git-gui@19195fb

Some testing reports would be nice before I merge it in.

How can I test it?

I see in your commit, you have changed three files. One file is git-gui.sh, but I see I don't have such file under my path C:\Program Files\Git

The other two files are lib/index.tcl and lib/mergetool.tcl, I see I can just copy and paste these two files in my C:\Program Files\Git\mingw64\share\git-gui\lib.

Any ideas?

Thanks.

rimrul commented 4 years ago

that git-gui.sh file seems to be mingw64/libexec/git-core/git-gui.tcl in my Git for Windows installation.

asmwarrior commented 4 years ago

that git-gui.sh file seems to be mingw64/libexec/git-core/git-gui.tcl in my Git for Windows installation.

Thanks. OK, those two files are not exactly the same, (It looks like git-gui.sh is a template file, and some text is replaced when generate the git-gui.tcl. Anyway, I can manually apply the patch to my local mingw64/libexec/git-core/git-gui.tcl.

I just did a test with my steps I mentioned in https://github.com/git-for-windows/git/issues/2469#issuecomment-576713217, and I don't see the error message now. So, I believe the issue is gone.

dscho commented 4 years ago

If anybody wants this in Git for Windows before it hits an official Git version, please just open a PR.

prati0100 commented 4 years ago

Fixed upstream: https://github.com/prati0100/git-gui/commit/88db24d724619d6c2e602c3b56016f59326d31e3

dscho commented 4 years ago

I guess at this stage, there is not enough interest in having this in Git for Windows early: chances are that we'll get it via Git v2.27.0 (which is due June 1st).