Re: Commit dropped when swapping commits with rebase -i -p
On 16/09/17 14:45, Sebastian Schuberth wrote: On Sat, Sep 16, 2017 at 12:41 PM, Andreas Heiduk wrote: Therefore I would avoid "definitive wording" like "will drop" and use vague wording along "there are various dragons out there" like this: The todo list presented by `--preserve-merges --interactive` does not represent the topology of the revision graph. Editing I tried to avoid this introducing sentence from the original wording as it reads like from a scientific research paper instead of from a user's manual. commits and rewording their commit messages should work fine. But reordering, combining or dropping commits of a complex topology There is no need for complex topology. If you reorder the two most recent commits in a linear history, one gets dropped. can produce unexpected and useless results like missing commits, wrong merges, merges combining two unrelated histories and similar things. "can produce" is much too soft, IMO. Reordering commits goes wrong, period. Like wise "unexpected and useless results" is inappropriate. The results are wrong in case of reordering, and wrong results are of course unexpected and useless. I agree that the wording needs to be explicit that bad things will happen. It should spell out that if commits or reordered, or the fixup or squash commands are used then commits will be dropped and if commits are deleted from the list or the drop command is used other commits other than the intended ones will be dropped as well.
Re: Commit dropped when swapping commits with rebase -i -p
On Sat, Sep 16, 2017 at 12:41 PM, Andreas Heiduk wrote: > Therefore I would avoid "definitive wording" like "will drop" and use > vague wording along "there are various dragons out there" like this: > > The todo list presented by `--preserve-merges --interactive` does > not represent the topology of the revision graph. Editing I tried to avoid this introducing sentence from the original wording as it reads like from a scientific research paper instead of from a user's manual. > commits and rewording their commit messages should work fine. > But reordering, combining or dropping commits of a complex topology There is no need for complex topology. If you reorder the two most recent commits in a linear history, one gets dropped. > can produce unexpected and useless results like missing commits, > wrong merges, merges combining two unrelated histories and > similar things. "can produce" is much too soft, IMO. Reordering commits goes wrong, period. Like wise "unexpected and useless results" is inappropriate. The results are wrong in case of reordering, and wrong results are of course unexpected and useless. -- Sebastian Schuberth
Re: Commit dropped when swapping commits with rebase -i -p
Am 15.09.2017 um 22:52 schrieb Junio C Hamano: > Sebastian Schuberth writes: >> >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt >> index 6805a74aec..ccd0a04d54 100644 >> --- a/Documentation/git-rebase.txt >> +++ b/Documentation/git-rebase.txt >> @@ -782,10 +782,11 @@ case" recovery too! >> >> BUGS >> >> -The todo list presented by `--preserve-merges --interactive` does not >> -represent the topology of the revision graph. Editing commits and >> -rewording their commit messages should work fine, but attempts to >> -reorder commits tend to produce counterintuitive results. >> +Be careful when combining the `-i` / `--interactive` and `-p` / "Be careful" is not necessary because the text is already in the "BUGS" section. >> +`--preserve-merges` options. Reordering commits will drop commits from the >> +main line. This is because the todo list does not represent the topology of >> the >> +revision graph in this case. However, editing commits and rewording their >> +commit messages 'should' work fine. >> >> For example, an attempt to rearrange >> > > > Anybody? I personally feel that the updated text is not all that > stronger but it is clearer by clarifying what "counterintuitive > results" actually mean, but I am not the target audience this > paragraph is trying to help, nor I am the one who is making excuse > for a known bug, so... > For me the proposed wording implies that the only bad effect are dropped commits on the mainline. But I experienced something like this: O--O--O--O---M--O==> O--O--O--O---M--O \ / \ / O--X--O--O O--X O Where X was a commit without a ref and hence lost. Also the merge commit seemed to combine two unrelated histories. Therefore I would avoid "definitive wording" like "will drop" and use vague wording along "there are various dragons out there" like this: The todo list presented by `--preserve-merges --interactive` does not represent the topology of the revision graph. Editing commits and rewording their commit messages should work fine. But reordering, combining or dropping commits of a complex topology can produce unexpected and useless results like missing commits, wrong merges, merges combining two unrelated histories and similar things.
Re: Commit dropped when swapping commits with rebase -i -p
Sebastian Schuberth writes: > On 2017-09-02 02:04, Jonathan Nieder wrote: > >>> Anyway, this should really more explicitly say *what* you need to know >>> about, that is, reordering commits does not work. >> >> It tries to explain that, even with an example. If you have ideas for >> improving the wording, that would be welcome. > > As a first step, I indeed believe the wording must the stronger / clearer. > How about this: > > From f69854ce7b9359603581317d152421ff6d89f345 Mon Sep 17 00:00:00 2001 > From: Sebastian Schuberth > Date: Mon, 11 Sep 2017 10:41:27 +0200 > Subject: [PATCH] docs: use a stronger wording when describing bugs with > rebase -i -p > > Signed-off-by: Sebastian Schuberth > --- > Documentation/git-rebase.txt | 9 + > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 6805a74aec..ccd0a04d54 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -782,10 +782,11 @@ case" recovery too! > > BUGS > > -The todo list presented by `--preserve-merges --interactive` does not > -represent the topology of the revision graph. Editing commits and > -rewording their commit messages should work fine, but attempts to > -reorder commits tend to produce counterintuitive results. > +Be careful when combining the `-i` / `--interactive` and `-p` / > +`--preserve-merges` options. Reordering commits will drop commits from the > +main line. This is because the todo list does not represent the topology of > the > +revision graph in this case. However, editing commits and rewording their > +commit messages 'should' work fine. > > For example, an attempt to rearrange > Anybody? I personally feel that the updated text is not all that stronger but it is clearer by clarifying what "counterintuitive results" actually mean, but I am not the target audience this paragraph is trying to help, nor I am the one who is making excuse for a known bug, so...
Re: Commit dropped when swapping commits with rebase -i -p
On 2017-09-02 02:04, Jonathan Nieder wrote: >> Anyway, this should really more explicitly say *what* you need to know >> about, that is, reordering commits does not work. > > It tries to explain that, even with an example. If you have ideas for > improving the wording, that would be welcome. As a first step, I indeed believe the wording must the stronger / clearer. How about this: >From f69854ce7b9359603581317d152421ff6d89f345 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Mon, 11 Sep 2017 10:41:27 +0200 Subject: [PATCH] docs: use a stronger wording when describing bugs with rebase -i -p Signed-off-by: Sebastian Schuberth --- Documentation/git-rebase.txt | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 6805a74aec..ccd0a04d54 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -782,10 +782,11 @@ case" recovery too! BUGS -The todo list presented by `--preserve-merges --interactive` does not -represent the topology of the revision graph. Editing commits and -rewording their commit messages should work fine, but attempts to -reorder commits tend to produce counterintuitive results. +Be careful when combining the `-i` / `--interactive` and `-p` / +`--preserve-merges` options. Reordering commits will drop commits from the +main line. This is because the todo list does not represent the topology of the +revision graph in this case. However, editing commits and rewording their +commit messages 'should' work fine. For example, an attempt to rearrange -- 2.14.1.windows.1
Re: Commit dropped when swapping commits with rebase -i -p
Hi, Sebastian Schuberth wrote: > On Wed, Aug 30, 2017 at 8:07 PM, Martin Ågren wrote: >> The man-page for git rebase says that combining -p with -i is "generally >> not a good idea unless you know what you are doing (see BUGS below)". > > Thanks for pointing this out again. I remember to have read this some > time ago, but as I general consider myself to know what I'm doing, I > forgot about it :-) Heh. > Anyway, this should really more explicitly say *what* you need to know > about, that is, reordering commits does not work. It tries to explain that, even with an example. If you have ideas for improving the wording, that would be welcome. That said, ... >> So if you agree that a "dropped commit" is a "counterintuitive result", >> this is known and documented. Maybe the warning could be harsher, but it >> does say "unless you know what you are doing". > > I'd say it's worse than counterintuitive, as counterintuitive might > still be correct, while in my case it clearly is not. So yes, the > warning must be harsher in my opinion. Maybe we should even abort > rebase -i-p if reordering of commits is detected. This sounds like a more promising approach. If you can detect when the rebase -i -p is going to cause trouble, then I would be all for aborting. If you want to be extra nice to people, you can provide a --force escape valve to let them experience the broken behavior, but I don't think that is necessary. I also think a loud warning when -i -p is used even when it is not going to cause trouble would be a valuable change. E.g. maybe the template that opens in the editor could say something about reordering commits not being advisable? E.g. I could imagine the todo list including some instructions in the spirit of # git rebase --preserve-merges does not support reordering commits. # To attempt reordering anyway, add a line with the text "reorder". # It is not likely to behave as you expect. You have been # warned. Thanks, Jonathan
Re: Commit dropped when swapping commits with rebase -i -p
Hi Sebastian, On Wed, 30 Aug 2017, Sebastian Schuberth wrote: > On Wed, Aug 30, 2017 at 10:28 PM, Johannes Schindelin > wrote: > > > Please see 'exchange two commits with -p' in t3404. This is a known > > Thank for pointing out the test. > > > breakage, and due to the fact that -p and -i are fundamentally > > incompatible with one another (even if -p's implementation was based on > > -i's). I never had in mind for -p to be allowed together with -i, and was > > against allowing it because of the design. > > In any case, I wouldn't have expected *that* kind of side effect for > such a simple case (that does not involve any merge commits). > > If these options are fundamentally incompatible as you say, would you > agree that it makes sense to disallow their usage together (instead of > just documenting that you should know what you're doing)? As I said already, I agreed with you before you said it, but I was overruled. Ciao, Johannes
Re: Commit dropped when swapping commits with rebase -i -p
On Wed, Aug 30, 2017 at 10:28 PM, Johannes Schindelin wrote: > Please see 'exchange two commits with -p' in t3404. This is a known Thank for pointing out the test. > breakage, and due to the fact that -p and -i are fundamentally > incompatible with one another (even if -p's implementation was based on > -i's). I never had in mind for -p to be allowed together with -i, and was > against allowing it because of the design. In any case, I wouldn't have expected *that* kind of side effect for such a simple case (that does not involve any merge commits). If these options are fundamentally incompatible as you say, would you agree that it makes sense to disallow their usage together (instead of just documenting that you should know what you're doing)? -- Sebastian Schuberth
Re: Commit dropped when swapping commits with rebase -i -p
Hi Sebastian, On Wed, 30 Aug 2017, Sebastian Schuberth wrote: > I believe stumbled upon a nasty bug in Git: It looks like a commits gets > dropped during interactive rebase when asking to preserve merges. Please see 'exchange two commits with -p' in t3404. This is a known breakage, and due to the fact that -p and -i are fundamentally incompatible with one another (even if -p's implementation was based on -i's). I never had in mind for -p to be allowed together with -i, and was against allowing it because of the design. Short version: do not use -p with -i. Ciao, Johannes
Re: Commit dropped when swapping commits with rebase -i -p
On Wed, Aug 30, 2017 at 8:07 PM, Martin Ågren wrote: > The man-page for git rebase says that combining -p with -i is "generally > not a good idea unless you know what you are doing (see BUGS below)". Thanks for pointing this out again. I remember to have read this some time ago, but as I general consider myself to know what I'm doing, I forgot about it :-) Anyway, this should really more explicitly say *what* you need to know about, that is, reordering commits does not work. > So if you agree that a "dropped commit" is a "counterintuitive result", > this is known and documented. Maybe the warning could be harsher, but it > does say "unless you know what you are doing". I'd say it's worse than counterintuitive, as counterintuitive might still be correct, while in my case it clearly is not. So yes, the warning must be harsher in my opinion. Maybe we should even abort rebase -i-p if reordering of commits is detected. -- Sebastian Schuberth
Re: Commit dropped when swapping commits with rebase -i -p
On 30 August 2017 at 12:11, Sebastian Schuberth wrote: > Hi, > > I believe stumbled upon a nasty bug in Git: It looks like a commits gets > dropped during interactive rebase when asking to preserve merges. Steps: > > $ git clone -b git-bug --single-branch > https://github.com/heremaps/scancode-toolkit.git > $ git rebase -i -p HEAD~2 > # In the editor, swap the order of the two (non-merge) commits. > $ git diff origin/git-bug > > The last command will show a non-empty diff which looks as if the "Do not > shadow the os package with a variable name" commit has been dropped, and > indeed "git log" confirms this. The commit should not get dropped, and it > does not if just using "git rebase -i -p HEAD~2" (without "-p"). > > I'm observing this with Git 2.14.1 on Linux. The man-page for git rebase says that combining -p with -i is "generally not a good idea unless you know what you are doing (see BUGS below)". Under BUGS, it says "The todo list presented by --preserve-merges --interactive does not represent the topology of the revision graph. Editing commits and rewording their commit messages should work fine, but attempts to reorder commits tend to produce counterintuitive results." So if you agree that a "dropped commit" is a "counterintuitive result", this is known and documented. Maybe the warning could be harsher, but it does say "unless you know what you are doing". Martin
