[Toybox] [PATCH] ls: Ensure file names are separated by 2 spaces

Andrew Ilijic ilijic.andrew at gmail.com
Sun Oct 27 17:23:17 PDT 2019


Hi Rob,

Thank you for the critique. Whatever advice you have time to give I
will gladly take. I remember reading somewhere that it said to ping
you if you did not get to a patch in a week. It had not been a week so
I did not say anything. I put the links in my previous email to try
and make it easier for you. Let me know if it is better not to do
that.

> I saved the attachment. Did you create it with "git format-patch -1 $COMMIT" or
> some other way? (Is this git version skew...?)
 I did `git format-patch -2 $HEAD` "2" for two commits. Clearly, that
did not have the desired effect. I think I also had to copy and paste
the patch for some reason, which also may have messed it up. In the
future, I will do:
`git format-patch -1 <commit1>`
`git format-patch -1 <commit2>`
Do you want me to redo and submit the patches?

> -      if (curcol < 255) printf("%*c", curcol, ' ');
> +      // Use dtlen and ul to not print spaces when we are at the end
> +      if (curcol < 255 && ((dtlen - ul - 1) > 0)) printf("%*c", curcol, ' ');
>
> You don't need a comment on every single line of the code. That's a good commit
> comment, but there isn't a comment explaining why we're comparing curcol with
> 255.
I spent a while looking at `if (curcol < 255)` and could not figure
out why we are doing that comparison. I'll go look at the git history
and try to see why it is there. If that does not work I'll reach out
to the person that committed it.

> So you probably instead want something like dtlen>ul+1 as your test?
Yes, I had that and then decided to change it; I don't remember why. I
will incorportWrapping parts of the conditional is just a habit that I
picked up along the way. I will try to break the habit and let the
code do the work, instead of the parentheses. I thought I was okay
because, the loop/pointer offset would always be less than the length,
`dtlen.` I forgot to consider the case where the array is empty thus
making `dtlen` zero.

~Andrew



More information about the Toybox mailing list