From mboxrd@z Thu Jan 1 00:00:00 1970 From: Junio C Hamano Subject: Re: [PATCH] Start conforming code to "git subcmd" style Date: Sun, 31 Aug 2008 09:14:43 -0700 Message-ID: <7vk5dxb1qk.fsf@gitster.siamese.dyndns.org> References: <20080830111253.GA9148@zakalwe.fi> <7vwshyfctu.fsf@gitster.siamese.dyndns.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Heikki Orsila , git@vger.kernel.org To: Jakub Narebski X-From: git-owner@vger.kernel.org Sun Aug 31 18:16:06 2008 Return-path: Envelope-to: gcvg-git-2@gmane.org Received: from vger.kernel.org ([209.132.176.167]) by lo.gmane.org with esmtp (Exim 4.50) id 1KZpau-0000TQ-UL for gcvg-git-2@gmane.org; Sun, 31 Aug 2008 18:16:05 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753226AbYHaQOx (ORCPT ); Sun, 31 Aug 2008 12:14:53 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753004AbYHaQOw (ORCPT ); Sun, 31 Aug 2008 12:14:52 -0400 Received: from a-sasl-quonix.sasl.smtp.pobox.com ([208.72.237.25]:54609 "EHLO sasl.smtp.pobox.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752846AbYHaQOv (ORCPT ); Sun, 31 Aug 2008 12:14:51 -0400 Received: from localhost.localdomain (localhost [127.0.0.1]) by a-sasl-quonix.sasl.smtp.pobox.com (Postfix) with ESMTP id DAEB46EC29; Sun, 31 Aug 2008 12:14:49 -0400 (EDT) Received: from pobox.com (ip68-225-240-211.oc.oc.cox.net [68.225.240.211]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by a-sasl-quonix.sasl.smtp.pobox.com (Postfix) with ESMTPSA id D55646EC26; Sun, 31 Aug 2008 12:14:45 -0400 (EDT) In-Reply-To: (Jakub Narebski's message of "Sun, 31 Aug 2008 02:32:05 -0700 (PDT)") User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux) X-Pobox-Relay-ID: EFCB1EBC-7777-11DD-90CB-3113EBD4C077-77302942!a-sasl-quonix.pobox.com Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: Jakub Narebski writes: > Junio C Hamano writes: > >> Heikki Orsila 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.