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

* Re: [PATCH] Packfile-uris support excluding commit objects
@ 2021-05-18  8:08 Teng Long
  0 siblings, 0 replies; 3+ messages in thread
From: Teng Long @ 2021-05-18  8:08 UTC (permalink / raw)
  To: avarab; +Cc: dyroneteng, git, jonathantanmy

To Ævar Arnfjörð Bjarmaso wrote:

> It seems like this and your
> http://lore.kernel.org/git/20210506073354.27833-1-dyroneteng@gmail.com
> 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/ 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".

^ permalink raw reply	[flat|nested] 3+ messages in thread