Suggestion: make --left-right work with --merge

Suggestion: make --left-right work with --merge

[Paul Mackerras]

It would be nice if git-log --merge --left-right did what git log
--merge does but additionally, if the merge is a 2-way merge, marked
commits with '<' or '>' according to which side of the merge they came
from.  Would that be possible?  I think the visual indication that
--left-right gives in gitk would be useful with --merge.

Paul.

Re: Suggestion: make --left-right work with --merge

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> It would be nice if git-log --merge --left-right did what git log
> --merge does but additionally, if the merge is a 2-way merge, marked
> commits with '<' or '>' according to which side of the merge they came
> from.  Would that be possible?  I think the visual indication that
> --left-right gives in gitk would be useful with --merge.

Like this?

A trivial feature that can be implemented in -4 lines ;-)

 revision.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/revision.c b/revision.c
index 4e36b95..38d7d94 100644
--- a/revision.c
+++ b/revision.c
@@ -771,14 +771,9 @@ static void prepare_show_merge(struct rev_info *revs)
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, "MERGE_HEAD");
 	bases = get_merge_bases(head, other, 1);
-	while (bases) {
-		struct commit *it = bases->item;
-		struct commit_list *n = bases->next;
-		free(bases);
-		bases = n;
-		it->object.flags |= UNINTERESTING;
-		add_pending_object(revs, &it->object, "(merge-base)");
-	}
+	add_pending_commit_list(revs, bases, UNINTERESTING);
+	free_commit_list(bases);
+	head->object.flags |= SYMMETRIC_LEFT;
 
 	if (!active_nr)
 		read_cache();
@@ -797,6 +792,7 @@ static void prepare_show_merge(struct rev_info *revs)
 			i++;
 	}
 	revs->prune_data = prune;
+	revs->limited = 1;
 }
 
 int handle_revision_arg(const char *arg, struct rev_info *revs,

Re: Suggestion: make --left-right work with --merge

[Paul Mackerras]

Junio C Hamano writes:

> Like this?
> 
> A trivial feature that can be implemented in -4 lines ;-)

Cool!  Yes, that works nicely.

Another thing: I have noticed that git rev-parse --revs-only doesn't
pass the --merge and --left-right flags through, even though the help
for git rev-parse says that --revs-only means "Do not output flags and
parameters not meant for git-rev-list" (which I assume means the same
as "Only output flags and parameters meant for git rev-list"), and git
rev-list accepts and uses the --merge and --left-right flags.  I
assume this is just an oversight in git rev-parse.

Regards,
Paul.

Re: Suggestion: make --left-right work with --merge

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> Another thing: I have noticed that git rev-parse --revs-only doesn't
> pass the --merge and --left-right flags through, even though the help
> for git rev-parse says that --revs-only means "Do not output flags and
> parameters not meant for git-rev-list" (which I assume means the same
> as "Only output flags and parameters meant for git rev-list"), and git
> rev-list accepts and uses the --merge and --left-right flags.  I
> assume this is just an oversight in git rev-parse.

Yeah, the option list is not maintained well for the last few
years and needs to be updated.

Re: Suggestion: make --left-right work with --merge

[Paul Mackerras]

Junio C Hamano writes:

> Yeah, the option list is not maintained well for the last few
> years and needs to be updated.

Ah.  OK.  I am currently using git rev-parse in the gitk dev branch to
determine what starting commits git log will use, so that when the
user does an update, I can construct a git log command that will stop
when it gets to any of those previous starting points.  That way the
second git log will only give me new stuff that has been added since
the first git log.

Can you suggest an efficient way to do that that doesn't use git
rev-parse?

Thanks,
Paul.

Re: Suggestion: make --left-right work with --merge

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> Junio C Hamano writes:
>
>> Yeah, the option list is not maintained well for the last few
>> years and needs to be updated.
>
> Ah.  OK.  I am currently using git rev-parse in the gitk dev branch to
> determine what starting commits git log will use, so that when the
> user does an update, I can construct a git log command that will stop
> when it gets to any of those previous starting points.  That way the
> second git log will only give me new stuff that has been added since
> the first git log.

