* Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.
From: Johannes Schindelin @ 2018-12-18 12:35 UTC (permalink / raw)
To: Erin Dahlgren; +Cc: git, Junio C Hamano
In-Reply-To: <1544922308-740-1-git-send-email-eedahlgren@gmail.com>
Hi Erin,
On Sat, 15 Dec 2018, Erin Dahlgren wrote:
> diff --git a/setup.c b/setup.c
> index 1be5037..e1a9e17 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
> return NULL;
> }
>
> -static const char *setup_nongit(const char *cwd, int *nongit_ok)
> -{
> - if (!nongit_ok)
> - die(_("not a git repository (or any of the parent directories): %s"), DEFAULT_GIT_DIR_ENVIRONMENT);
> - if (chdir(cwd))
> - die_errno(_("cannot come back to cwd"));
> - *nongit_ok = 1;
> - return NULL;
> -}
> -
> static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)
> {
> struct stat buf;
> @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)
> prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);
> break;
> case GIT_DIR_HIT_CEILING:
> - prefix = setup_nongit(cwd.buf, nongit_ok);
> - break;
> + if (!nongit_ok)
> + die(_("not a git repository (or any of the parent directories): %s"),
> + DEFAULT_GIT_DIR_ENVIRONMENT);
I am terribly sorry to bother you about formatting issues (in my mind, it
is quite an annoying thing that we still have no fully automatic way to
format Git's source code according to Git's preferred coding style, but
there you go...): this `DEFAULT_GIT_DIR_ENVIRONMENT` should be aligned
with the first parameter of `die()`, i.e.
+ if (!nongit_ok)
+ die(_("not a git repository (or any of the parent directories): %s"),
+ DEFAULT_GIT_DIR_ENVIRONMENT);
> + *nongit_ok = 1;
> + strbuf_release(&dir);
> + return NULL;
> case GIT_DIR_HIT_MOUNT_POINT:
> - if (nongit_ok) {
> - *nongit_ok = 1;
> - strbuf_release(&cwd);
> - strbuf_release(&dir);
> - return NULL;
> - }
> - die(_("not a git repository (or any parent up to mount point %s)\n"
> - "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> - dir.buf);
> + if (!nongit_ok)
> + die(_("not a git repository (or any parent up to mount point %s)\n"
> + "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> + dir.buf);
Likewise, `dir.buf` should be aligned with the `_` two lines above.
Otherwise I think this patch is good to go!
Thank you,
Johannes
> + *nongit_ok = 1;
> + strbuf_release(&dir);
> + return NULL;
> default:
> BUG("unhandled setup_git_directory_1() result");
> }
> --
> 2.7.4
>
>
^ permalink raw reply
* Re: [PATCH] stripspace: allow -s/-c outside git repository
From: Johannes Schindelin @ 2018-12-18 12:00 UTC (permalink / raw)
To: Martin Ågren; +Cc: Jonathan Nieder, Git Mailing List, Han-Wen Nienhuys
In-Reply-To: <CAN0heSoekZ_vjR6DRjSAfw74hn_NU3-5TFf6-Hn9z2D_zE-mNg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]
Hi Martin,
On Tue, 18 Dec 2018, Martin Ågren wrote:
> On Mon, 17 Dec 2018 at 22:56, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > That makes experimenting with the stripspace command unnecessarily
> > fussy. Fix it by discovering the git directory gently, as intended
> > all along.
>
> > if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> > - setup_git_directory_gently(NULL);
> > + setup_git_directory_gently(&nongit);
> > git_config(git_default_config, NULL);
> > }
>
> Makes me wonder if `setup_git_directory_gently()` should BUG if it
> receives NULL. That would require some reshuffling in setup.c, since
> `setup_git_directory()` calls out to it with NULL... Just thinking out
> loud. In any case, and more importantly, this is the last user providing
> NULL except for the one in `setup_git_directory()`.
We could rename `setup_git_directory_gently()` to
`setup_git_directory_gently_2()`, make that `static` and then call it from
`setup_git_directory_gently()` and `setup_git_directory()`.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] stripspace: allow -s/-c outside git repository
From: Johannes Schindelin @ 2018-12-18 11:58 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <20181217165957.GA60293@google.com>
Hi Jonathan,
On Mon, 17 Dec 2018, Jonathan Nieder wrote:
> v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)
> improved stripspace --strip-comments / --comentlines by teaching them
> to read repository config, but it went a little too far: when running
> stripspace outside any repository, the result is
>
> $ git stripspace --strip-comments <test-input
> fatal: not a git repository (or any parent up to mount point /tmp)
>
> That makes experimenting with the stripspace command unnecessarily
> fussy. Fix it by discovering the git directory gently, as intended
> all along.
>
> Reported-by: Han-Wen Nienhuys <hanwen@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
ACK!
> builtin/stripspace.c | 3 ++-
> t/t0030-stripspace.sh | 12 +++++++++---
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index bdf0328869..be33eb83c1 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -30,6 +30,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
> {
> struct strbuf buf = STRBUF_INIT;
> enum stripspace_mode mode = STRIP_DEFAULT;
> + int nongit;
>
> const struct option options[] = {
> OPT_CMDMODE('s', "strip-comments", &mode,
> @@ -46,7 +47,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
> usage_with_options(stripspace_usage, options);
>
> if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> - setup_git_directory_gently(NULL);
> + setup_git_directory_gently(&nongit);
What a fool I was to believe that _gently() was always gentle...
> git_config(git_default_config, NULL);
> }
>
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 5ce47e8af5..0c24a0f9a3 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -430,9 +430,15 @@ test_expect_success '-c with changed comment char' '
> test_expect_success '-c with comment char defined in .git/config' '
> test_config core.commentchar = &&
> printf "= foo\n" >expect &&
> - printf "foo" | (
> - mkdir sub && cd sub && git stripspace -c
> - ) >actual &&
> + rm -fr sub &&
> + mkdir sub &&
> + printf "foo" | git -C sub stripspace -c >actual &&
> + test_cmp expect actual
> +'
Sneaky. But I like it.
Thanks,
Dscho
> +
> +test_expect_success '-c outside git repository' '
> + printf "# foo\n" >expect &&
> + printf "foo" | nongit git stripspace -c >actual &&
> test_cmp expect actual
> '
>
> --
> 2.20.0.405.gbc1bbc6f85
>
>
^ permalink raw reply
* [PATCH 2/2] pack-redundant: remove unused functions
From: Jiang Xin @ 2018-12-18 9:58 UTC (permalink / raw)
To: Git List; +Cc: Sun Chao, Lukas Sandström, Jiang Xin
In-Reply-To: <20181218095829.20092-1-worldhello.net@gmail.com>
From: Sun Chao <sunchao9@huawei.com>
Remove unused functions to find `min` packs, such as `get_permutations`,
`pll_free`, etc.
Signed-off-by: Sun Chao <sunchao9@huawei.com>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
builtin/pack-redundant.c | 81 ----------------------------------------
1 file changed, 81 deletions(-)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 19dcf74750..d0ff2377f3 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -63,15 +63,6 @@ static inline struct llist_item *llist_item_get(void)
return new_item;
}
-static void llist_free(struct llist *list)
-{
- while ((list->back = list->front)) {
- list->front = list->front->next;
- llist_item_put(list->back);
- }
- free(list);
-}
-
static inline void llist_init(struct llist **list)
{
*list = xmalloc(sizeof(struct llist));
@@ -285,78 +276,6 @@ static void cmp_two_packs(struct pack_list *p1, struct pack_list *p2)
}
}
-static void pll_free(struct pll *l)
-{
- struct pll *old;
- struct pack_list *opl;
-
- while (l) {
- old = l;
- while (l->pl) {
- opl = l->pl;
- l->pl = opl->next;
- free(opl);
- }
- l = l->next;
- free(old);
- }
-}
-
-/* all the permutations have to be free()d at the same time,
- * since they refer to each other
- */
-static struct pll * get_permutations(struct pack_list *list, int n)
-{
- struct pll *subset, *ret = NULL, *new_pll = NULL;
-
- if (list == NULL || pack_list_size(list) < n || n == 0)
- return NULL;
-
- if (n == 1) {
- while (list) {
- new_pll = xmalloc(sizeof(*new_pll));
- new_pll->pl = NULL;
- pack_list_insert(&new_pll->pl, list);
- new_pll->next = ret;
- ret = new_pll;
- list = list->next;
- }
- return ret;
- }
-
- while (list->next) {
- subset = get_permutations(list->next, n - 1);
- while (subset) {
- new_pll = xmalloc(sizeof(*new_pll));
- new_pll->pl = subset->pl;
- pack_list_insert(&new_pll->pl, list);
- new_pll->next = ret;
- ret = new_pll;
- subset = subset->next;
- }
- list = list->next;
- }
- return ret;
-}
-
-static int is_superset(struct pack_list *pl, struct llist *list)
-{
- struct llist *diff;
-
- diff = llist_copy(list);
-
- while (pl) {
- llist_sorted_difference_inplace(diff, pl->all_objects);
- if (diff->size == 0) { /* we're done */
- llist_free(diff);
- return 1;
- }
- pl = pl->next;
- }
- llist_free(diff);
- return 0;
-}
-
static size_t sizeof_union(struct packed_git *p1, struct packed_git *p2)
{
size_t ret = 0;
--
2.20.0.2.g660e9286fc
^ permalink raw reply related
* [PATCH 1/2] pack-redundant: new algorithm to find min packs
From: Jiang Xin @ 2018-12-18 9:58 UTC (permalink / raw)
To: Git List; +Cc: Sun Chao, Lukas Sandström, Jiang Xin
From: Sun Chao <sunchao9@huawei.com>
When calling `git pack-redundant --all`, if there are too many local
packs and too many redundant objects within them, the too deep iteration
of `get_permutations` will exhaust all the resources, and the process of
`git pack-redundant` will be killed.
The following script could create a repository with too many redundant
packs, and running `git pack-redundant --all` in the `test.git` repo
will die soon.
#!/bin/sh
repo="$(pwd)/test.git"
work="$(pwd)/test"
i=1
max=199
if test -d "$repo" || test -d "$work"; then
echo >&2 "ERROR: '$repo' or '$work' already exist"
exit 1
fi
git init -q --bare "$repo"
git --git-dir="$repo" config gc.auto 0
git --git-dir="$repo" config transfer.unpackLimit 0
git clone -q "$repo" "$work" 2>/dev/null
while :; do
cd "$work"
echo "loop $i: $(date +%s)" >$i
git add $i
git commit -q -sm "loop $i"
git push -q origin HEAD:master
printf "\rCreate pack %4d/%d\t" $i $max
if test $i -ge $max; then break; fi
cd "$repo"
git repack -q
if test $(($i % 2)) -eq 0; then
git repack -aq
pack=$(ls -t $repo/objects/pack/*.pack | head -1)
touch "${pack%.pack}.keep"
fi
i=$((i+1))
done
printf "\ndone\n"
To get the `min` unique pack list, we can replace the iteration in
`minimize` function with a new algorithm, and this could solve this
issue:
1. Get the unique and non_uniqe packs, add the unique packs to the
`min` list.
2. Remove the objects of unique packs from non_unique packs, then each
object left in the non_unique packs will have at least two copies.
3. Sort the non_unique packs by the objects' size, more objects first,
and add the first non_unique pack to `min` list.
4. Drop the duplicated objects from other packs in the ordered
non_unique pack list, and repeat step 3.
Original PR and discussions: https://github.com/jiangxin/git/pull/25
Signed-off-by: Sun Chao <sunchao9@huawei.com>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
builtin/pack-redundant.c | 116 ++++++++++++++++++++++++---------------
1 file changed, 73 insertions(+), 43 deletions(-)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index cf9a9aabd4..19dcf74750 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -421,14 +421,52 @@ static inline off_t pack_set_bytecount(struct pack_list *pl)
return ret;
}
-static void minimize(struct pack_list **min)
+static int cmp_pack_list_reverse(const void *a, const void *b)
{
- struct pack_list *pl, *unique = NULL,
- *non_unique = NULL, *min_perm = NULL;
- struct pll *perm, *perm_all, *perm_ok = NULL, *new_perm;
- struct llist *missing;
- off_t min_perm_size = 0, perm_size;
- int n;
+ struct pack_list *pl_a = *((struct pack_list **)a);
+ struct pack_list *pl_b = *((struct pack_list **)b);
+ size_t sz_a = pl_a->all_objects->size;
+ size_t sz_b = pl_b->all_objects->size;
+
+ if (sz_a == sz_b)
+ return 0;
+ else if (sz_a < sz_b)
+ return 1;
+ else
+ return -1;
+}
+
+/* Sort pack_list, greater size of all_objects first */
+static void sort_pack_list(struct pack_list **pl)
+{
+ struct pack_list **ary, *p;
+ int i;
+ size_t n = pack_list_size(*pl);
+
+ if (n < 2)
+ return;
+
+ /* prepare an array of packed_list for easier sorting */
+ ary = xcalloc(n, sizeof(struct pack_list *));
+ for (n = 0, p = *pl; p; p = p->next)
+ ary[n++] = p;
+
+ QSORT(ary, n, cmp_pack_list_reverse);
+
+ /* link them back again */
+ for (i = 0; i < n - 1; i++)
+ ary[i]->next = ary[i + 1];
+ ary[n - 1]->next = NULL;
+ *pl = ary[0];
+
+ free(ary);
+}
+
+
+static void minimize(struct pack_list **min, struct llist *ignore)
+{
+ struct pack_list *pl, *unique = NULL, *non_unique = NULL;
+ struct llist *missing, *unique_pack_objects;
pl = local_packs;
while (pl) {
@@ -446,49 +484,41 @@ static void minimize(struct pack_list **min)
pl = pl->next;
}
+ *min = unique;
+
/* return if there are no objects missing from the unique set */
if (missing->size == 0) {
- *min = unique;
free(missing);
return;
}
- /* find the permutations which contain all missing objects */
- for (n = 1; n <= pack_list_size(non_unique) && !perm_ok; n++) {
- perm_all = perm = get_permutations(non_unique, n);
- while (perm) {
- if (is_superset(perm->pl, missing)) {
- new_perm = xmalloc(sizeof(struct pll));
- memcpy(new_perm, perm, sizeof(struct pll));
- new_perm->next = perm_ok;
- perm_ok = new_perm;
- }
- perm = perm->next;
- }
- if (perm_ok)
- break;
- pll_free(perm_all);
- }
- if (perm_ok == NULL)
- die("Internal error: No complete sets found!");
-
- /* find the permutation with the smallest size */
- perm = perm_ok;
- while (perm) {
- perm_size = pack_set_bytecount(perm->pl);
- if (!min_perm_size || min_perm_size > perm_size) {
- min_perm_size = perm_size;
- min_perm = perm->pl;
- }
- perm = perm->next;
- }
- *min = min_perm;
- /* add the unique packs to the list */
- pl = unique;
+ unique_pack_objects = llist_copy(all_objects);
+ llist_sorted_difference_inplace(unique_pack_objects, missing);
+
+ /* remove all the ignored objects and unique pack objects from the non_unique packs */
+ pl = non_unique;
while (pl) {
- pack_list_insert(min, pl);
+ llist_sorted_difference_inplace(pl->all_objects, ignore);
+ llist_sorted_difference_inplace(pl->all_objects, unique_pack_objects);
pl = pl->next;
}
+
+ while ((pl = non_unique)) {
+ /* sort the non_unique packs, greater size of all_objects first */
+ sort_pack_list(&non_unique);
+ if (non_unique->all_objects->size == 0)
+ break;
+
+ pack_list_insert(min, non_unique);
+
+ while ((pl = pl->next)) {
+ if (pl->all_objects->size == 0)
+ break;
+ llist_sorted_difference_inplace(pl->all_objects, non_unique->all_objects);
+ }
+
+ non_unique = non_unique->next;
+ }
}
static void load_all_objects(void)
@@ -603,7 +633,7 @@ static void load_all(void)
int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
{
int i;
- struct pack_list *min, *red, *pl;
+ struct pack_list *min = NULL, *red, *pl;
struct llist *ignore;
struct object_id *oid;
char buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \n + \0 */
@@ -667,7 +697,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
pl = pl->next;
}
- minimize(&min);
+ minimize(&min, ignore);
if (verbose) {
fprintf(stderr, "There are %lu packs available in alt-odbs.\n",
--
2.20.0.2.g660e9286fc
^ permalink raw reply related
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
From: Ævar Arnfjörð Bjarmason @ 2018-12-18 9:28 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, git, Junio C Hamano, Brandon Williams, Jonathan Tan
In-Reply-To: <20181218000227.GB13835@google.com>
On Tue, Dec 18 2018, Jonathan Nieder wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Dec 17 2018, Jonathan Nieder wrote:
>
>>> This would make per-branch ACLs (as implemented both by Gerrit and
>>> gitolite) an essentially useless feature, so please no.
>>
>> Doesn't Gerrit always use JGit?
>>
>> The discussion upthread is about what we should do about git.git's
>> version of this feature since we document it doesn't do its job from a
>> security / ACL standpoint, but that doesn't preclude other server
>> implementations from having a working version of this.
>>
>> But if gitolite is relying on this to hide refs, and if our security
>> docs are to be believed, then it's just providing security through
>> obscurity.
>
> Yes, Gerrit uses JGit. JGit shares configuration with Git, so if we
> make that configuration in Git warn "This is just a placebo", then
> people will naturally assume that it's just a placebo in JGit, too.
Aside from the merits of this change it sounds like a good idea to
document the server options JGit shares with us somewhere, or have a
test mode similar to what I added in (but didn't follow-up on
integrating) in
https://public-inbox.org/git/20170516203712.15921-1-avarab@gmail.com/
> More importantly, if Git upstream stops caring about this use case,
> then the protocol and other features can evolve in directions that
> make it even harder to support. I am willing to take on some of the
> burden of keeping it working, even when I do not run any servers that
> use plain Git (I do interact with many).
>
>> Like you I'm curious about a POC. The patch I submitted in
>> <20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at
>> face value.
>
> One of the difficulties that security engineers have to deal with is
> living without absolutes. In other words, their experience is
> constant risk/benefit tradeoffs: if you want 0% risk, then don't use a
> computer. ;-)
>
> If someone has thoughts on what would lead people to be comfortable
> with removing the SECURITY notice, then I'm happy to listen. As
> already mentioned, I am strongly against abandoning this use case.
For me this would be docs backed up by tests (which can start as a ML
reply) showing a case where this actually works to hide data.
I.e. have a repo with "master" and a "root-password" branch, where the
"root-password" branch has content that's irresistible to "git repack"
for delta purposes, do we end up sending the "root-password" content
over on a fetch even when that branch isn't advertised & forbidden?
Or, if that fails are there ways to make it work? E.g. using hidden/* in
combination with delta islands?
^ permalink raw reply
* [PATCH 3/3] setup: add `clear_repository_format()`
From: Martin Ågren @ 2018-12-18 7:25 UTC (permalink / raw)
To: git; +Cc: Jeff King, brian m . carlson
In-Reply-To: <20181218072528.3870492-1-martin.agren@gmail.com>
After we set up a `struct repository_format`, it owns various pieces of
allocated memory. We then either use those members, because we decide we
want to use the "candidate" repository format, or we discard the
candidate / scratch space. In the first case, we transfer ownership of
the memory to a few global variables. In the latter case, we just
silently drop the struct and end up leaking memory.
Introduce a function `clear_repository_format()` which frees the memory
the struct holds on to. Call it in the code paths where we currently
leak the memory. Also call it in the error path of
`read_repository_format()` to clean up any partial result.
For hygiene, we need to at least set the pointers that we free to NULL.
For future-proofing, let's zero the entire struct instead. It just means
that in the error path of `read_...()` we need to restore the error
sentinel in the `version` field.
We could take this opportunity to stop claiming that all fields except
`version` are undefined in case of an error. On the other hand, having
them defined as zero is not much better than having them undefined. We
could define them to some fallback configuration (`is_bare = -1` and
`hash_algo = GIT_HASH_SHA1`?), but "clear()" and/or "read()" seem like
the wrong places to enforce fallback configurations. Let's leave things
as "undefined" instead to encourage users to check `version`.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
The error state can always be defined later. Defining it now, then
trying to backpedal, is probably not so fun. Filling the struct with
non-zero values might help flush out bugs like the one fixed in the
previous patch, but I'm wary of going that far in this patch.
cache.h | 6 ++++++
repository.c | 1 +
setup.c | 14 ++++++++++++++
3 files changed, 21 insertions(+)
diff --git a/cache.h b/cache.h
index 8b9e592c65..53ac01efa7 100644
--- a/cache.h
+++ b/cache.h
@@ -979,6 +979,12 @@ struct repository_format {
*/
void read_repository_format(struct repository_format *format, const char *path);
+/*
+ * Free the memory held onto by `format`, but not the struct itself.
+ * (No need to use this after `read_repository_format()` fails.)
+ */
+void clear_repository_format(struct repository_format *format);
+
/*
* Verify that the repository described by repository_format is something we
* can read. If it is, return 0. Otherwise, return -1, and "err" will describe
diff --git a/repository.c b/repository.c
index 5dd1486718..efa9d1d960 100644
--- a/repository.c
+++ b/repository.c
@@ -159,6 +159,7 @@ int repo_init(struct repository *repo,
if (worktree)
repo_set_worktree(repo, worktree);
+ clear_repository_format(&format);
return 0;
error:
diff --git a/setup.c b/setup.c
index 52c3c9d31f..babe5ea156 100644
--- a/setup.c
+++ b/setup.c
@@ -517,6 +517,18 @@ void read_repository_format(struct repository_format *format, const char *path)
format->hash_algo = GIT_HASH_SHA1;
string_list_init(&format->unknown_extensions, 1);
git_config_from_file(check_repo_format, path, format);
+ if (format->version == -1) {
+ clear_repository_format(format);
+ format->version = -1;
+ }
+}
+
+void clear_repository_format(struct repository_format *format)
+{
+ string_list_clear(&format->unknown_extensions, 0);
+ free(format->work_tree);
+ free(format->partial_clone);
+ memset(format, 0, sizeof(*format));
}
int verify_repository_format(const struct repository_format *format,
@@ -1043,9 +1055,11 @@ int discover_git_directory(struct strbuf *commondir,
strbuf_release(&err);
strbuf_setlen(commondir, commondir_offset);
strbuf_setlen(gitdir, gitdir_offset);
+ clear_repository_format(&candidate);
return -1;
}
+ clear_repository_format(&candidate);
return 0;
}
--
2.20.1
^ permalink raw reply related
* [PATCH 2/3] setup: do not use invalid `repository_format`
From: Martin Ågren @ 2018-12-18 7:25 UTC (permalink / raw)
To: git; +Cc: Jeff King, brian m . carlson
In-Reply-To: <20181218072528.3870492-1-martin.agren@gmail.com>
If `read_repository_format()` encounters an error, `format->version`
will be -1 and all other fields of `format` will be undefined. However,
in `setup_git_directory_gently()`, we use `repo_fmt.hash_algo`
regardless of the value of `repo_fmt.version`.
This can be observed by adding this to the end of
`read_repository_format()`:
if (format->version == -1)
format->hash_algo = 0; /* no-one should peek at this! */
This causes, e.g., "git branch -m q q2 without config should succeed" in
t3200 to fail with "fatal: Failed to resolve HEAD as a valid ref."
because it has moved .git/config out of the way and is now trying to use
a bad hash algorithm.
Check that `version` is non-negative before using `hash_algo`.
This patch adds no tests, but do note that if we skip this patch, the
next patch would cause existing tests to fail as outlined above.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
I fully admit to not understanding all of this setup code, neither in
its current incarnation, nor in terms of an ideal end game. This check
seems like a good thing to do though.
setup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index 27747af7a3..52c3c9d31f 100644
--- a/setup.c
+++ b/setup.c
@@ -1138,7 +1138,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
setup_git_env(gitdir);
}
- if (startup_info->have_repository)
+ if (startup_info->have_repository && repo_fmt.version > -1)
repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
}
--
2.20.1
^ permalink raw reply related
* [PATCH 1/3] setup: drop return value from `read_repository_format()`
From: Martin Ågren @ 2018-12-18 7:25 UTC (permalink / raw)
To: git; +Cc: Jeff King, brian m . carlson
In-Reply-To: <20181218072528.3870492-1-martin.agren@gmail.com>
No-one looks at the return value, so we might as well drop it. It's
still available as `format->version`.
In v1 of what became commit 2cc7c2c737 ("setup: refactor repo format
reading and verification", 2016-03-11), this function actually had
return type "void", but that was changed in v2. Almost three years
later, no-one has used this return value.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
I only discovered the full history after writing the patch. Had I known
it from the beginning, maybe I'd have just skipped this step, but I was
sufficiently disturbed by the redundant and unused return value that I
dropped it before working on the actual meat of this series.
cache.h | 7 +++----
setup.c | 3 +--
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/cache.h b/cache.h
index ca36b44ee0..8b9e592c65 100644
--- a/cache.h
+++ b/cache.h
@@ -974,11 +974,10 @@ struct repository_format {
/*
* Read the repository format characteristics from the config file "path" into
- * "format" struct. Returns the numeric version. On error, -1 is returned,
- * format->version is set to -1, and all other fields in the struct are
- * undefined.
+ * "format" struct. On error, format->version is set to -1, and all other
+ * fields in the struct are undefined.
*/
-int read_repository_format(struct repository_format *format, const char *path);
+void read_repository_format(struct repository_format *format, const char *path);
/*
* Verify that the repository described by repository_format is something we
diff --git a/setup.c b/setup.c
index 1be5037f12..27747af7a3 100644
--- a/setup.c
+++ b/setup.c
@@ -509,7 +509,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
return 0;
}
-int read_repository_format(struct repository_format *format, const char *path)
+void read_repository_format(struct repository_format *format, const char *path)
{
memset(format, 0, sizeof(*format));
format->version = -1;
@@ -517,7 +517,6 @@ int read_repository_format(struct repository_format *format, const char *path)
format->hash_algo = GIT_HASH_SHA1;
string_list_init(&format->unknown_extensions, 1);
git_config_from_file(check_repo_format, path, format);
- return format->version;
}
int verify_repository_format(const struct repository_format *format,
--
2.20.1
^ permalink raw reply related
* [PATCH 0/3] setup: add `clear_repository_format()`
From: Martin Ågren @ 2018-12-18 7:25 UTC (permalink / raw)
To: git; +Cc: Jeff King, brian m . carlson
Patch 3 adds `clear_repository_format()` to plug a few memory leaks
related to `struct repository_format`. In preparation for that, patch 2
tightens up some code which uses a possibly-undefined value from the
struct. Patch 1 drops a return value that no-one has ever used.
Cc-ing Peff, who introduced `struct repository_format`, and brian, who
introduced its `hash_algo` field (see patch 2).
Martin Ågren (3):
setup: drop return value from `read_repository_format()`
setup: do not use invalid `repository_format`
setup: add `clear_repository_format()`
cache.h | 13 +++++++++----
repository.c | 1 +
setup.c | 19 ++++++++++++++++---
3 files changed, 26 insertions(+), 7 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH] stripspace: allow -s/-c outside git repository
From: Martin Ågren @ 2018-12-18 6:09 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git Mailing List, Han-Wen Nienhuys, Johannes Schindelin
In-Reply-To: <20181217165957.GA60293@google.com>
On Mon, 17 Dec 2018 at 22:56, Jonathan Nieder <jrnieder@gmail.com> wrote:
> That makes experimenting with the stripspace command unnecessarily
> fussy. Fix it by discovering the git directory gently, as intended
> all along.
> if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> - setup_git_directory_gently(NULL);
> + setup_git_directory_gently(&nongit);
> git_config(git_default_config, NULL);
> }
Makes me wonder if `setup_git_directory_gently()` should BUG if it
receives NULL. That would require some reshuffling in setup.c, since
`setup_git_directory()` calls out to it with NULL... Just thinking out
loud. In any case, and more importantly, this is the last user providing
NULL except for the one in `setup_git_directory()`.
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 5ce47e8af5..0c24a0f9a3 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -430,9 +430,15 @@ test_expect_success '-c with changed comment char' '
> test_expect_success '-c with comment char defined in .git/config' '
> test_config core.commentchar = &&
> printf "= foo\n" >expect &&
> - printf "foo" | (
> - mkdir sub && cd sub && git stripspace -c
> - ) >actual &&
> + rm -fr sub &&
> + mkdir sub &&
> + printf "foo" | git -C sub stripspace -c >actual &&
> + test_cmp expect actual
> +'
A small while-at-it conversion from subshell (with a funny pipe into it)
to `-C sub`. The `rm -fr` invocation is not in the original, so
shouldn't be needed. This one looks safe enough, though it makes me
wonder why you need it. `mkdir -p sub`? Probably not worth a v2.
> +
> +test_expect_success '-c outside git repository' '
> + printf "# foo\n" >expect &&
> + printf "foo" | nongit git stripspace -c >actual &&
> test_cmp expect actual
> '
Nice.
Martin
^ permalink raw reply
* Re: [PATCH] shallow: clear shallow cache when committing lock
From: Jonathan Nieder @ 2018-12-18 3:46 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, avarab
In-Reply-To: <20181218010811.143608-1-jonathantanmy@google.com>
Hi,
Jonathan Tan wrote:
> When setup_alternate_shallow() is invoked a second time in the same
> process, it fails with the error "shallow file has changed since we read
> it". One way of reproducing this is to fetch using protocol v2 in such a
> way that backfill_tags() is required, and that the responses to both
> requests contain a "shallow-info" section.
>
> This is because when the shallow lock is committed, the stat information
> of the shallow file is not cleared. Ensure that this information is
> always cleared whenever the shallow lock is committed by introducing a
> new API that hides the shallow lock behind a custom struct.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> This was discovered with the help of Aevar's GIT_TEST_PROTOCOL_VERSION
> patches.
Good catch. I think the above note should go in the commit message,
since it would be very useful to anyone looking back at this commit
message and trying to find a quick reproduction recipe.
[...]
> I couldn't figure out if the test case included in this patch can be
> reduced - if any one has any idea, help is appreciated.
It's short enough to be comprehensible, so I wouldn't worry too
much. :)
> I'm also not sure why this issue only occurs with protocol v2. It's true
> that, when using protocol v0, the server can communicate shallow
> information both before and after "want"s are received, and if shallow
> communication is only communicated before, the client will invoke
> setup_temporary_shallow() instead of setup_alternate_shallow(). (In
> protocol v2, shallow information is always communicated after "want"s,
> thus demonstrating the issue.) But even in protocol v0, writes to the
> shallow file go through setup_alternate_shallow() anyway (in
> update_shallow()), so I would think that the issue would occur, but it
> doesn't.
Good question. I'll try to poke more tomorrow morning, too.
[...]
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1,7 +1,6 @@
> #include "builtin.h"
> #include "repository.h"
> #include "config.h"
> -#include "lockfile.h"
> #include "pack.h"
> #include "refs.h"
> #include "pkt-line.h"
> @@ -864,7 +863,7 @@ static void refuse_unconfigured_deny_delete_current(void)
> static int command_singleton_iterator(void *cb_data, struct object_id *oid);
> static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
> {
> - struct lock_file shallow_lock = LOCK_INIT;
> + struct shallow_lock shallow_lock;
Interesting. Is there another name that can convey what this object
does more clearly? At first glance I would expect this to be a less
deep kind of lock.
I'm awful at naming, but a couple of ideas to get things started:
- 'struct shallow_update', since this represents a pending update to
the .git/shallow file?
- struct lock_file_for_shallow?
- struct locked_shallow_file?
- "struct shallow_file" or "struct shallow_commits_file"? This is a
handle representing the state of what will become .git/shallow file
once the caller commits the update.
[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
[...]
> @@ -1660,7 +1659,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
> error(_("remote did not send all necessary objects"));
> free_refs(ref_cpy);
> ref_cpy = NULL;
> - rollback_lock_file(&shallow_lock);
> + rollback_shallow_lock(&shallow_lock);
For a moment, I was worried that this line could be reached without
setup_alternate_shallow having been called first. Fortunately,
shallow_lock is static so it is zero-initialized automatically, making
that harmless.
[...]
> --- a/shallow.c
> +++ b/shallow.c
[...]
> @@ -358,6 +359,22 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
> strbuf_release(&sb);
> }
>
> +int commit_shallow_lock(struct shallow_lock *shallow_lock)
> +{
> + int ret;
> +
> + if ((ret = commit_lock_file(&shallow_lock->lock)))
> + return ret;
> + the_repository->parsed_objects->is_shallow = -1;
> + stat_validity_clear(the_repository->parsed_objects->shallow_stat);
In 'next', some other functions in this file handle an arbitrary
repository argument 'r'. Should this one as well? (I.e. can this
patch come with a conflict-resolution patch to handle that?)
[...]
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -471,6 +471,22 @@ test_expect_success 'upload-pack respects client shallows' '
Yay, thanks for the test. I'll study it more closely tomorrow.
Thanks and hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH] log: add %S option (like --source) to log --format
From: Issac Trotts @ 2018-12-18 3:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Noemi Mercado
In-Reply-To: <xmqqk1k8bitm.fsf@gitster-ct.c.googlers.com>
Hi Junio, thanks for your feedback. I'll submit a new patch as soon as
I've addressed all the feedback.
On Mon, Dec 17, 2018 at 1:31 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Issac Trotts <issac.trotts@gmail.com> writes:
>
> > Make it possible to write for example
> >
> > git log --format="%H,%S"
> >
> > where the %S at the end is a new placeholder that prints out the ref
> > (tag/branch) for each commit.
> >
> > Using %d might seem like an alternative but it only shows the ref for
> > the last commit in the branch.
>
> Have your sign-off here (see Documentation/SubmittingPatches),
Done.
> and then have a line that only has three-dashes on it,
>
> and then any other additional info like this.
Done.
> > This change is based on a question from Stack Overflow:
> > https://stackoverflow.com/questions/12712775/git-get-source-information-in-format
> > ---
>
>
>
>
> > Documentation/pretty-formats.txt | 2 ++
> > builtin/log.c | 2 +-
> > log-tree.c | 1 +
> > pretty.c | 10 +++++++
> > t/t4205-log-pretty-formats.sh | 50 ++++++++++++++++++++++++++++++++
> > 5 files changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> > index 417b638cd..acfe7e0a4 100644
> > --- a/Documentation/pretty-formats.txt
> > +++ b/Documentation/pretty-formats.txt
> > @@ -104,6 +104,8 @@ The placeholders are:
> >
> > - '%H': commit hash
> > - '%h': abbreviated commit hash
> > +- '%S': ref name given on the command line by which the commit was reached
> > + (like `git log --source`)
>
> This looks entirely out of place, among the description of the basic
> basic data like the object name of the commit itself, its tree, etc.
>
> Describe this immediately after %d and %D are explained, perhaps.
Done.
>
> > diff --git a/builtin/log.c b/builtin/log.c
> > index e8e51068b..a314ea2b6 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -203,7 +203,7 @@ static void cmd_log_init_finish(int argc, const
> > char **argv, const char *prefix,
> > rev->diffopt.filter || rev->diffopt.flags.follow_renames)
> > rev->always_show_header = 0;
> >
> > - if (source) {
> > + if (source || (rev->pretty_given && rev->commit_format ==
> > CMIT_FMT_USERFORMAT)) {
>
> Broken line (you let your MUA line-wrap and corrupt your patch???)
I see. I need to use `git send-email`.
>
> > init_revision_sources(&revision_sources);
> > rev->sources = &revision_sources;
> > }
>
> This means anybody who asks for say --format='%aN %s' pays the price
> of keeping track of each commit's source, which is unreasonable.
>
> Perhaps mimick the way how presence of %N is detected in
> userformat_want_item() so that we do not pay the price for
> init_display_notes() when a format that does not care about %N is
> given?
Done.
>
> > @@ -1149,6 +1150,15 @@ static size_t format_commit_one(struct strbuf
> > *sb, /* in UTF-8 */
> > parse_object(the_repository, &commit->object.oid);
> >
> > switch (placeholder[0]) {
> > + case 'S': /* tag/branch like --source */
> > + slot = revision_sources_at(c->pretty_ctx->rev->sources, commit);
> > + if (slot && *slot) {
> > + strbuf_addstr(sb, *slot);
> > + return 1;
> > + } else {
> > + die(_("failed to get info for %%S"));
> > + return 0;
> > + }
>
> Have this next to case arms that deal with 'd' and 'D".
Done.
>
> > case 'H': /* commit hash */
> > strbuf_addstr(sb, diff_get_color(c->auto_color, DIFF_COMMIT));
> > strbuf_addstr(sb, oid_to_hex(&commit->object.oid));
>
> This is a tangent, but I think this existing case arm should move to
> the previous block before parsing the commit, as %H only needs the
> object name of the commit itself (reword the comment before that
> switch to read from "independent of the commit" to "computable
> without parsing the commit")..
I hope it's all right if I keep my patch focused on just one thing.
^ permalink raw reply
* [PATCH] shallow: clear shallow cache when committing lock
From: Jonathan Tan @ 2018-12-18 1:08 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, avarab
When setup_alternate_shallow() is invoked a second time in the same
process, it fails with the error "shallow file has changed since we read
it". One way of reproducing this is to fetch using protocol v2 in such a
way that backfill_tags() is required, and that the responses to both
requests contain a "shallow-info" section.
This is because when the shallow lock is committed, the stat information
of the shallow file is not cleared. Ensure that this information is
always cleared whenever the shallow lock is committed by introducing a
new API that hides the shallow lock behind a custom struct.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This was discovered with the help of Aevar's GIT_TEST_PROTOCOL_VERSION
patches.
If rebased onto Aevar's GIT_TEST_PROTOCOL_VERSION patches [1], all tests
still pass with GIT_TEST_PROTOCOL_VERSION=2 and without. Also, this
patch allows the following diff to be applied:
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 8b1217ea26..8baa57cf84 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -791,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '
> '
>
> test_expect_success 'fetch exclude tag one' '
> - GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&
> + git -C shallow12 fetch --shallow-exclude one origin &&
> git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
> test_write_lines three two >expected &&
> test_cmp expected actual
(There is still one more instance of GIT_TEST_PROTOCOL_VERSION in that
file that is still not fixed.)
I couldn't figure out if the test case included in this patch can be
reduced - if any one has any idea, help is appreciated.
I'm also not sure why this issue only occurs with protocol v2. It's true
that, when using protocol v0, the server can communicate shallow
information both before and after "want"s are received, and if shallow
communication is only communicated before, the client will invoke
setup_temporary_shallow() instead of setup_alternate_shallow(). (In
protocol v2, shallow information is always communicated after "want"s,
thus demonstrating the issue.) But even in protocol v0, writes to the
shallow file go through setup_alternate_shallow() anyway (in
update_shallow()), so I would think that the issue would occur, but it
doesn't.
[1] https://public-inbox.org/git/20181213155817.27666-1-avarab@gmail.com/
---
builtin/receive-pack.c | 7 +++----
commit.h | 8 +++++++-
fetch-pack.c | 13 ++++++-------
shallow.c | 25 +++++++++++++++++++++----
t/t5702-protocol-v2.sh | 16 ++++++++++++++++
5 files changed, 53 insertions(+), 16 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 33187bd8e9..ab7bc5ceb1 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1,7 +1,6 @@
#include "builtin.h"
#include "repository.h"
#include "config.h"
-#include "lockfile.h"
#include "pack.h"
#include "refs.h"
#include "pkt-line.h"
@@ -864,7 +863,7 @@ static void refuse_unconfigured_deny_delete_current(void)
static int command_singleton_iterator(void *cb_data, struct object_id *oid);
static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
{
- struct lock_file shallow_lock = LOCK_INIT;
+ struct shallow_lock shallow_lock;
struct oid_array extra = OID_ARRAY_INIT;
struct check_connected_options opt = CHECK_CONNECTED_INIT;
uint32_t mask = 1 << (cmd->index % 32);
@@ -881,12 +880,12 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
opt.env = tmp_objdir_env(tmp_objdir);
setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);
if (check_connected(command_singleton_iterator, cmd, &opt)) {
- rollback_lock_file(&shallow_lock);
+ rollback_shallow_lock(&shallow_lock);
oid_array_clear(&extra);
return -1;
}
- commit_lock_file(&shallow_lock);
+ commit_shallow_lock(&shallow_lock);
/*
* Make sure setup_alternate_shallow() for the next ref does
diff --git a/commit.h b/commit.h
index 98664536cb..233a30ceef 100644
--- a/commit.h
+++ b/commit.h
@@ -9,6 +9,7 @@
#include "string-list.h"
#include "pretty.h"
#include "commit-slab.h"
+#include "lockfile.h"
#define COMMIT_NOT_FROM_GRAPH 0xFFFFFFFF
#define GENERATION_NUMBER_INFINITY 0xFFFFFFFF
@@ -224,9 +225,14 @@ extern struct commit_list *get_shallow_commits_by_rev_list(
extern void set_alternate_shallow_file(struct repository *r, const char *path, int override);
extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
const struct oid_array *extra);
-extern void setup_alternate_shallow(struct lock_file *shallow_lock,
+struct shallow_lock {
+ struct lock_file lock;
+};
+extern void setup_alternate_shallow(struct shallow_lock *shallow_lock,
const char **alternate_shallow_file,
const struct oid_array *extra);
+extern int commit_shallow_lock(struct shallow_lock *shallow_lock);
+extern void rollback_shallow_lock(struct shallow_lock *shallow_lock);
extern const char *setup_temporary_shallow(const struct oid_array *extra);
extern void advertise_shallow_grafts(int);
diff --git a/fetch-pack.c b/fetch-pack.c
index 9691046e64..1ae1cf225a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1,7 +1,6 @@
#include "cache.h"
#include "repository.h"
#include "config.h"
-#include "lockfile.h"
#include "refs.h"
#include "pkt-line.h"
#include "commit.h"
@@ -34,7 +33,7 @@ static int fetch_fsck_objects = -1;
static int transfer_fsck_objects = -1;
static int agent_supported;
static int server_supports_filtering;
-static struct lock_file shallow_lock;
+static struct shallow_lock shallow_lock;
static const char *alternate_shallow_file;
static char *negotiation_algorithm;
static struct strbuf fsck_msg_types = STRBUF_INIT;
@@ -1512,9 +1511,9 @@ static void update_shallow(struct fetch_pack_args *args,
if (args->deepen && alternate_shallow_file) {
if (*alternate_shallow_file == '\0') { /* --unshallow */
unlink_or_warn(git_path_shallow(the_repository));
- rollback_lock_file(&shallow_lock);
+ rollback_shallow_lock(&shallow_lock);
} else
- commit_lock_file(&shallow_lock);
+ commit_shallow_lock(&shallow_lock);
return;
}
@@ -1537,7 +1536,7 @@ static void update_shallow(struct fetch_pack_args *args,
setup_alternate_shallow(&shallow_lock,
&alternate_shallow_file,
&extra);
- commit_lock_file(&shallow_lock);
+ commit_shallow_lock(&shallow_lock);
}
oid_array_clear(&extra);
return;
@@ -1574,7 +1573,7 @@ static void update_shallow(struct fetch_pack_args *args,
setup_alternate_shallow(&shallow_lock,
&alternate_shallow_file,
&extra);
- commit_lock_file(&shallow_lock);
+ commit_shallow_lock(&shallow_lock);
oid_array_clear(&extra);
oid_array_clear(&ref);
return;
@@ -1660,7 +1659,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
error(_("remote did not send all necessary objects"));
free_refs(ref_cpy);
ref_cpy = NULL;
- rollback_lock_file(&shallow_lock);
+ rollback_shallow_lock(&shallow_lock);
goto cleanup;
}
args->connectivity_checked = 1;
diff --git a/shallow.c b/shallow.c
index 02fdbfc554..0d39906419 100644
--- a/shallow.c
+++ b/shallow.c
@@ -333,22 +333,23 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
return "";
}
-void setup_alternate_shallow(struct lock_file *shallow_lock,
+void setup_alternate_shallow(struct shallow_lock *shallow_lock,
const char **alternate_shallow_file,
const struct oid_array *extra)
{
struct strbuf sb = STRBUF_INIT;
int fd;
- fd = hold_lock_file_for_update(shallow_lock,
+ fd = hold_lock_file_for_update(&shallow_lock->lock,
git_path_shallow(the_repository),
LOCK_DIE_ON_ERROR);
check_shallow_file_for_update(the_repository);
if (write_shallow_commits(&sb, 0, extra)) {
if (write_in_full(fd, sb.buf, sb.len) < 0)
die_errno("failed to write to %s",
- get_lock_file_path(shallow_lock));
- *alternate_shallow_file = get_lock_file_path(shallow_lock);
+ get_lock_file_path(&shallow_lock->lock));
+ *alternate_shallow_file =
+ get_lock_file_path(&shallow_lock->lock);
} else
/*
* is_repository_shallow() sees empty string as "no
@@ -358,6 +359,22 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
strbuf_release(&sb);
}
+int commit_shallow_lock(struct shallow_lock *shallow_lock)
+{
+ int ret;
+
+ if ((ret = commit_lock_file(&shallow_lock->lock)))
+ return ret;
+ the_repository->parsed_objects->is_shallow = -1;
+ stat_validity_clear(the_repository->parsed_objects->shallow_stat);
+ return 0;
+}
+
+void rollback_shallow_lock(struct shallow_lock *shallow_lock)
+{
+ rollback_lock_file(&shallow_lock->lock);
+}
+
static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *cb)
{
int fd = *(int *)cb;
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..390a71399d 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -471,6 +471,22 @@ test_expect_success 'upload-pack respects client shallows' '
grep "fetch< version 2" trace
'
+test_expect_success '2 fetches in one process updating shallow' '
+ rm -rf server client trace &&
+
+ test_create_repo server &&
+ test_commit -C server one &&
+ test_commit -C server two &&
+ test_commit -C server three &&
+ git clone --shallow-exclude two "file://$(pwd)/server" client &&
+
+ # Triggers tag following (thus, 2 fetches in one process)
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+ fetch --shallow-exclude one origin &&
+ # Ensure that protocol v2 is used
+ grep "fetch< version 2" trace
+'
+
# Test protocol v2 with 'http://' transport
#
. "$TEST_DIRECTORY"/lib-httpd.sh
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
From: Jonathan Nieder @ 2018-12-18 0:02 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, git, Junio C Hamano, Brandon Williams, Jonathan Tan
In-Reply-To: <874lbb209x.fsf@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason wrote:
> On Mon, Dec 17 2018, Jonathan Nieder wrote:
>> This would make per-branch ACLs (as implemented both by Gerrit and
>> gitolite) an essentially useless feature, so please no.
>
> Doesn't Gerrit always use JGit?
>
> The discussion upthread is about what we should do about git.git's
> version of this feature since we document it doesn't do its job from a
> security / ACL standpoint, but that doesn't preclude other server
> implementations from having a working version of this.
>
> But if gitolite is relying on this to hide refs, and if our security
> docs are to be believed, then it's just providing security through
> obscurity.
Yes, Gerrit uses JGit. JGit shares configuration with Git, so if we
make that configuration in Git warn "This is just a placebo", then
people will naturally assume that it's just a placebo in JGit, too.
More importantly, if Git upstream stops caring about this use case,
then the protocol and other features can evolve in directions that
make it even harder to support. I am willing to take on some of the
burden of keeping it working, even when I do not run any servers that
use plain Git (I do interact with many).
> Like you I'm curious about a POC. The patch I submitted in
> <20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at
> face value.
One of the difficulties that security engineers have to deal with is
living without absolutes. In other words, their experience is
constant risk/benefit tradeoffs: if you want 0% risk, then don't use a
computer. ;-)
If someone has thoughts on what would lead people to be comfortable
with removing the SECURITY notice, then I'm happy to listen. As
already mentioned, I am strongly against abandoning this use case.
Jonathan
^ permalink raw reply
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 23:36 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, git, Junio C Hamano, Brandon Williams, Jonathan Tan
In-Reply-To: <20181217231452.GA13835@google.com>
On Mon, Dec 17 2018, Jonathan Nieder wrote:
> Hi,
>
> Jeff King wrote:
>> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>>> More importantly this bypasses the security guarantee we've had with the
>>> default of uploadpack.allowAnySHA1InWant=false.
>>
>> IMHO those security guarantees there are overrated (due to delta
>> guessing attacks, though things are not quite as bad if the attacker
>> can't actually push to the repo).
>
> Do you have a proof of concept for delta guessing? My understanding
> was that without using a broken hash (e.g. uncorrected SHA-1), it is
> not feasible to carry out.
>
> JGit checks delta bases in received thin packs for reachability as
> well.
>
>> But I agree that people do assume it's the case. I was certainly
>> surprised by the v2 behavior, and I don't remember that aspect being
>> discussed.
>
> IMHO it's a plain bug (either in implementation or documentation).
>
> [...]
>>> I'm inclined to say that in the face of that "SECURITY" section we
>>> should just:
>>>
>>> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
>>> default. Make saying uploadpack.allowReachableSHA1InWant=false warn
>>> with "this won't work, see SECURITY...".
>>>
>>> * The uploadpack.allowTipSHA1InWant setting will also be turned on by
>>> default, and will be much faster, since it'll just degrade to
>>> uploadpack.allowReachableSHA1InWant=true and we won't need any
>>> reachability check. We'll also warn saying that setting it is
>>> useless.
>>
>> No real argument from me. I have always thought those security
>> guarantees were BS.
>
> This would make per-branch ACLs (as implemented both by Gerrit and
> gitolite) an essentially useless feature, so please no.
Doesn't Gerrit always use JGit?
The discussion upthread is about what we should do about git.git's
version of this feature since we document it doesn't do its job from a
security / ACL standpoint, but that doesn't preclude other server
implementations from having a working version of this.
But if gitolite is relying on this to hide refs, and if our security
docs are to be believed, then it's just providing security through
obscurity.
Like you I'm curious about a POC. The patch I submitted in
<20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at
face value. I.e. I think it's dangerous to expose this as-is given the
scary non-promise they make, but perhaps we'll find that this actually
works well enough in some cases, or there's other non-security uses for
this (e.g. simply discouraging users from cloning certain things,
e.g. for cache performance purposes).
> I would be all for changing the default, but making turning off
> allowReachableSHA1InWant an unsupported deprecated thing is a step too
> far, in my opinion.
>
> Is there somewhere that we can document these kinds of invariants or
> goals so that we don't have to keep repeating the same discussions?
>
> Thanks,
> Jonathan
^ permalink raw reply
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
From: Jonathan Nieder @ 2018-12-17 23:14 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Brandon Williams, Jonathan Tan
In-Reply-To: <20181217195713.GA10673@sigill.intra.peff.net>
Hi,
Jeff King wrote:
> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> More importantly this bypasses the security guarantee we've had with the
>> default of uploadpack.allowAnySHA1InWant=false.
>
> IMHO those security guarantees there are overrated (due to delta
> guessing attacks, though things are not quite as bad if the attacker
> can't actually push to the repo).
Do you have a proof of concept for delta guessing? My understanding
was that without using a broken hash (e.g. uncorrected SHA-1), it is
not feasible to carry out.
JGit checks delta bases in received thin packs for reachability as
well.
> But I agree that people do assume it's the case. I was certainly
> surprised by the v2 behavior, and I don't remember that aspect being
> discussed.
IMHO it's a plain bug (either in implementation or documentation).
[...]
>> I'm inclined to say that in the face of that "SECURITY" section we
>> should just:
>>
>> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
>> default. Make saying uploadpack.allowReachableSHA1InWant=false warn
>> with "this won't work, see SECURITY...".
>>
>> * The uploadpack.allowTipSHA1InWant setting will also be turned on by
>> default, and will be much faster, since it'll just degrade to
>> uploadpack.allowReachableSHA1InWant=true and we won't need any
>> reachability check. We'll also warn saying that setting it is
>> useless.
>
> No real argument from me. I have always thought those security
> guarantees were BS.
This would make per-branch ACLs (as implemented both by Gerrit and
gitolite) an essentially useless feature, so please no.
I would be all for changing the default, but making turning off
allowReachableSHA1InWant an unsupported deprecated thing is a step too
far, in my opinion.
Is there somewhere that we can document these kinds of invariants or
goals so that we don't have to keep repeating the same discussions?
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true
From: David Turner @ 2018-12-17 23:07 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Fredrik Medley, Matt McCutchen
In-Reply-To: <875zvr2235.fsf@evledraar.gmail.com>
On December 17, 2018 5:57:50 PM EST, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:
>
>On Mon, Dec 17 2018, David Turner wrote:
>
>> Overall, I like this. One nit:
>
>Thanks for the review!
>
>> On December 17, 2018 5:16:25 PM EST, "Ævar Arnfjörð Bjarmason"
><avarab@gmail.com> wrote:
>>>--- a/upload-pack.c
>>>+++ b/upload-pack.c
>>>@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;
>>> #define ALLOW_REACHABLE_SHA1 02
>>>/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and
>>>ALLOW_REACHABLE_SHA1. */
>>> #define ALLOW_ANY_SHA1 07
>>>-static unsigned int allow_unadvertised_object_request;
>>>+static unsigned int allow_unadvertised_object_request = (
>>>+ ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);
>>
>> ALLOW_ANY_SHA1 already includes the other two.
>
>FWIW (and maybe I made the wrong call, and have no preference realy) I
>opted for this as part of "this change doesn't do any of the hard work
>of extracting the now-dead ALLOW_[...]*.
>
>I.e. the diff context I had doesn't show all the ALLOW_* values, and
>with the context given it might be easier for reviewers to look no
>further than "this is what you'd get before if all
>uploadpack.allow.*SHA1InWant was true".
The context I quoted said /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and
ALLOW_REACHABLE_SHA1. */
But up to you (or Junio).
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:57 UTC (permalink / raw)
To: David Turner
Cc: git, Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Fredrik Medley, Matt McCutchen
In-Reply-To: <F368D768-7DC6-479C-8888-2A41360B0B6A@novalis.org>
On Mon, Dec 17 2018, David Turner wrote:
> Overall, I like this. One nit:
Thanks for the review!
> On December 17, 2018 5:16:25 PM EST, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:
>>--- a/upload-pack.c
>>+++ b/upload-pack.c
>>@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;
>> #define ALLOW_REACHABLE_SHA1 02
>>/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and
>>ALLOW_REACHABLE_SHA1. */
>> #define ALLOW_ANY_SHA1 07
>>-static unsigned int allow_unadvertised_object_request;
>>+static unsigned int allow_unadvertised_object_request = (
>>+ ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);
>
> ALLOW_ANY_SHA1 already includes the other two.
FWIW (and maybe I made the wrong call, and have no preference realy) I
opted for this as part of "this change doesn't do any of the hard work
of extracting the now-dead ALLOW_[...]*.
I.e. the diff context I had doesn't show all the ALLOW_* values, and
with the context given it might be easier for reviewers to look no
further than "this is what you'd get before if all
uploadpack.allow.*SHA1InWant was true".
^ permalink raw reply
* Re: Can git tell me which uncommitted files clash with the incoming changes?
From: Mark Kharitonov @ 2018-12-17 22:50 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Elijah Newren, Git Mailing List
In-Reply-To: <CACsJy8BFoK4hoXrSUi+P3xB1LumevvFe6XWAM2fLUq-UGNUs8A@mail.gmail.com>
Guys, having git merge --dry-run would be great, but I am OK with git
merge for real as long as its output is parseable.
However, somewhere in between git 2.18 and git 2.20 the output of
merge changed and now I do not know how to parse it.
it used to be something like that:
bla bla bla
<tab>file name 1
<tab>file name 2
...
bla bla bla
But now, the files are output in one line and given that some files
may have spaces in the name I do not see how this can be parsed. If we
could have easily parseable output of merge, it would be enough for
me.
Le lun. 17 déc. 2018 à 14:37, Duy Nguyen <pclouds@gmail.com> a écrit :
>
> On Mon, Dec 17, 2018 at 6:17 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Mon, Dec 17, 2018 at 8:26 AM Duy Nguyen <pclouds@gmail.com> wrote:
> > >
> > > On Mon, Dec 17, 2018 at 2:11 PM Mark Kharitonov
> > > <mark.kharitonov@gmail.com> wrote:
> > > >
> > > > Hi,
> > > > I have asked this question on SO
> > > > (https://stackoverflow.com/questions/53679167/can-git-tell-me-which-uncommitted-files-clash-with-the-incoming-changes)
> > > > and usually there are tons of responses on Git questions, but not on
> > > > this one.
> > > >
> > > > Allow me to quote it now.
> > > >
> > > > Please, observe:
> > > >
> > > > C:\Dayforce\test [master ↓2 +0 ~2 -0 !]> git pull
> > > > error: Your local changes to the following files would be
> > > > overwritten by merge:
> > > > 2.txt
> > > > Please commit your changes or stash them before you merge.
> > > > Aborting
> > > > Updating 2dc8bd0..ea343f8
> > > > C:\Dayforce\test [master ↓2 +0 ~2 -0 !]>
> > > >
> > > > Does git have a command that can tell me which uncommitted files cause
> > > > the this error? I can see them displayed by git pull, but I really do
> > > > not want to parse git pull output.
> > >
> > > Assume that you have done "git fetch origin" (or whatever master's
> > > upstream is). Do
> > >
> > > git diff --name-only HEAD origin/master
> > >
> > > You get the list of files that will need to be updated. Do
> > >
> > > git diff --name-only
> >
> > Are you assuming that `git diff --cached --name-only` is empty? If it
> > isn't, that alone will trigger a failure (unless using an esoteric
> > merge strategy or an older version of git), so this assumption is
> > fairly reasonable to make. But it may be worth being explicit about
> > for external readers.
>
> Actually I think Jeff's suggestion may be better since he compares
> worktree with HEAD and should catch everything.
>
> > > to get the list of files that have local changes. If this list shares
> > > some paths with the first list, these paths will very likely cause
> > > "git pull" to abort.
> > >
> > > For a better check, I think you need to do "git read-tree -m" by
> > > yourself (to a temporary index file with --index-output) then you can
> > > examine that file and determine what file has changed compared to HEAD
> > > (and if the same file has local changes, git-pull will be aborted).
> > > You may need to read more in read-tree man page.
> > >
> > > Ideally though, git-read-tree should be able to tell what paths are
> > > updated in "--dry-run -u" mode. But I don't think it's supported yet.
> >
> > merge-recursive currently uses unpack_trees to do this "files would be
> > overwritten by merge" checking, so the suggestion of read-tree (which
> > also uses unpack_trees) makes sense. BUT ... the error checking in
> > unpack_trees has both false positives and false negatives due to not
> > understanding renames, and it is somewhat of a nightmarish mess. See
> > [1] for details. Further, I think it warns in cases that shouldn't be
> > needed (both sides of history modified the same file, with the
> > modifications on HEAD's side being a superset of the changes on the
> > other side, in such a way that 3-way content merge happens to match
> > what is in HEAD already). So, while the suggestions made so far give
> > some useful approximations, it's an approximation that will get worse
> > over time.
>
> Ah.. dang. I guess we need "git merge --dry-run" then :)
>
> > I don't have a better approximation to provide at this
> > time, though.
> >
> >
> > Elijah
> >
> > [1] https://public-inbox.org/git/20171124195901.2581-1-newren@gmail.com/
> > , starting at "Note that unpack_trees() doesn't understand renames"
> > and running until "4-way merges simply cause the complexity to
> > increase with every new capability."
>
>
>
> --
> Duy
--
Be well and prosper.
==============================
"There are two kinds of people.Those whose guns are loaded and those who dig."
("The good, the bad and the ugly")
So let us drink for our guns always be loaded.
^ permalink raw reply
* Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true
From: David Turner @ 2018-12-17 22:34 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Fredrik Medley, Matt McCutchen
In-Reply-To: <20181217221625.1523-1-avarab@gmail.com>
Overall, I like this. One nit:
On December 17, 2018 5:16:25 PM EST, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:
>--- a/upload-pack.c
>+++ b/upload-pack.c
>@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;
> #define ALLOW_REACHABLE_SHA1 02
>/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and
>ALLOW_REACHABLE_SHA1. */
> #define ALLOW_ANY_SHA1 07
>-static unsigned int allow_unadvertised_object_request;
>+static unsigned int allow_unadvertised_object_request = (
>+ ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);
ALLOW_ANY_SHA1 already includes the other two.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* [PATCH v3 4/4] fetch-pack: support protocol version 2
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Josh Steadmon, Ævar Arnfjörð Bjarmason
In-Reply-To: <20181213155817.27666-1-avarab@gmail.com>
From: Jonathan Tan <jonathantanmy@google.com>
When the scaffolding for protocol version 2 was initially added in
8f6982b4e1 ("protocol: introduce enum protocol_version value
protocol_v2", 2018-03-14). As seen in:
git log -p -G'support for protocol v2 not implemented yet' --full-diff --reverse v2.17.0..v2.20.0
Many of those scaffolding "die" placeholders were removed, but we
hadn't gotten around to fetch-pack yet.
The test here for "fetch refs from cmdline" is very minimal. There's
much better coverage when running the entire test suite under the WIP
GIT_TEST_PROTOCOL_VERSION=2 mode[1], we should ideally have better
coverage without needing to invoke a special test mode.
1. https://public-inbox.org/git/20181213155817.27666-1-avarab@gmail.com/
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/fetch-pack.c | 9 ++++++---
t/t5500-fetch-pack.sh | 22 +++++++++++++++-------
2 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 63e69a58011..f6a513495ea 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
struct oid_array shallow = OID_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
struct packet_reader reader;
+ enum protocol_version version;
fetch_if_missing = 0;
@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_GENTLE_ON_EOF);
- switch (discover_version(&reader)) {
+ version = discover_version(&reader);
+ switch (version) {
case protocol_v2:
- die("support for protocol v2 not implemented yet");
+ get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+ break;
case protocol_v1:
case protocol_v0:
get_remote_heads(&reader, &ref, 0, NULL, &shallow);
@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
}
ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
- &shallow, pack_lockfile_ptr, protocol_v0);
+ &shallow, pack_lockfile_ptr, version);
if (pack_lockfile) {
printf("lock %s\n", pack_lockfile);
fflush(stdout);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 086f2c40f68..49c540b1e1d 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -439,15 +439,23 @@ test_expect_success 'setup tests for the --stdin parameter' '
) >input.dup
'
-test_expect_success 'fetch refs from cmdline' '
- (
- cd client &&
- git fetch-pack --no-progress .. $(cat ../input)
- ) >output &&
- cut -d " " -f 2 <output | sort >actual &&
- test_cmp expect actual
+test_expect_success 'setup fetch refs from cmdline v[12]' '
+ cp -r client client1 &&
+ cp -r client client2
'
+for version in '' 1 2
+do
+ test_expect_success "protocol.version=$version fetch refs from cmdline" "
+ (
+ cd client$version &&
+ GIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. \$(cat ../input)
+ ) >output &&
+ cut -d ' ' -f 2 <output | sort >actual &&
+ test_cmp expect actual
+ "
+done
+
test_expect_success 'fetch refs from stdin' '
(
cd client &&
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related
* [PATCH v3 2/4] parse_hide_refs_config: handle NULL section
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Josh Steadmon, Ævar Arnfjörð Bjarmason
In-Reply-To: <20181213155817.27666-1-avarab@gmail.com>
From: Jeff King <peff@peff.net>
This helper function looks for config in two places: transfer.hiderefs,
or $section.hiderefs, where $section is passed in by the caller (and is
"uploadpack" or "receive", depending on the context).
In preparation for callers which do not even have that context (namely
the "git-serve" command), let's handle a NULL by looking only at
transfer.hiderefs (as opposed to segfaulting).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index f9936355cda..099e91d9cc0 100644
--- a/refs.c
+++ b/refs.c
@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
{
const char *key;
if (!strcmp("transfer.hiderefs", var) ||
- (!parse_config_key(var, section, NULL, NULL, &key) &&
+ (section &&
+ !parse_config_key(var, section, NULL, NULL, &key) &&
!strcmp(key, "hiderefs"))) {
char *ref;
int len;
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related
* [PATCH v3 3/4] upload-pack: support hidden refs with protocol v2
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Josh Steadmon, Ævar Arnfjörð Bjarmason
In-Reply-To: <20181213155817.27666-1-avarab@gmail.com>
From: Jeff King <peff@peff.net>
In the v2 protocol, upload-pack's advertisement has been moved to the
"ls-refs" command. That command does not respect hidden-ref config (like
transfer.hiderefs) at all, and advertises everything.
While there are some features that are not supported in v2 (e.g., v2
always allows fetching any sha1 without respect to advertisements), the
lack of this feature is not documented and is likely just a bug. Let's
make it work, as otherwise upgrading a server to a v2-capable git will
start exposing these refs that the repository admin has asked to remain
hidden.
Note that we depend on the config_context from the caller here to
realize that we should respect uploadpack.hiderefs. When run from the
"git-serve" tool, we won't have that context and will only respect
transfer.hiderefs. This should be OK, as git-serve is not actually used
for normal protocol operations.
Note also that we don't need to worry about the "An attempt to update
or delete a hidden ref by git push is rejected" feature of
receive.hideRefs, see git-config(1). This is because there's no v2
push protocol yet.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
ls-refs.c | 12 ++++++++++++
t/t5512-ls-remote.sh | 6 ++++++
2 files changed, 18 insertions(+)
diff --git a/ls-refs.c b/ls-refs.c
index e8e31475f06..8a8143338b4 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -5,6 +5,7 @@
#include "argv-array.h"
#include "ls-refs.h"
#include "pkt-line.h"
+#include "config.h"
/*
* Check if one of the prefixes is a prefix of the ref.
@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
const char *refname_nons = strip_namespace(refname);
struct strbuf refline = STRBUF_INIT;
+ if (ref_is_hidden(refname_nons, refname))
+ return 0;
+
if (!ref_match(&data->prefixes, refname))
return 0;
@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,
return 0;
}
+static int ls_refs_config(const char *var, const char *value,
+ void *config_context)
+{
+ return parse_hide_refs_config(var, value, config_context);
+}
+
int ls_refs(struct repository *r,
const char *config_section,
struct argv_array *keys,
@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,
memset(&data, 0, sizeof(data));
+ git_config(ls_refs_config, (void *)config_section);
+
while (packet_reader_read(request) != PACKET_READ_FLUSH) {
const char *arg = request->line;
const char *out;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2ed..ca69636fd52 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
grep refs/tags/magic actual
'
+test_expect_success 'protocol v2 supports hiderefs' '
+ test_config uploadpack.hiderefs refs/tags &&
+ git -c protocol.version=2 ls-remote . >actual &&
+ ! grep refs/tags actual
+'
+
test_expect_success 'ls-remote --symref' '
git fetch origin &&
cat >expect <<-EOF &&
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related
* [PATCH v3 1/4] serve: pass "config context" through to individual commands
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Josh Steadmon, Ævar Arnfjörð Bjarmason
In-Reply-To: <20181213155817.27666-1-avarab@gmail.com>
From: Jeff King <peff@peff.net>
In protocol v2, instead of just running "upload-pack", we have a generic
"serve" loop which runs command requests from the client. What used to
be "upload-pack" is now generally split into two operations: "ls-refs"
and "fetch". The latter knows it must respect uploadpack.* config, but
the former is not actually specific to a fetch operation (we do not yet
do v2 receive-pack, but eventually we may, and ls-refs would support
both operations).
However, ls-refs does need to know which operation we're performing, in
order to read the correct config (e.g., uploadpack.hiderefs versus
receive.hiderefs; we don't read _either_ right now, but this is the
first step to fixing that).
In the generic "git-serve" program, we don't have that information. But
in the protocol as it is actually used by clients, the client still asks
to run "git-upload-pack", and then issues an "ls-refs" from there. So we
_do_ know at that point that "uploadpack" is the right config context.
This patch teaches the protocol v2 "serve" code to pass that context
through to the individual commands (a future patch will teach ls-refs to
actually use it).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/upload-pack.c | 1 +
ls-refs.c | 4 +++-
ls-refs.h | 3 ++-
serve.c | 9 +++++----
serve.h | 7 +++++++
upload-pack.c | 4 ++--
upload-pack.h | 4 ++--
7 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 42dc4da5a1f..67192cfa9e9 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
case protocol_v2:
serve_opts.advertise_capabilities = opts.advertise_refs;
serve_opts.stateless_rpc = opts.stateless_rpc;
+ serve_opts.config_context = "uploadpack";
serve(&serve_opts);
break;
case protocol_v1:
diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8d..e8e31475f06 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
return 0;
}
-int ls_refs(struct repository *r, struct argv_array *keys,
+int ls_refs(struct repository *r,
+ const char *config_section,
+ struct argv_array *keys,
struct packet_reader *request)
{
struct ls_refs_data data;
diff --git a/ls-refs.h b/ls-refs.h
index b62877e8dad..da26fc9824d 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -4,7 +4,8 @@
struct repository;
struct argv_array;
struct packet_reader;
-extern int ls_refs(struct repository *r, struct argv_array *keys,
+extern int ls_refs(struct repository *r, const char *config_context,
+ struct argv_array *keys,
struct packet_reader *request);
#endif /* LS_REFS_H */
diff --git a/serve.c b/serve.c
index bda085f09c8..70f89cf0d98 100644
--- a/serve.c
+++ b/serve.c
@@ -48,6 +48,7 @@ struct protocol_capability {
* This field should be NULL for capabilities which are not commands.
*/
int (*command)(struct repository *r,
+ const char *config_context,
struct argv_array *keys,
struct packet_reader *request);
};
@@ -158,7 +159,7 @@ enum request_state {
PROCESS_REQUEST_DONE,
};
-static int process_request(void)
+static int process_request(struct serve_options *opts)
{
enum request_state state = PROCESS_REQUEST_KEYS;
struct packet_reader reader;
@@ -222,7 +223,7 @@ static int process_request(void)
if (!command)
die("no command requested");
- command->command(the_repository, &keys, &reader);
+ command->command(the_repository, opts->config_context, &keys, &reader);
argv_array_clear(&keys);
return 0;
@@ -249,10 +250,10 @@ void serve(struct serve_options *options)
* a single request/response exchange
*/
if (options->stateless_rpc) {
- process_request();
+ process_request(options);
} else {
for (;;)
- if (process_request())
+ if (process_request(options))
break;
}
}
diff --git a/serve.h b/serve.h
index fe65ba9f469..d527224cbb1 100644
--- a/serve.h
+++ b/serve.h
@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,
struct serve_options {
unsigned advertise_capabilities;
unsigned stateless_rpc;
+
+ /*
+ * Some operations may need to know the context when looking up config;
+ * e.g., set this to "uploadpack" to respect "uploadpack.hiderefs" (as
+ * opposed to "receive.hiderefs").
+ */
+ const char *config_context;
};
#define SERVE_OPTIONS_INIT { 0 }
extern void serve(struct serve_options *options);
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24f..914bbb40bcd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1413,8 +1413,8 @@ enum fetch_state {
FETCH_DONE,
};
-int upload_pack_v2(struct repository *r, struct argv_array *keys,
- struct packet_reader *request)
+int upload_pack_v2(struct repository *r, const char *config_context,
+ struct argv_array *keys, struct packet_reader *request)
{
enum fetch_state state = FETCH_PROCESS_ARGS;
struct upload_pack_data data;
diff --git a/upload-pack.h b/upload-pack.h
index cab2178796a..2a9f51197c4 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);
struct repository;
struct argv_array;
struct packet_reader;
-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,
- struct packet_reader *request);
+extern int upload_pack_v2(struct repository *r, const char *config_context,
+ struct argv_array *keys, struct packet_reader *request);
struct strbuf;
extern int upload_pack_advertise(struct repository *r,
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox