Re: [PATCH v3 1/2] commit-reach: properly peel tags
- From
Jeff King <peff@peff.net>
- Date
- Sep 21, 2018, 23:56 UTC
- Message-ID
- <20180921235628.GA3437@sigill.intra.peff.net>
- In-Reply-To
- <0a1e661271660b1fab317aac3997589a94b7c98f.1537542323.git.gitgitgadget@gmail.com>
On Fri, Sep 21, 2018 at 08:05:26AM -0700, Derrick Stolee via GitGitGadget wrote:
Show 31 quoted lines
> diff --git a/commit-reach.c b/commit-reach.c
> index 86715c103c..e748414d04 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -544,20 +544,42 @@ int can_all_from_reach_with_flag(struct object_array *from,
> {
> struct commit **list = NULL;
> int i;
> + int nr_commits;
> int result = 1;
>
> ALLOC_ARRAY(list, from->nr);
> + nr_commits = 0;
> for (i = 0; i < from->nr; i++) {
> - list[i] = (struct commit *)from->objects[i].item;
> + struct object *from_one = from->objects[i].item;
>
> - if (parse_commit(list[i]) ||
> - list[i]->generation < min_generation)
> - return 0;
> + if (!from_one || from_one->flags & assign_flag)
> + continue;
> +
> + from_one = deref_tag(the_repository, from_one,
> + "a from object", 0);
> + if (!from_one || from_one->type != OBJ_COMMIT) {
> + /* no way to tell if this is reachable by
> + * looking at the ancestry chain alone, so
> + * leave a note to ourselves not to worry about
> + * this object anymore.
> + */A minor nit, but the original comment you restored here has a style violation. Might be worth fixing up (but certainly not worth a re-roll on its own).
> + from->objects[i].item->flags |= assign_flag;
OK, so here we mark the original tag with a flag...
Show 7 quoted lines
> +
> + list[nr_commits] = (struct commit *)from_one;
> + if (parse_commit(list[nr_commits]) ||
> + list[nr_commits]->generation < min_generation) {
> + result = 0;
> + goto cleanup;
> + }And we jump to the cleanup here, but...
Show 8 quoted lines
> @@ -600,7 +622,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
> }
>
> cleanup:
> - for (i = 0; i < from->nr; i++) {
> + for (i = 0; i < nr_commits; i++) {
> clear_commit_marks(list[i], RESULT);
> clear_commit_marks(list[i], assign_flag);This walks over the items in the list array, which don't include the tag we marked. Did we decide that's OK? I think it's actually how the original code behaved, too, but it seems funny. At the least we might want to call it out in the commit message.
Should we also be walking from->objects and clearing the flags from there (maybe not RESULT, but assign_flag)? It's not clear to me if it's unintentional for those to be marked after the function leaves or not.
Also, a minor aside, but I think it would be slightly more efficient for those final two lines to do:
clear_commit_marks(list[i], RESULT | assign_flag);
Of course, that's totally orthogonal to this patch, but it may make you feel better to offset the other round of clearing I'm suggesting. ;)
> diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c > index eb21103998..08d2ea68e8 100644 > --- a/t/helper/test-reach.c > +++ b/t/helper/test-reach.c
These bits all looked good to me.
-Peff