Git development
 help / color / mirror / Atom feed
* Re: [wishlist] git submodule update --reset-hard
From: Yaroslav Halchenko @ 2018-12-10 20:14 UTC (permalink / raw)
  Cc: git
In-Reply-To: <CAGZ79kYDa27EFk4A9uEzCnoW7scjb1U8fKwCo0P7rUZESto+Qg@mail.gmail.com>




>
>> Took me some time to figure out
>> why I was getting
>>
>>         fatal: bad value for update parameter
>>
>> after all my changes to the git-submodule.sh script after looking at
>an
>> example commit 42b491786260eb17d97ea9fb1c4b70075bca9523 which
>introduced
>> --merge to the update ;-)
>
>Yeah I saw you also updated the submodule related C code, was that
>fatal message related to that?

Yes

-- 
Yaroslav O. Halchenko (mobile version)
Center for Open Neuroscience   http://centerforopenneuroscience.org
Dartmouth College, NH, USA

^ permalink raw reply

* Re: [PATCH 7/8] checkout: allow ignoring unmatched pathspec
From: Elijah Newren @ 2018-12-10 20:25 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: Git Mailing List, Junio C Hamano,
	Nguyễn Thái Ngọc
In-Reply-To: <20181209200449.16342-8-t.gummerer@gmail.com>

On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> Currently when 'git checkout -- <pathspec>...' is invoked with
> multiple pathspecs, where one or more of the pathspecs don't match
> anything, checkout errors out.
>
> This can be inconvenient in some cases, such as when using git
> checkout from a script.  Introduce a new --ignore-unmatched option,
> which which allows us to ignore a non-matching pathspec instead of
> erroring out.
>
> In a subsequent commit we're going to start using 'git checkout' in
> 'git stash' and are going to make use of this feature.

This makes sense, but seems incomplete.  But to explain it, I think
there's another bug I need to demonstrate first because it's related
on builds on it.  First, the setup:

  $ echo foo >subdir/newfile
  $ git add subdir/newfile
  $ echo bar >>subdir/newfile
  $ git status
  On branch A
  Changes to be committed:
    (use "git reset HEAD <file>..." to unstage)

      new file:   subdir/newfile

  Changes not staged for commit:
    (use "git add <file>..." to update what will be committed)
    (use "git checkout -- <file>..." to discard changes in working directory)

      modified:   subdir/newfile

Now, does it do what we expect?

  $ git checkout HEAD -- subdir/newfile
  error: pathspec 'subdir/newfile' did not match any file(s) known to git

This is the old overlay behavior; kinda lame, but you made no claims
about fixing the default behavior.  What about with your new option?

  $ git checkout --no-overlay HEAD -- subdir
  $ git status
  On branch A
  nothing to commit, working tree clean

Yes, the feature seems to work as advertised.  However, let's try
again with a different variant:

  $ echo foo >subdir/newfile
  $ git checkout --no-overlay HEAD -- subdir
  $ git status
  On branch A
  Untracked files:
    (use "git add <file>..." to include in what will be committed)

      subdir/newfile

Why is the file ignored and left there?  Also:

  $ git checkout --no-overlay HEAD -- subdir/newfile
  error: pathspec 'subdir/newfile' did not match any file(s) known to git

That seems wrong to me.  The point of no-overlay is to make it match
HEAD, and while subdir/newfile doesn't exist in HEAD or the index it
does match in the working tree so the intent is clear. But let's say
that the user did go ahead and specify your new flag:


  $ git checkout --no-overlay --ignore-unmatch HEAD -- subdir/newfile
  $ git status
  On branch A
  Untracked files:
    (use "git add <file>..." to include in what will be committed)

      subdir/newfile

  nothing added to commit but untracked files present (use "git add" to track)

So now it avoids erroring out when the user does more work than
necessary, but it still misses appropriately cleaning up the file.

^ permalink raw reply

* Re: [PATCH 8/8] stash: use git checkout --no-overlay
From: Elijah Newren @ 2018-12-10 20:26 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: Git Mailing List, Junio C Hamano,
	Nguyễn Thái Ngọc
In-Reply-To: <20181209200449.16342-9-t.gummerer@gmail.com>

On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> Now that we have 'git checkout --no-overlay', we can use it in git
> stash, making the codepaths for 'git stash push' with and without
> pathspec more similar, and thus easier to follow.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>
> As mentioned in the cover letter, not sure if we want to apply this
> now.  There are two reasons I did this:
> - Showing the new functionality of git checkout
> - Increased test coverage, as we are running the new code with all git
>   stash tests for free, which helped look at some cases that I was
>   missing initially.
>
>  git-stash.sh | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 94793c1a91..67be04d996 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -314,19 +314,15 @@ push_stash () {
>
>         if test -z "$patch_mode"
>         then
> -               test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
> -               if test -n "$untracked" && test $# = 0
> +               test "$untracked" = "all" && CLEAN_X_OPTION=-X || CLEAN_X_OPTION=
> +               if test -n "$untracked"
>                 then
> -                       git clean --force --quiet -d $CLEAN_X_OPTION
> +                       git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
>                 fi
>
>                 if test $# != 0
>                 then
> -                       test -z "$untracked" && UPDATE_OPTION="-u" || UPDATE_OPTION=
> -                       test "$untracked" = "all" && FORCE_OPTION="--force" || FORCE_OPTION=
> -                       git add $UPDATE_OPTION $FORCE_OPTION -- "$@"
> -                       git diff-index -p --cached --binary HEAD -- "$@" |
> -                       git apply --index -R
> +                       git checkout --quiet --no-overlay --ignore-unmatched HEAD -- "$@"

Nice.  :-)

^ permalink raw reply

* Re: [wishlist] submodule.update config
From: Stefan Beller @ 2018-12-10 20:40 UTC (permalink / raw)
  To: Yaroslav Halchenko; +Cc: git
In-Reply-To: <20181208154539.GH4633@hopa.kiewit.dartmouth.edu>

On Sat, Dec 8, 2018 at 7:45 AM Yaroslav Halchenko <yoh@onerussian.com> wrote:

> I wondered, if you think it would be sensible to also add of
> submodule.update which would be considered before submodule.SUBMODULE.update
> variable possibly defined per submodule.  That would be more inline with desire
> to use any of the --merge, --rebase (and hopefully soon --reset-hard)
> strategies specified as an option for submodule update, where no per-submodule
> handling  is happening.
>
> Thanks in advance for the consideration!

So you are proposing a variable like submodule.update
without the .<name>. that would apply to any submodule?

The precedence in descending order of these
configs that modify the behavior of "git submodule update"
would be:

* the command line flag (--merge/--rebase/--checkout)
* submodule specific instructions (submodule.<name>.update)
* generic submodule config (the new submodule.update)
* default as --checkout

I first hesitated in thinking this would be a good addition,
as there is no plumbing command for submodules,
to easily modify submodules irrespective of the user
config. But that is out already with the submodule
specific update configs.
So I think it may be a good addition.

I wonder if we'd be better off to re-invent the UX instead
of hiding your intentions in a config setting for a command
that is already long to type. What about

  git submodule merge
  git submodule rebase
  git submodule checkout
  git submodule reset (--hard)

as aliases for
  git submodule update (...)

^ permalink raw reply

* Re: Retrieving a file in git that was deleted and committed
From: Elijah Newren @ 2018-12-10 21:33 UTC (permalink / raw)
  To: Jeff King; +Cc: biswaranjan.nitrkl, Bryan Turner, Git Mailing List
In-Reply-To: <20181207072004.GA32603@sigill.intra.peff.net>

On Thu, Dec 6, 2018 at 11:48 PM Jeff King <peff@peff.net> wrote:
>
> On Thu, Dec 06, 2018 at 11:07:00PM -0800, biswaranjan panda wrote:
>
> > Thanks! Strangely git log --follow does work.
>
> I suspect it would work even without --follow. When you limit a log
> traversal with a pathspec, like:
>
>   git log foo
>
> that is not about following some continuous stream of content, but
> rather just applying that pathspec to the diff of each commit, and
> pruning ones where it did not change. So even if there are gaps where
> the file did not exist, we continue to apply the pathspec to the older
> commits.
>
> Tools like git-blame will _not_ work, though, as they really are trying
> to track the content as they walk back through history. And Once all of
> the content seems to appear from nowhere in your new commit, that seems
> like a dead end.
>
> In theory there could be some machine-readable annotation in the commit
> object (or in a note created after the fact) to say "even though 'foo'
> is a new file here, it came from $commit:foo".  And then git-blame could
> keep following the content there. But such a feature does not yet exist.
>
> -Peff

Hmm...sure, if the file is deleted on the only relevant branch through
history...but what if there were another branch where it weren't
deleted?  What does git blame do then?

In other words, do NOT restore the file as biswaranjan suggested, but
instead restore it this way[1]:

git checkout -b keep-foo $REVISION_BEFORE_FOO_DELETED
git commit --allow-empty -m "We want to keep foo"
git checkout A
git merge --no-commit keep-foo
git checkout keep-foo -- foo.txt
git commit


Now, when you run
  git blame foo.txt

blame should notice that foo.txt didn't exist in the first parent
history on A, so it won't bother walking it to find that at some point
foo.txt did exist there.  Instead, it'll walk down the second parent
and follow its history, where it should keep walking back and show all
the old changes...right?  Or did I mess up my testcase and
misunderstand something somehow?


Elijah

[1] Sidenote: it seems like those commands I gave should have simplified down to

git merge --no-commit --no-ff $REVISION_BEFORE_FOO_DELETED
git checkout $REVISION_BEFORE_FOO_DELETED -- foo.txt
git commit

But that seems to error out with "Already up-to-date"...even when
testing with older versions of git, so it wasn't any of my changes.
Not sure why the --no-ff flag doesn't work.

^ permalink raw reply

* Re: What's cooking in git.git (Dec 2018, #01; Sun, 9)
From: Ævar Arnfjörð Bjarmason @ 2018-12-10 21:50 UTC (permalink / raw)
  To: Elijah Newren; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CABPp-BGVHnaZLg4fuptVmNa=jRHg0cMDTWjv1NdLQJXPe=+ahw@mail.gmail.com>


On Mon, Dec 10 2018, Elijah Newren wrote:

> On Sun, Dec 9, 2018 at 12:42 AM Junio C Hamano <gitster@pobox.com> wrote:
>
>> Git 2.20 has been tagged.  I'd expect that we would slow down to see
>> how stable it is and queue only the brown-paper-bag fixes for a week
>> or so, before opening the tree for the next cycle, rewinding the tip
>> of 'next', etc.
>
> Does this mean you'd prefer we continue to wait a little longer before
> sending in new series and re-rolls, or just that you are managing
> expectations about when they might be queued?
>
> (Just curious whether I should submit my rebase-merge-on-sequencer
> re-roll or continue waiting.  I'm happy to do whatever is more
> convenient for others.)

Related; Junio: I've submitted a few things in the last 2-3 weeks which
I knew weren't making it into 2.20, but I just wanted to kick out the
door as I had them ready. Things which are not noted in this "What's
Cooking".

I'm fine with re-submitting those once 2.21 picks up, but don't want to
needlessly spam if your plan is to circle around and pick up those
things you were (rightly) ignoring during the RC period.

^ permalink raw reply

* Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow
From: Josh Steadmon @ 2018-12-10 21:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, stolee, avarab, peff
In-Reply-To: <xmqqsgz74acm.fsf@gitster-ct.c.googlers.com>

On 2018.12.09 13:01, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> > index 5fe21db99f..5b6b44b78e 100755
> > --- a/t/t5318-commit-graph.sh
> > +++ b/t/t5318-commit-graph.sh
> > @@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \
> >  GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))
> >  GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
> >  
> > -# usage: corrupt_graph_and_verify <position> <data> <string>
> > +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
> >  # Manipulates the commit-graph file at the position
> > -# by inserting the data, then runs 'git commit-graph verify'
> > +# by inserting the data, optionally zeroing the file
> > +# starting at <zero_pos>, then runs 'git commit-graph verify'
> >  # and places the output in the file 'err'. Test 'err' for
> >  # the given string.
> >  corrupt_graph_and_verify() {
> >  	pos=$1
> >  	data="${2:-\0}"
> >  	grepstr=$3
> > +	orig_size=$(stat --format=%s $objdir/info/commit-graph)
> 
> "stat(1)" is not so portable, so you'll get complaints from minority
> platform users later.  So is "truncate(1)".

Ack, thanks for the catch. I have a workaround for stat in the form of
"wc -c", and for truncate with a combination of dd and /dev/zero.
However, I'm finding conflicting information about whether or not
/dev/zero exists on macOS. At the least, it sounds like it might not
work on very old versions. Would this be acceptable, or should I add a
new test function to do this?

> > +	zero_pos=${4:-${orig_size}}
> >  	cd "$TRASH_DIRECTORY/full" &&
> >  	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> >  	cp $objdir/info/commit-graph commit-graph-backup &&
> >  	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
> > +	truncate --size=$zero_pos $objdir/info/commit-graph &&
> > +	truncate --size=$orig_size $objdir/info/commit-graph &&
> >  	test_must_fail git commit-graph verify 2>test_err &&
> >  	grep -v "^+" test_err >err
> >  	test_i18ngrep "$grepstr" err
> >  }
> >  
> > +
> >  test_expect_success 'detect bad signature' '
> >  	corrupt_graph_and_verify 0 "\0" \
> >  		"graph signature"
> > @@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' '
> >  		"incorrect checksum"
> >  '
> >  
> > +test_expect_success 'detect incorrect chunk count' '
> > +	corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \
> 
> Implementations of printf(1) may not grok "\xff" as a valid
> representation of "\377".  The shell built-in of dash(1) for example
> would not work with this.

Ack, will fix in V4. Thanks.

> > +		"chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET
> > +'
> > +
> >  test_expect_success 'git fsck (checks commit-graph)' '
> >  	cd "$TRASH_DIRECTORY/full" &&
> >  	git fsck &&

^ permalink raw reply

* Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow
From: Josh Steadmon @ 2018-12-10 21:58 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, git, stolee, avarab, peff
In-Reply-To: <20181210042843.GQ30222@szeder.dev>

On 2018.12.10 05:28, SZEDER Gábor wrote:
> On Sun, Dec 09, 2018 at 01:01:29PM +0900, Junio C Hamano wrote:
> > Josh Steadmon <steadmon@google.com> writes:
> > 
> > > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> > > index 5fe21db99f..5b6b44b78e 100755
> > > --- a/t/t5318-commit-graph.sh
> > > +++ b/t/t5318-commit-graph.sh
> > > @@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \
> > >  GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))
> > >  GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
> > >  
> > > -# usage: corrupt_graph_and_verify <position> <data> <string>
> > > +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
> > >  # Manipulates the commit-graph file at the position
> > > -# by inserting the data, then runs 'git commit-graph verify'
> > > +# by inserting the data, optionally zeroing the file
> > > +# starting at <zero_pos>, then runs 'git commit-graph verify'
> > >  # and places the output in the file 'err'. Test 'err' for
> > >  # the given string.
> > >  corrupt_graph_and_verify() {
> > >  	pos=$1
> > >  	data="${2:-\0}"
> > >  	grepstr=$3
> > > +	orig_size=$(stat --format=%s $objdir/info/commit-graph)
> > 
> > "stat(1)" is not so portable, so you'll get complaints from minority
> > platform users later.  So is "truncate(1)".
> 
> I complain: this patch breaks on macOS (on Travis CI), but in a
> curious way.  First, 'stat' in the above line errors out with:
> 
>   +++stat --format=%s .git/objects/info/commit-graph
>   stat: illegal option -- -
>   usage: stat [-FlLnqrsx] [-f format] [-t timefmt] [file ...]
> 
> Alas, this doesn't immediately fail the test, because it's not part of
> the &&-chain.
> 
> > > +	zero_pos=${4:-${orig_size}}
> 
> No && here, either.
> 
> > >  	cd "$TRASH_DIRECTORY/full" &&
> > >  	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> > >  	cp $objdir/info/commit-graph commit-graph-backup &&
> > >  	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
> > > +	truncate --size=$zero_pos $objdir/info/commit-graph &&
> 
>   ++truncate --size= .git/objects/info/commit-graph
>   t5318-commit-graph.sh: line 385: truncate: command not found
> 
> Note that even if 'truncate' were available, it would most likely
> complain about the empty '--size=' argument resulting from the 'stat'
> error above.
> 
> Alas, this doesn't fail the test, either, because ...
> 
> > > +	truncate --size=$orig_size $objdir/info/commit-graph &&
> > >  	test_must_fail git commit-graph verify 2>test_err &&
> > >  	grep -v "^+" test_err >err
> 
> ... here the &&-chain was broken already before this patch.  However,
> since this above command was not executed due to the missing
> 'truncate', it didn't have a chance to create the 'err' file, ...
> 
> > >  	test_i18ngrep "$grepstr" err
> 
> ... so 'test_i18ngrep' can't find the file, which triggers its linting
> error, finally aborting the whole test script.
> 
> > >  }
> > >  
> > > +
> 
> Stray newline.
> 
> > >  test_expect_success 'detect bad signature' '
> > >  	corrupt_graph_and_verify 0 "\0" \
> > >  		"graph signature"
> > > @@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' '
> > >  		"incorrect checksum"
> > >  '
> > >  
> > > +test_expect_success 'detect incorrect chunk count' '
> > > +	corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \
> > 
> > Implementations of printf(1) may not grok "\xff" as a valid
> > representation of "\377".  The shell built-in of dash(1) for example
> > would not work with this.
> > 
> > > +		"chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET
> > > +'
> > > +
> > >  test_expect_success 'git fsck (checks commit-graph)' '
> > >  	cd "$TRASH_DIRECTORY/full" &&
> > >  	git fsck &&

Thanks for the catch. All these will be fixed in V4.

^ permalink raw reply

* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
From: Johannes Sixt @ 2018-12-10 21:58 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <50ac31ef7f4380f37a0e2d3b75e82b324afee9e3.1544467631.git.gitgitgadget@gmail.com>

Am 10.12.18 um 19:47 schrieb Johannes Schindelin via GitGitGadget:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> When specifying an absolute path without a drive prefix, we convert that
> path internally. Let's make sure that we handle that case properly, too
> ;-)
> 
> This fixes the command
> 
> 	git clone https://github.com/git-for-windows/git \G4W
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/mingw.c            | 10 +++++++++-
>   t/t5580-clone-push-unc.sh |  2 +-
>   2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 34b3880b29..4d009901d8 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)
>   char *mingw_mktemp(char *template)
>   {
>   	wchar_t wtemplate[MAX_PATH];
> +	int offset = 0;
> +
>   	if (xutftowcs_path(wtemplate, template) < 0)
>   		return NULL;
> +
> +	if (is_dir_sep(template[0]) && !is_dir_sep(template[1]) &&
> +	    iswalpha(wtemplate[0]) && wtemplate[1] == L':') {
> +		/* We have an absolute path missing the drive prefix */

This comment is true for the source part, template, but I can't find 
where the destination, wtemplate, suddenly gets the drive prefix. As far 
as I can see, xutftowcs_path() just does a plain textual conversion 
without any interpretation of the text as path. Can you explain it?

BTW, iswalpha() is not restricted to ASCII letters, I would rewrite it 
as (wtemplate[0] <= 127 && isalpha(wtemplate[0]).

> +		offset = 2;
> +	}
>   	if (!_wmktemp(wtemplate))
>   		return NULL;
> -	if (xwcstoutf(template, wtemplate, strlen(template) + 1) < 0)
> +	if (xwcstoutf(template, wtemplate + offset, strlen(template) + 1) < 0)
>   		return NULL;
>   	return template;
>   }
> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
> index c2b0082296..17c38c33a5 100755
> --- a/t/t5580-clone-push-unc.sh
> +++ b/t/t5580-clone-push-unc.sh
> @@ -29,7 +29,7 @@ case "$UNCPATH" in
>   	;;
>   esac
>   
> -test_expect_failure 'clone into absolute path lacking a drive prefix' '
> +test_expect_success 'clone into absolute path lacking a drive prefix' '
>   	USINGBACKSLASHES="$(echo "$WITHOUTDRIVE"/without-drive-prefix |
>   		tr / \\\\)" &&
>   	git clone . "$USINGBACKSLASHES" &&
> 

-- Hannes

^ permalink raw reply

* Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically
From: Johannes Sixt @ 2018-12-10 22:08 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Junio C Hamano
In-Reply-To: <pull.90.git.gitgitgadget@gmail.com>

Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:
> The idea was brought up by Paul Morelle.
> 
> To be honest, this idea of rescheduling a failed exec makes so much sense
> that I wish we had done this from the get-go.

The status quo was actually not that bad a decision, because it made 'x 
false' as a substitute for 'break' very convenient.

But now that we have a real 'break', I'm very much in favor of flipping 
the behavior over to rescheduling. (I'm actually not a user of the 
feature, but the proposed behavior is so compellingly logical.)

-- Hannes

^ permalink raw reply

* silent_exec_failure when calling gpg
From: John Passaro @ 2018-12-10 22:32 UTC (permalink / raw)
  To: git

I've noticed that in v2.19.1, when using git to pretty print
information about the signature, if git cannot find gpg (e.g. "git
config gpg.program nogpg"), it prints an error to stderr:

$ git show -s --pretty=%G?
fatal: cannot run nogpg: No such file or directory
N

When I build from master, that no longer happens:

$ ../git/git show -s --pretty=%G?
N

Is this intentional behavior, i.e. something I can count on being the
case in future releases? Or should I treat this as a bug report? (I
have no opinion on whether this should be a feature or a bug, but I'm
working on a patch whose implementation may look very different based
on the answer to this question.)

I've dug around the code and come up with the notion that setting
child_process.silent_exec_failure = 1 is the usual way of suppressing
this message. But "git log -S silent_exec_failure v2.19.1..master"
shows no changes that could have caused this, nor can I find any
mention of it in release notes. It occurs for just about any
interaction with GPG that I can find, whether signing tags or commits,
or verifying signatures through pretty-print or git verify-commit or
git merge --log or git merge --verify-signatures. In all these cases,
v2.19.1 issues "fatal: cannot run nogpg: No such file or directory" to
stderr, while a binary built from master behaves the same in all
respects (including other errors when trying to sign) except that it's
missing the "fatal:" message.

This behavior makes sense in a lot of ways. If you're interested in
verifying commit signatures, it's hard to imagine needing a reminder
to install the program it depends on (though the error might help you
identify bad configuration for "gpg.program"). Conversely, if you're
not familiar with crypto software and try git log --format=%G? or git
verify-commit for fun, "Fatal: cannot run gpg" may look like a bug. So
I can definitely imagine justifications for this. Nonetheless, the
fact that I can't find documentation for the change smells funny.

I'd be very grateful if somebody on this list could tell me whether I
can count on this behavior in the future, or whether my code should
account for a possibility that this behavior could change in the future.
I'd also be very very interested to see in what commit(s) this change
occurred.

Thanks in advance!

John Passaro
(917) 678-8293

^ permalink raw reply

* Re: [wishlist] submodule.update config
From: Yaroslav Halchenko @ 2018-12-10 22:49 UTC (permalink / raw)
  To: git
In-Reply-To: <CAGZ79kY+F776YfNBrx3wk3ffv4sqqabM5iJxbQDiPE6xoio69w@mail.gmail.com>


On Mon, 10 Dec 2018, Stefan Beller wrote:
> > I wondered, if you think it would be sensible to also add of
> > submodule.update which would be considered before submodule.SUBMODULE.update
> > variable possibly defined per submodule.  That would be more inline with desire
> > to use any of the --merge, --rebase (and hopefully soon --reset-hard)
> > strategies specified as an option for submodule update, where no per-submodule
> > handling  is happening.

> > Thanks in advance for the consideration!

> So you are proposing a variable like submodule.update
> without the .<name>. that would apply to any submodule?

yes

> The precedence in descending order of these
> configs that modify the behavior of "git submodule update"
> would be:

> * the command line flag (--merge/--rebase/--checkout)
> * submodule specific instructions (submodule.<name>.update)
> * generic submodule config (the new submodule.update)
> * default as --checkout

sound great

> I first hesitated in thinking this would be a good addition,
> as there is no plumbing command for submodules,
> to easily modify submodules irrespective of the user
> config. But that is out already with the submodule
> specific update configs.
> So I think it may be a good addition.

Glad to hear that. Not sure though I would know where to stick my
nose to figure out what to change. ;-)

> I wonder if we'd be better off to re-invent the UX instead
> of hiding your intentions in a config setting for a command
> that is already long to type. What about

>   git submodule merge
>   git submodule rebase
>   git submodule checkout
>   git submodule reset (--hard)

> as aliases for
>   git submodule update (...)

Well, not sure... In the long run, if UX is to be tuned up, I wonder if
it would be more worthwhile to look toward making all those git commands
(git merge, git checkout, git rebase, ..., git revert, git cherry-pick)
support --recurse-submodules with a consistent with the non-recursive
operation by default behavior (e.g.  not introducing detached HEADs or
controlling that via a set of additional options where needed).  I feel
that "git-submodule" ideally should not get its interface extended to
complement everything "git" commands can do, although that might need to
be extended to provide necessary plumbing.  As for the UX, it should
provide only the set of additional commands, which could not be present
in the main API (e.g. pure "git submodule" itself to list
submodules, and "submodule foreach", "init", "deinit").

-- 
Yaroslav O. Halchenko
Center for Open Neuroscience     http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419
WWW:   http://www.linkedin.com/in/yarik        

^ permalink raw reply

* [PATCH] http: add http.version option to select http protocol version
From: Silvio Fricke @ 2018-12-10 22:49 UTC (permalink / raw)
  To: git; +Cc: Silvio Fricke

HTTP has several protocol versions. By default, libcurl is using HTTP/2
today and check if the remote can use this protocol variant and fall
back to a previous version if not.

Under rare conditions it is needed to switch the used protocol version
to fight again wrongly implemented authorization mechanism like ntlm
with gssapi on remote side.

Signed-off-by: Silvio Fricke <silvio.fricke@gmail.com>
---

Notes:
    I hit a problem with a libcurl (Namely [this bug]). This bug looks
    like never get fixed and to just-use-git from my commandline I don't want
    compile a own libcurl with disabled gssapi or/and http/2.
    
    [this bug]: https://github.com/curl/curl/issues/876

 Documentation/config/http.txt | 10 ++++++++++
 http.c                        | 23 +++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git Documentation/config/http.txt Documentation/config/http.txt
index a56d848bc0..0d2840696b 100644
--- Documentation/config/http.txt
+++ Documentation/config/http.txt
@@ -68,6 +68,16 @@ http.saveCookies::
 	If set, store cookies received during requests to the file specified by
 	http.cookieFile. Has no effect if http.cookieFile is unset.
 
+http.version::
+	If set, use the specific http(s) protocol version.
+	Actually this versions are possible:
+
+	- 2.0 -> HTTP/2
+	- 2   -> HTTP/2
+	- 1.1 -> HTTP/1.1
+	- 1.0 -> HTTP/1.0
+	- 1   -> HTTP/1.0
+
 http.sslVersion::
 	The SSL version to use when negotiating an SSL connection, if you
 	want to force the default.  The available and default version
diff --git http.c http.c
index eacc2a75ef..99cdd327a5 100644
--- http.c
+++ http.c
@@ -83,6 +83,7 @@ static const char *ssl_cainfo;
 static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
+static const char *curl_http_version;
 static const char *curl_http_proxy;
 static const char *http_proxy_authmethod;
 static struct {
@@ -355,6 +356,10 @@ static int http_options(const char *var, const char *value, void *cb)
 		curl_ftp_no_epsv = git_config_bool(var, value);
 		return 0;
 	}
+
+	if (!strcmp("http.version", var))
+		return git_config_string(&curl_http_version, var, value);
+
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
 
@@ -926,6 +931,19 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
 
+	if (curl_http_version) {
+		if (!strcmp(curl_http_version, "2") || !strcmp(curl_http_version, "2.0"))
+			curl_easy_setopt(result, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2_0);
+		else if (!strcmp(curl_http_version, "2TLS"))
+			curl_easy_setopt(result, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2TLS);
+		else if (!strcmp(curl_http_version, "1.1"))
+			curl_easy_setopt(result, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1);
+		else if (!strcmp(curl_http_version, "1.0") || strcmp(curl_http_version, "1"))
+			curl_easy_setopt(result, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_0);
+		else
+			warning(_("Use default http(s) protocol"));
+	}
+
 #ifdef CURLOPT_USE_SSL
 	if (curl_ssl_try)
 		curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
@@ -1169,6 +1187,11 @@ void http_cleanup(void)
 	curl_slist_free_all(no_pragma_header);
 	no_pragma_header = NULL;
 
+	if (curl_http_version) {
+		free((void *)curl_http_version);
+		curl_http_version = NULL;
+	}
+
 	if (curl_http_proxy) {
 		free((void *)curl_http_proxy);
 		curl_http_proxy = NULL;
-- 
2.19.2


^ permalink raw reply related

* Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically
From: Stefan Beller @ 2018-12-10 22:56 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: gitgitgadget, git, Junio C Hamano
In-Reply-To: <d2c317fd-d10a-19c1-8b4f-a7311c69a52f@kdbg.org>

On Mon, Dec 10, 2018 at 2:08 PM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:
> > The idea was brought up by Paul Morelle.
> >
> > To be honest, this idea of rescheduling a failed exec makes so much sense
> > that I wish we had done this from the get-go.
>
> The status quo was actually not that bad a decision, because it made 'x
> false' as a substitute for 'break' very convenient.
>
> But now that we have a real 'break', I'm very much in favor of flipping
> the behavior over to rescheduling. (I'm actually not a user of the
> feature, but the proposed behavior is so compellingly logical.)

In rare cases I had commands that may be dangerous if rerun,
but I'd just not run them with -y but with -x.

That brings me to some confusion I had in the last patch:
To catch a flaky test I surely would be tempted to
    git rebase -x make -y "make test"
but that might reschedule a compile failure as well,
as a single -y has implications on all other -x's.

I wonder if it might be better to push this mechanism
one layer down: Instead of having a flag that changes
the behavior of the "exec" instructions and having a
handy '-y' short cut for the new mode, we'd rather have
a new type of command that executes&retries a command
("exnrt", 'n'), which can still get the '-y' command line flag,
but more importantly by having 2 separate sets of
commands we'd have one set that is a one-shot, and the
other that is retried. Then we can teach the user which
is safe and which isn't for rescheduling.

By having two classes, I would anticipate fewer compatibility
issues ('"Exec" behaves differently, and I forgot I had turned
on the rescheduling').

Talking about rescheduling: In the above example the flaky
test can flake more than once, so I'd be stuck with keeping
'git rebase --continue'ing after I see the test flaked once again.

My workflow with interactive rebase and fixing up things as I go
always involves a manual final "make test" to check "for real",
which I could lose now, which is nice.

Thanks,
Stefan

^ permalink raw reply

* Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically
From: Elijah Newren @ 2018-12-10 23:13 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Junio C Hamano, Johannes Schindelin

On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:

> The idea was brought up by Paul Morelle.
>
> To be honest, this idea of rescheduling a failed exec makes so much sense
> that I wish we had done this from the get-go.
> 
> So let's do the next best thing: implement a command-line option and a
> config setting to make it so.

I was a bit worried that the optics weren't good enough for recovering from
a typoed exec command (or one that otherwise wouldn't be in a state the
user could make pass but they wanted the rebase to continue).  However,
after trying it out, I found:

$ git rebase --exec /bin/false --reschedule-failed-exec HEAD~1
Executing: /bin/false
warning: execution failed: /bin/false
You can fix the problem, and then run

  git rebase --continue


hint: Could not execute the todo command
hint: 
hint:     exec /bin/false
hint: 
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint: 
hint:     git rebase --edit-todo
hint:     git rebase --continue

and if the user just runs "git rebase --continue" without looking at that big
hint, they'll get the hint again.  When they run "git rebase --edit-todo", they
see the failed command listed first and can remove that line.

So, I think my initial worry was unfounded.

 
> The obvious intent behind that config setting is to not only give users a
> way to opt into that behavior already, but also to make it possible to flip
> the default at a later date, after the feature has been battle-tested and
> after deprecating the non-rescheduling behavior for a couple of Git
> versions.
> 
> If the team agrees with my reasoning, then the 3rd patch (introducing -y
> <cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make
> much sense, as it would introduce a short option that would become obsolete
> soon.

This series is awesome; thanks much to Paul for suggesting this idea.
And yeah, I agree and hope the third patch won't be necessary.  :-)

> Johannes Schindelin (3):
>   rebase: introduce --reschedule-failed-exec
>   rebase: add a config option to default to --reschedule-failed-exec
>   rebase: introduce a shortcut for --reschedule-failed-exec
> 
>  Documentation/config/rebase.txt |  5 ++++
>  Documentation/git-rebase.txt    | 11 +++++++++
>  builtin/rebase--interactive.c   |  2 ++
>  builtin/rebase.c                | 42 ++++++++++++++++++++++++++++++++-
>  git-legacy-rebase.sh            | 24 ++++++++++++++++++-
>  git-rebase--common.sh           |  1 +
>  sequencer.c                     | 13 +++++++---
>  sequencer.h                     |  1 +
>  t/t3418-rebase-continue.sh      | 14 +++++++++++
>  9 files changed, 108 insertions(+), 5 deletions(-)
> 
> 
> base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6
> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-90%2Fdscho%2Freschedule-failed-exec-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-90/dscho/reschedule-failed-exec-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/90
> -- 
> gitgitgadget

^ permalink raw reply

* Re: [RFC PATCH 3/3] list-objects-filter: teach tree:# how to handle >0
From: Matthew DeVore @ 2018-12-10 23:15 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Stefan Beller, git, jeffhost, Jeff King, Stefan Beller,
	Jonathan Tan, pclouds
In-Reply-To: <xmqqlg706iv1.fsf@gitster-ct.c.googlers.com>

On Sun, Oct 14, 2018 at 7:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matthew DeVore <matvore@google.com> writes:
>
> > The long-term goal at the end of this is to allow a partial clone to
> > eagerly fetch an entire directory of files by fetching a tree and
> > specifying <depth>=1. This, for instance, would make a build operation
> > fast and convenient
>
> This would reduce round-trip, as you do not have to fetch the tree
> to see what its contents are before issuing another set of fetches
> for them.  Those who are building virtual filesystem that let you
> mount a specific tree object, perhaps via fuse, may find it useful,
> too, even though I suspect that may not be your primary focus.

I added a paragraph mentioning the "roundtrip" aspect to the commit
message, since this *may* help someone find a use for it.

>
> > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> > index c2c1c40e6..c78985c41 100644
> > --- a/Documentation/rev-list-options.txt
> > +++ b/Documentation/rev-list-options.txt
> > @@ -734,8 +734,12 @@ specification contained in <path>.
> >  +
> >  The form '--filter=tree:<depth>' omits all blobs and trees whose depth
> >  from the root tree is >= <depth> (minimum depth if an object is located
> > -at multiple depths in the commits traversed). Currently, only <depth>=0
> > -is supported, which omits all blobs and trees.
> > +at multiple depths in the commits traversed). <depth>=0 will not include
> > +any trees or blobs unless included explicitly in <object>. <depth>=1
>
> Here, <object> refers to the objects directly requested on the
> command line (or --stdin)?  Triggering this question from me is a
> sign that this description may want to say a bit more to avoid the
> same question from the real readers.  Perhaps replace "included
> explicitly in <object>" with "explicitly requested by listing them
> on the command line or feeding them with `--stdin`", or something
> like that?

I have reworded this to be more explicit about "stdin," and also to
avoid directly admitting that trees are allowed in the <commit>
argument, since I've dropped the s/<commit>/<object>/ patch.

Here is the new paragraph:

The form '--filter=tree:<depth>' omits all blobs and trees whose depth
from the root tree is >= <depth> (minimum depth if an object is located
at multiple depths in the commits traversed). <depth>=0 will not include
any trees or blobs unless included explicitly in the command-line (or
standard input when --stdin is used). <depth>=1 will include only the
tree and blobs which are referenced directly by a commit reachable from
<commit> or an explicitly-given object. <depth>=2 is like <depth>=1
while also including trees and blobs one more level removed from an
explicitly-given commit or tree.

> > +                                     _("expected 'tree:<int>'"));
>
> We do not accept "tree:-1", even though "-1" is an int.  Is it too
> obvious to worry about?  I do not think we want to say tree:<uint>
> even if we do want to make it clear we reject "tree:-1"
>
> I am wondering if "expected 'tree:<depth>'" would work better.

Yes, I agree, <depth> is more helpful and not ambiguous in a
significant way. Fixed.

> > +     unsigned long tree_depth_limit_value;
> >  };
>
> This change gets it right by adding "depth" in the field name and it
> is not a comment on this patch, but someday not in too distant
> future we should rename the "blob_limit_value" to make it clear that
> it is filtering by number of bytes, as other filtering criteria on
> blobs that can be expressed in ulong are quite possible.

Agreed.

>
> > -static enum list_objects_filter_result filter_trees_none(
> > +static enum list_objects_filter_result filter_trees_depth(
> >       enum list_objects_filter_situation filter_situation,
> >       struct object *obj,
> >       const char *pathname,
> >       const char *filename,
> >       void *filter_data_)
> >  {
> > -     struct filter_trees_none_data *filter_data = filter_data_;
> > +     struct filter_trees_depth_data *filter_data = filter_data_;
> > +
> > +     int too_deep = filter_data->current_depth >= filter_data->max_depth;
>
> Does max mean "maximum allowed", or "this and anything larger are
> rejected".  The latter sound wrong, but I offhand do not know if
> your current_depth counts from 0 or 1, so there may not be
> off-by-one.
>
It means "this and anything larger are rejected." The documentation
words it similarly:
"
The form '--filter=tree:<depth>' omits all blobs and trees whose depth
from the root tree is >= <depth> ...
"

There is no intuitive phrase to mean "distance from root tree minus
one" (left operand) and I don't want to change the filter option field
to something different from what the user entered (right operand), so
I think we'd best use ">=" and I've renamed the field to
"exclusion_trigger_depth".

> There may be other examples.  One existing violator I noticed was
> the "reject blobs that is this size or larger" in this file; it is
> called "max_bytes", but it is apparently not "maximum allowed",
> which we probably would want to fix.
>

That's not ideal. The documentation suggests it means "maximum
allowed," and JGit apparently is interpreting the value as "maximum
allowed," so we should probably fix the semantics in Git. Or were you
suggesting to keep the behavior the same but fix the naming and docs?

> > +     /*
> > +      * Note that we do not use _MARK_SEEN in order to allow re-traversal in
> > +      * case we encounter a tree or blob again at a shallower depth.
> > +      */
>
> Hmph.  Earlier tree:0 support never even read the actual tree, so
> this was not a problem.  We wouldn't have found a tree in deeper
> places first and then at a shallower depth, as we wouldn't have seen
> any tree in any depth deeper than the surface anyway.
>
> Now we need to worry about a tree that originally gets seen in a
> deeper depth (that is still below the allowed maximum) to reappear
> at a shallower place, so a subtree within it that used to be too
> deep may now be within the allowed maximum depth.

That's true, and it points out the fact that we can't use
LOFR_MARK_SEEN even if we do LOFR_DO_SHOW, since subtrees may or may
not be within the limit in the future. I've fixed that for v+1 of the
patch, and added a corresponding test. I'm also refactoring the
filters_tree_depth function to not use goto. I think it is easier to
follow that way.

>
> Step 1 of these three patches made sure trees are not even opened
> under "tree:0", so it was not just optimizing/shrinking the output
> of rev-list but also optimizing the traversal.  When we are
> collecting omits, however, this one now returns _ZERO which means we
> still traverse into the tree, even under "tree:0"?  I must be
> reading the code incorrectly (in general, when we are seeing a tree
> object that itself is at the maximum allowed depth, trees found by
> reading its contents will never become eligible for output, even if
> they are found at a shallower depth than their other copies were
> previously found, I would think).

You are reading the code right - when the tree is too deep, we
sometimes return LOFR_ZERO, sometimes _SKIP_TREE - the latter if we
are collecting omits. But as you observe, we don't need to traverse an
omitted tree *if it has already been marked as omitted*, since that
suggests that its children have also been marked as omitted. So I've
added a new commit to this patchset which optimizes this case. The
next patchset will have 2 commits (subtracting the documentation
rewrite one and ones that are already scheduled to merge):

Patch 1/2 - implement tree:<depth> for positive depths
Patch 2/2 - stop iterating through trees for collecting omits if the
omits are already marked

^ permalink raw reply

* Re: [PATCH 1/3] rebase: introduce --reschedule-failed-exec
From: Elijah Newren @ 2018-12-10 23:18 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin
In-Reply-To: <05d8792d12e692eeefa0021e8686b7211a055593.1544468695.git.gitgitgadget@gmail.com>

On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> A common use case for the `--exec` option is to verify that each commit
> in a topic branch compiles cleanly, via `git rebase -x make <base>`.
>
> However, when an `exec` in such a rebase fails, it is not re-scheduled,
> which in this instance is not particularly helpful.
>
> Let's offer a flag to reschedule failed `exec` commands.
>
> Based on an idea by Paul Morelle.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Documentation/git-rebase.txt  |  5 +++++
>  builtin/rebase--interactive.c |  2 ++
>  builtin/rebase.c              | 16 +++++++++++++++-
>  git-legacy-rebase.sh          | 16 +++++++++++++++-
>  git-rebase--common.sh         |  1 +
>  sequencer.c                   | 13 ++++++++++---
>  sequencer.h                   |  1 +
>  t/t3418-rebase-continue.sh    |  6 ++++++
>  8 files changed, 55 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 80793bad8d..9dd68f77f6 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -501,6 +501,11 @@ See also INCOMPATIBLE OPTIONS below.
>         with care: the final stash application after a successful
>         rebase might result in non-trivial conflicts.
>
> +--reschedule-failed-exec::
> +--no-reschedule-failed-exec::
> +       Automatically reschedule `exec` commands that failed. This only makes
> +       sense in interactive mode (or when an `--exec` option was provided).
> +
>  INCOMPATIBLE OPTIONS
>  --------------------
>
> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> index a2ab68ed06..a9ab009fdb 100644
> --- a/builtin/rebase--interactive.c
> +++ b/builtin/rebase--interactive.c
> @@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>                 OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
>                 OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
>                 OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
> +               OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
> +                        N_("automatically re-schedule any `exec` that fails")),
>                 OPT_END()
>         };
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b3e5baec8..6d556fc6c8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -104,6 +104,7 @@ struct rebase_options {
>         int rebase_merges, rebase_cousins;
>         char *strategy, *strategy_opts;
>         struct strbuf git_format_patch_opt;
> +       int reschedule_failed_exec;
>  };
>
>  static int is_interactive(struct rebase_options *opts)
> @@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)
>                         argv_array_push(&child.args, opts->gpg_sign_opt);
>                 if (opts->signoff)
>                         argv_array_push(&child.args, "--signoff");
> +               if (opts->reschedule_failed_exec)
> +                       argv_array_push(&child.args, "--reschedule-failed-exec");
>
>                 status = run_command(&child);
>                 goto finished_rebase;
> @@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                                    "strategy")),
>                 OPT_BOOL(0, "root", &options.root,
>                          N_("rebase all reachable commits up to the root(s)")),
> +               OPT_BOOL(0, "reschedule-failed-exec",
> +                        &options.reschedule_failed_exec,
> +                        N_("automatically re-schedule any `exec` that fails")),
>                 OPT_END(),
>         };
>         int i;
> @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                 break;
>         }
>
> +       if (options.reschedule_failed_exec && !is_interactive(&options))
> +               die(_("--reschedule-failed-exec requires an interactive rebase"));
> +

I was surprised at first that you checked is_interactive() rather than
checking for --exec being specified.  But I guess this is because
users can manually specify 'exec' lines.

What if the user specifies an implicitly interactive rebase (i.e. no
editing of the todo list, such as with --rebase-merges or
--keep-empty, or soon --strategy or --strategy-option) and also
doesn't specify --exec?

>         if (options.git_am_opts.argc) {
>                 /* all am options except -q are compatible only with --am */
>                 for (i = options.git_am_opts.argc - 1; i >= 0; i--)
> @@ -1220,7 +1229,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                 options.flags |= REBASE_FORCE;
>         }
>
> -       if (options.type == REBASE_PRESERVE_MERGES)
> +       if (options.type == REBASE_PRESERVE_MERGES) {
>                 /*
>                  * Note: incompatibility with --signoff handled in signoff block above
>                  * Note: incompatibility with --interactive is just a strong warning;
> @@ -1230,6 +1239,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                         die(_("error: cannot combine '--preserve-merges' with "
>                               "'--rebase-merges'"));
>
> +               if (options.reschedule_failed_exec)
> +                       die(_("error: cannot combine '--preserve-merges' with "
> +                             "'--reschedule-failed-exec'"));
> +       }
> +
>         if (options.rebase_merges) {
>                 if (strategy_options.nr)
>                         die(_("error: cannot combine '--rebase-merges' with "
> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index b97ffdc9dd..5f0f0c5ab8 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -48,6 +48,7 @@ skip!              skip current patch and continue
>  edit-todo!         edit the todo list during an interactive rebase
>  quit!              abort but keep HEAD where it is
>  show-current-patch! show the patch file being applied or merged
> +reschedule-failed-exec automatically reschedule failed exec commands
>  "
>  . git-sh-setup
>  set_reflog_action rebase
> @@ -92,6 +93,7 @@ autosquash=
>  keep_empty=
>  allow_empty_message=--allow-empty-message
>  signoff=
> +reschedule_failed_exec=
>  test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
>  case "$(git config --bool commit.gpgsign)" in
>  true)  gpg_sign_opt=-S ;;
> @@ -126,6 +128,8 @@ read_basic_state () {
>                 signoff="$(cat "$state_dir"/signoff)"
>                 force_rebase=t
>         }
> +       test -f "$state_dir"/reschedule-failed-exec &&
> +               reschedule_failed_exec=t
>  }
>
>  finish_rebase () {
> @@ -163,7 +167,8 @@ run_interactive () {
>                 "$allow_empty_message" "$autosquash" "$verbose" \
>                 "$force_rebase" "$onto_name" "$head_name" "$strategy" \
>                 "$strategy_opts" "$cmd" "$switch_to" \
> -               "$allow_rerere_autoupdate" "$gpg_sign_opt" "$signoff"
> +               "$allow_rerere_autoupdate" "$gpg_sign_opt" "$signoff" \
> +               "$reschedule_failed_exec"
>  }
>
>  run_specific_rebase () {
> @@ -378,6 +383,12 @@ do
>         --gpg-sign=*)
>                 gpg_sign_opt="-S${1#--gpg-sign=}"
>                 ;;
> +       --reschedule-failed-exec)
> +               reschedule_failed_exec=--reschedule-failed-exec
> +               ;;
> +       --no-reschedule-failed-exec)
> +               reschedule_failed_exec=
> +               ;;
>         --)
>                 shift
>                 break
> @@ -534,6 +545,9 @@ then
>         #       git-rebase.txt caveats with "unless you know what you are doing"
>         test -n "$rebase_merges" &&
>                 die "$(gettext "error: cannot combine '--preserve-merges' with '--rebase-merges'")"
> +
> +       test -n "$reschedule_failed_exec" &&
> +               die "$(gettext "error: cannot combine '--preserve-merges' with '--reschedule-failed-exec'")"
>  fi
>
>  if test -n "$rebase_merges"

In the builtin rebase, you checked that --reschedule-failed-exec had
to be used with an interactive rebase.  Here in the legacy rebase you
have no such check at all.

Not sure if that's an oversight, or if we're at the point where we
just start intentionally allowing legacy rebase to lag and soon throw
it out.  (When do we get to that point?)

> diff --git a/git-rebase--common.sh b/git-rebase--common.sh
> index 7e39d22871..a8a44608e0 100644
> --- a/git-rebase--common.sh
> +++ b/git-rebase--common.sh
> @@ -19,6 +19,7 @@ write_basic_state () {
>                 "$state_dir"/allow_rerere_autoupdate
>         test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
>         test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
> +       test -n "$reschedule_failed_exec" && : > "$state_dir"/reschedule-failed-exec
>  }
>
>  apply_autostash () {
> diff --git a/sequencer.c b/sequencer.c
> index e1a4dd15f1..69bee63e11 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
>  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
>  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
>  static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
> +static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
>
>  static int git_sequencer_config(const char *k, const char *v, void *cb)
>  {
> @@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)
>                         opts->signoff = 1;
>                 }
>
> +               if (file_exists(rebase_path_reschedule_failed_exec()))
> +                       opts->reschedule_failed_exec = 1;
> +
>                 read_strategy_opts(opts, &buf);
>                 strbuf_release(&buf);
>
> @@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
>                 write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
>         if (opts->signoff)
>                 write_file(rebase_path_signoff(), "--signoff\n");
> +       if (opts->reschedule_failed_exec)
> +               write_file(rebase_path_reschedule_failed_exec(), "%s", "");
>
>         return 0;
>  }
> @@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>                         *end_of_arg = saved;
>
>                         /* Reread the todo file if it has changed. */
> -                       if (res)
> -                               ; /* fall through */
> -                       else if (stat(get_todo_path(opts), &st))
> +                       if (res) {
> +                               if (opts->reschedule_failed_exec)
> +                                       reschedule = 1;
> +                       } else if (stat(get_todo_path(opts), &st))
>                                 res = error_errno(_("could not stat '%s'"),
>                                                   get_todo_path(opts));
>                         else if (match_stat_data(&todo_list->stat, &st)) {
> diff --git a/sequencer.h b/sequencer.h
> index 5071a73563..1f865dae26 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -39,6 +39,7 @@ struct replay_opts {
>         int allow_empty_message;
>         int keep_redundant_commits;
>         int verbose;
> +       int reschedule_failed_exec;
>
>         int mainline;
>
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 0210b2ac6f..54b26a9284 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -254,4 +254,10 @@ test_expect_success 'the todo command "break" works' '
>         test_path_is_file execed
>  '
>
> +test_expect_success '--reschedule-failed-exec' '
> +       test_when_finished "git rebase --abort" &&
> +       test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
> +       grep "^exec false" .git/rebase-merge/git-rebase-todo
> +'
> +
>  test_done

The rest of the patch looks good to me.

^ permalink raw reply

* Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically
From: Elijah Newren @ 2018-12-10 23:20 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <pull.90.git.gitgitgadget@gmail.com>

On Mon, Dec 10, 2018 at 3:13 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> The idea was brought up by Paul Morelle.
>
> To be honest, this idea of rescheduling a failed exec makes so much sense
> that I wish we had done this from the get-go.
>
> So let's do the next best thing: implement a command-line option and a
> config setting to make it so.
>
> The obvious intent behind that config setting is to not only give users a
> way to opt into that behavior already, but also to make it possible to flip
> the default at a later date, after the feature has been battle-tested and
> after deprecating the non-rescheduling behavior for a couple of Git
> versions.
>
> If the team agrees with my reasoning, then the 3rd patch (introducing -y
> <cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make
> much sense, as it would introduce a short option that would become obsolete
> soon.
>

Complete side-track: This email showed up for me just five minutes
ago, whereas the rest of the series showed up four hours ago, making
me think this email had disappeared and trying to figure out how to
respond when I didn't have the original.  Any ideas why there might be
that level of lag?

^ permalink raw reply

* [PATCH v2 0/2] support for filtering trees and blobs based on depth
From: Matthew DeVore @ 2018-12-10 23:40 UTC (permalink / raw)
  To: git
  Cc: Matthew DeVore, sbeller, git, jeffhost, peff, stefanbeller,
	jonathantanmy, pclouds

This is a follow-up to the original patchset at
https://public-inbox.org/git/cover.1539298957.git.matvore@google.com/ -
the purpose of that patchset and this one is to support positive integers for
tree:<depth>. Some of the prior patchset's patches were either already queued
(tree traversal optimization) or abandoned (documentation edit). This rendition
of the patchset adds a commit which optimizes away tree traversal when
collecting omits when iterating over a tree a second time.

Thanks,

Matthew DeVore (2):
  list-objects-filter: teach tree:# how to handle >0
  tree:<depth>: skip some trees even when collecting omits

 Documentation/rev-list-options.txt  |   9 ++-
 list-objects-filter-options.c       |   7 +-
 list-objects-filter-options.h       |   3 +-
 list-objects-filter.c               | 120 +++++++++++++++++++++++-----
 t/t6112-rev-list-filters-objects.sh | 115 +++++++++++++++++++++++++-
 5 files changed, 225 insertions(+), 29 deletions(-)

-- 
2.20.0.rc2.403.gdbc3b29805-goog


^ permalink raw reply

* [PATCH v2 1/2] list-objects-filter: teach tree:# how to handle >0
From: Matthew DeVore @ 2018-12-10 23:40 UTC (permalink / raw)
  To: git
  Cc: Matthew DeVore, sbeller, git, jeffhost, peff, stefanbeller,
	jonathantanmy, pclouds
In-Reply-To: <20181210234030.176178-1-matvore@google.com>

Implement positive values for <depth> in the tree:<depth> filter. The
exact semantics are described in Documentation/rev-list-options.txt.

The long-term goal at the end of this is to allow a partial clone to
eagerly fetch an entire directory of files by fetching a tree and
specifying <depth>=1. This, for instance, would make a build operation
fast and convenient. It is fast because the partial clone does not need
to fetch each file individually, and convenient because the user does
not need to supply a sparse-checkout specification.

Another way of considering this feature is as a way to reduce
round-trips, since the client can get any number of levels of
directories in a single request, rather than wait for each level of tree
objects to come back, whose entries are used to construct a new request.

Signed-off-by: Matthew DeVore <matvore@google.com>
---
 Documentation/rev-list-options.txt  |   9 ++-
 list-objects-filter-options.c       |   7 +-
 list-objects-filter-options.h       |   3 +-
 list-objects-filter.c               | 115 +++++++++++++++++++++++-----
 t/t6112-rev-list-filters-objects.sh | 104 +++++++++++++++++++++++++
 5 files changed, 210 insertions(+), 28 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index bab5f50b17..f8ab00f7c9 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -734,8 +734,13 @@ specification contained in <path>.
 +
 The form '--filter=tree:<depth>' omits all blobs and trees whose depth
 from the root tree is >= <depth> (minimum depth if an object is located
-at multiple depths in the commits traversed). Currently, only <depth>=0
-is supported, which omits all blobs and trees.
+at multiple depths in the commits traversed). <depth>=0 will not include
+any trees or blobs unless included explicitly in the command-line (or
+standard input when --stdin is used). <depth>=1 will include only the
+tree and blobs which are referenced directly by a commit reachable from
+<commit> or an explicitly-given object. <depth>=2 is like <depth>=1
+while also including trees and blobs one more level removed from an
+explicitly-given commit or tree.
 
 --no-filter::
 	Turn off any previous `--filter=` argument.
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index e8da2e8581..5285e7674d 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -50,16 +50,15 @@ static int gently_parse_list_objects_filter(
 		}
 
 	} else if (skip_prefix(arg, "tree:", &v0)) {
-		unsigned long depth;
-		if (!git_parse_ulong(v0, &depth) || depth != 0) {
+		if (!git_parse_ulong(v0, &filter_options->tree_exclude_depth)) {
 			if (errbuf) {
 				strbuf_addstr(
 					errbuf,
-					_("only 'tree:0' is supported"));
+					_("expected 'tree:<depth>'"));
 			}
 			return 1;
 		}
-		filter_options->choice = LOFC_TREE_NONE;
+		filter_options->choice = LOFC_TREE_DEPTH;
 		return 0;
 
 	} else if (skip_prefix(arg, "sparse:oid=", &v0)) {
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index af64e5c66f..477cd97029 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -10,7 +10,7 @@ enum list_objects_filter_choice {
 	LOFC_DISABLED = 0,
 	LOFC_BLOB_NONE,
 	LOFC_BLOB_LIMIT,
-	LOFC_TREE_NONE,
+	LOFC_TREE_DEPTH,
 	LOFC_SPARSE_OID,
 	LOFC_SPARSE_PATH,
 	LOFC__COUNT /* must be last */
@@ -44,6 +44,7 @@ struct list_objects_filter_options {
 	struct object_id *sparse_oid_value;
 	char *sparse_path_value;
 	unsigned long blob_limit_value;
+	unsigned long tree_exclude_depth;
 };
 
 /* Normalized command line arguments */
diff --git a/list-objects-filter.c b/list-objects-filter.c
index a62624a1ce..ca99f0dd02 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -10,6 +10,7 @@
 #include "list-objects.h"
 #include "list-objects-filter.h"
 #include "list-objects-filter-options.h"
+#include "oidmap.h"
 #include "oidset.h"
 #include "object-store.h"
 
@@ -84,11 +85,43 @@ static void *filter_blobs_none__init(
  * A filter for list-objects to omit ALL trees and blobs from the traversal.
  * Can OPTIONALLY collect a list of the omitted OIDs.
  */
-struct filter_trees_none_data {
+struct filter_trees_depth_data {
 	struct oidset *omits;
+
+	/*
+	 * Maps trees to the minimum depth at which they were seen. It is not
+	 * necessary to re-traverse a tree at deeper or equal depths than it has
+	 * already been traversed.
+	 *
+	 * We can't use LOFR_MARK_SEEN for tree objects since this will prevent
+	 * it from being traversed at shallower depths.
+	 */
+	struct oidmap seen_at_depth;
+
+	unsigned long exclude_depth;
+	unsigned long current_depth;
 };
 
-static enum list_objects_filter_result filter_trees_none(
+struct seen_map_entry {
+	struct oidmap_entry base;
+	size_t depth;
+};
+
+static void filter_trees_update_omits(
+	struct object *obj,
+	struct filter_trees_depth_data *filter_data,
+	int include_it)
+{
+	if (!filter_data->omits)
+		return;
+
+	if (include_it)
+		oidset_remove(filter_data->omits, &obj->oid);
+	else
+		oidset_insert(filter_data->omits, &obj->oid);
+}
+
+static enum list_objects_filter_result filter_trees_depth(
 	struct repository *r,
 	enum list_objects_filter_situation filter_situation,
 	struct object *obj,
@@ -96,43 +129,83 @@ static enum list_objects_filter_result filter_trees_none(
 	const char *filename,
 	void *filter_data_)
 {
-	struct filter_trees_none_data *filter_data = filter_data_;
+	struct filter_trees_depth_data *filter_data = filter_data_;
+	struct seen_map_entry *seen_info;
+	int include_it = filter_data->current_depth <
+		filter_data->exclude_depth;
+	int filter_res;
+	int already_seen;
+
+	/*
+	 * Note that we do not use _MARK_SEEN in order to allow re-traversal in
+	 * case we encounter a tree or blob again at a shallower depth.
+	 */
 
 	switch (filter_situation) {
 	default:
 		BUG("unknown filter_situation: %d", filter_situation);
 
-	case LOFS_BEGIN_TREE:
-	case LOFS_BLOB:
-		if (filter_data->omits) {
-			oidset_insert(filter_data->omits, &obj->oid);
-			/* _MARK_SEEN but not _DO_SHOW (hard omit) */
-			return LOFR_MARK_SEEN;
-		} else {
-			/*
-			 * Not collecting omits so no need to to traverse tree.
-			 */
-			return LOFR_SKIP_TREE | LOFR_MARK_SEEN;
-		}
-
 	case LOFS_END_TREE:
 		assert(obj->type == OBJ_TREE);
+		filter_data->current_depth--;
 		return LOFR_ZERO;
 
+	case LOFS_BLOB:
+		filter_trees_update_omits(obj, filter_data, include_it);
+		return include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO;
+
+	case LOFS_BEGIN_TREE:
+		seen_info = oidmap_get(
+			&filter_data->seen_at_depth, &obj->oid);
+		if (!seen_info) {
+			seen_info = xcalloc(1, sizeof(struct seen_map_entry));
+			seen_info->base.oid = obj->oid;
+			seen_info->depth = filter_data->current_depth;
+			oidmap_put(&filter_data->seen_at_depth, seen_info);
+			already_seen = 0;
+		} else
+			already_seen =
+				filter_data->current_depth >= seen_info->depth;
+
+		if (already_seen)
+			filter_res = LOFR_SKIP_TREE;
+		else {
+			seen_info->depth = filter_data->current_depth;
+			filter_trees_update_omits(obj, filter_data, include_it);
+
+			if (include_it)
+				filter_res = LOFR_DO_SHOW;
+			else if (filter_data->omits)
+				filter_res = LOFR_ZERO;
+			else
+				filter_res = LOFR_SKIP_TREE;
+		}
+
+		filter_data->current_depth++;
+		return filter_res;
 	}
 }
 
-static void* filter_trees_none__init(
+static void filter_trees_free(void *filter_data) {
+	struct filter_trees_depth_data* d = filter_data;
+	oidmap_free(&d->seen_at_depth, 1);
+	free(d);
+}
+
+static void* filter_trees_depth__init(
 	struct oidset *omitted,
 	struct list_objects_filter_options *filter_options,
 	filter_object_fn *filter_fn,
 	filter_free_fn *filter_free_fn)
 {
-	struct filter_trees_none_data *d = xcalloc(1, sizeof(*d));
+	struct filter_trees_depth_data *d = xcalloc(1, sizeof(*d));
 	d->omits = omitted;
+	oidmap_init(&d->seen_at_depth, 0);
+	d->exclude_depth = filter_options->tree_exclude_depth;
+	d->current_depth = 0;
 
-	*filter_fn = filter_trees_none;
-	*filter_free_fn = free;
+	*filter_fn = filter_trees_depth;
+	*filter_free_fn = filter_trees_free;
 	return d;
 }
 
@@ -430,7 +503,7 @@ static filter_init_fn s_filters[] = {
 	NULL,
 	filter_blobs_none__init,
 	filter_blobs_limit__init,
-	filter_trees_none__init,
+	filter_trees_depth__init,
 	filter_sparse_oid__init,
 	filter_sparse_path__init,
 };
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index eb32505a6e..54e7096d40 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -294,6 +294,110 @@ test_expect_success 'filter a GIANT tree through tree:0' '
 	! grep "Skipping contents of tree [^.]" filter_trace
 '
 
+# Test tree:# filters.
+
+expect_has () {
+	commit=$1 &&
+	name=$2 &&
+
+	hash=$(git -C r3 rev-parse $commit:$name) &&
+	grep "^$hash $name$" actual
+}
+
+test_expect_success 'verify tree:1 includes root trees' '
+	git -C r3 rev-list --objects --filter=tree:1 HEAD >actual &&
+
+	# We should get two root directories and two commits.
+	expect_has HEAD "" &&
+	expect_has HEAD~1 ""  &&
+	test_line_count = 4 actual
+'
+
+test_expect_success 'verify tree:2 includes root trees and immediate children' '
+	git -C r3 rev-list --objects --filter=tree:2 HEAD >actual &&
+
+	expect_has HEAD "" &&
+	expect_has HEAD~1 "" &&
+	expect_has HEAD dir1 &&
+	expect_has HEAD pattern &&
+	expect_has HEAD sparse1 &&
+	expect_has HEAD sparse2 &&
+
+	# There are also 2 commit objects
+	test_line_count = 8 actual
+'
+
+test_expect_success 'verify tree:3 includes everything expected' '
+	git -C r3 rev-list --objects --filter=tree:3 HEAD >actual &&
+
+	expect_has HEAD "" &&
+	expect_has HEAD~1 "" &&
+	expect_has HEAD dir1 &&
+	expect_has HEAD dir1/sparse1 &&
+	expect_has HEAD dir1/sparse2 &&
+	expect_has HEAD pattern &&
+	expect_has HEAD sparse1 &&
+	expect_has HEAD sparse2 &&
+
+	# There are also 2 commit objects
+	test_line_count = 10 actual
+'
+
+# Test provisional omit collection logic with a repo that has objects appearing
+# at multiple depths - first deeper than the filter's threshold, then shallow.
+
+test_expect_success 'setup r4' '
+	git init r4 &&
+
+	echo foo > r4/foo &&
+	mkdir r4/subdir &&
+	echo bar > r4/subdir/bar &&
+
+	mkdir r4/filt &&
+	cp -r r4/foo r4/subdir r4/filt &&
+
+	git -C r4 add foo subdir filt &&
+	git -C r4 commit -m "commit msg"
+'
+
+expect_has_with_different_name () {
+	repo=$1 &&
+	name=$2 &&
+
+	hash=$(git -C $repo rev-parse HEAD:$name) &&
+	! grep "^$hash $name$" actual &&
+	grep "^$hash " actual &&
+	! grep "~$hash" actual
+}
+
+test_expect_success 'test tree:# filter provisional omit for blob and tree' '
+	git -C r4 rev-list --objects --filter-print-omitted --filter=tree:2 \
+		HEAD >actual &&
+	expect_has_with_different_name r4 filt/foo &&
+	expect_has_with_different_name r4 filt/subdir
+'
+
+# Test tree:<depth> where a tree is iterated to twice - once where a subentry is
+# too deep to be included, and again where the blob inside it is shallow enough
+# to be included. This makes sure we don't use LOFR_MARK_SEEN incorrectly (we
+# can't use it because a tree can be iterated over again at a lower depth).
+
+test_expect_success 'tree:<depth> where we iterate over tree at two levels' '
+	git init r5 &&
+
+	mkdir -p r5/a/subdir/b &&
+	echo foo > r5/a/subdir/b/foo &&
+
+	mkdir -p r5/subdir/b &&
+	echo foo > r5/subdir/b/foo &&
+
+	git -C r5 add a subdir &&
+	git -C r5 commit -m "commit msg" &&
+
+	git -C r5 rev-list --objects --filter=tree:4 HEAD >actual &&
+	expect_has_with_different_name r5 a/subdir/b/foo
+'
+
 # Delete some loose objects and use rev-list, but WITHOUT any filtering.
 # This models previously omitted objects that we did not receive.
 
-- 
2.20.0.rc2.403.gdbc3b29805-goog


^ permalink raw reply related

* [PATCH v2 2/2] tree:<depth>: skip some trees even when collecting omits
From: Matthew DeVore @ 2018-12-10 23:40 UTC (permalink / raw)
  To: git
  Cc: Matthew DeVore, sbeller, git, jeffhost, peff, stefanbeller,
	jonathantanmy, pclouds
In-Reply-To: <20181210234030.176178-1-matvore@google.com>

If a tree has already been recorded as omitted, we don't need to
traverse it again just to collect its omits. Stop traversing trees a
second time when collecting omits.

Signed-off-by: Matthew DeVore <matvore@google.com>
---
 list-objects-filter.c               | 17 +++++++++++------
 t/t6112-rev-list-filters-objects.sh | 11 ++++++++++-
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/list-objects-filter.c b/list-objects-filter.c
index ca99f0dd02..3e9802c676 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -107,18 +107,18 @@ struct seen_map_entry {
 	size_t depth;
 };
 
-static void filter_trees_update_omits(
+static int filter_trees_update_omits(
 	struct object *obj,
 	struct filter_trees_depth_data *filter_data,
 	int include_it)
 {
 	if (!filter_data->omits)
-		return;
+		return 1;
 
 	if (include_it)
-		oidset_remove(filter_data->omits, &obj->oid);
+		return oidset_remove(filter_data->omits, &obj->oid);
 	else
-		oidset_insert(filter_data->omits, &obj->oid);
+		return oidset_insert(filter_data->omits, &obj->oid);
 }
 
 static enum list_objects_filter_result filter_trees_depth(
@@ -170,12 +170,17 @@ static enum list_objects_filter_result filter_trees_depth(
 		if (already_seen)
 			filter_res = LOFR_SKIP_TREE;
 		else {
+			int been_omitted = filter_trees_update_omits(
+				obj, filter_data, include_it);
 			seen_info->depth = filter_data->current_depth;
-			filter_trees_update_omits(obj, filter_data, include_it);
 
 			if (include_it)
 				filter_res = LOFR_DO_SHOW;
-			else if (filter_data->omits)
+			else if (!been_omitted)
+				/*
+				 * Must update omit information of children
+				 * recursively; they have not been omitted yet.
+				 */
 				filter_res = LOFR_ZERO;
 			else
 				filter_res = LOFR_SKIP_TREE;
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 54e7096d40..18b0b14d5a 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -283,7 +283,7 @@ test_expect_success 'verify tree:0 includes trees in "filtered" output' '
 
 # Make sure tree:0 does not iterate through any trees.
 
-test_expect_success 'filter a GIANT tree through tree:0' '
+test_expect_success 'verify skipping tree iteration when not collecting omits' '
 	GIT_TRACE=1 git -C r3 rev-list \
 		--objects --filter=tree:0 HEAD 2>filter_trace &&
 	grep "Skipping contents of tree [.][.][.]" filter_trace >actual &&
@@ -377,6 +377,15 @@ test_expect_success 'test tree:# filter provisional omit for blob and tree' '
 	expect_has_with_different_name r4 filt/subdir
 '
 
+test_expect_success 'verify skipping tree iteration when collecting omits' '
+	GIT_TRACE=1 git -C r4 rev-list --filter-print-omitted \
+		--objects --filter=tree:0 HEAD 2>filter_trace &&
+	grep "^Skipping contents of tree " filter_trace >actual &&
+
+	echo "Skipping contents of tree subdir/..." >expect &&
+	test_cmp expect actual
+'
+
 # Test tree:<depth> where a tree is iterated to twice - once where a subentry is
 # too deep to be included, and again where the blob inside it is shallow enough
 # to be included. This makes sure we don't use LOFR_MARK_SEEN incorrectly (we
-- 
2.20.0.rc2.403.gdbc3b29805-goog


^ permalink raw reply related

* Re: [PATCH] http: add http.version option to select http protocol version
From: Eric Sunshine @ 2018-12-10 23:47 UTC (permalink / raw)
  To: silvio.fricke; +Cc: Git List
In-Reply-To: <98295da2b5295795414eaf85a40b9ae62b1b2dca.1544482124.git.silvio.fricke@gmail.com>

On Mon, Dec 10, 2018 at 5:50 PM Silvio Fricke <silvio.fricke@gmail.com> wrote:
> HTTP has several protocol versions. By default, libcurl is using HTTP/2
> today and check if the remote can use this protocol variant and fall
> back to a previous version if not.
>
> Under rare conditions it is needed to switch the used protocol version
> to fight again wrongly implemented authorization mechanism like ntlm
> with gssapi on remote side.
>
> Signed-off-by: Silvio Fricke <silvio.fricke@gmail.com>

This looks very similar to [1] which is already in Junio's "next"
branch (although not yet in a released version of Git).

[1]: https://public-inbox.org/git/71f8b71b346f132d0dc9a23c9a7f2ca2cb91966f.1541735051.git.gitgitgadget@gmail.com/

^ permalink raw reply

* [PATCH] Re: [wishlist] submodule.update config
From: Stefan Beller @ 2018-12-11  0:08 UTC (permalink / raw)
  To: yoh; +Cc: git, Stefan Beller
In-Reply-To: <20181210224901.GL4633@hopa.kiewit.dartmouth.edu>

Signed-off-by: Stefan Beller <sbeller@google.com>
---

> > So you are proposing a variable like submodule.update
> [...]
>
> Glad to hear that. Not sure though I would know where to stick my
> nose to figure out what to change. ;-)

The update_module is computed via the submodule--helpers
update-module-mode command, which is a small wrapper
around determine_submodule_update_strategy()
which you already touched in the other patch that makes
--reset-hard another mode.

This contains code and tests, but we'd need some docs as well.
I am not sure about this patch as it allows for easier experimentation
with submodules (e.g. "git config submodule.update '!git reset --hard'"
sounds like what you're trying to get) and using them, but as discussed
below this might be too much convenience already and we'd rather want to
have it properly integrated into the real commands.

> Well, not sure... In the long run, if UX is to be tuned up, I wonder if
> it would be more worthwhile to look toward making all those git commands
> (git merge, git checkout, git rebase, ..., git revert, git cherry-pick)
> support --recurse-submodules with a consistent with the non-recursive
> operation by default behavior

That is the end goal, very much.

> (e.g.  not introducing detached HEADs or
> controlling that via a set of additional options where needed).

As with the discussion of the submodule.repoLike option (the patch I
referenced in the other discussion), this is tricky to get the right
behavior, so it takes some more time to do.

Also what is right for a "git merge --recursive" might be totally different
from a "git submodule update --merge" as the former is not centered around
submodules but merging, such that a sensible default would be expected,
whereas the "submodule update" is allowed to have a rough edge.

From what I get from this discussion is that the submodule.repoLike patch 
needs to offer different modes of submodule operation.

Currently the submodules are handled with the "detached HEAD, period" mode,
whereas that patch proposes a "follow the submodule branch, trust me they're
in sync with the superproject magically" mode, but what you'd rather want to
see is a "don't mess with submodules HEAD detachments, but still have
superproject powers come to be".

As soon as we have one of these modes in place, adding another one
"should be easy", famous last words.

Stefan

 builtin/submodule--helper.c |  4 ++++
 t/t7406-submodule-update.sh | 41 +++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d38113a31a..e1aa3a9995 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1472,6 +1472,10 @@ static void determine_submodule_update_strategy(struct repository *r,
 		if (parse_submodule_update_strategy(val, out) < 0)
 			die(_("Invalid update mode '%s' configured for submodule path '%s'"),
 				val, path);
+	} else if (!repo_config_get_string_const(r, "submodule.update", &val)) {
+		if (parse_submodule_update_strategy(val, out) < 0)
+			die(_("Invalid update mode '%s' configured for 'submodule.update'"),
+				val);
 	} else if (sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
 		out->type = sub->update_strategy.type;
 		out->command = sub->update_strategy.command;
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e87164aa8f..05880fd48f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -322,6 +322,33 @@ test_expect_success 'submodule update - rebase in .git/config' '
 	)
 '
 
+test_expect_success 'submodule update - rebase in generic .git/config' '
+	git -C super config submodule.update rebase &&
+	git -C super/submodule reset --hard HEAD~1 &&
+	(cd super &&
+	 (cd submodule &&
+	  compare_head
+	 ) &&
+	 git submodule update submodule &&
+	 cd submodule &&
+	 compare_head
+	)
+'
+
+test_expect_success 'submodule.<name>.update overrides submodule.update' '
+	git -C super config submodule.update merge &&
+	git -C super config submodule.submodule.update rebase &&
+	git -C super/submodule reset --hard HEAD~1 &&
+	(cd super &&
+	 (cd submodule &&
+	  compare_head
+	 ) &&
+	 git submodule update submodule &&
+	 cd submodule &&
+	 compare_head
+	)
+'
+
 test_expect_success 'submodule update - checkout in .git/config but --rebase given' '
 	(cd super &&
 	 git config submodule.submodule.update checkout
@@ -339,6 +366,20 @@ test_expect_success 'submodule update - checkout in .git/config but --rebase giv
 	)
 '
 
+test_expect_success 'submodule update - checkout in submodule.update in .git/config but --rebase given' '
+	test_when_finished "git -C super config --unset submodule.update" &&
+	git -C super config submodule.update checkout &&
+	git -C super/submodule reset --hard HEAD~1 &&
+	(cd super &&
+	 (cd submodule &&
+	  compare_head
+	 ) &&
+	 git submodule update --rebase submodule &&
+	 cd submodule &&
+	 compare_head
+	)
+'
+
 test_expect_success 'submodule update - merge in .git/config' '
 	(cd super &&
 	 git config submodule.submodule.update merge
-- 
2.20.0.rc2.403.gdbc3b29805-goog


^ permalink raw reply related

* [PATCH] gitk: Use themable foundbgcolor also in headlines and authors
From: Guillermo S. Romero @ 2018-12-11  0:14 UTC (permalink / raw)
  To: git; +Cc: paulus

Signed-off-by: Guillermo S. Romero <gsromero@infernal-iceberg.com>
---

Hi:

This removes the hardcoded color and uses the user config (still
yellow by default) so searches share a common look.

GSR
 

PS: Not subscribed to git@vger, so remember to CC: me.


 gitk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gitk b/gitk
index a14d7a1..8837e46 100755
--- a/gitk
+++ b/gitk
@@ -6952,7 +6952,7 @@ proc findselectline {l} {
 
 # mark the bits of a headline or author that match a find string
 proc markmatches {canv l str tag matches font row} {
-    global selectedline
+    global selectedline foundbgcolor
 
     set bbox [$canv bbox $tag]
     set x0 [lindex $bbox 0]
@@ -6966,7 +6966,7 @@ proc markmatches {canv l str tag matches font row} {
 	set xlen [font measure $font [string range $str 0 [expr {$end}]]]
 	set t [$canv create rect [expr {$x0+$xoff}] $y0 \
 		   [expr {$x0+$xlen+2}] $y1 \
-		   -outline {} -tags [list match$l matches] -fill yellow]
+		   -outline {} -tags [list match$l matches] -fill $foundbgcolor]
 	$canv lower $t
 	if {$row == $selectedline} {
 	    $canv raise $t secsel
-- 
2.20.0.rc2


^ permalink raw reply related

* RE: Questions about ubifs,ubi and mtd?
From: 武井 克明 @ 2018-12-11  0:42 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git@vger.kernel.org
In-Reply-To: <87o99txwvp.fsf@evledraar.gmail.com>

Dear Ævar Arnfjörð Bjarmason,

 Thank you for your advice.
 I will ask my question to ML who told me.

Best regards,
Katsuaki Takei/Oki Electric Industry Co., Ltd./JP

> -----Original Message-----
> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Sent: Monday, December 10, 2018 11:47 PM
> To: 武井 克明 <takei744@oki.com>
> Cc: git@vger.kernel.org
> Subject: Re: Questions about ubifs,ubi and mtd?
> 
> 
> On Mon, Dec 10 2018, 武井 克明 wrote:
> 
> > Hi all,
> >
> > We are developing the product using file system using ubi, ubifs on hardware
> with NAND flash memory.
> >
> > Although the development of the product was completed, we are seeking
> your help as the customer who is using it is having trouble with it.
> >
> > The product we have developed has 6 MTD partitions for NAND flash
> memory.
> > These consist of 6 MTDs named 'kernel-a' 'kernel-b', 'rootfs-a', 'rootfs-b',
> 'data-1', 'data-2', and online program only accesses 'data-1' and 'data-2' for
> write access.
> > Nevertheless, when loading our program from 'rootfs-a', trying to read
> > the inode with the ubifs_read_node() function will result in "bad node
> > type" (ex: 193 but expected 9) and the LEB can not be read with the
> > expected value. (Even though you do not have write access to rootfs)
> >
> > Is there anyone who encountered such a problem? Is there patch?
> >
> > Please let me know, if you have any questions.
> >
> > Best regards,
> > Katsuaki Takei/Oki Electric Industry Co., Ltd./JP
> 
> Hi. I think you're on the wrong mailing list (git@). You probably want to contact
> one of the linux lists, perhaps
> http://lists.infradead.org/mailman/listinfo/linux-mtd ?

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox