Ticket #3684 (closed defect: fixed)

Opened 8 years ago

Last modified 7 years ago

Setting MC_HOME makes mc ignore ~/.bashrc

Reported by: cri Owned by: andrew_b
Priority: major Milestone: 4.8.19
Component: mc-core Version: master
Keywords: Cc: opotapenko@…
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

Sometimes I want to start mc with a particular configuration; it seems that the only way to do this is by creating a customised copy of the ~/.config/mc tree and set MC_HOME to it. (is there a cleaner way?)

This works, but when I run mc with the new MC_HOME, it ignores my ~/.bashrc so all my usual shell customisations (aliases etc) won't work.

Workaround: if I symlink ~/.bashrc into the new MC_HOME dir, my aliases will work again, but I think this shouldn't be needed, so maybe this is a bug?

Change History

comment:1 Changed 8 years ago by cri

Ouch this is even worse: if I type cd on the mc command line this brings me to the MC_HOME directory, instead of my HOME (even if echo $HOME outputs my correct HOME)

comment:2 follow-up: ↓ 3 Changed 8 years ago by mooffie

when I run mc with the new MC_HOME, it ignores my ~/.bashrc so all my usual shell customisations (aliases etc) won't work.
Workaround: if I symlink ~/.bashrc into the new MC_HOME dir, my aliases will work again, but I think this shouldn't be needed, so maybe this is a bug?
Ouch this is even worse: if I type cd on the mc command line this brings me to the MC_HOME directory, instead of my HOME


This looks like a problematic behavior to me too.

I wonder what was the purpose of MC_HOME. The commit that introduced it doesn't explain the rationale behind it.

There could have been three reasons for the existence of MC_HOME:

  1. A means to set the paths of MC's configuration/data files. or:
  2. A replacement for the home directory. or:
  3. Some other reason.

If it's (1), then this env variable should be used in mc_config_init_config_paths() only. Not in mc_config_get_home_dir(). And it should be given a more correct name, like MC_PROFILE_HOME.

If it's (2), then... ok. In this case MC_HOME behaves "as advertised". This behavior doesn't seem to be very useful, though.

If it's (3), then... we'll have to wait for someone to explain this reason.


Sometimes I want to start mc with a particular configuration; it seems that the only way to do this is by


Well, when we think about it we see that MC doesn't provide a way to do this.

You're not the only one looking for this feature.

(While it's possible to set the XDG_{CONFIG,DATA,CACHE}_HOME environment variables before launching MC, this method has a problem: applications we launch from MC will inherit these modified XDG dirs too, so they won't find their data.)

This is a problem.

@Andrew: what do you suggest? Some possibilities I see:

  • Narrow the scope of MC_HOME to affect only configuration/data files. That is, option (1) above.
  • Introduce a new env variable: MC_PROFILE_HOME. It'd work like the MC_HOME I've just described.
  • Introduce a new env variable (or command-line option): MC_PROFILE_NAME. It'd stand for the "/mc/" component in the pathnames. The advantage over MC_PROFILE_HOME is that it's easier to backup (and less clutter in the home dir): the profiles reside inside the standard XDG homes instead of being outside of them.

comment:3 in reply to: ↑ 2 ; follow-up: ↓ 6 Changed 8 years ago by andrew_b

Replying to mooffie:

@Andrew: what do you suggest?

After 4,5 years I'm unable to remember what we wanted exactly. Рrobably, MC_HOME should be the root of configuration only.

Some possibilities I see:

  • Narrow the scope of MC_HOME to affect only configuration/data files. That is, option (1) above.
  • Introduce a new env variable: MC_PROFILE_HOME. It'd work like the MC_HOME I've just described.
  • Introduce a new env variable (or command-line option): MC_PROFILE_NAME. It'd stand for the "/mc/" component in the pathnames. The advantage over MC_PROFILE_HOME is that it's easier to backup (and less clutter in the home dir): the profiles reside inside the standard XDG homes instead of being outside of them.

