On Mon, Dec 07, 2015 at 11:24:52AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> writes: > > > On Wed, Dec 02, 2015 at 05:31:14PM -0500, Jeff King wrote: > >> On Wed, Dec 02, 2015 at 02:11:32PM -0800, Junio C Hamano wrote: > > [snip] > >> > "--keep-empty" has always been about keeping an originally empty > >> > commit, not a commit that becomes empty because of rebasing > >> > (i.e. what has already been applied to the updated base). The > >> > documentation, if it leads to any other interpretation, needs to be > >> > fixed. > >> > > >> > Besides, if "--keep-empty" were to mean "keep redundant ones that > >> > are already in the updated base", the patch must do a lot more, > >> > e.g. stop filtering with git-cherry patch equivalence. > >> > > >> > I'm inclined to eject this topic. > >> > >> That was my thinking too (and I notice it didn't get any review from > >> anybody else). > > [snip] > > > > Well, I kind of agree. The cherry-pick command has both > > --allow-empty and --keep-redundant flags, where the second one is > > the kind of behavior I want to achieve in my case. As an > > alternative to the proposed change to `--keep-empty` I could > > instead introduce a new flag `--keep-redundant-commits` to `git > > rebase` which would then pass the flag through to the > > cherry-pick. > > > > Any opinions on this? > > Honestly, I am not interested if that is only about passing that > option and doing nothing else. > > Imagine that you have two changes from the branch being rebased > already in the updated base, one was accepted verbatim, while the > other one was accepted with a slight tweak. Perhaps there were some > conflicts in the context, or something. > > When you run your rebase, the former will not even be considered > because command will notice, via patch equivalence, that you do not > need it, even before it attempts to replay it. But not the latter. > The command will attempt to replay it, and only in this step it may > notice, by seeing that the result becomes a no-op change, that the > change has already been included in the updated base. > > I cannot quite see how adding "--keep-redundant-commits" to the > command would help anybody by allowing the latter in the history but > not the former. That is primarily because you can imagine a future > in which the patch equivalence determination becomes smarter. With > or without "--keep-redundant-commits", both of these two changes > would not appear in the resulting history when that happens. > > The "--keep-redundant" option to "cherry-pick" makes sense, as the > user will be the one who is deciding which commit to replay on top > of a new base. If the user fed a commit that is already in the new > base, and the command is run with the option, that is a deliberate > request to leave a no-op cruft. > > We cannot say the same thing for "rebase", as it tries to avoid > redundant cruft, and it cannot do as good a job as humans in doing > so. The "--keep-redundant-commits" option will become a way to make > a distinction between what got dropped by the command automatically > and what didn't get dropped because the command did not look deeply > enough. That distinction is not at all useful, as that can change > as the tool improves. > > A "--keep-redundant-commits" to "rebase" that also disables the > patch equivalence filtering (not just keeping empty crufts in the > resulting history) might make sense, but I do not see myself (or > anybody sane) using it. "In what situation would such a feature be > useful?" is a question I cannot answer offhand. The scenario I require this feature for is somewhat more exotic, yes. We've got a CI where published branches can be rebased but should not be modified in their commit history. That is, the CI determines in a hook wether the branch that is being pushed drops or re-orders commits and if so, rejects the branch. So when a commit that is already included in origin/master gets rebased `git-rebase` currently simply drops the commit, which causes the CI to refuse the branch on a push. So obviously you are right in your conclusion that `--keep-redundant-commits` has to skip the patch equivalence determination in order to not drop any commits. Otherwise my change would only work for certain scenarios. That said, I do not care too deeply about this patch, especially as my scenario is rather uncommon. If you deem this to not have any greater benefit you may simply drop it. Patrick
Attachment:
signature.asc
Description: Digital signature