Re: [PATCH 3/3] reset: Print a warning when user uses git reset during a merge

[Junio C Hamano]

Andrew Wong andrew.k...@gmail.com writes:

 During a merge, --mixed is most likely not what the user wants. Using
 --mixed during a merge would leave the merged changes and new files
 mixed in with the local changes. The user would have to manually clean
 up the work tree, which is non-trivial. In future releases, we want to
 make git reset error out when used in the middle of a merge. For now,
 we simply print out a warning to the user.

 Signed-off-by: Andrew Wong andrew.k...@gmail.com
 ---
  builtin/reset.c | 21 +
  1 file changed, 21 insertions(+)

 diff --git a/builtin/reset.c b/builtin/reset.c
 index 4fd1c6c..04e8103 100644
 --- a/builtin/reset.c
 +++ b/builtin/reset.c
 @@ -331,8 +331,29 @@ int cmd_reset(int argc, const char **argv, const char 
 *prefix)
   _(reset_type_names[reset_type]));
   }
   if (reset_type == NONE)
 + {
   reset_type = MIXED; /* by default */
  
 + /* During a merge, --mixed is most likely not what the user

Two style niggles here.

 +  * wants. Using --mixed during a merge would leave the merged
 +  * changes and new files mixed in with the local changes. The
 +  * user would have to manually clean up the work tree, which is
 +  * non-trivial. In future releases, we want to make git reset

we want?  Has any of us decided on that?

 +  * error out when used in the middle of a merge. For now, we
 +  * simply print out a warning to the user. */
 + if (is_merge())
 + warning(_(You have used 'git reset' in the middle of a 
 merge. 'git reset' defaults to\n
 +   'git reset --mixed', which means git will 
 not clean up any merged changes and\n
 +   new files that were created in the work 
 tree. It also becomes impossible for\n
 +   git to automatically clean up the work tree 
 later, so you would have to clean\n
 +   up the work tree manually. To avoid this 
 next time, you may want to use 'git\n
 +   reset --merge', or equivalently 'git merge 
 --abort'.\n
 +   \n
 +   In future releases, using 'git reset' in the 
 middle of a merge will result in\n
 +   an error.
 +  ));
 + }
 +
   if (reset_type != SOFT  reset_type != MIXED)
   setup_work_tree();
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] reset: Print a warning when user uses git reset during a merge

[Marc Branchaud]

On 14-03-14 04:55 PM, Junio C Hamano wrote:


So I am OK with eventually error out by default, but not OK with
we know better than the user and will not allow it at all.


Can I interpret that as you being OK with my proposed Cowardly 
refusing approach?


M.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] reset: Print a warning when user uses git reset during a merge

[Marc Branchaud]

On 14-03-14 12:37 AM, Andrew Wong wrote:
 During a merge, --mixed is most likely not what the user wants. Using
 --mixed during a merge would leave the merged changes and new files
 mixed in with the local changes. The user would have to manually clean
 up the work tree, which is non-trivial. In future releases, we want to
 make git reset error out when used in the middle of a merge. For now,
 we simply print out a warning to the user.

I know this approach was suggested earlier, but given these dangers it seems
silly to give this big warning on a plain git reset but still go ahead and
do the things the warning talks about.

Is there any issue with changing git reset to error-out now but letting
git reset --mixed proceed?  Something like (note the reworded warning 
message):

$ git reset
Cowardly refusing to implicitly run 'git reset --mixed' during a merge.
This would not clean up any merged changes and would not remove any new
files that were created in the work tree.  It would also make it impossible
for git to automatically clean up the work tree later, so you would have to
clean up the work tree manually.
You probably meant to run 'git merge --abort' instead.
$ git reset --mixed   # Stoopid git!  I know what I'm doing!
$

This would mean that the 10% of git users who like to do git reset in the
middle of a conflicted merge will have to teach their fingers some extra
motions.  But these users are all veterans, and they can more easily type in
8 extra characters (6 with completion) than new users can recover from
accidentally misusing git-reset's power.

M.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] reset: Print a warning when user uses git reset during a merge

[Andrew Wong]

On Fri, Mar 14, 2014 at 10:33 AM, Marc Branchaud marcn...@xiplink.com wrote:
 I know this approach was suggested earlier, but given these dangers it seems
 silly to give this big warning on a plain git reset but still go ahead and
 do the things the warning talks about.

 Is there any issue with changing git reset to error-out now but letting
 git reset --mixed proceed?  Something like (note the reworded warning 
 message):

Yeah, I would have preferred to have git reset error out right now,
because the messed up work tree can be quite a pain to clean up. The
main argument for issuing the warning is about maintaining
compatibility.

For the users that really did mean --merge, the warning is silly.
It's basically saying We know that you're about to mess up your work
tree, but we let you mess up anyway. Learn the correct way so that you
don't mess up next time.

It actually doesn't seem too bad if we did make git reset to error
out (during a merge) right away. By erroring out, the command won't
cause some irreversible damage, and users don't lose data. Yes, it
breaks compatibility, but perhaps not in a bad way?

I'm really fine with either. Junio?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] reset: Print a warning when user uses git reset during a merge

[Junio C Hamano]

Andrew Wong andrew.k...@gmail.com writes:

 On Fri, Mar 14, 2014 at 10:33 AM, Marc Branchaud marcn...@xiplink.com wrote:
 I know this approach was suggested earlier, but given these dangers it seems
 silly to give this big warning on a plain git reset but still go ahead and
 do the things the warning talks about.

 Is there any issue with changing git reset to error-out now but letting
 git reset --mixed proceed?  Something like (note the reworded warning 
 message):

 Yeah, I would have preferred to have git reset error out right now,
 because the messed up work tree can be quite a pain to clean up. The
 main argument for issuing the warning is about maintaining
 compatibility.

 For the users that really did mean --merge, the warning is silly.
 It's basically saying We know that you're about to mess up your work
 tree, but we let you mess up anyway. Learn the correct way so that you
 don't mess up next time.

I suspect that you meant --mixed instead of --merge here.

I do not agree with the We know that you are about to mess up
above.  Whoever is issuing that warning message may not know better
than the user who is running reset.  As you wrote most likely not
what the user wants in your proposed log message, the only thing we
know is that it _often_ is a newbie mistake.

I recently needed to manually cherry-pick only one half of a patch,
and the way I did so was

git show $that_commit P.diff
git apply -3 P.diff
... conflicts are expected; that is why I used -3 in the
... first place
git reset
git diff HEAD
edit
... edit away the other half I did not want to cherry-pick
... while fixing the conflicted parts that happened to be
... in the part I did want to cherry-pick

git cherry-pick --no-commit $that_commit could have been used as
a replacement for the first two steps but being able to run the
reset without erroring out was an essential part to make this
workflow usable.

So I am OK with eventually error out by default, but not OK with
we know better than the user and will not allow it at all.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] reset: Print a warning when user uses git reset during a merge

[Andrew Wong]

On Fri, Mar 14, 2014 at 4:55 PM, Junio C Hamano gits...@pobox.com wrote:
 For the users that really did mean --merge, the warning is silly.
 It's basically saying We know that you're about to mess up your work
 tree, but we let you mess up anyway. Learn the correct way so that you
 don't mess up next time.

 I suspect that you meant --mixed instead of --merge here.

No, I did mean --merge. It's silly for inexperienced users because
it's too late to use --merge by the time they realized they should
not have used the default. The work tree has already become a mess. So
they'd immediately think if git was smart enough to warn me about the
mess, why not prevent me from getting into the mess in the first
place?

For the experienced users, they would understand the warning, because
they would be aware of the index, and the effect that --mixed and
--merge have on it.

 So I am OK with eventually error out by default, but not OK with
 we know better than the user and will not allow it at all.

Again, I didn't mean we know better than the user. However, from a
new user's perspective, they won't understand why git reset gives
the warning, but still knowingly messes up their work tree.

And we don't know better than the user is exactly why I think we
should eventually error out rather than automatically switching to
--merge. As Matthieu was saying, automatically switching to
--merge could discard conflict resolutions, which would be
undesirable. So it's better for git to error out then having git
decides what the user (probably) wants.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] reset: Print a warning when user uses git reset during a merge

[Andrew Wong]

During a merge, --mixed is most likely not what the user wants. Using
--mixed during a merge would leave the merged changes and new files
mixed in with the local changes. The user would have to manually clean
up the work tree, which is non-trivial. In future releases, we want to
make git reset error out when used in the middle of a merge. For now,
we simply print out a warning to the user.

Signed-off-by: Andrew Wong andrew.k...@gmail.com
---
 builtin/reset.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/builtin/reset.c b/builtin/reset.c
index 4fd1c6c..04e8103 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -331,8 +331,29 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
_(reset_type_names[reset_type]));
}
if (reset_type == NONE)
+   {
reset_type = MIXED; /* by default */
 
+   /* During a merge, --mixed is most likely not what the user
+* wants. Using --mixed during a merge would leave the merged
+* changes and new files mixed in with the local changes. The
+* user would have to manually clean up the work tree, which is
+* non-trivial. In future releases, we want to make git reset
+* error out when used in the middle of a merge. For now, we
+* simply print out a warning to the user. */
+   if (is_merge())
+   warning(_(You have used 'git reset' in the middle of a 
merge. 'git reset' defaults to\n
+ 'git reset --mixed', which means git will 
not clean up any merged changes and\n
+ new files that were created in the work 
tree. It also becomes impossible for\n
+ git to automatically clean up the work tree 
later, so you would have to clean\n
+ up the work tree manually. To avoid this 
next time, you may want to use 'git\n
+ reset --merge', or equivalently 'git merge 
--abort'.\n
+ \n
+ In future releases, using 'git reset' in the 
middle of a merge will result in\n
+ an error.
+));
+   }
+
if (reset_type != SOFT  reset_type != MIXED)
setup_work_tree();
 
-- 
1.9.0.174.g6f75b8f

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html