* [PATCH 1/6] Bring notes.c template handling in line with commit.c.
2011-06-18 21:06 ` [PATCH 0/6] Small notes usability improvements Yann Dirson
@ 2011-06-18 21:06 ` Yann Dirson
2011-06-19 21:23 ` Johan Herland
2011-06-18 21:06 ` [PATCH 2/6] Factorize shortening of notes refname for display Yann Dirson
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Yann Dirson @ 2011-06-18 21:06 UTC (permalink / raw)
To: git; +Cc: Yann Dirson
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
builtin/notes.c | 30 +++++++++++++++---------------
1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437d..bd342ac 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -108,19 +108,19 @@ static int list_each_note(const unsigned char *object_sha1,
return 0;
}
-static void write_note_data(int fd, const unsigned char *sha1)
+static void write_note_data(FILE *fp, const unsigned char *sha1)
{
unsigned long size;
enum object_type type;
char *buf = read_sha1_file(sha1, &type, &size);
if (buf) {
if (size)
- write_or_die(fd, buf, size);
+ fwrite(buf, 1, size, fp);
free(buf);
}
}
-static void write_commented_object(int fd, const unsigned char *object)
+static void write_commented_object(FILE *fp, const unsigned char *object)
{
const char *show_args[5] =
{"show", "--stat", "--no-notes", sha1_to_hex(object), NULL};
@@ -144,11 +144,11 @@ static void write_commented_object(int fd, const unsigned char *object)
if (show_out == NULL)
die_errno(_("can't fdopen 'show' output fd"));
- /* Prepend "# " to each output line and write result to 'fd' */
+ /* Prepend "# " to each output line and write result to 'fp' */
while (strbuf_getline(&buf, show_out, '\n') != EOF) {
- write_or_die(fd, "# ", 2);
- write_or_die(fd, buf.buf, buf.len);
- write_or_die(fd, "\n", 1);
+ fwrite("# ", 1, 2, fp);
+ fwrite(buf.buf, 1, buf.len, fp);
+ fwrite("\n", 1, 1, fp);
}
strbuf_release(&buf);
if (fclose(show_out))
@@ -166,23 +166,23 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
char *path = NULL;
if (msg->use_editor || !msg->given) {
- int fd;
+ FILE *fp;
/* write the template message before editing: */
path = git_pathdup("NOTES_EDITMSG");
- fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
- if (fd < 0)
+ fp = fopen(path, "w");
+ if (fp == NULL)
die_errno(_("could not create file '%s'"), path);
if (msg->given)
- write_or_die(fd, msg->buf.buf, msg->buf.len);
+ fwrite(msg->buf.buf, 1, msg->buf.len, fp);
else if (prev && !append_only)
- write_note_data(fd, prev);
- write_or_die(fd, note_template, strlen(note_template));
+ write_note_data(fp, prev);
+ fwrite(note_template, 1, strlen(note_template), fp);
- write_commented_object(fd, object);
+ write_commented_object(fp, object);
- close(fd);
+ fclose(fp);
strbuf_reset(&(msg->buf));
if (launch_editor(path, &(msg->buf), NULL)) {
--
1.7.5.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.
2011-06-18 21:06 ` [PATCH 1/6] Bring notes.c template handling in line with commit.c Yann Dirson
@ 2011-06-19 21:23 ` Johan Herland
2011-06-19 22:50 ` Junio C Hamano
0 siblings, 1 reply; 27+ messages in thread
From: Johan Herland @ 2011-06-19 21:23 UTC (permalink / raw)
To: Yann Dirson; +Cc: git
On Saturday 18 June 2011, Yann Dirson wrote:
> Signed-off-by: Yann Dirson <ydirson@free.fr>
Please mention in the commit message that the commit merely replaces
write_or_die()/int fd with the corresponding stdio functionality, and that
there is no (intended) change in behavior. It was not apparent from your
commit message that you had not made any other changes.
Otherwise the patch looks OK.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.
2011-06-19 21:23 ` Johan Herland
@ 2011-06-19 22:50 ` Junio C Hamano
2011-06-20 7:41 ` Johan Herland
0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2011-06-19 22:50 UTC (permalink / raw)
To: Johan Herland; +Cc: Yann Dirson, git
Johan Herland <johan@herland.net> writes:
> On Saturday 18 June 2011, Yann Dirson wrote:
>> Signed-off-by: Yann Dirson <ydirson@free.fr>
>
> Please mention in the commit message that the commit merely replaces
> write_or_die()/int fd with the corresponding stdio functionality, and that
> there is no (intended) change in behavior. It was not apparent from your
> commit message that you had not made any other changes.
>
> Otherwise the patch looks OK.
I had an impression that you would lose a lot of error checking, unless
you are careful, if you go from write_or_die() to stdio.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.
2011-06-19 22:50 ` Junio C Hamano
@ 2011-06-20 7:41 ` Johan Herland
2011-06-20 18:48 ` Yann Dirson
0 siblings, 1 reply; 27+ messages in thread
From: Johan Herland @ 2011-06-20 7:41 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Yann Dirson
On Monday 20 June 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > On Saturday 18 June 2011, Yann Dirson wrote:
> >> Signed-off-by: Yann Dirson <ydirson@free.fr>
> >
> > Please mention in the commit message that the commit merely replaces
> > write_or_die()/int fd with the corresponding stdio functionality, and
> > that there is no (intended) change in behavior. It was not apparent
> > from your commit message that you had not made any other changes.
> >
> > Otherwise the patch looks OK.
>
> I had an impression that you would lose a lot of error checking, unless
> you are careful, if you go from write_or_die() to stdio.
Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one
needs to check the return value, and handle errors accordingly.
An alternative solution would be to drop this patch, and instead use
strbuf_addf() to get the format printing functionality needed in PATCH 3/6.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.
2011-06-20 7:41 ` Johan Herland
@ 2011-06-20 18:48 ` Yann Dirson
2011-06-21 19:39 ` Yann Dirson
0 siblings, 1 reply; 27+ messages in thread
From: Yann Dirson @ 2011-06-20 18:48 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Junio C Hamano
On Mon, Jun 20, 2011 at 09:41:54AM +0200, Johan Herland wrote:
> On Monday 20 June 2011, Junio C Hamano wrote:
> > Johan Herland <johan@herland.net> writes:
> > > On Saturday 18 June 2011, Yann Dirson wrote:
> > >> Signed-off-by: Yann Dirson <ydirson@free.fr>
> > >
> > > Please mention in the commit message that the commit merely replaces
> > > write_or_die()/int fd with the corresponding stdio functionality, and
> > > that there is no (intended) change in behavior. It was not apparent
> > > from your commit message that you had not made any other changes.
> > >
> > > Otherwise the patch looks OK.
> >
> > I had an impression that you would lose a lot of error checking, unless
> > you are careful, if you go from write_or_die() to stdio.
>
> Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one
> needs to check the return value, and handle errors accordingly.
It appears I based my code on buildin/commit.c from 1.7.4.1 - I just
did not realize that this part changed much in between with 098d0e0e.
I'll look into that.
> An alternative solution would be to drop this patch, and instead use
> strbuf_addf() to get the format printing functionality needed in PATCH 3/6.
I have thought about that, but that will make the i18n process for the
template much more awkward - and we probably don't want to reimplement
stdio formatting for strbuf.
--
Yann.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.
2011-06-20 18:48 ` Yann Dirson
@ 2011-06-21 19:39 ` Yann Dirson
0 siblings, 0 replies; 27+ messages in thread
From: Yann Dirson @ 2011-06-21 19:39 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Junio C Hamano
On Mon, Jun 20, 2011 at 08:48:42PM +0200, Yann Dirson wrote:
> On Mon, Jun 20, 2011 at 09:41:54AM +0200, Johan Herland wrote:
> > On Monday 20 June 2011, Junio C Hamano wrote:
> > > Johan Herland <johan@herland.net> writes:
> > > > On Saturday 18 June 2011, Yann Dirson wrote:
> > > >> Signed-off-by: Yann Dirson <ydirson@free.fr>
> > > >
> > > > Please mention in the commit message that the commit merely replaces
> > > > write_or_die()/int fd with the corresponding stdio functionality, and
> > > > that there is no (intended) change in behavior. It was not apparent
> > > > from your commit message that you had not made any other changes.
> > > >
> > > > Otherwise the patch looks OK.
> > >
> > > I had an impression that you would lose a lot of error checking, unless
> > > you are careful, if you go from write_or_die() to stdio.
> >
> > Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one
> > needs to check the return value, and handle errors accordingly.
>
> It appears I based my code on buildin/commit.c from 1.7.4.1 - I just
> did not realize that this part changed much in between with 098d0e0e.
> I'll look into that.
Hm. So now builtin/commit.c heavily relies on status_printf_*. Those
do not do much more return-checking on fprintf() than the previous
code - but at least they provide a single point where such
return-checking can be inserted, which is already better.
Now, those require a wt_status struct... but AFAICT, it only uses the
FILE* inside. This seems a bit annoying for the purpose of reusing
the #-prefixing and line-folding mechanism in builtin/notes.c. Would
replacing those funcs with FILE*-based ones - let's say,
status_printf_*_fp() - and wrappers with the current names in
wt-status.h be seen as a good idea ?
> > An alternative solution would be to drop this patch, and instead use
> > strbuf_addf() to get the format printing functionality needed in PATCH 3/6.
>
> I have thought about that, but that will make the i18n process for the
> template much more awkward - and we probably don't want to reimplement
> stdio formatting for strbuf.
... and for that matter, it looks like status_printf* provide us with
all that's needed.
--
Yann
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/6] Factorize shortening of notes refname for display.
2011-06-18 21:06 ` [PATCH 0/6] Small notes usability improvements Yann Dirson
2011-06-18 21:06 ` [PATCH 1/6] Bring notes.c template handling in line with commit.c Yann Dirson
@ 2011-06-18 21:06 ` Yann Dirson
2011-06-19 21:25 ` Johan Herland
2011-06-18 21:06 ` [PATCH 3/6] Include name of notes ref in template when creating/editing notes Yann Dirson
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Yann Dirson @ 2011-06-18 21:06 UTC (permalink / raw)
To: git; +Cc: Yann Dirson
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
notes.c | 24 ++++++++++++++++--------
notes.h | 7 +++++++
2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/notes.c b/notes.c
index f6ce848..1a5676a 100644
--- a/notes.c
+++ b/notes.c
@@ -1235,16 +1235,11 @@ void format_note(struct notes_tree *t, const unsigned char *object_sha1,
msglen--;
if (flags & NOTES_SHOW_HEADER) {
- const char *ref = t->ref;
- if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {
+ const char *ref = notes_ref_shortname(t->ref);
+ if (!ref)
strbuf_addstr(sb, "\nNotes:\n");
- } else {
- if (!prefixcmp(ref, "refs/"))
- ref += 5;
- if (!prefixcmp(ref, "notes/"))
- ref += 6;
+ else
strbuf_addf(sb, "\nNotes (%s):\n", ref);
- }
}
for (msg_p = msg; msg_p < msg + msglen; msg_p += linelen + 1) {
@@ -1296,3 +1291,16 @@ void expand_notes_ref(struct strbuf *sb)
else
strbuf_insert(sb, 0, "refs/notes/", 11);
}
+
+const char *notes_ref_shortname(const char *ref)
+{
+ if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF))
+ return NULL;
+ else {
+ if (!prefixcmp(ref, "refs/"))
+ ref += 5;
+ if (!prefixcmp(ref, "notes/"))
+ ref += 6;
+ return ref;
+ }
+}
diff --git a/notes.h b/notes.h
index c716694..d8ae29d 100644
--- a/notes.h
+++ b/notes.h
@@ -64,6 +64,13 @@ extern struct notes_tree {
const char *default_notes_ref(void);
/*
+ * Return a short name for a notes ref, suitable for display to the user.
+ *
+ * No copy is done, the return value is a pointer into the original string.
+ */
+const char *notes_ref_shortname(const char *ref);
+
+/*
* Flags controlling behaviour of notes tree initialization
*
* Default behaviour is to initialize the notes tree from the tree object
--
1.7.5.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/6] Factorize shortening of notes refname for display.
2011-06-18 21:06 ` [PATCH 2/6] Factorize shortening of notes refname for display Yann Dirson
@ 2011-06-19 21:25 ` Johan Herland
2011-06-19 22:51 ` Junio C Hamano
0 siblings, 1 reply; 27+ messages in thread
From: Johan Herland @ 2011-06-19 21:25 UTC (permalink / raw)
To: Yann Dirson; +Cc: git
On Saturday 18 June 2011, Yann Dirson wrote:
> Signed-off-by: Yann Dirson <ydirson@free.fr>
> ---
> notes.c | 24 ++++++++++++++++--------
> notes.h | 7 +++++++
> 2 files changed, 23 insertions(+), 8 deletions(-)
>
> [...]
>
> /*
> + * Return a short name for a notes ref, suitable for display to the user.
> + *
> + * No copy is done, the return value is a pointer into the original string.
> + */
> +const char *notes_ref_shortname(const char *ref);
> +
> +/*
Please include in the documentation what a NULL return means.
Otherwise the patch looks OK.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] Factorize shortening of notes refname for display.
2011-06-19 21:25 ` Johan Herland
@ 2011-06-19 22:51 ` Junio C Hamano
2011-06-20 18:49 ` Yann Dirson
0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2011-06-19 22:51 UTC (permalink / raw)
To: Johan Herland; +Cc: Yann Dirson, git
Johan Herland <johan@herland.net> writes:
> On Saturday 18 June 2011, Yann Dirson wrote:
>> Signed-off-by: Yann Dirson <ydirson@free.fr>
>> ---
>> notes.c | 24 ++++++++++++++++--------
>> notes.h | 7 +++++++
>> 2 files changed, 23 insertions(+), 8 deletions(-)
>>
>> [...]
>>
>> /*
>> + * Return a short name for a notes ref, suitable for display to the user.
>> + *
>> + * No copy is done, the return value is a pointer into the original string.
>> + */
>> +const char *notes_ref_shortname(const char *ref);
>> +
>> +/*
>
> Please include in the documentation what a NULL return means.
>
> Otherwise the patch looks OK.
It may be just me, but every time somebody says Factorize, I find myself
looking for math textbook from middle school days, 12 = 2 * 2 * 3, etc.
Could we say Refactor instead pretty please?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] Factorize shortening of notes refname for display.
2011-06-19 22:51 ` Junio C Hamano
@ 2011-06-20 18:49 ` Yann Dirson
0 siblings, 0 replies; 27+ messages in thread
From: Yann Dirson @ 2011-06-20 18:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johan Herland, git
On Sun, Jun 19, 2011 at 03:51:34PM -0700, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
>
> > On Saturday 18 June 2011, Yann Dirson wrote:
> >> Signed-off-by: Yann Dirson <ydirson@free.fr>
> >> ---
> >> notes.c | 24 ++++++++++++++++--------
> >> notes.h | 7 +++++++
> >> 2 files changed, 23 insertions(+), 8 deletions(-)
> >>
> >> [...]
> >>
> >> /*
> >> + * Return a short name for a notes ref, suitable for display to the user.
> >> + *
> >> + * No copy is done, the return value is a pointer into the original string.
> >> + */
> >> +const char *notes_ref_shortname(const char *ref);
> >> +
> >> +/*
> >
> > Please include in the documentation what a NULL return means.
> >
> > Otherwise the patch looks OK.
>
> It may be just me, but every time somebody says Factorize, I find myself
> looking for math textbook from middle school days, 12 = 2 * 2 * 3, etc.
>
> Could we say Refactor instead pretty please?
Well, factorizing is just one kind of code refactoring - but OK, I'll
do that ;)
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/6] Include name of notes ref in template when creating/editing notes.
2011-06-18 21:06 ` [PATCH 0/6] Small notes usability improvements Yann Dirson
2011-06-18 21:06 ` [PATCH 1/6] Bring notes.c template handling in line with commit.c Yann Dirson
2011-06-18 21:06 ` [PATCH 2/6] Factorize shortening of notes refname for display Yann Dirson
@ 2011-06-18 21:06 ` Yann Dirson
2011-06-18 21:06 ` [PATCH 4/6] Allow "git notes merge" to use refs/remote-notes/ as a source Yann Dirson
` (3 subsequent siblings)
6 siblings, 0 replies; 27+ messages in thread
From: Yann Dirson @ 2011-06-18 21:06 UTC (permalink / raw)
To: git; +Cc: Yann Dirson
This will still show for the default "commits" notes:
# Write/edit notes for the following object:
For other notes refs it will show:
# Write/edit "foo" notes for the following object:
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
builtin/notes.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index bd342ac..ae89d38 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -91,7 +91,7 @@ static const char * const git_notes_get_ref_usage[] = {
static const char note_template[] =
"\n"
"#\n"
- "# Write/edit the notes for the following object:\n"
+ "# Write/edit %s%s%snotes for the following object:\n"
"#\n";
struct msg_arg {
@@ -167,6 +167,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
if (msg->use_editor || !msg->given) {
FILE *fp;
+ const char *ref = notes_ref_shortname(default_notes_tree.ref);
/* write the template message before editing: */
path = git_pathdup("NOTES_EDITMSG");
@@ -178,7 +179,10 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
fwrite(msg->buf.buf, 1, msg->buf.len, fp);
else if (prev && !append_only)
write_note_data(fp, prev);
- fwrite(note_template, 1, strlen(note_template), fp);
+ if (!ref)
+ fprintf(fp, note_template, "", "", "");
+ else
+ fprintf(fp, note_template, "\"", ref, "\" ");
write_commented_object(fp, object);
--
1.7.5.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 4/6] Allow "git notes merge" to use refs/remote-notes/ as a source.
2011-06-18 21:06 ` [PATCH 0/6] Small notes usability improvements Yann Dirson
` (2 preceding siblings ...)
2011-06-18 21:06 ` [PATCH 3/6] Include name of notes ref in template when creating/editing notes Yann Dirson
@ 2011-06-18 21:06 ` Yann Dirson
2011-06-19 21:45 ` Johan Herland
2011-06-18 21:06 ` [PATCH 5/6] Assume a note ref starting with refs must not be prepended refs/notes/ Yann Dirson
` (2 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Yann Dirson @ 2011-06-18 21:06 UTC (permalink / raw)
To: git; +Cc: Yann Dirson
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
Documentation/git-notes.txt | 5 +++++
builtin/notes.c | 4 ++--
notes.c | 5 +++--
notes.h | 2 +-
revision.c | 2 +-
5 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 6a187f2..7ce8a24 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -104,6 +104,11 @@ and instructs the user to manually resolve the conflicts there.
When done, the user can either finalize the merge with
'git notes merge --commit', or abort the merge with
'git notes merge --abort'.
++
+In addition to `refs/notes/`, the remote notes ref is accepted
+from the `refs/remote-notes/` namespace. This is intended to
+provide notes with support for a workflow similar to the one used
+for heads references.
remove::
Remove the notes for given objects (defaults to HEAD). When
diff --git a/builtin/notes.c b/builtin/notes.c
index ae89d38..6bff44f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -905,7 +905,7 @@ static int merge(int argc, const char **argv, const char *prefix)
o.local_ref = default_notes_ref();
strbuf_addstr(&remote_ref, argv[0]);
- expand_notes_ref(&remote_ref);
+ expand_notes_ref(&remote_ref, 1);
o.remote_ref = remote_ref.buf;
if (strategy) {
@@ -1075,7 +1075,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
if (override_notes_ref) {
struct strbuf sb = STRBUF_INIT;
strbuf_addstr(&sb, override_notes_ref);
- expand_notes_ref(&sb);
+ expand_notes_ref(&sb, 0);
setenv("GIT_NOTES_REF", sb.buf, 1);
strbuf_release(&sb);
}
diff --git a/notes.c b/notes.c
index 1a5676a..12afc02 100644
--- a/notes.c
+++ b/notes.c
@@ -1282,9 +1282,10 @@ int copy_note(struct notes_tree *t,
return 0;
}
-void expand_notes_ref(struct strbuf *sb)
+void expand_notes_ref(struct strbuf *sb, int allow_remotes)
{
- if (!prefixcmp(sb->buf, "refs/notes/"))
+ if (!prefixcmp(sb->buf, "refs/notes/") ||
+ (allow_remotes && !prefixcmp(sb->buf, "refs/remote-notes/")))
return; /* we're happy */
else if (!prefixcmp(sb->buf, "notes/"))
strbuf_insert(sb, 0, "refs/", 5);
diff --git a/notes.h b/notes.h
index d8ae29d..80219ec 100644
--- a/notes.h
+++ b/notes.h
@@ -317,6 +317,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
const char *globs);
/* Expand inplace a note ref like "foo" or "notes/foo" into "refs/notes/foo" */
-void expand_notes_ref(struct strbuf *sb);
+void expand_notes_ref(struct strbuf *sb, int allow_remotes);
#endif
diff --git a/revision.c b/revision.c
index c46cfaa..b482314 100644
--- a/revision.c
+++ b/revision.c
@@ -1393,7 +1393,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
}
else
strbuf_addstr(&buf, arg+8);
- expand_notes_ref(&buf);
+ expand_notes_ref(&buf, 1);
string_list_append(&revs->notes_opt.extra_notes_refs,
strbuf_detach(&buf, NULL));
} else if (!strcmp(arg, "--no-notes")) {
--
1.7.5.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 4/6] Allow "git notes merge" to use refs/remote-notes/ as a source.
2011-06-18 21:06 ` [PATCH 4/6] Allow "git notes merge" to use refs/remote-notes/ as a source Yann Dirson
@ 2011-06-19 21:45 ` Johan Herland
0 siblings, 0 replies; 27+ messages in thread
From: Johan Herland @ 2011-06-19 21:45 UTC (permalink / raw)
To: Yann Dirson; +Cc: git
On Saturday 18 June 2011, Yann Dirson wrote:
> Signed-off-by: Yann Dirson <ydirson@free.fr>
> ---
> Documentation/git-notes.txt | 5 +++++
> builtin/notes.c | 4 ++--
> notes.c | 5 +++--
> notes.h | 2 +-
> revision.c | 2 +-
> 5 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 6a187f2..7ce8a24 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -104,6 +104,11 @@ and instructs the user to manually resolve the
> conflicts there. When done, the user can either finalize the merge with
> 'git notes merge --commit', or abort the merge with
> 'git notes merge --abort'.
> ++
> +In addition to `refs/notes/`, the remote notes ref is accepted
> +from the `refs/remote-notes/` namespace. This is intended to
> +provide notes with support for a workflow similar to the one used
> +for heads references.
I would rephrase this as:
In addition to `refs/notes/*`, the remote notes ref can also be
from within `refs/remote-notes/*`. This allows the user to set up
fetch refspecs that transfers notes refs from a remote repo into
`refs/remote-notes/*`, and then merge those remote notes refs into
the corresponding local notes refs.
Also, AFAICS you're adding the possibility to read notes from refs/remote-
notes/*, but not WRITE to those notes using "git notes" (obviously, "git
fetch" and other tools can be used to manipulate them). Please add some
selftests verifying that "git notes" is still unable to manipulate notes in
refs/remote-notes/*.
Otherwise the patch looks good to me.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/6] Assume a note ref starting with refs must not be prepended refs/notes/.
2011-06-18 21:06 ` [PATCH 0/6] Small notes usability improvements Yann Dirson
` (3 preceding siblings ...)
2011-06-18 21:06 ` [PATCH 4/6] Allow "git notes merge" to use refs/remote-notes/ as a source Yann Dirson
@ 2011-06-18 21:06 ` Yann Dirson
2011-06-18 21:06 ` [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref Yann Dirson
2011-06-19 22:06 ` [PATCH 0/6] Small notes usability improvements Johan Herland
6 siblings, 0 replies; 27+ messages in thread
From: Yann Dirson @ 2011-06-18 21:06 UTC (permalink / raw)
To: git; +Cc: Yann Dirson
This caused strange behaviour when "git notes" was asked to manipulate
refs/<anything> outside of refs/notes/: it was attempting to use
refs/notes/refs/<anything>.
Dying early this way should avoid the need to check for the
refs/notes/ prefix in several places.
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
notes.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/notes.c b/notes.c
index 12afc02..c6a82da 100644
--- a/notes.c
+++ b/notes.c
@@ -1289,6 +1289,8 @@ void expand_notes_ref(struct strbuf *sb, int allow_remotes)
return; /* we're happy */
else if (!prefixcmp(sb->buf, "notes/"))
strbuf_insert(sb, 0, "refs/", 5);
+ else if (!prefixcmp(sb->buf, "refs/"))
+ die(_("Not a notes reference: %s"), sb->buf);
else
strbuf_insert(sb, 0, "refs/notes/", 11);
}
--
1.7.5.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.
2011-06-18 21:06 ` [PATCH 0/6] Small notes usability improvements Yann Dirson
` (4 preceding siblings ...)
2011-06-18 21:06 ` [PATCH 5/6] Assume a note ref starting with refs must not be prepended refs/notes/ Yann Dirson
@ 2011-06-18 21:06 ` Yann Dirson
2011-06-19 22:03 ` Johan Herland
2011-06-19 22:06 ` [PATCH 0/6] Small notes usability improvements Johan Herland
6 siblings, 1 reply; 27+ messages in thread
From: Yann Dirson @ 2011-06-18 21:06 UTC (permalink / raw)
To: git; +Cc: Yann Dirson
This causes the "merge empty notes ref (z => y)" test in t3308-notes-merge.sh
to fail - obviously, it is removing the functionnality that is tested for.
Is there any real use for this ? It just seems so different from
"git merge", which errors out in the similar situation:
$ git merge foo
fatal: 'foo' does not point to a commit
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
builtin/notes.c | 3 +++
t/t3308-notes-merge.sh | 6 ------
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 6bff44f..058b14d 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -908,6 +908,9 @@ static int merge(int argc, const char **argv, const char *prefix)
expand_notes_ref(&remote_ref, 1);
o.remote_ref = remote_ref.buf;
+ if (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))
+ die("'%s' does not point to a commit", o.remote_ref);
+
if (strategy) {
if (!strcmp(strategy, "manual"))
o.strategy = NOTES_MERGE_RESOLVE_MANUAL;
diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 24d82b4..2dcc1db 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -104,12 +104,6 @@ test_expect_success 'merge notes into empty notes ref (x => y)' '
test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)"
'
-test_expect_success 'merge empty notes ref (z => y)' '
- git notes merge z &&
- # y should not change (still == x)
- test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)"
-'
-
test_expect_success 'change notes on other notes ref (y)' '
# Not touching notes to 1st commit
git notes remove 2nd &&
--
1.7.5.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.
2011-06-18 21:06 ` [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref Yann Dirson
@ 2011-06-19 22:03 ` Johan Herland
2011-06-20 7:16 ` Jeff King
0 siblings, 1 reply; 27+ messages in thread
From: Johan Herland @ 2011-06-19 22:03 UTC (permalink / raw)
To: Yann Dirson; +Cc: git, Jeff King
On Saturday 18 June 2011, Yann Dirson wrote:
> This causes the "merge empty notes ref (z => y)" test in
> t3308-notes-merge.sh to fail - obviously, it is removing the
> functionnality that is tested for.
>
> Is there any real use for this ? It just seems so different from
> "git merge", which errors out in the similar situation:
>
> $ git merge foo
> fatal: 'foo' does not point to a commit
I understand your reasoning, and I don't have a problem with changing this
behavior to be in line with "git merge".
> Signed-off-by: Yann Dirson <ydirson@free.fr>
> ---
> builtin/notes.c | 3 +++
> t/t3308-notes-merge.sh | 6 ------
> 2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 6bff44f..058b14d 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -908,6 +908,9 @@ static int merge(int argc, const char **argv, const
> char *prefix) expand_notes_ref(&remote_ref, 1);
> o.remote_ref = remote_ref.buf;
>
> + if (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))
> + die("'%s' does not point to a commit", o.remote_ref);
Hmm. I'm not sure requiring the remote ref to always point to a _commit_ is
the right solution here. In previous discussions on the notes topic, some
people (Peff?) expressed a need/interest for history-less notes refs (i.e. a
notes tree where we don't keep track of its development, but only refer to
the latest/current version). Obviously, there are two ways to implement
history-less notes refs: (a) making the notes ref point to a notes commit
without any parents (i.e. each notes commit is a root commit), or (b) making
the notes ref point directly at the notes _tree_ object (i.e. no commit
object at all).
I can't remember off the top of my head whether our earlier discussions on
this topic resulted in us excluding support for option (b), but if we
didn't, it should be possible to merge notes refs where one or both refs
point directly at a tree object, and your above line would break this.
> [...]
>
> diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
> index 24d82b4..2dcc1db 100755
> --- a/t/t3308-notes-merge.sh
> +++ b/t/t3308-notes-merge.sh
> @@ -104,12 +104,6 @@ test_expect_success 'merge notes into empty notes
> ref (x => y)' ' test "$(git rev-parse refs/notes/x)" = "$(git rev-parse
> refs/notes/y)" '
>
> -test_expect_success 'merge empty notes ref (z => y)' '
> - git notes merge z &&
> - # y should not change (still == x)
> - test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)"
> -'
Instead of removing the test, please change it into verifying the _new_
expected behavior (that we fail with an appropriate error message when asked
to merge a non-existent notes ref).
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.
2011-06-19 22:03 ` Johan Herland
@ 2011-06-20 7:16 ` Jeff King
2011-06-20 7:29 ` Johan Herland
0 siblings, 1 reply; 27+ messages in thread
From: Jeff King @ 2011-06-20 7:16 UTC (permalink / raw)
To: Johan Herland; +Cc: Yann Dirson, git
On Mon, Jun 20, 2011 at 12:03:46AM +0200, Johan Herland wrote:
> > + if (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))
> > + die("'%s' does not point to a commit", o.remote_ref);
>
> Hmm. I'm not sure requiring the remote ref to always point to a _commit_ is
> the right solution here. In previous discussions on the notes topic, some
> people (Peff?) expressed a need/interest for history-less notes refs (i.e. a
> notes tree where we don't keep track of its development, but only refer to
> the latest/current version). Obviously, there are two ways to implement
> history-less notes refs: (a) making the notes ref point to a notes commit
> without any parents (i.e. each notes commit is a root commit), or (b) making
> the notes ref point directly at the notes _tree_ object (i.e. no commit
> object at all).
>
> I can't remember off the top of my head whether our earlier discussions on
> this topic resulted in us excluding support for option (b), but if we
> didn't, it should be possible to merge notes refs where one or both refs
> point directly at a tree object, and your above line would break this.
The notes-cache.[ch] implementation uses history-less notes for textconv
caching. Since it's just a cache, we don't care about history or
merging. And keeping a history would just mean useless old versions of
the cache are kept longer than necessary.
I ended up using a commit with no parents to store the cache. I don't
recall offhand whether there were any complications with using a raw
tree, but I realized that I needed some place to put extra metadata like
the cache validity. Wrapping the tree object in a commit provided that
place.
I don't think there is any real reason for somebody to need a bare tree
of notes. There is a certain elegance that refs can point directly to
trees in git, but the overhead of a single commit object to wrap it is
just not a big deal[1].
I didn't test, but I doubt that "git merge" will handle bare trees; this
would provide analagous behavior for notes-merging. But maybe I'm
wrong.
-Peff
[1] The only other time I recall seeing a bare tree is linux-2.6's
v2.6.11 tag. And even there it is wrapped by a tag object, so that Linus
could include metadata (a comment and a GPG signature). There's really
no reason that couldn't have had a commit, except that doing it as a
tree shows off how cool git is. :)
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.
2011-06-20 7:16 ` Jeff King
@ 2011-06-20 7:29 ` Johan Herland
0 siblings, 0 replies; 27+ messages in thread
From: Johan Herland @ 2011-06-20 7:29 UTC (permalink / raw)
To: Jeff King; +Cc: Yann Dirson, git
On Monday 20 June 2011, Jeff King wrote:
> On Mon, Jun 20, 2011 at 12:03:46AM +0200, Johan Herland wrote:
> > > + if (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))
> > > + die("'%s' does not point to a commit", o.remote_ref);
> >
> > Hmm. I'm not sure requiring the remote ref to always point to a
> > _commit_ is the right solution here. In previous discussions on the
> > notes topic, some people (Peff?) expressed a need/interest for
> > history-less notes refs (i.e. a notes tree where we don't keep track
> > of its development, but only refer to the latest/current version).
> > Obviously, there are two ways to implement history-less notes refs:
> > (a) making the notes ref point to a notes commit without any parents
> > (i.e. each notes commit is a root commit), or (b) making the notes ref
> > point directly at the notes _tree_ object (i.e. no commit object at
> > all).
> >
> > I can't remember off the top of my head whether our earlier discussions
> > on this topic resulted in us excluding support for option (b), but if
> > we didn't, it should be possible to merge notes refs where one or both
> > refs point directly at a tree object, and your above line would break
> > this.
>
> [...]
>
> I don't think there is any real reason for somebody to need a bare tree
> of notes. There is a certain elegance that refs can point directly to
> trees in git, but the overhead of a single commit object to wrap it is
> just not a big deal.
>
> I didn't test, but I doubt that "git merge" will handle bare trees; this
> would provide analagous behavior for notes-merging. But maybe I'm
> wrong.
You're not wrong. "git merge" when trying to merge a tree object:
$ git merge ee314a3
error: ee314a3: expected commit type, but the object dereferences to tree type
fatal: 'ee314a3' does not point to a commit
So I guess there's no reason to allow notes trees with no commit object.
Yann: Please disregard my complaint on the above two lines.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/6] Small notes usability improvements
2011-06-18 21:06 ` [PATCH 0/6] Small notes usability improvements Yann Dirson
` (5 preceding siblings ...)
2011-06-18 21:06 ` [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref Yann Dirson
@ 2011-06-19 22:06 ` Johan Herland
6 siblings, 0 replies; 27+ messages in thread
From: Johan Herland @ 2011-06-19 22:06 UTC (permalink / raw)
To: Yann Dirson; +Cc: git
On Saturday 18 June 2011, Yann Dirson wrote:
> Patches 1 and 2 are just preparing things for patch 3.
>
> Patch 4 is a (hopefully) temporary measure, to be able to implement a
> real-life until notes workflow without having to wait for
> refs/remotes/ to get its due restructuring,
>
> Patch 5 addresses the anomaly reported earlier this week.
>
> Patch 6 is a proposal to make "notes merge" more similar to "merge"
This series looks good to me, unless otherwise noted in my replies to
individual patches.
Thanks for your work!
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 27+ messages in thread