Re: [PATCH 2/2] reset: support the --stdin option

[Johannes Schindelin]

Hi Junio,

On Tue, 11 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > +   if (read_from_stdin) {
> > +   strbuf_getline_fn getline_fn = nul_term_line ?
> > +   strbuf_getline_nul : strbuf_getline_lf;
> > +   int flags = PATHSPEC_PREFER_FULL |
> > +   PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP;
> > +   struct strbuf buf = STRBUF_INIT;
> > +   struct strbuf unquoted = STRBUF_INIT;
> > +
> > +   if (patch_mode)
> > +   die(_("--stdin is incompatible with --patch"));
> > +
> > +   if (pathspec.nr)
> > +   die(_("--stdin is incompatible with path arguments"));
> > +
> > +   if (patch_mode)
> > +   flags |= PATHSPEC_PREFIX_ORIGIN;
> 
> Didn't we already die above under that mode?

Oh right. Copy/paste fail.

> > +   while (getline_fn(, stdin) != EOF) {
> > +   if (!nul_term_line && buf.buf[0] == '"') {
> > +   strbuf_reset();
> > +   if (unquote_c_style(, buf.buf, NULL))
> > +   die(_("line is badly quoted"));
> > +   strbuf_swap(, );
> > +   }
> > +   ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> > +   stdin_paths[stdin_nr++] = xstrdup(buf.buf);
> > +   strbuf_reset();
> > +   }
> > +   strbuf_release();
> > +   strbuf_release();
> > +
> > +   ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> > +   stdin_paths[stdin_nr++] = NULL;
> 
> It makes sense to collect, but...

It does, doesn't it? I really would have loved to start resetting right
away, but if the list were not sorted and traversed at the same time as
the tree-ish, the performance would just be suboptimal.

I think that is an important point and I adjusted the commit message
accordingly.

> > +   parse_pathspec(, 0, flags, prefix,
> > +  (const char **)stdin_paths);
> 
> ...letting them be used as if they are pathspec is wrong when
> stdin_paths[] contain wildcard, isn't it?  
> 
> I think flags |= PATHSPEC_LITERAL_PATH can help fixing it.  0/2 said
> this mimicks checkout-index and I think it should by not treating
> the input as wildcarded patterns (i.e. "echo '*.c' | reset --stdin"
> shouldn't be the way to reset all .c files --- that's something we
> would want to add to the test, I guess).

True. I adjust the flags accordingly now.

Thanks,
Dscho

Re: [PATCH 2/2] reset: support the --stdin option

[Johannes Schindelin]

Hi Kuba,

On Tue, 11 Oct 2016, Jakub Narębski wrote:

> W dniu 11.10.2016 o 18:09, Johannes Schindelin pisze:
> 
> >  SYNOPSIS
> >  
> >  [verse]
> > -'git reset' [-q] [] [--] ...
> > +'git reset' [-q] [--stdin [-z]] [] [--] ...
> 
> I think you meant here
> 
>   +'git reset' [-q] [--stdin [-z]] []

Good point. I overlooked that the ... are not optional here.

Thanks,
Dscho

Re: [PATCH 2/2] reset: support the --stdin option

[Jakub Narębski]

W dniu 11.10.2016 o 18:09, Johannes Schindelin pisze:

>  SYNOPSIS
>  
>  [verse]
> -'git reset' [-q] [] [--] ...
> +'git reset' [-q] [--stdin [-z]] [] [--] ...

I think you meant here

  +'git reset' [-q] [--stdin [-z]] []

Because you say "*Instead*" below.

> +--stdin::
> + Instead of taking list of paths from the command line,
> + read list of paths from the standard input.  Paths are
> + separated by LF (i.e. one path per line) by default.

And die if  were supplied:

> + if (pathspec.nr)
> + die(_("--stdin is incompatible with path arguments"));

Of course you need to fix it in built-in synopsis as well:

> + N_("git reset [-q] [--stdin [-z]] [] [--] ..."),
>   N_("git reset --patch [] [--] [...]"),

-- 
Jakub Narębski

Re: [PATCH 2/2] reset: support the --stdin option

[Junio C Hamano]

Johannes Schindelin  writes:

> + if (read_from_stdin) {
> + strbuf_getline_fn getline_fn = nul_term_line ?
> + strbuf_getline_nul : strbuf_getline_lf;
> + int flags = PATHSPEC_PREFER_FULL |
> + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP;
> + struct strbuf buf = STRBUF_INIT;
> + struct strbuf unquoted = STRBUF_INIT;
> +
> + if (patch_mode)
> + die(_("--stdin is incompatible with --patch"));
> +
> + if (pathspec.nr)
> + die(_("--stdin is incompatible with path arguments"));
> +
> + if (patch_mode)
> + flags |= PATHSPEC_PREFIX_ORIGIN;

Didn't we already die above under that mode?

> + while (getline_fn(, stdin) != EOF) {
> + if (!nul_term_line && buf.buf[0] == '"') {
> + strbuf_reset();
> + if (unquote_c_style(, buf.buf, NULL))
> + die(_("line is badly quoted"));
> + strbuf_swap(, );
> + }
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> + stdin_paths[stdin_nr++] = xstrdup(buf.buf);
> + strbuf_reset();
> + }
> + strbuf_release();
> + strbuf_release();
> +
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> + stdin_paths[stdin_nr++] = NULL;

It makes sense to collect, but...

> + parse_pathspec(, 0, flags, prefix,
> +(const char **)stdin_paths);

...letting them be used as if they are pathspec is wrong when
stdin_paths[] contain wildcard, isn't it?  

I think flags |= PATHSPEC_LITERAL_PATH can help fixing it.  0/2 said
this mimicks checkout-index and I think it should by not treating
the input as wildcarded patterns (i.e. "echo '*.c' | reset --stdin"
shouldn't be the way to reset all .c files --- that's something we
would want to add to the test, I guess).

Thanks.