Pratyush Yadav [off-list ref] writes:
This series adds the ability to revert selected lines and hunks in
git-gui. Partially based on the patch by Bert Wesarg [0].
The commits can be found in the topic branch 'py/revert-hunks-lines'
at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines
Once reviewed, pull the commits from
415ce3f8582769d1d454b3796dc6c9c847cefa87 till
0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and
apply them locally, whichever you prefer.
Let's see how we can work together by you playing the role of
git-gui maintainer and the others on the list (including me) playing
the role of reviewer and contributor. So I may keep an eye on the
discussion on this thread, I may even comment on them myself, but
you'll be the one waiting for the discussion to settle, adjusting
the patches in response to reviews, etc. and making the final
decision when/if the topic is done, at which time you'd be telling
me to pull from you.
Pratyush Yadav (4):
git-gui: Move revert confirmation dialog creation to separate function
git-gui: Add option to disable the revert confirmation prompt
git-gui: Add the ability to revert selected lines
git-gui: Add the ability to revert selected hunk
"Move" and "Add" after "git-gui:" would better be downcased to be
more in line with the others in "git shortlog --no-merges"; I also
think "allow doing X" is shorter and better way to say "add the
ability to do X".
If I am reading the first patch correctly, we already ask for
confirmation before reverting local changes, and the steps 3 and 4
are about allowing partial reversion in addition to the wholesale
reversion, right? An earlier objection from j6t sounded like we
require users to respond to an extra dialog after this series, but
that does not look like the case. Instead, step 2 adds a new
feature to allow those to opt-out of the existing dialog (which may
be reused to squelch the dialog to protect features added in steps 3
and 4). Am I reading the series correctly?
Thanks.
git-gui.sh | 40 +++++++++++++++++++++++++++++--
lib/diff.tcl | 65 ++++++++++++++++++++++++++++++++++++++++++--------
lib/index.tcl | 31 ++++++++++++++++--------
lib/option.tcl | 1 +
4 files changed, 115 insertions(+), 22 deletions(-)
--
2.21.0
On 22/08/19 03:34PM, Junio C Hamano wrote:
Pratyush Yadav [off-list ref] writes:
quoted
This series adds the ability to revert selected lines and hunks in
git-gui. Partially based on the patch by Bert Wesarg [0].
The commits can be found in the topic branch 'py/revert-hunks-lines'
at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines
Once reviewed, pull the commits from
415ce3f8582769d1d454b3796dc6c9c847cefa87 till
0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and
apply them locally, whichever you prefer.
Let's see how we can work together by you playing the role of
git-gui maintainer and the others on the list (including me) playing
the role of reviewer and contributor. So I may keep an eye on the
discussion on this thread, I may even comment on them myself, but
you'll be the one waiting for the discussion to settle, adjusting
the patches in response to reviews, etc. and making the final
decision when/if the topic is done, at which time you'd be telling
me to pull from you.
quoted
Pratyush Yadav (4):
git-gui: Move revert confirmation dialog creation to separate function
git-gui: Add option to disable the revert confirmation prompt
git-gui: Add the ability to revert selected lines
git-gui: Add the ability to revert selected hunk
"Move" and "Add" after "git-gui:" would better be downcased to be
more in line with the others in "git shortlog --no-merges"; I also
think "allow doing X" is shorter and better way to say "add the
ability to do X".
Will fix. Thanks.
If I am reading the first patch correctly, we already ask for
confirmation before reverting local changes, and the steps 3 and 4
are about allowing partial reversion in addition to the wholesale
reversion, right?
Yes. The ability to revert whole files already exists. This revert
always asks for confirmation. Steps 3 and 4 allow for partial reverts.
Step 2 allows the user to disable the confirmation dialog for both
whole-file reverts and for partial reverts.
An earlier objection from j6t sounded like we
require users to respond to an extra dialog after this series, but
that does not look like the case. Instead, step 2 adds a new
feature to allow those to opt-out of the existing dialog (which may
be reused to squelch the dialog to protect features added in steps 3
and 4). Am I reading the series correctly?
Yes. The users always responded to an extra dialog for whole file
reverts even before these changes. j6t was running a fork of git-gui
which had the ability for partial reverts, and his fork did not ask for
confirmation for partial reverts. Always asking for confirmation
disrupts his workflow, so I added an option to disable it. It disables
the dialog for partial reverts (which j6t cares about), and also for
whole file reverts, which I added to maintain consistency.
Thanks.
quoted
git-gui.sh | 40 +++++++++++++++++++++++++++++--
lib/diff.tcl | 65 ++++++++++++++++++++++++++++++++++++++++++--------
lib/index.tcl | 31 ++++++++++++++++--------
lib/option.tcl | 1 +
4 files changed, 115 insertions(+), 22 deletions(-)
--
2.21.0
--
Regards,
Pratyush Yadav
On Fri, Aug 23, 2019 at 12:51 AM Pratyush Yadav [off-list ref] wrote:
On 22/08/19 03:34PM, Junio C Hamano wrote:
quoted
Pratyush Yadav [off-list ref] writes:
quoted
This series adds the ability to revert selected lines and hunks in
git-gui. Partially based on the patch by Bert Wesarg [0].
The commits can be found in the topic branch 'py/revert-hunks-lines'
at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines
Once reviewed, pull the commits from
415ce3f8582769d1d454b3796dc6c9c847cefa87 till
0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and
apply them locally, whichever you prefer.
Let's see how we can work together by you playing the role of
git-gui maintainer and the others on the list (including me) playing
the role of reviewer and contributor. So I may keep an eye on the
discussion on this thread, I may even comment on them myself, but
you'll be the one waiting for the discussion to settle, adjusting
the patches in response to reviews, etc. and making the final
decision when/if the topic is done, at which time you'd be telling
me to pull from you.
quoted
Pratyush Yadav (4):
git-gui: Move revert confirmation dialog creation to separate function
git-gui: Add option to disable the revert confirmation prompt
git-gui: Add the ability to revert selected lines
git-gui: Add the ability to revert selected hunk
"Move" and "Add" after "git-gui:" would better be downcased to be
more in line with the others in "git shortlog --no-merges"; I also
think "allow doing X" is shorter and better way to say "add the
ability to do X".
Will fix. Thanks.
quoted
If I am reading the first patch correctly, we already ask for
confirmation before reverting local changes, and the steps 3 and 4
are about allowing partial reversion in addition to the wholesale
reversion, right?
Yes. The ability to revert whole files already exists. This revert
always asks for confirmation. Steps 3 and 4 allow for partial reverts.
Step 2 allows the user to disable the confirmation dialog for both
whole-file reverts and for partial reverts.
quoted
An earlier objection from j6t sounded like we
require users to respond to an extra dialog after this series, but
that does not look like the case. Instead, step 2 adds a new
feature to allow those to opt-out of the existing dialog (which may
be reused to squelch the dialog to protect features added in steps 3
and 4). Am I reading the series correctly?
Yes. The users always responded to an extra dialog for whole file
reverts even before these changes. j6t was running a fork of git-gui
which had the ability for partial reverts, and his fork did not ask for
confirmation for partial reverts. Always asking for confirmation
disrupts his workflow, so I added an option to disable it. It disables
the dialog for partial reverts (which j6t cares about), and also for
whole file reverts, which I added to maintain consistency.
as I'm the one who use this feature for more than 7 years, I can only
object to this. I'm happy to have the confirmation dialog for the
whole-file revert (the current state) but having the dialog also for
partial revert would be too disruptive. And disabling both at the same
time would not be an option for me.
The thing is, that the partial revert "just don't happen by accident".
Here are the minimum user actions needed to get to this dialog:
1. whole-file revert
- do a Ctrl+J, more or less anywhere in the GUI
2. hunk revert/revert one unselected line
- right click anywhere in the diff pane (thats around 60% of the window area)
- move the mouse pointer down 3/4 menu items
- click this menu item
3. partially revert selected lines
- select some content in the diff pane by starting by pressing and
holding a left click
- end the selection by releasing the left click
- move the mouse pointer down 3/4 menu items
- click this menu item
Thats always at least 2 user actions more than the whole-file revert.
Thus this cannot happen by accident quite easily in comparison to the
whole-file revert. And thats the reason why this dialog exists, from
my point of view.
I can see the need to disable the dialog for the whole-file revert,
and IIRC that was also requested a long time ago on this list. But I
don't see a reason to have this dialog also for the partial reverts as
a safety measure.
Best,
Bert
--
Regards,
Pratyush Yadav
On 23/08/19 08:04AM, Bert Wesarg wrote:
On Fri, Aug 23, 2019 at 12:51 AM Pratyush Yadav [off-list ref] wrote:
quoted
On 22/08/19 03:34PM, Junio C Hamano wrote:
[...]
as I'm the one who use this feature for more than 7 years, I can only
object to this. I'm happy to have the confirmation dialog for the
whole-file revert (the current state) but having the dialog also for
partial revert would be too disruptive. And disabling both at the same
time would not be an option for me.
The thing is, that the partial revert "just don't happen by accident".
Here are the minimum user actions needed to get to this dialog:
1. whole-file revert
- do a Ctrl+J, more or less anywhere in the GUI
Hmm, have to agree with you on this. It is kind of easy to trigger. But
read below why I think partial reverts are just as easy to trigger.
2. hunk revert/revert one unselected line
- right click anywhere in the diff pane (thats around 60% of the window area)
- move the mouse pointer down 3/4 menu items
- click this menu item
But what if you wanted to click "Stage hunk", and instead click "Revert
hunk" instead. This is what I mean by "accidentally".
This is even more a risk in the current layout of the buttons, which are
in the order:
Stage Hunk
Revert Hunk
Stage Lines
Revert Lines
In this layout, if you wanted to click Stage, you might end up clicking
Revert instead, losing part of your work. Even if we switch up the
layout a bit, I feel like the potential of "mis-aiming" your mouse is
there, but it can certainly be improved.
3. partially revert selected lines
- select some content in the diff pane by starting by pressing and
holding a left click
- end the selection by releasing the left click
- move the mouse pointer down 3/4 menu items
- click this menu item
Thats always at least 2 user actions more than the whole-file revert.
Thus this cannot happen by accident quite easily in comparison to the
whole-file revert. And thats the reason why this dialog exists, from
my point of view.
I can see the need to disable the dialog for the whole-file revert,
and IIRC that was also requested a long time ago on this list. But I
don't see a reason to have this dialog also for the partial reverts as
a safety measure.
I do (not too strongly, but I do), as I explained why above.
It shouldn't be too difficult to have separate knobs for whole-file and
partial reverts, but they will add two config options. Not necessarily a
bad thing, I just thought the people who wanted to disable partial
reverts would also want to disable whole-file reverts.
But I have another suggestion in mind. I'll reply to one of the other
emails in this thread about it. Please read it there, I'd rather not
type it twice.
--
Regards,
Pratyush Yadav