Add navigation between merge commits #525

phi-gamma · 2016-09-18T22:02:09Z

Allow skipping forward and backward between merge commits by means of
two actions “move-next-merge” and “move-prev-merge”. The search
considers all lines containing the merge symbol.

phi-gamma · 2016-09-18T22:06:32Z

I’ve been wanting this feature since the first time I use tig ;) I consider
it immensely useful when browsing larger projects like the kernel:
Pull Linus’ tree first thing in the morning an flip between merges to
get a broad idea of what changed since your last fetch.

jonas

Thanks for this contribution. I think it is a nice feature, however, there're a couple of things I think are needed before this can be merged. Let me know if you want me to polish things up.

jonas · 2016-10-18T00:28:42Z

src/search.c

+		return error("Unknown search request");
+	}
+
+	if (!view->matched_lines && find_merges(view))


This will always return ERROR_OUT_OF_MEMORY when find_merges succeeds.

jonas · 2016-10-18T00:30:47Z

src/search.c

+		break;
+
+	default:
+		return error("Unknown search request");


I think this error message should be changed.

jonas · 2016-10-18T00:37:30Z

src/tig.c

@@ -292,6 +292,11 @@ view_driver(struct view *view, enum request request)
 		find_next(view, request);
 		break;

+	case REQ_MOVE_NEXT_MERGE:
+	case REQ_MOVE_PREV_MERGE:
+		find_merge(view, request);


This will segfault on views that does not have a graph. I suggest this only reports an error:

report("Moving between merge commits is not supported by the %s view", view->name);

And the find_merge cases are moved to main_request in src/main.c.

jonas · 2016-10-18T00:39:09Z

src/search.c

+		if (!view->ops->get_column_data(view, line, &column_data))
+			continue;
+
+		if (!column_data.graph->is_merge(canvas))


column_data.graph will be NULL here for all views except main.

jonas · 2016-10-18T00:39:52Z

src/search.c

+			return false;
+
+		view->line[lineno].search_result = true;
+		view->matched_line[view->matched_lines++] = lineno;


Any particular reason to reuse these member that are usually for search highlighting?

phi-gamma · 2016-10-18T21:04:52Z

···<date: 2016-10-17, Monday>···<from: Jonas Fonseca>···

Thanks for this contribution. I think it is a nice feature,
however, there're a couple of things I think are needed before
this can be merged. Let me know if you want me to polish things
up.

I very much appreciate the review. I’ll try to have something
ready till the end of the week. So far I’ve addressed most of
your points but I’m not sure how to proceed with this one:

  struct line *line = &view->line[lineno];

```
struct commit *commit = line->data;
```

struct graph_canvas *canvas = &commit->graph;

```
struct view_column_data column_data;
```

if (!view->ops->get_column_data(view, line, &column_data))

```
    continue;
```

if (!column_data.graph->is_merge(canvas))

```
    continue;
```

if (!realloc_unsigned_ints(&view->matched_line, view->matched_lines, 1))

```
    return false;
```

view->line[lineno].search_result = true;

view->matched_line[view->matched_lines++] = lineno;

Any particular reason to reuse these member that are usually for search highlighting?

No particular reason except to have the code as close to the
search routines as possible.

The two obvious options would be a) to extend struct view
with similar fields for collecting the merges, and b) to find the
next merge on demand. Personally I’d go with the latter because
it’s less complex. Any thoughts?

Best,
Philipp

Fixes a potential NULL deref. As per review: jonas#525 (comment)

Handle merge ops in main view and emit a message when they are encountered outside. As per review: jonas#525 (comment)

As per review: jonas#525 (comment)

…uest Emit an error message that indicates a bad request when looking for merge commits. As per review: jonas#525 (comment)

Find merges by traversing lines directly. Instead of collecting hte merges like matches, the next merge is determined starting from the current line. Whether the search wraps around is controlled by wrap-search. As per review: jonas#525 (comment)

phi-gamma · 2016-10-22T20:12:26Z

I’ve addressed each of your remarks in a single commit. After review
give me a note and I’ll squash them into a single changeset.

