From: Jakub Narebski <jnareb@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>,
git@vger.kernel.org, Stefan Beller <sbeller@google.com>,
Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 1/7] replace: "libify" create_graft()
Date: Fri, 20 Apr 2018 13:51:34 +0200 [thread overview]
Message-ID: <86zi1ydrjt.fsf@gmail.com> (raw)
In-Reply-To: <xmqqh8o6d8rg.fsf@gitster-ct.c.googlers.com> (Junio C. Hamano's message of "Fri, 20 Apr 2018 09:25:07 +0900")
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> It is quite convenient to simply die() in builtins, in the absence of
>> proper exception handling, because it allows us to just go belly up
>> without having to implement error handling chains.
>>
>> Of course, for reusable library functions, this is a big no-no, so we
>> (try to) restrict the usage of die() to one-shot commands, i.e. places
>> where we know that the caller does not want to, say, give the user a
>> useful high-level error message, i.e. a message that the function calling
>> die() could not possibly know.
>>
>> The problem with this reasoning is that sooner or later, pretty much all
>> useful functions will *need* to be libified: the more useful a function,
>> the more likely it is to be called from a call chain where the outer
>> function implements a high-level operation that needs to provide
>> additional advice to the user in case of failure.
>>
>> This is the case here: the create_graft() function is useful enough to be
>> called in a loop, say, in the upcoming patch to convert a graft file in
>> one fell swoop. Therefore, this function must not be allowed to die(), nor
>> any of its callees.
>
> All of the first three paragraphs are already widely known to the
> project, and I do not think you need to verbosely repeat it, if
> brevity demands it.
[taking the role of Advocatus Diaboli]
Well, this information (first three paragraphs) is maybe widely known to
the project, but do we have it explicitely documented somwehere, like
CodingGuidelines, or FAQ on some web site?
> One thing that the proposed log message for this step would be far
> more useful to say is that the current callers of create_graft() is
> fine with the behaviour change (i.e. prepared to act on an error
> return).
Right.
>
> Also it may want to say why the other die() we see in this function
> in the pre-context is OK. I actually think it is not OK and should
> return error(not a valid object name) the same way (I suspect that
> this is mere omission/mistake, and you did not intend to leave it
> dying). As long as the caller is prepared to deal with an error
> return, that is, which was the previous point.
[taking the role of Advocatus Diaboli]
Maybe they are errors that caller cannot handle sanely in any way?
If I understand things correctly the problem is with parameter to the
--graft option...
>
> FWIW, I fully agree with the goal of making this (or other pieces
> that would be useful if they were reusable) reusable.
>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> builtin/replace.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/replace.c b/builtin/replace.c
>> index 935647be6bd..43264f0998e 100644
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -395,7 +395,9 @@ static int create_graft(int argc, const char **argv, int force)
>>
>> if (get_oid(old_ref, &old_oid) < 0)
>> die(_("Not a valid object name: '%s'"), old_ref);
>> - commit = lookup_commit_or_die(&old_oid, old_ref);
>> + commit = lookup_commit_reference(&old_oid);
>> + if (!commit)
>> + return error(_("could not parse %s"), old_ref);
>>
>> buffer = get_commit_buffer(commit, &size);
>> strbuf_add(&buf, buffer, size);
next prev parent reply other threads:[~2018-04-20 11:51 UTC|newest]
Thread overview: 115+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-13 11:11 [PATCH] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-13 18:22 ` Stefan Beller
2018-04-13 22:35 ` Johannes Schindelin
2018-04-13 22:45 ` Stefan Beller
2018-04-19 8:15 ` Johannes Schindelin
2018-04-13 18:57 ` Eric Sunshine
2018-04-19 8:14 ` Johannes Schindelin
2018-04-19 8:17 ` [PATCH v2 0/7] Deprecate .git/info/grafts Johannes Schindelin
2018-04-19 8:17 ` [PATCH v2 1/7] replace: "libify" create_graft() Johannes Schindelin
2018-04-20 0:25 ` Junio C Hamano
2018-04-20 11:51 ` Jakub Narebski [this message]
2018-04-20 15:32 ` Johannes Schindelin
2018-04-19 8:17 ` [PATCH v2 2/7] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-19 10:09 ` Christian Couder
2018-04-20 13:57 ` Johannes Schindelin
2018-04-20 1:05 ` Junio C Hamano
2018-04-20 15:26 ` Johannes Schindelin
2018-04-19 8:18 ` [PATCH v2 3/7] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-20 1:21 ` Junio C Hamano
2018-04-20 15:31 ` Johannes Schindelin
2018-04-19 8:18 ` [PATCH v2 4/7] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-19 8:18 ` [PATCH v2 5/7] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-19 8:19 ` [PATCH v2 6/7] technical/shallow: describe the relationship with replace refs Johannes Schindelin
2018-04-19 8:21 ` [PATCH v2 7/7] technical/shallow: describe why shallow cannot use " Johannes Schindelin
2018-04-20 22:20 ` [PATCH v3 00/11] Deprecate .git/info/grafts Johannes Schindelin
2018-04-20 22:20 ` [PATCH v3 01/11] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-20 23:29 ` Stefan Beller
2018-04-21 9:39 ` Johannes Schindelin
2018-04-20 22:21 ` [PATCH v3 02/11] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-20 22:21 ` [PATCH v3 03/11] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-20 22:21 ` [PATCH v3 04/11] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-20 22:22 ` [PATCH v3 05/11] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-20 22:23 ` [PATCH v3 06/11] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-21 6:20 ` SZEDER Gábor
2018-04-21 9:42 ` Johannes Schindelin
2018-04-20 22:24 ` [PATCH v3 07/11] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-20 22:25 ` [PATCH v3 08/11] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-20 22:26 ` [PATCH v3 09/11] technical/shallow: describe the relationship with replace refs Johannes Schindelin
2018-04-22 15:16 ` Philip Oakley
2018-04-24 19:10 ` Johannes Schindelin
2018-04-24 21:34 ` Philip Oakley
2018-04-25 0:40 ` Junio C Hamano
2018-04-25 7:17 ` Johannes Schindelin
2018-04-20 22:26 ` [PATCH v3 10/11] technical/shallow: describe why shallow cannot use " Johannes Schindelin
2018-04-20 22:27 ` [PATCH v3 11/11] Remove obsolete script to convert grafts to " Johannes Schindelin
2018-04-21 9:43 ` [PATCH v4 00/11] Deprecate .git/info/grafts Johannes Schindelin
2018-04-21 9:46 ` [PATCH v4 01/11] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-24 1:15 ` Junio C Hamano
2018-04-24 2:38 ` Junio C Hamano
2018-04-21 9:46 ` [PATCH v4 02/11] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-21 9:47 ` [PATCH v4 03/11] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-21 9:47 ` [PATCH v4 04/11] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-23 19:08 ` Stefan Beller
2018-04-24 18:51 ` Johannes Schindelin
2018-04-24 19:03 ` Stefan Beller
2018-04-25 7:10 ` Johannes Schindelin
2018-04-21 9:48 ` [PATCH v4 05/11] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-22 3:30 ` Eric Sunshine
2018-04-24 19:04 ` Johannes Schindelin
2018-04-21 9:48 ` [PATCH v4 06/11] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-21 9:49 ` [PATCH v4 07/11] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-21 9:49 ` [PATCH v4 08/11] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-21 9:51 ` [PATCH v4 09/11] technical/shallow: describe the relationship with replace refs Johannes Schindelin
2018-04-21 9:56 ` [PATCH v4 10/11] technical/shallow: describe why shallow cannot use " Johannes Schindelin
2018-04-21 9:56 ` [PATCH v4 11/11] Remove obsolete script to convert grafts to " Johannes Schindelin
2018-04-23 19:24 ` [PATCH v4 00/11] Deprecate .git/info/grafts Stefan Beller
2018-04-25 9:53 ` [PATCH v5 " Johannes Schindelin
2018-04-25 9:53 ` [PATCH v5 01/11] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 02/11] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 03/11] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 04/11] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 05/11] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 06/11] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 07/11] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 08/11] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 09/11] technical/shallow: stop referring to grafts Johannes Schindelin
2018-04-25 12:25 ` Jakub Narębski
2018-04-26 9:19 ` Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 10/11] technical/shallow: describe why shallow cannot use replace refs Johannes Schindelin
2018-04-25 9:54 ` [PATCH v5 11/11] Remove obsolete script to convert grafts to " Johannes Schindelin
2018-04-26 4:10 ` [PATCH v5 00/11] Deprecate .git/info/grafts Junio C Hamano
2018-04-27 21:03 ` Johannes Schindelin
2018-04-27 21:39 ` [PATCH v6 " Johannes Schindelin
2018-04-27 21:39 ` [PATCH v6 01/11] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-27 21:39 ` [PATCH v6 02/11] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-27 21:39 ` [PATCH v6 03/11] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-27 21:39 ` [PATCH v6 04/11] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-27 21:39 ` [PATCH v6 05/11] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-27 21:39 ` [PATCH v6 06/11] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-28 1:25 ` SZEDER Gábor
2018-04-28 13:07 ` Johannes Schindelin
2018-04-27 21:39 ` [PATCH v6 07/11] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-27 21:39 ` [PATCH v6 08/11] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-27 21:40 ` [PATCH v6 09/11] technical/shallow: stop referring to grafts Johannes Schindelin
2018-04-27 21:40 ` [PATCH v6 10/11] technical/shallow: describe why shallow cannot use replace refs Johannes Schindelin
2018-04-27 21:40 ` [PATCH v6 11/11] Remove obsolete script to convert grafts to " Johannes Schindelin
2018-04-28 9:04 ` Philip Oakley
2018-04-28 13:13 ` Johannes Schindelin
2018-04-28 15:00 ` Stefan Beller
2018-04-28 22:47 ` [Re-send PATCH v7 00/12] Deprecate .git/info/grafts Johannes Schindelin
2018-04-28 22:44 ` [PATCH v7 01/12] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-28 22:44 ` [PATCH v7 02/12] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-28 22:44 ` [PATCH v7 03/12] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-28 22:44 ` [PATCH v7 04/12] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-28 22:44 ` [PATCH v7 05/12] replace: prepare create_graft() for converting graft files wholesale Johannes Schindelin
2018-04-28 22:44 ` [PATCH v7 06/12] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-28 22:44 ` [PATCH v7 07/12] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-28 22:44 ` [PATCH v7 08/12] Deprecate support for .git/info/grafts Johannes Schindelin
2018-11-27 20:12 ` [PATCH] advice: don't pointlessly suggest --convert-graft-file Ævar Arnfjörð Bjarmason
2018-11-28 6:34 ` Junio C Hamano
2018-11-28 9:03 ` Johannes Schindelin
2018-04-28 22:44 ` [PATCH v7 09/12] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-28 22:45 ` [PATCH v7 10/12] technical/shallow: stop referring to grafts Johannes Schindelin
2018-04-28 22:45 ` [PATCH v7 11/12] technical/shallow: describe why shallow cannot use replace refs Johannes Schindelin
2018-04-28 22:45 ` [PATCH v7 12/12] Remove obsolete script to convert grafts to " Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86zi1ydrjt.fsf@gmail.com \
--to=jnareb@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=sbeller@google.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).