* Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'
From: Pratyush Yadav @ 2020-01-04 21:47 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List
In-Reply-To: <CAPig+cRL5w7azdALeBKKisTZwjgU6QhBqJRzQqDENjYiaTT0oA@mail.gmail.com>
Hi Eric,
Thanks for the review.
On 27/12/19 06:05AM, Eric Sunshine wrote:
> (Sorry for taking so long to review this patch; it ended up being a
> quite lengthy review.)
No problem :-). And sorry for being so late to reply.
> On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > When no branch name is supplied to 'worktree add', it creates a new
> > branch based on the name of the directory the new worktree is located
> > in. But when the worktree is later removed, that created branch is left
> > over.
>
> This is describing the existing (intentional) behavior but doesn't
> explain why this might be annoying or problematic. To help sell the
> patch, it might make sense to say something about how the behavior can
> trip up newcomers to git-worktree, leaving them to wonder why they are
> accumulating so many branches that they weren't aware they created. A
> comment about why you think "git worktree add -d foo" is not a viable
> way to side-step the creation of unwanted branches might also be
> worthwhile. For instance, you might say something about how newcomers
> might not read the documentation thoroughly enough to know about
> --detach or to understand what it means; indeed, some newcomers to Git
> presumably have trouble with the concept of a detached HEAD and may
> find it scary.
Will do.
> > Remove that branch when removing the worktree. To make sure no commits
> > are lost, the branch won't be deleted if it has moved.
>
> My knee-jerk reaction upon reading the first sentence of this
> paragraph was that this is a significant and undesirable behavior
> change, however, the second sentence helps to allay my fears about it.
>
> It's possible, I suppose, that there is some existing tooling
> somewhere which relies upon the current behavior, but it's hard to
> imagine any good reason to do so. (That is, "git worktree add foo &&
> git worktree remove foo" is just a glorified and expensive way to say
> "git branch foo".) So, I don't look upon this change with disfavor; it
> could well be beneficial for newcomers, and perhaps a nice convenience
> in general.
It is possible that some script somewhere does
git worktree add foo
do_something # doesn't move the branch
git worktree remove foo
git branch -d foo
Branch deletion would fail here, which might be considered as an error
by the script. Not sure how common that would be though.
> However, there is a rather serious flaw in the implementation. My
> expectation is that it should only automatically delete a branch if
> the branch creation was inferred; it should never automatically delete
> a branch which was created explicitly. You kind of have this covered
> (and even have a test for it), but it doesn't work correctly when the
> user explicitly requests branch creation via -b/-B and the branch name
> matches the worktree name. For instance:
>
> git worktree add -b foo foo
> git worktree remove foo
>
> incorrectly automatically removes branch "foo" even though the user
> requested its creation explicitly.
Thanks for pointing it out. Will fix.
> Another big question: Should an automatically-created branch be
> deleted automatically when a worktree is pruned? That is, although
> this sequence will remove an automatically-created branch:
>
> git worktree add foo
> git worktree remove foo
>
> the current patch will not clean up the branch given this sequence:
>
> git worktree add foo
> rm -rf foo
> git worktree prune
I see no problem with doing the same thing in 'prune' too.
> Either way, it might be worthwhile to update the documentation to mention this.
I'll see if I can make 'prune' delete the branch too. Otherwise, I'll
mention it in the documentation.
> > An example use case of when something like this is useful is when the
> > user wants to check out a separate worktree to run and test on an older
> > version, but don't want to touch the current worktree. So, they create a
> > worktree, run some tests, and then remove it. But this leaves behind a
> > branch the user never created in the first place.
>
> The last sentence isn't exactly accurate. The user _did_ create the
> branch. It would be more accurate to say "...the user did not
> necessarily _intentionally_ create..." or something like that.
Yes, that was the intention of the sentence. Will fix.
> > So, remove the branch if nothing was done on it.
>
> By the way, the ordering of the commit message paragraphs is a bit
> off; it somewhat tries to justifies the change before explaining what
> the problem is. I'd suggest this order:
>
> - describe current behavior
> - explain why current behavior can be undesirable in some circumstances;
> cite your example use-case here, perhaps
> - describe how this patch improves the situation
>
> The two paragraphs which talk about "remove the branch" are just
> repeating one another. I would drop one of them and keep the other as
> the final bullet point of the suggested commit message order.
Will fix.
> > Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
> > ---
> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> > @@ -73,8 +73,9 @@ If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
> > doesn't exist, a new branch based on HEAD is automatically created as
> > -if `-b <branch>` was given. If `<branch>` does exist, it will be
> > -checked out in the new worktree, if it's not checked out anywhere
> > +if `-b <branch>` was given. In this case, if `<branch>` is not moved, it is
> > +automatically deleted when the worktree is removed. If `<branch>` does exist,
> > +it will be checked out in the new worktree, if it's not checked out anywhere
>
> I found it confusing to find automatic branch deletion described here
> under the "worktree add" command...
>
> > @@ -108,6 +109,10 @@ Remove a working tree. Only clean working trees (no untracked files
> > +Removing a working tree might lead to its associated branch being deleted if
> > +it was auto-created and has not moved since. See `add` for more information on
> > +when exactly this can happen.
>
> Subjectively, it seems more natural to fully discuss automatic branch
> removal here rather than referring to the discussion of "worktree
> add".
I considered doing this but then left that part in 'add' because the
conditions in which the branch is auto deleted are described pretty well
in add's documentation. Will move it to 'remove'.
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > @@ -35,6 +35,7 @@ struct add_opts {
> > +static int auto_create;
>
> I think this variable belongs in the 'add_opts' structure rather than
> being file-global.
Ok.
> > @@ -270,11 +271,13 @@ static int add_worktree(const char *path, const char *refname,
> > - int len, ret;
> > + int len, ret, fd;
> > + struct object_id oid;
> > + char *hex;
>
> Rather than declaring 'fd', 'oid', and 'hex' here, how about declaring
> them in the scope of the "if (auto_create) {" conditional below, which
> is the only place they are used?
Will do. Some other projects I've contributed to in the past insist on
declaring everything up-front, so I played it safe and put them here.
> > @@ -353,6 +356,18 @@ static int add_worktree(const char *path, const char *refname,
> > + strbuf_reset(&sb);
> > + strbuf_addf(&sb, "%s/auto_created", sb_repo.buf);
>
> Why aren't these two lines inside the "if (auto_create) {" conditional
> below? They seem to be used only for that case.
Yes, they should be. Will fix.
> I think this new worktree metadata file warrants a documentation
> update. In particular, gitrepository-layout.txt talks about
> worktree-specific metadata files, and the "Details" section of
> git-worktree.txt may need an update.
Will fix.
> A bit of bikeshedding regarding the filename: "auto_created" is rather
> unusual. Most names in the .git hierarchy are short and sweet. Also,
> with the exception of ORIG_HEAD and FETCH_HEAD, all other multi-word
> filenames seem to use hyphen rather than underscore, which suggests
> "auto-created" would be a better choice. However, I'd probably drop
> the hyphen altogether. Finally, "auto_created", alone, does not
> necessarily convey that the branch was auto-created; someone could
> misinterpret it as meaning the worktree itself was auto-created, so I
> wonder if a better name can be found.
Any suggestions? Does "implicitbranch"/"implicit-branch" sound any
better? How about "branch-auto-created-at"? It is very clear but is a
mouthful.
> A bigger question, though, is whether we really want to see new files
> like this springing up in the .git/worktrees/<id>/ directory for each
> new piece of metadata which belongs to a worktree. I ask because this
> isn't the first such case in which some additional worktree-specific
> metadata was proposed (see, for instance, [1]). So, I'm wondering if
> we should have a more generalized solution, such as introducing a new
> file which can hold any sort of metadata which comes along in the
> future. In particular, I'm thinking about a file containing an
> extensible set of "key: value" tuples, in which case the "auto
> created" metadata would be just one of possibly many keys. For
> instance:
Do you worry that the number of metadata files might grow to be too
large? I can't say how worktrees will grow in the future, but right now
there are 4 metadata files ('commondir', 'gitdir', 'HEAD', 'ORIG_HEAD').
So, not a lot.
I chose to add a new file because from what I have noticed, Git keeps a
lot of metadata in files like this (HEAD, refs, etc). Do other
subsystems use a key-value store? What problems did they face?
> branch-auto-created-at: deadbeef
>
> The above is a genuine question. I'm not demanding that this patch
> implement it, but I think it deserves discussion and thought before
> making a decision.
I'd prefer to not take on this feature (since I expect it to be a lot of
work), but if there are strong opinions on using a key-value store then
I guess I'll bite the bullet.
> [1]: http://public-inbox.org/git/CAPig+cRGMEjVbJZKXOskN6=5zchisx7UuwW9ZKGwoq5GQZQ_rw@mail.gmail.com/
>
> > + /* Mark this branch as an "auto-created" one. */
>
> This comment doesn't really say anything which the code itself isn't
> already saying (especially if you move the strbuf_addf() call inside
> the conditional), so the comment could be dropped.
Ok.
> > + if (auto_create) {
> > + fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> > + get_oid("HEAD", &oid);
>
> Unless I'm mistaken, this is just wrong. You're grabbing the OID of
> HEAD from the worktree in which "worktree add" is being invoked,
> however, if the new branch name is DWIM'd from an existing
> tracking-branch, then the OID should be that of the tracking-branch,
> not HEAD of the current worktree. So, you should be using the OID
> already looked up earlier in the function, 'commit->object.oid', which
> should be correct for either case.
Oops! Thanks for pointing it out. Will fix.
> > + hex = oid_to_hex(&oid);
> > + write_file_buf(sb.buf, hex, strlen(hex));
> > +
> > + if (close(fd))
> > + die(_("could not close '%s'"), sb.buf);
> > + }
>
> Is there a reason you're creating the file in this rather manual
> fashion rather than using write_file() as is already heavily used in
> this code for creating all the other files residing in
> .git/worktrees/<id>/?
No particular reason. I didn't look around enough to catch the pattern
of using write_file() for this. Will fix.
> This code is correctly sandwiched within the "is_junk" scope, which
> means that "auto_created" will be cleaned up automatically, along with
> other .git/worktrees/<id>/ files, if "worktree add" fails for some
> reason. Good.
>
> > argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
> > argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
> > @@ -576,6 +591,8 @@ static int add(int ac, const char **av, const char *prefix)
> > if (run_command(&cp))
> > return -1;
> > branch = new_branch;
> > +
> > + auto_create = 1;
>
> Drop the unnecessary blank line.
>
> By the way, this suffers from the problem that if "git worktree add
> foo" fails for some reason, such as because path "foo" already exists,
> then the new branch will _not_ be cleaned up automatically since that
> failure will happen before "auto_created" is ever created (among other
> reasons). But that's not a new issue; it's an existing flaw of
> "worktree add" not cleaning up a branch it created before it discovers
> that it can't actually create the target directory for some reason, so
> I wouldn't expect you to fix that problem with this submission. (I'm
> just mentioning it for completeness.)
I'll see if I can come up with a fix for this as a follow-up patch.
> > @@ -912,9 +929,10 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> > OPT_END()
> > };
> > - struct strbuf errmsg = STRBUF_INIT;
> > + struct strbuf errmsg = STRBUF_INIT, sb = STRBUF_INIT, hex = STRBUF_INIT;
> > - int ret = 0;
> > + int ret = 0, delete_auto_created = 0;
> > + struct object_id oid;
>
> Perhaps move the declarations of 'hex' and 'oid' into the scope where
> they are used rather than making them global to the function.
Will do.
> > @@ -939,6 +957,23 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> > + /*
> > + * Check if we auto-created a branch for this worktree and it hasn't
> > + * moved since. Do it before the contents of the worktree get wiped.
> > + * Delete the branch later because it is checked out right now.
> > + */
>
> Good useful comment.
Thanks.
> > + git_path_buf(&sb, "worktrees/%s/auto_created", wt->id);
> > + if (file_exists(sb.buf)) {
> > + strbuf_read_file(&hex, sb.buf, 0);
>
> You can avoid an unnecessary race condition here by dropping the
> file_exists() call altogether and just checking the return code of
> strbuf_read_file() -- which you should probably be doing anyhow. If
> strbuf_read_file() returns a non-negative value, then you know it
> exists, so file_exists() is redundant.
Will fix. Though I don't see how it would be a "race condition". Is
file_exists() asynchronous in some way? Otherwise, how would a race
happen and between what?
> > + get_oid(wt->id, &oid);
> > +
>
> Drop the unnecessary blank line.
>
> > + if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)
> > + delete_auto_created = 1;
>
> I was wondering if it would be more semantically correct to parse
> 'hex' into an 'oid' and compare them with oidcmp() rather than doing a
> string comparison of the hex values (though I'm not sure it will
> matter in practice).
Since I haven't spent too much time in the Git internals, the string
representation feels more natural to me. And that's why I went this way
subconsciously. While I don't mind either, I wonder if it would make a
difference in practice. Anyway, if you have a preference for the other
way round, I'll trust your gut feeling.
> > + }
> > +
> > + strbuf_release(&sb);
> > + strbuf_release(&hex);
>
> Drop the unnecessary blank line.
>
> > @@ -952,6 +987,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> > + if (delete_auto_created) {
> > + struct child_process cp = CHILD_PROCESS_INIT;
> > + cp.git_cmd = 1;
> > +
> > + argv_array_push(&cp.args, "branch");
> > + argv_array_push(&cp.args, "-d");
> > + argv_array_push(&cp.args, wt->id);
> > +
> > + ret |= run_command(&cp);
> > + }
>
> Alternately:
>
> argv_array_pushl(&cp.args, "branch", "-d", wt->id, NULL);
Ok.
> However, I don't think it is correct to use 'wt->id' here as the
> branch name since there is no guarantee that the <id> in
> .git/worktrees/<id>/ matches the branch name with which the worktree
> was created. For instance:
>
> git worktree add foo/bar existing-branch
> git worktree add baz/bar
>
> will, due to name conflicts, create worktree metadata directories:
>
> .git/worktrees/bar
> .git/worktrees/bar1
>
> where the first is associated with branch "existing-branch", and the
> second is associated with new branch "bar". When you then invoke "git
> worktree remove baz/bar", it will try removing a branch named "bar1",
> not "bar" as intended. To fix this, I think you need to record the
> original auto-created branch name in the "auto_created" metadata file
> too, not just the OID.
Interesting! Didn't think of a situation like this. Thanks for pointing
it out. Will fix.
> Finally, make this code consistent with other existing similar code in
> this file by dropping the unnecessary blank lines in this hunk.
The blank lines are a personal preference for me mostly. I am not a huge
fan of seeing large chunks of code with do blank lines in between. IMO
it hurts readability. But, I think staying consistent with the code that
already exists is more important. Will remove all them.
> > diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
> > @@ -222,4 +222,49 @@ test_expect_success 'not remove a repo with initialized submodule' '
> > +test_expect_success 'remove auto-created branch' '
> > + (
> > + git worktree add to-remove &&
> > + git worktree remove to-remove &&
> > + git branch -l to-remove >branch_list &&
> > + test_line_count = 0 branch_list
> > + )
> > +'
>
> I don't think there is any need for this test to be run in a subshell,
> so you can drop the enclosing '(' and ')'.
I was following the pattern in the two tests above. Will drop the
parentheses.
> I worry about using porcelain git-branch to check whether the branch
> has actually been removed. Using git-rev-parse would likely be a more
> direct and safe way to test it. For instance:
>
> git worktree add to-remove &&
> git worktree remove to-remove &&
> test_must_fail git rev-parse --verify -q to-remove
>
> should be sufficient, I think. And, to be really thorough, you might say:
>
> test_might_fail git branch -D to-remove &&
> git worktree add to-remove &&
> git rev-parse --verify -q to-remove &&
> git worktree remove to-remove &&
> test_must_fail git rev-parse --verify -q to-remove
>
> The above comments apply to the other new tests added by this patch, as well.
Will fix.
> > +test_expect_success 'do not remove a branch that was not auto-created' '
> > + (
> > + git worktree add -b new_branch to-remove &&
>
> Nit: The inconsistent mix of underscore and hyphen in names is odd.
> Perhaps settle on one or the other (with a slight preference toward
> hyphen).
I'll change 'new_branch' to 'new-branch'.
> > + git worktree remove to-remove &&
> > + git branch -l new_branch >branch_list &&
> > + test_line_count = 1 branch_list &&
>
> As noted earlier, although this particular case of a branch created
> explicitly with -b works as expected by not deleting the branch, the
> similar case:
>
> git worktree add -b to-remove to-remove &&
>
> will incorrectly automatically delete the branch.
>
> > + git branch -d new_branch &&
> > + git branch foo &&
> > + git worktree add to-remove foo &&
> > + git worktree remove to-remove &&
> > + git branch -l foo >branch_list &&
> > + test_line_count = 1 branch_list &&
> > + git branch -d foo &&
> > + git branch to-remove &&
> > + git worktree add to-remove &&
> > + git worktree remove to-remove &&
> > + git branch -l to-remove >branch_list &&
> > + test_line_count = 1 branch_list &&
> > + git branch -d to-remove
>
> If any code above this "git branch -d" fails, then it will never get
> this far, thus won't remove "to-remove". To perform cleanup whether
> the test succeeds or fails, you should use test_when_finished()
> _early_ in the test:
>
> test_when_finished "git branch -d to-remove || :" &&
>
> However, if you restructure the tests as suggested above, then you
> might be able to get away without bothering with this cleanup.
>
> > + )
> > +'
>
> This test is checking three distinct cases of explicitly-created
> branches. It would make it easier to debug a failing case if you split
> it up into three tests -- one for each case.
I considered doing it, but then I thought maybe I shouldn't add so many
tests. And since there are only 3 rather independent cases, it would not
be that difficult to figure out which one is the culprit. Will split
them.
> > +test_expect_success 'do not remove auto-created branch that was moved' '
> > + (
> > + git worktree add to-remove &&
> > + cd to-remove &&
> > + test_commit foo &&
> > + cd ../ &&
>
> We normally avoid cd'ing around in tests like this because it can
> cause tests following this one to run in the wrong directory if
> something above the "cd ../" fails. In this particular case, it
> doesn't matter since the entire body of this test is within a
> subshell.
>
> However, if you take advantage of test_commits()'s -C argument, then
> you can ditch the cd's and the subshell altogether:
>
> test_commit -C to-remove foo &&
Ok.
> > + git worktree remove to-remove &&
> > + git branch -l to-remove >branch_list &&
> > + test_line_count = 1 branch_list &&
> > + git branch -D to-remove
> > + )
> > +'
I'll send out the v2 as soon as I can. Thanks.
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries
From: Johannes Schindelin @ 2020-01-04 21:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
In-Reply-To: <20200104015749.GE130883@google.com>
Hi Jonathan,
On Fri, 3 Jan 2020, Jonathan Nieder wrote:
> Johannes Schindelin wrote:
>
> > As mentioned above, the idea is to prevent Git from attempting to create
> > files with illegal file name characters.
> [...]
> > On Thu, 26 Dec 2019, Jonathan Nieder wrote:
>
> >> Is there anything we can or should do to prevent people checking in
> >> new examples of paths with backslash in them (on all platforms)?
> >
> > As mentioned in my reply to Junio, I don't think that it would be wise to
> > even try to warn about backslashes in the file names. There are _so_ many
> > Git users out there, I am convinced that at least some of them have valid
> > use cases for file names with backslashes in them.
>
> Thanks for the quick answers. It helps.
>
> I think allowing people to clone
> https://github.com/zephyrproject-rtos/civetweb but not to check out
> the problematic historic revision is a reasonable choice, especially
> since it's still possible to get the data from there using
>
> git checkout <rev> -- . ':!bad-paths'
>
> [...]
> > Or maybe you know of a code path in the `unpack_trees()` machinery that
> > does _not_ go through `add_index_entry()`? I would be very interested to
> > learn about such code paths.
>
> Every once in a while someone (e.g., in #git) has wanted "git checkout
> --skip-index <rev> -- <paths>", and that would be the natural way to
> implement such a thing. But no one has done it yet. :)
>
> We'll just have to keep a watchful eye as people make new
> contributions.
Right.
As to my patch, in the meantime I wondered whether I should simply extend
`verify_path()` to take a `flags` parameter where I can specify whether
this is intended for checkout, and handle the backslashes _there_. That
would cover e.g. uses of `make_transient_cache_entry()`, too.
The only reason why I would want to introduce `flags` is that `git apply`
has this `--unsafe-paths` option where `verify_paths()` is called on the
file names in the diff, and I was unsure whether there are legitimate use
cases with diffs containing DOS-style paths (I suspect that there are
legitimate scenarios where this is desirable).
What do you think of that?
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC] xl command for visualizing recent history
From: Johannes Schindelin @ 2020-01-04 21:21 UTC (permalink / raw)
To: Matthew DeVore
Cc: Emily Shaffer, Matthew DeVore, git, Matthew DeVore, jonathantanmy,
jrnieder, steadmon
In-Reply-To: <20200103201423.GA20975@comcast.net>
Hi Matthew,
On Fri, 3 Jan 2020, Matthew DeVore wrote:
> On Thu, Oct 31, 2019 at 09:26:48AM +0100, Johannes Schindelin wrote:
> >
> > am stands for "apply mbox", and I think that the only reason it is not
> > called `git apply-mbox` is that the Linux maintainer uses it a lot and
> > wanted to save on keystrokes.
> >
> > Having said that, I do agree that `xl` is not a good name for this. It
> > is neither intuitive, nor is it particularly easy to type (on a
> > US-English keyboard, the `x` and the `l` key are far apart), and to add
>
> There is a subjective element to this, but I would consider it easy to type
> since it is using two different hands. The property of "keys are far apart" is
> only bad if it's the same or close fingers doing the typing (i.e. on qwerty
> layout "ve" or "my")
Of course it is subjective! That's what I pointed out. And based on that
reasoning, I think it would be a mistake to use that name: it is _waaaaay_
too subjective.
> I'm not trying to justify an unpopular name, though :) There are other
> reasons to avoid "xl". I just found your statement surprising.
I hope I got my point across. I still think that my reason to avoid `xl`
should have been enough, even without all those other reasons (which I
actually not recall, this thread being so stale by now).
> > insult to injury, _any_ two-letter command is likely to shadow
> > already-existing aliases that users might have installed locally.
>
> "wip" seems more descriptive to me,
"wip" says "Work-In-Progress" to me. I would strongly suspect `git wip` to
mean something similar to `git stash`.
So no, it does not strike me as a good name for your command because it
suggests something _totally_ different to me, and I am not exactly what
you might call a Git newbie.
> or "logx", as I mentioned in the reply to Emily.
That name does not get my support, either. My mathematician self
associates `logx` with the natural logarithm of `x`.
I don't find this intuitive at all.
Mind, there are tons of unintuitive parts in Git's UI, but that should not
encourage anyone to make the situation even worse. To the contrary, it
should encourage you to do better than what is there already (think "Lake
Wobegon Strategy").
> > In addition, I would think that the introduction of ephemeral refs
> > should deserve its own patch. Such ephemeral refs might come in handy
> > for more things than just `xl` (or whatever better name we find).
> >
> > The design of such ephemeral refs is thoroughly interesting, too.
> >
> > One very obvious question is whether you want these refs to be
> > worktree-specific or not. I would tend to answer "yes" to that
> > question.
>
> We could key each set of ephemeral refs off of the ttyname(3) or as you
> suggested getsid(2). As you say, the Windows analog would be the handle
> of the Win32 console. (I'm guessing there is no concept of a terminal
> multiplexer unless you're using MinGW or WSL, in which case we can use
> getsid).
>
> getsid(2) seems the least likely to overlap with previous "keys" so we may
> prefer that one.
>
> getppid would not work that well if anyone ran the command (or any git command
> that refers to the ephemeral refs) in a wrapper script (I don't mean an
> automated script, which we definitely don't want people to try).
>
> I'm not so sure I would prefer this keying mechanism myself - I may be
> compelled to turn it off. I sometimes have two terminals open, visible at the
> same time, and expect them to share this kind of state. So I'm reserving
> judgment about whether it should be configurable or not. But it should probably
> be enabled (key by session ID) by default.
You have a good point. This should be an add-on patch. If you won't have
the time or inclination to implement it, I will feel compelled to do it.
> Now, if we key the refs off of the current session, it seems unnecessary to key
> off the worktree as well.
That's probably beneficial: if I `cd` to a worktree, `git log --devore` a
few commits, then `cd` back and want to cherry-pick one of the previously
show commits, I definitely do not want the ephemeral revisions to be
per-worktree.
> If someone remains in the current session, but cd to a different
> worktree, it would be natural for them to assume that the ephemeral refs
> that are still visible in the terminal window would stil work.
Yes.
> > Further, another obvious question is what to do with those refs after a
> > while. They are _clearly_ intended to be ephemeral, i.e. they should
> > just vanish after a reasonably short time. Which raises the question:
> > what is "reasonably short" in this context? We would probably want to
> > come up with a good default and then offer a config setting to change
> > it.
>
> I would propose expiring refs as the user introduced more sessions (getsid
> values) without using old ones, like and LRU cache, and to limit the repository
> to holding 16 getsid keys at a time. This way, we don't have concept of a
> real-world clock, and we let people go back to a terminal window which they
> left open for a month and still use refs that were left there (assuming of
> course they haven't been using the repository heavily otherwise, and the
> terminal content is still showing those ref numbers for them to refer to).
I don't know about you, but personally, when I find a window that had been
open for a gazillion days, there is a good chance that it is stale.
For example, I frequently find myself hitting the `Enter` key just to
trigger a re-rendering of the command prompt (which contains not only the
branch name, but also the information whether a rebase is in progress or
not) *just* because I suspect that that particular worktree is now at a
different branch.
I imagine that I am not the only person with this particular issue, so no,
I am not in favor of using an LRU. I _really_ think that we have to let
those ephemeral revisions expire based on age.
> Now, if in session 42, the user generated some ephemeral refs with
> "git log --ephemeral-refs", these would automatically destroy any existing
> ephemeral refs that were created by past invocations in session 42. I don't
> know how important it is that we clean those up, but it seems like the right
> thing to do anyway to save disk space (at least 40 bytes per commit).
I might be wrong, but in the non-public presentation I got the impression
that the use case was pretty much "I call `git xl` and then I want to use
one of those commits in a subsequent Git command".
In that respect, I really do not see the point of holding on to these
ephemeral revisions for even as much as 15 minutes. My suggestion to make
the maximal age configurable was more a conservative concern. I would be
very surprised if anybody wanted to use those ephemeral revisions for
anything else than an immediate reference.
But even then, if such a use case arises, we can easily implement it then.
_If_ it arises. Until then, I would rather avoid catering to unrealistic
(read: unneeded) scenarios.
> > Another important aspect is the naming. The naming schema you chose
> > (`h/<counter>`) is short-and-sweet, and might very well be in use
> > already, for totally different purposes. It would be a really good idea
> > to open that schema to allow for avoiding clashes with already-existing
> > refs.
> >
> > A better alternative might be to choose a naming schema that cannot
> > clash with existing refs because it would not make for valid ref names.
> > I had a look at the ref name validation, and `^<counter>` might be a
> > better naming schema to begin with: `^1` is not a valid ref name, for
> > example.
>
> I like having a new kind of syntax to make the ref names easier to type as well
> as non-conflicting with current use cases. "^" is hard-to-type if you're not
> a good touch-typist, but I guess that's fine. If you're a good touch-typist,
> "^" seems a tad easier to type than "h/" IMO.
>
> I don't see any mention of "%" in "gitrevisions(7)" so maybe that's OK to use?
> That is a little more of an everyday symbol than "^" so users are likely used to
> typing it, and is closer to the fingers' home position. But if I remember right
> this has special meaning in Windows shell (expand variables), so I guess it's
> not a good idea.
From the current `refs.c`:
/*
* How to handle various characters in refnames:
* 0: An acceptable character for refs
* 1: End-of-component
* 2: ., look for a preceding . to reject .. in refs
* 3: {, look for a preceding @ to reject @{ in refs
* 4: A bad character: ASCII control characters, and
* ":", "?", "[", "\", "^", "~", SP, or TAB
* 5: *, reject unless REFNAME_REFSPEC_PATTERN is set
*/
There is _no_ mention of `%`. In fact, `git update-ref refs/heads/% HEAD`
succeeds, while `git update-ref refs/heads/^ HEAD` fails with:
fatal: update_ref failed for ref 'refs/heads/^': refusing to
update ref with bad name 'refs/heads/^'
Also, I actually liked the implicit connotation of `^` being kind of an
upward arrow, as if it implied to refer to something above.
I fail to see any such connotation for the percent sign.
Maybe you see something there that I missed?
> > Side note: why `h/`? I really tried to think about possible motivations
> > and came up empty.
> >
>
> Mostly because it's easy to type and didn't require exotic new syntax :) And the
> "h" stands for hash.
And it totally clashes with a potential ref name:
$ git update-ref refs/heads/h/1 HEAD
$ git rev-parse h/1
79208035afdb095548daae82679b7942c6bb9579
Should we really _try_ to go out of our way to introduce ambiguities that
have not been there before? I would contend that we _do not_ want that.
Not unless forced, and I really fail to see the necessity here.
> > I would like to caution against targeting scripts with this. It is too
> > easy for two concurrently running scripts to stumble over each other.
>
> I think my wording before was too confusing. I totally agree we should
> discourage automated scripts. Convenience scripts that are meant to be used
> interactively (e.g. glorified aliases and workflow-optimization scripts) should
> be allowed, and I don't think we need to do anything special to make that work.
I would really like to caution against even _suggesting_ such "glorified"
usage of this feature. Scripts _can_, and therefore _should_, be more
stringent than to rely on ephemeral revisions. I would really make it
clear that this is _only_ intended for interactive use, by humans.
It strikes me as being similar to short revs: of course you _can_ use
shortened object names in scripts, but why _would_ you? It only opens
those scripts to run into collisions with new objects whose names
abbreviate to the same short object name. Those short names (and those
ephemeral revisions) come in handy only when there is a human who has to
type out these beasts. A script does not type, so they don't tire of using
the full names (or revision names).
Scripts which use those ephemeral revisions are very likely susceptible to
problems that non-ephemeral revisions simply do not have. So why even
bother to suggest using ephemeral revisions for scripts? I would actually
do the opposite: discourage script writers from relying on _ephemeral_
clues.
Ciao,
Dscho
^ permalink raw reply
* Re: Testsuite failures on ppc64, sparc64 and s390x (64-bit BE)
From: John Paul Adrian Glaubitz @ 2020-01-04 21:07 UTC (permalink / raw)
To: brian m. carlson; +Cc: git
In-Reply-To: <20200104205803.GE6570@camp.crustytoothpaste.net>
Hi!
On 1/4/20 9:58 PM, brian m. carlson wrote:
> I got rid of my UltraSPARC hardware when I last moved, so I won't be
> looking into this further, but it seems unlikely to me that the problem
> is in Git when the only non-Windows C change between the two versions is
> a one-line file name change.
You can get access to a very fast SPARC porterbox through the GCC compile
farm [1]. Anyone can request a free account.
Adrian
> [1] https://gcc.gnu.org/wiki/CompileFarm
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaubitz@debian.org
`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: Testsuite failures on ppc64, sparc64 and s390x (64-bit BE)
From: brian m. carlson @ 2020-01-04 20:58 UTC (permalink / raw)
To: John Paul Adrian Glaubitz; +Cc: git
In-Reply-To: <34ed7497-643e-5a38-d68c-7c075b647bcd@physik.fu-berlin.de>
[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]
On 2020-01-04 at 20:14:31, John Paul Adrian Glaubitz wrote:
> Hi!
>
> It seems that git is failing its testsuite on all 64-bit Big-Endian targets,
> full build log can be found in [1]. There seem to be multiple failures.
>
> The issue first showed with 2.25-rc1 [2].
This is interesting, because there are different failures on different
platforms. s390x shows failures only in t9100, which is one of the svn
tests. From the diff between rc0 and rc1, I don't see anything which
should affect git-svn in that way.
ppc64 and sparc64 failing a lot more tests, mostly due to segfaults, but
from a cursory glance they all appear to be SVN-related.
Are we sure that some dependency of libsvn1 hasn't caused this problem?
I noticed that all of the broken builds seem to have libserf-1-1
1.3.9-8, which appears to have received an overhaul recently. Of
course, it could be something else as well.
I got rid of my UltraSPARC hardware when I last moved, so I won't be
looking into this further, but it seems unlikely to me that the problem
is in Git when the only non-Windows C change between the two versions is
a one-line file name change.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [RFC] xl command for visualizing recent history
From: Johannes Schindelin @ 2020-01-04 20:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthew DeVore, Emily Shaffer, Matthew DeVore, git,
Matthew DeVore, jonathantanmy, jrnieder, steadmon
In-Reply-To: <xmqqk168cjn0.fsf@gitster-ct.c.googlers.com>
Hi,
On Fri, 3 Jan 2020, Junio C Hamano wrote:
> Matthew DeVore <matvore@comcast.net> writes:
>
> > On Thu, Oct 31, 2019 at 09:26:48AM +0100, Johannes Schindelin wrote:
> >>
> >> am stands for "apply mbox", and I think that the only reason it is not
> >> called `git apply-mbox` is that the Linux maintainer uses it a lot and
> >> wanted to save on keystrokes.
>
> No need to give an incorrect speculation if you do not know the
> history in this discussion.
Oh, but where would be the fun in _not_ speculating???
:-)
> Back then, the command to apply mbox contents existed and was called
> "git applymbox". "am" was invented as a better replacement with more
> rational behaviour and set of command line arguments.
Now that you mention it, I vaguely remember reading about it. But even
back then, I was not so much enthused with the idea of exporting Git
history into emails and then turning those emails back into Git history
(now with "New And Improved!" commit names), so I did actually not pay
much attention.
As you might recall, I was also a fervent opponent of `git rebase` (which
I think was based on `git am` from the get-go), claiming that history
should not be rewritten. Well, what did I know. I went on to write
`git-edit-patch-series.sh` which you accepted into Git as `git rebase
--interactive`, so there.
> >> Having said that, I do agree that `xl` is not a good name for this.
> >> It is neither intuitive, nor is it particularly easy to type (on a
> >> US-English keyboard, the `x` and the `l` key are far apart), and to
> >> add
> >
> > There is a subjective element to this, but I would consider it easy to
> > type since it is using two different hands....
>
> Give descriptive name to the command, define an alias of your choice and
> use it privately. Nobody would be able to guess what "git xl" or "git
> extra-long" command would do ;-)
I thought I made the point already that such short names are prone to be
already used by users' aliases, and that shorter command names are very
likely to break someone's setup.
While I do not have any `xl` alias defined, I have 20 custom two-letter
aliases, and I would be utterly surprised if there were less than a
thousand Git users who defined `xl` to mean something already (by now,
there are _a lot_ of Git users out there, and it would be foolish to
assume that less than even the tiny fraction of a percent that translates
into a thousand users didn't use this alias). While one might say that
forcing a thousand users to adjust is not a big deal, I would counter that
we should not, unless really necessary.
And in this case, I deem it totally not necessary at all.
But again, I was wrong before (see e.g. the `git rebase` comment above),
so what do I know.
In any case, as stated before, I would like to see this feature be
implemented as a `git log` (or even `git rev-list`) option before
implementing a dedicated command.
In other words, this new feature should be treated as a _mode_ rather than
a new command. The command can come later, just like `git whatchanged`
is essentially a special-case version of `git log`.
Ciao,
Dscho
^ permalink raw reply
* Testsuite failures on ppc64, sparc64 and s390x (64-bit BE)
From: John Paul Adrian Glaubitz @ 2020-01-04 20:14 UTC (permalink / raw)
To: git
Hi!
It seems that git is failing its testsuite on all 64-bit Big-Endian targets,
full build log can be found in [1]. There seem to be multiple failures.
The issue first showed with 2.25-rc1 [2].
Thanks,
Adrian
> [1] https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.25.0%7Erc1-1&stamp=1578096339&raw=0
> [2] https://buildd.debian.org/status/logs.php?pkg=git&arch=s390x
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaubitz@debian.org
`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.25.0-rc0
From: Taylor Blau @ 2020-01-04 17:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <nycvar.QRO.7.76.6.2001032036420.46@tvgsbejvaqbjf.bet>
Hi Johannes,
On Fri, Jan 03, 2020 at 08:39:34PM +0100, Johannes Schindelin wrote:
> Hi Taylor,
>
> On Thu, 2 Jan 2020, Taylor Blau wrote:
>
> > On Wed, Dec 25, 2019 at 01:44:54PM -0800, Junio C Hamano wrote:
> >
> > > * The beginning of rewriting "git add -i" in C.
> > >
> > > [snip]
> > >
> > > * The effort to reimplement "git add -i" in C continues.
> >
> > I noticed while preparing GitHub's blog post for 2.25 that the work to
> > rewrite "git add -i" in C was mentioned twice in the performance
> > improvements section.
> >
> > I'm not sure if this is intentional, or if this was added twice during
> > the merge(s) of and f7998d9793 (Merge branch 'js/builtin-add-i',
> > 2019-12-05) and 3beff388b2 (Merge branch 'js/builtin-add-i-cmds',
> > 2019-12-16).
>
> If you mention this feature in the blog post, please note that the
> built-in `git add -p` is not feature-complete until the
> `js/add-p-leftover-bits` branch is merged.
Yes, and thank you for reminding me. I have this in my list of 'cooking'
topics. This section is used to highlight ongoing work (e.g., NewHash
transition), but I often omit it from the posts. If other cooking topics
accumulate during the drafting, I'll make sure to include this in that
section, and note its in-progress status.
> And if you mention the built-in add -i/-p work, could I ask you to include
> a note that it is based on one of the two Outreachy projects in winter
> 2018/2019?
Of course :-).
> Thanks,
> Dscho
Thanks,
Taylor
^ permalink raw reply
* [PATCH] gitweb: fix a couple spelling errors in comments
From: Denis Ovsienko @ 2020-01-04 17:39 UTC (permalink / raw)
To: git
Date: Sat, 4 Jan 2020 17:33:55 +0000
Signed-off-by: Denis Ovsienko <denis@ovsienko.info>
---
gitweb/gitweb.perl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0f857d790b..65a3a9e62e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -741,7 +741,7 @@ sub evaluate_gitweb_config {
$GITWEB_CONFIG_SYSTEM = "" if ($GITWEB_CONFIG_SYSTEM eq $GITWEB_CONFIG_COMMON);
# Common system-wide settings for convenience.
- # Those settings can be ovverriden by GITWEB_CONFIG or GITWEB_CONFIG_SYSTEM.
+ # Those settings can be overridden by GITWEB_CONFIG or GITWEB_CONFIG_SYSTEM.
read_config_file($GITWEB_CONFIG_COMMON);
# Use first config file that exists. This means use the per-instance
@@ -5285,7 +5285,7 @@ sub format_ctx_rem_add_lines {
# + c
# + d
#
- # Otherwise the highlightling would be confusing.
+ # Otherwise the highlighting would be confusing.
if ($is_combined) {
for (my $i = 0; $i < @$add; $i++) {
my $prefix_rem = substr($rem->[$i], 0, $num_parents);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] RFC: commit: add a commit.all-ignore-submodules config option
From: Marc-André Lureau @ 2020-01-04 17:24 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git
In-Reply-To: <20200104004516.GB130883@google.com>
Hi
On Sat, Jan 4, 2020 at 4:45 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Hi,
>
> Marc-André Lureau wrote:
>
> > One of my most frequent mistake is to commit undesired submodules
> > changes when doing "commit -a", and I have seen a number of people doing
> > the same mistake in various projects. I wish there would be a config to
> > change this default behaviour.
>
> Can you say more about the overall workflow this is part of? What
> causes the submodules to change state in the first place here?
The most common case is, I guess, when you work on different branches
that have different (compatible) versions of the submodules. It is
easy to go unnoticed then, although I am usually quite careful what I
include in my commit, and will usually add changes interactively with
add -i instead.
I often rely on git commit -a during an interactive rebase. I check
the project at various points in history, find a small fix, and git
commit -a. At this point, I may have included a submodule change
inadvertently that may happen later in the series for example.
>
> [...]
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> [...]
> > @@ -1475,6 +1478,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)
> > return 0;
> > }
> >
> > + if (!strcmp(k, "commit.all-ignore-submodules")) {
> > + commit_all_ignore_submodules = git_config_bool(k, v);
> > + return 0;
> > + }
>
> nit, less important than the comment above: no other config items use
> this naming scheme. We'd have to come up with a different name if we
> want to pursue this.
Sure, I am open to suggestions.
>
> If I want to disable this setting for a particular "git commit"
> invocation, how do I do that? Typically when adding new settings, we
> add them first as command-line options and then as a separate followup
> can introduce configuration to change the defaults.
--all=no-ignore ?
>
> To summarize: I'm interested in hearing more about the overall
> workflow so we can make the standard behavior without any special
> configuration work better for it, too.
>
> Thanks and hope that helps,
> Jonathan
>
thanks for the feeback
^ permalink raw reply
* [PATCH] multi-pack-index: correct configuration in documentation
From: Johannes Berg @ 2020-01-04 12:43 UTC (permalink / raw)
To: git
It's core.multiPackIndex, not pack.multiIndex.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Documentation/technical/multi-pack-index.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/technical/multi-pack-index.txt b/Documentation/technical/multi-pack-index.txt
index 1e312396966c..4e7631437a58 100644
--- a/Documentation/technical/multi-pack-index.txt
+++ b/Documentation/technical/multi-pack-index.txt
@@ -36,7 +36,7 @@ Design Details
directory of an alternate. It refers only to packfiles in that
same directory.
-- The pack.multiIndex config setting must be on to consume MIDX files.
+- The core.multiPackIndex config setting must be on to consume MIDX files.
- The file format includes parameters for the object ID hash
function, so a future change of hash algorithm does not require
--
2.24.1
^ permalink raw reply related
* Re: [RFC PATCH] bisect run: allow inverting meaning of exit code
From: Stephen Oberholtzer @ 2020-01-04 6:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqftgvdhpz.fsf@gitster-ct.c.googlers.com>
Junio,
Thank you for your feedback! Several of your concerns were already
raised by Jonathan Nieder; I'm going to focus on the specifics you
brought up that Jonathan didn't, to avoid repeating myself too much.
On Fri, Jan 3, 2020 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Hmm.
>
> No off-the-shelf tool I know of exits 125 to signal "I dunno", so if
> you have to care about the special status 125, the "command" you are
> driving with "git bisect run" must be something that was written
> specifically to match what "git bisect" expects by knowing that 125
> is a special code to declare that the commit's goodness cannot be
> determined. Now, what's the reason why this "command" written
> specifically to be used with "git bisect", which even knows the
> special 125 convention, yields "this is good" in the wrong polarity?
>
> The only realistic reason I can think of is when the user is hunting
> for a commit that fixed what has long been broken. In such a use
> case, commits in the older part of the history would not pass the
> test (i.e. the exit status of the script would be non-zero) while
> commits in the newer part of the history would.
That's nearly exactly what happened: after bisecting the problem to
find when it was introduced, I found that an updated version did not
fail the test. I wanted to bisect the "fix" in order to review the
change and verify that it did, in fact, fix the problem -- as opposed
to change things so my test didn't fail (as it turned out, it was the
latter: rather than fix the issue, the "fix" commit simply turned the
bugged feature off by default.)
> I'd suggest dropping "-r", which has little connection to "--invert".
I was simply trying to be thorough, so if it doesn't need a short
name, that's fine by me. (And it's probably going to be
--invert-status.)
> Let's not make the style violations even worse.
Jonathan pointed that one out; you can see my corrected version in my
(at his suggestion) split-out version that simply adds support for
'run -- cmd'
> > + # how to localize part 2?
>
> Using things like "%2$s", you mean?
Hah, yeah, that was a leftover comment from before I came up with the
'$(printf $(gettext))' trick. (Which is now eval_gettext.)
> As I alluded earlier, it is unclear how this new feature should
> interact with the "we use 'xyzzy' to mean 'good', and 'frotz' to
> mean 'bad'" feature. One part of me would want to say that when
> running bisect with good and bad swapped, we should reverse the
> polarity of "bisect run" script automatically, but the rest of me
> of course would say that it would not necessarily be a good idea,
> and it is of course a huge backward incompatible change, so anything
> automatic is a no go.
This new feature works just fine with that (in fact, the last round of
tests in the test script explicitly covers this interaction.)
With "--invert-status" it doesn't matter what your names are, and with
"--success=<term>" it honors whatever terms you specified.
> > + echo "exit code $res means this commit is $state"
>
> Is this a leftover debugging output?
Yep, missed that one during my cursory review before uploading (since
test scripts hide their output by default, I didn't realize I'd left
it in there.)
> In any case, I wonder why something along the line of the attached
> patch is not sufficient and it needs this much code.
>
> git-bisect.sh | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index efee12b8b1..7fc4f9bd8f 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -247,6 +247,15 @@ bisect_run () {
> "$@"
> res=$?
>
> + if test -n "$invert_run_status"
> + then
> + case "$res" in
> + 0) res=1 ;;
> + 125) ;;
> + *) res=0 ;;
> + esac
> + fi
> +
Unfortunately, that doesn't behave properly.
As far as 'git bisect run' is concerned, there are four distinct sets
of status codes:
1. Test passed (translated to 'git bisect good') -- status 0
2. Test failed (translated to 'git bisect bad') -- status 1-124 or 126-127
3. Untestable (translated to 'git bisect skip') -- status 125
4. Malfunction (causes 'git bisect run' to halt immediately, leaving
the bisection incomplete) -- anything else
What needs to happen is that status code lists for "test passed" (#1)
and "test failed" (#2) are swapped; even when bisecting a fix, #3
(untestable) and #4 (malfunction) remain unchanged. Your patch remaps
case #4 to case #1.
(I'm actually going to put together a patch to allow the user to pare
down the exit code list for #2 to a specific list, to make 'run' less
dicey in the face of complex test requirements.)
--
-- Stevie-O
Real programmers use COPY CON PROGRAM.EXE
^ permalink raw reply
* Re: [RFC PATCH] bisect run: allow inverting meaning of exit code
From: Stephen Oberholtzer @ 2020-01-04 5:37 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, Christian Couder
In-Reply-To: <20200104012230.GD130883@google.com>
Jonathan,
Thanks for your feedback!
On Fri, Jan 3, 2020 at 8:22 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> (+cc: Christian Couder, bisect expert)
(noted)
> >
> > * '--' can be used as an option list terminator,
> > just as everywhere else.
>
> Could this part go in a separate commit? That way, it can go in
> more quickly while we figure out the rest. :)
Done! That one was simple enough I didn't tag it RFC.
>
> Initial thoughts:
>
> - it's not immediately clear to me that this is common enough
> to need the short-and-sweet name "-r". Could it have a long
> form only?
Of course; I was just trying to be thorough.
> - I think I agree with you that "git bisect run --expect=5" might
> be more clearly written as "git bisect run --success=5". Or even
> something explicitly referring to exit status, like
> --success-status=5.
That's not quite what I had here (although it touches on another idea
I've been mulling over.)
My patch adds "--success=<term>", meaning
git bisect run --success=foo ./myscript.sh
does: run myscript.sh, and if the script succeeds -- that is, exits
with status 0 -- then do 'git bisect foo'; otherwise, do whatever the
other one is.
WIthout any specialized options, the behavior today is equivalent to
"--success=good" (where status 0 means 'git bisect good', otherwise
'git bisect bad'.)
>
> - This has an interesting relationship to the "alternate terms"
> feature (--term-old / --term-new). I don't know if there's a
> good way to make that more explicit --- maybe just some notes
> with examples in the relevant parts of the manpage?
I actually had this detail spelled out in the blurb I had been writing
in Documentation/git-bisect.txt:
+The "good/bad" interpretation can be reversed by specifying -r,
+--invert, or --success=bad (or, if the bisect was started with
+--term-bad specified, the appropriate string.) In such circumstances,
+an exit code of 0 is taken to mean 'bad', while an exit code of 1-124
+or 126-127 is treated as 'good'. The meaning of all other exit codes
+is unaffected by this option.
That's why my initial version called it "expect": my thinking was that
I was "expecting" the script to say that the version was bad, rather
than that it was good.
> - the name --invert doesn't make it obvious to me what it is
> inverting: good versus bad, ones complement of the status
> code, revision range passed to "git bisect start"?
>
> Are there other commands we can try to make this consistent with?
> "find" supports arbitrary expressions involving '!' and '-not'.
> "git grep" has --invert-match: perhaps a name --invert-status
> would be clear enough?
"--invert-status" would work for me (or "--negate-status", perhaps?)
>
> > You're not allowed to specify more than one expectation.
>
> Usual convention would be "last specified option wins".
That's fine, I can make such a thing work.
> As you said, this needs docs. Writing docs often helps make the UI a
> bit better since it forces one to think about the various ways a tool
> would be used in practice.
I'd started writing them even before you replied; I didn't want to
write *too* much and have to do a lot of replacing on option names.
> > + while [ "$1" != "${1#-}" ]; do
>
> Might be simpler to do 'while true', and in the *) case to break.
I went with `test -n "$1"` after Junio pointed me at the coding guidelines :)
> It's more idiomatic to use eval_gettext here. See
> "git grep -e die -- '*.sh'" for some examples.
Yeah, I hadn't yet learned about about eval_gettext before sending the
patch. Once I saw it referenced in other code, it was easy to make
happen.
> "set -e" has
> enough exceptions that it hasn't been worth using for this.
Exceptions? Like what? As my comment pointed out, I don't know how the
test suite managed to make 'set -e' _not_ work (although I can see
that it has, in fact, managed this.) In fact, I suspect the fact that
it managed it is actually a bug in dash.
> The test
> harness is able to detect a missing "&&" so this is less error-prone
> than it sounds.)
'set -e' should be functionally equivalent to using '&&' between all
pipelines, except it also handles loops.
(also, all of those &&s are awkward to write.)
> Thanks for a clear and pleasant description of the problem being
> solved. I would like to say "here's a simple shell construct to use
> instead of ! that makes this all redundant" but lacking that, for what
> it's worth this set of new options seems like a good idea to me. :)
Well, yeah, if there was a simple shell construct to use instead of !,
I wouldn't have bothered to code this up in the first place :)
Here's what got me started on this stuff in the first place:
I was trying to bisect an kernel issue I was having. However, I had
two problems: (a) the affected machine is not powerful enough to build
a kernel in less than a day, and (b) my most reliable way to detect
the issue had to be initiated from an external system.
I wrote a script that would: build the kernel, install it onto
affected machine, boot new kernel on affected machine, and run some
tests to see if the problem occurs.
There were four possible outcomes from each attempt:
1. Kernel fails to build or boot: correct action is 'git bisect skip'
(this was mapped to exit code 125)
(note that, since there was no way to automatically determine if
the kernel booted correctly, that situation actually manifested as
case 4)
2. Problem is not present: correct action is 'git bisect good' (mapped
to exit code 0)
3. Problem is present: correct action is 'git bisect bad' (mapped to
exit code 99)
(also: need to reboot the machine back to a working kernel.)
4. Something unexpected went wrong: correct action is to hit the
emergency stop and let me fix it manually (mapped to exit code 255)
Situation #4 probably happened about five or six times, in all. (One
memorable instance was during the 16th or 17th revision, when the
target machine ran out of disk space while copying the new kernel
over. A couple of times, ssh had an error trying to connect. And
several times, for reasons beyond my understanding, the 255 failsafe
kicked in on exit code 99.)
I had to map a _lot_ of failure cases to 255. In essence, I did this:
(That's why I used 99 to indicate failure; it was unlikely to produce
a false positive.) I used a bunch of ultimately-not-very-reliable
techniques to try and map all exit codes other than 0, 99, or 125 to
255. I'm thinking of adding a "--fail-status=" option, which would
let you specify the explicit status code or codes to treat as 'bad',
with all other nonzero codes being hard errors and causing 'run' to
bail immediately. Such a thing would have made several aspects of my
script far less error-prone.
It took most of a day (much better than most of a month) but I found
the offending commit.
Later, I noticed that, in one of the recent -rc builds, the problem no
longer seemed to manifest. But then the question became: was the
problem truly fixed, or was the problem being masked otherwise?
That's when I discovered that properly adjusting the exit status from
my script was... complicated: I needed to reverse the behavior for
cases 2 and 3 above, but leave the behavior for cases 1 and 4
unchanged. And I figured that, if I had this problem, someone else
surely did as well.
--
-- Stevie-O
Real programmers use COPY CON PROGRAM.EXE
^ permalink raw reply
* Re: git checkout -
From: Todd Zullinger @ 2020-01-04 5:17 UTC (permalink / raw)
To: Jonathan Nieder
Cc: François WAUQUIER, Nguyễn Thái Ngọc Duy,
git
In-Reply-To: <20200104004826.GC130883@google.com>
Hi,
Jonathan Nieder wrote:
> Hi,
>
> François WAUQUIER wrote:
>
>> $ git checkout -
>>
>> I often use this command to go back to previous branch from my history.
>> It is quite natural as it uses the same syntax as “cd -“
>>
>> But i found out it is not documented in
>> https://git-scm.com/docs/git-checkout/2.24.0
>> I report this to help others to discover this time saving command.
>
> Thanks for reporting!
>
> Ideas for what the documentation should say about it? (Bonus points
> if it comes in the form of a patch against Documentation/git-checkout.txt.
> ;-) See [1] for more about how that works.)
Not to say that it can't possibly be improved, but it is
mentioned in the git-checkout docs here[1]. The second
paragraph in that section says:
You can use the @{-N} syntax to refer to the N-th last
branch/commit checked out using "git checkout"
operation. You may also specify - which is synonymous to
@{-1}.
This is also in the git-switch documentation, where it might
be easier to find, as it's very close to the beginning of
git-switch's man page, in the "<start-point>" entry of the
OPTIONS section[2].
Being easy to miss in the git-checkout documentation might
make Duy smile; it shows the benefit of splitting some of
the many features of 'checkout' to the 'switch' command. :)
[1] https://git-scm.com/docs/git-checkout#Documentation/git-checkout.txt-ltbranchgt
[2] https://git-scm.com/docs/git-switch#Documentation/git-switch.txt-ltstart-pointgt
--
Todd
^ permalink raw reply
* Re: [PATCH] bisect run: allow '--' as an options terminator
From: Junio C Hamano @ 2020-01-04 4:56 UTC (permalink / raw)
To: Stephen Oberholtzer; +Cc: git, Christian Couder, Jonathan Nieder
In-Reply-To: <20200104034551.23658-1-stevie@qrpff.net>
Stephen Oberholtzer <stevie@qrpff.net> writes:
> The 'bisect run' feature doesn't currently accept any options, but
> it could, and that means a '--' option.
Sorry, but the above description does not make much sense to me. I
do agree up to the "but it could" part with the above sentence, but
double-dash is hardly a logical consequence of it, at least to me.
> - git bisect run <cmd>...
> + git bisect run [--] <cmd>...
The only reason why you might want '--' disambiguator is if you have
a <cmd> that happens to begin with a dash (i.e. making it possible
to be confused as an unrecognised option), but I do not think it is
unreasonable at all if we declare that you cannot feed a cmd that
begins with a dash. On a rare occasion like that, the user could
even do the "prefix with "sh -c'" you alluded to in the other thread
to escape/quote the leading dash, if the user really wanted to use
such a strange command.
> Additionally, this adds an actual test script for bisect run - it
> creates a repository with a failed build, then attempts to bisect
> the offending commit.
I do not think I have seen enough to justify addition of "--", but
addition of tests by itself may have a value, and I'd prefer to see
it as its own patch. But I see hits for
git grep 'git bisect run' t/
already in t6030, so "adds an actual test script", while it is not
telling a lie, may be giving a false impression that it is adding
something new that has been missing.
So, I dunno.
> diff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh
> new file mode 100755
> index 0000000000..e173ca18b3
> --- /dev/null
> +++ b/t/t6071-bisect-run.sh
> @@ -0,0 +1,96 @@
> +#!/bin/sh
> +
> +test_description='Tests git bisect run'
> +
> +exec </dev/null
All of our test scripts are designed to be used unattended with the
use of test_expect_* harness helpers. Can you tell us why this new
file alone needs to dissociate the input to _all_ its subproesses by
doing this, while others do not have to?
> +. ./test-lib.sh
> +
> +{ test_expect_success 'Setting up repo for "git bisect run" tests.' "$(cat)" ; } <<'SETUP'
Yuck. I have to say this is trying to be too clever and cute than
its worth. Doesn't it defeat test-lint, for example?
> +echo '.DEFAULT: dummy
> +.PHONY: dummy
> +dummy:
> + true
> +' > Makefile &&
> +make &&
> +echo '0' >path0 &&
> +git update-index --add -- Makefile path0 &&
> +git commit -q -m 'initial commit' &&
> +git tag working0 &&
> +# make some commits that don't cause problems
> +for x in `test_seq 1 20`; do
More than one Documentation/CodingGuidelines violation on a single
line X-<.
> + echo "$x" >path0 &&
> + git update-index --replace -- path0 &&
> + git commit -q -m "working commit $x" &&
> + git tag "working$x" || exit 1
> +done &&
> +# break the makefile
> +sed -i.bak -e 's/true/false/' Makefile &&
"sed -i" is unportable, isn't it? Avoid it.
> ...
> +SETUP
> +
> +test_expect_success 'setup first bisect' 'git bisect start && git bisect good working0 && git bisect bad broken9'
Lay it out like so for readability:
test_expect_success "setup first bisect" '
git bisect start &&
...
'
In general, read and mimic existing tests that have been cleaned up
recently ("git log t/" is your friend).
> +
> +test_expect_failure() {
Do not override what we provide in test-lib and test-lib-functions.
Those who are familiar with the test framework depend on these names
to work the way they are used to.
Learn what are given by the test framework to you already before
trying to invent your own convention---that would hurt everybody,
including the current set of reviewers and future developers who
need to fix what you wrote.
> + shift
> + test_must_fail "$@"
> +}
The remainder of the file not reviewed (yet).
Thanks.
^ permalink raw reply
* Re: What the meaning of two commit ID in a merge commit's log
From: brian m. carlson @ 2020-01-04 4:08 UTC (permalink / raw)
To: wuzhouhui; +Cc: git
In-Reply-To: <56365810.1196c.16f6e224802.Coremail.wuzhouhui14@mails.ucas.ac.cn>
[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]
On 2020-01-04 at 01:18:58, wuzhouhui wrote:
> Hi,
>
> When I use "git log" to display commit log, I see there are
> two commit IDs in a merge commit, e.g.
>
> commit 2187f215ebaac73ddbd814696d7c7fa34f0c3de0
> Merge: 2d3145f8d280 fbd542971aa1
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Tue Dec 17 13:27:02 2019 -0800
>
> Merge tag 'for-5.5-rc2-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
>
> In the previous example, we can see two commit IDs: 2d3145f8d280 and
> fbd542971aa1. So, what's the meaning of them, precisely.
Non-merge commits have a single parent commit that they're based on
(unless they're a root commit). Merge commits, since they merge two (or
more) separate lines of development, contain multiple parent commits
(usually, but not always, two). So the two commit IDs here are the two
parent commits of this merge.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* [PATCH] bisect run: allow '--' as an options terminator
From: Stephen Oberholtzer @ 2020-01-04 3:45 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Christian Couder, Jonathan Nieder,
Stephen Oberholtzer
The 'bisect run' feature doesn't currently accept any options, but
it could, and that means a '--' option.
Additionally, this adds an actual test script for bisect run - it
creates a repository with a failed build, then attempts to bisect
the offending commit.
Signed-off-by: Stephen Oberholtzer <stevie@qrpff.net>
---
Documentation/git-bisect.txt | 2 +-
git-bisect.sh | 19 ++++++-
t/t6071-bisect-run.sh | 96 ++++++++++++++++++++++++++++++++++++
3 files changed, 115 insertions(+), 2 deletions(-)
create mode 100755 t/t6071-bisect-run.sh
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 7586c5a843..e72353e157 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -26,7 +26,7 @@ on the subcommand:
git bisect (visualize|view)
git bisect replay <logfile>
git bisect log
- git bisect run <cmd>...
+ git bisect run [--] <cmd>...
git bisect help
This command uses a binary search algorithm to find which commit in
diff --git a/git-bisect.sh b/git-bisect.sh
index efee12b8b1..46085651e1 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -26,7 +26,7 @@ git bisect replay <logfile>
replay bisection log.
git bisect log
show bisect log.
-git bisect run <cmd>...
+git bisect run [--] <cmd>...
use <cmd>... to automatically bisect.
Please use "git help bisect" to get the full man page.'
@@ -238,6 +238,23 @@ bisect_replay () {
bisect_run () {
git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
+ # check for options
+ # (there aren't any supported yet, unless you count "--")
+ while test -n "$1"
+ do
+ case "$1" in
+ --)
+ shift
+ break ;;
+ -*)
+ option="$1" # \$1 is not expanded by eval_gettext
+ die "$(eval_gettext "bisect run: invalid option: \$option")" ;;
+ *)
+ break ;;
+ esac
+ shift
+ done
+
test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
while true
diff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh
new file mode 100755
index 0000000000..e173ca18b3
--- /dev/null
+++ b/t/t6071-bisect-run.sh
@@ -0,0 +1,96 @@
+#!/bin/sh
+
+test_description='Tests git bisect run'
+
+exec </dev/null
+
+. ./test-lib.sh
+
+{ test_expect_success 'Setting up repo for "git bisect run" tests.' "$(cat)" ; } <<'SETUP'
+echo '.DEFAULT: dummy
+.PHONY: dummy
+dummy:
+ true
+' > Makefile &&
+make &&
+echo '0' >path0 &&
+git update-index --add -- Makefile path0 &&
+git commit -q -m 'initial commit' &&
+git tag working0 &&
+# make some commits that don't cause problems
+for x in `test_seq 1 20`; do
+ echo "$x" >path0 &&
+ git update-index --replace -- path0 &&
+ git commit -q -m "working commit $x" &&
+ git tag "working$x" || exit 1
+done &&
+# break the makefile
+sed -i.bak -e 's/true/false/' Makefile &&
+rm -f Makefile.bak &&
+! make &&
+git update-index --replace -- Makefile &&
+git commit -q -m "First broken commit" &&
+git tag broken0 &&
+# make some more commits that still FTBFS
+for x in `test_seq 1 20`; do
+ echo "$x" >path0 &&
+ git update-index --replace -- path0 &&
+ git commit -q -m "broken build $x" &&
+ git tag "broken$x" || exit 1
+done &&
+# repair it
+git checkout working0 -- Makefile &&
+make &&
+git update-index --replace -- Makefile &&
+git commit -q -m "First repaired commit" &&
+git tag fixed0 &&
+# make some more commits with the bugfix
+for x in `test_seq 1 20`; do
+ echo "$x" >path0 &&
+ git update-index --replace -- path0 &&
+ git commit -q -m "fixed build $x" &&
+ git tag "fixed$x" || exit 1
+done
+SETUP
+
+test_expect_success 'setup first bisect' 'git bisect start && git bisect good working0 && git bisect bad broken9'
+
+test_expect_failure() {
+ shift
+ test_must_fail "$@"
+}
+
+# okay, let's do some negative testing
+
+OLDPATH="$PATH"
+
+PATH="$PATH:."
+
+test_expect_success 'setup this-is-not-a-valid-option' '
+ echo "#/bin/sh" > --this-is-not-a-valid-option &&
+ chmod a+x -- --this-is-not-a-valid-option &&
+ --this-is-not-a-valid-option'
+
+test_expect_failure 'git bisect run: reject unrecognized options' git bisect run --this-is-not-a-valid-option
+
+PATH="$OLDPATH"
+
+# finally, verify that '--' is honored (note that will mess things up and require a bisect reset)
+PATH="$PATH:."
+
+test_expect_success 'git bisect run: honor --' 'git bisect run -- --this-is-not-a-valid-option'
+
+PATH="$OLDPATH"
+
+# now we have to undo the bisect run
+test_expect_success 'restarting bisection' 'git bisect reset && git bisect start && git bisect good working0 && git bisect bad broken9'
+
+test_expect_success "running bisection" "
+ git bisect run $success_option make &&
+ # we should have determined that broken0 is the first bad version
+ test_cmp_rev broken0 refs/bisect/bad &&
+ # and that version should be the one checked out
+ test_cmp_rev broken0 HEAD
+"
+
+test_done
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] Documentation: Mention `Date: ` in git-am
From: Junio C Hamano @ 2020-01-04 3:33 UTC (permalink / raw)
To: Paul Menzel; +Cc: git
In-Reply-To: <6ffcb6c0-d09e-45c4-d264-8a9024b67f3c@molgen.mpg.de>
Paul Menzel <pmenzel@molgen.mpg.de> writes:
> Subject: Re: [PATCH] Documentation: Mention `Date: ` in git-am
The technical term to refer to these From/Subject/Date override is
"in-body header", and using it would make it easier to tell what the
commit is about when this appears in "git shortlog" output. Perhaps
am: document that Date: can appear as an in-body header
> Date: Fri, 3 Jan 2020 12:48:46 +0100
When the general public sees the patch for the first time is when
the patch is authored as far as the participants of this list are
concerned.
Please don't lie about the author date on this list.
> Tested, that a line `Date: ` in the message body will be preferred by
> `git am` over the one in the message header. So, update the
> documentation.
>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
> Documentation/git-am.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index fc5750b3b8..11ca61b00b 100644
> --- a/Documentation/git-am.txt
> +++ b/Documentation/git-am.txt
> @@ -190,8 +190,8 @@ the commit, after stripping common prefix "[PATCH <anything>]".
> The "Subject: " line is supposed to concisely describe what the
> commit is about in one line of text.
>
> -"From: " and "Subject: " lines starting the body override the respective
> -commit author name and title values taken from the headers.
> +"From: ", "Date: ", and "Subject: " lines starting the body override the
> +respective commit author name and title values taken from the headers.
Correct. It would be a good idea to fix "will be preferred" in the
proposed log message to match this by reusing the verb "override".
Perhaps like
Similar to "From:" and "Subject:" already mentioned in the
documentation, "Date:" can also appear as an in-body header
to override the value in the e-mail headers. Document it.
> The commit message is formed by the title taken from the
> "Subject: ", a blank line and the body of the message up to
^ permalink raw reply
* Re: [RFC PATCH] bisect run: allow inverting meaning of exit code
From: Junio C Hamano @ 2020-01-04 3:27 UTC (permalink / raw)
To: Stephen Oberholtzer; +Cc: git
In-Reply-To: <20200103043027.4537-1-stevie@qrpff.net>
Stephen Oberholtzer <stevie@qrpff.net> writes:
> If your automated test naturally yields zero for _old_/_good_,
> 1-124 or 126-127 for _new_/_bad_, then you're set.
>
> If that logic is reversed, however, it's a bit more of a pain: you
> can't just stick a `sh -c !` in front of your command, because that
> doesn't account for exit codes 125 or 129-255.
Hmm.
No off-the-shelf tool I know of exits 125 to signal "I dunno", so if
you have to care about the special status 125, the "command" you are
driving with "git bisect run" must be something that was written
specifically to match what "git bisect" expects by knowing that 125
is a special code to declare that the commit's goodness cannot be
determined. Now, what's the reason why this "command" written
specifically to be used with "git bisect", which even knows the
special 125 convention, yields "this is good" in the wrong polarity?
The only realistic reason I can think of is when the user is hunting
for a commit that fixed what has long been broken. In such a use
case, commits in the older part of the history would not pass the
test (i.e. the exit status of the script would be non-zero) while
commits in the newer part of the history would.
> -git bisect run <cmd>...
> +git bisect run [--expect=<term> | -r | --invert] [--] <cmd>...
> use <cmd>... to automatically bisect.
I'd suggest dropping "-r", which has little connection to "--invert".
> @@ -238,6 +238,31 @@ bisect_replay () {
> bisect_run () {
> git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
>
> + SUCCESS_TERM=$TERM_GOOD
> + FAIL_TERM=$TERM_BAD
> + INVERT_SET=
> + while [ "$1" != "${1#-}" ]; do
Let's not make the style violations even worse. Ideally, a
preliminary clean-up patch to fix the existing ones before the main
patch would be a good idea (cf. Documentation/CodingGuidelines).
It is more efficient and conventional (hence easier to read) to
reuse the single "case" you would need to write anyway for the loop
control, i.e.
while :
do
case "$1" in
--) ... ;;
--invert | ... ) ... ;;
-*) die "unknown command ;;
*) break ;;
esac
done
> + --expect=*)
> + # how to localize part 2?
Using things like "%2$s", you mean?
As I alluded earlier, it is unclear how this new feature should
interact with the "we use 'xyzzy' to mean 'good', and 'frotz' to
mean 'bad'" feature. One part of me would want to say that when
running bisect with good and bad swapped, we should reverse the
polarity of "bisect run" script automatically, but the rest of me
of course would say that it would not necessarily be a good idea,
and it is of course a huge backward incompatible change, so anything
automatic is a no go.
> @@ -262,11 +287,13 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> state='skip'
> elif [ $res -gt 0 ]
> then
> - state="$TERM_BAD"
> + state="$FAIL_TERM"
> else
> - state="$TERM_GOOD"
> + state="$SUCCESS_TERM"
> fi
>
> + echo "exit code $res means this commit is $state"
Is this a leftover debugging output?
In any case, I wonder why something along the line of the attached
patch is not sufficient and it needs this much code.
git-bisect.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/git-bisect.sh b/git-bisect.sh
index efee12b8b1..7fc4f9bd8f 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -247,6 +247,15 @@ bisect_run () {
"$@"
res=$?
+ if test -n "$invert_run_status"
+ then
+ case "$res" in
+ 0) res=1 ;;
+ 125) ;;
+ *) res=0 ;;
+ esac
+ fi
+
# Check for really bad run error.
if [ $res -lt 0 -o $res -ge 128 ]
then
^ permalink raw reply related
* Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries
From: Jonathan Nieder @ 2020-01-04 1:57 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
In-Reply-To: <nycvar.QRO.7.76.6.2001022053530.46@tvgsbejvaqbjf.bet>
Johannes Schindelin wrote:
> As mentioned above, the idea is to prevent Git from attempting to create
> files with illegal file name characters.
[...]
> On Thu, 26 Dec 2019, Jonathan Nieder wrote:
>> Is there anything we can or should do to prevent people checking in
>> new examples of paths with backslash in them (on all platforms)?
>
> As mentioned in my reply to Junio, I don't think that it would be wise to
> even try to warn about backslashes in the file names. There are _so_ many
> Git users out there, I am convinced that at least some of them have valid
> use cases for file names with backslashes in them.
Thanks for the quick answers. It helps.
I think allowing people to clone
https://github.com/zephyrproject-rtos/civetweb but not to check out
the problematic historic revision is a reasonable choice, especially
since it's still possible to get the data from there using
git checkout <rev> -- . ':!bad-paths'
[...]
> Or maybe you know of a code path in the `unpack_trees()` machinery that
> does _not_ go through `add_index_entry()`? I would be very interested to
> learn about such code paths.
Every once in a while someone (e.g., in #git) has wanted "git checkout
--skip-index <rev> -- <paths>", and that would be the natural way to
implement such a thing. But no one has done it yet. :)
We'll just have to keep a watchful eye as people make new
contributions.
Sincerely,
Jonathan
^ permalink raw reply
* Re: [RFC PATCH] bisect run: allow inverting meaning of exit code
From: Jonathan Nieder @ 2020-01-04 1:22 UTC (permalink / raw)
To: Stephen Oberholtzer; +Cc: git, Junio C Hamano, Christian Couder
In-Reply-To: <20200103043027.4537-1-stevie@qrpff.net>
(+cc: Christian Couder, bisect expert)
Hi!
Stephen Oberholtzer wrote:
> NOTE: This is obviously not ready for merging; I just wanted to
> get feedback.
Thanks for writing. I like where you're going.
> In particular, I expect some bikeshedding on the specific option
> names (-r, --invert, --expect). I'm probably going to change
> `--expect` to `--success`, in fact.
>
> If we can come to a consensus on the names (and, of course, on the
> feature itself), I'll clean up the tests, remove the debug output,
> update the documentation, then resubmit.
>
> >8------------------------------------------------------8<
>
> If your automated test naturally yields zero for _old_/_good_,
> 1-124 or 126-127 for _new_/_bad_, then you're set.
>
> If that logic is reversed, however, it's a bit more of a pain: you
> can't just stick a `sh -c !` in front of your command, because that
> doesn't account for exit codes 125 or 129-255.
>
> This commit enhances `git bisect run` as follows:
>
> * '--' can be used as an option list terminator,
> just as everywhere else.
Could this part go in a separate commit? That way, it can go in
more quickly while we figure out the rest. :)
> * The treatment of the exit code can be selected via an option:
>
> - No option, of course, treats 0 as _old_/_good_
> - `-r` (for reverse) treats 0 as _new_/_bad_
> - `--invert` is the long form for `-r`
> - `--expect=<term>` treats 0 as <term>
Initial thoughts:
- it's not immediately clear to me that this is common enough
to need the short-and-sweet name "-r". Could it have a long
form only?
- I think I agree with you that "git bisect run --expect=5" might
be more clearly written as "git bisect run --success=5". Or even
something explicitly referring to exit status, like
--success-status=5.
- This has an interesting relationship to the "alternate terms"
feature (--term-old / --term-new). I don't know if there's a
good way to make that more explicit --- maybe just some notes
with examples in the relevant parts of the manpage?
- the name --invert doesn't make it obvious to me what it is
inverting: good versus bad, ones complement of the status
code, revision range passed to "git bisect start"?
I'm even tempted to call it something like '-!', to make the
allusion to ! in shells more explicit. (But that's probably not a
great idea, since quoting ! correctly in interactive shells can be
difficult.)
Are there other commands we can try to make this consistent with?
"find" supports arbitrary expressions involving '!' and '-not'.
"git grep" has --invert-match: perhaps a name --invert-status
would be clear enough?
> You're not allowed to specify more than one expectation.
Usual convention would be "last specified option wins".
> Note that this lets one specify `--expect=good` as an explicit
> selection of the default behavior. This is intentional.
>
> Signed-off-by: Stephen Oberholtzer <stevie@qrpff.net>
> ---
> git-bisect.sh | 33 +++++++++-
> t/t6071-bisect-run.sh | 142 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 172 insertions(+), 3 deletions(-)
> create mode 100755 t/t6071-bisect-run.sh
As you said, this needs docs. Writing docs often helps make the UI a
bit better since it forces one to think about the various ways a tool
would be used in practice.
> diff --git a/git-bisect.sh b/git-bisect.sh
> index efee12b8b1..dbeb213846 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -26,7 +26,7 @@ git bisect replay <logfile>
> replay bisection log.
> git bisect log
> show bisect log.
> -git bisect run <cmd>...
> +git bisect run [--expect=<term> | -r | --invert] [--] <cmd>...
> use <cmd>... to automatically bisect.
>
> Please use "git help bisect" to get the full man page.'
> @@ -238,6 +238,31 @@ bisect_replay () {
> bisect_run () {
> git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
>
> + SUCCESS_TERM=$TERM_GOOD
> + FAIL_TERM=$TERM_BAD
> + INVERT_SET=
> + while [ "$1" != "${1#-}" ]; do
Might be simpler to do 'while true', and in the *) case to break.
> + case "$1" in
> + --)
> + shift
> + break ;;
> + --expect=$TERM_GOOD)
> + [ -z "$INVERT_SET" ] || die "$(gettext "bisect run: multiple expect options specified")"
> + INVERT_SET=1 ;;
> + -r|--invert|--expect=$TERM_BAD)
> + [ -z "$INVERT_SET" ] || die "$(gettext "bisect run: multiple expect options specified")"
> + SUCCESS_TERM=$TERM_BAD
> + FAIL_TERM=$TERM_GOOD
> + INVERT_SET=1 ;;
> + --expect=*)
> + # how to localize part 2?
> + die "$(printf "$(gettext "bisect run: invalid --expect value, use --expect=%s or --expect=%s")" "$TERM_GOOD" "$TERM_BAD")" ;;
It's more idiomatic to use eval_gettext here. See
"git grep -e die -- '*.sh'" for some examples.
> + *)
> + die "$(printf "$(gettext "bisect run: invalid option: %s")" "$1")" ;;
> + esac
> + shift
> + done
> +
> test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
>
> while true
> @@ -262,11 +287,13 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> state='skip'
> elif [ $res -gt 0 ]
> then
> - state="$TERM_BAD"
> + state="$FAIL_TERM"
> else
> - state="$TERM_GOOD"
> + state="$SUCCESS_TERM"
> fi
>
> + echo "exit code $res means this commit is $state"
> +
> # We have to use a subshell because "bisect_state" can exit.
> ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> res=$?
> diff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh
> new file mode 100755
> index 0000000000..2708e0f854
> --- /dev/null
> +++ b/t/t6071-bisect-run.sh
> @@ -0,0 +1,142 @@
> +# verify that unrecognized options are rejected by 'git bisect run'
> +#!/bin/sh
> +
> +# the linter's not smart enough to handle set -e
> +GIT_TEST_CHAIN_LINT=0
> +
> +test_description='Tests git bisect run'
> +
> +exec </dev/null
> +
> +. ./test-lib.sh
> +
> +{ test_expect_success 'Setting up repo for "git bisect run" tests.' "$(cat)" ; } <<'SETUP'
Tests put the commands to be passed to test_expect_success in a
single-quoted argument, with explicit &&-chaining. ("set -e" has
enough exceptions that it hasn't been worth using for this. The test
harness is able to detect a missing "&&" so this is less error-prone
than it sounds.) See t/README and any recently added test scripts in
t/ (try "git log -p --diff-filter=A -- t") for more details.
Thanks for a clear and pleasant description of the problem being
solved. I would like to say "here's a simple shell construct to use
instead of ! that makes this all redundant" but lacking that, for what
it's worth this set of new options seems like a good idea to me. :)
Sincerely,
Jonathan
(the rest left unsnipped for reference)
> +(
> +# I don't know how they managed it, but the git test engine
> +# somehow ignores the effect of 'set -e'.
> +set -eu || exit 1
> +# set -e canary
> +false
> +# hopefully, next year, we get -o pipefail!
> +echo '.DEFAULT: dummy
> +.PHONY: dummy
> +dummy:
> + true
> +' > Makefile
> +make
> +echo '0' >path0
> +git update-index --add -- Makefile path0
> +git commit -q -m 'initial commit'
> +git tag working0
> +# make some commits that don't cause problems
> +for x in `test_seq 1 20`; do
> + echo "$x" >path0
> + git update-index --replace -- path0
> + git commit -q -m "working commit $x"
> + git tag "working$x"
> +done
> +# break the makefile
> +sed -i.bak -e 's/true/false/' Makefile
> +rm -f Makefile.bak
> +! make
> +git update-index --replace -- Makefile
> +git commit -q -m "First broken commit"
> +git tag broken0
> +# make some more commits that still FTBFS
> +echo "exit code was $?; flags are $-"
> +for x in `test_seq 1 20`; do
> + echo "$x" >path0
> + git update-index --replace -- path0
> + git commit -q -m "broken build $x"
> + git tag "broken$x"
> +done
> +# repair it
> +git checkout working0 -- Makefile
> +make
> +git update-index --replace -- Makefile
> +git commit -q -m "First repaired commit"
> +git tag fixed0
> +# make some more commits with the bugfix
> +for x in `test_seq 1 20`; do
> + echo "$x" >path0
> + git update-index --replace -- path0
> + git commit -q -m "fixed build $x"
> + git tag "fixed$x"
> +done
> +#sh -c 'bash -i <> /dev/tty >&0 2>&1'
> +)
> +
> +SETUP
> +
> +test_expect_success 'setup first bisect' 'git bisect start && git bisect good working0 && git bisect bad broken9'
> +
> +test_expect_failure() {
> + shift
> + #echo arguments are "$*"
> + test_must_fail "$@"
> +}
> +
> +# okay, let's do some negative testing
> +
> +OLDPATH="$PATH"
> +
> +PATH="$PATH:."
> +
> +test_expect_success 'setup this-is-not-a-valid-option' '
> + echo "#/bin/sh" > --this-is-not-a-valid-option &&
> + chmod a+x -- --this-is-not-a-valid-option &&
> + --this-is-not-a-valid-option'
> +
> +test_expect_failure 'git bisect run: reject unrecognized options' git bisect run --this-is-not-a-valid-option
> +
> +PATH="$OLDPATH"
> +
> +test_expect_failure 'git bisect run: reject invalid values for --expect' git bisect run --expect=invalid make
> +
> +# okay, all of these settings are mutually exclusive (for sanity's sake, even with themselves)
> +for a in --expect=bad --expect=good -r --invert; do
> + for b in --expect=bad --expect=good -r --invert; do
> + test_expect_failure 'git bisect run: reject multiple --expect options' git bisect run $a $b make
> + done
> +done
> +
> +# finally, verify that '--' is honored (note that will mess things up and require a bisect reset)
> +PATH="$PATH:."
> +
> +test_expect_success 'git bisect run: honor --' 'git bisect run -- --this-is-not-a-valid-option'
> +
> +PATH="$OLDPATH"
> +
> +for expect_syntax in '' --expect=good; do
> +
> + # now we have to undo the bisect run
> + test_expect_success 'restarting bisection' 'git bisect reset && git bisect start && git bisect good working0 && git bisect bad broken9'
> +
> + test_expect_success "running bisection ($expect_syntax)" "
> +git bisect run $expect_syntax make &&
> +git log --oneline &&
> + # we should have determined that broken0 is the first bad version
> + test_cmp_rev broken0 refs/bisect/bad &&
> + # and that version should be the one checked out
> + test_cmp_rev broken0 HEAD
> +"
> +done
> +
> +
> +# NOW, test the reverse: find when we fixed it again
> +
> +for expect_syntax in -r --invert --expect=fixed; do
> +
> + test_expect_success 'restarting bisection' 'git bisect reset && git bisect start --term-old=broken --term-new=fixed && git bisect broken broken20 && git bisect fixed fixed20'
> + test_expect_success "running bisection ($expect_syntax)" "
> + git bisect run $expect_syntax make &&
> + git log --oneline &&
> + test_cmp_rev fixed0 refs/bisect/fixed &&
> + test_cmp_rev fixed0 HEAD
> + "
> +done
> +
> +test_expect_failure 'sanity check error message with custom terms' git bisect run --expect=invalid make
> +
> +
> +test_done
> --
> 2.20.1
>
^ permalink raw reply
* What the meaning of two commit ID in a merge commit's log
From: wuzhouhui @ 2020-01-04 1:18 UTC (permalink / raw)
To: git
Hi,
When I use "git log" to display commit log, I see there are
two commit IDs in a merge commit, e.g.
commit 2187f215ebaac73ddbd814696d7c7fa34f0c3de0
Merge: 2d3145f8d280 fbd542971aa1
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Tue Dec 17 13:27:02 2019 -0800
Merge tag 'for-5.5-rc2-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
In the previous example, we can see two commit IDs: 2d3145f8d280 and
fbd542971aa1. So, what's the meaning of them, precisely.
Thanks
^ permalink raw reply
* Re: git checkout -
From: Jonathan Nieder @ 2020-01-04 0:48 UTC (permalink / raw)
To: François WAUQUIER; +Cc: git
In-Reply-To: <CAFS-fjvhAB5EcfHhfp6HYN57W11tkHOc8K8T3oey8qceutuYsg@mail.gmail.com>
Hi,
François WAUQUIER wrote:
> $ git checkout -
>
> I often use this command to go back to previous branch from my history.
> It is quite natural as it uses the same syntax as “cd -“
>
> But i found out it is not documented in
> https://git-scm.com/docs/git-checkout/2.24.0
> I report this to help others to discover this time saving command.
Thanks for reporting!
Ideas for what the documentation should say about it? (Bonus points
if it comes in the form of a patch against Documentation/git-checkout.txt.
;-) See [1] for more about how that works.)
Sincerely,
Jonathan
[1] https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html
^ permalink raw reply
* Re: [PATCH] RFC: commit: add a commit.all-ignore-submodules config option
From: Jonathan Nieder @ 2020-01-04 0:45 UTC (permalink / raw)
To: marcandre.lureau; +Cc: git
In-Reply-To: <20200103120613.1063828-1-marcandre.lureau@redhat.com>
Hi,
Marc-André Lureau wrote:
> One of my most frequent mistake is to commit undesired submodules
> changes when doing "commit -a", and I have seen a number of people doing
> the same mistake in various projects. I wish there would be a config to
> change this default behaviour.
Can you say more about the overall workflow this is part of? What
causes the submodules to change state in the first place here?
[...]
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
[...]
> @@ -1475,6 +1478,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)
> return 0;
> }
>
> + if (!strcmp(k, "commit.all-ignore-submodules")) {
> + commit_all_ignore_submodules = git_config_bool(k, v);
> + return 0;
> + }
nit, less important than the comment above: no other config items use
this naming scheme. We'd have to come up with a different name if we
want to pursue this.
If I want to disable this setting for a particular "git commit"
invocation, how do I do that? Typically when adding new settings, we
add them first as command-line options and then as a separate followup
can introduce configuration to change the defaults.
To summarize: I'm interested in hearing more about the overall
workflow so we can make the standard behavior without any special
configuration work better for it, too.
Thanks and hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED
From: Jonathan Nieder @ 2020-01-04 0:13 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20200102201630.180969-1-jonathantanmy@google.com>
Jonathan Tan wrote:
> In a partial clone, if a user provides the hash of the empty tree ("git
> mktree </dev/null" - for SHA-1, this is 4b825d...) to a command which
> requires that that object be parsed, for example:
>
> git diff-tree 4b825d <a non-empty tree>
>
> then Git will lazily fetch the empty tree, unnecessarily, because
> parsing of that object invokes repo_has_object_file(), which does not
> special-case the empty tree.
>
> Instead, teach repo_has_object_file() to consult find_cached_object()
> (which handles the empty tree), thus bringing it in line with the rest
> of the object-store-accessing functions. A cost is
Lovely, thank you.
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> object-store.h | 2 --
> sha1-file.c | 38 ++++++++++++++++++--------------------
> 2 files changed, 18 insertions(+), 22 deletions(-)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
To follow up on Junio's hint in his review: callers can inject
additional cached objects by using pretend_object_file. Junio
described how this would make sense as a mechanism for building
the virtual ancestor object, but we don't do that. In fact, the
only caller is fake_working_tree_commit in "git blame", a read-only
code path. *phew*
-- >8 --
Subject: sha1-file: document how to use pretend_object_file
Like in-memory alternates, pretend_object_file contains a trap for the
unwary: careless callers can use it to create references to an object
that does not exist in the on-disk object store.
Add a comment documenting how to use the function without risking such
problems.
The only current caller is blame, which uses pretend_object_file to
create an in-memory commit representing the working tree state.
Noticed during a discussion of how to safely use this function in
operations like "git merge" which, unlike blame, are not read-only.
Inspired-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
object-store.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/object-store.h b/object-store.h
index 55ee639350..d0fc7b091b 100644
--- a/object-store.h
+++ b/object-store.h
@@ -208,6 +208,14 @@ int hash_object_file_literally(const void *buf, unsigned long len,
const char *type, struct object_id *oid,
unsigned flags);
+/*
+ * Add an object file to the in-memory object store, without writing it
+ * to disk.
+ *
+ * Callers are responsible for calling write_object_file to record the
+ * object in persistent storage before writing any other new objects
+ * that reference it.
+ */
int pretend_object_file(void *, unsigned long, enum object_type,
struct object_id *oid);
--
2.24.1.735.g03f4e72817
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox