Re: [GSoC update] Sequencer for inclusion

[Jeff King <peff@peff.net>]

On Wed, Jul 13, 2011 at 03:11:54PM +0530, Ramkumar Ramachandra wrote:

> Ha, true.  After all .git/objects/ is "polluted" with lots of files :p
> A couple of questions arise:
> 1. Would opening, writing, closing many files not put too much strain
> on I/O, and hence affect performance significantly?

In a tight loop, perhaps. At the begining of invoking a program,
probably not. We're talking about what, a dozen or so files?

If you have doubts, then measure.

> 2. Will two options from different instructions (when we extend the
> instruction sheet to accommodate them) have the same name, but
> different effects?  Having a gitconfig-like configuration file doesn't
> solve this problem either, so it's not a "configfile versus key-value
> store" question.

If you have per-instruction options, then I think you would want your
storage of options to be per-instruction. Whether it's a config file or
files in a directory, you'd want to be accessing ".../opts/$hash" as a
config file (or ".../opts/$hash/*" for a directory).

But I have only been paying a little attention to the deeper parts of
this topic, so I may be misunderstanding what you're asking.

> > Using git-config is maybe a little more self-documenting than something
> > like "sq_quote_argv". And probably not much more code (maybe even less,
> > since it can handle the file update for you).
> 
> Yeah :)

One other advantage of "git config" over sq_quote_argv is that it is
fairly transparent to outside programs. You can just say "git config
--file=.git/sequencer/opts --get-all strategy" or whatever.

That transparency is one of the things I liked about the "files in a
directory" approach taken by rebase currently. But given that the "git
config" command exists, they are probably equivalent in that respect.

> > I recently used the config code to write out a non-standard config file.
> > My two complaints were:
> >
> >  1. You can't queue up a bunch of changes and then write the file once.
> >     Every time you call git_config_set, it rewrites the whole file.
> >
> >  2. There's no way to write to a nonstandard file short of the horribly
> >     hack-ish:
> >
> >       const char *saved = config_exclusive_filename;
> >       config_exclusive_filename = "foo.conf";
> >       git_config_set(...);
> >       config_exclusive_filename = saved;
> >
> > Point (2) is pretty easy to fix. But point (1) might be a bit more
> > involved. I haven't really looked yet.
> 
> Thanks for pointing these out.  Yes, I'm aware of these issues, and
> it's part of the reason I implemented my own parser.  It'll take some
> time to refactor config.c so that it's usable by others in a sane
> manner, no?  What do you suggest I do until then?  Can I try to get my
> custom parser merged (and replaced by a more generic configparser when
> it's ready), or should I throw away my parser and go with the
> key-value store?

I think a reasonable strategy is:

  1. Refactor git_config_set into git_config_set_in_file with a file
     argument (and make git_config_set a wrapper that uses the current
     file-selection). This should be easy to do.

  2. Ignore the fact that you are writing the file multiple times. It's
     not a correctness issue, but rather an optimization. If and when
     the config code shows itself to be too slow, then we can do that
     optimization (which will benefit not just your code, but all config
     writers).

And then throw away your parser and just use git_config_from_file and
git_config_set_in_file.

Between config and a key/value store of files, I think it is largely a
matter of taste.

> Just to clarify, here's an example to illustrate what I understand
> when you say key-value store (please correct me if I got the idea
> wrong):
> .git/sequencer/opts/ will have files like:
> $ cat ff
> true
> $ cat record-origin
> true
> $ cat mainline
> 1
> $ cat strategy
> ours
> $ cat strategy-option
> patience + renormalize

Yeah, exactly. Though I would probably use "\n" to split list items in
strategy-options, which is a little more traditional. I.e.,:

  $ cat strategy-option
  patience
  renormalize

-Peff