* Re: [PATCH v3 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question
From: Heiko Voigt @ 2016-11-16 14:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, git@vger.kernel.org, Jeff King, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqqtwb8s2m8.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 15, 2016 at 04:13:51PM -0800, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> >> "We do not know" ...
> >
> > ... because there is no way to check for us as we don't have the
> > submodule commits.
> >
> > " We do consider it safe as no one in their sane mind would
> > have changed the submodule pointers without having the
> > submodule around. If a user did however change the submodules
> > without having the submodule commits around, this indicates an
> > expert who knows what they were doing."
>
> I didn't think it through myself to arrive at such a conclusion, but
> to me the above sounds like a sensible reasoning [*1*].
I think you have a point here. If I rephrase it like this: "We do
consider it safe as no one in their sane mind *could* have changed the
submodule pointers without having the submodule around..."
Since its actually hard to create such a situation without the submodule
commit around I agree here.
> *1* My version was more like "we do not know if they would get into
> a situation where they do not have enough submodule commits if
> we pushed our superproject, but more importantly, we DO KNOW
> that it would not help an iota if we pushed our submodule to
> them, so there is no point stopping the push of superproject
> saying 'no, no, no, you must push the submodule first'".
Yes saying that would be wrong. I was rather suggesting that we tell the
user that we could not find the submodule commits to and that if he
wants to proceed he should either pass --recurse-submodules=no or
initialize the submodule.
But I think the above reasoning obsoletes my suggestion. I would adjust
the comment accordingly but still keep the patch so we have
documentation that this behavior is on purpose.
Cheers Heiko
^ permalink raw reply
* Re: [RFH] limiting ref advertisements
From: Duy Nguyen @ 2016-11-16 13:42 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161114212122.rkgeoh4rj5cxdadf@sigill.intra.peff.net>
On Tue, Nov 15, 2016 at 4:21 AM, Jeff King <peff@peff.net> wrote:
> Thanks for responding to this.
Glad to help (or more precisely annoy you somewhat :D)
> I've been meaning to get back to it with
> some code experiments, but they keep getting bumped down in priority. So
> let me at least outline some of my thoughts, without code. :)
>
> I was hoping to avoid right-anchoring because it's expensive to find all
> of the right-anchored cases (assuming that ref storage is generally
> hierarchical, which it is now and probably will be for future backends).
Urgh.. I completely forgot about future refs backends. Yeah this would
kick both wildmatch and right-anchoring out of the window.
For the record I almost suggested BPF as well (to keep the server side
simple, but at the super high cost of client side). That would also go
out of the window the same way wildmatch and right-anchoring does.
> But remember that these are "early capabilities", before the server
> has spoken at all. So the client doesn't know if we can handle v2.
> So we have to send _both_ (and v2-aware servers can ignore the v1).
>
> advertise-lookup-v1=master
> advertise-lookup-v2=master
>
> But that's not quite enough. A v1 server won't look in refs/notes
> at all. So we have to say that, too:
>
> advertise-lookup-v1=refs/notes/master
>
> And of course the v1 server has no idea that this isn't necessary
> if we already found refs/heads/master.
We discussed a bit about upgrading upload-pack version 1 to 2 in more
than one session: the first fetch just does v1 as normal, the server
returns v1 response to but also advertises that v2 is supported. The
client keeps this info and skips v1 and tries v2 right away in the
following fetches, falling back to v1 (new fetch session) if v2 is
unsupported. Can it work the same way here too?
I'm in favor of this option 2 (without trying to be absolute backward
compatible with older lookup versions) since it allows us to optimize
for common case and experiment a bit. Once we know better we can make
the next version that hopefully suits everybody.
> So I think you really do need the client to be able to say "also
> look at this pattern".
What about the order of patterns? Does it matter "this pattern" is in
the middle or the end of the pattern list? I suppose not, just
checking...
But does this call for the ability to remove a pattern from the
pattern list as well, as a way to narrow down the search scope and
avoid sending unwanted refs?
> Of course we do still want left-anchoring, too. Wildcards like
> "refs/heads/*" are always left-anchored. So I think we'd have two types,
> and a full request for
>
> git fetch origin +refs/heads/*:refs/remotes/origin/* master:foo
>
> would look like:
>
> (1) advertise-pattern-v1
> (2) advertise-pattern=refs/notes/%s
> (3) advertise-prefix=refs/heads
> (4) advertise-lookup=master
>
> where the lines mean:
>
> 1. Use the standard v1 patterns (we could spell them out, but this
> just saves bandwidth. In fact, it could just be implicit that v1
> patterns are included, and we could skip this line).
>
> 2. This is for our fictional future version where the client knows
> added refs/notes/* to its DWIM but the server hasn't yet.
>
> 3. Give me all of refs/heads/*
>
> 4. Look up "master" using the advertise patterns and give me the first
> one you find.
Well.. it sounds good to me. But I would not trust myself on refs matters :D
> So given that we can omit (1), and that (2) is just an example for the
> future, it could look like:
>
> advertise-prefix=refs/heads
> advertise-lookup=master
>
> which is pretty reasonable. It's not _completely_ bulletproof in terms
> of backwards compatibility. The "v1" thing means the client can't insert
> a new pattern in the middle (remember they're ordered by priority).
OK so pattern order probably matters...
> So
> maybe it is better to spell them all out (one thing that makes me
> hesitate is that these will probably end up as URL parameters for the
> HTTP version, which means our URL can start to get a little long).
>
> Anyway. That's the direction I'm thinking. I haven't written the code
> yet. The trickiest thing will probably be that the server would want to
> avoid advertising the same ref twice via two mechanisms (or perhaps the
> client just be tolerant of duplicates; that relieves the server of any
> duplicate-storage requirements).
Thanks for sharing.
--
Duy
^ permalink raw reply
* Re: [PATCH] worktree: fix a sparse 'Using plain integer as NULL pointer' warning
From: Duy Nguyen @ 2016-11-16 13:17 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <5b7d7d0b-8a6c-d516-4eb9-4e4ea13dce73@ramsayjones.plus.com>
On Wed, Nov 16, 2016 at 3:28 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Duy,
>
> If you need to re-roll your 'nd/worktree-move' branch, could you
> please squash this into the relevant patch [commit c49e92f5c
> ("worktree move: refuse to move worktrees with submodules", 12-11-2016)].
Will do, thanks (and it's also "thanks" for your other similar emails,
I just don't want to send a mail with just 'thanks' that adds nothing
else).
> Also, one of the new tests introduced by commit 31a8f3066 ("worktree move:
> new command", 12-11-2016), fails for me, thus:
>
> $ ./t2028-worktree-move.sh -i -v
> ...
> --- expected 2016-11-15 20:22:50.647241458 +0000
> +++ actual 2016-11-15 20:22:50.647241458 +0000
> @@ -1,3 +1,3 @@
> worktree /home/ramsay/git/t/trash directory.t2028-worktree-move
> -worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
> worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/elsewhere
> +worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
> not ok 12 - move worktree
> #
> # git worktree move source destination &&
> # test_path_is_missing source &&
> # git worktree list --porcelain | grep "^worktree" >actual &&
> # cat <<-EOF >expected &&
> # worktree $TRASH_DIRECTORY
> # worktree $TRASH_DIRECTORY/destination
> # worktree $TRASH_DIRECTORY/elsewhere
> # EOF
> # test_cmp expected actual &&
> # git -C destination log --format=%s >actual2 &&
> # echo init >expected2 &&
> # test_cmp expected2 actual2
> #
> $
>
> Is there an expectation that the submodules will be listed in
> any particular order by 'git worktree list --porcelain' ?
I just sent a patch [1] to fix this before reading this mail. The
order so far has been determined by readdir() which is not great.
[1] https://public-inbox.org/git/CACsJy8DOT_4N_48UaoYK61G_8JUaXbEs7N=n24CH2q1GN=++5g@mail.gmail.com/T/#mfcf797219a1a143ed2ac45198015f19e82c70db2
--
Duy
^ permalink raw reply
* Re: Git status takes too long- How to improve the performance of git
From: Fredrik Gustafsson @ 2016-11-16 13:21 UTC (permalink / raw)
To: Renuka Pampana; +Cc: Fredrik Gustafsson, git
In-Reply-To: <CAEAva_1JAu+kWmk3MZDFK=4CgQB5M+JN8FwzMVr6zKgXTAhdXw@mail.gmail.com>
On Wed, Nov 16, 2016 at 05:13:57PM +0530, Renuka Pampana wrote:
> > On Tue, Nov 15, 2016 at 02:33:12AM -0700, ravalika wrote:
> > > It is an centralized server and git status takes too long
> >
> > A centralized server? How? git is designed to be runned locally. If
> > you're running git on a network file system, the performance will
> > suffer. Could you elaborate on how your environment is setup?
> >
> >
> We have setup main git repository in remote location on Linux server
> And created a git repository in local Linux server, as a reference for the
> remote git repository,
> And update the local git repository for every 15 min in local server
>
> Users will be able to access the local git repository through NFS
And each user will have their own copy of the repository locally on
their machine? That is having done a git clone?
>
> All users will clone the git repository from remote project url by using
> local git repo as reference
>
> For example : git clone --reference ~/gitcaches/reference user@drupal
> :/home/project/drupal.git
>
> All the users have ssh credentials for the remote server
Why are you using --reference for a 8.9MB big clone?
>
>
> What is the best way to implement remote git repo and able to access the
> git repo from other location, without any performance glitches?
> Users should be able to access git repo from different servers and from
> different locations.
The best way is to have it locally cloned. Yes the initial clone will be
expensive but operations after that will be fairly smooth. You do not(!)
want to execute git on one machine and having the repository beeing on
an other machine (for example via a network file system, except git
clone, git fetch, git push, etc.).
> >
> > > How to improve the performance of git status
> > >
> > > Git repo details:
> > >
> > > Size of the .git folder is 8.9MB
> > > Number of commits approx 53838 (git rev-list HEAD --count)
> > > Number of branches - 330
> > > Number of files - 63883
> > > Working tree clone size is 4.3GB
> >
> > .git folder of 8.9 MEGABYTE and working tree of 4.3 GIGABYTE? Is this a
> > typo?
> >
> > All git related information is stored in .git directory of the working
> directory
> It is 8.9M
> And size of the local workspace is 4.3G
Can you please elaborate on this? How can you store 8.9 MB of data that
will result in a 4.3 G workspace?
--
Fredrik Gustafsson
phone: +46 733-608274
e-mail: iveqy@iveqy.com
website: http://www.iveqy.com
^ permalink raw reply
* Re: [PATCH 00/11] git worktree (re)move
From: Duy Nguyen @ 2016-11-16 13:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <20161116130533.GA5393@ash>
On Wed, Nov 16, 2016 at 8:05 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> diff --git a/worktree.c b/worktree.c
> index f7869f8..fe92d6f 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -173,6 +173,13 @@ static void mark_current_worktree(struct worktree **worktrees)
> free(git_dir);
> }
>
> +static int compare_worktree(const void *a_, const void *b_)
> +{
> + const struct worktree *const *a = a_;
> + const struct worktree *const *b = b_;
> + return fspathcmp((*a)->path, (*b)->path);
> +}
> +
> struct worktree **get_worktrees(void)
> {
> struct worktree **list = NULL;
> @@ -205,6 +212,11 @@ struct worktree **get_worktrees(void)
> ALLOC_GROW(list, counter + 1, alloc);
> list[counter] = NULL;
>
> + /*
> + * don't sort the first item (main worktree), which will
> + * always be the first
> + */
Urgh.. I should review my patches more carefully before sending out :(
The main worktree could be missing (failing to parse HEAD) so I need a
better trick than simply assuming the first item is the main worktree
here. Tests did not catch this, naturally..
> + qsort(list + 1, counter - 1, sizeof(*list), compare_worktree);
> mark_current_worktree(list);
> return list;
> }
--
Duy
^ permalink raw reply
* Re: [PATCH 00/11] git worktree (re)move
From: Duy Nguyen @ 2016-11-16 13:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa8d4yts7.fsf@gitster.mtv.corp.google.com>
On Sat, Nov 12, 2016 at 06:53:44PM -0800, Junio C Hamano wrote:
> not ok 12 - move worktree
> #
> # git worktree move source destination &&
> # test_path_is_missing source &&
> # git worktree list --porcelain | grep "^worktree" >actual &&
> # cat <<-EOF >expected &&
> # worktree $TRASH_DIRECTORY
> # worktree $TRASH_DIRECTORY/destination
> # worktree $TRASH_DIRECTORY/elsewhere
> # EOF
> # test_cmp expected actual &&
> # git -C destination log --format=%s >actual2 &&
> # echo init >expected2 &&
> # test_cmp expected2 actual2
I think I've seen this (i.e. 'expected' and 'actual' differ only in
the order of items) once after a rebase and ignored it, assuming
something was changed during the rebase that caused this.
The following patch should fix it if that's the same thing you saw. I
could pile it on worktree-move series, or you can make it a separate
one-patch series. What's your preference?
-- 8< --
Subject: [PATCH] worktree list: keep the list sorted
It makes it easier to write tests for. But it should also be good for
the user since locating a worktree by eye would be easier once they
notice this.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
worktree.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/worktree.c b/worktree.c
index f7869f8..fe92d6f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -173,6 +173,13 @@ static void mark_current_worktree(struct worktree **worktrees)
free(git_dir);
}
+static int compare_worktree(const void *a_, const void *b_)
+{
+ const struct worktree *const *a = a_;
+ const struct worktree *const *b = b_;
+ return fspathcmp((*a)->path, (*b)->path);
+}
+
struct worktree **get_worktrees(void)
{
struct worktree **list = NULL;
@@ -205,6 +212,11 @@ struct worktree **get_worktrees(void)
ALLOC_GROW(list, counter + 1, alloc);
list[counter] = NULL;
+ /*
+ * don't sort the first item (main worktree), which will
+ * always be the first
+ */
+ qsort(list + 1, counter - 1, sizeof(*list), compare_worktree);
mark_current_worktree(list);
return list;
}
--
2.8.2.524.g6ff3d78
-- 8< --
^ permalink raw reply related
* Re: RFC: Enable delayed responses to Git clean/smudge filter requests
From: Lars Schneider @ 2016-11-16 9:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, git
In-Reply-To: <xmqqvavotych.fsf@gitster.mtv.corp.google.com>
On 15 Nov 2016, at 19:03, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>>> The filter itself would need to be aware of parallelism
>>> if it lives for multiple objects, right?
>>
>> Correct. This way Git doesn't need to deal with threading...
>
> I think you need to be careful about three things (at least; there
> may be more):
>
> * Codepaths that check out multiple cache entries do rely on the
> order of checkout. We checkout removals first to make room so
> that creation of a path X can succeed if an existing path X/Y
> that used to want to see X as a directory can succeed (see the
> use of checkout_entry() by "git checkout", which does have two
> separate loops to explicitly guarantee this), for example. I
> think "remove all and then create" you do not specifically have
> to worry about with the proposed change, but you may need to
> inspect and verify there aren't other kind of order dependency.
OK
> * Done naively, it will lead to unmaintainable code, like this:
>
> + struct list_of_cache_entries *list = ...;
> for (i = 0; i < active_nr; i++)
> - checkout_entry(active_cache[i], state, NULL);
> + if (checkout_entry(active_cache[i], state, NULL) == DELAYED)
> + add_cache_to_queue(&list, active_cache[i]);
> + while (list) {
> + wait_for_checkout_to_finish(*list);
> + list = list->next;
> + }
>
> I do not think we want to see such a rewrite all over the
> codepaths. It might be OK to add such a "these entries are known
> to be delayed" list in struct checkout so that the above becomes
> more like this:
>
> for (i = 0; i < active_nr; i++)
> checkout_entry(active_cache[i], state, NULL);
> + checkout_entry_finish(state);
>
> That is, addition of a single "some of the checkout_entry() calls
> done so far might have been lazy, and I'll give them a chance to
> clean up" might be palatable. Anything more than that on the
> caller side is not.
I haven't thought hard about the implementation, yet, but I'll try
to stick to your suggestion and change as less code as possible on
the caller sides.
> * You'd need to rein in the maximum parallelism somehow, as you do
> not want to see hundreds of competing filter processes starting
> only to tell the main loop over an index with hundreds of entries
> that they are delayed checkouts.
I intend to implement this feature only for the new long running filter
process protocol. OK with you?
Thanks,
Lars
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Johannes Schindelin @ 2016-11-16 9:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael J Gruber, Jacob Keller, Dennis Kaarsemaker,
Git mailing list
In-Reply-To: <xmqqtwb9wywp.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 14 Nov 2016, Junio C Hamano wrote:
> I _think_ the no-index mode was primarily for those who want to use
> our diff as a replacement for GNU and other diffs, and from that
> point of view, I'd favour not doing the "comparing symbolic link?
> We'll show the difference between the link contents, not target"
> under no-index mode myself.
If I read this correctly, then we are in agreement that the default for
--no-index should be as it is right now, i.e. comparing symlink targets as
opposed to --follow-links.
> That is a lot closer to the diff other people implemented, not ours.
> Hence the knee-jerk reaction I gave in
>
> http://public-inbox.org/git/xmqqinrt1zcx.fsf@gitster.mtv.corp.google.com
Let me quote the knee-jerk reaction:
> My knee-jerk reaction is:
>
> * The --no-index mode should default to your --follow-symlinks
> behaviour, without any option to turn it on or off.
But this is the exact opposite of what I find reasonable.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Johannes Schindelin @ 2016-11-16 9:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, Johannes Sixt, git, Jeff King
In-Reply-To: <alpine.DEB.2.20.1611151753300.3746@virtualbox>
Hi,
On Tue, 15 Nov 2016, Johannes Schindelin wrote:
> On Mon, 14 Nov 2016, Junio C Hamano wrote:
>
> > Dscho's mention of 'still times out' may be an indiciation that
> > something unspecified on 'pu' is not ready to be merged to 'next',
> > but blocking all of 'pu' with a blanket statement is not useful,
> > and that was where my response comes from.
>
> Until the time when the test suite takes less than the insane three hours
> to run, I am afraid that a blanket statement on `pu` is the best I can do.
Well, I should add that the test suite does not take 3 hours to run for
`pu` these days.
It used to time out after four hours until two days ago (I think; I was a
bit too busy with other CI work to pay close attention to the constantly
failing `pu` job, with quite a few failing `next`s and even a couple of
failing `master`s thrown in).
As of two days ago, the test suite takes no time at all. The build already
fails (which makes me wonder why a couple of patch series I contributed
had such a hard time getting into `pu` when they compiled and tested
just fine, whereas some obviously non-building stuff gets into `pu`
already, makes no sense to me).
This is the offending part from last night's build:
-- snipsnap --
2016-11-16T00:31:57.5321220Z copy.c: In function 'copy_dir_1':
2016-11-16T00:31:57.5321220Z copy.c:369:8: error: implicit declaration of function 'lchown' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Z if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
2016-11-16T00:31:57.5321220Z ^~~~~~
2016-11-16T00:31:57.5321220Z copy.c:391:7: error: implicit declaration of function 'mknod' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Z if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
2016-11-16T00:31:57.5321220Z ^~~~~
2016-11-16T00:31:57.5321220Z copy.c:405:7: error: implicit declaration of function 'utimes' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Z if (utimes(dest, times) < 0)
2016-11-16T00:31:57.5321220Z ^~~~~~
2016-11-16T00:31:57.5321220Z copy.c:407:7: error: implicit declaration of function 'chown' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Z if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
2016-11-16T00:31:57.5321220Z ^~~~~
2016-11-16T00:31:57.7982432Z CC ctype.o
2016-11-16T00:31:58.1418929Z cc1.exe: all warnings being treated as errors
2016-11-16T00:31:58.6368128Z make: *** [Makefile:1988: copy.o] Error 1
^ permalink raw reply
* Re: gitweb html validation
From: Raphaël Gertz @ 2016-11-16 9:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ralf Thielow, Jakub Narębski, git
In-Reply-To: <xmqqy40ks301.fsf@gitster.mtv.corp.google.com>
Le 16.11.2016 01:05, Junio C Hamano a écrit :
> Ralf Thielow <ralf.thielow@gmail.com> writes:
>
>> Only block level elements are
>> allowed to be inside form tags, according to
>> https://www.w3.org/2010/04/xhtml10-strict.html#elem_form
>> ...
>> I think it's better to just move the <form>-Tag outside of the
>> surrounding div?
>> Something like this perhaps, I didn't test it myself yet.
>
> That sounds like a sensible update to me (no, I do not run gitweb
> myself). Is this the only <form> we have in the UI, or is it the
> only one that is problematic?
>
There is an other form in the cgi line 4110 :
print $cgi->start_form(-method => "get", -action => $action) .
"<div class=\"search\">\n" .
But this one has a <div class="search"> inside.
The problem with projsearch I want to change is that the div is around
the form without a container inside.
I agree with moving the <div class="projsearch"> inside the form if it's
a better option.
Best regards
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-16 7:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqq4m38vdw4.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 15, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> dirname makes sense. What about implementing a reverse variant of
>> strip, which you could perform stripping of right-most components and
>> instead of stripping by a number, strip "to" a number, ie: keep the
>> left N most components, and then you could use something like
>> ...
>> I think that would be more general purpose than basename, and less confusing?
>
> I think you are going in the right direction. I had a similar
> thought but built around a different axis. I.e. if strip=1 strips
> one from the left, perhaps we want to have rstrip=1 that strips one
> from the right, and also strip=-1 to mean strip everything except
> one from the left and so on?. I think this and your keep (and
> perhaps you'll have rkeep for completeness) have the same expressive
> power. I do not offhand have a preference one over the other.
>
If we do implement strip with negative numbers, it definitely would be
neat, but to get
the desired feature which I've mentioned below, we'd need to call
strip twice, i.e
to get remotes from /refs/foo/abc/xyz we'd need to do
strip=1,strip=-1, which could be
done but would need a lot of tweaking, since we have the same
subatom/option having
multiple values.
On the other hand it would be easier maybe to just introduce rstrip,
where we strip from
right and ensure that strip and rstrip can be used together.
On Wed, Nov 16, 2016 at 2:49 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On November 15, 2016 9:42:03 AM PST, Junio C Hamano <gitster@pobox.com> wrote:
>>Somehow it sounds a bit strange to me to treat 'remotes' as the same
>>class of token as 'heads' and 'tags' (I'd expect 'heads' and
>>'remotes/origin' would be at the same level in end-user's mind), but
>>that is probably an unrelated tangent. The reason this series wants
>>to introduce :base must be to emulate an existing feature, so that
>>existing feature is a concrete counter-example that argues against
>>my "it sounds a bit strange" reaction.
>
> It may be a bit strange indeed. What is the requirement for this?
>
> I think implementing a strip and rstrip ( if necessary ) with negative numbers would be most ideal.
The necessity is that we need to do different formatting as per the
ref type in branch -l. If you see the
last but one patch, we do
strbuf_addf(&fmt,
"%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
local.buf, remote.buf);
where its using ':base' to check for the ref type and do conditional
printing along with the %(if)...%(end) atoms.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: Bug with disabling compression and 'binary' files.
From: Junio C Hamano @ 2016-11-16 5:23 UTC (permalink / raw)
To: Douglas Cox; +Cc: git
In-Reply-To: <CA+i4re4FZhshheftQnsogaY7601wgf_GfsFUxy+doLcOYPWv7Q@mail.gmail.com>
Douglas Cox <ziflin@gmail.com> writes:
>> This may or may not be related to the symptom
>> you are observing (if it is, then you would see a packfile created
>> in objects/pack/, not in loose objects in object/??/ directories).
>
> No, the file is loose (it's in .git/objects/eb in this case). This is
> seen immediately after the add, though I believe it's the same way
> when doing a commit on a changed file.
Then I do not have a guess as to where the symptom you are seeing is
coming from.
^ permalink raw reply
* [PATCH v2] compression: unify pack.compression configuration parsing
From: Junio C Hamano @ 2016-11-16 5:21 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqfumsrvge.fsf@gitster.mtv.corp.google.com>
There are three codepaths that use a variable whose name is
pack_compression_level to affect how objects and deltas sent to a
packfile is compressed. Unlike zlib_compression_level that controls
the loose object compression, however, this variable was static to
each of these codepaths. Two of them read the pack.compression
configuration variable, using core.compression as the default, and
one of them also allowed overriding it from the command line.
The other codepath in bulk-checkin did not pay any attention to the
configuration.
Unify the configuration parsing to git_default_config(), where we
implement the parsing of core.loosecompression and core.compression
and make the former override the latter, by moving code to parse
pack.compression and also allow core.compression to give default to
this variable.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Now comes with new tests to cover pack-objects and fast-import,
which should not fail with or without code change because they
are only here to prevent regression with this change.
On the code side, *_compression_seen variables are also made
private to config.c, as these variables are used only by the
config reader to implement "core.compression gives the default to
*.compression" logic and the code is consolidated to this single
file.
builtin/pack-objects.c | 14 --------
bulk-checkin.c | 2 --
cache.h | 2 +-
config.c | 16 +++++++++
environment.c | 2 +-
fast-import.c | 13 -------
t/t1050-large.sh | 29 ++++++++++++++++
t/t5315-pack-objects-compression.sh | 44 ++++++++++++++++++++++++
t/t9303-fast-import-compression.sh | 67 +++++++++++++++++++++++++++++++++++++
9 files changed, 158 insertions(+), 31 deletions(-)
create mode 100755 t/t5315-pack-objects-compression.sh
create mode 100755 t/t9303-fast-import-compression.sh
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0fd52bd6b4..8841f8b366 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -61,8 +61,6 @@ static int delta_search_threads;
static int pack_to_stdout;
static int num_preferred_base;
static struct progress *progress_state;
-static int pack_compression_level = Z_DEFAULT_COMPRESSION;
-static int pack_compression_seen;
static struct packed_git *reuse_packfile;
static uint32_t reuse_packfile_objects;
@@ -2368,16 +2366,6 @@ static int git_pack_config(const char *k, const char *v, void *cb)
depth = git_config_int(k, v);
return 0;
}
- if (!strcmp(k, "pack.compression")) {
- int level = git_config_int(k, v);
- if (level == -1)
- level = Z_DEFAULT_COMPRESSION;
- else if (level < 0 || level > Z_BEST_COMPRESSION)
- die("bad pack compression level %d", level);
- pack_compression_level = level;
- pack_compression_seen = 1;
- return 0;
- }
if (!strcmp(k, "pack.deltacachesize")) {
max_delta_cache_size = git_config_int(k, v);
return 0;
@@ -2869,8 +2857,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
reset_pack_idx_option(&pack_idx_opts);
git_config(git_pack_config, NULL);
- if (!pack_compression_seen && core_compression_seen)
- pack_compression_level = core_compression_level;
progress = isatty(2);
argc = parse_options(argc, argv, prefix, pack_objects_options,
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 4347f5c76a..991b4a13e2 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -7,8 +7,6 @@
#include "pack.h"
#include "strbuf.h"
-static int pack_compression_level = Z_DEFAULT_COMPRESSION;
-
static struct bulk_checkin_state {
unsigned plugged:1;
diff --git a/cache.h b/cache.h
index a50a61a197..b14c0e8e38 100644
--- a/cache.h
+++ b/cache.h
@@ -670,7 +670,7 @@ extern const char *git_attributes_file;
extern const char *git_hooks_path;
extern int zlib_compression_level;
extern int core_compression_level;
-extern int core_compression_seen;
+extern int pack_compression_level;
extern size_t packed_git_window_size;
extern size_t packed_git_limit;
extern size_t delta_base_cache_limit;
diff --git a/config.c b/config.c
index 83fdecb1bc..2f1aef742e 100644
--- a/config.c
+++ b/config.c
@@ -66,6 +66,8 @@ static struct key_value_info *current_config_kvi;
*/
static enum config_scope current_parsing_scope;
+static int core_compression_seen;
+static int pack_compression_seen;
static int zlib_compression_seen;
/*
@@ -865,6 +867,8 @@ static int git_default_core_config(const char *var, const char *value)
core_compression_seen = 1;
if (!zlib_compression_seen)
zlib_compression_level = level;
+ if (!pack_compression_seen)
+ pack_compression_level = level;
return 0;
}
@@ -1125,6 +1129,18 @@ int git_default_config(const char *var, const char *value, void *dummy)
pack_size_limit_cfg = git_config_ulong(var, value);
return 0;
}
+
+ if (!strcmp(var, "pack.compression")) {
+ int level = git_config_int(var, value);
+ if (level == -1)
+ level = Z_DEFAULT_COMPRESSION;
+ else if (level < 0 || level > Z_BEST_COMPRESSION)
+ die(_("bad pack compression level %d"), level);
+ pack_compression_level = level;
+ pack_compression_seen = 1;
+ return 0;
+ }
+
/* Add other config variables here and to Documentation/config.txt. */
return 0;
}
diff --git a/environment.c b/environment.c
index 0935ec696e..4bce3eebfa 100644
--- a/environment.c
+++ b/environment.c
@@ -34,7 +34,7 @@ const char *git_attributes_file;
const char *git_hooks_path;
int zlib_compression_level = Z_BEST_SPEED;
int core_compression_level;
-int core_compression_seen;
+int pack_compression_level = Z_DEFAULT_COMPRESSION;
int fsync_object_files;
size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
diff --git a/fast-import.c b/fast-import.c
index cb545d7df5..f561ba833b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -284,8 +284,6 @@ static unsigned long max_depth = 10;
static off_t max_packsize;
static int unpack_limit = 100;
static int force_update;
-static int pack_compression_level = Z_DEFAULT_COMPRESSION;
-static int pack_compression_seen;
/* Stats and misc. counters */
static uintmax_t alloc_count;
@@ -3381,15 +3379,6 @@ static void git_pack_config(void)
if (max_depth > MAX_DEPTH)
max_depth = MAX_DEPTH;
}
- if (!git_config_get_int("pack.compression", &pack_compression_level)) {
- if (pack_compression_level == -1)
- pack_compression_level = Z_DEFAULT_COMPRESSION;
- else if (pack_compression_level < 0 ||
- pack_compression_level > Z_BEST_COMPRESSION)
- git_die_config("pack.compression",
- "bad pack compression level %d", pack_compression_level);
- pack_compression_seen = 1;
- }
if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
pack_idx_opts.version = indexversion_value;
if (pack_idx_opts.version > 2)
@@ -3454,8 +3443,6 @@ int cmd_main(int argc, const char **argv)
setup_git_directory();
reset_pack_idx_option(&pack_idx_opts);
git_pack_config();
- if (!pack_compression_seen && core_compression_seen)
- pack_compression_level = core_compression_level;
alloc_objects(object_entry_alloc);
strbuf_init(&command_buf, 0);
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index 096dbffecc..6fd264cff0 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -5,6 +5,12 @@ test_description='adding and checking out large blobs'
. ./test-lib.sh
+# This should be moved to test-lib.sh together with the
+# copy in t0021 after both topics have graduated to 'master'.
+file_size () {
+ perl -e 'print -s $ARGV[0]' "$1"
+}
+
test_expect_success setup '
# clone does not allow us to pass core.bigfilethreshold to
# new repos, so set core.bigfilethreshold globally
@@ -17,6 +23,29 @@ test_expect_success setup '
export GIT_ALLOC_LIMIT
'
+# add a large file with different settings
+while read expect config
+do
+ test_expect_success "add with $config" '
+ test_when_finished "rm -f .git/objects/pack/pack-*.* .git/index" &&
+ git $config add large1 &&
+ sz=$(file_size .git/objects/pack/pack-*.pack) &&
+ case "$expect" in
+ small) test "$sz" -le 100000 ;;
+ large) test "$sz" -ge 100000 ;;
+ esac
+ '
+done <<\EOF
+large -c core.compression=0
+small -c core.compression=9
+large -c core.compression=0 -c pack.compression=0
+large -c core.compression=9 -c pack.compression=0
+small -c core.compression=0 -c pack.compression=9
+small -c core.compression=9 -c pack.compression=9
+large -c pack.compression=0
+small -c pack.compression=9
+EOF
+
test_expect_success 'add a large file or two' '
git add large1 huge large2 &&
# make sure we got a single packfile and no loose objects
diff --git a/t/t5315-pack-objects-compression.sh b/t/t5315-pack-objects-compression.sh
new file mode 100755
index 0000000000..34c47dae09
--- /dev/null
+++ b/t/t5315-pack-objects-compression.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+
+test_description='pack-object compression configuration'
+
+. ./test-lib.sh
+
+# This should be moved to test-lib.sh together with the
+# copy in t0021 after both topics have graduated to 'master'.
+file_size () {
+ perl -e 'print -s $ARGV[0]' "$1"
+}
+
+test_expect_success setup '
+ printf "%2000000s" X |
+ git hash-object -w --stdin >object-name &&
+ # make sure it resulted in a loose object
+ ob=$(sed -e "s/\(..\).*/\1/" object-name) &&
+ ject=$(sed -e "s/..\(.*\)/\1/" object-name) &&
+ test -f .git/objects/$ob/$ject
+'
+
+while read expect config
+do
+ test_expect_success "pack-objects with $config" '
+ test_when_finished "rm -f pack-*.*" &&
+ git $config pack-objects pack <object-name &&
+ sz=$(file_size pack-*.pack) &&
+ case "$expect" in
+ small) test "$sz" -le 100000 ;;
+ large) test "$sz" -ge 100000 ;;
+ esac
+ '
+done <<\EOF
+large -c core.compression=0
+small -c core.compression=9
+large -c core.compression=0 -c pack.compression=0
+large -c core.compression=9 -c pack.compression=0
+small -c core.compression=0 -c pack.compression=9
+small -c core.compression=9 -c pack.compression=9
+large -c pack.compression=0
+small -c pack.compression=9
+EOF
+
+test_done
diff --git a/t/t9303-fast-import-compression.sh b/t/t9303-fast-import-compression.sh
new file mode 100755
index 0000000000..856219f46a
--- /dev/null
+++ b/t/t9303-fast-import-compression.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description='compression setting of fast-import utility'
+. ./test-lib.sh
+
+# This should be moved to test-lib.sh together with the
+# copy in t0021 after both topics have graduated to 'master'.
+file_size () {
+ perl -e 'print -s $ARGV[0]' "$1"
+}
+
+import_large () {
+ (
+ echo blob
+ echo "data <<EOD"
+ printf "%2000000s\n" "$*"
+ echo EOD
+ ) | git "$@" fast-import
+}
+
+while read expect config
+do
+ test_expect_success "fast-import (packed) with $config" '
+ test_when_finished "rm -f .git/objects/pack/pack-*.*" &&
+ test_when_finished "rm -rf .git/objects/??" &&
+ import_large -c fastimport.unpacklimit=0 $config &&
+ sz=$(file_size .git/objects/pack/pack-*.pack) &&
+ case "$expect" in
+ small) test "$sz" -le 100000 ;;
+ large) test "$sz" -ge 100000 ;;
+ esac
+ '
+done <<\EOF
+large -c core.compression=0
+small -c core.compression=9
+large -c core.compression=0 -c pack.compression=0
+large -c core.compression=9 -c pack.compression=0
+small -c core.compression=0 -c pack.compression=9
+small -c core.compression=9 -c pack.compression=9
+large -c pack.compression=0
+small -c pack.compression=9
+EOF
+
+while read expect config
+do
+ test_expect_success "fast-import (loose) with $config" '
+ test_when_finished "rm -f .git/objects/pack/pack-*.*" &&
+ test_when_finished "rm -rf .git/objects/??" &&
+ import_large -c fastimport.unpacklimit=9 $config &&
+ sz=$(file_size .git/objects/??/????*) &&
+ case "$expect" in
+ small) test "$sz" -le 100000 ;;
+ large) test "$sz" -ge 100000 ;;
+ esac
+ '
+done <<\EOF
+large -c core.compression=0
+small -c core.compression=9
+large -c core.compression=0 -c core.loosecompression=0
+large -c core.compression=9 -c core.loosecompression=0
+small -c core.compression=0 -c core.loosecompression=9
+small -c core.compression=9 -c core.loosecompression=9
+large -c core.loosecompression=0
+small -c core.loosecompression=9
+EOF
+
+test_done
--
2.11.0-rc1-154-g4118e37061
^ permalink raw reply related
* Re: Bug with disabling compression and 'binary' files.
From: Douglas Cox @ 2016-11-16 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqk2c4ryj8.fsf@gitster.mtv.corp.google.com>
> This may or may not be related to the symptom
> you are observing (if it is, then you would see a packfile created
> in objects/pack/, not in loose objects in object/??/ directories).
No, the file is loose (it's in .git/objects/eb in this case). This is
seen immediately after the add, though I believe it's the same way
when doing a commit on a changed file.
On Tue, Nov 15, 2016 at 8:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Douglas Cox <ziflin@gmail.com> writes:
>
>> I narrowed this down to the '-text' attribute that is set when
>> specifying 'binary'. For some reason this flag is cancelling out the
>> core.compression = 0 setting and I think this is a bug?
>>
>> Unfortunately core.compression = 0 is also global. Ideally it would be
>> great if there was a separate 'compression' attribute that could be
>> specified in .gitattributes per wildcard similar to how -delta can be
>> used. This way we would still be able to get compression for
>> text/source files, while still getting the speed of skipping
>> compression for binary files that do not compress well.
>>
>> Has there been any discussion on having an attribute similar to this?
>
> Nope.
>
> I do not offhand think of a way for '-text' attribute (or any
> attribute for what matter) to interfere with compression level, but
> while reading the various parts of the system that futz with the
> compression level configuration, I noticed one thing. When we do an
> initial "bulk-checkin" optimization that sends all objects to a
> single packfile upon "git add", the packfile creation uses its own
> compression level that is not affected by any configuration or
> command line option. This may or may not be related to the symptom
> you are observing (if it is, then you would see a packfile created
> in objects/pack/, not in loose objects in object/??/ directories).
>
>
>
^ permalink raw reply
* [PATCH] compression: unify pack.compression configuration parsing
From: Junio C Hamano @ 2016-11-16 2:48 UTC (permalink / raw)
To: git
There are three codepaths that use a variable whose name is
pack_compression_level to affect how objects and deltas sent to a
packfile is compressed. Unlike zlib_compression_level that controls
the loose object compression, however, this variable was static to
each of these codepaths. Two of them read the pack.compression
configuration variable, using core.compression as the default, and
one of them also allowed overriding it from the command line.
The other codepath in bulk-checkin did not pay any attention to the
configuration.
Unify the configuration parsing to git_default_config(), where we
implement the parsing of core.loosecompression and core.compression
and make the former override the latter, by moving code to parse
pack.compression and also allow core.compression to give default to
this variable.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I was primarily interested in teaching the configuration to
bulk-checkin codepath, and added a test that succeeds only with
the code change. The handling of confugraiton in other two
codepaths, pack-objects and fast-import, may be totally broken
with this change if there is no existing test. A similar set of
tests for them are very much welcomed, as without them this
change won't be 'next' worthy yet.
builtin/pack-objects.c | 14 --------------
bulk-checkin.c | 2 --
cache.h | 2 ++
config.c | 14 ++++++++++++++
environment.c | 2 ++
fast-import.c | 13 -------------
t/t1050-large.sh | 30 ++++++++++++++++++++++++++++++
7 files changed, 48 insertions(+), 29 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0fd52bd6b4..8841f8b366 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -61,8 +61,6 @@ static int delta_search_threads;
static int pack_to_stdout;
static int num_preferred_base;
static struct progress *progress_state;
-static int pack_compression_level = Z_DEFAULT_COMPRESSION;
-static int pack_compression_seen;
static struct packed_git *reuse_packfile;
static uint32_t reuse_packfile_objects;
@@ -2368,16 +2366,6 @@ static int git_pack_config(const char *k, const char *v, void *cb)
depth = git_config_int(k, v);
return 0;
}
- if (!strcmp(k, "pack.compression")) {
- int level = git_config_int(k, v);
- if (level == -1)
- level = Z_DEFAULT_COMPRESSION;
- else if (level < 0 || level > Z_BEST_COMPRESSION)
- die("bad pack compression level %d", level);
- pack_compression_level = level;
- pack_compression_seen = 1;
- return 0;
- }
if (!strcmp(k, "pack.deltacachesize")) {
max_delta_cache_size = git_config_int(k, v);
return 0;
@@ -2869,8 +2857,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
reset_pack_idx_option(&pack_idx_opts);
git_config(git_pack_config, NULL);
- if (!pack_compression_seen && core_compression_seen)
- pack_compression_level = core_compression_level;
progress = isatty(2);
argc = parse_options(argc, argv, prefix, pack_objects_options,
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 4347f5c76a..991b4a13e2 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -7,8 +7,6 @@
#include "pack.h"
#include "strbuf.h"
-static int pack_compression_level = Z_DEFAULT_COMPRESSION;
-
static struct bulk_checkin_state {
unsigned plugged:1;
diff --git a/cache.h b/cache.h
index a50a61a197..a5d689f9d3 100644
--- a/cache.h
+++ b/cache.h
@@ -671,6 +671,8 @@ extern const char *git_hooks_path;
extern int zlib_compression_level;
extern int core_compression_level;
extern int core_compression_seen;
+extern int pack_compression_level;
+extern int pack_compression_seen;
extern size_t packed_git_window_size;
extern size_t packed_git_limit;
extern size_t delta_base_cache_limit;
diff --git a/config.c b/config.c
index 83fdecb1bc..0477386f27 100644
--- a/config.c
+++ b/config.c
@@ -865,6 +865,8 @@ static int git_default_core_config(const char *var, const char *value)
core_compression_seen = 1;
if (!zlib_compression_seen)
zlib_compression_level = level;
+ if (!pack_compression_seen)
+ pack_compression_level = level;
return 0;
}
@@ -1125,6 +1127,18 @@ int git_default_config(const char *var, const char *value, void *dummy)
pack_size_limit_cfg = git_config_ulong(var, value);
return 0;
}
+
+ if (!strcmp(var, "pack.compression")) {
+ int level = git_config_int(var, value);
+ if (level == -1)
+ level = Z_DEFAULT_COMPRESSION;
+ else if (level < 0 || level > Z_BEST_COMPRESSION)
+ die(_("bad pack compression level %d"), level);
+ pack_compression_level = level;
+ pack_compression_seen = 1;
+ return 0;
+ }
+
/* Add other config variables here and to Documentation/config.txt. */
return 0;
}
diff --git a/environment.c b/environment.c
index 0935ec696e..88ab1c38c6 100644
--- a/environment.c
+++ b/environment.c
@@ -35,6 +35,8 @@ const char *git_hooks_path;
int zlib_compression_level = Z_BEST_SPEED;
int core_compression_level;
int core_compression_seen;
+int pack_compression_level = Z_DEFAULT_COMPRESSION;
+int pack_compression_seen;
int fsync_object_files;
size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
diff --git a/fast-import.c b/fast-import.c
index cb545d7df5..f561ba833b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -284,8 +284,6 @@ static unsigned long max_depth = 10;
static off_t max_packsize;
static int unpack_limit = 100;
static int force_update;
-static int pack_compression_level = Z_DEFAULT_COMPRESSION;
-static int pack_compression_seen;
/* Stats and misc. counters */
static uintmax_t alloc_count;
@@ -3381,15 +3379,6 @@ static void git_pack_config(void)
if (max_depth > MAX_DEPTH)
max_depth = MAX_DEPTH;
}
- if (!git_config_get_int("pack.compression", &pack_compression_level)) {
- if (pack_compression_level == -1)
- pack_compression_level = Z_DEFAULT_COMPRESSION;
- else if (pack_compression_level < 0 ||
- pack_compression_level > Z_BEST_COMPRESSION)
- git_die_config("pack.compression",
- "bad pack compression level %d", pack_compression_level);
- pack_compression_seen = 1;
- }
if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
pack_idx_opts.version = indexversion_value;
if (pack_idx_opts.version > 2)
@@ -3454,8 +3443,6 @@ int cmd_main(int argc, const char **argv)
setup_git_directory();
reset_pack_idx_option(&pack_idx_opts);
git_pack_config();
- if (!pack_compression_seen && core_compression_seen)
- pack_compression_level = core_compression_level;
alloc_objects(object_entry_alloc);
strbuf_init(&command_buf, 0);
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index 096dbffecc..f800af1451 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -5,6 +5,13 @@ test_description='adding and checking out large blobs'
. ./test-lib.sh
+# This should be moved to test-lib.sh together with the
+# copy in t0021 after both topics have graduated to 'master'.
+
+file_size () {
+ perl -e 'print -s $ARGV[0]' "$1"
+}
+
test_expect_success setup '
# clone does not allow us to pass core.bigfilethreshold to
# new repos, so set core.bigfilethreshold globally
@@ -17,6 +24,29 @@ test_expect_success setup '
export GIT_ALLOC_LIMIT
'
+# add a large file with different settings
+while read expect config
+do
+ test_expect_success "add with $config" '
+ test_when_finished "rm -f .git/objects/pack/pack-*.* .git/index" &&
+ git $config add large1 &&
+ sz=$(file_size .git/objects/pack/pack-*.pack) &&
+ case "$expect" in
+ small) test "$sz" -le 100000 ;;
+ large) test "$sz" -ge 100000 ;;
+ esac
+ '
+done <<-EOF
+large -c core.compression=0
+small -c core.compression=9
+large -c core.compression=0 -c pack.compression=0
+large -c core.compression=9 -c pack.compression=0
+small -c core.compression=0 -c pack.compression=9
+small -c core.compression=9 -c pack.compression=9
+large -c pack.compression=0
+small -c pack.compression=9
+EOF
+
test_expect_success 'add a large file or two' '
git add large1 huge large2 &&
# make sure we got a single packfile and no loose objects
--
2.11.0-rc1-154-g4118e37061
^ permalink raw reply related
* Re: Bug with disabling compression and 'binary' files.
From: Junio C Hamano @ 2016-11-16 1:42 UTC (permalink / raw)
To: Douglas Cox; +Cc: git
In-Reply-To: <CA+i4re65SsxcaLcpGyMDnJygQFmAq4X_x_uxrkqB0yqQkEYPUQ@mail.gmail.com>
Douglas Cox <ziflin@gmail.com> writes:
> I narrowed this down to the '-text' attribute that is set when
> specifying 'binary'. For some reason this flag is cancelling out the
> core.compression = 0 setting and I think this is a bug?
>
> Unfortunately core.compression = 0 is also global. Ideally it would be
> great if there was a separate 'compression' attribute that could be
> specified in .gitattributes per wildcard similar to how -delta can be
> used. This way we would still be able to get compression for
> text/source files, while still getting the speed of skipping
> compression for binary files that do not compress well.
>
> Has there been any discussion on having an attribute similar to this?
Nope.
I do not offhand think of a way for '-text' attribute (or any
attribute for what matter) to interfere with compression level, but
while reading the various parts of the system that futz with the
compression level configuration, I noticed one thing. When we do an
initial "bulk-checkin" optimization that sends all objects to a
single packfile upon "git add", the packfile creation uses its own
compression level that is not affected by any configuration or
command line option. This may or may not be related to the symptom
you are observing (if it is, then you would see a packfile created
in objects/pack/, not in loose objects in object/??/ directories).
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Jacob Keller @ 2016-11-15 21:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Git mailing list
In-Reply-To: <xmqq4m38vdw4.fsf@gitster.mtv.corp.google.com>
On November 15, 2016 9:42:03 AM PST, Junio C Hamano <gitster@pobox.com> wrote:
>I think you are going in the right direction. I had a similar
>thought but built around a different axis. I.e. if strip=1 strips
>one from the left, perhaps we want to have rstrip=1 that strips one
>from the right, and also strip=-1 to mean strip everything except
>one from the left and so on?. I think this and your keep (and
>perhaps you'll have rkeep for completeness) have the same expressive
>power. I do not offhand have a preference one over the other.
I prefer strip implemented with negative numbers. That is simple and expressive and if we need we can implement rstrip if necessary.
>
>Somehow it sounds a bit strange to me to treat 'remotes' as the same
>class of token as 'heads' and 'tags' (I'd expect 'heads' and
>'remotes/origin' would be at the same level in end-user's mind), but
>that is probably an unrelated tangent. The reason this series wants
>to introduce :base must be to emulate an existing feature, so that
>existing feature is a concrete counter-example that argues against
>my "it sounds a bit strange" reaction.
It may be a bit strange indeed. What is the requirement for this?
I think implementing a strip and rstrip ( if necessary ) with negative numbers would be most ideal.
Thanks
Jake
^ permalink raw reply
* Re: [PATCH v3 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Stefan Beller @ 2016-11-16 1:09 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jonathan Tan, Junio C Hamano
In-Reply-To: <1478908273-190166-6-git-send-email-bmwill@google.com>
On Fri, Nov 11, 2016 at 3:51 PM, Brandon Williams <bmwill@google.com> wrote:
> to:
> HEAD:file
> HEAD:sub/file
Maybe indent this ;)
> static struct argv_array submodule_options = ARGV_ARRAY_INIT;
> +static const char *parent_basename;
>
> static int grep_submodule_launch(struct grep_opt *opt,
> const struct grep_source *gs);
> @@ -535,19 +537,53 @@ static int grep_submodule_launch(struct grep_opt *opt,
> {
> struct child_process cp = CHILD_PROCESS_INIT;
> int status, i;
> + const char *end_of_base;
> + const char *name;
> struct work_item *w = opt->output_priv;
>
> + end_of_base = strchr(gs->name, ':');
> + if (end_of_base)
> + name = end_of_base + 1;
> + else
> + name = gs->name;
> +
> prepare_submodule_repo_env(&cp.env_array);
>
> /* Add super prefix */
> argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
> super_prefix ? super_prefix : "",
> - gs->name);
> + name);
> argv_array_push(&cp.args, "grep");
>
> + /*
> + * Add basename of parent project
> + * When performing grep on a <tree> object the filename is prefixed
> + * with the object's name: '<tree-name>:filename'.
This comment is hard to read as it's unclear what the <angle brackets> mean.
(Are the supposed to indicate a variable? If so why is file name not marked up?)
> In order to
> + * provide uniformity of output we want to pass the name of the
> + * parent project's object name to the submodule so the submodule can
> + * prefix its output with the parent's name and not its own SHA1.
> + */
> + if (end_of_base)
> + argv_array_pushf(&cp.args, "--parent-basename=%.*s",
> + (int) (end_of_base - gs->name),
> + gs->name);
Do we pass this only with the tree-ish?
What if we are grepping the working tree and the file name contains a colon?
> +test_expect_success 'grep tree HEAD^' '
> + cat >expect <<-\EOF &&
> + HEAD^:a:foobar
> + HEAD^:b/b:bar
> + HEAD^:submodule/a:foobar
> + EOF
> +
> + git grep -e "bar" --recurse-submodules HEAD^ > actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'grep tree HEAD^^' '
> + cat >expect <<-\EOF &&
> + HEAD^^:a:foobar
> + HEAD^^:b/b:bar
> + EOF
> +
> + git grep -e "bar" --recurse-submodules HEAD^^ > actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'grep tree and pathspecs' '
> + cat >expect <<-\EOF &&
> + HEAD:submodule/a:foobar
> + HEAD:submodule/sub/a:foobar
> + EOF
> +
> + git grep -e "bar" --recurse-submodules HEAD -- submodule > actual &&
> + test_cmp expect actual
> +'
Mind to add tests for
* recursive submodules (say 2 levels), preferrably not having the
gitlink at the root each, i.e. root has a sub1 at path subs/sub1 and
sub1 has a sub2
at path subs/sub2, such that recursing would produce a path like
HEAD:subs/sub1/subs/sub2/dir/file ?
* file names with a colon in it
* instead of just HEAD referencing trees, maybe a sha1 referenced test as well
(though it is not immediately clear what the benefit would be)
* what if the submodule doesn't have the commit referenced in the given sha1
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 14/16] checkout: recurse into submodules if asked to
From: Brandon Williams @ 2016-11-16 0:33 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jrnieder, mogulguy10, David.Turner
In-Reply-To: <20161115230651.23953-15-sbeller@google.com>
On 11/15, Stefan Beller wrote:
> +int option_parse_recurse_submodules(const struct option *opt,
> + const char *arg, int unset)
> +{
> + if (unset) {
> + recurse_submodules = RECURSE_SUBMODULES_OFF;
> + return 0;
> + }
> + if (arg)
> + recurse_submodules =
> + parse_update_recurse_submodules_arg(opt->long_name,
> + arg);
> + else
> + recurse_submodules = RECURSE_SUBMODULES_ON;
> +
> + return 0;
> +}
I assume it is ok to always return 0 from this function? Also, I know
we don't like having braces on single statement if's but it is a bit
hard to read that multi-line statement without braces. But that's just
my opinion :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 13/16] submodule: teach unpack_trees() to update submodules
From: Brandon Williams @ 2016-11-16 0:25 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jrnieder, mogulguy10, David.Turner
In-Reply-To: <20161115230651.23953-14-sbeller@google.com>
On 11/15, Stefan Beller wrote:
> + if (!S_ISGITLINK(ce->ce_mode)) {
> + int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
For readability you may want to have spaces between the two flags
> + if (o->index_only
> + || (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old))
> + && (o->reset || ce_uptodate(old))))
> + return 0;
The coding guidelines say that git prefers to have the logical operators
on the right side like this:
if (o->index_only ||
(!((old->ce_flags & CE_VALID) || ce_skip_worktree(old)) &&
(o->reset || ce_uptodate(old))))
return 0;
It also says the other way is ok too, just a thought :)
> + if (submodule_is_interesting(old->name, null_sha1)
> + && is_submodule_checkout_safe(new->name, &new->oid))
> + return 0;
here too.
> + } else {
> + /*
> + * new is not a submodule any more, so only
> + * care if we care:
> + */
> + if (submodule_is_interesting(old->name, null_sha1)
> + && ok_to_remove_submodule(old->name))
> + return 0;
and here
--
Brandon Williams
^ permalink raw reply
* RE: [PATCH 13/16] submodule: teach unpack_trees() to update submodules
From: David Turner @ 2016-11-16 0:22 UTC (permalink / raw)
To: 'Stefan Beller'
Cc: git@vger.kernel.org, bmwill@google.com, gitster@pobox.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <20161115230651.23953-14-sbeller@google.com>
[I've reviewed up-to and including 13; I'll look at 14-16 tomorrow-ish]
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Tuesday, November 15, 2016 6:07 PM
> Cc: git@vger.kernel.org; bmwill@google.com; gitster@pobox.com;
> jrnieder@gmail.com; mogulguy10@gmail.com; David Turner; Stefan Beller
> Subject: [PATCH 13/16] submodule: teach unpack_trees() to update
> submodules
...
> msgs[ERROR_NOT_UPTODATE_DIR] =
> _("Updating the following directories would lose untracked
> files in it:\n%s");
> + msgs[ERROR_NOT_UPTODATE_SUBMODULE] =
> + _("Updating the following submodules would lose modifications
> in
> +it:\n%s");
s/it/them/
> if (!strcmp(cmd, "checkout"))
> msg = advice_commit_before_merge
> @@ -1315,19 +1320,18 @@ static int verify_uptodate_1(const struct
> cache_entry *ce,
> return 0;
>
> if (!lstat(ce->name, &st)) {
> - int flags =
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
> - unsigned changed = ie_match_stat(o->src_index, ce, &st,
> flags);
> - if (!changed)
> - return 0;
> - /*
> - * NEEDSWORK: the current default policy is to allow
> - * submodule to be out of sync wrt the superproject
> - * index. This needs to be tightened later for
> - * submodules that are marked to be automatically
> - * checked out.
> - */
> - if (S_ISGITLINK(ce->ce_mode))
> - return 0;
> + if (!S_ISGITLINK(ce->ce_mode)) {
I generally prefer to avoid if (!x) { A } else { B } -- I would rather just see if (x) { B } else { A }.
> + if (!changed) {
> + /* old is always a submodule */
> + if (S_ISGITLINK(new->ce_mode)) {
> + /*
> + * new is also a submodule, so check if we care
> + * and then if can checkout the new sha1 safely
> + */
> + if (submodule_is_interesting(old->name, null_sha1)
> + && is_submodule_checkout_safe(new->name, &new-
> >oid))
> + return 0;
> + } else {
> + /*
> + * new is not a submodule any more, so only
> + * care if we care:
> + */
> + if (submodule_is_interesting(old->name, null_sha1)
> + && ok_to_remove_submodule(old->name))
> + return 0;
> + }
Do we need a return 1 in here somewhere? Because otherwise, we fall through and return 0 later.
^ permalink raw reply
* Bug with disabling compression and 'binary' files.
From: Douglas Cox @ 2016-11-16 0:21 UTC (permalink / raw)
To: git
I was doing some experiments today with large-ish (100-200MB) binary
files and was trying to determine the best configuration for Git. Here
are the steps and timings I saw:
git init Test
cp .../largemovie.mp4 .
time git add largemovie.mp4
This took 6.5s for a 200MB file.
This file compressed a little bit, but not enough to matter, so I
wanted to see how much faster the add would be with compression
disabled. So I ran:
git config core.compression = 0
I then completely reran the test above starting with a clean
repository. This time the add took only 2.08s. I repeated these two
tests about 10 times using the same file each time to verify it wasn't
related to disk caching, etc.
At this point I decided that this was likely a good setting for this
repository, so I also created a .gitattributes file and added a few
entries often seen in suggestions for dealing with binary files:
*.mp4 binary -delta
The goal here was to disable any delta compression done during a
gc/pack and use the other settings 'binary' applies. Unfortunately
when I ran the test again (still using compression = 0), the test was
back to taking 6.5s and the file inside the .git/objects/ folder was
compressed again.
I narrowed this down to the '-text' attribute that is set when
specifying 'binary'. For some reason this flag is cancelling out the
core.compression = 0 setting and I think this is a bug?
Unfortunately core.compression = 0 is also global. Ideally it would be
great if there was a separate 'compression' attribute that could be
specified in .gitattributes per wildcard similar to how -delta can be
used. This way we would still be able to get compression for
text/source files, while still getting the speed of skipping
compression for binary files that do not compress well.
Has there been any discussion on having an attribute similar to this?
Thanks,
-Doug
^ permalink raw reply
* Re: [PATCH v15 01/27] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
From: Junio C Hamano @ 2016-11-16 0:18 UTC (permalink / raw)
To: Stephan Beyer
Cc: git, Pranit Bauva, Christian Couder, Matthieu Moy, Alex Henrie,
Antoine Delaite
In-Reply-To: <20ffa616-765d-ef73-4133-977561105eff@gmx.net>
Stephan Beyer <s-beyer@gmx.net> writes:
> Besides the things I'm mentioning in respective patch e-mails, I wonder
> why several bisect--helper commands are prefixed by "bisect"; I'm
> talking about:
>
> git bisect--helper --bisect-clean-state
>...
> git bisect--helper --bisect-start
> etc.
>
> instead of
>
> git bisect--helper --clean-state
>...
> git bisect--helper --start
> etc.
>
> Well, I know *why* they have these names: because the shell function
> names are simply reused. But I don't know why these prefixes are kept in
> the bisect--helper command options. On the other hand, these command
> names are not exposed to the user and may hence not be that important.(?)
That's a good point ;-)
These are not intended to be end-user entry points, so names that
are bit longer than necessary does not bother me too much.
Hopefully the longer-term endgame would be not to need a separate
"bisect-helper" binary at all but to have a "git bisect" binary
making these requests as subroutine calls, and at that point, the
names of the functions would want to have "bisect" prefix.
^ permalink raw reply
* RE: [PATCH 07/16] update submodules: introduce submodule_is_interesting
From: David Turner @ 2016-11-16 0:14 UTC (permalink / raw)
To: 'Stefan Beller'
Cc: git@vger.kernel.org, bmwill@google.com, gitster@pobox.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <20161115230651.23953-8-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Tuesday, November 15, 2016 6:07 PM
> Cc: git@vger.kernel.org; bmwill@google.com; gitster@pobox.com;
> jrnieder@gmail.com; mogulguy10@gmail.com; David Turner; Stefan Beller
> Subject: [PATCH 07/16] update submodules: introduce
> submodule_is_interesting
> +int submodule_is_interesting(const char *path, const unsigned char
> +*sha1) {
This is apparently only ever (in this series) called with null_sha1. So either this arg is unnecessary, or there are bugs elsewhere in the code.
^ permalink raw reply
* Re: [PATCH 11/16] teach unpack_trees() to remove submodule contents
From: Brandon Williams @ 2016-11-16 0:14 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jrnieder, mogulguy10, David.Turner
In-Reply-To: <20161115230651.23953-12-sbeller@google.com>
On 11/15, Stefan Beller wrote:
> Extend rmdir_or_warn() to remove the directories of those submodules which
> are scheduled for removal. Also teach verify_clean_submodule() to check
> that a submodule configured to be removed is not modified before scheduling
> it for removal.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> unpack-trees.c | 6 ++----
> wrapper.c | 4 ++++
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index ea6bdd2..576e1d5 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -9,6 +9,7 @@
> #include "refs.h"
> #include "attr.h"
> #include "split-index.h"
> +#include "submodule.h"
> #include "dir.h"
>
> /*
> @@ -1361,15 +1362,12 @@ static void invalidate_ce_path(const struct cache_entry *ce,
> /*
> * Check that checking out ce->sha1 in subdir ce->name is not
> * going to overwrite any working files.
> - *
> - * Currently, git does not checkout subprojects during a superproject
> - * checkout, so it is not going to overwrite anything.
> */
> static int verify_clean_submodule(const struct cache_entry *ce,
> enum unpack_trees_error_types error_type,
> struct unpack_trees_options *o)
> {
> - return 0;
> + return submodule_is_interesting(ce->name, null_sha1) && is_submodule_modified(ce->name, 0);
> }
So what does the return value from this function meant to mean? Is '1'
mean the submodule is clean while '0' indicates it is dirty or is it the
reverse of that? Reading this it seems to me a value of '1' means "yes
the submodule is clean!" but the way the return value is calculated
tells a different story. Either I'm understanding it incorrectly or I
think the return should be something like this:
return submodule_is_interesting(ce->name, null_sha1) && !is_submodule_modified(ce->name, 0);
Where we return '1' if the submodule is interesting and it hasn't been
modified.
>
> static int verify_clean_subdirectory(const struct cache_entry *ce,
> diff --git a/wrapper.c b/wrapper.c
> index e7f1979..17c08de 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -2,6 +2,7 @@
> * Various trivial helper wrappers around standard functions
> */
> #include "cache.h"
> +#include "submodule.h"
>
> static void do_nothing(size_t size)
> {
> @@ -592,6 +593,9 @@ int unlink_or_warn(const char *file)
>
> int rmdir_or_warn(const char *file)
> {
> + if (submodule_is_interesting(file, null_sha1)
> + && depopulate_submodule(file))
> + return -1;
> return warn_if_unremovable("rmdir", file, rmdir(file));
> }
It seems weird to me that rmdir is doing checks to see if the file being
removed is a submodule. Shouldn't those checks have occurred before
calling rmdir?
--
Brandon Williams
^ permalink raw reply
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