I think, MC_HOME should be renamed to MC_PROFILE_HOME or MC_PROFILE_ROOT. In addition to environment variable, the new command line option can be created: -p/--profile or -r/--config-root or something else. As usually, the command-line option overrides the environment variable.

comment:4 follow-up: ↓ 8 Changed 8 years ago by andrew_b

  • Blocked By 3682 added

comment:5 Changed 8 years ago by cri

Thank you for your detailed answers.

three reasons for the existence of MC_HOME:

A means to set the paths of MC's configuration/data files

...

Narrow the scope of MC_HOME to affect only configuration/data files. That is, option (1) above.

That's exactly what I was looking for; like many other programs do, mc should provide a way to temporarily override the usual config file(s)/dir and use a different one.

I think, MC_HOME should be renamed to MC_PROFILE_HOME or MC_PROFILE_ROOT. In addition to environment variable, the new command line option can be created: -p/--profile or -r/--config-root or something else. As usually, the command-line option overrides the environment variable.

Sounds like a very good plan to me!

Alternatively, if you can't recall the original rationale of MC_HOME, instead of renaming it and possibly breaking backwards compatibility, you can add a new "narrower" variable MC_PROFILE_HOME (or whatever you want to call it) for the purpose described above. (and explain in the man page the difference in "scope")

comment:6 in reply to: ↑ 3 Changed 8 years ago by mooffie

Replying to andrew_b:

[...] MC_HOME should be the root of configuration only. [...] should be renamed to MC_PROFILE_HOME or MC_PROFILE_ROOT.


