Rewrite much of the graph code #240

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@BenBergman
Contributor

BenBergman commented Feb 7, 2014

Fix for jonas/tig#144.


This patch reimplements much of the logic used to produce the graph in
the main view. A set of git log inputs and expected outputs was created
in tools/test-graph-samples/, and a script to automatically evaluate
these examples was made in tools/unit-test-graph.sh. Please update these
samples if you find a new edge case or if you make changes to the
graphing code.

There are two aspects to the displayed graph which were not included in
this patch as the scope of the changes was deemed too large.

  1. Commits that are both merge commits and branch points (or merge
    commits and below a commit that is a branch point) can display a bit
    ambiguously. The ideal fix for this is creating non-commit lines in the
    main view when the graph is displayed. Example input and output can be
    found in the samples folder.
  2. Line colouring for lines crossing over other lines. Currently, each
    pair of symbols (eg. '-|') shares a colour attribute. However, if the
    two portions of the symbol are not connected, they should most likely
    not share a colour. There are no unit tests implemented for this.

Let me know if there is anything I can do to improve this pull request. The number of additions is pretty epic, but that is mostly the hashtab library and the sample inputs and expected outputs. The actual changes are fewer than 1000 lines (still large, but nowhere near the full 21000).

I made some changes to the Makefile that could use another set of eyes to make sure I didn't mess something up too badly.

@jonas

This comment has been minimized.

Show comment
Hide comment
@jonas

jonas Feb 10, 2014

Owner

Great to see the number of lines go down ;-)

I've started to review this and here are a couple of comments.

  • Please always define all scope variables at the start of the scope followed by an empty line. E.g.
