Re: [PATCH] use pop_commit() for consuming the first entry of a struct commit_list

[René Scharfe]

Am 26.10.2015 um 22:33 schrieb Junio C Hamano:

René Scharfe  writes:


Instead of open-coding the function pop_commit() just call it.  This
makes the intent clearer and reduces code size.

Signed-off-by: Rene Scharfe 
---
  builtin/fmt-merge-msg.c |  9 +++--
  builtin/merge.c | 12 +---
  builtin/reflog.c|  6 +-
  builtin/rev-parse.c |  7 ++-
  builtin/show-branch.c   | 17 +++--
  commit.c| 27 +++
  remote.c|  6 ++
  revision.c  | 27 +--
  shallow.c   |  6 +-
  upload-pack.c   |  6 ++
  10 files changed, 31 insertions(+), 92 deletions(-)


While I appreciate this kind of simplification very much, I also
hate to review this kind of simplification at the same time, as it
is very easy to make and miss simple mistakes.


Yeah, it's tempting to doze off after a few similar cases.


Let me try to go through it very carefully.


diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 4ba7f28..846004b 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -537,7 +537,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
  static void find_merge_parents(struct merge_parents *result,
   struct strbuf *in, unsigned char *head)
  {
-   struct commit_list *parents, *next;
+   struct commit_list *parents;
struct commit *head_commit;
int pos = 0, i, j;

@@ -576,13 +576,10 @@ static void find_merge_parents(struct merge_parents 
*result,
parents = reduce_heads(parents);

while (parents) {
+   struct commit *cmit = pop_commit();
for (i = 0; i < result->nr; i++)
-   if (!hashcmp(result->item[i].commit,
-parents->item->object.sha1))
+   if (!hashcmp(result->item[i].commit, cmit->object.sha1))
result->item[i].used = 1;
-   next = parents->next;
-   free(parents);
-   parents = next;


OK, I would have called it "commit" not "cmit", but this is
trivially equivalent to the original.


This is just to keep the line shorter than 80 characters, and it's not 
the first "cmit" in the tree..





diff --git a/builtin/merge.c b/builtin/merge.c
index a0a9328..31241b8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1019,7 +1019,7 @@ static struct commit_list *reduce_parents(struct commit 
*head_commit,
  int *head_subsumed,
  struct commit_list *remoteheads)
  {
-   struct commit_list *parents, *next, **remotes = 
+   struct commit_list *parents, **remotes;


Interesting.  I viewed the result of applying this patch with "git
show -U32" to make sure that this initialization of remotes was
unnecessary.


With "git show -W" you don't need any magic number. ;)

Thanks for reviewing!

René
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] use pop_commit() for consuming the first entry of a struct commit_list

[Junio C Hamano]

René Scharfe  writes:

> Instead of open-coding the function pop_commit() just call it.  This
> makes the intent clearer and reduces code size.
>
> Signed-off-by: Rene Scharfe 
> ---
>  builtin/fmt-merge-msg.c |  9 +++--
>  builtin/merge.c | 12 +---
>  builtin/reflog.c|  6 +-
>  builtin/rev-parse.c |  7 ++-
>  builtin/show-branch.c   | 17 +++--
>  commit.c| 27 +++
>  remote.c|  6 ++
>  revision.c  | 27 +--
>  shallow.c   |  6 +-
>  upload-pack.c   |  6 ++
>  10 files changed, 31 insertions(+), 92 deletions(-)

While I appreciate this kind of simplification very much, I also
hate to review this kind of simplification at the same time, as it
is very easy to make and miss simple mistakes.

Let me try to go through it very carefully.

> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
> index 4ba7f28..846004b 100644
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -537,7 +537,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
>  static void find_merge_parents(struct merge_parents *result,
>  struct strbuf *in, unsigned char *head)
>  {
> - struct commit_list *parents, *next;
> + struct commit_list *parents;
>   struct commit *head_commit;
>   int pos = 0, i, j;
>  
> @@ -576,13 +576,10 @@ static void find_merge_parents(struct merge_parents 
> *result,
>   parents = reduce_heads(parents);
>  
>   while (parents) {
> + struct commit *cmit = pop_commit();
>   for (i = 0; i < result->nr; i++)
> - if (!hashcmp(result->item[i].commit,
> -  parents->item->object.sha1))
> + if (!hashcmp(result->item[i].commit, cmit->object.sha1))
>   result->item[i].used = 1;
> - next = parents->next;
> - free(parents);
> - parents = next;

OK, I would have called it "commit" not "cmit", but this is
trivially equivalent to the original.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index a0a9328..31241b8 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1019,7 +1019,7 @@ static struct commit_list *reduce_parents(struct commit 
> *head_commit,
> int *head_subsumed,
> struct commit_list *remoteheads)
>  {
> - struct commit_list *parents, *next, **remotes = 
> + struct commit_list *parents, **remotes;

Interesting.  I viewed the result of applying this patch with "git
show -U32" to make sure that this initialization of remotes was
unnecessary.

> @@ -1033,16 +1033,14 @@ static struct commit_list *reduce_parents(struct 
> commit *head_commit,
>   /* Find what parents to record by checking independent ones. */
>   parents = reduce_heads(remoteheads);
>  
> - for (remoteheads = NULL, remotes = 
> -  parents;
> -  parents = next) {
> - struct commit *commit = parents->item;
> - next = parents->next;
> + remoteheads = NULL;
> + remotes = 
> + while (parents) {
> + struct commit *commit = pop_commit();
>   if (commit == head_commit)
>   *head_subsumed = 0;
>   else
>   remotes = _list_insert(commit, remotes)->next;
> - free(parents);
>   }
>   return remoteheads;
>  }

Trivially equivalent.  Good.  I'll stop saying this if there is
nothing noteworthy from now on.

> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 092b59b..ac5141d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -53,17 +53,6 @@ static struct commit *interesting(struct commit_list *list)
>   return NULL;
>  }
>  
> -static struct commit *pop_one_commit(struct commit_list **list_p)
> -{
> - struct commit *commit;
> - struct commit_list *list;
> - list = *list_p;
> - commit = list->item;
> - *list_p = list->next;
> - free(list);
> - return commit;
> -}
> -

Interesting.  We had our own implementation that is less lenient
than the global one.  And this one is newer by two months!  Good
riddance.

> @@ -2733,10 +2726,7 @@ static void simplify_merges(struct rev_info *revs)
>   yet_to_do = NULL;
>   tail = _to_do;
>   while (list) {
> - commit = list->item;
> - next = list->next;
> - free(list);
> - list = next;
> + commit = pop_commit();
>   tail = simplify_one(revs, commit, tail);
>   }
>   }

Any conversion in this set that does not eliminate the intermediate
variable "next" (or named similarly but differently in some
contexts) is