Re: [PATCH] mailmap: update brandon williams's email address
On Fri, Dec 7, 2018 at 10:08 PM Junio C Hamano wrote: > > Stefan Beller writes: > > > On Fri, Dec 7, 2018 at 1:40 PM Jonathan Nieder wrote: > >> > >> Brandon Williams wrote: > >> > >> > Signed-off-by: Brandon Williams > >> > --- > >> > .mailmap | 1 + > >> > 1 file changed, 1 insertion(+) > >> > >> I can confirm that this is indeed the same person. > > > > What would be more of interest is why we'd be interested in this patch > > as there is no commit/patch sent by Brandon with this email in gits history. > > Once I "git am" the message that began this thread, there will be a > commit under this new ident, so that would be somewhat a moot point. > > If this were "Jonathan asked Brandon if we want to record an address > we can reach him in our .mailmap file and sent a patch to add one", > then the story is different, and I tend to agree with you that such > a patch is more or less pointless. That's not the purpose of the > mailmap file. > Turns out this is exactly the reason :) I've had a couple of people reach out to me asking me to do this because CCing my old email bounces and they've wanted my input/comments on something related to work I've done. If that's not the intended purpose then please ignore this patch > Not until git-send-email learns to use that file to rewrite > To/cc/etc to the "canonical" addresses, anyway ;-) > > I am not sure if there are people whose "canonical" address to be > used as the author is not necessarily the best address they want to > get their e-mails at, though. If we can be reasonably sure that the > set of such people is empty, then people can take the above mention > about send-email as a hint about a low-hanging fruit ;-) > > Thanks. > >
[PATCH] mailmap: update brandon williams's email address
Signed-off-by: Brandon Williams --- .mailmap | 1 + 1 file changed, 1 insertion(+) diff --git a/.mailmap b/.mailmap index eb7b5fc7b..247a3deb7 100644 --- a/.mailmap +++ b/.mailmap @@ -27,6 +27,7 @@ Ben Walton Benoit Sigoure Bernt Hansen Brandon Casey +Brandon Williams brian m. carlson brian m. carlson Bryan Larsen -- 2.19.1
Re: [PATCH 2/2] submodule: munge paths to submodule git directories
On 08/29, Stefan Beller wrote: > On Wed, Aug 29, 2018 at 2:09 PM Jonathan Nieder wrote: > > > > Jeff King wrote: > > > On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote: > > > > >> Yeah, then let's just convert '/' with as little overhead as possible. > > > > > > Do you care about case-folding issues (e.g., submodules "FOO" and "foo" > > > colliding)? > > > > > > I'm OK if the answer is "no", but if you do want to deal with it, the > > > time is probably now. > > > > Have we rejected the config approach? > > I did not reject that approach, but am rather waiting for patches. ;-) Note I did send out a patch using this approach, so no need to wait any longer! :D https://public-inbox.org/git/20180816181940.46114-1-bmw...@google.com/ -- Brandon Williams
[PATCH] submodule: add config for where gitdirs are located
Introduce the config "submodule..gitdirpath" which is used to
indicate where a submodule's gitdir is located inside of a repository's
"modules" directory.
Signed-off-by: Brandon Williams
---
Maybe something like this on top? Do you think we should disallow "../"
in this config, even though it is a repository local configuration and
not shipped in .gitmodules?
submodule.c| 13 -
t/t7400-submodule-basic.sh | 10 ++
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/submodule.c b/submodule.c
index 4854d88ce8..0cb00a9f24 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1966,16 +1966,27 @@ void submodule_name_to_gitdir(struct strbuf *buf,
struct repository *r,
const char *submodule_name)
{
size_t modules_len;
+ char *key;
+ char *gitdir_path;
strbuf_git_common_path(buf, r, "modules/");
modules_len = buf->len;
- strbuf_addstr(buf, submodule_name);
+
+ key = xstrfmt("submodule.%s.gitdirpath", submodule_name);
+ if (!repo_config_get_string(r, key, _path)) {
+ strbuf_addstr(buf, gitdir_path);
+ free(key);
+ free(gitdir_path);
+ return;
+ }
+ free(key);
/*
* If the submodule gitdir already exists using the old-fashioned
* location (which uses the submodule name as-is, without munging it)
* then return that.
*/
+ strbuf_addstr(buf, submodule_name);
if (!access(buf->buf, F_OK))
return;
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 963693332c..1555329a2f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1351,6 +1351,16 @@ test_expect_success 'resolve submodule gitdir in
superprojects modules directory
$(git -C superproject rev-parse --git-common-dir)/modules/sub/module
EOF
git -C superproject submodule--helper gitdir "sub/module" >actual &&
+ test_cmp expect actual &&
+
+ # Test using "submodule..gitdirpath" config for where the
submodules
+ # gitdir is located inside the superprojecs "modules" directory
+ mv superproject/.git/modules/sub/module
superproject/.git/modules/submodule &&
+ cat >expect <<-EOF &&
+ $(git -C superproject rev-parse --git-common-dir)/modules/submodule
+ EOF
+ git -C superproject config "submodule.sub/module.gitdirpath"
"submodule" &&
+ git -C superproject submodule--helper gitdir "sub/module" >actual &&
test_cmp expect actual
'
--
2.18.0.865.gffc8e1a3cd6-goog
Re: [PATCH 2/2] submodule: munge paths to submodule git directories
On 08/15, Jonathan Nieder wrote:
> Stefan Beller wrote:
> > Jonathan Nieder wrote:
>
> >> All at the cost of recording a little configuration somewhere. If we
> >> want to decrease the configuration, we can avoid recording it there in
> >> the easy cases (e.g. when name == gitdirname). That's "just" an
> >> optimization.
> >
> > Sounds good, but gerrit for example would not take advantage of such
> > optimisation as they have slashes in their submodules. :-(
> > I wonder if we can optimize further and keep slashes if there is
> > no conflict (as then name == gitdirname, so it can be optimized).
>
> One possibility would be to treat gsub("/", "%2f") as another of the
> easy cases.
>
> [...]
> >> And then we have the ability later to handle all the edge cases we
> >> haven't handled yet today:
> >>
> >> - sharding when the number of submodules is too large
> >> - case-insensitive filesystems
> >> - path name length limits
> >> - different sets of filesystem-special characters
> >>
> >> Sane?
Seems like a sensible thing to do. Let me work up some patches to
implement this using config primarily and these other schemes as
fallbacks.
> >
> > I'll keep thinking about it.
>
> Thanks.
>
> > FYI: the reduction in configuration was just sent out.
>
> https://public-inbox.org/git/20180816023100.161626-1-sbel...@google.com/
> for those following along.
>
> Ciao,
> Jonathan
--
Brandon Williams
Re: Syncing HEAD
On 08/14, Stefan Beller wrote: > On Tue, Aug 14, 2018 at 1:09 PM Christian Couder > wrote: > > > > Hi, > > > > When cloning with --mirror, the clone gets its HEAD initialized with > > the value HEAD has in its origin remote. After that if HEAD changes in > > origin there is no simple way to sync HEAD at the same time as the > > refs are synced. > > > > It looks like the simplest way to sync HEAD is: > > > > 1) git remote show origin > > 2) parse "HEAD branch: XXX" from the output of the above command > > 3) git symbolic-ref HEAD refs/heads/XXX > > > > It looks like it would be quite easy to add an option to `fetch` to > > sync HEAD at the same time as regular refs are synced because every > > fetch from an origin that uses a recent Git contains something like: > > > > 19:55:39.304976 pkt-line.c:80 packet: git< > > HEAD\0multi_ack thin-pack side-band side-band-64k ofs-delta shallow > > deepen-since deepen-not deepen-relative no-progress include-tag > > multi_ack_detailed no-done symref=HEAD:refs/heads/test-1 > > agent=git/2.18.0 > > > > which in this example shows that HEAD is a symref to refs/heads/test-1 > > in origin. > > > > Is there a reason why no such option already exists? Would it makes > > sense to add one? Is there any reason why it's not a good idea? Or am > > I missing something? > > I think it is a great idea to add that. IIRC there was some talk when > designing protocol v2 on how fetching of symrefs could be added later > on in the protocol, which is why I cc'd Brandon who did the work there. Actually the functionality for fetching symrefs already exists (when using protocol v2 of course). Despite this functionality existing its not being used right now. When performing a v2 fetch the first thing that a client does is request the list of refs (by doing an ls-refs request). The output from ls-refs (if asked) will included information about each ref including if they are a symref and what ref they resolve to. So really we just need to plumb that information through fetch to actually update HEAD, or even update other symrefs which exist on the server. > > > I am asking because GitLab uses HEAD in the bare repos it manages to > > store the default branch against which the Merge Requests (same thing > > as Pull Requests on GitHub) are created. > > > > So when people want to keep 2 GitLab hosted repos in sync, GitLab > > needs to sync HEADs too, not just the refs. > > > > I think this could be useful to other setups than GitLab though. > > As said, I can see how that is useful; I recently came across some > HEAD bug related to submodules, and there we'd also have the application. > > git clone --recurse-submodules file://... > > might clone the submodules that are in detached HEAD, which is totally > not a long term viable good HEAD, so a subsequent fetch might want > to change the detached HEAD in submodules or re-affix it to branches. > > Unrelated/extended: I think it would be cool to mirror a repository even > more, e.g. it would be cool to be able to fetch (if configured as allowed) > the remote reflog, (not to be confused with you local reflog of the remote). > I think that work would be enabled once reftables are available, which you > have an eye on? -- Brandon Williams
Re: [PATCH 2/2] submodule: munge paths to submodule git directories
On 08/09, Jeff King wrote: > On Wed, Aug 08, 2018 at 03:33:23PM -0700, Brandon Williams wrote: > > > Commit 0383bbb901 (submodule-config: verify submodule names as paths, > > 2018-04-30) introduced some checks to ensure that submodule names don't > > include directory traversal components (e.g. "../"). > > > > This addresses the vulnerability identified in 0383bbb901 but the root > > cause is that we use submodule names to construct paths to the > > submodule's git directory. What we really should do is munge the > > submodule name before using it to construct a path. > > > > Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url > > encoding it) before using it to build a path to the submodule's gitdir. > > I like this approach very much, and I think using url encoding is much > better than an opaque hash (purely because it makes debugging and > inspection saner). > > Two thoughts, though: > > > + modules_len = buf->len; > > strbuf_addstr(buf, submodule_name); > > + > > + /* > > +* If the submodule gitdir already exists using the old-fashioned > > +* location (which uses the submodule name as-is, without munging it) > > +* then return that. > > +*/ > > + if (!access(buf->buf, F_OK)) > > + return; > > I think this backwards-compatibility is necessary to avoid pain. But > until it goes away, I don't think this is helping the vulnerability from > 0383bbb901. Because there the issue was that the submodule name pointed > back into the working tree, so this access() would find the untrusted > working tree code and say "ah, an old-fashioned name!". > > In theory a fresh clone could set a config option for "I only speak > use new-style modules". And there could even be a conversion program > that moves the modules as appropriate, fixes up the .git files in the > working tree, and then sets that config. > > In fact, I think that config option _could_ be done by bumping > core.repositoryformatversion and then setting extensions.submodulenames > to "url" or something. Then you could never run into the confusing case > where you have a clone done by a new version of git (using new-style > names), but using an old-style version gets confused because it can't > find the module directories (instead, it would barf and say "I don't > know about that extension"). > > I don't know if any of that is worth it, though. We already fixed the > problem from 0383bbb901. There may be a _different_ "break out of the > modules directory" vulnerability, but since we disallow ".." it's hard > to see what it would be (the best I could come up with is maybe pointing > one module into the interior of another module, but I think you'd have > to trouble overwriting anything useful). > > And while an old-style version of Git being confused might be annoying, > I suspect that bumping the repository version would be even _more_ > annoying, because it would hit every command, not just ones that try to > touch those submodules. Oh I know that this doesn't help with that vulnerability. As you've said we fix it and now disallow ".." at the submodule-config level so really this path is simply about using what we get out of submodule-config in a more sane manor. > > > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > > index 2c2c97e144..963693332c 100755 > > --- a/t/t7400-submodule-basic.sh > > +++ b/t/t7400-submodule-basic.sh > > @@ -933,7 +933,7 @@ test_expect_success 'recursive relative submodules stay > > relative' ' > > cd clone2 && > > git submodule update --init --recursive && > > echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect && > > - echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" > > >./sub3/dirdir/subsub/.git_expect > > + echo "gitdir: > > ../../../.git/modules/sub3/modules/dirdir%2fsubsub" > > >./sub3/dirdir/subsub/.git_expect > > One interesting thing about url-encoding is that it's not one-to-one. > This case could also be %2F, which is a different file (on a > case-sensitive filesystem). I think "%20" and "+" are similarly > interchangeable. > > If we were decoding the filenames, that's fine. The round-trip is > lossless. > > But that's not quite how the new code behaves. We encode the input and > then check to see if it matches an encoding we previously performed. So > if our urlencode routines ever change, this will
Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out
more details about this check, see
> + # builtin/submodule--helper.c::module_config()
> + if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules >
> /dev/null 2>&1
> + then
> + die "$(eval_gettext "please make sure that the .gitmodules
> file in the current branch is checked out")"
> + fi
> +
> if test -n "$reference_path"
> then
> is_absolute_path "$reference_path" ||
> diff --git a/submodule-config.c b/submodule-config.c
> index b7ef055c63..088dabb56f 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -1,4 +1,5 @@
> #include "cache.h"
> +#include "dir.h"
> #include "repository.h"
> #include "config.h"
> #include "submodule-config.h"
> @@ -603,8 +604,19 @@ static void submodule_cache_check_init(struct repository
> *repo)
> static void config_from_gitmodules(config_fn_t fn, struct repository *repo,
> void *data)
> {
> if (repo->worktree) {
> - char *file = repo_worktree_path(repo, GITMODULES_FILE);
> - git_config_from_file(fn, file, data);
> + struct git_config_source config_source = { 0 };
> + const struct config_options opts = { 0 };
> + struct object_id oid;
> + char *file;
> +
> + file = repo_worktree_path(repo, GITMODULES_FILE);
> + if (file_exists(file))
> + config_source.file = file;
> + else if (get_oid(GITMODULES_HEAD, ) >= 0)
> + config_source.blob = GITMODULES_HEAD;
> +
> + config_with_options(fn, data, _source, );
> +
> free(file);
> }
> }
> diff --git a/t/t7416-submodule-sparse-gitmodules.sh
> b/t/t7416-submodule-sparse-gitmodules.sh
> new file mode 100755
> index 00..5341e9b012
> --- /dev/null
> +++ b/t/t7416-submodule-sparse-gitmodules.sh
> @@ -0,0 +1,90 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2018 Antonio Ospite
> +#
> +
> +test_description='Test reading/writing .gitmodules when not in the working
> tree
> +
> +This test verifies that, when .gitmodules is in the current branch but is not
> +in the working tree reading from it still works but writing to it does not.
> +
> +The test setup uses a sparse checkout, however the same scenario can be set
> up
> +also by committing .gitmodules and then just removing it from the filesystem.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'sparse checkout setup which hides .gitmodules' '
> + echo file > file &&
> + git add file &&
> + test_tick &&
> + git commit -m upstream &&
> + git clone . super &&
> + git clone super submodule &&
> + git clone super new_submodule &&
> + (cd super &&
> + git submodule add ../submodule &&
> + test_tick &&
> + git commit -m submodule &&
> + cat >.git/info/sparse-checkout <<\EOF &&
> +/*
> +!/.gitmodules
> +EOF
> + git config core.sparsecheckout true &&
> + git read-tree -m -u HEAD &&
> + test_path_is_missing .gitmodules
> + )
> +'
> +
> +test_expect_success 'reading gitmodules config file when it is not checked
> out' '
> + (cd super &&
> + echo "../submodule" >expected &&
> + git submodule--helper config submodule.submodule.url >actual &&
> + test_cmp expected actual
> + )
> +'
> +
> +test_expect_success 'not writing gitmodules config file when it is not
> checked out' '
> + (cd super &&
> + test_must_fail git submodule--helper config
> submodule.submodule.url newurl
> + )
> +'
> +
> +test_expect_success 'initialising submodule when the gitmodules config is
> not checked out' '
> + (cd super &&
> + git submodule init
> + )
> +'
> +
> +test_expect_success 'showing submodule summary when the gitmodules config is
> not checked out' '
> + (cd super &&
> + git submodule summary
> + )
> +'
> +
> +test_expect_success 'updating submodule when the gitmodules config is not
> checked out' '
> + (cd submodule &&
> + echo file2 >file2 &&
> + git add file2 &&
> + git commit -m "add file2 to submodule"
> + ) &&
> + (cd super &&
> + git submodule update
> + )
> +'
> +
> +test_expect_success 'not adding submodules when the gitmodules config is not
> checked out' '
> + (cd super &&
> + test_must_fail git submodule add ../new_submodule
> + )
> +'
> +
> +# This test checks that the previous "git submodule add" did not leave the
> +# repository in a spurious state when it failed.
> +test_expect_success 'init submodule still works even after the previous add
> failed' '
> + (cd super &&
> + git submodule init
> + )
> +'
> +
> +test_done
> --
> 2.18.0
>
--
Brandon Williams
Re: [PATCH v3 5/7] submodule: use the 'submodule--helper config' command
On 08/14, Antonio Ospite wrote:
> Use the 'submodule--helper config' command in git-submodules.sh to avoid
> referring explicitly to .gitmodules by the hardcoded file path.
>
> This makes it possible to access the submodules configuration in a more
> controlled way.
>
> Signed-off-by: Antonio Ospite
Looks great. I also like you're approach of introducing the new API and
testing it in one commit, and then using it in the next. Makes the
patch set very easy to follow.
> ---
> git-submodule.sh | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 8b5ad59bde..ff258e2e8c 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -72,7 +72,7 @@ get_submodule_config () {
> value=$(git config submodule."$name"."$option")
> if test -z "$value"
> then
> - value=$(git config -f .gitmodules submodule."$name"."$option")
> + value=$(git submodule--helper config
> submodule."$name"."$option")
> fi
> printf '%s' "${value:-$default}"
> }
> @@ -283,11 +283,11 @@ or you are unsure what this means choose another name
> with the '--name' option."
> git add --no-warn-embedded-repo $force "$sm_path" ||
> die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
>
> - git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
> - git config -f .gitmodules submodule."$sm_name".url "$repo" &&
> + git submodule--helper config submodule."$sm_name".path "$sm_path" &&
> + git submodule--helper config submodule."$sm_name".url "$repo" &&
> if test -n "$branch"
> then
> - git config -f .gitmodules submodule."$sm_name".branch "$branch"
> + git submodule--helper config submodule."$sm_name".branch
> "$branch"
> fi &&
> git add --force .gitmodules ||
> die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
> --
> 2.18.0
>
--
Brandon Williams
Re: [PATCH v3 4/7] submodule--helper: add a new 'config' subcommand
On 08/14, Antonio Ospite wrote:
> Add a new 'config' subcommand to 'submodule--helper', this extra level
> of indirection makes it possible to add some flexibility to how the
> submodules configuration is handled.
>
> Signed-off-by: Antonio Ospite
> ---
> builtin/submodule--helper.c | 14 ++
> new | 0
Looks like you may have accidentally left in an empty file "new" it should
probably be removed from this commit before it gets merged.
Aside from that this patch looks good. I've recently run into issues
where we don't have a good enough abstraction layer around how we
interact with submodules so I'm glad we're moving towards better
abstractions :)
> t/t7411-submodule-config.sh | 26 ++
> 3 files changed, 40 insertions(+)
> create mode 100644 new
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a3c4564c6c..7481d03b63 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2029,6 +2029,19 @@ static int connect_gitdir_workingtree(int argc, const
> char **argv, const char *p
> return 0;
> }
>
> +static int module_config(int argc, const char **argv, const char *prefix)
> +{
> + /* Equivalent to ACTION_GET in builtin/config.c */
> + if (argc == 2)
> + return print_config_from_gitmodules(argv[1]);
> +
> + /* Equivalent to ACTION_SET in builtin/config.c */
> + if (argc == 3)
> + return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> +
> + die("submodule--helper config takes 1 or 2 arguments: name [value]");
> +}
> +
> #define SUPPORT_SUPER_PREFIX (1<<0)
>
> struct cmd_struct {
> @@ -2057,6 +2070,7 @@ static struct cmd_struct commands[] = {
> {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> {"is-active", is_active, 0},
> {"check-name", check_name, 0},
> + {"config", module_config, 0},
> };
>
> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/new b/new
> new file mode 100644
> index 00..e69de29bb2
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index c6b6cf6fae..4afb6f152e 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -142,4 +142,30 @@ test_expect_success 'error in history in
> fetchrecursesubmodule lets continue' '
> )
> '
>
> +test_expect_success 'reading submodules config with "submodule--helper
> config"' '
> + (cd super &&
> + echo "../submodule" >expected &&
> + git submodule--helper config submodule.submodule.url >actual &&
> + test_cmp expected actual
> + )
> +'
> +
> +test_expect_success 'writing submodules config with "submodule--helper
> config"' '
> + (cd super &&
> + echo "new_url" >expected &&
> + git submodule--helper config submodule.submodule.url "new_url"
> &&
> + git submodule--helper config submodule.submodule.url >actual &&
> + test_cmp expected actual
> + )
> +'
> +
> +test_expect_success 'overwriting unstaged submodules config with
> "submodule--helper config"' '
> + (cd super &&
> + echo "newer_url" >expected &&
> + git submodule--helper config submodule.submodule.url
> "newer_url" &&
> + git submodule--helper config submodule.submodule.url >actual &&
> + test_cmp expected actual
> + )
> +'
> +
> test_done
> --
> 2.18.0
>
--
Brandon Williams
Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up
On 08/14, Antonio Ospite wrote: > Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with > invalid lines in .gitmodules but then only the second commit is removed. > > This may affect future subsequent tests if they assume that the > .gitmodules file has no errors. > > Remove both the commits as soon as they are not needed anymore. > > The error introduced in test 5 is also required by test 6, so the two > commits from above are removed respectively in tests 6 and 8. Thanks for cleaning this up. We seem to have a habit for leaving testing state around for longer than is necessary which makes it a bit more difficult to read and understand when looking at it later. What would really be nice is if each test was self-contained...course that would take a herculean effort to realize in our testsuite so I'm not suggesting you do that :) > > Signed-off-by: Antonio Ospite > --- > t/t7411-submodule-config.sh | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh > index 0bde5850ac..c6b6cf6fae 100755 > --- a/t/t7411-submodule-config.sh > +++ b/t/t7411-submodule-config.sh > @@ -98,6 +98,9 @@ test_expect_success 'error in one submodule config lets > continue' ' > ' > > test_expect_success 'error message contains blob reference' ' > + # Remove the error introduced in the previous test. > + # It is not needed in the following tests. > + test_when_finished "git -C super reset --hard HEAD^" && > (cd super && > sha1=$(git rev-parse HEAD) && > test-tool submodule-config \ > @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' ' > ' > > test_expect_success 'error in history in fetchrecursesubmodule lets > continue' ' > + test_when_finished "git -C super reset --hard HEAD^" && > (cd super && > git config -f .gitmodules \ > submodule.submodule.fetchrecursesubmodules blabla && > @@ -134,8 +138,7 @@ test_expect_success 'error in history in > fetchrecursesubmodule lets continue' ' > HEAD b \ > HEAD submodule \ > >actual && > - test_cmp expect_error actual && > - git reset --hard HEAD^ > + test_cmp expect_error actual > ) > ' > > -- > 2.18.0 > -- Brandon Williams
Re: [PATCH 00/24] Kill the_index part3
On 08/13, Nguyễn Thái Ngọc Duy wrote: > This is the third part of killing the_index (at least outside > builtin/). Part 1 [1] is dropped. Part 2 is nd/no-extern on 'pu'. This > part is built on top of nd/no-extern. > > This series would actually break 'pu' because builtin/stash.c uses > three functions that are updated here. So we would need something like > the following patch to make it build again. > > I don't know if that adds too much work on Junio. If it does, I guess > I'll hold this off for a while until builtin/stash.c gets merged > because reordering these patches, pushing the patches that break > stash.c away, really takes a lot of work. > > [1] https://public-inbox.org/git/20180616054157.32433-1-pclo...@gmail.com/ I went through and found this to be a pleasant read and hopefully others agree with the approach this series took vs what your part 1 did so that we can get this change in. -- Brandon Williams
Re: [PATCH 16/24] attr: remove index from git_attr_set_direction()
x); > + git_attr_set_direction(GIT_ATTR_CHECKOUT); > > if (should_update_submodules() && o->update && !o->dry_run) > load_gitmodules_file(index, NULL); > @@ -413,7 +413,7 @@ static int check_updates(struct unpack_trees_options *o) > stop_progress(); > errs |= finish_delayed_checkout(); > if (o->update) > - git_attr_set_direction(GIT_ATTR_CHECKIN, NULL); > + git_attr_set_direction(GIT_ATTR_CHECKIN); > return errs != 0; > } > > -- > 2.18.0.1004.g6639190530 > -- Brandon Williams
Re: [PATCH 05/24] dir.c: remove an implicit dependency on the_index in pathspec code
On 08/13, Nguyễn Thái Ngọc Duy wrote:
> diff --git a/dir.c b/dir.c
> index 29fbbd48c8..e25aed013b 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -276,12 +276,13 @@ static int do_read_blob(const struct object_id *oid,
> struct oid_stat *oid_stat,
> #define DO_MATCH_DIRECTORY (1<<1)
> #define DO_MATCH_SUBMODULE (1<<2)
>
> -static int match_attrs(const char *name, int namelen,
> +static int match_attrs(const struct index_state *istate,
> +const char *name, int namelen,
> const struct pathspec_item *item)
> {
> int i;
>
> - git_check_attr(_index, name, item->attr_check);
> + git_check_attr(istate, name, item->attr_check);
> for (i = 0; i < item->attr_match_nr; i++) {
> const char *value;
> int matched;
> @@ -318,7 +319,8 @@ static int match_attrs(const char *name, int namelen,
> *
> * It returns 0 when there is no match.
> */
> -static int match_pathspec_item(const struct pathspec_item *item, int prefix,
> +static int match_pathspec_item(const struct index_state *istate,
> +const struct pathspec_item *item, int prefix,
> const char *name, int namelen, unsigned flags)
> {
> /* name/namelen has prefix cut off by caller */
> @@ -358,7 +360,7 @@ static int match_pathspec_item(const struct pathspec_item
> *item, int prefix,
> strncmp(item->match, name - prefix, item->prefix))
> return 0;
>
> - if (item->attr_match_nr && !match_attrs(name, namelen, item))
> + if (item->attr_match_nr && !match_attrs(istate, name, namelen, item))
> return 0;
Yuck, all of this just because I added the ability to match against
attrs with pathspecs. Part of me wonders if it would be better to put a
pointer to the needed istate in the pathspec struct...but then I can
think of a ton of reasons why that wouldn't be good either.
So yes I think this is probably the right approach, I'm just sorry I
made it this messy :/
--
Brandon Williams
Re: [PATCH 03/24] attr: remove an implicit dependency on the_index
const char *path)
>
> if (!check)
> check = attr_check_initl("conflict-marker-size", NULL);
> - if (!git_check_attr(path, check) && check->items[0].value) {
> + if (!git_check_attr(_index, path, check) && check->items[0].value) {
> marker_size = atoi(check->items[0].value);
> if (marker_size <= 0)
> marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> diff --git a/userdiff.c b/userdiff.c
> index 36af25e7f9..f3f4be579c 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -278,7 +278,7 @@ struct userdiff_driver *userdiff_find_by_path(const char
> *path)
> check = attr_check_initl("diff", NULL);
> if (!path)
> return NULL;
> - if (git_check_attr(path, check))
> + if (git_check_attr(_index, path, check))
> return NULL;
>
> if (ATTR_TRUE(check->items[0].value))
> diff --git a/ws.c b/ws.c
> index a07caedd5a..5b67b426e7 100644
> --- a/ws.c
> +++ b/ws.c
> @@ -78,7 +78,7 @@ unsigned whitespace_rule(const char *pathname)
> if (!attr_whitespace_rule)
> attr_whitespace_rule = attr_check_initl("whitespace", NULL);
>
> - if (!git_check_attr(pathname, attr_whitespace_rule)) {
> + if (!git_check_attr(_index, pathname, attr_whitespace_rule)) {
> const char *value;
>
> value = attr_whitespace_rule->items[0].value;
> --
> 2.18.0.1004.g6639190530
>
--
Brandon Williams
Re: [PATCH 6/7] submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree
On 08/10, Stefan Beller wrote:
> > > + cfg_file = xstrfmt("%s/config", subrepo.gitdir);
> >
> > As I mentioned here:
> > https://public-inbox.org/git/20180807230637.247200-1-bmw...@google.com/T/#t
> >
> > This lines should probably be more like:
> >
> > cfg_file = repo_git_path(, "config");
> >
>
> Why? You did not mention the benefits for writing it this way
> here or on the reference. Care to elaborate?
Its more future proof especially because we have the difference bettwen
commondir and gitdir for worktrees. Using the "repo_git_path" function
handles path rewritting when using worktrees. Here (when working with
worktrees) "subrepo.gitdir" refers to the worktree specific gitdir while
"subrepo.commondir" refers to the global common gitdir where the
repository config actually lives.
--
Brandon Williams
Re: [PATCH 6/7] submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree
odule--helper name "$sm_path") || exit
> if ! test -z "$update"
> then
> @@ -577,11 +579,6 @@ cmd_update()
> die "$(eval_gettext "Unable to find current
> \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
> fi
>
> - if ! $(git config -f "$(git rev-parse
> --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
> - then
> - git submodule--helper connect-gitdir-workingtree
> "$name" "$sm_path"
> - fi
> -
> if test "$subsha1" != "$sha1" || test -n "$force"
> then
> subforce=$force
> --
> 2.18.0.132.g195c49a2227
>
--
Brandon Williams
Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs
On 08/10, Junio C Hamano wrote:
> Brandon Williams writes:
>
> > Introduce a helper function "submodule_name_to_gitdir()" (and the
> > submodule--helper subcommand "gitdir") which constructs a path to a
> > submodule's gitdir, located in the provided repository's "modules"
> > directory.
> >
> > This consolidates the logic needed to build up a path into a
> > repository's "modules" directory, abstracting away the fact that
> > submodule git directories are stored in a repository's common gitdir.
> > This makes it easier to adjust how submodules gitdir are stored in the
> > "modules" directory in a future patch.
> >
> > Signed-off-by: Brandon Williams
> > ---
> > ...
> > @@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const
> > char **argv, const char *p
> > name = argv[1];
> > path = argv[2];
> >
> > - strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
> > + submodule_name_to_gitdir(, the_repository, name);
> > sm_gitdir = absolute_pathdup(sb.buf);
> >
> > connect_work_tree_and_git_dir(path, sm_gitdir, 0);
>
> This function goes away with 1c866b98 ("submodule--helper: replace
> connect-gitdir-workingtree by ensure-core-worktree", 2018-08-03) in
> sb/submodule-update-in-c topic. git-submodule.sh has simlar
> conflicts.
>
> I guess its replacement function does not care as deeply as its
> predecessor used to about where the submodule's $GIT_DIR is, so the
> correct resolution may be just to ignore the change made to this
> caller to the new name-to-gitdir function.
Well that patch still cares about where the gitdir is except it
initializes a "struct repository" for the submodule and then builds a
path to the config using:
cfg_file = xstrfmt("%s/config", subrepo.gitdir);
hmm...I didn't get a chance to look at that series but that line looks
wrong. It probably should be more like:
cfg_file = repo_git_path(, "config");
I'll go comment on that series.
>
> It would have been nicer to see a bit better inter-developer
> coordination, especially between two who sit practically next to
> each other ;-)
>
> Thanks.
--
Brandon Williams
Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs
On 08/08, Stefan Beller wrote:
> On Wed, Aug 8, 2018 at 3:33 PM Brandon Williams wrote:
> >
> > Introduce a helper function "submodule_name_to_gitdir()" (and the
> > submodule--helper subcommand "gitdir") which constructs a path to a
> > submodule's gitdir, located in the provided repository's "modules"
> > directory.
>
> Makes sense.
>
> >
> > This consolidates the logic needed to build up a path into a
> > repository's "modules" directory, abstracting away the fact that
> > submodule git directories are stored in a repository's common gitdir.
> > This makes it easier to adjust how submodules gitdir are stored in the
> > "modules" directory in a future patch.
>
> and yet, all places that we touch were and still are broken for old-style
> submodules that have their git directory inside the working tree?
> Do we need to pay attention to those, too?
This series only tries to address the issues with submodules stored in
$GITDIR/modules/ and places in our codebase that explicitly reference
submodules stored there.
For those old-old-style submodules, wouldn't the absorb submodule
functions handle that migration?
>
>
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 8b5ad59bde..053747d290 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
>
> > @@ -577,7 +578,7 @@ cmd_update()
> > die "$(eval_gettext "Unable to find current
> > \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
> > fi
> >
> > - if ! $(git config -f "$(git rev-parse
> > --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
> > + if ! $(git config -f "$(git submodule--helper gitdir
> > "$name")/config" core.worktree) 2>/dev/null
>
> This will collide with origin/sb/submodule-update-in-c specifically
> 1c866b9831d (submodule--helper: replace connect-gitdir-workingtree
> by ensure-core-worktree, 2018-08-03), but as that removes these lines,
> it should be easy to resolve the conflict.
--
Brandon Williams
[PATCH 2/2] submodule: munge paths to submodule git directories
Commit 0383bbb901 (submodule-config: verify submodule names as paths,
2018-04-30) introduced some checks to ensure that submodule names don't
include directory traversal components (e.g. "../").
This addresses the vulnerability identified in 0383bbb901 but the root
cause is that we use submodule names to construct paths to the
submodule's git directory. What we really should do is munge the
submodule name before using it to construct a path.
Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url
encoding it) before using it to build a path to the submodule's gitdir.
Signed-off-by: Brandon Williams
---
submodule.c | 14 ++
t/t7400-submodule-basic.sh | 32 +++-
t/t7406-submodule-update.sh | 21 ++---
3 files changed, 51 insertions(+), 16 deletions(-)
diff --git a/submodule.c b/submodule.c
index 24eced34e7..4854d88ce8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1965,6 +1965,20 @@ int submodule_to_gitdir(struct strbuf *buf, const char
*submodule)
void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,
const char *submodule_name)
{
+ size_t modules_len;
+
strbuf_git_common_path(buf, r, "modules/");
+ modules_len = buf->len;
strbuf_addstr(buf, submodule_name);
+
+ /*
+* If the submodule gitdir already exists using the old-fashioned
+* location (which uses the submodule name as-is, without munging it)
+* then return that.
+*/
+ if (!access(buf->buf, F_OK))
+ return;
+
+ strbuf_setlen(buf, modules_len);
+ strbuf_addstr_urlencode(buf, submodule_name, 1);
}
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2c2c97e144..963693332c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -933,7 +933,7 @@ test_expect_success 'recursive relative submodules stay
relative' '
cd clone2 &&
git submodule update --init --recursive &&
echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect &&
- echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub"
>./sub3/dirdir/subsub/.git_expect
+ echo "gitdir:
../../../.git/modules/sub3/modules/dirdir%2fsubsub"
>./sub3/dirdir/subsub/.git_expect
) &&
test_cmp clone2/sub3/.git_expect clone2/sub3/.git &&
test_cmp clone2/sub3/dirdir/subsub/.git_expect
clone2/sub3/dirdir/subsub/.git
@@ -1324,4 +1324,34 @@ test_expect_success 'recursive clone respects -q' '
test_must_be_empty actual
'
+test_expect_success 'resolve submodule gitdir in superprojects modules
directory' '
+ test_when_finished "rm -rf superproject submodule" &&
+
+ # Create a superproject with a submodule which contains a "/"
+ test_create_repo submodule &&
+ test_commit -C submodule one &&
+ test_create_repo superproject &&
+ git -C superproject submodule add ../submodule sub/module &&
+ git -C superproject commit -m "add submodule" &&
+
+ # "/" characters in submodule names are properly urlencoded before
+ # being used to construct a path to the submodules gitdir.
+ cat >expect <<-EOF &&
+ $(git -C superproject rev-parse --git-common-dir)/modules/sub%2fmodule
+ EOF
+ git -C superproject submodule--helper gitdir "sub/module" >actual &&
+ test_cmp expect actual &&
+ test_path_is_dir "superproject/.git/modules/sub%2fmodule" &&
+
+ # Test the old-fashioned way of storing submodules in the
+ # "modules" directory by directly renaming the submodules gitdir
+ mkdir superproject/.git/modules/sub/ &&
+ mv superproject/.git/modules/sub%2fmodule
superproject/.git/modules/sub/module &&
+ cat >expect <<-EOF &&
+ $(git -C superproject rev-parse --git-common-dir)/modules/sub/module
+ EOF
+ git -C superproject submodule--helper gitdir "sub/module" >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a72..fb744c5c39 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -777,12 +777,8 @@ test_expect_success 'submodule add places git-dir in
superprojects git-dir' '
(cd super &&
mkdir deeper &&
git submodule add ../submodule deeper/submodule &&
-(cd deeper/submodule &&
- git log > ../../expected
-) &&
-(cd .git/modules/deeper/submodule &&
- git lo
[PATCH 1/2] submodule: create helper to build paths to submodule gitdirs
Introduce a helper function "submodule_name_to_gitdir()" (and the
submodule--helper subcommand "gitdir") which constructs a path to a
submodule's gitdir, located in the provided repository's "modules"
directory.
This consolidates the logic needed to build up a path into a
repository's "modules" directory, abstracting away the fact that
submodule git directories are stored in a repository's common gitdir.
This makes it easier to adjust how submodules gitdir are stored in the
"modules" directory in a future patch.
Signed-off-by: Brandon Williams
---
builtin/submodule--helper.c | 28 +++--
dir.c| 2 +-
git-submodule.sh | 7 +++--
repository.c | 3 +-
submodule.c | 53
submodule.h | 7 +
t/t7410-submodule-checkout-to.sh | 6 ++--
7 files changed, 75 insertions(+), 31 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a3c4564c6c..5bfd2d0be9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -906,6 +906,21 @@ static int module_name(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_gitdir(int argc, const char **argv, const char *prefix)
+{
+ struct strbuf gitdir = STRBUF_INIT;
+
+ if (argc != 2)
+ usage(_("git submodule--helper gitdir "));
+
+ submodule_name_to_gitdir(, the_repository, argv[1]);
+
+ printf("%s\n", gitdir.buf);
+
+ strbuf_release();
+ return 0;
+}
+
struct sync_cb {
const char *prefix;
unsigned int flags;
@@ -1268,18 +1283,24 @@ static int add_possible_reference_from_superproject(
* standard layout with .git/(modules/)+/objects
*/
if (ends_with(alt->path, "/objects")) {
+ struct repository alternate;
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
strbuf_add(, alt->path, strlen(alt->path) -
strlen("objects"));
+ repo_init(, sb.buf, NULL);
+
/*
* We need to end the new path with '/' to mark it as a dir,
* otherwise a submodule name containing '/' will be broken
* as the last part of a missing submodule reference would
* be taken as a file name.
*/
- strbuf_addf(, "modules/%s/", sas->submodule_name);
+ strbuf_reset();
+ submodule_name_to_gitdir(, , sas->submodule_name);
+ strbuf_addch(, '/');
+ repo_clear();
sm_alternate = compute_alternate_path(sb.buf, );
if (sm_alternate) {
@@ -1392,7 +1413,7 @@ static int module_clone(int argc, const char **argv,
const char *prefix)
usage_with_options(git_submodule_helper_usage,
module_clone_options);
- strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
+ submodule_name_to_gitdir(, the_repository, name);
sm_gitdir = absolute_pathdup(sb.buf);
strbuf_reset();
@@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const
char **argv, const char *p
name = argv[1];
path = argv[2];
- strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
+ submodule_name_to_gitdir(, the_repository, name);
sm_gitdir = absolute_pathdup(sb.buf);
connect_work_tree_and_git_dir(path, sm_gitdir, 0);
@@ -2040,6 +2061,7 @@ struct cmd_struct {
static struct cmd_struct commands[] = {
{"list", module_list, 0},
{"name", module_name, 0},
+ {"gitdir", module_gitdir, 0},
{"clone", module_clone, 0},
{"update-clone", update_clone, 0},
{"connect-gitdir-workingtree", connect_gitdir_workingtree, 0},
diff --git a/dir.c b/dir.c
index 21e6f2520a..7a9827ea4b 100644
--- a/dir.c
+++ b/dir.c
@@ -3053,7 +3053,7 @@ static void connect_wt_gitdir_in_nested(const char
*sub_worktree,
strbuf_reset(_wt);
strbuf_reset(_gd);
strbuf_addf(_wt, "%s/%s", sub_worktree, sub->path);
- strbuf_addf(_gd, "%s/modules/%s", sub_gitdir, sub->name);
+ submodule_name_to_gitdir(_gd, , sub->name);
connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 1);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index 8b5ad59bde..053747d290 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -252,12 +252,13 @@ Use -f if you really want to add it." >&2
fi
[PATCH 0/2] munge submodule names
Here's a more polished series taking into account some of the feedback on the RFC. As Junio pointed out URL encoding makes the directories much more human readable, but I'm open to other ideas if we don't think URL encoding is the right thing to do. Brandon Williams (2): submodule: create helper to build paths to submodule gitdirs submodule: munge paths to submodule git directories builtin/submodule--helper.c | 28 +++-- dir.c| 2 +- git-submodule.sh | 7 ++-- repository.c | 3 +- submodule.c | 67 ++-- submodule.h | 7 t/t7400-submodule-basic.sh | 32 ++- t/t7406-submodule-update.sh | 21 +++--- t/t7410-submodule-checkout-to.sh | 6 ++- 9 files changed, 126 insertions(+), 47 deletions(-) -- 2.18.0.597.ga71716f1ad-goog
[RFC] submodule: munge paths to submodule git directories
Commit 0383bbb901 (submodule-config: verify submodule names as paths,
2018-04-30) introduced some checks to ensure that submodule names don't
include directory traversal components (e.g. "../").
This addresses the vulnerability identified in 0383bbb901 but the root
cause is that we use submodule names to construct paths to the
submodule's git directory. What we really should do is munge the
submodule name before using it to construct a path.
Introduce a function "strbuf_submodule_gitdir()" which callers can use
to build a path to a submodule's gitdir. This allows for a single
location where we can munge the submodule name (by url encoding it)
before using it as part of a path.
Signed-off-by: Brandon Williams
---
Using submodule names as is continues to be not such a good idea. Maybe
we could apply something like this to stop using them as is. url
encoding seems like the easiest approach, but I've also heard
suggestions that would could use the SHA1 of the submodule name.
Any thoughts?
builtin/submodule--helper.c | 10 --
dir.c| 2 +-
repository.c | 3 +-
submodule.c | 57 +++-
submodule.h | 3 ++
t/t7400-submodule-basic.sh | 2 +-
t/t7406-submodule-update.sh | 21
t/t7410-submodule-checkout-to.sh | 6 ++--
8 files changed, 65 insertions(+), 39 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bd250ca216..37b7353167 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1122,18 +1122,24 @@ static int add_possible_reference_from_superproject(
* standard layout with .git/(modules/)+/objects
*/
if (ends_with(alt->path, "/objects")) {
+ struct repository alternate;
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
strbuf_add(, alt->path, strlen(alt->path) -
strlen("objects"));
+ repo_init(, sb.buf, NULL);
+
/*
* We need to end the new path with '/' to mark it as a dir,
* otherwise a submodule name containing '/' will be broken
* as the last part of a missing submodule reference would
* be taken as a file name.
*/
- strbuf_addf(, "modules/%s/", sas->submodule_name);
+ strbuf_reset();
+ strbuf_submodule_gitdir(, , sas->submodule_name);
+ strbuf_addch(, '/');
+ repo_clear();
sm_alternate = compute_alternate_path(sb.buf, );
if (sm_alternate) {
@@ -1246,7 +1252,7 @@ static int module_clone(int argc, const char **argv,
const char *prefix)
usage_with_options(git_submodule_helper_usage,
module_clone_options);
- strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
+ strbuf_submodule_gitdir(, the_repository, name);
sm_gitdir = absolute_pathdup(sb.buf);
strbuf_reset();
diff --git a/dir.c b/dir.c
index fe9bf58e4c..3463a5e0a5 100644
--- a/dir.c
+++ b/dir.c
@@ -3052,7 +3052,7 @@ static void connect_wt_gitdir_in_nested(const char
*sub_worktree,
strbuf_reset(_wt);
strbuf_reset(_gd);
strbuf_addf(_wt, "%s/%s", sub_worktree, sub->path);
- strbuf_addf(_gd, "%s/modules/%s", sub_gitdir, sub->name);
+ strbuf_submodule_gitdir(_gd, , sub->name);
connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 1);
}
diff --git a/repository.c b/repository.c
index 02fe884603..15fabbd08d 100644
--- a/repository.c
+++ b/repository.c
@@ -194,8 +194,7 @@ int repo_submodule_init(struct repository *submodule,
* submodule would not have a worktree.
*/
strbuf_reset();
- strbuf_repo_git_path(, superproject,
-"modules/%s", sub->name);
+ strbuf_submodule_gitdir(, superproject, sub->name);
if (repo_init(submodule, gitdir.buf, NULL)) {
ret = -1;
diff --git a/submodule.c b/submodule.c
index 939d6870ec..1d571845e8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1625,20 +1625,22 @@ int submodule_move_head(const char *path,
absorb_git_dir_into_superproject("", path,
ABSORB_GITDIR_RECURSE_SUBMODULES);
} else {
- char *gitdir = xstrfmt("%s/modules/%s",
- get_git_common_dir(), sub->name);
- connect_work_tree_and_git_dir(path, gitdir, 0);
-
Re: What's cooking in git.git (Aug 2018, #01; Thu, 2)
On 08/03, Junio C Hamano wrote: > Junio C Hamano writes: > > > "brian m. carlson" writes: > > > >> On Fri, Aug 03, 2018 at 11:40:08AM -0700, Junio C Hamano wrote: > >>> "brian m. carlson" writes: > >>> > >>> > On Thu, Aug 02, 2018 at 04:02:36PM -0700, Junio C Hamano wrote: > >>> >> -- > >>> >> [New Topics] > >>> > > >>> > I had expected to see > >>> > <20180729192803.1047050-1-sand...@crustytoothpaste.net> (the refspec @ > >>> > handling) in this list, but I don't. Were you expecting changes or > >>> > additional feedback before picking it up? > >>> > >>> Neither. I just am not all that interested in seeing @ used for > >>> HEAD in the first place, as I find it quite confusing. Discussion on our contributor workflow (and potentially adjusting or changing it) tends to be an area that can result in heated discussion, I would like to avoid that. At some point I would really like it if we could have a constructive discussion on how to improve our workflow as well as make things easier for newcomers. Maybe this isn't the time or place but I find this situation particularly challenging as a contributor. Someone sent a patch to the list, they received a review (which doesn't happen all the time because no one is assigned to review a patches) and then they wait. Our project doesn't have a bug tracker (yes jrn you'll say we have one, https://crbug.com/git/, but its not very discoverable and the project as a whole hasn't started using it) (and I mention a bug tracker because that's also a place which could be used to communicate the 'status' of a series), and it doesn't make use of a code review tool which has a definitive status of a patch series. As a contributor I'm left waiting and unsure of if my patch series needs more review, do I need to send a re-roll, does the project not like the idea and I should drop it, or the patch is good as-is and will be merged. When I started contributing to the project I was told that these "What's cooking" emails were supposed to be that status[1]. I knew that they weren't real-time but it meant that I could at least get an idea about once or twice a week about the various status' of ongoing series in the project. When something is just silently omitted, what is a contributor to think? Even if someone sends something that the project isn't interested in taking, shouldn't they at least get informed of that? Anyway those are just some of my thoughts. If my thinking is mistaken or I'm missing something please point it out to me. [1] https://public-inbox.org/git/cagz79kygs4dvoetygx01cinrxxlcqgxovsplhmgyz8b51lz...@mail.gmail.com/ This mail seems to counter that indicating that the "What's Cooking" emails should not be used as a status update. -- Brandon Williams
Re: [PATCH] fetch-pack: unify ref in and out param
On 08/01, Jonathan Tan wrote:
> When a user fetches:
> - at least one up-to-date ref and at least one non-up-to-date ref,
> - using HTTP with protocol v0 (or something else that uses the fetch
>command of a remote helper)
> some refs might not be updated after the fetch.
>
> This bug was introduced in commit 989b8c4452 ("fetch-pack: put shallow
> info in output parameter", 2018-06-28) which allowed transports to
> report the refs that they have fetched in a new out-parameter
> "fetched_refs". If they do so, transport_fetch_refs() makes this
> information available to its caller.
>
> Users of "fetched_refs" rely on the following 3 properties:
> (1) it is the complete list of refs that was passed to
> transport_fetch_refs(),
> (2) it has shallow information (REF_STATUS_REJECT_SHALLOW set if
> relevant), and
> (3) it has updated OIDs if ref-in-want was used (introduced after
> 989b8c4452).
>
> In an effort to satisfy (1), whenever transport_fetch_refs()
> filters the refs sent to the transport, it re-adds the filtered refs to
> whatever the transport supplies before returning it to the user.
> However, the implementation in 989b8c4452 unconditionally re-adds the
> filtered refs without checking if the transport refrained from reporting
> anything in "fetched_refs" (which it is allowed to do), resulting in an
> incomplete list, no longer satisfying (1).
>
> An earlier effort to resolve this [1] solved the issue by readding the
> filtered refs only if the transport did not refrain from reporting in
> "fetched_refs", but after further discussion, it seems that the better
> solution is to revert the API change that introduced "fetched_refs".
> This API change was first suggested as part of a ref-in-want
> implementation that allowed for ref patterns and, thus, there could be
> drastic differences between the input refs and the refs actually fetched
> [2]; we eventually decided to only allow exact ref names, but this API
> change remained even though its necessity was decreased.
>
> Therefore, revert this API change by reverting commit 989b8c4452, and
> make receive_wanted_refs() update the OIDs in the sought array (like how
> update_shallow() updates shallow information in the sought array)
> instead. A test is also included to show that the user-visible bug
> discussed at the beginning of this commit message no longer exists.
>
> [1] https://public-inbox.org/git/20180801171806.ga122...@google.com/
> [2]
> https://public-inbox.org/git/86a128c5fb710a41791e7183207c4d64889f9307.1485381677.git.jonathanta...@google.com/
>
> Signed-off-by: Jonathan Tan
> ---
> I now think that it's better to revert the API change introducing
> "fetched_refs" (or as Peff describes it, "this whole 'return the fetched
> refs' scheme from 989b8c4452"), so here is a patch doing so. I hope to
> have covered all of Peff's and Junio's questions in the commit message.
>
> As for Brandon's question:
>
> > I haven't thought too much about what we would need to do in the event
> > we add patterns to ref-in-want, but couldn't we possible mutate the
> > input list again in this case and just simply add the resulting refs to
> > the input list?
>
> If we support ref patterns, we would need to support deletion of refs,
> not just addition (because a ref might have existed in the initial ref
> advertisement, but not when the packfile is delivered). But it should
> be possible to add a flag stating "don't use this" to the ref, and
> document that transport_fetch_refs() can append additional refs to the
> tail of the input list. Upon hindsight, maybe this should have been the
> original API change instead of the "fetched_refs" mechanism.
Thanks for getting this out, it looks good to me. If we end up adding
patterns to ref-in-want then we can explore what changes would need to
be made then, I expect we may need to do a bit more work on the whole
fetching stack to get what we'd want in that case (because we would want
to avoid this issue again).
--
Brandon Williams
Re: [PATCH] transport: report refs only if transport does
On 07/31, Jonathan Tan wrote:
> > On Mon, Jul 30, 2018 at 03:56:01PM -0700, Jonathan Tan wrote:
> >
> > > Commit 989b8c4452 ("fetch-pack: put shallow info in output parameter",
> > > 2018-06-28) allows transports to report the refs that they have fetched
> > > in a new out-parameter "fetched_refs". If they do so,
> > > transport_fetch_refs() makes this information available to its caller.
> > >
> > > Because transport_fetch_refs() filters the refs sent to the transport,
> > > it cannot just report the transport's result directly, but first needs
> > > to readd the excluded refs, pretending that they are fetched. However,
> > > this results in a wrong result if the transport did not report the refs
> > > that they have fetched in "fetched_refs" - the excluded refs would be
> > > added and reported, presenting an incomplete picture to the caller.
> >
> > This part leaves me confused. If we are not fetching them, then why do
> > we need to pretend that they are fetched?
>
> The short answer is that we need:
> (1) the complete list of refs that was passed to
> transport_fetch_refs(),
> (2) with shallow information (REF_STATUS_REJECT_SHALLOW set if
> relevant), and
> (3) with updated OIDs if ref-in-want was used.
>
> The fetched_refs out param already fulfils (2) and (3), and this patch
> makes it fulfil (1). As for calling them fetched_refs, perhaps that is a
> misnomer, but they do appear in FETCH_HEAD even though they are not
> truly fetched.
>
> Which raises the question...if completeness is so important, why not
> reuse the input list of refs and document that transport_fetch_refs()
> can mutate the input list? You ask the same question below, so I'll put
> the answer after quoting your paragraph.
>
> > I think I am showing my lack of understanding about the reason for this
> > whole "return the fetched refs" scheme from 989b8c4452, and probably
> > reading the rest of that series would make it more clear. But from the
> > perspective of somebody digging into history and finding just this
> > commit, it probably needs to lay out a little more of the reasoning.
>
> I think it's because 989b8c4452 is based on my earlier work [1] which
> also had a fetched_refs out param. Its main reason is to enable the
> invoker of transport_fetch_refs() to specify ref patterns (as you can
> see in a later commit in the same patch set [2]) - and if we specify
> patterns, the invoker of transport_fetch_refs() needs the resulting refs
> (which are provided through fetched_refs).
>
> In the version that made it to master, however, there was some debate
> about whether ref patterns need to be allowed. In the end, ref patterns
> were not allowed [3], but the fetched_refs out param was still left in.
>
> I think that reverting the API might work, but am on the fence about it.
> It would reduce the number of questions about the code (and would
> probably automatically fix the issue that I was fixing in the first
If you believe the API is difficult to work with (which given this bug
it is) then perhaps we go with your suggestion and revert the API back
to only providing a list of input refs and having the fetch operation
mutate that input list.
> place), but if we were to revert the API and then decide that we do want
> ref patterns in "want-ref" (or expand transport_fetch_refs in some
> similar way), we would need to revert our revert, causing code churn.
I haven't thought too much about what we would need to do in the event
we add patterns to ref-in-want, but couldn't we possible mutate the
input list again in this case and just simply add the resulting refs to
the input list?
>
> [1]
> https://public-inbox.org/git/86a128c5fb710a41791e7183207c4d64889f9307.1485381677.git.jonathanta...@google.com/
> [2]
> https://public-inbox.org/git/eef2b77d88df0db08e4a1505b06e0af2d40143d5.1485381677.git.jonathanta...@google.com/
> [3] https://public-inbox.org/git/20180620213235.10952-1-bmw...@google.com/
--
Brandon Williams
Re: [PATCH] refspec: allow @ on the left-hand side of refspecs
On 07/30, brian m. carlson wrote: > On Mon, Jul 30, 2018 at 10:50:51AM -0700, Brandon Williams wrote: > > On 07/29, brian m. carlson wrote: > > > The object ID parsing machinery is aware of "@" as a synonym for "HEAD" > > > and this is documented accordingly in gitrevisions(7). The push > > > documentation describes the source portion of a refspec as "any > > > arbitrary 'SHA-1 expression'"; however, "@" is not allowed on the > > > left-hand side of a refspec, since we attempt to check for it being a > > > valid ref name and fail (since it is not). > > > > > > Teach the refspec machinery about this alias and silently substitute > > > "HEAD" when we see "@". This handles the fact that HEAD is a symref and > > > preserves its special behavior. We need not handle other arbitrary > > > object ID expressions (such as "@^") when pushing because the revision > > > machinery already handles that for us. > > > > So this claims that using "@^" should work despite not accounting for it > > explicitly or am I misreading? Unless I'm mistaken, it looks like we > > don't really support arbitrary rev syntax in refspecs since "HEAD^" > > doesn't work either. > > Correct, it does indeed work, at least for me: > > genre ok % git push castro HEAD^:refs/heads/temp > Total 0 (delta 0), reused 0 (delta 0) > To https://git.crustytoothpaste.net/git/bmc/git.git > * [new branch]HEAD^ -> temp > > genre ok % git push castro @^:refs/heads/temp > Total 0 (delta 0), reused 0 (delta 0) > To https://git.crustytoothpaste.net/git/bmc/git.git > * [new branch]@^ -> temp > > Note that in this case, I had to specify a full ref since it didn't > exist on the remote and the left side wasn't a ref name. That's what I was missing, a full refspec! Thanks for the illustration. > > Now it doesn't work for fetches, only pushes. Only the left side of a > push refspec can be an arbitrary expression. > -- > brian m. carlson: Houston, Texas, US > OpenPGP: https://keybase.io/bk2204 -- Brandon Williams
Re: [BUG] fetching sometimes doesn't update refs
On 07/29, Jeff King wrote: > I've noticed for the past couple of weeks that some of my fetches don't > seem to actually update refs, but a follow-up fetch will. I finally > managed to catch it in the act and track it down. It bisects to your > 989b8c4452 (fetch-pack: put shallow info in output parameter, > 2018-06-27). > > A reproduction recipe is below. I can't imagine why this repo in > particular triggers it, but it was the one where I initially saw the > problem (and doing a tiny reproduction does not seem to work). I'm > guessing it has something to do with the refs, since the main change in > the offending commit is that we recompute the refmap. I've noticed this behavior sporadically as well, though I've never been able to reliably reproduce it, so thanks for creating a reproduction recipe. I suspected that it had to do with the ref-in-want series so thanks for tracking that down too. We'll take a look. > > -- >8 -- > # clone the repo as it is today > git clone https://github.com/cmcaine/tridactyl.git > cd tridactyl > > # roll back the refs so that there is something to fetch > for i in refs/heads/master refs/remotes/origin/master; do > git update-ref $i $i^ > done > > # and delete the now-unreferenced objects, pretending we are an earlier > # clone that had not yet fetched > rm -rf .git/logs > git repack -ad > > # now fetch; this will get the objects but fail to update refs > git fetch > > # and fetching again will actually update the refs > git fetch > -- 8< -- > > -Peff -- Brandon Williams
Re: [PATCH] refspec: allow @ on the left-hand side of refspecs
On 07/29, brian m. carlson wrote:
> The object ID parsing machinery is aware of "@" as a synonym for "HEAD"
> and this is documented accordingly in gitrevisions(7). The push
> documentation describes the source portion of a refspec as "any
> arbitrary 'SHA-1 expression'"; however, "@" is not allowed on the
> left-hand side of a refspec, since we attempt to check for it being a
> valid ref name and fail (since it is not).
>
> Teach the refspec machinery about this alias and silently substitute
> "HEAD" when we see "@". This handles the fact that HEAD is a symref and
> preserves its special behavior. We need not handle other arbitrary
> object ID expressions (such as "@^") when pushing because the revision
> machinery already handles that for us.
So this claims that using "@^" should work despite not accounting for it
explicitly or am I misreading? Unless I'm mistaken, it looks like we
don't really support arbitrary rev syntax in refspecs since "HEAD^"
doesn't work either.
>
> Signed-off-by: brian m. carlson
> ---
> I probably type "git push upstream HEAD" from five to thirty times a
> day, many of those where I typo "HEAD", so I decided to implement the
> shorter form. This design handles @ as HEAD in both fetch and push,
> whereas alternate solutions would not.
I'm always a fan of finding shortcuts and reducing how much I type, so
thank you :)
>
> check_refname_format explicitly rejects "@"; I tried at first to simply
> ignore that with a flag, but we end up calling that from several other
> places in the codebase and rejecting it and all of those places would
> have needed updating.
>
> I thought about putting the if/else logic in a function, but since it's
> just four lines, I decided not to. However, if people think it would be
> tidier, I can do so.
>
> Note that the test portion of the patch is best read with git diff -w;
> the current version is very noisy.
>
> refspec.c | 6 ++-
> t/t5516-fetch-push.sh | 104 +-
> 2 files changed, 58 insertions(+), 52 deletions(-)
>
> diff --git a/refspec.c b/refspec.c
> index e8010dce0c..57c2f65104 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -62,8 +62,12 @@ static int parse_refspec(struct refspec_item *item, const
> char *refspec, int fet
> return 0;
> }
>
> + if (llen == 1 && lhs[0] == '@')
> + item->src = xstrdup("HEAD");
> + else
> + item->src = xstrndup(lhs, llen);
> +
This is probably the easiest place to put the aliasing logic so I don't
have any issue with including it here.
--
Brandon Williams
Re: [PATCH] config: fix case sensitive subsection names on writing
On 07/27, Stefan Beller wrote:
> A use reported a submodule issue regarding strange case indentation
> issues, but it could be boiled down to the following test case:
>
> $ git init test && cd test
> $ git config foo."Bar".key test
> $ git config foo."bar".key test
> $ tail -n 3 .git/config
> [foo "Bar"]
> key = test
> key = test
>
> Sub sections are case sensitive and we have a test for correctly reading
> them. However we do not have a test for writing out config correctly with
> case sensitive subsection names, which is why this went unnoticed in
> 6ae996f2acf (git_config_set: make use of the config parser's event
> stream, 2018-04-09)
>
> Make the subsection case sensitive and provide a test for both reading
> and writing.
>
> Reported-by: JP Sugarbroad
> Signed-off-by: Stefan Beller
> ---
> config.c | 2 +-
> t/t1300-config.sh | 18 ++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/config.c b/config.c
> index 3aacddfec4b..3ded92b678b 100644
> --- a/config.c
> +++ b/config.c
> @@ -2374,7 +2374,7 @@ static int store_aux_event(enum config_event_t type,
> store->is_keys_section =
> store->parsed[store->parsed_nr].is_keys_section =
> cf->var.len - 1 == store->baselen &&
> - !strncasecmp(cf->var.buf, store->key, store->baselen);
> + !strncmp(cf->var.buf, store->key, store->baselen);
I've done some work in the config part of our codebase but I don't
really know whats going on here due to two things: this is a callback
function and it relies on global state...
I can say though that we might want to be careful about completely
converting this to a case sensitive compare. Our config is a key
value store with the following format: 'section.subsection.key'. IIRC
both section and key are always compared case insensitively. The
subsection can be case sensitive or insensitive based on how its
stored in the config files itself:
# Case insensitive
[section.subsection]
key = value
# Case sensitive
[section "subsection"]
key = value
But I don't know how you distinguish between these cases when a config
is specified on a single line (section.subsection.key). Do we always
assume the sensitive version because the insensitive version is
documented to be deprecated?
Either way you're probably going to need to be careful about how you do
string comparison against the different parts.
> if (store->is_keys_section) {
> store->section_seen = 1;
> ALLOC_GROW(store->seen, store->seen_nr + 1,
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 03c223708eb..8325d4495f4 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' '
> test_cmp expect actual
> '
>
> +test_expect_success 'setting different case subsections ' '
> + test_when_finished "rm -f caseSens caseSens_actual caseSens_expect" &&
> +
> + # v.a.r and v.A.r are not the same variable, as the middle
> + # level of a three-level configuration variable name is
> + # case sensitive.
> + git config -f caseSens v."A".r VAL &&
> + git config -f caseSens v."a".r val &&
> +
> + echo VAL >caseSens_expect &&
> + git config -f caseSens v."A".r >caseSens_actual &&
> + test_cmp caseSens_expect caseSens_actual &&
> +
> + echo val >caseSens_expect &&
> + git config -f caseSens v."a".r >caseSens_actual &&
> + test_cmp caseSens_expect caseSens_actual
> +'
> +
> for VAR in a .a a. a.0b a."b c". a."b c".0d
> do
> test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
> --
> 2.18.0.345.g5c9ce644c3-goog
>
--
Brandon Williams
Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API
On 07/27, Duy Nguyen wrote: > On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller wrote: > > > > Currently the refs API takes a 'ref_store' as an argument to specify > > which ref store to iterate over; however it is more useful to specify > > the repository instead (or later a specific worktree of a repository). > > There is no 'later'. worktrees.c already passes a worktree specific > ref store. If you make this move you have to also design a way to give > a specific ref store now. > > Frankly I still dislike the decision to pass repo everywhere, > especially when refs code already has a nice ref-store abstraction. > Some people frown upon back pointers. But I think adding a back > pointer in ref-store, pointing back to the repository is the right > move. I don't quite understand why the refs code would need a whole repository and not just the ref-store it self. I thought the refs code was self contained enough that all its state was based on the passed in ref-store. If its not, then we've done a terrible job at avoiding layering violations (well actually we're really really bad at this in general, and I *think* we're trying to make this better though the object store/index refactoring). If anything I would expect that the actual ref-store code would remain untouched by any refactoring and that instead the higher-level API that hasn't already been converted to explicitly use a ref-store (and instead just calls the underlying impl with get_main_ref_store()). Am I missing something here? -- Brandon Williams
Re: [RFC] push: add documentation on push v2
On 07/25, Duy Nguyen wrote: > On Tue, Jul 24, 2018 at 9:29 PM Brandon Williams wrote: > > > > On 07/17, Brandon Williams wrote: > > > Signed-off-by: Brandon Williams > > > --- > > > > > > Since introducing protocol v2 and enabling fetch I've been thinking > > > about what its inverse 'push' would look like. After talking with a > > > number of people I have a longish list of things that could be done to > > > improve push and I think I've been able to distill the core features we > > > want in push v2. Thankfully (due to the capability system) most of the > > > other features/improvements can be added later with ease. > > > > > > What I've got now is a rough design for a more flexible push, more > > > flexible because it allows for the server to do what it wants with the > > > refs that are pushed and has the ability to communicate back what was > > > done to the client. The main motivation for this is to work around > > > issues when working with Gerrit and other code-review systems where you > > > need to have Change-Ids in the commit messages (now the server can just > > > insert them for you and send back new commits) and you need to push to > > > magic refs to get around various limitations (now a Gerrit server should > > > be able to communicate that pushing to 'master' doesn't update master > > > but instead creates a refs/changes/ ref). > > > > > > Before actually moving to write any code I'm hoping to get some feedback > > > on if we think this is an acceptable base design for push (other > > > features like atomic-push, signed-push, etc can be added as > > > capabilities), so any comments are appreciated. > > > > > > Documentation/technical/protocol-v2.txt | 76 + > > > 1 file changed, 76 insertions(+) > > > > Pinging this thread again to hopefully reach some more people for > > commentary. > > Could you send a v2 that covers all the push features in pack version > 1? I see some are discussed but it's probably good to summarize in > this document too. I can mention the ones we want to implement, but I expect that a push v2 would not require that all features in the current push are supported out of the box. Some servers may not want to support signed-push, etc. Also I don't want to have to implement every single feature that exists before getting something merged. This way follow on series can be written to implement those as new features to push v2. > > A few other comments > > If I remember correctly, we always update the remote refs locally > after a push, assuming that the next 'fetch' will do the same anyway. > This is not true for special refs (like those from gerrit). Looking > from this I don't think it can say "yes we have received your pack and > stored it "somewhere" but there's no visible ref created for it" so > that we can skip the local remote ref update? This is one of the pain points for gerrit and one of the reasons why they have this funky push syntax "push origin HEAD:refs/for/master". Because its not a remote tracking branch for the current branch, we don't update (or create) a local branch "for/master" under the "refs/remotes/origin" namespace (at least that's how i understand it). So in order to support the server doing more things (rebasing, or creating new branches) based on what is pushed the status that the server sends in response needs to be more fluid so that the server can describe what it did in a way that the client can either: update the remote tracking branches, or not (and in this case maybe do what you suggest down below). > > Is it simpler to tell a push client at the end that "yes there's new > stuff now on the server, do another fetch", sort of like HTTP > redirect, then the client can switch to fetch protocol to get the new > stuff that the server has created (e.g. rebase stuff)? I assume we > could reuse the same connection for both push and fetch if needed. > This way both fetch and push send packs in just one direction. I really, really like this suggestion. Thank you for your input. This would actually make the protocol much simpler by keeping push for pushing packs and fetch for fetching packs. And since my plan is to have the status-report for push include all the relevant ref changes that the server made, if there are any that don't correspond with what was pushed (either the server rebased a change or created a new ref I didn't) then we can skip the ref-advertisement and go straight to fetching those refs. And yes, since protocol v2 is command based we could reuse the existing connection and simply send a "fetch" request after our "push" one. This does mean that it'll be an extra round trip than what I originally had in mind, but it should be simpler. -- Brandon Williams
Re: [PATCH] pack-protocol: mention and point to docs for protocol v2
On 07/24, Junio C Hamano wrote:
> Brandon Williams writes:
>
> >> Not about this patch, but I wonder if an organization along the
> >> following lines would make sense?
> >>
> >> 1. Rename pack-protocol.txt to protocol-v1.txt. Rename
> >> protocol-v2.txt to pack-protocol.txt.
> >>
> >> 2. Make pack-protocol.txt self-contained, and remove any redundant
> >> sections from protocol-v1.txt.
> >>
> >> 3. Add a new protocol-v2.txt that briefly describes the benefits and
> >> highlights of protocol v2, referring to pack-protocol.txt for
> >> details.
> >>
> >> That way, newcomers of the future could read pack-protocol.txt and
> >> quickly glean the main protocol in (then) current use.
> >>
> >> What do you think?
> >
> > I dislike the idea of renaming protocol-v2.txt to pack-protocol.txt. I
> > agree that we should probably have protocol-v1 broken out into its own
> > file, taking the parts from pack-protocol.txt, but what really should
> > happen is that pack-protocol.txt could describe the basics of the wire
> > protocol (pkt-lines, the format of the various transports, etc) and then
> > refer to the protocol-v{1,2}.txt documents themselves.
>
> WRT the naming, are we happy with the idea of (1) pretending that
> when we say 'protocol', there is nothing but the on-the-wire
> pkt-line protocol (i.e. that is why we call "protocol-v2" without
> giving any other adjective---are we sure we won't have need for any
> other kind of protocol?) and (2) tying the "pack" ness to the name of
> on-the-wire pkt-line protocol (i.e. that is where the name of the
> original pack-protocol.txt came from, as it started only for the
> packfile transfer---are we happy to keep newer protocols tied to
> "pack" the same way)?
If so I suggest we move away from the term "pack" protocol. Mostly
because maybe at some future date we don't only want to communicate to
transfer packs. So at the risk of bikeshedding (and because naming is
hard) I think we should begin talking about the over the wire protocol
as just that, the "wire protocol" or if we need to be more explicit the
"git wire protocol". Thoughts?
--
Brandon Williams
Re: What's cooking in git.git (Jul 2018, #02; Wed, 18)
On 07/24, Junio C Hamano wrote:
> Jonathan Tan writes:
>
> >> Here are the topics that have been cooking. Commits prefixed with
> >> '-' are only in 'pu' (proposed updates) while commits prefixed with
> >> '+' are in 'next'. The ones marked with '.' do not appear in any of
> >> the integration branches, but I am still holding onto them.
> >
> > What do you think about my fixes to protocol v2 tag following [1]? There
> > was some discussion about correctness vs the drop in performance, but it
> > seems to me that there is some consensus that the drop in performance is
> > OK.
> >
> > [1]
> > https://public-inbox.org/git/cover.1528234587.git.jonathanta...@google.com/
>
> Thanks for reminding. I think I was waiting for Brandon or somebody
> else to say something after [2] as the final confirmation before
> queuing it, and then the thread was forgotten ;-)
>
> Will pick it up; it seems to have some interaction with Brandon's
> 6d1700d5 ("fetch: refactor to make function args narrower",
> 2018-06-27), and I think the correct resolution is to move your
> removal of "&& !rs->nr" to do_fetch() function where that commit
> moved to.
>
> Thanks.
>
> [2] https://public-inbox.org/git/xmqqd0vwcfkr@gitster-ct.c.googlers.com/
Yeah I still don't like it from a performance perspective, but given
people rely on this functionality I've been convinced its necessary for
correctness until we make other changes.
--
Brandon Williams
Re: [RFC] push: add documentation on push v2
On 07/17, Brandon Williams wrote: > Signed-off-by: Brandon Williams > --- > > Since introducing protocol v2 and enabling fetch I've been thinking > about what its inverse 'push' would look like. After talking with a > number of people I have a longish list of things that could be done to > improve push and I think I've been able to distill the core features we > want in push v2. Thankfully (due to the capability system) most of the > other features/improvements can be added later with ease. > > What I've got now is a rough design for a more flexible push, more > flexible because it allows for the server to do what it wants with the > refs that are pushed and has the ability to communicate back what was > done to the client. The main motivation for this is to work around > issues when working with Gerrit and other code-review systems where you > need to have Change-Ids in the commit messages (now the server can just > insert them for you and send back new commits) and you need to push to > magic refs to get around various limitations (now a Gerrit server should > be able to communicate that pushing to 'master' doesn't update master > but instead creates a refs/changes/ ref). > > Before actually moving to write any code I'm hoping to get some feedback > on if we think this is an acceptable base design for push (other > features like atomic-push, signed-push, etc can be added as > capabilities), so any comments are appreciated. > > Documentation/technical/protocol-v2.txt | 76 + > 1 file changed, 76 insertions(+) Pinging this thread again to hopefully reach some more people for commentary. Looking back through the comments so far there are concerns that a server shouldn't be trusted rewriting my local changes, so to address that we could have the be a config option which is defaulted to not take changes from a server. Apart from that I didn't see any other major concerns. I'm hoping to get a bit more discussion going before actually beginning work on this. -- Brandon Williams
Re: [RFC] push: add documentation on push v2
On 07/20, Jeff Hostetler wrote: > > > On 7/18/2018 1:15 PM, Brandon Williams wrote: > > On 07/18, Stefan Beller wrote: > > > On Wed, Jul 18, 2018 at 6:31 AM Derrick Stolee wrote: > > > > > > > > On 7/17/2018 7:25 PM, Stefan Beller wrote: > > > > > On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams > > > > > wrote: > > > > > > Signed-off-by: Brandon Williams > > > > > > --- > > > > > > > > > > > > Since introducing protocol v2 and enabling fetch I've been thinking > > > > > > about what its inverse 'push' would look like. After talking with a > > > > > > number of people I have a longish list of things that could be done > > > > > > to > > > > > > improve push and I think I've been able to distill the core > > > > > > features we > > > > > > want in push v2. > > > > > It would be nice to know which things you want to improve. > > > > > > > > Hopefully we can also get others to chime in with things they don't like > > > > about the existing protocol. What pain points exist, and what can we do > > > > to improve at the transport layer before considering new functionality? > > > > > > Another thing that I realized last night was the possibility to chunk > > > requests. > > > The web of today is driven by lots of small http(s) requests. I know our > > > server > > > team fights with the internal tools all the time because the communication > > > involved in git-fetch is usually a large http request (large packfile). > > > So it would be nice to have the possibility of chunking the request. > > > But I think that can be added as a capability? (Not sure how) > > > > Fetch and push requests/responses are already "chunked" when using the > > http transport. So I'm not sure what you mean by adding a capability > > because the protocol doesn't care about which transport you're using. > > This is of course unless you're talking about a different "chunking" > > from what it means to chunk an http request/response. > > > > Internally, we've talked about wanting to have resumable pushes and > fetches. I realize this is difficult to do when the server is > replicated and the repeated request might be talking to a different > server instance. And there's a problem with temp files littering the > server as it waits for the repeated attempt. But still, the packfile > sent/received can be large and connections do get dropped. > > That is, if we think about sending 1 large packfile and just using a > byte-range-like approach to resuming the transfer. > > If we allowed the request to send a series of packfiles, with each > "chunk" being self-contained and usable. So if a push connection was > dropped the server could apply the successfully received packfile(s) > (add the received objects and update the refs to the commits received so > far). And ignore the interrupted and unreceived packfile(s) and let the > client retry later. When/if the client retried the push, it would > renegotiate haves/wants and send a new series of packfile(s). With the > assumption being that the server would have updated refs from the > earlier aborted push, so the packfile(s) computed for the second attempt > would not repeat the content successfully transmitted in the first > attempt. > > This would require that the client build an ordered set of packfiles > from oldest to newest so that the server can apply them in-order and > the graph remain connected. That may be outside your scope here. > > Also, we might have to add a few messages to the protocol after the > negotiation, for the client to say that it is going to send the push > content in 'n' packfiles and send 'n' messages with the intermediate > ref values being updated in each packfile. > > Just thinking out loud here. > Jeff We've talked about working on resumable fetch/push (both of which are out of the scope of this work), but we haven't started working on anything just yet. There's a couple different ways to do this like you've pointed out, we can either have the server redirect the client to fetch from a CDN (where its put the packfile) and then the client can use ranged requests to fetch until the server decides to remove it from the CDN. This can be tricky because every fetch can produce a unique packfile so maybe you don't want to put a freshly constructed, unique packfile for each client request up on a CDN somewhere. Breaking up a response into multiple packfiles and small ref-updates could work, that way as long as some of the smaller packs/updates are applied then the client is making headway towards being up to date with the server. -- Brandon Williams
Re: [PATCH] pack-protocol: mention and point to docs for protocol v2
On 07/23, Jonathan Nieder wrote:
> Hi,
>
> Brandon Williams wrote:
>
> > --- a/Documentation/technical/pack-protocol.txt
> > +++ b/Documentation/technical/pack-protocol.txt
> > @@ -50,7 +50,8 @@ Each Extra Parameter takes the form of `=` or
> > ``.
> >
> > Servers that receive any such Extra Parameters MUST ignore all
> > unrecognized keys. Currently, the only Extra Parameter recognized is
> > -"version=1".
> > +"version" with a vlue of '1' or '2'. See protocol-v2.txt for more
>
> value?
yep, missed a letter.
>
> > +information on protocol version 2.
>
> Thanks. Some thoughts on other parts of this document that may need
> updating:
>
> - the whole document assumes that 0 and 1 are the only protocol
> versions. E.g. the discussion of the version number line in the
> response when "version=1" is sent as an Extra Paramter should probably
> apply to version 2, too.
>
> - because the document was written before protocol v2, it describes the
> more complicated v1 that many readers shouldn't have to care about
>
> - there is no one document that describes v2 in a self contained way,
> since protocol-v2.txt makes reference to protocol v1.
>
> - the description of pkt-line format in protocol-common.txt is missing
> a discussion of delim-pkt.
>
> Not about this patch, but I wonder if an organization along the
> following lines would make sense?
>
> 1. Rename pack-protocol.txt to protocol-v1.txt. Rename
> protocol-v2.txt to pack-protocol.txt.
>
> 2. Make pack-protocol.txt self-contained, and remove any redundant
> sections from protocol-v1.txt.
>
> 3. Add a new protocol-v2.txt that briefly describes the benefits and
> highlights of protocol v2, referring to pack-protocol.txt for
> details.
>
> That way, newcomers of the future could read pack-protocol.txt and
> quickly glean the main protocol in (then) current use.
>
> What do you think?
I dislike the idea of renaming protocol-v2.txt to pack-protocol.txt. I
agree that we should probably have protocol-v1 broken out into its own
file, taking the parts from pack-protocol.txt, but what really should
happen is that pack-protocol.txt could describe the basics of the wire
protocol (pkt-lines, the format of the various transports, etc) and then
refer to the protocol-v{1,2}.txt documents themselves.
--
Brandon Williams
Re: [PATCH] fetch-pack: mark die strings for translation
On 07/23, Junio C Hamano wrote:
> Junio C Hamano writes:
>
> > Brandon Williams writes:
> >
> >> Signed-off-by: Brandon Williams
> >> ---
> >> fetch-pack.c | 16
> >> 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > I do think this is a good idea, but what release does this target?
> > It does not seem to apply to master@{4.hours.ago}, and I do not
> > think a change as trivial like this wants to be taken of a hostage
> > of _all_ topics that are in 'next'.
> >
>
> Answering myself, this would come on top of (and would become part
> of) bw/ref-in-want topic, I would think. And I am perfectly OK for
> this change to be hostage of that topic ;-)
haha yes sorry for not mention it, but yes I built it on top of
bw/ref-in-want because it was in response to this topic :)
--
Brandon Williams
[PATCH] fetch-pack: mark die strings for translation
Signed-off-by: Brandon Williams
---
fetch-pack.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 0b4a9f288f..51abee6181 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1245,13 +1245,13 @@ static int process_section_header(struct packet_reader
*reader,
int ret;
if (packet_reader_peek(reader) != PACKET_READ_NORMAL)
- die("error reading section header '%s'", section);
+ die(_("error reading section header '%s'"), section);
ret = !strcmp(reader->line, section);
if (!peek) {
if (!ret)
- die("expected '%s', received '%s'",
+ die(_("expected '%s', received '%s'"),
section, reader->line);
packet_reader_read(reader);
}
@@ -1289,12 +1289,12 @@ static int process_acks(struct packet_reader *reader,
struct oidset *common)
continue;
}
- die("unexpected acknowledgment line: '%s'", reader->line);
+ die(_("unexpected acknowledgment line: '%s'"), reader->line);
}
if (reader->status != PACKET_READ_FLUSH &&
reader->status != PACKET_READ_DELIM)
- die("error processing acks: %d", reader->status);
+ die(_("error processing acks: %d"), reader->status);
/* return 0 if no common, 1 if there are common, or 2 if ready */
return received_ready ? 2 : (received_ack ? 1 : 0);
@@ -1331,7 +1331,7 @@ static void receive_shallow_info(struct fetch_pack_args
*args,
if (reader->status != PACKET_READ_FLUSH &&
reader->status != PACKET_READ_DELIM)
- die("error processing shallow info: %d", reader->status);
+ die(_("error processing shallow info: %d"), reader->status);
setup_alternate_shallow(_lock, _shallow_file, NULL);
args->deepen = 1;
@@ -1346,7 +1346,7 @@ static void receive_wanted_refs(struct packet_reader
*reader, struct ref *refs)
struct ref *r = NULL;
if (parse_oid_hex(reader->line, , ) || *end++ != ' ')
- die("expected wanted-ref, got '%s'", reader->line);
+ die(_("expected wanted-ref, got '%s'"), reader->line);
for (r = refs; r; r = r->next) {
if (!strcmp(end, r->name)) {
@@ -1356,11 +1356,11 @@ static void receive_wanted_refs(struct packet_reader
*reader, struct ref *refs)
}
if (!r)
- die("unexpected wanted-ref: '%s'", reader->line);
+ die(_("unexpected wanted-ref: '%s'"), reader->line);
}
if (reader->status != PACKET_READ_DELIM)
- die("error processing wanted refs: %d", reader->status);
+ die(_("error processing wanted refs: %d"), reader->status);
}
enum fetch_state {
--
2.18.0.233.g985f88cf7e-goog
Re: [PATCH v6 8/8] fetch-pack: implement ref-in-want
On 07/22, Duy Nguyen wrote:
> On Thu, Jun 28, 2018 at 12:33 AM Brandon Williams wrote:
> > +static void receive_wanted_refs(struct packet_reader *reader, struct ref
> > *refs)
> > +{
> > + process_section_header(reader, "wanted-refs", 0);
> > + while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
> > + struct object_id oid;
> > + const char *end;
> > + struct ref *r = NULL;
> > +
> > + if (parse_oid_hex(reader->line, , ) || *end++ != '
> > ')
> > + die("expected wanted-ref, got '%s'", reader->line);
>
> Could you do a follow and wrap all these strings in _() since this one
> is already in 'next'?
What criteria is used to determine if something should be translated?
To me, this looks like a wire-protocol error which would benefit from
not being translated because it would be easier to grep for if it
occurs. That and if a user sees this sort of error I don't think that
they could really do anything about it anyway.
Of course it appears as if all other 'die' calls in fetch-pack have been
marked for translation so I guess my though process doesn't hold :)
--
Brandon Williams
[PATCH] pack-protocol: mention and point to docs for protocol v2
Signed-off-by: Brandon Williams --- Documentation/technical/pack-protocol.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index 7fee6b780a..25acd9edb1 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -50,7 +50,8 @@ Each Extra Parameter takes the form of `=` or ``. Servers that receive any such Extra Parameters MUST ignore all unrecognized keys. Currently, the only Extra Parameter recognized is -"version=1". +"version" with a vlue of '1' or '2'. See protocol-v2.txt for more +information on protocol version 2. Git Transport - -- 2.18.0.233.g985f88cf7e-goog
[PATCH v2] clone: send ref-prefixes when using protocol v2
Teach clone to send a list of ref-prefixes, when using protocol v2, to
allow the server to filter out irrelevant references from the
ref-advertisement. This reduces wasted time and bandwidth when cloning
repositories with a larger number of references.
Signed-off-by: Brandon Williams
---
builtin/clone.c| 20 +++-
t/t5702-protocol-v2.sh | 7 ++-
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae85..5c0adbd6d0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -895,7 +895,8 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
int err = 0, complete_refs_before_fetch = 1;
int submodule_progress;
- struct refspec_item refspec;
+ struct refspec rs = REFSPEC_INIT_FETCH;
+ struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
fetch_if_missing = 0;
@@ -1077,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (option_required_reference.nr || option_optional_reference.nr)
setup_reference();
- refspec_item_init(, value.buf, REFSPEC_FETCH);
+ refspec_append(, value.buf);
strbuf_reset();
@@ -1134,10 +1135,18 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (transport->smart_options && !deepen && !filter_options.choice)
transport->smart_options->check_self_contained_and_connected =
1;
- refs = transport_get_remote_refs(transport, NULL);
+
+ argv_array_push(_prefixes, "HEAD");
+ refspec_ref_prefixes(, _prefixes);
+ if (option_branch)
+ expand_ref_prefix(_prefixes, option_branch);
+ if (!option_no_tags)
+ argv_array_push(_prefixes, "refs/tags/");
+
+ refs = transport_get_remote_refs(transport, _prefixes);
if (refs) {
- mapped_refs = wanted_peer_refs(refs, );
+ mapped_refs = wanted_peer_refs(refs, [0]);
/*
* transport_get_remote_refs() may return refs with null sha-1
* in mapped_refs (see struct transport->get_refs_list
@@ -1231,6 +1240,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
strbuf_release();
junk_mode = JUNK_LEAVE_ALL;
- refspec_item_clear();
+ refspec_clear();
+ argv_array_clear(_prefixes);
return err;
}
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index a4fe6508bd..9c6ea04a69 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -181,7 +181,12 @@ test_expect_success 'clone with file:// using protocol v2'
'
test_cmp expect actual &&
# Server responded using protocol v2
- grep "clone< version 2" log
+ grep "clone< version 2" log &&
+
+ # Client sent ref-prefixes to filter the ref-advertisement
+ grep "ref-prefix HEAD" log &&
+ grep "ref-prefix refs/heads/" log &&
+ grep "ref-prefix refs/tags/" log
'
test_expect_success 'fetch with file:// using protocol v2' '
--
2.18.0.233.g985f88cf7e-goog
[PATCH] clone: send ref-prefixes when using protocol v2
Signed-off-by: Brandon Williams
---
Noticed we miss out on server side filtering of refs when cloning using
protocol v2, this will enable that.
builtin/clone.c | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae85..55cc10e93a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -895,7 +895,8 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
int err = 0, complete_refs_before_fetch = 1;
int submodule_progress;
- struct refspec_item refspec;
+ struct refspec rs = REFSPEC_INIT_FETCH;
+ struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
fetch_if_missing = 0;
@@ -1077,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (option_required_reference.nr || option_optional_reference.nr)
setup_reference();
- refspec_item_init(, value.buf, REFSPEC_FETCH);
+ refspec_append(, value.buf);
strbuf_reset();
@@ -1134,10 +1135,20 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (transport->smart_options && !deepen && !filter_options.choice)
transport->smart_options->check_self_contained_and_connected =
1;
- refs = transport_get_remote_refs(transport, NULL);
+
+ argv_array_push(_prefixes, "HEAD");
+ refspec_ref_prefixes(, _prefixes);
+ if (option_branch) {
+ expand_ref_prefix(_prefixes, option_branch);
+ }
+ if (!option_no_tags) {
+ argv_array_push(_prefixes, "refs/tags/");
+ }
+
+ refs = transport_get_remote_refs(transport, _prefixes);
if (refs) {
- mapped_refs = wanted_peer_refs(refs, );
+ mapped_refs = wanted_peer_refs(refs, [0]);
/*
* transport_get_remote_refs() may return refs with null sha-1
* in mapped_refs (see struct transport->get_refs_list
@@ -1231,6 +1242,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
strbuf_release();
junk_mode = JUNK_LEAVE_ALL;
- refspec_item_clear();
+ refspec_clear();
+ argv_array_clear(_prefixes);
return err;
}
--
2.18.0.233.g985f88cf7e-goog
Re: [RFC] push: add documentation on push v2
you don't care if one existed or
> > not, you just want the ref to have a certain value.
>
> What I was trying to say is to have
>
> update = txn_id SP (modifier SP) action
> modifier = "force" | "atomic"
> action = (create | delete | update)
> create = "create" SP
> update = "update" SP SP
> delete = "delete" SP
>
> i.e. only one hash for the create and delete action.
> (I added the "atomic" modifier to demonstrate (A) from above, not needed here)
I understood what you were asking, I was just pointing out the rationale
for including the zero-id but i guess you're correct in that simply
omitting it would work just as well for what I outlined above. So I
think I'll go with what you suggested here.
>
> > >
> > > > + * Forced ref-updates only include the new value of a ref as we
> > > > don't
> > > > + care what the old value was.
> > >
> > > How are you implementing force-with-lease then?
> >
> > Currently force-with-lease/force is implemented 100% on the client side,
>
> Uh? That would be bad. Reading 631b5ef219c (push --force-with-lease: tie
> it all together, 2013-07-08) I think that send-pack is done server-side?
send-pack.c is the client side of push while receive-pack is the
server side. There currently doesn't exist a way for a server to
understand the difference between a force/force-with-lease/non-force
push.
>
> > this proposal extends these two to be implemented on the server as well.
> > non-forced variant are basically the "with-lease" case and "force" now
> > actually forces an update.
>
> I think we have 3 modes:
> (1) standard push, where both client and server check for a fast-forward
> (2) "force" that blindly overwrites the ref, but as that has a race condition
> in case multiple people can push to the remove we have
> (3) "with-lease", disables the fast forward check both on client and server
> but still takes out a lock on the server to ensure no races happen
>
> Now you propose to have only 2, making (1) and (3) the same, deferring
> the check to have "fast forwards only" to be client only?
> The server surely wants to ensure that, too (maybe you need
> special permission for non-ff; depends on the server implementation).
>
> I am not sure I like it, as on the protocol level this indeed looks the same
> and the server and client need to care in their implementation how/when
> the ff-check is done. Though it would be nice for the client UX that you
> need to give a flag to check for ff (both client and server side? or can we
> rely
> on the client alone then?)
IIRC there are no checks done server-side for force, with-lease, or
fast-forward (well fast-forward can be checked for but this is a config
and has nothing to do with the protocol). Most of the work is done by
the client to ensure these checks are made.
>
> > > > + (delim-pkt | flush-pkt)
> > > > +
> > > > +unpack-error = PKT-LINE("ERR" SP error-msg LF)
> > > > +
> > > > +ref-update-status = *(update-result | update-error)
> > > > +update-result = *PKT-LINE(txn_id SP result LF)
> > > > +result = ("created" | "deleted" | "updated") SP refname SP old_oid
> > > > SP new_oid
> > > > +update-error = PKT-LINE(txn_id SP "error" SP error-msg LF)
> > >
> > > Can we unify "ERR" and "error" ?
> >
> > No, these are very different. You could have one ref update succeed
> > while another doesn't for some reason, unless you want everything to be
> > atomic.
>
> I did not mean to unify them on the semantic level, but on the
> representation level, i.e. have both of them spelled the same,
> as they can still be differentiated by the leading txn id?
Oh I misunderstood again :) yeas we could standardize on ERR.
>
>
> Thanks,
> Stefan
>
> P.S.: another feature that just came to mind is localisation of error
> messages.
> But that is also easy to do with capabilities (the client sends a capability
> such as "preferred-i18n=DE" and the server may translate all its errors
> if it can.
>
> That brings me to another point: do we assume all errors to be read
> by humans? or do we want some markup things in there, too, similar to
> EAGAIN?
This sort of thing could be added as a protocol-level capability where
the client sends LANG= so that those sorts of msgs could
be translated server side before sending them.
--
Brandon Williams
Re: [RFC] push: add documentation on push v2
On 07/18, Duy Nguyen wrote: > On Wed, Jul 18, 2018 at 7:13 PM Brandon Williams wrote: > > > > > What I've got now is a rough design for a more flexible push, more > > > > > flexible because it allows for the server to do what it wants with the > > > > > refs that are pushed and has the ability to communicate back what was > > > > > done to the client. The main motivation for this is to work around > > > > > issues when working with Gerrit and other code-review systems where > > > > > you > > > > > need to have Change-Ids in the commit messages (now the server can > > > > > just > > > > > insert them for you and send back new commits) and you need to push to > > > > > magic refs to get around various limitations (now a Gerrit server > > > > > should > > > > > be able to communicate that pushing to 'master' doesn't update master > > > > > but instead creates a refs/changes/ ref). > > > > Well Gerrit is our main motivation, but this allows for other workflows > > > > as well. > > > > For example Facebook uses hg internally and they have a > > > > "rebase-on-the-server-after-push" workflow IIRC as pushing to a single > > > > repo > > > > brings up quite some contention. The protocol outlined below would allow > > > > for such a workflow as well? (This might be an easier sell to the Git > > > > community as most are not quite familiar with Gerrit) > > > > > > I'm also curious how this "change commits on push" would be helpful to > > > other > > > scenarios. > > > > > > Since I'm not familiar with Gerrit: what is preventing you from having a > > > commit hook that inserts (or requests) a Change-Id when not present? How > > > can > > > the server identify the Change-Id automatically when it isn't present? > > > > Right now all Gerrit users have a commit hook installed which inserts > > the Change-Id. The issue is that if you push to gerrit and you don't > > have Change-ids, the push fails and you're prompted to blindly run a > > command to install the commit-hook. So if we could just have the server > > handle this completely then the users of gerrit wouldn't ever need to > > have a hook installed in the first place. > > I don't trust the server side to rewrite commits for me. And this is That's a fair point. Though I think there are a number of projects where this would be very beneficial for contributors. The main reason for wanting a feature like this is to make the UX easier for Gerrit users (Having server insert change ids as well as potentially getting rid of the weird HEAD:refs/for/master syntax you need to push for review). Also, if you don't control a server yourself, then who ever controls it can do what it wants with the objects/ref-updates you send them. Of course even if they rewrite history that doesn't mean your local copy needs to mimic those changes if you don't want them too. So even if we move forward with a design like this, there would need to be some config option to actually accept and apply the changes a server makes and sends back to you. This RFC doesn't actually address those sorts of UX implications because I expect those are things which can be added and tweaked at some point in the future. I'm just trying to build the foundation for such changes. > basically rewriting history (e.g. I can push multiple commits to > gerrit if I remember correctly; if they all don't have change-id, then > the history must be rewritten for change-id to be inserted). Don't we > already have "plans" to push config from server to client? There's I know there has been talk about this, but I don't know any of any current proposals or work being done in this area. > also talk about configuring hooks with config file. These should make > it possible to deal with change-id generation with minimum manual > intervention. -- Brandon Williams
Re: [RFC] push: add documentation on push v2
On 07/18, Stefan Beller wrote: > On Wed, Jul 18, 2018 at 6:31 AM Derrick Stolee wrote: > > > > On 7/17/2018 7:25 PM, Stefan Beller wrote: > > > On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams > > > wrote: > > >> Signed-off-by: Brandon Williams > > >> --- > > >> > > >> Since introducing protocol v2 and enabling fetch I've been thinking > > >> about what its inverse 'push' would look like. After talking with a > > >> number of people I have a longish list of things that could be done to > > >> improve push and I think I've been able to distill the core features we > > >> want in push v2. > > > It would be nice to know which things you want to improve. > > > > Hopefully we can also get others to chime in with things they don't like > > about the existing protocol. What pain points exist, and what can we do > > to improve at the transport layer before considering new functionality? > > Another thing that I realized last night was the possibility to chunk > requests. > The web of today is driven by lots of small http(s) requests. I know our > server > team fights with the internal tools all the time because the communication > involved in git-fetch is usually a large http request (large packfile). > So it would be nice to have the possibility of chunking the request. > But I think that can be added as a capability? (Not sure how) Fetch and push requests/responses are already "chunked" when using the http transport. So I'm not sure what you mean by adding a capability because the protocol doesn't care about which transport you're using. This is of course unless you're talking about a different "chunking" from what it means to chunk an http request/response. -- Brandon Williams
Re: [RFC] push: add documentation on push v2
On 07/18, Derrick Stolee wrote: > On 7/17/2018 7:25 PM, Stefan Beller wrote: > > On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote: > > > Signed-off-by: Brandon Williams > > > --- > > > > > > Since introducing protocol v2 and enabling fetch I've been thinking > > > about what its inverse 'push' would look like. After talking with a > > > number of people I have a longish list of things that could be done to > > > improve push and I think I've been able to distill the core features we > > > want in push v2. > > It would be nice to know which things you want to improve. > > Hopefully we can also get others to chime in with things they don't like > about the existing protocol. What pain points exist, and what can we do to > improve at the transport layer before considering new functionality? > > > > Thankfully (due to the capability system) most of the > > > other features/improvements can be added later with ease. > > > > > > What I've got now is a rough design for a more flexible push, more > > > flexible because it allows for the server to do what it wants with the > > > refs that are pushed and has the ability to communicate back what was > > > done to the client. The main motivation for this is to work around > > > issues when working with Gerrit and other code-review systems where you > > > need to have Change-Ids in the commit messages (now the server can just > > > insert them for you and send back new commits) and you need to push to > > > magic refs to get around various limitations (now a Gerrit server should > > > be able to communicate that pushing to 'master' doesn't update master > > > but instead creates a refs/changes/ ref). > > Well Gerrit is our main motivation, but this allows for other workflows as > > well. > > For example Facebook uses hg internally and they have a > > "rebase-on-the-server-after-push" workflow IIRC as pushing to a single repo > > brings up quite some contention. The protocol outlined below would allow > > for such a workflow as well? (This might be an easier sell to the Git > > community as most are not quite familiar with Gerrit) > > I'm also curious how this "change commits on push" would be helpful to other > scenarios. > > Since I'm not familiar with Gerrit: what is preventing you from having a > commit hook that inserts (or requests) a Change-Id when not present? How can > the server identify the Change-Id automatically when it isn't present? Right now all Gerrit users have a commit hook installed which inserts the Change-Id. The issue is that if you push to gerrit and you don't have Change-ids, the push fails and you're prompted to blindly run a command to install the commit-hook. So if we could just have the server handle this completely then the users of gerrit wouldn't ever need to have a hook installed in the first place. -- Brandon Williams
Re: [RFC] push: add documentation on push v2
On 07/17, Stefan Beller wrote:
> On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote:
> >
> > Signed-off-by: Brandon Williams
> > ---
> >
> > Since introducing protocol v2 and enabling fetch I've been thinking
> > about what its inverse 'push' would look like. After talking with a
> > number of people I have a longish list of things that could be done to
> > improve push and I think I've been able to distill the core features we
> > want in push v2.
>
> It would be nice to know which things you want to improve.
I mean this tackles the main point I want to improve. Others include:
rebase/merge-on-push, push symrefs, and then the inclusion of other
capabilities from v0 like atomic push, signed push, etc.
>
> > Thankfully (due to the capability system) most of the
> > other features/improvements can be added later with ease.
> >
> > What I've got now is a rough design for a more flexible push, more
> > flexible because it allows for the server to do what it wants with the
> > refs that are pushed and has the ability to communicate back what was
> > done to the client. The main motivation for this is to work around
> > issues when working with Gerrit and other code-review systems where you
> > need to have Change-Ids in the commit messages (now the server can just
> > insert them for you and send back new commits) and you need to push to
> > magic refs to get around various limitations (now a Gerrit server should
> > be able to communicate that pushing to 'master' doesn't update master
> > but instead creates a refs/changes/ ref).
>
> Well Gerrit is our main motivation, but this allows for other workflows as
> well.
> For example Facebook uses hg internally and they have a
> "rebase-on-the-server-after-push" workflow IIRC as pushing to a single repo
> brings up quite some contention. The protocol outlined below would allow
> for such a workflow as well? (This might be an easier sell to the Git
> community as most are not quite familiar with Gerrit)
Yes the idea would be such that we could easily add a "rebase" or
"merge" verb later for explicit user controlled workflows like that.
This proposal would already make it possible (although the server would
need to be configured as such) since the server can do what it wants
with the updates a client sends to it.
>
> > Before actually moving to write any code I'm hoping to get some feedback
> > on if we think this is an acceptable base design for push (other
> > features like atomic-push, signed-push, etc can be added as
> > capabilities), so any comments are appreciated.
> >
> > Documentation/technical/protocol-v2.txt | 76 +
> > 1 file changed, 76 insertions(+)
> >
> > diff --git a/Documentation/technical/protocol-v2.txt
> > b/Documentation/technical/protocol-v2.txt
> > index 49bda76d23..16c1ce60dd 100644
> > --- a/Documentation/technical/protocol-v2.txt
> > +++ b/Documentation/technical/protocol-v2.txt
> > @@ -403,6 +403,82 @@ header.
> > 2 - progress messages
> > 3 - fatal error message just before stream aborts
> >
> > + push
> > +~~
> > +
> > +`push` is the command used to push ref-updates and a packfile to a remote
> > +server in v2.
> > +
> > +Additional features not supported in the base command will be advertised
> > +as the value of the command in the capability advertisement in the form
> > +of a space separated list of features: "= "
> > +
> > +The format of a push request is as follows:
> > +
> > +request = *section
> > +section = (ref-updates | packfile)
>
> This reads as if a request consists of sections, which
> each can be a "ref-updates" or a packfile, no order given,
> such that multiple ref-update sections mixed with packfiles
> are possible.
>
> I would assume we'd only want to allow for ref-updates
> followed by the packfile.
>
> Given the example above for "rebase-on-push" though
> it is better to first send the packfile (as that is assumed to
> take longer) and then send the ref updates, such that the
> rebasing could be faster and has no bottleneck.
I don't really follow this logic. I don't think it would change
anything much considering the ref-updates section would usually be
much smaller than the packfile itself, course I don't have any data so
idk.
>
> > + (delim-pkt | flush-pkt)
>
>
>
> > +
> > +ref-updates = PKT-LINE("ref-updates" LF)
> > + *PKT-Line(update/force-update LF)
> > +
> > +update =
[RFC] push: add documentation on push v2
Signed-off-by: Brandon Williams
---
Since introducing protocol v2 and enabling fetch I've been thinking
about what its inverse 'push' would look like. After talking with a
number of people I have a longish list of things that could be done to
improve push and I think I've been able to distill the core features we
want in push v2. Thankfully (due to the capability system) most of the
other features/improvements can be added later with ease.
What I've got now is a rough design for a more flexible push, more
flexible because it allows for the server to do what it wants with the
refs that are pushed and has the ability to communicate back what was
done to the client. The main motivation for this is to work around
issues when working with Gerrit and other code-review systems where you
need to have Change-Ids in the commit messages (now the server can just
insert them for you and send back new commits) and you need to push to
magic refs to get around various limitations (now a Gerrit server should
be able to communicate that pushing to 'master' doesn't update master
but instead creates a refs/changes/ ref).
Before actually moving to write any code I'm hoping to get some feedback
on if we think this is an acceptable base design for push (other
features like atomic-push, signed-push, etc can be added as
capabilities), so any comments are appreciated.
Documentation/technical/protocol-v2.txt | 76 +
1 file changed, 76 insertions(+)
diff --git a/Documentation/technical/protocol-v2.txt
b/Documentation/technical/protocol-v2.txt
index 49bda76d23..16c1ce60dd 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -403,6 +403,82 @@ header.
2 - progress messages
3 - fatal error message just before stream aborts
+ push
+~~
+
+`push` is the command used to push ref-updates and a packfile to a remote
+server in v2.
+
+Additional features not supported in the base command will be advertised
+as the value of the command in the capability advertisement in the form
+of a space separated list of features: "= "
+
+The format of a push request is as follows:
+
+request = *section
+section = (ref-updates | packfile)
+ (delim-pkt | flush-pkt)
+
+ref-updates = PKT-LINE("ref-updates" LF)
+ *PKT-Line(update/force-update LF)
+
+update = txn_id SP action SP refname SP old_oid SP new_oid
+force-update = txn_id SP "force" SP action SP refname SP new_oid
+action = ("create" | "delete" | "update")
+txn_id = 1*DIGIT
+
+packfile = PKT-LINE("packfile" LF)
+ *PKT-LINE(*%x00-ff)
+
+ref-updates section
+ * Transaction id's allow for mapping what was requested to what the
+ server actually did with the ref-update.
+ * Normal ref-updates require that the old value of a ref is supplied so
+ that the server can verify that the reference that is being updated
+ hasn't changed while the request was being processed.
+ * Forced ref-updates only include the new value of a ref as we don't
+ care what the old value was.
+
+packfile section
+ * A packfile MAY not be included if only delete commands are used or if
+ an update only incorperates objects the server already has
+
+The server will receive the packfile, unpack it, then validate each ref-update,
+and it will run any update hooks to make sure that the update is acceptable.
+If all of that is fine, the server will then update the references.
+
+The format of a push response is as follows:
+
+response = *section
+section = (unpack-error | ref-update-status | packfile)
+ (delim-pkt | flush-pkt)
+
+unpack-error = PKT-LINE("ERR" SP error-msg LF)
+
+ref-update-status = *(update-result | update-error)
+update-result = *PKT-LINE(txn_id SP result LF)
+result = ("created" | "deleted" | "updated") SP refname SP old_oid SP
new_oid
+update-error = PKT-LINE(txn_id SP "error" SP error-msg LF)
+
+packfile = PKT-LINE("packfile" LF)
+ *PKT-LINE(*%x00-ff)
+
+ref-update-status section
+ * This section is always included unless there was an error unpacking
+ the packfile sent in the request.
+ * The server is given the freedom to do what it wants with the
+ ref-updates provided in the reqeust. This means that an update sent
+ from the server may result in the creation of a ref or rebasing the
+ update on the server.
+ * If a server creates any new objects due to a ref-update, a packfile
+ MUST be sent back in the response.
+
+packfile section
+ * This section is included if the server decided to do something with
+ the ref-updates that involved creating new objects.
+
server-option
~~~
--
2.18.0.203.gfac676dfb9-goog
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 07/09, Junio C Hamano wrote: > Brandon Williams writes: > > > I agree with this observation, though I'm a bit sad about it. I think > > that having tag auto-following the default is a little confusing (and > > hurts perf[1] when using proto v2) but since thats the way its always been > > we'll have to live with it for now. I think exploring changing the > > defaults might be a good thing to do in the future. But for now we've > > had enough people comment on this lacking functionality that we should > > fix it. > > > > [1] Thankfully it doesn't completely undo what protocol v2 did, as we > > still are able to eliminate refs/changes or refs/pull or other various > > refs which significantly add to the number of refs advertised during > > fetch. > > I thought JTan's <20180618231642.174650-1-jonathanta...@google.com> > showed us a way forward to reduce the overhead even further without > having to be sad ;-). Am I mistaken? That's true, what Jonathan mentioned there would avoid having to send "refs/tags/*" when requesting the refs. The question is do we wait on implementing that functionality (as another feature to fetch) or do we fix this now? -- Brandon Williams
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 07/09, Jonathan Nieder wrote:
> Hi,
>
> Jonathan Tan wrote:
>
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> > *transport,
> > refspec_ref_prefixes(>remote->fetch, _prefixes);
> >
> > if (ref_prefixes.argc &&
> > - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
>
> Makes a lot of sense.
>
> This means that if I run
>
> git fetch origin master
>
> then the ls-refs step will now include refs/tags/* in addition to
> refs/heads/master, erasing some of the gain that protocol v2 brought.
> But since the user didn't pass --no-tags, that is what the user asked
> for.
>
> One could argue that the user didn't *mean* to ask for that. In other
> words, I wonder if the following would better match the user intent:
>
> - introduce a new tagopt value, --tags=auto or something, that means
>that tags are only followed when no refspec is supplied. In other
>words,
>
> git fetch --tags=auto
>
>would perform tag auto-following, while
>
> git fetch --tags=auto
>
>would act like fetch --no-tags.
>
> - make --tags=auto the default for new clones.
>
> What do you think?
>
> Some related thoughts on tag auto-following:
>
> It would be tempting to not use tag auto-following at all in the
> default configuration and just include refs/tags/*:refs/tags/* in the
> default clone refspec. Unfortunately, that would be a pretty
> significant behavior change from the present behavior, since
>
> - it would fetch tags pointing to objects unrelated to the fetched
>history
> - if we have ref-in-want *with* pattern support, it would mean we
>try to fetch all tags on every fetch. As discussed in the
>thread [1], this is expensive and difficult to get right.
> - if an unannotated tag is fast-forwarded, it would allow existing
>tags to be updated
> - it interacts poorly with --prune
> - ...
>
> I actually suspect it's a good direction in the long term, but until
> we address those downsides (see also the occasional discussions on tag
> namespaces), we're not ready for it. The --tags=auto described above
> is a sort of half approximation.
>
> Anyway, this is all a long way of saying that despite the performance
> regression I believe this patch heads in the right direction. The
> performance regression is inevitable in what the user is
> (unintentionally) requesting, and we can address it separately by
> changing the defaults to better match what the user intends to
> request.
I agree with this observation, though I'm a bit sad about it. I think
that having tag auto-following the default is a little confusing (and
hurts perf[1] when using proto v2) but since thats the way its always been
we'll have to live with it for now. I think exploring changing the
defaults might be a good thing to do in the future. But for now we've
had enough people comment on this lacking functionality that we should
fix it.
[1] Thankfully it doesn't completely undo what protocol v2 did, as we
still are able to eliminate refs/changes or refs/pull or other various
refs which significantly add to the number of refs advertised during
fetch.
--
Brandon Williams
Re: [PATCH 07/17] commit: increase commit message buffer size
On 07/09, Junio C Hamano wrote:
> Brandon Williams writes:
>
> >> > > diff --git a/refs/files-backend.c b/refs/files-backend.c
> >> > > index a9a066dcfb..252f835bae 100644
> >> > > --- a/refs/files-backend.c
> >> > > +++ b/refs/files-backend.c
> >> > > @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct
> >> > > object_id *old_oid,
> >> > > char *logrec;
> >> > >
> >> > > msglen = msg ? strlen(msg) : 0;
> >> > > - maxlen = strlen(committer) + msglen + 100;
> >> > > + maxlen = strlen(committer) + msglen + 200;
> >> > > logrec = xmalloc(maxlen);
> >> > > len = xsnprintf(logrec, maxlen, "%s %s %s\n",
> >> > > oid_to_hex(old_oid),
> >> >
> >> > nit: 100 is not enough anymore, but wasn't a very descriptive value. 200
> >> > may be enough now, but I'm not sure why.
> >>
> >> That line was touched in by Michael in 7bd9bcf372d (refs: split
> >> filesystem-based
> >> refs code into a new file, 2015-11-09) and before that by Ronnie in
> >> 2c6207abbd6
> >> (refs.c: add a function to append a reflog entry to a fd, 2014-12-12)
> >> and introduced
> >> by Junio in 8ac65937d03 (Make sure we do not write bogus reflog
> >> entries., 2007-01-26)
> >> and it appears to me that 2*40 + 5 ought to be sufficient, but no
> >> comments or commit
> >> messages are found as to why we rather choose 100.
> >
> > Whats the reason for not using a strbuf here so that we don't have to
> > play with magic numbers?
>
> Quite a legitimate question.
>
> I suspect that the reason is because the code (even though it now
> sits in a file that was relatively recently creted) predates either
> the introduction or wide adoption of strbuf.
>
> Back when 6de08ae6 ("Log ref updates to logs/refs/",
> 2006-05-17) was done, we already had strbuf.c, but it only had
> read_line() and nothing else back then, so it wouldn't have been
> possible to use a strbuf there.
Fair enough, having never working in the code back then I don't know
what life was like without strbuf.
--
Brandon Williams
Re: [PATCH 07/17] commit: increase commit message buffer size
On 07/09, Stefan Beller wrote: > On Mon, Jul 9, 2018 at 6:09 AM Derrick Stolee wrote: > > > > On 7/8/2018 7:36 PM, brian m. carlson wrote: > > > 100 bytes is not sufficient to ensure we can write a commit message > > > buffer when using a 32-byte hash algorithm. Increase the buffer size to > > > ensure we have sufficient space. > > > > > > Signed-off-by: brian m. carlson > > > --- > > > refs/files-backend.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/refs/files-backend.c b/refs/files-backend.c > > > index a9a066dcfb..252f835bae 100644 > > > --- a/refs/files-backend.c > > > +++ b/refs/files-backend.c > > > @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct > > > object_id *old_oid, > > > char *logrec; > > > > > > msglen = msg ? strlen(msg) : 0; > > > - maxlen = strlen(committer) + msglen + 100; > > > + maxlen = strlen(committer) + msglen + 200; > > > logrec = xmalloc(maxlen); > > > len = xsnprintf(logrec, maxlen, "%s %s %s\n", > > > oid_to_hex(old_oid), > > > > nit: 100 is not enough anymore, but wasn't a very descriptive value. 200 > > may be enough now, but I'm not sure why. > > That line was touched in by Michael in 7bd9bcf372d (refs: split > filesystem-based > refs code into a new file, 2015-11-09) and before that by Ronnie in > 2c6207abbd6 > (refs.c: add a function to append a reflog entry to a fd, 2014-12-12) > and introduced > by Junio in 8ac65937d03 (Make sure we do not write bogus reflog > entries., 2007-01-26) > and it appears to me that 2*40 + 5 ought to be sufficient, but no > comments or commit > messages are found as to why we rather choose 100. Whats the reason for not using a strbuf here so that we don't have to play with magic numbers? -- Brandon Williams
Re: [PATCH v4 9/9] diff.c: add white space mode to move detection that allows indent changes
On 06/28, Stefan Beller wrote:
> The option of --color-moved has proven to be useful as observed on the
> mailing list. However when refactoring sometimes the indentation changes,
> for example when partitioning a functions into smaller helper functions
> the code usually mostly moved around except for a decrease in indentation.
>
> To just review the moved code ignoring the change in indentation, a mode
> to ignore spaces in the move detection as implemented in a previous patch
> would be enough. However the whole move coloring as motivated in commit
> 2e2d5ac (diff.c: color moved lines differently, 2017-06-30), brought
> up the notion of the reviewer being able to trust the move of a "block".
>
> As there are languages such as python, which depend on proper relative
> indentation for the control flow of the program, ignoring any white space
> change in a block would not uphold the promises of 2e2d5ac that allows
> reviewers to pay less attention to the inside of a block, as inside
> the reviewer wants to assume the same program flow.
>
> This new mode of white space ignorance will take this into account and will
> only allow the same white space changes per line in each block. This patch
> even allows only for the same change at the beginning of the lines.
>
> As this is a white space mode, it is made exclusive to other white space
> modes in the move detection.
>
> This patch brings some challenges, related to the detection of blocks.
> We need a white net the catch the possible moved lines, but then need to
s/white/wide/
> +
> +/**
> + * The struct ws_delta holds white space differences between moved lines,
> i.e.
> + * between '+' and '-' lines that have been detected to be a move.
> + * The string contains the difference in leading white spaces, before the
> + * rest of the line is compared using the white space config for move
> + * coloring. The current_longer indicates if the first string in the
> + * comparision is longer than the second.
> + */
> +struct ws_delta {
> + char *string;
> + unsigned int current_longer : 1;
> };
> +#define WS_DELTA_INIT { NULL, 0 }
> +
> +static int compute_ws_delta(const struct emitted_diff_symbol *a,
> + const struct emitted_diff_symbol *b,
> + struct ws_delta *out)
> +{
> + const struct emitted_diff_symbol *longer = a->len > b->len ? a : b;
> + const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;
> + int d = longer->len - shorter->len;
> +
> + out->string = xmemdupz(longer->line, d);
> + out->current_longer = (a == longer);
> +
> + return !strncmp(longer->line + d, shorter->line, shorter->len);
> +}
I'm having a harder time understanding this block. This is used to fill
a ws_delta struct by calculating the whitespace delta between two lines.
If that is the case then why doesn't this function verify that the first
'd' characters in the longer line are indeed whitespace? Also, maybe
this is just because I'm not as familiar with the move detection code,
but how would the whitespace detection handle a line being moved from
being indented with tabs to spaces or vice versa? Is this something
already handled and not an issue?
--
Brandon Williams
Re: [PATCH v4 7/9] diff.c: decouple white space treatment from move detection algorithm
; > + q_to_tab <<-\EOF >text.txt && > + a long line to exceed per-line minimum > + another long line to exceed per-line minimum > + original file > + EOF > + git add text.txt && > + git commit -m "add text" && > + q_to_tab <<-\EOF >text.txt && > + Qa long line to exceed per-line minimum > + Qanother long line to exceed per-line minimum > + new file > + EOF > + > + # Make sure we get a different diff using -w > + git diff --color --color-moved -w | > + grep -v "index" | > + test_decode_color >actual && > + q_to_tab <<-\EOF >expected && > + diff --git a/text.txt b/text.txt > + --- a/text.txt > + +++ b/text.txt > + @@ -1,3 +1,3 @@ > + Qa long line to exceed per-line minimum > + Qanother long line to exceed per-line minimum > + -original file > + +new file > + EOF > + test_cmp expected actual && > + > + # And now ignoring white space only in the move detection > + git diff --color --color-moved \ > + > --color-moved-ws=ignore-all-space,ignore-space-change,ignore-space-at-eol | > + grep -v "index" | > + test_decode_color >actual && > + q_to_tab <<-\EOF >expected && > + diff --git a/text.txt b/text.txt > + --- a/text.txt > + +++ b/text.txt > + @@ -1,3 +1,3 @@ > + -a long line to exceed per-line minimum > + -another long line to exceed per-line minimum > + -original file > + +Qa long line to exceed per-line > minimum > + +Qanother long line to exceed per-line > minimum > + +new file > + EOF > + test_cmp expected actual > ' > > test_done > -- > 2.18.0.399.gad0ab374a1-goog > -- Brandon Williams
Re: [PATCH v4 6/9] diff.c: add a blocks mode for moved code detection
ot; && > + git diff HEAD --no-renames --color-moved=blocks --color >actual.raw && > + grep -v "index" actual.raw | test_decode_color >actual && > + cat <<-\EOF >expected && > + diff --git a/lines.txt b/lines.txt > + --- a/lines.txt > + +++ b/lines.txt > + @@ -1,16 +1,16 @@ > + -long line 1 > + -long line 2 > + -long line 3 > + line 4 > + line 5 > + line 6 > + line 7 > + line 8 > + line 9 > + +long line 1 > + +long line 2 > + +long line 3 > + +long line 14 > + +long line 15 > + +long line 16 > + line 10 > + line 11 > + line 12 > + line 13 > + -long line 14 > + -long line 15 > + -long line 16 > + EOF > + test_cmp expected actual > + Theres an empty line here. Not worth fixing if its the only issue though. -- Brandon Williams
Re: [PATCH 00/12] Kill the_index part2, header file cleanup
On 06/30, Nguyễn Thái Ngọc Duy wrote: > Like part 1 this is also boring. I wanted to drop these 'extern' > everywhere actually, so before I touched any header file in this > series, I did a clean up first. This is the result (and to reduce diff > noise later) I've scanned through the series and it looks good. Again, thanks for putting in the work to get this done. I'm looking forward to the end product :) -- Brandon Williams
Re: [PATCH v3 00/32] object-store: lookup_commit
On 06/29, Junio C Hamano wrote: > Stefan Beller writes: > > > On Fri, Jun 29, 2018 at 11:03 AM Junio C Hamano wrote: > >> > >> Junio C Hamano writes: > >> > >> > One technique these (not just this) recent efforts seem to be > >> > forgetting is to introduce "new" names that take a_repo and then > >> > make the existing one a thin wrapper that calls the new one with > >> > _repo as the argument. > > > > So you'd rather want to see it less invasive done similar to > > NO_THE_INDEX_COMPATIBILITY_MACROS ? Someone (jrnieder?) > > called that a failed experiment, as now we need to carry that baggage > > for quite some time and never cleaned up the started migration; > > only recently Duy started to kill off the_index, which would finish > > that migration? > > I do not think it was a failed experiment at all. In fact, most of > our code is still happy with the single in-core instance of > the_index, and I think it is a mistake to trying to use the variant > that take an istate instance as parameter just for the sake of it. > > IOW, if there is a good concrete reason why it helps to teach the > set of functions involved in a callchain to operate on an in-core > instance of the index_state, passing an istate through the callchain > is a very welcome change. If you do not do so, and then just > replace $foo_cache(...) with $foo_index(_index, ...), that is an > irritating and useless code churn that harms other topics in flight. I 100% think that we need to continue these refactorings with both the object store as well as with the_index (removing the index macros and removing the dependency on global state). The whole compat macros most definitely was a failed experiment as we still haven't rid the code base of them yet. Having two APIs which can be used interchangeably from different pieces of library code which have very different semantics is _very_ difficult for contributors to reason about and work around. Especially when one API relies on implied global state while the other does not. (If global state isn't involved, as in the char[20] -> OID work, then having two APIs is 'ok' so long as you're working towards converging to one API). Having clean APIs makes it incredible easy to reason about sections of code, reason about multi-threading, and implement newer features in a sane manner (talking about submodules here). I implemented submodule support for grep twice, once using a multi-process paradigm because we could not have two repositories open in a single process and then an in-process one once a minimum viable solution for opening multiple repositories in a single process was created. Being able to open up a submodule repository in-process made things infinitely simpler to reason about as well as made it much easier to handle errors. This work needs to continue if we want to improve the submodule experience in git. Yes this might mean there are a few more conflicts when merging a series (because of the scope of these refactorings) but it is well worth it given what the end-state will look like. -- Brandon Williams
Re: [PATCH v3] fetch-pack: support negotiation tip whitelist
On 06/28, Jonathan Tan wrote: > During negotiation, fetch-pack eventually reports as "have" lines all > commits reachable from all refs. Allow the user to restrict the commits > sent in this way by providing a whitelist of tips; only the tips > themselves and their ancestors will be sent. > > Both globs and single objects are supported. > > This feature is only supported for protocols that support connect or > stateless-connect (such as HTTP with protocol v2). > > This will speed up negotiation when the repository has multiple > relatively independent branches (for example, when a repository > interacts with multiple repositories, such as with linux-next [1] and > torvalds/linux [2]), and the user knows which local branch is likely to > have commits in common with the upstream branch they are fetching. > > [1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/ > [2] https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/ > > Signed-off-by: Jonathan Tan > --- > This is on jt/fetch-pack-negotiator. > > > I don't think that would be strange at all, and no where in git do we > > handle heads/* but we do already handle refs/heads/* as well as DWIM > > master. > > > > > and (2) I can't think of anywhere in Git > > > where you can provide either one - it's either SHA-1 and DWIM name, or > > > SHA-1 and refspec, but not all three. > > > > fetch is a perfect example of supporting all three. I can do > > > > git fetch origin SHA1 > > git fetch origin master > > git fetch origin refs/heads/*:refs/heads/* > > OK, Brandon managed to convince me that this is fine. I've included glob > support, supporting the same globs that git notes supports. > --- > Documentation/fetch-options.txt | 16 +++ > builtin/fetch.c | 41 + > fetch-pack.c| 19 +++- > fetch-pack.h| 7 +++ > t/t5510-fetch.sh| 78 + > transport-helper.c | 3 ++ > transport.c | 1 + > transport.h | 10 + > 8 files changed, 173 insertions(+), 2 deletions(-) > > diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt > index 97d3217df..6e4db1738 100644 > --- a/Documentation/fetch-options.txt > +++ b/Documentation/fetch-options.txt > @@ -42,6 +42,22 @@ the current repository has the same history as the source > repository. > .git/shallow. This option updates .git/shallow and accept such > refs. > > +--negotiation-tip=:: > + By default, Git will report, to the server, commits reachable > + from all local refs to find common commits in an attempt to > + reduce the size of the to-be-received packfile. If specified, > + Git will only report commits reachable from the given tips. > + This is useful to speed up fetches when the user knows which > + local ref is likely to have commits in common with the > + upstream ref being fetched. > ++ > +This option may be specified more than once; if so, Git will report > +commits reachable from any of the given commits. > ++ > +The argument to this option may be a glob on ref names, a ref, or the > (possibly > +abbreviated SHA-1 of a commit. Specifying a glob is equivalent to specifying > +this option multiple times, one for each matching ref name. I think you're missing a closing ')' Aside from that nit this patch looks good, thanks! -- Brandon Williams
Re: [PATCH v2] fetch-pack: support negotiation tip whitelist
On 06/28, Jonathan Tan wrote: > > This seems like a pretty difficult to use feature, requiring that I > > provide the actual OIDs. I think a much better UI would probably be to > > accept a number of different things ranging from exact OIDs to actual > > ref names or even better, allowing for ref-patterns which include globs. > > That way I can do the following: > > > > git fetch --negotiation-tip=refs/remotes/my-remote/* my-remote > > > > in order to easily limit the tips to all the refs I have from that > > particular remote. > > Actual ref names are already supported - it uses the same DWIM as > others. As for globs, I thought of supporting both DWIM objects and the > LHS of refspecs, but (1) it might be strange to support master and > refs/heads/* but not heads/*, I don't think that would be strange at all, and no where in git do we handle heads/* but we do already handle refs/heads/* as well as DWIM master. > and (2) I can't think of anywhere in Git > where you can provide either one - it's either SHA-1 and DWIM name, or > SHA-1 and refspec, but not all three. fetch is a perfect example of supporting all three. I can do git fetch origin SHA1 git fetch origin master git fetch origin refs/heads/*:refs/heads/* -- Brandon Williams
Re: [PATCH v2] fetch-pack: support negotiation tip whitelist
test_commit -C "$SERVER" alpha_1 &&
> + test_commit -C "$SERVER" alpha_2 &&
> + git -C "$SERVER" checkout --orphan beta &&
> + test_commit -C "$SERVER" beta_1 &&
> + test_commit -C "$SERVER" beta_2 &&
> +
> + git clone "$URL" client &&
> +
> + if [ "$USE_PROTOCOL_V2" -eq 1 ]
> + then
> + git -C "$SERVER" config protocol.version 2
> + git -C client config protocol.version 2
> + fi &&
> +
> + test_commit -C "$SERVER" beta_s &&
> + git -C "$SERVER" checkout master &&
> + test_commit -C "$SERVER" alpha_s &&
> + git -C "$SERVER" tag -d alpha_1 alpha_2 beta_1 beta_2 &&
> +
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> + --negotiation-tip=alpha_1 --negotiation-tip=beta_1 \
> + origin alpha_s beta_s &&
> +
> + # Ensure that {alpha,beta}_1 are sent as "have", but not {alpha_beta}_2
> + ALPHA_1=$(git -C client rev-parse alpha_1) &&
> + grep "fetch> have $ALPHA_1" trace &&
> + BETA_1=$(git -C client rev-parse beta_1) &&
> + grep "fetch> have $BETA_1" trace &&
> + ALPHA_2=$(git -C client rev-parse alpha_2) &&
> + ! grep "fetch> have $ALPHA_2" trace &&
> + BETA_2=$(git -C client rev-parse beta_2) &&
> + ! grep "fetch> have $BETA_2" trace
> +}
> +
> +test_expect_success '--negotiator-tip limits "have" lines sent' '
> + negotiator_tip server server 0
> +'
> +
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +start_httpd
> +
> +test_expect_success 'fetch with --filter=blob:limit=0 and HTTP' '
> + negotiator_tip "$HTTPD_DOCUMENT_ROOT_PATH/server" \
> + "$HTTPD_URL/smart/server" 1
> +'
> +
> +stop_httpd
> +
> test_done
> diff --git a/transport-helper.c b/transport-helper.c
> index 1f8ff7e942..ad8f7c7726 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -684,6 +684,9 @@ static int fetch(struct transport *transport,
> transport, "filter",
> data->transport_options.filter_options.filter_spec);
>
> + if (data->transport_options.negotiation_tips)
> + warning("Ignoring --negotiation-tip because the protocol does
> not support it.");
> +
> if (data->fetch)
> return fetch_with_fetch(transport, nr_heads, to_fetch);
>
> diff --git a/transport.c b/transport.c
> index a32da30dee..9f10f8ad9f 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -318,6 +318,7 @@ static int fetch_refs_via_pack(struct transport
> *transport,
> args.filter_options = data->options.filter_options;
> args.stateless_rpc = transport->stateless_rpc;
> args.server_options = transport->server_options;
> + args.negotiation_tips = data->options.negotiation_tips;
>
> if (!data->got_remote_heads)
> refs_tmp = get_refs_via_connect(transport, 0, NULL);
> diff --git a/transport.h b/transport.h
> index 7792b08582..d31be5be63 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -25,6 +25,16 @@ struct git_transport_options {
> const char *receivepack;
> struct push_cas_option *cas;
> struct list_objects_filter_options filter_options;
> +
> + /*
> + * This is only used during fetch. See the documentation of
> + * negotiation_tips in struct fetch_pack_args.
> + *
> + * This field is only supported by transports that support connect or
> + * stateless_connect. Set this field directly instead of using
> + * transport_set_option().
> + */
> + struct oid_array *negotiation_tips;
> };
>
> enum transport_family {
> --
> 2.18.0.rc2.346.g013aa6912e-goog
>
--
Brandon Williams
[PATCH v6 0/8] ref-in-want
Changes in v6: * Stricter checks in tests * Change the name of a test to better reflect what is being tested * Various style issues fixed in shell scripts * Renamed one-time-sed.sh to apply-one-time-sed.sh * Client now errors out when an unexpected ref is sent from the server during the wanted-ref section. Also changed the docs around the server's responsibility with the refs that are sent during this section. Brandon Williams (8): test-pkt-line: add unpack-sideband subcommand upload-pack: implement ref-in-want upload-pack: test negotiation with changing repository fetch: refactor the population of peer ref OIDs fetch: refactor fetch_refs into two functions fetch: refactor to make function args narrower fetch-pack: put shallow info in output parameter fetch-pack: implement ref-in-want Documentation/config.txt| 7 + Documentation/technical/protocol-v2.txt | 28 +- builtin/clone.c | 4 +- builtin/fetch.c | 135 + fetch-object.c | 2 +- fetch-pack.c| 53 +++- remote.c| 1 + remote.h| 1 + t/helper/test-pkt-line.c| 33 +++ t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 8 + t/lib-httpd/apply-one-time-sed.sh | 22 ++ t/t5703-upload-pack-ref-in-want.sh | 377 transport-helper.c | 6 +- transport-internal.h| 9 +- transport.c | 34 ++- transport.h | 3 +- upload-pack.c | 66 + 18 files changed, 715 insertions(+), 75 deletions(-) create mode 100644 t/lib-httpd/apply-one-time-sed.sh create mode 100755 t/t5703-upload-pack-ref-in-want.sh -- 2.18.0.399.gad0ab374a1-goog
[PATCH v6 5/8] fetch: refactor fetch_refs into two functions
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them. This is in preparation
for allowing additional processing of the fetched refs before updating
the local ref store.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 545635448..2fabfed0e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -968,9 +968,21 @@ static int fetch_refs(struct transport *transport, struct
ref *ref_map)
if (ret)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
- ret |= store_updated_refs(transport->url,
- transport->remote->name,
- ref_map);
+ /*
+* Keep the new pack's ".keep" file around to allow the caller
+* time to update refs to reference the new objects.
+*/
+ return 0;
+ transport_unlock_pack(transport);
+ return ret;
+}
+
+/* Update local refs based on the ref values fetched from a remote */
+static int consume_refs(struct transport *transport, struct ref *ref_map)
+{
+ int ret = store_updated_refs(transport->url,
+transport->remote->name,
+ref_map);
transport_unlock_pack(transport);
return ret;
}
@@ -1116,7 +1128,8 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_refs(transport, ref_map);
+ if (!fetch_refs(transport, ref_map))
+ consume_refs(transport, ref_map);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1165,7 +1178,7 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map)) {
+ if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
free_refs(ref_map);
retcode = 1;
goto cleanup;
--
2.18.0.399.gad0ab374a1-goog
[PATCH v6 2/8] upload-pack: implement ref-in-want
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement and
there exists replication delay.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref " parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams
---
Documentation/config.txt| 7 ++
Documentation/technical/protocol-v2.txt | 28 -
t/t5703-upload-pack-ref-in-want.sh | 160
upload-pack.c | 66 ++
4 files changed, 260 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a..fb1dd7428 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it
is seen in the
repository-level config (this is a safety measure against fetching from
untrusted repositories).
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command. This feature
+ is intended for the benefit of load-balanced servers which may
+ not have the same view of what OIDs their refs point to due to
+ replication delay.
+
url..insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with . In cases where some site serves a
diff --git a/Documentation/technical/protocol-v2.txt
b/Documentation/technical/protocol-v2.txt
index 49bda76d2..1da71d0dd 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -299,12 +299,21 @@ included in the client's request:
for use with partial clone and partial fetch operations. See
`rev-list` for possible "filter-spec" values.
+If the 'ref-in-want' feature is advertised, the following argument can
+be included in the client's request as well as the potential addition of
+the 'wanted-refs' section in the server's response as explained below.
+
+want-ref
+ Indicates to the server that the client wants to retrieve a
+ particular ref, where is the full name of a ref on the
+ server.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
output = *section
-section = (acknowledgments | shallow-info | packfile)
+section = (acknowledgments | shallow-info | wanted-refs | packfile)
(flush-pkt | delim-pkt)
acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -319,6 +328,10 @@ header.
shallow = "shallow" SP obj-id
unshallow = "unshallow" SP obj-id
+wanted-refs = PKT-LINE("wanted-refs" LF)
+ *PKT-LINE(wanted-ref LF)
+wanted-ref = obj-id SP refname
+
packfile = PKT-LINE("packfile" LF)
*PKT-LINE(%x01-03 *%x00-ff)
@@ -379,6 +392,19 @@ header.
* This section is only included if a packfile section is also
included in the response.
+wanted-refs section
+ * This section is only included if the client has requested a
+ ref using a 'want-ref' line and if a packfile section is also
+ included in the response.
+
+ * Always begins with the section header "wanted-refs".
+
+ * The server will send a ref listing (" ") for
+ each reference requested using 'want-ref' lines.
+
+ * The server MUST NOT send any refs which were not requested
+ using 'want-ref' lines.
+
packfile section
* This section is only included if the client has sent 'want'
lines in its request and either requested that no more
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
new file mode 100755
index 0..da86f7cde
--- /dev/null
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -0,0 +1,160 @@
+#!/bin/sh
+
+test_description='upload-pack ref-in-want'
+
+. ./test-lib.sh
+
+get_actual_refs () {
+ sed -n -e '/wanted-refs/,/0001/{
+ /wanted-refs/d
+ /0001/d
+ p
+ }' actual_refs
+}
+
+get_actual_commits () {
+ sed -n -e '/packfile/,//{
+ /packfile/d
+ p
+
[PATCH v6 3/8] upload-pack: test negotiation with changing repository
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf| 8
t/lib-httpd/apply-one-time-sed.sh | 22 ++
t/t5703-upload-pack-ref-in-want.sh | 68 ++
4 files changed, 99 insertions(+)
create mode 100644 t/lib-httpd/apply-one-time-sed.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..ed41b155a 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+ install_script apply-one-time-sed.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 724d9ae46..581c010d8 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -111,9 +111,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
Options FollowSymlinks
@@ -123,6 +128,9 @@ ScriptAlias /error/ error.sh/
Options ExecCGI
+
+ Options ExecCGI
+
Options ExecCGI
diff --git a/t/lib-httpd/apply-one-time-sed.sh
b/t/lib-httpd/apply-one-time-sed.sh
new file mode 100644
index 0..fcef72892
--- /dev/null
+++ b/t/lib-httpd/apply-one-time-sed.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+# If "one-time-sed" exists in $HTTPD_ROOT_PATH, run sed on the HTTP response,
+# using the contents of "one-time-sed" as the sed command to be run. If the
+# response was modified as a result, delete "one-time-sed" so that subsequent
+# HTTP responses are no longer modified.
+#
+# This can be used to simulate the effects of the repository changing in
+# between HTTP request-response pairs.
+if [ -e one-time-sed ]; then
+ "$GIT_EXEC_PATH/git-http-backend" >out
+ sed "$(cat one-time-sed)" out_modified
+
+ if diff out out_modified >/dev/null; then
+ cat out
+ else
+ cat out_modified
+ rm one-time-sed
+ fi
+else
+ "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index da86f7cde..32527a59c 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -157,4 +157,72 @@ test_expect_success 'want-ref with ref we already have
commit for' '
check_output
'
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+ (
+ git init "$REPO" &&
+ cd "$REPO" &&
+ >.git/git-daemon-export-ok &&
+ test_commit m1 &&
+ git tag -d m1 &&
+
+ # Local repo with many commits (so that negotiation will take
+ # more than 1 request/response pair)
+ git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo;
"$LOCAL_PRISTINE" &&
+ cd "$LOCAL_PRISTINE" &&
+ git checkout -b side &&
+ for i in $(seq 1 33); do test_commit s$i; done &&
+
+ # Add novel commits to upstream
+ git checkout master &&
+ cd "$REPO" &&
+ test_commit m2 &&
+ test_commit m3 &&
+ git tag -d m2 m3
+ ) &&
+ git -C "$LOCAL_PRISTINE" remote set-url origin
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo; &&
+ git -C "$LOCAL_PRISTINE" config protocol.version 2
+'
+
+inconsistency () {
+ # Simulate that the server initially reports $2 as the ref
+ # corresponding to $1, and after that, $1 as the ref corresponding to
+ # $1. This co
[PATCH v6 7/8] fetch-pack: put shallow info in output parameter
Expand the transport fetch method signature, by adding an output
parameter, to allow transports to return information about the refs they
have fetched. Then communicate shallow status information through this
mechanism instead of by modifying the input list of refs.
This does require clients to sometimes generate the ref map twice: once
from the list of refs provided by the remote (as is currently done) and
potentially once from the new list of refs that the fetch mechanism
provides.
Signed-off-by: Brandon Williams
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 28
fetch-object.c | 2 +-
fetch-pack.c | 15 ---
transport-helper.c | 6 --
transport-internal.h | 9 -
transport.c | 34 --
transport.h | 3 ++-
8 files changed, 77 insertions(+), 24 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae8..8f86d99c5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1155,7 +1155,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
}
if (!is_local && !complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
@@ -1197,7 +1197,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (is_local)
clone_local(path, git_dir);
else if (refs && complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
update_remote_refs(refs, mapped_refs, remote_head_points_at,
branch_top.buf, reflog_msg.buf, transport,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bda00e826..0347cf016 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -946,11 +946,13 @@ static int quickfetch(struct ref *ref_map)
return check_connected(iterate_ref_map, , );
}
-static int fetch_refs(struct transport *transport, struct ref *ref_map)
+static int fetch_refs(struct transport *transport, struct ref *ref_map,
+ struct ref **updated_remote_refs)
{
int ret = quickfetch(ref_map);
if (ret)
- ret = transport_fetch_refs(transport, ref_map);
+ ret = transport_fetch_refs(transport, ref_map,
+ updated_remote_refs);
if (!ret)
/*
* Keep the new pack's ".keep" file around to allow the caller
@@ -1112,7 +1114,7 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- if (!fetch_refs(transport, ref_map))
+ if (!fetch_refs(transport, ref_map, NULL))
consume_refs(transport, ref_map);
if (gsecondary) {
@@ -1128,6 +1130,7 @@ static int do_fetch(struct transport *transport,
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
+ struct ref *updated_remote_refs = NULL;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
@@ -1178,7 +1181,24 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
+
+ if (fetch_refs(transport, ref_map, _remote_refs)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
+ if (updated_remote_refs) {
+ /*
+* Regenerate ref_map using the updated remote refs. This is
+* to account for additional information which may be provided
+* by the transport (e.g. shallow info).
+*/
+ free_refs(ref_map);
+ ref_map = get_ref_map(transport->remote, updated_remote_refs,
rs,
+ tags, );
+ free_refs(updated_remote_refs);
+ }
+ if (consume_refs(transport, ref_map)) {
free_refs(ref_map);
retcode = 1;
goto cleanup;
diff --git a/fetch-object.c b/fetch-object.c
index 853624f81..48fe63dd6 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -19,7 +19,7 @@ static void fetch_refs(const char *remote_name, struct ref
*ref)
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
[PATCH v6 4/8] fetch: refactor the population of peer ref OIDs
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..545635448 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,6 +351,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
const struct ref *remote_refs;
if (rs->nr)
@@ -458,7 +459,23 @@ static struct ref *get_ref_map(struct transport *transport,
tail = >next;
}
- return ref_remove_duplicates(ref_map);
+ ref_map = ref_remove_duplicates(ref_map);
+
+ for_each_ref(add_existing, _refs);
+ for (rm = ref_map; rm; rm = rm->next) {
+ if (rm->peer_ref) {
+ struct string_list_item *peer_item =
+ string_list_lookup(_refs,
+ rm->peer_ref->name);
+ if (peer_item) {
+ struct object_id *old_oid = peer_item->util;
+ oidcpy(>peer_ref->old_oid, old_oid);
+ }
+ }
+ }
+ string_list_clear(_refs, 1);
+
+ return ref_map;
}
#define STORE_REF_ERROR_OTHER 1
@@ -1110,14 +1127,10 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
- struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
- struct ref *rm;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
- for_each_ref(add_existing, _refs);
-
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
tags = TAGS_SET;
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map);
- for (rm = ref_map; rm; rm = rm->next) {
- if (rm->peer_ref) {
- struct string_list_item *peer_item =
- string_list_lookup(_refs,
- rm->peer_ref->name);
- if (peer_item) {
- struct object_id *old_oid = peer_item->util;
- oidcpy(>peer_ref->old_oid, old_oid);
- }
- }
- }
-
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,
}
cleanup:
- string_list_clear(_refs, 1);
return retcode;
}
--
2.18.0.399.gad0ab374a1-goog
[PATCH v6 6/8] fetch: refactor to make function args narrower
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 52 -
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 2fabfed0e..bda00e826 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -254,9 +254,9 @@ static int will_fetch(struct ref **head, const unsigned
char *sha1)
return 0;
}
-static void find_non_local_tags(struct transport *transport,
- struct ref **head,
- struct ref ***tail)
+static void find_non_local_tags(const struct ref *refs,
+ struct ref **head,
+ struct ref ***tail)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct string_list remote_refs = STRING_LIST_INIT_NODUP;
@@ -264,7 +264,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
for_each_ref(add_existing, _refs);
- for (ref = transport_get_remote_refs(transport, NULL); ref; ref =
ref->next) {
+ for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -338,7 +338,8 @@ static void find_non_local_tags(struct transport *transport,
string_list_clear(_refs, 0);
}
-static struct ref *get_ref_map(struct transport *transport,
+static struct ref *get_ref_map(struct remote *remote,
+ const struct ref *remote_refs,
struct refspec *rs,
int tags, int *autotags)
{
@@ -346,27 +347,11 @@ static struct ref *get_ref_map(struct transport
*transport,
struct ref *rm;
struct ref *ref_map = NULL;
struct ref **tail = _map;
- struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
struct string_list existing_refs = STRING_LIST_INIT_DUP;
- const struct ref *remote_refs;
-
- if (rs->nr)
- refspec_ref_prefixes(rs, _prefixes);
- else if (transport->remote && transport->remote->fetch.nr)
- refspec_ref_prefixes(>remote->fetch, _prefixes);
-
- if (ref_prefixes.argc &&
- (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
- argv_array_push(_prefixes, "refs/tags/");
- }
-
- remote_refs = transport_get_remote_refs(transport, _prefixes);
-
- argv_array_clear(_prefixes);
if (rs->nr) {
struct refspec *fetch_refspec;
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,
if (refmap.nr)
fetch_refspec =
else
- fetch_refspec = >remote->fetch;
+ fetch_refspec = >fetch;
for (i = 0; i < fetch_refspec->nr; i++)
get_fetch_map(ref_map, _refspec->items[i],
_tail, 1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,
die("--refmap option is only meaningful with command-line
refspec(s).");
} else {
/* Use the defaults */
- struct remote *remote = transport->remote;
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, , 0);
else if (tags == TAGS_DEFAULT && *autotags)
- find_non_local_tags(transport, _map, );
+ find_non_local_tags(remote_refs, _map, );
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1143,6 +1127,8 @@ static int do_fetch(struct transport *transport,
struct ref *ref_map;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
+ const struct ref *remote_refs;
+ struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1158,7 +1144,21 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
- ref_map = get_ref_map(transport, rs, tags, );
+ if (rs->nr)
+ r
[PATCH v6 1/8] test-pkt-line: add unpack-sideband subcommand
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams
---
t/helper/test-pkt-line.c | 33 +
1 file changed, 33 insertions(+)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 0f19e53c7..30775f986 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,3 +1,4 @@
+#include "cache.h"
#include "pkt-line.h"
static void pack_line(const char *line)
@@ -48,6 +49,36 @@ static void unpack(void)
}
}
+static void unpack_sideband(void)
+{
+ struct packet_reader reader;
+ packet_reader_init(, 0, NULL, 0,
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+
+ while (packet_reader_read() != PACKET_READ_EOF) {
+ int band;
+ int fd;
+
+ switch (reader.status) {
+ case PACKET_READ_EOF:
+ break;
+ case PACKET_READ_NORMAL:
+ band = reader.line[0] & 0xff;
+ if (band < 1 || band > 2)
+ die("unexpected side band %d", band);
+ fd = band;
+
+ write_or_die(fd, reader.line + 1, reader.pktlen - 1);
+ break;
+ case PACKET_READ_FLUSH:
+ return;
+ case PACKET_READ_DELIM:
+ break;
+ }
+ }
+}
+
int cmd_main(int argc, const char **argv)
{
if (argc < 2)
@@ -57,6 +88,8 @@ int cmd_main(int argc, const char **argv)
pack(argc - 2, argv + 2);
else if (!strcmp(argv[1], "unpack"))
unpack();
+ else if (!strcmp(argv[1], "unpack-sideband"))
+ unpack_sideband();
else
die("invalid argument '%s'", argv[1]);
--
2.18.0.399.gad0ab374a1-goog
[PATCH v6 8/8] fetch-pack: implement ref-in-want
Implement ref-in-want on the client side so that when a server supports
the "ref-in-want" feature, a client will send "want-ref" lines for each
reference the client wants to fetch. This feature allows clients to
tolerate inconsistencies that exist when a remote repository's refs
change during the course of negotiation.
This allows a client to request to request a particular ref without
specifying the OID of the ref. This means that instead of hitting an
error when a ref no longer points at the OID it did at the beginning of
negotiation, negotiation can continue and the value of that ref will be
sent at the termination of negotiation, just before a packfile is sent.
More information on the ref-in-want feature can be found in
Documentation/technical/protocol-v2.txt.
Signed-off-by: Brandon Williams
---
fetch-pack.c | 38 +++-
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 149 +
4 files changed, 186 insertions(+), 3 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 73890b894..0b4a9f288 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf,
static void add_wants(const struct ref *wants, struct strbuf *req_buf)
{
+ int use_ref_in_want = server_supports_feature("fetch", "ref-in-want",
0);
+
for ( ; wants ; wants = wants->next) {
const struct object_id *remote = >old_oid;
- const char *remote_hex;
struct object *o;
/*
@@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct
strbuf *req_buf)
continue;
}
- remote_hex = oid_to_hex(remote);
- packet_buf_write(req_buf, "want %s\n", remote_hex);
+ if (!use_ref_in_want || wants->exact_oid)
+ packet_buf_write(req_buf, "want %s\n",
oid_to_hex(remote));
+ else
+ packet_buf_write(req_buf, "want-ref %s\n", wants->name);
}
}
@@ -1334,6 +1337,32 @@ static void receive_shallow_info(struct fetch_pack_args
*args,
args->deepen = 1;
}
+static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
+{
+ process_section_header(reader, "wanted-refs", 0);
+ while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+ struct object_id oid;
+ const char *end;
+ struct ref *r = NULL;
+
+ if (parse_oid_hex(reader->line, , ) || *end++ != ' ')
+ die("expected wanted-ref, got '%s'", reader->line);
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(>old_oid, );
+ break;
+ }
+ }
+
+ if (!r)
+ die("unexpected wanted-ref: '%s'", reader->line);
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
+ die("error processing wanted refs: %d", reader->status);
+}
+
enum fetch_state {
FETCH_CHECK_LOCAL = 0,
FETCH_SEND_REQUEST,
@@ -1408,6 +1437,9 @@ static struct ref *do_fetch_pack_v2(struct
fetch_pack_args *args,
if (process_section_header(, "shallow-info", 1))
receive_shallow_info(args, );
+ if (process_section_header(, "wanted-refs", 1))
+ receive_wanted_refs(, ref);
+
/* get the pack */
process_section_header(, "packfile", 0);
if (get_pack(args, fd, pack_lockfile))
diff --git a/remote.c b/remote.c
index abe80c139..2c2376fff 100644
--- a/remote.c
+++ b/remote.c
@@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
if (refspec->exact_sha1) {
ref_map = alloc_ref(name);
get_oid_hex(name, _map->old_oid);
+ ref_map->exact_oid = 1;
} else {
ref_map = get_remote_ref(remote_refs, name);
}
diff --git a/remote.h b/remote.h
index 45ecc6cef..976292152 100644
--- a/remote.h
+++ b/remote.h
@@ -73,6 +73,7 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
+ exact_oid:1,
deletion:1;
enum {
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 32527a59c..a73c55a47 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-wan
Re: [PATCH v5 2/8] upload-pack: implement ref-in-want
On 06/27, Junio C Hamano wrote: > Brandon Williams writes: > > >> > +* The server SHOULD NOT send any refs which were not requested > >> > + using 'want-ref' lines and a client MUST ignore refs which > >> > + weren't requested. > >> > >> Just being curious, but the above feels the other way around. Why > >> are we being more lenient to writers of broken server than writers > >> of broken clients? The number of installations they need to take > >> back and replace is certainly lower for the former, which means > >> that, if exchanges of unsoliclited refs are unwanted, clients should > >> notice and barf (or warn) if the server misbehaves, and the server > >> should be forbidden from sending unsolicited refs, no? > > > > Ok so should I change the server part to "MUST NOT" and the client part > > to "SHOULD"? And I can add code to die when we see refs that weren't > > requested, but i feel like if we add an ability to request a pattern in > > the future this will completely change, which is why I currently have a > > client just ignoring anything else. > > I did not have enough information to give an answer to "should I do > X?"; that is why I asked these questions prefixed with "Just being > curious". I do not quite get a good feeling that I now know enough > to answer, still, but let me try. > > If we anticipate backward incompatible changes between this early > WIP stage and the final completed protocol, it would be GOOD to make > sure that an early WIP clients/servers fail when seeing the other > side gives them something they do not understand, no? > > So... Yeah after thinking more about this I agree, we should have the client fail out and require that the server MUST not send additional refs. This can of course be loosened through a capability if we want to do something else in the future. Thanks for sanity checking me :) -- Brandon Williams
Re: [PATCH v5 8/8] fetch-pack: implement ref-in-want
On 06/27, Jonathan Tan wrote: > > +test_expect_success 'setup repos for change-while-negotiating test' ' > > The tests that follow are basic ref-in-want tests, not tests on a repo > that changes during negotiation - this would be just "setup repos for > fetch tests". That looks like a copy-paste error. > > > +test_expect_success 'fetching with exact OID' ' > > + rm -rf local && > > + cp -r "$LOCAL_PRISTINE" local && > > + git -C local fetch origin $(git -C "$REPO" rev-parse > > d):refs/heads/actual && > > + > > + git -C "$REPO" rev-parse "d" >expected && > > + git -C local rev-parse refs/heads/actual >actual && > > + test_cmp expected actual > > +' > > Also verify that "want-ref refs/tags/d" is being sent over the wire, and > not any "want ...". (If not we can't distinguish these from the usual > non-want-ref behavior.) Same comment for the other tests. I think your mistaken on how what this test is looking for. no want-ref line is going to be sent because we're requesting an exact OID here, not a ref. But I can add checks for want-ref in the tests that should be sending want-ref. > > Other than that (and my other comments), this patch series looks good. -- Brandon Williams
Re: [PATCH v5 7/8] fetch-pack: put shallow info in output parameter
On 06/26, Junio C Hamano wrote: > Brandon Williams writes: > > > Expand the transport fetch method signature, by adding an output > > parameter, to allow transports to return information about the refs they > > have fetched. Then communicate shallow status information through this > > mechanism instead of by modifying the input list of refs. > > Makes sense. Would this mechanism also allow us to be more explicit > about the "tag following"? > Yes most likely. We could change it so that when a packfile is sent the result of tag following could be sent along too (the actual tag refs themselves that is) instead of having the client rely on the ref advertisement for tag following. -- Brandon Williams
Re: [PATCH v5 3/8] upload-pack: test negotiation with changing repository
On 06/26, Junio C Hamano wrote:
> Brandon Williams writes:
>
> > diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
> > new file mode 100644
> > index 0..8a9a5aca0
> > --- /dev/null
> > +++ b/t/lib-httpd/one-time-sed.sh
> > @@ -0,0 +1,22 @@
> > +#!/bin/sh
> > +
> > +# If "one-time-sed" exists in $HTTPD_ROOT_PATH, run sed on the HTTP
> > response,
> > +# using the contents of "one-time-sed" as the sed command to be run. If the
> > +# response was modified as a result, delete "one-time-sed" so that
> > subsequent
> > +# HTTP responses are no longer modified.
>
> ;-) clever.
>
> > +#
> > +# This can be used to simulate the effects of the repository changing in
> > +# between HTTP request-response pairs.
> > +if [ -e one-time-sed ]; then "$GIT_EXEC_PATH/git-http-backend" >out
>
> Style (cf. Documentation/CodingGuidelines).
>
> > +
> > + sed "$(cat one-time-sed)" out_modified
> > +
> > + if diff out out_modified >/dev/null; then
> > + cat out
> > + else
> > + cat out_modified
> > + rm one-time-sed
> > + fi
> > +else
> > + "$GIT_EXEC_PATH/git-http-backend"
> > +fi
>
> OK. I was worried if the removal-after-use was about _this_ script
> (in which case it is a bad hygiene), but this is not a one-time
> script, but merely the mechanism to use the one-time sed script.
>
> Perhaps rename this to t/lib-httpd/apply-one-time-sed.sh or something
> to avoid confusion?
Sure, I'll go ahead and rename it to that.
>
> > diff --git a/t/t5703-upload-pack-ref-in-want.sh
> > b/t/t5703-upload-pack-ref-in-want.sh
> > index 0ef182970..a4fe0e7e4 100755
> > --- a/t/t5703-upload-pack-ref-in-want.sh
> > +++ b/t/t5703-upload-pack-ref-in-want.sh
> > @@ -150,4 +150,72 @@ test_expect_success 'want-ref with ref we already have
> > commit for' '
> > check_output
> > '
> >
> > +. "$TEST_DIRECTORY"/lib-httpd.sh
> > +start_httpd
> > +
> > +REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
> > +LOCAL_PRISTINE="$(pwd)/local_pristine"
> > +
> > +test_expect_success 'setup repos for change-while-negotiating test' '
> > + (
> > + git init "$REPO" &&
> > + cd "$REPO" &&
> > + >.git/git-daemon-export-ok &&
> > + test_commit m1 &&
> > + git tag -d m1 &&
> > +
> > + # Local repo with many commits (so that negotiation will take
> > + # more than 1 request/response pair)
> > + git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo;
> > "$LOCAL_PRISTINE" &&
> > + cd "$LOCAL_PRISTINE" &&
> > + git checkout -b side &&
> > + for i in $(seq 1 33); do test_commit s$i; done &&
> > +
> > + # Add novel commits to upstream
> > + git checkout master &&
> > + cd "$REPO" &&
> > + test_commit m2 &&
> > + test_commit m3 &&
> > + git tag -d m2 m3
> > + ) &&
> > + git -C "$LOCAL_PRISTINE" remote set-url origin
> > "http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo; &&
> > + git -C "$LOCAL_PRISTINE" config protocol.version 2
> > +'
> > +
> > +inconsistency() {
>
> Style. "inconsistency () {"
>
> > + # Simulate that the server initially reports $2 as the ref
> > + # corresponding to $1, and after that, $1 as the ref corresponding to
> > + # $1. This corresponds to the real-life situation where the server's
> > + # repository appears to change during negotiation, for example, when
> > + # different servers in a load-balancing arrangement serve (stateless)
> > + # RPCs during a single negotiation.
> > + printf "s/%s/%s/" \
> > + $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
> > + $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
> > + >"$HTTPD_ROOT_PATH/one-time-sed"
> > +}
> > +
> > +test_expect_success 'server is initially ahead - no ref in want' '
> > + git -C "$REPO" config uploadpack.allowRefInWant false &&
> > + rm -rf local &&
> > + cp -r "$LOCAL_PRISTINE" local &&
> > + inconsistency master 1234567890123456789012345678901234567890 &&
> > + test_must_fail git -C local fetch 2>err &&
> > + grep "ERR upload-pack: not our ref" err
> > +'
> > +
> > +test_expect_success 'server is initially behind - no ref in want' '
> > + git -C "$REPO" config uploadpack.allowRefInWant false &&
> > + rm -rf local &&
> > + cp -r "$LOCAL_PRISTINE" local &&
> > + inconsistency master "master^" &&
> > + git -C local fetch &&
> > +
> > + git -C "$REPO" rev-parse --verify "master^" >expected &&
> > + git -C local rev-parse --verify refs/remotes/origin/master >actual &&
> > + test_cmp expected actual
> > +'
> > +
> > +stop_httpd
> > +
> > test_done
--
Brandon Williams
Re: [PATCH v5 2/8] upload-pack: implement ref-in-want
On 06/26, Junio C Hamano wrote:
> Brandon Williams writes:
>
> > +wanted-refs section
> > + * This section is only included if the client has requested a
> > + ref using a 'want-ref' line and if a packfile section is also
> > + included in the response.
> > +
> > + * Always begins with the section header "wanted-refs".
> > +
> > + * The server will send a ref listing (" ") for
> > + each reference requested using 'want-ref' lines.
> > +
> > + * The server SHOULD NOT send any refs which were not requested
> > + using 'want-ref' lines and a client MUST ignore refs which
> > + weren't requested.
>
> Just being curious, but the above feels the other way around. Why
> are we being more lenient to writers of broken server than writers
> of broken clients? The number of installations they need to take
> back and replace is certainly lower for the former, which means
> that, if exchanges of unsoliclited refs are unwanted, clients should
> notice and barf (or warn) if the server misbehaves, and the server
> should be forbidden from sending unsolicited refs, no?
Ok so should I change the server part to "MUST NOT" and the client part
to "SHOULD"? And I can add code to die when we see refs that weren't
requested, but i feel like if we add an ability to request a pattern in
the future this will completely change, which is why I currently have a
client just ignoring anything else.
>
>
> > diff --git a/t/t5703-upload-pack-ref-in-want.sh
> > b/t/t5703-upload-pack-ref-in-want.sh
> > new file mode 100755
> > index 0..0ef182970
> > --- /dev/null
> > +++ b/t/t5703-upload-pack-ref-in-want.sh
> > @@ -0,0 +1,153 @@
> > +#!/bin/sh
> > +
> > +test_description='upload-pack ref-in-want'
> > +
> > +. ./test-lib.sh
> > +
> > +get_actual_refs() {
>
> Style. "get_actual_refs () {"
>
> > + sed -n '/wanted-refs/,/0001/p' > unpack >actual_refs
>
> Unnecessary piping of sed output into another sed invocation?
>
> sed -n -e '/wanted-refs/,/0001/{
> /wanted-refs/d
> /0001/d
> p
> }'
>
> or something like that?
Yeah thanks for the help with sed :)
--
Brandon Williams
[PATCH v5 0/8] ref-in-want
Changes in v5: * Added a comment explaining the one-time-sed.sh script * Added a number of tests per reviewer feedback * Fixed a typo in documentation Brandon Williams (8): test-pkt-line: add unpack-sideband subcommand upload-pack: implement ref-in-want upload-pack: test negotiation with changing repository fetch: refactor the population of peer ref OIDs fetch: refactor fetch_refs into two functions fetch: refactor to make function args narrower fetch-pack: put shallow info in output parameter fetch-pack: implement ref-in-want Documentation/config.txt| 7 + Documentation/technical/protocol-v2.txt | 29 +- builtin/clone.c | 4 +- builtin/fetch.c | 135 + fetch-object.c | 2 +- fetch-pack.c| 50 +++- remote.c| 1 + remote.h| 1 + t/helper/test-pkt-line.c| 33 +++ t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 8 + t/lib-httpd/one-time-sed.sh | 22 ++ t/t5703-upload-pack-ref-in-want.sh | 351 transport-helper.c | 6 +- transport-internal.h| 9 +- transport.c | 34 ++- transport.h | 3 +- upload-pack.c | 66 + 18 files changed, 687 insertions(+), 75 deletions(-) create mode 100644 t/lib-httpd/one-time-sed.sh create mode 100755 t/t5703-upload-pack-ref-in-want.sh -- 2.18.0.rc2.346.g013aa6912e-goog
[PATCH v5 7/8] fetch-pack: put shallow info in output parameter
Expand the transport fetch method signature, by adding an output
parameter, to allow transports to return information about the refs they
have fetched. Then communicate shallow status information through this
mechanism instead of by modifying the input list of refs.
This does require clients to sometimes generate the ref map twice: once
from the list of refs provided by the remote (as is currently done) and
potentially once from the new list of refs that the fetch mechanism
provides.
Signed-off-by: Brandon Williams
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 28
fetch-object.c | 2 +-
fetch-pack.c | 15 ---
transport-helper.c | 6 --
transport-internal.h | 9 -
transport.c | 34 --
transport.h | 3 ++-
8 files changed, 77 insertions(+), 24 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae8..8f86d99c5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1155,7 +1155,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
}
if (!is_local && !complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
@@ -1197,7 +1197,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (is_local)
clone_local(path, git_dir);
else if (refs && complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
update_remote_refs(refs, mapped_refs, remote_head_points_at,
branch_top.buf, reflog_msg.buf, transport,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bda00e826..0347cf016 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -946,11 +946,13 @@ static int quickfetch(struct ref *ref_map)
return check_connected(iterate_ref_map, , );
}
-static int fetch_refs(struct transport *transport, struct ref *ref_map)
+static int fetch_refs(struct transport *transport, struct ref *ref_map,
+ struct ref **updated_remote_refs)
{
int ret = quickfetch(ref_map);
if (ret)
- ret = transport_fetch_refs(transport, ref_map);
+ ret = transport_fetch_refs(transport, ref_map,
+ updated_remote_refs);
if (!ret)
/*
* Keep the new pack's ".keep" file around to allow the caller
@@ -1112,7 +1114,7 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- if (!fetch_refs(transport, ref_map))
+ if (!fetch_refs(transport, ref_map, NULL))
consume_refs(transport, ref_map);
if (gsecondary) {
@@ -1128,6 +1130,7 @@ static int do_fetch(struct transport *transport,
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
+ struct ref *updated_remote_refs = NULL;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
@@ -1178,7 +1181,24 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
+
+ if (fetch_refs(transport, ref_map, _remote_refs)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
+ if (updated_remote_refs) {
+ /*
+* Regenerate ref_map using the updated remote refs. This is
+* to account for additional information which may be provided
+* by the transport (e.g. shallow info).
+*/
+ free_refs(ref_map);
+ ref_map = get_ref_map(transport->remote, updated_remote_refs,
rs,
+ tags, );
+ free_refs(updated_remote_refs);
+ }
+ if (consume_refs(transport, ref_map)) {
free_refs(ref_map);
retcode = 1;
goto cleanup;
diff --git a/fetch-object.c b/fetch-object.c
index 853624f81..48fe63dd6 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -19,7 +19,7 @@ static void fetch_refs(const char *remote_name, struct ref
*ref)
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
[PATCH v5 8/8] fetch-pack: implement ref-in-want
Implement ref-in-want on the client side so that when a server supports
the "ref-in-want" feature, a client will send "want-ref" lines for each
reference the client wants to fetch. This feature allows clients to
tolerate inconsistencies that exist when a remote repository's refs
change during the course of negotiation.
This allows a client to request to request a particular ref without
specifying the OID of the ref. This means that instead of hitting an
error when a ref no longer points at the OID it did at the beginning of
negotiation, negotiation can continue and the value of that ref will be
sent at the termination of negotiation, just before a packfile is sent.
More information on the ref-in-want feature can be found in
Documentation/technical/protocol-v2.txt.
Signed-off-by: Brandon Williams
---
fetch-pack.c | 35 +++-
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 130 +
4 files changed, 164 insertions(+), 3 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 73890b894..3a18f5bcd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf,
static void add_wants(const struct ref *wants, struct strbuf *req_buf)
{
+ int use_ref_in_want = server_supports_feature("fetch", "ref-in-want",
0);
+
for ( ; wants ; wants = wants->next) {
const struct object_id *remote = >old_oid;
- const char *remote_hex;
struct object *o;
/*
@@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct
strbuf *req_buf)
continue;
}
- remote_hex = oid_to_hex(remote);
- packet_buf_write(req_buf, "want %s\n", remote_hex);
+ if (!use_ref_in_want || wants->exact_oid)
+ packet_buf_write(req_buf, "want %s\n",
oid_to_hex(remote));
+ else
+ packet_buf_write(req_buf, "want-ref %s\n", wants->name);
}
}
@@ -1334,6 +1337,29 @@ static void receive_shallow_info(struct fetch_pack_args
*args,
args->deepen = 1;
}
+static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
+{
+ process_section_header(reader, "wanted-refs", 0);
+ while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+ struct object_id oid;
+ const char *end;
+ struct ref *r = NULL;
+
+ if (parse_oid_hex(reader->line, , ) || *end++ != ' ')
+ die("expected wanted-ref, got '%s'", reader->line);
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(>old_oid, );
+ break;
+ }
+ }
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
+ die("error processing wanted refs: %d", reader->status);
+}
+
enum fetch_state {
FETCH_CHECK_LOCAL = 0,
FETCH_SEND_REQUEST,
@@ -1408,6 +1434,9 @@ static struct ref *do_fetch_pack_v2(struct
fetch_pack_args *args,
if (process_section_header(, "shallow-info", 1))
receive_shallow_info(args, );
+ if (process_section_header(, "wanted-refs", 1))
+ receive_wanted_refs(, ref);
+
/* get the pack */
process_section_header(, "packfile", 0);
if (get_pack(args, fd, pack_lockfile))
diff --git a/remote.c b/remote.c
index abe80c139..2c2376fff 100644
--- a/remote.c
+++ b/remote.c
@@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
if (refspec->exact_sha1) {
ref_map = alloc_ref(name);
get_oid_hex(name, _map->old_oid);
+ ref_map->exact_oid = 1;
} else {
ref_map = get_remote_ref(remote_refs, name);
}
diff --git a/remote.h b/remote.h
index 45ecc6cef..976292152 100644
--- a/remote.h
+++ b/remote.h
@@ -73,6 +73,7 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
+ exact_oid:1,
deletion:1;
enum {
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index a4fe0e7e4..392be4959 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -204,6 +204,18 @@ test_expect_success 'server is initially ahead - no ref in
want' '
grep "ER
[PATCH v5 6/8] fetch: refactor to make function args narrower
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 52 -
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 2fabfed0e..bda00e826 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -254,9 +254,9 @@ static int will_fetch(struct ref **head, const unsigned
char *sha1)
return 0;
}
-static void find_non_local_tags(struct transport *transport,
- struct ref **head,
- struct ref ***tail)
+static void find_non_local_tags(const struct ref *refs,
+ struct ref **head,
+ struct ref ***tail)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct string_list remote_refs = STRING_LIST_INIT_NODUP;
@@ -264,7 +264,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
for_each_ref(add_existing, _refs);
- for (ref = transport_get_remote_refs(transport, NULL); ref; ref =
ref->next) {
+ for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -338,7 +338,8 @@ static void find_non_local_tags(struct transport *transport,
string_list_clear(_refs, 0);
}
-static struct ref *get_ref_map(struct transport *transport,
+static struct ref *get_ref_map(struct remote *remote,
+ const struct ref *remote_refs,
struct refspec *rs,
int tags, int *autotags)
{
@@ -346,27 +347,11 @@ static struct ref *get_ref_map(struct transport
*transport,
struct ref *rm;
struct ref *ref_map = NULL;
struct ref **tail = _map;
- struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
struct string_list existing_refs = STRING_LIST_INIT_DUP;
- const struct ref *remote_refs;
-
- if (rs->nr)
- refspec_ref_prefixes(rs, _prefixes);
- else if (transport->remote && transport->remote->fetch.nr)
- refspec_ref_prefixes(>remote->fetch, _prefixes);
-
- if (ref_prefixes.argc &&
- (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
- argv_array_push(_prefixes, "refs/tags/");
- }
-
- remote_refs = transport_get_remote_refs(transport, _prefixes);
-
- argv_array_clear(_prefixes);
if (rs->nr) {
struct refspec *fetch_refspec;
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,
if (refmap.nr)
fetch_refspec =
else
- fetch_refspec = >remote->fetch;
+ fetch_refspec = >fetch;
for (i = 0; i < fetch_refspec->nr; i++)
get_fetch_map(ref_map, _refspec->items[i],
_tail, 1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,
die("--refmap option is only meaningful with command-line
refspec(s).");
} else {
/* Use the defaults */
- struct remote *remote = transport->remote;
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, , 0);
else if (tags == TAGS_DEFAULT && *autotags)
- find_non_local_tags(transport, _map, );
+ find_non_local_tags(remote_refs, _map, );
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1143,6 +1127,8 @@ static int do_fetch(struct transport *transport,
struct ref *ref_map;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
+ const struct ref *remote_refs;
+ struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1158,7 +1144,21 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
- ref_map = get_ref_map(transport, rs, tags, );
+ if (rs->nr)
+ r
[PATCH v5 3/8] upload-pack: test negotiation with changing repository
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf| 8
t/lib-httpd/one-time-sed.sh| 22 ++
t/t5703-upload-pack-ref-in-want.sh | 68 ++
4 files changed, 99 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..84f8efdd4 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+ install_script one-time-sed.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 724d9ae46..fe68d37bb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -111,9 +111,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1
Options FollowSymlinks
@@ -123,6 +128,9 @@ ScriptAlias /error/ error.sh/
Options ExecCGI
+
+ Options ExecCGI
+
Options ExecCGI
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..8a9a5aca0
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+# If "one-time-sed" exists in $HTTPD_ROOT_PATH, run sed on the HTTP response,
+# using the contents of "one-time-sed" as the sed command to be run. If the
+# response was modified as a result, delete "one-time-sed" so that subsequent
+# HTTP responses are no longer modified.
+#
+# This can be used to simulate the effects of the repository changing in
+# between HTTP request-response pairs.
+if [ -e one-time-sed ]; then "$GIT_EXEC_PATH/git-http-backend" >out
+
+ sed "$(cat one-time-sed)" out_modified
+
+ if diff out out_modified >/dev/null; then
+ cat out
+ else
+ cat out_modified
+ rm one-time-sed
+ fi
+else
+ "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 0ef182970..a4fe0e7e4 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -150,4 +150,72 @@ test_expect_success 'want-ref with ref we already have
commit for' '
check_output
'
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+ (
+ git init "$REPO" &&
+ cd "$REPO" &&
+ >.git/git-daemon-export-ok &&
+ test_commit m1 &&
+ git tag -d m1 &&
+
+ # Local repo with many commits (so that negotiation will take
+ # more than 1 request/response pair)
+ git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo;
"$LOCAL_PRISTINE" &&
+ cd "$LOCAL_PRISTINE" &&
+ git checkout -b side &&
+ for i in $(seq 1 33); do test_commit s$i; done &&
+
+ # Add novel commits to upstream
+ git checkout master &&
+ cd "$REPO" &&
+ test_commit m2 &&
+ test_commit m3 &&
+ git tag -d m2 m3
+ ) &&
+ git -C "$LOCAL_PRISTINE" remote set-url origin
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo; &&
+ git -C "$LOCAL_PRISTINE" config protocol.version 2
+'
+
+inconsistency() {
+ # Simulate that the server initially reports $2 as the ref
+ # corresponding to $1, and after that, $1 as the ref corresponding to
+ # $1. This corresponds to the real-life situation where the se
[PATCH v5 2/8] upload-pack: implement ref-in-want
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement and
there exists replication delay.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref " parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams
---
Documentation/config.txt| 7 ++
Documentation/technical/protocol-v2.txt | 29 -
t/t5703-upload-pack-ref-in-want.sh | 153
upload-pack.c | 66 ++
4 files changed, 254 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a..fb1dd7428 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it
is seen in the
repository-level config (this is a safety measure against fetching from
untrusted repositories).
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command. This feature
+ is intended for the benefit of load-balanced servers which may
+ not have the same view of what OIDs their refs point to due to
+ replication delay.
+
url..insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with . In cases where some site serves a
diff --git a/Documentation/technical/protocol-v2.txt
b/Documentation/technical/protocol-v2.txt
index 49bda76d2..b53cfc6ce 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -299,12 +299,21 @@ included in the client's request:
for use with partial clone and partial fetch operations. See
`rev-list` for possible "filter-spec" values.
+If the 'ref-in-want' feature is advertised, the following argument can
+be included in the client's request as well as the potential addition of
+the 'wanted-refs' section in the server's response as explained below.
+
+want-ref
+ Indicates to the server that the client wants to retrieve a
+ particular ref, where is the full name of a ref on the
+ server.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
output = *section
-section = (acknowledgments | shallow-info | packfile)
+section = (acknowledgments | shallow-info | wanted-refs | packfile)
(flush-pkt | delim-pkt)
acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -319,6 +328,10 @@ header.
shallow = "shallow" SP obj-id
unshallow = "unshallow" SP obj-id
+wanted-refs = PKT-LINE("wanted-refs" LF)
+ *PKT-LINE(wanted-ref LF)
+wanted-ref = obj-id SP refname
+
packfile = PKT-LINE("packfile" LF)
*PKT-LINE(%x01-03 *%x00-ff)
@@ -379,6 +392,20 @@ header.
* This section is only included if a packfile section is also
included in the response.
+wanted-refs section
+ * This section is only included if the client has requested a
+ ref using a 'want-ref' line and if a packfile section is also
+ included in the response.
+
+ * Always begins with the section header "wanted-refs".
+
+ * The server will send a ref listing (" ") for
+ each reference requested using 'want-ref' lines.
+
+ * The server SHOULD NOT send any refs which were not requested
+ using 'want-ref' lines and a client MUST ignore refs which
+ weren't requested.
+
packfile section
* This section is only included if the client has sent 'want'
lines in its request and either requested that no more
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
new file mode 100755
index 0..0ef182970
--- /dev/null
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -0,0 +1,153 @@
+#!/bin/sh
+
+test_description='upload-pack ref-in-want'
+
+. ./test-lib.sh
+
+get_actual_refs() {
+ sed -n '/wanted-refs/,/0001/p' actual_refs
+}
+
+get_actual_commits() {
+ sed -n '/packfile/,//p' o.pack &&
+ git index-pack o.pack &&
+ git v
[PATCH v5 4/8] fetch: refactor the population of peer ref OIDs
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..545635448 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,6 +351,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
const struct ref *remote_refs;
if (rs->nr)
@@ -458,7 +459,23 @@ static struct ref *get_ref_map(struct transport *transport,
tail = >next;
}
- return ref_remove_duplicates(ref_map);
+ ref_map = ref_remove_duplicates(ref_map);
+
+ for_each_ref(add_existing, _refs);
+ for (rm = ref_map; rm; rm = rm->next) {
+ if (rm->peer_ref) {
+ struct string_list_item *peer_item =
+ string_list_lookup(_refs,
+ rm->peer_ref->name);
+ if (peer_item) {
+ struct object_id *old_oid = peer_item->util;
+ oidcpy(>peer_ref->old_oid, old_oid);
+ }
+ }
+ }
+ string_list_clear(_refs, 1);
+
+ return ref_map;
}
#define STORE_REF_ERROR_OTHER 1
@@ -1110,14 +1127,10 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
- struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
- struct ref *rm;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
- for_each_ref(add_existing, _refs);
-
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
tags = TAGS_SET;
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map);
- for (rm = ref_map; rm; rm = rm->next) {
- if (rm->peer_ref) {
- struct string_list_item *peer_item =
- string_list_lookup(_refs,
- rm->peer_ref->name);
- if (peer_item) {
- struct object_id *old_oid = peer_item->util;
- oidcpy(>peer_ref->old_oid, old_oid);
- }
- }
- }
-
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,
}
cleanup:
- string_list_clear(_refs, 1);
return retcode;
}
--
2.18.0.rc2.346.g013aa6912e-goog
[PATCH v5 5/8] fetch: refactor fetch_refs into two functions
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them. This is in preparation
for allowing additional processing of the fetched refs before updating
the local ref store.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 545635448..2fabfed0e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -968,9 +968,21 @@ static int fetch_refs(struct transport *transport, struct
ref *ref_map)
if (ret)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
- ret |= store_updated_refs(transport->url,
- transport->remote->name,
- ref_map);
+ /*
+* Keep the new pack's ".keep" file around to allow the caller
+* time to update refs to reference the new objects.
+*/
+ return 0;
+ transport_unlock_pack(transport);
+ return ret;
+}
+
+/* Update local refs based on the ref values fetched from a remote */
+static int consume_refs(struct transport *transport, struct ref *ref_map)
+{
+ int ret = store_updated_refs(transport->url,
+transport->remote->name,
+ref_map);
transport_unlock_pack(transport);
return ret;
}
@@ -1116,7 +1128,8 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_refs(transport, ref_map);
+ if (!fetch_refs(transport, ref_map))
+ consume_refs(transport, ref_map);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1165,7 +1178,7 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map)) {
+ if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
free_refs(ref_map);
retcode = 1;
goto cleanup;
--
2.18.0.rc2.346.g013aa6912e-goog
[PATCH v5 1/8] test-pkt-line: add unpack-sideband subcommand
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams
---
t/helper/test-pkt-line.c | 33 +
1 file changed, 33 insertions(+)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 0f19e53c7..30775f986 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,3 +1,4 @@
+#include "cache.h"
#include "pkt-line.h"
static void pack_line(const char *line)
@@ -48,6 +49,36 @@ static void unpack(void)
}
}
+static void unpack_sideband(void)
+{
+ struct packet_reader reader;
+ packet_reader_init(, 0, NULL, 0,
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+
+ while (packet_reader_read() != PACKET_READ_EOF) {
+ int band;
+ int fd;
+
+ switch (reader.status) {
+ case PACKET_READ_EOF:
+ break;
+ case PACKET_READ_NORMAL:
+ band = reader.line[0] & 0xff;
+ if (band < 1 || band > 2)
+ die("unexpected side band %d", band);
+ fd = band;
+
+ write_or_die(fd, reader.line + 1, reader.pktlen - 1);
+ break;
+ case PACKET_READ_FLUSH:
+ return;
+ case PACKET_READ_DELIM:
+ break;
+ }
+ }
+}
+
int cmd_main(int argc, const char **argv)
{
if (argc < 2)
@@ -57,6 +88,8 @@ int cmd_main(int argc, const char **argv)
pack(argc - 2, argv + 2);
else if (!strcmp(argv[1], "unpack"))
unpack();
+ else if (!strcmp(argv[1], "unpack-sideband"))
+ unpack_sideband();
else
die("invalid argument '%s'", argv[1]);
--
2.18.0.rc2.346.g013aa6912e-goog
Re: [PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config
On 06/26, Antonio Ospite wrote: > Hi, > > this is version 2 of the series from > https://public-inbox.org/git/20180622162656.19338-1-...@ao2.it/ > > The .gitmodules file is not meant for arbitrary configuration, it should > be used only for submodules properties. > > Plus, arbitrary git configuration should not be distributed with the > repository, and .gitmodules might be a possible "vector" for that. > > The series tries to alleviate both these issues by moving the > 'config_from_gitmodules' function from config.[ch] to submodule-config.c > and making it private. > > This should discourage future code from using the function with > arbitrary config callbacks which might turn .gitmodules into a mechanism > to load arbitrary configuration stored in the repository. > > Backward compatibility exceptions to the rules above are handled by > ad-hoc helpers. > > Finally (in patch 6) some duplication is removed by using > 'config_from_gitmodules' to load the submodules configuration in > 'repo_read_gitmodules'. > > Changes since v1: > * Remove an extra space before an arrow operator in patch 2 > * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs > * Add a note in the commit message of patch 6 about checking the > worktree before loading .gitmodules > * Drop patch 7, it was meant as a cleanup but resulted in parsing the > .gitmodules file twice Thanks for making these changes, this version looks good to me! -- Brandon Williams
[PATCH v4 3/8] upload-pack: test negotiation with changing repository
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf| 8 +++
t/lib-httpd/one-time-sed.sh| 16 ++
t/t5703-upload-pack-ref-in-want.sh | 92 ++
4 files changed, 117 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..84f8efdd4 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+ install_script one-time-sed.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 724d9ae46..fe68d37bb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -111,9 +111,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1
Options FollowSymlinks
@@ -123,6 +128,9 @@ ScriptAlias /error/ error.sh/
Options ExecCGI
+
+ Options ExecCGI
+
Options ExecCGI
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..a9c4aa5f4
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+if [ -e one-time-sed ]; then
+ "$GIT_EXEC_PATH/git-http-backend" >out
+
+ sed "$(cat one-time-sed)" out_modified
+
+ if diff out out_modified >/dev/null; then
+ cat out
+ else
+ cat out_modified
+ rm one-time-sed
+ fi
+else
+ "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 0ef182970..979ab6d03 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -150,4 +150,96 @@ test_expect_success 'want-ref with ref we already have
commit for' '
check_output
'
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+ (
+ git init "$REPO" &&
+ cd "$REPO" &&
+ >.git/git-daemon-export-ok &&
+ test_commit m1 &&
+ git tag -d m1 &&
+
+ # Local repo with many commits (so that negotiation will take
+ # more than 1 request/response pair)
+ git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo;
"$LOCAL_PRISTINE" &&
+ cd "$LOCAL_PRISTINE" &&
+ git checkout -b side &&
+ for i in $(seq 1 33); do test_commit s$i; done &&
+
+ # Add novel commits to upstream
+ git checkout master &&
+ cd "$REPO" &&
+ test_commit m2 &&
+ test_commit m3 &&
+ git tag -d m2 m3
+ ) &&
+ git -C "$LOCAL_PRISTINE" remote set-url origin
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo; &&
+ git -C "$LOCAL_PRISTINE" config protocol.version 2
+'
+
+inconsistency() {
+ # Simulate that the server initially reports $2 as the ref
+ # corresponding to $1, and after that, $1 as the ref corresponding to
+ # $1. This corresponds to the real-life situation where the server's
+ # repository appears to change during negotiation, for example, when
+ # different servers in a load-balancing arrangement serve (stateless)
+ # RPCs during a single negotiation.
+ printf "s/%s/%s/" \
+ $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+ $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
+
[PATCH v4 7/8] fetch-pack: put shallow info in output parameter
Expand the transport fetch method signature, by adding an output
parameter, to allow transports to return information about the refs they
have fetched. Then communicate shallow status information through this
mechanism instead of by modifying the input list of refs.
This does require clients to sometimes generate the ref map twice: once
from the list of refs provided by the remote (as is currently done) and
potentially once from the new list of refs that the fetch mechanism
provides.
Signed-off-by: Brandon Williams
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 28
fetch-object.c | 2 +-
fetch-pack.c | 15 ---
transport-helper.c | 6 --
transport-internal.h | 9 -
transport.c | 34 --
transport.h | 3 ++-
8 files changed, 77 insertions(+), 24 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae8..8f86d99c5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1155,7 +1155,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
}
if (!is_local && !complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
@@ -1197,7 +1197,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (is_local)
clone_local(path, git_dir);
else if (refs && complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
update_remote_refs(refs, mapped_refs, remote_head_points_at,
branch_top.buf, reflog_msg.buf, transport,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bda00e826..0347cf016 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -946,11 +946,13 @@ static int quickfetch(struct ref *ref_map)
return check_connected(iterate_ref_map, , );
}
-static int fetch_refs(struct transport *transport, struct ref *ref_map)
+static int fetch_refs(struct transport *transport, struct ref *ref_map,
+ struct ref **updated_remote_refs)
{
int ret = quickfetch(ref_map);
if (ret)
- ret = transport_fetch_refs(transport, ref_map);
+ ret = transport_fetch_refs(transport, ref_map,
+ updated_remote_refs);
if (!ret)
/*
* Keep the new pack's ".keep" file around to allow the caller
@@ -1112,7 +1114,7 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- if (!fetch_refs(transport, ref_map))
+ if (!fetch_refs(transport, ref_map, NULL))
consume_refs(transport, ref_map);
if (gsecondary) {
@@ -1128,6 +1130,7 @@ static int do_fetch(struct transport *transport,
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
+ struct ref *updated_remote_refs = NULL;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
@@ -1178,7 +1181,24 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
+
+ if (fetch_refs(transport, ref_map, _remote_refs)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
+ if (updated_remote_refs) {
+ /*
+* Regenerate ref_map using the updated remote refs. This is
+* to account for additional information which may be provided
+* by the transport (e.g. shallow info).
+*/
+ free_refs(ref_map);
+ ref_map = get_ref_map(transport->remote, updated_remote_refs,
rs,
+ tags, );
+ free_refs(updated_remote_refs);
+ }
+ if (consume_refs(transport, ref_map)) {
free_refs(ref_map);
retcode = 1;
goto cleanup;
diff --git a/fetch-object.c b/fetch-object.c
index 853624f81..48fe63dd6 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -19,7 +19,7 @@ static void fetch_refs(const char *remote_name, struct ref
*ref)
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
[PATCH v4 8/8] fetch-pack: implement ref-in-want
Implement ref-in-want on the client side so that when a server supports
the "ref-in-want" feature, a client will send "want-ref" lines for each
reference the client wants to fetch. This feature allows clients to
tolerate inconsistencies that exist when a remote repository's refs
change during the course of negotiation.
This allows a client to request to request a particular ref without
specifying the OID of the ref. This means that instead of hitting an
error when a ref no longer points at the OID it did at the beginning of
negotiation, negotiation can continue and the value of that ref will be
sent at the termination of negotiation, just before a packfile is sent.
More information on the ref-in-want feature can be found in
Documentation/technical/protocol-v2.txt.
Signed-off-by: Brandon Williams
---
fetch-pack.c | 35 +++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 73890b894..3a18f5bcd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf,
static void add_wants(const struct ref *wants, struct strbuf *req_buf)
{
+ int use_ref_in_want = server_supports_feature("fetch", "ref-in-want",
0);
+
for ( ; wants ; wants = wants->next) {
const struct object_id *remote = >old_oid;
- const char *remote_hex;
struct object *o;
/*
@@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct
strbuf *req_buf)
continue;
}
- remote_hex = oid_to_hex(remote);
- packet_buf_write(req_buf, "want %s\n", remote_hex);
+ if (!use_ref_in_want || wants->exact_oid)
+ packet_buf_write(req_buf, "want %s\n",
oid_to_hex(remote));
+ else
+ packet_buf_write(req_buf, "want-ref %s\n", wants->name);
}
}
@@ -1334,6 +1337,29 @@ static void receive_shallow_info(struct fetch_pack_args
*args,
args->deepen = 1;
}
+static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
+{
+ process_section_header(reader, "wanted-refs", 0);
+ while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+ struct object_id oid;
+ const char *end;
+ struct ref *r = NULL;
+
+ if (parse_oid_hex(reader->line, , ) || *end++ != ' ')
+ die("expected wanted-ref, got '%s'", reader->line);
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(>old_oid, );
+ break;
+ }
+ }
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
+ die("error processing wanted refs: %d", reader->status);
+}
+
enum fetch_state {
FETCH_CHECK_LOCAL = 0,
FETCH_SEND_REQUEST,
@@ -1408,6 +1434,9 @@ static struct ref *do_fetch_pack_v2(struct
fetch_pack_args *args,
if (process_section_header(, "shallow-info", 1))
receive_shallow_info(args, );
+ if (process_section_header(, "wanted-refs", 1))
+ receive_wanted_refs(, ref);
+
/* get the pack */
process_section_header(, "packfile", 0);
if (get_pack(args, fd, pack_lockfile))
diff --git a/remote.c b/remote.c
index abe80c139..2c2376fff 100644
--- a/remote.c
+++ b/remote.c
@@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
if (refspec->exact_sha1) {
ref_map = alloc_ref(name);
get_oid_hex(name, _map->old_oid);
+ ref_map->exact_oid = 1;
} else {
ref_map = get_remote_ref(remote_refs, name);
}
diff --git a/remote.h b/remote.h
index 45ecc6cef..976292152 100644
--- a/remote.h
+++ b/remote.h
@@ -73,6 +73,7 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
+ exact_oid:1,
deletion:1;
enum {
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 979ab6d03..b94a51380 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -204,7 +204,7 @@ test_expect_success 'server is initially ahead - no ref in
want' '
grep "ERR upload
[PATCH v4 6/8] fetch: refactor to make function args narrower
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 52 -
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 2fabfed0e..bda00e826 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -254,9 +254,9 @@ static int will_fetch(struct ref **head, const unsigned
char *sha1)
return 0;
}
-static void find_non_local_tags(struct transport *transport,
- struct ref **head,
- struct ref ***tail)
+static void find_non_local_tags(const struct ref *refs,
+ struct ref **head,
+ struct ref ***tail)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct string_list remote_refs = STRING_LIST_INIT_NODUP;
@@ -264,7 +264,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
for_each_ref(add_existing, _refs);
- for (ref = transport_get_remote_refs(transport, NULL); ref; ref =
ref->next) {
+ for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -338,7 +338,8 @@ static void find_non_local_tags(struct transport *transport,
string_list_clear(_refs, 0);
}
-static struct ref *get_ref_map(struct transport *transport,
+static struct ref *get_ref_map(struct remote *remote,
+ const struct ref *remote_refs,
struct refspec *rs,
int tags, int *autotags)
{
@@ -346,27 +347,11 @@ static struct ref *get_ref_map(struct transport
*transport,
struct ref *rm;
struct ref *ref_map = NULL;
struct ref **tail = _map;
- struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
struct string_list existing_refs = STRING_LIST_INIT_DUP;
- const struct ref *remote_refs;
-
- if (rs->nr)
- refspec_ref_prefixes(rs, _prefixes);
- else if (transport->remote && transport->remote->fetch.nr)
- refspec_ref_prefixes(>remote->fetch, _prefixes);
-
- if (ref_prefixes.argc &&
- (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
- argv_array_push(_prefixes, "refs/tags/");
- }
-
- remote_refs = transport_get_remote_refs(transport, _prefixes);
-
- argv_array_clear(_prefixes);
if (rs->nr) {
struct refspec *fetch_refspec;
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,
if (refmap.nr)
fetch_refspec =
else
- fetch_refspec = >remote->fetch;
+ fetch_refspec = >fetch;
for (i = 0; i < fetch_refspec->nr; i++)
get_fetch_map(ref_map, _refspec->items[i],
_tail, 1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,
die("--refmap option is only meaningful with command-line
refspec(s).");
} else {
/* Use the defaults */
- struct remote *remote = transport->remote;
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, , 0);
else if (tags == TAGS_DEFAULT && *autotags)
- find_non_local_tags(transport, _map, );
+ find_non_local_tags(remote_refs, _map, );
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1143,6 +1127,8 @@ static int do_fetch(struct transport *transport,
struct ref *ref_map;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
+ const struct ref *remote_refs;
+ struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1158,7 +1144,21 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
- ref_map = get_ref_map(transport, rs, tags, );
+ if (rs->nr)
+ r
[PATCH v4 4/8] fetch: refactor the population of peer ref OIDs
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..545635448 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,6 +351,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
const struct ref *remote_refs;
if (rs->nr)
@@ -458,7 +459,23 @@ static struct ref *get_ref_map(struct transport *transport,
tail = >next;
}
- return ref_remove_duplicates(ref_map);
+ ref_map = ref_remove_duplicates(ref_map);
+
+ for_each_ref(add_existing, _refs);
+ for (rm = ref_map; rm; rm = rm->next) {
+ if (rm->peer_ref) {
+ struct string_list_item *peer_item =
+ string_list_lookup(_refs,
+ rm->peer_ref->name);
+ if (peer_item) {
+ struct object_id *old_oid = peer_item->util;
+ oidcpy(>peer_ref->old_oid, old_oid);
+ }
+ }
+ }
+ string_list_clear(_refs, 1);
+
+ return ref_map;
}
#define STORE_REF_ERROR_OTHER 1
@@ -1110,14 +1127,10 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
- struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
- struct ref *rm;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
- for_each_ref(add_existing, _refs);
-
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
tags = TAGS_SET;
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map);
- for (rm = ref_map; rm; rm = rm->next) {
- if (rm->peer_ref) {
- struct string_list_item *peer_item =
- string_list_lookup(_refs,
- rm->peer_ref->name);
- if (peer_item) {
- struct object_id *old_oid = peer_item->util;
- oidcpy(>peer_ref->old_oid, old_oid);
- }
- }
- }
-
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,
}
cleanup:
- string_list_clear(_refs, 1);
return retcode;
}
--
2.18.0.rc2.346.g013aa6912e-goog
[PATCH v4 5/8] fetch: refactor fetch_refs into two functions
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them. This is in preparation
for allowing additional processing of the fetched refs before updating
the local ref store.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 545635448..2fabfed0e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -968,9 +968,21 @@ static int fetch_refs(struct transport *transport, struct
ref *ref_map)
if (ret)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
- ret |= store_updated_refs(transport->url,
- transport->remote->name,
- ref_map);
+ /*
+* Keep the new pack's ".keep" file around to allow the caller
+* time to update refs to reference the new objects.
+*/
+ return 0;
+ transport_unlock_pack(transport);
+ return ret;
+}
+
+/* Update local refs based on the ref values fetched from a remote */
+static int consume_refs(struct transport *transport, struct ref *ref_map)
+{
+ int ret = store_updated_refs(transport->url,
+transport->remote->name,
+ref_map);
transport_unlock_pack(transport);
return ret;
}
@@ -1116,7 +1128,8 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_refs(transport, ref_map);
+ if (!fetch_refs(transport, ref_map))
+ consume_refs(transport, ref_map);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1165,7 +1178,7 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map)) {
+ if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
free_refs(ref_map);
retcode = 1;
goto cleanup;
--
2.18.0.rc2.346.g013aa6912e-goog
[PATCH v4 2/8] upload-pack: implement ref-in-want
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement and
there exists replication delay.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref " parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams
---
Documentation/config.txt| 7 ++
Documentation/technical/protocol-v2.txt | 28 -
t/t5703-upload-pack-ref-in-want.sh | 153
upload-pack.c | 66 ++
4 files changed, 253 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a..fb1dd7428 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it
is seen in the
repository-level config (this is a safety measure against fetching from
untrusted repositories).
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command. This feature
+ is intended for the benefit of load-balanced servers which may
+ not have the same view of what OIDs their refs point to due to
+ replication delay.
+
url..insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with . In cases where some site serves a
diff --git a/Documentation/technical/protocol-v2.txt
b/Documentation/technical/protocol-v2.txt
index 49bda76d2..d44ac10f1 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -299,12 +299,21 @@ included in the client's request:
for use with partial clone and partial fetch operations. See
`rev-list` for possible "filter-spec" values.
+If the 'ref-in-want' feature is advertised, the following argument can
+be included in the client's request as well as the potential addition of
+the 'wanted-refs' section in the server's response as explained below.
+
+want-ref
+ Indicates to the server that the client wants to retrieve a
+ particular ref, where is the full name of a ref on the
+ server.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
output = *section
-section = (acknowledgments | shallow-info | packfile)
+section = (acknowledgments | shallow-info | wanted-refs | packfile)
(flush-pkt | delim-pkt)
acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -319,6 +328,10 @@ header.
shallow = "shallow" SP obj-id
unshallow = "unshallow" SP obj-id
+wanted-refs = PKT-LINE("wanted-refs" LF)
+ *PKT-LINE(wanted-ref LF)
+wanted-ref = obj-id SP refname
+
packfile = PKT-LINE("packfile" LF)
*PKT-LINE(%x01-03 *%x00-ff)
@@ -379,6 +392,19 @@ header.
* This section is only included if a packfile section is also
included in the response.
+wanted-refs section
+ * This section is only included if the client has requested a
+ ref using a 'want-ref' line and if a packfile section is also
+ included in the response.
+
+ * Always begins with the section header "wanted-refs"
+
+ * The server will send a ref listing (" ") for
+ each reference requested using 'want-ref' lines.
+
+ * The server SHOULD NOT send any refs which were not requested
+ using 'want-ref' lines.
+
packfile section
* This section is only included if the client has sent 'want'
lines in its request and either requested that no more
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
new file mode 100755
index 0..0ef182970
--- /dev/null
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -0,0 +1,153 @@
+#!/bin/sh
+
+test_description='upload-pack ref-in-want'
+
+. ./test-lib.sh
+
+get_actual_refs() {
+ sed -n '/wanted-refs/,/0001/p' actual_refs
+}
+
+get_actual_commits() {
+ sed -n '/packfile/,//p' o.pack &&
+ git index-pack o.pack &&
+ git verify-pack -v o.idx | grep commit | cut -c-40 | sort
>actual_commits
+}
[PATCH v4 1/8] test-pkt-line: add unpack-sideband subcommand
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams
---
t/helper/test-pkt-line.c | 33 +
1 file changed, 33 insertions(+)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 0f19e53c7..30775f986 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,3 +1,4 @@
+#include "cache.h"
#include "pkt-line.h"
static void pack_line(const char *line)
@@ -48,6 +49,36 @@ static void unpack(void)
}
}
+static void unpack_sideband(void)
+{
+ struct packet_reader reader;
+ packet_reader_init(, 0, NULL, 0,
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+
+ while (packet_reader_read() != PACKET_READ_EOF) {
+ int band;
+ int fd;
+
+ switch (reader.status) {
+ case PACKET_READ_EOF:
+ break;
+ case PACKET_READ_NORMAL:
+ band = reader.line[0] & 0xff;
+ if (band < 1 || band > 2)
+ die("unexpected side band %d", band);
+ fd = band;
+
+ write_or_die(fd, reader.line + 1, reader.pktlen - 1);
+ break;
+ case PACKET_READ_FLUSH:
+ return;
+ case PACKET_READ_DELIM:
+ break;
+ }
+ }
+}
+
int cmd_main(int argc, const char **argv)
{
if (argc < 2)
@@ -57,6 +88,8 @@ int cmd_main(int argc, const char **argv)
pack(argc - 2, argv + 2);
else if (!strcmp(argv[1], "unpack"))
unpack();
+ else if (!strcmp(argv[1], "unpack-sideband"))
+ unpack_sideband();
else
die("invalid argument '%s'", argv[1]);
--
2.18.0.rc2.346.g013aa6912e-goog
[PATCH v4 0/8] ref-in-want
Changes in v4 are fairly minor. There are a few documentation changes, commit message updates, as well as a few small style tweaks based on reviewer comments. Brandon Williams (8): test-pkt-line: add unpack-sideband subcommand upload-pack: implement ref-in-want upload-pack: test negotiation with changing repository fetch: refactor the population of peer ref OIDs fetch: refactor fetch_refs into two functions fetch: refactor to make function args narrower fetch-pack: put shallow info in output parameter fetch-pack: implement ref-in-want Documentation/config.txt| 7 + Documentation/technical/protocol-v2.txt | 28 ++- builtin/clone.c | 4 +- builtin/fetch.c | 135 - fetch-object.c | 2 +- fetch-pack.c| 50 - remote.c| 1 + remote.h| 1 + t/helper/test-pkt-line.c| 33 t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 8 + t/lib-httpd/one-time-sed.sh | 16 ++ t/t5703-upload-pack-ref-in-want.sh | 245 transport-helper.c | 6 +- transport-internal.h| 9 +- transport.c | 34 +++- transport.h | 3 +- upload-pack.c | 66 +++ 18 files changed, 574 insertions(+), 75 deletions(-) create mode 100644 t/lib-httpd/one-time-sed.sh create mode 100755 t/t5703-upload-pack-ref-in-want.sh -- 2.18.0.rc2.346.g013aa6912e-goog
Re: [PATCH v3 0/7] Refactor fetch negotiation into its own API
On 06/14, Jonathan Tan wrote:
> Thanks, Brandon and Junio, for your comments.
>
> Updates since v2:
> - nothing new in patches 1 and 2
> - patch 3: now clear priority queue unconditionally once we are done
>with it (as requested by Brandon in a comment for a later patch)
> - patch 4: updated commit message to not mention everything_local() (as
>pointed out by Brandon), updated test to not rely on the fact that
>fetch-pack uses prefix matching (thanks, Junio, for the observation)
> - patch 5: used a more descriptive name ("struct negotiation_state")
>for the struct, instead of "struct data"
> - squashed patch 8 into patch 7; this means that the comments are not
>moved verbatim, but for the reviewer, verbatim-ness of comments is
>probably not that important anyway
Thanks this series looks good now.
Reviewed-by: Brandon Williams
--
Brandon Williams
Re: [PATCH 0/2] Object store refactoring: make bitmap_git not global
On 06/07, Jonathan Tan wrote: > This is a continuation of the object store refactoring effort. > > We cannot truly free an object store without ensuring that any generated > bitmaps are first freed, so here are patches to drastically reduce the > lifetime of any bitmaps generated. As a bonus, the API is also improved, > and global state reduced. I've reviewed this series and haven't found any issues. Reviewed-by: Brandon Williams -- Brandon Williams
Re: [PATCH v3 2/8] upload-pack: implement ref-in-want
On 06/25, Jonathan Tan wrote:
> > +static int parse_want_ref(const char *line, struct string_list
> > *wanted_refs)
> > +{
> > + const char *arg;
> > + if (skip_prefix(line, "want-ref ", )) {
> > + struct object_id oid;
> > + struct string_list_item *item;
> > + struct object *o;
> > +
> > + if (read_ref(arg, ))
> > + die("unknown ref %s", arg);
>
> One more thing - if you're planning to "die" here, also write out an
> error to the user, just like in parse_want().
Oh good idea, I'll add an ERR pkt here
--
Brandon Williams
Re: [PATCH v3 7/8] fetch-pack: put shallow info in output parameter
On 06/25, Jonathan Tan wrote:
> > static void update_shallow(struct fetch_pack_args *args,
> > - struct ref **sought, int nr_sought,
> > + struct ref *refs,
>
> update_shallow() now takes in a linked list of refs instead of an array.
> I see that the translation of this function is straightforward -
> occasionally, we need to iterate through the linked list and count up
> from 0 at the same time, but that is not a problem.
>
> >struct shallow_info *si)
> > {
> > struct oid_array ref = OID_ARRAY_INIT;
> > int *status;
> > - int i;
> > + int i = 0;
>
> Remove the " = 0" - I've verified that it does not need to be there, and
> it might inhibit useful "unintialized variable" warnings if others were
> to change the code later.
>
> Optional: I would also remove this declaration and declare "int i;" in
> each of the blocks that need it.
>
> > static int fetch_refs_via_pack(struct transport *transport,
> > - int nr_heads, struct ref **to_fetch)
> > + int nr_heads, struct ref **to_fetch,
> > + struct ref **fetched_refs)
> > {
> > int ret = 0;
> > struct git_transport_data *data = transport->data;
> > @@ -354,8 +356,12 @@ static int fetch_refs_via_pack(struct transport
> > *transport,
> > if (report_unmatched_refs(to_fetch, nr_heads))
> > ret = -1;
> >
> > + if (fetched_refs)
> > + *fetched_refs = refs;
> > + else
> > + free_refs(refs);
> > +
> > free_refs(refs_tmp);
> > - free_refs(refs);
> > free(dest);
> > return ret;
> > }
>
> Instead of just freeing the linked list, we return it if requested by
> the client. This makes sense.
>
> > -int transport_fetch_refs(struct transport *transport, struct ref *refs)
> > +int transport_fetch_refs(struct transport *transport, struct ref *refs,
> > +struct ref **fetched_refs)
> > {
> > int rc;
> > int nr_heads = 0, nr_alloc = 0, nr_refs = 0;
> > struct ref **heads = NULL;
> > + struct ref *nop_head = NULL, **nop_tail = _head;
> > struct ref *rm;
> >
> > for (rm = refs; rm; rm = rm->next) {
> > nr_refs++;
> > if (rm->peer_ref &&
> > !is_null_oid(>old_oid) &&
> > - !oidcmp(>peer_ref->old_oid, >old_oid))
> > + !oidcmp(>peer_ref->old_oid, >old_oid)) {
> > + /*
> > +* These need to be reported as fetched, but we don't
> > +* actually need to fetch them.
> > +*/
> > + if (fetched_refs) {
> > + struct ref *nop_ref = copy_ref(rm);
> > + *nop_tail = nop_ref;
> > + nop_tail = _ref->next;
> > + }
> > continue;
> > + }
> > ALLOC_GROW(heads, nr_heads + 1, nr_alloc);
> > heads[nr_heads++] = rm;
> > }
> > @@ -1245,7 +1263,11 @@ int transport_fetch_refs(struct transport
> > *transport, struct ref *refs)
> > heads[nr_heads++] = rm;
> > }
> >
> > - rc = transport->vtable->fetch(transport, nr_heads, heads);
> > + rc = transport->vtable->fetch(transport, nr_heads, heads, fetched_refs);
> > + if (fetched_refs && nop_head) {
> > + *nop_tail = *fetched_refs;
> > + *fetched_refs = nop_head;
> > + }
> >
> > free(heads);
> > return rc;
>
> And sometimes, even if we are merely simulating the fetching of refs, we
> still need to report those refs in fetched_refs. This is correct.
>
> I also see that t5703 now passes.
>
> Besides enabling the writing of subsequent patches, I see that this also
> makes the API clearer in that the input refs to transport_fetch_refs()
> are not overloaded to output shallow information. Other than the " = 0"
> change above, this patch looks good to me.
Perfect, I'll just drop the " = 0" part (making the diff slightly
smaller)
--
Brandon Williams
Re: [PATCH v3 8/8] fetch-pack: implement ref-in-want
On 06/22, Jonathan Nieder wrote:
> Hi,
>
> Brandon Williams wrote:
>
> > Implement ref-in-want on the client side so that when a server supports
> > the "ref-in-want" feature, a client will send "want-ref" lines for each
> > reference the client wants to fetch.
> >
> > Signed-off-by: Brandon Williams
> > ---
> > fetch-pack.c | 35 +++---
> > remote.c | 1 +
> > remote.h | 1 +
> > t/t5703-upload-pack-ref-in-want.sh | 4 ++--
> > 4 files changed, 36 insertions(+), 5 deletions(-)
>
> This commit message doesn't tell me what ref-in-want is or is for. Could
> it include
>
> A. a pointer to Documentation/technical/protocol-v2.txt, or
> B. an example illustrating the effect e.g. using GIT_TRACE_PACKET
>
> or both?
Yeah I can imporve the message here.
> > +
> > + for (r = refs; r; r = r->next) {
> > + if (!strcmp(end, r->name)) {
> > + oidcpy(>old_oid, );
> > + break;
> > + }
>
> Stefan mentioned that the spec says
>
> * The server MUST NOT send any refs which were not requested
> using 'want-ref' lines.
>
> Can client enforce that? If not, can the spec say SHOULD NOT for the
> server and add a MUST describing appropriate client behavior?
Yeah I can update the docs in an earlier patch.
>
> > + }
> > + }
> > +
> > + if (reader->status != PACKET_READ_DELIM)
>
> The spec says
>
> * This section is only included if the client has requested a
> ref using a 'want-ref' line and if a packfile section is also
> included in the response.
>
> What should happen if the client already has all the relevant objects
> (or in other words if there is no packfile to send in the packfile
> section)? Is the idea that the client should already have known that
> based on the ref advertisement? What if ref values change to put us
> in that state between the ls-refs and fetch steps?
I believe the current functionality is that if all wants are already
satisfied by all haves then an empty packfile is sent, so that would
fall under that case.
--
Brandon Williams
Re: [PATCH 0/7] Restrict the usage of config_from_gitmodules()
On 06/22, Antonio Ospite wrote: > On Fri, 22 Jun 2018 10:13:10 -0700 > Brandon Williams wrote: > > [...] > > Thanks for working on this. I think its a good approach and the end > > result makes it much harder for arbitrary config to sneak back in to the > > .gitmodules file. And after this series it looks like you should be in > > a good place to read the .gitmodules file from other places (not just in > > the worktree). > > > > :) > > > As you've mentioned here I also agree we could do without the last patch > > but I'll leave that up to you. Other than a couple typos I found I > > think this series looks good! Thanks again for revisiting this. > > > > Thanks for the review. > > I understand your compromise solution for patch 7, but I'd say let's > keep it simple and just drop patch 7 for now. Yeah that's probably the best approach for the time being. > > I am going to wait a couple of days and then send a v2. Perfect, thanks again! -- Brandon Williams
