w3m icon indicating copy to clipboard operation
w3m copied to clipboard

Set `rc_dir` based on `W3M_DIR` environment variable.

Open yashlala opened this issue 3 years ago • 17 comments

By default, w3m puts all of its data in the ~/.w3m/ directory (creating it as necessary). This was not configurable in any way.

This commit addresses this problem. When the W3M_DIR environment variable is set, w3m will use its contents to store its data files instead. The default location is unchanged.

This PR should resolve issue #130. Sorry for the delay :)

yashlala avatar Dec 12 '21 08:12 yashlala

Thanks for the feedback @rkta -- I've made the changes you suggested (+ some minor fixes).

yashlala avatar Dec 12 '21 20:12 yashlala

I had a closer look and found another more serious issue.

PATH_MAX is problematic, but I'm ok with its use here. Though the strncpy(component, dir, sizeof(component)); needs an error check.

And I would squash those commits, but that's not on me to decide. Otherwise LGTM.

Thanks!

rkta avatar Dec 13 '21 18:12 rkta

Thanks for catching the strncpy bug -- the latest push should fix it. I've updated to a proper malloc style implementation to address the former reservation.

I was under the impression that squashing (if desired) happens when merging -- I can squash them now no problem though.

yashlala avatar Dec 15 '21 02:12 yashlala

With this changes we don't need limits.h anymore, do we?

rkta avatar Dec 15 '21 12:12 rkta

Fixed. I appreciate your patience.

yashlala avatar Dec 15 '21 17:12 yashlala

One more thing:

rc.c: In function ‘init_rc’: rc.c:1353:17: warning: unused variable ‘st’ [-Wunused-variable] 1353 | struct stat st; | ^~

Sorry for being that late.

rkta avatar Jan 14 '22 16:01 rkta

Fixed.

yashlala avatar Jan 14 '22 19:01 yashlala

I hope this will be merged eventually. It's a useful feature.

butyoutried avatar Jun 01 '22 11:06 butyoutried

On Wed, Jun 01, 2022 at 04:59:13AM -0700, N-R-K wrote:

@N-R-K commented on this pull request.

  • n = strlen(dir);
  • if (n == 0)
  • return -1;
  • if ((dircpy = malloc(n + 1)) == NULL)
  • return -1;
  • strcpy(dircpy, dir);

Any reason for not using strdup(3) ?

Or even better 'Strnew_charp()' - and then replace the above 'strlen' call and 'if' with:

if (*n = '\0') return -1;

rkta avatar Jun 01 '22 13:06 rkta

@rkta The syscall below needs a const char *, so I'd have to break encapsulation anyways (and I thought the Str getters and indexing would overcomplicate the function). do_mkdir got away with standard C, so I followed :)

Is there a reason you'd prefer that construct?

yashlala avatar Jun 03 '22 01:06 yashlala

On Thu, Jun 02, 2022 at 06:16:55PM -0700, yashlala wrote:

@rkta The syscall below needs a const char *, so I'd have to break encapsulation anyways (+ I thought the Str getters and indexing would overcomplicate the function). do_mkdir got away with standard C, so I followed :)

Instead of: if ((dircpy = malloc(n + 1)) == NULL) return -1; strcpy(dircpy, dir);

you can do: dircpy = Strnew_charp(dir)->ptr;

and you don't need to care about malloc() and free(). Strnew_charp() will exit(1) if malloc() fails and garbage collect the memory if it's not used anymore.

Is there a reason you'd prefer that construct?

I'd say it's the common pattern used in the code base, but it's not really that important.

rkta avatar Jun 03 '22 06:06 rkta

Huh. I'm amazed the garbage collector can differentiate C pointer data from integer data to that degree of confidence.

I've made the change you suggested. Please let me know if there's anything else I should do before it's merged.

yashlala avatar Jun 04 '22 00:06 yashlala

Hi @N-R-K ; Github is telling me that you need to review this before the patch can be merged into main, so I'm reaching out again. If there's anything I need to do before it's ready for upstream, I'm happy to do so.

yashlala avatar Jul 01 '22 19:07 yashlala

Github is telling me that you need to review this before the patch can be merged into main

Huh, I think that's just some generic UI thing. I'm not the maintainer of this project (@tats is) nor am I in any position to decide what gets merged to master.

N-R-K avatar Jul 01 '22 19:07 N-R-K

Alright then. @tats -- unless you have suggestions, I think this PR is finalized and ready to merge.

yashlala avatar Jul 01 '22 19:07 yashlala

Any update?

ignamartinoli avatar Aug 22 '22 07:08 ignamartinoli

I plan to merge this, though still XDG-non-compliant.

tats avatar Aug 22 '22 21:08 tats

Merged, thanks for your contribution.

tats avatar Dec 21 '22 11:12 tats