ref-filter: add %(raw) atom

ref-filter.c +0 -1

@@ -1307,10 +1349,22 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			continue;
 		if (deref)
 			name++;
-		if (strcmp(name, "body") &&

static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b) {

ref-filter.c +17 -4

 	struct atom_value *va, *vb;

@@ -2304,6 +2382,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 	int cmp_detached_head = 0;
 	cmp_type cmp_type = used_atom[s->atom].type;
 	struct strbuf err = STRBUF_INIT;
+	size_t slen = 0;
 
 	if (get_ref_atom_value(a, s->atom, &va, &err))
 		die("%s", err.buf);

@@ -2317,10 +2396,32 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 	} else if (s->sort_flags & REF_SORTING_VERSION) {
 		cmp = versioncmp(va->s, vb->s);
 	} else if (cmp_type == FIELD_STR) {
-		int (*cmp_fn)(const char *, const char *);
-		cmp_fn = s->sort_flags & REF_SORTING_ICASE
-			? strcasecmp : strcmp;
-		cmp = cmp_fn(va->s, vb->s);
+		if (va->s_size == ATOM_VALUE_S_SIZE_INIT &&
+		    vb->s_size == ATOM_VALUE_S_SIZE_INIT) {
+			int (*cmp_fn)(const char *, const char *);
+			cmp_fn = s->sort_flags & REF_SORTING_ICASE
+				? strcasecmp : strcmp;
+			cmp = cmp_fn(va->s, vb->s);
+		} else {
+			int (*cmp_fn)(const void *, const void *, size_t);
+			cmp_fn = s->sort_flags & REF_SORTING_ICASE
+				? memcasecmp : memcmp;
+
+			if (va->s_size != ATOM_VALUE_S_SIZE_INIT &&
+			    vb->s_size != ATOM_VALUE_S_SIZE_INIT) {
+				cmp = cmp_fn(va->s, vb->s, va->s_size > vb->s_size ?
+				       vb->s_size : va->s_size);
+			} else if (va->s_size == ATOM_VALUE_S_SIZE_INIT) {

ref-filter.c static int contents_atom_parser(const struct ref_format *format, struct used_ato · line 381

Quoted 6 lines · ZheNing Hu via GitGitGadget

ref-filter.c +6 -0

+static int raw_atom_parser(const struct ref_format *format, struct used_atom *atom,
+                             const char *arg, struct strbuf *err)
+{
+     if (!arg) {
+             atom->u.raw_data.option = RAW_BARE;
+     } else if (!strcmp(arg, "size"))

No need for braces.

if (!arg)

    ...

else

Quoted 10 lines

ref-filter.c +2 -4

@@ -1307,10 +1349,22 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
                      continue;
              if (deref)
                      name++;
-             if (strcmp(name, "body") &&
-                 !starts_with(name, "subject") &&
-                 !starts_with(name, "trailers") &&
-                 !starts_with(name, "contents"))
+
+             if (starts_with(name, "raw")) {

ref-filter.c static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) · line 1358

Quoted 5 lines

ref-filter.c +5 -0

+                     if (atom->u.raw_data.option == RAW_BARE) {
+                             v->s = xmemdupz(buf, buf_size);
+                             v->s_size = buf_size;
+                     } else if (atom->u.raw_data.option == RAW_LENGTH)
+                             v->s = xstrfmt("%"PRIuMAX, (uintmax_t)buf_size);

I think it's better to be consistent: if you used braces in the if, uses braces in else.

ref-filter.c static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)

Quoted 4 lines

ref-filter.c +3 -0

+                     continue;
+             }
 
+static int memcasecmp(const void *vs1, const void *vs2, size_t n)

Why void *? We can delcare as char *.

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2363

Quoted 4 lines

ref-filter.c +4 -0

+{
+     size_t i;
+     const char *s1 = (const char *)vs1;
+     const char *s2 = (const char *)vs2;

Then we avoid this extra step.

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2367

Quoted 3 lines

ref-filter.c +3 -0

+     for (i = 0; i < n; i++) {
+             unsigned char u1 = s1[i];
+             unsigned char u2 = s2[i];

There's no need for two entirely new variables...

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2369

Quoted 2 lines

ref-filter.c +2 -0

+             int U1 = toupper (u1);
+             int U2 = toupper (u2);

You can do toupper(s1[i]) directly (BTW, there's an extra space: foo(x), not foo (x)).

While we are at it, why keep an extra index from s1, when s1 is never used again?

We can simply advance both s1 and s2:

s1++, s2++

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2371

Quoted 2 lines

ref-filter.c +2 -0

+             int diff = (UCHAR_MAX <= INT_MAX ? U1 - U2
+                     : U1 < U2 ? -1 : U2 < U1);

I don't understand what this is supposed to achieve. Both U1 and U2 are integers, pretty low integers actually.

If we get rid if that complexity we don't even need U1 or U2, just do:

  diff = toupper(u1) - toupper(u2);

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2376

Quoted 5 lines

ref-filter.c +5 -0

+             if (diff)
+                     return diff;
+     }
+     return 0;
+}

All we have to do is define the end point, and then we don't need i:

        static int memcasecmp(const char *s1, const char *s2, size_t n)
        {
                const char *end = s1 + n;
                for (; s1 < end; s1++, s2++) {
                        int diff = tolower(*s1) - tolower(*s2);
                        if (diff)
                                return diff;
                }
                return 0;
        }

(and I personally prefer lower to upper)

Check the following resource for a detailed explanation of why my modified version is considered good taste:

https://github.com/felipec/linked-list-good-taste

Quoted 40 lines

static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)

 {
      struct atom_value *va, *vb;

ref-filter.c +22 -4

@@ -2304,6 +2382,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
      int cmp_detached_head = 0;
      cmp_type cmp_type = used_atom[s->atom].type;
      struct strbuf err = STRBUF_INIT;
+     size_t slen = 0;
 
      if (get_ref_atom_value(a, s->atom, &va, &err))
              die("%s", err.buf);

@@ -2317,10 +2396,32 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
      } else if (s->sort_flags & REF_SORTING_VERSION) {
              cmp = versioncmp(va->s, vb->s);
      } else if (cmp_type == FIELD_STR) {
-             int (*cmp_fn)(const char *, const char *);
-             cmp_fn = s->sort_flags & REF_SORTING_ICASE
-                     ? strcasecmp : strcmp;
-             cmp = cmp_fn(va->s, vb->s);
+             if (va->s_size == ATOM_VALUE_S_SIZE_INIT &&
+                 vb->s_size == ATOM_VALUE_S_SIZE_INIT) {
+                     int (*cmp_fn)(const char *, const char *);
+                     cmp_fn = s->sort_flags & REF_SORTING_ICASE
+                             ? strcasecmp : strcmp;
+                     cmp = cmp_fn(va->s, vb->s);
+             } else {
+                     int (*cmp_fn)(const void *, const void *, size_t);
+                     cmp_fn = s->sort_flags & REF_SORTING_ICASE
+                             ? memcasecmp : memcmp;
+
+                     if (va->s_size != ATOM_VALUE_S_SIZE_INIT &&
+                         vb->s_size != ATOM_VALUE_S_SIZE_INIT) {
+                             cmp = cmp_fn(va->s, vb->s, va->s_size > vb->s_size ?
+                                    vb->s_size : va->s_size);
+                     } else if (va->s_size == ATOM_VALUE_S_SIZE_INIT) {
+                             slen = strlen(va->s);
+                             cmp = cmp_fn(va->s, vb->s, slen > vb->s_size ?
+                                          vb->s_size : slen);
+                     } else {
+                             slen = strlen(vb->s);

ref-filter.c static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru · line 2424

Quoted 5 lines

ref-filter.c +5 -0

+                             cmp = cmp_fn(va->s, vb->s, slen > va->s_size ?
+                                          slen : va->s_size);
+                     }
+                     cmp = cmp ? cmp : va->s_size - vb->s_size;
+             }

This hurts my eyes. I think the complexity of this chunk warrants a separate function. Then the logic would be easer to see.

Cheers.

--
Felipe Contreras

Sorry for the weird, unclean memcasecmp(), I referred to memcmp() in glibc before, and then I was afraid that my writing was not standard enough like "UCHAR_MAX <= INT_MAX", I can't consider such an extreme situation. So I copied it directly from gnulib:
https://github.com/gagern/gnulib/blob/master/lib/memcasecmp.c

Quoted 4 lines

Check the following resource for a detailed explanation of why my modified version is considered good taste:

https://github.com/felipec/linked-list-good-taste

OK. I will gradually standardize my code style.

Quoted 5 lines · ZheNing Hu

Sorry for the weird, unclean memcasecmp(), I referred to memcmp() in glibc before, and then I was afraid that my writing was not standard enough like "UCHAR_MAX <= INT_MAX", I can't consider such an extreme situation. So I copied it directly from gnulib:
https://github.com/gagern/gnulib/blob/master/lib/memcasecmp.c

Yeah, I imagined you copied it from somewhere, but when you do that you need to transform the code to the style of the project. I've seen GNU code, and in my opinion it's too verbose and redundant. Not a good style.

But more importantly: at the header of that file you can see the license is GPLv3, that's incompatible with the license of this project, which is GPLv2 only (see the note in COPYING).

You can't just copy code like that. You need to be careful.

And if you do copy code--even if allowed by the license--it's something that should be mentioned in the commit message, preferably with a link to the original, that way if there's trouble in the future with that code, we can follow the link and figure out why it was done that way.

Also, it's just nice to give attribution to the people that wrote the original code.

Quoted 5 lines

Quoted 4 lines

Check the following resource for a detailed explanation of why my modified version is considered good taste:

https://github.com/felipec/linked-list-good-taste

Quoted 1 line

OK. I will gradually standardize my code style.

It is not a standard, it is my personal opinion, which is shared by Linus Torvalds, and I presume other members of the Git project.

The style is not something that can be standardized, you get a feeling of it as you read more code of the project, write, and then receive feedback on what you write.

It's like learning the slang of a new city; it takes a while.

Cheers.

--
Felipe Contreras

ZheNing Hu via GitGitGadget wrote:
[...]

Quoted 1 line

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)

Why void *? We can delcare as char *.

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2363

Quoted 4 lines

ref-filter.c +4 -0

+{
+	size_t i;
+	const char *s1 = (const char *)vs1;
+	const char *s2 = (const char *)vs2;

Then we avoid this extra step.

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2367

Quoted 3 lines

ref-filter.c +3 -0

+	for (i = 0; i < n; i++) {
+		unsigned char u1 = s1[i];
+		unsigned char u2 = s2[i];

There's no need for two entirely new variables...

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2369

Quoted 2 lines

ref-filter.c +2 -0

+		int U1 = toupper (u1);
+		int U2 = toupper (u2);

You can do toupper(s1[i]) directly (BTW, there's an extra space: foo(x), not foo (x)).

While we are at it, why keep an extra index from s1, when s1 is never used again?

We can simply advance both s1 and s2:

   s1++, s2++

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2371

Quoted 2 lines

ref-filter.c +2 -0

+		int diff = (UCHAR_MAX <= INT_MAX ? U1 - U2
+			: U1 < U2 ? -1 : U2 < U1);

I don't understand what this is supposed to achieve. Both U1 and U2 are integers, pretty low integers actually.

If we get rid if that complexity we don't even need U1 or U2, just do:

   diff = toupper(u1) - toupper(u2);

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2376

Quoted 5 lines

ref-filter.c +5 -0

+		if (diff)
+			return diff;
+	}
+	return 0;
+}

All we have to do is define the end point, and then we don't need i:

	static int memcasecmp(const char *s1, const char *s2, size_t n)
	{
		const char *end = s1 + n;
		for (; s1 < end; s1++, s2++) {
			int diff = tolower(*s1) - tolower(*s2);
			if (diff)
				return diff;
		}
		return 0;
	}

(and I personally prefer lower to upper)

Check the following resource for a detailed explanation of why my modified version is considered good taste:

https://github.com/felipec/linked-list-good-taste

Quoted 35 lines

  static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)
  {
  	struct atom_value *va, *vb;

ref-filter.c +17 -4

@@ -2304,6 +2382,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
  	int cmp_detached_head = 0;
  	cmp_type cmp_type = used_atom[s->atom].type;
  	struct strbuf err = STRBUF_INIT;
+	size_t slen = 0;
 
  	if (get_ref_atom_value(a, s->atom, &va, &err))
  		die("%s", err.buf);

@@ -2317,10 +2396,32 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
  	} else if (s->sort_flags & REF_SORTING_VERSION) {
  		cmp = versioncmp(va->s, vb->s);
  	} else if (cmp_type == FIELD_STR) {
-		int (*cmp_fn)(const char *, const char *);
-		cmp_fn = s->sort_flags & REF_SORTING_ICASE
-			? strcasecmp : strcmp;
-		cmp = cmp_fn(va->s, vb->s);
+		if (va->s_size == ATOM_VALUE_S_SIZE_INIT &&
+		    vb->s_size == ATOM_VALUE_S_SIZE_INIT) {
+			int (*cmp_fn)(const char *, const char *);
+			cmp_fn = s->sort_flags & REF_SORTING_ICASE
+				? strcasecmp : strcmp;
+			cmp = cmp_fn(va->s, vb->s);
+		} else {
+			int (*cmp_fn)(const void *, const void *, size_t);
+			cmp_fn = s->sort_flags & REF_SORTING_ICASE
+				? memcasecmp : memcmp;
+
+			if (va->s_size != ATOM_VALUE_S_SIZE_INIT &&
+			    vb->s_size != ATOM_VALUE_S_SIZE_INIT) {
+				cmp = cmp_fn(va->s, vb->s, va->s_size > vb->s_size ?
+				       vb->s_size : va->s_size);
+			} else if (va->s_size == ATOM_VALUE_S_SIZE_INIT) {

ref-filter.c static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru · line 2424

Quoted 10 lines

ref-filter.c +10 -0

+				slen = strlen(va->s);
+				cmp = cmp_fn(va->s, vb->s, slen > vb->s_size ?
+					     vb->s_size : slen);
+			} else {
+				slen = strlen(vb->s);
+				cmp = cmp_fn(va->s, vb->s, slen > va->s_size ?
+					     slen : va->s_size);
+			}
+			cmp = cmp ? cmp : va->s_size - vb->s_size;
+		}

This hurts my eyes. I think the complexity of this chunk warrants a separate function. Then the logic would be easer to see.

Cheers.

Quoted 5 lines · Felipe Contreras

ZheNing Hu via GitGitGadget wrote:
[...]

Quoted 1 line

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)

Why void *? We can delcare as char *.

If you look at how this function is used you'll see

	int (*cmp_fn)(const void *, const void *, size_t);
	cmp_fn = s->sort_flags & REF_SORTING_ICASE
			? memcasecmp : memcmp;

Quoted 1 line

(and I personally prefer lower to upper)

We should be using tolower() as that is what POSIX specifies for strcasecmp() [1] which we are trying to emulate and there are cases[2] where

	(tolower(c1) == tolower(c2)) != (toupper(c1) == toupper(c2))

Quoted 11 lines · Phillip Wood

Quoted 5 lines · Felipe Contreras

ZheNing Hu via GitGitGadget wrote:
[...]

Quoted 1 line

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)

Why void *? We can delcare as char *.

If you look at how this function is used you'll see

	int (*cmp_fn)(const void *, const void *, size_t);
	cmp_fn = s->sort_flags & REF_SORTING_ICASE
			? memcasecmp : memcmp;

Yeah, but why?

We know we are comparing two char *. Presumably the reason is that memcmp and memcasecmp use void *, but that could be remedied with:

	cmp_fn = (int (*)(const char *, const char *, size_t))memcmp;

That way the same cmp_fn could be used for the two cases.

Either way I don't care particularly much. It also could be possible to use void * and do the casting in tolower().

Quoted 5 lines

Quoted 1 line

(and I personally prefer lower to upper)

We should be using tolower() as that is what POSIX specifies for strcasecmp() [1] which we are trying to emulate and there are cases[2] where

	(tolower(c1) == tolower(c2)) != (toupper(c1) == toupper(c2))

That's true.

Quoted 11 lines · Phillip Wood

Quoted 5 lines · Felipe Contreras

ZheNing Hu via GitGitGadget wrote:
[...]

Quoted 1 line

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)

Why void *? We can delcare as char *.

If you look at how this function is used you'll see

      int (*cmp_fn)(const void *, const void *, size_t);
      cmp_fn = s->sort_flags & REF_SORTING_ICASE
                      ? memcasecmp : memcmp;

Yeah, but why?

We know we are comparing two char *. Presumably the reason is that memcmp and memcasecmp use void *, but that could be remedied with:

        cmp_fn = (int (*)(const char *, const char *, size_t))memcmp;

That way the same cmp_fn could be used for the two cases.

Either way I don't care particularly much. It also could be possible to use void * and do the casting in tolower().

Quoted 5 lines

Quoted 1 line

(and I personally prefer lower to upper)

We should be using tolower() as that is what POSIX specifies for strcasecmp() [1] which we are trying to emulate and there are cases[2] where

      (tolower(c1) == tolower(c2)) != (toupper(c1) == toupper(c2))

That's true.

--
Felipe Contreras

Quoted 19 lines · Felipe Contreras <[email protected]> 于2021年5月29日周六 下午11:24写道：

Quoted 11 lines · Phillip Wood

Quoted 5 lines · Felipe Contreras

ZheNing Hu via GitGitGadget wrote:
[...]

Quoted 1 line

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)

Why void *? We can delcare as char *.

If you look at how this function is used you'll see

       int (*cmp_fn)(const void *, const void *, size_t);
       cmp_fn = s->sort_flags & REF_SORTING_ICASE
                       ? memcasecmp : memcmp;

Yeah, but why?

We know we are comparing two char *. Presumably the reason is that memcmp and memcasecmp use void *, but that could be remedied with:

Quoted 7 lines

         cmp_fn = (int (*)(const char *, const char *, size_t))memcmp;

That way the same cmp_fn could be used for the two cases.

Either way I don't care particularly much. It also could be possible to use void * and do the casting in tolower().

I agree with Phillip's point of view here:
It would be better for memcasecmp and memcmp to be consistent.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

(and I personally prefer lower to upper)

We should be using tolower() as that is what POSIX specifies for strcasecmp() [1] which we are trying to emulate and there are cases[2] where

       (tolower(c1) == tolower(c2)) != (toupper(c1) == toupper(c2))

That's true.

How about something like this:

static int memcasecmp(const void *vs1, const void *vs2, size_t n) {

(quoted diff) +0 -3

-       size_t i;
-       const char *s1 = (const char *)vs1;
-       const char *s2 = (const char *)vs2;

(quoted diff) +4 -0

+       const char *end = s1 + n;
+
+       for (; s1 < end; s1++, s2++) {
+               int diff = tolower(*s1) - tolower(*s2);
                 if (diff)
                         return diff;
         }

}

Quoted 2 lines

--
Felipe Contreras

Thanks.
--
ZheNing Hu

Quoted 6 lines

static int memcasecmp(const void *vs1, const void *vs2, size_t n) {

(quoted diff) +0 -3

-       size_t i;
-       const char *s1 = (const char *)vs1;
-       const char *s2 = (const char *)vs2;

Review

Quoted 10 lines

(quoted diff) +2 -8

-
-       for (i = 0; i < n; i++) {
-               unsigned char u1 = s1[i];
-               unsigned char u2 = s2[i];
-               int U1 = toupper (u1);
-               int U2 = toupper (u2);
-               int diff = (UCHAR_MAX <= INT_MAX ? U1 - U2
-                       : U1 < U2 ? -1 : U2 < U1);
+       const char *s1 = (const void *)vs1;
+       const char *s2 = (const void *)vs2;

I think the new version looks fine apart from these casts. vs1 declared as 'const void *' in the function signature so this cast does not do anything. You could cast using (const char *) instead if you wanted but that is not required as you can assign a 'const void *' to 'const whatever *' without a cast.

Best Wishes

Phillip

Quoted 21 lines · Felipe Contreras <[email protected]> 于2021年5月29日周六 下午11:24写道：

Quoted 11 lines · Phillip Wood

Quoted 5 lines · Felipe Contreras

ZheNing Hu via GitGitGadget wrote:
[...]

Quoted 1 line

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)

Why void *? We can delcare as char *.

If you look at how this function is used you'll see

      int (*cmp_fn)(const void *, const void *, size_t);
      cmp_fn = s->sort_flags & REF_SORTING_ICASE
                      ? memcasecmp : memcmp;

Yeah, but why?

We know we are comparing two char *. Presumably the reason is that memcmp and memcasecmp use void *, but that could be remedied with:

        cmp_fn = (int (*)(const char *, const char *, size_t))memcmp;

Quoted 5 lines

That way the same cmp_fn could be used for the two cases.

Either way I don't care particularly much. It also could be possible to use void * and do the casting in tolower().

I agree with Phillip's point of view here:
It would be better for memcasecmp and memcmp to be consistent.

static int memcasecmp(const void *vs1, const void *vs2, size_t n) {

(quoted diff) +0 -3

-       size_t i;
-       const char *s1 = (const char *)vs1;
-       const char *s2 = (const char *)vs2;

Quoted 5 lines · Felipe Contreras

ZheNing Hu via GitGitGadget wrote:
[...]

Quoted 1 line

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)

Why void *? We can delcare as char *.

If you look at how this function is used you'll see

        int (*cmp_fn)(const void *, const void *, size_t);
        cmp_fn = s->sort_flags & REF_SORTING_ICASE
                        ? memcasecmp : memcmp;

So the signature must match memcmp to avoid undefined behavior (a ternary expression is undefined unless both sides evaluate to the same type and calling a function through a pointer a different type is undefined as well)

Quoted 55 lines

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2363

Quoted 4 lines

ref-filter.c +4 -0

+{
+    size_t i;
+    const char *s1 = (const char *)vs1;
+    const char *s2 = (const char *)vs2;

Then we avoid this extra step.

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2367

Quoted 3 lines

ref-filter.c +3 -0

+    for (i = 0; i < n; i++) {
+            unsigned char u1 = s1[i];
+            unsigned char u2 = s2[i];

There's no need for two entirely new variables...

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2369

Quoted 2 lines

ref-filter.c +2 -0

+            int U1 = toupper (u1);
+            int U2 = toupper (u2);

You can do toupper(s1[i]) directly (BTW, there's an extra space: foo(x), not foo (x)).

While we are at it, why keep an extra index from s1, when s1 is never used again?

We can simply advance both s1 and s2:

   s1++, s2++

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2371

Quoted 2 lines

ref-filter.c +2 -0

+            int diff = (UCHAR_MAX <= INT_MAX ? U1 - U2
+                    : U1 < U2 ? -1 : U2 < U1);

I don't understand what this is supposed to achieve. Both U1 and U2 are integers, pretty low integers actually.

If we get rid if that complexity we don't even need U1 or U2, just do:

   diff = toupper(u1) - toupper(u2);

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2376

Quoted 5 lines

ref-filter.c +5 -0

+            if (diff)
+                    return diff;
+    }
+    return 0;
+}

All we have to do is define the end point, and then we don't need i:

      static int memcasecmp(const char *s1, const char *s2, size_t n)
      {
              const char *end = s1 + n;
              for (; s1 < end; s1++, s2++) {
                      int diff = tolower(*s1) - tolower(*s2);
                      if (diff)
                              return diff;
              }
              return 0;
      }

Quoted 1 line

(and I personally prefer lower to upper)

We should be using tolower() as that is what POSIX specifies for strcasecmp() [1] which we are trying to emulate and there are cases[2] where

        (tolower(c1) == tolower(c2)) != (toupper(c1) == toupper(c2))

Best Wishes

Phillip

[1] https://pubs.opengroup.org/onlinepubs/9699919799/
[2] https://en.wikipedia.org/wiki/Dotted_and_dotless_I#In_computing

Quoted 17 lines · Phillip Wood <[email protected]> 于2021年5月29日周六 下午9:23写道：

Quoted 15 lines · Felipe Contreras

ZheNing Hu via GitGitGadget wrote:
[...]
All we have to do is define the end point, and then we don't need i:

       static int memcasecmp(const char *s1, const char *s2, size_t n)
       {
               const char *end = s1 + n;
               for (; s1 < end; s1++, s2++) {
                       int diff = tolower(*s1) - tolower(*s2);
                       if (diff)
                               return diff;
               }
               return 0;
       }

Quoted 6 lines

Quoted 1 line

(and I personally prefer lower to upper)

We should be using tolower() as that is what POSIX specifies for strcasecmp() [1] which we are trying to emulate and there are cases[2] where

         (tolower(c1) == tolower(c2)) != (toupper(c1) == toupper(c2))

I don’t know if we overlooked a fact: This static memcasecmp() is not a POSIX version. tolower() or toupper() are in git-compat-util.h, sane_istest('\0', GIT_ALPHA) == false . So in sane_case(), whatever tolower(), toupper(), they just return '\0' itself.

Quoted 7 lines

Best Wishes

Phillip

[1] https://pubs.opengroup.org/onlinepubs/9699919799/
[2] https://en.wikipedia.org/wiki/Dotted_and_dotless_I#In_computing

Thanks.
--
ZhenNing Hu

The raw data of blob, tree objects may contain '\0', but most of the logic in ref-filter depands on the output of the atom being a structured string (end with '\0').

E.g. quote_formatting() use strbuf_addstr() or *._quote_buf() add the data to the buffer. The raw data 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, which can record raw object size, it can help us add raw object data to the buffer or compare two buffers which contain raw object data.

Beyond, --format=%(raw) should not combine with --python, --shell, --tcl, --perl because if our binary raw data is passed to a variable in the host language, the host languages may cause escape errors.

Documentation/git-for-each-ref.txt +1 -0

@@ -235,6 +235,20 @@ 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 raw data in a object is `raw`, For commit and tag objects, `raw` contain

Documentation/git-for-each-ref.txt +3 -0

+`--perl` because if our binary raw data is passed to a variable in the host language,
+the host languages may cause escape errors.
+
 The message in a commit or a tag object is `contents`, from which
 `contents:<part>` can be used to extract various parts out of:
 

ref-filter.c +16 -0

index 97116e12d7c4..c2abf5da7006 100644
@@ -138,6 +138,9 @@ static struct used_atom {
 			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
+		struct {
+			enum { RAW_BARE, RAW_LENGTH } option;
+		} raw_data;
 		struct {
 			cmp_status cmp_status;
 			const char *str;

@@ -370,6 +373,18 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
 	return 0;
 }
 
+static int raw_atom_parser(const struct ref_format *format, struct used_atom *atom,
+				const char *arg, struct strbuf *err)
+{
+	if (!arg) {
+		atom->u.raw_data.option = RAW_BARE;
+	} else if (!strcmp(arg, "size"))
+		atom->u.raw_data.option = RAW_LENGTH;
+	else
+		return strbuf_addf_ret(err, -1, _("unrecognized %%(raw) argument: %s"), arg);
+	return 0;
+}
+
 static int oid_atom_parser(const struct ref_format *format, struct used_atom *atom,
 			   const char *arg, struct strbuf *err)
 {

@@ -530,6 +545,7 @@ static struct {
 	{ "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
 	{ "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
 	{ "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+	{ "raw", SOURCE_OBJ, FIELD_STR, raw_atom_parser },
 	{ "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
 	{ "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
 	{ "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },

ref-filter.c +2 -0

 	int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
 		       struct strbuf *err);
 	uintmax_t value; /* used for sorting when not FIELD_STR */
 	struct used_atom *atom;
 };
 
+#define ATOM_VALUE_S_SIZE_INIT (-1)
+
 /*
  * Used to parse format string and sort specifiers
  */

ref-filter.c +0 -0

@@ -652,11 +675,14 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	return at;
 }
 

ref-filter.c +6 -6

@@ -683,9 +709,12 @@ static int append_atom(struct atom_value *v, struct ref_formatting_state *state,
 	 * encountered.
 	 */
 	if (!state->stack->prev)
-		quote_formatting(&state->stack->output, v->s, state->quote_style);
+		quote_formatting(&state->stack->output, v->s, v->s_size, state->quote_style);
 	else
-		strbuf_addstr(&state->stack->output, v->s);
+		if (v->s_size != ATOM_VALUE_S_SIZE_INIT)
+			strbuf_add(&state->stack->output, v->s, v->s_size);
+		else
+			strbuf_addstr(&state->stack->output, v->s);
 	return 0;
 }
 

@@ -785,14 +814,16 @@ static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state
 	return 0;
 }
 
-static int is_empty(const char *s)
+static int is_empty(struct strbuf *buf)
 {
-	while (*s != '\0') {
-		if (!isspace(*s))
-			return 0;

ref-filter.c +3 -0

@@ -800,6 +831,7 @@ static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
 {
 	struct ref_formatting_stack *cur = state->stack;
 	struct if_then_else *if_then_else = NULL;
+	size_t str_len = 0;
 
 	if (cur->at_end == if_then_else_handler)
 		if_then_else = (struct if_then_else *)cur->at_end_data;

@@ -810,18 +842,28 @@ static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
 	if (if_then_else->else_atom_seen)
 		return strbuf_addf_ret(err, -1, _("format: %%(then) atom used after %%(else)"));
 	if_then_else->then_atom_seen = 1;
+	if (if_then_else->str)
+		str_len = strlen(if_then_else->str);

ref-filter.c +6 -5

+		if (str_len == cur->output.len &&
+		    !memcmp(if_then_else->str, cur->output.buf, cur->output.len))
 			if_then_else->condition_satisfied = 1;
 	} else if (if_then_else->cmp_status == COMPARE_UNEQUAL) {
-		if (strcmp(if_then_else->str, cur->output.buf))
+		if (!if_then_else->str)
+			BUG("when if_then_else->cmp_status == COMPARE_UNEQUAL,"
+			    "if_then_else->str must not be null");
 
 
 
 /* See grab_values */
-static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
+static void grab_raw_data(struct atom_value *val, int deref, void *buf, unsigned long buf_size, struct object *obj)
 {
 	int i;
 	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;

@@ -1307,10 +1349,22 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			continue;
 		if (deref)
 			name++;
-		if (strcmp(name, "body") &&
-		    !starts_with(name, "subject") &&
-		    !starts_with(name, "trailers") &&

ref-filter.c +4 -1

+		    (strcmp(name, "body") &&
+		     !starts_with(name, "subject") &&
+		     !starts_with(name, "trailers") &&
+		     !starts_with(name, "contents")))
 			continue;
 		if (!subpos)
 			find_subpos(buf,

@@ -1374,25 +1428,30 @@ static void fill_missing_values(struct atom_value *val)
  * pointed at by the ref itself; otherwise it is the object the
  * ref (which is a tag) refers to.
  */
-static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf)

ref-filter.c +6 -4

+		if (diff)
+			return diff;
+	}
+	return 0;
+}

@@ -2304,6 +2382,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 	int cmp_detached_head = 0;
 	cmp_type cmp_type = used_atom[s->atom].type;
 	struct strbuf err = STRBUF_INIT;
+	size_t slen = 0;
 
 	if (get_ref_atom_value(a, s->atom, &va, &err))
 		die("%s", err.buf);

@@ -2317,10 +2396,32 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 	} else if (s->sort_flags & REF_SORTING_VERSION) {
 		cmp = versioncmp(va->s, vb->s);
 	} else if (cmp_type == FIELD_STR) {
-		int (*cmp_fn)(const char *, const char *);
-		cmp_fn = s->sort_flags & REF_SORTING_ICASE
-			? strcasecmp : strcmp;
-		cmp = cmp_fn(va->s, vb->s);
 

ref-filter.c +8 -0

+			if (va->s_size != ATOM_VALUE_S_SIZE_INIT &&
+			    vb->s_size != ATOM_VALUE_S_SIZE_INIT) {
+				cmp = cmp_fn(va->s, vb->s, va->s_size > vb->s_size ?
+				       vb->s_size : va->s_size);
+			} else if (va->s_size == ATOM_VALUE_S_SIZE_INIT) {
+				slen = strlen(va->s);
+				cmp = cmp_fn(va->s, vb->s, slen > vb->s_size ?
+					     vb->s_size : slen);

Quoted 3 lines · ZheNing Hu via GitGitGadget

The raw data of blob, tree objects may contain '\0', but most of the logic in ref-filter depands on the output of the atom being a structured string (end with '\0').

Text, yes, string is also OK. But structured? Probably not.

... being text (specifically, no embedded NULs in it).

Quoted 3 lines

Beyond, --format=%(raw) should not combine with --python, --shell, --tcl, --perl because if our binary raw data is passed to a variable in the host language, the host languages may cause escape errors.

OK. I think at least --perl and possibly --python should be able to express NULs in the "string" type we use from the host language, but I am perfectly fine with the decision to leave it to later updates.

Quoted 1 line

+Note that --format=%(raw) should not combine with --python, --shell, --tcl,

"should not combine" -> "cannot be used" would make it read more naturally (ditto for the phrase used in the proposed log message).

Quoted 4 lines

 struct atom_value {
      const char *s;

+ size_t s_size;

OK, so everything used to be a C-string that cannot hold NULs in it, but now it is a counted <ptr, len> string. Good.

Quoted 4 lines

ref-filter.c +0 -0

@@ -588,6 +607,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,
              return strbuf_addf_ret(err, -1, _("malformed field name: %.*s"),
                                     (int)(ep-atom), atom);
 

ref-filter.c static int parse_ref_filter_atom(const struct ref_format *format, · line 612

Quoted 3 lines

ref-filter.c +3 -0

+     if (format->quote_style && starts_with(sp, "raw"))
+             return strbuf_addf_ret(err, -1, _("--format=%.*s should not combine with"
+                             "--python, --shell, --tcl, --perl"), (int)(ep-atom), atom);

Don't we want to allow "raw:size" that would be a plain text? I am not sure if this check belongs here in the first place. Shouldn't it be done in raw_atom_parser() instead?

Another idea is to teach a more generic rule to quote_formatting() to detect NULs in v->s[0..v->s_size] at runtime and barf, i.e. a plain-text blob object can be used with "--shell --format=%(raw)" just fine.

Quoted 4 lines

ref-filter.c +0 -0

@@ -652,11 +675,14 @@ static int parse_ref_filter_atom(const struct ref_format *format,
      return at;
 }
 

ref-filter.c static int parse_ref_filter_atom(const struct ref_format *format, · line 685

Quoted 10 lines

ref-filter.c +5 -2

-static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
+static void quote_formatting(struct strbuf *s, const char *str, size_t len, int quote_style)
 {
      switch (quote_style) {
      case QUOTE_NONE:
-             strbuf_addstr(s, str);
+             if (len != ATOM_VALUE_S_SIZE_INIT)
+                     strbuf_add(s, str, len);
+             else
+                     strbuf_addstr(s, str);

It probably is a good idea to invent a C preprocessor macro for a named constant to be used when a structure is initialized, but it would be easier to read if the rule is "len field is negative when the value is a C-string", e.g.

                if (len < 0)

Quoted 10 lines

ref-filter.c +1 -4

@@ -785,14 +814,16 @@ static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state
      return 0;
 }
 
-static int is_empty(const char *s)
+static int is_empty(struct strbuf *buf)
 {
-     while (*s != '\0') {
-             if (!isspace(*s))
-                     return 0;

ref-filter.c static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state · line 827

Quoted 10 lines

ref-filter.c +6 -1

+     const char *s = buf->buf;
+     size_t cur_len = 0;
+
+     while ((cur_len != buf->len) && (isspace(*s) || *s == '\0')) {
              s++;
+             cur_len++;
      }
-     return 1;
+     return cur_len == buf->len;
 }

Assuming that we do want to treat NULs the same way as whitespaces, the updated code works as intended, which is good. But I have no reason to support that design decision. I do not have a strong

reason to support a design decision that goes the opposite way to treat a NUL just like we treat an 'X', but at least I can understand it (i.e. "because we have no reason to special case NUL any more than 'X' when trying to see if a buffer is 'empty', we don't"). This code on the other hand must be supported with "because we need to special case NUL for such and such reasons for the purpose of determining if a buffer is 'empty', we treat them the same way as whitespaces".

Can the change in this commit violate the invariant that if_then_else->str cannot be NULL, which seems to have been the case forever as we see an unchecked strcmp() done in the original?

If so, perhaps you can check the condition upfront, where you compute str_len above, e.g.

        if (!if_then_else->str) {
                if (if_then_else->cmp_status == COMPARE_EQUAL ||
                    if_then_else->cmp_status == COMPARE_UNEQUAL)
                        BUG(...);
        } else
                str_len = strlen(...);

If not, then I do not see the point of adding this (and later) check with BUG to this code.

Or is the invariant that .str must not be NULL could have been violated without this patch (i.e. the original was buggy in running strcmp() on .str without checking)? If so, please make it a separate preliminary change to add such an assert.

Quoted 13 lines

ref-filter.c +1 -1

 /* See grab_values */
-static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
+static void grab_raw_data(struct atom_value *val, int deref, void *buf, unsigned long buf_size, struct object *obj)

 {
      int i;
      const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;

ref-filter.c +0 -3

@@ -1307,10 +1349,22 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
                      continue;
              if (deref)
                      name++;
-             if (strcmp(name, "body") &&
-                 !starts_with(name, "subject") &&
-                 !starts_with(name, "trailers") &&

ref-filter.c static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) · line 1360

Quoted 10 lines

ref-filter.c +9 -1

-                 !starts_with(name, "contents"))
+
+             if (starts_with(name, "raw")) {
+                     if (atom->u.raw_data.option == RAW_BARE) {
+                             v->s = xmemdupz(buf, buf_size);
+                             v->s_size = buf_size;
+                     } else if (atom->u.raw_data.option == RAW_LENGTH)
+                             v->s = xstrfmt("%"PRIuMAX, (uintmax_t)buf_size);
+                     continue;
+             }

I can understand that "raw[:<options>]" handling has been inserted above the existing "from here on, we only deal with log message components" check. But

ref-filter.c static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) · line 1363

Quoted 2 lines

ref-filter.c +2 -0

+             if ((obj->type != OBJ_TAG &&
+                  obj->type != OBJ_COMMIT) ||

I do not see why these new conditions are added. The change is not justified in the proposed log message, the original did not need these conditions, and this does not concern the primary point of the change, which is to start supporting %(raw[:<option>]) placeholder.

If it is needed as a bugfix (e.g. it may be that you consider "if a blob has contents that looks very similar to 'git cat-file commit HEAD', %(body) and friends parse these out, even though it is not a commit" is a bug and the change to add these extra tests is meant as a fix), that should be done as a preliminary change before adding the support for a new atom.

Quoted 21 lines

ref-filter.c +8 -2

+                 (strcmp(name, "body") &&
+                  !starts_with(name, "subject") &&
+                  !starts_with(name, "trailers") &&
+                  !starts_with(name, "contents")))
                      continue;
              if (!subpos)
                      find_subpos(buf,

@@ -1374,25 +1428,30 @@ static void fill_missing_values(struct atom_value *val)
  * pointed at by the ref itself; otherwise it is the object the
  * ref (which is a tag) refers to.
  */
-static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf)
+static void grab_values(struct atom_value *val, int deref, struct object *obj, struct expand_data *data)
 {
+     void *buf = data->content;
+     unsigned long buf_size = data->size;
+
      switch (obj->type) {
      case OBJ_TAG:
              grab_tag_values(val, deref, obj);
-             grab_sub_body_contents(val, deref, buf);

Review static void fill_missing_values(struct atom_value *val) · line 1439

Quoted 1 line

(quoted diff) +1 -0

+             grab_raw_data(val, deref, buf, buf_size, obj);

It is very strange that a helper that is named to grab raw data can still process pieces out of a structured data. The original name is still a far better match to what the function does, even after this patch teaches it to also honor %(raw) placeholder. It is still about grabbing various "sub"-pieces out of "body contents", and the sub-piece the %(raw) grabs just happens to be "the whole thing".

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2369

Quoted 11 lines

ref-filter.c +11 -0

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)
+{
+     size_t i;
+     const char *s1 = (const char *)vs1;
+     const char *s2 = (const char *)vs2;
+
+     for (i = 0; i < n; i++) {
+             unsigned char u1 = s1[i];
+             unsigned char u2 = s2[i];
+             int U1 = toupper (u1);
+             int U2 = toupper (u2);

Does toupper('\0') even have a defined meaning?

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2371

Quoted 2 lines

ref-filter.c +2 -0

+             int diff = (UCHAR_MAX <= INT_MAX ? U1 - U2
+                     : U1 < U2 ? -1 : U2 < U1);

Looks crazy to worry about uchar wider than int. Such a system is not even standard compliant, is it?

Quoted 17 lines

ref-filter.c +1 -4

@@ -2304,6 +2382,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
      int cmp_detached_head = 0;
      cmp_type cmp_type = used_atom[s->atom].type;
      struct strbuf err = STRBUF_INIT;
+     size_t slen = 0;
 
      if (get_ref_atom_value(a, s->atom, &va, &err))
              die("%s", err.buf);

@@ -2317,10 +2396,32 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
      } else if (s->sort_flags & REF_SORTING_VERSION) {
              cmp = versioncmp(va->s, vb->s);
      } else if (cmp_type == FIELD_STR) {
-             int (*cmp_fn)(const char *, const char *);
-             cmp_fn = s->sort_flags & REF_SORTING_ICASE
-                     ? strcasecmp : strcmp;
-             cmp = cmp_fn(va->s, vb->s);
 

ref-filter.c static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru · line 2409

Quoted 10 lines

ref-filter.c +10 -0

+             if (va->s_size == ATOM_VALUE_S_SIZE_INIT &&
+                 vb->s_size == ATOM_VALUE_S_SIZE_INIT) {
+                     int (*cmp_fn)(const char *, const char *);
+                     cmp_fn = s->sort_flags & REF_SORTING_ICASE
+                             ? strcasecmp : strcmp;
+                     cmp = cmp_fn(va->s, vb->s);
+             } else {
+                     int (*cmp_fn)(const void *, const void *, size_t);
+                     cmp_fn = s->sort_flags & REF_SORTING_ICASE
+                             ? memcasecmp : memcmp;

Why not introduce two local temporary variables a_size and b_size and initialize them upfront like so:

                a_size = va->s_size < 0 ? strlen(va->s) : va->s_size;
                b_size = vb->s_size < 0 ? strlen(vb->s) : vb->s_size;

Wouldn't that allow you to do without the complex "if both are counted, do this, if A is counted but B is not, do that, ..." cascade?

I can sort-of see the point of special casing "both are traditional C strings" case (i.e. the "if" side of the "else" we are discussing here) and using strcasecmp/strcmp instead of memcasecmp/memcmp, but I do not see much point in having the if/elseif/else cascade inside this "else" clause.

Thanks.

Quoted 15 lines · Junio C Hamano <[email protected]> 于2021年5月28日周五 上午11:04写道：

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2369

Quoted 11 lines · ZheNing Hu via GitGitGadget

ref-filter.c +11 -0

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)
+{
+     size_t i;
+     const char *s1 = (const char *)vs1;
+     const char *s2 = (const char *)vs2;
+
+     for (i = 0; i < n; i++) {
+             unsigned char u1 = s1[i];
+             unsigned char u2 = s2[i];
+             int U1 = toupper (u1);
+             int U2 = toupper (u2);

Does toupper('\0') even have a defined meaning?

Forget about this inelegant help function. As I said in my reply to Felipe, this is copied from gunlib...

Quoted 15 lines

Can the change in this commit violate the invariant that if_then_else->str cannot be NULL, which seems to have been the case forever as we see an unchecked strcmp() done in the original?

If so, perhaps you can check the condition upfront, where you compute str_len above, e.g.

        if (!if_then_else->str) {
                if (if_then_else->cmp_status == COMPARE_EQUAL ||
                    if_then_else->cmp_status == COMPARE_UNEQUAL)
                        BUG(...);
        } else
                str_len = strlen(...);

Quoted 8 lines

If not, then I do not see the point of adding this (and later) check with BUG to this code.

Or is the invariant that .str must not be NULL could have been violated without this patch (i.e. the original was buggy in running strcmp() on .str without checking)? If so, please make it a separate preliminary change to add such an assert.

The BUG() here actually acts as an "assert()". ".str must not be NULL" is right, it point to "xxx" in "%(if:equals=xxx)", so it seems that these BUG() are somewhat redundant, I will remove them.

ref-filter.c +4 -0

+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)header_len);
+			continue;
+		}
+
 		if (atom->u.contents.option == C_SUB)
 			v->s = copy_subject(subpos, sublen);
 		else if (atom->u.contents.option == C_SUB_SANITIZE) {

From: ZheNing Hu <[email protected]>

Add new formatting option %(header), which will print the the structured header part of the raw object data.

In the storage layout of an object: blob and tree only contains raw data; commit and tag raw data contains two part:
header and contents. The header of tag contains "object OOO", "type TTT", "tag AAA", "tagger GGG"; The header of commit contains "tree RRR", "parent PPP", "author UUU", "committer CCC".

Signed-off-by ZheNing Hu <[email protected]>

---
 Documentation/git-for-each-ref.txt |  7 +++++
 ref-filter.c                       | 26 +++++++++++++++++
 t/t6300-for-each-ref.sh            | 45 ++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)

ref-filter.c +7 -0

+			if (atom->u.header.option == H_BARE) {
+				v->s = xmemdupz(buf, header_len);
+			} else if (atom->u.header.option == H_LENGTH)
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)header_len);
+			continue;
+		}
+
 		if (atom->u.contents.option == C_SUB)
 			v->s = copy_subject(subpos, sublen);
 		else if (atom->u.contents.option == C_SUB_SANITIZE) {

ref-filter.c static struct used_atom { · line 146

Quoted 6 lines · ZheNing Hu via GitGitGadget

              struct {
                      enum { RAW_BARE, RAW_LENGTH } option;
              } raw_data;

ref-filter.c +3 -0

+             struct {
+                     enum { H_BARE, H_LENGTH } option;
+             } header;

Raw does not use R_{BARE,LENGTH} and uses raw_data member. Header should follow suit unless there is a compelling reason not to, no?

                struct {
                        enum { HEADER_BARE, HEADER_LENGTH } option;
                } header_data;

perhaps?

ref-filter.c static void grab_raw_data(struct atom_value *val, int deref, void *buf, unsigned · line 1393

Quoted 6 lines

ref-filter.c +2 -0

@@ -1372,6 +1389,15 @@ static void grab_raw_data(struct atom_value *val, int deref, void *buf, unsigned
                                  &bodypos, &bodylen, &nonsiglen,
                                  &sigpos, &siglen);
 
+             if (starts_with(name, "header")) {
+                     size_t header_len = subpos - (const char *)buf - 1;

Hmph, is this correct? I would expect that the "header" part of a commit or a tag object excludes the blank line after the header fields. In other words, the "header" would be separated by a blank line from the "body", and that separating blank line is not part of "header" or "body".

Otherwise, if there is a user of %(header), it needs to be coded to ignore the last blank line but has to diagnose it as an error if there is a blank line before that.

ref-filter.c static void grab_raw_data(struct atom_value *val, int deref, void *buf, unsigned · line 1393

Quoted 1 line

ref-filter.c +1 -0

+                     size_t header_len = subpos - (const char *)buf - 1;

While having this may not be wrong, I am not sure who needs it. Is your "cat-file --batch" topic needs this new atom?

In order to make git cat-file --batch use ref-filter logic, I add %(raw) atom to ref-filter.

Change from last version:

1. In my discussion with Junio, I came to the conclusion that --format="%(raw)" should not be used with --python, --perl, --shell, --tcl. Therefore, die if both --format="%(raw)" and

--language are given in parse_ref_filter_atom(). The reason I don't move this part to raw_atom_parser() is if I move it to raw_atom_parser(), when we use:

git --format=%raw --sort=raw --python`

From: ZheNing Hu <[email protected]>

Only tag and commit will use grab_sub_body_contents() to grab object contents in origin implement. If we want to make blob, tree can also use grab_sub_body_contents() to get objects' raw data, a blob look like commit or tag will be wrongly regarded as commit, tag by find_subpos().

So we must add a test before find_subpos() to reject blob, tree objects. This will help us add %(raw) atom which can grab raw data of four type objects.

Helped-by Junio C Hamano <[email protected]>

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Beyond, --format=%(raw) cannot be used with --python, --shell, --tcl, --perl because if our binary raw data is passed to a variable in the host language, the host languages may cause escape errors.

+The raw data in a object is raw, For commit and tag objects, raw contain

Quoted 3 lines · ZheNing Hu via GitGitGadget

Beyond, --format=%(raw) cannot be used with --python, --shell, --tcl, --perl because if our binary raw data is passed to a variable in the host language, the host languages may cause escape errors.

"may cause escape errors" just says you are not escaping correctly in your code (implying that this patch is not good enough and with more effort we should be able to fix it to allow binaries), but the problem is the host languages may not support binaries (specifically, anything with a NUL in it) at all, which is fundamentally unfixable, in which case, rejecting is the only sensible choice.

... because the host language may not support a NUL in the variables of its string type.

Quoted 1 line

+The raw data in a object is raw, For commit and tag objects, raw contain

s/contain/contains/, but more importantly, as we are not introducing %(header), I do not see why we want to talk about its details. For commits and tags, just like for trees and blobs, 'raw' is the raw data in the object, so beyond "The raw data of a object is %(raw)", I do not think there is anything to talk about.

ref-filter.c static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state · line 824

Quoted 6 lines

ref-filter.c +5 -0

+     const char *s = buf->buf;
+     size_t cur_len = 0;
+
+     while ((cur_len != buf->len) && (isspace(*s) || *s == '\0')) {
              s++;
+             cur_len++;

Is NUL treated the same as a whitespace letter for the purpose of determining if a line is empty? WHY?

Quoted 3 lines

... because the host language may not support a NUL in the variables of its string type.

I agree. But host language not only support NUL but also some Non-ASCII character and Non-UTF-8 code:

Quoted 7 lines

Quoted 6 lines

ref-filter.c +5 -0

+     const char *s = buf->buf;
+     size_t cur_len = 0;
+
+     while ((cur_len != buf->len) && (isspace(*s) || *s == '\0')) {
              s++;
+             cur_len++;

Quoted 2 lines

Is NUL treated the same as a whitespace letter for the purpose of determining if a line is empty? WHY?

Well, there seems to be no correction here. But is it true that memory like "\0abc" is considered empty?

Quoted 3 lines

Well, there seems to be no correction here. But is it true that memory like "\0abc" is considered empty?

That sample has 'a' or 'b' or 'c' that are clearly not part of an "empty" string and irrelevant. After all, a string " abc" is not treated as empty in the original implementation, either.

You are treating a block of memory with e.g. " \000 " (SP NUL SP) as
an "empty line" just like you do for " " (SP SP SP), but I think we
should treat it more like " \001 " or " \007 ", i.e. not an empty string at all.

ref-filter.c static struct { · line 551

Quoted 8 lines · ZheNing Hu via GitGitGadget

ref-filter.c +1 -0

@@ -530,6 +545,7 @@ static struct {
      { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
      { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
      { "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+     { "raw", SOURCE_OBJ, FIELD_STR, raw_atom_parser },
      { "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
      { "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
      { "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },

Doesn't this conflict with your own zh/ref-filter-atom-type topic? Shouldn't one build on top of the other?

Or did we find something fundamentally broken about the other topic to make us retract it that I do not remember?

Thanks.

Quoted 7 lines

Doesn't this conflict with your own zh/ref-filter-atom-type topic? Shouldn't one build on top of the other?

Or did we find something fundamentally broken about the other topic to make us retract it that I do not remember?

Thanks.

I am waiting for zh/ref-filter-atom-type to be merged into master. But it

hasn't happened yet. But if I want to base the current topic on zh/ref-filter-atom-type, GGG will send past patches (zh/ref-filter-atom-type) repeatedly.

Quoted 6 lines · ZheNing Hu

Quoted 6 lines

Doesn't this conflict with your own zh/ref-filter-atom-type topic? Shouldn't one build on top of the other?

Or did we find something fundamentally broken about the other topic to make us retract it that I do not remember?

Quoted 3 lines

Quoted 1 line

Thanks.

I am waiting for zh/ref-filter-atom-type to be merged into master. But it

As you sent this that conflicts with it, clearly you are doing something else that conflicts with it _without waiting_ ;-).

Quoted 3 lines

hasn't happened yet. But if I want to base the current topic on zh/ref-filter-atom-type, GGG will send past patches (zh/ref-filter-atom-type) repeatedly.

I thought GGG lets you say "this is based on that other branch, not on the 'master' branch" to solve that exact issue?

Quoted 6 lines · Junio C Hamano <[email protected]> 于2021年6月1日周二 下午4:54写道：

Quoted 3 lines · ZheNing Hu

[...] But if I want to base the current topic on zh/ref-filter-atom-type, GGG will send past patches (zh/ref-filter-atom-type) repeatedly.

Quoted 2 lines

I thought GGG lets you say "this is based on that other branch, not on the 'master' branch" to solve that exact issue?

I'm not sure...I will try it after I rebasing this topic to zh/ref-filter-atom-type.

I just remember that it looked like something went wrong with my base on your patch last time.[1]

[1] https://lore.kernel.org/git/[email protected]/

ref-filter.c +9 -0

+	const char *end = s1 + n;
+
+	for (; s1 < end; s1++, s2++) {
+		int diff = tolower(*s1) - tolower(*s2);
+		if (diff)
+			return diff;
+	}
+	return 0;
+}

Quoted 1 line · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2358

Quoted 4 lines

ref-filter.c +4 -0

+static int memcasecmp(const void *vs1, const void *vs2, size_t n)
+{
+     const char *s1 = (const void *)vs1;
+     const char *s2 = (const void *)vs2;

As I explained in another mail, I think this is better:

        const char *s1 = vs1, *s2 = vs2;

Quoted 4 lines

ref-filter.c +4 -0

+     const char *end = s1 + n;
+
+     for (; s1 < end; s1++, s2++) {
+             int diff = tolower(*s1) - tolower(*s2);

ref-filter.c static int compare_detached_head(struct ref_array_item *a, struct ref_array_item · line 2367

Quoted 5 lines

ref-filter.c +5 -0

+             if (diff)
+                     return diff;
+     }
+     return 0;
+}

--
Felipe Contreras