[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
