Message ID | pull.590.git.1585429894378.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | doc: log takes multiple revision ranges | expand |
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Philippe Blain <levraiphilippeblain@gmail.com> > > `git log` accepts multiple revision ranges, but makes no mention of that > in its documentation. Currently readers have to go to the `git > rev-list` documentation to discover this useful feature. I have a mixed feeling about this change. Surely you can write git log v1.0..v2.0 v4.0..v5.0 -- Makefile and you can explain that the command appears to take two "ranges", but I do not think we want to encourage it to those who do not understand what the above actually *means*. Specifially, it does *NOT* list commits between v1.0 and v2.0, or between v4.0 and v5.0, that touch the path Makefile. In other words, you didn't really give two ranges with the syntax. What the A..B notation means could be called a range, only when it appears alone. And we have *no* intention of changing the semantics. "A..B" is a mere short-hand for ^A B, and it may appear to be a range, but "A..B C..D" does not make "two ranges". It still merely is a short-hand for ^A B ^C D, and if C reaches A and B (in the above example, v4.0 is very likely to reach both v1.0 and v2.0), it means the same thing as "C..D", i.e. "git log v4.0..v5.0 -- Makefile". So I have a mildly strong opinion against the change to the synopsys and the short-help; it is a bad change that does not help users. If there are not sufficient explanation on the equivalence between A..B and ^A B on "git log" documentation but there is one elsewhere, adding a reference to help users learn is very much appreciated, though, and I initially was hoping that the second hunk of this patch was such a change, but it appears that it is not (it just adds the misleading "you can have more than one" without making readers aware of what it means to write "A..B C..D"). Thanks.
Hi Junio, > Le 28 mars 2020 à 21:34, Junio C Hamano <gitster@pobox.com> a écrit : > > "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Philippe Blain <levraiphilippeblain@gmail.com> >> >> `git log` accepts multiple revision ranges, but makes no mention of that >> in its documentation. Currently readers have to go to the `git >> rev-list` documentation to discover this useful feature. > > I have a mixed feeling about this change. Surely you can write > > git log v1.0..v2.0 v4.0..v5.0 -- Makefile > > and you can explain that the command appears to take two "ranges", > but I do not think we want to encourage it to those who do not > understand what the above actually *means*. > > Specifially, it does *NOT* list commits between v1.0 and v2.0, or > between v4.0 and v5.0, that touch the path Makefile. In other > words, you didn't really give two ranges with the syntax. What the > A..B notation means could be called a range, only when it appears > alone. > > And we have *no* intention of changing the semantics. Of course, I understand. > > "A..B" is a mere short-hand for ^A B, and it may appear to be a > range, but "A..B C..D" does not make "two ranges". It still merely > is a short-hand for ^A B ^C D, and if C reaches A and B (in the > above example, v4.0 is very likely to reach both v1.0 and v2.0), it > means the same thing as "C..D", i.e. "git log v4.0..v5.0 -- Makefile". Yes, you are right. > > So I have a mildly strong opinion against the change to the synopsys > and the short-help; it is a bad change that does not help users. If > there are not sufficient explanation on the equivalence between A..B > and ^A B on "git log" documentation but there is one elsewhere, adding > a reference to help users learn is very much appreciated, though, So the 'git log' documentation states: origin..HEAD specifies all the commits reachable from the current commit (i.e. HEAD), but not from origin. For a complete list of ways to spell <revision range>, see the Specifying Ranges section of gitrevisions[7]. What I wanted to achieve with this patch is to make readers aware that they can write e.g. git log branch1 branch2 branch3 branch4 --not master to see commits on each of branch1-4, that are not on master, since this is not immediately obvious (at least to me) in the 'git log' man page. Even following the link to gitrevisions[7], it is not explicitly mentioned in the "Specifying Ranges" section that a range denotes the set of all commits reachable by all given refs, minus the ones reachable by all given refs prefixed by '^' or '--not'. However, this is really clear in the 'git rev-list' documentation, which uses '<commit>...' in the synopsis and clearly talks about the "set of commits" point of view: You can think of this as a set operation. Commits given on the command line form a set of commits that are reachable from any of them, and then commits reachable from any of the ones given with ^ in front are subtracted from that set. The remaining commits are what comes out in the command’s output. Various other options and paths parameters can be used to further limit the result. Do you think it would be appropriate that this explanation could somehow also appear in the 'git log' (and maybe also 'gitrevisions') documentation ? Thanks for the quick feedback, Philippe.
Philippe Blain <levraiphilippeblain@gmail.com> writes: > However, this is really clear in the 'git rev-list' documentation, > which uses '<commit>...' in the synopsis > and clearly talks about the "set of commits" point of view: > > You can think of this as a set operation. Commits given on the command line form > a set of commits that are reachable from any of them, and then commits reachable > from any of the ones given with ^ in front are subtracted from that set. The remaining > commits are what comes out in the command’s output. Various other options and > paths parameters can be used to further limit the result. > > Do you think it would be appropriate that this explanation could somehow also appear > in the 'git log' (and maybe also 'gitrevisions') documentation ? Thanks for finding that paragraph. I said in my previous message that I didn't think the synopsis and short-help changes were bad, and I hoped that the other hunk of the patch added reference to "git log" page that existed elsewhere to help readers understand what the positive and negative options do. A reference to the "set operation" is exactly the kind of addition I meant I hoped to see. By the way, I do not write "git log ab/topic-1 cd/topic-2 --not master", but "git log ^master ab/topic-1 cd/topic-2 ef/topic-3" is something I'd quite often write.
On Sat, Mar 28, 2020 at 06:34:14PM -0700, Junio C Hamano wrote: > "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Philippe Blain <levraiphilippeblain@gmail.com> > > > > `git log` accepts multiple revision ranges, but makes no mention of that > > in its documentation. Currently readers have to go to the `git > > rev-list` documentation to discover this useful feature. > > I have a mixed feeling about this change. Surely you can write > > git log v1.0..v2.0 v4.0..v5.0 -- Makefile > > and you can explain that the command appears to take two "ranges", > but I do not think we want to encourage it to those who do not > understand what the above actually *means*. > > Specifially, it does *NOT* list commits between v1.0 and v2.0, or > between v4.0 and v5.0, that touch the path Makefile. In other > words, you didn't really give two ranges with the syntax. What the > A..B notation means could be called a range, only when it appears > alone. > > And we have *no* intention of changing the semantics. > > "A..B" is a mere short-hand for ^A B, and it may appear to be a > range, but "A..B C..D" does not make "two ranges". It still merely > is a short-hand for ^A B ^C D, and if C reaches A and B (in the > above example, v4.0 is very likely to reach both v1.0 and v2.0), it > means the same thing as "C..D", i.e. "git log v4.0..v5.0 -- Makefile". > > So I have a mildly strong opinion against the change to the synopsys > and the short-help; it is a bad change that does not help users. I second this opinion. I think that reading: [<revision range> [...]] is very misleading. It guides the reader to think that they can specify multiple ranges, but does not provide clear guidance about how multiple ranges are interpreted. I think most readers will naturally assume that the interpretation is "take the union of each provided range", but this is not the case as Junio points out. > If there are not sufficient explanation on the equivalence between > A..B and ^A B on "git log" documentation but there is one elsewhere, > adding a reference to help users learn is very much appreciated, > though, and I initially was hoping that the second hunk of this patch > was such a change, but it appears that it is not (it just adds the > misleading "you can have more than one" without making readers aware > of what it means to write "A..B C..D"). I think the documentation is probably clearer as it is. Having this extra explanation about the difference between the two doesn't guarantee that it will be read, so it may just be adding additional complexity that isn't warranted ;). > Thanks. Thanks, Taylor
Hi Philippe, This had been sitting in my inbox.. On 30/03/2020 13:52, Philippe Blain wrote: > Hi Junio, >> Le 28 mars 2020 à 21:34, Junio C Hamano <gitster@pobox.com> a écrit : >> >> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> From: Philippe Blain <levraiphilippeblain@gmail.com> >>> >>> `git log` accepts multiple revision ranges, but makes no mention of that >>> in its documentation. Currently readers have to go to the `git >>> rev-list` documentation to discover this useful feature. I'd concur that sometime the documentation is not as educationally informative as it could be. Often it leaves readers to second guess what is meant, or redirects to other reference sections with similar presumptions that the reader already understands. Documentation is harder than coding because of these differences in writing style. Often Git appears to prefer the normative standard style. >> I have a mixed feeling about this change. Surely you can write >> >> git log v1.0..v2.0 v4.0..v5.0 -- Makefile >> >> and you can explain that the command appears to take two "ranges", >> but I do not think we want to encourage it to those who do not >> understand what the above actually *means*. >> >> Specifially, it does *NOT* list commits between v1.0 and v2.0, or >> between v4.0 and v5.0, that touch the path Makefile. In other >> words, you didn't really give two ranges with the syntax. What the >> A..B notation means could be called a range, only when it appears >> alone. >> >> And we have *no* intention of changing the semantics. > Of course, I understand. > >> "A..B" is a mere short-hand for ^A B, and it may appear to be a >> range, but "A..B C..D" does not make "two ranges". It still merely >> is a short-hand for ^A B ^C D, and if C reaches A and B (in the >> above example, v4.0 is very likely to reach both v1.0 and v2.0), it >> means the same thing as "C..D", i.e. "git log v4.0..v5.0 -- Makefile". > Yes, you are right. > >> So I have a mildly strong opinion against the change to the synopsys >> and the short-help; it is a bad change that does not help users. If >> there are not sufficient explanation on the equivalence between A..B >> and ^A B on "git log" documentation but there is one elsewhere, adding >> a reference to help users learn is very much appreciated, though, > So the 'git log' documentation states: > > origin..HEAD specifies all the commits reachable from the current commit > (i.e. HEAD), but not from origin. For a complete list of ways to spell <revision range>, > see the Specifying Ranges section of gitrevisions[7]. > > What I wanted to achieve with this patch is to make readers aware that > they can write e.g. > > git log branch1 branch2 branch3 branch4 --not master > > to see commits on each of branch1-4, that are not on master, > since this is not immediately obvious (at least to me) > in the 'git log' man page. The whole revision walk stuff is tricky. Finding the right starting point for explanations is key, along with picking out where 'normal' errors are made, as these do help users who aren't simply 'checking the manual'. (the 20-20 hindsight problem) For example, your "that are not on master", and what that means, needs to be brought out to the new reader rather than being buried mid-sentence. Most readers will miss those mid-sentence key points and they can't learn until they have some significant problem that points it out to them. Sometime we need to point at the 'wrong' things, not just the right things. (e.g. if branch3 had ben merged a couple of revisions ago...) > Even following the link to gitrevisions[7], > it is not explicitly mentioned in the "Specifying Ranges" section > that a range denotes the set of all commits reachable by all given refs, > minus the ones reachable by all given refs prefixed by '^' or '--not'. > However, this is really clear in the 'git rev-list' documentation, > which uses '<commit>...' in the synopsis > and clearly talks about the "set of commits" point of view: > > You can think of this as a set operation. Commits given on the command line form > a set of commits that are reachable from any of them, and then commits reachable > from any of the ones given with ^ in front are subtracted from that set. The remaining > commits are what comes out in the command’s output. Various other options and > paths parameters can be used to further limit the result. > Set operations don't help everyone, just sayin'. In some cases: The fact that we exclude any commit reachable from any of the "^" commits should be mentioned first, and then we select any left that are reachable from the named tips. This puts a different spin on the understanding, and helps catch those that have not understood the other descriptions. It can then be easily linked to the 'two ranges' case producing otherwise unexpected results. > Do you think it would be appropriate that this explanation could somehow also appear > in the 'git log' (and maybe also 'gitrevisions') documentation ? > > Thanks for the quick feedback, > Philippe. Philip
Hi Philip, > Le 5 juil. 2020 à 16:42, Philip Oakley <philipoakley@iee.email> a écrit : > > Hi Philippe, > This had been sitting in my inbox.. > -----8<----- >> >> What I wanted to achieve with this patch is to make readers aware that >> they can write e.g. >> >> git log branch1 branch2 branch3 branch4 --not master >> >> to see commits on each of branch1-4, that are not on master, >> since this is not immediately obvious (at least to me) >> in the 'git log' man page. > > The whole revision walk stuff is tricky. Finding the right starting > point for explanations is key, along with picking out where 'normal' > errors are made, as these do help users who aren't simply 'checking the > manual'. (the 20-20 hindsight problem) > > For example, your "that are not on master", and what that means, needs > to be brought out to the new reader rather than being buried > mid-sentence. Most readers will miss those mid-sentence key points and > they can't learn until they have some significant problem that points it > out to them. Sometime we need to point at the 'wrong' things, not just > the right things. (e.g. if branch3 had ben merged a couple of revisions > ago...) Yes, here I meant "on master" in the "not yet merged into master" sense, of course. I'd be more careful if I was writing a commit message or part of the doc itself. > >> Even following the link to gitrevisions[7], >> it is not explicitly mentioned in the "Specifying Ranges" section >> that a range denotes the set of all commits reachable by all given refs, >> minus the ones reachable by all given refs prefixed by '^' or '--not'. >> However, this is really clear in the 'git rev-list' documentation, >> which uses '<commit>...' in the synopsis >> and clearly talks about the "set of commits" point of view: >> >> You can think of this as a set operation. Commits given on the command line form >> a set of commits that are reachable from any of them, and then commits reachable >> from any of the ones given with ^ in front are subtracted from that set. The remaining >> commits are what comes out in the command’s output. Various other options and >> paths parameters can be used to further limit the result. >> > Set operations don't help everyone, just sayin'. > > In some cases: The fact that we exclude any commit reachable from any of > the "^" commits should be mentioned first, and then we select any left > that are reachable from the named tips. This puts a different spin on > the understanding, and helps catch those that have not understood the > other descriptions. It can then be easily linked to the 'two ranges' > case producing otherwise unexpected results. I'll try to tweak the wording for v3. Thanks, Philippe.
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt index bed09bb09e5..fa8affd436a 100644 --- a/Documentation/git-log.txt +++ b/Documentation/git-log.txt @@ -9,15 +9,15 @@ git-log - Show commit logs SYNOPSIS -------- [verse] -'git log' [<options>] [<revision range>] [[--] <path>...] +'git log' [<options>] [<revision range>...] [[--] <path>...] DESCRIPTION ----------- Shows the commit logs. -The command takes options applicable to the `git rev-list` +The command takes options applicable to the linkgit:git-rev-list[1] command to control what is shown and how, and options applicable to -the `git diff-*` commands to control how the changes +the linkgit:git-diff[1] family of commands to control how the changes each commit introduces are shown. @@ -90,9 +90,9 @@ include::line-range-format.txt[] <revision range> is specified, it defaults to `HEAD` (i.e. the whole history leading to the current commit). `origin..HEAD` specifies all the commits reachable from the current commit - (i.e. `HEAD`), but not from `origin`. For a complete list of - ways to spell <revision range>, see the 'Specifying Ranges' - section of linkgit:gitrevisions[7]. + (i.e. `HEAD`), but not from `origin`. You can give more than + one revision range. For a complete list of ways to spell <revision range>, + see the 'Specifying Ranges' section of linkgit:gitrevisions[7]. [--] <path>...:: Show only commits that are enough to explain how the files diff --git a/builtin/log.c b/builtin/log.c index 83a4a6188e2..51c2ef54823 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -53,7 +53,7 @@ static const char *fmt_patch_subject_prefix = "PATCH"; static const char *fmt_pretty; static const char * const builtin_log_usage[] = { - N_("git log [<options>] [<revision-range>] [[--] <path>...]"), + N_("git log [<options>] [<revision-range>...] [[--] <path>...]"), N_("git show [<options>] <object>..."), NULL };