threads / rfc / 49253

[RFC] revision: Don't let ^<rev> cancel out the default <rev>

Subject: [RFC] revision: Don't let ^<rev> cancel out the default <rev>

## tl;dr

3 messages between Aug 29, 2018 and Aug 29, 2018.

replies: 2people: 3as markdown or json

Andreas Gruenbacher· Aug 29, 2018, 20:05 UTC · lore

Some commands like 'log' default to HEAD if no other revisions are specified on the command line or otherwise. Unfortunately, excludes (^<rev>) cancel out that default, so when a command only excludes revisions (e.g., 'git log ^origin/master'), the command will not produce any result.

If all the specified revisions are excludes, it seems more useful to stick with the default revision instead.

This makes writing wrappers that exclude certain revisions much easier: for example, a simple alias l='git log ^origin/master' will show the revisions between origin/master and HEAD by default, and 'l foo' will show the revisions between origin/master and foo, as you would usually expect.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 revision.c     | 18 ++++++++++++++----
 t/t4202-log.sh |  6 ++++++
 2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/revision.c b/revision.c
index de4dce600..c2c51bd5d 100644
--- a/revision.c
+++ b/revision.c
@@ -1158,6 +1158,18 @@ static void add_rev_cmdline(struct rev_info *revs,
 	info->nr++;
 }
 
+static int has_interesting_revisions(struct rev_info *revs)
+{
+	struct rev_cmdline_info *info = &revs->cmdline;
+	unsigned int n;
+
+	for (n = 0; n < info->nr; n++) {
+		if (!(info->rev[n].flags & UNINTERESTING))
+			return 1;
+	}
+	return 0;
+}
+
 static void add_rev_cmdline_list(struct rev_info *revs,
 				 struct commit_list *commit_list,
 				 int whence,
@@ -2318,7 +2330,7 @@ static void NORETURN diagnose_missing_default(const char *def)
  */
 int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)
 {
-	int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0, revarg_opt;
+	int i, flags, left, seen_dashdash, read_from_stdin, revarg_opt;
 	struct argv_array prune_data = ARGV_ARRAY_INIT;
 	const char *submodule = NULL;
 
@@ -2401,8 +2413,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			argv_array_pushv(&prune_data, argv + i);
 			break;
 		}
-		else
-			got_rev_arg = 1;
 	}
 
 	if (prune_data.argc) {
@@ -2431,7 +2441,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		opt->tweak(revs, opt);
 	if (revs->show_merge)
 		prepare_show_merge(revs);
-	if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {
+	if (revs->def && !revs->rev_input_given && !has_interesting_revisions(revs)) {
 		struct object_id oid;
 		struct object *object;
 		struct object_context oc;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 153a50615..e6670859c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -213,6 +213,12 @@ test_expect_success 'git show <commits> leaves list of commits as given' '
 	test_cmp expect actual
 '
 
+printf "sixth\nfifth\n" > expect
+test_expect_success '^<rev>' '
+	git log --pretty="tformat:%s" ^HEAD~2 > actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup case sensitivity tests' '
 	echo case >one &&
 	test_tick &&

-- 
2.17.1
Junio C Hamano· Aug 29, 2018, 21:03 UTC · re: Andreas Gruenbacher · lore

Re: [RFC] revision: Don't let ^<rev> cancel out the default <rev>

Andreas Gruenbacher <agruenba@redhat.com> writes:
Show 14 quoted lines
> Some commands like 'log' default to HEAD if no other revisions are
> specified on the command line or otherwise.  Unfortunately, excludes
> (^<rev>) cancel out that default, so when a command only excludes
> revisions (e.g., 'git log ^origin/master'), the command will not produce
> any result.
>
> If all the specified revisions are excludes, it seems more useful to
> stick with the default revision instead.
>
> This makes writing wrappers that exclude certain revisions much easier:
> for example, a simple alias l='git log ^origin/master' will show the
> revisions between origin/master and HEAD by default, and 'l foo' will
> show the revisions between origin/master and foo, as you would usually
> expect.

That is a _huge_ departure from the behaviour established for the past 10 years, but it would certainly be more useful.

As long as we can prove that that a command line with only negative revs is absolutely useless, the backward incompatibility may be OK to swallow, especially for commands like "git log" that implicitly use "--default HEAD", as they are meant for human consumption, and not for scripts.

I am not offhand 100% sure that a rev list with only negative revs is totally useless, though.

Jeff King· Aug 29, 2018, 23:30 UTC · re: Junio C Hamano · lore

Re: [RFC] revision: Don't let ^<rev> cancel out the default <rev>

On Wed, Aug 29, 2018 at 02:03:34PM -0700, Junio C Hamano wrote:
Show 28 quoted lines
> Andreas Gruenbacher <agruenba@redhat.com> writes:
> 
> > Some commands like 'log' default to HEAD if no other revisions are
> > specified on the command line or otherwise.  Unfortunately, excludes
> > (^<rev>) cancel out that default, so when a command only excludes
> > revisions (e.g., 'git log ^origin/master'), the command will not produce
> > any result.
> >
> > If all the specified revisions are excludes, it seems more useful to
> > stick with the default revision instead.
> >
> > This makes writing wrappers that exclude certain revisions much easier:
> > for example, a simple alias l='git log ^origin/master' will show the
> > revisions between origin/master and HEAD by default, and 'l foo' will
> > show the revisions between origin/master and foo, as you would usually
> > expect.
> 
> That is a _huge_ departure from the behaviour established for the
> past 10 years, but it would certainly be more useful.
> 
> As long as we can prove that that a command line with only negative
> revs is absolutely useless, the backward incompatibility may be OK
> to swallow, especially for commands like "git log" that implicitly
> use "--default HEAD", as they are meant for human consumption, and
> not for scripts.
> 
> I am not offhand 100% sure that a rev list with only negative revs
> is totally useless, though.

Yeah, I'm uneasy that somebody is relying on the current behavior, especially for scripting where we often do something like:

  git rev-list $new --not --all

and an empty $new really should return an empty result (that's our usual connectivity check, but I'd imagine some pre-receive hooks may end up doing similar things). For rev-list I think you'd have to specify --default to trigger this behavior, so that helps. But it still makes me nervous.

I'm _less_ uneasy with it for git-log, though I think people do have a habit of scripting around it (because it knows some tricks that rev-list doesn't).

Given that the mentioned use case was writing wrappers, could we hide this behind:

  git config alias.l 'git log --default-on-negative ^origin/master'

That's obviously less convenient to type, but I think it would usually be part of scripted calls (otherwise you'd know already that you have no positive refs and would just add ".." or HEAD or whatever).

-Peff

← back to recent threads