[PATCH 1/2] t3301-notes: Test the creation of reflog entries

[PATCH 1/2] t3301-notes: Test the creation of reflog entries

[Michael J Gruber]

Test whether the notes code writes reflog entries. It intends to
(setting up the reflog messages) but currently does not.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t3301-notes.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 1d6cd45..5410a6d 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -65,6 +65,15 @@ test_expect_success 'create notes' '
 	test_must_fail git notes show HEAD^
 '
 
+cat >expect <<EOF
+d423f8c refs/notes/commits@{0}: notes: Notes added by 'git notes add'
+EOF
+
+test_expect_failure 'create reflog entry' '
+	git reflog show refs/notes/commits >output &&
+	test_cmp expect output
+'
+
 test_expect_success 'edit existing notes' '
 	MSG=b3 git notes edit &&
 	test ! -f .git/NOTES_EDITMSG &&
-- 
1.7.0.3.448.g82eeb

[PATCH 2/2] refs.c: Write reflogs for notes just like for branch heads

[Michael J Gruber]

The notes code intends to write reflog entries, but currently they are
not written because log_ref_write() checks for the refname path
explicitly.

Add refs/notes to the list of allowed paths so that notes references are
treated just like branch heads, i.e. according to core.logAllRefUpdates
and core.bare.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
This is actually inspired by Jeff's novel notes use. I think there are
use cases where a notes log makes sense (notes on commits) and those
where it does not (metadata/textconv). In both cases having a reflog is
useful. So, the next step is really to allow notes trees without
history, which also takes care of the pruning issue. I know how to do this,
I just have to decide about the configuration options.

 refs.c           |    1 +
 t/t3301-notes.sh |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/refs.c b/refs.c
index 0f24c8d..d3db15a 100644
--- a/refs.c
+++ b/refs.c
@@ -1276,6 +1276,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 	if (log_all_ref_updates &&
 	    (!prefixcmp(ref_name, "refs/heads/") ||
 	     !prefixcmp(ref_name, "refs/remotes/") ||
+	     !prefixcmp(ref_name, "refs/notes/") ||
 	     !strcmp(ref_name, "HEAD"))) {
 		if (safe_create_leading_directories(log_file) < 0)
 			return error("unable to create directory for %s",
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 5410a6d..b2e7b07 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -69,7 +69,7 @@ cat >expect <<EOF
 d423f8c refs/notes/commits@{0}: notes: Notes added by 'git notes add'
 EOF
 
-test_expect_failure 'create reflog entry' '
+test_expect_success 'create reflog entry' '
 	git reflog show refs/notes/commits >output &&
 	test_cmp expect output
 '
-- 
1.7.0.3.448.g82eeb

Re: [PATCH 2/2] refs.c: Write reflogs for notes just like for branch heads

[Johan Herland]

On Monday 29 March 2010, Michael J Gruber wrote:
> The notes code intends to write reflog entries, but currently they
> are not written because log_ref_write() checks for the refname path
> explicitly.
>
> Add refs/notes to the list of allowed paths so that notes references
> are treated just like branch heads, i.e. according to
> core.logAllRefUpdates and core.bare.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>

Both patches are

Acked-by: Johan Herland <johan@herland.net>

> ---
> This is actually inspired by Jeff's novel notes use. I think there
> are use cases where a notes log makes sense (notes on commits) and
> those where it does not (metadata/textconv). In both cases having a
> reflog is useful. So, the next step is really to allow notes trees
> without history, which also takes care of the pruning issue. I know
> how to do this, I just have to decide about the configuration
> options.

I noticed that Jeff's proof-of-concept wrote notes trees without making 
notes commits, and although it seemed like a bug at first, it does - as 
you say - provide a rather nice way to store notes trees without 
history.

Note that I haven't explicitly designed the notes feature with this in 
mind, so it's wise to add testcases for expected behaviour once we 
start use history-less notes trees.

Thinking about it, the notes code itself (notes.h/.c) only wants a notes 
_tree_ object, so will probably work fine with history-less notes 
trees. But builtin/notes.c with its public commit_notes() function may 
be another story...


...Johan


>  refs.c           |    1 +
>  t/t3301-notes.sh |    2 +-
>  2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 0f24c8d..d3db15a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1276,6 +1276,7 @@ static int log_ref_write(const char *ref_name,
> const unsigned char *old_sha1, if (log_all_ref_updates &&
>  	    (!prefixcmp(ref_name, "refs/heads/") ||
>  	     !prefixcmp(ref_name, "refs/remotes/") ||
> +	     !prefixcmp(ref_name, "refs/notes/") ||
>  	     !strcmp(ref_name, "HEAD"))) {
>  		if (safe_create_leading_directories(log_file) < 0)
>  			return error("unable to create directory for %s",
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 5410a6d..b2e7b07 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -69,7 +69,7 @@ cat >expect <<EOF
>  d423f8c refs/notes/commits@{0}: notes: Notes added by 'git notes
> add' EOF
>
> -test_expect_failure 'create reflog entry' '
> +test_expect_success 'create reflog entry' '
>  	git reflog show refs/notes/commits >output &&
>  	test_cmp expect output
>  '



-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/2] refs.c: Write reflogs for notes just like for branch heads

[Jeff King]

On Mon, Mar 29, 2010 at 04:25:22PM +0200, Johan Herland wrote:

> > This is actually inspired by Jeff's novel notes use. I think there
> > are use cases where a notes log makes sense (notes on commits) and
> > those where it does not (metadata/textconv). In both cases having a
> > reflog is useful. So, the next step is really to allow notes trees
> > without history, which also takes care of the pruning issue. I know
> > how to do this, I just have to decide about the configuration
> > options.
> 
> I noticed that Jeff's proof-of-concept wrote notes trees without making 
> notes commits, and although it seemed like a bug at first, it does - as 
> you say - provide a rather nice way to store notes trees without 
> history.

No, it was very much intentional.

However, I think the next iteration will wrap the tree in an actual
commit, but just keep each commit parentless. That will provide a nice
spot for metadata like the cache validity information.

I like the idea of having a reflog, just because you could use it to
salvage an old cache if you were playing around with your helper's
options (or debugging your helper :) ). The usual 90-day expiration
time is perhaps too long, though.

> Note that I haven't explicitly designed the notes feature with this in 
> mind, so it's wise to add testcases for expected behaviour once we 
> start use history-less notes trees.
> 
> Thinking about it, the notes code itself (notes.h/.c) only wants a notes 
> _tree_ object, so will probably work fine with history-less notes 
> trees. But builtin/notes.c with its public commit_notes() function may 
> be another story...

I was planning on using my own cache-specific helper instead of
commit_notes() anyway, so that shouldn't be a problem. By using a commit
wrapper, I don't think any of the display code should be confused (since
they need to handle the case of a root note commit anyway). Once I have
some example trees, I can poke at them with the existing notes code and
see how they behave (and how we _want_ them to behave, since I'm not
sure yet what sort of cache introspection, if any, would be useful).

-Peff

Re: [PATCH 2/2] refs.c: Write reflogs for notes just like for branch heads

[Johan Herland]

On Tuesday 30 March 2010, Jeff King wrote:
> On Mon, Mar 29, 2010 at 04:25:22PM +0200, Johan Herland wrote:
> > > This is actually inspired by Jeff's novel notes use. I think
> > > there are use cases where a notes log makes sense (notes on
> > > commits) and those where it does not (metadata/textconv). In both
> > > cases having a reflog is useful. So, the next step is really to
> > > allow notes trees without history, which also takes care of the
> > > pruning issue. I know how to do this, I just have to decide about
> > > the configuration options.
> >
> > I noticed that Jeff's proof-of-concept wrote notes trees without
> > making notes commits, and although it seemed like a bug at first,
> > it does - as you say - provide a rather nice way to store notes
> > trees without history.
>
> No, it was very much intentional.
>
> However, I think the next iteration will wrap the tree in an actual
> commit, but just keep each commit parentless. That will provide a
> nice spot for metadata like the cache validity information.

Agreed.

> I like the idea of having a reflog, just because you could use it to
> salvage an old cache if you were playing around with your helper's
> options (or debugging your helper :) ). The usual 90-day expiration
> time is perhaps too long, though.

Yes, 90 days as a default might be excessive, but you can always 
override it with a "git gc --prune=now"...

> > Note that I haven't explicitly designed the notes feature with this
> > in mind, so it's wise to add testcases for expected behaviour once
> > we start use history-less notes trees.
> >
> > Thinking about it, the notes code itself (notes.h/.c) only wants a
> > notes _tree_ object, so will probably work fine with history-less
> > notes trees. But builtin/notes.c with its public commit_notes()
> > function may be another story...
>
> I was planning on using my own cache-specific helper instead of
> commit_notes() anyway, so that shouldn't be a problem. By using a
> commit wrapper, I don't think any of the display code should be
> confused (since they need to handle the case of a root note commit
> anyway). Once I have some example trees, I can poke at them with the
> existing notes code and see how they behave (and how we _want_ them
> to behave, since I'm not sure yet what sort of cache introspection,
> if any, would be useful).

Looking forward to your patches. :)


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/2] refs.c: Write reflogs for notes just like for branch heads

[Jakub Narebski]

Johan Herland <johan@herland.net> writes:
> On Tuesday 30 March 2010, Jeff King wrote:

> > I like the idea of having a reflog, just because you could use it to
> > salvage an old cache if you were playing around with your helper's
> > options (or debugging your helper :) ). The usual 90-day expiration
> > time is perhaps too long, though.
> 
> Yes, 90 days as a default might be excessive, but you can always 
> override it with a "git gc --prune=now"...

You can always set different expire time for notes by using

  [gc "refs/notes"]
        reflogExpire = 7 # days, I suppose

Which is not documented (I have found it in RelNotes-1.6.0.txt).  
Oh well...
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 2/2] refs.c: Write reflogs for notes just like for branch heads

[Jeff King]

On Tue, Mar 30, 2010 at 12:18:16PM -0700, Jakub Narebski wrote:

> > > I like the idea of having a reflog, just because you could use it to
> > > salvage an old cache if you were playing around with your helper's
> > > options (or debugging your helper :) ). The usual 90-day expiration
> > > time is perhaps too long, though.
> > 
> > Yes, 90 days as a default might be excessive, but you can always 
> > override it with a "git gc --prune=now"...
> 
> You can always set different expire time for notes by using
> 
>   [gc "refs/notes"]
>         reflogExpire = 7 # days, I suppose
> 
> Which is not documented (I have found it in RelNotes-1.6.0.txt).  
> Oh well...

Thanks, I didn't know about that feature. I just posted my series
without dealing with the reflogs at all, but I think it may be sensible
to drop the default for "refs/notes/textconv" in a followup patch.

-Peff