[PATCH] help.c: Pull cmd_version out of this file.

[PATCH] help.c: Pull cmd_version out of this file.

[Thiago Farina]

Promote cmd_version to a builtin, by moving it to its own file
in builtin/version.c

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 Makefile          |    1 +
 builtin/version.c |    7 +++++++
 help.c            |    6 ------
 3 files changed, 8 insertions(+), 6 deletions(-)
 create mode 100644 builtin/version.c

diff --git a/Makefile b/Makefile
index b4745a5..0f3b6d8 100644
--- a/Makefile
+++ b/Makefile
@@ -738,6 +738,7 @@ BUILTIN_OBJS += builtin/upload-archive.o
 BUILTIN_OBJS += builtin/var.o
 BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
+BUILTIN_OBJS += builtin/version.o
 BUILTIN_OBJS += builtin/write-tree.o
 
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
diff --git a/builtin/version.c b/builtin/version.c
new file mode 100644
index 0000000..d98e260
--- /dev/null
+++ b/builtin/version.c
@@ -0,0 +1,7 @@
+#include "builtin.h"
+
+int cmd_version(int argc, const char **argv, const char *prefix)
+{
+	printf("git version %s\n", git_version_string);
+	return 0;
+}
diff --git a/help.c b/help.c
index 7f4928e..ad4f923 100644
--- a/help.c
+++ b/help.c
@@ -362,9 +362,3 @@ const char *help_unknown_cmd(const char *cmd)
 
 	exit(1);
 }
-
-int cmd_version(int argc, const char **argv, const char *prefix)
-{
-	printf("git version %s\n", git_version_string);
-	return 0;
-}
-- 
1.7.2.1.95.g3d045

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Jonathan Nieder]

Thiago Farina wrote:

> Promote cmd_version to a builtin, by moving it to its own file
> in builtin/version.c

Could you explain further?  If the goal is "one command per source
file", then we already violate that in a number of places:

 - "git blame" and "git pickaxe" are both cmd_blame() in
   builtin/blame.c.

 - "git whatchanged", "git show", "git reflog", and "git log" are
   separate builtins in builtin/log.c.

 - etc

What does this make easier?

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Jonathan Nieder]

Jonathan Nieder wrote:
> Thiago Farina wrote:
>
>> Promote cmd_version to a builtin, by moving it to its own file
>> in builtin/version.c
[...]
> What does this make easier?

To answer my own question: it moves the code to the builtin/
subdirectory.

 $ git grep -F -e cmd_version -- builtin;	# before
 $ git grep -F -e cmd_version -- builtin;	# after
 builtin/version.c:int cmd_version(int argc, const char **argv, const char *prefix)

So for what it's worth,

Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Thiago Farina]

On Sun, Aug 29, 2010 at 11:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>> Thiago Farina wrote:
>>
>>> Promote cmd_version to a builtin, by moving it to its own file
>>> in builtin/version.c
> [...]
> So for what it's worth,
>
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>

Ping Junio.

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Nguyen Thai Ngoc Duy]

On Wed, Sep 1, 2010 at 12:38 PM, Thiago Farina <tfransosi@gmail.com> wrote:
> On Sun, Aug 29, 2010 at 11:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Jonathan Nieder wrote:
>>> Thiago Farina wrote:
>>>
>>>> Promote cmd_version to a builtin, by moving it to its own file
>>>> in builtin/version.c
>> [...]
>> So for what it's worth,
>>
>> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>>
>
> Ping Junio.

Not my call, but should you also update .gitignore to ignore git-version?
-- 
Duy

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Thiago Farina]

On Wed, Sep 1, 2010 at 12:04 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> Not my call, but should you also update .gitignore to ignore git-version?

Thanks for the catch, fixed in patchset v2.

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Junio C Hamano]

Thiago Farina <tfransosi@gmail.com> writes:

> On Sun, Aug 29, 2010 at 11:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Jonathan Nieder wrote:
>>> Thiago Farina wrote:
>>>
>>>> Promote cmd_version to a builtin, by moving it to its own file
>>>> in builtin/version.c
>> [...]
>> So for what it's worth,
>>
>> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Ping Junio.

If the patch were to also move git_version_string[] from git.c to this new
file, whose sole purpose will be to give the version information, then it
might be worth it, as we would need to recompile a file with only 10 lines
instead of git.c with 570 lines, but what the patch does is not even that.

Not that I would be happy to see such a change that affects dependency and
risk causing broken builds this late in the cycle, though.

I ended up spending 10+ minutes re-checking how the dependency between the
version string and the resulting binaries work.  The patch probably would
cause "git clean" to report a new unignored file, too.

What did we gain through this exercise?  Did we fix any real problem?

"git grep" sees directory boundaries through just fine, and I do not find
Jonathan's explanation very satisfactory.


		Junio, who gets a lot more grumpy than necessary at night...

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Thiago Farina]

On Wed, Sep 1, 2010 at 3:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> If the patch were to also move git_version_string[] from git.c to this new
> file, whose sole purpose will be to give the version information, then it
> might be worth it, as we would need to recompile a file with only 10 lines
> instead of git.c with 570 lines, but what the patch does is not even that.
>
It doesn't do that, but if you suggest that, I can fix it up later and
resend it.

>risk causing broken builds?
Hum, how such trivial change can cause that?

>What did we gain through this exercise?  Did we fix any real problem?
All patches should just fix real problems?

I think it just makes more sense, since *all* other commands are in
builtin/ and it's own files.

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Junio C Hamano]

Thiago Farina <tfransosi@gmail.com> writes:

>>risk causing broken builds?
> Hum, how such trivial change can cause that?

Imagine if we had this in help.c:

        int cmd_version(int argc, const char **argv, const char *prefix)
        {
                printf("git version %s\n", GIT_VERSION);
                return 0;
        }

which were compiled with this rule in Makefile:

	help.o: help.c GIT-VERSION-FILE

and then you moved the function to a new file without touching the
dependency.  Especially because the change is trivial (just moving a
function from here to there), you don't immediately notice the broken
dependency.

I just spent 15+ minutes explaining this to you (and another 10+ minutes
checking that the above is _not_ the actual case before giving the same
explanation to you last night).  Now is this still a trivial change that
costs nothing?

>>What did we gain through this exercise?  Did we fix any real problem?
> All patches should just fix real problems?

There is a "cost vs benefit" ratio you need to consider.

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Thiago Farina]

On Wed, Sep 1, 2010 at 10:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thiago Farina <tfransosi@gmail.com> writes:
>
>>>risk causing broken builds?
>> Hum, how such trivial change can cause that?
>
> Imagine if we had this in help.c:
>
But we don't have.

>        int cmd_version(int argc, const char **argv, const char *prefix)
>        {
>                printf("git version %s\n", GIT_VERSION);
>                return 0;
>        }
>
> which were compiled with this rule in Makefile:
>
>        help.o: help.c GIT-VERSION-FILE
>
But it's not.

> and then you moved the function to a new file without touching the
> dependency.
That would be a mistake no?

>  Especially because the change is trivial (just moving a
> function from here to there), you don't immediately notice the broken
> dependency.
>
As far as I know that does not go unnoticed.


> I just spent 15+ minutes explaining this to you (and another 10+ minutes
> checking that the above is _not_ the actual case before giving the same
> explanation to you last night).
Thanks for being so careful.

>  Now is this still a trivial change that
> costs nothing?
>
Why you impose so much obstacles to a trivial change like this?

>>>What did we gain through this exercise?  Did we fix any real problem?
>> All patches should just fix real problems?
>
> There is a "cost vs benefit" ratio you need to consider.
>
Thanks.

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Jonathan Nieder]

Thiago Farina wrote:

> All patches should just fix real problems?

Yes.

> I think it just makes more sense, since *all* other commands are in
> builtin/ and it's own files.

I was careless before and was not even thinking about

 BUILT_INS += $(patsubst builtin/%.o,git-%$X,$(BUILTIN_OBJS))

Sorry.

With that line in mind, what Junio wrote in the "what's cooking"
message makes a lot of sense.  A good direction to go in long term
might be to freeze the list of built-ins installed to $gitexecdir in
some clean way and avoid adding new ones.

Would you be interested in working on that?  That would solve a
problem (turning back the tide of scripts that use git- commands,
without breaking compatibility) that some people are interested in.
The hardest part would probably be to make sure commands like
"git help version" work properly.

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Thiago Farina wrote:
>
>> All patches should just fix real problems?
>
> Yes.

This answer while it is correct needs to be qualified ;-)

A bugfix for end-user facing part of the system is obviously a fix for a
"real problem", but clean-up without changing external behaviour to reduce
maintenance burden in the longer term also is fixing a real problem, i.e.
"the code, while it might be bug-free, is error prone and costs a lot to
maintain".

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Thiago Farina]

What happened to this patch? I can't see it on any branch (master, pu, maint).

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Jonathan Nieder]

Thiago Farina wrote:

> What happened to this patch? I can't see it on any branch (master, pu, maint).

I thought there was some discussion about another binary in
/usr/lib/git-core being a bad thing?

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Thiago Farina]

On Wed, Oct 27, 2010 at 1:18 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Thiago Farina wrote:
>
>> What happened to this patch? I can't see it on any branch (master, pu, maint).
>
> I thought there was some discussion about another binary in
> /usr/lib/git-core being a bad thing?
>

I don't recall, why it's a bad thing?

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Jonathan Nieder]

Thiago Farina wrote:
> On Wed, Oct 27, 2010 at 1:18 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> I thought there was some discussion about another binary in
>> /usr/lib/git-core being a bad thing?
>
> I don't recall, why it's a bad thing?

Because those hardlinks for builtins are present for historical reasons
only.  They slow down filename lookup by some miniscule amount and tempt
authors of scripts to try the dashed form of commands.

Ah, found it.[1]

| At this point, adding the dashed-form "git-version" to the filesystem is
| going backwards.  These files are only to help people who have old scripts
| from pre 1.6.0 days with "PATH=$(git --exec-path):$PATH" with their
| environment.
| 
| We should instead try to not adding them (which would probably need tweak
| in the command list generation code in "git help"), as nobody from pre
| 1.6.0 era would have called "git-version" binary.

Hope that helps.

[1] 4a7a62f3:whats-cooking.txt
http://thread.gmane.org/gmane.comp.version-control.git/155073

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Thiago Farina]

On Wed, Oct 27, 2010 at 2:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ah, found it.[1]
>
> | At this point, adding the dashed-form "git-version" to the filesystem is
> | going backwards.  These files are only to help people who have old scripts
> | from pre 1.6.0 days with "PATH=$(git --exec-path):$PATH" with their
> | environment.
> |
> | We should instead try to not adding them (which would probably need tweak
> | in the command list generation code in "git help"), as nobody from pre
> | 1.6.0 era would have called "git-version" binary.
>
> Hope that helps.
>
Nope, sorry. I don't fully understand his explanation.

Also, Junio, thanks for the "I don't like
churning-for-the-sake-of-churning". This is very incentive.

Re: [PATCH] help.c: Pull cmd_version out of this file.

[Jonathan Nieder]

Thiago Farina wrote:

> Nope, sorry. I don't fully understand his explanation.

Any questions then?

> Also, Junio, thanks for the "I don't like
> churning-for-the-sake-of-churning". This is very incentive.

For the scenario "one person likes this change and another doesn't",
in the Linux kernel world (which I find amusing to take as a model),
there is a well established method to deal with that.  (I'm a big fan
of this method.)  It works like this:

 1. Write a patch.
 2. Send a copy to the list and get feedback.
 3. Use it locally.  Foist it on your friends.  Accept bug reports
    and keep a git tree to handle them.  When conscience or users
    provide the pressure for it, move on to step 4:
 4. Send another report to the list and get more feedback.
    ...
 n. (Ideally) the patch evolves to be more useful.  Eventually, it
    gets applied upstream or, if upstream is out of touch, the
    patched version becomes the new mainstream.

Why am I saying such things?  Isn't it extreme to ask you to take
matters into you own hands, especially for such an unrisky patch?

What I want to get at is that to make your work available, you don't
need Junio.  You can do that yourself.  It can still be very useful
to people!  What Junio offers is help in maintaining code --- once a
patch hits mainline, there is no need to keep forward-porting it, you
are less alone in dealing with bug reports, you will find more people
out there to give advice in changing it for new requirements.

So when someone like Junio says

 I don't like churning for the sake of churning

part of what this means is that in order to take on new code, there
has to be an obvious benefit that outweighs the maintenance burden.
(Keep in mind: "obvious" doesn't mean "big", it just means "clear".)

What maintenance burden?  Here, I thought I had explained that if
it weren't for existing scripts, there would not be more than one
hardlink for builtins in /usr/lib/git-core at all.  Those hardlinks
are technically just bad --- they require munging the PATH and
basing behavior on argv[0], they are confusing, their command-line
syntax is not as flexible as the git <options> foo syntax, and use
of them makes it hard to grep for a real bug that some old programs
have of assuming dashed-form commands are in the user's $PATH.

So your patch is exciting in a way, because it serves as a reminder
of a problem it would be nice to solve (that each new builtin adds
to the builtins in /usr/lib/git-core for no good reason and that
'git help' is relying on that).

Sorry to be vague, hope that helps.