Re: [PATCH] Packfile-uris support excluding commit objects

To Ævar Arnfjörð Bjarmaso wrote:

It seems like this and your
http://lore.kernel.org/git/20210506073354.27833-1-dyroneteng@gmail.com (local)
should be part of one series, not split up.

Obviously, the current series of patches will be longer, and the patches
of the separate documents you mentioned can be repaired and released in
advance. The latter, Junio said, will be added to the queue.

Per my understanding in
https://lore.kernel.org/git/87o8hk820f.fsf@evledraar.gmail.com/ (local) this +
Jonathan's earlier bfc2a36ff2a (Doc: clarify contents of packfile sent
as URI, 2021-01-20) still makes this whole thing more confusing that it
needs to be.

I think we should just have a new uploadpack.excludeObject, and document
that uploadpack.blobpackfileuri is an (unfortunately named) synonym for
it. I.e. the actual implementation doesn't care about the objec type it
just excludes any object listed via an oidmap. No?

Regarding the naming of "uploadpack.blobpackfileuri" and future
scalability issues, I have similar feelings. In next round, I will follow
your naming suggestions about "uploadpack.excludeObject". In addition,
the naming modification may cause a compatible question, although I know
packfile-uris is an experimental feature, I still hope to get some
compatibility-related suggestions.

I realize you're probably not a native English speaker (neither am I),
but I honestly can't understand that "This work will be done in a
further patch recently.". Do you mean something like:
       ......

Thanks for correcting, I may rewrite the commit message in the next
patch. I'm not a native English speaker, improving :)

Please send the earlier doc cleanup + the spec change for this + any doc
updates as one series.

The reason for splitting the two patches is mentioned above, and the
corresponding document modification to support the exclusion of commit
will be added in the next patch series.

Nit: Split this across two lines.
Indending with spaces.
More indenting with spaces, also don't need the {} here.
Don't indent the "then", also spaces...
Use ">objh" not "> objh".

Thanks, will correct them in the next round.

I think by having a uploadpack.excludeObject documented as the primary
interface to this we could just say "object already listed by an earlier
exclusion" or something like that.

Thanks, will refer to your suggestions.

This whole if/else seems like it could be better split up by discovering
the variable first, using that as a variable, and then avoiding the
duplication. But if we just used uploadpack.excludeObject...

I will try to modify it, but I am not sure to fully understand what you
mean. If this problem persists in the next patch, please help to point
out the problem.

Put stuff like this in "test_when_finished"
You can just use test_commit here, no?

Thanks, now I know about "test_when_finished" and "test_commit".
In addition, there are some problems with using "rm" directly in t5702,
I will replace them in the next patch series.

Personally I'd just skip this whole "rev-parse HEAD" etc. and just pass
the tag name(s) created by earlier test_commit, then have
configure_exclusion ust always do a rev-parse...

Thanks, will use tag name(s) instead of "HEAD".