Doesn't

	git rev-parse --revs-only --no-flags "$@" | grep '^[0-9a-f]'

give you what you want?

Interesting.

 * gitk has already done what the user asked once.  E.g. "git
   log next..master" to find out new trivially correct fixes
   that are already applied to 'master' and will be brought to
   'next' when I merge 'master' to 'next' next time;

 * The user does "git merge master" while on 'next', and tell
   gitk to "Update".

 * gitk is expected to re-run "git log next..master" (textually
   the same command line), but most part of the graph it already
   knows about.  You need a way to omit what you already know,
   so when you run the query for the first time you would want
   to remember the actual object names, so that you use them to
   tweak the query to "git log next..master --not <positive ones
   in the first round>" for the second query.

In the above example, the set of commits actually will shrink
('next' moves and you will exclude more than before upon
"Update").

If you are only interested in cases that the only positive end
grows (in the above example, the user adds commit to 'master'
instead of merging it into 'next'), then grabbing only the
positive ends (e.g. 'master' in 'next..master'), negate them and
append them to the original query would speed things up, but
obviously that would not work in the above example.

Re: Suggestion: make --left-right work with --merge

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> Junio C Hamano writes:
>
>> Like this?
>> 
>> A trivial feature that can be implemented in -4 lines ;-)
>
> Cool!  Yes, that works nicely.

I did not check this myself, but do arrows point in the right
direction?  I just randomly painted HEAD side left (and other
side right) in the patch.

Re: Suggestion: make --left-right work with --merge

[Paul Mackerras]

Junio C Hamano writes:

> Doesn't
> 
> 	git rev-parse --revs-only --no-flags "$@" | grep '^[0-9a-f]'
> 
> give you what you want?

Well, it does, except for --merge, which is perhaps a special case.

