Allow patchs in diff format

[piranna]

This fixes issue #82

[kpdecker]

The -r portion feels pretty specific to the exact file mentioned in #82. Feels like we might want that to be more generic.

[piranna]

The -r portion feels pretty specific to the exact file mentioned in #82

Maybe, I'm not sure. I've looked for some info about that argument and seems to be related to CVS revisions, that make sense. According to this text, there could appear zero, one or more times, so maybe I should change the RegExp to show this case... Other option is to ignore all the flags and get only the last fragment (the index path), what do you think?

[kpdecker]

Lets ignore everything but the path component. Can you add a test case to cover this?

[kpdecker]

What behavior does this change that is necessary to do two iterations?

[piranna]

This is WiP to allow to calculate the location offset where the hunks best fit. This is to fix issue #84

[piranna]

Lets ignore everything but the path component. Can you add a test case to cover this?

I have reading some docs and thinking about this thing, and I believe we are taking a bad approach. We are considering it like the Index:, while that's only the filename when in fact the patch can have several revisions, so if we want to use it as an unique identifier, we should capture the full line. Other option would be not set the index at all and allow it to be undefined, problem is that currently this is not supported and all hunks of an undefined index are send at once and without the filenames, just the original problem of issue #82. Independtly of what we do with the Index: and the diff -r lines, maybe we should allow to undefined index and not send all the hunks at once... What do you think?

[piranna]

It's said, what I means is that according to docs both the Index: and diff lines are superfluous headers that add metadata and can be ignored altogether, being Index: a somewhat unique file identifier, and the diff line just a command line that can generate the hunk. I think we shouldn't trust on them too much...

[piranna]

Ok, offsets now are correctly calculated and NodeOS can be able to download all its build dependencies purely on Javascript :-) I'mgoing to write some tests to prevent regressions.

[piranna]

Test for hunk offsets added.

[kpdecker]

The logic here is really hard to follow and I worry that it's going to lead to confusion down the road. Rather than having side effects that terminate the loop in the middle of the block and the loop written as an infinite loop, I think it would be clearer to have the termination case expressed in the for/while statement.

[piranna]

It's an optimization, the alternative is to have the old code and use a conditional to check it, or do the check twice on the most common case (the hunk fits at the first try, when localOffset is zero). This has only moved the block to check the current or next line before the loop, so it's checked always in a loop rollback fashion. The same would be achieved by doing the check previously to enter the loop, but code is duplicated. I can add more comments if you don't see it clear.

[piranna]

I've improved the code documentation.

[kpdecker]

After coming back to this a few days later, I'm still worried that I'm not going to be able to maintain this.

What if instead we extract out the iteration logic into an iterator? Something like:

function distanceIterator(target, minimum, maximum) {
  let wantForward = true,
      backwardExhausted = false,
      forwardExhausted = false,
      offset = 0;
  return function iterate() {
    // Continue iteration on the rear when the 
    if (forwardExhausted) {
      wantForward = false;
      offset++;
    }

    if (!wantForward) {
      // Check if we have a potential position behind us
      let candidate = target - offset;
      if (candidate > minimum) {
        wantForward = true;
        return candidate;
      } else {
        backwardExhausted = true;
      }
    }

    // Check if we have a potential position in front of us
    candidate = target + offset;
    if (candidate < maximium) {
      wantForward = false;
      offset++;
      return candidate;
    } else if (!backwardExhausted) {
      // We've exhausted the forward section, reenter to continue
      // iteration on the behind.
      forwardExhausted = true;
      return iterate();
    }
  };
}

Which can live in it's own file and we can unit test in isolation. This loop here then becomes something like:

for (let i = 0; i < hunks.length; i++) {
  let ...
      toPos = offest + hunk.oldStart + 1,
      maxLine = lines.length - hunk.oldLines,
      iterator = distanceIterator(toPos, minLine, maxLine),
      toCheck = iterator();

  while (toCheck !== null) {
    if (hunkFits(hunk, toCheck)) {
      hunk.offset = toCheck;
      break;
    }

    toCheck = iterator();
  }
}

(I have not actually tested or even run this code, so it might need some work to get it running)

I think separating general flip flopping iteration and checking the hunk itself into separate code will help with clarity as well as give us a nice reusable component for this iteration should we need it in other locations.

[piranna]

I think separating general flip flopping iteration and checking the hunk itself into separate code will help with clarity as well as give us a nice reusable component for this iteration should we need it in other locations.

I find this code a little bit more complicated and less eficient, but I like the idea of separate the flip-flop logic from the main loop. I'll fetch your code and try to check it out :-)

[piranna]

I've just splitted the logic to calculate candidates on an iterator. Maybe the code gets more readable, but I think it's more complicated and less performance, but at least it does the job.

[kpdecker]

You need !== here or != null otherwise zero will fail. We should have a test case for that.

[piranna]

You need !== here or != null otherwise zero will fail.

[piranna@Mabuk:~/Proyectos/NodeOS/node_modules]
 (vagga) > node
> 0 != undefined
true
> 

I think it's not needed... :-) But I could agree on the !== undefined, in any case is the only thing it will return... I left it this way to be more generic.

We should have a test case for that.

A test case for what, exactly?

[kpdecker]

Perhaps it's been too long that I've been using != null. Generally prefer != null for "anything other than null and undefined" and "!== undefined" for "just looking for undefined". (I actually thought that was in the linter rules, but I guess not).

Little bit of a nit picky change but would be nice to have so it doesn't cause confusion down the road.

Test case: To make sure that it can patch at an offset of zero.

[kpdecker]

This should probably be a separate module to help make things clearer.

[kpdecker]

Thanks for making the changes.

Regarding complicated, by breaking out the components, it should be easier to think about and diagnose what is going. Not always the case, but I've been burned by a few patches in this repo that I didn't fully understand that caused me great headaches down the road.

Regarding performance this is still the same O, something like O(m*n) while it would need to be tested, I presume that this would perform at a similar level. The repeated substr, etc in the hunkFits logic feels like it will have more if an impact than a few function calls.

[piranna]

Regarding complicated, by breaking out the components, it should be easier to think about and diagnose what is going. Not always the case, but I've been burned by a few patches in this repo that I didn't fully understand that caused me great headaches down the road.

Sometimes overengineering promotes this (at least in my case), so I try to keep things as simple and minimal as possible, so it's also less error-prone.

Regarding performance this is still the same O, something like O(m*n) while it would need to be tested, I presume that this would perform at a similar level.

It's the same O, but implementation is more verbose so it will be slower, that's what I said about performance.

[kpdecker]

Please keep the changes focused on this PR.

[piranna]

Little bit of a nit picky change but would be nice to have so it doesn't cause confusion down the road.

Done.

Test case: To make sure that it can patch at an offset of zero.

That's the trivial case, when offset is zero it fitted at first try, so it didn't need to exec the iterator() at all... It's already tested by the other test cases.

[piranna]

I was able to put distanceIterator() as a module-level private function without problems. I tried to do the same with hunkFits() but has some secondary effects, so better left it as is.

So, to summarize, what changes need to be done (if any) so this pull-request can land on upstream? I need this to be available as soon as possible for so NodeOS (my bachelor thesis) can work... :-/

[kpdecker]

Merging this now and I'll take care of the rest of the cleanup.

[kpdecker]

This was merged, but the combination of rebase and master on master merge from the CLI commands caused github to not detect this. Closing this out, but it is merged.

[kpdecker]

048e7f7 is what I was intending. Doing this and explicit functionality tests in the micro scale helps dramatically with code quality. Best of luck with your bachelors.

[piranna]

This was merged, but the combination of rebase and master on master merge from the CLI commands caused github to not detect this

Yes, this happened to me before. Sometimes it detect it, though. Don't worry :-)

048e7f7 is what I was intending. Doing this and explicit functionality tests in the micro scale helps dramatically with code quality

Ok, I see. I'm not too much to split such code to independent files (I prefer to have it near to where it's used, or at least as global private function to reduce memory usage) except if it has a clear own entity (so maybe I would put it on its own module) or if it's being used from several places.

I like the description that you put there, simple and clear. I didn't think about it, but would be a good candidate for a simple test :-)

Best of luck with your bachelors

Thank you! :-) You can see how it goes on at https://github.com/NodeOS/NodeOS :-)