Re: [PATCH 2/3] builtin/commit-graph.c: introduce '--input=<source>'

["Martin Ågren" <martin.agren@gmail.com>]

On Fri, 31 Jan 2020 at 01:30, Taylor Blau <me@ttaylorr.com> wrote:
> 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