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

[Jacob Keller]

On Thu, May 17, 2018 at 2:48 PM, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> 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 later commit fix a
>> memory leak without having to worry about those early returns.
>>
>> This patch is best viewed using something like this (note the tab!):
>> --color-moved --anchored="trees[nr_trees] = parse_tree_indirect"
>
> This side remark is interesting because it totally depends on how
> you look at it.  I think "initialize opts late" and "attempt to
> parse the trees first and fail early" are the sides of the same
> coin, and the diff shown without the anchor matches the latter,
> which is also perfectly acceptable interpretation of what this patch
> does.
>

Yes. I like that we have tools available to show diffs in different
hopefully meaningful ways.

I happen to like when the diff matches my mental map of the change
after reading the commit message, so having the author indicate how
best to view it is useful, but definitely cool to see that we can get
different interpretations.

Thanks,
Jake

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

[Junio C Hamano]

Martin Ågren  writes:

> 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 later commit fix a
> memory leak without having to worry about those early returns.
>
> This patch is best viewed using something like this (note the tab!):
> --color-moved --anchored="trees[nr_trees] = parse_tree_indirect"

This side remark is interesting because it totally depends on how
you look at it.  I think "initialize opts late" and "attempt to
parse the trees first and fail early" are the sides of the same
coin, and the diff shown without the anchor matches the latter,
which is also perfectly acceptable interpretation of what this patch
does.

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

[Jacob Keller]

On Wed, May 16, 2018 at 12:29 PM, Martin Ågren  wrote:
> On 16 May 2018 at 18:41, Stefan Beller  wrote:
>> On Wed, May 16, 2018 at 9:30 AM, Martin Ågren  wrote:
>>>
>>> This patch is best viewed using something like this (note the tab!):
>>> --color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"
>>
>> Heh! Having a "is best viewed" paragraph is the new shiny thing in
>> commit messages as 'git log origin/pu --grep "is best viewed"' tells me.
>
> :-)
>
>> Regarding the anchoring, I wonder if we can improve it by ignoring
>> whitespaces or just looking for substrings, or by allowing regexes or ...
>
> FWIW, because my first naive attempt failed (for some reason I did not
> consider the leading tab part of the "line" so I did not provide it), I
> had the same thought. Ignoring leading whitespace seemed easy enough in
> the implementation.
>
> Then I started thinking about all the ways in which whitespace can be
> ignored. My reaction in the end was to not try and open that can right
> there and then. I did not think about regexes.
>
> I guess this boils down to the usage. Copying the line to anchor on from
> an editor could run into these kind of whitespace-issues, and shell
> escaping. Typing an anchor could become easier with regexes since one
> could skip typing common substrings and just anchor on /unique-part/.
>
> Martin

Simpler approach is to just match substring instead. Then, the user
can decide how much of the string is required to get the anchor they
wanted.

Thanks,
Jake

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

[Jacob Keller]

On Wed, May 16, 2018 at 9:41 AM, Stefan Beller  wrote:
> + Jonathan Tan for a side discussion on anchoring.
>
> On Wed, May 16, 2018 at 9:30 AM, Martin Ågren  wrote:
>>
>> This patch is best viewed using something like this (note the tab!):
>> --color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"
>
> Heh! Having a "is best viewed" paragraph is the new shiny thing in
> commit messages as 'git log origin/pu --grep "is best viewed"' tells me.
>
> Regarding the anchoring, I wonder if we can improve it by ignoring
> whitespaces or just looking for substrings, or by allowing regexes or ...
>
> Thanks,
> Stefan

I think expanding it to be regexp would be nicest. To be honest, I
already thought it was substring based

It'd be *really* cool if we had a way for a commit messages (or maybe
notes?) to indicate the anchor so that git show could (optionally)
figure out the anchor automatically. It's been REALLY useful for me
when showing diffs to be able to provide a better idea of what a human
*actually* did vs what the smallest diff was.

Thanks,
Jake

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

[Martin Ågren]

On 16 May 2018 at 18:41, Stefan Beller  wrote:
> On Wed, May 16, 2018 at 9:30 AM, Martin Ågren  wrote:
>>
>> This patch is best viewed using something like this (note the tab!):
>> --color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"
>
> Heh! Having a "is best viewed" paragraph is the new shiny thing in
> commit messages as 'git log origin/pu --grep "is best viewed"' tells me.

:-)

> Regarding the anchoring, I wonder if we can improve it by ignoring
> whitespaces or just looking for substrings, or by allowing regexes or ...

FWIW, because my first naive attempt failed (for some reason I did not
consider the leading tab part of the "line" so I did not provide it), I
had the same thought. Ignoring leading whitespace seemed easy enough in
the implementation.

Then I started thinking about all the ways in which whitespace can be
ignored. My reaction in the end was to not try and open that can right
there and then. I did not think about regexes.

I guess this boils down to the usage. Copying the line to anchor on from
an editor could run into these kind of whitespace-issues, and shell
escaping. Typing an anchor could become easier with regexes since one
could skip typing common substrings and just anchor on /unique-part/.

Martin

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

[Stefan Beller]

+ Jonathan Tan for a side discussion on anchoring.

On Wed, May 16, 2018 at 9:30 AM, Martin Ågren  wrote:
>
> This patch is best viewed using something like this (note the tab!):
> --color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"

Heh! Having a "is best viewed" paragraph is the new shiny thing in
commit messages as 'git log origin/pu --grep "is best viewed"' tells me.

Regarding the anchoring, I wonder if we can improve it by ignoring
whitespaces or just looking for substrings, or by allowing regexes or ...

Thanks,
Stefan

[PATCH v2 1/3] 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 later commit fix a
memory leak without having to worry about those early returns.

This patch is best viewed using something like this (note the tab!):
--color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"

Signed-off-by: Martin Ågren 
---
 merge.c | 34 ++
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/merge.c b/merge.c
index f06a4773d4..f123658e58 100644
--- a/merge.c
+++ b/merge.c
@@ -94,23 +94,7 @@ int checkout_fast_forward(const struct object_id *head,
return -1;
 
memset(, 0, sizeof(trees));
-   memset(, 0, sizeof(opts));
memset(, 0, sizeof(t));
-   if (overwrite_ignore) {
-   memset(, 0, sizeof(dir));
-   dir.flags |= DIR_SHOW_IGNORED;
-   setup_standard_excludes();
-   opts.dir = 
-   }
-
-   opts.head_idx = 1;
-   opts.src_index = _index;
-   opts.dst_index = _index;
-   opts.update = 1;
-   opts.verbose_update = 1;
-   opts.merge = 1;
-   opts.fn = twoway_merge;
-   setup_unpack_trees_porcelain(, "merge");
 
trees[nr_trees] = parse_tree_indirect(head);
if (!trees[nr_trees++]) {
@@ -126,6 +110,24 @@ int checkout_fast_forward(const struct object_id *head,
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;
+   setup_standard_excludes();
+   opts.dir = 
+   }
+
+   opts.head_idx = 1;
+   opts.src_index = _index;
+   opts.dst_index = _index;
+   opts.update = 1;
+   opts.verbose_update = 1;
+   opts.merge = 1;
+   opts.fn = twoway_merge;
+   setup_unpack_trees_porcelain(, "merge");
+
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
return -1;
-- 
2.17.0.583.g9a75a153ac