threads / patch / 24904

patchhelp.c: Pull cmd_version out of this file.

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

## tl;dr

18 messages between Aug 29, 2010 and Oct 28, 2010. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Thiago Farina· Aug 29, 2010, 20:44 UTC · lore

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
Show changes to 3 files +8 −6

Makefile, builtin/version.c, help.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
Jonathan Nieder· Aug 30, 2010, 02:38 UTC · re: Thiago Farina · lore

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

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?
Jonathan Nieder· Aug 30, 2010, 02:40 UTC · re: Jonathan Nieder · lore

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

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>
Thiago Farina· Sep 1, 2010, 02:38 UTC · re: Jonathan Nieder · lore

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

On Sun, Aug 29, 2010 at 11:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 10 quoted lines
> 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.
Nguyen Thai Ngoc Duy· Sep 1, 2010, 03:04 UTC · re: Thiago Farina · lore

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

On Wed, Sep 1, 2010 at 12:38 PM, Thiago Farina <tfransosi@gmail.com> wrote:
Show 13 quoted lines
> 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
Thiago Farina· Sep 1, 2010, 03:11 UTC · re: Nguyen Thai Ngoc Duy · lore

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

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.
Junio C Hamano· Sep 1, 2010, 06:11 UTC · re: Thiago Farina · lore

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

Thiago Farina <tfransosi@gmail.com> writes:
Show 12 quoted lines
> 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...
Thiago Farina· Sep 1, 2010, 06:22 UTC · re: Junio C Hamano · lore

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

On Wed, Sep 1, 2010 at 3:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> 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.

Junio C Hamano· Sep 2, 2010, 01:04 UTC · re: Thiago Farina · lore

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

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.
Thiago Farina· Sep 2, 2010, 01:16 UTC · re: Junio C Hamano · lore

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

On Wed, Sep 1, 2010 at 10:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> 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.
Show 10 quoted lines
>        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?
Show 5 quoted lines
>>>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.
Jonathan Nieder· Sep 2, 2010, 04:35 UTC · re: Thiago Farina · lore

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

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.

Junio C Hamano· Sep 2, 2010, 16:31 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 5 quoted lines
> 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".

Jonathan Nieder· Oct 27, 2010, 15:18 UTC · re: Thiago Farina · lore

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

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?

Thiago Farina· Oct 27, 2010, 16:06 UTC · re: Jonathan Nieder · lore

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

On Wed, Oct 27, 2010 at 1:18 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 7 quoted lines
> 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?
Jonathan Nieder· Oct 27, 2010, 16:45 UTC · re: Thiago Farina · lore

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

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

Thiago Farina· Oct 28, 2010, 02:13 UTC · re: Jonathan Nieder · lore

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

On Wed, Oct 27, 2010 at 2:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 13 quoted lines
> 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.

Jonathan Nieder· Oct 28, 2010, 03:11 UTC · re: Thiago Farina · lore

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

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.

← back to recent threads