Re: [PATCH 2/6] http: always update the base URL for redirects
On 01/12/16 09:04, Jeff King wrote: > If a malicious server redirects the initial ref > advertisement, it may be able to leak sha1s from other, > unrelated servers that the client has access to. For > example, imagine that Alice is a git user, she has access to > a private repository on a server hosted by Bob, and Mallory > runs a malicious server and wants to find out about Bob's > private repository. > > Mallory asks Alice to clone an unrelated repository from her ---^^^ ... from _him_ ? (ie Mallory) > over HTTP. When Alice's client contacts Mallory's server for > the initial ref advertisement, the server issues an HTTP > redirect for Bob's server. Alice contacts Bob's server and > gets the ref advertisement for the private repository. If > there is anything to fetch, she then follows up by asking > the server for one or more sha1 objects. But who is the > server? > > If it is still Mallory's server, then Alice will leak the > existence of those sha1s to her. ------^^^ ... to _him_ ? (again Mallory) ATB, Ramsay Jones
Re: [PATCH 4/6] http: make redirects more obvious
On 01/12/16 09:04, Jeff King wrote: > We instruct curl to always follow HTTP redirects. This is > convenient, but it creates opportunities for malicious > servers to create confusing situations. For instance, > imagine Alice is a git user with access to a private > repository on Bob's server. Mallory runs her own server and Ahem, so Mallory is female? (-blush-) :( ATB, Ramsay Jones
Re: [PATCH] difftool.c: mark a file-local symbol with static
On 30/11/16 23:46, Junio C Hamano wrote:
> Jeff King <p...@peff.net> writes:
>
>> I forgot, we ended up reversing course later and silencing them:
>>
>> http://public-inbox.org/git/20140505052117.gc6...@sigill.intra.peff.net/
>>
>> By the rationale of that conversation, we should be doing:
>>
>> warning("%s", "");
>>
>> here.
>
> I forgot too. Thanks for digging up that thread.
Yes, I blamed wt-status.c:227 and came up with commit 7d7d68022
as well.
So, by the same rationale, we should remove -Wno-format-zero-length
from DEVELOPER_CFLAGS. yes?
ATB,
Ramsay Jones
Re: [PATCH] difftool.c: mark a file-local symbol with static
On 30/11/16 21:25, Jeff King wrote:
> On Wed, Nov 30, 2016 at 12:40:25PM -0800, Junio C Hamano wrote:
>
>> Ramsay Jones <ram...@ramsayjones.plus.com> writes:
>>
>>> [I have fixed my config.mak file now, so I don't see the warning
>>> anymore! Having -Wno-format-zero-length in DEVELOPER_CFLAGS, or
>>> not, is a separate matter.]
>>
>> I suspect that 658df95a4a ("add DEVELOPER makefile knob to check for
>> acknowledged warnings", 2016-02-25) took it from me (namely, Make
>> script in my 'todo' branch). In turn, I added it to my set of flags
>> in order to squelch this exact warning, so...
>
> For anybody interested in the history, we started using this when
> status_printf() got the format attribute. Relevant patch and discussion:
>
>
> http://public-inbox.org/git/20130710002328.gc19...@sigill.intra.peff.net/T/#u
Ah, thank you! I've been trying to remember this for the last hour or so ...
(I misremembered something about gettext, and went looking in the wrong
direction ;-) ).
> We went with disabling the warning because it really is wrong. It makes
> an assumption that calling a format function with an empty string
> doesn't do anything, but that's only true of the stock printf functions.
> Our custom functions _do_ have a side effect in this case.
Yes, I agree, gcc is making an unwarranted assumption.
> The other options are to have a special function for "print a warning
> (or status) line with no content". Or to teach those functions to handle
> newlines specially. We've often discussed that you should be able to do:
>
> warning("foo\nbar");
>
> and have it print:
>
> warning: foo
> warning: bar
>
> That's useful in itself, and would probably make cases like this easier
> to handle, too. But it's a pretty big change. Another option would be to
> just teach formatting functions to handle a single "\n" as a synonym for
> the empty string (or even detect trailing newlines and avoid appending
> our own in that case). That would mean you could do:
>
> warning("\n");
>
> to print a blank line. That's arguably more obvious about the intent to
> a reader (I say arguably because the new behavior _is_ subtle if you
> happen to know that warning() usually appends a newline).
Yes, I remember the discussion now and agree that this could
cause problems.
> Anyway. Those are all options, but I don't think there is any problem
> with sticking with warning("") for now. It is not the first part of the
> code that tickles the format-zero-length warning.
Hmm, well I have been building git for some time with the warning
enabled without problem.
[I can see a few examples of 'status_printf_ln(s, c, "%s", "");' in
git-grep output, so ...]
I am not suggesting any change here, and was just curious why it
seemed to be unnecessary now (I knew it was necessary at one time).
ATB,
Ramsay Jones
Re: [PATCH] difftool.c: mark a file-local symbol with static
On 30/11/16 11:07, Johannes Schindelin wrote:
> Hi Ramsay,
>
> On Tue, 29 Nov 2016, Ramsay Jones wrote:
>> Also, due to a problem in my config.mak file on Linux (a commented
>> out line that had a line continuation '\', gr!), gcc issued a
>> warning, thus:
>>
>> builtin/difftool.c: In function ‘run_dir_diff’:
>> builtin/difftool.c:568:13: warning: zero-length gnu_printf format string
>> [-Wformat-zero-length]
>>warning("");
>>^
>> I am not sure why -Wno-format-zero-length is set in DEVELOPER_CFLAGS,
>> but do you really need to space the output with an an 'empty'
>> "warning:" line? (Just curious).
>
> That `warning("");` comes from a straight-forward port of this line (see
> https://github.com/git/git/blob/v2.11.0/git-difftool.perl#L425):
>
> $errmsg .= "warning:\n";
Ah, OK, so it is being used to 'space out' the (possibly multiple)
'Both files modified' warning(s) followed by a 2-line warning
summary. Hmm, I am not sure the 'Working tree file has been left'
(after each pair of files) part of the message is adding much ...
> I could see two possible ways out:
>
> - warning("%s", ""); (ugly!)
>
> - do away with the "prefix every line with warning:" convention and simply
> have a multi-line `warning(_("...\n...\n"), ...)`
>
> What do you think?
I think, for now, it is probably best to do nothing. ;-)
Until you have replaced the 'legacy' difftool, it would be best
not to alter the output from the tool, so that the tests can be
used unaltered on both. This could be addressed (if necessary)
after you complete your series.
[I have fixed my config.mak file now, so I don't see the warning
anymore! Having -Wno-format-zero-length in DEVELOPER_CFLAGS, or
not, is a separate matter.]
ATB,
Ramsay Jones
[PATCH] difftool.c: mark a file-local symbol with static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Johannes,
If you need to re-roll your 'js/difftool-builtin' branch, could
you please squash this into the relevant patch.
Thanks!
Also, due to a problem in my config.mak file on Linux (a commented
out line that had a line continuation '\', gr!), gcc issued a
warning, thus:
builtin/difftool.c: In function ‘run_dir_diff’:
builtin/difftool.c:568:13: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
warning("");
^
I am not sure why -Wno-format-zero-length is set in DEVELOPER_CFLAGS,
but do you really need to space the output with an an 'empty'
"warning:" line? (Just curious).
ATB,
Ramsay Jones
builtin/difftool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 3480920..830369c 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -170,7 +170,7 @@ struct path_entry {
char path[FLEX_ARRAY];
};
-int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
+static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void
*key)
{
return strcmp(a->path, key ? key : b->path);
}
--
2.9.0
Re: [PATCH] worktree: fix a sparse 'Using plain integer as NULL pointer' warning
On 15/11/16 20:28, Ramsay Jones wrote:
>
> Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
> ---
>
> Hi Duy,
>
> If you need to re-roll your 'nd/worktree-move' branch, could you
> please squash this into the relevant patch [commit c49e92f5c
> ("worktree move: refuse to move worktrees with submodules", 12-11-2016)].
>
> Also, one of the new tests introduced by commit 31a8f3066 ("worktree move:
> new command", 12-11-2016), fails for me, thus:
>
> $ ./t2028-worktree-move.sh -i -v
> ...
> --- expected2016-11-15 20:22:50.647241458 +
> +++ actual 2016-11-15 20:22:50.647241458 +
> @@ -1,3 +1,3 @@
>worktree /home/ramsay/git/t/trash directory.t2028-worktree-move
> -worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
>worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/elsewhere
> +worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
> not ok 12 - move worktree
> #
> # git worktree move source destination &&
> # test_path_is_missing source &&
> # git worktree list --porcelain | grep "^worktree" >actual &&
> # cat <<-EOF >expected &&
> # worktree $TRASH_DIRECTORY
> # worktree $TRASH_DIRECTORY/destination
> # worktree $TRASH_DIRECTORY/elsewhere
> # EOF
> # test_cmp expected actual &&
> # git -C destination log --format=%s >actual2 &&
> # echo init >expected2 &&
> # test_cmp expected2 actual2
> #
> $
>
> Is there an expectation that the submodules will be listed in
Er, that should read 'worktrees', of course! :(
ATB,
Ramsay Jones
[PATCH] worktree: fix a sparse 'Using plain integer as NULL pointer' warning
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Duy,
If you need to re-roll your 'nd/worktree-move' branch, could you
please squash this into the relevant patch [commit c49e92f5c
("worktree move: refuse to move worktrees with submodules", 12-11-2016)].
Also, one of the new tests introduced by commit 31a8f3066 ("worktree move:
new command", 12-11-2016), fails for me, thus:
$ ./t2028-worktree-move.sh -i -v
...
--- expected 2016-11-15 20:22:50.647241458 +
+++ actual2016-11-15 20:22:50.647241458 +
@@ -1,3 +1,3 @@
worktree /home/ramsay/git/t/trash directory.t2028-worktree-move
-worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/elsewhere
+worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
not ok 12 - move worktree
#
# git worktree move source destination &&
# test_path_is_missing source &&
# git worktree list --porcelain | grep "^worktree" >actual &&
# cat <<-EOF >expected &&
# worktree $TRASH_DIRECTORY
# worktree $TRASH_DIRECTORY/destination
# worktree $TRASH_DIRECTORY/elsewhere
# EOF
# test_cmp expected actual &&
# git -C destination log --format=%s >actual2 &&
# echo init >expected2 &&
# test_cmp expected2 actual2
#
$
Is there an expectation that the submodules will be listed in
any particular order by 'git worktree list --porcelain' ?
Thanks!
ATB,
Ramsay Jones
builtin/worktree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index e738142..abdf462 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -526,7 +526,7 @@ static int unlock_worktree(int ac, const char **av, const
char *prefix)
static void validate_no_submodules(const struct worktree *wt)
{
- struct index_state istate = {0};
+ struct index_state istate = { NULL };
int i, found_submodules = 0;
if (read_index_from(, worktree_git_path(wt, "index")) > 0) {
--
2.10.0
Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
On 14/11/16 17:01, Jeff King wrote: > On Mon, Nov 14, 2016 at 05:35:56PM +0100, Torsten Bögershausen wrote: > >>> Git 'pu' does not compile on macOS right now: >>> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used >>> uninitialized >>> whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > > The next step is to make sure that the topic author is aware (in this > case, one assumes it's pb/bisect). [+cc Pranit] Yep, I had a quick squint, and it looks like the compiler is correct. It should be complaining about the 'bad_syn' variable for exactly the same reason: namely, whenever the if condition is true, the only exit from that block is via 'goto finish' which bypasses the initialisation of 'good_syn' and 'bad_syn'. > Better still is to make a patch that can either be applied on top, or > squashed as appropriate. No patch this time, but it simply requires those variables to be initialised to NULL in their declarations. :-D >I know that Ramsay Jones does this, for > example, with some of his sparse-related checks, and I'm pretty sure > from the turnaround-time that he runs it against "pu". Yep, the idea being to catch these simple problems before the topic reaches 'next'. ATB, Ramsay Jones
[PATCH] attr: mark a file-local symbol as static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Stefan,
If you need to re-roll your 'sb/attr' branch, could you please
squash this into the relevant patch.
Alternatively, since there is only a single call site for git_attr()
(on line #1005), you could perhaps remove git_attr() and inline that
call. (However, that does make that line exceed 80 columns).
Thanks!
ATB,
Ramsay Jones
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 667ba85..84c4b08 100644
--- a/attr.c
+++ b/attr.c
@@ -169,7 +169,7 @@ static struct git_attr *git_attr_internal(const char *name,
int len)
return a;
}
-struct git_attr *git_attr(const char *name)
+static struct git_attr *git_attr(const char *name)
{
return git_attr_internal(name, strlen(name));
}
--
2.10.0
Re: [PATCHv2 00/36] Revamp the attr subsystem!
On 28/10/16 19:54, Stefan Beller wrote: > previous discussion at > https://public-inbox.org/git/20161022233225.8883-1-sbel...@google.com > > This implements the discarded series': > jc/attr > jc/attr-more > sb/pathspec-label > sb/submodule-default-paths > > This includes > * The fixes for windows > * Junios latest suggestion to use git_attr_check_initv instead of > alloc/append. > > * I implemented the thread safe attr API in patch 27 (attr: convert to new > threadsafe API) > * patch 28 (attr: keep attr stack for each check) makes it actually possible > to run in a multithreaded environment. > * I added a test for the multithreaded when it is introduced in patch 32 > (pathspec: allow querying for attributes) as well as a test to disallow > multiple "attr"s in a pathspec. By the end of this series, 'git_attr_counted()' and 'git_attr()' are both file local symbols and can be marked with static. (I gave up the search for which actual patch should change the symbols to static). Also, 'git_attr()' ends up with a single caller, so maybe inline that call? I was about to have a moan about PTHREAD_MUTEX_INITIALIZER, since it causes sparse to issue some warnings, but I see that you have decided not to use it. So, phew! ;-) ATB, Ramsay Jones
Re: [PATCH] Allow stashes to be referenced by index only
On 25/10/16 22:41, Junio C Hamano wrote:
> Aaron M Watson <watso...@gmail.com> writes:
>
> Aaron M Watson <watso...@gmail.com> writes:
>
>> Instead of referencing "stash@{n}" explicitly, it can simply be
>> referenced as "n".
>> Most users only reference stashes by their position
>> in the stash stask (what I refer to as the "index").
>
> It is unclear if the first sentence is a statement of the fact, an
> expression of desire, or something else. With the current codebase,
> it cannot simply be referenced as "n", and you either "wish it were
> possible", or "make it possible to do so", or perhaps little bit of
> both.
>
> This is why we tend to use imperative mood to give an order to the
> codebase to "be like so" to make it clear.
>
> Perhaps
>
> Instead of referencing "stash@{n}" explicitly, make it possible to
> simply reference as "n". Most users only reference stashes by their
> position in the stash stask (what I refer to as the "index" here).
s/stask/stack/
ATB,
Ramsay Jones
Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary
On 23/10/16 10:26, Christian Couder wrote: > When writing a new split-index and there is a big number of cache > entries in the split-index compared to the shared index, it is a > good idea to regenerate the shared index. > > By default when the ratio reaches 20%, we will push back all > the entries from the split-index into a new shared index file > instead of just creating a new split-index file. > > The threshold can be configured using the > "splitIndex.maxPercentChange" config variable. > > We need to adjust the existing tests in t1700 by setting > "splitIndex.maxPercentChange" to 100 at the beginning of t1700, > as the existing tests are assuming that the shared index is > regenerated only when `git update-index --split-index` is used. > > Signed-off-by: Christian Couder <chrisc...@tuxfamily.org> > --- > read-cache.c | 33 - > t/t1700-split-index.sh | 1 + > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/read-cache.c b/read-cache.c > index bb53823..a91fabe 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -2216,6 +2216,36 @@ static int write_shared_index(struct index_state > *istate, > return ret; > } > > +static const int default_max_percent_split_change = 20; > + > +int too_many_not_shared_entries(struct index_state *istate) This function is a file-loacal symbol; could you please make it a static function. Thanks. ATB, Ramsay Jones
Re: [PATCH 28/36] attr: keep attr stack for each check
On 23/10/16 00:32, Stefan Beller wrote:
> Instead of having a global attr stack, attach the stack to each check.
> This allows to use the attr in a multithreaded way.
>
>
>
> Signed-off-by: Stefan Beller <sbel...@google.com>
> ---
> attr.c| 101
> +++---
> attr.h| 4 ++-
> hashmap.h | 2 ++
> 3 files changed, 69 insertions(+), 38 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index 89ae155..b65437d 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -372,15 +372,17 @@ static struct match_attr *parse_attr_line(const char
> *line, const char *src,
> * .gitignore file and info/excludes file as a fallback.
> */
>
> -/* NEEDSWORK: This will become per git_attr_check */
> -static struct attr_stack {
> +struct attr_stack {
> struct attr_stack *prev;
> char *origin;
> size_t originlen;
> unsigned num_matches;
> unsigned alloc;
> struct match_attr **attrs;
> -} *attr_stack;
> +};
> +
> +struct hashmap all_attr_stacks;
> +int all_attr_stacks_init;
Mark symbols 'all_attr_stacks' and 'all_attr_stacks_init' with
the static keyword. (ie. these are file-local symbols).
ATB,
Ramsay Jones
Re: [PATCH 17/36] attr: expose validity check for attribute names
On 23/10/16 00:32, Stefan Beller wrote: > From: Junio C Hamano <gits...@pobox.com> > > Export attr_name_valid() function, and a helper function that > returns the message to be given when a given <name, len> pair > is not a good name for an attribute. > > We could later update the message to exactly spell out what the > rules for a good attribute name are, etc. > > Signed-off-by: Junio C Hamano <gits...@pobox.com> > Signed-off-by: Stefan Beller <sbel...@google.com> > --- [snip] > +extern int attr_name_valid(const char *name, size_t namelen); > +extern void invalid_attr_name_message(struct strbuf *, const char *, int); > + The symbol 'attr_name_valid()' is not used outside of attr.c, even by the end of this series. Do you expect this function to be used in any future series? (The export is deliberate and it certainly seems like it should be part of the public interface, but ...) In contrast, the 'invalid_attr_name_message()' function is called from code in pathspec.c, which relies on 'git_attr_counted()' to call 'attr_name_valid()' internally to check for validity. :-D ATB, Ramsay Jones
Re: [PATCH 1/2] t9000-addresses: update expected results after fix
On 21/10/16 17:48, Junio C Hamano wrote: > Matthieu Moy <matthieu@imag.fr> writes: > >> e3fdbcc8e1 (parse_mailboxes: accept extra text after <...> address, >> 2016-10-13) improved our in-house address parser and made it closer to >> Mail::Address. As a consequence, some tests comparing it to >> Mail::Address now pass, but e3fdbcc8e1 forgot to update the test. >> >> Signed-off-by: Matthieu Moy <matthieu@imag.fr> >> --- > > Thanks. Yep, thanks for looking into this Matthieu. I applied these cleanly (to both next and pu) and tested on Linux and cygwin. Thanks again. ATB, Ramsay Jones
t9000-addresses.sh: unexpected pases
Hi Matthieu,
I have started seeing unexpected passes in this test (am I the
only one?) on the next and pu branch, which seems to be caused
by commit e3fdbcc8 ("parse_mailboxes: accept extra text after
<...> address", 13-10-2016). Thus:
$ tail -15 ntest-out
[15:17:44]
All tests successful.
Test Summary Report
---
t9000-addresses.sh (Wstat: 0 Tests: 37 Failed:
0)
TODO passed: 28, 30-31
Files=760, Tests=13940, 484 wallclock secs ( 4.04 usr 1.30 sys + 60.52 cusr
36.76 csys = 102.62 CPU)
Result: PASS
make clean-except-prove-cache
make[2]: Entering directory '/home/ramsay/git/t'
rm -f -r 'trash directory'.* 'test-results'
rm -f -r valgrind/bin
make[2]: Leaving directory '/home/ramsay/git/t'
make[1]: Leaving directory '/home/ramsay/git/t'
$
I have not even looked, but I suspect that it simply requires
a change from expect_fail to expect_success, since your commit
has 'fixed' these tests ... would you mind taking a quick look?
Thanks!
ATB,
Ramsay Jones
Re: [PATCH] convert: mark a file-local symbol static
On 17/10/16 21:07, Junio C Hamano wrote:
> Ramsay Jones <ram...@ramsayjones.plus.com> writes:
>
>> Heh, I actually have the following in my config.mak already:
>>
>> extra-clean: clean
>> find . -iname '*.o' -exec rm {} \;
>>
>> But for some reason I _always_ type 'make clean' and then, to top
>> it off, I _always_ type the 'find' command by hand (I have no idea
>> why) :-D
>
> "git clean -x" anybody?
I don't use 'git clean' because, on the very few occasions that I have
tried to use it, it always deletes _far_ more than I thought it would
or should. (particularly config.mak). Hmm, "git clean -X -- '*.o'"
_might_ do what I want, but 'find' is so much easier ... :-D
ATB,
Ramsay Jones
Re: [PATCH] convert: mark a file-local symbol static
On 17/10/16 10:37, Jeff King wrote:
> On Mon, Oct 17, 2016 at 11:04:19AM +0200, Johannes Schindelin wrote:
>
>>> Gross. I would not be opposed to a Makefile rule that outputs the
>>> correct set of OBJECTS so this (or other) scripts could build on it.
>>
>> You could also use the method I use in Git for Windows to "extend" the
>> Makefile:
>>
>> -- snipsnap --
>> cat >dummy.mak <> include Makefile
>>
>> blub: $(OBJECTS)
>> do-something-with $^
>> EOF
>>
>> make -f dummy.mak blub
>
> Hacky but clever. I like it.
>
> In the particular case of git, I think I've cheated similarly before by
> putting things in config.mak, though of course an arbitrary script can't
> assume it can overwrite that file.
Heh, I actually have the following in my config.mak already:
extra-clean: clean
find . -iname '*.o' -exec rm {} \;
But for some reason I _always_ type 'make clean' and then, to top
it off, I _always_ type the 'find' command by hand (I have no idea
why) :-D
ATB,
Ramsay Jones
Re: [PATCH] convert: mark a file-local symbol static
On 17/10/16 03:18, Jeff King wrote: > On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote: > >> Hmm, well, you have to remember that 'make clean' sometimes >> doesn't make clean. Ever since the Makefile was changed to only >> remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to >> remember to 'make clean' _before_ you switch branches. Otherwise, >> you risk leaving some objects laying around. Since the script >> runs 'nm' on all objects it finds, any stale ones can cause problems. >> (Of course, I almost always forget, so I frequently have to manually >> check for and remove stale objects!) > > Gross. I would not be opposed to a Makefile rule that outputs the > correct set of OBJECTS so this (or other) scripts could build on it. > > IIRC, BSD make has an option to do this "make -V OBJECTS" or something, > but I don't thnk there's an easy way to do so. Hmm, I would go in the opposite direction and take a leaf out of Ævar's book (see commit bc548efe) and this one-liner: diff --git a/Makefile b/Makefile index ee89c06..c08c25e 100644 --- a/Makefile +++ b/Makefile @@ -2506,7 +2506,7 @@ profile-clean: clean: profile-clean coverage-clean $(RM) *.res - $(RM) $(OBJECTS) + $(RM) $(addsuffix *.o,$(object_dirs)) $(RM) $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB) $(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X $(RM) $(TEST_PROGRAMS) $(NO_INSTALL) This would actually solve my problem, but it actually isn't a _complete_ solution. (Hint: think about what isn't in $(OBJECTS), depending on the configuration). ;-) > Or, since it seems to find useful results quite frequently, maybe it > would be worth including the script inside git (and triggering it with > an optional Makefile rule). It sounds like we'd need a way to annotate > known false positives, but if it were in common use, it would be easier > to get people to keep that list up to date. Hmm, I suspect that wouldn't happen, which would reduce it usefulness and ultimately lead to it not being used. (Updating the 'stop list' would fast become a burden.) I find it useful to flag these issues automatically, but I still need to look at each symbol and decide what to do (you may not agree with some of my choices either - take a look at the output on the master branch!). The way I use it, I effectively ignore the 'stop list' maintenance issues. ATB, Ramsay Jones
Re: [PATCH] convert: mark a file-local symbol static
On 16/10/16 01:15, Lars Schneider wrote: >> On 15 Oct 2016, at 14:01, Ramsay Jones <ram...@ramsayjones.plus.com> wrote: >> On 15/10/16 16:05, Lars Schneider wrote: >>>> On 11 Oct 2016, at 16:46, Ramsay Jones <ram...@ramsayjones.plus.com> wrote: >> [snip] >>>> -void stop_multi_file_filter(struct child_process *process) >>>> +static void stop_multi_file_filter(struct child_process *process) >>> >>> Done! Do you have some kind of script to detect these things >>> automatically or do you read the code that carefully? >> >> Heh, I'm _far_ too lazy to read the code that carefully. :-D >> >> A combination of 'make sparse' and a perl script (originally >> posted to the list by Junio) find all of these for me. > > Interesting. Do you have a link to this script? I'm attaching _a_ version of the script (you would think that I would have it under version control; but no, I copy versions around the several machines/git.git repos I have ... :-P ). > Does it generate false positives? Hmm, well, you have to remember that 'make clean' sometimes doesn't make clean. Ever since the Makefile was changed to only remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to remember to 'make clean' _before_ you switch branches. Otherwise, you risk leaving some objects laying around. Since the script runs 'nm' on all objects it finds, any stale ones can cause problems. (Of course, I almost always forget, so I frequently have to manually check for and remove stale objects!) Also, you have to interpret the results of the script. Just because it shows you an unused external, that doesn't make it an error. The way I use it, BTW, is to run it on a freshly built branch (saving to a file) and then compare the files from master->next->pu. I don't bother looking at the output on master (the 'stop list' in the script is _way_ out of date), just the diffs master->next and next->pu. The current next->pu diff looks like: $ diff nsc psc 0a1,2 > attr.o- attr_name_valid > attr.o- invalid_attr_name_message 17a20 > convert.o - stop_multi_file_filter 34d36 < quote.o - sq_quotef 43a46 > sequencer.o - append_new_todo 50a54 > trailer.o - conf_head 55a60,61 > wt-status.o - has_uncommitted_changes > wt-status.o - has_unstaged_changes $ I have already sent mails about 'stop_multi_file_filter', 'append_new_todo' and 'conf_head'. The symbols in attr.o are part of the revamp of the attribute system that Junio and Stefan are working on, which is very much in flux. If they are still there when it looks to have firmed up, then I will take a look. The 'sq_quotef' symbol is an example of a useful function that was written without having an immediate user (actually, if memory serves correctly, it was originally used in the series which introduced it, but later modifications removed the caller). It now has a user. The wt-status.o symbols have been introduced by Johannes in his 'rebase -i' series (of series) and I am assuming that these symbols will find additional external callers in up-coming series. > Maybe I can add `make sparse` to the TravisCI build? I don't know how feasible that would be. Apparently, the version of sparse in most distribution repositories is so old that it spews so many spurious errors as to make it unusable. See, for example: https://public-inbox.org/git/56ca5753.9030...@ramsayjones.plus.com/ So, you would need to build from source to get a usable version (I am currently running a version built with some local patches). If you were to build from the sparse repo's master branch, then it would work great on Linux, at least. I have no idea if it would work on mac OS X. However, even then, a single sparse warning is issued on Linux (for the master branch, several others on pu): SP pack-revindex.c pack-revindex.c:65:23: warning: memset with byte count of 262144 This is a problem with sparse (the byte count limit used is hardcoded into sparse), but I haven't sent a patch upstream to fix it yet. (I've been looking at that warning for _many_ years, so don't hold your breath!) ATB, Ramsay Jones static-check.pl Description: Perl program
Re: [PATCH] convert: mark a file-local symbol static
On 15/10/16 16:05, Lars Schneider wrote: >> On 11 Oct 2016, at 16:46, Ramsay Jones <ram...@ramsayjones.plus.com> wrote: [snip] >> -void stop_multi_file_filter(struct child_process *process) >> +static void stop_multi_file_filter(struct child_process *process) > > Done! Do you have some kind of script to detect these things > automatically or do you read the code that carefully? Heh, I'm _far_ too lazy to read the code that carefully. :-D A combination of 'make sparse' and a perl script (originally posted to the list by Junio) find all of these for me. ATB, Ramsay Jones
[PATCH] trailer: mark file-local symbol 'conf_head' static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Jonathan,
If you need to re-roll your 'jt/trailer-with-cruft' branch, could you
please squash this into the relevant patch. [commit 3fb120de ("trailer:
use list.h for doubly-linked list", 14-10-2016)]
Thanks!
ATB,
Ramsay Jones
trailer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/trailer.c b/trailer.c
index 1be6ec8..2862a48 100644
--- a/trailer.c
+++ b/trailer.c
@@ -42,7 +42,7 @@ struct arg_item {
struct conf_info conf;
};
-LIST_HEAD(conf_head);
+static LIST_HEAD(conf_head);
static char *separators = ":";
--
2.10.0
[PATCH] sequencer: mark a file-local symbol static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Johannes,
If you need to re-roll your 'js/prepare-sequencer' branch, could you
please squash this into commit 53f8024e ("sequencer: completely revamp
the "todo" script parsing", 10-10-2016).
Thanks.
ATB,
Ramsay Jones
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index d662c6b..aa65628 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -880,7 +880,7 @@ static void todo_list_release(struct todo_list *todo_list)
todo_list->nr = todo_list->alloc = 0;
}
-struct todo_item *append_new_todo(struct todo_list *todo_list)
+static struct todo_item *append_new_todo(struct todo_list *todo_list)
{
ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
return todo_list->items + todo_list->nr++;
--
2.10.0
[PATCH] convert: mark a file-local symbol static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Lars,
If you need to re-roll your 'ls/filter-process' branch, could you
please squash this into commit 85290197
("convert: add filter..process option", 08-10-2016).
Thanks.
ATB,
Ramsay Jones
convert.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/convert.c b/convert.c
index fe68316..cf30380 100644
--- a/convert.c
+++ b/convert.c
@@ -568,7 +568,7 @@ static void kill_multi_file_filter(struct hashmap *hashmap,
struct cmd2process *
free(entry);
}
-void stop_multi_file_filter(struct child_process *process)
+static void stop_multi_file_filter(struct child_process *process)
{
sigchain_push(SIGPIPE, SIG_IGN);
/* Closing the pipe signals the filter to initiate a shutdown. */
--
2.10.0
Re: Regression: git no longer works with musl libc's regex impl
On 06/10/16 20:18, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Oct 4, 2016 at 6:08 PM, Johannes Schindelin
> <johannes.schinde...@gmx.de> wrote:
>> As to making NO_REGEX conditional on REG_STARTEND: you are talking about
>> apples and oranges here. NO_REGEX is a Makefile flag, while REG_STARTEND
>> is a C preprocessor macro.
>>
>> Unless you can convince the rest of the Git developers (you would not
>> convince me) to simulate autoconf by compiling an executable every time
>> `make` is run, to determine whether REG_STARTEND is defined, this is a
>> no-go.
>
> But just to clarify, does anyone have any objection to making our
> configure.ac compile a C program to check for this sort of thing?
> Because that seems like the easiest solution to this class of problem.
Err, you do know that we already do that, right?
[see commit a1e3b669 ("autoconf: don't use platform regex if it lacks
REG_STARTEND", 17-08-2010)]
In fact, if you run the auto tools on cygwin, you get a different setting
for NO_REGEX than via config.mak.uname. Which is why I don't run configure
on cygwin. :-D
[The issue is exposed by t7008-grep-binary.sh, where the cygwin native
regex library matches '.' in a pattern with the NUL character. ie the
test_expect_failure test passes.]
ATB,
Ramsay Jones
[PATCH] run-command: fix build on cygwin (stdin is a macro)
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Lars,
Commit 6007c69e ("run-command: add wait_on_exit", 04-10-2016), which
is part of your 'ls/filter-process' branch, causes the build to fail
on cygwin, since 'stdin' is defined as a macro thus:
#define stdin (_REENT->_stdin)
(you can probably guess what stdout and stderr look like!) where _REENT
in turn expands to a function call (__getreent()) which returns a pointer
to a 'struct _reent', etc., ...
I am not suggesting that you apply this exact patch (stdin_ is not a good
choice), but I wanted to show the exact patch I used to get the build to
complete on cygwin.
ATB,
Ramsay Jones
run-command.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/run-command.c b/run-command.c
index 96c54fe..a9dd91a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -22,7 +22,7 @@ void child_process_clear(struct child_process *child)
struct child_to_clean {
pid_t pid;
char *name;
- int stdin;
+ int stdin_;
int wait;
struct child_to_clean *next;
};
@@ -37,8 +37,8 @@ static void cleanup_children(int sig, int in_signal)
/* Close the the child's stdin as indicator that Git will exit soon */
while (p) {
if (p->wait)
- if (p->stdin > 0)
- close(p->stdin);
+ if (p->stdin_ > 0)
+ close(p->stdin_);
p = p->next;
}
@@ -73,12 +73,12 @@ static void cleanup_children_on_exit(void)
cleanup_children(SIGTERM, 0);
}
-static void mark_child_for_cleanup(pid_t pid, const char *name, int stdin, int
wait)
+static void mark_child_for_cleanup(pid_t pid, const char *name, int stdin_,
int wait)
{
struct child_to_clean *p = xmalloc(sizeof(*p));
p->pid = pid;
p->wait = wait;
- p->stdin = stdin;
+ p->stdin_ = stdin_;
if (name)
p->name = xstrdup(name);
else
@@ -94,7 +94,7 @@ static void mark_child_for_cleanup(pid_t pid, const char
*name, int stdin, int w
}
#ifdef NO_PTHREADS
-static void mark_child_for_cleanup_no_wait(pid_t pid, const char *name, int
timeout, int stdin)
+static void mark_child_for_cleanup_no_wait(pid_t pid, const char *name, int
timeout, int stdin_)
{
mark_child_for_cleanup(pid, NULL, 0, 0);
}
--
2.10.0
[PATCH] tmp-objdir: mark some file local symbols static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Jeff,
If you need to re-roll your 'jk/quarantine-received-objects' branch,
could you please squash this into the relevant patches.
[I also note that tmp_objdir_destroy(), declared to be part of the
public interface, is not currently called from outside tmp_objdir.c.
Despite being unlikely, it is at least plausible that this function
may be useful in future. ;-) ]
Thanks!
ATB,
Ramsay Jones
builtin/receive-pack.c | 2 +-
tmp-objdir.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 25afa3f..4194088 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -87,7 +87,7 @@ static enum {
} use_keepalive;
static int keepalive_in_sec = 5;
-struct tmp_objdir *tmp_objdir;
+static struct tmp_objdir *tmp_objdir;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 2181a42..3a58404 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -17,7 +17,7 @@ struct tmp_objdir {
* more than one, and we can expand later if so. You can have many such
* tmp_objdirs simultaneously in many processes, of course.
*/
-struct tmp_objdir *the_tmp_objdir;
+static struct tmp_objdir *the_tmp_objdir;
static void tmp_objdir_free(struct tmp_objdir *t)
{
--
2.10.0
Re: [PATCH v2] gpg-interface: use more status letters
On 28/09/16 20:59, Junio C Hamano wrote: > Michael J Gruber <g...@drmicha.warpmail.net> writes: >> + "X" for a good expired signature, or good signature made by an expired >> key, > > As an attempt to clarify that we cover both EXPSIG and EXPKEYSIG > cases, I think this is good enough. I may have phrased the former > slightly differently, though: "a good signature that has expired". > > I have no strong opinion if we want to stress that we cover both > cases, though, which is I think what Ramsay's comment was about. Kinda! ;-) I'm not sure that it is a good idea to mash both EXPSIG and EXPKEYSIG into one status letter, but I was also fishing for some information about EXPSIG. I was only vaguely aware that a signature could expire _independently_ of the key used to do the signing. Also, according to https://www.gnupg.org/documentation/manuals/gnupg/Automated-signature-checking.html for the EXPSIG case 'Note, that this case is currently not implemented.' Hmm, I guess these are so closely related that a single status letter is OK, but I think I would prefer your phrasing; namely: "X" for a good signature that has expired, or a good signature made with an expired key, [Although that is still a bit cumbersome.] ATB, Ramsay Jones
Re: [PATCH v2] gpg-interface: use more status letters
On 28/09/16 15:24, Michael J Gruber wrote:
> According to gpg2's doc/DETAILS:
> "For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
> EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted."
>
> gpg1 ("classic") behaves the same (although doc/DETAILS
> differs).
>
> Currently, we parse gpg's status output for GOODSIG, BADSIG and trust
> information and translate that into status codes G, B, U, N for the %G?
> format specifier.
>
> git-verify-* returns success in the GOODSIG case only. This is somewhat in
> disagreement with gpg, which considers the first 5 of the 6 above as VALIDSIG,
> but we err on the very safe side.
>
> Introduce additional status codes E, X, R for ERRSIG, EXP*SIG, REVKEYSIG
> so that a user of %G? gets more information about the absence of a 'G'
> on first glance.
>
> Requested-by: Alex <agram...@gmail.com>
> Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
> ---
> Changes in v2:
>
> - Use GNUPGHOME="$HOME/gnupg-home-not-used" just like in other tests (lib).
> - Do not parse for signer UID in the ERRSIG case (and test that we do not).
> - Retreat "rather" addition from the doc: good/valid are terms that we use
> differently from gpg anyways.
>
> Documentation/pretty-formats.txt | 9 +++--
> gpg-interface.c | 13 ++---
> pretty.c | 3 +++
> t/t7510-signed-commit.sh | 12 +++-
> 4 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt
> b/Documentation/pretty-formats.txt
> index a942d57..c28ff2b 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -143,8 +143,13 @@ ifndef::git-rev-list[]
> - '%N': commit notes
> endif::git-rev-list[]
> - '%GG': raw verification message from GPG for a signed commit
> -- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
> - "U" for a good signature with unknown validity and "N" for no signature
> +- '%G?': show "G" for a good (valid) signature,
> + "B" for a bad signature,
> + "U" for a good signature with unknown validity,
> + "X" for a good expired signature, or good signature made by an expired key,
Hmm, this looks odd. Would the following:
"X" for a good signature made with an expired key,
mean something different?
ATB,
Ramsay Jones
Re: [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
On 25/09/16 09:55, Vegard Nossum wrote:
> I use rev^..rev daily, and I'm surely not the only one. To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev^- a shorthand for that.
>
> The existing syntax rev^! seems like it should do the same, but it
> doesn't really do the right thing for merge commits (it gives only the
> merge itself).
>
> As a natural generalisation, we also accept rev^-n where n excludes the
> nth parent of rev, although this is expected to be generally less useful.
>
> [v2: Use ^- instead of % as suggested by Junio Hamano and use some
> common helper functions for parsing.]
I would place this v2 commentary below the '---' marker (so that it
won't appear in the commit message) ...
>
> Signed-off-by: Vegard Nossum <vegard.nos...@oracle.com>
> ---
... here.
> Documentation/revisions.txt | 14 +++
> builtin/rev-parse.c | 28 ++
> revision.c | 91
> +
> revision.h | 1 +
> 4 files changed, 134 insertions(+)
>
> diff --git Documentation/revisions.txt Documentation/revisions.txt
> index 4bed5b1..6e33801 100644
> --- Documentation/revisions.txt
> +++ Documentation/revisions.txt
> @@ -281,6 +281,14 @@ is a shorthand for 'HEAD..origin' and asks "What did the
> origin do since
> I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an
> empty range that is both reachable and unreachable from HEAD.
>
> +Parent Exclusion Notation
> +~
> +The '{caret}-{}', Parent Exclusion Notation::
> +Shorthand for '{caret}..', with '' = 1 if not
> +given. This is typically useful for merge commits where you
> +can just pass '{caret}-' to get all the commits in the branch
> +that was merged in merge commit ''.
> +
> Other {caret} Parent Shorthand Notations
> ~
> Two other shorthands exist, particularly useful for merge commits,
> @@ -316,6 +324,10 @@ Revision Range Summary
>but exclude those that are reachable from both. When
> either or is omitted, it defaults to `HEAD`.
>
> +'{caret}-{}', e.g. 'HEAD{caret}, HEAD{caret}-2'::
missing '-' --^
> + Equivalent to '{caret}..', with '' = 1 if not
> + given.
> +
> '{caret}@', e.g. 'HEAD{caret}@'::
>A suffix '{caret}' followed by an at sign is the same as listing
>all parents of '' (meaning, include anything reachable from
> @@ -339,6 +351,8 @@ spelt out:
> CI J F C
> B..C = ^B CC
> B...C = B ^F C G H D E B C
> + B^-= B^..B
> + = B ^B^1 E I J F B
> C^@= C^1
> = F I J F
> B^@= B^1 B^2 B^3
> diff --git builtin/rev-parse.c builtin/rev-parse.c
> index 76cf05e..ad5e6ac 100644
> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
> return 0;
> }
>
> +static int try_parent_exclusion(const char *arg)
> +{
> + int ret = 0;
> + char *to_rev = NULL;
> + char *from_rev = NULL;
> + unsigned char to_sha1[20];
> + unsigned char from_sha1[20];
As Matthieu already mentioned, maybe use 'struct object_id' here.
> +
> + if (parse_parent_exclusion(arg, _rev, _rev))
> + goto out;
> + if (get_sha1_committish(to_rev, to_sha1))
... then 'to_sha1.hash' here, etc ...
ATB,
Ramsay Jones
Re: [PATCH] run-command: async_exit no longer needs to be public
On 23/09/16 20:26, Junio C Hamano wrote: > Lars Schneider <larsxschnei...@gmail.com> writes: > >>> I do not offhand know if the topic is otherwise ready as-is, or >>> needs further work. When you need to reroll, you'd also need to >>> fetch from the result of the above from me first and then start your >>> work from it, though, if we go that route. >> >> Sounds good to me! > > OK, here is what I queued, then. This looks good to me. Thanks! ATB, Ramsay Jones
[PATCH] run-command: async_exit no longer needs to be public
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Lars,
If you need to re-roll your 'ls/filter-process' branch, could you please
squash this into the relevant commit c42a4cbc ("run-command: move check_pipe()
from write_or_die to run_command", 20-09-2016).
[Note that commit 9658846c ("write_or_die: handle EPIPE in async threads",
24-02-2016) introduced async_exit() specifically for use in the implementation
of check_pipe(). Now that you have moved check_pipe() into run-command.c,
it no longer needs to be public.]
Thanks!
ATB,
Ramsay Jones
run-command.c | 30 +++---
run-command.h | 3 +--
2 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/run-command.c b/run-command.c
index b72f6d1..3269362 100644
--- a/run-command.c
+++ b/run-command.c
@@ -6,19 +6,6 @@
#include "thread-utils.h"
#include "strbuf.h"
-void check_pipe(int err)
-{
- if (err == EPIPE) {
- if (in_async())
- async_exit(141);
-
- signal(SIGPIPE, SIG_DFL);
- raise(SIGPIPE);
- /* Should never happen, but just in case... */
- exit(141);
- }
-}
-
void child_process_init(struct child_process *child)
{
memset(child, 0, sizeof(*child));
@@ -647,7 +634,7 @@ int in_async(void)
return !pthread_equal(main_thread, pthread_self());
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
pthread_exit((void *)(intptr_t)code);
}
@@ -697,13 +684,26 @@ int in_async(void)
return process_is_async;
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
exit(code);
}
#endif
+void check_pipe(int err)
+{
+ if (err == EPIPE) {
+ if (in_async())
+ async_exit(141);
+
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
+ }
+}
+
int start_async(struct async *async)
{
int need_in, need_out;
diff --git a/run-command.h b/run-command.h
index e7c5f71..bb89c30 100644
--- a/run-command.h
+++ b/run-command.h
@@ -54,7 +54,6 @@ int finish_command(struct child_process *);
int finish_command_in_signal(struct child_process *);
int run_command(struct child_process *);
-void check_pipe(int err);
/*
* Returns the path to the hook file, or NULL if the hook is missing
@@ -141,7 +140,7 @@ struct async {
int start_async(struct async *async);
int finish_async(struct async *async);
int in_async(void);
-void NORETURN async_exit(int code);
+void check_pipe(int err);
/**
* This callback should initialize the child process and preload the
--
2.10.0
[PATCH] pkt-line: mark a file-local symbol static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Lars,
If you need to re-roll your 'ls/filter-process' branch, could you
please squash this into the relevant patch; commit 2afd9b22
("pkt-line: add packet_write_gently()", 08-09-2016).
[If you think the symbol should be public (I don't), then add a
suitable declaration to pkt-line.h instead.]
Thanks!
ATB,
Ramsay Jones
pkt-line.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pkt-line.c b/pkt-line.c
index 538e35f..4900fc0 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -181,7 +181,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
return status;
}
-int packet_write_gently(const int fd_out, const char *buf, size_t size)
+static int packet_write_gently(const int fd_out, const char *buf, size_t size)
{
static char packet_write_buffer[LARGE_PACKET_MAX];
--
2.10.0
Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function
On 08/09/16 09:35, Jeff King wrote:
> On Thu, Sep 08, 2016 at 04:14:46AM -0400, Jeff King wrote:
>
>> On Thu, Sep 08, 2016 at 04:10:24AM -0400, Jeff King wrote:
>>
>>> On Thu, Sep 08, 2016 at 09:54:51AM +0200, Johannes Schindelin wrote:
>>>
>>>>> diff.c | 3 ++-
>>>>> diffcore-pickaxe.c | 18 --
>>>>> xdiff-interface.c | 13 -
>>>>> 3 files changed, 14 insertions(+), 20 deletions(-)
>>>>
>>>> I just realized that this should switch the test_expect_failure from 1/3
>>>> to a test_expect_success.
>>>
>>> Yep. I wonder if we also would want to test that we correctly find
>>> regexes inside binary files.
>>>
>>> E.g., given a mixed binary/text file like:
>>>
>>> printf 'binary\0text' >file &&
>>> git add file &&
>>> git commit -m file
>>>
>>> then "git log -Stext" will find that file, but "--pickaxe-regex" will
>>> not (using stock git). Ditto for "-Gtext".
>>>
>>> Your patch should fix that.
>>
>> Of course if I had actually _looked carefully_ at your patch, I would
>> have seen that your test doesn't just check that we don't segfault, but
>> actually confirms that we find the entry.
>>
>> Sorry for the noise.
>
> Actually, I take it back again. Your test case doesn't have an embedded
> NUL in it (so we check that git finds it, but aside from the lack of
> segfault, stock git would already find it).
This reminds me ... despite the native cygwin regex library no longer
having the 'regex bug' (ie t0070.5 now passes), I still have NO_REGEX
set on cygwin. This is because, when building with the native library,
we have an "unexpected pass" for t7008.12, which looks like:
test_expect_failure 'git grep .fi a' '
git grep .fi a
'
[where the file a is set up earlier by: echo 'binaryQfile' | q_to_nul >a]
commit f96e5673 ("grep: use REG_STARTEND for all matching if available",
22-05-2010) introduced this test and expects ".. NUL characters themselves
are not matched in any way". With the native library on cygwin they are
matched, with the compat/regex they are not. Indeed, if you use the system
'grep' command (rather than 'git grep'), then it will also not match ... :-D
Slightly off topic, but ...
ATB,
Ramsay Jones
Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result
On 07/09/16 23:04, Jeff King wrote:
> All of our errors come from diff_get_patch_id(), which has
> exactly three error conditions. The first is an internal
> assertion, which should be a die("BUG") in the first place.
>
> The other two are caused by an inability to two diff blobs,
^
Huh? ... to diff two blobs?
ATB,
Ramsay Jones
Re: [PATCHv4] diff.c: emit moved lines with a different color
or_slot(const char *var)
> return DIFF_WHITESPACE;
> if (!strcasecmp(var, "func"))
> return DIFF_FUNCINFO;
> + if (!strcasecmp(var, "movedfrom"))
> + return DIFF_FILE_MOVED_FROM;
> + if (!strcasecmp(var, "movedto"))
> + return DIFF_FILE_MOVED_TO;
> return -1;
> }
>
> @@ -180,6 +188,10 @@ int git_diff_ui_config(const char *var, const char
> *value, void *cb)
> diff_use_color_default = git_config_colorbool(var, value);
> return 0;
> }
> + if (!strcmp(var, "color.moved")) {
> + diff_color_moved_default = git_config_bool(var, value);
> + return 0;
> + }
> if (!strcmp(var, "diff.context")) {
> diff_context_default = git_config_int(var, value);
> if (diff_context_default < 0)
> @@ -287,6 +299,26 @@ int git_diff_basic_config(const char *var, const char
> *value, void *cb)
> return git_default_config(var, value, cb);
> }
>
> +static int diff_line_moved_entry_cmp(const struct diff_line_moved_entry *a,
> + const struct diff_line_moved_entry *b,
> + const void *unused)
> +{
> + return strcmp(a->line, b->line) &&
> +a->hash_prev_line == b->hash_prev_line;
I doubt it would make much difference, but my knee-jerk reaction to
this was to suggest swapping the order of the expression, thus:
return a->hash_prev_line == b->hash_prev_line &&
strcmp(a->line, b->line);
... but perhaps it doesn't read quite so well, and probably wouldn't affect
performance much (except in strange edge cases), so it may not be worth it.
ATB,
Ramsay Jones
Re: [PATCH v13 10/14] apply: change error_routine when silent
On 04/09/16 11:54, Christian Couder wrote: > On Thu, Sep 1, 2016 at 10:19 AM, Christian Couder > <christian.cou...@gmail.com> wrote: >> On Thu, Sep 1, 2016 at 12:20 AM, Stefan Beller <sbel...@google.com> wrote: >>> On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder >>> <christian.cou...@gmail.com> wrote: >>>> To avoid printing anything when applying with >>>> `state->apply_verbosity == verbosity_silent`, let's save the >>>> existing warn and error routines before applying, and let's >>>> replace them with a routine that does nothing. >>>> >>>> Then after applying, let's restore the saved routines. >>>> >>>> Helped-by: Stefan Beller <sbel...@google.com> >>>> Signed-off-by: Christian Couder <chrisc...@tuxfamily.org> >>>> --- >>>> apply.c | 21 - >>>> apply.h | 8 >>>> 2 files changed, 28 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/apply.c b/apply.c >>>> index ddbb0a2..bf81b70 100644 >>>> --- a/apply.c >>>> +++ b/apply.c >>>> @@ -112,6 +112,11 @@ void clear_apply_state(struct apply_state *state) >>>> /* >fn_table is cleared at the end of apply_patch() */ >>>> } >>>> >>>> +static void mute_routine(const char *bla, va_list params) >>> >>> Instead of 'bla' you could go with 'format' as the man page for >>> [f]printf puts it. >>> Or you could leave it empty, i.e. >>> >>> static void mute_routine(const char *, va_list) >>> ... >> >> Ok to do that. > > Actually I get the following error when doing that: > > apply.c: In function ‘mute_routine’: > apply.c:115:1: error: parameter name omitted > static void mute_routine(const char *, va_list) > ^ > apply.c:115:1: error: parameter name omitted > make: *** [apply.o] Error 1 Yes, this is not C++. ;-) > So I will leave it as is. I think I would prefer to see: static void mute_routine(const char *msg, va_list params) given that it would either be an error-msg or a warning-msg. ATB, Ramsay Jones
[PATCH] diff: mark a file-local symbol static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Michael,
If you need to re-roll your 'mh/diff-indent-heuristic' branch, could
you please squash this into the relevant patch (commit 8f5cc189,
"diff: improve positioning of add/delete blocks in diffs", 22-08-2016).
Thanks!
ATB,
Ramsay Jones
xdiff/xdiffi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index dc79628..67c1ccc 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -684,7 +684,7 @@ static void score_add_split(const struct split_measurement
*m, struct split_scor
}
}
-int score_cmp(struct split_score *s1, struct split_score *s2)
+static int score_cmp(struct split_score *s1, struct split_score *s2)
{
/* -1 if s1.effective_indent < s2->effective_indent, etc. */
int cmp_indents = ((s1->effective_indent > s2->effective_indent) -
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] submodule: mark a file-local symbol as static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Stefan,
If you need to re-roll your 'sb/submodule-clone-rr' branch, could
you please squash this into the relevant patch (commit 7bcd1d17,
"clone: recursive and reference option triggers submodule alternates",
11-08-2016).
Thanks!
[What, deja vu? :-D]
ATB,
Ramsay Jones
builtin/submodule--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c617667..cb0b1d7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -483,7 +483,7 @@ struct submodule_alternate_setup {
#define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \
SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }
-int add_possible_reference_from_superproject(
+static int add_possible_reference_from_superproject(
struct alternate_object_database *alt, void *sas_cb)
{
struct submodule_alternate_setup *sas = sas_cb;
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] submodule: mark a file-local symbol as static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Stefan,
If you need to re-roll your 'sb/submodule-clone-rr' branch, could
you please squash this into the relevant patch (commit 336c21d,
"submodule: try alternates when superproject has an alternate",
08-08-2016).
Thanks!
ATB,
Ramsay Jones
builtin/submodule--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4c765e1..4c7d03c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -641,7 +641,7 @@ struct submodule_alternate_setup {
struct strbuf *out;
};
-int add_possible_reference(struct alternate_object_database *alt, void *sas_cb)
+static int add_possible_reference(struct alternate_object_database *alt, void
*sas_cb)
{
struct submodule_alternate_setup *sas = sas_cb;
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Forward declaration of enum iterator_selection?
On 08/08/16 19:28, Ramsay Jones wrote: > > > On 08/08/16 17:30, Johannes Sixt wrote: >> Am 07.08.2016 um 22:34 schrieb Ramsay Jones: >>> On 05/08/16 23:26, Johannes Sixt wrote: > [snip] >>> At this point 'enum iterator_selection' is an incomplete type and may >>> be used when the size of the object is not required. It is not needed, >>> for example, when a typedef name is being declared as a pointer to, or >>> as a function returning such a type. However, such a type must be >>> complete before such a function is called or defined. >> >> All you say is true when it is a struct type, of course. But I doubt that >> there exists such a thing called "incomplete enumeration type" in C. In >> fact, with these keywords I found >> https://gcc.gnu.org/onlinedocs/gcc/Incomplete-Enums.html which indicates >> that this is a GCC extension. > > Ah, well spotted! > > You prompted me to look at the C99 (and C11) standards, in particular > sections 6.7.2.2 (Enumeration specifiers) and 6.7.2.3 (Tags). > > So, while (technically) enumeration types are incomplete prior to the > closing } in its definition, the constraint imposed in 6.7.2.3-2 states: > > "A type specifier of the form > enum identifier > without an enumerator list shall only appear after > the type it specifies is complete" > > which pretty much rules out its use here. BTW, you can make gcc be that 'pickier compiler' you mentioned, thus: $ rm refs.o $ make CFLAGS='-g -O2 -Wall -std=c99 -pedantic' refs.o * new build flags CC refs.o In file included from refs.c:8:0: refs/refs-internal.h:363:14: warning: ISO C forbids forward references to ‘enum’ types [-Wpedantic] typedef enum iterator_selection ref_iterator_select_fn( ^ $ :-D ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Forward declaration of enum iterator_selection?
On 08/08/16 17:30, Johannes Sixt wrote: > Am 07.08.2016 um 22:34 schrieb Ramsay Jones: >> On 05/08/16 23:26, Johannes Sixt wrote: [snip] >> At this point 'enum iterator_selection' is an incomplete type and may >> be used when the size of the object is not required. It is not needed, >> for example, when a typedef name is being declared as a pointer to, or >> as a function returning such a type. However, such a type must be >> complete before such a function is called or defined. > > All you say is true when it is a struct type, of course. But I doubt that > there exists such a thing called "incomplete enumeration type" in C. In fact, > with these keywords I found > https://gcc.gnu.org/onlinedocs/gcc/Incomplete-Enums.html which indicates that > this is a GCC extension. Ah, well spotted! You prompted me to look at the C99 (and C11) standards, in particular sections 6.7.2.2 (Enumeration specifiers) and 6.7.2.3 (Tags). So, while (technically) enumeration types are incomplete prior to the closing } in its definition, the constraint imposed in 6.7.2.3-2 states: "A type specifier of the form enum identifier without an enumerator list shall only appear after the type it specifies is complete" which pretty much rules out its use here. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Forward declaration of enum iterator_selection?
On 05/08/16 23:26, Johannes Sixt wrote: > When refs.c is being compiled, the only mention of enum iterator_selection is > in this piece of code pulled in from refs-internal.h (have a look at the > preprocessed code): > > typedef enum iterator_selection ref_iterator_select_fn( > struct ref_iterator *iter0, struct ref_iterator *iter1, > void *cb_data); > > This looks like a forward declarations of an enumeration type name, something > that I thought is illegal in C. Am I wrong? (That may well be the case, my > C-foo is quite rusty.) At this point 'enum iterator_selection' is an incomplete type and may be used when the size of the object is not required. It is not needed, for example, when a typedef name is being declared as a pointer to, or as a function returning such a type. However, such a type must be complete before such a function is called or defined. > My compiler does not complain (it's gcc 4.8), but I thought I mention it > before someone with a pickier compiler stumbles over it... So, I think this is correct. Having said that, I would rather the 'enum iterator_selection' be defined before this declaration. One solution could be to #include "iterator.h" prior to _all_ #include "refs/refs-internal.h" in all compilation units (Note it is in the opposite order in refs/iterator.c). Alternatively, you could put the #include "../iterator.h" into refs/refs-internal.h directly (some people would object to this). ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] xdiffi.c: mark some file-local symbols as static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Michael,
If you need to re-roll your 'mh/diff-indent-heuristic' branch, could
you please squash this into the relevant patch (e199b6e2, "diff: improve
positioning of add/delete blocks in diffs", 04-08-2016).
Thanks!
ATB,
Ramsay Jones
xdiff/xdiffi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 80307f5..90226f8 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -499,7 +499,7 @@ struct split_measurement {
/*
* Fill m with information about a hypothetical split of xdf above line split.
*/
-void measure_split(const xdfile_t *xdf, long split, struct split_measurement
*m)
+static void measure_split(const xdfile_t *xdf, long split, struct
split_measurement *m)
{
long i;
@@ -557,7 +557,7 @@ void measure_split(const xdfile_t *xdf, long split, struct
split_measurement *m)
* Also see that project if you want to improve the weights based on, for
example,
* a larger or more diverse corpus.
*/
-int score_split(const struct split_measurement *m)
+static int score_split(const struct split_measurement *m)
{
/*
* A place to accumulate bonus factors (positive makes this index more
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] wt-status.c: mark a file-local symbol as static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Jeff,
If you need to re-roll your 'jh/status-v2-porcelain' branch, could
you please squash this into the relevant patch (37f7104f, "status:
print per-file porcelain v2 status data", 02-08-2016).
Thanks!
ATB,
Ramsay Jones
wt-status.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/wt-status.c b/wt-status.c
index 3396bf5..a80400e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2152,7 +2152,7 @@ static void wt_porcelain_v2_print_other(
* []*
*
*/
-void wt_porcelain_v2_print(struct wt_status *s)
+static void wt_porcelain_v2_print(struct wt_status *s)
{
struct wt_status_change_data *d;
struct string_list_item *it;
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] apply: mark some file-local symbols static
On 03/08/16 10:47, Christian Couder wrote:
> Hi Ramsay,
>
> On Wed, Aug 3, 2016 at 12:44 AM, Junio C Hamano <gits...@pobox.com> wrote:
>> On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones
>> <ram...@ramsayjones.plus.com> wrote:
>>>
>>> Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
>>> ---
>>>
>>> Hi Christian,
>>>
[snip]
>>> What am I missing?
>
> These symbols are still used in builtin/apply.c until 9f87c22 ("apply:
> refactor `git apply` option parsing") at the end of the series, for
> example:
>
> $ git checkout 4d18b33a
> $ git grep -n apply_option_parse_directory builtin/apply.c
> builtin/apply.c:86: 0, apply_option_parse_directory },
>
Heh, thanks. I thought I had done exactly this, but I obviously
messed up!
[snip]
> Yeah, I did not notice that they no longer need to be extern.
> Now there are different options to fix this:
>
> 1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option
> parsing") at the end of the series, or
> 2) move 4820e13 (apply: make some parsing functions static again) at
> the end of the series and make it also remove them, or:
> 3) add another patch to remove them after 9f87c22 ("apply: refactor
> `git apply` option parsing")
>
> My preference is to do 1). This way, or if I do 3), I would not need
> to resend the first 31 patches in the series.
FWIW, I would go with option #1.
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] apply: mark some file-local symbols static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Christian,
I had intended to ask you to squash this into your 'cc/apply-am'
branch, specifically commit 4d18b33a (apply: move libified code
from builtin/apply.c to apply.{c,h}, 30-07-2016).
However, having read that commit a little closer, it seems that
you deliberately made these symbols public. The commit message
does not mention this issue at all, and it is not clear to me
why these symbols should be public.
What am I missing?
ATB,
Ramsay Jones
apply.c | 26 +-
apply.h | 14 --
2 files changed, 13 insertions(+), 27 deletions(-)
diff --git a/apply.c b/apply.c
index 96f02fa..ec545f6 100644
--- a/apply.c
+++ b/apply.c
@@ -4743,16 +4743,16 @@ static int apply_patch(struct apply_state *state,
return res;
}
-int apply_option_parse_exclude(const struct option *opt,
- const char *arg, int unset)
+static int apply_option_parse_exclude(const struct option *opt,
+ const char *arg, int unset)
{
struct apply_state *state = opt->value;
add_name_limit(state, arg, 1);
return 0;
}
-int apply_option_parse_include(const struct option *opt,
- const char *arg, int unset)
+static int apply_option_parse_include(const struct option *opt,
+ const char *arg, int unset)
{
struct apply_state *state = opt->value;
add_name_limit(state, arg, 0);
@@ -4760,9 +4760,9 @@ int apply_option_parse_include(const struct option *opt,
return 0;
}
-int apply_option_parse_p(const struct option *opt,
-const char *arg,
-int unset)
+static int apply_option_parse_p(const struct option *opt,
+ const char *arg,
+ int unset)
{
struct apply_state *state = opt->value;
state->p_value = atoi(arg);
@@ -4770,8 +4770,8 @@ int apply_option_parse_p(const struct option *opt,
return 0;
}
-int apply_option_parse_space_change(const struct option *opt,
- const char *arg, int unset)
+static int apply_option_parse_space_change(const struct option *opt,
+ const char *arg, int unset)
{
struct apply_state *state = opt->value;
if (unset)
@@ -4781,8 +4781,8 @@ int apply_option_parse_space_change(const struct option
*opt,
return 0;
}
-int apply_option_parse_whitespace(const struct option *opt,
- const char *arg, int unset)
+static int apply_option_parse_whitespace(const struct option *opt,
+const char *arg, int unset)
{
struct apply_state *state = opt->value;
state->whitespace_option = arg;
@@ -4791,8 +4791,8 @@ int apply_option_parse_whitespace(const struct option
*opt,
return 0;
}
-int apply_option_parse_directory(const struct option *opt,
-const char *arg, int unset)
+static int apply_option_parse_directory(const struct option *opt,
+ const char *arg, int unset)
{
struct apply_state *state = opt->value;
strbuf_reset(>root);
diff --git a/apply.h b/apply.h
index 66ed0c5..010726e 100644
--- a/apply.h
+++ b/apply.h
@@ -107,20 +107,6 @@ struct apply_state {
int applied_after_fixing_ws;
};
-extern int apply_option_parse_exclude(const struct option *opt,
- const char *arg, int unset);
-extern int apply_option_parse_include(const struct option *opt,
- const char *arg, int unset);
-extern int apply_option_parse_p(const struct option *opt,
- const char *arg,
- int unset);
-extern int apply_option_parse_whitespace(const struct option *opt,
-const char *arg, int unset);
-extern int apply_option_parse_directory(const struct option *opt,
- const char *arg, int unset);
-extern int apply_option_parse_space_change(const struct option *opt,
- const char *arg, int unset);
-
extern int apply_parse_options(int argc, const char **argv,
struct apply_state *state,
int *force_apply, int *options,
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] convert: add filter..useProtocol option
On 24/07/16 18:16, Lars Schneider wrote:
>
> On 23 Jul 2016, at 01:19, Ramsay Jones <ram...@ramsayjones.plus.com> wrote:
>
>> On 22/07/16 16:49, larsxschnei...@gmail.com wrote:
>>> From: Lars Schneider <larsxschnei...@gmail.com>
>>>
>>> Git's clean/smudge mechanism invokes an external filter process for every
>>> single blob that is affected by a filter. If Git filters a lot of blobs
>>> then the startup time of the external filter processes can become a
>>> significant part of the overall Git execution time.
>>>
>>> This patch adds the filter..useProtocol option which, if enabled,
>>> keeps the external filter process running and processes all blobs with
>>> the following protocol over stdin/stdout.
>>>
>>> 1. Git starts the filter on first usage and expects a welcome message
>>> with protocol version number:
>>> Git <-- Filter: "git-filter-protocol\n"
>>> Git <-- Filter: "version 1"
>>
>> Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
>> interaction is fully defined, I guess it doesn't matter).
>>
>> [If you wanted to check for a version, you could add a "version" command
>> instead, just like "clean" and "smudge".]
>
> It was a conscious decision to have the `filter` talk first. My reasoning was:
>
> (1) I want a reliable way to distinguish the existing filter protocol
> ("single-shot
> invocation") from the new one ("long running"). I don't think there would be a
> situation where the existing protocol would talk first. Therefore the users
> would
> not accidentally mix them with a possibly half working, undetermined, outcome.
If an 'single-shot' filter were incorrectly configured, instead of a new one,
then
the interaction could last a little while - since it would result in deadlock!
;-)
[If Git talks first instead, configuring a 'single-shot' filter _may_ still
result
in a deadlock - depending on pipe size, etc.]
>
> (2) In the future we could extend the pipe protocol (see $gmane/297994, it's
> very
> interesting). A filter could check Git's version and then pick the most
> appropriate
> filter protocol on startup.
>
>
>> [...]
>>> +static struct cmd2process *start_protocol_filter(const char *cmd)
>>> +{
>>> + int ret = 1;
>>> + struct cmd2process *entry = NULL;
>>> + struct child_process *process = NULL;
>>> + struct strbuf nbuf = STRBUF_INIT;
>>> + struct string_list split = STRING_LIST_INIT_NODUP;
>>> + const char *argv[] = { NULL, NULL };
>>> + const char *header = "git-filter-protocol\nversion";
>>> +
>>> + entry = xmalloc(sizeof(*entry));
>>> + hashmap_entry_init(entry, strhash(cmd));
>>> + entry->cmd = cmd;
>>> + process = >process;
>>> +
>>> + child_process_init(process);
>>> + argv[0] = cmd;
>>> + process->argv = argv;
>>> + process->use_shell = 1;
>>> + process->in = -1;
>>> + process->out = -1;
>>> +
>>> + if (start_command(process)) {
>>> + error("cannot fork to run external persistent filter '%s'",
>>> cmd);
>>> + return NULL;
>>> + }
>>> + strbuf_reset();
>>> +
>>> + sigchain_push(SIGPIPE, SIG_IGN);
>>> + ret &= strbuf_read_once(, process->out, 0) > 0;
>>
>> Hmm, how much will be read into nbuf by this single call?
>> Since strbuf_read_once() makes a single call to xread(), with
>> a len argument that will probably be 8192, you can not really
>> tell how much it will read, in general. (xread() does not
>> guarantee how many bytes it will read.)
>>
>> In particular, it could be less than strlen(header).
>
> As mentioned to Torsten in $gmane/300156, I will add a newline
> and then read until I find the second newline. That should solve
> the problem, right?
>
> (You wrote in $gmane/300119 that I should ignore your email but
> I think you have a valid point here ;-)
Heh, as I said, it was late and I was trying to do several things
at once. (I am updating 3 installations of Linux Mint 17.3 to Linux
Mint 18 - I decided to do a complete re-install, since I needed to
change partition sizes anyway. I have only just got email back up ...)
I stopped commenting on the patch early but, after sending the first
email, I decided to scan the rest of your patch before going to bed
and noticed something which would invalidate my comments ...
>
>
>>
Re: [PATCH v1 3/3] convert: add filter..useProtocol option
Hi Lars,
On 23/07/16 00:19, Ramsay Jones wrote:
>
>
> On 22/07/16 16:49, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>>
>> Git's clean/smudge mechanism invokes an external filter process for every
>> single blob that is affected by a filter. If Git filters a lot of blobs
>> then the startup time of the external filter processes can become a
>> significant part of the overall Git execution time.
>>
>> This patch adds the filter..useProtocol option which, if enabled,
>> keeps the external filter process running and processes all blobs with
>> the following protocol over stdin/stdout.
>>
>> 1. Git starts the filter on first usage and expects a welcome message
>> with protocol version number:
>> Git <-- Filter: "git-filter-protocol\n"
>> Git <-- Filter: "version 1"
>
> Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
> interaction is fully defined, I guess it doesn't matter).
>
> [If you wanted to check for a version, you could add a "version" command
> instead, just like "clean" and "smudge".]
>
> [snip]
>
>> diff --git a/convert.c b/convert.c
>> index 522e2c5..91ce86f 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -481,12 +481,188 @@ static int apply_filter(const char *path, const char
>> *src, size_t len, int fd,
>> return ret;
>> }
>>
>> +static int cmd_process_map_init = 0;
>> +static struct hashmap cmd_process_map;
>> +
>> +struct cmd2process {
>> +struct hashmap_entry ent; /* must be the first member! */
>> +const char *cmd;
>> +long protocol;
>> +struct child_process process;
>> +};
>> +
>> +static int cmd2process_cmp(const struct cmd2process *e1,
>> +const struct
>> cmd2process *e2,
>> +const void *unused)
>> +{
>> +return strcmp(e1->cmd, e2->cmd);
>> +}
>> +
>> +static struct cmd2process *find_protocol_filter_entry(const char *cmd)
>> +{
>> +struct cmd2process k;
>> +hashmap_entry_init(, strhash(cmd));
>> +k.cmd = cmd;
>> +return hashmap_get(_process_map, , NULL);
>> +}
>> +
>> +static void stop_protocol_filter(struct cmd2process *entry) {
>> +if (!entry)
>> +return;
>> +sigchain_push(SIGPIPE, SIG_IGN);
>> +close(entry->process.in);
>> +close(entry->process.out);
>> +sigchain_pop(SIGPIPE);
>> +finish_command(>process);
>> +child_process_clear(>process);
>> +hashmap_remove(_process_map, entry, NULL);
>> +free(entry);
>> +}
>> +
>> +static struct cmd2process *start_protocol_filter(const char *cmd)
>> +{
>> +int ret = 1;
>> +struct cmd2process *entry = NULL;
>> +struct child_process *process = NULL;
>> +struct strbuf nbuf = STRBUF_INIT;
>> +struct string_list split = STRING_LIST_INIT_NODUP;
>> +const char *argv[] = { NULL, NULL };
>> +const char *header = "git-filter-protocol\nversion";
>> +
>> +entry = xmalloc(sizeof(*entry));
>> +hashmap_entry_init(entry, strhash(cmd));
>> +entry->cmd = cmd;
>> +process = >process;
>> +
>> +child_process_init(process);
>> +argv[0] = cmd;
>> +process->argv = argv;
>> +process->use_shell = 1;
>> +process->in = -1;
>> +process->out = -1;
>> +
>> +if (start_command(process)) {
>> +error("cannot fork to run external persistent filter '%s'",
>> cmd);
>> +return NULL;
>> +}
>> +strbuf_reset();
>> +
>> +sigchain_push(SIGPIPE, SIG_IGN);
>> +ret &= strbuf_read_once(, process->out, 0) > 0;
>
> Hmm, how much will be read into nbuf by this single call?
> Since strbuf_read_once() makes a single call to xread(), with
> a len argument that will probably be 8192, you can not really
> tell how much it will read, in general. (xread() does not
> guarantee how many bytes it will read.)
>
> In particular, it could be less than strlen(header).
Please ignore this email, it's late ... ;-)
Sorry for the noise.
[Off to bed now]
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] convert: add filter..useProtocol option
}
> + }
> + string_list_clear(, 0);
> + strbuf_release();
> +
> + if (!ret) {
> + error("initialization for external persistent filter '%s'
> failed", cmd);
> + return NULL;
> + }
> +
> + hashmap_add(_process_map, entry);
> + return entry;
> +}
> +
> +static int apply_protocol_filter(const char *path, const char *src, size_t
> len,
> + int fd, struct strbuf *dst,
> const char *cmd,
> + const char *filter_type)
> +{
> + int ret = 1;
> + struct cmd2process *entry = NULL;
> + struct child_process *process = NULL;
> + struct stat fileStat;
> + struct strbuf nbuf = STRBUF_INIT;
> + size_t nbuf_len;
> + char *strtol_end;
> + char c;
> +
> + if (!cmd || !*cmd)
> + return 0;
> +
> + if (!dst)
> + return 1;
> +
> + if (!cmd_process_map_init) {
> + cmd_process_map_init = 1;
> + hashmap_init(_process_map, (hashmap_cmp_fn)
> cmd2process_cmp, 0);
> + } else {
> + entry = find_protocol_filter_entry(cmd);
> + }
> +
> + if (!entry){
> + entry = start_protocol_filter(cmd);
> + if (!entry) {
> + stop_protocol_filter(entry);
> + return 0;
> + }
> + }
> + process = >process;
> +
> + sigchain_push(SIGPIPE, SIG_IGN);
> + switch (entry->protocol) {
> + case 1:
> + if (fd >= 0 && !src) {
> + ret &= fstat(fd, ) != -1;
> + len = fileStat.st_size;
> + }
> + strbuf_reset();
> + strbuf_addf(, "%s\n%s\n%zu\n", filter_type, path,
> len);
> + ret &= write_str_in_full(process->in, nbuf.buf) > 1;
why not write_in_full(process->in, nbuf.buf, nbuf.len) ?
> + if (len > 0) {
> + if (src)
> + ret &= write_in_full(process->in, src,
> len) == len;
> + else if (fd >= 0)
> + ret &= copy_fd(fd, process->in) == 0;
> + else
> + ret &= 0;
> + }
> +
> + strbuf_reset();
> + while (xread(process->out, , 1) == 1 && c != '\n')
> + strbuf_addchars(, c, 1);
> + nbuf_len = (size_t)strtol(nbuf.buf, _end, 10);
> + ret &= (strtol_end != nbuf.buf && errno != ERANGE);
> + strbuf_reset();
> + if (nbuf_len > 0)
> + ret &= strbuf_read_once(, process->out,
> nbuf_len) == nbuf_len;
Again, how many bytes will be read?
Note, that in the default configuration, a _maximum_ of
MAX_IO_SIZE (8MB or SSIZE_MAX, whichever is smaller) bytes
will be read.
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] builtin/apply: include declaration of cmd_apply()
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Christian,
If you need to re-roll your 'cc/apply-am' branch, could you please
squash this into the relevant patch. Commit 95a3b0ba ("apply: move
libified code from builtin/apply.c to apply.{c,h}", 22-04-2016)
removed this '#include "builtin.h"' line, which causes sparse to
issue a warning.
Thanks!
ATB,
Ramsay Jones
builtin/apply.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/apply.c b/builtin/apply.c
index 066cb29..9c66474 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "builtin.h"
#include "parse-options.h"
#include "lockfile.h"
#include "apply.h"
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #10; Wed, 29)
On 30/06/16 00:22, Junio C Hamano wrote:
[snip]
>
> * mh/ref-store (2016-06-20) 38 commits
> - refs: implement iteration over only per-worktree refs
> - refs: make lock generic
> - refs: add method to rename refs
> - refs: add methods to init refs db
> - refs: make delete_refs() virtual
> - refs: add method for initial ref transaction commit
> - refs: add methods for reflog
> - refs: add method iterator_begin
> - files_ref_iterator_begin(): take a ref_store argument
> - split_symref_update(): add a files_ref_store argument
> - lock_ref_sha1_basic(): add a files_ref_store argument
> - lock_ref_for_update(): add a files_ref_store argument
> - commit_ref_update(): add a files_ref_store argument
> - lock_raw_ref(): add a files_ref_store argument
> - repack_without_refs(): add a files_ref_store argument
> - refs: make peel_ref() virtual
> - refs: make create_symref() virtual
> - refs: make pack_refs() virtual
> - refs: make verify_refname_available() virtual
> - refs: make read_raw_ref() virtual
> - resolve_gitlink_ref(): rename path parameter to submodule
> - resolve_gitlink_ref(): avoid memory allocation in many cases
> - resolve_gitlink_ref(): implement using resolve_ref_recursively()
> - resolve_ref_recursively(): new function
> - read_raw_ref(): take a (struct ref_store *) argument
> - resolve_gitlink_packed_ref(): remove function
> - resolve_packed_ref(): rename function from resolve_missing_loose_ref()
> - refs: reorder definitions
> - refs: add a transaction_commit() method
> - {lock,commit,rollback}_packed_refs(): add files_ref_store arguments
> - resolve_missing_loose_ref(): add a files_ref_store argument
> - get_packed_ref(): add a files_ref_store argument
> - add_packed_ref(): add a files_ref_store argument
> - refs: create a base class "ref_store" for files_ref_store
> - refs: add a backend method structure
> - refs: rename struct ref_cache to files_ref_store
> - rename_ref_available(): add docstring
> - resolve_gitlink_ref(): eliminate temporary variable
> (this branch uses mh/ref-iterators and mh/split-under-lock; is tangled with
> mh/update-ref-errors.)
>
> The ref-store abstraction was introduced to the refs API so that we
> can plug in different backends to store references.
>
> Is everybody happy with this version?
> If so, will merge to 'next'.
A small fixup required for this one.
see http://thread.gmane.org/gmane.comp.version-control.git/298137
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] refs/files-backend: mark a file-local symbol static
Commit 6d41edc6 ("refs: add methods for reflog", 24-02-2016), moved the
reflog handling into the ref-storage backend. In particular, the
files_reflog_iterator_begin() API was removed from internal refs API
header, resulting in sparse warnings.
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Michael,
If you need to re-roll your 'mh/ref-store' branch, could you please
squash this into the relevant patch. (or something like it - if you
think this function should be public, then re-introduce the declaration
to the header).
Thanks!
ATB,
Ramsay Jones
refs/files-backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b6ce19f..426e439 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3331,7 +3331,7 @@ static struct ref_iterator_vtable
files_reflog_iterator_vtable = {
files_reflog_iterator_abort
};
-struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
+static struct ref_iterator *files_reflog_iterator_begin(struct ref_store
*ref_store)
{
struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
struct ref_iterator *ref_iterator = >base;
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] archive-tar: add UL type suffix to unsigned long constant
On 18/06/16 00:40, Jeff King wrote:
> On Fri, Jun 17, 2016 at 10:06:14PM +0100, Ramsay Jones wrote:
>
>> If you need to re-roll your 'jk/big-and-old-archive-tar' branch, could
>> you please squash this into the relevant patch (commit 8035a1e3,
>> "archive-tar: write extended headers for far-future mtime", 16-06-2016).
>>
>> Thanks!
>>
>> ATB,
>> Ramsay Jones
>>
>> archive-tar.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/archive-tar.c b/archive-tar.c
>> index 749722f..c7b85fd 100644
>> --- a/archive-tar.c
>> +++ b/archive-tar.c
>> @@ -187,7 +187,7 @@ static inline unsigned long ustar_size(uintmax_t size)
>>
>> static inline unsigned long ustar_mtime(time_t mtime)
>> {
>> -if (mtime < 0777)
>> +if (mtime < 0777UL)
>
> Whoops. I even got it right in the similar line above. :-/
>
> This did make me wonder how the whole thing fares on a system where
> "unsigned long" is 32-bit (AIUI, Git for Windows is such a system).
>
> The sizes passed around (here and all through git) are "unsigned long",
> so I don't think we're making anything _worse_.
Heh, I had exactly the same thought! ;-)
I have a TODO item that reads: check odb code for object size limitations
imposed by using 'unsigned long'. This is a known problem on Git for Windows.
[Not for cygwin, however, since long is 64bit (just like linux). The win32api
headers on cygwin have been written in terms of a __LONG32 #define to allow
them to be used on LLP64 and LP64 systems.]
However, since I don't have GFW installed, I couldn't actually test it, so ...
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] run-command: mark file-local symbols static
On 18/06/16 00:33, Jeff King wrote:
> On Fri, Jun 17, 2016 at 10:01:24PM +0100, Ramsay Jones wrote:
>
>> If you need to re-roll your 'jk/gpg-interface-cleanup' branch, could
>> you please squash this into the relevant patch (commit 74287e34,
>> "run-command: add pipe_command helper", 16-06-2016).
>
> Thanks, yes.
>
>> BTW, also on that branch, commit 6fec0a89 ("verify_signed_buffer: use
>> tempfile object", 16-06-2016) removes the last use of the git_mkstemp()
>> function. Should it be removed?
>
> I think so. We still have git_mkstemp_mode and friends, so in that sense
> this is part of a family of commands that somebody might use again. But:
>
> 1. Unlike the others in the family, where we implement mkstemp
> ourselves, this one uses the system mkstemp. Which probably behaves
> in totally the same way, but it's kind of weird and oddball.
>
> 2. I think we should be steering people towards tempfile.c anyway, for
> its auto-cleanup properties.
>
> Want to do a patch on top?
OK, will do. (tomorrow, it is 2am here ...)
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] archive-tar: add UL type suffix to unsigned long constant
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Jeff,
If you need to re-roll your 'jk/big-and-old-archive-tar' branch, could
you please squash this into the relevant patch (commit 8035a1e3,
"archive-tar: write extended headers for far-future mtime", 16-06-2016).
Thanks!
ATB,
Ramsay Jones
archive-tar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/archive-tar.c b/archive-tar.c
index 749722f..c7b85fd 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -187,7 +187,7 @@ static inline unsigned long ustar_size(uintmax_t size)
static inline unsigned long ustar_mtime(time_t mtime)
{
- if (mtime < 0777)
+ if (mtime < 0777UL)
return mtime;
else
return 0;
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] run-command: mark file-local symbols static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Jeff,
If you need to re-roll your 'jk/gpg-interface-cleanup' branch, could
you please squash this into the relevant patch (commit 74287e34,
"run-command: add pipe_command helper", 16-06-2016).
BTW, also on that branch, commit 6fec0a89 ("verify_signed_buffer: use
tempfile object", 16-06-2016) removes the last use of the git_mkstemp()
function. Should it be removed?
Thanks!
ATB,
Ramsay Jones
run-command.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/run-command.c b/run-command.c
index 609fa4c..33bc63a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -886,7 +886,7 @@ struct io_pump {
struct pollfd *pfd;
};
-int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
+static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
{
int pollsize = 0;
int i;
@@ -950,7 +950,7 @@ int pump_io_round(struct io_pump *slots, int nr, struct
pollfd *pfd)
return 1;
}
-int pump_io(struct io_pump *slots, int nr)
+static int pump_io(struct io_pump *slots, int nr)
{
struct pollfd *pfd;
int i;
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] write_or_die: remove the unused write_or_whine() function
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Junio,
Commit f0bca72d ("send-pack: use buffered I/O to talk to pack-objects",
08-06-2016) removed the last use of write_or_whine(). I had intended to
include this 'commit citation' in the commit message, but until it reaches
the next branch, I don't know how stable that information will be.
Anyway, on reflection, the subject line says everything that needs to be
said. However, you need to know which commit this one depends on. ;-)
ATB,
Ramsay Jones
cache.h| 1 -
write_or_die.c | 11 ---
2 files changed, 12 deletions(-)
diff --git a/cache.h b/cache.h
index 84d8812..258f154 100644
--- a/cache.h
+++ b/cache.h
@@ -1767,7 +1767,6 @@ extern int copy_file(const char *dst, const char *src,
int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
extern void write_or_die(int fd, const void *buf, size_t count);
-extern int write_or_whine(int fd, const void *buf, size_t count, const char
*msg);
extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const
char *msg);
extern void fsync_or_die(int fd, const char *);
diff --git a/write_or_die.c b/write_or_die.c
index 49e80aa..9816879 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -94,14 +94,3 @@ int write_or_whine_pipe(int fd, const void *buf, size_t
count, const char *msg)
return 1;
}
-
-int write_or_whine(int fd, const void *buf, size_t count, const char *msg)
-{
- if (write_in_full(fd, buf, count) < 0) {
- fprintf(stderr, "%s: write error (%s)\n",
- msg, strerror(errno));
- return 0;
- }
-
- return 1;
-}
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects
On 09/06/16 18:12, Jeff King wrote: > On Thu, Jun 09, 2016 at 03:34:59PM +0100, Ramsay Jones wrote: > >> Just FYI, this patch removes the last use of write_or_whine() - should it >> be removed? > > That sounds reasonable. Want to do a patch on top? OK, will do. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects
On 09/06/16 13:10, Matthieu Moy wrote:
> Jeff King <p...@peff.net> writes:
>
>> --- a/send-pack.c
>> +++ b/send-pack.c
>> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,
>> die("bad %s argument: %s", opt->long_name, arg);
>> }
>>
>> -static int feed_object(const unsigned char *sha1, int fd, int negative)
>> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)
>> {
>> -char buf[42];
>> -
>> if (negative && !has_sha1_file(sha1))
>> -return 1;
>> +return;
> [...]
>> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct
>> sha1_array *extra, stru
> [...]
>> for (i = 0; i < extra->nr; i++)
>> -if (!feed_object(extra->sha1[i], po.in, 1))
>> -break;
>> +feed_object(extra->sha1[i], po_in, 1);
>
> I may have missed the obvious, but doesn't this change the behavior when
> "negative && !has_sha1_file(sha1)" happens? I understand that you don't
> need write_or_whine anymore, but don't understand how you get rid of the
> "return 1" here.
Just FYI, this patch removes the last use of write_or_whine() - should it
be removed?
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] refs: mark the file-local vtable symbols as static
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Michael, Junio,
I would normally ask you to squash this into the relevant patch when
you next re-roll your 'mh/ref-iterators' branch, but this has already
been merged into next. (I normally have a bit more time ... sorry!).
Perhaps, after the release, when the next branch is re-wound/re-built,
this could be squashed into your branch then.
Anyway, after applying this patch, the following symbols are still
'public but unused':
> refs/files-backend.o - files_reflog_iterator_begin
> refs/iterator.o - is_empty_ref_iterator
> refs/iterator.o - merge_ref_iterator_begin
These all look (potentially) useful for the implementation of
additional 'ref-iter' types and look to be part of the _internal_
iterator API - so they should not be marked static. Can you just
confirm my interpretation.
Thanks.
ATB,
Ramsay Jones
refs/files-backend.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a006a65..6213891 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -711,7 +711,7 @@ static int cache_ref_iterator_abort(struct ref_iterator
*ref_iterator)
return ITER_DONE;
}
-struct ref_iterator_vtable cache_ref_iterator_vtable = {
+static struct ref_iterator_vtable cache_ref_iterator_vtable = {
cache_ref_iterator_advance,
cache_ref_iterator_peel,
cache_ref_iterator_abort
@@ -1933,7 +1933,7 @@ static int files_ref_iterator_abort(struct ref_iterator
*ref_iterator)
return ok;
}
-struct ref_iterator_vtable files_ref_iterator_vtable = {
+static struct ref_iterator_vtable files_ref_iterator_vtable = {
files_ref_iterator_advance,
files_ref_iterator_peel,
files_ref_iterator_abort
@@ -3354,7 +3354,7 @@ static int files_reflog_iterator_abort(struct
ref_iterator *ref_iterator)
return ok;
}
-struct ref_iterator_vtable files_reflog_iterator_vtable = {
+static struct ref_iterator_vtable files_reflog_iterator_vtable = {
files_reflog_iterator_advance,
files_reflog_iterator_peel,
files_reflog_iterator_abort
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] regex: fix a SIZE_MAX macro redefinition warning
Since commit 56a1a3ab ("Silence GCC's \"cast of pointer to integer of a
different size\" warning", 26-10-2015), sparse has been issuing a macro
redefinition warning for the SIZE_MAX macro. However, gcc did not issue
any such warning.
After commit 56a1a3ab, in terms of the order of #includes and #defines,
the code looked something like:
$ cat -n junk.c
1#include
2
3#define SIZE_MAX ((size_t) -1)
4
5#include
6
7int main(int argc, char *argv[])
8{
9return 0;
10}
$
$ gcc junk.c
$
However, if you compile that file with -Wsystem-headers, then it will
also issue a warning. Having set -Wsystem-headers in CFLAGS, using the
config.mak file, then (on cygwin):
$ make compat/regex/regex.o
CC compat/regex/regex.o
In file included from
/usr/lib/gcc/x86_64-pc-cygwin/4.9.3/include/stdint.h:9:0,
from compat/regex/regcomp.c:21,
from compat/regex/regex.c:77:
/usr/include/stdint.h:362:0: warning: "SIZE_MAX" redefined
#define SIZE_MAX (__SIZE_MAX__)
^
In file included from compat/regex/regex.c:69:0:
compat/regex/regex_internal.h:108:0: note: this is the location of the
previous definition
# define SIZE_MAX ((size_t) -1)
^
$
The compilation of the compat/regex code is somewhat unusual in that the
regex.c file directly #includes the other c files (regcomp.c, regexec.c
and regex_internal.c). Commit 56a1a3ab added an #include of
to the regcomp.c file, which results in the redefinition, since this is
included after the regex_internal.h header. This header file contains a
'fallback' definition for SIZE_MAX, in order to support systems which do
not have the header (the HAVE_STDINT_H macro is not defined).
In order to suppress the warning, we move the #include of
from regcomp.c to the start of the compilation unit, close to the top
of regex.c, prior to the #include of the regex_internal.h header.
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Change from v1:
- add the #include to the top of regex.c, rather than
use HAVE_STDINT_H to 'activate' the conditional inclusion of
in the regex_internal.h header.
Junio and Johannes - thanks for the feedback on v1 of the patch.
ATB,
Ramsay Jones
compat/regex/regcomp.c | 2 --
compat/regex/regex.c | 1 +
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index fba5986..d8bde06 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -18,8 +18,6 @@
Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301 USA. */
-#include
-
static reg_errcode_t re_compile_internal (regex_t *preg, const char * pattern,
size_t length, reg_syntax_t syntax);
static void re_compile_fastmap_iter (regex_t *bufp,
diff --git a/compat/regex/regex.c b/compat/regex/regex.c
index 6aaae00..5cb23e5 100644
--- a/compat/regex/regex.c
+++ b/compat/regex/regex.c
@@ -60,6 +60,7 @@
GNU regex allows. Include it before , which correctly
#undefs RE_DUP_MAX and sets it to the right value. */
#include
+#include
#ifdef GAWK
#undef alloca
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] regex: fix a SIZE_MAX macro redefinition warning
On 05/06/16 08:15, Johannes Schindelin wrote: > Hi Ramsay, > > thanks for working on this! > > On Sat, 4 Jun 2016, Ramsay Jones wrote: > >> diff --git a/Makefile b/Makefile >> index 0d59718..3f6c70a 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -1987,7 +1987,7 @@ endif >> >> ifdef NO_REGEX >> compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \ >> --DGAWK -DNO_MBSUPPORT >> +-DGAWK -DNO_MBSUPPORT -DHAVE_STDINT_H > > Maybe a comment here, something like "the fallback regex implementation > *requires* stdint.h"? The original version of this patch looked like this: diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c index fba5986..d8bde06 100644 --- a/compat/regex/regcomp.c +++ b/compat/regex/regcomp.c @@ -18,8 +18,6 @@ Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ -#include - static reg_errcode_t re_compile_internal (regex_t *preg, const char * pattern, size_t length, reg_syntax_t syntax); static void re_compile_fastmap_iter (regex_t *bufp, diff --git a/compat/regex/regex.c b/compat/regex/regex.c index 6aaae00..5cb23e5 100644 --- a/compat/regex/regex.c +++ b/compat/regex/regex.c @@ -60,6 +60,7 @@ GNU regex allows. Include it before , which correctly #undefs RE_DUP_MAX and sets it to the right value. */ #include +#include #ifdef GAWK #undef alloca So, just move the unconditional inclusion to the start of the compilation unit root file, before the #include of the regex_internal.h header. In some ways this is a better fix, because it makes it clear that, currently, the compat/regex code requires . This would remove the need for such a comment. This effectively makes the conditional inclusion of , and the SIZE_MAX fallback, in regex_internal.h dead code. (The C99 standard _requires_ the definition of SIZE_MAX in , thankfully! ;-). So, I was tempted to remove them as part of the patch. However, I also wanted to minimize the changes to the regex code, just in case we ever wanted to re-import a newer version from upstream. Setting HAVE_STDINT_H seemed like a good solution, but maybe the first patch would be more honest? As I said earlier, I was a little concerned about the 'unconditional' aspect of the inclusion of . At one time we wanted to support systems that didn't have (or didn't have but did have ). However, it has been several months and we have not heard anyone scream, so ... It is slightly amusing that the reason you #included was to get the definition of 'intptr_t' and the C standard states that this type is optional. In practice, I suspect that the number of platforms which do not define 'intptr_t' and 'uintptr_t' in the header is rather small. Having said that ... If I'm reading the code/config correctly, HP-NONSTOP would be failing to compile at the moment. (Although it has , it does not define 'intptr_t' - ie it defines 'NO_REGEX = YesPlease' and 'NO_INTPTR_T = \ UnfortunatelyYes'). > Other than that, I think this patch is an improvement. Thanks. What do you think of replacing it with the original patch (above)? ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] regex: fix a SIZE_MAX macro redefinition warning
Since commit 56a1a3ab ("Silence GCC's \"cast of pointer to integer of a
different size\" warning", 26-10-2015), sparse has been issuing a macro
redefinition warning for the SIZE_MAX macro. However, gcc did not issue
any such warning.
After commit 56a1a3ab, in terms of the order of #includes and #defines,
the code looked something like:
$ cat -n junk.c
1#include
2
3#define SIZE_MAX ((size_t) -1)
4
5#include
6
7int main(int argc, char *argv[])
8{
9return 0;
10}
$
$ gcc junk.c
$
However, if you compile that file with -Wsystem-headers, then it will
also issue a warning. Having set -Wsystem-headers in CFLAGS, using the
config.mak file, then (on cygwin):
$ make compat/regex/regex.o
CC compat/regex/regex.o
In file included from
/usr/lib/gcc/x86_64-pc-cygwin/4.9.3/include/stdint.h:9:0,
from compat/regex/regcomp.c:21,
from compat/regex/regex.c:77:
/usr/include/stdint.h:362:0: warning: "SIZE_MAX" redefined
#define SIZE_MAX (__SIZE_MAX__)
^
In file included from compat/regex/regex.c:69:0:
compat/regex/regex_internal.h:108:0: note: this is the location of the
previous definition
# define SIZE_MAX ((size_t) -1)
^
$
The compilation of the compat/regex code is somewhat unusual in that the
regex.c file directly #includes the other c files (regcomp.c, regexec.c
and regex_internal.c). Commit 56a1a3ab added an #include of
to the regcomp.c file, which results in the redefinition, since this is
included after the regex_internal.h header. This header file contains a
'fallback' definition for SIZE_MAX, in order to support systems which do
not have the header (the HAVE_STDINT_H macro is not defined).
In order to suppress the warning, we remove the #include of
from regcomp.c and set the HAVE_STDINT_H macro, using the regex.o build
rule within the Makefile, to ensure that is #included from
the regex_internal.h header.
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Junio,
About seven months ago, sparse started complaining but, because gcc wasn't, I
just assumed this was a sparse bug. I put it on my sparse TODO list and pretty
much forgot about it. Tonight I decided to take a quick look and was a bit
surprised by what I found. ;-)
I spent some time worrying about the 'unconditional inclusion' of
(there used to be systems that didn't supply that header that we wanted to
support), but I suspect that is now a non-issue. In any event, I don't
think this patch makes things any worse.
[Just a flavour of the rabbit holes I went down; HP-NONSTOP seems to supply
both and but it doesn't supply the intptr_t and
uintptr_t types (ie it defines NO_INTPTR_T). So, I checked the C99 and C11
standards and, blow me down, but the standard states that those types are
_optional_. (C99 7.18.1.4) :-D ]
ATB,
Ramsay Jones
Makefile | 2 +-
compat/regex/regcomp.c | 2 --
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 0d59718..3f6c70a 100644
--- a/Makefile
+++ b/Makefile
@@ -1987,7 +1987,7 @@ endif
ifdef NO_REGEX
compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
- -DGAWK -DNO_MBSUPPORT
+ -DGAWK -DNO_MBSUPPORT -DHAVE_STDINT_H
endif
ifdef USE_NED_ALLOCATOR
diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index fba5986..d8bde06 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -18,8 +18,6 @@
Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301 USA. */
-#include
-
static reg_errcode_t re_compile_internal (regex_t *preg, const char * pattern,
size_t length, reg_syntax_t syntax);
static void re_compile_fastmap_iter (regex_t *bufp,
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/10] builtin/tag.c: convert trivial snprintf calls to xsnprintf
On 03/06/16 09:52, Jeff King wrote:
> On Fri, Jun 03, 2016 at 07:47:17AM +, Elia Pinto wrote:
>
>> builtin/tag.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index 50e4ae5..0345ca3 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -225,7 +225,7 @@ static void create_tag(const unsigned char *object,
>> const char *tag,
>> if (type <= OBJ_NONE)
>> die(_("bad object type."));
>>
>> -header_len = snprintf(header_buf, sizeof(header_buf),
>> +header_len = xsnprintf(header_buf, sizeof(header_buf),
>>"object %s\n"
>>"type %s\n"
>>"tag %s\n"
>
> This is another of my "type 2" cases. I'd argue it should be using a
> heap buffer to handle tag and tagger names of arbitrary size.
Yep. As it stands, the code following this hunk:
if (header_len > sizeof(header_buf) - 1)
die(_("tag header too big."));
is now dead code, and the new error message is not as useful.
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf
On 03/06/16 09:53, Jeff King wrote:
> On Fri, Jun 03, 2016 at 07:47:16AM +, Elia Pinto wrote:
>
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>> index e8c71fc..c032fe7 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -1443,7 +1443,7 @@ static void final(const char *final_pack_name, const
>> char *curr_pack_name,
>> printf("%s\n", sha1_to_hex(sha1));
>> } else {
>> char buf[48];
>> -int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
>> +int len = xsnprintf(buf, sizeof(buf), "%s\t%s\n",
>> report, sha1_to_hex(sha1));
>> write_or_die(1, buf, len);
>
> So it's pretty unclear here whether that 48 is big enough (it is, if you
> read the whole function, because "report" is always a 4-char string).
> Yuck. At least there should be a comment explaining why 48 is big
> enough.
Agreed, again I would use something like:
char buf[GIT_SHA1_HEXSZ + 7]; /* 40 (sha1) + 4 (report) + 3
(\t\n\0) */
(and yes yuck - is report ever likely to increase? "bitmap" perhaps?)
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf
On 03/06/16 09:49, Jeff King wrote:
> On Fri, Jun 03, 2016 at 07:47:15AM +, Elia Pinto wrote:
>
[snip]
> For this particular change:
>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 443ff91..c65abaa 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1552,7 +1552,7 @@ static int run_rewrite_hook(const unsigned char
>> *oldsha1,
>> code = start_command();
>> if (code)
>> return code;
>> -n = snprintf(buf, sizeof(buf), "%s %s\n",
>> +n = xsnprintf(buf, sizeof(buf), "%s %s\n",
>> sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>
> I think it's the first type, as earlier we have:
>
> /* oldsha1 SP newsha1 LF NUL */
> static char buf[2*40 + 3];
>
> So unless that calculation is wrong, this would never truncate. The move
> to xsnprintf is an improvement,
I was going to suggest, if we stay with the static buffer, that the size
expression be changed to '2 * GIT_SHA1_HEXSZ + 3'. However, ...
> but I wonder if we would be happier
> still with:
>
> buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>
> Then we do not even have to wonder about the size computation. True, it
> uses a heap buffer when we do not need to, but I find it hard to care
> about grabbing 80 bytes of heap when we are in the midst of exec-ing an
> entirely new process.
... I agree with this also.
>
> By the way, there are some other uses of snprintf in the same file, that
> I think would fall into the type 2 I mentioned above (they use PATH_MAX,
> which I think is shorter than necessary on some systems).
>
> Those ones would also benefit from using higher-level constructs. Like:
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 443ff91..9336724 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1563,24 +1563,23 @@ static int run_rewrite_hook(const unsigned char
> *oldsha1,
>
> int run_commit_hook(int editor_is_used, const char *index_file, const char
> *name, ...)
> {
> - const char *hook_env[3] = { NULL };
> - char index[PATH_MAX];
> + struct argv_array hook_env = ARGV_ARRAY_INIT;
> va_list args;
> int ret;
>
> - snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> - hook_env[0] = index;
> + argv_array_pushf(_env, "GIT_INDEX_FILE=%s", index_file);
>
> /*
>* Let the hook know that no editor will be launched.
>*/
> if (!editor_is_used)
> - hook_env[1] = "GIT_EDITOR=:";
> + argv_array_push(_env, "GIT_EDITOR=:");
>
> va_start(args, name);
> ret = run_hook_ve(hook_env, name, args);
> va_end(args);
>
> + argv_array_clear(_env);
> return ret;
> }
Indeed.
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/6] worktree.c: add find_worktree()
On 03/06/16 13:19, Nguyễn Thái Ngọc Duy wrote:
> So far we haven't needed to identify an existing worktree from command
> line. Future commands such as lock or move will need it. The current
> implementation identifies worktrees by path (*). In future, the function
> could learn to identify by $(basename $path) or tags...
>
> (*) We could probably go cheaper with comparing inode number (and
> probably more reliable than paths when unicode enters the game). But not
> all systems have good inode that so let's stick to something simple for
> now.
>
> Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
> ---
> worktree.c | 15 +++
> worktree.h | 8
> 2 files changed, 23 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index e2a94e0..554f566 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -214,6 +214,21 @@ const char *get_worktree_git_dir(const struct worktree
> *wt)
> return git_common_path("worktrees/%s", wt->id);
> }
>
> +struct worktree *find_worktree(struct worktree **list,
> +const char *prefix,
> +const char *arg)
> +{
> + char *path;
> +
> + arg = prefix_filename(prefix, strlen(prefix), arg);
> + path = xstrdup(real_path(arg));
> + for (; *list; list++)
> + if (!fspathcmp(path, real_path((*list)->path)))
The results of the call to real_path() should probably be cached
in the worktree structure, since real_path() is relatively expensive
(it calls chdir(), lstat(), readlink() etc.), so you don't want to
re-compute the same result time-after-time ...
> + break;
> + free(path);
> + return *list;
> +}
> +
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] pathspec: allow escaped query values
On 02/06/16 20:46, Junio C Hamano wrote: > Ramsay Jones <ram...@ramsayjones.plus.com> writes: > >> I think Junio wants to go with just " quoting (see other thread). > > No. I meant just \ quoting. Yes, sorry, I only just read your last email on the other thread. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] pathspec: allow escaped query values
On 02/06/16 20:29, Junio C Hamano wrote: > Junio C Hamano <gits...@pobox.com> writes: > >> On Thu, Jun 2, 2016 at 11:42 AM, Ramsay Jones >> <ram...@ramsayjones.plus.com> wrote: >>>> >>>> That would be workable, I would think. Before attr:VAR=VAL >>>> extention, supported pathspec were only single lowercase-ascii >>>> alphabet tokens, so nobody would have used " as a part of magic. So >>>> quting with double-quote pair would work. >>> >>> I was thinking about both ' and ", so that you could do: >> >> Yes, I understood your suggestion as such. Quoting like shells would work >> without breaking backward compatibility for the same reason quoting with >> double-quote and backslash only without supporting single-quotes would >> work. > > Having said that, "It would work" does not have to mean "Hence we > must do it that way" at all. Quoting character pairs make the > parsing and unquoting significantly more complex. > > As you said, not many people used attributes and pathspec magic, and > I do not think those who want to use the new "further limits with > attributes" magic, envisioned primarily to be those who want to give > classes to submodules, have compelling reason to name their classes > with anything but lowercase-ascii-alphabet tokens. So for a practical > purposes, I'd rather see Stefan > > * just implement backquote-blindly-passes-the-next-byte and nothing >more elaborate; and > > * forbid [^-a-z0-9,_] from being used in the value part in the >attr:VAR=VAL magic. > > at least for now, and concentrate more on the other more important > parts of the submodule enhancement topic. OK, that reasonable. I didn't mean to derail Stefan's development! ATB, Ramsay Jones > > That way, those who will start using attr:VAR=VAL magic will stick > themselves to lowercase-ascii-alphabet tokens for now (simply > because an attribut that has the forbidden characters in its value > would not be usable with the magic), and we can later extend the > magic syntax parser in a backward compatible way to allow paired > quotes and other "more convenient" syntax. > > > [Footnote] > > *1* The reason I prefer to keep the initially allowed value > characters narrow is because I envision that something like > > :(attr:VAR=()) > > may become necessary, and do not want to promise users that open or > close parentheses will forever be taken literally. > > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] pathspec: allow escaped query values
On 02/06/16 20:04, Stefan Beller wrote:
> On Thu, Jun 2, 2016 at 11:42 AM, Ramsay Jones
> <ram...@ramsayjones.plus.com> wrote:
>>
>>
>> On 02/06/16 17:10, Junio C Hamano wrote:
>>> Ramsay Jones <ram...@ramsayjones.plus.com> writes:
>>>
>>>> So, at risk of annoying you, let me continue in my ignorance a little
>>>> longer and ask: even if you have to protect all of this 'magic' from
>>>> the shell with '/" quoting, could you not use (nested) quotes to
>>>> protect the part of an ? For example:
>>>>
>>>> git ls-files ':(attr:whitespace="indent,trail,space",icase)'
>>>
>>> That would be workable, I would think. Before attr:VAR=VAL
>>> extention, supported pathspec were only single lowercase-ascii
>>> alphabet tokens, so nobody would have used " as a part of magic. So
>>> quting with double-quote pair would work.
>>
>> I was thinking about both ' and ", so that you could do:
>>
>>$ ./args ':(attr:whitespace="indent,trail,space",icase)'
>> 1::(attr:whitespace="indent,trail,space",icase)
>>
>>$ ./args ":(attr:whitespace='indent,trail,space',icase)"
>> 1::(attr:whitespace='indent,trail,space',icase)
>>
>>$ p=':(attr:whitespace="indent,trail,space",icase)'
>>$ ./args "$p"
>> 1::(attr:whitespace="indent,trail,space",icase)
>>
>>$ p=":(attr:whitespace=\"indent,trail,space\",icase)"
>>$ ./args "$p"
>> 1::(attr:whitespace="indent,trail,space",icase)
>>
>> but limiting it to " would probably be OK too.
>>
>>> You'd need to come up with a way to quote a double quote that
>>> happens to be a part of VAL somehow, though.
>>
>> Yes I was assuming \ quoting as well - I just want to reduce the
>> need for such quoting (especially on windows).
>>
>>> I think attribute
>>> value is limited to a string with non-whitespace letters; even
>>> though the built-in attributes that have defined meaning to the Git
>>> itself may not use values with letters beyond [-a-zA-Z0-9,], end
>>> users and projects can add arbitrary values within the allowed
>>> syntax, so it is not unconceivable that some project may have a
>>> custom attribute that lists forbidden characters in a path with
>>>
>>> === .gitattributes ===
>>> *.txt forbidden=`"
>
> We restrict the 'forbidden' to follow [-a-zA-Z0-9,], so we could enforce
> it for the values, too.
>
>
>>
>>$ ./args ":(attr:*.txt forbidden=\'\\\",icase)"
>> 1::(attr:*.txt forbidden=\'\",icase)
>
> You should lose the *.txt in there, but put it at the back
Ah, yes, just shows my ignorance of the attribute system!
>
>> $ ./args ":(attr:forbidden=\'\\\",icase)*.txt"
>
>>
>>$ ./args ':(attr:*.txt forbidden=\'\''\",icase)'
>> 1::(attr:*.txt forbidden=\'\",icase)
>
> I see, so quoting by " or ' is preferred. What if the user
> wants to do a
I think Junio wants to go with just " quoting (see other thread).
> forbidden=',"
>
> so we have to escape those in there, such as
>
> ./args ':(attr:"forbidden=\',\"")'
No, that won't work (" is not terminated), try this:
$ ./args ':(attr:"forbidden='\'',\"")'
1::(attr:"forbidden=',\"")
$
$ ./args ":(attr:\"forbidden=',\\\"\")"
1::(attr:"forbidden=',\"")
$
[half of the problem is just getting past the shell]
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] pathspec: allow escaped query values
On 02/06/16 17:10, Junio C Hamano wrote: > Ramsay Jones <ram...@ramsayjones.plus.com> writes: > >> So, at risk of annoying you, let me continue in my ignorance a little >> longer and ask: even if you have to protect all of this 'magic' from >> the shell with '/" quoting, could you not use (nested) quotes to >> protect the part of an ? For example: >> >> git ls-files ':(attr:whitespace="indent,trail,space",icase)' > > That would be workable, I would think. Before attr:VAR=VAL > extention, supported pathspec were only single lowercase-ascii > alphabet tokens, so nobody would have used " as a part of magic. So > quting with double-quote pair would work. I was thinking about both ' and ", so that you could do: $ ./args ':(attr:whitespace="indent,trail,space",icase)' 1::(attr:whitespace="indent,trail,space",icase) $ ./args ":(attr:whitespace='indent,trail,space',icase)" 1::(attr:whitespace='indent,trail,space',icase) $ p=':(attr:whitespace="indent,trail,space",icase)' $ ./args "$p" 1::(attr:whitespace="indent,trail,space",icase) $ p=":(attr:whitespace=\"indent,trail,space\",icase)" $ ./args "$p" 1::(attr:whitespace="indent,trail,space",icase) but limiting it to " would probably be OK too. > You'd need to come up with a way to quote a double quote that > happens to be a part of VAL somehow, though. Yes I was assuming \ quoting as well - I just want to reduce the need for such quoting (especially on windows). > I think attribute > value is limited to a string with non-whitespace letters; even > though the built-in attributes that have defined meaning to the Git > itself may not use values with letters beyond [-a-zA-Z0-9,], end > users and projects can add arbitrary values within the allowed > syntax, so it is not unconceivable that some project may have a > custom attribute that lists forbidden characters in a path with > > === .gitattributes === > *.txt forbidden=`" > > that tells their documentation cannot have these letters in it, or > something like that. Heh, yeah, that gets ugly: $ ./args ":(attr:*.txt forbidden=\'\\\",icase)" 1::(attr:*.txt forbidden=\'\",icase) $ ./args ':(attr:*.txt forbidden=\'\''\",icase)' 1::(attr:*.txt forbidden=\'\",icase) [Note the initial ' 1:' above is output from args] ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] pathspec: allow escaped query values
On 02/06/16 06:46, Junio C Hamano wrote: > Ramsay Jones <ram...@ramsayjones.plus.com> writes: > >> Not having given this much thought at all, but the question which comes >> to mind is: can you use some other separator for the -s rather than >> a comma? That way you don't need to quote them in the part of the >> -spec. >> >> (I dunno, maybe use ; or : instead?) > > There are two kinds of comma involved in this discussion. > > * Multiple pathspec magic can be attached to augment the way > selects paths. ":(,,...)" is >the syntax, and are things like "icase" (select the path >that matches case-insensitively), "top" ( must >match from the top level of the working tree, even when you are >running the command from a subdirectory). We added a new kind of > whose syntax is "attr:VAR=VAL" recently, which says "not >only must match the path, in order to be selected, the >path must have the attribute VAR with value VAL". > >The comma that separates multiple magic is not something you can >change now; it has been with us since v1.7.6 (Jun 2011) > > * My example wanted to use the attr:VAR=VAL form to select those >paths that has one specific string as the value for whitespace >attribute, i.e. VAR in this case is "whitespace". The value for >whitespace attribute determines what kind of whitespace anomalies >are considered as errors by "git apply" and "git diff", and it is >formed by concatenating things like "indent-with-non-tab" (starts >a line with more than 8 consecutive SPs), "space-before-tab" (a >SP appears immediately before HT in the indent), etc., with a >comma in between. > >The comma that separates various kinds of whitespace errors is >not something you can change now; it has been with us since >v1.5.4 (Feb 2008). > > So using different separator is not a viable solution. Ah, OK, makes sense. Note that I have not used 'pathspec magic' or the attribute system in git (never felt/had the need)! ;-) So, at risk of annoying you, let me continue in my ignorance a little longer and ask: even if you have to protect all of this 'magic' from the shell with '/" quoting, could you not use (nested) quotes to protect the part of an ? For example: git ls-files ':(attr:whitespace="indent,trail,space",icase)' [Hmm, I don't seem to be able to find documentation on the syntax of these features (apart from the code, of course).] ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] pathspec: allow escaped query values
On 02/06/16 00:52, Stefan Beller wrote: > In our own .gitattributes file we have attributes such as: > > *.[ch] whitespace=indent,trail,space > > When querying for attributes we want to be able to ask for the exact > value, i.e. > > git ls-files :(attr:whitespace=indent,trail,space) > > should work, but the commas are used in the attr magic to introduce > the next attr, such that this query currently fails with > > fatal: Invalid pathspec magic 'trail' in > ':(attr:whitespace=indent,trail,space)' > > This change allows escaping characters by a backslash, such that the query > > git ls-files :(attr:whitespace=indent\,trail\,space) Not having given this much thought at all, but the question which comes to mind is: can you use some other separator for the -s rather than a comma? That way you don't need to quote them in the part of the -spec. (I dunno, maybe use ; or : instead?) ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t4014: shell portability fix
On 31/05/16 23:53, Junio C Hamano wrote: > One-shot assignment to an environment variable, i.e. > > VAR=VAL cmd > > does not work as expected for "cmd" that is a shell function on > certain shells. test_commit _is_ a helper function and cannot be > driven that way portably. > > Signed-off-by: Junio C Hamano <gits...@pobox.com> > --- > t/t4014-format-patch.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > index 8049cad..c3aa543 100755 > --- a/t/t4014-format-patch.sh > +++ b/t/t4014-format-patch.sh > @@ -1072,7 +1072,7 @@ test_expect_success '--from omits redundant in-body > header' ' > ' > > test_expect_success 'in-body headers trigger content encoding' ' > - GIT_AUTHOR_NAME="éxötìc" test_commit exotic && > + (export GIT_AUTHOR_NAME="éxötìc"; test_commit exotic) && Isn't 'export VAR=VAL' non-portable? So, maybe: (GIT_AUTHOR_NAME="éxötìc"; export GIT_AUTHOR_NAME; test_commit exotic) && > test_when_finished "git reset --hard HEAD^" && > git format-patch -1 --stdout --from >patch && > cat >expect <<-\EOF && ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/13] refs: introduce an iterator interface
On 30/05/16 16:22, Ramsay Jones wrote:
>
>
> On 30/05/16 08:55, Michael Haggerty wrote:
> [snip]
>
>> /* Reference is a symbolic reference. */
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 8ab4d5f..dbf1587 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -1,6 +1,7 @@
>> #include "../cache.h"
>> #include "../refs.h"
>> #include "refs-internal.h"
>> +#include "../iterator.h"
>> #include "../lockfile.h"
>> #include "../object.h"
>> #include "../dir.h"
>> @@ -704,6 +705,154 @@ static void prime_ref_dir(struct ref_dir *dir)
>> }
>> }
>>
>> +/*
>> + * A level in the reference hierarchy that is currently being iterated
>> + * through.
>> + */
>> +struct cache_ref_iterator_level {
>> +/*
>> + * The ref_dir being iterated over at this level. The ref_dir
>> + * is sorted before being stored here.
>> + */
>> +struct ref_dir *dir;
>> +
>> +/*
>> + * The index of the current entry within dir (which might
>> + * itself be a directory). If index == -1, then the iteration
>> + * hasn't yet begun. If index == dir->nr, then the iteration
>> + * through this level is over.
>> + */
>> +int index;
>> +};
>> +
>> +/*
>> + * Represent an iteration through a ref_dir in the memory cache. The
>> + * iteration recurses through subdirectories.
>> + */
>> +struct cache_ref_iterator {
>> +struct ref_iterator base;
>> +
>> +/*
>> + * The number of levels currently on the stack. This is always
>> + * at least 1, because when it becomes zero the iteration is
>> + * ended and this struct is freed.
>> + */
>> +size_t levels_nr;
>> +
>> +/* The number of levels that have been allocated on the stack */
>> +size_t levels_alloc;
>> +
>> +/*
>> + * A stack of levels. levels[0] is the uppermost level that is
>> + * being iterated over in this iteration. (This is not
>> + * necessary the top level in the references hierarchy. If we
>> + * are iterating through a subtree, then levels[0] will hold
>> + * the ref_dir for that subtree, and subsequent levels will go
>> + * on from there.)
>> + */
>> +struct cache_ref_iterator_level *levels;
>> +};
>> +
>> +static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
>> +{
>> +struct cache_ref_iterator *iter =
>> +(struct cache_ref_iterator *)ref_iterator;
>> +
>> +while (1) {
>> +struct cache_ref_iterator_level *level =
>> +>levels[iter->levels_nr - 1];
>> +struct ref_dir *dir = level->dir;
>> +struct ref_entry *entry;
>> +
>> +if (level->index == -1)
>> +sort_ref_dir(dir);
>
> do you need to sort here ...
>> +
>> +if (++level->index == level->dir->nr) {
>> +/* This level is exhausted; pop up a level */
>> +if (--iter->levels_nr == 0)
>> +return ref_iterator_abort(ref_iterator);
>> +
>> +continue;
>> +}
>> +
>> +entry = dir->entries[level->index];
>> +
>> +if (entry->flag & REF_DIR) {
>> +/* push down a level */
>> +ALLOC_GROW(iter->levels, iter->levels_nr + 1,
>> + iter->levels_alloc);
>> +
>> +level = >levels[iter->levels_nr++];
>> +level->dir = get_ref_dir(entry);
>> +sort_ref_dir(level->dir);
>
> ... given that you sort here?
I had intended to say 'or vice versa' here. When I wrote this, I had not
finished reading this patch (let alone the series). Now, I suspect that
you can simply drop this 'sort_ref_dir()' call site. Unless I've misread
the code, of course! ;-)
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/13] refs: introduce an iterator interface
On 30/05/16 08:55, Michael Haggerty wrote:
[snip]
> /* Reference is a symbolic reference. */
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8ab4d5f..dbf1587 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1,6 +1,7 @@
> #include "../cache.h"
> #include "../refs.h"
> #include "refs-internal.h"
> +#include "../iterator.h"
> #include "../lockfile.h"
> #include "../object.h"
> #include "../dir.h"
> @@ -704,6 +705,154 @@ static void prime_ref_dir(struct ref_dir *dir)
> }
> }
>
> +/*
> + * A level in the reference hierarchy that is currently being iterated
> + * through.
> + */
> +struct cache_ref_iterator_level {
> + /*
> + * The ref_dir being iterated over at this level. The ref_dir
> + * is sorted before being stored here.
> + */
> + struct ref_dir *dir;
> +
> + /*
> + * The index of the current entry within dir (which might
> + * itself be a directory). If index == -1, then the iteration
> + * hasn't yet begun. If index == dir->nr, then the iteration
> + * through this level is over.
> + */
> + int index;
> +};
> +
> +/*
> + * Represent an iteration through a ref_dir in the memory cache. The
> + * iteration recurses through subdirectories.
> + */
> +struct cache_ref_iterator {
> + struct ref_iterator base;
> +
> + /*
> + * The number of levels currently on the stack. This is always
> + * at least 1, because when it becomes zero the iteration is
> + * ended and this struct is freed.
> + */
> + size_t levels_nr;
> +
> + /* The number of levels that have been allocated on the stack */
> + size_t levels_alloc;
> +
> + /*
> + * A stack of levels. levels[0] is the uppermost level that is
> + * being iterated over in this iteration. (This is not
> + * necessary the top level in the references hierarchy. If we
> + * are iterating through a subtree, then levels[0] will hold
> + * the ref_dir for that subtree, and subsequent levels will go
> + * on from there.)
> + */
> + struct cache_ref_iterator_level *levels;
> +};
> +
> +static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
> +{
> + struct cache_ref_iterator *iter =
> + (struct cache_ref_iterator *)ref_iterator;
> +
> + while (1) {
> + struct cache_ref_iterator_level *level =
> + >levels[iter->levels_nr - 1];
> + struct ref_dir *dir = level->dir;
> + struct ref_entry *entry;
> +
> + if (level->index == -1)
> + sort_ref_dir(dir);
do you need to sort here ...
> +
> + if (++level->index == level->dir->nr) {
> + /* This level is exhausted; pop up a level */
> + if (--iter->levels_nr == 0)
> + return ref_iterator_abort(ref_iterator);
> +
> + continue;
> + }
> +
> + entry = dir->entries[level->index];
> +
> + if (entry->flag & REF_DIR) {
> + /* push down a level */
> + ALLOC_GROW(iter->levels, iter->levels_nr + 1,
> +iter->levels_alloc);
> +
> + level = >levels[iter->levels_nr++];
> + level->dir = get_ref_dir(entry);
> + sort_ref_dir(level->dir);
... given that you sort here?
> + level->index = -1;
> + } else {
> + iter->base.refname = entry->name;
> + iter->base.oid = >u.value.oid;
> + iter->base.flags = entry->flag;
> + return ITER_OK;
> + }
> + }
> +}
> +
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] log: document the --decorate=auto option
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com> --- Hi Junio, While reading an email from Linus earlier (RFC: dynamic "auto" date formats), I noticed that log.decorate was being set to 'auto'. Since I didn't recall that setting (even if it's easy to guess), I went looking for the documentation ... This should probably be marked RFC, since I haven't checked that the formatting is OK (I don't have the documentation toolchain installed these days). ATB, Ramsay Jones Documentation/config.txt | 5 - Documentation/git-log.txt | 8 +--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 53f00db..0707b3b 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1956,7 +1956,10 @@ log.decorate:: command. If 'short' is specified, the ref name prefixes 'refs/heads/', 'refs/tags/' and 'refs/remotes/' will not be printed. If 'full' is specified, the full ref name (including prefix) will be printed. - This is the same as the log commands '--decorate' option. + If 'auto' is specified, then if the output is going to a terminal, + the ref names are shown as if 'short' were given, otherwise no ref + names are shown. This is the same as the log commands '--decorate' + option. log.follow:: If `true`, `git log` will act as if the `--follow` option was used when diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt index 03f9580..dec379b 100644 --- a/Documentation/git-log.txt +++ b/Documentation/git-log.txt @@ -29,12 +29,14 @@ OPTIONS (works only for a single file). --no-decorate:: ---decorate[=short|full|no]:: +--decorate[=short|full|auto|no]:: Print out the ref names of any commits that are shown. If 'short' is specified, the ref name prefixes 'refs/heads/', 'refs/tags/' and 'refs/remotes/' will not be printed. If 'full' is specified, the - full ref name (including prefix) will be printed. The default option - is 'short'. + full ref name (including prefix) will be printed. If 'auto' is + specified, then if the output is going to a terminal, the ref names + are shown as if 'short' were given, otherwise no ref names are + shown. The default option is 'short'. --source:: Print out the ref name given on the command line by which each -- 2.8.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clone: don't use an integer as a NULL pointer
On 26/05/16 00:30, Stefan Beller wrote: > On Wed, May 25, 2016 at 4:12 PM, Ramsay Jones > <ram...@ramsayjones.plus.com> wrote: >> >> Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com> >> --- >> >> Hi Stefan, >> >> If you need to re-roll your 'sb/submodule-default-paths' branch, could >> you please squash this into the relevant patch. (commit 8efbe28b, >> "clone: add --init-submodule= switch", 23-05-2016). >> >> Thanks! >> >> ATB, >> Ramsay Jones > > Thanks for pointing out! > > I am sorry for having you write me these emails; It's no problem ... > Out of curiosity, how much of this is manual work and how > much did you automate of this? ... because this particular problem is caught by 'make sparse'. [OK, I have to write the patch, test, etc. manually, but that's not a great burden.] ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] xemit.c: fix a [-Wchar-subscripts] compiler warning
While compiling on cygwin (x86_64), gcc complains thus:
CC xdiff/xemit.o
xdiff/xemit.c: In function ‘is_empty_rec’:
xdiff/xemit.c:163:2: warning: array subscript has type ‘char’
[-Wchar-subscripts]
while (len > 0 && isspace(*rec)) {
^
A comment in the header reads, in part, like so:
These macros are intentionally written in a manner that will trigger
a gcc -Wall warning if the user mistakenly passes a 'char' instead
of an int containing an 'unsigned char'.
In order to suppress the warning, cast the 'char *' pointer 'rec' to an
'unsigned char *' pointer, prior to passing the dereferenced pointer to
the isspace() macro.
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi René,
If you need to re-roll your 'rs/xdiff-hunk-with-func-line' branch, could
you please squash this (or something like it) into the relevant patch.
[A comment in the linux header, says that the ctype-info tables ...
point into arrays of 384, so they can be indexed by any `unsigned
char' value [0,255]; by EOF (-1); or by any `signed char' value
[-128,-1).
So, this is not a problem on linux.]
Thanks!
ATB,
Ramsay Jones
xdiff/xemit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d0c0738..ae9adac 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -160,7 +160,7 @@ static int is_empty_rec(xdfile_t *xdf, xdemitconf_t const
*xecfg, long ri)
const char *rec;
long len = xdl_get_rec(xdf, ri, );
- while (len > 0 && isspace(*rec)) {
+ while (len > 0 && isspace(*((unsigned char *)rec))) {
rec++;
len--;
}
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] clone: don't use an integer as a NULL pointer
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
Hi Stefan,
If you need to re-roll your 'sb/submodule-default-paths' branch, could
you please squash this into the relevant patch. (commit 8efbe28b,
"clone: add --init-submodule= switch", 23-05-2016).
Thanks!
ATB,
Ramsay Jones
builtin/clone.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 22b6eac..a056f72 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -925,7 +925,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
struct strbuf sb = STRBUF_INIT;
for_each_string_list_item(item, _submodules) {
strbuf_addf(, "submodule.defaultUpdatePath=%s",
item->string);
- string_list_append(_config, strbuf_detach(,
0));
+ string_list_append(_config, strbuf_detach(,
NULL));
}
}
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] index-helper: use watchman to avoid refreshing index with lstat()
> +
> + strbuf_release();
> +}
> +
> +static int mark_untracked_invalid(struct string_list_item *item, void *uc)
> +{
> + struct untracked_cache *untracked = uc;
> + struct untracked_cache_dir *dir;
> +
> + dir = find_untracked_cache_dir(untracked, untracked->root,
> +item->string);
> + if (dir)
> + dir->valid = 0;
> +
> + return 0;
> }
>
> static int read_watchman_ext(struct index_state *istate, const void *data,
> @@ -1409,10 +1473,24 @@ static int read_watchman_ext(struct index_state
> *istate, const void *data,
> ewah_each_bit(bitmap, mark_no_watchman, istate);
> ewah_free(bitmap);
>
> - /*
> - * TODO: update the untracked cache from the untracked data in this
> - * extension.
> - */
> + if (istate->untracked && istate->untracked->root) {
> + int i;
> + const char *untracked;
> +
> + untracked = (const char *)data + len + 8 + bitmap_size;
> + for (i = 0; i < untracked_nr; ++i) {
> + int len = strlen(untracked);
> +
> string_list_append(>untracked->invalid_untracked,
> +untracked);
> + untracked += len + 1;
> + }
> +
> + for_each_string_list(>untracked->invalid_untracked,
> + mark_untracked_invalid, istate->untracked);
> +
> + if (untracked_nr)
> + istate->cache_changed |= WATCHMAN_CHANGED;
> + }
> return 0;
> }
>
> @@ -1645,29 +1723,88 @@ static void post_read_index_from(struct index_state
> *istate)
> tweak_untracked_cache(istate);
> }
>
> +/* in ms */
> +#define WATCHMAN_TIMEOUT 1000
> +
> +static int poke_and_wait_for_reply(int fd)
> +{
> + int ret = -1;
> + struct pollfd pollfd;
> + int bytes_read;
> + char reply_buf[4096];
> + const char *requested_capabilities = "";
> +
> +#ifdef USE_WATCHMAN
> + requested_capabilities = "watchman";
> +#endif
> +
> + if (fd < 0)
> + return -1;
> +
> + if (packet_write_gently(fd, "poke %d %s", getpid(),
> requested_capabilities))
So, adding the empty capabilities (and more importantly the
separating space) is not so much 'doesn't hurt', rather than
'prevents a core-dump!' ;-)
> + return -1;
> + if (packet_flush_gently(fd))
> + return -1;
And yes, I'd forgotten about the 'maybe sometime in the future, we
could buffer the packets' ...
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 11/20] index-helper: use watchman to avoid refreshing index with lstat()
On 12/05/16 21:20, David Turner wrote:
> From: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
[snip]
>
> +/* in ms */
> +#define WATCHMAN_TIMEOUT 1000
> +
> +static int poke_and_wait_for_reply(int fd)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + int ret = -1;
> + struct pollfd pollfd;
> + int bytes_read;
> + char reply_buf[4096];
> + const char *requested_capabilities = "";
> +
> +#ifdef USE_WATCHMAN
> + requested_capabilities = "watchman";
> +#endif
> +
> + if (fd < 0)
> + return -1;
> +
> + strbuf_addf(, "poke %d %s", getpid(), requested_capabilities);
> + if (packet_write_gently(fd, buf.buf, buf.len))
This is not causing a problem or bug, but is none the less not
correct - as you know, packet_write_gently() takes a 'printf' like
variable argument list. (So, buf.buf is the format specifier and
buf.len is an unused arg).
I think I would write this as:
strbuf_addf(, "poke %d", getpid());
if (requested_capabilities && *requested_capabilities)
strbuf_addf(, " %s", requested_capabilities);
if (packet_write_gently(fd, "%s", buf.buf))
... or something similar. [Note, just typing into my email client, so
it's not been close to a compiler.]
> + return -1;
> + if (packet_flush_gently(fd))
> + return -1;
Why are you sending a flush packet - doesn't the index-helper
simply ignore it?
I haven't tried this yet BTW, just reading patches as they float
on past... ;-)
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 00/19] index-helper/watchman
On 10/05/16 21:30, Junio C Hamano wrote: > Ramsay Jones <ram...@ramsayjones.plus.com> writes: > >> On 10/05/16 12:52, Dennis Kaarsemaker wrote: >>> On ma, 2016-05-09 at 15:22 -0700, Junio C Hamano wrote: >>>> It passes on one box and fails on another. They both run the same >>>> Ubuntu 14.04 derivative, with same ext3 filesystem. The failing one >>>> is on a VM. >>> >>> Same here, except ext4 instead of ext3. Failing on a virtual machine, >>> not failing on a physical one. >> >> I can confirm the trend: >> >> Linux Mint 17.3, ext4 - bare-metal pass, (Virtual Box) VM fail. >> > I do not think there is anything VM specific, though. If it is > SIGPIPE, it is very much understandable it is timing dependent, and > VMness may be a cause of timing differences, nothing more. Yeah, I had previously noted that this was probably a timing related issue - this was only meant as confirmation that David could probably reproduce the issue if he tested in a VM. ;-) However, it appears Dennis has found a way to force a failure, even on bare-metal (at least on v8, I think). > I seem to getting the failure on my physical box today, by the way. Ah, interesting. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 00/19] index-helper/watchman
On 10/05/16 12:52, Dennis Kaarsemaker wrote: > On ma, 2016-05-09 at 15:22 -0700, Junio C Hamano wrote: >> It passes on one box and fails on another. They both run the same >> Ubuntu 14.04 derivative, with same ext3 filesystem. The failing one >> is on a VM. > > Same here, except ext4 instead of ext3. Failing on a virtual machine, > not failing on a physical one. I can confirm the trend: Linux Mint 17.3, ext4 - bare-metal pass, (Virtual Box) VM fail. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 00/19] index-helper/watchman
On 10/05/16 00:12, David Turner wrote:
> On Mon, 2016-05-09 at 15:32 -0700, Junio C Hamano wrote:
>> Junio C Hamano <gits...@pobox.com> writes:
>>
>>> David Turner <dtur...@twopensource.com> writes:
>>>
>>>> On Mon, 2016-05-09 at 14:40 -0700, Junio C Hamano wrote:
>>>>> Hmmm, I seem to be getting
>>>>>
>>>>> $ cat t/trash*7900*/err
>>>>> fatal: Already running
>>>>>
>>>>> after running t7900 and it fails at #5, after applying
>>>>> "index-helper: optionally automatically run"
>>
>> The symptom looks pretty similar to $gmane/293461 reported earlier.
>> Here is how "t7900-index-helper.sh -i -v -x -d" ends.
>>
>>
>> expecting success:
>> test_when_finished "git index-helper --kill" &&
>> rm -f .git/index-helper.sock &&
>> git status &&
>> test_path_is_missing .git/index-helper.sock &&
>> test_config indexhelper.autorun true &&
>> git status &&
>> test -S .git/index-helper.sock &&
>> git status 2>err &&
>> test -S .git/index-helper.sock &&
>> test_must_be_empty err &&
>> git index-helper --kill &&
>> test_config indexhelper.autorun false &&
>> git status &&
>> test_path_is_missing .git/index-helper.sock
>>
>> + test_when_finished git index-helper --kill
>> + test 0 = 0
>> + test_cleanup={ git index-helper --kill
>> } && (exit "$eval_ret"); eval_ret=$?; :
>> + rm -f .git/index-helper.sock
>> + git status
>> On branch master
>> Untracked files:
>> (use "git add ..." to include in what will be committed)
>>
>> err
>>
>> nothing added to commit but untracked files present (use "git add" to
>> track)
>> + test_path_is_missing .git/index-helper.sock
>> + test -e .git/index-helper.sock
>> + test_config indexhelper.autorun true
>> + config_dir=
>> + test indexhelper.autorun = -C
>> + test_when_finished test_unconfig 'indexhelper.autorun'
>> + test 0 = 0
>> + test_cleanup={ test_unconfig 'indexhelper.autorun'
>> } && (exit "$eval_ret"); eval_ret=$?; { git index
>> -helper --kill
>> } && (exit "$eval_ret"); eval_ret=$?; :
>> + git config indexhelper.autorun true
>> + git status
>> error: last command exited with $?=141
>
> I think that's a SIGPIPE on the first git status. Weird, since I just
> added sigpipe-avoidance code (in v8). Does anyone have any idea why
> the sigchain stuff isn't doing what I think it is?
Sorry for a late report (I've been a bit busy last couple of days), but
I've been seeing exactly the same on v8 of this series.
Note that the above 'git status' is actually the second git-status in the
test.
I haven't been able to debug it too much, but I can tell you that it is
not failing at exactly the same place every time (so it may be time
sensitive). However, it often fails in poke_and_wait_for_reply() at the
first packet_flush() (which in turn calls write_or_die() which calls
check_pipe() with an EPIPE(32)). At other times it fails when issuing
a flush after a refresh packet. For example, on one run with packet
tracing enabled, I got this for the trace:
trace: built-in: git 'status'
packet: git> poke 3221
packet: git>
packet: git< OK
packet: git> refresh
packet: git>
So, its getting the EPIPE for the refresh in this case, even though
the index-helper is still running, the unix socket is in .git/
(and so is a shm-* file BTW).
I didn't get any further than that I'm afraid.
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: t6044 broken on pu
On 07/05/16 14:15, Ramsay Jones wrote: > > > On 07/05/16 13:19, Andreas Schwab wrote: >> Torsten Bögershausen <tbo...@web.de> writes: >> >>> The "seq" is not understood by all shells, >>> using printf fixes this, >>> >>> index 20a3ffe..48d964e 100755 >>> --- a/t/t6044-merge-unrelated-index-changes.sh >>> +++ b/t/t6044-merge-unrelated-index-changes.sh >>> @@ -20,7 +20,7 @@ test_description="merges with unrelated index changes" >>> # Commit E: renames a->subdir/a, adds subdir/e >>> >>> test_expect_success 'setup trivial merges' ' >>> - seq 1 10 >a && >>> + printf 1 2 3 4 5 7 8 9 10 >a && >> >> $ printf 1 2 3 4 5 7 8 9 10 >> 1 > > yep, I think: > > printf "%d\n" 1 2 3 4 5 6 7 8 9 10 >a && > > would be equivalent. > Having said that, there is also 'test_seq' which you can use to avoid portability problems (although it uses perl, so could be viewed as a bit heavyweight): test_seq 1 10 >a && ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: t6044 broken on pu
On 07/05/16 13:19, Andreas Schwab wrote: > Torsten Bögershausen <tbo...@web.de> writes: > >> The "seq" is not understood by all shells, >> using printf fixes this, >> >> index 20a3ffe..48d964e 100755 >> --- a/t/t6044-merge-unrelated-index-changes.sh >> +++ b/t/t6044-merge-unrelated-index-changes.sh >> @@ -20,7 +20,7 @@ test_description="merges with unrelated index changes" >> # Commit E: renames a->subdir/a, adds subdir/e >> >> test_expect_success 'setup trivial merges' ' >> - seq 1 10 >a && >> + printf 1 2 3 4 5 7 8 9 10 >a && > > $ printf 1 2 3 4 5 7 8 9 10 > 1 yep, I think: printf "%d\n" 1 2 3 4 5 6 7 8 9 10 >a && would be equivalent. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: /* compiler workaround */ - what was the issue?
On 06/05/16 21:21, Ramsay Jones wrote:
> On 06/05/16 19:54, Junio C Hamano wrote:
>> Ramsay Jones <ram...@ramsayjones.plus.com> writes:
>>
[snip]
> I still can't get gcc to complain, e.g. (on top of above):
>
> $ git diff
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index deae1f3..845fcdc 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -377,7 +377,7 @@ int cmd_rev_list(int argc, const char **argv, const
> char *prefix)
> mark_edges_uninteresting(, show_edge);
>
> if (bisect_list) {
> - int reaches = 0, all = 0;
> + int reaches, all;
>
> revs.commits = find_bisection(revs.commits, , ,
> bisect_find_all);
> $ rm builtin/rev-list.o
> $ make V=1 CFLAGS='-g -O3 -Wall -Wextra -Wuninitialized
> -Wno-unused-parameter' builtin/rev-list.o
> cc -o builtin/rev-list.o -c -MF builtin/.depend/rev-list.o.d -MQ
> builtin/rev-list.o -MMD -MP -g -O3 -Wall -Wextra -Wuninitialized
> -Wno-unused-parameter -I. -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND
> -DHAVE_PATHS_H -DHAVE_DEV_TTY -DXDL_FAST_HASH -DHAVE_CLOCK_GETTIME
> -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM -DSHA1_HEADER=''
> -DNO_STRLCPY -DNO_MKSTEMPS -DSHELL_PATH='"/bin/sh"' builtin/rev-list.c
> In file included from ./cache.h:4:0,
>from builtin/rev-list.c:1:
> ./git-compat-util.h: In function ‘xsize_t’:
> ./git-compat-util.h:838:10: warning: comparison between signed and unsigned
> integer expressions [-Wsign-compare]
> if (len > (size_t) len)
> ^
> $
BTW, another patch that I have hanging around ... this time the
patch below was built on master from a couple of years ago (so,
I haven't bothered to rebase it, but you should get the idea):
ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] git-compat-util.h: xsize_t(): avoid signed comparison warnings
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
git-compat-util.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index c07e0c1..3a9cf6c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -838,9 +838,10 @@ static inline char *xstrdup_or_null(const char *str)
static inline size_t xsize_t(off_t len)
{
- if (len > (size_t) len)
+ size_t r = (size_t)len;
+ if (len != (off_t)r)
die("Cannot handle files this big");
- return (size_t)len;
+ return r;
}
__attribute__((format (printf, 3, 4)))
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: /* compiler workaround */ - what was the issue?
On 06/05/16 19:54, Junio C Hamano wrote:
> Ramsay Jones <ram...@ramsayjones.plus.com> writes:
>
>> The patch below applies to master (I haven't checked for any more
>> additions).
>>
>> if (bisect_list) {
>> -int reaches = reaches, all = all;
>> +int reaches = 0, all = 0;
>
> One thing that is somewhat sad is that this makes future readers
> wonder if these values '0' are sensible initial values.
>
> Having to wonder "is it sensible to initialize this variable to 0?
> Shouldn't it be initialized to INT_MAX instead?" is wasting their
> time exactly because we _know_ these are not even "initial values".
> We know these do not have to be initialized, because some more
> appropriate values will get assigned to them before they are used,
> and have these only because some compilers get it wrong.
>
> The original "reaches = reaches" had the documentation value (at
> least for those who knew the convention) to save the readers from
> wasting their time that way. Now these 0 are indistinguishable from
> the other initializations that require to be zero.
Ah, I think I remember now why I hadn't sent this patch before. ;-)
[This started off as one patch, was then split into two (int and pointer),
and then back into one again - presumably because I had by that time
forgotten why I split it up!]
I have a very vague recollection of you expressing your dislike of
these parts of the patch before. I had intended to investigate why
gcc was incorrectly issuing these warnings - but I couldn't get my
currently installed compiler to complain. That would have meant
building various gcc versions, so that I could bisect ...
So, that's why I didn't get around to it ... :-D
I still can't get gcc to complain, e.g. (on top of above):
$ git diff
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index deae1f3..845fcdc 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -377,7 +377,7 @@ int cmd_rev_list(int argc, const char **argv, const char
*prefix)
mark_edges_uninteresting(, show_edge);
if (bisect_list) {
- int reaches = 0, all = 0;
+ int reaches, all;
revs.commits = find_bisection(revs.commits, , ,
bisect_find_all);
$ rm builtin/rev-list.o
$ make V=1 CFLAGS='-g -O3 -Wall -Wextra -Wuninitialized
-Wno-unused-parameter' builtin/rev-list.o
cc -o builtin/rev-list.o -c -MF builtin/.depend/rev-list.o.d -MQ
builtin/rev-list.o -MMD -MP -g -O3 -Wall -Wextra -Wuninitialized
-Wno-unused-parameter -I. -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND
-DHAVE_PATHS_H -DHAVE_DEV_TTY -DXDL_FAST_HASH -DHAVE_CLOCK_GETTIME
-DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM -DSHA1_HEADER=''
-DNO_STRLCPY -DNO_MKSTEMPS -DSHELL_PATH='"/bin/sh"' builtin/rev-list.c
In file included from ./cache.h:4:0,
from builtin/rev-list.c:1:
./git-compat-util.h: In function ‘xsize_t’:
./git-compat-util.h:838:10: warning: comparison between signed and unsigned
integer expressions [-Wsign-compare]
if (len > (size_t) len)
^
$
[Note: gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4]
>
>> diff --git a/read-cache.c b/read-cache.c
>> index d9fb78b..978d6b6 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1870,7 +1870,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd,
>> struct cache_entry *ce,
>> {
>> int size;
>> struct ondisk_cache_entry *ondisk;
>> -int saved_namelen = saved_namelen; /* compiler workaround */
>> +int saved_namelen = 0;
>
> I wonder if can we come up with a short and sweet notation to remind
> futhre readers that this "initialization" is not initializing but
> merely squelching warnings from stupid compilers, and agree to use
> it consistently?
Nothing comes to mind.
Do current compilers complain?
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: /* compiler workaround */ - what was the issue?
On 06/05/16 14:15, Philip Oakley wrote:
> From: "Duy Nguyen" <pclo...@gmail.com>
>> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <gits...@pobox.com> wrote:
>>> "Philip Oakley" <philipoak...@iee.org> writes:
>>>
>>>> int saved_namelen = saved_namelen; /* compiler workaround */
>>>>
>>>> Which then becomes an MSVC compile warning C4700: uninitialized local
>>>> variable.
>>>>
>>>> I'm wondering what was the compiler workaround being referred to? i.e. why
>>>> does it need that tweak? There's no mention of the reason in the commit
>>>> message.
>>>
>>> That was a fairly well-known workaround for GCC that issues a false
>>> warning that variable is used before initialized. I thought we
>>> stopped using it several years ago in new code after doing a bulk
>>> sanitizing
>>
>> I guess that's 803a777 (cat-file: Fix an gcc -Wuninitialized warning -
>> 2013-03-26) and more commits around that time. The split-index commit
>> is in 2014. I must have missed the trend.
>>
>>> (I think the new recommended workaround was to initialise
>>> such a variable to the nil value like '0' for integers).
>>
>> Yep. First Jeff removed the " = xxx" part from "xxx = xxx" then Ramsay
>> added the " = NULL" back. So we probably just do "int saved_namelen =
>> 0;" in this case.
>> --
> Thanks,
>
> I'll try and work up a patch - probably next week as I'm away for the weekend.
Yeah, I don't remember why these were left over from the previous
attempt to clean these up (maybe they conflicted with in-flight
topics?), but I have had a patch hanging around ... :-D
The patch below applies to master (I haven't checked for any more
additions).
ATB,
Ramsay Jones
-- >8 --
From: Ramsay Jones <ram...@ramsayjones.plus.com>
Subject: [PATCH] -Wuninitialized: remove a gcc specific workaround
Signed-off-by: Ramsay Jones <ram...@ramsayjones.plus.com>
---
builtin/rev-list.c | 2 +-
fast-import.c | 4 ++--
merge-recursive.c | 2 +-
read-cache.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 275da0d..deae1f3 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -377,7 +377,7 @@ int cmd_rev_list(int argc, const char **argv, const char
*prefix)
mark_edges_uninteresting(, show_edge);
if (bisect_list) {
- int reaches = reaches, all = all;
+ int reaches = 0, all = 0;
revs.commits = find_bisection(revs.commits, , ,
bisect_find_all);
diff --git a/fast-import.c b/fast-import.c
index 9fc7093..ca66d80 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2935,7 +2935,7 @@ static void cat_blob(struct object_entry *oe, unsigned
char sha1[20])
static void parse_get_mark(const char *p)
{
- struct object_entry *oe = oe;
+ struct object_entry *oe = NULL;
char output[42];
/* get-mark SP LF */
@@ -2952,7 +2952,7 @@ static void parse_get_mark(const char *p)
static void parse_cat_blob(const char *p)
{
- struct object_entry *oe = oe;
+ struct object_entry *oe = NULL;
unsigned char sha1[20];
/* cat-blob SP LF */
diff --git a/merge-recursive.c b/merge-recursive.c
index 06d31ed..9cecc24 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1897,7 +1897,7 @@ int merge_recursive(struct merge_options *o,
{
struct commit_list *iter;
struct commit *merged_common_ancestors;
- struct tree *mrtree = mrtree;
+ struct tree *mrtree = NULL;
int clean;
if (show(o, 4)) {
diff --git a/read-cache.c b/read-cache.c
index d9fb78b..978d6b6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1870,7 +1870,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct
cache_entry *ce,
{
int size;
struct ondisk_cache_entry *ondisk;
- int saved_namelen = saved_namelen; /* compiler workaround */
+ int saved_namelen = 0;
char *name;
int result;
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mingw: introduce the 'core.hideDotFiles' setting
On 04/05/16 15:40, Johannes Schindelin wrote: > From: Erik Faye-Lund <kusmab...@googlemail.com> > > On Unix (and Linux) it is common that files and directories whose names > start with a dot are not shown by default. This convention is used by Git: > the .git/ directory should be left alone by regular users, and only > accessed through Git itself. > > On Windows, no such convention exists. Instead, there is an explicit flag > to mark files or directories as hidden. > > In the early days, Git for Windows did not mark the .git/ directory (or > for that matter, any file or directory whose name starts with a dot) > hidden. This lead to quite a bit of confusion, and even loss of data. > > Consequently, Git for Windows introduced the core.hideDotFiles setting, > with three possible values: true, false, and dotGitOnly, defaulting to > marking only the .git/ directory as hidden. > > The rationale: users do not need to access .git/ directly, and indeed (as > was demonstrated) should not really see that directory, either. However, > not all dot files should be hidden, as e.g. Eclipse does not show them > (and the user would therefore be unable to add, say, a .gitattributes > file). > > In over five years since the last attempt to bring this patch into core > Git, this patch has served Git for Windows' users well: no single report > indicated problems with the hidden .git/ directory, and the stream of > problems caused by the previously non-hidden .git/ directory simply > stopped. > > Initial-Test-By: Pat Thoyts <pattho...@users.sourceforge.net> > Signed-off-by: Erik Faye-Lund <kusmab...@gmail.com> > Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de> > --- > > Let's try this again (I will not point you to the previous > submission, out of personal embarrassment). > > This patch has served us so well in the Git for Windows project > that there is little sense in hiding it from core Git. > > Documentation/config.txt | 6 ++ > builtin/init-db.c| 1 + > cache.h | 7 +++ > compat/mingw.c | 38 ++ > config.c | 8 > environment.c| 1 + > git-compat-util.h| 4 > t/t0001-init.sh | 30 ++ > 8 files changed, 95 insertions(+) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 42d2b50..a9f599d 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -269,6 +269,12 @@ See linkgit:git-update-index[1]. > + > The default is true (when core.filemode is not specified in the config file). > > +core.hideDotFiles:: > + (Windows-only) If true (which is the default), mark newly-created The patch (if I'm reading it correctly) and the commit message indicate that the default is 'dotGitOnly'. > + directories and files whose name starts with a dot as hidden. > + If 'dotGitOnly', only the .git/ directory is hidden, but no other > + files starting with a dot. > + ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] Documentation: fix linkgit references
On 04/05/16 09:43, Lars Schneider wrote: > > On 04 May 2016, at 10:38, larsxschnei...@gmail.com wrote: > >> From: Lars Schneider <larsxschnei...@gmail.com> >> >> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com> >> --- >> Documentation/config.txt| 4 ++-- >> Documentation/git-check-ignore.txt | 2 +- >> Documentation/git-filter-branch.txt | 4 ++-- >> Documentation/git-for-each-ref.txt | 2 +- >> 4 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/config.txt b/Documentation/config.txt >> index c7bbe98..5683400 100644 >> --- a/Documentation/config.txt >> +++ b/Documentation/config.txt >> @@ -1494,7 +1494,7 @@ gui.diffContext:: >> made by the linkgit:git-gui[1]. The default is "5". >> >> gui.displayUntracked:: >> -Determines if linkgit::git-gui[1] shows untracked files >> +Determines if linkgit:git-gui[1] shows untracked files >> in the file list. The default is "true". >> >> gui.encoding:: >> @@ -1665,7 +1665,7 @@ http.cookieFile:: >> File containing previously stored cookie lines which should be used >> in the Git http session, if they match the server. The file format >> of the file to read cookies from should be plain HTTP headers or >> -the Netscape/Mozilla cookie file format (see linkgit:curl[1]). >> +the Netscape/Mozilla cookie file format (see `curl(1)`). >> NOTE that the file specified with http.cookieFile is only used as >> input unless http.saveCookies is set. >> >> diff --git a/Documentation/git-check-ignore.txt >> b/Documentation/git-check-ignore.txt >> index e94367a..9a85998 100644 >> --- a/Documentation/git-check-ignore.txt >> +++ b/Documentation/git-check-ignore.txt >> @@ -112,7 +112,7 @@ EXIT STATUS >> SEE ALSO >> >> linkgit:gitignore[5] >> -linkgit:gitconfig[5] >> +linkgit:git-config[5] I think Junio already noted, git-config is in section 1 not 5. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
t7900-*.sh tesr #5 failure
Hi David, Test t7900.5 fails for me, thus: $ ./t7900-index-helper.sh -i -v -x -d ... + test -S .git/index-helper.sock + git status On branch master Untracked files: (use "git add ..." to include in what will be committed) err nothing added to commit but untracked files present (use "git add" to track) + test -S .git/index-helper.sock + grep -q . err error: last command exited with $?=1 not ok 5 - index-helper autorun works # # rm -f .git/index-helper.sock && # git status && # test_path_is_missing .git/index-helper.sock && # test_config indexhelper.autorun true && # git status && # test -S .git/index-helper.sock && # git status 2>err && # test -S .git/index-helper.sock && # ! grep -q . err && # git index-helper --kill && # test_config indexhelper.autorun false && # git status && # test_path_is_missing .git/index-helper.sock # $ cd trash\ directory.t7900-index-helper/ $ ls err x.t $ cat err warning: We requested watchman support from index-helper, but it doesn't support it. Please use a version of git index-helper with watchman support. $ [Yes, that is one long line in err!] [At least, this is one of the failures, I have also seen git status failing with a SIGPIPE.] Note that I do not have the watchman libraries etc., so USE_WATCHMAN is not defined. Note also, that I had an instance of git-index-helper still running after the test failure (which I kill-ed). I haven't spent any time debuging this, but some questions spring to mind: - can index-helper be used without watchman support? - why is index-helper requesting watchman support, when it was built without USE_WATCHMAN being defined? - why is read-cache.o exporting the verify_index and write_watchman_ext symbols? - is index-helper any use/help without watchman support? - is '! grep -q . err' meant to determine if the err file is empty (ie git status did not issue an error message)? [if yes, maybe 'test_must_be_empty err &&' would read better!] Unfortunately, I have to run now, ... ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 1/2] http.c: implement the GIT_TRACE_CURL environment variable
On 02/05/16 15:28, Elia Pinto wrote: [snip] > diff --git a/http.h b/http.h > index 36f558b..cd186a4 100644 > --- a/http.h > +++ b/http.h > @@ -225,4 +225,8 @@ extern int finish_http_object_request(struct > http_object_request *freq); > extern void abort_http_object_request(struct http_object_request *freq); > extern void release_http_object_request(struct http_object_request *freq); > > +/* Debug callback and setup routine for curl_easy_setopt > CURLOPT_DEBUGFUNCTION */ > +void setup_curl_trace(CURL *handle); > +int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, > void *userp); Given that you have wrapped the use of the debug callback into the setup_curl_trace() routine (good), do you still need to export the curl_trace() function? I would make that static, so that only setup_curl_trace() needs to be a public symbol. (I would also re-order the function definitions, so that setup_curl_trace() comes after curl_trace(), but that is a minor point). ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config doc: improve exit code listing
On 26/04/16 20:18, Stefan Beller wrote: > On Tue, Apr 26, 2016 at 12:11 PM, Ramsay Jones > <ram...@ramsayjones.plus.com> wrote: >> >> >> On 26/04/16 19:10, Stefan Beller wrote: >>> The possible reasons for exiting are now ordered by the exit code value. >>> While at it, rewrite the `can not write to the config file` to >>> `the config file cannot be written` to be grammatically correct and a >>> proper sentence. >>> >>> Signed-off-by: Stefan Beller <sbel...@google.com> >>> --- >>> Documentation/git-config.txt | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt >>> index 6fc08e3..6843114 100644 >>> --- a/Documentation/git-config.txt >>> +++ b/Documentation/git-config.txt >>> @@ -58,10 +58,10 @@ that location (you can say '--local' but that is the >>> default). >>> This command will fail with non-zero status upon error. Some exit >>> codes are: >>> >>> -- The config file is invalid (ret=3), >> ^ >> I don't see why this is capitalised, so ... > > Because the whole listing is a bunch of sentences, > stringed together with commas at the end of each line. > Note that there is a ',' at the end of each line, except for > the last, where you see a '.'. Heh, I hadn't noticed the commas, no - I assumed periods. > I thought about breaking that > up into a list and make all of the bullet points either a sentence > (all capitalised and ending in dot) or part sentences (lower > case for each bullet point, not clear about the ending) That's probably what I would have done. > I kept it as is in a long sentence as I expected to see > lowest resistance there. ;) 'One long sentence' split into a bullet/numbered list is err ... Hmm, I'm lost for words. ;-) >> Only a minor point. > > If the current state bothers you too much, > please send a patch with correct lists. :) > (Feel free to squash this patch into that or > just on top of this) Having said all that, (since I have very few documentation skills), I will leave this for others to improve, if necessary. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config doc: improve exit code listing
On 26/04/16 19:10, Stefan Beller wrote: > The possible reasons for exiting are now ordered by the exit code value. > While at it, rewrite the `can not write to the config file` to > `the config file cannot be written` to be grammatically correct and a > proper sentence. > > Signed-off-by: Stefan Beller <sbel...@google.com> > --- > Documentation/git-config.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index 6fc08e3..6843114 100644 > --- a/Documentation/git-config.txt > +++ b/Documentation/git-config.txt > @@ -58,10 +58,10 @@ that location (you can say '--local' but that is the > default). > This command will fail with non-zero status upon error. Some exit > codes are: > > -- The config file is invalid (ret=3), ^ I don't see why this is capitalised, so ... > -- can not write to the config file (ret=4), > +- The section or key is invalid (ret=1), ^ I don't think you should transfer the capital here. ;-) > - no section or name was provided (ret=2), > -- the section or key is invalid (ret=1), > +- the config file is invalid (ret=3), > +- the config file cannot be written (ret=4), > - you try to unset an option which does not exist (ret=5), > - you try to unset/set an option for which multiple lines match (ret=5), or > - you try to use an invalid regexp (ret=6). > Only a minor point. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote.c: spell __attribute__ correctly
On 25/04/16 22:50, Philip Oakley wrote: > From: "Jeff King" <p...@peff.net> >> On Mon, Apr 25, 2016 at 05:10:30PM -0400, Jeff King wrote: >> >>> It should be handled in git-compat-util.h, which is included by cache.h, >>> which is included by remote.c. >>> >>> There we have: >>> >>> #ifndef __GNUC__ >>> #ifndef __attribute__ >>> #define __attribute__(x) >>> #endif >>> #endif >>> >>> which should make it a noop on compilers which don't know about it. Is >>> VS (or another file) setting __GNUC__? >> >> Of course it helps if we spell the name right... Indeed! ;-) Not that it matters, but the above #define in git-compat-util.h is not the relevant definition - msvc will not see it. However, it does see the #define on line 12 of compat/msvc.h. :-D ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/83] libify apply and use lib in am
On 24/04/16 17:56, Christian Couder wrote: > On Sun, Apr 24, 2016 at 6:27 PM, Christian Couder > <christian.cou...@gmail.com> wrote: >> On Sun, Apr 24, 2016 at 5:23 PM, Ramsay Jones >> <ram...@ramsayjones.plus.com> wrote: >>> >>> >>> On 24/04/16 14:33, Christian Couder wrote: >>>> This is a patch series about libifying `git apply` functionality, and >>>> using this libified functionality in `git am`, so that no 'git apply' >>>> process is spawn anymore. This makes `git am` significantly faster, so >>>> `git rebase`, when it uses the am backend, is also significantly >>>> faster. >>> >>> I just tried to git-am these patches, but patch #78 did not make it >>> to the list. >> >> That's strange because gmail tells me it has been sent, and it made it >> to chrisc...@tuxfamily.org. > > Instead of waiting for the patch to appear on the list, you might want > to use branch libify-apply-use-in-am25 in my GitHub repo: > > https://github.com/chriscool/git/commits/libify-apply-use-in-am25 Hmm, that branch doesn't correspond directly to the patches you sent out (there are 86 commits, some marked with draft. I think commit d13d2ac corresponds kinda to patch #83, but ). I think I'll wait to see the patches as you intend them to be seen. ;-) ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
