On Sun, Sep 2, 2012 at 9:50 PM, Adam Spiers <git@xxxxxxxxxxxxxx> wrote: > I'm no expert on .gitattributes and check-attr, but AFAICS, all the > opportunities to share code in the plumbing and front-end seem to be > taken already, e.g. the directory traversal and path handling. The > CLI argument parsing is necessarily different because check-attr > requires a list of attributes as well as a list of files, and of > course the output routines have to be different too. > > The only opportunity for code reuse which I saw but /didn't/ take was > around the --stdin line parsing code which is duplicated between: > > check_attr_stdin_paths > check_ignore_stdin_paths > cmd_checkout_index > cmd_update_index > hash_stdin_paths > > I attempted to refactor these, but quickly realised that due to the > lack of proper closures in C, the overheads and complexity incurred by > performing such a refactoring probably outweighed the benefits, so I > gave up on the idea. > > Having said that, I'm totally open to suggestions if you can spot > other places where code could be reused :) Yeah. That was my impression too. I just hoped a new set of eyes might discover something ;) At lease we could prepare the output format that can be reused (maybe with little changes) for check-attr debugging if it comes later. Or make this command part of check-attr.. >> Also --quiet option, where check-ignore returns 0 if the given path is >> ignored, 1 otherwise? > > I considered that, but couldn't think of appropriate behaviour when > multiple paths are given, so in the end I decided to remain consistent > with check-attr, which always returns 0. But I'm happy to change it > if you can think of a more useful behaviour. For example we could > have a --count option which produces no output but has an exit status > corresponding to the number of ignored files. We could take this opportunity to kill "add --ignore-missing", which is basically .gitignore checking and it accepts multiple paths, I think. >> - If many paths are given, then perhaps we could print ignored paths >> (no extra info). > > How is this different to git ls-files -i -o ? I think ls-files requires real files on working directory, but check-ignore can deal with just non-existing paths. >> - For debugging, given one path, we print all the rules that are >> applied to it, which may help understand how/why it goes wrong. > > That would be nice, but I'm not sure it's a tremendously common use > case. Could you think of a scenario in which it would be useful? I > guess it could be done by adding a new DIR_DEBUG_IGNORED flag to > dir_struct which would make the exclude matcher functions collect all > matching patterns, rather than just returning the first one. This in > turn would require another field for collecting all matched patterns. Mixing include/exclude ignore rules multiple times could be hard to figure out what goes wrong. But as we haven't seen an actual use case yet, just leave it out. >> I don't think we really need NEED_WORK_TREE here. .gitignore can be >> read from index only. > > I thought about that, but in the end I decided it probably didn't make > sense, because none of the exclude matching routines match against the > index - they all match against the working tree and core.excludesfile. > This would also require changing the matching logic to honor the index, > but I didn't see the benefit in doing that, since all operations which > involve excludes (add, status, etc.) relate to a work tree. > > But as with all of the above, please don't hesitate to point out if > I've missed something. You guys are the experts, not me ;-) Again I was thinking that check-ignore could work with imaginary paths and could be used by scripts. If a script just wants to check certain paths are excluded, it should not need to move to working directory (though it probably is in working directory most of the time). -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html