(Actually, what would git rev-parse --revs-only --no-flags output that
isn't a SHA1 ID?  Why do you have the grep command there?)

Also, --left-right with symmetric difference is an interesting case,
because git rev-parse will turn a symmetric difference into three SHA1
IDs, with the 3rd one negated.  If I pass what I get from git
rev-parse to git log --left-right, then git log doesn't recognize that
as a symmetric difference and shows all commits with the ">" marker.

> Interesting.
> 
>  * gitk has already done what the user asked once.  E.g. "git
>    log next..master" to find out new trivially correct fixes
>    that are already applied to 'master' and will be brought to
>    'next' when I merge 'master' to 'next' next time;
> 
>  * The user does "git merge master" while on 'next', and tell
>    gitk to "Update".
> 
>  * gitk is expected to re-run "git log next..master" (textually
>    the same command line), but most part of the graph it already
>    knows about.  You need a way to omit what you already know,
>    so when you run the query for the first time you would want
>    to remember the actual object names, so that you use them to
>    tweak the query to "git log next..master --not <positive ones
>    in the first round>" for the second query.
> 
> In the above example, the set of commits actually will shrink
> ('next' moves and you will exclude more than before upon
> "Update").

"Update" adds new commits and moves the head/tag markers as necessary,
but doesn't remove commits from the graph, because that's a bit hard.
There is also a "Reload" function that restarts from scratch and so
will not show the newly-removed commits (i.e., the same as "Update"
does now in current gitk master), but it is a lot slower than
"Update".  In the common case (a few commits added), "Update" takes
less than a second.

> If you are only interested in cases that the only positive end
> grows (in the above example, the user adds commit to 'master'
> instead of merging it into 'next'), then grabbing only the
> positive ends (e.g. 'master' in 'next..master'), negate them and
> append them to the original query would speed things up, but
> obviously that would not work in the above example.

That's essentially what I do.  I think I'll just have to do special
cases for --merge and --left-right.

Paul.

Re: Suggestion: make --left-right work with --merge

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> Junio C Hamano writes:
>
>> Doesn't
>> 
>> 	git rev-parse --revs-only --no-flags "$@" | grep '^[0-9a-f]'
>> 
>> give you what you want?
>
> Well, it does, except for --merge, which is perhaps a special case.
>
> (Actually, what would git rev-parse --revs-only --no-flags output that
> isn't a SHA1 ID?  Why do you have the grep command there?)

Try it on user inputs like "master..next", "next...master".  You
wanted to grab only the positive ones, no?

> That's essentially what I do.  I think I'll just have to do special
> cases for --merge and --left-right.

Are you sure?  I can imagine that --merge may be problematic,
because it implicitly is about MERGE_HEAD...HEAD, but rev-parse
may not know about it.  I think --left-right and symmetric
should be fine.

	$ set x --left-right master...next; shift
	$ git rev-parse --revs-only --no-flags "$@" |
          git name-rev --stdin
        88113cfea67557067f1363b210b803637d186632 (next)
        97b97c58e609a1cd23b3c2514f41cdb0405870ee (master)
        ^97b97c58e609a1cd23b3c2514f41cdb0405870ee (master)

In the above case, master is a strict ancestor of next, but the
third line that is negative (and my grep would exclude) is
generally the merge-base between the left and right positives.
The two positives are what you wanted to exclude from the second
round, aren't they?

And in order to handle them sanely without worrying about
revision machinery and rev-parse going out-of-sync, I would
almost suggest doing something like the attached patch and say:

	$ git rev-list --no-walk "$@"

The user options can contain --left-right and --boundary, so the
output from this could begin with ">", "<" and "-".  If you
discard the ones that are prefixed with "-" (i.e. the user had
the --boundary option in "$@"), grab the ones that are not
prefixed (i.e. the user did not have the --left-right option in
"$@"), and the ones prefixed with ">" and "<" (i.e. the user did
have --left-right, and you would strip the prefix from such a
line), you would find all positive ones the user specified, I
think.

-- >8 --
revision --no-walk: do not refuse to show the list with negatives

Scripts may want to find out what revs the user gave from the
command line.

	git rev-list --no-walk --boundary "$@"

would be a natural way to find out what revs (with flags) they
are, but --no-walk refused to work if there are negative ones.
Lift that restriction.
 
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 revision.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 0eb6faa..c0931eb 100644
--- a/revision.c
+++ b/revision.c
@@ -129,8 +129,6 @@ void mark_parents_uninteresting(struct commit *commit)
 
 static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode)
 {
-	if (revs->no_walk && (obj->flags & UNINTERESTING))
-		die("object ranges do not make sense when not walking revisions");
 	if (revs->reflog_info && obj->type == OBJ_COMMIT &&
 			add_reflog_for_walk(revs->reflog_info,
 				(struct commit *)obj, name))

Re: Suggestion: make --left-right work with --merge

[Linus Torvalds]

On Fri, 29 Feb 2008, Paul Mackerras wrote:
> 
> Well, it does, except for --merge, which is perhaps a special case.

Well, clearly --merge is a special case, but it probably really shouldn't 
be.

If you want to parse revisions, then --merge should do that. But --merge 
is really special in that it expands into *both* a revision range *and* a 
set of files, so it doesn't really work wonderfully well for the trivial 
kind of parsing that git-rev-parse does.

So we certainly *could* just add "--merge" to the list of magic flags that 
is known about by is_rev_argument(), since that's a trivial one-liner. But 
that's really only ok for the case where we are supposed to output both 
revisions *and* flags, and even then it should also suppress the default 
HEAD generation (although it won't actually hurt, since the revision 
output from --merge will include HEAD in teh "HEAD...MERGE_HEAD" logic 
anyway, so with --default always being HEAD in practice..)

So yeah, I think git-rev-list has trouble handling --merge sanely the way 
it is currently written.

		Linus

Re: Suggestion: make --left-right work with --merge

[Paul Mackerras]

Junio C Hamano writes:

> Try it on user inputs like "master..next", "next...master".  You
> wanted to grab only the positive ones, no?

No... gitk passes the IDs it gets from git rev-parse (both positive
and negative) to git log, rather than the original arguments.  This is
to avoid gitk having an incorrect idea about what commits git log has
started from, in the situation where somebody changes some head or tag
between when gitk does the git rev-parse and when it does the git log.

That unfortunately means --left-right doesn't work however, because
git log doesn't recognize "a b ^merge_base(a,b)" as being equivalent
to "a...b".

> And in order to handle them sanely without worrying about
> revision machinery and rev-parse going out-of-sync, I would
> almost suggest doing something like the attached patch and say:
> 
> 	$ git rev-list --no-walk "$@"
> 
> The user options can contain --left-right and --boundary, so the
> output from this could begin with ">", "<" and "-".  If you
> discard the ones that are prefixed with "-" (i.e. the user had
> the --boundary option in "$@"), grab the ones that are not
> prefixed (i.e. the user did not have the --left-right option in
> "$@"), and the ones prefixed with ">" and "<" (i.e. the user did
> have --left-right, and you would strip the prefix from such a
> line), you would find all positive ones the user specified, I
> think.

Yes, that would be useful.  Now, if I can just think of a way to avoid
the race, or work around it, the problem will be solved. :)  I can
probably do that by checking whether I see all the (positive) commits
from git rev-list --no-walk in the git log output.

Thanks,
Paul.

Re: Suggestion: make --left-right work with --merge

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> Junio C Hamano writes:
>
>> Try it on user inputs like "master..next", "next...master".  You
>> wanted to grab only the positive ones, no?
>
> No... gitk passes the IDs it gets from git rev-parse (both positive
> and negative) to git log, rather than the original arguments.

I am not sure if I was answering the right question, then.

I thought the issue you were addressing was this.

 (1) the user says "gitk master...next" to start you.

 (2) you run "git log master...next" (or whatever the equivalent of what
     the user gave you) and draw.

 (3) the user does things outside your control to modify refs, and says
     "Update".

 (4) you could re-run "git log master...next" again, but that would show
     mostly what you already know.

     If you saved all the positive ones you used in (2), you could instead
     run "git log master...next --not <positives in 2>" to ask only the
     incremental changes (as long as the branches do not rewind, but you
     do not discard nodes from the already drawn graph anyway).

     <positives in 2> in this example sequence would be the object names
     for master and next when (2) was run.

I think your "gitk passes the IDs" part is about the master...next part in
the above example.  I do not think it really matters if they are converted
to object names or kept symbolic when given to "git log".  I was talking
about the part you add after --not in the second round, and that was why
my answer was only about positive ones.

Come to think of it, when you are told to "Update", you already know the
positive tips you can use to optimize (4), don't you?  They are the
commits you drew in (3) that do not have children.

Re: Suggestion: make --left-right work with --merge

[Paul Mackerras]

Junio C Hamano writes:

> I think your "gitk passes the IDs" part is about the master...next part in
> the above example.  I do not think it really matters if they are converted
> to object names or kept symbolic when given to "git log".

The only time it would matter is if they were kept symbolic AND
somehow the user changed them between the git rev-parse (or git
rev-list) invocation and the git log invocation.

> Come to think of it, when you are told to "Update", you already know the
> positive tips you can use to optimize (4), don't you?  They are the
> commits you drew in (3) that do not have children.

It's possible the user could do "Update" before we had finished
reading stuff from the first git log, so I don't necessarily know all
the commits that have no children.  And in fact that's a reasonable
thing to do if gitk is being used on a really really large
repository.

I'm a kernel guy - I worry about race conditions. :)

Thanks for the tip about git rev-list --no-walk.

Paul.