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

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Fri, 11 Dec 2015 16:23:37 +0100

Heyho

On Fri, Dec 11, 2015 at 3:41 PM, Xarchus <xarchus_AT_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
>> <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? ;)

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 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.

I don't agree because the whole point of the history patch is to use
the history so it has to be set for the functionality to be there.

In the vanilla dmenu case, the program would work without a cache as
well. It's just an implementation detail for efficiency reasons.


> 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.

Yepp, I think they should look at the code they downloaded before they
run it and it will be immediately obvious what they will have to do.


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

Sure, that's fine. Other people don't seem to have a strong opinion on
this so let's move ahead. I am already running my version but I think
it's more important that there is an updated version of the patch
available online soon.


> 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)

Ok. If the optimization is not too much work to implement just send an
updated version of the patch for discussion.
Received on Fri Dec 11 2015 - 16:23:37 CET