{"thread":{"id":"45264","subject":"Re: Rebase sequencer changes prevent exec commands from modifying the todo file?","startedAt":"2017-03-02T16:02:20Z","lastAt":"2017-04-13T13:52:27Z","messageCount":4,"participants":["Johannes Schindelin","Stephen Hicks"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"313077","messageId":"alpine.DEB.2.20.1703021617510.3767@virtualbox","threadId":"45264","inReplyTo":"CAKNkOnM366uiJKkz31hS8V3NTa8qksP2pXrH4+F-zodZaNdsqg@mail.gmail.com","subject":"Re: Rebase sequencer changes prevent exec commands from modifying the todo file?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-02T15:25:25Z","receivedAt":"2017-03-02T16:02:20Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stephen,\n\nOn Wed, 1 Mar 2017, Stephen Hicks wrote:\n\n> I have a preferred rebase workflow wherein I generate my own TODO file\n> with 'exec' lines that further modify the TODO file.  I recently noticed\n> that these changes weren't sticking: they seemed to persist until the\n> end of the exec'd command, and then as soon as the next command ran\n> (i.e.  GIT_EDITOR=cat git rebase --edit-todo) they were gone.\n> \n> I don't understand the changes that have been going through, but I suspect\n> this is a result of the sequencer refactoring.  Is there a way to prevent\n> git rebase from overwriting these changes?\n> \n> To reproduce:\n> $ git rebase -i HEAD -x \"echo x false >> \\\"$(git rev-parse\n> --git-dir)/rebase-merge/git-rebase-todo\\\"\"\n> \n> This should cause the rebase to fail (and indeed, it does on 2.11.0.390)\n> since it inserts an \"x false\".  But somewhere between there and\n> 2.12.0.rc1.440, this behavior is changed.\n\nDo you also modify the author-script file to execute arbitrary shell\ncommands? ;-)\n\nSeriously again, it should not be too much of a deal to handle your use\ncase by re-reading the git-rebase-todo file if it has changed after an\n`exec` has modified it. We already force a re-read of the index.\n\nI won't be able to take care of that immediately, though, as I have a\npressing other patch series I need to get done.\n\nIf you want to take a crack at it in the meantime, I think this is where I\nwould start:\n\nhttps://github.com/git-for-windows/git/blob/ce4c6ca554/sequencer.c#L2020-L2027\n\nI would probably try to make the code smart by looking at the\ntimestamp/size/inode fields of the stat data of git-rebase-todo, and only\nforce a re-read in case those fields are different, the `res` variable is\n0, and then the code would need to `continue;` in order to skip the\nincrement of `todo_list->current`.\n\nAnd before that, I'd turn your example into a test case in\nt/t3405-rebase-malformed.sh...\n\nCiao,\nJohannes\n"},{"id":"316705","messageId":"alpine.DEB.2.20.1704122355440.2135@virtualbox","threadId":"45264","inReplyTo":"CAKNkOnOkSgFei7jpck8Z7tH+jYn_MXvarA86GAadT8jJt4aO-g@mail.gmail.com","subject":"Re: Rebase sequencer changes prevent exec commands from modifying the todo file?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-12T22:05:08Z","receivedAt":"2017-04-12T22:05:16Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stephen,\n\nOn Tue, 11 Apr 2017, Stephen Hicks wrote:\n\n> Thanks for the tips.  I think I have an approach that works, by simply\n> returning sequencer_continue() immediately after a successful exec.\n\nI am not sure that that works really as expected, as you re-enter the\nsequencer_continue() and neither the original author nor I expected nested\ncalls.\n\n> I'm hesitant to only use mtime, size, and inode, since it's quite likely\n> that these are all identical even if the file has changed.\n\nNot at all. The mtime and the size will most likely be different.\n\nI am reluctant to take your wholesale approach, as I perform literally\ndozens of rebases with >100 commits, including plenty of exec calls, and I\nwant the rebase to become faster instead of slower.\n\n> Say, the command is simply a `sed -i 's/^exec /#### /g'`, then the\n> timestamp (in seconds) will almost definitely be the same, and the size\n> and inode will be the same as well.\n\nTry it. The inode is different.\n\n> Granted this is a contrived example, but it would be unfortunate if\n> accidentally keeping the size the same were to cause the change to not\n> be picked up.\n> \n> Another option would be to hash the contents, but at that point, I'm not\n> sure it's any better than simply unconditionally re-parsing the TODO.\n\nAgain, my intent is to make rebase faster, not slower. Hashing the\ncontents would make it slower. So would re-reading it.\n\n> https://github.com/git/git/pull/343\n\nThank you for starting to work on this. I left a couple of comments.\nPlease do not be offended by their terseness, I really wanted to point out\na couple of things I think we can improve together, but I am also way past\nmy bedtime.\n\nCiao,\nJohannes\n"},{"id":"316708","messageId":"CAKNkOnNMH72QnuDrja3XNG8Z6zLsuXdCERA==iBQztRQW1+O3Q@mail.gmail.com","threadId":"45264","inReplyTo":"alpine.DEB.2.20.1704122355440.2135@virtualbox","subject":"Re: Rebase sequencer changes prevent exec commands from modifying the todo file?","fromName":"Stephen Hicks","fromEmail":"stephenhicks@gmail.com","sentAt":"2017-04-12T23:26:30Z","receivedAt":"2017-04-12T23:26:57Z","isPatch":false,"sender":{"key":"stephenhicks@gmail.com","avatar":null},"body":"On Wed, Apr 12, 2017 at 3:05 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Stephen,\n>\n> On Tue, 11 Apr 2017, Stephen Hicks wrote:\n>\n> > Thanks for the tips.  I think I have an approach that works, by simply\n> > returning sequencer_continue() immediately after a successful exec.\n>\n> I am not sure that that works really as expected, as you re-enter the\n> sequencer_continue() and neither the original author nor I expected nested\n> calls.\n\nAlright, I can reproduce the longer version in place here.\n\n> > I'm hesitant to only use mtime, size, and inode, since it's quite likely\n> > that these are all identical even if the file has changed.\n>\n> Not at all. The mtime and the size will most likely be different.\n\nIn my experience, mtime is almost always the same, since the file is\nwritten pretty much immediately before the exec - as long as the\ncommand takes a small fraction of a second (which it usually does) the\nmtime (which is in seconds, not millis or micros) will look the same.\nFor shell redirects, the inode stays the same, though the size will\nusually be different, but this is not guaranteed.\n\n> I am reluctant to take your wholesale approach, as I perform literally\n> dozens of rebases with >100 commits, including plenty of exec calls, and I\n> want the rebase to become faster instead of slower.\n\nI'm in favor of that, but not sure of a reliable way to tell whether\nthe file is modified.  As mentioned above, mtime, size, and inode only\nget us *most* of the way there.  This leaves it open to very\nsurprising issues when suddenly one edit out of a thousand\naccidentally doesn't change any of these things and so doesn't get\npicked up.  This would be a very difficult to debug issue as well,\nsince everything would look very reasonable from the outside.\nPotentially it could be mitigated by actually writing a message when\nthe file is reloaded, making it more obvious when that doesn't occur\n(though it's still not the most ergonomic situation).\n\n> > Say, the command is simply a `sed -i 's/^exec /#### /g'`, then the\n> > timestamp (in seconds) will almost definitely be the same, and the size\n> > and inode will be the same as well.\n>\n> Try it. The inode is different.\n\nYou are correct that sed -i does change the inode.  `c=$(sed 's/^exec\n/#### /g' git-rebase-todo); echo \"$c\" > git-rebase-todo` is a better\nexample, albeit a bit more contrived.  I've added a failing test case\nto demonstrate the problem.\n\n> > Granted this is a contrived example, but it would be unfortunate if\n> > accidentally keeping the size the same were to cause the change to not\n> > be picked up.\n> >\n> > Another option would be to hash the contents, but at that point, I'm not\n> > sure it's any better than simply unconditionally re-parsing the TODO.\n>\n> Again, my intent is to make rebase faster, not slower. Hashing the\n> contents would make it slower. So would re-reading it.\n>\n> > https://github.com/git/git/pull/343\n>\n> Thank you for starting to work on this. I left a couple of comments.\n> Please do not be offended by their terseness, I really wanted to point out\n> a couple of things I think we can improve together, but I am also way past\n> my bedtime.\n>\n> Ciao,\n> Johannes\n"},{"id":"316722","messageId":"alpine.DEB.2.20.1704131526500.2135@virtualbox","threadId":"45264","inReplyTo":"CAKNkOnNMH72QnuDrja3XNG8Z6zLsuXdCERA==iBQztRQW1+O3Q@mail.gmail.com","subject":"Re: Rebase sequencer changes prevent exec commands from modifying the todo file?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-13T13:52:08Z","receivedAt":"2017-04-13T13:52:27Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stephen,\n\nOn Wed, 12 Apr 2017, Stephen Hicks wrote:\n\n> On Wed, Apr 12, 2017 at 3:05 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> >\n> > On Tue, 11 Apr 2017, Stephen Hicks wrote:\n> >\n> > > I'm hesitant to only use mtime, size, and inode, since it's quite\n> > > likely that these are all identical even if the file has changed.\n> >\n> > Not at all. The mtime and the size will most likely be different.\n> \n> In my experience, mtime is almost always the same, since the file is\n> written pretty much immediately before the exec - as long as the\n> command takes a small fraction of a second (which it usually does) the\n> mtime (which is in seconds, not millis or micros) will look the same.\n\nOh, I see, you assume that mtime is in seconds. However, on pretty much\nall platforms supported by Git we use nanosecond-precision mtimes [*1*].\nIn practice, the mtimes are not always nanosecond-precision, as some file\nsystems have a coarser granularity. It is typically a pretty fine a\ngranularity, though, much finer than a second [*2*].\n\nMy mistake, I should have explained that I wanted to suggest adding a\nfield of type `struct cache_time` to the `todo_list` structure, and to use\nST_MTIME_NSEC(&st) to populate and compare the `nsec` part of that.\n\nCiao,\nJohannes\n\nFootnote *1*: The platforms for which we disable nanosecond mtimes are:\nOSF-1, AIX, HP-UX, Interix, Minix, NONSTOP_KERNEL (?), and QNX. (If you\nlook at git.git's config.mak.uname, you will see that Git for Windows'\nnanosecond support has not yet made it upstream.) In other words, the\nmajor platforms (Windows, MacOSX and Linux) all compare the nanosecond\npart of the mtime.\n\nFootnote *2*: the coarsest mtime of which I know is FAT32 (2-second\ngranularity). If you want to use Git on such a file system, well, that's\nwhat you get. And then some performance penalties, too. It would appear\nfrom a quick web search that ext4 has true nanosecond granularitry. NTFS\nhas a 100ns granularity which is still plenty enough for our purposes.\n"}]}