Re: [PATCH 2/4] Teach git-add--interactive to accept a file path to patch

From: Wincent Colaiuta <win@wincent.com>
Date: Nov 22, 2007, 13:34 UTC
Message-ID: <FDAFAD8C-8FF5-4DD7-AC5E-BA39790281A4@wincent.com>
In-Reply-To: <7vlk8q5xj4.fsf@gitster.siamese.dyndns.org>

El 22/11/2007, a las 12:29, Junio C Hamano escribió:

Show 16 quoted lines

> It is quite valid for users to run:
>
> 	git add -i \*.sh
>
> and be able to choose from a list which paths to stage (as a
> whole), as well as choose from a list which files to run the
> per-hunk staging interface.  "git add \*.sh" won't give you any
> chance to choose which ones to stage, and that's what we have
> "-i" (interactive) mode in "git add" for.
>
> I think you can massage "git add --partial foo" given by the
> user internally into "git-add--interactive -i --patch foo".  I
> strongly suspect that "direct to patch subcommand" mode needs
> more than just initially jumping into the subcommand (for
> example, you would want to exit when the patch selection
> interaction ends, without going back to the main menu)

This will become especially important if we decide to allow "git commit --interactive" to accept a path spec as well (although exactly when we should exit isn't immediately clear to me); I didn't touch this for the time-being because builtin-commit is getting very close to replacing git-commit.sh.

Show 9 quoted lines

> and we
> would want a signal stronger than mere presense of pathspecs to
> trigger such a specialized behaviour.
>
> By the way, the arguments on the command line to git commands
> after "--" are generally pathspecs, iow, patterns to specify
> groups of files.  Please do not introduce unnecessary
> inconsistencies to the UI by requiring them to be exact pathname
> only in this particular mode of the command and nowhere else.

Well, I it wasn't my intention to introduce any such requirement. The path parameters get passed in and eventually handed over unmodified to:

	git diff-files -p --

Which was what was previously in the parse_diff function. It turns out that if you pass something like \*.sh to git-diff-files then it won't match, but it's not a restriction that I chose to impose, rather it's a consequence of the way git-diff-files works. Are there options we could pass to avoid that restriction?

In any case, one good thing to come out of this discussion is the ability to use pathspecs to limit the number of files displayed when in interactive mode. But it wasn't what I was originally aiming for. My initial goal was to have git-add--interactive do something useful and convenient when you type:

	git add -i foo

If we don't jump straight to the subcommand then the user has to:

	- press 5 or p, then Enter
	- press 1, then Enter

This was easier than it was before (same number of keystrokes, but fewer files to visually scan in the list of candidates), but it's still not that much easier.

Or if we add some kind of "--patch" switch, instead do:

	git add -i --patch foo

Once again, not that easy but you could always set up a Git alias, I guess, as a shortcut. Not sure whether this is offers enough of a workflow improvement to make it worthwhile (ie. you may as well just fire up git-gui).

Show 14 quoted lines

> There was one funny thing I fixed up.  The arguments to the
> interactive_add() function in builtin-add.c was like this:
>
> 	int interactive_add(const char **argv, int argc)
>
> Anybody who writes a function with such a signature and do not
> notice its craziness before sending it out either (1) has never
> programmed in C, (2) did not review the code before submitting,
> or (3) worked too hard and was too tired.
>
> I suspect, judging from the timestamp of your message, it was
> (3) this time.  The collaborative development is not a race ---
> don't work too hastily and too hard; please relax and review
> after a good night's sleep before sending things out.

In any case I've rebased and squished the changes down to a nice 4- patch series, incorporating all the feedback given so far. Will sit on it a while before sending it out, as you suggest. But I don't really have a clear idea where to go forward from here.

Cheers, Wincent