Fwd: [hackers] [st-orig][PATCH] Add MS Office 365 account requirement.

[Silvan Jegen]

Hi Christoph

Finally I can make use my Office 365 account! Thank you for this!!

One comment below.

On Fri, Apr 1, 2022 at 6:46 AM Christoph Lohmann <2...@r-36.net> wrote:
>
> ---
> [...]
>
> @@ -0,0 +1,27 @@
> +#!/usr/bin/env python
> +# coding=utf.8
> +#
> +# See st LICENSE for license details.
> +#
> +
> +import os
> +import sys
> +
> +from O365 import Account
> +
> +def main(args):
> +   clientid = os.getenv("ST_O365_CLIENTID", None)
> +   clientsecret = os.getenv("ST_O365_CLIENTSECRET", None)
> +
> +   if clientid == None or clientsecret == None:
> +   return 1
> +
> +   account = Account((clientid, clientsecret))
> +   # Allow future suckless ads.
> +   if account.authenticate(scopes=['basic', 'message_all']):

I think we are missing the 'onedrive_all' and 'sharepoint_dl' scopes.
Otherwise it will be difficult for us to download all their files to
analyse with our ML-as-a service and extract actionable info to
leverage our computing on the edge infra!


Cheers,

Silvan

Re: [hackers] [dmenu|libsl][PATCH] optimize drw_text() for large strings

[Silvan Jegen]

Hi

On Sat, Mar 19, 2022 at 12:59 PM Stein Gunnar Bakkeby  wrote:
>
> Hi NRK,
>
> I was tinkering with this for a few hours earlier this week and there are 
> several issues with this area of code and not specifically related to this 
> modification of the patch.
>
> The handling of adding the ellipsis (...) when the text is too long is rather 
> hacky and can cause several issues.
>
> The general idea is that:
>- we work out how many consecutive characters we can print for the same 
> font
>- then we check if the length of the string exceeds the remaining drawable 
> space
>- if it is too long then we check the length of the string for every 
> character until we find that it no longer fits
>- then we replace the last three bytes with periods (.)
>
> These are the problematic areas I have found:
>
> 1. In the original code the text is allowed to bleed into the right padding 
> as long as it fits (I presume this is intentional). If you add one more 
> character then the text is too long and it will crop of four characters to 
> add the ellipsis. This has to do with that the for loop reduces len once more 
> than necessary.
>
> 2. The adding of the ellipsis naively assumes that the last characters in the 
> string are single-byte UTF-8 characters. As such when it replaces the last 
> bytes of the string then you can end up with split multi-byte UTF-8 
> characters which can have various effects.

I *think* I somewhat improved this issue in `vis-menu` (which is based
on `dmenu`, IIRC):

https://github.com/Shugyousha/vis/commit/d59b98d934815e54320ad000eebfdaaf8fee344d

Note that this is not well tested and still has issues though.


Cheers,

Silvan

Re: [hackers] [libgrapheme] Mark likely branches || Laslo Hunhold

[Silvan Jegen]

NRK  wrote:
> On Wed, Jan 05, 2022 at 01:56:26AM +0600, NRK wrote:
> > Just curious, why not use:
> > 
> > #if defined(__has_builtin) && __has_builtin(__builtin_expect)
> > #define likely(expr) __builtin_expect(!!(expr), 1)
> > #define unlikely(expr) __builtin_expect(!!(expr), 0)
> > #else
> > #define likely(expr) (expr)
> > #define unlikely(expr) (expr)
> > #endif
> 
> Answering my own question: because it fails if `__has_builtin` is not
> defined. I was expecting the 2nd expression wouldn't get evaluated at
> all. Should probably take some time and learn more about the
> pre-processor sometimes.

Same for me! I didn't know that the C pre-processor doesn't
short-circuit ...

Thanks for answering your own question! As you can see, it's
useful for others as well :)


Cheers,

Silvan

Re: [hackers] [libgrapheme] Refactor libgrapheme.7 || Laslo Hunhold

[Silvan Jegen]

Dear Laslo

Laslo Hunhold  wrote:
> On Sat, 10 Oct 2020 22:47:01 +0200
> "Silvan Jegen"  wrote:
> 
> Dear Silvan,
> 
> > I think libgrapheme is a very good idea! I have just one comment
> > below.
> 
> thanks for taking your time to review this commit and give feedback. I
> really appreciate it!

Thank you for making the software world a better place! :)


> > > +The
> > >  .Nm
> > > -is a C library for working with grapheme clusters. What are
> > > grapheme -clusters? In C, one usually uses 8-Bit unsigned integers
> > > (chars) to -store strings, and many people assume that one such
> > > char represents -one visible character in a printed output.
> > > +library provides functions to properly count characters
> > > +.Dq ( grapheme clusters )  
> > 
> > I feel like it should be made clear that from that point on, when the
> > man page mentions a "character" it refers to a grapheme cluster. The
> > reader can then either look up its definition or you could give a
> > short description together with it.
> > 
> > That should make the uses of "character" further down less amibiguous
> > for the reader who is not familiar with the concept of a grapheme
> > cluster.
> > 
> > Just my two cents!
> 
> This is a good point. I worked this into the manpage in commit
> 706b4d4c. Is it clearer now or do you have further ideas to improve it?

It definitely looks (even) better to me now!

Below I have added a few more comments for the rendered man page.


> In many applications, it is necessary to count the number of user-per‐

I *think* it's better to leave out the first comma here but I could be
wrong about this.

> ceived characters, i.e. grapheme clusters, in a string. This is pretty

I would add an example of why you would want to know how many perceived
characters are in a UTF-8 string (to really drive home the point),
so something like:

In many applications, it is necessary to count the number of user-per‐
ceived characters, i.e. grapheme clusters, in a string. *In a text editor,
for example, you need to know how many of these grapheme clusters you
have to draw on the screen so you can calculate if you have enough space
to do that on one line, etc.* This is pretty ...


