Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle a mode change on a renamed file. #875

Merged
merged 2 commits into from
Jan 5, 2022

Conversation

WayneD
Copy link
Contributor

@WayneD WayneD commented Dec 30, 2021

I changed the diff parsing to cache the mode info from the old/new mode lines until the parsing bumps into the start of the actual related diff, finds the diff line for an unrelated diff, or runs out of input. This allows the mode info to be output in conjunction with a file event instead of as a separate heading (that used to have an empty name when the mode change was for a rename).

The mode info is passed down into the draw routines as a separate "addendum" string that the draw code can use as it likes. Currently that means that it appends the addendum string to the non-raw string in parens. I imagine that future draw routines could decide to put it in a separate box or some other per-routine method. There is currently a single function in src/handlers/draw.rs that joins the strings in the same way for everyone.

A couple examples of how the new code looks:

Δ foo.rs (mode +x)
───────────────────────────────────────────────────
renamed: old-longer-name 🡒 shorter-name (mode +x)
───────────────────────────────────────────────────

Would it look better on its own line?

Δ foo.rs
• mode +x
───────────────────────────────────────────────────
renamed: old-longer-name 🡒 shorter-name
• mode +x
───────────────────────────────────────────────────

Would it look better appended after a "•" character?

Δ foo.rs • mode +x
───────────────────────────────────────────────────
renamed: old-longer-name 🡒 shorter-name • mode +x
───────────────────────────────────────────────────

Should it be a user option? If so, we can do that later.

@WayneD WayneD force-pushed the mode-with-rename branch 3 times, most recently from 9569334 to ac8e4a3 Compare December 31, 2021 17:18
@dandavison
Copy link
Owner

dandavison commented Jan 1, 2022

Hi @WayneD, thanks for working on this!

I haven't thought about how this should work -- can I check, is this the intended behavior?

git mv src/delta.rs src/delta.1.rs
chmod +x src/delta.1.rs
git add src/
git diff --cached | delta --no-gitconfig

src/delta.rs: mode +x
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

renamed: src/delta.rs ⟶   src/delta.1.rs
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

@WayneD
Copy link
Contributor Author

WayneD commented Jan 2, 2022

I haven't thought about how this should work -- can I check, is this the intended behavior?

Yeah, that's what I ended up with given the current way the parsing works. My thoughts on an alternative method that delays the mode info pending more parsing would allow the 2 bits of info to be output in a more unified manner. Combined with your thoughts on wanting improved prefix handling, I think it would be better to go with the delayed-mode method (which should mean that the diff line is only ever used when no renaming is going on, and thus the current splitting function wouldn't need to change).

@dandavison
Copy link
Owner

that's what I ended up with given the current way the parsing works. My thoughts on an alternative method that delays the mode info pending more parsing would allow the 2 bits of info to be output in a more unified manner. Combined with your thoughts on wanting improved prefix handling, I think it would be better to go with the delayed-mode method

That sounds good. I agree that, seeing as you're working on this, it makes sense to try to achieve an output format that you're really happy with. What do you think, something like this maybe? (This is just the first thing that occurred to me; I haven't thought through the problem/solution space properly.)

renamed: src/delta.rs ⟶ src/delta.1.rs (mode +x)

@WayneD WayneD force-pushed the mode-with-rename branch 2 times, most recently from 3453ac7 to 9de7127 Compare January 3, 2022 20:50
I changed the diff parsing to cache the mode info from the old/new mode
lines until the parsing bumps into the start of the actual related diff,
finds the diff line for an unrelated diff, or runs out of input.  This
allows the mode info to be output in conjunction with a file event
instead of as a separate heading (that used to have an empty name when
the mode change was for a rename).

The mode info is passed down into the draw routines as a separate
"addendum" string that the draw code can use as it likes. Currently that
means that it appends the addendum string to the non-raw string in
parens. I imagine that future draw routines could decide to put it in a
separate box or some other per-routine method. There is currently a
single function in src/handlers/draw.rs that joins the strings in the
same way for everyone.

A couple examples of how the new code looks:

Δ foo.rs (mode +x)
───────────────────────────────────────────────────

renamed: old-longer-name 🡒 shorter-name (mode +x)
───────────────────────────────────────────────────

Would it look better on its own line?

Δ foo.rs
• mode +x
───────────────────────────────────────────────────

renamed: old-longer-name 🡒 shorter-name
• mode +x
───────────────────────────────────────────────────

Would it look better appended after a "•" character?

Δ foo.rs • mode +x
───────────────────────────────────────────────────

renamed: old-longer-name 🡒 shorter-name • mode +x
───────────────────────────────────────────────────

Should it be a user option? If so, we can do that later.
@WayneD
Copy link
Contributor Author

WayneD commented Jan 4, 2022

I fixed the conflicts with the DeltaTest strings now that the delta-test branch has merged. You'll note that my latest version uses parentheses to display the mode info, which I thought was a good choice in the various output idioms. It should be ready to merge now, IMHO.

@dandavison dandavison merged commit c76a26b into dandavison:master Jan 5, 2022
@dandavison
Copy link
Owner

Excellent, thanks a lot for this work @WayneD!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants