Re: [PATCH 1/2] merge: setup `opts` later in `checkout_fast_forward()`

[Johannes Schindelin]

Hi Martin,

On Tue, 24 Apr 2018, Martin Ågren wrote:

> On 24 April 2018 at 08:20, Jacob Keller  wrote:
> > I'm guessing the diff algorithm simply found that this was a more
> > compact representation of the change? It's a bit confusing when your
> > description indicates you "moved" some code down, but it looks like
> > you moved code up.
> 
> Agreed. I'll play with --anchored and other magic stuff to see if I can
> improve this. Or I could instead try to sell this patch as "move some
> other stuff out of the way" ;-) That seems a bit less direct though.

Or you could add a remark to the commit message along the lines "best
viewed with `--anchored=...`". This is what I would do ;-)

Ciao,
Dscho

Re: [PATCH 1/2] merge: setup `opts` later in `checkout_fast_forward()`

[Martin Ågren]

On 24 April 2018 at 08:20, Jacob Keller  wrote:
> I'm guessing the diff algorithm simply found that this was a more
> compact representation of the change? It's a bit confusing when your
> description indicates you "moved" some code down, but it looks like
> you moved code up.

Agreed. I'll play with --anchored and other magic stuff to see if I can
improve this. Or I could instead try to sell this patch as "move some
other stuff out of the way" ;-) That seems a bit less direct though.

Thanks
Martin

Re: [PATCH 1/2] merge: setup `opts` later in `checkout_fast_forward()`

[Jacob Keller]

On Mon, Apr 23, 2018 at 10:13 PM, Martin Ågren  wrote:
> After we initialize the various fields in `opts` but before we actually
> use them, we might return early. Move the initialization further down,
> to immediately before we use `opts`.
>
> This limits the scope of `opts` and will help a subsequent commit fix a
> memory leak without having to worry about those early returns.
>
> Signed-off-by: Martin Ågren 
> ---
>  merge.c | 32 +---
>  1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/merge.c b/merge.c
> index f06a4773d4..f123658e58 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -94,8 +94,24 @@ int checkout_fast_forward(const struct object_id *head,
> return -1;
>
> memset(, 0, sizeof(trees));
> -   memset(, 0, sizeof(opts));
> memset(, 0, sizeof(t));
> +
> +   trees[nr_trees] = parse_tree_indirect(head);
> +   if (!trees[nr_trees++]) {
> +   rollback_lock_file(_file);
> +   return -1;
> +   }
> +   trees[nr_trees] = parse_tree_indirect(remote);
> +   if (!trees[nr_trees++]) {
> +   rollback_lock_file(_file);
> +   return -1;
> +   }
> +   for (i = 0; i < nr_trees; i++) {
> +   parse_tree(trees[i]);
> +   init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
> +   }
> +
> +   memset(, 0, sizeof(opts));
> if (overwrite_ignore) {
> memset(, 0, sizeof(dir));

I'm guessing the diff algorithm simply found that this was a more
compact representation of the change? It's a bit confusing when your
description indicates you "moved" some code down, but it looks like
you moved code up.

Thanks,
Jake

> dir.flags |= DIR_SHOW_IGNORED;
> @@ -112,20 +128,6 @@ int checkout_fast_forward(const struct object_id *head,
> opts.fn = twoway_merge;
> setup_unpack_trees_porcelain(, "merge");
>
> -   trees[nr_trees] = parse_tree_indirect(head);
> -   if (!trees[nr_trees++]) {
> -   rollback_lock_file(_file);
> -   return -1;
> -   }
> -   trees[nr_trees] = parse_tree_indirect(remote);
> -   if (!trees[nr_trees++]) {
> -   rollback_lock_file(_file);
> -   return -1;
> -   }
> -   for (i = 0; i < nr_trees; i++) {
> -   parse_tree(trees[i]);
> -   init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
> -   }
> if (unpack_trees(nr_trees, t, )) {
> rollback_lock_file(_file);
> return -1;
> --
> 2.17.0
>

[PATCH 1/2] merge: setup `opts` later in `checkout_fast_forward()`

[Martin Ågren]

After we initialize the various fields in `opts` but before we actually
use them, we might return early. Move the initialization further down,
to immediately before we use `opts`.

This limits the scope of `opts` and will help a subsequent commit fix a
memory leak without having to worry about those early returns.

Signed-off-by: Martin Ågren 
---
 merge.c | 32 +---
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/merge.c b/merge.c
index f06a4773d4..f123658e58 100644
--- a/merge.c
+++ b/merge.c
@@ -94,8 +94,24 @@ int checkout_fast_forward(const struct object_id *head,
return -1;
 
memset(, 0, sizeof(trees));
-   memset(, 0, sizeof(opts));
memset(, 0, sizeof(t));
+
+   trees[nr_trees] = parse_tree_indirect(head);
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
+   return -1;
+   }
+   trees[nr_trees] = parse_tree_indirect(remote);
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
+   return -1;
+   }
+   for (i = 0; i < nr_trees; i++) {
+   parse_tree(trees[i]);
+   init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
+   }
+
+   memset(, 0, sizeof(opts));
if (overwrite_ignore) {
memset(, 0, sizeof(dir));
dir.flags |= DIR_SHOW_IGNORED;
@@ -112,20 +128,6 @@ int checkout_fast_forward(const struct object_id *head,
opts.fn = twoway_merge;
setup_unpack_trees_porcelain(, "merge");
 
-   trees[nr_trees] = parse_tree_indirect(head);
-   if (!trees[nr_trees++]) {
-   rollback_lock_file(_file);
-   return -1;
-   }
-   trees[nr_trees] = parse_tree_indirect(remote);
-   if (!trees[nr_trees++]) {
-   rollback_lock_file(_file);
-   return -1;
-   }
-   for (i = 0; i < nr_trees; i++) {
-   parse_tree(trees[i]);
-   init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
-   }
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
return -1;
-- 
2.17.0