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

[Johannes Schindelin]

Hi Junio,

On Fri, 19 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> 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?
> >
> > No, I would rather not test for that. These conditionals are purely for
> > any user's convenience, in case they specify an option that has no effect.
> > They are absolutely not essential for the function introduced in this
> > patch series.
> 
> I didn't say "you would want to test these, no?", did I?
> 
> I do not want to see bugreports that say "I wanted to use this new
> feature and by mistake gave only --path without giving --filter; Git
> should have complained.  I found a bug, hooray!" when somebody in
> the future refactors the command line option parsing and breaks the
> check you already have.

I added a test to verify that --path without --filters nor --textconv
complains.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> 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?
>
> No, I would rather not test for that. These conditionals are purely for
> any user's convenience, in case they specify an option that has no effect.
> They are absolutely not essential for the function introduced in this
> patch series.

I didn't say "you would want to test these, no?", did I?

I do not want to see bugreports that say "I wanted to use this new
feature and by mistake gave only --path without giving --filter; Git
should have complained.  I found a bug, hooray!" when somebody in
the future refactors the command line option parsing and breaks the
check you already have.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

Hi Junio,

On Thu, 18 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  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?

name of containing tree, actually.

> > 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 ":".

"this" was not the code in question. "this" is the use case. Just put my
shoes on for a moment. As described: you have a list of object names and
their path. Now you need to use the --textconv or --filters option, but
you do not have the commit name. What do you do? Concoct a script that
goes through `git rev-list --all -- ` in the hopes of finding a
revision soon? I hope after this little exercise you agree that
introducing the --path= option simplifies this use case
dramatically.

> We already have a precedent in "hash-object --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.

Agreed. I was unaware of hash-object's existing option.

> 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  part in this construct:
> 
>   git cat-file --filter --path= HEAD:
> 
> for the purpose of filtering, right?

True, and I even make use of that in the test for the batch mode.

> > 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:  must be "
> > "", 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, , ))
> 
> 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.

I see your point. All I wanted to do was to enable

git cat-file --filters --path=Makefile cafebabe

in which case obj_context.mode == S_IFINVALID. I fixed it (see interdiff
of the next iteration).

> > 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= 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= 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?

No, I would rather not test for that. These conditionals are purely for
any user's 

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

[Junio C Hamano]

Johannes Schindelin  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 ":".

We already have a precedent in "hash-object --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  part in this construct:

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

for the purpose of filtering, right?

> 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:  must be "
>   "", 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, , ))

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.

> 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= 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= 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

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
looking at a diff generated by Git, the object names are recorded, but
not the revision. As a matter of fact, the revisions from which the diff
was generated may not even exist locally.

In such a case, the user would have to generate a fake revision just to
be able to use --textconv or --filters.

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.

Example usage:

git cat-file --textconv --use-path=main.c 0f1937fd

Signed-off-by: Johannes Schindelin 
---
 Documentation/git-cat-file.txt |  7 ++-
 builtin/cat-file.c | 22 +-
 t/t8010-cat-file-filters.sh| 13 +
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 7d48735..59a3c37 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,7 +9,7 @@ git-cat-file - Provide content or type and size information for 
repository objec
 SYNOPSIS
 
 [verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p 
|  | --textconv | --filters ) 
+'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p 
|  | --textconv | --filters ) [--use-path=] 
 'git cat-file' (--batch | --batch-check) [--follow-symlinks]
 
 DESCRIPTION
@@ -64,6 +64,11 @@ OPTIONS
end-of-line conversion, etc). In this case,  has to be of
the form :, or :.
 
+--use-path=::
+   For use with --textconv or --filters, to allow specifying an object
+   name and a path separately, e.g. when it is difficult to figure out
+   the revision from which the blob came.
+
 --batch::
 --batch=::
Print object information and contents for each object provided
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:  must be "
"", 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, , ))
return -1;
break;
 
case 'c':
-   if (!obj_context.path[0])
+   if (!force_path && !obj_context.path[0])
die("git cat-file --textconv %s:  must be 
",
obj_name);
 
-   if (textconv_object(obj_context.path, obj_context.mode, sha1, 
1, , ))
+   if (textconv_object(force_path ? force_path : obj_context.path,
+   force_path ? 0100644 : obj_context.mode,
+   sha1, 1, , ))
break;
 
case 'p':
@@ -477,7 +482,7 @@ static int batch_objects(struct batch_options *opt)
 }
 
 static const char * const cat_file_usage[] = {
-   N_("git cat-file (-t [--allow-unknown-type]|-s 
[--allow-unknown-type]|-e|-p||--textconv|--filters) "),
+   N_("git cat-file (-t [--allow-unknown-type]|-s 
[--allow-unknown-type]|-e|-p||--textconv|--filters) [--use-path=] 
"),
N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
NULL
 };
@@ -525,6 +530,8 @@ int cmd_cat_file(int argc, const char **argv, const char 
*prefix)
N_("for blob objects, run textconv on object's 
content"), 'c'),
OPT_CMDMODE(0, "filters", ,
N_("for blob objects, run filters on object's 
content"), 'w'),
+   OPT_STRING(0, "use-path", _path, N_("blob"),
+  N_("use a specific path for --textconv/--filters")),
OPT_BOOL(0, "allow-unknown-type", _type,
  N_("allow -s and -t to work with broken/corrupt 
objects")),
OPT_BOOL(0, "buffer", _output, N_("buffer --batch 
output")),
@@ -567,6 +574,11 @@ int cmd_cat_file(int argc, const char **argv, const char