RFC patch, 5 partsrebase --continue: add --autostage to stage unstaged changes

From: Phillip Wood <phillip.wood@dunelm.org.uk>

These patches add an '--autostage' option (and corresponding config variable) to 'rebase --continue' that will stage any unstaged changes before continuing. This saves the user having to type 'git add' before running 'git rebase --continue'.

Phillip Wood (5):
  rebase --continue: add --autostage to stage unstaged changes
  rebase -i: improve --continue --autostage
  Unify rebase amend message when HEAD has changed
  Add tests for rebase --continue --autostage
  Add rebase.continue.autostage config setting

 git-rebase--am.sh             |   1 +
 git-rebase--interactive.sh    | 111 ++++++++++++++++++++++++++++++++----------
 git-rebase--merge.sh          |   1 +
 git-rebase.sh                 |  76 ++++++++++++++++++++++++++---
 sequencer.c                   |  22 +++++++--
 t/t3404-rebase-interactive.sh |   2 +-
 t/t3418-rebase-continue.sh    |  50 ++++++++++++++++++-
 7 files changed, 222 insertions(+), 41 deletions(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

After resolving conflicts during a rebase it is a pain to have to run 'git add' before 'git rebase --continue'. Passing --autostage to 'git rebase --continue' will stage them automatically so long as 'git diff --check' says they are ok. This is a safety measure to avoid accidentally staging files containing unresolved conflicts.

Continuing an interactive rebase after a failed exec or if HEAD has changed since rebase stopped with --autostage will stage changes that wont be committed as rebase --continue will bail out. This will be fixed in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

Using diff check is not ideal as it will error out on whitespace changes (which should be check in a commit hook if the user is worried about them) as well as merge markers. Looking through diff.c it should be possible to add a --check=merge-markers optional argument as the two checks are enabled independently in the code. As Junio pointed out in a previous message [1] the absence of conflict markers does not indicate that a file is fully resolved but checking for them does at least avoid the case of the user blindly continuing when they have forgotten to look at a conflicted file.

The autostaging behaviour is opt-in so users who like the additional safety of having to do git add before git rebase --continue can continue using their current workflow.

I wonder if check_unstaged() should give different error messages depending on the presence of unmerged paths rather than saying 'unstaged changes' all the time. Also it should probably have some message after the check for merge markers fails rather than just the raw output from diff --check.

[1] https://public-inbox.org/git/xmqqmv7td0a5.fsf@gitster.mtv.corp.google.com/

 git-rebase--am.sh          |  1 +
 git-rebase--interactive.sh |  1 +
 git-rebase--merge.sh       |  1 +
 git-rebase.sh              | 76 +++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 71 insertions(+), 8 deletions(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If rebase --interactive is unable to commit staged changes because HEAD has changed since rebase stopped the user gets different messages depending on whether they specified --autostage or not. Update the messages in the other code paths to match the --autostage one.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

The change from error() to fprintf() is to keep the messages consistent, maybe the messages in the shell script should be prefixed with 'error:' instead.

 git-rebase--interactive.sh | 10 ++++++----
 sequencer.c                | 22 +++++++++++++++++-----
 2 files changed, 23 insertions(+), 9 deletions(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Make sure that --autostage stages any changes and fails if there are merge markers in the file to be staged.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3418-rebase-continue.sh | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This enables the user to always specify --autostage with --continue

The tests check that setting rebase.continue.autostage=true results in 'git rebase --continue' autostaging unstaged changes and that '--no-autostage' overrides it.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-rebase.sh              |  2 +-
 t/t3418-rebase-continue.sh | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If HEAD has changed since the rebase stopped or rebase stopped due to a failed exec then 'git rebase --continue --autostage' will autostage changes that cannot be commited. Fix this by reordering some of the checks so that 'git rebase --continue --autostage' never stages changes unless they can be committed. Also reword some error messages to account of --autostage and try and make them clearer.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

This could do with some tests the check the correct error message is given in each failure case. However I'm expecting to change the error messages based on the feedback I get to this patch so I'll add the tests once the messages are finalized. I've tried to give a bit more explanation in the error messages as I found some of the previous messages didn't explain why rebase couldn't continue. Splitting the advice out means it is consistent between different code paths and should make it easier to optionally disable it in future if anyone wanted to add that. As for the formatting of the messages I wonder if they would be better without so many empty lines (I find the current messages a bit intimating as they seem so long). e.g.

If you wish to squash the unstaged changes into the last commit, run:
    git add -u
    git commit --amend \$gpg_sign_opt_quoted
If they are meant to go into a new commit, run:
    git add -u
    git commit \$gpg_sign_opt_quoted
In both cases, once you're done, continue with:
    git rebase --continue

Instead of

If you wish to squash the unstaged changes into the last commit, run:

  git add -u
  git commit --amend \$gpg_sign_opt_quoted

If they are meant to go into a new commit, run:

  git add -u
  git commit \$gpg_sign_opt_quoted

In both cases, once you're done, continue with:

  git rebase --continue

 git-rebase--interactive.sh    | 104 ++++++++++++++++++++++++++++++++----------
 t/t3404-rebase-interactive.sh |   2 +-
 2 files changed, 81 insertions(+), 25 deletions(-)

Phillip Wood <phillip.wood@talktalk.net> writes:

I wonder if this interacts with existing rerere.autoupdate configuration variable and if so how (i.e. would they conflict and fight with each other? would they work well together? would one of them make the other unnecessary?). In any case, they look closely related and perhaps should be named similarly.

I even have a suspicion that they may be essentially doing the same thing.

For a previous discussion, you start from here:

  https://public-inbox.org/git/7vej6xb4lr.fsf@gitster.siamese.dyndns.org/#t

and for the context, look at the original post by Ingo, to which the above message is a response to.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Hmph, this is interesting.

"git rebase" does take "--rerere-autoupdate" option from the command line, and propagates it to a later invocation of "rebase --continue" by storing the value to $state_dir/allow_rerere_autoupdate file and reading the value from it. $allow_rerere_autoupdate shell variable is used to hold the setting.

I'd expect that this variable to be used in invocations of "git am" in git-rebase--am.sh; but that does not seem to be the case. I wonder if this was once working but over time we broke the feature without anybody noticing it, or if the support was added but not completed and the feature was a no-op from the beginning?

Junio C Hamano <gitster@pobox.com> writes:

At least in v1.7.0 when doing "rebase -m", the rerere-autoupdate was plumbed correctly through to the invocation of "git merge" that is done inside git-rebase.sh. I do not see the same option passed down to the invocation of "git am", so perhaps nobody cared back then about rerere during "git rebase" that does not use "git am" backend, even though "git am" itself were capable of talking the option.

In any case, if you corrected the existing "git rebase" and its backend so that "--rerere-autoupdate" works as advertised, I think you are already 80% there without adding a yet another option, as I suspect that the most of the need for avoiding "git add" during a "git rebase" session is during a conflict resolution, and allowing "rerere" to automatically update the index with auto-resolution will leave _only_ changes to the paths the end user actually needs to take a look and manually fix still not in the index. And from the workflow point of view, encouraging them to "git add" their manual resolution after they are satisified with their changes by not doing "git add" blindly for all changes, like your --autostage" does, is probably a good thing.

On 26/07/17 23:12, Junio C Hamano wrote:

I've had a quick look and I think it's just a case of adding '$allow_rerere_autoupdate' to the invocation of 'git am'. 'git am' calls rerere_clear() when skipping so that doesn't need to go into the shell script. I'll come up with a test to check it works as I think it does.

I'm interested in the 20% as it's about 100% of my rebase conflicts. I've got rerere enabled but I cannot recall it helping me with a rebase conflict in the five or six years I've been using git [1]. A lot of my rebase conflicts come from fixup! commits or reordering things with rebase -i and generally they're only going to happen once as I don't apply the same fixup! more than once and if I've rearranged commits the conflicts are likely to be different in the (possibly unlikely) event I rearrange them again in the future.

Once I've resolved a conflict I will look at the diff and maybe compile and run some tests. Then, if I'm happy I'll run 'git rebase --continue', to me the 'git add' stage doesn't really add anything useful, it's just an inconvenience.

The other use I have for --autostage is when editing commits with rebase -i, once I'm happy with the edit I just want to continue and have rebase amend the commit. I don't feel having to run 'git add' is helpful in this case.

In my mind running 'git rebase --continue' is the signal to git that the conflicts are resolved.

Best Wishes

Phillip

[1] Maybe I've not noticed it helping when rebasing onto an updated upstream, although if I'm actively changing the rebased commits between upstream updates the conflicts may well change between updates. Also I don't tend to get many upstream conflicts with the projects I'm working on.

On 27/07/17 11:36, Phillip Wood wrote:

Git allows 'git commit -a' to complete a conflicted merge which I think is much the same thing as I'm proposing. Also there's nothing to stop a user accidentally running 'git add' on a path that isn't resolved or just blindly running 'add -u' before continuing the rebase because they think they've resolved everything. I guess it is a kind of convenience/safety trade-off.

Thinking about it some more, once a conflict is resolved, the next time that rewritten commit is rebased onto an updated upstream the same conflict will not occur as the rewritten commit that's being replayed does not conflict with the previous upstream - the resolution is baked into the commit that's being replayed.

Phillip Wood <phillip.wood@talktalk.net> writes:

OK, then at least a fixed --rerere-autoupdate would hopefully limit the scope of the additional 20%; I'd suspect that a new option would also internally turn on --rerere-autoupdate, so that the remaining changes you would see upon --continue would be limited to what the user had to manually resolve (and edit without having textual conflict, aka evil merge to resolve semantic conflicts).

I am *not* opposed to an option to tell the command to blindly take such remaining changes, as long as it stays optional---the use of the option can then be taken as a strong signal that the user is OK with the local changes in the working tree, even if the user may not have marked them as resolved with "git add".

"-a" is a strong enough sign that the user is OK with all the paths; "git commit" without an option does not. So it is OK for a new option (perhaps "--all-autoupdate", which does more than the existing "--rerere-autoupdate") to become the signal that the user is OK with all the local changes.

This is a tangent, but concluding a merge with "commit -a" (or "add -u && commit -a") has always been discouraged among Git expert users, and it will stay to be so. If you search the list archive, you would find a good explanation by Linus on this, but a short version is that this is because it is normal to start a merge in a dirty working tree where the user has local changes that the user knows will not interfere with the merge.

Because "rebase" refuses to work in a dirty working tree, the analogy with "merge" does not quite hold. Doing "add -u" before telling it to "--continue" would be much safer.

Thanks.

On 27/07/17 16:24, Junio C Hamano wrote:

I can see the logic in that but I was imagining (and the patches implement) that the --autostage option would be passed to 'rebase --continue' not when the rebase was started by which time it is too late to turn on --rerere-autoupdate for the current conflicts. I prefer having to pass --autostage with --continue so that it is a concious decision by the user to stage unstaged changes when they continue rather than rebase just doing it each time it continues.

I agree it definitely needs to be optional (possibly with a config option to have it on by default but I'm not wedded to that as it is easy to set up a git or shell alias)

What is the best way forward on this? The patches I posted add a '--autostage' option to be passed with '--continue' which means that staging all the unstaged changes remains optional but does not allow --rerere-autoupdate to be automatically enabled. I'm not sure about the check for merge markers, as it is implemented it also checks for whitespace errors which is really the domain of the pre-commit hook. If we go for an explicit --autostage without the config key to make it on by default then maybe it is OK to drop the check, but it keeping it could be a useful safety measure. I don't think it would be too much work the change diff to allow '--check=merge-markers' as internally the whitespace and marker checks are implemented separately.

Best Wishes

Phillip

Phillip Wood <phillip.wood@talktalk.net> writes:

In other words, instead of

	git add -u && git rebase --continue

you would want a quicker way to say

	git rebase --continue $something_here 

If that is the case, that is understandable to me. Is the "-u" (I think "git add -u" is short for "--update" but I didn't check) taken as a valid option to "git rebase"? If not, that $something_here could be "-u".

Thanks for pinging the thread; otherwise I would have forgotten, especially because not many other people were involved in the discussion to begin with.

On 21/08/17 23:41, Junio C Hamano wrote:

Exactly

At the moment $something_else is -a/--autostage but -u/--update (I've checked the add man page and you're right) could be good as well.

rebase --continue -a

behaves like commit -a in that it commits all updated tracked files and does not take pathspecs, if we go for -u then there is a difference with 'git add -u' as that can take an optional pathspec.

Did you have any further thoughts on what checks if any this new option should make to avoid staging obviously unresolved files?

Yes it would be interesting to hear if this would be useful for others too.

Best Wishes

Phillip

Phillip Wood <phillip.wood@talktalk.net> writes:

Hmph, but what 'a' in "commit -a" wants to convey is "all", and in the context of "rebase" we want to avoid saying "all". A user can be confused into thinking "all" refers to "all subsequent commits" not "all paths conflicted".

Besides, with the $something_here option, the user is explicitly telling us that the user finisished the resolution of conflicts left in the working tree. There is nothing "auto" about it.

The more I think about this, the more I am convinced that it is a very bad idea to give such a $something_here option only to "rebase".

The standard flow for any operation that could stop in the middle because the command needs help from the user with conflicts that cannot be mechanically resolved is

 (1) write out conflicted state in the working tree, and mark these
     paths in the index by leaving them in higher stages
     (i.e. "ls-files -u" would list them);

 (2) the user edits them and marks the ones that are now resolved;

 (3) the user tells us to "continue".

and this is not limited to "rebase". "cherry-pick", "am", and "revert" all share the above, and even "merge" (which is a single commit operation to which "continue" in its original meaning---to tell us the user is done with this step and wants us to go on processing further commits or patches---does not quite apply) does.

And "rebase" is an oddball in that it is the only command that we could deduce which files in the working tree should be "add"ed, i.e. "all the files that are different from HEAD". All others allow users to begin in a dirty working tree (they only require the index to be clean), so your

	git [cherry-pick|am|revert] --continue $something_here

cannot be "git add -u && git [that command] --continue". Blindly taking any and all files that have modifications from HEAD will produce a wrong result.

You cannot even sanely solve that by first recording the paths that are conflicted before giving control back to the user and limit the "add" to them, i.e. doing "git add" only for paths that appear in "git ls-files -u" output would not catch additional changes the was needed in files that did not initially conflict (i.e. "evil merge" will have to modify a file that was not involved in the mechanical part of the conflict), because out of the files that are dirty in the working tree, some are evil merge resolutions and others are ones that were dirty from the beginning.

The only reason "git rebase" can get away without having to worry about all of the above is because it insists that the working tree is clean before it begins. In the ideal world, however, we would want to lift that and allow it to start in a working tree that has an unfinished local modification that does not interfere with the rebase, just like all other operations that could stop upon a conflict. And when that happens, your $something_here option to "git rebase --continue" will have to worry about it, too.

So...

On 22/08/17 16:54, Junio C Hamano wrote:

Are you sure about that, the second sentence of the cherry-pick man page is "This requires your working tree to be clean (no modifications from the HEAD commit).". If you pass '--no-commit' then this is relaxed so that the index can differ from HEAD but it is not clear from the man page if the working tree can differ from the index (potentially that could give different conflicts in the index and working tree). I'm not sure what a rebase that does not commit changes would look like.

I wonder if git could record the state of the working tree in a temporary index just before it exits with conflicts. Then when the user has resolved the conflicts

git [cherry-pick|am|revert] --continue $something_here

could do something like

{ GIT_INDEX_FILE=$TMP_INDEX git diff-files --name-only &&
    git ls-files -u } | git update-index --remove --stdin

to stage the files that the user has edited. This would not catch the case where a new untracked file has been created that should be added to the conflict resolution, but I think it is reasonable to expect the user to manually add new files in this case. It would also not add an unchanged file whose changes should be part of the conflict resolution but I can't think of a sensible case where the user might want that at the moment.

It could be expensive to check that the local modifications will not interfere with the rebase - wouldn't it have to look at all the files touched by each commit before starting? What do cherry-pick and am do here when picking several commits?

 And when that happens, your $something_here option to

Phillip Wood <phillip.wood@talktalk.net> writes:

Yes, so?

"I do not think of a way to implement it cheaply, so we forbid anybody from finding a clever optimization to implement it cheaply by adding a feature that will not work well with it when it happens?"

On 24/08/17 17:46, Junio C Hamano wrote:

Ouch, I think that is a rather unfair characterization of what I said and certainly does not reflect what I was trying to say. I was asking since cherry-pick and am can pick/apply multiple commits/patches and apparently allow the user to start with a dirty tree what do they do about checking that the applied commits/patches do not interfere with any local changes. As for "adding a feature that will not work well with it when it happens?" I suggested a possible solution in the message.

Anyway you're not convinced this feature is a good idea and no one else has commented so I'll drop it.

Phil