> simple with ASCII-strings, where you just count the number of bytes (as
> each byte is a code point and each code point is a grapheme cluster).
> With Unicode-strings, it is a common mistake to simply adapt the ASCII-
> approach and count the number of code points. This is wrong, as, for ex‐
> ample, the sequence “0x41 0x308 0x304”, while made up of 3 code points,
> is a single grapheme cluster and represents the user-perceived character
> ‘Ǟ’.
>
> The proper way to segment a string into user-perceived characters is to
> segment it into its grapheme clusters by applying the Unicode grapheme
> cluster breaking algorithm (UAX #29). It is based on a complex ruleset
> and lookup-tables and determines if a grapheme cluster ends or is contin‐
> ued between two code points. Libraries like ICU, which also offer this
> functionality, are often bloated, not correct, difficult to use or not
> statically linkable. The motivation behind libgrapheme is to make
> grapheme cluster handling suck less and abide the UNIX philosophy.

s/abide/abide by/ ? Not sure, we may need some native speaker input here.

Otherwise the page looks great to me!


Kind regards,

Silvan

Re: [hackers] [libgrapheme] Refactor libgrapheme.7 || Laslo Hunhold

[Silvan Jegen]

Hi Laslo

I think libgrapheme is a very good idea! I have just one comment below.

g...@suckless.org wrote:
> commit 51eca9eff65def13d1370e32dad2988731d38e7d
> Author: Laslo Hunhold 
> AuthorDate: Sat Oct 10 18:56:47 2020 +0200
> Commit: Laslo Hunhold 
> CommitDate: Sat Oct 10 18:56:47 2020 +0200
> 
> Refactor libgrapheme.7
> 
> It read more than a rant and didn't get to the point of what a manual
> should do: Provide an overview. Still, I felt like adding a few
> paragraphs on the motivation and added a section "BACKGROUND" for this
> purpose.
> 
> The other manual pages will follow accordingly.
> 
> Signed-off-by: Laslo Hunhold 
> 
> diff --git a/man/libgrapheme.7 b/man/libgrapheme.7
> index 70eba76..eb8d76e 100644
> --- a/man/libgrapheme.7
> +++ b/man/libgrapheme.7
> @@ -1,38 +1,90 @@
> -.Dd 2020-03-26
> +.Dd 2020-10-10
>  .Dt LIBGRAPHEME 7
>  .Os suckless.org
>  .Sh NAME
>  .Nm libgrapheme
> -.Nd grapheme cluster utility library
> +.Nd grapheme cluster detection library
> +.Sh SYNOPSIS
> +.In grapheme.h
>  .Sh DESCRIPTION
> +The
>  .Nm
> -is a C library for working with grapheme clusters. What are grapheme
> -clusters? In C, one usually uses 8-Bit unsigned integers (chars) to
> -store strings, and many people assume that one such char represents
> -one visible character in a printed output.
> +library provides functions to properly count characters
> +.Dq ( grapheme clusters )

I feel like it should be made clear that from that point on, when the
man page mentions a "character" it refers to a grapheme cluster. The
reader can then either look up its definition or you could give a short
description together with it.

That should make the uses of "character" further down less amibiguous
for the reader who is not familiar with the concept of a grapheme cluster.

Just my two cents!


Cheers,

Silvan


> +in Unicode strings using the Unicode grapheme
> +cluster breaking algorithm (UAX #29).
>  .Pp
> -This is not true and only holds for encodings that map numbers from
> -0-255 to characters. Modern Unicode maps numbers ('code points') far
> -larger than that to characters. A common encoding to represent such
> -code points is UTF-8. A common misunderstanding is that a code
> -point represents a single printed character, which is not correct.
> -Instead, Unicode has a concept of so called 'grapheme clusters', which
> -are a set of one or more code points that in total make up one printed
> -character.
> -.Pp
> -To put it shortly: To count printed characters in a string, it is
> -neither enough to just count the chars nor to count the UTF-8 code points.
> -Instead, what is necessary is to apply a complex ruleset, specified
> -by Unicode, to determine if a set of code points belongs together in the
> -form of a grapheme cluster, which then counts as a single character.
> -.Pp
> -.Nm
> -is a suckless response to the bloated ecosystem of grapheme cluster
> -handling (e.g. ICU) and provides a simple interface for this complex
> -concept. The rules are automatically downloaded from unicode.org
> -and parsed and automatic testing is performed based on tests provided
> -by Unicode.
> +You can either count the characters in an UTF-8-encoded string (see
> +.Xr grapheme_len 3 )
> +or determine if a grapheme cluster breaks between two code points (see
> +.Xr grapheme_boundary 3 ) ,
> +while a safe UTF-8-de/encoder for the latter purpose is provided (see
> +.Xr grapheme_cp_decode 3
> +and
> +.Xr grapheme_cp_encode 3 ) .
>  .Sh SEE ALSO
> +.Xr grapheme_boundary 3 ,
> +.Xr grapheme_cp_decode 3 ,
> +.Xr grapheme_cp_encode 3 ,
>  .Xr grapheme_len 3
> +.Sh STANDARDS
> +.Nm
> +is compliant with the Unicode 13.0.0 specification.
> +.Sh MOTIVATION
> +The idea behind every character encoding scheme like ASCII or Unicode
> +is to assign numbers to abstract characters. ASCII for instance, which
> +comprises the range 0 to 127, assigns the number 65 (0x41) to the
> +character
> +.Sq A .
> +This number is called a
> +.Dq code point ,
> +and all code points of an encoding make up its so-called
> +.Dq code space .
> +.Pp
> +Unicode's code space is much larger, ranging from 0 to 0x10, but its
> +first 128 code points are identical to ASCII's. The additional code
> +points are needed as Unicode's goal is to express all writing systems
> +of the world. To give an example, the character
> +.Sq \[u00C4]
> +is not expressable in ASCII, as it lacks a code point for it. It can be
> +expressed in Unicode, though, as the code point 196 (0xC4) has been
> +assigned to it.
> +.Pp
> +At some point, when more and more characters were assigned to code
> +points, the Unicode Consortium (that defines the Unicode standard)
> +noticed a problem: Many languages have much more complex characters,
> +for example
> +.Sq \[u01DE]
> +(Unicode code point 0x1DE), which is an
> +.Sq A
> +with an umlaut and a macron, and it gets much more complicated in some
> +non-European languages. Instead of assigning a code 

Re: [hackers] sfakeroot (announce?)

[Silvan Jegen]

Hi Richard

Richard Ipsum  wrote:
> For a while I've been writing some tests which cover basic POSIX util
> functionality, these tests have been quite useful in helping me find
> bugs in sbase and other implementations. Recently though I wrote some

Those tests sound interesting! Are they publicly available somewhere?


Cheers,

Silvan

Re: [hackers] Announcing libschrift (a TrueType font rendering library)

[Silvan Jegen]

Michael Forney  wrote:
> On 2020-04-24, Silvan Jegen  wrote:
> > Yeah, that's where my missing understanding of graphics programming
> > becomes apparent. I assumed a font rendering library will just take a
> > pointer to some memory as an argument to its rendering function to which
> > it will then write the bitmapped glyph. Surely you will have to take the
> > different buffer formats into account there though so I am not sure how
> > that would work.
> 
> You would also have to consider how the glyph should be blended with
> the destination, whether you want to use mask image (for example to
> render in different colors or with transparency), and other details.
> Rendering the glyphs to separate images in a grayscale format gives
> the most flexibility. This also allows you to cache the glyphs so you
> don't have to re-render them all the time.

Yes, that makes sense.

I have heard about the atlas approach that you have mentioned
before. There you render all the glyphs to some texture which you can then
blend with a texture that holds all the other stuff you want to render.

In the libSDL2 library, they even have a function to adjust color values
for textures so you can render the same glyph atlas in different colours
if you need colored text.


> > I'm also stomped by how the shared memory (like wl_shm) differs in Wayland
> > from a buffer you would get from EGL (like dmabuf-based wl_buffers) or
> > another graphics library. I thought I could just use pixman to render
> > into both but I don't think that's actually true... This example[0]
> > definitely looks relevant to this topic.
> 
> The buffers you get from OpenGL/Vulkan are generally tiled in various
> vendor-specific (or even model-specific) formats, meaning they don't
> use a linear memory layout. If you were to render into it as if it
> were linear, you'd get jumbled output on the screen. The GPU memory is

Yeah, I figured something like this would end up happening in that case.


> also not necessarily accessible by the CPU. There are ways to allocate
> an image in a linear layout backed by host-mappable memory, but that
> is likely to come with performance penalty (comparatively). Usually,
> you would only do this to upload textures to the GPU.
> 
> Recently, most GPU vendors have agreed to settle on an opaque 64-bit
> "format modifier", which can be passed between APIs along with the
> DMA-BUF fd so they know how the image is laid out in memory.
> Previously, the image layout was either assumed, or passed along
> through some out-of-band channel (for example, AMD stores this
> information in a metadata structure associated with the buffer).

I assume that means you would have to read these "format modifiers"
and then adjust the rendering in your library according to the format of
the two (or more) buffers that are involved. Since there will be quite
a few combinations this is a PITA, I would imagine.


> I've been working off-and-on for a while on a library[0] to provide
> some sort of API to do simple 2D rendering and presentation operations
> that can be implemented with XRender, Vulkan, pixman, and also
> directly by submitting command buffers to the GPU. It's meant to be
> the successor to wld, but I still have a really long way to go, and
> have not begun to thing about how text rendering would work (but when
> I do, I'll definitely be looking at libschrift).

That sounds awesome and I will have a look for sure!

I have actually been looking at the wld source for a while since I have
an AMD Polaris card and thought it would be awesome to implement its
interface for the amdgpu driver. I didnt' get very far though since to
me it seems that the interface to what the AMD kernel driver exposes is
severely under-documented (at least for a novice like me). I couldn't
figure out where to get the memory from that wld can render into at all.


Cheers,

Silvan

> 
> [0] https://git.sr.ht/~mcf/libblit/

Re: [hackers] Announcing libschrift (a TrueType font rendering library)

[Silvan Jegen]

Hi Michael

Thanks for your input!

Michael Forney  wrote:
> On 2020-04-23, Silvan Jegen  wrote:
> > I had a quick look and currently it looks like it's mostly useful for
> > rendering of fonts in X. I wonder how an interface would look like that
> > could also be used for text rendering for a Wayland client. I assume the
> > library would instead just render to some graphics memory to be rendered
> > by the compositor, but I am not completely sure.
> 
> I think the interface would look the same for Wayland, but the missing
> piece is something to composite the glyphs into the application's
> window buffer, which is handled by XRender in the demo.
> 
> If you are rendering to shared memory, pixman (which is essentially
> XRender in a library) can be used similarly. You can create a glyph
> cache, load the glyph images produced by libschrift into it, and then
> use pixman_composite_glyphs to render them onto your frame.
>
> For OpenGL/Vulkan, it's the same on X11 and Wayland, since the client
> is doing direct rendering in both cases. I believe it's generally done
> by creating an "atlas" texture containing a bunch of glyphs at
> different positions, and then rendering subregions of it onto your
> frame. Most code using freetype directly could probably be adapted to
> libschrift fairly easily.

Yeah, that's where my missing understanding of graphics programming
becomes apparent. I assumed a font rendering library will just take a
pointer to some memory as an argument to its rendering function to which
it will then write the bitmapped glyph. Surely you will have to take the
different buffer formats into account there though so I am not sure how
that would work.

I'm also stomped by how the shared memory (like wl_shm) differs in Wayland
from a buffer you would get from EGL (like dmabuf-based wl_buffers) or
another graphics library. I thought I could just use pixman to render
into both but I don't think that's actually true... This example[0]
definitely looks relevant to this topic.

These are the questions I will have to answer for myself before I will
be able to contribute much there, I am afraid.


Cheers,

Silvan

[0] 
https://github.com/wayland-project/weston/blob/master/clients/simple-dmabuf-egl.c

Re: [hackers] Announcing libschrift (a TrueType font rendering library)

[Silvan Jegen]

Hi Thomas

Thomas Oltmann  wrote:
> Last year at slcon6, I demo'd a toy TrueType font renderer (*) to a
> couple people.
> Someone there suggested it'd be really useful to have this as a proper
> library for suckless projects to use,
> and so after a complete rework to make it actually usable, I'm finally
> able to release it under the name libschrift!
> You can find it here:  https://www.github.com/tomolt/libschrift
> 
> What you'll notice is that, similar to other font rendering libraries,
> libschrifts API is very low-level;
> In future, I'll probably write a wrapper library that abstracts away
> most of this.
> For right now, there's at least an example/demo application called sftdemo.
> sftdemo shows how to use libschrift to render text to an X11 window
> completely without Xft or FreeType2.
> To do this, sftdemo uses the same underlying interface that Xft is
> also built upon.
> 
> Still, you shouldn't use libschrift for anything serious quite yet.
> Most notably, compound glyph support is still missing,
> so some characters like Umlauts or accents will likely not work yet.
> 
> As a proof of concept, I might at some point write a patch for dmenu
> (or something like it)
> that replaces all of its Xft / FreeType2 usage with libschrift.
> 
> I can keep you all posted if you're interested.

That's a really cool project!

I am interested in linguistics and, to a lesser degree, in 2D
graphics. Lately I was thinking that I really should look into font
rendering as that is where those two interests meet :P this library is
a good starting point for me!

I had a quick look and currently it looks like it's mostly useful for
rendering of fonts in X. I wonder how an interface would look like that
could also be used for text rendering for a Wayland client. I assume the
library would instead just render to some graphics memory to be rendered
by the compositor, but I am not completely sure.


Cheers,

Silvan

Re: [hackers] [st] [PATCH] Work around BadLength error by disallowing color fonts

[Silvan Jegen]

Hi Laslo

Thanks for giving this a shot!

A comment below.

Laslo Hunhold  wrote:
> Dear fellow hackers,
> 
> this patch will hopefully resolve the many mails we get on dev@ and
> hackers@ regarding crashes of st due to emoji-fonts triggering some
> voodoo-condition in Xft.
> 
> I hope my port of Anselm's change to dwm to st is correct and would
> love to hear feedback.
> 
> With best regards
> 
> Laslo Hunhold
>
> [...]
> 
> diff --git a/x.c b/x.c
> index 5828a3b..074df47 100644
> --- a/x.c
> +++ b/x.c
> @@ -837,12 +837,24 @@ xgeommasktogravity(int mask)
>  int
>  xloadfont(Font *f, FcPattern *pattern)
>  {
> +   FcBool iscol;
> FcPattern *configured;
> FcPattern *match;
> FcResult result;
> XGlyphInfo extents;
> int wantattr, haveattr;
> 
> +   /* Do not allow using color fonts. This is a workaround for a 
> BadLength
> +* error from Xft with color glyphs. Modelled on the Xterm 
> workaround. See
> +* https://bugzilla.redhat.com/show_bug.cgi?id=1498269
> +* https://lists.suckless.org/dev/1701/30932.html
> +* https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=916349
> +* and lots more all over the internet.
> +*/
> +   if(FcPatternGetBool(pattern, FC_COLOR, 0, ) == FcResultMatch && 
> iscol) {
> +   return 1;
> +   }
> +

From what I can tell, returning 1 in this case won't work because st
calls die() when xloadfont returns non-0 at all call sites. We would
have to load a fallback font instead, I think.

The dwm patch for for this issue [0] also adds this call to choose the
fallback font.

+ FcPatternAddBool(fcpattern, FC_COLOR, FcFalse);

We probably have to do the same in st.


Cheers,

Silvan


[0] 
https://git.suckless.org/libsl/commit/53ebcb48c6b12882c6dbe352ee43c96b2fb01b84.html

Re: [hackers] [dmenu] fix crash when XOpenIM returns NULL || Hiltjo Posthuma

[Silvan Jegen]

Hi Anselm

[2019-02-12 11:32] Anselm Garbe 
> This is so ugly. People should learn English instead of using
> antiquated text input methods ;)

I also think this code is very ugly and should be simplified like in
the patches that have been sent before for this issue. Something like
(untested):

+ /* input methods */
+ if ((xim = XOpenIM(dpy, NULL, NULL, NULL)) == NULL) {
+ XSetLocaleModifiers("@im=local");
+ xim = XOpenIM(dpy, NULL, NULL, NULL);
+ }

That is still ugly but better at least.


> Suckless is also about the input interface. If a typographic system
> sucks, because it consists of thousands of letters, it has to be
> fixed.

Yeahh, here I obviously can't agree...


Cheers,

Silvan

Re: [hackers] [dmenu][PATCH] Replace dmenu_run shell with executed application

[Silvan Jegen]

On February 9, 2019 7:48:34 PM GMT+00:00, Nick  
wrote:
>Quoth Hiltjo Posthuma:
>> On Sat, Feb 09, 2019 at 01:48:34PM +0100, Jan Bessai wrote:
>> > On Sun, Dec 30, 2018 at 02:59:13PM +0100, Jan Bessai wrote:
>> > > Thanks! I've attached the updated patch below.
>> > 
>> > Sorry if I'm breaching any rules, but any update on
>accepting/rejecting
>> > the patch?
>> > 
>> 
>> Rejected
>
>Ignore if you're too busy, but why is this considered bad practise?  
>Is there some case of possible shell escaping or something I'm 
>failing to see? I just ask for my own education.

I would also be interested in the reason for the rejection. It seems sensible 
to me to not let all these shells linger around for no reason.


Cheers,

Silvan

Re: [hackers] [libsl][PATCH] Workaround Xft BadLength X error

[Silvan Jegen]

Hi

Thanks for the patch!

[2019-01-16 21:51] Thomas Spurden 
> Modify the fontconfig pattern to prefer non-color fonts, and discard any
> selected font which has the color flag set. Using these fonts with Xft is just
> going to generate a BadLength X error.
>
> ---
>  drw.c | 14 ++
>  1 file changed, 14 insertions(+)

I have tested the patch and it works for me.

Ignoring fonts that contain colour emoji seems to me like the best way
to handle this unfortunate situation.

Just a week or two ago dwm crashed on me because of some stupid colour
emoji so your patch comes just in time. I will run dwm with the patch
applied for a while just to see if I encounter any issues even though
I don't expect to.


Cheers,

Silvan

Re: [hackers] [st][PATCH] separate blinking timer from drawing in run()

[Silvan Jegen]

Hi

[2019-01-02 02:50] kais euchi 
> This article [?] made me wonder how to improve latency in st, and i
> thought i would share this small modification for a non-blinking setup.
> When blinktimeout is set to 0, it reduces latency by ca. 5ms [?] by avoiding 
> useless delay calculation making it also independent from the xfps setting.

I measured the difference on my machine (dwm on a Ryzen 7 1700X) and it
was quite a bit bigger:

# Title   Min   Max   Avg   SD
1 st normal without blinking  20.0  29.6  22.7  3.0
2 vim in st without blinking  20.1  29.5  21.8  1.1
3 st-patched normal without blinking  3.9   12.8  7.8   1.9
4 vim in st-patched without blinking  4.9   12.3  8.6   1.3

where "st normal" is just st running bash. It's somewhat suspect that
in the non-patched case the average latency of st running vim is lower
than with just bash. For now I will assume it was a statistical fluke.

All datapoints generated by typometer can be downloaded from here[0].

Not sure the difference is actually notable in practice but the average
latency is reduced to less than half on my machine. Looks nice! It comes
at a cost of a few more lines of code though.


Cheers,

Silvan

[0] https://sillymon.ch/data/resultspatchedandunpatched.csv

Re: [hackers] [dmenu][PATCH] Replace dmenu_run shell with executed application

[Silvan Jegen]

Hi

[2018-12-30 13:41] Jan Bessai 
>
> Currently dmenu_run spawns a subshell and keeps running for each process
> it executes. Over time this litters up the process list with useless
> instances of dmenu_run, which do nothing but wait for their child to
> exit. The patch below replaces the dmenu_run process with its child,
> freeing up resources immediately. The difference is especially
> noticeable when dmenu is used in window managers.
>
> -- Jan
>
> ---
>  dmenu_run | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dmenu_run b/dmenu_run
> index 834ede5..5c9b4e8 100755
> --- a/dmenu_run
> +++ b/dmenu_run
> @@ -1,2 +1,2 @@
>  #!/bin/sh
> -dmenu_path | dmenu "$@" | ${SHELL:-"/bin/sh"} &
> +exec `dmenu_path | dmenu "$@"`

To keep this portable you should refrain from using backticks (`) and
use the POSIX-way:

exec $(dmenu_path | dmenu "$@")


Cheers,

Silvan

[hackers][sbase][PATCH] readme: add mention of a seperate testing repo

[Silvan Jegen]

---
The original subject was

"[hackers][sbase][PATCH v3] testing: add first shell-based tests"

I put the testing code into a seperate repo and just added a blurp about
it to the README. Let me know if this works for you.

Currently the code is hosted on my own server but we could also change
that.

Changes compared to v3:
* put the code into a seperate testing repo

 README | 17 +
 1 file changed, 17 insertions(+)

diff --git a/README b/README
index da2e500..b993f43 100644
--- a/README
+++ b/README
@@ -125,6 +125,22 @@ individual tools to sbase-box or run: make 
sbase-box-install
 Ideally you will  want to statically link sbase.  If  you are on Linux
 we recommend using musl-libc[2].
 
+
+Testing
+---
+
+There is a (still young) seperate testing repo[3] which you can clone
+by running:
+
+git clone  https://sillymon.ch/git/unixtoolstesting.git
+
+inside the root of your sbase repo. Inside this cloned directory you can
+then run the './runalltests' shell script to run the existing (functional)
+tests. You are very welcome to write more tests and send them as patches
+to either the suckless hackers mailing list or to the mailing list of
+the project at ~shugyousha/public-in...@lists.sr.ht .
+
+
 Portability
 ---
 
@@ -138,3 +154,4 @@ You can build sbase with gcc, clang, tcc, nwcc and pcc.
 
 [1] http://git.suckless.org/ubase/
 [2] http://www.musl-libc.org/
+[3] https://sillymon.ch/cgit/unixtoolstesting/
-- 
2.20.0

[hackers][sbase][PATCH v3] testing: add first shell-based tests

[Silvan Jegen]

We add some shell helper functions to test the expected output of sbase
tools.

In addition to the helper functions themselves we add some tests for
'dirname'.
---
Changes compared to v2:
* use a glob instead of a subshell

Changes compared to v1:
* use "ls" instead of "find" in subshell
* return number of failed tests in "runalltests" script
* use lowercase variable names more consistently
* print error message on cmp error condition
* make testing Make target depend on all


---
 Makefile |  6 +-
 tests/dirname.test   | 20 
 tests/runalltests| 12 
 tests/test-common.sh | 42 ++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100755 tests/dirname.test
 create mode 100755 tests/runalltests
 create mode 100644 tests/test-common.sh

diff --git a/Makefile b/Makefile
index 0e421e7..2395b50 100644
--- a/Makefile
+++ b/Makefile
@@ -274,4 +274,8 @@ clean:
rm -f $(BIN) $(OBJ) $(LIB) sbase-box sbase-$(VERSION).tar.gz
rm -f getconf.h
 
-.PHONY: all install uninstall dist sbase-box sbase-box-install 
sbase-box-uninstall clean
+testing: all
+   @cd tests/; \
+   ./runalltests
+
+.PHONY: all install uninstall dist sbase-box sbase-box-install 
sbase-box-uninstall clean testing
diff --git a/tests/dirname.test b/tests/dirname.test
new file mode 100755
index 000..3bc45ca
--- /dev/null
+++ b/tests/dirname.test
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+. ./test-common.sh
+
+#"test name"   "command to execute" "expected output"
+check_stdout "dirname-noarg"   "../dirname" "" && \
+check_stderr "dirname-noarg-stderr" "../dirname" "usage: ../dirname path\n" && 
\
+check_stdout "dirname-non-existing" "../dirname a b c" "" && \
+check_stdout "dirname-slash" "../dirname /" "/\n" && \
+check_stdout "dirname-dashes-slash" "../dirname -- /" "/\n" && \
+check_stdout "dirname-dashes-slash-a" "../dirname -- /a" "/\n"  && \
+check_stdout "dirname-doublequotes" "../dirname \"\"" ".\n" && \
+check_stdout "dirname-slashes" "../dirname ///" "/\n" && \
+check_stdout "dirname-a/b" "../dirname a/b" "a\n" && \
+check_stdout "dirname-a/b/" "../dirname a/b/" "a\n" && \
+check_stdout "dirname-a/b//" "../dirname a/b//" "a\n" && \
+check_stdout "dirname-a" "../dirname a" ".\n" && \
+check_stdout "dirname-a/" "../dirname a/" ".\n" && \
+check_stdout "dirname-/a/b/c" "../dirname /a/b/c" "/a/b\n" && \
+check_stdout "dirname-//a/b/c" "../dirname //a/b/c" "//a/b\n"
diff --git a/tests/runalltests b/tests/runalltests
new file mode 100755
index 000..99270c8
--- /dev/null
+++ b/tests/runalltests
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+for testfile in *.test; do
+   ./$testfile
+   ret=$(expr $ret + $?)
+done
+
+if [ $ret -gt 0 ]; then
+   printf "%d tests failed\n" $ret
+fi
+
+exit $ret
diff --git a/tests/test-common.sh b/tests/test-common.sh
new file mode 100644
index 000..b1d7572
--- /dev/null
+++ b/tests/test-common.sh
@@ -0,0 +1,42 @@
+check_output() {
+   testname=$1
+   cmdtorun=$2
+   expectedoutput=$3
+   usestdout=$4
+   expoutfile=$(mktemp)
+   actualoutfile=$(mktemp)
+   ret=0
+
+   printf "$expectedoutput" > $expoutfile
+   if [ $usestdout -eq 1 ]; then
+   eval $cmdtorun > $actualoutfile 2> /dev/null
+   else
+   eval $cmdtorun 2> $actualoutfile 1> /dev/null
+   fi
+
+   cmp $expoutfile $actualoutfile  2>&1 > /dev/null
+   if [ $? -eq 1 ]; then
+   printf "$testname:\n"
+   printf "\tWanted:\n"
+   cat $expoutfile
+   printf "\n\tGot:\n"
+   cat $actualoutfile
+   printf "\n\n"
+   ret=1
+   fi
+   if [ $? -eq 2 ]; then
+   printf "cmp error\n"
+   ret=1
+   fi
+
+   rm $expoutfile $actualoutfile
+   return $ret
+}
+
+check_stdout() {
+   check_output "$1" "$2" "$3" 1
+}
+
+check_stderr() {
+   check_output "$1" "$2" "$3" 0
+}
-- 
2.19.1

Re: [hackers][sbase][PATCH v2] testing: add first shell-based tests

[Silvan Jegen]

[2018-10-07 09:47] Evan Gates 
> On Sun, Oct 7, 2018 at 6:38 AM Silvan Jegen  wrote:
> > * use "ls" instead of "find" in subshell
> 
> Don't do that. Use globs.
> https://mywiki.wooledge.org/ParsingLs

Looks like my version has issues with spaces in filenames then. I will
fix it.


Cheers,

Silvan

[hackers][sbase][PATCH v2] testing: add first shell-based tests

[Silvan Jegen]

We add some shell helper functions to test the expected output of sbase
tools.

In addition to the helper functions themselves we add some tests for
'dirname'.
---
Changes compared to v1:

* use "ls" instead of "find" in subshell
* return number of failed tests in "runalltests" script
* use lowercase variable names more consistently
* print error message on cmp error condition
* make testing Make target depend on all


 Makefile |  6 +-
 tests/dirname.test   | 20 
 tests/runalltests| 12 
 tests/test-common.sh | 42 ++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100755 tests/dirname.test
 create mode 100755 tests/runalltests
 create mode 100644 tests/test-common.sh

diff --git a/Makefile b/Makefile
index 0e421e7..2395b50 100644
--- a/Makefile
+++ b/Makefile
@@ -274,4 +274,8 @@ clean:
rm -f $(BIN) $(OBJ) $(LIB) sbase-box sbase-$(VERSION).tar.gz
rm -f getconf.h
 
-.PHONY: all install uninstall dist sbase-box sbase-box-install 
sbase-box-uninstall clean
+testing: all
+   @cd tests/; \
+   ./runalltests
+
+.PHONY: all install uninstall dist sbase-box sbase-box-install 
sbase-box-uninstall clean testing
diff --git a/tests/dirname.test b/tests/dirname.test
new file mode 100755
index 000..3bc45ca
--- /dev/null
+++ b/tests/dirname.test
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+. ./test-common.sh
+
+#"test name"   "command to execute" "expected output"
+check_stdout "dirname-noarg"   "../dirname" "" && \
+check_stderr "dirname-noarg-stderr" "../dirname" "usage: ../dirname path\n" && 
\
+check_stdout "dirname-non-existing" "../dirname a b c" "" && \
+check_stdout "dirname-slash" "../dirname /" "/\n" && \
+check_stdout "dirname-dashes-slash" "../dirname -- /" "/\n" && \
+check_stdout "dirname-dashes-slash-a" "../dirname -- /a" "/\n"  && \
+check_stdout "dirname-doublequotes" "../dirname \"\"" ".\n" && \
+check_stdout "dirname-slashes" "../dirname ///" "/\n" && \
+check_stdout "dirname-a/b" "../dirname a/b" "a\n" && \
+check_stdout "dirname-a/b/" "../dirname a/b/" "a\n" && \
+check_stdout "dirname-a/b//" "../dirname a/b//" "a\n" && \
+check_stdout "dirname-a" "../dirname a" ".\n" && \
+check_stdout "dirname-a/" "../dirname a/" ".\n" && \
+check_stdout "dirname-/a/b/c" "../dirname /a/b/c" "/a/b\n" && \
+check_stdout "dirname-//a/b/c" "../dirname //a/b/c" "//a/b\n"
diff --git a/tests/runalltests b/tests/runalltests
new file mode 100755
index 000..b6d5b5d
--- /dev/null
+++ b/tests/runalltests
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+for testfile in $(ls *.test); do
+   ./$testfile
+   ret=$(expr $ret + $?)
+done
+
+if [ $ret -gt 0 ]; then
+   printf "%d tests failed\n" $ret
+fi
+
+exit $ret
diff --git a/tests/test-common.sh b/tests/test-common.sh
new file mode 100644
index 000..b1d7572
--- /dev/null
+++ b/tests/test-common.sh
@@ -0,0 +1,42 @@
+check_output() {
+   testname=$1
+   cmdtorun=$2
+   expectedoutput=$3
+   usestdout=$4
+   expoutfile=$(mktemp)
+   actualoutfile=$(mktemp)
+   ret=0
+
+   printf "$expectedoutput" > $expoutfile
+   if [ $usestdout -eq 1 ]; then
+   eval $cmdtorun > $actualoutfile 2> /dev/null
+   else
+   eval $cmdtorun 2> $actualoutfile 1> /dev/null
+   fi
+
+   cmp $expoutfile $actualoutfile  2>&1 > /dev/null
+   if [ $? -eq 1 ]; then
+   printf "$testname:\n"
+   printf "\tWanted:\n"
+   cat $expoutfile
+   printf "\n\tGot:\n"
+   cat $actualoutfile
+   printf "\n\n"
+   ret=1
+   fi
+   if [ $? -eq 2 ]; then
+   printf "cmp error\n"
+   ret=1
+   fi
+
+   rm $expoutfile $actualoutfile
+   return $ret
+}
+
+check_stdout() {
+   check_output "$1" "$2" "$3" 1
+}
+
+check_stderr() {
+   check_output "$1" "$2" "$3" 0
+}
-- 
2.19.0

Re: [hackers][sbase][PATCH] testing: add first shell-based tests

[Silvan Jegen]

Hi Michael

Thanks for having a look!

[2018-09-24 12:41] Michael Forney 
>
> Hi Silvan,
> 
> On 2018-09-09, Silvan Jegen  wrote:
> > We add some shell helper functions to test the expected output of sbase
> > tools. In addition to the helper functions themselves we add some tests
> > for 'dirname'.
> > ---
> > Hi
> >
> > I fixed some of the issues pointed out by Mattias and made the tests
> > runnable from the Makefile.
> >
> > Let me know if there is a chance that we can get some shell-based testing
> > like this into sbase.
> 
> I do like the idea of a test suite for sbase. It is too easy to
> accidentally break something when fixing another bug.
> 
> Shell-based testing seems fine to me. However, I wonder if it might be
> better to make this a separate repository (similar to musl and
> libc-test). That way, you could test any implementation of the POSIX
> utilities.

I am just worried that devs won't be running any tests if there is an
extra hurdle to jump through. If we put tests into a different repo,
do you think it's enough to mention the testing repo and its tests in
the README?

Users could just "git clone" the test repo into the root of the sbase
repo and run the tests from there, I guess.

Do you prefer tests like these in their own repo or in sbase? If you
prefer to put them somewhere else, do you have a preference or can I
just host them wherever?


> >  Makefile |  6 +-
> >  tests/dirname.test   | 20 
> >  tests/runalltests|  5 +
> >  tests/test-common.sh | 37 +
> >  4 files changed, 67 insertions(+), 1 deletion(-)
> >  create mode 100755 tests/dirname.test
> >  create mode 100755 tests/runalltests
> >  create mode 100644 tests/test-common.sh
> >
> > diff --git a/Makefile b/Makefile
> > index 0e421e7..9ddd873 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -274,4 +274,8 @@ clean:
> > rm -f $(BIN) $(OBJ) $(LIB) sbase-box sbase-$(VERSION).tar.gz
> > rm -f getconf.h
> >
> > -.PHONY: all install uninstall dist sbase-box sbase-box-install
> > sbase-box-uninstall clean
> > +testing:
> 
> If this were to go into the sbase repository, the `testing` target
> should depend on the utilities being tested.

Makes sense.


> > +   @cd tests/; \
> > +   ./runalltests
> > +
> > +.PHONY: all install uninstall dist sbase-box sbase-box-install
> > sbase-box-uninstall clean testing
> > diff --git a/tests/dirname.test b/tests/dirname.test
> > new file mode 100755
> > index 000..3bc45ca
> > --- /dev/null
> > +++ b/tests/dirname.test
> > @@ -0,0 +1,20 @@
> > +#!/bin/sh
> > +
> > +. ./test-common.sh
> > +
> > +#"test name"   "command to execute" "expected output"
> > +check_stdout "dirname-noarg"   "../dirname" "" && \
> > +check_stderr "dirname-noarg-stderr" "../dirname" "usage: ../dirname path\n"
> > && \
> > +check_stdout "dirname-non-existing" "../dirname a b c" "" && \
> > +check_stdout "dirname-slash" "../dirname /" "/\n" && \
> > +check_stdout "dirname-dashes-slash" "../dirname -- /" "/\n" && \
> > +check_stdout "dirname-dashes-slash-a" "../dirname -- /a" "/\n"  && \
> > +check_stdout "dirname-doublequotes" "../dirname \"\"" ".\n" && \
> > +check_stdout "dirname-slashes" "../dirname ///" "/\n" && \
> > +check_stdout "dirname-a/b" "../dirname a/b" "a\n" && \
> > +check_stdout "dirname-a/b/" "../dirname a/b/" "a\n" && \
> > +check_stdout "dirname-a/b//" "../dirname a/b//" "a\n" && \
> > +check_stdout "dirname-a" "../dirname a" ".\n" && \
> > +check_stdout "dirname-a/" "../dirname a/" ".\n" && \
> > +check_stdout "dirname-/a/b/c" "../dirname /a/b/c" "/a/b\n" && \
> > +check_stdout "dirname-//a/b/c" "../dirname //a/b/c" "//a/b\n"
> > diff --git a/tests/runalltests b/tests/runalltests
> > new file mode 100755
> > index 000..4cf7933
> > --- /dev/null
> > +++ b/tests/runalltests
> > @@ -0,0 +1,5 @@
> > +#!/bin/sh
> > +
> >

Re: [hackers] [st][patch] Increase the buffer size for escape sequences

[Silvan Jegen]

On Tue, Sep 25, 2018 at 10:05 AM Roberto E. Vargas Caballero
 wrote:
> On Mon, Sep 24, 2018 at 05:45:29PM -0700, Eric Pruitt wrote:
> > I agree that the current buffer is too small. I'm pretty sure I've run
> > into this problem myself with Vim and Bash, but I hadn't gotten around
> > to digging into the problem.
>
> If we go to increase that size, I would go to use dynamic memory. Having
> an array of 1MB statically allocated is a crazy idea (and it is not
> C99 compliant, where the maximun allocated size is 128K).

On my machine, st uses 11MB up to 15MB of memory currently. If we just add 1MB
of statically-allocated space it would blow up that size by 9% in some cases so
I am also in favor of using dynamic memory there.

Re: [hackers] [PATCH][sbase] find: fix flag setting

[Silvan Jegen]

On Mon, Sep 24, 2018 at 10:03 PM Michael Forney  wrote:
> On 7/8/18, Silvan Jegen  wrote:
> > Heyho
> >
> > Found this when running smatch on sbase.
>
> The current code is correct. -H should turn on gflags.h, and turn off
> gflags.l. POSIX says each flag should override the other. I agree that
> the current code is a little confusing.

Yes, you are right. Smatch and me got confused. In my case it was
probably because I
didn't know that assignments in C return the assigned value.


> It would probably be clearer as
>
> case 'H': gflags.h = 1; gflags.l = 0; break;
> case 'L': gflags.h = 0; gflags.l = 1; break;

I also think this would be clearer and shorter.

The current code is correct though so maybe it's not worth to send a patch.

Thanks for having a look!


Cheers,

Silvan

[hackers][sbase][PATCH] testing: add first shell-based tests

[Silvan Jegen]

We add some shell helper functions to test the expected output of sbase
tools. In addition to the helper functions themselves we add some tests
for 'dirname'.
---
Hi

I fixed some of the issues pointed out by Mattias and made the tests
runnable from the Makefile.

Let me know if there is a chance that we can get some shell-based testing
like this into sbase.


 Makefile |  6 +-
 tests/dirname.test   | 20 
 tests/runalltests|  5 +
 tests/test-common.sh | 37 +
 4 files changed, 67 insertions(+), 1 deletion(-)
 create mode 100755 tests/dirname.test
 create mode 100755 tests/runalltests
 create mode 100644 tests/test-common.sh

diff --git a/Makefile b/Makefile
index 0e421e7..9ddd873 100644
--- a/Makefile
+++ b/Makefile
@@ -274,4 +274,8 @@ clean:
rm -f $(BIN) $(OBJ) $(LIB) sbase-box sbase-$(VERSION).tar.gz
rm -f getconf.h
 
-.PHONY: all install uninstall dist sbase-box sbase-box-install 
sbase-box-uninstall clean
+testing:
+   @cd tests/; \
+   ./runalltests
+
+.PHONY: all install uninstall dist sbase-box sbase-box-install 
sbase-box-uninstall clean testing
diff --git a/tests/dirname.test b/tests/dirname.test
new file mode 100755
index 000..3bc45ca
--- /dev/null
+++ b/tests/dirname.test
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+. ./test-common.sh
+
+#"test name"   "command to execute" "expected output"
+check_stdout "dirname-noarg"   "../dirname" "" && \
+check_stderr "dirname-noarg-stderr" "../dirname" "usage: ../dirname path\n" && 
\
+check_stdout "dirname-non-existing" "../dirname a b c" "" && \
+check_stdout "dirname-slash" "../dirname /" "/\n" && \
+check_stdout "dirname-dashes-slash" "../dirname -- /" "/\n" && \
+check_stdout "dirname-dashes-slash-a" "../dirname -- /a" "/\n"  && \
+check_stdout "dirname-doublequotes" "../dirname \"\"" ".\n" && \
+check_stdout "dirname-slashes" "../dirname ///" "/\n" && \
+check_stdout "dirname-a/b" "../dirname a/b" "a\n" && \
+check_stdout "dirname-a/b/" "../dirname a/b/" "a\n" && \
+check_stdout "dirname-a/b//" "../dirname a/b//" "a\n" && \
+check_stdout "dirname-a" "../dirname a" ".\n" && \
+check_stdout "dirname-a/" "../dirname a/" ".\n" && \
+check_stdout "dirname-/a/b/c" "../dirname /a/b/c" "/a/b\n" && \
+check_stdout "dirname-//a/b/c" "../dirname //a/b/c" "//a/b\n"
diff --git a/tests/runalltests b/tests/runalltests
new file mode 100755
index 000..4cf7933
--- /dev/null
+++ b/tests/runalltests
@@ -0,0 +1,5 @@
+#!/bin/sh
+
+for testfile in $(find -name '*.test' -type f); do
+   $testfile
+done
diff --git a/tests/test-common.sh b/tests/test-common.sh
new file mode 100644
index 000..e12fc78
--- /dev/null
+++ b/tests/test-common.sh
@@ -0,0 +1,37 @@
+check_output() {
+   testname=$1
+   cmdtorun=$2
+   expectedOutput=$3
+   usestdout=$4
+   expOutFile=$(mktemp)
+   actualOutFile=$(mktemp)
+   ret=0
+
+   printf "$expectedOutput" > $expOutFile
+   if [ $usestdout -eq 1 ]; then
+   eval $cmdtorun > $actualOutFile 2> /dev/null
+   else
+   eval $cmdtorun 2> $actualOutFile 1> /dev/null
+   fi
+
+   cmp $expOutFile $actualOutFile  2>&1 > /dev/null
+   if [ $? -eq 1 ]; then
+   printf "$testname:\n"
+   printf "\tWanted:\n"
+   cat $expOutFile
+   printf "\n\tGot:\n"
+   cat $actualOutFile
+   ret=1
+   fi
+
+   rm $expOutFile $actualOutFile
+   return $ret
+}
+
+check_stdout() {
+   check_output "$1" "$2" "$3" 1
+}
+
+check_stderr() {
+   check_output "$1" "$2" "$3" 0
+}
-- 
2.18.0

Re: [hackers] [sbase][PATCH v2] Add tests for some utilities

[Silvan Jegen]

Hi Roberto

On Fri, Aug 3, 2018 at 8:37 AM, Roberto E. Vargas Caballero
 wrote:
> Hi,
>
> On Wed, Aug 01, 2018 at 09:16:26PM +0200, Silvan Jegen wrote:
>> > * `echo` is unportable and `printf` should be used instead.
>>
>> Didn't know that echo was not portable. Thought it was just a builtin
>> that should work the same everywhere. It's probably the flags that are
>> the issue...
>
> echo is portable, bad usage of echo is not portable. Use printf
> when you want complex numeric conversions or escape sequences,
> otherwise use echo.
>
>> If we go with option 1) I would like to wait to see which C functionality
>> we would end up needing in the end. Looking at the C code I would postpone
>> until after that decision has been made.
>
> You can do whatever you want, but you should ask to the maintainers first,
> and I am pretty sure they don't agree with the kind of tests that you want to
> implement, so don't lose your time and try to have a conversation first with
> the maintainers. Everything else that is not agreed with them is not going
> to be applied.

Sadly, the maintainers haven't chimed in on this discussion at all
yet. AFAIK Michael is the current maintainer for sbase and of course I
was hoping to elicit some response from any of the maintainers by
discussing testing approaches on the mailing list.

If you are a maintainer, I can tell that we would have to use a
different approach already :P


Cheers,

Silvan

Re: [hackers] [sbase][PATCH v2] Add tests for some utilities

[Silvan Jegen]

On Wed, Aug 01, 2018 at 10:12:54PM +0200, Mattias Andrée wrote:
> On Wed, 1 Aug 2018 22:07:33 +0200
> Mattias Andrée  wrote:
> 
> [...] 
> 
> > On Wed, 1 Aug 2018 21:16:26 +0200
> > Silvan Jegen  wrote:
> > 
> > > On Wed, Aug 01, 2018 at 07:53:18PM +0200, Mattias Andrée wrote:  
> > > > Thank you for your time!
> > > 
> > > Would you be open for working on some (portable) shell code for the most
> > > common tools?  
> > 
> > I am open to it, however I fear that it is not the simpler solution
> > as it may require two implementations of the same functionality.
> 
> Sorry, I'm mixing things up, the idea is to completely replace
> the C version of test-common, not just add another implementation.

That is the plan but the C code that has to be written for the tools
with "special" testing requirements will probably have to check some
stdout/stderr output too. Not having too much duplication between the
C and the shell code would be nice...


> > Have a nice evening,

You too!


Cheers,

Silvan

Re: [hackers] [sbase][PATCH v2] Add tests for some utilities

[Silvan Jegen]

On Wed, Aug 01, 2018 at 10:07:33PM +0200, Mattias Andrée wrote:
> On Wed, 1 Aug 2018 21:16:26 +0200
> Silvan Jegen  wrote:
> 
> [...] 
> 
> > On Wed, Aug 01, 2018 at 07:53:18PM +0200, Mattias Andrée wrote:
> > > Thank you for your time!  
> > > * uname:
> > >   Most of uname can be tested in ed, however, the output
> 
> s/ed/sh/ # I guess you understood that, but I cannot stand not correcting it.

I think there was just a lifted eyebrow that is all...


> > >
> > > [...]
> > >
> > > As I see it, the most complex parts of the C code are:
> > > 
> > > * start_process:
> > >   It's probably enough to split out some code to
> > >   separate functions: the `if (in->flags & DATA)`,
> > >   the dup2–close loops at the end.
> > > 
> > > * wait_process:
> > >   Instead of ready all file descriptors as fast as
> > >   possible, the they could probably be read in order.
> > > 
> > > * check_output_test:
> > >   It's probably enough to add a few short comments
> > >   and improve variable names.
> > > 
> > > * print_failure:
> > >   It's probably enough to add some empty lines add a
> > >   few short comments.
> > > 
> > > The other parts are pretty straight forward.  
> > 
> > If we go with option 1) I would like to wait to see which C functionality
> > we would end up needing in the end. Looking at the C code I would postpone
> > until after that decision has been made.
> 
> I will make a sh reimplementation, but since I'm back at work
> now, it will take some time. In the meanwhile, why not enjoy
> my new painting.

Haha, the painting is a thing of beauty!

It taking time is not an issue. The earlier you send any code, the faster
everyone will be able to give feedback though!


Cheers,

Silvan


signature.asc
Description: PGP signature

Re: [hackers] [sbase][PATCH v2] Add tests for some utilities

[Silvan Jegen]

On Wed, Aug 01, 2018 at 07:53:18PM +0200, Mattias Andrée wrote:
> Thank you for your time!

Thank you for all your work! :P


> The common code is 590 lines of code, including:
> 
> * 102 lines of code related to identifying the error when the
>   test fails.
> 
> * 14 lines of code for properly killing processes on failure,
>   abortion, and when a test case hangs.
> 
> * 32 lines of code, plus 13 of the lines counted above,
>   for supporting concurrent tests.
> 
> This leaves 442 lines for the fundamental stuff and a few
> lines to support printing all errors rather than just the
> first error.
> 
> Some note on your sh code (since you wrote “crappy and
> probably non-portable”, you are probably aware of this,
> but just in case):
> 
> * `source` is a Bash synonym for the portable `.` command.

Yeah, that sounds familiar.
 

> * `echo` is unportable and `printf` should be used instead.

Didn't know that echo was not portable. Thought it was just a builtin
that should work the same everywhere. It's probably the flags that are
the issue...


> * I have never seen `&>` before, so I my guess is that it
>   is a Bashism.

Yeah, seems likely. It's supposed to redirect both stderr and stdout. "sh"
did not complain about it but that doesn't mean much...


> * It looks like whichever file descriptor is not being
>   inspected by `check_output` is printed the inherited
>   file descriptor rather than to /dev/null.

Printing behaviour of the tests should looked at and fixed for sure.


> * I think it would be better to have something like:
> 
>   set -e
> 
>   run "test name" "./dirname //"
>   check_stderr ""
>   check_stdout / || check_stdout //
>   check_exit 0
> 
> Your sh code, with check_exit added, covers most current tests.
> However, it every time the usage output is modified it has to
> be change in the test case, which I guess is acceptable but
> undesirable. The tests that are currently implemented

I think that is working as intended. It's a behaviour change in the code
and should result in an error (unless we decide that we don't want to
test the usage output of course).


> and which need to be handled specially are:
> 
> * sleep:
>   This can be done with sh. With some adaption to the sh
>   code, tests can also be done in parallel as it is in
>   the C code.
> 
> * test:
>   test takes a lot of time to test, which is why multiple
>   tests are run in parallel in the C code. Like tty(1),
>   this test also requires the creation of terminals, but
>   it also requires the creation of sockets.
> 
> * time:
>   Requires setitimer(3) and pause(3).
> 
> * tty:
>   This test requires the creation of terminals.
> 
> * uname:
>   Most of uname can be tested in ed, however, the output
>   of uname with only one flag applied requires the uname
>   system call.
> 
> * whoami:
>   The user name can be retrieved via $LOGNAME according
>   to POSIX, however this requires that your login program
>   actually sets it. Additionally (and this should be added
>   to the test) when whoami is called from a program with
>   setuid the owner of the program should be printed (i.e.
>   the effective user), not real user which is stored in
>   $LOGNAME.
> 
> Additionally env, time, and yes requires identifying which signal
> the processed was killed by. I have never done this in sh, but I
> understand that it should be doable. time and env also requires a
> way to kill the process it runs with a specific signal. Furthermore
> the test for tty should include a case where stdin does not exist,
> which for the moment is not done. All tests excepts test and sleep
> fundamentally requires support for stdout, but they also use stderr
> for checking error support.
> 
> These tests require all the features in the C code, except the
> extra functionally enumerated at the beginning of this mail and
> the support for binary data. These are tests we should focus on,
> whichever solution is found for them should be applied to the
> rest tests since those require almost only subset of the
> functionality. The only extra functionality other tests require

Thanks for compiling the list of tools to be handled in a special way.

I do feel that it would be better to find the simplest solution that
works for most of the tests. For the rest we can then decide how to
handle them in a minimal way (most likely some custom C code for each
one. Common parts should still be shared of course).


> is support for binary data; which I really don't think warrant
> a separate solution.
> 
> The following utilities (from both sbase and ubase) still need
> tests, as well as the utilities listed under ubase's TODO. I
> have most of the work already done for the utilities marked
> with an asterisk.
> 
>   at  awk bc  cal
>   cat chgrp   chmod   chown
>   chroot  chvt

Re: [hackers] [sbase][PATCH v2] Add tests for some utilities

[Silvan Jegen]

Hi Mattias!

On Wed, Jul 11, 2018 at 09:39:23PM +0200, Mattias Andrée wrote:
> The following utilities are tested:
> - basename(1)
> - dirname(1)
> - echo(1)
> - false(1)
> - link(1)
> - printenv(1)
> - sleep(1)
> - test(1)
> - time(1)
> - true(1)
> - tty(1)
> - uname(1)
> - unexpand(1)
> - unlink(1)
> - whoami(1)
> - yes(1)
> 
> Some tests contain "#ifdef TODO", these tests current
> fail, but there are patches submitted for most of them.
> There are not patches submitted for fixing the
> "#ifdef TODO"s in expand.test.c and unexpand.test.c.
> 
> Signed-off-by: Mattias Andrée 

Sorry for not getting around to looking at this earlier.

I definitely think we should have unit tests for sbase (and other
projects?) as soon as possible. What concerns me with your approach is
that we have about 700 lines of C code in testing-common.{c,h} of which
I feel quite a bit could be dropped.

I have written some (crappy and probably non-portable) shell script
functions to check the stdout and stderr of a process. It's about 40
lines. I also converted your tests for dirname to use these functions
(both files attached. The test coverage is not exactly the same but
relatively similar).

I wonder if we couldn't use some cleaned-up version of the shell script
functions for the easy test cases that only check stdout and stderr
output and your custom C code for the more specialised test cases (like
'tty').

What do you think?


Cheers,

Silvan


> ---
>  Makefile|  45 -
>  basename.test.c |  68 +++
>  dirname.test.c  |  55 ++
>  echo.test.c |  51 ++
>  expand.test.c   |  92 ++
>  false.test.c|  32 
>  link.test.c |  58 ++
>  printenv.test.c |  79 
>  sleep.test.c|  53 ++
>  test-common.c   | 560 
> 
>  test-common.h   | 219 ++
>  test.test.c | 408 +
>  time.test.c | 218 ++
>  true.test.c |  31 
>  tty.test.c  |  44 +
>  uname.test.c| 283 
>  unexpand.test.c |  97 ++
>  unlink.test.c   |  56 ++
>  whoami.test.c   |  38 
>  yes.test.c  | 131 +
>  20 files changed, 2614 insertions(+), 4 deletions(-)
>
> [snip]


test-common.sh
Description: Bourne shell script
#! /bin/sh

source ./test-common.sh

#"test name"   "command to execute" "expected output"
check_stdout "dirname-noarg"   "./dirname" "" && \
check_stderr "dirname-noarg-stderr" "./dirname" "usage: ./dirname path\n" && \
check_stdout "dirname-non-existing" "./dirname a b c" "" && \
check_stdout "dirname-slash" "./dirname /" "/\n" && \
check_stdout "dirname-dashes-slash" "./dirname -- /" "/\n" && \
check_stdout "dirname-dashes-slash-a" "./dirname -- /a" "/\n"  && \
check_stdout "dirname-doublequotes" "./dirname \"\"" ".\n" && \
check_stdout "dirname-slashes" "./dirname ///" "/\n" && \
check_stdout "dirname-a/b" "./dirname a/b" "a\n" && \
check_stdout "dirname-a/b/" "./dirname a/b/" "a\n" && \
check_stdout "dirname-a/b//" "./dirname a/b//" "a\n" && \
check_stdout "dirname-a" "./dirname a" ".\n" && \
check_stdout "dirname-a/" "./dirname a/" ".\n" && \
check_stdout "dirname-/a/b/c" "./dirname /a/b/c" "/a/b\n" && \
check_stdout "dirname-//a/b/c" "./dirname //a/b/c" "//a/b\n"

[hackers] [PATCH][sbase] find: fix flag setting

[Silvan Jegen]

Heyho

Found this when running smatch on sbase.


Cheers,

Silvan
From: Silvan Jegen 
Date: Sun, 8 Jul 2018 14:42:55 +0200
Subject: [PATCH][sbase] find: fix flag setting

Most likely, this is what was intended.
---
 find.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/find.c b/find.c
index e095015..146d6ef 100644
--- a/find.c
+++ b/find.c
@@ -1023,8 +1023,8 @@ main(int argc, char **argv)
 	struct tok *t;
 
 	ARGBEGIN {
-	case 'H': gflags.l = !(gflags.h = 1); break;
-	case 'L': gflags.h = !(gflags.l = 1); break;
+	case 'H': gflags.l = !(gflags.h == 1); break;
+	case 'L': gflags.h = !(gflags.l == 1); break;
 	default : usage();
 	} ARGEND
 
-- 
2.17.1

Re: [hackers] [sbase][PATCH] Add test framework with a test for tty(1)

[Silvan Jegen]

Hi Mattias

Just some comments below (please ignore the mangled formatting.)

On Sat, Jul 7, 2018 at 11:26 PM, Mattias Andrée  wrote:
> Signed-off-by: Mattias Andrée 
> ---
>  Makefile  |  20 +-
>  test-common.c | 823 
> ++
>  test-common.h | 190 ++
>  tty.test.c|  26 ++
>  4 files changed, 1055 insertions(+), 4 deletions(-)
>  create mode 100644 test-common.c
>  create mode 100644 test-common.h
>  create mode 100644 tty.test.c
>
> diff --git a/Makefile b/Makefile
> index 0e421e7..005cf13 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,7 +1,7 @@
>  include config.mk
>
>  .SUFFIXES:
> -.SUFFIXES: .o .c
> +.SUFFIXES: .test .test.o .o .c
>
>  HDR =\
>  arg.h\
> @@ -19,7 +19,8 @@ HDR =\
>  sha512-256.h\
>  text.h\
>  utf.h\
> - util.h
> + util.h\
> + test-common.h
>
>  LIBUTF = libutf.a
>  LIBUTFSRC =\
> @@ -181,9 +182,12 @@ BIN =\
>  xinstall\
>  yes
>
> +TEST =\
> + tty.test
> +
>  LIBUTFOBJ = $(LIBUTFSRC:.c=.o)
>  LIBUTILOBJ = $(LIBUTILSRC:.c=.o)
> -OBJ = $(BIN:=.o) $(LIBUTFOBJ) $(LIBUTILOBJ)
> +OBJ = $(BIN:=.o) $(TEST:=.o) test-common.o $(LIBUTFOBJ) $(LIBUTILOBJ)
>  SRC = $(BIN:=.c)
>  MAN = $(BIN:=.1)
>
> @@ -193,12 +197,17 @@ $(BIN): $(LIB) $(@:=.o)
>
>  $(OBJ): $(HDR) config.mk
>
> +$(TEST): $(@:=.o) test-common.o
> +
>  .o:
>  $(CC) $(LDFLAGS) -o $@ $< $(LIB)
>
>  .c.o:
>  $(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<
>
> +.test.o.test:
> + $(CC) $(LDFLAGS) -o $@ $< test-common.o
> +
>  $(LIBUTF): $(LIBUTFOBJ)
>  $(AR) rc $@ $?
>  $(RANLIB) $@
> @@ -212,6 +221,9 @@ getconf.o: getconf.h
>  getconf.h: getconf.sh
>  ./getconf.sh > $@
>
> +check: $(TEST) $(BIN)
> + @set -e; for f in $(TEST); do echo ./$$f; ./$$f; done
> +
>  install: all
>  mkdir -p $(DESTDIR)$(PREFIX)/bin
>  cp -f $(BIN) $(DESTDIR)$(PREFIX)/bin
> @@ -271,7 +283,7 @@ sbase-box-uninstall: uninstall
>  cd $(DESTDIR)$(PREFIX)/bin && rm -f sbase-box
>
>  clean:
> - rm -f $(BIN) $(OBJ) $(LIB) sbase-box sbase-$(VERSION).tar.gz
> + rm -f $(BIN) $(TEST) $(OBJ) $(LIB) sbase-box sbase-$(VERSION).tar.gz
>  rm -f getconf.h
>
>  .PHONY: all install uninstall dist sbase-box sbase-box-install 
> sbase-box-uninstall clean
> diff --git a/test-common.c b/test-common.c
> new file mode 100644
> index 000..458b094
> --- /dev/null
> +++ b/test-common.c
> @@ -0,0 +1,823 @@
> +/* See LICENSE file for copyright and license details. */
> +#include "test-common.h"
> +
> +struct Counter {
> + const char *name;
> + size_t value;
> +};
> +
> +const char *test_file = NULL;
> +int test_line = 0;
> +int main_ret = 0;
> +int timeout = 10;
> +int pdeath_sig = SIGINT;
> +void (*atfork)(void) = NULL;
> +
> +static struct Counter counters[16];
> +static size_t ncounters = 0;
> +static pid_t async_pids[1024];
> +static size_t async_npids = 0;
> +
> +static void
> +eperror(const char *prefix)
> +{
> + perror(prefix);
> + fflush(stderr);
> + exit(64);
> +}
> +
> +struct Process *
> +stdin_text(struct Process *proc, char *s)
> +{
> + proc->input[0].data = s;
> + proc->input[0].flags &= ~IO_STREAM_BINARY;
> + return proc;
> +}
> +
> +struct Process *
> +stdin_bin(struct Process *proc, char *s, size_t n)
> +{
> + proc->input[0].data = s;
> + proc->input[0].len = n;
> + proc->input[0].flags |= IO_STREAM_BINARY;
> + return proc;
> +}
> +
> +struct Process *
> +stdin_fds(struct Process *proc, int input_fd, int output_fd)
> +{
> + proc->input[0].flags &= ~IO_STREAM_DATA;
> + proc->input[0].input_fd = input_fd;
> + proc->input[0].output_fd = output_fd;
> + return proc;
> +}
> +
> +struct Process *
> +stdin_type(struct Process *proc, int type)
> +{
> + proc->input[0].flags &= ~IO_STREAM_CREATE_MASK;
> + proc->input[0].flags |= type;
> + return proc;
> +}
> +
> +struct Process *
> +stdout_fds(struct Process *proc, int input_fd, int output_fd)
> +{
> + proc->output[0].flags &= ~IO_STREAM_DATA;
> + proc->output[0].input_fd = input_fd;
> + proc->output[0].output_fd = output_fd;
> + return proc;
> +}
> +
> +struct Process *
> +stdout_type(struct Process *proc, int type)
> +{
> + proc->output[0].flags &= ~IO_STREAM_CREATE_MASK;
> + proc->output[0].flags |= type;
> + return proc;
> +}
> +
> +struct Process *
> +stderr_fds(struct Process *proc, int input_fd, int output_fd)
> +{
> + proc->output[1].flags &= ~IO_STREAM_DATA;
> + proc->output[1].input_fd = input_fd;
> + proc->output[1].output_fd = output_fd;
> + return proc;
> +}
> +
> +struct Process *
> +stderr_type(struct Process *proc, int type)
> +{
> + proc->output[1].flags &= ~IO_STREAM_CREATE_MASK;
> + proc->output[1].flags |= type;
> + return proc;
> +}
> +
> +struct Process *
> +set_preexec(struct Process *proc, void (*preexec)(struct Process *))
> +{
> + proc->preexec = preexec;
> + return proc;
> +}
> +
> +struct Process *
> +set_async(struct Process *proc)
> +{
> + proc->flags |= PROCESS_ASYNC;
> + return proc;
> +}
> +
> +struct Process *
> +set_setsid(struct Process *proc)
> +{
> + proc->flags |= PROCESS_SETSID;
> + return proc;
> +}
> +
> +void
> 

Re: [hackers] [sbase][PATCH] basename: support --

[Silvan Jegen]

On Fri, Jul 6, 2018 at 11:19 PM, Mattias Andrée  wrote:
> POSIX-2017 clarifies that -- and normal option parsing must be supported.
> See EXAMPLES in basename(1p).
>
> Signed-off-by: Mattias Andrée 
> ---
>  basename.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)

Looks good!

I also tested it.


Cheers,

Silvan

> diff --git a/basename.c b/basename.c
> index d211799..94a2848 100644
> --- a/basename.c
> +++ b/basename.c
> @@ -17,7 +17,10 @@ main(int argc, char *argv[])
> ssize_t off;
> char *p;
>
> -   argv0 = *argv, argv0 ? (argc--, argv++) : (void *)0;
> +   ARGBEGIN {
> +   default:
> +   usage();
> +   } ARGEND
>
> if (argc != 1 && argc != 2)
> usage();
> --
> 2.11.1
>
>

Re: [hackers] [sbase][PATCH] dirname: support --

[Silvan Jegen]

On Fri, Jul 6, 2018 at 11:19 PM, Mattias Andrée  wrote:
> POSIX-2017 clarifies that -- and normal option parsing must be supported.
> See EXAMPLES in basename(1p)
>
> Signed-off-by: Mattias Andrée 
> ---
>  dirname.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)

Looks good!

I also tested it.


Cheers,

Silvan


> diff --git a/dirname.c b/dirname.c
> index 8392bc0..45e1a7e 100644
> --- a/dirname.c
> +++ b/dirname.c
> @@ -13,7 +13,10 @@ usage(void)
>  int
>  main(int argc, char *argv[])
>  {
> -   argv0 = *argv, argv0 ? (argc--, argv++) : (void *)0;
> +   ARGBEGIN {
> +   default:
> +   usage();
> +   } ARGEND
>
> if (argc != 1)
> usage();
> --
> 2.11.1
>
>

Re: [hackers] [st][PATCH] Set minimum window size to avoid crash when resizing out of bounds

[Silvan Jegen]

Hi

Thanks for the patch!

Some comments below.

On Wed, May 16, 2018 at 10:36 PM  wrote:

> From: Michael Buch 

> The termianl window is created without lower/upper bounds for window
width/height. On resizing the terminal to a size of 1x1 or below we crash.
This patch sets the minimum window height and width to prevent resizing
past this size thus handling the scenario similar to other terminals.

It would be nice to have this commit message wrapped at around 80 chars.

There is also a typo: s/termianl/terminal/


> ---
>   x.c | 6 --
>   1 file changed, 4 insertions(+), 2 deletions(-)

> diff --git a/x.c b/x.c
> index c0bd890..00ea444 100644
> --- a/x.c
> +++ b/x.c
> @@ -788,15 +788,17 @@ xhints(void)

>  sizeh = XAllocSizeHints();

> -   sizeh->flags = PSize | PResizeInc | PBaseSize;
> +   sizeh->flags = PSize | PResizeInc | PBaseSize | PMinSize;
>  sizeh->height = win.h;
>  sizeh->width = win.w;
>  sizeh->height_inc = win.ch;
>  sizeh->width_inc = win.cw;
>  sizeh->base_height = 2 * borderpx;
>  sizeh->base_width = 2 * borderpx;
> +   sizeh->min_height = 2 * win.ch;
> +   sizeh->min_width = 2 * win.cw;
>  if (xw.isfixed) {
> -   sizeh->flags |= PMaxSize | PMinSize;
> +   sizeh->flags |= PMaxSize;

I think in the isfixed case we don't want the size to change at all. So we
should keep both the PMaxSize and PMinSize flags set.

Other than that the patch looks good to me!


Cheers,

Silvan

>  sizeh->min_width = sizeh->max_width = win.w;
>  sizeh->min_height = sizeh->max_height = win.h;
>  }
> --
> 2.17.0

Re: [hackers] Re: [st][PATCH] Port the copyurl patch to the 0.8.1 st release. Mainly fix usage of depracted selcopy

[Silvan Jegen]

Hi

On Wed, Apr 18, 2018 at 3:04 AM, Michael Buch  wrote:
> Apologies, just realized I should've added it to the Wiki patches section
> instead of sending it here.

Sending it here for review at the same time as you add it to the
patches section seems like the right thing to do.


Cheers,

Silvan

Re: [hackers] [PATCH] [dwm] Make unset property fallback strings configurable

[Silvan Jegen]

Hi

On Sun, Apr 01, 2018 at 02:45:23PM -0700, Eric Pruitt wrote:
> 
> Since the default rule matching does substring comparisons, using the
> fixed string "broken" as a fallback value can complicate or make
> unambiguous matching impossible, so this change makes various fallback
> strings for unset properties configurable via config.h.
> ---
>  config.def.h | 5 +
>  dwm.c| 7 +++
>  2 files changed, 8 insertions(+), 4 deletions(-)

The patch did not apply for me when saving the attached patch and applying
it with "git apply" on dwm HEAD at

3bd8466e93b2c81be86e67c6ecdda4e1d240fe4b

I get the following error:

error: git diff header lacks filename information when removing 1
leading pathname component (line 5)

Can someone else confirm that it is working on their repo? Maybe it's my
setup that's broken somehow.


Cheers,

Silvan

Re: [hackers] [dwm][PATCH] ColBorder has been moved to the enum with ColFg and ColBg.

[Silvan Jegen]

Dear Laslo

On Thu, Mar 15, 2018 at 10:39 PM, Laslo Hunhold <d...@frign.de> wrote:
> On Thu, 15 Mar 2018 22:17:25 +0100
> Silvan Jegen <s.je...@gmail.com> wrote:
>
> Dear Silvan,
>
>> I see, thanks!
>>
>> Still sounds to me like having patches as attachments just causes me
>> to have to change my default configuration though.
>>
>> What is the advantage of attaching the patches instead of just sending
>> them inline, I wonder.
>
> because not everyone uses Mutt. Having dedicated attachments is
> consistent when you send multiple patches, makes it easier to save them
> somewhere for people who use "normal" mail-clients (no offense against
> mutt of course).

I am sure they work great with nmh too! :P


> The most prominent reason I see though is that when people browse the
> mailing list archives of suckless.org, it's pretty much impossible to
> extract the patch files from the archived messages, however it is
> trivial when they are attachments.

That is a good point. I have actually been quite frustrated with not
being able to download raw emails from mailing lists for a while now.
I assume the common email archivers don't allow that because it would
make it too easy to harvest email addresses? Not sure.


Cheers,

Silvan

Re: [hackers] [dwm][PATCH] ColBorder has been moved to the enum with ColFg and ColBg.

[Silvan Jegen]

On Thu, Mar 15, 2018 at 10:38 PM, Eric Pruitt <eric.pru...@gmail.com> wrote:
> On Thu, Mar 15, 2018 at 10:17:25PM +0100, Silvan Jegen wrote:
>> Still sounds to me like having patches as attachments just causes me to
>> have to change my default configuration though.
>
> Change the world or change your configuration: the choice is yours! By
> adding a handful of bytes to your Mutt files, you can get the patches to
> effectively be inlined regardless of how the sender actually sent them.

Yeah, I can live with changing my mutt config in this case...


>> What is the advantage of attaching the patches instead of just sending
>> them inline, I wonder.
>
> I think a large part of what people prefer will depend on how they
> access the mailing list. I imagine it's easier for most common mail
> clients to deal with inline patches when it comes to reviewing &
> commenting, but I prefer attachments because:
>
> - They're easier for me to send. The various mail functions that Git
>   provides make some assumptions about one's MUA that, IMO, make it
>   tedious (albeit scriptable) to use them with Mutt for **sending**
>   patches. If the machine I'm using isn't configured to send emails,
>   it's easier for me to move a patch / diff file between machines rather
>   than a Git-generated email that I'll have to re-edit to fix things
>   like the "From:" header.

It's true that I have to fixup the headers but sending is handled
quite conveniently by "git send-email" (after it has been configured).
Just pass the directory containing the patches or the patch file names
as arguments to it and off you go. As you pointed out, it's also
easily scriptable.


> - I've seen inline patches get munged by MUAs. I have personally sent
>   some broken patches because I forgot to disable automatic text
>   wrapping of an email with an inline patch.

Obviously that doesn't happen with send-email.


> - When people send batches of related patches, sending a single email
>   means that all of the patches will be immediately accessible whereas
>   separate emails might not arrive in close proximity to one another or
>   even in order because of things like rate limit and spam
>   countermeasures.

While true in theory, in practice send-email threads a series of
patches so that they will always show up in the right order (provided
they were not spam-filtered and that your MUA supports email threads).


Cheers,

Silvan

Re: [hackers] [dwm][PATCH] ColBorder has been moved to the enum with ColFg and ColBg.

[Silvan Jegen]

On Thu, Mar 15, 2018 at 12:51:38PM -0700, Eric Pruitt wrote:
> On Thu, Mar 15, 2018 at 08:44:02PM +0100, Silvan Jegen wrote:
> > Ah, I wasn't aware that the tags also work on attachments, neat. Thanks
> > for the tip!
> >
> > That means both are equally convenient for me. However, if you send
> > patches inline (not as attachments) commenting on specific parts of
> > patches becomes as easy as clicking "reply" (or "r"...).
> 
> If you add the appropriate MIME types to Mutt's auto_view list
> (http://www.mutt.org/doc/manual/#auto-view), the contents of the
> attached patches will be included in your reply, and it will show the
> patches without you having to switch to attachment mode first.

I see, thanks!

Still sounds to me like having patches as attachments just causes me to
have to change my default configuration though.

What is the advantage of attaching the patches instead of just sending
them inline, I wonder.


Cheers,

Silvan

Re: [hackers] [dwm][PATCH] ColBorder has been moved to the enum with ColFg and ColBg.

[Silvan Jegen]

On Thu, Mar 15, 2018 at 12:35:07PM -0700, Eric Pruitt wrote:
> On Thu, Mar 15, 2018 at 08:28:26PM +0100, Silvan Jegen wrote:
> > > That depends on what email client you use. Mutt makes piping both whole
> > > messages and individual attachments to arbitrary commands pretty
> > > painless IMO.
> >
> > I use mutt as well but I don't know how to deal with attachments 
> > efficiently.
> >
> > Mails I can just tag so to apply a series of patches sent with "git
> > send-email" I tag them and pipe them to "git am". If several patches are
> > attached to a mail, I haven't found a good way to download them all in
> > one go and then apply them in batch to a repo.
> >
> > Would definitely like to learn about a more efficient way to deal with
> > attached patches in mutt.
> 
> Move the cursor to the message in question then invoke view-attachments
> ("v" by default). From there you can use pipe-message ("|") with
> git-apply, and you can tag multiple attachments using tag-message ("t")
> just as you would with messages.

Ah, I wasn't aware that the tags also work on attachments, neat. Thanks
for the tip!

That means both are equally convenient for me. However, if you send
patches inline (not as attachments) commenting on specific parts of
patches becomes as easy as clicking "reply" (or "r"...).


Cheers,

Silvan

Re: [hackers] [dwm][PATCH] ColBorder has been moved to the enum with ColFg and ColBg.

[Silvan Jegen]

On Thu, Mar 15, 2018 at 09:53:41AM -0700, Eric Pruitt wrote:
> On Thu, Mar 15, 2018 at 05:11:10PM +0100, Silvan Jegen wrote:
> > The disadvantage of this is that you will have to save those
> > attachments somewhere and then apply them. When using "git send-email"
> > you can just feed the emails directly to "git am".
> 
> That depends on what email client you use. Mutt makes piping both whole
> messages and individual attachments to arbitrary commands pretty
> painless IMO.

I use mutt as well but I don't know how to deal with attachments efficiently.

Mails I can just tag so to apply a series of patches sent with "git
send-email" I tag them and pipe them to "git am". If several patches are
attached to a mail, I haven't found a good way to download them all in
one go and then apply them in batch to a repo.

Would definitely like to learn about a more efficient way to deal with
attached patches in mutt.


Cheers,

Silvan

Re: [hackers] [dwm][PATCH] ColBorder has been moved to the enum with ColFg and ColBg.

[Silvan Jegen]

On Thu, Mar 15, 2018 at 4:28 PM, Laslo Hunhold  wrote:
> On Wed, 14 Mar 2018 13:24:53 -0400
> Christopher Drelich  wrote:
>
> Hey Christopher,
>
>> Ok, so that is an absolute requirement then, and the only issue? Was
>> doing the patching in a very unconfigured chroot environment on a
>> temporary computer, so hadn't setup email on there. I will for next
>> time.
>
> no, it's not a requirement. I personally dislike git-send-email and
> prefer git format-patch and attaching the patches as files to a
> separate e-mail.

The disadvantage of this is that you will have to save those
attachments somewhere and then apply them. When using "git send-email"
you can just feed the emails directly to "git am".

To be fair, I feel that my mail client is better at dealing with raw
emails than with attachments so "git send-email" is more convenient
for me. That may not be the case for other people.


Cheers,

Silvan

Re: [hackers] [dwm][PATCH] ColBorder has been moved to the enum with ColFg and ColBg.

[Silvan Jegen]

Hi

On Wed, Mar 14, 2018 at 6:24 PM, Christopher Drelich <c...@cdrakka.com> wrote:
> Ok, so that is an absolute requirement then, and the only issue? Was doing
> the patching in a very unconfigured chroot environment on a temporary
> computer, so hadn't setup email on there. I will for next time.

I don't think it's an absolute requirement but using "git send-email"
makes sure you don't end up with email headers in the body of your
email, like it seems to be the case in the patch of yours:

On Wed, Mar 14, 2018 at 10:12:55AM -0400, Christopher Drelich wrote:
>My first patch, I'm hoping this is the way you want them
> submitted. I
>couldn't find any reason for ColBorder to be a #define while ColFg
> and
>ColBg are in an enum together.
>---
>From eefea3c310db3c9460c5fdee3e8a8d0cb45c9819 Mon Sep 17 00:00:00
> 2001
>From: ude <ude@ude>
>Date: Wed, 14 Mar 2018 10:01:00 -0400
>Subject: [PATCH] ColBorder has been moved to the enum with ColFg

^

Looks like email headers to me and I am not sure how well 'git am'
deals with these.


Cheers,

Silvan


> and
>ColBg.
>>---
> drw.h | 2 +-
> dwm.c | 1 -
>index 4c67419..4bcd5ad 100644
> ...

> I know there are plenty of patches for dwm available, and I've made plenty
> of my own that are not meant for mainstream (I'll clean them up and submit
> someday.) I was wondering though, are there any things that are wanted for
> mainstream dwm? Happy to help. The TODO file seemed rather old, and I wasn't
> quite clear on what was meant by the updategeom hook. I also don't know what
> an appropriate change for scrot would be, as something with that
> functionality is available as a patch already (I'm guessing though something
> about it doesn't conform to mainstream dwm?)
> Chris
>
> On Wed, Mar 14, 2018 at 1:19 PM, Silvan Jegen <s.je...@gmail.com> wrote:
>>
>> Hi
>>
>> On Wed, Mar 14, 2018 at 5:58 PM, Christopher Drelich <c...@cdrakka.com>
>> wrote:
>> > Any idea what I might have done wrong in creating the patch? I figure
>> > learning now will save us all time in the future.
>> >
>> > I followed the instructions on the website and used a fresh pull of dwm.
>> > Looking at what's in git, it seems the same to me, other than my email
>> > as
>> > you noted.
>>
>> You should use git send-email to send your patches by email (this
>> command consumes the output of git format-patch). I just saw that
>> https://suckless.org/hacking does not mention this...
>>
>>
>> Cheers,
>>
>> Silvan
>>
>>
>> > Chris
>> >
>> > On Wed, Mar 14, 2018 at 12:48 PM, Hiltjo Posthuma
>> > <hil...@codemadness.org>
>> > wrote:
>> >>
>> >> On Wed, Mar 14, 2018 at 10:12:55AM -0400, Christopher Drelich wrote:
>> >> >My first patch, I'm hoping this is the way you want them
>> >> > submitted. I
>> >> >couldn't find any reason for ColBorder to be a #define while ColFg
>> >> > and
>> >> >ColBg are in an enum together.
>> >> >---
>> >> >From eefea3c310db3c9460c5fdee3e8a8d0cb45c9819 Mon Sep 17 00:00:00
>> >> > 2001
>> >> >From: ude <ude@ude>
>> >> >Date: Wed, 14 Mar 2018 10:01:00 -0400
>> >> >Subject: [PATCH] ColBorder has been moved to the enum with ColFg
>> >> > and
>> >> >ColBg.
>> >> >---
>> >> > drw.h | 2 +-
>> >> > dwm.c | 1 -
>> >> > 2 files changed, 1 insertion(+), 2 deletions(-)
>> >> >diff --git a/drw.h b/drw.h
>> >> >index 4c67419..4bcd5ad 100644
>> >> >--- a/drw.h
>> >> >+++ b/drw.h
>> >> >@@ -12,7 +12,7 @@ typedef struct Fnt {
>> >> >  struct Fnt *next;
>> >> > } Fnt;
>> >> >
>> >> >-enum { ColFg, ColBg }; /* Clr scheme index */
>> >> >+enum { ColFg, ColBg, ColBorder }; /* Clr scheme index */
>> >> > typedef XftColor Clr;
>> >> >
>> >> > typedef struct {
>> >> >diff --git a/dwm.c b/dwm.c
>> >> >index ec6a27c..ab16c75 100644
>> >> >--- a/dwm.c
>> >> >+++ b/dwm.c
>> >> >@@ -56,7 +56,6 @@
>> >> > #define HEIGHT(X)   ((X)->h + 2 * (X)->bw)
>> >> > #define TAGMASK ((1 << LENGTH(tags)) - 1)
>> >> > #define TEXTW(X)(drw_fontset_getwidth(drw, (X)) +
>> >> >lrpad)
>> >> >-#define ColBorder   2
>> >> >
>> >> > /* enums */
>> >> > enum { CurNormal, CurResize, CurMove, CurLast }; /* cursor */
>> >> >--
>> >> >2.7.4
>> >>
>> >> Hey,
>> >>
>> >> Thanks for your patch.
>> >>
>> >> The patch didn't apply for me and your e-mail is changed in the commit.
>> >>
>> >> I've fixed this and pushed the commit to master.
>> >>
>> >> --
>> >> Kind regards,
>> >> Hiltjo
>> >>
>> >
>>
>

Re: [hackers] [dwm][PATCH] ColBorder has been moved to the enum with ColFg and ColBg.

[Silvan Jegen]

Hi

On Wed, Mar 14, 2018 at 5:58 PM, Christopher Drelich  wrote:
> Any idea what I might have done wrong in creating the patch? I figure
> learning now will save us all time in the future.
>
> I followed the instructions on the website and used a fresh pull of dwm.
> Looking at what's in git, it seems the same to me, other than my email as
> you noted.

You should use git send-email to send your patches by email (this
command consumes the output of git format-patch). I just saw that
https://suckless.org/hacking does not mention this...


Cheers,

Silvan


> Chris
>
> On Wed, Mar 14, 2018 at 12:48 PM, Hiltjo Posthuma 
> wrote:
>>
>> On Wed, Mar 14, 2018 at 10:12:55AM -0400, Christopher Drelich wrote:
>> >My first patch, I'm hoping this is the way you want them submitted. I
>> >couldn't find any reason for ColBorder to be a #define while ColFg
>> > and
>> >ColBg are in an enum together.
>> >---
>> >From eefea3c310db3c9460c5fdee3e8a8d0cb45c9819 Mon Sep 17 00:00:00
>> > 2001
>> >From: ude 
>> >Date: Wed, 14 Mar 2018 10:01:00 -0400
>> >Subject: [PATCH] ColBorder has been moved to the enum with ColFg and
>> >ColBg.
>> >---
>> > drw.h | 2 +-
>> > dwm.c | 1 -
>> > 2 files changed, 1 insertion(+), 2 deletions(-)
>> >diff --git a/drw.h b/drw.h
>> >index 4c67419..4bcd5ad 100644
>> >--- a/drw.h
>> >+++ b/drw.h
>> >@@ -12,7 +12,7 @@ typedef struct Fnt {
>> >  struct Fnt *next;
>> > } Fnt;
>> >
>> >-enum { ColFg, ColBg }; /* Clr scheme index */
>> >+enum { ColFg, ColBg, ColBorder }; /* Clr scheme index */
>> > typedef XftColor Clr;
>> >
>> > typedef struct {
>> >diff --git a/dwm.c b/dwm.c
>> >index ec6a27c..ab16c75 100644
>> >--- a/dwm.c
>> >+++ b/dwm.c
>> >@@ -56,7 +56,6 @@
>> > #define HEIGHT(X)   ((X)->h + 2 * (X)->bw)
>> > #define TAGMASK ((1 << LENGTH(tags)) - 1)
>> > #define TEXTW(X)(drw_fontset_getwidth(drw, (X)) +
>> >lrpad)
>> >-#define ColBorder   2
>> >
>> > /* enums */
>> > enum { CurNormal, CurResize, CurMove, CurLast }; /* cursor */
>> >--
>> >2.7.4
>>
>> Hey,
>>
>> Thanks for your patch.
>>
>> The patch didn't apply for me and your e-mail is changed in the commit.
>>
>> I've fixed this and pushed the commit to master.
>>
>> --
>> Kind regards,
>> Hiltjo
>>
>

Re: [hackers] issues with 6.1 and single_taglist

[Silvan Jegen]

Hi

Since there is no patch attached this should probably go to the dev@
mailing list.

On Fri, Feb 9, 2018 at 12:22 AM, Allan Lindsay
 wrote:
> downloaded stock 6.1. same rejection problem - in each case, the last chunk.
> looked at the two diffs offered by the 'clean_patches' github, same problem.
> (well, one had the identical problem, the other broke on almost everything.)

Definitely sounds like the patch is just too old and nobody has
bothered to update it.


> am fairly familiar with c, and messed around with my dwm.c for a couple of
> hours, but only once did i pass 'make', and then that was really, really
> broken.

The best thing to do would probably be to post links to the patches,
the changes that you made and the errors that you got (failed chunks,
compilation errors, etc.) . That would remove the first barriers for
people that are on the fence about investing time looking into this
issue.

There won't be a magic fix. Somebody will have to sit down and adjust
the patch to make it work (if that is still possible without
completely rewriting everything). If you don't do it, maybe nobody
else will...


Cheers,

Silvan

Re: [hackers] [lsw][patch] Better handling of window title with non-ascii chars

[Silvan Jegen]

On Fri, Nov 24, 2017 at 12:35 PM, Hiltjo Posthuma
 wrote:
> On Wed, Nov 22, 2017 at 10:47:39PM +0100, Julien Steinhauser wrote:
>> Hello
>>
>> I've seen window title with non-ascii chars incorrectly displayed,
>> using Xutf8TextPropertyToTextList instead of XmbTextPropertyToTextList
>> as in the diff below fixed it.
>>
>> Regards
>>
>> Julien
>>
>> diff --git a/lsw.c b/lsw.c
>> index fc40fef..9a5bee1 100644
>> --- a/lsw.c
>> +++ b/lsw.c
>> @@ -55,7 +55,7 @@ getname(Window win) {
>>   if(!XGetTextProperty(dpy, win, , netwmname) || prop.nitems == 0)
>>   if(!XGetWMName(dpy, win, ) || prop.nitems == 0)
>>   return "";
>> - if(!XmbTextPropertyToTextList(dpy, , , ) && n > 0) {
>> + if(!Xutf8TextPropertyToTextList(dpy, , , ) && n > 0) {
>>   strncpy(buf, list[0], sizeof buf);
>>   XFreeStringList(list);
>>   } else
>>
>
> Hey,
>
> Which OS, libc and locale do you use?
> uname -a, locale, libc + version?
>
> On both OpenBSD and Linux glibc with UTF-8 locale it should not be needed.

I suspect that this is not standard-conform since the locale is set to
the "C" one on startup of a program according to

https://linux.die.net/man/3/setlocale

until setlocale is called.

The "C" locale does not support multibyte characters by default it seems:

"A program that hasn't called setlocale() can't expect to be able to
use the multibyte
interfaces reasonably anyway, so it doesn't matter that they default
to byte mode when the program starts up." from [0]


Cheers,

Silvan

[0] http://www.openwall.com/lists/musl/2014/06/27/4

Re: [hackers] [lsw][patch] Better handling of window title with non-ascii chars

[Silvan Jegen]

Hi

On Wed, Nov 22, 2017 at 10:47 PM, Julien Steinhauser  wrote:
> I've seen window title with non-ascii chars incorrectly displayed,
> using Xutf8TextPropertyToTextList instead of XmbTextPropertyToTextList
> as in the diff below fixed it.

We do not seem to be using locale.h and its setlocale function here so
using the UTF8 variant of that call is what we have to use to make it
work if people are using UTF8-encoded properties. As long as we are
happy with just supporting UTF8 this is the right function to call so
this patch looks good to me.


Cheers,

Silvan

Re: [hackers] [sbase][PATCH] patch: improvments suggested by Silvan

[Silvan Jegen]

Hi Mattias

On Sun, Sep 24, 2017 at 8:20 PM, Mattias Andrée  wrote:
> On Sun, 24 Sep 2017 11:12:35 -0700
> Michael Forney  wrote:
>
>> Hi Mattias,
>>
>> Instead of sending these patches on top of your original patch, can
>> you send amended versions (v2, v3, etc)? You can use `git format-patch
>> -v 2` to make this clear in the subject.
>>
>> I think that this would make it easier to review and keep track of your 
>> patch.
>>
>> Thanks!
>>
>
> Hi Michael!
>
> I thought it would be easier to see the changes I make.
> I think an amended version makes more sense when it's
> time to merge.

I would prefer an amended version of the patch as well. You could
mention the changes between the versions under the '---' separator
like it's done here (that way the change comments don't get picked up
by 'git am'): https://lkml.org/lkml/2017/9/25/19

Re: [hackers] [PATCH][sbase] Add patch(1)

[Silvan Jegen]

On Sun, Sep 24, 2017 at 8:57 PM, Mattias Andrée <maand...@kth.se> wrote:
> On Sun, 24 Sep 2017 19:24:10 +0200
> Silvan Jegen <s.je...@gmail.com> wrote:
>
>> Heyho
>>
>> On Sun, Sep 24, 2017 at 06:28:57PM +0200, Mattias Andrée wrote:
>> > On Sun, 24 Sep 2017 14:08:41 +0200
>> > Silvan Jegen <s.je...@gmail.com> wrote:
>> >
>> > > > +
>> > > > +   if (!new->len)
>> > > > +   for (i = 0; i < old->len; i++)
>> > > > +   if (old->lines[i].data[-2] != '-')
>> > >
>> > > I think according to the standard, refering to data[-2] invokes undefined
>> > > behaviour, since at this time, data points to the beginning of the array.
>> >
>> > I'm not sure whether it is defined or undefined; I would think that it
>> > defined, but that adding integers larger than INTPTR_MAX is undefined.
>> > I will change to `*(data - 2)` as this is clearly defined.
>>
>> I was referring to
>> https://stackoverflow.com/questions/3473675/are-negative-array-indexes-allowed-in-c/3473686#3473686
>> . `*(data -2) is equivalent to 'data[-2]' but since 'data' doesn't point
>> to the second element of the array, I don't think this is valid.
>
> Hi!
>
> I think there has been some misunderstanding here,
> and that we are in agreement that `a[-b]` in it self
> is not invalid, but that question is whether the
> deferenced address is valid. I understand why this
> looks incorrect, `old->lines->data[0]` does not
> actually point to the first character on a line
> in a line but rather to the first character in the
> line that is part of the content of the file that
> hunk patches. For example if the patchfile contains
> the line "- abc", `old->lines->data[0]` is `a`, not
> `-`, because "- " part of the annotations in the
> hunk.

Ah, I missed that. In that case this negative index is ok.


> This should probably be clarified, but you can see
> that this happening just above this code.

Pointing this out in a comment seems like a good idea to me.


> I will look that your other comments later.

Sure!


Cheers,

Silvan

Re: [hackers] [PATCH][sbase] Add patch(1)

[Silvan Jegen]

Heyho

On Sun, Sep 24, 2017 at 06:28:57PM +0200, Mattias Andrée wrote:
> On Sun, 24 Sep 2017 14:08:41 +0200
> Silvan Jegen <s.je...@gmail.com> wrote:
> 
> > Heyho Mattias!
> > 
> > I had a look at the patch. It's a lot of code (still only about 1/3 of
> > GNU's patch size though) and it was rather hard for me to follow so more
> > review should be done. Find my comments below.
> > 
> > On Sun, Sep 03, 2017 at 07:13:20PM +0200, Mattias Andrée wrote:
> > > +static void
> > > +save_file_cpp(FILE *f, struct file_data *file)
> > > +{
> > > + size_t i, j, n;
> > > + char annot = ' ';
> > > +
> > > + for (i = 0; i <= file->n; i++) {
> > > + if ((n = file->d[i].nold)) {  
> > 
> > In other places you iterate with "i < file->n" (see save_file below for
> > example) so I think this may be an off-by-one error.
> 
> There is an if-statement, that breaks the loop if `i == file->`,
> after this clause, so this should be correct. I'll add blank lines

I saw the break statement but the 'n = file->d[i].nold' will be executed before
reaching that break statement resulting in a segfault when i == file->n.


> around that if-statement to make it clearer.
> 
> > 
> > 
> > > + fprintf(f, "%s\n", annot == '+' ? "#else" : ifndef);
> > > + for (j = 0; j < n; j++) {
> > > + fwriteline(f, file->d[i].old[j]);
> > > + if (missinglf(file->d[i].old[j]))
> > > + fprintf(f, "\n");
> > > + }
> > > + annot = '-';
> > > + }
> > > + if (i == file->n)
> > > + break;
> > > + if (annot == '-')
> > > + fprintf(f, "%s\n", file->d[i].new ? "#else" : "#endif");
> > > + else if (annot == ' ' && file->d[i].new)
> > > + fprintf(f, "%s\n", ifdef);
> > > + else if (annot == '+' && !file->d[i].new)
> > > + fprintf(f, "#endif\n");
> > > + fwriteline(f, file->d[i].line);
> > > + if ((i + 1 < file->n || file->d[i].new) && 
> > > missinglf(file->d[i].line))
> > > + fprintf(f, "\n");
> > > + annot = file->d[i].new ? '+' : ' ';
> > > + }
> > > + if (annot != ' ')
> > > + fprintf(f, "#endif\n");
> > > +}
> > > +
> > > +static void
> > > +parse_hunk_copied(struct hunk *hunk, struct parsed_hunk *parsed)
> > > +{
> > > + struct hunk_content *old = >old, *new = >new;
> > > + size_t i = 0, a, b;
> > > + char *p;
> > > +
> > > + free(hunk->head->data);
> > > +
> > > + old->lines = enmalloc(FAILURE, hunk->nlines * sizeof(*old->lines));
> > > + new->lines = enmalloc(FAILURE, hunk->nlines * sizeof(*new->lines));
> > > + parsed->annot = enmalloc(FAILURE, hunk->nlines + 1);
> > > +
> > > + p = hunk->lines[i++].data + 4;
> > > + old->start = strtoul(p, , 10);
> > > + old->len = 0;
> > > +
> > > + for (; hunk->lines[i].data[1] == ' '; i++)
> > > + subline(old->lines + old->len++, hunk->lines + i, 2);
> > > +
> > > + p = hunk->lines[i++].data + 4;
> > > + new->start = strtoul(p, , 10);
> > > + new->len = 0;
> > > +
> > > + if (old->len) {
> > > + for (; i < hunk->nlines; i++)
> > > + subline(new->lines + new->len++, hunk->lines + i, 2);
> > > + } else {
> > > + for (; i < hunk->nlines; i++) {
> > > + subline(new->lines + new->len++, hunk->lines + i, 2);
> > > + if (hunk->lines[i].data[0] != '+')
> > > + subline(old->lines + old->len++, hunk->lines + 
> > > i, 2);
> > > + }
> > > + }  
> > 
> > I think this if-else block can be rewritten like this.
> > 
> > for (; i < hunk->nlines; i++) {
> > subline(new->lines + new->len++, hunk->lines + i, 2);
> > if (old->len == 0 && hunk->lines[i].data[0] != '+')
> > subline(old->lines + old->len++, 

Re: [hackers] [PATCH][sbase] Add patch(1)

[Silvan Jegen]

Heyho Mattias!

I had a look at the patch. It's a lot of code (still only about 1/3 of
GNU's patch size though) and it was rather hard for me to follow so more
review should be done. Find my comments below.

On Sun, Sep 03, 2017 at 07:13:20PM +0200, Mattias Andrée wrote:
> Signed-off-by: Mattias Andrée 
> ---
>  Makefile   |2 +
>  README |1 +
>  TODO   |1 -
>  libutil/asprintf.c |   74 +++
>  libutil/getlines.c |   17 +-
>  patch.1|  250 +++
>  patch.c| 1835 
> 
>  text.h |4 +-
>  util.h |5 +
>  9 files changed, 2182 insertions(+), 7 deletions(-)
>  create mode 100644 libutil/asprintf.c
>  create mode 100644 patch.1
>  create mode 100644 patch.c
> 
> diff --git a/Makefile b/Makefile
> index 1c39fef..014db74 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -45,6 +45,7 @@ LIBUTFSRC =\
>  
>  LIBUTIL = libutil.a
>  LIBUTILSRC =\
> + libutil/asprintf.c\
>   libutil/concat.c\
>   libutil/cp.c\
>   libutil/crypt.c\
> @@ -132,6 +133,7 @@ BIN =\
>   nohup\
>   od\
>   paste\
> + patch\
>   pathchk\
>   printenv\
>   printf\
> diff --git a/README b/README
> index da2e500..6c94f2f 100644
> --- a/README
> +++ b/README
> @@ -59,6 +59,7 @@ The following tools are implemented:
>  0#*|o nl  .
>  0=*|o nohup   .
>  0=*|o od  .
> +0=patch   .
>  0#* o pathchk .
>   #*|o paste   .
>  0=*|x printenv.
> diff --git a/TODO b/TODO
> index 5edb8a3..fe2344e 100644
> --- a/TODO
> +++ b/TODO
> @@ -8,7 +8,6 @@ awk
>  bc
>  diff
>  ed manpage
> -patch
>  stty
>  
>  If you are looking for some work to do on sbase, another option is to
> diff --git a/libutil/asprintf.c b/libutil/asprintf.c
> new file mode 100644
> index 000..929ed09
> --- /dev/null
> +++ b/libutil/asprintf.c
> @@ -0,0 +1,74 @@
> +/* See LICENSE file for copyright and license details. */
> +#include 
> +#include 
> +#include 
> +
> +#include "../util.h"
> +
> +static int xenvasprintf(int, char **, const char *, va_list);
> +
> +int
> +asprintf(char **strp, const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + va_start(ap, fmt);
> + ret = xenvasprintf(-1, strp, fmt, ap);
> + va_end(ap);
> +
> + return ret;
> +}
> +
> +int
> +easprintf(char **strp, const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + va_start(ap, fmt);
> + ret = xenvasprintf(1, strp, fmt, ap);
> + va_end(ap);
> +
> + return ret;
> +}
> +
> +int
> +enasprintf(int status, char **strp, const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + va_start(ap, fmt);
> + ret = xenvasprintf(status, strp, fmt, ap);
> + va_end(ap);
> +
> + return ret;
> +}
> +
> +int
> +xenvasprintf(int status, char **strp, const char *fmt, va_list ap)
> +{
> + int ret;
> + va_list ap2;
> +
> + va_copy(ap2, ap);
> + ret = vsnprintf(0, 0, fmt, ap2);
> + va_end(ap2);
> + if (ret < 0) {
> + if (status >= 0)
> + enprintf(status, "vsnprintf:");
> + *strp = 0;
> + return -1;
> + }
> +
> + *strp = malloc(ret + 1);
> + if (!*strp) {
> + if (status >= 0)
> + enprintf(status, "malloc:");
> + return -1;
> + }
> +
> + vsprintf(*strp, fmt, ap);
> + return ret;
> +}
> diff --git a/libutil/getlines.c b/libutil/getlines.c
> index b912769..9af7684 100644
> --- a/libutil/getlines.c
> +++ b/libutil/getlines.c
> @@ -7,7 +7,7 @@
>  #include "../util.h"
>  
>  void
> -getlines(FILE *fp, struct linebuf *b)
> +ngetlines(int status, FILE *fp, struct linebuf *b)
>  {
>   char *line = NULL;
>   size_t size = 0, linelen = 0;
> @@ -16,17 +16,24 @@ getlines(FILE *fp, struct linebuf *b)
>   while ((len = getline(, , fp)) > 0) {
>   if (++b->nlines > b->capacity) {
>   b->capacity += 512;
> - b->lines = erealloc(b->lines, b->capacity * 
> sizeof(*b->lines));
> + b->lines = enrealloc(status, b->lines, b->capacity * 
> sizeof(*b->lines));
>   }
>   linelen = len;
> - b->lines[b->nlines - 1].data = memcpy(emalloc(linelen + 1), 
> line, linelen + 1);
> + b->lines[b->nlines - 1].data = memcpy(enmalloc(status, linelen 
> + 1), line, linelen + 1);
>   b->lines[b->nlines - 1].len = linelen;
>   }
>   free(line);
> - if (b->lines && b->nlines && linelen && b->lines[b->nlines - 
> 1].data[linelen - 1] != '\n') {
> - b->lines[b->nlines - 1].data = erealloc(b->lines[b->nlines - 
> 1].data, linelen + 2);
> + b->nolf = b->lines && b->nlines && linelen && b->lines[b->nlines - 
> 1].data[linelen - 1] != '\n';
> + if (b->nolf) {
> + b->lines[b->nlines - 1].data = 

Re: [hackers] [PATCH][sbase] Add patch(1)

[Silvan Jegen]

On Mon, Sep 11, 2017 at 08:57:02PM +0200, Mattias Andrée wrote:
> On Mon, 11 Sep 2017 20:09:33 +0200
> Silvan Jegen <s.je...@gmail.com> wrote:
> 
> >> +when comparing directories. If however, the  
> > 
> > There should probably be an additional comma like this:
> > 
> > "If, however, the file..."
> 
> I think “However, if the file...” is better.

Sounds good to me!


> > > +portion of a patch. A patch is a signal
> > > +file-comparison output from  
> > 
> > Not sure what a "signal file-comparison output" is... is this official
> > POSIX/patch terminology?
> 
> s/signal/singel/, so a patch file includes
> a patchset with is a number of patches, one
> per file in the patch file.

Haha, I totally didn't think of that...

 
> >> +Symbolic links are treated as regular files,
> >> +provided that they lead to regular files.  
> > 
> > maybe s/lead/link/ ?
> 
> Not sure, perhaps “link” sounds more natural,
> but for me, ”link” means we are talking about
> the step and not the final step when following
> the link. However, I will add this change.

I think both are understandable but the term "link" for symbolic links
is definitely the more common one, I would say.


signature.asc
Description: PGP signature

Re: [hackers] [PATCH][sbase] Add patch(1)

[Silvan Jegen]

Hi Mattias!

Thanks for (the) patch! :P

Some comments below. For now I only managed to look at whitespace issues
in the patch and suggest some corrections for spelling/grammar issues
in the man page text. I hope to get around looking at the code in the
near future.

On Sun, Sep 03, 2017 at 07:13:20PM +0200, Mattias Andrée wrote:
> Signed-off-by: Mattias Andrée 
> ---
>  Makefile   |2 +
>  README |1 +
>  TODO   |1 -
>  libutil/asprintf.c |   74 +++
>  libutil/getlines.c |   17 +-
>  patch.1|  250 +++
>  patch.c| 1835 
> 
>  text.h |4 +-
>  util.h |5 +
>  9 files changed, 2182 insertions(+), 7 deletions(-)
>  create mode 100644 libutil/asprintf.c
>  create mode 100644 patch.1
>  create mode 100644 patch.c
> 
> diff --git a/Makefile b/Makefile
> index 1c39fef..014db74 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -45,6 +45,7 @@ LIBUTFSRC =\
>  
>  LIBUTIL = libutil.a
>  LIBUTILSRC =\
> + libutil/asprintf.c\
>   libutil/concat.c\
>   libutil/cp.c\
>   libutil/crypt.c\
> @@ -132,6 +133,7 @@ BIN =\
>   nohup\
>   od\
>   paste\
> + patch\
>   pathchk\
>   printenv\
>   printf\
> diff --git a/README b/README
> index da2e500..6c94f2f 100644
> --- a/README
> +++ b/README
> @@ -59,6 +59,7 @@ The following tools are implemented:
>  0#*|o nl  .
>  0=*|o nohup   .
>  0=*|o od  .
> +0=patch   .
>  0#* o pathchk .
>   #*|o paste   .
>  0=*|x printenv.
> diff --git a/TODO b/TODO
> index 5edb8a3..fe2344e 100644
> --- a/TODO
> +++ b/TODO
> @@ -8,7 +8,6 @@ awk
>  bc
>  diff
>  ed manpage
> -patch
>  stty
>  
>  If you are looking for some work to do on sbase, another option is to
> diff --git a/libutil/asprintf.c b/libutil/asprintf.c
> new file mode 100644
> index 000..929ed09
> --- /dev/null
> +++ b/libutil/asprintf.c
> @@ -0,0 +1,74 @@
> +/* See LICENSE file for copyright and license details. */
> +#include 
> +#include 
> +#include 
> +
> +#include "../util.h"
> +
> +static int xenvasprintf(int, char **, const char *, va_list);
> +
> +int
> +asprintf(char **strp, const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + va_start(ap, fmt);
> + ret = xenvasprintf(-1, strp, fmt, ap);
> + va_end(ap);
> +
> + return ret;
> +}
> +
> +int
> +easprintf(char **strp, const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + va_start(ap, fmt);
> + ret = xenvasprintf(1, strp, fmt, ap);
> + va_end(ap);
> +
> + return ret;
> +}
> +
> +int
> +enasprintf(int status, char **strp, const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + va_start(ap, fmt);
> + ret = xenvasprintf(status, strp, fmt, ap);
> + va_end(ap);
> +
> + return ret;
> +}
> +
> +int
> +xenvasprintf(int status, char **strp, const char *fmt, va_list ap)
> +{
> + int ret;
> + va_list ap2;
> +
> + va_copy(ap2, ap);
> + ret = vsnprintf(0, 0, fmt, ap2);
> + va_end(ap2);
> + if (ret < 0) {
> + if (status >= 0)
> + enprintf(status, "vsnprintf:");
> + *strp = 0;
> + return -1;
> + }
> +
> + *strp = malloc(ret + 1);
> + if (!*strp) {
> + if (status >= 0)
> + enprintf(status, "malloc:");
> + return -1;
> + }
> +
> + vsprintf(*strp, fmt, ap);
> + return ret;
> +}
> diff --git a/libutil/getlines.c b/libutil/getlines.c
> index b912769..9af7684 100644
> --- a/libutil/getlines.c
> +++ b/libutil/getlines.c
> @@ -7,7 +7,7 @@
>  #include "../util.h"
>  
>  void
> -getlines(FILE *fp, struct linebuf *b)
> +ngetlines(int status, FILE *fp, struct linebuf *b)
>  {
>   char *line = NULL;
>   size_t size = 0, linelen = 0;
> @@ -16,17 +16,24 @@ getlines(FILE *fp, struct linebuf *b)
>   while ((len = getline(, , fp)) > 0) {
>   if (++b->nlines > b->capacity) {
>   b->capacity += 512;
> - b->lines = erealloc(b->lines, b->capacity * 
> sizeof(*b->lines));
> + b->lines = enrealloc(status, b->lines, b->capacity * 
> sizeof(*b->lines));
>   }
>   linelen = len;
> - b->lines[b->nlines - 1].data = memcpy(emalloc(linelen + 1), 
> line, linelen + 1);
> + b->lines[b->nlines - 1].data = memcpy(enmalloc(status, linelen 
> + 1), line, linelen + 1);
>   b->lines[b->nlines - 1].len = linelen;
>   }
>   free(line);
> - if (b->lines && b->nlines && linelen && b->lines[b->nlines - 
> 1].data[linelen - 1] != '\n') {
> - b->lines[b->nlines - 1].data = erealloc(b->lines[b->nlines - 
> 1].data, linelen + 2);
> + b->nolf = b->lines && b->nlines && linelen && b->lines[b->nlines - 
> 1].data[linelen - 1] != '\n';
> + if (b->nolf) 

Re: [hackers] [PATCH][sbase] tar: use bigger buffer size to increase performance

[Silvan Jegen]

Hi

On Sat, Sep 09, 2017 at 10:29:21AM -0700, Michael Forney wrote:
> On Sat, Sep 9, 2017 at 2:08 AM, Silvan Jegen <s.je...@gmail.com> wrote:
> > From: Jim Beveridge <ji...@chromium.org>
> >
> > The original code is by Jim Beveridge working on Fuchsia. I merged it
> > with slight changes.
> >
> > Time to tar two 1GB files:
> >
> > Before patch:
> >
> > real0m6.428s
> > user0m0.245s
> > sys 0m4.881s
> >
> > real0m6.454s
> > user0m0.239s
> > sys 0m4.883s
> >
> > real0m6.515s
> > user0m0.259s
> > sys 0m4.839s
> >
> > After patch:
> >
> > real0m4.755s
> > user0m0.026s
> > sys 0m1.598s
> >
> > real0m4.788s
> > user0m0.063s
> > sys 0m1.578s
> >
> > real0m4.822s
> > user0m0.007s
> > sys 0m1.662s
> >
> > A similar speedup can be observed for untaring.
> >
> > In addition to the buffer size increase we change the code to only create
> > directories for non-compliant tar files and we check for st to be NULL
> > in the recursive copy function.
> 
> He also sent me a pull request on my github branch for oasis:
> https://github.com/michaelforney/sbase/pull/2

Ah, I did not see that one.
 

> I think we should work on fixing correctness of tar before trying to
> optimize it. Currently it does not handle short reads or writes at all
> (when working with pipes). I was thinking we should add a readall in
> libutil analogous to writeall and then make use of that.

That sounds good to me.
 

> Regarding COPY_CHUNK_SIZE, it is probably a good idea to put that in
> util.h (perhaps with a different name). concat has the same problem
> with a small BUFSIZ (musl's is only 1024).

What do you think of Hiltjo's idea of querying for the page size with

long sz = sysconf(_SC_PAGESIZE);

or similar? Such code could still be put inot util.h of course.
 

> > ---
> >  tar.c | 72 
> > +++
> >  1 file changed, 55 insertions(+), 17 deletions(-)
> >
> > diff --git a/tar.c b/tar.c
> > index 53a737c..8cd1abe 100644
> > --- a/tar.c
> > +++ b/tar.c
> > @@ -16,6 +16,8 @@
> >  #include "util.h"
> >
> >  #define BLKSIZ 512
> > +// COPY_CHUNK_SIZE must be a power of 2
> > +#define COPY_CHUNK_SIZE 8192
> >
> >  enum Type {
> > REG   = '0',
> > @@ -236,10 +238,13 @@ archive(const char *path)
> > ewrite(tarfd, b, BLKSIZ);
> >
> > if (fd != -1) {
> > -   while ((l = eread(fd, b, BLKSIZ)) > 0) {
> > -   if (l < BLKSIZ)
> > -   memset(b + l, 0, BLKSIZ - l);
> > -   ewrite(tarfd, b, BLKSIZ);
> > +   char chunk[COPY_CHUNK_SIZE];
> > +   while ((l = eread(fd, chunk, COPY_CHUNK_SIZE)) > 0) {
> > +   // Ceiling to BLKSIZ boundary
> > +   int ceilsize = (l + (BLKSIZ-1)) & ~(BLKSIZ-1);
> > +   if (l < ceilsize)
> > +   memset(chunk + l, 0, ceilsize - l);
> > +   ewrite(tarfd, chunk, ceilsize);
> > }
> > close(fd);
> > }
> > @@ -250,7 +255,7 @@ archive(const char *path)
> >  static int
> >  unarchive(char *fname, ssize_t l, char b[BLKSIZ])
> >  {
> > -   char lname[101], *tmp, *p;
> > +   char lname[101], *p;
> > long mode, major, minor, type, mtime, uid, gid;
> > struct header *h = (struct header *)b;
> > int fd = -1;
> > @@ -261,9 +266,13 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ])
> > if (remove(fname) < 0 && errno != ENOENT)
> > weprintf("remove %s:", fname);
> >
> > -   tmp = estrdup(fname);
> > -   mkdirp(dirname(tmp), 0777, 0777);
> > -   free(tmp);
> > +   // tar files normally create the directory chain. This is a fallback
> > +   // for noncompliant tar files.
> > +   if (h->type != DIRECTORY) {
> > +   char* tmp = estrdup(fname);
> > +   mkdirp(dirname(tmp), 0777, 0777);
> > +   free(tmp);
> > +   }
> 
> If you tar a file within another directory, you end up with just one
> entry. This check means that the parent directory won't be created
> when trying to extract this file. Other tar implementations are able
> to extra

Re: [hackers] [PATCH][sbase] tar: use bigger buffer size to increase performance

[Silvan Jegen]

Hi Hiltjo

Thanks for the review!

On Sat, Sep 09, 2017 at 01:06:21PM +0200, Hiltjo Posthuma wrote:
> On Sat, Sep 09, 2017 at 11:08:42AM +0200, Silvan Jegen wrote:
> > From: Jim Beveridge <ji...@chromium.org>
> > 
> > The original code is by Jim Beveridge working on Fuchsia. I merged it
> > with slight changes.
> > 
> 
> To be clear: is it under the sbase LICENSE?

Yes, from what I can tell the License for this code is the same as for
the rest of sbase.

> >  #define BLKSIZ 512
> > +// COPY_CHUNK_SIZE must be a power of 2
> > +#define COPY_CHUNK_SIZE 8192
> >  
> 
> Instead of COPY_CHUNK_SIZE is might be worthwhile to query the pagesize, but
> i've not tested it.

Yes, I will have a look.

 
> >  enum Type {
> > REG   = '0',
> > @@ -236,10 +238,13 @@ archive(const char *path)
> > ewrite(tarfd, b, BLKSIZ);
> >  
> > if (fd != -1) {
> > -   while ((l = eread(fd, b, BLKSIZ)) > 0) {
> > -   if (l < BLKSIZ)
> > -   memset(b + l, 0, BLKSIZ - l);
> > -   ewrite(tarfd, b, BLKSIZ);
> > +   char chunk[COPY_CHUNK_SIZE];
> > +   while ((l = eread(fd, chunk, COPY_CHUNK_SIZE)) > 0) {
> > +   // Ceiling to BLKSIZ boundary
> > +   int ceilsize = (l + (BLKSIZ-1)) & ~(BLKSIZ-1);
> > +   if (l < ceilsize)
> > +   memset(chunk + l, 0, ceilsize - l);
> > +   ewrite(tarfd, chunk, ceilsize);
> > }
> > close(fd);
> > }
> > @@ -250,7 +255,7 @@ archive(const char *path)
> >  static int
> >  unarchive(char *fname, ssize_t l, char b[BLKSIZ])
> >  {
> > -   char lname[101], *tmp, *p;
> > +   char lname[101], *p;
> > long mode, major, minor, type, mtime, uid, gid;
> > struct header *h = (struct header *)b;
> > int fd = -1;
> > @@ -261,9 +266,13 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ])
> > if (remove(fname) < 0 && errno != ENOENT)
> > weprintf("remove %s:", fname);
> >  
> > -   tmp = estrdup(fname);
> > -   mkdirp(dirname(tmp), 0777, 0777);
> > -   free(tmp);
> > +   // tar files normally create the directory chain. This is a fallback
> > +   // for noncompliant tar files.
> > +   if (h->type != DIRECTORY) {
> > +   char* tmp = estrdup(fname);
> > +   mkdirp(dirname(tmp), 0777, 0777);
> > +   free(tmp);
> > +   }
> >  
> > switch (h->type) {
> > case REG:
> > @@ -319,9 +328,25 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ])
> > eprintf("strtol %s: invalid number\n", h->gid);
> >  
> > if (fd != -1) {
> > -   for (; l > 0; l -= BLKSIZ)
> > -   if (eread(tarfd, b, BLKSIZ) > 0)
> > -   ewrite(fd, b, MIN(l, BLKSIZ));
> > +   // Ceiling to BLKSIZ boundary
> > +   int readsize = (l + (BLKSIZ-1)) & ~(BLKSIZ-1);
> > +   char chunk[COPY_CHUNK_SIZE];
> > +   int lastread = 0;
> > +
> > +   for (; readsize > 0; l -= lastread, readsize -= lastread) {
> > +   int chunk_size = MIN(readsize, COPY_CHUNK_SIZE);
> > +   // Short reads are legal, so don't expect to read
> > +   // everything that was requested.
> > +   lastread = eread(tarfd, chunk, chunk_size);
> > +   if (lastread == 0) {
> > +   close(fd);
> > +   remove(fname);
> 
> Do all the tar tools remove the file in this case? It might be better to not
> remove it.

You are right, this code should probably be removed if we use a
ReadAll-like function.


> > +   eprintf("unexpected end of file reading %s.\n",
> > +   fname);
> > +   }
> > +
> > +   ewrite(fd, chunk, MIN(l, lastread));
> > +   }
> > close(fd);
> > }
> >  
> > @@ -331,7 +356,7 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ])
> > times[0].tv_sec = times[1].tv_sec = mtime;
> > times[0].tv_nsec = times[1].tv_nsec = 0;
> > if (!mflag && utimensat(AT_FDCWD, fname, times, AT_SYMLINK_NOFOLLOW) < 
> > 0)
> > -   weprintf("utimensat %s:\n", fname);
> > +   weprintf("utimensat %s %d:\n", fna

[hackers] [PATCH][sbase] tar: use bigger buffer size to increase performance

[Silvan Jegen]

From: Jim Beveridge 

The original code is by Jim Beveridge working on Fuchsia. I merged it
with slight changes.

Time to tar two 1GB files:

Before patch:

real0m6.428s
user0m0.245s
sys 0m4.881s

real0m6.454s
user0m0.239s
sys 0m4.883s

real0m6.515s
user0m0.259s
sys 0m4.839s

After patch:

real0m4.755s
user0m0.026s
sys 0m1.598s

real0m4.788s
user0m0.063s
sys 0m1.578s

real0m4.822s
user0m0.007s
sys 0m1.662s

A similar speedup can be observed for untaring.

In addition to the buffer size increase we change the code to only create
directories for non-compliant tar files and we check for st to be NULL
in the recursive copy function.
---
 tar.c | 72 +++
 1 file changed, 55 insertions(+), 17 deletions(-)

diff --git a/tar.c b/tar.c
index 53a737c..8cd1abe 100644
--- a/tar.c
+++ b/tar.c
@@ -16,6 +16,8 @@
 #include "util.h"
 
 #define BLKSIZ 512
+// COPY_CHUNK_SIZE must be a power of 2
+#define COPY_CHUNK_SIZE 8192
 
 enum Type {
REG   = '0',
@@ -236,10 +238,13 @@ archive(const char *path)
ewrite(tarfd, b, BLKSIZ);
 
if (fd != -1) {
-   while ((l = eread(fd, b, BLKSIZ)) > 0) {
-   if (l < BLKSIZ)
-   memset(b + l, 0, BLKSIZ - l);
-   ewrite(tarfd, b, BLKSIZ);
+   char chunk[COPY_CHUNK_SIZE];
+   while ((l = eread(fd, chunk, COPY_CHUNK_SIZE)) > 0) {
+   // Ceiling to BLKSIZ boundary
+   int ceilsize = (l + (BLKSIZ-1)) & ~(BLKSIZ-1);
+   if (l < ceilsize)
+   memset(chunk + l, 0, ceilsize - l);
+   ewrite(tarfd, chunk, ceilsize);
}
close(fd);
}
@@ -250,7 +255,7 @@ archive(const char *path)
 static int
 unarchive(char *fname, ssize_t l, char b[BLKSIZ])
 {
-   char lname[101], *tmp, *p;
+   char lname[101], *p;
long mode, major, minor, type, mtime, uid, gid;
struct header *h = (struct header *)b;
int fd = -1;
@@ -261,9 +266,13 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ])
if (remove(fname) < 0 && errno != ENOENT)
weprintf("remove %s:", fname);
 
-   tmp = estrdup(fname);
-   mkdirp(dirname(tmp), 0777, 0777);
-   free(tmp);
+   // tar files normally create the directory chain. This is a fallback
+   // for noncompliant tar files.
+   if (h->type != DIRECTORY) {
+   char* tmp = estrdup(fname);
+   mkdirp(dirname(tmp), 0777, 0777);
+   free(tmp);
+   }
 
switch (h->type) {
case REG:
@@ -319,9 +328,25 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ])
eprintf("strtol %s: invalid number\n", h->gid);
 
if (fd != -1) {
-   for (; l > 0; l -= BLKSIZ)
-   if (eread(tarfd, b, BLKSIZ) > 0)
-   ewrite(fd, b, MIN(l, BLKSIZ));
+   // Ceiling to BLKSIZ boundary
+   int readsize = (l + (BLKSIZ-1)) & ~(BLKSIZ-1);
+   char chunk[COPY_CHUNK_SIZE];
+   int lastread = 0;
+
+   for (; readsize > 0; l -= lastread, readsize -= lastread) {
+   int chunk_size = MIN(readsize, COPY_CHUNK_SIZE);
+   // Short reads are legal, so don't expect to read
+   // everything that was requested.
+   lastread = eread(tarfd, chunk, chunk_size);
+   if (lastread == 0) {
+   close(fd);
+   remove(fname);
+   eprintf("unexpected end of file reading %s.\n",
+   fname);
+   }
+
+   ewrite(fd, chunk, MIN(l, lastread));
+   }
close(fd);
}
 
@@ -331,7 +356,7 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ])
times[0].tv_sec = times[1].tv_sec = mtime;
times[0].tv_nsec = times[1].tv_nsec = 0;
if (!mflag && utimensat(AT_FDCWD, fname, times, AT_SYMLINK_NOFOLLOW) < 
0)
-   weprintf("utimensat %s:\n", fname);
+   weprintf("utimensat %s %d:\n", fname, errno);
if (h->type == SYMLINK) {
if (!getuid() && lchown(fname, uid, gid))
weprintf("lchown %s:\n", fname);
@@ -349,10 +374,23 @@ static void
 skipblk(ssize_t l)
 {
char b[BLKSIZ];
-
-   for (; l > 0; l -= BLKSIZ)
-   if (!eread(tarfd, b, BLKSIZ))
-   break;
+   int lastread = 0;
+   // Ceiling to BLKSIZ boundary
+   int ceilsize = (l + (BLKSIZ-1)) & ~(BLKSIZ-1);
+
+   off_t offset = lseek(tarfd, ceilsize, SEEK_CUR);
+   if (offset >= ceilsize)
+   

Re: [hackers] [sbase] libutil/cp.c leaks

[Silvan Jegen]

Heyho

On Sun, May 07, 2017 at 01:54:42PM +0200, Hiltjo Posthuma wrote:
> I think there are some cases where libutil/cp.c can leak file descriptors.
> It is a warning case, but the descriptors are not closed.
> 
> I think only `mv` is affected.
> 
> Patch below:
> 
> 
> From af392d1a764d7420c7b05bb9e13d7766a5979894 Mon Sep 17 00:00:00 2001
> From: Hiltjo Posthuma 
> Date: Sun, 7 May 2017 13:50:26 +0200
> Subject: [PATCH] libutil: fix leaks
> 
> ---
>  libutil/cp.c | 4 
>  1 file changed, 4 insertions(+)

Looks good to me!


Cheers,

Silvan
 

> diff --git a/libutil/cp.c b/libutil/cp.c
> index 9bb517a..a8db0a2 100644
> --- a/libutil/cp.c
> +++ b/libutil/cp.c
> @@ -79,6 +79,7 @@ cp(const char *s1, const char *s2, int depth)
>   if (mkdir(s2, st.st_mode) < 0 && errno != EEXIST) {
>   weprintf("mkdir %s:", s2);
>   cp_status = 1;
> + closedir(dp);
>   return 0;
>   }
>  
> @@ -122,15 +123,18 @@ cp(const char *s1, const char *s2, int depth)
>   if (unlink(s2) < 0 && errno != ENOENT) {
>   weprintf("unlink %s:", s2);
>   cp_status = 1;
> + fclose(f1);
>   return 0;
>   } else if (!(f2 = fopen(s2, "w"))) {
>   weprintf("fopen %s:", s2);
>   cp_status = 1;
> + fclose(f1);
>   return 0;
>   }
>   } else {
>   weprintf("fopen %s:", s2);
>   cp_status = 1;
> + fclose(f1);
>   return 0;
>   }
>   }
> -- 
> 2.12.2
> 
> 
> 
> -- 
> Kind regards,
> Hiltjo
> 

Re: [hackers] [dwm] [PATCH 2/3] Button passthrough when client is not focused

[Silvan Jegen]

Hi Markus

>From what I understand, in case that the client does not have focus
already, with this patch we call XGrabButton twice compared to before the
patch. I assume that corresponds to the advertised functionality.

One more comment below.

On Sat, Jan 07, 2017 at 05:21:29PM +0100, Markus Teich wrote:
> Before this change it is not possible to press a button in a client on the 
> first
> click if the client is not yet focused. The first click on the button would
> only focus the client and a second click on the button is needed to activate 
> it.
> This situation can occur when moving the mouse over a client (therefore 
> focusing
> it) and then moving the focus to another client with keyboard shortcuts.
> 
> After this commit the behavior is fixed and button presses on unfocused 
> clients
> are passed to the client correctly.
> ---
> 
> 
> Heyho,
> 
> this patch is the same as submitted previously. I just added it to this patch
> series for a better overview of the pending patches.
> 
> --Markus
> 
> 
>  dwm.c | 21 +++--
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/dwm.c b/dwm.c
> index 3f80b63..9c01d1a 100644
> --- a/dwm.c
> +++ b/dwm.c
> @@ -446,6 +446,8 @@ buttonpress(XEvent *e)
>   click = ClkWinTitle;
>   } else if ((c = wintoclient(ev->window))) {
>   focus(c);
> + restack(selmon);
> + XAllowEvents(dpy, ReplayPointer, CurrentTime);
>   click = ClkClientWin;
>   }
>   for (i = 0; i < LENGTH(buttons); i++)
> @@ -932,17 +934,16 @@ grabbuttons(Client *c, int focused)
>   unsigned int i, j;
>   unsigned int modifiers[] = { 0, LockMask, numlockmask, 
> numlockmask|LockMask };
>   XUngrabButton(dpy, AnyButton, AnyModifier, c->win);
> - if (focused) {
> - for (i = 0; i < LENGTH(buttons); i++)
> - if (buttons[i].click == ClkClientWin)
> - for (j = 0; j < LENGTH(modifiers); j++)
> - XGrabButton(dpy, 
> buttons[i].button,
> - buttons[i].mask | 
> modifiers[j],
> - c->win, False, 
> BUTTONMASK,
> - GrabModeAsync, 
> GrabModeSync, None, None);
> - } else
> + if (!focused)
>   XGrabButton(dpy, AnyButton, AnyModifier, c->win, False,
> - BUTTONMASK, GrabModeAsync, GrabModeSync, 
> None, None);
> + BUTTONMASK, GrabModeSync, GrabModeSync, 
> None, None);

Here we pass "GrabModeSync" instead of "GrabModeAsync" as the "pointer
mode". According to https://tronche.com/gui/x/xlib/input/XGrabPointer.html
if the mode is "GrabModeSync",

"the state of the pointer, as seen by client applications, appears to
freeze, and the X server generates no further pointer events until
the grabbing client calls XAllowEvents() or until the pointer grab
is released."

Since we are calling XAllowEvents on buttonpress that should make sure
that only the focused client gets any pointer events, I assume?

I have been runnig with this patch for most of the day and haven't
encountered any issues.


Cheers,

Silvan


> + for (i = 0; i < LENGTH(buttons); i++)
> + if (buttons[i].click == ClkClientWin)
> + for (j = 0; j < LENGTH(modifiers); j++)
> + XGrabButton(dpy, buttons[i].button,
> + buttons[i].mask | 
> modifiers[j],
> + c->win, False, BUTTONMASK,
> + GrabModeAsync, 
> GrabModeSync, None, None);
>   }
>  }
>  
> -- 
> 2.10.2
> 
> 

Re: [hackers] [PATCH] simplify client moving on monitor count decrease

[Silvan Jegen]

Hi Markus

On Wed, Jan 04, 2017 at 06:05:33PM +0100, Markus Teich wrote:
> ---
>  dwm.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Looks good to me.

I have tested this patch yesterday and today in daily use and haven't
found any issues on my 2-monitor setup (yet).


Cheers,

Silvan

 
> diff --git a/dwm.c b/dwm.c
> index 4eba952..88d49d7 100644
> --- a/dwm.c
> +++ b/dwm.c
> @@ -1896,9 +1896,8 @@ updategeom(void)
>   /* less monitors available nn < n */
>   for (i = nn; i < n; i++) {
>   for (m = mons; m && m->next; m = m->next);
> - while (m->clients) {
> + while ((c = m->clients)) {
>   dirty = 1;
> - c = m->clients;
>   m->clients = c->next;
>   detachstack(c);
>   c->mon = mons;
> -- 
> 2.10.2
> 
> 

Re: [hackers] [sbase] [PATCH 1/5] Remove st != NULL checks from recursor functions

[Silvan Jegen]

Hi Laslo

On Tue, Dec 27, 2016 at 02:59:56PM +0100, Laslo Hunhold wrote:
> On Wed, 14 Dec 2016 19:40:02 -0800
> Michael Forney  wrote:
> 
> Hey Michael,
> 
> > In the description of 3111908b034c73673a2f079b2b13a88c18379baa, it
> > says that the functions must be able to handle st being NULL, but
> > recurse always passes a valid pointer. The only function that was
> > ever passed NULL was rm(), but this was changed to go through recurse
> > in 2f4ab527391135e651b256f8654b050ea4a48f3d, so now the checks are
> > pointless.
> 
> have you tested this patchset extensively? I hate to admit that the
> recursor-subsystem is probably the most fragile part of sbase and
> really need more feedback on these patches by more people (Silvan, have
> you had the chance to test this?).

Sadly, I have not :/

I just did some very basic testing using the following directory
structure.

testdir/
testdir/testdir2
testdir/testdir2/testB.txt
testdir/testdir2/testA.txt
testdir/link (symbolic link)
testdir/test1.txt
testdir/test2.txt

I used the recursive options of tar, chgrp, chmod, chown, du and rm after
applying Michael's patch on these files/directories and everything worked
as expected.

Ideally, somebody that uses sbase in a development system regularly
should apply and test the patches further.


Cheers,

Silvan
 
> Cheers
> 
> Laslo
> 
> -- 
> Laslo Hunhold 
> 

Re: [hackers] [sbase] [PATCH 5/5] mkdir: Fix created directory permissions

[Silvan Jegen]

Hi

On Thu, Dec 15, 2016 at 4:40 AM, Michael Forney  wrote:
> Previously, with -p, the specified directory and all of its parents
> would be 0777&~filemask (regardless of the -m flag). POSIX says parent
> directories must created as (0300|~filemask)&0777, and of course if -m
> is set, the specified directory should be created with those
> permissions.
>
> Additionally, POSIX says that for symbolic_mode strings, + and - should
> be interpretted relative to a default mode of 0777 (not 0).
>
> Without -p, previously the directory would be created first with
> 0777&~filemask (before a chmod), but POSIX says that the directory shall
> at no point in time have permissions less restrictive than the -m mode
> argument.
>
> Rather than dealing with mkdir removing the filemask bits by calling
> chmod afterward, just clear the umask and remove the bits manually.
> ---
>  libutil/mkdirp.c |  6 +++---
>  mkdir.c  | 19 ---
>  tar.c|  2 +-
>  util.h   |  2 +-
>  4 files changed, 13 insertions(+), 16 deletions(-)

I have stared at the patch for a while and it removes more lines than
it adds so I think this one is okay.

Note that I did not have time to test it though.


Cheers,

Silvan


> diff --git a/libutil/mkdirp.c b/libutil/mkdirp.c
> index 2ef94a3..c3c678c 100644
> --- a/libutil/mkdirp.c
> +++ b/libutil/mkdirp.c
> @@ -7,7 +7,7 @@
>  #include "../util.h"
>
>  int
> -mkdirp(const char *path)
> +mkdirp(const char *path, mode_t mode, mode_t pmode)
>  {
> char tmp[PATH_MAX], *p;
> struct stat st;
> @@ -25,13 +25,13 @@ mkdirp(const char *path)
> if (*p != '/')
> continue;
> *p = '\0';
> -   if (mkdir(tmp, S_IRWXU | S_IRWXG | S_IRWXO) < 0 && errno != 
> EEXIST) {
> +   if (mkdir(tmp, pmode) < 0 && errno != EEXIST) {
> weprintf("mkdir %s:", tmp);
> return -1;
> }
> *p = '/';
> }
> -   if (mkdir(tmp, S_IRWXU | S_IRWXG | S_IRWXO) < 0 && errno != EEXIST) {
> +   if (mkdir(tmp, mode) < 0 && errno != EEXIST) {
> weprintf("mkdir %s:", tmp);
> return -1;
> }
> diff --git a/mkdir.c b/mkdir.c
> index 3e32d90..3e20b1a 100644
> --- a/mkdir.c
> +++ b/mkdir.c
> @@ -15,17 +15,18 @@ usage(void)
>  int
>  main(int argc, char *argv[])
>  {
> -   mode_t mode = 0, mask;
> -   int pflag = 0, mflag = 0, ret = 0;
> +   mode_t mode, mask;
> +   int pflag = 0, ret = 0;
> +
> +   mask = umask(0);
> +   mode = 0777 & ~mask;
>
> ARGBEGIN {
> case 'p':
> pflag = 1;
> break;
> case 'm':
> -   mflag = 1;
> -   mask = getumask();
> -   mode = parsemode(EARGF(usage()), mode, mask);
> +   mode = parsemode(EARGF(usage()), 0777, mask);
> break;
> default:
> usage();
> @@ -36,16 +37,12 @@ main(int argc, char *argv[])
>
> for (; *argv; argc--, argv++) {
> if (pflag) {
> -   if (mkdirp(*argv) < 0)
> +   if (mkdirp(*argv, mode, 0777 & (~mask | 0300)) < 0)
> ret = 1;
> -   } else if (mkdir(*argv, S_IRWXU | S_IRWXG | S_IRWXO) < 0) {
> +   } else if (mkdir(*argv, mode) < 0) {
> weprintf("mkdir %s:", *argv);
> ret = 1;
> }
> -   if (mflag && chmod(*argv, mode) < 0) {
> -   weprintf("chmod %s:", *argv);
> -   ret = 1;
> -   }
> }
>
> return ret;
> diff --git a/tar.c b/tar.c
> index d2161d4..f213039 100644
> --- a/tar.c
> +++ b/tar.c
> @@ -262,7 +262,7 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ])
> weprintf("remove %s:", fname);
>
> tmp = estrdup(fname);
> -   mkdirp(dirname(tmp));
> +   mkdirp(dirname(tmp), 0777, 0777);
> free(tmp);
>
> switch (h->type) {
> diff --git a/util.h b/util.h
> index b5860dc..29325d2 100644
> --- a/util.h
> +++ b/util.h
> @@ -75,6 +75,6 @@ long long strtonum(const char *, long long, long long, 
> const char **);
>  long long enstrtonum(int, const char *, long long, long long);
>  long long estrtonum(const char *, long long, long long);
>  size_t unescape(char *);
> -int mkdirp(const char *);
> +int mkdirp(const char *, mode_t, mode_t);
>  #undef memmem
>  void *memmem(const void *, size_t, const void *, size_t);
> --
> 2.11.0
>
>

Re: [hackers] [sbase] [PATCH 04/10] Don't use buffered IO (fread) when not appropriate

[Silvan Jegen]

Hi Laslo

On Tue, Dec 6, 2016 at 12:51 PM, Laslo Hunhold <d...@frign.de> wrote:
> On Tue, 6 Dec 2016 10:26:22 +0100
> Silvan Jegen <s.je...@gmail.com> wrote:
>> It only compiled for me because "util.h" includes stdio.h so the
>> definitions are included already. We can still remove this include if
>> we don't want it to be included twice but I don't know which approach
>> is preferred here.
>
> we definitely need to include it here as your implementation does not
> represent everyone's implementation. If Posix demands to include
> something for a given function, we should include it.

What I meant is that cksum.c includes  transitively (because
cksum.c includes "util.h" which includes ). As far as I know,
the setup of my development environment should not matter in that case
and the code should compile on any C compiler (with a preprocessor)
without having to include  in cksum.c again.

Including  directly in cksum.c makes the dependency more
explicit though and even if this results in the code of the header
being included again (as I think it will), I doubt it will have a big
impact on compilation speed.


Cheers,

Silvan

Re: [hackers] [sbase] [PATCH 04/10] Don't use buffered IO (fread) when not appropriate

[Silvan Jegen]

On Tue, Dec 6, 2016 at 9:17 AM, Michael Forney <mfor...@mforney.org> wrote:
> On Mon, Dec 5, 2016 at 12:15 PM, Silvan Jegen <s.je...@gmail.com> wrote:
>> Hi
>>
>> Some comments below.
>>
>> On Sun, Dec 04, 2016 at 09:55:06PM -0800, Michael Forney wrote:
>>> diff --git a/cksum.c b/cksum.c
>>> index 570ca81..b53ec17 100644
>>> --- a/cksum.c
>>> +++ b/cksum.c
>>> @@ -1,7 +1,9 @@
>>>  /* See LICENSE file for copyright and license details. */
>>> +#include 
>>>  #include 
>>>  #include 
>>
>> This include is not needed anymore.
>
> It is needed for printf, putchar, fputs, and stdout.

You're right.

It only compiled for me because "util.h" includes stdio.h so the
definitions are included already. We can still remove this include if
we don't want it to be included twice but I don't know which approach
is preferred here.


>>> diff --git a/od.c b/od.c
>>> index b5884e7..9ae1e29 100644
>>> --- a/od.c
>>> +++ b/od.c
>>> @@ -1,8 +1,10 @@
>>>  /* See LICENSE file for copyright and license details. */
>>> +#include 
>>>  #include 
>>>  #include 
>>
>> This include is not needed anymore.
>
> It is needed for printf, fputc, and stdout.

Same here.


>>>   for (i = 0; i < argc; i++) {
>>> - if (!(fps[i] = fopen(argv[i], aflag ? "a" : "w"))) {
>>> - weprintf("fopen %s:", argv[i]);
>>> + if ((fds[i] = open(argv[i], O_WRONLY|O_CREAT|aflag, 0666)) < 
>>> 0) {
>>
>> umask will be honored when creating a file but I am wondering if just
>> setting mode_t to 0660 would be the safer option here.
>
> POSIX says that it has to be 0666[0]:
>
> """
> When a file that does not exist is created, the following features
> defined in the System Interfaces volume of POSIX.1-2008 shall apply
> unless the utility or function description states otherwise:
>
> ...
>
> 3. If the file is a regular file, the permission bits of the file
> shall be set to: S_IROTH | S_IWOTH | S_IRGRP | S_IWGRP | S_IRUSR |
> S_IWUSR
>
> (see the description of File Modes in XBD Headers, )
> except that the bits specified by the file mode creation mask of the
> process shall be cleared.
> """

Ah, I did not know that POSIX had anything to say on this. Then we
keep it the way it is.


Cheers,

Silvan

> [0] 
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap01.html#tag_17_01_01_04
>

Re: [hackers] [sbase] [PATCH 02/10] od: Fix buffer overflow if -N flag is larger than BUFSIZ

[Silvan Jegen]

On Tue, Dec 6, 2016 at 9:08 AM, Michael Forney <mfor...@mforney.org> wrote:
> On Mon, Dec 5, 2016 at 4:47 AM, Silvan Jegen <s.je...@gmail.com> wrote:
>> From what I understand, max is an off_t which is signed and set to -1
>> (if not changed by a command line flag). If we cast this to the
>> unsigned size_t we get a very big number in the case where 'max' is
>> not set by a flag and the buffer size is used instead. Looks correct
>> to me.
>
> I will switch it back to check if max >= 0, because I think there
> could be a problem if off_t was larger than size_t.

As far as I can tell, both of these *may* be defined as 64-bit
integers. I would assume that there exists no programming environment
where only one of them is defined as a 64-bit integer and the other
one isn't so the bit-size should always be the same.

Checking for "max >= 0" may be clearer in any case though.

Re: [hackers] [sbase] [PATCH 07/10] concat: Use plain read/write instead of buffered stdio

[Silvan Jegen]

On Sun, Dec 04, 2016 at 09:55:09PM -0800, Michael Forney wrote:
> If we are just copying data from one file to another, we don't need to
> fill a complete buffer, just read a chunk at a time, and write it to the
> output.
> ---
>  cat.c| 34 ++
>  libutil/concat.c | 24 ++--
>  libutil/cp.c | 42 --
>  sponge.c | 31 +--
>  text.h   |  1 -
>  util.h   |  1 +
>  xinstall.c   | 25 -
>  7 files changed, 70 insertions(+), 88 deletions(-)
> 
> diff --git a/cat.c b/cat.c
> index e3741aa..55585dd 100644
> --- a/cat.c
> +++ b/cat.c
> @@ -1,22 +1,11 @@
>  /* See LICENSE file for copyright and license details. */
> -#include 
> +#include 
>  #include 
>  #include 
>  
> -#include "text.h"
>  #include "util.h"
>  
>  static void
> -uconcat(FILE *fp1, const char *s1, FILE *fp2, const char *s2)
> -{
> - int c;
> -
> - setbuf(fp2, NULL);
> - while ((c = getc(fp1)) != EOF)
> - putc(c, fp2);
> -}
> -
> -static void
>  usage(void)
>  {
>   eprintf("usage: %s [-u] [file ...]\n", argv0);
> @@ -25,37 +14,34 @@ usage(void)
>  int
>  main(int argc, char *argv[])
>  {
> - FILE *fp;
> - int ret = 0;
> - void (*cat)(FILE *, const char *, FILE *, const char *) = 
> + int fd, ret = 0;
>  
>   ARGBEGIN {
>   case 'u':
> - cat = 
>   break;
>   default:
>   usage();
>   } ARGEND
>  
>   if (!argc) {
> - cat(stdin, "", stdout, "");
> + if (concat(0, "", 1, "") < 0)
> + ret = 1;
>   } else {
>   for (; *argv; argc--, argv++) {
>   if (!strcmp(*argv, "-")) {
>   *argv = "";
> - fp = stdin;
> - } else if (!(fp = fopen(*argv, "r"))) {
> - weprintf("fopen %s:", *argv);
> + fd = 0;
> + } else if ((fd = open(*argv, O_RDONLY)) < 0) {
> + weprintf("open %s:", *argv);
>   ret = 1;
>   continue;
>   }
> - cat(fp, *argv, stdout, "");
> - if (fp != stdin && fshut(fp, *argv))
> + if (concat(fd, *argv, 1, "") < 0)
>   ret = 1;
> + if (fd != 0)
> + close(fd);
>   }
>   }
>  
> - ret |= fshut(stdin, "") | fshut(stdout, "");
> -
>   return ret;
>  }
> diff --git a/libutil/concat.c b/libutil/concat.c
> index fad9471..3f617e2 100644
> --- a/libutil/concat.c
> +++ b/libutil/concat.c
> @@ -1,19 +1,23 @@
>  /* See LICENSE file for copyright and license details. */
> -#include 
> +#include 
>  
> -#include "../text.h"
>  #include "../util.h"
>  
> -void
> -concat(FILE *fp1, const char *s1, FILE *fp2, const char *s2)
> +int
> +concat(int f1, const char *s1, int f2, const char *s2)
>  {
>   char buf[BUFSIZ];
> - size_t n;
> + ssize_t n;
>  
> - while ((n = fread(buf, 1, sizeof(buf), fp1))) {
> - fwrite(buf, 1, n, fp2);
> -
> - if (feof(fp1) || ferror(fp1) || ferror(fp2))
> - break;
> + while ((n = read(f1, buf, sizeof(buf))) > 0) {
> + if (writeall(f2, buf, n) < 0) {
> + weprintf("write %s:", s2);
> + return -1;
> + }
> + }
> + if (n < 0) {
> + weprintf("read %s:", s1);
> + return -1;
>   }
> + return 0;
>  }
> diff --git a/libutil/cp.c b/libutil/cp.c
> index c398962..8cd0a7d 100644
> --- a/libutil/cp.c
> +++ b/libutil/cp.c

The stdio.h #include can now also be removed from libutil/cp.c.


> @@ -12,7 +12,6 @@
>  #include 
>  
>  #include "../fs.h"
> -#include "../text.h"
>  #include "../util.h"
>  
>  int cp_aflag  = 0;
> @@ -27,7 +26,7 @@ int
>  cp(const char *s1, const char *s2, int depth)
>  {
>   DIR *dp;
> - FILE *f1, *f2;
> + int f1, f2;
>   struct dirent *d;
>   struct stat st;
>   struct timespec times[2];
> @@ -112,43 +111,34 @@ cp(const char *s1, const char *s2, int depth)
>   return 0;
>   }
>   } else {
> - if (!(f1 = fopen(s1, "r"))) {
> - weprintf("fopen %s:", s1);
> + if ((f1 = open(s1, O_RDONLY)) < 0) {
> + weprintf("open %s:", s1);
>   cp_status = 1;
>   return 0;
>   }
> - if (!(f2 = fopen(s2, "w"))) {
> - if (cp_fflag) {
> - if (unlink(s2) < 0 && errno != ENOENT) {
> - weprintf("unlink %s:", s2);
> - cp_status 

Re: [hackers] [sbase] [PATCH 04/10] Don't use buffered IO (fread) when not appropriate

[Silvan Jegen]

Hi

Some comments below.

On Sun, Dec 04, 2016 at 09:55:06PM -0800, Michael Forney wrote:
> fread reads the entire requested size (BUFSIZ), which causes tools to
> block if only small amounts of data are available at a time. At best,
> this causes unnecessary copies and inefficiency, at worst, tools like
> tee and cat are almost unusable in some cases since they only display
> large chunks of data at a time.
> ---
>  cksum.c | 31 +--
>  crypt.h |  2 +-
>  libutil/crypt.c | 37 +++--
>  od.c| 42 ++
>  tee.c   | 39 +++
>  5 files changed, 82 insertions(+), 69 deletions(-)
> 
> diff --git a/cksum.c b/cksum.c
> index 570ca81..b53ec17 100644
> --- a/cksum.c
> +++ b/cksum.c
> @@ -1,7 +1,9 @@
>  /* See LICENSE file for copyright and license details. */
> +#include 
>  #include 
>  #include 

This include is not needed anymore.


>  #include 
> +#include 
>  
>  #include "util.h"
>  
> @@ -61,19 +63,20 @@ static const unsigned long crctab[] = { 
> 0x,
>  };
>  
>  static void
> -cksum(FILE *fp, const char *s)
> +cksum(int fd, const char *s)
>  {
> - size_t len = 0, i, n;
> + ssize_t n;
> + size_t len = 0, i;
>   uint32_t ck = 0;
>   unsigned char buf[BUFSIZ];
>  
> - while ((n = fread(buf, 1, sizeof(buf), fp))) {
> + while ((n = read(fd, buf, sizeof(buf))) > 0) {
>   for (i = 0; i < n; i++)
>   ck = (ck << 8) ^ crctab[(ck >> 24) ^ buf[i]];
>   len += n;
>   }
> - if (ferror(fp)) {
> - weprintf("fread %s:", s ? s : "");
> + if (n < 0) {
> + weprintf("read %s:", s ? s : "");
>   ret = 1;
>   return;
>   }
> @@ -92,29 +95,29 @@ cksum(FILE *fp, const char *s)
>  int
>  main(int argc, char *argv[])
>  {
> - FILE *fp;
> + int fd;
>  
>   argv0 = argv[0], argc--, argv++;
>  
>   if (!argc) {
> - cksum(stdin, NULL);
> + cksum(0, NULL);
>   } else {
>   for (; *argv; argc--, argv++) {
>   if (!strcmp(*argv, "-")) {
>   *argv = "";
> - fp = stdin;
> - } else if (!(fp = fopen(*argv, "r"))) {
> - weprintf("fopen %s:", *argv);
> + fd = 0;
> + } else if ((fd = open(*argv, O_RDONLY)) < 0) {
> + weprintf("open %s:", *argv);
>   ret = 1;
>   continue;
>   }
> - cksum(fp, *argv);
> - if (fp != stdin && fshut(fp, *argv))
> - ret = 1;
> + cksum(fd, *argv);
> + if (fd != 0)
> + close(fd);
>   }
>   }
>  
> - ret |= fshut(stdin, "") | fshut(stdout, "");
> + ret |= fshut(stdout, "");
>  
>   return ret;
>  }
> diff --git a/crypt.h b/crypt.h
> index e0cc08d..2fd2932 100644
> --- a/crypt.h
> +++ b/crypt.h
> @@ -8,5 +8,5 @@ struct crypt_ops {
>  
>  int cryptcheck(int, char **, struct crypt_ops *, uint8_t *, size_t);
>  int cryptmain(int, char **, struct crypt_ops *, uint8_t *, size_t);
> -int cryptsum(struct crypt_ops *, FILE *, const char *, uint8_t *);
> +int cryptsum(struct crypt_ops *, int, const char *, uint8_t *);
>  void mdprint(const uint8_t *, const char *, size_t);
> diff --git a/libutil/crypt.c b/libutil/crypt.c
> index 6991c39..e285614 100644
> --- a/libutil/crypt.c
> +++ b/libutil/crypt.c
> @@ -1,8 +1,10 @@
>  /* See LICENSE file for copyright and license details. */
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "../crypt.h"
>  #include "../text.h"
> @@ -41,7 +43,7 @@ static void
>  mdchecklist(FILE *listfp, struct crypt_ops *ops, uint8_t *md, size_t sz,
>  int *formatsucks, int *noread, int *nonmatch)
>  {
> - FILE *fp;
> + int fd;
>   size_t bufsiz = 0;
>   int r;
>   char *line = NULL, *file, *p;
> @@ -59,12 +61,12 @@ mdchecklist(FILE *listfp, struct crypt_ops *ops, uint8_t 
> *md, size_t sz,
>   file += 2;
>   for (p = file; *p && *p != '\n' && *p != '\r'; p++); /* strip 
> newline */
>   *p = '\0';
> - if (!(fp = fopen(file, "r"))) {
> - weprintf("fopen %s:", file);
> + if ((fd = open(file, O_RDONLY)) < 0) {
> + weprintf("open %s:", file);
>   (*noread)++;
>   continue;
>   }
> - if (cryptsum(ops, fp, file, md)) {
> + if (cryptsum(ops, fd, file, md)) {
>   (*noread)++;
>   continue;
>   }
> @@ -77,7 +79,7 @@ 

Re: [hackers] [sbase] [PATCH 10/10] cp: Check result of utimensat

[Silvan Jegen]

On Mon, Dec 5, 2016 at 6:55 AM, Michael Forney  wrote:
> POSIX says that if duplicating the modification/access times fails, then
> an error should be written to stderr.
> ---
>  libutil/cp.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

LGTM

> diff --git a/libutil/cp.c b/libutil/cp.c
> index 339c892..15e4ce5 100644
> --- a/libutil/cp.c
> +++ b/libutil/cp.c
> @@ -142,8 +142,10 @@ cp(const char *s1, const char *s2, int depth)
> if (!S_ISLNK(st.st_mode)) {
> times[0] = st.st_atim;
> times[1] = st.st_mtim;
> -   utimensat(AT_FDCWD, s2, times, 0);
> -
> +   if (utimensat(AT_FDCWD, s2, times, 0) < 0) {
> +   weprintf("utimensat %s:", s2);
> +   cp_status = 1;
> +   }
> if (chown(s2, st.st_uid, st.st_gid) < 0) {
> weprintf("chown %s:", s2);
> cp_status = 1;
> --
> 2.11.0
>
>

Re: [hackers] [sbase] [PATCH 06/10] xinstall: Check result of fchmod

[Silvan Jegen]

On Mon, Dec 5, 2016 at 6:55 AM, Michael Forney  wrote:
> ---
>  xinstall.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

LGTM

> diff --git a/xinstall.c b/xinstall.c
> index bf921fb..5a0e390 100644
> --- a/xinstall.c
> +++ b/xinstall.c
> @@ -119,7 +119,8 @@ install(const char *s1, const char *s2, int depth)
> }
> concat(f1, s1, f2, s2);
>
> -   fchmod(fileno(f2), mode);
> +   if (fchmod(fileno(f2), mode) < 0)
> +   eprintf("fchmod %s:", s2);
>
> if (fclose(f2) == EOF)
> eprintf("fclose %s:", s2);
> --
> 2.11.0
>
>

Re: [hackers] [sbase] [PATCH 03/10] libutil: Add writeall utility function

[Silvan Jegen]

On Mon, Dec 5, 2016 at 6:55 AM, Michael Forney  wrote:
> writeall makes successive write calls to write an entire buffer to the
> output file descriptor. It returns the number of bytes written, or -1 on
> the first error.
> ---
>  Makefile   |  3 ++-
>  libutil/writeall.c | 21 +
>  util.h |  3 +++
>  3 files changed, 26 insertions(+), 1 deletion(-)
>  create mode 100644 libutil/writeall.c


LGTM


Cheers,

Silvan

> diff --git a/Makefile b/Makefile
> index 25bab70..a337ead 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -79,7 +79,8 @@ LIBUTILSRC =\
> libutil/strlcpy.c\
> libutil/strsep.c\
> libutil/strtonum.c\
> -   libutil/unescape.c
> +   libutil/unescape.c\
> +   libutil/writeall.c
>
>  LIB = $(LIBUTF) $(LIBUTIL)
>
> diff --git a/libutil/writeall.c b/libutil/writeall.c
> new file mode 100644
> index 000..4725ced
> --- /dev/null
> +++ b/libutil/writeall.c
> @@ -0,0 +1,21 @@
> +/* See LICENSE file for copyright and license details. */
> +#include 
> +
> +#include "../util.h"
> +
> +ssize_t
> +writeall(int fd, const void *buf, size_t len)
> +{
> +   const char *p = buf;
> +   ssize_t n;
> +
> +   while (len) {
> +   n = write(fd, p, len);
> +   if (n <= 0)
> +   return n;
> +   p += n;
> +   len -= n;
> +   }
> +
> +   return p - (const char *)buf;
> +}
> diff --git a/util.h b/util.h
> index b5860dc..eaad3ce 100644
> --- a/util.h
> +++ b/util.h
> @@ -62,6 +62,9 @@ char *strsep(char **, const char *);
>  int enregcomp(int, regex_t *, const char *, int);
>  int eregcomp(regex_t *, const char *, int);
>
> +/* io */
> +ssize_t writeall(int, const void *, size_t);
> +
>  /* misc */
>  void enmasse(int, char **, int (*)(const char *, const char *, int));
>  void fnck(const char *, const char *, int (*)(const char *, const char *, 
> int), int);
> --
> 2.11.0
>
>

Re: [hackers] [dwm] applied Ivan Delalande's NET_SUPPORTING_WM_CHECK patch for gtk3 compatibility || Anselm R Garbe

[Silvan Jegen]

On Mon, Dec 5, 2016 at 1:45 PM, Markus Teich  wrote:
> Anselm R Garbe wrote:
>> I agree, but my MUA couldn't detect a proper attachment.
>
> Heyho Anselm,
>
> `git am` also works with the whole source-text of a mail. For example with 
> mutt
> you can create a local patches.mbox maildir on your filesystem and then copy 
> the
> mail source there with `C`. Then `git am PATH/TO/patches.mbox/cur/FILENAME` 
> does
> the trick.

It's also possible to just pipe the text of an email directly to 'git
am' by using mutt's '|' command (obviously you will have to run mutt
from your repo dir for this to work).


Cheers,

Silvan

Re: [hackers] [sbase] [PATCH 02/10] od: Fix buffer overflow if -N flag is larger than BUFSIZ

[Silvan Jegen]

Hi

On Mon, Dec 5, 2016 at 6:55 AM, Michael Forney  wrote:
> Previously, if max was specified, od will call read with that size,
> potentially overflowing buf with data read from the file.
> ---
>  od.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/od.c b/od.c
> index 9b83501..b5884e7 100644
> --- a/od.c
> +++ b/od.c
> @@ -132,20 +132,19 @@ od(FILE *fp, char *fname, int last)
> size_t i;
> unsigned char buf[BUFSIZ];
> static off_t addr;
> -   size_t buflen;
> +   size_t n;
>
> while (skip - addr > 0) {
> -   buflen = fread(buf, 1, MIN(skip - addr, BUFSIZ), fp);
> -   addr += buflen;
> +   n = fread(buf, 1, MIN(skip - addr, sizeof(buf)), fp);
> +   addr += n;
> if (feof(fp) || ferror(fp))
> return;
> }
> if (!line)
> line = emalloc(linelen);
>
> -   while ((buflen = fread(buf, 1, max >= 0 ?
> -  max - (addr - skip) : BUFSIZ, fp))) {
> -   for (i = 0; i < buflen; i++, addr++) {
> +   while ((n = fread(buf, 1, MIN((size_t)max - (addr - skip), 
> sizeof(buf)), fp))) {

>From what I understand, max is an off_t which is signed and set to -1
(if not changed by a command line flag). If we cast this to the
unsigned size_t we get a very big number in the case where 'max' is
not set by a flag and the buffer size is used instead. Looks correct
to me.

The brackets around 'addr - skip' are not needed but I assume they are
there for documentation purposes (and they were present before the
patch too) so

LGTM


Cheers,

Silvan


> +   for (i = 0; i < n; i++, addr++) {
> line[lineoff++] = buf[i];
> if (lineoff == linelen) {
> printline(line, lineoff, addr - lineoff + 1);
> --
> 2.11.0
>
>

Re: [hackers] [sbase] [PATCH 01/10] crypt: Add some missing error checks for cryptsum

[Silvan Jegen]

Hi

And thanks for the patches!

Comments below.

On Mon, Dec 5, 2016 at 6:55 AM, Michael Forney  wrote:
> Previously, if a file failed to read in a checksum list, it would be
> reported as not matched rather than a read failure.
>
> Also, if reading from stdin failed, previously a bogus checksum would be
> printed anyway.
> ---
>  libutil/crypt.c | 16 ++--
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/libutil/crypt.c b/libutil/crypt.c
> index 3f849ba..6991c39 100644
> --- a/libutil/crypt.c
> +++ b/libutil/crypt.c
> @@ -64,7 +64,10 @@ mdchecklist(FILE *listfp, struct crypt_ops *ops, uint8_t 
> *md, size_t sz,
> (*noread)++;
> continue;
> }
> -   cryptsum(ops, fp, file, md);
> +   if (cryptsum(ops, fp, file, md)) {
> +   (*noread)++;
> +   continue;
> +   }
> r = mdcheckline(line, md, sz);
> if (r == 1) {
> printf("%s: OK\n", file);
> @@ -125,8 +128,10 @@ cryptmain(int argc, char *argv[], struct crypt_ops *ops, 
> uint8_t *md, size_t sz)
> int ret = 0;
>
> if (argc == 0) {
> -   cryptsum(ops, stdin, "", md);
> -   mdprint(md, "", sz);
> +   if (cryptsum(ops, stdin, "", md))
> +   ret = 1;

The return values in crypt.c are not consistent which tripped me up here.

"cryptsum" returns 1 on error (as does "cryptmain") but "mdcheckline"
returns 0 (or -1) on error and 1 on success. For consistency we should
change "mdcheckline" to return 1/-1 on error and 0 on success.

If we make that change it should be in a different patch though.

> +   else
> +   mdprint(md, "", sz);
> } else {
> for (; *argv; argc--, argv++) {
> if ((*argv)[0] == '-' && !(*argv)[1]) {
> @@ -137,11 +142,10 @@ cryptmain(int argc, char *argv[], struct crypt_ops 
> *ops, uint8_t *md, size_t sz)
> ret = 1;
> continue;
> }
> -   if (cryptsum(ops, fp, *argv, md)) {
> +   if (cryptsum(ops, fp, *argv, md))
> ret = 1;
> -   } else {
> +   else
> mdprint(md, *argv, sz);
> -   }

This is a style change and some people may thus want to have it put in
a separate patch. I think combining such a minor change with this
patch is fine.

This patch LGTM!


Cheers,

Silvan


> if (fp != stdin && fshut(fp, *argv))
> ret = 1;
> }
> --
> 2.11.0
>
>

Re: [hackers] [sinit] [PATCH] Use switch for fork()

[Silvan Jegen]

On Fri, Sep 23, 2016 at 9:48 AM, FRIGN  wrote:
> To be precise, I think this can be considered the first patch of this
> year's slcon3 hacking sessions. ;)

Haha, so early...

I will be arriving around 21h tonight. I assume that there will be
more time for coding on Sunday as well!


Cheers,

Silvan

Re: [hackers][vis][RFC][PATCH 0/2] Suggestion for basic autocomplete functionality

[Silvan Jegen]

Hi Marc

On Wed, May 18, 2016 at 08:10:44PM +0200, Marc André Tanner wrote:
> After some refactoring the functionality of your patches should now be
> merged, thanks! It should now also work with slmenu, I guess you only

Nice!


> used it with dmenu?

I only tested it with dmenu because I assumed slmenu would scribble
all over the curses TUI. Turns out that just redrawing it afterwards
works fine.

However, in my short testing of both I found that dmenu's substring
matching works better for me. If you want to complete something like

vis_window_focus

starting from

vis

you can just continue with

vis wi fo

(the space char has to be typed too) in dmenu and you will be there. This
does not work with slmenu. I assume that's because the partial substring
matching is implemented differently in slmenu.


> The behavior could still be improved. It would be handy to pass
> the prefix as the initial prompt value to the menu tool which would
> start with the filter applied. Instead of removing the prefix
> from the returned text (as you did with sed(1)), we would replace
> the whole word/prefix with whatever is returned from the external
> process.
> 
> Conceptually the syntax would become:
> 
>  $ extract_words | grep '^prefix' | sort | uniq | slmenu 'prefix'
> 
> or even:
> 
>  $ extract_words | sort | uniq | slmenu 'prefix'

Not completely sure I am following. What would be the advantage compared
to what we have now?


> I'm considering importing slmenu into the vis repository as vis-menu
> and implementing the necessary changes. This would ease packaging by
> distributions and make vis behave always the same independent of
> whether it is run in a X11 environment.
> 
> It would however also make it more difficult for users who prefer
> dmenu, opinions?

Because of the different behaviour regarding partial substring matching
I prefer dmenu. It's easy to change though so I don't mind either way.

slmenu seems to be the default "menu program" used by vis now
and dmenu is only the fallback (unless you export VIS_MENU=dmenu
explicitly). Personally, I don't see the advantage of adding more code
to vis that has to be kept up-to-date if people who care can easily
set up the environment just like they prefer it.

I don't know what the packagers' opinion about that is though.


Cheers,

Silvan

[hackers][vis][RFC][PATCH 2/2 v2] Add autocompletion for file names

[Silvan Jegen]

We use the new infrastructure for autocompletion to implement
autocompletion of file names within the working directory of vis.
---
v2 changes:
- pass empty Filerange

 config.def.h |  1 +
 main.c   | 19 +++
 2 files changed, 20 insertions(+)

diff --git a/config.def.h b/config.def.h
index 9e52102..5219d00 100644
--- a/config.def.h
+++ b/config.def.h
@@ -320,6 +320,7 @@ static const KeyBinding bindings_insert[] = {
{ "",  ALIAS("")  },
{ "",  ALIAS("")},
{ "",  ACTION(FILE_TEXT_AUTOCOMPLETE)  },
+   { "", ACTION(FILE_NAME_AUTOCOMPLETE)  },
{ "",  ALIAS("")},
{ "",  ACTION(MODE_OPERATOR_PENDING)   },
{ "",  ACTION(INSERT_REGISTER) },
diff --git a/main.c b/main.c
index 34cd81d..1a8893c 100644
--- a/main.c
+++ b/main.c
@@ -131,6 +131,8 @@ static void insert_dialog_selection(Vis*, Filerange range, 
const char *cmdline,
 static char *get_output_of_external_command(Vis*, Filerange range, const char 
*argv[]);
 /* Autocomplete input text at cursor based on the words in the current file */
 static const char *autocomplete_file_text(Vis*, const char *keys, const Arg 
*arg);
+/* Autocomplete input text at cursor based on file names of the current 
directory */
+static const char *autocomplete_file_name(Vis*, const char *keys, const Arg 
*arg);
 
 enum {
VIS_ACTION_EDITOR_SUSPEND,
@@ -314,6 +316,7 @@ enum {
VIS_ACTION_OPEN_FILE_UNDER_CURSOR,
VIS_ACTION_OPEN_FILE_UNDER_CURSOR_NEW_WINDOW,
VIS_ACTION_FILE_TEXT_AUTOCOMPLETE,
+   VIS_ACTION_FILE_NAME_AUTOCOMPLETE,
VIS_ACTION_NOP,
 };
 
@@ -1223,6 +1226,11 @@ static const KeyAction vis_action[] = {
"Autocomplete text in file",
autocomplete_file_text,
},
+   [VIS_ACTION_FILE_NAME_AUTOCOMPLETE] = {
+   "autocomplete-file-name",
+   "Autocomplete file name",
+   autocomplete_file_name,
+   },
[VIS_ACTION_NOP] = {
"nop",
"Ignore key, do nothing",
@@ -2160,6 +2168,17 @@ static char *get_prefix_for_autocomplete(Vis *vis) {
return prefix;
 }
 
+static const char *autocomplete_file_name(Vis *vis, const char *keys, const 
Arg *arg) {
+   char *prefix = get_prefix_for_autocomplete(vis);
+   if (!prefix)
+   return keys;
+
+   insert_dialog_selection(vis, text_range_empty(), "ls | grep \"^%s\" | 
sort | vis-menu | tr -d '\n' | sed \"s/%s//\"", prefix, prefix);
+
+   free(prefix);
+   return keys;
+}
+
 static const char *autocomplete_file_text(Vis *vis, const char *keys, const 
Arg *arg) {
Text *txt = vis_text(vis);
size_t txtsize = text_size(txt);
-- 
2.8.2

[hackers][vis][RFC][PATCH 1/2 v2] Add autocompletion for current file contents

[Silvan Jegen]

We add some infrastructure in order to run a shell command and get its
output which we then can insert into vis.

This infrastructure we use to execute a shell command which sends all
unique words of the current file to dmenu. The word selected in dmenu
is then inserted into vis at all cursor positions.
---
v2 changes:
- use vis-menu
- pipe current file content instead of content of file saved to disk
- use primary cursor (obtained with view_cursors_primary_get)

 config.def.h |   1 +
 main.c   | 115 +++
 vis.c|   6 +++-
 vis.h|   1 +
 4 files changed, 122 insertions(+), 1 deletion(-)

diff --git a/config.def.h b/config.def.h
index 8699136..9e52102 100644
--- a/config.def.h
+++ b/config.def.h
@@ -319,6 +319,7 @@ static const KeyBinding bindings_insert[] = {
{ "",  ALIAS("<b) or new (arg->b) window */
 static const char *open_file_under_cursor(Vis*, const char *keys, const Arg 
*arg);
+/* Insert text chosen in external file dialog at cursor position(s) */
+static void insert_dialog_selection(Vis*, Filerange range, const char 
*cmdline, ...);
+/* Get output of external command */
+static char *get_output_of_external_command(Vis*, Filerange range, const char 
*argv[]);
+/* Autocomplete input text at cursor based on the words in the current file */
+static const char *autocomplete_file_text(Vis*, const char *keys, const Arg 
*arg);
 
 enum {
VIS_ACTION_EDITOR_SUSPEND,
@@ -307,6 +313,7 @@ enum {
VIS_ACTION_NUMBER_DECREMENT,
VIS_ACTION_OPEN_FILE_UNDER_CURSOR,
VIS_ACTION_OPEN_FILE_UNDER_CURSOR_NEW_WINDOW,
+   VIS_ACTION_FILE_TEXT_AUTOCOMPLETE,
VIS_ACTION_NOP,
 };
 
@@ -1211,6 +1218,11 @@ static const KeyAction vis_action[] = {
"Open file under the cursor in a new window",
open_file_under_cursor, { .b = true }
},
+   [VIS_ACTION_FILE_TEXT_AUTOCOMPLETE] = {
+   "autocomplete-file-text",
+   "Autocomplete text in file",
+   autocomplete_file_text,
+   },
[VIS_ACTION_NOP] = {
"nop",
"Ignore key, do nothing",
@@ -2093,6 +2105,109 @@ static const char *open_file_under_cursor(Vis *vis, 
const char *keys, const Arg
return keys;
 }
 
+ssize_t read_buffer(void *context, char *data, size_t len) {
+   buffer_append(context, data, len);
+   return len;
+}
+
+static char *get_output_of_external_command(Vis *vis, Filerange range, const 
char *argv[]) {
+   char *out = NULL;
+   Buffer bufout, buferr;
+   buffer_init();
+   buffer_init();
+
+   if (!text_range_valid())
+   range = text_range_empty();
+
+   int status = vis_pipe(vis, , argv, , read_buffer,
+   , read_buffer);
+
+   if (status != 0) {
+   vis_info_show(vis, "Command failed %s", 
buffer_content0());
+   } else {
+   out = malloc(bufout.len);
+   strncpy(out, buffer_content0(), buffer_length0());
+   out[buffer_length0()] = '\0';
+   }
+
+   buffer_release();
+   buffer_release();
+   return out;
+}
+
+// Caller has to free the allocated memory for the prefix
+static char *get_prefix_for_autocomplete(Vis *vis) {
+   View *view = vis_view(vis);
+   Cursor *c = view_cursors(view);
+   Text *txt = vis_text(vis);
+
+   Filerange r = text_object_word(txt, view_cursors_pos(c)-1);
+   if (!text_range_valid())
+   return NULL;
+
+   char *prefix = text_bytes_alloc0(txt, r.start, text_range_size());
+   char *check;
+   for (check = prefix; *check; check++) {
+   if (!isspace(*check))
+   break;
+   }
+   if (*check == '\0') {
+   vis_info_show(vis, "Autocompletion without prefix input is not 
valid.");
+   free(prefix);
+   return NULL;
+   }
+
+   return prefix;
+}
+
+static const char *autocomplete_file_text(Vis *vis, const char *keys, const 
Arg *arg) {
+   Text *txt = vis_text(vis);
+   size_t txtsize = text_size(txt);
+
+   char *prefix = 

[hackers][vis][RFC][PATCH 1/2] Add autocompletion for current file contents

[Silvan Jegen]

We add some infrastructure in order to run a shell command and get its
output which we then can insert into vis.

This infrastructure we use to execute a shell command which sends all
unique words of the current file to dmenu. The word selected in dmenu
is then inserted into vis at all cursor positions.
---
 config.def.h |   1 +
 main.c   | 114 +++
 vis.c|   6 +++-
 vis.h|   1 +
 4 files changed, 121 insertions(+), 1 deletion(-)

diff --git a/config.def.h b/config.def.h
index 8699136..9e52102 100644
--- a/config.def.h
+++ b/config.def.h
@@ -319,6 +319,7 @@ static const KeyBinding bindings_insert[] = {
{ "",  ALIAS("<b) or new (arg->b) window */
 static const char *open_file_under_cursor(Vis*, const char *keys, const Arg 
*arg);
+/* Insert text chosen in external file dialog at cursor position(s) */
+static void insert_dialog_selection(Vis*, const char *cmdline, ...);
+/* Get output of external command */
+static char *get_output_of_external_command(Vis*, const char *argv[]);
+/* Autocomplete input text at cursor based on the words in the current file */
+static const char *autocomplete_file_text(Vis*, const char *keys, const Arg 
*arg);
 
 enum {
VIS_ACTION_EDITOR_SUSPEND,
@@ -307,6 +313,7 @@ enum {
VIS_ACTION_NUMBER_DECREMENT,
VIS_ACTION_OPEN_FILE_UNDER_CURSOR,
VIS_ACTION_OPEN_FILE_UNDER_CURSOR_NEW_WINDOW,
+   VIS_ACTION_FILE_TEXT_AUTOCOMPLETE,
VIS_ACTION_NOP,
 };
 
@@ -1211,6 +1218,11 @@ static const KeyAction vis_action[] = {
"Open file under the cursor in a new window",
open_file_under_cursor, { .b = true }
},
+   [VIS_ACTION_FILE_TEXT_AUTOCOMPLETE] = {
+   "autocomplete-file-text",
+   "Autocomplete text in file",
+   autocomplete_file_text,
+   },
[VIS_ACTION_NOP] = {
"nop",
"Ignore key, do nothing",
@@ -2093,6 +2105,108 @@ static const char *open_file_under_cursor(Vis *vis, 
const char *keys, const Arg
return keys;
 }
 
+ssize_t read_buffer(void *context, char *data, size_t len) {
+   buffer_append(context, data, len);
+   return len;
+}
+
+static char *get_output_of_external_command(Vis *vis, const char *argv[]) {
+   char *out = NULL;
+   Buffer bufout, buferr;
+   buffer_init();
+   buffer_init();
+
+   Filerange empty = text_range_empty();
+   int status = vis_pipe(vis, , argv, , read_buffer,
+   , read_buffer);
+
+   if (status != 0) {
+   vis_info_show(vis, "Command failed %s", 
buffer_content0());
+   } else {
+   out = malloc(bufout.len);
+   strncpy(out, buffer_content0(), buffer_length0());
+   out[buffer_length0()] = '\0';
+   }
+
+   buffer_release();
+   buffer_release();
+   return out;
+}
+
+// Caller has to free the allocated memory for the prefix
+static char *get_prefix_for_autocomplete(Vis *vis) {
+   View *view = vis_view(vis);
+   Cursor *c = view_cursors(view);
+   Text *txt = vis_text(vis);
+
+   Filerange r = text_object_word(txt, view_cursors_pos(c)-1);
+   if (!text_range_valid())
+   return NULL;
+
+   char *prefix = text_bytes_alloc0(txt, r.start, text_range_size());
+   char *check;
+   for (check = prefix; *check; check++) {
+   if (!isspace(*check))
+   break;
+   }
+   if (*check == '\0') {
+   vis_info_show(vis, "Autocompletion without prefix input is not 
valid.");
+   free(prefix);
+   return NULL;
+   }
+
+   return prefix;
+}
+
+static const char *autocomplete_file_text(Vis *vis, const char *keys, const 
Arg *arg) {
+   Win *win = vis_window(vis);
+   const char *fn = vis_window_filename(win);
+
+   char *prefix = get_prefix_for_autocomplete(vis);
+   if (!prefix)
+   return keys;
+
+   // TODO: get menu/dialog program to use from config?
+   insert_dialog_selection(vis, "cat '%s' | tr \" ;:$<>#?{}()[],.'\" '\n' 
| grep \"^%s\" | sort | uniq | dmenu | tr -d 

[hackers][vis][RFC][PATCH 0/2] Suggestion for basic autocomplete functionality

[Silvan Jegen]

Heyhey

I have implemented basic autocomplete functionality by sending the text
at cursor position as a context to shell commands and dmenu. The user
then chooses the autocompletion text he wants to use in dmenu which is
then inserted at cursor position.

Ideally the same approach should be used for code completion by sending
some context to a code completion daemon (like gocode[0] for example). The
daemon's output can then be sent to dmenu and selected by the user.

The patches are still rough but I did not want to invest more time
without knowing if such an approach would be considered for inclusion
or not. Let me know what you think.


Cheers,

Silvan


[0] https://github.com/nsf/gocode

Silvan Jegen (2):
  Add autocompletion for current file contents
  Add autocompletion for file names

 config.def.h |   2 +
 main.c   | 134 +++
 vis.c|   6 ++-
 vis.h|   1 +
 4 files changed, 142 insertions(+), 1 deletion(-)

-- 
2.8.2

Re: [hackers] [scc] Implement proper #pragma support || sin

[Silvan Jegen]

On Thu, May 12, 2016 at 11:45:45AM -0700, Menche wrote:
> wat

Haha, I compiled and ran it and it's thing of beauty!

Re: [hackers][vis][PATCH] Handle quote matching in its own function

[Silvan Jegen]

On Mon, Feb 15, 2016 at 05:50:12PM +0100, Marc André Tanner wrote:
> On Mon, Feb 15, 2016 at 04:01:48PM +0100, Silvan Jegen wrote:
>
> [...]
>
> What would be more interesting is to add a way to define motions and
> text objects in Lua. Integration with the LPeg based lexers should
> then be relatively easy ...

That would mean that you would call out to lua each time you want to
use one of these motions/objects, if I understand correctly.

Writing a grammar for matching these things in LPeg should be possible.
Do you think it would be worth implementing these motions/text objects in
LPeg? The main advantage I can see with this approach is that you could
create robust grammars for these motions/objects using LPeg instead of
doing some ad-hoc parsing like I did in this patch.


Cheers,

Silvan

Re: [hackers][vis][PATCH] Implement a first version of the 'gf' family of commands

[Silvan Jegen]

Heyho!

On Sat, Feb 13, 2016 at 02:16:37PM +0100, Marc André Tanner wrote:
> On Mon, Feb 08, 2016 at 11:02:03AM +0100, Silvan Jegen wrote:
> > I assume you mean that you're not sure if this functionality should go
> > in at all?
> 
> Yes vis is becoming bloated ;-) Anyway I merged it, does it work for
> your use case?

Hehe, as long as vis does not include a terminal emulator like nvim does
we should be fine...

Yes, this works for me, thanks! Not letting you open non-existing files
but showing a warning instead is a simple, good and practical approach.


> Any other important pending issues? I would like to do another release
> fairly soon ...

I have not yet had the time to use the current version of vis for too
long so there are no important issues I can report yet.


Cheers,

Silvan


> Thanks.
> 
> -- 
>  Marc André Tanner >< http://www.brain-dump.org/ >< GPG key: 10C93617
> 

Re: [hackers][vis][PATCH] Implement a first version of the 'gf' family of commands

[Silvan Jegen]

Hi Marc

On Sun, Feb 7, 2016 at 4:37 PM, Marc André Tanner <m...@brain-dump.org> wrote:
> The attached patches on top of current git HEAD should provide the same
> functionality you provided.
>
> Not sure whether they should be merged. Could be useful when displaying
> grep search results in a window as in:
>
>  :| grep something *.c

I assume you mean that you're not sure if this functionality should go
in at all?

I find the feature really useful. If you edit config files it's nice
to jump directly to the configured file name (even if only to see
whether it exists at all). The functionality let's you jump easily to
files mentioned in the output of other programs. You mention grep but
there are others too ("make" output for example though vim has a
tighter integration there).

Some comments below.

> Date: Sun, 7 Feb 2016 16:06:31 +0100
> Subject: [PATCH 1/2] vis: export vis_window_closable
>
> ---
>  vis-cmds.c | 10 ++
>  vis.c  | 10 ++
>  vis.h  |  3 +++
>  3 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/vis-cmds.c b/vis-cmds.c
> index 3f612f4..b3dad5b 100644
> --- a/vis-cmds.c
> +++ b/vis-cmds.c
> @@ -429,19 +429,13 @@ static bool cmd_open(Vis *vis, Filerange *range, enum 
> CmdOpt opt, const char *ar
>   return openfiles(vis, [1]);
>  }
> [...]
>
> +Win *vis_window(Vis *vis) {
> + return vis->win;
> +}
> +
>  Text *vis_file_text(File *file) {
>   return file->text;
>  }
> diff --git a/vis.h b/vis.h
> index 3bcd9c7..eb84695 100644
> --- a/vis.h
> +++ b/vis.h
> @@ -67,6 +67,8 @@ void vis_suspend(Vis*);
>  bool vis_window_new(Vis*, const char *filename);
>  /* reload the file currently displayed in the window from disk */
>  bool vis_window_reload(Win*);
> +/* check whether closing the window would loose unsaved changes */
> +bool vis_window_closable(Win*);
>  /* close window, redraw user interface */
>  void vis_window_close(Win*);
>  /* split the given window. changes to the displayed text will be reflected
> @@ -405,6 +407,7 @@ bool vis_signal_handler(Vis*, int signum, const siginfo_t 
> *siginfo, const void *
>  /* TODO: expose proper API to iterate through files etc */
>  Text *vis_text(Vis*);
>  View *vis_view(Vis*);
> +Win *vis_window(Vis*);
>  Text *vis_file_text(File*);
>  const char *vis_file_name(File*);
>
> --
> 2.1.4

LGTM. That's about how my second version looks as well. It may be
better to put the "vis_window" in your second patch because it's not used
in this one and the commit does not mention it.


> From b02aff69f90ffb5fe214d10e11386cadab2df55c Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marc=20Andr=C3=A9=20Tanner?= <m...@brain-dump.org>
> Date: Sun, 7 Feb 2016 16:07:44 +0100
> Subject: [PATCH 2/2] vis: implement gf and gf to open filename under
>  cursor
>
> Based on a patch by Silvan Jegen.
> ---
>  config.def.h |  2 ++
>  main.c   | 40 
>  2 files changed, 42 insertions(+)
>
> diff --git a/config.def.h b/config.def.h
> index 47ed326..774871b 100644
> --- a/config.def.h
> +++ b/config.def.h
> @@ -233,6 +233,8 @@ static const KeyBinding bindings_normal[] = {
>   { "gP", ACTION(PUT_BEFORE_END)  },
>   { "~",  ALIAS("l")  },
>   { "",  ALIAS("$")  },
> + { "gf", ACTION(OPEN_FILE_UNDER_CURSOR)  },
> + { "gf",ACTION(OPEN_FILE_UNDER_CURSOR_NEW_WINDOW)   },
>   { 0 /* empty last element, array terminator */  },
>  };
>
> diff --git a/main.c b/main.c
> index c3ae7a3..21a1066 100644
> --- a/main.c
> +++ b/main.c
> @@ -111,6 +111,8 @@ static const char *unicode_info(Vis*, const char *keys, 
> const Arg *arg);
>  static const char *percent(Vis*, const char *keys, const Arg *arg);
>  /* either increment (arg->i > 0) or decrement (arg->i < 0) number under 
> cursor */
>  static const char *number_increment_decrement(Vis*, const char *keys, const 
> Arg *arg);
> +/* open a filename under cursor in same (!arg->b) or new (arg->b) window */
> +static const char *open_file_under_cursor(Vis*, const char *keys, const Arg 
> *arg);
>
>  enum {
>   VIS_ACTION_EDITOR_SUSPEND,
> @@ -272,6 +274,8 @@ enum {
>   VIS_ACTION_UNICODE_INFO,
>   VIS_ACTION_NUMBER_INCREMENT,
>   VIS_ACTION_NUMBER_DECREMENT,
> + VIS_ACTION_OPEN_FILE_UNDER_CURSOR,
> + VIS_ACTION_OPEN_FILE_UNDER_CURSOR_NEW_WINDOW,
>   VIS_ACTION_NOP,
>  };
>
> @@ -1071,6 +1075,16 @@ static KeyAction vis_action[] = {
>   "Decrement number under cursor&

[hackers][vis][PATCH] Implement a first version of the 'gf' family of commands

[Silvan Jegen]

There are still a lot of rough edges. We don't change the jumplist for
example, which means that we won't be able to jump back to the old
file. We also don't check the file paths we want to open. If a path
element of a file we open with one of the implemented commands does not
exist before using the command we won't be able to save it to that path.
---

I like these commands but I assume not everybody does. Before spending
more time on this implementation I thought I would gather some feedback.

 config.def.h   |  2 ++
 main.c | 12 
 text-motions.c | 13 +
 text-motions.h |  9 +
 vis.c  | 53 +
 vis.h  |  4 
 6 files changed, 93 insertions(+)

diff --git a/config.def.h b/config.def.h
index 47ed326..af4b2b2 100644
--- a/config.def.h
+++ b/config.def.h
@@ -227,6 +227,8 @@ static const KeyBinding bindings_normal[] = {
{ "m",  ACTION(MARK_SET)},
{ "",   ALIAS(":help")   },
{ "ga", ACTION(UNICODE_INFO)},
+   { "gf", ACTION(OPEN_FILE_UNDER_CURSOR)  },
+   { "gf",ACTION(OPEN_FILE_UNDER_CURSOR_NEW_WINDOW)   },
{ "p",  ACTION(PUT_AFTER)   },
{ "P",  ACTION(PUT_BEFORE)  },
{ "gp", ACTION(PUT_AFTER_END)   },
diff --git a/main.c b/main.c
index 918f4c8..2404522 100644
--- a/main.c
+++ b/main.c
@@ -213,6 +213,8 @@ enum {
VIS_ACTION_INSERT_LINE_START,
VIS_ACTION_OPEN_LINE_ABOVE,
VIS_ACTION_OPEN_LINE_BELOW,
+   VIS_ACTION_OPEN_FILE_UNDER_CURSOR,
+   VIS_ACTION_OPEN_FILE_UNDER_CURSOR_NEW_WINDOW,
VIS_ACTION_JOIN_LINE_BELOW,
VIS_ACTION_JOIN_LINES,
VIS_ACTION_PROMPT_SHOW,
@@ -757,6 +759,16 @@ static KeyAction vis_action[] = {
"Focus previous window",
call, { .f = vis_window_prev }
},
+   [VIS_ACTION_OPEN_FILE_UNDER_CURSOR] = {
+   "open-file-under-cursor",
+   "Open file under the cursor",
+   call, { .f = vis_open_file }
+   },
+   [VIS_ACTION_OPEN_FILE_UNDER_CURSOR_NEW_WINDOW] = {
+   "open-file-under-cursor-new_window",
+   "Open file under the cursor in a new window",
+   call, { .f = vis_open_file_new_window }
+   },
[VIS_ACTION_APPEND_CHAR_NEXT] = {
"append-char-next",
"Append text after the cursor",
diff --git a/text-motions.c b/text-motions.c
index f406da3..2752158 100644
--- a/text-motions.c
+++ b/text-motions.c
@@ -26,6 +26,11 @@ int is_word_boundry(int c) {
 ('A' <= c && c <= 'Z') || c == '_');
 }
 
+int is_filename_boundry(int c) {
+   return isspace(c) || (c == ';' ||
+c == '"' || c == '\'' );
+}
+
 size_t text_begin(Text *txt, size_t pos) {
return 0;
 }
@@ -357,6 +362,14 @@ size_t text_word_start_prev(Text *txt, size_t pos) {
return text_customword_start_prev(txt, pos, is_word_boundry);
 }
 
+size_t text_filename_start_prev(Text *txt, size_t pos) {
+   return text_customword_start_prev(txt, pos, is_filename_boundry);
+}
+
+size_t text_filename_end_next(Text *txt, size_t pos) {
+   return text_customword_end_next(txt, pos, is_filename_boundry);
+}
+
 static size_t text_paragraph_sentence_next(Text *txt, size_t pos, bool 
sentence) {
char c;
bool content = false, paragraph = false;
diff --git a/text-motions.h b/text-motions.h
index 32e4a1f..67a366b 100644
--- a/text-motions.h
+++ b/text-motions.h
@@ -66,6 +66,15 @@ size_t text_longword_end_next(Text*, size_t pos);
 size_t text_longword_end_prev(Text*, size_t pos);
 size_t text_longword_start_next(Text*, size_t pos);
 size_t text_longword_start_prev(Text*, size_t pos);
+
+/*
+ * Get the beginning and the end of a file name. Every non-punctuation
+ * character except white space is being considered a file name
+ * character. TODO?: handle escaped space characters in filenames.
+ */
+size_t text_filename_start_prev(Text*, size_t pos);
+size_t text_filename_end_next(Text*, size_t pos);
+
 /*
  * A word consists of a sequence of letters, digits and underscores, or a
  * sequence of other non-blank characters, separated with white space.
diff --git a/vis.c b/vis.c
index b65724c..795dd7d 100644
--- a/vis.c
+++ b/vis.c
@@ -335,6 +335,59 @@ err:
return NULL;
 }
 
+/* Gets file name under cursor. Freeing the allocated memory is the
+ * caller's responsibility.
+ */
+static char* vis_get_filename_under_cursor(Vis *vis) {
+   Win *win = vis->win;
+   Cursor *c = view_cursors(win->view);
+   size_t pos = view_cursors_pos(c);
+   Text *txt = vis->win->file->text;
+   // We have to put in some pos offsets here because the 

[hackers][vis][PATCH] Don't use an offset of 1 for the 'L' command by default

[Silvan Jegen]

---

This was the easiest way to fix this that I could think of (without
duplicating most of the 'return' line).

 vis-motions.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/vis-motions.c b/vis-motions.c
index 9bd95b9..f00d525 100644
--- a/vis-motions.c
+++ b/vis-motions.c
@@ -99,7 +99,10 @@ static size_t view_lines_middle(Vis *vis, View *view) {
 
 static size_t view_lines_bottom(Vis *vis, View *view) {
int h = view_height_get(vis->win->view);
-   return view_screenline_goto(vis->win->view, h - vis->action.count);
+   int offset = 0;
+   if (vis->action.count > 1)
+   offset = vis->action.count;
+   return view_screenline_goto(vis->win->view, h - offset);
 }
 
 static size_t window_changelist_next(Vis *vis, Win *win, size_t pos) {
-- 
2.5.0

[hackers][vis][PATCH] Don't use an offset of 1 for the 'L' command by default

[Silvan Jegen]

---

This was the easiest way to fix this that I could think of (without
duplicating most of the 'return' line).

 vis-motions.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/vis-motions.c b/vis-motions.c
index 9bd95b9..f00d525 100644
--- a/vis-motions.c
+++ b/vis-motions.c
@@ -99,7 +99,10 @@ static size_t view_lines_middle(Vis *vis, View *view) {
 
 static size_t view_lines_bottom(Vis *vis, View *view) {
int h = view_height_get(vis->win->view);
-   return view_screenline_goto(vis->win->view, h - vis->action.count);
+   int offset = 0;
+   if (vis->action.count > 1)
+   offset = vis->action.count;
+   return view_screenline_goto(vis->win->view, h - offset);
 }
 
 static size_t window_changelist_next(Vis *vis, Win *win, size_t pos) {
-- 
2.5.0

Re: [hackers] [dmenu][RFC][PATCH] History functionality

[Silvan Jegen]

Heyho

and sorry for not answering earlier. I thought you were not waiting for
any OK from me but would apply it as soon as possible.

On Thu, Dec 17, 2015 at 05:07:38AM -0800, Xarchus wrote:
> Attached is the version of the text that will go under the "scripts"
> section, in a separate page. Let me know if all is OK, any changes, etc.

LGTM!


> It will have a link pointing to it from the main "scripts/"
> list:
> 
> * 
> [dmenu_run_history](http://tools.suckless.org/dmenu/scripts/dmenu_run_with_command_history)
> 
> As a separate page, I think it will also show up in the left vertical bar,
> under "scripts/".
> 
> I will deal with moving or deleting the old history patch later, I'm
> thinking of starting a thread in the wiki list about the fate of the
> "legacy" section, although I doubt there will be much interest.

> dmenu_run with command history
> ==
> 
> A self-contained alternative to dmenu_run which also handles history.
> 
> History is saved in a file in `$XDG_CACHE_HOME`, with fallback to a dot
> file in `$HOME`. Change as necessary.
> 
> In addition to the above, dmenu_run_history will launch each entry
> immediately on `Ctrl-Return` (multiselect).
> 
> The script can be used with the 4.6 version of dmenu.
> 
> Download
> 
> 
> [dmenu_run_history](dmenu_run_history) (20151217)
> 
> Authors
> ===
> 
> * Silvan Jegen

Since I did not really contribute a lot of code a "Contributor" tag may
be more apt here. I don't mind either way.

> * Xarchus


Cheers and thanks for the work!

Silvan

[hackers][vis][PATCH] Initialize enum values to the public API ones

[Silvan Jegen]

---

The goal of the original patch was to get rid of an enum that was defined
twice with similar values. Since one of them seems to be the public API
we use this one to initialize the internal enum.

Adding the enum values from vis-core.h to the public API in vis.h
would unify the enums but expose the other enum values which may not
be desirable.

So for now I just initialize the internal enum using the values from
vis.h as suggested by Marc.
 
 vis-core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vis-core.h b/vis-core.h
index 1ecc1b6..22bec33 100644
--- a/vis-core.h
+++ b/vis-core.h
@@ -59,8 +59,8 @@ typedef struct { /* Motion implementation, takes a cursor 
postion and returns a
size_t (*view)(Vis*, View*);
size_t (*win)(Vis*, Win*, size_t pos);
enum {
-   LINEWISE  = 1 << 0,  /* should the covered range be extended to 
whole lines? */
-   CHARWISE  = 1 << 1,  /* scrolls window content until position 
is visible */
+   LINEWISE  = VIS_MOTIONTYPE_LINEWISE,  /* should the covered 
range be extended to whole lines? */
+   CHARWISE  = VIS_MOTIONTYPE_CHARWISE,  /* scrolls window content 
until position is visible */
INCLUSIVE = 1 << 2,  /* should new position be included in 
operator range? */
IDEMPOTENT = 1 << 3, /* does the returned postion remain the 
same if called multiple times? */
JUMP = 1 << 4,
-- 
2.6.4

Re: [hackers][sbase][PATCH] Activate the "else if" branch

[Silvan Jegen]

Heyho

On Wed, Dec 16, 2015 at 9:28 AM, Roberto E. Vargas Caballero
<k...@shike2.com> wrote:
> On Tue, Dec 15, 2015 at 07:54:28PM +0100, Silvan Jegen wrote:
>> We checked the same condition in the "if" branch so it was never true
>> in the "else if" one.  Removing this condition makes the "else if"
>> branch viable.
>
> I'm sorry, but you are wrong here. Setjmp saves the current state

No need to be sorry I am wrong all the time :P


> of the program, and it allows that a deeper call to longjmp restores
> the state. When setjmp is called directly it returns always 0,
> but when it returns dur to a call to longjmp it returns a value
> passed as parameter to longjmp (in our case 1). It is a kind of
> try {} catch{} ala C.
>
>>   dowrite("ed.hup", 1);
>> - } else if (home && !setjmp(savesp)) {
>> + } else if (home) {
>>   n = snprintf(fname,
>
> If you remove the setjmp in the else if branch, then any call
> to error (which calls to longjmp) will resume the execution in
> the if branch, making a new execution of the else if branch,
> which in some cases will produce an infinite loop.

That means that the "else if" clause should only run when setjmp
returns directly and not due to a call to longjmp.

I wasn't considering the side effects of calling setjmp so ignore this
patch. Thanks for the feedback!


Cheers,

Silvan

[hackers][vis][PATCH] Make normal mode 'S' behave like in Vim

[Silvan Jegen]

---
 config.def.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.def.h b/config.def.h
index a2a4b04..b8b9dd5 100644
--- a/config.def.h
+++ b/config.def.h
@@ -247,7 +247,7 @@ static KeyBinding vis_mode_normal[] = {
{ "v",  ACTION(MODE_VISUAL) },
{ "V",  ACTION(MODE_VISUAL_LINE)},
{ "R",  ACTION(MODE_REPLACE)},
-   { "S",  ALIAS("cc") },
+   { "S",  ALIAS("^c$")},
{ "s",  ALIAS("cl") },
{ "Y",  ALIAS("yy") },
{ "X",  ALIAS("dh") },
-- 
2.6.3

[hackers][vis][PATCH] Use VisMotionType enum consistently

[Silvan Jegen]

---
 main.c | 4 ++--
 vis-core.h | 8 +---
 vis.h  | 7 +--
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/main.c b/main.c
index a3b4023..9c99a94 100644
--- a/main.c
+++ b/main.c
@@ -1051,12 +1051,12 @@ static KeyAction vis_action[] = {
[VIS_ACTION_MOTION_CHARWISE] = {
"motion-charwise",
"Force motion to be charwise",
-   motiontype, { .i = VIS_MOTIONTYPE_CHARWISE }
+   motiontype, { .i = CHARWISE }
},
[VIS_ACTION_MOTION_LINEWISE] = {
"motion-linewise",
"Force motion to be linewise",
-   motiontype, { .i = VIS_MOTIONTYPE_LINEWISE }
+   motiontype, { .i = LINEWISE }
},
[VIS_ACTION_UNICODE_INFO] = {
"unicode-info",
diff --git a/vis-core.h b/vis-core.h
index 1ecc1b6..062a3da 100644
--- a/vis-core.h
+++ b/vis-core.h
@@ -58,13 +58,7 @@ typedef struct { /* Motion implementation, takes a cursor 
postion and returns a
size_t (*vis)(Vis*, Text*, size_t pos);
size_t (*view)(Vis*, View*);
size_t (*win)(Vis*, Win*, size_t pos);
-   enum {
-   LINEWISE  = 1 << 0,  /* should the covered range be extended to 
whole lines? */
-   CHARWISE  = 1 << 1,  /* scrolls window content until position 
is visible */
-   INCLUSIVE = 1 << 2,  /* should new position be included in 
operator range? */
-   IDEMPOTENT = 1 << 3, /* does the returned postion remain the 
same if called multiple times? */
-   JUMP = 1 << 4,
-   } type;
+   enum VisMotionType type;
 } Movement;
 
 typedef struct {
diff --git a/vis.h b/vis.h
index 4ee5a99..7f04154 100644
--- a/vis.h
+++ b/vis.h
@@ -264,8 +264,11 @@ int vis_count_get(Vis*);
 void vis_count_set(Vis*, int count);
 
 enum VisMotionType {
-   VIS_MOTIONTYPE_LINEWISE  = 1 << 0,
-   VIS_MOTIONTYPE_CHARWISE  = 1 << 1,
+   LINEWISE   = 1 << 0,  /* should the covered range be extended to whole 
lines? */
+   CHARWISE   = 1 << 1,  /* scrolls window content until position is 
visible */
+   INCLUSIVE  = 1 << 2,  /* should new position be included in operator 
range? */
+   IDEMPOTENT = 1 << 3,  /* does the returned postion remain the same if 
called multiple times? */
+   JUMP   = 1 << 4,
 };
 /* force certain motion to behave in line or character wise mode */
 void vis_motion_type(Vis *vis, enum VisMotionType);
-- 
2.6.3

[hackers][sbase][PATCH] Activate the "else if" branch

[Silvan Jegen]

We checked the same condition in the "if" branch so it was never true
in the "else if" one.  Removing this condition makes the "else if"
branch viable.
---
 ed.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ed.c b/ed.c
index 3f878eb..12411ee 100644
--- a/ed.c
+++ b/ed.c
@@ -1318,7 +1318,7 @@ sighup(int dummy)
line2 = lastln;
if (!setjmp(savesp)) {
dowrite("ed.hup", 1);
-   } else if (home && !setjmp(savesp)) {
+   } else if (home) {
n = snprintf(fname,
 sizeof(fname), "%s/%s", home, "ed.hup");
if (n < sizeof(fname) && n > 0)
-- 
2.6.3

Re: [hackers] [dmenu][RFC][PATCH] History functionality

[Silvan Jegen]

Heyho

On Fri, Dec 11, 2015 at 3:41 PM, Xarchus <xarc...@comcast.net> wrote:
> On Wed, Dec 09, 2015 at 11:16:07AM +0100, Silvan Jegen wrote:
>> On Wed, Dec 9, 2015 at 11:12 AM, Roberto E. Vargas Caballero
>> <k...@shike2.com> wrote:
>> > On Wed, Dec 09, 2015 at 10:31:09AM +0100, Silvan Jegen wrote:
>> >> I realized that I am not dealing with the case that the history file
>> >> does not exist already. I added a simple check for that (although I
>> >> was considering just putting in a comment saying that it has to).
>> >>
>> >
>> >> +if [ ! -e $historyfile ]; then
>> >> +   touch $historyfile
>> >> +fi
>> >
>> > Why the if?, why not directly touch the file?
>>
>> Removing the if is even simpler.
>
> To paraphrase Roberto:
> Why the touch?, why not just let awk create the file?  ;)

Because when I ran dmenu_run awk complained (and may then have created
the file?) and dmenu showed no entries. I think it is bad behaviour
for a program to behave differently when being run two times in a row
with the same arguments.


> That is removing the touch is even simpler.
> (try it: remove the touch, delete the file, run the stuff: in the END block
> in dmenu_run the redirection will create the file)
>
>> I realized that I am not dealing with the case that the history file
>> does not exist already. I added a simple check for that (although I
>> was considering just putting in a comment saying that it has to).
>
> You are actually conflating a few things about that history file.
>
> First of all, let's talk about the variable that holds the name of the
> file.  Such a variable, be it in shell or in awk, should exist and be
> non-empty in order to have some history working.

As I said before, I don't agree with you on this.


> So, to keep things clear, when you said ...
>> I also missed the opportunity to remove file checks from the awk code.
> ... Those checks were guarding against the case when the _variable_ is
> empty/not set, and they had nothing to do with the existence of the actual
> file.

Could be, but it the difference does not matter if you hard code the
file location like I do.


> But since you expressed the preference to have some history specified at
> all times, I will assume for the remaining of the explanation that such a
> variable exists and has a non-empty value.
>
> OK, now the file *name* exists in the code. Such a name can be obviously a
> file *path*. Here are, roughly, the main cases:
>
> A.  The location pointed by the path does not exist.
>
> What I mean here is the `dirname` of that path does not exist; a
> non-existent file can be created, but if one of the intermediate
> directories does not exist, that's bad and the whole dmenu_run fails ugly.
> Do you want to 'mkdir -p $(dirname $HISTORY)' ?
>
> B. The actual file does not exist.
>
>As I've shown above when talking about the extraneous 'touch', the code
> was already handling that.
>
> C. The path is accessible but not writable (including the case the file
> already exists and is not writable).
>
>Updating the history will fail with an error message, but other than
> that, no disasters. This will manifest as a "frozen" history. (The ' stest
> -v -qfrw "$HISTORY" ' in my initial version of dmenu_path was meant to
> guard against that.)
>
> To recap, B is not a problem, C is rare and relatively benign, I can live
> with it; but A is a bit of a problem.
>
> Which brings me back to your preference to hardcode a certain value for the
> history file path. One problem with hardcoded values is to pick one that
> works in all cases.

The whole point is that the person downloading the patch looks at the
script and adjusts the variable to point to a destination he wants to
use. That renders all the above concerns moot.

The ~/.cache/dmenu/history I put there is just what I use.

I would add a comment above the variable like

# Adjust to the location you want to be used for the history file

but even that seems rather redundant given the variable name and the value.


> You picked ~/.cache/dmenu/history . I guess because all those intermediate
> directories exist on _your_ machine. But for other people that could cause
> a failure of type A.
>
> What if this is an account that does not have ~/.cache ?  Or what if the
> owner of that account sets XDG_CACHE_HOME to some other value (and again
> ~/.cache does not exist ?)
>
> If you'll always create the path (with that 'mkdir -p' I've shown earlier),
> or if you just use a file directly in $HOME, you might pollute someone's
> home dir. Are you sure they are ok with that ? (maybe they explicitly set
> that

Re: [hackers] [dmenu][RFC][PATCH] History functionality

[Silvan Jegen]

On Tue, Dec 8, 2015 at 8:34 PM, Silvan Jegen <s.je...@gmail.com> wrote:
> Heyhey
>
> On Thu, Dec 03, 2015 at 02:57:52AM -0800, Xarchus wrote:
>> [...]
>>
>> - improved the history/cache parsing/de-duplication awk one-liner in
>>   dmenu_path; the former 'NR==FNR' test was not enough: in case of a not
>> supplied or empty history file it attempted to remove a count followed by a
>> tab from the file names in the cache. Obviously to find such an occurrence
>> would be crazy rare, so it was quite harmless (I suppose that you don't
>> have any executables in your path of the form "count\tname", do you ?
>> Anyway, now you can :) )
>>
>> New patch attached.
>
> I have tested it briefly and it works for me. I would remove all the
> checks as done in the attached version of the patch.
>
> If you are happy with this version we should put it on the website. The
> question is whether we should replace the current version that does not
> apply to tip or just add it as an alternate history patch.


I realized that I am not dealing with the case that the history file
does not exist already. I added a simple check for that (although I
was considering just putting in a comment saying that it has to).

I also missed the opportunity to remove file checks from the awk code.

Find a new patch containing these changes attached.
diff --git a/dmenu_path b/dmenu_path
old mode 100644
new mode 100755
index 338bac4..c928173
--- a/dmenu_path
+++ b/dmenu_path
@@ -1,4 +1,6 @@
 #!/bin/sh
+HISTORY="$1"
+
 cachedir=${XDG_CACHE_HOME:-"$HOME/.cache"}
 if [ -d "$cachedir" ]; then
 	cache=$cachedir/dmenu_run
@@ -7,7 +9,6 @@ else
 fi
 IFS=:
 if stest -dqr -n "$cache" $PATH; then
-	stest -flx $PATH | sort -u | tee "$cache"
-else
-	cat "$cache"
+	stest -flx $PATH | sort -u > "$cache"
 fi
+awk '!cache { sub("^[0-9]+\t","") } !x[$0]++' "$HISTORY" cache=1 "$cache"
diff --git a/dmenu_run b/dmenu_run
index 834ede5..ef7c579 100755
--- a/dmenu_run
+++ b/dmenu_run
@@ -1,2 +1,31 @@
 #!/bin/sh
-dmenu_path | dmenu "$@" | ${SHELL:-"/bin/sh"} &
+
+historyfile=~/.cache/dmenu/history
+if [ ! -e $historyfile ]; then
+   touch $historyfile
+fi
+
+dmenu_path $historyfile | dmenu "$@" \
+	| awk -v histfile=$historyfile '
+		BEGIN {
+			FS=OFS="\t"
+			while ( (getline < histfile) > 0 ) {
+count=$1
+sub("^[0-9]+\t","")
+fname=$0
+history[fname]=count
+			}
+			close(histfile)
+		}
+
+		{
+			history[$0]++
+			print
+		}
+
+		END {
+			for (f in history)
+print history[f],f | "sort -t '\t' -k1rn >" histfile
+		}
+	' \
+	| while read cmd; do ${SHELL:-"/bin/sh"} -c "$cmd" & done

Re: [hackers] [dmenu][RFC][PATCH] History functionality

[Silvan Jegen]

On Wed, Dec 9, 2015 at 11:12 AM, Roberto E. Vargas Caballero
<k...@shike2.com> wrote:
> On Wed, Dec 09, 2015 at 10:31:09AM +0100, Silvan Jegen wrote:
>> I realized that I am not dealing with the case that the history file
>> does not exist already. I added a simple check for that (although I
>> was considering just putting in a comment saying that it has to).
>>
>
>> +if [ ! -e $historyfile ]; then
>> +   touch $historyfile
>> +fi
>
> Why the if?, why not directly touch the file?

You are right. I confused the history file with the cache file. The
cache file's time stamp is compared to the directories in the PATH to
determine if it has to be updated or not.

Removing the if is even simpler.

Re: [hackers] [dmenu][RFC][PATCH] History functionality

[Silvan Jegen]

Heyhey

On Thu, Dec 03, 2015 at 02:57:52AM -0800, Xarchus wrote:
> [...]
>
> - improved the history/cache parsing/de-duplication awk one-liner in
>   dmenu_path; the former 'NR==FNR' test was not enough: in case of a not
> supplied or empty history file it attempted to remove a count followed by a
> tab from the file names in the cache. Obviously to find such an occurrence
> would be crazy rare, so it was quite harmless (I suppose that you don't
> have any executables in your path of the form "count\tname", do you ?
> Anyway, now you can :) )
> 
> New patch attached.

I have tested it briefly and it works for me. I would remove all the
checks as done in the attached version of the patch.

If you are happy with this version we should put it on the website. The
question is whether we should replace the current version that does not
apply to tip or just add it as an alternate history patch.


Cheers,

Silvan

diff --git a/dmenu_path b/dmenu_path
old mode 100644
new mode 100755
index 338bac4..c928173
--- a/dmenu_path
+++ b/dmenu_path
@@ -1,4 +1,6 @@
 #!/bin/sh
+HISTORY="$1"
+
 cachedir=${XDG_CACHE_HOME:-"$HOME/.cache"}
 if [ -d "$cachedir" ]; then
 	cache=$cachedir/dmenu_run
@@ -7,7 +9,6 @@ else
 fi
 IFS=:
 if stest -dqr -n "$cache" $PATH; then
-	stest -flx $PATH | sort -u | tee "$cache"
-else
-	cat "$cache"
+	stest -flx $PATH | sort -u > "$cache"
 fi
+awk '!cache { sub("^[0-9]+\t","") } !x[$0]++' "$HISTORY" cache=1 "$cache"
diff --git a/dmenu_run b/dmenu_run
index 834ede5..cfb8323 100755
--- a/dmenu_run
+++ b/dmenu_run
@@ -1,2 +1,32 @@
 #!/bin/sh
-dmenu_path | dmenu "$@" | ${SHELL:-"/bin/sh"} &
+
+historyfile=~/.cache/dmenu/history
+
+dmenu_path $historyfile | dmenu "$@" \
+	| awk -v histfile=$historyfile '
+		BEGIN {
+			FS=OFS="\t"
+			if(histfile) {
+while ( (getline < histfile) > 0 ) {
+	count=$1
+	sub("^[0-9]+\t","")
+	fname=$0
+	history[fname]=count
+}
+close(histfile)
+			}
+		}
+
+		{
+			history[$0]++
+			print
+		}
+
+		END {
+			if(!histfile)
+exit
+			for (f in history)
+print history[f],f | "sort -t '\t' -k1rn >" histfile
+		}
+	' \
+	| while read cmd; do ${SHELL:-"/bin/sh"} -c "$cmd" & done

Re: [hackers] [dmenu][RFC][PATCH] History functionality

[Silvan Jegen]

Hi

On Thu, Dec 3, 2015 at 11:57 AM, Xarchus  wrote:
> And a couple of fixes for the 'history' patch:
>
> - fixed the code in the BEGIN block of the inline awk program in dmenu_run;
>   if no history file was supplied the awk script was just ignoring any
> output from dmenu
>
> - improved the history/cache parsing/de-duplication awk one-liner in
>   dmenu_path; the former 'NR==FNR' test was not enough: in case of a not
> supplied or empty history file it attempted to remove a count followed by a
> tab from the file names in the cache. Obviously to find such an occurrence
> would be crazy rare, so it was quite harmless (I suppose that you don't
> have any executables in your path of the form "count\tname", do you ?
> Anyway, now you can :) )
>
> New patch attached.

Thanks!

At the moment I don't have time to test the patch I am afraid but I
will test it as soon as I find the time.

I would prefer to not deal with the case the HISTORY is unset and just
assume it is. The reason being that if people don't want to use the
history functionality, they should not use this patch. Under this
assumption the code can be further simplified.

I also prefer setting the HISTORY variable to a file name explicitly
as opposed to in an environment variable with fallback code but that
may only be me. Setting the history file name directly in dmenu_run is
not any harder than setting an environment variable and checking for
default location would also not be needed then.


Cheers,

Silvan

Re: [hackers] [dmenu][RFC][PATCH 0/4] Using sort and simple C program to get dmenu history functionality

[Silvan Jegen]

On Tue, Dec 1, 2015 at 8:04 PM, Silvan Jegen <s.je...@gmail.com> wrote:
> Heyho!
>
> On Tue, Dec 01, 2015 at 05:51:59AM -0800, Xarchus wrote:
>>
>> This updhist awk script replacement will work with multiselect (multiple
>> inputs will simply increment their count or added as new). This includes
>> the case when dmenu outputs duplicate strings (with multiselect, a same
>> entry can be generated multiple times).
>>
>> But speaking of multiselect, the dmenu_run as it is now does not handle
>> very gracefully multiple selections ... Multiple programs selected in dmenu
>> will all be started, but sequentially, with the next waiting for the
>> previous to exit (an artifact of them all being fed to one shell instance).
>> I am not sure if that is the intention: my preference would be to start
>> each program as soon as it's selected with Ctrl-Return.
>
> Personally I don't use multiselect so I will let others deal with that
> use case...

Maybe I care a bit after all...

Using something like

echo -e "echo 1\nsleep 5; echo awesome\necho 200" | while read cmd; do
${SHELL:-"/bin/sh"} -c "$cmd" & done

would start each program after after a newline has been read. I don't
know how multiselect is implemented but it should be easily possible
to send the program name with newline after Ctrl-Return has been
pressed.

Re: [hackers] [dmenu][RFC][PATCH 0/4] Using sort and simple C program to get dmenu history functionality

[Silvan Jegen]

Heyho!

On Tue, Dec 01, 2015 at 05:51:59AM -0800, Xarchus wrote:
> On Mon, Nov 30, 2015 at 03:28:42PM +0100, Silvan Jegen wrote:
> > Heyho!
> > 
> > On Sat, Nov 28, 2015 at 11:25 PM, Hiltjo Posthuma
> > <hil...@codemadness.org> wrote:
> > >
> > > This can be implemented in a few lines of shell (wc, sort) and maybe awk.
> > 
> > I *have* implemented the history part with sort. If you think the
> > history updating functionality that I ended up writing in C can be
> > (easily?) implemented in some shell script and/or awk then I would
> > like to see it on the list :)
> > 
> > I thought it should be possible to implement in awk but because you
> > have to both read input from stdin (the command) and from a file (the
> > history file) I couldn't figure it out in the admittedly short time I
> > kept trying to do it.
> > 
> > I tried abusing the -v option of gawk to set the command name as a
> > variable value but that only seems to work in a BEGIN block which is
> > executed before the input file is read. It may be possible to use the
> > shell to read the command and then initialize a variable in the awk
> > code given on the command line but I think the argument-escaping hell
> > would be very annoying to deal with.
> > 
> 
> As both Silvan and Hiltjo mentioned, here is an awk script that does the
> job of updhist. (more than that, it fixes a problem: the updhist.c as sent
> does not work with multisel, it will corrupt the history file if multiple
> selections are generated from a single dmenu invocation; but more about
> multiselect later)

I was aware that update.c does not deal with the case of multiple-line
commands. I did not know that there exists such a functionality for
dmenu though so I ignored it in the first version...


> In order to make the script more portable, I tried to keep the awk features
> limited to the nawk set and not using any of the gawk stuff. With gawk,
> things can get even simpler: for example gawk has sort, so the external sort
> would not be needed in the END block.
> (BTW, which is the target as the *official* awk?)

I prefer using the external sort.

I don't know if there is an "official" suckless awk. Do you know if your
script works with the BSD awk implementation?


> The script assumes the format of the history file to be 'filename tab
> count', the same as the C program. The problem with this is what happens
> if a file name contains tabs ... Crazy, but not impossible. A more reliable
> approach would be to put the count first then the remaining line is all a
> file name.

While I would be willing to take that risk, doing it your way should
simplify the sort command slightly as well and is a good idea.


> Anyhow, here is the script (tested) :
> 
> -%<- updhist.awk
> #!/usr/bin/awk -f
> 
> BEGIN {
> if(histfile=="") {
> print "updhist.awk: no history file specified" > "/dev/stderr"
> print "usage: awk -v histfile= -f updhist.awk" 
> > "/dev/stderr"
> exit(1)
> }
> FS=OFS="\t" # explicit tabs for input to allow file names with spaces 
> while ( (getline < histfile) > 0 )
> history[$1]=$2 # assumption: file name does not contain tabs
> close(histfile)
> }
> 
> {
> history[$0]++
> print
> }
> 
> END {
> for (f in history)
> print f,history[f] | "sort -t '\t' -k2rn >" histfile
> }
> -%<-
> 
> To use the above, replace in dmenu_run the call to updhist with 'awk -v
> histfile=$historyfile -f updhist.awk' (assuming updhist.awk is placed
> somewhere in AWKPATH, or /usr/share/awk, otherwise use the full path to the
> awk script).
> 
> Because the script will keep the history file already sorted, in
> 'dmenu_path' there is no need to sort the history when fed to dmenu (so
> leave out the 'sort -r -n -t ' ' -k 2').
> 
> Silvan talked about the fact that the commands in the history will show up
> twice: once from the history, once in the normal list. This too can be
> taken care of with a tiny awk one-liner to filter duplicates; that will
> replace the 'cat' at the end of the two lines in dmenu_path with this:
> 
> awk '!x[$0]++' - "$cache"
> 
> Going even further, the cut operation can be factored into awk (I assume it
> does cut on the tab that separates the name from the counter), so the whole
> line now becomes (this relies on a history format with the name first, tab,
> followed by the count):
> 
> awk -F$'\t' '!x[$1]++' "$HISTORY" "$cache&qu

Re: [hackers] [dmenu][RFC][PATCH 0/4] Using sort and simple C program to get dmenu history functionality

[Silvan Jegen]

Heyho!

On Sat, Nov 28, 2015 at 11:25 PM, Hiltjo Posthuma
<hil...@codemadness.org> wrote:
> On Fri, Nov 27, 2015 at 7:38 PM, Silvan Jegen <s.je...@gmail.com> wrote:
>> Heyhey
>>
>> I kept thinking about a more general way to implement history
>> functionality for dmenu and this is what I came up with.
>>
>> We use the sort command to generate an input list for dmenu sorted by
>> count (first patch). dmenu itself is not modified and shows the available
>> commands with the most often used ones first (due to the sorting done
>> by sort). Before sending the dmenu output to a shell we use a simple
>> C program that reads a command from stdin for which it increments the
>> usage count in the history file before sending the command to the shell
>> to execute (patch 4; rest of the patches are just glue code).
>>
>
> This can be implemented in a few lines of shell (wc, sort) and maybe awk.

I *have* implemented the history part with sort. If you think the
history updating functionality that I ended up writing in C can be
(easily?) implemented in some shell script and/or awk then I would
like to see it on the list :)

I thought it should be possible to implement in awk but because you
have to both read input from stdin (the command) and from a file (the
history file) I couldn't figure it out in the admittedly short time I
kept trying to do it.

I tried abusing the -v option of gawk to set the command name as a
variable value but that only seems to work in a BEGIN block which is
executed before the input file is read. It may be possible to use the
shell to read the command and then initialize a variable in the awk
code given on the command line but I think the argument-escaping hell
would be very annoying to deal with.


>> * It uses the -t and -k options of sort which are not available for
>>   all sort implementations (not in sbase for example). If there is an
>>   easy way to replicate this functionality without using these sort
>>   options I would like to hear about it.
>
> Sbase sort supports -t and -k, if some part of it is broken: send a patch.

You are right. I must have checked an old sbase version. I will test
the patch with sbase sort as soon as I find the time.


Cheers,

Silvan

[hackers] [dmenu][RFC][PATCH 4/4] Add the updhist program

[Silvan Jegen]

This program reads a command (or any character sequence) from stdin and
takes the path to the history file as an argument. The history file should
be empty or contain a list of commands each of which should be followed
by a tab character and a usage count. An example for a line in such a
history file is the following.

st  234

updhist reads a command from stdin, increases its count in the history
file, writes the file out again (overwriting the old one) and then writes
the command to stdout before exiting.
---
 updhist.c | 84 +++
 1 file changed, 84 insertions(+)
 create mode 100644 updhist.c

diff --git a/updhist.c b/updhist.c
new file mode 100644
index 000..8078b3e
--- /dev/null
+++ b/updhist.c
@@ -0,0 +1,84 @@
+/* See LICENSE file for copyright and license details. */
+#include 
+#include 
+#include 
+
+#include "util.h"
+
+static void
+usage(void)
+{
+   printf("usage: updhist historyfile\n");
+   exit(0);
+}
+
+static void
+update_hist_file(char* p, char* cmdname, size_t len)
+{
+   FILE *hist = NULL, *nhist = NULL;
+   char *tmpname;
+   char buf[BUFSIZ];
+   int ret, searching = 1;
+   long int count;
+   size_t fnlen;
+
+   if (!(hist = fopen(p, "r")))
+   die("Could not open history file: %s\n", p);
+
+   fnlen = strlen(p);
+   tmpname = malloc(fnlen+5);
+   strcpy(tmpname, p);
+   strcat(tmpname, ".tmp");
+   if (!(nhist = fopen(tmpname, "w")))
+   die("Could not open the history file to write: %s\n", p);
+
+   while (fscanf(hist, "%200s\t%ld", buf, ) == 2) {
+   if (searching) {
+   searching = strcmp(buf, cmdname);
+   if (searching==0)
+   count++;
+   }
+
+   fprintf(nhist, "%s\t%lu\n", buf, count);
+   }
+   if (searching)
+   fprintf(nhist, "%s\t%d\n", cmdname, 1);
+
+   ret = feof(hist);
+   if (!ret)
+   die("Could not parse history file completely. Exiting.\n");
+
+   fclose(hist);
+   fclose(nhist);
+
+   ret = rename(tmpname, p);
+   if (ret)
+   die("Could not overwrite old history file with the new one. 
Exiting.\n");
+   free(tmpname);
+}
+
+int
+main(int argc, char *argv[])
+{
+   size_t n, len;
+   char stdinbuf[BUFSIZ];
+
+   if (argc < 2)
+   usage();
+
+   while ((n = fread(stdinbuf, 1, sizeof(stdinbuf), stdin))) {
+   if (fwrite(stdinbuf, 1, n, stdout) == n)
+   continue;
+   fprintf(stderr, "fwrite to stdout did not write all data. Wrote 
only %lu bytes.\n", n);
+   }
+   fclose(stdout);
+
+   len = strlen(stdinbuf);
+   if (len == 0)
+   return 1;
+
+   stdinbuf[--len] = '\0';
+   update_hist_file(argv[1], stdinbuf, len);
+
+   return 0;
+}
-- 
2.6.2

[hackers] [dmenu][RFC][PATCH 0/4] Using sort and simple C program to get dmenu history functionality

[Silvan Jegen]

Heyhey

I kept thinking about a more general way to implement history
functionality for dmenu and this is what I came up with.

We use the sort command to generate an input list for dmenu sorted by
count (first patch). dmenu itself is not modified and shows the available
commands with the most often used ones first (due to the sorting done
by sort). Before sending the dmenu output to a shell we use a simple
C program that reads a command from stdin for which it increments the
usage count in the history file before sending the command to the shell
to execute (patch 4; rest of the patches are just glue code).

Issues with this approach:

* It uses the -t and -k options of sort which are not available for
  all sort implementations (not in sbase for example). If there is an
  easy way to replicate this functionality without using these sort
  options I would like to hear about it.

* Because the regular list of commands is still sent to dmenu, all the
  ones in the history file will be present twice. As long as you use only
  the most often used commands that should not matter (unless you are
  bothered by the same command being shown twice next to each other when
  you filter out enough of the more rarely used ones).

* The updhist program would be a good candidate to be implemented in a
  scripting language since it's so simple. Ideally it would be a small one
  like awk but I could not figure out how to make awk's string matching
  depend on some input other than the input file itself. Suggestions
  are welcome!


Silvan Jegen (4):
  Use sort to generate the command list
  Pass a file path to dmenu_path and run updhist
  Add updhist to the Makefile
  Add the updhist program

 Makefile   | 12 ++---
 dmenu_path | 12 +++--
 dmenu_run  |  5 +++-
 updhist.c  | 84 ++
 4 files changed, 106 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 dmenu_path
 create mode 100644 updhist.c

-- 
2.6.2