vn971 / rua

Build tool for Arch Linux providing control, review and jailed build options
GNU General Public License v3.0
421 stars 38 forks source link

Allow more fine grained controls in diff #195

Closed samuelstjean closed 3 months ago

samuelstjean commented 1 year ago

Seems like the git diff command is hardcoded in the source file and also flushes out the git config options if I read it correctly. As everything is also sandboxed, we use a regular git diff, without any bells or whistle the user might have installed to pimp the command. It would be nice to have the option to pass additional arguments, like say --word-diff, which would make one life much easier to quickly see when the only change in a package is only a version bump as we could highlight only the small changes instead of the whole line.

This would also make it more obvious if the upstream source changes by say typosquatting the url with a new maintainer instead of checking again the whole line as git diff would tell me.

vn971 commented 11 months ago

Hi Samuel, unfortunately, I don't think there's a technical possibility to fit all the possible ways to show diffs in a way that would still be trivial for the user to access. (I wouldn't want to introduce more commands to show different kinds of diffs.)

If you have a proposal to always show diffs in a certain way, we could discuss / decide on that.

If the proposal is to add a +1 option then I'm sorry but I'll decline.

Thoughts? Is there any particular diff style that one could use as a meaningful default for all AUR packages?

By the way, sorry for the late reply. Apparently email notifications in github silently broke for me late last year, and I simply didn't noticed that new comments/issues were added. (At work I've been mostly using gitlab.)

samuelstjean commented 11 months ago

I'd guess a default to only show word diff would be better than a full line diff. Right now, one needs to check what changed in the line, while most of the time it is a simple version bump or a small url change to indicate the new version of the package.

This would directly show up, here is an example from git show and a custom pager, too fancy but it shows the idea of just showing what really changed instead of the lines that changed to the user. That would mean replacing the internal git diff with additional args for everyone though so you may have other people complaining they liked the old one better.

Capture d’écran du 2023-08-01 23-35-20

I was thinking it might also be an idea to copy the user .gitconfig locally when building, but that would kind of defeat the idea of build isolation in itself.

vn971 commented 11 months ago

I've tried different modes now, I'm a bit conflicted. git diff --word-diff=color is nice, but it doesn't work for color-blind people. git diff --word-diff, subjectively, looks unappealing to me. Raw git diff looks rough, but always readable.

I do subjectively like diff-highlight's output, but I don't know how to simply override it for one command and not for the whole system.. (And of course there's an infinite number of other alternatives, such as delta, diffr and many more.)

vn971 commented 11 months ago

@samuelstjean UPD: also, on some inputs, --word-diff=color gives absolutely terrible (unreadable) results.. E.g. a really long line with few parts in green and few parts in red. What you have on the screenshot above is definitely not just git diff --word-diff=color, I think you have something extra.

samuelstjean commented 11 months ago

that's true, I am using diff-so-fancy, but that's too advanced since you need to install it and everything. Since allowing to pass in extra options to git diff itself is too complicated/makes stuff more complex for nothing, then I guess we can just let this idea rest instead of risking breaking stuff after all.

vn971 commented 11 months ago

@samuelstjean Some configuration options could actually be passed through, such as the core.pager, which should be sufficient for e.g. diff-so-fancy I think. Git allows that via GIT_CONFIG_COUNT, GIT_CONFIG_KEY_<n>, GIT_CONFIG_VALUE_<n> (documented in man git-config)

samuelstjean commented 11 months ago

Does rua pass through env variable in the building process already or would that require something extra to be added on your side? It might be undesirable if a pkgbuild tries to set or overwrite new variables and they get passed through for the build isolation.

vn971 commented 11 months ago

@samuelstjean currently, environment variables are passed through to rua and then to the build process (makepkg and what makepkg invokes). The environment and other things like that cannot get back to rua or to your shell though, so the pass-through is single-directed.

Come to think of it, it could be possible to clean the environment inside the jail for better reproducibility.. But I'm not sure how much that might break stuff. This would probably be a separate discussion, I'm personally not sure about it yet.

vn971 commented 3 months ago

Everything configurable through environment variables is currently passed through, so I'll close the issue for now. At least until a clear proposal is reached on what we need to do and why.