Re: [PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering
On Mon, Aug 13, 2012 at 2:31 PM, Junio C Hamano wrote: > Martin von Zweigbergk writes: > >> Makes sense. The shortlog example is a good example of sorting that >> completely reorders the commit graph sometimes even making sense for >> ranges. Thanks! > > By the way, does this topic relate to the long stalled "rebase" > topic from you, and if so how? Yes, but only through the first patch in the series. Unless I'm mistaken, I would can get a list of revisions to rebase using git-patch-id, but to convert that into a instruction list with running git-log on each commit, I planned to use 'git rev-list --format=... --no-walk=unsorted --stdin', which of course doesn't exist before patch 1/4. The rest of the current series is a little fuzzy to me, especially the confusion about reversing or not. Feel free to split out patch 1 into a separate topic if you like, or however you would handle that. -- 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 writes: > Makes sense. The shortlog example is a good example of sorting that > completely reorders the commit graph sometimes even making sense for > ranges. Thanks! By the way, does this topic relate to the long stalled "rebase" topic from you, and if so how? -- 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 wrote: > Martin von Zweigbergk 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: [PATCH 0/4] Re: cherry-pick and 'log --no-walk' and ordering
Martin von Zweigbergk 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 > 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 12:17 AM, Junio C Hamano 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 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 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 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 Signed-off-by: Junio C Hamano -- 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 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
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 > > 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 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
