git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 14/25] shallow.c: implement a generic shallow boundary finder based on rev-list

From
Junio C Hamano <gitster@pobox.com>
Date
Feb 8, 2016, 21:09 UTC
Message-ID
<xmqqvb5y7vuj.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1454576641-29615-15-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 12 quoted lines
> 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.

Show 25 quoted lines
> +/*
> + * 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.

Show 20 quoted lines
> +	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, &not_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?

Show 25 quoted lines
> +		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)
Previous: Nguyễn Thái Ngọc DuyNext: Duy Nguyen
Message 32 of 63 in “More flexibility in making shallow clones”
  1. 00/25 More flexibility in making shallow clonesNguyễn Thái Ngọc Duy, Feb 4, 2016
  2. 01/25 remote-curl.c: convert fetch_git() to use argv_arrayNguyễn Thái Ngọc Duy, Feb 4, 2016
  3. Junio C HamanoFeb 4, 2016
  4. 02/25 transport-helper.c: refactor set_helper_option()Nguyễn Thái Ngọc Duy, Feb 4, 2016
  5. Junio C HamanoFeb 4, 2016
  6. 03/25 transport-helper.c: do not send null option to remote helperNguyễn Thái Ngọc Duy, Feb 4, 2016
  7. Junio C HamanoFeb 4, 2016
  8. Duy NguyenFeb 6, 2016
  9. Junio C HamanoFeb 8, 2016
  10. 04/25 upload-pack: move shallow deepen code out of receive_needs()Nguyễn Thái Ngọc Duy, Feb 4, 2016
  11. Junio C HamanoFeb 4, 2016
  12. 05/25 upload-pack: move "shallow" sending code out of deepen()Nguyễn Thái Ngọc Duy, Feb 4, 2016
  13. 06/25 upload-pack: remove unused variable "backup"Nguyễn Thái Ngọc Duy, Feb 4, 2016
  14. Junio C HamanoFeb 4, 2016
  15. 07/25 upload-pack: move "unshallow" sending code out of deepen()Nguyễn Thái Ngọc Duy, Feb 4, 2016
  16. Junio C HamanoFeb 4, 2016
  17. 08/25 upload-pack: use skip_prefix() instead of starts_with() when possibleNguyễn Thái Ngọc Duy, Feb 4, 2016
  18. Junio C HamanoFeb 4, 2016
  19. 09/25 upload-pack: tighten number parsing at "deepen" linesNguyễn Thái Ngọc Duy, Feb 4, 2016
  20. Junio C HamanoFeb 4, 2016
  21. Duy NguyenFeb 15, 2016
  22. 10/25 upload-pack: move rev-list code out of check_non_tip()Nguyễn Thái Ngọc Duy, Feb 4, 2016
  23. 11/25 fetch-pack: use skip_prefix() instead of starts_with() when possibleNguyễn Thái Ngọc Duy, Feb 4, 2016
  24. Junio C HamanoFeb 4, 2016
  25. 12/25 fetch-pack: use a common function for verbose printingNguyễn Thái Ngọc Duy, Feb 4, 2016
  26. Junio C HamanoFeb 5, 2016
  27. Eric SunshineFeb 5, 2016
  28. 13/25 fetch-pack: use a separate flag for fetch in deepening modeNguyễn Thái Ngọc Duy, Feb 4, 2016
  29. Junio C HamanoFeb 5, 2016
  30. Eric SunshineFeb 5, 2016
  31. 14/25 shallow.c: implement a generic shallow boundary finder based on rev-listNguyễn Thái Ngọc Duy, Feb 4, 2016
  32. Junio C HamanoFeb 8, 2016
  33. Duy NguyenFeb 15, 2016
  34. Duy NguyenFeb 19, 2016
  35. 15/25 upload-pack: add deepen-since to cut shallow repos based on timeNguyễn Thái Ngọc Duy, Feb 4, 2016
  36. Junio C HamanoFeb 8, 2016
  37. 16/25 fetch: define shallow boundary with --shallow-sinceNguyễn Thái Ngọc Duy, Feb 4, 2016
  38. 17/25 clone: define shallow clone boundary based on time with --shallow-sinceNguyễn Thái Ngọc Duy, Feb 4, 2016
  39. Junio C HamanoFeb 8, 2016
  40. 18/25 t5500, t5539: tests for shallow depth since a specific dateNguyễn Thái Ngọc Duy, Feb 4, 2016
  41. Junio C HamanoFeb 8, 2016
  42. Duy NguyenFeb 15, 2016
  43. 19/25 refs: add expand_ref()Nguyễn Thái Ngọc Duy, Feb 4, 2016
  44. Junio C HamanoFeb 8, 2016
  45. 20/25 upload-pack: support define shallow boundary by excluding revisionsNguyễn Thái Ngọc Duy, Feb 4, 2016
  46. Eric SunshineFeb 5, 2016
  47. Junio C HamanoFeb 8, 2016
  48. Eric SunshineFeb 5, 2016
  49. Duy NguyenFeb 15, 2016
  50. 21/25 fetch: define shallow boundary with --shallow-excludeNguyễn Thái Ngọc Duy, Feb 4, 2016
  51. Eric SunshineFeb 5, 2016
  52. Duy NguyenFeb 15, 2016
  53. Eric SunshineFeb 15, 2016
  54. Eric SunshineFeb 15, 2016
  55. Duy NguyenFeb 15, 2016
  56. Eric SunshineFeb 19, 2016
  57. 22/25 clone: define shallow clone boundary with --shallow-excludeNguyễn Thái Ngọc Duy, Feb 4, 2016
  58. 23/25 t5500, t5539: tests for shallow depth excluding a refNguyễn Thái Ngọc Duy, Feb 4, 2016
  59. 24/25 upload-pack: make check_reachable_object() return unreachable list if askedNguyễn Thái Ngọc Duy, Feb 4, 2016
  60. Eric SunshineFeb 5, 2016
  61. 25/25 fetch, upload-pack: --deepen=N extends shallow boundary by N commitsNguyễn Thái Ngọc Duy, Feb 4, 2016
  62. Junio C HamanoFeb 8, 2016
  63. Duy NguyenFeb 12, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.