search

emacscollective / borg

Assimilate Emacs packages as Git submodules
https://emacsmirror.net/manual/borg
GNU General Public License v3.0
259 stars 28 forks source link

Activate clone only after borg--build-interactive is finished #139

Closed OkamiW closed 1 year ago

OkamiW commented 1 year ago

Before, borg-assimilate would run borg-activate immediately after borg--build-interactive, though borg--build-interactive probably isn't finished yet. (Thereby borg-activate won't activate the autoloads)

This commit fixes that.

tarsius commented 1 year ago

Thanks! I'll do the actual review later.

This commit fixes that.

It also contains some whitespace cleanup and an unwelcome (*) whitespace change. I am dealing with this in a separate commit and rebased your pull-request on top of that.

(* In my configuration lambda is displayed as λ, in yours it is not.)

You are also switching from start-process to make-process. That isn't necessary to achieve the goal of this pr, is it? Please do that in a separate second commit.

OkamiW commented 1 year ago

Done :)

tarsius commented 1 year ago

Done :)

Unfortunately that did not have the desired effect of making the diff of the relevant change easier to understand. For that it is necessary that the switch to make-process before the main change. I have done that now and pushed the result to the tmp branch. The goal when asking you to split up things was to only have to look at something like the tip of that branch: 0d53c08ca7a90df47a5fe91455dfe7203e921123.

Once I could look at that change I realized that the cleanup of borg--build-interactive isn't actually necessary, but I decided to keep it anyway. Ultimately I changed the bug fix into just this: a33760328424af1a2137e596a0076f08cba4d639. (That works because subprocesses aren't actually started until the command that starts is done and we return to the command loop.) That's currently on the work branch, but I intend to push it to master shortly.

tarsius commented 1 year ago

I've merged the split up version.