Re: [PATCH v4 00/14] output improvements for git range-diff
On 11/07/2019 17:08, Thomas Gummerer wrote:
> Thanks Junio for the comment on the previous round [1]. This round
> reanmes the struct we're using in apply.c to 'struct gitdiff_data',
> and updates the commit message of 7/14 to reflect the new name of the
> renamed function.
>
> [1]: https://public-inbox.org/git/20190708163315.29912-1-t.gumme...@gmail.com/
>
> Thomas Gummerer (14):
> apply: replace marc.info link with public-inbox
> apply: only pass required data to skip_tree_prefix
> apply: only pass required data to git_header_name
> apply: only pass required data to check_header_line
> apply: only pass required data to find_name_*
> apply: only pass required data to gitdiff_* functions
> apply: make parse_git_diff_header public
> range-diff: fix function parameter indentation
> range-diff: split lines manually
> range-diff: don't remove funcname from inner diff
> range-diff: suppress line count in outer diff
> range-diff: add section header instead of diff header
> range-diff: add filename to inner diff
> range-diff: add headers to the outer hunk header
>
> apply.c| 186 ++---
> apply.h| 48 +++
> diff.c | 5 +-
> diff.h | 1 +
> range-diff.c | 124 +++
> t/t3206-range-diff.sh | 124 ++-
> t/t3206/history.export | 84 ++-
> 7 files changed, 409 insertions(+), 163 deletions(-)
Yes, the patch I just sent related to the previous version of
this series. However, I believe it still applies to this version
(looking at the range-diff below).
ATB,
Ramsay Jones
> Range-diff against v3:
> 1: ef2245edda = 1: ef2245edda apply: replace marc.info link with
> public-inbox
> 2: 94578fa45c = 2: 94578fa45c apply: only pass required data to
> skip_tree_prefix
> 3: 988269a68e = 3: 988269a68e apply: only pass required data to
> git_header_name
> 4: a2c1ef3f5f = 4: a2c1ef3f5f apply: only pass required data to
> check_header_line
> 5: 0f4cfe21cb = 5: 0f4cfe21cb apply: only pass required data to
> find_name_*
> 6: 07a271518d ! 6: 42665e5295 apply: only pass required data to gitdiff_*
> functions
> @@ -28,7 +28,7 @@
> #include "rerere.h"
> #include "apply.h"
>
> -+struct parse_git_header_state {
> ++struct gitdiff_data {
> +struct strbuf *root;
> +int linenr;
> +int p_value;
> @@ -42,7 +42,7 @@
> }
>
> -static int gitdiff_hdrend(struct apply_state *state,
> -+static int gitdiff_hdrend(struct parse_git_header_state *state,
> ++static int gitdiff_hdrend(struct gitdiff_data *state,
> const char *line,
> struct patch *patch)
> {
> @@ -51,7 +51,7 @@
> #define DIFF_NEW_NAME 1
>
> -static int gitdiff_verify_name(struct apply_state *state,
> -+static int gitdiff_verify_name(struct parse_git_header_state *state,
> ++static int gitdiff_verify_name(struct gitdiff_data *state,
> const char *line,
> int isnull,
> char **name,
> @@ -77,7 +77,7 @@
> }
>
> -static int gitdiff_oldname(struct apply_state *state,
> -+static int gitdiff_oldname(struct parse_git_header_state *state,
> ++static int gitdiff_oldname(struct gitdiff_data *state,
> const char *line,
> struct patch *patch)
> {
> @@ -86,7 +86,7 @@
> }
>
> -static int gitdiff_newname(struct apply_state *state,
> -+static int gitdiff_newname(struct parse_git_header_state *state,
> ++static int gitdiff_newname(struct gitdiff_data *state,
> const char *line,
> struct patch *patch)
> {
> @@ -95,7 +95,7 @@
> }
>
> -static int gitdiff_oldmode(struct apply_state *state,
> -+static int gitdiff_oldmode(struct parse_git_header_state *state,
> ++static int gitdiff_oldmode(struct gitdiff_data *state,
> const char *line,
> struct patch *patch)
> {
> @@ -103,7 +103,7 @@
> }
>
> -static int gitdiff_newmode(struct apply_state *state,
> -+static int gitdiff_newmode(struct parse_git_header_state *state,
> ++static int gitdiff_newmode(struct gitdiff_data *state,
>
[PATCH] range-diff: fix some 'hdr-check' and sparse warnings
Signed-off-by: Ramsay Jones
---
Hi Thomas,
If you need to re-roll your 'tg/range-diff-output-update' branch, could
you please squash (parts) of this into the relevant patches.
The first hunk fixes a couple of 'hdr-check' warnings:
$ diff nhcout phcout | head
4a5,13
> apply.h:146:22: error: ‘GIT_MAX_HEXSZ’ undeclared here (not in a function);
did you mean ‘NI_MAXHOST’?
> char old_oid_prefix[GIT_MAX_HEXSZ + 1];
> ^
> NI_MAXHOST
> apply.h:151:19: error: array type has incomplete element type ‘struct
object_id’
> struct object_id threeway_stage[3];
>^~
> Makefile:2775: recipe for target 'apply.hco' failed
> make: *** [apply.hco] Error 1
$
and needs to be applied to commit b9f62a7e24 ("apply: make parse_git_header
public", 2019-07-08).
The second hunk fixes a sparse 'Using plain integer as NULL pointer'
warning, and needs to be applied to commit 04539fc67b ("range-diff: add
section header instead of diff header", 2019-07-08).
Thanks!
ATB,
Ramsay Jones
apply.h | 1 +
range-diff.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/apply.h b/apply.h
index ade50f66c5..c8c9287cb2 100644
--- a/apply.h
+++ b/apply.h
@@ -3,6 +3,7 @@
#include "lockfile.h"
#include "string-list.h"
+#include "hash.h"
struct repository;
diff --git a/range-diff.c b/range-diff.c
index ba1e9a4265..0f24a4ad12 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -102,7 +102,7 @@ static int read_patches(const char *range, struct
string_list *list)
}
if (starts_with(line, "diff --git")) {
- struct patch patch = { 0 };
+ struct patch patch = { NULL };
struct strbuf root = STRBUF_INIT;
int linenr = 0;
--
2.22.0
[PATCH] env--helper: mark a file-local symbol as static
Signed-off-by: Ramsay Jones
---
Hi Junio,
It seems Ævar didn't have a need to re-roll his patches [1], before
the 'ab/test-env' branch was merged to next. This version of the
patch is based on current 'next'.
Thanks!
ATB,
Ramsay Jones
[1]
https://public-inbox.org/git/f576dfa8-4beb-6430-67b3-6c05f520b...@ramsayjones.plus.com/
builtin/env--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/env--helper.c b/builtin/env--helper.c
index 1083c0f707..23c214fff6 100644
--- a/builtin/env--helper.c
+++ b/builtin/env--helper.c
@@ -7,7 +7,7 @@ static char const * const env__helper_usage[] = {
NULL
};
-enum {
+static enum {
ENV_HELPER_TYPE_BOOL = 1,
ENV_HELPER_TYPE_ULONG
} cmdmode = 0;
--
2.22.0
[PATCH] promisor-remote.h: fix an 'hdr-check' warning
Signed-off-by: Ramsay Jones --- Hi Christian, If you need to re-roll your 'cc/multi-promisor' branch, could you please squash this into the relevant patch (commit 9e27beaa23, "promisor-remote: implement promisor_remote_get_direct()", 2019-06-25). [No, this is not the same as the April patch! :-D ] Thanks! ATB, Ramsay Jones promisor-remote.h | 1 + 1 file changed, 1 insertion(+) diff --git a/promisor-remote.h b/promisor-remote.h index 8200dfc940..a9a9c77b7c 100644 --- a/promisor-remote.h +++ b/promisor-remote.h @@ -2,6 +2,7 @@ #define PROMISOR_REMOTE_H struct object_id; +struct repository; /* * Promisor remote linked list -- 2.22.0
[PATCH] env--helper: mark a file-local symbol as static
Signed-off-by: Ramsay Jones
---
Hi Ævar,
If you need to re-roll your 'ab/test-env' branch, could you please
squash this into the relevant patch (commit b4f207f339, "env--helper:
new undocumented builtin wrapping git_env_*()", 2019-06-21).
Thanks!
ATB,
Ramsay Jones
builtin/env--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/env--helper.c b/builtin/env--helper.c
index 1083c0f707..23c214fff6 100644
--- a/builtin/env--helper.c
+++ b/builtin/env--helper.c
@@ -7,7 +7,7 @@ static char const * const env__helper_usage[] = {
NULL
};
-enum {
+static enum {
ENV_HELPER_TYPE_BOOL = 1,
ENV_HELPER_TYPE_ULONG
} cmdmode = 0;
--
2.22.0
Re: [PATCH v2 00/10] Add 'ls-files --debug-json' to dump the index in json
On 25/06/2019 15:10, Johannes Schindelin wrote: > Hi Duy, [snip] >> Again our experiences differ. Mine is mostly about extensions, >> probably because I had to work on them more often. For normal entries >> "ls-files --debug" gives you 99% what's in the index file already. > > Like the device. And the ctime. And the file size. And the uid/gid. Is > that what you mean? Hmm, well I think so: $ git ls-files --debug git.c git-compat-util.h git-compat-util.h ctime: 1561457278:502638001 mtime: 1561457278:502638001 dev: 2049 ino: 262663 uid: 1000 gid: 1000 size: 35440 flags: 0 git.c ctime: 1561457278:518646000 mtime: 1561457278:518646000 dev: 2049 ino: 263083 uid: 1000 gid: 1000 size: 26837 flags: 0 $ I have occasionally added stuff to the '--debug' output while debugging something, but the above is usually sufficient for my uses. (Having said that, I have not had the need to debug extensions [yet!]). ATB, Ramsay Jones
[PATCH] json-writer: fix a 'hdr-check' warning
Signed-off-by: Ramsay Jones
---
Hi Duy,
If you need to re-roll your 'nd/index-dump-in-json' branch, could
you please squash this into the relevant patch (commit 53f1666b3a,
'split-index.c: dump "link" extension as json', 2019-06-19).
Thanks!
ATB,
Ramsay Jones
json-writer.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/json-writer.h b/json-writer.h
index f778e019a2..29d3dd91e0 100644
--- a/json-writer.h
+++ b/json-writer.h
@@ -45,6 +45,7 @@
#include "strbuf.h"
struct stat_data;
+struct ewah_bitmap;
struct json_writer
{
--
2.22.0
Re: [PATCH 09/17] object: convert create_object() to use object_id
On 20/06/2019 08:41, Jeff King wrote: > There are no callers left of lookup_object() that aren't just passing us s/lookup_object/create_object/ ATB, Ramsay Jones
Re: [RFC/PATCH v1 0/4] compat/obstack: update from upstream
On 14/06/2019 21:30, Ramsay Jones wrote:
>
>
> On 14/06/2019 11:00, SZEDER Gábor wrote:
>> Update 'compat/obstack.{c,h}' from upstream, because they already use
>> 'size_t' instead of 'long' in places that might eventually end up as
>> an argument to malloc(), which might solve build errors with GCC 8 on
>> Windows.
>>
>> The first patch just imports from upstream and doesn't modify anything
>> at all, and, consequently, it can't be compiled because of a screenful
>> or two of errors. This is bad for future bisects, of course.
>>
>> OTOH, adding all the necessary build fixes right away makes review
>> harder...
>>
>> I'm not sure how to deal with this situation, so here is a series with
>> the fixes in separate patches for review, for now. If there's an
>> agreement that this is the direction to take, then I'll squash in the
>> fixes in the first patch and touch up the resulting commit message.
>>
>>
>> Ramsay, could you please run sparse on top of these patch series to
>> make sure that I caught and converted all "0 instead of NULL" usages
>> in the last patch? Thanks.
>
> I applied your patches to current master (@0aae918dd9) and, since
> you dropped the final hunk of commit 3254310863 ("obstack.c: Fix
> some sparse warnings", 2011-09-11), sparse complains, thus:
>
> $ diff sp-out sp-out1
> 27a28,30
> > compat/obstack.c:331:5: warning: incorrect type in initializer (different
> modifiers)
> > compat/obstack.c:331:5:expected void ( *[addressable] [toplevel]
> obstack_alloc_failed_handler )( ... )
> > compat/obstack.c:331:5:got void ( [noreturn] * )( ... )
> $
>
> So, yes you did catch all "using plain integer as NULL pointer"
> warnings! :-D
Sorry for being a bit slow here, but I just realized that
I should not have seen that on Linux (and should have tested
on cygwin), because the obstack code gets elided on Linux ...
Oh, wait:
$ diff sc sc1
3a4,7
> compat/obstack.o- _obstack_allocated_p
> compat/obstack.o- obstack_alloc_failed_handler
> compat/obstack.o- _obstack_begin_1
> compat/obstack.o- _obstack_memory_used
$
Hmm, so on master, this code is totally elided on Linux, but
that is no longer the case with your patches applied. I guess
you need to look at the definition of the {_OBSTACK_}ELIDE_CODE
preprocessor variable(s).
HTH.
ATB,
Ramsay Jones
Re: [RFC/PATCH v1 0/4] compat/obstack: update from upstream
On 14/06/2019 11:00, SZEDER Gábor wrote:
> Update 'compat/obstack.{c,h}' from upstream, because they already use
> 'size_t' instead of 'long' in places that might eventually end up as
> an argument to malloc(), which might solve build errors with GCC 8 on
> Windows.
>
> The first patch just imports from upstream and doesn't modify anything
> at all, and, consequently, it can't be compiled because of a screenful
> or two of errors. This is bad for future bisects, of course.
>
> OTOH, adding all the necessary build fixes right away makes review
> harder...
>
> I'm not sure how to deal with this situation, so here is a series with
> the fixes in separate patches for review, for now. If there's an
> agreement that this is the direction to take, then I'll squash in the
> fixes in the first patch and touch up the resulting commit message.
>
>
> Ramsay, could you please run sparse on top of these patch series to
> make sure that I caught and converted all "0 instead of NULL" usages
> in the last patch? Thanks.
I applied your patches to current master (@0aae918dd9) and, since
you dropped the final hunk of commit 3254310863 ("obstack.c: Fix
some sparse warnings", 2011-09-11), sparse complains, thus:
$ diff sp-out sp-out1
27a28,30
> compat/obstack.c:331:5: warning: incorrect type in initializer (different
modifiers)
> compat/obstack.c:331:5:expected void ( *[addressable] [toplevel]
obstack_alloc_failed_handler )( ... )
> compat/obstack.c:331:5: got void ( [noreturn] * )( ... )
$
So, yes you did catch all "using plain integer as NULL pointer"
warnings! :-D
Thanks.
ATB,
Ramsay Jones
Re: [PATCH v4 12/14] commit-graph: create options for split files
; };
>
> static const char * const builtin_commit_graph_write_usage[] = {
> - N_("git commit-graph write [--object-dir ] [--append|--split]
> [--reachable|--stdin-packs|--stdin-commits]"),
> + N_("git commit-graph write [--object-dir ] [--append|--split]
> [--reachable|--stdin-packs|--stdin-commits] "),
> NULL
> };
>
> @@ -135,6 +135,7 @@ static int graph_read(int argc, const char **argv)
> }
>
> extern int read_replace_refs;
> +struct split_commit_graph_opts split_opts;
This 'split_opts' variable needs to be marked 'static'.
Thanks.
ATB,
Ramsay Jones
Re: [PATCH 00/52] fix some -Wmissing-field-initializer warnings
On 30/05/2019 13:04, Jeff King wrote:
> On Thu, May 30, 2019 at 10:47:37AM +0200, Johannes Sixt wrote:
>
>> I had a brief look at the series. IMO, it is a mistake to appease
>> -Wmissing-field-initializer.
>>
>> We have two sorts of initializers:
>>
>> - zero initializers: they just want to null out every field,
>>like CHILD_PROCESS_INIT and ad-hoc initializers of structs
>>such as xpparam_t pp = { 0 }; in range-diff.c
>>
>> - value initializers are always macros, such as STRING_LIST_INIT_DUP
>>and the OPT_* family.
>>
>> I am strongly against forcing zero initializers to write down a value
>> for every field. It is much more preferable to depend on that the
>> compiler does the right thing with them. -Wmissing-field-initializer
>> would provide guidance in the wrong direction. A zero initializer looks
>> like this: = { 0 }; and nothing else.
>
> I had a similar impression while perusing the commits. I don't mind
> forcing some extra work on programmers to appease a warning if
> disregarding it is a common source of errors. But I didn't see any real
> bug-fixes in the series, so it doesn't seem like that good a tradeoff to
> me.
>
> Contrast that with the -Wunused-parameters warning. I found a dozen or
> so actual bugs by sifting through the results, and another couple dozen
> spots where the code could be cleaned up or simplified. If we want to
> shut up the warning completely (so we can pay attention to it), we'll
> then have to annotate probably a couple hundred spots, and keep those
> annotations up to date. But I feel better doing that knowing that it's
> shown real-world value.
OK, I will drop this branch then.
Thanks all.
ATB,
Ramsay Jones
Re: [PATCH 00/52] fix some -Wmissing-field-initializer warnings
On 24/05/2019 23:30, Ævar Arnfjörð Bjarmason wrote: > > On Fri, May 24 2019, Ramsay Jones wrote: > >> [No, I won't be sending 52 patches to the list!] >> [...] >> This series does not fix any problems or add any new features, so it >> is not important (hence the tendency to 'slip'). I don't want to >> flood the mailing list with patches that nobody wants, so: is there >> any interest in these kinds of patches? If not, I will stop now! >> (I have a 2-3 year old branch that addressed the '-Wsign-compare' >> warnings, but that is probably beyond salvaging by now :( ). >> >> This series is available from: git://repo.or.cz/git/raj.git with the >> branch name 'warn-master'. A trial merge to current 'next' and 'pu' >> branches can be found at 'warn-next' and 'warn-pu' branches. (The >> merge to 'next' went without problem, and 'pu' only required a fixup >> to the builtin/commit patch). >> [...] >> What do you think? > > I think just send it to the list. We've seen worse, and it's easier to > review than needing to get out of the normal E-Mail workflow. I forgot to mention that this series has a long tail. The first ten patches (in addition to all pedantic warnings) removes 1246 warnings. The next 10 removes another 70 and the final 32 patches only removes 55 warnings (the last 18 patches remove only one warning each). Hmm, so maybe I should only send the first few out? I will give it some thought (while waiting for some more comments). Thanks. ATB, Ramsay Jones
[PATCH 00/52] fix some -Wmissing-field-initializer warnings
[No, I won't be sending 52 patches to the list!] This series, started last year, has been hanging around because the time never seemed right to send it to the list. It may still not be the right time ... :-D I would like to be able to compile git using '-Wall -Wextra -Werror' compiler options. In order to see how far away we are from that possibility, you can build with DEVELOPER=1 along with the additional settings: 'DEVOPTS=extra-all no-error pedantic'. That leads to many warnings: $ git describe v2.22.0-rc1 $ make >out 2>&1 $ grep warning out | sed -e 's/.*\[-W/\[-W/' | sort | uniq -c 9 [-Wempty-body] 1694 [-Wmissing-field-initializers] 159 [-Wpedantic] 945 [-Wsign-compare] 2821 [-Wunused-parameter] $ Note that at the beginning of this cycle, the numbers were quite a bit smaller (and this series was only 37 patches, rather than 52): $ git describe v2.21.0 $ make >out 2>&1 $ grep warning out | sed -e 's/.*\[-W/\[-W/' | sort | uniq -c 9 [-Wempty-body] 759 [-Wmissing-field-initializers] 925 [-Wsign-compare] 2732 [-Wunused-parameter] $ This series removes the, newly introduced, pedantic warnings and eliminates all 1371 'missing-field-initializers' warnings that relate to 'struct option': $ cat out-warn-master.stats 9 [-Wempty-body] 323 [-Wmissing-field-initializers] 945 [-Wsign-compare] 2821 [-Wunused-parameter] $ Thus, after about six months, I am further away from my target than when I started! ;-) This series does not fix any problems or add any new features, so it is not important (hence the tendency to 'slip'). I don't want to flood the mailing list with patches that nobody wants, so: is there any interest in these kinds of patches? If not, I will stop now! (I have a 2-3 year old branch that addressed the '-Wsign-compare' warnings, but that is probably beyond salvaging by now :( ). This series is available from: git://repo.or.cz/git/raj.git with the branch name 'warn-master'. A trial merge to current 'next' and 'pu' branches can be found at 'warn-next' and 'warn-pu' branches. (The merge to 'next' went without problem, and 'pu' only required a fixup to the builtin/commit patch). [The 'warn-v2.21' branch shows the previous version of the series.] What do you think? Thanks! ATB, Ramsay Jones Ramsay Jones (52): parse-options: reformat the OPT_X() macros parse-options: move some one-line OPT_X() macros parse-options: rename callback parameter from 'f' to 'cb' parse-options: add missing field initializers list-objects-filter-options: add missing initializer ref-filter.h: add missing field initializers parse-options: add an OPT_LL_CALLBACK() macro parse-options.h: add 'd' parameter to OPT_INTEGER_F() parse-options.h: fix some -Wpedantic warnings builtin/update-index: fix some -Wmissing-field-initializers warnings builtin/log: fix some -Wmissing-field-initializers warnings builtin/notes: fix some -Wmissing-field-initializers warnings builtin/grep: fix some -Wmissing-field-initializers warnings builtin/commit: fix some -Wmissing-field-initializers warnings apply.c: fix some -Wmissing-field-initializers warnings builtin/rebase: fix some -Wmissing-field-initializers warnings builtin/config: add missing field initializers test-parse-options: fix some -Wmissing-field-initializers warnings builtin/read-tree: fix some -Wmissing-field-initializers warnings builtin/merge: fix some -Wmissing-field-initializers warnings builtin/fetch: fix some -Wmissing-field-initializers warnings builtin/commit-tree: fix some -Wmissing-field-initializers warnings builtin/tag: fix an -Wmissing-field-initializers warning builtin/push: fix some -Wmissing-field-initializers warnings builtin/pack-objects: fix some -Wmissing-field-initializers warnings builtin/ls-files: fix some -Wmissing-field-initializers warnings builtin/blame: fix some -Wmissing-field-initializers warnings builtin/show-ref: fix some -Wmissing-field-initializers warnings builtin/show-branch: fix an -Wmissing-field-initializers warning builtin/send-pack: fix some -Wmissing-field-initializers warnings builtin/pull: fix some -Wmissing-field-initializers warnings builtin/fmt-merge-msg: fix some -Wmissing-field-initializers warnings builtin/describe: fix some -Wmissing-field-initializers warnings builtin/cat-file: fix some -Wmissing-field-initializers warnings builtin/shortlog: fix an -Wmissing-field-initializers warning builtin/reset: fix an -Wmissing-field-initializers warning builtin/remote: fix an -Wmissing-field-initializers warning builtin/ls-remote: fix an
Re: Incorrect diff-parseopt conversion?
On 22/05/2019 01:11, Duy Nguyen wrote:
> On Wed, May 22, 2019 at 2:56 AM Ramsay Jones
> wrote:
>>
>> Hi Duy,
>>
>> I am in the middle of rebasing a long running branch onto
>> current master (v2.22.0-rc1) and noticed something odd with
>> commit af2f368091 ("diff-parseopt: convert --output-*",
>> 2019-02-21).
>>
>> As part of the branch I am rebasing, I have defined a new
>> OPT_LL_CALLBACK() macro[1], which I had intended to apply to
>> the 'output' option to diff. However, commit af2f368091
>> defines that option thus:
>>
>> + { OPTION_CALLBACK, 0, "output", options, N_(""),
>> + N_("Output to a specific file"),
>> + PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
>>
>> Note that the 'option type' is given as OPTION_CALLBACK, not
>> as OPTION_LOWLEVEL_CALLBACK. Is this intended?
>
> Yeah I think this is correct (phew!).
OK, I just had a look at the code in parse-options.c.
Hmm, somewhat ugly! :-D
Thanks.
ATB,
Ramsay Jones
Incorrect diff-parseopt conversion?
Hi Duy,
I am in the middle of rebasing a long running branch onto
current master (v2.22.0-rc1) and noticed something odd with
commit af2f368091 ("diff-parseopt: convert --output-*",
2019-02-21).
As part of the branch I am rebasing, I have defined a new
OPT_LL_CALLBACK() macro[1], which I had intended to apply to
the 'output' option to diff. However, commit af2f368091
defines that option thus:
+ { OPTION_CALLBACK, 0, "output", options, N_(""),
+ N_("Output to a specific file"),
+ PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
Note that the 'option type' is given as OPTION_CALLBACK, not
as OPTION_LOWLEVEL_CALLBACK. Is this intended?
ATB,
Ramsay Jones
[1] Yes, the reason my branch is long running is because
we keep changing the same files! We have both defined new
OPT_() macros, some with the same name ... ;-)
Re: jt/fetch-cdn-offload (was What's cooking in git.git (Apr 2019, #04; Mon, 22))
On 22/04/2019 18:51, Jonathan Tan wrote:
>> * jt/fetch-cdn-offload (2019-03-12) 9 commits
>> - SQUASH???
>> - upload-pack: send part of packfile response as uri
>> - fetch-pack: support more than one pack lockfile
>> - upload-pack: refactor reading of pack-objects out
>> - Documentation: add Packfile URIs design doc
>> - Documentation: order protocol v2 sections
>> - http-fetch: support fetching packfiles by URL
>> - http: improve documentation of http_pack_request
>> - http: use --stdin when getting dumb HTTP pack
>>
>> WIP for allowing a response to "git fetch" to instruct the bulk of
>> the pack contents to be instead taken from elsewhere (aka CDN).
>>
>> Waiting for the final version.
>
> Sorry for getting back to you late on this. The current status is that
> v2 (this version) looks good to me, except that not many people seems to
> be interested in this - I sent out v2 [1] with a relatively significant
> protocol change to v1 (requiring the server to also send the packfile's
> hash, meaning that a workflow that Ævar has described will no longer
> work), but nobody replied to it except for Josh Steadmon (who did give
> his Reviewed-By).
>
> In the meantime, I have been working on a server-side JGit
> implementation [2], but not all parts are done (and it will take some
> time).
>
> If this version is good with everyone, then this is the final version. I
> know it has been some time, but if I squash "SQUASH???" onto
> "upload-pack: refactor reading of pack-objects out" and then rebase onto
> latest master (14c0f8d3ab ("The sixth batch", 2019-04-22)), there's only
> one small merge conflict.
... not forgetting the second hunk of [1], of course. ;-)
[1]
https://public-inbox.org/git/5f0c12d5-6714-1516-3579-33d839ad7...@ramsayjones.plus.com/
ATB,
Ramsay Jones
Re: [PATCH/RFC] Makefile: dedup list of files obtained from ls-files
d4b252b0aca22dba9946f7eedf86 2 a 100644 f8829dfb9bf82721903d239ef069fb5de395f3e7 3 a 100644 4f2e6529203aa6d44b5af6e3292c837ceda003f9 0 b 100644 a296d0bb611188cabb256919f36bc30117cca005 0 c 100644 a296d0bb611188cabb256919f36bc30117cca005 0 c $ Er, ... well, I obviously don't have a clue how it is supposed to work. This just looks broken to me. :( > So the patch itself looks good to me (though I agree that Eric's > suggestion to de-dup inside "make" is better still). Agreed. ATB, Ramsay Jones
Re: [PATCH v2 01/13] packfile.h: drop extern from function declarations
On 05/04/2019 19:03, Jeff King wrote: > As CodingGuidelines recommends, we do not need an "extern" when > declaring a public function. Let's drop these. Note that we leave the > extern on report_garbage(), as that is actually a function pointer, not > a function itself. Hmm, perhaps we need to edit CodingGuidelines to make it clear that the 'extern' keyword is not needed on _function_ declarations only. ;-) Because, ... [snip] > /* global flag to enable extra checks when accessing packed objects */ > -extern int do_check_packed_object_crc; > +int do_check_packed_object_crc; ... removing this 'extern' on an int variable sends 'sparse' into a frenzy of warnings! :-D [You didn't use a global s/extern// by any chance?] ATB, Ramsay Jones
Re: [PATCH 01/12] t5319: fix bogus cat-file argument
On 05/04/2019 00:22, Jeff King wrote: > There's no such argument as "--unordered"; it's spelled "--unsorted". Err, isn't this back-to-front? (i.e. cat-file has the _option_ "--unordered" but not "--unsorted"). I suspect that I am not reading that right! :-D ATB, Ramsay Jones
[PATCH] promisor-remote.h: fix an 'hdr-check' warning
Signed-off-by: Ramsay Jones
---
Hi Christian,
If you need to re-roll your 'cc/multi-promisor' branch, could you
please squash this into the relevant patch (commit e52d417b57
("promisor-remote: implement promisor_remote_get_direct()", 2019-04-01)).
[I had a deja-vu moment writing this - it seems I sent a very
similar mail about 3 weeks ago. ;-) ]
Thanks!
ATB,
Ramsay Jones
promisor-remote.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/promisor-remote.h b/promisor-remote.h
index e29cf507ab..562c7ad8a4 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -1,6 +1,8 @@
#ifndef PROMISOR_REMOTE_H
#define PROMISOR_REMOTE_H
+struct object_id;
+
/*
* Promisor remote linked list
*
--
2.21.0
Re: [PATCH] commit-graph: fix sparse 'NULL pointer' warning
On 25/03/2019 12:02, Ævar Arnfjörð Bjarmason wrote:
>
> On Sat, Mar 23 2019, Ramsay Jones wrote:
>
>> Signed-off-by: Ramsay Jones
>> ---
>>
>> Hi Ævar,
>>
>> If you need to re-roll your 'ab/commit-graph-fixes' branch, could you
>> please squash this into the relevant patch (commit aeb244adbc
>> ("commit-graph: don't early exit(1) on e.g. \"git status\"", 2019-02-21).
>
> Thanks. Will squash & re-submit. It's still just in "pu". Is there a
> compiler that warns about it? Didn't on clang/gcc, but then again it's
> just-as-valid-C & just a style issue, so compilers don't care...
This is a 'sparse' warning (see subject line). :-D
I run the 'sparse' Makefile target on each branch and save to
a file, sp-out, nsp-out and psp-out, then diff from branch to
branch. So, currently, the diff between the 'next' and 'pu'
branches look like:
$ diff nsp-out psp-out
25a26
> commit-graph.c:103:24: warning: Using plain integer as NULL pointer
123a125
> SP promisor-remote.c
188a191
> upload-pack.c:1167:56: warning: Using plain integer as NULL pointer
289d291
< SP builtin/rebase--interactive.c
$
Alternatively, I can simply run 'sparse' over a single file:
$ make commit-graph.sp
SP commit-graph.c
commit-graph.c:103:24: warning: Using plain integer as NULL pointer
$
Of course, you need to have 'sparse' installed ... ;-)
>
>> This same commit (aeb244adbc) also removes the last call, outside of the
>> commit-graph.c file, to the function load_commit_graph_one(). So, this
>> function is now a file-local symbol and could be marked 'static'.
>>
>> Also, the function verify_commit_graph_lite(), introduced in commit
>> d8acf37ff7 ("commit-graph: fix segfault on e.g. \"git status\"",
>> 2019-02-21), currently has no external callers. This function is also
>> a file-local symbol and could be marked 'static', unless you have plans
>> for future external calls?
>
> Fixing these too. Just missed them. Thanks.
And these resulted from running my static-check.pl script.
ATB,
Ramsay Jones
[PATCH] commit-graph: fix sparse 'NULL pointer' warning
Signed-off-by: Ramsay Jones
---
Hi Ævar,
If you need to re-roll your 'ab/commit-graph-fixes' branch, could you
please squash this into the relevant patch (commit aeb244adbc
("commit-graph: don't early exit(1) on e.g. \"git status\"", 2019-02-21).
This same commit (aeb244adbc) also removes the last call, outside of the
commit-graph.c file, to the function load_commit_graph_one(). So, this
function is now a file-local symbol and could be marked 'static'.
Also, the function verify_commit_graph_lite(), introduced in commit
d8acf37ff7 ("commit-graph: fix segfault on e.g. \"git status\"",
2019-02-21), currently has no external callers. This function is also
a file-local symbol and could be marked 'static', unless you have plans
for future external calls?
Thanks!
ATB,
Ramsay Jones
commit-graph.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/commit-graph.c b/commit-graph.c
index 4696ee0036..680c6f5714 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -100,7 +100,7 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd,
struct stat *st)
if (graph_size < GRAPH_MIN_SIZE) {
close(fd);
error(_("commit-graph file is too small"));
- return 0;
+ return NULL;
}
graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
ret = parse_commit_graph(graph_map, fd, graph_size);
--
2.21.0
Re: [PATCH v8 10/11] sequencer.c: define describe_cleanup_mode
On 18/03/2019 20:04, Eric Sunshine wrote: > On Sun, Mar 17, 2019 at 6:16 AM Denton Liu wrote: >> Define a function which allows us to get the string configuration value >> of a enum commit_msg_cleanup_mode. This is done by refactoring >> get_cleanup_mode such that it uses a lookup table to find the mappings >> between string and enum and then using the same LUT in reverse to define >> describe_cleanup_mode. >> >> Reviewed-by: Eric Sunshine >> Reviewed-by: Junio C Hamano > > These two Reviewed-by: lines should be dropped for a couple reasons. > > First, neither Junio nor I reviewed _this_ version of the patch. > > Second, a Reviewed-by: is given explicitly (not taken). When a > reviewer has thoroughly read and understood a patch and considers it > problem-free, he or she may say explicitly "Reviewed-by: ", > stating satisfaction that the patch seems worthy of inclusion in the > project. If he sees fit, Junio may then pick up that Reviewed-by: at > the time he queues the patch in his tree. > >> Signed-off-by: Ramsay Jones I was similarly surprised to see this SOB line as well. There was nothing copyright-able in the 'squash patch' I sent, so I don't think this is warranted. (A 'Helped-by:' at _most_, I would think). ;-) ATB, Ramsay Jones
Re: [PATCH] packfile: use extra variable to clarify code in use_pack()
On 14/03/2019 00:19, Jeff King wrote:
> On Wed, Mar 13, 2019 at 09:49:58PM +0000, Ramsay Jones wrote:
>
>> From: Jeff King
>> [...]
>> Signed-off-by: Ramsay Jones
>
> Signed-off-by: Jeff King
>
> Naturally. :)
>
>> As promised, I am forwarding a 'saved' patch from Jeff, which was
>> a by-product of a long-ago discussion regarding commit 5efde212fc
>> ("zlib.c: use size_t for size", 2018-10-14).
>>
>> I have tested this patch on 'pu' (@6fd68134c8) and directly on top
>> of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib').
>>
>> However, whilst I have been waiting for the tests to finish, I have
>> been looking at the code and concluded that this does not _have_ to
>> be applied on top of commit 5efde212fc. (I haven't done it, but just
>> tweak the context line to read 'unsigned long *left)' rather than
>> 'size_t *left)' and this should apply cleanly to 'master'. Also, it
>> would have _exactly_ the same effect as the current code! ;-) ).
>
> I think it does apply, though the reasoning in the commit message of
> "this is OK because 'left' is large enough" becomes a lot more
> hand-wavy. The patch is not making anything _worse_, certainly, but the
> fact of the matter is that "left" still might not be big enough, if it
> is not a size_t.
Yep, the commit message would have to change (it says 'left' is
a size_t), but I think the patch is _still_ an improvement on
the existing code, even without s/unsigned long *left/size_t *left/.
(ie the code is still 'clarified'). :-D
Anyway, it was just an idle FYI while waiting. ;-)
ATB,
Ramsay Jones
Re: [PATCH] packfile: use extra variable to clarify code in use_pack()
On 13/03/2019 21:49, Ramsay Jones wrote:
> From: Jeff King
>
> We use the "offset" variable for two purposes. It's the offset into
> the packfile that the caller provides us (which is rightly an off_t,
> since we might have a packfile much larger than memory). But later we
> also use it as the offset within a given mmap'd window, and that
> window cannot be larger than a size_t.
>
> For the second use, the fact that we have an off_t leads to some
> confusion when we assign it to the "left" variable, which is a size_t.
> It is in fact correct (because our earlier "offset -= win->offset" means
> we must be within the pack window), but using a separate variable of the
> right type makes that much more obvious.
>
> Signed-off-by: Ramsay Jones
> ---
>
> Hi Junio,
>
> As promised, I am forwarding a 'saved' patch from Jeff, which was
> a by-product of a long-ago discussion regarding commit 5efde212fc
> ("zlib.c: use size_t for size", 2018-10-14).
>
> I have tested this patch on 'pu' (@6fd68134c8) and directly on top
> of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib').
>
> However, whilst I have been waiting for the tests to finish, I have
> been looking at the code and concluded that this does not _have_ to
> be applied on top of commit 5efde212fc. (I haven't done it, but just
> tweak the context line to read 'unsigned long *left)' rather than
> 'size_t *left)' and this should apply cleanly to 'master'. Also, it
> would have _exactly_ the same effect as the current code! ;-) ).
I have now done:
$ diff 0001-packfile-use-extra-variable-to-clarify-code-in-use_p.patch
ttt.patch
28c28
< size_t *left)
---
> unsigned long *left)
$
... this and it applies cleanly to 'master', builds and passes tests.
Just FYI. ;-)
ATB,
Ramsay Jones
[PATCH] packfile: use extra variable to clarify code in use_pack()
From: Jeff King
We use the "offset" variable for two purposes. It's the offset into
the packfile that the caller provides us (which is rightly an off_t,
since we might have a packfile much larger than memory). But later we
also use it as the offset within a given mmap'd window, and that
window cannot be larger than a size_t.
For the second use, the fact that we have an off_t leads to some
confusion when we assign it to the "left" variable, which is a size_t.
It is in fact correct (because our earlier "offset -= win->offset" means
we must be within the pack window), but using a separate variable of the
right type makes that much more obvious.
Signed-off-by: Ramsay Jones
---
Hi Junio,
As promised, I am forwarding a 'saved' patch from Jeff, which was
a by-product of a long-ago discussion regarding commit 5efde212fc
("zlib.c: use size_t for size", 2018-10-14).
I have tested this patch on 'pu' (@6fd68134c8) and directly on top
of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib').
However, whilst I have been waiting for the tests to finish, I have
been looking at the code and concluded that this does not _have_ to
be applied on top of commit 5efde212fc. (I haven't done it, but just
tweak the context line to read 'unsigned long *left)' rather than
'size_t *left)' and this should apply cleanly to 'master'. Also, it
would have _exactly_ the same effect as the current code! ;-) ).
So, dunno.
ATB,
Ramsay Jones
packfile.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/packfile.c b/packfile.c
index b0efe8cb3d..0e59f929c5 100644
--- a/packfile.c
+++ b/packfile.c
@@ -622,6 +622,7 @@ unsigned char *use_pack(struct packed_git *p,
size_t *left)
{
struct pack_window *win = *w_cursor;
+ size_t offset_in_window;
/* Since packfiles end in a hash of their content and it's
* pointless to ask for an offset into the middle of that
@@ -683,10 +684,14 @@ unsigned char *use_pack(struct packed_git *p,
win->inuse_cnt++;
*w_cursor = win;
}
- offset -= win->offset;
+ /*
+* We know this difference will fit in a size_t, because our mmap
+* window by definition can be no larger than a size_t.
+*/
+ offset_in_window = xsize_t(offset - win->offset);
if (left)
- *left = win->len - xsize_t(offset);
- return win->base + offset;
+ *left = win->len - offset_in_window;
+ return win->base + offset_in_window;
}
void unuse_pack(struct pack_window **w_cursor)
--
2.21.0
[PATCH] promisor-remote.h: fix a 'hdr-check' warning
Signed-off-by: Ramsay Jones
---
Hi Christian,
If you need to re-roll your 'cc/multi-promisor' branch, could you
please squash this into the relevant patch (commit 9e8a7cf4be
("promisor-remote: implement promisor_remote_get_direct()", 2019-03-12)).
Also, I note that the promisor_remote_new() function is not called
outside of 'promisor-remote.c' and so could be a file-local symbol.
Are there any plans for future calls to this function (outside of
this file)? If not, could you please mark this function as static
and remove the extern declaration.
Thanks!
ATB,
Ramsay Jones
promisor-remote.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/promisor-remote.h b/promisor-remote.h
index f0d609a3f5..d90df11996 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -1,6 +1,8 @@
#ifndef PROMISOR_REMOTE_H
#define PROMISOR_REMOTE_H
+struct object_id;
+
/*
* Promisor remote linked list
*
--
2.21.0
Re: [RFC/PATCH] packfile: use extra variable to clarify code in use_pack()
On 12/03/2019 20:26, Jeff King wrote:
> On Tue, Mar 12, 2019 at 05:39:13PM +0000, Ramsay Jones wrote:
>
>> On 12/03/2019 16:55, Ramsay Jones wrote:
>>> From: Jeff King
>>>
>>> Signed-off-by: Ramsay Jones
>
> Could definitely use a commit message. I think it's something like:
>
> We use the "offset" variable for two purposes. It's the offset into
> the packfile that the caller provides us (which is rightly an off_t,
> since we might have a packfile much larger than memory). But later we
> also use it as the offset within a given mmap'd window, and that
> window cannot be larger than a size_t.
>
> For the second use, the fact that we have an off_t leads to some
> confusion when we assign it to the "left" variable, is a size_t. It is
> in fact correct (because our earlier "offset -= win->offset" means we
> must be within the pack window), but using a separate variable of the
> right type makes that much more obvious.
>
> You'll note that I snuck in the assumption that "left" is a size_t,
> which as you noted is not quite valid yet. :)
Looks good to me! :-D
>> Heh, of course I should have tried applying on top of today's
>> codebase before sending it out! :(
>>
>> Having just done so, it quickly showed that this patch assumes
>> that the 'left' parameter to use_pack() has been changed from
>> an 'unsigned long *' to an 'size_t *' as part of the series
>> that was being discussed in the above link.
>
> Yep. Until then, I do not think there is much point (and in fact I'd
> suspect this code behaves incorrectly on Windows, where "unsigned long"
> is too short; hopefully they clamp pack windows to 4GB by default
> there, which would work around it).
>
> But I would be very happy if you wanted to resurrect the "left" patch
> and then do this on top. :)
It actually applies on top of the 'pu' branch, because the 'left'
patch is commit 5efde212fc ("zlib.c: use size_t for size", 2018-10-18).
If you recall, this was going to be just the first step in a series
of patches to replace 'unsigned long' as the type used for various
'size' or 'length' variables.
So, if I add the above commit message, it could apply on top of the
current 'mk/use-size-t-in-zlib' branch.
I may not get to that tonight (busy with something else), but I can
hopefully do that tomorrow.
ATB,
Ramsay Jones
Re: [RFC/PATCH] packfile: use extra variable to clarify code in use_pack()
On 12/03/2019 16:55, Ramsay Jones wrote: > From: Jeff King > > Signed-off-by: Ramsay Jones > --- > > Hi Jeff, > > I recently tried (yet again) to tidy up some old branches. When I get > around to doing a 'git gc; git fsck' I always take a quick look at > the 'dangling' commits, just before a 'git gc --prune=now'. > > I had no recollection of this commit, from last October, but a quick > look at the ML archive found this [1] discussion. I obviously thought > it was worth saving this thought of yours. ;-) So, having deleted this > already, I did a quick 'format-patch' to see if anyone thinks it is > worth applying. > > [1] > https://public-inbox.org/git/20181013024624.gb15...@sigill.intra.peff.net/#t > Heh, of course I should have tried applying on top of today's codebase before sending it out! :( Having just done so, it quickly showed that this patch assumes that the 'left' parameter to use_pack() has been changed from an 'unsigned long *' to an 'size_t *' as part of the series that was being discussed in the above link. Ah, well, sorry for the noise! ATB, Ramsay Jones
[RFC/PATCH] packfile: use extra variable to clarify code in use_pack()
From: Jeff King
Signed-off-by: Ramsay Jones
---
Hi Jeff,
I recently tried (yet again) to tidy up some old branches. When I get
around to doing a 'git gc; git fsck' I always take a quick look at
the 'dangling' commits, just before a 'git gc --prune=now'.
I had no recollection of this commit, from last October, but a quick
look at the ML archive found this [1] discussion. I obviously thought
it was worth saving this thought of yours. ;-) So, having deleted this
already, I did a quick 'format-patch' to see if anyone thinks it is
worth applying.
[1] https://public-inbox.org/git/20181013024624.gb15...@sigill.intra.peff.net/#t
Thanks!
ATB,
Ramsay Jones
packfile.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/packfile.c b/packfile.c
index 013294aec7..2f81ec9345 100644
--- a/packfile.c
+++ b/packfile.c
@@ -588,6 +588,7 @@ unsigned char *use_pack(struct packed_git *p,
size_t *left)
{
struct pack_window *win = *w_cursor;
+ size_t offset_in_window;
/* Since packfiles end in a hash of their content and it's
* pointless to ask for an offset into the middle of that
@@ -649,10 +650,14 @@ unsigned char *use_pack(struct packed_git *p,
win->inuse_cnt++;
*w_cursor = win;
}
- offset -= win->offset;
+ /*
+* We know this difference will fit in a size_t, because our mmap
+* window by definition can be no larger than a size_t.
+*/
+ offset_in_window = xsize_t(offset - win->offset);
if (left)
- *left = win->len - xsize_t(offset);
- return win->base + offset;
+ *left = win->len - offset_in_window;
+ return win->base + offset_in_window;
}
void unuse_pack(struct pack_window **w_cursor)
--
2.21.0
[PATCH] sequencer: mark a file-local symbol as static
Signed-off-by: Ramsay Jones
---
Hi Denton,
If you need to re-roll your 'dl/merge-cleanup-scissors-fix' branch,
could you please squash this into the relevant patch (commit 9bcdf520cb
("sequencer.c: define get_config_from_cleanup", 2019-03-10)).
I note also, that the get_config_from_cleanup() function is not
called outside of sequencer.c, so that this function could also
be marked static (and remove the extern declaration from the
header file) if there are no plans for future callers.
Thanks!
ATB,
Ramsay Jones
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 19d1279fa8..833017eb2d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -166,7 +166,7 @@ struct cleanup_config_mapping {
};
/* note that we assume that cleanup_config_mapping[0] contains the default
settings */
-struct cleanup_config_mapping cleanup_config_mappings[] = {
+static struct cleanup_config_mapping cleanup_config_mappings[] = {
{ "default", COMMIT_MSG_CLEANUP_ALL, COMMIT_MSG_CLEANUP_SPACE },
{ "verbatim", COMMIT_MSG_CLEANUP_NONE, COMMIT_MSG_CLEANUP_NONE },
{ "whitespace", COMMIT_MSG_CLEANUP_SPACE, COMMIT_MSG_CLEANUP_SPACE },
--
2.21.0
[PATCH] upload-pack: fix sparse warnings
Signed-off-by: Ramsay Jones
---
Hi Jonathan,
If you need to re-roll your 'jt/fetch-cdn-offload' branch, could
you please squash this into the relevant patches. (The first hunk
into commit a8d662e3da4 ("upload-pack: refactor reading of pack-objects
out", 2019-03-08) and the second hunk into commit 820a5361db1
("upload-pack: send part of packfile response as uri", 2019,-03-08)).
[Johannes mentioned 'clang' complaining - I have clang v5.0.1 and
it does not issue any warnings for the new initialization.]
This patch fixes the following sparse warnings:
$ diff psp-out psp-out1
190,191d189
< upload-pack.c:182:45: warning: missing braces around initializer
< upload-pack.c:1167:56: warning: Using plain integer as NULL pointer
$
If you don't like the new initializer expression, maybe don't
initialize in the declaration and use a traditional:
memset(&output_state, 0, sizeof(struct output_state));
instead?
Thanks!
ATB,
Ramsay Jones
upload-pack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index d36e1fc06a..52309d40ae 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -179,7 +179,7 @@ static void create_pack_file(const struct object_array
*have_obj,
const struct string_list *uri_protocols)
{
struct child_process pack_objects = CHILD_PROCESS_INIT;
- struct output_state output_state = {0};
+ struct output_state output_state = { { 0 }, 0, 0, 0 };
char progress[128];
char abort_msg[] = "aborting due to possible repository "
"corruption on the remote side.";
@@ -1164,7 +1164,7 @@ void upload_pack(struct upload_pack_options *options)
if (want_obj.nr) {
struct object_array have_obj = OBJECT_ARRAY_INIT;
get_common_commits(&reader, &have_obj, &want_obj);
- create_pack_file(&have_obj, &want_obj, 0);
+ create_pack_file(&have_obj, &want_obj, NULL);
}
}
--
2.21.0
[PATCH] upload-pack.c: fix a sparse 'NULL pointer' warning
Signed-off-by: Ramsay Jones
---
Hi Jonathan,
If you need to re-roll your 'jt/fetch-cdn-offload' branch, could you
please squash this into the relevant patch (commit 0e821b4427
("upload-pack: send part of packfile response as uri", 2019-02-23)).
Thanks!
ATB,
Ramsay Jones
upload-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/upload-pack.c b/upload-pack.c
index 534e8951a2..a421df2bbb 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1164,7 +1164,7 @@ void upload_pack(struct upload_pack_options *options)
if (want_obj.nr) {
struct object_array have_obj = OBJECT_ARRAY_INIT;
get_common_commits(&reader, &have_obj, &want_obj);
- create_pack_file(&have_obj, &want_obj, 0);
+ create_pack_file(&have_obj, &want_obj, NULL);
}
}
--
2.21.0
Re: [PATCH v3 00/21] Add new command "switch"
On 08/03/2019 09:57, Nguyễn Thái Ngọc Duy wrote: [snip] > Range-diff dựa trên v2: > -: -- > 1: 949f3dd4fd git-checkout.txt: spell out --no-option > 1: 8358b9ca36 = 2: 1ddbbae3e2 git-checkout.txt: fix one syntax line > 2: 1686ccbf8d ! 3: b0cb2372db doc: document --overwrite-ignore > @@ -14,14 +14,15 @@ > out anyway. In other words, the ref can be held by more than one > worktree. > > -+--[no-]overwrite-ignore:: > ++--overwrite-ignore:: > ++--no-overwrite-ignore:: Just curious, but why? Is '--[no-]overwrite-ignore' thought to be harder to read? What about the rest of the man-pages? ATB, Ramsay Jones
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
On 05/03/2019 23:07, Jeff King wrote: > On Tue, Mar 05, 2019 at 02:50:11PM +0900, Junio C Hamano wrote: > >> Jeff King writes: >> >>> This makes sense to me, though as you noted elsewhere, it doesn't fix >>> the gcrypt problem, since that file unconditionally wants to look at the >>> system gcrypt.h (and we control at the Makefile level whether we >>> actually look at sha256/gcrypt.h). >> >> Hmm, is that because the header check target does not know which *.h >> files we ship are actually used in a particular build? > > Yes, exactly. > >> After a normal build, with dynamic dependency checking on, we would >> have sufficient information to figure it out. Would that help? > > Yeah, that's what I was hinting at earlier in the thread. Here it is > sketched out to an actual working patch. The sub-make bits could > actually be a shell script instead of a Makefile; the only point in > using make is to use the parent "-j" for parallelism. sigh. :( I wish my patch removing this target had been picked up now! Earlier this evening I spent an hour or so writing an email which tried to discourage spending any time on this, because of the potential for this to be a huge time-suck. An unrewarding one at that! :-D The email was actually prompted by someone mentioning 'dynamic compiler dependencies', because that's how it always starts ... I deleted that email (it's not in my drafts folder anyway) because, in the end, it is not up to me to say how people should spend their time. So I won't! :-D ATB, Ramsay Jones
[PATCH] Makefile: fix 'hdr-check' when GCRYPT not installed
If the GCRYPT_SHA256 build variable is not set, then the 'hdr-check' target complains about the missing header file. Add the 'sha256/gcrypt.h' header file to the exception list, if the build variable is not defined. While here, replace the 'xdiff%' filter pattern with 'xdiff/%' (and similarly for the compat pattern) since the original pattern inadvertently excluded the 'xdiff-interface.h' header. Signed-off-by: Ramsay Jones --- Hi Junio, In my local 'hdr-check-fixup' branch, the gcrypt failure is fixed almost by default. However, given that we are going the 'git ls-files' route, this patch will fix it up. Thanks! ATB, Ramsay Jones Makefile | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index c5240942f2..fbc84cb541 100644 --- a/Makefile +++ b/Makefile @@ -2736,7 +2736,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE sparse: $(SP_OBJ) GEN_HDRS := command-list.h unicode-width.h -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% +EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/% +ifndef GCRYPT_SHA256 + EXCEPT_HDRS += sha256/gcrypt.h +endif CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) -- 2.21.0
Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible
On 04/03/2019 20:38, Ramsay Jones wrote: > > > On 04/03/2019 13:47, Johannes Schindelin via GitGitGadget wrote: >> From: Johannes Schindelin >> >> In d85b0dff72 (Makefile: use `find` to determine static header >> dependencies, 2014-08-25), we switched from a static list of header >> files to a dynamically-generated one, asking `find` to enumerate them. >> >> Back in those days, we did not use `$(LIB_H)` by default, and many a >> `make` implementation seems smart enough not to run that `find` command >> in that case, so it was deemed okay to run `find` for special targets >> requiring this macro. >> >> However, as of ebb7baf02f (Makefile: add a hdr-check target, >> 2018-09-19), $(LIB_H) is part of a global rule and therefore must be >> expanded. Meaning: this `find` command has to be run upon every >> `make` invocation. In the presence of many a worktree, this can tax the >> developers' patience quite a bit. >> >> Even in the absence of worktrees or other untracked files and >> directories, the cost of I/O to generate that list of header files is >> simply a lot larger than a simple `git ls-files` call. >> >> Therefore, just like in 335339758c (Makefile: ask "ls-files" to list >> source files if available, 2011-10-18), we now prefer to use `git >> ls-files` to enumerate the header files to enumerating them via `find`, >> falling back to the latter if the former failed (which would be the case >> e.g. in a worktree that was extracted from a source .tar file rather >> than from a clone of Git's sources). >> >> This has one notable consequence: we no longer include `command-list.h` >> in `LIB_H`, as it is a generated file, not a tracked one, but that is > > Heh, just to be _unnecessarily_ picky, but this is not always true. > The 'command-list.h' header is _sometimes_ not included in the LIB_H > variable - it simply depends on whether it has been generated by the > time the $(FIND) is called. > > Obviously, not worth a re-roll. Otherwise, this LGTM. Ahem! Obviously, I didn't read the commit message closely enough! However, _before_ this change, then 'command-list.h' was sometimes not included in $(LIB_H) ... Sorry for the noise! ATB, Ramsay Jones
Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible
On 04/03/2019 13:47, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > In d85b0dff72 (Makefile: use `find` to determine static header > dependencies, 2014-08-25), we switched from a static list of header > files to a dynamically-generated one, asking `find` to enumerate them. > > Back in those days, we did not use `$(LIB_H)` by default, and many a > `make` implementation seems smart enough not to run that `find` command > in that case, so it was deemed okay to run `find` for special targets > requiring this macro. > > However, as of ebb7baf02f (Makefile: add a hdr-check target, > 2018-09-19), $(LIB_H) is part of a global rule and therefore must be > expanded. Meaning: this `find` command has to be run upon every > `make` invocation. In the presence of many a worktree, this can tax the > developers' patience quite a bit. > > Even in the absence of worktrees or other untracked files and > directories, the cost of I/O to generate that list of header files is > simply a lot larger than a simple `git ls-files` call. > > Therefore, just like in 335339758c (Makefile: ask "ls-files" to list > source files if available, 2011-10-18), we now prefer to use `git > ls-files` to enumerate the header files to enumerating them via `find`, > falling back to the latter if the former failed (which would be the case > e.g. in a worktree that was extracted from a source .tar file rather > than from a clone of Git's sources). > > This has one notable consequence: we no longer include `command-list.h` > in `LIB_H`, as it is a generated file, not a tracked one, but that is Heh, just to be _unnecessarily_ picky, but this is not always true. The 'command-list.h' header is _sometimes_ not included in the LIB_H variable - it simply depends on whether it has been generated by the time the $(FIND) is called. Obviously, not worth a re-roll. Otherwise, this LGTM. Thanks! ATB, Ramsay Jones > easily worked around. Of the three sites that use `LIB_H`, two > (`LOCALIZED_C` and `CHK_HDRS`) already handle generated headers > separately. In the third, the computed-dependency fallback, we can just > add in a reference to $(GENERATED_H). > > Likewise, we no longer include not-yet-tracked header files in `LIB_H`. > > Given the speed improvements, these consequences seem a comparably small > price to pay. > > Signed-off-by: Johannes Schindelin > --- > Makefile | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index c5240942f2..0c4712cb48 100644 > --- a/Makefile > +++ b/Makefile > @@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a > > GENERATED_H += command-list.h > > -LIB_H = $(shell $(FIND) . \ > +LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || > \ > + $(FIND) . \ > -name .git -prune -o \ > -name t -prune -o \ > -name Documentation -prune -o \ > @@ -2363,7 +2364,7 @@ else > # should _not_ be included here, since they are necessary even when > # building an object for the first time. > > -$(OBJECTS): $(LIB_H) > +$(OBJECTS): $(LIB_H) $(GENERATED_H) > endif > > exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX >
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
; the GCRYPT error is simply an accident of timing! Today, it would be conditionally added to the exception list. Yes, I watched that error go from the 'pu' branch down to the 'master' branch). >> +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS)) >>HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) >> >>$(HCO): %.hco: %.h FORCE >> >> >> ... which drops the use of LIB_H entirely. >> >> So, if we really need to solve the problem, allowing for some >> unrelated headers in your worktree, then we can only use a >> static list, or a 'git ls-files' approach. > > Or we can use `ls-files`, and fall back to your wildcard idea if > `ls-files` fails for some reason (typically because `.git/` is missing, > e.g. in case of an unpacked source .tar). Yes, I think an 'git ls-files' approach is the way to go. (I am not sure that the 'hdr-check' target would be of any use 'offline' at all, but I suppose we could use a generated file in that case). >> Anyway, for now, since I seem to be the only person using this >> target, I think we should just remove it while I think again. >> (I can put it in my config.mak, so there will be no loss for me). > > As I said above, I would rather keep it, with the `ls-files` and `:=` > fixup. I would be happy with that, if you are. :-D ATB, Ramsay Jones
[PATCH] Makefile: remove the 'hdr-check' target
The 'hdr-check' target has proved to be costly for some developers and
platforms, depending on the configuration, even when not using this
target. In part, this is due to the use of $(FIND) in the definition
of the $(LIB_H) variable. This effectively reverts commit ebb7baf02f
("Makefile: add a hdr-check target", 2018-09-19).
Signed-off-by: Ramsay Jones
---
Makefile | 12
1 file changed, 12 deletions(-)
diff --git a/Makefile b/Makefile
index c5240942f2..dd3e38dc1f 100644
--- a/Makefile
+++ b/Makefile
@@ -1852,7 +1852,6 @@ 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; \
@@ -2735,17 +2734,6 @@ $(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.21.0
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
On 03/03/2019 17:19, Jeff King wrote: > On Sat, Mar 02, 2019 at 09:05:24PM +0100, Johannes Schindelin wrote: > >>> That seems reasonable (regardless of whether it is in a script or in the >>> Makefile). Another option is to use -maxdepth, but that involves >>> guessing how deep people might actually put header files. >> >> It would also fail to work when somebody clones an unrelated repository >> that contains header files in the top-level directory into the Git >> worktree. I know somebody like that: me. > > Good point. [Sorry for the late reply - I have been AWOL this weekend and I am only just catching up with email! :-D ] So, tl;dr: soon, I will be submitting a patch to remove the 'hdr-check' target completely, for now anyway. > By the way, "make hdr-check" already fails for me on master, as I do not have > libgcrypt installed, and it unconditionally checks sha256/gcrypt.h. Yep, for me too. There is a similar problem if you build with NO_CURL and don't have the 'curl/curl.h' header file, etc ... The first version of the 'hdr-check' target re-introduced a static list of header files, but I didn't think people would appreciate having to maintain the list manually, so I gave up on that! The next version was essentially the same patch that started this thread from Johannes (ie. the 'git ls-files' patch), given that the 'tags' targets had found it necessary. However, when I did some 'informal' timing tests (ie 5x 'time make ...' and average), this did not make any noticeable difference for me (_even_ on cygwin!). ;-) Of course, I had already made the mistake of trying to re-use something that was 'handy' (ie. LIB_H) and already there. However, LIB_H wasn't quite what I wanted - I needed a sub-set of it. So, I already have a 'hdr-check-fixup' branch (I think I have already mentioned it), in which the first commit looks like: diff --git a/Makefile b/Makefile index c5240942f2..3401d1b9fb 100644 --- a/Makefile +++ b/Makefile @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ) +HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \ + sha1dc/*.h refs/*.h vcs-svn/*.h) GEN_HDRS := command-list.h unicode-width.h -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS)) HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) $(HCO): %.hco: %.h FORCE ... which drops the use of LIB_H entirely. Now, I have timed this and, for me, it no faster ... (I suspect that it would be faster for Johannes, but it would still cause a problem if you have 'top-level header files from some other repo ...'). So, if we really need to solve the problem, allowing for some unrelated headers in your worktree, then we can only use a static list, or a 'git ls-files' approach. Anyway, for now, since I seem to be the only person using this target, I think we should just remove it while I think again. (I can put it in my config.mak, so there will be no loss for me). ATB, Ramsay Jones
Re: [PATCH v2 2/6] Makefile: move "strip" assignment down from flags
On 22/02/2019 15:18, Jeff King wrote:
> On Fri, Feb 22, 2019 at 03:41:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> Move the assignment of the "STRIP" variable down to where we're
>> setting variables with the names of other programs.
>>
>> For consistency with those use "=" for the assignment instead of
>> "?=". I can't imagine why this would need to be different than the
>> rest, and 4dc00021f7 ("Makefile: add 'strip' target", 2006-01-12)
>> which added it doesn't provide an explanation.
>
> This might annoy somebody expecting $STRIP in the environment to have
> precedence. But I agree that consistency is probably our best strategy
> here, and I don't see any reason the same argument would not apply to
> $SPATCH, or $CC for that matter.
>
> (So I could see an argument for moving them all to "?=", but that can
> create its own confusion as environment variables accidentally start
> taking effect).
$STRIP and $SPATCH will work OK, but you would be disappointed
with $CC (or any other variable from make's built-in database). ;-)
Try this:
$ cat -n Makefile
1
2CC ?= gcc
3
4all:
5@echo "CC is $(CC), origin " $(origin CC)
6
$
The command-line works OK:
$ make CC=cmd-line
CC is cmd-line, origin command line
$
So does the environment:
$ CC=env make
CC is env, origin environment
$
But that conditional assignment:
$ make
CC is cc, origin default
$
... not so much! :-D
ATB,
Ramsay Jones
Re: [PATCH v2 1/1] worktree add: sanitize worktree names
On 21/02/2019 12:19, Nguyễn Thái Ngọc Duy wrote:
> Worktree names are based on $(basename $GIT_WORK_TREE). They aren't
> significant until 3a3b9d8cde (refs: new ref types to make per-worktree
> refs visible to all worktrees - 2018-10-21), where worktree name could
> be part of a refname and must follow refname rules.
>
> Update 'worktree add' code to remove special characters to follow
> these rules. The code could replace chars with '-' more than
> necessary, but it keeps the code simple. In the future the user will
> be able to specify the worktree name by themselves if they're not
> happy with this dumb character substitution.
>
> Reported-by: Konstantin Kharlamov
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/worktree.c | 51 -
> t/t2025-worktree-add.sh | 7 ++
> 2 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 3f9907fcc9..53e41db229 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -262,6 +262,50 @@ static void validate_worktree_add(const char *path,
> const struct add_opts *opts)
> free_worktrees(worktrees);
> }
>
> +/*
> + * worktree name is part of refname and has to pass
> + * check_refname_component(). Remove unallowed characters to make it
> + * valid.
> + */
> +static void sanitize_worktree_name(struct strbuf *name)
> +{
> + char *orig_name = xstrdup(name->buf);
> + int i;
> +
> + /*
> + * All special chars replaced with dashes. See
> + * check_refname_component() for reference.
> + * Note that .lock is also turned to -lock, removing its
> + * special status.
> + */
> + for (i = 0; i < name->len; i++) {
> + if (strchr(":?[]\\~ \t@{}*/.", name->buf[i]))
> + name->buf[i] = '-';
> + }
> +
> + /* remove consecutive dashes, leading or trailing dashes */
Why? So, '[fred]' will be 'sanitized' to 'fred' (rather than '-fred-'),
which would increase the chance of a 'collision' with the 'fred'
worktree (not very likely, but still). Is that useful? How about
'x86_64-*-gnu' which now becomes 'x86_64-gnu'?
> + for (i = 0; i < name->len; i++) {
> + while (name->buf[i] == '-' &&
> +(i == 0 ||
> + i == name->len - 1 ||
> + (i < name->len - 1 && name->buf[i + 1] == '-')))
> + strbuf_remove(name, i, 1);
> + }
> +
> + /*
> + * a worktree name of only special chars would be reduced to
> + * an empty string
> + */> + if (name->len == 0)
> + strbuf_addstr(name, "worktree");
If you didn't 'collapse' the name above, you could check for
an empty name at the top and wouldn't need this (presumably
an empty name would not be valid).
ATB,
Ramsay Jones
Re: [Fix v1] builtin/ls-files.c: add error check on lstat for modified files
On 17/02/2019 16:34, randall.s.bec...@rogers.com wrote: > From: "Randall S. Becker" > > The result from lstat, checking whether a file has been deleted, is now > included priot to calling id_modified when showing modified files. Prior s/priot/prior/; s/id_modified/ie_modified/ > to this fix, it is possible that files that were deleted could show up > as being modified because the lstat error was unchecked. > > Reported-by: Joe Ranieri > Signed-off-by: Randall S. Becker > --- > builtin/ls-files.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/ls-files.c b/builtin/ls-files.c > index 29a8762d4..fc21f4795 100644 > --- a/builtin/ls-files.c > +++ b/builtin/ls-files.c > @@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct > dir_struct *dir) > err = lstat(fullname.buf, &st); > if (show_deleted && err) To be pedantic, this should probably check for (err == ENOENT), since lstat() can fail for several reasons which don't imply that the path has been deleted. However, that is unlikely. No reason to include such a check in this patch, of course. ATB, Ramsay Jones > show_ce(repo, dir, ce, fullname.buf, > tag_removed); > - if (show_modified && ie_modified(repo->index, ce, &st, > 0)) > + if (show_modified && !err && ie_modified(repo->index, > ce, &st, 0)) > show_ce(repo, dir, ce, fullname.buf, > tag_modified); > } > } >
Re: [PATCH v2] read-cache: add post-indexchanged hook
On 14/02/2019 14:42, Ben Peart wrote: > From: Ben Peart > > Add a post-indexchanged hook that is invoked after the index is written in s/post-indexchanged/post-index-changed/ > do_write_locked_index(). > > This hook is meant primarily for notification, and cannot affect > the outcome of git commands that trigger the index write. > > The hook is passed a flag to indicate whether the working directory was > updated or not and a flag indicating if a skip-worktree bit could have > changed. These flags enable the hook to optmize its response to the s/optmize/optimize/ ATB, Ramsay Jones
[PATCH] prune-packed: check for too many arguments
Signed-off-by: Ramsay Jones
---
Hi Junio,
Another 'old find' (from feb 2017), but this time I don't seem to have
sent this one to the list before. It is possible that I didn't think
it was up to scratch ... dunno. ;-)
ATB,
Ramsay Jones
builtin/prune-packed.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index a9e7b552b9..48c5e78e33 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -63,6 +63,11 @@ int cmd_prune_packed(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, prune_packed_options,
prune_packed_usage, 0);
+ if (argc > 0)
+ usage_msg_opt(_("too many arguments"),
+ prune_packed_usage,
+ prune_packed_options);
+
prune_packed_objects(opts);
return 0;
}
--
2.20.0
[PATCH] sequencer: make sign_off_header a file local symbol
Commit d0aaa46fd3 ("commit: move empty message checks to libgit",
2017-11-10) removes the last use of 'sign_off_header' outside of
the "sequencer.c" source file. Remove the extern declaration from
the header file and mark the definition of the symbol with the
static keyword.
Signed-off-by: Ramsay Jones
---
Hi Junio,
This has been hanging around for a while. I sent it to the list last
time in [1], but it seems to have been dropped. (Found while attempting
to rebase loads of old branches to a newer base!)
ATB,
Ramsay Jones
[1]
https://public-inbox.org/git/8caabe3e-4dc3-657a-236d-6cf91e1e6...@ramsayjones.plus.com/
sequencer.c | 2 +-
sequencer.h | 2 --
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index e1a4dd15f1..20f1823b06 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -35,7 +35,7 @@
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
-const char sign_off_header[] = "Signed-off-by: ";
+static const char sign_off_header[] = "Signed-off-by: ";
static const char cherry_picked_prefix[] = "(cherry picked from commit ";
GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
diff --git a/sequencer.h b/sequencer.h
index 5071a73563..e5e4845f14 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -102,8 +102,6 @@ int complete_action(struct replay_opts *opts, unsigned
flags,
unsigned autosquash);
int rearrange_squash(void);
-extern const char sign_off_header[];
-
/*
* Append a signoff to the commit message in "msgbuf". The ignore_footer
* parameter specifies the number of bytes at the end of msgbuf that should
--
2.20.0
Re: Confusion about the PACK format
On 10/02/2019 19:05, Ramsay Jones wrote: > > > On 10/02/2019 16:02, Florian Steenbuck wrote: >> Hello to all, >> >> I try to understand the git protocol only on the server site. So I >> start without reading any docs and which turns to be fine until I got >> to the PACK format (pretty early failure I know). >> >> I have read this documentation: >> https://raw.githubusercontent.com/git/git/c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1/Documentation/technical/pack-format.txt >> >> But their are some confusion about this text. >> >> The basic header is no problem, but somehow I got stuck while try to >> read the length and type of the objects, which are ints that can be >> resolved with 3-bits and 4-bits. The question is where and how ? >> > > Hmm, the 'type and length' encoding could be described more clearly! > Hopefully, just on this issue, the following could help: > > In my git.git repo, which is fully packed, I have a single pack file, with > > $ git count-objects -v > count: 0 > size: 0 > in-pack: 270277 > packs: 1 > size-pack: 101929 > prune-packable: 0 > garbage: 0 > size-garbage: 0 > $ > > ... 270277 objects in it. The beginning of the file looks like: > > $ xxd .git/objects/pack/pack-d554e6d8335601c2525b40487faf36493094ab50.pack > | head > : 5041 434b 0002 0004 1fc5 9d13 789c PACK..x. > 0010: 9d8f cd6a c330 1084 ef7a 8a3d 171a b4ab ...j.0...z.= > 0020: 9525 8750 0abd 945c f304 ab95 5cfb 602b .%.P...\\.`+ > 0030: b84a 7fde 3e2a 943e 406f c3f0 cd30 d3f6 .J..>*.>@o...0.. > 0040: 5260 741a 5025 92e2 1458 917c c294 a3c3 R`t.P%...X.| > 0050: 4803 e521 395f c2d8 4d73 95bd 6c0d 82f5 H..!9_..Ms..l... > 0060: 6172 310f 0529 7a2f d6a7 40c5 d9a0 d185 ar1..)z/..@. > 0070: 622d 8789 9cb8 3f1e 5132 6366 4de4 8531 b-?.Q2cfM..1 > 0080: 114a 70ec 9447 2f5a 526f e29c 3847 23b7 .Jp..G/ZRo..8G#. > 0090: 36d7 1dce b76d a9f0 02af b2ca 56e1 f4b6 6m..V... > $ > > You can see the header, which consists of 3 32-bit values, where the > packfile signature is the '5041 434b', then the version number which > is ' 0002', followed by the number of objects '0004 1fc5' which > is 270277. Next comes the first 'object entry', which starts '9d13'. > > Now, the 'n-byte type and length' is a variable length encoding of > the object type and length. The number of bytes used to encode this > data is content dependant. If the top bit of a byte is set, then we > need to process the next byte, otherwise we are done. So, looking > at the first 'object entry' byte (at offset 12) '9d', we take the > top nibble, remove the top bit, and shift right 4 bits to get the > object type. ie. (0x9d >> 4) & 7 which gives an object type of 1 > (which is a commit object). The lower nibble of the first byte > contains the first (or only) 4 bits of the size, here (0x9d & 15) > which is 0xd. Given that the top bit of this byte is set, we now > process the next byte. After the first byte, each byte contains 7 > bits of the size field which is combined with the value from the > previous byte by shifting and adding (first by 4 bits, then 11, 18, > 25 etc.). So, in this case we have (0x13 << 4) + 0xd = 317. Sorry, to be clear, I should have said, "mask off the top bit, shift and add", so: ((0x13 & 0x7f) << 4) + 0xd = 317 ATB, Ramsay Jones > > The compressed data follows, '789c' ... > > We can use git-verify-pack to confirm the details here: > > $ git verify-pack -v > .git/objects/pack/pack-d554e6d8335601c2525b40487faf36493094ab50.idx | head -n > 1 > 878e2cd30e1656909c5073043d32fe9d02204daa commit 317 216 12 > $ > > So the object 878e2cd30e, at offset 12 in the file, is a commit object > with size 317 (which has an in-pack size of 216). > > Hope this helps. > > ATB, > Ramsay Jones > >
Re: Confusion about the PACK format
On 10/02/2019 16:02, Florian Steenbuck wrote: > Hello to all, > > I try to understand the git protocol only on the server site. So I > start without reading any docs and which turns to be fine until I got > to the PACK format (pretty early failure I know). > > I have read this documentation: > https://raw.githubusercontent.com/git/git/c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1/Documentation/technical/pack-format.txt > > But their are some confusion about this text. > > The basic header is no problem, but somehow I got stuck while try to > read the length and type of the objects, which are ints that can be > resolved with 3-bits and 4-bits. The question is where and how ? > Hmm, the 'type and length' encoding could be described more clearly! Hopefully, just on this issue, the following could help: In my git.git repo, which is fully packed, I have a single pack file, with $ git count-objects -v count: 0 size: 0 in-pack: 270277 packs: 1 size-pack: 101929 prune-packable: 0 garbage: 0 size-garbage: 0 $ ... 270277 objects in it. The beginning of the file looks like: $ xxd .git/objects/pack/pack-d554e6d8335601c2525b40487faf36493094ab50.pack | head : 5041 434b 0002 0004 1fc5 9d13 789c PACK..x. 0010: 9d8f cd6a c330 1084 ef7a 8a3d 171a b4ab ...j.0...z.= 0020: 9525 8750 0abd 945c f304 ab95 5cfb 602b .%.P...\\.`+ 0030: b84a 7fde 3e2a 943e 406f c3f0 cd30 d3f6 .J..>*.>@o...0.. 0040: 5260 741a 5025 92e2 1458 917c c294 a3c3 R`t.P%...X.| 0050: 4803 e521 395f c2d8 4d73 95bd 6c0d 82f5 H..!9_..Ms..l... 0060: 6172 310f 0529 7a2f d6a7 40c5 d9a0 d185 ar1..)z/..@. 0070: 622d 8789 9cb8 3f1e 5132 6366 4de4 8531 b-?.Q2cfM..1 0080: 114a 70ec 9447 2f5a 526f e29c 3847 23b7 .Jp..G/ZRo..8G#. 0090: 36d7 1dce b76d a9f0 02af b2ca 56e1 f4b6 6m..V... $ You can see the header, which consists of 3 32-bit values, where the packfile signature is the '5041 434b', then the version number which is ' 0002', followed by the number of objects '0004 1fc5' which is 270277. Next comes the first 'object entry', which starts '9d13'. Now, the 'n-byte type and length' is a variable length encoding of the object type and length. The number of bytes used to encode this data is content dependant. If the top bit of a byte is set, then we need to process the next byte, otherwise we are done. So, looking at the first 'object entry' byte (at offset 12) '9d', we take the top nibble, remove the top bit, and shift right 4 bits to get the object type. ie. (0x9d >> 4) & 7 which gives an object type of 1 (which is a commit object). The lower nibble of the first byte contains the first (or only) 4 bits of the size, here (0x9d & 15) which is 0xd. Given that the top bit of this byte is set, we now process the next byte. After the first byte, each byte contains 7 bits of the size field which is combined with the value from the previous byte by shifting and adding (first by 4 bits, then 11, 18, 25 etc.). So, in this case we have (0x13 << 4) + 0xd = 317. The compressed data follows, '789c' ... We can use git-verify-pack to confirm the details here: $ git verify-pack -v .git/objects/pack/pack-d554e6d8335601c2525b40487faf36493094ab50.idx | head -n 1 878e2cd30e1656909c5073043d32fe9d02204daa commit 317 216 12 $ So the object 878e2cd30e, at offset 12 in the file, is a commit object with size 317 (which has an in-pack size of 216). Hope this helps. ATB, Ramsay Jones
[PATCH v2 2/2] Makefile: improve SPARSE_FLAGS customisation
In order to enable greater user customisation of the SPARSE_FLAGS variable, we introduce a new SP_EXTRA_FLAGS variable to use for target specific settings. Without using the new variable, setting the SPARSE_FLAGS on the 'make' command-line would also override the value set by the target-specific rules in the Makefile (effectively making them useless). Also, this enables the SP_EXTRA_FLAGS to be used in the future for any other internal customisations, such as for some platform specific values. In addition, we initialise the SPARSE_FLAGS to the default (empty) value using a conditional assignment (?=). This allows SPARSE_FLAGS to be set from the environment as well as from the command-line. Signed-off-by: Ramsay Jones --- Makefile | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 6e8d017e8e..fcb7575e1b 100644 --- a/Makefile +++ b/Makefile @@ -574,7 +574,11 @@ SPATCH = spatch export TCL_PATH TCLTK_PATH -SPARSE_FLAGS = +# user customisation variable for 'sparse' target +SPARSE_FLAGS ?= +# internal/platform customisation variable for 'sparse' +SP_EXTRA_FLAGS = + SPATCH_FLAGS = --all-includes --patch . @@ -2369,10 +2373,10 @@ gettext.sp gettext.s gettext.o: GIT-PREFIX gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \ -DGIT_LOCALE_PATH='"$(localedir_relative_SQ)"' -http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \ +http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \ -DCURL_DISABLE_TYPECHECK -pack-revindex.sp: SPARSE_FLAGS += -Wno-memcpy-max-count +pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count ifdef NO_EXPAT http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT @@ -2386,7 +2390,7 @@ endif ifdef USE_NED_ALLOCATOR compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \ -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR -compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null +compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null endif git-%$X: %.o GIT-LDFLAGS $(GITLIBS) @@ -2710,7 +2714,7 @@ SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ)) $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ - $(SPARSE_FLAGS) $< + $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ) -- 2.20.0
[PATCH v2 1/2] config.mak.uname: remove obsolete SPARSE_FLAGS setting
An upcoming commit will change the semantics of the SPARSE_FLAGS variable from an internal to a user only customisation variable. The MinGW configuration section contains an obsolete setting for this variable which was used (some years ago) to cater to an error in the Win32 system header files. Since 'sparse' does not currently support the MinGW platform, nobody on that platform can be relying on this setting today. Remove this use of the SPARSE_FLAGS variable. Signed-off-by: Ramsay Jones --- config.mak.uname | 1 - 1 file changed, 1 deletion(-) diff --git a/config.mak.uname b/config.mak.uname index 7b36a1dfe7..786bb2f913 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -555,7 +555,6 @@ ifneq (,$(findstring MINGW,$(uname_S))) RC = windres -O coff NATIVE_CRLF = YesPlease X = .exe - SPARSE_FLAGS = -Wno-one-bit-signed-bitfield ifneq (,$(wildcard ../THIS_IS_MSYSGIT)) htmldir = doc/git/html/ prefix = -- 2.20.0
[PATCH v2 0/2] Using sparse in a CI job
as not really intended to be used that way. This will cause problems for those files which have target specific settings for SPARSE_FLAGS. For example: $ make pack-revindex.sp SP pack-revindex.c $ make SPARSE_FLAGS=-Wsparse-error pack-revindex.sp SP pack-revindex.c pack-revindex.c:65:23: error: memset with byte count of 262144 Makefile:2729: recipe for target 'pack-revindex.sp' failed make: *** [pack-revindex.sp] Error 1 $ echo $? 2 $ So, passing SPARSE_FLAGS on the command-line, effectively nullifies the target specific settings (making them useless). This patch splits the duties of SPARSE_FLAGS, by introducing a separate SP_EXTRA_FLAGS variable, for use with the target specific settings. In addition, we use a conditional assignment (?=) of the default (empty) value of SPARSE_FLAGS, to allow setting the value of this variable from the environment as well as the command-line. So, after this patch: $ make SPARSE_FLAGS=-Wsparse-error pack-revindex.sp SP pack-revindex.c $ echo $? 0 $ $ SPARSE_FLAGS=-Wsparse-error make pack-revindex.sp SP pack-revindex.c $ echo $? 0 $ Now, we should be able to run the sparse Makefile target in a CI job, and still find all sparse errors and warnings (now marked as errors also), using something like this: $ SPARSE_FLAGS=-Wsparse-error make -k sparse >sp-out 2>&1 Note that the '-k' argument to 'make' is now required. ;-) ATB, Ramsay Jones Ramsay Jones (2): config.mak.uname: remove obsolete SPARSE_FLAGS setting Makefile: improve SPARSE_FLAGS customisation Makefile | 14 +- config.mak.uname | 1 - 2 files changed, 9 insertions(+), 6 deletions(-) Range-diff against v1: -: -- > 1: 7b15bd9b31 config.mak.uname: remove obsolete SPARSE_FLAGS setting 1: 889cc64f90 ! 2: c2b6ce71da Makefile: improve SPARSE_FLAGS customisation @@ -7,10 +7,13 @@ target specific settings. Without using the new variable, setting the SPARSE_FLAGS on the 'make' command-line would also override the value set by the target-specific rules in the Makefile (effectively -making them useless). In addition, we initialise the SPARSE_FLAGS -to the default (empty) value using a conditional assignment (?=). -This allows the SPARSE_FLAGS to be set from the environment as -well as from the command-line. +making them useless). Also, this enables the SP_EXTRA_FLAGS to be +used in the future for any other internal customisations, such as +for some platform specific values. + +In addition, we initialise the SPARSE_FLAGS to the default (empty) +value using a conditional assignment (?=). This allows SPARSE_FLAGS +to be set from the environment as well as from the command-line. diff --git a/Makefile b/Makefile --- a/Makefile @@ -20,7 +23,11 @@ export TCL_PATH TCLTK_PATH -SPARSE_FLAGS = ++# user customisation variable for 'sparse' target +SPARSE_FLAGS ?= ++# internal/platform customisation variable for 'sparse' ++SP_EXTRA_FLAGS = ++ SPATCH_FLAGS = --all-includes --patch . -- 2.20.0
Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation
On 04/02/2019 20:15, Junio C Hamano wrote: > Ramsay Jones writes: > >> On 04/02/2019 18:12, Junio C Hamano wrote: >>> Ramsay Jones writes: >>> >>>>> Thanks for a detailed and clear explanation here and in the cover >>>>> letter. I agree with the motivation and most of the things I see in >>>>> this patch, but one thing that stands out at me is if we still want >>>>> to += append to SP_EXTRA_FLAGS in target specific way. Before this >>>>> patch, because SPARSE_FLAGS was a dual use variable, it needed += >>>>> appending to it in these two places, but that rationale is gone with >>>>> this patch. >>>> >>>> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS >>>> should be used for any 'internal' settings (not just the target >>>> specific settings), whereas SPARSE_FLAGS would now be used _only_ for >>>> user customisation. >>> >>> OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS >> >> Err, no, that clearly wouldn't be an improvement! As I said above, >> this is not just for target specific settings. > > Ah, do you mean that there may be globally applicable internal > setting? I would have expected that such an option would be done > directly on the command line, e.g. > > $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE > $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ > $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) \ > -Wsparse-settings-for-everybody $< global, possibly, but more likely platform variations - as I tried (but obviously failed) to indicate with the cygwin and MinGW examples in my previous email. ATB, Ramsay Jones
Re: [PATCH] trace2: fix hdr-check warnings
On 30/01/2019 12:29, Jeff Hostetler wrote: > > > On 1/26/2019 4:07 PM, Ramsay Jones wrote: >> >> Signed-off-by: Ramsay Jones >> --- >> >> Hi Jeff, >> >> If you need to re-roll your 'jh/trace2' branch, could you please >> squash this into the relevant patches (sorry, I didn't look to >> see which patches need to be modified). > > Will do. Thanks. > > BTW, how do you find these? I ran both "make sparse" and > "make DEVELOPER=1" and it didn't complain about these items. Carlo already replied about 'make hdr-check', but you seem to have missed squashing half of the original patch, since the re-rolled series still causes 'make -k hdr-check >phcout 2>&1' to show: $ diff nhcout phcout 22a23,34 > HDR trace2/tr2_dst.h > HDR trace2/tr2_cfg.h > HDR trace2/tr2_tgt.h > HDR trace2/tr2_cmd_name.h > HDR trace2/tr2_sid.h > HDR trace2/tr2_tls.h > trace2/tr2_tls.h:12:16: error: field ‘thread_name’ has incomplete type > struct strbuf thread_name; > ^~~ > Makefile:2739: recipe for target 'trace2/tr2_tls.hco' failed > make: *** [trace2/tr2_tls.hco] Error 1 > HDR trace2/tr2_tbuf.h 131c143 < Makefile:2725: recipe for target 'sha256/gcrypt.hco' failed --- > Makefile:2739: recipe for target 'sha256/gcrypt.hco' failed 164a177 > HDR trace2.h $ So, quoting the last part of the original patch: diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h index 99ea9018ce..bb80e3f8e7 100644 --- a/trace2/tr2_tls.h +++ b/trace2/tr2_tls.h @@ -1,6 +1,8 @@ #ifndef TR2_TLS_H #define TR2_TLS_H +#include "strbuf.h" + /* * Arbitry limit for thread names for column alignment. */ -- ATB, Ramsay Jones
Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation
On 04/02/2019 18:12, Junio C Hamano wrote: > Ramsay Jones writes: > >>> Thanks for a detailed and clear explanation here and in the cover >>> letter. I agree with the motivation and most of the things I see in >>> this patch, but one thing that stands out at me is if we still want >>> to += append to SP_EXTRA_FLAGS in target specific way. Before this >>> patch, because SPARSE_FLAGS was a dual use variable, it needed += >>> appending to it in these two places, but that rationale is gone with >>> this patch. >> >> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS >> should be used for any 'internal' settings (not just the target >> specific settings), whereas SPARSE_FLAGS would now be used _only_ for >> user customisation. > > OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS Err, no, that clearly wouldn't be an improvement! As I said above, this is not just for target specific settings. Am I missing something? ATB, Ramsay Jones
Re: [PATCH 0/1] Using sparse in a CI job
On 03/02/2019 12:12, SZEDER Gábor wrote: > On Sun, Feb 03, 2019 at 01:49:37AM +0000, Ramsay Jones wrote: >> On 02/02/2019 00:41, SZEDER Gábor wrote: >>> On Fri, Feb 01, 2019 at 09:01:20PM +, Ramsay Jones wrote: >>>> At the moment, on Linux, the sp-out file is free from any sparse errors >>>> or warnings. So are next and pu: >>>> >>>> $ grep error sp-out >>>> $ grep warning sp-out >>> >>> On 'master' I get: >>> >>> $ grep error sp-out >>> $ grep warning sp-out >>> connect.c:652:40: warning: incorrect type in argument 2 (invalid types) >>> pack-revindex.c:65:23: warning: memset with byte count of 262144 >>> unix-socket.c:83:26: warning: incorrect type in argument 2 (invalid types) >>> unix-socket.c:108:23: warning: incorrect type in argument 2 (invalid >>> types) >>> daemon.c:1041:36: warning: incorrect type in argument 2 (invalid types) >>> daemon.c:1184:67: warning: incorrect type in argument 2 (invalid types) >>> imap-send.c:1022:42: warning: incorrect type in argument 2 (invalid types) >>> credential-cache--daemon.c:180:37: warning: incorrect type in argument 2 >>> (invalid types) >>> $ sparse --version >>> v0.5.0 >> >> Yeah, that version of sparse is a bit too old. >> >> If memory serves (it may not), all of the 'argument 2 (invalid types)' >> errors are caused by the glibc headers using a 'transparent union' to >> define the 'struct sockaddr' type. sparse could not handle that until >> commit 7698bae699 (aka. v0.5.0-5-g7698bae). The only remaining warning >> was addressed by commit bcfe020ed9 (aka. v0.5.1-rc1-22-gbcfe020) in >> sparse and commit 54360a1956 in git. >> >> So, it seems you need at least v0.5.2 of sparse on your Linux system >> (which can't be too recent, or you would need v0.6.0). > > The latest Linux image available on Travis CI is based on Ubuntu 16.04 > LTS, which contains sparse 0.5.0, while their default image is based > on 14.04, with sparse 0.4.5-rc1. Even the latest Ubuntu LTS is only > at 0.5.1. > > https://travis-ci.org/szeder/git/jobs/488113660#L487 > https://travis-ci.org/szeder/git/jobs/488113661#L208 > Indeed. [This is why I build sparse from source! :-D ] With a bit of luck, v0.6.1 will be available soon. ATB, Ramsay Jones
Re: [PATCH 0/1] Using sparse in a CI job
On 01/02/2019 22:40, Luc Van Oostenryck wrote: > On Fri, Feb 01, 2019 at 09:01:20PM +0000, Ramsay Jones wrote: >> >> I suspect that the Makefile sparse target is not easy to use in a CI >> job, since the 'sparse' program (via cgcc -no-compile) does not exit >> with a non-zero value, even when issuing errors and warnings. > > ... > >> We can change that by passing '-Wsparse-error' to 'sparse': >> >> $ make SPARSE_FLAGS=-Wsparse-error change-table.sp >> SP change-table.c >> change-table.h:53:24: error: dubious one-bit signed bitfield >> change-table.h:54:25: error: dubious one-bit signed bitfield >> change-table.h:55:25: error: dubious one-bit signed bitfield >> change-table.h:56:26: error: dubious one-bit signed bitfield >> Makefile:2729: recipe for target 'change-table.sp' failed >> make: *** [change-table.sp] Error 1 >> $ echo $? >> 2 >> $ >> >> Note that '-Wsparse-error' not only returns a non-zero exit code (1), but >> it also changes a 'warning' into an 'error' (see above): > > Yes, I know :( > The fact that, by default, sparse doesn't fail on errors is wanted > (otherwise it would break the kernel compile). But that the only way > to return an error is to use -Wsparse-error (which is supposed to > replace GCC's -Werror) is a real problem. Given that I only use sparse as a checker, I actually don't mind the current behaviour. That would be different if I was using sparsec/sparsei etc., as a compiler, of course! ;-) [If I were to suggest any change at all to -Wsparse-error it would be: do not change the 'warning' to an 'error' (yes, the actual text label of the message), exit(1) if any errors or warnings, but *if* only warnings have been issued, then print an "treating warnings as errors [-Wsparse-error]" message.] ATB, Ramsay Jones
Re: [PATCH 0/1] Using sparse in a CI job
On 02/02/2019 00:41, SZEDER Gábor wrote: > On Fri, Feb 01, 2019 at 09:01:20PM +0000, Ramsay Jones wrote: [snip] >> At the moment, on Linux, the sp-out file is free from any sparse errors >> or warnings. So are next and pu: >> >> $ grep error sp-out >> $ grep warning sp-out > > On 'master' I get: > > $ grep error sp-out > $ grep warning sp-out > connect.c:652:40: warning: incorrect type in argument 2 (invalid types) > pack-revindex.c:65:23: warning: memset with byte count of 262144 > unix-socket.c:83:26: warning: incorrect type in argument 2 (invalid types) > unix-socket.c:108:23: warning: incorrect type in argument 2 (invalid types) > daemon.c:1041:36: warning: incorrect type in argument 2 (invalid types) > daemon.c:1184:67: warning: incorrect type in argument 2 (invalid types) > imap-send.c:1022:42: warning: incorrect type in argument 2 (invalid types) > credential-cache--daemon.c:180:37: warning: incorrect type in argument 2 > (invalid types) > $ sparse --version > v0.5.0 Yeah, that version of sparse is a bit too old. If memory serves (it may not), all of the 'argument 2 (invalid types)' errors are caused by the glibc headers using a 'transparent union' to define the 'struct sockaddr' type. sparse could not handle that until commit 7698bae699 (aka. v0.5.0-5-g7698bae). The only remaining warning was addressed by commit bcfe020ed9 (aka. v0.5.1-rc1-22-gbcfe020) in sparse and commit 54360a1956 in git. So, it seems you need at least v0.5.2 of sparse on your Linux system (which can't be too recent, or you would need v0.6.0). ATB, Ramsay Jones
Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation
On 01/02/2019 21:46, Junio C Hamano wrote:
> Ramsay Jones writes:
>
>> In order to enable greater user customisation of the SPARSE_FLAGS
>> variable, we introduce a new SP_EXTRA_FLAGS variable to use for
>> target specific settings. Without using the new variable, setting
>> the SPARSE_FLAGS on the 'make' command-line would also override the
>> value set by the target-specific rules in the Makefile (effectively
>> making them useless). In addition, we initialise the SPARSE_FLAGS
>> to the default (empty) value using a conditional assignment (?=).
>> This allows the SPARSE_FLAGS to be set from the environment as
>> well as from the command-line.
>
> Thanks for a detailed and clear explanation here and in the cover
> letter. I agree with the motivation and most of the things I see in
> this patch, but one thing that stands out at me is if we still want
> to += append to SP_EXTRA_FLAGS in target specific way. Before this
> patch, because SPARSE_FLAGS was a dual use variable, it needed +=
> appending to it in these two places, but that rationale is gone with
> this patch.
As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS
should be used for any 'internal' settings (not just the target
specific settings), whereas SPARSE_FLAGS would now be used _only_ for
user customisation.
The commit message doesn't make that clear, (and the patch text adds
to the confusion, since only target specific settings are changed) so
I need to reword that somehow. Also, ...
> Also, don't we want to clear SP_EXTRA_FLAGS at the beginning?
... (Ahem) I just simply forgot to initialise the new variable! :(
(Yes, it actually doesn't matter, but it gives a wrong impression). ;-)
BTW, the first name I chose was SP_FLAGS, but while editing the second
hunk I decided that wasn't a good name. On several other projects I have
seen exactly this 'split' happen, where the user facing variable was
called _FLAGS and the 'internal' variable was then called
_EXTRA_FLAGS, so I decided to go with that instead. (Yes, I
abbreviated SPARSE). However, I have to say that I have also seen (less
often) the exact opposite: "... if some idiot user wants to add extra
flags ...". :-D
So, yes SP_EXTRA_FLAGS could be used for other 'internal' uses; for
example, look back to commit 6bc8606be3 ("config.mak.uname: remove
SPARSE_FLAGS setting for cygwin", 2018-02-12), which removed:
'SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield'
from config.mak.uname. As you can see, although gcc could find the
win32 header files, sparse needed a little help. Also, the win32 system
header files had an instance of a 'one-bit signed bitfield', which caused
sparse to spew many many many errors. If I needed to do something like
that again, then I would use SP_EXTRA_FLAGS instead.
[Looking back now, I am a little shocked that it seems to have taken
me nearly 5 years to submit that patch! :-P ]
I could give quite a few examples, but ... Oh wait! ... Hmm, it seems
that I need to add a new patch to remove line 558 of config.mak.uname.
This line has a setting for SPARSE_FLAGS in the MINGW section of that
file. Back in around 2011, having ported sparse to MinGW (the original
msysgit, not MSYS2), I naturally had the same issue with the Win32
header files. Since I didn't upstream my sparse patches, I don't think
anyone can be running sparse on MinGW these days.
Anyway, its late, so I will look at redoing the patches soon.
ATB,
Ramsay Jones
[PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation
In order to enable greater user customisation of the SPARSE_FLAGS variable, we introduce a new SP_EXTRA_FLAGS variable to use for target specific settings. Without using the new variable, setting the SPARSE_FLAGS on the 'make' command-line would also override the value set by the target-specific rules in the Makefile (effectively making them useless). In addition, we initialise the SPARSE_FLAGS to the default (empty) value using a conditional assignment (?=). This allows the SPARSE_FLAGS to be set from the environment as well as from the command-line. Signed-off-by: Ramsay Jones --- Makefile | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 6e8d017e8e..dc02825c88 100644 --- a/Makefile +++ b/Makefile @@ -574,7 +574,7 @@ SPATCH = spatch export TCL_PATH TCLTK_PATH -SPARSE_FLAGS = +SPARSE_FLAGS ?= SPATCH_FLAGS = --all-includes --patch . @@ -2369,10 +2369,10 @@ gettext.sp gettext.s gettext.o: GIT-PREFIX gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \ -DGIT_LOCALE_PATH='"$(localedir_relative_SQ)"' -http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \ +http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \ -DCURL_DISABLE_TYPECHECK -pack-revindex.sp: SPARSE_FLAGS += -Wno-memcpy-max-count +pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count ifdef NO_EXPAT http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT @@ -2386,7 +2386,7 @@ endif ifdef USE_NED_ALLOCATOR compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \ -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR -compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null +compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null endif git-%$X: %.o GIT-LDFLAGS $(GITLIBS) @@ -2710,7 +2710,7 @@ SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ)) $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ - $(SPARSE_FLAGS) $< + $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ) -- 2.20.0
[PATCH 0/1] Using sparse in a CI job
nt of 262144 Makefile:2729: recipe for target 'pack-revindex.sp' failed make: *** [pack-revindex.sp] Error 1 $ echo $? 2 $ So, passing SPARSE_FLAGS on the command-line, effectively nullifies the target specific settings (making them useless). This patch splits the duties of SPARSE_FLAGS, by introducing a separate SP_EXTRA_FLAGS variable, for use with the target specific settings. In addition, we use a conditional assignment (?=) of the default (empty) value of SPARSE_FLAGS, to allow setting the value of this variable from the environment as well as the command-line. So, after this patch: $ make SPARSE_FLAGS=-Wsparse-error pack-revindex.sp SP pack-revindex.c $ echo $? 0 $ $ SPARSE_FLAGS=-Wsparse-error make pack-revindex.sp SP pack-revindex.c $ echo $? 0 $ Now, we should be able to run the sparse Makefile target in a CI job, and still find all sparse errors and warnings (now marked as errors also), using something like this: $ SPARSE_FLAGS=-Wsparse-error make -k sparse >sp-out 2>&1 Note that the '-k' argument to 'make' is now required. ;-) ATB, Ramsay Jones Ramsay Jones (1): Makefile: improve SPARSE_FLAGS customisation Makefile | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) -- 2.20.0
Re: [PATCH v4 5/8] evolve: add the change-table structure
On 01/02/2019 03:09, sxe...@google.com wrote:
> From: Stefan Xenos
>
> A change table stores a list of changes, and supports efficient lookup
> from a commit hash to the list of changes that reference that commit
> directly.
>
> It can be used to look up content commits or metacommits at the head
> of a change, but does not support lookup of commits referenced as part
> of the commit history.
>
> Signed-off-by: Stefan Xenos
> ---
> Makefile | 1 +
> change-table.c | 176 +
> change-table.h | 127 +++
> 3 files changed, 304 insertions(+)
> create mode 100644 change-table.c
> create mode 100644 change-table.h
>
[snip]
> diff --git a/change-table.h b/change-table.h
> new file mode 100644
> index 00..023bca37d1
> --- /dev/null
> +++ b/change-table.h
> @@ -0,0 +1,127 @@
> +#ifndef CHANGE_TABLE_H
> +#define CHANGE_TABLE_H
> +
> +#include "oidmap.h"
> +
> +struct commit;
> +struct ref_filter;
> +
> +/*
> + * This struct holds a list of change refs. The first element is stored
> inline,
> + * to optimize for small lists.
> + */
> +struct change_list {
> + /* Ref name for the first change in the list, or null if none.
> + *
> + * This field is private. Use for_each_change_in to read.
> + */
> + const char* first_refname;
> + /* List of additional change refs. Note that this is empty if the list
> + * contains 0 or 1 elements.
> + *
> + * This field is private. Use for_each_change_in to read.
> + */
> + struct string_list additional_refnames;
> +};
> +
> +/*
> + * Holds information about the head of a single change.
> + */
> +struct change_head {
> + /*
> + * The location pointed to by the head of the change. May be a commit
> or a
> + * metacommit.
> + */
> + struct object_id head;
> + /*
> + * The content commit for the latest commit in the change. Always
> points to a
> + * real commit, never a metacommit.
> + */
> + struct object_id content;
> + /*
> + * Abandoned: indicates that the content commit should be removed from
> the
> + * history.
> + *
> + * Hidden: indicates that the change is an inactive change from the
> + * hiddenmetas namespace. Such changes will be hidden from the user by
> + * default.
> + *
> + * Deleted: indicates that the change has been removed from the
> repository.
> + * That is the ref was deleted since the time this struct was created.
> Such
> + * entries should be ignored.
> + */
> + int abandoned:1,
> + hidden:1,
> + remote:1,
> + deleted:1;
This causes sparse to issue errors about 'dubious one-bit signed
bitfield' for each of these fields (and for each file which #includes
this header file).
The field type should be 'unsigned int', thus:
-- >8 --
diff --git a/change-table.h b/change-table.h
index 85bb19c3bf..1c385e076e 100644
--- a/change-table.h
+++ b/change-table.h
@@ -50,10 +50,10 @@ struct change_head {
* That is the ref was deleted since the time this struct was created.
Such
* entries should be ignored.
*/
- int abandoned:1,
- hidden:1,
- remote:1,
- deleted:1;
+ unsigned int abandoned:1,
+ hidden:1,
+ remote:1,
+ deleted:1;
};
/*
-- >8 --
[Note: this diff was against the v3 series].
ATB,
Ramsay Jones
Re: sparse job, was Re: [PATCH] test-xml-encode: fix sparse NULL pointer warnings
On 29/01/2019 01:52, Luc Van Oostenryck wrote: > On Mon, Jan 28, 2019 at 08:13:03PM +0000, Ramsay Jones wrote: > > Hi > >> The dependencies for the 'sparse' package includes: libc6 (>= 2.14), >> libllvm4.0 (>= 1:4.0~), libxml2 (>= 2.7.4), perl:any >> >> However, for git, we only need to build 'cgcc' and 'sparse', which >> means we can forget about libxml (only used for c2xml), and libllvm >> (only used for sparse-llvm/sparsec/sparsei). Also, I'm not sure what >> perl is doing there ... > > perl is only used as the interpreter of cgcc. heh, yeah (palm-face), I was only thinking about _build_ dependency! :-D ATB, Ramsay Jones
Re: sparse job, was Re: [PATCH] test-xml-encode: fix sparse NULL pointer warnings
On 28/01/2019 22:34, Johannes Schindelin wrote: > Hi Ramsay, > > On Mon, 28 Jan 2019, Ramsay Jones wrote: > >> Hmm, I've never built an Ubuntu package before, so I don't know >> exactly what would be required (spec file etc.) to create a PPA. >> But I suspect you are not talking about doing that, right? > > I would have gone for `checkinstall`... That still works, right? Ah, I never think about using checkinstall - I haven't really used it in anger. For some reason, I thought you needed to structure your Makefile a certain way (using $DESTDIR or somesuch), but I seem to be confusing it with something else. Apparently, no change to the Makefile is required - it uses some kind of filesystem watcher to note which files are copied into place by 'make install'. heh, go figure! :-D So, I think you only need to set the PREFIX when building (the default installation PREFIX is $HOME), or create a local.mk file to configure the build (I don't do that). The 'sparse' build does not make use of any 'auto-tools', so no configure script. Ah, I think you will need to have pkg-config installed. I have never built sparse from a tar-ball - I assume it works! ;-) So (just typing into my email client - not tested): $ wget http://www.kernel.org/pub/software/devel/sparse/dist/sparse-0.6.0.tar.gz $ tar xvf sparse-0.6.0.tar.gz $ cd sparse-0.6.0 $ make PREFIX=/usr/local $ sudo checkinstall make PREFIX=/usr/local install ... should do it. (famous last words). ATB, Ramsay Jones
Re: sparse job, was Re: [PATCH] test-xml-encode: fix sparse NULL pointer warnings
On 28/01/2019 16:10, Johannes Schindelin wrote:
> Hi Ramsay,
>
> On Sat, 26 Jan 2019, Ramsay Jones wrote:
>
>>
>> Signed-off-by: Ramsay Jones
>> ---
>>
>> Hi Johannes,
>>
>> If you need to re-roll your 'js/vsts-ci' branch, could you please
>> squash this into the relevant patch (commit af7747e7c7 ("tests: optionally
>> write results as JUnit-style .xml", 2019-01-23)).
>
> Certainly!
>
> BTW would you be interested in working with me on an Azure Pipeline that
> runs `sparse` on all of Junio's branches? (I am now pretty proficient with
> building a software package in one Azure Pipeline, publishing it as a
> build artifact, then consuming it from another Azure Pipeline, so I would
> build the `sparse` package as an Ubuntu package and offer it as a build
> artifact.)
Happy to help, if I can.
Looking at the sparse wiki [1] we can see that the most recent
release of sparse is v0.6.0, released on December 26th 2018
(just too late for crimble!). This is the release you will
need to use for more recent Linux distros (eg. fedora 27+,
Ubuntu 18.04, 18.10, etc).
The releases are available from [2] as a compressed tar-ball
using '.gz' or .'xz' compression. eg. sparse-0.6.0.tar.gz
(there is also a sparse-0.6.0.tar.sign).
[The git repo is at [3], BTW].
[I was a little surprised that Linux Mint 19.1 (based on Ubuntu
18.04) has v0.5.1 - Debian unstable has v0.5.2, but both of those
are just a little too old for use with git on recent Linux.]
It seems Ubuntu has split sparse into two packages, the main
'sparse' package, which contains the command-line programs and
a 'sparse-test-inspect' package, which contains the 'test-inspect'
GUI program (and so depends on GTK).
The dependencies for the 'sparse' package includes: libc6 (>= 2.14),
libllvm4.0 (>= 1:4.0~), libxml2 (>= 2.7.4), perl:any
However, for git, we only need to build 'cgcc' and 'sparse', which
means we can forget about libxml (only used for c2xml), and libllvm
(only used for sparse-llvm/sparsec/sparsei). Also, I'm not sure what
perl is doing there ...
Note that these dependencies (along with the dependent programs) are
optional and I can happily build sparse without them:
$ make clean
Makefile:124: Your system does not have libxml, disabling c2xml
Makefile:146: Your system does not have gtk3/gtk2, disabling test-inspect
Makefile:179: Your system does not have llvm, disabling sparse-llvm
CLEAN
$
Hmm, I've never built an Ubuntu package before, so I don't know
exactly what would be required (spec file etc.) to create a PPA.
But I suspect you are not talking about doing that, right?
ATB,
Ramsay Jones
[1] https://sparse.wiki.kernel.org/index.php/Main_Page
[2] http://www.kernel.org/pub/software/devel/sparse/dist/
[3] git://git.kernel.org/pub/scm/devel/sparse/sparse.git
[PATCH] trace2: fix hdr-check warnings
Signed-off-by: Ramsay Jones --- Hi Jeff, If you need to re-roll your 'jh/trace2' branch, could you please squash this into the relevant patches (sorry, I didn't look to see which patches need to be modified). Thanks! ATB, Ramsay Jones trace2/tr2_tgt.h | 4 trace2/tr2_tls.h | 2 ++ 2 files changed, 6 insertions(+) diff --git a/trace2/tr2_tgt.h b/trace2/tr2_tgt.h index 4fdf253b57..8a46bbad4e 100644 --- a/trace2/tr2_tgt.h +++ b/trace2/tr2_tgt.h @@ -1,6 +1,10 @@ #ifndef TR2_TGT_H #define TR2_TGT_H +struct child_process; +struct repository; +struct json_writer; + /* * Function prototypes for a TRACE2 "target" vtable. */ diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h index 99ea9018ce..bb80e3f8e7 100644 --- a/trace2/tr2_tls.h +++ b/trace2/tr2_tls.h @@ -1,6 +1,8 @@ #ifndef TR2_TLS_H #define TR2_TLS_H +#include "strbuf.h" + /* * Arbitry limit for thread names for column alignment. */ -- 2.20.0
[PATCH] test-xml-encode: fix sparse NULL pointer warnings
Signed-off-by: Ramsay Jones
---
Hi Johannes,
If you need to re-roll your 'js/vsts-ci' branch, could you please
squash this into the relevant patch (commit af7747e7c7 ("tests: optionally
write results as JUnit-style .xml", 2019-01-23)).
Thanks!
ATB,
Ramsay Jones
t/helper/test-xml-encode.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/helper/test-xml-encode.c b/t/helper/test-xml-encode.c
index 367c4875e6..a648bbd961 100644
--- a/t/helper/test-xml-encode.c
+++ b/t/helper/test-xml-encode.c
@@ -26,7 +26,7 @@ int cmd__xml_encode(int argc, const char **argv)
if (tmp2) {
if ((ch & 0xc0) != 0x80) {
fputs(utf8_replace_character, stdout);
- tmp2 = 0;
+ tmp2 = NULL;
cur--;
continue;
}
@@ -34,7 +34,7 @@ int cmd__xml_encode(int argc, const char **argv)
tmp2++;
if (--remaining == 0) {
fwrite(tmp, tmp2 - tmp, 1, stdout);
- tmp2 = 0;
+ tmp2 = NULL;
}
continue;
}
--
2.20.0
Re: Git Test Coverage Report (Sat Jan 19)
On 24/01/2019 19:18, Derrick Stolee wrote: > On 1/24/2019 1:15 PM, Junio C Hamano wrote: >> Derrick Stolee writes: >> >>> Here is today's test coverage report. >>> >>> Also, there has been some feedback that it can be hard to manually >>> match up uncovered lines with names at the bottom of the summary. The >>> suggestion was to auto-generate an HTML report that could be posted to >>> a public page and referenced in this mail for those who prefer >>> that. >> I wanted to "grep" for lines attributed to certain commits that >> appear in the list, by filtering lines that begin with enough number >> of hexdigits, except for those object names, but the attempt failed >> miserably because of the line wrapping (which probably comes from >> the assumption that it is OK because the "text/plain; format=flowed" >> would not care). If you can keep the long lines (due to the object >> names and line numbers prefixed to each line) unsplit, it would be >> more useful to locate and isolate lines. > This is likely more a problem with my workflow (pasting the report into > Thunderbird and sending) than with the content itself. Have you read Doucmentation/git-format-patch.txt (Thunderbird> Approach #2 (configuration) - approx. line 487)? ATB, Ramsay Jones
Re: [PATCH/RFC] fsck: complain when .gitignore and .gitattributes are symlinks
On 22/01/2019 07:23, Jeff King wrote:
> On Fri, Jan 18, 2019 at 01:41:08AM +0000, Ramsay Jones wrote:
>
>> I don't do this "from time to time", but *every* build on all
>> platforms! :-D
>>
>> As I have mentioned before, I run the script on 'master', 'next'
>> and 'pu', but I don't look at the results for 'master', I simply
>> look at the diffs master->next and next->pu.
>
> Ah, ok, that explains it, then. As you noted, these made it straight to
> master because of the security embargo.
>
> Thanks for satisfying my curiosity (and for running your script!).
>
> I do wonder if you might be better off comparing master@{1} to master to
> see if anything new appears (since I assume the whole point is ignoring
> historical false positives, and just looking at patches under active
> development).
Hmm, well it's not so much 'historical false positives' as 'oh dear,
they managed to get through' (along with a promise to myself to get
around to tidying up the symbols in master - yet again!). ;-)
I try to make people aware of the issues, when they appear in 'pu',
so that we have a chance not to make things worse. However, it is
never as simple as 'this symbol is not used/local to this file,
please fix' (despite what it looks like, I don't like to annoy
contributors with those emails :-D ). Many recent large changes
have been split into several series with earlier series introducing
symbols which 'will be used later'. Sometimes later never comes. ;-)
Recently, Brian's 'bc/sha-256' branch merged into 'next', so now:
$ diff sc nsc
37a38,39
> hex.o - hash_to_hex
> hex.o - hash_to_hex_algop_r
74a77,78
> sha1-file.o - hash_algo_by_id
> sha1-file.o - hash_algo_by_name
$
Brian has already indicated [1] that future patches will add uses
for these symbols.
[1]
https://public-inbox.org/git/20181114021118.gn890...@genre.crustytoothpaste.net/
[Just to be clear, my script only notes symbols that are not
referenced outside of the object file which contains its
definition - so that includes file-local and unused symbols].
There are currently 90 symbols in the 'sc' file, some of which
should be added to the outdated 'skip list'. Just FYI, the file
which has the most hits is:
$ cut -f1 sc | sort | uniq -c | sort -rn
26 config.o
6 sha1dc/sha1.o
6 refs.o
6 json-writer.o
3 utf8.o
3 sha1-file.o
3 revision.o
3 refs/ref-cache.o
2 vcs-svn/fast_export.o
2 refs/packed-backend.o
2 path.o
2 parse-options.o
2 graph.o
2 attr.o
1 worktree.o
1 trace.o
1 tmp-objdir.o
1 tempfile.o
1 strbuf.o
1 serve.o
1 sequencer.o
1 refspec.o
1 refs/iterator.o
1 read-cache.o
1 pkt-line.o
1 oidmap.o
1 line-log.o
1 ident.o
1 hex.o
1 gettext.o
1 fuzz-pack-idx.o
1 fuzz-pack-headers.o
1 editor.o
1 credential.o
1 convert.o
1 builtin/pack-objects.o
$
... and the symbols in that file:
$ grep config.o sc
config.o - git_config_copy_section_in_file
config.o - git_config_from_file_with_options
config.o - git_config_from_parameters
config.o - git_config_get_bool_or_int
config.o - git_config_get_maybe_bool
config.o - git_config_get_pathname
config.o - git_config_include
config.o - git_config_key_is_valid
config.o - git_configset_get_bool
config.o - git_configset_get_bool_or_int
config.o - git_configset_get_int
config.o - git_configset_get_maybe_bool
config.o - git_configset_get_pathname
config.o - git_configset_get_string
config.o - git_configset_get_string_const
config.o - git_configset_get_ulong
config.o - git_config_set_multivar_in_file
config.o - git_config_system
config.o - git_die_config_linenr
config.o - repo_config
config.o - repo_config_get_bool_or_int
config.o - repo_config_get_int
config.o - repo_config_get_maybe_bool
config.o - repo_config_get_pathname
config.o - repo_config_get_ulong
config.o - repo_config_get_value
$
ATB,
Ramsay Jones
Re: [PATCH/RFC] fsck: complain when .gitignore and .gitattributes are symlinks
On 17/01/2019 21:24, Jeff King wrote:
> On Thu, Jan 17, 2019 at 12:13:12PM -0800, Junio C Hamano wrote:
>
>>> @@ -966,7 +968,9 @@ int verify_path(const char *path, unsigned mode)
>>> if (is_hfs_dotgit(path))
>>> return 0;
>>> if (S_ISLNK(mode)) {
>>> - if (is_hfs_dotgitmodules(path))
>>> + if (is_hfs_dotgitmodules(path) ||
>>> + is_hfs_dotgitignore(path) ||
>>> + is_hfs_dotgitattributes(path))
>>> return 0;
>>> }
>>> }
>>> @@ -974,7 +978,9 @@ int verify_path(const char *path, unsigned mode)
>>> if (is_ntfs_dotgit(path))
>>> return 0;
>>> if (S_ISLNK(mode)) {
>>> - if (is_ntfs_dotgitmodules(path))
>>> + if (is_ntfs_dotgitmodules(path) ||
>>> + is_ntfs_dotgitignore(path) ||
>>> + is_ntfs_dotgitattributes(path))
>>> return 0;
>>
>> Curious that we already have these helpers, nobody seems to call
>> them in the current codebase, and we haven't seen the "these are
>> unused" linter message on the list for a while ;-).
>
> Heh. Yeah, I was surprised by that, too. They were added by e7cb0b4455
> (is_ntfs_dotgit: match other .git files, 2018-05-11). The original
> version of my series had the hunks quoted above, and then we backed off
> on handling them as part of the emergency fix, but I never re-rolled the
> preparatory patch to get rid of them.
>
> I think they got overlooked because they're not file-local statics, and
> it's much harder to say "this is never called by any function in another
> translation unit". You probably have to do analysis on the complete
> binaries using "nm" or similar. I think maybe Ramsay does that from time
> to time, but I don't offhand know the correct incantation.
I don't do this "from time to time", but *every* build on all
platforms! :-D
As I have mentioned before, I run the script on 'master', 'next'
and 'pu', but I don't look at the results for 'master', I simply
look at the diffs master->next and next->pu.
I put the output of 'static-check.pl' in the sc, nsc and psc files
(guess which files are for which branches!). For example, tonight
I find:
$ wc -l sc nsc psc
90 sc
90 nsc
100 psc
280 total
$ diff sc nsc
$ diff nsc psc
29a30,32
> config.o - repo_config_set
> config.o - repo_config_set_gently
> config.o - repo_config_set_worktree_gently
32a36
> fuzz-commit-graph.o - LLVMFuzzerTestOneInput
37a42,43
> hex.o - hash_to_hex
> hex.o - hash_to_hex_algop_r
74a81,83
> sha1-file.o - hash_algo_by_id
> sha1-file.o - hash_algo_by_name
> sha1-file.o - repo_has_sha1_file_with_flags
80a90
> strbuf.o - strbuf_vinsertf
$
BTW, if my memory serves (and it may not), the symbols you
refer to came directly into 'master' (via 'maint') as a
result of security updates - so I would never have seen
them in 'pu' or 'next'. They are, indeed, currently noted
in the 'master' branch:
$ grep is_ntfs_ sc
path.o - is_ntfs_dotgitattributes
path.o - is_ntfs_dotgitignore
$ grep is_hfs_ sc
utf8.o - is_hfs_dotgitattributes
utf8.o - is_hfs_dotgitignore
$
ATB,
Ramsay Jones
Re: [PATCH] repository.c: fix sparse warning
On 17/01/2019 10:06, Duy Nguyen wrote: > On Thu, Jan 17, 2019 at 8:21 AM Ramsay Jones > wrote: >> >> >> Signed-off-by: Ramsay Jones >> --- >> >> Hi Duy, >> >> If you need to re-roll your 'nd/the-index-final' branch, could you >> please squash this into the relevant patch (commit 4478671442, >> "cache.h: flip NO_THE_INDEX_COMPATIBILITY_MACROS switch", 2019-01-12). >> >> [the warning is caused by the lack of the extern declaration of the >> 'the_index' symbol.] > > Is it a false alarm? The variable is actually defined in this file now > which should also function as a declaration, yes? Ah, no, absolutely not! :( (er, well yes, but no! :-D ). I hope you agree that _all_ uses of a symbol should be within the scope of the same declaration of that symbol (by #include-ing the same header/interface file). This is _especially_ true of the file which has the definition of that symbol - how else do you expect the compiler to detect a mismatch between the declaration and definition? ATB, Ramsay Jones
[PATCH] repository.c: fix sparse warning
Signed-off-by: Ramsay Jones --- Hi Duy, If you need to re-roll your 'nd/the-index-final' branch, could you please squash this into the relevant patch (commit 4478671442, "cache.h: flip NO_THE_INDEX_COMPATIBILITY_MACROS switch", 2019-01-12). [the warning is caused by the lack of the extern declaration of the 'the_index' symbol.] Thanks! ATB, Ramsay Jones repository.c | 1 + 1 file changed, 1 insertion(+) diff --git a/repository.c b/repository.c index 0c6814627e..3ecbb1b6e3 100644 --- a/repository.c +++ b/repository.c @@ -1,3 +1,4 @@ +#define USE_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "repository.h" #include "object-store.h" -- 2.20.0
[PATCH] config.h: fix hdr-check warnings
commit 8f7c7f ("config.c: add repo_config_set_worktree_gently()",
2018-12-27) adds three function declarations that cause the hdr-check
make target to complain. The hdr-check complaint is caused by placing
the new declarations, which include 'struct repository *' parameters,
before the declaration of 'struct repository'. Move the struct
declaration to the top of the file.
Signed-off-by: Ramsay Jones
---
Hi Duy,
If you need to re-roll your 'nd/config-move-to' branch, could you please
squash this into the relevant patch (commit 8f7c7f).
BTW, none of the three new functions are called outside of config.c on
the 'pu' branch - I assume future patches will add some calls to these
functions (yes?).
Thanks!
ATB,
Ramsay Jones
config.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/config.h b/config.h
index 62204dc252..77c5b12873 100644
--- a/config.h
+++ b/config.h
@@ -5,6 +5,7 @@
#include "string-list.h"
struct object_id;
+struct repository;
/* git_config_parse_key() returns these negated: */
#define CONFIG_INVALID_KEY 1
@@ -215,7 +216,6 @@ extern int git_configset_get_maybe_bool(struct config_set
*cs, const char *key,
extern int git_configset_get_pathname(struct config_set *cs, const char *key,
const char **dest);
/* Functions for reading a repository's config */
-struct repository;
extern void repo_config(struct repository *repo, config_fn_t fn, void *data);
extern int repo_config_get_value(struct repository *repo,
const char *key, const char **value);
--
2.20.0
[PATCH] rebase-interactive.h: fix hdr-check warnings
Signed-off-by: Ramsay Jones
---
Hi Alban,
If you need to re-roll your 'ag/sequencer-reduce-rewriting-todo' branch,
could you please squash this into the relevant patch [commit c27b32f0ec4
("sequencer: refactor check_todo_list() to work on a todo_list",
2018-12-29)].
[Both commit e5b1c9d9299 and c27b32f0ec4 add function declarations
that cause the hdr-check target to complain about the lack of a
declaration for 'struct todo_list'. However, c27b32f0ec4 is earlier
in the branch ...].
Thanks!
ATB,
Ramsay Jones
rebase-interactive.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 42cc3f865d..44dbb06311 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -3,6 +3,7 @@
struct strbuf;
struct repository;
+struct todo_list;
void append_todo_help(unsigned keep_empty, int command_count,
const char *shortrevisions, const char *shortonto,
--
2.20.0
[PATCH] sequencer: mark file local symbols as static
Signed-off-by: Ramsay Jones
---
Hi Alban,
If you need to re-roll your 'ag/sequencer-reduce-rewriting-todo' branch,
could you please squash this into the relevant patch [commit 45f215c912
("rebase-interactive: use todo_list_write_to_file() in edit_todo_list()",
2018-12-29)].
I believe this commit removes the final calls to write_message() outside
of sequencer.c, so that this is now a file-local symbol.
Thanks!
[another patch for this branch is just coming up ...]
ATB,
Ramsay Jones
sequencer.c | 4 ++--
sequencer.h | 3 ---
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 60beeacdeb..64753af68e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -383,8 +383,8 @@ static void print_advice(struct repository *r, int
show_hint,
}
}
-int write_message(const void *buf, size_t len, const char *filename,
- int append_eol)
+static int write_message(const void *buf, size_t len, const char *filename,
+int append_eol)
{
struct lock_file msg_file = LOCK_INIT;
diff --git a/sequencer.h b/sequencer.h
index 33a6070c64..0ccbe390b2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -67,9 +67,6 @@ struct replay_opts {
};
#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
-int write_message(const void *buf, size_t len, const char *filename,
- int append_eol);
-
/*
* Note that ordering matters in this enum. Not only must it match the mapping
* of todo_command_info (in sequencer.c), it is also divided into several
--
2.20.0
[PATCH] revision.c: fix sparse warnings (sparse algorithm)
Signed-off-by: Ramsay Jones
---
Hi Derrick,
If you need to re-roll your 'ds/push-sparse-tree-walk' branch, could
you please squash this into the relevant patch [commit 9949aaeef4
("revision: implement sparse algorithm", 2018-12-14)].
This commit caused both 'sparse' and my 'static-check.pl' script to
complain about the visibility of the 'map_flags' variable (it is a
file local variable), so one solution would be to mark it 'static'.
However, it is simply not being used, so ...
Thanks!
ATB,
Ramsay Jones
revision.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index a048da3cf5..c4982a70b1 100644
--- a/revision.c
+++ b/revision.c
@@ -114,10 +114,9 @@ static int path_and_oids_cmp(const void
*hashmap_cmp_fn_data,
return strcmp(e1->path, e2->path);
}
-int map_flags = 0;
static void paths_and_oids_init(struct hashmap *map)
{
- hashmap_init(map, (hashmap_cmp_fn) path_and_oids_cmp, &map_flags, 0);
+ hashmap_init(map, (hashmap_cmp_fn) path_and_oids_cmp, NULL, 0);
}
static void paths_and_oids_clear(struct hashmap *map)
--
2.20.0
Re: [PATCH v17 0/7] git bisect: convert from shell to C
On 02/01/2019 15:38, Tanushree Tumane via GitGitGadget wrote:
[snip]
> base-commit: 7f4e64169352e03476b0ea64e7e2973669e491a2
> Published-As:
> https://github.com/gitgitgadget/git/releases/tag/pr-101%2Ftanushree27%2Fgit-bisect_part2_fixup-v17
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> pr-101/tanushree27/git-bisect_part2_fixup-v17
> Pull-Request: https://github.com/gitgitgadget/git/pull/101
I didn't look at the patches, only the range-diff below, and the
only thing I noticed was ...
>
> Range-diff vs v16:
>
> 1: f1e89ba517 ! 1: 338ebdc97a bisect--helper: `bisect_reset` shell
> function in C
> @@ -16,8 +16,9 @@
>
> Mentored-by: Lars Schneider
> Mentored-by: Christian Couder
> +Mentored by: Johannes Schindelin
> Signed-off-by: Pranit Bauva
> -Signed-off-by: Junio C Hamano
> +Signed-off-by: Tanushree Tumane
>
>diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>--- a/builtin/bisect--helper.c
> @@ -53,8 +54,10 @@
> + struct strbuf branch = STRBUF_INIT;
> +
> + if (!commit) {
> -+ if (strbuf_read_file(&branch, git_path_bisect_start(),
> 0) < 1)
> -+ return !printf(_("We are not bisecting.\n"));
> ++ if (strbuf_read_file(&branch, git_path_bisect_start(),
> 0) < 1) {
> ++ printf(_("We are not bisecting.\n"));
> ++ return 0;
> ++ }
> + strbuf_rtrim(&branch);
> + } else {
> + struct object_id oid;
> @@ -69,11 +72,11 @@
> +
> + argv_array_pushl(&argv, "checkout", branch.buf, "--",
> NULL);
> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> -+ error(_("Could not check out original HEAD
> '%s'. Try "
> -+ "'git bisect reset '."),
> branch.buf);
> + strbuf_release(&branch);
> + argv_array_clear(&argv);
> -+ return -1;
> ++ return error(_("could not check out original"
> ++ " HEAD '%s'. Try 'git bisect"
> ++ "reset '."), branch.buf);
... this 'branch.buf' will refer to the empty 'slopbuf', since
the call to 'strbuf_release(&branch)' now precedes this call
to error().
ATB,
Ramsay Jones
Re: [PATCH 1/2] t5403: Refactor
On 28/12/2018 22:34, Junio C Hamano wrote:
> org...@gmail.com writes:
>
>> Subject: Re: [PATCH 1/2] t5403: Refactor
>
[snip]
>> if test "$(git config --bool core.filemode)" = true; then
>
> This is a tangent but this conditional came from an ancient d42ec126
> ("disable post-checkout test on Cygwin", 2009-03-17) that says
>
> disable post-checkout test on Cygwin
>
> It is broken because of the tricks we have to play with
> lstat to get the bearable perfomance out of the call.
> Sadly, it disables access to Cygwin's executable attribute,
> which Windows filesystems do not have at all.
>
> I wonder if this is still relevant these days (Cc'ed Ramsay for
> input).
Ah, no, the 'tricks we have to play with lstat' mentioned in that
commit message are long gone! ;-) If you remove that conditional,
then the test passes just fine.
ATB,
Ramsay Jones
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(&dup->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
