From: Junio C Hamano <gitster@pobox•com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail•com>
Cc: git@vger•kernel.org, Eric Sunshine <sunshine@sunshineco•com>
Subject: Re: [PATCH v2 14/25] shallow.c: implement a generic shallow boundary finder based on rev-list
Date: Mon, 08 Feb 2016 13:09:24 -0800 [thread overview]
Message-ID: <xmqqvb5y7vuj.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1454576641-29615-15-git-send-email-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Thu, 4 Feb 2016 16:03:50 +0700")
Nguyễn Thái Ngọc Duy <pclouds@gmail•com> writes:
> Instead of a custom commit walker like get_shallow_commits(), this new
> function uses rev-list to mark NOT_SHALLOW to all reachable commits,
> except borders. The definition of reachable is to be defined by the
> protocol later. This makes it more flexible to define shallow boundary.
>
> Note: if a commit has one NOT_SHALLOW parent and one SHALLOW parent,
> then it's considered the boundary. Which means in the client side, this
> commit has _no_ parents. This could lead to surprising cuts if we're not
> careful.
>
> Another option is to include more commits and only mark commits whose
> all parents are SHALLOW as boundary.
The second and third are greek to me at this point ;-) but hopefully
they will become clear as we read on.
> +/*
> + * Given rev-list arguments, run rev-list. All reachable commits
> + * except border ones are marked with not_shallow_flag. Border commits
> + * are marked with shallow_flag. The list of border/shallow commits
> + * are also returned.
> + */
> +struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,
> + int shallow_flag,
> + int not_shallow_flag)
> +{
> + struct commit_list *result = NULL, *p;
> + struct rev_info revs;
> + unsigned int i, nr;
> +
> + /*
> + * SHALLOW (excluded) and NOT_SHALLOW (included) should not be
> + * set at this point. But better be safe than sorry.
> + */
> + nr = get_max_object_index();
> + for (i = 0; i < nr; i++) {
> + struct object *o = get_indexed_object(i);
> + if (!o || o->type != OBJ_COMMIT)
> + continue;
> + o->flags &= ~(shallow_flag | not_shallow_flag);
> + }
This is slightly different from clear_object_flags(), but I cannot
tell if it is intended, or if you forgot that the function exists.
> + is_repository_shallow(); /* make sure shallows are read */
> +
> + init_revisions(&revs, NULL);
> + save_commit_buffer = 0;
> + setup_revisions(ac, av, &revs, NULL);
> +
> + /* Mark all reachable commits as NOT_SHALLOW */
> + if (prepare_revision_walk(&revs))
> + die("revision walk setup failed");
> + traverse_commit_list(&revs, show_commit, NULL, ¬_shallow_flag);
> +
> + /*
> + * mark border commits SHALLOW + NOT_SHALLOW.
> + * We cannot clear NOT_SHALLOW right now. Imagine border
> + * commit A is processed first, then commit B, whose parent is
> + * A, later. If NOT_SHALLOW on A is cleared at step 1, B
> + * itself is considered border at step 2, which is incorrect.
> + */
> + nr = get_max_object_index();
> + for (i = 0; i < nr; i++) {
I'd really like not to see a loop over 0..get_max_object_index().
Are there many codepaths that peek into the in-core entire object
store already? Would it work equally well to keep track of the
commits discovered in show_commit() to use as the set of commits
you need to visit in this second pass?
> + struct object *o = get_indexed_object(i);
> + struct commit *c = (struct commit *)o;
> +
> + if (!o || o->type != OBJ_COMMIT ||
> + !(o->flags & not_shallow_flag))
> + continue;
> +
> + if (parse_commit(c))
> + die("unable to parse commit %s",
> + oid_to_hex(&c->object.oid));
> +
> + for (p = c->parents; p; p = p->next)
> + if (!(p->item->object.flags & not_shallow_flag)) {
> + o->flags |= shallow_flag;
> + commit_list_insert(c, &result);
> + break;
> + }
> + }
> +
> + /*
> + * Now we can clean up NOT_SHALLOW on border commits. Having
> + * both flags set can confuse the caller.
> + */
> + for (p = result; p; p = p->next) {
> + struct object *ro = &p->item->object;
Why "ro" only in this third pass, unlike the other two passes that
said "o" which is in a sense more descriptive?
> + if ((ro->flags & not_shallow_flag) &&
> + (ro->flags & shallow_flag))
If you introduce a "both_flags = shallow_flag | not_shallow_flag"
at the very beginning, this will become
if (o->flags & both_flags)
o->flags &= ~not_shallow_flag;
which would probably be easier to read. You can pass the same to
clear_object_flags() at the first pass.
> + ro->flags &= ~not_shallow_flag;
> + }
> + return result;
> +}
Other than that, this step looks quite straight-forward to me.
Thanks.
> +
> static void check_shallow_file_for_update(void)
> {
> if (is_shallow == -1)
next prev parent reply other threads:[~2016-02-08 21:09 UTC|newest]
Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-04 9:03 [PATCH v2 00/25] More flexibility in making shallow clones Nguyễn Thái Ngọc Duy
2016-02-04 9:03 ` [PATCH v2 01/25] remote-curl.c: convert fetch_git() to use argv_array Nguyễn Thái Ngọc Duy
2016-02-04 22:59 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 02/25] transport-helper.c: refactor set_helper_option() Nguyễn Thái Ngọc Duy
2016-02-04 23:18 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 03/25] transport-helper.c: do not send null option to remote helper Nguyễn Thái Ngọc Duy
2016-02-04 23:22 ` Junio C Hamano
2016-02-06 9:38 ` Duy Nguyen
2016-02-08 20:53 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 04/25] upload-pack: move shallow deepen code out of receive_needs() Nguyễn Thái Ngọc Duy
2016-02-04 23:30 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 05/25] upload-pack: move "shallow" sending code out of deepen() Nguyễn Thái Ngọc Duy
2016-02-04 9:03 ` [PATCH v2 06/25] upload-pack: remove unused variable "backup" Nguyễn Thái Ngọc Duy
2016-02-04 23:32 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 07/25] upload-pack: move "unshallow" sending code out of deepen() Nguyễn Thái Ngọc Duy
2016-02-04 23:39 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 08/25] upload-pack: use skip_prefix() instead of starts_with() when possible Nguyễn Thái Ngọc Duy
2016-02-04 23:42 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 09/25] upload-pack: tighten number parsing at "deepen" lines Nguyễn Thái Ngọc Duy
2016-02-04 23:48 ` Junio C Hamano
2016-02-15 3:07 ` Duy Nguyen
2016-02-04 9:03 ` [PATCH v2 10/25] upload-pack: move rev-list code out of check_non_tip() Nguyễn Thái Ngọc Duy
2016-02-04 9:03 ` [PATCH v2 11/25] fetch-pack: use skip_prefix() instead of starts_with() when possible Nguyễn Thái Ngọc Duy
2016-02-04 23:56 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 12/25] fetch-pack: use a common function for verbose printing Nguyễn Thái Ngọc Duy
2016-02-05 0:02 ` Junio C Hamano
2016-02-05 4:03 ` Eric Sunshine
2016-02-04 9:03 ` [PATCH v2 13/25] fetch-pack: use a separate flag for fetch in deepening mode Nguyễn Thái Ngọc Duy
2016-02-05 0:03 ` Junio C Hamano
2016-02-05 4:13 ` Eric Sunshine
2016-02-04 9:03 ` [PATCH v2 14/25] shallow.c: implement a generic shallow boundary finder based on rev-list Nguyễn Thái Ngọc Duy
2016-02-08 21:09 ` Junio C Hamano [this message]
2016-02-15 8:00 ` Duy Nguyen
2016-02-19 9:30 ` Duy Nguyen
2016-02-04 9:03 ` [PATCH v2 15/25] upload-pack: add deepen-since to cut shallow repos based on time Nguyễn Thái Ngọc Duy
2016-02-08 21:14 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 16/25] fetch: define shallow boundary with --shallow-since Nguyễn Thái Ngọc Duy
2016-02-04 9:03 ` [PATCH v2 17/25] clone: define shallow clone boundary based on time " Nguyễn Thái Ngọc Duy
2016-02-08 21:20 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 18/25] t5500, t5539: tests for shallow depth since a specific date Nguyễn Thái Ngọc Duy
2016-02-08 21:24 ` Junio C Hamano
2016-02-15 7:17 ` Duy Nguyen
2016-02-04 9:03 ` [PATCH v2 19/25] refs: add expand_ref() Nguyễn Thái Ngọc Duy
2016-02-08 21:27 ` Junio C Hamano
2016-02-04 9:03 ` [PATCH v2 20/25] upload-pack: support define shallow boundary by excluding revisions Nguyễn Thái Ngọc Duy
2016-02-05 5:03 ` Eric Sunshine
2016-02-08 21:34 ` Junio C Hamano
2016-02-05 5:05 ` Eric Sunshine
2016-02-15 3:31 ` Duy Nguyen
2016-02-04 9:03 ` [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude Nguyễn Thái Ngọc Duy
2016-02-05 5:26 ` Eric Sunshine
2016-02-15 3:53 ` Duy Nguyen
2016-02-15 5:52 ` Eric Sunshine
2016-02-15 5:56 ` Eric Sunshine
2016-02-15 8:15 ` Duy Nguyen
2016-02-19 1:35 ` Eric Sunshine
2016-02-04 9:03 ` [PATCH v2 22/25] clone: define shallow clone " Nguyễn Thái Ngọc Duy
2016-02-04 9:03 ` [PATCH v2 23/25] t5500, t5539: tests for shallow depth excluding a ref Nguyễn Thái Ngọc Duy
2016-02-04 9:04 ` [PATCH v2 24/25] upload-pack: make check_reachable_object() return unreachable list if asked Nguyễn Thái Ngọc Duy
2016-02-05 5:41 ` Eric Sunshine
2016-02-04 9:04 ` [PATCH v2 25/25] fetch, upload-pack: --deepen=N extends shallow boundary by N commits Nguyễn Thái Ngọc Duy
2016-02-08 21:45 ` [PATCH v2 00/25] More flexibility in making shallow clones Junio C Hamano
2016-02-12 0:24 ` Duy Nguyen
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqvb5y7vuj.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox