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

Refactor: Parse environment variables into arrays #338

Closed
wants to merge 2 commits into from

Conversation

sandr01d
Copy link
Collaborator

@sandr01d sandr01d commented Jan 29, 2024

Check list

  • I have performed a self-review of my code
  • 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
  • Refactor
  • Breaking change
  • Documentation change

Test environment

  • Shell
    • bash
    • zsh
    • fish
  • OS
    • Linux
    • Mac OS X
    • Windows
    • Others:

Note

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

@sandr01d
Copy link
Collaborator Author

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

@sandr01d sandr01d mentioned this pull request Jan 29, 2024
15 tasks
@carlfriedrich
Copy link
Collaborator

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
Copy link
Collaborator

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.

bin/git-forgit Outdated Show resolved Hide resolved
@sandr01d sandr01d changed the title Parse environment variables into arrays Refactor: Parse environment variables into arrays Jan 30, 2024
Forgit allows specifying git options in environment variables that are passed along to the individual git commands. We currently treat those as strings. This commit adds a _forgit_parse_array function and uses it to parse all such environment variables into arrays instead. This will allow us to get rid of deferred code, since we can pass the parsed arrays directly to the git commands and don't have to rely on eval.
@sandr01d
Copy link
Collaborator Author

@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 #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?

…o not spent time parsing arrays we never use
@sandr01d
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@cjappl
Copy link
Collaborator

cjappl commented Jan 31, 2024 via email

@carlfriedrich
Copy link
Collaborator

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
Copy link
Collaborator Author

Changes have been merged with #326

@sandr01d sandr01d closed this Mar 24, 2024
@sandr01d sandr01d deleted the parse branch April 26, 2024 21:37
@sandr01d sandr01d restored the parse branch April 26, 2024 21:37
@sandr01d sandr01d deleted the parse branch April 26, 2024 21:37
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.

3 participants