Allow skipping forward and backward between merge commits by means of two actions “move-next-merge” and “move-prev-merge”. The search considers all lines containing the merge symbol.

Fixes a potential NULL deref. As per review: jonas#525 (comment)

Handle merge ops in main view and emit a message when they are encountered outside. As per review: jonas#525 (comment)

As per review: jonas#525 (comment)

…uest Emit an error message that indicates a bad request when looking for merge commits. As per review: jonas#525 (comment)

Find merges by traversing lines directly. Instead of collecting hte merges like matches, the next merge is determined starting from the current line. Whether the search wraps around is controlled by wrap-search. As per review: jonas#525 (comment)

jonas · 2016-11-13T03:31:13Z

Thanks squashed and merged.

phi-gamma force-pushed the move-merge branch from 99bde99 to 009fa32 Compare September 19, 2016 05:22

jonas requested changes Oct 18, 2016

View reviewed changes

phi-gamma added a commit to phi-gamma/tig that referenced this pull request Oct 22, 2016

Check for NULL before using the graph

b8ae837

Fixes a potential NULL deref. As per review: jonas#525 (comment)

phi-gamma added a commit to phi-gamma/tig that referenced this pull request Oct 22, 2016

Handle merge navigation in main view

7ec4d5d

Handle merge ops in main view and emit a message when they are encountered outside. As per review: jonas#525 (comment)

phi-gamma added a commit to phi-gamma/tig that referenced this pull request Oct 22, 2016

Inverted borked check on calling find_merges()

e8f72ae

As per review: jonas#525 (comment)

phi-gamma added 5 commits October 22, 2016 22:30

Add navigation between merge commits

7a3c994

Allow skipping forward and backward between merge commits by means of two actions “move-next-merge” and “move-prev-merge”. The search considers all lines containing the merge symbol.

Check for NULL before using the graph

e42f1cc

Fixes a potential NULL deref. As per review: jonas#525 (comment)

Handle merge navigation in main view

60de877

Handle merge ops in main view and emit a message when they are encountered outside. As per review: jonas#525 (comment)

Inverted borked check on calling find_merges()

12e95f0

As per review: jonas#525 (comment)

Improve error message when calling find_next_merge() with unsound req…

62ee9a0

…uest Emit an error message that indicates a bad request when looking for merge commits. As per review: jonas#525 (comment)

phi-gamma force-pushed the move-merge branch from 0a8c055 to 4ce01c9 Compare October 22, 2016 20:30

Find next merge directly

40e8c63

Find merges by traversing lines directly. Instead of collecting hte merges like matches, the next merge is determined starting from the current line. Whether the search wraps around is controlled by wrap-search. As per review: jonas#525 (comment)

phi-gamma force-pushed the move-merge branch from 4ce01c9 to 40e8c63 Compare October 23, 2016 19:30

jonas closed this Nov 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add navigation between merge commits #525

Add navigation between merge commits #525

phi-gamma commented Sep 18, 2016

phi-gamma commented Sep 18, 2016

jonas left a comment

jonas Oct 18, 2016

jonas Oct 18, 2016

jonas Oct 18, 2016

jonas Oct 18, 2016

jonas Oct 18, 2016

phi-gamma commented Oct 18, 2016

phi-gamma commented Oct 22, 2016

jonas commented Nov 13, 2016

Add navigation between merge commits #525

Add navigation between merge commits #525

Conversation

phi-gamma commented Sep 18, 2016

phi-gamma commented Sep 18, 2016

jonas left a comment

Choose a reason for hiding this comment

jonas Oct 18, 2016

Choose a reason for hiding this comment

jonas Oct 18, 2016

Choose a reason for hiding this comment

jonas Oct 18, 2016

Choose a reason for hiding this comment

jonas Oct 18, 2016

Choose a reason for hiding this comment

jonas Oct 18, 2016

Choose a reason for hiding this comment

phi-gamma commented Oct 18, 2016

phi-gamma commented Oct 22, 2016

jonas commented Nov 13, 2016