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

From: Xarchus <xarchus_AT_comcast.net>
Date: Fri, 11 Dec 2015 06:41:00 -0800

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
> <k0ga_AT_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? ;)

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.

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.

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.

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 XDG_CACHE_HOME for a reason ? :) ).


The stock 'dmenu_path' handles similar cases for its cache file: locates the
correct .cache if it exists, otherwise *falls back* to something in the
home dir. All that is *guaranteed* to work. It is just tidy to do a similar
thing with the history.


Which brings me to a bit of discussion about principles. You might say
"works for me as it is, I will add comments in the file as instructions...
Let the others edit it themselves". I think the courteous thing to do is
have a general version of the patch that works as is no matter what.

And if you want to change things, just edit your local copy. How about
that?


To summarize, have HISTORY set at all times (so no extra checks in the
awk), but with a code similar to what dmenu_path does for the cache.
Also, no code to guard against a non-writable path. Agree?

(and BTW, I found a small optimization but we can talk about that later)
Received on Fri Dec 11 2015 - 15:41:00 CET