Re: [PATCH v6 6/7] git-reflog: add create and exists functions

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jul 6, 2015, 16:51 UTC
Message-ID: <559AB200.5020108@alum.mit.edu>
In-Reply-To: <1435609076-8592-7-git-send-email-dturner@twopensource.com>

On 06/29/2015 10:17 PM, David Turner wrote:

Show 64 quoted lines

> These are necessary because alternate ref backends might store reflogs
> somewhere other than .git/logs.  Code that now directly manipulates
> .git/logs should instead go through git-reflog.
> 
> In a moment, we will use these functions to make git stash work with
> alternate ref backends.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  Documentation/git-reflog.txt | 10 ++++++
>  builtin/reflog.c             | 75 +++++++++++++++++++++++++++++++++++++++++++-
>  t/t1411-reflog-show.sh       | 12 +++++++
>  3 files changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
> index 5e7908e..2bf8aa6 100644
> --- a/Documentation/git-reflog.txt
> +++ b/Documentation/git-reflog.txt
> @@ -23,6 +23,8 @@ depending on the subcommand:
>  	[--dry-run] [--verbose] [--all | <refs>...]
>  'git reflog delete' [--rewrite] [--updateref]
>  	[--dry-run] [--verbose] ref@\{specifier\}...
> +'git reflog create' <refs>...
> +'git reflog exists' <ref>
>  
>  Reference logs, or "reflogs", record when the tips of branches and
>  other references were updated in the local repository. Reflogs are
> @@ -52,6 +54,14 @@ argument must be an _exact_ entry (e.g. "`git reflog delete
>  master@{2}`"). This subcommand is also typically not used directly by
>  end users.
>  
> +The "create" subcommand creates a reflog for one or more refs. Most
> +refs (those under refs/heads, refs/remotes, and refs/tags) will
> +automatically have reflogs created. Other refs will not. This command
> +allows manual ref creation.
> +
> +The "exists" subcommand checks whether a ref has a reflog.  It exists
> +with zero status if the reflog exists, and non-zero status if it does
> +not.
>  
>  OPTIONS
>  -------
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index c2eb8ff..3080865 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -13,6 +13,10 @@ static const char reflog_expire_usage[] =
>  "git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...";
>  static const char reflog_delete_usage[] =
>  "git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...";
> +static const char reflog_create_usage[] =
> +"git reflog create <refs>...";
> +static const char reflog_exists_usage[] =
> +"git reflog exists <ref>";
>  
>  static unsigned long default_reflog_expire;
>  static unsigned long default_reflog_expire_unreachable;
> @@ -699,12 +703,75 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  	return status;
>  }
>  
> +static int cmd_reflog_create(int argc, const char **argv, const char *prefix)
> +{
> +	int i, status = 0, start = 0;

It looks like start is initialized unconditionally after the first loop, so the initialization here is a red herring.

Show 17 quoted lines

> +	struct strbuf err = STRBUF_INIT;
> +
> +	for (i = 1; i < argc; i++) {
> +		const char *arg = argv[i];
> +		if (!strcmp(arg, "--")) {
> +			i++;
> +			break;
> +		}
> +		else if (arg[0] == '-')
> +			usage(reflog_create_usage);
> +		else
> +			break;
> +	}
> +
> +	start = i;
> +
> +	if (argc - start < 1)

    if (start == argc)

seems clearer, but that might just be me.

Show 17 quoted lines

> +		return error("Nothing to create?");
> +
> +	for (i = start; i < argc; i++) {
> +		if (check_refname_format(argv[i], REFNAME_ALLOW_ONELEVEL))
> +			die("invalid ref format: %s", argv[i]);
> +	}
> +	for (i = start; i < argc; i++) {
> +		if (safe_create_reflog(argv[i], &err, 1)) {
> +			error("could not create reflog %s: %s", argv[i],
> +			      err.buf);
> +			status = 1;
> +			strbuf_release(&err);
> +		}
> +	}
> +	return status;
> +}
> +

So, I have a philosophical question here with a practical side...

It appears that this command allows you to create a reflog for a reference that doesn't yet exist. At first blush, this seems to make sense. Suppose you want the creation of a reference to be logged. Then you can first turn on its reflog, then create it.

But I think this is going to create problems. Reflogs are rather intimately connected to their references. For example, writes to a reflog are guarded by locking the corresponding reference. And currently a reflog file is deleted when the corresponding reference is deleted. Also, there are constraints about which references can coexist; for example, references "refs/heads/foo" and "refs/heads/foo/bar" cannot exist at the same time, because (at least when using the filesystem backend), "refs/heads/foo" would have to be both a file and a directory at the same time. So if one of these references already exists, it would be an error to create a reflog for the other one.

Similarly, if there is a reflog file for one of these references that was created by this command, but there is not yet a corresponding reference, then any command that later tries to create the other reference will not be able to create the reflog for *that* reference because it will conflict with the existing reflog. I doubt that the reference creation is smart enough to deal with this situation.

So all in all, I think it is unwise to allow a reflog to be created without its corresponding reference.

This, in turn, suggests one or both of the following alternatives:

1. Allow "git reflog create", but only for references that already exist.

2. If we want to allow a reflog to be created at the same time as the
corresponding reference, the reference-creation commands ("git branch",
"git tag", "git update-ref", and maybe some others) probably need a new
option like "--create-reflog" (and, for symmetry, probably
"--no-create-reflog").

At the API level, it might make sense for the ref-transaction functions to get a new "REF_FORCE_CREATE_REFLOG" flag or something.

Show 70 quoted lines

> +static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)
> +{
> +	int i, start = 0;
> +
> +	for (i = 1; i < argc; i++) {
> +		const char *arg = argv[i];
> +		if (!strcmp(arg, "--")) {
> +			i++;
> +			break;
> +		}
> +		else if (arg[0] == '-')
> +			usage(reflog_exists_usage);
> +		else
> +			break;
> +	}
> +
> +	start = i;
> +
> +	if (argc - start != 1)
> +		usage(reflog_exists_usage);
> +
> +	if (check_refname_format(argv[start], REFNAME_ALLOW_ONELEVEL))
> +		die("invalid ref format: %s", argv[start]);
> +	return !reflog_exists(argv[start]);
> +}
> +
>  /*
>   * main "reflog"
>   */
>  
>  static const char reflog_usage[] =
> -"git reflog [ show | expire | delete ]";
> +"git reflog [ show | expire | delete | create | exists ]";
>  
>  int cmd_reflog(int argc, const char **argv, const char *prefix)
>  {
> @@ -724,5 +791,11 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
>  	if (!strcmp(argv[1], "delete"))
>  		return cmd_reflog_delete(argc - 1, argv + 1, prefix);
>  
> +	if (!strcmp(argv[1], "create"))
> +		return cmd_reflog_create(argc - 1, argv + 1, prefix);
> +
> +	if (!strcmp(argv[1], "exists"))
> +		return cmd_reflog_exists(argc - 1, argv + 1, prefix);
> +
>  	return cmd_log_reflog(argc, argv, prefix);
>  }
> diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
> index 6f47c0d..6e1abe7 100755
> --- a/t/t1411-reflog-show.sh
> +++ b/t/t1411-reflog-show.sh
> @@ -166,4 +166,16 @@ test_expect_success 'git log -g -p shows diffs vs. parents' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'reflog exists works' '
> +	git reflog exists refs/heads/master &&
> +	! git reflog exists refs/heads/nonexistent
> +'
> +
> +test_expect_success 'reflog create works' '
> +	git update-ref non-refs-dir HEAD &&
> +	! git reflog exists non-refs-dir &&
> +	git reflog create non-refs-dir &&
> +	git reflog exists non-refs-dir
> +'
> +
>  test_done
>

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu