git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Teng Long <dyroneteng@gmail.com>
To: avarab@gmail.com
Cc: dyroneteng@gmail.com, git@vger.kernel.org, jonathantanmy@google.com
Subject: Re: [PATCH] Packfile-uris support excluding commit objects
Date: Tue, 18 May 2021 16:08:36 +0800	[thread overview]
Message-ID: <CADMgQSReX+Ue0AtPh6JWNde-pyFC5-r+juQFoKd5fZp5Wv6_Ow@mail.gmail.com> (raw)

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".

             reply	other threads:[~2021-05-18  8:08 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18  8:08 Teng Long [this message]
  -- strict thread matches above, loose matches on Subject: below --
2021-05-07  2:11 [PATCH] Packfile-uris support excluding commit objects Teng Long
2021-05-10 11:14 ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CADMgQSReX+Ue0AtPh6JWNde-pyFC5-r+juQFoKd5fZp5Wv6_Ow@mail.gmail.com \
    --to=dyroneteng@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).