@matheusbsbs / matheus-bagliano / commits / 04a0e98515

Fork 0

revision: set rev_input_given in handle_revision_arg()

Commit 7ba826290a (revision: add rev_input_given flag, 2017-08-02) added a flag to rev_info to tell whether we got any revision arguments. As explained there, this is necessary because some revision arguments may not produce any pending traversal objects, but should still inhibit default behaviors (e.g., a glob that matches nothing). However, it only set the flag in the globbing code, but not for revisions we get on the command-line or via stdin. This leads to two problems: - the command-line code keeps its own separate got_rev_arg flag; this isn't wrong, but it's confusing and an extra maintenance burden - even specifically-named rev arguments might end up not adding any pending objects: if --ignore-missing is set, then specifying a missing object is a noop rather than an error. And that leads to some user-visible bugs: - when deciding whether a default rev like "HEAD" should kick in, we check both got_rev_arg and rev_input_given. That means that "--ignore-missing $ZERO_OID" works on the command-line (where we set got_rev_arg) but not on --stdin (where we don't) - when rev-list decides whether it should complain that it wasn't given a starting point, it relies on rev_input_given. So it can't even get the command-line "--ignore-missing $ZERO_OID" right Let's consistently set the flag if we got any revision argument. That lets us clean up the redundant got_rev_arg, and fixes both of those bugs (but note there are three new tests: we'll confirm the already working git-log command-line case). A few implementation notes: - conceptually we want to set the flag whenever handle_revision_arg() finds an actual revision arg ("handles" it, you might say). But it covers a ton of cases with early returns. Rather than annotating each one, we just wrap it and use its success exit-code to set the flag in one spot. - the new rev-list test is in t6018, which is titled to cover globs. This isn't exactly a glob, but it made sense to stick it with the other tests that handle the "even though we got a rev, we have no pending objects" case, which are globs. - the tests check for the oid of a missing object, which it's pretty clear --ignore-missing should ignore. You can see the same behavior with "--ignore-missing a-ref-that-does-not-exist", because --ignore-missing treats them both the same. That's perhaps less clearly correct, and we may want to change that in the future. But the way the code and tests here are written, we'd continue to do the right thing even if it does. Reported-by: Bryan Turner <bturner@atlassian.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Aug 26, 2020 at 16:13 UTC 04a0e98515a7b6e2e43eddea239aacd2a5a71632

3 files changed +26 -5

revision.c

+11 -5

@@ -1997,7 +1997,7 @@ static int handle_dotdot(const char *arg,

1997 return ret;

1998 }

1999

2000 -int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)

2000 +static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)

2001 {

2002 struct object_context oc;

2003 char *mark;

@@ -2072,6 +2072,14 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi

2072 return 0;

2073 }

2074

2075 +int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)

2076 +{

2077 + int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);

2078 + if (!ret)

2079 + revs->rev_input_given = 1;

2080 + return ret;

2081 +}

2082 +

2083 static void read_pathspec_from_stdin(struct strbuf *sb,

2084 struct argv_array *prune)

2085 {

@@ -2674,7 +2682,7 @@ static void NORETURN diagnose_missing_default(const char *def)

2682 */

2683 int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)

2684 {

2677 - int i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;

2685 + int i, flags, left, seen_dashdash, revarg_opt;

2686 struct argv_array prune_data = ARGV_ARRAY_INIT;

2687 const char *submodule = NULL;

2688 int seen_end_of_options = 0;

@@ -2763,8 +2771,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s

2771 argv_array_pushv(&prune_data, argv + i);

2772 break;

2773 }

2766 - else

2767 - got_rev_arg = 1;

2774 }

2775

2776 if (prune_data.argc) {

@@ -2793,7 +2799,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s

2799 opt->tweak(revs, opt);

2800 if (revs->show_merge)

2801 prepare_show_merge(revs);

2796 - if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {

2802 + if (revs->def && !revs->pending.nr && !revs->rev_input_given) {

2803 struct object_id oid;

2804 struct object *object;

2805 struct object_context oc;

t/t4202-log.sh

+10

@@ -1850,6 +1850,16 @@ test_expect_success 'log does not default to HEAD when rev input is given' '

1850 test_must_be_empty actual

1851 '

1852

1853 +test_expect_success 'do not default to HEAD with ignored object on cmdline' '

1854 + git log --ignore-missing $ZERO_OID >actual &&

1855 + test_must_be_empty actual

1856 +'

1857 +

1858 +test_expect_success 'do not default to HEAD with ignored object on stdin' '

1859 + echo $ZERO_OID | git log --ignore-missing --stdin >actual &&

1860 + test_must_be_empty actual

1861 +'

1862 +

1863 test_expect_success 'set up --source tests' '

1864 git checkout --orphan source-a &&

1865 test_commit one &&

t/t6018-rev-list-glob.sh

@@ -345,6 +345,11 @@ test_expect_success 'rev-list should succeed with empty output with empty glob'

345 test_must_be_empty actual

346 '

347

348 +test_expect_success 'rev-list should succeed with empty output when ignoring missing' '

349 + git rev-list --ignore-missing $ZERO_OID >actual &&

350 + test_must_be_empty actual

351 +'

352 +

353 test_expect_success 'shortlog accepts --glob/--tags/--remotes' '

354

355 compare shortlog "subspace/one subspace/two" --branches=subspace &&