Re: [PATCH 5/5] merge-recursive: simplify handle_change_delete

[Elijah Newren]

Hi Dscho,

On Mon, May 21, 2018 at 6:41 AM, Johannes Schindelin
 wrote:
> On Sat, 19 May 2018, Elijah Newren wrote:
>> On Sat, May 19, 2018 at 12:32 AM, Johannes Sixt  wrote:

>> >
>> > Oh, there is a reason for the repeated message text: translations! Please 
>> > do
>> > not play sentence Lego with translated strings. The original code is
>> > preferable.
>>
>> Ah, translations; that makes sense now.  I'm still annoyed by the
>> code, but I retract patch 5 then.
>
> Maybe you can remove the temptation for others, too, my replacing your
> patch 5 with one that adds the code comment "No sentence Lego! Think of
> our poor translators and refrain from making their life miserable!" or
> some more appropriate one?

That sounds reasonable.  I'll make that change (plus your other
suggested changes on other patches; thanks for all the reviews) in the
next round.

I'm also considering adopting a setup_unpack_trees_porcelain() style
of handling (from unpack-trees.c) for all these messages in
merge-recursive.c, but that's a bigger change that I'd probably put in
a separate series if I decide to go with it.

Elijah

Re: [PATCH 5/5] merge-recursive: simplify handle_change_delete

[Johannes Schindelin]

Hi Elijah,

On Sat, 19 May 2018, Elijah Newren wrote:

> On Sat, May 19, 2018 at 12:32 AM, Johannes Sixt  wrote:
> > Am 19.05.2018 um 04:07 schrieb Elijah Newren:
> >>
> >> There is really no need for four branches of nearly identical messages
> >> when we can store the differences into small variables before printing.
> >
> > Oh, there is a reason for the repeated message text: translations! Please do
> > not play sentence Lego with translated strings. The original code is
> > preferable.
> 
> Ah, translations; that makes sense now.  I'm still annoyed by the
> code, but I retract patch 5 then.

Maybe you can remove the temptation for others, too, my replacing your
patch 5 with one that adds the code comment "No sentence Lego! Think of
our poor translators and refrain from making their life miserable!" or
some more appropriate one?

Ciao,
Dscho

Re: [PATCH 5/5] merge-recursive: simplify handle_change_delete

[Elijah Newren]

On Sat, May 19, 2018 at 12:32 AM, Johannes Sixt  wrote:
> Am 19.05.2018 um 04:07 schrieb Elijah Newren:
>>
>> There is really no need for four branches of nearly identical messages
>> when we can store the differences into small variables before printing.
>
> Oh, there is a reason for the repeated message text: translations! Please do
> not play sentence Lego with translated strings. The original code is
> preferable.

Ah, translations; that makes sense now.  I'm still annoyed by the
code, but I retract patch 5 then.

Re: [PATCH 5/5] merge-recursive: simplify handle_change_delete

[Johannes Sixt]

Am 19.05.2018 um 04:07 schrieb Elijah Newren:

There is really no need for four branches of nearly identical messages
when we can store the differences into small variables before printing.


Oh, there is a reason for the repeated message text: translations! 
Please do not play sentence Lego with translated strings. The original 
code is preferable.



It does require a few allocations this way, but makes the code much
easier to parse for human readers.

Signed-off-by: Elijah Newren 
---
  merge-recursive.c | 36 +++-
  1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 273ee79afa..3bd727995b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1290,31 +1290,17 @@ static int handle_change_delete(struct merge_options *o,
if (!ret)
ret = update_file(o, 0, o_oid, o_mode, update_path);
} else {
-   if (!alt_path) {
-   if (!old_path) {
-   output(o, 1, _("CONFLICT (%s/delete): %s deleted in 
%s "
-  "and %s in %s. Version %s of %s left in 
tree."),
-  change, path, delete_branch, change_past,
-  change_branch, change_branch, path);
-   } else {
-   output(o, 1, _("CONFLICT (%s/delete): %s deleted in 
%s "
-  "and %s to %s in %s. Version %s of %s left in 
tree."),
-  change, old_path, delete_branch, 
change_past, path,
-  change_branch, change_branch, path);
-   }
-   } else {
-   if (!old_path) {
-   output(o, 1, _("CONFLICT (%s/delete): %s deleted in 
%s "
-  "and %s in %s. Version %s of %s left in tree 
at %s."),
-  change, path, delete_branch, change_past,
-  change_branch, change_branch, path, 
alt_path);
-   } else {
-   output(o, 1, _("CONFLICT (%s/delete): %s deleted in 
%s "
-  "and %s to %s in %s. Version %s of %s left in 
tree at %s."),
-  change, old_path, delete_branch, 
change_past, path,
-  change_branch, change_branch, path, 
alt_path);
-   }
-   }
+   const char *deleted_path = old_path ? old_path : path;
+   char *supp1 = xstrfmt(old_path ? " to %s" : "", path);
+   char *supp2 = xstrfmt(alt_path ? " at %s" : "", alt_path);
+
+   output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
+  "and %s%s in %s. Version %s of %s left in tree%s."),
+  change, deleted_path, delete_branch, change_past,
+  supp1, change_branch, change_branch, path, supp2);
+   free(supp1);
+   free(supp2);
+
/*
 * No need to call update_file() on path when change_branch ==
 * o->branch1 && !alt_path, since that would needlessly touch

[PATCH 5/5] merge-recursive: simplify handle_change_delete

[Elijah Newren]

There is really no need for four branches of nearly identical messages
when we can store the differences into small variables before printing.
It does require a few allocations this way, but makes the code much
easier to parse for human readers.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 36 +++-
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 273ee79afa..3bd727995b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1290,31 +1290,17 @@ static int handle_change_delete(struct merge_options *o,
if (!ret)
ret = update_file(o, 0, o_oid, o_mode, update_path);
} else {
-   if (!alt_path) {
-   if (!old_path) {
-   output(o, 1, _("CONFLICT (%s/delete): %s 
deleted in %s "
-  "and %s in %s. Version %s of %s left in 
tree."),
-  change, path, delete_branch, change_past,
-  change_branch, change_branch, path);
-   } else {
-   output(o, 1, _("CONFLICT (%s/delete): %s 
deleted in %s "
-  "and %s to %s in %s. Version %s of %s 
left in tree."),
-  change, old_path, delete_branch, 
change_past, path,
-  change_branch, change_branch, path);
-   }
-   } else {
-   if (!old_path) {
-   output(o, 1, _("CONFLICT (%s/delete): %s 
deleted in %s "
-  "and %s in %s. Version %s of %s left in 
tree at %s."),
-  change, path, delete_branch, change_past,
-  change_branch, change_branch, path, 
alt_path);
-   } else {
-   output(o, 1, _("CONFLICT (%s/delete): %s 
deleted in %s "
-  "and %s to %s in %s. Version %s of %s 
left in tree at %s."),
-  change, old_path, delete_branch, 
change_past, path,
-  change_branch, change_branch, path, 
alt_path);
-   }
-   }
+   const char *deleted_path = old_path ? old_path : path;
+   char *supp1 = xstrfmt(old_path ? " to %s" : "", path);
+   char *supp2 = xstrfmt(alt_path ? " at %s" : "", alt_path);
+
+   output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
+  "and %s%s in %s. Version %s of %s left in tree%s."),
+  change, deleted_path, delete_branch, change_past,
+  supp1, change_branch, change_branch, path, supp2);
+   free(supp1);
+   free(supp2);
+
/*
 * No need to call update_file() on path when change_branch ==
 * o->branch1 && !alt_path, since that would needlessly touch
-- 
2.17.0.847.g20b8963732