Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`

> On Tue, May 02, 2023 at 08:09:47PM -0400, Taylor Blau wrote:
>
> > However, there is no option to be able to keep around certain objects
> > that have otherwise aged out of the grace period. The only way to retain
> > those objects is:
> >
> >   - to point a reference at them, which may be undesirable or
> >     infeasible,
> >   - to track them via the reflog, which may be undesirable since the
> >     reflog's lifetime is limited to that of the reference it's tracking
> >     (and callers may want to keep those unreachable objects around for
> >     longer)
> >   - to extend the grace period, which may keep around other objects that
> >     the caller *does* want to discard,
> >   - or, to force the caller to construct the pack of objects they want
> >     to keep themselves, and then mark the pack as kept by adding a
> >     ".keep" file.
>
> OK, I understand the use case you're trying to support, and your
> approach mostly makes sense. But there are two things I was surprised by
> in the implementation:
>
>   1. Does this need to be tied to cruft packs? The same logic would
>      apply to "repack -A" which turns objects loose (of course you
>      probably don't want to do that in the long term for performance
>      reasons, but it's conceptually the same thing; see below).
>
>   2. Why is there a separate walk distinct from the one that rescues
>      recent-but-unreachable objects?

> Conceptually it seems to me that the simplest and most flexible way to
> think of this new feature is: pretend these objects are recent enough to
> be kept in the grace period, even though their mtimes do not qualify".
>
> And then everything else would just fall out naturally. Am I missing
> something?

> > +static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args)
> > +{
> > +	struct child_process cmd = CHILD_PROCESS_INIT;
> > +	struct strbuf buf = STRBUF_INIT;
> > +	FILE *out = NULL;
> > +	int ret = 0;
> > +
> > +	cmd.use_shell = 1;
> > +	cmd.out = -1;
> > +
> > +	strvec_push(&cmd.args, args);
> > +	strvec_pushv(&cmd.env, (const char **)local_repo_env);
>
> Why does this clear the environment of local-repo variables? That seems
> unlike most other hooks we have, and would cause confusion if $GIT_DIR
> or various config variables are important (e.g., should "git -c foo.bar
> gc" persist foo.bar when running the hook? It usually does).
>
> I know that some hooks that try to change repositories by changing
> directories have the opposite confusion ($GIT_DIR is set, but they did
> not want it). But it makes more sense to me to remain consistent with
> how other hooks behave.

> > +	if (start_command(&cmd)) {
> > +		ret = -1;
> > +		goto done;
> > +	}
>
> This may be a matter of taste, but you can "return -1" directly here, as
> nothing has been allocated (and a failed start_command() will call
> child_process_clear() for you). This would mean "out" is always set in
> the "done:" label, so it wouldn't need a NULL-check (nor an
> initialization).

> > +
> > +	out = xfdopen(cmd.out, "r");
> > +	while (strbuf_getline_lf(&buf, out) != EOF) {
>
> is there any reason to be a stickler for LF versus CRLF here? I.e.,
> wouldn't strbuf_getline() be more friendly, with little chance that we
> misinterpret the result?

> > +		struct object_id oid;
> > +		struct object *obj;
> > +		const char *rest;
> > +
> > +		if (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {
> > +			ret = error(_("invalid extra cruft tip: '%s'"), buf.buf);
> > +			goto done;
> > +		}
> > +
> > +		obj = parse_object(the_repository, &oid);
> > +		if (!obj)
> > +			continue;
>
> This parse_object() can be pretty expensive, especially if you are
> rescuing a lot of objects (or if your program directly references large
> blobs). Can we use oid_object_info() here in combination with
> lookup_object_by_type()?