* [PATCH] doc: format.notes specify a ref under refs/notes/ hierarchy
From: Junio C Hamano @ 2023-12-15 22:28 UTC (permalink / raw)
To: git; +Cc: Ramsay Jones
In-Reply-To: <xmqqttojjegr.fsf@gitster.g>
There is no 'ref/notes/' hierarchy. '[format] notes = foo' uses notes
that are found in 'refs/notes/foo'.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* According to my eyeballing "git grep refs/ Documentation" result,
this was the only remaining mention of "ref/" in Documentation/
hierarchy that misspells "refs/".
Documentation/config/format.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git c/Documentation/config/format.txt w/Documentation/config/format.txt
index c98412b697..7410e930e5 100644
--- c/Documentation/config/format.txt
+++ w/Documentation/config/format.txt
@@ -119,7 +119,7 @@ format.notes::
`--notes=<ref>`, where `ref` is the non-boolean value. Defaults
to false.
+
-If one wishes to use the ref `ref/notes/true`, please use that literal
+If one wishes to use the ref `refs/notes/true`, please use that literal
instead.
+
This configuration can be specified multiple times in order to allow
^ permalink raw reply related
* Re: [PATCH 1/5] git.txt: HEAD is not that special
From: Ramsay Jones @ 2023-12-15 22:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt
In-Reply-To: <xmqqttojjegr.fsf@gitster.g>
On 15/12/2023 22:19, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>>
>>>> -may contain the SHA-1 name of an object or the name of another ref. Refs
>>>> -with names beginning `ref/head/` contain the SHA-1 name of the most
>>>> +may contain the SHA-1 name of an object or the name of another ref (the
>>>> +latter is called a "symbolic ref").
>>>> +Refs with names beginning `ref/head/` contain the SHA-1 name of the most
>>>
>>> Hmm, s:ref/head:refs/heads: right?
>>
>> Yeah, right, not a new problem with this change, but is indeed a
>> good thing to catch and correct. Thanks for a careful review.
>
> And we have ref/tags/ just below, which I also have fixed locally.
Heh, yeah I missed that, along with 'ref/notes'. ;)
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 0/5] make room for "special ref"
From: Ramsay Jones @ 2023-12-15 22:44 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Patrick Steinhardt
In-Reply-To: <xmqq5y0zkvqx.fsf@gitster.g>
On 15/12/2023 21:21, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> ... For example, FETCH_HEAD currently stores not
>> just a single object name, but can and is used to store multiple
>> object names, each with annotations to record where they came from.
>> There indeed may be a need to introduce a new term to refer to such
>> "special refs".
>
> The "may be" here vaguely hints another possibility. If we manage
> to get rid of the "special refs", we do not even have to mention
> "special refs", and more importantly, we do not need extra code to
> deal with them.
>
> For FETCH_HEAD, for example, I wonder if an update along this line
> is possible:
>
> * Teach "git fetch" to store what it writes to FETCH_HEAD to a
> different file, under a distinctly different filename (e.g.,
> $GIT_DIR/fetched-tips). Demote FETCH_HEAD to a pseudoref, and
> store the first object name in that "fetched-tips" file to it.
>
> * Teach "git pull" to learn what it used to learn from FETCH_HEAD
> (i.e., list of fetched tips, each annotated with what ref at what
> repository it came from and if it is to be merged) from the new
> "fetched-tips" file.
>
> The "special" ness of FETCH_HEAD is really an implementation detail
> of how "git pull" works and how the findings of "git fetch" are
> communicated to "git pull". The general refs API should not have to
> worry about it, and the refs backends should not have to worry about
> storing more than just an object name (or if it is a symbolic ref,
> the target refname).
>
> An end-user command like "git log ORIG_HEAD..FETCH_HEAD" would not
> be affected by changes along the above line, because the current
> FETCH_HEAD, when used as a revision, will work as if it stores the
> single object name that is listed first in the file.
>
> If somebody is reading FETCH_HEAD and acting on its contents (rather
> than merely consuming it as a ref of the first object), perhaps
> feeding it to "git fmt-merge-msg", they will be broken by such a
> change (indeed, our own "git pull" will be broken by the change to
> "git fetch", and the second bullet point above is about fixing the
> exact fallout from it), but I am not sure if that is a use case worth
> worrying about.
>
> Hmm?
>
Yes, I was going to suggest exactly this, after Patrick pointed out
that there were only two 'special psuedo-refs' (I had a vague feeling
there were some more than that) FETCH_HEAD and MERGE_HEAD.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 0/5] make room for "special ref"
From: Junio C Hamano @ 2023-12-16 0:44 UTC (permalink / raw)
To: Ramsay Jones; +Cc: git, Patrick Steinhardt
In-Reply-To: <321b8084-fddb-4b5d-86af-7f88cb3edf7b@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> Yes, I was going to suggest exactly this, after Patrick pointed out
> that there were only two 'special psuedo-refs' (I had a vague feeling
> there were some more than that) FETCH_HEAD and MERGE_HEAD.
Glad to see that I am not alone. We should be able to treat
MERGE_HEAD similarly. It is used to communicate the list of "other
parents" from "git merge" that stops in the middle (either for merge
conflict, or in response to the "--no-commit" command line option)
to "git commit" that concludes such an unfinished merge. Many
commands merely use the presence of MERGE_HEAD as a sign that a
merge is in progress (e.g. "git status"), which would not break if
we just started to record the first parent in a pseudoref MERGE_HEAD
and wrote the other octopus parents elsewhere, but some commands do
need all these parents from MERGE_HEAD (e.g. "git blame" that
synthesizes a fake starting commit out of the working tree state).
If we cannot get rid of all "special refs" anyway, however, I think
there is little that we can gain from doing such "make FETCH_HEAD
and MERGE_HEAD into a single-object pseudoref, and write other info
in separate files" exercise. We can treat the current FETCH_HEAD
and MERGE_HEAD as "file that is not and is more than a ref", which
is what the current code is doing anyway, which means we would
declare that they have to stay to be files under $GIT_DIR/ and will
be accessed via the filesystem access. At that point, calling them
"special ref" might even be more misleading than its worth and we
may be better off to admit that they are not even refs but a
datafile some commands can use to obtain input from, but the phrase
we use to refer to them, be it "special ref" or some random
datafile, does not make a fundamental change on anything.
^ permalink raw reply
* Re: [PATCH 3/5] refs.h: HEAD is not that special
From: Andy Koppe @ 2023-12-16 10:03 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Patrick Steinhardt
In-Reply-To: <20231215203245.3622299-4-gitster@pobox.com>
On 15/12/2023 20:32, Junio C Hamano wrote:
> In-code comment explains pseudorefs but used a wrong nomenclature
> "special ref".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> refs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs.h b/refs.h
> index 23211a5ea1..ff113bb12a 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -56,7 +56,7 @@ struct worktree;
> * Even with RESOLVE_REF_ALLOW_BAD_NAME, names that escape the refs/
> * directory and do not consist of all caps and underscores cannot be
> * resolved. The function returns NULL for such ref names.
> - * Caps and underscores refers to the special refs, such as HEAD,
> + * Caps and underscores refers to the pseudorefs, such as HEAD,
> * FETCH_HEAD and friends, that all live outside of the refs/ directory.
> */
> #define RESOLVE_REF_READING 0x01
gitglossary thinks that HEAD is not a pseudoref:
"Pseudorefs are a class of files under $GIT_DIR which behave like refs
for the purposes of rev-parse, but which are treated specially by git.
Pseudorefs both have names that are all-caps, and always start with a
line consisting of a SHA‐1 followed by whitespace. So, HEAD is not a
pseudoref, because it is sometimes a symbolic ref."
(Also, the "sometimes" there actually is "whenever you're on a branch",
which is most of the time for most people.)
Regards,
Andy
^ permalink raw reply
* Re: [PATCH 0/5] make room for "special ref"
From: Andy Koppe @ 2023-12-16 10:20 UTC (permalink / raw)
To: Ramsay Jones, Junio C Hamano, git; +Cc: Patrick Steinhardt
In-Reply-To: <321b8084-fddb-4b5d-86af-7f88cb3edf7b@ramsayjones.plus.com>
On 15/12/2023 22:44, Ramsay Jones wrote:
> On 15/12/2023 21:21, Junio C Hamano wrote:
>> If somebody is reading FETCH_HEAD and acting on its contents (rather
>> than merely consuming it as a ref of the first object), perhaps
>> feeding it to "git fmt-merge-msg", they will be broken by such a
>> change (indeed, our own "git pull" will be broken by the change to
>> "git fetch", and the second bullet point above is about fixing the
>> exact fallout from it), but I am not sure if that is a use case worth
>> worrying about.
>
> Yes, I was going to suggest exactly this, after Patrick pointed out
> that there were only two 'special psuedo-refs' (I had a vague feeling
> there were some more than that) FETCH_HEAD and MERGE_HEAD.
According to the pseudoref entry of gitglossary, CHERRY_PICK_HEAD also
stores additional data (which would imply that REVERT_HEAD does too).
Looking at CHERRY_PICK_HEAD during a pick though, I only see a single
hash, even when picking multiple commits.
Regards,
Andy
^ permalink raw reply
* Re: [PATCH] Use ^=1 to toggle between 0 and 1
From: René Scharfe @ 2023-12-16 10:46 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood
Cc: Jeff King, AtariDreams via GitGitGadget, git, Seija Kijin
In-Reply-To: <xmqqo7erl7er.fsf@gitster.g>
Am 15.12.23 um 18:09 schrieb Junio C Hamano:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> Even if it unlikely that we would directly compare a boolean variable
>> to "true" or "false" it is certainly conceivable that we'd compare two
>> boolean variables directly. For the integer fallback to be safe we'd
>> need to write
>>
>> if (!cond_a == !cond_b)
>>
>> rather than
>>
>> if (cond_a == cond_b)
>
> Eek, it defeats the benefit of using true Boolean type if we had to
> train ourselves to write the former, doesn't it?
Indeed.
>> [1] 7bc341e21b (git-compat-util: add a test balloon for C99 support,
>> 2021-12-01)
>
> Nice to be reminded of this one.
>
> The cited commit does not start to use any specific feature from
> C99, other than that we now require that the compiler claims C99
> conformance by __STDC_VERSION__ set appropriately. The commit log
> message says C99 "provides a variety of useful features, including
> ..., many of which we already use.", which implies that our wish was
> to officially allow any and all features in C99 to be used in our
> codebase after a successful flight of this test balloon.
>
> Now, I think we saw a successful flight of this test balloon by now.
> Is allowing all the C99 the next step we really want to take?
>
> I still personally have an aversion against decl-after-statement and
> //-comments, not due to portability reasons at all, but because I
> find that the code is easier to read without it. But in principle,
> it is powerful to be able to say "OK, as long as the feature is in
> C99 you can use it", instead of having to decide on individual
> features, and I am not fundamentally against going that route if it
> is where people want to go.
C99 added a lot of features, but we already use several of them.
Support for individual features may vary, though -- who knows?
E.g. http://www.compilers.de/vbcc.html claims to support "most of
ISO/IEC 9899:1999 (C99)", yet _Bool is not mentioned in its docs (but
__STDC_VERSION__ 199901L is). It's not a particularly interesting
compiler for us, but still a real-world example of selective C99
support.
The table at the bottom of https://en.cppreference.com/w/c/99 would be
useful if it was filled out for more compilers. And it also doesn't
mention _Bool and stdbool.h.
TenDRA and the M/o/Vfuscator are the only compilers without stdbool.h
support on https://godbolt.org/ as far as I can see, but that website
doesn't have a lot of commercial compilers (understandably).
So I guess in practice we still need to check each new feature, even
though in theory we should be fine after the two-year test.
René
^ permalink raw reply
* [PATCH] git-compat-util: convert skip_{prefix,suffix}{,_mem} to bool
From: René Scharfe @ 2023-12-16 10:47 UTC (permalink / raw)
To: git
Cc: AtariDreams via GitGitGadget, Seija Kijin, Junio C Hamano,
Jeff King, Phillip Wood
In-Reply-To: <99b3a727-36fd-4fa5-a6be-60ae6fc5911e@gmail.com>
Use the data type bool and its values true and false to document the
binary return value of skip_prefix() and friends more explicitly.
This first use of stdbool.h, introduced with C99, is meant to check
whether there are platforms that claim support for C99, as tested by
7bc341e21b (git-compat-util: add a test balloon for C99 support,
2021-12-01), but still lack that header for some reason.
A fallback based on a wider type, e.g. int, would have to deal with
comparisons somehow to emulate that any non-zero value is true:
bool b1 = 1;
bool b2 = 2;
if (b1 == b2) puts("This is true.");
int i1 = 1;
int i2 = 2;
if (i1 == i2) puts("Not printed.");
#define BOOLEQ(a, b) (!(a) == !(b))
if (BOOLEQ(i1, i2)) puts("This is true.");
So we'd be better off using bool everywhere without a fallback, if
possible. That's why this patch doesn't include any.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
git-compat-util.h | 42 ++++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 3e7a59b5ff..603c97e3b3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -225,6 +225,7 @@ struct strbuf;
#include <stddef.h>
#include <stdlib.h>
#include <stdarg.h>
+#include <stdbool.h>
#include <string.h>
#ifdef HAVE_STRINGS_H
#include <strings.h> /* for strcasecmp() */
@@ -684,11 +685,11 @@ report_fn get_warn_routine(void);
void set_die_is_recursing_routine(int (*routine)(void));
/*
- * If the string "str" begins with the string found in "prefix", return 1.
+ * If the string "str" begins with the string found in "prefix", return true.
* The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in
* the string right after the prefix).
*
- * Otherwise, return 0 and leave "out" untouched.
+ * Otherwise, return false and leave "out" untouched.
*
* Examples:
*
@@ -699,57 +700,58 @@ void set_die_is_recursing_routine(int (*routine)(void));
* [skip prefix if present, otherwise use whole string]
* skip_prefix(name, "refs/heads/", &name);
*/
-static inline int skip_prefix(const char *str, const char *prefix,
- const char **out)
+static inline bool skip_prefix(const char *str, const char *prefix,
+ const char **out)
{
do {
if (!*prefix) {
*out = str;
- return 1;
+ return true;
}
} while (*str++ == *prefix++);
- return 0;
+ return false;
}
/*
* Like skip_prefix, but promises never to read past "len" bytes of the input
* buffer, and returns the remaining number of bytes in "out" via "outlen".
*/
-static inline int skip_prefix_mem(const char *buf, size_t len,
- const char *prefix,
- const char **out, size_t *outlen)
+static inline bool skip_prefix_mem(const char *buf, size_t len,
+ const char *prefix,
+ const char **out, size_t *outlen)
{
size_t prefix_len = strlen(prefix);
if (prefix_len <= len && !memcmp(buf, prefix, prefix_len)) {
*out = buf + prefix_len;
*outlen = len - prefix_len;
- return 1;
+ return true;
}
- return 0;
+ return false;
}
/*
- * If buf ends with suffix, return 1 and subtract the length of the suffix
- * from *len. Otherwise, return 0 and leave *len untouched.
+ * If buf ends with suffix, return true and subtract the length of the suffix
+ * from *len. Otherwise, return false and leave *len untouched.
*/
-static inline int strip_suffix_mem(const char *buf, size_t *len,
- const char *suffix)
+static inline bool strip_suffix_mem(const char *buf, size_t *len,
+ const char *suffix)
{
size_t suflen = strlen(suffix);
if (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen))
- return 0;
+ return false;
*len -= suflen;
- return 1;
+ return true;
}
/*
- * If str ends with suffix, return 1 and set *len to the size of the string
- * without the suffix. Otherwise, return 0 and set *len to the size of the
+ * If str ends with suffix, return true and set *len to the size of the string
+ * without the suffix. Otherwise, return false and set *len to the size of the
* string.
*
* Note that we do _not_ NUL-terminate str to the new length.
*/
-static inline int strip_suffix(const char *str, const char *suffix, size_t *len)
+static inline bool strip_suffix(const char *str, const char *suffix,
+ size_t *len)
{
*len = strlen(str);
return strip_suffix_mem(str, len, suffix);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 0/5] make room for "special ref"
From: Andy Koppe @ 2023-12-16 10:56 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Patrick Steinhardt
In-Reply-To: <20231215203245.3622299-1-gitster@pobox.com>
On 15/12/2023 20:32, Junio C Hamano wrote:
> A pseudo ref is merely a normal ref with a funny naming convention,
> i.e., being outside the refs/ hierarchy and has names with all
> uppercase letters (or an underscore).
I know what you mean, but gitglossary defines pseudorefs as separate
from refs, albeit behaving like refs. Their name itself implies the same.
Although the 'ref' entry then goes on to say that "there are a few
special-purpose refs that do not begin with 'refs/', the most notable
example being HEAD."
That implies that at least some of the pseudorefs are refs after all,
while keeping in mind that "HEAD is not a pseudoref, because it is
sometimes a symbolic ref" according to the 'pseudoref' entry.
I think a clearer answer on whether pseudorefs are refs is needed, or at
least a better-defined fudge, such as "pseudorefs are refs except when ...".
Defining everything under "refs/" as refs, and the stuff outside it
including HEAD itself as pseudorefs, would draw clearer lines. The fact
HEAD is usually symbolic doesn't seem all that relevant from the
perspective of a user trying to get a grasp of refs and pseudorefs.
Regards,
Andy
^ permalink raw reply
* Re: [PATCH 5/5] docs: MERGE_AUTOSTASH is not that special
From: Andy Koppe @ 2023-12-16 11:04 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Patrick Steinhardt
In-Reply-To: <20231215203245.3622299-6-gitster@pobox.com>
On 15/12/2023 20:32, Junio C Hamano wrote:
> A handful of manual pages called MERGE_AUTOSTASH a "special ref",
> but there is nothing special about it. It merely is yet another
> pseudoref.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/merge-options.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index d8f7cd7ca0..3eaefc4e94 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -191,7 +191,7 @@ endif::git-pull[]
> --autostash::
> --no-autostash::
> Automatically create a temporary stash entry before the operation
> - begins, record it in the special ref `MERGE_AUTOSTASH`
> + begins, record it in the ref `MERGE_AUTOSTASH`
> and apply it after the operation ends. This means
> that you can run the operation on a dirty worktree. However, use
> with care: the final stash application after a successful
Should that say 'pseudoref' instead of 'ref'?
And since MERGE_AUTOSTASH is documented here, it probably should be in
gitrevisions as well.
Regards,
Andy
^ permalink raw reply
* Re: Propose a change in open for passing in the file type.
From: Torsten Bögershausen @ 2023-12-16 13:28 UTC (permalink / raw)
To: Đoàn Trần Công Danh; +Cc: Haritha D, git@vger.kernel.org
In-Reply-To: <ZXkwTYD9nmPYn9UW@danh.dev>
On Wed, Dec 13, 2023 at 11:17:17AM +0700, Đoàn Trần Công Danh wrote:
[]
> Would it work if you always open the file as BINARY?
Yes, I think so.
> And let's all the
> conversion done by git via some configs (core.encoding?)?
We already have an attribute, "working-tree-encoding", that
can tell Git to do the encoding/reencoding.
The advantage of the .gitattributes file is, that it is
typically commit into the repo, and travels with `git push`
and `git fetch` or `git pull` to the different work stations,
so that everybody has the same settings.
In opposite, config files are always local.
So that everybody should do the same (local) git config.
If that is forgotten for some reason, then different
configurations leads often to some kind of chaos.
But I don't have a z/os sytem to test on.
^ permalink raw reply
* MyFirstContribution.txt leaks memory
From: Vinayak Dev @ 2023-12-16 13:45 UTC (permalink / raw)
To: git
Hello everyone!
I was going through the MyFirstContribution tutorial and implemented
its code as an exercise to warm up my git skills.
However, when I pushed the code to my fork on GitHub, I noticed that
it fails the linux-leaks test.
So I fired up the leaks checker that MacOS provides and got a number
of leaks in the tutorial code.
One was that the strbuf commitline was not released, so I was able to
fix that trivially.
However, the stack trace for the second leak shows that the memory
leaks when git_config() is called on the wt_status struct, which in turn
calls the git_default_config() function. As it must go through my
.gitconfig, I noticed that when it checks for "core.editor", it calls
git_config_string(), where the string is duplicated into the
editor_program variable.
This is precisely where the leak seems to be, as pointed out by the
stack of function calls. I thought that maybe freeing dest as in
free((void *)*dest) in git_config_string() before calling xstrdup()
might free up the leakage (this has been done in a number of places in
the code-base).
While this does free up the leakage, I am getting a number of failures in
the test suite(some of which are occurring even without the changes),
particularly related to setting up bare repositories.
Also, as only the tutorial code's binary(./bin-wrappers/git psuh) that I
implemented leaks memory, I suspect that it is my fault somewhere.
Could anyone point out what is the correct way to free up memory in this case?
Or would the situation warrant adding the call to free() in git_config_string()?
Thanks a lot!
Vinayak
^ permalink raw reply
* Question: rerere preimage format
From: Mohamed Mohey @ 2023-12-16 15:46 UTC (permalink / raw)
To: git
Hi,
I was reading the Rerere section of the Pro Git book when I came
across the following snippet:
$ git rerere diff
--- a/hello.rb
+++ b/hello.rb
@@ -1,11 +1,11 @@
#! /usr/bin/env ruby
def hello
-<<<<<<<
- puts 'hello mundo'
-=======
+<<<<<<< HEAD
puts 'hola world'
->>>>>>>
+=======
+ puts 'hello mundo'
+>>>>>>> i18n-world
end
If I understand correctly, this is the diff between rerere's preimage
and the version in the current working directory, as explained in this
StackOverflow answer:
https://stackoverflow.com/a/27364585
What I don't understand, however, is why rerere records its initial
preimage without labels. This seems to have been rerere's behavior
ever since it was introduced as a perl script back in 2006:
https://lore.kernel.org/git/7v4q3no0v7.fsf@assigned-by-dhcp.cox.net/
+ $one = join('', @{$side[0]});
+ $two = join('', @{$side[1]});
+ if ($two le $one) {
+ ($one, $two) = ($two, $one);
+ }
+ print $out "<<<<<<<\n";
+ print $out $one;
+ print $out "=======\n";
+ print $out $two;
+ print $out ">>>>>>>\n";
+ @side = ();
So the preimage format doesn't contain any labels, and outputs the
lexicographically smaller hunk first,
while the default merge conflict output has labels and outputs the
current branch first followed by the other branch.
If rerere recorded its preimage in the same format as the default
merge conflict format,
the above rerere diff output would be empty as there would be no
difference between what we started with and what we resolved into.
The same behaviour would still occur with the diff3 style format, for
example, but at least it wouldn't occur with the default merge style
format.
My question is:
Is there a reason rerere's output is like this that I'm missing? Or is
it just there for convenience since it doesn't affect rerere's
intended functionality?
Thanks,
Mohamed Mohey
^ permalink raw reply
* Re: Question: rerere preimage format
From: Junio C Hamano @ 2023-12-16 17:34 UTC (permalink / raw)
To: Mohamed Mohey; +Cc: git
In-Reply-To: <CAAY9aimx4X2jDaFz+jWtAj==g+J3QA8LKPQFePyGFhVvbwKNtQ@mail.gmail.com>
Mohamed Mohey <mmi9433@gmail.com> writes:
> My question is:
>
> Is there a reason rerere's output is like this that I'm missing? Or is
> it just there for convenience since it doesn't affect rerere's
> intended functionality?
>
>
> Thanks,
> Mohamed Mohey
Documentation/technical/rerere.txt would be a good place to start.
^ permalink raw reply
* [PATCH v3 0/2] fix fetch atomic error message
From: Jiang Xin @ 2023-12-17 14:11 UTC (permalink / raw)
To: Git List, Junio C Hamano, Patrick Steinhardt; +Cc: Jiang Xin
In-Reply-To: <cover.1702556642.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
# Changes since v2:
Changed the patches with help from Patrick.
# range-diff v2...v3
1: 0b9865f1df ! 1: 0a6c53de7c t5574: test porcelain output of atomic fetch
@@ Commit message
In a later commit, we will fix this issue.
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## t/t5574-fetch-output.sh ##
@@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '
@@ t/t5574-fetch-output.sh: test_expect_success 'fetch porcelain output' '
+ FORCE_UPDATED_NEW=$(git rev-parse HEAD)
'
-+for opt in off on
++for opt in "" "--atomic"
+do
-+ case $opt in
-+ on)
-+ opt=--atomic
-+ ;;
-+ off)
-+ opt=
-+ ;;
-+ esac
+ test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+ test_when_finished "rm -rf porcelain" &&
+
2: e10fa198dd ! 2: a8a7658fb2 fetch: no redundant error message for atomic fetch
@@ Commit message
will appear at the end of do_fetch(). It was introduced in b3a804663c
(fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).
- In function do_fetch(), a failure message is already shown before the
- retcode is set, so we should not call additional error() at the end of
- this function.
+ Because a failure message is displayed before setting retcode in the
+ function do_fetch(), calling error() on the err message at the end of
+ this function may result in redundant or empty error message to be
+ displayed.
We can remove the redundant error() function, because we know that
the function ref_transaction_abort() never fails. While we can find a
@@ Commit message
if (ref_transaction_abort(transaction, &error))
error("abort: %s", error.buf);
- We can fix this issue follow this pattern, and the test case "fetch
- porcelain output (atomic)" in t5574 will also be fixed. If in the future
- we decide that we don't need to check the return value of the function
- ref_transaction_abort(), this change can be fixed along with it.
+ Following this pattern, we can tolerate the return value of the function
+ ref_transaction_abort() being changed in the future. We also delay the
+ output of the err message to the end of do_fetch() to reduce redundant
+ code. With these changes, the test case "fetch porcelain output
+ (atomic)" in t5574 will also be fixed.
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## builtin/fetch.c ##
+@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
+ if (atomic_fetch) {
+ transaction = ref_transaction_begin(&err);
+ if (!transaction) {
+- retcode = error("%s", err.buf);
++ retcode = -1;
+ goto cleanup;
+ }
+ }
+@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
+
+ retcode = ref_transaction_commit(transaction, &err);
+ if (retcode) {
+- error("%s", err.buf);
+ ref_transaction_free(transaction);
+ transaction = NULL;
+ goto cleanup;
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
}
cleanup:
- if (retcode && transaction) {
- ref_transaction_abort(transaction, &err);
-+ if (retcode && transaction && ref_transaction_abort(transaction, &err))
- error("%s", err.buf);
-- }
+- error("%s", err.buf);
++ if (retcode) {
++ if (err.len) {
++ error("%s", err.buf);
++ strbuf_reset(&err);
++ }
++ if (transaction && ref_transaction_abort(transaction, &err) &&
++ err.len)
++ error("%s", err.buf);
+ }
display_state_release(&display_state);
Jiang Xin (2):
t5574: test porcelain output of atomic fetch
fetch: no redundant error message for atomic fetch
builtin/fetch.c | 14 ++++---
t/t5574-fetch-output.sh | 89 +++++++++++++++++++++++------------------
2 files changed, 59 insertions(+), 44 deletions(-)
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply
* [PATCH v3 1/2] t5574: test porcelain output of atomic fetch
From: Jiang Xin @ 2023-12-17 14:11 UTC (permalink / raw)
To: Git List, Junio C Hamano, Patrick Steinhardt; +Cc: Jiang Xin
In-Reply-To: <cover.1702821462.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
The test case "fetch porcelain output" checks output of the fetch
command. The error output must be empty with the follow assertion:
test_must_be_empty stderr
But this assertion fails if using atomic fetch. Refactor this test case
to use different fetch options by splitting it into three test cases.
1. "setup for fetch porcelain output".
2. "fetch porcelain output": for non-atomic fetch.
3. "fetch porcelain output (atomic)": for atomic fetch.
Add new command "test_commit ..." in the first test case, so that if we
run these test cases individually (--run=4-6), "git rev-parse HEAD~"
command will work properly. Run the above test cases, we can find that
one test case has a known breakage, as shown below:
ok 4 - setup for fetch porcelain output
ok 5 - fetch porcelain output # TODO known breakage vanished
not ok 6 - fetch porcelain output (atomic) # TODO known breakage
The failed test case has an error message with only the error prompt but
no message body, as follows:
'stderr' is not empty, it contains:
error:
In a later commit, we will fix this issue.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
t/t5574-fetch-output.sh | 89 +++++++++++++++++++++++------------------
1 file changed, 50 insertions(+), 39 deletions(-)
diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 90e6dcb9a7..b579364c47 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -61,11 +61,10 @@ test_expect_success 'fetch compact output' '
test_cmp expect actual
'
-test_expect_success 'fetch porcelain output' '
- test_when_finished "rm -rf porcelain" &&
-
+test_expect_success 'setup for fetch porcelain output' '
# Set up a bunch of references that we can use to demonstrate different
# kinds of flag symbols in the output format.
+ test_commit commit-for-porcelain-output &&
MAIN_OLD=$(git rev-parse HEAD) &&
git branch "fast-forward" &&
git branch "deleted-branch" &&
@@ -74,15 +73,10 @@ test_expect_success 'fetch porcelain output' '
FORCE_UPDATED_OLD=$(git rev-parse HEAD) &&
git checkout main &&
- # Clone and pre-seed the repositories. We fetch references into two
- # namespaces so that we can test that rejected and force-updated
- # references are reported properly.
- refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
- git clone . porcelain &&
- git -C porcelain fetch origin $refspecs &&
+ # Backup to preseed.git
+ git clone --mirror . preseed.git &&
- # Now that we have set up the client repositories we can change our
- # local references.
+ # Continue changing our local references.
git branch new-branch &&
git branch -d deleted-branch &&
git checkout fast-forward &&
@@ -91,36 +85,53 @@ test_expect_success 'fetch porcelain output' '
git checkout force-updated &&
git reset --hard HEAD~ &&
test_commit --no-tag force-update-new &&
- FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
-
- cat >expect <<-EOF &&
- - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
- - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
- $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
- ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
- * $ZERO_OID $MAIN_OLD refs/unforced/new-branch
- $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
- + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
- * $ZERO_OID $MAIN_OLD refs/forced/new-branch
- $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
- + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
- * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
- EOF
-
- # Execute a dry-run fetch first. We do this to assert that the dry-run
- # and non-dry-run fetches produces the same output. Execution of the
- # fetch is expected to fail as we have a rejected reference update.
- test_must_fail git -C porcelain fetch \
- --porcelain --dry-run --prune origin $refspecs >actual &&
- test_cmp expect actual &&
-
- # And now we perform a non-dry-run fetch.
- test_must_fail git -C porcelain fetch \
- --porcelain --prune origin $refspecs >actual 2>stderr &&
- test_cmp expect actual &&
- test_must_be_empty stderr
+ FORCE_UPDATED_NEW=$(git rev-parse HEAD)
'
+for opt in "" "--atomic"
+do
+ test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+ test_when_finished "rm -rf porcelain" &&
+
+ # Clone and pre-seed the repositories. We fetch references into two
+ # namespaces so that we can test that rejected and force-updated
+ # references are reported properly.
+ refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
+ git clone preseed.git porcelain &&
+ git -C porcelain fetch origin $opt $refspecs &&
+
+ cat >expect <<-EOF &&
+ - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
+ - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
+ $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
+ ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
+ * $ZERO_OID $MAIN_OLD refs/unforced/new-branch
+ $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
+ + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
+ * $ZERO_OID $MAIN_OLD refs/forced/new-branch
+ $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
+ + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
+ * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
+ EOF
+
+ # Change the URL of the repository to fetch different references.
+ git -C porcelain remote set-url origin .. &&
+
+ # Execute a dry-run fetch first. We do this to assert that the dry-run
+ # and non-dry-run fetches produces the same output. Execution of the
+ # fetch is expected to fail as we have a rejected reference update.
+ test_must_fail git -C porcelain fetch $opt \
+ --porcelain --dry-run --prune origin $refspecs >actual &&
+ test_cmp expect actual &&
+
+ # And now we perform a non-dry-run fetch.
+ test_must_fail git -C porcelain fetch $opt \
+ --porcelain --prune origin $refspecs >actual 2>stderr &&
+ test_cmp expect actual &&
+ test_must_be_empty stderr
+ '
+done
+
test_expect_success 'fetch porcelain with multiple remotes' '
test_when_finished "rm -rf porcelain" &&
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* [PATCH v3 2/2] fetch: no redundant error message for atomic fetch
From: Jiang Xin @ 2023-12-17 14:11 UTC (permalink / raw)
To: Git List, Junio C Hamano, Patrick Steinhardt; +Cc: Jiang Xin
In-Reply-To: <cover.1702821462.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
If an error occurs during an atomic fetch, a redundant error message
will appear at the end of do_fetch(). It was introduced in b3a804663c
(fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).
Because a failure message is displayed before setting retcode in the
function do_fetch(), calling error() on the err message at the end of
this function may result in redundant or empty error message to be
displayed.
We can remove the redundant error() function, because we know that
the function ref_transaction_abort() never fails. While we can find a
common pattern for calling ref_transaction_abort() by running command
"git grep -A1 ref_transaction_abort", e.g.:
if (ref_transaction_abort(transaction, &error))
error("abort: %s", error.buf);
Following this pattern, we can tolerate the return value of the function
ref_transaction_abort() being changed in the future. We also delay the
output of the err message to the end of do_fetch() to reduce redundant
code. With these changes, the test case "fetch porcelain output
(atomic)" in t5574 will also be fixed.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
builtin/fetch.c | 14 +++++++++-----
t/t5574-fetch-output.sh | 2 +-
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index fd134ba74d..a284b970ef 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1651,7 +1651,7 @@ static int do_fetch(struct transport *transport,
if (atomic_fetch) {
transaction = ref_transaction_begin(&err);
if (!transaction) {
- retcode = error("%s", err.buf);
+ retcode = -1;
goto cleanup;
}
}
@@ -1711,7 +1711,6 @@ static int do_fetch(struct transport *transport,
retcode = ref_transaction_commit(transaction, &err);
if (retcode) {
- error("%s", err.buf);
ref_transaction_free(transaction);
transaction = NULL;
goto cleanup;
@@ -1775,9 +1774,14 @@ static int do_fetch(struct transport *transport,
}
cleanup:
- if (retcode && transaction) {
- ref_transaction_abort(transaction, &err);
- error("%s", err.buf);
+ if (retcode) {
+ if (err.len) {
+ error("%s", err.buf);
+ strbuf_reset(&err);
+ }
+ if (transaction && ref_transaction_abort(transaction, &err) &&
+ err.len)
+ error("%s", err.buf);
}
display_state_release(&display_state);
diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index b579364c47..1400ef14cd 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -90,7 +90,7 @@ test_expect_success 'setup for fetch porcelain output' '
for opt in "" "--atomic"
do
- test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+ test_expect_success "fetch porcelain output ${opt:+(atomic)}" '
test_when_finished "rm -rf porcelain" &&
# Clone and pre-seed the repositories. We fetch references into two
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* [PATCH v4 0/3] Sideband-all demultiplexer fixes
From: Jiang Xin @ 2023-12-17 14:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1696425168.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
# Changes since v3:
Change commit message and comments to make them clear for review.
# range-diff v3...v4
1: e387088da2 ! 1: ff4e5aff2a test-pkt-line: add option parser for unpack-sideband
@@ Commit message
* Use "test-tool pkt-line send-split-sideband" to generate sideband
messages.
- * We can pipe these generated sideband messages to command "test-tool
- pkt-line unpack-sideband" to test packet_reader_read() function.
+ * Pipe these generated sideband messages to command "test-tool pkt-line
+ unpack-sideband" to test packet_reader_read() function.
In order to make a complete test of the packet_reader_read() function,
add option parser for command "test-tool pkt-line unpack-sideband".
- To remove newlines in sideband messages, we can use:
+ * To remove newlines in sideband messages, we can use:
- $ test-tool pkt-line unpack-sideband --chomp-newline
+ $ test-tool pkt-line unpack-sideband --chomp-newline
- To preserve newlines in sideband messages, we can use:
+ * To preserve newlines in sideband messages, we can use:
- $ test-tool pkt-line unpack-sideband --no-chomp-newline
+ $ test-tool pkt-line unpack-sideband --no-chomp-newline
- To parse sideband messages using "demultiplex_sideband()" inside the
- function "packet_reader_read()", we can use:
+ * To parse sideband messages using "demultiplex_sideband()" inside the
+ function "packet_reader_read()", we can use:
- $ test-tool pkt-line unpack-sideband --reader-use-sideband
+ $ test-tool pkt-line unpack-sideband --reader-use-sideband
- Add several new test cases in t0070. Among these test cases, we pipe
+ We also add new example sideband packets in send_split_sideband() and
+ add several new test cases in t0070. Among these test cases, we pipe
output of the "send-split-sideband" subcommand to the "unpack-sideband"
subcommand. We found two issues:
1. The two splitted sideband messages "Hello," and " world!\n" should
- be concatenated together. But when we enabled the function
- "demultiplex_sideband()" to parse sideband messages, the first part
- of the splitted message ("Hello,") is lost.
+ be concatenated together. But when we turn on use_sideband field of
+ reader to parse sideband messages, the first part of the splitted
+ message ("Hello,") is lost.
2. The newline characters in sideband 2 (progress info) and sideband 3
- (error message) should be preserved, but they are also trimmed.
+ (error message) should be preserved, but they are both trimmed.
Will fix the above two issues in subsequent commits.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## t/helper/test-pkt-line.c ##
@@
@@ t/helper/test-pkt-line.c: static void unpack_sideband(void)
+ /*
+ * When the "use_sideband" field of the reader is turned
+ * on, sideband packets other than the payload have been
-+ * parsed and consumed.
++ * parsed and consumed in packet_reader_read(), and only
++ * the payload arrives here.
+ */
+ if (reader.use_sideband) {
+ write_or_die(1, reader.line, reader.pktlen - 1);
@@ t/helper/test-pkt-line.c: static void unpack_sideband(void)
packet_response_end(1);
+ /*
-+ * The unpack_sideband() function above requires a flush
-+ * packet to end parsing.
++ * We use unpack_sideband() to consume packets. A flush packet
++ * is required to end parsing.
+ */
+ packet_flush(1);
+
@@ t/t0070-fundamental.sh: test_expect_success 'missing sideband designator is repo
+ test_cmp expect-err err
+'
+
-+test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
++test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
@@ t/t0070-fundamental.sh: test_expect_success 'missing sideband designator is repo
+ test_cmp expect-err err
+'
+
-+test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
++test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
2: 633bfbac39 ! 2: 5942b74cab pkt-line: memorize sideband fragment in reader
@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband: --chomp-newline (d
test_cmp expect-err err
'
--test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
-+test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
+-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
++test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
3: 2a2da65fac ! 3: dd2e34da16 pkt-line: do not chomp newlines for sideband messages
@@ pkt-line.h: void packet_fflush(FILE *f);
/*
## t/t0070-fundamental.sh ##
-@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli
+@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no
test_cmp expect-err err
'
--test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
-+test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '
+-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
++test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
Jiang Xin (3):
test-pkt-line: add option parser for unpack-sideband
pkt-line: memorize sideband fragment in reader
pkt-line: do not chomp newlines for sideband messages
pkt-line.c | 36 ++++++++++++++++++++----
pkt-line.h | 4 +++
t/helper/test-pkt-line.c | 59 ++++++++++++++++++++++++++++++++++++----
t/t0070-fundamental.sh | 58 +++++++++++++++++++++++++++++++++++++++
4 files changed, 147 insertions(+), 10 deletions(-)
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply
* [PATCH v4 1/3] test-pkt-line: add option parser for unpack-sideband
From: Jiang Xin @ 2023-12-17 14:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1702823801.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
We can use the test helper program "test-tool pkt-line" to test pkt-line
related functions. E.g.:
* Use "test-tool pkt-line send-split-sideband" to generate sideband
messages.
* Pipe these generated sideband messages to command "test-tool pkt-line
unpack-sideband" to test packet_reader_read() function.
In order to make a complete test of the packet_reader_read() function,
add option parser for command "test-tool pkt-line unpack-sideband".
* To remove newlines in sideband messages, we can use:
$ test-tool pkt-line unpack-sideband --chomp-newline
* To preserve newlines in sideband messages, we can use:
$ test-tool pkt-line unpack-sideband --no-chomp-newline
* To parse sideband messages using "demultiplex_sideband()" inside the
function "packet_reader_read()", we can use:
$ test-tool pkt-line unpack-sideband --reader-use-sideband
We also add new example sideband packets in send_split_sideband() and
add several new test cases in t0070. Among these test cases, we pipe
output of the "send-split-sideband" subcommand to the "unpack-sideband"
subcommand. We found two issues:
1. The two splitted sideband messages "Hello," and " world!\n" should
be concatenated together. But when we turn on use_sideband field of
reader to parse sideband messages, the first part of the splitted
message ("Hello,") is lost.
2. The newline characters in sideband 2 (progress info) and sideband 3
(error message) should be preserved, but they are both trimmed.
Will fix the above two issues in subsequent commits.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
t/helper/test-pkt-line.c | 59 ++++++++++++++++++++++++++++++++++++----
t/t0070-fundamental.sh | 58 +++++++++++++++++++++++++++++++++++++++
2 files changed, 112 insertions(+), 5 deletions(-)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index f4d134a145..6b306cf5ca 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -2,6 +2,7 @@
#include "test-tool.h"
#include "pkt-line.h"
#include "write-or-die.h"
+#include "parse-options.h"
static void pack_line(const char *line)
{
@@ -64,12 +65,33 @@ static void unpack(void)
}
}
-static void unpack_sideband(void)
+static void unpack_sideband(int argc, const char **argv)
{
struct packet_reader reader;
- packet_reader_init(&reader, 0, NULL, 0,
- PACKET_READ_GENTLE_ON_EOF |
- PACKET_READ_CHOMP_NEWLINE);
+ int options = PACKET_READ_GENTLE_ON_EOF;
+ int chomp_newline = 1;
+ int reader_use_sideband = 0;
+ const char *const unpack_sideband_usage[] = {
+ "test_tool unpack_sideband [options...]", NULL
+ };
+ struct option cmd_options[] = {
+ OPT_BOOL(0, "reader-use-sideband", &reader_use_sideband,
+ "set use_sideband bit for packet reader (Default: off)"),
+ OPT_BOOL(0, "chomp-newline", &chomp_newline,
+ "chomp newline in packet (Default: on)"),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, "", cmd_options, unpack_sideband_usage,
+ 0);
+ if (argc > 0)
+ usage_msg_opt(_("too many arguments"), unpack_sideband_usage,
+ cmd_options);
+
+ if (chomp_newline)
+ options |= PACKET_READ_CHOMP_NEWLINE;
+ packet_reader_init(&reader, 0, NULL, 0, options);
+ reader.use_sideband = reader_use_sideband;
while (packet_reader_read(&reader) != PACKET_READ_EOF) {
int band;
@@ -79,6 +101,17 @@ static void unpack_sideband(void)
case PACKET_READ_EOF:
break;
case PACKET_READ_NORMAL:
+ /*
+ * When the "use_sideband" field of the reader is turned
+ * on, sideband packets other than the payload have been
+ * parsed and consumed in packet_reader_read(), and only
+ * the payload arrives here.
+ */
+ if (reader.use_sideband) {
+ write_or_die(1, reader.line, reader.pktlen - 1);
+ break;
+ }
+
band = reader.line[0] & 0xff;
if (band < 1 || band > 2)
continue; /* skip non-sideband packets */
@@ -97,15 +130,31 @@ static void unpack_sideband(void)
static int send_split_sideband(void)
{
+ const char *foo = "Foo.\n";
+ const char *bar = "Bar.\n";
const char *part1 = "Hello,";
const char *primary = "\001primary: regular output\n";
const char *part2 = " world!\n";
+ /* Each sideband message has a trailing newline character. */
+ send_sideband(1, 2, foo, strlen(foo), LARGE_PACKET_MAX);
+ send_sideband(1, 2, bar, strlen(bar), LARGE_PACKET_MAX);
+
+ /*
+ * One sideband message is divided into part1 and part2
+ * by the primary message.
+ */
send_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);
packet_write(1, primary, strlen(primary));
send_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);
packet_response_end(1);
+ /*
+ * We use unpack_sideband() to consume packets. A flush packet
+ * is required to end parsing.
+ */
+ packet_flush(1);
+
return 0;
}
@@ -126,7 +175,7 @@ int cmd__pkt_line(int argc, const char **argv)
else if (!strcmp(argv[1], "unpack"))
unpack();
else if (!strcmp(argv[1], "unpack-sideband"))
- unpack_sideband();
+ unpack_sideband(argc - 1, argv + 1);
else if (!strcmp(argv[1], "send-split-sideband"))
send_split_sideband();
else if (!strcmp(argv[1], "receive-sideband"))
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 574de34198..297a7f772e 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -53,4 +53,62 @@ test_expect_success 'missing sideband designator is reported' '
test_i18ngrep "missing sideband" err
'
+test_expect_success 'unpack-sideband: --no-chomp-newline' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --no-chomp-newline <split-sideband >out 2>err &&
+ cat >expect-out <<-EOF &&
+ primary: regular output
+ EOF
+ cat >expect-err <<-EOF &&
+ Foo.
+ Bar.
+ Hello, world!
+ EOF
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
+test_expect_success 'unpack-sideband: --chomp-newline (default)' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --chomp-newline <split-sideband >out 2>err &&
+ printf "primary: regular output" >expect-out &&
+ printf "Foo.Bar.Hello, world!" >expect-err &&
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --reader-use-sideband \
+ --no-chomp-newline <split-sideband >out 2>err &&
+ cat >expect-out <<-EOF &&
+ primary: regular output
+ EOF
+ printf "remote: Foo. \n" >expect-err &&
+ printf "remote: Bar. \n" >>expect-err &&
+ printf "remote: Hello, world! \n" >>expect-err &&
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --reader-use-sideband \
+ --chomp-newline <split-sideband >out 2>err &&
+ printf "primary: regular output" >expect-out &&
+ printf "remote: Foo. \n" >expect-err &&
+ printf "remote: Bar. \n" >>expect-err &&
+ printf "remote: Hello, world! \n" >>expect-err &&
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
test_done
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* [PATCH v4 2/3] pkt-line: memorize sideband fragment in reader
From: Jiang Xin @ 2023-12-17 14:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1702823801.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
When we turn on the "use_sideband" field of the packet_reader,
"packet_reader_read()" will call the function "demultiplex_sideband()"
to parse and consume sideband messages. Sideband fragment which does not
end with "\r" or "\n" will be saved in the sixth parameter "scratch"
and it can be reused and be concatenated when parsing another sideband
message.
In "packet_reader_read()" function, the local variable "scratch" can
only be reused by subsequent sideband messages. But if there is a
payload message between two sideband fragments, the first fragment
which is saved in the local variable "scratch" will be lost.
To solve this problem, we can add a new field "scratch" in
packet_reader to memorize the sideband fragment across different calls
of "packet_reader_read()".
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
pkt-line.c | 5 ++---
pkt-line.h | 3 +++
t/t0070-fundamental.sh | 2 +-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index af83a19f4d..5943777a17 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -592,12 +592,11 @@ void packet_reader_init(struct packet_reader *reader, int fd,
reader->options = options;
reader->me = "git";
reader->hash_algo = &hash_algos[GIT_HASH_SHA1];
+ strbuf_init(&reader->scratch, 0);
}
enum packet_read_status packet_reader_read(struct packet_reader *reader)
{
- struct strbuf scratch = STRBUF_INIT;
-
if (reader->line_peeked) {
reader->line_peeked = 0;
return reader->status;
@@ -620,7 +619,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
break;
if (demultiplex_sideband(reader->me, reader->status,
reader->buffer, reader->pktlen, 1,
- &scratch, &sideband_type))
+ &reader->scratch, &sideband_type))
break;
}
diff --git a/pkt-line.h b/pkt-line.h
index 954eec8719..be1010d34e 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -194,6 +194,9 @@ struct packet_reader {
/* hash algorithm in use */
const struct git_hash_algo *hash_algo;
+
+ /* hold temporary sideband message */
+ struct strbuf scratch;
};
/*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 297a7f772e..275edbf6e7 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -81,7 +81,7 @@ test_expect_success 'unpack-sideband: --chomp-newline (default)' '
test_cmp expect-err err
'
-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
+test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* [PATCH v4 3/3] pkt-line: do not chomp newlines for sideband messages
From: Jiang Xin @ 2023-12-17 14:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1702823801.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
When calling "packet_read_with_status()" to parse pkt-line encoded
packets, we can turn on the flag "PACKET_READ_CHOMP_NEWLINE" to chomp
newline character for each packet for better line matching. But when
receiving data and progress information using sideband, we should turn
off the flag "PACKET_READ_CHOMP_NEWLINE" to prevent mangling newline
characters from data and progress information.
When both the server and the client support "sideband-all" capability,
we have a dilemma that newline characters in negotiation packets should
be removed, but the newline characters in the progress information
should be left intact.
Add new flag "PACKET_READ_USE_SIDEBAND" for "packet_read_with_status()"
to prevent mangling newline characters in sideband messages.
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
pkt-line.c | 31 +++++++++++++++++++++++++++++--
pkt-line.h | 1 +
t/t0070-fundamental.sh | 2 +-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index 5943777a17..e9061e61a4 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -462,8 +462,32 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
}
if ((options & PACKET_READ_CHOMP_NEWLINE) &&
- len && buffer[len-1] == '\n')
- len--;
+ len && buffer[len-1] == '\n') {
+ if (options & PACKET_READ_USE_SIDEBAND) {
+ int band = *buffer & 0xff;
+ switch (band) {
+ case 1:
+ /* Chomp newline for payload */
+ len--;
+ break;
+ case 2:
+ case 3:
+ /*
+ * Do not chomp newline for progress and error
+ * message.
+ */
+ break;
+ default:
+ /*
+ * Bad sideband, let's leave it to
+ * demultiplex_sideband() to catch this error.
+ */
+ break;
+ }
+ } else {
+ len--;
+ }
+ }
buffer[len] = 0;
if (options & PACKET_READ_REDACT_URI_PATH &&
@@ -602,6 +626,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
return reader->status;
}
+ if (reader->use_sideband)
+ reader->options |= PACKET_READ_USE_SIDEBAND;
+
/*
* Consume all progress packets until a primary payload packet is
* received
diff --git a/pkt-line.h b/pkt-line.h
index be1010d34e..a7ff2e2f18 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -85,6 +85,7 @@ void packet_fflush(FILE *f);
#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)
#define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
#define PACKET_READ_REDACT_URI_PATH (1u<<4)
+#define PACKET_READ_USE_SIDEBAND (1u<<5)
int packet_read(int fd, char *buffer, unsigned size, int options);
/*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 275edbf6e7..0d2b7d8d93 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no
test_cmp expect-err err
'
-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
+test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* Re: [PATCH v2] test-lib-functions.sh: fix test_grep fail message wording
From: Shreyansh Paliwal @ 2023-12-17 15:07 UTC (permalink / raw)
To: git; +Cc: five231003, gitster, shreyp135
In-Reply-To: <20231203171956.771-1-shreyanshpaliwalcmsmn@gmail.com>
From: shreyp135 <shreyanshpaliwalcmsmn@gmail.com>
ping.
^ permalink raw reply
* Re: End-of-line comments are prompted with "is not a valid attribute name"
From: Junio C Hamano @ 2023-12-17 17:15 UTC (permalink / raw)
To: D无数; +Cc: git
In-Reply-To: <xmqqsf45nbzm.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> This is totally expected; nothing to see here.
To put it another way, we do not have "end-of-line comment"
(i.e. the leading part of a line has payload, but the line is
chomped in the middle with a comment character and the remainder of
the line is ignored) at all. We have "commented line" (in other
words, a line that is totally commented out and gets ignored).
I think it is very clearly documented in "git help attributes":
A `gitattributes` file is a simple text file that gives
`attributes` to pathnames.
Each line in `gitattributes` file is of form:
pattern attr1 attr2 ...
That is, a pattern followed by an attributes list,
separated by whitespaces. Leading and trailing whitespaces are
ignored. Lines that begin with '#' are ignored. Patterns
that begin with a double quote are quoted in C style.
^ permalink raw reply
* Re: End-of-line comments are prompted with "is not a valid attribute name"
From: D无数 @ 2023-12-17 17:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqo7eoiwcb.fsf@gitster.g>
I see, thanks for the answer.
I was fooled by the VSCode highlighting, but also because I didn't try
to figure out the documentation carefully.
Junio C Hamano <gitster@pobox.com> 于2023年12月18日周一 01:15写道：
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > This is totally expected; nothing to see here.
>
> To put it another way, we do not have "end-of-line comment"
> (i.e. the leading part of a line has payload, but the line is
> chomped in the middle with a comment character and the remainder of
> the line is ignored) at all. We have "commented line" (in other
> words, a line that is totally commented out and gets ignored).
>
> I think it is very clearly documented in "git help attributes":
>
> A `gitattributes` file is a simple text file that gives
> `attributes` to pathnames.
>
> Each line in `gitattributes` file is of form:
>
> pattern attr1 attr2 ...
>
> That is, a pattern followed by an attributes list,
> separated by whitespaces. Leading and trailing whitespaces are
> ignored. Lines that begin with '#' are ignored. Patterns
> that begin with a double quote are quoted in C style.
>
^ permalink raw reply
* Unable to install git-2.43.0 from source on macOS Big Sur 11.7.10
From: Jonathan Abrams @ 2023-12-17 22:54 UTC (permalink / raw)
To: git@vger.kernel.org
Hello,
I am trying to install git-2.43.0 from source on macOS Big Sur 11.7.10. I have Xcode 13.2.1 installed. I have read https://github.com/git/git/blob/master/INSTALL.
The command that will not complete successfully is sudo make all doc. The Terminal output before the error is as follows.
--
AR libgit.a
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libgit.a(zlib-uncompress2.o) has no symbols
CC xdiff/xdiffi.o
CC xdiff/xemit.o
CC xdiff/xhistogram.o
CC xdiff/xmerge.o
CC xdiff/xpatience.o
CC xdiff/xprepare.o
CC xdiff/xutils.o
AR xdiff/lib.a
CC reftable/basics.o
CC reftable/error.o
CC reftable/block.o
CC reftable/blocksource.o
CC reftable/iter.o
CC reftable/publicbasics.o
CC reftable/merged.o
CC reftable/pq.o
CC reftable/reader.o
CC reftable/record.o
CC reftable/refname.o
CC reftable/generic.o
CC reftable/stack.o
CC reftable/tree.o
CC reftable/writer.o
AR reftable/libreftable.a
LINK git-daemon
Undefined symbols for architecture x86_64:
"_libiconv", referenced from:
_precompose_utf8_readdir in libgit.a(precompose_utf8.o)
_reencode_string_iconv in libgit.a(utf8.o)
"_libiconv_close", referenced from:
_precompose_string_if_needed in libgit.a(precompose_utf8.o)
_precompose_utf8_closedir in libgit.a(precompose_utf8.o)
_reencode_string_len in libgit.a(utf8.o)
"_libiconv_open", referenced from:
_precompose_string_if_needed in libgit.a(precompose_utf8.o)
_precompose_utf8_opendir in libgit.a(precompose_utf8.o)
_reencode_string_len in libgit.a(utf8.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [git-daemon] Error 1
--
Does anyone reading this know how to resolve this error?
Thank you,
Jonathan S. Abrams
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox