Proposed Project: overhaul user-facing messages

[blairconrad]

Motivation

The git-absorb user messages (essentially log messages at INFO, WARN, or CRIT levels) do not always present the best information to the user, especially when staged changes remain after running the tool. This can be seen in issues such as #9, #21, #46, and more.

Sometimes the user is left not knowing what to do with unabsorbed changes, and sometimes they’re given repetitive messages or messages that won’t help them.

I’d like to work on that.

Goals

Improve user-visible messages so that git-absorb

• Doesn’t repeat messages without supplying additional information
• Emits only actionable warning messages (as suggested in the TODO at the bottom of the README)
• Doesn’t warn about conditions that don’t affect outcome (e.g. if all changes are found 3 commits from HEAD, warning about a commit 5 down from HEAD)
• Doesn’t warn about a condition when a more serious one was found (e.g, suggesting increasing the stack limit if we’ve already hit a merge commit)
• Suggests a path forward when changes could not be applied to commits

Proposed plan

@tummychow, if it suits you, I’d like to try to achieve these goals, probably in a few PRs, since the plan would probably be a little big to digest (or produce) in one go. Anyhow, I’m suggesting to

1. Capture INFO and worse log output in the lib.rs tests, to characterize the state of things now
2. Add additional tests at this level, to cover the various situations that can cause a change to not be absorbed.
   See below for my notes on the factors that contribute to finding candidate commits and to the messages that users see. There are going to be a few combinations that need to be tested. I haven't performed an exhaustive survey of what's already covered, but I wouldn't be surprised if the number of new tests was near a dozen.
3. Adjust stack::working_stack to also return a reason why the commit-finding search ended (e.g. reached a merge commit, hit the stack depth limit, etc.)
4. Move user-facing logging out of stack.rs and use the reasons found above to help the lib.rs code present better messages to the user

If you hate this, please tell me. If you’re unsure and figure you’ll know better if you see it in flight, I can proceed anyhow. It won’t bother me if I code up some changes and throw them away.

---

Supporting Information

Factors that limit stack discovery

(Casual readers, here stack refers to the straight line of commits from HEAD through the first parents that will be considered as targets to fix up. Taken from stack::working_stack in the source.)

• HEAD; you have to start somewhere
• Merge commits always stop stack discovery; we cannot fix up a merge commit or any ancestors
• Commits by other authors (or their ancestors) are not fixed up by default. Config exists to enable fixup. If enabled, does not affect stack generation or messages seen by the user. Prefer warnings about merge commits.
• User-supplied base or any ancestor commit stops stack discovery. Prefer warnings about merge commits.
• Stack depth limit; only applies when no user-supplied base. Commits past this limit will not be considered. Prefer warnings about merge commits. That is, if the first commit excluded due to depth is a merge commit, tell the user it’s a merge commit instead of suggesting to increase the stack limit. See also “Non-HEAD branches” below
• Non-HEAD branches and their ancestors will, when there’s no user-supplied base, stop stack discovery just as the user-supplied base would. As with user-supplied base, prefer warnings about merge commits
• The repository root will stop stack discovery. As with merge commits, there’s no recourse here, so prefer to warn about this over anything else.

Messages to the user are also affected by

In addition to the commit stack that’s discovered, the messages that users see should (or are) also be influenced by

• Whether HEAD is detached. A detached head will usually prohibit absorption. If configured to allow absorption, it will emit a message but not otherwise affect stack discovery
• The presence of a change that could not be fixed up to a commit in the stack (Add instructions on what to do when there are still staged changes after running git absorb #21)
• Whether such a change is a non-modification change (addition or removal of file) ("No additions staged" when a new file is added #46)
• Whether such a non-modification change was staged due to the auto-stage functionality (if we auto-staged an untracked file and it doesn’t fix up any commit from the stack, don’t bother the user about it)

[tummychow]

yes, this is good work to do.

i think your points 3&4 (moving logging out of stack.rs) are a good place to start. i'm generally skeptical of tests for side effects like logs and metrics (because it's such a nuisance to keep them up to date), but i could change my mind on this especially in targeted circumstances (ie, testing for the presence or absence of specific log lines rather than testing that a given situation results in a specific exact sequence of log lines). i would say to do some of the implementation first and then consider what log lines are actually important enough to test.

one other thing: most of the work in this kind of project tends to be deciding on the desired behavior, not actually implementing it. the supporting information you have above is a good start. i'd suggest making a table or flowchart so that it's easier to visualize and discuss the changes along the way

[blairconrad]

Thanks for the response, @tummychow.

i'd suggest making a table or flowchart

Confession time. I'd sort of done. Well, informally. It contributed to the notes upstairs and my overall thoughts that a larger change was probably appropriate. Crystalizing it is a good idea though. So here goes. Warning: I took "flowchart" in school over 40 years ago, so am probably abusing symbols. But this should be the gist. I've broken out flows for constructing the stack and absorbing the changes, focusing on how we get what output. I've ignored trivialities like "successful absorption" and special messages about hunks that commute with everything (which I'd probably leave to a later phase or issue anyhow, given my druthers).

Oh, note that I left "detached head" detection in the stack calculation, but it feels to me like it is a lib.rs concern; almost a precondition to whether we want to begin working.

