* [PATCH 8/8] reftable/stack: fix stale lock when dying
From: Patrick Steinhardt @ 2023-11-21 7:04 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder
In-Reply-To: <cover.1700549493.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4515 bytes --]
When starting a transaction via `reftable_stack_init_addition()`, we
create a lockfile for the reftable stack itself which we'll write the
new list of tables to. But if we terminate abnormally e.g. via a call to
`die()`, then we do not remove the lockfile. Subsequent executions of
Git which try to modify references will thus fail with an out-of-date
error.
Fix this bug by registering the lock as a `struct tempfile`, which
ensures automatic cleanup for us.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 47 +++++++++++++++--------------------------------
1 file changed, 15 insertions(+), 32 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 2dd2373360..2f1494aef2 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -17,6 +17,8 @@ license that can be found in the LICENSE file or at
#include "reftable-merged.h"
#include "writer.h"
+#include "tempfile.h"
+
static int stack_try_add(struct reftable_stack *st,
int (*write_table)(struct reftable_writer *wr,
void *arg),
@@ -440,8 +442,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)
}
struct reftable_addition {
- int lock_file_fd;
- struct strbuf lock_file_name;
+ struct tempfile *lock_file;
struct reftable_stack *stack;
char **new_tables;
@@ -449,24 +450,19 @@ struct reftable_addition {
uint64_t next_update_index;
};
-#define REFTABLE_ADDITION_INIT \
- { \
- .lock_file_name = STRBUF_INIT \
- }
+#define REFTABLE_ADDITION_INIT {0}
static int reftable_stack_init_addition(struct reftable_addition *add,
struct reftable_stack *st)
{
+ struct strbuf lock_file_name = STRBUF_INIT;
int err = 0;
add->stack = st;
- strbuf_reset(&add->lock_file_name);
- strbuf_addstr(&add->lock_file_name, st->list_file);
- strbuf_addstr(&add->lock_file_name, ".lock");
+ strbuf_addf(&lock_file_name, "%s.lock", st->list_file);
- add->lock_file_fd = open(add->lock_file_name.buf,
- O_EXCL | O_CREAT | O_WRONLY, 0666);
- if (add->lock_file_fd < 0) {
+ add->lock_file = create_tempfile(lock_file_name.buf);
+ if (!add->lock_file) {
if (errno == EEXIST) {
err = REFTABLE_LOCK_ERROR;
} else {
@@ -475,7 +471,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
goto done;
}
if (st->config.default_permissions) {
- if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {
+ if (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
@@ -495,6 +491,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
if (err) {
reftable_addition_close(add);
}
+ strbuf_release(&lock_file_name);
return err;
}
@@ -512,15 +509,7 @@ static void reftable_addition_close(struct reftable_addition *add)
add->new_tables = NULL;
add->new_tables_len = 0;
- if (add->lock_file_fd > 0) {
- close(add->lock_file_fd);
- add->lock_file_fd = 0;
- }
- if (add->lock_file_name.len > 0) {
- unlink(add->lock_file_name.buf);
- strbuf_release(&add->lock_file_name);
- }
-
+ delete_tempfile(&add->lock_file);
strbuf_release(&nm);
}
@@ -536,8 +525,10 @@ void reftable_addition_destroy(struct reftable_addition *add)
int reftable_addition_commit(struct reftable_addition *add)
{
struct strbuf table_list = STRBUF_INIT;
+ int lock_file_fd = get_tempfile_fd(add->lock_file);
int i = 0;
int err = 0;
+
if (add->new_tables_len == 0)
goto done;
@@ -550,28 +541,20 @@ int reftable_addition_commit(struct reftable_addition *add)
strbuf_addstr(&table_list, "\n");
}
- err = write_in_full(add->lock_file_fd, table_list.buf, table_list.len);
+ err = write_in_full(lock_file_fd, table_list.buf, table_list.len);
strbuf_release(&table_list);
if (err < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
- err = close(add->lock_file_fd);
- add->lock_file_fd = 0;
- if (err < 0) {
- err = REFTABLE_IO_ERROR;
- goto done;
- }
-
- err = rename(add->lock_file_name.buf, add->stack->list_file);
+ err = rename_tempfile(&add->lock_file, add->stack->list_file);
if (err < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
/* success, no more state to clean up. */
- strbuf_release(&add->lock_file_name);
for (i = 0; i < add->new_tables_len; i++) {
reftable_free(add->new_tables[i]);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* reftable: How to represent deleted referees of symrefs in the reflog?
From: Patrick Steinhardt @ 2023-11-21 13:46 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Terry Parker
[-- Attachment #1: Type: text/plain, Size: 5004 bytes --]
Hi,
I'm currently trying to fully understand how exactly reflogs and reflog
entries are supposed to be deleted in the reftable backend. For ref
records it's easy enough, as explained in the technical documentation
for reftables that is part of our tree:
> Deletion of any reference can be explicitly stored by setting the type
> to 0x0 and omitting the value field of the ref_record. This serves as
> a tombstone, overriding any assertions about the existence of the
> reference from earlier files in the stack.
So you simply create a new ref record with type 0x0 and are done.
For log records it seems to be a bit of a different story though.
Again, quoting the technical documentation:
> The log_type = 0x0 is mostly useful for git stash drop, removing an
> entry from the reflog of refs/stash in a transaction file (below),
> without needing to rewrite larger files. Readers reading a stack of
> reflogs must treat this as a deletion.
To me it seems like deletions in this case only delete a particular log
entry instead of the complete log for a particular reference. And some
older discussion [1] seems to confirm my hunch that a complete reflog is
deleted not with `log_type = 0x0`, but instead by writing the null
object ID as new ID.
So: a single entry is deleted with `log_type = 0x0`, the complete reflog
entry is deleted with the null object ID as new ID. Fair enough, even
though the documentation could be updated to make this easier to
understand. I'll happily do so if my understanding is correct here.
In any case though, this proposed behaviour is not sufficient to cover
all cases that the files-based reflog supports. The following case may
be weird, but we do have tests for it in t1400:
```
$ git init repo
Initialized empty Git repository in /tmp/repo/.git/
$ cd repo/
$ git commit --allow-empty --message initial-commit
[main (root-commit) 9f10b3f] initial-commit
# Everything looks as expected up to this point.
$ git reflog show HEAD
9f10b3f (HEAD -> main) HEAD@{0}: commit (initial): initial-commit
# This behaviour is a bit more on the weird side. We delete the
# referee, and that causes the files backend to claim that the reflog
# for HEAD is gone, as well. The reflog still exists though, as
# demonstrated in the next command.
$ git update-ref -m delete-main -d refs/heads/main
$ git reflog show HEAD
fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
# We now re-create the referee, which revives the reflog _including_
# the old entry.
$ git update-ref -m recreate-main refs/heads/main 9f10b3f
$ git reflog show HEAD
9f10b3f (HEAD -> main) HEAD@{0}: recreate-main
9f10b3f (HEAD -> main) HEAD@{2}: commit (initial): initial-commit
$ cat .git/logs/HEAD
0000000000000000000000000000000000000000 9f10b3f9b20962690fdeff76cd592722fbf57deb Patrick Steinhardt <ps@pks.im> 1700573003 +0100 commit (initial): initial-commit
9f10b3f9b20962690fdeff76cd592722fbf57deb 0000000000000000000000000000000000000000 Patrick Steinhardt <ps@pks.im> 1700573060 +0100 delete-main
0000000000000000000000000000000000000000 9f10b3f9b20962690fdeff76cd592722fbf57deb Patrick Steinhardt <ps@pks.im> 1700573078 +0100 recreate-main
```
It kind of feels like the second step in the files backend where the
reflog is claimed to not exist is buggy -- I'd have expected to still
see the reflog, as the HEAD reference exists quite alright and has never
stopped to exist. And in the third step, I'd have expected to see three
reflog entries, including the deletion that is indeed still present in
the on-disk logfile.
But with the reftable backend the problem becomes worse: we cannot even
represent the fact that the reflog still exists, but that the deletion
of the referee has caused the HEAD to point to the null OID, because the
null OID indicates complete deletion of the reflog. Consequentially, if
we wrote the null OID, we'd only be able to see the last log entry here.
It may totally be that I'm missing something obvious here. But if not,
it leaves us with a couple of options for how to fix it:
1. Disregard this edge case and accept that the reftable backend
does not do exactly the same thing as the files backend in very
specific situations like this.
2. Change the reftable format so that it can discern these cases,
e.g. by indicating deletion via a new log type.
3. Change the files backend to adapt.
None of these alternatives feel particularly great to me. In my opinion
(2) is likely the best option, but would require us to change the format
format that's already in use by Google in the context of multiple
projects. So I'm not quite sure how thrilled you folks at Google and
other users of the reftable library are with this prospect.
Anyway, happy to hear about alternative takes or corrections.
Patrick
[1]: <CAFQ2z_P-cf38yR-VyvfDSgPUO_d38mgsi32UkRSPWMZKJOmjZg@mail.gmail.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Migration of git-scm.com to a static web site: ready for review/testing
From: Johannes Schindelin @ 2023-11-21 14:25 UTC (permalink / raw)
To: Todd Zullinger; +Cc: git, Matt Burke, Victoria Dye, Matthias Aßhauer
In-Reply-To: <ZVgoKPAg6jKZk_M6@pobox.com>
[-- Attachment #1: Type: text/plain, Size: 4881 bytes --]
Hi Todd,
On Fri, 17 Nov 2023, Todd Zullinger wrote:
> Johannes Schindelin wrote:
> >> For checking links, a tool like linkcheker[1] is very handy.
> >> This is run against the local docs in the Fedora package
> >> builds to catch broken links.
> >
> > Hmm, `linkchecker` is really slow for me, even locally.
>
> Yeah, it took an hour and a half to run for me, both on an
> old laptop and a fast server with plenty of threads,
> bandwidth, and memory.
>
> Checking the git HTML documentation takes under 30 seconds,
> which is largely the only place I've used it. It has been
> very helpful in catching broken links in the docs during the
> build and the time is short enough that I never minded.
I found https://lychee.cli.rs/#/ in the meantime and figured out how to
use it in a local setup:
First, I run:
HUGO_TIMEOUT=777 HUGO_BASEURL= HUGO_UGLYURLS=false time hugo
The first `HUGO_*` setting is to make sure that even though I sometimes
use all of the cores of my laptop's CPU it should not fail. The other two
are to override settings from `hugo.yml` so that `lychee` can handle the
output (`lychee` will not auto-append `.html`, unlike GitHub Pages, and
would therefore mis-detect tons of broken links, without
`HUGO_UGLYURLS=false`).
In my setup, this command typically runs for something like half a minute,
but sometimes takes for as long as 1 minute. (I noticed that it is much
slower when I open the directory in VS Code because I'm running this in
WSL and the filesystem watcher kind of eats all resources.)
After that, I run:
time lychee --offline --exclude-mail \
--exclude file:///path/to/repo.git/ \
--exclude file:///caminho/para/o/reposit%C3%B3rio.git/ \
--exclude file:///ruta/a/repositorio.git/ \
--exclude file:///sl%C3%B3%C3%B0/til/hirsla.git/ \
--exclude file:///Pfad/zum/Repo.git/ \
--exclude file:///chemin/du/d%C3%A9p%C3%B4t.git/ \
--exclude file:///srv/git/project.git \
--exclude "file://$PWD/public/pagefind/pagefind-ui.css" \
--format markdown -o lychee-local.md public/
Without `--offline`, there would be a couple of broken links (the
http://git.or.cz/gitwiki/InterfacesFrontendsAndTools link leads to
"Forbidden", it needs to be changed to https://).
The `file:///` URLs are all examples that are not expected to be valid.
And we do not want to check the emails (tons of `xyz@example.com` would be
"broken").
This command typically takes another half minute, sometimes a bit longer.
Given those times and the configurability (and the lure of a GitHub
Action that could be easily integrated into a GitHub workflow:
https://github.com/marketplace/actions/lychee-broken-link-checker), I have
up on linkchecker and focused exclusively on lychee.
Now, when I started working on this on Friday, lychee reported about
12,000 broken links.
There were a couple of legitimate mistakes I made (when feeding paths to
Hugo's `relURL` function, the path must not have a leading slash or it
will remain unchanged, for example). These are fixed.
But there were also many other issues such as some manual page translation
being incomplete yet linking to not-yet-existing pages. In those cases, I
changed he code to generate redirects to the English version. For example,
https://git.github.io/git-scm.com/docs/git-clone/fr#_git has a link to
`git[1]` that _should_ lead to the French version of the `git` manual
page. However, that does not exist. So both the Rails App as well as the
static website redirect to the English variant of that page.
My most recent lychee run results in 0 broken links.
As a bonus, some of the links that are currently broken on
https://git-scm.com/ are fixed in https://git.github.io/git-scm.com/.
For example, following the `Pull Request Referləri` link at the top of
https://git-scm.com/book/az/v2/Appendix-C:-Git-%C6%8Fmrl%C9%99ri-Plumbing-%C6%8Fmrl%C9%99ri/
leads to a 404. But following it in
https://git.github.io/git-scm.com/book/az/v2/Appendix-C:-Git-%C6%8Fmrl%C9%99ri-Plumbing-%C6%8Fmrl%C9%99ri/
directs the browser to the correct URL:
https://git.github.io/git-scm.com/book/az/v2/GitHub-Bir-Layih%C9%99nin-Saxlan%C4%B1lmas%C4%B1/#_pr_refs
Another thing that is broken on https://git-scm.com/ are the footnotes in
the Czech translation of the ProGit book. These were broken in the Hugo
version, too, but now they are fixed. See e.g.
https://dscho.github.io/git-scm.com/book/cs/v2/Z%C3%A1klady-pr%C3%A1ce-se-syst%C3%A9mem-Git-Zobrazen%C3%AD-historie-reviz%C3%AD/#_footnotedef_7
and note that the Rails App redirects to
https://git-scm.com/book/cs/v2/Z%C3%A1klady-pr%C3%A1ce-se-syst%C3%A9mem-Git-Zobrazen%C3%AD-historie-reviz%C3%AD/ch00/_footnotedef_7
when clicking on the `[7]`, which 404s.
Could you double-check that the links in the current version?
Thank you,
Johannes
^ permalink raw reply
* Re: [PATCH v2] merge-file: add --diff-algorithm option
From: Phillip Wood @ 2023-11-21 14:58 UTC (permalink / raw)
To: Antonin Delpeuch via GitGitGadget, git; +Cc: Antonin Delpeuch
In-Reply-To: <pull.1606.v2.git.git.1700507932937.gitgitgadget@gmail.com>
Hi Antonin
On 20/11/2023 19:18, Antonin Delpeuch via GitGitGadget wrote:
> From: Antonin Delpeuch <antonin@delpeuch.eu>
>
> This makes it possible to use other diff algorithms than the 'myers'
> default algorithm, when using the 'git merge-file' command. This helps
> avoid spurious conflicts by selecting a more recent algorithm such as
> 'histogram', for instance when using 'git merge-file' as part of a custom
> merge driver.
>
> Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>
> Reviewed-by: Phillip Wood <phillip.wood@dunelm.org.uk>
This version looks good to me. Thanks for adding the tests and well done
for finding a test case that shows the benefits of changing the diff
algorithm so clearly.
For future reference note that the custom on this list is not to add
"Reviewed-by:" unless the reviewer explicitly suggests it. In this case
I'm happy for it to be left as is.
Best Wishes
Phillip
> ---
> merge-file: add --diff-algorithm option
>
> Changes since v1:
>
> * improve commit message to mention the use case of custom merge
> drivers
> * improve documentation to show available options and recommend
> switching to "histogram"
> * add tests
>
> I have left out:
>
> * switching the default to "histogram", because it should only be done
> in a subsequent release
> * adding a configuration variable to control this option, because I was
> not sure how to call it. Perhaps "merge-file.diffAlgorithm"?
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1606%2Fwetneb%2Fmerge_file_configurable_diff_algorithm-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1606/wetneb/merge_file_configurable_diff_algorithm-v2
> Pull-Request: https://github.com/git/git/pull/1606
>
> Range-diff vs v1:
>
> 1: 4aa453e30be ! 1: 842b5abf33c merge-file: add --diff-algorithm option
> @@ Commit message
> merge-file: add --diff-algorithm option
>
> This makes it possible to use other diff algorithms than the 'myers'
> - default algorithm, when using the 'git merge-file' command.
> + default algorithm, when using the 'git merge-file' command. This helps
> + avoid spurious conflicts by selecting a more recent algorithm such as
> + 'histogram', for instance when using 'git merge-file' as part of a custom
> + merge driver.
>
> Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>
> + Reviewed-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> ## Documentation/git-merge-file.txt ##
> @@ Documentation/git-merge-file.txt: object store and the object ID of its blob is written to standard output.
> Instead of leaving conflicts in the file, resolve conflicts
> favouring our (or their or both) side of the lines.
>
> -+--diff-algorithm <algorithm>::
> -+ Use a different diff algorithm while merging, which can help
> ++--diff-algorithm={patience|minimal|histogram|myers}::
> ++ Use a different diff algorithm while merging. The current default is "myers",
> ++ but selecting more recent algorithm such as "histogram" can help
> + avoid mismerges that occur due to unimportant matching lines
> -+ (such as braces from distinct functions). See also
> ++ (such as braces from distinct functions). See also
> + linkgit:git-diff[1] `--diff-algorithm`.
>
> EXAMPLES
> @@ builtin/merge-file.c: int cmd_merge_file(int argc, const char **argv, const char
> OPT_INTEGER(0, "marker-size", &xmp.marker_size,
> N_("for conflicts, use this marker size")),
> OPT__QUIET(&quiet, N_("do not warn about conflicts")),
> +
> + ## t/t6403-merge-file.sh ##
> +@@ t/t6403-merge-file.sh: test_expect_success 'setup' '
> + deduxit me super semitas jusitiae,
> + EOF
> +
> +- printf "propter nomen suum." >>new4.txt
> ++ printf "propter nomen suum." >>new4.txt &&
> ++
> ++ cat >base.c <<-\EOF &&
> ++ int f(int x, int y)
> ++ {
> ++ if (x == 0)
> ++ {
> ++ return y;
> ++ }
> ++ return x;
> ++ }
> ++
> ++ int g(size_t u)
> ++ {
> ++ while (u < 30)
> ++ {
> ++ u++;
> ++ }
> ++ return u;
> ++ }
> ++ EOF
> ++
> ++ cat >ours.c <<-\EOF &&
> ++ int g(size_t u)
> ++ {
> ++ while (u < 30)
> ++ {
> ++ u++;
> ++ }
> ++ return u;
> ++ }
> ++
> ++ int h(int x, int y, int z)
> ++ {
> ++ if (z == 0)
> ++ {
> ++ return x;
> ++ }
> ++ return y;
> ++ }
> ++ EOF
> ++
> ++ cat >theirs.c <<-\EOF
> ++ int f(int x, int y)
> ++ {
> ++ if (x == 0)
> ++ {
> ++ return y;
> ++ }
> ++ return x;
> ++ }
> ++
> ++ int g(size_t u)
> ++ {
> ++ while (u > 34)
> ++ {
> ++ u--;
> ++ }
> ++ return u;
> ++ }
> ++ EOF
> + '
> +
> + test_expect_success 'merge with no changes' '
> +@@ t/t6403-merge-file.sh: test_expect_success '--object-id fails without repository' '
> + grep "not a git repository" err
> + '
> +
> ++test_expect_success 'merging C files with "myers" diff algorithm creates some spurious conflicts' '
> ++ cat >expect.c <<-\EOF &&
> ++ int g(size_t u)
> ++ {
> ++ while (u < 30)
> ++ {
> ++ u++;
> ++ }
> ++ return u;
> ++ }
> ++
> ++ int h(int x, int y, int z)
> ++ {
> ++ <<<<<<< ours.c
> ++ if (z == 0)
> ++ ||||||| base.c
> ++ while (u < 30)
> ++ =======
> ++ while (u > 34)
> ++ >>>>>>> theirs.c
> ++ {
> ++ <<<<<<< ours.c
> ++ return x;
> ++ ||||||| base.c
> ++ u++;
> ++ =======
> ++ u--;
> ++ >>>>>>> theirs.c
> ++ }
> ++ return y;
> ++ }
> ++ EOF
> ++
> ++ test_must_fail git merge-file -p --diff3 --diff-algorithm myers ours.c base.c theirs.c >myers_output.c &&
> ++ test_cmp expect.c myers_output.c
> ++'
> ++
> ++test_expect_success 'merging C files with "histogram" diff algorithm avoids some spurious conflicts' '
> ++ cat >expect.c <<-\EOF &&
> ++ int g(size_t u)
> ++ {
> ++ while (u > 34)
> ++ {
> ++ u--;
> ++ }
> ++ return u;
> ++ }
> ++
> ++ int h(int x, int y, int z)
> ++ {
> ++ if (z == 0)
> ++ {
> ++ return x;
> ++ }
> ++ return y;
> ++ }
> ++ EOF
> ++
> ++ git merge-file -p --diff3 --diff-algorithm histogram ours.c base.c theirs.c >histogram_output.c &&
> ++ test_cmp expect.c histogram_output.c
> ++'
> ++
> + test_done
>
>
> Documentation/git-merge-file.txt | 6 ++
> builtin/merge-file.c | 28 +++++++
> t/t6403-merge-file.sh | 124 ++++++++++++++++++++++++++++++-
> 3 files changed, 157 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
> index 6a081eacb72..71915a00fa4 100644
> --- a/Documentation/git-merge-file.txt
> +++ b/Documentation/git-merge-file.txt
> @@ -92,6 +92,12 @@ object store and the object ID of its blob is written to standard output.
> Instead of leaving conflicts in the file, resolve conflicts
> favouring our (or their or both) side of the lines.
>
> +--diff-algorithm={patience|minimal|histogram|myers}::
> + Use a different diff algorithm while merging. The current default is "myers",
> + but selecting more recent algorithm such as "histogram" can help
> + avoid mismerges that occur due to unimportant matching lines
> + (such as braces from distinct functions). See also
> + linkgit:git-diff[1] `--diff-algorithm`.
>
> EXAMPLES
> --------
> diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> index 832c93d8d54..1f987334a31 100644
> --- a/builtin/merge-file.c
> +++ b/builtin/merge-file.c
> @@ -1,5 +1,6 @@
> #include "builtin.h"
> #include "abspath.h"
> +#include "diff.h"
> #include "hex.h"
> #include "object-name.h"
> #include "object-store.h"
> @@ -28,6 +29,30 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> +static int set_diff_algorithm(xpparam_t *xpp,
> + const char *alg)
> +{
> + long diff_algorithm = parse_algorithm_value(alg);
> + if (diff_algorithm < 0)
> + return -1;
> + xpp->flags = (xpp->flags & ~XDF_DIFF_ALGORITHM_MASK) | diff_algorithm;
> + return 0;
> +}
> +
> +static int diff_algorithm_cb(const struct option *opt,
> + const char *arg, int unset)
> +{
> + xpparam_t *xpp = opt->value;
> +
> + BUG_ON_OPT_NEG(unset);
> +
> + if (set_diff_algorithm(xpp, arg))
> + return error(_("option diff-algorithm accepts \"myers\", "
> + "\"minimal\", \"patience\" and \"histogram\""));
> +
> + return 0;
> +}
> +
> int cmd_merge_file(int argc, const char **argv, const char *prefix)
> {
> const char *names[3] = { 0 };
> @@ -48,6 +73,9 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
> XDL_MERGE_FAVOR_THEIRS),
> OPT_SET_INT(0, "union", &xmp.favor, N_("for conflicts, use a union version"),
> XDL_MERGE_FAVOR_UNION),
> + OPT_CALLBACK_F(0, "diff-algorithm", &xmp.xpp, N_("<algorithm>"),
> + N_("choose a diff algorithm"),
> + PARSE_OPT_NONEG, diff_algorithm_cb),
> OPT_INTEGER(0, "marker-size", &xmp.marker_size,
> N_("for conflicts, use this marker size")),
> OPT__QUIET(&quiet, N_("do not warn about conflicts")),
> diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
> index 2c92209ecab..fb872c5a113 100755
> --- a/t/t6403-merge-file.sh
> +++ b/t/t6403-merge-file.sh
> @@ -56,7 +56,67 @@ test_expect_success 'setup' '
> deduxit me super semitas jusitiae,
> EOF
>
> - printf "propter nomen suum." >>new4.txt
> + printf "propter nomen suum." >>new4.txt &&
> +
> + cat >base.c <<-\EOF &&
> + int f(int x, int y)
> + {
> + if (x == 0)
> + {
> + return y;
> + }
> + return x;
> + }
> +
> + int g(size_t u)
> + {
> + while (u < 30)
> + {
> + u++;
> + }
> + return u;
> + }
> + EOF
> +
> + cat >ours.c <<-\EOF &&
> + int g(size_t u)
> + {
> + while (u < 30)
> + {
> + u++;
> + }
> + return u;
> + }
> +
> + int h(int x, int y, int z)
> + {
> + if (z == 0)
> + {
> + return x;
> + }
> + return y;
> + }
> + EOF
> +
> + cat >theirs.c <<-\EOF
> + int f(int x, int y)
> + {
> + if (x == 0)
> + {
> + return y;
> + }
> + return x;
> + }
> +
> + int g(size_t u)
> + {
> + while (u > 34)
> + {
> + u--;
> + }
> + return u;
> + }
> + EOF
> '
>
> test_expect_success 'merge with no changes' '
> @@ -447,4 +507,66 @@ test_expect_success '--object-id fails without repository' '
> grep "not a git repository" err
> '
>
> +test_expect_success 'merging C files with "myers" diff algorithm creates some spurious conflicts' '
> + cat >expect.c <<-\EOF &&
> + int g(size_t u)
> + {
> + while (u < 30)
> + {
> + u++;
> + }
> + return u;
> + }
> +
> + int h(int x, int y, int z)
> + {
> + <<<<<<< ours.c
> + if (z == 0)
> + ||||||| base.c
> + while (u < 30)
> + =======
> + while (u > 34)
> + >>>>>>> theirs.c
> + {
> + <<<<<<< ours.c
> + return x;
> + ||||||| base.c
> + u++;
> + =======
> + u--;
> + >>>>>>> theirs.c
> + }
> + return y;
> + }
> + EOF
> +
> + test_must_fail git merge-file -p --diff3 --diff-algorithm myers ours.c base.c theirs.c >myers_output.c &&
> + test_cmp expect.c myers_output.c
> +'
> +
> +test_expect_success 'merging C files with "histogram" diff algorithm avoids some spurious conflicts' '
> + cat >expect.c <<-\EOF &&
> + int g(size_t u)
> + {
> + while (u > 34)
> + {
> + u--;
> + }
> + return u;
> + }
> +
> + int h(int x, int y, int z)
> + {
> + if (z == 0)
> + {
> + return x;
> + }
> + return y;
> + }
> + EOF
> +
> + git merge-file -p --diff3 --diff-algorithm histogram ours.c base.c theirs.c >histogram_output.c &&
> + test_cmp expect.c histogram_output.c
> +'
> +
> test_done
>
> base-commit: 98009afd24e2304bf923a64750340423473809ff
^ permalink raw reply
* [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>
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.
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>
---
Notes:
v2 changes:
- fixed code style
- replaced potentially unsafe use of `sub->path` with `path`
builtin/submodule--helper.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6f3bf33e61..af461ada8b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2901,19 +2901,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);
config_set_in_gitmodules_file_gently(config_name, newurl);
+
+ repo_read_gitmodules(the_repository, 0);
sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
free(config_name);
-
return 0;
}
@@ -2941,19 +2948,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);
free(config_name);
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/6] submodule--helper: return error from set-url when modifying failed
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
Jan Alexander Steffens (heftig)
In-Reply-To: <20231121203413.176414-1-heftig@archlinux.org>
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.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
builtin/submodule--helper.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index af461ada8b..0013ea1ab0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2889,39 +2889,41 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
static int module_set_url(int argc, const char **argv, const char *prefix)
{
- int quiet = 0;
+ int quiet = 0, ret;
const char *newurl;
const char *path;
char *config_name;
struct option options[] = {
OPT__QUIET(&quiet, N_("suppress output for setting url of a submodule")),
OPT_END()
};
const char *const usage[] = {
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);
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);
- config_set_in_gitmodules_file_gently(config_name, newurl);
+ ret = config_set_in_gitmodules_file_gently(config_name, newurl);
- repo_read_gitmodules(the_repository, 0);
- sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
+ if (!ret) {
+ repo_read_gitmodules(the_repository, 0);
+ sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
+ }
free(config_name);
- return 0;
+ return !!ret;
}
static int module_set_branch(int argc, const char **argv, const char *prefix)
--
2.43.0
^ permalink raw reply related
* [PATCH v2 4/6] t7419, t7420: use test_cmp_config instead of grepping .gitmodules
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
Jan Alexander Steffens (heftig)
In-Reply-To: <20231121203413.176414-1-heftig@archlinux.org>
We have a test function to verify config files. Use it as it's more
precise.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
t/t7419-submodule-set-branch.sh | 10 +++++-----
t/t7420-submodule-set-url.sh | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 5ac16d0eb7..3cd30865a7 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -44,53 +44,53 @@ test_expect_success 'submodule config cache setup' '
test_expect_success 'ensure submodule branch is unset' '
(cd super &&
- ! grep branch .gitmodules
+ test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch
)
'
test_expect_success 'test submodule set-branch --branch' '
(cd super &&
git submodule set-branch --branch topic submodule &&
- grep "branch = topic" .gitmodules &&
+ test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
git submodule update --remote &&
cat <<-\EOF >expect &&
b
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch --default' '
(cd super &&
git submodule set-branch --default submodule &&
- ! grep branch .gitmodules &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
git submodule update --remote &&
cat <<-\EOF >expect &&
a
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch -b' '
(cd super &&
git submodule set-branch -b topic submodule &&
- grep "branch = topic" .gitmodules &&
+ test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
git submodule update --remote &&
cat <<-\EOF >expect &&
b
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch -d' '
(cd super &&
git submodule set-branch -d submodule &&
- ! grep branch .gitmodules &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
git submodule update --remote &&
cat <<-\EOF >expect &&
a
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index d6bf62b3ac..aa63d806fe 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -49,7 +49,7 @@ test_expect_success 'test submodule set-url' '
cd super &&
test_must_fail git submodule update --remote &&
git submodule set-url submodule ../newsubmodule &&
- grep -F "url = ../newsubmodule" .gitmodules &&
+ test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
git submodule update --remote
) &&
git -C super/submodule show >actual &&
--
2.43.0
^ permalink raw reply related
* [PATCH v2 3/6] t7419: actually test the branch switching
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
Jan Alexander Steffens (heftig)
In-Reply-To: <20231121203413.176414-1-heftig@archlinux.org>
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.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
Notes:
v2 changes:
- fixed subject
t/t7419-submodule-set-branch.sh | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
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
+
. ./test-lib.sh
test_expect_success 'setup' '
git config --global protocol.file.allow always
'
test_expect_success 'submodule config cache setup' '
mkdir submodule &&
(cd submodule &&
git init &&
echo a >a &&
git add . &&
git commit -ma &&
git checkout -b topic &&
echo b >a &&
git add . &&
- git commit -mb
+ git commit -mb &&
+ git checkout main
) &&
mkdir super &&
(cd super &&
@@ -57,41 +62,38 @@ test_expect_success 'test submodule set-branch --branch' '
'
test_expect_success 'test submodule set-branch --default' '
- test_commit -C submodule c &&
(cd super &&
git submodule set-branch --default submodule &&
! grep branch .gitmodules &&
git submodule update --remote &&
cat <<-\EOF >expect &&
- c
+ a
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch -b' '
- test_commit -C submodule b &&
(cd super &&
git submodule set-branch -b topic submodule &&
grep "branch = topic" .gitmodules &&
git submodule update --remote &&
cat <<-\EOF >expect &&
b
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch -d' '
- test_commit -C submodule d &&
(cd super &&
git submodule set-branch -d submodule &&
! grep branch .gitmodules &&
git submodule update --remote &&
cat <<-\EOF >expect &&
- d
+ a
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
--
2.43.0
^ permalink raw reply related
* [PATCH v2 5/6] t7419: test that we correctly handle renamed submodules
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
Jan Alexander Steffens (heftig)
In-Reply-To: <20231121203413.176414-1-heftig@archlinux.org>
Add the submodule again with an explicitly different name and path. Test
that calling set-branch modifies the correct .gitmodules entries. Make
sure we don't create a section named after the path instead of the name.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
t/t7419-submodule-set-branch.sh | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 3cd30865a7..a5d1bc5c54 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -38,7 +38,8 @@ test_expect_success 'submodule config cache setup' '
(cd super &&
git init &&
git submodule add ../submodule &&
- git commit -m "add submodule"
+ git submodule add --name thename ../submodule thepath &&
+ git commit -m "add submodules"
)
'
@@ -100,4 +101,31 @@ test_expect_success 'test submodule set-branch -d' '
)
'
+test_expect_success 'test submodule set-branch --branch with named submodule' '
+ (cd super &&
+ git submodule set-branch --branch topic thepath &&
+ test_cmp_config topic -f .gitmodules submodule.thename.branch &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.thepath.branch &&
+ git submodule update --remote &&
+ cat <<-\EOF >expect &&
+ b
+ EOF
+ git -C thepath show -s --pretty=%s >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'test submodule set-branch --default with named submodule' '
+ (cd super &&
+ git submodule set-branch --default thepath &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.thename.branch &&
+ git submodule update --remote &&
+ cat <<-\EOF >expect &&
+ a
+ EOF
+ git -C thepath show -s --pretty=%s >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* [PATCH v2 6/6] t7420: test that we correctly handle renamed submodules
From: Jan Alexander Steffens (heftig) @ 2023-11-21 20:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Shourya Shukla,
Ævar Arnfjörð Bjarmason, Denton Liu,
Jan Alexander Steffens (heftig)
In-Reply-To: <20231121203413.176414-1-heftig@archlinux.org>
Create a second submodule with a name that differs from its path. Test
that calling set-url modifies the correct .gitmodules entries. Make sure
we don't create a section named after the path instead of the name.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
t/t7420-submodule-set-url.sh | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index aa63d806fe..bf7f15ee79 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -25,34 +25,56 @@ test_expect_success 'submodule config cache setup' '
git add file &&
git commit -ma
) &&
+ mkdir namedsubmodule &&
+ (
+ cd namedsubmodule &&
+ git init &&
+ echo 1 >file &&
+ git add file &&
+ git commit -m1
+ ) &&
mkdir super &&
(
cd super &&
git init &&
git submodule add ../submodule &&
- git commit -m "add submodule"
+ git submodule add --name thename ../namedsubmodule thepath &&
+ git commit -m "add submodules"
)
'
test_expect_success 'test submodule set-url' '
- # add a commit and move the submodule (change the url)
+ # add commits and move the submodules (change the urls)
(
cd submodule &&
echo b >>file &&
git add file &&
git commit -mb
) &&
mv submodule newsubmodule &&
+ (
+ cd namedsubmodule &&
+ echo 2 >>file &&
+ git add file &&
+ git commit -m2
+ ) &&
+ mv namedsubmodule newnamedsubmodule &&
+
git -C newsubmodule show >expect &&
+ git -C newnamedsubmodule show >>expect &&
(
cd super &&
test_must_fail git submodule update --remote &&
git submodule set-url submodule ../newsubmodule &&
test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
+ git submodule set-url thepath ../newnamedsubmodule &&
+ test_cmp_config ../newnamedsubmodule -f .gitmodules submodule.thename.url &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.thepath.url &&
git submodule update --remote
) &&
git -C super/submodule show >actual &&
+ git -C super/thepath show >>actual &&
test_cmp expect actual
'
--
2.43.0
^ permalink raw reply related
* Orphan branch not well-defined?
From: Craig H Maynard @ 2023-11-22 0:28 UTC (permalink / raw)
To: Git Community
Team,
Recently I tried creating an orphan branch in an existing repo using git-checkout and git-switch.
Both commands have an --orphan option.
The results were different:
git checkout --orphan <newbranch> retained the entire working tree, including subdirectories and their contents.
git switch --orphan <newbranch> retained subdirectories but NOT their content.
Leaving aside the question of whether or not this is a bug, there doesn't appear to be any formal definition of the term "orphan branch" in the git documentation. Am I missing something?
Thanks,
Craig
^ permalink raw reply
* Re: Orphan branch not well-defined?
From: Junio C Hamano @ 2023-11-22 1:08 UTC (permalink / raw)
To: Craig H Maynard; +Cc: Git Community
In-Reply-To: <FE2AD666-88DE-4F70-8D6D-3A426689EB41@me.com>
Craig H Maynard <chmaynard@me.com> writes:
> Recently I tried creating an orphan branch in an existing repo using git-checkout and git-switch.
>
> Both commands have an --orphan option.
>
> The results were different:
This is one of the very much deliberate differences between "switch"
and "checkout" (there are others).
The "switch" command was introduced as an experiment to figure out a
better UI choices for one half of "checkout", which deals with
checking out a branch to a working tree (the other half being
"restore", which is about checking out files out of a tree-ish).
The initial round of "switch" proposed to go with the identical
semantics as "checkout" for the "--orphan" option [*1*], but during
review discussion, a concensus was reached that a better behaviour
for creating an entirely new history may be to start from void
[*2*], and that is what is in the experimental command you see.
[Reference]
*1* https://lore.kernel.org/git/20190130094831.10420-9-pclouds@gmail.com/
*2* https://lore.kernel.org/git/CABPp-BF3_p3+fmQcWYEu2z3J4FfPmDmiMyFiBRXyz8TxKLL7jA@mail.gmail.com/
^ permalink raw reply
* Re: Orphan branch not well-defined?
From: Chris Torek @ 2023-11-22 1:42 UTC (permalink / raw)
To: Craig H Maynard; +Cc: Git Community
In-Reply-To: <FE2AD666-88DE-4F70-8D6D-3A426689EB41@me.com>
On Tue, Nov 21, 2023 at 4:36 PM Craig H Maynard <chmaynard@me.com> wrote:
> [git checkout and git switch treat --orphan differently]
>
> Leaving aside the question of whether or not this is a bug,
Just to answer the implied question: this is intentional.
> there doesn't appear to be any formal definition of the term "orphan branch"
> in the git documentation. Am I missing something?
Whether it's documented anywhere or not, it's not done well. This is
not surprising: It is hard to do it well! Git uses two phrases for
this: "orphan branch" and "unborn branch". To understand them
properly, let's start at the real beginning. Bear with me for a
moment here.
In Git, the identity of a commit -- the way that Git locates the
commit internally -- is its hash ID. (Aside: until the SHA-256
conversion, therewas only ever one hash ID for any commit ever made
anywhere. Now that Git supports both SHA-1 and SHA-256, there are two
possible IDs, depending on which scheme you're using.) It's possible,
at least in theory, to use Git without ever creating a branch name:
all you have to do is memorize these random-looking hash IDs. But
that's not how people's brains work, and it's quite impractical. So
Git offers us branch names, like "main" or "master", "dev" or
"develop", and so on.
In Git, a branch name is just a human-readable name for one of Git's
internal hash IDs, with a special and very useful property that
distinguishes it from a tag name. Each tag name is a human-readable
name for a hash ID too; they just lack the special property of the
branch names. We won't get into all the properties here though, and
for the moment, we just need to know that the name stands in as a
memorable version of the ID.
As a result, a Git branch name literally cannot exist unless it
identifies one specific commit! We call that one specific commit the
"tip commit" of that branch (which introduces a whole new confusion,
of whether a "branch" is *one commit* or *many commits*, but again we
won't get into this here).
This leaves us with a big "chicken or egg" problem
(https://en.wikipedia.org/wiki/Chicken_or_the_egg). Suppose we've
just created a new, empty repository, which by definition has no
commits in it: it's *new*, and *empty*. How many branch names can we
have in this new, empty repository? We've just claimed that a branch
name must identify some specific commit, and we have no commits, so
the answer is: none. We cannot have any branch names at all.
But -- here's the other paradox -- whenever we make a *new* commit,
it's to be *added on to the current branch*. But we have an empty
repository, which cannot have any branch names, so how do we know what
the "current branch" even *is*?
** Unborn Branch is the better term **
Now that we understand the basic problem -- that a new repository
can't have any branches, but that we want Git to *create* a branch
when we make that very first commit -- we can see what an "orphan" or
"unborn" branch is all about. It papers over our chicken-or-egg
problem. We simply save the *name we want Git to create* somewhere,
then we make a new commit as usual. When, eventually, we do make that
commit, Git says: "OK, I should add this new commit to the current
branch br1", or whatever the name is. Git then creates the new commit
*and* creates the branch name, all as one big operation. Now the
branch exists: it's born.
When we have a normal (not-unborn) branch and create a new commit, Git
creates the new commit as usual and then -- here's the unique property
of *branch names* that makes them so special -- *updates* the branch
name to hold the new commit's new hash ID. Git also makes sure that
the new commit we just made links back to the commit that *was* the
tip commit of the branch, just a moment ago. So this is how branches
"grow" as you make commits. The *name* holds only the *last* commit
hash ID. Each commit holds the *previous* hash ID, so that Git can
start at the end of a branch and work backwards. The previous, or
parent, commit, has its own parent, which has another parent, all the
way back to the beginning of time.
This is also where the dual meaning of "branch" clears up somewhat: a
branch is both the tip commit *and* the whole-chain-of-commits,
starting at the tip and working backwards. How do we know which
meaning someone means? Sometimes it's clear from context. Sometimes
it's not clear. Sometimes whoever used the word isn't even aware of
the issue!
** The `--orphan` options **
That weird problematic state for a *new* repository, where no branches
can exist, yet you want to be "on" the branch you're going to create,
only exists as a problem for a new and empty repository. But given
that Git has to solve that problem, Git can let you enter that weird
state any time. That's what `--orphan` was originally invented for:
to go back into that state even if you have some commits.
That is, `git checkout --orphan` meant: make the current branch name
be an unborn branch, the way it is in a new and totally-empty
repository. Then when I make my next commit, that will create a new
commit that has no parent commit. Whether (and when and how) this is
actually useful is another question entirely, as is the reason for
switch and checkout behaving differently in terms of how they treat
the index and working tree. But this is the heart of the option: it
means "go into the unborn branch state".
(Side note: there are other ways to solve the "new repository"
problem, and there are other ways to define "branch". Other version
control systems sometimes use other ways. Git's rather peculiar
definition of branch was rare, perhaps even unique, in the early days
of Git.)
Chris
^ permalink raw reply
* [commit-graph] v2.43.0 segfault with fetch.writeCommitGraph enabled when fetch
From: ryenus @ 2023-11-22 4:05 UTC (permalink / raw)
To: Git mailing list
The issue appeared after updating to git v2.43.0, now git fetch would cause
segmentation fault when commit graph is enabled. Though I only observed this
issue in a repo with two submodules, regardless of whether the submodules are
checked out or not. Meanwhile most other repos without submodules worked fine.
> 15:57:10.660377 run-command.c:726 child_start[2] git maintenance run --auto --no-quiet
> 15:57:10.665870 common-main.c:55 version 2.43.0
> 15:57:10.666265 common-main.c:56 start /opt/homebrew/opt/git/libexec/git-core/git maintenance run --auto --no-quiet
> 15:57:10.666469 repository.c:143 worktree /path/to/repo/sub/module2
> 15:57:10.666649 git.c:464 cmd_name maintenance (_run_dashed_/_run_git_alias_/pull/fetch/fetch/maintenance)
> 15:57:10.668232 git.c:723 exit elapsed:0.003405 code:0
> 15:57:10.668241 trace2/tr2_tgt_normal.c:127 atexit elapsed:0.003415 code:0
> 15:57:10.668611 run-command.c:979 child_exit[2] pid:46018 code:0 elapsed:0.008227
> 15:57:10.668635 git.c:723 exit elapsed:1.837179 code:0
> 15:57:10.668639 trace2/tr2_tgt_normal.c:127 atexit elapsed:1.837182 code:0
> 15:57:10.669007 run-command.c:979 child_exit[3] pid:46006 code:0 elapsed:1.843739
> 15:57:10.671522 usage.c:80 error fetch died of signal 11
> error: fetch died of signal 11
> 15:57:10.671645 run-command.c:979 child_exit[1] pid:45980 code:139 elapsed:5.292927
> 15:57:10.671658 git.c:723 exit elapsed:5.337363 code:1
> 15:57:10.671663 trace2/tr2_tgt_normal.c:127 atexit elapsed:5.337368 code:1
> 15:57:10.672048 run-command.c:979 child_exit[1] pid:45978 code:1 elapsed:5.345050
> 15:57:10.672099 git.c:819 exit elapsed:5.355644 code:1
> 15:57:10.672105 trace2/tr2_tgt_normal.c:127 atexit elapsed:5.355649 code:1
Subsequent `git fetch` would then fail due to the left over lock file:
> 15:57:19.059375 run-command.c:726 child_start[2] git maintenance run --auto --no-quiet
> 15:57:19.065689 common-main.c:55 version 2.43.0
> 15:57:19.066027 common-main.c:56 start /opt/homebrew/opt/git/libexec/git-core/git maintenance run --auto --no-quiet
> 15:57:19.066206 repository.c:143 worktree /path/to/repo/sub/module2
> 15:57:19.066387 git.c:464 cmd_name maintenance (_run_dashed_/_run_git_alias_/pull/fetch/fetch/maintenance)
> 15:57:19.067888 git.c:723 exit elapsed:0.003122 code:0
> 15:57:19.067896 trace2/tr2_tgt_normal.c:127 atexit elapsed:0.003131 code:0
> 15:57:19.068239 run-command.c:979 child_exit[2] pid:46076 code:0 elapsed:0.008852
> 15:57:19.068276 git.c:723 exit elapsed:1.661854 code:0
> 15:57:19.068281 trace2/tr2_tgt_normal.c:127 atexit elapsed:1.661858 code:0
> 15:57:19.068714 run-command.c:979 child_exit[3] pid:46065 code:0 elapsed:1.667321
> 15:57:19.069327 usage.c:61 error Unable to create '/path/to/repo/.git/objects/info/commit-graphs/commit-graph-chain.lock': File exists.
>
> Another git process seems to be running in this repository, e.g.
> an editor opened by 'git commit'. Please make sure all processes
> are terminated then try again. If it still fails, a git process
> may have crashed in this repository earlier:
> remove the file manually to continue.
> fatal: Unable to create '/path/to/repo/.git/objects/info/commit-graphs/commit-graph-chain.lock': File exists.
As a workaround, I disabled fetch.writeCommitGraph to get fetch work again.
^ permalink raw reply
* Re: [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
From: Junio C Hamano @ 2023-11-22 7:54 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig)
Cc: git, Shourya Shukla, Ævar Arnfjörð Bjarmason,
Denton Liu
In-Reply-To: <20231121203413.176414-1-heftig@archlinux.org>
"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:
> Notes:
> v2 changes:
> - fixed code style
> - replaced potentially unsafe use of `sub->path` with `path`
Hasn't the previous iteration of this topic long been merged to not
just 'next' but to 'master' and appears in a released version of Git?
We are all human, so mistakes are inevitable, but if we discover a
mistake that needs fixing after a topic hits 'next', we take it as a
sign that the particular mistake was easy to make and hard to spot,
and the fix for it deserves its own explanation. Please make an
incremental update on top of what has already been merged with a
good explanation (explain why sub->path is "potentially unsafe" and
why using path is better, for example).
Thanks.
^ permalink raw reply
* [PATCH] git-p4: fix fast import when empty commit is first
From: Alisha Kim via GitGitGadget @ 2023-11-22 7:56 UTC (permalink / raw)
To: git; +Cc: Alisha Kim, Alisha Kim
From: Alisha Kim <pril@pril.cc>
When executing p4 sync by specifying an excluded path, an empty commit
will be created if there is only a change in the excluded path in
revision.
If git-p4.keepEmptyCommits is turned off and an empty commit is the
first, fast-import will fail.
Signed-off-by: Alisha Kim <pril@pril.cc>
---
git-p4: fix fast import when empty commit is first
When executing p4 sync by specifying an excluded path, an empty commit
will be created if there is only a change in the excluded path in
revision. If git-p4.keepEmptyCommits is turned off and an empty commit
is the first, fast-import will fail.
The error log is as follows Ignoring revision 14035 as it would produce
an empty commit. fast-import failed: warning: Not updating
refs/heads/p4/master (new tip new commit hash does not contain parent
commit hash) fast-import statistics: ...
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1609%2Fdaebo01%2Fgit-p4-pr-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1609/daebo01/git-p4-pr-v1
Pull-Request: https://github.com/git/git/pull/1609
git-p4.py | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index 0eb3bb4c47d..a61200e29e4 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3466,7 +3466,7 @@ class P4Sync(Command, P4UserMap):
if not files and not allow_empty:
print('Ignoring revision {0} as it would produce an empty commit.'
.format(details['change']))
- return
+ return False
self.gitStream.write("commit %s\n" % branch)
self.gitStream.write("mark :%s\n" % details["change"])
@@ -3533,6 +3533,8 @@ class P4Sync(Command, P4UserMap):
print("Tag %s does not match with change %s: file count is different."
% (labelDetails["label"], change))
+ return True
+
def getLabels(self):
"""Build a dictionary of changelists and labels, for "detect-labels"
option.
@@ -3876,10 +3878,12 @@ class P4Sync(Command, P4UserMap):
self.commit(description, filesForCommit, branch, parent)
else:
files = self.extractFilesFromCommit(description)
- self.commit(description, files, self.branch,
+ isCommitted = self.commit(description, files, self.branch,
self.initialParent)
# only needed once, to connect to the previous commit
- self.initialParent = ""
+ if isCommitted:
+ self.initialParent = ""
+
except IOError:
print(self.gitError.read())
sys.exit(1)
base-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
From: Jan Alexander Steffens (heftig) @ 2023-11-22 9:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Shourya Shukla, Ævar Arnfjörð Bjarmason,
Denton Liu
In-Reply-To: <xmqq4jhedxza.fsf@gitster.g>
On Wed, 2023-11-22 at 16:54 +0900, Junio C Hamano wrote:
> "Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:
>
> > Notes:
> > v2 changes:
> > - fixed code style
> > - replaced potentially unsafe use of `sub->path` with
> > `path`
>
> Hasn't the previous iteration of this topic long been merged to not
> just 'next' but to 'master' and appears in a released version of Git?
>
> We are all human, so mistakes are inevitable, but if we discover a
> mistake that needs fixing after a topic hits 'next', we take it as a
> sign that the particular mistake was easy to make and hard to spot,
> and the fix for it deserves its own explanation. Please make an
> incremental update on top of what has already been merged with a
> good explanation (explain why sub->path is "potentially unsafe" and
> why using path is better, for example).
>
> Thanks.
So it has. I'm sorry. I was only keeping track of the 'maint' branch.
^ permalink raw reply
* Thanks
From: Tatyana Polunin @ 2023-11-22 18:43 UTC (permalink / raw)
To: git
Thanks!
^ permalink raw reply
* git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox
From: Willem Verstraeten @ 2023-11-22 19:08 UTC (permalink / raw)
To: git
# What did you do before the bug happened? (Steps to reproduce your issue)
Clone a repo
Create an additional worktree for that clone
Use `git checkout -B branch-of-primary-clone ...` to checkout the
branch that is already checked out in the primary clone
For example, with the pathfinder repo on GitHub:
git clone https://github.com/servo/pathfinder.git primary
cd primary
git worktree add -b metal ../secondary origin/metal
cd ../secondary
git checkout -b main #reports a fatal error, as expected
git checkout -f main origin/main #also reports a fatal error, as expected
git checkout -B main origin/main # ----> this succeeds, which is
unexpected <----
# What did you expect to happen? (Expected behavior)
I expected a fatal error stating that the branch could not be checked
out since it was already checked out in my primary worktree
In `git checkout --help`, it is documented that `git checkout -B` is
the atomic equivalent of `git branch -f <branch> <commit> ; git
checkout <branch>` :
> If -B is given, <new-branch> is created if it doesn’t exist; otherwise, it is reset. This is the transactional equivalent of
>
> $ git branch -f <branch> [<start-point>]
> $ git checkout <branch>
# What happened instead? (Actual behavior)
The branch was checked out in the secondary worktree. If I then work
and make commits in this secondary worktree, the status of my primary
worktree gets clobbered as well.
What's different between what you expected and what actually happened?
The checkout in the secondary worktree is allowed, but it shouldn't be
[System Info]
git version:
git version 2.41.0
cpu: arm64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Darwin 22.6.0 Darwin Kernel Version 22.6.0: Wed Jul 5 22:21:53
PDT 2023; root:xnu-8796.141.3~6/RELEASE_ARM64_T6020 arm64
compiler info: clang: 14.0.3 (clang-1403.0.22.14.1)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh
[Enabled Hooks]
^ permalink raw reply
* [PATCH 0/4] Redact unsafe URLs in the Trace2 output
From: Johannes Schindelin via GitGitGadget @ 2023-11-22 19:18 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
The Trace2 output can contain secrets when a user issues a Git command with
sensitive information in the command-line. A typical (if highly discouraged)
example is: git clone https://user:password@host.com/.
With this PR, the Trace2 output redacts passwords in such URLs by default.
This series also includes a commit to temporarily disable leak checking on
t0210,t0211 because the tests uncover other unrelated bugs in Git.
These patches were integrated into Microsoft's fork of Git, as
https://github.com/microsoft/git/pull/616, and have been cooking there ever
since.
Jeff Hostetler (3):
trace2: fix signature of trace2_def_param() macro
t0211: test URL redacting in PERF format
t0212: test URL redacting in EVENT format
Johannes Schindelin (1):
trace2: redact passwords from https:// URLs by default
t/helper/test-trace2.c | 55 ++++++++++++++++++
t/t0210-trace2-normal.sh | 20 ++++++-
t/t0211-trace2-perf.sh | 21 ++++++-
t/t0212-trace2-event.sh | 40 +++++++++++++
trace2.c | 120 ++++++++++++++++++++++++++++++++++++++-
trace2.h | 4 +-
6 files changed, 253 insertions(+), 7 deletions(-)
base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1616%2Fdscho%2Ftrace2-redact-credentials-in-https-urls-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1616/dscho/trace2-redact-credentials-in-https-urls-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1616
--
gitgitgadget
^ permalink raw reply
* [PATCH 1/4] trace2: fix signature of trace2_def_param() macro
From: Jeff Hostetler via GitGitGadget @ 2023-11-22 19:18 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff Hostetler
In-Reply-To: <pull.1616.git.1700680717.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Add `struct key_value_info` argument to `trace2_def_param()`.
In dc90208497 (trace2: plumb config kvi, 2023-06-28) a `kvi`
argument was added to `trace2_def_param_fl()` but the macro
was not up updated. Let's fix that.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
trace2.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/trace2.h b/trace2.h
index 40d8c2e02a5..1f0669bbd2d 100644
--- a/trace2.h
+++ b/trace2.h
@@ -337,8 +337,8 @@ struct key_value_info;
void trace2_def_param_fl(const char *file, int line, const char *param,
const char *value, const struct key_value_info *kvi);
-#define trace2_def_param(param, value) \
- trace2_def_param_fl(__FILE__, __LINE__, (param), (value))
+#define trace2_def_param(param, value, kvi) \
+ trace2_def_param_fl(__FILE__, __LINE__, (param), (value), (kvi))
/*
* Tell trace2 about a newly instantiated repo object and assign
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/4] trace2: redact passwords from https:// URLs by default
From: Johannes Schindelin via GitGitGadget @ 2023-11-22 19:18 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1616.git.1700680717.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
It is an unsafe practice to call something like
git clone https://user:password@example.com/
This not only risks leaking the password "over the shoulder" or into the
readline history of the current Unix shell, it also gets logged via
Trace2 if enabled.
Let's at least avoid logging such secrets via Trace2, much like we avoid
logging secrets in `http.c`. Much like the code in `http.c` is guarded
via `GIT_TRACE_REDACT` (defaulting to `true`), we guard the new code via
`GIT_TRACE2_REDACT` (also defaulting to `true`).
The new tests added in this commit uncover leaks in `builtin/clone.c`
and `remote.c`. Therefore we need to turn off
`TEST_PASSES_SANITIZE_LEAK`. The reasons:
- We observed that `the_repository->remote_status` is not released
properly.
- We are using `url...insteadOf` and that runs into a code path where an
allocated URL is replaced with another URL, and the original URL is
never released.
- `remote_states` contains plenty of `struct remote`s whose refspecs
seem to be usually allocated by never released.
More investigation is needed here to identify the exact cause and
proper fixes for these leaks/bugs.
Co-authored-by: Jeff Hostetler <jeffhostetler@github.com>
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t0210-trace2-normal.sh | 20 ++++++-
trace2.c | 120 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 136 insertions(+), 4 deletions(-)
diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
index 80e76a4695e..c312657a12c 100755
--- a/t/t0210-trace2-normal.sh
+++ b/t/t0210-trace2-normal.sh
@@ -2,7 +2,7 @@
test_description='test trace2 facility (normal target)'
-TEST_PASSES_SANITIZE_LEAK=true
+TEST_PASSES_SANITIZE_LEAK=false
. ./test-lib.sh
# Turn off any inherited trace2 settings for this test.
@@ -283,4 +283,22 @@ test_expect_success 'using global config with include' '
test_cmp expect actual
'
+test_expect_success 'unsafe URLs are redacted by default' '
+ test_when_finished \
+ "rm -r trace.normal unredacted.normal clone clone2" &&
+
+ test_config_global \
+ "url.$(pwd).insteadOf" https://user:pwd@example.com/ &&
+ test_config_global trace2.configParams "core.*,remote.*.url" &&
+
+ GIT_TRACE2="$(pwd)/trace.normal" \
+ git clone https://user:pwd@example.com/ clone &&
+ ! grep user:pwd trace.normal &&
+
+ GIT_TRACE2_REDACT=0 GIT_TRACE2="$(pwd)/unredacted.normal" \
+ git clone https://user:pwd@example.com/ clone2 &&
+ grep "start .* clone https://user:pwd@example.com" unredacted.normal &&
+ grep "remote.origin.url=https://user:pwd@example.com" unredacted.normal
+'
+
test_done
diff --git a/trace2.c b/trace2.c
index 6dc74dff4c7..87d9a3a0361 100644
--- a/trace2.c
+++ b/trace2.c
@@ -20,6 +20,7 @@
#include "trace2/tr2_tmr.h"
static int trace2_enabled;
+static int trace2_redact = 1;
static int tr2_next_child_id; /* modify under lock */
static int tr2_next_exec_id; /* modify under lock */
@@ -227,6 +228,8 @@ void trace2_initialize_fl(const char *file, int line)
if (!tr2_tgt_want_builtins())
return;
trace2_enabled = 1;
+ if (!git_env_bool("GIT_TRACE2_REDACT", 1))
+ trace2_redact = 0;
tr2_sid_get();
@@ -247,12 +250,93 @@ int trace2_is_enabled(void)
return trace2_enabled;
}
+/*
+ * Redacts an argument, i.e. ensures that no password in
+ * https://user:password@host/-style URLs is logged.
+ *
+ * Returns the original if nothing needed to be redacted.
+ * Returns a pointer that needs to be `free()`d otherwise.
+ */
+static const char *redact_arg(const char *arg)
+{
+ const char *p, *colon;
+ size_t at;
+
+ if (!trace2_redact ||
+ (!skip_prefix(arg, "https://", &p) &&
+ !skip_prefix(arg, "http://", &p)))
+ return arg;
+
+ at = strcspn(p, "@/");
+ if (p[at] != '@')
+ return arg;
+
+ colon = memchr(p, ':', at);
+ if (!colon)
+ return arg;
+
+ return xstrfmt("%.*s:<REDACTED>%s", (int)(colon - arg), arg, p + at);
+}
+
+/*
+ * Redacts arguments in an argument list.
+ *
+ * Returns the original if nothing needed to be redacted.
+ * Otherwise, returns a new array that needs to be released
+ * via `free_redacted_argv()`.
+ */
+static const char **redact_argv(const char **argv)
+{
+ int i, j;
+ const char *redacted = NULL;
+ const char **ret;
+
+ if (!trace2_redact)
+ return argv;
+
+ for (i = 0; argv[i]; i++)
+ if ((redacted = redact_arg(argv[i])) != argv[i])
+ break;
+
+ if (!argv[i])
+ return argv;
+
+ for (j = 0; argv[j]; j++)
+ ; /* keep counting */
+
+ ALLOC_ARRAY(ret, j + 1);
+ ret[j] = NULL;
+
+ for (j = 0; j < i; j++)
+ ret[j] = argv[j];
+ ret[i] = redacted;
+ for (++i; argv[i]; i++) {
+ redacted = redact_arg(argv[i]);
+ ret[i] = redacted ? redacted : argv[i];
+ }
+
+ return ret;
+}
+
+static void free_redacted_argv(const char **redacted, const char **argv)
+{
+ int i;
+
+ if (redacted != argv) {
+ for (i = 0; argv[i]; i++)
+ if (redacted[i] != argv[i])
+ free((void *)redacted[i]);
+ free((void *)redacted);
+ }
+}
+
void trace2_cmd_start_fl(const char *file, int line, const char **argv)
{
struct tr2_tgt *tgt_j;
int j;
uint64_t us_now;
uint64_t us_elapsed_absolute;
+ const char **redacted;
if (!trace2_enabled)
return;
@@ -260,10 +344,14 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)
us_now = getnanotime() / 1000;
us_elapsed_absolute = tr2tls_absolute_elapsed(us_now);
+ redacted = redact_argv(argv);
+
for_each_wanted_builtin (j, tgt_j)
if (tgt_j->pfn_start_fl)
tgt_j->pfn_start_fl(file, line, us_elapsed_absolute,
- argv);
+ redacted);
+
+ free_redacted_argv(redacted, argv);
}
void trace2_cmd_exit_fl(const char *file, int line, int code)
@@ -409,6 +497,7 @@ void trace2_child_start_fl(const char *file, int line,
int j;
uint64_t us_now;
uint64_t us_elapsed_absolute;
+ const char **orig_argv = cmd->args.v;
if (!trace2_enabled)
return;
@@ -419,10 +508,24 @@ void trace2_child_start_fl(const char *file, int line,
cmd->trace2_child_id = tr2tls_locked_increment(&tr2_next_child_id);
cmd->trace2_child_us_start = us_now;
+ /*
+ * The `pfn_child_start_fl` API takes a `struct child_process`
+ * rather than a simple `argv` for the child because some
+ * targets make use of the additional context bits/values. So
+ * temporarily replace the original argv (inside the `strvec`)
+ * with a possibly redacted version.
+ */
+ cmd->args.v = redact_argv(orig_argv);
+
for_each_wanted_builtin (j, tgt_j)
if (tgt_j->pfn_child_start_fl)
tgt_j->pfn_child_start_fl(file, line,
us_elapsed_absolute, cmd);
+
+ if (cmd->args.v != orig_argv) {
+ free_redacted_argv(cmd->args.v, orig_argv);
+ cmd->args.v = orig_argv;
+ }
}
void trace2_child_exit_fl(const char *file, int line, struct child_process *cmd,
@@ -493,6 +596,7 @@ int trace2_exec_fl(const char *file, int line, const char *exe,
int exec_id;
uint64_t us_now;
uint64_t us_elapsed_absolute;
+ const char **redacted;
if (!trace2_enabled)
return -1;
@@ -502,10 +606,14 @@ int trace2_exec_fl(const char *file, int line, const char *exe,
exec_id = tr2tls_locked_increment(&tr2_next_exec_id);
+ redacted = redact_argv(argv);
+
for_each_wanted_builtin (j, tgt_j)
if (tgt_j->pfn_exec_fl)
tgt_j->pfn_exec_fl(file, line, us_elapsed_absolute,
- exec_id, exe, argv);
+ exec_id, exe, redacted);
+
+ free_redacted_argv(redacted, argv);
return exec_id;
}
@@ -637,13 +745,19 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
{
struct tr2_tgt *tgt_j;
int j;
+ const char *redacted;
if (!trace2_enabled)
return;
+ redacted = redact_arg(value);
+
for_each_wanted_builtin (j, tgt_j)
if (tgt_j->pfn_param_fl)
- tgt_j->pfn_param_fl(file, line, param, value, kvi);
+ tgt_j->pfn_param_fl(file, line, param, redacted, kvi);
+
+ if (redacted != value)
+ free((void *)redacted);
}
void trace2_def_repo_fl(const char *file, int line, struct repository *repo)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/4] t0211: test URL redacting in PERF format
From: Jeff Hostetler via GitGitGadget @ 2023-11-22 19:18 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff Hostetler
In-Reply-To: <pull.1616.git.1700680717.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
This transmogrifies the test case that was just added to t0210, to also
cover the `GIT_TRACE2_PERF` backend.
Just like t0211, we now have to toggle the `TEST_PASSES_SANITIZE_LEAK`
annotation.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t0211-trace2-perf.sh | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index cfba6861322..290b6eaaab1 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -2,7 +2,7 @@
test_description='test trace2 facility (perf target)'
-TEST_PASSES_SANITIZE_LEAK=true
+TEST_PASSES_SANITIZE_LEAK=false
. ./test-lib.sh
# Turn off any inherited trace2 settings for this test.
@@ -268,4 +268,23 @@ test_expect_success PTHREADS 'global counter test/test2' '
have_counter_event "main" "counter" "test" "test2" 60 actual
'
+test_expect_success 'unsafe URLs are redacted by default' '
+ test_when_finished \
+ "rm -r actual trace.perf unredacted.perf clone clone2" &&
+
+ test_config_global \
+ "url.$(pwd).insteadOf" https://user:pwd@example.com/ &&
+ test_config_global trace2.configParams "core.*,remote.*.url" &&
+
+ GIT_TRACE2_PERF="$(pwd)/trace.perf" \
+ git clone https://user:pwd@example.com/ clone &&
+ ! grep user:pwd trace.perf &&
+
+ GIT_TRACE2_REDACT=0 GIT_TRACE2_PERF="$(pwd)/unredacted.perf" \
+ git clone https://user:pwd@example.com/ clone2 &&
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <unredacted.perf >actual &&
+ grep "d0|main|start|.* clone https://user:pwd@example.com" actual &&
+ grep "d0|main|def_param|.*|remote.origin.url:https://user:pwd@example.com" actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 4/4] t0212: test URL redacting in EVENT format
From: Jeff Hostetler via GitGitGadget @ 2023-11-22 19:18 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff Hostetler
In-Reply-To: <pull.1616.git.1700680717.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
In the added tests cases, skip testing the `GIT_TRACE2_REDACT=0` case
because we would need to exactly model the full JSON event stream like
we did in the preceding basic tests and I do not think it is worth it.
Furthermore, the Trace2 routines print the same content in normal, perf,
or event format, and in t0210 and t0211 we already tested the basic
functionality, so no need to repeat it here.
In this test, we use the test-helper to unit test each of the event
messages where URLs can appear and confirm that they are redacted in
each event.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/helper/test-trace2.c | 55 +++++++++++++++++++++++++++++++++++++++++
t/t0212-trace2-event.sh | 40 ++++++++++++++++++++++++++++++
2 files changed, 95 insertions(+)
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index d5ca0046c89..1adac29a575 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -412,6 +412,56 @@ static int ut_201counter(int argc, const char **argv)
return 0;
}
+static int ut_300redact_start(int argc, const char **argv)
+{
+ if (!argc)
+ die("expect <argv...>");
+
+ trace2_cmd_start(argv);
+
+ return 0;
+}
+
+static int ut_301redact_child_start(int argc, const char **argv)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ int k;
+
+ if (!argc)
+ die("expect <argv...>");
+
+ for (k = 0; argv[k]; k++)
+ strvec_push(&cmd.args, argv[k]);
+
+ trace2_child_start(&cmd);
+
+ strvec_clear(&cmd.args);
+
+ return 0;
+}
+
+static int ut_302redact_exec(int argc, const char **argv)
+{
+ if (!argc)
+ die("expect <exe> <argv...>");
+
+ trace2_exec(argv[0], &argv[1]);
+
+ return 0;
+}
+
+static int ut_303redact_def_param(int argc, const char **argv)
+{
+ struct key_value_info kvi = KVI_INIT;
+
+ if (argc < 2)
+ die("expect <key> <value>");
+
+ trace2_def_param(argv[0], argv[1], &kvi);
+
+ return 0;
+}
+
/*
* Usage:
* test-tool trace2 <ut_name_1> <ut_usage_1>
@@ -438,6 +488,11 @@ static struct unit_test ut_table[] = {
{ ut_200counter, "200counter", "<v1> [<v2> [<v3> [...]]]" },
{ ut_201counter, "201counter", "<v1> <v2> <threads>" },
+
+ { ut_300redact_start, "300redact_start", "<argv...>" },
+ { ut_301redact_child_start, "301redact_child_start", "<argv...>" },
+ { ut_302redact_exec, "302redact_exec", "<exe> <argv...>" },
+ { ut_303redact_def_param, "303redact_def_param", "<key> <value>" },
};
/* clang-format on */
diff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh
index 6d3374ff773..147643d5826 100755
--- a/t/t0212-trace2-event.sh
+++ b/t/t0212-trace2-event.sh
@@ -323,4 +323,44 @@ test_expect_success 'discard traces when there are too many files' '
head -n2 trace_target_dir/git-trace2-discard | tail -n1 | grep \"event\":\"too_many_files\"
'
+# In the following "...redact..." tests, skip testing the GIT_TRACE2_REDACT=0
+# case because we would need to exactly model the full JSON event stream like
+# we did in the basic tests above and I do not think it is worth it.
+
+test_expect_success 'unsafe URLs are redacted by default in cmd_start events' '
+ test_when_finished \
+ "rm -r trace.event" &&
+
+ GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+ test-tool trace2 300redact_start git clone https://user:pwd@example.com/ clone2 &&
+ ! grep user:pwd trace.event
+'
+
+test_expect_success 'unsafe URLs are redacted by default in child_start events' '
+ test_when_finished \
+ "rm -r trace.event" &&
+
+ GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+ test-tool trace2 301redact_child_start git clone https://user:pwd@example.com/ clone2 &&
+ ! grep user:pwd trace.event
+'
+
+test_expect_success 'unsafe URLs are redacted by default in exec events' '
+ test_when_finished \
+ "rm -r trace.event" &&
+
+ GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+ test-tool trace2 302redact_exec git clone https://user:pwd@example.com/ clone2 &&
+ ! grep user:pwd trace.event
+'
+
+test_expect_success 'unsafe URLs are redacted by default in def_param events' '
+ test_when_finished \
+ "rm -r trace.event" &&
+
+ GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+ test-tool trace2 303redact_def_param url https://user:pwd@example.com/ &&
+ ! grep user:pwd trace.event
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* Re: git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox
From: Junio C Hamano @ 2023-11-23 1:28 UTC (permalink / raw)
To: Willem Verstraeten; +Cc: git
In-Reply-To: <CAGX9RpFMCVLQV7RbK2u9AabusvkZD+RZNv_UD=R00cSUrjutBg@mail.gmail.com>
Willem Verstraeten <willem.verstraeten@gmail.com> writes:
> git checkout -b main #reports a fatal error, as expected
This is expected because "main" already exists, not because "main"
is checked out elsewhere.
> git checkout -f main origin/main #also reports a fatal error, as expected
This is expected because origin/main is taken as pathspec, and it is
a request to checkout the paths that match the pathspec out of the
named tree-ish (i.e., "main"), even when these paths have local
changes, but you do not have paths that match "origin/main". The
failure is not because "main" is checked out elsewhere.
A slight variant of the command
git checkout -f -b main origin/main
still fails for the same reason as the first of your examples above.
It is a tangent, but I suspect this failure may be a bit unexpected.
In this example, "-f"orce could be overriding the usual failure from
"-b" to switch to a branch that already exists, but that is what
"-B" does, and "-f -b" does not work as a synonym for "-B".
In any case, these example you marked "fail as expected" do fail as
expected, but they fail for reasons that have nothing to do with the
protection of branches that are used in other worktrees.
> git checkout -B main origin/main # ----> this succeeds, which is
> unexpected <----
I agree this may be undesirable.
But it makes sort of sense, because "-B" is a forced form of "-b"
(i.e., it tells git: even when "-b" would fail, take necessary
measures to make it work), and we can view that it is part of
"forcing" to override the protection over branches that are used
elsewhere.
I guess we could change the behaviour so that
git checkout -B <branch> [<start-point>]
fails when <branch> is an existing branch that is in use in another
worktree, and allow "-f" to be used to override the safety, i.e.,
git checkout -f -B <branch> [<start-point>]
would allow the <branch> to be repointed to <start-point> (or HEAD)
even when it is used elsewhere.
Thoughts, whether they agree or disagree with what I just said, by
other experienced contributors are very much welcome, before I can
say "patches welcome" ;-).
Willem, thanks for raising the issue.
^ 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