Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:
> Minor changes since v2 in response to reviews by Junio and Eric
> Sunshine.
>
> Should be ready to merge soon.
That, and the "completed" on the Subject line are not for you to
decide, I would have to say ;-)
Overall the patches look cleanly done, but I did not carefully look
into the implementation. Especially I did not check if there are
still undesirable data loss behaviour in corner cases that people
were worried about in the discussion. It would have been nice to CC
people who voiced concerns during the previous reviews.
A few comments on possible follow-up work, not meant as a suggestion
to include in this series:
* The series shows that it is necessary to first prepare a stash
entry with "stash create", then later decide to queue it to the
stash (or decide not to do so) in some applications.
In the longer term, it is unmaintainable to make such users (like
this new code) of the stash mechanism manually do so, and we may
want to add a "git stash __store" subcommand, not meant for the
interactive use of end users. The implementation of the stash
can later be changed without affecting such users by doing so.
* The primary reason why you wanted this autostash was because
"rebase" is implemented in a lazy and stupid way, compared to
"merge" that detects possible conflicts with local changes and
refrains from touching the working tree at all, and any local
change that do not interfere with the merge are permitted.
Perhaps "rebase" can be taught to be more careful when checking
if local changes may overlap with the changes being replayed.
When replaying a range A..B on top of the onto commit O, perhaps
"rebase" can at least do these:
- If the index is dirty with respect to HEAD, stop just like
"merge" does.
- Take a union of paths different between O and the working
tree, and untracked new paths in the working tree. These are
the possible "local changes". If there is no way the replay
of A..B touches any of these paths, we do not even have to
worry about stashing.
- Take output from "diff-tree -m --name-only" for each commit in
the replayed range (note that I am not adding -M to take
maximal coverage). Check if any of the paths overlap with the
"local changes", and if so, stop.
to be par with "merge".
When the latter is done, there is _no_ justification for autostash
to be an option specific fo "rebase. At that point, "rebase" and
"merge" would refuse to start in the same situation, i.e. the local
changes you have are known to interfere with the integration with
the branch, diminishing the need to "autostash" greatly.
When autostash is still needed to proceed after such change to
"rebase", I suspect that the user may be better off saving such
work-in-progress in a temporary commit (possibly after making it
into a better shape before even starting to "rebase" or "merge")
than using a transient mechanism like stash to save such a work,
because it is likely that such local changes have a real conflict
with the work being integrated.
Those who still want to use stash would benefit from having an
autostash, but at that point, there is no reason to single out
"rebase" for the autostash feature. Those who want to stash
immediately before a "merge" that is known to conflict can use the
same autostash and may want to do so for exactly the same reason
they may want to use it for "rebase".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html