Re: [PATCH v2 3/7] log: do not free parents when walking reflog

[Jeff King]

On Sun, Jul 09, 2017 at 09:59:33AM -0700, Junio C Hamano wrote:

> >> After step 6/7, we no longer "allow cycles in reflog ancestry", as
> >> there will be no reflog ancestry to speak of ;-), so it would be
> >> nice to remove the comment above in that step.  But alternatively,
> >> we can rephrase the comment here, to say something like "the same
> >> commit can be shown multiple times while showing entries from the
> >> reflog" instead.
> >
> > I actually think the comment is a bit obtuse in the first place. The
> > real issue is that we show commits multiple times. That's caused by
> > cycles, yes, but also by us clearing the SEEN flag. ;)
> >
> > Maybe this on top?
> 
> Yup, that is a much better version of what I had in mind that can go
> either before this step as a preparatory cleanup, squashed into this
> as "while at it", or after the series as a finishing touches.  The
> last one will let the codebase lie for a short while, though, so I am
> likely to squash it in or wiggle it under.

Where you put it on jk/reflog-walk in your tree is fine, though note the
commit message is slightly inaccurate there (it says "commit buffer and
parents" but at that point it is really just "commit buffer").

-Peff

Re: [PATCH v2 3/7] log: do not free parents when walking reflog

[Junio C Hamano]

Jeff King  writes:

> On Fri, Jul 07, 2017 at 10:10:54AM -0700, Junio C Hamano wrote:
>
>> > diff --git a/builtin/log.c b/builtin/log.c
>> > index 8ca1de9894..9c8bb3b5c3 100644
>> > --- a/builtin/log.c
>> > +++ b/builtin/log.c
>> > @@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)
>> >if (!rev->reflog_info) {
>> >/* we allow cycles in reflog ancestry */
>> >free_commit_buffer(commit);
>> > +  free_commit_list(commit->parents);
>> > +  commit->parents = NULL;
>> 
>> After step 6/7, we no longer "allow cycles in reflog ancestry", as
>> there will be no reflog ancestry to speak of ;-), so it would be
>> nice to remove the comment above in that step.  But alternatively,
>> we can rephrase the comment here, to say something like "the same
>> commit can be shown multiple times while showing entries from the
>> reflog" instead.
>
> I actually think the comment is a bit obtuse in the first place. The
> real issue is that we show commits multiple times. That's caused by
> cycles, yes, but also by us clearing the SEEN flag. ;)
>
> Maybe this on top?

Yup, that is a much better version of what I had in mind that can go
either before this step as a preparatory cleanup, squashed into this
as "while at it", or after the series as a finishing touches.  The
last one will let the codebase lie for a short while, though, so I am
likely to squash it in or wiggle it under.

Thanks.



>
> -- >8 --
> Subject: [PATCH] log: clarify comment about reflog cycles
>
> When we're walking reflogs, we leave the commit buffer and
> parents in place. A comment explains that this is due to
> "cycles". But the interesting thing is the unsaid
> implication: that the cycles (plus our clearing of the SEEN
> flag) will cause us to show commits multiple times. Let's
> spell it out.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/log.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 9c8bb3b5c..630d6cff2 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -372,7 +372,10 @@ static int cmd_log_walk(struct rev_info *rev)
>*/
>   rev->max_count++;
>   if (!rev->reflog_info) {
> - /* we allow cycles in reflog ancestry */
> + /*
> +  * We may show a given commit multiple times when
> +  * walking the reflogs.
> +  */
>   free_commit_buffer(commit);
>   free_commit_list(commit->parents);
>   commit->parents = NULL;

Re: [PATCH v2 3/7] log: do not free parents when walking reflog

[Jeff King]

On Fri, Jul 07, 2017 at 10:10:54AM -0700, Junio C Hamano wrote:

> > diff --git a/builtin/log.c b/builtin/log.c
> > index 8ca1de9894..9c8bb3b5c3 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)
> > if (!rev->reflog_info) {
> > /* we allow cycles in reflog ancestry */
> > free_commit_buffer(commit);
> > +   free_commit_list(commit->parents);
> > +   commit->parents = NULL;
> 
> After step 6/7, we no longer "allow cycles in reflog ancestry", as
> there will be no reflog ancestry to speak of ;-), so it would be
> nice to remove the comment above in that step.  But alternatively,
> we can rephrase the comment here, to say something like "the same
> commit can be shown multiple times while showing entries from the
> reflog" instead.

I actually think the comment is a bit obtuse in the first place. The
real issue is that we show commits multiple times. That's caused by
cycles, yes, but also by us clearing the SEEN flag. ;)

Maybe this on top?

-- >8 --
Subject: [PATCH] log: clarify comment about reflog cycles

When we're walking reflogs, we leave the commit buffer and
parents in place. A comment explains that this is due to
"cycles". But the interesting thing is the unsaid
implication: that the cycles (plus our clearing of the SEEN
flag) will cause us to show commits multiple times. Let's
spell it out.

Signed-off-by: Jeff King 
---
 builtin/log.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9c8bb3b5c..630d6cff2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -372,7 +372,10 @@ static int cmd_log_walk(struct rev_info *rev)
 */
rev->max_count++;
if (!rev->reflog_info) {
-   /* we allow cycles in reflog ancestry */
+   /*
+* We may show a given commit multiple times when
+* walking the reflogs.
+*/
free_commit_buffer(commit);
free_commit_list(commit->parents);
commit->parents = NULL;
-- 
2.13.2.1066.gabaed60bd

Re: [PATCH v2 3/7] log: do not free parents when walking reflog

[Junio C Hamano]

Jeff King  writes:

> When we're doing a reflog walk (instead of walking the
> actual parent pointers), we may see commits multiple times.
> For this reason, we hold on to the commit buffer for each
> commit rather than freeing it after we've showed the commit.
>
> We should do the same for the parent list. Right now this is
> just a minor optimization. But once we refactor how reflog
> walks are performed, keeping the parents will avoid
> confusing us the second time we see the commit.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 8ca1de9894..9c8bb3b5c3 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)
>   if (!rev->reflog_info) {
>   /* we allow cycles in reflog ancestry */
>   free_commit_buffer(commit);
> + free_commit_list(commit->parents);
> + commit->parents = NULL;

After step 6/7, we no longer "allow cycles in reflog ancestry", as
there will be no reflog ancestry to speak of ;-), so it would be
nice to remove the comment above in that step.  But alternatively,
we can rephrase the comment here, to say something like "the same
commit can be shown multiple times while showing entries from the
reflog" instead.

>   }
> - free_commit_list(commit->parents);
> - commit->parents = NULL;
>   if (saved_nrl < rev->diffopt.needed_rename_limit)
>   saved_nrl = rev->diffopt.needed_rename_limit;
>   if (rev->diffopt.degraded_cc_to_c)