Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Agnostic #115

Closed
wants to merge 9 commits into from
Closed

Agnostic #115

wants to merge 9 commits into from

Conversation

andyleejordan
Copy link
Contributor

As per PR #114, this adds the vcsh setup and vcsh where commands, with necessary documentation. It also enables the sourcing of ./.config/vcsh/config, which vcsh setup will automatically create with the $VCSH_BASE and $XDG_CONFIG_HOME variables set to $(pwd) and $(pwd)/.config respectively, thus enable "agnostic" use of vcsh from the folders with the a ./.config/vcsh/config file.

It's a work in progress, and probably needs more testing, but I think it's a good way to go about doing this.

@andyleejordan
Copy link
Contributor Author

One major caveat is that I'd rather not recursively search folders for .config. If we can figure out a clean way to do so, I'd be all for it, but currently it expects to be used from the root of the tracked folder.

@deweysasser
Copy link

I don't think recursing up the directory tree is much of a problem -- even an incredibly deep directory structure will be less than dozens of directories.

I'm going to try this out.

@andyleejordan
Copy link
Contributor Author

Cool. Also, I should note that I don't have ronn, and so didn't regen the updated man page.

@deweysasser deweysasser mentioned this pull request Mar 7, 2014
@deweysasser
Copy link

On 03/06/2014 11:20 PM, Andrew Schwartzmeyer wrote:

Cool. Also, I should note that I don't have ronn, and so didn't regen
the updated man page.

I tried out the changes and they clearly work in the normal scenarios.

Also, I modified the code to look up the directory tree for the config file.

I then set up a 30 level deep directory structure and ran vcsh with and
without directory ascension 1000 times running "list", first without any
repos and then with 2 repos.

Climbing the tree appears to add roughly 0.01 seconds per invocation on
average with 30 directories between pwd and the vcsh config directory.
With only 10 directories the difference is barely measurable

Test Case
Total Time
Average Time
No repos, no ascension, depth 30
0.3
0.0003
No repos, ascension, depth 30
11.69
0.0117
2 repos, no ascension, depth 30
9.88
0.0099
2 repos, ascension, depth 30
20.23
0.0202
2 repos, no ascension, depth 10
9.16
0.0091
2 repos, ascension, depth 10
10.18
0.0102
2 repos, no ascension, depth 3
10.14
0.0101
2 repos, ascension, depth 3
9.93
0.0099

I have a ronn-capable system around so I regenerated the man page.

I will create another pull request for these changes. Note that they
include Andrew's changes.

Dewey

This was referenced Mar 7, 2014
Merging in recursive search for configuration file.
Using this function to recurse up the directory tree looking for the
configuration file. Renamed to test_dir_upwards() because the function
is just testing with [ $1 $2 ]. Switched to $(execute) from `execute` as
the latter is preferred. Tested, this works well.
@andyleejordan
Copy link
Contributor Author

There's just one issue left with recursion: since we stop at /, if the user is somewhere ahead of $XDG_CONFIG_HOME and has no "agnostic" vcsh setups, then $XDG_CONFIG_HOME/vcsh/config is going to be parsed twice.

@andyleejordan
Copy link
Contributor Author

@dmsasser Nice work by the way :)

@deweysasser
Copy link

Thanks

On March 13, 2014 2:04:50 AM EDT, Andrew Schwartzmeyer [email protected] wrote:

@dmsasser Nice work by the way :)


Reply to this email directly or view it on GitHub:
#115 (comment)

Dewey

@deweysasser
Copy link

What can I do to help make this ready for merge?

@andyleejordan
Copy link
Contributor Author

Likewise! I think these are good improvements. They've been roughly tested too. Should we even worry about parsing $XDG_CONFIG_HOME/vcsh/config twice? It'll happen, but isn't the worst thing in the world.

@RichiH
Copy link
Owner

RichiH commented Mar 23, 2014

Hi,

at the moment, you can't do much as it hinges on me :(

I am horribly behind on my FLOSS fronts due to real life and thus didn't have time to really review, yet ... I had hoped to do this over the weekend, but... Anyway, I will try to review in the coming days.

Sorry & thanks,
Richard

@andyleejordan
Copy link
Contributor Author

@RichiH Any chance you'll have us on the docket soon?

@andyleejordan
Copy link
Contributor Author

Bump :)

@RichiH
Copy link
Owner

RichiH commented Oct 22, 2014