static size_t
colors_get_color(struct colors *colors, const char *id)
{
    struct id_color *key = id_color_new(id, 0);
    struct id_color *node = (struct id_color *) htab_find(colors->id_map, key);
    id_color_delete(key);

should be

static size_t
colors_get_color(struct colors *colors, const char *id)
{
    struct id_color *key = id_color_new(id, 0);
    struct id_color *node = (struct id_color *) htab_find(colors->id_map, key);

    id_color_delete(key);
  • I would prefer to keep libiberty code inside the compat/ directory
  • Tests should go into a new toplevel test/ directory if possible. Ideally with a make test target.
  • There are multiple errors reported by the tests. Is that expected?
Owner

jonas commented Feb 10, 2014

Great to see the number of lines go down ;-)

I've started to review this and here are a couple of comments.

  • Please always define all scope variables at the start of the scope followed by an empty line. E.g.
static size_t
colors_get_color(struct colors *colors, const char *id)
{
    struct id_color *key = id_color_new(id, 0);
    struct id_color *node = (struct id_color *) htab_find(colors->id_map, key);
    id_color_delete(key);

should be

static size_t
colors_get_color(struct colors *colors, const char *id)
{
    struct id_color *key = id_color_new(id, 0);
    struct id_color *node = (struct id_color *) htab_find(colors->id_map, key);

    id_color_delete(key);
  • I would prefer to keep libiberty code inside the compat/ directory
  • Tests should go into a new toplevel test/ directory if possible. Ideally with a make test target.
  • There are multiple errors reported by the tests. Is that expected?
@BenBergman

This comment has been minimized.

Show comment
Hide comment
@BenBergman

BenBergman Feb 10, 2014

Contributor

Thanks for the comments. I'll fix up my commit shortly.

Regarding the failed tests, all the failures are related to the non-commit lines which I ended up deciding to leave for another separate feature. I can remove the non-commit line tests, or possibly leave just a single file for reference.

Contributor

BenBergman commented Feb 10, 2014

Thanks for the comments. I'll fix up my commit shortly.

Regarding the failed tests, all the failures are related to the non-commit lines which I ended up deciding to leave for another separate feature. I can remove the non-commit line tests, or possibly leave just a single file for reference.

Rewrite much of the graph code
This patch reimplements much of the logic used to produce the graph in
the main view. A set of git log inputs and expected outputs was created
in tools/test-graph-samples/, and a script to automatically evaluate
these examples was made in tools/unit-test-graph.sh. Please update these
samples if you find a new edge case or if you make changes to the
graphing code.

There are two aspects to the displayed graph which were not included in
this patch as the scope of the changes was deemed too large.

1. Commits that are both merge commits and branch points (or merge
commits and below a commit that is a branch point) can display a bit
ambiguously. The ideal fix for this is creating non-commit lines in the
main view when the graph is displayed. Example input and output can be
found in the samples folder.

2. Line colouring for lines crossing over other lines. Currently, each
pair of symbols (eg. '-|') shares a colour attribute. However, if the
two portions of the symbol are not connected, they should most likely
not share a colour. There are no unit tests implemented for this.
@BenBergman

This comment has been minimized.

Show comment
Hide comment
@BenBergman

BenBergman Feb 11, 2014

Contributor

I applied the above changes, leaving in the one test file for non-commit lines so that whoever works on that feature can see what was expected. If this is undesirable, I can remove this too. Tests can be run with make test.

Contributor

BenBergman commented Feb 11, 2014

I applied the above changes, leaving in the one test file for non-commit lines so that whoever works on that feature can see what was expected. If this is undesirable, I can remove this too. Tests can be run with make test.

@jonas jonas closed this in 87009cb Feb 11, 2014

@jonas

This comment has been minimized.

Show comment
Hide comment
@jonas

jonas Feb 11, 2014

Owner

Thanks a lot for making the changes!

Owner

jonas commented Feb 11, 2014

Thanks a lot for making the changes!

@vivien

This comment has been minimized.

Show comment
Hide comment
@vivien

vivien Feb 11, 2014

Contributor

Using the graph in the linux repo is a real pleasure now. Thanks a lot for your work @BenBergman!

Contributor

vivien commented Feb 11, 2014

Using the graph in the linux repo is a real pleasure now. Thanks a lot for your work @BenBergman!

@vivien

This comment has been minimized.

Show comment
Hide comment
@vivien

vivien Feb 11, 2014

Contributor

@BenBergman just one question. In the following screenshot:

Shouldn't the first horizontal line be yellow and the second one be green?

Contributor

vivien commented Feb 11, 2014

@BenBergman just one question. In the following screenshot:

Shouldn't the first horizontal line be yellow and the second one be green?

@BenBergman

This comment has been minimized.

Show comment
Hide comment
@BenBergman

BenBergman Feb 11, 2014

Contributor

@vivien You are absolutely correct. The way that Tig currently does colours is based on pairs of symbols. This is most obvious in an example like this:

screenshot from 2014-02-11 12 23 41

There were two hurdles I faced regarding a fix for this. One, the code changes would need to extend outside the graph specific code into the main Tig code, and two, my initial attempt at a fix had some issues with splitting up the UTF-8 character pairs. I'll probably try to fix this at some point in the future, but I have been buried in the graph code for several weeks now and need to take a bit of a breather. 😃

If you wanted to take a look at fixing it yourself, I suggest adding a secondary color to each character pair here and then drawing one character at a time in Tig somewhere around here.

Contributor

BenBergman commented Feb 11, 2014

@vivien You are absolutely correct. The way that Tig currently does colours is based on pairs of symbols. This is most obvious in an example like this:

screenshot from 2014-02-11 12 23 41

There were two hurdles I faced regarding a fix for this. One, the code changes would need to extend outside the graph specific code into the main Tig code, and two, my initial attempt at a fix had some issues with splitting up the UTF-8 character pairs. I'll probably try to fix this at some point in the future, but I have been buried in the graph code for several weeks now and need to take a bit of a breather. 😃

If you wanted to take a look at fixing it yourself, I suggest adding a secondary color to each character pair here and then drawing one character at a time in Tig somewhere around here.

@vivien

This comment has been minimized.

Show comment
Hide comment
@vivien

vivien Feb 11, 2014

Contributor

Thanks for your explanation @BenBergman

Contributor

vivien commented Feb 11, 2014

Thanks for your explanation @BenBergman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment