Re: [PATCH v3 1/1] ci(osx): use new location of the `perforce` cask
"Johannes Schindelin via GitGitGadget"
writes:
> From: Johannes Schindelin
>
> The Azure Pipelines builds are failing for macOS due to a change in the
> location of the perforce cask. The command outputs the following error:
>
> + brew install caskroom/cask/perforce
> Error: caskroom/cask was moved. Tap homebrew/cask-cask instead.
> ...
> In any case, as the error message at the top of this commit message
> shows, 'brew install caskroom/cask/perforce' has stopped working
> recently, but 'brew cask install perforce' still does, so let's use
> that.
It appears that OSX jobs at Travis are getting hit by this issue.
Here is what our failed build ends with, for example:
+brew install caskroom/cask/perforce
Error: caskroom/cask was moved. Tap homebrew/cask-cask instead.
cf. https://travis-ci.org/git/git/jobs/601697815
Today's 'pu' has this topic queued, and it seems to help even the
builds at Travis ('pu' seems to fail the test for totally different
reason, though):
+brew link gcc@8
Error: No such keg: /usr/local/Cellar/gcc@8
cf. https://travis-ci.org/git/git/jobs/601697903
Thanks.
[PATCH] doc: am --show-current-patch gives an entire e-mail message
The existing wording gives an impression that it only gives the contents of the $GIT_DIR/rebase-apply/patch file, i.e. the patch proper, but the option actually emits the entire e-mail message being processed (iow, one of the output files from "git mailsplit"). Signed-off-by: Junio C Hamano --- Documentation/git-am.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index fc3b993c33..fc5750b3b8 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -177,7 +177,7 @@ default. You can use `--no-utf8` to override this. untouched. --show-current-patch:: - Show the patch being applied when "git am" is stopped because + Show the entire e-mail message "git am" has stopped at, because of conflicts. DISCUSSION -- 2.24.0-rc0-248-ga4c21000fb
Re: [Git Developer Blog] [PATCH] post: a tour of git's object types
Emily Shaffer writes:
> +# Commit
> +
> +This is the one we're all familiar with - commits are those things we write
> at
> +1am, angry at a pesky bug, and label with something like "really fix it this
> +time", right?
> +
> +A commit references exactly one tree. That's the root directory of your
> project.
> +It also references zero or more other commits - and this is where we diverge
> +from the filesystem parallel, because the other commits it references are its
> +parent(s), each of which has its own copy of the project at that commit's
> point
> +in time. (Commits with more than one parent are merge commits; otherwise,
> your
> +commit only has the one parent.)
I do not see a need for (parentheses) around the last sentence, but if
you must, then s/in time. /in time/; and s/one parent.)/one parent)./
would be better.
> +Commits represent a specific state of the repository which someone thought
> was
> +worth saving - a new feature, or a small step of progress which you don't
> want
> +to lose as you're hacking your way through that homework assignment. Each
> commit
> +points to a complete image of the repository - but that's not as bad as it
> +sounds, as we'll see when we try it out.
That's half of a commit (i.e. the tree that represents the specific
state).
The other half is that a commit (at least in a serious-enough
project) is a statement by its author: I considered all the parent
commits, and declare that the tree this commit records suits my goal
better than any and all of these parents' trees.
That is what makes 3-way merge work correctly at the philosophical
level. As long as the project participants share the same goal and
trust each other, when one creates a merge, one trusts what the
others built in the side branch (i.e. each of the commits they made
got us closer to our collective goals) and take their changes to
where one did not touch.
Of course, a good description in the log message helps the one
who makes such a merge to see if the workmade on the side branch
moves the tree in the direction that truly fits one's goal.
> +# Tag
> +
> +Tags are a little lackluster after all the exciting types up until now. They
> are
> +essentially a label; they serve as an entry point into the graph and point to
> +either another tag or a commit.
Either say "generally point to", or "point to another object"
(i.e. a tag that points to a tree or a blob is normal---it is just
they do not so frequently appear).
> They're generally used to mark releases, and you
> +can see them pretty easily with `git log --oneline --decorate=full`.
> +
> +# A quick return to an overloaded word
> +
> +"Tree", "worktree", and "working tree" seem to refer to different concepts.
Not just seem to. They do refer to different things. "tree" is a
type of object. "working tree" is a directory hiearchy where you
did "git checkout" to materialize the contents of a tree object
(recursively) and are using to work towards updating the index to
create the next commit. "worktree" is a mechanism that allows you
to have multiple "working tree"s that is backed by the same repository.
They may share the same word "tree". You may want to update this
document to say "tree object" when you mean it---that would help
disambiguating it from other uses of words with "tree" in them.
> ...
> +predictable way. Let's walk through creating a pretty basic repository and
> +examining it with some low-level plumbing commands!
> +
> +# An empty repo
> +
> +For starters, we'll make a new, shiny, totally empty repo.
> +
> +{% highlight shell_session %}
> +$ mkdir demo
> +$ cd demo
> +$ git init
> +{% endhighlight %}
> +
> +We've got nothing. If we try `git log`, we'll be assured that we have no
> +commits, and if we try `git branch -a` we'll see we have no branches, either.
> +So let's make a very simple first commit.
> +
> +# A single commit
> +
> +{% highlight shell_session %}
> +$ echo "abcd" >foo.txt
> +$ git add foo.txt
> +$ git commit -m "first commit"
> +{% endhighlight %}
> +
> +I know this is boring, but bear with me and run `git ls-tree HEAD`.
You may probably want to say that, even though you upfront raised
the expectation of readers that they would hear about plumbing soon,
you haven't so far used any plumbing yet. And stress that ls-tree
is a plumbing, what the readers have been waiting for!
> +{% highlight shell_session %}
> +$ git ls-tree HEAD
> +100644 blob acbe86c7c89586e0912a0a851bacf309c595c308 foo.txt
> +$ git cat-file -p acbe86c7c89586e0912a0a851bacf309c595c308
> +abcd
> +{% endhighlight %}
> +
> +While we're here, we can also take a look at the commit object. Use `git log`
> +to determine your commit's OID, then use `git cat-file -p` to print the
> +contents:
I doubt that "cat-file -p" is helpful to a reader who is learning
the basic object layer. For non-tree types, learning "cat-file -t"
followed by "cat-file " would be more useful to gain proper
understanding (and for trees, as you showed above, ls-tr
Re: [RFC/WIP] range-diff: show old/new blob OIDs in comments
Eric Wong writes: > What Konstantin said about git repos being transient. > It wasn't too much work to recreate those blobs from > scratch since git-apply has done it since 2005. ;-) > We could get around transient repos with automatic mirroring > bots which never deletes or overwrites anything published. > That includes preserving pre-force-push data in case of > force pushes. > >> Instead, we will have to rely on your centralized, non-distributed >> service... > > I'm curious how you came to believe that, since that's the > opposite of what public-inbox has always been intended to be. I think the (mis)perception comes from the fact that the website and the newsfeed you give are both too easy to use and directly attract end users, instead of enticing them to keep their own mirrors for offline use. Thanks for injecting dose of sanity.
Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
SZEDER Gábor writes: >> - b9317d55a3 added two new keys to the trie: 'logs/refs/rewritten' >> and 'logs/refs/worktree', next to the already existing >> 'logs/refs/bisect'. This resulted in a trie node with the path >> 'logs/refs', which didn't exist before, and which doesn't have a > > Oops, I missed the trailing slash, that must be 'logs/refs/'! > >> value attached. A query for 'logs/refs/' finds this node and then >> hits that one callsite of the match function which doesn't check >> for the value's existence, and thus invokes the match function >> with NULL as value. Given that the trie is maintained by hand in common_list[], I wonder if we can mechanically catch errors like the one b9317d55a3 added, by perhaps having a self-test function that a t/helper/ program calls to perform consistency check after the "git" gets built. Thanks.
Re: [PATCH v5 00/17] New sparse-checkout builtin and "cone" mode
"Derrick Stolee via GitGitGadget" writes: > V4 UPDATE: Rebased on latest master to include ew/hashmap and > ds/include-exclude in the base. > > This series makes the sparse-checkout feature more user-friendly. While > there, I also present a way to use a limited set of patterns to gain a > significant performance boost in very large repositories. > ... > Updates in V5: > > * The 'set' subcommand now enables the core.sparseCheckout config setting >(unless the checkout fails). > > > * If the in-process unpack_trees() fails with the new patterns, the >index.lock file is rolled back before the replay of the old >sparse-checkout patterns. > > > * Some documentation fixes, f(d)open->xf(d)open calls, and other nits. >Thanks everyone! Thanks. I quickly scanned the changes between the last round and this one, and all I saw were pure improvements ;-) Not that I claim to have read the previous round very carefully, though. Will replace and queue.
Re: [PATCH v5 13/17] read-tree: show progress by default
Derrick Stolee writes: >> I'm slightly wary of changing the output of plumbing commands >> like this. If a script wants progress output it can already get >> it by passing --verbose. With this change a script that does not >> want that output now has to pass --no-verbose. > > If a script is calling this, then won't stderr not be a terminal window, and > isatty(2) return 0? Unless the script tries to capture the error output and react differently depending on the error message from the plumbing (which is not localized), iow most of the time, standard error stream is left unredirected and likely to be connected to the terminal if the script is driven from a terminal command line. > Or, if the script is run with stderr passing through to > a terminal, then the user would see progress while running the script, which > seems like a side-effect but not one that will cause a broken script. It will show unwanted output to the end users, no? That is the complaint about having to pass --no-verbose, if I understand correctly, if the script does not want to show the progress output.
Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly
SZEDER Gábor writes: >> +char *base = buf->buf + git_dir_len, *base2 = NULL; >> + >> +if (ends_with(base, ".lock")) >> +base = base2 = xstrndup(base, strlen(base) - 5); > > Hm, this adds the magic number 5 and calls strlen(base) twice, because > ends_with() calls strip_suffix(), which calls strlen(). Calling > strip_suffix() directly would allow us to avoid the magic number and > the second strlen(): > > size_t len; > if (strip_suffix(base, ".lock", &len)) > base = base2 = xstrndup(base, len); Makes sense, and is easy to squash in.
Re: [PATCH 0/3] commit: fix advice for empty commits during rebases
"Johannes Schindelin via GitGitGadget"
writes:
> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we
> introduced a helpful message that suggests to run git cherry-pick --skip
> (instead of the previous message that talked about git reset) when a
> cherry-pick failed due to an empty patch.
>
> However, the same message is displayed during a rebase, when the patch
> to-be-committed is empty. In this case, git reset would also have worked,
> but git cherry-pick --skip does not work. This is a regression introduced in
> this cycle.
>
> Let's be more careful here.
>
> Johannes Schindelin (3):
> cherry-pick: add test for `--skip` advice in `git commit`
> sequencer: export the function to get the path of `.git/rebase-merge/`
> commit: give correct advice for empty commit during a rebase
Overall they looked nicely done. The last one may have started its
life as "let's fix advice for empty", but no longer is.
The old code used the sequencer_in_use boolean to tell between two
states ("are we doing a single pick, or a range pick?"), but now we
have another boolean rebase_in_progress, and depending on the shape
of the if statements existing codepaths happen to have, these two
are combined in different ways to deal with new states. E.g. some
places say
if (rebase_in_progress && !sequencer_in_use)
we are doing a rebase;
else
we are doing a cherry-pick;
and some others say
if (sequencer_in_use)
we are doing a multi pick;
else if (rebase_in_progress)
we are doing a rebase;
else
we are doing a single pick;
I wonder if it makes the resulting logic simpler to reason about if
these combination of two variables are rewritten to use a single
variable that enumerates three (or is it four, counting "we are
doing neither a cherry-pick or a rebase"?) possible state.
Other than that, looked sensible. Will queue.
Thanks.
Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase
"Johannes Schindelin via GitGitGadget" writes: > Note: we take pains to handle the situation when a user runs a `git > cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec` > line in an interactive rebase). On the other hand, it is not possible to > run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and > `sequencer/` exist, we still want to advise to use `git cherry-pick > --skip`. Good description of why the code does what it does, that future readers will surely appreciate. Nice.
Re: [BUG] "--show-current-patch" return a mail instead of a patch
Jerome Pouiller writes:
> Hello all,
>
> I try to use "git am" to apply a patch sent using "git send-email". This
> patch does not apply properly. I try to use "git am --show-current-patch"
> to understand the problem. However, since original mail is encoded in quoted-
> printable, data returned by --show-current-patch is not a valid patch.
I agree that --show-current-patch is a misdesigned feature. We'd be
doing a better service to our users if we documented that the patch
and log message are found at .git/rebase-apply/{patch,msg} instead
of trying to hide the path.
Unfortunately, it is likely that those who added that feature have
built their tooling around it to depend on its output being the full
e-mail message "am" was fed (and split by "git mailsplit"). So I do
not think we will be changing the output to the patch file only.
But even then, the documentation can be fixed without any backward
compatibility issues. Perhaps like this?
Documentation/git-am.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 6f6c34b0f4..f63b70325c 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -172,7 +172,7 @@ default. You can use `--no-utf8` to override this.
untouched.
--show-current-patch::
- Show the patch being applied when "git am" is stopped because
+ Show the entire e-mail message "git am" has stopped at, because
of conflicts.
DISCUSSION
Re: [PATCH] t7419: change test_must_fail to ! for grep
Denton Liu writes: > According to t/README, test_must_fail() should only be used to test for > failure in Git commands. Replace the invocations of > `test_must_fail grep` with `! grep`. > > Signed-off-by: Denton Liu > --- > *sigh* Here's another cleanup patch for 'dl/submodule-set-branch'. It's > inspired by Eric Sunshine's comments on the t5520 patchset from earlier. > It's definitely not urgent, though, and can wait for v2.25.0. True, but they are trivially correct and removes the risk of inexperienced developers copying and pasting this bad pattern to other tests that was added during this cycle, so I think it is a good idea to take it now. Thanks for being extra careful.
Re: [PATCH v2 1/1] config: move documentation to config.h
Emily Shaffer writes: >> ... >> +/** >> + * The config API gives callers a way to access Git configuration files >> + * (and files which have the same syntax). See linkgit:git-config[1] for a > > Ah, here's another place where the Asciidoc link isn't going to do > anything anymore. > > Otherwise I didn't still see anything jumping out. When the commit > message is cleaned up I'm ready to add my Reviewed-by line. Thanks. Your review(s) have been quite sensible and helpful.
Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly
Johannes Schindelin writes: > However, I think this is _really_ ugly and intrusive. The opposite of > what my goals were. > > So I think I'll just bite the bullet and use a temporary copy if the > argument ends in `.lock`. Sounds like a quite sensible design decision to me.
Re: [PATCH v5 1/2] format-patch: create leading components of output directory
Bert Wesarg writes: > Please ignore this. Will rebase on 2.24-rc0 and will only include the > test changes. Thanks.
Re: [PATCH v2 0/2] Fix the speed of the CI (Visual Studio) tests
"Johannes Schindelin via GitGitGadget" writes: > Changes since v1: > > * Fixed typo "nore" -> "nor" in the commit message. > > Johannes Schindelin (2): > ci(visual-studio): use strict compile flags, and optimization > ci(visual-studio): actually run the tests in parallel > > azure-pipelines.yml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Thanks. I will take the change to 'master' directly, as cooking in 'next' won't give it any extra exposure when the topic touches only this file and the patch comes from those who do exercise azure CI build before sending it out to the list ;-)
Re: [PATCH] test-progress: fix test failures on big-endian systems
Jeff King writes: > But here's where it gets tricky. In addition to catching any size > mismatches, this will also catch signedness problems. I.e., if we make > OPT_INTEGER() use "intp", then everybody passing in &unsigned_var now > gets a compiler warning. Which maybe is a good thing, I dunno. Hmph, true. I'd agree with back-burnering it for now. Perhaps we'd fix the signedness issue one by one in a preparatory series before converting the value field to a union, if we want to pursue this idea further (in which I am mildly interested, by the way), but it does sound like it should be given lower priority. > So that's where I gave up. Converting between signed and unsigned > variables needs to be done very carefully, as there are often subtle > impacts (e.g., loop terminations). And because we have so many sign > issues already, compiling with "-Wsign-compare", etc, isn't likely to > help. True. Thanks.
Re: [PATCH 1/1] documentation: remove empty doc files
Emily Shaffer writes: > As for the content of this change, I absolutely approve. I've stumbled > across some of these empty docs while looking for answers before and > found it really demoralizing - the community is so interested in > teaching me how to contribute that they've sat on a TODO for 12 years? > :( I even held up api-grep.txt as a (bad) example in a talk I gave this > year. I'm happy to see these files go. I'd approve this move, too, especially if we accompanied deletion with addition (or verification of existence) of necessary docs elsewhere (perhaps in *.h headers).
Re: [PATCH] pretty: Add "%aU"|"%au" option to output author's username
Prarit Bhargava writes:
> Subject: Re: [PATCH] pretty: Add "%aU"|"%au" option to output author's
> username
Downcase "Add" (see "git shortlog --no-merges -100 master" and
mimick the project convention).
> Add a "%aU"|"%au" option that outputs the author's email username.
Even though I personally do not see the use for it, I agree it would
make sense to have an option to show the local part only where the
e-mail address is shown.
I do not know if u/U is a good mnemonic; it hints too strongly that
it may come from GIT_{AUTHOR/COMMITTER}_NAME but that is not what
you are doing---isn't there a letter that better conveys that this
is about RFC 2822 local-part (cf. page 16 ieft.org/rfc/rfc2822.txt)?
> diff --git a/Documentation/pretty-formats.txt
> b/Documentation/pretty-formats.txt
> index b87e2e83e6d0..479a15a8ab12 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -163,6 +163,9 @@ The placeholders are:
> '%ae':: author email
> '%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]
> or linkgit:git-blame[1])
> +'%au':: author username
> +'%aU':: author username (respecting .mailmap, see linkgit:git-shortlog[1]
> + or linkgit:git-blame[1])
> '%ad':: author date (format respects --date= option)
> '%aD':: author date, RFC2822 style
> '%ar':: author date, relative
> diff --git a/pretty.c b/pretty.c
> index b32f0369531c..2a5b93022050 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char
> part,
> strbuf_add(sb, mail, maillen);
> return placeholder_len;
> }
> + if (part == 'u' || part == 'U') { /* username */
> + maillen = strstr(s.mail_begin, "@") - s.mail_begin;
> + strbuf_add(sb, mail, maillen);
> + return placeholder_len;
> + }
I think users get %eu and %eU for free with this change, which should
be documented.
Re: [PATCH v2 1/1] ci(osx): use new location of the `perforce` cask
Johannes Schindelin writes: >> This is already in 'next' X-<; reverting a merge is cheap but I >> prefer to do so when we already have a replacement. > > I force-pushed (see https://github.com/gitgitgadget/git/pull/400), and > once Stolee approves, he will submit v3. This will only change the > commit message, though, as I disagree that hard-coding the URL would be > an improvement: the nice thing about a package management system is that > the user does not need to know the details (or need to know if the > details change, like, ever). If this were meant for the upcoming release, I would rather see us copy a butt-ugly-but-known-working procedure if we have one this close to -rc1. If the hard-coded URL ever changes, the procedure we would be copying from would be broken anyway. But I agree 100% that we should take a conceptually cleaner approach for the longer term. Let's replace the original one with this and cook in 'next'---it would be ideal if the ugly-but-know-working one be updated to match in the meantime, but if it is bypassing package management for a reason (the upstream just publishes the URL to download from without packaging it properly, for example?), that would not be possible, and it is OK if that is the case. Thanks.
Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
Jeff King writes: >> ... >> I agree with you that it did correctly sort them in ASCII order. > > What's the purpose of sorting them, though? I thought it was less for > aesthetics and more to to keep lines deterministic (to avoid two > branches adding the same line in different places, thus causing > weirdness when the two are merged). In that case, I think we care less > about the exact order and more that anybody can easily reproduce the > same sort (by running "10:!sort" or whatever you weird emacs-types would > type). In the ideal world, "sort" would have a handy option we can tell it to reshuffle the ASCII table in such a way that all punctuations come before alphanumeric, making sure "/" and "." are the first two letters in the alphabet, and everybody can use it to sort the lines reproducibly and also readably. But I do not know of such a widely used implementation of "sort", so... If we had known better, we would have used such a custom sort order to sort the index entries, making sure that slash sorts before any other byte ;-)
Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
Johannes Schindelin writes: >> ... I do not particularly see this change (there may be similar >> ones) desirable. I'd find it it be much more natural to sort >> "commit-anything" after "commit", and that is true with or without >> the common extension ".o" added to these entries. >> >> In short, flipping these entries because '.' sorts later than '-' is >> making the result look "less sorted", at least to me. > > The problem with this argument is that it disagrees with ASCII, as `-` > has code 0x2d while `.` has code 0x2e, i.e. it is lexicographically > _larger_. I am saying that sorting these in ASCII order did not produce result that is easy to the eyes. You are saying that Denton's patch sorted these lines in ASCII order. I agree with you that it did correctly sort them in ASCII order. That does not make the patch right ;-)
Re: [PATCH] test-progress: fix test failures on big-endian systems
Jeff King writes:
> I wondered if we could be a bit more clever with the definition of
> "struct option". Something like:
>
> diff --git a/parse-options.h b/parse-options.h
> index 38a33a087e..99c7ff466d 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -126,7 +126,10 @@ struct option {
> enum parse_opt_type type;
> int short_name;
> const char *long_name;
> - void *value;
> + union {
> + int *intp;
> + const char *strp;
> + } value;
> const char *argh;
> const char *help;
>
>
> which would let the compiler complain about the type mismatch (of course
> it can't help you if you assign to "intp" while trying to parse a
> string).
>
> Initializing the union from a compound literal becomes more painful,
> but:
>
> 1. That's mostly hidden behind OPT_INTEGER(), etc.
>
> 2. I think we're OK with named initializers these days. I.e., I think:
>
> { OPTION_INTEGER, 'f', "--foo", { .intp = &foo } }
>
> would work OK.
The side that actually use .vale would need to change for obvious
reasons, which may be painful, but I agree it would have easily
prevented the regression from happening in the first place.
Thanks for a food for thought.
Re: [Git Developer Blog] [PATCH] post: a tour of git's object types
Emily Shaffer writes: > +Under the covers, Git is mostly a directed graph of objects. Those objects > come > +in four flavors; from root to leaf (generally), those flavors are: Is "acyclic" worth mentioning, I wonder. > + > +- Tag > +- Commit > +- Tree > +- Blob > + > +We'll take a closer look in the opposite order, though. > + > +# Blob > + > +Surprise! It's a file. Well, kind of - it can also be a symlink to a file - > but > +this is the most atomic type of object. We'll explore these a little more > later, > +but really, it's just a file. It may be easier to understand if we said it is "just a stream of bytes". And of course the simplest applciation of a stream of bytes is to store contents of a file, but it also can be used to store the value of a symbolic file, and also can be used to store the notes. So, really, it's just a stream of bytes. > +A tree references zero or more trees or blobs. Said another way, a tree holds > +one or more trees or files. That captures only half of a tree. It is a mapping from names to objects. Of course, being a mapping, it references other objects (by the way, do not limit the contents to "trees or blobs") on the value side of the mapping. A tree gives names to objects within its scope. It maps names to objects, typically a blob or a tree. Thus, it can be used (and it indeed is used) to represent a directory full of files by storing mapping from filenames to blob objects that store their contents. A subdirectory can be represented by having a mapping from its name to the tree object that represents the contents of the subdirectory. > This sounds familiar - basically, a tree is a > +directory. (Okay, or a symlink to a directory.) It points to more trees No, I do not think it is a symlink to a directory. What makes you think so? I'd stop here for now. I am certain that I haven't read enough to say things either negative or positive about the "naming is hard, naming used in the canonical documentation of Git is unnecessary hard to read and we propose a better wording" premise given by the introduction, so I won't comment on it yet. Thanks.
Re: [PATCH v2] t/README: the test repo does not have global or system configs
Philip Oakley writes: > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index 899e92a1c9..d4c792076d 100644 > --- a/Documentation/git-config.txt > +++ b/Documentation/git-config.txt > @@ -107,7 +107,7 @@ OPTIONS > For writing options: write to global `~/.gitconfig` file > rather than the repository `.git/config`, write to > `$XDG_CONFIG_HOME/git/config` file if this file exists and the > - `~/.gitconfig` file doesn't. > + `~/.gitconfig` file if it doesn't. The original is not easy to read, but this is not that much of an improvement. I think what the original wants to say is - write to global `~/.gitconfig` - but write to the XDG place instead, if XDG one exists and ~/.gitconfig does not exist How about touching a bit more, e.g. For writing, rather than writing the per-repository config file .git/config, write to the global config file, which is $XDG_CONFIG_HOME/git/config (if it exists), or $HOME/.gitconfig (otherwise). to streamline the description? > diff --git a/t/README b/t/README > index 60d5b77bcc..71946902d7 100644 > --- a/t/README > +++ b/t/README > @@ -485,6 +485,13 @@ This test harness library does the following things: > the --root option documented above, and a '.stress-' suffix > appended by the --stress option. > > + - The test framework sets GIT_CONFIG_NOSYSTEM=1, thus ignoring any > + --system config files. The --global config is redirected through > + the environment variables. It unsets the $XDG_CONFIG_HOME variable > + and sets HOME="$TRASH_DIRECTORY" for the tests. > + A basic --local config is created in the test repository. > + See linkgit:git-config[1]. Correct, even though I would say s/thus ignoring/in order to ignore/ instead ;-) Thanks.
Re: [PATCH] rebase: hide --preserve-merges option
Denton Liu writes:
> Since --preserve-merges has been deprecated in favour of
> --rebase-merges, mark this option as hidden so it no longer shows up in
> the usage and completions.
>
> Signed-off-by: Denton Liu
> ---
> This patch continues the deprecation effort and can be based on top of
> `js/rebase-deprecate-preserve-merges`.
Sounds good. Let's keep it and have it part of the first batch
after the release.
>
> builtin/rebase.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 21ac10f739..0d63651d95 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1099,9 +1099,10 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> N_("let the user edit the list of commits to rebase"),
> PARSE_OPT_NOARG | PARSE_OPT_NONEG,
> parse_opt_interactive },
> - OPT_SET_INT('p', "preserve-merges", &options.type,
> - N_("(DEPRECATED) try to recreate merges instead of "
> -"ignoring them"), REBASE_PRESERVE_MERGES),
> + OPT_SET_INT_F('p', "preserve-merges", &options.type,
> + N_("(DEPRECATED) try to recreate merges instead
> of "
> + "ignoring them"),
> + REBASE_PRESERVE_MERGES, PARSE_OPT_HIDDEN),
> OPT_BOOL(0, "rerere-autoupdate",
>&options.allow_rerere_autoupdate,
>N_("allow rerere to update index with resolved "
Re: [PATCH v5] Doc: Bundle file usage
Philip Oakley writes: > @@ -20,11 +20,14 @@ DESCRIPTION > Some workflows require that one or more branches of development on one > machine be replicated on another machine, but the two machines cannot > be directly connected, and therefore the interactive Git protocols (git, > +ssh, http) cannot be used. > + > +The 'git bundle' command packages objects and references in an archive > +at the originating machine, which can then be imported into another > +repository using 'git fetch', 'git pull', or 'git clone', > +after moving the archive by some means (e.g., by sneakernet). > + > +As no > direct connection between the repositories exists, the user must specify a > basis for the bundle that is held by the destination repository: the > bundle assumes that all objects in the basis are already in the Notice that we use the term `basis` here. It is referring to the bottom end(s) of the commit graph. > +`git clone` can use any bundle created without negative refspecs > +(e.g., `new`, but not `old..new`). To be consistent with the phrasing of this particular document we saw earlier, you would have said "without basis", but I think the use of `basis` did not spread beyond "git bundle" documentation. If we were writing "git bundle" and its documentation from scratch using more modern lingo, we probably would say "negative revisions" here. Note that the word `refspec` has no place in the context of this sentence; they are to specify the mapping of refs between the repository in which transferred objects originate and the repository that accept the objects. Also note that `basis` discussed in 'git bundle' is a bit wider concept than `negative revisions`, so mere mechanical replacements would not be sufficient as a preliminary modernization/prepation step for this patch. > +If you want to match `git clone --mirror`, which would include your > +refs such as `refs/remotes/*`, use `--all`. > +If you want to provide the same set of refs that a clone directly > +from the source repository would get, use `--branches --tags` for > +the ``. This is not wrong per-se, but may lead to confusion. The readers must be able to learn: - "git clone --mirror full.bndl dst/" from a full bundle created with "git bundle create full.bndl --all" can mimic creation of a full mirror of the original. - "git clone full.bndl dst/" from such a bundle does *not* result in creation of a mirror. - "git clone slim.bndl dst/" from a minimum bundle created wth "git bundle create slim.bndl --branches --tags" would be sufficient to obtain the same result as the above. - "git clone --mirror slim.bndl dst/" from such a minimum bundle cannot mimic creation of a full mirror of the original. I am not sure the second point is conveyed well with the new paragraph. That is, "--all" must be used if the resulting bundle is meant to be usable to "--mirror" clone from, but it can still be used to clone normally. If you do not intend to mirror-clone from, there is not much point in using "--all" to record extra refs. Not having the new paragraph does not convey anything at all, so it definitely is an improvement, though ;-) Thanks.
Re: [PATCH v2 1/1] ci(osx): use new location of the `perforce` cask
SZEDER Gábor writes: > On Thu, Oct 17, 2019 at 12:47:33PM +, Johannes Schindelin via > GitGitGadget wrote: >> From: Johannes Schindelin >> >> The CI builds are failing for Mac OS X due to a change in the > > s/CI/Azure Pipelines/ > > Our Travis CI builds are fine. > >> location of the perforce cask. The command outputs the following >> error: >> >> + brew install caskroom/cask/perforce >> Error: caskroom/cask was moved. Tap homebrew/cask-cask instead. >> >> So let's try to call `brew cask install perforce` first (which is what >> that error message suggests, in a most round-about way). >> >> The "caskroom" way was added in 672f51cb (travis-ci: >> fix Perforce install on macOS, 2017-01-22) and the justification >> is that the call "brew cask install perforce" can fail due to a checksum >> mismatch: the recipe simply downloads the official Perforce distro, and >> whenever that is updated, the recipe needs to be updated, too. > > This paragraph is wrong, it mixes up things too much. > > Prior to 672f51cb we used to install the 'perforce' _package_ with > 'brew install perforce' (note: no 'cask' in there). The justification > for 672f51cb was that the command 'brew install perforce' simply > stopped working, after Homebrew folks decided that it's better to move > the 'perforce' package to a "cask". It was _their_ justification for > this move that 'brew install perforce' "can fail due to a checksum > mismatch ...", and casks can be installed without checksum > verification. And indeed, both 'brew cask install perforce' and 'brew > install caskroom/cask/perforce' printed something along the lines of: > > ==> No checksum defined for Cask perforce, skipping verification > > It's unclear to me why 672f51cb used 'brew install > caskroom/cask/perforce' instead of 'brew cask install perforce'. It > appears (by running both commands on old Travis CI macOS images) that > both commands worked all the same already back then. > > Anyway, as the error message at the top of the log message shows, > 'brew install caskroom/cask/perforce' has stopped working recently, > but 'brew cask install perforce' still does, so let's use that. > > Note that on Travis CI we explicitly specify which macOS image to use, > and nowadays we don't run 'brew update' during the build process [1], > so both commands work in our builds there. > ... > Now, let's take a step back. > > All 'brew cask install perforce' really does is ... > ... in fact, that's what we have been doing in some of our Linux jobs > since the very beginning, so basically only the download URL has to be > adjusted. This is already in 'next' X-<; reverting a merge is cheap but I prefer to do so when we already have a replacement. Thanks for taking a closer look.
Re: [PATCH v2 3/4] Make die_if_checked_out() prune missing checkouts of unlocked worktrees.
Peter Jones writes:
[jc: won't repeat comments on the title]
> @@ -360,6 +360,12 @@ void die_if_checked_out(const char *branch, int
> ignore_current_worktree)
> wt = find_shared_symref("HEAD", branch);
> if (!wt || (ignore_current_worktree && wt->is_current))
> return;
die-if-checked-out is called from callers that expect to be stopped
before they do any harm, so it feels dirty to make a side effect
like this.
If the user tries to check out a branch that used to be checked out
in an already removed worktree, doesn't that indicate that an
earlier worktree removal was done incorrectly, which is something
worth reporting to the user and give the user a chance to think and
choose what corrective action(s) need to be taken?
For that, instead of automatically losing information like this
patch does, it may make more sense to fail the checkout and stop at
giving diagnosis (e.g. "our record shows that the branch is checked
out in that worktree, but you seem to have lost it. if you forgot
to prune it, then here is the command you can give to do so.")
without actually touching the filesystem.
Thanks.
> +
> + if (prune_worktree_if_missing(wt) >= 0) {
> + delete_worktrees_dir_if_empty();
> + return;
> + }
> +
> skip_prefix(branch, "refs/heads/", &branch);
> die(_("'%s' is already checked out at '%s'"),
> branch, wt->path);
Re: [PATCH v2 2/4] libgit: Expose more worktree functionality.
Peter Jones writes:
Same comment on the commit title as 1/4; also, we tend not to upcase
the first word after the : word and omit the full-stop on the
title (see "git shortlog -32 --no-merges" on our project for
examples).
> Add delete_worktrees_dir_if_empty() and prune_worktree() to the public
> API, so they can be used from more places. Also add a new function,
> prune_worktree_if_missing(), which prunes unlocked worktrees if they
> aren't present on the filesystem.
It probably is cleaner to do the "also" part as a separate step, as
that allows readers to skip this step without reading it deeply, but
let's see how it is done.
> @@ -144,7 +73,7 @@ static void prune_worktrees(void)
> if (is_dot_or_dotdot(d->d_name))
> continue;
> strbuf_reset(&reason);
> - if (!prune_worktree(d->d_name, &reason))
> + if (!prune_worktree(d->d_name, &reason, expire))
> continue;
> if (show_only || verbose)
> printf("%s\n", reason.buf);
> diff --git a/worktree.c b/worktree.c
> index 4924805c389..08454a4e65d 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -608,3 +608,91 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
> +int prune_worktree(const char *id, struct strbuf *reason, timestamp_t expire)
This is not a mere code movement, because the original relied on the
file-scope static "expire", and the public version wants to give
callers control over the expiration value. That is a good change
that deserves to be advertised and explained in the proposed log
message.
> +int prune_worktree_if_missing(const struct worktree *wt)
> +{
> + struct strbuf reason = STRBUF_INIT;
> + int ret;
> +
> + if (is_worktree_locked(wt) ||
> + access(wt->path, F_OK) >= 0 ||
> + (errno != ENOENT && errno == ENOTDIR)) {
> + errno = EEXIST;
> + return -1;
> + }
When access() failed but not because the named path did not exist
(i.e. the directory may still exist---it is just this invocation of
the process happened to fail to see it---or it may not exist but we
cannot see far enough to notice that it does not exist) then we play
safe, assume it does exist, and refrain from calling prune_worktree()
on it. Which makes sense, but do we need to set errno to EEXIST
here? Does prune_worktree() ensure the value left in errno when it
returns failure in a similar way to allow the caller of this new
helper make effective and reliable use of errno?
> + strbuf_addf(&reason, _("Removing worktrees/%s: worktree directory is
> not present"), wt->id);
> + ret = prune_worktree(wt->id, &reason, TIME_MAX);
> + return ret;
> +}
Re: [PATCH v2 1/4] libgit: Add a read-only helper to test the worktree lock
Peter Jones writes:
> Subject: Re: [PATCH v2 1/4] libgit: Add a read-only helper to test the
> worktree lock
Having a word "worktree" somewhere on the title is good, but have it
as the "I am changing this area"; "libgit" does not give readers the
hint that this is a step about the worktree subsystem.
Subject: [PATCH v2 1/4] worktree: add is_worktree_locked() helper
When the new helper function is properly named, like yours, there is
not much need to explain what it does (i.e. "to test the worktree
lock"), so just "worktree: add is_worktree_locked()" is sufficient.
> Add the function is_worktree_locked(), which is a helper to tell if a
> worktree is locked without having to be able to modify it.
I do not see the reason why your proposed title and log message
stress the fact that this helper can be used even by callers that
are not permitted to modify the worktree (i.e. the emphasis on
"read-only"). Asking for worktree_lock_reason() can be done by
anybody, but I do not think we particularly advertise it as
read-only.
Perhaps drop "without having to..."?
> - locked = !!worktree_lock_reason(wt);
> + locked = is_worktree_locked(wt);
> if ((!locked && opts->force) || (locked && opts->force > 1)) {
> if (delete_git_dir(wt->id))
> die(_("unable to re-add worktree '%s'"), path);
> diff --git a/worktree.c b/worktree.c
> index 5b4793caa34..4924805c389 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -244,6 +244,22 @@ int is_main_worktree(const struct worktree *wt)
> return !wt->id;
> }
>
> +int is_worktree_locked(const struct worktree *wt)
> +{
> + struct strbuf path = STRBUF_INIT;
> + int locked = 0;
> +
> + if (wt->lock_reason_valid && wt->lock_reason)
> + return 1;
> +
> + strbuf_addstr(&path, worktree_git_path(wt, "locked"));
> + if (file_exists(path.buf))
> + locked = 1;
If you write
locked = file_exists(path.buf);
here, then readers do not have to scan backwards and find that the
variable is initialized to zero, and that no other statement since
its initialization touches its value, in order to see what value is
returned when file does not exist. Writing the RHS !!file_exists()
concisely allows readers to tell that this function returns only 0
or 1 without having to check what file_exists() returns, but that
may probably be overkill.
> + strbuf_release(&path);
> + return locked;
> +}
I wondered why this is not just
#define is_worktree_locked(wt) (!!worktree_lock_reason(wt))
There are a few differences compared to worktree_lock_reason():
- this can be called on the main worktree by mistake and would
probably yield "not locked" (but the existing guard is a mere
assert() which probably is stripped away in production builds)
- this can be used by a process that cannot even read the contents
of the locked file for the reason;
- because reason is not read, reason or reason_valid fields are not
updated, and repeated calls on the same worktree structure would
result in repeated lstat() calls.
Shouldn't we be advising the callers that the last one as a
potential downside? The fact that the new helper is usable even by
read-only callers hints that any caching of earlier results is
disabled, but it is somewhat a round-about way to say so.
As I do not see why being able to take "const struct worktree *", as
opposed to non-const version is a huge advantage, for this helper, I
wonder if it would make even more sense to introduce one more level
to "lock-reason-valid" and allow caching of is_worktree_locked().
Currently, "lock-reason-valid" only tells us "lock-reason may be
NULL, but that does not necessarily mean it is not locked---you have
to check it" boolean, but it could be instead a tristate:
A: lock-reason may be NULL but that is only because we haven't
even tried to see if the lock file exists
B: NULL-ness of lock-reason reliably tells if the worktree is
locked or not because we have tried file_exists(), but if the
field has non-NULL value, that is *not* the string we read;
if you want to know the reason, you must read the file.
C: NULL in lock-reason means it is not locked; non-NULL in
lock-reason is what we read form the file.
Also, it may make sense to correct the first difference and in a
more meaningful way than assert(), given that the reason why this
helper is introduced is eventually to perform an destructive action
later in the series. Perhaps
if (is_main_worktree(wt))
BUG("is-worktree-locked called for the main worktree");
at the front.
Thanks.
> const char *worktree_lock_reason(struct worktree *wt)
> {
> assert(!is_main_worktree(wt));
> diff --git a/worktree.h b/worktree.h
> index caecc7a281c..5ff16c414b5 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -56,6 +56,11 @@ struct worktree *find_worktree(struct worktree **list,
> */
> int is_main_worktre
Re: [PATCH] test-progress: fix test failures on big-endian systems
SZEDER Gábor writes:
> The reason for that bogus value is that '--total's parameter is parsed
> via parse-options's OPT_INTEGER into a uint64_t variable [1]...
>
> Change the type of that variable from uint64_t to int, to match what
> parse-options expects; in the tests of the progress output we won't
> use values that don't fit into an int anyway.
OK, so when the call to start_progress() is made, the second
argument (i.e. "total" which now is int) is promoted to what the
callee expects, so there needs no other change. Makes sense.
> [1] start_progress() expects the total number as an uint64_t, that's
> why I chose the same type when declaring the variable holding the
> value given on the command line.
I can sympathize, but I do not think it is worth inventing OPT_U64()
or adding "int total_i" whose value is assigned to "u64 total" after
parsing a command line arg with OPT_INTEGER() into the former.
Catching a pointer whose type is not "int*" passed at the third
position of OPT_INTGER() mechanically may be worth it, though.
Would Coccinelle be a suitable tool for that kind of thing?
> int cmd__progress(int argc, const char **argv)
> {
> - uint64_t total = 0;
> + int total = 0;
> const char *title;
> struct strbuf line = STRBUF_INIT;
> struct progress *progress;
Re: [PATCH v1] config/branch: state that .merge is the remote ref
Philip Oakley writes: > branch..merge:: > Defines, for the local branch , the upstream branch ref > _on the remote_ (as given by branch..remote). > The upstream ref may be different from the local branch ref. > > optionally s/different from/ same as/ ? That "optionally" part is exactly why I said "upstream and remote tracking names may or may not differ is irrelevant information". >> The name of the branch at the remote `branch..remote` that >> is used as the upstream branch for the given branch. It tells >> `git fetch`, etc., which branch to merge and ... >> > If this, should we also say it (the key value) is that of the upstream > branch _ref_? Yeah, that makes it clear that readers should not write "master" and use "refs/heads/master" instead. It may even be more (technically) correct to say just "ref" without branch (this ref does not have to be a branch at the remote repository at all). I am not sure if we want to go that far to make it more correct and also make it hint that using a non-branch ref is a valid configuration to readers, but I agree it is a good idea to avoid saying "name" (which implies that "master" is OK, which is not). Thanks.
What's cooking in git.git (Oct 2019, #05; Fri, 18)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. A preview release 2.24-rc0 has been tagged. There still are a few topics in 'next' that we'd like to see in the upcoming release, but the tip of 'master' should otherwise be pretty close to the final already. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * bb/compat-util-comment-fix (2019-10-12) 1 commit (merged to 'next' on 2019-10-15 at c15e45fe28) + git-compat-util: fix documentation syntax Code cleanup. * bb/utf8-wcwidth-cleanup (2019-10-12) 1 commit (merged to 'next' on 2019-10-15 at 92ba59e1d6) + utf8: use ARRAY_SIZE() in git_wcwidth() Code cleanup. * bw/format-patch-o-create-leading-dirs (2019-10-12) 1 commit (merged to 'next' on 2019-10-15 at 93c9949b6a) + format-patch: create leading components of output directory "git format-patch -o " did an equivalent of "mkdir " not "mkdir -p ", which got corrected. * dl/allow-running-cocci-verbosely (2019-10-12) 1 commit (merged to 'next' on 2019-10-15 at 474dc9f86e) + Makefile: respect $(V) in %.cocci.patch target Dev support update. * dl/compat-cleanup (2019-10-11) 1 commit (merged to 'next' on 2019-10-15 at 1ed99770ba) + pthread.h: manually align parameter lists Code formatting micronit fix. * jj/stash-reset-only-toplevel (2019-10-15) 1 commit (merged to 'next' on 2019-10-15 at 28f398daab) + stash: avoid recursive hard reset on submodules "git stash save" lost local changes to submodules, which has been corrected. * js/doc-stash-save (2019-10-11) 1 commit (merged to 'next' on 2019-10-15 at e588bff32c) + doc(stash): clarify the description of `save` Doc clarification. * rs/column-use-utf8-strnwidth (2019-10-15) 1 commit (merged to 'next' on 2019-10-15 at 3be15b4478) + column: use utf8_strnwidth() to strip out ANSI color escapes Code cleanup. * rs/http-push-simplify (2019-10-15) 1 commit (merged to 'next' on 2019-10-15 at 4abc29286c) + http-push: simplify deleting a list item Code cleanup. * rs/remote-curl-use-argv-array (2019-10-15) 1 commit (merged to 'next' on 2019-10-15 at 8d0375a874) + remote-curl: use argv_array in parse_push() Code cleanup. * ta/t1308-typofix (2019-10-11) 1 commit (merged to 'next' on 2019-10-15 at 0228b44688) + t1308-config-set: fix a test that has a typo Test fix. -- [New Topics] * dd/notes-copy-default-dst-to-head (2019-10-18) 2 commits (merged to 'next' on 2019-10-18 at 2588a175ec) + notes: fix minimum number of parameters to "copy" subcommand + t3301: test diagnose messages for too few/many paramters "git notes copy $original" ought to copy the notes attached to the original object to HEAD, but a mistaken tightening to command line parameter validation made earlier disabled that feature by mistake. Will cook in 'next'. No need for rush, as this is a fix for an ancient regression. * jc/log-graph-simplify (2019-10-16) 13 commits - graph: fix coloring of octopus dashes - graph: flatten edges that fuse with their right neighbor - graph: smooth appearance of collapsing edges on commit lines - graph: rename `new_mapping` to `old_mapping` - graph: commit and post-merge lines for left-skewed merges - graph: tidy up display of left-skewed merges - graph: example of graph output that can be simplified - graph: extract logic for moving to GRAPH_PRE_COMMIT state - graph: remove `mapping_idx` and `graph_update_width()` - graph: reduce duplication in `graph_insert_into_new_columns()` - graph: reuse `find_new_column_by_commit()` - graph: handle line padding in `graph_next_line()` - graph: automatically track display width of graph lines The implementation of "git log --graph" got refactored and then its output got simplified. Will merge to 'next'. * js/azure-ci-osx-fix (2019-10-18) 1 commit (merged to 'next' on 2019-10-18 at bc6a502f6e) + ci(osx): use new location of the `perforce` cask Update installation procedure for Perforce on MacOS in the CI jobs running on Azure pipelines, which was failing. Will merge to 'master'. * js/git-path-head-dot-lock-fix (2019-10-18) 2 commits - git_path(): handle `.lock` files correctly - t1400: wrap setup code in test case "git rev-parse --git-path HEAD.lock" did not give the right path when run in a secondary worktree. * pw/post-commit-from-sequencer (2019-10-16) 6 commits (merged to 'next' on 2019-10-18 at 15b41a097d) + sequencer: run post-commit hook + move run_commit_hook() to libgit and use it there + sequencer.h fix placement of #endif + t3404: remove uneeded calls to set
[ANNOUNCE] Git v2.24.0-rc0
An early preview release Git v2.24.0-rc0 is now available for testing at the usual places. It is comprised of 493 non-merge commits since v2.23.0, contributed by 63 people, 15 of which are new faces. The tarballs are found at: https://www.kernel.org/pub/software/scm/git/testing/ The following public repositories all have a copy of the 'v2.24.0-rc0' tag and the 'master' branch that the tag points at: url = https://kernel.googlesource.com/pub/scm/git/git url = git://repo.or.cz/alt-git.git url = https://github.com/gitster/git New contributors whose contributions weren't in v2.23.0 are as follows. Welcome to the Git development community! Alexandr Miloslavskiy, Ali Utku Selen, Ben Milman, Cameron Steffen, CB Bailey, Garima Singh, Hervé Beraud, Jakob Jarmar, Kunal Tyagi, Max Rothman, Paul Wise, Pedro Sousa, Philip.McGraw, Pratyush Yadav, and YanKe. Returning contributors who helped this release are as follows. Thanks for your continued support. Adam Roben, Ævar Arnfjörð Bjarmason, Alex Henrie, Andrey Mazo, Beat Bolli, Ben Wijen, Bert Wesarg, Birger Skogeng Pedersen, brian m. carlson, Carlo Marcelo Arenas Belón, Christian Couder, Clément Chigot, Corentin BOMPARD, David Turner, Denton Liu, Derrick Stolee, Elijah Newren, Emily Shaffer, Eric Wong, Gabriele Mazzotta, Jeff Hostetler, Jeff King, Johannes Schindelin, Johannes Sixt, Jonathan Tan, Jon Simons, Josh Steadmon, Junio C Hamano, Martin Ågren, Masaya Suzuki, Matheus Tavares, Matthew DeVore, Michael J Gruber, Mike Hommey, Mischa POSLAWSKY, Paul Mackerras, Phillip Wood, René Scharfe, Stephen Boyd, Stephen P. Smith, Sun Chao, SZEDER Gábor, Tanay Abhra, Taylor Blau, Thomas Gummerer, Tobias Klauser, Torsten Bögershausen, and Varun Naik. Git 2.24 Release Notes (draft) == Updates since v2.23 --- Backward compatibility note * Although it is not officially deprecated, "filter-branch" is showing its age and alternatives are available. From this release, we started to discourage its uses and hint people about filter-repo. UI, Workflows & Features * We now have an active interim maintainer for the Git-Gui part of the system. Praise and thank Pratyush Yadav for volunteering. * The command line parser learned "--end-of-options" notation; the standard convention for scripters to have hardcoded set of options first on the command line, and force the command to treat end-user input as non-options, has been to use "--" as the delimiter, but that would not work for commands that use "--" as a delimiter between revs and pathspec. * A mechanism to affect the default setting for a (related) group of configuration variables is introduced. * "git fetch" learned "--set-upstream" option to help those who first clone from their private fork they intend to push to, add the true upstream via "git remote add" and then "git fetch" from it. * Device-tree files learned their own userdiff patterns. (merge 3c81760bc6 sb/userdiff-dts later to maint). * "git rebase --rebase-merges" learned to drive different merge strategies and pass strategy specific options to them. * A new "pre-merge-commit" hook has been introduced. * Command line completion updates for "git -c var.name=val" have been added. * The lazy clone machinery has been taught that there can be more than one promisor remote and consult them in order when downloading missing objects on demand. * The list-objects-filter API (used to create a sparse/lazy clone) learned to take a combined filter specification. * The documentation and tests for "git format-patch" have been cleaned up. * On Windows, the root level of UNC share is now allowed to be used just like any other directory. * The command line completion support (in contrib/) learned about the "--skip" option of "git revert" and "git cherry-pick". * "git rebase --keep-base " tries to find the original base of the topic being rebased and rebase on top of that same base, which is useful when running the "git rebase -i" (and its limited variant "git rebase -x"). The command also has learned to fast-forward in more cases where it can instead of replaying to recreate identical commits. * A configuration variable tells "git fetch" to write the commit graph after finishing. * "git add -i" has been taught to show the total number of hunks and the hunks that has been processed so far when showing prompts. * "git fetch --jobs=" allowed parallel jobs when fetching submodules, but this did not apply to "git fetch --multiple" that fetches from multiple remo
Re: [PATCH v2 2/3] grep: make PCRE2 aware of custom allocator
"Carlo Marcelo Arenas Belón via GitGitGadget" writes: > builtin/grep.c | 1 + > grep.c | 34 +- > grep.h | 1 + > 3 files changed, 35 insertions(+), 1 deletion(-) > > +#if defined(USE_LIBPCRE2) > + if (!pcre2_global_context) > + pcre2_global_context = pcre2_general_context_create( > + pcre2_malloc, pcre2_free, NULL); > +#endif This part should use the same "#ifdef" as the other one which is a minor nit, for consistency. I do not care too deeply which way we unify, but we should stick to one. > + > #ifdef USE_LIBPCRE1 > pcre_malloc = malloc; > pcre_free = free; Other than that, all 3 patches look sensible, and they certainly got simplified by dropping the conditional ;-).
Re: [PATCH v1] config/branch: state that .merge is the remote ref
Philip Oakley writes: > branch..merge:: > Defines, together with branch..remote, the upstream branch > - for the given branch. It tells 'git fetch'/'git pull'/'git rebase' which > + for the given branch. It defines the branch name _on the remote_, > + which may be different from the local branch name. While I do agree with the goal of make things more clear, I'd avoid being overly redundant and giving irrelevant information (e.g. the copy you have locally may be made under arbitrary name that is different from the name used at the remote). After all, the users do not even need to know about the remote-tracking branch to understand this naming mechanism. Perhaps something along this line instead: The name of the branch at the remote `branch..remote` that is used as the upstream branch for the given branch. It tells `git fetch`, etc., which branch to merge and ...
Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly
"Johannes Schindelin via GitGitGadget"
writes:
> From: Johannes Schindelin
>
> Ever since worktrees were introduced, the `git_path()` function _really_
> needed to be called e.g. to get at the path to `logs/HEAD` (`HEAD` is
> specific to the worktree). However, the wrong path is returned for
> `logs/HEAD.lock`.
>
> This does not matter as long as the Git executable is doing the asking,
> as the path for that `index.lock` file is constructed from
> `git_path("index")` by appending the `.lock` suffix.
Is this still git_path("index") or is it now HEAD?
> Side note: Git GUI _does_ ask for `index.lock`, but that is already
> resolved correctly.
Is that s/but/and/?
> diff --git a/path.c b/path.c
> index e3da1f3c4e..ff85692b45 100644
> --- a/path.c
> +++ b/path.c
> @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key,
> match_fn fn,
> int result;
> struct trie *child;
>
> - if (!*key) {
> + if (!*key || !strcmp(key, ".lock")) {
We only do strcmp for the tail part at the end of the path, so this
should probably OK from performance point of view but semantically
it is not very satisfying to see a special case for a single .suffix
this deep in the callchain. I wonder if it is nicer to have the
higher level callers notice ".lock" or whatever other suffixes they
care about and ask the lower layer for a key with the suffix
stripped?
Will queue.
Thanks.
Re: [PATCH v3 08/13] graph: tidy up display of left-skewed merges
Derrick Stolee writes: > I hit this very situation recently when I was experimenting with > 'git fast-import' and accidentally created many parallel, independent > histories. Running "git log --graph --all --simplify-by-decoration" > made it look like all the refs were in a line, but they were not. > (The one way I knew something was up: the base commits also appeared > without a decoration. That was the only clue that the histories did > not continue in a line.) > >> >> and the fact that B and A do not share parent-child relationships is >> lost. An easy way to show that would be to draw the bottom three >> lines of the full history output we saw earlier: >> >> | * e6277a9 C >> | * 13ae9b2 B >> * afee005 A >> >> either with or without the vertical bar to imply that A may have a >> child. > The natural extension of this would be multiple columns: > > | | | | | * > | | | | * > | | | * > | | * > | * > * After sleeping over it, I now think we shouldn't draw lines that imply a child for each of these commits, as we haven't seen, but I agree that this can be extended to 3 or more roots.
Re: [PATCH 2/2] notes: fix minimum number of parameters to "copy" subcommand
Doan Tran Cong Danh writes: > Documentation/git-notes.txt | 6 +++--- > builtin/notes.c | 2 +- > t/t3301-notes.sh| 40 +++-- > 3 files changed, 42 insertions(+), 6 deletions(-) Thanks, makes sense.
Re: [PATCH 1/2] t3301: test diagnose messages for too few/many paramters
Doan Tran Cong Danh writes: > If we accidentally lifted the check inside our code base, the test may > still failed because the provided parameter is not a valid ref. Makes sense. Another option would be to use a valid ref to make sure there are no other possible reason for the command to fail, which would make the test robust against us accidentally switching the order of the check to see if they are refs first and then complain about too many or too few arguments ;-) But I think what we have here is fine as-is (I'll copy-edit the proposed log message for grammos, though). Thanks. > > Correct it. > > Signed-off-by: Doan Tran Cong Danh > --- > t/t3301-notes.sh | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh > index d3fa298c6a..d7767e4438 100755 > --- a/t/t3301-notes.sh > +++ b/t/t3301-notes.sh > @@ -1167,8 +1167,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides > config' ' > ' > > test_expect_success 'git notes copy diagnoses too many or too few > parameters' ' > - test_must_fail git notes copy && > - test_must_fail git notes copy one two three > + test_must_fail git notes copy 2>error && > + test_i18ngrep "too few parameters" error && > + test_must_fail git notes copy one two three 2>error && > + test_i18ngrep "too many parameters" error > ' > > test_expect_success 'git notes get-ref expands refs/heads/master to > refs/notes/refs/heads/master' '
Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
Denton Liu writes: > There are many += lists in the Makefile and, over time, they have gotten > slightly out of order, alphabetically. Alphabetically sort all += lists > to bring them back in order. > ... Hmm. I like the general thrust, but ... > LIB_OBJS += combine-diff.o > -LIB_OBJS += commit.o > LIB_OBJS += commit-graph.o > LIB_OBJS += commit-reach.o > +LIB_OBJS += commit.o ... I do not particularly see this change (there may be similar ones) desirable. I'd find it it be much more natural to sort "commit-anything" after "commit", and that is true with or without the common extension ".o" added to these entries. In short, flipping these entries because '.' sorts later than '-' is making the result look "less sorted", at least to me. Thanks.
Re: [PATCH v2 1/3] doc: provide guidance on user.name format
Jeff King writes: > I think this is a good distinction to draw, but... > >> Documentation/git-commit-tree.txt | 6 ++ >> 1 file changed, 6 insertions(+) > > ...I was surprised to see it here, where I think most users wouldn't > find it. Would it make sense in git-commit(1), or in the description of > the user.name config? Yeah, I had the same reaction as you had, both positive and negative (eh, rather "surprised").
Re: [PATCH 1/1] builtin/blame.c: constants into bit shift format
"Hariom Verma via GitGitGadget" writes: > -#define OUTPUT_SHOW_AGE_WITH_COLOR 04000 > +#define OUTPUT_ANNOTATE_COMPAT (1<<0) > +#define OUTPUT_LONG_OBJECT_NAME (1<<1) > +#define OUTPUT_RAW_TIMESTAMP(1<<2) > +#define OUTPUT_PORCELAIN(1<<3) > +#define OUTPUT_SHOW_NAME(1<<4) > +#define OUTPUT_SHOW_NUMBER (1<<5) > +#define OUTPUT_SHOW_SCORE (1<<6) > +#define OUTPUT_NO_AUTHOR(1<<7) > +#define OUTPUT_SHOW_EMAIL (1<<8) > +#define OUTPUT_LINE_PORCELAIN (1<<9) > +#define OUTPUT_COLOR_LINE (1<<10) > +#define OUTPUT_SHOW_AGE_WITH_COLOR (1<<11) For these small shift counts it probably would not matter, but it may be a good discipline to make sure they are treated as constants of an unsigned type (i.e. write them as (1U<<0) etc.). It probably starts to matter when you reach 1<<31 if these are bits stuffed into "unsigned int" on 32-bit arch. One advantage of octal and hexadecimal notations have is that 0x8000 is automatically unsigned, IIRC, on such an archtecture.
Re: [PATCH 0/1] builtin/blame.c: bit field constants into bit shift format
"Hariom Verma via GitGitGadget" writes: > we are looking at bitfield constants, and elsewhere in the Git source code, > such cases are handled via bit shift operators rather than octal numbers, > which also makes it easier to spot holes in the range (if, say, 1<<5 was > missing, it is easier to spot it between 1<<4 and 1<<6 than it is to spot a > missing 040 between a 020 and a 0100). Also, bit shifts lead to low-level > optimizations because they require fewer calculations for the CPU. I think the last sentence is a nonsense for any decent compiler that turns "1<<5" into 040 at compile time and treats it as literal integer. Luckily, it only appears here in the cover letter and does not appear in the patch proper, so no need to resend the patch to correct this ;-)
Re: [PATCH 2/2] git_path(): handle `.lock` files correctly
SZEDER Gábor writes: > However, I'm not sure what the right path should be in the first > place, given that each working tree has its own 'logs' directory, but > only for HEAD's reflog, while everything else goes to the main working > tree's 'logs' directory. As all worktrees should share the same view of where 'master' (for example) branch points at, what commit it was pointing at before, etc., the reflogs should also be shared for refs. The exception is the HEAD where each worktree can point at its own commit (when detached) or a branch (when not). The above "should" does not mean "I know the code works that way so if your git behaves differently your compiler is wrong"; it just means "that's the logical conclusion of the basic design, and if your git does not behave that way, we have a bug (or two)". Thanks.
Re: [PATCH] remote-curl: pass on atomic capability to remote side
"brian m. carlson" writes: > Yeah, my default editor configuration for AsciiDoc is two spaces. I > noticed that Junio's already picked it up for next, but I'll send a v2 > with this fixed in case he wants to merge the fixed version to master > instead. > > If it's more convenient, I can send a follow-up patch fixing the whitespace. Either is fine. At this point in the cycle where we are very close to -rc0, I do not mind too much about having a few reverts of 'next' to give a clean slate to an obvious and trivial fix. Thanks.
Re: [RFC PATCH 10/10] pack-objects: improve partial packfile reuse
Jeff King writes: >> Hmm, I am kind of surprised that the decoding side allowed such a >> padding. > > IIRC, the "padding" is just a sequence of 0-length-plus-continuation-bit > varint bytes. And for some reason it worked for the size but not for the > delta offset value. I think the reason is because they use different varint definition. The encoding used in builtin/pack-objects.c::write_no_reuse_object() is for offsets, and it came much later and with an improvement over the encoding used for delta size in diff-delta.c::create_delta(). The more recent encoding does not allow padding (when I compare the decoders for these two encodings, I notice there is +1 for each 7-bit iteration; this essentially declares that a byte with "not the final byte" bit set with all other bits clear does not mean 0 but it means 1, which breaks the idea of padding to encode filler zero bits).
Re: email as a bona fide git transport
Vegard Nossum writes: > Step 1: > > * git send-email needs to include parent SHA1s and generally all the > information needed to perfectly recreate the commit when applied so > that all the SHA1s remain the same > > * git am (or an alternative command) needs to recreate the commit > perfectly when applied, including applying it to the correct parent You can record and convey the commit object name a series is meant to be applied on top already, and it in general is a good way to give a wider context in order to explain and justify the series. On the other hand, "all the information needed to recreate..." is not very useful. If you want the commit object to be exactly what you want to see at the tip of the end result, you are better off asking your upstream to pull. Using e-mail for that makes you and project participants give up a lot of benefits the workflow based on e-mail gives you, the biggest of which is the ease of giving suggestions for improvements. Once you insist "perfectly recreate the commit", you are not willing to take any input from the sidelines---worse yet, you are even dictating when the upstream runs "git am" to turn them into commits, and do so without reading the patches (there is no point reviewing as the person who runs "git am" is not even allowed to fix typo or make obvious fixes to the code, which will fail to perfectly recreate the commit). In short, one should resist temptation to bring up "perfect reproduction" when one talks about e-mail workflow. > * Instead of describing a patchset in a separate introduction email, we > can create a merge commit between the parent of the first commit in > the series and the last and put the patchset description in the merge > commit [5]. This means the patchset description also gets to be part > of git history. This has been done with tools around git-core, and merits a more official support. When merging a topic, it is a good idea to explain in the merge commit that brings in the topic to the mainline what the topic is about, and at least in the past few years Linus and other maintainers both within and outside the kernel have been doing so. The cover-letter material in [PATCH 00/NN] obviously can help those integrators when they write the merge message. > (This would require support for git send-email/am to be able to send > and apply merge commits -- at least those which have the same tree as > one of the parents. This is _not_ yet supported in my proposed git > patches.) This does not require much from format-patch and am. All you need to do is to ensure that they can handle an empty commit. What you need more is a support in merge. The outline for the workflow would go like this: * The contributor prepares an N patch series 1/N..N/N on a single topic branch. * The summary of the series, the message that is meant to help the integrator, is recorded as (N+1)th commit at the tip of the topic branch, as an empty commit (i.e. a commit that records the same tree as its parent). * "git format-patch" is taught, when told to prepare the patch e-mails from such a topic branch, to notice the unusual "an empty commit at the tip" layout, and turn that into 0/N of the message. * "git am" is taught a new option to cap a topic branch made from patches 1/N..N/N from the incoming mbox with an extra empty commit, whose message is taken from the 0/N cover-letter material, to recreate what the contributor had in the second step above. * "git merge" is taught, when told to merge a topic branch, to notice the unusual "an empty commit at the tip" layout, and (1) merge topic~1 instead to excise the empty commit itself, (2) take the log message from the empty commit at the tip and use it to help prepare the log message of the merge.
Re: [PATCH v2] Doc: Bundle file usage
Jeff King writes: > Maybe: > > 'git fetch', 'git pull', and 'git clone' > > ? Given the repetition below, though: ... including this one, I think you covered everything I wanted to say on the patch already. Thanks.
Re: [PATCH v3 01/13] graph: automatically track display width of graph lines
Junio C Hamano writes: >> int graph_next_line(struct git_graph *graph, struct strbuf *sb) > > I just noticed it but does this have to be extern? Nobody outside > graph.[ch] seems to have any reference to it. I was stupid; strike this part out. Thanks.
Re: [PATCH v3 08/13] graph: tidy up display of left-skewed merges
"James Coglan via GitGitGadget" writes: > This effect is applied to both "normal" two-parent merges, and to > octopus merges. It also reduces the vertical space needed for pre-commit > lines, as the merge occupies one less column than usual. > > Before: After: > > | * | * > | |\| |\ > | | \ | * \ > | | \ |/|\ \ > | *-. \ > | |\ \ \ Looking at these drawings reminded me of a tangent that is brought up from time to time. We do not do great job when showing multiple roots. If you have a history like this: A---D---E / B---C drawing the graph _with_ the merge gives a reasonable representation of the entire topology. * 46f67dd E * 6f89516 D |\ | * e6277a9 C | * 13ae9b2 B * afee005 A But if you start drawing from parents of D (excluding D), you'd get this: * e6277a9 C * 13ae9b2 B * afee005 A and the fact that B and A do not share parent-child relationships is lost. An easy way to show that would be to draw the bottom three lines of the full history output we saw earlier: | * e6277a9 C | * 13ae9b2 B * afee005 A either with or without the vertical bar to imply that A may have a child. This is not something that has to be done as part of this series, but I am hoping that the internal simplification and code restructuring that is done by this series would make it easier to enhance the system to allow such an output. Thanks.
Re: [PATCH v3 02/13] graph: handle line padding in `graph_next_line()`
"James Coglan via GitGitGadget" writes: > From: James Coglan > > Now that the display width of graph lines is implicitly tracked via the > `graph_line` interface, the calls to `graph_pad_horizontally()` no > longer need to be located inside the individual output functions, where > the character counting was previously being done. > > All the functions called by `graph_next_line()` generate a line of > output, then call `graph_pad_horizontally()`, and finally change the > graph state if necessary. As padding is the final change to the output > done by all these functions, it can be removed from all of them and done > in `graph_next_line()` instead. Very well explained.
Re: [PATCH v3 01/13] graph: automatically track display width of graph lines
"James Coglan via GitGitGadget" writes:
> +struct graph_line {
> + struct strbuf *buf;
> + size_t width;
> +};
> +
> +static inline void graph_line_addch(struct graph_line *line, int c)
> +{
> + strbuf_addch(line->buf, c);
> + line->width++;
> +}
> +
> +static inline void graph_line_addchars(struct graph_line *line, int c,
> size_t n)
> +{
> + strbuf_addchars(line->buf, c, n);
> + line->width += n;
> +}
> +
> +static inline void graph_line_addstr(struct graph_line *line, const char *s)
> +{
> + strbuf_addstr(line->buf, s);
> + line->width += strlen(s);
> +}
> +
> +static inline void graph_line_addcolor(struct graph_line *line, unsigned
> short color)
> +{
> + strbuf_addstr(line->buf, column_get_color_code(color));
> +}
All makes sense, and as long as nobody uses strbuf_add*() on
line->buf directly, it shouldn't be too hard to extend these
to support graph drawn characters outside ASCII in the future
after this series settles.
> static void graph_output_pre_commit_line(struct git_graph *graph,
> ...
> for (i = 0; i < graph->num_columns; i++) {
> struct column *col = &graph->columns[i];
> if (col->commit == graph->commit) {
> seen_this = 1;
> - strbuf_write_column(sb, col, '|');
> - strbuf_addchars(sb, ' ', graph->expansion_row);
> - chars_written += 1 + graph->expansion_row;
> + graph_line_write_column(line, col, '|');
> + graph_line_addchars(line, ' ', graph->expansion_row);
Nice reduction of noise, as the proposed log message promises.
> int graph_next_line(struct git_graph *graph, struct strbuf *sb)
I just noticed it but does this have to be extern? Nobody outside
graph.[ch] seems to have any reference to it. That might mean that
it may be easier than we thought earlier to change the second
parameter of this to "struct graph_line *", instead of us wrapping
an incoming strbuf (which external callers are aware of, as opposed
to graph_line which we prefer not to expose to outsiders). Whether
we change the second parameter or not, the first clean-up may be to
turn this function into a file-scope static, perhaps?
All the changes are very pleasing to see. Thanks.
Re: [PATCH 01/11] graph: automatically track visible width of `strbuf`
James Coglan writes:
>> Is there a reason why you need a pointer to a strbuf that is
>> allocated separately? E.g. would it make it harder to manage
>> if the above were
>>
>> struct graphbuf {
>> struct strbuf buf;
>> int width; /* display width in columns */
>> };
>>
>> which is essentially what Dscho suggested?
>
> I used a pointer here because I create the wrapper struct in
> `graph_next_line()`, which is an external interface that takes a
> `struct strbuf *`:
>
> int graph_next_line(struct git_graph *graph, struct strbuf *sb)
> {
> struct graph_line line = { .buf = sb, .width = 0 };
> // ...
> }
>
> So I'm not allocating the strbuf here, just wrapping a pointer to
> it.
OK, so existing callers allocate strbuf, and you are merely adding a
wrapper structure to keep track of the width. The management of the
lifetime of the strbuf is not your business so there is no reason to
inline the structure in graph_line. Makes sense. Thanks.
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag
William Baker writes: > At this time there are no other MIDX_* flags and so there's always the option > to revisit the best way to handle multiple MIDX_* flags if/when additional > flags are added. I do not care too deeply either way, but if you wrote it in one way, how about completing the series without changing it in the middle, and leave the clean-ups to a follow-up series (if needed)?
Re: [PATCH v3 1/1] doc: Change zsh git completion file name
"Maxim Belsky via GitGitGadget" writes: > From: Maxim Belsky > Subject: Re: [PATCH v3 1/1] doc: Change zsh git completion file name Lack of attention to the detail. This is not about changing the filename anymore. Subject: [PATCH v3] completion: clarify installation instruction for zsh perhaps. I'll make the change locally, so unless there are other changes you want to make, there is no need to resend. Thanks. > The original comment does not describe type of ~/.zsh/_git explicitly > and zsh does not warn or fail if a user create it as a dictionary. > So unexperienced users could be misled by the original comment. > > There is a small update to clarify it. > > Helped-by: Johannes Schindelin > Helped-by: Junio C Hamano > Helped-by: SZEDER Gábor > Signed-off-by: Maxim Belsky > --- > contrib/completion/git-completion.zsh | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/contrib/completion/git-completion.zsh > b/contrib/completion/git-completion.zsh > index 886bf95d1f..b480e3f316 100644 > --- a/contrib/completion/git-completion.zsh > +++ b/contrib/completion/git-completion.zsh > @@ -11,8 +11,9 @@ > # > # zstyle ':completion:*:*:git:*' script ~/.git-completion.zsh > # > -# The recommended way to install this script is to copy to '~/.zsh/_git', and > -# then add the following to your ~/.zshrc file: > +# The recommended way to install this script is to make a copy of it in > +# '~/.zsh/' directory as '~/.zsh/_git' and then add the following to your > +# ~/.zshrc file: > # > # fpath=(~/.zsh $fpath)
Re: [PATCH 1/1] notes: copy notes to HEAD by default
Doan Tran Cong Danh writes:
> The target objects for copying notes was defaulted to HEAD from very
> early stage of git-notes.
>
> However, that default was limited by commit bbb1b8a35a, ("notes: check
> number of parameters to "git notes copy"", 2010-06-28).
Sorry, I don't quite get the above. The said commit made sure 'git
notes copy' gets the right number of arguments, saying """Otherwise
we may segfault with too few parameters.""" I take that as a sign
that before that commit it was not defaulting to HEAD but attempting
to access the missing argv[2] (or whatever the index the
should be at) and dereferencing a NULL?
... goes and digs ...
I think v1.6.6.1-458-g74884b524e is the commit that made the command
line parsing into the current shape, i.e. one parse_options() call
in each of the subcommand that gets dispatched, and you are right
that with that version a single argument given on the command line
is taken as the and defaults to HEAD.
So... what happend between that vesrion and v1.7.1-200-gbbb1b8a35a?
... goes and looks at bbb1b8a35a again ...
Ah, I think there is an off-by-one. When not from-stdin and not
using rewrite-cmd, before that patch, we did not even check if
from-obj exists, so in that sense, the commit had a right idea that
it must check for "too few parameters", but it shouldn't have
insisted that we have at least two. It is OK to have just one,
i.e. only the from-obj, for our purpose.
> copy::
> - Copy the notes for the first object onto the second object.
> - Abort if the second object already has notes, or if the first
> + Copy the notes for the first object onto the second object (defaults to
> + HEAD). Abort if the second object already has notes, or if the first
> object has none (use -f to overwrite existing notes to the
> second object). This subcommand is equivalent to:
> `git notes add [-f] -C $(git notes list ) `
This is OK.
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 02e97f55c5..95456f3165 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -513,7 +513,7 @@ static int copy(int argc, const char **argv, const char
> *prefix)
> }
> }
>
> - if (argc < 2) {
> + if (argc < 1) {
> error(_("too few parameters"));
> usage_with_options(git_notes_copy_usage, options);
> }
So is this.
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index d3fa298c6a..a8f9a0f36c 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -908,6 +908,10 @@ test_expect_success 'allow overwrite with "git notes
> copy -f"' '
> git notes copy -f HEAD~2 HEAD &&
> git log -1 >actual &&
> test_cmp expect actual &&
> + test "$(git notes list HEAD)" = "$(git notes list HEAD~2)" &&
> + git notes copy -f HEAD~2 &&
> + git log -1 >actual &&
> + test_cmp expect actual &&
> test "$(git notes list HEAD)" = "$(git notes list HEAD~2)"
> '
This I am not sure is a good test to add to, especially as a fix to
bbb1b8a, which added this test:
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 64f32ad94d..2d67a40fc1 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1044,4 +1044,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides
config' '
git log -1 > output &&
test_cmp expect output
'
+
+test_expect_success 'git notes copy diagnoses too many or too few parameters' '
+ test_must_fail git notes copy &&
+ test_must_fail git notes copy one two three
+'
+
test_done
The lack of testing that "git notes copy " succeeding is
why the off-by-one bug was not noticed, so I think that test (which
still exists to this day) is the right place to add a test to
protect this fix.
As to the log message, here is how I would explain/justify the
change, if I were writing it.
notes: fix minimum number of parameters to "copy" subcommand
The builtin/notes.::copy() function is prepared to handle
only one argument given from the command line; in such a
case, to-obj defaults to HEAD.
When bbb1b8a3 ("notes: check number of parameters to "git
notes copy"", 2010-06-28) tried to make sure "git notes
copy" (with *no* other argument) does not dereference NULL
by making sure we did not get too few parameters, it
incorrectly insisted that we get two arguments, instead of
just one. This disabled the defaulting to-obj to HEAD.
Correct it.
Thanks.
Re: [PATCH v2 0/6] sequencer: start running post-commit hook again
Johannes Schindelin writes: > Hi Phillip, > > On Tue, 15 Oct 2019, Phillip Wood via GitGitGadget wrote: > >> When I converted the sequencer to avoid forking git commit i forgot about >> the post-commit hook. These patches are based on >> pw/rebase-i-show-HEAD-to-reword, otherwise the new test fails as that branch >> changes the number of commits we make. >> ... > > I had a look over the patches, too, and all looks good to me. Me three; thanks, both.
Re: [PATCH v6 0/3] format-patch: learn --infer-cover-subject option (also t4014 cleanup)
Thanks. I think we are ready for 'next'.
Re: What's cooking in git.git (Oct 2019, #04; Tue, 15)
Elijah Newren writes: >> * en/merge-recursive-directory-rename-fixes (2019-10-12) 2 commits >> (merged to 'next' on 2019-10-15 at ebfdc3ff7b) >> + merge-recursive: fix merging a subdirectory into the root directory >> + merge-recursive: clean up get_renamed_dir_portion() >> >> A few glitches in the heuristic in merge-recursive to infer file >> movements based on movements of other files in the same directory >> have been corrected. >> >> Will merge to 'master'. > > I'm surprised this one was merged straight down to next; perhaps I > should have highlighted my plans a bit clearer in the thread? My mistake. I am willing to revert the merge to give the topic a clean slate. Just tell me so. Thanks. > Also, a very minor point but "glitches" may be misleading; it suggests > (to me at least) a malfunction rather than a failure to trigger,... I used the word to mean a failure to trigger (after all, a heuristic that fails to trigger when most people would naturelly expect it to is showing a glitch in that case). A better phrasing, please?
Re: ds/sparse-cone, was Re: What's cooking in git.git (Oct 2019, #03; Fri, 11)
Derrick Stolee writes: > On 10/15/2019 3:11 AM, Eric Wong wrote: >> Junio C Hamano wrote: >>> Eric Wong writes: >>> >>>> I just took a brief look, but that appears to leak memory. >>>> >>>> "hashmap_free(var, 1)" should be replaced with >>>> "hashmap_free_entries(var, struct foo, member)" >>>> >>>> Only "hashmap_free(var, 0)" can become "hashmap_free(var)" >>> >>> I deliberately avoided merge-time band-aid fixups on this topic and >>> ew/hashmap exactly because I was sure that I'd introduce a similar >>> bugs by doing so myself. Using evil merges can be a great way to >>> help multiple topics polished independently at the same time, but >>> when overused, can hide this kind of gotchas quite easily. >>> >>> A reroll on top of ew/hashmap would be desirable, now that topic is >>> ready for 'master'. >> >> Just to be clear, that reroll should come from Stolee (+Cc-ed), right? >> I'll be around to help answer questions, but also pretty busy >> with other stuff and I think Stolee knows this API pretty well :> > > I'm working on the re-roll, yes. I was waiting for ew/hashmap to merge > with history that included ds/include-exclude. Now the current 'master' > has both, so I can rebase and check everything carefully. v4 should > have every commit compile with the new hashmap API. Thanks, both.
What's cooking in git.git (Oct 2019, #04; Tue, 15)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ab/pcre-jit-fixes (2019-08-19) 18 commits (merged to 'next' on 2019-10-04 at 2d55f2b470) + grep: under --debug, show whether PCRE JIT is enabled + grep: do not enter PCRE2_UTF mode on fixed matching + grep: stess test PCRE v2 on invalid UTF-8 data + grep: create a "is_fixed" member in "grep_pat" + grep: consistently use "p->fixed" in compile_regexp() + grep: stop using a custom JIT stack with PCRE v1 + grep: stop "using" a custom JIT stack with PCRE v2 + grep: remove overly paranoid BUG(...) code + grep: use PCRE v2 for optimized fixed-string search + grep: remove the kwset optimization + grep: drop support for \0 in --fixed-strings + grep: make the behavior for NUL-byte in patterns sane + grep tests: move binary pattern tests into their own file + grep tests: move "grep binary" alongside the rest + grep: inline the return value of a function call used only once + t4210: skip more command-line encoding tests on MinGW + grep: don't use PCRE2?_UTF8 with "log --encoding=" + log tests: test regex backends in "--encode=" tests (this branch is used by cb/pcre1-cleanup.) A few simplification and bugfixes to PCRE interface. * ah/cleanups (2019-10-03) 4 commits (merged to 'next' on 2019-10-04 at 1345f09afb) + git_mkstemps_mode(): replace magic numbers with computed value + wrapper: use a loop instead of repetitive statements + diffcore-break: use a goto instead of a redundant if statement + commit-graph: remove a duplicate assignment Miscellaneous code clean-ups. * am/t0028-utf16-tests (2019-09-28) 2 commits (merged to 'next' on 2019-10-09 at 453900a4e8) + t0028: add more tests + t0028: fix test for UTF-16-LE-BOM Test fixes. * am/visual-studio-config-fix (2019-09-28) 1 commit (merged to 'next' on 2019-10-04 at 135d93143b) + contrib/buildsystems: fix Visual Studio Debug configuration Dev support. * as/shallow-slab-use-fix (2019-10-02) 1 commit (merged to 'next' on 2019-10-04 at f3a22d2b18) + shallow.c: don't free unallocated slabs Correct code that tried to reference all entries in a sparse array of pointers by mistake. * bc/object-id-part17 (2019-08-19) 26 commits (merged to 'next' on 2019-10-04 at b0460b0db2) + midx: switch to using the_hash_algo + builtin/show-index: replace sha1_to_hex + rerere: replace sha1_to_hex + builtin/receive-pack: replace sha1_to_hex + builtin/index-pack: replace sha1_to_hex + packfile: replace sha1_to_hex + wt-status: convert struct wt_status to object_id + cache: remove null_sha1 + builtin/worktree: switch null_sha1 to null_oid + builtin/repack: write object IDs of the proper length + pack-write: use hash_to_hex when writing checksums + sequencer: convert to use the_hash_algo + bisect: switch to using the_hash_algo + sha1-lookup: switch hard-coded constants to the_hash_algo + config: use the_hash_algo in abbrev comparison + combine-diff: replace GIT_SHA1_HEXSZ with the_hash_algo + bundle: switch to use the_hash_algo + connected: switch GIT_SHA1_HEXSZ to the_hash_algo + show-index: switch hard-coded constants to the_hash_algo + blame: remove needless comparison with GIT_SHA1_HEXSZ + builtin/rev-parse: switch to use the_hash_algo + builtin/blame: switch uses of GIT_SHA1_HEXSZ to the_hash_algo + builtin/receive-pack: switch to use the_hash_algo + fetch-pack: use parse_oid_hex + patch-id: convert to use the_hash_algo + builtin/replace: make hash size independent Preparation for SHA-256 upgrade continues. * cb/pcre1-cleanup (2019-08-26) 2 commits (merged to 'next' on 2019-10-04 at a2dd896ee8) + grep: refactor and simplify PCRE1 support + grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 (this branch uses ab/pcre-jit-fixes.) PCRE fixes. * dl/format-patch-doc-test-cleanup (2019-10-09) 1 commit (merged to 'next' on 2019-10-11 at 992da06f37) + t4014: treat rev-list output as the expected value (this branch is used by dl/format-patch-cover-from-desc.) test cleanup. * dl/octopus-graph-bug (2019-10-04) 5 commits (merged to 'next' on 2019-10-07 at c6bc2fe4a0) + t4214: demonstrate octopus graph coloring failure + t4214: explicitly list tags in log + t4214: generate expect in their own test cases + t4214: use test_merge + test-lib: let test_merge() perform octopus merges "git log --graph" for an octopus merge is sometimes colored incorrectly, which is demonstrated and documented but not yet fixed. * dl/rev-list-doc-cleanup (2019-10-06) 1 commit (merged to 'next' on 2019-10-07 at
Re: ds/sparse-cone, was Re: What's cooking in git.git (Oct 2019, #03; Fri, 11)
Eric Wong writes: > I just took a brief look, but that appears to leak memory. > > "hashmap_free(var, 1)" should be replaced with > "hashmap_free_entries(var, struct foo, member)" > > Only "hashmap_free(var, 0)" can become "hashmap_free(var)" I deliberately avoided merge-time band-aid fixups on this topic and ew/hashmap exactly because I was sure that I'd introduce a similar bugs by doing so myself. Using evil merges can be a great way to help multiple topics polished independently at the same time, but when overused, can hide this kind of gotchas quite easily. A reroll on top of ew/hashmap would be desirable, now that topic is ready for 'master'. Thanks.
Re: [RFC PATCH 1/1] Teach remote add the --prefix-tags option
Wink Saville writes:
> When --prefix-tags is passed to `git remote add` the tagopt is set to
> --prefix-tags and a second fetch line is added so tags are placed in
> a separate hierarchy per remote.
In the olden days, there was no refs/remotes/$remoteName/ hiearchy,
and until we made it the default at around Git 1.5.0, such a modern
layout for the branches were called the "separate remote" layout,
and can be opted into with "clone --use-separate-remote" by early
adopters.
I doubt that use of refs/tags/$remoteName/ is a good design if we
want to achieve similar isolation between local tags and and tags
obtained from each remote.
An obvious alternative, refs/remotes/$remoteName/tags/, is not a
good design for exactly the same reason. You cannot tell between a
local tag foo/bar and a tag bar obtained from remote foo when you
see refs/tags/foo/bar, and you cannot tell between a branch tag/bar
obtained from remote foo and a tag bar obtained from remote foo when
you see refs/remotes/foo/tags/bar. In the past, people suggested to
use refs/remoteTags/$remoteName/ for proper isolation, and it might
be a better middle-ground than either of the two, at least in the
shorter term, but not ideal.
In short, if you truly want to see "separate hierarchy per remote",
you should consider how you can reliably implement an equivalent of
"git branch --list --remote"; a design that does not allow it is a
failure.
A better solution with longer lifetime would probably be to use
refs/remotes/$remoteName/{heads,tags,...}/
when core.useTotallySeparateRemote configuration exists (and
eventually at Git 3.0 make the layout the default). It would
involve changes in the refname look-up rules, but it would not have
to pollute refs/ namespace like the refs/remoteTags/ half-ground
design, which would require us to add refs/remoteNotes/ and friends,
who knows how many more we would end up having to support if we go
that route.
Thanks.
Re: [PATCH v5 3/3] format-patch: teach --cover-from-description option
Denton Liu writes:
> diff --git a/builtin/log.c b/builtin/log.c
> index d212a8305d..af33fe9ffb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1057,6 +1072,44 @@ static void show_diffstat(struct rev_info *rev,
> fprintf(rev->diffopt.file, "\n");
> }
>
> +static void pp_from_desc(struct pretty_print_context *pp,
> + const char *branch_name,
> + struct strbuf *sb,
> + const char *encoding,
> + int need_8bit_cte)
> +{
> + const char *subject = "*** SUBJECT HERE ***";
> + const char *body = "*** BLURB HERE ***";
> + struct strbuf description_sb = STRBUF_INIT;
> + struct strbuf subject_sb = STRBUF_INIT;
> +
> + if (cover_from_description_mode == COVER_FROM_NONE)
> + goto do_pp;
> +
> + if (branch_name && *branch_name)
> + read_branch_desc(&description_sb, branch_name);
> + if (!description_sb.len)
> + goto do_pp;
> +
> + if (cover_from_description_mode == COVER_FROM_SUBJECT ||
> + cover_from_description_mode == COVER_FROM_AUTO)
> + body = format_subject(&subject_sb, description_sb.buf, " ");
> +
> + if (cover_from_description_mode == COVER_FROM_MESSAGE ||
> + (cover_from_description_mode == COVER_FROM_AUTO &&
> + subject_sb.len > COVER_FROM_AUTO_MAX_SUBJECT_LEN))
> + body = description_sb.buf;
> + else
> + subject = subject_sb.buf;
> +
> +do_pp:
> + pp_title_line(pp, &subject, sb, encoding, need_8bit_cte);
> + pp_remainder(pp, &body, sb, 0);
> +
> + strbuf_release(&description_sb);
> + strbuf_release(&subject_sb);
> +}
> +
This implementation is very clear and easy to follow, and ...
> @@ -1064,8 +1117,6 @@ static void make_cover_letter(struct rev_info *rev, int
> use_stdout,
> int quiet)
> {
> const char *committer;
> struct shortlog log;
> struct strbuf sb = STRBUF_INIT;
> int i;
> @@ -1095,15 +1146,12 @@ static void make_cover_letter(struct rev_info *rev,
> int use_stdout,
> if (!branch_name)
> branch_name = find_branch_name(rev);
>
> pp.fmt = CMIT_FMT_EMAIL;
> pp.date_mode.type = DATE_RFC2822;
> pp.rev = rev;
> pp.print_email_subject = 1;
> pp_user_info(&pp, NULL, &sb, committer, encoding);
> + pp_from_desc(&pp, branch_name, &sb, encoding, need_8bit_cte);
> fprintf(rev->diffopt.file, "%s\n", sb.buf);
>
> strbuf_release(&sb);
... made the caller much simpler.
One large nit is that pp_user_info() is about "pretty printing the
user info", pp_title_line() is about "pretty printing the title
line", but pp_from_desc() is not about "pretty printing the from
desc". Naming matters.
How about calling it with more emphasis on what it does (i.e. the
helper is about preparing the subject and body of the cover letter
e-mail) and less emphasis on how it does or what it bases its
decision on? prepare_cover_text() or soemthing, perhaps?
Other than that, this version is very much more preferrable than the
previous one. Quite nicely done.
Thanks.
Re: [PATCH v5 1/3] format-patch: change erroneous and condition
Denton Liu writes:
> Since this
> seems to be a Python-ism that's mistakenly leaked into our code, convert
The conclusion is OK, but as the inventor of format-patch and a
non-pythonista, I do not think the above claim is correct, and even
if Thomas thought it was a good idea to follow Python style in
30984ed2 ("format-patch: support deep threading", 2009-02-19), which
I doubt he did, I do not think there is much point in speculating.
Both the log message and the patch text in the previous round were
better than this round, I would have to say.
Thanks.
> diff --git a/builtin/log.c b/builtin/log.c
> index 44b10b3415..351f4ffcfd 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -835,7 +835,7 @@ static int git_format_config(const char *var, const char
> *value, void *cb)
> thread = THREAD_SHALLOW;
> return 0;
> }
> - thread = git_config_bool(var, value) && THREAD_SHALLOW;
> + thread = git_config_bool(var, value) ? THREAD_SHALLOW :
> THREAD_UNSET;
> return 0;
> }
> if (!strcmp(var, "format.signoff")) {
Re: [PATCH v2 1/1] doc: Change zsh git completion file name
SZEDER Gábor writes: >> -# The recommended way to install this script is to copy to '~/.zsh/_git', >> and >> -# then add the following to your ~/.zshrc file: >> +# The recommended way to install this script is to make a copy of it in >> +# ~/.zsh/ directory as ~/.zsh/git-completion.zsh and then add the following > > These updated instructions don't work for me, following them gives me > zsh's git completion instead of ours: > ... > Instructing users to copy our completion script to > '~/.zsh/git-completion.zsh' goes against this convention. > More importantly, it appears that this is more than a mere convention, > maybe a requirement even; at least renaming our zsh completion script > to follow the convention (IOW following the current install > instructions) makes it work all of a sudden: Thanks for a dose of sanity. My "Helped-by" was about phrasing and had nothing with the name of the file, so it should be fixable without invalidating it ;-) > -# The recommended way to install this script is to copy to '~/.zsh/_git', and > -# then add the following to your ~/.zshrc file: > +# The recommended way to install this script is to make a copy of it in > +# ~/.zsh/ directory as ~/.zsh/git-completion.zsh and then add the following # The recommended way to install this script is to make a copy of it in # '~/.zsh/' directory as '~/.zsh/_git' and then add the following
Re: [PATCH 1/1] Improve Japanese translation
"kdnakt via GitGitGadget" writes: > @@ -661,7 +662,7 @@ msgstr "" > #: lib/merge.tcl:108 > #, tcl-format > msgid "%s of %s" > -msgstr "%s の %s ブランチ" > +msgstr "%2$s の %1$s ブランチ" > > #: lib/merge.tcl:122 > #, tcl-format > @@ -956,7 +957,7 @@ msgstr "エラー: コマンドが失敗しました" > #: lib/checkout_op.tcl:85 > #, tcl-format > msgid "Fetching %s from %s" > -msgstr "%s から %s をフェッチしています" > +msgstr "%2$s から %1$s をフェッチしています" Both of these changes to word order make sense. It's a bit surprising that these haven't been noticed/fixed for quite some time ;-).
Re: [PATCH] http-push: simplify deleting a list item
René Scharfe writes:
> The first step for deleting an item from a linked list is to locate the
> item preceding it. Be more careful in release_request() and handle an
> empty list. This only has consequences for invalid delete requests
> (removing the same item twice, or deleting an item that was never added
> to the list), but simplifies the loop condition as well as the check
> after the loop.
>
> Once we found the item's predecessor in the list, update its next
> pointer to skip over the item, which removes it from the list. In other
> words: Make the item's successor the successor of its predecessor.
> (At this point entry->next == request and prev->next == lock,
> respectively.) This is a bit simpler and saves a pointer dereference.
>
> Signed-off-by: René Scharfe
> ---
> http-push.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
Nice simplification. I wonder how much longer we should be
maintaining this program, though;-)
Will queue.
>
> diff --git a/http-push.c b/http-push.c
> index 0353f9f514..822f326599 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -501,10 +501,10 @@ static void release_request(struct transfer_request
> *request)
> if (request == request_queue_head) {
> request_queue_head = request->next;
> } else {
> - while (entry->next != NULL && entry->next != request)
> + while (entry && entry->next != request)
> entry = entry->next;
> - if (entry->next == request)
> - entry->next = entry->next->next;
> + if (entry)
> + entry->next = request->next;
> }
>
> free(request->url);
> @@ -981,7 +981,7 @@ static int unlock_remote(struct remote_lock *lock)
> while (prev && prev->next != lock)
> prev = prev->next;
> if (prev)
> - prev->next = prev->next->next;
> + prev->next = lock->next;
> }
>
> free(lock->owner);
> --
> 2.23.0
Re: [PATCH v4] stash: avoid recursive hard reset on submodules
Jakob Jarmar writes: > git stash push does not recursively stash submodules, but if > submodule.recurse is set, it may recursively reset --hard them. Having > only the destructive action recurse is likely to be surprising > behaviour, and unlikely to be desirable, so the easiest fix should be to > ensure that the call to git reset --hard never recurses into submodules. > > This matches the behavior of check_changes_tracked_files, which ignores > submodules. > > Signed-off-by: Jakob Jarmar > --- > > Notes: > 1. Added space between function name and parentheses > 2. Moved test_when_finished cleanup to top of setup_basic Looks good; will queue. Thanks.
Re: [PATCH v4 0/3] format-patch: learn --cover-from-description option
Denton Liu writes: > Changes since v3: > > * Change --infer-cover-subject to --cover-from-description > > * No more test cleanup patches (they were merged in > 'dl/format-patch-doc-test-cleanup') With these patches, t4013 and t9902 seem to break, when queued on top of dl/format-patch-doc-test-cleanup and also when merged to 'pu'. Thanks.
Re: [PATCH v4 3/3] format-patch: teach --cover-from-description option
Denton Liu writes:
> +format.coverFromDescription::
> + The default mode for format-patch to determine which parts of
> + the cover letter will be populated using the branch's
> + description. See the `--cover-from-description` option in
> + linkgit:git-format-patch[1].
> +
> format.signature::
> The default for format-patch is to output a signature containing
> the Git version number. Use this variable to change that default.
> diff --git a/Documentation/git-format-patch.txt
> b/Documentation/git-format-patch.txt
> index 0ac56f4b70..86114e4c22 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -19,6 +19,7 @@ SYNOPSIS
> [--start-number ] [--numbered-files]
> [--in-reply-to=] [--suffix=.]
> [--ignore-if-in-upstream]
> +[--cover-from-description=]
> [--rfc] [--subject-prefix=]
> [(--reroll-count|-v) ]
> [--to=] [--cc=]
> @@ -171,6 +172,26 @@ will want to ensure that threading is disabled for `git
> send-email`.
> patches being generated, and any patch that matches is
> ignored.
>
> +--cover-from-description=::
> + Controls which parts of the cover letter will be automatically
> + populated using the branch's description.
> ++
> +If `` is `message` or `default`, the cover letter subject will be
> +populated with placeholder text. The body of the cover letter will be
> +populated with the branch's description.
I understand that this is what we do now, so those who want to live
in the past can set the configuration variable to 'message'.
> +If `` is `subject`, the beginning of the branch description (up to
> +the first blank line) will populate the cover letter subject. The
> +remainder of the description will populate the body of the cover
> +letter.
s/the beginning of .*blank line)/the first paragraph of the branch description/
may be shorter, but the above is OK, too.
When description is prepared appropriately, this mode would fill
both subject and body, which sounds sensible.
> +If `` is `auto`, if the beginning of the branch description (up to
> +the first line) is greater than 100 characters then the mode will be
> +`message`, otherwise `subject` will be used.
I understand that this is a more clever and safer variant of
'subject'. Do you want to say 100 characters or 100 bytes?
> +If `` is `none`, both the cover letter subject and body will be
> +populated with placeholder text.
OK, this is done for completeness? I wonder who finds it useful to
set it to 'none' *AND* set the branch description. Not a rhetorical
question that suggests removing this choice, but purely soliciting
opinions from others.
It is unclear (other than the mode word being 'default' for one of
the choices) what the new default mode of operation is after the
patch is applied among the four presented mode. "This is the
default when no configuration nor command line option specifies the
desired mode" or something may want to be added to one of these
paragraphs.
> @@ -1061,13 +1076,16 @@ static void make_cover_letter(struct rev_info *rev,
> int use_stdout,
> struct commit *origin,
> int nr, struct commit **list,
> const char *branch_name,
> + enum cover_from_description
> cover_from_description_mode,
> int quiet)
> {
> const char *committer;
> - const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
> - const char *msg;
> + const char *subject = "*** SUBJECT HERE ***";
> + const char *body = "*** BLURB HERE ***";
> struct shortlog log;
> struct strbuf sb = STRBUF_INIT;
> + struct strbuf description_sb = STRBUF_INIT;
> + struct strbuf subject_sb = STRBUF_INIT;
> int i;
> const char *encoding = "UTF-8";
> int need_8bit_cte = 0;
> @@ -1095,17 +1113,34 @@ static void make_cover_letter(struct rev_info *rev,
> int use_stdout,
> if (!branch_name)
> branch_name = find_branch_name(rev);
>
> - msg = body;
> + if (branch_name && *branch_name)
> + read_branch_desc(&description_sb, branch_name);
It may not matter in practice but strictly speaking there is no need
to read the description if we know that the mode is NONE. Removing
the support for the NONE mode may be an easier fix than adding "&&
mode != NONE" to the if () condition guarding this call---I dunno.
> + if (cover_from_description_mode != COVER_FROM_NONE &&
> description_sb.len) {
> + if (cover_from_description_mode == COVER_FROM_SUBJECT ||
> + cover_from_description_mode == COVER_FROM_AUTO)
> + body = format_subject(&subject_sb, description_sb.buf,
> " ");
> +
> + if (cover_from_description_mode == COVER_FROM_MESSAGE ||
> +
Re: [PATCH v4 2/3] format-patch: use enum variables
Denton Liu writes:
> -#define THREAD_SHALLOW 1
> -#define THREAD_DEEP 2
> -static int thread;
> +enum thread_level {
> + THREAD_UNSET,
> + THREAD_SHALLOW,
> + THREAD_DEEP
> +};
> +static enum thread_level thread;
As the assignment of values do not change, this is a safe conversion
even if an existing code did things like
if (!thread)
... do this ...;
Hopefully nobody did arithmetic on it; thread_level may now become
unsigned depending on the compiler.
> -static int config_cover_letter;
> +static enum cover_setting config_cover_letter;
Likewise about the signedness.
Re: [PATCH v2] utf8: use ARRAY_SIZE() in git_wcwidth()
Beat Bolli writes: > diff --git a/utf8.c b/utf8.c > index 3b42fadffd..5c8f151f75 100644 > --- a/utf8.c > +++ b/utf8.c > @@ -95,13 +95,11 @@ static int git_wcwidth(ucs_char_t ch) > return -1; > > /* binary search in table of non-spacing characters */ > - if (bisearch(ch, zero_width, sizeof(zero_width) > - / sizeof(struct interval) - 1)) > + if (bisearch(ch, zero_width, ARRAY_SIZE(zero_width) - 1)) I wondered if we want a wrapper similar to QSORT() macro so that we do not have to repeat zero_width (and double_width below) like this, but bisearch() itself is local to this file and there are only these two callers, so it probably is not worth it. The original is waiting for a bug when zero_width changes its type, by not dividing with sizeof(*zero_width). The use of ARRAY_SIZE() is quite appropriate here. Thanks. > return 0; > > /* binary search in table of double width characters */ > - if (bisearch(ch, double_width, sizeof(double_width) > - / sizeof(struct interval) - 1)) > + if (bisearch(ch, double_width, ARRAY_SIZE(double_width) - 1)) > return 2; > > return 1;
Re: [PATCH v3] stash: avoid recursive hard reset on submodules
Jakob Jarmar writes:
> diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
> index d7219d6f8f..83106fa958 100755
> --- a/t/t3906-stash-submodule.sh
> +++ b/t/t3906-stash-submodule.sh
> @@ -1,6 +1,6 @@
> #!/bin/sh
>
> -test_description='stash apply can handle submodules'
> +test_description='stash can handle submodules'
Good attention to the detail ;-)
> +setup_basic() {
Style. SP on both sides of () in our shell scripts (as seen in the
existing shell function in the same file).
> + git init sub &&
> + (
> + cd sub &&
> + test_commit sub_file
> + ) &&
> + git init main &&
> + (
> + cd main &&
> + git submodule add ../sub &&
> + test_commit main_file
> + ) &&
> + test_when_finished "rm -rf main sub"
Have test_when_finished that removes main and sub _before_ you start
creating sub and main.
When the &&-cascade breaks anywhere, the control may not even reach
your test_when_finished that registers the clean-up procedure.
Imagine "git init sub" succeeds but "git init main" somehow
fails---you still want to clean up "sub".
Other than that, looks reasonably well done.
Thanks for working on this.
> +}
> +
> +test_expect_success 'stash push with submodule.recurse=true preserves dirty
> submodule worktree' '
> + setup_basic &&
> + (
> + cd main &&
> + git config submodule.recurse true &&
> + echo "x" >main_file.t &&
> + echo "y" >sub/sub_file.t &&
> + git stash push &&
> + test_must_fail git -C sub diff --quiet
> + )
> +'
> +
> +test_expect_success 'stash push and pop with submodule.recurse=true
> preserves dirty submodule worktree' '
> + setup_basic &&
> + (
> + cd main &&
> + git config submodule.recurse true &&
> + echo "x" >main_file.t &&
> + echo "y" >sub/sub_file.t &&
> + git stash push &&
> + git stash pop &&
> + test_must_fail git -C sub diff --quiet
> + )
> +'
> +
> test_done
Re: [PATCH v2 1/1] doc: Change zsh git completion file name
"Maxim Belsky via GitGitGadget" writes: > Signed-off-by: Maxim Belsky > Helped-by: Johannes Schindelin > Helped-by: Junio C Hamano No need to resend (as I'll fix it up locally while queuing), but your sign-off comes last, as we keep these things chronological. With help from others, you wrote the patch and finally signed it off to be applied. Thanks for working on this.
Re: [PATCH v3 1/1] fsmonitor: don't fill bitmap with entries to be removed
"William Baker via GitGitGadget" writes: > +# Test staging/unstaging files that appear at the end of the index. Test > +# file names begin with 'z' so that they are sorted to the end of the index. Well, the test is now done in a freshly created repository, so the z* files are the only thing you have in here---technically they are at the end of the index, but so they are at the beginning, too. Would it affect the effectiveness of the test that you do not have any other paths in the working tree or in the index, unlike the test in the previous rounds that did not use a newly created test repository? This is not a rhetorical question, but purely asking. "no, this still tests what we want to test and shows breakage when the fix to the code in the patch gets reverted" is perfectly a good answer, but in that case, is "the end of" the most important trait of the condition this test is checking? Wouldn't the bug be exposed as long as we remove sufficiently large number of entries (like "removing more paths than the paths still in the index at the end" or something like that)? Thanks. > +test_expect_success 'status succeeds after staging/unstaging ' ' > + test_create_repo fsmonitor-stage-unstage && > + ( > + cd fsmonitor-stage-unstage && > + test_commit initial && > + git update-index --fsmonitor && > + removed=$(test_seq 1 100 | sed "s/^/z/") && > + touch $removed && > + git add $removed && > + git config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-env" > && > + FSMONITOR_LIST="$removed" git restore -S $removed && > + FSMONITOR_LIST="$removed" git status > + ) > +' > + > test_done > diff --git a/t/t7519/fsmonitor-env b/t/t7519/fsmonitor-env > new file mode 100755 > index 00..8f1f7ab164 > --- /dev/null > +++ b/t/t7519/fsmonitor-env > @@ -0,0 +1,24 @@ > +#!/bin/sh > +# > +# An test hook script to integrate with git to test fsmonitor. > +# > +# The hook is passed a version (currently 1) and a time in nanoseconds > +# formatted as a string and outputs to stdout all files that have been > +# modified since the given time. Paths must be relative to the root of > +# the working tree and separated by a single NUL. > +# > +#echo "$0 $*" >&2 > + > +if test "$#" -ne 2 > +then > + echo "$0: exactly 2 arguments expected" >&2 > + exit 2 > +fi > + > +if test "$1" != 1 > +then > + echo "Unsupported core.fsmonitor hook version." >&2 > + exit 1 > +fi > + > +printf '%s\n' $FSMONITOR_LIST
Re: Adding a line after the signed-off git am -s
Daniel Lezcano writes:
> I would like to do something:
>
> git am -s -l "Link: https://lore.kernel.org/r/"
>
> Which will give:
>
> blabla
>
> Signed-off-by: aut...@kairnail.org
> Signed-off-by: commi...@kairnail.org
> Link: https://lore.kernel.org/r/
>
> This way it is compatible with patchwork, git-pw, etc...
There is the post-applypatch hook you can define after the patch
gets applied and produces a commit.
I use it to maintain the amlog notes in my repository (iow, I do not
amend the commit, but add notes to the resulting commit so that I
can tell, given a commit, which message resulted in it).
If you want to amend the resulting commit instead, the place to do
so would be where I call "git notes --ref amlog" in the sample
script.
-- >8 -- post-applypatch hook example -- >8 --
#!/bin/sh
GIT_DIR=.git
dotest="$GIT_DIR/rebase-apply"
prec=4 &&
this=$(cat 2>/dev/null "$dotest/next") &&
msgnum=$(printf "%0${prec}d" $this) &&
test -f "$dotest/$msgnum" &&
message_id=$(sed -ne '
/^[ ]/{
# Append continuation line to hold space
H
# Swap hold and pattern
x
# Remove the LF, making it a single line
s/\n//
# Swap hold and pattern back
x
# Discard the pattern and go on
n
}
# Hold this new line, and look at what is in the hold space
x
# Is it the Message-ID line? If so, spit it out and finish.
/^[Mm][Ee][Ss][Ss][Aa][Gg][Ee]-[Ii][Dd]:[ ]*/{
s///p
q
}
# Otherwise, check if this new line is empty
x
# Is it? Then we are done with the header
/^$/b end
# Otherwise we need to hold onto this header line
x
# And start the next cycle
b
: end
# ??? do we want to check if we held onto the last message-id line
# and process it here if we did???
q
' "$dotest/$msgnum") &&
if test -n "$message_id" &&
head=$(git rev-parse --verify HEAD 2>/dev/null)
then
echo "$head $message_id" >>"$GIT_DIR"/am.log &&
git notes --ref amlog add -f -m "Message-Id: $message_id" "$head"
fi
Re: What's cooking in git.git (Oct 2019, #03; Fri, 11)
Junio C Hamano writes: > By the way, I think I made a mistake in my calendar math. > > This topic was merged to 'next' on the 7th and it is not especially > tricky; unless I (or somebody else) find glaring issues in it during > the final sanity check before merging it to 'master' during the next > batch, it would take the normal course to 'master' before the 18th, > on which the rc0 is planned. Unless I lose power or net due to typhoon, I plan to examine topics that were merged to 'next' to see which ones should be part of the next batch over the weekend. This will be among those topics. Thanks.
Re: [PATCH v2] send-pack: never fetch when checking exclusions
Jeff King writes:
> As a general rule (and why I'm raising this issue in reply to Jonathan's
> patch), I think most or all sites that want OBJECT_INFO_QUICK will want
> SKIP_FETCH_OBJECT as well, and vice versa. The reasoning is generally
> the same:
>
> - it's OK to racily have a false negative (we'll still be correct, but
> possibly a little less optimal)
>
> - it's expected and normal to be missing the object, so spending time
> double-checking the pack store wastes measurable time in real-world
> cases
31f5256c ("sha1-file: split OBJECT_INFO_FOR_PREFETCH", 2019-05-28)
separated SKIP_FETCH_OBJECT out of FOR_PREFETCH, the latter of which
was and is SKIP_FETCH and QUICK combined. Use SKIP_FETCH_OBJECT
alone may need to be re-examined and discouraged?
Re: [Outreachy] [PATCH] blame: Convert pickaxe_blame defined constants to enums
Jonathan Tan writes: >> > Also, I have a slight preference for putting "= 02" on the BLAME_COPY >> > line but that is not necessary. >> >> That is absolutely necessary; it is not like "we do not care what >> exact value _COPY gets; it can be any value as long as it is _MOVE >> plus 1", as these are used in set of bits (and again, I do not think >> it is such a brilliant idea to use enum for such a purpose). > > Good point. Doesn't that also show that enums are not quite a good fit for set of bits (i.e. 1<
Re: [PATCH 01/11] graph: automatically track visible width of `strbuf`
James Coglan writes:
> - We don't want a general solution to this problem for everything
> `strbuf` could be used for; it only needs to address the graph
> padding problem.
Of course. Somebody may use strbuf to hold rows of a table and you
do not want to contaminate strbuf with fields like width of each
column etc, that are very specific to the application. IOW, strbuf
is merely _one_ component of a larger solution to each specific
problem, and the latter may be things like "struct graphbuf" like
Dscho suggested, which might use strbuf as an underlying
tuple mechanism, but that is an implementation
detail that should not be exposed to the users of the struct (and
that is why he did not call, and you should not call, the data
structure "graph-strbuf" or anything with "strbuf").
> - We only want to count printing characters, not color formatting sequences.
OK. But I'd phrase "count printing characters" as "measure display
width" for at least two reasons. Whitespaces are typically counted
as non-printing, but you do want to take them into account for this
application. Also the graph may not be limited to ASCII graphics
forever, and byte- or character-count may not match display width on
a fixed-width display.
> - We only need to consider the width of a small set of characters:
> { | / \ _ - . * }. We don't need to worry about Unicode, and the
> simple character counting in graph.c was working fine.
I have to warn you that we saw attempts to step outside these ASCII
graphics and use Unicode characters for prettier output in the past.
If you can do so without too much complexity, I'd suggest you try
not to close the door to those people who follow your footsteps to
further improve the system by pursuing the avenue further.
> To this end I've prepared a different implementation that
> introduces the indirection described above, and does not modify
> `strbuf`:
>
> +struct graph_strbuf {
> + struct strbuf *buf;
> + size_t width;
> +};
Is there a reason why you need a pointer to a strbuf that is
allocated separately? E.g. would it make it harder to manage
if the above were
struct graphbuf {
struct strbuf buf;
int width; /* display width in columns */
};
which is essentially what Dscho suggested?
> +static inline void graph_strbuf_addch(struct graph_strbuf *sb, int c)
> +{
> + strbuf_addch(sb->buf, c);
> + sb->width++;
> +}
> +
> +void graph_strbuf_addchars(struct graph_strbuf *sb, int c, size_t n)
> +{
> + strbuf_addchars(sb->buf, c, n);
> + sb->width += n;
> +}
> +
> +static inline void graph_strbuf_addstr(struct graph_strbuf *sb, const char
> *s)
> +{
> + strbuf_addstr(sb->buf, s);
> + sb->width += strlen(s);
> +}
I'd probably introduce another helper that takes color code and
graphbuf (also notice how I name the variables and types; calling
something sb that is not a strbuf is misleading and inviting
unnecessary bugs):
static inline void graphbuf_addcolor(struct graphbuf *gb, unsigned short
color)
{
strbuf_addstr(gb->buf, column_get_color_code(color));
}
if I were writing this code. The goal is to make any strbuf_add*()
that is done directly on gb->buf outside these helpers a bug--that
way, grepping for strbuf_add* in this file would become a very easy
way to catch such a bug that bypasses the graphbuf_add*() layer and
potentially screw up the column counting.
Other than these three points (i.e. naming without "strbuf" that is
an implementation detail, embedded vs pointer of strbuf in the
graphbuf, and making sure everything can be done via graphbuf_add*()
wrappers to make it easier to spot bugs), it seems you are going in
the right direction. Thanks for working on this.
Re: [RFC PATCH 10/10] pack-objects: improve partial packfile reuse
Jeff King writes: > The current code does so by creating a new entry in the reused_chunks > array. In the worst case that can grow to have the same number of > entries as we have objects. So this code was an attempt to pad the > header of a shrunken entry to keep it the same size. I don't remember > all the problems we ran into with that, but in the end we found that it > didn't actually help much, because in practice we don't end up with a > lot of chunks anyway. Hmm, I am kind of surprised that the decoding side allowed such a padding. > For instance, packing torvalds/linux on our servers just now reused 6.5M > objects, but only needed ~50k chunks. OK, that's a good argument for not trying to optimize. > I think the original code may simply have been buggy and nobody noticed. > Here's what I wrote when this line was added in our fork: > > pack-objects: check reused pack bitmap for duplicate objects > > If a client both asks for a tag by sha1 and specifies > "include-tag", we may end up including the tag in the reuse > bitmap (due to the first thing), and then later adding it to > the packlist (due to the second). This results in duplicate > objects in the pack, which git chokes on. We should notice > that we are already including it when doing the include-tag > portion, and avoid adding it to the packlist. > > The simplest place to fix this is right in add_ref_tag, > where we could avoid peeling the tag at all if we know that > we are already including it. However, I've pushed the check > instead into have_duplicate_entry. This fixes not only this > case, but also means that we cannot have any similar > problems lurking in other code. > > No tests, because git does not actually exhibit this "ask > for it and also include-tag" behavior. We do one or the > other on clone, depending on whether --single-branch is set. > However, libgit2 does both. > > I'm not sure why we didn't notice it with the older reuse code. It may > simply have been that it hardly ever triggered on our servers. Impressed by the careful thinking here. Thanks.
Re: What's cooking in git.git (Oct 2019, #03; Fri, 11)
Elijah Newren writes: > Did I shoot myself in the foot by being quick to jump on Rene's couple > of cosmetic touch-up suggestions he posted over a week after the > series was originally posted? If the suggested updates were simple enough to do and would improve the result sufficiently (which is probably true in this case), it is possible that the topic would have been marked "Expecting a reroll" and stayed out of 'next' longer. Or I may have missed the suggested updates before merging the known-incomplete series to 'next', and the topic may have got marked "Expecting a follow-up" and stayed in that state until the update happened. Or I may have simply missed the suggestion and allowed you to successfully game the system. So you have 2/3 chance of delaying the topic's graduation by doing so ;-). But over time in the longer run, those contributors who prioritize quality over haste are rewarded with more trust (which results in shorter gestation period for their topics in both 'pu' and 'next'), while those contributors with tendency to sneakily gaming the system eventually get caught and their contribution has to be looked at with finer toothed comb than usual, making more work on everybody and delaying the whole thing, including but not limited to their topics. At least, that is how the system hopefully works. By the way, I think I made a mistake in my calendar math. This topic was merged to 'next' on the 7th and it is not especially tricky; unless I (or somebody else) find glaring issues in it during the final sanity check before merging it to 'master' during the next batch, it would take the normal course to 'master' before the 18th, on which the rc0 is planned.
Re: What's cooking in git.git (Oct 2019, #03; Fri, 11)
Elijah Newren writes: >> * en/fast-imexport-nested-tags (2019-10-04) 8 commits >> (merged to 'next' on 2019-10-07 at 3e75779e10) >> + fast-export: handle nested tags >> ... >> + fast-export: fix exporting a tag and nothing else >> >> Updates to fast-import/export. >> >> Will merge to 'master'. > > Any chance this will merge down before 2.24.0? I'd really like to see > and use it within filter-repo. A few general guidelines I use are that a typical topic spends 1 week in 'next' (a trivial one-liner may be there for much shorter time, an involved multi-patch topic that touches the core part of the system may have to spend more), that an involved topic that is not in 'master' by rc0 would not appear in the next release, and that any topic that is not in 'master' by rc1 needs compelling reason to be in the next release. So it is cutting a bit too close for this topic, it seems, but we'll see.
What's cooking in git.git (Oct 2019, #03; Fri, 11)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. We may want to merge down fixes accumulated on the 'master' front to 'maint', to prepare for v2.23.1 soonish. I'll look into it over the weekend. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ab/pcre-jit-fixes (2019-08-19) 18 commits (merged to 'next' on 2019-10-04 at 2d55f2b470) + grep: under --debug, show whether PCRE JIT is enabled + grep: do not enter PCRE2_UTF mode on fixed matching + grep: stess test PCRE v2 on invalid UTF-8 data + grep: create a "is_fixed" member in "grep_pat" + grep: consistently use "p->fixed" in compile_regexp() + grep: stop using a custom JIT stack with PCRE v1 + grep: stop "using" a custom JIT stack with PCRE v2 + grep: remove overly paranoid BUG(...) code + grep: use PCRE v2 for optimized fixed-string search + grep: remove the kwset optimization + grep: drop support for \0 in --fixed-strings + grep: make the behavior for NUL-byte in patterns sane + grep tests: move binary pattern tests into their own file + grep tests: move "grep binary" alongside the rest + grep: inline the return value of a function call used only once + t4210: skip more command-line encoding tests on MinGW + grep: don't use PCRE2?_UTF8 with "log --encoding=" + log tests: test regex backends in "--encode=" tests (this branch is used by cb/pcre1-cleanup.) A few simplification and bugfixes to PCRE interface. * ah/cleanups (2019-10-03) 4 commits (merged to 'next' on 2019-10-04 at 1345f09afb) + git_mkstemps_mode(): replace magic numbers with computed value + wrapper: use a loop instead of repetitive statements + diffcore-break: use a goto instead of a redundant if statement + commit-graph: remove a duplicate assignment Miscellaneous code clean-ups. * am/visual-studio-config-fix (2019-09-28) 1 commit (merged to 'next' on 2019-10-04 at 135d93143b) + contrib/buildsystems: fix Visual Studio Debug configuration Dev support. * as/shallow-slab-use-fix (2019-10-02) 1 commit (merged to 'next' on 2019-10-04 at f3a22d2b18) + shallow.c: don't free unallocated slabs Correct code that tried to reference all entries in a sparse array of pointers by mistake. * bc/object-id-part17 (2019-08-19) 26 commits (merged to 'next' on 2019-10-04 at b0460b0db2) + midx: switch to using the_hash_algo + builtin/show-index: replace sha1_to_hex + rerere: replace sha1_to_hex + builtin/receive-pack: replace sha1_to_hex + builtin/index-pack: replace sha1_to_hex + packfile: replace sha1_to_hex + wt-status: convert struct wt_status to object_id + cache: remove null_sha1 + builtin/worktree: switch null_sha1 to null_oid + builtin/repack: write object IDs of the proper length + pack-write: use hash_to_hex when writing checksums + sequencer: convert to use the_hash_algo + bisect: switch to using the_hash_algo + sha1-lookup: switch hard-coded constants to the_hash_algo + config: use the_hash_algo in abbrev comparison + combine-diff: replace GIT_SHA1_HEXSZ with the_hash_algo + bundle: switch to use the_hash_algo + connected: switch GIT_SHA1_HEXSZ to the_hash_algo + show-index: switch hard-coded constants to the_hash_algo + blame: remove needless comparison with GIT_SHA1_HEXSZ + builtin/rev-parse: switch to use the_hash_algo + builtin/blame: switch uses of GIT_SHA1_HEXSZ to the_hash_algo + builtin/receive-pack: switch to use the_hash_algo + fetch-pack: use parse_oid_hex + patch-id: convert to use the_hash_algo + builtin/replace: make hash size independent Preparation for SHA-256 upgrade continues. * cb/pcre1-cleanup (2019-08-26) 2 commits (merged to 'next' on 2019-10-04 at a2dd896ee8) + grep: refactor and simplify PCRE1 support + grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 (this branch uses ab/pcre-jit-fixes.) PCRE fixes. * dl/rev-list-doc-cleanup (2019-10-06) 1 commit (merged to 'next' on 2019-10-07 at 712594feb1) + git-rev-list.txt: prune options in synopsis Doc update. * en/clean-nested-with-ignored (2019-10-02) 13 commits (merged to 'next' on 2019-10-03 at 969ec06cc7) + dir: special case check for the possibility that pathspec is NULL (merged to 'next' on 2019-09-30 at 778cc31eac) + clean: fix theoretical path corruption + clean: rewrap overly long line + clean: avoid removing untracked files in a nested git repository + clean: disambiguate the definition of -d + git-clean.txt: do not claim we will delete files with -n/--dry-run + dir: add commentary explaining match_pathspec_item's return value + dir: if our pathspec might match files under a dir, recurse into it + dir: make th
Re: [PATCH v2] stash: avoid recursive hard reset on submodules
Jakob Jarmar writes:
> git stash push does not recursively stash submodules, but if
> submodule.recurse is set, it may recursively reset --hard them. Having
> only the destructive action recurse is likely to be surprising
> behaviour, and unlikely to be desirable, so the easiest fix should be to
> ensure that the call to git reset --hard never recurses into submodules.
>
> This matches the behavior of check_changes_tracked_files, which ignores
> submodules.
Makes sense. Can we demonstrate an existing breakage and the fix in
a new test or two in perhaps t3906-stash-submodule.sh?
Thanks.
>
> Signed-off-by: Jakob Jarmar
> ---
>
> Sorry for sending a patch with messed up whitespace. This one should be
> correct.
>
> builtin/stash.c | 2 +-
> git-legacy-stash.sh | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index b5a301f24d..c986c258f2 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1383,7 +1383,7 @@ static int do_push_stash(const struct pathspec *ps,
> const char *stash_msg, int q
> struct child_process cp = CHILD_PROCESS_INIT;
> cp.git_cmd = 1;
> argv_array_pushl(&cp.args, "reset", "--hard", "-q",
> - NULL);
> + "--no-recurse-submodules", NULL);
> if (run_command(&cp)) {
> ret = -1;
> goto done;
> diff --git a/git-legacy-stash.sh b/git-legacy-stash.sh
> index f60e9b3e87..07ad4a5459 100755
> --- a/git-legacy-stash.sh
> +++ b/git-legacy-stash.sh
> @@ -370,7 +370,7 @@ push_stash () {
> git diff-index -p --cached --binary HEAD -- "$@" |
> git apply --index -R
> else
> - git reset --hard -q
> + git reset --hard -q --no-recurse-submodules
> fi
>
> if test "$keep_index" = "t" && test -n "$i_tree"
Re: [PATCH] pthread.h: manually align parameter lists
Denton Liu writes:
> In previous patches, extern was mechanically removed from function
> declarations without care to formatting, causing parameter lists to be
> misaligned. Manually format changed sections such that the parameter
> lists are realigned.
>
> Viewing this patch with 'git diff -w' should produce no output.
>
> Signed-off-by: Denton Liu
> ---
> I missed a step in 'dl/compat-cleanup'. This patch can be applied on the
> tip of that branch.
>
> compat/win32/pthread.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index f1cfe73de9..737983d00b 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -51,7 +51,7 @@ typedef struct {
> } pthread_t;
>
> int pthread_create(pthread_t *thread, const void *unused,
> - void *(*start_routine)(void*), void *arg);
> +void *(*start_routine)(void*), void *arg);
Yup, I missed that too. Thanks for a clean-up.
>
> /*
> * To avoid the need of copying a struct, we use small macro wrapper to pass
Re: [PATCH 1/1] doc: Change zsh git completion file name
Johannes Schindelin writes: > On Thu, 10 Oct 2019, Maxim Belsky via GitGitGadget wrote: > >> From: Maxim Belsky >> >> Signed-off-by: Maxim Belsky > > I totally agree with the patch, but only because I read a couple of > reports that users were struggling with this. Maybe add a couple of > reference points to the commit message, to describe how easy it is to > miss the fact that it needs to be a _file_, not a directory, and also > describe how `zsh` tells you that there is a problem (even if it does > not report a problem, that's a valuable piece of information for the > commit message). After reading the updated text, and then hearing many people cannot get if the existing reference to ~/.zsh/_git is about a directory (to which this should be added as one of the files in it) or a file, I doubt that the updated text is clear enough for these people. >> -# The recommended way to install this script is to copy to '~/.zsh/_git', >> and >> -# then add the following to your ~/.zshrc file: >> +# The recommended way to install this script is to copy to >> +# '~/.zsh/.git-completion.zsh', and then add the following to your ~/.zshrc >> +# file: Perhaps making it absolutely clear by risking to be overly verbose, like so? The recommended way to install this script is to make a copy of it in ~/.zsh/ directory as ~/.zsh/git-completion.zsh and then ... If my attempt to be overly verbose and absolutely clear above is giving a wrong piece of advice, then that would be an indication that the original nor the updated text is clear enough, as they are the primary sources of information on this matter I used to come up with the suggestion ;-) >> # >> # fpath=(~/.zsh $fpath) >> >> -- >> gitgitgadget >>
Re: [PATCH v5] documentation: add tutorial for object walking
Emily Shaffer writes: > @@ -77,6 +77,7 @@ API_DOCS = $(patsubst %.txt,%,$(filter-out > technical/api-index-skel.txt technica > SP_ARTICLES += $(API_DOCS) > > TECH_DOCS += MyFirstContribution > +TECH_DOCS += MyFirstRevWalk s/Rev/Object/ probably (if so I can locally amend). > diff --git a/Documentation/MyFirstObjectWalk.txt > b/Documentation/MyFirstObjectWalk.txt > new file mode 100644 > index 00..7085f17072 > --- /dev/null > +++ b/Documentation/MyFirstObjectWalk.txt > @@ -0,0 +1,905 @@ > +My First Object Walk > +== > + > +== What's an Object Walk? > + > +The object walk is a key concept in Git - this is the process that underpins > +operations like `git log`, `git blame`, and `git reflog`. Beginning at HEAD, > the > +list of objects is found by walking parent relationships between objects. The above is more about revision walk, for which we have plenty of docs already, isn't it? Walking objects, while walking the commit DAG, is a lessor concept than the key "revision walk" concept and underpins different set of operations like object transfer and fsck. Also, the object walk, unlike the revision walk, follows containment relationships between objects. > +A related concept is the revision walk, which is focused on commit objects > and > +their relationships. Yes, s/their/& parent/ perhaps, to contrast the two a bit better. `git log` and friends, if they need to be listed, should come on this side, I think.
Re: [PATCH 1/1] t1308-config-set: fix a test that has a typo
Johannes Schindelin writes: > I should note that I looked through all of the hits of `git grep -w > except -- t/` and did not find any other typo. Thanks.
Re: [PATCH 1/1] doc(stash): clarify the description of `save`
Thomas Gummerer writes: > On 10/10, Johannes Schindelin via GitGitGadget wrote: >> From: Johannes Schindelin >> >> The original phrasing of this paragraph made at least one person stumble >> over the word "from" (thinking that it was a typo and "from" was >> intended), and other readers chimed in, agreeing that it was confusing: >> https://public-inbox.org/git/0102016b8d597569-c1f6cfdc-cb45-4428-8737-cb1bc30655d8-000...@eu-west-1.amazonses.com/#t >> >> Let's rewrite that paragraph for clarity. >> >> Inspired-by-a-patch-by: Catalin Criste >> Signed-off-by: Johannes Schindelin > > Thanks for picking this thread up again, I had already forgotten about > it. The updated wording sounds like an improvement to me. To me too. I would have just done s/form/become/ out of laziness and aiming for conciseness, but the new and separate sentence is good, too. Thanks, both. > >> --- >> Documentation/git-stash.txt | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt >> index 8fbe12c66c..53e1a1205d 100644 >> --- a/Documentation/git-stash.txt >> +++ b/Documentation/git-stash.txt >> @@ -87,8 +87,9 @@ The `--patch` option implies `--keep-index`. You can use >> save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] >> [-a|--all] [-q|--quiet] []:: >> >> This option is deprecated in favour of 'git stash push'. It >> -differs from "stash push" in that it cannot take pathspecs, >> -and any non-option arguments form the message. >> +differs from "stash push" in that it cannot take pathspecs. >> +Instead, all non-option arguments are concatenated to form the stash >> +message. >> >> list []:: >> >> -- >> gitgitgadget
Re: Raise your hand to Ack jk/code-of-conduct if your Ack fell thru cracks
Junio C Hamano writes: > ... but I'd still wait for a few > days for people who expressed their Acks but your scan missed, or > those who wanted to give their Acks but forgot to do so, to raise > their hands on this thread. Now, two days and four hours have passed, so I'll merge the result to 'next' (and thusly this poll is now closed). Thanks, all.
Re: [PATCH] parser: Unmangle From: headers that have been mangled for DMARC purposes
Jeff King writes: > This might provide an alternate solution (or vice versa). I kind of like > this one better in that it doesn't require the sender to do anything > differently (but it may be less robust, as it assumes the receiver > reliably de-mangling). I share the assessment. I also feel that relying on Reply-To: would make the result a lot less reliable (I do not have much problem with the use of X-Original-Sender, though).
Re: [PATCH 3/3] sequencer: run post-commit hook
Johannes Schindelin writes:
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index d898a57f5d..adb8c89c60 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1653,7 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char
>> *prefix)
>>
>> repo_rerere(the_repository, 0);
>> run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
>> -run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
>> +run_post_commit_hook(use_editor, get_index_file());
>
> Does it really make sense to abstract the hook name away? It adds a lot
> of churn for just two callers...
After looking at the three patches, I do not think so.
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index d2f1d5bd23..d9217235b6 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1467,4 +1467,21 @@ test_expect_success 'valid author header when author
>> contains single quote' '
>> test_cmp expected actual
>> '
>>
>> +test_expect_success 'post-commit hook is called' '
>> +test_when_finished "rm -f .git/hooks/post-commit commits" &&
>> +mkdir -p .git/hooks &&
>> +write_script .git/hooks/post-commit <<-\EOS &&
>> +git rev-parse HEAD >>commits
>
> Should `commits` be initialized before this script is written, e.g.
> using
>
> >commits &&
Yes.
>> +git rev-list --no-walk=unsorted HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} \
>> +HEAD@{1} HEAD >expected &&
>
> Wouldn't this be better as:
>
> git rev-parse HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} HEAD@{1} HEAD \
> >expect &&
>
Yes.
>> +test_cmp expected commits
>
> We usually use the name `expect` instead of `expected` in the test
> suite.
And the actual output file is called 'actual'.
Thanks.
Re: [PATCH 2/3] sequencer: use run_commit_hook()
Johannes Schindelin writes: >> builtin/commit.c | 22 -- >> commit.h | 3 --- >> sequencer.c | 45 ++--- >> sequencer.h | 2 ++ >> 4 files changed, 36 insertions(+), 36 deletions(-) > > Hmm. I would have thought that `commit.c` would be a more logical home > for that function (and that the declaration could remain in `commit.h`)? Good correction. Thanks.
Re: [PATCH 0/1] Pass through the exit code of post-checkout
"Johannes Schindelin via GitGitGadget" writes: > To answer Jonathan's question, at long last, yes, it is useful. A hook is > not only an opportunity to run code at given points in Git's life cycle, but > also an opportunity to stop Git in its tracks. In general that is correct, and especially so for pre_$git_action hooks, which are about interfering and vetoing an action that is being carried out. On the other hand, post_$git_action hooks are specifically defined as a way to trigger an extra action and documented that they cannot affect the outcome of the operation (in other words, they trigger at a point in the operation flow that is too late to affect the outcome). Now, it is somewhat debatable how the "outcome" should be defined. A post-rebase hook that drops the last commit in the sequence, for example, should not be allowed (the rebase has rebuilt a sequence and that final sequence of commits should be left), but should it be allowed to signal back to "git rebase" and affect its exit status? I am not 100% sure if it is a good idea to allow post-checkout to affect the exit status of "git checkout" or its friends. If one codepath in "git checkout" or its friends lets post-checkout to influence the exit status while another codepath ignores, it makes sense to discuss if the inconsistency needs to be removed, but in such a case, I think it would make sense to unify in the direction of ignoring (i.e. not allowing post-* hook to affect the outcome).
Re: [Outreachy] [PATCH] blame: Convert pickaxe_blame defined constants to enums
Jonathan Tan writes:
>> -if ((opt & PICKAXE_BLAME_COPY_HARDEST)
>> -|| ((opt & PICKAXE_BLAME_COPY_HARDER)
>> +if ((opt & BLAME_COPY_HARDEST)
>> +|| ((opt & BLAME_COPY_HARDER)
>
> Any reason why the names are renamed to omit "PICKAXE_"? In particular,
> these names are still global, so it is good to retain the extra context.
Absolutely. Removing them is wrong, I would have to say.
>> #define BLAME_DEFAULT_MOVE_SCORE20
>> #define BLAME_DEFAULT_COPY_SCORE40
>>
>> +enum pickaxe_blame_action {
>> +BLAME_MOVE = 01,
>> +BLAME_COPY,
>> +BLAME_COPY_HARDER = 04,
>> +BLAME_COPY_HARDEST = 010,
>> +};
We had a bit of discussion recently about using (or rather, not
abusing) enum for set of bits on a different topic.
> Also, I have a slight preference for putting "= 02" on the BLAME_COPY
> line but that is not necessary.
That is absolutely necessary; it is not like "we do not care what
exact value _COPY gets; it can be any value as long as it is _MOVE
plus 1", as these are used in set of bits (and again, I do not think
it is such a brilliant idea to use enum for such a purpose).
Thanks.
Re: [PATCH 1/1] commit: add support to provide --coauthor
Jeff King writes: > Yes, I agree that ordering and de-duplication rules are useful, too. > Some of that can be expressed already in trailer.* config, but I don't > know if it would be capable enough to do everything you want (though > again, it would be really nice to _make_ it capable enough so that other > types besides co-authored-by can make use of them). Yup, absolutely. As a mature system, unlike the early days, randomly adding a "desired" feature without first considering if a generalized form can be added is simply irresponsible to our users. Response by Brian on the side thread also indicates that we have already spent enough effort to the generalized trailer support and building on top, instead of randomly adding an ad-hoc "feature", may be the right and feasible approach.
