Re: [PATCH 3/3] reset: Print a warning when user uses "git reset" during a merge
Andrew Wong 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 > --- > 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
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
On Fri, Mar 14, 2014 at 4:55 PM, Junio C Hamano 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
Re: [PATCH 3/3] reset: Print a warning when user uses "git reset" during a merge
Andrew Wong writes: > On Fri, Mar 14, 2014 at 10:33 AM, Marc Branchaud 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
On Fri, Mar 14, 2014 at 10:33 AM, Marc Branchaud 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
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
[PATCH 3/3] reset: Print a warning when user uses "git reset" during a merge
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 --- 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