Re: [PATCH 3/4] cat-file --textconv/--filters: allow specifying the path separately

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 18, 2016, 16:52 UTC
Message-ID: <xmqqfuq26mu3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <787224ce9a382ddd1180a408e784ca26993e5603.1471524357.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> There are circumstances when it is relatively easy to figure out the
> object name for a given path, but not the revision. For example, when

revision of the containing tree, I presume?

> Let's simplify this dramatically, because we do not really need that
> revision at all: all we care about is that we know the path. In the
> scenario described above, we do know the path, and we just want to
> specify it separately from the object name.

I wouldn't call it "simplifying dramatically". It is just to support two use cases that is different from the use case where you want to use "<tree>:<path>".

We already have a precedent in "hash-object --path=<path>" for these two different uses cases from the primary one. That form can be used when we know the contents but we do not know where the contents came from. It can also be used when we want to pretend that contents came from a specific path, that may be different from the file we are hashing.

Let's be consistent and (1) call it "--path", and (2) explain it as a feature to allow you to tell the path separately or allow you to pretend that the content is at a path different from reality.

After all, you would want to ignore <path2> part in this construct:

	git cat-file --filter --path=<path1> HEAD:<path2>

for the purpose of filtering, right?

Show 26 quoted lines

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 0b74afa..5ff58b3 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -20,6 +20,8 @@ struct batch_options {
>  	const char *format;
>  };
>  
> +static const char *force_path;
> +
>  static int filter_object(const char *path, unsigned mode,
>  			 const unsigned char *sha1,
>  			 char **buf, unsigned long *size)
> @@ -89,21 +91,24 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>  		return !has_sha1_file(sha1);
>  
>  	case 'w':
> -		if (!obj_context.path[0])
> +		if (!force_path && !obj_context.path[0])
>  			die("git cat-file --filters %s: <object> must be "
>  			    "<sha1:path>", obj_name);
>  
> -		if (filter_object(obj_context.path, obj_context.mode,
> +		if (filter_object(force_path ? force_path : obj_context.path,
> +				  force_path ? 0100644 : obj_context.mode,
>  				  sha1, &buf, &size))

The mode override is somewhat questionable. Wouldn't you want to reject

	git cat-file --filter --path=Makefile HEAD:RelNotes

when HEAD:RelNotes blob is known to be a symbolic link? After all, you would reject this

	git cat-file --filter --path=Makefile HEAD:t/

and it is quite similar, no? I think this can be argued both ways, but I have a feeling that obj_context.mode, if available, should be honored with or without force_path.

Show 20 quoted lines

> diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
> index e466634..fd17159 100755
> --- a/t/t8010-cat-file-filters.sh
> +++ b/t/t8010-cat-file-filters.sh
> @@ -31,4 +31,17 @@ test_expect_success 'cat-file --filters converts to worktree version' '
>  	has_cr actual
>  '
>  
> +test_expect_success 'cat-file --filters --use-path=<path> works' '
> +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> +	git cat-file --filters --use-path=world.txt $sha1 >actual &&
> +	has_cr actual
> +'
> +
> +test_expect_success 'cat-file --textconv --use-path=<path> works' '
> +	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> +	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
> +	git cat-file --textconv --use-path=hello.txt $sha1 >rot13 &&
> +	test uryyb = "$(cat rot13 | remove_cr)"
> +'

I think I saw some code to ensure "when giving this option you need that option in effect, too"; they should be tested here, too, no?