threads / discuss / 22298

git notes: notes

Subject: git notes: notes

## tl;dr

36 messages between Jan 20, 2010 and Jan 27, 2010.

replies: 35people: 11as markdown or json

Joey Hess· Jan 20, 2010, 05:03 UTC · lore

Just a quick note that the new notes feature can break things that parse git log. For example a parser that assumes it can split the log on blank lines to separate the header and commit message, can easily become confused by the new blank line before "Notes:".

Might be worth documenting in release notes, maybe too late now though. But really, it's all good, notes are a great feature.

PS, Has anyone thought about using notes to warn bisect away from commits that are known to be unbuildable or otherwise cause bisection trouble?

-- 
see shy jo
Thomas Rast· Jan 20, 2010, 09:48 UTC · re: Joey Hess · lore

Re: git notes: notes

Joey Hess wrote:
> Just a quick note that the new notes feature can break things that parse
> git log.

Umm. git-log is porcelain and we're allowed to change it. Worse, even the user can change it in very significant ways, just try:

  git config format.pretty email
  git log

For a better alternative, I'm afraid you'll either have to look to git-rev-list (which also takes --pretty) or 'git cat-file --batch'.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Joey Hess· Jan 20, 2010, 18:14 UTC · re: Thomas Rast · lore

Re: git notes: notes

Thomas Rast wrote:
Show 5 quoted lines
> Umm.  git-log is porcelain and we're allowed to change it.  Worse,
> even the user can change it in very significant ways, just try:
> 
>   git config format.pretty email
>   git log

Is git log --pretty=raw --raw really intended to be porcelain? Above does not affect it.

> For a better alternative, I'm afraid you'll either have to look to
> git-rev-list (which also takes --pretty) or 'git cat-file --batch'.
I don't see a way to get the per-commit diff-tree info using rev-list.
-- 
see shy jo
Johan Herland· Jan 20, 2010, 10:48 UTC · re: Joey Hess · lore

Re: git notes: notes

On Wednesday 20 January 2010, Joey Hess wrote:
> Just a quick note that the new notes feature can break things that parse
> git log. For example a parser that assumes it can split the log on blank
> lines to separate the header and commit message, can easily become
> confused by the new blank line before "Notes:".

As Thomas already stated, git log is porcelain, and its output format is not set in stone. If you need a stable, script-friendly format, you should probably use the --format option, or use plumbing instead (such as e.g. git rev-list, which also has a --format option).

Show 6 quoted lines
> Might be worth documenting in release notes, maybe too late now though.
> But really, it's all good, notes are a great feature.
> 
> PS, Has anyone thought about using notes to warn bisect away from
> commits that are known to be unbuildable or otherwise cause bisection
> trouble?
No, I haven't thought of that specific use case. Great idea! :)

BTW, since I started talking about git notes, people on this list have found more and more interesting use cases for them:

- Free-form text extension to the commit message
- Help in bug tracking with header-like lines such as:
    - Causes-Bug: #12345
    - Fixes-Bug: #54321
- Store after-the-fact "Acked-By", "Reviewed-By", etc. annotations
- In a repo converted from a merge-unfriendly VCS (such as CVS), use notes
  to identify merges without having to rewrite Git history (note that you
  can also use grafts, or "git replace" to accomplish this).
- Refer to related commits elsewhere in the repo (i.e. relationships that
  are not already apparent from the commit graph)
- When cherry-picking, add a reverse link from the source commit to the
  cherry-picked commit (since it may be of interest to people reviewing the
  source commit
- Rebasing public branches is forbidden, but if you wanted to change that,
  you could potentially help solve it by using notes to add reverse links
  from source commits to rebased commits, so that downstream people could
  more easily traverse your history when rebasing/merging their own
  branches.
- Initially, there were some discussion whether it could also be used to
  guide git blame to make better decisions, although I don't currently see
  how that would be done in practice.

In any case, it seems the notes idea may have the potential to become one of the more useful features in Git.

Have fun! :)
...Johan
[1]: ...almost 3 years ago (wow, time flies...):
     http://article.gmane.org/gmane.comp.version-control.git/46883
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Joey Hess· Jan 20, 2010, 18:24 UTC · re: Johan Herland · lore

Re: git notes: notes

Johan Herland wrote:
> As Thomas already stated, git log is porcelain, and its output format is not 
> set in stone. If you need a stable, script-friendly format, you should 
> probably use the --format option, or use plumbing instead (such as e.g. git 
> rev-list, which also has a --format option).
But git log --format=raw --raw output was changed by notes.
Show 8 quoted lines
> > Might be worth documenting in release notes, maybe too late now though.
> > But really, it's all good, notes are a great feature.
> > 
> > PS, Has anyone thought about using notes to warn bisect away from
> > commits that are known to be unbuildable or otherwise cause bisection
> > trouble?
> 
> No, I haven't thought of that specific use case. Great idea! :)

Only problem I see with doing it is it might be too easy to overwrite such a note with git notes edit -m

Did you consider having -m append a line to an existing note?
-- 
see shy jo
Junio C Hamano· Jan 20, 2010, 19:30 UTC · re: Joey Hess · lore

Re: git notes: notes

Joey Hess <joey@kitenet.net> writes:
Show 7 quoted lines
> Johan Herland wrote:
>> As Thomas already stated, git log is porcelain, and its output format is not 
>> set in stone. If you need a stable, script-friendly format, you should 
>> probably use the --format option, or use plumbing instead (such as e.g. git 
>> rev-list, which also has a --format option).
>
> But git log --format=raw --raw output was changed by notes.

Let me ask a stupid question. Did the output change before and after the notes code even when your history does not have notes?

>> > Might be worth documenting in release notes, maybe too late now though.

This depends on the answer to the above question. If the answers is "No", then I don't see the need to say much more than "New 'git notes' feature allows comments applied to existing commits after the fact to be shown by log and friends". If it is "Yes", we should fix the code not to change the output.

In any case, "log" is still a Porcelain, so it is understandable that by triggering a new feature you would get output from the new feature. It is called progress ;-)

Joey Hess· Jan 20, 2010, 19:56 UTC · re: Junio C Hamano · lore

Re: git notes: notes

Junio C Hamano wrote:
> Let me ask a stupid question.  Did the output change before and after the
> notes code even when your history does not have notes?
No. 
Show 11 quoted lines
> >> > Might be worth documenting in release notes, maybe too late now though.
> 
> This depends on the answer to the above question.  If the answers is "No",
> then I don't see the need to say much more than "New 'git notes' feature
> allows comments applied to existing commits after the fact to be shown by
> log and friends".  If it is "Yes", we should fix the code not to change
> the output.
> 
> In any case, "log" is still a Porcelain, so it is understandable that by
> triggering a new feature you would get output from the new feature.  It is
> called progress ;-)

Do you think it makes sense for even git log --format=format:%s to be porcelain and potentially change when new features are used? Seems to me that parts of git log walk the line between porcelain and plumbing. So it's not clear which parts are safe to use.

-- 
see shy jo
Junio C Hamano· Jan 20, 2010, 20:10 UTC · re: Joey Hess · lore

Re: git notes: notes

Joey Hess <joey@kitenet.net> writes:
> Do you think it makes sense for even git log --format=format:%s to be
> porcelain and potentially change when new features are used?

If the series changed the meaning of "%s" format to mean "the subject of the commit and notes information", with or without documenting it, then it is just a bug we would like to fix.

But I cannot reproduce such a bug.  In my tree locally:
    $ git notes show a97a74
    Origin of commit notes feature was at
    a97a74).
    $ git show -s --pretty=short a97a74
    commit a97a74686d70a318cd802003498054cc1e8b0ae2
    Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
        Introduce commit notes
    Notes:
        Origin of commit notes feature was at
        a97a74).
    $ git show -s --pretty=format:%s a97a74
    Introduce commit notes
Puzzled...
Joey Hess· Jan 20, 2010, 20:36 UTC · re: Junio C Hamano · lore

Re: git notes: notes

Junio C Hamano wrote:
Show 10 quoted lines
> Joey Hess <joey@kitenet.net> writes:
> 
> > Do you think it makes sense for even git log --format=format:%s to be
> > porcelain and potentially change when new features are used?
> 
> If the series changed the meaning of "%s" format to mean "the subject of
> the commit and notes information", with or without documenting it, then it
> is just a bug we would like to fix.
> 
> But I cannot reproduce such a bug.  In my tree locally:

I was asking hypothetically, trying to point out that parts of git log seem to make sense to be used as plumbing, with the hope I can continue to use it that way.

(Note that git instaweb parses output of git log --pretty=format:%H --raw like it's plumbing.)

-- 
see shy jo
Jeff King· Jan 20, 2010, 20:54 UTC · re: Joey Hess · lore

Re: git notes: notes

On Wed, Jan 20, 2010 at 03:36:36PM -0500, Joey Hess wrote:
Show 6 quoted lines
> I was asking hypothetically, trying to point out that parts of git log
> seem to make sense to be used as plumbing, with the hope I can continue
> to use it that way.
> 
> (Note that git instaweb parses output of git log --pretty=format:%H --raw
> like it's plumbing.)

I think this is a valid point. Note that "gitk" uses "git log --pretty=raw". However, I believe it splits the entries on "^commit". So I think there is some precedent for scripting "git log"; it has features that are simply not available through other interfaces. And scripting around "--pretty=raw" seems pretty reasonable to me, too. Why else would you want the raw format?

Is splitting on blank lines an error? I don't think so. The original format was never strictly defined, but given the --pretty=raw format, it seems like a fairly obvious thing to do.

I am inclined to cut the notes output from --pretty=raw, and let callers ask for them explicitly with --show-notes or something similar. We can leave them on by default in the "normal" output. This will still break scripts doing "git log | ./script", but I don't think we have ever condoned that practice.

-Peff
Junio C Hamano· Jan 20, 2010, 20:59 UTC · re: Jeff King · lore

Re: git notes: notes

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> Is splitting on blank lines an error? I don't think so. The original
> format was never strictly defined, but given the --pretty=raw format, it
> seems like a fairly obvious thing to do.
>
> I am inclined to cut the notes output from --pretty=raw, and let callers
> ask for them explicitly with --show-notes or something similar. We can
> leave them on by default in the "normal" output. This will still break
> scripts doing "git log | ./script", but I don't think we have ever
> condoned that practice.
Sounds like a plan.
Jeff King· Jan 20, 2010, 21:31 UTC · re: Junio C Hamano · lore

Re: git notes: notes

On Wed, Jan 20, 2010 at 12:59:38PM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > Is splitting on blank lines an error? I don't think so. The original
> > format was never strictly defined, but given the --pretty=raw format, it
> > seems like a fairly obvious thing to do.
> >
> > I am inclined to cut the notes output from --pretty=raw, and let callers
> > ask for them explicitly with --show-notes or something similar. We can
> > leave them on by default in the "normal" output. This will still break
> > scripts doing "git log | ./script", but I don't think we have ever
> > condoned that practice.
> 
> Sounds like a plan.
We can start with this patch, which clears up Joey's problem.
-- >8 --
Subject: [PATCH] don't show notes for --pretty=raw

The --pretty=raw format of the log family is likely to be used by scripts. Such scripts may parse the output into records on blank lines, since doing so in the past has always worked. However, with the recently added notes output, such parsers will see an extra stanza for any commits that have notes.

This patch turns off the notes output for the raw format to avoid breaking such scripts.

Signed-off-by: Jeff King <peff@peff.net>
---
 pretty.c         |    2 +-
 t/t3301-notes.sh |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/pretty.c b/pretty.c
index 9001379..0674027 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1094,7 +1094,7 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	if (fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
 		strbuf_addch(sb, '\n');
 
-	if (fmt != CMIT_FMT_ONELINE)
+	if (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW)
 		get_commit_notes(commit, sb, encoding,
 				 NOTES_SHOW_HEADER | NOTES_INDENT);
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 1e34f48..4c3de9d 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -147,4 +147,18 @@ test_expect_success 'show -m and -F notes' '
 	test_cmp expect-m-and-F output
 '
 
+cat >expect << EOF
+commit 15023535574ded8b1a89052b32673f84cf9582b8
+tree e070e3af51011e47b183c33adf9736736a525709
+parent 1584215f1d29c65e99c6c6848626553fdd07fd75
+author A U Thor <author@example.com> 1112912173 -0700
+committer C O Mitter <committer@example.com> 1112912173 -0700
+
+    4th
+EOF
+test_expect_success 'git log --pretty=raw does not show notes' '
+	git log -1 --pretty=raw >output &&
+	test_cmp expect output
+'
+
 test_done
-- 
1.6.6.510.g159cf
Jeff King· Jan 20, 2010, 21:41 UTC · re: Jeff King · lore

Re: git notes: notes

On Wed, Jan 20, 2010 at 04:31:38PM -0500, Jeff King wrote:
Show 14 quoted lines
> We can start with this patch, which clears up Joey's problem.
> 
> -- >8 --
> Subject: [PATCH] don't show notes for --pretty=raw
> 
> The --pretty=raw format of the log family is likely to be
> used by scripts. Such scripts may parse the output into
> records on blank lines, since doing so in the past has
> always worked. However, with the recently added notes
> output, such parsers will see an extra stanza for any
> commits that have notes.
> 
> This patch turns off the notes output for the raw format to
> avoid breaking such scripts.

And the second half would be something like the patch below implementing --show-notes, so that things like "gitk" which can handle the new feature can turn it on.

I'm not that happy with this patch, though. If we have --show-notes, we should probably have --no-show-notes, which this doesn't do, since a lack of --show-notes simply means "do whatever the format dictates".

Also, passing everything through a pretty_print_context seems kind of hack-ish, as we just end up copying items from the rev-list context into the pp_context. I did so here only in the log_tree_commit case. I don't think there are other places which would want to propagate it, but I might have missed one. I wonder if we would do better to simply past the rev-list options into pretty_print_commit and let it pick what it wants out of the struct.

Anyway, I am out of time to work on git for now, so maybe somebody who cares more about notes can pick this up. I think the first patch (the one I am replying to) should definitely go in to un-break Joey's case (or an alternative patch that turns it off in even more cases), and people who care about it can fight about whether and by what mechanism things like gitk should see the notes.

-- >8 --
Subject: [PATCH] teach log family to --show-notes

Most log formats will implicitly show commit notes, but the raw format will not. Callers can now explicitly call --show-notes to enable them.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit.h         |    1 +
 log-tree.c       |    1 +
 pretty.c         |    3 ++-
 revision.c       |    2 ++
 revision.h       |    1 +
 t/t3301-notes.sh |   16 ++++++++++++++++
 6 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/commit.h b/commit.h
index 24128d7..4646751 100644
--- a/commit.h
+++ b/commit.h
@@ -71,6 +71,7 @@ struct pretty_print_context
 	enum date_mode date_mode;
 	int need_8bit_cte;
 	struct reflog_walk_info *reflog_info;
+	int show_notes;
 };
 
 extern int has_non_ascii(const char *text);
diff --git a/log-tree.c b/log-tree.c
index 0fdf159..9155a31 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -412,6 +412,7 @@ void show_log(struct rev_info *opt)
 	ctx.abbrev = opt->diffopt.abbrev;
 	ctx.after_subject = extra_headers;
 	ctx.reflog_info = opt->reflog_info;
+	ctx.show_notes = opt->show_notes;
 	pretty_print_commit(opt->commit_format, commit, &msgbuf, &ctx);
 
 	if (opt->add_signoff)
diff --git a/pretty.c b/pretty.c
index 0674027..95fe39a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1094,7 +1094,8 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	if (fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
 		strbuf_addch(sb, '\n');
 
-	if (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW)
+	if (context->show_notes ||
+	    (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW))
 		get_commit_notes(commit, sb, encoding,
 				 NOTES_SHOW_HEADER | NOTES_INDENT);
 
diff --git a/revision.c b/revision.c
index 7328201..3815dd3 100644
--- a/revision.c
+++ b/revision.c
@@ -1175,6 +1175,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		get_commit_format("oneline", revs);
 		revs->abbrev_commit = 1;
+	} else if (!strcmp(arg, "--show-notes")) {
+		revs->show_notes = 1;
 	} else if (!strcmp(arg, "--graph")) {
 		revs->topo_order = 1;
 		revs->rewrite_parents = 1;
diff --git a/revision.h b/revision.h
index d368003..e51842f 100644
--- a/revision.h
+++ b/revision.h
@@ -83,6 +83,7 @@ struct rev_info {
 			abbrev_commit:1,
 			use_terminator:1,
 			missing_newline:1,
+			show_notes:1,
 			date_mode_explicit:1;
 	unsigned int	disable_stdin:1;
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 4c3de9d..d51e8bd 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -161,4 +161,20 @@ test_expect_success 'git log --pretty=raw does not show notes' '
 	test_cmp expect output
 '
 
+cat >>expect <<EOF
+
+Notes:
+    spam
+$whitespace
+    xyzzy
+$whitespace
+    foo
+    bar
+    baz
+EOF
+test_expect_success 'git log --show-notes' '
+	git log -1 --pretty=raw --show-notes >output &&
+	test_cmp expect output
+'
+
 test_done
-- 
1.6.6.510.g159cf
Junio C Hamano· Jan 20, 2010, 22:07 UTC · re: Jeff King · lore

Re: git notes: notes

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> diff --git a/pretty.c b/pretty.c
> index 0674027..95fe39a 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1094,7 +1094,8 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
>  	if (fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
>  		strbuf_addch(sb, '\n');
>  
> -	if (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW)
> +	if (context->show_notes ||
> +	    (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW))
>  		get_commit_notes(commit, sb, encoding,
>  				 NOTES_SHOW_HEADER | NOTES_INDENT);

Heh, without this hunk I would have thought Peff and Gitster were the same person ;-).

Once you introduce --no-notes, the above condition would not work well.
Jeff King· Jan 20, 2010, 22:21 UTC · re: Junio C Hamano · lore

Re: git notes: notes

On Wed, Jan 20, 2010 at 02:07:47PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> > -	if (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW)
> > +	if (context->show_notes ||
> > +	    (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW))
> >  		get_commit_notes(commit, sb, encoding,
> >  				 NOTES_SHOW_HEADER | NOTES_INDENT);
> 
> Heh, without this hunk I would have thought Peff and Gitster were the same
> person ;-).
> 
> Once you introduce --no-notes, the above condition would not work well.

Yeah, I know, or I would have just added the 2 lines for --no-show-notes. :) I think your patch is better; I'll comment on it separately.

-Peff
Michael J Gruber· Jan 20, 2010, 21:02 UTC · re: Junio C Hamano · lore

Re: git notes: notes

Junio C Hamano venit, vidit, dixit 20.01.2010 21:10:
Show 8 quoted lines
> Joey Hess <joey@kitenet.net> writes:
> 
>> Do you think it makes sense for even git log --format=format:%s to be
>> porcelain and potentially change when new features are used?
> 
> If the series changed the meaning of "%s" format to mean "the subject of
> the commit and notes information", with or without documenting it, then it
> is just a bug we would like to fix.

No, but outputting the note as part of the log is the standard. So for example, when you do a format-patch | apply cycle, format-patch will insert the note as part of the commit message, and apply will *store* the note text (including Note:\n) as part of the commit message of the new commit.

So, I would say the notes feature is not that well integrated right now, and either log has to learn --no-notes (and format-patch has to use it, or rather the corresponding internal flag), or apply has to learn to parse "Note:" headers. Or, depending on how you use notes, it may be better if format-patch puts the note after the "--"; that way you can store the usual "after-the-message" patch comments in a note.

Similarly, I don't think rebasing and cherry-picking adjust the notes tree for commits with notes whose sha1 changes - which may or may not be the appropriate behaviour.

In both cases, the "right" way depends on how you use notes, and there should be an easy way to specify your choice.

I'm not complaining, I actually have this on a maybe-to-do list, but the way the series went kept me from investing time.

Cheers, Michael

Junio C Hamano· Jan 20, 2010, 21:08 UTC · re: Michael J Gruber · lore

Re: git notes: notes

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 15 quoted lines
> Junio C Hamano venit, vidit, dixit 20.01.2010 21:10:
>> Joey Hess <joey@kitenet.net> writes:
>> 
>>> Do you think it makes sense for even git log --format=format:%s to be
>>> porcelain and potentially change when new features are used?
>> 
>> If the series changed the meaning of "%s" format to mean "the subject of
>> the commit and notes information", with or without documenting it, then it
>> is just a bug we would like to fix.
>
> No, but outputting the note as part of the log is the standard. So for
> example, when you do a format-patch | apply cycle, format-patch will
> insert the note as part of the commit message, and apply will *store*
> the note text (including Note:\n) as part of the commit message of the
> new commit.

Thanks; that was the kind of breakage report I was looking for (and wished to have heard a lot earlier). Personally I find it is unexcusable that format-patch defaults to giving notes.

> So, I would say the notes feature is not that well integrated right now,
No question about it.
> I'm not complaining, I actually have this on a maybe-to-do list, but the
> way the series went kept me from investing time.

Hmm, that hints there is a failure in the review and merge process. Care to explain how we could have done better please?

Jeff King· Jan 20, 2010, 21:36 UTC · re: Junio C Hamano · lore

Re: git notes: notes

On Wed, Jan 20, 2010 at 01:08:07PM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> > No, but outputting the note as part of the log is the standard. So for
> > example, when you do a format-patch | apply cycle, format-patch will
> > insert the note as part of the commit message, and apply will *store*
> > the note text (including Note:\n) as part of the commit message of the
> > new commit.
> 
> Thanks; that was the kind of breakage report I was looking for (and wished
> to have heard a lot earlier).  Personally I find it is unexcusable that
> format-patch defaults to giving notes.

I agree. I noticed this while doing the "don't show in raw" feature elsewhere in the thread and wanted to ask: which formats _should_ have notes by default?

To be honest, I am not sure _any_ format should have it by default. If I am running "git log" and my notes are filled with random automatically generated bisection cruft, I don't want to see that cluttering my output. Yes, all of our test notes are human-written annotations, but I think we really don't know yet what sorts of things people will be putting in them.

Long ago I proposed a set of notes namespaces to deal with this (so automatic bisection cruft would go into its own notes namespace, and human-readable ones would be in some default namespace), but I don't know how much of that idea (if any) survived into the current implementation.

Show 5 quoted lines
> > I'm not complaining, I actually have this on a maybe-to-do list, but the
> > way the series went kept me from investing time.
> 
> Hmm, that hints there is a failure in the review and merge process.  Care
> to explain how we could have done better please?

Personally, I stopped paying attention simply because it was gigantic and I am not all that interested in using the feature personally.

-Peff
Junio C Hamano· Jan 20, 2010, 21:59 UTC · re: Junio C Hamano · lore

Re: git notes: notes

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> No, but outputting the note as part of the log is the standard. So for
>> example, when you do a format-patch | apply cycle, format-patch will
>> insert the note as part of the commit message, and apply will *store*
>> the note text (including Note:\n) as part of the commit message of the
>> new commit.
>
> Thanks; that was the kind of breakage report I was looking for (and wished
> to have heard a lot earlier).  Personally I find it is unexcusable that
> format-patch defaults to giving notes.
>
>> So, I would say the notes feature is not that well integrated right now,
>
> No question about it.
How about solving it this way?

It _could_ break some tests, if the set of tests were carefully written to cover not only the positive ("I am showing off my shiny new toy") cases but also the negative ("These commands share the same codepath touched by the series, but I don't intend to change their behaviour, and here is to make sure the new toy does not affect them") cases and the latter set assumed it is ok to sprinkle notes in commit log messages without being asked, but I haven't tried running the test suite yet.

---
Subject: Fix "log" family not to be too agressive about showing notes

Giving "Notes" information in the default output format of "log" and "show" is a sensible progress (the user has asked for it by having the notes), but for some commands (e.g. "format-patch") spewing notes into the formatted commit log message without being asked is too aggressive.

Enable notes output only for "log", "show", "whatchanged" by default; other users can ask for it by setting show_notes field to true.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-log.c |    2 ++
 commit.h      |    1 +
 log-tree.c    |    1 +
 pretty.c      |    2 +-
 revision.c    |    4 ++++
 revision.h    |    1 +
 6 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 41b6df4..da0ba1d 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -41,6 +41,8 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	rev->commit_format = CMIT_FMT_DEFAULT;
 	if (fmt_pretty)
 		get_commit_format(fmt_pretty, rev);
+	else
+		rev->show_notes = 1;
 	rev->verbose_header = 1;
 	DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
 	rev->show_root_diff = default_show_root;
diff --git a/commit.h b/commit.h
index e5332ef..2c0742b 100644
--- a/commit.h
+++ b/commit.h
@@ -70,6 +70,7 @@ struct pretty_print_context
 	const char *after_subject;
 	enum date_mode date_mode;
 	int need_8bit_cte;
+	int show_notes;
 	struct reflog_walk_info *reflog_info;
 };
 
diff --git a/log-tree.c b/log-tree.c
index 0fdf159..27afcf6 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -284,6 +284,7 @@ void show_log(struct rev_info *opt)
 	struct pretty_print_context ctx = {0};
 
 	opt->loginfo = NULL;
+	ctx.show_notes = opt->show_notes;
 	if (!opt->verbose_header) {
 		graph_show_commit(opt->graph);
 
diff --git a/pretty.c b/pretty.c
index 8f5bd1a..b2ee7fe 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1094,7 +1094,7 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	if (fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
 		strbuf_addch(sb, '\n');
 
-	if (fmt != CMIT_FMT_ONELINE)
+	if (context->show_notes)
 		get_commit_notes(commit, sb, encoding,
 				 NOTES_SHOW_HEADER | NOTES_INDENT);
 
diff --git a/revision.c b/revision.c
index 25fa14d..03c280f 100644
--- a/revision.c
+++ b/revision.c
@@ -1165,6 +1165,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!prefixcmp(arg, "--pretty=") || !prefixcmp(arg, "--format=")) {
 		revs->verbose_header = 1;
 		get_commit_format(arg+9, revs);
+	} else if (!strcmp(arg, "--show-notes")) {
+		revs->show_notes = 1;
+	} else if (!strcmp(arg, "--no-notes")) {
+		revs->show_notes = 0;
 	} else if (!strcmp(arg, "--oneline")) {
 		revs->verbose_header = 1;
 		get_commit_format("oneline", revs);
diff --git a/revision.h b/revision.h
index d368003..4167c1e 100644
--- a/revision.h
+++ b/revision.h
@@ -80,6 +80,7 @@ struct rev_info {
 	/* Format info */
 	unsigned int	shown_one:1,
 			show_merge:1,
+			show_notes:1,
 			abbrev_commit:1,
 			use_terminator:1,
 			missing_newline:1,
Jeff King· Jan 20, 2010, 22:25 UTC · re: Junio C Hamano · lore

Re: git notes: notes

On Wed, Jan 20, 2010 at 01:59:36PM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> Subject: Fix "log" family not to be too agressive about showing notes
> 
> Giving "Notes" information in the default output format of "log" and
> "show" is a sensible progress (the user has asked for it by having the
> notes), but for some commands (e.g. "format-patch") spewing notes into the
> formatted commit log message without being asked is too aggressive.
> 
> Enable notes output only for "log", "show", "whatchanged" by default;
> other users can ask for it by setting show_notes field to true.

What I didn't get out of reading this but did from reading the code (I think) is what you meant by "by default" here. That is, doing:

  git log
will show notes, but neither
  git log --pretty=raw
nor even
  git log --pretty=medium

will do so, even though the latter otherwise produces identical output to the default.

That seems like a reasonable rule to me, but I just wanted to make sure that was both what was happening and what was intended.

Show 9 quoted lines
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin-log.c |    2 ++
>  commit.h      |    1 +
>  log-tree.c    |    1 +
>  pretty.c      |    2 +-
>  revision.c    |    4 ++++
>  revision.h    |    1 +
>  6 files changed, 10 insertions(+), 1 deletions(-)

No tests or docs, of course. :) You can squash the --pretty=raw test from my patch, but you will need to exercise --show-notes and --no-show-notes, too, as well as checking other formats and things like format-patch. So probably writing your own tests will make it easier to more thoroughly check each case.

-Peff
Junio C Hamano· Jan 20, 2010, 22:33 UTC · re: Jeff King · lore

Re: git notes: notes

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> No tests or docs, of course. :) You can squash the --pretty=raw test
> from my patch, but you will need to exercise --show-notes and
> --no-show-notes, too, as well as checking other formats and things like
> format-patch. So probably writing your own tests will make it easier to
> more thoroughly check each case.
Thanks, but Ugh.

Didn't I say elsewhere that I am too busy to become a janitor for everybody's itch, especially for topics that are merely "Meh" to me?

If there is no volunteer, I might be forced to do something about it, but no promises, and I am reasonably certain that not much will happen at my end for coming 48 hours, as I am cutting 1.6.6.1 (and perhaps 1.6.5.8) and looking at other topics in 'next' that deserve to go to 1.7.0-rc0.

Johannes Schindelin· Jan 20, 2010, 22:58 UTC · re: Junio C Hamano · lore

Re: git notes: notes

Hi,
On Wed, 20 Jan 2010, Junio C Hamano wrote:
Show 12 quoted lines
> Subject: Fix "log" family not to be too agressive about showing notes
> 
> Giving "Notes" information in the default output format of "log" and
> "show" is a sensible progress (the user has asked for it by having the
> notes), but for some commands (e.g. "format-patch") spewing notes into the
> formatted commit log message without being asked is too aggressive.
> 
> Enable notes output only for "log", "show", "whatchanged" by default;
> other users can ask for it by setting show_notes field to true.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

Makes sense, and the patch actually removes what could be seen as an ugly side effect (why it only ONELINE not getting notes?).

I would agree with Peff about the mention of --pretty disabling notes (unless asked for by a user format) in the commit notes as well as in the pretty options, but I fully disagree on the need for tests. We should not have a thorough test suite that runs for days on end, but we should concentrate on things that are more likely to get broken. And the added code is just too obvious for that.

(Anybody remember the initial suggestion for testing git-commit before making it builtin? It had something like 70 tests.)

Ciao, Dscho

Jeff King· Jan 20, 2010, 23:06 UTC · re: Johannes Schindelin · lore

Re: git notes: notes

On Wed, Jan 20, 2010 at 11:58:07PM +0100, Johannes Schindelin wrote:
Show 6 quoted lines
> I would agree with Peff about the mention of --pretty disabling notes 
> (unless asked for by a user format) in the commit notes as well as in the 
> pretty options, but I fully disagree on the need for tests.  We should not 
> have a thorough test suite that runs for days on end, but we should 
> concentrate on things that are more likely to get broken.  And the added 
> code is just too obvious for that.

Sure, we don't have to go all out. But I think there is some confusion right now about just what behavior we _should_ have, so I think documenting it in the form of tests is reasonable.

-Peff
Junio C Hamano· Jan 20, 2010, 23:14 UTC · re: Johannes Schindelin · lore

Re: git notes: notes

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Makes sense, and the patch actually removes what could be seen as an ugly 
> side effect (why it only ONELINE not getting notes?).
Thanks.

The motivation was that the user should be able to get notes even under ONELINE mode if desired. But then the call to get_commit_notes() may want to inspect the commit format being used and tweak the flag parameter; right now it always sends NOTES_SHOW_HEADER and NOTES_INDENT.

> I would agree with Peff about the mention of --pretty disabling notes 
> (unless asked for by a user format) in the commit notes as well as in the 
> pretty options,...

Actually I am of two minds regarding --pretty={short,medium} and the like. The "how about this" patch may be the safest for people who are used to read "log --pretty=xxx" output with scripts, but it does look inconsistent and hard to explain to new people who do not even know that there were versions of git that does not know about notes.

> but I fully disagree on the need for tests.  We should not 
> have a thorough test suite that runs for days on end, but we should 
> concentrate on things that are more likely to get broken.  And the added 
> code is just too obvious for that.

I agree with that principle, but it doesn't explain nor justify the lack of tests for format-patch, which would have caught the breakage a lot earlier.

Or perhaps we all (not just you but I am just as guilty) misjudged "things that are more likely to get broken", even though we are very well aware that touching log-tree infrastructure will have fallout all over the "log" family.

Johan Herland· Jan 21, 2010, 02:54 UTC · re: Junio C Hamano · lore

Re: git notes: notes

On Thursday 21 January 2010, Junio C Hamano wrote:
> [...]

I just want to note that I've read the whole thread (up to here), and I agree with pretty much everything that's been said so far:

- We should be more conservative about showing notes, especially in contexts 
that may be used by scripts. Disabling notes by default when --pretty/--
format is in use, sounds like a good idea. So does adding a --show-notes 
option for overriding the default.
- The format-patch bug is grave and unexcusable and must be fixed. Michael: 
Thanks for discovering.
- I'd still like to keep notes as part of the default output from git log 
and friends (when NOT using --pretty/--format). Only notes from a single 
notes ref (typically the default "refs/notes/commits") should be shown.
- Re. Peff's worry that "git log" will fill up with random bisection cruft: 
Any notes that are related to bisection (or any other special use case for 
notes) should live on its own notes ref (typically "refs/notes/bisect" for 
bisection cruft) that is not used by "git log" (unless you explicitly say so 
through $GIT_NOTES_REF or core.notesRef).
- Re. Junio's worry that he will become the janitor for these patches. 
Please don't. As long as the patch series is in 'pu', it is MY 
responsibility to address issues and organize any additional patches on top 
of the series. Feel free to ignore all additional patches, and wait for an 
updated series from my end.
- Yes, there should be more tests verifying that there is no negative impact 
on git log and friends. Docs must be updated as well, where needed.

Unfortunately I don't have the time to work on this right now, but I'll do my best to get around to it as soon as possible (at least by the end of the coming weekend).

Again, thanks for your involvement. It is really appreciated.
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Junio C Hamano· Jan 21, 2010, 03:03 UTC · re: Johan Herland · lore

Re: git notes: notes

Johan Herland <johan@herland.net> writes:
> - Re. Junio's worry that he will become the janitor for these patches. 
>
> Please don't. As long as the patch series is in 'pu', it is MY 
> responsibility ...

I am not worried so much about what is not in 'next' yet. My worry right now is primarily about what to do with upcoming 1.7.0 and also the 1.6.6.X series, which already shipped with "format-patch" that injects notes in its output.

Junio C Hamano· Jan 21, 2010, 03:37 UTC · re: Junio C Hamano · lore

Re: git notes: notes

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> Actually I am of two minds regarding --pretty={short,medium} and the
> like.  The "how about this" patch may be the safest for people who are
> used to read "log --pretty=xxx" output with scripts, but it does look
> inconsistent and hard to explain to new people who do not even know that
> there were versions of git that does not know about notes.

Heh, it is not surprising that we have this bug, given that all of us just missed how bogus my "how about this" patch was ;-)

The check for "if (fmt_pretty)" only kicks in when that thing was read from the configuration; handling of command line --pretty and --pretty= options happen long after that, when we call setup_revisions().

So if we really wanted to say "If the user explicitly tells us to run under a particular --pretty mode, we don't show notes by default.", we would need a patch like this on top of it.

Another thing to note is that "work differently between no --pretty on the command line and an explicit --pretty=medium" is more work than "we always default to show notes if showing in the default verbosity".

This version considers a user supplied "format.pretty" configuration as "the user told us to use this specific format, and we won't show notes unless explicitly told". I personally don't care either way exactly because I don't use that configuration (nor teach others to use it), but in a sense the configuration is setting a personal "default", so I think it could be argued that we should instead show the notes by default in that case (i.e. remove "rev->pretty_given = 1" in the first hunk).

 builtin-log.c |    9 ++++++---
 revision.c    |    4 ++++
 revision.h    |    2 ++
 3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 3bc3919..1e05b0f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -39,10 +39,10 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
-	if (fmt_pretty)
+	if (fmt_pretty) {
 		get_commit_format(fmt_pretty, rev);
-	else
-		rev->show_notes = 1;
+		rev->pretty_given = 1;
+	}
 	rev->verbose_header = 1;
 	DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
 	rev->show_root_diff = default_show_root;
@@ -60,6 +60,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, "HEAD");
 
+	if (!rev->show_notes_given && !rev->pretty_given)
+		rev->show_notes = 1;
+
 	if (rev->diffopt.pickaxe || rev->diffopt.filter)
 		rev->always_show_header = 0;
 	if (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {
diff --git a/revision.c b/revision.c
index 7e00a6c..0de78fb 100644
--- a/revision.c
+++ b/revision.c
@@ -1161,14 +1161,18 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 	} else if (!strcmp(arg, "--pretty")) {
 		revs->verbose_header = 1;
+		revs->pretty_given = 1;
 		get_commit_format(arg+8, revs);
 	} else if (!prefixcmp(arg, "--pretty=") || !prefixcmp(arg, "--format=")) {
 		revs->verbose_header = 1;
+		revs->pretty_given = 1;
 		get_commit_format(arg+9, revs);
 	} else if (!strcmp(arg, "--show-notes")) {
 		revs->show_notes = 1;
+		revs->show_notes_given = 1;
 	} else if (!strcmp(arg, "--no-notes")) {
 		revs->show_notes = 0;
+		revs->show_notes_given = 1;
 	} else if (!strcmp(arg, "--oneline")) {
 		revs->verbose_header = 1;
 		get_commit_format("oneline", revs);
diff --git a/revision.h b/revision.h
index 4167c1e..a14deef 100644
--- a/revision.h
+++ b/revision.h
@@ -81,6 +81,8 @@ struct rev_info {
 	unsigned int	shown_one:1,
 			show_merge:1,
 			show_notes:1,
+			show_notes_given:1,
+			pretty_given:1,
 			abbrev_commit:1,
 			use_terminator:1,
 			missing_newline:1,
Michael J Gruber· Jan 21, 2010, 08:45 UTC · re: Junio C Hamano · lore

Re: git notes: notes

[Adding cc from elsewhere in this thread, for fairness.] Junio C Hamano venit, vidit, dixit 20.01.2010 22:08:

Show 31 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Junio C Hamano venit, vidit, dixit 20.01.2010 21:10:
>>> Joey Hess <joey@kitenet.net> writes:
>>>
>>>> Do you think it makes sense for even git log --format=format:%s to be
>>>> porcelain and potentially change when new features are used?
>>>
>>> If the series changed the meaning of "%s" format to mean "the subject of
>>> the commit and notes information", with or without documenting it, then it
>>> is just a bug we would like to fix.
>>
>> No, but outputting the note as part of the log is the standard. So for
>> example, when you do a format-patch | apply cycle, format-patch will
>> insert the note as part of the commit message, and apply will *store*
>> the note text (including Note:\n) as part of the commit message of the
>> new commit.
> 
> Thanks; that was the kind of breakage report I was looking for (and wished
> to have heard a lot earlier).  Personally I find it is unexcusable that
> format-patch defaults to giving notes.
> 
>> So, I would say the notes feature is not that well integrated right now,
> 
> No question about it.
> 
>> I'm not complaining, I actually have this on a maybe-to-do list, but the
>> way the series went kept me from investing time.
> 
> Hmm, that hints there is a failure in the review and merge process.  Care
> to explain how we could have done better please?

Well, I can only recall why it kept *me* from investing more time. I actually have very little free time available, I contribute to Git because it's fun, or I want a certain feature, or, admittedly, having a commit in a project like Git gives me a certain satisfaction.

The notes feature looked very promising to me, and when Dscho came up with it (or followed up on someone else's proposal, I don't remember) I invested time in testing and contributing (minor) fixes. Then the series took on a life on it's own, first "disappearing" (when I was wondering if anything is going forward), then reappearing with (not only my) commits squashed in (which took away the satisfaction part), then going through a lengthy technical discussion on fan-out schemes (which was necessary, but took away the fun part). [The last two parts may have been the other way round, which doesn't matter.]

When the first part of the series landed I began to look at it again and use it for the comments which go after the "--" part of a patch, only to find out that format-patch issue. I noticed the bad consequence on format-patch|apply only later. So I looked at the code, the log flags, and was about to code when the notes API changed again (on pu). So I decided to wait until the API is baked (that decision may have been influenced by my expectation that my patches would get squashed in again) and to fix that issue before 1.7.0.

Note that I have to match the project's necessities with time availability on my side - otherwise I would have written that patch when more of that series had landed. Now I reported it because it came up in some disguise (and didn't want anyone spend time needlessly fixing a side issue), and I'm not the one fixing it, but that's fine.

Besides the sociological aspect, I think you mentioned the main technical aspects:

* If you introduce a new features, write extensive tests covering
non-uses and mixed uses of the feature.
* Write redundancy tests, such as checking that format-patch|apply and
apply|format-patch both amount to the "identity" in the appropriate sense.

Right now we do very atomic testing, which does have its merits (for determining the cause of a breakage). But since many features and commands are not orthogonal, atomic testing does not test for side effects, and test repos are minimal. Trying to test for specific combinations makes you miss some combinations, especially combinations with future features. But testing for those identity operations (quasi-noops, or cycles) should ensure some consistency.

Maybe we should have a test repo which has all kinds of features turned on and used, and on which a set of those identities are tested. With every new feature, the repo as well as the set of supposed identities would need to be amended (maybe by cloning the repo, adding commits, and testing on an increasing set of repos). That would have caught at least the current issue immediately.

Cheers, Michael

Matthieu Moy· Jan 24, 2010, 14:20 UTC · re: Junio C Hamano · lore

Re: git notes: notes

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> No, but outputting the note as part of the log is the standard. So for
>> example, when you do a format-patch | apply cycle, format-patch will
>> insert the note as part of the commit message, and apply will *store*
>> the note text (including Note:\n) as part of the commit message of the
>> new commit.
>
> Thanks; that was the kind of breakage report I was looking for (and wished
> to have heard a lot earlier).  Personally I find it is unexcusable that
> format-patch defaults to giving notes.

OTOH, format-patch could give the notes, below the ---, where it will be ignored by apply. That would make notes handy to prepare a patch serie with additional messages: prepare everything within Git, and use git send-email to send it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Sverre Rabbelier· Jan 24, 2010, 14:27 UTC · re: Matthieu Moy · lore

Re: git notes: notes

Heya,

On Sun, Jan 24, 2010 at 15:20, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

> OTOH, format-patch could give the notes, below the ---, where it will
> be ignored by apply. That would make notes handy to prepare a patch
> serie with additional messages: prepare everything within Git, and use
> git send-email to send it.

I like that idea, but as an orthogonal feature, prepare notes in a special namespace, say refs/notes/format-patch/, and then teach format-patch a flag to honor that namespace.

-- 
Cheers,

Sverre Rabbelier
Johan Herland· Jan 21, 2010, 02:05 UTC · re: Joey Hess · lore

Re: git notes: notes

On Wednesday 20 January 2010, Joey Hess wrote:
Show 9 quoted lines
> Johan Herland wrote:
> > > PS, Has anyone thought about using notes to warn bisect away from
> > > commits that are known to be unbuildable or otherwise cause bisection
> > > trouble?
> >
> > No, I haven't thought of that specific use case. Great idea! :)
> 
> Only problem I see with doing it is it might be too easy to overwrite
> such a note with git notes edit -m

Well, you would have to run "git notes edit -m" with core.notesRef or $GIT_NOTES_REF set to the notes ref where bisect information is stored (e.g. "refs/notes/bisect").

In any case, I would not use "git notes" to maintain the bisect hints. Rather, I'd add subcommands to "git bisect" that would take care of maintaining the notes tree @ "refs/notes/bisect". Much more user-friendly than telling the user to write their own bisect-notes by hand.

> Did you consider having -m append a line to an existing note?

Hmm. Not really. The "git notes" porcelain was originally written by Dscho, and my builtin-ification of it (currently in 'pu') preserves the original semantics of "git notes edit -m". It might make sense to change the defaults; what do you think, Dscho?

Have fun! :)
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Johannes Schindelin· Jan 21, 2010, 03:59 UTC · re: Johan Herland · lore

Re: git notes: notes

Hi,
On Thu, 21 Jan 2010, Johan Herland wrote:
Show 8 quoted lines
> On Wednesday 20 January 2010, Joey Hess wrote:
>
> > Did you consider having -m append a line to an existing note?
> 
> Hmm. Not really. The "git notes" porcelain was originally written by 
> Dscho, and my builtin-ification of it (currently in 'pu') preserves the 
> original semantics of "git notes edit -m". It might make sense to change 
> the defaults; what do you think, Dscho?

I do not really care as long as there is a nice way to edit the complete note interactively.

Of course, I _do_ expect people to get confused just like they do with the current inconsistencies: "git commit -m" does not really append, but set the commit message, even if you amend a commit.

So maybe you want to use a different command line option for that.

Ciao, Dscho

Joey Hess· Jan 21, 2010, 04:05 UTC · re: Johannes Schindelin · lore

Re: git notes: notes

Johannes Schindelin wrote:
Show 8 quoted lines
> I do not really care as long as there is a nice way to edit the complete 
> note interactively.
> 
> Of course, I _do_ expect people to get confused just like they do with the 
> current inconsistencies: "git commit -m" does not really append, but set 
> the commit message, even if you amend a commit.
> 
> So maybe you want to use a different command line option for that.
Maybe: git notes add [-m|-F]
-- 
see shy jo
Johan Herland· Jan 27, 2010, 11:55 UTC · re: Joey Hess · lore

Re: git notes: notes

On Thursday 21 January 2010, Joey Hess wrote:
Show 11 quoted lines
> Johannes Schindelin wrote:
> > I do not really care as long as there is a nice way to edit the
> > complete note interactively.
> >
> > Of course, I _do_ expect people to get confused just like they do with
> > the current inconsistencies: "git commit -m" does not really append,
> > but set the commit message, even if you amend a commit.
> >
> > So maybe you want to use a different command line option for that.
> 
> Maybe: git notes add [-m|-F]

Thanks for the suggestion. I've added this to the new iteration of the jh/notes series.

...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
John Koleszar· Jan 25, 2010, 18:08 UTC · re: Johan Herland · lore

Re: git notes: notes

On Wed, 2010-01-20 at 21:05 -0500, Johan Herland wrote:
Show 8 quoted lines
> On Wednesday 20 January 2010, Joey Hess wrote:
> > Johan Herland wrote:
> > > > PS, Has anyone thought about using notes to warn bisect away from
> > > > commits that are known to be unbuildable or otherwise cause bisection
> > > > trouble?
> > >
> > > No, I haven't thought of that specific use case. Great idea! :)
> > 
[...]
Show 6 quoted lines
> 
> In any case, I would not use "git notes" to maintain the bisect hints. 
> Rather, I'd add subcommands to "git bisect" that would take care of 
> maintaining the notes tree @ "refs/notes/bisect". Much more user-friendly 
> than telling the user to write their own bisect-notes by hand.
> 

