Re: [PATCH 3/3] pull: introduce --[no-]autostash and pull.autostash
Phil Hord wrote: > If I follow the latter advice about 'rm -rf', who will remember that > 'rebase' had something stashed, and what will they do with it when > they do? > > What if it is weeks or months later? I would be surprised to see > long-forgotten wip show up in my workspace all of a sudden. Ultimately, I think an ideal implementation requires this entire autostash implementation to reside completely within the $state_dir. The issue of a long-forgotten WIP is then the same issue as a long-forgotten rebase, and a rm -rf $state_dir will get rid of the WIP as well. The other reason is that it shouldn't interact with the rest of git. This means no touching any refs or reflogs, and this can be problematic if we decide to use the standard git stash. I'm currently working towards seeing if it's possible to get stash to create "named stashes" that we can predictably retrieve later, to avoid rolling our own homegrown stash. Yes, the problem is much more complex than I initially thought. It was much simpler to implement it for git-pull.sh. -- 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] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy writes: > Ramkumar Ramachandra writes: > >> If it is 0 (which means that the rebase completely successfully), pop >> the stash before exiting as usual. > > And you're going to pop the stash even if no stash were triggered when > starting the rebase. > > Really, think about it again: you're not going to guess whether you have > to unstash without storing this information somewhere. You may argue > about storing it outside the todolist, you can't unstash > unconditionally. Yes, it can be part of todo, but it does not have to be. Regardless of where the information is stored, implementing the last step as "stash pop" will add a small inconsistency the end user needs to be aware of. Imagine "git rebase" stops, asks you to help with conflicts, and you look at it, play with the working tree, and made a mess. If this was the last "stash pop" invocation, the way to go back and start again is "reset --hard && stash pop". For all the other steps, that is not how the user goes back to the originally conflicted state in order to retry from scratch. Makes me wonder if the world were a better place if the rebase-todo list were implemented as a dedicated stash and "rebase --continue" were a mere "stash pop" followed by a commit if pop goes smoothly. I am not suggesting to actually do so, but it is an intriguing thought from the perspective of end user experience, isn't it? In any case, I am not saying that it is a _bad_ thing to implement the last "restore the WIP" stage as "stash pop". I am just pointing out that the messaging needs to be done carefully to make sure the user understands which one is which, as the way to deal with the situation where it stops and asks the user for help would be different from normal step in the rebase sequence that replays a 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 3/3] pull: introduce --[no-]autostash and pull.autostash
On Tue, Apr 16, 2013 at 5:20 AM, Ramkumar Ramachandra wrote: > Matthieu Moy wrote: > No. Ultimately, the entry point of all these invocations is > git-rebase.sh. The plan is to refactor calls from git-rebase.sh to > git-rebase--*.sh scripts so that those scripts return control to > git-rebase.sh, which will be the final exit point. The logic is very > simple: On the very first invocation of rebase (ie. no existing rebase > in progress), stash. If the return statement from the specific rebase > script is 1 (which means that there are conflicts to be resolved), > exit as usual. If it is 0 (which means that the rebase completely > successfully), pop the stash before exiting as usual. > > What's so complicated about that? I'm against leaking the autostash > implementation detail into specific rebases, because I value a clean > and pleasant implementation over everything else. It can be more complex than you realize. $ git pull --rebase --stash It seems that there is already a .git/rebase-apply directory, and I wonder if you are in the middle of another rebase. If that is the case, please try git rebase (--continue | --abort | --skip) If that is not the case, please rm -fr .git/rebase-apply and run me again. I am stopping in case you still have something valuable there. If I follow the latter advice about 'rm -rf', who will remember that 'rebase' had something stashed, and what will they do with it when they do? What if it is weeks or months later? I would be surprised to see long-forgotten wip show up in my workspace all of a sudden. Phil -- 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] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy writes: > Ramkumar Ramachandra writes: > >> Yes, touching a simple "autostash" file at stash-time, and removing it >> at pop-time will do. I don't see why it should be part of a >> (potentially user-editable) todo-list, which serves an entirely >> different purpose. > > You'll have to apply the index state and then the tree state. How do you > know whether the next call to "git rebase" should apply one or the other? Plus: what happens if the user ran "git stash" during rebase? In Junio's version, the right commits are applied. Running blindly "stash pop" may pop the wrong stash. -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > Yes, touching a simple "autostash" file at stash-time, and removing it > at pop-time will do. I don't see why it should be part of a > (potentially user-editable) todo-list, which serves an entirely > different purpose. You'll have to apply the index state and then the tree state. How do you know whether the next call to "git rebase" should apply one or the other? -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: > And you're going to pop the stash even if no stash were triggered when > starting the rebase. > > Really, think about it again: you're not going to guess whether you have > to unstash without storing this information somewhere. You may argue > about storing it outside the todolist, you can't unstash > unconditionally. Yes, touching a simple "autostash" file at stash-time, and removing it at pop-time will do. I don't see why it should be part of a (potentially user-editable) todo-list, which serves an entirely different purpose. -- 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] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > If it is 0 (which means that the rebase completely successfully), pop > the stash before exiting as usual. And you're going to pop the stash even if no stash were triggered when starting the rebase. Really, think about it again: you're not going to guess whether you have to unstash without storing this information somewhere. You may argue about storing it outside the todolist, you can't unstash unconditionally. -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: > Because "git rebase" needs multiple runs in case of conflicts. You have > to store the information somewhere in the filesystem, not in a variable. > What you need to store is whether you need to unstash, and where you are > in the process (in Junio's version, you may be doing the actual rebase, > or fixing conflicts in index state application, or fixing conflicts in > tree state application, or done). Storing what you have to do and where > you are in the process really sounds like a job for the instruction > sheet, no? No. Ultimately, the entry point of all these invocations is git-rebase.sh. The plan is to refactor calls from git-rebase.sh to git-rebase--*.sh scripts so that those scripts return control to git-rebase.sh, which will be the final exit point. The logic is very simple: On the very first invocation of rebase (ie. no existing rebase in progress), stash. If the return statement from the specific rebase script is 1 (which means that there are conflicts to be resolved), exit as usual. If it is 0 (which means that the rebase completely successfully), pop the stash before exiting as usual. What's so complicated about that? I'm against leaking the autostash implementation detail into specific rebases, because I value a clean and pleasant implementation over everything else. -- 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] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra wrote: > "If there were uncommitted worktree changes present when the merge > started, git merge --abort will in some cases be unable to reconstruct > these changes. It is therefore recommended to always commit or stash > your changes before running git merge." > > Matthieu (and probably others) run git merge with a dirty worktree > most of the time, while I never do (because I read this section). The above says "in some cases". It's presumably referring to the case "One exception is when the changed index entries are in the state that would result from the merge already" described in the "PRE-MERGE CHECKS" section. Improved wording welcome. Ciao, Jonathan -- 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] pull: introduce --[no-]autostash and pull.autostash
Jonathan Nieder wrote: > I wouldn't be surprised if there's still a documentation bug, though: > a lack of clarity, a missing hint somewhere else, something else > misleading. That seems especially likely when you say "I trusted the > documentation on this one". Care to point to the appropriate section, > so it can be fixed? As I pointed out in a previous email, I'm referring to the --abort section of git-merge(1): "If there were uncommitted worktree changes present when the merge started, git merge --abort will in some cases be unable to reconstruct these changes. It is therefore recommended to always commit or stash your changes before running git merge." Matthieu (and probably others) run git merge with a dirty worktree most of the time, while I never do (because I read this section). -- 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] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra wrote: > Matthieu Moy wrote: >> No, you don't. Just try if you're not convinced: > > Oh, I trusted the documentation on this one and never tried with a > dirty worktree myself. Please fix the documentation, if you know how > exactly to correct it. The manual says: "git pull" and "git merge" will stop without doing anything when local uncommitted changes overlap with files that git pull/git merge may need to update. That accurately describes the behavior. I wouldn't be surprised if there's still a documentation bug, though: a lack of clarity, a missing hint somewhere else, something else misleading. That seems especially likely when you say "I trusted the documentation on this one". Care to point to the appropriate section, so it can be fixed? Thanks, Jonathan -- 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] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: > Junio C Hamano writes: >> Yes, that is why I said for pull-merge, --authsquash is neutral-to-better >> and pull.autosquash is harmful. > > To speak from my experience: I find myself typing "git stash && git pull > && git stash pop" relatively often. To that end, maybe "git pull --stash" would be an easier to type option name than "git pull --autostash". That would also avoid the question of "What is automatically going to happen here? Is it going to stash only sometimes? Is it going to automatically stash what I pull?". I'm not a big stash user (except "git stash --keep-index" to prepare to test changes in the index), so I can't say much more about the details. Thanks, Jonathan -- 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] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > Junio C Hamano wrote: >> If "rebase -m" were to be taught to do this, the natural way to do >> so is to >> >> (1) Prepare the todo the usual way >> (2) Do those two commits for index and working tree >> (3) Append two insns (exec reset HEAD^ and exec reset --soft >> HEAD^) at the end of the rebase todo file. > > Er, no. I don't want to touch the instruction sheet. It becomes > especially problematic in -i, when the instruction sheet is > user-editable. I do not find this problematic. It shows the user what's going on. It may be a good idea to append the last instructions after launching the editor if we want to partially hide it (but it's still going to be visible with rebase --edit-todo) >> "rebase--am" could also be told to generate (on the preparation >> side) and notice (on the application side) a pair of patch files at >> the end that represent the index state and the working tree state >> and apply them without making the WIP part into a commit. > > Ugh, no. I don't want to leak the implementation detail of autostash > into specific rebases. Why can't I wrap the last statment in > git-rebase.sh in git stash/ git stash pop like I did with git-pull.sh? Because "git rebase" needs multiple runs in case of conflicts. You have to store the information somewhere in the filesystem, not in a variable. What you need to store is whether you need to unstash, and where you are in the process (in Junio's version, you may be doing the actual rebase, or fixing conflicts in index state application, or fixing conflicts in tree state application, or done). Storing what you have to do and where you are in the process really sounds like a job for the instruction sheet, no? -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
On Mon, Apr 15, 2013 at 11:38:20PM +0530, Ramkumar Ramachandra wrote: > Junio C Hamano wrote: > > If "rebase -m" were to be taught to do this, the natural way to do > > so is to > > > > (1) Prepare the todo the usual way > > (2) Do those two commits for index and working tree > > (3) Append two insns (exec reset HEAD^ and exec reset --soft > > HEAD^) at the end of the rebase todo file. > > Er, no. I don't want to touch the instruction sheet. It becomes > especially problematic in -i, when the instruction sheet is > user-editable. > > > "rebase--am" could also be told to generate (on the preparation > > side) and notice (on the application side) a pair of patch files at > > the end that represent the index state and the working tree state > > and apply them without making the WIP part into a commit. > > Ugh, no. I don't want to leak the implementation detail of autostash > into specific rebases. Why can't I wrap the last statment in > git-rebase.sh in git stash/ git stash pop like I did with git-pull.sh? How does that work with the following: - run_specific_rebase fails, so the user needs to fix it up and then run "git rebase --continue". We don't want to pop the stash in this case. - the user runs "git rebase --continue" with staged changes, knowing the git-rebase will commit those. We don't want to create a stash in this case since it will remove the changes the user wants to 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Junio C Hamano wrote: > If "rebase -m" were to be taught to do this, the natural way to do > so is to > > (1) Prepare the todo the usual way > (2) Do those two commits for index and working tree > (3) Append two insns (exec reset HEAD^ and exec reset --soft > HEAD^) at the end of the rebase todo file. Er, no. I don't want to touch the instruction sheet. It becomes especially problematic in -i, when the instruction sheet is user-editable. > "rebase--am" could also be told to generate (on the preparation > side) and notice (on the application side) a pair of patch files at > the end that represent the index state and the working tree state > and apply them without making the WIP part into a commit. Ugh, no. I don't want to leak the implementation detail of autostash into specific rebases. Why can't I wrap the last statment in git-rebase.sh in git stash/ git stash pop like I did with git-pull.sh? -- 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] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > Junio C Hamano wrote: >> git-rebase --autostash >> >> git commit --allow-empty -m 'index part' >> git commit --allow-empty -a -m 'working tree part' >> git rebase --onto $UPSTREAM >> git reset HEAD^ >> git reset --soft HEAD^ > > Why are you rolling your own stash? What do you have against git stash? Nothing. It just felt that it does not _have to be_ implemented with stash, and explaining things in terms of commit and reset would be simpler. If "rebase -m" were to be taught to do this, the natural way to do so is to (1) Prepare the todo the usual way (2) Do those two commits for index and working tree (3) Append two insns (exec reset HEAD^ and exec reset --soft HEAD^) at the end of the rebase todo file. Then the usual "rebase --continue" would just work. Instead you could use "git stash" at step #2 and add "exec git stash pop" at step #3. "rebase--am" could also be told to generate (on the preparation side) and notice (on the application side) a pair of patch files at the end that represent the index state and the working tree state and apply them without making the WIP part into a 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 3/3] pull: introduce --[no-]autostash and pull.autostash
John Keeping wrote: > Why restrict it to non-interactive? I'd find it useful when doing > interactive rebases as well - consider the case when you simply want to > re-order some commits. Actually, I made a mistake: it should be doable for any specific rebase (includes rebase--interactive, rebase--am, and rebase--merge) just as easily, without leaking the autostash detail into them. The last statement in git-rebase.sh is run_specific_rebase, which just needs to be wrapped in a git stash/ git stash pop. -- 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] pull: introduce --[no-]autostash and pull.autostash
On Mon, Apr 15, 2013 at 10:15:54PM +0530, Ramkumar Ramachandra wrote: > Junio's criticism of pull.autostash hurting pull-merge people is > cogent; my current plan is to ditch pull.autostash altogether, and > implement rebase.autostash for the reduced case of a non-interactive > rebase. Why restrict it to non-interactive? I'd find it useful when doing interactive rebases as well - consider the case when you simply want to re-order some commits. -- 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] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: > No, they don't. Git forbids redefining commands with aliases. They may > have an alias like "git pullauto" or so, but not "git pull". Ofcourse, but you get the point. I use p for push, and pu for pull myself. > There's not much we can do about it now, as Git cannot guess whether a > stash is to be re-applied later or just kept "in case". My main use of > "git stash" is "I want a reset --hard, but stash is safer", I wouldn't > want "status" to remind me when I have a stash because it is almost > always the case. > > Showing the "autostash" status in "git status" would make sense OTOH, > but I agree that it's another topic. If the HEAD of the stash contains a stash beginning with the message "pull.autostash: ", show it in the status. End of story. Anyway, no point arguing about this since we've decided not to pursue pull.autostash anyway. -- 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] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: > No, you don't. Just try if you're not convinced: Oh, I trusted the documentation on this one and never tried with a dirty worktree myself. Please fix the documentation, if you know how exactly to correct it. > No, I'm not proposing to do anything for merge. There's no reason to try > being uniform in conflict resolution for pull-merge and pull-rebase as > it is already different now. We already have "git rebase --continue", we > don't have "git merge --continue". So what? The fact that merge doesn't > have the equivalent doesn't mean we should not do something for "rebase > --continue". Well, you can't blame me for the misunderstanding then. In a previous email, you wrote: > Shouldn't this belong to "git merge" instead (i.e. implement "git merge > --autosquash" and call it from "git pull")? Users doing "git fetch && > git merge" by hand should be able to use --autosquash, I think. Junio's criticism of pull.autostash hurting pull-merge people is cogent; my current plan is to ditch pull.autostash altogether, and implement rebase.autostash for the reduced case of a non-interactive 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
Re: [PATCH 3/3] pull: introduce --[no-]autostash and pull.autostash
Junio C Hamano wrote: > git-rebase --autostash > > git commit --allow-empty -m 'index part' > git commit --allow-empty -a -m 'working tree part' > git rebase --onto $UPSTREAM > git reset HEAD^ > git reset --soft HEAD^ Why are you rolling your own stash? What do you have against git stash? -- 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] pull: introduce --[no-]autostash and pull.autostash
Junio C Hamano wrote: > By tempting the user to use autostash first, you are forcing the > user to say "reset --hard" (the first one), "ORIG_HEAD" (to start > from the pre-pull state), "stash pop" (to recover the autostashed > WIP), to a user who got conflict during "stash pop" after the pull > integrated the committed work with the remote side. > > If the user did this instead: > > ... Let's save my large WIP away in a more permanent place first > git checkout -b wip > ... perhaps work a bit more ... > git commit -a -m wip > git checkout - > ... and then ... > git pull > > the user wouldn't have had to do those extra steps. Hm, yes. For a pull-merge guy who opts for pull.autostash, the penalty for forgetting to commit a big WIP in advance is too high. It probably makes sense to restrict the autostash feature to kick in only in the rebase codepath: it might be a good idea to implement rebase.autostash for reduced case of non-interactive rebases instead. I'm only slightly iffy about --onto, but it's not a big concern. -- 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] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy writes: > Ramkumar Ramachandra writes: > >> Matthieu Moy wrote: >>> AFAICT, "git merge --abort" is an alias for "git reset --merge" >> >> Yes, that is correct. >> >>> which >>> was precisely designed to reset only modifications comming from a merge, >>> and not the local changes that were present before the merge was >>> started. The man pages are relatively obscure on the subject, but I'd >>> call that a documentation bug. >> >> I see. Either way, we need a clean worktree for it to work, no? > > No, you don't. Just try if you're not convinced: Heh, I still remember breaking "git merge" and got yelled at loudly. cf. http://thread.gmane.org/gmane.comp.version-control.git/9073 >>> It does. stashing means the user will have to "stash pop" later. One >>> extra step, one extra opportunity to forget something important. >> >> That's only if there are conflicts. If there are conflicts, you'll >> have to stash anyway if: >> - You're doing a pull-merge and want merge --abort to work. > > Again, no. > >>> A minor annoyance is that it will touch files that have no reason to be >>> touched, hence may trigger extra rebuilds with "make", disturbing text >>> editors that have the file open, etc. >> >> Okay, I need to ask you something at this point: do you ever run merge >> on a dirty worktree unless you're absolutely sure that your local >> changes won't conflict with the changes introduced by the merge? > > Most of the time, I just run "git pull" or "git merge". I know it's > conservative enough, to it will stop if there's anything dangerous. Exactly. > No, I'm not proposing to do anything for merge. There's no reason to try > being uniform in conflict resolution for pull-merge and pull-rebase as > it is already different now. We already have "git rebase --continue", we > don't have "git merge --continue". So what? The fact that merge doesn't > have the equivalent doesn't mean we should not do something for "rebase > --continue". Well said. -- 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] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > Matthieu Moy wrote: >> I disagree. A configuration option is something you set once, and then >> forget about. A command, or a command-line option, is something you >> explicitely add when you need it. > > You're making it out to be a much bigger difference than it actually > is. Users will simply alias pull to 'pull --autostash' (a lot of them > already alias it to pull --ff-only, and I'm going to fix this soon). No, they don't. Git forbids redefining commands with aliases. They may have an alias like "git pullauto" or so, but not "git pull". > If your criticism were that git status doesn't show stash state, I > agree with you. There's not much we can do about it now, as Git cannot guess whether a stash is to be re-applied later or just kept "in case". My main use of "git stash" is "I want a reset --hard, but stash is safer", I wouldn't want "status" to remind me when I have a stash because it is almost always the case. Showing the "autostash" status in "git status" would make sense OTOH, but I agree that it's another topic. -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > Matthieu Moy wrote: >> AFAICT, "git merge --abort" is an alias for "git reset --merge" > > Yes, that is correct. > >> which >> was precisely designed to reset only modifications comming from a merge, >> and not the local changes that were present before the merge was >> started. The man pages are relatively obscure on the subject, but I'd >> call that a documentation bug. > > I see. Either way, we need a clean worktree for it to work, no? No, you don't. Just try if you're not convinced: $ git checkout -b branch Switched to a new branch 'branch' $ date > test.txt && git commit -m 'on branch' test.txt [branch 2482623] on branch 1 file changed, 1 insertion(+), 1 deletion(-) $ git checkout - Switched to branch 'master' $ date > test.txt && git commit -m 'on master' test.txt [master c322d35] on master 1 file changed, 1 insertion(+), 1 deletion(-) $ date > other.txt $ git status # On branch master # Changes not staged for commit: # # modified: other.txt # no changes added to commit (use "git add" and/or "git commit -a") $ git merge branch Auto-merging test.txt CONFLICT (content): Merge conflict in test.txt Automatic merge failed; fix conflicts and then commit the result. $ git status # On branch master # You have unmerged paths. # # Unmerged paths: # # both modified: test.txt # # Changes not staged for commit: # # modified: other.txt # no changes added to commit (use "git add" and/or "git commit -a") $ git merge --abort $ git status # On branch master # Changes not staged for commit: # # modified: other.txt # no changes added to commit (use "git add" and/or "git commit -a") $ There may be corner-cases where it doesn't work, but I never encountered such case. >> It does. stashing means the user will have to "stash pop" later. One >> extra step, one extra opportunity to forget something important. > > That's only if there are conflicts. If there are conflicts, you'll > have to stash anyway if: > - You're doing a pull-merge and want merge --abort to work. Again, no. >> A minor annoyance is that it will touch files that have no reason to be >> touched, hence may trigger extra rebuilds with "make", disturbing text >> editors that have the file open, etc. > > Okay, I need to ask you something at this point: do you ever run merge > on a dirty worktree unless you're absolutely sure that your local > changes won't conflict with the changes introduced by the merge? Most of the time, I just run "git pull" or "git merge". I know it's conservative enough, to it will stop if there's anything dangerous. > That's only a pull-merge. Unfortunately, making git-pull.sh uniform > means that we have to fall back to the least-common-denominator of > functionality (which is currently pull-rebase). You may want to, but you don't have to. pull-merge and pull-rebase already have different behavior in case of non-overlapping changes: $ git pull --rebase . branch Cannot pull with rebase: You have unstaged changes. Please commit or stash them. $ git pull --no-rebase . branch >From . * branchbranch -> FETCH_HEAD [...] I don't see any reason to restrict to the common denominator in the same situation for another feature. I can accept the "it's too hard to implement" argument, but not "it doesn't bring anything". >> As a user, when I run "git rebase --continue" and it tells me it's done, >> I expect the work to actually be done. This is the case today. This >> won't be the case after autostash is introduced if the user has to >> remember to run "stash pop" afterwards. > > And how will you implement that for merge, since there is no merge > --continue to execute stash pop from? Do you propose to make commit > do the stash pop'ing? No, I'm not proposing to do anything for merge. There's no reason to try being uniform in conflict resolution for pull-merge and pull-rebase as it is already different now. We already have "git rebase --continue", we don't have "git merge --continue". So what? The fact that merge doesn't have the equivalent doesn't mean we should not do something for "rebase --continue". -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy writes: > I think you're taking it the wrong way. You seem to insist that > autostash should be a pull feature. I think it should be a feature of > merge and rebase, and the convenience script "git pull" should expose > them to the user. > > I see no reason to prevent users running fetch and then merge or rebase > to use the autostash feature. I agree that is a good way to look at the problem and would lead to a much better design of the division of labor among these three moving parts. > It's not about fixing the existing rebase. It's about implementing > autostash the right way. > > As a user, when I run "git rebase --continue" and it tells me it's done, > I expect the work to actually be done. This is the case today. This > won't be the case after autostash is introduced if the user has to > remember to run "stash pop" afterwards. Your "rebase can do the autostash right way" idea in the other message is a good one, I think. The rebase proper will sequencially either apply patches (if the user is using the "format-patch | am" variant) or cherry-pick commits ("rebase -m"). Conceptually we can view the WIP in the working tree as just another commit at the tip of the original history to be rebased (modulo that it should not be left as a commit in the resulting history, and we may need to differentiate what was in the index and what was only in the working tree). Ignoring the "conflicts at stash pop time" issue for now, a rough outline may look like: git-rebase --autostash git commit --allow-empty -m 'index part' git commit --allow-empty -a -m 'working tree part' git rebase --onto $UPSTREAM git reset HEAD^ git reset --soft HEAD^ The first "reset" is to match the index to what was "stashed" with the first "commit" ('index part') while keeping the working tree changes the original WIP had ('working tree part'), and the last "reset --soft" is to move the HEAD back to the tip of the originally committed history. -- 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] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > Junio C Hamano wrote: >> In the end, simple cases (the canonical example being Linus keeping >> a local change to "Name = Unicycling Gorilla" in his Makefile while >> running dozens of "git pull" every day) would not be helped with >> this feature very much because there is rarely overlap, while in a >> complex case the user really needs help to save away his WIP, the >> user is forced to resolve the conflict immediately upon the failed >> "stash pop" at the end of "git pull". If the conflict turns out to >> be something the user would not want to resolve at that moment, the >> user needs to know the way out, something like: >> >> git reset --hard;# go back to the result of pull >> git checkout -b wip ORIG_HEAD ;# start from before the pull >> git stash pop ;# recover his wip >> ... perhaps work a bit more ... >> git commit -a -m wip;# save it away for safety >> git checkout - ;# result of pull >> >> so that he can come back tomorrow, check out "wip" branch, and >> decide what to do. > > What does this have to do with pull at all? Exactly. By tempting the user to use autostash first, you are forcing the user to say "reset --hard" (the first one), "ORIG_HEAD" (to start from the pre-pull state), "stash pop" (to recover the autostashed WIP), to a user who got conflict during "stash pop" after the pull integrated the committed work with the remote side. If the user did this instead: ... Let's save my large WIP away in a more permanent place first git checkout -b wip ... perhaps work a bit more ... git commit -a -m wip git checkout - ... and then ... git pull the user wouldn't have had to do those extra steps. Another thing to consider is that even with the current system, many users do not have a clear idea on what the state of the working tree is when a "pull" fails (there are largely two classes). Either the merge failed without damaging the WIP before the pull at all, or there wasn't any interaction between the WIP and the change pull wanted to bring in and only the paths with conflicts need to be resolved and added (i.e. "commit -a" is a no-no). This "auto-pop" adds a third failure mode to "pull". It is a non-issue for Git-heads like us (we do read the messages from the programs), but not all users are like us. >> The "--autosquash" option (or not adding either the configuration or >> the option) would encourage the user to think about the nature of >> WIP he has in the working tree before running "git pull". > > I'm lost. What does git rebase --autosquash (or rebase.autosquash) > have anything to do with git pull --autostash? Sorry, the typo meant --autostash. >> Is it a >> too complex beast that may interfere with what others are doing, or >> is it a trivial change that he can stash away and pop it back? If >> the former, he will be far better off doing "checkout -b", keep >> working until the local change gets into somewhat a better shape and >> "commit", before pulling into the original branch. > > A pull-merge person having complex worktree changes should not execute > git pull blindly in the first place. That's what git fetch is for: Huh? You are not making sense. I would understand it if it were "That's what git commit is for", though. > But I'm a pull-rebase guy, and I always want pull.autostash. That is why I said the equation is different for pull-rebase, where "rebase" insists that you start from a squeaky clean working tree, so you can check the condition early before "git pull" even starts. While --autostash will not improve the life for pull-merge people (and pull.autostash will actively hurt them), pull.autostash does help pull-rebase people work around the limitation of "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
Re: [PATCH 3/3] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra wrote: > Okay, so you're proposing a uniform --autostash feature for both merge > and rebase. Sorry, I didn't get it last time due to the typos in your > email; yeah, this is worth investigating. So, I thought about this and have concluded that pull is the Right place for autostash because of the following reasons: 0. autostash is purely a convenience function that's useful in simple reduced cases like a dumb 'git pull'; when the script we're implementing it for doesn't have a ton of command-line options. 1. git stash; git pull; git stash pop; is a very common idiom. git stash; git rebase master; git stash pop; is a less common idiom, but I agree that it is one rebase case where autostash could be useful. Having git rebase -i show "exec git stash pop" at the end of a user-editable instruction sheet is less than ideal. Having git rebase --onto master HEAD~3 do an autostash _might_ be useful, but I'm not sure about it. git stash; git merge feature-branch; git stash pop; is not a common idiom at all. 2. git-stash.sh is a helper script, in the same spirit as git-pull.sh and git-rebase.sh. It is natural and easy to implement autostash for pull and rebase, but not for a C builtin like merge. In fact, it's impossible to implement it for merge unless we make git commit execute git stash pop (yuck!). If you want, you can implement a rebase.autostash, but that is not my itch. Considering the gymnastics the implementation would have to do, I'd be against a merge.autostash. So, again: what is your gripe against my pull.autostash implementation, apart from the fact that git status doesn't show stash information? (I _might_ decide to fix this) -- 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] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: > I disagree. A configuration option is something you set once, and then > forget about. A command, or a command-line option, is something you > explicitely add when you need it. You're making it out to be a much bigger difference than it actually is. Users will simply alias pull to 'pull --autostash' (a lot of them already alias it to pull --ff-only, and I'm going to fix this soon). The decision making process for creating a configuration variable shouldn't be "this is potentially dangerous, and therefore therefore it shouldn't be a configuration variable", but rather "this is a rarely used option that users only need <50% of the time, and therefore it shouldn't be a configuration variable". In my case, pull.autostash is my 90~95% usecase, and I'm not unique in this aspect. Therefore, it should be a configuration variable that can be consciously turned off with a --no-autostash. If your criticism were that git status doesn't show stash state, I agree with you. However, I don't agree with basing it on user forgetfulness in having set pull.autostash a long time ago + lack of observation skills to notice the message printed by git pull. -- 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] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: > AFAICT, "git merge --abort" is an alias for "git reset --merge" Yes, that is correct. > which > was precisely designed to reset only modifications comming from a merge, > and not the local changes that were present before the merge was > started. The man pages are relatively obscure on the subject, but I'd > call that a documentation bug. I see. Either way, we need a clean worktree for it to work, no? > It does. stashing means the user will have to "stash pop" later. One > extra step, one extra opportunity to forget something important. That's only if there are conflicts. If there are conflicts, you'll have to stash anyway if: - You're doing a pull-merge and want merge --abort to work. - You're doing a pull-rebase. In the case when there are no conflicts, what is the problem? > A minor annoyance is that it will touch files that have no reason to be > touched, hence may trigger extra rebuilds with "make", disturbing text > editors that have the file open, etc. Okay, I need to ask you something at this point: do you ever run merge on a dirty worktree unless you're absolutely sure that your local changes won't conflict with the changes introduced by the merge? I _never_ run merge on a dirty worktree myself. > The fact that "git pull" just works on dirty trees with non-overlapping > changes is an important feature of Git. That's only a pull-merge. Unfortunately, making git-pull.sh uniform means that we have to fall back to the least-common-denominator of functionality (which is currently pull-rebase). > I think you're taking it the wrong way. You seem to insist that > autostash should be a pull feature. I think it should be a feature of > merge and rebase, and the convenience script "git pull" should expose > them to the user. > > I see no reason to prevent users running fetch and then merge or rebase > to use the autostash feature. Okay, so you're proposing a uniform --autostash feature for both merge and rebase. Sorry, I didn't get it last time due to the typos in your email; yeah, this is worth investigating. > As a user, when I run "git rebase --continue" and it tells me it's done, > I expect the work to actually be done. This is the case today. This > won't be the case after autostash is introduced if the user has to > remember to run "stash pop" afterwards. And how will you implement that for merge, since there is no merge --continue to execute stash pop from? Do you propose to make commit do the stash pop'ing? -- 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] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > First, this is not why Junio said pull.autostash is harmful: he had a > completely different objection. Yes. This is another objection. > Second, this makes no sense. The user consciously made the decision > to set pull.autostash to true ... possibly a long time ago. > This case is roughly equivalent to the user executing 'git stash' and > suddenly waking up one day and complaining "WTF, where are my > important changes?!?". I disagree. A configuration option is something you set once, and then forget about. A command, or a command-line option, is something you explicitely add when you need it. -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > In the pull-merge case, maybe: if your worktree isn't clean, you lose > a lot of goodies like merge --abort anyway, and I hate that. AFAICT, "git merge --abort" is an alias for "git reset --merge" which was precisely designed to reset only modifications comming from a merge, and not the local changes that were present before the merge was started. The man pages are relatively obscure on the subject, but I'd call that a documentation bug. > Overall, I don't see how an extra stash/ stash pop where not > absolutely necessary hurts. It does. stashing means the user will have to "stash pop" later. One extra step, one extra opportunity to forget something important. A minor annoyance is that it will touch files that have no reason to be touched, hence may trigger extra rebuilds with "make", disturbing text editors that have the file open, etc. The fact that "git pull" just works on dirty trees with non-overlapping changes is an important feature of Git. >> Shouldn't this belong to "git merge" instead (i.e. implement "git merge >> --autosquash" and call it from "git pull")? Users doing "git fetch && >> git merge" by hand should be able to use --autosquash, I think. > > --autosquash reminds me of rebase.autosquash, and that is completely > unrelated to the issue at hand. A typo seems to have propagated in this thread. I meant auto*stash*, not auto*squash*, sorry. I guess it's the same typo in Junio's message. >> Something should be done for "git rebase" and "git pull --rebase" too. >> In this case, the UI can be much smoother, since the user would have to >> run "git rebase --continue" anyway, so you can do the auto-stash-pop for >> him by adding something like "exec git stash pop" at the end of the >> todo-list. > > No. I'm against executing a special codepath for a pull-rebase that > has no equivalent in the pull-merge world. I think you're taking it the wrong way. You seem to insist that autostash should be a pull feature. I think it should be a feature of merge and rebase, and the convenience script "git pull" should expose them to the user. I see no reason to prevent users running fetch and then merge or rebase to use the autostash feature. >> Ideally, "git rebase --abort" should auto-stash-pop too, although this >> is much less trivial to implement. > > As I already pointed out in my message to Junio, "fixing" rebase is > not the topic of discussion at all. It's not about fixing the existing rebase. It's about implementing autostash the right way. As a user, when I run "git rebase --continue" and it tells me it's done, I expect the work to actually be done. This is the case today. This won't be the case after autostash is introduced if the user has to remember to run "stash pop" afterwards. -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: > I tend to agree that pull.autostash is harmful. At least in its current > form, it is really too easy to overlook the "Please run 'git stash pop' > after commiting the conflict resolution." message: > > > $ git pull > > $ git status > > $ git commit > First, this is not why Junio said pull.autostash is harmful: he had a completely different objection. Second, this makes no sense. The user consciously made the decision to set pull.autostash to true: it isn't turned on by default, and there's no magic that's turning it on without the user knowing. This case is roughly equivalent to the user executing 'git stash' and suddenly waking up one day and complaining "WTF, where are my important changes?!?". Sure, it might be a good idea to teach git status to show the stash state, but that's an orthogonal topic. -- 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] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: > Isn't this too pessimistic? If the local changes do not overlap (in > terms of files) with the pulled changes, then autosquash is not needed. > > There should be a test for the case of non-overlapping changes. In the pull-rebase case, no; it is not too pessimistic. In the pull-merge case, maybe: if your worktree isn't clean, you lose a lot of goodies like merge --abort anyway, and I hate that. Secondly, it's impossible to determine whether the changes overlap or not before actually running merge_trees(). If there were an easy way to do it, I might have considered it. Overall, I don't see how an extra stash/ stash pop where not absolutely necessary hurts. > Shouldn't this belong to "git merge" instead (i.e. implement "git merge > --autosquash" and call it from "git pull")? Users doing "git fetch && > git merge" by hand should be able to use --autosquash, I think. --autosquash reminds me of rebase.autosquash, and that is completely unrelated to the issue at hand. Did you mean git merge --squash (to update the worktree/index but not create the actual commit?). Sure, it's probably useful to have a merge.squash configuration variable, but I don't see what it has to do with the pull.autostash I'm proposing. > Something should be done for "git rebase" and "git pull --rebase" too. > In this case, the UI can be much smoother, since the user would have to > run "git rebase --continue" anyway, so you can do the auto-stash-pop for > him by adding something like "exec git stash pop" at the end of the todo-list. No. I'm against executing a special codepath for a pull-rebase that has no equivalent in the pull-merge world. Or did you mean: have one configuration variable to git merge --squash and do this for git rebase, as if they're equivalent from the pull perspective? No, they aren't. > Ideally, "git rebase --abort" should auto-stash-pop too, although this > is much less trivial to implement. As I already pointed out in my message to Junio, "fixing" rebase is not the topic of discussion at all. > Maybe a good first step is to implement --autosquash only for non-rebase > pull, and later to implement "git rebase --autosquash" and call it from > "git pull". "Implement" git rebase --autosquash? If I just set rebase.autosquash to true, the rebase will automatically autosquash whether called from git pull or otherwise. Sorry, I just don't understand what you're saying. -- 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] pull: introduce --[no-]autostash and pull.autostash
Junio C Hamano wrote: > In the end, simple cases (the canonical example being Linus keeping > a local change to "Name = Unicycling Gorilla" in his Makefile while > running dozens of "git pull" every day) would not be helped with > this feature very much because there is rarely overlap, while in a > complex case the user really needs help to save away his WIP, the > user is forced to resolve the conflict immediately upon the failed > "stash pop" at the end of "git pull". If the conflict turns out to > be something the user would not want to resolve at that moment, the > user needs to know the way out, something like: > > git reset --hard;# go back to the result of pull > git checkout -b wip ORIG_HEAD ;# start from before the pull > git stash pop ;# recover his wip > ... perhaps work a bit more ... > git commit -a -m wip;# save it away for safety > git checkout - ;# result of pull > > so that he can come back tomorrow, check out "wip" branch, and > decide what to do. What does this have to do with pull at all? git-pull.sh is a simple shell script that runs fetch followed by a merge or rebase. You're handling a merge now, and there's proper support for it in the tool; git merge --abort will work with one caveat: "If there were uncommitted worktree changes present when the merge started, git merge --abort will in some cases be unable to reconstruct these changes. It is therefore recommended to always commit or stash your changes before running git merge.", to directly quote the manpage. Really, it has the same problem as rebase, if you want --abort to work. "Fixing" git merge --abort and git rebase are topics for another day. What I'm doing it making it easier to work with the merge/rebase following the fetch by automatically stashing worktree + index changes. > The "--autosquash" option (or not adding either the configuration or > the option) would encourage the user to think about the nature of > WIP he has in the working tree before running "git pull". I'm lost. What does git rebase --autosquash (or rebase.autosquash) have anything to do with git pull --autostash? > Is it a > too complex beast that may interfere with what others are doing, or > is it a trivial change that he can stash away and pop it back? If > the former, he will be far better off doing "checkout -b", keep > working until the local change gets into somewhat a better shape and > "commit", before pulling into the original branch. A pull-merge person having complex worktree changes should not execute git pull blindly in the first place. That's what git fetch is for: inspect the incoming changes, and decide how to integrate it into the local branch. git pull is a tool that serves an entirely different purpose, in my opinion. > But I suspect that pull.autostash > configuration is not a good addition because it encourages a bad, > pain-inducing workflow. In simple cases it may not hurt, but when > local changes are complex, it would actively hurt than not having it, > and the configuration robs the incentive to choose. But I'm a pull-rebase guy, and I always want pull.autostash. Will you deny me the configuration variable, simply because it is potentially pain-inducing to pull-merge people who set it blindly and run git pull blindly? I'm not arguing linking pull.autostash to pull.rebase either: my change is admittedly useful to *some* pull-merge people. And I'm not arguing for making it true by default. > The equation is somewhat different for "pull-rebase", as "rebase" > insists you to start from a clean working tree, so "download and > then stop" annoyance feels bigger. I have a suspicion that > loosening that may be a more productive fix to the real problem. Rebase performs relatively simple operation on the worktree, and I would like to keep it that way. I have no desire to "fix" rebase. > (require_clean_work_tree --quiet) || git stash Good point. Will fix in the next iteration. > This "cannot pop stash" is really where the user gets in the deep > yoghurt and needs help. Yes, and your point being? That life would've been easier if the user had committed all those changes in the first place? And that the penalty for not having done that is now a git checkout wip ORIG_HEAD; git stash pop; git checkout master; git rebase wip? > Please resolve conflicts and commit, and then 'git stash pop'. Cool, will fix. -- 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] pull: introduce --[no-]autostash and pull.autostash
Junio C Hamano writes: > Yes, that is why I said for pull-merge, --authsquash is neutral-to-better > and pull.autosquash is harmful. To speak from my experience: I find myself typing "git stash && git pull && git stash pop" relatively often. Typical use-case: I start working on something, a colleague works on the same thing and I need to see what he did to continue. Probably not something so frequent for large projects, but very frequent for small projects (e.g. writing a paper together with a single .tex file and the deadline approaching). In this case, "git pull --rebase" makes sense for advanced enough users, but newbies who have been told "rebase is too dangerous for you, don't use it", it would be cool to have --autostash too. I tend to agree that pull.autostash is harmful. At least in its current form, it is really too easy to overlook the "Please run 'git stash pop' after commiting the conflict resolution." message: $ git pull $ git status $ git commit rebase wouldn't have this issue if "stash pop" is part of the sequence to apply. -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy writes: > Ramkumar Ramachandra writes: > >> +stash_required () { >> +! (git diff-files --quiet --ignore-submodules && >> +git diff-index --quiet --cached HEAD --ignore-submodules) >> +} > > Isn't this too pessimistic? If the local changes do not overlap (in > terms of files) with the pulled changes, then autosquash is not needed. Yes, that is why I said for pull-merge, --authsquash is neutral-to-better and pull.autosquash is harmful. But for pull-rebase folks, I can understand why this "working tree must be squeakily clean" logic is appropriate, if we were to do this. The root cause is because rebase insists to be run on such a working tree. And the worst part is that in order to check if local changes overlap, you need to fetch first. But Ram's annoyance is about the user being told the merge/rebase cannot proceed _after_ fetch is done. > Shouldn't this belong to "git merge" instead (i.e. implement "git merge > --autosquash" and call it from "git pull")? Users doing "git fetch && > git merge" by hand should be able to use --autosquash, I think. See my other message. I do not think autosquash would help "merge" folks very much, and will actively hurt when it matters. > Something should be done for "git rebase" and "git pull --rebase" too. That I would agree. I am not sure autosquash is the best approach, but we should be able to help them more. -- 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] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > +stash_required () { > + ! (git diff-files --quiet --ignore-submodules && > + git diff-index --quiet --cached HEAD --ignore-submodules) > +} Isn't this too pessimistic? If the local changes do not overlap (in terms of files) with the pulled changes, then autosquash is not needed. There should be a test for the case of non-overlapping changes. > +if test "$autostash" = true && stash_required > +then > + git stash || die "$(gettext "Cannot autostash")" && > + require_clean_work_tree "pull" "Please commit or stash them." && > + if eval "$eval" > + then > + git stash pop || die "$(gettext "Cannot pop stash")" > + else > + exit_code=$? > + echo "Please run 'git stash pop' after commiting the conflict > resolution." > + exit $exit_code > + fi > +else > + eval "exec $eval" > +fi Shouldn't this belong to "git merge" instead (i.e. implement "git merge --autosquash" and call it from "git pull")? Users doing "git fetch && git merge" by hand should be able to use --autosquash, I think. Something should be done for "git rebase" and "git pull --rebase" too. In this case, the UI can be much smoother, since the user would have to run "git rebase --continue" anyway, so you can do the auto-stash-pop for him by adding something like "exec git stash pop" at the end of the todo-list. Ideally, "git rebase --abort" should auto-stash-pop too, although this is much less trivial to implement. Maybe a good first step is to implement --autosquash only for non-rebase pull, and later to implement "git rebase --autosquash" and call it from "git pull". -- 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 3/3] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra writes: > Currently, executing a 'git pull' on a dirty worktree gives the > following annoying message: s/annoying//; In general, avoid forcing your value judgement to readers when you do not have to. Instead, you can just highlight the source of your annoyance and let them naturally feel the annoyance themselves, perhaps like this: When the user runs "git pull" with local changes in the working tree that interfere with the merge (or any change if the user is using --rebase), the user is told that the merge or rebase cannot be done _after_ the command spends time to download necessary objects. The user can then decide not to perform the pull and finish what he was in the middle of doing (e.g. by "git checkout -b topic" and then finishing the work), before coming back to the branch and running "git pull" again, or "git stash" the work in progress, run "git pull" and then "git stash pop". Introduce a new flag "git pull --autostash" to perform the latter automatically. Also add pull.autostash configuration to do so without giving the command line flag, whose effect can be overriden by the --no-autostash option. While I understand where this wish comes from, I am not sure if this is generally a good idea, or if we are solving a wrong problem. > Saved working directory and index state WIP on master: 8ea73ed baz > HEAD is now at 8ea73ed baz > > ... # The merge/rebase process I think there is one step missing after "The merge/rebase process" above, which is ... # The 'stash pop' process > Dropped refs/stash@{0} (83f47fbfb07a741817633f9191dc4a1530f79249) If the pull-merge were something that would induce the "annoyance" that triggered this topic, by definition, the local change overlaps with the merge, and this internal "stash pop" will touch the paths the merge touched and it is likely not result in "Dropped" but leave further conflicts to be resolved. In the end, simple cases (the canonical example being Linus keeping a local change to "Name = Unicycling Gorilla" in his Makefile while running dozens of "git pull" every day) would not be helped with this feature very much because there is rarely overlap, while in a complex case the user really needs help to save away his WIP, the user is forced to resolve the conflict immediately upon the failed "stash pop" at the end of "git pull". If the conflict turns out to be something the user would not want to resolve at that moment, the user needs to know the way out, something like: git reset --hard;# go back to the result of pull git checkout -b wip ORIG_HEAD ;# start from before the pull git stash pop ;# recover his wip ... perhaps work a bit more ... git commit -a -m wip;# save it away for safety git checkout - ;# result of pull so that he can come back tomorrow, check out "wip" branch, and decide what to do. The "--autosquash" option (or not adding either the configuration or the option) would encourage the user to think about the nature of WIP he has in the working tree before running "git pull". Is it a too complex beast that may interfere with what others are doing, or is it a trivial change that he can stash away and pop it back? If the former, he will be far better off doing "checkout -b", keep working until the local change gets into somewhat a better shape and "commit", before pulling into the original branch. If the latter, he is better off doing either - "git pull", after finding it conflicts, run "git stash", "git merge FETCH_HEAD" and "git stash pop"; or - "git pull --autostash". And not having "--autostash" would mean he would do the former, and will do "stash" only when it matters. "--autostash" is a good to neutral addition in that sense. But I suspect that pull.autostash configuration is not a good addition because it encourages a bad, pain-inducing workflow. In simple cases it may not hurt, but when local changes are complex, it would actively hurt than not having it, and the configuration robs the incentive to choose. The equation is somewhat different for "pull-rebase", as "rebase" insists you to start from a clean working tree, so "download and then stop" annoyance feels bigger. I have a suspicion that loosening that may be a more productive fix to the real problem. > +stash_required () { > + ! (git diff-files --quiet --ignore-submodules && > + git diff-index --quiet --cached HEAD --ignore-submodules) > +} require_clean_work_tree refreshes the index before running diff-files to make this check safe, but you do not do that here. Do you know the index has been refreshed when this is called? Even though this is mere two-line logic, the duplication of the logic look a bit wasteful. Is it too hard to teach "dry-run, quiet" mode to require_cle