Re: [PATCH 2/3] builtin/commit-graph.c: introduce '--input=<source>'
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Jan 31, 2020, 19:34 UTC
- Message-ID
- <CAN0heSqGe0HpAj+qoYkif4pyjaW5EBNsRce9OmxMaZXTSW7C9w@mail.gmail.com>
- In-Reply-To
- <8effe35bcd1dadee3a29d996f269353cf6e4982d.1580430057.git.me@ttaylorr.com>
On Fri, 31 Jan 2020 at 01:30, Taylor Blau <me@ttaylorr.com> wrote:
Show 14 quoted lines
> The 'write' mode of the 'commit-graph' supports input from a number of > different sources: pack indexes over stdin, commits over stdin, commits > reachable from all references, and so on. Each of these options are > specified with a unique option: '--stdin-packs', '--stdin-commits', etc. > > Similar to our replacement of 'git config [--<type>]' with 'git config > [--type=<type>]' (c.f., fb0dc3bac1 (builtin/config.c: support > `--type=<type>` as preferred alias for `--<type>`, 2018-04-18)), softly > deprecate '[--<input>]' in favor of '[--input=<source>]'. > > This makes it more clear to implement new options that are combinations > of other options (such as, for example, "none", a combination of the old > "--append" and a new sentinel to specify to _not_ look in other packs, > which we will implement in a future patch).
Makes sense.
> Unfortunately, the new enumerated type is a bitfield, even though it
> makes much more sense as '0, 1, 2, ...'. Even though *almost* all
> options are pairwise exclusive, '--stdin-{packs,commits}' *is*
> compatible with '--append'. For this reason, use a bitfield.> -With the `--append` option, include all commits that are present in the > -existing commit-graph file. > +With the `--input=append` option, include all commits that are present > +in the existing commit-graph file.
Would it be too crazy to call this `--input=existing` instead, and have it be the same as `--append`? I find that `--append` makes a lot of sense (it's a mode we can turn on or off), whereas "input = append" seems more odd.
From the next commit message, we learn that a long `--input=append` triggers `fill_oids_from_all_packs()`, which wouldn't match my expecting from "--input=existing". So...
Does this hint that we could leave `--append` alone? We'd have lots of different inputs to choose from using `--input`, and an `--append` mode on top of that. That would make your inputs truly mutually exclusive and you don't need the bitfield anymore, as you mention above. Hmm?
Would that mean that the falling back to `fill_oids_from_all_packs()` would follow from "is there an --input?", as opposed to from "is there an --input except --input=append?"?
(I don't know whether these inputs really *have* to be exclusive, or if that's more of an implementation detail. That is, even without an "append" input, might we one day be able to handle more inputs at once? Maybe this is not the time to worry about that.)
Martin