I haven't read up on notes more than enough to know its in the pipe, but I had a similar idea for using them to store bisect hints. I've been doing a lot of bisecting lately into a range that had a couple dormant bugs where I'm trying to bisect bug B but bug A prevents me from making a determination. Rather than skip what I know is an interesting commit, I cherry-pick the bugfix commit(s) A' and test that, then reset and continue bisecting.

Teaching bisect to consistently skip a commit, or to automatically squash in A' if we have A and not A', would be a desirable feature. I will have to read up some more on notes.

Christian Couder· Jan 27, 2010, 20:01 UTC · re: John Koleszar · lore

Re: git notes: notes

On lundi 25 janvier 2010, John Koleszar wrote:
Show 20 quoted lines
> On Wed, 2010-01-20 at 21:05 -0500, Johan Herland wrote:
> > On Wednesday 20 January 2010, Joey Hess wrote:
>
> > In any case, I would not use "git notes" to maintain the bisect hints.
> > Rather, I'd add subcommands to "git bisect" that would take care of
> > maintaining the notes tree @ "refs/notes/bisect". Much more
> > user-friendly than telling the user to write their own bisect-notes by
> > hand.
>
> I haven't read up on notes more than enough to know its in the pipe, but
> I had a similar idea for using them to store bisect hints. I've been
> doing a lot of bisecting lately into a range that had a couple dormant
> bugs where I'm trying to bisect bug B but bug A prevents me from making
> a determination. Rather than skip what I know is an interesting commit,
> I cherry-pick the bugfix commit(s) A' and test that, then reset and
> continue bisecting.
>
> Teaching bisect to consistently skip a commit, or to automatically
> squash in A' if we have A and not A', would be a desirable feature. I
> will have to read up some more on notes.
Perhaps you can read about "git replace" in my article:
http://www.kernel.org/pub/software/scm/git/docs/git-bisect-lk2009.html
and/or my related presentation:
http://www.linux-kongress.org/2009/slides/fighting_regressions_with_git_bisect_christian_couder.pdf

I think in the long run it's much better to use git replace rather than notes, especially as replace refs for bisecting could be in their own refs/replace/bisect namespace. I may take the time to implement that soon if you or other people are interested.

Regards, Christian.

← back to recent threads