diff mbox series

[3/8] ahead-behind: implement --ignore-missing option

Message ID b1d022c7cac5aed6e2d64b45f20dba5b3297536c.1678111599.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series ahead-behind: new builtin for counting multiple commit ranges | expand

Commit Message

Derrick Stolee March 6, 2023, 2:06 p.m. UTC 
From: Derrick Stolee <derrickstolee@github.com>

When parsing the tip revisions from the ahead-behind inputs, it is
important to check that those tips exist before adding them to the list
for computation. The previous change caused the builtin to return with
errors if the revisions could not be resolved.

However, when running 'git ahead-behind' in an environment with
concurrent edits, such as a Git server, then the references could be
deleted from underneath the caller between reading the reference list
and starting the 'git ahead-behind' process. Avoid this race by allowing
the caller to specify '--ignore-missing' and continue using the
information that is still available.

Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 Documentation/git-ahead-behind.txt | 6 ++++++
 builtin/ahead-behind.c             | 8 +++++++-
 t/t4218-ahead-behind.sh            | 6 ++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

Comments

Taylor Blau March 7, 2023, 12:46 a.m. UTC | #1 
On Mon, Mar 06, 2023 at 02:06:33PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> When parsing the tip revisions from the ahead-behind inputs, it is
> important to check that those tips exist before adding them to the list
> for computation. The previous change caused the builtin to return with
> errors if the revisions could not be resolved.
>
> However, when running 'git ahead-behind' in an environment with
> concurrent edits, such as a Git server, then the references could be
> deleted from underneath the caller between reading the reference list
> and starting the 'git ahead-behind' process. Avoid this race by allowing
> the caller to specify '--ignore-missing' and continue using the
> information that is still available.

Well explained, thanks for writing this all out :-).

> diff --git a/builtin/ahead-behind.c b/builtin/ahead-behind.c
> index c1212cc8d46..e4f65fc0548 100644
> --- a/builtin/ahead-behind.c
> +++ b/builtin/ahead-behind.c
> @@ -8,13 +8,18 @@ static const char * const ahead_behind_usage[] = {
>  	NULL
>  };
>
> +static int ignore_missing;
> +
>  static int handle_arg(struct string_list *tips, const char *arg)
>  {
>  	struct string_list_item *item;
>  	struct commit *c = lookup_commit_reference_by_name(arg);
>
> -	if (!c)
> +	if (!c) {
> +		if (ignore_missing)
> +			return 0;
>  		return error(_("could not resolve '%s'"), arg);
> +	}

And the diff makes complete sense here, too.

>  	item = string_list_append(tips, arg);
>  	item->util = c;
> @@ -30,6 +35,7 @@ int cmd_ahead_behind(int argc, const char **argv, const char *prefix)
>  	struct option ahead_behind_opts[] = {
>  		OPT_STRING('b', "base", &base_ref, N_("base"), N_("base reference to process")),
>  		OPT_BOOL(0 , "stdin", &from_stdin, N_("read rev names from stdin")),
> +		OPT_BOOL(0 , "ignore-missing", &ignore_missing, N_("ignore missing tip references")),

The spacing between "0" and the comma and "ignore-missing" (as well with
"stdin" above, though I didn't notice it when reading the previous
patch) is a little funky.

I suspect that it is carried over from some historical typo from many
years ago, but probably worth fixing while we're thinking about it.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/Documentation/git-ahead-behind.txt b/Documentation/git-ahead-behind.txt
index 0e2f989a1a0..2dd5147f6b2 100644
--- a/Documentation/git-ahead-behind.txt
+++ b/Documentation/git-ahead-behind.txt
@@ -50,6 +50,12 @@  OPTIONS
 	Read revision tips and ranges from stdin instead of from the
 	command-line.
 
+--ignore-missing::
+	When parsing tip references, ignore any references that are not
+	found. This is useful when operating in an environment where a
+	reference could be deleted between reading the reference and
+	starting the `git ahead-behind` process.
+
 
 SEE ALSO
 --------
diff --git a/builtin/ahead-behind.c b/builtin/ahead-behind.c
index c1212cc8d46..e4f65fc0548 100644
--- a/builtin/ahead-behind.c
+++ b/builtin/ahead-behind.c
@@ -8,13 +8,18 @@  static const char * const ahead_behind_usage[] = {
 	NULL
 };
 
+static int ignore_missing;
+
 static int handle_arg(struct string_list *tips, const char *arg)
 {
 	struct string_list_item *item;
 	struct commit *c = lookup_commit_reference_by_name(arg);
 
-	if (!c)
+	if (!c) {
+		if (ignore_missing)
+			return 0;
 		return error(_("could not resolve '%s'"), arg);
+	}
 
 	item = string_list_append(tips, arg);
 	item->util = c;
@@ -30,6 +35,7 @@  int cmd_ahead_behind(int argc, const char **argv, const char *prefix)
 	struct option ahead_behind_opts[] = {
 		OPT_STRING('b', "base", &base_ref, N_("base"), N_("base reference to process")),
 		OPT_BOOL(0 , "stdin", &from_stdin, N_("read rev names from stdin")),
+		OPT_BOOL(0 , "ignore-missing", &ignore_missing, N_("ignore missing tip references")),
 		OPT_END()
 	};
 
diff --git a/t/t4218-ahead-behind.sh b/t/t4218-ahead-behind.sh
index 3b8b9dc9887..56f16515896 100755
--- a/t/t4218-ahead-behind.sh
+++ b/t/t4218-ahead-behind.sh
@@ -19,6 +19,12 @@  test_expect_success 'git ahead-behind with broken tip' '
 	grep "could not resolve '\''bogus'\''" err
 '
 
+test_expect_success 'git ahead-behind with broken tip and --ignore-missing' '
+	git ahead-behind --base=HEAD --ignore-missing bogus 2>err >out &&
+	test_must_be_empty err &&
+	test_must_be_empty out
+'
+
 test_expect_success 'git ahead-behind without tips' '
 	git ahead-behind --base=HEAD 2>err &&
 	test_must_be_empty err