Refactor: Parse environment variables into arrays

sandr01d commented 5 months ago

Check list

[X] I have performed a self-review of my code
[X] I have commented my code in hard-to-understand areas
[ ] I have made corresponding changes to the documentation

Description

Forgit allows specifying git options in environment variables that are then passed along to the individual git commands. We currently treat those as strings. This PR adds a _forgit_parse_array function and uses it to parse all such environment variables into arrays instead.

Rationale

This step is necessary to get rid of deferred code. Currently, the environment variables are space separated strings, which do get split by using eval. Parsing these variables into arrays allows us to get rid of the eval usage further down the line. As a positive side effect, it avoids unintended globbing and word splitting.

Type of change

[ ] Bug fix
[ ] New feature
[X] Refactor
[ ] Breaking change
[ ] Documentation change

Test environment

Shell
- [x] bash
- [X] zsh
- [ ] fish
OS
- [X] Linux
- [ ] Mac OS X
- [ ] Windows
- [ ] Others:

Note

This PR belongs to #324 and resulted from discussions in #326.

sandr01d commented 5 months ago

FYI: I removed the empty line at 120 that did not make sense anymore. Everything else is exactly as it is in #326.

carlfriedrich commented 5 months ago

Great, that's exactly how I meant it. 👍 Looking at this change it's obvious what it does, so approved content-wise.

One thing to improve, though: I would like to see the PR description in the commit message. Furthermore it should have a "Refactor: " prefix or something similar in the headline IMO.

And just to be sure: this PR does not change any behavior, does it? It really is pure refactoring, right?

carlfriedrich commented 5 months ago

And for completeness: I did a quick test with FORGIT_LOG_GIT_OPTS="--oneline --decorate" bin/git-forgit log on this branch using bash and it did what I expected, so I'm gonna check bash in the PR description.

I also added a link to the original issue #324 above and removed the "won't merge" part, because we will actually merge this, we just wait until the review and rebase in #326 is finished.

sandr01d commented 5 months ago

@carlfriedrich

One thing to improve, though: I would like to see the PR description in the commit message. Furthermore it should have a "Refactor: " prefix or something similar in the headline IMO.

Done

And just to be sure: this PR does not change any behavior, does it? It really is pure refactoring, right?

Yes, the behavior should be identical to what is was before.

removed the "won't merge" part, because we will actually merge this, we just wait until the review and rebase in https://github.com/wfxr/forgit/pull/326 is finished.

I'm a bit confused by that. I thought we would close down this PR and merge the commit with #326 or are we talking about the same thing here?

sandr01d commented 5 months ago

FYI: I will keep changes as commits around here to make the review process easier, but will squash everything from this PR together when I rebase #326.

carlfriedrich commented 5 months ago

Done

@sandr01d Great, thanks a lot!

Yes, the behavior should be identical to what is was before.

Great, then this is approved from my side.

I'm a bit confused by that. I thought we would close down this PR and merge the commit with #326 or are we talking about the same thing here?

I just re-read our thread in the other PR and noticed that I obviously got you wrong when I said "Exactly." 🙈 Sorry for that.

I imagine in the end we'll have something like this:

PR 1 (this one) contains commit A
PR 2 contains commits A & B
PR 3 contains commits A, B & C
PR 4 (the original one) contains commits A, B, C & D

Of course we could just merge PR 4 and close the others when we're finished. But we can as well merge all PRs sequentially in the end, one after another, so that effectively each PR only contains its dedicated commit afterwards. We just don't merge every PR right away, but instead wait until we have all PRs ready, so that we don't have to test each one individually. I'd consider that much cleaner. Got the idea?

That being said, I think in this case we actually could merge right away if we're sure that it does not change the behavior. What do you think?

sandr01d commented 5 months ago

I imagine in the end we'll have something like this:
PR 1 (this one) contains commit A
PR 2 contains commits A & B
PR 3 contains commits A, B & C
PR 4 (the original one) contains commits A, B, C & D
Of course we could just merge PR 4 and close the others when we're finished. But we can as well merge all PRs sequentially in the end, one after another, so that effectively each PR only contains its dedicated commit afterwards. We just don't merge every PR right away, but instead wait until we have all PRs ready, so that we don't have to test each one individually. I'd consider that much cleaner. Got the idea?

Gotcha now.

That being said, I think in this case we actually could merge right away if we're sure that it does not change the behavior. What do you think?

I think I would prefer to wait until we're done with the review process before we start merging anything. I don't see any benefit in merging it right away and #326 has seen way more testing by now, while the changes here are still pretty new. Also in case we decide to change an implementation detail in this PR further down the review process, not having this merged already will save us from additional rebasing.

sandr01d commented 5 months ago

@cjappl I've implemented the changes you requested, do you approve aswell?

cjappl commented 5 months ago

Yep approved! Thanks.

On Wed, Jan 31, 2024 at 10:36 AM, sandr01d @.***(mailto:On Wed, Jan 31, 2024 at 10:36 AM, sandr01d < wrote:

@.***(https://github.com/cjappl) I've implemented the changes you requested, do you approve aswell?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

carlfriedrich commented 5 months ago

I think I would prefer to wait until we're done with the review process before we start merging anything. I don't see any benefit in merging it right away and #326 has seen way more testing by now, while the changes here are still pretty new. Also in case we decide to change an implementation detail in this PR further down the review process, not having this merged already will save us from additional rebasing.

Alright, that sounds reasonable!

sandr01d commented 3 months ago

Changes have been merged with #326

wfxr / forgit