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

Re: [PATCH v6 09/40] Add initial external odb support

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Sep 19, 2017, 17:45 UTC
Message-ID
<20170919104553.494d7d3b@twelve2.svl.corp.google.com>
In-Reply-To
<20170916080731.13925-10-chriscool@tuxfamily.org>

I wonder if it's better to get a change like this (PATCH v6 09/40 and any of the previous patches that this depends on) in and then build on it rather than to review the whole patch set at a time. This would reduce ripple effects (of needing to change later patches in a patch set multiple times unnecessarily) and help collaboration (in that multiple people can write patches, since the foundation would already be laid).

The same concerns about fsck apply, but that shouldn't be a problem, since this patch provides the same internal API as mine ("get" function taking in a single hash, and "have" function taking in a single hash) so it shouldn't be too difficult to adapt my fsck and gc patches [1]. (I can do that if necessary.)

[1] https://public-inbox.org/git/20170915134343.3814dc38@twelve2.svl.corp.google.com/

One possible issue (with committing something early) is that later work (for example, a fast long-running process protocol) will make the earlier work (for example, here, a simple single-shot protocol) obsolete, while saddling us with the necessity of maintaining the earlier one. To that end, if we want to start with the support for a hook, a better approach might be to only code the fast long-running process protocol, and put a wrapper script in contrib/ that can wrap a single-shot process in a long-running process.

And another possible issue is that we design ourselves into a corner. Thinking about the use cases that I know about (the Android use case and the Microsoft GVFS use case), I don't think we are doing that - for Android, this means that large blob metadata needs to be part of the design (and this patch series does provide for that), and for Microsoft GVFS, "get" is relatively cheap, so a configuration option to not invoke "have" first when loading a missing object might be sufficient.

As for the design itself (including fetch and clone), it differs from my patches (linked above as [1]) in that mine is self-contained (requiring only an updated Git server and Git client) whereas this, as far as I can tell, requires an external process and some measure of coordination between the administrator of the server and the client user (for example, the client must have the same ODB mechanism as the server, if not, the server might omit certain blobs that the client does not know how to fetch).

And I think that my design can be extended to support a use case in which, for example, blobs corresponding to a certain type of filename (defined by a glob like in gitattributes) can be excluded during fetch/clone, much like --blob-max-bytes, and they can be fetched either through the built-in mechanism or through a custom hook.

For those reasons, I still lean towards my design, but if we do want to go with this design, here are my comments about this patch...

First of all:
 - You'll probably need to add a repository extension.
 - I get compile errors when I "git am" these onto master. I think
   '#include "config.h"' is needed in some places.

On Sat, 16 Sep 2017 10:07:00 +0200 Christian Couder <christian.couder@gmail.com> wrote:

Show 9 quoted lines
> The external-odb.{c,h} files contains the functions that are
> called by the rest of Git from "sha1_file.c".
> 
> The odb-helper.{c,h} files contains the functions to
> actually implement communication with the external scripts or
> processes that will manage external git objects.
> 
> For now only script mode is supported, and only the 'have' and
> 'get_git_obj' instructions are supported.

This "have", as I see from this commit, is more like a "list" command in that it lists all hashes that it knows about, and does not check if a given hash exists.

> +static struct odb_helper *helpers;
> +static struct odb_helper **helpers_tail = &helpers;
This could be done with the helpers in list.h instead.
Show 9 quoted lines
> +int external_odb_get_object(const unsigned char *sha1)
> +{
> +	struct odb_helper *o;
> +	const char *path;
> +
> +	if (!external_odb_has_object(sha1))
> +		return -1;
> +
> +	path = sha1_file_name_alt(external_odb_root(), sha1);

If the purpose of making these functions global in the previous patch is just for temporary names, I don't think it's necessary for them to be global. Just concatenate the hex SHA1 to external_odb_root()?

Show 34 quoted lines
>  /* Returns 1 if we have successfully freshened the file, 0 otherwise. */
> @@ -667,7 +684,7 @@ static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)
>  		if (check_and_freshen_file(path, freshen))
>  			return 1;
>  	}
> -	return 0;
> +	return external_odb_has_object(sha1);
>  }
>  
>  static int check_and_freshen(const unsigned char *sha1, int freshen)
> @@ -824,6 +841,9 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st,
>  			return 0;
>  	}
>  
> +	if (!external_odb_get_object(sha1) && !lstat(*path, st))
> +		return 0;
> +
>  	return -1;
>  }
>  
> @@ -859,7 +879,14 @@ static int open_sha1_file(const unsigned char *sha1, const char **path)
>  	if (fd >= 0)
>  		return fd;
>  
> -	return open_sha1_file_alt(sha1, path);
> +	fd = open_sha1_file_alt(sha1, path);
> +	if (fd >= 0)
> +		return fd;
> +
> +	if (!external_odb_get_object(sha1))
> +		fd = open_sha1_file_alt(sha1, path);
> +
> +	return fd;
>  }

Any reason why you prefer to update the loose object functions than to update the generic one (sha1_object_info_extended)? My concern with just updating the loose object functions was that a caller might have obtained the path by iterating through the loose object dirs, and in that case we shouldn't query the external ODB for anything.

Show 15 quoted lines
> +ALT_SOURCE="$PWD/alt-repo/.git"
> +export ALT_SOURCE
> +write_script odb-helper <<\EOF
> +GIT_DIR=$ALT_SOURCE; export GIT_DIR
> +case "$1" in
> +have)
> +	git cat-file --batch-check --batch-all-objects |
> +	awk '{print $1 " " $3 " " $2}'
> +	;;
> +get_git_obj)
> +	cat "$GIT_DIR"/objects/$(echo $2 | sed 's#..#&/#')
> +	;;
> +esac
> +EOF
> +HELPER="\"$PWD\"/odb-helper"

Thanks for the clear test. It is very obvious that "have" returns a list of objects, and "get_git_obj" returns the compressed loose object with the Git loose object header included.

Show 5 quoted lines
> +
> +test_expect_success 'setup alternate repo' '
> +	git init alt-repo &&
> +	(cd alt-repo &&
> +	 test_commit one &&
Probably better written as "test_commit -C alt-repo one".
> +	 test_commit two
> +	) &&
> +	alt_head=`cd alt-repo && git rev-parse HEAD`
I think the style is to use $() and "git -C alt-repo rev-parse HEAD".
Previous: Christian CouderNext: Christian Couder
Message 39 of 49 in “Add initial experimental external ODB support”
  1. 00/40 Add initial experimental external ODB supportChristian Couder, Sep 16, 2017
  2. 01/40 builtin/clone: get rid of 'value' strbufChristian Couder, Sep 16, 2017
  3. 04/40 t0021/rot13-filter: improve error messageChristian Couder, Sep 16, 2017
  4. 05/40 t0021/rot13-filter: add packet_initialize()Christian Couder, Sep 16, 2017
  5. 07/40 Add Git/Packet.pm from parts of t0021/rot13-filter.plChristian Couder, Sep 16, 2017
  6. 11/40 t0400: add 'put_raw_obj' instruction to odb-helper scriptChristian Couder, Sep 16, 2017
  7. 10/40 odb-helper: add odb_helper_init() to send 'init' instructionChristian Couder, Sep 16, 2017
  8. 16/40 Add t0410 to test external ODB transferChristian Couder, Sep 16, 2017
  9. 14/40 t0400: add test for external odb write supportChristian Couder, Sep 16, 2017
  10. 19/40 lib-httpd: add list.shChristian Couder, Sep 16, 2017
  11. 22/40 pack-objects: don't pack objects in external odbsChristian Couder, Sep 16, 2017
  12. 26/40 odb-helper: add init_object_process()Christian Couder, Sep 16, 2017
  13. 29/40 odb-helper: add put_object_process()Christian Couder, Sep 16, 2017
  14. 30/40 Add t0470 to test passing raw objectsChristian Couder, Sep 16, 2017
  15. 31/40 odb-helper: add have_object_process()Christian Couder, Sep 16, 2017
  16. 32/40 Add t0480 to test "have" capability and raw objectsChristian Couder, Sep 16, 2017
  17. 33/40 external-odb: use 'odb=magic' attribute to mark odb blobsChristian Couder, Sep 16, 2017
  18. 38/40 Add tests for 'clone --initial-refspec'Christian Couder, Sep 16, 2017
  19. 39/40 Add t0430 to test cloning using bundlesChristian Couder, Sep 16, 2017
  20. 40/40 Doc/external-odb: explain transfering objects and metadataChristian Couder, Sep 16, 2017
  21. 37/40 clone: disable external odb before initial cloneChristian Couder, Sep 16, 2017
  22. 35/40 clone: add 'initial' param to write_remote_refs()Christian Couder, Sep 16, 2017
  23. 36/40 clone: add --initial-refspec optionChristian Couder, Sep 16, 2017
  24. 34/40 Add Documentation/technical/external-odb.txtChristian Couder, Sep 16, 2017
  25. 25/40 odb-helper: add 'script_mode' to 'struct odb_helper'Christian Couder, Sep 16, 2017
  26. 28/40 Add t0460 to test passing git objectsChristian Couder, Sep 16, 2017
  27. 23/40 Add t0420 to test transfer to HTTP external odbChristian Couder, Sep 16, 2017
  28. 21/40 odb-helper: add odb_helper_get_raw_object()Christian Couder, Sep 16, 2017
  29. 20/40 lib-httpd: add apache-e-odb.confChristian Couder, Sep 16, 2017
  30. 18/40 lib-httpd: add upload.shChristian Couder, Sep 16, 2017
  31. 24/40 external-odb: add 'get_direct' supportChristian Couder, Sep 16, 2017
  32. 27/40 Add t0450 to test 'get_direct' mechanismChristian Couder, Sep 16, 2017
  33. 15/40 Add GIT_NO_EXTERNAL_ODB env variableChristian Couder, Sep 16, 2017
  34. 13/40 external-odb: accept only blobs for nowChristian Couder, Sep 16, 2017
  35. 17/40 lib-httpd: pass config file to start_httpd()Christian Couder, Sep 16, 2017
  36. 12/40 external odb: add 'put_raw_obj' supportChristian Couder, Sep 16, 2017
  37. 06/40 t0021/rot13-filter: add capability functionsChristian Couder, Sep 16, 2017
  38. 09/40 Add initial external odb supportChristian Couder, Sep 16, 2017
  39. Jonathan TanSep 19, 2017
  40. Christian CouderSep 27, 2017
  41. Jonathan TanSep 29, 2017
  42. Ben PeartOct 2, 2017
  43. Christian CouderOct 3, 2017
  44. Jonathan TanOct 4, 2017
  45. 03/40 t0021/rot13-filter: improve 'if .. elsif .. else' styleChristian Couder, Sep 16, 2017
  46. 08/40 sha1_file: prepare for external odbsChristian Couder, Sep 16, 2017
  47. 02/40 t0021/rot13-filter: refactor packet reading functionsChristian Couder, Sep 16, 2017
  48. Ben PeartOct 2, 2017
  49. Christian CouderOct 3, 2017

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.