Bug#894997: [PATCH] git-svn: avoid warning on undef readline()

[Junio C Hamano]

Eric Wong  writes:

> Ævar Arnfjörð Bjarmason  wrote:
>> See https://public-inbox.org/git/86h8oobl36@phe.ftfl.ca/ for the
>> original report.
>
> Thanks for taking a look at this.  Also https://bugs.debian.org/894997
>
>> --- a/perl/Git.pm
>> +++ b/perl/Git.pm
>> @@ -554,7 +554,7 @@ sub get_record {
>>  my ($fh, $rs) = @_;
>>  local $/ = $rs;
>>  my $rec = <$fh>;
>> -chomp $rec if defined $rs;
>> +chomp $rec if defined $rs and defined $rec;
>
> I'm struggling to understand the reason for the "defined $rs"
> check.  I think it was a braino on my part and meant to use:
>
>   chomp $rec if defined $rec;

That sounds quite plausible, so if there is no further discussion,
let me queue a version that does s/rs/rec/ on that line myself
(bypassing a pull from your repository at bogomips.org/).

Thanks.

Bug#842477: [PATCH] git-sh-setup: Restore sourcability from outside scripts

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

(commenting out of order)

> It's probably worthwhile to split off git-sh-setup into git-sh-setup &
> git-sh-setup-internal along with a documentation fix. A lot of what
> it's doing (e.g. git_broken_path_fix(), and adding a die() function)
> is probably only needed internally by git itself. The
> git-sh-setup-internal should be the thing sourcing "git-sh-i18n", I
> don't see how anyone out-of-tree could make use of that. Surely nobody
> needs to re-emit the exact message we shipped with our *.po files.

My reading of d323c6b641 ("i18n: git-sh-setup.sh: mark strings for
translation", 2016-06-17) is abit different.  It needs to dot-source
the i18n stuff because the shell library functions it contains need
the localization support in the messages they emit.  IOW, I do not
think i18n belongs to -internal at all.

> I don't see why we shouldn't have some stable shellscript function API
> if that's needed either.
>
> I just wanted to point out that currently git-sh-setup isn't
> documented as such. So at least a follow-up patch to the documentation
> seems in order.
>
> This did break in v2.10.0, and it's taken a couple of months to notice
> this, so clearly it's not very widely used, which says something about
> the cost-benefit of maintaining this for external users.

I am not sure if "stable API" in sh-setup is a good thing for the
ecosystem in the longer term.

As more and more in-tree scripted Porcelain commands migrate to C,
many helper functions in sh-setup will lose their in-tree users.
For example, get_author_ident_from_commit used to have three in-tree
customers (git-commit.sh, git-am.sh and git-rebase--interactive.sh),
but the first two is long gone and the third one may soon lose its
need to call it.  Once a helper function in setup-sh loses all
in-tree users, we may no longer _break_ that helper, but that is
simply because we feel no need to touch it.  The in-tree Porcelain
commands that migrated to C however will enhance the counterpart
they use in C to be more featureful or fix longstanding bugs in the
C version they use, while sh-setup version bitrot and making old
practice obsolete for "modern" use of Git of the day.  

Keeping such a stale version that we do not use, or even we attempt
to update it without having a good vehicle to test the change
ourselves (because we no longer have any in-tree users) will be
disservice to third-party scripts---the only thing they are getting
by using the stale one, instead of reinventing their own that they
may be responsible to keep up to date, is that they share the same
staleness as everybody else that use the sh-setup version as a
third-party.

I am not arguing that we should remove what loses all in-tree users
immediately.  At least not yet.  But I wanted to point out that it
may not be a good use of our brain cycles to keep the API "stable"
by keeping what in-tree users do not use anymore, especially if it
does not help third-party users in the long run.

Bug#842477: [PATCH] git-sh-setup: Restore sourcability from outside scripts

[Junio C Hamano]

Anders Kaseorg  writes:

> v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for
> translation” broke outside scripts such as guilt that source
> git-sh-setup as described in the documentation:
>
> $ . "$(git --exec-path)/git-sh-setup"
> sh: 6: .: git-sh-i18n: not found
>
> This also affects contrib/convert-grafts-to-replace-refs.sh and
> contrib/rerere-train.sh in tree.  Fix this by using git --exec-path to
> find git-sh-i18n.
>
> While we’re here, move the sourcing of git-sh-i18n below the shell
> portability fixes.
>
> Signed-off-by: Anders Kaseorg 
> ---

Looks good.

Our in-tree scripts rely on the fact that $PATH is adjusted to have
$GIT_EXEC_PATH early (either by getting invoked indirectly by "git"
potty, or the requirement to do so for people and scripts that still
run our in-tree scripts with dashed e.g. "git-rebase" form) by the
time they run.  But when sh-setup dot-sources git-sh-i18n for its
own use, it should be explicit to name which one of the many copies
that may appear in directories on user's $PATH (one among which is
the one in $GIT_EXEC_PATH) it wants to use.  And this patch does the
right thing by not relying on the $PATH, but instead naming the
exact path using $(git --exec-path)/ prefix, to the included file.

In other words, I think this patch is a pure bugfix, even if there
is no third-party script that includes it.  We may want to have the
above as the rationale to apply this patch in the proposed log
message, though.

> Is this a supported use of git-sh-setup?  Although the documentation is
> clear that the end user should not invoke it directly, it seems to imply
> that scripts may do this, and in practice it has worked until v2.10.0.

It is correct for the documentation to say that this is not a
"command" end users would want to run; they cannot invoke it as a
standalone command as it is written as a dot-sourced shell library.

Even though it is intended solely for internal use, so far we have
not removed things from there, which would have signalled people
that third-party scripts can also dot-source it.  We may want to
reserve the right to break them in the future, but because this is a
pure bugfix, "can third-party rely on the interface not changing?"
is not a question we need to answer in this thread---there is no
reason to leave this broken.

Thanks.

>  git-sh-setup.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index a8a4576..240c7eb 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -2,9 +2,6 @@
>  # to set up some variables pointing at the normal git directories and
>  # a few helper shell functions.
>  
> -# Source git-sh-i18n for gettext support.
> -. git-sh-i18n
> -
>  # Having this variable in your environment would break scripts because
>  # you would cause "cd" to be taken to unexpected places.  If you
>  # like CDPATH, define it for your interactive shell sessions without
> @@ -46,6 +43,9 @@ git_broken_path_fix () {
>  
>  # @@BROKEN_PATH_FIX@@
>  
> +# Source git-sh-i18n for gettext support.
> +. "$(git --exec-path)/git-sh-i18n"
> +
>  die () {
>   die_with_status 1 "$@"
>  }

Bug#434599: [PATCH] git-imap-send: use libcurl for implementation

[Junio C Hamano]

Bernhard Reiter ock...@raz.or.at writes:

 Resending this once more, as indicated by 
 xmqqbnp4hu8g@gitster.dls.corp.google.com
 Hope my formatting and posting style is now conformant. Sorry for the noise.

Thanks.

The patch does not apply for me (please send a trial message to
yourself and then try to apply it out of your mailbox to avoid such
an incident in the future), unfortunately.  I'll comment on the
patch from just code inspection without applying it, though.

 diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
 index 7d991d9..1c24e1f 100644
 --- a/Documentation/git-imap-send.txt
 +++ b/Documentation/git-imap-send.txt
 @@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an 
 IMAP folder
  SYNOPSIS
  
  [verse]
 -'git imap-send'
 +'git imap-send' [-v] [-q] --[no-]curl
DESCRIPTION
 @@ ...

The hunk header says that the original and the updated both starts
at line #9 and they both should have 7 lines, but I count only 5
lines.  Where did you lose two lines of the post-context?

 diff --git a/Makefile b/Makefile
 index f34a2d4..69b2fbf 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -992,6 +992,9 @@ ifdef HAVE_ALLOCA_H
   BASIC_CFLAGS += -DHAVE_ALLOCA_H
  endif
  +IMAP_SEND_BUILDDEPS =
 +IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
 +

Here the patch looks funny as well.  I guess IMAP_SEND_BUILDDEPS =
is an added line prefixed with +, but where does the SP before the
plus sign come from?

I won't point out other patch breakages in the remainder of this
message.

 diff --git a/imap-send.c b/imap-send.c
 index 70bcc7a..9cc80ae 100644
 --- a/imap-send.c
 +++ b/imap-send.c
 @@ -26,11 +26,31 @@
  #include credential.h
  #include exec_cmd.h
  #include run-command.h
 +#include parse-options.h
  #ifdef NO_OPENSSL
  typedef void *SSL;
  #endif
 +#ifdef USE_CURL_FOR_IMAP_SEND
 +#include http.h
 +#endif
  -static const char imap_send_usage[] = git imap-send  mbox;
 +static int Verbose, Quiet;

Yikes.  I know this is not a new problem, but please do not name
variables Capitalized.  Also what would it mean to set both Verbose
and Quiet at the same time?

I think we would want to use OPT__VERBOSITY with a single variable
verbosity (see hits from git grep OPT__VERBOSITY for examples).
Such a change should probably come before this change to use libcurl
as a separate preparatory clean-up patch.

 +#ifdef USE_CURL_FOR_IMAP_SEND
 +static int use_curl = 1; // on by default; set later by parse_options
 +#else
 +static int use_curl = 0; // not available
 +#endif

Please don't use // comments.  Besides, I think this should be
initialized to -1 to mean unspecified by the user in both cases
(i.e. no need for #ifdef/#endif).

 +static struct option imap_send_options[] = {
 +#ifdef USE_CURL_FOR_IMAP_SEND
 + OPT_BOOL(0, curl, use_curl, use libcurl to communicate with the 
 IMAP server),
 +#endif

And lose the ifdef here (i.e. take --use-curl even on a build that
does not support it).

 +int main(int argc, char **argv)
 +{
 + struct strbuf all_msgs = STRBUF_INIT;
 + int total;
   int nongit_ok;
   git_extract_argv0_path(argv[0]);
  -git_setup_gettext();
 + argc = parse_options(argc, (const char **)argv, , imap_send_options, 
 imap_send_usage, 0);
  -if (argc != 1)
 - usage(imap_send_usage);
 + git_setup_gettext();
   setup_git_directory_gently(nongit_ok);
   git_imap_config();

Usually we read config and then parse options, so that people can
set configuration variables for their usual use pattern and override
what is configured from the command line option as needed.

For example, you may want to add

[imap]
useCurl = true

in the configuration file and run git imap-send without --curl
option on the command line almost always, but in some specific
occasion, git imap-send --no-curl to countermand the
configuration.

Was there a specific reason why you had to add parse_options()
before git_imap_config()?

With use_curl initialized to -1 (unspecified), the final code
structure may look more like this:

...
git_imap_config();
parse_options();

#ifndef USE_CURL_FOR_IMAP_SEND
if (use_curl  0)
use_curl = 0;
if (use_curl) {
warning(--use-curl not supported in this build);
use_curl = 0;
}
#else
if (use_curl  0) /* default to enable libcurl */
use_curl = 1;
#endif

although I think it is also OK to make this feature strictly optional
by defaulting to use_curl = 0 in the #else part above.  Initializing
the static variable to 0 would make the result even shorter if we
were to go that route, i.e.

static int use_curl; /* strictly opt in */
...
main()
{
...

Bug#434599: [PATCH/RFC] git-imap-send: use libcurl for implementation

[Junio C Hamano]

Bernhard Reiter ock...@raz.or.at writes:

 Am 2014-08-17 um 20:42 schrieb Jeff King:
 [...]
 
 I'm not sure I understand this comment. Even if SSL is not in use,
 wouldn't we be passing a regular pipe to curl, which would break?

 Yeah, we can't do that, and thus would have to keep the handwritten IMAP
 implementation just for the tunnel case (allowing to drop only the
 OpenSSL specific stuff), see my other email:
 http://www.mail-archive.com/git@vger.kernel.org/msg56791.html (the
 relevant part is pretty far down at the bottom).
 
 I'd really love it if we could make this work with tunnels and
 eventually get rid of the hand-written imap code entirely. I agree with
 Jonathan that we probably need to keep it around a bit for people on
 older curl, but dropping it is a good goal in the long run. That code
 was forked from the isync project, but mangled enough that we could not
 take bug fixes from upstream. As not many people use imap-send, I
 suspect it is largely unmaintained and the source of many lurking
 bugs[1]. Replacing it with curl's maintained implementation is probably
 a good step.

I would agree with s/a good step/a good goal/ ;-)

 I'll work on this as soon as I find some time, but as that will include
 changes to run-command.c (and possibly other files?), I'd like to cover
 that in a commit of its own. Do you guys think the current patch [1] is
 good enough for official submission already?

My impression from reading the discussion in this thread has been
that the patch that started this thread would break the tunneling
code, i.e. is not there yet.  Or did you mean some other patch?

The other patch $gmane/255403 from you looked good and I think I
already have a copy queued on 'pu' as f9dc5d65 (git-imap-send:
simplify tunnel construction, 2014-08-13).

Thanks.


[References]

*$gmane/255403*
http://thread.gmane.org/gmane.comp.version-control.git/255220/focus=255403


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#757297: 'git status' output is confusing after 'git add -N'

[Junio C Hamano]

Duy Nguyen pclo...@gmail.com writes:

 On Thu, Aug 7, 2014 at 7:34 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 Package: git
 Version: 1:2.0.0-1
 Tags: upstream

   $ git init foo
   Initialized empty Git repository in /tmp/t/foo/.git/
   $ cd foo
   $ echo hi README
   $ git add -N README
   $ git status
   On branch master

   Initial commit

   Changes to be committed:
 (use git rm --cached file... to unstage)

   new file:   README

   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:   README

 If I then run git commit, it does not actually commit the addition
 of the README file.

 We used to reject such a commit operation before 3f6d56d (commit:
 ignore intent-to-add entries instead of refusing - 2012-02-07) so it
 was harder to misunderstand this case.

 It would be clearer to have a separate section,like so:

   Tracked files not to be committed:
 (use git rm --cached file... to stop tracking)

new file:   README


 Or make the Changes not staged for commit part say new file:
 README (modified is implied)

Yeah, after reading the justification in the quoted commit, I agree
that it is status that is at fault in the above; new file: README
is part of Changes not staged for commit in this case (it is told
to the index, but the user never said it is for commit yet, which
is the whole point of -N), so instead of adding a new section, I
agree that it should be classified as new file not modified
there.


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#727226: [PATCH] cvsserver: Determinize output to combat Perl 5.18 hash randomization

[Junio C Hamano]

Anders Kaseorg ande...@mit.edu writes:

 Perl 5.18 randomizes the seed used by its hash function, so iterating
 through hashes results in different orders from run to run:
   http://perldoc.perl.org/perl5180delta.html#Hash-overhaul

 This usually broke t9400 (gitcvs.dbname, gitcvs.ext.dbname, when
 running cmp on two .sqlite files) and t9402 (check [cvswork3] diff,
 when running test_cmp on two diffs).

 To fix this, hide the internal order of hashes with sort when sending
 output or running database queries.

 (An alternative workaround is PERL_HASH_SEED=0, but this seems nicer.)

 Signed-off-by: Anders Kaseorg ande...@mit.edu

Thanks, will queue.

 ---
  git-cvsserver.perl | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

 diff --git a/git-cvsserver.perl b/git-cvsserver.perl
 index 67b1e7b..6177f4a 100755
 --- a/git-cvsserver.perl
 +++ b/git-cvsserver.perl
 @@ -430,10 +430,10 @@ sub req_validrequests
  
  $log-debug(req_validrequests);
  
 -$log-debug(SEND : Valid-requests  . join( ,keys %$methods));
 +$log-debug(SEND : Valid-requests  . join( ,sort keys %$methods));
  $log-debug(SEND : ok);
  
 -print Valid-requests  . join( ,keys %$methods) . \n;
 +print Valid-requests  . join( ,sort keys %$methods) . \n;
  print ok\n;
  }
  
 @@ -2124,7 +2124,7 @@ sub req_diff
  print M retrieving revision $meta2-{revision}\n
  }
  print M diff ;
 -foreach my $opt ( keys %{$state-{opt}} )
 +foreach my $opt ( sort keys %{$state-{opt}} )
  {
  if ( ref $state-{opt}{$opt} eq ARRAY )
  {
 @@ -4050,7 +4050,7 @@ sub update
  close FILELIST;
  
  # Detect deleted files
 -foreach my $file ( keys %$head )
 +foreach my $file ( sort keys %$head )
  {
  unless ( exists $seen_files-{$file} or 
 $head-{$file}{filehash} eq deleted )
  {
 @@ -4078,7 +4078,7 @@ sub update
  }
  
  $self-delete_head();
 -foreach my $file ( keys %$head )
 +foreach my $file ( sort keys %$head )
  {
  $self-insert_head(
  $file,


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#666250: [PATCH] Documentation: replace 'vi' for 'editor' to reflect build-time option

[Junio C Hamano]

Rodrigo Silva li...@rodrigosilva.com writes:

 At 23:16 29/3/2012, Junio C Hamano wrote:
 ...
I've already rejected this patch once, but that was primarily because the
patch was not justified with the above I read everybody else's git uses
'vi' on the Interweb, and even though my distro's manual page says it uses
'nano', I didn't bother to read it. scenario.

 @Jonathan: I'm really sorry for this confusion...
 ... Junio suggested me to send this patch to
 Debian, that's why I filled the bug in Debian BTS. It was meant to be a
 Debian-only patch.

It is a Debian package bug that the git_*.diff patch they use to customize
the upstream source ends up making a resulting binary to use different
editor and pager but does so without editing Documentation/ files to match
the description to what their executable does.  The resulting package is
not self consistent.

But we aren't making it particularly easy for Debian to do so.

The debian/rules (the Makefile that invokes our Makefile) file in the
git_*.diff patch merely overrides DEFAULT_EDITOR and DEFAULT_PAGER,
instead of patching editor.c and pager.c to replace the fallback hardcoded
defaults vi and less.  But these Makefile variables only affect that
goes in the executable, and they do not affect the documentation.

So in the shorter term, I think Jonathan's patch would be OK for me to
carry in my tree, but even in that case I think Debian still needs to
replace the vi/less in the documentation to tell what they chosen for
their users.

In the longer term, it may make more sense to let these Makefile variables
replace vi and less with whatever values set to them, while keeping
the default chosen at the build time, usually wording.

In any case, I'd appreciate it if either of you can turn Jonathan's How
about something like this? into the final version to be fed to git am.

Thanks.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#575917: [long] worktree setup cases

[Junio C Hamano]

Nguyen Thai Ngoc Duy pclo...@gmail.com writes:

 Assumptions:

 1. When .git is a file and contains a relative path, I assume it is
relative to .git file's parent directory.  read_gitfile_gently()
function will make the path absolute, but it depends on (maybe
wrong) cwd.

Ok.  I agree that a regular file .git that has gitdir: ../there in it
should be naming there directory next to the directory .git lives in.
It is insane for it to take $(cwd) into account.

 There are a few factors that affect gitdir/worktree/cwd/prefix setup.
 Those are:

  - GIT_DIR (or --git-dir)
  - GIT_WORK_TREE (or --work-tree)
  - core.worktree
  - .git is a file pointing to real .git directory
  - Bare repo

 So there are 2^5 = 32 cases in total.

Hmm, why is core.worktree separate from the second item?  In any case, the
three mechanisms to specify the working tree should only be in effect when
GIT_DIR/--git-dir is used, so the above are not orthogonal, and the total
count should be smaller than 32 cases.

 The following questions must be answered for each case:

 0. What setup function handles that case?
 1. Is worktree available?
 2. Where is new cwd? Is it at worktree root?
 3. Prefix?
 4. Is (relative) $GIT_DIR accessible from current cwd?
 5. What if original cwd is outside worktree, or new cwd if it's not at
worktree root?

All good questions to ask, except for the last one which I cannot quite
parse.

 Table of Contents
 =
 1 Cases
 1.1 Case 0, 8
 1.2 Case 1, 4, 5, 9, 12, 13
 1.2.1 cwd outside worktree
 1.3 Case 2, 10
 1.4 Case 3, 6, 7, 11, 14, 15
 1.4.1 cwd outside worktree
 1.5 Case 16, 24
 1.6 Case 17, 20, 21, 25, 28, 29
 1.6.1 cwd outside worktree
 1.7 Case 18, 26
 1.8 Case 19, 22, 23, 27, 30, 31


 1 Cases
 

 Case#  Bare  .git-file  core.worktree  GIT_DIR  GIT_WORK_TREE
 0  0 0  0  00
 8  0 1  0  00

Ok.

 1  0 0  0  01
 4  0 0  1  00
 5  0 0  1  01
 9  0 1  0  01
 12 0 1  1  00
 13 0 1  1  01

How does it make sense to have GIT_WORK_TREE without GIT_DIR?  Without
GIT_DIR, we will run repository discovery, which means we will always go
up to find a .git dir, and the reason for doing that is because we want to
also work in a subdirectory of a working tree (the very original git never
worked from anywhere other than the root level of the working tree).  By
definition, the root of the working tree is the same as in cases 0/8.

 2  0 0  0  10
 10 0 1  0  10

If you set GIT_DIR, we do no discovery, so git will work only from the
root level of the working tree (or bare repository operation) if you do
not tell us where the working tree is.

 3  0 0  0  11
 6  0 0  1  10
 7  0 0  1  11
 11 0 1  0  11
 14 0 1  1  10
 15 0 1  1  11

Without discovery, we know where the root level of the working tree is,
and we know where the repository is, because you told us.  The answers to
questions 1-5, i.e. semantics observable by the end user, should be the
same as case 0/8 even though the internal codepath to achieve that,
i.e. question 0, may be different.

 16 1 0  0  00
 ...
 31 1 1  1  11

Shouldn't all of these 16 be the same, if the repository is bare?  What is
your definition of bareness?  core.bare?  In any case we should say you
are using a bare repository, there is no working tree and cwd shouldn't
change in these cases.  They are all bare and there is no working tree.

Alternatively, you could give precedence to core.worktree and friends, in
which case these can go to the other categories in your description,
ignoring core.bare settings.  For example, 31 explicitly specifies where
the .git directory and the working tree are, which would fall into the
same category as 3,6,7,11,14,15.

Either way is fine.

 1.1 Case 0, 8
 ==

 The most used case, nothing special is set.

 0. setup_discovered_git_dir()
 1. work tree is .git dir's parent directory
 2. cwd is at worktree root, i.e. .git dir's parent dir
 3. prefix is calculated from original cwd
 4. git_dir is set to .git (#0) or according to .git file, made
absolute (#8)
 5. N/A

 TODO: turn git_dir to relative in #8

Ok.

 1.2 Case 1, 4, 5, 9, 12, 13

As I said already, isn't this a nonsense combination you should error out?

 1.3 Case 2, 10
 ===

 0. setup_explicit_git_dir()
 1. worktree is set at cwd for legacy reason
 

Bug#554682: [PATCH 2/8] bundle: use libified rev-list --boundary

[Junio C Hamano]

Jonathan Nieder jrnie...@gmail.com writes:

 The revision walker produces structured output, which should be a
 little easier to work with than the text from rev-list.

Hmm, doesn't it negatively affect later traversal you would need to do if
you smudged the flag bits by running revision traversal like this?



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#554682: [PATCH 3/8] bundle: give list_prerequisites() loop body its own function

[Junio C Hamano]

Jonathan Nieder jrnie...@gmail.com writes:

 No functional change intended.

 Signed-off-by: Jonathan Nieder jrnie...@gmail.com
 ---
  bundle.c |   57 +++--
  1 files changed, 31 insertions(+), 26 deletions(-)

 diff --git a/bundle.c b/bundle.c
 index 0dd2acb..e90b5c5 100644
 --- a/bundle.c
 +++ b/bundle.c
 @@ -193,6 +193,33 @@ static int is_tag_in_date_range(struct object *tag, 
 struct rev_info *revs)
   (revs-min_age == -1 || revs-min_age  date);
  }
  
 +static void list_prerequisite(int bundle_fd, struct rev_info *revs,
 + struct commit *rev)
 +{
 + struct strbuf buf = STRBUF_INIT;
 + struct pretty_print_context ctx = {0};
 + enum object_type type;
 + unsigned long size;
 +
 + /*
 +  * The commit buffer is needed
 +  * to pretty-print boundary commits.
 +  */
 + rev-buffer = read_sha1_file(rev-object.sha1, type, size);
 +
 + strbuf_addch(buf, '-');
 + strbuf_add(buf, sha1_to_hex(rev-object.sha1), 40);
 + strbuf_addch(buf, ' ');
 + pretty_print_commit(CMIT_FMT_ONELINE, rev, buf, ctx);
 + strbuf_addch(buf, '\n');
 +
 + write_or_die(bundle_fd, buf.buf, buf.len);
 +
 + rev-object.flags |= UNINTERESTING;
 + add_pending_object(revs, rev-object, buf.buf);
 + strbuf_release(buf);
 +}
 +
  static int list_prerequisites(int bundle_fd, struct rev_info *revs,
   int argc, const char * const *argv)
  {
 @@ -209,33 +236,11 @@ static int list_prerequisites(int bundle_fd, struct 
 rev_info *revs,
   if (prepare_revision_walk(boundary_revs))
   return error(revision walk setup failed);
  
 - while ((rev = get_revision(boundary_revs))) {
 - if (rev-object.flags  BOUNDARY) {
 -...
 - } else {
 + while ((rev = get_revision(revs))) {
 + if (rev-object.flags  BOUNDARY)
 + list_prerequisite(bundle_fd, revs, rev);
 + else
   rev-object.flags |= SHOWN;
 - }

You used to walk boundary_revs but now you walk revs that is given by the
caller, exhausting the revs.pending the caller wanted to use later to feed
pack_objects with?

Confused...



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#578764: 'commit -a' safety

[Junio C Hamano]

Petr Baudis pa...@suse.cz writes:

 On Sat, Apr 24, 2010 at 01:10:24PM +0200, Wincent Colaiuta wrote:
 El 24/04/2010, a las 11:40, Jakub Narebski escribió:
  I'd like for 'git commit -a' to *fail* if there are staged changes for
  tracked files, excluding added, removed and renamed files.

 Thanks for this suggestion, this is exactly what I wanted to propose!

I am somewhat torn.

I have made mistake of running commit -a after I spent time sifting my
changes in the work tree.  I can see that I would have been helped by it
if the safety were there.

But at the same time, I also know that my development is often a cycle of
change then diff then add (to mark the part I am happy with), and when I
am happy with the output from diff, I conclude it with commit -a to
conclude the whole thing.  I can see that I would be irritated to if that
final step failed.

But I suspect the irritation would be relatively mild: ah, these days I
shouldn't use 'commig -a' to conclude these incremental change-review-add
cycle; instead, I should say 'add -u' then 'commit'.




--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#578764: Please default to 'commit -a' when no changes were added

[Junio C Hamano]

Goswin von Brederlow goswin-...@web.de writes:

 But you are already rejecting it in the design phase before there even
 is a patch.

We do review both the design and the implementation on this list, and it
actually is a *good* thing if a proposal is rejected when its design is
flawed at the conceptual level.

A perfect implementation of an undesirable design is just as undesirable
as a buggy implementation of the same design.  It is a change that we do
not want to have in the system.

As to this particular proposed change to commit everything only when there
is no change between the index and the HEAD, I think it is a bad change.
As several people have already said, it adds unnecessary complexity to the
end user's mental model.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#578764: Please default to 'commit -a' when no changes were added

[Junio C Hamano]

Goswin von Brederlow goswin-...@web.de writes:

 Exact.  It is therefore not progress to impose some inconvenience to one 
 work flow in order to make another one easier.  And in this case we're 
 talking about the difference between having to type an additional -a vs 
 the risk of creating a commit with unexpected content.

 Is there a risk?

Absolutely.

Think of this sequence:

... edit edit edit to enhance the program a lot
... oops, noticed that there is a small typo in hello.c
... fix and do git add hello.c (at least I thought I did)
... edit edit edit to enhance the program a lot more.
... it is a good time to get rid of the trivial fix first.
$ git commit -m 'hello.c: typofix the message'
... oops, I mistyped the earlier one as git ad hello.c and
... didn't notice it.

which is just an example.

And the problem is a lot bigger at the _conceptual_ level.

We promise to the user that git commit without paths (nor -a which is
merely a short hand to specify all the paths that have changed) to commit
only what has been added to the index.  If you earlier did git add foo
and git add bar, changes made to these two files are the only changes
that are committed.  If you did only git add foo, then changes made to
this one file are the only changes that are committed.  If you haven't
added anything yet, there is no change to be committed.

Special casing the last case (and only the last case) breaks consistency a
big way.  It is one more pitfall that users need to worry about.




-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#462557: [PATCH] Let 'git command -h' show usage without a git dir

[Junio C Hamano]

Jonathan Nieder jrnie...@gmail.com writes:

 diff --git a/git.c b/git.c
 index bd2c5fe..bfa9518 100644
 --- a/git.c
 +++ b/git.c
 @@ -220,6 +220,11 @@ const char git_version_string[] = GIT_VERSION;
   * RUN_SETUP for reading from the configuration file.
   */
  #define NEED_WORK_TREE   (12)
 +/*
 + * Let RUN_SETUP, USE_PAGER, and NEED_WORK_TREE take effect even if
 + * passed the -h option.
 + */
 +#define H_IS_NOT_HELP(13)

Yuck.  Let's think of a way to avoid this ugliness.

 @@ -278,7 +287,8 @@ static void handle_internal_command(int argc, const char 
 **argv)
   { annotate, cmd_annotate, RUN_SETUP },
   { apply, cmd_apply },
   { archive, cmd_archive },
 - { bisect--helper, cmd_bisect__helper, RUN_SETUP | 
 NEED_WORK_TREE },
 + { bisect--helper, cmd_bisect__helper,
 + RUN_SETUP | NEED_WORK_TREE },

Besides, this hunk is totally unwarranted.

Here are the relevant parts (some of your H_IS_NOT_HELP are not visible
because you needlessly wrapped the lines):

 + { cherry, cmd_cherry, RUN_SETUP | H_IS_NOT_HELP },
 + { commit-tree, cmd_commit_tree, RUN_SETUP | H_IS_NOT_HELP },
 + { fetch--tool, cmd_fetch__tool, RUN_SETUP | H_IS_NOT_HELP },
 + { grep, cmd_grep, RUN_SETUP | USE_PAGER | H_IS_NOT_HELP },
 + { merge-ours, cmd_merge_ours, RUN_SETUP | H_IS_NOT_HELP },
 + { merge-recursive, cmd_merge_recursive,
 + { merge-subtree, cmd_merge_recursive,
 + { show-ref, cmd_show_ref, RUN_SETUP | H_IS_NOT_HELP },

Except for grep and show-ref, none of these have a valid -h option
that means something else.

Considering that this niggle is strictly about git cmd -h, and not about
git cmd --otheropt -h somearg, we can even say that git grep -h is
asking for help, and not do not show filenames from match, as there is
no pattern specified.

So I think the right approach is something like how you handled http-push;
namely, check if the sole argument is -h, and if so show help and exit.

Clarification. the following description only talks about cmd -h
without any other options and arguments.

Such a change cannot be breaking backward compatibility for...

 * cherry -h could be asking to compare histories that leads to our HEAD
   and a commit that can be named as -h.  Strictly speaking, that may be
   a valid refname, but the user would have to say something like
   tags/-h to name such a pathological ref already, so I do not think it
   is such a big deal.

 * commit-tree -h is to make a root commit that records a tree-ish
   pointed by a tag whose name is -h.  Same as above.

 * The first word to fetch--tool is a subcommand name, so fetch--tool -h
   is an error and there cannot be any existing callers.  Besides, is it
   still being used?

 * grep -h cannot be asking for suppressing filenames as there is no
   match pattern specified.

 * merge-* strategy backends take the merge base (or --) as the first
   parameter; it cannot sanely be -h. The callers are supposed to run
   rev-parse to make it 40-hexdigit and the command won't see a refname
   anyway.

That leaves show-ref -h.  It shows all the refs/* and HEAD, as opposed
to show-ref that shows all the refs/* and not HEAD.

Does anybody use show-ref -h?  It was in Linus's original, and I suspect
it was done only because he thought it might be handy, not because the
command should not show the HEAD by default for such and such reasons.
So I think it actually is Ok if show-ref -h (but not show-ref --head)
gave help and exit.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#553296: gitignore broken completely

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Fri, Oct 30, 2009 at 02:41:55PM -0400, Jeff King wrote:

 6. Revert the patch and rework it so that it will only have effect if
there is no -i option on the command line. (That is similiar to a
mix of 3 and 4.)
 
 Yeah, that would actually be the least invasive change, and would keep
 -i just as it is. If we do anything except a simple, I think your (6)
 makes the most sense.
 
 Let me see if I can make a patch.

 Here it is. I think this is the right thing to do. Junio?

I am not sure; my head spins when I see tracked but ignored (you have
a new one in the test) which is quite a bogus concept.

Does it mean tracked but would be ignored if it weren't---perhaps it was
added by mistake??



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#553296: gitignore broken completely

[Junio C Hamano]

Jeff King p...@peff.net writes:

 I am not sure simply reverting is the best choice; the patch does do
 something useful. And while it strictly breaks backwards compatibility
 on the output without -i, the old behavior was considered a bug. But
 the -i behavior is useless now, so we need to figure out how to
 proceed. We can:

   1. Revert and accept that the behavior is historical. Callers need to
  work around it by dropping --exclude* when invoking ls-files.

   2. Declare -i useless, deprecate and/or remove. Obviously this is
  also breaking existing behavior, but again, I don't think that
  using -i actually accomplishes anything.

   3. Revert for now, and then reinstate the patch during a major version
  bump when we are declaring some compatibility breakages.

   4. Re-work -i to show tracked but ignored files, but still show all
  files when -i is not given at all.

 I think (4) preserves the benefit of the patch in question, but still
 allows your usage (git ls-files -i --exclude-standard). I do question
 whether that usage is worth supporting. Certainly I wouldn't implement
 it if I were writing git-ls-files from scratch today, but we do in
 general try to maintain backwards compatibility, especially for plumbing
 like ls-files.

 Junio, what do you want to do?

I've never understood the use of ls-files -i without -o, so in that
sense, I have done 2. myself already long time ago.

In other words, I do not really care that much, and the choice would be
between 0. do not do anything---the patch in question was a bugfix for
longstanding insanity and your 4. -i without -o didn't make much sense
but now it does and here is the new meaning.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#553296: gitignore broken completely

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Fri, Oct 30, 2009 at 12:41:27PM -0700, Junio C Hamano wrote:

 I've never understood the use of ls-files -i without -o, so in that
 sense, I have done 2. myself already long time ago.
 
 In other words, I do not really care that much, and the choice would be
 between 0. do not do anything---the patch in question was a bugfix for
 longstanding insanity and your 4. -i without -o didn't make much sense
 but now it does and here is the new meaning.

 OK, I think the patch I sent elsewhere in the thread should be applied,
 then, as it should make you, me, and Klaus happy.

Thanks; will queue on top of b5227d8.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Junio C Hamano]

Johannes Schindelin [EMAIL PROTECTED] writes:

 Hi,

 On Tue, 4 Mar 2008, David Bremner wrote:

 It looks like line 435 of builtin-commit.c disables stdin for hooks 
 (with the disclaimer that I first looked at the git source ten minutes 
 ago).
 
 hook.no_stdin = 1
 
 I'm not sure if this was by design, but just so you know, this breaks 
 people's hooks.  In particular the default metastore pre-commit hook no 
 longer works.  I didn't find anything relevant in the docs [1].

 Pardon my ignorance, but what business has metastore reading stdin?  There 
 should be nothing coming in, so the change you mentioned should be 
 correct, and your hook relies on something it should not rely on.

It is not metastore.  It is an interactive hook that reads from the user
who is sitting on the terminal and invoked the git-commit program.



-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Junio C Hamano]

Johannes Sixt [EMAIL PROTECTED] writes:

 It is not metastore.  It is an interactive hook that reads from the user
 who is sitting on the terminal and invoked the git-commit program.

 Are you saying stdin should not be directed to /dev/null, or that an
 interactive hook is required to do

 exec  /dev/tty || { echo 21 not interactive; exit 1; }

 before it reads from stdin?

I am saying that scripted version left the stdin as-is but somehow we
ended up spawning with .no_stdin = 1 in the C-rewrite, which is a change
in established behaviour.  It is often called a regression, unless the
change has a very good reason.  And I tend to think this particular one
falls into the former.

We should audit how the hooks are called from various commands
re-implemented, comparing the environment the scripted version used to
give them, which includes:

 - what directory the hook is run in;
 - what environment variables are exported to it;
 - what temporary files are visible to them for inspection;
 - in what order they are run;
 - which file descriptor is connected to what;

I think we already caught some of the environment and ordering issues in
commit and checkout, but I am far from confident to say that what we have
behave identically to the scripted version.



-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#427078: [PATCH] Create a new manpage for the gitignore format, and reference it elsewhere

[Junio C Hamano]

Josh Triplett [EMAIL PROTECTED] writes:

Thanks, but shouldn't all the in-text mention of gitignore(5)
and friends, not just in See Also section, use
gitlink:gitignore[5] instead?



-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#386495: git-apply fails to apply some patches

[Junio C Hamano]

Gerrit Pape [EMAIL PROTECTED] writes:

...
 On top of that, I try to apply this interdiff generated patch:

 diff -u pciutils-2.1.11/debian/dirs pciutils-2.1.11/debian/dirs
 --- pciutils-2.1.11/debian/dirs
 +++ pciutils-2.1.11/debian/dirs
 @@ -6,0 +7 @@
 +var/lib/pciutils

 and git-apply says:

 error: debian/dirs: already exists in working directory

 I suspect it's confused by the '-x,0' thinking that means file does not
 exist rather than we have 0 context for this diff.

There are a few safety checks we perform assuming that the patch
has a few lines of context, and this is falsely triggering one
of them.  It incorrectly thinks that seeing _no_ context nor old
lines in the patch means this is a file creation patch, and
complains because it knows the file being patched already
exists.

I am looking into a way to handle a context-less patch line the
one quoted, without breaking the sanity check we have.

In the meantime you should be able to work things around by not
feeding --unified=0 diff output.





-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#386495: [PATCH] apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches

[Junio C Hamano]

One request and one question:

 (1) I think the attached patch should fix this.  Could you
 see if it works for you?

 (2) Is it absolutely necessary to work with a context-free
 patch in the workflow of the particular job (pciutils
 maintenance)?  If so what is the reason?

-- 8 --
In git-apply, we have a few sanity checks and heuristics that
expects that the patch fed to us is a unified diff with at least
one line of context.

 * When there is no leading context line in a hunk, the hunk
   must apply at the beginning of the preimage.  Similarly, no
   trailing context means that the hunk is anchored at the end.

 * We learn a patch deletes the file from a hunk that has no
   resulting line (i.e. all lines are prefixed with '-') if it
   has not otherwise been known if the patch deletes the file.
   Similarly, no old line means the file is being created.

And we declare an error condition when the file created by a
creation patch already exists, and/or when a deletion patch
still leaves content in the file.

These sanity checks are good safety measures, but breaks down
when people feed a diff generated with --unified=0.  This was
recently noticed first by Matthew Wilcox and Gerrit Pape.

This adds a new flag, --unified-zero, to allow bypassing these
checks.  If you are in control of the patch generation process,
you should not use --unified=0 patch and fix it up with this
flag; rather you should try work with a patch with context.  But
if all you have to work with is a patch without context, this
flag may come handy as the last resort.

Signed-off-by: Junio C Hamano [EMAIL PROTECTED]
---
 builtin-apply.c   |  112 +++-
 t/t3403-rebase-skip.sh|4 +-
 t/t4104-apply-boundary.sh |  115 +
 3 files changed, 197 insertions(+), 34 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 6e0864c..25e90d8 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -27,6 +27,7 @@ static const char *prefix;
 static int prefix_length = -1;
 static int newfd = -1;
 
+static int unidiff_zero;
 static int p_value = 1;
 static int check_index;
 static int write_index;
@@ -854,11 +855,10 @@ static int find_header(char *line, unsig
 }
 
 /*
- * Parse a unified diff. Note that this really needs
- * to parse each fragment separately, since the only
- * way to know the difference between a --- that is
- * part of a patch, and a --- that starts the next
- * patch is to look at the line counts..
+ * Parse a unified diff. Note that this really needs to parse each
+ * fragment separately, since the only way to know the difference
+ * between a --- that is part of a patch, and a --- that starts
+ * the next patch is to look at the line counts..
  */
 static int parse_fragment(char *line, unsigned long size, struct patch *patch, 
struct fragment *fragment)
 {
@@ -875,31 +875,14 @@ static int parse_fragment(char *line, un
leading = 0;
trailing = 0;
 
-   if (patch-is_new  0) {
-   patch-is_new =  !oldlines;
-   if (!oldlines)
-   patch-old_name = NULL;
-   }
-   if (patch-is_delete  0) {
-   patch-is_delete = !newlines;
-   if (!newlines)
-   patch-new_name = NULL;
-   }
-
-   if (patch-is_new  oldlines)
-   return error(new file depends on old contents);
-   if (patch-is_delete != !newlines) {
-   if (newlines)
-   return error(deleted file still has contents);
-   fprintf(stderr, ** warning: file %s becomes empty but is not 
deleted\n, patch-new_name);
-   }
-
/* Parse the thing.. */
line += len;
size -= len;
linenr++;
added = deleted = 0;
-   for (offset = len; size  0; offset += len, size -= len, line += len, 
linenr++) {
+   for (offset = len;
+0  size;
+offset += len, size -= len, line += len, linenr++) {
if (!oldlines  !newlines)
break;
len = linelen(line, size);
@@ -972,12 +955,18 @@ static int parse_fragment(char *line, un
 
patch-lines_added += added;
patch-lines_deleted += deleted;
+
+   if (0  patch-is_new  oldlines)
+   return error(new file depends on old contents);
+   if (0  patch-is_delete  newlines)
+   return error(deleted file still has contents);
return offset;
 }
 
 static int parse_single_patch(char *line, unsigned long size, struct patch 
*patch)
 {
unsigned long offset = 0;
+   unsigned long oldlines = 0, newlines = 0, context = 0;
struct fragment **fragp = patch-fragments;
 
while (size  4  !memcmp(line, @@ -, 4)) {
@@ -988,9 +977,11 @@ static int parse_single_patch(char *line
len = parse_fragment(line, size, patch, fragment);
if (len = 0)
die