pijul_org / pijul

#239 ".ignore" for the pijul ignore file

Opened by cheng, on December 11, 2017
Closed
cheng commented on December 11, 2017

Hi, I would like to try this VCS out, but a first thing I notice is that it uses ".ignore" files for excluding files out from the repository. I only see a discussion here (https://nest.pijul.com/pijul_org/pijul/discussions/23) mentioning the name ".pijulignore", which I think is better. Is ".ignore" decided or only a temporary name?

I am concerned that this filename is too generic. Similar concern was raised when this idea was initially discussed on Hacker News (https://news.ycombinator.com/item?id=12568245).

lthms commented on December 12, 2017

We use the ignore to deal with this matter. By default, it looks at .gitignore and .ignore file and this is why we choose .ignore. It is more convenient. I would personally enjoy something like darcs has, where you can choose the name of your ignore files (with a better default); it might be possible, but not without forking ignore I fear.

spacefrogg commented on December 12, 2017

I would like to second that. I also find the name too generic.

EDIT: Is the ignore crate following the .gitignore syntax? If so, the pijul .ignore file would be semantically equivalent to .gitignore, would it not? For two reasons I would change the name to .gitignore, then. First, as long as it is semantically equivalent an extra name would be superficial and misleading. Second, when, in the future, its semantics change, it might need renaming anyhow and could be easily renamed to .pijulignore avoiding the generic term .ignore.

cheng commented on December 18, 2017

I have just come up with an argument against the generic ".ignore" files.

It is introduced to specify files to be ignored by any generic applications. However, we actually already have a mechanism for this, and it is called hidden files. ".gitignore" and others are introduced because a single generic ignoring mechanism isn't enough for the various ways a file may be used. Introducing another generic ignoring mechanism won't solve the problem.

pmeunier commented on December 18, 2017

Wait, by looking at the source code, it seems that .pijul/local/ignore is another possible path for that file. I'm adding @lthms's suggestion now.

lthms commented on December 18, 2017

@pmeunier beware .ignore files and .pijul/local/ignore are similar but have a major difference. The latter is local (not versionned), the former is shared (hence versionned)

lthms commented on December 18, 2017

I just had a look at your file, I am not sure this is what we really should have.

The .ignore files are versionned, basically like the _boring file is versionned in darcs. Darcs allows you to change the file name, and this is include as a change in a patch, so that everyone can share the same name (for instance, I personally like to rename it .boring instead of _boring. .pijul/local/ignore serves another purpose: it is local and can be used by someone not to bother with other but being able to ignore things; for instance, if my editor generates certain files, I can use .pijul/local/ignore because this is strictly personal and I am the only one to use this editor feature.

I think your patch keeps the dichotomy .ignore and .pijul/local/ignore, but I fear you cannot share the new ignore file name.

pmeunier commented on December 18, 2017

I wasn't aware of that feature in darcs, I wonder how conflicts are handled between two patches that rename the file (maybe they're just not handled). How about .pijulignore instead of .ignore, then?

lthms commented on December 18, 2017

.pijulignore sounds great!

pmeunier commented on December 18, 2017

(btw, I fixed lots of things in the Nest's patches view in the last two days).

lthms commented on December 18, 2017

I fear there is one last thing that is missing with this patch.

Currently, as far as I know, you can have several ignore files in several location, and they will apply in the directory they are and its subdirectories.

Maybe we can drop this feature, but it can be nice if, for instance, you have a tex/ directory and put the related ignored files for tex in a tex/.ignore file.

Also, the patch does not build, as it looks like it does not import what it uses.

(btw, I fixed lots of things in the Nest's patches view in the last two days).

I’ve noticed and this is great! thanks a lot for that.

lthms commented on December 18, 2017

The patch is broken, currently. Can you push it in a fork of yours and maybe we can unrecord it from pijul-0.9, at least until we know exactly what to do?

cheng commented on December 18, 2017

Happy to see the progress, thanks!

Note that if we only support '.pijulignore' in the root directory of the repo for now, it won't be a breaking change when we add support for '.pijulignore' in subdirectories. (EDIT: unless there are '.pijulignore' in subdirectories before upgrading)

cheng commented on December 18, 2017

I looked into the documentation of 'ignore' crate. If I understand it correctly, the .parents(true) function call ( https://docs.rs/ignore/0.3.1/ignore/struct.WalkBuilder.html#method.parents ) in the patch would take care of '.pijulignore' in subdirectories.

However, it may be worth noting that '.pijulignore' in any parent paths are considered, even those '.pijulignore' outside the repository. (I am not sure, the 'ignore' documentation is unclear)

lthms commented on December 18, 2017

@cheng this is pretty great. I don’t think this is an issue, as, I far as I know, ignore is used as a filter most of the time.

cheng commented on December 18, 2017

I looked at the 'ignore' source code, and it seems that I was right, each parent directory of a absolute path is considered. ( https://github.com/BurntSushi/ripgrep/blob/d73a75d6cd82068252c35c5718900b6a1acb296e/ignore/src/dir.rs#L153 ) It should be easy to verify by running a version of pijul though.

We should make the semantics in this case well-defined, as otherwise it would be left as an undefined behavior. It is clear to me that '.pijulignore' outside the repository (assuming a repository root directory is already identified) shouldn't be considered. Do you know how git and darcs handle this?

Assuming that we want the semantics as I suggested, I am afraid that we have to fork 'ignore' for this to be correctly handled.

cheng commented on December 18, 2017

I just noticed the comments of the same function add_parents in 'ignore' crate here: https://github.com/BurntSushi/ripgrep/blob/d73a75d6cd82068252c35c5718900b6a1acb296e/ignore/src/dir.rs#L163 . This error handling is clearly improper to me. What do you think?

cheng commented on December 18, 2017

Sorry to disappoint. I also checked the source code for the issue ".pijulignore in subdirectories". Unfortunately, .parents(true) doesn't seem to work with .add_ignore('.pijulignore'). :(

All the filenames that work with .parents(true) are listed here: https://github.com/BurntSushi/ripgrep/blob/d73a75d6cd82068252c35c5718900b6a1acb296e/ignore/src/dir.rs#L212 . It includes a '.rgignore' which cannot be configured independently to '.ignore'. It seems to me a weird design decision to have each of the cases considered separately, instead of a configurable list of paths for ignore files. The documentation is unclear so that I was forced to read source codes to learn the semantics.

pmeunier commented on December 18, 2017

Ok, I've unrecorded my patch anyway, as we don't seem to have a good solution right now, and we're close to releasing Pijul 0.9. Feel free to use this page to add any new ideas or comments about this feature.

cheng commented on December 18, 2017

I found that 'ignore' actually handles the issue of ".gitignore outside a git repository", in which the git repository is identified by a '.git' directory. But '.ignore' and '.rgignore' have a different semantics, they are applied when outside a git repository. https://github.com/BurntSushi/ripgrep/blob/d73a75d6cd82068252c35c5718900b6a1acb296e/ignore/src/dir.rs#L326

Learning based on a partial understanding of a program is too error-prone. My current understanding is still possible to be wrong, but I will stop putting time into this.

If you want to fork 'ignore' for use in pijul, replacing all 'git' with 'pijul' may be a good start.

onio commented on September 28, 2018

bump

pmeunier commented on September 29, 2018

Applied, thanks!

pmeunier commented on December 14, 2018

I like the current state of this issue, I just used it to fix pijul add . --recursive (discussion #336). I'm closing, feel free to reopen if it is a problem.