From: Junio C Hamano <gitster@pobox.com>
To: Teng Long <dyroneteng@gmail.com>
Cc: git@vger.kernel.org, jonathantanmy@google.com, avarab@gmail.com
Subject: Re: [PATCH v2 1/3] packfile-uris: support for excluding commit object
Date: Wed, 19 May 2021 13:28:09 +0900 [thread overview]
Message-ID: <xmqq5yzfz6sm.fsf@gitster.g> (raw)
In-Reply-To: <73e64147b17cb382d34357c913616095b6169650.1621327467.git.dyroneteng@gmail.com> (Teng Long's message of "Tue, 18 May 2021 16:49:51 +0800")
Teng Long <dyroneteng@gmail.com> writes:
> On the server, more sophisticated means of excluding objects should be
> supported, such as commit object. This commit introduces a new
> configuration `uploadpack.excludeobject` for this.
This "should" is not justfied at all, it seems? What is lacking in
what we already have? What new things does it all us to do by
adding a new configuration variable?
> The old configuration `uploadpack.blobpackfileuri` is only support to
> exclude blobs and the name has no abstract meaning, so the configruation
> name changes, to support more object types. Compatibility issues will
> not be considered because packfile-uris now is an experimental feature.
I'll let Jonathan speak up, but even for an experimental feature,
whatever new and incompatible way to do things should have a clear
advantage compared to the old way. Sell the backward incomptibility
along that line---"it is an experimental so I'll trash it" is not,
but "by doing this it gets this much better, and migrating existing
users won't be too taxing (it is just this simple thing)" is an
acceptable way to justify such a change.
Note that I am not opposed to the proposed change (and I am not
supporting it, either). I do have a problem with the way the change
is sold, though.
> builtin/pack-objects.c | 53 ++++++++++++++++++++++++++++++------------
> fetch-pack.c | 5 ++++
> upload-pack.c | 5 ++--
> 3 files changed, 45 insertions(+), 18 deletions(-)
Even though the name of the configuration variable changed, and the
semantics of the value of it changed, there is no documentation
change, because...?
Because the original didn't even document the variable properly? It
may be another reason why changing it may not impact the existing
users too much.
> @@ -132,6 +134,7 @@ struct configured_exclusion {
> struct oidmap_entry e;
> char *pack_hash_hex;
> char *uri;
> + int recursively:1;
> };
> static struct oidmap configured_exclusions;
>
> @@ -1291,10 +1294,16 @@ static int want_object_in_pack_one(struct packed_git *p,
> * and its offset in these variables.
> */
> static int want_object_in_pack(const struct object_id *oid,
> + enum object_type type,
> int exclude,
> struct packed_git **found_pack,
> off_t *found_offset)
> {
> + if (exclude_until_next_commit && type != OBJ_COMMIT)
> + return 0;
> + if (type == OBJ_COMMIT)
> + exclude_until_next_commit = 0 ;
Lose SP before the semicolon.
Our codebase does not allow statements before declarations. Move
all of the above down to be below the block of decls at the
beginning of the function.
> int want;
> struct list_head *pos;
> struct multi_pack_index *m;
> @@ -1345,6 +1354,8 @@ static int want_object_in_pack(const struct object_id *oid,
> &p) &&
> *p == ':') {
> oidset_insert(&excluded_by_config, oid);
> + if(ex->recursively && type == OBJ_COMMIT)
> + exclude_until_next_commit = 1;
This depends on a new file-scope global variable, which means two
things.
* if two or more threads are deciding which object to pack and not
to pack, this code will horribly break, as they are traversing
totally different parts of the object DAG to find out which
objects to pack, but one thread hitting a commit to be excluded
and setting this flag will cause other thread skip unrelated
blobs and trees that it discovers, doesn't it?
* even if we assume there is no concurrency and reentrancy issues
(e.g. by forcing single-threaded operation when this feature is
in use), the code _assumes_ a concrete order in which this helper
function gets called, namely, non-commit objects fed to this
helper after the helper gets a single commit object *all* belong
to that commit. With the current code that feeds objects as they
are discovered during depth first traversal of the top-level tree
starting at each commit, that assumption might hold, but it feels
that the assumption is too much to be healty. For example, would
it be possible for the bitmap code to cause this helper to be
called in different order (i.e. it might find it more convenent
to feed a tree, a blob or a tag that is unrelated to the commit
that was last fed to the helper)? If so, the logic in this code
will constrain the caller too much.
I'll stop reading for now at this place; review of the remainder may
come at a later time, but not now.
Thanks.
next prev parent reply other threads:[~2021-05-19 4:28 UTC|newest]
Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-07 2:11 [PATCH] Packfile-uris support excluding commit objects Teng Long
2021-05-10 11:14 ` Ævar Arnfjörð Bjarmason
2021-05-18 8:49 ` [PATCH v2 0/3] packfile-uris: commit objects exclusion Teng Long
2021-05-18 8:49 ` [PATCH v2 1/3] packfile-uris: support for excluding commit object Teng Long
2021-05-19 4:28 ` Junio C Hamano [this message]
2021-05-20 4:46 ` Junio C Hamano
2021-05-18 8:49 ` [PATCH v2 2/3] packfile-uris.txt: " Teng Long
2021-05-18 8:49 ` [PATCH v2 3/3] t5702: excluding commits with packfile-uris Teng Long
2021-07-26 9:46 ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Teng Long
2021-07-26 9:46 ` [PATCH v3 1/3] packfile-uris: support for excluding commit objects Teng Long
2021-07-26 18:15 ` Junio C Hamano
2021-07-26 19:45 ` Felipe Contreras
2021-08-11 1:44 ` Teng Long
2021-07-26 9:46 ` [PATCH v3 2/3] t5702: " Teng Long
2021-07-26 15:03 ` Ævar Arnfjörð Bjarmason
2021-08-11 1:46 ` [PATCH v3 1/3] packfile-uris: " Teng Long
2021-07-26 9:46 ` [PATCH v3 3/3] packfile-uri.txt: " Teng Long
2021-07-26 20:52 ` Junio C Hamano
2021-08-11 1:47 ` Teng Long
2021-07-26 12:34 ` [PATCH v3 0/3] packfile-uris: commit objects exclusio Ævar Arnfjörð Bjarmason
2021-08-11 1:48 ` Teng Long
2021-08-11 7:45 ` [PATCH v4 0/7] packfile-uris: commits and trees exclusion Teng Long
2021-08-11 7:45 ` [PATCH v4 1/7] pack-objects.c: introduce new method `match_packfile_uri_exclusions` Teng Long
2021-08-11 7:45 ` [PATCH v4 2/7] Add new parameter "carry_data" for "show_object" function Teng Long
2021-08-11 7:45 ` [PATCH v4 3/7] packfile-uri: support for excluding commit objects Teng Long
2021-08-11 7:45 ` [PATCH v4 4/7] packfile-uri: support for excluding tree objects Teng Long
2021-08-11 7:45 ` [PATCH v4 5/7] packfile-uri.txt: support for excluding commits and trees Teng Long
2021-08-11 9:59 ` Bagas Sanjaya
2021-08-11 7:45 ` [PATCH v4 6/7] t5702: replace with "test_when_finished" for cleanup Teng Long
2021-08-11 7:45 ` [PATCH v4 7/7] t5702: support for excluding commit objects Teng Long
2021-08-25 2:21 ` [PATCH v5 00/14] packfile-uris: commits, trees and tags exclusion Teng Long
2021-08-25 2:21 ` [PATCH v5 01/14] pack-objects.c: introduce new method `match_packfile_uri_exclusions` Teng Long
2021-08-25 2:21 ` [PATCH v5 02/14] Add new parameter "carry_data" for "show_object" function Teng Long
2021-08-26 20:45 ` Junio C Hamano
2021-09-02 11:08 ` Teng Long
2021-08-25 2:21 ` [PATCH v5 03/14] packfile-uri: support for excluding commit objects Teng Long
2021-08-25 23:49 ` Ævar Arnfjörð Bjarmason
2021-09-02 12:26 ` Teng Long
2021-08-26 20:56 ` Junio C Hamano
2021-09-02 12:51 ` Teng Long
2021-08-25 2:21 ` [PATCH v5 04/14] packfile-uri: support for excluding tree objects Teng Long
2021-08-25 2:21 ` [PATCH v5 05/14] packfile-uri.txt: support for excluding commits and trees Teng Long
2021-08-25 23:52 ` Ævar Arnfjörð Bjarmason
2021-09-02 11:23 ` Teng Long
2021-08-25 2:21 ` [PATCH v5 06/14] t5702: replace with "test_when_finished" for cleanup Teng Long
2021-08-25 23:55 ` Ævar Arnfjörð Bjarmason
2021-09-02 11:37 ` Teng Long
2021-08-25 2:21 ` [PATCH v5 07/14] t5702: support for excluding commit objects Teng Long
2021-08-25 2:21 ` [PATCH v5 08/14] Add new parameter "carry_data" for "show_commit function Teng Long
2021-08-25 2:21 ` [PATCH v5 09/14] commit.h: add wrapped tags in commit struct Teng Long
2021-08-25 23:58 ` Ævar Arnfjörð Bjarmason
2021-09-02 12:17 ` Teng Long
2021-09-02 12:39 ` ZheNing Hu
2021-09-02 13:01 ` Teng Long
2021-08-25 2:21 ` [PATCH v5 10/14] object.h: add referred tags in `referred_objects` struct Teng Long
2021-08-25 2:21 ` [PATCH v5 11/14] packfile-uri: support for excluding tag objects Teng Long
2021-08-25 2:21 ` [PATCH v5 12/14] packfile-uri.txt: " Teng Long
2021-08-25 2:21 ` [PATCH v5 13/14] t5702: add tag exclusion test case Teng Long
2021-08-25 2:21 ` [PATCH v5 14/14] pack-objects.c: introduce `want_exclude_object` function Teng Long
2021-10-19 11:38 ` [PATCH v6 00/12] packfile-uri: support excluding multiple object types Teng Long
2021-10-19 11:38 ` [PATCH v6 01/12] objects.c: introduce `exclude_level` enum Teng Long
2021-10-19 11:38 ` [PATCH v6 02/12] Introduce function `match_packfile_uri_exclusions` Teng Long
2021-10-19 11:38 ` [PATCH v6 03/12] Replace `show_data` with structure `show_info` Teng Long
2021-10-19 11:38 ` [PATCH v6 04/12] Introduce `uploadpack.excludeobject` configuration Teng Long
2021-10-19 11:38 ` [PATCH v6 05/12] t5702: test cases for `uploadpack.excludeobject` Teng Long
2021-10-19 11:38 ` [PATCH v6 06/12] packfile-uri: support for excluding commits Teng Long
2021-10-19 11:38 ` [PATCH v6 07/12] t5702: test cases " Teng Long
2021-10-19 11:38 ` [PATCH v6 08/12] packfile-uri: support for excluding trees Teng Long
2021-10-19 11:38 ` [PATCH v6 09/12] t5702: test cases " Teng Long
2021-10-19 11:38 ` [PATCH v6 10/12] packfile-uri: support for excluding tags Teng Long
2021-10-19 11:38 ` [PATCH v6 11/12] t5702: test cases " Teng Long
2021-10-19 11:38 ` [PATCH v6 12/12] packfile-uri.txt: support multiple object types Teng Long
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=xmqq5yzfz6sm.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=dyroneteng@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).