Re: Truncating HEAD reflog on branch move

From: Jeff King <peff@peff.net>
Date: Jun 22, 2017, 21:52 UTC
Message-ID: <20170622215235.to6yleo3adt5klv2@sigill.intra.peff.net>
In-Reply-To: <xmqqd19vix03.fsf@gitster.mtv.corp.google.com>

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