Re: [PATCH] rebase -i: introduce the 'test' command
On Tue, Dec 11, 2018 at 01:40:25PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> Are you thinking of the "break" command (not "pause") which Dscho > >> already added[1]? > >> > >> [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12) > > > > Yes, thanks (as you can see, I haven't actually used it yet ;) ). > > Related: I got poked about a bug where someone was doing "exec bash" to > do the same, and would then CD to other repos, and git operations would > still be executed on the original repo because we set GIT_DIR=* in the > envioronment (but not for "break"). > > Not a big deal, but I wondered if that was a bug in itself, i.e. if we > need to set GIT_DIR et al in the environment for "exec". Isn't having > the current directory set to the checkout sufficient? This behavior is discussed in this sub-thread: https://public-inbox.org/git/xmqqsh4jt5d0@gitster-ct.c.googlers.com/ -Peff
Re: [PATCH] rebase -i: introduce the 'test' command
On Sun, Dec 02 2018, Jeff King wrote: > On Sat, Dec 01, 2018 at 09:28:47PM -0500, Eric Sunshine wrote: > >> On Sat, Dec 1, 2018 at 3:02 PM Jeff King wrote: >> > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: >> > > In reality, I think that it would even make sense to change the default >> > > to >> > > reschedule failed `exec` commands. Which is why I suggested to also add a >> > > config option. >> > >> > I sometimes add "x false" to the top of the todo list to stop and create >> > new commits before the first one. That would be awkward if I could never >> > get past that line. However, I think elsewhere a "pause" line has been >> > discussed, which would serve the same purpose. >> >> Are you thinking of the "break" command (not "pause") which Dscho >> already added[1]? >> >> [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12) > > Yes, thanks (as you can see, I haven't actually used it yet ;) ). Related: I got poked about a bug where someone was doing "exec bash" to do the same, and would then CD to other repos, and git operations would still be executed on the original repo because we set GIT_DIR=* in the envioronment (but not for "break"). Not a big deal, but I wondered if that was a bug in itself, i.e. if we need to set GIT_DIR et al in the environment for "exec". Isn't having the current directory set to the checkout sufficient?
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Peff, On Mon, 3 Dec 2018, Jeff King wrote: > On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote: > > > > In this sort of situation, I often whish to be able to do nested rebases. > > > Even more because it happen relatively often that I forget that I'm > > > working in a rebase and not on the head, and then it's quite natural > > > to me to type things like 'git rebase -i @^^^' while already rebasing. > > > But I suppose this has already been discussed. > > > > Varieties of this have been discussed, but no, not nested rebases. > > > > The closest we thought about was re-scheduling the latest commits, > > which is now harder because of the `--rebase-merges` mode. > > > > But I think it would be doable. Your idea of a "nested" rebase actually > > opens that door quite nicely. It would not *really* be a nested rebase, > > and it would still only be possible in interactive mode, but I could > > totally see > > > > git rebase --nested -i HEAD~3 > > > > to generate and prepend the following lines to the `git-rebase-todo` file: > > > > reset abcdef01 # This is HEAD~3 > > pick abcdef02 # This is HEAD~2 > > pick abcdef03 # This is HEAD~ > > pick abcdef04 # This is HEAD > > > > (assuming that the latest 3 commits were non-merge commits; It would look > > quite a bit more complicated in other situations.) > > Yeah, I would probably use that if it existed. I kind of use it, even if it does not exist ;-) > It would be nicer to have real nested sequencer operations, I think, for > other situations. I agree. But for the moment, our data format is too married to the exact layout of .git/, thanks to `git rebase`'s evolution from a Unix shell script. Alban has this really great patch series to work on the todo list in-memory, and that paves the way to decouple the entire sequencer thing from the file system. The most notably thing that still would need to be encapsulated would be the options: currently, there is a plethora of inconsistent options files being saved into the state directory (for some, the mere presence indicates `true`, some contain `true` or `false`, others contain text, etc). > E.g., cherry-picking a sequence of commits while you're in the middle of > a rebase. You will be delighted to learn that you can cherry-pick a sequence of commits in the middle of a rebase already. I do `exec git cherry-pick ` *all* the time. > But I suspect getting that right would be _loads_ more work, and > probably would involve some funky UI corner cases to handle the stack of > operations (so truly aborting a rebase may mean an arbitrary number of > "rebase --abort" calls to pop the stack). Your suggestion is probably a > reasonable trick in the meantime. You know what is an even more reasonable trick? Worktrees. I only thought about that this morning, but I should have mentioned it right away, as I use it quite frequently. When I have tricky nested rebases to perform, I do use throw-away worktrees where I check out unnamed branches, work on those, and then integrate them back into the "outer rebase" via the `reset` command in the todo list. Ciao, Dscho
Re: [PATCH] rebase -i: introduce the 'test' command
On Mon, Dec 03, 2018 at 06:53:22PM +0100, Duy Nguyen wrote: > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > I sometimes add "x false" to the top of the todo list to stop and create > > new commits before the first one. > > And here I've been doing the same by "edit" the first commit, add a > new commit then reorder them in the second interactive rebase :P > > This made me look at git-rebase.txt to really learn about interactive > rebase. I think the interactive rebase section could use some > improvements. Its style looks.. umm.. more story telling than a > reference. Perhaps something like this to at least highlight the > commands. Yeah, I think the typographic change is an improvement that doesn't really have a downside. As Dscho mentioned, sometimes the story style is what you want, so I don't think we'd want to simply rearrange it. It may be useful to present the material twice, though: once as a table/list for reference, and then once as a story working through an example. -Peff
Re: [PATCH] rebase -i: introduce the 'test' command
On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote: > > In this sort of situation, I often whish to be able to do nested rebases. > > Even more because it happen relatively often that I forget that I'm > > working in a rebase and not on the head, and then it's quite natural > > to me to type things like 'git rebase -i @^^^' while already rebasing. > > But I suppose this has already been discussed. > > Varieties of this have been discussed, but no, not nested rebases. > > The closest we thought about was re-scheduling the latest commits, > which is now harder because of the `--rebase-merges` mode. > > But I think it would be doable. Your idea of a "nested" rebase actually > opens that door quite nicely. It would not *really* be a nested rebase, > and it would still only be possible in interactive mode, but I could > totally see > > git rebase --nested -i HEAD~3 > > to generate and prepend the following lines to the `git-rebase-todo` file: > > reset abcdef01 # This is HEAD~3 > pick abcdef02 # This is HEAD~2 > pick abcdef03 # This is HEAD~ > pick abcdef04 # This is HEAD > > (assuming that the latest 3 commits were non-merge commits; It would look > quite a bit more complicated in other situations.) Yeah, I would probably use that if it existed. It would be nicer to have real nested sequencer operations, I think, for other situations. E.g., cherry-picking a sequence of commits while you're in the middle of a rebase. But I suspect getting that right would be _loads_ more work, and probably would involve some funky UI corner cases to handle the stack of operations (so truly aborting a rebase may mean an arbitrary number of "rebase --abort" calls to pop the stack). Your suggestion is probably a reasonable trick in the meantime. -Peff
Re: [PATCH] rebase -i: introduce the 'test' command
On Mon, Dec 03, 2018 at 02:31:37PM +, Phillip Wood wrote: > > How would I move past the test that fails to continue? I guess "git > > rebase --edit-todo" and then manually remove it (and any other remaining > > test lines)? > > Perhaps we could teach git rebase --skip to skip a rescheduled command, it > could be useful if people want to skip rescheduled picks as well (though I > don't think I've ever had that happen in the wild). I can see myself turning > on the rescheduling config setting but occasionally wanting to be able to > skip over the rescheduled exec command. Yeah, I agree that would give a nice user experience. -Peff
Re: [PATCH] rebase -i: introduce the 'test' command
On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote: > Hi Luc, > > On Mon, 3 Dec 2018, Luc Van Oostenryck wrote: > > > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > > I sometimes add "x false" to the top of the todo list to stop and create > > > new commits before the first one. That would be awkward if I could never > > > get past that line. However, I think elsewhere a "pause" line has been > > > discussed, which would serve the same purpose. > > > > > > I wonder how often this kind of "yes, I know it fails, but keep going > > > anyway" situation would come up. And what the interface is like for > > > getting past it. E.g., what if you fixed a bunch of stuff but your tests > > > still fail? You may not want to abandon the changes you've made, but you > > > need to "rebase --continue" to move forward. I encounter this often when > > > the correct fix is actually in an earlier commit than the one that > > > yields the test failure. You can't rewind an interactive rebase, so I > > > complete and restart it, adding an "e"dit at the earlier commit. > > > > In this sort of situation, I often whish to be able to do nested rebases. > > Even more because it happen relatively often that I forget that I'm > > working in a rebase and not on the head, and then it's quite natural > > to me to type things like 'git rebase -i @^^^' while already rebasing. > > But I suppose this has already been discussed. > > Varieties of this have been discussed, but no, not nested rebases. Interesting :) > The closest we thought about was re-scheduling the latest commits, > which is now harder because of the `--rebase-merges` mode. > > But I think it would be doable. Your idea of a "nested" rebase actually > opens that door quite nicely. It would not *really* be a nested rebase, > and it would still only be possible in interactive mode, but I could > totally see > > git rebase --nested -i HEAD~3 I don't mind much if it would be "really nested" or "as-if nested" but with this flag --nested I wonder what would happen if I would use it in a 'top-level' rebase (or, said in another way, would I be able to alias 'rebase' to 'rebase --nested')? > to generate and prepend the following lines to the `git-rebase-todo` file: > > reset abcdef01 # This is HEAD~3 > pick abcdef02 # This is HEAD~2 > pick abcdef03 # This is HEAD~ > pick abcdef04 # This is HEAD > OK, I see. This would not be nestable/stackable but would solve the problem nicely. Best regards, -- Luc
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Duy, On Mon, 3 Dec 2018, Duy Nguyen wrote: > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > I sometimes add "x false" to the top of the todo list to stop and create > > new commits before the first one. > > And here I've been doing the same by "edit" the first commit, add a > new commit then reorder them in the second interactive rebase :P > > This made me look at git-rebase.txt to really learn about interactive > rebase. I think the interactive rebase section could use some > improvements. Its style looks.. umm.. more story telling than a > reference. Perhaps something like this to at least highlight the > commands. And maybe, just maybe, that "story telling" is more useful for users who want to learn about the interactive rebase, just like yourself, when compared to a mere "reference". Ciao, Johannes
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Luc, On Mon, 3 Dec 2018, Luc Van Oostenryck wrote: > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > > > > > Would it not make more sense to add a command-line option (and a > > > > > config > > > > > setting) to re-schedule failed `exec` commands? Like so: > > > > > > > > Your proposition would do in most cases, however it is not possible to > > > > make a distinction between reschedulable and non-reschedulable commands. > > > > > > True. But I don't think that's so terrible. > > > > > > What I think is something to avoid is two commands that do something very, > > > very similar, but with two very, very different names. > > > > > > In reality, I think that it would even make sense to change the default to > > > reschedule failed `exec` commands. Which is why I suggested to also add a > > > config option. > > > > I sometimes add "x false" to the top of the todo list to stop and create > > new commits before the first one. That would be awkward if I could never > > get past that line. However, I think elsewhere a "pause" line has been > > discussed, which would serve the same purpose. > > > > I wonder how often this kind of "yes, I know it fails, but keep going > > anyway" situation would come up. And what the interface is like for > > getting past it. E.g., what if you fixed a bunch of stuff but your tests > > still fail? You may not want to abandon the changes you've made, but you > > need to "rebase --continue" to move forward. I encounter this often when > > the correct fix is actually in an earlier commit than the one that > > yields the test failure. You can't rewind an interactive rebase, so I > > complete and restart it, adding an "e"dit at the earlier commit. > > In this sort of situation, I often whish to be able to do nested rebases. > Even more because it happen relatively often that I forget that I'm > working in a rebase and not on the head, and then it's quite natural > to me to type things like 'git rebase -i @^^^' while already rebasing. > But I suppose this has already been discussed. Varieties of this have been discussed, but no, not nested rebases. The closest we thought about was re-scheduling the latest commits, which is now harder because of the `--rebase-merges` mode. But I think it would be doable. Your idea of a "nested" rebase actually opens that door quite nicely. It would not *really* be a nested rebase, and it would still only be possible in interactive mode, but I could totally see git rebase --nested -i HEAD~3 to generate and prepend the following lines to the `git-rebase-todo` file: reset abcdef01 # This is HEAD~3 pick abcdef02 # This is HEAD~2 pick abcdef03 # This is HEAD~ pick abcdef04 # This is HEAD (assuming that the latest 3 commits were non-merge commits; It would look quite a bit more complicated in other situations.) Ciao, Dscho
Re: [PATCH] rebase -i: introduce the 'test' command
On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > I sometimes add "x false" to the top of the todo list to stop and create > new commits before the first one. And here I've been doing the same by "edit" the first commit, add a new commit then reorder them in the second interactive rebase :P This made me look at git-rebase.txt to really learn about interactive rebase. I think the interactive rebase section could use some improvements. Its style looks.. umm.. more story telling than a reference. Perhaps something like this to at least highlight the commands. -- 8< -- diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 80793bad8d..c569b3370b 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -637,22 +637,22 @@ The oneline descriptions are purely for your pleasure; 'git rebase' will not look at them but at the commit names ("deadbee" and "fa1afe1" in this example), so do not delete or edit the names. -By replacing the command "pick" with the command "edit", you can tell +By replacing the command `pick` with the command `edit`, you can tell 'git rebase' to stop after applying that commit, so that you can edit the files and/or the commit message, amend the commit, and continue rebasing. To interrupt the rebase (just like an "edit" command would do, but without -cherry-picking any commit first), use the "break" command. +cherry-picking any commit first), use the `break` command. If you just want to edit the commit message for a commit, replace the -command "pick" with the command "reword". +command "pick" with the command `reword`. -To drop a commit, replace the command "pick" with "drop", or just +To drop a commit, replace the command "pick" with `drop`, or just delete the matching line. If you want to fold two or more commits into one, replace the command -"pick" for the second and subsequent commits with "squash" or "fixup". +"pick" for the second and subsequent commits with `squash` or `fixup`. If the commits had different authors, the folded commit will be attributed to the author of the first commit. The suggested commit message for the folded commit is the concatenation of the commit @@ -693,7 +693,7 @@ $ git rebase -i -p --onto Q O Reordering and editing commits usually creates untested intermediate steps. You may want to check that your history editing did not break anything by running a test, or at least recompiling at intermediate -points in history by using the "exec" command (shortcut "x"). You may +points in history by using the `exec` command (shortcut `x`). You may do so by creating a todo list like this one: --- -- 8< -- -- Duy
Re: [PATCH] rebase -i: introduce the 'test' command
On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > > > Would it not make more sense to add a command-line option (and a config > > > > setting) to re-schedule failed `exec` commands? Like so: > > > > > > Your proposition would do in most cases, however it is not possible to > > > make a distinction between reschedulable and non-reschedulable commands. > > > > True. But I don't think that's so terrible. > > > > What I think is something to avoid is two commands that do something very, > > very similar, but with two very, very different names. > > > > In reality, I think that it would even make sense to change the default to > > reschedule failed `exec` commands. Which is why I suggested to also add a > > config option. > > I sometimes add "x false" to the top of the todo list to stop and create > new commits before the first one. That would be awkward if I could never > get past that line. However, I think elsewhere a "pause" line has been > discussed, which would serve the same purpose. > > I wonder how often this kind of "yes, I know it fails, but keep going > anyway" situation would come up. And what the interface is like for > getting past it. E.g., what if you fixed a bunch of stuff but your tests > still fail? You may not want to abandon the changes you've made, but you > need to "rebase --continue" to move forward. I encounter this often when > the correct fix is actually in an earlier commit than the one that > yields the test failure. You can't rewind an interactive rebase, so I > complete and restart it, adding an "e"dit at the earlier commit. In this sort of situation, I often whish to be able to do nested rebases. Even more because it happen relatively often that I forget that I'm working in a rebase and not on the head, and then it's quite natural to me to type things like 'git rebase -i @^^^' while already rebasing. But I suppose this has already been discussed. -- Luc
Re: [PATCH] rebase -i: introduce the 'test' command
On 01/12/2018 20:02, Jeff King wrote: On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: Would it not make more sense to add a command-line option (and a config setting) to re-schedule failed `exec` commands? Like so: Your proposition would do in most cases, however it is not possible to make a distinction between reschedulable and non-reschedulable commands. True. But I don't think that's so terrible. What I think is something to avoid is two commands that do something very, very similar, but with two very, very different names. In reality, I think that it would even make sense to change the default to reschedule failed `exec` commands. Which is why I suggested to also add a config option. I sometimes add "x false" to the top of the todo list to stop and create new commits before the first one. That would be awkward if I could never get past that line. However, I think elsewhere a "pause" line has been discussed, which would serve the same purpose. I wonder how often this kind of "yes, I know it fails, but keep going anyway" situation would come up. And what the interface is like for getting past it. E.g., what if you fixed a bunch of stuff but your tests still fail? You may not want to abandon the changes you've made, but you need to "rebase --continue" to move forward. I encounter this often when the correct fix is actually in an earlier commit than the one that yields the test failure. You can't rewind an interactive rebase, so I complete and restart it, adding an "e"dit at the earlier commit. How would I move past the test that fails to continue? I guess "git rebase --edit-todo" and then manually remove it (and any other remaining test lines)? Perhaps we could teach git rebase --skip to skip a rescheduled command, it could be useful if people want to skip rescheduled picks as well (though I don't think I've ever had that happen in the wild). I can see myself turning on the rescheduling config setting but occasionally wanting to be able to skip over the rescheduled exec command. Best Wishes Phillip That's not too bad, but I wonder if people would find it more awkward than the current way (which is to just "rebase --continue" until you get to the end). I dunno. I am not sure if I am for or against changing the default, so these are just some musings. :) -Peff
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Peff, On Sat, 1 Dec 2018, Jeff King wrote: > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > > > Would it not make more sense to add a command-line option (and a config > > > > setting) to re-schedule failed `exec` commands? Like so: > > > > > > Your proposition would do in most cases, however it is not possible to > > > make a distinction between reschedulable and non-reschedulable commands. > > > > True. But I don't think that's so terrible. > > > > What I think is something to avoid is two commands that do something very, > > very similar, but with two very, very different names. > > > > In reality, I think that it would even make sense to change the default to > > reschedule failed `exec` commands. Which is why I suggested to also add a > > config option. > > I sometimes add "x false" to the top of the todo list to stop and create > new commits before the first one. That would be awkward if I could never > get past that line. However, I think elsewhere a "pause" line has been > discussed, which would serve the same purpose. Yep, `break`, as Eric pointed out. After all, you did not really want a command to fail, you just wanted the interactive rebase to give you a break. > I wonder how often this kind of "yes, I know it fails, but keep going > anyway" situation would come up. And what the interface is like for > getting past it. E.g., what if you fixed a bunch of stuff but your tests > still fail? You may not want to abandon the changes you've made, but you > need to "rebase --continue" to move forward. I encounter this often when > the correct fix is actually in an earlier commit than the one that > yields the test failure. You can't rewind an interactive rebase, so I > complete and restart it, adding an "e"dit at the earlier commit. > > How would I move past the test that fails to continue? I guess "git > rebase --edit-todo" and then manually remove it (and any other remaining > test lines)? Yes, the current way would be to use `git rebase --edit-todo`. > That's not too bad, but I wonder if people would find it more awkward > than the current way (which is to just "rebase --continue" until you get > to the end). > > I dunno. I am not sure if I am for or against changing the default, so > these are just some musings. :) It's good that you chimed in with your side of things. If you missed the `break` command, so will many others have missed it. And continue to miss it. Besides, Junio mentioned elsewhere that he is in the camp of people who wait for enough users to complain why some config option isn't the default to actually change the default. So... I guess we'll leave the default where it is for now. Ciao, Dscho
Re: [PATCH] rebase -i: introduce the 'test' command
On Sat, Dec 01, 2018 at 09:28:47PM -0500, Eric Sunshine wrote: > On Sat, Dec 1, 2018 at 3:02 PM Jeff King wrote: > > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > In reality, I think that it would even make sense to change the default to > > > reschedule failed `exec` commands. Which is why I suggested to also add a > > > config option. > > > > I sometimes add "x false" to the top of the todo list to stop and create > > new commits before the first one. That would be awkward if I could never > > get past that line. However, I think elsewhere a "pause" line has been > > discussed, which would serve the same purpose. > > Are you thinking of the "break" command (not "pause") which Dscho > already added[1]? > > [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12) Yes, thanks (as you can see, I haven't actually used it yet ;) ). -Peff
Re: [PATCH] rebase -i: introduce the 'test' command
On Sat, Dec 1, 2018 at 3:02 PM Jeff King wrote: > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > In reality, I think that it would even make sense to change the default to > > reschedule failed `exec` commands. Which is why I suggested to also add a > > config option. > > I sometimes add "x false" to the top of the todo list to stop and create > new commits before the first one. That would be awkward if I could never > get past that line. However, I think elsewhere a "pause" line has been > discussed, which would serve the same purpose. Are you thinking of the "break" command (not "pause") which Dscho already added[1]? [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12)
Re: [PATCH] rebase -i: introduce the 'test' command
On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > Would it not make more sense to add a command-line option (and a config > > > setting) to re-schedule failed `exec` commands? Like so: > > > > Your proposition would do in most cases, however it is not possible to > > make a distinction between reschedulable and non-reschedulable commands. > > True. But I don't think that's so terrible. > > What I think is something to avoid is two commands that do something very, > very similar, but with two very, very different names. > > In reality, I think that it would even make sense to change the default to > reschedule failed `exec` commands. Which is why I suggested to also add a > config option. I sometimes add "x false" to the top of the todo list to stop and create new commits before the first one. That would be awkward if I could never get past that line. However, I think elsewhere a "pause" line has been discussed, which would serve the same purpose. I wonder how often this kind of "yes, I know it fails, but keep going anyway" situation would come up. And what the interface is like for getting past it. E.g., what if you fixed a bunch of stuff but your tests still fail? You may not want to abandon the changes you've made, but you need to "rebase --continue" to move forward. I encounter this often when the correct fix is actually in an earlier commit than the one that yields the test failure. You can't rewind an interactive rebase, so I complete and restart it, adding an "e"dit at the earlier commit. How would I move past the test that fails to continue? I guess "git rebase --edit-todo" and then manually remove it (and any other remaining test lines)? That's not too bad, but I wonder if people would find it more awkward than the current way (which is to just "rebase --continue" until you get to the end). I dunno. I am not sure if I am for or against changing the default, so these are just some musings. :) -Peff
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Paul, On Thu, 29 Nov 2018, Johannes Schindelin wrote: > I already added a test... See the reschedule-failed-exec branch on > https://github.com/dscho/git. And now I put up a proper Pull Request, to be submitted via GitGitGadget right after Git v2.20.0 will be released (technically, we are in a feature freeze period right now, I just could no resist, as I found your reasoning for rescheduling failed `exec` commands compelling, and there were no `rebase` bugs for me to fix). https://github.com/gitgitgadget/git/pull/90 Ciao, Johannes
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Paul, On Wed, 28 Nov 2018, Paul Morelle wrote: > On 28/11/18 16:19, Johannes Schindelin wrote: > > > > On Wed, 28 Nov 2018, Paul Morelle wrote: > > > >> The 'exec' command can be used to run tests on a set of commits, > >> interrupting on failing commits to let the user fix the tests. > >> > >> However, the 'exec' line has been consumed, so it won't be ran again by > >> 'git rebase --continue' is ran, even if the tests weren't fixed. > >> > >> This commit introduces a new command 'test' equivalent to 'exec', except > >> that it is automatically rescheduled in the todo list if it fails. > >> > >> Signed-off-by: Paul Morelle > > Would it not make more sense to add a command-line option (and a config > > setting) to re-schedule failed `exec` commands? Like so: > > Your proposition would do in most cases, however it is not possible to > make a distinction between reschedulable and non-reschedulable commands. True. But I don't think that's so terrible. What I think is something to avoid is two commands that do something very, very similar, but with two very, very different names. In reality, I think that it would even make sense to change the default to reschedule failed `exec` commands. Which is why I suggested to also add a config option. > Do you think that we can ignore the distinction between reschedulable > and non-reschedulable commands in the same script? Yes, I think that there *should not* be any distinction. It would just make it harder to use the feature (users would have to keep in mind that one version reschedules, one version does not). > In this case, I would add some tests to your patch below, and propose > the result on this mailing list. I already added a test... See the reschedule-failed-exec branch on https://github.com/dscho/git. And I had another idea. To make this feature more useful for *myself*, I would like to introduce the `-X` option as a shortcut for `--reschedule-failed-exec -x`, e.g. git rebase -X "make -j15 test" What do you think? Johannes > > > -- snip -- > > diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c > > index a2ab68ed0632..a9ab009fdbca 100644 > > --- a/builtin/rebase--interactive.c > > +++ b/builtin/rebase--interactive.c > > @@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char > > **argv, const char *prefix) > > OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), > > N_("onto name")), > > OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")), > > OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto), > > + OPT_BOOL(0, "reschedule-failed-exec", > > &opts.reschedule_failed_exec, > > +N_("automatically re-schedule any `exec` that fails")), > > OPT_END() > > }; > > > > diff --git a/builtin/rebase.c b/builtin/rebase.c > > index 5b3e5baec8a0..700cbc4e150e 100644 > > --- a/builtin/rebase.c > > +++ b/builtin/rebase.c > > @@ -104,6 +104,7 @@ struct rebase_options { > > int rebase_merges, rebase_cousins; > > char *strategy, *strategy_opts; > > struct strbuf git_format_patch_opt; > > + int reschedule_failed_exec; > > }; > > > > static int is_interactive(struct rebase_options *opts) > > @@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options > > *opts) > > argv_array_push(&child.args, opts->gpg_sign_opt); > > if (opts->signoff) > > argv_array_push(&child.args, "--signoff"); > > + if (opts->reschedule_failed_exec) > > + argv_array_push(&child.args, > > "--reschedule-failed-exec"); > > > > status = run_command(&child); > > goto finished_rebase; > > @@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char > > *prefix) > >"strategy")), > > OPT_BOOL(0, "root", &options.root, > > N_("rebase all reachable commits up to the root(s)")), > > + OPT_BOOL(0, "reschedule-failed-exec", > > +&options.reschedule_failed_exec, > > +N_("automatically re-schedule any `exec` that fails")), > > OPT_END(), > > }; > > int i; > > @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const > > char *prefix) > > break; > > } > > > > + if (options.reschedule_failed_exec && !is_interactive(&options)) > > + die(_("--reschedule-failed-exec requires an interactive > > rebase")); > > + > > if (options.git_am_opts.argc) { > > /* all am options except -q are compatible only with --am */ > > for (i = options.git_am_opts.argc - 1; i >= 0; i--) > > diff --git a/sequencer.c b/sequencer.c > > index e1a4dd15f1a8..69bee63e116f 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, > > "rebase-merge/strategy") > > static GIT_PATH_FUNC(re
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Johannes, On 28/11/18 16:19, Johannes Schindelin wrote: > Hi Paul, > > On Wed, 28 Nov 2018, Paul Morelle wrote: > >> The 'exec' command can be used to run tests on a set of commits, >> interrupting on failing commits to let the user fix the tests. >> >> However, the 'exec' line has been consumed, so it won't be ran again by >> 'git rebase --continue' is ran, even if the tests weren't fixed. >> >> This commit introduces a new command 'test' equivalent to 'exec', except >> that it is automatically rescheduled in the todo list if it fails. >> >> Signed-off-by: Paul Morelle > Would it not make more sense to add a command-line option (and a config > setting) to re-schedule failed `exec` commands? Like so: Your proposition would do in most cases, however it is not possible to make a distinction between reschedulable and non-reschedulable commands. Do you think that we can ignore the distinction between reschedulable and non-reschedulable commands in the same script? In this case, I would add some tests to your patch below, and propose the result on this mailing list. > -- snip -- > diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c > index a2ab68ed0632..a9ab009fdbca 100644 > --- a/builtin/rebase--interactive.c > +++ b/builtin/rebase--interactive.c > @@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, > const char *prefix) > OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), > N_("onto name")), > OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")), > OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto), > + OPT_BOOL(0, "reschedule-failed-exec", > &opts.reschedule_failed_exec, > + N_("automatically re-schedule any `exec` that fails")), > OPT_END() > }; > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 5b3e5baec8a0..700cbc4e150e 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -104,6 +104,7 @@ struct rebase_options { > int rebase_merges, rebase_cousins; > char *strategy, *strategy_opts; > struct strbuf git_format_patch_opt; > + int reschedule_failed_exec; > }; > > static int is_interactive(struct rebase_options *opts) > @@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options > *opts) > argv_array_push(&child.args, opts->gpg_sign_opt); > if (opts->signoff) > argv_array_push(&child.args, "--signoff"); > + if (opts->reschedule_failed_exec) > + argv_array_push(&child.args, > "--reschedule-failed-exec"); > > status = run_command(&child); > goto finished_rebase; > @@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char > *prefix) > "strategy")), > OPT_BOOL(0, "root", &options.root, >N_("rebase all reachable commits up to the root(s)")), > + OPT_BOOL(0, "reschedule-failed-exec", > + &options.reschedule_failed_exec, > + N_("automatically re-schedule any `exec` that fails")), > OPT_END(), > }; > int i; > @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char > *prefix) > break; > } > > + if (options.reschedule_failed_exec && !is_interactive(&options)) > + die(_("--reschedule-failed-exec requires an interactive > rebase")); > + > if (options.git_am_opts.argc) { > /* all am options except -q are compatible only with --am */ > for (i = options.git_am_opts.argc - 1; i >= 0; i--) > diff --git a/sequencer.c b/sequencer.c > index e1a4dd15f1a8..69bee63e116f 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, > "rebase-merge/strategy") > static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts") > static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, > "rebase-merge/allow_rerere_autoupdate") > static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet") > +static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, > "rebase-merge/reschedule-failed-exec") > > static int git_sequencer_config(const char *k, const char *v, void *cb) > { > @@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts) > opts->signoff = 1; > } > > + if (file_exists(rebase_path_reschedule_failed_exec())) > + opts->reschedule_failed_exec = 1; > + > read_strategy_opts(opts, &buf); > strbuf_release(&buf); > > @@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const > char *head_name, > write_file(rebase_path_gpg_sign_opt(), "-S%s\n", > opts->gpg_sign); > if (opts->signoff) > write_file(reba
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Paul, On Wed, 28 Nov 2018, Paul Morelle wrote: > The 'exec' command can be used to run tests on a set of commits, > interrupting on failing commits to let the user fix the tests. > > However, the 'exec' line has been consumed, so it won't be ran again by > 'git rebase --continue' is ran, even if the tests weren't fixed. > > This commit introduces a new command 'test' equivalent to 'exec', except > that it is automatically rescheduled in the todo list if it fails. > > Signed-off-by: Paul Morelle Would it not make more sense to add a command-line option (and a config setting) to re-schedule failed `exec` commands? Like so: -- snip -- diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index a2ab68ed0632..a9ab009fdbca 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")), OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")), OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto), + OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec, +N_("automatically re-schedule any `exec` that fails")), OPT_END() }; diff --git a/builtin/rebase.c b/builtin/rebase.c index 5b3e5baec8a0..700cbc4e150e 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -104,6 +104,7 @@ struct rebase_options { int rebase_merges, rebase_cousins; char *strategy, *strategy_opts; struct strbuf git_format_patch_opt; + int reschedule_failed_exec; }; static int is_interactive(struct rebase_options *opts) @@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts) argv_array_push(&child.args, opts->gpg_sign_opt); if (opts->signoff) argv_array_push(&child.args, "--signoff"); + if (opts->reschedule_failed_exec) + argv_array_push(&child.args, "--reschedule-failed-exec"); status = run_command(&child); goto finished_rebase; @@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) "strategy")), OPT_BOOL(0, "root", &options.root, N_("rebase all reachable commits up to the root(s)")), + OPT_BOOL(0, "reschedule-failed-exec", +&options.reschedule_failed_exec, +N_("automatically re-schedule any `exec` that fails")), OPT_END(), }; int i; @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) break; } + if (options.reschedule_failed_exec && !is_interactive(&options)) + die(_("--reschedule-failed-exec requires an interactive rebase")); + if (options.git_am_opts.argc) { /* all am options except -q are compatible only with --am */ for (i = options.git_am_opts.argc - 1; i >= 0; i--) diff --git a/sequencer.c b/sequencer.c index e1a4dd15f1a8..69bee63e116f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy") static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts") static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate") static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet") +static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec") static int git_sequencer_config(const char *k, const char *v, void *cb) { @@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts) opts->signoff = 1; } + if (file_exists(rebase_path_reschedule_failed_exec())) + opts->reschedule_failed_exec = 1; + read_strategy_opts(opts, &buf); strbuf_release(&buf); @@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign); if (opts->signoff) write_file(rebase_path_signoff(), "--signoff\n"); + if (opts->reschedule_failed_exec) + write_file(rebase_path_reschedule_failed_exec(), "%s", ""); return 0; } @@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) *end_of_arg = saved; /* Reread the todo file if it has changed. */ - if (res) - ; /* fall through */ - else if (stat(get_todo_path(opts), &st)) +
[PATCH] rebase -i: introduce the 'test' command
The 'exec' command can be used to run tests on a set of commits, interrupting on failing commits to let the user fix the tests. However, the 'exec' line has been consumed, so it won't be ran again by 'git rebase --continue' is ran, even if the tests weren't fixed. This commit introduces a new command 'test' equivalent to 'exec', except that it is automatically rescheduled in the todo list if it fails. Signed-off-by: Paul Morelle --- Documentation/git-rebase.txt | 9 ++--- rebase-interactive.c | 1 + sequencer.c | 23 +++ t/lib-rebase.sh | 4 +++- t/t3404-rebase-interactive.sh | 16 5 files changed, 41 insertions(+), 12 deletions(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 80793bad8..c8f565637 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -693,8 +693,8 @@ $ git rebase -i -p --onto Q O Reordering and editing commits usually creates untested intermediate steps. You may want to check that your history editing did not break anything by running a test, or at least recompiling at intermediate -points in history by using the "exec" command (shortcut "x"). You may -do so by creating a todo list like this one: +points in history by using the "exec" command (shortcut "x") or the +"test" command. You may do so by creating a todo list like this one: --- pick deadbee Implement feature XXX @@ -702,7 +702,7 @@ fixup f1a5c00 Fix to feature XXX exec make pick c0ffeee The oneline of the next commit edit deadbab The oneline of the commit after -exec cd subdir; make test +test cd subdir; make test ... --- @@ -715,6 +715,9 @@ in `$SHELL`, or the default shell if `$SHELL` is not set), so you can use shell features (like "cd", ">", ";" ...). The command is run from the root of the working tree. +The "test" command does the same, but will remain in the todo list as +the next command, until it succeeds. + -- $ git rebase -i --exec "make test" -- diff --git a/rebase-interactive.c b/rebase-interactive.c index 78f3263fc..4a408661d 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -14,6 +14,7 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, "s, squash = use commit, but meld into previous commit\n" "f, fixup = like \"squash\", but discard this commit's log message\n" "x, exec = run command (the rest of the line) using shell\n" +" test = same as exec command, but keep it in TODO if it fails\n" "b, break = stop here (continue rebase later with 'git rebase --continue')\n" "d, drop = remove commit\n" "l, label = label current HEAD with a name\n" diff --git a/sequencer.c b/sequencer.c index e1a4dd15f..c3dde6910 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1508,6 +1508,7 @@ enum todo_command { TODO_SQUASH, /* commands that do something else than handling a single commit */ TODO_EXEC, + TODO_TEST, TODO_BREAK, TODO_LABEL, TODO_RESET, @@ -1530,6 +1531,7 @@ static struct { { 'f', "fixup" }, { 's', "squash" }, { 'x', "exec" }, + { 0, "test" }, { 'b', "break" }, { 'l', "label" }, { 't', "reset" }, @@ -2072,7 +2074,7 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) command_to_string(item->command)); if (item->command == TODO_EXEC || item->command == TODO_LABEL || - item->command == TODO_RESET) { + item->command == TODO_RESET || item->command == TODO_TEST) { item->commit = NULL; item->arg = bol; item->arg_len = (int)(eol - bol); @@ -3576,7 +3578,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) item->arg, item->arg_len, opts, res, to_amend); } - } else if (item->command == TODO_EXEC) { + } else if (item->command == TODO_EXEC || item->command == TODO_TEST) { char *end_of_arg = (char *)(item->arg + item->arg_len); int saved = *end_of_arg; struct stat st; @@ -3586,9 +3588,12 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) *end_of_arg = saved; /* Reread the todo file if it has changed. */ - if (res) + if (res) { ; /* fall through */ - else if (stat(get_todo_path(opts), &st)) + if (item->command == TODO_TEST) { + reschedule = 1; +