Skip to content

fixes tinyproxy/tinyproxy#235 Basic realm string editable in config file#547

Merged
rofl0r merged 1 commit intotinyproxy:masterfrom
telekom-digioss:issues/235
Jul 14, 2024
Merged

fixes tinyproxy/tinyproxy#235 Basic realm string editable in config file#547
rofl0r merged 1 commit intotinyproxy:masterfrom
telekom-digioss:issues/235

Conversation

@gruummy
Copy link
Copy Markdown
Contributor

@gruummy gruummy commented Jul 9, 2024

Please excuse potenial basic errors in this change.
I tested it well.

... but it is my very first change on a "C" programm.

I implemented it exactly like described in your proposal with a new setting in the configfile

I also relocated the handler "HANDLE_FUNC (handle_basicauth)"
... the old position inside the file "conf.c" simply felt wrong ... for me.

For the same reason i relocated the line about "STDCONF (xtinyproxy, BOOL, handle_xtinyproxy)," to the "/* boolean arguments */" section.
.. i thought .. when there is a headline section for it, then the BOOL should be defined in the section for BOOL's

It would be great if this could be merged ... i would want to do a few more small changes to make it good usable in our support organisation

This pull request ... is a first starter for me ... and get in touch with you

@gruummy gruummy force-pushed the issues/235 branch 2 times, most recently from 2255d35 to ae6365c Compare July 9, 2024 22:23
@rofl0r
Copy link
Copy Markdown
Contributor

rofl0r commented Jul 10, 2024

I also relocated the handler "HANDLE_FUNC (handle_basicauth)"
... the old position inside the file "conf.c" simply felt wrong ... for me.

one PR, one change. for longterm maintenance it's crucial to have high quality changesets - that means commits that only change the bare minimum, and with good descriptive commit messages. random code snippet reorderings and changes to gitignore may be accepted, but as separarate PRs with separate discussion and a reasoning for why the change is desired. "feels wrong" is typically not a good reasoning to get a change accepted, though. i will now proceed to make a few inline comments in the code.

Comment thread docs/man5/tinyproxy.conf.txt.in Outdated
Comment thread etc/tinyproxy.conf.in Outdated
Comment thread etc/tinyproxy.conf.in Outdated
Comment thread etc/tinyproxy.conf.in Outdated
Comment thread src/conf.c
Comment thread src/conf.h Outdated
Comment thread src/html-error.c Outdated
@gruummy gruummy force-pushed the issues/235 branch 2 times, most recently from e933cd2 to f2074a3 Compare July 11, 2024 20:46
@gruummy
Copy link
Copy Markdown
Contributor Author

gruummy commented Jul 11, 2024

Hi @rofl0r
Now a second chance ... i cared about all you review comments, squashed all my commits ... and forced pushed again.

Many things should be much better now than the first push I presented to you.

Thank you for your helpful comments and patience.

Comment thread src/html-error.c Outdated
Comment thread src/html-error.c Outdated
Comment thread src/html-error.c
Comment thread src/html-error.c Outdated
Comment thread src/html-error.c Outdated
Comment thread .gitignore
@gruummy
Copy link
Copy Markdown
Contributor Author

gruummy commented Jul 12, 2024

@rofl0r

3rd attempt ?

I looked around in the code how the error handling was done in other sections of the code. and the following looked prettiest to me, so I have done it also this way

goto ERROR_EXIT;

Comment thread src/html-error.c Outdated
Comment thread src/conf.c Outdated
Comment thread src/html-error.c Outdated
@rofl0r rofl0r merged commit 73da8a3 into tinyproxy:master Jul 14, 2024
@telekom-digioss telekom-digioss deleted the issues/235 branch July 14, 2024 16:14
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