Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid initializing ncurses before parsing options #385

Closed
wants to merge 1 commit into from

Conversation

johnkeeping
Copy link
Contributor

If called before a view has been set up, report() will call die() which
prints to stderr and exits. However, because init_display() is called
well before the first view is set, this is likely to result in endwin()
being called which will clear the user's terminal meaning that they do
not see the message.

For example, running "tig blame master" results in:

user@host ~/src/tig $ tig blame master
user@host ~/src/tig $d options to blame

tig 2.0.3 (Dec 20 2014)

Usage: tig        [options] [revs] [--] [paths]
   or: tig log    [options] [revs] [--] [paths]
   or: tig show   [options] [revs] [--] [paths]
   or: tig blame  [options] [rev] [--] path
   or: tig grep   [options] [pattern]
   or: tig refs
   or: tig stash
   or: tig status
   or: tig <      [git command output]

Options:
  +<number>       Select line <number> in the first view
  -v, --version   Show version and exit
  -h, --help      Show help message and exit

with the cursor positioned on the second prompt above most of the error
output.

If called before a view has been set up, report() will call die() which
prints to stderr and exits.  However, because init_display() is called
well before the first view is set, this is likely to result in endwin()
being called which will clear the user's terminal meaning that they do
not see the message.

For example, running "tig blame master" results in:

	user@host ~/src/tig $ tig blame master
	user@host ~/src/tig $d options to blame

	tig 2.0.3 (Dec 20 2014)

	Usage: tig        [options] [revs] [--] [paths]
	   or: tig log    [options] [revs] [--] [paths]
	   or: tig show   [options] [revs] [--] [paths]
	   or: tig blame  [options] [rev] [--] path
	   or: tig grep   [options] [pattern]
	   or: tig refs
	   or: tig stash
	   or: tig status
	   or: tig <      [git command output]

	Options:
	  +<number>       Select line <number> in the first view
	  -v, --version   Show version and exit
	  -h, --help      Show help message and exit

with the cursor positioned on the second prompt above most of the error
output.

Signed-off-by: John Keeping <john@keeping.me.uk>
@jonas
Copy link
Owner

jonas commented Feb 28, 2015

What terminal does this happen with?

In any case, I think it would be best to put the lazy initialization code in reset_display().

@johnkeeping
Copy link
Contributor Author

I see the problem with xterm.

Do you mean resize_display()? I can't find reset_display()... I expect you're right that it would be better elsewhere, do you want me to update the PR or are you happy to apply a change of your own?

@jonas
Copy link
Owner

jonas commented Feb 28, 2015

Yes, sorry, I meant resize_display(). I will amend the fix when I merge it. Thanks.

@jonas
Copy link
Owner

jonas commented Mar 2, 2015

It looks like the problem is that done_display() and therefore endwin() is called twice when die() is called. First via die_callback (which always points to done_display() once init_display() has been called), and again by the atexit() handler also installed once init_display() has been called.

atexit() must be there so that ncurses is "shutdown" when Tig exits due to a signal. I therefore propose the following patch which fixes the issue for me.

diff --git a/src/display.c b/src/display.c
index 82d903c..f77bc78 100644
--- a/src/display.c
+++ b/src/display.c
@@ -411,7 +411,9 @@ report(const char *msg, ...)
 static void
 done_display(void)
 {
-   endwin();
+   if (cursed)
+       endwin();
+   cursed = FALSE;
 }

 void

Note that in order to reproduce this issue I had to test in a fresh terminal; once Tig has run it looks like there is some code that makes the bug go away.

@jonas
Copy link
Owner

jonas commented Mar 2, 2015

When you have time could you please give this a try?

@johnkeeping
Copy link
Contributor Author

That fixes it for me. Thanks!

@jonas jonas closed this in 277d5d8 Mar 9, 2015
@jonas
Copy link
Owner

jonas commented Mar 9, 2015

Thanks for testing @johnkeeping

@johnkeeping johnkeeping deleted the no-die-after-display branch March 10, 2015 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants