Re: [PATCH] Start conforming code to "git subcmd" style

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 31, 2008, 16:14 UTC
Message-ID: <7vk5dxb1qk.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <m38wudr0mq.fsf@localhost.localdomain>

Jakub Narebski <jnareb@gmail.com> writes:

Show 24 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Heikki Orsila <heikki.orsila@iki.fi> writes:
>> 
>> > User notifications are presented as 'git cmd', and code comments
>> > are presented as '"cmd"' or 'git's cmd', rather than 'git-cmd'.
> ...
>> > diff --git a/builtin-apply.c b/builtin-apply.c
>> > ...
>> > @@ -506,17 +506,17 @@ static char *gitdiff_verify_name(const char *li
>> > ...
>> > -			die("git-apply: bad git-diff - expected /dev/nu...
>> > +			die("git apply: bad git-diff - expected /dev/nu...
>> > ...
>> 
>> I'd vote for doing "s/git-diff/patch/" here.  After looking at
>> builtin-apply.c, there is no other error/die messages that would become
>> ambiguous, so such a rewording won't make it harder to help people who saw
>> any of these error messages (or other error messages from the "git-apply"
>> program).
>
> I agree.  git-apply in general is presented a patch output, not
> necessary git-diff output (it could be output generated by GNU diff,
> or by 'scm diff' from some SCM...).

This codepath only is reached after we determine "diff --git" header, so we are specifically expecting a patch intput with git flavor here. The original wording of the message helps somebody who diagnoses at what point in git-apply program the input is considered corrupt. My point was that that line of thinking caters to a git programmer/debugger but changing it to end user language (I suggested "patch", but I think "bad input" might be even better) would not harm the debuggability this message is giving us, because there is no similar message from the command from any other codepath.

> I think that "git foo: message" is unambiguous, and I guess _that_
> could be even in one single large patch.  Other cases I guess need
> careful review and thinking about in a case by case basis,
> unfortunately.

Yes, I think that is a very good suggestion.  Thanks.

    $ git grep -c -e '\(die\|error\|warning\)("git-[^ ]*:' 34baebc -- '*.c'
    ho/dashless:builtin-apply.c:3
    ho/dashless:builtin-checkout-index.c:3
    ho/dashless:builtin-commit-tree.c:1
    ho/dashless:builtin-fetch-pack.c:1
    ho/dashless:builtin-grep.c:1
    ho/dashless:builtin-ls-files.c:3
    ho/dashless:builtin-rm.c:2
    ho/dashless:builtin-show-ref.c:3
    ho/dashless:builtin-tar-tree.c:2
    ho/dashless:builtin-update-index.c:8
    ho/dashless:connect.c:2
    ho/dashless:entry.c:10
    ho/dashless:merge-index.c:3
    ho/dashless:tree-diff.c:1
    ho/dashless:upload-pack.c:9

I've looked at the hits from the above command (without -c) and they all looked good candidate, so I'll do that myself to reduce the amount of patches that do need thinking and inspection.