On Wed, Apr 12, 2017 at 3:05 PM, Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote: > > Hi Stephen, > > On Tue, 11 Apr 2017, Stephen Hicks wrote: > > > Thanks for the tips. I think I have an approach that works, by simply > > returning sequencer_continue() immediately after a successful exec. > > I am not sure that that works really as expected, as you re-enter the > sequencer_continue() and neither the original author nor I expected nested > calls. Alright, I can reproduce the longer version in place here. > > I'm hesitant to only use mtime, size, and inode, since it's quite likely > > that these are all identical even if the file has changed. > > Not at all. The mtime and the size will most likely be different. In my experience, mtime is almost always the same, since the file is written pretty much immediately before the exec - as long as the command takes a small fraction of a second (which it usually does) the mtime (which is in seconds, not millis or micros) will look the same. For shell redirects, the inode stays the same, though the size will usually be different, but this is not guaranteed. > I am reluctant to take your wholesale approach, as I perform literally > dozens of rebases with >100 commits, including plenty of exec calls, and I > want the rebase to become faster instead of slower. I'm in favor of that, but not sure of a reliable way to tell whether the file is modified. As mentioned above, mtime, size, and inode only get us *most* of the way there. This leaves it open to very surprising issues when suddenly one edit out of a thousand accidentally doesn't change any of these things and so doesn't get picked up. This would be a very difficult to debug issue as well, since everything would look very reasonable from the outside. Potentially it could be mitigated by actually writing a message when the file is reloaded, making it more obvious when that doesn't occur (though it's still not the most ergonomic situation). > > Say, the command is simply a `sed -i 's/^exec /#### /g'`, then the > > timestamp (in seconds) will almost definitely be the same, and the size > > and inode will be the same as well. > > Try it. The inode is different. You are correct that sed -i does change the inode. `c=$(sed 's/^exec /#### /g' git-rebase-todo); echo "$c" > git-rebase-todo` is a better example, albeit a bit more contrived. I've added a failing test case to demonstrate the problem. > > Granted this is a contrived example, but it would be unfortunate if > > accidentally keeping the size the same were to cause the change to not > > be picked up. > > > > Another option would be to hash the contents, but at that point, I'm not > > sure it's any better than simply unconditionally re-parsing the TODO. > > Again, my intent is to make rebase faster, not slower. Hashing the > contents would make it slower. So would re-reading it. > > > https://github.com/git/git/pull/343 > > Thank you for starting to work on this. I left a couple of comments. > Please do not be offended by their terseness, I really wanted to point out > a couple of things I think we can improve together, but I am also way past > my bedtime. > > Ciao, > Johannes