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

Ignore user pretty when generating diff patch log with <TAB> #54

Closed
wants to merge 3 commits into from

Conversation

faisal-shah
Copy link

This PR fixes #53 .

If the user has setup a custom pretty formatter (perhaps using
format.pretty as a default), it can break fugitive's ability to parse
the resulting log and extract the information it needs. This sets the
output to a standard format (--pretty=medium is git's default).
@rbong
Copy link
Owner

rbong commented Apr 25, 2021

This also appears to contain the changes for --author-date-order.

Besides that though, I'm not sure that this is a desirable change. Flog doesn't currently add any options to any of its default bindings which alters behaviour in a way that isn't directly required for the command it's trying to perform, and it's worked well so far.

Besides established precedent, why would I as a user expect to see anything different when Flog calls :Git show for me versus when I use :Git show manually? What if I care more about my own pretty format more than being able to jump between commits? After all, I'm the one who added the format configuration. If we added this option by default, and such a user submits a bug report saying they can't see the format they specified, that bug would be just as valid as this one.

IMO, even though I personally think being able to navigate between commits is more desirable, it should be on the user to fix it if it's the user's configuration causing the problem in cases like this. Flog provides the customizability to get around issues like this because it's nearly impossible to deal with every edge case.

My suggested fix is implementing a custom binding for <Tab>:

augroup flog
  autocmd FileType floggraph nno <buffer> <silent> <Tab> :<C-U>call flog#run_tmp_command('vertical belowright Git show --format=medium %h -- %p')<CR>
augroup END

However I'm still open for arguments as to why this should get in. It just doesn't mesh well with the philosophy of Flog.

@faisal-shah
Copy link
Author

This also appears to contain the changes for --author-date-order.

Sorry about that, if you end up wanting to merge this I'll clean it up.

Besides established precedent, why would I as a user expect to see anything different when Flog calls :Git show for me versus when I use :Git show manually? What if I care more about my own pretty format more than being able to jump between commits? After all, I'm the one who added the format configuration. If we added this option by default, and such a user submits a bug report saying they can't see the format they specified, that bug would be just as valid as this one.

One reason could be that the pretty formatter is assumed to be used for non-interactive output that git show/log/whatchanged generate.

Flog and Fugitive take that that output and make it interactive in ways useful to the user. If in the current state, neither are able to correctly parse the output, it makes sense to enforce a default formatting. Otherwise, what's the utility beyond !git show?

My suggested fix is implementing a custom binding for <Tab>:

augroup flog
  autocmd FileType floggraph nno <buffer> <silent> <Tab> :<C-U>call flog#run_tmp_command('vertical belowright Git show --format=medium %h -- %p')<CR>
augroup END

However I'm still open for arguments as to why this should get in. It just doesn't mesh well with the philosophy of Flog.

Thanks for the tip, and thanks for your work on this plugin!

@rbong
Copy link
Owner

rbong commented May 2, 2021

I've considering this and I've come to the conclusion that if we are going to make any kind of guarantee that behavior like this is going to work 100% of the time, it needs a proper system implemented. I won't get into it, but I don't think the command system, being as thin as it is now, can make guarantees like this. It's simply too dumb to do it.

The way it is now, if Fugitive wants to add certain global guarantees to commands like :Git show they can absolutely do so. But if not, we're not going to add that for them. Otherwise, there is a way around it that offers more control than a more strict system would allow. Fugitive is entirely relied on for the feature broken by this issue, so we are going to go with Fugitive's choice on this issue, which is to allow people to add their own arguments if they want. You could even create your own command if you want. All of the relevant workarounds still apply.

If commands are to become more reliable, it would be part of whatever comes after Flog, or a new breaking version. The plan would be to stop relying on Fugitive and git log entirely. I don't know if this is ever going to happen unless if Flog completely reaches its limits as a useful branch viewer for me and needs to be reconceptualized.

Thank you for the issue and the PR, you really have given me a lot to think about.

@rbong rbong closed this May 2, 2021
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.

Opening diff by <CR> on diff line when -path specified doesn't work
2 participants