ref-filter: add contents:raw atom
PATCH 1/3 quote: add *.quote_buf_with_size functions
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
With the original functions *._quote_buf(), there may be '\0' in our source string, only the content before the first '\0' will be processed and added to the buffer.
Add perl_quote_buf_with_size(), python_quote_buf_with_size(), tcl_quote_buf_with_size,sq_quote_buf_with_size() to quote.c, they will process the source string with specified length characters. With them, the content after '\0' will not be truncated.
This will help us add binary data containing '\0' to quote_formatting() in ref-filter.c.
--- quote.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ quote.h | 4 ++ 2 files changed, 120 insertions(+)
quote.c +116 -0
@@ -43,6 +43,39 @@ void sq_quote_buf(struct strbuf *dst, const char *src)free (to_free); } +void sq_quote_buf_with_size (struct strbuf *dst,const char *src,size_t size) +{ +char *to_free =NULL ; +size_t cur_size =0 ; + +if (dst->buf == src) + to_free =strbuf_detach (dst,NULL ); + +strbuf_addch (dst,'\'' ); +while (cur_size < size) { +size_t len =strcspn (src,"'!" ); +if (!len) { +strbuf_add (dst, src,1 ); + src++; + cur_size++; + }else { +strbuf_add (dst, src, len); + src += len; + cur_size += len; + } +if (cur_size >= size) +break ; +while (need_bs_quote (*src)) { +strbuf_addstr (dst,"'\\" ); +strbuf_addch (dst, *src++); + cur_size++; +strbuf_addch (dst,'\'' ); + } + } +strbuf_addch (dst,'\'' ); +free (to_free); +} +void sq_quote_buf_pretty (struct strbuf *dst,const char *src) {static const char ok_punct[] ="+,-./:=@_^" ;
@@ -471,6 +504,25 @@ void perl_quote_buf(struct strbuf *sb, const char *src)strbuf_addch (sb, sq); } +void perl_quote_buf_with_size (struct strbuf *sb,const char *src,size_t size) +{ +const char sq ='\'' ; +const char bq ='\\' ; +char c; +size_t cur_size =0 ; + +strbuf_addch (sb, sq); +while (cur_size < size) { + c = *src++; + cur_size++; + +if (c == sq || c == bq) +strbuf_addch (sb, bq); +strbuf_addch (sb, c); + } +strbuf_addch (sb, sq); +} +void python_quote_buf (struct strbuf *sb,const char *src) {const char sq ='\'' ;
@@ -492,6 +544,31 @@ void python_quote_buf(struct strbuf *sb, const char *src)strbuf_addch (sb, sq); } +void python_quote_buf_with_size (struct strbuf *sb,const char *src,size_t size) +{ +const char sq ='\'' ; +const char bq ='\\' ; +const char nl ='\n' ; +char c; +size_t cur_size =0 ; + +strbuf_addch (sb, sq); +while (cur_size < size) { + c = *src++; + cur_size++; + +if (c == nl) { +strbuf_addch (sb, bq); +strbuf_addch (sb,'n' ); +continue ; + } +if (c == sq || c == bq) +strbuf_addch (sb, bq); +strbuf_addch (sb, c); + } +strbuf_addch (sb, sq); +} +void tcl_quote_buf (struct strbuf *sb,const char *src) {char c;
@@ -527,6 +604,45 @@ void tcl_quote_buf(struct strbuf *sb, const char *src)strbuf_addch (sb,'"' ); } +void tcl_quote_buf_with_size (struct strbuf *sb,const char *src,size_t size) +{ +char c; +size_t cur_size =0 ; + +strbuf_addch (sb,'"' ); +while (cur_size < size) { + c = *src++; + cur_size++; + +switch (c) { +case '[' :case ']' : +case '{' :case '}' : +case '$' :case '\\' :case '"' : +strbuf_addch (sb,'\\' ); +/* fallthrough */ +default : +strbuf_addch (sb, c); +break ; +case '\f' : +strbuf_addstr (sb,"\\f" ); +break ; +case '\r' : +strbuf_addstr (sb,"\\r" ); +break ; +case '\n' : +strbuf_addstr (sb,"\\n" ); +break ; +case '\t' : +strbuf_addstr (sb,"\\t" ); +break ; +case '\v' : +strbuf_addstr (sb,"\\v" ); +break ; + } + } +strbuf_addch (sb,'"' ); +} +void basic_regex_quote_buf (struct strbuf *sb,const char *src) {char c;
quote.h +4 -0
@@ -30,6 +30,7 @@ struct strbuf;*/ void sq_quote_buf (struct strbuf *,const char *src); +void sq_quote_buf_with_size (struct strbuf *,const char *src,size_t size);void sq_quote_argv (struct strbuf *,const char **argv);void sq_quotef (struct strbuf *,const char *fmt, ...);
@@ -94,8 +95,11 @@ char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigne/* quoting as a string literal for other languages */ void perl_quote_buf (struct strbuf *sb,const char *src); +void perl_quote_buf_with_size (struct strbuf *sb,const char *src,size_t size);void python_quote_buf (struct strbuf *sb,const char *src); +void python_quote_buf_with_size (struct strbuf *sb,const char *src,size_t size);void tcl_quote_buf (struct strbuf *sb,const char *src); +void tcl_quote_buf_with_size (struct strbuf *sb,const char *src,size_t size);void basic_regex_quote_buf (struct strbuf *sb,const char *src);#endif
Signature · 1 line
gitgitgadget
cover ref-filter: add contents:raw atom
In (a2f3241: [GSOC] ref-filter: add contents:raw atom) I did not notice the breakage that occurred during the test, In the later remediation, I found a very serious problem: The output object data of "git cat-file tree foo" or "git cat-file blob foo" may contain '\0'. However, most of the logic in ref-filter depends on the atomic output not containing'\0'.
Therefore, we must carry out a series of repairs to ref-filter so that it can support output of data containing '\0'.
In first patch, I add *.quote_buf_with_size() functions, this can deal with data with containing'\0'.
In second patch, I add the member s_size in struct atom_value, and protects the output of the atom from being truncated at '\0', and successfully supported the %(contents) of blob and tree.
In third patch, I added the%(contents:raw) atom, It can print the original content of an object.
What needs to be reconsidered:
For a binary object blob, tree,
git for-each-ref --format="%(contents)" --python refs/mytrees/first
will output a string processed by python_quote_buf_with_size(), which contains'\0'. But the binary files seem to be useless after quoting. Should we allow these binary files to be output in the default way with strbuf_add()? If so, we can remove the first patch.
ZheNing Hu (3):
[GSOC] quote: add *.quote_buf_with_size functions [GSOC] ref-filter: support %(contents) for blob, tree [GSOC] ref-filter: add contents:raw atom
Documentation/git-for-each-ref.txt | 19 ++- quote.c | 116 +++++++++++++++ quote.h | 4 + ref-filter.c | 229 +++++++++++++++++++++-------- t/t6300-for-each-ref.sh | 214 ++++++++++++++++++++++++++- 5 files changed, 511 insertions(+), 71 deletions(-)
base-commit: 97eea85a0a1ec66d356567808a1e4ca2367e0ce7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-959%2Fadlternative%2Fref-filter-contents-raw-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-959/adlternative/ref-filter-contents-raw-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/959
Signature · 1 line
gitgitgadget
PATCH 2/3 ref-filter: support %(contents) for blob, tree
From: ZheNing Hu <[email protected]>
In order to let cat-file --batch use ref-filter logic, we need to print the original content of an object. We can reuse the existing atom %(contents) in ref-filter, The original %(contents) only supports tag and commit objects. If we want to support both blob and tree objects, we must consider the following issues:
The original contents of blob, tree objects may contain '\0', most of the logic in ref-filter depends on the output of the atom being a string (end with '\0'):
quote_formatting() use strbuf_addstr() or *._quote_buf() add content to the buffer. E.g. The original content of a tree object is 100644 one\0..., only the 100644 one will be added to the buffer, which is incorrect.
Therefore, add a new member in struct atom_value: s_size, when we record the original content of the blob and tree objects in grab_contents(), use v->s_size to record the size of the objects contents, then in quote_formatting(), if the length of the contents passed in quote_formatting() is not equal to 0, we can use strbuf_add() instead of strbuf_addstr() or *._quote_buf_with_size() instead of *._quote_buf() to add contents with a specified length if the length of the contents is not equal to 0. It will not cause truncation problems.
Similarly, in append_atom(), we use strbuf_add() instead of strbuf_addstr(); In then_atom_handler(), we use memcmp() instread of strcmp(); In cmp_ref_sorting(), we use memcmp() and memcasecmp() instead of strcmp() and strcasecmp() when the v->s_size of one of the two atoms is not equals to 0.
--- Documentation/git-for-each-ref.txt | 11 +- ref-filter.c | 220 +++++++++++++++++++++-------- t/t6300-for-each-ref.sh | 166 +++++++++++++++++++++- 3 files changed, 327 insertions(+), 70 deletions(-)
Documentation/git-for-each-ref.txt +7 -4
@@ -235,11 +235,12 @@ and `date` to extract the named component. For email fields (`authoremail`, without angle brackets, and `:localpart` to get the part before the `@` symbol out of the trimmed email. -The message in a commit or a tag object is `contents`, from which -`contents:<part>` can be used to extract various parts out of: +The data in a object is `contents`, from which `contents:<part>` can be used +to extract various parts out of: contents:size:: - The size in bytes of the commit or tag message. + The size in bytes of the commit or tag message, and the raw object size + of the blob or tree. contents:subject:: The first paragraph of the message, which typically is a
@@ -257,7 +258,9 @@ contents:signature:: The optional GPG signature of the tag. contents:lines=N:: - The first `N` lines of the message. + The first `N` lines of the message of the commit or tag message. + +Note: blob and tree objects only support `%(contents)` and `%(contents:size)`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] are obtained as `trailers[:options]` (or by using the historical alias
ref-filter.c +158 -62
351 lines not on this page · raw message
t/t6300-for-each-ref.sh +162 -4
184 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH 3/3 ref-filter: add contents:raw atom
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
Add new formatting option %(contents:raw), which will print all the object contents without any changes. For blob and tree, it priont all object data, its output is equivalent to %(contents); For commit and tag, it not only print objects’ messages (as %(contents) does), but also print objects headers, like the "tree XXX", "parent YYY", etc lines in commits, and the "object OOO", "type TTT", etc lines in tags.
It will help further to migrate all cat-file formatting logic from cat-file to ref-filter.
E.g.
git for-each-ref --format=%(contents:raw) refs/heads/foo
It will have similar output to:
git rev-parse refs/heads/foo | git cat-file --batch
--- Documentation/git-for-each-ref.txt | 10 ++++++- ref-filter.c | 11 +++++-- t/t6300-for-each-ref.sh | 48 ++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 3 deletions(-)
Documentation/git-for-each-ref.txt +9 -1
@@ -260,7 +260,15 @@ contents:signature:: contents:lines=N:: The first `N` lines of the message of the commit or tag message. -Note: blob and tree objects only support `%(contents)` and `%(contents:size)`. +contents:raw:: + The raw data of the object. For blob and tree, it priont all object data, + its output is equivalent to `%(contents)`; For commit and tag, it not only + print objects’ messages (as `%(contents)` does), but also print objects + headers, like the "tree XXX", "parent YYY", etc lines in commits, and the + "object OOO", "type TTT", etc lines in tags. + +Note: blob and tree objects only support `%(contents)`, `%(contents:raw)` and +`%(contents:size)`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] are obtained as `trailers[:options]` (or by using the historical alias
ref-filter.c +9 -2
@@ -134,7 +134,7 @@ static struct used_atom { } remote_ref;struct {enum {C_BARE ,C_BODY ,C_BODY_DEP ,C_LENGTH ,C_LINES , -C_SIG ,C_SUB ,C_SUB_SANITIZE ,C_TRAILERS } option; +C_RAW ,C_SIG ,C_SUB ,C_SUB_SANITIZE ,C_TRAILERS } option;struct process_trailer_options trailer_opts;unsigned int nlines; } contents;
@@ -347,6 +347,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato {if (!arg) atom->u.contents.option =C_BARE ; +else if (!strcmp (arg,"raw" )) + atom->u.contents.option =C_RAW ;else if (!strcmp (arg,"body" )) atom->u.contents.option =C_BODY ;else if (!strcmp (arg,"size" ))
@@ -1387,11 +1389,16 @@ static void grab_contents(struct atom_value *val, int deref, void *buf, v->s =strbuf_detach (&s,NULL ); }else if (atom->u.contents.option ==C_BARE ) v->s =xstrdup (subpos); +else if (atom->u.contents.option ==C_RAW ) { + v->s_size = buf_size; + v->s =xmemdupz (buf, buf_size); + }break ; }case OBJ_BLOB :case OBJ_TREE : { -if (atom->u.contents.option ==C_BARE ) { +if (atom->u.contents.option ==C_BARE || + atom->u.contents.option ==C_RAW ) { v->s_size = buf_size; v->s =xmemdupz (buf, buf_size); }else if (atom->u.contents.option ==C_LENGTH )
t/t6300-for-each-ref.sh +48 -0
84 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Quoted 2 lines · ZheNing Hu via GitGitGadget
In (a2f3241: [GSOC] ref-filter: add contents:raw atom) I did not notice the
...
Before going into any of these details, remember that the cover letter is where you first sell the series to the readers. Why is it worth their time to read it? What problem does it solve in the bigger picture? Mention that we want to let "cat-file --batch" use the ref-filter --format logic, if that is the primary reason why we have these three patches, for example.
I actually do not know if a modified form of %(contents) is a good match for this feature. It was invented as a way to extract the unstructured "free text" part of structured textual objects (namely, commits and tags) so it is natural that it would not apply to trees (they are structured and there is no unstractured "free text" part) nor blobs (they are totally unstructured and does not even have to be text).
Is there another word to refer to the entire payload unadulterated?
Quoted 6 lines
git for-each-ref --format="%(contents)" --python refs/mytrees/first
will output a string processed by
python_quote_buf_with_size(), which contains'\0'. But the binary files seem to be useless after quoting. Should we allow these binary files to be output in the default way withstrbuf_add()? If so, we can remove the first patch.
The --language option is designed to be used to write a small script in the language and used like this:
git for-each-ref --format='
name=%(refname)
var=%(placeholder)
mkdir -p "$(dirname "$name")"
printf "%%s" "$var" >"$name"
' --shell | /bin/shNote that %(refname) and %(placeholder) in the --format string is not quoted at all; the "--shell" option knows how values are quoted in the host language (shell) and writes single-quotes around %(refname). If %(placeholder) produces something with a single-quote in it, that will (eh, at least "should") be quoted appropriately.
So It does not make any sense not to quote a value that comes from %(placeholder), whether it is binary or not, to match the syntax of the host language you are making the "for-each-ref --format=" to write such a script in.
So, "binary files seem to be useless after quoting" is a misunderstanding. They are useless if you do not quote them.
Thanks.
P.S. I am mostly offline today, and response will be slow.
Quoted 1 line · Junio C Hamano
Is there another word to refer to the entire payload unadulterated?
Another word better than "raw"? I don't think so: basic, crude, green, rough.
Signature · 1 line
Felipe Contreras
Quoted 7 lines · Felipe Contreras
Quoted 1 line · Junio C Hamano
Is there another word to refer to the entire payload unadulterated?
Another word better than "raw"? I don't think so: basic, crude, green, rough.
I think we should go with "raw", because the payloads we discussed here are unmodified. It is akin to "raw data" that is processed further to produce porcelain output, such as templating engine that process raw HTML into HTML pages that would be served to end user.
Signature · 1 line
An old man doll... just what I always wanted! - Clara
Quoted 6 lines · Junio C Hamano <[email protected]> 于2021年5月24日周一 上午9:09写道:
Quoted 2 lines · ZheNing Hu via GitGitGadget
In (a2f3241: [GSOC] ref-filter: add contents:raw atom) I did not notice the
...
Quoted 7 lines
Before going into any of these details, remember that the cover letter is where you first sell the series to the readers. Why is it worth their time to read it? What problem does it solve in the bigger picture? Mention that we want to let "cat-file --batch" use the ref-filter --format logic, if that is the primary reason why we have these three patches, for example.
Thanks for reminding, Indeed, as you said, this cover-letter needs to explain the patches' main purpose: let "cat-file --batch" use the ref-filter --format logic.
Quoted 8 lines
I actually do not know if a modified form of %(contents) is a good match for this feature. It was invented as a way to extract the unstructured "free text" part of structured textual objects (namely, commits and tags) so it is natural that it would not apply to trees (they are structured and there is no unstractured "free text" part) nor blobs (they are totally unstructured and does not even have to be text).
This is exactly what I worry about: Does %(contents) have good semantics to explain things like blob, tree objects which have no unstractured "free text" part or totally unstructured?
Quoted 2 lines
Is there another word to refer to the entire payload unadulterated?
If you specifically mean "raw" in %(contents:raw), I agree with Felipe Contreras or Bagas Sanjaya, "raw" seems to be enough. But if you mean we need another atom name, then "%(binary)", "%(text)" may be possible choices.
There is a good word in ref-filter.c: C_BARE, but this enum vale corresponds to %(contents). Therefore, I cannot use the name "%(contents:bare)".
Quoted 17 lines
Quoted 6 lines
git for-each-ref --format="%(contents)" --python refs/mytrees/first
will output a string processed by
python_quote_buf_with_size(), which contains'\0'. But the binary files seem to be useless after quoting. Should we allow these binary files to be output in the default way withstrbuf_add()? If so, we can remove the first patch.The --language option is designed to be used to write a small script in the language and used like this:
git for-each-ref --format=' name=%(refname) var=%(placeholder) mkdir -p "$(dirname "$name")" printf "%%s" "$var" >"$name" ' --shell | /bin/sh
Quoted 6 lines
Note that %(refname) and %(placeholder) in the --format string is not quoted at all; the "--shell" option knows how values are quoted in the host language (shell) and writes single-quotes around %(refname). If %(placeholder) produces something with a single-quote in it, that will (eh, at least "should") be quoted appropriately.
Well, now I have also noticed that --language can make these atoms be surrounded by single quotes, special characters in the output will be escaped e.g. "a'b" will turn to "'a\'b'", In this way, shell, python... can directly quote the content.
Quoted 8 lines
So It does not make any sense not to quote a value that comes from %(placeholder), whether it is binary or not, to match the syntax of the host language you are making the "for-each-ref --format=" to write such a script in.
So, "binary files seem to be useless after quoting" is a misunderstanding. They are useless if you do not quote them.
Now I agree that It seems that the content of the blob or tree is meaningful after being quoted.
Quoted 4 lines
Thanks.
P.S. I am mostly offline today, and response will be slow.
Thanks!
--
ZheNing Hu
Quoted 4 lines · Bagas Sanjaya
I think we should go with "raw", because the payloads we discussed here are unmodified. It is akin to "raw data" that is processed further to produce porcelain output, such as templating engine that process raw HTML into HTML pages that would be served to end user.
Oh, I can certainly live with --format='%(raw)'; I just view that %(contents:raw) is problematic, especially if we meant to apply it to trees and blobs, because they are not what %(contents) is about.
Thanks.
Re: [PATCH 2/3] [GSOC] ref-filter: support %(contents) for blob, tree
In reply to ZheNing Hu via GitGitGadgetref-filter.c static void append_lines(struct strbuf *out, const char *buf, unsigned long size · line 1295
Quoted 5 lines · ZheNing Hu via GitGitGadget
ref-filter.c +0
-1@@ -1292,7 +1326,8 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size }/* See grab_values */ -static void grab_sub_body_contents (struct atom_value *val,int deref,void *buf)
As I already said, I do not think %(contents) mixes well with this use for trees and blobs where you give the raw bytes, since %(contents) for commit and tag objects was never about the full byte sequence of the object. It was to give the unstructured part meant for human consumption, stripping the structured "header" part of the object.
Nevertheless, since queuing this topic breaks the build and gets in the way to find issues in other proposed regression fixes of higher importance,...
ref-filter.c static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) · line 1369
Quoted 12 lines
ref-filter.c +8
-0+static void grab_contents (struct atom_value *val,int deref,void *buf, +unsigned long buf_size,enum object_type object_type) { +switch (object_type) { +case OBJ_TAG : +case OBJ_COMMIT : { + v->s =xmemdupz (bodypos, bodylen); + else if (atom->u.contents.option == C_LENGTH) + v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
Note that this part inherits from the original and knows how to feed a value to PRIuMAX with correct cast ...
ref-filter.c static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) · line 1398
Quoted 11 lines
ref-filter.c +10
-0+ v->s =xmemdupz (bodypos, nonsiglen); + else if (atom->u.contents.option == C_SIG) + } +case OBJ_BLOB : +case OBJ_TREE : { +if (atom->u.contents.option ==C_BARE ) { + v->s_size = buf_size; + v->s =xmemdupz (buf, buf_size); + }else if (atom->u.contents.option ==C_LENGTH ) + v->s =xstrfmt ("%" PRIuMAX, buf_size);
... but this one gets sloppy, and breaks the windows-build of 'seen'. Fix is simple:
+ v->s = xstrfmt("%"PRIuMAX, (uintmax_t)buf_size);
I'll squash it in before rebuilding 'seen'.
Re: [PATCH 2/3] [GSOC] ref-filter: support %(contents) for blob, tree
Quoted 6 lines · Junio C Hamano
As I already said, I do not think %(contents) mixes well with this use for trees and blobs where you give the raw bytes, since %(contents) for commit and tag objects was never about the full byte sequence of the object. It was to give the unstructured part meant for human consumption, stripping the structured "header" part of the object.
To extend on this point a bit (even though this is not all that urgent during the prerelease freeze), conceptually, the %(content) field is understood in the larger picture like this:
+--- (the whole thing) ----------------------------------------------+ | | | +--- "header" ---------------------------------------------------+ | | | tree 678c03dca0a26afd746e8c8bb9e4aadc8bf479b1 | | | | parent 378c7c6ad48c4ccddf9b534616a0e86f28440bd3 | | | | author Junio C Hamano <[email protected]> 1621675665 +0900 | | | | committer Junio C Hamano <[email protected]> 1621675741 +0900 | | | +----------------------------------------------------------------+ | | | | +--- "contents" -------------------------------------------------+ | | | | | | | +--- "subject" ----------------------------------------------+ | | | | | Git 2.32-rc1 | | | | | +------------------------------------------------------------+ | | | | | | | | +--- "body" -------------------------------------------------+ | | | | | Signed-off-by: Junio C Hamano <[email protected]> | | | | | +------------------------------------------------------------+ | | | | | | | +----------------------------------------------------------------+ | | | +--------------------------------------------------------------------+
Even though %(header), when it is invented, would make perfect sense for commits and tags, it will never make sense for trees and blobs. Which means "contents", which is "the whole thing except for the header part", would not, either.
There is no %(placeholder) to ask for "the whole thing", and that is what you want to use for cat-file --batch if I am not mistaken, and adding one would be a good idea. There is no %(header) yet, either, but if somebody needs it for their scripts, it is clear where it fits in the picture.
Re: [PATCH 2/3] [GSOC] ref-filter: support %(contents) for blob, tree
Quoted 19 lines
To extend on this point a bit (even though this is not all that urgent during the prerelease freeze), conceptually, the %(content) field is understood in the larger picture like this:
+--- (the whole thing) ----------------------------------------------+ | | | +--- "header" ---------------------------------------------------+ | | | tree 678c03dca0a26afd746e8c8bb9e4aadc8bf479b1 | | | | parent 378c7c6ad48c4ccddf9b534616a0e86f28440bd3 | | | | author Junio C Hamano <[email protected]> 1621675665 +0900 | | | | committer Junio C Hamano <[email protected]> 1621675741 +0900 | | | +----------------------------------------------------------------+ | | | | +--- "contents" -------------------------------------------------+ | | | | | | | +--- "subject" ----------------------------------------------+ | | | | | Git 2.32-rc1 | | | | | +------------------------------------------------------------+ | |
Quoted 9 lines
| | | | | | +--- "body" -------------------------------------------------+ | | | | | Signed-off-by: Junio C Hamano <[email protected]> | | | | | +------------------------------------------------------------+ | | | | | | | +----------------------------------------------------------------+ | | | +--------------------------------------------------------------------+
Thank you for providing such a complete view. This also means that the "raw" of contents and tag will contain two parts: "header" and "contents". But for blobs and trees, they don’t have these things.
Quoted 5 lines
Even though %(header), when it is invented, would make perfect sense for commits and tags, it will never make sense for trees and blobs. Which means "contents", which is "the whole thing except for the header part", would not, either.
Although we don’t have a %(header), but in fact we already have fragments of "%(numparent)", "%(parent)" %(tree)" (see grab_commit_values()) and "%(tag)"," %(type)","%(object)" (see grab_tag_values()), but they are not obtained through the "header" part of the raw object buffer.
Quoted 6 lines
There is no %(placeholder) to ask for "the whole thing", and that is what you want to use for cat-file --batch if I am not mistaken, and adding one would be a good idea. There is no %(header) yet, either, but if somebody needs it for their scripts, it is clear where it fits in the picture.
So I don't know if adding %(header) will cause duplication of functions.
Thanks!
--
ZheNing Hu
Re: [PATCH 2/3] [GSOC] ref-filter: support %(contents) for blob, tree
Quoted 1 line · ZheNing Hu
So I don't know if adding %(header) will cause duplication of functions.
I do not think you can duplicate the feature of %(header) with other existing placeholders. You may want to dump the headers of series of commits so that you can find if there is any commit with unusal header elements, but %(tree), %(parents), etc. would by definition be the known ones, so any combination of them will not be a substitute.
But nobody is asking for it right now, and your cat-file --batch does not need it right away.
What I wanted to say in the message you are responding to was *not* that you would want to add %(header) for completeness right now. But thinking beyond your immediate need (i.e. the "whole thing") and imagining how various pieces, including the ones that do not exist yet, would fit together, would help you to avoid designing the parts you immediately need in a way you would regret in the future.
Quoted 23 lines · Junio C Hamano
Quoted 6 lines
git for-each-ref --format="%(contents)" --python refs/mytrees/first
will output a string processed by
python_quote_buf_with_size(), which contains'\0'. But the binary files seem to be useless after quoting. Should we allow these binary files to be output in the default way withstrbuf_add()? If so, we can remove the first patch.The --language option is designed to be used to write a small script in the language and used like this:
git for-each-ref --format=' name=%(refname) var=%(placeholder) mkdir -p "$(dirname "$name")" printf "%%s" "$var" >"$name" ' --shell | /bin/shNote that %(refname) and %(placeholder) in the --format string is not quoted at all; the "--shell" option knows how values are quoted in the host language (shell) and writes single-quotes around %(refname). If %(placeholder) produces something with a single-quote in it, that will (eh, at least "should") be quoted appropriately.
Quoted 7 lines
So It does not make any sense not to quote a value that comes from %(placeholder), whether it is binary or not, to match the syntax of the host language you are making the "for-each-ref --format=" to write such a script in.
So, "binary files seem to be useless after quoting" is a misunderstanding. They are useless if you do not quote them.
Another thing to keep in mind is that not all host languages may be capable of expressing a string with NUL in it. Most notably, shell. The --shell quoting rule used by for-each-ref would produce an equivalent of the "script" produced like this:
$ tr Q '\000' >script <<\EOF #!/bin/sh varname='varQname' echo "$varname" EOF
but I do not think it would say 'var' followed by a NUL followed by 'name'. The NUL is likely lost when assigned to the variable.
So for some host languages, binaries may be useless with or without quoting. But for ones that can use strings to hold arbitrary byte sequence, it should be OK to let for-each-ref to quote the byte sequence as a string literal for the language (so that the exact byte sequence will end up being in the variable after assignment).
That reminds me of another thing. The --python thing was written back when Python3 was still a distant dream and strings were the appropriate type for a random sequence of bytes (as opposed to unicode, which cannot have a random sequence of bytes). Somebody needs to check if it needs any update to work with Python3.
Quoted 6 lines
Another thing to keep in mind is that not all host languages may be capable of expressing a string with NUL in it. Most notably, shell. The --shell quoting rule used by for-each-ref would produce an equivalent of the "script" produced like this:
Quoted 9 lines
$ tr Q '\000' >script <<\EOF #!/bin/sh varname='varQname' echo "$varname" EOFbut I do not think it would say 'var' followed by a NUL followed by 'name'. The NUL is likely lost when assigned to the variable.
Yes, in the following example you mentioned earlier, I have also noticed the loss of '\0'.
Quoted 8 lines
Quoted 7 lines
git for-each-ref --format=' name=%(refname) var=%(placeholder) mkdir -p "$(dirname "$name")" printf "%%s" "$var" >"$name" ' --shell | /bin/sh
Quoted 6 lines
So for some host languages, binaries may be useless with or without quoting. But for ones that can use strings to hold arbitrary byte sequence, it should be OK to let for-each-ref to quote the byte sequence as a string literal for the language (so that the exact byte sequence will end up being in the variable after assignment).
I agree, and maybe some'\0' can be escaped appropriately to let host languages recognize it....
Quoted 5 lines
That reminds me of another thing. The --python thing was written back when Python3 was still a distant dream and strings were the appropriate type for a random sequence of bytes (as opposed to unicode, which cannot have a random sequence of bytes). Somebody needs to check if it needs any update to work with Python3.
$ printf '%b' "name='a\\\0b\\\0c'\nprint(name)" | python2.7 | od -c 0000000 a \0 b \0 c \n 0000006
$ printf '%b' "name='a\\\0b\\\0c'\necho -e \"\$name\"" | sh | od -c 0000000 a \0 b \0 c \n 0000006
In shell or python2/3, we can replace'\0' with "\\0".
In Tcl and perl, they are ok with '\0'.
$ printf '%b' "set name \"a\0b\0c\"\nputs \$name" | tclsh | od -c 0000000 ' a \0 b \0 c ' \n 0000010
$ printf '%b' "\$name = 'a\0b\0c';\n print \"\$name\"" | perl | od -c 0000000 a \0 b \0 c 0000005
So I currently think that a possible strategy is to modify python_quote_buf_with_size() and sq_quote_buf_with_size() from '\0' to "\\0".
Thanks!
--
ZheNing Hu
Quoted 3 lines · ZheNing Hu
$ printf '%b' "name='a\\\0b\\\0c'\necho -e \"\$name\"" | sh | od -c 0000000 a \0 b \0 c \n 0000006
This is wrong. In the above, the variable name does not have a NUL in it. It insead has 2-byte sequence "\" and "0" in it, and you are letting "echo -e" to convert it into binary, which is not portable at all.
I'd suggest instead to declare that some host languages, like shell, are not binary-clean and either refuse to process atoms whose values have NUL in them. Silently truncating strings at NUL or striping NULs in strings are also acceptable options if clearly documented. Claiming that we stuff binaries into variables of the host language, while not doing so and instead assigning a quoted form, is not good.
I have not thought about Python3 very much. For the purpose of most %(placeholders), it is vastly more preferrable to use str (i.e. text sequence type) as opposed to bytes, as you do not have to .decode() to use the resulting "string", but even for things like %(refname), it is not technically kosher to assume that the contents are UTF-8 encoded text, as filenames used to represent refnames are merely a sequence of bytes except NUL, but for consistency with binary gunk, we might have to emit everything as bytes. I dunno.
Quoted 1 line
In shell or python2/3, we can replace'\0' with "\\0".
Not for shell. We should declare that it is not supported to feed binary to shell.
Re: [PATCH 2/3] [GSOC] ref-filter: support %(contents) for blob, tree
In reply to Junio C HamanoQuoted 12 lines · Junio C Hamano <[email protected]> 于2021年5月26日周三 上午1:11写道:
Quoted 1 line · ZheNing Hu
So I don't know if adding %(header) will cause duplication of functions.
I do not think you can duplicate the feature of %(header) with other existing placeholders. You may want to dump the headers of series of commits so that you can find if there is any commit with unusal header elements, but %(tree), %(parents), etc. would by definition be the known ones, so any combination of them will not be a substitute.
Well, "find if there is any commit with unusal header elements" is indeed one thing that may need to be done (not now).
I tried to build "%(header)" yesterday and found some problems:
1. These atoms are too "flat":
"tree", "parent", "numparent", "object"... they are all part of the header, why not use "%(header:tree)" like "%(contents:subject)" does? Similarly, why not use "%(author:name)" instead of "%(authorname)"...
2. Why is there no state machine to save the parsing state of an object? parse_tag_buffer(), parse_commit_buffer(), they only parse part of the content of the object. E.g. tag parsed "object", "type", "tag" and "taggerdate" in parse_tag_buffer(), but it did not save the starting position and length of the parsed fields, which caused grab_person(), grab_date() to parse the object again and again. I think this may be an optimization point.
Quoted 3 lines
But nobody is asking for it right now, and your cat-file --batch does not need it right away.
Or we only provide the simplest "%(header)" and "%(header:size)" (it can be easily supported with "%(raw)"), leaving the other feature to the future.
Quoted 6 lines
What I wanted to say in the message you are responding to was *not* that you would want to add %(header) for completeness right now. But thinking beyond your immediate need (i.e. the "whole thing") and imagining how various pieces, including the ones that do not exist yet, would fit together, would help you to avoid designing the parts you immediately need in a way you would regret in the future.
As I said before, many atomic designs are not very regular. Our ultimate goal may be we can simultaneously support:
"%(raw)"
"%(raw:header)"
"%(raw:header:tagger)"
"%(raw:header:tagger:eamil)"
"%(raw:header:taggereamil)"
"%(header:taggereamil)"
"%(header:tagger:eamil)"
"%(taggereamil)"
"%(tagger:eamil)"
...
Each layer is a namespace. Of course, it may take a long distance to support this.
Thank.
--
ZheNing Hu
Quoted 12 lines · Junio C Hamano <[email protected]> 于2021年5月26日周三 下午3:06写道:
Quoted 3 lines · ZheNing Hu
$ printf '%b' "name='a\\\0b\\\0c'\necho -e \"\$name\"" | sh | od -c 0000000 a \0 b \0 c \n 0000006This is wrong. In the above, the variable name does not have a NUL in it. It insead has 2-byte sequence "\" and "0" in it, and you are letting "echo -e" to convert it into binary, which is not portable at all.
Indeed I was wrong, the var name does not contain '\0'.
Quoted 7 lines
I'd suggest instead to declare that some host languages, like shell, are not binary-clean and either refuse to process atoms whose values have NUL in them. Silently truncating strings at NUL or striping NULs in strings are also acceptable options if clearly documented. Claiming that we stuff binaries into variables of the host language, while not doing so and instead assigning a quoted form, is not good.
Makes sense. Either choose to truncate, or choose to reject.
Quoted 9 lines
I have not thought about Python3 very much. For the purpose of most %(placeholders), it is vastly more preferrable to use str (i.e. text sequence type) as opposed to bytes, as you do not have to .decode() to use the resulting "string", but even for things like %(refname), it is not technically kosher to assume that the contents are UTF-8 encoded text, as filenames used to represent refnames are merely a sequence of bytes except NUL, but for consistency with binary gunk, we might have to emit everything as bytes. I dunno.
Quoted 4 lines
Quoted 1 line
In shell or python2/3, we can replace'\0' with "\\0".
Not for shell. We should declare that it is not supported to feed binary to shell.
Eh, it seems that we adopt a "reject" strategy.
$ git hash-object a.out -w | xargs git update-ref refs/myblobs/aoutblob $ git for-each-ref --format="name=%(raw)" refs/myblobs/aoutblob --python | python2 File "<stdin>", line 1 SyntaxError: Non-ASCII character '\x8b' in file <stdin> on line 2, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details
$ git for-each-ref --format="name=%(raw)" refs/myblobs/aoutblob --python |python3 SyntaxError: Non-UTF-8 code starting with '\x8b' in file <stdin> on line 2, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details
It seems that --python also needs to "reject", no matter python2 or python3. What about tcl and perl?
$ cat a.out | od >1.od $ git for-each-ref --format="set name %(raw) puts -nonewline \$name" refs/myblobs/aoutblob --tcl | tclsh | od > 2.od $ diff 1.od 2.od | head 7,12c7,12 < 0000140 114303 000002 000000 000000 141400 001230 000000 000000 < 0000160 000000 000010 000000 000000 000000 000003 000000 000004 < 0000200 000000 001430 000000 000000 000000 001430 000000 000000 < 0000220 000000 001430 000000 000000 000000 000034 000000 000000 < 0000240 000000 000034 000000 000000 000000 000001 000000 000000 < 0000260 000000 000001 000000 000004 000000 000000 000000 000000 ---
Quoted 6 lines
0000140 001330 000000 000000 000000 001330 000000 000000 000000 0000160 000010 000000 000000 000000 000003 000000 000004 000000 0000200 001430 000000 000000 000000 001430 000000 000000 000000 0000220 001430 000000 000000 000000 000034 000000 000000 000000 0000240 000034 000000 000000 000000 000001 000000 000000 000000 0000260 000001 000000 000004 000000 000000 000000 000000 000000
It seems that a.out contents passed into tcl and then the output is very different...
But,
$ cat a.out | od >1.od $ git for-each-ref --format="\$name= %(raw); print \"\$name\"" refs/myblobs/aoutblob --perl | perl | od >6.od $ diff 1.od 2.od
There was no error this time, so for perl, it's ok... The "binary security" we care about is currently only complied with by the Perl language.
So I think we better reject them all languages together for normative. The clear definition of this rejection strategy is that %(raw) and --language cannot be used at the same time. If our binary data is passed to a variable in the host language, there may be escape errors for the host language.
Thanks.
--
ZheNing Hu