Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
On Wed, Aug 15, 2012 at 11:39 AM, Junio C Hamano wrote: > Martin von Zweigbergk writes: > >> Makes sense, I'll try to implement it that way. I was afraid that >> we would need to call prepare_revision_walk() once first and then >> if we afterwards find out that we should not walk, we would need >> to call it again without the reverse option. > >> But after looking at >> how rev_info.reverse is used, it seem like it's only used in >> get_revision(), so we can leave it either on or off during the >> prepare_revision_walk() and the and set appropriately before >> calling get_revision(), like so: >> >> init_revisions(&revs); >> revs.no_walk = REVISION_WALK_NO_WALK_UNSORTED; >> setup_revisions(...); >> prepare_revision_walk(&revs); >> revs.reverse = !revs.no_walk; > > Sorry, but I do not understand why you frutz with "reverse" after > prepare, and not before. > > I think you can just set no_walk and let setup_revisions() turn it > off upon seeing a range (this happens in add_pending_object()). Ah, of course. For some reason I thought that was called from prepare_revision_walk() > After setup_revisions() returns, if no_walk is still set, you only > got individual refs without ranges, so no reversing required. Yes, it's in the other case (e.g. 'git cherry-pick A..C', when no_walk is not set), that we need to set reverse before walking. > You also need to be careful about "revert" that shares the code; > when reverting range A..C in your example, you want to undo C and > then B, and you do not want to reverse them. Yep. It looks like this, so should be safe. But thanks for the reminder. if (opts->action != REPLAY_REVERT) opts->revs->reverse ^= 1; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
Martin von Zweigbergk writes: > Makes sense, I'll try to implement it that way. I was afraid that > we would need to call prepare_revision_walk() once first and then > if we afterwards find out that we should not walk, we would need > to call it again without the reverse option. > But after looking at > how rev_info.reverse is used, it seem like it's only used in > get_revision(), so we can leave it either on or off during the > prepare_revision_walk() and the and set appropriately before > calling get_revision(), like so: > > init_revisions(&revs); > revs.no_walk = REVISION_WALK_NO_WALK_UNSORTED; > setup_revisions(...); > prepare_revision_walk(&revs); > revs.reverse = !revs.no_walk; Sorry, but I do not understand why you frutz with "reverse" after prepare, and not before. I think you can just set no_walk and let setup_revisions() turn it off upon seeing a range (this happens in add_pending_object()). After setup_revisions() returns, if no_walk is still set, you only got individual refs without ranges, so no reversing required. You also need to be careful about "revert" that shares the code; when reverting range A..C in your example, you want to undo C and then B, and you do not want to reverse them. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
On Wed, Aug 15, 2012 at 10:16 AM, Junio C Hamano wrote: > Martin von Zweigbergk writes: > >> So all of the above case give the right result in the end as long >> as the timestamps are chronological, and case 1) gives the right >> result regardless. The other two cases only works in most cases >> because the unexpcted sorting when no-walk is in effect >> counteracts the final reversal. > > In short, if you have three commits in a row, A--B--C, with > timestamps that are not skewed, and want to replay changes of B and > then C in that order, all three you listed ends up doing the right > thing. But if you want to apply the change C and then B: > > - "git cherry-pick A..C" is obviously not a way to do so, so we > won't discuss it further. > > - "git cherry-pick C B" is the most natural way the user would > want to express this request, but because of the sorting > (i.e. commit_list_sort_by_date() in prepare_revision_walk(), > combined with ->reverse in sequencer.c::prepare_revs()), it > applies B and then C. That is the real bug. > > Feeding the revs to "git cherry-pick --stdin" in the order the > user wishes them to be applied has the same issue. Exactly. > I actually think your approach to place the "do not sort when we are > not walking" logic in prepare_revision_walk() makes more sense. > "show" has to look at pending.objects[] because it needs to show > objects other than commits (e.g. "git show :foo"), so there won't be > any change in its implementation with your change. It will have to > look at pending.objects[] itself. Yes, I noticed that's why "show" has to do it that way. > But "cherry-pick" and sequencer-derived commands only deal with > commits. It would be far less error prone to let them call > get_revision() repeatedly like all other revision enumerating > commands do, than to have them go over the pending.objects[] list, > dereferencing tags and using only commits. The resulting callers > would be more readable, too, I would think. Makes sense, I'll try to implement it that way. I was afraid that we would need to call prepare_revision_walk() once first and then if we afterwards find out that we should not walk, we would need to call it again without the reverse option. But after looking at how rev_info.reverse is used, it seem like it's only used in get_revision(), so we can leave it either on or off during the prepare_revision_walk() and the and set appropriately before calling get_revision(), like so: init_revisions(&revs); revs.no_walk = REVISION_WALK_NO_WALK_UNSORTED; setup_revisions(...); prepare_revision_walk(&revs); revs.reverse = !revs.no_walk; // iterate over revisions -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
Martin von Zweigbergk writes: > So all of the above case give the right result in the end as long > as the timestamps are chronological, and case 1) gives the right > result regardless. The other two cases only works in most cases > because the unexpcted sorting when no-walk is in effect > counteracts the final reversal. In short, if you have three commits in a row, A--B--C, with timestamps that are not skewed, and want to replay changes of B and then C in that order, all three you listed ends up doing the right thing. But if you want to apply the change C and then B: - "git cherry-pick A..C" is obviously not a way to do so, so we won't discuss it further. - "git cherry-pick C B" is the most natural way the user would want to express this request, but because of the sorting (i.e. commit_list_sort_by_date() in prepare_revision_walk(), combined with ->reverse in sequencer.c::prepare_revs()), it applies B and then C. That is the real bug. Feeding the revs to "git cherry-pick --stdin" in the order the user wishes them to be applied has the same issue. > IIUC, this could be implemented by making cherry-pick iterate > over rev_info.pending.objects just like 'git show' does when not > walking. Yes, that was exactly why I said sequencer.c::prepare_revs() is wrong to call prepare_revision_walk() unconditionally, even when there is no revision walking involved. I actually think your approach to place the "do not sort when we are not walking" logic in prepare_revision_walk() makes more sense. "show" has to look at pending.objects[] because it needs to show objects other than commits (e.g. "git show :foo"), so there won't be any change in its implementation with your change. It will have to look at pending.objects[] itself. But "cherry-pick" and sequencer-derived commands only deal with commits. It would be far less error prone to let them call get_revision() repeatedly like all other revision enumerating commands do, than to have them go over the pending.objects[] list, dereferencing tags and using only commits. The resulting callers would be more readable, too, I would think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
On Mon, Aug 13, 2012 at 2:05 PM, Junio C Hamano wrote: > Martin von Zweigbergk writes: > >> To connect to the other mail I sent on this thread (in parallel with >> yours), do you think "git cherrry-pick HEAD HEAD~1" should apply the >> commits in the same order as "git cherry-pick HEAD~2..HEAD" (which >> would give the same result if passed to 'rev-list --no-walk' for a >> linear history) or in the order specified on the command line? > > Definitely the latter; I do not think of any semi-reasonable excuse > to do otherwise. Indeed. My patches tried to fix the wrong problem. Sorry I'm slow, but I think I'm finally starting to understand what you've been saying all along about the bug being in sequencer. I'll try to recapitulate a bit for my own and maybe others' understanding. For simplicity, let's assume a linear history with unique timestamps, but not necessarily increasing with each commit. Currently: 1) 'git cherry-pick A..C' picks the commits order in reverse "default" order 2) 'git cherry-pick B C' picks the commits in chronological order 3) 'git rev-list --reverse A..C | git cherry-pick --stdin' behaves just like 'git cherry-pick B C' and therefore picks the commits in chronological order In cases 2) and 3), even though cherry-pick tells the revision walker not to walk, it still sorts the commits in reverse chronological order. But cherry-pick also tells the revision walker explicitly to reverse the list, so in the end, the order is chronological. In case 2), however, the first ordering make no difference in this "limited" case (IIUC). So the "default" ordering (which would be C, then B in this case, regardless of timestamps), gets reversed and B gets applied first, followed by C. So all of the above case give the right result in the end as long as the timestamps are chronological, and case 1) gives the right result regardless. The other two cases only works in most cases because the unexpcted sorting when no-walk is in effect counteracts the final reversal. When I noticed that the order of inputs to cases 2) and 3) above was ignored, and thinking that 'git rev-list A..C | git cherry-pick --stdin' should mimic 'git cherry-pick A..C', I incorrectly thought that the error was the use of --reverse to 'git rev-list' as well as the sorting done in the no-walk case. I think completely ignored case 2) at this point. I now think I understand that the sorting done in the no-walk case is indeed incorrect, but that the --reverse passed to rev-list is correct. Instead, the final reversal, which is currently unconditional, should not be done in the no-walk case. IIUC, this could be implemented by making cherry-pick iterate over rev_info.pending.objects just like 'git show' does when not walking. Junio, I think it makes sense to just drop this whole series for now. I'll probably include patch 1/4 in my stalled rebase-range series instead. If I understood you correctly, you didn't have any objections to that patch. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
Martin von Zweigbergk writes: > To connect to the other mail I sent on this thread (in parallel with > yours), do you think "git cherrry-pick HEAD HEAD~1" should apply the > commits in the same order as "git cherry-pick HEAD~2..HEAD" (which > would give the same result if passed to 'rev-list --no-walk' for a > linear history) or in the order specified on the command line? Definitely the latter; I do not think of any semi-reasonable excuse to do otherwise. > I couldn't find any conclusive evidence of what was intended in > either log messages or test cases. Do not take the "multi-commit handling" that was bolted on to cherry-pick and revert long after these commands with a single commit form were polished and have become stable too seriously and its behaviour cast in stone. There is no reason to believe the bolted-on part was designed with sufficient thoughts behind it, nor was implemented with the same competency as the code before it was introduced. I recall myself applying these patches after only cursory review, saying "Meh, I wouldn't do multiple commits anyway, and bugs found by people can be fixed later" ;-). It is OK to consider its doneness as "the developers declared success based on their limited testing; it internally still sorts, but sorting a range by timestamp happens to yield the correct result most of the time, and this bug was not found until much later. There certainly are other bugs, at both implementation and design level, yet to be discovered." phase of its lifecycle. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
Martin von Zweigbergk writes: > By the way, I can see the usefulness of --reverse when giving a range, > but I think it's a little confusing when not giving a range. "git rev-list --reverse --root v1.0.0" is a way to say "give me a list of commits to be replayed in sequence" without having a bottom, no? Ah, you mean when we do _not_ walk. Yeah, that is why I said that when we do not walk, we should not even call into prepare_revision_walk() in the first place in my earlier message. We should take the commits as given from the revs->pending.objects list instead. With your "no_walk = NO_WALK_UNSORTED", calling prepare_revision_walk() would amont to the same thing, as you would not sort the commits and use them as given by the user. > So "git cherry-pick A B" will apply B first, then A. I am confused a bit. Are you describing a buggy behaviour in the current codebase, or are you saying we should fix it to behave that way? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
On Mon, Aug 13, 2012 at 1:05 PM, Junio C Hamano wrote: > y...@google.com writes: > >> From: Martin von Zweigbergk >> >> 'git cherry-pick' internally sets the --reverse option while walking >> revisions, so that 'git cherry-pick branch@{u}..branch' will apply the >> revisions starting at the oldest one. If no uninteresing revisions are >> given, --no-walk is implied. Still, the documentation for 'git >> cherry-pick --stdin' uses the following example: >> >> git rev-list --reverse master -- README | git cherry-pick -n --stdin >> >> The above would seem to reverse the revisions in the output (which it >> does), and then pipe them to 'git cherry-pick', which would reverse >> them again and apply them in the wrong order. > > I think we have cleared this confusion up in the previous > discussion. It it sequencer's bug that reorders the commits when > the caller ("rev-list --reverse" in this case) gives list of > individual commits to replay. > > So I think we are all OK with chucking this patch. Am I mistaken? I can't really say. I suppose the current patch is smaller (it can't really get smaller than one line), but iterating over the arguments the sequencer level might be more correct. Would the result be different in some cases? I would be happy to add a test case at least, although I'm not sure when I would have time to implement it in sequencer. To connect to the other mail I sent on this thread (in parallel with yours), do you think "git cherrry-pick HEAD HEAD~1" should apply the commits in the same order as "git cherry-pick HEAD~2..HEAD" (which would give the same result if passed to 'rev-list --no-walk' for a linear history) or in the order specified on the command line? I couldn't find any conclusive evidence of what was intended in either log messages or test cases. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
On Sun, Aug 12, 2012 at 11:27 PM, wrote: > From: Martin von Zweigbergk > > 'git cherry-pick' internally sets the --reverse option while walking > revisions, so that 'git cherry-pick branch@{u}..branch' will apply the > revisions starting at the oldest one. By the way, I can see the usefulness of --reverse when giving a range, but I think it's a little confusing when not giving a range. So "git cherry-pick A B" will apply B first, then A. I thought I'd mention that explicitly in case it wasn't clear. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order
y...@google.com writes: > From: Martin von Zweigbergk > > 'git cherry-pick' internally sets the --reverse option while walking > revisions, so that 'git cherry-pick branch@{u}..branch' will apply the > revisions starting at the oldest one. If no uninteresing revisions are > given, --no-walk is implied. Still, the documentation for 'git > cherry-pick --stdin' uses the following example: > > git rev-list --reverse master -- README | git cherry-pick -n --stdin > > The above would seem to reverse the revisions in the output (which it > does), and then pipe them to 'git cherry-pick', which would reverse > them again and apply them in the wrong order. I think we have cleared this confusion up in the previous discussion. It it sequencer's bug that reorders the commits when the caller ("rev-list --reverse" in this case) gives list of individual commits to replay. So I think we are all OK with chucking this patch. Am I mistaken? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] revisions passed to cherry-pick should be in "default" order
From: Martin von Zweigbergk 'git cherry-pick' internally sets the --reverse option while walking revisions, so that 'git cherry-pick branch@{u}..branch' will apply the revisions starting at the oldest one. If no uninteresing revisions are given, --no-walk is implied. Still, the documentation for 'git cherry-pick --stdin' uses the following example: git rev-list --reverse master -- README | git cherry-pick -n --stdin The above would seem to reverse the revisions in the output (which it does), and then pipe them to 'git cherry-pick', which would reverse them again and apply them in the wrong order. The same problem occurs when supplying revisions explicitly on the command line instead of sending them to stdin. Because of the sorting-by-date that is done by the revision walker (even with the implied --no-walk), the ordering in the output from 'git rev-list' in the example above is effectively ignored, and the above actually works most of the time. However, if revisions share a commit date (as can easily happen as a result of rebasing), they do get applied out-of-order. Update the documentation not to suggest reversing the input to 'git cherry-pick'. Also update test cases where the inputs are reversed. Signed-off-by: Martin von Zweigbergk --- Documentation/git-cherry-pick.txt | 2 +- t/t3508-cherry-pick-many-commits.sh | 2 +- t/t3510-cherry-pick-sequence.sh | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 0e170a5..454e205 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -181,7 +181,7 @@ EXAMPLES are in next but not HEAD to the current branch, creating a new commit for each new change. -`git rev-list --reverse master -- README | git cherry-pick -n --stdin`:: +`git rev-list master -- README | git cherry-pick -n --stdin`:: Apply the changes introduced by all commits on the master branch that touched README to the working tree and index, diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index 75f7ff4..020baaf 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -164,7 +164,7 @@ test_expect_success 'cherry-pick --stdin works' ' git checkout -f master && git reset --hard first && test_tick && - git rev-list --reverse first..fourth | git cherry-pick --stdin && + git rev-list first..fourth | git cherry-pick --stdin && git diff --quiet other && git diff --quiet HEAD other && check_head_differs_from fourth diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index f4e6450..9e28910 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -400,7 +400,7 @@ test_expect_success '--continue of single-pick respects -x' ' test_expect_success '--continue respects -x in first commit in multi-pick' ' pristine_detach initial && - test_must_fail git cherry-pick -x picked anotherpick && + test_must_fail git cherry-pick -x anotherpick picked && echo c >foo && git add foo && git cherry-pick --continue && @@ -430,7 +430,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' ' pristine_detach initial && - test_must_fail git cherry-pick -s picked anotherpick && + test_must_fail git cherry-pick -s anotherpick picked && echo c >foo && git add foo && git cherry-pick --continue && -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html