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

Preserve "garbage" #522

Closed
wants to merge 16 commits into from
Closed

Preserve "garbage" #522

wants to merge 16 commits into from

Conversation

ExplodingCabbage
Copy link
Collaborator

No description provided.

@ExplodingCabbage ExplodingCabbage self-assigned this Jun 10, 2024
@ExplodingCabbage ExplodingCabbage marked this pull request as ready for review June 24, 2024 14:20
@ExplodingCabbage
Copy link
Collaborator Author

I've decided this may have been misguided. It's way too complicated and the correct behaviours to implement are not clear. Several dilemmas:

  • Should we also support parsing/preserving trailing garbage? I can't find any programs that output it, yet the GNU patch util supports ignoring it. What's the right behaviour?

  • What about trailing garbage after a hunk header, on the same line as it? Git heuristically generates such garbage to try to show the enclosing context (e.g. function declaration) that a hunk appears in - e.g. the export function formatPatch(diff) { here:

     diff --git a/src/patch/create.js b/src/patch/create.js
     index e04f13d..462a171 100644
     --- a/src/patch/create.js
     +++ b/src/patch/create.js
     @@ -165,10 +165,6 @@ export function formatPatch(diff) {
          ret.push.apply(ret, hunk.lines);
        }
      
     -  if (diff.trailingGarbage) {
     -    ret.push(diff.trailingGarbage);
     -  }
     -
        return ret.join('\n') + '\n';
      }

    if we don't support that, we're still not really letting people roundtrip their patches through a parsePatch / formatPatch combo.

  • What should formatPatch do if someone passes it a structured patch object where they've manually set the leadingGarbage to include file headers - i.e. to not be valid garbage? We should probably reject that somehow, but that further complicated the API.

Not worth it - at least not without a lot more subtle and nuanced design and implementation work than I've already put into this PR. If someone wants to take that on in due course, good for them, but I don't want to merge something half-assed because I fear I'm just making a mess that's worse than the status quo.

I will carefully cherry-pick out the actual bugfix from this PR, but I'm abandoning the rest.

ExplodingCabbage added a commit that referenced this pull request Jun 24, 2024
* Fix patch parser, making most tests pass (but why do I have a weird new failure?...)

* Fix bug I introduced, caught by test failure

* Fix bugs with my tests

* Remove mention of leadingGarbage (introduced by cherry-picking changes from #522)

* Improve test coverage

* Add release notes
@ExplodingCabbage ExplodingCabbage deleted the leading-garbage branch September 12, 2024 08:46
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.

1 participant