| Message ID | pull.599.git.1586308923544.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | revision: --include-diversions adds helpful merges | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This --include-diversions option could use a better name.
True, but I do not think of a better (or for that matter a worse)
one.
As a new feature, I think this is a reasonable thing to want,
especially it is in line with the push in the past few years to
treat the first parent history specially.
I wonder how this would interact with the ancestry-path option?
That one also, like the simplify-merges option, needs a limited
traversal, and if this new mode can do without a limited traversal
(in other words, the output can be done incrementally from the tip)
and achieve something similar to what these other options wanted to
show, that would be great.
Thanks.
On 4/7/2020 9:30 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> This --include-diversions option could use a better name. > > True, but I do not think of a better (or for that matter a worse) > one. > > As a new feature, I think this is a reasonable thing to want, > especially it is in line with the push in the past few years to > treat the first parent history specially. > > I wonder how this would interact with the ancestry-path option? > That one also, like the simplify-merges option, needs a limited > traversal, and if this new mode can do without a limited traversal > (in other words, the output can be done incrementally from the tip) > and achieve something similar to what these other options wanted to > show, that would be great. You're right. I briefly considered the --ancestry-path option before realizing that would get a huge set of commits (for example: every topic based on the branch after the pull request was merged). The --include-diversions works incrementally like simplified merges. Based on the implementation, it would not change the results when added to a --full-history query. This makes sense: a diversion would appear in the --full-history results, anyway. It is worth adding tests for the combination with --ancestry-path and --simplify-merges, as the --include-diversions option would add results to those queries. Thanks, -Stolee
On 2020-04-08 at 01:22:03, Derrick Stolee via GitGitGadget wrote: > The --simplify-merges option was introduced to remove these extra > merge commits. By noticing that the rewritten parents are reachable > from their first parents, those edges can be simplified away. Finally, > the commits now look like single-parent commits that are TREESAME to > their "only" parent. Thus, they are removed and this issue does not > cause issues anymore. However, this also ends up removing the commit > M from the history view! Even worse, the --simplify-merges option > requires walking the entire history before returning a single result. I was not aware --simplify-merges required walking the entire history. Now I know why my alias using it performs so poorly on $HUGE_REPOSITORY with thousands of extra backmerges at $DAYJOB. Thanks for teaching me something. > Many Git users are using Git alongside a Git service that provides > code storage alongside a code review tool commonly called "Pull > Requests" or "Merge Requests" against a target branch. When these > requests are accepted and merged, they typically create a merge > commit whose first parent is the previous branch tip and the second > parent is the tip of the topic branch used for the request. This > presents a valuable order to the parents, but also makes that merge > commit slightly special. Users may want to see not only which > commits changed a file, but which pull requests merged those commits > into their branch. In the previous example, this would mean the > users want to see the merge commit "M" in addition to the single- > parent commit "C". I should not hesitate to point out that this history is also true of the Git Project's repository, although of course the merges may be of less interest here. > In some sense, users are asking for the "first" merge commit to > bring in the change to their branch. As long as the parent order is > consistent, this can be handled with the following rule: > > Include a merge commit if it is not TREESAME to its first > parent, but is TREESAME to a later parent. > > I call such merge commits "diversions" because they divert the > history walk away from the first-parent history. As such, this > change adds the "--include-diversions" option to rev-list and log. > To test these options, extend the standard test example to include > a merge commit that is not TREESAME to its first parent. It is > surprising that that option was not already in the example, as it > is instructive. > > In particular, this extension demonstrates a common issue with file > history simplification. When a user resolves a merge conflict using > "-Xours" or otherwise ignoring one side of the conflict, they create > a TREESAME edge that probably should not be TREESAME. This leads > users to become frustrated and complain that "my change disappeared!" > In my experience, showing them history with --full-history and > --simplify-merges quickly reveals the problematic merge. As mentioned, > this option is expensive to compute. The --include-diversions option > _might_ show the merge commit (usually titled "resolving conflicts") > more quickly. Of course, this depends on the user having the correct > parent order, which is backwards when using "git pull". I can't comment on the contents of the patch, since I'm really not at all familiar with the revision machinery, but I do think this change is a good idea. I see this as a very common use case, and I think this commit message explains the rationale well. > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > Add a new history mode > > This --include-diversions option could use a better name. As we all know, I'm terrible at naming things, so I have no suggestions here. I'm happy with the name as it stands, but am of course open to other ideas. > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt > index bfd02ade991..0c878be94a9 100644 > --- a/Documentation/rev-list-options.txt > +++ b/Documentation/rev-list-options.txt > @@ -342,6 +342,12 @@ Default mode:: > branches if the end result is the same (i.e. merging branches > with the same content) > > +--include-diversions:: > + Include all commits from the default mode, but also any merge > + commits that are not TREESAME to the first parent but are > + TREESAME to a later parent. This mode is helpful for showing > + the merge commits that "first introduced" a change to a branch. I wasn't sure if this use of "TREESAME" would be confusing, but it looks like we already use it extensively throughout the documentation, so it's probably fine.
On 4/7/2020 9:39 PM, Derrick Stolee wrote: > On 4/7/2020 9:30 PM, Junio C Hamano wrote: >> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> This --include-diversions option could use a better name. >> >> True, but I do not think of a better (or for that matter a worse) >> one. Here are some alternative names: --audit-merges --audit-trunk --first-merges --subtle-merges --more-merges The "audit" name implies some of the intent: we are trying to audit which merge commits introduced these changes. The --audit-trunk implies we are using a trunk-based workflow where parent order is critical. However, "trunk" may be confusing when there are multiple long-lived branches. A "first merge" is confusing when we see a sequence of multiple diversion merges (I include a test with this exact situation in my next version.) "subtle" is a bit wishy-washy. "--more-merges" is not very specific. The way we are adding merges to the final result may not be the only way we want to add "more" merges in the future. So, I think "--audit-merges" is the best of these alternatives. I'd be happy to be overruled with a different option. Hopefully, these options inspire better ideas from the community. >> As a new feature, I think this is a reasonable thing to want, >> especially it is in line with the push in the past few years to >> treat the first parent history specially. >> >> I wonder how this would interact with the ancestry-path option? >> That one also, like the simplify-merges option, needs a limited >> traversal, and if this new mode can do without a limited traversal >> (in other words, the output can be done incrementally from the tip) >> and achieve something similar to what these other options wanted to >> show, that would be great. > > You're right. I briefly considered the --ancestry-path option before > realizing that would get a huge set of commits (for example: every > topic based on the branch after the pull request was merged). > > The --include-diversions works incrementally like simplified merges. > Based on the implementation, it would not change the results when > added to a --full-history query. This makes sense: a diversion would > appear in the --full-history results, anyway. > > It is worth adding tests for the combination with --ancestry-path > and --simplify-merges, as the --include-diversions option would > add results to those queries. My gitgitgadget PR [1] is updated with tests and some new logic to handle the new option along with --simplify-merges. The situation was a bit subtle, so my next version will include a significant update to the rev-list documentation under the "History Simplification" mode. I'll give things some time to calm on the name of the option before sending a v2. My v2 also includes adding a new object flag "DIVERSION" to track these commits from the TREESAME calculation through the simplify-merges logic. When I was adding a new flag, I realized that I already messed up the 32-bit alignment of "struct object" when adding the TOPO_ORDER flags. The parsed, type, and flags bitfields add up to 33 bits! A solution would include pulling the TOPO_ORDER_* flags to be bits 22 and 23 instead of 26 and 27, but that would collide with what is happening in builtin/show-branch.c. But then I saw the following comment in builtin/show-branch.c: /* * TODO: convert this use of commit->object.flags to commit-slab * instead to store a pointer to ref name directly. Then use the same * UNINTERESTING definition from revision.h here. */ Is anyone interested in tackling this problem? I don't see any test failures when I swap the TOPO_ORDER_ flag locations, but that might just mean that show-branch isn't tested enough. Thanks, -Stolee [1] https://github.com/gitgitgadget/git/pull/599
On Wed, Apr 08, 2020 at 01:22:03AM +0000, Derrick Stolee via GitGitGadget wrote: > The default file history simplification of "git log -- <path>" or > "git rev-list -- <path>" focuses on providing the smallest set of > commits that first contributed a change. The revision walk greatly > restricts the set of walked commits by visiting only the first > TREESAME parent of a merge commit, when one exists. This means > that portions of the commit-graph are not walked, which can be a > performance benefit, but can also "hide" commits that added changes > but were ignored by a merge resolution. > [...] Thanks for a really great description of the problem. Playing around with the patch, I found one curiosity. Try this: git log --graph --oneline origin -- GIT-VERSION-GEN >old git log --graph --oneline --include-diversions \ origin -- GIT-VERSION-GEN >new diff -u old new The first hunk has: @@ -70,6 +70,7 @@ * 20769079d2 Git 2.12-rc2 * 5588dbffbd Git 2.12-rc1 * 6e3a7b3398 Git 2.12-rc0 +* 0a45050a14 Merge branch 'rj/git-version-gen-do-not-force-abbrev' * a7659747c2 GIT-VERSION-GEN: do not force abbreviation length used by 'describe' * 8d7a455ed5 Start post 2.11 cycle * 454cb6bd52 Git 2.11 which makes sense. That merge brought in a7659747c2, and the other side hadn't touched it. But I can't tell from the output how the two are related. Nor can I just add "-p" to the invocation; after we've simplified, it has only a single parent, but it's TREESAME to that parent. So it has no diff. I actually think the most descriptive output here would be something like: * 6e3a7b3398 Git 2.12-rc0 * 0a45050a14 Merge branch 'rj/git-version-gen-do-not-force-abbrev' |\ | * a7659747c2 GIT-VERSION-GEN: do not force abbreviation length used by 'describe' |/ * Start post 2.11 cycle I.e., leaving both parents intact for a "diversion" merge. But maybe that would have secondary effects in other places. -Peff
Derrick Stolee <stolee@gmail.com> writes: > On 4/7/2020 9:39 PM, Derrick Stolee wrote: >> On 4/7/2020 9:30 PM, Junio C Hamano wrote: >>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: >>> >>>> This --include-diversions option could use a better name. >>> >>> True, but I do not think of a better (or for that matter a worse) >>> one. > > Here are some alternative names: > > --audit-merges > --audit-trunk > --first-merges > --subtle-merges > --more-merges > > The "audit" name implies some of the intent: we are trying to > audit which merge commits introduced these changes. The --audit-trunk > implies we are using a trunk-based workflow where parent order is > critical. However, "trunk" may be confusing when there are multiple > long-lived branches. Our features written with the intent to be useful for one purpose often end up being used for purposes other than what the feature was originally written for (the "--pickaxe" has always been a bitter example of this for me). For that reason, I am a bit hesitant to endorse "audit" exactly because of the implication of "intent". We usually try to give a single simplest history that explains how the current content in the path came to be. For that, commit M in the illustration in your original message does not really help when we ignore which parent chain owns the history, but in practice, many workflows treat the first parent chain as something special, so we want to show, not just the individual changes that matter, where the changes are introduced in the first-parent chain. I wonder if there is a simple-enough phrase to convey what the latter half of above sentence says. "include" and "keep" are both good verbs---normally we discard these merges, because they do not contribute at the level of individual changes, but with the option, we "include" or "keep" these merges in the output. It's not like we keep _all_ the merges, but selected merges only. How do we decide which merges to keep? I guess your "--first-merges" came from such a line of thought, and is the closest among the five to what I have in mind, but it drops too many words and loses too much meaning. "--keep-first-parent-merges", perhaps?
On Wed, Apr 08, 2020 at 12:46:41PM -0700, Junio C Hamano wrote: > Our features written with the intent to be useful for one purpose > often end up being used for purposes other than what the feature was > originally written for (the "--pickaxe" has always been a bitter > example of this for me). > > For that reason, I am a bit hesitant to endorse "audit" exactly > because of the implication of "intent". Yeah, I agree with this. > I wonder if there is a simple-enough phrase to convey what the > latter half of above sentence says. "include" and "keep" are both > good verbs---normally we discard these merges, because they do not > contribute at the level of individual changes, but with the option, > we "include" or "keep" these merges in the output. It's not like > we keep _all_ the merges, but selected merges only. How do we > decide which merges to keep? > > I guess your "--first-merges" came from such a line of thought, and > is the closest among the five to what I have in mind, but it drops > too many words and loses too much meaning. > > "--keep-first-parent-merges", perhaps? FWIW, this name left me more confused, because "first-parent merges" isn't an already-defined term I knew. And it seems like all merges have a first parent. Having read the patch description, I guess it's "a merge which isn't TREESAME to its first-parent". I can't think of a more succinct way to name that, though. And possibly if we gave that definition in the documentation, that would be enough. The name doesn't have to be a complete description; it only has to make sense once you know what you're trying to do (and be memorable enough). -Peff
On 4/8/2020 4:05 PM, Jeff King wrote: > On Wed, Apr 08, 2020 at 12:46:41PM -0700, Junio C Hamano wrote: > >> Our features written with the intent to be useful for one purpose >> often end up being used for purposes other than what the feature was >> originally written for (the "--pickaxe" has always been a bitter >> example of this for me). >> >> For that reason, I am a bit hesitant to endorse "audit" exactly >> because of the implication of "intent". > > Yeah, I agree with this. > >> I wonder if there is a simple-enough phrase to convey what the >> latter half of above sentence says. "include" and "keep" are both >> good verbs---normally we discard these merges, because they do not >> contribute at the level of individual changes, but with the option, >> we "include" or "keep" these merges in the output. It's not like >> we keep _all_ the merges, but selected merges only. How do we >> decide which merges to keep? >> >> I guess your "--first-merges" came from such a line of thought, and >> is the closest among the five to what I have in mind, but it drops >> too many words and loses too much meaning. >> >> "--keep-first-parent-merges", perhaps? > > FWIW, this name left me more confused, because "first-parent merges" > isn't an already-defined term I knew. And it seems like all merges have > a first parent. Having read the patch description, I guess it's "a merge > which isn't TREESAME to its first-parent". > > I can't think of a more succinct way to name that, though. And possibly > if we gave that definition in the documentation, that would be enough. > The name doesn't have to be a complete description; it only has to make > sense once you know what you're trying to do (and be memorable enough). Then I suppose we should focus on naming merge commits with this property: A merge commit that is not TREESAME to its first parent (but is TREESAME to a later parent). The part in parentheses may be optional, because a merge commit that is not TREESAME to any parent will be included by every history mode. In my latest attempt at documentation, I called these merges "diverters" yet still used "--include-diversions". Here are a few other words that we could use: * diverters or diversions * redirects * switches (think railroad switch). Synonym: exchange * detours The "switch" or "exchange" words are probably bad because they have noun _and_ verb forms. Or we could look again at the history results as a whole to find inspiration for the command-line option: * --merge-trail Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > Then I suppose we should focus on naming merge commits with this property: > > A merge commit that is not TREESAME to its first parent (but is TREESAME > to a later parent). > > The part in parentheses may be optional, because a merge commit that is > not TREESAME to any parent will be included by every history mode. A merge that is TREESAME to its first parent does not introduce anything new to the mainline (as far as the paths that match the pathspec are concerned). We are trying to find names to call merges that are not those no-op merges. Hmph... > In my latest attempt at documentation, I called these merges "diverters" > yet still used "--include-diversions". Here are a few other words that we > could use: > > * diverters or diversions > * redirects > * switches (think railroad switch). Synonym: exchange > * detours ...none of the above tells me that they are not no-op (in other words, they do something meaningful), so I must be coming from a direction different from you are. What redirects from what other thing, for example?
On 4/8/2020 5:35 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >> Then I suppose we should focus on naming merge commits with this property: >> >> A merge commit that is not TREESAME to its first parent (but is TREESAME >> to a later parent). >> >> The part in parentheses may be optional, because a merge commit that is >> not TREESAME to any parent will be included by every history mode. > > A merge that is TREESAME to its first parent does not introduce > anything new to the mainline (as far as the paths that match the > pathspec are concerned). We are trying to find names to call merges > that are not those no-op merges. Hmph... There are three situations for a merge commit: 1. TREESAME to _all_ parents. These are not included. 2. not TREESAME to _all_ parents. These are already included. 3. TREESAME to some, but not TREESAME to others. The third mode is the one that default mode will drop, but --full-history will include. The new mode will include some of these (the ones that are NOT TREESAME to their first parent). >> In my latest attempt at documentation, I called these merges "diverters" >> yet still used "--include-diversions". Here are a few other words that we >> could use: >> >> * diverters or diversions >> * redirects >> * switches (think railroad switch). Synonym: exchange >> * detours > > ...none of the above tells me that they are not no-op (in other > words, they do something meaningful), so I must be coming from > a direction different from you are. What redirects from what other > thing, for example? The merges do something meaningful: they "merge in" a "real" change. I'll just submit my v2 as-is, which includes a significant change to the documentation that should make things more clear. Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: >>> In my latest attempt at documentation, I called these merges "diverters" >>> yet still used "--include-diversions". Here are a few other words that we >>> could use: >>> >>> * diverters or diversions >>> * redirects >>> * switches (think railroad switch). Synonym: exchange >>> * detours >> >> ...none of the above tells me that they are not no-op (in other >> words, they do something meaningful), so I must be coming from >> a direction different from you are. What redirects from what other >> thing, for example? > > The merges do something meaningful: they "merge in" a "real" change. Yes, but "redirect", "switch", "detour", or "divert" do not quite mean "merging in a real change", at least to me. > I'll just submit my v2 as-is, which includes a significant change to > the documentation that should make things more clear. Thanks.
On 4/8/2020 8:08 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >>>> In my latest attempt at documentation, I called these merges "diverters" >>>> yet still used "--include-diversions". Here are a few other words that we >>>> could use: >>>> >>>> * diverters or diversions >>>> * redirects >>>> * switches (think railroad switch). Synonym: exchange >>>> * detours >>> >>> ...none of the above tells me that they are not no-op (in other >>> words, they do something meaningful), so I must be coming from >>> a direction different from you are. What redirects from what other >>> thing, for example? >> >> The merges do something meaningful: they "merge in" a "real" change. > > Yes, but "redirect", "switch", "detour", or "divert" do not quite > mean "merging in a real change", at least to me. Makes sense to me. The way you explain why certain words don't work for you helps me think of new words to describe these merges: * signposts * guides * signals For the argument, we cwould add "-merge" to each of these, such as "--signpost-merges" or "--signal-merges". I'm going to keep replying to this thread with ideas until someone says "This one makes sense to me" or an equivalent. Alternatively, someone else could present an idea and then I get to say "Aha! That captures this concept clearly with an obvious metaphor!" Thanks, -Stolee
Hi On 09/04/2020 01:08, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >>>> In my latest attempt at documentation, I called these merges "diverters" >>>> yet still used "--include-diversions". Here are a few other words that we >>>> could use: >>>> >>>> * diverters or diversions >>>> * redirects >>>> * switches (think railroad switch). Synonym: exchange >>>> * detours >>> ...none of the above tells me that they are not no-op (in other >>> words, they do something meaningful), so I must be coming from >>> a direction different from you are. What redirects from what other >>> thing, for example? >> The merges do something meaningful: they "merge in" a "real" change. > Yes, but "redirect", "switch", "detour", or "divert" do not quite > mean "merging in a real change", at least to me. > >> I'll just submit my v2 as-is, which includes a significant change to >> the documentation that should make things more clear. > Thanks. Can I suggest "--side-merges" as a possible descriptor for these non-mainline diversions? My thesaurus had suggested detour and sidetracked, which led to the side-merge view. Philip
Philip Oakley <philipoakley@iee.email> writes: >> Yes, but "redirect", "switch", "detour", or "divert" do not quite >> mean "merging in a real change", at least to me. >> >>> I'll just submit my v2 as-is, which includes a significant change to >>> the documentation that should make things more clear. >> Thanks. > Can I suggest "--side-merges" as a possible descriptor for these > non-mainline diversions? > > My thesaurus had suggested detour and sidetracked, which led to the > side-merge view. Ahh, sorry Derrick for being slow and thanks Philip for repeating "diversion", as the word did not click for me at all when I saw the patch and wrote my response. But I think it started slowly to dawn on me. Does it come from the worldview where we want to follow the "trunk" but because when we notice at a merge that we got everything that matters to us from a side branch, we switch the track out of the mainline and from then on follow that side branch? Switching the track and following the side branch happens silently with the default "history simplification", but the new feature shows where that side-tracking happens more prominently---is that where the words "divert" etc. come from? Then I can understand how these candidate words may have place in describing the situation we want to use the feature; I am not yet convinced any of the concrete option names floated on the thread (or what I can come up with right now) would be clear to our target audiences, but at least I am not as confused as I was before. Thanks.
On 4/9/2020 11:56 AM, Junio C Hamano wrote: > Philip Oakley <philipoakley@iee.email> writes: > >>> Yes, but "redirect", "switch", "detour", or "divert" do not quite >>> mean "merging in a real change", at least to me. >>> >>>> I'll just submit my v2 as-is, which includes a significant change to >>>> the documentation that should make things more clear. >>> Thanks. >> Can I suggest "--side-merges" as a possible descriptor for these >> non-mainline diversions? >> >> My thesaurus had suggested detour and sidetracked, which led to the >> side-merge view. > > Ahh, sorry Derrick for being slow and thanks Philip for repeating > "diversion", as the word did not click for me at all when I saw the > patch and wrote my response. > > But I think it started slowly to dawn on me. > > Does it come from the worldview where we want to follow the "trunk" > but because when we notice at a merge that we got everything that > matters to us from a side branch, we switch the track out of the > mainline and from then on follow that side branch? Switching the > track and following the side branch happens silently with the > default "history simplification", but the new feature shows where > that side-tracking happens more prominently---is that where the > words "divert" etc. come from? > > Then I can understand how these candidate words may have place in > describing the situation we want to use the feature; I am not yet > convinced any of the concrete option names floated on the thread (or > what I can come up with right now) would be clear to our target > audiences, but at least I am not as confused as I was before. After thinking about all the great responses here, and having a chat with Dscho about this, then taking a break, I had an "Aha!" moment. We should call this option: --show-pulls The direction here is important. Let's look at a potential "git log --graph --oneline" output to explore this idea: * (master) A |\ | * (feature) B | |\ | | * (topic) C | | |\ | | | | | | * | D | | | | | * | | E | | | | * | | | F | |_|/ |/| | * | | G |/ / * / H |/ * I I use (master), (feature), and (topic) to decorate branches that are updated only by "git commit" or "git pull". The file 'foo' was created by commit I. In this graph, the single-parent commits G and D change 'foo'. The commit G enters master using "git commit". The commit G enters topic using "git pull" starting from D. The developer on that branch resolves conflicts by taking the version of 'foo' from D. Thus C is TREESAME to D but not G. The commit B is created by running "git pull topic" from the feature branch. The commit A is created by running "git pull feature" from the master branch. Thus, A and B "pulled" the change into their branches. The commit C "pulled" G into the branch, but did not keep the change to 'foo'. Thus 'git log --graph --oneline master -- foo' would output: * D * I 'git log --graph --oneline --show-pulls master -- foo' shows: * A * B * D * I 'git log --graph --oneline --full-history -- foo' shows: * (master) A |\ | * (feature) B | |\ | | * (topic) C | | |\ | | | | | | * | D | |_|/ |/| | * | | G |/ / | / |/ * I 'git log --graph --oneline --full-history --simplify-merges -- foo' would show: * C |\ * | D | | | * G |/ * I 'git log --graph --oneline --full-history --simplify-merges --show-pulls -- foo' would show: * A * B * C |\ * | D | | | * G |/ * I In conclusion, I think "--show-pulls" provides the right context for these extra merges to show in the history view. It also roots these merges in a Git-native name (that also happens to evoke the "pull request" concept that is _not_ native to Git). What do you think? Thanks, -Stolee
On Thu, Apr 09, 2020 at 01:20:57PM -0400, Derrick Stolee wrote: > In conclusion, I think "--show-pulls" provides the right context for these > extra merges to show in the history view. It also roots these merges in a > Git-native name (that also happens to evoke the "pull request" concept that > is _not_ native to Git). > > What do you think? Yeah, after reading more of the thread, I think the simplest way to think about is "keep merges that pulled in something" with the implication of "(even if the other side didn't touch anything)". And "something you pulled" is a sensible way to think of that. So --show-pulls makes sense to me. Or if we really want to tie it in to simplification, --no-simplify-pulls. But that's more awkward to type, and none of the existing simplification options use the word simplify. ;) -Peff
Jeff King <peff@peff.net> writes: > On Thu, Apr 09, 2020 at 01:20:57PM -0400, Derrick Stolee wrote: > >> In conclusion, I think "--show-pulls" provides the right context for these >> extra merges to show in the history view. It also roots these merges in a >> Git-native name (that also happens to evoke the "pull request" concept that >> is _not_ native to Git). >> >> What do you think? > > Yeah, after reading more of the thread, I think the simplest way to > think about is "keep merges that pulled in something" with the > implication of "(even if the other side didn't touch anything)". Isn't it more like "even if our side didn't touch anything", though? If a merge pulled in something, the other side by definition did something (i.e. what was pulled in); if we did something since they forked, we would have shown the merge without this patch---the only new behaviour we are adding is to show the merge even when our side didn't touch since they forked---so far we never showed that merge, but now with this option we would when we are asked to. I agree that "this is showing pulls" is an easy way to explain. > And "something you pulled" is a sensible way to think of that. So > --show-pulls makes sense to me. Or if we really want to tie it in to > simplification, --no-simplify-pulls. But that's more awkward to type, > and none of the existing simplification options use the word simplify. ;) ;-)
On Thu, Apr 09, 2020 at 11:55:51AM -0700, Junio C Hamano wrote: > > On Thu, Apr 09, 2020 at 01:20:57PM -0400, Derrick Stolee wrote: > > > >> In conclusion, I think "--show-pulls" provides the right context for these > >> extra merges to show in the history view. It also roots these merges in a > >> Git-native name (that also happens to evoke the "pull request" concept that > >> is _not_ native to Git). > >> > >> What do you think? > > > > Yeah, after reading more of the thread, I think the simplest way to > > think about is "keep merges that pulled in something" with the > > implication of "(even if the other side didn't touch anything)". > > Isn't it more like "even if our side didn't touch anything", though? I meant the _other_ other. :) I.e., the other one that is not what just got pulled in. Which is the first parent. ;) So yes, I think we are on the same page, and I just said it badly. Using "our side" is better than trying to double-negate "other". -Peff
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index bfd02ade991..0c878be94a9 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -342,6 +342,12 @@ Default mode:: branches if the end result is the same (i.e. merging branches with the same content) +--include-diversions:: + Include all commits from the default mode, but also any merge + commits that are not TREESAME to the first parent but are + TREESAME to a later parent. This mode is helpful for showing + the merge commits that "first introduced" a change to a branch. + --full-history:: Same as the default mode, but does not prune some history. diff --git a/revision.c b/revision.c index 8136929e236..915d8febdc4 100644 --- a/revision.c +++ b/revision.c @@ -870,7 +870,19 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit) } parent->next = NULL; commit->parents = parent; - commit->object.flags |= TREESAME; + + /* + * A merge commit is a "diversion" if it is not + * TREESAME to its first parent but is TREESAME + * to a later parent. In the simplified history, + * we "divert" the history walk to the later + * parent. These commits are shown when "diversions" + * is enabled, so do not mark the object as + * TREESAME here. + */ + if (!revs->diversions || !nth_parent) + commit->object.flags |= TREESAME; + return; case REV_TREE_NEW: @@ -2265,6 +2277,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg } else if (!strcmp(arg, "--full-diff")) { revs->diff = 1; revs->full_diff = 1; + } else if (!strcmp(arg, "--include-diversions")) { + revs->diversions = 1; } else if (!strcmp(arg, "--full-history")) { revs->simplify_history = 0; } else if (!strcmp(arg, "--relative-date")) { diff --git a/revision.h b/revision.h index 475f048fb61..f06a73cbcd8 100644 --- a/revision.h +++ b/revision.h @@ -129,6 +129,7 @@ struct rev_info { no_walk:2, remove_empty_trees:1, simplify_history:1, + diversions:1, topo_order:1, simplify_merges:1, simplify_by_decoration:1, diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh index a10f0df02b0..9c91226f737 100755 --- a/t/t6012-rev-list-simplify.sh +++ b/t/t6012-rev-list-simplify.sh @@ -154,4 +154,48 @@ test_expect_success '--full-diff is not affected by --parents' ' test_cmp expected actual ' +# +# Modify the test repo to add a merge whose first parent is not TREESAME +# but whose second parent is TREESAME +# +# A--B----------G--H--I--K--L--N +# \ \ / / / +# \ \ / / / +# C------E---F J / +# \ \_/ / +# \ / +# M----------------- +test_expect_success 'expand graph' ' + git switch -c branchM C && + echo "new data" >file && + git add file && + test_tick && + test_commit M && + + git checkout master && + git merge -Xtheirs branchM -m "N" && + note N +' + +check_result 'M C A' -- file +check_result 'N M C A' --include-diversions -- file + +check_result 'N M L K J I H F E D C G B A' --full-history --topo-order +check_result 'N M L K I H G F E D C B J A' --full-history +check_result 'N M L K I H G F E D C B J A' --full-history --date-order +check_result 'N M L K I H G F E D B C J A' --full-history --author-date-order +check_result 'N M K I H E C B A' --full-history -- file +check_result 'N M K I H E C B A' --full-history --topo-order -- file +check_result 'N M K I H E C B A' --full-history --date-order -- file +check_result 'N M K I H E B C A' --full-history --author-date-order -- file +check_result 'N M I E C B A' --simplify-merges -- file +check_result 'N M I E C B A' --simplify-merges --topo-order -- file +check_result 'N M I E C B A' --simplify-merges --date-order -- file +check_result 'N M I E B C A' --simplify-merges --author-date-order -- file +check_result 'M C A' --topo-order -- file +check_result 'M C A' --date-order -- file +check_result 'M C A' --author-date-order -- file +check_result 'H' --first-parent -- another-file +check_result 'H' --first-parent --topo-order -- another-file + test_done