diff mbox series

[v2] commit-graph: remove a duplicate assignment

Message ID 20190927021952.364646-1-alexhenrie24@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] commit-graph: remove a duplicate assignment | expand

Commit Message

Alex Henrie Sept. 27, 2019, 2:19 a.m. UTC
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Johannes Schindelin Sept. 27, 2019, 12:21 p.m. UTC | #1
Hi,


On Thu, 26 Sep 2019, Alex Henrie wrote:

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>

Hrm, the commit message should reflect Stolee's careful consideration,
no? An empty commit message won't be helpful e.g. when cherry-picking on
top of a branch that already has your v1.

Ciao,
Johannes

> ---
>  commit-graph.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 9b02d2c426..d0e1f9e1f2 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1522,7 +1522,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>
>  static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
>  {
> -	struct commit_graph *g = ctx->r->objects->commit_graph;
> +	struct commit_graph *g;
>  	uint32_t num_commits = ctx->commits.nr;
>  	uint32_t i;
>
> --
> 2.23.0
>
>
Derrick Stolee Sept. 27, 2019, 12:45 p.m. UTC | #2
On 9/27/2019 8:21 AM, Johannes Schindelin wrote:
> Hi,
> 
> 
> On Thu, 26 Sep 2019, Alex Henrie wrote:
> 
>> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> 
> Hrm, the commit message should reflect Stolee's careful consideration,
> no? An empty commit message won't be helpful e.g. when cherry-picking on
> top of a branch that already has your v1.

I agree that we need more in the commit messages here. It is
helpful to include _why_ you are doing this. Keep in mind
that your cover letter is not recorded with the commit at all.

Further, when updating a patch series, you should re-roll the
entire series as a v2 (preferably as a reply to the original
series). Keep in mind that this patch should NOT be applied
on top of your other series because then 'g' is not assigned
at all. Mechanically, it COULD apply, but it would break
in tests.

Patches 2 and 3 in your series also have feedback that would
be good to apply in your v2.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/commit-graph.c b/commit-graph.c
index 9b02d2c426..d0e1f9e1f2 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1522,7 +1522,7 @@  static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 
 static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
 {
-	struct commit_graph *g = ctx->r->objects->commit_graph;
+	struct commit_graph *g;
 	uint32_t num_commits = ctx->commits.nr;
 	uint32_t i;