flowchart TD
    RunWithRepo([run_with_repo]) --> CallWorkingStack[[working_stack]]
    CallWorkingStack --> TryFixup{Loop Over
    Commits and
    Hunks}
    TryFixup -->|unabsorbed| StackStopReason{Reason Stack
    Stopped}
    TryFixup -->|all absorbed| Party([Party! 🥳])
    StackStopReason -->|Reached Root| StackReachedRoot[/reached root/]
    StackStopReason -->|Reached Merge| StackReachedMerge[/reached merge/]
    StackStopReason -->|Reached Other Author| StackReachedOtherAuthor[/reached other
    author/]
    StackStopReason -->|Exceeded Stack Limit| IncreaseLimit[/increase limit/] --> TryByHand
    StackStopReason -->|Hidden Commits| IsExplicitBase{user-defined base?}
    IsExplicitBase -->|yes| TryDifferentBase[/try a different base/]
    IsExplicitBase -->|no| HiddenByBranches[/commits hidden by
    other branches/]
    StackReachedRoot --> TryByHand
    StackReachedMerge --> TryByHand
    StackReachedOtherAuthor --> TryByHand
    TryDifferentBase --> TryByHand
    HiddenByBranches --> TryByHand

    TryByHand[/Unabsorbed changes.
    Try manual fixup./] --> done([done])

    WorkingStack([working_stack]) --> IsDetached
    IsDetached{is head
    detached?} -->|yes| Detached{allowed?} -->|no| ThrowErr([Error 💣]) 
    Detached -->|yes| UB1{User-Defined
    Base?}
    UB1 -->|yes| HideBase{Hide Base}
    UB1 -->|no| HideBranches{Hide Other
    Branches}
    HideBranches --> Walk{walk first
    parents}
    HideBase --> Walk
    Walk -->|1 found merge| ReturnMerge([Merge Commit])
    Walk -->|2 found forbidden author| ReturnForbiddenAuthor([Forbidden Other Author])
    Walk -->|3 exceeded stack limit| ReturnExceededLimit([Exceeded Stack Limit])
    Walk -->|4 ran out of commits| CheckNextCommit{Check Next
    Commit}
    CheckNextCommit -->|1 none| ReachedRoot([Reached Root])
    CheckNextCommit -->|2 merge| ReturnMerge
    CheckNextCommit -->|3 forbidden author| ReturnForbiddenAuthor
    CheckNextCommit -->|4 just a commit| ReturnHiddenByBase([Hidden Commits])

Loading

(mermaid reordered my nodes for me, so the numbers in the decision branches indicate order of preference. e.g. we check for merge commits before forbidden author before exceeding the stack limit)

[blairconrad]

[Ugh. This was sitting in draft for a day, with me thinking I'd posted.]

Now I'm going to be (or make your life) a little difficult! 😁

i think your points 3&4 (moving logging out of stack.rs) are a good place to start

Could do, and it's certainly easy to make some of these adjustments (I have a WIP), but I perennially worry about unexpected changes to observed behaviour, so would prefer to add characterization tests first. I won't insist of course (even if I were inclined, how could I? this is your project).

i'm generally skeptical of tests for side effects like logs and metrics (because it's such a nuisance to keep them up to date), but i could change my mind on this especially in targeted circumstances (ie, testing for the presence or absence of specific log lines rather than testing that a given situation results in a specific exact sequence of log lines)

Heard. And I generally agree about testing logs and metrics. In this case I'd make an exception because the logs (at INFO, WARN, and above, at least) are the user interface. They are going to shape the users' experience, ultimately leading them down the path of success or into the underbrush of confusion.
I think in this case we want to examine those lines quite closely. Not doing so can result in surprises like inconsistent lines or duplication. Basically I think that the deviations that crop up are likely to be so unanticipated that looser tests for inclusion or exclusion will have a relatively high false negative rate.

i would say to do some of the implementation first and then consider what log lines are actually important enough to test.

Prudent. If I may continue to be bold, I'll interpret this as "test for all the INFO+ logs, if only to show us what we have today". Then we can see the output and adjust the tests based on your assessment. Depending on how that goes, I'd even consider keeping the strict tests for at least as long as this sub-project continues. It probably won't be months or anything, and difficulties in maintaining the tests during this phase are probably going to annoy me most of all (and I have high tolerance for this sort of thing). Also I have already implemented the log-tester and adjusted existing tests to incorporate it. (I did a lot of exploration before opening this issue.)

So I guess all that is a way of saying "I understand your concerns, and why you might be a little nervous about my proposed intensity and order of implementation, but maybe give me a little leeway to try and we'll see if it works?". If you don't mind, I'd like to submit PRs roughly in the order of my 1 2 3 4 points above. As we go, we can refine and merge them. Or leave unmerged and I can extend and/or create stacked PRs for continued evaluation.

[tummychow]

the flowchart looks like a reasonable plan to me. re testing logs - if you have the pr for point1 already then yeah just submit it and let's discuss there

[blairconrad]

Thanks, @tummychow. I'm actually closing in on a PR that addresses all 4 points. I was going to take another pass to check formatting, consistent terminology and overall wording of messages before subjecting you to it, but #175 has parts 1 and 2 and I think is about as fit for eyeballs as I'm likely to make it.

If you want a preview, the WIP I have is at https://github.com/blairconrad/git-absorb/tree/better-logs.