Re: [PATCH v4] add --summary option to git-push and git-fetch
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 11, 2009, 19:05 UTC
- Message-ID
- <7viqhzm454.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20090711174156.GA17154@cthulhu>
Larry D'Anna <larry@elder-gods.org> writes:
Show 16 quoted lines
> --summary will cause git-push to output a one-line of each commit pushed. > --summary=n will display at most n commits for each ref pushed. > > $ git push --dry-run --summary origin : > To /home/larry/gitsandbox/a > 80f0e50..5593a38 master -> master > > 5593a38 foo > > 81c03f8 bar > Fetch works the same way. > > Signed-off-by: Larry D'Anna <larry@elder-gods.org> > --- > > Changes sicne last version of this patch: > > * added some tests
Thanks.
Show 10 quoted lines
> @@ -373,12 +379,15 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
> fputc(url[i], fp);
> fputc('\n', fp);
>
> - if (ref)
> - rc |= update_local_ref(ref, what, note);
> - else
> + if (ref) {
> + *quickref = 0;
> + rc |= update_local_ref(ref, what, note, quickref);Makes me wonder why update_local_ref() does not put that NUL upon entry.
Show 13 quoted lines
> +void print_summary_for_push_or_fetch(const char *quickref, int limit)
> +{
> + struct rev_info rev;
> + int i, max;
> + struct object *obj;
> + struct commit *commit;
> +
> + max = get_max_object_index();
> + for (i = 0; i < max; i++) {
> + obj = get_indexed_object(i);
> + if (obj)
> + obj->flags &= ~ALL_REV_FLAGS;
> + }Yuck; this is a horribly heavy sledgehammer. Couldn't you at least do clear_commit_marks() to limit the extent of the damage?
Show 9 quoted lines
> + init_revisions(&rev, NULL);
> + rev.prune = 0;
> + assert(!handle_revision_arg(quickref, &rev, 0, 1));
> + assert(!prepare_revision_walk(&rev));
> +
> + while ((commit = get_revision(&rev)) != NULL) {
> + struct strbuf buf = STRBUF_INIT;
> + if (limit == 0) {
> + fprintf(stderr, " ...\n");How would you know, when you asked 20 and you showed 20 here, that there is no more to come?
> + break; > + }
Show 9 quoted lines
> + if (!commit->buffer) {
> + enum object_type type;
> + unsigned long size;
> + commit->buffer =
> + read_sha1_file(commit->object.sha1, &type, &size);
> + if (!commit->buffer)
> + die("Cannot read commit %s", sha1_to_hex(commit->object.sha1));
> + }
> + format_commit_message(commit, " %m %h %s\n", &buf, 0);Hmm, why so many spaces before %m and after %m?
> -static int do_push(const char *repo, int flags) > +static int do_push(const char *repo, int flags, int summary)
Couldn't this be just another bit in the flag? I didn't check but I suspect you wouldn't have to touch the intermediate functions in the call chain that way.
Show 9 quoted lines
> diff --git a/builtin.h b/builtin.h > index 20427d2..5aea3a3 100644 > --- a/builtin.h > +++ b/builtin.h > @@ -113,4 +113,6 @@ extern int cmd_verify_pack(int argc, const char **argv, const char *prefix); > extern int cmd_show_ref(int argc, const char **argv, const char *prefix); > extern int cmd_pack_refs(int argc, const char **argv, const char *prefix); > > +extern void print_summary_for_push_or_fetch(const char *quickref, int limit);
Please; not at the end, but at the front where all the other command helpers live. I actually suspect that it might be better to migrate some part of builtin-log.c, together with your new helper function, to a new file log.c with accompanying header file log.h, but that could be a separate patch.
Show 13 quoted lines
> +test_expect_success 'fetch --summary' ' > + mk_empty && > + ( > + cd testrepo && > + git fetch --summary .. refs/heads/master:refs/remotes/origin/master 2>stderr && > + > + r=$(git show-ref -s --verify refs/remotes/origin/master) && > + test "z$r" = "z$the_commit" && > + > + test 1 = $(git for-each-ref refs/remotes/origin | wc -l) && > + > + grep -E "^ > [a-fA-F0-9]+ second$" stderr && > + grep -E "^ > [a-fA-F0-9]+ repo$" stderr
Look at the output from
$ git grep -n -e 'grep .*-E' -e 'egrep '
before applying your patch. I think we support people with grep that does not know about -E option.
Show 7 quoted lines
> +test_expect_success 'fetch --summary forced update' ' > + mk_empty && > + ( > ... > + ) > + > +'
There are at least two missing combinations. (1) "fetch --summary" to fetch a new branch, and (2) "fetch --summary" does not try segfaulting by accessing unavailable information after a failed fetch.
The same comment applies to the push side of the tests.
> -static void print_ok_ref_status(struct ref *ref, int porcelain) > +static void print_ok_ref_status(struct ref *ref, int porcelain, int summary)
The same comment on "flags" applies here and the all the functions in the call chain that adds this extra parameter. porcelain/summary should be a single int with two bits used. It may be cleaner to change "porcelain" to "a bit inside flag" in a separate patch _before_ this one, as a cleanup of the previous "add --porcelain option to git-push" patch.