Re: [PATCH] clone: fix colliding file detection on APFS
On 20/11/2018 16:28, Nguyễn Thái Ngọc Duy wrote:
> Commit b878579ae7 (clone: report duplicate entries on case-insensitive
> filesystems - 2018-08-17) adds a warning to user when cloning a repo
> with case-sensitive file names on a case-insensitive file system. The
> "find duplicate file" check was doing by comparing inode number (and
> only fall back to fspathcmp() when inode is known to be unreliable
> because fspathcmp() can't cover all case folding cases).
>
> The inode check is very simple, and wrong. It compares between a
> 32-bit number (sd_ino) and potentially a 64-bit number (st_ino). When
> an inode is larger than 2^32 (which seems to be the case for APFS), it
> will be truncated and stored in sd_ino, but comparing with itself will
> fail.
>
> As a result, instead of showing a pair of files that have the same
> name, we show just one file (marked before the beginning of the
> loop). We fail to find the original one.
>
> The fix could be just a simple type cast (*)
>
> dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino
>
> but this is no longer a reliable test, there are 4G possible inodes
> that can match sd_ino because we only match the lower 32 bits instead
> of full 64 bits.
>
> There are two options to go. Either we ignore inode and go with
> fspathcmp() on Apple platform. This means we can't do accurate inode
> check on HFS anymore, or even on APFS when inode numbers are still
> below 2^32.
>
> Or we just to to reduce the odds of matching a wrong file by checking
> more attributes, counting mostly on st_size because st_xtime is likely
> the same. This patch goes with this direction, hoping that false
> positive chances are too small to be seen in practice.
>
> While at there, enable the test on Cygwin (verified working by Ramsay
> Jones)
Well, no, I tested the previous version of this patch. However, this
patch also passes the test. (Note _test_ singular - in order to check
that this patch doesn't cause a regression I would need to run the
whole test-suite - that takes 3.5 hours, if I'm not doing anything
else!)
>
> (*) this is also already done inside match_stat_data()
>
> Reported-by: Carlo Arenas
> Helped-by: Ramsay Jones
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> So I'm going with match_stat_data(). But I don't know, perhaps just
> ignoring inode (like Carlo's original patch) is safer/better?
>
> Tested on case-insensitive JFS on Linux. But I don't think it really
> matters because I'm not even sure if I could push inode above 2^32
> with this. Hacking JFS for this test sounds fun, but no time for that.
>
> entry.c | 4 ++--
> t/t5601-clone.sh | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 5d136c5d55..0a3c451f5f 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -404,7 +404,7 @@ static void mark_colliding_entries(const struct checkout
> *state,
> {
> int i, trust_ino = check_stat;
>
> -#if defined(GIT_WINDOWS_NATIVE)
> +#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
I was a little curious about this (but couldn't be bothered actually
read the code, post-application), so I removed this hunk from the
patch, rebuilt and ran the test again: it _passed_ the test. :-D
So, ...
ATB,
Ramsay Jones
> trust_ino = 0;
> #endif
>
> @@ -419,7 +419,7 @@ static void mark_colliding_entries(const struct checkout
> *state,
> if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
> continue;
>
> - if ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||
> + if ((trust_ino && !match_stat_data(>ce_stat_data, st)) ||
> (!trust_ino && !fspathcmp(ce->name, dup->name))) {
> dup->ce_flags |= CE_MATCHED;
> break;
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index f1a49e94f5..c28d51bd59 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '
> )
> '
>
> -test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file
> detection' '
> +test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '
> grep X icasefs/warning &&
> grep x icasefs/warning &&
> test_i18ngrep "the following paths have collided" icasefs/warning
>
Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems
On 19/11/2018 23:29, Ramsay Jones wrote: > > > On 19/11/2018 21:03, Duy Nguyen wrote: >> First of all, Ramsay, it would be great if you could test the below >> patch and see if it works on Cygwin. I assume since Cygwin shares the >> underlying filesystem, it will share the same "no trusting inode" >> issue with native builds (or it calculates inodes anyway using some >> other source?). > > Hmm, I have no idea why you would like me to try this patch - care > to explain? [I just saw, "Has this been tested on cygwin?" and, since > it has been happily passing for some time, responded yes!] > > Just for the giggles, I removed the !CYGWIN prerequisite from the > test and when, as expected, the test failed, had a look around: > > $ pwd > /home/ramsay/git/t/trash directory.t5601-clone > $ cat icasefs/warning > Cloning into 'bogus'... > done. > warning: the following paths have collided (e.g. case-sensitive paths > on a case-insensitive filesystem) and only one from the same > colliding group is in the working tree: > > 'x' > $ cd icasefs/bogus > $ ls -l > total 0 > -rw-r--r-- 1 ramsay None 0 Nov 19 22:40 x > $ git ls-files --debug > ignoring EOIE extension > X > ctime: 1542667201:664036600 > mtime: 1542667201:663055400 > dev: 2378432ino: 324352 > uid: 1001 gid: 513 > size: 0 flags: 0 > x > ctime: 1542667201:665026800 > mtime: 1542667201:665026800 > dev: 2378432ino: 324352 > uid: 1001 gid: 513 > size: 0 flags: 0 > $ > > So, both X and x are in the index with the same inode number. > > Does that help? Well, I haven't even looked at the patch, but when I apply it to the current 'pu' branch (just what I happened to have checked out) and run that one test: $ ./t5601-clone.sh ... ok 96 - shallow clone locally ok 97 - GIT_TRACE_PACKFILE produces a usable pack ok 98 - clone on case-insensitive fs ok 99 - colliding file detection ok 100 - partial clone ok 101 - partial clone: warn if server does not support object filtering ok 102 - batch missing blob request during checkout ok 103 - batch missing blob request does not inadvertently try to fetch gitlinks # passed all 103 test(s) # SKIP no web server found at '/usr/sbin/apache2' 1..103 $ ... the colliding file detection test passes! ATB, Ramsay Jones
Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems
On 19/11/2018 21:03, Duy Nguyen wrote: > First of all, Ramsay, it would be great if you could test the below > patch and see if it works on Cygwin. I assume since Cygwin shares the > underlying filesystem, it will share the same "no trusting inode" > issue with native builds (or it calculates inodes anyway using some > other source?). Hmm, I have no idea why you would like me to try this patch - care to explain? [I just saw, "Has this been tested on cygwin?" and, since it has been happily passing for some time, responded yes!] Just for the giggles, I removed the !CYGWIN prerequisite from the test and when, as expected, the test failed, had a look around: $ pwd /home/ramsay/git/t/trash directory.t5601-clone $ cat icasefs/warning Cloning into 'bogus'... done. warning: the following paths have collided (e.g. case-sensitive paths on a case-insensitive filesystem) and only one from the same colliding group is in the working tree: 'x' $ cd icasefs/bogus $ ls -l total 0 -rw-r--r-- 1 ramsay None 0 Nov 19 22:40 x $ git ls-files --debug ignoring EOIE extension X ctime: 1542667201:664036600 mtime: 1542667201:663055400 dev: 2378432 ino: 324352 uid: 1001 gid: 513 size: 0 flags: 0 x ctime: 1542667201:665026800 mtime: 1542667201:665026800 dev: 2378432 ino: 324352 uid: 1001 gid: 513 size: 0 flags: 0 $ So, both X and x are in the index with the same inode number. Does that help? ATB, Ramsay Jones
Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems
On 19/11/2018 12:28, Torsten Bögershausen wrote:
> On 2018-11-19 09:20, Carlo Marcelo Arenas Belón wrote:
>> While I don't have an HFS+ volume to test, I suspect this patch should be
>> needed for both, even if I have to say thay even the broken output was
>> better than the current state.
>>
>> Travis seems to be using a case sensitive filesystem so wouldn't catch this.
>>
>> Was windows/cygwin tested?
>>
>> Carlo
>> -- >8 --
>> Subject: [PATCH] entry: fix t5061 on macOS
>>
>> b878579ae7 ("clone: report duplicate entries on case-insensitive
>> filesystems",
>> 2018-08-17) was tested on Linux with an excemption for Windows that needs
>> to be expanded for macOS (using APFS), which then would show :
>>
>> $ git clone git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
>> warning: the following paths have collided (e.g. case-sensitive paths
>> on a case-insensitive filesystem) and only one from the same
>> colliding group is in the working tree:
>>
>> 'man2/_Exit.2'
>> 'man2/_exit.2'
>> 'man3/NAN.3'
>> 'man3/nan.3'
>>
>> Signed-off-by: Carlo Marcelo Arenas Belón
>> ---
>> entry.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/entry.c b/entry.c
>> index 5d136c5d55..3845f570f7 100644
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -404,7 +404,7 @@ static void mark_colliding_entries(const struct checkout
>> *state,
>> {
>> int i, trust_ino = check_stat;
>>
>> -#if defined(GIT_WINDOWS_NATIVE)
>> +#if defined(GIT_WINDOWS_NATIVE) || defined(__APPLE__)
>> trust_ino = 0;
>> #endif
>>
>>
>
> Sorry,
> but I can't reproduce your problem here.
>
> Did you test it on Mac ?
> If I run t5601 on a case sensitive files system
> (Mac, mounted NFS, exported from Linux)
> I get:
> ok 99 # skip colliding file detection (missing CASE_INSENSITIVE_FS of
> !MINGW,!CYGWIN,CASE_INSENSITIVE_FS)
I tested v2.20.0-rc0 on cygwin last night and it passed just fine.
I just ran t5601-clone.sh on its own and got:
$ ./t5601-clone.sh
...
ok 98 - clone on case-insensitive fs
ok 99 # skip colliding file detection (missing !CYGWIN of
!MINGW,!CYGWIN,CASE_INSENSITIVE_FS)
ok 100 - partial clone
ok 101 - partial clone: warn if server does not support object filtering
ok 102 - batch missing blob request during checkout
ok 103 - batch missing blob request does not inadvertently try to fetch
gitlinks
# passed all 103 test(s)
# SKIP no web server found at '/usr/sbin/apache2'
1..103
$
ATB,
Ramsay Jones
Re: [PATCH v5 02/12] sha1-file: provide functions to look up hash algorithms
On 14/11/2018 02:11, brian m. carlson wrote:
> On Wed, Nov 14, 2018 at 12:11:07AM +0000, Ramsay Jones wrote:
>>
>>
>> On 13/11/2018 18:42, Derrick Stolee wrote:
>>> On 11/4/2018 6:44 PM, brian m. carlson wrote:
>>>> +int hash_algo_by_name(const char *name)
>>>> +{
>>>> + int i;
>>>> + if (!name)
>>>> + return GIT_HASH_UNKNOWN;
>>>> + for (i = 1; i < GIT_HASH_NALGOS; i++)
>>>> + if (!strcmp(name, hash_algos[i].name))
>>>> + return i;
>>>> + return GIT_HASH_UNKNOWN;
>>>> +}
>>>> +
>>>
>>> Today's test coverage report [1] shows this method is not covered in the
>>> test suite. Looking at 'pu', it doesn't have any callers.
>>>
>>> Do you have a work in progress series that will use this? Could we add a
>>> test-tool to exercise this somehow?
>>
>> There are actually 4 unused external symbols resulting from Brian's
>> 'bc/sha-256' branch. The new unused externals in 'pu' looks like:
>>
>> $ diff nsc psc
>> 37a38,39
>> > hex.o - hash_to_hex
>
> I have code that uses this in my object-id-part15 series. I also have
> another series coming after this one that makes heavy use of it.
>
>> > hex.o - hash_to_hex_algop_r
>
> I believe this is because it's inline, since it is indeed used just a
> few lines below its definition. I'll drop the inline, since it's meant
> to be externally visible.
No, this has nothing to do with the 'inline', it is simply not
called outside of hex.c (at present). If you look at the assembler
(objdump -d hex.o), you will find practically all of the function
calls in that file are inlined (even those not marked with 'inline').
[I think the external declaration in cache.h forces the compiler to
add the external definition, despite the 'inline'. If you remove the
'inline' and re-compile and disassemble again, the result is identical.]
Thanks for confirming upcoming patches will add uses for all of
these functions - I suspected that would be the case.
Thanks!
ATB,
Ramsay Jones
>
>> > sha1-file.o- hash_algo_by_id
>
> This will be used when I write pack index v3, which will be in my
> object-id-part15 series.
>
>> > sha1-file.o- hash_algo_by_name
>
> This is used in my object-id-part15 series.
>
Re: [PATCH v5 02/12] sha1-file: provide functions to look up hash algorithms
On 14/11/2018 00:11, Ramsay Jones wrote:
>
>
> On 13/11/2018 18:42, Derrick Stolee wrote:
>> On 11/4/2018 6:44 PM, brian m. carlson wrote:
>>> +int hash_algo_by_name(const char *name)
>>> +{
>>> + int i;
>>> + if (!name)
>>> + return GIT_HASH_UNKNOWN;
>>> + for (i = 1; i < GIT_HASH_NALGOS; i++)
>>> + if (!strcmp(name, hash_algos[i].name))
>>> + return i;
>>> + return GIT_HASH_UNKNOWN;
>>> +}
>>> +
>>
>> Today's test coverage report [1] shows this method is not covered in the
>> test suite. Looking at 'pu', it doesn't have any callers.
>>
>> Do you have a work in progress series that will use this? Could we add a
>> test-tool to exercise this somehow?
>
> There are actually 4 unused external symbols resulting from Brian's
> 'bc/sha-256' branch. The new unused externals in 'pu' looks like:
>
> $ diff nsc psc
> 37a38,39
> > hex.o - hash_to_hex
> > hex.o - hash_to_hex_algop_r
> 48a51
> > parse-options.o - optname
> 71a75
> > sha1-file.o - for_each_file_in_obj_subdir
> 72a77,78
> > sha1-file.o - hash_algo_by_id
> > sha1-file.o - hash_algo_by_name
> $
>
> The symbols from hex.o and sha1-file.o being the 4 symbols from
> this branch.
>
> I suspect that upcoming patches will make use of them. ;-)
BTW, if you were puzzling over the 3rd symbol from sha1-file.o
(which I wasn't counting in the 4 symbols above! ;-) ), then I
believe that is because Jeff's commit 3a2e08245c ("object-store:
provide helpers for loose_objects_cache", 2018-11-12) effectively
moved the only call outside of sha1-file.c (in sha1-name.c) back
into sha1-file.c
So, for_each_file_in_obj_subdir() could now be marked 'static'.
(whether it should is a different issue).
ATB,
Ramsay Jones
Re: [PATCH v5 02/12] sha1-file: provide functions to look up hash algorithms
On 13/11/2018 18:42, Derrick Stolee wrote:
> On 11/4/2018 6:44 PM, brian m. carlson wrote:
>> +int hash_algo_by_name(const char *name)
>> +{
>> + int i;
>> + if (!name)
>> + return GIT_HASH_UNKNOWN;
>> + for (i = 1; i < GIT_HASH_NALGOS; i++)
>> + if (!strcmp(name, hash_algos[i].name))
>> + return i;
>> + return GIT_HASH_UNKNOWN;
>> +}
>> +
>
> Today's test coverage report [1] shows this method is not covered in the test
> suite. Looking at 'pu', it doesn't have any callers.
>
> Do you have a work in progress series that will use this? Could we add a
> test-tool to exercise this somehow?
There are actually 4 unused external symbols resulting from Brian's
'bc/sha-256' branch. The new unused externals in 'pu' looks like:
$ diff nsc psc
37a38,39
> hex.o - hash_to_hex
> hex.o - hash_to_hex_algop_r
48a51
> parse-options.o - optname
71a75
> sha1-file.o - for_each_file_in_obj_subdir
72a77,78
> sha1-file.o - hash_algo_by_id
> sha1-file.o - hash_algo_by_name
$
The symbols from hex.o and sha1-file.o being the 4 symbols from
this branch.
I suspect that upcoming patches will make use of them. ;-)
ATB,
Ramsay Jones
Re: [PATCH 3/9] rename "alternate_object_database" to "object_directory"
On 12/11/2018 15:36, Jeff King wrote: > On Mon, Nov 12, 2018 at 10:30:55AM -0500, Derrick Stolee wrote: > >> On 11/12/2018 9:48 AM, Jeff King wrote: >>> In preparation for unifying the handling of alt odb's and the normal >>> repo object directory, let's use a more neutral name. This patch is >>> purely mechanical, swapping the type name, and converting any variables >>> named "alt" to "odb". There should be no functional change, but it will >>> reduce the noise in subsequent diffs. >>> >>> Signed-off-by: Jeff King >>> --- >>> I waffled on calling this object_database instead of object_directory. >>> But really, it is very specifically about the directory (packed >>> storage, including packs from alternates, is handled elsewhere). >> >> That makes sense. Each alternate makes its own object directory, but is part >> of a larger object database. It also helps clarify a difference from the >> object_store. >> >> My only complaint is that you have a lot of variable names with "odb" which >> are now object_directory pointers. Perhaps "odb" -> "objdir"? Or is that >> just too much change? > > Yeah, that was part of my waffling. ;) > >>From my conversions, usually "objdir" is a string holding the pathname, > though that's not set in stone. I also like that "odb" is the same short > length as "alt", which helps with conversion. While reading the patch, I keep thinking it should be 'obd' for OBject Directory. ;-) [Given my track record in naming things, please take with a _huge_ pinch of salt!] ATB, Ramsay Jones
Re: [PATCH v2 12/16] parse-options: replace opterror() with optname()
On 10/11/2018 04:55, Duy Nguyen wrote: > On Tue, Nov 6, 2018 at 3:07 PM Ramsay Jones > wrote: >> Also, this patch does not replace opterror() calls outside of >> the 'parse-options.c' file with optname(). This tickles my >> static-check.pl script, since optname() is an external function >> which is only called from 'parse-options.c'. >> >> So, at present, optname() could be marked as a local 'static' >> symbol. However, I could also imagine it being used by new callers >> outside of 'parse-options.c' in the future. (maybe) Your call. ;-) > > I was making it static, but the compiler complained about undefined > function and I would need to either move optname() up in > parse-options.c or add a forward declaration (but I was going to > _remove_ the declaration!) > > Since it could be potentially used by Jeff's series (and I think it > does have potential in parse-options-cb.c), I'll just leave it > exported and caress your static-check.pl script Fair enough. >(how did it not catch > optbug() not being used outside parse-options.c either)? It did, some time ago (presumably, I haven't checked). Which is to say: the output from the master branch notes it: $ grep parse-options sc parse-options.o - optbug $ ... but if it gets to the master branch I tend to forget it. (I have been meaning to go through the 'sc' file and clean out some of the 'easy' cases). So, if optname() doesn't get any new callers, I will watch it go from 'pu' to 'next' and then to 'master' and ... ;-) ATB, Ramsay Jones
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
On 07/11/2018 11:19, Johannes Schindelin wrote: [snip] >>> Hmm, this doesn't quite fit with the intended use of this >>> function! ;-) (even on windows!) >>> >>> I haven't looked very deeply, but doesn't this affect all >>> absolute paths in the config read by git_config_pathname(), >>> along with all 'included config' files? >> >> I think so. I have not thought things through to say if replacing a >> "full path in the current drive" with system_path() is a sensible >> thing to do in the first place, but I am getting the impression from >> review comments that it probably is not. >> >>> I am pretty sure that I would not want the absolute paths >>> in my config file(s) magically 'moved' depending on whether >>> git has been compiled with 'runtime prefix' support or not! > > The cute thing is: your absolute paths would not be moved because we are > talking about Windows. Therefore your absolute paths would not start with > a forward slash. Ah, sorry, I must have misunderstood a comment in your cover letter: The reason is this: something like this (make paths specified e.g. via http.sslCAInfo relative to the runtime prefix) is potentially useful also in the non-Windows context, as long as Git was built with the runtime prefix feature. ... so I thought you meant to add this code for POSIX systems as well. My mistake. :( ATB, Ramsay Jones
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
On 06/11/2018 15:54, Ramsay Jones wrote:
>
>
> On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin
>>
>> On Windows, an absolute POSIX path needs to be turned into a Windows
>> one.
>>
>> Signed-off-by: Johannes Schindelin
>> ---
>> path.c | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/path.c b/path.c
>> index 34f0f98349..a72abf0e1f 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -11,6 +11,7 @@
>> #include "path.h"
>> #include "packfile.h"
>> #include "object-store.h"
>> +#include "exec-cmd.h"
>>
>> static int get_st_mode_bits(const char *path, int *mode)
>> {
>> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>>
>> if (path == NULL)
>> goto return_null;
>> +#ifdef __MINGW32__
>> +if (path[0] == '/')
>> +return system_path(path + 1);
>> +#endif
>
> Hmm, this doesn't quite fit with the intended use of this
> function! ;-) (even on windows!)
>
> I haven't looked very deeply, but doesn't this affect all
> absolute paths in the config read by git_config_pathname(),
> along with all 'included config' files?
>
> I am pretty sure that I would not want the absolute paths
> in my config file(s) magically 'moved' depending on whether
> git has been compiled with 'runtime prefix' support or not!
So, I hit 'send' before finishing my thought ...
I can't think of a non-backwards compatible way of doing
what you want. If backward compatibility wasn't an issue,
then we could (maybe) have used some kind of pathname prefix
like 'system:/path/relative/to/git/executable', or somesuch.
ATB,
Ramsay Jones
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin
>
> On Windows, an absolute POSIX path needs to be turned into a Windows
> one.
>
> Signed-off-by: Johannes Schindelin
> ---
> path.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/path.c b/path.c
> index 34f0f98349..a72abf0e1f 100644
> --- a/path.c
> +++ b/path.c
> @@ -11,6 +11,7 @@
> #include "path.h"
> #include "packfile.h"
> #include "object-store.h"
> +#include "exec-cmd.h"
>
> static int get_st_mode_bits(const char *path, int *mode)
> {
> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>
> if (path == NULL)
> goto return_null;
> +#ifdef __MINGW32__
> + if (path[0] == '/')
> + return system_path(path + 1);
> +#endif
Hmm, this doesn't quite fit with the intended use of this
function! ;-) (even on windows!)
I haven't looked very deeply, but doesn't this affect all
absolute paths in the config read by git_config_pathname(),
along with all 'included config' files?
I am pretty sure that I would not want the absolute paths
in my config file(s) magically 'moved' depending on whether
git has been compiled with 'runtime prefix' support or not!
ATB,
Ramsay Jones
> if (path[0] == '~') {
> const char *first_slash = strchrnul(path, '/');
> const char *username = path + 1;
>
Re: [PATCH v2 12/16] parse-options: replace opterror() with optname()
On 06/11/2018 02:33, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> There are a few issues with opterror() >> >> - it tries to assemble an English sentence from pieces. This is not >> great for translators because we give them pieces instead of a full >> sentence. >> >> - It's a wrapper around error() and needs some hack to let the >> compiler know it always returns -1. >> >> - Since it takes a string instead of printf format, one call site has >> to assemble the string manually before passing to it. >> >> Kill it and produce the option name with optname(). The user will use >> error() directly. This solves the second and third problems. > > The proposed log message is not very friendly to reviewers, as there > is no hint what optname() does nor where it came from; it turns out > that this patch introduces it. > > Introduce optname() that does the early half of original > opterror() to come up with the name of the option reported back > to the user, and use it to kill opterror(). The callers of > opterror() now directly call error() using the string returned > by opterror() instead. > > or something like that perhaps. > > Theoretically not very friendly to topics in flight, but I do not > expect there would be any right now that wants to add new callers of > opterror(). > > I do agree with the reasoning behind this change. Thanks for > working on it. > Also, this patch does not replace opterror() calls outside of the 'parse-options.c' file with optname(). This tickles my static-check.pl script, since optname() is an external function which is only called from 'parse-options.c'. So, at present, optname() could be marked as a local 'static' symbol. However, I could also imagine it being used by new callers outside of 'parse-options.c' in the future. (maybe) Your call. ;-) ATB, Ramsay Jones
Re: [PATCH 3/3] commit-reach.h: add missing declarations (hdr-check)
On 29/10/2018 01:13, Junio C Hamano wrote: > Ramsay Jones writes: > >> ... >>24 clear_contains_cache >> $ >> >> you will find 24 copies of the commit-slab routines for the contains_cache. >> Of course, when you enable optimizations again, these duplicate static >> functions (mostly) disappear. Compiling with gcc at -O2, leaves two static >> functions, thus: >> >> $ nm commit-reach.o | grep contains_cache >> 0870 t contains_cache_at_peek.isra.1.constprop.6 >> $ nm ref-filter.o | grep contains_cache >> 02b0 t clear_contains_cache.isra.14 >> $ >> >> However, using a shared 'contains_cache' would result in all six of the >> above functions as external public functions in the git binary. > > Sorry, but you lost me here. Where does the 6 in above 'all six' > come from? I saw 24 (from unoptimized), and I saw 2 (from > optimized), but... As you already gathered, the 'six of the above functions' was the list of 6 functions which were each duplicated 24 times (you only left the last one un-snipped above) in the unoptimized git binary. > Ah, OK, the slab system gives us a familiy of 6 helper functions to > deal with the "contains_cache" slab, and we call only 3 of them > (i.e. the implementation of other three are left unused). Makes > sense. Yep, only clear_contains_cache(), contains_cache_at() and init_contains_cache() are called. >> At present, >> only three of these functions are actually called, so the trade-off >> seems to favour letting the compiler inline the commit-slab functions. > > OK. I vaguely recall using a linker that can excise the > implementation an unused public function out of the resulting > executable in the past, which may tip the balance in the opposite > direction, Yes, and I thought I was using such a linker - I was surprised that seems not to be the case! ;-) [I know I have used such a linker, and I could have sworn it was on Linux ... ] As I said in [1], the first version of this patch actually used a shared contains_cache (so as not to #include "commit.h"). I changed that just before sending it out, because I felt the patch conflated two issues - I fully intended to send a "let's use a shared contains cache instead" follow up patch! (Again, see [1] for the initial version of that follow up patch). But then Derrick said he preferred this version of the patch and I couldn't really justify the follow up patch, other than to say "you are making your compiler work harder than it needs to ..." - not very convincing! :-P For example, applying the RFC/PATCH from [1] on top of this patch we can see: $ nm git | grep contains_cache 000d21f0 T clear_contains_cache 000d2400 T contains_cache_at 000d2240 T contains_cache_at_peek 000d2410 T contains_cache_peek 000d21d0 T init_contains_cache 000d2190 T init_contains_cache_with_stride $ size git text data bss dec hex filename 2531234 70736 274832 2876802 2be582 git $ whereas, without that patch on top (ie this patch): $ nm git | grep contains_cache 00161e30 t clear_contains_cache.isra.14 000d2190 t contains_cache_at_peek.isra.1.constprop.6 $ size git text data bss dec hex filename 2530962 70736 274832 2876530 2be472 git $ which means the 'shared contains_cache' patch leads to a +272 bytes of bloat in text section. (from the 3 unused external functions). [1] https://public-inbox.org/git/2809251f-6eba-b0ac-68fe-b972809cc...@ramsayjones.plus.com/ > but the above reasonong certainly makes sense to me. Thanks! ATB, Ramsay Jones
[PATCH 3/3] commit-reach.h: add missing declarations (hdr-check)
Add the necessary #includes and forward declarations to allow the header
file to pass the 'hdr-check' target.
Note that, since this header includes the commit-slab implementation
header file (indirectly via commit-slab.h), some of the commit-slab
inline functions (e.g contains_cache_at_peek()) will not compile without
the complete type of 'struct commit'. Hence, we replace the forward
declaration of 'struct commit' with the an #include of the 'commit.h'
header file.
It is possible, using the 'commit-slab-{decl,impl}.h' files, to avoid
this inclusion of the 'commit.h' header. Commit a9f1f1f9f8 ("commit-slab.h:
code split", 2018-05-19) separated the commit-slab interface from its
implementation, to allow for the definition of a public commit-slab data
structure. This enabled us to avoid including the commit-slab implementation
in a header file, which could result in the replication of the commit-slab
functions in each compilation unit in which it was included.
Indeed, if you compile with optimizations disabled, then run this script:
$ cat -n dup-static.sh
1 #!/bin/sh
2
3 nm $1 | grep ' t ' | cut -d' ' -f3 | sort | uniq -c |
4sort -rn | grep -v ' 1'
$
$ ./dup-static.sh git | grep contains
24 init_contains_cache_with_stride
24 init_contains_cache
24 contains_cache_peek
24 contains_cache_at_peek
24 contains_cache_at
24 clear_contains_cache
$
you will find 24 copies of the commit-slab routines for the contains_cache.
Of course, when you enable optimizations again, these duplicate static
functions (mostly) disappear. Compiling with gcc at -O2, leaves two static
functions, thus:
$ nm commit-reach.o | grep contains_cache
0870 t contains_cache_at_peek.isra.1.constprop.6
$ nm ref-filter.o | grep contains_cache
02b0 t clear_contains_cache.isra.14
$
However, using a shared 'contains_cache' would result in all six of the
above functions as external public functions in the git binary. At present,
only three of these functions are actually called, so the trade-off
seems to favour letting the compiler inline the commit-slab functions.
Signed-off-by: Ramsay Jones
---
commit-reach.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/commit-reach.h b/commit-reach.h
index 7d313e2975..f41d8f6ba3 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -1,12 +1,13 @@
#ifndef __COMMIT_REACH_H__
#define __COMMIT_REACH_H__
+#include "commit.h"
#include "commit-slab.h"
-struct commit;
struct commit_list;
-struct contains_cache;
struct ref_filter;
+struct object_id;
+struct object_array;
struct commit_list *get_merge_bases_many(struct commit *one,
int n,
--
2.19.0
[PATCH 2/3] ewok_rlw.h: add missing 'inline' to function definition
The 'ewok_rlw.h' header file contains the rlw_get_run_bit() function
definition, which is marked as 'static' but not 'inline'. At least when
compiled by gcc, with the default -O2 optimization level, the function
is actually inlined and leaves no static version in the ewah_bitmap.o
and ewah_rlw.o object files. Despite this, add the missing 'inline'
keyword to better describe the intended behaviour.
Signed-off-by: Ramsay Jones
---
ewah/ewok_rlw.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ewah/ewok_rlw.h b/ewah/ewok_rlw.h
index d487966935..bafa24f4c3 100644
--- a/ewah/ewok_rlw.h
+++ b/ewah/ewok_rlw.h
@@ -31,7 +31,7 @@
#define RLW_RUNNING_LEN_PLUS_BIT (((eword_t)1 << (RLW_RUNNING_BITS + 1)) - 1)
-static int rlw_get_run_bit(const eword_t *word)
+static inline int rlw_get_run_bit(const eword_t *word)
{
return *word & (eword_t)1;
}
--
2.19.0
[PATCH 1/3] fetch-object.h: add missing declaration (hdr-check)
Signed-off-by: Ramsay Jones --- fetch-object.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fetch-object.h b/fetch-object.h index d2f996d4e8..d6444caa5a 100644 --- a/fetch-object.h +++ b/fetch-object.h @@ -1,6 +1,8 @@ #ifndef FETCH_OBJECT_H #define FETCH_OBJECT_H +struct object_id; + void fetch_objects(const char *remote_name, const struct object_id *oids, int oid_nr); -- 2.19.0
[PATCH 0/3] some more hdr-check clean headers
I have some changes to the hdr-check Makefile target in the works, but these clean-ups don't have to be held up by those changes. The 'fetch-object.h' patch is the same one I sent separately last time, since it only applied on 'next' at the time. The 'ewok_rlw.h' patch is just something I noticed while messing around the the Makefile changes. The 'commit-reach.h' patch is the patch #9 from the original series, but now with a commit message that explains the '#include "commit.h"' issue. These changes are on top of current master (@c670b1f876) and merge without conflict to 'next' and with a 'minor' conflict on 'pu'. Ramsay Jones (3): fetch-object.h: add missing declaration (hdr-check) ewok_rlw.h: add missing 'inline' to function definition commit-reach.h: add missing declarations (hdr-check) commit-reach.h | 5 +++-- ewah/ewok_rlw.h | 2 +- fetch-object.h | 2 ++ 3 files changed, 6 insertions(+), 3 deletions(-) -- 2.19.0
Re: [PATCH v3 3/3] commit-slab: missing definitions and forward declarations (hdr-check)
On 26/10/2018 04:15, Carlo Arenas wrote: > On Thu, Oct 25, 2018 at 2:09 PM Ramsay Jones > wrote: >> Yes, this will 'fix' the 'commit-reach.h' header (not surprising), >> but I prefer my patch. ;-) > > I apologize, I joined the list recently and so might had missed a > reroll; the merged series in pu doesn't seem to include it and the > error was around the code I changed, so wanted to make sure it would > be addressed sooner rather than later. > > eitherway, I agree with you my patch (or something better) would fit > better in your topic branch than on mine and while I haven't seen your > patch I am sure is most likely better. Hmm, I don't know about that! Since the original series has progressed, any additions will now result in a new set of patches, rather than a re-roll. The original 'commit-reach.h' patch was not applied as part of the last series, since the commit message was felt to be lacking (well, it was actually non-existent!). ;-) I have been making some additional changes to the 'hdr-check' target in the Makefile, but I haven't quite finished. I will send the other (non-Makefile) changes soon. [These patches will make the 'master' and 'next' branches 'hdr-check' clean for me]. ATB, Ramsay Jones
Re: [PATCH v3 3/3] commit-slab: missing definitions and forward declarations (hdr-check)
On 25/10/2018 19:54, Ramsay Jones wrote: > > > On 25/10/2018 12:04, Carlo Marcelo Arenas Belón wrote: >> struct commmit needs to be defined before commit-slab can generate >> working code, object_id should be at least known through a forward >> declaration >> >> Signed-off-by: Carlo Marcelo Arenas Belón >> --- >> commit-slab-impl.h | 2 ++ >> commit-slab.h | 2 ++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/commit-slab-impl.h b/commit-slab-impl.h >> index e352c2f8c1..db7cf3f19b 100644 >> --- a/commit-slab-impl.h >> +++ b/commit-slab-impl.h >> @@ -1,6 +1,8 @@ >> #ifndef COMMIT_SLAB_IMPL_H >> #define COMMIT_SLAB_IMPL_H >> >> +#include "commit.h" >> + >> #define implement_static_commit_slab(slabname, elemtype) \ >> implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static) >> >> diff --git a/commit-slab.h b/commit-slab.h >> index 69bf0c807c..722252de61 100644 >> --- a/commit-slab.h >> +++ b/commit-slab.h >> @@ -1,6 +1,8 @@ >> #ifndef COMMIT_SLAB_H >> #define COMMIT_SLAB_H >> >> +struct object_id; >> + >> #include "commit-slab-decl.h" >> #include "commit-slab-impl.h" >> >> > > Hmm, sorry, I don't see how this patch has anything to do > with the other two patches! ;-) > > Also, I have a patch to fix up the 'commit-reach.h' header > (it was part of my original series, just had to update the > commit message), which adds these very #includes and forward > declarations when _using_ the commit-slab. > > I haven't tried applying your patches yet, which may answer > my questions, so I am a little puzzled. So, having now applied your patches, I still don't see what this patch has to do with the others! I suppose it is dependent on the compiler/version? (the most up-to-date version of clang available to me is 5.0.1). Yes, this will 'fix' the 'commit-reach.h' header (not surprising), but I prefer my patch. ;-) Still puzzled. ATB, Ramsay Jones
Re: [PATCH v3 3/3] commit-slab: missing definitions and forward declarations (hdr-check)
On 25/10/2018 12:04, Carlo Marcelo Arenas Belón wrote: > struct commmit needs to be defined before commit-slab can generate > working code, object_id should be at least known through a forward > declaration > > Signed-off-by: Carlo Marcelo Arenas Belón > --- > commit-slab-impl.h | 2 ++ > commit-slab.h | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/commit-slab-impl.h b/commit-slab-impl.h > index e352c2f8c1..db7cf3f19b 100644 > --- a/commit-slab-impl.h > +++ b/commit-slab-impl.h > @@ -1,6 +1,8 @@ > #ifndef COMMIT_SLAB_IMPL_H > #define COMMIT_SLAB_IMPL_H > > +#include "commit.h" > + > #define implement_static_commit_slab(slabname, elemtype) \ > implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static) > > diff --git a/commit-slab.h b/commit-slab.h > index 69bf0c807c..722252de61 100644 > --- a/commit-slab.h > +++ b/commit-slab.h > @@ -1,6 +1,8 @@ > #ifndef COMMIT_SLAB_H > #define COMMIT_SLAB_H > > +struct object_id; > + > #include "commit-slab-decl.h" > #include "commit-slab-impl.h" > > Hmm, sorry, I don't see how this patch has anything to do with the other two patches! ;-) Also, I have a patch to fix up the 'commit-reach.h' header (it was part of my original series, just had to update the commit message), which adds these very #includes and forward declarations when _using_ the commit-slab. I haven't tried applying your patches yet, which may answer my questions, so I am a little puzzled. ATB, Ramsay Jones
Re: [PATCH v4 2/3] reset: add new reset.quiet config setting
On 25/10/2018 10:26, Junio C Hamano wrote: > Junio C Hamano writes: > >> To be honest, I find the second sentence in your rewrite even more >> confusing. It reads as if `reset.quiet` configuration variable >> can be used to restore the "show what is yet to be added" >> behaviour, due to the parenthetical mention of the default behaviour >> without any configuration. >> >> The command reports what is yet to be added to the index >> after `reset` by default. It can be made to only report >> errors with the `--quiet` option, or setting `reset.quiet` >> configuration variable to `true` (the latter can be >> overriden with `--no-quiet`). >> >> That may not be much better, though X-<. > > In any case, the comments are getting closer to the bikeshedding > territory, that can be easily addressed incrementally. I am getting > the impression that everbody agrees that the change is desirable, > sufficiently documented and properly implemented. > > Shall we mark it for "Will merge to 'next'" in the what's cooking > report and leave further refinements to incremental updates as > needed? Yeah, the first version gave me a 'huh?' moment (hence the comment), the last version was better and, as you can see, I am no great shakes at wordsmith-ing documentation! ;-) Thanks! ATB, Ramsay Jones
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On 25/10/2018 02:09, Jeff King wrote: > On Thu, Oct 25, 2018 at 10:00:31AM +0900, Junio C Hamano wrote: > >> Jeff King writes: >> >>> but then you lose the default handling. I think if we added a new >>> option, it would either be: >>> >>> # interpret a value directly; use default on empty, I guess? >>> git config --default=false --type=bool --interpret-value >>> "$GIT_WHATEVER_ENV" >>> >>> or >>> >>> # less flexible, but the --default semantics are more obvious >>> git config --default=false --type=bool --get-env GIT_WHATEVER_ENV >> >> Yeah, my thinko. The latter would be closer to what this patch >> wants to have, but obviously the former would be more flexible and >> useful in wider context. Both have the "Huh?" factor---what they >> are doing has little to do with "config", but I did not think of a >> better kitchen-sink (and our default kitchen-sink "rev-parse" is >> even further than "config", I would think, for this one). > > Heh, I thought through the exact sequence in your paragraph when writing > my other message. That's probably a good sign that we should probably > not pursue this further unless we see the use case come up again a few > more times (and if we do, then consider "config" the least-bad place to > do it). I was thinking: $ git var -e GIT_WHATEVER_ENV [-e for environment]. ... but that is really no different than git-config. ;-) ATB, Ramsay Jones
Re: [PATCH v4 2/3] reset: add new reset.quiet config setting
On 23/10/2018 20:04, Ben Peart wrote:
> From: Ben Peart
Sorry for the late reply, ... I've been away from email - I am
still trying to catch up.
>
> Add a reset.quiet config setting that sets the default value of the --quiet
> flag when running the reset command. This enables users to change the
> default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset. Defaults to false.
>
> Signed-off-by: Ben Peart
> ---
> Documentation/config.txt| 3 +++
> Documentation/git-reset.txt | 5 -
> builtin/reset.c | 1 +
> 3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f6f4c21a54..a2d1b8b116 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2728,6 +2728,9 @@ rerere.enabled::
> `$GIT_DIR`, e.g. if "rerere" was previously used in the
> repository.
>
> +reset.quiet::
> + When set to true, 'git reset' will default to the '--quiet' option.
Mention that this 'Defaults to false'?
> +
> include::sendemail-config.txt[]
>
> sequence.editor::
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index 1d697d9962..2dac95c71a 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -95,7 +95,10 @@ OPTIONS
>
> -q::
> --quiet::
> - Be quiet, only report errors.
> +--no-quiet::
> + Be quiet, only report errors. The default behavior is set by the
> + `reset.quiet` config option. `--quiet` and `--no-quiet` will
> + override the default behavior.
Better than last time, but how about something like:
-q::
--quiet::
--no-quiet::
Be quiet, only report errors. The default behaviour of the
command, which is to not be quiet, can be specified by the
`reset.quiet` configuration variable. The `--quiet` and
`--no-quiet` options can be used to override any configured
default.
Hmm, I am not sure that is any better! :-D
Also, note that the --no-option is often described separately to
the --option (in a separate paragraph). I don't know if that would
help here.
[The default behaviour is _not_ set by the configuration, if no
configuration is specified. :-P ]
Not sure if that helps!
ATB,
Ramsay Jones
>
>
> EXAMPLES
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 04f0d9b4f5..3b43aee544 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char
> *prefix)
> };
>
> git_config(git_reset_config, NULL);
> + git_config_get_bool("reset.quiet", );
>
> argc = parse_options(argc, argv, prefix, options, git_reset_usage,
> PARSE_OPT_KEEP_DASHDASH);
>
Re: [PATCH v3 2/3] reset: add new reset.quiet config setting
On 22/10/2018 14:18, Ben Peart wrote:
> From: Ben Peart
>
> Add a reset.quiet config setting that sets the default value of the --quiet
> flag when running the reset command. This enables users to change the
> default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset. Defaults to false.
>
> Signed-off-by: Ben Peart
> ---
> Documentation/config.txt| 3 +++
> Documentation/git-reset.txt | 4 +++-
> builtin/reset.c | 1 +
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f6f4c21a54..a2d1b8b116 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2728,6 +2728,9 @@ rerere.enabled::
> `$GIT_DIR`, e.g. if "rerere" was previously used in the
> repository.
>
> +reset.quiet::
> + When set to true, 'git reset' will default to the '--quiet' option.
> +
> include::sendemail-config.txt[]
>
> sequence.editor::
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index 1d697d9962..51a427a34a 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -95,7 +95,9 @@ OPTIONS
>
> -q::
> --quiet::
> - Be quiet, only report errors.
> +--no-quiet::
> + Be quiet, only report errors. The default behavior respects the
> + `reset.quiet` config option, or `--no-quiet` if that is not set.
Sorry, I can't quite parse this; -q,--quiet and --no-quiet on the
command line (should) trump whatever rest.quiet is set to in the
configuration. Is that not the case?
ATB,
Ramsay Jones
>
>
> EXAMPLES
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 04f0d9b4f5..3b43aee544 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char
> *prefix)
> };
>
> git_config(git_reset_config, NULL);
> + git_config_get_bool("reset.quiet", );
>
> argc = parse_options(argc, argv, prefix, options, git_reset_usage,
> PARSE_OPT_KEEP_DASHDASH);
>
Re: [PATCH] config.mak.dev: enable -Wunused-function
On 18/10/2018 08:05, Jeff King wrote:
> We explicitly omitted -Wunused-function when we added
> -Wextra, but there is no need: the current code compiles
> cleanly with it. And it's worth having, since it can let you
> know when there are cascading effects from a cleanup (e.g.,
> deleting one function lets you delete its static helpers).
>
> There are cases where we may need an unused function to
> exist, but we can handle these easily:
>
> - macro-generated code like commit-slab; there we have the
> MAYBE_UNUSED annotation to silence the compiler
>
> - conditional compilation, where we may or may not need a
> static helper. These generally fall into one of two
> categories:
>
> - the call should not be conditional, but rather the
> function body itself should be (and may just be a
> no-op on one side of the #if). That keeps the
> conditional pollution out of the main code.
>
> - call-chains of static helpers should all be in the
> same #if block, so they are all-or-nothing
>
> And if there's some case that doesn't cover, we still
> have MAYBE_UNUSED as a fallback.
>
> Signed-off-by: Jeff King
> ---
> config.mak.dev | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/config.mak.dev b/config.mak.dev
> index 92d268137f..bbeeff44fe 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -34,7 +34,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),)
> CFLAGS += -Wno-empty-body
> CFLAGS += -Wno-missing-field-initializers
> CFLAGS += -Wno-sign-compare
> -CFLAGS += -Wno-unused-function
> CFLAGS += -Wno-unused-parameter
> endif
> endif
>
Heh, as luck would have it, tonight I had an -Wunused-function
warning on cygwin, but not Linux, when building the 'pu' branch.
[On linux, I am using DEVELOPER=1 in my config.mak etc., whereas
on cygwin I am still using an explicit (but very similar) list
of -W args.]
I haven't looked too deeply, but this seems to be caused by
Junio's commit 42c89ea70a ("SQUASH??? - convert the other user of
string-list as db", 2018-10-17) which removes a call to the
add_existing() function - the subject of the warning.
[BTW there is another 'static add_existing()' in builtin/show_ref.c]
ATB,
Ramsay Jones
[PATCH v2] headers: normalize the spelling of some header guards
Signed-off-by: Ramsay Jones --- Hi Junio, Since I didn't get any adverse comments, this version has the RFC label removed. Also, given that it seems the vcs-svn directory is not going away soon, I have included those headers this time as well. [Note: my email client (thunderbird) was updated yesterday to 60.2.1. As a result of recent reports, I sent this patch to myself and applied it with 'git am' and ... the results seem to be fine! ;-) Famous last words!] Thanks. ATB, Ramsay Jones alias.h | 4 ++-- commit-reach.h | 4 ++-- fetch-negotiator.h | 4 ++-- midx.h | 4 ++-- t/helper/test-tool.h | 4 ++-- vcs-svn/fast_export.h| 4 ++-- vcs-svn/line_buffer.h| 4 ++-- vcs-svn/sliding_window.h | 4 ++-- vcs-svn/svndiff.h| 4 ++-- vcs-svn/svndump.h| 4 ++-- 10 files changed, 20 insertions(+), 20 deletions(-) diff --git a/alias.h b/alias.h index 79933f2457..aef4843bb7 100644 --- a/alias.h +++ b/alias.h @@ -1,5 +1,5 @@ -#ifndef __ALIAS_H__ -#define __ALIAS_H__ +#ifndef ALIAS_H +#define ALIAS_H struct string_list; diff --git a/commit-reach.h b/commit-reach.h index 7d313e2975..122a23a24d 100644 --- a/commit-reach.h +++ b/commit-reach.h @@ -1,5 +1,5 @@ -#ifndef __COMMIT_REACH_H__ -#define __COMMIT_REACH_H__ +#ifndef COMMIT_REACH_H +#define COMMIT_REACH_H #include "commit-slab.h" diff --git a/fetch-negotiator.h b/fetch-negotiator.h index ddb44a22dc..9e3967ce66 100644 --- a/fetch-negotiator.h +++ b/fetch-negotiator.h @@ -1,5 +1,5 @@ -#ifndef FETCH_NEGOTIATOR -#define FETCH_NEGOTIATOR +#ifndef FETCH_NEGOTIATOR_H +#define FETCH_NEGOTIATOR_H struct commit; diff --git a/midx.h b/midx.h index ce80b91c68..ee83702309 100644 --- a/midx.h +++ b/midx.h @@ -1,5 +1,5 @@ -#ifndef __MIDX_H__ -#define __MIDX_H__ +#ifndef MIDX_H +#define MIDX_H #include "repository.h" diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index e4890566da..71f470b871 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -1,5 +1,5 @@ -#ifndef __TEST_TOOL_H__ -#define __TEST_TOOL_H__ +#ifndef TEST_TOOL_H +#define TEST_TOOL_H #include "git-compat-util.h" diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h index 60b79c35b9..9dcf9337c1 100644 --- a/vcs-svn/fast_export.h +++ b/vcs-svn/fast_export.h @@ -1,5 +1,5 @@ -#ifndef FAST_EXPORT_H_ -#define FAST_EXPORT_H_ +#ifndef FAST_EXPORT_H +#define FAST_EXPORT_H struct strbuf; struct line_buffer; diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h index ee23b4f490..e192aedea2 100644 --- a/vcs-svn/line_buffer.h +++ b/vcs-svn/line_buffer.h @@ -1,5 +1,5 @@ -#ifndef LINE_BUFFER_H_ -#define LINE_BUFFER_H_ +#ifndef LINE_BUFFER_H +#define LINE_BUFFER_H #include "strbuf.h" diff --git a/vcs-svn/sliding_window.h b/vcs-svn/sliding_window.h index b43a825cba..189c32d84c 100644 --- a/vcs-svn/sliding_window.h +++ b/vcs-svn/sliding_window.h @@ -1,5 +1,5 @@ -#ifndef SLIDING_WINDOW_H_ -#define SLIDING_WINDOW_H_ +#ifndef SLIDING_WINDOW_H +#define SLIDING_WINDOW_H #include "strbuf.h" diff --git a/vcs-svn/svndiff.h b/vcs-svn/svndiff.h index 74eb464bab..10a2cbc40e 100644 --- a/vcs-svn/svndiff.h +++ b/vcs-svn/svndiff.h @@ -1,5 +1,5 @@ -#ifndef SVNDIFF_H_ -#define SVNDIFF_H_ +#ifndef SVNDIFF_H +#define SVNDIFF_H struct line_buffer; struct sliding_view; diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h index b8eb12954e..26faed5968 100644 --- a/vcs-svn/svndump.h +++ b/vcs-svn/svndump.h @@ -1,5 +1,5 @@ -#ifndef SVNDUMP_H_ -#define SVNDUMP_H_ +#ifndef SVNDUMP_H +#define SVNDUMP_H int svndump_init(const char *filename); int svndump_init_fd(int in_fd, int back_fd); -- 2.19.0
Re: [PATCH v2 1/1] zlib.c: use size_t for size
On 15/10/18 01:01, Jeff King wrote:
> On Sun, Oct 14, 2018 at 04:03:48PM +0100, Ramsay Jones wrote:
>
>>> So I kind of wonder if a comment would be better than xsize_t here.
>>> Something like:
>>>
>>> if (avail > len) {
>>> /*
>>> * This can never truncate because we know that len is smaller
>>> * than avail, which is already a size_t.
>>> */
>>> avail = (size_t)len;
>>> }
>>
>> Heh, you are, of course, correct! (that will learn me[1]). :-D
>>
>> Hmm, let's see if I can muster the enthusiasm to do all that
>> testing again!
>
> For the record, I am not opposed to including the comment _and_ using
> xsize_t() to do the cast, giving us an assertion that the comment is
> correct.
Heh, I haven't found any enthusiasm tonight. Let's see if there
are any more comments/opinions.
Thanks.
ATB,
Ramsay Jones
Re: [RFC/PATCH] headers: normalize the spelling of some header guards
On 15/10/18 00:59, Jeff King wrote: > On Sun, Oct 14, 2018 at 09:13:09PM +0100, Ramsay Jones wrote: > >> This patch is marked RFC because I am not aware of any policy with >> regard to header guard spelling. Having said that, apart from the >> fetch-negotiator.h header, all of these headers are using a reserved >> identifier (see C99 Standard 7.1.3). >> >> These headers were found, thus: >> >> $ git grep -n -E '^#ifn?def ' -- '*.h' | grep 'h\:1\:' | grep -v '^compat' >> | grep -v -E '[A-Z_]*_H$' >> alias.h:1:#ifndef __ALIAS_H__ >> commit-reach.h:1:#ifndef __COMMIT_REACH_H__ >> fetch-negotiator.h:1:#ifndef FETCH_NEGOTIATOR >> midx.h:1:#ifndef __MIDX_H__ >> t/helper/test-tool.h:1:#ifndef __TEST_TOOL_H__ >> vcs-svn/fast_export.h:1:#ifndef FAST_EXPORT_H_ >> vcs-svn/line_buffer.h:1:#ifndef LINE_BUFFER_H_ >> vcs-svn/sliding_window.h:1:#ifndef SLIDING_WINDOW_H_ >> vcs-svn/svndiff.h:1:#ifndef SVNDIFF_H_ >> vcs-svn/svndump.h:1:#ifndef SVNDUMP_H_ > > I think the ones with a trailing underscore are actually OK according to > the standard (not sure if your "all of these" was including the ones in > vcs-svn ;) ). Yes, trailing underscore is fine - "all of these" meant the headers in the patch, with the noted exception of fetch-negotiator.h. > I'm in favor of normalizing even the ones that aren't illegal, though > I'm OK either way on the vcs-svn bits if they're going away anyway. I wasn't sure about vcs-svn, but assumed that they might go away soon (hence the cc: list). I will be happy to add them to the patch, if that is not the case. Thanks. ATB, Ramsay Jones
[RFC/PATCH] headers: normalize the spelling of some header guards
Signed-off-by: Ramsay Jones --- Hi Junio, This patch is marked RFC because I am not aware of any policy with regard to header guard spelling. Having said that, apart from the fetch-negotiator.h header, all of these headers are using a reserved identifier (see C99 Standard 7.1.3). These headers were found, thus: $ git grep -n -E '^#ifn?def ' -- '*.h' | grep 'h\:1\:' | grep -v '^compat' | grep -v -E '[A-Z_]*_H$' alias.h:1:#ifndef __ALIAS_H__ commit-reach.h:1:#ifndef __COMMIT_REACH_H__ fetch-negotiator.h:1:#ifndef FETCH_NEGOTIATOR midx.h:1:#ifndef __MIDX_H__ t/helper/test-tool.h:1:#ifndef __TEST_TOOL_H__ vcs-svn/fast_export.h:1:#ifndef FAST_EXPORT_H_ vcs-svn/line_buffer.h:1:#ifndef LINE_BUFFER_H_ vcs-svn/sliding_window.h:1:#ifndef SLIDING_WINDOW_H_ vcs-svn/svndiff.h:1:#ifndef SVNDIFF_H_ vcs-svn/svndump.h:1:#ifndef SVNDUMP_H_ $ Note that I haven't included the headers in vcs-svn because there is a patch being discussed which would move that directory to contrib. ATB, Ramsay Jones alias.h | 4 ++-- commit-reach.h | 4 ++-- fetch-negotiator.h | 4 ++-- midx.h | 4 ++-- t/helper/test-tool.h | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/alias.h b/alias.h index 79933f2457..aef4843bb7 100644 --- a/alias.h +++ b/alias.h @@ -1,5 +1,5 @@ -#ifndef __ALIAS_H__ -#define __ALIAS_H__ +#ifndef ALIAS_H +#define ALIAS_H struct string_list; diff --git a/commit-reach.h b/commit-reach.h index 7d313e2975..122a23a24d 100644 --- a/commit-reach.h +++ b/commit-reach.h @@ -1,5 +1,5 @@ -#ifndef __COMMIT_REACH_H__ -#define __COMMIT_REACH_H__ +#ifndef COMMIT_REACH_H +#define COMMIT_REACH_H #include "commit-slab.h" diff --git a/fetch-negotiator.h b/fetch-negotiator.h index ddb44a22dc..9e3967ce66 100644 --- a/fetch-negotiator.h +++ b/fetch-negotiator.h @@ -1,5 +1,5 @@ -#ifndef FETCH_NEGOTIATOR -#define FETCH_NEGOTIATOR +#ifndef FETCH_NEGOTIATOR_H +#define FETCH_NEGOTIATOR_H struct commit; diff --git a/midx.h b/midx.h index ce80b91c68..ee83702309 100644 --- a/midx.h +++ b/midx.h @@ -1,5 +1,5 @@ -#ifndef __MIDX_H__ -#define __MIDX_H__ +#ifndef MIDX_H +#define MIDX_H #include "repository.h" diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index e4890566da..71f470b871 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -1,5 +1,5 @@ -#ifndef __TEST_TOOL_H__ -#define __TEST_TOOL_H__ +#ifndef TEST_TOOL_H +#define TEST_TOOL_H #include "git-compat-util.h" -- 2.19.0
Re: [PATCH v2 1/1] zlib.c: use size_t for size
On 14/10/18 03:52, Jeff King wrote:
> On Sun, Oct 14, 2018 at 03:16:36AM +0100, Ramsay Jones wrote:
>
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index b059b86aee..3b5f2c38b3 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -269,12 +269,12 @@ static void copy_pack_data(struct hashfile *f,
>> off_t len)
>> {
>> unsigned char *in;
>> -unsigned long avail;
>> +size_t avail;
>>
>> while (len) {
>> in = use_pack(p, w_curs, offset, );
>> if (avail > len)
>> -avail = (unsigned long)len;
>> +avail = xsize_t(len);
>
> We don't actually care about truncation here. The idea is that we take a
> bite-sized chunk via use_pack, and loop as necessary. So mod-2^32
> truncation via a cast would be bad (we might not make forward progress),
> but truncating to SIZE_MAX would be fine.
>
> That said, we know that would not truncate here, because we must
> strictly be shrinking "avail", which was already a size_t (unless "len"
> is negative, but then we are really screwed ;) ).
>
> So I kind of wonder if a comment would be better than xsize_t here.
> Something like:
>
> if (avail > len) {
> /*
>* This can never truncate because we know that len is smaller
>* than avail, which is already a size_t.
>*/
> avail = (size_t)len;
> }
Heh, you are, of course, correct! (that will learn me[1]). :-D
Hmm, let's see if I can muster the enthusiasm to do all that
testing again!
ATB,
Ramsay Jones
[1] Since I started with my patch, when I had finished 'paring
it back', the result didn't have this xsize_t() call. In order
to make the result 'v2 + SZEDER's patch' (which I thought was
quite neat) I added this call right at the end. :-P
Re: [PATCH v2 1/1] zlib.c: use size_t for size
On 14/10/18 03:16, Ramsay Jones wrote:
>
>
> On 13/10/18 06:00, Torsten Bögershausen wrote:
>> []
>>> Neither v1 nor v2 of this patch compiles on 32 bit Linux; see
>>>
>>> https://travis-ci.org/git/git/jobs/440514375#L628
>>>
>>> The fixup patch below makes it compile on 32 bit and the test suite
>>> passes as well, but I didn't thoroughly review the changes; I only
>>> wanted to get 'pu' build again.
>>>
>>
>> Oh, yes, I didn't test under Linux 32 bit (and neither Windows)
>> I will try to compose a proper v3 the next days.
>
> I had a look at this today, and the result is given below.
>
> The patch is effectively your v2 patch with SZEDER's fix-up
> patch on top! (I actually started with my own patch and 'pared
> it back' by removing the off_t -> size_t changes, etc; so I was
> somewhat amused to note that the end result was effectively
> SZEDER's patch on top of your v2 ;-) ).
>
> I have tested this on 32- and 64-bit Linux, along with 64-bit
> cygwin (the test suite run hasn't finished yet, but I don't
> expect any problem). I have an old Msys2 installation on Windows,
> which is the closest thing I have to a windows dev system, so I
> also built this on MINGW32 and MINGW64 along with some light
> manual testing (the test suite has never passed on Msys2 for me).
> This is not the same as testing on Gfw, of course.
Ho, Hum. Of course, I have just noticed that, similar to
copy_pack_data(), check_pack_crc() should probably use a
call to xsize_t(len) when assigning to avail.
Sorry about that. (I need some sleep now!)
ATB,
Ramsay Jones
> -- >8 --
> From: Martin Koegler
> Subject: [PATCH v3 1/1] zlib.c: use size_t for size
>
> Signed-off-by: Martin Koegler
> Signed-off-by: Junio C Hamano
> Signed-off-by: Torsten Bögershausen
> Helped-by: SZEDER Gábor
> Signed-off-by: Ramsay Jones
> ---
> builtin/pack-objects.c | 11 ++-
> cache.h| 10 +-
> pack-check.c | 4 ++--
> packfile.c | 4 ++--
> packfile.h | 2 +-
> wrapper.c | 8
> zlib.c | 8
> 7 files changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index b059b86aee..3b5f2c38b3 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -269,12 +269,12 @@ static void copy_pack_data(struct hashfile *f,
> off_t len)
> {
> unsigned char *in;
> - unsigned long avail;
> + size_t avail;
>
> while (len) {
> in = use_pack(p, w_curs, offset, );
> if (avail > len)
> - avail = (unsigned long)len;
> + avail = xsize_t(len);
> hashwrite(f, in, avail);
> offset += avail;
> len -= avail;
> @@ -1529,8 +1529,8 @@ static void check_object(struct object_entry *entry)
> struct pack_window *w_curs = NULL;
> const unsigned char *base_ref = NULL;
> struct object_entry *base_entry;
> - unsigned long used, used_0;
> - unsigned long avail;
> + size_t used, used_0;
> + size_t avail;
> off_t ofs;
> unsigned char *buf, c;
> enum object_type type;
> @@ -2002,7 +2002,8 @@ unsigned long oe_get_size_slow(struct packing_data
> *pack,
> struct pack_window *w_curs;
> unsigned char *buf;
> enum object_type type;
> - unsigned long used, avail, size;
> + unsigned long used, size;
> + size_t avail;
>
> if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
> read_lock();
> diff --git a/cache.h b/cache.h
> index 5d83022e3b..ba0ad73de1 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -20,10 +20,10 @@
> #include
> typedef struct git_zstream {
> z_stream z;
> - unsigned long avail_in;
> - unsigned long avail_out;
> - unsigned long total_in;
> - unsigned long total_out;
> + size_t avail_in;
> + size_t avail_out;
> + size_t total_in;
> + size_t total_out;
> unsigned char *next_in;
> unsigned char *next_out;
> } git_zstream;
> @@ -40,7 +40,7 @@ void git_deflate_end(git_zstream *);
> int git_deflate_abort(git_zstream *);
> int git_deflate_end_gently(git_zstream *);
> int git_deflate(git_zstream *, int flush);
> -unsigned long git_deflate_bound(git_zstream *, unsigned long);
> +size_t git_deflate_bound(git_zstream *, size_t);
>
> /* The length in bytes and in hex digits of an object n
Re: [PATCH v2 1/1] zlib.c: use size_t for size
On 13/10/18 06:00, Torsten Bögershausen wrote:
> []
>> Neither v1 nor v2 of this patch compiles on 32 bit Linux; see
>>
>> https://travis-ci.org/git/git/jobs/440514375#L628
>>
>> The fixup patch below makes it compile on 32 bit and the test suite
>> passes as well, but I didn't thoroughly review the changes; I only
>> wanted to get 'pu' build again.
>>
>
> Oh, yes, I didn't test under Linux 32 bit (and neither Windows)
> I will try to compose a proper v3 the next days.
I had a look at this today, and the result is given below.
The patch is effectively your v2 patch with SZEDER's fix-up
patch on top! (I actually started with my own patch and 'pared
it back' by removing the off_t -> size_t changes, etc; so I was
somewhat amused to note that the end result was effectively
SZEDER's patch on top of your v2 ;-) ).
I have tested this on 32- and 64-bit Linux, along with 64-bit
cygwin (the test suite run hasn't finished yet, but I don't
expect any problem). I have an old Msys2 installation on Windows,
which is the closest thing I have to a windows dev system, so I
also built this on MINGW32 and MINGW64 along with some light
manual testing (the test suite has never passed on Msys2 for me).
This is not the same as testing on Gfw, of course.
ATB,
Ramsay Jones
-- >8 --
From: Martin Koegler
Subject: [PATCH v3 1/1] zlib.c: use size_t for size
Signed-off-by: Martin Koegler
Signed-off-by: Junio C Hamano
Signed-off-by: Torsten Bögershausen
Helped-by: SZEDER Gábor
Signed-off-by: Ramsay Jones
---
builtin/pack-objects.c | 11 ++-
cache.h| 10 +-
pack-check.c | 4 ++--
packfile.c | 4 ++--
packfile.h | 2 +-
wrapper.c | 8
zlib.c | 8
7 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..3b5f2c38b3 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -269,12 +269,12 @@ static void copy_pack_data(struct hashfile *f,
off_t len)
{
unsigned char *in;
- unsigned long avail;
+ size_t avail;
while (len) {
in = use_pack(p, w_curs, offset, );
if (avail > len)
- avail = (unsigned long)len;
+ avail = xsize_t(len);
hashwrite(f, in, avail);
offset += avail;
len -= avail;
@@ -1529,8 +1529,8 @@ static void check_object(struct object_entry *entry)
struct pack_window *w_curs = NULL;
const unsigned char *base_ref = NULL;
struct object_entry *base_entry;
- unsigned long used, used_0;
- unsigned long avail;
+ size_t used, used_0;
+ size_t avail;
off_t ofs;
unsigned char *buf, c;
enum object_type type;
@@ -2002,7 +2002,8 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
struct pack_window *w_curs;
unsigned char *buf;
enum object_type type;
- unsigned long used, avail, size;
+ unsigned long used, size;
+ size_t avail;
if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
read_lock();
diff --git a/cache.h b/cache.h
index 5d83022e3b..ba0ad73de1 100644
--- a/cache.h
+++ b/cache.h
@@ -20,10 +20,10 @@
#include
typedef struct git_zstream {
z_stream z;
- unsigned long avail_in;
- unsigned long avail_out;
- unsigned long total_in;
- unsigned long total_out;
+ size_t avail_in;
+ size_t avail_out;
+ size_t total_in;
+ size_t total_out;
unsigned char *next_in;
unsigned char *next_out;
} git_zstream;
@@ -40,7 +40,7 @@ void git_deflate_end(git_zstream *);
int git_deflate_abort(git_zstream *);
int git_deflate_end_gently(git_zstream *);
int git_deflate(git_zstream *, int flush);
-unsigned long git_deflate_bound(git_zstream *, unsigned long);
+size_t git_deflate_bound(git_zstream *, size_t);
/* The length in bytes and in hex digits of an object name (SHA-1 value). */
#define GIT_SHA1_RAWSZ 20
diff --git a/pack-check.c b/pack-check.c
index fa5f0ff8fa..d1e7f554ae 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -33,7 +33,7 @@ int check_pack_crc(struct packed_git *p, struct pack_window
**w_curs,
uint32_t data_crc = crc32(0, NULL, 0);
do {
- unsigned long avail;
+ size_t avail;
void *data = use_pack(p, w_curs, offset, );
if (avail > len)
avail = len;
@@ -68,7 +68,7 @@ static int verify_packfile(struct packed_git *p,
the_hash_algo->init_fn();
do {
- unsigned long remaining;
+ size_t remaining;
Re: [PATCH] zlib.c: use size_t for size
On 12/10/18 16:34, Johannes Schindelin wrote:
> Hi Junio,
>
> On Fri, 12 Oct 2018, Junio C Hamano wrote:
>
>> Johannes Schindelin writes:
>>
>>> Hi Junio,
>>>
>>> On Fri, 12 Oct 2018, Junio C Hamano wrote:
>>>
>>>> From: Martin Koegler
>>>> Date: Thu, 10 Aug 2017 20:13:08 +0200
>>>>
>>>> Signed-off-by: Martin Koegler
>>>> Signed-off-by: Junio C Hamano
>>>> ---
>>>>
>>>> * I made minimal adjustments to make the change apply to today's
>>>>codebase. I still find some choices and mixing of off_t and
>>>>size_t done by the patch a bit iffy, and some arith may need to
>>>>become st_addX(). Extra set of eyes are very much appreciated.
>>>>
>>>> builtin/pack-objects.c | 10 +-
>>>> cache.h| 10 +-
>>>> pack-check.c | 6 +++---
>>>> pack.h | 2 +-
>>>> packfile.h | 2 +-
>>>> wrapper.c | 8
>>>> zlib.c | 8
>>>> 7 files changed, 23 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>>>> index e6316d294d..b9ca04eb8a 100644
>>>> --- a/builtin/pack-objects.c
>>>> +++ b/builtin/pack-objects.c
>>>> @@ -266,15 +266,15 @@ static void copy_pack_data(struct hashfile *f,
>>>>struct packed_git *p,
>>>>struct pack_window **w_curs,
>>>>off_t offset,
>>>> - off_t len)
>>>> + size_t len)
>>>> {
>>>>unsigned char *in;
>>>> - unsigned long avail;
>>>> + size_t avail;
>>>>
>>>>while (len) {
>>>>in = use_pack(p, w_curs, offset, );
>>>>if (avail > len)
>>>
>>> Do we have to be careful to handle sizeof(off_t) != sizeof(size_t) here? I
>>> guess we would receive a compiler warning about different sizes in that
>>> case, but it makes me worry.
>>
>> Yup, you just said exactly the same thing as I already said in the
>> part you quoted. I still find the mixed use of off_t and size_t in
>> this patch iffy, and that was the secondary reason why the patch was
>> kept in the stalled state for so long. The primary reason was that
>> nobody tried to dust it off and reignite the topic so far---which I
>> am trying to correct, but as I said, this is just minimally adjusted
>> to today's codebase, without any attempt to improve relative to the
>> original patch.
>>
>> I think we are in agreement in that this is not making things worse,
>> as we are already in the mixed arith territory before this patch.
>
> I will *try* to find the time to audit this some more, then. Maybe next
> week, maybe the one afterwards... ;-)
This fails to compile on 32-bit Linux. The patch given below is
what I used to get git to build on 32-bit Linux (and it even
passes the tests).
I haven't even given the off_t -> size_t issue any thought, but I
suspect that will have to change as well. Anyway, I have no more
time tonight ...
ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] zlib: minimum fix-up on 32-bit linux
Signed-off-by: Ramsay Jones
---
builtin/pack-objects.c | 3 ++-
packfile.c | 16
packfile.h | 2 +-
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 7e693071b..cc11a0426 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2002,7 +2002,8 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
struct pack_window *w_curs;
unsigned char *buf;
enum object_type type;
- unsigned long used, avail, size;
+ size_t used, avail;
+ unsigned long size;
if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
read_lock();
diff --git a/packfile.c b/packfile.c
index f2850a00b..7571ac988 100644
--- a/packfile.c
+++ b/packfile.c
@@ -585,7 +585,7 @@ static int in_window(struct pack_window *win, off_t offset)
unsigned char *use_pack(struct packed_git *p,
struct pack_window **w_cursor,
off_t offset,
- unsigned long *left)
+ size_t *left)
{
struct pack_window *win = *w_cursor;
@@ -1034,19 +1034,19 @@ struct list_head *get_packed_git_mru(struct repository
*r)
return >objects->packed_git_mru;
}
-unsigned long unpack_object_header_buffer(const unsi
Re: [PATCH v4 4/4] transport.c: introduce core.alternateRefsPrefixes
On 02/10/18 03:24, Taylor Blau wrote:
[snip]
> diff --git a/t/t5410-receive-pack-alternates.sh
> b/t/t5410-receive-pack-alternates.sh
> index 49d0fe44fb..94794c35da 100755
> --- a/t/t5410-receive-pack-alternates.sh
> +++ b/t/t5410-receive-pack-alternates.sh
> @@ -30,4 +30,12 @@ test_expect_success 'with core.alternateRefsCommand' '
> test_cmp expect actual.haves
> '
>
> +test_expect_success 'with core.alternateRefsPrefixes' '
> + test_config -C fork core.alternateRefsPrefixes "refs/heads/private" &&
> + git rev-parse private/branch expect &&
s/expect/>expect/ ?
ATB,
Ramsay Jones
> + printf "" | git receive-pack fork >actual &&
> + extract_haves actual.haves &&
> + test_cmp expect actual.haves
> +'
> +
> test_done
> diff --git a/transport.c b/transport.c
> index e271b66603..83474add28 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct
> child_process *cmd,
> argv_array_pushf(>args, "--git-dir=%s", repo_path);
> argv_array_push(>args, "for-each-ref");
> argv_array_push(>args, "--format=%(objectname)");
> +
> + if (!git_config_get_value("core.alternateRefsPrefixes",
> )) {
> + argv_array_push(>args, "--");
> + argv_array_split(>args, value);
> + }
> }
>
> cmd->env = local_repo_env;
>
Re: [PATCH v6 4/7] config: add new index.threads config setting
On 28/09/18 20:41, Ben Peart wrote:
>
>
> On 9/28/2018 1:07 PM, Junio C Hamano wrote:
>> Ben Peart writes:
>>
>>>> Why does multithreading have to be disabled in this test?
>>>
>>> If multi-threading is enabled, it will write out the IEOT extension
>>> which changes the SHA and causes the test to fail.
>>
>> I think it is a design mistake to let the writing processes's
>> capability decide what is written in the file to be read later by a
>> different process, which possibly may have different capability. If
>> you are not writing with multiple threads, it should not matter if
>> that writer process is capable of and configured to spawn 8 threads
>> if the process were reading the file---as it is not reading the file
>> it is writing right now.
>>
>> I can understand if the design is to write IEOT only if the
>> resulting index is expected to become large enough (above an
>> arbitrary threshold like 100k entries) to matter. I also can
>> understand if IEOT is omitted when the repository configuration says
>> that no process is allowed to read the index with multi-threaded
>> codepath in that repository.
>>
>
> There are two different paths which determine how many blocks are written to
> the IEOT. The first is the default path. On this path, the number of blocks
> is determined by the number of cache entries divided by the THREAD_COST. If
> there are sufficient entries to make it faster to use threading, then it will
> automatically use enough blocks to optimize the performance of reading the
> entries across multiple threads.
>
> I currently cap the maximum number of blocks to be the number of cores that
> would be available to process them on that same machine purely as an
> optimization. The majority of the time, the index will be read from the same
> machine that it was written on so this works well. Before I added that
> logic, you would usually end up with more blocks than available threads which
> meant some threads had more to do than the other threads and resulted in
> worse performance. For example, 4 blocks across 3 threads results in the 1st
> thread having twice as much work to do as the other threads.
>
> If the index is copied to a machine with a different number of cores, it will
> still all work - it just may not be optimal for that machine. This is self
> correcting because as soon as the index is written out, it will be optimized
> for that machine.
>
> If the "automatically try to make it perform optimally" logic doesn't work
> for some reason, we have path #2.
>
> The second path is when the user specifies a specific number of blocks via
> the GIT_TEST_INDEX_THREADS= environment variable or the index.threads=
> config setting. If they ask for n blocks, they will get n blocks. This is
> the "I know what I'm doing and want to control the behavior" path.
>
> I just added one additional test (see patch below) to avoid a divide by zero
> bug and simplify things a bit. With this change, if there are fewer than two
> blocks, the IEOT extension is not written out as it isn't needed. The load
> would be single threaded anyway so there is no reason to write out a IEOT
> extensions that won't be used.
>
>
>
> diff --git a/read-cache.c b/read-cache.c
> index f5d766088d..a1006fa824 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2751,18 +2751,23 @@ static int do_write_index(struct index_state *istate,
> struct tempfile *tempfil
> e,
> */
> if (!nr) {
> ieot_blocks = istate->cache_nr / THREAD_COST;
> - if (ieot_blocks < 1)
> - ieot_blocks = 1;
> cpus = online_cpus();
> if (ieot_blocks > cpus - 1)
> ieot_blocks = cpus - 1;
So, am I reading this correctly - you need cpus > 2 before an
IEOT extension block is written out?
OK.
ATB,
Ramsay Jones
> } else {
> ieot_blocks = nr;
> }
> - ieot = xcalloc(1, sizeof(struct index_entry_offset_table)
> - + (ieot_blocks * sizeof(struct index_entry_offset)));
> - ieot->nr = 0;
> - ieot_work = DIV_ROUND_UP(entries, ieot_blocks);
> +
> + /*
> + * no reason to write out the IEOT extension if we don't
> + * have enough blocks to utilize multi-threading
> + */
> + if (ieot_blocks > 1) {
> + ieot = xcalloc(1, sizeof(struct
> index_entry_offset_table)
> + + (ieot_blocks * sizeof(struct
> index_entry_offset)));
> + ieot->nr = 0;
> + ieot_work = DIV_ROUND_UP(entries, ieot_blocks);
> + }
> }
> #endif
>
>
Re: [PATCH] read-cache: fix division by zero core-dump
On 28/09/18 02:20, Ben Peart wrote:
>
>
> On 9/27/2018 6:24 PM, Ramsay Jones wrote:
>>
>> commit 225df8a468 ("ieot: add Index Entry Offset Table (IEOT)
>> extension", 2018-09-26) added a 'DIV_ROUND_UP(entries, ieot_blocks)
>> expression, where ieot_blocks was set to zero for a single cpu
>> platform. This caused an SIGFPE and a core dump in practically
>> every test in the test-suite, until test t4056-diff-order.sh, which
>> then went into an infinite loop!
>>
>> Signed-off-by: Ramsay Jones
>> ---
>>
>> Hi Ben,
>>
>> Could you please squash this into the relevant commits on your
>> 'bp/read-cache-parallel' branch. (The first hunk fixes a sparse
>> warning about using an integer as a NULL pointer).
>>
>
> Absolutely - thanks for the patch.
>
> I don't know how long it's been since I've been on a single core CPU - I'm
> sad for you. ;-)
Heh, don't be - whilst I do still have a single cpu laptop about
the place _somewhere_, I haven't booted it up in about 15 years! :-D
I used to regularly test git (and other software) on my old 32-bit
laptop (windows xp/Linux Mint 17.x, Core2 duo), but just lately
I have taken to using a 32-bit VM on my current laptop (4th gen i5)
instead. (The git test-suite would take approx 50 min to run on
the 32-bit hardware, whereas it only takes 8 min on the VM!).
I have configured the 32-bit VM with a single cpu, because when
the VM was configured with two cpus the git test-suite would take
longer to run (approx. 8 -> 10 min)! Taking more resources from
the host, but increasing the running time, didn't seem like a good
return. ;-)
Also, this is not the first time some multi-threaded code in git
has 'failed' by assuming more than one cpu, so ...
ATB,
Ramsay Jones
[PATCH] read-cache: fix division by zero core-dump
commit 225df8a468 ("ieot: add Index Entry Offset Table (IEOT)
extension", 2018-09-26) added a 'DIV_ROUND_UP(entries, ieot_blocks)
expression, where ieot_blocks was set to zero for a single cpu
platform. This caused an SIGFPE and a core dump in practically
every test in the test-suite, until test t4056-diff-order.sh, which
then went into an infinite loop!
Signed-off-by: Ramsay Jones
---
Hi Ben,
Could you please squash this into the relevant commits on your
'bp/read-cache-parallel' branch. (The first hunk fixes a sparse
warning about using an integer as a NULL pointer).
Thanks!
ATB,
Ramsay Jones
read-cache.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 6755d58877..40f096f70a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2141,7 +2141,7 @@ int do_read_index(struct index_state *istate, const char
*path, int must_exist)
size_t extension_offset = 0;
#ifndef NO_PTHREADS
int nr_threads, cpus;
- struct index_entry_offset_table *ieot = 0;
+ struct index_entry_offset_table *ieot = NULL;
#endif
if (istate->initialized)
@@ -2771,7 +2771,7 @@ static int do_write_index(struct index_state *istate,
struct tempfile *tempfile,
if (ieot_blocks < 1)
ieot_blocks = 1;
cpus = online_cpus();
- if (ieot_blocks > cpus - 1)
+ if (cpus > 1 && ieot_blocks > cpus - 1)
ieot_blocks = cpus - 1;
} else {
ieot_blocks = nr;
--
2.19.0
[PATCH] fetch: fix compilation warning
commit 440fc7c0729 ("fetch: replace string-list used as a look-up
table with a hashmap", 2018-09-25) renamed a string-list variable
(while adding a hashmap of the same name) and forgot to rename the
string-list variable in a call to string_list_clear().
Signed-off-by: Ramsay Jones
---
Hi Junio,
You probably already know, but I had to add this on top of the 'pu'
branch to get a clean compile tonight (your 'jc/war-on-string-list'
branch).
Thanks!
ATB,
Ramsay Jones
builtin/fetch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0a71953bc5..aea2e10364 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -391,7 +391,7 @@ static void find_non_local_tags(const struct ref *refs,
}
}
hashmap_free(_refs, 1);
- string_list_clear(_refs, 0);
+ string_list_clear(_refs_list, 0);
}
static struct ref *get_ref_map(struct remote *remote,
--
2.19.0
Re: [PATCH] fetch-object.h: add missing declaration (hdr-check)
On 21/09/18 17:21, Junio C Hamano wrote: > Ramsay Jones writes: > >> Signed-off-by: Ramsay Jones >> --- >> >> Hi Junio, >> >> This is the patch I needed for the current 'next' branch to get >> a clean 'hdr-check' > > Which means that this is a fix on top of jt/lazy-object-fetch-fix > topic, I think. > > Will apply there. Yes, indeed. Sorry, I should have added that information, rather than forcing you to look it up! (Similar comment on the userdiff.h patch as well) :( BTW, I notice that patch #9 (commit-reach.h: add missing declarations (hdr-check)) didn't make it onto 'pu' - was there something else I needed to do? (I am still in two minds about sending an RFC patch on-top of patch #9). Thanks! ATB, Ramsay Jones
Re: [PATCH 1/9] Makefile: add a hdr-check target
On 20/09/18 15:26, Junio C Hamano wrote:
> Ramsay Jones writes:
>
>> Commit ef3ca95475 ("Add missing includes and forward declarations",
>> 2018-08-15) resulted from the author employing a manual method to
>> create a C file consisting of a pair of pre-processor #include
>> lines (for 'git-compat-util.h' and a given toplevel header), and
>> fixing any resulting compiler errors or warnings.
>
> It makes sense to have tool do what we do not have to do manually.
>
> One thing that makes me wonder with the patch is that the new check
> command does not seem to need to see what is on CFLAGS and friends.
> Having seen that "make DEVELOPER=1" adds more -W... on the command
> line of the compiler and makes a build fail on a source that
> otherwise would build, I am wondering if there are some (subset of)
> options that the header-check command line wants to give to the
> compiler.
Yes, this was one of my first concerns (I even asked Elijah what
compiler options he used), but I was getting useful results without
passing CFLAGS, so I just ignored that issue ... :-D
[The 'on-the-fly' compilation units don't correspond to any _actual_
compilation unit, so it's not easy to use existing rules ... but we
could use 'hco' rule specific definitions to add flags, I suppose ...]
> Of course, there are also conditionally compiled sections of code,
> which are affected by the choice of -DMACRO=VALUE; how does this new
> feature handle that?
Indeed. This bothered me as well. The 'compat' directory does not
follow the 'usual pattern' of the main headers and is particularly
sensitive to the lack of various -DMACROs. I had initially included
_all_ sub-directories in the 'exclude list' (to follow what Elijah
had done), but then removed one at a time ...
I am open to suggestions for improvements. ;-)
ATB,
Ramsay Jones
Re: [PATCH 9/9] commit-reach.h: add missing declarations (hdr-check)
On 20/09/18 00:38, Derrick Stolee wrote:
> On 9/18/2018 8:15 PM, Ramsay Jones wrote:
>> Signed-off-by: Ramsay Jones
>> ---
>> commit-reach.h | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/commit-reach.h b/commit-reach.h
>> index 7d313e2975..f41d8f6ba3 100644
>> --- a/commit-reach.h
>> +++ b/commit-reach.h
>> @@ -1,12 +1,13 @@
>> #ifndef __COMMIT_REACH_H__
>> #define __COMMIT_REACH_H__
>> +#include "commit.h"
>> #include "commit-slab.h"
>> -struct commit;
>> struct commit_list;
>> -struct contains_cache;
>
> Interesting that you needed all of commit.h here, and these 'struct commit'
> and 'struct contains_cache' were not enough. Was there a reason you needed
> the entire header file instead of just adding a missing struct declaration?
Actually, the original version of this patch didn't add that
include! I changed my mind just before sending this series
out, since I felt the original patch was conflating two issues.
The reason for the #include of 'commit.h' in this patch, but
not with my original patch, has to to with the inline functions
used in the commit-slab implementation. My original patch used
the 'commit-slab-{decl,impl}.h' header files to sidestep the
need for the definition of 'struct commit'.
I have included an 'RFC on-top' patch below to show you what I
had in mind. NOTE: I had not finished writing the commit message
and you could argue that the 'implement' macro should go in the
ref-filter.c file, since that is were the contains_cache is
'defined and initialised'. I had not intended to send this to
the list ... :-D
Note that, if you compile with optimizations disabled, then
run this script:
$ cat -n dup-static.sh
1 #!/bin/sh
2
3 nm $1 | grep ' t ' | cut -d' ' -f3 | sort | uniq -c |
4sort -rn | grep -v ' 1'
$
$ ./dup-static.sh git | grep contains
24 init_contains_cache_with_stride
24 init_contains_cache
24 contains_cache_peek
24 contains_cache_at_peek
24 contains_cache_at
24 clear_contains_cache
$
you will find 24 copies of the commit-slab routines for the
contains_cache. Of course, when you enable optimizations again,
these duplicate static functions (mostly) disappear. (Two static
functions remain, the rest are actually inlined at -O2).
However, if you apply the patch below, you end up with all of
the functions in the contains_cache commit-slab implementation
as external functions. Some of those functions are never called,
so it's not necessarily a win ... ;-)
ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] commit-reach: use a shared commit-slab for the contains_cache
Commit a9f1f1f9f8 ("commit-slab.h: code split", 2018-05-19) separated
the commit-slab interface from its implementation, to allow for the
definition of a public commit-slab data structure. This enabled us to
avoid including the commit-slab implementation in a header file, which
could result in the replication of the commit-slab functions in each
compilation unit in which it was included.
Signed-off-by: Ramsay Jones
---
commit-reach.c | 3 +++
commit-reach.h | 6 +++---
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 622eeb313d..7223cacdd1 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -1,5 +1,6 @@
#include "cache.h"
#include "commit.h"
+#include "commit-slab-impl.h"
#include "commit-graph.h"
#include "decorate.h"
#include "prio-queue.h"
@@ -18,6 +19,8 @@
static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
+implement_shared_commit_slab(contains_cache, enum contains_result);
+
static int queue_has_nonstale(struct prio_queue *queue)
{
int i;
diff --git a/commit-reach.h b/commit-reach.h
index f41d8f6ba3..acf3b2d188 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -1,9 +1,9 @@
#ifndef __COMMIT_REACH_H__
#define __COMMIT_REACH_H__
-#include "commit.h"
-#include "commit-slab.h"
+#include "commit-slab-decl.h"
+struct commit;
struct commit_list;
struct ref_filter;
struct object_id;
@@ -55,7 +55,7 @@ enum contains_result {
CONTAINS_YES
};
-define_commit_slab(contains_cache, enum contains_result);
+define_shared_commit_slab(contains_cache, enum contains_result);
int commit_contains(struct ref_filter *filter, struct commit *commit,
struct commit_list *list, struct contains_cache *cache);
--
2.19.0
Re: [PATCH 1/9] Makefile: add a hdr-check target
On 19/09/18 18:49, Martin Ågren wrote: > Hi Ramsay, > > On Wed, 19 Sep 2018 at 02:07, Ramsay Jones > wrote: >> @@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE >> .PHONY: sparse $(SP_OBJ) >> sparse: $(SP_OBJ) >> >> +GEN_HDRS := command-list.h unicode-width.h > > Most of the things happening around here are obvious steps towards the > end-goal and seem to logically belong here, together. This one though, > "generated headers"(?) seems like it is more general in nature, so could > perhaps live somewhere else. Yes, generated headers, but no, not more general. ;-) I originally included those headers directly in the EXCEPT_HDRS macro, along with the list of excluded directories (which was much longer at one point). The 'command-list.h' is generated as part of the build (and so may or may not be part of the LIB_H macro), whereas the unicode-width.h header is only re-generated when someone runs the 'contrib/update-unicode/update_unicode.sh' script (presumably when a new standard version is announced) and commits the result. Both headers fail the 'hdr-check', although both generator scripts could be 'fixed' so that they passed. I just didn't think it was worth the effort - neither header was likely to be #included anywhere else. I guess I could eliminate that macro, absorbing it into EXCEPT_HDRS, if that would lead to less confusion ... [I suspect the fact that LIB_H (almost always) contains 'command-list.h' has not been noticed ... :-P ] > Actually, we have a variable `GENERATED_H` which seems to try to do more > or less the same thing. It lists just one file, though, command-list.h. > And unicode-width.h seems to be tracked in git.git. Hmm, GENERATED_H seems only to be used by the i18n part of the makefile and, as a result of its appearance in LIB_H, sometimes results in command-list.h appearing twice in LOCALIZED_C. (which is probably not a problem). ATB, Ramsay Jones > Maybe use `GENERATED_H` instead, and list unicode-width.h on the next > line instead? Or am I completely misreading "GEN_HDRS"? > >> +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% >> +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) >> +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) >> + >> +$(HCO): %.hco: %.h FORCE >> + $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc >> $< >> + >> +.PHONY: hdr-check $(HCO) >> +hdr-check: $(HCO) >> + >> .PHONY: style >> style: >> git clang-format --style file --diff --extensions c,h >
[PATCH] userdiff.h: add missing declaration (hdr-check)
Signed-off-by: Ramsay Jones
---
Hi Junio,
... and this is the patch I needed for the current 'pu' branch.
ATB,
Ramsay Jones
userdiff.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/userdiff.h b/userdiff.h
index dad3fc03c1..b072bfe89a 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -3,6 +3,8 @@
#include "notes-cache.h"
+struct index_state;
+
struct userdiff_funcname {
const char *pattern;
int cflags;
--
2.19.0
[PATCH] fetch-object.h: add missing declaration (hdr-check)
Signed-off-by: Ramsay Jones --- Hi Junio, This is the patch I needed for the current 'next' branch to get a clean 'hdr-check' ATB, Ramsay Jones fetch-object.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fetch-object.h b/fetch-object.h index d2f996d4e8..d6444caa5a 100644 --- a/fetch-object.h +++ b/fetch-object.h @@ -1,6 +1,8 @@ #ifndef FETCH_OBJECT_H #define FETCH_OBJECT_H +struct object_id; + void fetch_objects(const char *remote_name, const struct object_id *oids, int oid_nr); -- 2.19.0
[PATCH 9/9] commit-reach.h: add missing declarations (hdr-check)
Signed-off-by: Ramsay Jones --- commit-reach.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/commit-reach.h b/commit-reach.h index 7d313e2975..f41d8f6ba3 100644 --- a/commit-reach.h +++ b/commit-reach.h @@ -1,12 +1,13 @@ #ifndef __COMMIT_REACH_H__ #define __COMMIT_REACH_H__ +#include "commit.h" #include "commit-slab.h" -struct commit; struct commit_list; -struct contains_cache; struct ref_filter; +struct object_id; +struct object_array; struct commit_list *get_merge_bases_many(struct commit *one, int n, -- 2.19.0
[PATCH 8/9] delta-islands.h: add missing forward declarations (hdr-check)
Signed-off-by: Ramsay Jones --- delta-islands.h | 4 1 file changed, 4 insertions(+) diff --git a/delta-islands.h b/delta-islands.h index f9725730f4..b635cd07d8 100644 --- a/delta-islands.h +++ b/delta-islands.h @@ -1,6 +1,10 @@ #ifndef DELTA_ISLANDS_H #define DELTA_ISLANDS_H +struct object_id; +struct packing_data; +struct commit; + int island_delta_cmp(const struct object_id *a, const struct object_id *b); int in_same_island(const struct object_id *, const struct object_id *); void resolve_tree_islands(int progress, struct packing_data *to_pack); -- 2.19.0
[PATCH 7/9] midx.h: add missing forward declarations (hdr-check)
Signed-off-by: Ramsay Jones
---
midx.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/midx.h b/midx.h
index a210f1af2a..622ddac472 100644
--- a/midx.h
+++ b/midx.h
@@ -3,6 +3,9 @@
#include "repository.h"
+struct object_id;
+struct pack_entry;
+
struct multi_pack_index {
struct multi_pack_index *next;
--
2.19.0
[PATCH 6/9] refs/refs-internal.h: add missing declarations (hdr-check)
Signed-off-by: Ramsay Jones --- refs/refs-internal.h | 4 1 file changed, 4 insertions(+) diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 04425d6d1e..44d53672c7 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -1,8 +1,12 @@ #ifndef REFS_REFS_INTERNAL_H #define REFS_REFS_INTERNAL_H +#include "cache.h" +#include "refs.h" #include "iterator.h" +struct ref_transaction; + /* * Data structures and functions for the internal use of the refs * module. Code outside of the refs module should use only the public -- 2.19.0
[PATCH 5/9] refs/packed-backend.h: add missing declaration (hdr-check)
Signed-off-by: Ramsay Jones --- refs/packed-backend.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/refs/packed-backend.h b/refs/packed-backend.h index 640245d3b9..a01a0aff9c 100644 --- a/refs/packed-backend.h +++ b/refs/packed-backend.h @@ -1,6 +1,8 @@ #ifndef REFS_PACKED_BACKEND_H #define REFS_PACKED_BACKEND_H +struct ref_transaction; + /* * Support for storing references in a `packed-refs` file. * -- 2.19.0
[PATCH 4/9] refs/ref-cache.h: add missing declarations (hdr-check)
Signed-off-by: Ramsay Jones --- refs/ref-cache.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/refs/ref-cache.h b/refs/ref-cache.h index eda65e73ed..3bfb89d2b3 100644 --- a/refs/ref-cache.h +++ b/refs/ref-cache.h @@ -1,7 +1,10 @@ #ifndef REFS_REF_CACHE_H #define REFS_REF_CACHE_H +#include "cache.h" + struct ref_dir; +struct ref_store; /* * If this ref_cache is filled lazily, this function is used to load -- 2.19.0
[PATCH 3/9] ewah/ewok_rlw.h: add missing include (hdr-check)
Signed-off-by: Ramsay Jones --- ewah/ewok_rlw.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ewah/ewok_rlw.h b/ewah/ewok_rlw.h index 7cdfdd0c02..d487966935 100644 --- a/ewah/ewok_rlw.h +++ b/ewah/ewok_rlw.h @@ -19,6 +19,8 @@ #ifndef __EWOK_RLW_H__ #define __EWOK_RLW_H__ +#include "ewok.h" + #define RLW_RUNNING_BITS (sizeof(eword_t) * 4) #define RLW_LITERAL_BITS (sizeof(eword_t) * 8 - 1 - RLW_RUNNING_BITS) -- 2.19.0
[PATCH 2/9] json-writer.h: add missing include (hdr-check)
Signed-off-by: Ramsay Jones
---
json-writer.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/json-writer.h b/json-writer.h
index fc18acc7d9..83906b09c1 100644
--- a/json-writer.h
+++ b/json-writer.h
@@ -42,6 +42,8 @@
* of the given strings.
*/
+#include "strbuf.h"
+
struct json_writer
{
/*
--
2.19.0
[PATCH 1/9] Makefile: add a hdr-check target
Commit ef3ca95475 ("Add missing includes and forward declarations",
2018-08-15) resulted from the author employing a manual method to
create a C file consisting of a pair of pre-processor #include
lines (for 'git-compat-util.h' and a given toplevel header), and
fixing any resulting compiler errors or warnings.
Add a Makefile target to automate this process. This implementation
relies on the '-include' and '-xc' arguments to the 'gcc' and 'clang'
compilers, which allows us to effectively create the required C
compilation unit on-the-fly. This limits the portability of this
solution to those systems which have such a compiler.
The new 'hdr-check' target can be used to check most header files in
the project (for various reasons, the 'compat' and 'xdiff' directories
are not included). Also, note that individual header files can be
checked directly using the '.hco' extension (read: Hdr-Check Object)
like so:
$ make config.hco
HDR config.h
$
Signed-off-by: Ramsay Jones
---
Makefile | 12
1 file changed, 12 insertions(+)
diff --git a/Makefile b/Makefile
index b567ccca45..835030e22b 100644
--- a/Makefile
+++ b/Makefile
@@ -1793,6 +1793,7 @@ ifndef V
QUIET_MSGFMT = @echo ' ' MSGFMT $@;
QUIET_GCOV = @echo ' ' GCOV $@;
QUIET_SP = @echo ' ' SP $<;
+ QUIET_HDR = @echo ' ' HDR $<;
QUIET_RC = @echo ' ' RC $@;
QUIET_SUBDIR0 = +@subdir=
QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \
@@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
.PHONY: sparse $(SP_OBJ)
sparse: $(SP_OBJ)
+GEN_HDRS := command-list.h unicode-width.h
+EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
+CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
+HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
+
+$(HCO): %.hco: %.h FORCE
+ $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
+
+.PHONY: hdr-check $(HCO)
+hdr-check: $(HCO)
+
.PHONY: style
style:
git clang-format --style file --diff --extensions c,h
--
2.19.0
Subject: [PATCH 0/9] hdr-check
This series follows on from a quick "just before bedtime" exercise recently[1]. The new 'hdr-check' target essentially automates what Elijah did by hand. I tend to run: $ make -h hdr-check >hcout 2>&1 $ vim hcout ... and I only just realised that if somebody wanted to add this to an travis CI job (it won't be me), then it would be a good idea to add '-Werror' to the compiler command; otherwise 'make' would not exit with an error if the compiler only issues warnings. This series was built on the current 'master' branch (@2d3b1c576c), although patches #1-6 apply on v2.19.0. (The last three patches used to be on 'next' until last night!). If I merge this to 'next', I have to add an additional patch for a clean 'hdr-check'. Exactly the same comment for the current 'pu' branch. [1] https://public-inbox.org/git/b8553a50-6b97-2b45-2f7b-cfe257654...@ramsayjones.plus.com/ ATB, Ramsay Jones Ramsay Jones (9): Makefile: add a hdr-check target json-writer.h: add missing include (hdr-check) ewah/ewok_rlw.h: add missing include (hdr-check) refs/ref-cache.h: add missing declarations (hdr-check) refs/packed-backend.h: add missing declaration (hdr-check) refs/refs-internal.h: add missing declarations (hdr-check) midx.h: add missing forward declarations (hdr-check) delta-islands.h: add missing forward declarations (hdr-check) commit-reach.h: add missing declarations (hdr-check) Makefile | 12 commit-reach.h| 5 +++-- delta-islands.h | 4 ewah/ewok_rlw.h | 2 ++ json-writer.h | 2 ++ midx.h| 3 +++ refs/packed-backend.h | 2 ++ refs/ref-cache.h | 3 +++ refs/refs-internal.h | 4 9 files changed, 35 insertions(+), 2 deletions(-) -- 2.19.0
Re: [PATCH] read-cache.c: fix a sparse warning
On 17/09/18 17:27, Ramsay Jones wrote: > > > On 17/09/18 15:15, Ben Peart wrote: >> >> >> On 9/16/2018 3:17 AM, Eric Sunshine wrote: >>> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones >>> wrote: >>>> At one time, the POSIX standard required the type used to represent >>>> a thread handle (pthread_t) be an arithmetic type. This is no longer >>>> the case, probably because different platforms used to regularly >>>> ignore that requirement. For example, on cygwin a pthread_t is a >>>> pointer to a structure (a quite common choice), whereas on Linux it >>>> is defined as an 'unsigned long int'. >>>> >>>> On cygwin, but not on Linux, 'sparse' currently complains about an >>>> initialiser used on a 'struct load_index_extensions' variable, whose >>>> first field may be a pthread handle (if not compiled with NO_PTHREADS >>>> set). >>>> >>>> In order to fix the warning, move the (conditional) pthread field to >>>> the end of the struct and change the initialiser to use a NULL, since >>>> the new (unconditional) first field is a pointer type. >>>> >>>> Signed-off-by: Ramsay Jones >>>> --- >>>> If you need to re-roll your 'bp/read-cache-parallel' branch, could you >>>> please squash this into the relevant patch (commit a090af334, >>>> "read-cache: load cache extensions on a worker thread", 2018-09-12). >>> >>> The information contained in this commit message is so useful that it >>> might make sense to plop this patch at the end of the series rather >>> than merely squashing it in. (Or, if it is squashed, include the above >>> explanation in the commit message of the appropriate patch.) >>> >> >> I'm happy to squash it in if I end up re-rolling the patch series. I'll >> include the information in the commit message above as a comment so that it >> is in close proximity to the code impacted. >> > > I will be happy with whatever decision you take regarding whether > to squash this in or add it on top of your series. However, if you > do squash it in, please don't add the commit message info as a > comment to the code. No matter how you word it, I can't imagine > that it would be anything but superfluous - the kind of comment > that would be removed after review! ;-) > > The information in the commit message about pthread_t, which I > thought was common knowledge, was not really the main point of > the argument supporting the patch. (Search for "How do I print > a pthread_t", for variations on this theme). > > The main point for me: don't conditionally include a field at the > beginning of a structure and then use an initialiser in a variable > declaration. (Unless, I suppose, the first unconditional field had > the same type - but probably not not even then!) > > The fact that the conditionally included field itself had an 'opaque' > type was just an additional complication. BTW, I just noticed that you explicitly initialise each field of that structure (not surprising), so an even simpler solution is to simply remove the unneeded initialiser! ;-) ATB, Ramsay Jones
Re: [PATCH] read-cache.c: fix a sparse warning
On 17/09/18 15:15, Ben Peart wrote: > > > On 9/16/2018 3:17 AM, Eric Sunshine wrote: >> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones >> wrote: >>> At one time, the POSIX standard required the type used to represent >>> a thread handle (pthread_t) be an arithmetic type. This is no longer >>> the case, probably because different platforms used to regularly >>> ignore that requirement. For example, on cygwin a pthread_t is a >>> pointer to a structure (a quite common choice), whereas on Linux it >>> is defined as an 'unsigned long int'. >>> >>> On cygwin, but not on Linux, 'sparse' currently complains about an >>> initialiser used on a 'struct load_index_extensions' variable, whose >>> first field may be a pthread handle (if not compiled with NO_PTHREADS >>> set). >>> >>> In order to fix the warning, move the (conditional) pthread field to >>> the end of the struct and change the initialiser to use a NULL, since >>> the new (unconditional) first field is a pointer type. >>> >>> Signed-off-by: Ramsay Jones >>> --- >>> If you need to re-roll your 'bp/read-cache-parallel' branch, could you >>> please squash this into the relevant patch (commit a090af334, >>> "read-cache: load cache extensions on a worker thread", 2018-09-12). >> >> The information contained in this commit message is so useful that it >> might make sense to plop this patch at the end of the series rather >> than merely squashing it in. (Or, if it is squashed, include the above >> explanation in the commit message of the appropriate patch.) >> > > I'm happy to squash it in if I end up re-rolling the patch series. I'll > include the information in the commit message above as a comment so that it > is in close proximity to the code impacted. > I will be happy with whatever decision you take regarding whether to squash this in or add it on top of your series. However, if you do squash it in, please don't add the commit message info as a comment to the code. No matter how you word it, I can't imagine that it would be anything but superfluous - the kind of comment that would be removed after review! ;-) The information in the commit message about pthread_t, which I thought was common knowledge, was not really the main point of the argument supporting the patch. (Search for "How do I print a pthread_t", for variations on this theme). The main point for me: don't conditionally include a field at the beginning of a structure and then use an initialiser in a variable declaration. (Unless, I suppose, the first unconditional field had the same type - but probably not not even then!) The fact that the conditionally included field itself had an 'opaque' type was just an additional complication. ATB, Ramsay Jones
[PATCH] read-cache.c: fix a sparse warning
At one time, the POSIX standard required the type used to represent
a thread handle (pthread_t) be an arithmetic type. This is no longer
the case, probably because different platforms used to regularly
ignore that requirement. For example, on cygwin a pthread_t is a
pointer to a structure (a quite common choice), whereas on Linux it
is defined as an 'unsigned long int'.
On cygwin, but not on Linux, 'sparse' currently complains about an
initialiser used on a 'struct load_index_extensions' variable, whose
first field may be a pthread handle (if not compiled with NO_PTHREADS
set).
In order to fix the warning, move the (conditional) pthread field to
the end of the struct and change the initialiser to use a NULL, since
the new (unconditional) first field is a pointer type.
Signed-off-by: Ramsay Jones
---
Hi Ben,
If you need to re-roll your 'bp/read-cache-parallel' branch, could you
please squash this into the relevant patch (commit a090af334,
"read-cache: load cache extensions on a worker thread", 2018-09-12).
Thanks.
ATB,
Ramsay Jones
read-cache.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index b27dc01696..6254291742 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1908,13 +1908,13 @@ static void write_eoie_extension(struct strbuf *sb,
git_hash_ctx *eoie_context,
struct load_index_extensions
{
-#ifndef NO_PTHREADS
- pthread_t pthread;
-#endif
struct index_state *istate;
const char *mmap;
size_t mmap_size;
unsigned long src_offset;
+#ifndef NO_PTHREADS
+ pthread_t pthread;
+#endif
};
static void *load_index_extensions(void *data)
@@ -2145,7 +2145,7 @@ int do_read_index(struct index_state *istate, const char
*path, int must_exist)
const struct cache_header *hdr;
const char *mmap;
size_t mmap_size;
- struct load_index_extensions p = { 0 };
+ struct load_index_extensions p = { NULL };
unsigned long extension_offset = 0;
#ifndef NO_PTHREADS
int cpus, nr_threads;
--
2.19.0
[PATCH] midx.c: mark a file-local symbol as static
Signed-off-by: Ramsay Jones
---
Hi Derrick,
If you need to re-roll your 'ds/multi-pack-verify' branch, could you
please squash this into the relevant patch (commit 64cbf3df21,
"multi-pack-index: add 'verify' verb", 2018-09-13).
[noticed by sparse].
Thanks.
ATB,
Ramsay Jones
midx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/midx.c b/midx.c
index 4d4c930522..713d6f9dde 100644
--- a/midx.c
+++ b/midx.c
@@ -926,7 +926,7 @@ void clear_midx_file(const char *object_dir)
free(midx);
}
-int verify_midx_error;
+static int verify_midx_error;
static void midx_report(const char *fmt, ...)
{
--
2.19.0
Re: [PATCH 6/9] submodule.c: do not copy around submodule list
On 12/09/18 00:49, Stefan Beller wrote: > 'calculate_changed_submodule_paths' uses a local list to compute the > changed submodules, and then produces the result by copying appropriate > items into the result list. > > Instead use the result list directly and prune items afterwards > using string_list_remove_empty_items. > > By doin so we'll have access to the util pointer for longer that s/doin/doing/ ATB, Ramsay Jones
Re: [PATCH 1/9] string-list: add string_list_{pop, last} functions
On 12/09/18 00:49, Stefan Beller wrote:
> Add a few functions to allow a string-list to be used as a stack:
>
> - string_list_last() lets a caller peek the string_list_item at the
>end of the string list. The caller needs to be aware that it is
>borrowing a pointer, which can become invalid if/when the
>string_list is resized.
>
> - string_list_pop() removes the string_list_item at the end of
>the string list.
>
> - _pop usually has a friend _push. This role is taken by
> string_list_append already, as they are not symmetrical
> in our code base: _append returns the pointer, such that
> adding a util is easy, but _pop doesn't return such a pointer.
>
> You can use them in this pattern:
>
> while (list.nr) {
> struct string_list_item *item = string_list_last();
>
> work_on(item);
> string_list_pop();
string_list_pop() takes a second int parameter (free_util).
ATB,
Ramsay Jones
Re: git silently ignores include directive with single quotes
On 08/09/18 22:14, Jeff King wrote: > On Sat, Sep 08, 2018 at 09:54:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> The reason missing includes are ignored is that the way this is expected >> to be used is e.g.: >> >> [include] >> path ~/.gitconfig.work >> >> Where .gitconfig.work is some configuration you're going to drop into >> place on your $dayjob servers, but not on your personal machine, even >> though you sync the same ~/.gitconfig everywhere. >> >> A lot of people who use includes rely on this, but I see from this >> thread this should be better documented. > > Right, this was an intentional choice at the time the feature was added, > to support this kind of feature. I'd note also that it mirrors other > misspelled keys. E.g.: > > [include] > psth = whatever > [snip] > That said, it _does_ behave the same and people are likely depending on > it at this point. So if we introduce a warning, for example, there needs > to be some way to suppress it. > > Probably: > > [include] > warnOnMissing = false > path = ... I was going to suggest, inspired by Makefile syntax, that [-include] would not complain if the file was missing ... except, of course, it's too late for that! ;-) I suppose [+include] could complain if the file is missing instead, ... dunno. ATB, Ramsay Jones
Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability
On 31/08/18 01:54, Jeff King wrote:
> On Fri, Aug 31, 2018 at 12:49:39AM +0100, Ramsay Jones wrote:
>
>> On 30/08/18 21:14, Junio C Hamano wrote:
>>> Jeff King writes:
>>>
>>>> I suppose so. I don't think I've _ever_ used distclean, and I only
>>>> rarely use "clean" (a testament to our Makefile's efforts to accurately
>>>> track dependencies). I'd usually use "git clean" when I want something
>>>> pristine (because I don't want to trust the Makefile at all).
>>>
>>> I do not trust "git clean" all that much, and pre-cleaning with
>>> "make distclean" and then running "git clean -x" has become my bad
>>> habit. I jump around quite a bit during the day, which would end up
>>> littering the working tree with *.o files that are only known to one
>>> but not both of {maint,pu}/Makefile's distclean rules. I even do
>>> "for i in pu maint master next; do git checkout $i; make distclean; done"
>>> sometimes before running "git clean -x" ;-)
>>>
>>
>> 'git clean -x' always removes _way_ more than I want it
>> to - in particular, I lost my config.mak more than once.
>
> Heh. I have done that, too, but fortunately mine is a symlink to a copy
> that is held in a git repository. ;)
:-D
Now, why didn't I think of that! ;-)
ATB,
Ramsay Jones
Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability
On 30/08/18 21:14, Junio C Hamano wrote:
> Jeff King writes:
>
>> I suppose so. I don't think I've _ever_ used distclean, and I only
>> rarely use "clean" (a testament to our Makefile's efforts to accurately
>> track dependencies). I'd usually use "git clean" when I want something
>> pristine (because I don't want to trust the Makefile at all).
>
> I do not trust "git clean" all that much, and pre-cleaning with
> "make distclean" and then running "git clean -x" has become my bad
> habit. I jump around quite a bit during the day, which would end up
> littering the working tree with *.o files that are only known to one
> but not both of {maint,pu}/Makefile's distclean rules. I even do
> "for i in pu maint master next; do git checkout $i; make distclean; done"
> sometimes before running "git clean -x" ;-)
>
'git clean -x' always removes _way_ more than I want it
to - in particular, I lost my config.mak more than once.
So, no I don't trust it. ;-)
ATB,
Ramsay Jones
Re: Missing Tagger Entry
On 30/08/18 02:56, Stephen & Linda Smith wrote: > I am getting the following warning when runing a git fsck command against tag > v0.99. Yes, that is expected. > > $ git --version > git version 2.18.0 > > $ git fsck > checking object directories: 100% (256/256), done. > warning in tag d6602ec5194c87b0fc87103ca4d67251c76f233a: missingTaggerEntry: > invalid format - expected 'tagger' line > Checking objects: 100% (254339/254339), done. > Checking connectivity: 254329, done. This tag is so old that it dates to before a change in the tag object format - in particular, before the 'tagger' line was included. Viz: $ git cat-file tag v0.99 object a3eb250f996bf5e12376ec88622c4ccaabf20ea8 type commit tag v0.99 Test-release for wider distribution. I'll make the first public RPM's etc, thus the tag. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQBC0b9oF3YsRnbiHLsRAlUUAKCJEyvw8tewGFKd/A3aCd82Wi/zAgCgl7z4 GYPjO+Xio0IvuEYsrhFc2KI= =TVVN -END PGP SIGNATURE- $ Note the lack of a 'tagger' line, unlike a more up-to-date tag: $ git cat-file tag v2.18.0 object 53f9a3e157dbbc901a02ac2c73346d375e24978c type commit tag v2.18.0 tagger Junio C Hamano 1529600438 -0700 Git 2.18 -BEGIN PGP SIGNATURE- iQIzBAABCAAdFiEE4fA2sf7nIh/HeOzvsLXohpav5ssFAlsr2bYACgkQsLXohpav 5suqEhAAgDu2A1n9G7ik+HdKoH2VNGwDqaRu/3k8znLPR6NmcOpHqopCgaxPYN4T gH69ff+8Le8NiOYcoWaOE2WdpGGY9Gu12N65MpxYbEhehEGo7ze4T8jDNlHz7q5B XC55FKHAwqy51NtdzvqNgsptc3bASy+ThxNM5XS0GSeqz00ublquHhiGTzhkBKm2 KbexWhGWjzq0zP+wOrRIX4zU1lAOHXzjVV7G8vo3pTcg+GgK0BmiAz8zmOlef2au SYlU2LJCcQFm12j7pdDx42qCfZYM3QB0vJkHAcEdKYlcSEKRYUdOEnIQHxHwPPvB A/uogytfeExnpBd/aHA/YBKlr8FNBMZeDKGHiwxWsBK5yExxfelIFnOg27YBIxl2 zzbMnHubBqHs5luo2Yv9JmFCbmuqV6ei6qgDKn2BXtJkuXVqYI1FYuKQyO26b3cz C6hF5n3OIixL0wv1S+44QqDEc/ss8kvqosT2Ypjd56dNeZripTe3jC+bqUouHblD NGaUn+V2YGBKc3rPw1UE3WnXgqOcbyvxn8AoZIKhJveaq7z89CbcvQYpqNjGhmrp OvqSVG3NUoOKGXiMAg4/a4wx6JWTyu5SLHY269tC3cPfxQkD3br6hMsBy+AXrCwq 5yk6A3kQ2d6S9QfbWr6PGT7FI/AhG9CftFXPjpF0h9W9xbPJfkE= =ResM -END PGP SIGNATURE- $ You can suppress this warning by using an fsck 'skiplist': 1) Add the following lines to your .git/config file: [fsck] skiplist = .git/skip 2) Add the object-id of the v0.99 tag to the skiplist file: $ echo d6602ec5194c87b0fc87103ca4d67251c76f233a >.git/skip Hope this helps. ATB, Ramsay Jones
Re: [PATCHv4 0/6] Add missing includes and forward declares
On 15/08/18 18:54, Elijah Newren wrote:
> This series fixes compilation errors when using a simple test.c file that
> includes git-compat-util.h and then exactly one other header (and repeating
> this for different headers of git).
>
[snip]
> 1: f7d50cef3b ! 1: e6a93208b2 Add missing includes and forward declares
> @@ -1,6 +1,13 @@
> Author: Elijah Newren
>
> -Add missing includes and forward declares
> +Add missing includes and forward declarations
> +
> +I looped over the toplevel header files, creating a temporary
> two-line C
> +program for each consisting of
> + #include "git-compat-util.h"
> + #include $HEADER
> +This patch is the result of manually fixing errors in compiling those
> +tiny programs.
As a quick ("just before bedtime") exercise, I tried adding
a Makefile target to perform a similar check. The result is
given below, but I haven't had time to look too closely at
the results:
$ make -k hdr-check >zzz 2>&1
$ grep error zzz | wc -l
18
$ grep warning zzz | wc -l
21
$ grep error zzz | cut -d: -f1 | grep -v make | uniq -c | sort -nr
11 refs/refs-internal.h
2 unicode-width.h
2 json-writer.h
1 refs/ref-cache.h
1 commit-slab-impl.h
$ grep warning zzz | cut -d: -f1 | grep -v make | uniq -c | sort -nr
7 refs/refs-internal.h
5 delta-islands.h
2 unicode-width.h
2 midx.h
2 commit-reach.h
1 refs/ref-cache.h
1 refs/packed-backend.h
1 pack-bitmap.h
$
BTW, I happen to be on the 'pu' branch.
I think some of the errors are due to missing compiler flags
(-I, -D, etc); which flags did you pass to the compiler?
Well, it killed 15min. before bed! ;-)
ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] Makefile: add a hdr-check target
Signed-off-by: Ramsay Jones
---
Makefile | 10 ++
1 file changed, 10 insertions(+)
diff --git a/Makefile b/Makefile
index 9923db888c..798396c52e 100644
--- a/Makefile
+++ b/Makefile
@@ -2684,6 +2684,16 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
.PHONY: sparse $(SP_OBJ)
sparse: $(SP_OBJ)
+EXCEPT_HDRS := ./compat% ./xdiff% ./ewah%
+CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(LIB_H))
+HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
+
+$(HCO): %.hco: %.h FORCE
+ $(CC) -Wall -include git-compat-util.h -I. -o /dev/null -c -xc $<
+
+.PHONY: hdr-check $(HCO)
+hdr-check: $(HCO)
+
.PHONY: style
style:
git clang-format --style file --diff --extensions c,h
--
2.18.0
Re: [PATCH v4 1/7] Add delta-islands.{c,h}
On 13/08/18 04:33, Christian Couder wrote:
> On Mon, Aug 13, 2018 at 3:14 AM, Ramsay Jones
[snip]
>> And neither the sha1 or str hash-maps are destroyed here.
>> (That is not necessarily a problem, of course! ;-) )
>
> The instances are declared as static:
>
> static khash_sha1 *island_marks;
>
> static kh_str_t *remote_islands;
>
> so it maybe ok.
Yes, that's fine.
>
>>> +struct island_bitmap {
>>> + uint32_t refcount;
>>> + uint32_t bits[];
>>
>> Use FLEX_ARRAY here? We are slowly moving toward requiring
>> certain C99 features, but I can't remember a flex array
>> weather-balloon patch.
>
> This was already discussed by Junio and Peff there:
>
> https://public-inbox.org/git/20180727130229.gb18...@sigill.intra.peff.net/
>
That is a fine discussion, without a firm conclusion, but I don't
think you can simply do nothing here:
$ cat -n junk.c
1#include
2
3struct island_bitmap {
4uint32_t refcount;
5uint32_t bits[];
6};
7
$ gcc --std=c89 --pedantic -c junk.c
junk.c:5:11: warning: ISO C90 does not support flexible array members
[-Wpedantic]
uint32_t bits[];
^~~~
$ gcc --std=c99 --pedantic -c junk.c
$
>>> +};
>
>>> +int in_same_island(const struct object_id *trg_oid, const struct object_id
>>> *src_oid)
>>
>> Hmm, what does the trg_ prefix stand for?
>>
>>> +{
>>> + khiter_t trg_pos, src_pos;
>>> +
>>> + /* If we aren't using islands, assume everything goes together. */
>>> + if (!island_marks)
>>> + return 1;
>>> +
>>> + /*
>>> + * If we don't have a bitmap for the target, we can delta it
>>
>> ... Ah, OK, trg_ => target.
>
> I am ok to replace "trg" with "target" (or maybe "dst"? or something
> else) and "src" with "source" if you think it would make things
> clearer.
If it had been dst_ (or target), I would not have had a 'huh?'
moment, but it is not all that important.
>
>>> +static void add_ref_to_island(const char *island_name, const struct
>>> object_id *oid)
>>> +{
>>> + uint64_t sha_core;
>>> + struct remote_island *rl = NULL;
>>> +
>>> + int hash_ret;
>>> + khiter_t pos = kh_put_str(remote_islands, island_name, _ret);
>>> +
>>> + if (hash_ret) {
>>> + kh_key(remote_islands, pos) = xstrdup(island_name);
>>> + kh_value(remote_islands, pos) = xcalloc(1, sizeof(struct
>>> remote_island));
>>> + }
>>> +
>>> + rl = kh_value(remote_islands, pos);
>>> + oid_array_append(>oids, oid);
>>> +
>>> + memcpy(_core, oid->hash, sizeof(uint64_t));
>>> + rl->hash += sha_core;
>>
>> Hmm, so the first 64-bits of the oid of each ref that is part of
>> this island is added together as a 'hash' for the island. And this
>> is used to de-duplicate the islands? Any false positives? (does it
>> matter - it would only affect performance, not correctness, right?)
>
> I would think that a false positive from pure chance is very unlikely.
> We would need to approach billions of delta islands (as 2 to the power
> 64/2 is in the order of billions) for the probability to be
> significant. GitHub has less than 50 millions users and it is very
> unlikely that a significant proportion of these users will fork the
> same repo.
>
> Now if there is a false positive because two forks have exactly the
> same refs, then it is not a problem if they are considered the same,
> because they are actually the same.
Yep, good point.
ATB,
Ramsay Jones
Re: [PATCH v4 1/7] Add delta-islands.{c,h}
; + if (list[ref]->hash == list[src]->hash)
> + continue;
> +
> + if (src != dst)
> + list[dst] = list[src];
> +
> + dst++;
> + }
> + island_count = dst;
> + }
> +
> + island_bitmap_size = (island_count / 32) + 1;
> + core = get_core_island();
> +
> + for (i = 0; i < island_count; ++i) {
> + mark_remote_island_1(list[i], core && list[i]->hash ==
> core->hash);
> + }
> +
> + free(list);
> +}
> +
> +void load_delta_islands(void)
> +{
> + island_marks = kh_init_sha1();
> + remote_islands = kh_init_str();
> +
> + git_config(island_config_callback, NULL);
> + for_each_ref(find_island_for_ref, NULL);
> + deduplicate_islands();
> +
> + fprintf(stderr, _("Marked %d islands, done.\n"), island_counter);
> +}
> +
> +void propagate_island_marks(struct commit *commit)
> +{
> + khiter_t pos = kh_get_sha1(island_marks, commit->object.oid.hash);
> +
> + if (pos < kh_end(island_marks)) {
> + struct commit_list *p;
> + struct island_bitmap *root_marks = kh_value(island_marks, pos);
> +
> + parse_commit(commit);
> + set_island_marks(_commit_tree(commit)->object, root_marks);
> + for (p = commit->parents; p; p = p->next)
> + set_island_marks(>item->object, root_marks);
> + }
> +}
> +
> +int compute_pack_layers(struct packing_data *to_pack)
> +{
> + uint32_t i;
> +
> + if (!core_island_name || !island_marks)
> + return 1;
> +
> + for (i = 0; i < to_pack->nr_objects; ++i) {
> + struct object_entry *entry = _pack->objects[i];
> + khiter_t pos = kh_get_sha1(island_marks, entry->idx.oid.hash);
> +
> + entry->layer = 1;
> +
> + if (pos < kh_end(island_marks)) {
> + struct island_bitmap *bitmap = kh_value(island_marks,
> pos);
> +
> + if (island_bitmap_get(bitmap, island_counter_core))
> + entry->layer = 0;
> + }
> + }
> +
> + return 2;
> +}
> diff --git a/delta-islands.h b/delta-islands.h
> new file mode 100644
> index 00..f9725730f4
> --- /dev/null
> +++ b/delta-islands.h
> @@ -0,0 +1,11 @@
> +#ifndef DELTA_ISLANDS_H
> +#define DELTA_ISLANDS_H
> +
> +int island_delta_cmp(const struct object_id *a, const struct object_id *b);
> +int in_same_island(const struct object_id *, const struct object_id *);
> +void resolve_tree_islands(int progress, struct packing_data *to_pack);
> +void load_delta_islands(void);
> +void propagate_island_marks(struct commit *commit);
> +int compute_pack_layers(struct packing_data *to_pack);
> +
> +#endif /* DELTA_ISLANDS_H */
> diff --git a/pack-objects.h b/pack-objects.h
> index edf74dabdd..8eecd67991 100644
> --- a/pack-objects.h
> +++ b/pack-objects.h
> @@ -100,6 +100,10 @@ struct object_entry {
> unsigned type_:TYPE_BITS;
> unsigned no_try_delta:1;
> unsigned in_pack_type:TYPE_BITS; /* could be delta */
> +
> + unsigned int tree_depth; /* should be repositioned for packing? */
> + unsigned char layer;
> +
> unsigned preferred_base:1; /*
> * we do not pack this, but is available
> * to be used as the base object to delta
>
Sorry, I spent so long reading this patch, I have run out of
time tonight (and I am busy tomorrow) to read the rest of the
series.
ATB,
Ramsay Jones
function get_delta_base() is a file-local symbol
Hi Christian, My static-check.pl script has pinged me about the get_delta_base() symbol from packfile.[co]. The first patch from your 'cc/delta-islands' branch exports this symbol, saying that it will soon be called from outside packfile.c. As far as I can tell, no other patch in that series adds any such call. Do you have plans to extend that series with additional patches which will add such calls? ATB, Ramsay Jones
[PATCH] rebase: fix a sparse 'plain integer as NULL pointer' warning
Signed-off-by: Ramsay Jones
---
Hi Pratik,
If you need to re-roll your 'pk/rebase-in-c-4-opts' branch, could
you please squash this into the relevant patch (commit b0721e7b48,
"builtin rebase: support `-C` and `--whitespace=`", 2018-08-08).
Thanks!
ATB,
Ramsay Jones
builtin/rebase.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 42f5884ce9..fa7c5582f8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -776,7 +776,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
N_("gpg-sign?"), N_("GPG-sign commits")),
OPT_STRING_LIST(0, "whitespace", ,
N_("whitespace"), N_("passed to 'git apply'")),
- OPT_SET_INT('C', 0, _c, N_("passed to 'git apply'"),
+ OPT_SET_INT('C', NULL, _c, N_("passed to 'git apply'"),
REBASE_AM),
OPT_BOOL(0, "autostash", ,
N_("automatically stash/stash pop before and after")),
--
2.18.0
Re: [PATCHv2 3/6] Move definition of enum branch_track from cache.h to branch.h
On 11/08/18 21:50, Elijah Newren wrote:
> 'branch_track' feels more closely related to branching, and it is
> needed later in branch.h; rather than #include'ing cache.h in branch.h
> for this small enum, just move the enum and the external declaration
> for git_branch_track to branch.h.
>
> Signed-off-by: Elijah Newren
> ---
> branch.h | 11 +++
> cache.h | 10 --
> config.c | 1 +
> environment.c | 1 +
> 4 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/branch.h b/branch.h
> index 7d9b330eba..5cace4581f 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -3,6 +3,17 @@
>
> struct strbuf;
>
> +enum branch_track {
> + BRANCH_TRACK_UNSPECIFIED = -1,
> + BRANCH_TRACK_NEVER = 0,
> + BRANCH_TRACK_REMOTE,
> + BRANCH_TRACK_ALWAYS,
> + BRANCH_TRACK_EXPLICIT,
> + BRANCH_TRACK_OVERRIDE
> +};
> +
> +extern enum branch_track git_branch_track;
> +
> /* Functions for acting on the information about branches. */
>
> /*
> diff --git a/cache.h b/cache.h
> index 8dc7134f00..a1c0c594fb 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -919,15 +919,6 @@ enum log_refs_config {
> };
> extern enum log_refs_config log_all_ref_updates;
>
> -enum branch_track {
> - BRANCH_TRACK_UNSPECIFIED = -1,
> - BRANCH_TRACK_NEVER = 0,
> - BRANCH_TRACK_REMOTE,
> - BRANCH_TRACK_ALWAYS,
> - BRANCH_TRACK_EXPLICIT,
> - BRANCH_TRACK_OVERRIDE
> -};
> -
> enum rebase_setup_type {
> AUTOREBASE_NEVER = 0,
> AUTOREBASE_LOCAL,
> @@ -944,7 +935,6 @@ enum push_default_type {
> PUSH_DEFAULT_UNSPECIFIED
> };
>
> -extern enum branch_track git_branch_track;
> extern enum rebase_setup_type autorebase;
> extern enum push_default_type push_default;
>
> diff --git a/config.c b/config.c
> index 66645047eb..b60d7c0308 100644
> --- a/config.c
> +++ b/config.c
> @@ -5,6 +5,7 @@
> * Copyright (C) Johannes Schindelin, 2005
> *
> */
> +#include "branch.h"
git-compat-util.h, cache.h or builtin.h _must_ be the first
header file #included in a C file, so this new #include of
the "branch.h" header should be moved ...
> #include "cache.h"
... after here.
> #include "config.h"
> #include "repository.h"
> diff --git a/environment.c b/environment.c
> index 6cf0079389..920362900c 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -7,6 +7,7 @@
> * even if you might want to know where the git directory etc
> * are.
> */
> +#include "branch.h"
ditto
> #include "cache.h"
> #include "repository.h"
> #include "config.h"
>
ATB,
Ramsay Jones
Re: [PATCH 2/2] fsck: use oidset for skiplist
On 11/08/18 16:47, René Scharfe wrote:
> Object IDs to skip are stored in a shared static oid_array. Lookups do
> a binary search on the sorted array. The code checks if the object IDs
> are already in the correct order while loading and skips sorting in that
> case.
>
> Simplify the code by using an oidset instead. Memory usage is a bit
> higher, but lookups are done in constant time and there is no need to
> worry about any sort order.
>
> Embed the oidset into struct fsck_options to make its ownership clear
> (no hidden sharing) and avoid unnecessary pointer indirection.
>
> Signed-off-by: Rene Scharfe
> ---
> fsck.c | 23 ++-
> fsck.h | 8 +---
> 2 files changed, 7 insertions(+), 24 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 83f4562390..9246afee22 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -10,7 +10,6 @@
> #include "fsck.h"
> #include "refs.h"
> #include "utf8.h"
> -#include "sha1-array.h"
> #include "decorate.h"
> #include "oidset.h"
> #include "packfile.h"
> @@ -182,19 +181,10 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,
>
> static void init_skiplist(struct fsck_options *options, const char *path)
> {
> - static struct oid_array skiplist = OID_ARRAY_INIT;
> - int sorted;
> FILE *fp;
> struct strbuf sb = STRBUF_INIT;
> struct object_id oid;
>
> - if (options->skiplist)
> - sorted = options->skiplist->sorted;
> - else {
> - sorted = 1;
> - options->skiplist =
> - }
> -
> fp = fopen(path, "r");
> if (!fp)
> die("Could not open skip list: %s", path);
> @@ -202,17 +192,10 @@ static void init_skiplist(struct fsck_options *options,
> const char *path)
> const char *p;
> if (parse_oid_hex(sb.buf, , ) || *p != '\0')
> die("Invalid SHA-1: %s", sb.buf);
> - oid_array_append(, );
> - if (sorted && skiplist.nr > 1 &&
> - oidcmp([skiplist.nr - 2],
> - ) > 0)
> - sorted = 0;
> + oidset_insert(>skiplist, );
> }
> fclose(fp);
> strbuf_release();
> -
> - if (sorted)
> - skiplist.sorted = 1;
> }
>
> static int parse_msg_type(const char *str)
> @@ -317,9 +300,7 @@ static void append_msg_id(struct strbuf *sb, const char
> *msg_id)
>
> static int object_on_skiplist(struct fsck_options *opts, struct object *obj)
> {
> - if (opts && opts->skiplist && obj)
> - return oid_array_lookup(opts->skiplist, >oid) >= 0;
> - return 0;
> + return opts && obj && oidset_contains(>skiplist, >oid);
> }
>
> __attribute__((format (printf, 4, 5)))
> diff --git a/fsck.h b/fsck.h
> index c3cf5e0034..26120e6186 100644
> --- a/fsck.h
> +++ b/fsck.h
> @@ -1,6 +1,8 @@
> #ifndef GIT_FSCK_H
> #define GIT_FSCK_H
>
> +#include "oidset.h"
> +
> #define FSCK_ERROR 1
> #define FSCK_WARN 2
> #define FSCK_IGNORE 3
> @@ -34,12 +36,12 @@ struct fsck_options {
> fsck_error error_func;
> unsigned strict:1;
> int *msg_type;
> - struct oid_array *skiplist;
> + struct oidset skiplist;
> struct decoration *object_names;
> };
>
> -#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL }
> -#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL }
> +#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL,
> OIDSET_INIT }
> +#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL,
> OIDSET_INIT }
Note that a NULL initialiser, for the object_names field, is missing
(not introduced by this patch). Since you have bumped into the 80th
column, you may not want to add that NULL to the end of these macros
(it is not _necessary_ after all). However, ... :-D
Otherwise, LGTM.
Thanks!
ATB,
Ramsay Jones
>
> /* descend in all linked child objects
> * the return value is:
Re: [PATCH 1/6] add, update-index: fix --chmod argument help
On 02/08/18 20:17, René Scharfe wrote:
> Don't translate the argument specification for --chmod; "+x" and "-x"
> are the literal strings that the commands accept.
>
> Separate alternatives using a pipe character instead of a slash, for
> consistency.
>
> Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding a
> pair of angular brackets around the argument help string, as that would
> wrongly indicate that users need to replace the literal strings with
> some kind of value.
>
> Signed-off-by: Rene Scharfe
> ---
> builtin/add.c | 4 +++-
> builtin/update-index.c | 2 +-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 8a155dd41e..84bfec9b73 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -304,7 +304,9 @@ static struct option builtin_add_options[] = {
> OPT_BOOL( 0 , "refresh", _only, N_("don't add, only refresh the
> index")),
> OPT_BOOL( 0 , "ignore-errors", _add_errors, N_("just skip files
> which cannot be added because of errors")),
> OPT_BOOL( 0 , "ignore-missing", _missing, N_("check if - even
> missing - files are ignored in dry run")),
> - OPT_STRING( 0 , "chmod", _arg, N_("(+/-)x"), N_("override the
> executable bit of the listed files")),
> + { OPTION_STRING, 0, "chmod", _arg, "(+|-)x",
Am I alone in thinking that "(+x|-x)" is more readable?
ATB,
Ramsay Jones
> + N_("override the executable bit of the listed files"),
> + PARSE_OPT_LITERAL_ARGHELP },
> OPT_HIDDEN_BOOL(0, "warn-embedded-repo", _on_embedded_repo,
> N_("warn when adding an embedded repository")),
> OPT_END(),
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index a8709a26ec..7feda6e271 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -971,7 +971,7 @@ int cmd_update_index(int argc, const char **argv, const
> char *prefix)
> PARSE_OPT_NOARG | /* disallow --cacheinfo= form */
> PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> (parse_opt_cb *) cacheinfo_callback},
> - {OPTION_CALLBACK, 0, "chmod", _executable_bit, N_("(+/-)x"),
> + {OPTION_CALLBACK, 0, "chmod", _executable_bit, "(+|-)x",
> N_("override the executable bit of the listed files"),
> PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> chmod_callback},
>
Re: [PATCH 2/3] config: fix case sensitive subsection names on writing
On 01/08/18 20:34, Stefan Beller wrote: > A use reported a submodule issue regarding strange case indentation s/use/user/ ? ATB, Ramsay Jones
[PATCH] builtin.h: remove declaration of cmd_rebase__helper
Commit 94d4e2fb88 ("rebase -i: move rebase--helper modes to
rebase--interactive", 2018-07-24) removed the definition of the
'cmd_rebase__helper' symbol, but forgot to remove the corresponding
declaration in the 'builtin.h' header file.
Signed-off-by: Ramsay Jones
---
Hi Alban,
If you need to re-roll your 'ag/rebase-i-in-c' branch, could you
please squash this into the relevant patch (commit 94d4e2fb88).
Thanks!
ATB,
Ramsay Jones
builtin.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/builtin.h b/builtin.h
index aac8f5f340..6538932e99 100644
--- a/builtin.h
+++ b/builtin.h
@@ -206,7 +206,6 @@ extern int cmd_range_diff(int argc, const char **argv,
const char *prefix);
extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
extern int cmd_rebase(int argc, const char **argv, const char *prefix);
extern int cmd_rebase__interactive(int argc, const char **argv, const char
*prefix);
-extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
extern int cmd_reflog(int argc, const char **argv, const char *prefix);
extern int cmd_remote(int argc, const char **argv, const char *prefix);
--
2.18.0
Re: [PATCH] t5562: avoid non-portable "export FOO=bar" construct
On 29/07/18 04:13, Eric Sunshine wrote:
> On Sat, Jul 28, 2018 at 6:51 PM Ramsay Jones
> wrote:
>> Commit 6c213e863a ("http-backend: respect CONTENT_LENGTH for
>> receive-pack", 2018-07-27) adds a test which uses the non-portable
>> export construct. Replace it with "FOO=bar && export FOO" instead.
>>
>> Signed-off-by: Ramsay Jones
>> ---
>> Could you please put this on top of the 'mk/http-backend-content-length'
>> branch. This test tickles the new "export FOO=bar" check, so the test
>> suite does not run otherwise.
>
> The "export FOO=bar" check comes from 99680d (test-lint: detect
> 'export FOO=bar', 2013-07-08), so is not exactly new.
>
> Perhaps you were thinking of [1] a0a630192d
Heh, yes you are obviously correct. Although 'thinking' might
be being too generous! ;-)
[I can't even claim that it was late ... midnight is actually
quite early for me!]
> (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13),
> when you wrote this, though it is not related to "export FOO=bar"
> detection.
>
> The patch itself looks fine, by the way.
>
> [1]:
> https://public-inbox.org/git/20180713055205.32351-5-sunsh...@sunshineco.com/
>
Thanks!
ATB,
Ramsay Jones
[PATCH] t5562: avoid non-portable "export FOO=bar" construct
Commit 6c213e863a ("http-backend: respect CONTENT_LENGTH for
receive-pack", 2018-07-27) adds a test which uses the non-portable
export construct. Replace it with "FOO=bar && export FOO" instead.
Signed-off-by: Ramsay Jones
---
Hi Junio,
Could you please put this on top of the 'mk/http-backend-content-length'
branch. This test tickles the new "export FOO=bar" check, so the test
suite does not run otherwise.
[If Max needs to re-roll that patch series, then he can squash this in.]
BTW, t3404.#4 fails for me, but I think you are already aware of that
test failure, right?
Thanks!
ATB,
Ramsay Jones
t/t5562-http-backend-content-length.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t5562-http-backend-content-length.sh
b/t/t5562-http-backend-content-length.sh
index 057dcb85d6..43570ce120 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -45,7 +45,8 @@ ssize_b100dots() {
}
test_expect_success 'setup' '
- export HTTP_CONTENT_ENCODING="identity" &&
+ HTTP_CONTENT_ENCODING="identity" &&
+ export HTTP_CONTENT_ENCODING &&
git config http.receivepack true &&
test_commit c0 &&
test_commit c1 &&
--
2.18.0
Re: [PATCH 00/16] Consolidate reachability logic
On 16/07/18 14:00, Derrick Stolee via GitGitGadget wrote: > There are many places in Git that use a commit walk to determine > reachability between commits and/or refs. A lot of this logic is > duplicated. [snip] ... This is not your problem, but I find these GitGitGadget submissions somewhat annoying. This series has been spewed all over my in-box in, what I assume, is commit date order. So, patches #4,5 dated 19/06, then #1,2,3 dated 25/06, then #15 dated 28/06, then #6,7 dated 12/07, then #8-16 dated 13/07, then 00/16 dated today. No I don't use a threaded display (I hate it), be even with that turned on, the patches still appear in the above order under the cover letter (but at least all together). Annoyed. ATB, Ramsay Jones
Re: [PATCH] fsck: check skiplist for object in fsck_blob()
On 13/07/18 20:46, Jeff King wrote: > On Fri, Jul 13, 2018 at 03:41:19PM -0400, Jeff King wrote: > >>> not ok 18 - push rejects corrupt .gitmodules (policy) >>> # >>> # rm -rf dst.git && >>> # git init --bare dst.git && >>> # git -C dst.git config transfer.fsckObjects true && >>> # git -C dst.git config fsck.gitmodulesParse error && >>> # test_must_fail git -C corrupt push ../dst.git HEAD 2>output && >>> # grep gitmodulesParse output && >>> # test_i18ngrep ! "bad config" output >> >> There are separate config slots for local fsck versus receiving objects. >> So I think you need to be setting receive.fsck.gitmodulesParse. > > I confirmed that s/fsck/receive.fsck/ in your patch makes the tests > pass. Doh! Thanks for catching my stupid mistake! I was rushing a bit just before going out (yes, I'm going to be late now!). > I didn't bother adding extra push tests in the patch I just sent, since > upgrading of config error classes is already covered elsewhere in t5504. yeah, I like to 'test' by adding tests if I can (makes repeating the steps less effort ...). So, I was just 'showing my working', as it were. > That said, I'm not opposed to adding more tests on top even if they are > slightly redundant (well, not redundant if you're into black-box > testing, but our current tests are usually written with an assumption of > where the module boundaries are, and what is likely to be a problem). I don't mind either way. I will let you and Junio decide. Thanks! ATB, Ramsay Jones
Re: [PATCH] fsck: check skiplist for object in fsck_blob()
On 11/07/18 20:31, Ramsay Jones wrote:
> On 07/07/18 02:32, Jeff King wrote:
[snip]
>> Hmm, we seem to have "info" these days, so maybe that would do what I
>> want. I.e., I wonder if the patch below does everything we'd want. It's
>> late here and I probably won't get back to this until Monday, but you
>> may want to play with it in the meantime.
>
> Sorry, I've been busy with other things and have not had the
> time to try the patch below (still trying to catch up with
> the mailing-list emails!).
>
>> diff --git a/fsck.c b/fsck.c
>> index 48e7e36869..0b0003055e 100644
>> --- a/fsck.c
>> +++ b/fsck.c
>> @@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
>> FUNC(ZERO_PADDED_DATE, ERROR) \
>> FUNC(GITMODULES_MISSING, ERROR) \
>> FUNC(GITMODULES_BLOB, ERROR) \
>> -FUNC(GITMODULES_PARSE, ERROR) \
>> FUNC(GITMODULES_NAME, ERROR) \
>> FUNC(GITMODULES_SYMLINK, ERROR) \
>> /* warnings */ \
>> @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
>> FUNC(NUL_IN_COMMIT, WARN) \
>> /* infos (reported as warnings, but ignored by default) */ \
>> FUNC(BAD_TAG_NAME, INFO) \
>> +FUNC(GITMODULES_PARSE, INFO) \
>> FUNC(MISSING_TAGGER_ENTRY, INFO)
>>
>> #define MSG_ID(id, msg_type) FSCK_MSG_##id,
>>
>
> So, just squinting at this in my email client, if this allowed
> a push/fetch to succeed (along with an 'info' message), while
> providing an admin the means to configure it to loudly deny
> the push/fetch - then I think we have a winner! ;-)
>
> Sorry for not testing the patch.
OK, so I found some time to test this tonight. It is not good
news (assuming that I haven't messed up the testing, of course). :(
On top of 'pu' (@9026cfc855), I reverted commit d4c5675233
("fsck: silence stderr when parsing .gitmodules", 2018-06-28) and
added the patch given below.
Unfortunately, the final test fails, thus:
$ cd t
$ ./t7415-submodule-names.sh
ok 1 - check names
ok 2 - create innocent subrepo
ok 3 - submodule add refuses invalid names
ok 4 - add evil submodule
ok 5 - add other submodule
ok 6 - clone evil superproject
ok 7 - fsck detects evil superproject
ok 8 - transfer.fsckObjects detects evil superproject (unpack)
ok 9 - transfer.fsckObjects detects evil superproject (index)
ok 10 - create oddly ordered pack
ok 11 - transfer.fsckObjects handles odd pack (unpack)
ok 12 - transfer.fsckObjects handles odd pack (index)
ok 13 - index-pack --strict works for non-repo pack
ok 14 - fsck detects symlinked .gitmodules file
ok 15 - fsck detects non-blob .gitmodules
ok 16 - fsck detects corrupt .gitmodules
ok 17 - push warns about corrupt .gitmodules
not ok 18 - push rejects corrupt .gitmodules (policy)
#
# rm -rf dst.git &&
# git init --bare dst.git &&
# git -C dst.git config transfer.fsckObjects true &&
# git -C dst.git config fsck.gitmodulesParse error &&
# test_must_fail git -C corrupt push ../dst.git HEAD 2>output &&
# grep gitmodulesParse output &&
# test_i18ngrep ! "bad config" output
#
# failed 1 among 18 test(s)
1..18
$
i.e. the test_must_fail doesn't! ;-)
I have to go out now, but I will hopefully take a look at
this again tomorrow. (Do the test additions look OK?)
ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] WIP: try jeff's last patch
Signed-off-by: Ramsay Jones
---
fsck.c | 2 +-
t/t7415-submodule-names.sh | 26 ++
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/fsck.c b/fsck.c
index 17b3a51fa..b74856cee 100644
--- a/fsck.c
+++ b/fsck.c
@@ -64,7 +64,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
FUNC(ZERO_PADDED_DATE, ERROR) \
FUNC(GITMODULES_MISSING, ERROR) \
FUNC(GITMODULES_BLOB, ERROR) \
- FUNC(GITMODULES_PARSE, ERROR) \
FUNC(GITMODULES_NAME, ERROR) \
FUNC(GITMODULES_SYMLINK, ERROR) \
/* warnings */ \
@@ -79,6 +78,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
FUNC(NUL_IN_COMMIT, WARN) \
/* infos (reported as warnings, but ignored by default) */ \
FUNC(BAD_TAG_NAME, INFO) \
+ FUNC(GITMODULES_PARSE, INFO) \
FUNC(MISSING_TAGGER_ENTRY, INFO)
#define MSG_ID(id, msg_type) FSCK_MSG_##id,
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index ba8af785a..ef9535a44 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -185,10 +185,36 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
git add .gitmodules &&
git commit -m "broken gitm
[PATCH] ref-filter: mark some file-local symbols as static
Signed-off-by: Ramsay Jones
---
Hi Olga,
If you need to re-roll your 'ot/ref-filter-object-info' branch,
could you please squash this into the relevant patch (commit c5d9a471d6,
"ref-filter: use oid_object_info() to get object", 2018-07-09).
[Both sparse and my static-check.pl script are complaining about
the 'oi' and 'oi_deref' symbols.]
Thanks!
ATB,
Ramsay Jones
ref-filter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ref-filter.c b/ref-filter.c
index 9aedf29e0..70fb15619 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -63,7 +63,7 @@ struct refname_atom {
int lstrip, rstrip;
};
-struct expand_data {
+static struct expand_data {
struct object_id oid;
enum object_type type;
unsigned long size;
--
2.18.0
[PATCH] t6036: fix broken && chain in sub-shell
Signed-off-by: Ramsay Jones --- Hi Junio, I had a test failure on 'pu' today - Eric's chain-lint series found another broken chain in one of Elijah's new tests (on the 'en/t6036-recursive-corner-cases' branch). ATB, Ramsay Jones t/t6036-recursive-corner-cases.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh index 2a44acace..59e52c5a0 100755 --- a/t/t6036-recursive-corner-cases.sh +++ b/t/t6036-recursive-corner-cases.sh @@ -495,7 +495,7 @@ test_expect_success 'setup differently handled merges of directory/file conflict test_write_lines 1 2 3 4 5 6 7 8 >a && git add a && git commit -m E3 && - git tag E3 + git tag E3 && git checkout C^0 && test_must_fail git merge B^0 && -- 2.18.0
Re: [PATCH] fsck: check skiplist for object in fsck_blob()
On 07/07/18 02:32, Jeff King wrote: [snip] >> I'm not interested in any savings - it would have to be a pretty >> wacky repo for there to be much in the way of savings! >> >> Simply, I have found (for many different reasons) that, if there >> is no good reason to execute some code, it is _far_ better to not >> do so! ;-) > > Heh. I also agree with that as a guiding principle. But I _also_ like > the principle of "if you do not need to do add this code, do not add > it". So the two are a little at odds here. :) I agree with that also! ;-) However, in this case, I can't imagine having to do less, to do nothing - if you see what I mean! So, I think "don't execute code you don't need to" trumps "don't add code you don't need to" here. [snip] > What next? I was kind of leaning towards loosening, but it sounded like > Junio thought the opposite. One thing I didn't like about the patch I > sent earlier is that it removes the option for the admin to say "no, I > really do want to enforce this". I don't know if that's of value or not. Yes, it would be good to let the admin set the policy. > In an ideal world, I think we'd detect the problem and then react > according to the receiver's receive.fsck.* config. And we could just > flip the default for receive.fsck.gitmodulesParse to "warning". But > IIRC, the fsck code in index-pack does not bother distinguishing between > warnings and errors. I think it ought to, but that's too big a change to > go on maint. > > It _might_ work to just flip the default to "ignore". That leaves the > paranoid admin with a way to re-enable it if they want, and I _think_ it > would be respected by index-pack. Ah, that would be good, if it works. > [goes and looks at the code] > > Hmm, we seem to have "info" these days, so maybe that would do what I > want. I.e., I wonder if the patch below does everything we'd want. It's > late here and I probably won't get back to this until Monday, but you > may want to play with it in the meantime. Sorry, I've been busy with other things and have not had the time to try the patch below (still trying to catch up with the mailing-list emails!). > diff --git a/fsck.c b/fsck.c > index 48e7e36869..0b0003055e 100644 > --- a/fsck.c > +++ b/fsck.c > @@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT; > FUNC(ZERO_PADDED_DATE, ERROR) \ > FUNC(GITMODULES_MISSING, ERROR) \ > FUNC(GITMODULES_BLOB, ERROR) \ > - FUNC(GITMODULES_PARSE, ERROR) \ > FUNC(GITMODULES_NAME, ERROR) \ > FUNC(GITMODULES_SYMLINK, ERROR) \ > /* warnings */ \ > @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT; > FUNC(NUL_IN_COMMIT, WARN) \ > /* infos (reported as warnings, but ignored by default) */ \ > FUNC(BAD_TAG_NAME, INFO) \ > + FUNC(GITMODULES_PARSE, INFO) \ > FUNC(MISSING_TAGGER_ENTRY, INFO) > > #define MSG_ID(id, msg_type) FSCK_MSG_##id, > So, just squinting at this in my email client, if this allowed a push/fetch to succeed (along with an 'info' message), while providing an admin the means to configure it to loudly deny the push/fetch - then I think we have a winner! ;-) Sorry for not testing the patch. ATB, Ramsay Jones
Re: [PATCH] fsck: check skiplist for object in fsck_blob()
On 03/07/18 15:34, Jeff King wrote: > On Fri, Jun 29, 2018 at 02:10:59AM +0100, Ramsay Jones wrote: > >> On 28/06/18 23:03, Jeff King wrote: >>> On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote: >> [snip] >>> Yes, it can go in quickly. But I'd prefer not to keep it in the long >>> term if it's literally doing nothing. >> >> Hmm, I don't think you can say its doing nothing! >> >> "Yeah, this solution seems sensible. Given that we would >> never report any error for that blob, there is no point >> in even looking at it." >> >> ... is no less true, with or without additional patches! ;-) > > True that we don't even bother doing the parsing with your patch. But I > think I talked myself out of that part being a significant savings > elsewhere. [Sorry for the late reply - watching football again!] I'm not interested in any savings - it would have to be a pretty wacky repo for there to be much in the way of savings! Simply, I have found (for many different reasons) that, if there is no good reason to execute some code, it is _far_ better to not do so! ;-) > I guess it would be OK to leave it in. It just feels like it would be > vestigial after the rest of the patches. > [snip] >>> Yes, it would include any syntax error. I also have a slight worry about >>> that, but nobody seems to have screamed _yet_. :) >> >> Hmm, I don't think we can ignore this. :( > > I'm not sure. This has been running on every push to GitHub for the past > 6 weeks, and this is the first report. It's hard to say what that means, > and technically speaking of course this _is_ a regression. Hmm, are you concerned about old clients 'transmitting' the bad data via large hosting sites? (New clients would complain about a dodgy .gitmodules file, no matter were it came from, right?) Has the definition of the config file syntax changed recently? If not, then old client versions will see the same syntax errors as recently 'fixed' versions. So they should error out without 'looking' at the bad data, right? (ignoring the 'lets fix the obvious syntax error' idea). > There's a nearby thread of interest, too, which I cc'd you on: > > https://public-inbox.org/git/20180703070650.b3drk5a6kb4k4...@glandium.org/ Yeah, I don't quite follow what's going on there - I would have to read up some more. ;-) >> So, FWIW, Ack! >> >> [I still think my original patch, with the 'to_be_skipped' >> function name changed to 'object_on_skiplist', should be >> the first patch of the series!] > > Thanks. If we're going to do any loosening, I think we may want to > address that _first_, so it can go directly on top of the patches in > v2.17.1 (because it's a bigger issue than the stray message, IMHO). Agreed. I probably haven't given it sufficient thought, but my immediate reaction is to loosen the check - I don't see how this could be exploited to significantly reduce security. (My lack of imagination has been noted several times in the past, however!) Having said that, I am no security expert, so I will let those who have security expertise decide what is best to do in this situation. Thanks! ATB, Ramsay Jones
Re: [PATCH 4/4] fsck: silence stderr when parsing .gitmodules
On 28/06/18 23:12, Jeff King wrote: > On Thu, Jun 28, 2018 at 06:06:03PM -0400, Jeff King wrote: > >> Note that we didn't test this case at all, so I've added >> coverage in t7415. We may end up toning down or removing >> this fsck check in the future. So take this test as checking >> what happens now with a focus on stderr, and not any >> ironclad guarantee that we must detect and report parse >> failures in the future. > > And such a patch _could_ look something like this. Though we could also > perhaps leave it in place and tone it down to "ignore" by default. > > There's another case that triggers gitmodulesParse, too, which is a blob > so gigantic that we try not to hold it in memory. Technically that could > also happen due to somebody using .gitmodules for something unrelated. > But that seems even more far-fetched. And it _is_ dangerous to leave, > because I think existing vulnerable clients will try to load a 500MB > .gitmodules file in memory and parse it. I also applied and tested the patch below. I think this patch must be included in the series. ATB, Ramsay Jones > --- > diff --git a/fsck.c b/fsck.c > index 87b0e228bd..296e8a8a7c 100644 > --- a/fsck.c > +++ b/fsck.c > @@ -1013,11 +1013,9 @@ static int fsck_blob(struct blob *blob, const char > *buf, > data.options = options; > data.ret = 0; > config_opts.error_action = CONFIG_ERROR_SILENT; > - if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB, > - ".gitmodules", buf, size, , _opts)) > - data.ret |= report(options, >object, > -FSCK_MSG_GITMODULES_PARSE, > -"could not parse gitmodules blob"); > + /* ignore errors */ > + git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB, > + ".gitmodules", buf, size, , _opts); > > return data.ret; > } > diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh > index ba8af785a5..9a7dae88a5 100755 > --- a/t/t7415-submodule-names.sh > +++ b/t/t7415-submodule-names.sh > @@ -176,7 +176,7 @@ test_expect_success 'fsck detects non-blob .gitmodules' ' > ) > ' > > -test_expect_success 'fsck detects corrupt .gitmodules' ' > +test_expect_success 'fsck does not complain about corrupt .gitmodules' ' > git init corrupt && > ( > cd corrupt && > @@ -185,9 +185,8 @@ test_expect_success 'fsck detects corrupt .gitmodules' ' > git add .gitmodules && > git commit -m "broken gitmodules" && > > - test_must_fail git fsck 2>output && > - grep gitmodulesParse output && > - test_i18ngrep ! "bad config" output > + git fsck 2>output && > + ! test -s output > ) > ' > >
Re: [PATCH] fsck: check skiplist for object in fsck_blob()
On 28/06/18 23:03, Jeff King wrote: > On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote: [snip] > Yes, it can go in quickly. But I'd prefer not to keep it in the long > term if it's literally doing nothing. Hmm, I don't think you can say its doing nothing! "Yeah, this solution seems sensible. Given that we would never report any error for that blob, there is no point in even looking at it." ... is no less true, with or without additional patches! ;-) > I have some patches which I think solve your problem. They apply on > v2.18.0, but not on v2.17.1 (because they rely on Dscho's increased > passing of config_options in v2.18). Is that good enough? Heh, I was also writing patches to address this tonight (but I was also watching the football, so I was somewhat behind you). My patches were not too dissimilar to yours, except I was aiming to allow even do_config_from_file() etc., to suppress errors. Your patches were cleaner and more focused than mine. (Instead of turning die_on_error into an enum, I added an additional 'quiet' flag. When pushing the stack (eg. for include files), I had to copy the quiet flag from the parent struct, etc, ... ;-) ). > Yes, it would include any syntax error. I also have a slight worry about > that, but nobody seems to have screamed _yet_. :) Hmm, I don't think we can ignore this. :( > Here are the patches I came up with. Yes, I applied these locally and tested them. All OK here. So, FWIW, Ack! [I still think my original patch, with the 'to_be_skipped' function name changed to 'object_on_skiplist', should be the first patch of the series!] ATB, Ramsay Jones
Re: [PATCH] fsck: check skiplist for object in fsck_blob()
On 28/06/18 18:45, Jeff King wrote: > On Thu, Jun 28, 2018 at 05:56:18PM +0100, Ramsay Jones wrote: [snip] >>> One thing we could do is turn the parse failure into a noop. The main >>> point of the check is to protect people against the malicious >>> .gitmodules bug. If the file can't be parsed, then it also can't be an >>> attack vector (assuming the parser used for the fsck check and the >>> parser used by the victim behave identically). >> >> Hmm, yeah, but I would have to provide a means of suppressing >> the config parser error messages. Something I wanted to avoid. :( > > Yes, though I don't think it's too bad. We already have a "die_on_error" > flag in the config code. I think it just needs to become a tristate: > die/error/silent (and probably get passed in via config_options, since I > think we tie it right now to the file/blob source). Yes, but this code is already very crufty - and I'm just waiting for someone to want to have per-repo/submodule config parsing i... ;-) >> Junio, do you want me to address the above 'rejected push' >> issue in this patch, or with a follow-up patch? (It should >> be a pretty rare problem - famous last words!) > > Hmm, if we end up doing the config thing above, then this patch would > become unnecessary. I was thinking of timing - the current patch could go in quickly to solve the immediate problem (eg. in maint). Also, it does not hurt to do this _as well as_ suppress the config errors. > And I think doing that would help _all_ cases, even ones without a > skiplist. They don't need to see random config error messages either, > even if we do eventually report an fsck error. Oh, yes, I agree. You will have noticed that it was my first suggested solution. (I have started writing that patch a few times, but it just makes me want to throw the current code away and start again)! Hmm, BTW, the 'rejected push' problem would include *any* '.gitmodules' blob that contained a syntax error, right? Maybe it won't be as rare as all that! (Imagine not being able to push due to a compiler error/warning in source files. How irritating would that be! :-P ). (if we fix this, you could hide some nefarious settings after an obvious syntax error - then get the victim to fix the syntax error ...) ATB, Ramsay Jones
Re: [PATCH] fsck: check skiplist for object in fsck_blob()
On 28/06/18 12:49, Jeff King wrote:
> On Wed, Jun 27, 2018 at 07:39:53PM +0100, Ramsay Jones wrote:
>
>> Since commit ed8b10f631 ("fsck: check .gitmodules content", 2018-05-02),
>> fsck will issue an error message for '.gitmodules' content that cannot
>> be parsed correctly. This is the case, even when the corresponding blob
>> object has been included on the skiplist. For example, using the cgit
>> repository, we see the following:
>>
>> $ git fsck
>> Checking object directories: 100% (256/256), done.
>> error: bad config line 5 in blob .gitmodules
>> error in blob 51dd1eff1edc663674df9ab85d2786a40f7ae3a5: gitmodulesParse:
>> could not parse gitmodules blob
>> Checking objects: 100% (6626/6626), done.
>> $
>>
>> $ git config fsck.skiplist '.git/skip'
>> $ echo 51dd1eff1edc663674df9ab85d2786a40f7ae3a5 >.git/skip
>> $
>>
>> $ git fsck
>> Checking object directories: 100% (256/256), done.
>> error: bad config line 5 in blob .gitmodules
>> Checking objects: 100% (6626/6626), done.
>> $
>>
>> Note that the error message issued by the config parser is still
>> present, despite adding the object-id of the blob to the skiplist.
>>
>> One solution would be to provide a means of suppressing the messages
>> issued by the config parser. However, given that (logically) we are
>> asking fsck to ignore this object, a simpler approach is to just not
>> call the config parser if the object is to be skipped. Add a check to
>> the 'fsck_blob()' processing function, to determine if the object is
>> on the skiplist and, if so, exit the function early.
>
> Yeah, this solution seems sensible. Given that we would never report any
> error for that blob, there is no point in even looking at it. I wonder
> if we ought to do the same for other types, too. Is there any point in
> opening a tree that is in the skiplist?
Note that the 'blob' object has already been 'loaded' at this
point anyway (and the basic 'object' checks have been done).
I did think about this, briefly, but decided that we should
only 'skip' the leaf nodes (blobs). (So, when processing
commits, trees and tags, we should not report an error for
that object-id, but that should not stop us from checking
the tree object associated with a commit, just because of
a problem with the commit message).
[Oh, wait - Hmm, each object could be checked independently
of all others and not used for any object traversal right?
(e.g. using packfile index). I saw fsck_walk() and didn't
look any further ... Ah, broken link check, ... I obviously
need to read the code some more ... :-D ]
>> I noticed recently that the 'cgit.git' repo was complaining when
>> doing an 'git fsck' ...
>>
>> Note that 'cgit' had a '.gitmodules' file and a 'submodule.sh'
>> script back in 2007, which had nothing to do with the current
>> git submodule facilities, ... ;-)
>
> Yikes. I worried about that sort of regression when adding the
> .gitmodules checks. But this _is_ a pretty unique case: somebody was
> implementing their own version of submodules (pre-git-submodule) and
> decided to use the same name. So I'm not sure if this is a sign that we
> need to think through the regression, or a sign that it really is rare.
I don't have any numbers, but my gut tells me that this would
be very rare indeed. Of course, my gut has been wrong before ... :-D
> One thing we could do is turn the parse failure into a noop. The main
> point of the check is to protect people against the malicious
> .gitmodules bug. If the file can't be parsed, then it also can't be an
> attack vector (assuming the parser used for the fsck check and the
> parser used by the victim behave identically).
Hmm, yeah, but I would have to provide a means of suppressing
the config parser error messages. Something I wanted to avoid. :(
> That wouldn't help with your stray message, of course, but it would
> eliminate the need to deal with the skiplist here (and skiplists aren't
> always easy to do -- for instance, pushing up a non-fork of cgit to
> GitHub would now be rejected because of this old file, and you'd have to
> contact support to resolve it).
Good point.
>> I just remembered that I had intended to review the name of the
>> function that this patch introduces before sending, but totally
>> forgot! :(
>>
>> [Hmm, 'to_be_skipped' -> object_to_be_skipped, object_on_skiplist,
>> etc., ... suggestions welcome!]
>
> The current name is OK to be, but object_on_skiplist() also seems quite
> obvious.
object_on_skiplist() it is!
Junio, do you want me to address the above 'rejected push'
issue in this patch, or with a follow-up patch? (It should
be a pretty rare problem - famous last words!)
ATB,
Ramsay Jones
[PATCH] fsck: check skiplist for object in fsck_blob()
Since commit ed8b10f631 ("fsck: check .gitmodules content", 2018-05-02),
fsck will issue an error message for '.gitmodules' content that cannot
be parsed correctly. This is the case, even when the corresponding blob
object has been included on the skiplist. For example, using the cgit
repository, we see the following:
$ git fsck
Checking object directories: 100% (256/256), done.
error: bad config line 5 in blob .gitmodules
error in blob 51dd1eff1edc663674df9ab85d2786a40f7ae3a5: gitmodulesParse:
could not parse gitmodules blob
Checking objects: 100% (6626/6626), done.
$
$ git config fsck.skiplist '.git/skip'
$ echo 51dd1eff1edc663674df9ab85d2786a40f7ae3a5 >.git/skip
$
$ git fsck
Checking object directories: 100% (256/256), done.
error: bad config line 5 in blob .gitmodules
Checking objects: 100% (6626/6626), done.
$
Note that the error message issued by the config parser is still
present, despite adding the object-id of the blob to the skiplist.
One solution would be to provide a means of suppressing the messages
issued by the config parser. However, given that (logically) we are
asking fsck to ignore this object, a simpler approach is to just not
call the config parser if the object is to be skipped. Add a check to
the 'fsck_blob()' processing function, to determine if the object is
on the skiplist and, if so, exit the function early.
Signed-off-by: Ramsay Jones
---
Hi Junio,
I noticed recently that the 'cgit.git' repo was complaining when
doing an 'git fsck' ...
Note that 'cgit' had a '.gitmodules' file and a 'submodule.sh'
script back in 2007, which had nothing to do with the current
git submodule facilities, ... ;-)
Viz:
$ git show 51dd1eff1e
# This file maps a submodule path to an url from where the submodule
# can be obtained. The script "submodules.sh" finds the url in this file
# when invoked with -i to clone the submodules.
git git://git.kernel.org/pub/scm/git/git.git
$
I just remembered that I had intended to review the name of the
function that this patch introduces before sending, but totally
forgot! :(
[Hmm, 'to_be_skipped' -> object_to_be_skipped, object_on_skiplist,
etc., ... suggestions welcome!]
ATB,
Ramsay Jones
fsck.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/fsck.c b/fsck.c
index 48e7e3686..702ceb629 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,6 +281,13 @@ static void append_msg_id(struct strbuf *sb, const char
*msg_id)
strbuf_addstr(sb, ": ");
}
+static int to_be_skipped(struct fsck_options *opts, struct object *obj)
+{
+ if (opts && opts->skiplist && obj)
+ return oid_array_lookup(opts->skiplist, >oid) >= 0;
+ return 0;
+}
+
__attribute__((format (printf, 4, 5)))
static int report(struct fsck_options *options, struct object *object,
enum fsck_msg_id id, const char *fmt, ...)
@@ -292,8 +299,7 @@ static int report(struct fsck_options *options, struct
object *object,
if (msg_type == FSCK_IGNORE)
return 0;
- if (options->skiplist && object &&
- oid_array_lookup(options->skiplist, >oid) >= 0)
+ if (to_be_skipped(options, object))
return 0;
if (msg_type == FSCK_FATAL)
@@ -963,6 +969,9 @@ static int fsck_blob(struct blob *blob, const char *buf,
return 0;
oidset_insert(_done, >object.oid);
+ if (to_be_skipped(options, >object))
+ return 0;
+
if (!buf) {
/*
* A missing buffer here is a sign that the caller found the
--
2.18.0
[PATCH] diff: fix a sparse 'dubious one-bit signed bitfield' error
Signed-off-by: Ramsay Jones
---
Hi Stefan,
If you need to re-roll your 'sb/diff-color-move-more' branch, could
you please squash this into the relevant patch (commit f2d78d2c67,
"diff.c: add white space mode to move detection that allows indent
changes", 2018-06-21).
Thanks!
ATB,
Ramsay Jones
diff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index c91480141..652521ff2 100644
--- a/diff.c
+++ b/diff.c
@@ -756,7 +756,7 @@ struct moved_entry {
*/
struct ws_delta {
char *string;
- int current_longer : 1;
+ unsigned int current_longer : 1;
};
#define WS_DELTA_INIT { NULL, 0 }
--
2.18.0
[PATCH] ewah: delete unused 'rlwit_discharge_empty()'
From: Junio C Hamano
Complete the removal of unused 'ewah bitmap' code by removing the now
unused 'rlwit_discharge_empty()' function. Also, the 'ewah_clear()'
function can now be made a file-scope static symbol.
Signed-off-by: Ramsay Jones
---
Hi Junio,
Can you please add this to the 'ds/ewah-cleanup' branch, before
we forget to do so! ;-)
Thanks!
ATB,
Ramsay Jones
ewah/ewah_bitmap.c | 20
ewah/ewah_rlw.c| 8
ewah/ewok.h| 6 --
ewah/ewok_rlw.h| 1 -
4 files changed, 12 insertions(+), 23 deletions(-)
diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c
index 017c677f9..d59b1afe3 100644
--- a/ewah/ewah_bitmap.c
+++ b/ewah/ewah_bitmap.c
@@ -276,6 +276,18 @@ void ewah_each_bit(struct ewah_bitmap *self, void
(*callback)(size_t, void*), vo
}
}
+/**
+ * Clear all the bits in the bitmap. Does not free or resize
+ * memory.
+ */
+static void ewah_clear(struct ewah_bitmap *self)
+{
+ self->buffer_size = 1;
+ self->buffer[0] = 0;
+ self->bit_size = 0;
+ self->rlw = self->buffer;
+}
+
struct ewah_bitmap *ewah_new(void)
{
struct ewah_bitmap *self;
@@ -288,14 +300,6 @@ struct ewah_bitmap *ewah_new(void)
return self;
}
-void ewah_clear(struct ewah_bitmap *self)
-{
- self->buffer_size = 1;
- self->buffer[0] = 0;
- self->bit_size = 0;
- self->rlw = self->buffer;
-}
-
void ewah_free(struct ewah_bitmap *self)
{
if (!self)
diff --git a/ewah/ewah_rlw.c b/ewah/ewah_rlw.c
index b9643b7d0..5093d43e2 100644
--- a/ewah/ewah_rlw.c
+++ b/ewah/ewah_rlw.c
@@ -104,11 +104,3 @@ size_t rlwit_discharge(
return index;
}
-
-void rlwit_discharge_empty(struct rlw_iterator *it, struct ewah_bitmap *out)
-{
- while (rlwit_word_size(it) > 0) {
- ewah_add_empty_words(out, 0, rlwit_word_size(it));
- rlwit_discard_first_words(it, rlwit_word_size(it));
- }
-}
diff --git a/ewah/ewok.h b/ewah/ewok.h
index 0c504f28e..84b2a29fa 100644
--- a/ewah/ewok.h
+++ b/ewah/ewok.h
@@ -72,12 +72,6 @@ void ewah_pool_free(struct ewah_bitmap *self);
*/
struct ewah_bitmap *ewah_new(void);
-/**
- * Clear all the bits in the bitmap. Does not free or resize
- * memory.
- */
-void ewah_clear(struct ewah_bitmap *self);
-
/**
* Free all the memory of the bitmap
*/
diff --git a/ewah/ewok_rlw.h b/ewah/ewok_rlw.h
index bb3c6ff7e..7cdfdd0c0 100644
--- a/ewah/ewok_rlw.h
+++ b/ewah/ewok_rlw.h
@@ -98,7 +98,6 @@ void rlwit_init(struct rlw_iterator *it, struct ewah_bitmap
*bitmap);
void rlwit_discard_first_words(struct rlw_iterator *it, size_t x);
size_t rlwit_discharge(
struct rlw_iterator *it, struct ewah_bitmap *out, size_t max, int
negate);
-void rlwit_discharge_empty(struct rlw_iterator *it, struct ewah_bitmap *out);
static inline size_t rlwit_word_size(struct rlw_iterator *it)
{
--
2.17.0
Re: [PATCH 2/8] ewah/bitmap.c: delete unused 'bitmap_each_bit()'
On 15/06/18 15:30, Derrick Stolee wrote:
> Reported-by: Ramsay Jones
> Signed-off-by: Derrick Stolee
> ---
> ewah/bitmap.c | 24
> 1 file changed, 24 deletions(-)
>
> diff --git a/ewah/bitmap.c b/ewah/bitmap.c
> index d61dc6114a..52f1178db4 100644
> --- a/ewah/bitmap.c
> +++ b/ewah/bitmap.c
> @@ -129,30 +129,6 @@ void bitmap_or_ewah(struct bitmap *self, struct
> ewah_bitmap *other)
> self->words[i++] |= word;
> }
>
> -void bitmap_each_bit(struct bitmap *self, ewah_callback callback, void *data)
> -{
> - size_t pos = 0, i;
> -
> - for (i = 0; i < self->word_alloc; ++i) {
> - eword_t word = self->words[i];
> - uint32_t offset;
> -
> - if (word == (eword_t)~0) {
> - for (offset = 0; offset < BITS_IN_EWORD; ++offset)
> - callback(pos++, data);
> - } else {
> - for (offset = 0; offset < BITS_IN_EWORD; ++offset) {
> - if ((word >> offset) == 0)
> - break;
> -
> - offset += ewah_bit_ctz64(word >> offset);
> - callback(pos + offset, data);
> - }
> - pos += BITS_IN_EWORD;
> - }
> - }
> -}
> -
> size_t bitmap_popcount(struct bitmap *self)
> {
> size_t i, count = 0;
>
Again, remove extern declaration from header file.
ATB,
Ramsay Jones
Re: [PATCH 8/8] ewah_io: delete unused 'ewah_serialize_native()'
On 15/06/18 15:30, Derrick Stolee wrote: > Signed-off-by: Derrick Stolee > --- > ewah/ewah_io.c | 26 -- > ewah/ewok.h| 1 - > 2 files changed, 27 deletions(-) This duplicates Jeff's patch #3. ATB, Ramsay Jones
Re: [PATCH 1/8] ewah/bitmap.c: delete unused 'bitmap_clear()'
On 15/06/18 15:30, Derrick Stolee wrote:
> Reported-by: Ramsay Jones
> Signed-off-by: Derrick Stolee
> ---
> ewah/bitmap.c | 8
> 1 file changed, 8 deletions(-)
>
> diff --git a/ewah/bitmap.c b/ewah/bitmap.c
> index 756bdd050e..d61dc6114a 100644
> --- a/ewah/bitmap.c
> +++ b/ewah/bitmap.c
> @@ -45,14 +45,6 @@ void bitmap_set(struct bitmap *self, size_t pos)
> self->words[block] |= EWAH_MASK(pos);
> }
>
> -void bitmap_clear(struct bitmap *self, size_t pos)
> -{
> - size_t block = EWAH_BLOCK(pos);
> -
> - if (block < self->word_alloc)
> - self->words[block] &= ~EWAH_MASK(pos);
> -}
> -
> int bitmap_get(struct bitmap *self, size_t pos)
> {
> size_t block = EWAH_BLOCK(pos);
>
I haven't read all the patches yet, but what about the extern
declaration in ewah/ewok.h?
ATB,
Ramsay Jones
Re: [PATCH 3/3] ewah: drop ewah_serialize_native function
On 15/06/18 14:56, Ramsay Jones wrote: > > > On 15/06/18 04:32, Jeff King wrote: >> We don't call this function, and never have. The on-disk >> bitmap format uses network-byte-order integers, meaning that >> we cannot use the native-byte-order format written here. >> >> Let's drop it in the name of simplicity. > > Hmm, if you are in the mood to drop ewah dead code, how about: > > ewah/bitmap.o - bitmap_clear > ewah/bitmap.o - bitmap_each_bit > ewah/ewah_bitmap.o - ewah_and > ewah/ewah_bitmap.o - ewah_and_not > ewah/ewah_bitmap.o - ewah_not > ewah/ewah_bitmap.o - ewah_or > > ... in addition to these *(de)serialize* functions. ;-) Hmm, I didn't spot it at first, but ewah_serialize() also seems to be unused! ATB, Ramsay Jones
Re: [PATCH 3/3] ewah: drop ewah_serialize_native function
On 15/06/18 04:32, Jeff King wrote: > We don't call this function, and never have. The on-disk > bitmap format uses network-byte-order integers, meaning that > we cannot use the native-byte-order format written here. > > Let's drop it in the name of simplicity. Hmm, if you are in the mood to drop ewah dead code, how about: ewah/bitmap.o - bitmap_clear ewah/bitmap.o - bitmap_each_bit ewah/ewah_bitmap.o - ewah_and ewah/ewah_bitmap.o - ewah_and_not ewah/ewah_bitmap.o - ewah_not ewah/ewah_bitmap.o - ewah_or ... in addition to these *(de)serialize* functions. ;-) ATB, Ramsay Jones
Re: [PATCH v7 2/2] json-writer: t0019: add Python unit test
On 07/06/18 03:23, Jeff King wrote: > On Thu, Jun 07, 2018 at 01:16:14AM +0100, Ramsay Jones wrote: > >>> Probably. We may want to go the same route as we did for perl in >>> a0e0ec9f7d (t: provide a perl() function which uses $PERL_PATH, >>> 2013-10-28) so that test writers don't have to remember this. >>> >>> That said, I wonder if it would be hard to simply do the python bits >>> here in perl. This is the first use of python in our test scripts (and >> >> Hmm, not quite the _first_ use: >> >> $ git grep PYTHON_PATH -- t >> t/lib-git-p4.sh:(cd / && "$PYTHON_PATH" -c 'import time; >> print(int(time.time()))') >> t/lib-git-p4.sh:"$PYTHON_PATH" "$TRASH_DIRECTORY/marshal-dump.py" >> t/t9020-remote-svn.sh:export PATH PYTHON_PATH GIT_BUILD_DIR >> t/t9020-remote-svn.sh:exec "$PYTHON_PATH" >> "$GIT_BUILD_DIR/contrib/svn-fe/svnrdump_sim.py" "$@" >> t/t9802-git-p4-filetype.sh: "$PYTHON_PATH" >> "$TRASH_DIRECTORY/k_smush.py" <"$cli/k-text-k" >cli-k-text-k-smush && >> t/t9802-git-p4-filetype.sh: "$PYTHON_PATH" >> "$TRASH_DIRECTORY/ko_smush.py" <"$cli/k-text-ko" >cli-k-text-ko-smush && >> t/t9802-git-p4-filetype.sh: "$PYTHON_PATH" >> "$TRASH_DIRECTORY/gendouble.py" >%double.png && >> t/t9810-git-p4-rcs.sh: "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_k.py" >> <"$git/$file" >"$scrub" && >> t/t9810-git-p4-rcs.sh: "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_ko.py" >> <"$git/$file" >"$scrub" && >> $ > > OK, the first for a feature that is not already written in python > (leading into my second claim that python is used only for fringe > commands ;) ). > > Though maybe I am wrong that the remote-svn stuff requires python. I > thought it did, but poking around, it looks like it's all C, and just > the "svnrdump_sim" helper is python. Heh, I was a bit tired last night, so although I knew that I required python to be installed to run the test-suite, I could not remember why. As soon as I went to bed, ... :) I recently installed Ubuntu 18.04, so that I could get a heads up on any possible problems later this month when I install Linux Mint 19. In order to get the test-suite to run, I had to set 'PYTHON_PATH = /usr/bin/python3' in my config.mak file. (yes, I could have set NO_PYTHON, but you get the idea). Ubuntu 18.04 no longer installs python2 by default (it has ported to python3), but presumably you can still install it as '/usr/bin/python' (I didn't check). In any event, it was t9020-remote-svn.sh that was failing which, despite the name, does not depend on 'svn'. As you already noted, it does run svnrdump_sim.py and the git-remote-testsvn. > At any rate, I think the point still stands that perl is our main > scripting language. I'd rather keep us to that unless there's a good > reason not to. Agreed. And I see it has come to pass ... :-D ATB, Ramsay Jones
Re: [PATCH v7 2/2] json-writer: t0019: add Python unit test
On 06/06/18 22:03, Jeff King wrote: > On Wed, Jun 06, 2018 at 01:10:52PM -0400, Todd Zullinger wrote: > >> g...@jeffhostetler.com wrote: >>> +# As a sanity check, ask Python to parse our generated JSON. Let Python >>> +# recursively dump the resulting dictionary in sorted order. Confirm that >>> +# that matches our expectations. >>> +test_expect_success PYTHON 'parse JSON using Python' ' >> [...] >>> + python "$TEST_DIRECTORY"/t0019/parse_json_1.py actual && >> >> Would this be better using $PYTHON_PATH rather than >> hard-coding python as the command? > > Probably. We may want to go the same route as we did for perl in > a0e0ec9f7d (t: provide a perl() function which uses $PERL_PATH, > 2013-10-28) so that test writers don't have to remember this. > > That said, I wonder if it would be hard to simply do the python bits > here in perl. This is the first use of python in our test scripts (and Hmm, not quite the _first_ use: $ git grep PYTHON_PATH -- t t/lib-git-p4.sh:(cd / && "$PYTHON_PATH" -c 'import time; print(int(time.time()))') t/lib-git-p4.sh:"$PYTHON_PATH" "$TRASH_DIRECTORY/marshal-dump.py" t/t9020-remote-svn.sh:export PATH PYTHON_PATH GIT_BUILD_DIR t/t9020-remote-svn.sh:exec "$PYTHON_PATH" "$GIT_BUILD_DIR/contrib/svn-fe/svnrdump_sim.py" "$@" t/t9802-git-p4-filetype.sh: "$PYTHON_PATH" "$TRASH_DIRECTORY/k_smush.py" <"$cli/k-text-k" >cli-k-text-k-smush && t/t9802-git-p4-filetype.sh: "$PYTHON_PATH" "$TRASH_DIRECTORY/ko_smush.py" <"$cli/k-text-ko" >cli-k-text-ko-smush && t/t9802-git-p4-filetype.sh: "$PYTHON_PATH" "$TRASH_DIRECTORY/gendouble.py" >%double.png && t/t9810-git-p4-rcs.sh: "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_k.py" <"$git/$file" >"$scrub" && t/t9810-git-p4-rcs.sh: "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_ko.py" <"$git/$file" >"$scrub" && $ I don't run the p4 or svn tests, so ... :-D > really the only user in the whole code base outside of a few fringe > commands). Leaving aside any perl vs python flame-war, I think there's > value in keeping the number of languages limited when there's not a > compelling reason to do otherwise. I agree that fewer languages is (generally) a good idea. ATB, Ramsay Jones
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
On 06/06/18 08:39, Nguyễn Thái Ngọc Duy wrote: > Make the attr API take an index_state instead of assuming the_index in > attr code. All call sites are converted blindly to keep the patch > simple and retain current behavior. Individual call sites may receive > further updates to use the right index instead of the_index. > > There is one ugly temporary workaround added in attr.c that needs some > more explanation. > > Commit c24f3abace (apply: file commited * with CRLF should roundtrip s/commited * with/commited with/ ATB, Ramsay Jones
Re: [PATCH 2/8] upload-pack: implement ref-in-want
On 05/06/18 18:51, Brandon Williams wrote:
> Currently, while performing packfile negotiation, clients are only
> allowed to specify their desired objects using object ids. This causes
> a vulnerability to failure when an object turns non-existent during
> negotiation, which may happen if, for example, the desired repository is
> provided by multiple Git servers in a load-balancing arrangement.
>
> In order to eliminate this vulnerability, implement the ref-in-want
> feature for the 'fetch' command in protocol version 2. This feature
> enables the 'fetch' command to support requests in the form of ref names
> through a new "want-ref " parameter. At the conclusion of
> negotiation, the server will send a list of all of the wanted references
> (as provided by "want-ref" lines) in addition to the generated packfile.
>
> Signed-off-by: Brandon Williams
> ---
> Documentation/config.txt| 4 +
> Documentation/technical/protocol-v2.txt | 28 -
> t/t5703-upload-pack-ref-in-want.sh | 153
> upload-pack.c | 64 ++
> 4 files changed, 248 insertions(+), 1 deletion(-)
> create mode 100755 t/t5703-upload-pack-ref-in-want.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ab641bf5a..acafe6c8d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -3479,6 +3479,10 @@ Note that this configuration variable is ignored if it
> is seen in the
> repository-level config (this is a safety measure against fetching from
> untrusted repositories).
>
> +uploadpack.allowRefInWant::
> + If this option is set, `upload-pack` will support the `ref-in-want`
> + feature of the protocol version 2 `fetch` command.
> +
> url..insteadOf::
> Any URL that starts with this value will be rewritten to
> start, instead, with . In cases where some site serves a
> diff --git a/Documentation/technical/protocol-v2.txt
> b/Documentation/technical/protocol-v2.txt
> index 49bda76d2..8367e09b8 100644
> --- a/Documentation/technical/protocol-v2.txt
> +++ b/Documentation/technical/protocol-v2.txt
> @@ -299,12 +299,21 @@ included in the client's request:
> for use with partial clone and partial fetch operations. See
> `rev-list` for possible "filter-spec" values.
>
> +If the 'ref-in-want' feature is advertised, the following argument can
> +be included in the client's request as well as the potential addition of
> +the 'wanted-refs' section in the server's response as explained below.
> +
> +want-ref
> + Indicates to the server than the client wants to retrieve a
> + particular ref, where is the full name of a ref on the
> + server.
> +
> The response of `fetch` is broken into a number of sections separated by
> delimiter packets (0001), with each section beginning with its section
> header.
>
> output = *section
> -section = (acknowledgments | shallow-info | packfile)
> +section = (acknowledgments | shallow-info | wanted-refs | packfile)
> (flush-pkt | delim-pkt)
>
> acknowledgments = PKT-LINE("acknowledgments" LF)
> @@ -319,6 +328,10 @@ header.
> shallow = "shallow" SP obj-id
> unshallow = "unshallow" SP obj-id
>
> +wanted-refs = PKT-LINE("wanted-refs" LF)
> + *PKT-Line(wanted-ref LF)
s/PKT-Line/PKT-LINE/
ATB,
Ramsay Jones
