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 gits...@pobox.com wrote: Martin von Zweigbergk martin.von.zweigbe...@gmail.com 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 martin.von.zweigbe...@gmail.com 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 Wed, Aug 15, 2012 at 10:16 AM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martin.von.zweigbe...@gmail.com 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 martin.von.zweigbe...@gmail.com 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 11:39 AM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martin.von.zweigbe...@gmail.com 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
[PATCH 2/4] revisions passed to cherry-pick should be in default order
From: Martin von Zweigbergk martin.von.zweigbe...@gmail.com
'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 martin.von.zweigbe...@gmail.com
---
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
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 gits...@pobox.com wrote:
y...@google.com writes:
From: Martin von Zweigbergk martin.von.zweigbe...@gmail.com
'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
Martin von Zweigbergk martin.von.zweigbe...@gmail.com 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
