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.

Sign up for GitHub

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

Jump to bottom

git-gui: revert untracked files by deleting them #436

Closed
wants to merge 3 commits into from
Closed

git-gui: revert untracked files by deleting them #436

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da1704c
git-gui: consolidate naming conventions
logiclrd Oct 30, 2019
8fe9dfc
git-gui: update status bar to track operations
logiclrd Oct 30, 2019
5f8120f
git-gui: revert untracked files by deleting them
logiclrd Nov 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 15 additions & 12 deletions git-gui.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1797,10 +1797,10 @@ proc ui_status {msg} {
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

Hi Jonathan,

Thanks for the re-roll.

On 24/11/19 08:37PM, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
> 
> Update the status bar to track updates as individual "operations" that
> can overlap. Update all call sites to interact with the new status bar
> mechanism. Update initialization to explicitly clear status text,
> since otherwise it may persist across future operations.
> 
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
>  git-gui.sh          |   7 +-
>  lib/blame.tcl       |  22 +++--
>  lib/checkout_op.tcl |  15 +--
>  lib/index.tcl       |  31 +++---
>  lib/merge.tcl       |  14 ++-
>  lib/status_bar.tcl  | 228 +++++++++++++++++++++++++++++++++++++++-----
>  6 files changed, 260 insertions(+), 57 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 0d21f5688b..db02e399e7 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1797,10 +1797,10 @@ proc ui_status {msg} {
>  	}
>  }
>  
> -proc ui_ready {{test {}}} {
> +proc ui_ready {} {

This is not quite correct. There is one user of 'ui_ready' that uses 
'test'. It is in git-gui.sh:2211. It is used when starting gitk. This 
change breaks that call. 10 seconds after opening gitk via the 
"Visualise master's history" option, I get the following error:

  wrong # args: should be "ui_ready"
      while executing
  "ui_ready $starting_gitk_msg"
      ("after" script)
 
The code that calls it (git-gui.sh:2211) looks like:

  ui_status $::starting_gitk_msg
  after 10000 {
  	ui_ready $starting_gitk_msg
  }

I am not quite sure why this is done though. It was introduced in 
e210e67 (git-gui: Corrected keyboard bindings on Windows, improved state 
management., 2006-11-06) [0], but the commit message doesn't really 
explain why (probably because it is a small part of a larger change, 
though it doesn't really fit in with the topic of the change). I can't 
find a mailing list thread about the commit so I don't think we'll ever 
know for sure.

From looking at it, my guess is that it was added because gitk took a 
long time to start up (maybe it still does, but for me its almost 
instant). And so, this message was shown for 10 seconds, and then 
cleared because by then it probably would have started. But to avoid 
over-writing some other message, 'test' was used to make sure only the 
message intended to be cleared is cleared.

I'm not sure if this heuristic/hack is really needed, and that we need 
to keep the "Starting gitk..." message around for 10 seconds. The way I 
see it, it doesn't add too much value unless gitk takes a long time to 
start up on other platforms or repos. In that case an indication of 
"we're working on starting gitk" would be nice. Otherwise, I don't mind 
seeing this go. And even then, I think it is gitk's responsibility to 
give some sort of indication to the user that it is booting up, and not 
ours.

So, I vote for just getting rid of this hack.

>  	global main_status
>  	if {[info exists main_status]} {
> -		$main_status show [mc "Ready."] $test
> +		$main_status show [mc "Ready."]
>  	}
>  }
>  
> @@ -4159,6 +4159,9 @@ if {$picked && [is_config_true gui.autoexplore]} {
>  	do_explore
>  }
>  
> +# Clear "Initializing..." status
> +after 500 {$main_status show ""}
> +
>  # Local variables:
>  # mode: tcl
>  # indent-tabs-mode: t
> diff --git a/lib/blame.tcl b/lib/blame.tcl
> index a1aeb8b96e..888f98bab2 100644
> --- a/lib/blame.tcl
> +++ b/lib/blame.tcl
> @@ -24,6 +24,7 @@ field w_cviewer  ; # pane showing commit message
>  field finder     ; # find mini-dialog frame
>  field gotoline   ; # line goto mini-dialog frame
>  field status     ; # status mega-widget instance
> +field status_operation ; # status operation

Nitpick: The comment doesn't give any information the field name doesn't 
already give. Either remove it or replace it with something more 
descriptive.

>  field old_height ; # last known height of $w.file_pane
>  
>  
> @@ -274,6 +275,7 @@ constructor new {i_commit i_path i_jump} {
>  	pack $w_cviewer -expand 1 -fill both
>  
>  	set status [::status_bar::new $w.status]
> +	set status_operation {}
>  
>  	menu $w.ctxm -tearoff 0
>  	$w.ctxm add command \
> @@ -602,16 +604,21 @@ method _exec_blame {cur_w cur_d options cur_s} {
>  	} else {
>  		lappend options $commit
>  	}
> +
> +	# We may recurse in from another call to _exec_blame and already have
> +	# a status operation.

Thanks for being thorough enough to spot this :)

> +	if {$status_operation == {}} {
> +		set status_operation [$status start \
> +			$cur_s \
> +			[mc "lines annotated"]]

The call to this method from '_read_blame' specifies a different $cur_s. 
So shouldn't we be destroying $status_operation (after stopping it), and 
creating a new one?

> +	}
> +
>  	lappend options -- $path
>  	set fd [eval git_read --nice blame $options]
>  	fconfigure $fd -blocking 0 -translation lf -encoding utf-8
>  	fileevent $fd readable [cb _read_blame $fd $cur_w $cur_d]
>  	set current_fd $fd
>  	set blame_lines 0
> -
> -	$status start \
> -		$cur_s \
> -		[mc "lines annotated"]
>  }
>  
>  method _read_blame {fd cur_w cur_d} {

You did not update 'lib/choose_repository.tcl'. It still uses the old 
version of the status bar. Other than that, the rest of the patch looks 
good. Thanks.

[0]:
  Curiously, if I do 'git log -L 2208,+5:git-gui.sh' to find the origins 
  of the line, it leads me to the commit 25476c6. And looking at the 
  commit, it does indeed appear to be the origin of the line since the 
  line is in the post-image, and not the pre-image. But I accidentally 
  noticed the line in a parent of that commit. Looking further, it turns 
  out the line originated in e210e67. Probably a bug in some really old 
  versions of git. Interesting nonetheless.

-- 
Regards,
Pratyush Yadav

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jonathan Gilbert wrote (reply to this):

On Wed, Nov 27, 2019 at 3:59 PM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> On 24/11/19 08:37PM, Jonathan Gilbert via GitGitGadget wrote:
> > -proc ui_ready {{test {}}} {
> > +proc ui_ready {} {
>
> This is not quite correct. There is one user of 'ui_ready' that uses
> 'test'. It is in git-gui.sh:2211. It is used when starting gitk. This
> change breaks that call. 10 seconds after opening gitk via the
> "Visualise master's history" option, I get the following error:
>
>   wrong # args: should be "ui_ready"
>       while executing
>   "ui_ready $starting_gitk_msg"
>       ("after" script)
[..]
> I'm not sure if this heuristic/hack is really needed, and that we need
> to keep the "Starting gitk..." message around for 10 seconds.
[..]
> So, I vote for just getting rid of this hack.

Oh geeze, I can't believe I missed this. This looks like it ought to
be relatively straightforward to port to the new operations, though,
which is a more isolated approach (keeping this change's fingers where
they belong), and then the operation provides segregation that means
it can just be ended after X seconds without caring what anything else
might be doing with the status bar. We can independently figure out if
we want to restructure that part. Given that computers are faster now
and that the status bar could end up doing something else in the
meantime (well let's be realistic, probably not, but who knows :-) ),
I'd vote for reducing the time the message is shown from 10 seconds
to, I dunno, 3 or 4 seconds.

One other thing I note is that both `do_gitk` and `do_git_gui` use
`$starting_gitk_msg`, which means that when `do_git_gui` is invoked to
launch a Git Gui child process for a submodule, it will be setting the
status bar text to say that it is launching Gitk.

Speaking of things that are out of scope for this PR, I did notice
this in the code:

> # -- Always start git gui through whatever we were loaded with.  This
> #    lets us bypass using shell process on Windows systems.
> #
> set exe [list [_which git]]

As far as I can tell, there's virtually no connection between the
comment and what the code is actually doing. I haven't yet figured out
what exactly it is or where it comes from, but on my Windows systems,
`git-gui` is actually an EXE file `git-gui.exe`, and I _think_ what it
is doing is running `wish.exe`, which I'm guessing has something to do
with hosting a Tcl interpreter with Win32 support for Tk GUI.

I'm not sure whether the code is doing the right thing here or not,
but I'm pretty sure what it's _not_ doing is figuring out how the
current `git-gui` process was started/is being hosted. :-P

> >  field finder     ; # find mini-dialog frame
> >  field gotoline   ; # line goto mini-dialog frame
> >  field status     ; # status mega-widget instance
> > +field status_operation ; # status operation
>
> Nitpick: The comment doesn't give any information the field name doesn't
> already give. Either remove it or replace it with something more
> descriptive.

Hmm, okay. I didn't want something that felt wildly imbalanced with
respect to the other lines, but you're right that this particular line
is literally just repeating the variable name. :-P

> > +     if {$status_operation == {}} {
> > +             set status_operation [$status start \
> > +                     $cur_s \
> > +                     [mc "lines annotated"]]
>
> The call to this method from '_read_blame' specifies a different $cur_s.
> So shouldn't we be destroying $status_operation (after stopping it), and
> creating a new one?

We can change the text by calling `$status_operation show`.

> >  method _read_blame {fd cur_w cur_d} {
>
> You did not update 'lib/choose_repository.tcl'. It still uses the old
> version of the status bar. Other than that, the rest of the patch looks
> good. Thanks.

Ugh, I can't believe I overlooked this. I was aware of the file using
the status bar, because it's the one place that uses the `two_line`
constructor, but then I forgot to actually make it create and use the
(single concurrent) operation that a `two_line`-er is allowed.

The code in there seems to overload the purpose of the `o_cons`
variable, so that sometimes it is pointing at a status bar and
sometimes it is pointing at whatever `console::embed` returns. I will
change this.

This code also depends heavily on `update` to keep the UI active,
which as I understand it is problematic because it could potentially
result in re-entrance since the user can interact with the UI in the
middle of the operation. I will not make any attempt to change this,
though. :-)

> [0]:
>   Curiously, if I do 'git log -L 2208,+5:git-gui.sh' to find the origins
>   of the line, it leads me to the commit 25476c6. And looking at the
>   commit, it does indeed appear to be the origin of the line since the
>   line is in the post-image, and not the pre-image. But I accidentally
>   noticed the line in a parent of that commit. Looking further, it turns
>   out the line originated in e210e67. Probably a bug in some really old
>   versions of git. Interesting nonetheless.

In e210e67, I see this:

set starting_gitk_msg {Please wait... Starting gitk...}
proc do_gitk {} {
        global tcl_platform ui_status_value starting_gitk_msg

        set ui_status_value $starting_gitk_msg
        after 5000 {
                if {$ui_status_value == $starting_gitk_msg} {
                        set ui_status_value {Ready.}
                }
        }
        ...

In 043f7011, all string comparisons were changed from ==/!= to eq/ne.
The commit message explains that when you use == and !=, Tcl will
attempt to convert either side to numeric if one of the two sides
looks like a numeric. Guess I should review my commit for this error
:-P

-                if {$ui_status_value == $starting_gitk_msg} {
+                if {$ui_status_value eq $starting_gitk_msg} {

In 699d5601 "Refactor our ui_status_value update technique", this became:

set starting_gitk_msg [mc "Starting gitk... please wait..."]
...
        global ... starting_gitk_msg
...
        ui_status $starting_gitk_msg
        after 10000 {
                ui_ready $starting_gitk_msg
        }

Finally it became this in 02efd48f, apparently an unrelated
refactoring removed the global variable declaration:

set starting_gitk_msg [mc "Starting gitk... please wait..."]
...
        ui_status $::starting_gitk_msg
        after 10000 {
                ui_ready $starting_gitk_msg
        }

I gathered this information using Git Gui's blame function, which I
guess is a good demonstration that my latest blame.tcl revision
corrects the problems in the earlier submission :-D

Next revision coming soon.

Jonathan Gilbert

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Benjamin Poirier wrote (reply to this):

On 2019/12/01 02:28 +0000, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
> 
> Update the status bar to track updates as individual "operations" that
> can overlap. Update all call sites to interact with the new status bar
> mechanism. Update initialization to explicitly clear status text,
> since otherwise it may persist across future operations.
> 
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---

Hi Jonathan,

It appears that this change has caused a regression when using git-gui
blame <file> -> right click on a source line -> "Show History Context"

There is an "Application Error" window that appears with the following
details:
can't read "::main_status": no such variable
can't read "::main_status": no such variable
    while executing
"$::main_status  start  [mc "Starting %s... please wait..." "gitk"]"
    (procedure "do_gitk" line 55)
    invoked from within
"do_gitk $cmdline"
    (procedure "blame::_gitkcommit" line 47)
    invoked from within
"blame::_gitkcommit ::blame::__o1::__d"
    invoked from within
".ctxm invoke active"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 [list $w invoke active]"
    (procedure "tk::MenuInvoke" line 50)
    invoked from within
"tk::MenuInvoke .ctxm 1"
    (command bound to event)

The rest of the functionality seems unaffected but it's pretty annoying
to have to dismiss this message each time.

Can you please look into it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

Hi Benjamin,

On 26/02/20 05:24PM, Benjamin Poirier wrote:
> On 2019/12/01 02:28 +0000, Jonathan Gilbert via GitGitGadget wrote:
> > From: Jonathan Gilbert <JonathanG@iQmetrix.com>
> > 
> > Update the status bar to track updates as individual "operations" that
> > can overlap. Update all call sites to interact with the new status bar
> > mechanism. Update initialization to explicitly clear status text,
> > since otherwise it may persist across future operations.
> > 
> > Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> > ---
> 
> Hi Jonathan,
> 
> It appears that this change has caused a regression when using git-gui
> blame <file> -> right click on a source line -> "Show History Context"
> 
> There is an "Application Error" window that appears with the following
> details:
> can't read "::main_status": no such variable
> can't read "::main_status": no such variable
>     while executing
> "$::main_status  start  [mc "Starting %s... please wait..." "gitk"]"
>     (procedure "do_gitk" line 55)
>     invoked from within
> "do_gitk $cmdline"
>     (procedure "blame::_gitkcommit" line 47)
>     invoked from within
> "blame::_gitkcommit ::blame::__o1::__d"
>     invoked from within
> ".ctxm invoke active"
>     ("uplevel" body line 1)
>     invoked from within
> "uplevel #0 [list $w invoke active]"
>     (procedure "tk::MenuInvoke" line 50)
>     invoked from within
> "tk::MenuInvoke .ctxm 1"
>     (command bound to event)
> 
> The rest of the functionality seems unaffected but it's pretty annoying
> to have to dismiss this message each time.
> 
> Can you please look into it?

Does the following patch fix the problem? I will submit a proper patch 
soon.

-----8<-----
diff --git a/git-gui.sh b/git-gui.sh
index f41ed2e..d939844 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2205,11 +2205,13 @@ proc do_gitk {revs {is_submodule false}} {
 		set env(GIT_WORK_TREE) $_gitworktree
 		cd $pwd

-		set status_operation [$::main_status \
-			start \
-			[mc "Starting %s... please wait..." "gitk"]]
+		if {[info exists main_status]} {
+			set status_operation [$::main_status \
+				start \
+				[mc "Starting %s... please wait..." "gitk"]]

-		after 3500 [list $status_operation stop]
+			after 3500 [list $status_operation stop]
+		}
 	}
 }
-----8<-----

-- 
Regards,
Pratyush Yadav

}

proc ui_ready {{test {}}} {
proc ui_ready {} {
global main_status
if {[info exists main_status]} {
$main_status show [mc "Ready."] $test
$main_status show [mc "Ready."]
}
}

Expand Down Expand Up @@ -2150,8 +2150,6 @@ proc incr_font_size {font {amt 1}} {
##
## ui commands

set starting_gitk_msg [mc "Starting gitk... please wait..."]

proc do_gitk {revs {is_submodule false}} {
global current_diff_path file_states current_diff_side ui_index
global _gitdir _gitworktree
Expand Down Expand Up @@ -2206,10 +2204,11 @@ proc do_gitk {revs {is_submodule false}} {
set env(GIT_WORK_TREE) $_gitworktree
cd $pwd

ui_status $::starting_gitk_msg
after 10000 {
ui_ready $starting_gitk_msg
}
set status_operation [$::main_status \
start \
[mc "Starting %s... please wait..." "gitk"]]

after 3500 [list $status_operation stop]
}
}

Expand Down Expand Up @@ -2240,10 +2239,11 @@ proc do_git_gui {} {
set env(GIT_WORK_TREE) $_gitworktree
cd $pwd

ui_status $::starting_gitk_msg
after 10000 {
ui_ready $starting_gitk_msg
}
set status_operation [$::main_status \
start \
[mc "Starting %s... please wait..." "git-gui"]]

after 3500 [list $status_operation stop]
}
}

Expand Down Expand Up @@ -4159,6 +4159,9 @@ if {$picked && [is_config_true gui.autoexplore]} {
do_explore
}

# Clear "Initializing..." status
after 500 {$main_status show ""}

# Local variables:
# mode: tcl
# indent-tabs-mode: t
Expand Down
24 changes: 17 additions & 7 deletions lib/blame.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ field w_cviewer ; # pane showing commit message
field finder ; # find mini-dialog frame
field gotoline ; # line goto mini-dialog frame
field status ; # status mega-widget instance
field status_operation ; # operation displayed by status mega-widget
field old_height ; # last known height of $w.file_pane


Expand Down Expand Up @@ -274,6 +275,7 @@ constructor new {i_commit i_path i_jump} {
pack $w_cviewer -expand 1 -fill both

set status [::status_bar::new $w.status]
set status_operation {}

menu $w.ctxm -tearoff 0
$w.ctxm add command \
Expand Down Expand Up @@ -602,16 +604,23 @@ method _exec_blame {cur_w cur_d options cur_s} {
} else {
lappend options $commit
}

# We may recurse in from another call to _exec_blame and already have
# a status operation.
if {$status_operation == {}} {
set status_operation [$status start \
$cur_s \
[mc "lines annotated"]]
} else {
$status_operation restart $cur_s
}

lappend options -- $path
set fd [eval git_read --nice blame $options]
fconfigure $fd -blocking 0 -translation lf -encoding utf-8
fileevent $fd readable [cb _read_blame $fd $cur_w $cur_d]
set current_fd $fd
set blame_lines 0

$status start \
$cur_s \
[mc "lines annotated"]
}

method _read_blame {fd cur_w cur_d} {
Expand Down Expand Up @@ -806,10 +815,11 @@ method _read_blame {fd cur_w cur_d} {
[mc "Loading original location annotations..."]
} else {
set current_fd {}
$status stop [mc "Annotation complete."]
$status_operation stop [mc "Annotation complete."]
set status_operation {}
}
} else {
$status update $blame_lines $total_lines
$status_operation update $blame_lines $total_lines
}
} ifdeleted { catch {close $fd} }

Expand Down Expand Up @@ -1124,7 +1134,7 @@ method _blameparent {} {
set diffcmd [list diff-tree --unified=0 $cparent $cmit -- $new_path]
}
if {[catch {set fd [eval git_read $diffcmd]} err]} {
$status stop [mc "Unable to display parent"]
$status_operation stop [mc "Unable to display parent"]
error_popup [strcat [mc "Error loading diff:"] "\n\n$err"]
return
}
Expand Down
15 changes: 8 additions & 7 deletions lib/checkout_op.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,9 +341,9 @@ method _readtree {} {
global HEAD

set readtree_d {}
$::main_status start \
set status_bar_operation [$::main_status start \
[mc "Updating working directory to '%s'..." [_name $this]] \
[mc "files checked out"]
[mc "files checked out"]]

set fd [git_read --stderr read-tree \
-m \
Expand All @@ -354,26 +354,27 @@ method _readtree {} {
$new_hash \
]
fconfigure $fd -blocking 0 -translation binary
fileevent $fd readable [cb _readtree_wait $fd]
fileevent $fd readable [cb _readtree_wait $fd $status_bar_operation]
}

method _readtree_wait {fd} {
method _readtree_wait {fd status_bar_operation} {
global current_branch

set buf [read $fd]
$::main_status update_meter $buf
$status_bar_operation update_meter $buf
append readtree_d $buf

fconfigure $fd -blocking 1
if {![eof $fd]} {
fconfigure $fd -blocking 0
$status_bar_operation stop
return
}

if {[catch {close $fd}]} {
set err $readtree_d
regsub {^fatal: } $err {} err
$::main_status stop [mc "Aborted checkout of '%s' (file level merging is required)." [_name $this]]
$status_bar_operation stop [mc "Aborted checkout of '%s' (file level merging is required)." [_name $this]]
warn_popup [strcat [mc "File level merge required."] "

$err
Expand All @@ -384,7 +385,7 @@ $err
return
}

$::main_status stop
$status_bar_operation stop
_after_readtree $this
}

Expand Down