* Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
From: Eric Sunshine @ 2023-10-03 21:10 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231003220951+0200.221551-stepnem@smrk.net>
On Tue, Oct 3, 2023 at 4:09 PM Štěpán Němec <stepnem@smrk.net> wrote:
> On Tue, 3 Oct 2023 14:30:38 -0400 Eric Sunshine wrote:
> > On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
> >> diff --git a/contrib/README b/contrib/README
> >> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the
> >> I expect that things that start their life in the contrib/ area
> >> -to graduate out of contrib/ once they mature, either by becoming
> >> +graduate out of contrib/ once they mature, either by becoming
> >
> > You probably want to add a comma after "area".
>
> That would read awkward to me. How about going the other way,
>
> I expect things that start their life in the contrib/ area
> to graduate out of contrib/ once they mature
>
> instead?
Sounds fine.
> >> @@ -579,11 +579,10 @@ This test harness library does the following things:
> >> -Here are some recommented styles when writing test case.
> >
> > Do you want to fix the spelling error while you're here or is that
> > done in a later patch?
> >
> > s/recommented/recommended/
>
> You really had me double-checking both the branch and the patch I sent
> here. :-D Unless I'm very much missing something, that line is _removed_
> by the patch (seemed redundant given the title immediately preceding
> it).
Ugh, so it is. Sorry for the noise.
> >> - - Keep test title the same line with test helper function itself.
> >> + - Keep test titles and helper function invocations on the same line.
> >
> > This would be clearer if it was switched around. Either:
> >
> > Keep the test_expect_* function call and test title on the same line.
> >
> > or, more verbosely:
> >
> > Place the test title on the same line as the test_expect_* call
> > which precedes it.
>
> I'll take your first suggestion.
I like the first suggestion better too.
^ permalink raw reply
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Junio C Hamano @ 2023-10-03 21:21 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231003082323.3002663-1-stepnem@smrk.net>
Štěpán Němec <stepnem@smrk.net> writes:
> Turns out having `pwd` (which TEST_DIRECTORY defaults to) print $PWD
> with a trailing slash isn't very difficult, in my case it went something
> like
>
> ; tmux new-window -c ~/src/git/t/
> [in the new window]
> ; sh ./t0000-basic.sh
> PANIC: Running in a /home/stepnem/src/git/t/ that doesn't end in '/t'?
> ; pwd
> /home/stepnem/src/git/t/
>
> (tmux(1) apparently sets PWD in the environment in addition to calling
> chdir(2), which seems enough to make at least some shells preserve the
> trailing slash in `pwd` output.)
>
> Strip the trailing slash, if present, to prevent bailing out with the
> PANIC message in such cases.
>
> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
> ---
> t/test-lib.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1656c9eed006..3b6f1a17e349 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -35,6 +35,7 @@ else
> # needing to exist.
> TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
> fi
> +TEST_DIRECTORY="${TEST_DIRECTORY%/}"
> if test -z "$TEST_OUTPUT_DIRECTORY"
> then
> # Similarly, override this to store the test-results subdir
>
> base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
While this would certainly squelch the particular test on TEST_DIRECTORY
I am not sure if a safer fix would be to fix your PWD when it has a
trailing slash. Your tests with this patch may be taking unintended
branch when they do something like
if test "$TRASH_DIRECTORY" = "$(pwd)"
then
...
fi
as TRASH_DIRECTORY, whose default is derived from TEST_DIRECTORY and
thanks to your fix it now does not have an extra slash in it, does
not have a trailing slash but your $(pwd) or $PWD would have the
trailing slash, simply because the patch only fixes TEST_DIRECTORY
without fixing PWD.
I wonder if this would be a safer alternative, or is it doing too
much more than what is necessary?
t/test-lib.sh | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git c/t/test-lib.sh w/t/test-lib.sh
index 1656c9eed0..6ec42eab0d 100644
--- c/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -22,19 +22,26 @@ then
# ensure that TEST_DIRECTORY is an absolute path so that it
# is valid even if the current working directory is changed
TEST_DIRECTORY=$(pwd)
-else
- # The TEST_DIRECTORY will always be the path to the "t"
- # directory in the git.git checkout. This is overridden by
- # e.g. t/lib-subtest.sh, but only because its $(pwd) is
- # different. Those tests still set "$TEST_DIRECTORY" to the
- # same path.
- #
- # See use of "$GIT_BUILD_DIR" and "$TEST_DIRECTORY" below for
- # hard assumptions about "$GIT_BUILD_DIR/t" existing and being
- # the "$TEST_DIRECTORY", and e.g. "$TEST_DIRECTORY/helper"
- # needing to exist.
- TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
fi
+
+# The TEST_DIRECTORY will always be the path to the "t"
+# directory in the git.git checkout. This is overridden by
+# e.g. t/lib-subtest.sh, but only because its $(pwd) is
+# different. Those tests still set "$TEST_DIRECTORY" to the
+# same path.
+#
+# See use of "$GIT_BUILD_DIR" and "$TEST_DIRECTORY" below for
+# hard assumptions about "$GIT_BUILD_DIR/t" existing and being
+# the "$TEST_DIRECTORY", and e.g. "$TEST_DIRECTORY/helper"
+# needing to exist.
+#
+# Also, apparently a shell spawned in some tricky way can be
+# talked into keeping a slash at the end of its $PWD.
+#
+# All of the above can be worked around by doing an extra chdir
+# and asking where we ended up to be.
+TEST_DIRECTORY=$(cd "$TEST_DIRECTORY/." && pwd) || exit 1
+
if test -z "$TEST_OUTPUT_DIRECTORY"
then
# Similarly, override this to store the test-results subdir
^ permalink raw reply related
* Re: [silly] loose, pack, and another thing?
From: Junio C Hamano @ 2023-10-03 21:26 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git
In-Reply-To: <20231003190955.GA1562@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> One thing that scares me about a regular "ln" between the worktree and
> odb is that you are very susceptible to corrupting the repository by
> modifying the worktree file with regular tools. If they do a complete
> rewrite and atomic rename (or link) to put the new file in place, that
> is OK. But opening the file for appending, or general writing, is bad.
Very true.
> You can get some safety with the immutable attribute (which applies to
> the inode itself, and thus any path that hardlinks to it). But setting
> that usually requires being root. And it creates other irritations for
> normal use (you have to unset it before even removing the hardlink).
As a regular user, "chmod a-w" has the same characteristics (works
at the inode level) but without "cannot remove it" downside. It
used to be sufficient in RCS and CVS days, though, as a signal that
you are only to look at it without touching it, to "chmod a-w" a
path that is checked out but not for modifying. Some editors even
offer to do chmod u+w for you when saving, so if we want absolute
safety, it may not be enough.
> It would be nice if there was some portable copy-on-write abstraction we
> could rely on, but I don't think there is one.
;-)
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Taylor Blau @ 2023-10-03 21:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Patrick Steinhardt
In-Reply-To: <xmqqa5szzcdz.fsf@gitster.g>
On Tue, Oct 03, 2023 at 01:20:24PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:
> >> I've had this sitting in my patch queue for a while now. It's a
> >> non-critical performance fix that avoids the repack/MIDX machinery from
> >> ever choosing a cruft pack as preferred when writing a MIDX bitmap
> >> without a given --preferred-pack.
> >>
> >> There is no correctness issue here, but choosing a pack with few/no
> >> reachable objects means that our pack reuse mechanism will rarely kick
> >> in, resulting in performance degradation.
> >>
> >> builtin/repack.c | 47 ++++++++++++++++++++++++++++++++++++++++-
> >> t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++
> >> 2 files changed, 85 insertions(+), 1 deletion(-)
> >
> > Oops, I should have mentioned that this is meant to be applied on top of
> > 'tb/multi-cruft-pack' to reduce the conflict resolution burden. Sorry
> > about that.
>
> Sorry, but I do not follow. tb/multi-cruft-pack was merged to
> 'master' at c0b5d46d (Documentation/gitformat-pack.txt: drop mixed
> version section, 2023-08-28) but back then t7704 did not exist. Do
> you mean the other topic in-flight from you about max-cruft-size?
My mistake -- this should be applied on top of the patches at this
series:
https://lore.kernel.org/git/cover.1696293862.git.me@ttaylorr.com/
which is the other topic that you're referring to. I ran "git
for-each-ref 'refs/remotes/origin/tb/*' | grep cruft" and mistakenly
grabbed 'tb/multi-cruft-pack' thinking that that was that series and not
the one already merged to 'master'.
But it looks like the series linked above hasn't yet grown a topic
branch in gitster/git yet, hence my mistake. Sorry about that!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Taylor Blau @ 2023-10-03 21:52 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <20231003201617.GE1562@coredump.intra.peff.net>
On Tue, Oct 03, 2023 at 04:16:17PM -0400, Jeff King wrote:
> On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:
>
> > Note that this behavior is usually just a performance regression. But
> > it's possible it could be a correctness issue.
> >
> > Suppose an object was duplicated among the cruft and non-cruft pack. The
> > MIDX will pick the one from the pack with the lowest mtime, which will
> > always be the cruft one. But if the non-cruft pack happens to sort
> > earlier in lexical order, we'll treat that one as preferred, but not all
> > duplicates will be resolved in favor of that pack.
> >
> > So if we happened to have an object which appears in both packs
> > (e.g., due to a cruft object being freshened, causing it to appear
> > loose, and then repacking it via the `--geometric` repack) it's possible
> > the duplicate would be picked from the non-preferred pack.
>
> I'm not sure I understand how that is a correctness issue. The contents
> of the object are the same in either pack. Or do you mean that the
> pack-reuse code in pack-objects.c may get confused and try to use the
> wrong pack/offset when sending the object out? I would think it would
> always be coming from the preferred pack for that code (so the outcome
> is just that we fail to do the pack-reuse optimization very well, but we
> don't generate a wrong answer).
Admittedly I'm not 100% sure of my interpretation here either, since I
wrote most of this patch's log message nearly two years ago ;-).
I think the issue was as follows:
- you have an object duplicated among some cruft and non-cruft pack
- the cruft pack happens to have an older mtime than the non-cruft one
- the repack reused no non-cruft packs, which (before this patch)
would let the MIDX avoid picking a preferred pack
- if the non-cruft pack happens to sort ahead of the cruft one in
lexical order, it'll appear in the first few bits of the bitmap's
ordering, causing us to treat it as if it were the preferred pack
- ...but the MIDX will resolve duplicate objects in favor of the
oldest pack when neither are preferred, causing us to pick a
duplicate from the non-"preferred" pack.
(The last point is what causes the pack-bitmap reading code to get
confused, since it expects and makes assumption based on resolving ties
in favor of the preferred pack.)
That feels right to me, but admittedly my memory is pretty hazy on those
bitmap bugs. However, I don't think that this is an issue anymore, since
this patch was written before we had 177c0d6e63 (midx: infer preferred
pack when not given one, 2021-08-31) in GitHub's private fork, and the
patch message here incorrectly refers to statements about GitHub's fork,
not git.git.
But since we have 177c0d6e63 in git.git, this paragraph can and should
be dropped as finding ourselves in that situation is impossible, since
we would infer a preferred pack when not given one, and resolve
duplicates in favor of it.
That makes this purely a performance issue.
> Other than that, the explanation and patch make perfect sense to me, and
> the patch looks good.
Thanks!
> -Peff
Thanks,
Taylor
^ permalink raw reply
* [PATCH v2] builtin/repack.c: avoid making cruft packs preferred
From: Taylor Blau @ 2023-10-03 21:54 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com>
When doing a `--geometric` repack, we make sure that the preferred pack
(if writing a MIDX) is the largest pack that we *didn't* repack. That
has the effect of keeping the preferred pack in sync with the pack
containing a majority of the repository's reachable objects.
But if the repository happens to double in size, we'll repack
everything. Here we don't specify any `--preferred-pack`, and instead
let the MIDX code choose.
In the past, that worked fine, since there would only be one pack to
choose from: the one we just wrote. But it's no longer necessarily the
case that there is one pack to choose from. It's possible that the
repository also has a cruft pack, too.
If the cruft pack happens to come earlier in lexical order (and has an
earlier mtime than any non-cruft pack), we'll pick that pack as
preferred. This makes it impossible to reuse chunks of the reachable
pack verbatim from pack-objects, so is sub-optimal.
Luckily, this is a somewhat rare circumstance to be in, since we would
have to repack the entire repository during a `--geometric` repack, and
the cruft pack would have to sort ahead of the pack we just created.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
A small re-roll of the original patch, removing a few paragraphs from
the proposed log message that are irrelevant and impossible to produce
in today's git.git.
For more details, see <ZRyNHRf+tQwV+T6P@nand.local>.
Range-diff against v1:
1: 19d9aae08e ! 1: 0353939351 builtin/repack.c: avoid making cruft packs preferred
@@ Commit message
have to repack the entire repository during a `--geometric` repack, and
the cruft pack would have to sort ahead of the pack we just created.
- Note that this behavior is usually just a performance regression. But
- it's possible it could be a correctness issue.
-
- Suppose an object was duplicated among the cruft and non-cruft pack. The
- MIDX will pick the one from the pack with the lowest mtime, which will
- always be the cruft one. But if the non-cruft pack happens to sort
- earlier in lexical order, we'll treat that one as preferred, but not all
- duplicates will be resolved in favor of that pack.
-
- So if we happened to have an object which appears in both packs
- (e.g., due to a cruft object being freshened, causing it to appear
- loose, and then repacking it via the `--geometric` repack) it's possible
- the duplicate would be picked from the non-preferred pack.
-
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## builtin/repack.c ##
builtin/repack.c | 47 ++++++++++++++++++++++++++++++++++++++++-
t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 04770b15fe..a1a893d952 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -355,6 +355,18 @@ static struct generated_pack_data *populate_pack_exts(const char *name)
return data;
}
+static int has_pack_ext(const struct generated_pack_data *data,
+ const char *ext)
+{
+ int i;
+ for (i = 0; i < ARRAY_SIZE(exts); i++) {
+ if (strcmp(exts[i].name, ext))
+ continue;
+ return !!data->tempfiles[i];
+ }
+ BUG("unknown pack extension: '%s'", ext);
+}
+
static void repack_promisor_objects(const struct pack_objects_args *args,
struct string_list *names)
{
@@ -772,6 +784,7 @@ static void midx_included_packs(struct string_list *include,
static int write_midx_included_packs(struct string_list *include,
struct pack_geometry *geometry,
+ struct string_list *names,
const char *refs_snapshot,
int show_progress, int write_bitmaps)
{
@@ -801,6 +814,38 @@ static int write_midx_included_packs(struct string_list *include,
if (preferred)
strvec_pushf(&cmd.args, "--preferred-pack=%s",
pack_basename(preferred));
+ else if (names->nr) {
+ /* The largest pack was repacked, meaning that either
+ * one or two packs exist depending on whether the
+ * repository has a cruft pack or not.
+ *
+ * Select the non-cruft one as preferred to encourage
+ * pack-reuse among packs containing reachable objects
+ * over unreachable ones.
+ *
+ * (Note we could write multiple packs here if
+ * `--max-pack-size` was given, but any one of them
+ * will suffice, so pick the first one.)
+ */
+ for_each_string_list_item(item, names) {
+ struct generated_pack_data *data = item->util;
+ if (has_pack_ext(data, ".mtimes"))
+ continue;
+
+ strvec_pushf(&cmd.args, "--preferred-pack=pack-%s.pack",
+ item->string);
+ break;
+ }
+ } else {
+ /*
+ * No packs were kept, and no packs were written. The
+ * only thing remaining are .keep packs (unless
+ * --pack-kept-objects was given).
+ *
+ * Set the `--preferred-pack` arbitrarily here.
+ */
+ ;
+ }
if (refs_snapshot)
strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
@@ -1360,7 +1405,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
struct string_list include = STRING_LIST_INIT_NODUP;
midx_included_packs(&include, &existing, &names, &geometry);
- ret = write_midx_included_packs(&include, &geometry,
+ ret = write_midx_included_packs(&include, &geometry, &names,
refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
show_progress, write_bitmaps > 0);
diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh
index dc86ca8269..be3735dff0 100755
--- a/t/t7704-repack-cruft.sh
+++ b/t/t7704-repack-cruft.sh
@@ -372,4 +372,43 @@ test_expect_success '--max-cruft-size ignores non-local packs' '
)
'
+test_expect_success 'reachable packs are preferred over cruft ones' '
+ repo="cruft-preferred-packs" &&
+ git init "$repo" &&
+ (
+ cd "$repo" &&
+
+ # This test needs to exercise careful control over when a MIDX
+ # is and is not written. Unset the corresponding TEST variable
+ # accordingly.
+ sane_unset GIT_TEST_MULTI_PACK_INDEX &&
+
+ test_commit base &&
+ test_commit --no-tag cruft &&
+
+ non_cruft="$(echo base | git pack-objects --revs $packdir/pack)" &&
+ # Write a cruft pack which both (a) sorts ahead of the non-cruft
+ # pack in lexical order, and (b) has an older mtime to appease
+ # the MIDX preferred pack selection routine.
+ cruft="$(echo pack-$non_cruft.pack | git pack-objects --cruft $packdir/pack-A)" &&
+ test-tool chmtime -1000 $packdir/pack-A-$cruft.pack &&
+
+ test_commit other &&
+ git repack -d &&
+
+ git repack --geometric 2 -d --write-midx --write-bitmap-index &&
+
+ # After repacking, there are two packs left: one reachable one
+ # (which is the result of combining both of the existing two
+ # non-cruft packs), and one cruft pack.
+ find .git/objects/pack -type f -name "*.pack" >packs &&
+ test_line_count = 2 packs &&
+
+ # Make sure that the pack we just wrote is marked as preferred,
+ # not the cruft one.
+ pack="$(test-tool read-midx --preferred-pack $objdir)" &&
+ test_path_is_missing "$packdir/$(basename "$pack" ".idx").mtimes"
+ )
+'
+
test_done
--
2.42.0.341.g9d86323b73.dirty
^ permalink raw reply related
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Junio C Hamano @ 2023-10-03 21:57 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <xmqqwmw3wgeo.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I wonder if this would be a safer alternative, or is it doing too
> much more than what is necessary?
An alternative with much smaller blast radius would be to do this.
Hopefully, by going "$(pwd)/." before asking the value returned by
the `pwd` command, we can make sure that the trailing slash is
removed (or at least $(pwd) and $TEST_DIRECTORY should be identical
after this is done).
t/test-lib.sh | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git c/t/test-lib.sh w/t/test-lib.sh
index 1656c9eed0..d159358b21 100644
--- c/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -19,9 +19,13 @@
# t/ subdirectory and are run in 'trash directory' subdirectory.
if test -z "$TEST_DIRECTORY"
then
- # ensure that TEST_DIRECTORY is an absolute path so that it
+ # Ensure that TEST_DIRECTORY is an absolute path so that it
# is valid even if the current working directory is changed
- TEST_DIRECTORY=$(pwd)
+ # Some environments can talk the shell into keeping trailing
+ # slash in $PWD---go there and ask where we are to work it
+ # around, as we expect TEST_DIRECTORY and PWD are both
+ # canonical and can textually be compared for equality
+ TEST_DIRECTORY=$(cd "$(pwd)/." && pwd)
else
# The TEST_DIRECTORY will always be the path to the "t"
# directory in the git.git checkout. This is overridden by
^ permalink raw reply related
* Re: [PATCH 2/6] submodule--helper: return error from set-url when modifying failed
From: Junio C Hamano @ 2023-10-03 23:10 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig); +Cc: git
In-Reply-To: <20231003185047.2697995-2-heftig@archlinux.org>
"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:
> set-branch will return an error when setting the config fails so I don't
> see why set-url shouldn't. Also skip the sync in this case.
Two other failures in this helper function gives end-user readable
message (parameter errors are greeted with usage, giving wrong path
is greeted with a die()), but this new error condition will silently
die unless config_set_in_gitmodules_file_gently() complains.
I wonder if we should give an error message in this case, too.
In any case, not calling sync after failed set_in_gitmodules is a
sensible design decision.
^ permalink raw reply
* Re: [Outreachy] Move existing tests to a unit testing framework
From: Luma @ 2023-10-03 23:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqedibzgi1.fsf@gitster.g>
oh yes, "Move existing tests to a unit testing framework" was the
only listed project for this current Outreachy cohort. So, I used it
to express my intent.
I appreciate the clarification on authorship identity for patches. I
will update subsequent patches with a legal full name to conform to
the community rules.
Regards.
On Tue, Oct 3, 2023 at 7:51 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Luma <ach.lumap@gmail.com> writes:
>
> > Hi;
> > My name is Luma, and I wanted to take a moment to introduce myself
> > and share some
> > insights on an essential aspect of avoiding pipes in git related
> > commands in test scripts.
> >
> > I am an outreachy applicant for the December 2023 cohort and look
> > forward to learning from you.
>
> I notice that the title of the message and the immediate topic you
> discuss in the body of the message do not match. I presume that the
> topic on the title is what you prefer to work on if the unit testing
> framework is ready by the time Outreachy program starts, and the
> mention about "do not clobber exit code of Git with pipes in the
> tests" is your "dip the tip of a toe in water" microproject?
>
> Welcome to the Git development community.
>
> Do you have a single word name? If so please disregard the below,
> but in case "Luma" is just a nickname (e.g. like I am introducing
> myself to my Git friends "Hi, I am Gitster!") you use online, please
> read on.
>
> For signing off your patches, we'd prefer to see your real name
> used, as Signed-off-by: is meant to have legal significance. And
> because we also expect the authorship identity to match the
> name/e-mail of the sign-off, it would mean your patch submissions
> are expected to look like:
>
> From: Luma <ach.lumap@gmail.com>
> Subject: ... title of the patch goes here ...
>
> ... body of the proposed commit log message goes here...
>
> Signed-off-by: Luma <ach.lumap@gmail.com>
>
> but "Luma" replaced with your full real name.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH 3/6] t7419: Actually test the branch switching
From: Junio C Hamano @ 2023-10-04 0:20 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig); +Cc: git
In-Reply-To: <20231003185047.2697995-3-heftig@archlinux.org>
"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:
> Subject: Re: [PATCH 3/6] t7419: Actually test the branch switching
"Actually" -> "actually".
> The submodule repo the test set up had the 'topic' branch checked out,
> meaning the repo's default branch (HEAD) is the 'topic' branch.
>
> The following tests then pretended to switch between the default branch
> and the 'topic' branch. This was papered over by continually adding
> commits to the 'topic' branch and checking if the submodule gets updated
> to this new commit.
>
> Return the submodule repo to the 'main' branch after setup so we can
> actually test the switching behavior.
Nicely spotted.
> diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
> index 232065504c..5ac16d0eb7 100755
> --- a/t/t7419-submodule-set-branch.sh
> +++ b/t/t7419-submodule-set-branch.sh
> @@ -11,23 +11,28 @@ as expected.
>
> TEST_PASSES_SANITIZE_LEAK=true
> TEST_NO_CREATE_REPO=1
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
And it is good that this variable is used to prepare the test for
both kinds of CI runs that use 'main' or 'master' as the default
branch.
^ permalink raw reply
* Re: [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
From: Junio C Hamano @ 2023-10-04 1:10 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig); +Cc: git
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>
"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:
> The commands need a path to a submodule but treated it as the name when
> modifying the .gitmodules file, leading to confusion when a submodule's
> name does not match its path.
Thanks for noticing and fixing this common mix-up.
> Because calling submodule_from_path initializes the submodule cache, we
> need to manually trigger a reread before syncing, as the cache is
> missing the config change we just made.
> Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f6871efd95..f376466a5e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2902,19 +2902,26 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
> N_("git submodule set-url [--quiet] <path> <newurl>"),
> NULL
> };
> + const struct submodule *sub;
>
> argc = parse_options(argc, argv, prefix, options, usage, 0);
>
> if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
> usage_with_options(usage, options);
>
> - config_name = xstrfmt("submodule.%s.url", path);
> + sub = submodule_from_path(the_repository, null_oid(), path);
>
> + if (!sub)
> + die(_("no submodule mapping found in .gitmodules for path '%s'"),
> + path);
> +
> + config_name = xstrfmt("submodule.%s.url", sub->name);
Looks correct.
> config_set_in_gitmodules_file_gently(config_name, newurl);
> - sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
> +
> + repo_read_gitmodules (the_repository, 0);
Style. No extra space between function name and "(".
But more importantly, is this sufficient? repo_read_gitmodules()
does not seem to clear the cache and build its contents from scratch
(as submodule_cache_check_init() bypasses itself upon second call).
The code is correct because submodule-config.c::config_from() does
set .overwrite to true, so submodule.name.url would be overwritten
to the new value, I think, but somebody else who is more familiar
with the recent submodule code may want to sanity check my analysis.
> + sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
Is the use of "sub" still safe here?
I think it is safe as repo_read_gitmodules() did not rebuild the
in-core cache from scratch but did selective overwrite, so the
in-core instance "sub" is still valid, but again somebody else who
is more familiar with the recent submodule code may want to sanity
check.
> @@ -2942,19 +2949,26 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
> N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
> NULL
> };
> + const struct submodule *sub;
>
> argc = parse_options(argc, argv, prefix, options, usage, 0);
>
> if (!opt_branch && !opt_default)
> die(_("--branch or --default required"));
>
> if (opt_branch && opt_default)
> die(_("options '%s' and '%s' cannot be used together"), "--branch", "--default");
>
> if (argc != 1 || !(path = argv[0]))
> usage_with_options(usage, options);
>
> - config_name = xstrfmt("submodule.%s.branch", path);
> + sub = submodule_from_path(the_repository, null_oid(), path);
> +
> + if (!sub)
> + die(_("no submodule mapping found in .gitmodules for path '%s'"),
> + path);
> +
> + config_name = xstrfmt("submodule.%s.branch", sub->name);
> ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
This side happens not to require re-reading of gitmodules file,
because, unlike the URL helper, we do not care what we have in the
in-core cache is stale. It is correct but feels a bit brittle.
^ permalink raw reply
* Re: Possible to update-ref remote repository?
From: Junio C Hamano @ 2023-10-04 1:13 UTC (permalink / raw)
To: Jeff King; +Cc: Jesse Hopkins, git
In-Reply-To: <20231003200018.GB1562@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> All that said, I do think it might be reasonable for git-push to support
> this directly.
Yup. It certainly is simpler if you can leverage existing helpers.
It will become even simpler in a reasonably modularlized world that
hopefully may materialize before we all retire ;-). I am hoping
that some of the folks who are interested in and talking about
libification can be fooled into doing the necessary work to
introduce proper abstraction, in addition to whatever they are
doing.
Wouldn't it be great if you can have an in-core repository object,
that knows what its object store is, has an index_state object that
is tied to that object store, has a reference database whose values
point into the object store, and if you can choose and mix these
repository components' implementations? If done right, parts of the
above set of components can be replaced with mock implementations
that are in-core only.
To run "git push --repoint-only there 01beef23:master", you should
be able to start your process totally outside an repository, yet
create an in-core-only repository instance with an in-core-only
object store instance, and because you took the object name to push
on the command line, your in-core object store can "lie" to a call
"create an in-core object for this SHA-1" by returning a fake
in-core commit object, and your in-core-only ref database has that
commit pointed at by some ref. Then because higher level "client"
code to walk revisions, enumerate refs, etc., would all implement
what they need to do by calling vtable of these in-core objects, you
can do the "repoint-only" push without being in any repository, as
such an implementation would not touch any filesystem (you can then
plug in different implementation of object store etc., and even make
them perform reasonably well if you manage to do the abstraction
right).
But that would probably be at least 6 months away, even if we had a
handful of competent developers totally dedicated to the effort
without any distraction, which I do not know how likely it is to
happen.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Eric W. Biederman @ 2023-10-04 1:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric W. Biederman
In-Reply-To: <xmqqedic35u4.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> * eb/hash-transition (2023-10-02) 30 commits
> - t1016-compatObjectFormat: add tests to verify the conversion between objects
> - t1006: test oid compatibility with cat-file
> - t1006: rename sha1 to oid
> - test-lib: compute the compatibility hash so tests may use it
> - builtin/ls-tree: let the oid determine the output algorithm
> - object-file: handle compat objects in check_object_signature
> - tree-walk: init_tree_desc take an oid to get the hash algorithm
> - builtin/cat-file: let the oid determine the output algorithm
> - rev-parse: add an --output-object-format parameter
> - repository: implement extensions.compatObjectFormat
> - object-file: update object_info_extended to reencode objects
> - object-file-convert: convert commits that embed signed tags
> - object-file-convert: convert commit objects when writing
> - object-file-convert: don't leak when converting tag objects
> - object-file-convert: convert tag objects when writing
> - object-file-convert: add a function to convert trees between algorithms
> - object: factor out parse_mode out of fast-import and tree-walk into in object.h
> - cache: add a function to read an OID of a specific algorithm
> - tag: sign both hashes
> - commit: export add_header_signature to support handling signatures on tags
> - commit: convert mergetag before computing the signature of a commit
> - commit: write commits for both hashes
> - object-file: add a compat_oid_in parameter to write_object_file_flags
> - object-file: update the loose object map when writing loose objects
> - loose: compatibilty short name support
> - loose: add a mapping between SHA-1 and SHA-256 for loose objects
> - repository: add a compatibility hash algorithm
> - object-names: support input of oids in any supported hash
> - oid-array: teach oid-array to handle multiple kinds of oids
> - object-file-convert: stubs for converting from one object format to another
>
> Teach a repository to work with both SHA-1 and SHA-256 hash algorithms.
>
> Breaks a few CI jobs when merged to 'seen'.
> cf. <xmqqbkdmjbkp.fsf@gitster.g>
I see that you have picked up the v2 version. Thank you.
I pushed v2 out precisely because it contains fixes that should have
fixed all of the CI breakages.
I am not really familiar with github but looking at the recent CI runs
it appears since v2 landed the seen branch has been building cleanly.
I haven't misread something have I?
I just don't want people to avoid reviewing it because it is that huge
patchset that causes problems in seen.
Eric
^ permalink raw reply
* Is SANITIZE=leak make test unreliable for anyone else?
From: Eric W. Biederman @ 2023-10-04 1:33 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
Jeff,
Please accept my apologies for slightly hijacking your posting, but I
see you have been fixing some leaks, and so presumably you are familiar
with building git with "SANITIZE=leak".
I have fixed some leaks in my SHA1+SHA256 patchset recently and while
tracking them down I found that simply enabling SANITIZE=leak caused
"make test" on git v2.42 without patches to give different failures
from test run to test run.
Well actually I wound up with the following command line:
GIT_TEST_PASSING_SANITIZE_LEAK=true GIT_TEST_SANITIZE_LEAK_LOG=true SANITIZE=leak DEVELOPER=1 make test
I had removed "-j32" to make things more reproducible.
I observed this unreliability with SANITIZE=leak when building git on
an fully updated version of debian 12.
My big question is:
Do other people see random test failures when SANITIZE=leak is enabled?
Is it just me?
Thanks,
Eric
^ permalink raw reply
* Re: [PATCH v2] git-status.txt: fix minor asciidoc format issue
From: Javier Mora @ 2023-10-04 2:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: cousteau via GitGitGadget, git
In-Reply-To: <xmqq4jj7zc5b.fsf@gitster.g>
Fair enough; I'll update the commit message. I just thought it still
worked more or less so I left it as is.
El mar, 3 oct 2023 a las 21:25, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> "cousteau via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Javier Mora <cousteaulecommandant@gmail.com>
> >
> > The paragraph below the list of short option combinations
> > isn't correctly formatted, making the result hard to read.
> >
> > Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
> > ---
>
> The above probably no longer describes the situation the patch
> intends to correct, I suspect. It used to be near-impossible hard
> to read, but at least with them indented they are legible.
>
> The additional states for submodules are typeset differently
> from how the states for paths for normal blobs are listed as
> enumeration. Format them in the same way for consistency.
>
> or something like that, perhaps.
>
> > Documentation/git-status.txt | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> > index b27d127b5e2..48f46eb2047 100644
> > --- a/Documentation/git-status.txt
> > +++ b/Documentation/git-status.txt
> > @@ -246,10 +246,9 @@ U U unmerged, both modified
> >
> > Submodules have more state and instead report
> >
> > - M the submodule has a different HEAD than
> > - recorded in the index
> > - m the submodule has modified content
> > - ? the submodule has untracked files
> > +* 'M' = the submodule has a different HEAD than recorded in the index
> > +* 'm' = the submodule has modified content
> > +* '?' = the submodule has untracked files
> >
> > since modified content or untracked files in a submodule cannot be added
> > via `git add` in the superproject to prepare a commit.
>
> Thanks for making this part of the documentation better.
>
^ permalink raw reply
* [PATCH v3] git-status.txt: fix minor asciidoc format issue
From: cousteau via GitGitGadget @ 2023-10-04 2:22 UTC (permalink / raw)
To: git; +Cc: Javier Mora, cousteau, Javier Mora
In-Reply-To: <pull.1591.v2.git.1696350802820.gitgitgadget@gmail.com>
From: Javier Mora <cousteaulecommandant@gmail.com>
The list of additional XY values for submodules in short format
isn't formatted consistently with the rest of the document.
Format as list for consistency.
Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
---
git-status.txt: minor asciidoc format correction
The paragraph below the list of short option combinations was hard to
read; turns out it wasn't correctly formatted in asciidoc.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1591%2Fcousteaulecommandant%2Fman-git-status-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1591/cousteaulecommandant/man-git-status-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1591
Range-diff vs v2:
1: 811885a275f ! 1: 819499eb4c8 git-status.txt: fix minor asciidoc format issue
@@ Metadata
## Commit message ##
git-status.txt: fix minor asciidoc format issue
- The paragraph below the list of short option combinations
- isn't correctly formatted, making the result hard to read.
+ The list of additional XY values for submodules in short format
+ isn't formatted consistently with the rest of the document.
+ Format as list for consistency.
Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
Documentation/git-status.txt | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index b27d127b5e2..48f46eb2047 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -246,10 +246,9 @@ U U unmerged, both modified
Submodules have more state and instead report
- M the submodule has a different HEAD than
- recorded in the index
- m the submodule has modified content
- ? the submodule has untracked files
+* 'M' = the submodule has a different HEAD than recorded in the index
+* 'm' = the submodule has modified content
+* '?' = the submodule has untracked files
since modified content or untracked files in a submodule cannot be added
via `git add` in the superproject to prepare a commit.
base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
--
gitgitgadget
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Junio C Hamano @ 2023-10-04 8:20 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <874jj7lh7x.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> I believe I've addressed this in details in my reply here:
>>> <87o7hok8dx.fsf@osv.gnss.ru>, and got no further objections from you
>>> since then, so I figure I'd ask to finally let the patch in.
>>
>> You need to know that no response does not mean no objection. You
>> repeated why the less useful combination is what you want, but that
>> does not mean the combination deserves to squat on short-and-sweet
>> 'd' and prevent others from coming up with a better use for it.
>
> Yep, but I've asked what's better use for -d than "get me diff"? Do you
> really have an idea?
The primary point is to leave it open for future developers.
If I have to pick a candidate for "get me diff" that is the most
useful among those currently are available, it is "give patches to
all single-parent commit, and show tricky conflict resolution part
only for merge commits". Before "--remerge-diff" was invented, my
answer would have been "give patches to all single-parent commit,
and show combined diff in the compact form for merge commits", aka
"git log --cc". Even though we did not know if a better output
presentation for merge commits would be coming, we did not let it
squat on any short-and-sweet single letter synonym.
^ permalink raw reply
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Štěpán Němec @ 2023-10-04 9:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzs3wer0.fsf@gitster.g>
On Tue, 03 Oct 2023 14:57:39 -0700
Junio C. Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I wonder if this would be a safer alternative, or is it doing too
>> much more than what is necessary?
>
> An alternative with much smaller blast radius would be to do this.
>
> Hopefully, by going "$(pwd)/." before asking the value returned by
> the `pwd` command, we can make sure that the trailing slash is
> removed (or at least $(pwd) and $TEST_DIRECTORY should be identical
> after this is done).
>
>
>
> t/test-lib.sh | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git c/t/test-lib.sh w/t/test-lib.sh
> index 1656c9eed0..d159358b21 100644
> --- c/t/test-lib.sh
> +++ w/t/test-lib.sh
> @@ -19,9 +19,13 @@
> # t/ subdirectory and are run in 'trash directory' subdirectory.
> if test -z "$TEST_DIRECTORY"
> then
> - # ensure that TEST_DIRECTORY is an absolute path so that it
> + # Ensure that TEST_DIRECTORY is an absolute path so that it
> # is valid even if the current working directory is changed
> - TEST_DIRECTORY=$(pwd)
> + # Some environments can talk the shell into keeping trailing
> + # slash in $PWD---go there and ask where we are to work it
> + # around, as we expect TEST_DIRECTORY and PWD are both
> + # canonical and can textually be compared for equality
> + TEST_DIRECTORY=$(cd "$(pwd)/." && pwd)
> else
> # The TEST_DIRECTORY will always be the path to the "t"
> # directory in the git.git checkout. This is overridden by
Yes, actually, AFAICT just $(cd . && pwd) fixes things (and saves a few
syscalls), and I agree this is a much better approach than my naive fix.
And functionally it should be equivalent to your other solution that did
this unconditionally, because the else branch (TEST_DIRECTORY set) was
already cd-ing anyway.
Thanks,
Štěpán
^ permalink raw reply
* Re: [Outreachy] Introduction and Interest in Contributing to the Git Community
From: Christian Couder @ 2023-10-04 10:20 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git
In-Reply-To: <CAJHH8bHBA4emP2DkDEzcXncT4K5zEN-pCS+7jjer4R1_kkCkFA@mail.gmail.com>
Hi Isoken,
On Tue, Oct 3, 2023 at 12:21 PM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
>
> Dear Git Community,
>
> I hope this email finds you well. My name is Isoken Ibizugbe, and I am
> writing to express my strong interest in joining the Git community as
> a contributor to the FOSS project. I recently came across the project
> description regarding "Moving existing tests to a unit testing
> framework" and was particularly intrigued by the opportunity to be
> part of this exciting endeavor.
Great, welcome to the Git community!
> As an aspiring software engineer, I have always admired the incredible
> work done by the Git community in developing and maintaining this
> widely-used version control system. The project's commitment to
> fostering collaboration and innovation aligns perfectly with my values
> and aspirations as a developer.
>
> I understand that Christian Couder is the mentor for this project, and
> I would be honored to have the opportunity to work under his guidance
> and expertise. I would greatly appreciate any advice or direction he
> can provide to help me get started on this journey.
>
> I am eager to learn, collaborate with the community, and contribute
> meaningfully to this project. Please let me know how I can formally
> start my journey as a Git contributor and if there are any specific
> guidelines or resources that you recommend for newcomers, as it was a
> bit confusing process for me to join this mailing list.
Sure, please take a look at the documentation we have on
https://git.github.io/, especially the pages and sections I mention
below.
First, we require that applicants make a small code contribution (we
call that a micro-project) to the Git project. This is explained here:
https://git.github.io/General-Microproject-Information/
If I have time, I will perhaps prepare a small list of possible
micro-project. For example we prepared this one for the Outreachy
Winter 2021-2022 round:
https://git.github.io/Outreachy-23-Microprojects/
but no promise. The "General-Microproject-Information" page has
information about how to find micro-project ideas anyway.
Now we have recently added a "Thoroughly check your eligibility in the
program" sub-section to that page. Please read it, check your
eligibility and confirm that you meet all the requirements soon.
Then there are links to tutorials and a number of other useful link
for Git developers on this page:
https://git.github.io/Hacking-Git/
Also the following page is useful as it contains more general
information about how to apply:
https://git.github.io/General-Application-Information/
> Once again, thank you for your time and for providing an opportunity
> for individuals like me to contribute to this remarkable project. I am
> enthusiastic about the potential of this project and the journey
> ahead.
Thanks for your enthusiasm,
Christian.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Sergey Organov @ 2023-10-04 11:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqo7hessro.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sergey Organov <sorganov@gmail.com> writes:
>>>
>>>> I believe I've addressed this in details in my reply here:
>>>> <87o7hok8dx.fsf@osv.gnss.ru>, and got no further objections from you
>>>> since then, so I figure I'd ask to finally let the patch in.
>>>
>>> You need to know that no response does not mean no objection. You
>>> repeated why the less useful combination is what you want, but that
>>> does not mean the combination deserves to squat on short-and-sweet
>>> 'd' and prevent others from coming up with a better use for it.
>>
>> Yep, but I've asked what's better use for -d than "get me diff"? Do you
>> really have an idea?
>
> The primary point is to leave it open for future developers.
Well, I'm not actually convinced it's justified in this particular case,
but I'll re-roll with another option name, even though I suspect this
will leave short-and-sweet '-d' unused for yet another 18 years.
Just for better understanding: does it mean that *any* addition of
one-letter option is prohibited from any existing Git command? Cause it
definitely sounds this way.
And while we are at it, is it allowed to have "long" one-letter options,
e.g., "--d"?
>
> If I have to pick a candidate for "get me diff" that is the most
> useful among those currently are available, it is "give patches to
> all single-parent commit, and show tricky conflict resolution part
> only for merge commits".
I'm afraid you need to pick a candidate that will be natural for '-d',
not just most useful output for your workflows, whatever it happens to
be.
> Before "--remerge-diff" was invented, my answer would have been "give
> patches to all single-parent commit, and show combined diff in the
> compact form for merge commits", aka "git log --cc".
And this is already there as well, or do you suggest
-d == --remerge-diff ?
-d == --cc ?
> Even though we did not know if a better output presentation for merge
> commits would be coming, we did not let it squat on any
> short-and-sweet single letter synonym.
Except -m and -c, and when "better" actually came where "better" means
basic functionality that should *better* have been there from the very
beginning, you argue against it.
And then, as in your view diff is not the best presentation for merge
commits, the best will have nothing to do with diff anyway, and so won't
be using '-d' with 99.99% probability.
Then, hopefully, somebody sometime will agree that we can finally use -d
for what it fits best: diff, plain old simple diff.
Overall, please expect a re-roll with another option name.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages
From: Jiang Xin @ 2023-10-04 13:02 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Junio C Hamano, Git List, Jonathan Tan, Jiang Xin
In-Reply-To: <ZRKax7Me5uIHKHoC@ugly>
On Tue, Sep 26, 2023 at 4:48 PM Oswald Buddenhagen
<oswald.buddenhagen@gmx.de> wrote:
>
> >Jiang Xin <worldhello.net@gmail.com> writes:
> >
> >> +++ b/pkt-line.c
> >> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> >> }
> >> + case 2:
> >> + /* fallthrough */
> >> + case 3:
> >
> while not entirely unprecedented, it's unnecessary and even
> counter-productive to annotate directly adjacent cases with fallthrough.
I see in "blame.c" there are directly adjacent cases like below. I
will remove the fallthrough statement.
case 'A':
case 'T':
/* Did not exist in parent, or type changed */
break;
Thanks.
^ permalink raw reply
* Re: [silly] loose, pack, and another thing?
From: Jeff King @ 2023-10-04 13:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, git
In-Reply-To: <xmqqo7hfwg6m.fsf@gitster.g>
On Tue, Oct 03, 2023 at 02:26:41PM -0700, Junio C Hamano wrote:
> > You can get some safety with the immutable attribute (which applies to
> > the inode itself, and thus any path that hardlinks to it). But setting
> > that usually requires being root. And it creates other irritations for
> > normal use (you have to unset it before even removing the hardlink).
>
> As a regular user, "chmod a-w" has the same characteristics (works
> at the inode level) but without "cannot remove it" downside. It
> used to be sufficient in RCS and CVS days, though, as a signal that
> you are only to look at it without touching it, to "chmod a-w" a
> path that is checked out but not for modifying. Some editors even
> offer to do chmod u+w for you when saving, so if we want absolute
> safety, it may not be enough.
Ah, right. For some reason I was thinking that only affected the link
entry, but of course the mode bits are on the linked inode itself. So
that does easily give some protection, though I agree that many programs
are happy to circumvent it for you.
It has been a long time since I've used it, but I think there may be
some prior at in git-annex:
https://git-annex.branchable.com/
IIRC it can work in a "copy" mode where contents are copied into the
working tree. But since the point is to deal with large data sets, it
also has a linking mode (maybe even symlinks?) that point directly from
the working tree into the annex storage. If we are considering a similar
feature, we might be able to learn from their experience.
-Peff
^ permalink raw reply
* Re: [PATCH v2] builtin/repack.c: avoid making cruft packs preferred
From: Jeff King @ 2023-10-04 13:13 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com>
On Tue, Oct 03, 2023 at 05:54:19PM -0400, Taylor Blau wrote:
> A small re-roll of the original patch, removing a few paragraphs from
> the proposed log message that are irrelevant and impossible to produce
> in today's git.git.
>
> For more details, see <ZRyNHRf+tQwV+T6P@nand.local>.
Thanks, this looks good to me, and the explanation in the linked message
makes sense.
-Peff
^ permalink raw reply
* [PATCH v3 0/3] Sideband demultiplexer fixes
From: Jiang Xin @ 2023-10-04 13:18 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <ZRKax7Me5uIHKHoC@ugly>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Sideband demultiplexer fixes.
Range-diff v2...v3
1: fd6d61893d ! 1: 68ac3ea711 pkt-line: do not chomp newlines for sideband messages
@@ Commit message
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 ##
@@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
+ len--;
+ break;
+ case 2:
-+ /* fallthrough */
+ case 3:
+ /*
+ * Do not chomp newline for progress and error
---
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 | 58 ++++++++++++++++++++++++++++++++++++----
t/t0070-fundamental.sh | 58 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 146 insertions(+), 10 deletions(-)
--
2.40.1.50.gf560bcc116.dirty
^ permalink raw reply
* [PATCH v3 2/3] pkt-line: memorize sideband fragment in reader
From: Jiang Xin @ 2023-10-04 13:18 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>
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 1053913d2d..a927c665d6 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 with demultiplex_sideband(), no chomp newline' '
+test_expect_success 'unpack-sideband with demultiplex_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 \
--
2.40.1.50.gf560bcc116.dirty
^ permalink raw reply related
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