Re: [PATCH 6/6] reflog-walk: stop using fake parents

[Junio C Hamano]

Eric Wong  writes:

> To notice similar errors sooner, I wonder if we should have a
> test target for 64-bit users to test with -m32 enabled, somehow.

Thanks.  Hopefully https://travis-ci.org/git/git/jobs/251772744 and
its later incarnations should be sufficient?

Re: [PATCH 6/6] reflog-walk: stop using fake parents

[Jeff King]

On Mon, Jul 10, 2017 at 09:42:55AM +, Eric Wong wrote:

> > Thanks, I was able to reproduce with CFLAGS=-m32.
> 
> No problem and thanks for the quick response!  Latest pu is
> fine for me.
> 
> To notice similar errors sooner, I wonder if we should have a
> test target for 64-bit users to test with -m32 enabled, somehow.

It's "just"

  make CFLAGS=-m32 test

once you are setup for building 32-bit binaries. But that setup can be a
bit tricky.  On Debian, I have multi-arch set up, and then I have to
"apt-get install zlib1g-dev:i386" etc for all the dependencies. Which
isn't _too_ bad except that libcurl4-openssl-dev conflicts between the
amd64 and i386 versions due to /usr/bin/curl-config. So I have to
install the i386 package, do the -m32 build, and then reinstall the
amd64 one to go back to my regular builds.

At any rate, the 32-bit thing in this instance[1] mostly just tickled
the bug in a different way. Testing with either ASan or valgrind also
detected the problem on 64-bit machines, and people (including me) do
run those periodically. In this case the bug was years old. The reason
it wasn't caught earlier is that there wasn't test coverage, and my
series added it for other reasons. So you really _did_ catch it as soon
as it hit pu, which seems pretty reasonable.

-Peff

[1] There are bugs that really are 32-bit-only, of course, and
instrumenting tools wouldn't catch those. I think we had a bug a
while back where the code mistook sizeof(uint64_t*) for
sizeof(uint64_t). They're the same on 64-bit platforms, but not on
32-bit ones. I do think such bugs are relatively rare, though.

Re: [PATCH 6/6] reflog-walk: stop using fake parents

[Eric Wong]

Jeff King  wrote:
> On Fri, Jul 07, 2017 at 12:32:39AM +, Eric Wong wrote:
> 
> > I'm not sure why, but this is causing t1414.8 failures on 32-bit
> > x86 with the latest pu with Debian jessie (oldstable).
> > 
> > Reverting this (beafb2c62947a6d4a97b9c3baf99fe62ec8e830f) in pu
> > seems to fix the test for me.
> > 
> > +Cc: Ramsay since he also had a 32-bit environment.
> 
> Thanks, I was able to reproduce with CFLAGS=-m32.

No problem and thanks for the quick response!  Latest pu is
fine for me.

To notice similar errors sooner, I wonder if we should have a
test target for 64-bit users to test with -m32 enabled, somehow.

Fwiw, I started having a cronjob which runs "make test-pu" on my
32-bit machine, where config.mak has this:

==> config.mak <==
DEVELOPER := 1
pulog := /tmp/git-test-pu.log

cur_head = $(shell git rev-parse HEAD^0)
cur_pu = $(shell git rev-parse refs/remotes/origin/pu^0)

test-pu:
git fetch -q origin
test x"$(cur_pu)" = x"$(cur_head)" || { \
$(MAKE) clean && \
git reset --hard origin/pu && \
{ $(MAKE) -k test >$(pulog) 2>&1 || \
 mutt -s 'pufail' e...@80x24.org <$(pulog); }; }
-- 
EW

Re: [PATCH 6/6] reflog-walk: stop using fake parents

[Jeff King]

On Thu, Jul 06, 2017 at 11:02:24PM -0400, Jeff King wrote:

> On Fri, Jul 07, 2017 at 12:32:39AM +, Eric Wong wrote:
> 
> > I'm not sure why, but this is causing t1414.8 failures on 32-bit
> > x86 with the latest pu with Debian jessie (oldstable).
> > 
> > Reverting this (beafb2c62947a6d4a97b9c3baf99fe62ec8e830f) in pu
> > seems to fix the test for me.
> > 
> > +Cc: Ramsay since he also had a 32-bit environment.
> 
> Thanks, I was able to reproduce with CFLAGS=-m32.
> 
> > --- expect  2017-07-07 00:30:57.796325232 +
> > +++ actual  2017-07-07 00:30:57.796325232 +
> > @@ -3,6 +3,8 @@
> >  HEAD@{2} checkout: moving from master to side
> >  HEAD@{3} commit: two
> >  HEAD@{4} commit (initial): one
> > -side@{0} commit (merge): Merge branch 'master' into side
> > -side@{1} commit: three
> > -side@{2} branch: Created from HEAD^
> > +HEAD@{0} commit (merge): Merge branch 'master' into side
> > +HEAD@{1} commit: three
> > +HEAD@{2} checkout: moving from master to side
> > +HEAD@{3} commit: two
> > +HEAD@{4} commit (initial): one
> 
> That's quite an unexpected error (to show the HEAD reflog twice). Given
> that it triggers with 32-bit builds, it's like there's some funny memory
> error. And indeed, running it under valgrind shows a problem in
> add_reflog_for_walk. I'll try to dig into it. Thanks for reporting.

Ah, it's a bug in the existing code. It inserts an entry into a string
list and then frees the memory, but the string list wasn't asked to make
a copy of the string. You can see the bug by running t1414 with
--valgrind even before any of the code changes (though note that you'll
have to look at the "-v" output, since it's already marked as
expect-failure).

This fixes it:

diff --git a/reflog-walk.c b/reflog-walk.c
index b7e489ad32..c1f61c524a 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -136,6 +136,7 @@ struct reflog_walk_info {
 void init_reflog_walk(struct reflog_walk_info **info)
 {
*info = xcalloc(1, sizeof(struct reflog_walk_info));
+   (*info)->complete_reflogs.strdup_strings = 1;
 }
 
 int add_reflog_for_walk(struct reflog_walk_info *info,

I'll add that to the 'maint' portion of the series.

-Peff

Re: [PATCH 6/6] reflog-walk: stop using fake parents

[Jeff King]

On Fri, Jul 07, 2017 at 12:32:39AM +, Eric Wong wrote:

> I'm not sure why, but this is causing t1414.8 failures on 32-bit
> x86 with the latest pu with Debian jessie (oldstable).
> 
> Reverting this (beafb2c62947a6d4a97b9c3baf99fe62ec8e830f) in pu
> seems to fix the test for me.
> 
> +Cc: Ramsay since he also had a 32-bit environment.

Thanks, I was able to reproduce with CFLAGS=-m32.

> --- expect2017-07-07 00:30:57.796325232 +
> +++ actual2017-07-07 00:30:57.796325232 +
> @@ -3,6 +3,8 @@
>  HEAD@{2} checkout: moving from master to side
>  HEAD@{3} commit: two
>  HEAD@{4} commit (initial): one
> -side@{0} commit (merge): Merge branch 'master' into side
> -side@{1} commit: three
> -side@{2} branch: Created from HEAD^
> +HEAD@{0} commit (merge): Merge branch 'master' into side
> +HEAD@{1} commit: three
> +HEAD@{2} checkout: moving from master to side
> +HEAD@{3} commit: two
> +HEAD@{4} commit (initial): one

That's quite an unexpected error (to show the HEAD reflog twice). Given
that it triggers with 32-bit builds, it's like there's some funny memory
error. And indeed, running it under valgrind shows a problem in
add_reflog_for_walk. I'll try to dig into it. Thanks for reporting.

-Peff

Re: [PATCH 6/6] reflog-walk: stop using fake parents

[Eric Wong]

I'm not sure why, but this is causing t1414.8 failures on 32-bit
x86 with the latest pu with Debian jessie (oldstable).

Reverting this (beafb2c62947a6d4a97b9c3baf99fe62ec8e830f) in pu
seems to fix the test for me.

+Cc: Ramsay since he also had a 32-bit environment.

--8<--
ok 7 - --parents shows true parents

expecting success: 
{
do_walk HEAD &&
do_walk side
} >expect &&
do_walk HEAD side >actual &&
test_cmp expect actual

--- expect  2017-07-07 00:30:57.796325232 +
+++ actual  2017-07-07 00:30:57.796325232 +
@@ -3,6 +3,8 @@
 HEAD@{2} checkout: moving from master to side
 HEAD@{3} commit: two
 HEAD@{4} commit (initial): one
-side@{0} commit (merge): Merge branch 'master' into side
-side@{1} commit: three
-side@{2} branch: Created from HEAD^
+HEAD@{0} commit (merge): Merge branch 'master' into side
+HEAD@{1} commit: three
+HEAD@{2} checkout: moving from master to side
+HEAD@{3} commit: two
+HEAD@{4} commit (initial): one
not ok 8 - walking multiple reflogs shows both
#   
#   {
#   do_walk HEAD &&
#   do_walk side
#   } >expect &&
#   do_walk HEAD side >actual &&
#   test_cmp expect actual
#   

expecting success: 
head=$(git rev-parse HEAD) &&
one=$(git rev-parse one) &&
ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" 
&&
echo "$head $one $ident broken reflog entry" >>.git/logs/HEAD &&

echo $one >expect &&
git log -g --format=%H -1 >actual &&
test_cmp expect actual

ok 9 - walk prefers reflog to ref tip

[PATCH 6/6] reflog-walk: stop using fake parents

[Jeff King]

The reflog-walk system works by putting a ref's tip into the
pending queue, and then "traversing" the reflog by
pretending that the parent of each commit is the previous
reflog entry.

This causes a number of user-visible oddities, as documented
in t1414 (and the commit message which introduced it). We
can fix all of them in one go by replacing the fake-reflog
system with a much simpler one: just keeping a list of
reflogs to show, and walking through them entry by entry.

The implementation is fairly straight-forward, but there are
a few items to note:

  1. We obviously must skip calling add_parents_to_list()
 when we are traversing reflogs, since we do not want to
 walk the original parents at all.

 As a result, we must call try_to_simplify_commit()
 ourselves and skip any TREESAME commits.

 There are other parts of add_parents_to_list() we skip,
 as well, but none of them should matter for a reflog
 traversal:

   -  We do not allow UNINTERESTING commits, nor
  symmetric ranges (and we bail when these are used
  with "-g").

   - Using --source makes no sense, since we aren't
 traversing. The reflog selector shows the same
 information with more detail.

   - Using --first-parent is still sensible, since you
 may want to see the first-parent diff for each
 entry. But since we're not traversing, we don't
 need to cull the parent list here.

  2. Since we now just walk the reflog entries themselves,
 rather than starting with the ref tip, we now look at
 the "new" field of each entry rather than the "old"
 (i.e., we are showing entries, not faking parents).
 This removes all of the tricky logic around skipping
 past root commits.

 But note that we have no way to show an entry with the
 null sha1 in its "new" field (because such a commit
 obviously does not exist). Normally this would not
 happen, since we delete reflogs along with refs, but
 there is one special case. When we rename the currently
 checked out branch, we write two reflog entries into
 the HEAD log: one where the commit goes away, and
 another where it comes back.

 Prior to this commit, we show both entries with
 identical reflog messages. After this commit, we show
 only the "comes back" entry. See the update in t3200
 which demonstrates this.

 Arguably either is fine, as the whole double-entry
 thing is a bit hacky in the first place. And until a
 recent fix, we truncated the traversal in such a case
 anyway, which was _definitely_ wrong.

Signed-off-by: Jeff King 
---
 reflog-walk.c  | 116 +
 reflog-walk.h  |   4 +-
 revision.c |  30 -
 t/t1414-reflog-walk.sh |  12 ++---
 t/t3200-branch.sh  |   3 +-
 5 files changed, 57 insertions(+), 108 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index 89e719c459..a7644d944e 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,52 +188,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
commit_reflog->selector = selector;