[PATCH] convert: mark a file-local symbol static

[PATCH] convert: mark a file-local symbol static

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@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.<driver>.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: [PATCH] convert: mark a file-local symbol static

[Lars Schneider]

> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> If you need to re-roll your 'ls/filter-process' branch, could you
> please squash this into commit 85290197
> ("convert: add filter.<driver>.process option", 08-10-2016).
> 
> 
> -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?

Thanks,
Lars

Re: [PATCH] convert: mark a file-local symbol static

[Ramsay Jones]

On 15/10/16 16:05, Lars Schneider wrote:
>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@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

Re: [PATCH] convert: mark a file-local symbol static

[Lars Schneider]

> On 15 Oct 2016, at 14:01, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> 
> On 15/10/16 16:05, Lars Schneider wrote:
>>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@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?
Does it generate false positives? 
Maybe I can add `make sparse` to the TravisCI build?

Cheers,
Lars

Re: [PATCH] convert: mark a file-local symbol static

[Ramsay Jones]

[-- Attachment #1: Type: text/plain, Size: 4128 bytes --]



On 16/10/16 01:15, Lars Schneider wrote:
>> On 15 Oct 2016, at 14:01, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>> On 15/10/16 16:05, Lars Schneider wrote:
>>>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@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.9030308@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



[-- Attachment #2: static-check.pl --]
[-- Type: application/x-perl, Size: 1333 bytes --]

Re: [PATCH] convert: mark a file-local symbol static

[Jeff King]

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.

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.

-Peff

Re: [PATCH] convert: mark a file-local symbol static

[Johannes Schindelin]

Hi Ramsay & Peff,

On Sun, 16 Oct 2016, 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.

You could also use the method I use in Git for Windows to "extend" the
Makefile:

-- snipsnap --
cat >dummy.mak <<EOF
include Makefile

blub: $(OBJECTS)
	do-something-with $^
EOF

make -f dummy.mak blub

Re: [PATCH] convert: mark a file-local symbol static

[Jeff King]

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 <<EOF
> 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.

-Peff

Re: [PATCH] convert: mark a file-local symbol static

[Ramsay Jones]

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

[Ramsay Jones]

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 <<EOF
>> 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

[Junio C Hamano]

Ramsay Jones <ramsay@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?

Re: [PATCH] convert: mark a file-local symbol static

[René Scharfe]

Am 17.10.2016 um 22:07 schrieb Junio C Hamano:
> Ramsay Jones <ramsay@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?

This removes config.mak as well.

René

Re: [PATCH] convert: mark a file-local symbol static

[Stefan Beller]

On Mon, Oct 17, 2016 at 1:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ramsay Jones <ramsay@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 only want to cleanup compiled stuff and such, not the precious
unversioned text files I have laying around here. So I guess

  git clean -x -e:(attr:!binary)

would suffice, though. ;)

Re: [PATCH] convert: mark a file-local symbol static

[Ramsay Jones]

On 17/10/16 21:07, Junio C Hamano wrote:
> Ramsay Jones <ramsay@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