Re: [RFC] cherry-pick notes to find out cherry-picks from the origin
On Wed, Oct 17, 2018 at 07:39:21AM -0700, Tejun Heo wrote: > A while ago, I proposed changes to name-rev and describe so that they > can identify the commits cherry-picked from the one which is being > shown. > > > https://public-inbox.org/git/20180726153714.gx1934...@devbig577.frc2.facebook.com/T/ > > While the use-cases - e.g. tracking down which release / stable > branches a given commit ended up in - weren't controversial, it was > suggested that it'd make more sense to use notes to link cherry-picks > instead of building the feature into name-rev. Sorry for the slow reply. This was on my to-look-at pile, but for some reason I accidentally put in my done pile. > The patch appended to this message implements most of it (sans tests > and documentation). It's composed of the following two parts. > > * A new built-in command note-cherry-picks, which walks the specified > commits and if they're marked with the cherry-pick trailer, adds the > backlink to the origin commit using Cherry-picked-to tag in a > cherry-picks note. That makes sense. I think this could also be an option to cherry-pick, to instruct it to create the note when the cherry-pick is made. But you may still want a command to backfill older cherry-picks, or those done by other people who do not care themselves about maintaining the notes tree. It _feels_ like this is something that should be do-able by plugging a few commands together, rather than writing a new C program. I.e., something like: git rev-list --format='%(trailers)' HEAD | perl -lne ' /^commit ([0-9]+)/ and $commit = $1; /^\(cherry picked from commit ([0-9]+)/ and print "$commit $1"; ' | while read from to; do # One process per note isn't very efficient. Ideally there would # be an "append --stdin" mode. Double points if it understands # how to avoid adding existing lines. git notes append -m "Cherry-picked-to: $to" $from done which is roughly what your program is doing. Not that I'm entirely opposed to doing something in C (we've been moving away from shell scripts anyway). But mostly I am wondering if we can leverage existing tools, and fill in their gaps in a way that lets people easily do similar things. And on that note... > * When formatting a cherry-picks note for display, nested cherry-picks > are followed from each Cherry-picked-to tag and printed out with > matching indentations. That makes sense to me, but does this have to be strictly related to cherry-picks? I.e., in the more generic form, could we have a way of marking a note as "transitive" for display, and the notes-display code would automatically recognize and walk hashes? That would serve your purpose, but would also allow similar things to easily be done in the future. > Combined with name-rev --stdin, it can produce outputs like the following. > [...] Yeah, that looks pretty good. > Locally, the notes can be kept up-to-date with a trivial post-commit > hook which invokes note-cherry-picks on the new commit; however, I'm > having a bit of trouble figuring out a way to keep it up-to-date when > multiple trees are involved. AFAICS, there are two options. > > 1. Ensuring that the notes are always generated on local commits and >whenever new commits are received through fetch/pulls. > > 2. Ensuring that the notes are always generated on local commits and >transported with push/pulls. > > 3. A hybrid approach - also generate notes on the receiving end and >ensure that fetch/pulls receives the notes together (ie. similar to >--tags option to git-fetch). > > #1 seems simpler and more robust to me. Unfortunately, I can't see a > way to implement any of the three options with the existing hooks. > For #1, there's no post-fetch hook. For #2 and #3, there doesn't seem > to be a fool-proof way to make sure that the notes are transported > together. Any suggestions would be greatly appreciated. Yeah, I think (1) is the simplest: it becomes a purely local thing that you've generated these annotations. Unfortunately, no, I don't think there's anything like a post-fetch hook. This might be a good reason to have one. One can always do "git fetch && update-notes" of course, but having fetch feed the script the set of updated ref tips would be very helpful (so you know you can traverse from $old..$new looking for cherry-picks). For (2) and (3), you can push/pull a notes tree, but setting up the refspecs and config is fairly manual. Using the cherry-pick option I suggested above would most locally-made picks, but you'd probably still need to backfill sometimes anyway. I don't think you'd need to be (nor am I sure you _could_ be) as fancy as fetching notes and their respective commits together. The notes are bound together in a single tree, so you cannot say "I don't have commit 1234abcd, I don't need the note for it". You get the whole tree. And that is not such a bad thing. The big reason _not_
Re: approxidate woes
On Thu, Nov 15, 2018 at 09:48:54AM -0500, Jeff King wrote:
> I don't think "48h" does what you expect either:
>
> $ t/helper/test-date approxidate now
> now -> 2018-11-15 14:43:32 +
>
> $ t/helper/test-date approxidate 48h
> 48h -> 2018-11-15 14:43:34 +
>
> $ t/helper/test-date approxidate 48.hours
> 48.hours -> 2018-11-13 14:43:38 +
Whoops, those should all be:
t/helper/test-tool date approxidate ...
in recent versions of Git (I was bisecting something earlier, and has a
crufty test-date left over from an old version!).
Adding new unit aliases would be something like:
diff --git a/date.c b/date.c
index 9bc15df6f9..eb477d1601 100644
--- a/date.c
+++ b/date.c
@@ -1016,6 +1016,7 @@ static const struct typelen {
{ "minutes", 60 },
{ "hours", 60*60 },
{ "days", 24*60*60 },
+ { "d", 24*60*60 },
{ "weeks", 7*24*60*60 },
{ NULL }
};
but I suspect we need to tighten up the string matching a bit. I think
that would allow "2 dogs" to be parsed as "days".
-Peff
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Thu, Nov 15, 2018 at 1:59 PM Johannes Schindelin wrote: > So yes, we are trying to unlink the `.lock` file, and as far as I can tell > that > `unlink()` call comes from the tempfile cleanup asked for by Martin. However, > as > we still have a handle open to that file, that call fails. > > I do not think that there is any better way to fix this than to close the file > explicitly. I may be talking nonsense here but I remember someone mentioned something about deleting a file while it's still open on Windows and after a quick internet search, it may be FILE_SHARE_DELETE. Since _we_ open these files, can we just open them with this to be able to delete them? -- Duy
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote:
> > I looked at the test to see if it would pass, but it is not even
> > checking anything about lockfiles! It just checks that we exit 1 by
> > returning up the callstack instead of calling die(). And of course it
> > would not have a problem under Linux either way. But if I run something
> > similar under strace, I see:
> >
> > $ strace ./git bundle create foobar.bundle HEAD..HEAD
> > [...]
> > openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock",
> > O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
> > [...]
> > close(3)= 0
> > unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
> > exit_group(128) = ?
> >
> > which seems right.
>
> Without the fix, the added regression test fails thusly:
>
> -- snip --
> [...]
> ++ test_expect_code 1 git bundle create foobar.bundle master..master
> ++ want_code=1
> ++ shift
> ++ git bundle create foobar.bundle master..master
> fatal: Refusing to create empty bundle.
> warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash
> directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied
Hmph. So who has it open, and why isn't the tempfile code working as
designed?
Aha, I see the problem. We dup() the descriptor in create_bundle(). So
the patch _is_ necessary and (fairly) correct. But the explanation
probably ought to be something like:
In create_bundle(), we duplicate the lockfile descriptor via dup().
This means that even though the lockfile code carefully calls close()
before unlinking the lockfile, we may still have the file open. Unix
systems don't care, but under Windows, this prevents the unlink
(causing an annoying warning and a stale lockfile).
But that also means that all of the other places we could die (e.g., in
write_or_die) are going to have the same problem. We've fixed only one.
Is there a way we can avoid doing the dup() in the first place?
The comment there explains that we duplicate because write_pack_data()
will close the descriptor. Unfortunately, that's hard to change because
it comes from run-command. But we don't actually need the descriptor
ourselves after it's closed; we're just trying to appease the lockfile
code; see e54c347c1c (create_bundle(): duplicate file descriptor to
avoid closing it twice, 2015-08-10).
We just need some reasonable way of telling the lock code what's
happening. Something like the patch below, which is a moral revert of
e54c347c1c, but instead wrapping the "lock->fd = -1" in an official API.
Does this make your warning go away?
diff --git a/bundle.c b/bundle.c
index 1ef584b93b..dc26551b83 100644
--- a/bundle.c
+++ b/bundle.c
@@ -244,7 +244,7 @@ static int is_tag_in_date_range(struct object *tag, struct
rev_info *revs)
/* Write the pack data to bundle_fd, then close it if it is > 1. */
-static int write_pack_data(int bundle_fd, struct rev_info *revs)
+static int write_pack_data(int bundle_fd, struct lock_file *lock, struct
rev_info *revs)
{
struct child_process pack_objects = CHILD_PROCESS_INIT;
int i;
@@ -256,6 +256,14 @@ static int write_pack_data(int bundle_fd, struct rev_info
*revs)
pack_objects.in = -1;
pack_objects.out = bundle_fd;
pack_objects.git_cmd = 1;
+
+ /*
+* At this point we know that start_command is going to close our
+* bundle_fd, whether successful or not. Tell the lock code that
+* it is no longer in charge of it, so we don't try to double-close.
+*/
+ lock_file_release_descriptor(lock);
+
if (start_command(_objects))
return error(_("Could not spawn pack-objects"));
@@ -421,21 +429,10 @@ int create_bundle(struct bundle_header *header, const
char *path,
bundle_to_stdout = !strcmp(path, "-");
if (bundle_to_stdout)
bundle_fd = 1;
- else {
+ else
bundle_fd = hold_lock_file_for_update(, path,
LOCK_DIE_ON_ERROR);
- /*
-* write_pack_data() will close the fd passed to it,
-* but commit_lock_file() will also try to close the
-* lockfile's fd. So make a copy of the file
-* descriptor to avoid trying to close it twice.
-*/
- bundle_fd = dup(bundle_fd);
- if (bundle_fd < 0)
- die_errno("unable to dup file descriptor");
- }
-
/* write signature */
write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
@@ -463,10 +460,8 @@ int create_bundle(struct bundle_header *header, const char
*path,
goto err;
/* write pack */
- if (write_pack_data(bundle_fd, )) {
- bundle_fd = -1; /* already closed by the above call */
+ if (write_pack_data(bundle_fd, , ))
goto err;
Re: [PATCH] technical doc: add a design doc for the evolve command
On Thu, Nov 15 2018, sxe...@google.com wrote: > +Detailed design > +=== > +Obsolescence information is stored as a graph of meta-commits. A meta-commit > is > +a specially-formatted merge commit that describes how one commit was created > +from others. > + > +Meta-commits look like this: > + > +$ git cat-file -p > +tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 > +parent aa7ce55545bf2c14bef48db91af1a74e2347539a > +parent d64309ee51d0af12723b6cb027fc9f195b15a5e9 > +parent 7e1bbcd3a0fa854a7a9eac9bf1eea6465de98136 > +author Stefan Xenos 1540841596 -0700 > +committer Stefan Xenos 1540841596 -0700 > +parent-type content > +parent-type obsolete > +parent-type origin > + > +This says “commit aa7ce555 makes commit d64309ee obsolete. It was created by > +cherry-picking commit 7e1bbcd3”. > + > +The tree for meta-commits is always the empty tree whose hash matches > +4b825dc642cb6eb9a060e54bf8d69288fbee4904 exactly, but future versions of git > may > +attach other trees here. For forward-compatibility fsck should ignore such > trees > +if found on future repository versions. Similarly, current versions of git > +should always fill in an empty commit comment and tools like fsck should > ignore > +the content of the commit comment if present in a future repository version. > +This will allow future versions of git to add metadata to the meta-commit > +comments or tree without breaking forwards compatibility. > + > +Parent-type > +--- > +The “parent-type” field in the commit header identifies a commit as a > +meta-commit and indicates the meaning for each of its parents. It is never > +present for normal commits. It is a list of enum values whose order matches > the > +order of the parents. Possible parent types are: > + > +- content: the content parent identifies the commit that this meta-commit is > + describing. > +- obsolete: indicates that this parent is made obsolete by the content > parent. > +- origin: indicates that this parent was generated from the given commit. > + > +There must be exactly one content parent for each meta-commit and it is > always > +be the first parent. The content commit will always be a normal commit and > not a > +meta-commit. However, future versions of git may create meta-commits for > other > +meta-commits and the fsck tool must be aware of this for forwards > compatibility. > + > +A meta-commit can have zero or more obsolete parents. An amend operation > creates > +a single obsolete parent. A merge used to resolve divergence (see divergence, > +below) will create multiple obsolete parents. A meta-commit may have zero > +obsolete parents if it describes a cherry-pick or squash merge that copies > one > +or more commits but does not replace them. > + > +A meta-commit can have zero or more origin parents. A cherry-pick creates a > +single origin parent. Certain types of squash merge will create multiple > origin > +parents. > + > +An obsolete parent or origin parent may be either a normal commit (indicating > +the oldest-known version of a change) or another meta-commit (for a change > that > +has already been modified one or more times). I think it's worth pointing out for those that are rusty on commit object details (but I checked) is that the reason for it not being: tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 parent aa7ce55545bf2c14bef48db91af1a74e2347539a parent-type content parent d64309ee51d0af12723b6cb027fc9f195b15a5e9 parent-type obsolete parent 7e1bbcd3a0fa854a7a9eac9bf1eea6465de98136 parent-type origin author Stefan Xenos 1540841596 -0700 committer Stefan Xenos 1540841596 -0700 Which would be easier to read, is that we're very sensitive to the order of the first few fields (tree -> parent -> author -> committer) and fsck will error out if we interjected a new field.
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 09:40:38AM +0100, Johannes Schindelin wrote: > > > From @chucklu: > > > > > my user case is like this : > > > > > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > > > are merge commits. Then I will get lots of popup like: > > > > > >The previous cherry-pick is now empty, possibly due to conflict > > >resolution. > > >If you wish to commit it anyway, use: > > > > > >git commit --allow-empty > > > > > >If you wish to skip this commit, use: > > > > > >git reset > > > > > >Then "git cherry-pick --continue" will resume cherry-picking > > >the remaining commits. > > > > My quick interpretation of this is that the user actually needs a way to > > skip silently commits which are now empty. > > If it's always intended to be used with cherry-pick, shouldn't > cherry-pick learn a --keep-empty (like rebase has)? That would avoid > even stopping for this case in the first place. I'd go for the other way round: --skip-empty. However, given the very unhappy turn in that Git for Windows ticket (somebody asks for a feature, then just sits back, and does not even confirm that the analysis covers their use case, let alone participates in this discussion), I am personally not really interested in driving this one any further. Tanushree proved that they know how to contribute to the Git mailing list, as a pre-requisite for the Outreachy project, and that is the positive outcome of this thread as far as I am concerned. I am pretty happy about that, too. Ciao, Dscho
Re: approxidate woes
On Thu, Nov 15, 2018 at 02:45:28PM +0100, Andreas Krey wrote: > I've now located why our backup repo shrinks every month: > > git gc --prune=2d > > doesn't do what I expected, and differs a lot from --prune=48h. Yeah, it understands "2 days", but not "d" as a unit. I don't think "48h" does what you expect either: $ t/helper/test-date approxidate now now -> 2018-11-15 14:43:32 + $ t/helper/test-date approxidate 48h 48h -> 2018-11-15 14:43:34 + $ t/helper/test-date approxidate 48.hours 48.hours -> 2018-11-13 14:43:38 + It might be reasonable to teach approxidate these obvious shorthands (one tricky one is "m"; normally I'd say "minute", but in Git timescales "month" is more likely). > Mildly irritating, and worse, hard to find in the documentation. > I failed at the latter and fell back to the sources, finding > './bin-wrappers/test-date approxidate' for trying. > > Where would I look? I don't think approxidate is really documented at all. It started as Linus's idea of "handle what people would probably say", and the fixes over the years have mostly been "eh, that's crazy, let's do better with this input". You'd have to reverse engineer it a bit from the source, unfortunately. -Peff
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote: > I cannot claim that I wrapped my head around your explanation or your diff (I > am > busy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0), > but it does fix the problem. Thank you so much! > > The line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code > 128`, of course, and to discern from the fixed problem (which also exits with > code 128), the error output should be verified, like so: > > -- snip -- > test_expect_success 'try to create a bundle with empty ref count' ' > test_must_fail git bundle create foobar.bundle master..master 2>err && > test_i18ngrep "Refusing to create empty bundle" err > ' > -- snap -- It seems like we should be checking that the stale lockfile isn't left, which is the real problem (the warning is annoying, but is a symptom of the same thing). I.e., something like: test_must_fail git bundle create foobar.bundle master..master && test_path_is_missing foobar.bundle.lock That would already pass on non-Windows platforms, but that's OK. It will never give a false failure. If you don't mind, can you confirm that the test above fails without either of the two patches under discussion? > Do you want to integrate this test into your patch and run with it, or do you > want me to shepherd your patch? I'll wrap it up with a commit message and a test. -Peff
approxidate woes
Hi everybody, I've now located why our backup repo shrinks every month: git gc --prune=2d doesn't do what I expected, and differs a lot from --prune=48h. The latter actually means 'older than two days', while the former is 'since the second day of this month, same time as now'. Even '2d ago' does not help - '2 days ago' does. Mildly irritating, and worse, hard to find in the documentation. I failed at the latter and fell back to the sources, finding './bin-wrappers/test-date approxidate' for trying. Where would I look? - Andreas -- "Totally trivial. Famous last words." From: Linus Torvalds Date: Fri, 22 Jan 2010 07:29:21 -0800
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
Hi Peff,
On Thu, 15 Nov 2018, Jeff King wrote:
> On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote:
>
> > > I looked at the test to see if it would pass, but it is not even
> > > checking anything about lockfiles! It just checks that we exit 1 by
> > > returning up the callstack instead of calling die(). And of course it
> > > would not have a problem under Linux either way. But if I run something
> > > similar under strace, I see:
> > >
> > > $ strace ./git bundle create foobar.bundle HEAD..HEAD
> > > [...]
> > > openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock",
> > > O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
> > > [...]
> > > close(3)= 0
> > > unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
> > > exit_group(128) = ?
> > >
> > > which seems right.
> >
> > Without the fix, the added regression test fails thusly:
> >
> > -- snip --
> > [...]
> > ++ test_expect_code 1 git bundle create foobar.bundle master..master
> > ++ want_code=1
> > ++ shift
> > ++ git bundle create foobar.bundle master..master
> > fatal: Refusing to create empty bundle.
> > warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash
> > directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied
>
> Hmph. So who has it open, and why isn't the tempfile code working as
> designed?
>
> Aha, I see the problem. We dup() the descriptor in create_bundle(). So
> the patch _is_ necessary and (fairly) correct. But the explanation
> probably ought to be something like:
>
> In create_bundle(), we duplicate the lockfile descriptor via dup().
> This means that even though the lockfile code carefully calls close()
> before unlinking the lockfile, we may still have the file open. Unix
> systems don't care, but under Windows, this prevents the unlink
> (causing an annoying warning and a stale lockfile).
>
> But that also means that all of the other places we could die (e.g., in
> write_or_die) are going to have the same problem. We've fixed only one.
> Is there a way we can avoid doing the dup() in the first place?
>
> The comment there explains that we duplicate because write_pack_data()
> will close the descriptor. Unfortunately, that's hard to change because
> it comes from run-command. But we don't actually need the descriptor
> ourselves after it's closed; we're just trying to appease the lockfile
> code; see e54c347c1c (create_bundle(): duplicate file descriptor to
> avoid closing it twice, 2015-08-10).
>
> We just need some reasonable way of telling the lock code what's
> happening. Something like the patch below, which is a moral revert of
> e54c347c1c, but instead wrapping the "lock->fd = -1" in an official API.
>
> Does this make your warning go away?
>
> diff --git a/bundle.c b/bundle.c
> [...]
I cannot claim that I wrapped my head around your explanation or your diff (I am
busy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0),
but it does fix the problem. Thank you so much!
The line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code
128`, of course, and to discern from the fixed problem (which also exits with
code 128), the error output should be verified, like so:
-- snip --
test_expect_success 'try to create a bundle with empty ref count' '
test_must_fail git bundle create foobar.bundle master..master 2>err &&
test_i18ngrep "Refusing to create empty bundle" err
'
-- snap --
Do you want to integrate this test into your patch and run with it, or do you
want me to shepherd your patch?
Ciao,
Dscho
Re: Confusing behavior with ignored submodules and `git commit -a`
> I have a git repository which contains a number of submodules that > refer to external repositories. Some of these repositories need to > patched in some way, so patches are stored alongside the submodules, > and are applied when building. This mostly works fine, but causes > submodules to show up as modified in `git status` and get updated with > `git commit -a`. To resolve this, I've added `ignore = all` to > .gitmodules for all the submodules that need patches applied. This > way, I can explicitly `git add` the submodule when I want to update > the base commit, but otherwise pretend that they are clean. This has > worked pretty well for me, but less so since git 2.15 when this issue > was introduced. > > This is really bad. git-status and git-commit share some code, > > and we'll populate the commit message with a status output. > > So it seems reasonable to expect the status and the commit to match, > > i.e. if status tells me there is no change, then commit should not record > > the submodule update. > > I just checked and if I don't specify a message on the command-line, > the status output in the message template *does* mention that `inner` > is getting updated. That's good. > >> > There have been a couple occasions where I accidentally pushed local > >> > changes to ignored submodules because of this. Since they don't show > >> > up in the log output, it is difficult to figure out what actually has > >> > gone wrong. > > > > How was it prevented before? Just by git commit -a not picking up the > > submodule change? > > Yes. Previously, `git commit -a` would not pick up the change (unless > I added it explicitly with `git add`), and `git log` would still show > changes to ignored submodules (which is the behavior I want). and both are broken currently (commit -a will commit a submodule if it is changed, and it will also not show that in log, but it did show that it is committing it in the commit message template) > I just came across someone else affected by this issue: > https://github.com/git/git/commit/55568086#commitcomment-27137460 Point taken.
Re: [PATCH 18/23] submodule: use submodule repos for object lookup
On Thu, Nov 15, 2018 at 11:54 AM Jonathan Tan wrote:
>
> > +/*
> > + * Initialize 'out' based on the provided submodule path.
> > + *
> > + * Unlike repo_submodule_init, this tolerates submodules not present
> > + * in .gitmodules. This function exists only to preserve historical
> > behavior,
> > + *
> > + * Returns 0 on success, -1 when the submodule is not present.
> > */
> > -static void show_submodule_header(struct diff_options *o, const char *path,
> > +static struct repository *open_submodule(const char *path)
>
> The function documentation needs to be reworded - there's no "out", and
> the return value is now a possibly NULL pointer to struct repository.
Noted.
>
> > +{
> > + struct strbuf sb = STRBUF_INIT;
> > + struct repository *out = xmalloc(sizeof(*out));
> > +
> > + if (submodule_to_gitdir(, path) || repo_init(out, sb.buf, NULL)) {
> > + strbuf_release();
> > + free(out);
> > + return NULL;
> > + }
> > +
> > + out->submodule_prefix = xstrdup(path);
>
> I've discussed this submodule_prefix line before [1] - do we really need
> this? Tests pass even if I remove this line.
We might not need it yet as the tests indicate, but it's the right thing to do:
/*
* Path from the root of the top-level superproject down to this
* repository. This is only non-NULL if the repository is initialized
* as a submodule of another repository.
*/
We're not (yet) using this string in our error reporting, but
I anticipate that we'll do eventually.
> Other than that, this patch looks good.
Thanks,
Stefan
insteadOf and git-request-pull output
Hi, all: Looks like setting url.insteadOf rules alters the output of git-request-pull. I'm not sure that's the intended use of insteadOf, which is supposed to replace URLs for local use, not to expose them publicly (but I may be wrong). E.g.: $ git request-pull HEAD^ git://foo.example.com/example | grep example git://foo.example.com/example $ git config url.ssh://bar.insteadOf git://foo $ git request-pull HEAD^ git://foo.example.com/example | grep example ssh://bar.example.com/example I think that if we use the "principle of least surprise," insteadOf rules shouldn't be applied for git-request-pull URLs. Best, -K
Re: insteadOf and git-request-pull output
On Thu, Nov 15 2018, Konstantin Ryabitsev wrote: > Hi, all: > > Looks like setting url.insteadOf rules alters the output of > git-request-pull. I'm not sure that's the intended use of insteadOf, > which is supposed to replace URLs for local use, not to expose them > publicly (but I may be wrong). E.g.: > > $ git request-pull HEAD^ git://foo.example.com/example | grep example > git://foo.example.com/example > > $ git config url.ssh://bar.insteadOf git://foo > > $ git request-pull HEAD^ git://foo.example.com/example | grep example > ssh://bar.example.com/example > > I think that if we use the "principle of least surprise," insteadOf > rules shouldn't be applied for git-request-pull URLs. I haven't used request-pull so I don't have much of an opinion on this, but do you think the same applies to 'git remote get-url '? I.e. should it also show the original unmunged URL, or the munged one as it does now?
Re: insteadOf and git-request-pull output
On Thu, Nov 15, 2018 at 07:54:32PM +0100, Ævar Arnfjörð Bjarmason wrote: > > I think that if we use the "principle of least surprise," insteadOf > > rules shouldn't be applied for git-request-pull URLs. > > I haven't used request-pull so I don't have much of an opinion on this, > but do you think the same applies to 'git remote get-url '? > > I.e. should it also show the original unmunged URL, or the munged one as > it does now? I don't know, maybe both? As opposed to git-request-pull, this is not exposing the insteadOf URL to someone other than the person who set it up, so even if it does return the munged URL, it wouldn't be unexpected. -K
Re: [PATCH 18/23] submodule: use submodule repos for object lookup
> +/*
> + * Initialize 'out' based on the provided submodule path.
> + *
> + * Unlike repo_submodule_init, this tolerates submodules not present
> + * in .gitmodules. This function exists only to preserve historical behavior,
> + *
> + * Returns 0 on success, -1 when the submodule is not present.
> */
> -static void show_submodule_header(struct diff_options *o, const char *path,
> +static struct repository *open_submodule(const char *path)
The function documentation needs to be reworded - there's no "out", and
the return value is now a possibly NULL pointer to struct repository.
> +{
> + struct strbuf sb = STRBUF_INIT;
> + struct repository *out = xmalloc(sizeof(*out));
> +
> + if (submodule_to_gitdir(, path) || repo_init(out, sb.buf, NULL)) {
> + strbuf_release();
> + free(out);
> + return NULL;
> + }
> +
> + out->submodule_prefix = xstrdup(path);
I've discussed this submodule_prefix line before [1] - do we really need
this? Tests pass even if I remove this line.
Other than that, this patch looks good.
[1]
https://public-inbox.org/git/20181019203750.110741-1-jonathanta...@google.com/
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote: > > > I cannot claim that I wrapped my head around your explanation or your > > diff (I am busy trying to prepare Git for Windows' `master` for > > rebasing to v2.20.0-rc0), but it does fix the problem. Thank you so > > much! > > > > The line `test_expect_code 1 ...` needs to be adjusted to > > `test_expect_code 128`, of course, and to discern from the fixed > > problem (which also exits with code 128), the error output should be > > verified, like so: > > > > -- snip -- > > test_expect_success 'try to create a bundle with empty ref count' ' > > test_must_fail git bundle create foobar.bundle master..master 2>err && > > test_i18ngrep "Refusing to create empty bundle" err > > ' > > -- snap -- > > It seems like we should be checking that the stale lockfile isn't left, > which is the real problem (the warning is annoying, but is a symptom of > the same thing). I.e., something like: > > test_must_fail git bundle create foobar.bundle master..master && > test_path_is_missing foobar.bundle.lock > > That would already pass on non-Windows platforms, but that's OK. It will > never give a false failure. > > If you don't mind, can you confirm that the test above fails without > either of the two patches under discussion? This test succeeds with your patch as well as with Gaël's, and fails when neither patch is applied. So you're right, it is the much better test. > > Do you want to integrate this test into your patch and run with it, or > > do you want me to shepherd your patch? > > I'll wrap it up with a commit message and a test. Thank you so much! Dscho
Re: Confusing behavior with ignored submodules and `git commit -a`
On 2018-11-15, Stefan Beller wrote:
> On Wed, Nov 14, 2018 at 10:05 PM Michael Forney
> wrote:
>> Looking at ff6f1f564c, I don't really see anything that might be
>> related to git-add, git-reset, or git-diff, so I'm guessing that this
>> only worked before because the submodule config wasn't getting loaded
>> during `git add` or `git reset`. Now that the config is loaded
>> automatically, submodule..ignore started taking effect where it
>> shouldn't.
>>
>> Unfortunately, this doesn't really get me much closer to finding a fix.
>
> Maybe selectively unloading or overwriting the config?
>
> Or we can change is_submodule_ignored() in diff.c
> to be only applied selectively whether we are running the
> right command? For this approach we'd have to figure out the
> set of commands to which the ignore config should apply or
> not (and come up with a more concise documentation then)
>
> This approach sounds appealing to me as it would cover
> new commands as well and we'd only have a central point
> where the decision for ignoring is made.
Well, currently the submodule config can be disabled in diff_flags by
setting override_submodule_config=1. However, I'm thinking it may be
simpler to selectively *enable* the submodule config in diff_flags
where it is needed instead of disabling it everywhere else (i.e.
use_submodule_config instead of override_submodule_config).
I'm also starting to see why this is tricky. The only difference that
diff.c:run_diff_files sees between `git add inner` and `git add --all`
is whether the index entry matched the pathspec exactly or not.
Here is a work-in-progress diff that seems to have the correct
behavior in all cases I tried. Can you think of any cases that it
breaks? I'm not quite sure of the consequences of having diff_change
and diff_addremove always ignore the submodule config; git-diff and
git-status still seem to work correctly.
diff --git a/builtin/add.c b/builtin/add.c
index f65c17229..9902f7742 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
rev.diffopt.format_callback_data =
- rev.diffopt.flags.override_submodule_config = 1;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(, DIFF_RACY_IS_MODIFIED);
clear_pathspec(_data);
diff --git a/diff-lib.c b/diff-lib.c
index 83fce5151..fbb048cca 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry
*ce, struct stat *st)
static int match_stat_with_submodule(struct diff_options *diffopt,
const struct cache_entry *ce,
struct stat *st, unsigned ce_option,
-unsigned *dirty_submodule)
+unsigned *dirty_submodule,
+int exact)
{
int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
if (S_ISGITLINK(ce->ce_mode)) {
struct diff_flags orig_flags = diffopt->flags;
- if (!diffopt->flags.override_submodule_config)
+ if (!diffopt->flags.override_submodule_config && !exact)
set_diffopt_flags_from_submodule_config(diffopt,
ce->name);
if (diffopt->flags.ignore_submodules)
changed = 0;
@@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct
diff_options *diffopt,
int run_diff_files(struct rev_info *revs, unsigned int option)
{
- int entries, i;
+ int entries, i, matched;
int diff_unmerged_stage = revs->max_count;
unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
? CE_MATCH_RACY_IS_DIRTY : 0);
@@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned
int option)
if (diff_can_quit_early(>diffopt))
break;
- if (!ce_path_match(istate, ce, >prune_data, NULL))
+ matched = ce_path_match(istate, ce, >prune_data, NULL);
+ if (!matched)
continue;
if (ce_stage(ce)) {
@@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned
int option)
}
changed = match_stat_with_submodule(>diffopt, ce,
,
- ce_option,
_submodule);
+ ce_option,
_submodule,
+ matched ==
MATCHED_EXACTLY);
newmode = ce_mode_from_stat(ce, st.st_mode);
}
@@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce,
return -1;
}
Re: [PATCH 1/1] mingw: replace an obsolete link with the superseding one
Am 15.11.18 um 12:22 schrieb Johannes Schindelin via GitGitGadget:
From: Johannes Schindelin
The MSDN documentation has been superseded by Microsoft Docs (which is
backed by a repository on GitHub containing many, many files in Markdown
format).
Signed-off-by: Johannes Schindelin
---
compat/mingw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index d2f4fabb44..9e42b0ee26 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1028,8 +1028,8 @@ char *mingw_getcwd(char *pointer, int len)
}
/*
- * See http://msdn2.microsoft.com/en-us/library/17w5ykft(vs.71).aspx
- * (Parsing C++ Command-Line Arguments)
+ * See "Parsing C++ Command-Line Arguments" at Microsoft's Docs:
+ * https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments
*/
static const char *quote_arg(const char *arg)
{
Thank you! That's much better than the original obfuscated page name.
-- Hannes
Re: Confusing behavior with ignored submodules and `git commit -a`
On Wed, Nov 14, 2018 at 10:05 PM Michael Forney wrote: > > +bmwill > > On 2018-11-14, Michael Forney wrote: > > On 2018-10-25, Stefan Beller wrote: > >> I guess reverting that commit is not a good idea now, as > >> I would expect something to break. > >> > >> Maybe looking through the series 614ea03a71 > >> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26) > >> to understand why it happened in the context would be a good start. > > > > Thanks, that's a good idea. I'll take a look through that series. > > Interesting. If I build git from master after reverting 55568086, I do > indeed observe the issue it claims to fix (unable to add ignored > submodules). However, if I build from 9ef23f91fc (the immediate parent > of 55568086), I do not see the issue. > > Investigating this further, it seems that 55568086 addresses an issue > that does not appear until later on in the series in ff6f1f564c > (submodule-config: lazy-load a repository's .gitmodules file). Perhaps > this was a result of reordering commits during a rebase. In other > words, I get correct behavior until 55568086, and in > 55568086..ff6f1f564c^ if I revert 55568086. > > Looking at ff6f1f564c, I don't really see anything that might be > related to git-add, git-reset, or git-diff, so I'm guessing that this > only worked before because the submodule config wasn't getting loaded > during `git add` or `git reset`. Now that the config is loaded > automatically, submodule..ignore started taking effect where it > shouldn't. > > Unfortunately, this doesn't really get me much closer to finding a fix. Maybe selectively unloading or overwriting the config? Or we can change is_submodule_ignored() in diff.c to be only applied selectively whether we are running the right command? For this approach we'd have to figure out the set of commands to which the ignore config should apply or not (and come up with a more concise documentation then) This approach sounds appealing to me as it would cover new commands as well and we'd only have a central point where the decision for ignoring is made. Stefan
Re: Confusing behavior with ignored submodules and `git commit -a`
On 2018-11-15, Michael Forney wrote:
> Here is a work-in-progress diff that seems to have the correct
> behavior in all cases I tried.
I was hoping that gmail wouldn't mess with the whitespace, but apparently
it has, sorry about that. Let me try again.
---
builtin/add.c | 1 -
diff-lib.c| 15 +--
diff.c| 22 ++
3 files changed, 11 insertions(+), 27 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index f65c17229..9902f7742 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
rev.diffopt.format_callback_data =
- rev.diffopt.flags.override_submodule_config = 1;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(, DIFF_RACY_IS_MODIFIED);
clear_pathspec(_data);
diff --git a/diff-lib.c b/diff-lib.c
index 83fce5151..fbb048cca 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry *ce,
struct stat *st)
static int match_stat_with_submodule(struct diff_options *diffopt,
const struct cache_entry *ce,
struct stat *st, unsigned ce_option,
-unsigned *dirty_submodule)
+unsigned *dirty_submodule,
+int exact)
{
int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
if (S_ISGITLINK(ce->ce_mode)) {
struct diff_flags orig_flags = diffopt->flags;
- if (!diffopt->flags.override_submodule_config)
+ if (!diffopt->flags.override_submodule_config && !exact)
set_diffopt_flags_from_submodule_config(diffopt,
ce->name);
if (diffopt->flags.ignore_submodules)
changed = 0;
@@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct diff_options
*diffopt,
int run_diff_files(struct rev_info *revs, unsigned int option)
{
- int entries, i;
+ int entries, i, matched;
int diff_unmerged_stage = revs->max_count;
unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
? CE_MATCH_RACY_IS_DIRTY : 0);
@@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
if (diff_can_quit_early(>diffopt))
break;
- if (!ce_path_match(istate, ce, >prune_data, NULL))
+ matched = ce_path_match(istate, ce, >prune_data, NULL);
+ if (!matched)
continue;
if (ce_stage(ce)) {
@@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
}
changed = match_stat_with_submodule(>diffopt, ce,
,
- ce_option,
_submodule);
+ ce_option,
_submodule,
+ matched ==
MATCHED_EXACTLY);
newmode = ce_mode_from_stat(ce, st.st_mode);
}
@@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce,
return -1;
}
changed = match_stat_with_submodule(diffopt, ce, ,
- 0, dirty_submodule);
+ 0, dirty_submodule, 0);
if (changed) {
mode = ce_mode_from_stat(ce, st.st_mode);
oid = _oid;
diff --git a/diff.c b/diff.c
index e38d1ecaf..73dc75286 100644
--- a/diff.c
+++ b/diff.c
@@ -6209,24 +6209,6 @@ int diff_can_quit_early(struct diff_options *opt)
opt->flags.has_changes);
}
-/*
- * Shall changes to this submodule be ignored?
- *
- * Submodule changes can be configured to be ignored separately for each path,
- * but that configuration can be overridden from the command line.
- */
-static int is_submodule_ignored(const char *path, struct diff_options *options)
-{
- int ignored = 0;
- struct diff_flags orig_flags = options->flags;
- if (!options->flags.override_submodule_config)
- set_diffopt_flags_from_submodule_config(options, path);
- if (options->flags.ignore_submodules)
- ignored = 1;
- options->flags = orig_flags;
- return ignored;
-}
-
void diff_addremove(struct diff_options *options,
int addremove, unsigned mode,
const struct object_id *oid,
@@ -6235,7 +6217,7 @@ void diff_addremove(struct diff_options *options,
{
struct diff_filespec *one, *two;
- if
Re: Confusing behavior with ignored submodules and `git commit -a`
On Thu, Nov 15, 2018 at 1:33 PM Michael Forney wrote:
>
> On 2018-11-15, Stefan Beller wrote:
> > On Wed, Nov 14, 2018 at 10:05 PM Michael Forney
> > wrote:
> >> Looking at ff6f1f564c, I don't really see anything that might be
> >> related to git-add, git-reset, or git-diff, so I'm guessing that this
> >> only worked before because the submodule config wasn't getting loaded
> >> during `git add` or `git reset`. Now that the config is loaded
> >> automatically, submodule..ignore started taking effect where it
> >> shouldn't.
> >>
> >> Unfortunately, this doesn't really get me much closer to finding a fix.
> >
> > Maybe selectively unloading or overwriting the config?
> >
> > Or we can change is_submodule_ignored() in diff.c
> > to be only applied selectively whether we are running the
> > right command? For this approach we'd have to figure out the
> > set of commands to which the ignore config should apply or
> > not (and come up with a more concise documentation then)
> >
> > This approach sounds appealing to me as it would cover
> > new commands as well and we'd only have a central point
> > where the decision for ignoring is made.
>
> Well, currently the submodule config can be disabled in diff_flags by
> setting override_submodule_config=1. However, I'm thinking it may be
> simpler to selectively *enable* the submodule config in diff_flags
> where it is needed instead of disabling it everywhere else (i.e.
> use_submodule_config instead of override_submodule_config).
This sounds like undoing the good(?) part of the series that introduced
this regression, as before that we selectively loaded the submodule
config, which lead to confusion when you forgot it. Selectively *enabling*
the submodule config sounds like that state before?
Or do we *only* talk about enabling the ignore flag, while loading the
rest of the submodule config automatic?
> I'm also starting to see why this is tricky. The only difference that
> diff.c:run_diff_files sees between `git add inner` and `git add --all`
> is whether the index entry matched the pathspec exactly or not.
Unrelated to the trickiness, I think we'd need to document the behavior
of the -a flag in git-add and git-commit better as adding the diff below
will depart from the "all" rule again, which I thought was a strong
motivator for Brandons series (IIRC).
> Here is a work-in-progress diff that seems to have the correct
> behavior in all cases I tried. Can you think of any cases that it
> breaks? I'm not quite sure of the consequences of having diff_change
> and diff_addremove always ignore the submodule config; git-diff and
> git-status still seem to work correctly.
>
> diff --git a/builtin/add.c b/builtin/add.c
> index f65c17229..9902f7742 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,
> rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
> rev.diffopt.format_callback = update_callback;
> rev.diffopt.format_callback_data =
> - rev.diffopt.flags.override_submodule_config = 1;
This line partially reverts 5556808, taking 02f2f56bc377c28
into account.
> diff --git a/diff-lib.c b/diff-lib.c
> index 83fce5151..fbb048cca 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry
> *ce, struct stat *st)
> static int match_stat_with_submodule(struct diff_options *diffopt,
> const struct cache_entry *ce,
> struct stat *st, unsigned ce_option,
> -unsigned *dirty_submodule)
> +unsigned *dirty_submodule,
> +int exact)
> [...];
This is an interesting take so far as it is all about *detecting* change
here via stat information and not like the previous (before the regression)
where it was about correcting output.
match_stat_with_submodule would grow its documentation to be
slightly more complicated as a result.
> diff --git a/diff.c b/diff.c
> index e38d1ecaf..73dc75286 100644
> --- a/diff.c
> +++ b/diff.c
> [...]
> -static int is_submodule_ignored(const char *path, struct diff_options
> *options)
> -{
> [...]
> - if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))
> + if (S_ISGITLINK(mode) && options->flags.ignore_submodules)
> return;
This basically inlines the function is_submodule_ignored,
except for the part:
if (!options->flags.override_submodule_config)
set_diffopt_flags_from_submodule_config(options, path);
but that was taken care off in match_stat_with_submodule in diff-lib?
This WIP looks really promising, thanks for looking into this!
Stefan
Re: Confusing behavior with ignored submodules and `git commit -a`
On 2018-11-15, Stefan Beller wrote:
> On Thu, Nov 15, 2018 at 1:33 PM Michael Forney wrote:
>> Well, currently the submodule config can be disabled in diff_flags by
>> setting override_submodule_config=1. However, I'm thinking it may be
>> simpler to selectively *enable* the submodule config in diff_flags
>> where it is needed instead of disabling it everywhere else (i.e.
>> use_submodule_config instead of override_submodule_config).
>
> This sounds like undoing the good(?) part of the series that introduced
> this regression, as before that we selectively loaded the submodule
> config, which lead to confusion when you forgot it. Selectively *enabling*
> the submodule config sounds like that state before?
>
> Or do we *only* talk about enabling the ignore flag, while loading the
> rest of the submodule config automatic?
Yes, that is what I meant. I believe the automatic loading of
submodule config is the right thing to do, it just uncovered cases
where the current handling of override_submodule_config is not quite
sufficient.
My suggestion of replacing override_submodule_config with
use_submodule_config is because it seems like there are fewer places
where we want to apply the ignore config than not. I think it should
only apply in diffs against the working tree and when staging changes
to the index (unless specified explicitly). The documentation just
mentions the "diff family", but all but one of the possible values for
submodule..ignore ("all") don't make sense unless comparing with
the working tree. This is also how show/log -p behaved in git <2.15.
So I think that clarifying that it is about modifications *to the
working tree* would be a good idea.
>> I'm also starting to see why this is tricky. The only difference that
>> diff.c:run_diff_files sees between `git add inner` and `git add --all`
>> is whether the index entry matched the pathspec exactly or not.
>
> Unrelated to the trickiness, I think we'd need to document the behavior
> of the -a flag in git-add and git-commit better as adding the diff below
> will depart from the "all" rule again, which I thought was a strong
> motivator for Brandons series (IIRC).
Can you explain what you mean by the "all" rule?
>> Here is a work-in-progress diff that seems to have the correct
>> behavior in all cases I tried. Can you think of any cases that it
>> breaks? I'm not quite sure of the consequences of having diff_change
>> and diff_addremove always ignore the submodule config; git-diff and
>> git-status still seem to work correctly.
>>
>> diff --git a/builtin/add.c b/builtin/add.c
>> index f65c17229..9902f7742 100644
>> --- a/builtin/add.c
>> +++ b/builtin/add.c
>> @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,
>> rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
>> rev.diffopt.format_callback = update_callback;
>> rev.diffopt.format_callback_data =
>> - rev.diffopt.flags.override_submodule_config = 1;
>
> This line partially reverts 5556808, taking 02f2f56bc377c28
> into account.
Correct. The problem with 55568086 is that add_files_to_cache is used
by both git-add and git-commit, regardless of whether --all was
specified. So, while it made it possible to stage ignored submodules,
it also made it so the submodule ignore config was overridden in all
uses of git-add and git-commit.
So, this diff attempts to make it so the ignore config is only applied
when the pathspec matches exactly rather than just always overriding
the ignore config.
>> diff --git a/diff-lib.c b/diff-lib.c
>> index 83fce5151..fbb048cca 100644
>> --- a/diff-lib.c
>> +++ b/diff-lib.c
>> @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry
>> *ce, struct stat *st)
>> static int match_stat_with_submodule(struct diff_options *diffopt,
>> const struct cache_entry *ce,
>> struct stat *st, unsigned ce_option,
>> -unsigned *dirty_submodule)
>> +unsigned *dirty_submodule,
>> +int exact)
>> [...];
>
> This is an interesting take so far as it is all about *detecting* change
> here via stat information and not like the previous (before the regression)
> where it was about correcting output.
>
> match_stat_with_submodule would grow its documentation to be
> slightly more complicated as a result.
Yes, this is one part I'm not quite happy with. I wonder if instead
match_stat_with_submodule could be made simpler by moving the
ie_match_stat call up to the two call sites, and then it could be
selectively called by run_diff_files depending on the value of
matched.
>> diff --git a/diff.c b/diff.c
>> index e38d1ecaf..73dc75286 100644
>> --- a/diff.c
>> +++ b/diff.c
>> [...]
>> -static int is_submodule_ignored(const char *path, struct diff_options
>> *options)
>> -{
>> [...]
>> - if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath,
>> options))
Re: [PATCHv3 00/23] Bring more repository handles into our code base
> Please have a look at the last 4 patches specifically as they were new in > the last iteration (but did not receive any comment), as they demonstrate > and fix a problem that is only exposed when using GIT_TEST_COMMIT_GRAPH=1 > for the test suite. Thanks. I only reviewed patches 18 and 20-23, as only those are different from the previous iteration according to the range-diff. I've written my comments about patch 18 already [1], and the other patches look good to me. In patch 21, I could go either way about whether it's more desirable to pass the pool or the repository to the freeing functions. Thanks for discovering the issue that patch 23 illustrates. I thought that the tests were written carefully enough in that the_repository didn't have any relevant objects or configurations (all relevant data was in a path that is not the default repository), but apparently some still slipped through. [1] https://public-inbox.org/git/20181115195416.21890-1-jonathanta...@google.com/
Re: [PATCH] remote-curl: die on server-side errors
On 2018.11.14 02:00, Jeff King wrote:
> On Tue, Nov 13, 2018 at 07:49:15PM -0500, Jeff King wrote:
>
> > Yes, the packet_read_line_buf() interface will both advance the buf
> > pointer and decrement the length. So if we want to "peek", we have to
> > do so with a copy (there's a peek function if you use the packet_reader
> > interface, but that might be overkill here).
> >
> > You can rewrite it like this, which is a pretty faithful conversion and
> > passes the tests (but see below).
> > [...]
>
> Here's a version which is less faithful, but I think does sensible
> things in all cases, and is much easier to follow. I get a little
> nervous just because it tightens some cases, and one never knows if
> other implementations might be relying on the looseness. E.g.:
>
> - in the current code we'd still drop back to dumb http if the server
> tells us "application/x-git-upload-pack" but the initial pktline
> doesn't start with "#" (even though if it _does_ have "#" at
> position 5 but isn't a valid pktline, we'd complain!)
>
> - right now the v2 protocol does not require the server to say
> "application/x-git-upload-pack" for the content-type
>
> This patch tightens both of those (I also made a few stylistic tweaks,
> and added the ERR condition to show where it would go). I dunno. Part of
> me sees this as a nice cleanup, but maybe it is better to just leave it
> alone. A lot of these behaviors are just how it happens to work now, and
> not part of the spec, but we don't know what might be relying on them.
At least according to the protocol-v2 and http-protocol docs, the
stricter behavior seems correct:
For the first point above, dumb servers should never use an
"application/x-git-*" content type (http-protocol.txt line 163-167).
For the second point, the docs require v2 servers to use
"application/x-git-*" content types. protocol-v2.txt lines 63-65 state
that v2 clients should make a smart http request, while
http-protocol.txt lines 247-252 state that a smart server's response
type must be "application/x-git-*".
Of course we don't know if other implementations follow the spec, but
ISTM that this patch at least doesn't contradict how we've promised the
protocols should work.
If no one has any objections, I'll include the diff below in v2. Thanks
for the help Jeff!
> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..1adb96311b 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -330,9 +330,61 @@ static int get_protocol_http_header(enum
> protocol_version version,
> return 0;
> }
>
> +static void check_smart_http(struct discovery *d, const char *service,
> + struct strbuf *type)
> +{
> + char *src_buf;
> + size_t src_len;
> + char *line;
> + const char *p;
> +
> + if (!skip_prefix(type->buf, "application/x-", ) ||
> + !skip_prefix(p, service, ) ||
> + strcmp(p, "-advertisement"))
> + return;
> +
> + /*
> + * We speculatively try to read a packet, which means we must preserve
> + * the original buf/len pair in some cases.
> + */
> + src_buf = d->buf;
> + src_len = d->len;
> + line = packet_read_line_buf(_buf, _len, NULL);
> + if (!line)
> + die("invalid server response; expected service, got flush
> packet");
> +
> + if (skip_prefix(line, "# service=", ) && !strcmp(p, service)) {
> + /*
> + * The header can include additional metadata lines, up
> + * until a packet flush marker. Ignore these now, but
> + * in the future we might start to scan them.
> + */
> + while (packet_read_line_buf(_buf, _len, NULL))
> + ;
> +
> + /*
> + * v0 smart http; callers expect us to soak up the
> + * service and header packets
> + */
> + d->buf = src_buf;
> + d->len = src_len;
> + d->proto_git = 1;
> +
> + } else if (starts_with(line, "version 2")) { /* should be strcmp()? */
> + /*
> + * v2 smart http; do not consume version packet, which will
> + * be handled elsewhere.
> + */
> + d->proto_git = 1;
> + } else if (skip_prefix(line, "ERR ", )) {
> + die(_("remote error: %s"), p);
> + } else {
> + die("invalid server response; got '%s'", line);
> + }
> +}
> +
> static struct discovery *discover_refs(const char *service, int for_push)
> {
> - struct strbuf exp = STRBUF_INIT;
> struct strbuf type = STRBUF_INIT;
> struct strbuf charset = STRBUF_INIT;
> struct strbuf buffer = STRBUF_INIT;
> @@ -405,38 +457,8 @@ static struct discovery *discover_refs(const char
> *service, int for_push)
> last->buf_alloc = strbuf_detach(, >len);
> last->buf = last->buf_alloc;
>
> - strbuf_addf(, "application/x-%s-advertisement", service);
> - if
Re: [[PATCH v2] ] INSTALL: add macOS gettext and sdk header explanation to INSTALL
On Thu, Nov 15, 2018 at 11:07 PM Junio C Hamano wrote: > yanke131...@gmail.com writes: > > * add macOS gettext explanation to get the i18n locale translation take > > effect in macOS, as the most polular way of gettext > > install in macOS, the gettext is not linked by default, this commit give > > a tip on this thing. > > Also I am not quite sure what it wants to say. Perhaps you meant > to say something like this? > > Explain how to make the gettext library usable on macOS, as > with the most popular way to install it, it won't be linked > to /usr/local. > > I think the part that I had most trouble understanding was your use > of the verb "link"; it was unclear (and I am guessing) that you > meant there are missing links on the filesystem to make stuff from > gettext package available to programs that want to build with it [...] You inferred correctly, and your rewritten text conveys the needed information. > > * add macOS Command Line Tool sdk header explanation to get correct build > > in macOS 10.14+, as the CLT not install > > the header by default, we need install it by self, this commit give a way > > to install the loss headers. > > Similarly, is > > Explain how to install the Command Line Tool SDK headers > manually on macOS 10.14+ in order to correctly build Git, as > they are not installed by default. > > what you meant? Also correct. > > +In macOS, can install gettext with brew as "brew install gettext" > > +and "brew link --force gettext", the gettext is keg-only so brew not > > link > > +it to /usr/local by default, so link operation is necessary, or you can > > +follow the brew tips after install gettext. > > My best guess of what you wanted to say is > > On macOS, `gettext` can be installed with `brew install > gettext`, but because the `gettext` package is keg-only and > is not made available in `/usr/local` by default. `brew s/./,/ > link --force gettext` must be run after `brew install > gettext` to correct this to use i18n features of Git. > > but now the sentence structure is quite different and I no longer > know if that is what you meant to say. And it does not help that I > am not a Mac user. Aside from the minor punctuation issue, your rewrite correctly captures the intent and is understandable. > > If not link gettext correctly, > > +the git after build will not have correct locale translations, english > > is the > > +default language. > > If my rephrasing above is correct, then these three lines become > unnecessary, I think. Yep. > > + - In macOs 10.14, the Command Line Tool not contains sdk headers as > > before, so > > +need install Command Line Tool 10.14 and install the headers with > > command > > On macOS 10.14, the Command Line Tool no longer contains the > SDK headers; you need to also install them with the command: > > > +If not install the sdk headers correctly, git build will get errors > > blew, factly is > > +is because of this problem. > > I can guess this wants to say "without sdk headers your attempt to > build Git will blow up in your face", but not quite. > > Unless you install the SDK headers, building git will fail > with error messages like the following: Although, as you note for the other case, this sentence and the following example error message are likely unnecessary.
Re: [PATCH 0/3] clone: respect configured fetch respecs during initial fetch
Jeff King writes:
> On Wed, Nov 14, 2018 at 11:46:17AM +0100, SZEDER Gábor wrote:
>
>> This patch series should have been marked as v6, but I chose to reset
>> the counter, because:
>>
>> - v5 was sent out way over a year ago [1], and surely everybody has
>> forgotten about it since then anyway. But more importantly:
>>
>> - A lot has happened since then, most notably we now have a refspec
>> API, which makes this patch series much simpler (now it only
>> touches 'builtin/clone.c', the previous version had to add stuff
>> to 'remote.{c,h}' as well).
>
> Thanks for sticking with this!
>
> I skimmed over the old discussion, mostly just to make sure there wasn't
> anything subtle that might have been forgotten. But nope, all of the
> subtlety went away because of the refspec API you mentioned.
>
> The whole series looks good to me.
Thanks, both.
Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin
Ævar Arnfjörð Bjarmason writes:
> The rebase.useBuiltin variable introduced in 55071ea248 ("rebase:
> start implementing it as a builtin", 2018-08-07) was turned on by
> default in 5541bd5b8f ("rebase: default to using the builtin rebase",
> 2018-08-08), but had no documentation.
I actually thought that everybody understood that this was merely an
aid to be used during the development of the feature and never meant
to be given to the end users.
With my devil's advocate hat on, how much do we trust it as an
escape hatch? After all, the codepath to hide the "rebase in C"
implementation and use the scripted version were never in 'master'
(or 'next' for that matter) with this variable turned off, so I am
reasonably sure it had no serious exposure to real world usage.
Having said that, assuming that the switching back to scripted
version works correctly and assuming that we want to expose this to
end users, I think the patch text makes sense.
Thanks.
> Let's document it so that users who run into any stability issues with
> the C rewrite know there's an escape hatch[1], and make it clear that
> needing to turn off builtin rebase means you've found a bug in git.
>
> 1. https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> Documentation/config/rebase.txt | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index 42e1ba7575..f079bf6b7e 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -1,3 +1,17 @@
> +rebase.useBuiltin::
> + Set to `false` to use the legacy shellscript implementation of
> + linkgit:git-rebase[1]. Is `true` by default, which means use
> + the built-in rewrite of it in C.
> ++
> +The C rewrite is first included with Git version 2.20. This option
> +serves an an escape hatch to re-enable the legacy version in case any
> +bugs are found in the rewrite. This option and the shellscript version
> +of linkgit:git-rebase[1] will be removed in some future release.
> ++
> +If you find some reason to set this option to `false` other than
> +one-off testing you should report the behavior difference as a bug in
> +git.
> +
> rebase.stat::
> Whether to show a diffstat of what changed upstream since the last
> rebase. False by default.
Re: [[PATCH v2] ] INSTALL: add macOS gettext and sdk header explanation to INSTALL
yanke131...@gmail.com writes: > Subject: Re: [[PATCH v2] ] INSTALL: add macOS gettext and sdk header > explanation to INSTALL The above should look more like Subject: [PATCH v2] INSTALL: add macOS ... > From: out0fmemory This line is to give information that records who the patch was writtten by, and later Signed-off-by: line certifies that the person (which typically is, and in this case also is, the person who wrote it) has the right to contribute it to the project under the project's licensing terms. We want to see the name and address on these two lines to match. > * add macOS gettext explanation to get the i18n locale translation take > effect in macOS, as the most polular way of gettext > install in macOS, the gettext is not linked by default, this commit give a > tip on this thing. > Please wrap overlong lines to ~70 cols. Also I am not quite sure what it wants to say. Perhaps you meant to say something like this? Explain how to make the gettext library usable on macOS, as with the most popular way to install it, it won't be linked to /usr/local. I think the part that I had most trouble understanding was your use of the verb "link"; it was unclear (and I am guessing) that you meant there are missing links on the filesystem to make stuff from gettext package available to programs that want to build with it, and instead your original text (aside from grammatical issues) sounded as if you were reporting lack of linker flags when building binary or something. > * add macOS Command Line Tool sdk header explanation to get correct build in > macOS 10.14+, as the CLT not install > the header by default, we need install it by self, this commit give a way > to install the loss headers. Similarly, is Explain how to install the Command Line Tool SDK headers manually on macOS 10.14+ in order to correctly build Git, as they are not installed by default. what you meant? > Signed-off-by: yanke > --- > INSTALL | 20 > 1 file changed, 20 insertions(+) > > diff --git a/INSTALL b/INSTALL > index c39006e8e7..ed4bd29f8f 100644 > --- a/INSTALL > +++ b/INSTALL > @@ -165,6 +165,13 @@ Issues of note: > use English. Under autoconf the configure script will do this > automatically if it can't find libintl on the system. > > +In macOS, can install gettext with brew as "brew install gettext" > +and "brew link --force gettext", the gettext is keg-only so brew not link > +it to /usr/local by default, so link operation is necessary, or you can > +follow the brew tips after install gettext. Sorry, but I cannot quite understand this overlong and grammatically unparsable single sentence. There is no subject for verb phrase "can install" at the beginning of the sentence where I already got stuck X-<. My best guess of what you wanted to say is On macOS, `gettext` can be installed with `brew install gettext`, but because the `gettext` package is keg-only and is not made available in `/usr/local` by default. `brew link --force gettext` must be run after `brew install gettext` to correct this to use i18n features of Git. but now the sentence structure is quite different and I no longer know if that is what you meant to say. And it does not help that I am not a Mac user. > If not link gettext correctly, > +the git after build will not have correct locale translations, english > is the > +default language. > + If my rephrasing above is correct, then these three lines become unnecessary, I think. > + - In macOs 10.14, the Command Line Tool not contains sdk headers as > before, so > +need install Command Line Tool 10.14 and install the headers with command On macOS 10.14, the Command Line Tool no longer contains the SDK headers; you need to also install them with the command: > +"sudo installer -pkg > /Library/Developer/CommandLineTools/Packages/macOS_SDK_headers_for_macOS_10.14.pkg > -target". > +If not install the sdk headers correctly, git build will get errors > blew, factly is > +is because of this problem. I can guess this wants to say "without sdk headers your attempt to build Git will blow up in your face", but not quite. Unless you install the SDK headers, building git will fail with error messages like the following: Perhaps. > +ld: archive has no table of contents file 'xdiff/lib.a' for architecture > x86_64 > +clang: error: linker command failed with exit code 1 (use -v to see > invocation) > +make: *** [Makefile:2369: git-fast-import] Error 1 > +ld: archive has no table of contents file 'xdiff/lib.a' for architecture > x86_64 > +clang: error: linker command failed with exit code 1 (use -v to see > invocation) > +make: *** [Makefile:2046: git] Error 1 > + > - To build and install documentation suite, you need to have > the asciidoc/xmlto
Re: insteadOf and git-request-pull output
Konstantin Ryabitsev writes: > On Thu, Nov 15, 2018 at 07:54:32PM +0100, Ævar Arnfjörð Bjarmason wrote: >> > I think that if we use the "principle of least surprise," insteadOf >> > rules shouldn't be applied for git-request-pull URLs. >> >> I haven't used request-pull so I don't have much of an opinion on this, >> but do you think the same applies to 'git remote get-url '? >> >> I.e. should it also show the original unmunged URL, or the munged one as >> it does now? > > I don't know, maybe both? As opposed to git-request-pull, this is not > exposing the insteadOf URL to someone other than the person who set it > up, so even if it does return the munged URL, it wouldn't be unexpected. Yeah, I think the local "git remote" should show the rewritten URL (because that information is not available otherwise) and it is OK to optionally show the original. Also, I think it would be nice if request-pull gave external-facing names. After all, insteadOf address is for your own use (e.g. maybe pointing at corporate mirror), and not for the intended recipient of request-pull. In short, I think I agree with things you are saying in this exchange.
Re: [PATCH] ref-filter: don't look for objects when outside of a repository
Jeff King writes:
> On Thu, Nov 15, 2018 at 04:38:44AM -0500, Jeff King wrote:
>
>> Is SOURCE_NONE a complete match for what we want?
>>
>> I see problems in both directions:
>>
>> - sorting by "objectname" works now, but it's marked with SOURCE_OBJ,
>>and would be forbidden with your patch. I'm actually not sure if
>>SOURCE_OBJ is accurate; we shouldn't need to access the object to
>>show it (and we are probably wasting effort loading the full contents
>>for tools like for-each-ref).
>>
>>However, that's not the full story. For objectname:short, it _does_ call
>>find_unique_abbrev(). So we expect to have an object directory.
>
> Oops, I'm apparently bad at reading. It is in fact SOURCE_OTHER, which
> makes sense (outside of this whole "--sort outside a repo thing").
>
> But we'd ideally distinguish between "objectname" (which should be OK
> outside a repo) and "objectname:short" (which currently segfaults).
Arguably, use of ref-filter machinery in ls-remote, whether it is
given from inside or outside a repo, was a mistake in 1fb20dfd
("ls-remote: create '--sort' option", 2018-04-09), as the whole
point of "ls-remote" is to peek the list of refs and it is perfectly
normal that the objects listed are not available.
"ls-remote --sort=authorname" that is run in a repository may not
segfault on a ref that points at a yet-to-be-fetched commit, but it
cannot be doing anything sensible. Is it still better to silently
produce a nonsense result than refusing to --sort no matter what the
sort keys are, whether we are inside or outside a repository?
Re: [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity
Slavica Djukic writes:
> git-stash.sh | 17 +
> t/t3903-stash.sh | 2 +-
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 94793c1a9..789ce2f41 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -55,6 +55,20 @@ untracked_files () {
> git ls-files -o $z $excl_opt -- "$@"
> }
>
> +prepare_fallback_ident () {
> + if ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null
> 2>&1
> + then
> + GIT_AUTHOR_NAME="git stash"
> + GIT_AUTHOR_EMAIL=git@stash
> + GIT_COMMITTER_NAME="git stash"
> + GIT_COMMITTER_EMAIL=git@stash
> + export GIT_AUTHOR_NAME
> + export GIT_AUTHOR_EMAIL
> + export GIT_COMMITTER_NAME
> + export GIT_COMMITTER_EMAIL
> + fi
> +}
> +
> clear_stash () {
> if test $# != 0
> then
> @@ -67,6 +81,9 @@ clear_stash () {
> }
>
> create_stash () {
> +
> + prepare_fallback_ident
> +
> stash_msg=
> untracked=
> while test $# != 0
That looks like a sensible implementation to me.
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index bab8bec67..0b0814421 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1096,7 +1096,7 @@ test_expect_success 'stash -- works with
> binary files' '
> test_path_is_file subdir/untracked
> '
>
> -test_expect_failure 'stash works when user.name and user.email are not set' '
> +test_expect_success 'stash works when user.name and user.email are not set' '
This line claims to the readers of patch that the known breakage
this known test piece demonstrated has been corrected, but they need
to refresh their memory by going back to the previous patch to see
if this "failure-to-success" flipping is done to the right test
piece, and what exactly the test piece tested to see the existing
breakage, because all the interesting part of the test are chomped
outside the post-context of this hunk.
Unless the fix is fairly complex, adding ought-to-succeed tests that
expect success that break when the code change gets omitted from the
patch in the same patch as the fix itself (i.e. squash patch 1/2 and
patch 2/2 into a single patch) would be more helpful for the readers
(it also helps cherry-picking the fix later to earlier maintenance
tracks if it becomes necessary).
> git reset &&
> git var GIT_COMMITTER_IDENT >expected &&
> >1 &&
Re: [PATCH v3 1/1] protocol: advertise multiple supported versions
Josh Steadmon writes:
>> What I was alludding to was a lot simpler, though. An advert string
>> "version=0:version=1" from a client that prefers version 0 won't be
>> !strcmp("version=0", advert) but seeing many strcmp() in the patch
>> made me wonder.
>
> Ah I see. The strcmp()s against "version=0" are special cases for where
> it looks like the client does not understand any sort of version
> negotiation. If we see multiple versions listed in the advert, then the
> rest of the selection logic should do the right thing.
OK, let me try to see if I understand correctly by rephrasing.
If the client does not say anything about which version it prefers
(because it only knows about version 0 without even realizing that
there is a version negotiation exchange), we substitute the lack of
proposed versions with string "version=0", and the strcmp()s I saw
and was puzzled by are all about special casing such a client. But
we would end up behaving the same way (at least when judged only by
externally visible effects) to a client that is aware of version
negotiation and tells us it prefers version 0 (and nothing else)
with the selection logic anyway.
Did I get it right? If so, yeah, I think it makes sense to avoid
two codepaths that ends up doing the same thing by removing the
special case.
> However, I think that it might work to remove the special cases. In the
> event that the client is so old that it doesn't understand any form of
> version negotiation, it should just ignore the version fields /
> environment vars. If you think it's cleaner to remove the special cases,
> let me know.
Re: [RFC PATCH 1/2] commit: don't add scissors line if one exists
Denton Liu writes: >> If the current invocation of "git commit" added a scissors line in >> the buffer to be edited already, and we are adding another one in >> this function, is it possible that the real problem that somebody >> else has called wt_status_add_cut_line() before this function is >> called, in which case that other caller is what we need to fix, >> instead of this one? >> > > In patch 2/2, I intentionally inserted a scissors line into MERGE_MSG so > this patch ensures that we don't get duplicate scissors. That is exactly what the paragraph you are responding to questions. Is the code that adds a scissors line before this function is called done the right way? Shouldn't it be doing something differnetly? Looking for an existing scissors looking line in this function does not let this function differenciate two cases, i.e. we deliberately added one already before calling this function (in which case this function should not add another one), or we didn't add anything on our own, but the material supplied by the end user had one (in which case, not adding ours is losing information---imagine that the user notices a scissors-looking line that came from the original maerial and want to munge it, as it is part of proper message, so that it would remain in the committed result, but because [PATCH 1/2] stopped adding a scissors line at the right location, the user would have to guess where to add one). There must be an explicit way (e.g. a bit in a flag word parameter given to this function) for the caller who knows when the new code in [PATCH 2/2] triggers, to tell this function not to add another one, instead of a sloppy (and less efficient) "lets's scan to see if there already is a scissors looking line". > With the existing behaviour, any messages that contain a scissors > looking line will get cut at the earliest scissors anyway, so I believe > that this patch would not change the behaviour. If the users were > dealing with commit messages with a scissors looking line, the current > behaviour already requires users to be extra careful to ensure that the > scissors don't get accidentally removed so in the interest of preserving > the existing behaviour, I don't think that any extra information would > be lost from this patch. Doing the "is there already a scissors looing line" approach will *make* it harder to fix that issue, so the patch is making things worse.
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Slavica Djukic writes:
> +test_expect_failure 'stash works when user.name and user.email are not set' '
> + git reset &&
> + git var GIT_COMMITTER_IDENT >expected &&
All the other existing test pieces in this file calls the expected
result "expect"; is there a reason why this patch needs to be
different (e.g. 'expect' file left by the earlier step needs to be
kept unmodified for later use, or something like that)? If not,
please avoid making a difference in irrelevant details, as that
would waste time of readers by forcing them to guess if there is
such a reason that readers cannot immediately see.
Anyway, we grab the committer ident we use by default during the
test with this command. OK.
> + >1 &&
> + git add 1 &&
> + git stash &&
And we make sure we can create stash.
> + git var GIT_COMMITTER_IDENT >actual &&
> + test_cmp expected actual &&
I am not sure what you are testing with this step. There is nothing
that changed environment variables or configuration since we ran
"git var" above. Why does this test suspect that somebody in the
future may break the expectation that after running 'git add' and/or
'git stash', our committer identity may have been changed, and how
would such a breakage happen?
> + >2 &&
> + git add 2 &&
> + test_config user.useconfigonly true &&
> + test_config stash.usebuiltin true &&
Now we start using use-config-only, so unsetting environment
variables will cause trouble when Git insists on having an
explicitly configured identities. Makes sense.
> + (
> + sane_unset GIT_AUTHOR_NAME &&
> + sane_unset GIT_AUTHOR_EMAIL &&
> + sane_unset GIT_COMMITTER_NAME &&
> + sane_unset GIT_COMMITTER_EMAIL &&
> + test_unconfig user.email &&
> + test_unconfig user.name &&
And then we try the same test, but without environment or config.
Since we are unsetting the environment, in order to be nice for
future test writers, we do this in a subshell, so that we do not
have to restore the original values of environment variables.
Don't we need to be nice the same way for configuration variables,
though? We _know_ that nobody sets user.{email,name} config up to
this point in the test sequence, so that is why we do not do a "save
before test and then restore to the original" dance on them. Even
though we are relying on the fact that these two variables are left
unset in the configuration file, we unconfig them here anyway, and I
do think it is a good idea for documentation purposes (i.e. we are
not documenting what we assume the config before running this test
would be; we are documenting what state we want these two variables
are in when running this "git stash"---that is, they are both unset).
So these later part of this test piece makes sense. I still do not
know what you wanted to check in the earlier part of the test,
though.
> + git stash
> + )
> +'
> +
> test_done
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Junio C Hamano writes: > Now we start using use-config-only, so unsetting environment > variables will cause trouble when Git insists on having an > explicitly configured identities. Makes sense. > >> +( >> +sane_unset GIT_AUTHOR_NAME && >> +sane_unset GIT_AUTHOR_EMAIL && >> +sane_unset GIT_COMMITTER_NAME && >> +sane_unset GIT_COMMITTER_EMAIL && >> +test_unconfig user.email && >> +test_unconfig user.name && > > And then we try the same test, but without environment or config. I suspect that it makes sense to replace the "git stash" we see below with something like this: test_must_fail git commit -m should fail && echo "git stash " >expect && echo >2 && git stash && git show -s --format="%(authorname) <%(authoremail)>" refs/stash >actual && test_cmp expect actual That is - we make sure "commit" would not go through, to make sure our preparation to unset environment variables was sufficient; - we make sure "stash" does succeed (which is the primary thing you are interested in); - we make sure the resulting "stash" is not created under our default identity but under our fallback one. >> +git stash >> +) >> +' >> + >> test_done
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Junio C Hamano writes: > Slavica Djukic writes: > >> +test_expect_failure 'stash works when user.name and user.email are not set' >> ' >> +git reset && >> +git var GIT_COMMITTER_IDENT >expected && > ... > Anyway, we grab the committer ident we use by default during the > test with this command. OK. > >> +>1 && >> +git add 1 && >> +git stash && > > And we make sure we can create stash. > >> +git var GIT_COMMITTER_IDENT >actual && >> +test_cmp expected actual && > > I am not sure what you are testing with this step. There is nothing > that changed environment variables or configuration since we ran > "git var" above. Why does this test suspect that somebody in the > future may break the expectation that after running 'git add' and/or > 'git stash', our committer identity may have been changed, and how > would such a breakage happen? Just a note. "git var GIT_COMMITTER_IDENT" has timestamp in it, so a naïve reader might wonder what would happen if "git add 1" and "git stash" took more than one second. But it won't be a problem in this case as the committer date comes from the environment GIT_COMMITTER_DATE, which is set to a fixed known value and is incremented only by calling test_commit helper function, which does not happen between the two "git var" calls. In any case, I am not sure I understand the point of comparing two output from "git var" invocations we see ablve in this test.
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi, On Wed, 14 Nov 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > Agreed. I'm happy to see the test for-loop gone as I noted in > > https://public-inbox.org/git/87d0rm7zeo@evledraar.gmail.com/ but as > > noted in that v3 feedback the whole "why would anyone want this?" > > explanation is still missing, and this still smells like a workaround > > for a bug we should be fixing elsewhere in the sequencing code. > > Thanks. I share the same impression that this is sweeping a bug > under a wrong rug. I asked for clarification at https://github.com/git-for-windows/git/issues/1854 and in my best imitation of Lt Tawney Madison, I report back: >From @chucklu: > my user case is like this : > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > are merge commits. Then I will get lots of popup like: > >The previous cherry-pick is now empty, possibly due to conflict >resolution. >If you wish to commit it anyway, use: > >git commit --allow-empty > >If you wish to skip this commit, use: > >git reset > >Then "git cherry-pick --continue" will resume cherry-picking >the remaining commits. My quick interpretation of this is that the user actually needs a way to skip silently commits which are now empty. Ciao, Dscho
Re: [PATCH] ref-filter: don't look for objects when outside of a repository
On Thu, Nov 15, 2018 at 04:38:44AM -0500, Jeff King wrote: > Is SOURCE_NONE a complete match for what we want? > > I see problems in both directions: > > - sorting by "objectname" works now, but it's marked with SOURCE_OBJ, >and would be forbidden with your patch. I'm actually not sure if >SOURCE_OBJ is accurate; we shouldn't need to access the object to >show it (and we are probably wasting effort loading the full contents >for tools like for-each-ref). > >However, that's not the full story. For objectname:short, it _does_ call >find_unique_abbrev(). So we expect to have an object directory. Oops, I'm apparently bad at reading. It is in fact SOURCE_OTHER, which makes sense (outside of this whole "--sort outside a repo thing"). But we'd ideally distinguish between "objectname" (which should be OK outside a repo) and "objectname:short" (which currently segfaults). -Peff
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
On Thu, Nov 15, 2018 at 09:40:38AM +0100, Johannes Schindelin wrote: > From @chucklu: > > > my user case is like this : > > > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > > are merge commits. Then I will get lots of popup like: > > > >The previous cherry-pick is now empty, possibly due to conflict > >resolution. > >If you wish to commit it anyway, use: > > > >git commit --allow-empty > > > >If you wish to skip this commit, use: > > > >git reset > > > >Then "git cherry-pick --continue" will resume cherry-picking > >the remaining commits. > > My quick interpretation of this is that the user actually needs a way to > skip silently commits which are now empty. If it's always intended to be used with cherry-pick, shouldn't cherry-pick learn a --keep-empty (like rebase has)? That would avoid even stopping for this case in the first place. -Peff
Re: [PATCH] ref-filter: don't look for objects when outside of a repository
On Wed, Nov 14, 2018 at 01:27:25PM +0100, SZEDER Gábor wrote:
> The command 'git ls-remote --sort=authordate ' segfaults when
> run outside of a repository, ever since the introduction of its
> '--sort' option in 1fb20dfd8e (ls-remote: create '--sort' option,
> 2018-04-09).
>
> While in general the 'git ls-remote' command can be run outside of a
> repository just fine, its '--sort=' option with certain keys does
> require access to the referenced objects. This sorting is implemented
> using the generic ref-filter sorting facility, which already handles
> missing objects gracefully with the appropriate 'missing object
> deadbeef for HEAD' message. However, being generic means that it
> checks replace refs while trying to retrieve an object, and while
> doing so it accesses the 'git_replace_ref_base' variable, which has
> not been initialized and is still a NULL pointer when outside of a
> repository, thus causing the segfault.
>
> Make ref-filter more careful upfront while parsing the format string,
> and make it error out when encountering a format atom requiring object
> access when we are not in a repository. Also add a test to ensure
> that 'git ls-remote --sort' fails gracefully when executed outside of
> a repository.
Thanks for picking up this loose end. I like the general approach here,
but...
> diff --git a/ref-filter.c b/ref-filter.c
> index 0c45ed9d94..a1290659af 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -534,6 +534,10 @@ static int parse_ref_filter_atom(const struct ref_format
> *format,
> if (ARRAY_SIZE(valid_atom) <= i)
> return strbuf_addf_ret(err, -1, _("unknown field name: %.*s"),
> (int)(ep-atom), atom);
> + if (valid_atom[i].source != SOURCE_NONE && !have_git_dir())
> + return strbuf_addf_ret(err, -1,
> +_("not a git repository, but the field
> '%.*s' requires access to object data"),
> +(int)(ep-atom), atom);
Is SOURCE_NONE a complete match for what we want?
I see problems in both directions:
- sorting by "objectname" works now, but it's marked with SOURCE_OBJ,
and would be forbidden with your patch. I'm actually not sure if
SOURCE_OBJ is accurate; we shouldn't need to access the object to
show it (and we are probably wasting effort loading the full contents
for tools like for-each-ref).
However, that's not the full story. For objectname:short, it _does_ call
find_unique_abbrev(). So we expect to have an object directory.
- sorting by "HEAD" hits a BUG(), and would still be allowed with your
patch.
So I like the idea here that the particular atoms would tell us whether
they're going to need to be in a repository or not, but I think the
annotations have to be cleaned up first.
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 91ee6841c1..32e722db2e 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -302,6 +302,12 @@ test_expect_success 'ls-remote works outside repository'
> '
> nongit git ls-remote dst.git
> '
>
> +test_expect_success 'ls-remote --sort fails gracefully outside repository' '
> + # Use a sort key that requires access to the referenced objects.
> + nongit test_must_fail git ls-remote --sort=authordate
> "$TRASH_DIRECTORY" 2>err &&
> + test_i18ngrep "^fatal: not a git repository, but the field
> '\''authordate'\'' requires access to object data" err
> +'
Regardless of our solution, we probably want to add an extra test making
sure that something vanilla like:
nongit git ls-remote --sort=v:refname "$TRASH_DIRECTORY"
continues to work (we do test ls-remote outside a repo already, but not
with a sort specifier).
-Peff
Re: [PATCH 2/3] clone: respect additional configured fetch refspecs during initial fetch
On Wed, Nov 14, 2018 at 11:46:19AM +0100, SZEDER Gábor wrote: > The initial fetch during a clone doesn't transfer refs matching > additional fetch refspecs given on the command line as configuration > variables, e.g. '-c remote.origin.fetch='. This contradicts > the documentation stating that configuration variables specified via > 'git clone -c = ...' "take effect immediately after the > repository is initialized, but before the remote history is fetched" > and the given example specifically mentions "adding additional fetch > refspecs to the origin remote". Furthermore, one-shot configuration > variables specified via 'git -c = clone ...', though not > written to the newly created repository's config file, live during the > lifetime of the 'clone' command, including the initial fetch. All > this implies that any fetch refspecs specified this way should already > be taken into account during the initial fetch. > > The reason for this is that the initial fetch is not a fully fledged > 'git fetch' but a bunch of direct calls into the fetch/transport > machinery with clone's own refs-to-refspec matching logic, which > bypasses parts of 'git fetch' processing configured fetch refspecs. > This logic only considers a single default refspec, potentially > influenced by options like '--single-branch' and '--mirror'. The > configured refspecs are, however, already read and parsed properly > when clone calls remote.c:remote_get(), but it never looks at the > parsed refspecs in the resulting 'struct remote'. > > Modify clone to take the remote's configured fetch refspecs into > account to retrieve all matching refs during the initial fetch. Note > that we have to explicitly add the default fetch refspec to the > remote's refspecs, because at that point the remote only includes the > fetch refspecs specified on the command line. Nicely explained. > Add tests to check that refspecs given both via 'git clone -c ...' and > 'git -c ... clone' retrieve all refs matching either the default or > the additional refspecs, and that it works even when the user > specifies an alternative remote name via '--origin='. Good. This is all sufficiently subtle that we definitely want to check the related cases. > @@ -1081,11 +1086,12 @@ int cmd_clone(int argc, const char **argv, const char > *prefix) > if (option_required_reference.nr || option_optional_reference.nr) > setup_reference(); > > + remote = remote_get(option_origin); > + > strbuf_addf(_refspec, "+%s*:%s*", src_ref_prefix, > branch_top.buf); > - refspec_append(, default_refspec.buf); > + refspec_append(>fetch, default_refspec.buf); Wow, this is so much nicer than the older versions. :) I wondered briefly whether this ought to only kick in when the user didn't specify any refspecs. I.e., there is no way with this to say "don't fetch refs/heads/*, but this other thing instead". But the way the existing documentation is written, it's pretty clear that it's about adding to the list of refspecs. We can always something like "--no-default-refspec" later if somebody wants it. > [...] Most of the rest of the patch is just swapping out "rs" for "remote->fetch", which makes sense. So the implementation looks good to me. > diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh > index 39329eb7a8..60c1ba951b 100755 > --- a/t/t5611-clone-config.sh > +++ b/t/t5611-clone-config.sh > @@ -45,6 +45,53 @@ test_expect_success 'clone -c config is available during > clone' ' > test_cmp expect child/file > ' > > +test_expect_success 'clone -c remote.origin.fetch= works' ' > + rm -rf child && > + git update-ref refs/grab/it refs/heads/master && > + git update-ref refs/leave/out refs/heads/master && Checking one in and one out; nice. > + git clone -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" . child && > + git -C child for-each-ref --format="%(refname)" >actual && > + > + cat >expect <<-\EOF && > + refs/grab/it > + refs/heads/master > + refs/remotes/origin/HEAD > + refs/remotes/origin/master > + EOF > + test_cmp expect actual > +' Makes sense. > +test_expect_success 'git -c remote.origin.fetch= clone works' ' > + rm -rf child && > + git -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" clone . child && > + git -C child for-each-ref --format="%(refname)" >actual && > + > + cat >expect <<-\EOF && > + refs/grab/it > + refs/heads/master > + refs/remotes/origin/HEAD > + refs/remotes/origin/master > + EOF > + test_cmp expect actual > +' This relies on establishing grab/it in the previous test. A minor nit, but we could break that more clear by breaking it out into its own "set up refs for extra refspec tests" block. > +test_expect_success 'clone -c remote..fetch= > --origin=' ' > + rm -rf child && > + git clone --origin=upstream \ > + -c "remote.upstream.fetch=+refs/grab/*:refs/grab/*" \ > +
Re: [PATCH] INSTALL: add macOS gettext and sdk header explanation to INSTALL
Thanks for review, I will summit a new patch soon to improve this. Jonathan Nieder 于2018年11月15日周四 上午11:05写道： > > Hi! > > yanke131...@gmail.com wrote: > > > From: out0fmemory > > > > --- > > INSTALL | 7 +++ > > 1 file changed, 7 insertions(+) > > Thanks for writing. A few bits of administrivia, from > https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html: > > Can we forge your sign-off? See the section "certify your work" in > SubmittingPatches for what this means. > > What name should we attribute this change to? Documentation/SubmittingPatches > explains: > > Also notice that a real name is used in the Signed-off-by: line. > Please don’t hide your real name. > > [...] > > --- a/INSTALL > > +++ b/INSTALL > > @@ -165,6 +165,9 @@ Issues of note: > > use English. Under autoconf the configure script will do this > > automatically if it can't find libintl on the system. > > > > +In macOS, can install and link gettext with brew as "brew install > > gettext" > > +and "brew link --force gettext", the link operation is necessary. > > + > > As context (e.g. to go in the commit message), can you tell us more > about this? E.g. what happens if you don't perform the link > operation? > > [...] > > @@ -178,6 +181,10 @@ Issues of note: > > will include them. Note that config.mak is not distributed; > > the name is reserved for local settings. > > > > + - In macOs 10.14, the Command Line Tool not contains the headers as > > before, so it > > +need install Command Line Tool 10.14 and install the headers which > > command > > +"sudo installer -pkg > > /Library/Developer/CommandLineTools/Packages/macOS_SDK_headers_for_macOS_10.14.pkg > > -target". > > + > > Likewise: what is the symptom if this isn't done? > > Is there more general advice we can put here that will survive past > macOS 10.14? > > Thanks and hope that helps, > Jonathan
Re: [PATCH 0/3] clone: respect configured fetch respecs during initial fetch
On Wed, Nov 14, 2018 at 11:46:17AM +0100, SZEDER Gábor wrote:
> This patch series should have been marked as v6, but I chose to reset
> the counter, because:
>
> - v5 was sent out way over a year ago [1], and surely everybody has
> forgotten about it since then anyway. But more importantly:
>
> - A lot has happened since then, most notably we now have a refspec
> API, which makes this patch series much simpler (now it only
> touches 'builtin/clone.c', the previous version had to add stuff
> to 'remote.{c,h}' as well).
Thanks for sticking with this!
I skimmed over the old discussion, mostly just to make sure there wasn't
anything subtle that might have been forgotten. But nope, all of the
subtlety went away because of the refspec API you mentioned.
The whole series looks good to me.
-Peff
Re: [PATCH 1/3] clone: use a more appropriate variable name for the default refspec
On Wed, Nov 14, 2018 at 11:46:18AM +0100, SZEDER Gábor wrote: > cmd_clone() declares two strbufs 'key' and 'value' on the same line, > suggesting that they are used to contruct a config variable's name and > value. However, this is not the case: 'key' is used to construct the > names of multiple config variables, while 'value' is never used as a > value for any of those config variables, or for any other config > variable for that matter, but only to contruct the default fetch > refspec. > > Let's rename 'value' to 'default_refspec' to make the intent clearer. Yep, this is much nicer. -Peff
Re: [PATCH 3/3] Documentation/clone: document ignored configuration variables
On Wed, Nov 14, 2018 at 11:46:20AM +0100, SZEDER Gábor wrote:
> Due to limitations in the current implementation, some configuration
> variables specified via 'git clone -c var=val' (or 'git -c var=val
> clone') are ignored during the initial fetch and checkout.
>
> Let the users know which configuration variables are known to be
> ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the
> documentation of 'git clone -c', along with hints to use the options
> '--mirror' and '--no-tags' instead.
Good idea.
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index a55536f0bf..2fd12524f9 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -189,6 +189,12 @@ objects from the source repository into a pack in the
> cloned repository.
> values are given for the same key, each value will be written to
> the config file. This makes it safe, for example, to add
> additional fetch refspecs to the origin remote.
> ++
> +Due to limitations of the current implementation, some configuration
> +variables do not take effect until after the initial fetch and checkout.
> +Configuration variables known to not take effect are:
> +`remote..mirror` and `remote..tagOpt`. Use the
> +corresponding `--mirror` and `--no-tags` options instead.
This looks good. I considered at first that this might want to go in a
BUGS section of the manpage, but it makes the most sense being right
next to the definition of "-c".
-Peff
[PATCH 1/1] mingw: replace an obsolete link with the superseding one
From: Johannes Schindelin
The MSDN documentation has been superseded by Microsoft Docs (which is
backed by a repository on GitHub containing many, many files in Markdown
format).
Signed-off-by: Johannes Schindelin
---
compat/mingw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index d2f4fabb44..9e42b0ee26 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1028,8 +1028,8 @@ char *mingw_getcwd(char *pointer, int len)
}
/*
- * See http://msdn2.microsoft.com/en-us/library/17w5ykft(vs.71).aspx
- * (Parsing C++ Command-Line Arguments)
+ * See "Parsing C++ Command-Line Arguments" at Microsoft's Docs:
+ * https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments
*/
static const char *quote_arg(const char *arg)
{
--
gitgitgadget
[PATCH 0/1] mingw: update a link to the official documentation
It is a pretty neat thing that the bulky MSDN documentation has been replaced by something a lot more open, something that can be updated via Pull Requests on GitHub. Let's link to this new documentation instead of the obsolete one. I know, it is really close to the code freeze leading up to Git v2.20.0. But this is just an update to a code comment... :-) Johannes Schindelin (1): mingw: replace an obsolete link with the superseding one compat/mingw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) base-commit: d166e6afe5f257217836ef24a73764eba390c58d Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-81%2Fdscho%2Fmingw-update-msdn-link-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-81/dscho/mingw-update-msdn-link-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/81 -- gitgitgadget
[[PATCH v2] ] INSTALL: add macOS gettext and sdk header explanation to INSTALL
From: out0fmemory * add macOS gettext explanation to get the i18n locale translation take effect in macOS, as the most polular way of gettext install in macOS, the gettext is not linked by default, this commit give a tip on this thing. * add macOS Command Line Tool sdk header explanation to get correct build in macOS 10.14+, as the CLT not install the header by default, we need install it by self, this commit give a way to install the loss headers. Signed-off-by: yanke --- INSTALL | 20 1 file changed, 20 insertions(+) diff --git a/INSTALL b/INSTALL index c39006e8e7..ed4bd29f8f 100644 --- a/INSTALL +++ b/INSTALL @@ -165,6 +165,13 @@ Issues of note: use English. Under autoconf the configure script will do this automatically if it can't find libintl on the system. +In macOS, can install gettext with brew as "brew install gettext" +and "brew link --force gettext", the gettext is keg-only so brew not link +it to /usr/local by default, so link operation is necessary, or you can +follow the brew tips after install gettext. If not link gettext correctly, +the git after build will not have correct locale translations, english is the +default language. + - Python version 2.4 or later (but not 3.x, which is not supported by Perforce) is needed to use the git-p4 interface to Perforce. @@ -178,6 +185,19 @@ Issues of note: will include them. Note that config.mak is not distributed; the name is reserved for local settings. + - In macOs 10.14, the Command Line Tool not contains sdk headers as before, so +need install Command Line Tool 10.14 and install the headers with command +"sudo installer -pkg /Library/Developer/CommandLineTools/Packages/macOS_SDK_headers_for_macOS_10.14.pkg -target". +If not install the sdk headers correctly, git build will get errors blew, factly is +is because of this problem. + +ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64 +clang: error: linker command failed with exit code 1 (use -v to see invocation) +make: *** [Makefile:2369: git-fast-import] Error 1 +ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64 +clang: error: linker command failed with exit code 1 (use -v to see invocation) +make: *** [Makefile:2046: git] Error 1 + - To build and install documentation suite, you need to have the asciidoc/xmlto toolchain. Because not many people are inclined to install the tools, the default build target -- 2.19.1.1052.gd166e6afe5
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Elijah, On Wed, 14 Nov 2018, Elijah Newren wrote: > On Mon, Nov 12, 2018 at 8:21 AM Johannes Schindelin > wrote: > > > t3425: topology linearization was inconsistent across flavors of rebase, > > > as already noted in a TODO comment in the testcase. This was not > > > considered a bug before, so getting a different linearization due > > > to switching out backends should not be considered a bug now. > > > > Ideally, the test would be fixed, then. If it fails for other reasons than > > a real regression, it is not a very good regression test, is it. > > I agree, it's not the best regression test. It'd be better if it > defined and tested for "the correct behavior", but I suspect it has > some value in that it's is guaranteeing that the rebase flavors at > least give a "somewhat reasonable" result. Sadly, It just allowed all > three rebase types to have slightly different "somewhat reasonable" > answers. True. I hope, though, that we can address this relatively easily (by toggling the `topo_order` flag). > > > t5407: different rebase types varied slightly in how many times checkout > > > or commit or equivalents were called based on a quick comparison > > > of this tests and previous ones which covered different rebase > > > flavors. I think this is just attributable to this difference. > > > > This concerns me. > > > > In bigger repositories (no, I am not talking about Linux kernel sized > > ones, I consider those small-ish) there are a ton of files, and checkout > > and commit (and even more so reset) sadly do not have a runtime complexity > > growing with the number of modified files, but with the number of tracked > > files (and some commands even with the number of worktree files). > > > > So a larger number of commit/checkout operations makes me expect > > performance regressions. > > > > In this light, could you elaborate a bit on the differences you see > > between rebase -i and rebase -m? > > I wrote this comment months ago and don't remember the full details. > From the wording and as best I remember, I suspect I was at least > partially annoyed by the fact that these and other regression tests > for rebase seemed to not be testing for "correct" behavior, but > "existing" behavior. That wouldn't be so bad, except that existing > behavior differed on the exact same test setup for different rebase > backends and the differences in behavior were checked and enforced in > the regression tests. So, it became a matter of taste as to how much > things should be made identical and to which backend, or whether it > was more important to at least just consolidate the code first and > keep the results at least reasonable. At the time, I figured getting > fewer backends, all with reasonable behavior was a bit more important, > but since this difference is worrisome to you, I will try to dig > further into it. I agree that there is a ton of inconsistency going on, both in the behavior of the different backends as well as in the tests and their quality. The best explanation I have for this is: it grew historically, and we always have to strike a balance between strict rule enforcement and fun. > > > t9903: --merge uses the interactive backend so the prompt expected is > > > now REBASE-i. > > > > We should be able to make that test pass, still, by writing out a special > > file (e.g. $state_dir/opt_m) and testing for that. Users are oddly upset > > when their expectations are broken... (and I actually agree with them.) > > I agree users are upset when expectations are broken, but why would > they expect REBASE-m? In fact, I thought the prompt was a reflection > of what backend was in use, so that if someone reported an issue to > the git mailing list or a power user wanted to know where the control > files were located, they could look for them. As such, I thought that > switching the prompt to REBASE-i was the right thing to do so that it > would match user expectations. Yes, the backend changed; that's part > of the release notes. When you put it that way, I have to agree with you. > Is there documentation somewhere that specifies the meaning of this > prompt differently than the expectations I had somehow built up? I was just looking from my perspective, with my experience: if I was a heavy user of `git rebase -m` (I am not), I would stumble over this prompt. That's all. With your explanation, I agree that this is the best course, though. > > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > > > index 3407d835bd..35084f5681 100644 > > > --- a/Documentation/git-rebase.txt > > > +++ b/Documentation/git-rebase.txt > > > @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below. > > > INCOMPATIBLE OPTIONS > > > > > > > > > -git-rebase has many flags that are incompatible with each other, > > > -predominantly due to the fact that it has three different underlying > > >
Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
Hi Junio, On Wed, 14 Nov 2018, Jeff King wrote: > On Wed, Nov 14, 2018 at 02:16:37PM +0100, Johannes Schindelin wrote: > > > > Makes perfect sense. Shouldn't we be asking where the template > > > directory of the installed version is and using it instead of the > > > freshly built one, no? > > > > It would make sense, but we don't know how to get that information, do we? > > > > $ git grep DEFAULT_GIT_TEMPLATE_DIR > > Makefile: -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"' > > builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR > > builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR > > "/usr/share/git-core/templates" > > builtin/init-db.c: template_dir = to_free = > > system_path(DEFAULT_GIT_TEMPLATE_DIR); > > contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \ > > > > In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the > > only user in the code is init-db.c which uses it in copy_templates(). > > > > And changing the code *now* to let us query Git where it thinks its > > templates should be won't work, as this patch is about using the installed > > Git (at whatever pre-compiled version that might be). > > Do we actually care where the templates are? I thought the point was to > override for the built Git to use the built templates instead of the > installed one. For an installed Git, shouldn't we not be overriding the > templates at all? I.e.: > > if test -n "$GIT_TEST_INSTALLED" > then > "$GIT_TEST_INSTALLED/git" init > else > "$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt" > fi >&3 2>&4 > > (That's all leaving aside the question of whether we ought to be using a > clean template dir instead of this). I fear that that might buy us a ton of trouble. Just like we override the system config, we should override the templates. Ciao, Dscho
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Hi Slavica, this looks very good to me. Just one grammar thing: On Wed, 14 Nov 2018, Slavica Djukic wrote: > Add test to document that stash fails if user.name and user.email > are not configured. > In the later commit, test will be updated to expect success. In a later commit [...] Otherwise, I would be totally fine with this version being merged. Ciao, Johannes > > Signed-off-by: Slavica Djukic > --- > t/t3903-stash.sh | 23 +++ > 1 file changed, 23 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index cd216655b..bab8bec67 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1096,4 +1096,27 @@ test_expect_success 'stash -- works with > binary files' ' > test_path_is_file subdir/untracked > ' > > +test_expect_failure 'stash works when user.name and user.email are not set' ' > + git reset && > + git var GIT_COMMITTER_IDENT >expected && > + >1 && > + git add 1 && > + git stash && > + git var GIT_COMMITTER_IDENT >actual && > + test_cmp expected actual && > + >2 && > + git add 2 && > + test_config user.useconfigonly true && > + test_config stash.usebuiltin true && > + ( > + sane_unset GIT_AUTHOR_NAME && > + sane_unset GIT_AUTHOR_EMAIL && > + sane_unset GIT_COMMITTER_NAME && > + sane_unset GIT_COMMITTER_EMAIL && > + test_unconfig user.email && > + test_unconfig user.name && > + git stash > + ) > +' > + > test_done > -- > 2.19.1.1052.gd166e6afe > >
Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
On Thu, Nov 15, 2018 at 01:29:58PM +0100, Johannes Schindelin wrote: > > Do we actually care where the templates are? I thought the point was to > > override for the built Git to use the built templates instead of the > > installed one. For an installed Git, shouldn't we not be overriding the > > templates at all? I.e.: > > > > if test -n "$GIT_TEST_INSTALLED" > > then > > "$GIT_TEST_INSTALLED/git" init > > else > > "$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt" > > fi >&3 2>&4 > > > > (That's all leaving aside the question of whether we ought to be using a > > clean template dir instead of this). > > I fear that that might buy us a ton of trouble. Just like we override the > system config, we should override the templates. Yes, it might. I guess it just seems plausible to me that somebody would expect GIT_TEST_INSTALLED to be as close to the installed experience as possible. I dunno. I do not use it myself. At any rate, my point was that for GIT_TEST_INSTALLED, either: 1. We can use a known-clean set of templates (either our local templates/blt, or an even-cleaner empty set). or 2. We do not need to specify any template, and it will just use whatever it came installed with. And in either case, we do not have to worry about asking it "where are your templates?". -Peff
Re: [PATCH] technical doc: add a design doc for the evolve command
Hi Stefan, On Wed, 14 Nov 2018, sxe...@google.com wrote: > From: Stefan Xenos > > This document describes what an obsolescence graph for > git would look like, the behavior of the evolve command, > and the changes planned for other commands. Thanks, this is a good discussion starter. > +Objective > +- > +Track the edits to a commit over time in an obsolescence graph. I am not sure that we necessarily need this to be a graph. I think part of the problems with not being able to GC *any* of this is by this requirement to have it stored in a graph, rather than having mappings from which you could reconstruct any non-GC'ed parts of that graph, if you really want. > +Background > +-- > +Imagine you have three dependent changes up for review and you receive > feedback > +that requires editing all three changes. While you're editing one, more > feedback > +arrives on one of the others. What do you do? > + > +The evolve command is a convenient way to work with chains of commits that > are > +under review. Whenever you rebase or amend a commit, the repository remembers > +that the old commit is obsolete and has been replaced by the new one. Then, > at > +some point in the future, you can run "git evolve" and the correct sequence > of > +rebases will occur in the correct order such that no commit has an obsolete > +parent. > + > +Part of making the "evolve" command work involves tracking the edits to a > commit > +over time, which is why we need an obsolescence graph. However, the > obsolescence > +graph will also bring other benefits: > + > +- Users can view the history of a commit directly (the sequence of amends and > + rebases it has undergone, orthogonal to the history of the branch it is > on). > +- It will be possible to quickly locate and list all the changes the user > + currently has in progress. > +- It can be used as part of other high-level commands that combine or split > + changes. > +- It can be used to decorate commits (in git log, gitk, etc) that are either > + obsolete or are the tip of a work in progress. > +- By pushing and pulling the obsolescence graph, users can collaborate more > + easily on changes-in-progress. This is better than pushing and pulling the > + changes themselves since the obsolescence graph can be used to locate a > more > + specific merge base, allowing for better merges between different versions > of > + the same change. > +- It could be used to correctly rebase local changes and other local branches > + after running git-filter-branch. > +- It can replace the change-id footer used by gerrit. Okay. > +Similar technologies > + > +There are some other technologies that address the same end-user problem. > + > +Rebase -i can be used to solve the same problem, but users can't easily > switch > +tasks midway through an interactive rebase or have more than one interactive > +rebase going on at the same time. It can't handle the case where you have > +multiple changes sharing the same parent when that parent needs to be rebased > +and won't let you collaborate with others on resolving a complicated > interactive > +rebase. You can think of rebase -i as a top-down approach and the evolve > command > +as the bottom-up approach to the same problem. > + > +Several patch queue managers have been built on top of git (such as topgit, > +stgit, and quilt). They address the same user need. However they also rely on > +state managed outside git that needs to be kept in sync. Such state can be > +easily damaged when running a git native command that is unaware of the patch > +queue. They also typically require an explicit initialization step to be > done by > +the user which creates workflow problems. > + > +Replacements (refs/replace) are superficially similar to obsolescences in > that > +they describe that one commit should be replaced by another. However, they > +differ in both how they are created and how they are intended to be used. > +Obsolescences are created automatically by the commands a user runs, and they > +describe the user’s intent to perform a future rebase. Obsolete commits still > +appear in branches, logs, etc like normal commits (possibly with an extra > +decoration that marks them as obsolete). Replacements are typically created > +explicitly by the user, they are meant to be kept around for a long time, and > +they describe a replacement to be applied at read-time rather than as the > input > +to a future operation. When a replaced commit is queried, it is typically > hidden > +and swapped out with its replacement as though the replacement has already > +occurred. Why is this missing most notably `hg evolve`? Also, there should be *at least* a brief introduction how `hg evolve` works. They do have the benefit of real-world testing, and probably encountered problems and came up with solutions, and we would be remiss if we did not learn from them. Also, please do not forget `git imerge`.
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
Hi Peff,
On Wed, 14 Nov 2018, Jeff King wrote:
> On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote:
>
> > On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren wrote:
> > >
> > > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
> > > wrote:
> > > > However, the `.lock` file was still open and on Windows that means
> > > > that it could not be deleted properly. This patch fixes that issue.
> > >
> > > Hmmm, doesn't the tempfile machinery remove the lock file when we die?
> >
> > On Windows this seems not to be the case. (Open files cannot be deleted
> > as the open file is not kept by inode or similar but by the file path
> > there?)
> >
> > Rewording your concern: Could the tempfile machinery be taught to
> > work properly on Windows, e.g. by first closing all files and then deleting
> > them afterwards?
>
> It already tries to do so. See delete_tempfile(), or more likely in the
> die() case, the remove_tempfiles() handler which is called at exit.
>
> Are we sure this is still a problem?
>
> I looked at the test to see if it would pass, but it is not even
> checking anything about lockfiles! It just checks that we exit 1 by
> returning up the callstack instead of calling die(). And of course it
> would not have a problem under Linux either way. But if I run something
> similar under strace, I see:
>
> $ strace ./git bundle create foobar.bundle HEAD..HEAD
> [...]
> openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock",
> O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
> [...]
> close(3)= 0
> unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
> exit_group(128) = ?
>
> which seems right.
Without the fix, the added regression test fails thusly:
-- snip --
[...]
++ test_expect_code 1 git bundle create foobar.bundle master..master
++ want_code=1
++ shift
++ git bundle create foobar.bundle master..master
fatal: Refusing to create empty bundle.
warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash
directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied
++ exit_code=128
++ test 128 = 1
++ echo 'test_expect_code: command exited with 128, we wanted 1 git bundle
create foobar.bundle master..master'
test_expect_code: command exited with 128, we wanted 1 git bundle create
foobar.bundle master..master
++ return 1
error: last command exited with $?=1
not ok 9 - try to create a bundle with empty ref count
#
# test_expect_code 1 git bundle create foobar.bundle
master..master
#
-- snap --
So yes, we are trying to unlink the `.lock` file, and as far as I can tell that
`unlink()` call comes from the tempfile cleanup asked for by Martin. However, as
we still have a handle open to that file, that call fails.
I do not think that there is any better way to fix this than to close the file
explicitly. If we tried to just close whatever file descriptor is still open to
that file before deleting it, we would possibly cause problems in code that is
still to be executed and assumes that it has a perfectly valid file descriptor.
Besides, trying to do this kind of "automatically" won't work, like, at all,
when it is one child process that holds an open file descriptor while another
process wants to delete the file.
Ciao,
Dscho
