Re: Watchman/inotify support and other ways to speed up git status
On Wed, Oct 28, 2015 at 12:54 AM, David Turner wrote: > > On Thu, 2015-10-22 at 07:59 +0200, Christian Couder wrote: >> Hi everyone, >> >> I am starting to investigate ways to speed up git status and other git >> commands for Booking.com (thanks to AEvar) and I'd be happy to discuss >> the current status or be pointed to relevant documentation or mailing >> list threads. >> >> From the threads below ([0], [1], [2], [3], [4], [5], [6], [7], [8]) I >> understand that the status is roughly the following: >> >> - instead of working on inotify support it's better to work on using a >> cross platform tool like Watchman >> >> - instead of working on Watchman support it is better to work first on >> caching information in the index >> >> - git update-index --untracked-cache has been developed by Duy and >> others and merged to master in May 2015 to cache untracked status in >> the index; it is still considered experimental >> >> - git index-helper has been worked on by Duy but its status is not >> clear (at least to me) >> >> Is that correct? >> What are the possible/planned next steps in this area? improving > > We're using Watchman at Twitter. A week or two ago posted a dump of our > code to github, but I would advise waiting a day or two to use it, as > I'm about to pull a large number of bugfixes into it (I'll update this > thread and provide a link once I do so). Great, I will have a look at it then! > It's good, but it's not great. One major problem is a bug on OS X[1] > that causes missed updates. Another is that wide changes end up being > quite inefficient when querying watchman. This means that we do some > hackery to manually update the fs_cache during various large git > operations. > > I agree that in general it would be better to store or all some of this > information in the index, and the untracked-cache is a good step on > that. But with it enabled and watchman disabled, there still appears to > be 1 lstat per file (plus one stat per dir). The stats per-directory > alone are a large issue for Twitter because we have a relatively deep > and bushy directory structure (an average dir has about 3 or 4 entries > in it). As a result, git status with watchman is almost twice as fast > as with the untracked cache (on my particular machine). Thanks for this detailled description. > [1] https://github.com/facebook/watchman/issues/172 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git fsck failure on OS X with files >= 4 GiB
Awesome, building with NO_OPENSSL = 1 NO_GETTEXT = 1 produces a working git :-) Cheers, Rafael On 28 October 2015 at 23:37, Filipe Cabecinhas wrote: > I did some debugging, and it seems CC_SHA1_Update (used by > write_sha1_file_prepare if APPLE_COMMON_CRYPTO is defined in the Makefile) > takes a uint32_t as a "length" parameter, which explains why it stops > working at 4GiB (UINT_MAX+1). > > In the OS X 10.11 SDK header CommonCrypto/CommonDigest.h, we have: > > typedef uint32_t CC_LONG; /* 32 bit unsigned integer */ > //... > extern int CC_SHA1_Update(CC_SHA1_CTX *c, const void *data, CC_LONG len) > > A possible fix would be to either call SHA1_Update with a maximum of > UINT_MAX, looping if necessary. Or have a compatibility SHA1_Update for OS X > which can handle data longer than UINT_MAX. > > I'm not sure what the git maintainers would prefer. > > Regards, > > Filipe > > On Wed, Oct 28, 2015 at 4:10 PM, Rafael Espíndola > wrote: >> >> I first noticed this with "2.4.9 (Apple Git-60)", but it reproduces >> with git built from 37023ba381b6d251d7140a997b39b566dbc63c42. >> >> Create two files with just 0s: >> >> -rw-r--r-- 1 espindola staff 4294967296 28 Oct 11:09 exactly-4gib >> -rw-r--r-- 1 espindola staff 4294967295 28 Oct 11:09 one-less-than-4gib >> >> >> and run >> >> git init >> git add one-less-than-4gib >> git commit -m bar >> git fsck >> git add exactly-4gib >> git commit -m bar >> git fsck >> >> The first fsck will run with no problems, but the second one fails: >> >> error: packed cfdaf54c9ccfd8f5e4cee562f7d5f92df13d3106 from >> .git/objects/pack/pack-ff08480fd7f767b6bd0aeb559f0f5dea2245b0b3.pack >> is corrupt >> >> Using the very same revision on freebsd doesn't cause any errors. >> >> Cheers, >> Rafael > > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Reply
Hello My Name is Capt. Lucas Alves from the US Army base here in Damascus, Syria. I have a Deal of Sixteen Million Two Hundred Thousand United States I would like to partner with you. Kindly get back to me if it would be of interest to you for more details. Regards, Lucas Alves alvescaptlu...@gmail.com -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 0/8] Expose the submodule parallelism to the user
On 28/10/15 23:21, Stefan Beller wrote:
> This replaces origin/sb/submodule-parallel-update
> (anchoring at 74367d8938, Merge branch 'sb/submodule-parallel-fetch'
> into sb/submodule-parallel-update)
>
> What does it do?
> ---
> This series should finish the on going efforts of parallelizing
> submodule network traffic. The patches contain tests for clone,
> fetch and submodule update to use the actual parallelism both via
> command line as well as a configured option. I decided to go with
> "submodule.jobs" for all three for now.
>
> What is new in v2?
> ---
> * The patches got reordered slightly
> * Documentation was adapted
>
> Interdiff below
>
> Stefan Beller (8):
> run_processes_parallel: Add output to tracing messages
> submodule config: keep update strategy around
> submodule config: remove name_and_item_from_var
> submodule-config: parse_config
> fetching submodules: Respect `submodule.jobs` config option
> git submodule update: have a dedicated helper for cloning
> submodule update: expose parallelism to the user
> clone: allow an explicit argument for parallel submodule clones
>
> Documentation/config.txt| 7 ++
> Documentation/git-clone.txt | 6 +-
> Documentation/git-submodule.txt | 7 +-
> builtin/clone.c | 23 +++-
> builtin/fetch.c | 2 +-
> builtin/submodule--helper.c | 244
>
> git-submodule.sh| 54 -
> run-command.c | 4 +
> submodule-config.c | 98 ++--
> submodule-config.h | 3 +
> submodule.c | 5 +
> t/t5526-fetch-submodules.sh | 14 +++
> t/t7400-submodule-basic.sh | 4 +-
> t/t7406-submodule-update.sh | 27 +
> 14 files changed, 418 insertions(+), 80 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0de0138..785721a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2643,12 +2643,12 @@ submodule..ignore::
> "--ignore-submodules" option. The 'git submodule' commands are not
> affected by this setting.
>
> -submodule::jobs
> +submodule.jobs::
> This is used to determine how many submodules can be operated on in
> parallel. Specifying a positive integer allows up to that number
> - of submodules being fetched in parallel. Specifying 0 the number
> - of cpus will be taken as the maximum number. Currently this is
> - used in fetch and clone operations only.
> + of submodules being fetched in parallel. This is used in fetch
> + and clone operations only. A value of 0 will give some reasonable
> + default. The defaults may change with different versions of Git.
>
> tag.sort::
> This variable controls the sort ordering of tags when displayed by
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index affa52e..01bd6b7 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -216,9 +216,10 @@ objects from the source repository into a pack in the
> cloned repository.
> The result is Git repository can be separated from working
> tree.
>
> --j::
> ---jobs::
> +-j ::
> +--jobs ::
> The number of submodules fetched at the same time.
> + Defaults to the `submodule.jobs` option.
Hmm, is there a way to _not_ fetch in parallel (override the
config) from the command line for a given command?
ATB,
Ramsay Jones
>
> ::
> The (possibly remote) repository to clone from. See the
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index f5429fa..c70fafd 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -374,10 +374,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared`
> options carefully.
> clone with a history truncated to the specified number of revisions.
> See linkgit:git-clone[1]
>
> --j::
> ---jobs::
> +-j ::
> +--jobs ::
> This option is only valid for the update command.
> Clone new submodules in parallel with as many jobs.
> + Defaults to the `submodule.jobs` option.
>
> ...::
> Paths to submodule(s). When specified this will restrict the command
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 5ac2d89..22b9924 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -727,10 +727,7 @@ static int checkout(void)
> struct argv_array args = ARGV_ARRAY_INIT;
> argv_array_pushl(&args, "submodule", "update", "--init",
> "--recursive", NULL);
>
> - if (max_jobs == -1)
> - if (git_config_get_int("submodule.jobs", &max_jobs))
> - max_jobs = 1;
> - if (max_jobs != 1) {
> + if (max_jobs != -1) {
> struct strbuf sb = STRBUF_INIT;
> strbuf_addf(&sb, "--jobs=%d", max_
Re: [PATCH v3] ref-filter: fallback on alphabetical comparison
On Thu, Oct 29, 2015 at 3:46 AM, Junio C Hamano wrote: > Karthik Nayak writes: > >> On Wed, Oct 28, 2015 at 1:20 AM, Junio C Hamano wrote: Hence, fallback to alphabetical comparison based on the refname whenever the other criterion is equal. Fix the test in t3203 in this regard. >>> >>> It is unclear what "in this regard" is. Do you mean this (I am not >>> suggesting you to spell these out in a very detailed way in the >>> final log message; I am deliberately being detailed here to help me >>> understand what you really mean)? >>> >>> A test in t3203 was expecting that branch-two sorts before HEAD, >>> which happened to be how qsort(3) on Linux sorted the array, but >>> (1) that outcome was not even guaranteed, and (2) once we start >>> breaking ties with the refname, "HEAD" should sort before >>> "branch-two" so the original expectation was inconsistent with >>> the criterion we now use. >>> >> >> Exactly what you're saying, they happened to have the same objectsize. >> Hence sorting them would put them together, but since we compare the >> refname's the "HEAD" ref would come before "branch-two". >> >>> Update it to match the new world order, which we can now depend >>> on being stable. >>> >>> I am not sure about "HEAD" and "branch-two" in the above (it may be >>> comparison between "HEAD" and "refs/heads/branch-two", for example). >> >> It actually is, we consider "refs/heads/branch-two rather then the shortened >> version of this. It makes sense to classify refs this way, even though this >> was a side effect of this commit. > > Now these are enough bits of info, that can and needs to be > condenced into an updated log message to help future readers. > > Thanks. Will update and send, thanks :) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-cherry doesn't detect a "copied" commit
Hello Junio, On Sat, Oct 24, 2015 at 12:41 AM, Junio C Hamano wrote: > Francis Moreau writes: > >> I was mislead by the git-cherry manpage somehow which says: >> >> "git-cherry therefore >> detects when commits have been "copied" by means of git-cherry-pick(1), >> >> which is not exactly true. > > Yeah, I agree; the sentence is merely giving a description from > layperson's point of view, and it should have expressed it as such, > e.g. "roughly speaking, you can think of it like so", not sounding > as if it is giving a strictly correct and authoritative statement. > >> Would it make sense to add a "--fuzz" option which would reduce the >> diff context area used to generate the hash ? > > There could be situations where such fuzzing might be useful, but I > do not think this particular use case of yours is one of them. > > I'd imagine that you had two branches A (with "Unkown") and B (with > "Unknown"), and wanted to apply changes in them to your integration > branch (let's call that 'master'). You ask cherry "what commits in > A are missing in my 'master'?" and apply them. Next you ask cherry > "what commits in B are missing in my 'master' now?" and apply them. > > Because "Unkown" and "Unknown" are not considered the "same" patches > (one is most likely an update to the other), you get conflict when > applying the second copy, and that is how you can notice that one of > them is a stale and buggy one. If you haven't made your interim > integration result available to others after processing branch A, > you even have a chance to replace the "Unkown" one you already > applied with the corrected "Unknown" one before continuing. Even if > you choose not to bother and skip the "Unknown" one from branch B, > at least you know that in the end result you have a typo that would > eventually need to be fixed from "Unkown" into "Unknown". > > If you did a fuzzy version and ignored s/Unkown/Unknown/ typofix > between the "same" patches, you can avoid the conflict and all > patches from branch B may apply cleanly and automatically on top of > applying changes from branch A. But depending on the order you > processed A and B, you have a 50% chance of keeping the buggy > version without even realizing. > > So erring on the safe side and judging "Unkown" and "Unknown" are > different changes, inducing one extra conflict you had to look at, > is actively a good thing in this case. > In this case, ie where code modification happens, I agree that we should play in the safe side. But in my case I was more using git-cherry as a tool to help me compare my integration branch and the master one. I'm interested to know which commits have been picked up from upstream and which ones are specific to my branch. And when backporting (cherry-picking) commits from upstream, it's quite frequent that the context is slightly different. I think in this case and not the one you describe, such 'fuzz' option might make sense. Fuzzy match could be reported with a different tag, '~' for example. > One thing that helps to know while learning Git is that we try to > avoid being overly clever and outsmarting human users. Instead, we > err on the safe side to avoid silently doing a wrong thing. > > This is because a tool that automates 100% of cases with 2% chance > of producing wrong result cannot be trusted---you have to manually > inspect all 100% cases it automatically handled to make sure it did > the right thing. We instead automate 98% of simple cases where it > is obvious what the right result is, and ask the human user for help > on the remaining 2%. > > And this design principle is not limited to cherry. The design of > our merge algorithms is the same way, for example. I fully agree with this principle. Thanks -- Francis -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git fsck failure on OS X with files >= 4 GiB
Defining BLK_SHA1 = YesPlease (when calling make) should just change the SHA functions, instead of completely removing OpenSSL or CommonCrypto. Regards, Filipe On Thu, Oct 29, 2015 at 3:46 AM, Rafael Espíndola wrote: > Awesome, building with > > NO_OPENSSL = 1 > NO_GETTEXT = 1 > > produces a working git :-) > > Cheers, > Rafael > > > On 28 October 2015 at 23:37, Filipe Cabecinhas wrote: >> I did some debugging, and it seems CC_SHA1_Update (used by >> write_sha1_file_prepare if APPLE_COMMON_CRYPTO is defined in the Makefile) >> takes a uint32_t as a "length" parameter, which explains why it stops >> working at 4GiB (UINT_MAX+1). >> >> In the OS X 10.11 SDK header CommonCrypto/CommonDigest.h, we have: >> >> typedef uint32_t CC_LONG; /* 32 bit unsigned integer */ >> //... >> extern int CC_SHA1_Update(CC_SHA1_CTX *c, const void *data, CC_LONG len) >> >> A possible fix would be to either call SHA1_Update with a maximum of >> UINT_MAX, looping if necessary. Or have a compatibility SHA1_Update for OS X >> which can handle data longer than UINT_MAX. >> >> I'm not sure what the git maintainers would prefer. >> >> Regards, >> >> Filipe >> >> On Wed, Oct 28, 2015 at 4:10 PM, Rafael Espíndola >> wrote: >>> >>> I first noticed this with "2.4.9 (Apple Git-60)", but it reproduces >>> with git built from 37023ba381b6d251d7140a997b39b566dbc63c42. >>> >>> Create two files with just 0s: >>> >>> -rw-r--r-- 1 espindola staff 4294967296 28 Oct 11:09 exactly-4gib >>> -rw-r--r-- 1 espindola staff 4294967295 28 Oct 11:09 one-less-than-4gib >>> >>> >>> and run >>> >>> git init >>> git add one-less-than-4gib >>> git commit -m bar >>> git fsck >>> git add exactly-4gib >>> git commit -m bar >>> git fsck >>> >>> The first fsck will run with no problems, but the second one fails: >>> >>> error: packed cfdaf54c9ccfd8f5e4cee562f7d5f92df13d3106 from >>> .git/objects/pack/pack-ff08480fd7f767b6bd0aeb559f0f5dea2245b0b3.pack >>> is corrupt >>> >>> Using the very same revision on freebsd doesn't cause any errors. >>> >>> Cheers, >>> Rafael >> >> -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 0/8] Expose the submodule parallelism to the user
On Thu, Oct 29, 2015 at 6:19 AM, Ramsay Jones wrote: > Hmm, is there a way to _not_ fetch in parallel (override the > config) from the command line for a given command? > > ATB, > Ramsay Jones git config submodule.jobs 42 git --jobs 1 # should run just one task, despite having 42 configured It does use the parallel processing machinery though, but with a maximum of one subcommand being spawned. Is that what you're asking? Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git fsck failure on OS X with files >= 4 GiB
Here is a solution that avoids problems with OS-specific
implementations of SHA_Update() by limiting the size of each update
request to 1GiB and calling the function repeatedly in a loop.
Atousa
--
[PATCH] Limit the size of the data block passed to SHA1_Update()
This avoids issues where OS-specific implementations use
a 32-bit integer to specify block size. Limit currently
set to 1GiB.
---
cache.h | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/cache.h b/cache.h
index 79066e5..c305985 100644
--- a/cache.h
+++ b/cache.h
@@ -14,10 +14,28 @@
#ifndef git_SHA_CTX
#define git_SHA_CTX SHA_CTX
#define git_SHA1_Init SHA1_Init
-#define git_SHA1_Update SHA1_Update
#define git_SHA1_Final SHA1_Final
#endif
+#define SHA1_MAX_BLOCK_SIZE (1024*1024*1024)
+
+static inline int git_SHA1_Update(SHA_CTX *c, const void *data, size_t len)
+{
+ size_t nr;
+ size_t total = 0;
+ char *cdata = (char*)data;
+ while(len > 0) {
+ nr = len;
+ if(nr > SHA1_MAX_BLOCK_SIZE)
+ nr = SHA1_MAX_BLOCK_SIZE;
+ SHA1_Update(c, cdata, nr);
+ total += nr;
+ cdata += nr;
+ len -= nr;
+ }
+ return total;
+}
+
#include
typedef struct git_zstream {
z_stream z;
--
2.4.9 (Apple Git-60)
On Thu, Oct 29, 2015 at 8:15 AM, Filipe Cabecinhas wrote:
> Defining BLK_SHA1 = YesPlease (when calling make) should just change
> the SHA functions, instead of completely removing OpenSSL or
> CommonCrypto.
>
> Regards,
> Filipe
>
>
> On Thu, Oct 29, 2015 at 3:46 AM, Rafael Espíndola
> wrote:
>> Awesome, building with
>>
>> NO_OPENSSL = 1
>> NO_GETTEXT = 1
>>
>> produces a working git :-)
>>
>> Cheers,
>> Rafael
>>
>>
>> On 28 October 2015 at 23:37, Filipe Cabecinhas wrote:
>>> I did some debugging, and it seems CC_SHA1_Update (used by
>>> write_sha1_file_prepare if APPLE_COMMON_CRYPTO is defined in the Makefile)
>>> takes a uint32_t as a "length" parameter, which explains why it stops
>>> working at 4GiB (UINT_MAX+1).
>>>
>>> In the OS X 10.11 SDK header CommonCrypto/CommonDigest.h, we have:
>>>
>>> typedef uint32_t CC_LONG; /* 32 bit unsigned integer */
>>> //...
>>> extern int CC_SHA1_Update(CC_SHA1_CTX *c, const void *data, CC_LONG len)
>>>
>>> A possible fix would be to either call SHA1_Update with a maximum of
>>> UINT_MAX, looping if necessary. Or have a compatibility SHA1_Update for OS X
>>> which can handle data longer than UINT_MAX.
>>>
>>> I'm not sure what the git maintainers would prefer.
>>>
>>> Regards,
>>>
>>> Filipe
>>>
>>> On Wed, Oct 28, 2015 at 4:10 PM, Rafael Espíndola
>>> wrote:
I first noticed this with "2.4.9 (Apple Git-60)", but it reproduces
with git built from 37023ba381b6d251d7140a997b39b566dbc63c42.
Create two files with just 0s:
-rw-r--r-- 1 espindola staff 4294967296 28 Oct 11:09 exactly-4gib
-rw-r--r-- 1 espindola staff 4294967295 28 Oct 11:09 one-less-than-4gib
and run
git init
git add one-less-than-4gib
git commit -m bar
git fsck
git add exactly-4gib
git commit -m bar
git fsck
The first fsck will run with no problems, but the second one fails:
error: packed cfdaf54c9ccfd8f5e4cee562f7d5f92df13d3106 from
.git/objects/pack/pack-ff08480fd7f767b6bd0aeb559f0f5dea2245b0b3.pack
is corrupt
Using the very same revision on freebsd doesn't cause any errors.
Cheers,
Rafael
>>>
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Atousa Pahlevan, PhD
M.Math. University of Waterloo, Canada
Ph.D. Department of Computer Science, University of Victoria, Canada
Voice: 415-341-6206
Email: apahle...@ieee.org
Website: www.apahlevan.org
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb
Duy Nguyen writes: > On Mon, Oct 26, 2015 at 2:15 PM, Johannes Schindelin > wrote: >> When prefixing a Git call in the test suite with 'TEST_GDB_GIT=1 ', it >> will now be run with GDB, allowing the developer to debug test failures >> more conveniently. > > I'm very slowly catching up with git traffic. Apologies if it's > already mentioned elsewhere since I have only read this mail thread. > > Is it more convenient to add a sh function "gdb" instead? Changing a line of git invocation you want to debug from git frotz && to debug git frotz && indeed is slightly more pleasing to the eye than TEST_GDB_GIT=1 git frotz && I do not terribly care either way, as long as that feature is availble ;-) Either way these tweaks are temporary changes we make while figuring out where things go wrong, and from that point of view, (1) the longer and more cumbersome to type, the more cumbersome to use, but (2) the longer and more visually identifiable, the easier to spot in "diff" a tweak you forgot to revert before committing. > We can even go further with supporting gdbserver function, to launch > gdbserver, then I can debug from outside, works even without -v -i. Yes, that may be useful, but you can do so whether you use your shell function or TEST_GDB_GIT=1 that trigeers inside the "git" wrapper in bin-wrappers, I would think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git fsck failure on OS X with files >= 4 GiB
Atousa Duprat writes:
> [PATCH] Limit the size of the data block passed to SHA1_Update()
>
> This avoids issues where OS-specific implementations use
> a 32-bit integer to specify block size. Limit currently
> set to 1GiB.
> ---
> cache.h | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/cache.h b/cache.h
> index 79066e5..c305985 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -14,10 +14,28 @@
> #ifndef git_SHA_CTX
> #define git_SHA_CTX SHA_CTX
> #define git_SHA1_Init SHA1_Init
> -#define git_SHA1_Update SHA1_Update
> #define git_SHA1_Final SHA1_Final
> #endif
>
> +#define SHA1_MAX_BLOCK_SIZE (1024*1024*1024)
> +
> +static inline int git_SHA1_Update(SHA_CTX *c, const void *data, size_t len)
> +{
> + size_t nr;
> + size_t total = 0;
> + char *cdata = (char*)data;
> + while(len > 0) {
> + nr = len;
> + if(nr > SHA1_MAX_BLOCK_SIZE)
> + nr = SHA1_MAX_BLOCK_SIZE;
> + SHA1_Update(c, cdata, nr);
> + total += nr;
> + cdata += nr;
> + len -= nr;
> + }
> + return total;
> +}
> +
I think the idea illustrated above is a good start, but there are
a few issues:
* SHA1_Update() is used in fairly many places; it is unclear if it
is a good idea to inline.
* There is no need to punish implementations with working
SHA1_Update by another level of wrapping.
* What would you do when you find an implementation for which 1G is
still too big?
Perhaps something like this in the header
#ifdef SHA1_MAX_BLOCK_SIZE
extern int SHA1_Update_Chunked(SHA_CTX *, const void *, size_t);
#define git_SHA1_Update SHA1_Update_Chunked
#endif
with compat/sha1_chunked.c that has
#ifdef SHA1_MAX_BLOCK_SIZE
int SHA1_Update_Chunked(SHA_CTX *c, const void *data, size_t len)
{
... your looping implementation ...
}
#endif
in it, that is only triggered via a Makefile macro, e.g.
might be a good workaround.
diff --git a/Makefile b/Makefile
index 8466333..83348b8 100644
--- a/Makefile
+++ b/Makefile
@@ -139,6 +139,10 @@ all::
# Define PPC_SHA1 environment variable when running make to make use of
# a bundled SHA1 routine optimized for PowerPC.
#
+# Define SHA1_MAX_BLOCK_SIZE if your SSH1_Update() implementation can
+# hash only a limited amount of data in one call (e.g. APPLE_COMMON_CRYPTO
+# may want 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined).
+#
# Define NEEDS_CRYPTO_WITH_SSL if you need -lcrypto when using -lssl (Darwin).
#
# Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
@@ -1002,6 +1006,7 @@ ifeq ($(uname_S),Darwin)
ifndef NO_APPLE_COMMON_CRYPTO
APPLE_COMMON_CRYPTO = YesPlease
COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO
+ SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L
endif
NO_REGEX = YesPlease
PTHREAD_LIBS =
@@ -1350,6 +1355,11 @@ endif
endif
endif
+ifdef SHA1_MAX_BLOCK_SIZE
+LIB_OBJS += compat/sha1_chunked.o
+BASIC_CFLAGS += SHA1_MAX_BLOCK_SIZE="$(SHA1_MAX_BLOCK_SIZE)"
+endif
+
ifdef NO_PERL_MAKEMAKER
export NO_PERL_MAKEMAKER
endif
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 0/8] Expose the submodule parallelism to the user
Stefan Beller writes: > On Thu, Oct 29, 2015 at 6:19 AM, Ramsay Jones > wrote: > >> Hmm, is there a way to _not_ fetch in parallel (override the >> config) from the command line for a given command? >> >> ATB, >> Ramsay Jones > > git config submodule.jobs 42 > git --jobs 1 # should run just one task, despite having 42 configured > > It does use the parallel processing machinery though, but with a maximum of > one subcommand being spawned. Is that what you're asking? With this patch, do we still keep a separate machinery that bypasses the parallel thing altogether in the first place? I was hoping that the underlying parallel machinery is polished enough that using it with max=1 parallelism would be equivalent to serial execution. At least, that was my understanding of our goal, and back when we reviewed the previous "fetch --recurse-sub" series, my impression was we were already there. And in that ideal endgame world, your "Give '-j1' from the command line" would be perfectly an acceptable answer ;-). Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 0/8] Expose the submodule parallelism to the user
On Thu, Oct 29, 2015 at 10:23 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Thu, Oct 29, 2015 at 6:19 AM, Ramsay Jones >> wrote: >> >>> Hmm, is there a way to _not_ fetch in parallel (override the >>> config) from the command line for a given command? >>> >>> ATB, >>> Ramsay Jones >> >> git config submodule.jobs 42 >> git --jobs 1 # should run just one task, despite having 42 configured >> >> It does use the parallel processing machinery though, but with a maximum of >> one subcommand being spawned. Is that what you're asking? > > With this patch, do we still keep a separate machinery that bypasses > the parallel thing altogether in the first place? No. > > I was hoping that the underlying parallel machinery is polished > enough that using it with max=1 parallelism would be equivalent to > serial execution. There is no special code path for jobs=1. It should be pretty close, just with the overhead of the parallel engine spawning it one after the other and being an intermediate for output piping. The one subcommand would still output via a pipe to the parallel engine, which then outputs it immediately. > At least, that was my understanding of our goal, > and back when we reviewed the previous "fetch --recurse-sub" series, > my impression was we were already there. > > And in that ideal endgame world, your "Give '-j1' from the command > line" would be perfectly an acceptable answer ;-). ok. :) > > Thanks. > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-fetch pulls already-pulled objects?
Matt Glazar writes: > On a remote, I have two Git commit objects which point to the same tree > object (created with git commit-tree). What you are expecting _could_ be implemented by exchanging all tree and blob objects sending and receiving sides have and computing the set difference, but the sender and the receiver do not exchange such a huge list. The object transfer is done by first finding the common ancestor of histories of the sending and the receiving sides, which allows the sender to enumerate commits that the sender has but the receiver doesn't. From there, all objects [*1*] that are referenced by these commits that need to be sent. [Footnote] *1* There is an optimization to exclude the trees and blobs that can be cheaply proven to exist on the receiving end. If the receiving end has a commit that the sending end does *not* have, and that commit happens to record a tree the sending end needs to send, however, the sending end cannot prove that the tree does not have to be sent without first fetching that commit from the receiving end, which fails "can be cheaply proven to exist" test. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git-gui is still using old-style git-merge invocation
Performing a merge with git gui presents the following message in the merge result window: warning: old-style 'git merge HEAD ' is deprecated. Merge made by the 'recursive' strategy. a | 1 + 1 file changed, 1 insertion(+) create mode 100644 a But I am unable to find where the invocation happens. Can somebody help? -- Hannes PS: Reproducer: git init echo a > a git add a git commit -ma a git checkout -b side @~ echo b > b git add b git commit -mb git gui # to merge: Ctrl-M, Enter, Enter -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-gui is still using old-style git-merge invocation
On do, 2015-10-29 at 18:50 +0100, Johannes Sixt wrote: > Performing a merge with git gui presents the following message in the > merge result window: > > warning: old-style 'git merge HEAD ' is deprecated. > Merge made by the 'recursive' strategy. > a | 1 + > 1 file changed, 1 insertion(+) > create mode 100644 a > > But I am unable to find where the invocation happens. Can somebody > help? git-gui/lib/merge.tcl, method _start The command is constructed on lines 115-120 (master as of today, 37023ba3) -- Dennis Kaarsemaker www.kaarsemaker.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] difftool: avoid symlinks when reusing worktree files
David Aguilar writes: > Right. At first I thought I could revise the commit message to > make it clearer that we simply want to skip all symlinks, since > it never makes sense to reuse a worktree symlinks, but looking > at the tests and implementation makes me realize that it's not > that simple. > > This is going to take a bit more time to get right. John, I was > hoping you'd be able to take a look -- I'm playing catch-up too. > When it was first reported I let it sit for a while in hopes > that the original author would pickup the issue, but months > passed and I figured I'd take a stab at helping the user out. > > Anyways, it'll take me a bit more time to understand the code > and work out a sensible solution. My gut feeling is that we > should adjust the dir-diff feature so that it ignores all > symlinks. That seems like a simple answer since we're deciding > to skip that chunk of complexity. What dir-diff wants to do is to prepare two directory hierarchies on the filesystem and run "diff -r" (or an equivalent command of user's choice) on them, e.g. "diff -r left/ right/". "left/" tree is typically what you want to compare your working tree files agaist (e.g. a clean checkout of "the other version"), and "right/" tree is populated with either copies of the working tree or symbolic links. The copying to "right/" feels wasteful, but your working tree may be littered with build artifacts, and making a clean copy with only tracked files is one way to make sure that "diff -r" with a clean checout of "the other version" will not show them. In the loop that walks the @rawdiff array, there are a lot of "if we see a symbolic link on the left, do this" before the last one that says "if the working tree side is not $null (i.e. not missing), ask ut_wt_file()". That code remembers which path on either side had symbolic links. Later in the same function, there is this comment "Symbolic links require special treatment." The intent of the code here is that any path that involves a symbolic link should be tweaked there. The loop over %symlink expects left/ and right/ to be populated normally by the loop over @working_tree, and then for any path that is a symbolic link is replaced with a phony regular file (not a symbolic link) that says "Here is a symbolic link". So I think it is fine to return $use=0 for any symbolic link from use_wt_file. Anything you do there will be replaced by the loop over %symlink that appears later in the caller. The caller discards $wt_sha1 when $use=0 is returned, so the second return value does not matter. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-fetch pulls already-pulled objects?
> What you are expecting _could_ be implemented by exchanging all
> tree and blob objects sending and receiving sides have and computing
> the set difference, but the sender and the receiver do not exchange
> such a huge list.
In my case, I only want to exchange the tree object hash pointed directly
by the commit object; I don't care about all subtrees and blobs reachable
from the commit. I think a naive approach would only double the number of
hashes sent worst case.
Would negotiating the tree object hashes be possible on the client without
server changes? Is the protocol that flexible?
If what I want is *not* possible, is it possible to explicitly put a tree
(and its descendants) into its own pack? I think that will speed up the
git-fetch a bit by doing this on the server. (I know what trees/commits
will be sent ahead of time.) (The server does less work pulling the
objects out of an existing pack and repacking them for the client. (Or
maybe my mental model of git packs is wrong?))
> The object transfer is done by first finding the common ancestor of
> histories of the sending and the receiving sides, which allows the
> sender to enumerate commits that the sender has but the receiver
> doesn't. From there, all objects [*1*] that are referenced by these
> commits that need to be sent.
Thanks for clarifying.
> *1* There is an optimization to exclude the trees and blobs that can
> be cheaply proven to exist on the receiving end.
That makes sense (especially for 'git revert HEAD' situations).
Thank you for your reply, Junio.
-Original Message-
From: Junio C Hamano
Date: Thursday, October 29, 2015 at 10:32 AM
To: Matt Glazer
Cc: "git@vger.kernel.org"
Subject: Re: git-fetch pulls already-pulled objects?
>Matt Glazar writes:
>
>> On a remote, I have two Git commit objects which point to the same tree
>> object (created with git commit-tree).
>
>What you are expecting _could_ be implemented by exchanging all
>tree and blob objects sending and receiving sides have and computing
>the set difference, but the sender and the receiver do not exchange
>such a huge list.
>
>The object transfer is done by first finding the common ancestor of
>histories of the sending and the receiving sides, which allows the
>sender to enumerate commits that the sender has but the receiver
>doesn't. From there, all objects [*1*] that are referenced by these
>commits that need to be sent.
>
>
>[Footnote]
>
>*1* There is an optimization to exclude the trees and blobs that can
>be cheaply proven to exist on the receiving end. If the receiving
>end has a commit that the sending end does *not* have, and that
>commit happens to record a tree the sending end needs to send,
>however, the sending end cannot prove that the tree does not have to
>be sent without first fetching that commit from the receiving end,
>which fails "can be cheaply proven to exist" test.
>
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
Re: [PATCH] difftool: avoid symlinks when reusing worktree files
Junio C Hamano writes:
> So I think it is fine to return $use=0 for any symbolic link from
> use_wt_file. Anything you do there will be replaced by the loop
> over %symlink that appears later in the caller. The caller discards
> $wt_sha1 when $use=0 is returned, so the second return value does
> not matter.
So let me try to update your patch with the result of the study of
the codeflow.
-- >8 --
From: David Aguilar
Subject: difftool: ignore symbolic links in use_wt_file
The caller is preparing a narrowed-down copy of the working tree and
this function is asked if the path should be included in that copy.
If we say yes, the path from the working tree will be either symlinked
or copied into the narrowed-down copy.
For any path that is a symbolic link, the caller later fixes up the
narrowed-down copy by unlinking the path and replacing it with a
regular file it writes out that mimics the way how "git diff"
compares symbolic links.
Let's answer "no, you do not want to copy/symlink the working tree
file" for all symbolic links from this function, as we know the
result will not be used because it will be overwritten anyway.
Incidentally, this also stops the function from feeding a symbolic
link in the working tree to hash-object, which is a wrong thing to
do to begin with. The link may be pointing at a directory, or worse
may be dangling (both would be noticed as an error). Even if the
link points at a regular file, hashing the contents of a file that
is pointed at by the link is not correct (Git hashes the contents of
the link itself, not the pointee).
Signed-off-by: David Aguilar
Signed-off-by: Junio C Hamano
---
git-difftool.perl | 4 +---
t/t7800-difftool.sh | 19 +++
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 7df7c8a..488d14b 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -70,9 +70,7 @@ sub use_wt_file
my ($repo, $workdir, $file, $sha1) = @_;
my $null_sha1 = '0' x 40;
- if (! -e "$workdir/$file") {
- # If the file doesn't exist in the working tree, we cannot
- # use it.
+ if (-l "$workdir/$file" || ! -e _) {
return (0, $null_sha1);
}
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ea35a02..a771cf7 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -504,4 +504,23 @@ test_expect_success PERL 'difftool properly honors gitlink
and core.worktree' '
)
'
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
+ git init dirlinks &&
+ (
+ cd dirlinks &&
+ git config diff.tool checktrees &&
+ git config difftool.checktrees.cmd "echo good" &&
+ mkdir foo &&
+ : >foo/bar &&
+ git add foo/bar &&
+ test_commit symlink-one &&
+ ln -s foo link &&
+ git add link &&
+ test_commit symlink-two &&
+ echo good >expect &&
+ git difftool --tool=checktrees --dir-diff HEAD~ >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-fetch pulls already-pulled objects?
Matt Glazar writes:
> Would negotiating the tree object hashes be possible on the client without
> server changes? Is the protocol that flexible?
The protocol is strictly "find common ancestor in the commit
history". Everything else is done on the sender.
>>The object transfer is done by first finding the common ancestor of
>>histories of the sending and the receiving sides, which allows the
>>sender to enumerate commits that the sender has but the receiver
>>doesn't. From there, all objects [*1*] that are referenced by these
>>commits that need to be sent.
>>[Footnote]
>>
>>*1* There is an optimization to exclude the trees and blobs that can
>>be cheaply proven to exist on the receiving end. If the receiving
>>end has a commit that the sending end does *not* have, and that
>>commit happens to record a tree the sending end needs to send,
>>however, the sending end cannot prove that the tree does not have to
>>be sent without first fetching that commit from the receiving end,
>>which fails "can be cheaply proven to exist" test.
I forgot to mention the recent "pack bitmap" addition. It makes the
set of "can be cheaply proven to exist" a lot larger.
If for example the sender needs to send one commit C because it
determined that the receiver has history up to commit C~1, without
the bitmap, even when C^{tree} (i.e. the tree of C) is identical to
C~2^{tree} (i.e. the tree of C~2), it would have sent that tree
object because "proving that the receiver already has it" would
require the sender to dig its history back, starting from C~1
(i.e. the commit that is known to exist at the receiver), to
enumerate the objects contained in the common part of the history,
which fails the "can be cheaply proven to exist" test.
The "pack bitmap" pre-computes what commits, trees and blobs should
already exist in the repository given a commit for which bitmap
exists. Using the bitmap, from C~1 (i.e. the commit known to exist
at the receiving end), it can be proven cheaply that C^{tree} that
happens to be identical to C~2^{tree} already exists over there, and
the sender can use this knowledge to reduce the transfer.
The "pack bitmap" however does not change the fundamental structure.
If your receiver has a commit that is not known to the sender, and
that commit happens to record the same tree recorded in the commit
that needs to be sent, there is no way for the sender to know that
the receiver has it, exactly because the exchange between them is
purely "find common ancestor in history".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-gui is still using old-style git-merge invocation
Thanks! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-fetch pulls already-pulled objects?
> I forgot to mention the recent "pack bitmap" addition. It makes the
> set of "can be cheaply proven to exist" a lot larger.
Cool! I tried this feature, and it worked! (At least, it worked for my
small test case.)
I ran on the server (after pushing the objects):
git config repack.writeBitmaps true
git repack -Ad
After this, the 'git fetch origin master2' was super quick.
Thanks for your help!
Aside: This test case is using (normal, C/sh) Git. My production
environment uses JGit on the server. I haven't tested this with JGit.
-Original Message-
From: Junio C Hamano
Date: Thursday, October 29, 2015 at 11:42 AM
To: Matt Glazer
Cc: "git@vger.kernel.org"
Subject: Re: git-fetch pulls already-pulled objects?
>Matt Glazar writes:
>
>> Would negotiating the tree object hashes be possible on the client
>>without
>> server changes? Is the protocol that flexible?
>
>The protocol is strictly "find common ancestor in the commit
>history". Everything else is done on the sender.
>
>>>The object transfer is done by first finding the common ancestor of
>>>histories of the sending and the receiving sides, which allows the
>>>sender to enumerate commits that the sender has but the receiver
>>>doesn't. From there, all objects [*1*] that are referenced by these
>>>commits that need to be sent.
>
>>>[Footnote]
>>>
>>>*1* There is an optimization to exclude the trees and blobs that can
>>>be cheaply proven to exist on the receiving end. If the receiving
>>>end has a commit that the sending end does *not* have, and that
>>>commit happens to record a tree the sending end needs to send,
>>>however, the sending end cannot prove that the tree does not have to
>>>be sent without first fetching that commit from the receiving end,
>>>which fails "can be cheaply proven to exist" test.
>
>I forgot to mention the recent "pack bitmap" addition. It makes the
>set of "can be cheaply proven to exist" a lot larger.
>
>If for example the sender needs to send one commit C because it
>determined that the receiver has history up to commit C~1, without
>the bitmap, even when C^{tree} (i.e. the tree of C) is identical to
>C~2^{tree} (i.e. the tree of C~2), it would have sent that tree
>object because "proving that the receiver already has it" would
>require the sender to dig its history back, starting from C~1
>(i.e. the commit that is known to exist at the receiver), to
>enumerate the objects contained in the common part of the history,
>which fails the "can be cheaply proven to exist" test.
>
>The "pack bitmap" pre-computes what commits, trees and blobs should
>already exist in the repository given a commit for which bitmap
>exists. Using the bitmap, from C~1 (i.e. the commit known to exist
>at the receiving end), it can be proven cheaply that C^{tree} that
>happens to be identical to C~2^{tree} already exists over there, and
>the sender can use this knowledge to reduce the transfer.
>
>The "pack bitmap" however does not change the fundamental structure.
>If your receiver has a commit that is not known to the sender, and
>that commit happens to record the same tree recorded in the commit
>that needs to be sent, there is no way for the sender to know that
>the receiver has it, exactly because the exchange between them is
>purely "find common ancestor in history".
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
Re: [PATCHv2 0/8] Expose the submodule parallelism to the user
Stefan Beller writes: > This replaces origin/sb/submodule-parallel-update > (anchoring at 74367d8938, Merge branch 'sb/submodule-parallel-fetch' > into sb/submodule-parallel-update) > > What does it do? > --- > This series should finish the on going efforts of parallelizing > submodule network traffic. The patches contain tests for clone, > fetch and submodule update to use the actual parallelism both via > command line as well as a configured option. I decided to go with > "submodule.jobs" for all three for now. > > What is new in v2? > --- > * The patches got reordered slightly > * Documentation was adapted A couple of things I noticed (other than "many issues pointed out in v1 have been updated") are: - The way 7/8 and 8/8 checks for uninitialized max_jobs are inconsistently written. The way 7/8 does, i.e. (max_jobs < 0), looks more conventional. - "Defaults to the `submodule.jobs` option" should say "configuration variable" instead. I haven't formed an opinion on 6/8 yet. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] l10n: de.po: improve some translations
Signed-off-by: Ralf Thielow --- po/de.po | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/po/de.po b/po/de.po index c682aaf..be30642 100644 --- a/po/de.po +++ b/po/de.po @@ -592,7 +592,7 @@ msgstr "Fehler beim Schreiben der Signatur nach '%s': %s" #: grep.c:1718 #, c-format msgid "'%s': unable to read %s" -msgstr "'%s': konnte nicht lesen %s" +msgstr "'%s': konnte %s nicht lesen" #: grep.c:1735 #, c-format @@ -11749,7 +11749,7 @@ msgstr "unbekannte Option: $opt" #: git-stash.sh:397 msgid "No stash found." -msgstr "Kein \"stash\" gefunden." +msgstr "Kein Stash-Eintrag gefunden." #: git-stash.sh:404 #, sh-format @@ -11773,7 +11773,7 @@ msgstr "'$args' ist keine \"stash\"-Referenz" #: git-stash.sh:457 msgid "unable to refresh index" -msgstr "unfähig den Index zu aktualisieren" +msgstr "Konnte den Index nicht aktualisieren." #: git-stash.sh:461 msgid "Cannot apply a stash in the middle of a merge" -- 2.6.2.441.gf54246d -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] checkout: added two options to control progress output
On Sat, Oct 24, 2015 at 08:59:28AM -0600, Edmundo Carmona wrote: > From: Edmundo Carmona Antoranz > > checkout will refuse to print progress information if it's not connected > to a TTY. These patches will add two options: Not just checkout, but all of git's progress code. The usual rules are: - if the user told us --progress, show progress - if the user told us --no-progress, don't show progress - if neither is set, guess based on isatty(2) So with that in mind... > --progress-no-tty: enable printing progress info even if not using a TTY This should just be "--progress", shouldn't it? It looks like checkout does not understand --progress and --no-progress. It does have "--quiet", but elsewhere we usually use that to mean "no progress, but also no other informational messages". We should probably make this consistent with other commands. See how builtin/clone.c does this, for example. Note also that clone's "quiet" option is part of OPT__VERBOSITY(), which provides both "-q" and "-v" to turn the verbosity level up/down. We could switch checkout to use that, too, but I do not think it buys us anything, as we have no "-v" output for checkout yet. > --progress-lf: print progress information using LFs instead of CRs I notice this is part of your patch 1, but it really seems orthogonal to checkout's --progress option. It should probably be a separate patch, and it probably needs some justification in the commit message (i.e., explain not just _what_ you are doing in the patch, but _why_ it is useful). It also seems like this has nothing to do with checkout in particular. Should it be triggered by an environment variable or by an option to the main git binary? E.g.: git --progress-lf checkout ... to affect the progress meter for all commands? > Edmundo Carmona Antoranz (2): > checkout: progress on non-tty. progress with lf > checkout: adjust documentation to the two new options I mentioned above that the two orthogonal features should each get their own patches. I think you should also do the reverse with the documentation: include it along with the implementation of the feature. Sometimes we do documentation as a separate patch (especially if it is large, or if the feature itself took many patches to complete). But for a small feature, as a reviewer (and when looking back through history) I usually find it simpler if the documentation is included in the same commit. > Documentation/git-checkout.txt | 10 ++ > builtin/checkout.c | 12 ++-- > progress.c | 8 +++- > progress.h | 1 + > unpack-trees.c | 3 +++ > unpack-trees.h | 2 ++ > 6 files changed, 33 insertions(+), 3 deletions(-) I didn't look too carefully at the patches themselves, as they would need to be reworked substantially to follow the suggestions above. But I didn't notice any style or patch-formatting problems, and you seem to generally be touching the right areas for what you want to do. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 6/8] git submodule update: have a dedicated helper for cloning
Stefan Beller writes:
> +struct submodule_update_clone {
> + int count;
> + int quiet;
> + int print_unmatched;
> + char *reference;
> + char *depth;
> + char *update;
> + const char *recursive_prefix;
> + const char *prefix;
> + struct module_list list;
> + struct string_list projectlines;
> + struct pathspec pathspec;
> +};
These fields should be split into at least two classes, the ones
that are primarily the "configuration", and the others that are
"states". I am guessing 'quiet' is what the caller prepares and
tells the pp callbacks that they must work with reduced verbosity,
and 'print_unmatched' is also in the same boat. From the above
structure definition, nobody can guess what 'count' represents. Is
that the number of modules you have in the top-level superproject?
Is that the number of modules updated so far? Some other number?
We can guess "list" is probably the list of modules to be cloned or
updated, but we have no idea what "projectlines" mean and what it
will be used for. The only word with 'project' we would use in the
context of discussing submodules is the "top level superproject",
but then that will not need a "list", so that is not it. Perhaps
this refers to a list of projects bound to our tree as submodules,
and perhaps each such submodule gives some kind of "lines", but it
is totally unclear what kind of lines they use.
> +static void fill_clone_command(struct child_process *cp, int quiet,
> +const char *prefix, const char *path,
> +const char *name, const char *url,
> +const char *reference, const char *depth)
> +{
> + cp->git_cmd = 1;
> + cp->no_stdin = 1;
> + cp->stdout_to_stderr = 1;
> + cp->err = -1;
> + argv_array_push(&cp->args, "submodule--helper");
> + argv_array_push(&cp->args, "clone");
> + if (quiet)
> + argv_array_push(&cp->args, "--quiet");
> +
> + if (prefix) {
> + argv_array_push(&cp->args, "--prefix");
> + argv_array_push(&cp->args, prefix);
> + }
> + argv_array_push(&cp->args, "--path");
> + argv_array_push(&cp->args, path);
The pattern makes readers wish if there were a way to make these
pair of pushes easier to read. The best I can come up with is
argv_array_pushl(&cp->args, "--path", path, NULL);
While that would be already a vast improvement, when we know there
are many "I want to push two", it makes me wonder if I am entitled
to find the repeated ", NULL" irritating.
argv_array_push2(&cp->args, "--path", path);
on the hand feels slightly too specific. I dunno.
> +static int update_clone_get_next_task(void **pp_task_cb,
> + struct child_process *cp,
> + struct strbuf *err,
> + void *pp_cb)
> +{
> + struct submodule_update_clone *pp = pp_cb;
> +
> + for (; pp->count < pp->list.nr; pp->count++) {
> + const struct submodule *sub = NULL;
> + const char *displaypath = NULL;
> + const struct cache_entry *ce = pp->list.entries[pp->count];
> + struct strbuf sb = STRBUF_INIT;
> + const char *update_module = NULL;
> + char *url = NULL;
> + int just_cloned = 0;
> +
> + if (ce_stage(ce)) {
> + if (pp->recursive_prefix)
> + strbuf_addf(err, "Skipping unmerged submodule
> %s/%s\n",
> + pp->recursive_prefix, ce->name);
> + else
> + strbuf_addf(err, "Skipping unmerged submodule
> %s\n",
> + ce->name);
> + continue;
> + }
> +
> + sub = submodule_from_path(null_sha1, ce->name);
> + if (!sub) {
> + strbuf_addf(err, "BUG: internal error managing
> submodules. "
> + "The cache could not locate '%s'",
> ce->name);
> + pp->print_unmatched = 1;
> + return 0;
This feels a bit inconsistent. When the pp->count'th submodule is
set not to update (i.e. "none" below), you let this loop to ignore
that submodule and continue on to process pp->count+1'th one without
returning to the caller. Is there a reason why this case should be
processed differently? If the rest of the code treats this
condition as a "grave error" that tells the caller to never call
get-next again (i.e. the "emergency abort" condition), that sort of
makes sense, but I cannot offhand see if that is being done in this
patch.
> + }
> +
> + if (pp->recursive_prefix)
> + displaypath = relative_path(pp->recursive_prefix,
> ce->name, &sb);
> + else
> + displaypath = ce->name;
> +
> + if (pp->up
Re: [PATCHv2 0/8] Expose the submodule parallelism to the user
On 29/10/15 15:51, Stefan Beller wrote: > On Thu, Oct 29, 2015 at 6:19 AM, Ramsay Jones > wrote: > >> Hmm, is there a way to _not_ fetch in parallel (override the >> config) from the command line for a given command? >> >> ATB, >> Ramsay Jones > > git config submodule.jobs 42 > git --jobs 1 # should run just one task, despite having 42 configured Heh, yes ... I didn't pose the question quite right ... > > It does use the parallel processing machinery though, but with a maximum of > one subcommand being spawned. Is that what you're asking? ... but, despite that, you correctly inferred what I was really asking about! :) I was just wondering what overhead the parallel processing machinery adds to the original 'non-parallel' code path (for the j=1 case). I suspect the answer is 'not much', but that's just a guess. Have you measured it? What happens if there is only a single submodule to fetch? ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] checkout: added two options to control progress output
On Thu, Oct 29, 2015 at 4:05 PM, Jeff King wrote: > On Sat, Oct 24, 2015 at 08:59:28AM -0600, Edmundo Carmona wrote: > >> From: Edmundo Carmona Antoranz >> >> checkout will refuse to print progress information if it's not connected >> to a TTY. These patches will add two options: > > Not just checkout, but all of git's progress code. The usual rules are: > > - if the user told us --progress, show progress > > - if the user told us --no-progress, don't show progress > > - if neither is set, guess based on isatty(2) > > So with that in mind... > >> --progress-no-tty: enable printing progress info even if not using a TTY > > This should just be "--progress", shouldn't it? It sure does! A comment there: I think more builtins support --progress than the ones that support --no-progress, right? > > It looks like checkout does not understand --progress and --no-progress. > It does have "--quiet", but elsewhere we usually use that to mean "no > progress, but also no other informational messages". We should probably > make this consistent with other commands. See how builtin/clone.c does > this, for example. Note also that clone's "quiet" option is part of > OPT__VERBOSITY(), which provides both "-q" and "-v" to turn the > verbosity level up/down. We could switch checkout to use that, too, but > I do not think it buys us anything, as we have no "-v" output for > checkout yet. > >> --progress-lf: print progress information using LFs instead of CRs > > I notice this is part of your patch 1, but it really seems orthogonal to > checkout's --progress option. It should probably be a separate patch, > and it probably needs some justification in the commit message (i.e., > explain not just _what_ you are doing in the patch, but _why_ it is > useful). > > It also seems like this has nothing to do with checkout in particular. > Should it be triggered by an environment variable or by an option to the > main git binary? E.g.: > > git --progress-lf checkout ... > > to affect the progress meter for all commands? I think it would be worth it but, given that this is a more "global" adjustment ( as in, for the whole progress and not just checkout), I think it's better I separate it from the "--progress for checkout" patch cause right now checkout is missing the --progress option that many other builtins already support say, for standardization's sake. > >> Edmundo Carmona Antoranz (2): >> checkout: progress on non-tty. progress with lf >> checkout: adjust documentation to the two new options > > I mentioned above that the two orthogonal features should each get their > own patches. I think you should also do the reverse with the > documentation: include it along with the implementation of the feature. > Sometimes we do documentation as a separate patch (especially if it is > large, or if the feature itself took many patches to complete). But for > a small feature, as a reviewer (and when looking back through history) I > usually find it simpler if the documentation is included in the same > commit. > >> Documentation/git-checkout.txt | 10 ++ >> builtin/checkout.c | 12 ++-- >> progress.c | 8 +++- >> progress.h | 1 + >> unpack-trees.c | 3 +++ >> unpack-trees.h | 2 ++ >> 6 files changed, 33 insertions(+), 3 deletions(-) > > I didn't look too carefully at the patches themselves, as they would > need to be reworked substantially to follow the suggestions above. But I > didn't notice any style or patch-formatting problems, and you seem to > generally be touching the right areas for what you want to do. > > -Peff It's ok. I knew I would have to rework them based on feedback from the list. Thank you very much! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Tue, Oct 27, 2015 at 11:46:20PM -0400, Noam Postavsky wrote: > > I dunno. I think the auto-spawn is really what makes it usable; you can > > drop it in with "git config credential.helper config" and forget about > > it. Anything more fancy requires touching your login/startup files. > > Certainly I'm not opposed to people setting it up outside of the > > auto-spawn, but I wouldn't want that feature to go away. > > Perhaps we could express the auto-spawn more explicitly, something > like "git config credential.pre-helper start-cache-daemon". A way to > run a command before the credential helpers start would be useful to > our magit workaround for this issue (currently we start the daemon > before "push", "fetch", and "pull", but it won't work with user > aliases that run those commands). I'm not clear on when the pre-helper would be run. Git runs the helper when it needs a credential. What git command would start it? Or are you proposing a new credential interface for programs (like magit) to call along the lines of "do any prep work you need; I might be asking for a credential soon", without having to know the specifics of the user's helper config. You can do that already like: echo url=dummy://a:b@c/ | git credential approve though I guess for some helpers, that may actually store the bogus value (for the cache, it inserts a dummy cache entry, but that's not a problem; something with permanent storage would be more annoying). I guess the most elegant thing would be to add an "init" command to the helper interface. So magit would run: git credential init which would then see that we have "credential.helper" defined as "foo", and would run: git credential-foo init which could be a noop, start a daemon, or whatever, depending on how the helper operates. I dunno. It almost seems like adding a credentialcache.ignoreHUP option would be less hacky. :) > I'm not sure it's that widely used. Perhaps most people use ssh-agent? > That's what I do, though I've been experimenting with credential-cache > since it was brought up by a magit user. I don't know. Certainly up until a few years ago, anybody sane was using ssh-agent, because the http password stuff was so awful (no storage, and we didn't even respect 401s for a long time). But these days sites like GitHub push people into using https as the protocol of choice. Mostly, I think, because there was a lot of support load in teaching people to set up ssh. But I guess a lot of those people are on non-Linux platforms. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] checkout: added two options to control progress output
On Thu, Oct 29, 2015 at 06:09:06PM -0600, Edmundo Carmona Antoranz wrote:
> A comment there: I think more builtins support --progress than the ones that
> support --no-progress, right?
Hopefully they are supported equally everywhere. Anybody using parseopt
should have something like (this is from builtin/clone.c):
OPT_BOOL(0, "progress", &option_progress,
N_("force progress reporting")),
That automatically gives us both --progress and --no-progress. And note
how option_progress is initialized to "-1" above that, so we know the
resulting value it stores will either be 1 (--progress),
0 (--no-progress), or -1 (no option specified).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Thu, Oct 29, 2015 at 8:10 PM, Jeff King wrote: > On Tue, Oct 27, 2015 at 11:46:20PM -0400, Noam Postavsky wrote: >> Perhaps we could express the auto-spawn more explicitly, something >> like "git config credential.pre-helper start-cache-daemon". A way to >> run a command before the credential helpers start would be useful to >> our magit workaround for this issue (currently we start the daemon >> before "push", "fetch", and "pull", but it won't work with user >> aliases that run those commands). > > I'm not clear on when the pre-helper would be run. Git runs the helper > when it needs a credential. What git command would start it? I was just thinking in terms of our current workaround, it would have been helpful to be able to run a command just before the helpers are run. Or in other words, as the first helper. (doing "git -c credential.helper=foo" puts foo as the last helper). > I guess the most elegant thing would be to add an "init" command to the > helper interface. So magit would run: > > git credential init Although, we could use something like that too, as we're currently checking the helpers configured and then running git credential-cache--daemon directly. > I dunno. It almost seems like adding a credentialcache.ignoreHUP option > would be less hacky. :) The pre-helper thing is probably the best way to make the current hacks less hacky, but maybe not so great for actually fixing the problem and getting rid of the need for said hacks. :) > Mostly, I think, because there was a lot of support load in teaching > people to set up ssh. But I guess a lot of those people are on > non-Linux platforms. Hmm, the pre-helper thing would also help for the hack I wrote getting ssh-agent to autostart and work from Emacs on Windows (ssh-agent on Windows is a total PITA). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Thu, Oct 29, 2015 at 08:43:58PM -0400, Noam Postavsky wrote: > > I'm not clear on when the pre-helper would be run. Git runs the helper > > when it needs a credential. What git command would start it? > > I was just thinking in terms of our current workaround, it would have > been helpful to be able to run a command just before the helpers are > run. Or in other words, as the first helper. (doing "git -c > credential.helper=foo" puts foo as the last helper). If you know the helper you want to run, you are free to just run "git credential-foo". So I don't think that helps the inelegance of your workaround (the real inelegance is that you are assuming that "foo" needs run in the first place). I'm still not sure how the pre-helper would work. What git command kicks off the pre-helper command? Wouldn't it also be subject to the SIGHUP problem? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/8] run_processes_parallel: Add output to tracing messages
On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:
> run_processes_parallel: Add output to tracing messages
This doesn't really say much. I guess you mean that the intention is
to delimit a section in which output from various tasks may be
intermixed. Perhaps:
run_processes_parallel: delimit intermixed task output
or something.
> This commit serves 2 purposes. First this may help the user who
> tries to diagnose intermixed process calls. Second this may be used
> in a later patch for testing. As the output of a command should not
> change visibly except for going faster, grepping for the trace output
> seems like a viable testing strategy.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/run-command.c b/run-command.c
> index 82cc238..49dec74 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -959,6 +959,9 @@ static struct parallel_processes *pp_init(int n,
> n = online_cpus();
>
> pp->max_processes = n;
> +
> + trace_printf("run_processes_parallel: preparing to run up to %d
> children in parallel", n);
s/children/tasks/ maybe?
Minor: Perhaps drop "in parallel" since the parallelism is already
implied by the "run_processes_parallel" prefix.
> +
> pp->data = data;
> if (!get_next_task)
> die("BUG: you need to specify a get_next_task function");
> @@ -988,6 +991,7 @@ static void pp_cleanup(struct parallel_processes *pp)
> {
> int i;
>
> + trace_printf("run_processes_parallel: parallel processing done");
Minor: Likewise, perhaps just "done" rather than "parallel processing
done" since the "run_processes_parallel" prefix already implies
parallelism.
> for (i = 0; i < pp->max_processes; i++) {
> strbuf_release(&pp->children[i].err);
> child_process_deinit(&pp->children[i].process);
> --
> 2.5.0.281.g4ed9cdb
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 2/8] submodule config: keep update strategy around
On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:
> We need the submodule update strategies in a later patch.
>
> Signed-off-by: Stefan Beller
> Signed-off-by: Junio C Hamano
> ---
> diff --git a/submodule-config.c b/submodule-config.c
> index afe0ea8..8b8c7d1 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -311,6 +312,16 @@ static int parse_config(const char *var, const char
> *value, void *data)
> free((void *) submodule->url);
> submodule->url = xstrdup(value);
> }
> + } else if (!strcmp(item.buf, "update")) {
> + if (!value)
> + ret = config_error_nonbool(var);
> + else if (!me->overwrite && submodule->update != NULL)
Although "foo != NULL" is unusual in this code-base, it is used
elsewhere in this file, including just outside the context seen above.
Okay.
> + warn_multiple_config(me->commit_sha1, submodule->name,
> +"update");
> + else {
> + free((void *)submodule->update);
Minor: Every other 'free((void *) foo)' in this file has a space after
"(void *)", one of which can be seen in the context just above.
> + submodule->update = xstrdup(value);
> + }
> }
>
> strbuf_release(&name);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Thu, Oct 29, 2015 at 8:50 PM, Jeff King wrote: > workaround (the real inelegance is that you are assuming that "foo" > needs run in the first place). Well, we currently check the output from "git config credential.helpers" to determine what's needed, so the inelegance here is that we reimplement git's checking of this option. > I'm still not sure how the pre-helper would work. What git command kicks > off the pre-helper command? Wouldn't it also be subject to the SIGHUP > problem? Ah, maybe the missing piece I forgot to mention is that we could make our pre/1st-helper be an emacsclient command, which would tell Emacs to startup the daemon. So the daemon would be a subprocess of Emacs, not "git push", thereby avoiding the SIGHUP. In our current workaround we startup the daemon (if it's not running) before git commands that we think are going to run credential helpers (i.e. "push", "pull", "fetch"), hence my thought that it would be nicer if we only did that before git is *actually* going to run the helpers. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 3/8] submodule config: remove name_and_item_from_var
On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:
> submodule config: remove name_and_item_from_var
>
> By inlining `name_and_item_from_var` it is easy to add later options
> which are not required to have a submodule name.
I guess you're trying to say that name_and_item_from_var() didn't
provide a proper abstraction, thus wasn't as useful as expected.
Perhaps that commit message could make this shortcoming clearer.
> Signed-off-by: Stefan Beller
> ---
> diff --git a/submodule-config.c b/submodule-config.c
> index 8b8c7d1..4d0563c 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -251,18 +235,25 @@ static int parse_config(const char *var, const char
> *value, void *data)
> {
> struct parse_config_parameter *me = data;
> struct submodule *submodule;
> - struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
> - int ret = 0;
> + int subsection_len, ret = 0;
> + const char *subsection, *key;
> + char *name;
>
> - /* this also ensures that we only parse submodule entries */
> - if (!name_and_item_from_var(var, &name, &item))
> + if (parse_config_key(var, "submodule", &subsection,
> +&subsection_len, &key) < 0)
> return 0;
>
> + if (!subsection_len)
> + return 0;
Alternately:
if (parse_config_key(var, "submodule", &subsection,
&subsection_len, &key) < 0 || !subsection_len)
return 0;
> +
> + /* subsection is not null terminated */
> + name = xmemdupz(subsection, subsection_len);
> submodule = lookup_or_create_by_name(me->cache,
> me->gitmodules_sha1,
> -name.buf);
> +name);
> + free(name);
Since this is all private to submodule-config.c, I wonder if it would
be cleaner to change lookup_or_create_by_name() to accept a
name_length argument?
> - if (!strcmp(item.buf, "path")) {
> + if (!strcmp(key, "path")) {
> if (!value)
> ret = config_error_nonbool(var);
> else if (!me->overwrite && submodule->path != NULL)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] checkout: add --progress option
Under normal circumstances, and like other git commands,
git checkout will write progress info to stderr if
attached to a terminal. This option allows progress
to be forced even if not using a terminal. Also,
progress can be skipped if using option --no-progress.
Signed-off-by: Edmundo Carmona Antoranz
---
Documentation/git-checkout.txt | 6 ++
builtin/checkout.c | 17 -
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index e269fb1..93ba35a 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -107,6 +107,12 @@ OPTIONS
--quiet::
Quiet, suppress feedback messages.
+--progress::
+ Progress status is reported on the standard error stream
+ by default when it is attached to a terminal, unless -q
+ is specified. This flag forces progress status even if the
+ standard error stream is not directed to a terminal.
+
-f::
--force::
When switching branches, proceed even if the index or the
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bc703c0..e28c36b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -27,6 +27,8 @@ static const char * const checkout_usage[] = {
NULL,
};
+static int option_progress = -1;
+
struct checkout_opts {
int patch_mode;
int quiet;
@@ -417,7 +419,19 @@ static int reset_tree(struct tree *tree, const struct
checkout_opts *o,
opts.reset = 1;
opts.merge = 1;
opts.fn = oneway_merge;
- opts.verbose_update = !o->quiet && isatty(2);
+ /**
+* Rules to display progress:
+* -q is selected
+* no verbiage
+* -q is _not_ selected and --no-progress _is_ selected,
+* progress will be skipped
+* -q is _not_ selected and --progress _is_ selected,
+* progress will be printed to stderr
+* -q is _not_ selected and --progress is 'undefined'
+* progress will be printed to stderr _if_ working on a terminal
+*/
+ opts.verbose_update = !o->quiet && (option_progress > 0 ||
+ (option_progress < 0 && isatty(2)));
opts.src_index = &the_index;
opts.dst_index = &the_index;
parse_tree(tree);
@@ -1156,6 +1170,7 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
N_("second guess 'git checkout
'")),
OPT_BOOL(0, "ignore-other-worktrees",
&opts.ignore_other_worktrees,
N_("do not check if another worktree is holding the
given ref")),
+ OPT_BOOL(0, "progress", &option_progress, N_("force progress
reporting")),
OPT_END(),
};
--
2.6.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/8] submodule-config: parse_config
On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:
> submodule-config: parse_config
Um, what?
> This rewrites parse_config to distinguish between configs specific to
> one submodule and configs which apply generically to all submodules.
> We do not have generic submodule configs yet, but the next patch will
> introduce "submodule.jobs".
>
> Signed-off-by: Stefan Beller
>
> # Conflicts:
> # submodule-config.c
Interesting.
> Signed-off-by: Stefan Beller
> ---
> diff --git a/submodule-config.c b/submodule-config.c
> index 4d0563c..1cea404 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -231,27 +231,23 @@ struct parse_config_parameter {
> int overwrite;
> };
>
> -static int parse_config(const char *var, const char *value, void *data)
> +static int parse_generic_submodule_config(const char *var,
> + const char *key,
> + const char *value)
> {
> - struct parse_config_parameter *me = data;
> - struct submodule *submodule;
> - int subsection_len, ret = 0;
> - const char *subsection, *key;
> - char *name;
> -
> - if (parse_config_key(var, "submodule", &subsection,
> -&subsection_len, &key) < 0)
> - return 0;
> -
> - if (!subsection_len)
> - return 0;
> + return 0;
> +}
>
> - /* subsection is not null terminated */
> - name = xmemdupz(subsection, subsection_len);
> - submodule = lookup_or_create_by_name(me->cache,
> -me->gitmodules_sha1,
> -name);
> - free(name);
> +static int parse_specific_submodule_config(struct parse_config_parameter *me,
> + const char *name,
> + const char *key,
> + const char *value,
> + const char *var)
Minor: Are these 'key', 'value', 'var' arguments analogous to the
like-named arguments of parse_generic_submodule_config()? If so, why
is the order of arguments different?
> +{
> + int ret = 0;
> + struct submodule *submodule = lookup_or_create_by_name(me->cache,
> +
> me->gitmodules_sha1,
> + name);
>
> if (!strcmp(key, "path")) {
> if (!value)
> @@ -318,6 +314,30 @@ static int parse_config(const char *var, const char
> *value, void *data)
> return ret;
> }
>
> +static int parse_config(const char *var, const char *value, void *data)
> +{
> + struct parse_config_parameter *me = data;
> +
> + int subsection_len;
> + const char *subsection, *key;
> + char *name;
> +
> + if (parse_config_key(var, "submodule", &subsection,
> +&subsection_len, &key) < 0)
> + return 0;
> +
> + if (!subsection_len)
> + return parse_generic_submodule_config(var, key, value);
> + else {
> + int ret;
> + /* subsection is not null terminated */
> + name = xmemdupz(subsection, subsection_len);
> + ret = parse_specific_submodule_config(me, name, key, value,
> var);
> + free(name);
> + return ret;
> + }
> +}
Minor: You could drop the 'else' and outdent its body, thus losing one
indentation level.
if (!subsection_len)
return parse_generic_submodule_config(...);
int ret;
...
return ret;
This might give you a less noisy diff and would be a bit more
consistent with the early part of the function where you don't bother
giving the if (parse_config_key(...)) an 'else' body.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 5/8] fetching submodules: Respect `submodule.jobs` config option
On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller wrote:
> This allows to configure fetching and updating in parallel
> without having the command line option.
>
> This moved the responsibility to determine how many parallel processes
> to start from builtin/fetch to submodule.c as we need a way to communicate
> "The user did not specify the number of parallel processes in the command
> line options" in the builtin fetch. The submodule code takes care of
> the precedence (CLI > config > default)
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 391a0c3..785721a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2643,6 +2643,13 @@ submodule..ignore::
> "--ignore-submodules" option. The 'git submodule' commands are not
> affected by this setting.
>
> +submodule.jobs::
> + This is used to determine how many submodules can be operated on in
> + parallel. Specifying a positive integer allows up to that number
> + of submodules being fetched in parallel. This is used in fetch
> + and clone operations only. A value of 0 will give some reasonable
> + default. The defaults may change with different versions of Git.
I'm not sure that "default" is the correct word here. When you talk
about a "default", you're normally explaining what happens when the
configuration is not provided. (In fact, the default number of jobs is
1, which you may want to document here).
> tag.sort::
> This variable controls the sort ordering of tags when displayed by
> linkgit:git-tag[1]. Without the "--sort=" option provided, the
> diff --git a/submodule-config.c b/submodule-config.c
> index 1cea404..07bdcdf 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -32,6 +32,7 @@ enum lookup_type {
>
> static struct submodule_cache cache;
> static int is_cache_init;
> +static int parallel_jobs = -1;
>
> static int config_path_cmp(const struct submodule_entry *a,
>const struct submodule_entry *b,
> @@ -235,6 +236,9 @@ static int parse_generic_submodule_config(const char *var,
> const char *key,
> const char *value)
> {
> + if (!strcmp(key, "jobs")) {
> + parallel_jobs = strtol(value, NULL, 10);
> + }
Style: unnecessary braces
Why does this allow a negative value? The documentation doesn't
mention anything about it.
> return 0;
> }
>
> diff --git a/submodule.c b/submodule.c
> index 0257ea3..188ba02 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -752,6 +752,11 @@ int fetch_populated_submodules(const struct argv_array
> *options,
> argv_array_push(&spf.args, "--recurse-submodules-default");
> /* default value, "--submodule-prefix" and its value are added later
> */
>
> + if (max_parallel_jobs < 0)
> + max_parallel_jobs = config_parallel_submodules();
> + if (max_parallel_jobs < 0)
> + max_parallel_jobs = 1;
run_process_parallel() itself specially handles max_parallel_jobs==0,
so you don't need to consider it here. Okay.
> +
> calculate_changed_submodule_paths();
> run_processes_parallel(max_parallel_jobs,
>get_next_submodule,
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 1b4ce69..5c3579c 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -470,4 +470,18 @@ test_expect_success "don't fetch submodule when newly
> recorded commits are alrea
> test_i18ncmp expect.err actual.err
> '
>
> +test_expect_success 'fetching submodules respects parallel settings' '
> + git config fetch.recurseSubmodules true &&
> + (
> + cd downstream &&
> + GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&
> + grep "7 children" trace.out &&
> + git config submodule.jobs 8 &&
> + GIT_TRACE=$(pwd)/trace.out git fetch &&
> + grep "8 children" trace.out &&
> + GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
> + grep "9 children" trace.out
> + )
> +'
Not specifically related to this test, but maybe add tests to check
cases when --jobs is not specified, and --jobs=1?
> +
> test_done
> --
> 2.5.0.281.g4ed9cdb
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git fsck failure on OS X with files >= 4 GiB
Thank you for the feedback. I have revised the proposed patch as
suggested, allowing the use of SHA1_MAX_BLOCK_SIZE to enable the
chunked implementation. When building for OSX with the CommonCrypto
library we error out if SHA1_MAX_BLOCK_SIZE is not defined, which will
avoid compiling a version of the tool that won't compute hashes
properly on large files. It should be easy to enable this on other
platforms if needed.
Atousa
On Thu, Oct 29, 2015 at 10:19 AM, Junio C Hamano wrote:
> Atousa Duprat writes:
>
>> [PATCH] Limit the size of the data block passed to SHA1_Update()
>>
>> This avoids issues where OS-specific implementations use
>> a 32-bit integer to specify block size. Limit currently
>> set to 1GiB.
>> ---
>> cache.h | 20 +++-
>> 1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/cache.h b/cache.h
>> index 79066e5..c305985 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -14,10 +14,28 @@
>> #ifndef git_SHA_CTX
>> #define git_SHA_CTX SHA_CTX
>> #define git_SHA1_Init SHA1_Init
>> -#define git_SHA1_Update SHA1_Update
>> #define git_SHA1_Final SHA1_Final
>> #endif
>>
>> +#define SHA1_MAX_BLOCK_SIZE (1024*1024*1024)
>> +
>> +static inline int git_SHA1_Update(SHA_CTX *c, const void *data, size_t len)
>> +{
>> + size_t nr;
>> + size_t total = 0;
>> + char *cdata = (char*)data;
>> + while(len > 0) {
>> + nr = len;
>> + if(nr > SHA1_MAX_BLOCK_SIZE)
>> + nr = SHA1_MAX_BLOCK_SIZE;
>> + SHA1_Update(c, cdata, nr);
>> + total += nr;
>> + cdata += nr;
>> + len -= nr;
>> + }
>> + return total;
>> +}
>> +
>
> I think the idea illustrated above is a good start, but there are
> a few issues:
>
> * SHA1_Update() is used in fairly many places; it is unclear if it
>is a good idea to inline.
>
> * There is no need to punish implementations with working
>SHA1_Update by another level of wrapping.
>
> * What would you do when you find an implementation for which 1G is
>still too big?
>
> Perhaps something like this in the header
>
> #ifdef SHA1_MAX_BLOCK_SIZE
> extern int SHA1_Update_Chunked(SHA_CTX *, const void *, size_t);
> #define git_SHA1_Update SHA1_Update_Chunked
> #endif
>
> with compat/sha1_chunked.c that has
>
> #ifdef SHA1_MAX_BLOCK_SIZE
> int SHA1_Update_Chunked(SHA_CTX *c, const void *data, size_t len)
> {
> ... your looping implementation ...
> }
> #endif
>
> in it, that is only triggered via a Makefile macro, e.g.
> might be a good workaround.
>
> diff --git a/Makefile b/Makefile
> index 8466333..83348b8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -139,6 +139,10 @@ all::
> # Define PPC_SHA1 environment variable when running make to make use of
> # a bundled SHA1 routine optimized for PowerPC.
> #
> +# Define SHA1_MAX_BLOCK_SIZE if your SSH1_Update() implementation can
> +# hash only a limited amount of data in one call (e.g. APPLE_COMMON_CRYPTO
> +# may want 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined).
> +#
> # Define NEEDS_CRYPTO_WITH_SSL if you need -lcrypto when using -lssl
> (Darwin).
> #
> # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto
> (Darwin).
> @@ -1002,6 +1006,7 @@ ifeq ($(uname_S),Darwin)
> ifndef NO_APPLE_COMMON_CRYPTO
> APPLE_COMMON_CRYPTO = YesPlease
> COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO
> + SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L
> endif
> NO_REGEX = YesPlease
> PTHREAD_LIBS =
> @@ -1350,6 +1355,11 @@ endif
> endif
> endif
>
> +ifdef SHA1_MAX_BLOCK_SIZE
> +LIB_OBJS += compat/sha1_chunked.o
> +BASIC_CFLAGS += SHA1_MAX_BLOCK_SIZE="$(SHA1_MAX_BLOCK_SIZE)"
> +endif
> +
> ifdef NO_PERL_MAKEMAKER
> export NO_PERL_MAKEMAKER
> endif
--
Atousa Pahlevan, PhD
M.Math. University of Waterloo, Canada
Ph.D. Department of Computer Science, University of Victoria, Canada
Voice: 415-341-6206
Email: apahle...@ieee.org
Website: www.apahlevan.org
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/3] blame: extract find_single_final
Signed-off-by: Max Kirillov
---
builtin/blame.c | 29 +
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 295ce92..38f6267 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2401,16 +2401,11 @@ static struct commit *fake_working_tree_commit(struct
diff_options *opt,
return commit;
}
-static char *prepare_final(struct scoreboard *sb)
+static struct object_array_entry *find_single_final(struct rev_info *revs)
{
int i;
- const char *final_commit_name = NULL;
- struct rev_info *revs = sb->revs;
+ struct object_array_entry *found = NULL;
- /*
-* There must be one and only one positive commit in the
-* revs->pending array.
-*/
for (i = 0; i < revs->pending.nr; i++) {
struct object *obj = revs->pending.objects[i].item;
if (obj->flags & UNINTERESTING)
@@ -2419,14 +2414,24 @@ static char *prepare_final(struct scoreboard *sb)
obj = deref_tag(obj, NULL, 0);
if (obj->type != OBJ_COMMIT)
die("Non commit %s?", revs->pending.objects[i].name);
- if (sb->final)
+ if (found)
die("More than one commit to dig from %s and %s?",
revs->pending.objects[i].name,
- final_commit_name);
- sb->final = (struct commit *) obj;
- final_commit_name = revs->pending.objects[i].name;
+ found->name);
+ found = &(revs->pending.objects[i]);
+ }
+ return found;
+}
+
+static char *prepare_final(struct scoreboard *sb)
+{
+ struct object_array_entry *found = find_single_final(sb->revs);
+ if (found) {
+ sb->final = (struct commit *) found->item;
+ return xstrdup(found->name);
+ } else {
+ return NULL;
}
- return xstrdup_or_null(final_commit_name);
}
static char *prepare_initial(struct scoreboard *sb)
--
2.3.4.2801.g3d0809b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/3] blame: test to describe use of blame --reverse --first-parent
Reverse blame can be used to locate removal of lines which does not change adjacent lines. Such edits do not appear in non-reverse blame, because the adjacent lines last changed commit is older history, before the edit. For a big and active project which uses topic branches, or analogous feature, for example pull-requests, the history can contain many concurrent branches, and even after an edit merged into the target branch, there are still many (sometimes several tens or even hundreds) topic branch which do not contain it: a0--a1-*a2-*a3-a4...-*a100 |\ / / / | b0-B1..bN / / |\ / / | c0.. ..cN / \/ z0....zN Here, the '*'s mark the first parent in merge, and uppercase B1 - the commit where the line being blamed for was removed. Since commits cN-zN do not contain the B1, the still have the line removed in B1, and reverce blame can report that the last commit for the line was zN (meaning that it was removed in a100). In fact it really does return some very late commit, and this makes it unusable for finding the B1 commit. The search could be done by blame --reverse --first-parent. For range a0..a100 it would return a1, and then only one additional blame along the a0..bN will return the desired commit b0. But combining --reverse and --first-parent was forbidden in 95a4fb0eac, because incorrectly specified range could produce unexpected and meaningless result. Add test which describes the expected behavior of `blame --reverse --first-parent` in the case described above. Signed-off-by: Max Kirillov --- t/t8009-blame-vs-topicbranches.sh | 36 1 file changed, 36 insertions(+) create mode 100755 t/t8009-blame-vs-topicbranches.sh diff --git a/t/t8009-blame-vs-topicbranches.sh b/t/t8009-blame-vs-topicbranches.sh new file mode 100755 index 000..175ad37 --- /dev/null +++ b/t/t8009-blame-vs-topicbranches.sh @@ -0,0 +1,36 @@ +#!/bin/sh + +test_description='blaming trough history with topic branches' +. ./test-lib.sh + +# Creates the history shown below. '*'s mark the first parent in the merges. +# The only line of file.t is changed in commit B2 +# +#+---C1 +# / \ +# A0--A1--*A2--*A3 +# \ / +#B1-B2 +# +test_expect_success setup ' + test_commit A0 file.t line0 && + test_commit A1 && + git reset --hard A0 && + test_commit B1 && + test_commit B2 file.t line0changed && + git reset --hard A1 && + test_merge A2 B2 && + git reset --hard A1 && + test_commit C1 && + git reset --hard A2 && + test_merge A3 C1 + ' + +test_expect_failure 'blame --reverse --first-parent finds A1' ' + git blame --porcelain --reverse --first-parent A0..A3 -- file.t >actual_full && + head -n 1 actual && + git rev-parse A1 >expect && + test_cmp expect actual + ' + +test_done -- 2.3.4.2801.g3d0809b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/3] blame: allow blame --reverse --first-parent when it makes sense
Allow combining --reverse and --first-parent if initial commit of
specified range is at the first-parent chain starting from the final
commit. Disable the prepare_revision_walk()'s builtin children
collection, instead picking only the ones which are along the first
parent chain.
Signed-off-by: Max Kirillov
---
builtin/blame.c | 32 ++--
t/t8009-blame-vs-topicbranches.sh | 2 +-
2 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 38f6267..98b1810 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2507,6 +2507,7 @@ int cmd_blame(int argc, const char **argv, const char
*prefix)
long dashdash_pos, lno;
char *final_commit_name = NULL;
enum object_type type;
+ struct commit *final_commit = NULL;
static struct string_list range_list;
static int output_option = 0, opt = 0;
@@ -2697,11 +2698,11 @@ parse_done:
}
else if (contents_from)
die("--contents and --children do not blend well.");
- else if (revs.first_parent_only)
- die("combining --first-parent and --reverse is not supported");
else {
final_commit_name = prepare_initial(&sb);
sb.commits.compare = compare_commits_by_reverse_commit_date;
+ if (revs.first_parent_only)
+ revs.children.name = NULL;
}
if (!sb.final) {
@@ -2718,6 +2719,14 @@ parse_done:
else if (contents_from)
die("Cannot use --contents with final commit object name");
+ if (reverse && revs.first_parent_only) {
+ struct object_array_entry *entry = find_single_final(sb.revs);
+ if (!entry)
+ die("--reverse and --first-parent together require
specified latest commit");
+ else
+ final_commit = (struct commit*) entry->item;
+ }
+
/*
* If we have bottom, this will mark the ancestors of the
* bottom commits we would reach while traversing as
@@ -2726,6 +2735,25 @@ parse_done:
if (prepare_revision_walk(&revs))
die(_("revision walk setup failed"));
+ if (reverse && revs.first_parent_only) {
+ struct commit *c = final_commit;
+
+ sb.revs->children.name = "children";
+ while (c->parents &&
+ hashcmp(c->object.sha1, sb.final->object.sha1)) {
+ struct commit_list *l = xcalloc(1, sizeof(*l));
+
+ l->item = c;
+ if (add_decoration(&sb.revs->children,
+ &c->parents->item->object, l))
+ die("BUG: not unique item in first-parent
chain");
+ c = c->parents->item;
+ }
+
+ if (hashcmp(c->object.sha1, sb.final->object.sha1))
+ die("--reverse --first-parent together require range
along first-parent chain");
+ }
+
if (is_null_sha1(sb.final->object.sha1)) {
o = sb.final->util;
sb.final_buf = xmemdupz(o->file.ptr, o->file.size);
diff --git a/t/t8009-blame-vs-topicbranches.sh
b/t/t8009-blame-vs-topicbranches.sh
index 175ad37..72596e3 100755
--- a/t/t8009-blame-vs-topicbranches.sh
+++ b/t/t8009-blame-vs-topicbranches.sh
@@ -26,7 +26,7 @@ test_expect_success setup '
test_merge A3 C1
'
-test_expect_failure 'blame --reverse --first-parent finds A1' '
+test_expect_success 'blame --reverse --first-parent finds A1' '
git blame --porcelain --reverse --first-parent A0..A3 -- file.t
>actual_full &&
head -n 1 actual &&
git rev-parse A1 >expect &&
--
2.3.4.2801.g3d0809b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/3] blame: allow blame --reverse --first-parent when it makes sense
Update the test: * Fix style notes in tests * Remove the non-first-parent case, because it's more like fature request, and is not fixed in this batch * Rewrite the commit message, hopely now it answers better to "why" Max Kirillov (3): blame: add test to describe use of blame --reverse --first-parent blame: extract find_single_final blame: allow blame --reverse --first-parent when it makes sense builtin/blame.c | 61 ++- t/t8009-blame-vs-topicbranches.sh | 36 +++ 2 files changed, 83 insertions(+), 14 deletions(-) create mode 100755 t/t8009-blame-vs-topicbranches.sh -- 2.3.4.2801.g3d0809b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