I'm attaching a patch.

  • I named it MC_PROFILE_ROOT, not MC_PROFILE_HOME, to make it clearer to users that the data isn't stored directly within but in subfolders. (And I also thought the word "home" is likelier to lead to confusion.)
  • I haven't added a cmdline option for setting it (i.e. --profile-root) as we don't have any other option for setting dirs and it looked weird to me to make an exception. I can easily add a cmdline option if you want me to. When I proposed a cmdline option (comment:2) it was for something else: for a profile name (I'll clarify this in the next comment).
  • The patch uses strcmp() in one place. An earlier version managed without but I think it was uglier.
  • Interegsintly, the modification to textconf.c reverses commit e4fc99d40.
  • An update to the manual page will come later.
  • To apply this patch you first need to apply the two patches I posted at #3641 (comment 14 there).

Replying to cri:

Alternatively, if you can't recall the original rationale of MC_HOME, instead of renaming it and possibly breaking backwards compatibility,


I don't think we need to bother about backward compatibility: anybody who's been using MC_HOME must have noticed that it was broken ("cd" brings him to illogical place; shell doesn't work as expected (no aliases etc)).

Last edited 8 years ago by mooffie (previous) (diff)

Changed 8 years ago by mooffie

comment:7 follow-up: ↓ 9 Changed 8 years ago by mooffie

Now, in comment:2 I proposed two different mechanisms: a profile root (which I called profile home) and a profile name. We should later decide if we want to implement the profile name mechanism as well. It's easy, but it should be done in a separate ticket.

Let me demonstrate the difference between the two. We'll take as example the "history" file:

Mechanism #1: profile root

$ MC_PROFILE_ROOT=~/koko mc

the file's path is:

~/koko/.local/share/mc/history

Mechanism #2: profile name

$ MC_PROFILE_NAME=mc-koko mc

the file's path is:

~/.local/share/mc-koko/history

comment:8 in reply to: ↑ 4 Changed 8 years ago by mooffie

Oops, I forgot to reply to this comment:

Replying to andrew_b:

Blocked By 3682 added


Why?

I don't see any connection between the issues.

comment:9 in reply to: ↑ 7 Changed 8 years ago by cri

Replying to mooffie:

Now, in comment:2 I proposed two different mechanisms: a profile root (which I called profile home) and a profile name. We should later decide if we want to implement the profile name mechanism as well. It's easy, but it should be done in a separate ticket.

For the use case that I described when I opened this ticket (i.e. if the user wants to create an "alternative" profile) maybe the "profile root" mechanism is better than the "profile name", since all the user customisations will stay in the MC_PROFILE_ROOT tree (with "profile name" they would be scattered in different trees under ~/.cache, ~/.config and ~/.local) so it seems more manageable.

Unfortunately I can't test your patch right now, I'm wondering what happens if I run

$ MC_PROFILE_ROOT=~/koko mc

when ~/koko doesn't exist (or is empty)?

My assumption is that it should be automatically created, and all the configuration files/dirs needed to make mc work should be created too, copying them from the standard mc configuration tree. This way, it would be very convenient for the user to:

  1. run $ MC_PROFILE_ROOT=~/koko mc
  2. exit mc
  3. make the needed customisation to configuration files in ~/koko
  4. enjoy the new profile

comment:10 follow-up: ↓ 12 Changed 8 years ago by alllexx88

To be frank, I haven't thoroughly read all comments, just scanned them through, so sorry if I'm missing something, however, as far as the original bug report goes, it can be easily fixed by changing just two lines of code (attaching patch next). The idea is to load user ~/.bashrc (or ~/.profile for dash) when respective mc rc files (mc_config_get_full_path ("bashrc"), mc_config_get_full_path ("ashrc")) don't exist, regardless of MC_HOME presence/value. But this is as simple as setting init_file values to full paths, e.g., 'init_file = g_strdup_printf ("%s/.bashrc", g_getenv ("HOME"));' instead of 'init_file = g_strdup (".bashrc");', since the latter will load .bashrc from CWD, which is mc home dir, and equals to $MC_HOME if set. This fix has been tested to work fine for bash, dash and busybox ash.

comment:11 Changed 8 years ago by alllexx88

Sorry if I sounded a bit obscure. I forgot to mention that subshell initialization code is in src/subshell/common.c, and bash subshell is actually started with this command:

execl (mc_global.shell->path, mc_global.shell->name, "-rcfile", init_file, (char *) NULL);

And before this is executed, working dir is changed to mc homedir, hence everything else I wrote earlier applies.

comment:12 in reply to: ↑ 10 ; follow-ups: ↓ 13 ↓ 14 Changed 8 years ago by cri

Replying to alllexx88:

To be frank, I haven't thoroughly read all comments

Well, if you do that, you'll find out that ~/.bashrc is only part of the problem; so your solution is partial.

I think the "profile root" solution proposed by mooffie is more desirable: it fixes the current bugs with MC_HOME and adds a useful feature to mc.

comment:13 in reply to: ↑ 12 ; follow-up: ↓ 16 Changed 8 years ago by mooffie

Replying to cri:

Unfortunately I can't test your patch right now, I'm wondering what happens if I run

$ MC_PROFILE_ROOT=~/koko mc

when ~/koko doesn't exist (or is empty)?

My assumption is that it should be automatically created, and all the configuration files/dirs needed to make mc work should be created too, copying them from the standard mc configuration tree.


Right. Your assumptions are correct. $MC_PROFILE_ROOT affects only the placement of the configuration/data files, not their behavior.

Replying to cri:

Replying to alllexx88:

To be frank, I haven't thoroughly read all comments

Well, if you do that, you'll find out that ~/.bashrc is only part of the problem; so your solution is partial.

I think the "profile root" solution proposed by mooffie is more desirable: it fixes the current bugs with MC_HOME and adds a useful feature to mc.


I agree. I think every patch should contribute something to MC's code health and I don't see this in @alllexx88's patch: it skirts a problem (and only partially) instead of fixing it. If MC_HOME is broken, it has to go.

@Andrew: What are you thoughts? (I'll provide a patch fixing the documentation if I get green light.)

comment:14 in reply to: ↑ 12 ; follow-up: ↓ 17 Changed 8 years ago by alllexx88

Replying to cri:

Well, if you do that, you'll find out that ~/.bashrc is only part of the problem; so your solution is partial.

I found only two issues you reported:
1) No aliases
2) cd brings to "illogical" places

The first one gets fixed with the patch, by loading proper ~/.bashrc. As for the second, this doesn't happen in the subshell:

jenkins@u0:~$ MC_HOME=/tmp mc

jenkins@u0:~$ cd /tmp
jenkins@u0:/tmp$ cd
jenkins@u0:~$ pwd
/home/jenkins

but indeed happens in mc command line, and it makes sense that cd in mc takes you to mc home.

What other issues do you mean?

Replying to cri:

I think the "profile root" solution proposed by mooffie is more desirable: it fixes the current bugs with MC_HOME and adds a useful feature to mc.

Replying to mooffie:

I agree. I think every patch should contribute something to MC's code health and I don't see this in @alllexx88's patch: it skirts a problem (and only partially) instead of fixing it. If MC_HOME is broken, it has to go.

I took a close look at the patch, and then made a research on its impact. After this patch mc_config_get_home_dir() always returns $HOME, and that's the only change that goes beyond config paths generation mechanism, so I grepped for where it's being invoked:

$ grep mc_config_get_home_dir  `find -type f -name '*.[ch]'`
./src/main.c:    if (!g_path_is_absolute (mc_config_get_home_dir ()))
./src/main.c:                            mc_config_get_home_dir ());
./src/subshell/common.c:        ret = chdir (mc_config_get_home_dir ());        /* FIXME? What about when we re-run the subshell? */
./src/vfs/ftpfs/ftpfs.c:    netrcname = g_build_filename (mc_config_get_home_dir (), ".netrc", (char *) NULL);
./src/textconf.c:    (void) printf ("%s %s\n", _("Root directory:"), mc_config_get_home_dir ());
./src/filemanager/hotlist.c:        listbox_add_item (l_hotlist, LISTBOX_APPEND_AT_END, 0, mc_config_get_home_dir (), NULL,
./src/filemanager/hotlist.c:        listbox_add_item (l_hotlist, LISTBOX_APPEND_AT_END, 0, mc_config_get_home_dir (), NULL,
./src/filemanager/tree.c:    vpath = vfs_path_from_str (mc_config_get_home_dir ());
./src/filemanager/usermenu.c:                mc_build_filename (mc_config_get_home_dir (),
./src/filemanager/command.c:            new_vpath = vfs_path_from_str (mc_config_get_home_dir ());
./src/filemanager/command.c:            q_vpath = vfs_path_from_str (mc_config_get_home_dir ());
./lib/mcconfig/paths.c:    return g_build_filename (mc_config_get_home_dir (), MC_OLD_USERCONF_DIR, (char *) NULL);
./lib/mcconfig/paths.c:    (void) mc_config_get_home_dir ();
./lib/mcconfig/paths.c:        dir = g_build_filename (mc_config_get_home_dir (), MC_USERCONF_DIR, (char *) NULL);
./lib/mcconfig/paths.c:mc_config_get_home_dir (void)
./lib/mcconfig.h:const char *mc_config_get_home_dir (void);
./lib/vfs/path.c:    const char *home_dir = mc_config_get_home_dir ();
./tests/src/filemanager/do_cd_command.c:static const char *mc_config_get_home_dir__return_value;
./tests/src/filemanager/do_cd_command.c:mc_config_get_home_dir (void)
./tests/src/filemanager/do_cd_command.c:    return mc_config_get_home_dir__return_value;
./tests/src/filemanager/do_cd_command.c:    mc_config_get_home_dir__return_value = "/home/test";
./tests/src/filemanager/do_cd_command.c:    mctest_assert_str_eq (mc_config_get_home_dir__return_value,
./tests/lib/vfs/path_recode.c:mc_config_get_home_dir (void)
./tests/lib/vfs/vfs_path_from_str_flags.c:mc_config_get_home_dir (void)

I already mentioned src/subshell/common.c -- that's the cause of the original missing aliases bug report. Let's look at other things that appear important to me:

in src/vfs/ftpfs/ftpfs.c, mc_config_get_home_dir () defines which .netrc file is loaded, and after the change it will always be ~/.netrc. On one hand it's good, but on the other we won't be able to have an mc-specific .netrc.

in src/filemanager/*.c, looks like MC_HOME is really being used as a home instead of the $HOME (sorry for the pun).

And finally, the *home_dir = mc_config_get_home_dir (); line in lib/vfs/path.c is the reason cd command in mc takes you to mc home rather then your $HOME, I think.

To sum up, looks like MC_HOME is indeed more of a home than a profile root, and if you want to change that, please take a closer look at src/vfs/ftpfs/ftpfs.c and those src/filemanager/*.c, and make sure you're happy with the change.

Regards,
Alex

comment:15 Changed 8 years ago by alllexx88

  • Cc opotapenko@… added

comment:16 in reply to: ↑ 13 Changed 8 years ago by alllexx88

Replying to mooffie:

I think every patch should contribute something to MC's code health and I don't see this in @alllexx88's patch

Don't mean to insult you, honestly, but you should have another look. Using relative file paths when we in fact know full paths isn't healthy. What if one day someone adds a patch to not change dir before subshell initialization for some important reason? How currently easy it is for them to figure out that they also have to change lines init_file = g_strdup (".bashrc"); and init_file = g_strdup (".profile");? This will only lead to missing aliases if you're outside of HOME, no errors, and they are quite capable to overlook that in their tests. If you use absolute paths, you have no such problems = obvious MC's code health contribution.

comment:17 in reply to: ↑ 14 Changed 8 years ago by mooffie

Replying to alllexx88:

Don't mean to insult you, honestly, but you should have another look.


@alllexx88: Your sin is called psychological projection. You assume that because you are sloppy, everybody else must be sloppy.

You dumped a list of 24 places where mc_config_get_home_dir() is used and you accuse me of botching them.

You didn't bother to ask first.

You didn't care about swamping this ticket with irrelevant data and noise, something that may possibly sabotage it.

That's not friendly.

Here's news for you: I went and checked every place before I posted the patch. I did my homework. I did "take a closer look" at those places. You didn't.

I took a close look at the patch, and then made a research on its impact.


No, you didn't. You made no such "research".

To sum up, looks like MC_HOME is indeed more of a home than a profile root, and if you want to change that, please take a closer look at src/vfs/ftpfs/ftpfs.c and those src/filemanager/*.c


Don't throw straw men at us. ftpfs.c proves the correctness of my patch. ftpfs.c should read from ~/.netrc. If you want .netrc to be read from some other place in addition to its standard place, open a separate ticket. That's off-topic here. As for filemanager/*.c: all these places expect home, not the profile root.

Those 24 grepped lines only strengthen the cause of my patch.

If you want to prove me wrong, I'd welcome it. That's the purpose of this public review of code. Start your research here, in all the places that call mc_config_get_home_dir().

Don't mean to insult you, honestly, but you should have another look. Using relative file paths when we in fact know full paths isn't healthy.


Here's another piece of news for you:

If you'd have only bothered to raise this concern of yours like a normal human being (meaning: without attacking/invalidating others), I'd have gladly shared my thoughts on this topic with you. Because, you see, while doing my homework I did not fail to notice the use of that relative pathname. But, knowing you, you'd only use whatever I'm going to write on the matter to "reinforce" you POV and further sabotage this ticket.

Last edited 8 years ago by mooffie (previous) (diff)

comment:18 follow-up: ↓ 19 Changed 8 years ago by zaytsev

Ah, the enjoyable flair of mc development discussions, I've been missing it so much ;-) (or have I?)

@alllexx88, please consider yourself just shown a yellow card.

comment:19 in reply to: ↑ 18 ; follow-up: ↓ 20 Changed 8 years ago by alllexx88

Replying to zaytsev:

Ah, the enjoyable flair of mc development discussions, I've been missing it so much ;-) (or have I?)

@alllexx88, please consider yourself just shown a yellow card.

@zaytsev Acknowledged. The text below is only aimed at resolving the misunderstanding :)

Replying to mooffie:

If you'd have only bothered to raise this concern of yours like a normal human being (meaning: without attacking/invalidating others), I'd have gladly shared my thoughts on this topic with you. Because, you see, while doing my homework I did not fail to not notice the use of that relative pathname. But, knowing you, you'd only use whatever I'm going to write on the matter to "reinforce" you POV and further sabotage this ticket.

Before this gets more personal, let's make it clear: being an embedded open-source project maintainer, I encountered the same bug as originally reported. I fixed it the way I figured it out right. Then, I though that, hey, it was a good idea to go and share this problem/solution upstream, and this got me here. At first short glance your solution didn't look the obvious match for the problem, so I did share the patch. After being replied, I made an effort to investigate a bit, and found some points that weren't mentioned in the comments, and didn't seem obvious, so I noted them down with some comments. Did I say a word about you botching these points? Or you not doing you homework? "please take a closer look at" on my side sounded that way, and I see it now, in retrospection. That's my fault, I admit. This phrase was aimed at mc developers in general rather than you personally, but the way it sounded was offensive, and I ask your forgiveness for this misunderstanding.

Don't throw straw men at us. ftpfs.c happen to prove that my patch is correct. ftpfs.c should read from ~/.netrc. If you want .netrc to be read from some other place in addition to its ​standard place, open a separate ticket. That's off-topic here. As for filemanager/*.c: all these places expect home, not the profile root.

When you want to push a change to the repo, you should try to inform on as many as you can changes in behaviour that stem from this change. Maybe a guy somewhere in the world relayed on loading '.netrc' from alternative location -- and then, after an upgrade it suddenly stops working. That's a breaking change, and it's no good to make such changes silently.

Those 24 grepped lines only strengthen the cause of my patch.
If you want to prove me wrong, I'd welcome it. That's the purpose of this public review of code.

You're actually wrong here. The purpose is doing best for the community, choosing the optimal solution -- not proving you're right/wrong or better/worse. And likewise, those 24 grepped lines did serve this purpose, having some previously silenced points elaborated one way or another.

As for the personal things you said, I saw it coming, though maybe not so much of it :D It's a bit childish to say "you started it", but please don't say other people's patches are useless ("I don't see this [adding to MC's code health] in @alllexx88's patch"), unless you do expect harsh reaction. I did justify its usefulness, and tried to make no unbased statements.

Sincerely, there's no profit for me in "sabotaging", "insulting" or "proving you wrong". I have my "turf" to rule lol, and no need for such cheap "glory". Let's toss aside this foolish conflict and concentrate on making the transition painless and documented.

With the best regards

comment:20 in reply to: ↑ 19 ; follow-up: ↓ 21 Changed 8 years ago by mooffie

Replying to alllexx88:

Before this gets more personal [...]


Alex: Yes, let's forget the past. I see that I had a wrong impression of you. We really ought to be friends.

It's a bit childish to say "you started it", but please don't say other people's patches are useless ("I don't see this [adding to MC's code health] in @alllexx88's patch")


I apologize for my words in comment:13. My original wording was different, was softer. I'm always very careful not to seem insulting, as this is extremely counter productive. However, it just happened that by chance several factors colluded that caused me to deviate form my habit. (1st factor: softer wording inadvertently giving maintainers impression I deemed your solution on par alternative thus delaying resolution by couple years. 2nd factor: lacked time to compose much longer & duly needed reply giving overview to one who "haven't thoroughly read comments, just scanned".)

I took a chance that you won't get offended (it wasn't at all my intention). That was bad judgment on my part.

When you want to push a change to the repo, you should try to inform on as many as you can changes in behaviour that stem from this change. Maybe a guy somewhere in the world relayed on [...]


Right. I basically agree. That's what I usually do. But this wasn't needed in this ticket: as I wrote at the end of comment:6, MC_HOME is broken. One couldn't have used it without encountering some problem sooner or later sooner still. I can't imagine someone relying on it.

I made an effort to investigate a bit, and found some points that weren't mentioned in the comments, and didn't seem obvious, so I noted them down with some comments.


I appreciate the intention. That's something I'd welcome. I was dead serious when I prodded you in comment:17, giving link to the source browser, to find problems in my solution. I'm sure @Andrew will voice some objections too!

But c'mon, let's be frank: comment:14 was nothing more than vague accusations that I was breaking gazillions of things ("make sure you're happy with the change. [Because I am washing my hands off your craziness. Folks, don't say I haven't warned you!]"). You haven't given there a concrete example of a problem. Lacking such examples, you left me no way to counter your accusations. It's like hitting below the belt. comment:16 did have merit, though (the shell subsystem's dependency on startup in $HOME isn't a very pretty coding style, but we oughtn't tackle this here).

But let's forget all that, we're friends now. Don't hesitate to criticize my patches.

comment:21 in reply to: ↑ 20 ; follow-up: ↓ 23 Changed 8 years ago by alllexx88

Replying to mooffie:

Alex: Yes, let's forget the past. I see that I had a wrong impression of you. We really ought to be friends.

Sorry for the delay, a lot of things happened recently. We're all sensitive about work we do voluntarily and for free, that's probably the greatest driving force behind open-source, so such things happen. I see where my wording has been far from considerate ("stupid" is the better word), and it's natural it rose your anger. I had some background reasons for not being as careful about what I said as I usually try to, but that's off-topic here. Good to be friends, and that's all that matters.

I think I do have an impression of what your patch does, and I think from user's point of view it's good as it is, as long as it's documented thoroughly, and I have an impression of you being good at that. However, some things look a bit misleading from code readability point of view:
1) mc_config_get_home_dir() function name implies that it's return actually depends on on mc config, which is not true (and I agree, in master it is mostly used in the code as user home, except for configs) and causes confusion. I does make some sense to have a function for that, instead of just using getenv(HOME) just in case the env value gets messed up, but maybe it's sensible to change the name to something like mc_config_get_user_home_dir() -- or whichever you see fit? What do you think?
2) (this is there before your patch actually) Some comments regarding config paths are misleading, like in src/subshell/common.c:

    case SHELL_BASH:
        /* Do we have a custom init file ~/.local/share/mc/bashrc? */
        init_file = mc_config_get_full_path ("bashrc");

This leaves you thinking that mc onfig root is always ~/.local/share/mc. Since your patch targets to fix the mc config paths discrepancy, it would be good to sort this confusion out too.

Except for these points, I see no reasons for this change to not be accepted.

As for my patch, please ignore it here: it's part of a greater change I hope to finish soon.

Last edited 8 years ago by alllexx88 (previous) (diff)

comment:22 Changed 8 years ago by zaytsev

I'm very happy that you guys have sorted it out. I was really afraid this is going to escalate, just as it (unfortunately) happened under similar circumstances many times in the past. We need more contributors like you so that we can all work in peace on mc in the little time we have and enjoy the experience regardless of the scope of the participation. Thumbs up!

comment:23 in reply to: ↑ 21 Changed 8 years ago by mooffie

Replying to alllexx88:

1) mc_config_get_home_dir() function name implies that it's return actually depends on on mc config [...] maybe it's sensible to change the name to [...]


Great idea. But, since the function will also have to be moved to a different file then (utils.c or such), this should be done in a separate ticket/patch or else it won't be easy to learn from the 'diff' how the function has changed.

2) Some comments regarding config paths are misleading, like in src/subshell/common.c:

    case SHELL_BASH:
        /* Do we have a custom init file ~/.local/share/mc/bashrc? */
        init_file = mc_config_get_full_path ("bashrc");

This leaves you thinking that mc onfig root is always ~/.local/share/mc.


I see strings like "~/.local/share/APP_NAME" as idioms. They mean "the user's data dir", wherever it is. After all, the user can already change this by setting the standard XDG_* environment variables. Unless somebody can come up with an alternative to the comment "~/.local/share/mc/bashrc" that's both succinct and informative, I'm not too sure it's worth it.

comment:24 Changed 7 years ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.19

Branch: 3684_mc_profile_root
Initial changeset:239a8d01179cd3c05f14f88a0444b5c5757a39e0

comment:25 Changed 7 years ago by andrew_b

  • Blocked By 3682 removed

comment:26 Changed 7 years ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:27 Changed 7 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [61c681de94b6bdb97fc91937b86ec4d690d286db].

git log --pretty=oneline 84433f4..61c681d

comment:28 Changed 7 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.