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

Fix quote - was severely broken #12

Closed
wants to merge 1 commit into from
Closed

Conversation

drok
Copy link

@drok drok commented Nov 5, 2023

The quote function was severely broken:

  1. operators were quoted; should have been output literally

  2. backslashes were used as escapes inside single-quoted strings (they lose the escape meaning inside single quoted strings)

  3. Ugliness - when no quoting was needed, it was applied anyway examples:

    • VAR=value needs no quoting. it was output as VAR=value
    • main^{commit} needs no quoting. was output as main^{commit}

Fixes Mergesium#1 and #11

Merge Conflict Note: This branch is based on v1.0.0 from substack's repo, meaning other forks in substack's network can merge it cleanly. Due to the refactor at 553fdfc there is now a merge conflict (Originally the quote functions were in index.js, at 553fdfc they were moved to quote.js). To resolve the merge conflict onto the refactored lib, you can use the quote.js file from 6e9606b, although it also contains the Unicode/unprintables escaping functionality and quote_ascii(), which are part of my 2.0.0 release, as well as the various fixes from substack's fork network that I was able to find. See CHANGELOG.md at Mergesium@6e9606b#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4ed for summary. I'll be happy to answer questions.

Author-Rebase-Consent: https://No-rebase.github.io

The quote function was severely broken:

1. operators were quoted; should have been output literally

2. backslashes were used as escapes inside single-quoted strings
   (they lose the escape meaning inside single quoted strings)

3. Ugliness - when no quoting was needed, it was applied anyway
   examples:
   - VAR=value needs no quoting. it was output as VAR\=value
   - main^{commit} needs no quoting. was output as main\^\{commit\}

Fixes #1 and ljharb#11

Author-Rebase-Consent: https://No-rebase.github.io
@drok drok marked this pull request as draft November 5, 2023 03:04
@ljharb
Copy link
Owner

ljharb commented Nov 5, 2023

If you don't consent to rebasing your PR (and re-check the box that defaults to "on", as it should), then let's just close it immediately, since a contribution inherently is waiving all control over the PR, and rebasing PRs is necessary to ensure a project remains clean.

@drok
Copy link
Author

drok commented Nov 5, 2023

If you don't consent to rebasing your PR (and re-check the box that defaults to "on", as it should), then let's just close it immediately, since a contribution inherently is waiving all control over the PR, and rebasing PRs is necessary to ensure a project remains clean.

Agreed, if rebasing is the only way you are able to accept contributions, then this PR is futile.

The PR cannot be rebased cleanly (because of the refactoring), it would almost certainly introduce bugs to attempt it, and I don't want my name on bugs introduced this way.

I merged your branch and all the other forks into my main branch and think it looks quite neat and easier to read than if I rebased it all into a pile of commits.

BTW, I did have to make a major version increase, because of breaking backwards compatibility, and reading some of the other remaining work I'll have to increase the major again when I deal with the #1 issue.

Cheers.

@drok drok marked this pull request as ready for review November 5, 2023 05:44
@ljharb ljharb closed this Nov 5, 2023
@ljharb
Copy link
Owner

ljharb commented Nov 5, 2023

Rebasing is how almost every project accepts contributions; I’ve required it on 450+ projects for years and you’re the first person who’s ever had an issue with it.

@drok
Copy link
Author

drok commented Nov 5, 2023

An appeal to tradition is not proof of merit, my new friend.

@ljharb
Copy link
Owner

ljharb commented Nov 5, 2023

Indeed; it merely illustrates how much friction you’ll likely run into.

I don’t need to argue the merits of why the maintainer would need the ability to clean up a PR before landing it in the repo; the project’s reputation and git hygiene is far far more at risk and important than that of a contributor.

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.

Why is the equal sign escaped in args to quote()
2 participants