Re: [PATCH v3 0/7] rebase.autostash completed
Junio C Hamano wrote: Especially I did not check if there are still undesirable data loss behaviour in corner cases that people were worried about in the discussion. Check the tests. What am I missing? In the longer term, it is unmaintainable to make such users (like this new code) of the stash mechanism manually do so, and we may want to add a git stash __store subcommand, not meant for the interactive use of end users. The implementation of the stash can later be changed without affecting such users by doing so. Yes, a store that stores a commit created with create would be nice. Why the horrible double-underscore though? git stash create is not meant for interactive use of end users either. Perhaps rebase can be taught to be more careful when checking if local changes may overlap with the changes being replayed. Frankly, I don't know if it's worth the effort. It might be a nice theoretical exercise, but what tangible benefit do I get as the end user (now that I have rebase.autostash)? In fact, I'll probably be slowing down the interactive rebase noticeably by executing a diff-tree at every step. And for what? Those who still want to use stash would benefit from having an autostash, but at that point, there is no reason to single out rebase for the autostash feature. Those who want to stash immediately before a merge that is known to conflict can use the same autostash and may want to do so for exactly the same reason they may want to use it for rebase. Each command that wants to have autostash will have to implement it independently. You argued that an implicit merge.autostash may be harmful earlier: maybe an explicit --autostash; but at this point, the returns are diminishing: what is the great advantage of --autostash over a quick manual g ss, g sp (git stash save, git stash pop)? I don't know what problem you're trying to solve anymore. -- 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 v3 0/7] rebase.autostash completed
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Especially I did not check if there are still undesirable data loss behaviour in corner cases that people were worried about in the discussion. Check the tests. What am I missing? I didn't do a thorough check, but my earlier comments are taken into account, I didn't see anything wrong and the tests in 7/7 are good. Perhaps rebase can be taught to be more careful when checking if local changes may overlap with the changes being replayed. Frankly, I don't know if it's worth the effort. It might be a nice theoretical exercise, but what tangible benefit do I get as the end user (now that I have rebase.autostash)? In fact, I'll probably be slowing down the interactive rebase noticeably by executing a diff-tree at every step. And for what? One benefit would be to avoid triggering rebuild (and editor reload) by keeping the timestamps intact. But I agree it's probably not worth the effort (and definitely isn't in the scope of this series). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 v3 0/7] rebase.autostash completed
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Especially I did not check if there are still undesirable data loss behaviour in corner cases that people were worried about in the discussion. Check the tests. What am I missing? In the longer term, it is unmaintainable to make such users (like this new code) of the stash mechanism manually do so, and we may want to add a git stash __store subcommand, not meant for the interactive use of end users. The implementation of the stash can later be changed without affecting such users by doing so. Yes, a store that stores a commit created with create would be nice. Why the horrible double-underscore though? git stash create is not meant for interactive use of end users either. create is not advertised very widely, but store is too close to what is already familiar to the people save and we really do not want to confuse them. store -m message commit sounds as if you are creating a stash to apply to the given $commit. -- 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 v3 0/7] rebase.autostash completed
Matthieu Moy matthieu@grenoble-inp.fr writes: One benefit would be to avoid triggering rebuild (and editor reload) by keeping the timestamps intact. But I agree it's probably not worth the effort (and definitely isn't in the scope of this series). It isn't in the scope, of course. If rebase were done right, the series will become much less necessary in the first place ;-). -- 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 v3 0/7] rebase.autostash completed
Junio C Hamano wrote: create is not advertised very widely, but store is too close to what is already familiar to the people save and we really do not want to confuse them. store -m message commit sounds as if you are creating a stash to apply to the given $commit. In the store series I posted a few minutes ago, I use the format store commit message. I've not advertised it in the usage (like create), and documented it just like create. Maybe we should add the line Not for end user interactive use to both descriptions? We can get that double-underscore if you really want, but it'd stick out like a sore thumb since we can't change create. -- 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 v3 0/7] rebase.autostash completed
Ramkumar Ramachandra artag...@gmail.com writes: Maybe we should add the line Not for end user interactive use to both descriptions? We can get that double-underscore if you really want, but it'd stick out like a sore thumb since we can't change create. Yeah, good point about the __name. It was a mistake that the description of 'create' does not have This is probably not what you want to use; see 'save' there. Let's do that, and do the same for 'store' (that is, not advertise in 'git stash -h' output, describe in the manual for scripters, and mark it not for the end user in the description). -- 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 v3 0/7] rebase.autostash completed
Ramkumar Ramachandra artag...@gmail.com writes: Minor changes since v2 in response to reviews by Junio and Eric Sunshine. Should be ready to merge soon. That, and the completed on the Subject line are not for you to decide, I would have to say ;-) Overall the patches look cleanly done, but I did not carefully look into the implementation. Especially I did not check if there are still undesirable data loss behaviour in corner cases that people were worried about in the discussion. It would have been nice to CC people who voiced concerns during the previous reviews. A few comments on possible follow-up work, not meant as a suggestion to include in this series: * The series shows that it is necessary to first prepare a stash entry with stash create, then later decide to queue it to the stash (or decide not to do so) in some applications. In the longer term, it is unmaintainable to make such users (like this new code) of the stash mechanism manually do so, and we may want to add a git stash __store subcommand, not meant for the interactive use of end users. The implementation of the stash can later be changed without affecting such users by doing so. * The primary reason why you wanted this autostash was because rebase is implemented in a lazy and stupid way, compared to merge that detects possible conflicts with local changes and refrains from touching the working tree at all, and any local change that do not interfere with the merge are permitted. Perhaps rebase can be taught to be more careful when checking if local changes may overlap with the changes being replayed. When replaying a range A..B on top of the onto commit O, perhaps rebase can at least do these: - If the index is dirty with respect to HEAD, stop just like merge does. - Take a union of paths different between O and the working tree, and untracked new paths in the working tree. These are the possible local changes. If there is no way the replay of A..B touches any of these paths, we do not even have to worry about stashing. - Take output from diff-tree -m --name-only for each commit in the replayed range (note that I am not adding -M to take maximal coverage). Check if any of the paths overlap with the local changes, and if so, stop. to be par with merge. When the latter is done, there is _no_ justification for autostash to be an option specific fo rebase. At that point, rebase and merge would refuse to start in the same situation, i.e. the local changes you have are known to interfere with the integration with the branch, diminishing the need to autostash greatly. When autostash is still needed to proceed after such change to rebase, I suspect that the user may be better off saving such work-in-progress in a temporary commit (possibly after making it into a better shape before even starting to rebase or merge) than using a transient mechanism like stash to save such a work, because it is likely that such local changes have a real conflict with the work being integrated. Those who still want to use stash would benefit from having an autostash, but at that point, there is no reason to single out rebase for the autostash feature. Those who want to stash immediately before a merge that is known to conflict can use the same autostash and may want to do so for exactly the same reason they may want to use it for rebase. -- 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
