| Message ID | 20240208135055.2705260-1-christian.couder@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rev-list: allow missing tips with --missing | expand |
Christian Couder <christian.couder@gmail.com> writes: > # Patch overview > > Patches 1/4 (revision: clarify a 'return NULL' in get_reference()), > 2/4 (oidset: refactor oidset_insert_from_set()) and > 3/4 (t6022: fix 'test' style and 'even though' typo) are very small > preparatory cleanups. > > Patch 4/4 (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. Thanks. There is an existing test that makes a bad assumption and fails with these patches. We may need something like this patch as a preliminary fix before these four patches. ----- >8 ---------- >8 ---------- >8 ---------- >8 ----- Subject: [PATCH] t9210: do not rely on lazy fetching to fail With "rev-list --missing=print $start", where "$start" is a 40-hex object name, the object may or may not be lazily fetched from the promisor. Make sure it fails by forcing dereference of "$start" at that point. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t9210-scalar.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh index 4432a30d10..428339e342 100755 --- a/t/t9210-scalar.sh +++ b/t/t9210-scalar.sh @@ -154,7 +154,14 @@ test_expect_success 'scalar clone' ' test_cmp expect actual && test_path_is_missing 1/2 && - test_must_fail git rev-list --missing=print $second && + + # This relies on the fact that the presence of "--missing" + # on the command line forces lazy fetching off before + # "$second^{blob}" gets parsed. Without "^{blob}", a + # bare object name "$second" is taken into the queue and + # the command may not fail with a fixed "rev-list --missing". + test_must_fail git rev-list --missing=print "$second^{blob}" -- && + git rev-list $second && git cat-file blob $second >actual && echo "second" >expect &&
On Fri, Feb 9, 2024 at 12:15 AM Junio C Hamano <gitster@pobox.com> wrote: > > Christian Couder <christian.couder@gmail.com> writes: > > > # Patch overview > > > > Patches 1/4 (revision: clarify a 'return NULL' in get_reference()), > > 2/4 (oidset: refactor oidset_insert_from_set()) and > > 3/4 (t6022: fix 'test' style and 'even though' typo) are very small > > preparatory cleanups. > > > > Patch 4/4 (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. > > Thanks. There is an existing test that makes a bad assumption and > fails with these patches. We may need something like this patch as > a preliminary fix before these four patches. Thanks, I have added your patch to the V3 I just sent.
# 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/20231026101109.43110-1-karthik.188@gmail.com/ # Patch overview Patches 1/4 (revision: clarify a 'return NULL' in get_reference()), 2/4 (oidset: refactor oidset_insert_from_set()) and 3/4 (t6022: fix 'test' style and 'even though' typo) are very small preparatory cleanups. Patch 4/4 (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 V1 Thanks to Linus Arver, Eric Sunshine and Junio who commented on V1! The changes since V1 are the following: - In patch 1/4 (revision: clarify a 'return NULL' in get_reference()), some 's/explicitely/explicitly/' typos were fixed in the commit message. Thanks to a suggestion from Eric. - Patch 2/4 (oidset: refactor oidset_insert_from_set()) is new. It refactors some code into "oidset.{c,h}" to avoid duplicating code to copy elements from one oidset into another one. Thanks to a suggestion from Linus. - Patch 3/4 (t6022: fix 'test' style and 'even though' typo) used to fix only an 'even though' typo, but while at it I made it use `if test ...` instead of `if [ ... ]` too. Thanks to a suggestion from Junio. - Patch 4/4 (rev-list: allow missing tips with --missing=[print|allow*]) was changed so that missing tips are always allowed when `--missing=[print|allow*]` is used, as suggested by Junio. So: - no new `--allow-missing-tips` option is implemented, - no ugly early parsing loop is added, - no new 'do_not_die_on_missing_tips' flag is added into 'struct rev_info', - the 'do_not_die_on_missing_objects' is used more instead, - the commit message as been changed accordingly. - In patch 4/4 (rev-list: allow missing tips with --missing=[print|allow*]), a code comment has been added before `if (get_oid_with_context(...))` in "revision.c::get_reference()" as suggested by Linus. - In patch 4/4 (rev-list: allow missing tips with --missing=[print|allow*]), a big NEEDSWORK comment has been added before the ugly early parsing loops for the `--exclude-promisor-objects` and `--missing=...` options in "builtin/rev-list.c". - In patch 4/4 (rev-list: allow missing tips with --missing=[print|allow*]), a code comment now uses "missing tips" instead of "missing commits" in "builtin/rev-list.c", as suggested by Linus. - In patch 4/4 (rev-list: allow missing tips with --missing=[print|allow*]), the added tests in t6022 have the following changes: - variables 'obj' and 'tip' have been renamed to 'missing_tip' and 'existing_tip' respectively as suggested by Linus, - a comment explaining how the 'existing_tip' variable is useful has been added as suggested by Linus, - `if test ...` is used instead of `if [ ... ]`, as suggested by Junio. - Also in patch 4/4 (rev-list: allow missing tips with --missing=[print|allow*]), the documentation of the `--missing=...` option has been improved to talk about missing tips. # Range-diff since V1 Unfortunately there has been many changes in patch 4/4, so the range-diff shows different patches: 1: b8abbc1d42 ! 1: 5233a83181 revision: clarify a 'return NULL' in get_reference() @@ Commit message revision: clarify a 'return NULL' in get_reference() In general when we know a pointer variable is NULL, it's clearer to - explicitely return NULL than to return that variable. + explicitly return NULL than to return that variable. In get_reference() when 'object' is NULL, we already return NULL when 'revs->exclude_promisor_objects && is_promisor_object(oid)' is true, but we return 'object' when 'revs->ignore_missing' is true. - Let's make the code clearer and more uniform by also explicitely + Let's make the code clearer and more uniform by also explicitly returning NULL when 'revs->ignore_missing' is true. + Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Christian Couder <chriscool@tuxfamily.org> ## revision.c ## -: ---------- > 2: cfa72c8cf1 oidset: refactor oidset_insert_from_set() 2: 208d43eb81 ! 3: 5668340516 t6022: fix 'even though' typo in comment @@ Metadata Author: Christian Couder <chriscool@tuxfamily.org> ## Commit message ## - t6022: fix 'even though' typo in comment + t6022: fix 'test' style and 'even though' typo Signed-off-by: Christian Couder <chriscool@tuxfamily.org> @@ t/t6022-rev-list-missing.sh: do - # Blobs are shared by all commits, so evethough a commit/tree + # Blobs are shared by all commits, so even though a commit/tree # might be skipped, its blob must be accounted for. - if [ $obj != "HEAD:1.t" ]; then +- if [ $obj != "HEAD:1.t" ]; then ++ if test $obj != "HEAD:1.t" ++ then echo $(git rev-parse HEAD:1.t) >>expect.raw && + echo $(git rev-parse HEAD:2.t) >>expect.raw + fi && 3: 8be34ce359 < -: ---------- rev-list: add --allow-missing-tips to be used with --missing=... -: ---------- > 4: 55792110ca rev-list: allow missing tips with --missing=[print|allow*] 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*] Documentation/rev-list-options.txt | 4 ++ builtin/rev-list.c | 15 +++++++- list-objects-filter.c | 11 +----- oidset.c | 10 +++++ oidset.h | 6 +++ revision.c | 16 ++++++-- t/t6022-rev-list-missing.sh | 61 +++++++++++++++++++++++++++++- 7 files changed, 106 insertions(+), 17 deletions(-)