@andschwa @dmsasser

Oi.

Can people try and break this code, please? I am still edgy on whether to include this, but with Debian's freeze upcoming, there should be a decision either way.

The 25th is the last day when I can merge stuff and be certain things will make it into jessie.

@RichiH
Copy link
Owner

RichiH commented Oct 22, 2014

PS: @andschwa If you could rebase on current master, that would be appreciated. If not, that's fine, too.

@RichiH
Copy link
Owner

RichiH commented Oct 25, 2014

Notes to self:

  • is where a good name? Maybe use base instead? Rationale being that base is already used internally in $VCSH_BASE.
  • is setup a good name? Maybe use base_new or newbase instead? Rationale being that setup() used to exist internally and that it should be tied to the other command.
  • $XDG_CONFIG_HOME is officially defined, need to switch everything to $VCSH_CONFIG_HOME, I fear.
  • Need to factor out static manpage pushes from commits
  • mkdir and potentially others don't have error handling
  • Go through manpage one last time with blank brain to see if new text explains everything that's needed

Else, it looks good for inclusion and we still have 24 hours :p

I put everything into https://github.com/RichiH/vcsh/tree/feature/agnostic to have a well-defined place to work on. Anyone submitting patches, please do it there.

PS: I may force-push that branch to get rid of vcsh.1

@RichiH
Copy link
Owner

RichiH commented Oct 25, 2014

Wouldn't it be better to use .vcsh_base instead of ./.config/vcsh/config?

RichiH added a commit that referenced this pull request Oct 25, 2014
As per #115 (comment) :

Is where a good name? Maybe use base instead? Rationale being that base
is already used internally in $VCSH_BASE.
@RichiH
Copy link
Owner

RichiH commented Oct 25, 2014

I think the correct answer might involve repo-specific configurations; which are now in master.

@RichiH
Copy link
Owner

RichiH commented Oct 26, 2014

I think the central question is whether those repos should be central, i.e. in $HOME, or decentral, i.e. stored in the other bases.

Central repos allow for one common namespace (thus avoiding confusion). Decentral ones allow you to, e.g., put things onto a USB thumb drive.

A third option is to default to central, but to allow decentral ones as an option. That would need .vcsh_trust similar to .mrtrust, though.

@andyleejordan
Copy link
Contributor Author

Aw man! Deadline passed yesterday, and I'm just getting back to you, my bad :/

It's been a while since I looked at this code, but IIRC correctly, what I didn't like was that with this feature, we started treating ~ as a special case, whereas really, if this is done correctly, any vcsh conf in ~ would be read by the recursive search (and if you're in ~, it would just look once).

I think we should take the ideas and work here and refactor the vcsh core to use it, rather than thinking of folder agnosticism as a separate feature from version control system for ~. Heck, it'd become vcsa (version control system for anywhere).

I would definitely advocate for decentralized repos across the board. Perhaps rather than storing all our stuff in ~/.config/vcsh/repos.d (or really ./.config/vcsh/*), we should stuff it all in ./.vcsh.d, with a config file and repositories in there, or something similar.

@andyleejordan
Copy link
Contributor Author

@RichiH What are your thoughts on decentralizing the whole thing? In the end I think it would come out much neater.

@bertenvdb
Copy link

Anyone still working on this? I would love to be able to track /etc files!

@andyleejordan
Copy link
Contributor Author

@bertenvdb IIRC it's working, just not merged, give it a try and let me know!

@bertenvdb
Copy link

Edit: It seems my poor understanding of git(hub) was the problem . Now I'm using version 1.20141025 and so far everything works as expected.

Great work, vcsh is a truly remarkable tool!

@andyleejordan
Copy link
Contributor Author

@bertenvdb were you able to apply this patch and use the agnostic features? Just curious.

@andyleejordan
Copy link
Contributor Author

If ya'll want to take this ever, feel free, but I am clearly never getting back to it 🤣

@alerque
Copy link
Collaborator

alerque commented Apr 2, 2021

I'm re-opening this PR as #283 because I think the work deserves further review. At least until we decide on some resolution to #110 (whether that is rejecting the feature or implementing something) this seems like at least one reasonable approach. @andschwa no worries at all if you aren't interested in being involved in that process, feel free to unsubscribe from notifications to this PR and anything else that generates noise in your notifications.

@andyleejordan
Copy link
Contributor Author

It'd be awesome if it ever gets merged, good luck @alerque!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants