* Re: Bug report: $program_name in error message
From: Stefan Beller @ 2016-12-19 17:07 UTC (permalink / raw)
To: Josh Bleecher Snyder, vascomalmeida; +Cc: git@vger.kernel.org
In-Reply-To: <CAFAcib9-rUSqyBRpauw3pTf9OPTKLYNf7bdh2gyykBNtJTZKGg@mail.gmail.com>
+ Vasco Almeida, who authored d323c6b641,
(i18n: git-sh-setup.sh: mark strings for translation, 2016-06-17)
On Sun, Dec 18, 2016 at 1:44 PM, Josh Bleecher Snyder
<josharian@gmail.com> wrote:
>>> To reproduce, run 'git submodule' from within a bare repo. Result:
>>>
>>> $ git submodule
>>> fatal: $program_name cannot be used without a working tree.
>>>
>>> Looks like the intent was for $program_name to be interpolated.
>>
>> Which version of git do you use?
>
> $ git version
> git version 2.11.0
>
>
>>> As an aside, I sent a message a few days ago about a segfault when
>>> working with a filesystem with direct_io on, but it appears not to
>>> have made it to the archives on marc.info. Am I perhaps still
>>> greylisted?
>>
>> Both emails show up in my mailbox (subscribed to the mailing list),
>> so I think that just nobody answered your first email as the answer
>> may be non trivial.
>
> Thanks for the confirmation.
>
> -josh
^ permalink raw reply
* Re: [PATCH v2 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
From: Johannes Schindelin @ 2016-12-19 17:14 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, git, Junio C Hamano, Kevin Daudt,
Dennis Kaarsemaker
In-Reply-To: <20161215190351.as76panrcz5rgibj@sigill.intra.peff.net>
Hi Peff,
On Thu, 15 Dec 2016, Jeff King wrote:
> On Tue, Dec 13, 2016 at 04:30:01PM +0100, Johannes Schindelin wrote:
>
> > + else {
> > + unsigned char head[20];
> > + struct commit *head_commit;
> > + const char *head_message, *body;
> > +
> > + if (get_sha1("HEAD", head))
> > + return error(_("need a HEAD to fixup"));
> > + if (!(head_commit = lookup_commit_reference(head)))
> > + return error(_("could not read HEAD"));
> > + if (!(head_message = get_commit_buffer(head_commit, NULL)))
> > + return error(_("could not read HEAD's commit message"));
>
> This get_commit_buffer() may allocate a fresh buffer...
>
> > + body = strstr(head_message, "\n\n");
> > + if (!body)
> > + body = "";
> > + else
> > + body = skip_blank_lines(body + 2);
> > + if (write_message(body, strlen(body),
> > + rebase_path_fixup_msg(), 0))
> > + return error(_("cannot write '%s'"),
> > + rebase_path_fixup_msg());
>
> ...and then this return leaks the result (the other code path hits
> unuse_commit_buffer(), and is fine).
Good point.
I found another leaked commit buffer in make_patch() and fixed it, too.
> This leak was noticed by Coverity. It has a _ton_ of false positives
> across the whole project, but it sends out a mail with new ones every
> few days, which is usually short enough that I can process it in 30
> seconds or so.
Yeah, I get these mails now, thanks to Stephan adding me in response to
some issues I introduced with the builtin difftool (and hence I did not
get the warnings when I introduced the problems with sequencer-i).
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] fix pushing to //server/share/dir paths on Windows
From: Johannes Schindelin @ 2016-12-19 17:18 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Git Mailing List
In-Reply-To: <2ff2613c-47da-a780-5d38-93e16cb16328@kdbg.org>
Hi Hannes,
On Tue, 13 Dec 2016, Johannes Sixt wrote:
> Dscho, it looks like this could fix the original report at
> https://github.com/git-for-windows/git/issues/979
Yes, thank you so much for fixing it!
I think your v2 is safe enough to cherry-pick directly into Git for
Windows' `master`.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH v2 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
From: Jeff King @ 2016-12-19 17:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612191758290.54750@virtualbox>
On Mon, Dec 19, 2016 at 05:59:06PM +0100, Johannes Schindelin wrote:
> > > > + sprintf((char *)p, "%d", ++count);
> > >
> > > Do we know the area pointed at p (which is inside buf) long enough
> > > not to overflow? If the original were 9 and you incremented to get
> > > 10, you would need one extra byte.
> >
> > Even if it is enough, I'd ask to please use xsnprintf(). In the off
> > chance that there's a programming error, we'd get a nice die("BUG")
> > instead of a buffer overflow (and it makes the code base easier to audit
> > for other overflows).
>
> I ended up with more verbose, easier-to-read code that does not try to do
> things in-place, in favor of being slightly more wasteful with strbufs.
Great. I agree that should make the whole thing way more readable.
-Peff
^ permalink raw reply
* Re: [PATCH v2 11/34] sequencer (rebase -i): remove CHERRY_PICK_HEAD when no longer needed
From: Johannes Schindelin @ 2016-12-19 17:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqqd1grsn53.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 16 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > The scripted version of the interactive rebase already does that.
>
> Sensible. I was wondering why this wasn't there while reviewing
> 10/34, comparing the two (this is not a suggestion to squash this
> into the previous step).
It's a bit of a historical wart, as I discovered test breakages only long
after implementing the fixup/squash commands (as you may have guessed, I
implemented the commands one after another, fixing things as discovered by
the test suite; it took something like a month until I got the
rebase--helper based rebase -i to pass t3404).
In the end, I decided not to squash this into 10/34 because it seemed to
be a significant enough change to merit its own commit.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v1] t0021: fix flaky test
From: Ramsay Jones @ 2016-12-19 17:24 UTC (permalink / raw)
To: larsxschneider, git; +Cc: peff, gitster
In-Reply-To: <20161218123748.72101-1-larsxschneider@gmail.com>
On 18/12/16 12:37, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> t0021.15 creates files, adds them to the index, and commits them. All
> this usually happens in a test run within the same second and Git cannot
> know if the files have been changed between `add` and `commit`. Thus,
> Git has to run the clean filter in both operations. Sometimes these
> invocations spread over two different seconds and Git can infer that the
> files were not changed between `add` and `commit` based on their
> modification timestamp. The test would fail as it expects the filter
> invocation. Remove this expectation to make the test stable.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>
> Notes:
> Base Commit: f8bf8f2a7b (next)
> Diff on Web: https://github.com/git/git/compare/f8bf8f2a7b...larsxschneider:9d88b66e03
> Checkout: git fetch https://github.com/larsxschneider/git filter-process/fix-flaky-test-v1 && git checkout 9d88b66e03
>
> t/t0021-conversion.sh | 17 +----------------
> 1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 6f16983d3e..161f560446 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -377,22 +377,7 @@ test_expect_success PERL 'required process filter should filter data' '
> EOF
> test_cmp_count expected.log rot13-filter.log &&
>
> - filter_git commit -m "test commit 2" &&
> - cat >expected.log <<-EOF &&
> - START
> - init handshake complete
> - IN: clean test.r $S [OK] -- OUT: $S . [OK]
> - IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
> - IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
> - IN: clean testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK] -- OUT: $S3 . [OK]
> - IN: clean test.r $S [OK] -- OUT: $S . [OK]
> - IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
> - IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
> - IN: clean testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK] -- OUT: $S3 . [OK]
> - STOP
> - EOF
> - test_cmp_count expected.log rot13-filter.log &&
> -
> + git commit -m "test commit 2" &&
> rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x=.r" &&
>
> filter_git checkout --quiet --no-progress . &&
>
I applied this to the pu branch and ran the test by hand
48 times in a row without failure. (the most trials without
error beforehand was 24).
Thanks.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v3 02/16] dir: remove struct path_simplify
From: Stefan Beller @ 2016-12-19 17:33 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <1481670870-66754-3-git-send-email-bmwill@google.com>
On Tue, Dec 13, 2016 at 3:14 PM, Brandon Williams <bmwill@google.com> wrote:
> Teach simplify_away() and exclude_matches_pathspec() to handle struct
> pathspec directly, eliminating the need for the struct path_simplify.
>
> Also renamed the len parameter to pathlen in exclude_matches_pathspec()
> to match the parameter names used in simplify_away().
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
Looks good to me,
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 14/34] sequencer (rebase -i): update refs after a successful rebase
From: Johannes Schindelin @ 2016-12-19 17:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqq8trfsmvr.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 16 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > An interactive rebase operates on a detached HEAD (to keep the reflog
> > of the original branch relatively clean), and updates the branch only
> > at the end.
> >
> > Now that the sequencer learns to perform interactive rebases, it also
> > needs to learn the trick to update the branch before removing the
> > directory containing the state of the interactive rebase.
> >
> > We introduce a new head_ref variable in a wider scope than necessary at
> > the moment, to allow for a later patch that prints out "Successfully
> > rebased and updated <ref>".
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > sequencer.c | 32 +++++++++++++++++++++++++++++++-
> > 1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index a6625e765d..a4e9b326ba 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -100,6 +100,8 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
> > static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
> > static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
> > static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
> > +static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
> > +static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
> >
> > static inline int is_rebase_i(const struct replay_opts *opts)
> > {
> > @@ -1793,12 +1795,39 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
> > }
> >
> > if (is_rebase_i(opts)) {
> > - struct strbuf buf = STRBUF_INIT;
> > + struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
> >
> > /* Stopped in the middle, as planned? */
> > if (todo_list->current < todo_list->nr)
> > return 0;
> >
> > + if (read_oneliner(&head_ref, rebase_path_head_name(), 0) &&
> > + starts_with(head_ref.buf, "refs/")) {
> > + unsigned char head[20], orig[20];
> > +
> > + if (get_sha1("HEAD", head))
> > + return error(_("cannot read HEAD"));
> > + if (!read_oneliner(&buf, rebase_path_orig_head(), 0) ||
> > + get_sha1_hex(buf.buf, orig))
> > + return error(_("could not read orig-head"));
> > + strbuf_addf(&buf, "rebase -i (finish): %s onto ",
> > + head_ref.buf);
> > + if (!read_oneliner(&buf, rebase_path_onto(), 0))
> > + return error(_("could not read 'onto'"));
> > + if (update_ref(buf.buf, head_ref.buf, head, orig,
> > + REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
> > + return error(_("could not update %s"),
> > + head_ref.buf);
> > + strbuf_reset(&buf);
> > + strbuf_addf(&buf,
> > + "rebase -i (finish): returning to %s",
> > + head_ref.buf);
> > + if (create_symref("HEAD", head_ref.buf, buf.buf))
> > + return error(_("could not update HEAD to %s"),
> > + head_ref.buf);
>
> All of the above return error() calls leak head_ref.buf; in addition
> some leak buf.buf, too.
Thanks, fixed.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Johannes Schindelin @ 2016-12-19 17:45 UTC (permalink / raw)
To: Kyle J. McKay; +Cc: Jonathan Tan, Junio C Hamano, Git mailing list
In-Reply-To: <900a55073f78a9f19daca67e468d334@3c843fe6ba8f3c586a21345a2783aa0>
Hi,
On Sat, 17 Dec 2016, Kyle J. McKay wrote:
> Since 6b4b013f18 (mailinfo: handle in-body header continuations,
> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an
> assert of the form:
>
> assert(call_a_function(...))
>
> The function in question, check_header, has side effects. This
> means that when NDEBUG is defined during a release build the
> function call is omitted entirely, the side effects do not
> take place and tests (fortunately) start failing.
>
> Move the function call outside of the assert and assert on
> the result of the function call instead so that the code
> still works properly in a release build and passes the tests.
>
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
ACK. I noticed this problem (and fixed it independently as a part of a
huge patch series I did not get around to submit yet) while trying to get
Git to build correctly with Visual C.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2] git-p4: Fix multi-path changelist empty commits
From: Junio C Hamano @ 2016-12-19 17:49 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, George Vanburgh
In-Reply-To: <010201590ed6ecaa-740c2532-827e-4f5a-af46-0f58d0722db6-000000@eu-west-1.amazonses.com>
George Vanburgh <george@vanburgh.me> writes:
> From: George Vanburgh <gvanburgh@bloomberg.net>
>
> When importing from multiple perforce paths - we may attempt to import
> a changelist that contains files from two (or more) of these depot
> paths. Currently, this results in multiple git commits - one
> containing the changes, and the other(s) as empty commit(s). This
> behavior was introduced in commit 1f90a64
> ("git-p4: reduce number of server queries for fetches", 2015-12-19).
>
> Reproduction Steps:
>
> 1. Have a git repo cloned from a perforce repo using multiple depot
> paths (e.g. //depot/foo and //depot/bar).
> 2. Submit a single change to the perforce repo that makes changes in
> both //depot/foo and //depot/bar.
> 3. Run "git p4 sync" to sync the change from #2.
>
> Change is synced as multiple commits, one for each depot path that was
> affected.
>
> Using a set, instead of a list inside p4ChangesForPaths() ensures that
> each changelist is unique to the returned list, and therefore only a
> single commit is generated for each changelist.
>
> Reported-by: James Farwell <jfarwell@vmware.com>
> Signed-off-by: George Vanburgh <gvanburgh@bloomberg.net>
> ---
Thanks, George. Luke, can I add your "Reviewed-by:" here?
> git-p4.py | 4 ++--
> t/t9800-git-p4-basic.sh | 22 +++++++++++++++++++++-
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..6307bc8 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -822,7 +822,7 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
> die("cannot use --changes-block-size with non-numeric revisions")
> block_size = None
>
> - changes = []
> + changes = set()
>
> # Retrieve changes a block at a time, to prevent running
> # into a MaxResults/MaxScanRows error from the server.
> @@ -841,7 +841,7 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
>
> # Insert changes in chronological order
> for line in reversed(p4_read_pipe_lines(cmd)):
> - changes.append(int(line.split(" ")[1]))
> + changes.add(int(line.split(" ")[1]))
>
> if not block_size:
> break
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 0730f18..4d93522 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -131,6 +131,26 @@ test_expect_success 'clone two dirs, @all, conflicting files' '
> )
> '
>
> +test_expect_success 'clone two dirs, each edited by submit, single git commit' '
> + (
> + cd "$cli" &&
> + echo sub1/f4 >sub1/f4 &&
> + p4 add sub1/f4 &&
> + echo sub2/f4 >sub2/f4 &&
> + p4 add sub2/f4 &&
> + p4 submit -d "sub1/f4 and sub2/f4"
> + ) &&
> + git p4 clone --dest="$git" //depot/sub1@all //depot/sub2@all &&
> + test_when_finished cleanup_git &&
> + (
> + cd "$git" &&
> + git ls-files >lines &&
> + test_line_count = 4 lines &&
> + git log --oneline p4/master >lines &&
> + test_line_count = 5 lines
> + )
> +'
> +
> revision_ranges="2000/01/01,#head \
> 1,2080/01/01 \
> 2000/01/01,2080/01/01 \
> @@ -147,7 +167,7 @@ test_expect_success 'clone using non-numeric revision ranges' '
> (
> cd "$git" &&
> git ls-files >lines &&
> - test_line_count = 6 lines
> + test_line_count = 8 lines
> )
> done
> '
>
> --
> https://github.com/git/git/pull/311
^ permalink raw reply
* Re: [PATCH v2] git-p4: Fix multi-path changelist empty commits
From: Luke Diamand @ 2016-12-19 17:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Users, George Vanburgh
In-Reply-To: <xmqqy3zbq05o.fsf@gitster.mtv.corp.google.com>
On 19 December 2016 at 17:49, Junio C Hamano <gitster@pobox.com> wrote:
> George Vanburgh <george@vanburgh.me> writes:
>
>> From: George Vanburgh <gvanburgh@bloomberg.net>
>>
>> When importing from multiple perforce paths - we may attempt to import
>> a changelist that contains files from two (or more) of these depot
>> paths. Currently, this results in multiple git commits - one
>> containing the changes, and the other(s) as empty commit(s). This
>> behavior was introduced in commit 1f90a64
>> ("git-p4: reduce number of server queries for fetches", 2015-12-19).
>>
>> Reproduction Steps:
>>
>> 1. Have a git repo cloned from a perforce repo using multiple depot
>> paths (e.g. //depot/foo and //depot/bar).
>> 2. Submit a single change to the perforce repo that makes changes in
>> both //depot/foo and //depot/bar.
>> 3. Run "git p4 sync" to sync the change from #2.
>>
>> Change is synced as multiple commits, one for each depot path that was
>> affected.
>>
>> Using a set, instead of a list inside p4ChangesForPaths() ensures that
>> each changelist is unique to the returned list, and therefore only a
>> single commit is generated for each changelist.
>>
>> Reported-by: James Farwell <jfarwell@vmware.com>
>> Signed-off-by: George Vanburgh <gvanburgh@bloomberg.net>
>> ---
>
> Thanks, George. Luke, can I add your "Reviewed-by:" here?
Yes, thanks.
Luke
^ permalink raw reply
* Re: [PATCH] config.c: handle error case for fstat() calls
From: Junio C Hamano @ 2016-12-19 18:14 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, josharian
In-Reply-To: <20161219092155.20359-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Will this fix the problem I'm replying to? I don't know. I found this
> while checking the code and it should be fixed regardless.
Yeah, from a cursory read, it is a step in the right direction to
check the return value of fstat().
Shouldn't the error-return path in the second hunk rollback the
lockfile to clean after itself? The existing "Oh, we cannot chmod
to match the original" check that comes immediately after shares the
same issue, so this is not a new problem, but making an existing one
worse.
> config.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index 83fdecb..4973256 100644
> --- a/config.c
> +++ b/config.c
> @@ -2194,7 +2194,12 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
> goto out_free;
> }
>
> - fstat(in_fd, &st);
> + if (fstat(in_fd, &st) == -1) {
> + error_errno(_("fstat on %s failed"), config_filename);
> + ret = CONFIG_INVALID_FILE;
> + goto out_free;
> + }
> +
> contents_sz = xsize_t(st.st_size);
> contents = xmmap_gently(NULL, contents_sz, PROT_READ,
> MAP_PRIVATE, in_fd, 0);
> @@ -2414,7 +2419,10 @@ int git_config_rename_section_in_file(const char *config_filename,
> goto unlock_and_out;
> }
>
> - fstat(fileno(config_file), &st);
> + if (fstat(fileno(config_file), &st) == -1) {
> + ret = error_errno(_("fstat on %s failed"), config_filename);
> + goto out;
> + }
>
> if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {
> ret = error_errno("chmod on %s failed",
^ permalink raw reply
* Re: [PATCHv8 6/6] submodule: add absorb-git-dir function
From: Stefan Beller @ 2016-12-19 18:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams
In-Reply-To: <20161219053507.GA2335@duynguyen.vn.dektech.internal>
On Sun, Dec 18, 2016 at 9:35 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, Dec 12, 2016 at 11:04:35AM -0800, Stefan Beller wrote:
>> diff --git a/dir.c b/dir.c
>> index e0efd3c2c3..d872cc1570 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2773,3 +2773,15 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
>> free(work_tree);
>> free(git_dir);
>> }
>> +
>> +/*
>> + * Migrate the git directory of the given path from old_git_dir to new_git_dir.
>> + */
>> +void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
>> +{
>> + if (rename(old_git_dir, new_git_dir) < 0)
>> + die_errno(_("could not migrate git directory from '%s' to '%s'"),
>> + old_git_dir, new_git_dir);
>> +
>> + connect_work_tree_and_git_dir(path, new_git_dir);
>
> Should we worry about recovering (e.g. maybe move new_git_dir back to
> old_git_dir) if this connect_work_tree_and_git_dir() fails?
What if the move back fails?
>
> Both write_file() and git_config_set_.. in this function may die(). In
> such a case the repo is in broken state and the user needs pretty good
> submodule understanding to recover from it, I think.
>
> Recovering is not easy (nor entirely safe) either, though I suppose if
> we keep original copies for modified files, then we could restore them
> after moving the directory back and pray the UNIX gods that all
> operations succeed.
There are different levels of brokenness available.
I just tried what happens if core.worktree is unset in a submodule
and that seems to not impact git operations (I only tested git-status
both in the superproject as well as in the submodule).
So I wonder why we set core.worktree at all here as it doesn't
seem to be needed.
Which means that the move of the git directory as well as the .git file
update to point at that moved directory are the important things
to get right.
So maybe:
1) rename the git dir or die
2) write the new gitlink
If that fails remove the .git file (if it exists partially or empty)
and undo 1) by calling rename again with swapped arguments
and then die
3) set core.worktree
If that fails, just warn the user
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
From: Junio C Hamano @ 2016-12-19 18:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1612191145080.54750@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Otherwise I'd prefer to drop it---at this point, the series is merely
>> "just because we can", not "because we need it to further improve this
>> or that".
>
> Oh, I thought that this was meant as a starting point for anybody
> interested in playing with resumable clones or with easing server loads.
It started as such, but the effort stalled and nobody seems to be
taking it further. I do not know it is clear it would be a good
starting point at this point. Perhaps it is.
^ permalink raw reply
* Re: [PATCH 0/5] git check-ref-format --stdin --report-errors
From: Junio C Hamano @ 2016-12-19 18:23 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Ian Jackson, git
In-Reply-To: <561c0338-66cd-f806-7b3b-b422f98a1564@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Especially given that the output is not especially machine-readable, it
> might be more consistent with other commands to call the new feature
> `--verbose` rather than `--report-errors`.
Don't we instead want to structure the output to be machine-readable
instead, given that check-ref-format is a very low level plumbing
command that is primarily meant for scriptors?
^ permalink raw reply
* [PATCH 0/3] push only submodules
From: Brandon Williams @ 2016-12-19 18:25 UTC (permalink / raw)
To: git; +Cc: sbeller, Brandon Williams
This series teaches 'git push' to be able to only push submodules while leaving
a superproject unpushed.
This is a desirable feature in a scenario where updates to the
superproject are handled automatically by some other means, perhaps a
code review tool. In this scenario a developer could make a change
which spans multiple submodules and then push their commits for code
review. Upon completion of the code review, their commits can be
accepted and applied to their respective submodules while the code
review tool can then automatically update the superproject to the most
recent SHA1 of each submodule. This would eliminate the merge conflicts
in the superproject that could occur if multiple people are contributing
to the same submodule.
Brandon Williams (3):
transport: refactor flag #defines to be more readable
submodules: add RECURSE_SUBMODULES_ONLY value
push: add option to push only submodules
builtin/push.c | 2 ++
submodule-config.c | 2 ++
submodule.h | 1 +
t/t5531-deep-submodule-push.sh | 21 +++++++++++++++++++++
transport.c | 15 +++++++++++----
transport.h | 31 ++++++++++++++++---------------
6 files changed, 53 insertions(+), 19 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH 1/3] transport: refactor flag #defines to be more readable
From: Brandon Williams @ 2016-12-19 18:25 UTC (permalink / raw)
To: git; +Cc: sbeller, Brandon Williams
In-Reply-To: <1482171933-180601-1-git-send-email-bmwill@google.com>
All of the #defines for the TRANSPORT_* flags are hardcoded to be powers
of two. This can be error prone when adding a new flag and is difficult
to read. This patch refactors these defines to instead use a shift
operation to generate the flags. This makes adding an additional flag
simpilar and makes the defines easier to read.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
transport.h | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/transport.h b/transport.h
index b8e4ee8..1b65458 100644
--- a/transport.h
+++ b/transport.h
@@ -131,21 +131,21 @@ struct transport {
enum transport_family family;
};
-#define TRANSPORT_PUSH_ALL 1
-#define TRANSPORT_PUSH_FORCE 2
-#define TRANSPORT_PUSH_DRY_RUN 4
-#define TRANSPORT_PUSH_MIRROR 8
-#define TRANSPORT_PUSH_PORCELAIN 16
-#define TRANSPORT_PUSH_SET_UPSTREAM 32
-#define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
-#define TRANSPORT_PUSH_PRUNE 128
-#define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256
-#define TRANSPORT_PUSH_NO_HOOK 512
-#define TRANSPORT_PUSH_FOLLOW_TAGS 1024
-#define TRANSPORT_PUSH_CERT_ALWAYS 2048
-#define TRANSPORT_PUSH_CERT_IF_ASKED 4096
-#define TRANSPORT_PUSH_ATOMIC 8192
-#define TRANSPORT_PUSH_OPTIONS 16384
+#define TRANSPORT_PUSH_ALL (1<<0)
+#define TRANSPORT_PUSH_FORCE (1<<1)
+#define TRANSPORT_PUSH_DRY_RUN (1<<2)
+#define TRANSPORT_PUSH_MIRROR (1<<3)
+#define TRANSPORT_PUSH_PORCELAIN (1<<4)
+#define TRANSPORT_PUSH_SET_UPSTREAM (1<<5)
+#define TRANSPORT_RECURSE_SUBMODULES_CHECK (1<<6)
+#define TRANSPORT_PUSH_PRUNE (1<<7)
+#define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND (1<<8)
+#define TRANSPORT_PUSH_NO_HOOK (1<<9)
+#define TRANSPORT_PUSH_FOLLOW_TAGS (1<<10)
+#define TRANSPORT_PUSH_CERT_ALWAYS (1<<11)
+#define TRANSPORT_PUSH_CERT_IF_ASKED (1<<12)
+#define TRANSPORT_PUSH_ATOMIC (1<<13)
+#define TRANSPORT_PUSH_OPTIONS (1<<14)
extern int transport_summary_width(const struct ref *refs);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 2/3] submodules: add RECURSE_SUBMODULES_ONLY value
From: Brandon Williams @ 2016-12-19 18:25 UTC (permalink / raw)
To: git; +Cc: sbeller, Brandon Williams
In-Reply-To: <1482171933-180601-1-git-send-email-bmwill@google.com>
Add the `RECURSE_SUBMODULES_ONLY` enum value to submodule.h. This enum
value will be used in a later patch to push to indicate that only
submodules should be pushed, while the superproject should remain
unpushed.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
submodule-config.c | 2 ++
submodule.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index 098085b..33eb62d 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -251,6 +251,8 @@ static int parse_push_recurse(const char *opt, const char *arg,
return RECURSE_SUBMODULES_ON_DEMAND;
else if (!strcmp(arg, "check"))
return RECURSE_SUBMODULES_CHECK;
+ else if (!strcmp(arg, "only"))
+ return RECURSE_SUBMODULES_ONLY;
else if (die_on_error)
die("bad %s argument: %s", opt, arg);
else
diff --git a/submodule.h b/submodule.h
index 23d7668..4a83a4c 100644
--- a/submodule.h
+++ b/submodule.h
@@ -6,6 +6,7 @@ struct argv_array;
struct sha1_array;
enum {
+ RECURSE_SUBMODULES_ONLY = -5,
RECURSE_SUBMODULES_CHECK = -4,
RECURSE_SUBMODULES_ERROR = -3,
RECURSE_SUBMODULES_NONE = -2,
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 3/3] push: add option to push only submodules
From: Brandon Williams @ 2016-12-19 18:25 UTC (permalink / raw)
To: git; +Cc: sbeller, Brandon Williams
In-Reply-To: <1482171933-180601-1-git-send-email-bmwill@google.com>
Teach push the --recurse-submodules=only option. This enables push to
recursively push all unpushed submodules while leaving the superproject
unpushed.
This is a desirable feature in a scenario where updates to the
superproject are handled automatically by some other means, perhaps a
code review tool. In this scenario a developer could make a change
which spans multiple submodules and then push their commits for code
review. Upon completion of the code review, their commits can be
accepted and applied to their respective submodules while the code
review tool can then automatically update the superproject to the most
recent SHA1 of each submodule. This would eliminate the merge conflicts
in the superproject that could occur if multiple people are contributing
to the same submodule.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/push.c | 2 ++
t/t5531-deep-submodule-push.sh | 21 +++++++++++++++++++++
transport.c | 15 +++++++++++----
transport.h | 1 +
4 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 3bb9d6b..9433797 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -565,6 +565,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;
+ else if (recurse_submodules == RECURSE_SUBMODULES_ONLY)
+ flags |= TRANSPORT_RECURSE_SUBMODULES_ONLY;
if (tags)
add_refspec("refs/tags/*");
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 1524ff5..676d686 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -454,4 +454,25 @@ test_expect_success 'push --dry-run does not recursively update submodules' '
test_cmp expected_submodule actual_submodule
'
+test_expect_success 'push --dry-run does not recursively update submodules' '
+ git -C work push --dry-run --recurse-submodules=only ../pub.git master &&
+
+ git -C submodule.git rev-parse master >actual_submodule &&
+ git -C pub.git rev-parse master >actual_pub &&
+ test_cmp expected_pub actual_pub &&
+ test_cmp expected_submodule actual_submodule
+'
+
+test_expect_success 'push only unpushed submodules recursively' '
+ git -C pub.git rev-parse master >expected_pub &&
+ git -C work/gar/bage rev-parse master >expected_submodule &&
+
+ git -C work push --recurse-submodules=only ../pub.git master &&
+
+ git -C submodule.git rev-parse master >actual_submodule &&
+ git -C pub.git rev-parse master >actual_pub &&
+ test_cmp expected_submodule actual_submodule &&
+ test_cmp expected_pub actual_pub
+'
+
test_done
diff --git a/transport.c b/transport.c
index 04e5d66..20ebee8 100644
--- a/transport.c
+++ b/transport.c
@@ -947,7 +947,9 @@ int transport_push(struct transport *transport,
if (run_pre_push_hook(transport, remote_refs))
return -1;
- if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
+ if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+ TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
+ !is_bare_repository()) {
struct ref *ref = remote_refs;
struct sha1_array commits = SHA1_ARRAY_INIT;
@@ -965,7 +967,8 @@ int transport_push(struct transport *transport,
}
if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
- ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) &&
+ ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+ TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
!pretend)) && !is_bare_repository()) {
struct ref *ref = remote_refs;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
@@ -984,7 +987,10 @@ int transport_push(struct transport *transport,
sha1_array_clear(&commits);
}
- push_ret = transport->push_refs(transport, remote_refs, flags);
+ if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY))
+ push_ret = transport->push_refs(transport, remote_refs, flags);
+ else
+ push_ret = 0;
err = push_had_errors(remote_refs);
ret = push_ret | err;
@@ -996,7 +1002,8 @@ int transport_push(struct transport *transport,
if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
set_upstreams(transport, remote_refs, pretend);
- if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
+ if (!(flags & (TRANSPORT_PUSH_DRY_RUN |
+ TRANSPORT_RECURSE_SUBMODULES_ONLY))) {
struct ref *ref;
for (ref = remote_refs; ref; ref = ref->next)
transport_update_tracking_ref(transport->remote, ref, verbose);
diff --git a/transport.h b/transport.h
index 1b65458..efd5fb6 100644
--- a/transport.h
+++ b/transport.h
@@ -146,6 +146,7 @@ struct transport {
#define TRANSPORT_PUSH_CERT_IF_ASKED (1<<12)
#define TRANSPORT_PUSH_ATOMIC (1<<13)
#define TRANSPORT_PUSH_OPTIONS (1<<14)
+#define TRANSPORT_RECURSE_SUBMODULES_ONLY (1<<15)
extern int transport_summary_width(const struct ref *refs);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
From: Junio C Hamano @ 2016-12-19 18:31 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Linus Torvalds, Git Mailing List, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612191237400.54750@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Tue, 13 Dec 2016, Junio C Hamano wrote:
>
>> Linus Torvalds <torvalds@linux-foundation.org> writes:
>>
>> > On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> >>
>> >>> +/*
>> >>> + * Note that ordering matters in this enum. Not only must it match the mapping
>> >>> + * below, it is also divided into several sections that matter. When adding
>> >>> + * new commands, make sure you add it in the right section.
>> >>> + */
>> >>
>> >> Good thinking.
>
> Not my thinking... This was in direct response to a suggestion by Dennis
> Kaarsemaker, I cannot take credit for the idea.
I now realize that I was unclear about what "thinking" I found good
in my comment. I do not particularly like defining two parallel
things and having to maintain them in sync. The "Good thinking"
praise goes to whoever thought that this burdensome fact deserves a
clear comment in front of these two things.
And ...
>> Makes me wish C were a better language, though ;-)
>> >
>> > Do this:
>> >
>> > static const char *todo_command_strings[] = {
>> > [TODO_PICK] = "pick",
>> > [TODO_REVERT] = "revert",
>> > [TODO_NOOP] = "noop:,
>> > };
>> >
>> > which makes the array be order-independent.
... solves only one-half of the problem with the language I had.
The order of the entries in this array[] may become more flexible
in the source, but you still have to define enum separately.
I guess if we really want to, we need to resort to something "ugly
but workable" like what you did in fsck.c with FOREACH_MSG_ID(X).
That approach may be the least ugly way if we have to maintain two
or more parallel things in sync.
... and then realizes you wrote pretty much the same thing
... after writing all of the above ;-)
But it is way overkill for sequencer commands that are only handful.
^ permalink raw reply
* Re: [PATCH 0/3] push only submodules
From: Junio C Hamano @ 2016-12-19 18:41 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller
In-Reply-To: <1482171933-180601-1-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> This series teaches 'git push' to be able to only push submodules
> while leaving a superproject unpushed.
> ...
> builtin/push.c | 2 ++
My knee-jerk reaction is "why is this even part of 'git push' if it
does not push?"
I think "git submodule foreach git push" is probably a mouthful to
say, but I am not sure "git push --recurse-submodule=only" is a
short-hand that is way better than that.
Maybe I'll find why the latter is better after reading the patches
through ;-)
^ permalink raw reply
* Re: [PATCH v2 03/34] sequencer (rebase -i): implement the 'edit' command
From: Junio C Hamano @ 2016-12-19 18:47 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612191438200.54750@virtualbox>
^ permalink raw reply
* Re: [PATCH v2 03/34] sequencer (rebase -i): implement the 'edit' command
From: Junio C Hamano @ 2016-12-19 18:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612191438200.54750@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > + strbuf_addf(&buf, "%s/patch", get_dir(opts));
>> > + memset(&log_tree_opt, 0, sizeof(log_tree_opt));
>> > + init_revisions(&log_tree_opt, NULL);
>> > + log_tree_opt.abbrev = 0;
>> > + log_tree_opt.diff = 1;
>> > + log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
>> > + log_tree_opt.disable_stdin = 1;
>> > + log_tree_opt.no_commit_id = 1;
>> > + log_tree_opt.diffopt.file = fopen(buf.buf, "w");
>> > + log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
>> > + if (!log_tree_opt.diffopt.file)
>> > + res |= error_errno(_("could not open '%s'"), buf.buf);
>> > + else {
>> > + res |= log_tree_commit(&log_tree_opt, commit);
>> > + fclose(log_tree_opt.diffopt.file);
>> > + }
>> > + strbuf_reset(&buf);
>> > + strbuf_addf(&buf, "%s/message", get_dir(opts));
>> > + if (!file_exists(buf.buf)) {
>> > + find_commit_subject(commit_buffer, &subject);
>> > + res |= write_message(subject, strlen(subject), buf.buf, 1);
>> > + unuse_commit_buffer(commit, commit_buffer);
>> > + }
>> > + strbuf_release(&buf);
>> > +
>> > + return res;
>> > +}
>>
>> OK. This seems to match what scripted make_patch does in a handful
>> of lines. We probably should have given you a helper to reduce
>> boilerplate that sets up log_tree_opt so that this function does not
>> have to be this long, but that is a separate topic.
>>
>> Does it matter output_format is set to FORMAT_PATCH here, though?
>> With --no-commit-id set, I suspect there is no log message or
>> authorship information given to the output.
Sorry, this was me being stupid.
FORMAT_PATCH here does not have anythning to do with "git
format-patch" (and "git log --pretty=email"). The PATCH there is as
opposed to things like --stat and --raw. We want patch text that
can be fed to "git apply" and it is absolutely the right thing to
use here.
^ permalink raw reply
* Re: [PATCH 3/3] push: add option to push only submodules
From: Stefan Beller @ 2016-12-19 18:56 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org
In-Reply-To: <1482171933-180601-4-git-send-email-bmwill@google.com>
On Mon, Dec 19, 2016 at 10:25 AM, Brandon Williams <bmwill@google.com> wrote:
> Teach push the --recurse-submodules=only option. This enables push to
> recursively push all unpushed submodules while leaving the superproject
> unpushed.
>
> This is a desirable feature in a scenario where updates to the
> superproject are handled automatically by some other means, perhaps a
e.g. Gerrit. (No need to be shy about our code review tool)
>
> code review tool. In this scenario a developer could make a change
> which spans multiple submodules and then push their commits for code
> review. Upon completion of the code review, their commits can be
> accepted and applied to their respective submodules while the code
> review tool can then automatically update the superproject to the most
> recent SHA1 of each submodule. This would eliminate the merge conflicts
> in the superproject that could occur if multiple people are contributing
> to the same submodule.
Code and tests look good to me, I think the commit message
is good enough, but let's hear Junio on this one.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 24/34] sequencer (rebase -i): respect strategy/strategy_opts settings
From: Junio C Hamano @ 2016-12-19 18:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <a21233f368f066408051e6bdc9a2b6ec513e9e11.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The sequencer already has an idea about using different merge
> strategies. We just piggy-back on top of that, using rebase -i's
> own settings, when running the sequencer in interactive rebase mode.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
A handful of steps before and including this one look quite faithful
port to C from the scripted one.
Looking good.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox