* Re: [PATCH v3 0/9] Fix the early config
From: Jeff King @ 2017-03-08 17:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Duy Nguyen
In-Reply-To: <xmqqy3wf3dbo.fsf@gitster.mtv.corp.google.com>
On Wed, Mar 08, 2017 at 09:09:31AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Good catch. Another "non-gentle" thing I noticed here while looking at
> > another thread: the repository-format version check uses the config
> > parser, which will die() in certain circumstances. So for instance:
> >
> > $ git init
> > $ git rev-parse && echo ok
> > ok
> >
> > $ echo '[core]repositoryformatversion = 10' >.git/config
> > $ git rev-parse && echo ok
> > fatal: Expected git repo version <= 1, found 10
>
> Just to set my expectation straight. Do you expect/wish this not to
> fail because of this in cmd_rev_parse()?
No, I was just using "rev-parse" as a sample command that tried to do
repo setup. I meant the above snippet that you quoted to both be fine
and expected outputs. The problem is the _other_ two cases where the
config code dies before we even get to the version-number check.
> Or are you discussing a more general issue, iow, anything that can
> work without repository (i.e. those who do _gently version of the
> setup and act on *nongit_ok) should pretend as if there were no
> (broken) repository and take the "no we are not in a repository"
> codepath?
Yes, exactly. It would have been less confusing if I picked something
that passed nongit_ok. Like hash-object:
$ git init
$ echo content >file
$ git hash-object file
d95f3ad14dee633a758d2e331151e950dd13e4ed
$ echo '[core]repositoryformatversion = 10' >.git/config
$ git hash-object file
warning: Expected git repo version <= 1, found 10
d95f3ad14dee633a758d2e331151e950dd13e4ed
The warning is fine and reasonable here. But then:
$ echo '[core]repositoryformatversion = foobar' >.git/config
$ git hash-object file
fatal: bad numeric config value 'foobar' for 'core.repositoryformatversion' in file .git/config: invalid unit
That's wrong. We're supposed to be gentle. And ditto:
$ echo '[co' >.git/config
$ git hash-object file
fatal: bad config line 1 in file .git/config
Those last two should issue a warning at most, and then let the command
continue.
-Peff
^ permalink raw reply
* Re: [PATCH 0/6] deadlock regression in v2.11.0 with failed mkdtemp
From: Horst Schirmeier @ 2017-03-08 17:58 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net>
On Tue, 07 Mar 2017, Jeff King wrote:
> On Tue, Mar 07, 2017 at 12:14:06PM +0100, Horst Schirmeier wrote:
> > On Tue, 07 Mar 2017, Horst Schirmeier wrote:
> > > I observe a regression that seems to have been introduced between
> > > v2.10.0 and v2.11.0. When I try to push into a repository on the local
> > > filesystem that belongs to another user and has not explicitly been
> > > prepared for shared use, v2.11.0 shows some of the usual diagnostic
> > > output and then freezes instead of announcing why it failed to push.
> >
> > Bisecting points to v2.10.1-373-g722ff7f:
> >
> > 722ff7f876c8a2ad99c42434f58af098e61b96e8 is the first bad commit
> > commit 722ff7f876c8a2ad99c42434f58af098e61b96e8
> > Author: Jeff King <peff@peff.net>
> > Date: Mon Oct 3 16:49:14 2016 -0400
> >
> > receive-pack: quarantine objects until pre-receive accepts
>
> Thanks, I was able to reproduce easily with:
>
> git init --bare foo.git
> chown -R nobody foo.git
> git push foo.git HEAD
>
> Here's a series to fix it. The first patch addresses the deadlock. The
> rest try to improve the output on the client side. With v2.10.0, this
> case looks like:
>
> $ git push ~/tmp/foo.git HEAD
> Counting objects: 209837, done.
> Delta compression using up to 8 threads.
> Compressing objects: 100% (52180/52180), done.
> remote: fatal: Unable to create temporary file '/home/peff/tmp/foo.git/./objects/pack/tmp_pack_XXXXXX': Permission denied
> error: failed to push some refs to '/home/peff/tmp/foo.git'
I tested your jk/push-deadlock-regression-fix branch in my local clone,
your patches fix the problem for me. Thanks!
Horst
--
PGP-Key 0xD40E0E7A
^ permalink raw reply
* Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error
From: René Scharfe @ 2017-03-08 18:12 UTC (permalink / raw)
To: Johannes Schindelin, git
Cc: Junio C Hamano, Brandon Williams, Stefan Beller, Jeff King
In-Reply-To: <0c0abc667d9b8dff299aa61aeb29a7e9e7316b66.1488987786.git.johannes.schindelin@gmx.de>
Am 08.03.2017 um 16:43 schrieb Johannes Schindelin:
> In 4ac9006f832 (real_path: have callers use real_pathdup and
> strbuf_realpath, 2016-12-12), we changed the xstrdup(real_path())
> pattern to use real_pathdup() directly.
>
> The only problem with this change is that real_path() calls
> strbuf_realpath() with die_on_error = 1 while real_pathdup() calls it
> with die_on_error = 0. Meaning that in cases where real_path() causes
> Git to die() with an error message, real_pathdup() is silent and returns
> NULL instead.
>
> The callers, however, are ill-prepared for that change, as they expect
> the return value to be non-NULL.
>
> This patch fixes that by extending real_pathdup()'s signature to accept
> the die_on_error flag and simply pass it through to strbuf_realpath(),
> and then adjust all callers after a careful audit whether they would
> handle NULLs well.
>
> Note: this fix not only prevents NULL pointer accesses, but it also
> reintroduces the error messages that were lost with the change to
> real_pathdup().
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> abspath.c | 4 ++--
> builtin/init-db.c | 6 +++---
> cache.h | 2 +-
> dir.c | 4 ++--
> environment.c | 2 +-
> setup.c | 4 ++--
> submodule.c | 10 +++++-----
> t/t1501-work-tree.sh | 2 +-
> worktree.c | 2 +-
> 9 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2c..b02e068aa34 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
> return strbuf_realpath(&realpath, path, 0);
> }
>
> -char *real_pathdup(const char *path)
> +char *real_pathdup(const char *path, int die_on_error)
Adding a gentle variant (with the current implementation) and making
real_pathdup() die on error would be nicer, as it doesn't require
callers to pass magic flag values. Most cases use the dying variant,
so such a patch would have to touch less places:
---
abspath.c | 7 +++++++
cache.h | 1 +
setup.c | 2 +-
t/t1501-work-tree.sh | 2 +-
4 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/abspath.c b/abspath.c
index 2f0c26e0e2..f3fcff8b1b 100644
--- a/abspath.c
+++ b/abspath.c
@@ -217,6 +217,13 @@ const char *real_path_if_valid(const char *path)
char *real_pathdup(const char *path)
{
struct strbuf realpath = STRBUF_INIT;
+ strbuf_realpath(&realpath, path, 1);
+ return strbuf_detach(&realpath, NULL);
+}
+
+char *real_pathdup_gently(const char *path)
+{
+ struct strbuf realpath = STRBUF_INIT;
char *retval = NULL;
if (strbuf_realpath(&realpath, path, 0))
diff --git a/cache.h b/cache.h
index 80b6372cf7..9dfbce702e 100644
--- a/cache.h
+++ b/cache.h
@@ -1154,6 +1154,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
const char *real_path(const char *path);
const char *real_path_if_valid(const char *path);
char *real_pathdup(const char *path);
+char *real_pathdup_gently(const char *path);
const char *absolute_path(const char *path);
char *absolute_pathdup(const char *path);
const char *remove_leading_path(const char *in, const char *prefix);
diff --git a/setup.c b/setup.c
index f14cbcd338..398ea8a913 100644
--- a/setup.c
+++ b/setup.c
@@ -806,7 +806,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
/* Keep entry but do not canonicalize it */
return 1;
} else {
- char *real_path = real_pathdup(ceil);
+ char *real_path = real_pathdup_gently(ceil);
if (!real_path) {
return 0;
}
diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index 046d9b7909..b06210ec5e 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -423,7 +423,7 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '
)
'
-test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '
+test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '
(
GIT_WORK_TREE=/.invalid/work/tree &&
export GIT_WORK_TREE &&
--
2.12.0
^ permalink raw reply related
* BUG Report: git branch ignore --no-abbrev flag
From: Guillaume Wenzek @ 2017-03-08 18:14 UTC (permalink / raw)
To: git
Hi,
After updating to git 2.12.0 on Monday I noticed that the "git branch"
wasn't behaving as usual.
As of today `git branch -vv --no-abbrev` outputs short hashes instead
of long one (as requested by --no-abbrev)[1]
git branch -vv --no-abbrev
* (HEAD detached at 2.12.1) 1c69bf2 Add
recap-release since previous release messages didn't go out.
master eb70249
[origin/master] Fix: support parsing "git://" remote URI to Github URL
Expected output:
git branch --vv --no-abbrev
* (HEAD detached at 2.12.1)
1c69bf24be6de096d801435378be85a936ab0f29 Add recap-release since
previous release messages didn't go out.
master
eb70249e724f933255b4d8c7096092f2764942b4 [origin/master] Fix: support
parsing "git://" remote URI to Github URL
[1] https://git-scm.com/docs/git-branch#git-branch---no-abbrev
I don't have any relevant configuration set.
^ permalink raw reply
* Re: [PATCH v3 0/9] Fix the early config
From: Jeff King @ 2017-03-08 16:29 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <alpine.DEB.2.20.1703081701100.3767@virtualbox>
On Wed, Mar 08, 2017 at 05:18:46PM +0100, Johannes Schindelin wrote:
> On Wed, 8 Mar 2017, Jeff King wrote:
>
> > Another "non-gentle" thing I noticed here while looking at
> > another thread: the repository-format version check uses the config
> > parser, which will die() in certain circumstances. [...]
>
> Yes, that is part of the reason why I was not eager to add that check to
> discover_git_directory(). The config code is die()-happy.
>
> This is a much bigger problem, of course, and related to a constant gripe
> of mine: I *always* get the quoting wrong in my aliases. Always. And when
> I want to fix it, `git config -e` simply errors out because of an invalid
> config. Yes, Git, I know, that is the exact reason why I want to edit the
> config in the first place.
>
> I am certain you will agree that this is a different topic, therefore
> subject to a separate patch series.
I agree that in general it is a separate issue. Technically it could
cause a regression because with your series we are more eager to call
discover_git_directory() even when the command would not otherwise need
to even look at the current git directory.
But I don't think that is the case in practice. Your new function is
only called when we would try to read the config anyway. So it would
only affect cases which were previously so broken they did not properly
read the config (like running a command without RUN_SETUP from a
subdirectory of the working tree, which was _supposed_ to respect the
config but failed to do so).
So yeah, I'm happy to leave it for now.
-Peff
^ permalink raw reply
* Re: BUG Report: git branch ignore --no-abbrev flag
From: Junio C Hamano @ 2017-03-08 18:33 UTC (permalink / raw)
To: Guillaume Wenzek; +Cc: git, Karthik Nayak
In-Reply-To: <CAAvNd=ir1qNQVaKphdg51AfGnsNgTrfvW2L6cca3SHiZrWNgHA@mail.gmail.com>
Guillaume Wenzek <guillaume.wenzek@gmail.com> writes:
> After updating to git 2.12.0 on Monday I noticed that the "git branch"
> wasn't behaving as usual.
Are you sure you are trying 2.12? v2.12.0 and before should behave
the same way and honor --no-abbrev as far as I know.
On the other hand, 'master' has 93e8cd8b ("Merge branch
'kn/ref-filter-branch-list'", 2017-02-27), which seems to introduce
the regression.
Karthik?
^ permalink raw reply
* Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error
From: Brandon Williams @ 2017-03-08 18:38 UTC (permalink / raw)
To: René Scharfe
Cc: Johannes Schindelin, git, Junio C Hamano, Stefan Beller,
Jeff King
In-Reply-To: <81f1e30b-e0e1-d587-4a4b-4848beffd38c@web.de>
On 03/08, René Scharfe wrote:
> Am 08.03.2017 um 16:43 schrieb Johannes Schindelin:
> > In 4ac9006f832 (real_path: have callers use real_pathdup and
> > strbuf_realpath, 2016-12-12), we changed the xstrdup(real_path())
> > pattern to use real_pathdup() directly.
> >
> > The only problem with this change is that real_path() calls
> > strbuf_realpath() with die_on_error = 1 while real_pathdup() calls it
> > with die_on_error = 0. Meaning that in cases where real_path() causes
> > Git to die() with an error message, real_pathdup() is silent and returns
> > NULL instead.
> >
> > The callers, however, are ill-prepared for that change, as they expect
> > the return value to be non-NULL.
> >
> > This patch fixes that by extending real_pathdup()'s signature to accept
> > the die_on_error flag and simply pass it through to strbuf_realpath(),
> > and then adjust all callers after a careful audit whether they would
> > handle NULLs well.
> >
> > Note: this fix not only prevents NULL pointer accesses, but it also
> > reintroduces the error messages that were lost with the change to
> > real_pathdup().
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > abspath.c | 4 ++--
> > builtin/init-db.c | 6 +++---
> > cache.h | 2 +-
> > dir.c | 4 ++--
> > environment.c | 2 +-
> > setup.c | 4 ++--
> > submodule.c | 10 +++++-----
> > t/t1501-work-tree.sh | 2 +-
> > worktree.c | 2 +-
> > 9 files changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/abspath.c b/abspath.c
> > index 2f0c26e0e2c..b02e068aa34 100644
> > --- a/abspath.c
> > +++ b/abspath.c
> > @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
> > return strbuf_realpath(&realpath, path, 0);
> > }
> >
> > -char *real_pathdup(const char *path)
> > +char *real_pathdup(const char *path, int die_on_error)
>
> Adding a gentle variant (with the current implementation) and making
> real_pathdup() die on error would be nicer, as it doesn't require
> callers to pass magic flag values. Most cases use the dying variant,
> so such a patch would have to touch less places:
I agree with Junio and Rene that a gentle version would make the api
slightly nicer (and more consistant with some of the other api's we have
in git).
This is exactly what I should have done back when I originally made the
change. Sorry for missing this!
> ---
> abspath.c | 7 +++++++
> cache.h | 1 +
> setup.c | 2 +-
> t/t1501-work-tree.sh | 2 +-
> 4 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2..f3fcff8b1b 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -217,6 +217,13 @@ const char *real_path_if_valid(const char *path)
> char *real_pathdup(const char *path)
> {
> struct strbuf realpath = STRBUF_INIT;
> + strbuf_realpath(&realpath, path, 1);
> + return strbuf_detach(&realpath, NULL);
> +}
> +
> +char *real_pathdup_gently(const char *path)
> +{
> + struct strbuf realpath = STRBUF_INIT;
> char *retval = NULL;
>
> if (strbuf_realpath(&realpath, path, 0))
> diff --git a/cache.h b/cache.h
> index 80b6372cf7..9dfbce702e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1154,6 +1154,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
> const char *real_path(const char *path);
> const char *real_path_if_valid(const char *path);
> char *real_pathdup(const char *path);
> +char *real_pathdup_gently(const char *path);
> const char *absolute_path(const char *path);
> char *absolute_pathdup(const char *path);
> const char *remove_leading_path(const char *in, const char *prefix);
> diff --git a/setup.c b/setup.c
> index f14cbcd338..398ea8a913 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -806,7 +806,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
> /* Keep entry but do not canonicalize it */
> return 1;
> } else {
> - char *real_path = real_pathdup(ceil);
> + char *real_path = real_pathdup_gently(ceil);
> if (!real_path) {
> return 0;
> }
> diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
> index 046d9b7909..b06210ec5e 100755
> --- a/t/t1501-work-tree.sh
> +++ b/t/t1501-work-tree.sh
> @@ -423,7 +423,7 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '
> )
> '
>
> -test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '
> +test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '
> (
> GIT_WORK_TREE=/.invalid/work/tree &&
> export GIT_WORK_TREE &&
> --
> 2.12.0
>
--
Brandon Williams
^ permalink raw reply
* Re: Crash on MSYS2 with GIT_WORK_TREE
From: Brandon Williams @ 2017-03-08 18:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: valtron, git
In-Reply-To: <alpine.DEB.2.20.1703081254480.3767@virtualbox>
On 03/08, Johannes Schindelin wrote:
> I did take a quick glance, but did you have a look at the time of day I
> sent this patch? You do not want to trust my judgement after that.
Haha Yeah I did notice, and I trust your newer patch more than the one
you sent at 2am :)
>
> Another thing: may I ask you to delete the quoted parts of the mail that
> you are actually not responding to? Junio also often simply keeps the rest
> of the mail quoted, and I always have to scroll all the way to the end
> just to verify that nothing more has been said, which can be slightly
> annoying when you are tired. I do plan to read your mails in the future,
> so culling the quoted-yet-unanswered part would save me trouble.
Of course, I usually try to clear the parts of the mail I'm not
responding to...though there are times where I forget or am a bit lazy.
I'll definitely work on remembering to do that for the future!
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
From: Junio C Hamano @ 2017-03-08 18:47 UTC (permalink / raw)
To: Jeff Hostetler; +Cc: git, peff, markbt, benpeart, jonathantanmy, Jeff Hostetler
In-Reply-To: <1488994685-37403-3-git-send-email-jeffhost@microsoft.com>
Jeff Hostetler <jeffhost@microsoft.com> writes:
> From: Jeff Hostetler <git@jeffhostetler.com>
>
> Teach pack-objects to omit blobs from the generated packfile.
>
> When the --partial-by-size=n[kmg] argument is used, only blobs
> smaller than the requested size are included. When n is zero,
> no blobs are included.
Does this interact with a more traditional way of feeding output of
an external "rev-list --objects" to pack-objects via its standard
input, and if so, should it (and if not, shouldn't it)?
It is perfectly OK if the answer is "this applies only to the case
where we generate the list of objects with internal traversal." but
that needs to be documented and discussed in the proposed log
message.
> When the --partial-special argument is used, git special files,
> such as ".gitattributes" and ".gitignores" are included.
And not ."gitmodules"?
What happens when we later add ".gitsomethingelse"?
Do we have to worry about the case where the set of git "special
files" (can we have a better name for them please, by the way?)
understood by the sending side and the receiving end is different?
I have a feeling that a mode that makes anything whose name begins
with ".git" excempt from the size based cutoff may generally be
easier to handle.
I am not sure how "back-filling" of a resulting narrow clone would
safely be done and how this impacts "git fsck" at this point, but if
they are solved within this effort, that would be a very welcome
change.
Thanks.
^ permalink raw reply
* [PATCH 00/10] RFC Partial Clone and Fetch
From: git @ 2017-03-08 18:50 UTC (permalink / raw)
To: git; +Cc: jeffhost, peff, gitster, markbt, benpeart, jonathantanmy
From: Jeff Hostetler <jeffhost@microsoft.com>
[RFC] Partial Clone and Fetch
=============================
This is a WIP RFC for a partial clone and fetch feature wherein the client
can request that the server omit various blobs from the packfile during
clone and fetch. Clients can later request omitted blobs (either from a
modified upload-pack-like request to the server or via a completely
independent mechanism).
The purpose here is to reduce the size of packfile downloads and help
git scale to extremely large repos.
I use the term "partial" here to refer to a portion of one or more commits
and to avoid use of loaded terms like "sparse", "lazy", "narrow", and "skeleton".
The concept of a partial clone/fetch is independent of and can complement
the existing shallow-clone, refspec, and limited-ref filtering mechanisms
since these all filter at the DAG level whereas the work described here
works *within* the set of commits already chosen for download.
A. Requesting a Partial Clone/Fetch
===================================
Clone, fetch, and fetch-pack will accept one or more new "partial"
command line arguments as described below. The fetch-pack/upload-pack
protocol will be extended to include these new arguments. Upload-pack
and pack-objects will be updated accordingly. Pack-objects will filter
out the unwanted blobs as it is building the packfile. Rev-list and
index-pack will be updated to not complain when missing blobs are
detected in the received packfile.
[1] "--partial-by-size=<n>[kmg]"
Where <n> is a non-negative integer with an optional unit.
Request that only blobs smaller than this be included in the packfile.
The client might use this to implement an alternate LFS or ODB mechanism
for large blobs, such as suggested in:
https://public-inbox.org/git/20161130210420.15982-1-chriscool@tuxfamily.org/
A special case of size zero would omit all blobs and is similar to the
commits-and-trees-only feature described in:
https://public-inbox.org/git/20170113155253.1644-1-benpeart@microsoft.com/
[2] "--partial-special"
Request that special files, such as ".gitignore" and .gitattributes",
be included.
[3] *TODO* "--partial-by-profile=<sparse-checkout-path>"
Where <sparse-checkout-path> is verson-controlled file in the repository
(either present in the requested commit or the default HEAD on the server).
[I envision a ".gitsparse/<path>" hierarchy where teams can store
common sparse-checkout profiles. And then they can reference
them from their private ".git/info/sparse-checkout" files.]
Pack-objects will use this file and the sparse-checkout rules to only
include blobs in the packfile that would be needed to do the corresponding
sparse-checkout (and let the client avoid having to demand-load their
entire enlistment).
When multiple "partial" options are given, they are treated as a simple OR
giving the union of the blobs selected.
The patch series describes the changes to the fetch-pack/upload-pack
protocol:
Documentation/technical/pack-protocol.txt
Documentation/technical/protocol-capabilities.txt
B. Issues Backfilling Omitted Blobs
===================================
Ideally, if the client only does "--partial-by-profile" fetches, it
should not need to fetch individual missing blobs, but we have to allow
for it to handle the other commands and other unexpected issues.
There are 3 orthogonal concepts here: when, how and where?
[1] When:
(1a) a pre-command or hook to identify needed blobs and pre-fetch them
before allowing the actual command to start;
(1b) a dry-run mode for the command to likewise pre-fetch them; or
(1c) "fault" them in as necessary in read_object() while the command is
running and without any pre-fetch (either synchronously or asynchronously
and with/without a helper process).
Ideas for (1c) are being addressed in the following threads:
https://public-inbox.org/git/20170113155253.1644-1-benpeart@microsoft.com/
https://public-inbox.org/git/20170117184258.sd7h2hkv27w52gzt@sigill.intra.peff.net/
https://public-inbox.org/git/20161130210420.15982-1-chriscool@tuxfamily.org/
so I won't consider them here.
Ideas (1a) and (1b) have the advantage that they try to obtain all
required blobs before allowing an operation to start, so there is
less opportunity to leave the user in a weird state.
The best solution may be a combination of (1a) and (1b) and may depend
on the individual command. However, (1b) will further complicate the
source in the existing commands, so in some cases it may be simpler to
just take the ideas and implement stand-alone pre-commands.
For now, I'm going to limit this RFC to (1a).
[2] How:
(2a) augment the existing git protocols to include blob requests;
(2b) local external process (such as a database client or a local bulk
fetch daemon);
Ideas for (2b) are being addressed in the above threads, so I won't
consider them here.
So I'm going to limit this RFC to (2a).
[3] Where:
(3a) the same remote server used for the partial clone/fetch;
(3b) anywhere else, such as a proxy server or Azure or S3 blob store.
There's no reason that the client should be limited to going back to
the same server, but I'm not going to consider it here, so I'm going
to limit this RFC to (3a).
C. New Blob-Fetch Protocol (2a)
===============================
*TODO* A new pair of commands, such as fetch-blob-pack and upload-blob-pack,
will be created to let the client request a batch of blobs and receive a
packfile. A protocol similar to the fetch-pack/upload-pack will be spoken
between them. (This avoids complicating the existing protocol and the work
of enumerating the refs.) Upload-blob-pack will use pack-objects to build
the packfile.
It is also more efficient than requesting a single blob at a time using
the existing fetch-pack/upload-pack mechanism (with the various allow
unreachable options).
*TODO* The new request protocol will be defined in the patch series.
It will include: a list of the desired blob SHAs. Possibly also the commit
SHA, branch name, and pathname of each blob (or whatever is necessary to let
the server address the reachability concerns). Possibly also the last
known SHA for each blob to allow for deltafication in the packfile.
D. Pre-fetching Blobs (1a)
==========================
On the client side, one or more special commands will be created to assemble
the list of blobs needed for an operation and passed to fetch-blob-pack.
Checkout Example: After running a command like:
'clone --partial-by-size=1m --no-checkout'
and before doing an actual checkout, we need a command to essentially do:
(1) "ls-tree -r <tree-ish>",
(2) filter that by the sparse-checkout currently in effect,
(3) filter that for missing blobs,
(4) and pass the resulting list to fetch-blob-pack.
Afterwards, checkout should complete without faulting.
A new "git ls-partial <treeish>" command has been created to do
steps 1 thru 3 and print the resulting list of SHAs on stdout.
E. Unresolved Thoughts
======================
*TODO* The server should optionally return (in a side-band?) a list
of the blobs that it omitted from the packfile (and possibly the sizes
or sha1_object_info() data for them) during the fetch-pack/upload-pack
operation. This would allow the client to distinguish from invalid
SHAs and missing ones. Size information would allow the client to
maybe choose between various servers.
*TODO* The partial clone arguments should be recorded in ".git/info/"
so that subsequent fetch commands can inherit them and rev-list/index-pack
know to not complain by default.
*TODO* Update GC like rev-list to not complain when there are missing blobs.
*TODO* Extend ls-partial to include the "-m" and 3 tree-ish arguments
like read-tree, so we can pre-fetch for merges that may require file
merges (that may or may not be within our sparse-checkout).
*TODO* I also need to review the RFC that Mark Thomas submitted over
the weekend:
https://public-inbox.org/git/20170304191901.9622-1-markbt%40efaref.net/t
Jeff Hostetler (10):
pack-objects: eat CR in addition to LF after fgets.
pack-objects: add --partial-by-size=n --partial-special
pack-objects: test for --partial-by-size --partial-special
upload-pack: add partial (sparse) fetch
fetch-pack: add partial-by-size and partial-special
rev-list: add --allow-partial option to relax connectivity checks
index-pack: add --allow-partial option to relax blob existence checks
fetch: add partial-by-size and partial-special arguments
clone: add partial-by-size and partial-special arguments
ls-partial: created command to list missing blobs
Documentation/technical/pack-protocol.txt | 14 ++
Documentation/technical/protocol-capabilities.txt | 7 +
Makefile | 2 +
builtin.h | 1 +
builtin/clone.c | 26 ++
builtin/fetch-pack.c | 9 +
builtin/fetch.c | 26 +-
builtin/index-pack.c | 20 +-
builtin/ls-partial.c | 110 +++++++++
builtin/pack-objects.c | 64 ++++-
builtin/rev-list.c | 22 +-
connected.c | 3 +
connected.h | 3 +
fetch-pack.c | 17 ++
fetch-pack.h | 2 +
git.c | 1 +
partial-utils.c | 279 ++++++++++++++++++++++
partial-utils.h | 93 ++++++++
t/5316-pack-objects-partial.sh | 72 ++++++
transport.c | 8 +
transport.h | 8 +
upload-pack.c | 32 ++-
22 files changed, 813 insertions(+), 6 deletions(-)
create mode 100644 builtin/ls-partial.c
create mode 100644 partial-utils.c
create mode 100644 partial-utils.h
create mode 100644 t/5316-pack-objects-partial.sh
--
2.7.4
^ permalink raw reply
* Re: [PATCH] submodule--helper.c: remove duplicate code
From: Stefan Beller @ 2017-03-08 18:53 UTC (permalink / raw)
To: Valery Tolstov; +Cc: git@vger.kernel.org
In-Reply-To: <20170308174449.24266-1-me@vtolstov.org>
On Wed, Mar 8, 2017 at 9:44 AM, <me@vtolstov.org> wrote:
> From: Valery Tolstov <me@vtolstov.org>
>
> Remove code fragment from module_clone that duplicates functionality
> of connect_work_tree_and_git_dir in dir.c
>
> Signed-off-by: Valery Tolstov <me@vtolstov.org>
> ---
>>> I think we can reuse code from module_clone that writes .git link.
>>> Possibly this code fragment needs to be factored out from module_clone
>>
>> That fragment already exists, see dir.h:
>> connect_work_tree_and_git_dir(work_tree, git_dir);
>> Maybe another good microproject is to use that in module_clone.
>
> By suggestion of Stefan Beller I would like to make this micro
> improvement as my microproject for GSoc.
Thanks for looking into this code deduplication!
Well these two parts of the code are subtly different, though.
When applying this patch to origin/master (3bc53220cb2 is the
last time I fetched from Junio), then
$ make
$ cd t
$ prove --timer --jobs 25 ./t[0-8][0-9]*.sh
... lots of output...
Test Summary Report
-------------------
./t3600-rm.sh (Wstat: 256 Tests: 79 Failed: 24)
Failed tests: 39-40, 43-59, 61-65
Non-zero exit status: 1
./t4059-diff-submodule-not-initialized.sh (Wstat: 256 Tests: 8 Failed: 5)
Failed tests: 4-8
Non-zero exit status: 1
./t7001-mv.sh (Wstat: 256 Tests: 47 Failed: 7)
Failed tests: 39-45
Non-zero exit status: 1
./t7412-submodule-absorbgitdirs.sh (Wstat: 256 Tests: 11 Failed: 6)
Failed tests: 3-7, 9
Non-zero exit status: 1
./t7400-submodule-basic.sh (Wstat: 256 Tests: 90 Failed: 13)
Failed tests: 46-47, 49, 75, 79-87
Non-zero exit status: 1
./t7406-submodule-update.sh (Wstat: 256 Tests: 52 Failed: 14)
Failed tests: 5, 28-31, 33-34, 36-39, 43, 45-46
Non-zero exit status: 1
When then running one of them with debug output
(See t/README for the debug flags, I remember divx,
the video format as a sufficient set of flags to get enough debug output)
$ ./t7406-submodule-update.sh -d -i -v -x
...
++ cd foo
++ git submodule deinit -f sub
Cleared directory 'sub'
Submodule 'foo/sub' (/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/../rebasing) unregistered
for path 'sub'
++ git submodule update --init sub
+ test_eval_ret_=1
+ want_trace
+ test t = t
+ test t = t
+ set +x
error: last command exited with $?=1
not ok 5 - submodule update --init from and of subdirectory
#
# git init withsubs &&
# (cd withsubs &&
# mkdir foo &&
# git submodule add "$(pwd)/../rebasing" foo/sub &&
# (cd foo &&
# git submodule deinit -f sub &&
# git submodule update --init sub 2>../../actual2
# )
# ) &&
# test_i18ncmp expect2 actual2
#
$ cd trash\ directory.t7406-submodule-update/withsubs/foo/
$ git submodule deinit -f sub
Cleared directory 'sub'
Submodule 'foo/sub' (/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/../rebasing) unregistered
for path 'sub'
$ git submodule update --init sub
Submodule 'foo/sub' (/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/../rebasing) registered for
path 'sub'
fatal: could not get submodule directory for
'/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/foo/sub'
Failed to clone 'foo/sub'. Retry scheduled
fatal: could not get submodule directory for
'/usr/local/google/home/sbeller/OSS/git/t/trash
directory.t7406-submodule-update/withsubs/foo/sub'
Failed to clone 'foo/sub' a second time, aborting
So I think we're missing to create the directory? (Just guessing)
Maybe we need to have 2293f77a081
(connect_work_tree_and_git_dir: safely create leading directories,
part of origin/sb/checkout-recurse-submodules, also found at
https://public-inbox.org/git/20170306205919.9713-8-sbeller@google.com/ )
first before we can apply this patch.
Thanks,
Stefan
>
> builtin/submodule--helper.c | 22 +++-------------------
> 1 file changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 899dc334e..cda8a3bc1 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -579,7 +579,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
> const char *name = NULL, *url = NULL, *depth = NULL;
> int quiet = 0;
> int progress = 0;
> - FILE *submodule_dot_git;
> char *p, *path = NULL, *sm_gitdir;
> struct strbuf rel_path = STRBUF_INIT;
> struct strbuf sb = STRBUF_INIT;
> @@ -653,27 +652,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)
> strbuf_reset(&sb);
> }
>
> - /* Write a .git file in the submodule to redirect to the superproject. */
> - strbuf_addf(&sb, "%s/.git", path);
> - if (safe_create_leading_directories_const(sb.buf) < 0)
> - die(_("could not create leading directories of '%s'"), sb.buf);
> - submodule_dot_git = fopen(sb.buf, "w");
> - if (!submodule_dot_git)
> - die_errno(_("cannot open file '%s'"), sb.buf);
> -
> - fprintf_or_die(submodule_dot_git, "gitdir: %s\n",
> - relative_path(sm_gitdir, path, &rel_path));
> - if (fclose(submodule_dot_git))
> - die(_("could not close file %s"), sb.buf);
> - strbuf_reset(&sb);
> - strbuf_reset(&rel_path);
> -
> - /* Redirect the worktree of the submodule in the superproject's config */
> p = git_pathdup_submodule(path, "config");
> if (!p)
> die(_("could not get submodule directory for '%s'"), path);
> - git_config_set_in_file(p, "core.worktree",
> - relative_path(path, sm_gitdir, &rel_path));
> +
> + /* Connect module worktree and git dir */
> + connect_work_tree_and_git_dir(path, sm_gitdir);
>
> /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
> git_config_get_string("submodule.alternateLocation", &sm_alternate);
> --
> 2.12.0.190.g250ed7eaf
>
^ permalink raw reply
* Re: diff.ignoreSubmoudles config setting broken?
From: Stefan Beller @ 2017-03-08 19:04 UTC (permalink / raw)
To: Sebastian Schuberth, Jacob Keller
Cc: Jeff King, Git Mailing List, Jens Lehmann
In-Reply-To: <CAHGBnuPGPcWwbrZX_92XDJu47bpH=kj2PZ7yWHK=MRfZ_RHXrQ@mail.gmail.com>
On Wed, Mar 8, 2017 at 7:07 AM, Sebastian Schuberth
<sschuberth@gmail.com> wrote:
>
> + Jens
>
+ Jacob Keller, who touched submodule diff display code last.
(I am thinking of fd47ae6a, diff: teach diff to display submodule
difference with an inline diff, 2016-08-31), which is first release as
part of v2.11.0 (that would fit your observance)
^ permalink raw reply
* Re: [PATCH 06/10] rev-list: add --allow-partial option to relax connectivity checks
From: Junio C Hamano @ 2017-03-08 18:55 UTC (permalink / raw)
To: Jeff Hostetler; +Cc: git, peff, markbt, benpeart, jonathantanmy, Jeff Hostetler
In-Reply-To: <1488994685-37403-7-git-send-email-jeffhost@microsoft.com>
Jeff Hostetler <jeffhost@microsoft.com> writes:
> From: Jeff Hostetler <git@jeffhostetler.com>
>
> Teach rev-list to optionally not complain when there are missing
> blobs. This is for use following a partial clone or fetch when
> the server omitted certain blobs.
This makes it impossible to tell from objects missing by design
(because we did an --partial-by-size clone earlier, expecting we can
later fetch from elsewhere when necessary) and objects inaccessible
by accident (because you have a repository corruption), no?
Even though I do very much like the basic "high level" premise to
omit often useless large blobs that are buried deep in the history
we would not necessarily need from the initial cloning and
subsequent fetches, I find it somewhat disturbing that the code
"Assume"s that any missing blob is due to an previous partial clone.
Adding this option smells like telling the users that they are not
supposed to run "git fsck" because a partially cloned repository is
inherently a corrupt repository.
Can't we do a bit better? If we want to make the world safer again,
what additional complexity is required to allow us to tell the
"missing by design" and "corrupt repository" apart?
Thanks.
^ permalink raw reply
* [PATCHv3] rev-parse: add --show-superproject-working-tree
From: Stefan Beller @ 2017-03-08 19:20 UTC (permalink / raw)
To: szeder.dev, email, git, sandals, ville.skytta; +Cc: Stefan Beller
In-Reply-To: <xmqq60jk488v.fsf@gitster.mtv.corp.google.com>
In some situations it is useful to know if the given repository
is a submodule of another repository.
Add the flag --show-superproject-working-tree to git-rev-parse
to make it easy to find out if there is a superproject.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
> Looks more or less right but invoke "ls-files -z" and reading the \0
> delimited output would be easier;
done
> otherwise you would have to worry
> about c-unquoting the pathname when the submodule is bound at a path
> with funny character (like a double-quote) in it.
paths from "rev-parse" (like "--git-dir", "--show-toplevel", etc.) already
are excempt from quoting rules, apparently.
$ git rev-parse --show-toplevel
/usr/local/google/home/sbeller/OSS/git/t/trash directory.t1500-rev-parse
interdiff to v2:
#diff --git a/submodule.c b/submodule.c
#index 06473d3646..bb405653fd 100644
#--- a/submodule.c
#+++ b/submodule.c
#@@ -1543,7 +1543,8 @@ const char *get_superproject_working_tree(void)
# argv_array_pop(&cp.env_array);
#
# argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
#- "ls-files", "--stage", "--full-name", "--", subpath, NULL);
#+ "ls-files", "-z", "--stage", "--full-name", "--",
#+ subpath, NULL);
# strbuf_reset(&sb);
#
# cp.no_stdin = 1;
#@@ -1564,13 +1565,12 @@ const char *get_superproject_working_tree(void)
#
# /*
# * There is a superproject having this repo as a submodule.
#- * The format is <mode> SP <hash> SP <stage> TAB <full name> LF,
#- * First remove LF, then skip up to \t.
#+ * The format is <mode> SP <hash> SP <stage> TAB <full name> \0,
#+ * We're only interested in the name after the tab.
# */
#- strbuf_rtrim(&sb);
# super_sub = strchr(sb.buf, '\t') + 1;
#+ super_sub_len = sb.buf + sb.len - super_sub - 1;
#
#- super_sub_len = sb.buf + sb.len - super_sub;
# if (super_sub_len > cwd_len ||
# strcmp(&cwd[cwd_len - super_sub_len], super_sub))
# die (_("BUG: returned path string doesn't match cwd?"));
#@@ -1579,7 +1579,6 @@ const char *get_superproject_working_tree(void)
# super_wt[cwd_len - super_sub_len] = '\0';
#
# ret = real_path(super_wt);
#-
# free(super_wt);
# }
# strbuf_release(&sb);
Thanks,
Stefan
Documentation/git-rev-parse.txt | 5 +++
builtin/rev-parse.c | 7 ++++
submodule.c | 82 +++++++++++++++++++++++++++++++++++++++++
submodule.h | 8 ++++
t/t1500-rev-parse.sh | 14 +++++++
5 files changed, 116 insertions(+)
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 91c02b8c85..b841bad7c7 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -261,6 +261,11 @@ print a message to stderr and exit with nonzero status.
--show-toplevel::
Show the absolute path of the top-level directory.
+--show-superproject-working-tree
+ Show the absolute path of the top-level directory of
+ the superproject. A superproject is a repository that records
+ this repository as a submodule.
+
--shared-index-path::
Show the path to the shared index file in split index mode, or
empty if not in split-index mode.
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index e08677e559..2549643267 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -12,6 +12,7 @@
#include "diff.h"
#include "revision.h"
#include "split-index.h"
+#include "submodule.h"
#define DO_REVS 1
#define DO_NOREV 2
@@ -779,6 +780,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
puts(work_tree);
continue;
}
+ if (!strcmp(arg, "--show-superproject-working-tree")) {
+ const char *superproject = get_superproject_working_tree();
+ if (superproject)
+ puts(superproject);
+ continue;
+ }
if (!strcmp(arg, "--show-prefix")) {
if (prefix)
puts(prefix);
diff --git a/submodule.c b/submodule.c
index 3b98766a6b..bb405653fd 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1514,3 +1514,85 @@ void absorb_git_dir_into_superproject(const char *prefix,
strbuf_release(&sb);
}
}
+
+const char *get_superproject_working_tree(void)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf sb = STRBUF_INIT;
+ const char *one_up = real_path_if_valid("../");
+ const char *cwd = xgetcwd();
+ const char *ret = NULL;
+ const char *subpath;
+ int code;
+ ssize_t len;
+
+ if (!is_inside_work_tree())
+ /*
+ * FIXME:
+ * We might have a superproject, but it is harder
+ * to determine.
+ */
+ return NULL;
+
+ if (!one_up)
+ return NULL;
+
+ subpath = relative_path(cwd, one_up, &sb);
+
+ prepare_submodule_repo_env(&cp.env_array);
+ argv_array_pop(&cp.env_array);
+
+ argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
+ "ls-files", "-z", "--stage", "--full-name", "--",
+ subpath, NULL);
+ strbuf_reset(&sb);
+
+ cp.no_stdin = 1;
+ cp.no_stderr = 1;
+ cp.out = -1;
+ cp.git_cmd = 1;
+
+ if (start_command(&cp))
+ die(_("could not start ls-files in .."));
+
+ len = strbuf_read(&sb, cp.out, PATH_MAX);
+ close(cp.out);
+
+ if (starts_with(sb.buf, "160000")) {
+ int super_sub_len;
+ int cwd_len = strlen(cwd);
+ char *super_sub, *super_wt;
+
+ /*
+ * There is a superproject having this repo as a submodule.
+ * The format is <mode> SP <hash> SP <stage> TAB <full name> \0,
+ * We're only interested in the name after the tab.
+ */
+ super_sub = strchr(sb.buf, '\t') + 1;
+ super_sub_len = sb.buf + sb.len - super_sub - 1;
+
+ if (super_sub_len > cwd_len ||
+ strcmp(&cwd[cwd_len - super_sub_len], super_sub))
+ die (_("BUG: returned path string doesn't match cwd?"));
+
+ super_wt = xstrdup(cwd);
+ super_wt[cwd_len - super_sub_len] = '\0';
+
+ ret = real_path(super_wt);
+ free(super_wt);
+ }
+ strbuf_release(&sb);
+
+ code = finish_command(&cp);
+
+ if (code == 128)
+ /* '../' is not a git repository */
+ return NULL;
+ if (code == 0 && len == 0)
+ /* There is an unrelated git repository at '../' */
+ return NULL;
+ if (code)
+ die(_("ls-tree returned unexpected return code %d"), code);
+
+ return ret;
+}
diff --git a/submodule.h b/submodule.h
index 05ab674f06..c8a0c9cb29 100644
--- a/submodule.h
+++ b/submodule.h
@@ -93,4 +93,12 @@ extern void prepare_submodule_repo_env(struct argv_array *out);
extern void absorb_git_dir_into_superproject(const char *prefix,
const char *path,
unsigned flags);
+
+/*
+ * Return the absolute path of the working tree of the superproject, which this
+ * project is a submodule of. If this repository is not a submodule of
+ * another repository, return NULL.
+ */
+extern const char *get_superproject_working_tree(void);
+
#endif
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 9ed8b8ccba..03d3c7f6d6 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -116,4 +116,18 @@ test_expect_success 'git-path inside sub-dir' '
test_cmp expect actual
'
+test_expect_success 'showing the superproject correctly' '
+ git rev-parse --show-superproject-working-tree >out &&
+ test_must_be_empty out &&
+
+ test_create_repo super &&
+ test_commit -C super test_commit &&
+ test_create_repo sub &&
+ test_commit -C sub test_commit &&
+ git -C super submodule add ../sub dir/sub &&
+ echo $(pwd)/super >expect &&
+ git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
+ test_cmp expect out
+'
+
test_done
--
2.12.0.190.g6e60aba09d.dirty
^ permalink raw reply related
* Re: [PATCH] submodule--helper.c: remove duplicate code
From: Valery Tolstov @ 2017-03-08 19:59 UTC (permalink / raw)
To: sbeller; +Cc: git, me
In-Reply-To: <CAGZ79kYJ9he-jhnA35m6az-T5Z58efmKsUaBw--_KzdGJPZb-Q@mail.gmail.com>
> Maybe we need to have 2293f77a081
> (connect_work_tree_and_git_dir: safely create leading directories,
> part of origin/sb/checkout-recurse-submodules, also found at
> https://public-inbox.org/git/20170306205919.9713-8-sbeller@google.com/ )
> first before we can apply this patch.
Thank you for your detailed responses. Yes, we difenitely need this
patch first. All tests passed when I applied it.
Regards,
Valery Tolstov
^ permalink raw reply
* Re: [PATCH 06/10] rev-list: add --allow-partial option to relax connectivity checks
From: Jeff Hostetler @ 2017-03-08 20:10 UTC (permalink / raw)
To: Junio C Hamano, Jeff Hostetler; +Cc: git, peff, markbt, benpeart, jonathantanmy
In-Reply-To: <xmqqd1dr38f0.fsf@gitster.mtv.corp.google.com>
On 3/8/2017 1:55 PM, Junio C Hamano wrote:
> Jeff Hostetler <jeffhost@microsoft.com> writes:
>
>> From: Jeff Hostetler <git@jeffhostetler.com>
>>
>> Teach rev-list to optionally not complain when there are missing
>> blobs. This is for use following a partial clone or fetch when
>> the server omitted certain blobs.
>
> This makes it impossible to tell from objects missing by design
> (because we did an --partial-by-size clone earlier, expecting we can
> later fetch from elsewhere when necessary) and objects inaccessible
> by accident (because you have a repository corruption), no?
Right. It will effectively neuter several commands like
index-pack, gc, and fsck WRT missing blobs.
> Even though I do very much like the basic "high level" premise to
> omit often useless large blobs that are buried deep in the history
> we would not necessarily need from the initial cloning and
> subsequent fetches, I find it somewhat disturbing that the code
> "Assume"s that any missing blob is due to an previous partial clone.
> Adding this option smells like telling the users that they are not
> supposed to run "git fsck" because a partially cloned repository is
> inherently a corrupt repository.
>
> Can't we do a bit better? If we want to make the world safer again,
> what additional complexity is required to allow us to tell the
> "missing by design" and "corrupt repository" apart?
I'm open to suggestions here. It would be nice to extend the
fetch-pack/upload-pack protocol to return a list of the SHAa
(and maybe the sizes) of the omitted blobs, so that a partial
clone or fetch would still be able to be integrity checked.
Jeff
^ permalink raw reply
* [PATCH] branch & tag: Add a --no-contains option
From: Ævar Arnfjörð Bjarmason @ 2017-03-08 20:20 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Lars Hjemli, Jeff King, Christian Couder,
Ævar Arnfjörð Bjarmason
Change the branch & tag commands to have a --no-contains option in
addition to their longstanding --contains options.
The use-case I have for this is mainly to find the last-good rollout
tag given a known-bad <commit>. Right given a hypothetically bad
commit v2.10.1-3-gcf5c7253e0 now you can find that with this hacky
one-liner:
(./git tag -l 'v[0-9]*'; ./git tag -l 'v[0-9]*' --contains v2.10.1-3-gcf5c7253e0)|sort|uniq -c|grep -E '^ *1 '|awk '{print $2}'
But with the --no-contains option you can now get the exact same
output with:
./git tag -l 'v[0-9]*' --no-contains v2.10.1-3-gcf5c7253e0 | sort
Once I'd implemented this for "tag" it was easy enough to add it for
"branch". I haven't added it to "for-each-ref" but that would be
trivial if anyone cares, but that use-case would be even more obscure
than adding it to "branch", so I haven't bothered. The "describe"
command also has a --contains option, but its semantics are unrelated
to what tag/branch/for-each-ref use --contains for, and I don't see
how a --no-contains option for it would make any sense.
More notes about this patch:
* I'm not really happy with the "special attention" documentation
example in git-branch.txt, but it follows logically from the
description for --contains just above it which I think is overly
specific as well. IMO that entire NOTES section in git-branch.txt
could just be removed.
* I'm adding a --without option as an alias for --no-contains for
consistency with --with and --contains. Since we don't even
document --with anymore (or test it) perhaps we shouldn't be adding
--without.
* Where I'm changing existing documentation lines I'm mainly word
wrapping at 75 columns to be consistent with the existing style.
* Ditto the minor change to git-completion.bash.
* Perhaps we should just copy/paste commit_contains() into
commit_no_contains() and skip the ternary struct assignment. It's a
hot loop, but I have faith in modern compilers.
* All of the test changes I've made are just doing the inverse of the
existing --contains tests, with this --no-contains for both tag &
branch should be just as tested as the existing --contains option.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/git-branch.txt | 24 +++--
Documentation/git-tag.txt | 6 +-
builtin/branch.c | 4 +-
builtin/tag.c | 2 +
contrib/completion/git-completion.bash | 9 +-
parse-options.h | 4 +-
ref-filter.c | 17 ++--
ref-filter.h | 1 +
t/t3201-branch-contains.sh | 40 +++++++-
t/t7004-tag.sh | 163 ++++++++++++++++++++++++++++++++-
10 files changed, 245 insertions(+), 25 deletions(-)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 092f1bcf9f..316ec5b2d4 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,7 @@ SYNOPSIS
'git branch' [--color[=<when>] | --no-color] [-r | -a]
[--list] [-v [--abbrev=<length> | --no-abbrev]]
[--column[=<options>] | --no-column]
- [(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>]
+ [(--merged | --no-merged | --contains | --no-contains) [<commit>]] [--sort=<key>]
[--points-at <object>] [--format=<format>] [<pattern>...]
'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
@@ -35,11 +35,12 @@ as branch creation.
With `--contains`, shows only the branches that contain the named commit
(in other words, the branches whose tip commits are descendants of the
-named commit). With `--merged`, only branches merged into the named
-commit (i.e. the branches whose tip commits are reachable from the named
-commit) will be listed. With `--no-merged` only branches not merged into
-the named commit will be listed. If the <commit> argument is missing it
-defaults to `HEAD` (i.e. the tip of the current branch).
+named commit), `--no-contains` inverts it. With `--merged`, only branches
+merged into the named commit (i.e. the branches whose tip commits are
+reachable from the named commit) will be listed. With `--no-merged` only
+branches not merged into the named commit will be listed. If the <commit>
+argument is missing it defaults to `HEAD` (i.e. the tip of the current
+branch).
The command's second form creates a new branch head named <branchname>
which points to the current `HEAD`, or <start-point> if given.
@@ -213,6 +214,10 @@ start-point is either a local or remote-tracking branch.
Only list branches which contain the specified commit (HEAD
if not specified). Implies `--list`.
+--no-contains [<commit>]::
+ Only list branches which don't contain the specified commit
+ (HEAD if not specified). Implies `--list`.
+
--merged [<commit>]::
Only list branches whose tips are reachable from the
specified commit (HEAD if not specified). Implies `--list`.
@@ -296,13 +301,16 @@ If you are creating a branch that you want to checkout immediately, it is
easier to use the git checkout command with its `-b` option to create
a branch and check it out with a single command.
-The options `--contains`, `--merged` and `--no-merged` serve three related
-but different purposes:
+The options `--contains`, `--no-contains`, `--merged` and `--no-merged`
+serve three related but different purposes:
- `--contains <commit>` is used to find all branches which will need
special attention if <commit> were to be rebased or amended, since those
branches contain the specified <commit>.
+- `--no-contains <commit>` is used to find those branches which won't need
+ that special attention.
+
- `--merged` is used to find all branches which can be safely deleted,
since those branches are fully contained by HEAD.
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 525737a5d8..4938496194 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,7 @@ SYNOPSIS
'git tag' [-a | -s | -u <keyid>] [-f] [-m <msg> | -F <file>]
<tagname> [<commit> | <object>]
'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
+'git tag' [-n[<num>]] -l [--[no-]contains <commit>] [--points-at <object>]
[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
'git tag' -v [--format=<format>] <tagname>...
@@ -124,6 +124,10 @@ This option is only applicable when listing tags without annotation lines.
Only list tags which contain the specified commit (HEAD if not
specified).
+--no-contains [<commit>]::
+ Only list tags which don't contain the specified commit (HEAD if
+ not secified).
+
--points-at <object>::
Only list tags of the given object.
diff --git a/builtin/branch.c b/builtin/branch.c
index 94f7de7fa5..e8d534604c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -548,7 +548,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
OPT_SET_INT('r', "remotes", &filter.kind, N_("act on remote-tracking branches"),
FILTER_REFS_REMOTES),
OPT_CONTAINS(&filter.with_commit, N_("print only branches that contain the commit")),
+ OPT_NO_CONTAINS(&filter.no_commit, N_("print only branches that don't contain the commit")),
OPT_WITH(&filter.with_commit, N_("print only branches that contain the commit")),
+ OPT_WITHOUT(&filter.with_commit, N_("print only branches that don't contain the commit")),
OPT__ABBREV(&filter.abbrev),
OPT_GROUP(N_("Specific git-branch actions:")),
@@ -604,7 +606,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
list = 1;
- if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
+ if (filter.with_commit || filter.no_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
list = 1;
if (!!delete + !!rename + !!new_upstream +
diff --git a/builtin/tag.c b/builtin/tag.c
index ad29be6923..737a83028a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -424,7 +424,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_GROUP(N_("Tag listing options")),
OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
+ OPT_NO_CONTAINS(&filter.no_commit, N_("print only tags that don't contain the commit")),
OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
+ OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
OPT_MERGED(&filter, N_("print only tags that are merged")),
OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index fc32286a43..fa3da49478 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1093,9 +1093,9 @@ _git_branch ()
--*)
__gitcomp "
--color --no-color --verbose --abbrev= --no-abbrev
- --track --no-track --contains --merged --no-merged
- --set-upstream-to= --edit-description --list
- --unset-upstream --delete --move --remotes
+ --track --no-track --contains --no-contains --merged
+ --no-merged --set-upstream-to= --edit-description
+ --list --unset-upstream --delete --move --remotes
--column --no-column --sort= --points-at
"
;;
@@ -2862,7 +2862,8 @@ _git_tag ()
__gitcomp "
--list --delete --verify --annotate --message --file
--sign --cleanup --local-user --force --column --sort=
- --contains --points-at --merged --no-merged --create-reflog
+ --contains --no-contains --points-at --merged
+ --no-merged --create-reflog
"
;;
esac
diff --git a/parse-options.h b/parse-options.h
index dcd8a0926c..0eac90b510 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -258,7 +258,9 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);
PARSE_OPT_LASTARG_DEFAULT | flag, \
parse_opt_commits, (intptr_t) "HEAD" \
}
-#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, 0)
+#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, PARSE_OPT_NONEG)
+#define OPT_NO_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("no-contains", v, h, PARSE_OPT_NONEG)
#define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH("with", v, h, PARSE_OPT_HIDDEN)
+#define OPT_WITHOUT(v, h) _OPT_CONTAINS_OR_WITH("without", v, h, PARSE_OPT_HIDDEN)
#endif
diff --git a/ref-filter.c b/ref-filter.c
index 1ec0fb8391..6a7ca1cdac 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1571,11 +1571,12 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
return contains_test(candidate, want);
}
-static int commit_contains(struct ref_filter *filter, struct commit *commit)
+static int commit_contains(struct ref_filter *filter, struct commit *commit, const int with_commit)
{
+ struct commit_list *tmp = with_commit ? filter->with_commit : filter->no_commit;
if (filter->with_commit_tag_algo)
- return contains_tag_algo(commit, filter->with_commit);
- return is_descendant_of(commit, filter->with_commit);
+ return contains_tag_algo(commit, tmp);
+ return is_descendant_of(commit, tmp);
}
/*
@@ -1765,13 +1766,17 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
* obtain the commit using the 'oid' available and discard all
* non-commits early. The actual filtering is done later.
*/
- if (filter->merge_commit || filter->with_commit || filter->verbose) {
+ if (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {
commit = lookup_commit_reference_gently(oid->hash, 1);
if (!commit)
return 0;
- /* We perform the filtering for the '--contains' option */
+ /* We perform the filtering for the '--contains' option... */
if (filter->with_commit &&
- !commit_contains(filter, commit))
+ !commit_contains(filter, commit, 1))
+ return 0;
+ /* ...or for the `--no-contains' option */
+ if (filter->no_commit &&
+ commit_contains(filter, commit, 0))
return 0;
}
diff --git a/ref-filter.h b/ref-filter.h
index 154e24c405..af85eb4592 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -53,6 +53,7 @@ struct ref_filter {
const char **name_patterns;
struct sha1_array points_at;
struct commit_list *with_commit;
+ struct commit_list *no_commit;
enum {
REF_FILTER_MERGED_NONE = 0,
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 7f3ec47241..9fb79e66f0 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='branch --contains <commit>, --merged, and --no-merged'
+test_description='branch --contains <commit>, --no-contains <commit> --merged, and --no-merged'
. ./test-lib.sh
@@ -45,6 +45,22 @@ test_expect_success 'branch --contains master' '
'
+test_expect_success 'branch --no-contains=master' '
+
+ git branch --no-contains=master >actual &&
+ >expect &&
+ test_cmp expect actual
+
+'
+
+test_expect_success 'branch --no-contains master' '
+
+ git branch --no-contains master >actual &&
+ >expect &&
+ test_cmp expect actual
+
+'
+
test_expect_success 'branch --contains=side' '
git branch --contains=side >actual &&
@@ -55,6 +71,16 @@ test_expect_success 'branch --contains=side' '
'
+test_expect_success 'branch --no-contains=side' '
+
+ git branch --no-contains=side >actual &&
+ {
+ echo " master"
+ } >expect &&
+ test_cmp expect actual
+
+'
+
test_expect_success 'branch --contains with pattern implies --list' '
git branch --contains=master master >actual &&
@@ -65,6 +91,14 @@ test_expect_success 'branch --contains with pattern implies --list' '
'
+test_expect_success 'branch --no-contains with pattern implies --list' '
+
+ git branch --no-contains=master master >actual &&
+ >expect &&
+ test_cmp expect actual
+
+'
+
test_expect_success 'side: branch --merged' '
git branch --merged >actual &&
@@ -126,7 +160,9 @@ test_expect_success 'branch --no-merged with pattern implies --list' '
test_expect_success 'implicit --list conflicts with modification options' '
test_must_fail git branch --contains=master -d &&
- test_must_fail git branch --contains=master -m foo
+ test_must_fail git branch --contains=master -m foo &&
+ test_must_fail git branch --no-contains=master -d &&
+ test_must_fail git branch --no-contains=master -m foo
'
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f5..f01bcafea4 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1385,6 +1385,23 @@ test_expect_success 'checking that first commit is in all tags (relative)' "
test_cmp expected actual
"
+# All the --contains tests above, but with --no-contains
+test_expect_success 'checking that first commit is not listed in any tag with --no-contains (hash)' "
+ >expected &&
+ git tag -l --no-contains $hash1 v* >actual &&
+ test_cmp expected actual
+"
+
+test_expect_success 'checking that first commit is in all tags (tag)' "
+ git tag -l --no-contains v1.0 v* >actual &&
+ test_cmp expected actual
+"
+
+test_expect_success 'checking that first commit is in all tags (relative)' "
+ git tag -l --no-contains HEAD~2 v* >actual &&
+ test_cmp expected actual
+"
+
cat > expected <<EOF
v2.0
EOF
@@ -1394,6 +1411,17 @@ test_expect_success 'checking that second commit only has one tag' "
test_cmp expected actual
"
+cat > expected <<EOF
+v0.2.1
+v1.0
+v1.0.1
+v1.1.3
+EOF
+
+test_expect_success 'inverse of the last test, with --no-contains' "
+ git tag -l --no-contains $hash2 v* >actual &&
+ test_cmp expected actual
+"
cat > expected <<EOF
EOF
@@ -1403,6 +1431,19 @@ test_expect_success 'checking that third commit has no tags' "
test_cmp expected actual
"
+cat > expected <<EOF
+v0.2.1
+v1.0
+v1.0.1
+v1.1.3
+v2.0
+EOF
+
+test_expect_success 'conversely --no-contains on the third commit lists all tags' "
+ git tag -l --no-contains $hash3 v* >actual &&
+ test_cmp expected actual
+"
+
# how about a simple merge?
test_expect_success 'creating simple branch' '
@@ -1424,6 +1465,19 @@ test_expect_success 'checking that branch head only has one tag' "
test_cmp expected actual
"
+cat > expected <<EOF
+v0.2.1
+v1.0
+v1.0.1
+v1.1.3
+v2.0
+EOF
+
+test_expect_success 'checking that branch head with --no-contains lists all but one tag' "
+ git tag -l --no-contains $hash4 v* >actual &&
+ test_cmp expected actual
+"
+
test_expect_success 'merging original branch into this branch' '
git merge --strategy=ours master &&
git tag v4.0
@@ -1445,6 +1499,20 @@ v1.0.1
v1.1.3
v2.0
v3.0
+EOF
+
+test_expect_success 'checking that original branch head with --no-contains lists all but one tag now' "
+ git tag -l --no-contains $hash3 v* >actual &&
+ test_cmp expected actual
+"
+
+cat > expected <<EOF
+v0.2.1
+v1.0
+v1.0.1
+v1.1.3
+v2.0
+v3.0
v4.0
EOF
@@ -1453,6 +1521,12 @@ test_expect_success 'checking that initial commit is in all tags' "
test_cmp expected actual
"
+test_expect_success 'checking that initial commit is in all tags with --no-contains' "
+ >expected &&
+ git tag -l --no-contains $hash1 v* >actual &&
+ test_cmp expected actual
+"
+
# mixing modes and options:
test_expect_success 'mixing incompatibles modes and options is forbidden' '
@@ -1708,8 +1782,91 @@ run_with_limited_stack () {
test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
+# These are all the tags we've created above
+cat >expect.no-contains <<EOF
+a1
+aa1
+annotated-again-v4.0
+annotated-tag
+annotated-v4.0
+blank-annotated-tag
+blank-signed-tag
+blankfile-annotated-tag
+blankfile-signed-tag
+blanknonlfile-annotated-tag
+blanknonlfile-signed-tag
+blanks-annotated-tag
+blanks-signed-tag
+cba
+comment-annotated-tag
+comment-signed-tag
+commentfile-annotated-tag
+commentfile-signed-tag
+commentnonlfile-annotated-tag
+commentnonlfile-signed-tag
+comments-annotated-tag
+comments-signed-tag
+empty-annotated-tag
+empty-signed-tag
+emptyfile-annotated-tag
+emptyfile-signed-tag
+far-far-away
+file-annotated-tag
+file-signed-tag
+foo1.10
+foo1.10-alpha
+foo1.10-beta
+foo1.10-delta
+foo1.10-gamma
+foo1.10-unlisted-suffix
+foo1.3
+foo1.6
+foo1.6-rc1
+foo1.6-rc2
+foo1.7
+foo1.7-after1
+foo1.7-before1
+foo1.8
+foo1.8-foo-bar
+foo1.8-foo-baz
+foo1.9-pre1
+foo1.9-pre2
+foo1.9-prerelease1
+forcesignannotated-annotate
+forcesignannotated-disabled
+forcesignannotated-implied-sign
+forcesignannotated-lightweight
+forged-tag
+implied-annotate
+implied-sign
+non-annotated-tag
+reuse
+signed-tag
+stag-lines
+stag-one-line
+stag-zero-lines
+stdin-annotated-tag
+stdin-signed-tag
+t210
+t211
+tag-from-subdir
+tag-from-subdir-2
+tag-lines
+tag-one-line
+tag-signed-tag
+tag-zero-lines
+u-signed-tag
+v0.2.1
+v1.0
+v1.0.1
+v1.1.3
+v2.0
+v3.0
+v4.0
+EOF
+
# we require ulimit, this excludes Windows
-test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '
+test_expect_success ULIMIT_STACK_SIZE '--contains and --no-contains work in a deep repo' '
>expect &&
i=1 &&
while test $i -lt 8000
@@ -1725,7 +1882,9 @@ EOF"
git checkout master &&
git tag far-far-away HEAD^ &&
run_with_limited_stack git tag --contains HEAD >actual &&
- test_cmp expect actual
+ test_cmp expect actual &&
+ run_with_limited_stack git tag --no-contains HEAD >actual &&
+ test_cmp expect.no-contains actual
'
test_expect_success '--format should list tags as per format given' '
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
From: Jeff Hostetler @ 2017-03-08 20:21 UTC (permalink / raw)
To: Junio C Hamano, Jeff Hostetler; +Cc: git, peff, markbt, benpeart, jonathantanmy
In-Reply-To: <xmqqh93338s2.fsf@gitster.mtv.corp.google.com>
On 3/8/2017 1:47 PM, Junio C Hamano wrote:
> Jeff Hostetler <jeffhost@microsoft.com> writes:
>
>> From: Jeff Hostetler <git@jeffhostetler.com>
>>
>> Teach pack-objects to omit blobs from the generated packfile.
>>
>> When the --partial-by-size=n[kmg] argument is used, only blobs
>> smaller than the requested size are included. When n is zero,
>> no blobs are included.
>
> Does this interact with a more traditional way of feeding output of
> an external "rev-list --objects" to pack-objects via its standard
> input, and if so, should it (and if not, shouldn't it)?
>
> It is perfectly OK if the answer is "this applies only to the case
> where we generate the list of objects with internal traversal." but
> that needs to be documented and discussed in the proposed log
> message.
>
Let me study that and see. I'm still thinking thru ways and
options for doing the sparse-checkout like filtering.
>> When the --partial-special argument is used, git special files,
>> such as ".gitattributes" and ".gitignores" are included.
>
> And not ."gitmodules"?
>
> What happens when we later add ".gitsomethingelse"?
>
> Do we have to worry about the case where the set of git "special
> files" (can we have a better name for them please, by the way?)
> understood by the sending side and the receiving end is different?
>
> I have a feeling that a mode that makes anything whose name begins
> with ".git" excempt from the size based cutoff may generally be
> easier to handle.
I forgot about ".gitmodules". The more I think about it, maybe
we should always include them (or anything starting with ".git*")
and ignore the size, since they are important for correct behavior.
> I am not sure how "back-filling" of a resulting narrow clone would
> safely be done and how this impacts "git fsck" at this point, but if
> they are solved within this effort, that would be a very welcome
> change.
>
> Thanks.
>
^ permalink raw reply
* Re: [PATCH] submodule--helper.c: remove duplicate code
From: Stefan Beller @ 2017-03-08 20:25 UTC (permalink / raw)
To: Valery Tolstov; +Cc: git@vger.kernel.org
In-Reply-To: <20170308195916.7349-1-me@vtolstov.org>
On Wed, Mar 8, 2017 at 11:59 AM, Valery Tolstov <me@vtolstov.org> wrote:
>> Maybe we need to have 2293f77a081
>> (connect_work_tree_and_git_dir: safely create leading directories,
>> part of origin/sb/checkout-recurse-submodules, also found at
>> https://public-inbox.org/git/20170306205919.9713-8-sbeller@google.com/ )
>> first before we can apply this patch.
>
> Thank you for your detailed responses. Yes, we difenitely need this
> patch first. All tests passed when I applied it.
>
Thanks for testing!
Then the next step (as outlined by Documentation/SubmittingPatches)
is to figure out how to best present this to the mailing list; I think the best
way is to send out a patch series consisting of both of these 2 patches,
the "connect_work_tree_and_git_dir: safely create leading directories,"
first and then your deduplication patch.
When applied in that order, then git passes the test suite at all commits
(which is very nice when using e.g. git-bisect on git).
Thanks,
Stefan
^ permalink raw reply
* [PATCH 08/10] fetch: add partial-by-size and partial-special arguments
From: git @ 2017-03-08 18:50 UTC (permalink / raw)
To: git
Cc: jeffhost, peff, gitster, markbt, benpeart, jonathantanmy,
Jeff Hostetler
In-Reply-To: <1488999039-37631-1-git-send-email-git@jeffhostetler.com>
From: Jeff Hostetler <git@jeffhostetler.com>
Teach fetch to accept --partial-by-size=n and --partial-special
arguments and pass them to fetch-patch to request that the
server certain blobs from the generated packfile.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
builtin/fetch.c | 26 +++++++++++++++++++++++++-
connected.c | 3 +++
connected.h | 3 +++
3 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b5ad09d..3d47107 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -52,6 +52,8 @@ static const char *recurse_submodules_default;
static int shown_url = 0;
static int refmap_alloc, refmap_nr;
static const char **refmap_array;
+static const char *partial_by_size;
+static int partial_special;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -141,6 +143,11 @@ static struct option builtin_fetch_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+ OPT_STRING(0, "partial-by-size", &partial_by_size,
+ N_("size"),
+ N_("only include blobs smaller than this")),
+ OPT_BOOL(0, "partial-special", &partial_special,
+ N_("only include blobs for git special files")),
OPT_END()
};
@@ -731,6 +738,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
int want_status;
int summary_width = transport_summary_width(ref_map);
+ struct check_connected_options opt = CHECK_CONNECTED_INIT;
+
+ if (partial_by_size || partial_special)
+ opt.allow_partial = 1;
fp = fopen(filename, "a");
if (!fp)
@@ -742,7 +753,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
url = xstrdup("foreign");
rm = ref_map;
- if (check_connected(iterate_ref_map, &rm, NULL)) {
+ if (check_connected(iterate_ref_map, &rm, &opt)) {
rc = error(_("%s did not send all necessary objects\n"), url);
goto abort;
}
@@ -882,6 +893,9 @@ static int quickfetch(struct ref *ref_map)
struct ref *rm = ref_map;
struct check_connected_options opt = CHECK_CONNECTED_INIT;
+ if (partial_by_size || partial_special)
+ opt.allow_partial = 1;
+
/*
* If we are deepening a shallow clone we already have these
* objects reachable. Running rev-list here will return with
@@ -1020,6 +1034,10 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes");
if (update_shallow)
set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
+ if (partial_by_size)
+ set_option(transport, TRANS_OPT_PARTIAL_BY_SIZE, partial_by_size);
+ if (partial_special)
+ set_option(transport, TRANS_OPT_PARTIAL_SPECIAL, "yes");
return transport;
}
@@ -1314,6 +1332,12 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix,
builtin_fetch_options, builtin_fetch_usage, 0);
+ if (partial_by_size) {
+ unsigned long s;
+ if (!git_parse_ulong(partial_by_size, &s))
+ die(_("invalid partial-by-size value"));
+ }
+
if (deepen_relative) {
if (deepen_relative < 0)
die(_("Negative depth in --deepen is not supported"));
diff --git a/connected.c b/connected.c
index 136c2ac..b07cbb5 100644
--- a/connected.c
+++ b/connected.c
@@ -62,6 +62,9 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,
argv_array_pushf(&rev_list.args, "--progress=%s",
_("Checking connectivity"));
+ if (opt->allow_partial)
+ argv_array_push(&rev_list.args, "--allow-partial");
+
rev_list.git_cmd = 1;
rev_list.env = opt->env;
rev_list.in = -1;
diff --git a/connected.h b/connected.h
index 4ca325f..756259e 100644
--- a/connected.h
+++ b/connected.h
@@ -34,6 +34,9 @@ struct check_connected_options {
/* If non-zero, show progress as we traverse the objects. */
int progress;
+ /* A previous partial clone/fetch may have omitted some blobs. */
+ int allow_partial;
+
/*
* Insert these variables into the environment of the child process.
*/
--
2.7.4
^ permalink raw reply related
* [PATCH 03/10] pack-objects: test for --partial-by-size --partial-special
From: git @ 2017-03-08 18:50 UTC (permalink / raw)
To: git
Cc: jeffhost, peff, gitster, markbt, benpeart, jonathantanmy,
Jeff Hostetler
In-Reply-To: <1488999039-37631-1-git-send-email-git@jeffhostetler.com>
From: Jeff Hostetler <git@jeffhostetler.com>
Some simple tests for pack-objects with the new --partial-by-size
and --partial-special options.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
t/5316-pack-objects-partial.sh | 72 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
create mode 100644 t/5316-pack-objects-partial.sh
diff --git a/t/5316-pack-objects-partial.sh b/t/5316-pack-objects-partial.sh
new file mode 100644
index 0000000..352de34
--- /dev/null
+++ b/t/5316-pack-objects-partial.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='pack-object partial'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ perl -e "print \"a\" x 11;" > a &&
+ perl -e "print \"a\" x 1100;" > b &&
+ perl -e "print \"a\" x 1100000;" > c &&
+ echo "ignored" > .gitignore &&
+ git add a b c .gitignore &&
+ git commit -m test
+ '
+
+test_expect_success 'all blobs' '
+ git pack-objects --revs --thin --stdout >all.pack <<-EOF &&
+ master
+
+ EOF
+ git index-pack all.pack &&
+ test 4 = $(git verify-pack -v all.pack | grep blob | wc -l)
+ '
+
+test_expect_success 'no blobs' '
+ git pack-objects --revs --thin --stdout --partial-by-size=0 >none.pack <<-EOF &&
+ master
+
+ EOF
+ git index-pack none.pack &&
+ test 0 = $(git verify-pack -v none.pack | grep blob | wc -l)
+ '
+
+test_expect_success 'small blobs' '
+ git pack-objects --revs --thin --stdout --partial-by-size=1M >small.pack <<-EOF &&
+ master
+
+ EOF
+ git index-pack small.pack &&
+ test 3 = $(git verify-pack -v small.pack | grep blob | wc -l)
+ '
+
+test_expect_success 'tiny blobs' '
+ git pack-objects --revs --thin --stdout --partial-by-size=100 >tiny.pack <<-EOF &&
+ master
+
+ EOF
+ git index-pack tiny.pack &&
+ test 2 = $(git verify-pack -v tiny.pack | grep blob | wc -l)
+ '
+
+test_expect_success 'special' '
+ git pack-objects --revs --thin --stdout --partial-special >spec.pack <<-EOF &&
+ master
+
+ EOF
+ git index-pack spec.pack &&
+ test 1 = $(git verify-pack -v spec.pack | grep blob | wc -l)
+ '
+
+test_expect_success 'union' '
+ git pack-objects --revs --thin --stdout --partial-by-size=0 --partial-special >union.pack <<-EOF &&
+ master
+
+ EOF
+ git index-pack union.pack &&
+ test 1 = $(git verify-pack -v union.pack | grep blob | wc -l)
+ '
+
+test_done
+
+
--
2.7.4
^ permalink raw reply related
* [PATCH 09/10] clone: add partial-by-size and partial-special arguments
From: git @ 2017-03-08 18:50 UTC (permalink / raw)
To: git
Cc: jeffhost, peff, gitster, markbt, benpeart, jonathantanmy,
Jeff Hostetler
In-Reply-To: <1488999039-37631-1-git-send-email-git@jeffhostetler.com>
From: Jeff Hostetler <git@jeffhostetler.com>
Teach clone to accept --partial-by-size=n and --partial-special
arguments to request that the server omit certain blobs from
the generated packfile.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
builtin/clone.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/builtin/clone.c b/builtin/clone.c
index 3f63edb..e5a5904 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -56,6 +56,8 @@ static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
static int option_dissociate;
static int max_jobs = -1;
+static const char *partial_by_size;
+static int partial_special;
static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
@@ -112,6 +114,11 @@ static struct option builtin_clone_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+ OPT_STRING(0, "partial-by-size", &partial_by_size,
+ N_("size"),
+ N_("only include blobs smaller than this")),
+ OPT_BOOL(0, "partial-special", &partial_special,
+ N_("only include blobs for git special files")),
OPT_END()
};
@@ -625,6 +632,9 @@ static void update_remote_refs(const struct ref *refs,
if (check_connectivity) {
struct check_connected_options opt = CHECK_CONNECTED_INIT;
+ if (partial_by_size || partial_special)
+ opt.allow_partial = 1;
+
opt.transport = transport;
opt.progress = transport->progress;
@@ -1021,6 +1031,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
warning(_("--shallow-since is ignored in local clones; use file:// instead."));
if (option_not.nr)
warning(_("--shallow-exclude is ignored in local clones; use file:// instead."));
+ if (partial_by_size)
+ warning(_("--partial-by-size is ignored in local clones; use file:// instead."));
+ if (partial_special)
+ warning(_("--partial-special is ignored in local clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
@@ -1052,6 +1066,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
transport_set_option(transport, TRANS_OPT_UPLOADPACK,
option_upload_pack);
+ if (partial_by_size) {
+ transport_set_option(transport, TRANS_OPT_PARTIAL_BY_SIZE,
+ partial_by_size);
+ if (transport->smart_options)
+ transport->smart_options->partial_by_size = partial_by_size;
+ }
+ if (partial_special) {
+ transport_set_option(transport, TRANS_OPT_PARTIAL_SPECIAL, "yes");
+ if (transport->smart_options)
+ transport->smart_options->partial_special = 1;
+ }
+
if (transport->smart_options && !deepen)
transport->smart_options->check_self_contained_and_connected = 1;
--
2.7.4
^ permalink raw reply related
* [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
From: git @ 2017-03-08 18:50 UTC (permalink / raw)
To: git
Cc: jeffhost, peff, gitster, markbt, benpeart, jonathantanmy,
Jeff Hostetler
In-Reply-To: <1488999039-37631-1-git-send-email-git@jeffhostetler.com>
From: Jeff Hostetler <git@jeffhostetler.com>
Teach pack-objects to omit blobs from the generated packfile.
When the --partial-by-size=n[kmg] argument is used, only blobs
smaller than the requested size are included. When n is zero,
no blobs are included.
When the --partial-special argument is used, git special files,
such as ".gitattributes" and ".gitignores" are included.
When both are given, the union of two are included.
This is intended to be used in a partial clone or fetch.
(This has also been called sparse- or lazy-clone.)
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
builtin/pack-objects.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 61 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 7e052bb..2df2f49 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -77,6 +77,10 @@ static unsigned long cache_max_small_delta_size = 1000;
static unsigned long window_memory_limit = 0;
+static signed long partial_by_size = -1;
+static int partial_special = 0;
+static struct trace_key trace_partial = TRACE_KEY_INIT(PARTIAL);
+
/*
* stats
*/
@@ -2532,6 +2536,54 @@ static void show_object(struct object *obj, const char *name, void *data)
obj->flags |= OBJECT_ADDED;
}
+/*
+ * If ANY --partial-* option was given, we want to OMIT all
+ * blobs UNLESS they match one of our patterns. We treat
+ * the options as OR's so that we get the resulting UNION.
+ */
+static void show_object_partial(struct object *obj, const char *name, void *data)
+{
+ unsigned long s = 0;
+
+ if (obj->type != OBJ_BLOB)
+ goto include_it;
+
+ /*
+ * When (partial_by_size == 0), we want to OMIT all blobs.
+ * When (partial_by_size > 0), we want blobs smaller than that.
+ */
+ if (partial_by_size > 0) {
+ enum object_type t = sha1_object_info(obj->oid.hash, &s);
+ assert(t == OBJ_BLOB);
+ if (s < partial_by_size)
+ goto include_it;
+ }
+
+ /*
+ * When (partial_special), we want the .git* special files.
+ */
+ if (partial_special) {
+ if (strcmp(name, GITATTRIBUTES_FILE) == 0 ||
+ strcmp(name, ".gitignore") == 0)
+ goto include_it;
+ else {
+ const char *last_slash = strrchr(name, '/');
+ if (last_slash)
+ if (strcmp(last_slash+1, GITATTRIBUTES_FILE) == 0 ||
+ strcmp(last_slash+1, ".gitignore") == 0)
+ goto include_it;
+ }
+ }
+
+ trace_printf_key(
+ &trace_partial, "omitting blob '%s' %"PRIuMAX" '%s'\n",
+ oid_to_hex(&obj->oid), (uintmax_t)s, name);
+ return;
+
+include_it:
+ show_object(obj, name, data);
+}
+
static void show_edge(struct commit *commit)
{
add_preferred_base(commit->object.oid.hash);
@@ -2794,7 +2846,11 @@ static void get_object_list(int ac, const char **av)
if (prepare_revision_walk(&revs))
die("revision walk setup failed");
mark_edges_uninteresting(&revs, show_edge);
- traverse_commit_list(&revs, show_commit, show_object, NULL);
+
+ if (partial_by_size >= 0 || partial_special)
+ traverse_commit_list(&revs, show_commit, show_object_partial, NULL);
+ else
+ traverse_commit_list(&revs, show_commit, show_object, NULL);
if (unpack_unreachable_expiration) {
revs.ignore_missing_links = 1;
@@ -2930,6 +2986,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
N_("use a bitmap index if available to speed up counting objects")),
OPT_BOOL(0, "write-bitmap-index", &write_bitmap_index,
N_("write a bitmap index together with the pack index")),
+ OPT_MAGNITUDE(0, "partial-by-size", (unsigned long *)&partial_by_size,
+ N_("only include blobs smaller than size in result")),
+ OPT_BOOL(0, "partial-special", &partial_special,
+ N_("only include blobs for git special files")),
OPT_END(),
};
--
2.7.4
^ permalink raw reply related
* [PATCH 01/10] pack-objects: eat CR in addition to LF after fgets.
From: git @ 2017-03-08 18:50 UTC (permalink / raw)
To: git
Cc: jeffhost, peff, gitster, markbt, benpeart, jonathantanmy,
Jeff Hostetler
In-Reply-To: <1488999039-37631-1-git-send-email-git@jeffhostetler.com>
From: Jeff Hostetler <git@jeffhostetler.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
builtin/pack-objects.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f294dcf..7e052bb 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2764,6 +2764,8 @@ static void get_object_list(int ac, const char **av)
int len = strlen(line);
if (len && line[len - 1] == '\n')
line[--len] = 0;
+ if (len && line[len - 1] == '\r')
+ line[--len] = 0;
if (!len)
break;
if (*line == '-') {
--
2.7.4
^ permalink raw reply related
* [PATCH 07/10] index-pack: add --allow-partial option to relax blob existence checks
From: git @ 2017-03-08 18:50 UTC (permalink / raw)
To: git
Cc: jeffhost, peff, gitster, markbt, benpeart, jonathantanmy,
Jeff Hostetler
In-Reply-To: <1488999039-37631-1-git-send-email-git@jeffhostetler.com>
From: Jeff Hostetler <git@jeffhostetler.com>
Teach index-pack to optionally not complain when there are missing
blobs. This is for use following a partial clone or fetch when
the server omitted certain blobs.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
builtin/index-pack.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f4b87c6..8f99408 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -13,7 +13,7 @@
#include "thread-utils.h"
static const char index_pack_usage[] =
-"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
+"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--allow-partial] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
struct object_entry {
struct pack_idx_entry idx;
@@ -81,6 +81,9 @@ static int show_resolving_progress;
static int show_stat;
static int check_self_contained_and_connected;
+static int allow_partial;
+static struct trace_key trace_partial = TRACE_KEY_INIT(PARTIAL);
+
static struct progress *progress;
/* We always read in 4kB chunks. */
@@ -220,9 +223,18 @@ static unsigned check_object(struct object *obj)
if (!(obj->flags & FLAG_CHECKED)) {
unsigned long size;
int type = sha1_object_info(obj->oid.hash, &size);
- if (type <= 0)
+ if (type <= 0) {
+ if (allow_partial > 0 && obj->type == OBJ_BLOB) {
+ /* Assume a previous partial clone/fetch omitted it. */
+ trace_printf_key(
+ &trace_partial, "omitted blob '%s'\n",
+ oid_to_hex(&obj->oid));
+ obj->flags |= FLAG_CHECKED;
+ return 0;
+ }
die(_("did not receive expected object %s"),
oid_to_hex(&obj->oid));
+ }
if (type != obj->type)
die(_("object %s: expected type %s, found %s"),
oid_to_hex(&obj->oid),
@@ -1718,6 +1730,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
die(_("bad %s"), arg);
} else if (skip_prefix(arg, "--max-input-size=", &arg)) {
max_input_size = strtoumax(arg, NULL, 10);
+ } else if (!strcmp(arg, "--allow-partial")) {
+ allow_partial = 1;
+ } else if (!strcmp(arg, "--no-allow-partial")) {
+ allow_partial = 0;
} else
usage(index_pack_usage);
continue;
--
2.7.4
^ 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