[PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering
From: Martin von Zweigbergk martin.von.zweigbe...@gmail.com This series adds supports for 'git log --no-walk=unsorted', which should be useful for the re-roll of my mz/rebase-range series. It also addresses the bug in cherry-pick/revert, which makes it sort revisions by date. On Fri, Aug 10, 2012 at 11:28 PM, Junio C Hamano gits...@pobox.com wrote: Range limited revision walking, e.g. git cherry-pick A..B D~4..D, fundamentally implies sorting and you cannot assume B would appear before D only because B comes before D on the command line (B may even be inside D~4..D range in which case it would not even appear in the final output). Sorry, I probably wasn't clear; I mentioned revision walking, but I only meant the no-walk case. I hope the patches make sense. Martin von Zweigbergk (4): teach log --no-walk=unsorted, which avoids sorting revisions passed to cherry-pick should be in default order cherry-pick/revert: respect order of revisions to pick cherry-pick/revert: default to topological sorting Documentation/git-cherry-pick.txt | 2 +- builtin/log.c | 2 +- builtin/revert.c| 3 ++- revision.c | 18 +++--- revision.h | 6 +- t/t3508-cherry-pick-many-commits.sh | 2 +- t/t3510-cherry-pick-sequence.sh | 4 ++-- t/t4202-log.sh | 10 ++ 8 files changed, 37 insertions(+), 10 deletions(-) -- 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 0/4] Re: cherry-pick and 'log --no-walk' and ordering
y...@google.com writes: [Administrivia: I somehow doubt y...@google.com would reach you, and futzed with the To: line above] From: Martin von Zweigbergk martin.von.zweigbe...@gmail.com This series adds supports for 'git log --no-walk=unsorted', which should be useful for the re-roll of my mz/rebase-range series. It also addresses the bug in cherry-pick/revert, which makes it sort revisions by date. On Fri, Aug 10, 2012 at 11:28 PM, Junio C Hamano gits...@pobox.com wrote: Range limited revision walking, e.g. git cherry-pick A..B D~4..D, fundamentally implies sorting and you cannot assume B would appear before D only because B comes before D on the command line (B may even be inside D~4..D range in which case it would not even appear in the final output). Sorry, I probably wasn't clear; I mentioned revision walking, but I only meant the no-walk case. I hope the patches make sense. I actually think --no-walk, especially when given any negative revision, that sorts is fundamentally a flawed concept (it led to the inconsistency that made git show A..B C vs git show C A..B behave differently, which we had to fix recently). Would anything break if we take your patch, but without two possibilities to revs-no_walk option (i.e. we never sort under no_walk)? That is, the core of your change would become something like this: revision.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/revision.c b/revision.c index 9e8f47a..589d17f 100644 --- a/revision.c +++ b/revision.c @@ -2116,12 +2116,12 @@ int prepare_revision_walk(struct rev_info *revs) } e++; } - commit_list_sort_by_date(revs-commits); if (!revs-leak_pending) free(list); if (revs-no_walk) return 0; + commit_list_sort_by_date(revs-commits); if (revs-limited) if (limit_list(revs) 0) return -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 0/4] Re: cherry-pick and 'log --no-walk' and ordering
Junio C Hamano gits...@pobox.com writes: Would anything break if we take your patch, but without two possibilities to revs-no_walk option (i.e. we never sort under no_walk)? By the way, by would anything break, I do not just mean if our existing tests trigger failures from test_expect_success; I suspect some do assume the sorting behaviour. I am wondering if the sorting makes sense in the real users; in other words, if the failing tests, if any, are expecting sensible and useful behaviour. After all, the sorting by the commit timestamp is made solely to optimize the limit_list() which wants to traverse commits ancestry near the tip of the history, and sorting by the commit timestamp is done because it is usually a good and quick approximation for topological sorting. -- 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 0/4] Re: cherry-pick and 'log --no-walk' and ordering
On Mon, Aug 13, 2012 at 12:17 AM, Junio C Hamano gits...@pobox.com wrote: y...@google.com writes: [Administrivia: I somehow doubt y...@google.com would reach you, and futzed with the To: line above] :-( Sorry, sendemail.from now set. (I apparently answered y instead of just enter to accept the default.) I actually think --no-walk, especially when given any negative revision, that sorts is fundamentally a flawed concept (it led to the inconsistency that made git show A..B C vs git show C A..B behave differently, which we had to fix recently). I completely agree. Would anything break if we take your patch, but without two possibilities to revs-no_walk option (i.e. we never sort under no_walk)? That is, the core of your change would become something like this: I also thought the sorting was just a bug. From what I understand by looking how the code has evolved, the sorting in the no-walk case was not intentional, but more of a consequence of the implementation. That patch you suggested was my first attempt and led me to find the broken cherry-pick test cases that I then fixed in patch 2/4. But, it clearly would break the test case in t4202 called 'git log --no-walk commits sorts by commit time'. So I started digging from there and found e.g. http://thread.gmane.org/gmane.comp.version-control.git/123205/focus=123216 For convenience, I have pasted the commit message of the commit mentioned in that thread at the end of this email. So we would be breaking at least Johannes's use case if we changed it. I would think almost everyone who doesn't already know would expect git rev-list A B to list them in that order, so is a migration desired? Or just change the default for --no-walk from sorted to unsorted in git 2.0? By the way, git-log's documentation says By default, the commits are shown in reverse chronological order., which to some degree is in support of the current behavior. commit 8e64006eee9c82eba513b98306c179c9e2385e4e Author: Johannes Schindelin johannes.schinde...@gmx.de Date: Tue Jul 24 00:38:40 2007 +0100 Teach revision machinery about --no-walk The flag no_walk is present in struct rev_info since a long time, but so far has been in use exclusively by git show. With this flag, you can see all your refs, ordered by date of the last commit: $ git log --abbrev-commit --pretty=oneline --decorate --all --no-walk which is extremely helpful if you have to juggle with a lot topic branches, and do not remember in which one you introduced that uber debug option, or simply want to get an overview what is cooking. (Note that the git log invocation above does not output the same as $ git show --abbrev-commit --pretty=oneline --decorate --all --quiet since git show keeps the alphabetic order that --all returns the refs in, even if the option --date-order was passed.) For good measure, this also adds the --do-walk option which overrides --no-walk. Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de Signed-off-by: Junio C Hamano gits...@pobox.com -- 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 0/4] Re: cherry-pick and 'log --no-walk' and ordering
Martin von Zweigbergk martin.von.zweigbe...@gmail.com writes: I also thought the sorting was just a bug. From what I understand by looking how the code has evolved, the sorting in the no-walk case was not intentional, but more of a consequence of the implementation. That patch you suggested was my first attempt and led me to find the broken cherry-pick test cases that I then fixed in patch 2/4. But, it clearly would break the test case in t4202 called 'git log --no-walk commits sorts by commit time'. So I started digging from there and found e.g. http://thread.gmane.org/gmane.comp.version-control.git/123205/focus=123216 For convenience, I have pasted the commit message of the commit mentioned in that thread at the end of this email. So we would be breaking at least Johannes's use case if we changed it. Ok. Having a way to conveniently sort based on committer date is indeed handy, and losing it would be a regression. Not that the accident that supports only on committer date is a nicely designed feature. The user may want to sort on author date instead, but there is no way to do so with --no-walk. So in that sense, Johannes's use case happens to work by accident. ... so is a migration desired? Or just change the default for --no-walk from sorted to unsorted in git 2.0? I think the proper support for Johannes's case should give users more control on what to sort on, and that switch should not be tied to --no-walk. After all, being able to sort commits in the result of limit_list() with various criteria would equally useful as being able to sort commits listed on the command line with --no-walk. Think about what git shortlog A..B does, for example. It is like first enumerating commits within the given range, and sorting the result using author as the primary and then timestamp as the secondary sort column. So let's not even think about migration, and go in the direction of giving --no-walk two flavours, for now. Either it keeps the order commits were given from the command line, or it does the default sort using the timestamp. We can later add the --sort-on option that would work with or without --no-walk for people who want output that is differently sorted, but that is outside the scope of your series. By the way, git-log's documentation says By default, the commits are shown in reverse chronological order., which to some degree is in support of the current behavior. That is talking about the presentation order of the result of limit_list(), predates --no-walk, and was not adjusted to the new world order when --no-walk was introduced, so I would not take it as a supporting argument. But not regressing the current you can see them sorted on the commit timestamp (this is merely an accident and not a designed feature, so you cannot choose to sort on other things) behaviour is a reason enough not to disable sorting for the plain --no-walk option. Thanks. -- 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 0/4] Re: cherry-pick and 'log --no-walk' and ordering
On Mon, Aug 13, 2012 at 10:05 AM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martin.von.zweigbe...@gmail.com writes: ... so is a migration desired? Or just change the default for --no-walk from sorted to unsorted in git 2.0? I think the proper support for Johannes's case should give users more control on what to sort on, and that switch should not be tied to --no-walk. After all, being able to sort commits in the result of limit_list() with various criteria would equally useful as being able to sort commits listed on the command line with --no-walk. Think about what git shortlog A..B does, for example. It is like first enumerating commits within the given range, and sorting the result using author as the primary and then timestamp as the secondary sort column. So let's not even think about migration, and go in the direction of giving --no-walk two flavours, for now. Either it keeps the order commits were given from the command line, or it does the default sort using the timestamp. We can later add the --sort-on option that would work with or without --no-walk for people who want output that is differently sorted, but that is outside the scope of your series. Makes sense. The shortlog example is a good example of sorting that completely reorders the commit graph sometimes even making sense for ranges. Thanks! -- 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: cherry-pick and 'log --no-walk' and ordering
Martin von Zweigbergk martin.von.zweigbe...@gmail.com writes: On Fri, Aug 10, 2012 at 2:38 PM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martin.von.zweigbe...@gmail.com writes: There is also cherry-pick/revert, which I _think_ does not really want the revisions sorted. Yes, I think sequencer.c::prepare_revs() is wrong to unconditoinally call prepare_revision_walk(). It instead should first check the revs-pending.objects list to see if what was given by the caller is a mere collection of individual objects or a range expression (i.e. check if any of them is marked with UNINTERESTING), and refrain from going into the body of the preparation steps, which has to involve sorting. Do you mean has to involve sorting as in has to involve sorting in order not to break current users of e.g. 'git log --no-walk --branches' or revision walking inherently involves sorting? Range limited revision walking, e.g. git cherry-pick A..B D~4..D, fundamentally implies sorting and you cannot assume B would appear before D only because B comes before D on the command line (B may even be inside D~4..D range in which case it would not even appear in the final output). Any caller that wants to retrieve the objects given from the command line in the order the user gave it, e.g. git cherry-pick A B C, using setup_revisions() and without walking the history, must look at revs-pending.objects without calling prepare_revision_walk(). -- 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
cherry-pick and 'log --no-walk' and ordering
A while ago when I was looking at revision.c, I was surprised to see that commits are sorted even when --no-walk is passed, but as 8e64006 (Teach revision machinery about --no-walk, 2007-07-24) points out, this can be useful for doing $ git log --abbrev-commit --pretty=oneline --decorate --all --no-walk and get the result sorted by date. However, it can also be useful _not_ to get a result sorted by date, e.g. when doing something like generate an ordered list of revisions | git rev-list --oneline --no-walk --stdin. Would a --no-sort option to rev-list be appreciated or are there better solutions? There is also cherry-pick/revert, which I _think_ does not really want the revisions sorted. cherry-pick implicitly reverses the order of the walk, so 'git cherry-pick branch~2..branch' applies them in the right order (at least in the absence of clock skew). The documentation for cherry-pick suggests git rev-list --reverse master -- README | git cherry-pick -n --stdin, which I think makes no sense -- this would reverse the output from rev-list only to have it reversed again in cherry-pick, if it wasn't for the sorting by date. I think the --reverse passed to rev-list might even break the cherry-pick if there are commits in non-increasing date order. This is also supported by the fact that test still pass after applying the patch below. I think the test cases make more sense after the patch. Do others agree with the analysis? I suppose it's too late to change cherry-pick to start differentiating between git cherry-pick commit1 commit2 and git cherry-pick commit2 commit1, but I think we should at least update the documentation as in the patch below (or maybe even with a --topo-order passed to cherry-pick?). We could possibly change cherry-pick's ordering from the default ordering to topological ordering. Martin Sorry about the mangled whitespace below; just for reference, not intended to be applied. 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 -- 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: cherry-pick and 'log --no-walk' and ordering
Martin von Zweigbergk martin.von.zweigbe...@gmail.com writes: There is also cherry-pick/revert, which I _think_ does not really want the revisions sorted. Yes, I think sequencer.c::prepare_revs() is wrong to unconditoinally call prepare_revision_walk(). It instead should first check the revs-pending.objects list to see if what was given by the caller is a mere collection of individual objects or a range expression (i.e. check if any of them is marked with UNINTERESTING), and refrain from going into the body of the preparation steps, which has to involve sorting. I think we had to fix a bug in git show coming from a similar root cause, but the bug manifested in the opposite direction. -- 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: cherry-pick and 'log --no-walk' and ordering
On Fri, Aug 10, 2012 at 2:38 PM, Junio C Hamano gits...@pobox.com wrote:
Martin von Zweigbergk martin.von.zweigbe...@gmail.com writes:
There is also cherry-pick/revert, which I _think_ does not really want
the revisions sorted.
Yes, I think sequencer.c::prepare_revs() is wrong to unconditoinally
call prepare_revision_walk().
It instead should first check the revs-pending.objects list to see
if what was given by the caller is a mere collection of individual
objects or a range expression (i.e. check if any of them is marked
with UNINTERESTING), and refrain from going into the body of the
preparation steps, which has to involve sorting.
Do you mean has to involve sorting as in has to involve sorting in
order not to break current users of e.g. 'git log --no-walk
--branches' or revision walking inherently involves sorting? My
current working assumption is that it is the former.
I will make rev_info.no_walk a tri-state {walk, no-walk-sorted,
no-walk-unsorted}. The third state would be used from
cherry-pick/revert (and maybe git-show, although it should make no
difference). I would also expose the third state to rev-list's command
line, maybe as --no-walk=unsorted.
Actually, all but command-line parsing is done now and test seem fine,
with quite a small patch:
$ git diff --stat
builtin/log.c| 2 +-
builtin/revert.c | 2 +-
revision.c | 5 +++--
revision.h | 6 +-
4 files changed, 10 insertions(+), 5 deletions(-)
Did you see a problem with this approach, since you said that
sequencer shouldn't unconditionally call prepare_revision_walk()? I
can see that git-show needs to go through revs-pending.objects
because it handles tags and stuff, but cherry-pick/revert only seem to
need the revisions.
Martin
--
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
