* Re: [PATCH v2] tests: modernize the test script t0010-racy-git.sh
From: Junio C Hamano @ 2024-02-29 23:36 UTC (permalink / raw)
To: Eric Sunshine
Cc: Aryan Gupta via GitGitGadget, git, Patrick Steinhardt,
Michal Suchánek, Jean-Noël AVILA, Kristoffer Haugsbakk,
Aryan Gupta
In-Reply-To: <CAPig+cQ5m86=pLTpFrik0xS6XPyK4tZQx_wkc1xh2r9WDFkhuQ@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Thu, Feb 29, 2024 at 6:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>> So, we may want to do it more like this, perhaps?
>>
>> test_expect_success "Racy GIT trial #$trial part A" '
>> rm -f .git/index &&
>> echo frotz >infocom &&
>> git update-index --add infocom &&
>> echo xyzzy >infocom &&
>>
>> files=$(git diff-files -p) &&
>> test "" != "$files"
>> '
>
> If taking it to this extent, then the modernized version of the last
> couple lines would be:
>
> git diff-files -p >out &&
> test_file_not_empty out
Yes. The modern style seems to prefer temporary files over
variables; the reason probably is because it tends to be easier to
remotely post-mortem?
^ permalink raw reply
* Re: [PATCH v2] tests: modernize the test script t0010-racy-git.sh
From: Eric Sunshine @ 2024-02-29 23:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Aryan Gupta via GitGitGadget, git, Patrick Steinhardt,
Michal Suchánek, Jean-Noël AVILA, Kristoffer Haugsbakk,
Aryan Gupta
In-Reply-To: <xmqqle72c17i.fsf@gitster.g>
On Thu, Feb 29, 2024 at 6:14 PM Junio C Hamano <gitster@pobox.com> wrote:
> So, we may want to do it more like this, perhaps?
>
> test_expect_success "Racy GIT trial #$trial part A" '
> rm -f .git/index &&
> echo frotz >infocom &&
> git update-index --add infocom &&
> echo xyzzy >infocom &&
>
> files=$(git diff-files -p) &&
> test "" != "$files"
> '
If taking it to this extent, then the modernized version of the last
couple lines would be:
git diff-files -p >out &&
test_file_not_empty out
^ permalink raw reply
* Re: [PATCH v5 3/9] trailer: prepare to expose functions as part of API
From: Junio C Hamano @ 2024-02-29 23:21 UTC (permalink / raw)
To: Linus Arver
Cc: Christian Couder, Linus Arver via GitGitGadget, git,
Christian Couder, Emily Shaffer, Josh Steadmon, Randall S. Becker,
Kristoffer Haugsbakk
In-Reply-To: <owlyttlq529h.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
>> Nit: this patch and the next one will become commits, so perhaps:
>>
>> s/In the next patch/In a following commit/
>
> TBH I've always wondered whether "patch" or "commit" matters --- I've
> seen examples of patch series that referred to "commits" instead of
> "patches", and vice versa. I was hoping to hear an opinion on this, so
> I'm happy to see (and apply) your suggestion. Thanks.
I think it is just fine to use either; sticking to one you pick
consistently in the same series would have value. If you prefer
commit, then fine. If you like patch, that's fine too.
^ permalink raw reply
* Re: [PATCH v2] tests: modernize the test script t0010-racy-git.sh
From: Junio C Hamano @ 2024-02-29 23:14 UTC (permalink / raw)
To: Aryan Gupta via GitGitGadget
Cc: git, Patrick Steinhardt, Michal Suchánek,
Jean-Noël AVILA, Kristoffer Haugsbakk, Eric Sunshine,
Aryan Gupta
In-Reply-To: <pull.1675.v2.git.1709243831190.gitgitgadget@gmail.com>
"Aryan Gupta via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Aryan Gupta <garyan447@gmail.com>
>
> Modernize the formatting of the test script to align with current
> standards and improve its overall readability.
OK.
> diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh
> index 837c8b7228b..2ceac06c9c2 100755
> --- a/t/t0010-racy-git.sh
> +++ b/t/t0010-racy-git.sh
> @@ -1,6 +1,6 @@
> #!/bin/sh
>
> -test_description='racy GIT'
> +test_description='racy git'
This does not look like updating formatting to the current standard,
or improving readability, though.
> - test_expect_success \
> - "Racy GIT trial #$trial part A" \
> - 'test "" != "$files"'
> + test_expect_success "Racy git trial #$trial part A" '
> + test "" != "$files"
> + '
> ...
> - test_expect_success \
> - "Racy GIT trial #$trial part B" \
> - 'test "" != "$files"'
> -
> + test_expect_success "Racy git trial #$trial part B" '
> + test "" != "$files"
> + '
These are not wrong per-se, but if we are updating, I wonder if we
want to get rid of statements outside the test_expect_success block,
not just the formatting of individual test_expect_success tests.
Looking at an iteration of the loop, the executable statements from
here ...
rm -f .git/index
echo frotz >infocom
git update-index --add infocom
echo xyzzy >infocom
files=$(git diff-files -p)
... to here is what we would make part of one test, namely ...
test_expect_success \
"Racy GIT trial #$trial part A" \
'test "" != "$files"'
... this one. Then
sleep 1
is a cricial delay to have before the next test, which we may want
to have outside to make it clear what is going on to readers. But
the following parts ...
echo xyzzy >cornerstone
git update-index --add cornerstone
files=$(git diff-files -p)
... up to here, we would make part of the next test ...
test_expect_success \
"Racy GIT trial #$trial part B" \
'test "" != "$files"'
... in the modern style.
So, we may want to do it more like this, perhaps?
test_expect_success "Racy GIT trial #$trial part A" '
rm -f .git/index &&
echo frotz >infocom &&
git update-index --add infocom &&
echo xyzzy >infocom &&
files=$(git diff-files -p) &&
test "" != "$files"
'
sleep 1
test_expect_success "Racy GIT trial #$trial part B" '
echo xyzzy >cornerstone &&
git update-index --add cornerstone &&
files=$(git diff-files -p) &&
test "" != "$files"
'
An added benefit of the more modern style to place as much as
possible to &&-chain in test_expect_success block is that we would
catch breakage in "git update-index" and other things used to set-up
the test as well. With the original loop, breakages in things
outside the test_expect_success blocks will go unchecked.
^ permalink raw reply
* Re: [PATCH v5 9/9] format_trailers_from_commit(): indirectly call trailer_info_get()
From: Linus Arver @ 2024-02-29 23:00 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Kristoffer Haugsbakk
In-Reply-To: <CAP8UFD0JV8VEC-MDu86Mzrya9G7JBZaP2vXjFKwcKddEkh=y5g@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> This is another preparatory refactor to unify the trailer formatters.
>>
>> Instead of calling trailer_info_get() directly, call parse_trailers()
>> which already calls trailer_info_get(). This change is a NOP because
>> format_trailer_info() only looks at the "trailers" string array, not the
>> trailer_item objects which parse_trailers() populates.
>
> Is the extra processing done by parse_trailers() compared to
> trailer_info_get() impacting performance?
I was going to answer this now but I see that you've already reached the
same conclusion as me further below. ;)
> Also when looking only at the patch, it's a bit difficult to
> understand that the "trailers" string array is the `char **trailers`
> field in `struct trailer_info` and that the trailer_item objects are
> the elements of the `struct list_head *head` linked list. It could
> also be confusing because the patch is adding a new 'trailers'
> variable with `LIST_HEAD(trailers);`. So a few more details could help
> understand what's going on.
Makes sense. Admittedly, this is one thing that did bother me at some
point but which I forgot about as I got more familiar with my own patch.
I will avoid shadowing the "trailers" word (and maybe at least add a
suffix or prefix to disambiguate it).
>> In a future patch, we'll change format_trailer_info() to use the parsed
>> trailer_item objects instead of the string array.
>
> Ok, so I guess the possible performance issue would disappear then, as
> populating the trailer_item objects will be useful.
Yep, exactly. In the larger series (some 20? 30?) commits down the road
in my local tree, I have it so that we remove `char **trailers`
entirely (because we should be using the (smarter) trailer_item objects,
not raw strings, where possible).
^ permalink raw reply
* Re: [PATCH 1/1] Documentation/user-manual.txt: example for generating object hashes
From: Junio C Hamano @ 2024-02-29 22:57 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git list
In-Reply-To: <gha5nigaq0.fsf@gouders.net>
Dirk Gouders <dirk@gouders.net> writes:
>> I'd rather not to waste readers' attention to historical wart.
>
> Yes, but -- I should have mentioned it -- the document itself suggests
> to read the initial commit.
Ahh, yes, we'd need to hedge that part. Good thinking.
I am still not sure if the first hunk below is a good idea or it is
too much detail. The second hunk may be worth doing.
Thanks.
Documentation/user-manual.txt | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git c/Documentation/user-manual.txt w/Documentation/user-manual.txt
index 6433903491..1027055784 100644
--- c/Documentation/user-manual.txt
+++ w/Documentation/user-manual.txt
@@ -4093,7 +4093,8 @@ that not only specifies their type, but also provides size information
about the data in the object. It's worth noting that the SHA-1 hash
that is used to name the object is the hash of the original data
plus this header, so `sha1sum` 'file' does not match the object name
-for 'file'.
+for 'file' (the earliest versions of Git hashed slightly differently
+but the conclusion is still the same).
As a result, the general consistency of an object can always be tested
independently of the contents or the type of the object: all objects can
@@ -4123,7 +4124,8 @@ $ git switch --detach e83c5163
----------------------------------------------------
The initial revision lays the foundation for almost everything Git has
-today, but is small enough to read in one sitting.
+today (even though details may differ in a few places), but is small
+enough to read in one sitting.
Note that terminology has changed since that revision. For example, the
README in that revision uses the word "changeset" to describe what we
^ permalink raw reply related
* Re: [PATCH v5 5/9] trailer: start preparing for formatting unification
From: Linus Arver @ 2024-02-29 22:53 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Kristoffer Haugsbakk
In-Reply-To: <CAP8UFD3KbbRApC3ktgegsi_oBDpzX_89v0QGvWoHQ057hKjbbg@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> Currently there are two functions for formatting trailers in
>> <trailer.h>:
>>
>> void format_trailers(const struct process_trailer_options *,
>> struct list_head *trailers, FILE *outfile);
>>
>> void format_trailers_from_commit(struct strbuf *out, const char *msg,
>> const struct process_trailer_options *opts);
>>
>> and although they are similar enough (even taking the same
>> process_trailer_options struct pointer) they are used quite differently.
>> One might intuitively think that format_trailers_from_commit() builds on
>> top of format_trailers(), but this is not the case. Instead
>> format_trailers_from_commit() calls format_trailer_info() and
>> format_trailers() is never called in that codepath.
>>
>> This is a preparatory refactor to help us deprecate format_trailers() in
>> favor of format_trailer_info() (at which point we can rename the latter
>> to the former). When the deprecation is complete, both
>> format_trailers_from_commit(), and the interpret-trailers builtin will
>> be able to call into the same helper function (instead of
>> format_trailers() and format_trailer_info(), respectively). Unifying the
>> formatters is desirable because it simplifies the API.
>>
>> Reorder parameters for format_trailers_from_commit() to prefer
>>
>> const struct process_trailer_options *opts
>>
>> as the first parameter, because these options are intimately tied to
>> formatting trailers. And take
>>
>> struct strbuf *out
>>
>> last, because it's an "out parameter" (something that the caller wants
>> to use as the output of this function).
>
> Here also I think the subject could be more specific like for example:
>
> "trailer: reorder format_trailers_from_commit() parameters"
Applied, thanks.
>> diff --git a/trailer.c b/trailer.c
>> index d23afa0a65c..5025be97899 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -1083,10 +1083,10 @@ void trailer_info_release(struct trailer_info *info)
>> free(info->trailers);
>> }
>>
>> -static void format_trailer_info(struct strbuf *out,
>> +static void format_trailer_info(const struct process_trailer_options *opts,
>> const struct trailer_info *info,
>> const char *msg,
>> - const struct process_trailer_options *opts)
>> + struct strbuf *out)
>
> Ok, so it's not just format_trailers_from_commit() parameters that are
> reordered, but also format_trailer_info() parameters. It would be nice
> if the commit message mentioned it.
Agreed. Will do.
^ permalink raw reply
* Re: [PATCH 1/1] Documentation/user-manual.txt: example for generating object hashes
From: Dirk Gouders @ 2024-02-29 22:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list
In-Reply-To: <xmqqil27c5p1.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Dirk Gouders <dirk@gouders.net> writes:
>
>> If someone spends the time to work through the documentation, the
>> subject "hashes" can lead to contradictions:
>>
>> The README of the initial commit states hashes are generated from
>> compressed data (which changed very soon), whereas
>> Documentation/user-manual.txt says they are generated from original
>> data.
>>
>> Don't give doubts a chance: clarify this and present a simple example
>> on how object hashes can be generated manually.
>
> I'd rather not to waste readers' attention to historical wart.
Yes, but -- I should have mentioned it -- the document itself suggests
to read the initial commit.
But I don't mean to argue about that, perhaps I digged to deep into
details.
>> @@ -4095,6 +4095,39 @@ that is used to name the object is the hash of the original data
>> plus this header, so `sha1sum` 'file' does not match the object name
>> for 'file'.
>
> The paragraph above (part of it is hidden before the hunk) clearly
> states what the naming rules are. We hash the original and then
> compress. If I use an implementation of Git that drives the zlib at
> compression level 1, and if you clone from my repository with
> another implementation of Git whose zlib is driven at compression
> level 9, our .git/objects/01/2345...90 files may not be identical,
> but when uncompressed they should store the same contents, so "hash
> then compress" is the only sensible choice that is not affected by
> the compression to give stable names to objects.
Thank your for that detail.
>> +Starting with the initial commit, hashing was done on the compressed
>> +data and the file README of that commit explicitely states this:
>> +
>> +"The SHA1 hash is always the hash of the _compressed_ object, not the
>> +original one."
>> +
>> +This changed soon after that with commit
>> +d98b46f8d9a3 (Do SHA1 hash _before_ compression.). Unfortunately, the
>> +commit message doesn't provide the detailed reasoning.
>
> These three are about Git development history, which by itself may
> be of interest for some people, but the main target audience of the
> user-manual is probably different from them. They may be interested
> to learn how Git works, but it is only to feel that they understand
> how the "magic" things Git does, like "a cryptographic hash of
> contents is enough to uniquely identify the contents being tracked",
> works well to trust their precious contents [*].
>
> Side note:
> https://lore.kernel.org/git/Pine.LNX.4.58.0504200144260.6467@ppc970.osdl.org/
> explains the reason behind the change to those who did not find
> it obvious.
>
> FYI, another "breaking" change we did earlier in the history of the
> project was to update the sort order of paths in tree objects. We
> do not need to confuse readers by talking about the original and
> updated sort order. The only thing they need, when they want to get
> the feeling that they understand how things work, is the description
> of how things work in the version of Git they have ready access to.
> Historical mistakes we made, corrections we made and why, are
> certainly of interest but not for the target audience of this
> document.
Again thank you, very interesting reading.
> On the other hand, ...
>
>> +The following is a short example that demonstrates how hashes can be
>> +generated manually:
>> +
>> +Let's asume a small text file with the content "Hello git.\n"
>> +-------------------------------------------------
>> +$ cat > hello.txt <<EOF
>> +Hello git.
>> +EOF
>> +-------------------------------------------------
>> +
>> +We can now manually generate the hash `git` would use for this file:
>> +
>> +- The object we want the hash for is of type "blob" and its size is
>> + 11 bytes.
>> +
>> +- Prepend the object header to the file content and feed this to
>> + sha1sum(1):
>> +
>> +-------------------------------------------------
>> +$ printf "blob 11\0" | cat - hello.txt | sha1sum
>> +7217614ba6e5f4e7db2edaa2cdf5fb5ee4358b57 .
>> +-------------------------------------------------
>> +
>
> ... something like the above (modulo coding style) would be a useful
> addition to help those who want to convince themselves they
> understand how (some parts of) Git works under the hood, and I think
> it would be a welcome addition to some subset of such readers (the
> rest of the world may feel it is way too much detail, though).
>
> I would draw the line between this one and a similar description and
> demonstration of historical mistakes, which is not as relevant as
> how things work in the current system. In other words, to me, it is
> OK to dig a bit deep to show how the current scheme works but it is
> way too much to do the same for versions of the system that do not
> exist anymore.
>
> But others may draw the line differently and consider even the above
> a bit too much detail, which is a position I would also accept.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH v5 3/9] trailer: prepare to expose functions as part of API
From: Linus Arver @ 2024-02-29 22:33 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Kristoffer Haugsbakk
In-Reply-To: <CAP8UFD1dE2EiSxohose6U9SGn+zeHPyVB=KZ2xdQi-v-a8d1XQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> In the next patch, we will move "process_trailers" from trailer.c to
>> builtin/interpret-trailers.c. That move will necessitate the growth of
>> the trailer.h API, forcing us to expose some additional functions in
>> trailer.h.
>
> Nit: actually this patch renames process_trailers() to
> interpret_trailers() so the function that will be moved will be
> interpret_trailers().
Oops, fixed locally.
> Nit: this patch and the next one will become commits, so perhaps:
>
> s/In the next patch/In a following commit/
TBH I've always wondered whether "patch" or "commit" matters --- I've
seen examples of patch series that referred to "commits" instead of
"patches", and vice versa. I was hoping to hear an opinion on this, so
I'm happy to see (and apply) your suggestion. Thanks.
>> Rename relevant functions so that they include the term "trailer" in
>> their name, so that clients of the API will be able to easily identify
>> them by their "trailer" moniker, just like all the other functions
>> already exposed by trailer.h.
>
> Except that "process_trailers()" already contains "trailer" but will
> still be renamed by this patch to "interpret_trailers()". So I think
> it might be nice to explain a bit why renaming process_trailers() to
> interpret_trailers() makes sense too.
I will add something like:
Rename process_trailers() to interpret_trailers(), because it
matches the name for the builtin command of the same name
(git-interpret-trailers), which is the sole user of
process_trailers().
> Also I think the subject, "trailer: prepare to expose functions as
> part of API" could be more explicit about what the patch is actually
> doing, like perhaps "trailer: rename functions to use 'trailer'".
Applied.
> In general, when there is a patch called "prepare to do X", then we
> might expect a following patch called something like "actually do X".
> But there isn't any patch in the series named like "trailer: expose
> functions as part of API".
Sounds like a very sensible rule. IOW, leave the detailed explanation to
the commit message and use the subject line only for the most obvious
explanation of what's going on in the patch. +1
>> Take the opportunity to start putting trailer processing options (opts)
>> as the first parameter. This will be the pattern going forward in this
>> series.
>
> It's interesting to know that this will be the pattern going forward
> in the series, but that doesn't quite tell why it's a good idea to do
> it.
>
> So I think it might be nice to repeat an explanation similar to the
> one you give in "trailer: start preparing for formatting unification"
> for format_trailers_from_commit():
>
> "Reorder parameters for format_trailers_from_commit() to prefer
>
> const struct process_trailer_options *opts
>
> as the first parameter, because these options are intimately tied to
> formatting trailers."
>
> And maybe also say that parameters like `FILE *outfile` should be last
> because they are some kind of 'out' parameters.
SGTM, will do.
>> diff --git a/trailer.c b/trailer.c
>> index f74915bd8cd..916175707d8 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -163,12 +163,12 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
>> fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
>> }
>>
>> -static void print_all(FILE *outfile, struct list_head *head,
>> - const struct process_trailer_options *opts)
>> +static void format_trailers(const struct process_trailer_options *opts,
>> + struct list_head *trailers, FILE *outfile)
>
> This also renames `struct list_head *head` to `struct list_head
> *trailers`. I think it would be nice if the commit message could talk
> a bit about these renames too.
Ah nice catch. Will do.
Really appreciate the quality of your reviews, thanks so much! :)
^ permalink raw reply
* Re: [PATCH v2] tests: modernize the test script t0010-racy-git.sh
From: Eric Sunshine @ 2024-02-29 22:19 UTC (permalink / raw)
To: Aryan Gupta via GitGitGadget
Cc: git, Patrick Steinhardt [ ], Michal Suchánek [ ],
Jean-Noël AVILA [ ], Aryan Gupta
In-Reply-To: <pull.1675.v2.git.1709243831190.gitgitgadget@gmail.com>
On Thu, Feb 29, 2024 at 4:57 PM Aryan Gupta via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Modernize the formatting of the test script to align with current
> standards and improve its overall readability.
>
> Signed-off-by: Aryan Gupta <garyan447@gmail.com>
> ---
> diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh
> @@ -16,19 +16,18 @@ do
> files=$(git diff-files -p)
> - test_expect_success \
> - "Racy GIT trial #$trial part A" \
> - 'test "" != "$files"'
> + test_expect_success "Racy git trial #$trial part A" '
> + test "" != "$files"
> + '
This version (v2) addresses my review comments about v1. Thanks.
^ permalink raw reply
* [PATCH v2] tests: modernize the test script t0010-racy-git.sh
From: Aryan Gupta via GitGitGadget @ 2024-02-29 21:57 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt [ ], Michal Suchánek [ ],
Jean-Noël AVILA [ ]
In-Reply-To: <pull.1675.git.1709209435242.gitgitgadget@gmail.com>
From: Aryan Gupta <garyan447@gmail.com>
Modernize the formatting of the test script to align with current
standards and improve its overall readability.
Signed-off-by: Aryan Gupta <garyan447@gmail.com>
---
[GSOC][PATCH] Modernize a test script
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1675
Range-diff vs v1:
1: 06377119ea3 ! 1: fa381d0b57a tests: modernize the test script t0010-racy-git.sh
@@
## Metadata ##
-Author: aryangupta701 <garyan447@gmail.com>
+Author: Aryan Gupta <garyan447@gmail.com>
## Commit message ##
tests: modernize the test script t0010-racy-git.sh
@@ t/t0010-racy-git.sh: do
- test_expect_success \
- "Racy GIT trial #$trial part A" \
- 'test "" != "$files"'
-+ test_expect_success 'Racy git trial #$trial part A' '
++ test_expect_success "Racy git trial #$trial part A" '
+ test "" != "$files"
+ '
@@ t/t0010-racy-git.sh: do
- "Racy GIT trial #$trial part B" \
- 'test "" != "$files"'
-
-+ test_expect_success 'Racy git trial #$trial part B' '
++ test_expect_success "Racy git trial #$trial part B" '
+ test "" != "$files"
+ '
done
t/t0010-racy-git.sh | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh
index 837c8b7228b..2ceac06c9c2 100755
--- a/t/t0010-racy-git.sh
+++ b/t/t0010-racy-git.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='racy GIT'
+test_description='racy git'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
@@ -16,19 +16,18 @@ do
echo xyzzy >infocom
files=$(git diff-files -p)
- test_expect_success \
- "Racy GIT trial #$trial part A" \
- 'test "" != "$files"'
+ test_expect_success "Racy git trial #$trial part A" '
+ test "" != "$files"
+ '
sleep 1
echo xyzzy >cornerstone
git update-index --add cornerstone
files=$(git diff-files -p)
- test_expect_success \
- "Racy GIT trial #$trial part B" \
- 'test "" != "$files"'
-
+ test_expect_success "Racy git trial #$trial part B" '
+ test "" != "$files"
+ '
done
test_done
base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/1] Documentation/user-manual.txt: example for generating object hashes
From: Junio C Hamano @ 2024-02-29 21:37 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git list
In-Reply-To: <a3902dad424983a4f0dfcda68e0b8bf64a0b2113.1709240261.git.dirk@gouders.net>
Dirk Gouders <dirk@gouders.net> writes:
> If someone spends the time to work through the documentation, the
> subject "hashes" can lead to contradictions:
>
> The README of the initial commit states hashes are generated from
> compressed data (which changed very soon), whereas
> Documentation/user-manual.txt says they are generated from original
> data.
>
> Don't give doubts a chance: clarify this and present a simple example
> on how object hashes can be generated manually.
I'd rather not to waste readers' attention to historical wart.
> @@ -4095,6 +4095,39 @@ that is used to name the object is the hash of the original data
> plus this header, so `sha1sum` 'file' does not match the object name
> for 'file'.
The paragraph above (part of it is hidden before the hunk) clearly
states what the naming rules are. We hash the original and then
compress. If I use an implementation of Git that drives the zlib at
compression level 1, and if you clone from my repository with
another implementation of Git whose zlib is driven at compression
level 9, our .git/objects/01/2345...90 files may not be identical,
but when uncompressed they should store the same contents, so "hash
then compress" is the only sensible choice that is not affected by
the compression to give stable names to objects.
> +Starting with the initial commit, hashing was done on the compressed
> +data and the file README of that commit explicitely states this:
> +
> +"The SHA1 hash is always the hash of the _compressed_ object, not the
> +original one."
> +
> +This changed soon after that with commit
> +d98b46f8d9a3 (Do SHA1 hash _before_ compression.). Unfortunately, the
> +commit message doesn't provide the detailed reasoning.
These three are about Git development history, which by itself may
be of interest for some people, but the main target audience of the
user-manual is probably different from them. They may be interested
to learn how Git works, but it is only to feel that they understand
how the "magic" things Git does, like "a cryptographic hash of
contents is enough to uniquely identify the contents being tracked",
works well to trust their precious contents [*].
Side note:
https://lore.kernel.org/git/Pine.LNX.4.58.0504200144260.6467@ppc970.osdl.org/
explains the reason behind the change to those who did not find
it obvious.
FYI, another "breaking" change we did earlier in the history of the
project was to update the sort order of paths in tree objects. We
do not need to confuse readers by talking about the original and
updated sort order. The only thing they need, when they want to get
the feeling that they understand how things work, is the description
of how things work in the version of Git they have ready access to.
Historical mistakes we made, corrections we made and why, are
certainly of interest but not for the target audience of this
document.
On the other hand, ...
> +The following is a short example that demonstrates how hashes can be
> +generated manually:
> +
> +Let's asume a small text file with the content "Hello git.\n"
> +-------------------------------------------------
> +$ cat > hello.txt <<EOF
> +Hello git.
> +EOF
> +-------------------------------------------------
> +
> +We can now manually generate the hash `git` would use for this file:
> +
> +- The object we want the hash for is of type "blob" and its size is
> + 11 bytes.
> +
> +- Prepend the object header to the file content and feed this to
> + sha1sum(1):
> +
> +-------------------------------------------------
> +$ printf "blob 11\0" | cat - hello.txt | sha1sum
> +7217614ba6e5f4e7db2edaa2cdf5fb5ee4358b57 .
> +-------------------------------------------------
> +
... something like the above (modulo coding style) would be a useful
addition to help those who want to convince themselves they
understand how (some parts of) Git works under the hood, and I think
it would be a welcome addition to some subset of such readers (the
rest of the world may feel it is way too much detail, though).
I would draw the line between this one and a similar description and
demonstration of historical mistakes, which is not as relevant as
how things work in the current system. In other words, to me, it is
OK to dig a bit deep to show how the current scheme works but it is
way too much to do the same for versions of the system that do not
exist anymore.
But others may draw the line differently and consider even the above
a bit too much detail, which is a position I would also accept.
Thanks.
^ permalink raw reply
* [PATCH 1/1] Documentation/user-manual.txt: example for generating object hashes
From: Dirk Gouders @ 2024-02-29 13:05 UTC (permalink / raw)
To: git list
In-Reply-To: <cover.1709240261.git.dirk@gouders.net>
If someone spends the time to work through the documentation, the
subject "hashes" can lead to contradictions:
The README of the initial commit states hashes are generated from
compressed data (which changed very soon), whereas
Documentation/user-manual.txt says they are generated from original
data.
Don't give doubts a chance: clarify this and present a simple example
on how object hashes can be generated manually.
Signed-off-by: Dirk Gouders <dirk@gouders.net>
---
Documentation/user-manual.txt | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index 6433903491..8dfb81e045 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -4095,6 +4095,39 @@ that is used to name the object is the hash of the original data
plus this header, so `sha1sum` 'file' does not match the object name
for 'file'.
+Starting with the initial commit, hashing was done on the compressed
+data and the file README of that commit explicitely states this:
+
+"The SHA1 hash is always the hash of the _compressed_ object, not the
+original one."
+
+This changed soon after that with commit
+d98b46f8d9a3 (Do SHA1 hash _before_ compression.). Unfortunately, the
+commit message doesn't provide the detailed reasoning.
+
+The following is a short example that demonstrates how hashes can be
+generated manually:
+
+Let's asume a small text file with the content "Hello git.\n"
+-------------------------------------------------
+$ cat > hello.txt <<EOF
+Hello git.
+EOF
+-------------------------------------------------
+
+We can now manually generate the hash `git` would use for this file:
+
+- The object we want the hash for is of type "blob" and its size is
+ 11 bytes.
+
+- Prepend the object header to the file content and feed this to
+ sha1sum(1):
+
+-------------------------------------------------
+$ printf "blob 11\0" | cat - hello.txt | sha1sum
+7217614ba6e5f4e7db2edaa2cdf5fb5ee4358b57 .
+-------------------------------------------------
+
As a result, the general consistency of an object can always be tested
independently of the contents or the type of the object: all objects can
be validated by verifying that (a) their hashes match the content of the
--
2.43.0
^ permalink raw reply related
* [PATCH 0/1] Documentation/user-manual.txt: try to clarify on object hashes
From: Dirk Gouders @ 2024-02-29 20:57 UTC (permalink / raw)
To: git list
I'm not sure if such patches are welcome -- they could cause more work
than it's worth it. On the other hand, the contradiction irritaded me
and I had to dig a bit and check it by example and writing this down
could perhaps help others who also stumble over this.
If someone knows the exact reasoning why the hashing changed from
_after_ compressed to _before_ compressed, we could perhaps add that,
too?
Dirk Gouders (1):
Documentation/user-manual.txt: example for generating object hashes
Documentation/user-manual.txt | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
--
2.43.0
^ permalink raw reply
* Re: [PATCH] setup: clarify TODO comment about ignoring core.bare
From: Ghanshyam Thakkar @ 2024-02-29 20:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, newren, christian.couder
In-Reply-To: <xmqqsf1bf5ew.fsf@gitster.g>
On Fri Mar 1, 2024 at 12:45 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > /*
> > - * TODO: heed core.bare from config file in templates if no
> > - * command-line override given
> > + * Note: The below line simply checks the presence of worktree (the
> > + * simplification of which is given after the line) and core.bare from
> > + * config file is not taken into account when deciding if the worktree
> > + * should be created or not, even if no command line override given.
> > + * That is intentional. Therefore, if in future we want to heed
> > + * core.bare from config file, we should do it before we create any
> > + * subsequent directories for worktree or repo because until this point
> > + * they should already be created.
> > */
> > is_bare_repository_cfg = prev_bare_repository || !work_tree;
>
> I do not recall the discussion; others may want to discuss if the
> change above is desirable, before I come back to the topic later.
>
> But I see this long comment totally unnecessary and distracting.
>
> > - /* TODO (continued):
> > + /* Note (continued):
> > *
> > - * Unfortunately, the line above is equivalent to
> > + * The line above is equivalent to
> > * is_bare_repository_cfg = !work_tree;
> > - * which ignores the config entirely even if no `--[no-]bare`
> > - * command line option was present.
> > *
> > * To see why, note that before this function, there was this call:
> > * prev_bare_repository = is_bare_repository()
>
> If it can be proven that the assignment can be simplified to lose
> the "prev_bare_repository ||" part, then the above comment can be
> used as part of the proposed log message for a commit that makes
> such a change. There is no reason to leave such a long comment to
> leave the more complex "A || B" expression when it can be simplified
> to "B", no?
I agree. In fact, we can remove the prev_bare_repository variable altogether
as it was used solely for the "A || B" expression. Initially this
function used to be in builtin/init-db.c and shared with
builtin/clone.c. In moving to setup.c, an unnecessary global variable
equivalent to prev_bare_repository was removed and therefore
prev_bare_repository was intruduced to not hinder the future possibility
of intruducing the (core.bare from config) feature which might have been
the global variables partial intent[1]. Therefore, I kept the original
expression.
However, in the same series that this was introduced, it was acknowledged
by Elijah[2] that create_default_files() would possibly be too late to heed
core.bare. And indeed it is, as the directories for the worktree or repo
are already created by this point. Therefore, prev_bare_repository does
not seem to have any usecase with/without supporting core.bare from
config.
[1]:
https://lore.kernel.org/git/xmqqsf1bf5ew.fsf@gitster.g/T/#m64125dd80d04ae177944434e7092522325b374c9
[2]: 0f7443bdc7 (init-db: document existing bug with core.bare in
template config, 2023-05-16)
Thanks.
^ permalink raw reply
* Re: [PATCH] branch: adjust documentation
From: Dragan Simic @ 2024-02-29 20:09 UTC (permalink / raw)
To: Rubén Justo; +Cc: Junio C Hamano, git
In-Reply-To: <d456a80d-5c2b-421b-a849-3c7cce288c6c@gmail.com>
On 2024-02-29 21:02, Rubén Justo wrote:
> On Thu, Feb 29, 2024 at 11:33:24AM -0800, Junio C Hamano wrote:
>
>> Do not forget that the objective of the larger-picture-revamping of
>> this page is to make the description of each option self-contained.
>> Similarity between -m's description and -c's description does not
>> count as being uselessly repetitive.
>
> OK. I was not considering this.
I'd also like to avoid repetition, but if we want to have self-contained
command descriptions (which is good), some repetition is unavoidable.
In fact, we can see what avoiding the repetition has lead us to in the
current state of the git-branch(1) man page.
^ permalink raw reply
* Re: [PATCH v5 2/3] git-std-lib: introduce Git Standard Library
From: Linus Arver @ 2024-02-29 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, Calvin Wan, git, Jonathan Tan
In-Reply-To: <xmqq8r33gkxu.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> It does make me wonder if we should stop being lazy and do the
>> work that the linker has been doing for us "for free"
>> ourselves. IOW, stop linking against a monolithic libgit.a.
>> ... which might help us understand which things need what.
>
> [...] If a tool is
> available to help us and if there is no downside of using the tool,
> we should keep using it.
Of course, if there is no downside, we should use the tool as is.
> If you are proposing to move away from the
> current build practice because you have a concrete downside of the
> approach and avoid that, then it might be a good proposal, though.
Right. I was just wondering if the "explicit dependencies declared in
the Makefile" would provide some value WRT libification. Currently IDK
the answer to that.
> And "we do not learn otherwise" is not a downside of the approach;
> "we do not learn" comes from your not learning, the tools do not
> force you to be ignorant. We do not propose to use more __asm__ in
> our C sources only because compilers were doing that for us "for
> free" and because the compilers were somehow robbing us the
> opportunity to learn micro-optimization techniques, do we?
True.
> A small downside I can immediately think of is possible in a
> situation where we have been throwing an object file into libgit.a
> archive that is no longer used by any final executable. In such a
> scenario, if you change the source file that is compiled into such
> an unused object file, your next "make" will update libgit.a to
> replace the unused object file with its new version with your
> updates, and that would cause the final build product to be linked
> again with objects needed from libgit.a, but there shouldn't be any
> change because we are talking about an object that is *not* used by
> them but still is in libgit.a due to be listed on LIB_OBJS variable.
IIUC, this (theoretical) downside will result in Make thinking that it
needs to rebuild libgit.a when it actually doesn't need to (because the
updated change is for an unused object). So it could slow down the build
unnecessarily. Makes sense.
> But that is a purely theoretical downside. It may be the case that
> we haven't done our spring cleaning recently and we haven't noticed
> that a source file or two are now unused but are still listed on
> LIB_OBJS to be included in the libgit.a archive. But even if that
> were the case, it is implausible that you are touching such an
> unused source file in the first place.
As you noted, libgit.a is not a true library; it's just a big archive of
everything and we let the linker figure out what the executables need
out of it. But I was under the impression that with Git libification, we
would want to create a real library in the fullest sense of the word ---
such that our executables also need this library ("-lgit") to be built
in the exact same way that external programs would need this same
(single) Git library. For example, I believe this is how curl is built
(it first builds libcurl, and then links against it as an internal user
for generating the "curl" executable).
Going back to libgit.a, I was just wondering if the exercise of breaking
it up into smaller pieces would have been helpful in figuring out this
"-lgit" library (or what a smaller version of it would look like).
I sense that I may be missing large pieces of context around the
git-std-lib discussions though, so I apologize if my points above are
not new and moot. Thanks.
^ permalink raw reply
* Re: [PATCH] branch: adjust documentation
From: Rubén Justo @ 2024-02-29 20:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Dragan Simic, git
In-Reply-To: <xmqqcysff4l7.fsf@gitster.g>
On Thu, Feb 29, 2024 at 11:33:24AM -0800, Junio C Hamano wrote:
> Do not forget that the objective of the larger-picture-revamping of
> this page is to make the description of each option self-contained.
> Similarity between -m's description and -c's description does not
> count as being uselessly repetitive.
OK. I was not considering this.
Thanks.
^ permalink raw reply
* Re: [PATCH] docs: sort configuration variable groupings alphabetically
From: Junio C Hamano @ 2024-02-29 19:59 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Eric Sunshine, Bruno Haible
In-Reply-To: <20240229190229.20222-1-ericsunshine@charter.net>
Eric Sunshine <ericsunshine@charter.net> writes:
> NOTE: This change only sorts the top-level groupings (i.e. "core.*"
> comes after "completion.*"); it does not touch the ordering of variables
> within each group since variables within individual groups might
> intentionally be ordered in some other fashion (such as
> most-common-first or most-important-first).
I think this is a useful first step.
It is tempting to think, in this day and age, searching in a
document is so easy to start with a single keystroke (either ^F in a
browser, or '/' in a pager) that the ordering of entries does not
matter as much as it used to, but the reader may not know exactly
what variable they are looking for, and a predictable ordering helps
while they are browsing the list.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/1] add: use unsigned type for collection of bits
From: Junio C Hamano @ 2024-02-29 19:52 UTC (permalink / raw)
To: Eugenio Gigante; +Cc: christian.couder, git, sunshine
In-Reply-To: <20240229194444.8499-2-giganteeugenio2@gmail.com>
Eugenio Gigante <giganteeugenio2@gmail.com> writes:
> The 'refresh' function in 'builtin/add.c'
> declares 'flags' as signed, and passes it
> as an argument to the 'refresh_index' function,
> which though expects an unsigned value.
OK. This has become much easier to read by dropping the mention of
the header file where the callee happens to be declared.
Just FYI, in my private rewrite (that I'll discard), I made it like
so:
builtin/add.c:refresh() declares 'flags' as signed, and uses it
to call refresh_index() that expects an unsigned value.
> Since in this case 'flags' represents
> a bag of bits, whose MSB is not used
> in special ways, change the type
> of 'flags' to unsigned.
Good, too.
Will queue.
> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
> ---
> builtin/add.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index ada7719561..393c10cbcf 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -115,7 +115,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
> int i, ret = 0;
> char *skip_worktree_seen = NULL;
> struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
> - int flags = REFRESH_IGNORE_SKIP_WORKTREE |
> + unsigned int flags = REFRESH_IGNORE_SKIP_WORKTREE |
> (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
>
> seen = xcalloc(pathspec->nr, 1);
^ permalink raw reply
* [PATCH v2 1/1] add: use unsigned type for collection of bits
From: Eugenio Gigante @ 2024-02-29 19:44 UTC (permalink / raw)
To: gitster; +Cc: christian.couder, giganteeugenio2, git, sunshine
In-Reply-To: <20240229194444.8499-1-giganteeugenio2@gmail.com>
The 'refresh' function in 'builtin/add.c'
declares 'flags' as signed, and passes it
as an argument to the 'refresh_index' function,
which though expects an unsigned value.
Since in this case 'flags' represents
a bag of bits, whose MSB is not used
in special ways, change the type
of 'flags' to unsigned.
Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
---
builtin/add.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/add.c b/builtin/add.c
index ada7719561..393c10cbcf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -115,7 +115,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
int i, ret = 0;
char *skip_worktree_seen = NULL;
struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
- int flags = REFRESH_IGNORE_SKIP_WORKTREE |
+ unsigned int flags = REFRESH_IGNORE_SKIP_WORKTREE |
(verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
seen = xcalloc(pathspec->nr, 1);
--
2.43.0
^ permalink raw reply related
* [GSoC][PATCH v2 0/1] Use unsigned integral type for collection of bits.
From: Eugenio Gigante @ 2024-02-29 19:44 UTC (permalink / raw)
To: gitster; +Cc: christian.couder, giganteeugenio2, git, sunshine
In-Reply-To: <xmqqsf1ekf34.fsf@gitster.g>
Improve the commit log message
based on reviewers feedback.
Eugenio Gigante (1):
add: use unsigned type for collection of bits
builtin/add.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
--
2.43.0
^ permalink raw reply
* Re: [PATCH] branch: adjust documentation
From: Junio C Hamano @ 2024-02-29 19:33 UTC (permalink / raw)
To: Rubén Justo; +Cc: Dragan Simic, git
In-Reply-To: <cbaf17e7-37a6-4c2e-82ba-65fe41dd86b1@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> If we wisely choose the placeholder, perhaps we can write:
>
> -m [<one>] <two>::
> Renames <one> (the current branch is used when not given) to
> a new name <two>, together with its reflog and configuration
> settings ...
>
> And if <one> is _good enough_ then "current branch is used when ..."
> should seem somewhat redundant. So it could be possible to end up
> having something like:
>
> -m [<one>] <two>::
> Renames <one> to a new name <two>, together with its reflog
> and configuration settings ...
If you use <the-current-branch-or-a-named-branch> or something
awkward like that as <one>, surely you can. But I do not think we
want to go there. And neither <branch-name> or <old-branch> would
remove the need for "if omitted then the current branch is used", I
am afraid, even though there may be a way to rephrase it more
concisely, e.g. "Rename the current branch (or <one> when given)..."
> Are we going to say "the current branch is used when ..." in the
> description for the other options too? The description for "-c|-C",
> "--edit-description", "--unset-upstream", ... Perhaps we are, and it
> will sound repetitive.
Do not forget that the objective of the larger-picture-revamping of
this page is to make the description of each option self-contained.
Similarity between -m's description and -c's description does not
count as being uselessly repetitive.
>> Even though the choice of words Rubén made in the patch under
>> discussion may work well in the current document structure.
>
> My patch is mainly about CodingGuideLines:
>
> If a placeholder has multiple words, they are separated by dashes:
> <new-branch-name>
> --template=<template-directory>
Yes, and that will be something we want to address _after_ we pick
what word or phrase would replace <one> or <two> in the above at the
conceptual level. If we picked a single word, say "branch", we do
not even need to worry about dashes, and spell it just <branch>. If
we did not pick "old branch", then we'd use "<old-branch>", but doing
such a conversion based on the current text is a wasted work, if we
end up using say "original branch" as the phrase, for example.
So if your patch is mainly about that part of the guideline, it is
addressing the documentation update in a wrong order, creating
possibly a wasted work.
^ permalink raw reply
* Re: [PATCH 3/4] completion: reflog with implicit "show"
From: Junio C Hamano @ 2024-02-29 19:22 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <ff1c650b-5776-4881-ad0d-c39d311fa7e7@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> I am happy with the current iteration and I still think that mixing
> branch names with options is a source of confusion.
>
> However, this topic in the latest 'What's cooking' is marked with:
>
> Expecting a reroll.
> cf. <dd106d87-3363-426a-90a2-16e1f2d04661@gmail.com>
> source: <98daf977-dbad-4d3b-a293-6a769895088f@gmail.com>
>
> I am confused about what the expectation is.
Expectation is to show both possible commands and branch names
available so that users with enough typing can pick from either, as
I do see it even more confusing if only branch names (or only
command names) are visible sometimes (namely, if the prefix is
shared by both) but some other times command names (or branch names)
are completed just fine (namely, if the prefix is unique to only one
set of names between command names and branch names).
^ permalink raw reply
* Re: [PATCH] setup: clarify TODO comment about ignoring core.bare
From: Junio C Hamano @ 2024-02-29 19:15 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, johannes.schindelin, newren, christian.couder
In-Reply-To: <20240229134114.285393-2-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> /*
> - * TODO: heed core.bare from config file in templates if no
> - * command-line override given
> + * Note: The below line simply checks the presence of worktree (the
> + * simplification of which is given after the line) and core.bare from
> + * config file is not taken into account when deciding if the worktree
> + * should be created or not, even if no command line override given.
> + * That is intentional. Therefore, if in future we want to heed
> + * core.bare from config file, we should do it before we create any
> + * subsequent directories for worktree or repo because until this point
> + * they should already be created.
> */
> is_bare_repository_cfg = prev_bare_repository || !work_tree;
I do not recall the discussion; others may want to discuss if the
change above is desirable, before I come back to the topic later.
But I see this long comment totally unnecessary and distracting.
> - /* TODO (continued):
> + /* Note (continued):
> *
> - * Unfortunately, the line above is equivalent to
> + * The line above is equivalent to
> * is_bare_repository_cfg = !work_tree;
> - * which ignores the config entirely even if no `--[no-]bare`
> - * command line option was present.
> *
> * To see why, note that before this function, there was this call:
> * prev_bare_repository = is_bare_repository()
If it can be proven that the assignment can be simplified to lose
the "prev_bare_repository ||" part, then the above comment can be
used as part of the proposed log message for a commit that makes
such a change. There is no reason to leave such a long comment to
leave the more complex "A || B" expression when it can be simplified
to "B", no?
Thanks.
^ 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