Skip to content

CommitFilter ahead/behind are confusing... #1069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 15, 2015
Merged

CommitFilter ahead/behind are confusing... #1069

merged 1 commit into from
Aug 15, 2015

Conversation

nulltoken
Copy link
Member

I’m using libgit2sharp to emulate this: “git log HEAD~2..HEAD”

This fails to find any commits:

var filter = new CommitFilter { FirstParentOnly = true, Since = "HEAD~2", Until = "HEAD" };
var commits = Repo.Commits.QueryBy(filter);

This successfully finds both commits:

var filter = new CommitFilter { FirstParentOnly = true, Since = "HEAD", Until = "HEAD~2" };
var commits = Repo.Commits.QueryBy(filter);

This makes sense only if you consider that internally, the walk is starting at HEAD and going backwards to HEAD2. However, this is grammatically confusing because I'm really intending to get the commits starting at HEAD and ending at HEAD2. The fact that it walks backwards feels like an implementation detail; it really isn't what I'm trying to express. This is especially true when compared to the git command line which grammatically reads left (start) to right (end).

You can see the very natural confusion in the sample:
https://github.com/libgit2/libgit2sharp/wiki/git-log

$ git log master..development
LibGit2Sharp
using (var repo = new Repository("path/to/your/repo"))
{
  var filter = new CommitFilter { Since = repo.Branches["master"], Until = repo.Branches["development"] };

Note that this is only a problem because the choice in words in the structure don't meet the natural expectation. Humans (even developers) just don't naturally think backwards; time (and relationships) moves "Since" something in the past "Until" something in the future.

It's too late to change the behavior but two new properties that are effectively the same (aliases) could be added. IMHO, going with the Git "revrange" documentation of "Rev1" and "Rev2" is best since it doesn't rely on natural grammatical terms which might have naturally different implication.

Dave

@nulltoken
Copy link
Member

It's too late to change the behavior but two new properties that are effectively the same (aliases) could be added. IMHO, going with the Git "revrange" documentation of "Rev1" and "Rev2" is best since it doesn't rely on natural grammatical terms which might have naturally different implication.

Thanks for this very detailed explanation that makes a lot of sense. Although I'm not sold on Rev1 and Rev2, I'm completely open to obsolete those properties in favor of new ones with more meaningful names.

Besides, after a quick look, I'd say the xml doc would also deserve some love as "starting points" means barely anything.

/cc @ethomson @jamill @carlosmn @Therzok @dahlbyk

@nulltoken
Copy link
Member

How about ReachableFrom and Excluding?

@whoisj
Copy link

whoisj commented Jun 3, 2015

I think the desired naming depends on your mental model.

Git CLI uses a time based model (ex git log <past>..<future>)

LG2# is using a graph search (descendant to ancestor) approach.

Both are legitimate. Our naming is poor. Better names and I think we're OK.

So, if we're sticking with the "graph search" concept, then Descendant and Ancestor seem logical.

@dahlbyk
Copy link
Member

dahlbyk commented Jun 3, 2015

Agree that Since and Until usage here feels reversed, especially contrasted with git log --since="1 week ago" --until="yesterday".

If not ReachableFrom and Excluding I'd propose Head and Base.

@Therzok
Copy link
Member

Therzok commented Jun 3, 2015

Head and Base make it sound like only 1 way traversal.

@Therzok
Copy link
Member

Therzok commented Jun 3, 2015

I'd go with ReachableFrom and Excluding, but those are vaguer, from a lexical pov. 🚓

@dahlbyk
Copy link
Member

dahlbyk commented Jun 3, 2015

Heads and Ancestors?

I suggested Base based on its meaning in a merge, but I'm not sure it conveys intent particularly well.

@Therzok
Copy link
Member

Therzok commented Jun 3, 2015

From Towards fit more in this context.

@whoisj
Copy link

whoisj commented Jun 4, 2015

From and To are a popular pair of bookend words as well.

@nulltoken
Copy link
Member

I know English isn't my native language, but I've got the impression that From/To isn't that semantically different from Since/Until 😜

How about IncludingAndAncestors and ExcludingAncestors?

@Therzok
Copy link
Member

Therzok commented Jun 4, 2015

From To mark a traversal direction from Ref X to Ref Y, regardless of their order in time.

Since Until mark a temporal traversal where Since is in the past and Until is in the future of Since.

@whoisj
Copy link

whoisj commented Jun 4, 2015

Let me ask this: is the search directional? If it is, Since/Until are fine, but if the search doesn't have to be directional, then From/To are best and we can use their relation to sort the results.

@nulltoken
Copy link
Member

Tagged as stabilization

@nulltoken
Copy link
Member

The git-log doc states

--since=<date>
--after=<date>
     Show commits more recent than a specific date.

--until=<date>
--before=<date>
     Show commits older than a specific date.

Considering this above, I'd agree that using Since and Until as keywords is quite misleading

@whoisj
Copy link

whoisj commented Jun 4, 2015

Agreed. However in both of the cases above the parameter type is a date and not a commitish.

How about we just use From and To then? We can decide what it means when From > To vs To > From independently and if or not it is supported in both way?.

@nulltoken
Copy link
Member

We can decide what it means when From > To vs To > From independently

Honestly, I don't know. If we can't decide which one is better than the other, maybe it's a sign that those may not be much clearer than the current implementation.

@carlosmn As you're our own "Name Whisperer", we'd really like some of your magic.

@dahlbyk
Copy link
Member

dahlbyk commented Jun 5, 2015

Got it: Older and Newer.

@whoisj
Copy link

whoisj commented Jun 5, 2015

What does older and newer mean in a directed graph when date is just meta data and arbitrary?

@dahlbyk
Copy link
Member

dahlbyk commented Jun 5, 2015

Let me ask this: is the search directional?

Yes, a revwalk starts from a list of "pushed" commits and keeps walking until it reaches an ancestor of a list of "hidden" commits.

What does older and newer mean in a directed graph when date is just meta data and arbitrary?

Ancestors are conceptually older than their descendants? I don't think it's a particularly good option, but it seems a little less ambiguous?

It's probably safest to align with the language from the git revisions docs:

When you have two commits r1 and r2 (named according to the syntax explained in SPECIFYING REVISIONS above), you can ask for commits that are reachable from r2 excluding those that are reachable from r1 by ^r1 r2 and it can be written as r1..r2.

@whoisj
Copy link

whoisj commented Jun 5, 2015

Given the constraints of revwalk and given @dahlbyk's reply, I'll assume (without looking - how lazy 😴) that LG2 has the same constraints and therefore desires the same behavior, I propose that we use Tip and Base.

/// <param name="Base">
/// The last commit to be discovered by the revwalk, 
/// the "base" of the results.</param>
/// <param name="Tip">
/// The first commit to be discovered by the revwalk, 
/// the starting point or "tip" of the results.
/// </param>

@whoisj
Copy link

whoisj commented Jun 5, 2015

Per Wikipedia we should be using edge and root for discussion of walking a tree graph.

@nulltoken
Copy link
Member

I kind of like Tip (Or Tips as we can pass an array of things).

Regarding the other one, I'd like something that also reflects the fact that the designated commit(s) won't be returned (as they will be hidden along with their ancestors).

@shiftkey
Copy link
Contributor

shiftkey commented Jun 5, 2015

👍 to avoiding time concepts with the parameter names

@shiftkey
Copy link
Contributor

shiftkey commented Jun 5, 2015

Given we're going back and forth on naming these parameters and not getting far, and the general confusion about traversing graphs appears to be a theme, would it be worth having some good samples to use as a reference for discussion?

A couple of benefits for this:

  • discussing concrete examples might help us design the API to be nicer
  • adds supporting material to help users understand the behaviour here

As a starting point, we could open a PR with a test repository containing a couple of example graphs (for example, the "diamond" scenario) to kick things along.

@nulltoken
Copy link
Member

How about plain From and Excluding?

@whoisj
Copy link

whoisj commented Jun 10, 2015

@nulltoken From sounds like a good term for where the search starts, unless it is taken in time context then it could be interpreted as the oldest commit based on date or depth in the graph. Excluding how ever sounds like it should be a list of commits to filter out and ignore.

Naming this damn API is harder than authoring it.

Maybe we should just stick to the Git docs and call the values rev1 and rev2. 😐

@dahlbyk
Copy link
Member

dahlbyk commented Jun 11, 2015

{ Include = "master", Except = "master~20" }

@ethomson
Copy link
Member

I like where @dahlbyk is going with this, but how about Include and Exclude for parity?

@whoisj
Copy link

whoisj commented Jun 11, 2015

Regardless of the names we choose, I do think we're going to need very good documentation both in the .md as well as in the inline documentation comments. Finally a use for the <example/> tag.

@nulltoken
Copy link
Member

Finally a use for the <example/> tag.

How does ASCII art renders in such tags?

@nulltoken
Copy link
Member

I like where @dahlbyk is going with this, but how about Include and Exclude for parity?

Why not.

@DavidJanson There are a ton of proposal above. Anyone that would feel like a better fit from a user's perspective?

@whoisj
Copy link

whoisj commented Jun 11, 2015

How does ASCII art renders in such tags?

Honestly do not know, but without a good example comparing it to how git.exe operates, confusion/frustration like what @DavidJanson reported will continue.

@nulltoken
Copy link
Member

I was hopping to include some commit graph...

@whoisj
Copy link

whoisj commented Jun 11, 2015

You mean like the examples on the Git SCM site?

      .-A---M---N---O---P---Q
     /     /   /   /   /   /
    I     B   C   D   E   Y
     \   /   /   /   /   /
      `-------------'   X

And by the way the parameter terms for git rev-list are commit1 and commit2, how creative...

We should look at the terms they have and how they use them, many of those terms have meaning in the context of a Git graph and should be excluded from this conversation.

Example:

--since=<date>
--after=<date>
Show commits more recent than a specific date.

--until=<date>
--before=<date>
Show commits older than a specific date.

--exclude=<glob-pattern>
Do not include refs matching <glob-pattern>...

@dahlbyk
Copy link
Member

dahlbyk commented Jun 12, 2015

I was hopping

🐰 🐇

@nulltoken
Copy link
Member

How about WalkFrom/StopAt?

@nulltoken
Copy link
Member

I was hopping

🐰 🐇

I'll just blame this on a nature-friendly keyboard layout. 😉

image

@whoisj
Copy link

whoisj commented Jun 15, 2015

How about WalkFrom/StopAt?

Oh, I like that.

@dahlbyk
Copy link
Member

dahlbyk commented Jun 15, 2015

Include/Except seems maybe more discoverable? Walk may be the preferred term for a graph, but commit logs tend to be characterized as lists.

@whoisj
Copy link

whoisj commented Jun 15, 2015

Include/Except seems maybe more discoverable? Walk may be the preferred term for a graph, but commit logs tend to be characterized as lists.

I've personally never thought of them that way. I suppose we return them as a linear collection, but that's arbitrary (and slightly incorrect behavior encourages by the paradigms of the language in use).

We're basically doing a "leaf to trunk" read back of a graph, with "leaf" being defined as the starting where we start walking and "trunk" being the end-point where we stop.

@nulltoken's suggestion of WalkFrom paired with StopAt is the most literal and obvious so far. It has my ☑️

@nulltoken
Copy link
Member

@dahlbyk That may be a language issue, but I read Except as Skip/Leave out. This doesn't express (to me) the notion of a boundary.

@dahlbyk
Copy link
Member

dahlbyk commented Jun 16, 2015

That may be a language issue, but I read Except as Skip/Leave out. This doesn't express (to me) the notion of a boundary.

But that's exactly what it is - a list of commits to leave out of the result. Could expand to IncludeReachableFrom and ExceptReachableFrom/ExcludeReachableFrom to be more literal?

StopAt seems potentially ambiguous if it's inclusive or exclusive. StopBefore?

@nulltoken
Copy link
Member

@dahlbyk I wasn't very clear. Except was leaving me under the impression that only the specified commit was targeted, not its ancestors.

IncludeReachableFrom/ExcludeReachableFrom sound unambiguous to me. I like those!

@nulltoken
Copy link
Member

IncludeReachableFrom/ExcludeReachableFrom sound unambiguous to me.

Unless anyone strongly opposes, I'm going to shortly issue a PR with this proposal

@nulltoken nulltoken self-assigned this Jul 9, 2015
@jamill
Copy link
Member

jamill commented Jul 9, 2015

IncludeReachableFrom/ExcludeReachableFrom

👍

@whoisj
Copy link

whoisj commented Jul 9, 2015

IncludeReachableFrom/ExcludeReachableFrom

👍

@nulltoken nulltoken added this to the v0.22 milestone Jul 26, 2015
nulltoken added a commit that referenced this pull request Aug 15, 2015
CommitFilter ahead/behind are confusing...
@nulltoken nulltoken merged commit 01b9a62 into vNext Aug 15, 2015
@nulltoken nulltoken deleted the ntk/since branch August 15, 2015 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants