Re: Truncating HEAD reflog on branch move

[Junio C Hamano]

Jeff King  writes:

> On Tue, Jul 04, 2017 at 07:58:06PM +, brian m. carlson wrote:
>
>> > And here's one more patch on top of those that's necessary to get the
>> > tests to pass (I don't expect anybody to necessarily be applying this
>> > slow string of patches; it's just to show the direction I'm looking in).
>> 
>> I've looked at your original patch, which modified reflog-walk.c, and it
>> does fix the issue.  I'm happy to send in a patch with that and a test
>> (provided you're okay with me adding your sign-off), or if you wanted to
>> send in something a bit more complete, like the series of patches here,
>> that's fine, too.
>
> I've been on vacation for the past week, but wrapping this up is on my
> todo. I'll try to get to it tonight.

;-)  Welcome back.  

Hopefully I haven't advanced topics to 'next' that are sub-par
during your absence.

Re: Truncating HEAD reflog on branch move

[Jeff King]

On Tue, Jul 04, 2017 at 09:27:09PM +, brian m. carlson wrote:

> On Tue, Jul 04, 2017 at 05:24:08PM -0400, Jeff King wrote:
> > On Tue, Jul 04, 2017 at 07:58:06PM +, brian m. carlson wrote:
> > > I've looked at your original patch, which modified reflog-walk.c, and it
> > > does fix the issue.  I'm happy to send in a patch with that and a test
> > > (provided you're okay with me adding your sign-off), or if you wanted to
> > > send in something a bit more complete, like the series of patches here,
> > > that's fine, too.
> > 
> > I've been on vacation for the past week, but wrapping this up is on my
> > todo. I'll try to get to it tonight.
> 
> Okay, if it's on your radar, that's fine.  Certainly you should enjoy
> your vacation and not feel obligated to work on Git unless you want to.

Heh, no, "tonight" is the first work day after the vacation ends. Once
the fireworks are done, it's back to work. ;)

-Peff

Re: Truncating HEAD reflog on branch move

[brian m. carlson]

On Tue, Jul 04, 2017 at 05:24:08PM -0400, Jeff King wrote:
> On Tue, Jul 04, 2017 at 07:58:06PM +, brian m. carlson wrote:
> > I've looked at your original patch, which modified reflog-walk.c, and it
> > does fix the issue.  I'm happy to send in a patch with that and a test
> > (provided you're okay with me adding your sign-off), or if you wanted to
> > send in something a bit more complete, like the series of patches here,
> > that's fine, too.
> 
> I've been on vacation for the past week, but wrapping this up is on my
> todo. I'll try to get to it tonight.

Okay, if it's on your radar, that's fine.  Certainly you should enjoy
your vacation and not feel obligated to work on Git unless you want to.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: Truncating HEAD reflog on branch move

[Jeff King]

On Tue, Jul 04, 2017 at 07:58:06PM +, brian m. carlson wrote:

> > And here's one more patch on top of those that's necessary to get the
> > tests to pass (I don't expect anybody to necessarily be applying this
> > slow string of patches; it's just to show the direction I'm looking in).
> 
> I've looked at your original patch, which modified reflog-walk.c, and it
> does fix the issue.  I'm happy to send in a patch with that and a test
> (provided you're okay with me adding your sign-off), or if you wanted to
> send in something a bit more complete, like the series of patches here,
> that's fine, too.

I've been on vacation for the past week, but wrapping this up is on my
todo. I'll try to get to it tonight.

-Peff

Re: Truncating HEAD reflog on branch move

[brian m. carlson]

On Thu, Jun 22, 2017 at 11:13:15PM -0400, Jeff King wrote:
> On Thu, Jun 22, 2017 at 06:25:46PM -0400, Jeff King wrote:
> 
> > So here's a patch on top of what I posted before that pushes the reflog
> > check into the loop (it just decides whether to pull from the reflogs or
> > from the commit queue at the top of the loop).
> > 
> > I was surprised to find, though, that simplify_commit() does not
> > actually do the pathspec limiting! It's done by
> > try_to_simplify_commit(), which is part of add_parents_to_list(). I
> > hacked around it in the later part of the loop by calling
> > try_to_simplify myself and checking the TREESAME flag. But I have a
> > feeling I'm missing something about how the traversal is supposed to
> > work.
> > 
> > This does behave sensibly with "--no-merges" and "--merges", as well as
> > pathspec limiting.
> 
> And here's one more patch on top of those that's necessary to get the
> tests to pass (I don't expect anybody to necessarily be applying this
> slow string of patches; it's just to show the direction I'm looking in).

I've looked at your original patch, which modified reflog-walk.c, and it
does fix the issue.  I'm happy to send in a patch with that and a test
(provided you're okay with me adding your sign-off), or if you wanted to
send in something a bit more complete, like the series of patches here,
that's fine, too.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: Truncating HEAD reflog on branch move

[Jeff King]

On Thu, Jun 22, 2017 at 06:25:46PM -0400, Jeff King wrote:

> So here's a patch on top of what I posted before that pushes the reflog
> check into the loop (it just decides whether to pull from the reflogs or
> from the commit queue at the top of the loop).
> 
> I was surprised to find, though, that simplify_commit() does not
> actually do the pathspec limiting! It's done by
> try_to_simplify_commit(), which is part of add_parents_to_list(). I
> hacked around it in the later part of the loop by calling
> try_to_simplify myself and checking the TREESAME flag. But I have a
> feeling I'm missing something about how the traversal is supposed to
> work.
> 
> This does behave sensibly with "--no-merges" and "--merges", as well as
> pathspec limiting.

And here's one more patch on top of those that's necessary to get the
tests to pass (I don't expect anybody to necessarily be applying this
slow string of patches; it's just to show the direction I'm looking in).

---
 builtin/log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 998437b23..512538479 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -373,9 +373,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;
}
-   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)

Re: Truncating HEAD reflog on branch move

[Jeff King]

On Thu, Jun 22, 2017 at 05:52:35PM -0400, Jeff King wrote:

> Which really makes me feel like this patch is going in the right
> direction, as it makes all of this behave conceptually like:
> 
>   while read old new etc ...
>   do
> git show $new
>   done <.git/logs/$ref
> 
> which is simple to explain and is what I'd expect (and is certainly the
> direction we went with the "diff uses real parents" commit).
> 
> We just need to hit the simplify_commit() code path here.

So here's a patch on top of what I posted before that pushes the reflog
check into the loop (it just decides whether to pull from the reflogs or
from the commit queue at the top of the loop).

I was surprised to find, though, that simplify_commit() does not
actually do the pathspec limiting! It's done by
try_to_simplify_commit(), which is part of add_parents_to_list(). I
hacked around it in the later part of the loop by calling
try_to_simplify myself and checking the TREESAME flag. But I have a
feeling I'm missing something about how the traversal is supposed to
work.

This does behave sensibly with "--no-merges" and "--merges", as well as
pathspec limiting.

diff --git a/revision.c b/revision.c
index 675247cd9..203468ddf 100644
--- a/revision.c
+++ b/revision.c
@@ -3112,19 +3112,19 @@ static void track_linear(struct rev_info *revs, struct 
commit *commit)
 
 static struct commit *get_revision_1(struct rev_info *revs)
 {
-   if (revs->reflog_info) {
-   struct commit *commit = next_reflog_entry(revs->reflog_info);
-   if (commit) {
-   commit->object.flags &= ~(ADDED | SEEN | SHOWN);
-   return commit;
-   }
-   }
+   while (1) {
+   struct commit *commit;
 
-   if (!revs->commits)
-   return NULL;
+   if (revs->reflog_info)
+   commit = next_reflog_entry(revs->reflog_info);
+   else
+   commit = pop_commit(>commits);
 
-   do {
-   struct commit *commit = pop_commit(>commits);
+   if (!commit)
+   return NULL;
+
+   if (revs->reflog_info)
+   commit->object.flags &= ~(ADDED | SEEN | SHOWN);
 
/*
 * If we haven't done the list limiting, we need to look at
@@ -3135,7 +3135,8 @@ static struct commit *get_revision_1(struct rev_info 
*revs)
if (revs->max_age != -1 &&
(commit->date < revs->max_age))
continue;
-   if (add_parents_to_list(revs, commit, >commits, 
NULL) < 0) {
+   if (!revs->reflog_info &&
+   add_parents_to_list(revs, commit, >commits, 
NULL) < 0) {
if (!revs->ignore_missing_links)
die("Failed to traverse parents of 
commit %s",

oid_to_hex(>object.oid));
@@ -3151,10 +3152,14 @@ static struct commit *get_revision_1(struct rev_info 
*revs)
default:
if (revs->track_linear)
track_linear(revs, commit);
+   if (revs->reflog_info) {
+   try_to_simplify_commit(revs, commit);
+   if (commit->object.flags & TREESAME)
+   continue;
+   }
return commit;
}
-   } while (revs->commits);
-   return NULL;
+   }
 }
 
 /*

Re: Truncating HEAD reflog on branch move

[Jeff King]

On Thu, Jun 22, 2017 at 01:32:44PM -0700, Junio C Hamano wrote:

> I do not think command line parser does not allow "log -g
> maint..master" so all the "limited" processing the remainder of
> get_revision_1() does shouldn't matter.

Yeah, I don't think negative endpoints work at all, and "foo...bar"
seems to also break (though with a confusing message). It seems clear to
me that multiple positive endpoints don't work well either, if they have
overlapping commits.

> I however think pathspec will affect simplify_commit() and suspect
> that "git log -g -20 HEAD path" will behave differently.  Perhaps
> the difference is "it used to use path in an unexplainable way, now
> it ignores", in which case this is an improvement.

The current behavior there does seem like nonsense, because it's based
on the fake parents. For instance, if I set up a simple two-branch case:

  commit() {
echo "$1" >"$1" && git add "$1" && git commit -m "$1"
  }

  git init repo
  cd repo
  commit base
  commit master
  git checkout -b side HEAD^
  commit side
  git merge --no-edit master
  commit combined

Then I get:

  $ git log -g --oneline --name-status -- master
  f06c3cd HEAD@{1}: merge master: Merge made by the 'recursive' strategy.
  5bf12c4 HEAD@{3}: checkout: moving from master to side
  dfa408b HEAD@{4}: commit: master
  A   master

Even though only one of those commits touched master. But with my patch,
it's also somewhat confusing. We ignore the pathspec when picking which
commits to show, but still apply it for diffing. So:

  03cf1ad HEAD@{0}: commit: combined
  f06c3cd HEAD@{1}: merge master: Merge made by the 'recursive' strategy.
  277042b HEAD@{2}: commit: side
  5bf12c4 HEAD@{3}: checkout: moving from master to side
  dfa408b HEAD@{4}: commit: master
  A   master
  5bf12c4 HEAD@{5}: commit (initial): base

I think we'd want to just omit any entries that are TREESAME to their
parents. We don't actually care about true parent rewriting (since we're
not walking the parents), but if it happened as a side effect that would
probably be OK.

It looks like simplify_commit() is also where we apply bits like
--no-merges, which doesn't work with my patch. It _also_ behaves
nonsensically in the current code, because of course the fake reflog
parents are never merges.

Which really makes me feel like this patch is going in the right
direction, as it makes all of this behave conceptually like:

  while read old new etc ...
  do
git show $new
  done <.git/logs/$ref

which is simple to explain and is what I'd expect (and is certainly the
direction we went with the "diff uses real parents" commit).

We just need to hit the simplify_commit() code path here.

-Peff

Re: Truncating HEAD reflog on branch move

[Junio C Hamano]

Jeff King  writes:

> So I'd be tempted to just ditch the whole thing and teach
> get_revision_1() to just walk through the list of logs, rather than this
> weird "add a pending commit and then try to figure out which reflog it
> referred to". For instance, right now:
>
>   git log -g HEAD $(git symbolic-ref HEAD)
>
> only shows _one_ reflog. The patch below is the direction I'm thinking.
> It fails two tests, but haven't dug yet.
>
> ---
>  reflog-walk.c | 112 +--
>  reflog-walk.h |   4 +-
>  revision.c|  24 
>  3 files changed, 43 insertions(+), 97 deletions(-)

Yeah, I agree with the "we now show diffs with true parents"
reasoning, and I like the above code reduction, obviously ;-)

> @@ -3114,18 +3112,20 @@ static void track_linear(struct rev_info *revs, 
> struct commit *commit)
>  
>  static struct commit *get_revision_1(struct rev_info *revs)
>  {
> + if (revs->reflog_info) {
> + struct commit *commit = next_reflog_entry(revs->reflog_info);
> + if (commit) {
> + commit->object.flags &= ~(ADDED | SEEN | SHOWN);
> + return commit;
> + }
> + }
> +
>   if (!revs->commits)
>   return NULL;
>  
>   do {
>   struct commit *commit = pop_commit(>commits);
>  
> - if (revs->reflog_info) {
> - save_parents(revs, commit);
> - fake_reflog_parent(revs->reflog_info, commit);
> - commit->object.flags &= ~(ADDED | SEEN | SHOWN);
> - }
> -
>   /*
>* If we haven't done the list limiting, we need to look at
>* the parents here. We also need to do the date-based limiting

This part of the patch I can 100% agree with ;-)  

I do not think command line parser does not allow "log -g
maint..master" so all the "limited" processing the remainder of
get_revision_1() does shouldn't matter.

I however think pathspec will affect simplify_commit() and suspect
that "git log -g -20 HEAD path" will behave differently.  Perhaps
the difference is "it used to use path in an unexplainable way, now
it ignores", in which case this is an improvement.

Thanks.

Re: Truncating HEAD reflog on branch move

[Jeff King]

On Thu, Jun 22, 2017 at 12:03:31PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I don't think this is quite right, though. We've decremented "recno"
> > after assigning the old pointer to "reflog". So in the existing code,
> > "reflog" in that second conditional pointing to the _next_ entry (or
> > previous, really, since we are going in reverse order).
> >
> > So I think you'd need to look at commit->reflog again (after checking
> > that we didn't go past the start of the array).
> 
> Perhaps.  I did the illustration that way simply because I was not
> sure if the current "the entry was NULL from something new, so skip
> and look at the previous entry's new" was correct to begin with.

I'm not sure it makes sense for an entry to have its "after" state as
its own parent. On the other hand, I'm not sure it makes sense to
consider this fake parentage in the first place.

I think in the old days we used truly rewrite the parents, and traversal
like "git log -g -p" would show the diff between reflog entries. But
that changed in 838f9a156 (log: use true parents for diff when walking
reflogs, 2013-08-03). Given that we disable reflog-walking with things
like "--graph" that show the relationship, are the fake parents actually
accomplishing anything anymore? The parents we show via "log -g
--parents" seem like nonsense, and I'm not sure how we would do any
history simplification based no the munged values.

So I'd be tempted to just ditch the whole thing and teach
get_revision_1() to just walk through the list of logs, rather than this
weird "add a pending commit and then try to figure out which reflog it
referred to". For instance, right now:

  git log -g HEAD $(git symbolic-ref HEAD)

only shows _one_ reflog. The patch below is the direction I'm thinking.
It fails two tests, but haven't dug yet.

---
 reflog-walk.c | 112 +--
 reflog-walk.h |   4 +-
 revision.c|  24 
 3 files changed, 43 insertions(+), 97 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index ed99437ad..77170a3cb 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -78,45 +78,6 @@ static int get_reflog_recno_by_time(struct complete_reflogs 
*array,
return -1;
 }
 
-struct commit_info_lifo {
-   struct commit_info {
-   struct commit *commit;
-   void *util;
-   } *items;
-   int nr, alloc;
-};
-
-static struct commit_info *get_commit_info(struct commit *commit,
-   struct commit_info_lifo *lifo, int pop)
-{
-   int i;
-   for (i = 0; i < lifo->nr; i++)
-   if (lifo->items[i].commit == commit) {
-   struct commit_info *result = >items[i];
-   if (pop) {
-   if (i + 1 < lifo->nr)
-   memmove(lifo->items + i,
-   lifo->items + i + 1,
-   (lifo->nr - i) *
-   sizeof(struct commit_info));
-   lifo->nr--;
-   }
-   return result;
-   }
-   return NULL;
-}
-
-static void add_commit_info(struct commit *commit, void *util,
-   struct commit_info_lifo *lifo)
-{
-   struct commit_info *info;
-   ALLOC_GROW(lifo->items, lifo->nr + 1, lifo->alloc);
-   info = lifo->items + lifo->nr;
-   info->commit = commit;
-   info->util = util;
-   lifo->nr++;
-}
-
 struct commit_reflog {
int recno;
enum selector_type {
@@ -128,7 +89,8 @@ struct commit_reflog {
 };
 
 struct reflog_walk_info {
-   struct commit_info_lifo reflogs;
+   struct commit_reflog **logs;
+   size_t nr, alloc, cur;
struct string_list complete_reflogs;
struct commit_reflog *last_commit_reflog;
 };
@@ -226,50 +188,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
commit_reflog->selector = selector;
commit_reflog->reflogs = reflogs;
 
-   add_commit_info(commit, commit_reflog, >reflogs);
-   return 0;
-}
-
-void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)
-{
-   struct commit_info *commit_info =
-   get_commit_info(commit, >reflogs, 0);
-   struct commit_reflog *commit_reflog;
-   struct object *logobj;
-   struct reflog_info *reflog;
-
-   info->last_commit_reflog = NULL;
-   if (!commit_info)
-   return;
+   ALLOC_GROW(info->logs, info->nr + 1, info->alloc);
+   info->logs[info->nr++] = commit_reflog;
 
-   commit_reflog = commit_info->util;
-   if (commit_reflog->recno < 0) {
-   commit->parents = NULL;
-   return;
-   }
-   info->last_commit_reflog = commit_reflog;
-
-   do {
-   reflog = _reflog->reflogs->items[commit_reflog->recno];
-   commit_reflog->recno--;
-  

Re: Truncating HEAD reflog on branch move

[Junio C Hamano]

Jeff King  writes:

> I don't think this is quite right, though. We've decremented "recno"
> after assigning the old pointer to "reflog". So in the existing code,
> "reflog" in that second conditional pointing to the _next_ entry (or
> previous, really, since we are going in reverse order).
>
> So I think you'd need to look at commit->reflog again (after checking
> that we didn't go past the start of the array).

Perhaps.  I did the illustration that way simply because I was not
sure if the current "the entry was NULL from something new, so skip
and look at the previous entry's new" was correct to begin with.

Re: Truncating HEAD reflog on branch move

[Jeff King]

On Thu, Jun 22, 2017 at 11:32:43AM -0700, Junio C Hamano wrote:

> > diff --git a/reflog-walk.c b/reflog-walk.c
> > index ed99437ad..b7e489ad3 100644
> > --- a/reflog-walk.c
> > +++ b/reflog-walk.c
> > @@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, 
> > struct commit *commit)
> > /* a root commit, but there are still more entries to show */
> > reflog = _reflog->reflogs->items[commit_reflog->recno];
> > logobj = parse_object(>noid);
> > +   if (!logobj)
> > +   logobj = parse_object(>ooid);
> > }
> >  
> > if (!logobj || logobj->type != OBJ_COMMIT) {
> 
> We already have a loop to find an entry that is a commit that
> discards any non-commit object before the pre-context of this hunk.
> This "oops, old side is NULL so let's cover it up by using the new
> side" kicks in after that.  I wonder if we can roll that cover-up
> logic into the loop, perhaps like

Yeah, I tried making the second conditional its own loop, but I agree it
probably ought to be part of a single loop looking for a sane parent
entry.

>   do {
>   reflog = _reflog->...[recno];
>   commit_reflog->recno--;
> - logobj = parse_object(>ooid);
> + logobj = parse_object(is_null_oid(>ooid) 
> + ? >noid : >ooid);
> - } while (commit_reflog->recno && (logobj && logobj->type != 
> OBJ_COMMIT));
> + } while (commit_reflog->recno && (!logobj || logobj->type != 
> OBJ_COMMIT));
> -
> - if (!logobj && commit_reflog->recno >= 0 && is_null_oid(>ooid)) 
> {
> - /* a root commit ... */
> - reflog = _reflog->...[recno];
> - logobj = parse_object(>noid);
> - }

I don't think this is quite right, though. We've decremented "recno"
after assigning the old pointer to "reflog". So in the existing code,
"reflog" in that second conditional pointing to the _next_ entry (or
previous, really, since we are going in reverse order).

So I think you'd need to look at commit->reflog again (after checking
that we didn't go past the start of the array).

-Peff

Re: Truncating HEAD reflog on branch move

[Junio C Hamano]

Jeff King  writes:

> There's some magic in fake_reflog_parent() when we hit an entry with a
> ...
> This whole fake-parents things does just feel like a gigantic hack,
> though.
> ...
> It seems like we should be able to just walk backwards down the
> reflog list and show the entries. The revision machinery already
> special-cases a bunch of reflog-walk bits; I don't know that adding one
> or two more would be the end of the world.

Unfortunate but I tend to agree that at least such an addition would
make the "gigantic hack" a bit more complete one ;-)

> diff --git a/reflog-walk.c b/reflog-walk.c
> index ed99437ad..b7e489ad3 100644
> --- a/reflog-walk.c
> +++ b/reflog-walk.c
> @@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, 
> struct commit *commit)
>   /* a root commit, but there are still more entries to show */
>   reflog = _reflog->reflogs->items[commit_reflog->recno];
>   logobj = parse_object(>noid);
> + if (!logobj)
> + logobj = parse_object(>ooid);
>   }
>  
>   if (!logobj || logobj->type != OBJ_COMMIT) {

We already have a loop to find an entry that is a commit that
discards any non-commit object before the pre-context of this hunk.
This "oops, old side is NULL so let's cover it up by using the new
side" kicks in after that.  I wonder if we can roll that cover-up
logic into the loop, perhaps like

do {
reflog = _reflog->...[recno];
commit_reflog->recno--;
-   logobj = parse_object(>ooid);
+   logobj = parse_object(is_null_oid(>ooid) 
+   ? >noid : >ooid);
-   } while (commit_reflog->recno && (logobj && logobj->type != 
OBJ_COMMIT));
+   } while (commit_reflog->recno && (!logobj || logobj->type != 
OBJ_COMMIT));
-
-   if (!logobj && commit_reflog->recno >= 0 && is_null_oid(>ooid)) 
{
-   /* a root commit ... */
-   reflog = _reflog->...[recno];
-   logobj = parse_object(>noid);
-   }

which may deal with your "both old and new sides were  NULL" case
better.

Re: Truncating HEAD reflog on branch move

[Jeff King]

On Wed, Jun 21, 2017 at 07:04:22PM -0400, Kyle Meyer wrote:

> Junio C Hamano  writes:
> 
> > "brian m. carlson"  writes:
> >
> >> I get the following on 2.11.0:
> >>
> >> 2cbfbd5 HEAD@{0}:
> >> 2cbfbd5 HEAD@{1}: checkout: moving from cPanel to master
> >> eaf8db2 HEAD@{2}: checkout: moving from master to cPanel
> >> 2cbfbd5 HEAD@{3}: clone: from 
> >> https://b...@git.crustytoothpaste.net/git/bmc/homedir.git
> >>
> >> and this on a recent next:
> >>
> >> 2cbfbd5 HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/new
> >>
> >> For this test, any repo will work; I just picked this one because it had
> >> two branches I could use to generate dummy reflog entries.
> >>
> >> A colleague reported this to me as a bug.  I don't see anything in the
> >> release notes about this as a desired behavior change, and it does seem
> >> undesirable to truncate the reflog this way.  If this isn't intentional,
> >> I'm happy to work up a patch.
> >
> > I do not think either behaviour is intentional (old one that gives a
> > meaningless empty entry probably is probably not what we want, the
> > new one that truncates is not what we want, either).
> 
> Eh, sorry about that.  I haven't dug very deeply, but it seems like the
> entry is still in .git/logs/HEAD, while "git reflog" is only showing
> they latest entry.  (Maybe because we stop consuming the entries when we
> hit a null sha1 in the old id field?)

Yeah, I think that's it.

I had trouble digging to find the place where this happens, but I
_think_ it's unintentional, and is an artifact of the way the reflog
iteration is bolted onto the regular revision machinery. In
get_revision_1(), we fake the commit parents as if the earlier reflog
entries formed an actual commit history.

There's some magic in fake_reflog_parent() when we hit an entry with a
null sha1 in the old-oid field. But it depends on looking at the new-oid
field of the entry before. And because our entries look like this:

  real_sha1 null_sha1 ...
  null_sha1 real_sha1 ...

Looking at the prior entry's new-oid field doesn't help at all. And we
end up claiming that there are no parents, and this is the end of the
traversal. The patch below makes it work for me by falling back further
to the previous entry's old oid. This feels like a terrible hack to make
this case work. I'd think we'd actually want some kind of loop to keep
looking. After all, what should:

  real_sha1 real_sha1 ...
  null_sha1 null_sha1 ...
  null_sha1 real_sha1 ...

show? Surely we should not stop traversing when we hit the double
null_sha1. We should either display it in some form, or skip it and
issue a warning that there's a funky entry in your reflog.

This whole fake-parents things does just feel like a gigantic hack,
though. It seems like we should be able to just walk backwards down the
reflog list and show the entries. The revision machinery already
special-cases a bunch of reflog-walk bits; I don't know that adding one
or two more would be the end of the world.

Another approach entirely would be to stop generating two entries in the
reflog, and generate a single one:

  real_sha1 real_sha1 ... Rename from "foo" to "bar"

I think we talked about that earlier, but I think the resolution was
just that doing so was hard with the existing ref-update calls.

diff --git a/reflog-walk.c b/reflog-walk.c
index ed99437ad..b7e489ad3 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, 
struct commit *commit)
/* a root commit, but there are still more entries to show */
reflog = _reflog->reflogs->items[commit_reflog->recno];
logobj = parse_object(>noid);
+   if (!logobj)
+   logobj = parse_object(>ooid);
}
 
if (!logobj || logobj->type != OBJ_COMMIT) {

Re: Truncating HEAD reflog on branch move

[Kyle Meyer]

Kyle Meyer  writes:

> Eh, sorry about that.  I haven't dug very deeply, but it seems like the
> entry is still in .git/logs/HEAD, while "git reflog" is only showing
> they latest entry.

s/entry is still/missing entries are still/

Re: Truncating HEAD reflog on branch move

[Kyle Meyer]

Junio C Hamano  writes:

> "brian m. carlson"  writes:
>
>> I get the following on 2.11.0:
>>
>> 2cbfbd5 HEAD@{0}:
>> 2cbfbd5 HEAD@{1}: checkout: moving from cPanel to master
>> eaf8db2 HEAD@{2}: checkout: moving from master to cPanel
>> 2cbfbd5 HEAD@{3}: clone: from 
>> https://b...@git.crustytoothpaste.net/git/bmc/homedir.git
>>
>> and this on a recent next:
>>
>> 2cbfbd5 HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/new
>>
>> For this test, any repo will work; I just picked this one because it had
>> two branches I could use to generate dummy reflog entries.
>>
>> A colleague reported this to me as a bug.  I don't see anything in the
>> release notes about this as a desired behavior change, and it does seem
>> undesirable to truncate the reflog this way.  If this isn't intentional,
>> I'm happy to work up a patch.
>
> I do not think either behaviour is intentional (old one that gives a
> meaningless empty entry probably is probably not what we want, the
> new one that truncates is not what we want, either).

Eh, sorry about that.  I haven't dug very deeply, but it seems like the
entry is still in .git/logs/HEAD, while "git reflog" is only showing
they latest entry.  (Maybe because we stop consuming the entries when we
hit a null sha1 in the old id field?)

-- 
Kyle

Re: Truncating HEAD reflog on branch move

[Junio C Hamano]

"brian m. carlson"  writes:

> I get the following on 2.11.0:
>
> 2cbfbd5 HEAD@{0}:
> 2cbfbd5 HEAD@{1}: checkout: moving from cPanel to master
> eaf8db2 HEAD@{2}: checkout: moving from master to cPanel
> 2cbfbd5 HEAD@{3}: clone: from 
> https://b...@git.crustytoothpaste.net/git/bmc/homedir.git
>
> and this on a recent next:
>
> 2cbfbd5 HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/new
>
> For this test, any repo will work; I just picked this one because it had
> two branches I could use to generate dummy reflog entries.
>
> A colleague reported this to me as a bug.  I don't see anything in the
> release notes about this as a desired behavior change, and it does seem
> undesirable to truncate the reflog this way.  If this isn't intentional,
> I'm happy to work up a patch.

I do not think either behaviour is intentional (old one that gives a
meaningless empty entry probably is probably not what we want, the
new one that truncates is not what we want, either).

Truncating HEAD reflog on branch move

[brian m. carlson]

In Git 2.11.0, using git branch -m caused an empty entry to be logged to
the reflog, but it preserved the previous history of the default (HEAD)
reflog.  In Git 2.13.1 (and in next and pu), we write a valid entry into
the reflog, but truncate it.

So for the following commands:

  git clone https://git.crustytoothpaste.net/git/bmc/homedir.git
  cd homedir
  git checkout cPanel
  git checkout master
  git branch -m master new

I get the following on 2.11.0:

2cbfbd5 HEAD@{0}:
2cbfbd5 HEAD@{1}: checkout: moving from cPanel to master
eaf8db2 HEAD@{2}: checkout: moving from master to cPanel
2cbfbd5 HEAD@{3}: clone: from 
https://b...@git.crustytoothpaste.net/git/bmc/homedir.git

and this on a recent next:

2cbfbd5 HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/new

For this test, any repo will work; I just picked this one because it had
two branches I could use to generate dummy reflog entries.

A colleague reported this to me as a bug.  I don't see anything in the
release notes about this as a desired behavior change, and it does seem
undesirable to truncate the reflog this way.  If this isn't intentional,
I'm happy to work up a patch.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature