{"thread":{"id":"31181","subject":"[PATCH 2/2] merge-recursive: eliminate flush_buffer() in favor of write_in_full()","startedAt":"2012-08-03T12:16:24Z","lastAt":"2012-08-03T12:17:58Z","messageCount":3,"participants":["Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"196409","messageId":"6668871a0573c4d82d914137e3c7ff31fa8ce1ef.1343995614.git.trast@student.ethz.ch","threadId":"31181","inReplyTo":"CAN0XMOLp_m4jUKv_=qfXdYx4jDJMBYQtY2-19sfC_7LzfY6uXA@mail.gmail.com","subject":"[PATCH 1/2] diff_setup_done(): return void","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-08-03T12:16:24Z","receivedAt":"2012-08-03T12:16:24Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"diff_setup_done() has historically returned an error code, but lost\nthe last nonzero return in 943d5b7 (allow diff.renamelimit to be set\nregardless of -M/-C, 2006-08-09).  The callers were in a pretty\nconfused state: some actually checked for the return code, and some\ndid not.\n\nLet it return void, and patch all callers to take this into account.\nThis conveniently also gets rid of a handful of different(!) error\nmessages that could never be triggered anyway.\n\nNote that the function can still die().\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\n> On Fri, Aug 3, 2012 at 10:59 AM, Thomas Rast <trast@inf.ethz.ch> wrote:\n> >\n> > Ralf Thielow <ralf.thielow@gmail.com> writes:\n> >\n> >>  #: merge-recursive.c:497\n> >> -#, fuzzy\n> >>  msgid \"diff setup failed\"\n> >> -msgstr \"diff_setup_done fehlgeschlagen\"\n> >> +msgstr \"Einrichtung der Optionen zur Bestimmung der Unterschiede \"\n> >> +\"fehlgeschlagen\"\n> >\n> > That one is really unwieldy.  Judging from a cursory look at the code,\n> > it's an internal error anyway and should probably be \"BUG:\n> > diff_setup_done failed\" to begin with.\n> >\n> \n> You're right. There are already places where the message is\n> \"diff_setup_done failed\"\n> \n> #: builtin/checkout.c:320 builtin/diff.c:302 builtin/merge.c:408\n> msgid \"diff_setup_done failed\"\n> msgstr \"diff_setup_done fehlgeschlagen\"\n> \n> We could just translate it in this way. It's not really the same message,\n> but the same meaning.\n\nI looked again.  They're entirely redundant.  Let's remove all of\nthem.\n\n builtin/blame.c    | 9 +++------\n builtin/checkout.c | 3 +--\n builtin/diff.c     | 3 +--\n builtin/merge.c    | 3 +--\n diff-no-index.c    | 3 +--\n diff.c             | 4 +---\n diff.h             | 2 +-\n line.c             | 3 +++\n merge-recursive.c  | 3 +--\n notes-merge.c      | 6 ++----\n patch-ids.c        | 3 +--\n revision.c         | 3 +--\n submodule.c        | 3 +--\n tree-diff.c        | 3 +--\n 14 files changed, 19 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bae576c..dbe969b 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -408,8 +408,7 @@ static struct origin *find_origin(struct scoreboard *sb,\n \tpaths[1] = NULL;\n \n \tdiff_tree_setup_paths(paths, &diff_opts);\n-\tif (diff_setup_done(&diff_opts) < 0)\n-\t\tdie(\"diff-setup\");\n+\tdiff_setup_done(&diff_opts);\n \n \tif (is_null_sha1(origin->commit->object.sha1))\n \t\tdo_diff_cache(parent->tree->object.sha1, &diff_opts);\n@@ -495,8 +494,7 @@ static struct origin *find_rename(struct scoreboard *sb,\n \tdiff_opts.single_follow = origin->path;\n \tpaths[0] = NULL;\n \tdiff_tree_setup_paths(paths, &diff_opts);\n-\tif (diff_setup_done(&diff_opts) < 0)\n-\t\tdie(\"diff-setup\");\n+\tdiff_setup_done(&diff_opts);\n \n \tif (is_null_sha1(origin->commit->object.sha1))\n \t\tdo_diff_cache(parent->tree->object.sha1, &diff_opts);\n@@ -1081,8 +1079,7 @@ static int find_copy_in_parent(struct scoreboard *sb,\n \n \tpaths[0] = NULL;\n \tdiff_tree_setup_paths(paths, &diff_opts);\n-\tif (diff_setup_done(&diff_opts) < 0)\n-\t\tdie(\"diff-setup\");\n+\tdiff_setup_done(&diff_opts);\n \n \t/* Try \"find copies harder\" on new path if requested;\n \t * we do not want to use diffcore_rename() actually to\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex d812219..7d922c6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -316,8 +316,7 @@ static void show_local_changes(struct object *head, struct diff_options *opts)\n \tinit_revisions(&rev, NULL);\n \trev.diffopt.flags = opts->flags;\n \trev.diffopt.output_format |= DIFF_FORMAT_NAME_STATUS;\n-\tif (diff_setup_done(&rev.diffopt) < 0)\n-\t\tdie(_(\"diff_setup_done failed\"));\n+\tdiff_setup_done(&rev.diffopt);\n \tadd_pending_object(&rev, head, NULL);\n \trun_diff_index(&rev, 0);\n }\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex bf72298..9650be2 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -302,8 +302,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \targc = setup_revisions(argc, argv, &rev, NULL);\n \tif (!rev.diffopt.output_format) {\n \t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n-\t\tif (diff_setup_done(&rev.diffopt) < 0)\n-\t\t\tdie(_(\"diff_setup_done failed\"));\n+\t\tdiff_setup_done(&rev.diffopt);\n \t}\n \n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex dd50a0c..e81fde6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -404,8 +404,7 @@ static void finish(struct commit *head_commit,\n \t\topts.output_format |=\n \t\t\tDIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n \t\topts.detect_rename = DIFF_DETECT_RENAME;\n-\t\tif (diff_setup_done(&opts) < 0)\n-\t\t\tdie(_(\"diff_setup_done failed\"));\n+\t\tdiff_setup_done(&opts);\n \t\tdiff_tree_sha1(head, new_head, \"\", &opts);\n \t\tdiffcore_std(&opts);\n \t\tdiff_flush(&opts);\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 0b46a0f..74da659 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -258,8 +258,7 @@ void diff_no_index(struct rev_info *revs,\n \tDIFF_OPT_SET(&revs->diffopt, NO_INDEX);\n \n \trevs->max_count = -2;\n-\tif (diff_setup_done(&revs->diffopt) < 0)\n-\t\tdie(\"diff_setup_done failed\");\n+\tdiff_setup_done(&revs->diffopt);\n \n \tsetup_diff_pager(&revs->diffopt);\n \tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\ndiff --git a/diff.c b/diff.c\nindex 0c1e4fc..e8678c8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3187,7 +3187,7 @@ void diff_setup(struct diff_options *options)\n \t}\n }\n \n-int diff_setup_done(struct diff_options *options)\n+void diff_setup_done(struct diff_options *options)\n {\n \tint count = 0;\n \n@@ -3286,8 +3286,6 @@ int diff_setup_done(struct diff_options *options)\n \t\toptions->output_format = DIFF_FORMAT_NO_OUTPUT;\n \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n \t}\n-\n-\treturn 0;\n }\n \n static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\ndiff --git a/diff.h b/diff.h\nindex e9284f2..d89d989 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -247,7 +247,7 @@ extern int parse_long_opt(const char *opt, const char **argv,\n extern int diff_use_color_default;\n extern void diff_setup(struct diff_options *);\n extern int diff_opt_parse(struct diff_options *, const char **, int);\n-extern int diff_setup_done(struct diff_options *);\n+extern void diff_setup_done(struct diff_options *);\n \n #define DIFF_DETECT_RENAME\t1\n #define DIFF_DETECT_COPY\t2\ndiff --git a/line.c b/line.c\nindex a008c2c..fe7ace7 100644\n--- a/line.c\n+++ b/line.c\n@@ -1177,6 +1177,9 @@ static int process_diff_filepair(struct rev_info *rev,\n \t\treturn 0;\n \tif (rg->ranges.nr == 0)\n \t\treturn 0;\n+\tif (rg->ranges.nr == 1 &&\n+\t    rg->ranges.ranges[0].start == rg->ranges.ranges[0].end)\n+\t\treturn 0;\n \n \tassert(pair->two->sha1_valid);\n \tdiff_populate_filespec(pair->two, 0);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8903a73..a7bb212 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -493,8 +493,7 @@ static struct string_list *get_renames(struct merge_options *o,\n \topts.rename_score = o->rename_score;\n \topts.show_rename_progress = o->show_rename_progress;\n \topts.output_format = DIFF_FORMAT_NO_OUTPUT;\n-\tif (diff_setup_done(&opts) < 0)\n-\t\tdie(_(\"diff setup failed\"));\n+\tdiff_setup_done(&opts);\n \tdiff_tree_sha1(o_tree->object.sha1, tree->object.sha1, \"\", &opts);\n \tdiffcore_std(&opts);\n \tif (opts.needed_rename_limit > o->needed_rename_limit)\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 29c6411..0f67bd3 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -126,8 +126,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,\n \tdiff_setup(&opt);\n \tDIFF_OPT_SET(&opt, RECURSIVE);\n \topt.output_format = DIFF_FORMAT_NO_OUTPUT;\n-\tif (diff_setup_done(&opt) < 0)\n-\t\tdie(\"diff_setup_done failed\");\n+\tdiff_setup_done(&opt);\n \tdiff_tree_sha1(base, remote, \"\", &opt);\n \tdiffcore_std(&opt);\n \n@@ -190,8 +189,7 @@ static void diff_tree_local(struct notes_merge_options *o,\n \tdiff_setup(&opt);\n \tDIFF_OPT_SET(&opt, RECURSIVE);\n \topt.output_format = DIFF_FORMAT_NO_OUTPUT;\n-\tif (diff_setup_done(&opt) < 0)\n-\t\tdie(\"diff_setup_done failed\");\n+\tdiff_setup_done(&opt);\n \tdiff_tree_sha1(base, local, \"\", &opt);\n \tdiffcore_std(&opt);\n \ndiff --git a/patch-ids.c b/patch-ids.c\nindex 5717257..bc8a28f 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -39,8 +39,7 @@ int init_patch_ids(struct patch_ids *ids)\n \tmemset(ids, 0, sizeof(*ids));\n \tdiff_setup(&ids->diffopts);\n \tDIFF_OPT_SET(&ids->diffopts, RECURSIVE);\n-\tif (diff_setup_done(&ids->diffopts) < 0)\n-\t\treturn error(\"diff_setup_done failed\");\n+\tdiff_setup_done(&ids->diffopts);\n \treturn 0;\n }\n \ndiff --git a/revision.c b/revision.c\nindex d20ff72..2253a82 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1870,8 +1870,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\trevs->topo_order = 1;\n \t}\n \n-\tif (diff_setup_done(&revs->diffopt) < 0)\n-\t\tdie(\"diff_setup_done failed\");\n+\tdiff_setup_done(&revs->diffopt);\n \n \tcompile_grep_patterns(&revs->grep_filter);\n \ndiff --git a/submodule.c b/submodule.c\nindex 959d349..19dc6a6 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -574,8 +574,7 @@ static void calculate_changed_submodule_paths(void)\n \t\t\tDIFF_OPT_SET(&diff_opts, RECURSIVE);\n \t\t\tdiff_opts.output_format |= DIFF_FORMAT_CALLBACK;\n \t\t\tdiff_opts.format_callback = submodule_collect_changed_cb;\n-\t\t\tif (diff_setup_done(&diff_opts) < 0)\n-\t\t\t\tdie(\"diff_setup_done failed\");\n+\t\t\tdiff_setup_done(&diff_opts);\n \t\t\tdiff_tree_sha1(parent->item->object.sha1, commit->object.sha1, \"\", &diff_opts);\n \t\t\tdiffcore_std(&diff_opts);\n \t\t\tdiff_flush(&diff_opts);\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 7e54833..ba01563 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -212,8 +212,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \tdiff_opts.rename_score = opt->rename_score;\n \tpaths[0] = NULL;\n \tdiff_tree_setup_paths(paths, &diff_opts);\n-\tif (diff_setup_done(&diff_opts) < 0)\n-\t\tdie(\"unable to set up diff options to follow renames\");\n+\tdiff_setup_done(&diff_opts);\n \tdiff_tree(t1, t2, base, &diff_opts);\n \tdiffcore_std(&diff_opts);\n \tdiff_tree_release_paths(&diff_opts);\n-- \n1.7.12.rc1.219.gb14e69c\n"},{"id":"196408","messageId":"853d452639066df2487b5766160ec2ebb692eab4.1343995614.git.trast@student.ethz.ch","threadId":"31181","inReplyTo":"6668871a0573c4d82d914137e3c7ff31fa8ce1ef.1343995614.git.trast@student.ethz.ch","subject":"[PATCH 2/2] merge-recursive: eliminate flush_buffer() in favor of write_in_full()","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-08-03T12:16:25Z","receivedAt":"2012-08-03T12:16:25Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"flush_buffer() is a thin wrapper around write_in_full() with two very\nconfusing properties:\n\n* It runs a loop to handle short reads, ensuring that we write\n  everything.  But that is precisely what write_in_full() does!\n\n* It checks for a return value of 0 from write_in_full(), which cannot\n  happen: it returns this value only if count=0, but flush_buffer()\n  will never call write_in_full() in this case.\n\nRemove it.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nMuch easier than worrying about the \"disk full?\" message.\n\n merge-recursive.c | 19 +------------------\n 1 file changed, 1 insertion(+), 18 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex a7bb212..3d4eb82 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -613,23 +613,6 @@ static char *unique_path(struct merge_options *o, const char *path, const char *\n \treturn newpath;\n }\n \n-static void flush_buffer(int fd, const char *buf, unsigned long size)\n-{\n-\twhile (size > 0) {\n-\t\tlong ret = write_in_full(fd, buf, size);\n-\t\tif (ret < 0) {\n-\t\t\t/* Ignore epipe */\n-\t\t\tif (errno == EPIPE)\n-\t\t\t\tbreak;\n-\t\t\tdie_errno(\"merge-recursive\");\n-\t\t} else if (!ret) {\n-\t\t\tdie(_(\"merge-recursive: disk full?\"));\n-\t\t}\n-\t\tsize -= ret;\n-\t\tbuf += ret;\n-\t}\n-}\n-\n static int dir_in_way(const char *path, int check_working_copy)\n {\n \tint pos, pathlen = strlen(path);\n@@ -788,7 +771,7 @@ static void update_file_flags(struct merge_options *o,\n \t\t\tfd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);\n \t\t\tif (fd < 0)\n \t\t\t\tdie_errno(_(\"failed to open '%s'\"), path);\n-\t\t\tflush_buffer(fd, buf, size);\n+\t\t\twrite_in_full(fd, buf, size);\n \t\t\tclose(fd);\n \t\t} else if (S_ISLNK(mode)) {\n \t\t\tchar *lnk = xmemdupz(buf, size);\n-- \n1.7.12.rc1.219.gb14e69c\n"},{"id":"196410","messageId":"87y5lw41ah.fsf@thomas.inf.ethz.ch","threadId":"31181","inReplyTo":"6668871a0573c4d82d914137e3c7ff31fa8ce1ef.1343995614.git.trast@student.ethz.ch","subject":"Re: [PATCH 1/2] diff_setup_done(): return void","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-08-03T12:17:58Z","receivedAt":"2012-08-03T12:17:58Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> diff --git a/line.c b/line.c\n> index a008c2c..fe7ace7 100644\n> --- a/line.c\n> +++ b/line.c\n> @@ -1177,6 +1177,9 @@ static int process_diff_filepair(struct rev_info *rev,\n>  \t\treturn 0;\n>  \tif (rg->ranges.nr == 0)\n>  \t\treturn 0;\n> +\tif (rg->ranges.nr == 1 &&\n> +\t    rg->ranges.ranges[0].start == rg->ranges.ranges[0].end)\n> +\t\treturn 0;\n\nThis hunk is totally unrelated, sorry for accidentally including it.\nYou can remove it before application, or I can resend.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}