Re: [PATCH 2/4] revisions passed to cherry-pick should be in default order

2012-08-15 Thread Martin von Zweigbergk
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

2012-08-15 Thread Junio C Hamano
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

2012-08-15 Thread Martin von Zweigbergk
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

2012-08-15 Thread Junio C Hamano
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

2012-08-15 Thread Martin von Zweigbergk
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


Re: [PATCH 2/4] revisions passed to cherry-pick should be in default order

2012-08-13 Thread Junio C Hamano
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?
--
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

2012-08-13 Thread Martin von Zweigbergk
On Sun, Aug 12, 2012 at 11:27 PM,  y...@google.com wrote:
 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.

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

2012-08-13 Thread Martin von Zweigbergk
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

2012-08-13 Thread Junio C Hamano
Martin von Zweigbergk martin.von.zweigbe...@gmail.com 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

2012-08-13 Thread Junio C Hamano
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