[v3,0/5] rev-list: allow missing tips with --missing

Message

Christian Couder Feb. 14, 2024, 2:25 p.m. UTC

# Intro

A recent patch series, kn/rev-list-missing-fix [1] extended the
`--missing` option in git-rev-list(1) to support commit objects.

Unfortunately, git-rev-list(1) with `--missing` set to something other
than 'error' still fails, usually with a "fatal: bad object <oid>"
error message, when a missing object is passed as an argument.

This patch series removes this limitation and when using
`--missing=print` allows all missing objects to be printed including
those that are passed as arguments.

[1] https://lore.kernel.org/git/[e-mail]/

# Patch overview

Patch 1/5 (t9210: do not rely on lazy fetching to fail) is a test fix
suggested by Junio, so that a mostly unrelated test will not wrongly
fail when this series is merged.

Patches 2/5 (revision: clarify a 'return NULL' in get_reference()),
3/5 (oidset: refactor oidset_insert_from_set()) and
4/5 (t6022: fix 'test' style and 'even though' typo) are very small
preparatory cleanups.

Patch 5/5 (rev-list: allow missing tips with --missing=[print|allow*])
allows git-rev-list(1) with `--missing=<arg>` when <arg> is not
'error' to not fail if some tips it is passed are missing.

# Changes since V2

Thanks to Linus Arver, and Junio who commented on V2!

The changes since V2 are the following:

  - Patch 1/5 (t9210: do not rely on lazy fetching to fail) was added
    to fix a test that wrongly failed when this series was applied,
    thanks to Junio who authored it.

  - In patch 5/5 (rev-list: allow missing tips with
    --missing=[print|allow*]), some grammos and typos were fixed in the
    commit message, thanks to Junio and Linus.

  - In patch 5/5 (rev-list: allow missing tips with
    --missing=[print|allow*]), the NEEDSWORK comment was improved, thanks
    to Junio. In particular, it doesn't use "ugly" and "dumb" anymore
    and gives an example of what's broken.

  - In patch 5/5 (rev-list: allow missing tips with
    --missing=[print|allow*]), a code comment as been clarified by removing
    "already" from it.

# Range-diff since V2

-:  ---------- > 1:  6e6f2cc26b t9210: do not rely on lazy fetching to fail
1:  5233a83181 = 2:  733c78144e revision: clarify a 'return NULL' in get_reference()
2:  cfa72c8cf1 = 3:  4c9e032456 oidset: refactor oidset_insert_from_set()
3:  5668340516 = 4:  35ca6e7c3d t6022: fix 'test' style and 'even though' typo
4:  55792110ca ! 5:  da36843b44 rev-list: allow missing tips with --missing=[print|allow*]
    @@ Commit message
         We could introduce a new option to make it work like this, but most
         users are likely to prefer the command to have this behavior as the
         default one. Introducing a new option would require another dumb loop
    -    to look for that options early, which isn't nice.
    +    to look for that option early, which isn't nice.
     
         Also we made `git rev-list` work with missing commits very recently
         and the command is most often passed commits as arguments. So let's
    -    consider this as a bug fix related to these previous change.
    +    consider this as a bug fix related to these recent changes.
     
         While at it let's add a NEEDSWORK comment to say that we should get
         rid of the existing ugly dumb loops that parse the
    @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr
         * Let "--missing" to conditionally set fetch_if_missing.
         */
     +  /*
    -+   * NEEDSWORK: These dump loops to look for some options early
    -+   * are ugly. We really need setup_revisions() to have a
    -+   * mechanism to allow and disallow some sets of options for
    -+   * different commands (like rev-list, replay, etc). Such
    -+   * mechanism should do an early parsing of option and be able
    -+   * to manage the `--exclude-promisor-objects` and `--missing=...`
    -+   * options below.
    ++   * NEEDSWORK: These loops that attempt to find presence of
    ++   * options without understanding that the options they are
    ++   * skipping are broken (e.g., it would not know "--grep
    ++   * --exclude-promisor-objects" is not triggering
    ++   * "--exclude-promisor-objects" option).  We really need
    ++   * setup_revisions() to have a mechanism to allow and disallow
    ++   * some sets of options for different commands (like rev-list,
    ++   * replay, etc). Such a mechanism should do an early parsing
    ++   * of options and be able to manage the `--missing=...` and
    ++   * `--exclude-promisor-objects` options below.
     +   */
        for (i = 1; i < argc; i++) {
                const char *arg = argv[i];
    @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr
     -  if (arg_missing_action == MA_PRINT)
     +  if (arg_missing_action == MA_PRINT) {
                oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
    -+          /* Already add missing tips */
    ++          /* Add missing tips */
     +          oidset_insert_from_set(&missing_objects, &revs.missing_commits);
     +          oidset_clear(&revs.missing_commits);
     +  }


Christian Couder (4):
  revision: clarify a 'return NULL' in get_reference()
  oidset: refactor oidset_insert_from_set()
  t6022: fix 'test' style and 'even though' typo
  rev-list: allow missing tips with --missing=[print|allow*]

Junio C Hamano (1):
  t9210: do not rely on lazy fetching to fail

 Documentation/rev-list-options.txt |  4 ++
 builtin/rev-list.c                 | 18 ++++++++-
 list-objects-filter.c              | 11 +-----
 oidset.c                           | 10 +++++
 oidset.h                           |  6 +++
 revision.c                         | 16 ++++++--
 t/t6022-rev-list-missing.sh        | 61 +++++++++++++++++++++++++++++-
 t/t9210-scalar.sh                  |  9 ++++-
 8 files changed, 117 insertions(+), 18 deletions(-)

Comments

Linus Arver Feb. 16, 2024, 9:56 p.m. UTC | #1

This v3 LGTM.

Reviewed-by: Linus Arver <[e-mail]>

Thanks!