I'm not sure about the icon handling there... seems a bit icky.

In D15480#326764, @tcberner wrote:

I'm not sure about the icon handling there... seems a bit icky.

I'm not very familiar with common icon formats. I'll suggest the reporter to change the format. Thanks!

My issue is more with them being shipped in files/ and not downloaded from somewhere -- you could for example create a tkdiff-icons.tar.gz and host it in your public distfiles and add it as an additional distfile.

In D15480#326764, @tcberner wrote:

I'm not sure about the icon handling there... seems a bit icky.

Icky? Seriously?

In D15480#327198, @tcberner wrote:

My issue is more with them being shipped in files/ and not downloaded from somewhere -- you could for example create a tkdiff-icons.tar.gz and host it in your public distfiles and add it as an additional distfile.

I'd argue that that adds more complexity, than my implementation. Base64 has been used since near the dawn of email for handling attachments. Browsers have been using it since the late 20th century for everything from favicons, to embedding sprites, and images within the accompanying css files. In fact google gives higher rankings for that. Granted; this is not a web page, nor email. But IMHO this seems a great deal more efficient, than downloading an additional archive, and then processing it's contents.
Maybe I should be asking; Why do you feel downloading an additional file, and processing it's contents, is more preferable, or better than the direction I chose here?

Thanks!

--Chris

Thanks Mat!
I made the changes you suggested/required, and attempted to add it to this review, by choosing Update Diff from the menu.
But I don't think that was the correct way to do it, as I don't see the change(s).
Sorry, I'm afraid phabricator is still a bit unfamiliar to me.
FWIW I also added the new diff to pr bug #228176

Thanks again.

--Chris

@mat does not seem to mind it, so it's OK, I gueess =)

But could you give them more sensible names like tkdiff-256.png.enc or something instead of just 256.

In D15480#327568, @tcberner wrote:

@mat does not seem to mind it, so it's OK, I gueess =)

But could you give them more sensible names like tkdiff-256.png.enc or something instead of just 256.

OK I've done it. Please see D15509

I think we're done here. :-)

--Chris

In D15480#327782, @fernape wrote:

Add .base64 to files in files/

Hi Tobias,

I can't commit this patch. A svn hook prevents it:

Committing transaction...
svn: E165001: Commit failed (details follow):
svn: E165001: Commit blocked by pre-commit hook (exit code 1) with output:
Path "head/textproc/tkdiff/files/icon128.base64" is missing the svn:keywords property (or an fbsd:nokeywords override)

Additional errors may compound and may not be accurate

Path "head/textproc/tkdiff/files/icon256.base64" is missing the svn:keywords property (or an fbsd:nokeywords override)
Path "head/textproc/tkdiff/files/icon32.base64" is missing the svn:keywords property (or an fbsd:nokeywords override)
Path "head/textproc/tkdiff/files/icon64.base64" is missing the svn:keywords property (or an fbsd:nokeywords override)

Pre-commit problem count: 4

I have all the properties mentioned here:
https://people.freebsd.org/~beat/cvs2svn/auto-props.txt

What properties are necessary to commit these files?

In D15480#327989, @fernape wrote:

In D15480#327782, @fernape wrote:

Add .base64 to files in files/

Hi Tobias,

I can't commit this patch. A svn hook prevents it:

Committing transaction...
svn: E165001: Commit failed (details follow):
svn: E165001: Commit blocked by pre-commit hook (exit code 1) with output:
Path "head/textproc/tkdiff/files/icon128.base64" is missing the svn:keywords property (or an fbsd:nokeywords override)

Additional errors may compound and may not be accurate

Path "head/textproc/tkdiff/files/icon256.base64" is missing the svn:keywords property (or an fbsd:nokeywords override)
Path "head/textproc/tkdiff/files/icon32.base64" is missing the svn:keywords property (or an fbsd:nokeywords override)
Path "head/textproc/tkdiff/files/icon64.base64" is missing the svn:keywords property (or an fbsd:nokeywords override)

Pre-commit problem count: 4

I have all the properties mentioned here:
https://people.freebsd.org/~beat/cvs2svn/auto-props.txt

What properties are necessary to commit these files?

I would say they should have svn:eol-style=native; fbsd:nokeywords=yes; svn:mime-type=text/plain.
The auto properties dont' work as ti does not match any of the patterns :)