Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes
Hi Peff, On 2015-08-24 16:43, Jeff King wrote: On Mon, Aug 24, 2015 at 12:23:52PM +0200, Johannes Schindelin wrote: You're right. I think the best approach for now is to error out when `--show-notes` is passed to rev-list. Do you agree? Yes (I imagine you didn't yet read my follow-up patch when you wrote this). :) Yep, and I forgot to reply when I saw it. Thanks for continuing on that front! Ciao, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes
On Mon, Aug 24, 2015 at 12:23:52PM +0200, Johannes Schindelin wrote: You're right. I think the best approach for now is to error out when `--show-notes` is passed to rev-list. Do you agree? Yes (I imagine you didn't yet read my follow-up patch when you wrote this). :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes
Hi Peff, On 2015-08-23 19:43, Jeff King wrote: On Sat, Aug 22, 2015 at 05:14:39PM +0200, Johannes Schindelin wrote: The `format_display_notes()` function clearly assumes that the data structure holding the notes has been initialized already, i.e. that the `display_notes_trees` variable is no longer `NULL`. However, when there are no notes whatsoever, this variable is still `NULL`, even after initialization. So let's be graceful and just return if that data structure is `NULL`. Hrm. This is supposed to be made non-NULL by calling init_display_notes. The git-log code does this properly, and can show notes. The rev-list code does not, and hits this assert. But that also means that it cannot actually _show_ notes, and your patch is papering over the problem. Good point... it looks like [patch] is not enough to convince the pretty-printer to actually show notes (I suspect we need something like the pretty_ctx-notes_message setup that is in show_log()). I don't know how deeply anybody _cares_ about showing notes via rev-list. It has clearly never worked. But rather than silently accepting --show-notes (and not showing any notes!), perhaps we should tell the user that it does not work. Likewise, the %N --format specifier never expands in rev-list, and should probably be removed from the rev-list documentation. Hmpf. I really did not want to uncover yet another rabbit hole... Reported in https://github.com/msysgit/git/issues/363. After reading your subject, I wondered why git rev-list --show-notes HEAD did not crash for me (whether or not I had notes). But the key element from that issue is the addition of --grep, which is what triggers us to actually look at the notes (since rev-list otherwise does not support displaying them). And that _does_ work with my patch above. So perhaps that is useful, though again, it has never worked in the past (and with your patch, it would silently return no results, whether the grep matched or not). Of course it's a terrible interface to make --show-notes --grep grep the notes, but not actually _show_ them. :-/ It is. So I'm not really in favor of this approach, but if we do go that route, the comment above the declaration of format_display_notes needs to be updated. You're right. I think the best approach for now is to error out when `--show-notes` is passed to rev-list. Do you agree? Ciao, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes
On Sat, Aug 22, 2015 at 05:14:39PM +0200, Johannes Schindelin wrote:
The `format_display_notes()` function clearly assumes that the data
structure holding the notes has been initialized already, i.e. that the
`display_notes_trees` variable is no longer `NULL`.
However, when there are no notes whatsoever, this variable is still
`NULL`, even after initialization.
So let's be graceful and just return if that data structure is `NULL`.
Hrm. This is supposed to be made non-NULL by calling init_display_notes.
The git-log code does this properly, and can show notes. The
rev-list code does not, and hits this assert. But that also means that
it cannot actually _show_ notes, and your patch is papering over the
problem.
I would think we would need something more like this:
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff84a82..fc73a6f 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -279,6 +279,7 @@ int cmd_rev_list(int argc, const char **argv, const char
*prefix)
int bisect_show_vars = 0;
int bisect_find_all = 0;
int use_bitmap_index = 0;
+ struct userformat_want w;
git_config(git_default_config, NULL);
init_revisions(revs, prefix);
@@ -349,6 +350,14 @@ int cmd_rev_list(int argc, const char **argv, const char
*prefix)
revs.diff)
usage(rev_list_usage);
+ memset(w, 0, sizeof(w));
+ userformat_find_requirements(NULL, w);
+
+ if (!revs.show_notes_given w.notes)
+ revs.show_notes = 1;
+ if (revs.show_notes)
+ init_display_notes(revs.notes_opt);
+
save_commit_buffer = (revs.verbose_header ||
revs.grep_filter.pattern_list ||
revs.grep_filter.header_list);
but it looks like that is not enough to convince the pretty-printer to
actually show notes (I suspect we need something like the
pretty_ctx-notes_message setup that is in show_log()).
I don't know how deeply anybody _cares_ about showing notes via
rev-list. It has clearly never worked. But rather than silently
accepting --show-notes (and not showing any notes!), perhaps we should
tell the user that it does not work. Likewise, the %N --format
specifier never expands in rev-list, and should probably be removed from
the rev-list documentation.
Although...
Reported in https://github.com/msysgit/git/issues/363.
After reading your subject, I wondered why git rev-list --show-notes
HEAD did not crash for me (whether or not I had notes). But the key
element from that issue is the addition of --grep, which is what
triggers us to actually look at the notes (since rev-list otherwise does
not support displaying them). And that _does_ work with my patch above.
So perhaps that is useful, though again, it has never worked in the
past (and with your patch, it would silently return no results, whether
the grep matched or not).
Of course it's a terrible interface to make --show-notes --grep grep
the notes, but not actually _show_ them. :-/
diff --git a/notes.c b/notes.c
index df08209..24a335a 100644
--- a/notes.c
+++ b/notes.c
@@ -1266,7 +1266,10 @@ void format_display_notes(const unsigned char
*object_sha1,
struct strbuf *sb, const char *output_encoding, int
raw)
{
int i;
- assert(display_notes_trees);
+
+ if (!display_notes_trees)
+ return;
+
So I'm not really in favor of this approach, but if we do go that route,
the comment above the declaration of format_display_notes needs to be
updated.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes
On Sat, Aug 22, 2015 at 5:14 PM, Johannes Schindelin johannes.schinde...@gmx.de wrote: The `format_display_notes()` function clearly assumes that the data structure holding the notes has been initialized already, i.e. that the `display_notes_trees` variable is no longer `NULL`. However, when there are no notes whatsoever, this variable is still `NULL`, even after initialization. So let's be graceful and just return if that data structure is `NULL`. Reported in https://github.com/msysgit/git/issues/363. Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de Acked-by: Johan Herland jo...@herland.net -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
