Re: [PATCH v3] revision: add `--ignore-missing-links` user option

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 19, 2023, 08:45 UTC
Message-ID: <CAOLa=ZQdtdpu3KMMpvgr16A19xtjtOXG=HAtNrLKv97-D=Cd+g@mail.gmail.com>
In-Reply-To: <xmqqv8c7mq4o.fsf@gitster.g>

On Mon, Sep 18, 2023 at 5:56 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
> > This is to be expected, in my opinion. In terms of revision.c and
> > setting the `revs->ignore_missing_links` bit, the traversal will
> > go throw all objects (commits and otherwise) and call
> > `show_commit` or `show_object` on them.
>
> Yes.  And the user can choose how to handle such an object here by
> telling finish_object__ma() with the --missing=<how> option, so
> letting them do so, instead of robbing the choice from them, would
> be a more flexible design here, right?
>
> > if a commit is
> > missing, git-rev-list(1) will still barf an error, but this error
>
> OK, yeah, I do see the need for setting the ignore-missing-links bit
> for what you are doing, and --missing and --ignore-missing-links are
> orthogonal options.  Getting rid of the hardcoded skipping of
> finish_object__ma() would make sense from this angle, too.

Well. The only problem is that setting `ignore_missing_links` bit never calls `show_commit` for missing commits (since commits are pre-parsed in revision.c). So to keep that behavior consistent for non-commit objects, I hardcoded the skipping of `finish_object__ma()` in `show_object`.

If I remove the hardcoding, it would mean that `--ignore-missing-links` would skip missing commits but for non-commits objects, the user would have to pass `--missing=allow-any` else rev-list would still error out with a missing object error.

Don't you think this would be confusing for the user? I'm happy to send a revised version removing this hardcoding if you still think otherwise :)